Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Xen>
  3. Devel
[XEN v4 07/11] xen/Arm: GICv3: Define ICH_LR<n>_EL2 on AArch32
ayan.kumar.halder at amd
Nov 28, 2022, 7:56 AM
Post #1 of 5 (70 views)
Permalink
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 <ayan.kumar.halder@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 ({ }).

v2 - 1. Use WRITE_SYSREG_LR(V, R) to make it consistent with before.
2. Defined the register alias.
3. Style issues.

v3 - 1. Addressed style issues.

xen/arch/arm/gic-v3.c | 132 +++++++++++------------
xen/arch/arm/include/asm/arm32/sysregs.h | 19 ++++
xen/arch/arm/include/asm/arm64/sysregs.h | 5 +
xen/arch/arm/include/asm/cpregs.h | 76 +++++++++++++
xen/arch/arm/include/asm/gic_v3_defs.h | 8 +-
5 files changed, 170 insertions(+), 70 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 64a76307dd..6457e7033c 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -73,37 +73,37 @@ static inline void gicv3_save_lrs(struct vcpu *v)
switch ( gicv3_info.nr_lrs )
{
case 16:
- v->arch.gic.v3.lr[15] = READ_SYSREG(ICH_LR15_EL2);
+ v->arch.gic.v3.lr[15] = READ_SYSREG_LR(15);
case 15:
- v->arch.gic.v3.lr[14] = READ_SYSREG(ICH_LR14_EL2);
+ v->arch.gic.v3.lr[14] = READ_SYSREG_LR(14);
case 14:
- v->arch.gic.v3.lr[13] = READ_SYSREG(ICH_LR13_EL2);
+ v->arch.gic.v3.lr[13] = READ_SYSREG_LR(13);
case 13:
- v->arch.gic.v3.lr[12] = READ_SYSREG(ICH_LR12_EL2);
+ v->arch.gic.v3.lr[12] = READ_SYSREG_LR(12);
case 12:
- v->arch.gic.v3.lr[11] = READ_SYSREG(ICH_LR11_EL2);
+ v->arch.gic.v3.lr[11] = READ_SYSREG_LR(11);
case 11:
- v->arch.gic.v3.lr[10] = READ_SYSREG(ICH_LR10_EL2);
+ v->arch.gic.v3.lr[10] = READ_SYSREG_LR(10);
case 10:
- v->arch.gic.v3.lr[9] = READ_SYSREG(ICH_LR9_EL2);
+ v->arch.gic.v3.lr[9] = READ_SYSREG_LR(9);
case 9:
- v->arch.gic.v3.lr[8] = READ_SYSREG(ICH_LR8_EL2);
+ v->arch.gic.v3.lr[8] = READ_SYSREG_LR(8);
case 8:
- v->arch.gic.v3.lr[7] = READ_SYSREG(ICH_LR7_EL2);
+ v->arch.gic.v3.lr[7] = READ_SYSREG_LR(7);
case 7:
- v->arch.gic.v3.lr[6] = READ_SYSREG(ICH_LR6_EL2);
+ v->arch.gic.v3.lr[6] = READ_SYSREG_LR(6);
case 6:
- v->arch.gic.v3.lr[5] = READ_SYSREG(ICH_LR5_EL2);
+ v->arch.gic.v3.lr[5] = READ_SYSREG_LR(5);
case 5:
- v->arch.gic.v3.lr[4] = READ_SYSREG(ICH_LR4_EL2);
+ v->arch.gic.v3.lr[4] = READ_SYSREG_LR(4);
case 4:
- v->arch.gic.v3.lr[3] = READ_SYSREG(ICH_LR3_EL2);
+ v->arch.gic.v3.lr[3] = READ_SYSREG_LR(3);
case 3:
- v->arch.gic.v3.lr[2] = READ_SYSREG(ICH_LR2_EL2);
+ v->arch.gic.v3.lr[2] = READ_SYSREG_LR(2);
case 2:
- v->arch.gic.v3.lr[1] = READ_SYSREG(ICH_LR1_EL2);
+ v->arch.gic.v3.lr[1] = READ_SYSREG_LR(1);
case 1:
- v->arch.gic.v3.lr[0] = READ_SYSREG(ICH_LR0_EL2);
+ v->arch.gic.v3.lr[0] = READ_SYSREG_LR(0);
break;
default:
BUG();
@@ -120,37 +120,37 @@ static inline void gicv3_restore_lrs(const struct vcpu *v)
switch ( gicv3_info.nr_lrs )
{
case 16:
- WRITE_SYSREG(v->arch.gic.v3.lr[15], ICH_LR15_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[15], 15);
case 15:
- WRITE_SYSREG(v->arch.gic.v3.lr[14], ICH_LR14_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[14], 14);
case 14:
- WRITE_SYSREG(v->arch.gic.v3.lr[13], ICH_LR13_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[13], 13);
case 13:
- WRITE_SYSREG(v->arch.gic.v3.lr[12], ICH_LR12_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[12], 12);
case 12:
- WRITE_SYSREG(v->arch.gic.v3.lr[11], ICH_LR11_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[11], 11);
case 11:
- WRITE_SYSREG(v->arch.gic.v3.lr[10], ICH_LR10_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[10], 10);
case 10:
- WRITE_SYSREG(v->arch.gic.v3.lr[9], ICH_LR9_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[9], 9);
case 9:
- WRITE_SYSREG(v->arch.gic.v3.lr[8], ICH_LR8_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[8], 8);
case 8:
- WRITE_SYSREG(v->arch.gic.v3.lr[7], ICH_LR7_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[7], 7);
case 7:
- WRITE_SYSREG(v->arch.gic.v3.lr[6], ICH_LR6_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[6], 6);
case 6:
- WRITE_SYSREG(v->arch.gic.v3.lr[5], ICH_LR5_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[5], 5);
case 5:
- WRITE_SYSREG(v->arch.gic.v3.lr[4], ICH_LR4_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[4], 4);
case 4:
- WRITE_SYSREG(v->arch.gic.v3.lr[3], ICH_LR3_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[3], 3);
case 3:
- WRITE_SYSREG(v->arch.gic.v3.lr[2], ICH_LR2_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[2], 2);
case 2:
- WRITE_SYSREG(v->arch.gic.v3.lr[1], ICH_LR1_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[1], 1);
case 1:
- WRITE_SYSREG(v->arch.gic.v3.lr[0], ICH_LR0_EL2);
+ WRITE_SYSREG_LR(v->arch.gic.v3.lr[0], 0);
break;
default:
BUG();
@@ -161,22 +161,22 @@ static uint64_t gicv3_ich_read_lr(int lr)
{
switch ( lr )
{
- case 0: return READ_SYSREG(ICH_LR0_EL2);
- case 1: return READ_SYSREG(ICH_LR1_EL2);
- case 2: return READ_SYSREG(ICH_LR2_EL2);
- case 3: return READ_SYSREG(ICH_LR3_EL2);
- case 4: return READ_SYSREG(ICH_LR4_EL2);
- case 5: return READ_SYSREG(ICH_LR5_EL2);
- case 6: return READ_SYSREG(ICH_LR6_EL2);
- case 7: return READ_SYSREG(ICH_LR7_EL2);
- case 8: return READ_SYSREG(ICH_LR8_EL2);
- case 9: return READ_SYSREG(ICH_LR9_EL2);
- case 10: return READ_SYSREG(ICH_LR10_EL2);
- case 11: return READ_SYSREG(ICH_LR11_EL2);
- case 12: return READ_SYSREG(ICH_LR12_EL2);
- case 13: return READ_SYSREG(ICH_LR13_EL2);
- case 14: return READ_SYSREG(ICH_LR14_EL2);
- case 15: return READ_SYSREG(ICH_LR15_EL2);
+ case 0: return READ_SYSREG_LR(0);
+ case 1: return READ_SYSREG_LR(1);
+ case 2: return READ_SYSREG_LR(2);
+ case 3: return READ_SYSREG_LR(3);
+ case 4: return READ_SYSREG_LR(4);
+ case 5: return READ_SYSREG_LR(5);
+ case 6: return READ_SYSREG_LR(6);
+ case 7: return READ_SYSREG_LR(7);
+ case 8: return READ_SYSREG_LR(8);
+ case 9: return READ_SYSREG_LR(9);
+ case 10: return READ_SYSREG_LR(10);
+ case 11: return READ_SYSREG_LR(11);
+ case 12: return READ_SYSREG_LR(12);
+ case 13: return READ_SYSREG_LR(13);
+ case 14: return READ_SYSREG_LR(14);
+ case 15: return READ_SYSREG_LR(15);
default:
BUG();
}
@@ -187,52 +187,52 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
switch ( lr )
{
case 0:
- WRITE_SYSREG(val, ICH_LR0_EL2);
+ WRITE_SYSREG_LR(val, 0);
break;
case 1:
- WRITE_SYSREG(val, ICH_LR1_EL2);
+ WRITE_SYSREG_LR(val, 1);
break;
case 2:
- WRITE_SYSREG(val, ICH_LR2_EL2);
+ WRITE_SYSREG_LR(val, 2);
break;
case 3:
- WRITE_SYSREG(val, ICH_LR3_EL2);
+ WRITE_SYSREG_LR(val, 3);
break;
case 4:
- WRITE_SYSREG(val, ICH_LR4_EL2);
+ WRITE_SYSREG_LR(val, 4);
break;
case 5:
- WRITE_SYSREG(val, ICH_LR5_EL2);
+ WRITE_SYSREG_LR(val, 5);
break;
case 6:
- WRITE_SYSREG(val, ICH_LR6_EL2);
+ WRITE_SYSREG_LR(val, 6);
break;
case 7:
- WRITE_SYSREG(val, ICH_LR7_EL2);
+ WRITE_SYSREG_LR(val, 7);
break;
case 8:
- WRITE_SYSREG(val, ICH_LR8_EL2);
+ WRITE_SYSREG_LR(val, 8);
break;
case 9:
- WRITE_SYSREG(val, ICH_LR9_EL2);
+ WRITE_SYSREG_LR(val, 9);
break;
case 10:
- WRITE_SYSREG(val, ICH_LR10_EL2);
+ WRITE_SYSREG_LR(val, 10);
break;
case 11:
- WRITE_SYSREG(val, ICH_LR11_EL2);
+ WRITE_SYSREG_LR(val, 11);
break;
case 12:
- WRITE_SYSREG(val, ICH_LR12_EL2);
+ WRITE_SYSREG_LR(val, 12);
break;
case 13:
- WRITE_SYSREG(val, ICH_LR13_EL2);
+ WRITE_SYSREG_LR(val, 13);
break;
case 14:
- WRITE_SYSREG(val, ICH_LR14_EL2);
+ WRITE_SYSREG_LR(val, 14);
break;
case 15:
- WRITE_SYSREG(val, ICH_LR15_EL2);
+ WRITE_SYSREG_LR(val, 15);
break;
default:
return;
@@ -417,12 +417,12 @@ static void gicv3_dump_state(const struct vcpu *v)
if ( v == current )
{
for ( i = 0; i < gicv3_info.nr_lrs; i++ )
- printk(" HW_LR[%d]=%lx\n", i, gicv3_ich_read_lr(i));
+ printk(" HW_LR[%d]=%" PRIx64 "\n", i, gicv3_ich_read_lr(i));
}
else
{
for ( i = 0; i < gicv3_info.nr_lrs; i++ )
- printk(" VCPU_LR[%d]=%lx\n", i, v->arch.gic.v3.lr[i]);
+ printk(" VCPU_LR[%d]=%" PRIx64 "\n", i, v->arch.gic.v3.lr[i]);
}
}

diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h b/xen/arch/arm/include/asm/arm32/sysregs.h
index 6841d5de43..22871999af 100644
--- a/xen/arch/arm/include/asm/arm32/sysregs.h
+++ b/xen/arch/arm/include/asm/arm32/sysregs.h
@@ -62,6 +62,25 @@
#define READ_SYSREG(R...) READ_SYSREG32(R)
#define WRITE_SYSREG(V, R...) WRITE_SYSREG32(V, R)

+/* Wrappers for accessing interrupt controller list registers. */
+#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(v, index) ({ \
+ 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..4638999514 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -472,6 +472,11 @@
#define READ_SYSREG(name) READ_SYSREG64(name)
#define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)

+/* Wrappers for accessing interrupt controller list registers. */
+#define ICH_LR_REG(index) ICH_LR ## index ## _EL2
+#define WRITE_SYSREG_LR(v, index) 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..b85e811f51 100644
--- a/xen/arch/arm/include/asm/cpregs.h
+++ b/xen/arch/arm/include/asm/cpregs.h
@@ -8,6 +8,8 @@
* support 32-bit guests.
*/

+#define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm, opc2
+
#define __HSR_CPREG_c0 0
#define __HSR_CPREG_c1 1
#define __HSR_CPREG_c2 2
@@ -259,6 +261,48 @@
#define VBAR p15,0,c12,c0,0 /* Vector Base Address Register */
#define HVBAR p15,4,c12,c0,0 /* Hyp. Vector Base Address Register */

+/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
+#define __LR0(x) ___CP32(p15, 4, c12, c12, x)
+#define __LR8(x) ___CP32(p15, 4, c12, c13, x)
+
+#define ICH_LR0 __LR0(0)
+#define ICH_LR1 __LR0(1)
+#define ICH_LR2 __LR0(2)
+#define ICH_LR3 __LR0(3)
+#define ICH_LR4 __LR0(4)
+#define ICH_LR5 __LR0(5)
+#define ICH_LR6 __LR0(6)
+#define ICH_LR7 __LR0(7)
+#define ICH_LR8 __LR8(0)
+#define ICH_LR9 __LR8(1)
+#define ICH_LR10 __LR8(2)
+#define ICH_LR11 __LR8(3)
+#define ICH_LR12 __LR8(4)
+#define ICH_LR13 __LR8(5)
+#define ICH_LR14 __LR8(6)
+#define ICH_LR15 __LR8(7)
+
+/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
+#define __LRC0(x) ___CP32(p15, 4, c12, c14, x)
+#define __LRC8(x) ___CP32(p15, 4, c12, c15, x)
+
+#define ICH_LRC0 __LRC0(0)
+#define ICH_LRC1 __LRC0(1)
+#define ICH_LRC2 __LRC0(2)
+#define ICH_LRC3 __LRC0(3)
+#define ICH_LRC4 __LRC0(4)
+#define ICH_LRC5 __LRC0(5)
+#define ICH_LRC6 __LRC0(6)
+#define ICH_LRC7 __LRC0(7)
+#define ICH_LRC8 __LRC8(0)
+#define ICH_LRC9 __LRC8(1)
+#define ICH_LRC10 __LRC8(2)
+#define ICH_LRC11 __LRC8(3)
+#define ICH_LRC12 __LRC8(4)
+#define ICH_LRC13 __LRC8(5)
+#define ICH_LRC14 __LRC8(6)
+#define ICH_LRC15 __LRC8(7)
+
/* CP15 CR13: */
#define FCSEIDR p15,0,c13,c0,0 /* FCSE Process ID Register */
#define CONTEXTIDR p15,0,c13,c0,1 /* Context ID Register */
@@ -317,6 +361,38 @@
#define HCR_EL2 HCR
#define HPFAR_EL2 HPFAR
#define HSTR_EL2 HSTR
+#define ICH_LR0_EL2 ICH_LR0
+#define ICH_LR1_EL2 ICH_LR1
+#define ICH_LR2_EL2 ICH_LR2
+#define ICH_LR3_EL2 ICH_LR3
+#define ICH_LR4_EL2 ICH_LR4
+#define ICH_LR5_EL2 ICH_LR5
+#define ICH_LR6_EL2 ICH_LR6
+#define ICH_LR7_EL2 ICH_LR7
+#define ICH_LR8_EL2 ICH_LR8
+#define ICH_LR9_EL2 ICH_LR9
+#define ICH_LR10_EL2 ICH_LR10
+#define ICH_LR11_EL2 ICH_LR11
+#define ICH_LR12_EL2 ICH_LR12
+#define ICH_LR13_EL2 ICH_LR13
+#define ICH_LR14_EL2 ICH_LR14
+#define ICH_LR15_EL2 ICH_LR15
+#define ICH_LRC0_EL2 ICH_LRC0
+#define ICH_LRC1_EL2 ICH_LRC1
+#define ICH_LRC2_EL2 ICH_LRC2
+#define ICH_LRC3_EL2 ICH_LRC3
+#define ICH_LRC4_EL2 ICH_LRC4
+#define ICH_LRC5_EL2 ICH_LRC5
+#define ICH_LRC6_EL2 ICH_LRC6
+#define ICH_LRC7_EL2 ICH_LRC7
+#define ICH_LRC8_EL2 ICH_LRC8
+#define ICH_LRC9_EL2 ICH_LRC9
+#define ICH_LRC10_EL2 ICH_LRC10
+#define ICH_LRC11_EL2 ICH_LRC11
+#define ICH_LRC12_EL2 ICH_LRC12
+#define ICH_LRC13_EL2 ICH_LRC13
+#define ICH_LRC14_EL2 ICH_LRC14
+#define ICH_LRC15_EL2 ICH_LRC15
#define ID_AFR0_EL1 ID_AFR0
#define ID_DFR0_EL1 ID_DFR0
#define ID_DFR1_EL1 ID_DFR1
diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
index 48a1bc401e..227533868f 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)

#define ICH_VTR_NRLRGS 0x3f
#define ICH_VTR_PRIBITS_MASK 0x7
@@ -195,7 +195,7 @@

#define ICH_SGI_IRQMODE_SHIFT 40
#define ICH_SGI_IRQMODE_MASK 0x1
-#define ICH_SGI_TARGET_OTHERS 1UL
+#define ICH_SGI_TARGET_OTHERS 1ULL
#define ICH_SGI_TARGET_LIST 0
#define ICH_SGI_IRQ_SHIFT 24
#define ICH_SGI_IRQ_MASK 0xf
--
2.17.1
Re: [XEN v4 07/11] xen/Arm: GICv3: Define ICH_LR<n>_EL2 on AArch32 [ In reply to ]
michal.orzel at amd
Nov 29, 2022, 6:33 AM
Post #2 of 5 (69 views)
Permalink
Hi Ayan,

On 28/11/2022 16:56, 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 <ayan.kumar.halder@amd.com>

Reviewed-by: Michal Orzel <michal.orzel@amd.com>,
with two remarks (this is up to maintainers as these are rather cosmetic changes).

> ---
>
> 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 ({ }).
>
> v2 - 1. Use WRITE_SYSREG_LR(V, R) to make it consistent with before.
> 2. Defined the register alias.
> 3. Style issues.
>
> v3 - 1. Addressed style issues.
>
> xen/arch/arm/gic-v3.c | 132 +++++++++++------------
> xen/arch/arm/include/asm/arm32/sysregs.h | 19 ++++
> xen/arch/arm/include/asm/arm64/sysregs.h | 5 +
> xen/arch/arm/include/asm/cpregs.h | 76 +++++++++++++
> xen/arch/arm/include/asm/gic_v3_defs.h | 8 +-
> 5 files changed, 170 insertions(+), 70 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 64a76307dd..6457e7033c 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -73,37 +73,37 @@ static inline void gicv3_save_lrs(struct vcpu *v)
> switch ( gicv3_info.nr_lrs )
> {
> case 16:
> - v->arch.gic.v3.lr[15] = READ_SYSREG(ICH_LR15_EL2);
> + v->arch.gic.v3.lr[15] = READ_SYSREG_LR(15);
> case 15:
> - v->arch.gic.v3.lr[14] = READ_SYSREG(ICH_LR14_EL2);
> + v->arch.gic.v3.lr[14] = READ_SYSREG_LR(14);
> case 14:
> - v->arch.gic.v3.lr[13] = READ_SYSREG(ICH_LR13_EL2);
> + v->arch.gic.v3.lr[13] = READ_SYSREG_LR(13);
> case 13:
> - v->arch.gic.v3.lr[12] = READ_SYSREG(ICH_LR12_EL2);
> + v->arch.gic.v3.lr[12] = READ_SYSREG_LR(12);
> case 12:
> - v->arch.gic.v3.lr[11] = READ_SYSREG(ICH_LR11_EL2);
> + v->arch.gic.v3.lr[11] = READ_SYSREG_LR(11);
> case 11:
> - v->arch.gic.v3.lr[10] = READ_SYSREG(ICH_LR10_EL2);
> + v->arch.gic.v3.lr[10] = READ_SYSREG_LR(10);
> case 10:
> - v->arch.gic.v3.lr[9] = READ_SYSREG(ICH_LR9_EL2);
> + v->arch.gic.v3.lr[9] = READ_SYSREG_LR(9);
> case 9:
> - v->arch.gic.v3.lr[8] = READ_SYSREG(ICH_LR8_EL2);
> + v->arch.gic.v3.lr[8] = READ_SYSREG_LR(8);
> case 8:
> - v->arch.gic.v3.lr[7] = READ_SYSREG(ICH_LR7_EL2);
> + v->arch.gic.v3.lr[7] = READ_SYSREG_LR(7);
> case 7:
> - v->arch.gic.v3.lr[6] = READ_SYSREG(ICH_LR6_EL2);
> + v->arch.gic.v3.lr[6] = READ_SYSREG_LR(6);
> case 6:
> - v->arch.gic.v3.lr[5] = READ_SYSREG(ICH_LR5_EL2);
> + v->arch.gic.v3.lr[5] = READ_SYSREG_LR(5);
> case 5:
> - v->arch.gic.v3.lr[4] = READ_SYSREG(ICH_LR4_EL2);
> + v->arch.gic.v3.lr[4] = READ_SYSREG_LR(4);
> case 4:
> - v->arch.gic.v3.lr[3] = READ_SYSREG(ICH_LR3_EL2);
> + v->arch.gic.v3.lr[3] = READ_SYSREG_LR(3);
> case 3:
> - v->arch.gic.v3.lr[2] = READ_SYSREG(ICH_LR2_EL2);
> + v->arch.gic.v3.lr[2] = READ_SYSREG_LR(2);
> case 2:
> - v->arch.gic.v3.lr[1] = READ_SYSREG(ICH_LR1_EL2);
> + v->arch.gic.v3.lr[1] = READ_SYSREG_LR(1);
> case 1:
> - v->arch.gic.v3.lr[0] = READ_SYSREG(ICH_LR0_EL2);
> + v->arch.gic.v3.lr[0] = READ_SYSREG_LR(0);
> break;
> default:
> BUG();
> @@ -120,37 +120,37 @@ static inline void gicv3_restore_lrs(const struct vcpu *v)
> switch ( gicv3_info.nr_lrs )
> {
> case 16:
> - WRITE_SYSREG(v->arch.gic.v3.lr[15], ICH_LR15_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[15], 15);
> case 15:
> - WRITE_SYSREG(v->arch.gic.v3.lr[14], ICH_LR14_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[14], 14);
> case 14:
> - WRITE_SYSREG(v->arch.gic.v3.lr[13], ICH_LR13_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[13], 13);
> case 13:
> - WRITE_SYSREG(v->arch.gic.v3.lr[12], ICH_LR12_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[12], 12);
> case 12:
> - WRITE_SYSREG(v->arch.gic.v3.lr[11], ICH_LR11_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[11], 11);
> case 11:
> - WRITE_SYSREG(v->arch.gic.v3.lr[10], ICH_LR10_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[10], 10);
> case 10:
> - WRITE_SYSREG(v->arch.gic.v3.lr[9], ICH_LR9_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[9], 9);
> case 9:
> - WRITE_SYSREG(v->arch.gic.v3.lr[8], ICH_LR8_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[8], 8);
> case 8:
> - WRITE_SYSREG(v->arch.gic.v3.lr[7], ICH_LR7_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[7], 7);
> case 7:
> - WRITE_SYSREG(v->arch.gic.v3.lr[6], ICH_LR6_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[6], 6);
> case 6:
> - WRITE_SYSREG(v->arch.gic.v3.lr[5], ICH_LR5_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[5], 5);
> case 5:
> - WRITE_SYSREG(v->arch.gic.v3.lr[4], ICH_LR4_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[4], 4);
> case 4:
> - WRITE_SYSREG(v->arch.gic.v3.lr[3], ICH_LR3_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[3], 3);
> case 3:
> - WRITE_SYSREG(v->arch.gic.v3.lr[2], ICH_LR2_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[2], 2);
> case 2:
> - WRITE_SYSREG(v->arch.gic.v3.lr[1], ICH_LR1_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[1], 1);
> case 1:
> - WRITE_SYSREG(v->arch.gic.v3.lr[0], ICH_LR0_EL2);
> + WRITE_SYSREG_LR(v->arch.gic.v3.lr[0], 0);
> break;
> default:
> BUG();
> @@ -161,22 +161,22 @@ static uint64_t gicv3_ich_read_lr(int lr)
> {
> switch ( lr )
> {
> - case 0: return READ_SYSREG(ICH_LR0_EL2);
> - case 1: return READ_SYSREG(ICH_LR1_EL2);
> - case 2: return READ_SYSREG(ICH_LR2_EL2);
> - case 3: return READ_SYSREG(ICH_LR3_EL2);
> - case 4: return READ_SYSREG(ICH_LR4_EL2);
> - case 5: return READ_SYSREG(ICH_LR5_EL2);
> - case 6: return READ_SYSREG(ICH_LR6_EL2);
> - case 7: return READ_SYSREG(ICH_LR7_EL2);
> - case 8: return READ_SYSREG(ICH_LR8_EL2);
> - case 9: return READ_SYSREG(ICH_LR9_EL2);
> - case 10: return READ_SYSREG(ICH_LR10_EL2);
> - case 11: return READ_SYSREG(ICH_LR11_EL2);
> - case 12: return READ_SYSREG(ICH_LR12_EL2);
> - case 13: return READ_SYSREG(ICH_LR13_EL2);
> - case 14: return READ_SYSREG(ICH_LR14_EL2);
> - case 15: return READ_SYSREG(ICH_LR15_EL2);
> + case 0: return READ_SYSREG_LR(0);
> + case 1: return READ_SYSREG_LR(1);
> + case 2: return READ_SYSREG_LR(2);
> + case 3: return READ_SYSREG_LR(3);
> + case 4: return READ_SYSREG_LR(4);
> + case 5: return READ_SYSREG_LR(5);
> + case 6: return READ_SYSREG_LR(6);
> + case 7: return READ_SYSREG_LR(7);
> + case 8: return READ_SYSREG_LR(8);
> + case 9: return READ_SYSREG_LR(9);
> + case 10: return READ_SYSREG_LR(10);
> + case 11: return READ_SYSREG_LR(11);
> + case 12: return READ_SYSREG_LR(12);
> + case 13: return READ_SYSREG_LR(13);
> + case 14: return READ_SYSREG_LR(14);
> + case 15: return READ_SYSREG_LR(15);
> default:
> BUG();
> }
> @@ -187,52 +187,52 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
> switch ( lr )
> {
> case 0:
> - WRITE_SYSREG(val, ICH_LR0_EL2);
> + WRITE_SYSREG_LR(val, 0);
> break;
> case 1:
> - WRITE_SYSREG(val, ICH_LR1_EL2);
> + WRITE_SYSREG_LR(val, 1);
> break;
> case 2:
> - WRITE_SYSREG(val, ICH_LR2_EL2);
> + WRITE_SYSREG_LR(val, 2);
> break;
> case 3:
> - WRITE_SYSREG(val, ICH_LR3_EL2);
> + WRITE_SYSREG_LR(val, 3);
> break;
> case 4:
> - WRITE_SYSREG(val, ICH_LR4_EL2);
> + WRITE_SYSREG_LR(val, 4);
> break;
> case 5:
> - WRITE_SYSREG(val, ICH_LR5_EL2);
> + WRITE_SYSREG_LR(val, 5);
> break;
> case 6:
> - WRITE_SYSREG(val, ICH_LR6_EL2);
> + WRITE_SYSREG_LR(val, 6);
> break;
> case 7:
> - WRITE_SYSREG(val, ICH_LR7_EL2);
> + WRITE_SYSREG_LR(val, 7);
> break;
> case 8:
> - WRITE_SYSREG(val, ICH_LR8_EL2);
> + WRITE_SYSREG_LR(val, 8);
> break;
> case 9:
> - WRITE_SYSREG(val, ICH_LR9_EL2);
> + WRITE_SYSREG_LR(val, 9);
> break;
> case 10:
> - WRITE_SYSREG(val, ICH_LR10_EL2);
> + WRITE_SYSREG_LR(val, 10);
> break;
> case 11:
> - WRITE_SYSREG(val, ICH_LR11_EL2);
> + WRITE_SYSREG_LR(val, 11);
> break;
> case 12:
> - WRITE_SYSREG(val, ICH_LR12_EL2);
> + WRITE_SYSREG_LR(val, 12);
> break;
> case 13:
> - WRITE_SYSREG(val, ICH_LR13_EL2);
> + WRITE_SYSREG_LR(val, 13);
> break;
> case 14:
> - WRITE_SYSREG(val, ICH_LR14_EL2);
> + WRITE_SYSREG_LR(val, 14);
> break;
> case 15:
> - WRITE_SYSREG(val, ICH_LR15_EL2);
> + WRITE_SYSREG_LR(val, 15);
> break;
> default:
> return;
> @@ -417,12 +417,12 @@ static void gicv3_dump_state(const struct vcpu *v)
> if ( v == current )
> {
> for ( i = 0; i < gicv3_info.nr_lrs; i++ )
> - printk(" HW_LR[%d]=%lx\n", i, gicv3_ich_read_lr(i));
> + printk(" HW_LR[%d]=%" PRIx64 "\n", i, gicv3_ich_read_lr(i));
1. We do not usually add spaces between " and PRIx.

> }
> else
> {
> for ( i = 0; i < gicv3_info.nr_lrs; i++ )
> - printk(" VCPU_LR[%d]=%lx\n", i, v->arch.gic.v3.lr[i]);
> + printk(" VCPU_LR[%d]=%" PRIx64 "\n", i, v->arch.gic.v3.lr[i]);
> }
> }
>
> diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h b/xen/arch/arm/include/asm/arm32/sysregs.h
> index 6841d5de43..22871999af 100644
> --- a/xen/arch/arm/include/asm/arm32/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm32/sysregs.h
> @@ -62,6 +62,25 @@
> #define READ_SYSREG(R...) READ_SYSREG32(R)
> #define WRITE_SYSREG(V, R...) WRITE_SYSREG32(V, R)
>
> +/* Wrappers for accessing interrupt controller list registers. */
> +#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(v, index) ({ \
> + 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..4638999514 100644
> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> @@ -472,6 +472,11 @@
> #define READ_SYSREG(name) READ_SYSREG64(name)
> #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
>
> +/* Wrappers for accessing interrupt controller list registers. */
> +#define ICH_LR_REG(index) ICH_LR ## index ## _EL2
> +#define WRITE_SYSREG_LR(v, index) 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..b85e811f51 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -8,6 +8,8 @@
> * support 32-bit guests.
> */
>
> +#define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm, opc2
2. As you are using ___CP32 much later in this file, it could be moved...
> +
> #define __HSR_CPREG_c0 0
> #define __HSR_CPREG_c1 1
> #define __HSR_CPREG_c2 2
> @@ -259,6 +261,48 @@
> #define VBAR p15,0,c12,c0,0 /* Vector Base Address Register */
> #define HVBAR p15,4,c12,c0,0 /* Hyp. Vector Base Address Register */

...here, before first use. The remark I gave in v3 was that the definition should occur before use,
but it does not mean placing the macro at the top of the file.

>
> +/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
> +#define __LR0(x) ___CP32(p15, 4, c12, c12, x)
> +#define __LR8(x) ___CP32(p15, 4, c12, c13, x)
> +
> +#define ICH_LR0 __LR0(0)
> +#define ICH_LR1 __LR0(1)
> +#define ICH_LR2 __LR0(2)
> +#define ICH_LR3 __LR0(3)
> +#define ICH_LR4 __LR0(4)
> +#define ICH_LR5 __LR0(5)
> +#define ICH_LR6 __LR0(6)
> +#define ICH_LR7 __LR0(7)
> +#define ICH_LR8 __LR8(0)
> +#define ICH_LR9 __LR8(1)
> +#define ICH_LR10 __LR8(2)
> +#define ICH_LR11 __LR8(3)
> +#define ICH_LR12 __LR8(4)
> +#define ICH_LR13 __LR8(5)
> +#define ICH_LR14 __LR8(6)
> +#define ICH_LR15 __LR8(7)
> +
> +/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
> +#define __LRC0(x) ___CP32(p15, 4, c12, c14, x)
> +#define __LRC8(x) ___CP32(p15, 4, c12, c15, x)
> +
> +#define ICH_LRC0 __LRC0(0)
> +#define ICH_LRC1 __LRC0(1)
> +#define ICH_LRC2 __LRC0(2)
> +#define ICH_LRC3 __LRC0(3)
> +#define ICH_LRC4 __LRC0(4)
> +#define ICH_LRC5 __LRC0(5)
> +#define ICH_LRC6 __LRC0(6)
> +#define ICH_LRC7 __LRC0(7)
> +#define ICH_LRC8 __LRC8(0)
> +#define ICH_LRC9 __LRC8(1)
> +#define ICH_LRC10 __LRC8(2)
> +#define ICH_LRC11 __LRC8(3)
> +#define ICH_LRC12 __LRC8(4)
> +#define ICH_LRC13 __LRC8(5)
> +#define ICH_LRC14 __LRC8(6)
> +#define ICH_LRC15 __LRC8(7)
> +
> /* CP15 CR13: */
> #define FCSEIDR p15,0,c13,c0,0 /* FCSE Process ID Register */
> #define CONTEXTIDR p15,0,c13,c0,1 /* Context ID Register */
> @@ -317,6 +361,38 @@
> #define HCR_EL2 HCR
> #define HPFAR_EL2 HPFAR
> #define HSTR_EL2 HSTR
> +#define ICH_LR0_EL2 ICH_LR0
> +#define ICH_LR1_EL2 ICH_LR1
> +#define ICH_LR2_EL2 ICH_LR2
> +#define ICH_LR3_EL2 ICH_LR3
> +#define ICH_LR4_EL2 ICH_LR4
> +#define ICH_LR5_EL2 ICH_LR5
> +#define ICH_LR6_EL2 ICH_LR6
> +#define ICH_LR7_EL2 ICH_LR7
> +#define ICH_LR8_EL2 ICH_LR8
> +#define ICH_LR9_EL2 ICH_LR9
> +#define ICH_LR10_EL2 ICH_LR10
> +#define ICH_LR11_EL2 ICH_LR11
> +#define ICH_LR12_EL2 ICH_LR12
> +#define ICH_LR13_EL2 ICH_LR13
> +#define ICH_LR14_EL2 ICH_LR14
> +#define ICH_LR15_EL2 ICH_LR15
> +#define ICH_LRC0_EL2 ICH_LRC0
> +#define ICH_LRC1_EL2 ICH_LRC1
> +#define ICH_LRC2_EL2 ICH_LRC2
> +#define ICH_LRC3_EL2 ICH_LRC3
> +#define ICH_LRC4_EL2 ICH_LRC4
> +#define ICH_LRC5_EL2 ICH_LRC5
> +#define ICH_LRC6_EL2 ICH_LRC6
> +#define ICH_LRC7_EL2 ICH_LRC7
> +#define ICH_LRC8_EL2 ICH_LRC8
> +#define ICH_LRC9_EL2 ICH_LRC9
> +#define ICH_LRC10_EL2 ICH_LRC10
> +#define ICH_LRC11_EL2 ICH_LRC11
> +#define ICH_LRC12_EL2 ICH_LRC12
> +#define ICH_LRC13_EL2 ICH_LRC13
> +#define ICH_LRC14_EL2 ICH_LRC14
> +#define ICH_LRC15_EL2 ICH_LRC15
> #define ID_AFR0_EL1 ID_AFR0
> #define ID_DFR0_EL1 ID_DFR0
> #define ID_DFR1_EL1 ID_DFR1
> diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
> index 48a1bc401e..227533868f 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)
>
> #define ICH_VTR_NRLRGS 0x3f
> #define ICH_VTR_PRIBITS_MASK 0x7
> @@ -195,7 +195,7 @@
>
> #define ICH_SGI_IRQMODE_SHIFT 40
> #define ICH_SGI_IRQMODE_MASK 0x1
> -#define ICH_SGI_TARGET_OTHERS 1UL
> +#define ICH_SGI_TARGET_OTHERS 1ULL
> #define ICH_SGI_TARGET_LIST 0
> #define ICH_SGI_IRQ_SHIFT 24
> #define ICH_SGI_IRQ_MASK 0xf
> --
> 2.17.1
>
>
~Michal
Re: [XEN v4 07/11] xen/Arm: GICv3: Define ICH_LR<n>_EL2 on AArch32 [ In reply to ]
julien at xen
Dec 3, 2022, 10:35 AM
Post #3 of 5 (64 views)
Permalink
Hi,

On 29/11/2022 14:33, Michal Orzel wrote:
>> @@ -417,12 +417,12 @@ static void gicv3_dump_state(const struct vcpu *v)
>> if ( v == current )
>> {
>> for ( i = 0; i < gicv3_info.nr_lrs; i++ )
>> - printk(" HW_LR[%d]=%lx\n", i, gicv3_ich_read_lr(i));
>> + printk(" HW_LR[%d]=%" PRIx64 "\n", i, gicv3_ich_read_lr(i));
> 1. We do not usually add spaces between " and PRIx.

I don't have a strong preference on this one.

>
>> }
>> else
>> {
>> for ( i = 0; i < gicv3_info.nr_lrs; i++ )
>> - printk(" VCPU_LR[%d]=%lx\n", i, v->arch.gic.v3.lr[i]);
>> + printk(" VCPU_LR[%d]=%" PRIx64 "\n", i, v->arch.gic.v3.lr[i]);
>> }
>> }
>>
>> diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h b/xen/arch/arm/include/asm/arm32/sysregs.h
>> index 6841d5de43..22871999af 100644
>> --- a/xen/arch/arm/include/asm/arm32/sysregs.h
>> +++ b/xen/arch/arm/include/asm/arm32/sysregs.h
>> @@ -62,6 +62,25 @@
>> #define READ_SYSREG(R...) READ_SYSREG32(R)
>> #define WRITE_SYSREG(V, R...) WRITE_SYSREG32(V, R)
>>
>> +/* Wrappers for accessing interrupt controller list registers. */
>> +#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(v, index) ({ \
>> + 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..4638999514 100644
>> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
>> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
>> @@ -472,6 +472,11 @@
>> #define READ_SYSREG(name) READ_SYSREG64(name)
>> #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
>>
>> +/* Wrappers for accessing interrupt controller list registers. */
>> +#define ICH_LR_REG(index) ICH_LR ## index ## _EL2
>> +#define WRITE_SYSREG_LR(v, index) 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..b85e811f51 100644
>> --- a/xen/arch/arm/include/asm/cpregs.h
>> +++ b/xen/arch/arm/include/asm/cpregs.h
>> @@ -8,6 +8,8 @@
>> * support 32-bit guests.
>> */
>>
>> +#define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm, opc2
> 2. As you are using ___CP32 much later in this file, it could be moved...

__CP32() is already defined in arm32/sysregs.h which includes cpregs.h.
We should not define __CP32() twice and the only reason the compiler
doesn't complain is because the definition is the same

So one of the two needs to be dropped. Also, I think __CP32(), __CP64(),
CP32() and CP64() should be defined together because they are all related.

However...

>> +
>> #define __HSR_CPREG_c0 0
>> #define __HSR_CPREG_c1 1
>> #define __HSR_CPREG_c2 2
>> @@ -259,6 +261,48 @@
>> #define VBAR p15,0,c12,c0,0 /* Vector Base Address Register */
>> #define HVBAR p15,4,c12,c0,0 /* Hyp. Vector Base Address Register */
>
> ...here, before first use. The remark I gave in v3 was that the definition should occur before use,
> but it does not mean placing the macro at the top of the file.
>
>>
>> +/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
>> +#define __LR0(x) ___CP32(p15, 4, c12, c12, x)
>> +#define __LR8(x) ___CP32(p15, 4, c12, c13, x)

... I don't understand why you need to use __CP32 here and everywhere
else in this header. In fact I have replaced in my tree all the
__CP32(foo) with foo and it still compiles.

So can you explain why they are necessary?

Cheers,

--
Julien Grall
Re: [XEN v4 07/11] xen/Arm: GICv3: Define ICH_LR<n>_EL2 on AArch32 [ In reply to ]
ayankuma at amd
Dec 3, 2022, 12:02 PM
Post #4 of 5 (64 views)
Permalink
On 03/12/2022 18:35, Julien Grall wrote:
> Hi,

Hi Julien,

>
> On 29/11/2022 14:33, Michal Orzel wrote:
>>> @@ -417,12 +417,12 @@ static void gicv3_dump_state(const struct vcpu
>>> *v)
>>>       if ( v == current )
>>>       {
>>>           for ( i = 0; i < gicv3_info.nr_lrs; i++ )
>>> -            printk("   HW_LR[%d]=%lx\n", i, gicv3_ich_read_lr(i));
>>> +            printk("   HW_LR[%d]=%" PRIx64 "\n", i,
>>> gicv3_ich_read_lr(i));
>> 1. We do not usually add spaces between " and PRIx.
>
> I don't have a strong preference on this one.
ok, I will then keep it as it is.
>
>>
>>>       }
>>>       else
>>>       {
>>>           for ( i = 0; i < gicv3_info.nr_lrs; i++ )
>>> -            printk("   VCPU_LR[%d]=%lx\n", i, v->arch.gic.v3.lr[i]);
>>> +            printk("   VCPU_LR[%d]=%" PRIx64 "\n", i,
>>> v->arch.gic.v3.lr[i]);
>>>       }
>>>   }
>>>
>>> diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h
>>> b/xen/arch/arm/include/asm/arm32/sysregs.h
>>> index 6841d5de43..22871999af 100644
>>> --- a/xen/arch/arm/include/asm/arm32/sysregs.h
>>> +++ b/xen/arch/arm/include/asm/arm32/sysregs.h
>>> @@ -62,6 +62,25 @@
>>>   #define READ_SYSREG(R...)       READ_SYSREG32(R)
>>>   #define WRITE_SYSREG(V, R...)   WRITE_SYSREG32(V, R)
>>>
>>> +/* Wrappers for accessing interrupt controller list registers. */
>>> +#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(v, index) ({                        \
>>> +    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..4638999514 100644
>>> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
>>> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
>>> @@ -472,6 +472,11 @@
>>>   #define READ_SYSREG(name)     READ_SYSREG64(name)
>>>   #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
>>>
>>> +/* Wrappers for accessing interrupt controller list registers. */
>>> +#define ICH_LR_REG(index)          ICH_LR ## index ## _EL2
>>> +#define WRITE_SYSREG_LR(v, index)  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..b85e811f51 100644
>>> --- a/xen/arch/arm/include/asm/cpregs.h
>>> +++ b/xen/arch/arm/include/asm/cpregs.h
>>> @@ -8,6 +8,8 @@
>>>    * support 32-bit guests.
>>>    */
>>>
>>> +#define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn,
>>> crm, opc2
>> 2. As you are using ___CP32 much later in this file, it could be
>> moved...
>
> __CP32() is already defined in arm32/sysregs.h which includes
> cpregs.h. We should not define __CP32() twice and the only reason the
> compiler doesn't complain is because the definition is the same

The definition is different :-

In xen/arch/arm/include/asm/arm32/sysregs.h

 "#define __CP32(r, coproc, opc1, crn, crm, opc2) coproc, opc1, r, crn,
crm, opc2"   (Note:- it takes 6 arguments)

And what I have defined in  xen/arch/arm/include/asm/cpregs.h:-

#define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm,
opc2 (It takes 5 arguments, also note it has 3 underscores before CP32).

>
> So one of the two needs to be dropped. Also, I think __CP32(),
> __CP64(), CP32() and CP64() should be defined together because they
> are all related.
>
> However...
>
>>> +
>>>   #define __HSR_CPREG_c0  0
>>>   #define __HSR_CPREG_c1  1
>>>   #define __HSR_CPREG_c2  2
>>> @@ -259,6 +261,48 @@
>>>   #define VBAR            p15,0,c12,c0,0  /* Vector Base Address
>>> Register */
>>>   #define HVBAR           p15,4,c12,c0,0  /* Hyp. Vector Base
>>> Address Register */
>>
>> ...here, before first use. The remark I gave in v3 was that the
>> definition should occur before use,
>> but it does not mean placing the macro at the top of the file.
>>
>>>
>>> +/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
>>> +#define __LR0(x)        ___CP32(p15, 4, c12, c12, x)
>>> +#define __LR8(x)        ___CP32(p15, 4, c12, c13, x)
>
> ... I don't understand why you need to use __CP32 here and everywhere
> else in this header. In fact I have replaced in my tree all the
> __CP32(foo) with foo and it still compiles.
I think you mean ___CP32 here (3 underscores).
>
> So can you explain why they are necessary?

Actually, they are not necessary. I will remove ___CP32 definition in
the v5 patch.

Sorry, I was blindly influenced by the definition of __CP32. :( But,
there it was needed.

Would you also review 8/11, 9/11, 10/11 and 10/11 before I re-spin a new
series ? They have been reviewed by Michal (with some minor comments
which I can address in v5).

- Ayan

>
> Cheers,
>
Re: [XEN v4 07/11] xen/Arm: GICv3: Define ICH_LR<n>_EL2 on AArch32 [ In reply to ]
julien at xen
Dec 3, 2022, 1:09 PM
Post #5 of 5 (62 views)
Permalink
Hi,

On 03/12/2022 20:02, Ayan Kumar Halder wrote:
> The definition is different :-
>
> In xen/arch/arm/include/asm/arm32/sysregs.h
>
>  "#define __CP32(r, coproc, opc1, crn, crm, opc2) coproc, opc1, r, crn,
> crm, opc2"   (Note:- it takes 6 arguments)
>
> And what I have defined in  xen/arch/arm/include/asm/cpregs.h:-
>
> #define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm,
> opc2 (It takes 5 arguments, also note it has 3 underscores before CP32).

Ah... I missed the 3rd underscore. That's one more reason to not define
such macro.

[...]

> Would you also review 8/11, 9/11, 10/11 and 10/11 before I re-spin a new
> series ?

Patch #8, #9 needs ___CP32 to be removed.
Patch #10, some of the use needs to be clarified
Patch #11 have a minor comment

Please note that I'm reviewing these patches in between my main work and
it can take a while to go through them in detail. So please wait a few
days before asking for reply.

Cheers,

--
Julien Grall

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal