| Message ID | 20221021153128.44226-6-ayankuma@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Arm: Enable GICv3 for AArch32 | expand |
Hi Ayan,
Title: To me, it reads as if you provide a brand new code for emulating
the registers. However, below, you are only fixing the code. So how about:
"xen/arm: gicv3: Fix GICR_{PENDBASER, PROPBASER} emulation on 32-bit host"
On 21/10/2022 16:31, Ayan Kumar Halder wrote:
> 'unsigned long long' is defined as 64 bit across both aarch32 and aarch64.
> So, use 'ULL' for 64 bit word instead of UL which is 32 bits for aarch32.
> GICR_PENDBASER and GICR_PROPBASER both are 64 bit registers.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com>
> ---
> xen/arch/arm/include/asm/gic_v3_defs.h | 16 ++++++++--------
> xen/arch/arm/vgic-v3.c | 6 ++++--
> 2 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
> index 728e28d5e5..48a1bc401e 100644
> --- a/xen/arch/arm/include/asm/gic_v3_defs.h
> +++ b/xen/arch/arm/include/asm/gic_v3_defs.h
> @@ -134,15 +134,15 @@
>
> #define GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT 56
> #define GICR_PROPBASER_OUTER_CACHEABILITY_MASK \
> - (7UL << GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT)
> + (7ULL << GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT)
> #define GICR_PROPBASER_SHAREABILITY_SHIFT 10
> #define GICR_PROPBASER_SHAREABILITY_MASK \
> - (3UL << GICR_PROPBASER_SHAREABILITY_SHIFT)
> + (3ULL << GICR_PROPBASER_SHAREABILITY_SHIFT)
> #define GICR_PROPBASER_INNER_CACHEABILITY_SHIFT 7
> #define GICR_PROPBASER_INNER_CACHEABILITY_MASK \
> - (7UL << GICR_PROPBASER_INNER_CACHEABILITY_SHIFT)
> + (7ULL << GICR_PROPBASER_INNER_CACHEABILITY_SHIFT)
> #define GICR_PROPBASER_RES0_MASK \
> - (GENMASK(63, 59) | GENMASK(55, 52) | GENMASK(6, 5))
> + (GENMASK_ULL(63, 59) | GENMASK_ULL(55, 52) | GENMASK_ULL(6, 5))
>
> #define GICR_PENDBASER_SHAREABILITY_SHIFT 10
> #define GICR_PENDBASER_INNER_CACHEABILITY_SHIFT 7
> @@ -152,11 +152,11 @@
> #define GICR_PENDBASER_INNER_CACHEABILITY_MASK \
> (7UL << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT)
> #define GICR_PENDBASER_OUTER_CACHEABILITY_MASK \
> - (7UL << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT)
> -#define GICR_PENDBASER_PTZ BIT(62, UL)
> + (7ULL << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT)
> +#define GICR_PENDBASER_PTZ BIT(62, ULL)
> #define GICR_PENDBASER_RES0_MASK \
> - (BIT(63, UL) | GENMASK(61, 59) | GENMASK(55, 52) | \
> - GENMASK(15, 12) | GENMASK(6, 0))
> + (BIT(63, ULL) | GENMASK_ULL(61, 59) | GENMASK_ULL(55, 52) | \
> + GENMASK_ULL(15, 12) | GENMASK_ULL(6, 0))rer
>
> #define DEFAULT_PMR_VALUE 0xff
>
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index d86b41a39f..9f31360f56 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -254,14 +254,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 value;
>
> 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 */
> + value = v->arch.vgic.rdist_pendbase;
> + value &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */
> + *r = vreg_reg64_extract(value, info);
The commit message suggests the code would only be replacing "UL" with
"ULL". But here, you are doing more than that. The code is re-order so
PTZ is cleared before extracting the value.
I agree the existing code was wrong if the guest was using 32-bit access
and your new approach is correct. Can you split the change in a separate
patch? (Please add a Fixes tag as well).
> spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> return 1;
> }
Cheers,
diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h index 728e28d5e5..48a1bc401e 100644 --- a/xen/arch/arm/include/asm/gic_v3_defs.h +++ b/xen/arch/arm/include/asm/gic_v3_defs.h @@ -134,15 +134,15 @@ #define GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT 56 #define GICR_PROPBASER_OUTER_CACHEABILITY_MASK \ - (7UL << GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT) + (7ULL << GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT) #define GICR_PROPBASER_SHAREABILITY_SHIFT 10 #define GICR_PROPBASER_SHAREABILITY_MASK \ - (3UL << GICR_PROPBASER_SHAREABILITY_SHIFT) + (3ULL << GICR_PROPBASER_SHAREABILITY_SHIFT) #define GICR_PROPBASER_INNER_CACHEABILITY_SHIFT 7 #define GICR_PROPBASER_INNER_CACHEABILITY_MASK \ - (7UL << GICR_PROPBASER_INNER_CACHEABILITY_SHIFT) + (7ULL << GICR_PROPBASER_INNER_CACHEABILITY_SHIFT) #define GICR_PROPBASER_RES0_MASK \ - (GENMASK(63, 59) | GENMASK(55, 52) | GENMASK(6, 5)) + (GENMASK_ULL(63, 59) | GENMASK_ULL(55, 52) | GENMASK_ULL(6, 5)) #define GICR_PENDBASER_SHAREABILITY_SHIFT 10 #define GICR_PENDBASER_INNER_CACHEABILITY_SHIFT 7 @@ -152,11 +152,11 @@ #define GICR_PENDBASER_INNER_CACHEABILITY_MASK \ (7UL << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT) #define GICR_PENDBASER_OUTER_CACHEABILITY_MASK \ - (7UL << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT) -#define GICR_PENDBASER_PTZ BIT(62, UL) + (7ULL << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT) +#define GICR_PENDBASER_PTZ BIT(62, ULL) #define GICR_PENDBASER_RES0_MASK \ - (BIT(63, UL) | GENMASK(61, 59) | GENMASK(55, 52) | \ - GENMASK(15, 12) | GENMASK(6, 0)) + (BIT(63, ULL) | GENMASK_ULL(61, 59) | GENMASK_ULL(55, 52) | \ + GENMASK_ULL(15, 12) | GENMASK_ULL(6, 0)) #define DEFAULT_PMR_VALUE 0xff diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index d86b41a39f..9f31360f56 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -254,14 +254,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 value; 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 */ + value = v->arch.vgic.rdist_pendbase; + value &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ + *r = vreg_reg64_extract(value, info); spin_unlock_irqrestore(&v->arch.vgic.lock, flags); return 1; }
'unsigned long long' is defined as 64 bit across both aarch32 and aarch64. So, use 'ULL' for 64 bit word instead of UL which is 32 bits for aarch32. GICR_PENDBASER and GICR_PROPBASER both are 64 bit registers. Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> --- xen/arch/arm/include/asm/gic_v3_defs.h | 16 ++++++++-------- xen/arch/arm/vgic-v3.c | 6 ++++-- 2 files changed, 12 insertions(+), 10 deletions(-)