Re: [PATCH v5 2/7] arm64: perf: Avoid PMXEV* indirection

From: Stephen Boyd
Date: Wed Jun 17 2020 - 16:11:32 EST

Next message: Christophe JAILLET: "[PATCH] ia64: Fix a typo in a comment"
Previous message: Olof Johansson: "Re: [PATCH] pcmcia/electra_cf: Fix some return values in 'electra_cf_probe()' in case of error"
In reply to: Alexandru Elisei: "[PATCH v5 2/7] arm64: perf: Avoid PMXEV* indirection"
Next in thread: Alexandru Elisei: "Re: [PATCH v5 2/7] arm64: perf: Avoid PMXEV* indirection"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Quoting Alexandru Elisei (2020-06-17 04:38:46)
> From: Mark Rutland <mark.rutland@xxxxxxx>
>
> 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@xxxxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
> [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@xxxxxxx>
> [Removed comment, removed trailing semicolons in macros, added ISB]
> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> ---
> 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);
>

Next message: Christophe JAILLET: "[PATCH] ia64: Fix a typo in a comment"
Previous message: Olof Johansson: "Re: [PATCH] pcmcia/electra_cf: Fix some return values in 'electra_cf_probe()' in case of error"
In reply to: Alexandru Elisei: "[PATCH v5 2/7] arm64: perf: Avoid PMXEV* indirection"
Next in thread: Alexandru Elisei: "Re: [PATCH v5 2/7] arm64: perf: Avoid PMXEV* indirection"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]