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

[Andre Przywara <andre.przywara@xxxxxxx>]

On Thu, 27 Oct 2022 16:40:01 +0100
Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:

Hi Ayan,

> On 27/10/2022 10:44, Andre Przywara wrote:
> > On Wed, 26 Oct 2022 19:30:04 +0100
> > Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
> >
> > Hi,  
> 
> Hi Andre,
> 
> I need a clarification.
> 
> >  
> >> 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.
> >>
> >> Also, we have removed spin_lock_irqsave()/spin_unlock_irqrestore() to 
> >> protect
> >> v->arch.vgic.rdist_pendbase. The reason being v->arch.vgic.rdist_pendbase 
> >> is
> >> now being read/written in an atomic manner (using 
> >> read_atomic()/write_atomic()).
> >>
> >> Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxx>
> >> 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.
> >> 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.
> >>
> >> v3 - 1. Added read_atomic()/write_atomic() for access to 
> >> v->arch.vgic.rdist_pendbase
> >> in __vgic_v3_rdistr_rd_mmio_write().
> >>
> >>   xen/arch/arm/vgic-v3.c | 19 ++++++-------------
> >>   1 file changed, 6 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> >> index 0c23f6df9d..1adbdc0e54 100644
> >> --- a/xen/arch/arm/vgic-v3.c
> >> +++ b/xen/arch/arm/vgic-v3.c
> >> @@ -249,16 +249,15 @@ 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 &= ~GICR_PENDBASER_PTZ;      /* WO, reads as 0 */
> >> +        *r = vreg_reg64_extract(val, info);  
> > That part looks fine now.
> >  
> >>           return 1;
> >>       }
> >>   
> >> @@ -566,25 +565,19 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct 
> >> vcpu *v, mmio_info_t *info,
> >>   
> >>       case VREG64(GICR_PENDBASER):
> >>       {
> >> -        unsigned long flags;
> >> -
> >>           if ( !v->domain->arch.vgic.has_its )
> >>               goto write_ignore_64;
> >>           if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
> >>   
> >> -        spin_lock_irqsave(&v->arch.vgic.lock, flags);
> >> -  
> > I don't think you can drop the lock here easily. If it would be just for
> > the LPIs enabled check, that'd be fine, because you can never turn LPIs off
> > again (but that would deserve an explicit comment then).
> >
> > But down below you do a read-modify-write operation of rdist_pendbase, and
> > need to make sure no one else is attempting that at the same time.
> >
> > Cheers,
> > Andre
> >  
> >>           /* Writing PENDBASER with LPIs enabled is UNPREDICTABLE. */
> >>           if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) )
> >>           {
> >> -            reg = v->arch.vgic.rdist_pendbase;
> >> +            reg = read_atomic(&v->arch.vgic.rdist_pendbase);
> >>               vreg_reg64_update(&reg, r, info);
> >>               reg = sanitize_pendbaser(reg);
> >> -            v->arch.vgic.rdist_pendbase = reg;
> >> +            write_atomic(&v->arch.vgic.rdist_pendbase, reg);
> >>           }
> >>   
> >> -        spin_unlock_irqrestore(&v->arch.vgic.lock, false);  
> 
> Shouldn't this be "spin_unlock_irqrestore(&v->arch.vgic.lock, flags)" ?

Ouch, indeed, looks like a typo!
There is a some type check in spin_lock_irqsave() and local_irq_restore(),
but not in spin_unlock_irqrestore(), so we missed that.

Cheers,
Andre

> 
> - Ayan
> 
> >> -
> >>           return 1;
> >>       }
> >>