[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/17 22:05, Stefano Stabellini wrote:
On Thu, 25 May 2017, Julien Grall wrote:
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.
I suggested the atomic as a way to replace the lock, to reduce the
number of lock order dependencies, rather than for performance (who
cares about performance for this case). If all accesses to
VGIC_V3_LPIS_ENABLED are atomic, then we wouldn't need the lock.
Another maybe simpler way to keep the vgic vcpu lock but avoid
introducing the vgic domain lock -> vgic vcpu lock dependency (the less
the better) would be to take the vgic vcpu lock first, release it, then
take the vgic domain lock and call vgic_vcpu_enable_lpis after. In
pseudo-code:
vgic vcpu lock
read old value of VGIC_V3_LPIS_ENABLED
write new value of VGIC_V3_LPIS_ENABLED
vgic vcpu unlock
vgic domain lock
vgic_vcpu_enable_lpis (minus the setting of arch.vgic.flags)
vgic domain unlock
It doesn't look like we need to set VGIC_V3_LPIS_ENABLED within
vgic_vcpu_enable_lpis, so this seems to be working. What do you think?
From a point of view of the vGIC you want to enable
VGIC_V3_LPIS_ENABLED after all the sanity checks have been done.
I would have expected the ITS to check if the redistributor has been
enabled before enabling it (see vgic_v3_verify_its_status). This is
because the ITS is using the priority table and also the number of LPIs.
So you effectively want to have VGIC_V3_LPIS_ENABLED set after in
vgic_vcpu_enable_lpis to avoid potential race condition. You may also
want to have a mb() before writing to it so you can using
VGIC_V3_LPIS_ENABLED without any lock safely.
Andre, can you explain why the ITS does not check whether the
rdists_enabled are enabled?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
