| Message ID | 20221031151326.22634-8-ayankuma@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Arm: Enable GICv3 for AArch32 (v8R) | expand |
Hi Ayan, On 31/10/2022 16:13, Ayan Kumar Halder wrote: > > > Refer "Arm IHI 0069H ID020922", 12.4.6, Interrupt Controller List Registers > > AArch64 System register ICH_LR<n>_EL2 bits [31:0] are architecturally > mapped to AArch32 System register ICH_LR<n>[31:0]. > AArch64 System register ICH_LR<n>_EL2 bits [63:32] are architecturally > mapped to AArch32 System register ICH_LRC<n>[31:0]. > > Defined ICH_LR<0...15>_EL2 and ICH_LRC<0...15>_EL2 for Aarch32. > For AArch32, the link register is stored as :- > (((uint64_t) ICH_LRC<0...15>_EL2) << 32) | ICH_LR<0...15>_EL2 > > Also, ICR_LR macros need to be modified as ULL is 64 bits for AArch32 and > AArch64. > > Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> > --- > > Changes from :- > v1 - 1. Moved the coproc register definitions to asm/cpregs.h. > 2. Use GENMASK(31, 0) to represent 0xFFFFFFFF > 3. Use READ_CP32()/WRITE_CP32() instead of READ_SYSREG()/WRITE_SYSREG(). > 4. Multi-line macro definitions should be enclosed within ({ }). > > xen/arch/arm/gic-v3.c | 132 +++++++++++------------ > xen/arch/arm/include/asm/arm32/sysregs.h | 17 +++ > xen/arch/arm/include/asm/arm64/sysregs.h | 3 + > xen/arch/arm/include/asm/cpregs.h | 42 ++++++++ > xen/arch/arm/include/asm/gic_v3_defs.h | 6 +- > 5 files changed, 131 insertions(+), 69 deletions(-) > > diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h b/xen/arch/arm/include/asm/arm32/sysregs.h > index 6841d5de43..8a9a014bef 100644 > --- a/xen/arch/arm/include/asm/arm32/sysregs.h > +++ b/xen/arch/arm/include/asm/arm32/sysregs.h > @@ -62,6 +62,23 @@ > #define READ_SYSREG(R...) READ_SYSREG32(R) > #define WRITE_SYSREG(V, R...) WRITE_SYSREG32(V, R) > > +#define ICH_LR_REG(INDEX) ICH_LR ## INDEX ## _EL2 > +#define ICH_LRC_REG(INDEX) ICH_LRC ## INDEX ## _EL2 You could align to WRITE_SYSREG32(V, R). Apart from that it would be good to add some comment before the code you added (ICH_LR_REG) to separate from the code above and its comment about registers coming in 3 types. Something like: /* Wrappers for accessing interrupt controller list registers. */ > + > +#define READ_SYSREG_LR(INDEX) ({ \ Opening ({ should be placed one space after READ_SYSREG_LR(INDEX). It does not need to be aligned. > + uint64_t _val; \ val is not really necessary. You could directly return the ((uint64_t) _lrc << 32) | _lr; Just something to consider, no need to replace. > + uint32_t _lrc = READ_CP32(ICH_LRC_REG(INDEX)); \ > + uint32_t _lr = READ_CP32(ICH_LR_REG(INDEX)); \ > + \ > + _val = ((uint64_t) _lrc << 32) | _lr; \ > + _val; }) Here, you did put }) at the same line... > + > +#define WRITE_SYSREG_LR(INDEX, V) ({ \ > + uint64_t _val = (V); \ > + WRITE_CP32(_val & GENMASK(31, 0), ICH_LR_REG(INDEX)); \ > + WRITE_CP32(_val >> 32, ICH_LRC_REG(INDEX)); \ Please, align \ > +}); ... and here you did not. > + > /* MVFR2 is not defined on ARMv7 */ > #define MVFR2_MAYBE_UNDEFINED > > diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h > index 54670084c3..353f0eea29 100644 > --- a/xen/arch/arm/include/asm/arm64/sysregs.h > +++ b/xen/arch/arm/include/asm/arm64/sysregs.h > @@ -471,6 +471,9 @@ > > #define READ_SYSREG(name) READ_SYSREG64(name) > #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name) Here, I would separate the below macros by adding the comment similar to the one I showed above. Or at least add a blank line. > +#define ICH_LR_REG(index) ICH_LR ## index ## _EL2 > +#define WRITE_SYSREG_LR(index, v) WRITE_SYSREG(v, ICH_LR_REG(index)) > +#define READ_SYSREG_LR(index) READ_SYSREG(ICH_LR_REG(index)) I find it a bit odd that the macro param 'index' is written in lower case and for arm32 in upper case. > > #endif /* _ASM_ARM_ARM64_SYSREGS_H */ > > diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h > index 6daf2b1a30..4421dd49ac 100644 > --- a/xen/arch/arm/include/asm/cpregs.h > +++ b/xen/arch/arm/include/asm/cpregs.h > @@ -362,6 +362,48 @@ > #define MVFR0_EL1 MVFR0 > #define MVFR1_EL1 MVFR1 > #define MVFR2_EL1 MVFR2 > + You could align everything below to MVFR2. Also maybe you could add some comment stating that the below relates to GIC system registers? > +#define ___CP32(a,b,c,d,e) a,b,c,d,e > +#define __LR0_EL2(x) ___CP32(p15,4,c12,c12,x) > +#define __LR8_EL2(x) ___CP32(p15,4,c12,c13,x) > + > +#define __LRC0_EL2(x) ___CP32(p15,4,c12,c14,x) > +#define __LRC8_EL2(x) ___CP32(p15,4,c12,c15,x) > + > +#define ICH_LR0_EL2 __LR0_EL2(0) > +#define ICH_LR1_EL2 __LR0_EL2(1) > +#define ICH_LR2_EL2 __LR0_EL2(2) > +#define ICH_LR3_EL2 __LR0_EL2(3) > +#define ICH_LR4_EL2 __LR0_EL2(4) > +#define ICH_LR5_EL2 __LR0_EL2(5) > +#define ICH_LR6_EL2 __LR0_EL2(6) > +#define ICH_LR7_EL2 __LR0_EL2(7) > +#define ICH_LR8_EL2 __LR8_EL2(0) > +#define ICH_LR9_EL2 __LR8_EL2(1) > +#define ICH_LR10_EL2 __LR8_EL2(2) > +#define ICH_LR11_EL2 __LR8_EL2(3) > +#define ICH_LR12_EL2 __LR8_EL2(4) > +#define ICH_LR13_EL2 __LR8_EL2(5) > +#define ICH_LR14_EL2 __LR8_EL2(6) > +#define ICH_LR15_EL2 __LR8_EL2(7) > + > +#define ICH_LRC0_EL2 __LRC0_EL2(0) > +#define ICH_LRC1_EL2 __LRC0_EL2(1) > +#define ICH_LRC2_EL2 __LRC0_EL2(2) > +#define ICH_LRC3_EL2 __LRC0_EL2(3) > +#define ICH_LRC4_EL2 __LRC0_EL2(4) > +#define ICH_LRC5_EL2 __LRC0_EL2(5) > +#define ICH_LRC6_EL2 __LRC0_EL2(6) > +#define ICH_LRC7_EL2 __LRC0_EL2(7) > +#define ICH_LRC8_EL2 __LRC8_EL2(0) > +#define ICH_LRC9_EL2 __LRC8_EL2(1) > +#define ICH_LRC10_EL2 __LRC8_EL2(2) > +#define ICH_LRC11_EL2 __LRC8_EL2(3) > +#define ICH_LRC12_EL2 __LRC8_EL2(4) > +#define ICH_LRC13_EL2 __LRC8_EL2(5) > +#define ICH_LRC14_EL2 __LRC8_EL2(6) > +#define ICH_LRC15_EL2 __LRC8_EL2(7) > + > #endif > > #endif > diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h > index 48a1bc401e..87115f8b25 100644 > --- a/xen/arch/arm/include/asm/gic_v3_defs.h > +++ b/xen/arch/arm/include/asm/gic_v3_defs.h > @@ -185,9 +185,9 @@ > #define ICH_LR_HW_SHIFT 61 > #define ICH_LR_GRP_MASK 0x1 > #define ICH_LR_GRP_SHIFT 60 > -#define ICH_LR_MAINTENANCE_IRQ (1UL<<41) > -#define ICH_LR_GRP1 (1UL<<60) > -#define ICH_LR_HW (1UL<<61) > +#define ICH_LR_MAINTENANCE_IRQ (1ULL<<41) > +#define ICH_LR_GRP1 (1ULL<<60) > +#define ICH_LR_HW (1ULL<<61) You could take the opportunity to add spaces between << to adhere to similar uses in this file. > > #define ICH_VTR_NRLRGS 0x3f > #define ICH_VTR_PRIBITS_MASK 0x7 > -- > 2.17.1 > > ~Michal
Hi, On 03/11/2022 12:13, Michal Orzel wrote: > Hi Ayan, > > On 31/10/2022 16:13, Ayan Kumar Halder wrote: >> >> >> Refer "Arm IHI 0069H ID020922", 12.4.6, Interrupt Controller List Registers >> >> AArch64 System register ICH_LR<n>_EL2 bits [31:0] are architecturally >> mapped to AArch32 System register ICH_LR<n>[31:0]. >> AArch64 System register ICH_LR<n>_EL2 bits [63:32] are architecturally >> mapped to AArch32 System register ICH_LRC<n>[31:0]. >> >> Defined ICH_LR<0...15>_EL2 and ICH_LRC<0...15>_EL2 for Aarch32. >> For AArch32, the link register is stored as :- >> (((uint64_t) ICH_LRC<0...15>_EL2) << 32) | ICH_LR<0...15>_EL2 >> >> Also, ICR_LR macros need to be modified as ULL is 64 bits for AArch32 and >> AArch64. >> >> Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> >> --- >> >> Changes from :- >> v1 - 1. Moved the coproc register definitions to asm/cpregs.h. >> 2. Use GENMASK(31, 0) to represent 0xFFFFFFFF >> 3. Use READ_CP32()/WRITE_CP32() instead of READ_SYSREG()/WRITE_SYSREG(). >> 4. Multi-line macro definitions should be enclosed within ({ }). >> >> xen/arch/arm/gic-v3.c | 132 +++++++++++------------ >> xen/arch/arm/include/asm/arm32/sysregs.h | 17 +++ >> xen/arch/arm/include/asm/arm64/sysregs.h | 3 + >> xen/arch/arm/include/asm/cpregs.h | 42 ++++++++ >> xen/arch/arm/include/asm/gic_v3_defs.h | 6 +- >> 5 files changed, 131 insertions(+), 69 deletions(-) >> >> diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h b/xen/arch/arm/include/asm/arm32/sysregs.h >> index 6841d5de43..8a9a014bef 100644 >> --- a/xen/arch/arm/include/asm/arm32/sysregs.h >> +++ b/xen/arch/arm/include/asm/arm32/sysregs.h >> @@ -62,6 +62,23 @@ >> #define READ_SYSREG(R...) READ_SYSREG32(R) >> #define WRITE_SYSREG(V, R...) WRITE_SYSREG32(V, R) >> >> +#define ICH_LR_REG(INDEX) ICH_LR ## INDEX ## _EL2 >> +#define ICH_LRC_REG(INDEX) ICH_LRC ## INDEX ## _EL2 > You could align to WRITE_SYSREG32(V, R). > > Apart from that it would be good to add some comment before the code you added (ICH_LR_REG) > to separate from the code above and its comment about registers coming in 3 types. > Something like: > /* Wrappers for accessing interrupt controller list registers. */ > >> + >> +#define READ_SYSREG_LR(INDEX) ({ \ > Opening ({ should be placed one space after READ_SYSREG_LR(INDEX). It does not need to be aligned. > >> + uint64_t _val; \ > val is not really necessary. You could directly return the ((uint64_t) _lrc << 32) | _lr; > Just something to consider, no need to replace. > >> + uint32_t _lrc = READ_CP32(ICH_LRC_REG(INDEX)); \ >> + uint32_t _lr = READ_CP32(ICH_LR_REG(INDEX)); \ >> + \ >> + _val = ((uint64_t) _lrc << 32) | _lr; \ >> + _val; }) > Here, you did put }) at the same line... > >> + >> +#define WRITE_SYSREG_LR(INDEX, V) ({ \ >> + uint64_t _val = (V); \ >> + WRITE_CP32(_val & GENMASK(31, 0), ICH_LR_REG(INDEX)); \ >> + WRITE_CP32(_val >> 32, ICH_LRC_REG(INDEX)); \ > Please, align \ +1 > >> +}); > ... and here you did not. FAOD, this is the correct approach. That said, the ';' should not be added. > >> + >> /* MVFR2 is not defined on ARMv7 */ >> #define MVFR2_MAYBE_UNDEFINED >> >> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h >> index 54670084c3..353f0eea29 100644 >> --- a/xen/arch/arm/include/asm/arm64/sysregs.h >> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h >> @@ -471,6 +471,9 @@ >> >> #define READ_SYSREG(name) READ_SYSREG64(name) >> #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name) > Here, I would separate the below macros by adding the comment similar to the one I showed above. > Or at least add a blank line. > >> +#define ICH_LR_REG(index) ICH_LR ## index ## _EL2 >> +#define WRITE_SYSREG_LR(index, v) WRITE_SYSREG(v, ICH_LR_REG(index)) >> +#define READ_SYSREG_LR(index) READ_SYSREG(ICH_LR_REG(index)) > I find it a bit odd that the macro param 'index' is written in lower case and for arm32 in upper cas FWIW, I would prefer the lower case version. That said, the arm32 code match with the rest of the file. Cheers,
Hi Ayan, On 31/10/2022 15:13, 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..8a9a014bef 100644 > --- a/xen/arch/arm/include/asm/arm32/sysregs.h > +++ b/xen/arch/arm/include/asm/arm32/sysregs.h > @@ -62,6 +62,23 @@ > #define READ_SYSREG(R...) READ_SYSREG32(R) > #define WRITE_SYSREG(V, R...) WRITE_SYSREG32(V, R) > > +#define ICH_LR_REG(INDEX) ICH_LR ## INDEX ## _EL2 > +#define ICH_LRC_REG(INDEX) ICH_LRC ## INDEX ## _EL2 > + > +#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(INDEX, V) ({ \ I would prefer if the prototype stays consistent with the other write helpers. I.e. the value is first and the name second. > + 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..353f0eea29 100644 > --- a/xen/arch/arm/include/asm/arm64/sysregs.h > +++ b/xen/arch/arm/include/asm/arm64/sysregs.h > @@ -471,6 +471,9 @@ > > #define READ_SYSREG(name) READ_SYSREG64(name) > #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name) > +#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/cpregs.h b/xen/arch/arm/include/asm/cpregs.h > index 6daf2b1a30..4421dd49ac 100644 > --- a/xen/arch/arm/include/asm/cpregs.h > +++ b/xen/arch/arm/include/asm/cpregs.h > @@ -362,6 +362,48 @@ > #define MVFR0_EL1 MVFR0 > #define MVFR1_EL1 MVFR1 > #define MVFR2_EL1 MVFR2 > + > +#define ___CP32(a,b,c,d,e) a,b,c,d,e Unless there are a reason to do, please use space after each comma. The same goes for the rest of the patch. Cheers,
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..8a9a014bef 100644 --- a/xen/arch/arm/include/asm/arm32/sysregs.h +++ b/xen/arch/arm/include/asm/arm32/sysregs.h @@ -62,6 +62,23 @@ #define READ_SYSREG(R...) READ_SYSREG32(R) #define WRITE_SYSREG(V, R...) WRITE_SYSREG32(V, R) +#define ICH_LR_REG(INDEX) ICH_LR ## INDEX ## _EL2 +#define ICH_LRC_REG(INDEX) ICH_LRC ## INDEX ## _EL2 + +#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(INDEX, V) ({ \ + 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..353f0eea29 100644 --- a/xen/arch/arm/include/asm/arm64/sysregs.h +++ b/xen/arch/arm/include/asm/arm64/sysregs.h @@ -471,6 +471,9 @@ #define READ_SYSREG(name) READ_SYSREG64(name) #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name) +#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/cpregs.h b/xen/arch/arm/include/asm/cpregs.h index 6daf2b1a30..4421dd49ac 100644 --- a/xen/arch/arm/include/asm/cpregs.h +++ b/xen/arch/arm/include/asm/cpregs.h @@ -362,6 +362,48 @@ #define MVFR0_EL1 MVFR0 #define MVFR1_EL1 MVFR1 #define MVFR2_EL1 MVFR2 + +#define ___CP32(a,b,c,d,e) a,b,c,d,e +#define __LR0_EL2(x) ___CP32(p15,4,c12,c12,x) +#define __LR8_EL2(x) ___CP32(p15,4,c12,c13,x) + +#define __LRC0_EL2(x) ___CP32(p15,4,c12,c14,x) +#define __LRC8_EL2(x) ___CP32(p15,4,c12,c15,x) + +#define ICH_LR0_EL2 __LR0_EL2(0) +#define ICH_LR1_EL2 __LR0_EL2(1) +#define ICH_LR2_EL2 __LR0_EL2(2) +#define ICH_LR3_EL2 __LR0_EL2(3) +#define ICH_LR4_EL2 __LR0_EL2(4) +#define ICH_LR5_EL2 __LR0_EL2(5) +#define ICH_LR6_EL2 __LR0_EL2(6) +#define ICH_LR7_EL2 __LR0_EL2(7) +#define ICH_LR8_EL2 __LR8_EL2(0) +#define ICH_LR9_EL2 __LR8_EL2(1) +#define ICH_LR10_EL2 __LR8_EL2(2) +#define ICH_LR11_EL2 __LR8_EL2(3) +#define ICH_LR12_EL2 __LR8_EL2(4) +#define ICH_LR13_EL2 __LR8_EL2(5) +#define ICH_LR14_EL2 __LR8_EL2(6) +#define ICH_LR15_EL2 __LR8_EL2(7) + +#define ICH_LRC0_EL2 __LRC0_EL2(0) +#define ICH_LRC1_EL2 __LRC0_EL2(1) +#define ICH_LRC2_EL2 __LRC0_EL2(2) +#define ICH_LRC3_EL2 __LRC0_EL2(3) +#define ICH_LRC4_EL2 __LRC0_EL2(4) +#define ICH_LRC5_EL2 __LRC0_EL2(5) +#define ICH_LRC6_EL2 __LRC0_EL2(6) +#define ICH_LRC7_EL2 __LRC0_EL2(7) +#define ICH_LRC8_EL2 __LRC8_EL2(0) +#define ICH_LRC9_EL2 __LRC8_EL2(1) +#define ICH_LRC10_EL2 __LRC8_EL2(2) +#define ICH_LRC11_EL2 __LRC8_EL2(3) +#define ICH_LRC12_EL2 __LRC8_EL2(4) +#define ICH_LRC13_EL2 __LRC8_EL2(5) +#define ICH_LRC14_EL2 __LRC8_EL2(6) +#define ICH_LRC15_EL2 __LRC8_EL2(7) + #endif #endif diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h index 48a1bc401e..87115f8b25 100644 --- a/xen/arch/arm/include/asm/gic_v3_defs.h +++ b/xen/arch/arm/include/asm/gic_v3_defs.h @@ -185,9 +185,9 @@ #define ICH_LR_HW_SHIFT 61 #define ICH_LR_GRP_MASK 0x1 #define ICH_LR_GRP_SHIFT 60 -#define ICH_LR_MAINTENANCE_IRQ (1UL<<41) -#define ICH_LR_GRP1 (1UL<<60) -#define ICH_LR_HW (1UL<<61) +#define ICH_LR_MAINTENANCE_IRQ (1ULL<<41) +#define ICH_LR_GRP1 (1ULL<<60) +#define ICH_LR_HW (1ULL<<61) #define ICH_VTR_NRLRGS 0x3f #define ICH_VTR_PRIBITS_MASK 0x7
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> --- Changes from :- v1 - 1. Moved the coproc register definitions to asm/cpregs.h. 2. Use GENMASK(31, 0) to represent 0xFFFFFFFF 3. Use READ_CP32()/WRITE_CP32() instead of READ_SYSREG()/WRITE_SYSREG(). 4. Multi-line macro definitions should be enclosed within ({ }). xen/arch/arm/gic-v3.c | 132 +++++++++++------------ xen/arch/arm/include/asm/arm32/sysregs.h | 17 +++ xen/arch/arm/include/asm/arm64/sysregs.h | 3 + xen/arch/arm/include/asm/cpregs.h | 42 ++++++++ xen/arch/arm/include/asm/gic_v3_defs.h | 6 +- 5 files changed, 131 insertions(+), 69 deletions(-)