diff mbox series

[v2,1/9] arm64: perf: avoid PMXEV* indirection

Message ID 1553271844-49003-2-git-send-email-julien.thierry@arm.com (mailing list archive)
State New, archived
Headers show
Series arm_pmu: Use NMI for perf interrupt | expand

Commit Message

Julien Thierry March 22, 2019, 4:23 p.m. UTC 
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
  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.

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.]
Signed-off-by: Julien Thierry <julien.thierry@arm.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>
---
 arch/arm64/kernel/perf_event.c | 87 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 13 deletions(-)

--
1.9.1

Comments

Wei Li March 25, 2019, 12:36 p.m. UTC | #1 
Calling the ARMV8_IDX_TO_COUNTER() is missed before accessing these counters.
It will access the wrong counters and may even cause undefinstr exception.
I think we need add it as armv8pmu_select_counter() do before.

Signed-off-by: Wei Li <liwei391@huawei.com>
---
 arch/arm64/kernel/perf_event.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index c6fe36f5a9f5..42fe7109468f 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -599,7 +599,9 @@ static inline int armv8pmu_counter_has_overflowed(u32 pmnc, int idx)
 
 static inline u32 armv8pmu_read_evcntr(int idx)
 {
-	return read_pmevcntrn(idx);
+	u32 counter = ARMV8_IDX_TO_COUNTER(idx);
+
+	return read_pmevcntrn(counter);
 }
 
 static inline u64 armv8pmu_read_hw_counter(struct perf_event *event)
@@ -633,7 +635,9 @@ static inline u64 armv8pmu_read_counter(struct perf_event *event)
 
 static inline void armv8pmu_write_evcntr(int idx, u32 value)
 {
-	write_pmevcntrn(idx, value);
+	u32 counter = ARMV8_IDX_TO_COUNTER(idx);
+
+	write_pmevcntrn(counter, value);
 }
 
 static inline void armv8pmu_write_hw_counter(struct perf_event *event,
@@ -674,8 +678,10 @@ static inline void armv8pmu_write_counter(struct perf_event *event, u64 value)
 
 static inline void armv8pmu_write_evtype(int idx, u32 val)
 {
+	u32 counter = ARMV8_IDX_TO_COUNTER(idx);
+
 	val &= ARMV8_PMU_EVTYPE_MASK;
-	write_pmevtypern(idx, val);
+	write_pmevtypern(counter, val);
 }
 
 static inline void armv8pmu_write_event_type(struct perf_event *event)
Wei Li March 25, 2019, 12:37 p.m. UTC | #2 
When PMSELR_EL0.SEL == 31, the register PMXEVTYPER_EL0 accesses PMCCFILTR_EL0.
I think we need to add a branch to process such condition.

Signed-off-by: Wei Li <liwei391@huawei.com>
---
 arch/arm64/kernel/perf_event.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 42fe7109468f..76bc55e67137 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -538,7 +538,7 @@ static inline bool armv8pmu_event_is_chained(struct perf_event *event)
 		PMEVN_CASE(28, case_macro);			\
 		PMEVN_CASE(29, case_macro);			\
 		PMEVN_CASE(30, case_macro);			\
-		default: WARN(1, "Inavlid PMEV* index");	\
+		default: WARN(1, "Inavlid PMEV* index %#x", __x);	\
 		}						\
 	} while (0)
 
@@ -684,7 +684,7 @@ static inline void armv8pmu_write_evtype(int idx, u32 val)
 	write_pmevtypern(counter, val);
 }
 
-static inline void armv8pmu_write_event_type(struct perf_event *event)
+static inline void armv8pmu_write_hw_type(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
 	int idx = hwc->idx;
@@ -705,6 +705,21 @@ static inline void armv8pmu_write_event_type(struct perf_event *event)
 	}
 }
 
