| Message ID | 20221026133540.52191-1-ayankuma@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN,v3] xen/arm: vGICv3: Emulate properly 32-bit access on GICR_PENDBASER | expand |
Hi Ayan, 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 <ayankuma@amd.com> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> Given the changes you made below, the reviewed-by tags below should not have been retained. > Release-acked-by: Henry Wang <Henry.Wang@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... > 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? Cheers,
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 <ayankuma@amd.com> >> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> >> Reviewed-by: Andre Przywara <andre.przywara@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.Wang@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, >
On 26/10/2022 16:06, Ayan Kumar Halder wrote: >> >> ... 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. I don't understand this argument. 'static' means the function is not exported. The local variables will still reside on the stack. So why does the use of 'val' needs to be protected with the lock? > > 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; Same here. Cheers,
Hi Julien, On 26/10/2022 17:45, Julien Grall wrote: > > > On 26/10/2022 16:06, Ayan Kumar Halder wrote: >>> >>> ... 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. > > I don't understand this argument. 'static' means the function is not > exported. The local variables will still reside on the stack. > > So why does the use of 'val' needs to be protected with the lock? Yes, you are correct. I was misunderstanding this as a static variable. Also, I understood from Stefano that pre-emption does not occur in Xen. So there will be no context switch. So, the only race is between __vgic_v3_rdistr_rd_mmio_read() and __vgic_v3_rdistr_rd_mmio_write() for reading/writing rdist_pendbase. - Ayan > >> >> 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; > > Same here. > > Cheers, >
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); + val = v->arch.vgic.rdist_pendbase; + val &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ + *r = vreg_reg64_extract(val, info); return 1; }
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 <ayankuma@amd.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Reviewed-by: Andre Przywara <andre.przywara@arm.com> Release-acked-by: Henry Wang <Henry.Wang@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. 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(-)