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

[Julien Grall <julien@xxxxxxx>]

(+Henry)

Hi Ayan,

Adding Henry because this is something we probably want to fix in Xen 
4.17. AFAIU, the value is not used at all in Xen, so the risk is mostly 
returning a wrong value to a domain using GICv3 ITS (not officially 
supported and only expose to dom0 so far). Therefore, I would say this 
should be OK to ingest in Xen 4.17.
Anyway back to the review...

Title: Technically a 64-bit guest is equally affected because it is 
allowed to do 32-bit access. Furthermore, I would prefer if you prefix 
the title with "xen/arm" so it is clear which component you are 
touching. So I would suggest the following title:
xen/arm: vGICv3: Emulate properly 32-bit access on GICR_PENDBASER

Note that I appended a "v" to "GICv3" to make clear this is referring to 
the virtual GICv3 rather than host GICv3.
On 24/10/2022 20: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 32bit register.
Not really, the result will be stored in a 64-bit register for AArch64 
host (even if for 32-bit guest). The main part is the top 32-bit of 
PENDBASER will be returned in the low 32-bit.
> 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.
I would be explicit and say that "the bit PTZ will not be 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>
> ---
>
> 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);
As you store v->arch.vgic.rdist_pendbase, the lock can be reduced to the 
first assignment. IOW:
val = v->arch....;
spin_unlock_irq_restore();
val &= ~GIC_PENDBASER_PTZ;

That said, I think the lock could now be completely dropped in favor of 
using read_atomic() (and write_atomic()).
I am saying this even with in mind that, IIUC, R52 may not support 
64-bit atomics (see [1]). There are a few places in Xen that expect 
64-bit access to be atomic. For instance the IOREQ code is using 
guest_cmpxchg64() and a 32-build of Xen with the "case 8" commented 
write_atomic_size() will fail. So there are some rework necessary for R52.
I would also rather not want to keep a bigger hammer (i.e. the lock) for 
architecture that support 64-bit atomic access.
Cheers,

[1] 20221025145506.5708839c@xxxxxxxxxxxxxxxxxxxxxxxxxx

--
Julien Grall