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

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 09/06/17 20:14, Stefano Stabellini wrote:
> On Fri, 9 Jun 2017, Andre Przywara wrote:
>> 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.
> 
> This is not a good argument, because if we want the read to be atomic,
> then we need to be using one of the _atomic functions regardless of the
> type.

OK, I see your point there. For the records (and to explain my ignorance
;-) I think it applies to a strict C standard point of view only. For
all practical means I think a read into a variable of a native data type
(especially that of a 1-byte sized bool) is always atomic on arm and
arm64 - especially with the load/store architecture of ARM. Also I have
a hard time to imagine intermediate values for a bool ;-), especially
since rdist_enabled only goes from false to true once in a domain's
lifetime.
But nevertheless I can see and agree that to be C standard compliant we
should use read_atomic().
Which makes me wonder if that would be true for other places in the code
as well ...

Another point of confusion may be that read_atomic() on its own does not
seem to be enough here, since we also need the barrier mechanism, maybe
even ACCESS_ONCE. But as I mentioned below this should be covered by the
control flow guarantee is this case.

I wonder if we should brainstorm if the usage of the atomic operations,
the barriers and ACCESS_ONCE is really correct in the current Xen code.
Comparing those to their Linux counterparts at least show some differences.

>> 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.
> 
> I haven't looked at the specific of rdists_enabled in this
> implementaion, but be aware that in general writing a variable under a
> lock, and reading it atomically is not safe. You either read and write
> under a lock, or read and write atomically.

Agreed. rdists_enabled may be special here because it's a bool and only
goes from false (initialized value) to true once (there is only one
rdists_enabled assignment, which sets it to true).

>> 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).
> 
> I think that this is true.
> 
> 
>> 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?
> 
> The first motivation isn't right, but I think that the latter
> explanation makes sense.

OK, I think I agree with that.

Cheers,
Andre.

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