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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?


> > +        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;
> > +    }
> > 
> >      case VREG32(GICR_IIDR):
> >          /* RO */

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