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@amd.com>
---
xen/arch/arm/gic-v3.c | 132 +++++++++++------------
xen/arch/arm/include/asm/arm32/sysregs.h | 52 +++++++++
xen/arch/arm/include/asm/arm64/sysregs.h | 7 +-
xen/arch/arm/include/asm/gic_v3_defs.h | 6 +-
4 files changed, 126 insertions(+), 71 deletions(-)
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 018fa0dfa0..8b4b168e78 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(15, v->arch.gic.v3.lr[15]);
case 15:
- WRITE_SYSREG(v->arch.gic.v3.lr[14], ICH_LR14_EL2);
+ WRITE_SYSREG_LR(14, v->arch.gic.v3.lr[14]);
case 14:
- WRITE_SYSREG(v->arch.gic.v3.lr[13], ICH_LR13_EL2);
+ WRITE_SYSREG_LR(13, v->arch.gic.v3.lr[13]);
case 13:
- WRITE_SYSREG(v->arch.gic.v3.lr[12], ICH_LR12_EL2);
+ WRITE_SYSREG_LR(12, v->arch.gic.v3.lr[12]);
case 12:
- WRITE_SYSREG(v->arch.gic.v3.lr[11], ICH_LR11_EL2);
+ WRITE_SYSREG_LR(11, v->arch.gic.v3.lr[11]);
case 11:
- WRITE_SYSREG(v->arch.gic.v3.lr[10], ICH_LR10_EL2);
+ WRITE_SYSREG_LR(10, v->arch.gic.v3.lr[10]);
case 10:
- WRITE_SYSREG(v->arch.gic.v3.lr[9], ICH_LR9_EL2);
+ WRITE_SYSREG_LR(9, v->arch.gic.v3.lr[9]);
case 9:
- WRITE_SYSREG(v->arch.gic.v3.lr[8], ICH_LR8_EL2);
+ WRITE_SYSREG_LR(8, v->arch.gic.v3.lr[8]);
case 8:
- WRITE_SYSREG(v->arch.gic.v3.lr[7], ICH_LR7_EL2);
+ WRITE_SYSREG_LR(7, v->arch.gic.v3.lr[7]);
case 7:
- WRITE_SYSREG(v->arch.gic.v3.lr[6], ICH_LR6_EL2);
+ WRITE_SYSREG_LR(6, v->arch.gic.v3.lr[6]);
case 6:
- WRITE_SYSREG(v->arch.gic.v3.lr[5], ICH_LR5_EL2);
+ WRITE_SYSREG_LR(5, v->arch.gic.v3.lr[5]);
case 5:
- WRITE_SYSREG(v->arch.gic.v3.lr[4], ICH_LR4_EL2);
+ WRITE_SYSREG_LR(4, v->arch.gic.v3.lr[4]);
case 4:
- WRITE_SYSREG(v->arch.gic.v3.lr[3], ICH_LR3_EL2);
+ WRITE_SYSREG_LR(3, v->arch.gic.v3.lr[3]);
case 3:
- WRITE_SYSREG(v->arch.gic.v3.lr[2], ICH_LR2_EL2);
+ WRITE_SYSREG_LR(2, v->arch.gic.v3.lr[2]);
case 2:
- WRITE_SYSREG(v->arch.gic.v3.lr[1], ICH_LR1_EL2);
+ WRITE_SYSREG_LR(1, v->arch.gic.v3.lr[1]);
case 1:
- WRITE_SYSREG(v->arch.gic.v3.lr[0], ICH_LR0_EL2);
+ WRITE_SYSREG_LR(0, v->arch.gic.v3.lr[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(0, val);
break;
case 1:
- WRITE_SYSREG(val, ICH_LR1_EL2);
+ WRITE_SYSREG_LR(1, val);
break;
case 2:
- WRITE_SYSREG(val, ICH_LR2_EL2);
+ WRITE_SYSREG_LR(2, val);
break;
case 3:
- WRITE_SYSREG(val, ICH_LR3_EL2);
+ WRITE_SYSREG_LR(3, val);
break;
case 4:
- WRITE_SYSREG(val, ICH_LR4_EL2);
+ WRITE_SYSREG_LR(4, val);
break;
case 5:
- WRITE_SYSREG(val, ICH_LR5_EL2);
+ WRITE_SYSREG_LR(5, val);
break;
case 6:
- WRITE_SYSREG(val, ICH_LR6_EL2);
+ WRITE_SYSREG_LR(6, val);
break;
case 7:
- WRITE_SYSREG(val, ICH_LR7_EL2);
+ WRITE_SYSREG_LR(7, val);
break;
case 8:
- WRITE_SYSREG(val, ICH_LR8_EL2);
+ WRITE_SYSREG_LR(8, val);
break;
case 9:
- WRITE_SYSREG(val, ICH_LR9_EL2);
+ WRITE_SYSREG_LR(9, val);
break;
case 10:
- WRITE_SYSREG(val, ICH_LR10_EL2);
+ WRITE_SYSREG_LR(10, val);
break;
case 11:
- WRITE_SYSREG(val, ICH_LR11_EL2);
+ WRITE_SYSREG_LR(11, val);
break;
case 12:
- WRITE_SYSREG(val, ICH_LR12_EL2);
+ WRITE_SYSREG_LR(12, val);
break;
case 13:
- WRITE_SYSREG(val, ICH_LR13_EL2);
+ WRITE_SYSREG_LR(13, val);
break;
case 14:
- WRITE_SYSREG(val, ICH_LR14_EL2);
+ WRITE_SYSREG_LR(14, val);
break;
case 15:
- WRITE_SYSREG(val, ICH_LR15_EL2);
+ WRITE_SYSREG_LR(15, val);
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]=%llx\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]=%llx\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..f3b4dfbca8 100644
--- a/xen/arch/arm/include/asm/arm32/sysregs.h
+++ b/xen/arch/arm/include/asm/arm32/sysregs.h
@@ -62,9 +62,61 @@
#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) \
+ (READ_SYSREG(ICH_LRC_REG(INDEX)))) << 32) | \
+ (READ_SYSREG(ICH_LR_REG(INDEX))))
+
+#define WRITE_SYSREG_LR(INDEX, V) WRITE_SYSREG \
+ (V&0xFFFFFFFF, ICH_LR_REG(INDEX)); \
+ WRITE_SYSREG(V>>32, ICH_LRC_REG(INDEX));
+
/* MVFR2 is not defined on ARMv7 */
#define MVFR2_MAYBE_UNDEFINED
+#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 /* __ASSEMBLY__ */
#endif /* __ASM_ARM_ARM32_SYSREGS_H */
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
index 54670084c3..d45fe815f9 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -469,8 +469,11 @@
asm volatile("mrs %0, "__stringify(name) : "=r" (_r)); \
_r; })
-#define READ_SYSREG(name) READ_SYSREG64(name)
-#define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
+#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/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)
#define ICH_VTR_NRLRGS 0x3f
#define ICH_VTR_PRIBITS_MASK 0x7
--
2.17.1Hi Ayan,
Title: Xen doesn't emulate ICH_LR* (we don't expose them to the guest).
Instead Xen will use the registers and your patch provides wrappers to
use access the registers on 32-bit host.
On 21/10/2022 16:31, 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..f3b4dfbca8 100644
> --- a/xen/arch/arm/include/asm/arm32/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm32/sysregs.h
> @@ -62,9 +62,61 @@
> #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) \
> + (READ_SYSREG(ICH_LRC_REG(INDEX)))) << 32) | \
> + (READ_SYSREG(ICH_LR_REG(INDEX))))
This is a bit dense to read. Also, we should use READ_CP64() when
dealing with arm32 only code. So how about (formatting will need to be
done):
#define READ_SYSREG_LR(INDEX) ({ \
uint32_t lrc_ = READ_CP64(ICH_LRC_REG(INDEX)); \
uint32_t lr_ = READ_CP64(ICH_LR_REG(INDEX)); \
\
(uint64_t)(lrc_ << 32) | lr_;
})
> +
> +#define WRITE_SYSREG_LR(INDEX, V) WRITE_SYSREG \
> + (V&0xFFFFFFFF, ICH_LR_REG(INDEX)); \
> + WRITE_SYSREG(V>>32, ICH_LRC_REG(INDEX));
This code is fragile. If V is a function call, then you will call it
twice. You want something like:
do {
uint64_t v_ = (V);
WRITE_SYSREG(v_ & 0xFFFFFFFF, ICH_LR_REG(INDEX));
WRITE_SYSREG(v_ >> 32, ICH_LRC_REG(INDEX));
} while(0);
And maybe replacing the opencoding Fs with GENMASK.
> +
> /* MVFR2 is not defined on ARMv7 */
> #define MVFR2_MAYBE_UNDEFINED
>
> +#define ___CP32(a,b,c,d,e) a,b,c,d,e
I am not entirely sure why you need to define __CP32() here. However,
co-processors registers should be defined in asm/cpregs.h rather than
arm32/sysregs.h.
> +#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 /* __ASSEMBLY__ */
>
> #endif /* __ASM_ARM_ARM32_SYSREGS_H */
> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
> index 54670084c3..d45fe815f9 100644
> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> @@ -469,8 +469,11 @@
> asm volatile("mrs %0, "__stringify(name) : "=r" (_r)); \
> _r; })
>
> -#define READ_SYSREG(name) READ_SYSREG64(name)
> -#define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
> +#define READ_SYSREG(name) READ_SYSREG64(name)
> +#define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
Please don't re-indent existing macro. This is only introducing
unnecessary extra churn.
> +#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/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)
>
> #define ICH_VTR_NRLRGS 0x3f
> #define ICH_VTR_PRIBITS_MASK 0x7
Cheers,
--
Julien Grall
On 22/10/2022 12:03, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
I need a clarification.
>
> Title: Xen doesn't emulate ICH_LR* (we don't expose them to the
> guest). Instead Xen will use the registers and your patch provides
> wrappers to use access the registers on 32-bit host.
>
> On 21/10/2022 16:31, 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..f3b4dfbca8 100644
>> --- a/xen/arch/arm/include/asm/arm32/sysregs.h
>> +++ b/xen/arch/arm/include/asm/arm32/sysregs.h
>> @@ -62,9 +62,61 @@
>> #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) \
>> + (READ_SYSREG(ICH_LRC_REG(INDEX)))) << 32) | \
>> + (READ_SYSREG(ICH_LR_REG(INDEX))))
>
> This is a bit dense to read. Also, we should use READ_CP64() when
> dealing with arm32 only code. So how about (formatting will need to be
> done):
>
> #define READ_SYSREG_LR(INDEX) ({ \
> uint32_t lrc_ = READ_CP64(ICH_LRC_REG(INDEX)); \
> uint32_t lr_ = READ_CP64(ICH_LR_REG(INDEX)); \
> \
I think this looks incorrect. These are read using 'mrc' so they should
be READ_CP32(). They are 32 bit registers.
However, READ_SYSREG is defined as READ_CP32(), so should we use
READ_CP32() or READ_SYSREG() ?
- Ayan
> (uint64_t)(lrc_ << 32) | lr_;
> })
>
>> +
>> +#define WRITE_SYSREG_LR(INDEX, V) WRITE_SYSREG \
>> + (V&0xFFFFFFFF, ICH_LR_REG(INDEX)); \
>> + WRITE_SYSREG(V>>32,
>> ICH_LRC_REG(INDEX));
> This code is fragile. If V is a function call, then you will call it
> twice. You want something like:
>
> do {
> uint64_t v_ = (V);
>
> WRITE_SYSREG(v_ & 0xFFFFFFFF, ICH_LR_REG(INDEX));
> WRITE_SYSREG(v_ >> 32, ICH_LRC_REG(INDEX));
> } while(0);
>
> And maybe replacing the opencoding Fs with GENMASK.
>
>> +
>> /* MVFR2 is not defined on ARMv7 */
>> #define MVFR2_MAYBE_UNDEFINED
>> +#define ___CP32(a,b,c,d,e) a,b,c,d,e
>
> I am not entirely sure why you need to define __CP32() here. However,
> co-processors registers should be defined in asm/cpregs.h rather than
> arm32/sysregs.h.
>
>> +#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 /* __ASSEMBLY__ */
>> #endif /* __ASM_ARM_ARM32_SYSREGS_H */
>> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h
>> b/xen/arch/arm/include/asm/arm64/sysregs.h
>> index 54670084c3..d45fe815f9 100644
>> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
>> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
>> @@ -469,8 +469,11 @@
>> asm volatile("mrs %0, "__stringify(name) : "=r" (_r)); \
>> _r; })
>> -#define READ_SYSREG(name) READ_SYSREG64(name)
>> -#define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
>> +#define READ_SYSREG(name) READ_SYSREG64(name)
>> +#define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
>
> Please don't re-indent existing macro. This is only introducing
> unnecessary extra churn.
>
>> +#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/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)
>> #define ICH_VTR_NRLRGS 0x3f
>> #define ICH_VTR_PRIBITS_MASK 0x7
>
> Cheers,
>
On 28/10/2022 15:22, Ayan Kumar Halder wrote:
>
> On 22/10/2022 12:03, Julien Grall wrote:
>> Hi Ayan,
>
> Hi Julien,
>
> I need a clarification.
>
>>
>> Title: Xen doesn't emulate ICH_LR* (we don't expose them to the
>> guest). Instead Xen will use the registers and your patch provides
>> wrappers to use access the registers on 32-bit host.
>>
>> On 21/10/2022 16:31, 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..f3b4dfbca8 100644
>>> --- a/xen/arch/arm/include/asm/arm32/sysregs.h
>>> +++ b/xen/arch/arm/include/asm/arm32/sysregs.h
>>> @@ -62,9 +62,61 @@
>>> #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) \
>>> + (READ_SYSREG(ICH_LRC_REG(INDEX)))) << 32) | \
>>> + (READ_SYSREG(ICH_LR_REG(INDEX))))
>>
>> This is a bit dense to read. Also, we should use READ_CP64() when
>> dealing with arm32 only code. So how about (formatting will need to be
>> done):
>>
>> #define READ_SYSREG_LR(INDEX) ({ \
>> uint32_t lrc_ = READ_CP64(ICH_LRC_REG(INDEX)); \
>> uint32_t lr_ = READ_CP64(ICH_LR_REG(INDEX)); \
>> \
>
> I think this looks incorrect. These are read using 'mrc' so they should
> be READ_CP32(). They are 32 bit registers.
That's my mistake. We should use...
>
> However, READ_SYSREG is defined as READ_CP32(), so should we use
> READ_CP32() or READ_SYSREG() ?
READ_CP32() instead of READ_SYSREG() for arm32 specific code. The latter
is only provided to avoid #ifdef in the common code.
Cheers,
--
Julien Grall
© 2016 - 2026 Red Hat, Inc.