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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 31/10/2022 16:13, Ayan Kumar Halder wrote:
> 
> 
> Refer "Arm IHI 0069H ID020922", 12.4.6, Interrupt Controller List Registers
> 
> AArch64 System register ICH_LR<n>_EL2 bits [31:0] are architecturally
> mapped to AArch32 System register ICH_LR<n>[31:0].
> AArch64 System register ICH_LR<n>_EL2 bits [63:32] are architecturally
> mapped to AArch32 System register ICH_LRC<n>[31:0].
> 
> Defined ICH_LR<0...15>_EL2 and ICH_LRC<0...15>_EL2 for Aarch32.
> For AArch32, the link register is stored as :-
> (((uint64_t) ICH_LRC<0...15>_EL2) << 32) | ICH_LR<0...15>_EL2
> 
> Also, ICR_LR macros need to be modified as ULL is 64 bits for AArch32 and
> AArch64.
> 
> Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxx>
> ---
> 
> Changes from :-
> v1 - 1. Moved the coproc register definitions to asm/cpregs.h.
> 2. Use GENMASK(31, 0) to represent 0xFFFFFFFF
> 3. Use READ_CP32()/WRITE_CP32() instead of READ_SYSREG()/WRITE_SYSREG().
> 4. Multi-line macro definitions should be enclosed within ({ }).
> 
>  xen/arch/arm/gic-v3.c                    | 132 +++++++++++------------
>  xen/arch/arm/include/asm/arm32/sysregs.h |  17 +++
>  xen/arch/arm/include/asm/arm64/sysregs.h |   3 +
>  xen/arch/arm/include/asm/cpregs.h        |  42 ++++++++
>  xen/arch/arm/include/asm/gic_v3_defs.h   |   6 +-
>  5 files changed, 131 insertions(+), 69 deletions(-)
> 
> 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
You could align to WRITE_SYSREG32(V, R).

Apart from that it would be good to add some comment before the code you added 
(ICH_LR_REG)
to separate from the code above and its comment about registers coming in 3 
types.
Something like:
/* Wrappers for accessing interrupt controller list registers. */

> +
> +#define READ_SYSREG_LR(INDEX)    ({                         \
Opening ({ should be placed one space after READ_SYSREG_LR(INDEX). It does not 
need to be aligned.

> +    uint64_t _val;                                          \
val is not really necessary. You could directly return the ((uint64_t) _lrc << 
32) | _lr;
Just something to consider, no need to replace.

> +    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; })
Here, you did put }) at the same line...

> +
> +#define WRITE_SYSREG_LR(INDEX, V) ({                        \
> +    uint64_t _val = (V);                                    \
> +    WRITE_CP32(_val & GENMASK(31, 0), ICH_LR_REG(INDEX)); \
> +    WRITE_CP32(_val >> 32, ICH_LRC_REG(INDEX));           \
Please, align \

> +});
... and here you did not.

> +
>  /* 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)
Here, I would separate the below macros by adding the comment similar to the 
one I showed above.
Or at least add a blank line.

> +#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))
I find it a bit odd that the macro param 'index' is written in lower case and 
for arm32 in upper case.

> 
>  #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
> +
You could align everything below to MVFR2.
Also maybe you could add some comment stating that the below relates to GIC 
system registers?

> +#define ___CP32(a,b,c,d,e)        a,b,c,d,e
> +#define __LR0_EL2(x)              ___CP32(p15,4,c12,c12,x)
> +#define __LR8_EL2(x)              ___CP32(p15,4,c12,c13,x)
> +
> +#define __LRC0_EL2(x)             ___CP32(p15,4,c12,c14,x)
> +#define __LRC8_EL2(x)             ___CP32(p15,4,c12,c15,x)
> +
> +#define ICH_LR0_EL2               __LR0_EL2(0)
> +#define ICH_LR1_EL2               __LR0_EL2(1)
> +#define ICH_LR2_EL2               __LR0_EL2(2)
> +#define ICH_LR3_EL2               __LR0_EL2(3)
> +#define ICH_LR4_EL2               __LR0_EL2(4)
> +#define ICH_LR5_EL2               __LR0_EL2(5)
> +#define ICH_LR6_EL2               __LR0_EL2(6)
> +#define ICH_LR7_EL2               __LR0_EL2(7)
> +#define ICH_LR8_EL2               __LR8_EL2(0)
> +#define ICH_LR9_EL2               __LR8_EL2(1)
> +#define ICH_LR10_EL2              __LR8_EL2(2)
> +#define ICH_LR11_EL2              __LR8_EL2(3)
> +#define ICH_LR12_EL2              __LR8_EL2(4)
> +#define ICH_LR13_EL2              __LR8_EL2(5)
> +#define ICH_LR14_EL2              __LR8_EL2(6)
> +#define ICH_LR15_EL2              __LR8_EL2(7)
> +
> +#define ICH_LRC0_EL2               __LRC0_EL2(0)
> +#define ICH_LRC1_EL2               __LRC0_EL2(1)
> +#define ICH_LRC2_EL2               __LRC0_EL2(2)
> +#define ICH_LRC3_EL2               __LRC0_EL2(3)
> +#define ICH_LRC4_EL2               __LRC0_EL2(4)
> +#define ICH_LRC5_EL2               __LRC0_EL2(5)
> +#define ICH_LRC6_EL2               __LRC0_EL2(6)
> +#define ICH_LRC7_EL2               __LRC0_EL2(7)
> +#define ICH_LRC8_EL2               __LRC8_EL2(0)
> +#define ICH_LRC9_EL2               __LRC8_EL2(1)
> +#define ICH_LRC10_EL2              __LRC8_EL2(2)
> +#define ICH_LRC11_EL2              __LRC8_EL2(3)
> +#define ICH_LRC12_EL2              __LRC8_EL2(4)
> +#define ICH_LRC13_EL2              __LRC8_EL2(5)
> +#define ICH_LRC14_EL2              __LRC8_EL2(6)
> +#define ICH_LRC15_EL2              __LRC8_EL2(7)
> +
>  #endif
> 
>  #endif
> diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h 
> b/xen/arch/arm/include/asm/gic_v3_defs.h
> index 48a1bc401e..87115f8b25 100644
> --- a/xen/arch/arm/include/asm/gic_v3_defs.h
> +++ b/xen/arch/arm/include/asm/gic_v3_defs.h
> @@ -185,9 +185,9 @@
>  #define ICH_LR_HW_SHIFT              61
>  #define ICH_LR_GRP_MASK              0x1
>  #define ICH_LR_GRP_SHIFT             60
> -#define ICH_LR_MAINTENANCE_IRQ       (1UL<<41)
> -#define ICH_LR_GRP1                  (1UL<<60)
> -#define ICH_LR_HW                    (1UL<<61)
> +#define ICH_LR_MAINTENANCE_IRQ       (1ULL<<41)
> +#define ICH_LR_GRP1                  (1ULL<<60)
> +#define ICH_LR_HW                    (1ULL<<61)
You could take the opportunity to add spaces between << to adhere to similar 
uses in this file.

> 
>  #define ICH_VTR_NRLRGS               0x3f
>  #define ICH_VTR_PRIBITS_MASK         0x7
> --
> 2.17.1
> 
> 

~Michal