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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.


> 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.


> 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.

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