Re: [XEN v3] xen/arm: vGICv3: Emulate properly 32-bit access on GICR_PENDBASER

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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@xxxxxxx>
> > Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> > Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
> 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@xxxxxxx>
> > ---
> >
> > 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,