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@amd.com>
Release-acked-by: Henry Wang <Henry.Wang@arm.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.

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

Hi Ayan,

On 27/10/2022 19:55, 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 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@amd.com>
> Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

On Thu, 27 Oct 2022 19:55:55 +0100
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 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@amd.com>
> Release-acked-by: Henry Wang <Henry.Wang@arm.com>

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