Re: [Xen-devel] [PATCH v10 25/32] ARM: vITS: handle MAPTI/MAPI command

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 02/06/17 18:12, Julien Grall wrote:
> Hi Andre,
> 
> On 05/26/2017 06:35 PM, Andre Przywara wrote:
>> The MAPTI commands associates a DeviceID/EventID pair with a LPI/CPU
>> pair and actually instantiates LPI interrupts. MAPI is just a variant
>> of this comment, where the LPI ID is the same as the event ID.
>> We connect the already allocated host LPI to this virtual LPI, so that
>> any triggering LPI on the host can be quickly forwarded to a guest.
>> Beside entering the domain and the virtual LPI number in the respective
>> host LPI entry, we also initialize and add the already allocated
>> struct pending_irq to our radix tree, so that we can now easily find it
>> by its virtual LPI number.
>> We also read the property table to update the enabled bit and the
>> priority for our new LPI, as we might have missed this during an earlier
>> INVALL call (which only checks mapped LPIs). But we make sure that the
>> property table is actually valid, as all redistributors might still
>> be disabled at this point.
>> Since write_itte_locked() now sees its first usage, we change the
>> declaration to static.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>>   xen/arch/arm/gic-v3-its.c        |  27 ++++++++
>>   xen/arch/arm/vgic-v3-its.c       | 138
>> ++++++++++++++++++++++++++++++++++++++-
>>   xen/include/asm-arm/gic_v3_its.h |   3 +
>>   3 files changed, 165 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> index 8864e0b..41fff64 100644
>> --- a/xen/arch/arm/gic-v3-its.c
>> +++ b/xen/arch/arm/gic-v3-its.c
>> @@ -876,6 +876,33 @@ int gicv3_remove_guest_event(struct domain *d,
>> paddr_t vdoorbell_address,
>>       return 0;
>>   }
>>   +/*
>> + * Connects the event ID for an already assigned device to the given
>> VCPU/vLPI
>> + * pair. The corresponding physical LPI is already mapped on the host
>> side
>> + * (when assigning the physical device to the guest), so we just
>> connect the
>> + * target VCPU/vLPI pair to that interrupt to inject it properly if
>> it fires.
>> + * Returns a pointer to the already allocated struct pending_irq that is
>> + * meant to be used by that event.
>> + */
>> +struct pending_irq *gicv3_assign_guest_event(struct domain *d,
>> +                                             paddr_t vdoorbell_address,
>> +                                             uint32_t vdevid,
>> uint32_t eventid,
>> +                                             uint32_t virt_lpi)
>> +{
>> +    struct pending_irq *pirq;
>> +    uint32_t host_lpi = 0;
> This should be INVALID_LPI and not 0.
> 
> [...]
> 
>> +/*
>> + * For a given virtual LPI read the enabled bit and priority from the
>> virtual
>> + * property table and update the virtual IRQ's state in the given
>> pending_irq.
>> + * Must be called with the respective VGIC VCPU lock held.
>> + */
>> +static int update_lpi_property(struct domain *d, struct pending_irq *p)
>> +{
>> +    paddr_t addr;
>> +    uint8_t property;
>> +    int ret;
>> +
>> +    /*
>> +     * If no redistributor has its LPIs enabled yet, we can't access the
>> +     * property table. In this case we just can't update the properties,
>> +     * but this should not be an error from an ITS point of view.
>> +     */
>> +    if ( !read_atomic(&d->arch.vgic.rdists_enabled) )
>> +        return 0;

I was just looking at rdists_enabled, and think that using read_atomic()
is a red herring.
First rdists_enabled is a bool, so I have a hard time to imagine how it
could be read non-atomically.
I think the intention of making this read "somewhat special" was to
cater for the fact that we write it under the domain lock, but read it
here without taking it. But I think for this case we don't need any
special read version, and anyway an *atomic* read would not help here.

What we want is to make sure that rdist_propbase is valid before we see
rdists_enabled gets true, this is what this check here is for. This
should be solved by a write barrier between the two on the other side.

Looking at Linux' memory_barriers.txt my understanding is that the
matching barrier on the read side does not necessarily need to be an
explicit barrier instruction, it could be a control flow dependency as
well. And here we have that: we check rdists_enabled and bail out if
it's not set, so neither the compiler nor the CPU can reorder this (as
this would violate program semantics).

Also rdists_enabled is a bit special in that it never gets reset once it
became true, very much like the LPI enablement in the GICv3 spec.

So I think we can really go with a normal read plus a comment.

Does that make sense?

Cheers,
Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel