| Message ID | 20231027012243.111070-1-ilkka@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm64/arm: perf: Don't discard upper 32 bits from PMCEID0/1 registers | expand |
On Thu, Oct 26, 2023 at 06:22:43PM -0700, Ilkka Koskinen wrote: > The upper 32 bits of PMCEID[n] registers are used to describe whether > architectural and microarchitectural events in range 0x4000-0x401f > exist. Due to discarding the bits, the driver made the events invisible, > even if they existed. > > Fixes: df29ddf4f04b ("arm64: perf: Abstract system register accesses away") Hmm... are we silently truncating anything else? I see that read_pmxevcntr() and write_pmxevcntr() treat PMXEVCNTR_EL0 as 32-bit despite it being 64-bit with PMUv3.5, but it doesn't matter as those functions are never used, and should just be deleted. Similarly PMCR_EL0 is 64-bit, with bit 32 bit FZS when FEAT_SPEv1p2 is implemented. That doesn't matter to us right now as we don't use FZS, but it's something that could catch us out in future if new bitss get added. Could we please go check all the accessors added in df29ddf4f04b, and fix those in one go? Mark. > Reported-by: Carl Worth <carl@os.amperecomputing.com> > Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> > --- > > Changes since v1: > > * Took arm32 specific code from Marc Zyngier's review comment > * Fixed a couple of typos in the commit message > > I have tested the patch on Arm64. However, what comes to Arm32 part, I have > only compared the code with Arm32 specification and cross compiled it. > > v1: https://lore.kernel.org/all/20231025200815.104017-1-ilkka@os.amperecomputing.com/ > > --- > > arch/arm/include/asm/arm_pmuv3.h | 33 +++++++++++++++++++++--------- > arch/arm64/include/asm/arm_pmuv3.h | 4 ++-- > 2 files changed, 25 insertions(+), 12 deletions(-) > > diff --git a/arch/arm/include/asm/arm_pmuv3.h b/arch/arm/include/asm/arm_pmuv3.h > index 72529f5e2bed..90841cb7ce43 100644 > --- a/arch/arm/include/asm/arm_pmuv3.h > +++ b/arch/arm/include/asm/arm_pmuv3.h > @@ -23,6 +23,8 @@ > #define PMUSERENR __ACCESS_CP15(c9, 0, c14, 0) > #define PMINTENSET __ACCESS_CP15(c9, 0, c14, 1) > #define PMINTENCLR __ACCESS_CP15(c9, 0, c14, 2) > +#define PMCEID2 __ACCESS_CP15(c9, 0, c14, 4) > +#define PMCEID3 __ACCESS_CP15(c9, 0, c14, 5) > #define PMMIR __ACCESS_CP15(c9, 0, c14, 6) > #define PMCCFILTR __ACCESS_CP15(c14, 0, c15, 7) > > @@ -205,16 +207,6 @@ static inline void write_pmuserenr(u32 val) > write_sysreg(val, PMUSERENR); > } > > -static inline u32 read_pmceid0(void) > -{ > - return read_sysreg(PMCEID0); > -} > - > -static inline u32 read_pmceid1(void) > -{ > - return read_sysreg(PMCEID1); > -} > - > static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {} > static inline void kvm_clr_pmu_events(u32 clr) {} > static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr) > @@ -231,6 +223,7 @@ static inline void kvm_vcpu_pmu_resync_el0(void) {} > > /* PMU Version in DFR Register */ > #define ARMV8_PMU_DFR_VER_NI 0 > +#define ARMV8_PMU_DFR_VER_V3P1 0x4 > #define ARMV8_PMU_DFR_VER_V3P4 0x5 > #define ARMV8_PMU_DFR_VER_V3P5 0x6 > #define ARMV8_PMU_DFR_VER_IMP_DEF 0xF > @@ -251,4 +244,24 @@ static inline bool is_pmuv3p5(int pmuver) > return pmuver >= ARMV8_PMU_DFR_VER_V3P5; > } > > +static inline u64 read_pmceid0(void) > +{ > + u64 val = read_sysreg(PMCEID0); > + > + if (read_pmuver() >= ARMV8_PMU_DFR_VER_V3P1) > + val |= (u64)read_sysreg(PMCEID2) << 32; > + > + return val; > +} > + > +static inline u64 read_pmceid1(void) > +{ > + u64 val = read_sysreg(PMCEID1); > + > + if (read_pmuver() >= ARMV8_PMU_DFR_VER_V3P1) > + val |= (u64)read_sysreg(PMCEID3) << 32; > + > + return val; > +} > + > #endif > diff --git a/arch/arm64/include/asm/arm_pmuv3.h b/arch/arm64/include/asm/arm_pmuv3.h > index 18dc2fb3d7b7..3e92b7cb57a4 100644 > --- a/arch/arm64/include/asm/arm_pmuv3.h > +++ b/arch/arm64/include/asm/arm_pmuv3.h > @@ -126,12 +126,12 @@ static inline void write_pmuserenr(u32 val) > write_sysreg(val, pmuserenr_el0); > } > > -static inline u32 read_pmceid0(void) > +static inline u64 read_pmceid0(void) > { > return read_sysreg(pmceid0_el0); > } > > -static inline u32 read_pmceid1(void) > +static inline u64 read_pmceid1(void) > { > return read_sysreg(pmceid1_el0); > } > -- > 2.41.0 >
Hi Mark, On Fri, 27 Oct 2023, Mark Rutland wrote: > On Thu, Oct 26, 2023 at 06:22:43PM -0700, Ilkka Koskinen wrote: >> The upper 32 bits of PMCEID[n] registers are used to describe whether >> architectural and microarchitectural events in range 0x4000-0x401f >> exist. Due to discarding the bits, the driver made the events invisible, >> even if they existed. >> >> Fixes: df29ddf4f04b ("arm64: perf: Abstract system register accesses away") > > Hmm... are we silently truncating anything else? > > I see that read_pmxevcntr() and write_pmxevcntr() treat PMXEVCNTR_EL0 as 32-bit > despite it being 64-bit with PMUv3.5, but it doesn't matter as those functions > are never used, and should just be deleted. > > Similarly PMCR_EL0 is 64-bit, with bit 32 bit FZS when FEAT_SPEv1p2 is > implemented. That doesn't matter to us right now as we don't use FZS, but it's > something that could catch us out in future if new bitss get added. > > Could we please go check all the accessors added in df29ddf4f04b, and fix those > in one go? I quickly look at the registers and unless I'm mistaken there's just one more register, PMCCFILTR_EL0, that needs to be taken care. It seems to have VS bits [57:56], if FEAT_PMUv3_SME is implemented. I'll try to submit a new patch that handles all of them tomorrow. Cheers, Ilkka > >> Reported-by: Carl Worth <carl@os.amperecomputing.com> >> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> >> --- >> >> Changes since v1: >> >> * Took arm32 specific code from Marc Zyngier's review comment >> * Fixed a couple of typos in the commit message >> >> I have tested the patch on Arm64. However, what comes to Arm32 part, I have >> only compared the code with Arm32 specification and cross compiled it. >> >> v1: https://lore.kernel.org/all/20231025200815.104017-1-ilkka@os.amperecomputing.com/ >> >> --- >> >> arch/arm/include/asm/arm_pmuv3.h | 33 +++++++++++++++++++++--------- >> arch/arm64/include/asm/arm_pmuv3.h | 4 ++-- >> 2 files changed, 25 insertions(+), 12 deletions(-) >> >> diff --git a/arch/arm/include/asm/arm_pmuv3.h b/arch/arm/include/asm/arm_pmuv3.h >> index 72529f5e2bed..90841cb7ce43 100644 >> --- a/arch/arm/include/asm/arm_pmuv3.h >> +++ b/arch/arm/include/asm/arm_pmuv3.h >> @@ -23,6 +23,8 @@ >> #define PMUSERENR __ACCESS_CP15(c9, 0, c14, 0) >> #define PMINTENSET __ACCESS_CP15(c9, 0, c14, 1) >> #define PMINTENCLR __ACCESS_CP15(c9, 0, c14, 2) >> +#define PMCEID2 __ACCESS_CP15(c9, 0, c14, 4) >> +#define PMCEID3 __ACCESS_CP15(c9, 0, c14, 5) >> #define PMMIR __ACCESS_CP15(c9, 0, c14, 6) >> #define PMCCFILTR __ACCESS_CP15(c14, 0, c15, 7) >> >> @@ -205,16 +207,6 @@ static inline void write_pmuserenr(u32 val) >> write_sysreg(val, PMUSERENR); >> } >> >> -static inline u32 read_pmceid0(void) >> -{ >> - return read_sysreg(PMCEID0); >> -} >> - >> -static inline u32 read_pmceid1(void) >> -{ >> - return read_sysreg(PMCEID1); >> -} >> - >> static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {} >> static inline void kvm_clr_pmu_events(u32 clr) {} >> static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr) >> @@ -231,6 +223,7 @@ static inline void kvm_vcpu_pmu_resync_el0(void) {} >> >> /* PMU Version in DFR Register */ >> #define ARMV8_PMU_DFR_VER_NI 0 >> +#define ARMV8_PMU_DFR_VER_V3P1 0x4 >> #define ARMV8_PMU_DFR_VER_V3P4 0x5 >> #define ARMV8_PMU_DFR_VER_V3P5 0x6 >> #define ARMV8_PMU_DFR_VER_IMP_DEF 0xF >> @@ -251,4 +244,24 @@ static inline bool is_pmuv3p5(int pmuver) >> return pmuver >= ARMV8_PMU_DFR_VER_V3P5; >> } >> >> +static inline u64 read_pmceid0(void) >> +{ >> + u64 val = read_sysreg(PMCEID0); >> + >> + if (read_pmuver() >= ARMV8_PMU_DFR_VER_V3P1) >> + val |= (u64)read_sysreg(PMCEID2) << 32; >> + >> + return val; >> +} >> + >> +static inline u64 read_pmceid1(void) >> +{ >> + u64 val = read_sysreg(PMCEID1); >> + >> + if (read_pmuver() >= ARMV8_PMU_DFR_VER_V3P1) >> + val |= (u64)read_sysreg(PMCEID3) << 32; >> + >> + return val; >> +} >> + >> #endif >> diff --git a/arch/arm64/include/asm/arm_pmuv3.h b/arch/arm64/include/asm/arm_pmuv3.h >> index 18dc2fb3d7b7..3e92b7cb57a4 100644 >> --- a/arch/arm64/include/asm/arm_pmuv3.h >> +++ b/arch/arm64/include/asm/arm_pmuv3.h >> @@ -126,12 +126,12 @@ static inline void write_pmuserenr(u32 val) >> write_sysreg(val, pmuserenr_el0); >> } >> >> -static inline u32 read_pmceid0(void) >> +static inline u64 read_pmceid0(void) >> { >> return read_sysreg(pmceid0_el0); >> } >> >> -static inline u32 read_pmceid1(void) >> +static inline u64 read_pmceid1(void) >> { >> return read_sysreg(pmceid1_el0); >> } >> -- >> 2.41.0 >> >
diff --git a/arch/arm/include/asm/arm_pmuv3.h b/arch/arm/include/asm/arm_pmuv3.h index 72529f5e2bed..90841cb7ce43 100644 --- a/arch/arm/include/asm/arm_pmuv3.h +++ b/arch/arm/include/asm/arm_pmuv3.h @@ -23,6 +23,8 @@ #define PMUSERENR __ACCESS_CP15(c9, 0, c14, 0) #define PMINTENSET __ACCESS_CP15(c9, 0, c14, 1) #define PMINTENCLR __ACCESS_CP15(c9, 0, c14, 2) +#define PMCEID2 __ACCESS_CP15(c9, 0, c14, 4) +#define PMCEID3 __ACCESS_CP15(c9, 0, c14, 5) #define PMMIR __ACCESS_CP15(c9, 0, c14, 6) #define PMCCFILTR __ACCESS_CP15(c14, 0, c15, 7) @@ -205,16 +207,6 @@ static inline void write_pmuserenr(u32 val) write_sysreg(val, PMUSERENR); } -static inline u32 read_pmceid0(void) -{ - return read_sysreg(PMCEID0); -} - -static inline u32 read_pmceid1(void) -{ - return read_sysreg(PMCEID1); -} - static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {} static inline void kvm_clr_pmu_events(u32 clr) {} static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr) @@ -231,6 +223,7 @@ static inline void kvm_vcpu_pmu_resync_el0(void) {} /* PMU Version in DFR Register */ #define ARMV8_PMU_DFR_VER_NI 0 +#define ARMV8_PMU_DFR_VER_V3P1 0x4 #define ARMV8_PMU_DFR_VER_V3P4 0x5 #define ARMV8_PMU_DFR_VER_V3P5 0x6 #define ARMV8_PMU_DFR_VER_IMP_DEF 0xF @@ -251,4 +244,24 @@ static inline bool is_pmuv3p5(int pmuver) return pmuver >= ARMV8_PMU_DFR_VER_V3P5; } +static inline u64 read_pmceid0(void) +{ + u64 val = read_sysreg(PMCEID0); + + if (read_pmuver() >= ARMV8_PMU_DFR_VER_V3P1) + val |= (u64)read_sysreg(PMCEID2) << 32; + + return val; +} + +static inline u64 read_pmceid1(void) +{ + u64 val = read_sysreg(PMCEID1); + + if (read_pmuver() >= ARMV8_PMU_DFR_VER_V3P1) + val |= (u64)read_sysreg(PMCEID3) << 32; + + return val; +} + #endif diff --git a/arch/arm64/include/asm/arm_pmuv3.h b/arch/arm64/include/asm/arm_pmuv3.h index 18dc2fb3d7b7..3e92b7cb57a4 100644 --- a/arch/arm64/include/asm/arm_pmuv3.h +++ b/arch/arm64/include/asm/arm_pmuv3.h @@ -126,12 +126,12 @@ static inline void write_pmuserenr(u32 val) write_sysreg(val, pmuserenr_el0); } -static inline u32 read_pmceid0(void) +static inline u64 read_pmceid0(void) { return read_sysreg(pmceid0_el0); } -static inline u32 read_pmceid1(void) +static inline u64 read_pmceid1(void) { return read_sysreg(pmceid1_el0); }
The upper 32 bits of PMCEID[n] registers are used to describe whether architectural and microarchitectural events in range 0x4000-0x401f exist. Due to discarding the bits, the driver made the events invisible, even if they existed. Fixes: df29ddf4f04b ("arm64: perf: Abstract system register accesses away") Reported-by: Carl Worth <carl@os.amperecomputing.com> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> --- Changes since v1: * Took arm32 specific code from Marc Zyngier's review comment * Fixed a couple of typos in the commit message I have tested the patch on Arm64. However, what comes to Arm32 part, I have only compared the code with Arm32 specification and cross compiled it. v1: https://lore.kernel.org/all/20231025200815.104017-1-ilkka@os.amperecomputing.com/ --- arch/arm/include/asm/arm_pmuv3.h | 33 +++++++++++++++++++++--------- arch/arm64/include/asm/arm_pmuv3.h | 4 ++-- 2 files changed, 25 insertions(+), 12 deletions(-)