[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v9 12/28] ARM: vGIC: advertise LPI support
Hi Stefano,
On 25/05/2017 19:49, Stefano Stabellini wrote:
On Thu, 25 May 2017, Andre Przywara wrote:
Hi,
On 23/05/17 18:47, 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:
@@ -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?
The domain lock (taken by vgic_lock()) protects rdists_enabled. This
variable stores whether at least one redistributor has LPIs enabled. In
this case the property table gets into use and since the table is shared
across all redistributors, we must not change it anymore, even on
another redistributor which has its LPIs still disabled.
So while this looks like this is a per-redistributor (=per-VCPU)
property, it is actually per domain, hence this lock.
The VGIC VCPU lock is then used to naturally protect the enable bit
against multiple VCPUs accessing this register simultaneously - the
redists are MMIO mapped, but not banked, so this is possible.
Does that make sense?
If the VGIC VCPU lock is only used to protect VGIC_V3_LPIS_ENABLED,
couldn't we just read/write the bit atomically? It's just a bit after
all, it doesn't need a lock.
The vGIC vCPU lock is also here to serialize access to the
re-distributor state when necessary.
For instance you don't want to allow write in PENDBASER after LPIs have
been enabled.
If you don't take the lock here, you would have a small race where
PENDBASER might be written whilst the LPIs are getting enabled.
The code in PENDBASER today does not strictly require the locking, but I
think we should keep the lock around. Moving to the atomic will not
really benefit here as write to those registers will be very unlikely so
we don't need very good performance.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
