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@lists.xen.org https://lists.xen.org/xen-devel