| Message ID | 20221024193002.57888-1-ayankuma@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN,v2] GICv3: Emulate GICR_PENDBASER correctly for 32 bit guests | expand |
On Mon, 24 Oct 2022 20:30:02 +0100 Ayan Kumar Halder <ayankuma@amd.com> wrote: Hi, > 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 32bit register. > > The 32bit register is then modified bitwise with a mask (ie GICR_PENDBASER_PTZ, > it clears the 62nd bit) which is greater than 32 bits. This will give an > incorrect result. > > 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> Indeed, the patch looks good to me. Also checked the other users of vreg_reg64_extract(), they seem to be all correct, by first building the value, then running the extract function on the final result. Reviewed-by: Andre Przywara <andre.przywara@arm.com> Cheers, Andre > --- > > 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. > > xen/arch/arm/vgic-v3.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index 0c23f6df9d..7930ab6330 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -250,14 +250,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 */ > + val = v->arch.vgic.rdist_pendbase; > + val &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ > + *r = vreg_reg64_extract(val, info); > spin_unlock_irqrestore(&v->arch.vgic.lock, flags); > return 1; > }
Hi Ayan, > On 24 Oct 2022, at 20:30, Ayan Kumar Halder <ayankuma@amd.com> 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 32bit register. > > The 32bit register is then modified bitwise with a mask (ie GICR_PENDBASER_PTZ, > it clears the 62nd bit) which is greater than 32 bits. This will give an > incorrect result. > > 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> Looks good to me to. Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > --- > > 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. > > xen/arch/arm/vgic-v3.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index 0c23f6df9d..7930ab6330 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -250,14 +250,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 */ > + val = v->arch.vgic.rdist_pendbase; > + val &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ > + *r = vreg_reg64_extract(val, info); > spin_unlock_irqrestore(&v->arch.vgic.lock, flags); > return 1; > } > -- > 2.17.1 >
(+Henry) Hi Ayan, Adding Henry because this is something we probably want to fix in Xen 4.17. AFAIU, the value is not used at all in Xen, so the risk is mostly returning a wrong value to a domain using GICv3 ITS (not officially supported and only expose to dom0 so far). Therefore, I would say this should be OK to ingest in Xen 4.17. Anyway back to the review... Title: Technically a 64-bit guest is equally affected because it is allowed to do 32-bit access. Furthermore, I would prefer if you prefix the title with "xen/arm" so it is clear which component you are touching. So I would suggest the following title: xen/arm: vGICv3: Emulate properly 32-bit access on GICR_PENDBASER Note that I appended a "v" to "GICv3" to make clear this is referring to the virtual GICv3 rather than host GICv3. On 24/10/2022 20:30, 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 32bit register. Not really, the result will be stored in a 64-bit register for AArch64 host (even if for 32-bit guest). The main part is the top 32-bit of PENDBASER will be returned in the low 32-bit. > > The 32bit register is then modified bitwise with a mask (ie GICR_PENDBASER_PTZ, > it clears the 62nd bit) which is greater than 32 bits. This will give an > incorrect result. I would be explicit and say that "the bit PTZ will not be 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> > --- > > 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. > > xen/arch/arm/vgic-v3.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index 0c23f6df9d..7930ab6330 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -250,14 +250,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 */ > + val = v->arch.vgic.rdist_pendbase; > + val &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ > + *r = vreg_reg64_extract(val, info); As you store v->arch.vgic.rdist_pendbase, the lock can be reduced to the first assignment. IOW: val = v->arch....; spin_unlock_irq_restore(); val &= ~GIC_PENDBASER_PTZ; That said, I think the lock could now be completely dropped in favor of using read_atomic() (and write_atomic()). I am saying this even with in mind that, IIUC, R52 may not support 64-bit atomics (see [1]). There are a few places in Xen that expect 64-bit access to be atomic. For instance the IOREQ code is using guest_cmpxchg64() and a 32-build of Xen with the "case 8" commented write_atomic_size() will fail. So there are some rework necessary for R52. I would also rather not want to keep a bigger hammer (i.e. the lock) for architecture that support 64-bit atomic access. Cheers, [1] 20221025145506.5708839c@donnerap.cambridge.arm.com
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Subject: Re: [XEN v2] GICv3: Emulate GICR_PENDBASER correctly for 32 bit > guests > > (+Henry) > > Hi Ayan, > > Adding Henry because this is something we probably want to fix in Xen > 4.17. AFAIU, the value is not used at all in Xen, so the risk is mostly > returning a wrong value to a domain using GICv3 ITS (not officially > supported and only expose to dom0 so far). Therefore, I would say this > should be OK to ingest in Xen 4.17. This sounds good to me (with your comments properly addressed): Release-acked-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 0c23f6df9d..7930ab6330 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -250,14 +250,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 */ + val = v->arch.vgic.rdist_pendbase; + val &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ + *r = vreg_reg64_extract(val, info); spin_unlock_irqrestore(&v->arch.vgic.lock, flags); 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 32bit register. The 32bit register is then modified bitwise with a mask (ie GICR_PENDBASER_PTZ, it clears the 62nd bit) which is greater than 32 bits. This will give an incorrect result. 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> --- 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. xen/arch/arm/vgic-v3.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)