Re: [XEN v2 05/12] xen/Arm: GICv3: Fix GICR_{PENDBASER, PROPBASER} emulation on 32-bit host

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 31/10/2022 16:13, 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@xxxxxxx>
> ---
> 
> Changes from -
> v1 - 1. Extract the bug fix for incorrect bit clearing (GICR_PENDBASER_PTZ)
> into a separate patch fix.
> https://patchwork.kernel.org/project/xen-devel/patch/20221027185555.46125-1-ayankuma@xxxxxxx/
> 
>  xen/arch/arm/include/asm/gic_v3_defs.h | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 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) |  \
It would be good to align \ but at the same time I can understand that it was 
not
aligned before your change and you may not want to do this, so:

Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal