On 26/10/2022 14:41, Julien Grall wrote:
Hi Ayan,
Hi Julien,
On 26/10/2022 14:35, Ayan Kumar Halder wrote:
If a guest is running in 32 bit mode and it tries to access
"GICR_PENDBASER + 4" mmio reg, it will be trapped to Xen.
vreg_reg64_extract()
will return the value stored "v->arch.vgic.rdist_pendbase + 4".
This will be stored in a 64bit cpu register.
>
So now we have the top 32 bits of GICR_PENDBASER (a 64 bit MMIO
register) stored
in the lower 32 bits of the 64bit cpu register.
This 64bit cpu register is then modified bitwise with a mask (ie
GICR_PENDBASER_PTZ, it clears the 62nd bit). But the PTZ (which is
bit 30 in the
64 bit cpu register) is not cleared as expected by the specification.
The correct thing to do here is to store the value of
"v->arch.vgic.rdist_pendbase" in a temporary 64 bit variable. This
variable is
then modified bitwise with GICR_PENDBASER_PTZ mask. It is then passed to
vreg_reg64_extract() which will extract 32 bits from the given offset.
Fixes: fe7fa1332dabd9ce4 ("ARM: vGICv3: handle virtual LPI pending
and property tables")
Signed-off-by: Ayan Kumar Halder <ayank...@amd.com>
Reviewed-by: Bertrand Marquis <bertrand.marq...@arm.com>
Reviewed-by: Andre Przywara <andre.przyw...@arm.com>
Given the changes you made below, the reviewed-by tags below should
not have been retained.
Sorry, I will take care of this henceforth.
Release-acked-by: Henry Wang <henry.w...@arm.com>
---
Changes from:-
v1 - 1. Extracted this fix from "[RFC PATCH v1 05/12] Arm: GICv3:
Emulate
GICR_PENDBASER and GICR_PROPBASER on AArch32" into a separate patch
with an
appropriate commit message.
v2 - 1. Removed spin_lock_irqsave(). Used read_atomic() to read
v->arch.vgic.rdist_pendbase in an atomic context.
Please in the commit message why the lock is removed. But...
ok
2. Rectified the commit message to state that the cpu register is 64
bit.
(because currently, GICv3 is supported on Arm64 only). Reworded to
make it
clear.
xen/arch/arm/vgic-v3.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 0c23f6df9d..958af1532e 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -249,16 +249,16 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct
vcpu *v, mmio_info_t *info,
case VREG64(GICR_PENDBASER):
{
- unsigned long flags;
+ uint64_t val;
if ( !v->domain->arch.vgic.has_its )
goto read_as_zero_64;
if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
- spin_lock_irqsave(&v->arch.vgic.lock, flags);
- *r = vreg_reg64_extract(v->arch.vgic.rdist_pendbase, info);
- *r &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */
- spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+ val = read_atomic(&v->arch.vgic.rdist_pendbase);
... you also need to ensure that the writers are atomically setting
rdist_pendbase. Please correct if I am wrong, but the callers are not
using write_atomic(). So how does that work?
I think read_atomic()/write_atomic() may not be the correct approach for
the following reasons :-
1. __vgic_v3_rdistr_rd_mmio_read is a static function. So 'val' has a
global lifetime. Thus, all the following three lines need to be
protected from concurrent access.
val = read_atomic(&v->arch.vgic.rdist_pendbase);
val &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */
/* If a context switch happens here, then the 'val' below may
potentially be incorrect. */
*r = vreg_reg64_extract(val, info);
2. The same holds true for 'reg' as well in __vgic_v3_rdistr_rd_mmio_write()
reg = v->arch.vgic.rdist_pendbase;
blah, blah
v->arch.vgic.rdist_pendbase = reg;
Thus, I am thinking of going back to
spin_lock_irqsave()/spin_unlock_irqrestore(). Unless, you have some
other opinions.
- Ayan
Cheers,
