Messages in this thread | | | Date | Fri, 3 Nov 2023 11:39:36 +0530 | Subject | Re: [PATCH v3] arm64/arm: arm_pmuv3: perf: Don't truncate 64-bit registers | From | Anshuman Khandual <> |
| |
On 11/3/23 00:00, Ilkka Koskinen wrote: > The driver used to truncate several 64-bit registers such as PMCEID[n] > registers 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. > > Moreover, PMCCFILTR and PMCR registers have additional bits in the upper > 32 bits. This patch makes them available although they aren't currently > used. Finally, functions handling PMXEVCNTR and PMXEVTYPER registers are > removed as they not being used at all. > > Fixes: df29ddf4f04b ("arm64: perf: Abstract system register accesses away") > Reported-by: Carl Worth <carl@os.amperecomputing.com>
This needs an URL for the original bug report in the following format.
Reported-by: Carl Worth <carl@os.amperecomputing.com> Closes: https://lore.kernel.org/..
Otherwise, the following checkpatch warning shows up.
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #17: Reported-by: Carl Worth <carl@os.amperecomputing.com> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> > --- > > v2: > > * 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. > > * https://lore.kernel.org/all/20231027012243.111070-1-ilkka@os.amperecomputing.com/ > > v3: > > * Removed read/write_pmxevcntr() and read/write_pmxevtyper() as suggested > by Mark Rutland > * Changed handling of PMCCFILTR and PMCR to 64-bit on Aarch64. Aarch32 doesn't > seem to use the upper 32 bits. > > > Tested the patch on Arm64. Arm32 version was only built and not tested on actual > hardware. > > --- > > arch/arm/include/asm/arm_pmuv3.h | 48 ++++++++++++++---------------- > arch/arm64/include/asm/arm_pmuv3.h | 25 ++++------------ > drivers/perf/arm_pmuv3.c | 6 ++-- > 3 files changed, 31 insertions(+), 48 deletions(-) > > diff --git a/arch/arm/include/asm/arm_pmuv3.h b/arch/arm/include/asm/arm_pmuv3.h > index 72529f5e2bed..a41b503b7dcd 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) > > @@ -150,21 +152,6 @@ static inline u64 read_pmccntr(void) > return read_sysreg(PMCCNTR); > } > > -static inline void write_pmxevcntr(u32 val) > -{ > - write_sysreg(val, PMXEVCNTR); > -} > - > -static inline u32 read_pmxevcntr(void) > -{ > - return read_sysreg(PMXEVCNTR); > -} > - > -static inline void write_pmxevtyper(u32 val) > -{ > - write_sysreg(val, PMXEVTYPER); > -} > - > static inline void write_pmcntenset(u32 val) > { > write_sysreg(val, PMCNTENSET); > @@ -205,16 +192,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 +208,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 +229,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..c27404fa4418 100644 > --- a/arch/arm64/include/asm/arm_pmuv3.h > +++ b/arch/arm64/include/asm/arm_pmuv3.h > @@ -46,12 +46,12 @@ static inline u32 read_pmuver(void) > ID_AA64DFR0_EL1_PMUVer_SHIFT); > } > > -static inline void write_pmcr(u32 val) > +static inline void write_pmcr(u64 val) > { > write_sysreg(val, pmcr_el0); > } > > -static inline u32 read_pmcr(void) > +static inline u64 read_pmcr(void) > { > return read_sysreg(pmcr_el0); > } > @@ -71,21 +71,6 @@ static inline u64 read_pmccntr(void) > return read_sysreg(pmccntr_el0); > } > > -static inline void write_pmxevcntr(u32 val) > -{ > - write_sysreg(val, pmxevcntr_el0); > -} > - > -static inline u32 read_pmxevcntr(void) > -{ > - return read_sysreg(pmxevcntr_el0); > -} > - > -static inline void write_pmxevtyper(u32 val) > -{ > - write_sysreg(val, pmxevtyper_el0); > -} > - > static inline void write_pmcntenset(u32 val) > { > write_sysreg(val, pmcntenset_el0); > @@ -106,7 +91,7 @@ static inline void write_pmintenclr(u32 val) > write_sysreg(val, pmintenclr_el1); > } > > -static inline void write_pmccfiltr(u32 val) > +static inline void write_pmccfiltr(u64 val) > { > write_sysreg(val, pmccfiltr_el0); > } > @@ -126,12 +111,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); > } > diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c > index 18b91b56af1d..6ca7be05229c 100644 > --- a/drivers/perf/arm_pmuv3.c > +++ b/drivers/perf/arm_pmuv3.c > @@ -428,12 +428,12 @@ static inline bool armv8pmu_event_is_chained(struct perf_event *event) > #define ARMV8_IDX_TO_COUNTER(x) \ > (((x) - ARMV8_IDX_COUNTER0) & ARMV8_PMU_COUNTER_MASK) > > -static inline u32 armv8pmu_pmcr_read(void) > +static inline u64 armv8pmu_pmcr_read(void) > { > return read_pmcr(); > } > > -static inline void armv8pmu_pmcr_write(u32 val) > +static inline void armv8pmu_pmcr_write(u64 val) > { > val &= ARMV8_PMU_PMCR_MASK; > isb(); > @@ -957,7 +957,7 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event, > static void armv8pmu_reset(void *info) > { > struct arm_pmu *cpu_pmu = (struct arm_pmu *)info; > - u32 pmcr; > + u64 pmcr; > > /* The counter and interrupt enable registers are unknown at reset. */ > armv8pmu_disable_counter(U32_MAX);
Otherwise, this LGTM.
Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
| |