Hi,
On 03/11/2022 12:13, Michal Orzel wrote:
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 <ayank...@amd.com>
---
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 \
+1
+});
... and here you did not.
FAOD, this is the correct approach. That said, the ';' should not be added.
+
/* 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 cas
FWIW, I would prefer the lower case version. That said, the arm32 code
match with the rest of the file.
Cheers,
--
Julien Grall
