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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 26/10/2022 19: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 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()).
> Fixes: fe7fa1332dabd9ce4 ("ARM: vGICv3: handle virtual LPI pending and 
> property tables")
> 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);
>           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);
> -
>           /* 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);
> -
>           return 1;
>       }
>