Re: [XEN v2 07/12] xen/Arm: GICv3: Define ICH_LR<n>_EL2 on AArch32

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 31/10/2022 15:13, Ayan Kumar Halder wrote:
> diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h 
> b/xen/arch/arm/include/asm/arm32/sysregs.h
> index 6841d5de43..8a9a014bef 100644
> --- a/xen/arch/arm/include/asm/arm32/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm32/sysregs.h
> @@ -62,6 +62,23 @@
>   #define READ_SYSREG(R...)       READ_SYSREG32(R)
>   #define WRITE_SYSREG(V, R...)   WRITE_SYSREG32(V, R)
>   
> +#define ICH_LR_REG(INDEX)        ICH_LR ## INDEX ## _EL2
> +#define ICH_LRC_REG(INDEX)       ICH_LRC ## INDEX ## _EL2
> +
> +#define READ_SYSREG_LR(INDEX)    ({                         \
> +    uint64_t _val;                                          \
> +    uint32_t _lrc = READ_CP32(ICH_LRC_REG(INDEX));          \
> +    uint32_t _lr = READ_CP32(ICH_LR_REG(INDEX));            \
> +                                                            \
> +    _val = ((uint64_t) _lrc << 32) | _lr;                   \
> +    _val; })
> +
> +#define WRITE_SYSREG_LR(INDEX, V) ({                        \
I would prefer if the prototype stays consistent with the other write 
helpers. I.e. the value is first and the name second.
> +    uint64_t _val = (V);                                    \
> +    WRITE_CP32(_val & GENMASK(31, 0), ICH_LR_REG(INDEX)); \
> +    WRITE_CP32(_val >> 32, ICH_LRC_REG(INDEX));           \
> +});
> +
>   /* MVFR2 is not defined on ARMv7 */
>   #define MVFR2_MAYBE_UNDEFINED
>   
> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
> index 54670084c3..353f0eea29 100644
> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> @@ -471,6 +471,9 @@
>   
>   #define READ_SYSREG(name)     READ_SYSREG64(name)
>   #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
> +#define ICH_LR_REG(index)          ICH_LR ## index ## _EL2
> +#define WRITE_SYSREG_LR(index, v)  WRITE_SYSREG(v, ICH_LR_REG(index))
> +#define READ_SYSREG_LR(index)      READ_SYSREG(ICH_LR_REG(index))
>   
>   #endif /* _ASM_ARM_ARM64_SYSREGS_H */
>   
> diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
> index 6daf2b1a30..4421dd49ac 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -362,6 +362,48 @@
>   #define MVFR0_EL1               MVFR0
>   #define MVFR1_EL1               MVFR1
>   #define MVFR2_EL1               MVFR2
> +
> +#define ___CP32(a,b,c,d,e)        a,b,c,d,e
Unless there are a reason to do, please use space after each comma. The 
same goes for the rest of the patch.
Cheers,

--
Julien Grall