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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?
If so, why?

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