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

From: Alexandru Elisei
Date: Thu Jun 18 2020 - 06:50:27 EST

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@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.

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@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!

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