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@amd.com>
---

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

(+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@amd.com>
> ---
> 
> 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@donnerap.cambridge.arm.com

-- 
Julien Grall

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [XEN v2] GICv3: Emulate GICR_PENDBASER correctly for 32 bit
> guests
> 
> (+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.

This sounds good to me (with your comments properly addressed):

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

Hi Ayan,

> On 24 Oct 2022, at 20:30, Ayan Kumar Halder <ayankuma@amd.com> 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.
> 
> 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@amd.com>

Looks good to me to.

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

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

On Mon, 24 Oct 2022 20:30:02 +0100
Ayan Kumar Halder <ayankuma@amd.com> 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@amd.com>

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@arm.com>

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