| Message ID | 20221128155649.31386-8-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Arm: Enable GICv3 for AArch32 | expand |
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
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,
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, >
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,
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
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(-)