Re: [Xen-devel] [PATCH v9 12/28] ARM: vGIC: advertise LPI support

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 25 May 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 25/05/2017 19:49, Stefano Stabellini wrote:
> > On Thu, 25 May 2017, Andre Przywara wrote:
> > > Hi,
> > > 
> > > On 23/05/17 18:47, Stefano Stabellini wrote:
> > > > On Tue, 23 May 2017, Julien Grall wrote:
> > > > > Hi Stefano,
> > > > > 
> > > > > On 22/05/17 23:19, Stefano Stabellini wrote:
> > > > > > On Tue, 16 May 2017, Julien Grall wrote:
> > > > > > > > @@ -436,8 +473,26 @@ static int
> > > > > > > > __vgic_v3_rdistr_rd_mmio_write(struct
> > > > > > > > vcpu
> > > > > > > > *v, mmio_info_t *info,
> > > > > > > >      switch ( gicr_reg )
> > > > > > > >      {
> > > > > > > >      case VREG32(GICR_CTLR):
> > > > > > > > -        /* LPI's not implemented */
> > > > > > > > -        goto write_ignore_32;
> > > > > > > > +    {
> > > > > > > > +        unsigned long flags;
> > > > > > > > +
> > > > > > > > +        if ( !v->domain->arch.vgic.has_its )
> > > > > > > > +            goto write_ignore_32;
> > > > > > > > +        if ( dabt.size != DABT_WORD ) goto bad_width;
> > > > > > > > +
> > > > > > > > +        vgic_lock(v);                   /* protects
> > > > > > > > rdists_enabled */
> > > > > > > 
> > > > > > > Getting back to the locking. I don't see any place where we get
> > > > > > > the domain
> > > > > > > vgic lock before vCPU vgic lock. So this raises the question why
> > > > > > > this
> > > > > > > ordering
> > > > > > > and not moving this lock into vgic_vcpu_enable_lpis.
> > > > > > > 
> > > > > > > At least this require documentation in the code and explanation in
> > > > > > > the
> > > > > > > commit
> > > > > > > message.
> > > > > > 
> > > > > > It doesn't look like we need to take the v->arch.vgic.lock here.
> > > > > > What is
> > > > > > it protecting?
> > > > > 
> > > > > The name of the function is a bit confusion. It does not take the vCPU
> > > > > vgic
> > > > > lock but the domain vgic lock.
> > > > > 
> > > > > I believe the vcpu is passed to avoid have v->domain in most of the
> > > > > callers.
> > > > > But we should probably rename the function.
> > > > > 
> > > > > In this case it protects vgic_vcpu_enable_lpis because you can
> > > > > configure the
> > > > > number of LPIs per re-distributor but this is a domain wide value. I
> > > > > know the
> > > > > spec is confusing on this.
> > > > 
> > > > The quoting here is very unhelpful. In Andre's patch:
> > > > 
> > > > @@ -436,8 +473,26 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct
> > > > vcpu *v, mmio_info_t *info,
> > > >      switch ( gicr_reg )
> > > >      {
> > > >      case VREG32(GICR_CTLR):
> > > > -        /* LPI's not implemented */
> > > > -        goto write_ignore_32;
> > > > +    {
> > > > +        unsigned long flags;
> > > > +
> > > > +        if ( !v->domain->arch.vgic.has_its )
> > > > +            goto write_ignore_32;
> > > > +        if ( dabt.size != DABT_WORD ) goto bad_width;
> > > > +
> > > > +        vgic_lock(v);                   /* protects rdists_enabled */
> > > > +        spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > > > +
> > > > +        /* LPIs can only be enabled once, but never disabled again. */
> > > > +        if ( (r & GICR_CTLR_ENABLE_LPIS) &&
> > > > +             !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) )
> > > > +            vgic_vcpu_enable_lpis(v);
> > > > +
> > > > +        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > > > +        vgic_unlock(v);
> > > > +
> > > > +        return 1;
> > > > +    }
> > > > 
> > > > My question is: do we need to take both vgic_lock and v->arch.vgic.lock?
> > > 
> > > The domain lock (taken by vgic_lock()) protects rdists_enabled. This
> > > variable stores whether at least one redistributor has LPIs enabled. In
> > > this case the property table gets into use and since the table is shared
> > > across all redistributors, we must not change it anymore, even on
> > > another redistributor which has its LPIs still disabled.
> > > So while this looks like this is a per-redistributor (=per-VCPU)
> > > property, it is actually per domain, hence this lock.
> > > The VGIC VCPU lock is then used to naturally protect the enable bit
> > > against multiple VCPUs accessing this register simultaneously - the
> > > redists are MMIO mapped, but not banked, so this is possible.
> > > 
> > > Does that make sense?
> > 
> > If the VGIC VCPU lock is only used to protect VGIC_V3_LPIS_ENABLED,
> > couldn't we just read/write the bit atomically? It's just a bit after
> > all, it doesn't need a lock.
> 
> The vGIC vCPU lock is also here to serialize access to the re-distributor
> state when necessary.
> 
> For instance you don't want to allow write in PENDBASER after LPIs have been
> enabled.
> 
> If you don't take the lock here, you would have a small race where PENDBASER
> might be written whilst the LPIs are getting enabled.
> 
> The code in PENDBASER today does not strictly require the locking, but I think
> we should keep the lock around. Moving to the atomic will not really benefit
> here as write to those registers will be very unlikely so we don't need very
> good performance.

I suggested the atomic as a way to replace the lock, to reduce the
number of lock order dependencies, rather than for performance (who
cares about performance for this case). If all accesses to
VGIC_V3_LPIS_ENABLED are atomic, then we wouldn't need the lock. 

Another maybe simpler way to keep the vgic vcpu lock but avoid
introducing the vgic domain lock -> vgic vcpu lock dependency (the less
the better) would be to take the vgic vcpu lock first, release it, then
take the vgic domain lock and call vgic_vcpu_enable_lpis after.  In
pseudo-code:

    vgic vcpu lock
    read old value of VGIC_V3_LPIS_ENABLED
    write new value of VGIC_V3_LPIS_ENABLED
    vgic vcpu unlock

    vgic domain lock
    vgic_vcpu_enable_lpis (minus the setting of arch.vgic.flags)
    vgic domain unlock

It doesn't look like we need to set VGIC_V3_LPIS_ENABLED within
vgic_vcpu_enable_lpis, so this seems to be working. What do you think?

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