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

[Andre Przywara <andre.przywara@xxxxxxx>]

On Thu, 27 Oct 2022 19:55:55 +0100
Ayan Kumar Halder <ayankuma@xxxxxxx> 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 in __vgic_v3_rdistr_rd_mmio_read(). The reason
> being v->arch.vgic.rdist_pendbase is now being read in an atomic manner.
> 
> Similarly in __vgic_v3_rdistr_rd_mmio_write(), we have used read_atomic(),
> write_atomic() to read/write v->arch.vgic.rdist_pendbase.
> 
> 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>

Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>

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.
> 
> 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().
> 2. Removed spin_lock_irqsave()/spin_unlock_irqrestore() for access to
> v->arch.vgic.rdist_pendbase in __vgic_v3_rdistr_rd_mmio_write().
> 
> v4 - 1. Retained the spin_lock_irqsave()/spin_unlock_irqrestore() for access 
> to
> v->arch.vgic.rdist_pendbase in __vgic_v3_rdistr_rd_mmio_write(). This is 
> because
> there could be a potential race while read-modify-write is performed on
> v->arch.vgic.rdist_pendbase, with another caller performing the same 
> operation.
> 
>  xen/arch/arm/vgic-v3.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 0c23f6df9d..d0e265634e 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;
>      }
>  
> @@ -577,10 +576,10 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu 
> *v, mmio_info_t *info,
>          /* 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);