+static inline void armv8pmu_write_event_type(struct perf_event *event)
+{
+	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	int idx = hwc->idx;
+
+	if (!armv8pmu_counter_valid(cpu_pmu, idx))
+		pr_err("CPU%u reading wrong counter %d\n",
+			smp_processor_id(), idx);
+	else if (idx == ARMV8_IDX_CYCLE_COUNTER)
+		write_sysreg(hwc->config_base & ARMV8_PMU_EVTYPE_MASK, pmccfiltr_el0);
+	else
+		armv8pmu_write_hw_type(event);
+}
+
 static inline int armv8pmu_enable_counter(int idx)
 {
 	u32 counter = ARMV8_IDX_TO_COUNTER(idx);
Marc Gonzalez March 25, 2019, 1:01 p.m. UTC | #3 
On 22/03/2019 17:23, Julien Thierry wrote:

> +/*
> + * This code is really good
> + */

IMHO, nothing involving preprocessor tom-foolery should qualify as "good".

> +
> +#define PMEVN_CASE(__n, case_macro) \
> +	case __n: case_macro(__n); break;
> +
> +#define PMEVN_SWITCH(__x, case_macro)				\

I've never understood: what's the point of prefixing macro parameters
with underscores?

What does the PMEVN_CASE macro buy us?

Regards.
Julien Thierry March 25, 2019, 1:59 p.m. UTC | #4 
Hi,

On 25/03/2019 12:36, liwei (GF) wrote:
> Calling the ARMV8_IDX_TO_COUNTER() is missed before accessing these counters.
> It will access the wrong counters and may even cause undefinstr exception.
> I think we need add it as armv8pmu_select_counter() do before.
> 

Thank you for reporting and fixing that issue. I'll include the fix in
my next version.

Thanks,

Julien

> Signed-off-by: Wei Li <liwei391@huawei.com>
> ---
>  arch/arm64/kernel/perf_event.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index c6fe36f5a9f5..42fe7109468f 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -599,7 +599,9 @@ static inline int armv8pmu_counter_has_overflowed(u32 pmnc, int idx)
>  
>  static inline u32 armv8pmu_read_evcntr(int idx)
>  {
> -	return read_pmevcntrn(idx);
> +	u32 counter = ARMV8_IDX_TO_COUNTER(idx);
> +
> +	return read_pmevcntrn(counter);
>  }
>  
>  static inline u64 armv8pmu_read_hw_counter(struct perf_event *event)
> @@ -633,7 +635,9 @@ static inline u64 armv8pmu_read_counter(struct perf_event *event)
>  
>  static inline void armv8pmu_write_evcntr(int idx, u32 value)
>  {
> -	write_pmevcntrn(idx, value);
> +	u32 counter = ARMV8_IDX_TO_COUNTER(idx);
> +
> +	write_pmevcntrn(counter, value);
>  }
>  
>  static inline void armv8pmu_write_hw_counter(struct perf_event *event,
> @@ -674,8 +678,10 @@ static inline void armv8pmu_write_counter(struct perf_event *event, u64 value)
>  
>  static inline void armv8pmu_write_evtype(int idx, u32 val)
>  {
> +	u32 counter = ARMV8_IDX_TO_COUNTER(idx);
> +
>  	val &= ARMV8_PMU_EVTYPE_MASK;
> -	write_pmevtypern(idx, val);
> +	write_pmevtypern(counter, val);
>  }
>  
>  static inline void armv8pmu_write_event_type(struct perf_event *event)
>
Julien Thierry March 25, 2019, 1:59 p.m. UTC | #5 
Hi

On 25/03/2019 12:37, liwei (GF) wrote:
> When PMSELR_EL0.SEL == 31, the register PMXEVTYPER_EL0 accesses PMCCFILTR_EL0.
> I think we need to add a branch to process such condition.
> 
> Signed-off-by: Wei Li <liwei391@huawei.com>
> ---
>  arch/arm64/kernel/perf_event.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index 42fe7109468f..76bc55e67137 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -538,7 +538,7 @@ static inline bool armv8pmu_event_is_chained(struct perf_event *event)
>  		PMEVN_CASE(28, case_macro);			\
>  		PMEVN_CASE(29, case_macro);			\
>  		PMEVN_CASE(30, case_macro);			\
> -		default: WARN(1, "Inavlid PMEV* index");	\
> +		default: WARN(1, "Inavlid PMEV* index %#x", __x);	\

I'm not convinced we need that. We're generating register accesses. If
we try to access a register that doesn't exist, the compiler/assembler
will complain and fail to build. So that default case would not be run.

Thanks,

Julien

>  		}						\
>  	} while (0)
>  
> @@ -684,7 +684,7 @@ static inline void armv8pmu_write_evtype(int idx, u32 val)
>  	write_pmevtypern(counter, val);
>  }
>  
> -static inline void armv8pmu_write_event_type(struct perf_event *event)
> +static inline void armv8pmu_write_hw_type(struct perf_event *event)
>  {
>  	struct hw_perf_event *hwc = &event->hw;
>  	int idx = hwc->idx;
> @@ -705,6 +705,21 @@ static inline void armv8pmu_write_event_type(struct perf_event *event)
>  	}
>  }
>  
> +static inline void armv8pmu_write_event_type(struct perf_event *event)
> +{
> +	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int idx = hwc->idx;
> +
> +	if (!armv8pmu_counter_valid(cpu_pmu, idx))
> +		pr_err("CPU%u reading wrong counter %d\n",
> +			smp_processor_id(), idx);
> +	else if (idx == ARMV8_IDX_CYCLE_COUNTER)
> +		write_sysreg(hwc->config_base & ARMV8_PMU_EVTYPE_MASK, pmccfiltr_el0);
> +	else
> +		armv8pmu_write_hw_type(event);
> +}
> +
>  static inline int armv8pmu_enable_counter(int idx)
>  {
>  	u32 counter = ARMV8_IDX_TO_COUNTER(idx);
>
Wei Li March 26, 2019, 2:10 a.m. UTC | #6 
Hi Julien,

On 2019/3/25 21:59, Julien Thierry wrote:
> 
> On 25/03/2019 12:37, liwei (GF) wrote:
>> When PMSELR_EL0.SEL == 31, the register PMXEVTYPER_EL0 accesses PMCCFILTR_EL0.
>> I think we need to add a branch to process such condition.
>>
>> Signed-off-by: Wei Li <liwei391@huawei.com>
>> ---
>>  arch/arm64/kernel/perf_event.c | 19 +++++++++++++++++--
>>  1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>> index 42fe7109468f..76bc55e67137 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -538,7 +538,7 @@ static inline bool armv8pmu_event_is_chained(struct perf_event *event)
>>  		PMEVN_CASE(28, case_macro);			\
>>  		PMEVN_CASE(29, case_macro);			\
>>  		PMEVN_CASE(30, case_macro);			\
>> -		default: WARN(1, "Inavlid PMEV* index");	\
>> +		default: WARN(1, "Inavlid PMEV* index %#x", __x);	\
> 
> I'm not convinced we need that. We're generating register accesses. If
> we try to access a register that doesn't exist, the compiler/assembler
> will complain and fail to build. So that default case would not be run.
> 
This issue is covered by the lack of ARMV8_IDX_TO_COUNTER(), the ARMV8_IDX_TO_COUNTER()
will convert the CYCLE_COUNTER sw_index 0 to hw_index 31.
It goes well when accessing PMCCFILTR_EL0 though armv8pmu_select_counter(), but it will just go
into the default case above when doing armv8pmu_enable_event(CYCLE_COUNTER_EVENT).

So we may not need the WARN info anymore, but we need the process to access the PMCCFILTR_EL0
here indeed, like what armv8pmu_write_counter() and armv8pmu_read_counter() do.

[   22.925659] ------------[ cut here ]------------
[   22.930264] Inavlid PMEV* index 0x1f
[   22.930269] WARNING: CPU: 58 PID: 411 at /home/liwei/main_code/hulk/arch/arm64/kernel/perf_event.c:566 write_pmevtypern+0x34/0x150
[   22.945552] Modules linked in:
[   22.948598] CPU: 58 PID: 411 Comm: kworker/58:1 Tainted: G        W         4.19.30+ #13
[   22.956674] Hardware name: Huawei TaiShan 2280 /BC11SPCD, BIOS 1.40 01/10/2018
[   22.963888] Workqueue: events smp_call_on_cpu_callback
[   22.969016] pstate: 60000005 (nZCv daif -PAN -UAO)
[   22.973796] pc : write_pmevtypern+0x34/0x150
[   22.978054] lr : write_pmevtypern+0x34/0x150
[   22.982312] sp : ffff0000187fb820
[   22.985614] pmr_save: 00000070
[   22.988657] x29: ffff0000187fb820 x28: 0000000000000000 
[   22.993960] x27: ffff00000b81bca8 x26: ffff000008255750 
[   22.999263] x25: ffff80dffbe59640 x24: ffff80dffbe59640 
[   23.004566] x23: ffff805f780dd000 x22: 0000000000000000 
[   23.009868] x21: 0000000000000002 x20: 0000000008000011 
[   23.015171] x19: 000000000000001f x18: ffffffffffffffff 
[   23.020474] x17: 0000000000000000 x16: 0000000000000000 
[   23.025776] x15: ffff0000097b9708 x14: ffff0000099ef938 
[   23.031079] x13: ffff0000099ef570 x12: ffffffffffffffff 
[   23.036381] x11: ffff0000097e3000 x10: 0000000005f5e0ff 
[   23.041684] x9 : 00000000ffffffd0 x8 : 6631783020786564 
[   23.046987] x7 : ffff0000099eeba0 x6 : 000000000000114f 
[   23.052290] x5 : 00ffffffffffffff x4 : 0000000000000000 
[   23.057592] x3 : 0000000000000000 x2 : ffffffffffffffff 
[   23.062895] x1 : 2a15229c4b9a7500 x0 : 0000000000000000 
[   23.068197] Call trace:
[   23.070633]  write_pmevtypern+0x34/0x150
[   23.074544]  armv8pmu_enable_event+0x7c/0x158
[   23.078889]  armpmu_start+0x4c/0x68
[   23.082366]  armpmu_add+0xcc/0xd8
[   23.085669]  event_sched_in.isra.56+0xc0/0x220
[   23.090100]  group_sched_in+0x60/0x188
[   23.093837]  pinned_sched_in+0x104/0x1e8
[   23.097748]  visit_groups_merge+0x12c/0x1d0
[   23.101920]  ctx_sched_in+0x108/0x168
[   23.105571]  perf_event_sched_in+0x2c/0xa8
[   23.109655]  ctx_resched+0x60/0xa8
[   23.113045]  __perf_event_enable+0x180/0x1f8
[   23.117303]  event_function+0x78/0xd8
[   23.120954]  remote_function+0x60/0x70
[   23.124692]  generic_exec_single+0x114/0x168
[   23.128950]  smp_call_function_single+0x15c/0x1b0
[   23.133643]  event_function_call+0x16c/0x178
[   23.137901]  _perf_event_enable+0xa4/0xd0
[   23.141899]  perf_event_enable+0x20/0x40

>>  
>> +static inline void armv8pmu_write_event_type(struct perf_event *event)
>> +{
>> +	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
>> +	struct hw_perf_event *hwc = &event->hw;
>> +	int idx = hwc->idx;
>> +
>> +	if (!armv8pmu_counter_valid(cpu_pmu, idx))
>> +		pr_err("CPU%u reading wrong counter %d\n",
This print info need to be replaced to "CPU%u writing wrong event %d\n".

>> +			smp_processor_id(), idx);
>> +	else if (idx == ARMV8_IDX_CYCLE_COUNTER)
>> +		write_sysreg(hwc->config_base & ARMV8_PMU_EVTYPE_MASK, pmccfiltr_el0);
>> +	else
>> +		armv8pmu_write_hw_type(event);
>> +}
>> +
>>  static inline int armv8pmu_enable_counter(int idx)
>>  {
>>  	u32 counter = ARMV8_IDX_TO_COUNTER(idx);
>>
> 

Thanks,
Wei
Julien Thierry March 28, 2019, 9:48 a.m. UTC | #7 
Hi,

On 26/03/2019 02:10, liwei (GF) wrote:
> Hi Julien,
> 
> On 2019/3/25 21:59, Julien Thierry wrote:
>>
>> On 25/03/2019 12:37, liwei (GF) wrote:
>>> When PMSELR_EL0.SEL == 31, the register PMXEVTYPER_EL0 accesses PMCCFILTR_EL0.
>>> I think we need to add a branch to process such condition.
>>>
>>> Signed-off-by: Wei Li <liwei391@huawei.com>
>>> ---
>>>  arch/arm64/kernel/perf_event.c | 19 +++++++++++++++++--
>>>  1 file changed, 17 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>>> index 42fe7109468f..76bc55e67137 100644
>>> --- a/arch/arm64/kernel/perf_event.c
>>> +++ b/arch/arm64/kernel/perf_event.c
>>> @@ -538,7 +538,7 @@ static inline bool armv8pmu_event_is_chained(struct perf_event *event)
>>>  		PMEVN_CASE(28, case_macro);			\
>>>  		PMEVN_CASE(29, case_macro);			\
>>>  		PMEVN_CASE(30, case_macro);			\
>>> -		default: WARN(1, "Inavlid PMEV* index");	\
>>> +		default: WARN(1, "Inavlid PMEV* index %#x", __x);	\
>>
>> I'm not convinced we need that. We're generating register accesses. If
>> we try to access a register that doesn't exist, the compiler/assembler
>> will complain and fail to build. So that default case would not be run.
>>
> This issue is covered by the lack of ARMV8_IDX_TO_COUNTER(), the ARMV8_IDX_TO_COUNTER()
> will convert the CYCLE_COUNTER sw_index 0 to hw_index 31.

As I said, pmevcntr31_el0 and pmevtyper31_el0 do not exist, and the
compiler/assembler should reject it. So I still don't think that default
case brings anything.

> It goes well when accessing PMCCFILTR_EL0 though armv8pmu_select_counter(), but it will just go
> into the default case above when doing armv8pmu_enable_event(CYCLE_COUNTER_EVENT).
> 
> So we may not need the WARN info anymore, but we need the process to access the PMCCFILTR_EL0
> here indeed, like what armv8pmu_write_counter() and armv8pmu_read_counter() do.
> 
> [   22.925659] ------------[ cut here ]------------
> [   22.930264] Inavlid PMEV* index 0x1f
> [   22.930269] WARNING: CPU: 58 PID: 411 at /home/liwei/main_code/hulk/arch/arm64/kernel/perf_event.c:566 write_pmevtypern+0x34/0x150
> [   22.945552] Modules linked in:
> [   22.948598] CPU: 58 PID: 411 Comm: kworker/58:1 Tainted: G        W         4.19.30+ #13
> [   22.956674] Hardware name: Huawei TaiShan 2280 /BC11SPCD, BIOS 1.40 01/10/2018
> [   22.963888] Workqueue: events smp_call_on_cpu_callback
> [   22.969016] pstate: 60000005 (nZCv daif -PAN -UAO)
> [   22.973796] pc : write_pmevtypern+0x34/0x150
> [   22.978054] lr : write_pmevtypern+0x34/0x150
> [   22.982312] sp : ffff0000187fb820
> [   22.985614] pmr_save: 00000070
> [   22.988657] x29: ffff0000187fb820 x28: 0000000000000000 
> [   22.993960] x27: ffff00000b81bca8 x26: ffff000008255750 
> [   22.999263] x25: ffff80dffbe59640 x24: ffff80dffbe59640 
> [   23.004566] x23: ffff805f780dd000 x22: 0000000000000000 
> [   23.009868] x21: 0000000000000002 x20: 0000000008000011 
> [   23.015171] x19: 000000000000001f x18: ffffffffffffffff 
> [   23.020474] x17: 0000000000000000 x16: 0000000000000000 
> [   23.025776] x15: ffff0000097b9708 x14: ffff0000099ef938 
> [   23.031079] x13: ffff0000099ef570 x12: ffffffffffffffff 
> [   23.036381] x11: ffff0000097e3000 x10: 0000000005f5e0ff 
> [   23.041684] x9 : 00000000ffffffd0 x8 : 6631783020786564 
> [   23.046987] x7 : ffff0000099eeba0 x6 : 000000000000114f 
> [   23.052290] x5 : 00ffffffffffffff x4 : 0000000000000000 
> [   23.057592] x3 : 0000000000000000 x2 : ffffffffffffffff 
> [   23.062895] x1 : 2a15229c4b9a7500 x0 : 0000000000000000 
> [   23.068197] Call trace:
> [   23.070633]  write_pmevtypern+0x34/0x150
> [   23.074544]  armv8pmu_enable_event+0x7c/0x158
> [   23.078889]  armpmu_start+0x4c/0x68
> [   23.082366]  armpmu_add+0xcc/0xd8
> [   23.085669]  event_sched_in.isra.56+0xc0/0x220
> [   23.090100]  group_sched_in+0x60/0x188
> [   23.093837]  pinned_sched_in+0x104/0x1e8
> [   23.097748]  visit_groups_merge+0x12c/0x1d0
> [   23.101920]  ctx_sched_in+0x108/0x168
> [   23.105571]  perf_event_sched_in+0x2c/0xa8
> [   23.109655]  ctx_resched+0x60/0xa8
> [   23.113045]  __perf_event_enable+0x180/0x1f8
> [   23.117303]  event_function+0x78/0xd8
> [   23.120954]  remote_function+0x60/0x70
> [   23.124692]  generic_exec_single+0x114/0x168
> [   23.128950]  smp_call_function_single+0x15c/0x1b0
> [   23.133643]  event_function_call+0x16c/0x178
> [   23.137901]  _perf_event_enable+0xa4/0xd0
> [   23.141899]  perf_event_enable+0x20/0x40
> 
>>>  
>>> +static inline void armv8pmu_write_event_type(struct perf_event *event)
>>> +{
>>> +	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
>>> +	struct hw_perf_event *hwc = &event->hw;
>>> +	int idx = hwc->idx;
>>> +
>>> +	if (!armv8pmu_counter_valid(cpu_pmu, idx))
>>> +		pr_err("CPU%u reading wrong counter %d\n",
> This print info need to be replaced to "CPU%u writing wrong event %d\n".
> 
>>> +			smp_processor_id(), idx);
>>> +	else if (idx == ARMV8_IDX_CYCLE_COUNTER)

However, after looking a bit more into it, I do think you've spotted an
issue that exists with armv8pmu_write_event_type() prior to my series
(and should be fixed out of it).

I'll send a fix shortly and put you in copy.

Thanks,
Julien Thierry July 8, 2019, 11:40 a.m. UTC | #8 
Hi Marc,

(Picking that series back after fixing NMIs, sorry for the late reply)

On 25/03/2019 13:01, Marc Gonzalez wrote:
> On 22/03/2019 17:23, Julien Thierry wrote:
> 
>> +/*
>> + * This code is really good
>> + */
> 
> IMHO, nothing involving preprocessor tom-foolery should qualify as "good".
> 
>> +
>> +#define PMEVN_CASE(__n, case_macro) \
>> +	case __n: case_macro(__n); break;
>> +
>> +#define PMEVN_SWITCH(__x, case_macro)				\
> 
> I've never understood: what's the point of prefixing macro parameters
> with underscores?
> 

I picked the patch like this, but I admit I agree with you. So unless
there is a reason to do otherwise, I'll remove those underscores.

> What does the PMEVN_CASE macro buy us?

Best I can think of is avoiding to make the mistake of:

	case 1: case_macro(10); break;


Thanks,
diff mbox series

Patch

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 4addb38..63d45e2 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -379,6 +379,77 @@  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(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, "Inavlid 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;
+}
+#undef RETURN_READ_PMEVCNTRN
+
+#define WRITE_PMEVCNTRN(n) \
+	write_sysreg(val, pmevcntr##n##_el0);
+static void write_pmevcntrn(int n, unsigned long val)
+{
+	PMEVN_SWITCH(n, WRITE_PMEVCNTRN);
+}
+#undef 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);
+}
+#undef WRITE_PMEVTYPERN
+
+#undef GENERATE_PMEVN_SWITCH
+
 static inline u32 armv8pmu_pmcr_read(void)
 {
 	return read_sysreg(pmcr_el0);
@@ -407,17 +478,9 @@  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)
-{
-	u32 counter = ARMV8_IDX_TO_COUNTER(idx);
-	write_sysreg(counter, pmselr_el0);
-	isb();
-}
-
 static inline u32 armv8pmu_read_evcntr(int idx)
 {
-	armv8pmu_select_counter(idx);
-	return read_sysreg(pmxevcntr_el0);
+	return read_pmevcntrn(idx);
 }

 static inline u64 armv8pmu_read_hw_counter(struct perf_event *event)
@@ -451,8 +514,7 @@  static inline u64 armv8pmu_read_counter(struct perf_event *event)

 static inline void armv8pmu_write_evcntr(int idx, u32 value)
 {
-	armv8pmu_select_counter(idx);
-	write_sysreg(value, pmxevcntr_el0);
+	write_pmevcntrn(idx, value);
 }

 static inline void armv8pmu_write_hw_counter(struct perf_event *event,
@@ -493,9 +555,8 @@  static inline void armv8pmu_write_counter(struct perf_event *event, u64 value)

 static inline void armv8pmu_write_evtype(int idx, u32 val)
 {
-	armv8pmu_select_counter(idx);
 	val &= ARMV8_PMU_EVTYPE_MASK;
-	write_sysreg(val, pmxevtyper_el0);
+	write_pmevtypern(idx, val);
 }

 static inline void armv8pmu_write_event_type(struct perf_event *event)