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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

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