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

[Julien Grall <julien.grall@xxxxxxx>]

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

--
Julien Grall

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