[PATCH V4] arm64: perf: Set PMCR.X of PMCR_EL0 during pmu reset

[Srinivasarao Pathipati]

On 5/17/2022 6:38 PM, Will Deacon wrote:
> On Mon, May 16, 2022 at 06:25:38PM +0530, Srinivasarao Pathipati wrote:
>> Enable exporting of events over PMU event export bus by setting
>> PMCR.X of PMCR_EL0 during pmu reset.
>>
>> As it impacts power consumption make it configurable at bootup
>> with kernel arguments and at runtime with sysctl.
>>
>> Signed-off-by: Srinivasarao Pathipati <quic_c_spathi at quicinc.com>
>> ---
>> Changes since V3:
>> 	- export bit is now configurable with sysctl
>> 	- enabling export bit on reset instead of retaining
>>
>> Changes since V2:
>> 	Done below changes as per Will's comments
>> 	- enabling pmcr_x now configurable with kernel parameters and
>> 	  by default it is disabled.
>> 	
>> Changes since V1:
>> 	- Preserving only PMCR_X bit as per Robin Murphy's comment.
>> ---
>>   Documentation/admin-guide/kernel-parameters.txt |  4 ++++
>>   Documentation/admin-guide/sysctl/kernel.rst     |  8 ++++++++
>>   arch/arm64/kernel/perf_event.c                  | 15 +++++++++++++++
>>   include/linux/perf_event.h                      |  1 +
>>   kernel/sysctl.c                                 | 12 ++++++++++++
>>   5 files changed, 40 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index de3da15..2139b81 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -5150,6 +5150,10 @@
>>   			Useful for devices that are detected asynchronously
>>   			(e.g. USB and MMC devices).
>>   
>> +	export_pmu_events
>> +			[KNL] Sets export bit of PMCR_EL0 to enable the exporting of
>> +			events over PMU event export bus.
> Sorry, I should've been clearer ahbout this before: if you add a sysctl,
> then you get the kernel cmdline option for free via something like
> "sysctl.kernel.export_pmu_events=foo", so I think you can drop this and
> the early_param().

Thanks for your suggestion , I tried this method and observed that 
pmu_reset is getting called even

before kernel argument updated the sysctl variable.   So exporting is 
not happening at early boot.

>> +
>>   	retain_initrd	[RAM] Keep initrd memory after extraction
>>   
>>   	rfkill.default_state=
>> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
>> index ddccd10..8fbc3a0 100644
>> --- a/Documentation/admin-guide/sysctl/kernel.rst
>> +++ b/Documentation/admin-guide/sysctl/kernel.rst
>> @@ -892,6 +892,14 @@ The default value is 0 (access disabled).
>>   
>>   See Documentation/arm64/perf.rst for more information.
>>   
>> +export_pmu_events
>> +=================
> You should add something like "(arm64 only)" to the title.
>
>> +Controls the export bit(4th bit) of PMCR_EL0 which enables the exporting of
> Just say "Controls the PMU export bit (PMCR_EL0.X), which enables ...".
>
>> +events over an IMPLEMENTATION DEFINED PMU event export bus to another device.
>> +
>> +0: disables exporting of events
>> +
>> +1: enables exporting of events
> Please state that the default value is 0.
>
>>   pid_max
>>   =======
>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>> index cb69ff1..271a8c6 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -34,6 +34,7 @@
>>   #define ARMV8_THUNDER_PERFCTR_L1I_CACHE_PREF_ACCESS		0xEC
>>   #define ARMV8_THUNDER_PERFCTR_L1I_CACHE_PREF_MISS		0xED
>>   
>> +int sysctl_export_pmu_events __read_mostly;
>>   /*
>>    * ARMv8 Architectural defined events, not all of these may
>>    * be supported on any given implementation. Unsupported events will
>> @@ -1025,6 +1026,17 @@ static int armv8pmu_filter_match(struct perf_event *event)
>>   	return evtype != ARMV8_PMUV3_PERFCTR_CHAIN;
>>   }
>>   
>> +static int __init export_pmu_events(char *str)
>> +{
>> +	/* Exporting of events can be enabled at runtime with sysctl or
>> +	 * statically at bootup with kernel parameters.
>> +	 */
>> +	sysctl_export_pmu_events = 1;
>> +	return 0;
>> +}
>> +
>> +early_param("export_pmu_events", export_pmu_events);
>> +
>>   static void armv8pmu_reset(void *info)
>>   {
>>   	struct arm_pmu *cpu_pmu = (struct arm_pmu *)info;
>> @@ -1047,6 +1059,9 @@ static void armv8pmu_reset(void *info)
>>   	if (armv8pmu_has_long_event(cpu_pmu))
>>   		pmcr |= ARMV8_PMU_PMCR_LP;
>>   
>> +	if (sysctl_export_pmu_events)
>> +		pmcr |= ARMV8_PMU_PMCR_X;
>> +
>>   	armv8pmu_pmcr_write(pmcr);
>
> Hmm, I think this reset path only runs when initialising/onlining a CPU,
> so it's not a great user interface where the sysctl is concerned. It's
> probably better to hook armv8pmu_start() for this.
>
>>   }
>>   
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index da75956..7790328 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -1311,6 +1311,7 @@ extern void put_callchain_entry(int rctx);
>>   
>>   extern int sysctl_perf_event_max_stack;
>>   extern int sysctl_perf_event_max_contexts_per_stack;
>> +extern int sysctl_export_pmu_events;
>>   
>>   static inline int perf_callchain_store_context(struct perf_callchain_entry_ctx *ctx, u64 ip)
>>   {
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index e52b6e3..3b751a2e 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -2008,6 +2008,18 @@ static struct ctl_table kern_table[] = {
>>   		.extra2		= SYSCTL_ONE_THOUSAND,
>>   	},
>>   #endif
>> +#ifdef CONFIG_HW_PERF_EVENTS
>> +	{
>> +		.procname       = "export_pmu_events",
>> +		.data           = &sysctl_export_pmu_events,
>> +		.maxlen         = sizeof(int),
>> +		.mode           = 0644,
>> +		.proc_handler   = proc_dointvec_minmax,
>> +		.extra1         = SYSCTL_ZERO,
>> +		.extra2         = SYSCTL_ONE,
>> +
>> +	},
>> +#endif
> Since this is arm64-specific, it should live in the arm64 code and not
> here. See how we already register 'armv8_pmu_sysctl_table' for the ARMv8
> PMU.
>
> Will