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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 05/23/2017 06:47 PM, 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:
Oh, though my point about vgic_lock naming stands :).

> @@ -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?
> If so, why?
I will let Andre confirm here.

Cheers,

--
Julien Grall

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