Re: [XEN v2] GICv3: Emulate GICR_PENDBASER correctly for 32 bit guests

[Andre Przywara <andre.przywara@xxxxxxx>]

On Mon, 24 Oct 2022 20:30:02 +0100
Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:

Hi,

> 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 <ayankuma@xxxxxxx>

Indeed, the patch looks good to me. Also checked the other users of
vreg_reg64_extract(), they seem to be all correct, by first building
the value, then running the extract function on the final result.

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.
> 
>  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;
>      }