| Message ID | 20200617113851.607706-3-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm_pmu: Use NMI for perf interrupt | expand |
Quoting Alexandru Elisei (2020-06-17 04:38:46) > From: Mark Rutland <mark.rutland@arm.com> > > Currently we access the counter registers and their respective type > registers indirectly. This requires us to write to PMSELR, issue an ISB, > then access the relevant PMXEV* registers. > > This is unfortunate, because: > > * Under virtualization, accessing one registers requires two traps to one register? Not plural presumably. > the hypervisor, even though we could access the register directly with > a single trap. > > * We have to issue an ISB which we could otherwise avoid the cost of. > > * When we use NMIs, the NMI handler will have to save/restore the select > register in case the code it preempted was attempting to access a > counter or its type register. > > We can avoid these issues by directly accessing the relevant registers. > This patch adds helpers to do so. > > In armv8pmu_enable_event() we still need the ISB to prevent the PE from > reordering the write to PMINTENSET_EL1 register. If the interrupt is > enabled before we disable the counter and the new event is configured, > we might get an interrupt triggered by the previously programmed event > overflowing, but which we wrongly attribute to the event that we are > enabling. > > In the process, remove the comment that refers to the ARMv7 PMU. > > Cc: Julien Thierry <julien.thierry.kdev@gmail.com> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: Jiri Olsa <jolsa@redhat.com> > Cc: Namhyung Kim <namhyung@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > [Julien T.: Don't inline read/write functions to avoid big code-size > increase, remove unused read_pmevtypern function, > fix counter index issue.] > Signed-off-by: Julien Thierry <julien.thierry@arm.com> > [Removed comment, removed trailing semicolons in macros, added ISB] > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- > arch/arm64/kernel/perf_event.c | 95 +++++++++++++++++++++++++++++----- > 1 file changed, 81 insertions(+), 14 deletions(-) > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c > index ee180b2a5b39..e95b5ca70a53 100644 > --- a/arch/arm64/kernel/perf_event.c > +++ b/arch/arm64/kernel/perf_event.c > @@ -323,6 +323,73 @@ 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) > > +/* > + * This code is really good > + */ Superb! > + > +#define PMEVN_CASE(n, case_macro) \ > + case n: case_macro(n); break > + > +#define PMEVN_SWITCH(x, case_macro) \ > + do { \ > + switch (x) { \ > + PMEVN_CASE(0, case_macro); \ > + PMEVN_CASE(1, case_macro); \ > + PMEVN_CASE(2, case_macro); \ > + PMEVN_CASE(3, case_macro); \ > + PMEVN_CASE(4, case_macro); \ > + PMEVN_CASE(5, case_macro); \ > + PMEVN_CASE(6, case_macro); \ > + PMEVN_CASE(7, case_macro); \ > + PMEVN_CASE(8, case_macro); \ > + PMEVN_CASE(9, case_macro); \ > + PMEVN_CASE(10, case_macro); \ > + PMEVN_CASE(11, case_macro); \ > + PMEVN_CASE(12, case_macro); \ > + PMEVN_CASE(13, case_macro); \ > + PMEVN_CASE(14, case_macro); \ > + PMEVN_CASE(15, case_macro); \ > + PMEVN_CASE(16, case_macro); \ > + PMEVN_CASE(17, case_macro); \ > + PMEVN_CASE(18, case_macro); \ > + PMEVN_CASE(19, case_macro); \ > + PMEVN_CASE(20, case_macro); \ > + PMEVN_CASE(21, case_macro); \ > + PMEVN_CASE(22, case_macro); \ > + PMEVN_CASE(23, case_macro); \ > + PMEVN_CASE(24, case_macro); \ > + PMEVN_CASE(25, case_macro); \ > + PMEVN_CASE(26, case_macro); \ > + PMEVN_CASE(27, case_macro); \ > + PMEVN_CASE(28, case_macro); \ > + PMEVN_CASE(29, case_macro); \ > + PMEVN_CASE(30, case_macro); \ > + default: WARN(1, "Invalid PMEV* index"); \ Missing newline on that WARN message? > + } \ > + } while (0) > + > +#define RETURN_READ_PMEVCNTRN(n) \ > + return read_sysreg(pmevcntr##n##_el0) > +static unsigned long read_pmevcntrn(int n) > +{ > + PMEVN_SWITCH(n, RETURN_READ_PMEVCNTRN); > + return 0; > +} > + > +#define WRITE_PMEVCNTRN(n) \ > + write_sysreg(val, pmevcntr##n##_el0) > +static void write_pmevcntrn(int n, unsigned long val) > +{ > + PMEVN_SWITCH(n, WRITE_PMEVCNTRN); > +} > + > +#define WRITE_PMEVTYPERN(n) \ > + write_sysreg(val, pmevtyper##n##_el0) > +static void write_pmevtypern(int n, unsigned long val) > +{ > + PMEVN_SWITCH(n, WRITE_PMEVTYPERN); > +} > + > static inline u32 armv8pmu_pmcr_read(void) > { > return read_sysreg(pmcr_el0); > @@ -351,17 +418,11 @@ static inline int armv8pmu_counter_has_overflowed(u32 pmnc, int idx) > return pmnc & BIT(ARMV8_IDX_TO_COUNTER(idx)); > } > > -static inline void armv8pmu_select_counter(int idx) > +static inline u32 armv8pmu_read_evcntr(int idx) > { > u32 counter = ARMV8_IDX_TO_COUNTER(idx); > - write_sysreg(counter, pmselr_el0); > - isb(); > -} > > -static inline u64 armv8pmu_read_evcntr(int idx) > -{ > - armv8pmu_select_counter(idx); > - return read_sysreg(pmxevcntr_el0); > + return read_pmevcntrn(counter); > } > > static inline u64 armv8pmu_read_hw_counter(struct perf_event *event) > @@ -433,8 +494,9 @@ static u64 armv8pmu_read_counter(struct perf_event *event) > > static inline void armv8pmu_write_evcntr(int idx, u64 value) > { > - armv8pmu_select_counter(idx); > - write_sysreg(value, pmxevcntr_el0); > + u32 counter = ARMV8_IDX_TO_COUNTER(idx); Might be a good idea to make ARMV8_IDX_TO_COUNTER a static inline function that has a return type of u32. I had to go check the code to make sure it wasn't something larger. > + > + write_pmevcntrn(counter, value); > } > > static inline void armv8pmu_write_hw_counter(struct perf_event *event, > @@ -469,9 +531,10 @@ static void armv8pmu_write_counter(struct perf_event *event, u64 value) > > static inline void armv8pmu_write_evtype(int idx, u32 val) > { > - armv8pmu_select_counter(idx); > + u32 counter = ARMV8_IDX_TO_COUNTER(idx); > + > val &= ARMV8_PMU_EVTYPE_MASK; > - write_sysreg(val, pmxevtyper_el0); > + write_pmevtypern(counter, val); > } > > static inline void armv8pmu_write_event_type(struct perf_event *event) > @@ -491,7 +554,10 @@ static inline void armv8pmu_write_event_type(struct perf_event *event) > armv8pmu_write_evtype(idx - 1, hwc->config_base); > armv8pmu_write_evtype(idx, chain_evt); > } else { > - armv8pmu_write_evtype(idx, hwc->config_base); > + if (idx == ARMV8_IDX_CYCLE_COUNTER) > + write_sysreg(hwc->config_base, pmccfiltr_el0); > + else > + armv8pmu_write_evtype(idx, hwc->config_base); > } > } > > @@ -595,9 +661,10 @@ static void armv8pmu_enable_event(struct perf_event *event) > * Disable counter > */ > armv8pmu_disable_event_counter(event); > + isb(); Same comment about uncommented isb(). > > /* > - * Set event (if destined for PMNx counters). > + * Set event. > */ > armv8pmu_write_event_type(event); >
Hello, On 6/17/20 9:11 PM, Stephen Boyd wrote: > Quoting Alexandru Elisei (2020-06-17 04:38:46) >> From: Mark Rutland <mark.rutland@arm.com> >> >> Currently we access the counter registers and their respective type >> registers indirectly. This requires us to write to PMSELR, issue an ISB, >> then access the relevant PMXEV* registers. >> >> This is unfortunate, because: >> >> * Under virtualization, accessing one registers requires two traps to > one register? Not plural presumably. That's another typo, will fix it. > >> the hypervisor, even though we could access the register directly with >> a single trap. >> >> * We have to issue an ISB which we could otherwise avoid the cost of. >> >> * When we use NMIs, the NMI handler will have to save/restore the select >> register in case the code it preempted was attempting to access a >> counter or its type register. >> >> We can avoid these issues by directly accessing the relevant registers. >> This patch adds helpers to do so. >> >> In armv8pmu_enable_event() we still need the ISB to prevent the PE from >> reordering the write to PMINTENSET_EL1 register. If the interrupt is >> enabled before we disable the counter and the new event is configured, >> we might get an interrupt triggered by the previously programmed event >> overflowing, but which we wrongly attribute to the event that we are >> enabling. >> >> In the process, remove the comment that refers to the ARMv7 PMU. >> >> Cc: Julien Thierry <julien.thierry.kdev@gmail.com> >> Cc: Will Deacon <will.deacon@arm.com> >> Cc: Peter Zijlstra <peterz@infradead.org> >> Cc: Ingo Molnar <mingo@redhat.com> >> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> >> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> >> Cc: Jiri Olsa <jolsa@redhat.com> >> Cc: Namhyung Kim <namhyung@kernel.org> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Signed-off-by: Mark Rutland <mark.rutland@arm.com> >> [Julien T.: Don't inline read/write functions to avoid big code-size >> increase, remove unused read_pmevtypern function, >> fix counter index issue.] >> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >> [Removed comment, removed trailing semicolons in macros, added ISB] >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >> --- >> arch/arm64/kernel/perf_event.c | 95 +++++++++++++++++++++++++++++----- >> 1 file changed, 81 insertions(+), 14 deletions(-) >> >> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c >> index ee180b2a5b39..e95b5ca70a53 100644 >> --- a/arch/arm64/kernel/perf_event.c >> +++ b/arch/arm64/kernel/perf_event.c >> @@ -323,6 +323,73 @@ 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) >> >> +/* >> + * This code is really good >> + */ > Superb! Exactly! I thought so too, that's why I kept the comment. > >> + >> +#define PMEVN_CASE(n, case_macro) \ >> + case n: case_macro(n); break >> + >> +#define PMEVN_SWITCH(x, case_macro) \ >> + do { \ >> + switch (x) { \ >> + PMEVN_CASE(0, case_macro); \ >> + PMEVN_CASE(1, case_macro); \ >> + PMEVN_CASE(2, case_macro); \ >> + PMEVN_CASE(3, case_macro); \ >> + PMEVN_CASE(4, case_macro); \ >> + PMEVN_CASE(5, case_macro); \ >> + PMEVN_CASE(6, case_macro); \ >> + PMEVN_CASE(7, case_macro); \ >> + PMEVN_CASE(8, case_macro); \ >> + PMEVN_CASE(9, case_macro); \ >> + PMEVN_CASE(10, case_macro); \ >> + PMEVN_CASE(11, case_macro); \ >> + PMEVN_CASE(12, case_macro); \ >> + PMEVN_CASE(13, case_macro); \ >> + PMEVN_CASE(14, case_macro); \ >> + PMEVN_CASE(15, case_macro); \ >> + PMEVN_CASE(16, case_macro); \ >> + PMEVN_CASE(17, case_macro); \ >> + PMEVN_CASE(18, case_macro); \ >> + PMEVN_CASE(19, case_macro); \ >> + PMEVN_CASE(20, case_macro); \ >> + PMEVN_CASE(21, case_macro); \ >> + PMEVN_CASE(22, case_macro); \ >> + PMEVN_CASE(23, case_macro); \ >> + PMEVN_CASE(24, case_macro); \ >> + PMEVN_CASE(25, case_macro); \ >> + PMEVN_CASE(26, case_macro); \ >> + PMEVN_CASE(27, case_macro); \ >> + PMEVN_CASE(28, case_macro); \ >> + PMEVN_CASE(29, case_macro); \ >> + PMEVN_CASE(30, case_macro); \ >> + default: WARN(1, "Invalid PMEV* index"); \ > Missing newline on that WARN message? Indeed, will add it. > >> + } \ >> + } while (0) >> + >> +#define RETURN_READ_PMEVCNTRN(n) \ >> + return read_sysreg(pmevcntr##n##_el0) >> +static unsigned long read_pmevcntrn(int n) >> +{ >> + PMEVN_SWITCH(n, RETURN_READ_PMEVCNTRN); >> + return 0; >> +} >> + >> +#define WRITE_PMEVCNTRN(n) \ >> + write_sysreg(val, pmevcntr##n##_el0) >> +static void write_pmevcntrn(int n, unsigned long val) >> +{ >> + PMEVN_SWITCH(n, WRITE_PMEVCNTRN); >> +} >> + >> +#define WRITE_PMEVTYPERN(n) \ >> + write_sysreg(val, pmevtyper##n##_el0) >> +static void write_pmevtypern(int n, unsigned long val) >> +{ >> + PMEVN_SWITCH(n, WRITE_PMEVTYPERN); >> +} >> + >> static inline u32 armv8pmu_pmcr_read(void) >> { >> return read_sysreg(pmcr_el0); >> @@ -351,17 +418,11 @@ static inline int armv8pmu_counter_has_overflowed(u32 pmnc, int idx) >> return pmnc & BIT(ARMV8_IDX_TO_COUNTER(idx)); >> } >> >> -static inline void armv8pmu_select_counter(int idx) >> +static inline u32 armv8pmu_read_evcntr(int idx) >> { >> u32 counter = ARMV8_IDX_TO_COUNTER(idx); >> - write_sysreg(counter, pmselr_el0); >> - isb(); >> -} >> >> -static inline u64 armv8pmu_read_evcntr(int idx) >> -{ >> - armv8pmu_select_counter(idx); >> - return read_sysreg(pmxevcntr_el0); >> + return read_pmevcntrn(counter); >> } >> >> static inline u64 armv8pmu_read_hw_counter(struct perf_event *event) >> @@ -433,8 +494,9 @@ static u64 armv8pmu_read_counter(struct perf_event *event) >> >> static inline void armv8pmu_write_evcntr(int idx, u64 value) >> { >> - armv8pmu_select_counter(idx); >> - write_sysreg(value, pmxevcntr_el0); >> + u32 counter = ARMV8_IDX_TO_COUNTER(idx); > Might be a good idea to make ARMV8_IDX_TO_COUNTER a static inline > function that has a return type of u32. I had to go check the code to > make sure it wasn't something larger. Architecturally, there are at most 32 counter registers, which would fit in an s8, so I don't think type checking would really help us here. > >> + >> + write_pmevcntrn(counter, value); >> } >> >> static inline void armv8pmu_write_hw_counter(struct perf_event *event, >> @@ -469,9 +531,10 @@ static void armv8pmu_write_counter(struct perf_event *event, u64 value) >> >> static inline void armv8pmu_write_evtype(int idx, u32 val) >> { >> - armv8pmu_select_counter(idx); >> + u32 counter = ARMV8_IDX_TO_COUNTER(idx); >> + >> val &= ARMV8_PMU_EVTYPE_MASK; >> - write_sysreg(val, pmxevtyper_el0); >> + write_pmevtypern(counter, val); >> } >> >> static inline void armv8pmu_write_event_type(struct perf_event *event) >> @@ -491,7 +554,10 @@ static inline void armv8pmu_write_event_type(struct perf_event *event) >> armv8pmu_write_evtype(idx - 1, hwc->config_base); >> armv8pmu_write_evtype(idx, chain_evt); >> } else { >> - armv8pmu_write_evtype(idx, hwc->config_base); >> + if (idx == ARMV8_IDX_CYCLE_COUNTER) >> + write_sysreg(hwc->config_base, pmccfiltr_el0); >> + else >> + armv8pmu_write_evtype(idx, hwc->config_base); >> } >> } >> >> @@ -595,9 +661,10 @@ static void armv8pmu_enable_event(struct perf_event *event) >> * Disable counter >> */ >> armv8pmu_disable_event_counter(event); >> + isb(); > Same comment about uncommented isb(). Will add a comment explaining the ISB. Thanks, Alex
Quoting Alexandru Elisei (2020-06-18 03:51:08) > On 6/17/20 9:11 PM, Stephen Boyd wrote: > > Quoting Alexandru Elisei (2020-06-17 04:38:46) > >> From: Mark Rutland <mark.rutland@arm.com> > >> @@ -433,8 +494,9 @@ static u64 armv8pmu_read_counter(struct perf_event *event) > >> > >> static inline void armv8pmu_write_evcntr(int idx, u64 value) > >> { > >> - armv8pmu_select_counter(idx); > >> - write_sysreg(value, pmxevcntr_el0); > >> + u32 counter = ARMV8_IDX_TO_COUNTER(idx); > > Might be a good idea to make ARMV8_IDX_TO_COUNTER a static inline > > function that has a return type of u32. I had to go check the code to > > make sure it wasn't something larger. > > Architecturally, there are at most 32 counter registers, which would fit in an s8, > so I don't think type checking would really help us here. Ok. It would have saved me a few seconds while reading the code, but I guess if I hold the architecture in my head I'll be ok too.
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c index ee180b2a5b39..e95b5ca70a53 100644 --- a/arch/arm64/kernel/perf_event.c +++ b/arch/arm64/kernel/perf_event.c @@ -323,6 +323,73 @@ 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) +/* + * This code is really good + */ + +#define PMEVN_CASE(n, case_macro) \ + case n: case_macro(n); break + +#define PMEVN_SWITCH(x, case_macro) \ + do { \ + switch (x) { \ + PMEVN_CASE(0, case_macro); \ + PMEVN_CASE(1, case_macro); \ + PMEVN_CASE(2, case_macro); \ + PMEVN_CASE(3, case_macro); \ + PMEVN_CASE(4, case_macro); \ + PMEVN_CASE(5, case_macro); \ + PMEVN_CASE(6, case_macro); \ + PMEVN_CASE(7, case_macro); \ + PMEVN_CASE(8, case_macro); \ + PMEVN_CASE(9, case_macro); \ + PMEVN_CASE(10, case_macro); \ + PMEVN_CASE(11, case_macro); \ + PMEVN_CASE(12, case_macro); \ + PMEVN_CASE(13, case_macro); \ + PMEVN_CASE(14, case_macro); \ + PMEVN_CASE(15, case_macro); \ + PMEVN_CASE(16, case_macro); \ + PMEVN_CASE(17, case_macro); \ + PMEVN_CASE(18, case_macro); \ + PMEVN_CASE(19, case_macro); \ + PMEVN_CASE(20, case_macro); \ + PMEVN_CASE(21, case_macro); \ + PMEVN_CASE(22, case_macro); \ + PMEVN_CASE(23, case_macro); \ + PMEVN_CASE(24, case_macro); \ + PMEVN_CASE(25, case_macro); \ + PMEVN_CASE(26, case_macro); \ + PMEVN_CASE(27, case_macro); \ + PMEVN_CASE(28, case_macro); \ + PMEVN_CASE(29, case_macro); \ + PMEVN_CASE(30, case_macro); \ + default: WARN(1, "Invalid PMEV* index"); \ + } \ + } while (0) + +#define RETURN_READ_PMEVCNTRN(n) \ + return read_sysreg(pmevcntr##n##_el0) +static unsigned long read_pmevcntrn(int n) +{ + PMEVN_SWITCH(n, RETURN_READ_PMEVCNTRN); + return 0; +} + +#define WRITE_PMEVCNTRN(n) \ + write_sysreg(val, pmevcntr##n##_el0) +static void write_pmevcntrn(int n, unsigned long val) +{ + PMEVN_SWITCH(n, WRITE_PMEVCNTRN); +} + +#define WRITE_PMEVTYPERN(n) \ + write_sysreg(val, pmevtyper##n##_el0) +static void write_pmevtypern(int n, unsigned long val) +{ + PMEVN_SWITCH(n, WRITE_PMEVTYPERN); +} + static inline u32 armv8pmu_pmcr_read(void) { return read_sysreg(pmcr_el0); @@ -351,17 +418,11 @@ static inline int armv8pmu_counter_has_overflowed(u32 pmnc, int idx) return pmnc & BIT(ARMV8_IDX_TO_COUNTER(idx)); } -static inline void armv8pmu_select_counter(int idx) +static inline u32 armv8pmu_read_evcntr(int idx) { u32 counter = ARMV8_IDX_TO_COUNTER(idx); - write_sysreg(counter, pmselr_el0); - isb(); -} -static inline u64 armv8pmu_read_evcntr(int idx) -{ - armv8pmu_select_counter(idx); - return read_sysreg(pmxevcntr_el0); + return read_pmevcntrn(counter); } static inline u64 armv8pmu_read_hw_counter(struct perf_event *event) @@ -433,8 +494,9 @@ static u64 armv8pmu_read_counter(struct perf_event *event) static inline void armv8pmu_write_evcntr(int idx, u64 value) { - armv8pmu_select_counter(idx); - write_sysreg(value, pmxevcntr_el0); + u32 counter = ARMV8_IDX_TO_COUNTER(idx); + + write_pmevcntrn(counter, value); } static inline void armv8pmu_write_hw_counter(struct perf_event *event, @@ -469,9 +531,10 @@ static void armv8pmu_write_counter(struct perf_event *event, u64 value) static inline void armv8pmu_write_evtype(int idx, u32 val) { - armv8pmu_select_counter(idx); + u32 counter = ARMV8_IDX_TO_COUNTER(idx); + val &= ARMV8_PMU_EVTYPE_MASK; - write_sysreg(val, pmxevtyper_el0); + write_pmevtypern(counter, val); } static inline void armv8pmu_write_event_type(struct perf_event *event) @@ -491,7 +554,10 @@ static inline void armv8pmu_write_event_type(struct perf_event *event) armv8pmu_write_evtype(idx - 1, hwc->config_base); armv8pmu_write_evtype(idx, chain_evt); } else { - armv8pmu_write_evtype(idx, hwc->config_base); + if (idx == ARMV8_IDX_CYCLE_COUNTER) + write_sysreg(hwc->config_base, pmccfiltr_el0); + else + armv8pmu_write_evtype(idx, hwc->config_base); } } @@ -595,9 +661,10 @@ static void armv8pmu_enable_event(struct perf_event *event) * Disable counter */ armv8pmu_disable_event_counter(event); + isb(); /* - * Set event (if destined for PMNx counters). + * Set event. */ armv8pmu_write_event_type(event);