Hi Ayan, > On 24 Oct 2022, at 20:30, Ayan Kumar Halder <[email protected]> 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 <[email protected]>
Looks good to me to. Reviewed-by: Bertrand Marquis <[email protected]> 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 >
