[v6] arm64: perf: Make exporting of pmu events configurable

[PATCH V6] arm64: perf: Make exporting of pmu events configurable

Posted by Srinivasarao Pathipati 1 year, 10 months ago

The PMU export bit (PMCR_EL0.X) is getting reset during pmu reset,
Make it configurable using sysctls to enable/disable at runtime.

Signed-off-by: Srinivasarao Pathipati <quic_c_spathi@quicinc.com>
---
Changes since V5:
	- removed configuring with kernel parameters.
Changes since V4:
	- Registering sysctls dynamically for only arm64 as suggested by Will
	- Not removed the code to configure with kernel parameters 
	  as the sysctl's kernel parameter(sysctl.kernel.export_pmu_events)
	  is not working at early bootup. pmu_reset() getting called before 
	  sysctl's kernel parameter is set.
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/sysctl/kernel.rst | 11 +++++++++++
 arch/arm64/kernel/perf_event.c              | 13 +++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
index ddccd10..c2ecd84 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -267,6 +267,17 @@ domain names are in general different. For a detailed discussion
 see the ``hostname(1)`` man page.
 
 
+export_pmu_events (arm64 only)
+==============================
+
+Controls the PMU export bit (PMCR_EL0.X), which enables the exporting of
+events over an IMPLEMENTATION DEFINED PMU event export bus to another device.
+
+0: disables exporting of events (default).
+
+1: enables exporting of events.
+
+
 firmware_config
 ===============
 
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index cb69ff1..a8c32a0 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -298,6 +298,7 @@ PMU_FORMAT_ATTR(long, "config1:0");
 PMU_FORMAT_ATTR(rdpmc, "config1:1");
 
 static int sysctl_perf_user_access __read_mostly;
+static int sysctl_export_pmu_events __read_mostly;
 
 static inline bool armv8pmu_event_is_64bit(struct perf_event *event)
 {
@@ -1047,6 +1048,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);
 }
 
@@ -1221,6 +1225,15 @@ static struct ctl_table armv8_pmu_sysctl_table[] = {
 		.extra1		= SYSCTL_ZERO,
 		.extra2		= SYSCTL_ONE,
 	},
+	{
+		.procname       = "export_pmu_events",
+		.data           = &sysctl_export_pmu_events,
+		.maxlen         = sizeof(unsigned int),
+		.mode           = 0644,
+		.proc_handler   = proc_dointvec_minmax,
+		.extra1         = SYSCTL_ZERO,
+		.extra2         = SYSCTL_ONE,
+	},
 	{ }
 };
 
-- 
2.7.4

Re: [PATCH V6] arm64: perf: Make exporting of pmu events configurable

Posted by Will Deacon 1 year, 10 months ago

On Thu, Jun 09, 2022 at 06:29:49PM +0530, Srinivasarao Pathipati wrote:
> The PMU export bit (PMCR_EL0.X) is getting reset during pmu reset,
> Make it configurable using sysctls to enable/disable at runtime.
> 
> Signed-off-by: Srinivasarao Pathipati <quic_c_spathi@quicinc.com>
> ---
> Changes since V5:
> 	- removed configuring with kernel parameters.
> Changes since V4:
> 	- Registering sysctls dynamically for only arm64 as suggested by Will
> 	- Not removed the code to configure with kernel parameters 
> 	  as the sysctl's kernel parameter(sysctl.kernel.export_pmu_events)
> 	  is not working at early bootup. pmu_reset() getting called before 
> 	  sysctl's kernel parameter is set.
> 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/sysctl/kernel.rst | 11 +++++++++++
>  arch/arm64/kernel/perf_event.c              | 13 +++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> index ddccd10..c2ecd84 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -267,6 +267,17 @@ domain names are in general different. For a detailed discussion
>  see the ``hostname(1)`` man page.
>  
>  
> +export_pmu_events (arm64 only)
> +==============================
> +
> +Controls the PMU export bit (PMCR_EL0.X), which enables the exporting of
> +events over an IMPLEMENTATION DEFINED PMU event export bus to another device.
> +
> +0: disables exporting of events (default).
> +
> +1: enables exporting of events.
> +
> +
>  firmware_config
>  ===============
>  
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index cb69ff1..a8c32a0 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -298,6 +298,7 @@ PMU_FORMAT_ATTR(long, "config1:0");
>  PMU_FORMAT_ATTR(rdpmc, "config1:1");
>  
>  static int sysctl_perf_user_access __read_mostly;
> +static int sysctl_export_pmu_events __read_mostly;
>  
>  static inline bool armv8pmu_event_is_64bit(struct perf_event *event)
>  {
> @@ -1047,6 +1048,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;

I think we need to do this in armv8pmu_start() rather than armv8pmu_reset(),
otherwise any changes to the sysctl at runtime won't take effect unless you
do something like re-online the CPU.

Will

Re: [PATCH V6] arm64: perf: Make exporting of pmu events configurable

Posted by Srinivasarao Pathipati 1 year, 9 months ago

On 6/24/2022 5:48 PM, Will Deacon wrote:
> On Thu, Jun 09, 2022 at 06:29:49PM +0530, Srinivasarao Pathipati wrote:
>> The PMU export bit (PMCR_EL0.X) is getting reset during pmu reset,
>> Make it configurable using sysctls to enable/disable at runtime.
>>
>> Signed-off-by: Srinivasarao Pathipati <quic_c_spathi@quicinc.com>
>> ---
>> Changes since V5:
>> 	- removed configuring with kernel parameters.
>> Changes since V4:
>> 	- Registering sysctls dynamically for only arm64 as suggested by Will
>> 	- Not removed the code to configure with kernel parameters
>> 	  as the sysctl's kernel parameter(sysctl.kernel.export_pmu_events)
>> 	  is not working at early bootup. pmu_reset() getting called before
>> 	  sysctl's kernel parameter is set.
>> 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/sysctl/kernel.rst | 11 +++++++++++
>>   arch/arm64/kernel/perf_event.c              | 13 +++++++++++++
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
>> index ddccd10..c2ecd84 100644
>> --- a/Documentation/admin-guide/sysctl/kernel.rst
>> +++ b/Documentation/admin-guide/sysctl/kernel.rst
>> @@ -267,6 +267,17 @@ domain names are in general different. For a detailed discussion
>>   see the ``hostname(1)`` man page.
>>   
>>   
>> +export_pmu_events (arm64 only)
>> +==============================
>> +
>> +Controls the PMU export bit (PMCR_EL0.X), which enables the exporting of
>> +events over an IMPLEMENTATION DEFINED PMU event export bus to another device.
>> +
>> +0: disables exporting of events (default).
>> +
>> +1: enables exporting of events.
>> +
>> +
>>   firmware_config
>>   ===============
>>   
>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>> index cb69ff1..a8c32a0 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -298,6 +298,7 @@ PMU_FORMAT_ATTR(long, "config1:0");
>>   PMU_FORMAT_ATTR(rdpmc, "config1:1");
>>   
>>   static int sysctl_perf_user_access __read_mostly;
>> +static int sysctl_export_pmu_events __read_mostly;
>>   
>>   static inline bool armv8pmu_event_is_64bit(struct perf_event *event)
>>   {
>> @@ -1047,6 +1048,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;
> I think we need to do this in armv8pmu_start() rather than armv8pmu_reset(),
> otherwise any changes to the sysctl at runtime won't take effect unless you
> do something like re-online the CPU.
>
> Will

Hi Will ,

We think it may not work.   Say for example pmu->reset is called which 
disables the exporting of events,
and pmu->start not called. This would lead to missing of events.

How about enabling it in both reset and start functions? do you see any 
issue?

--Srinivas

Re: [PATCH V6] arm64: perf: Make exporting of pmu events configurable

Posted by Srinivasarao Pathipati 1 year, 1 month ago

On 7/13/2022 9:35 AM, Srinivasarao Pathipati wrote:
>
> On 6/24/2022 5:48 PM, Will Deacon wrote:
>> On Thu, Jun 09, 2022 at 06:29:49PM +0530, Srinivasarao Pathipati wrote:
>>> The PMU export bit (PMCR_EL0.X) is getting reset during pmu reset,
>>> Make it configurable using sysctls to enable/disable at runtime.
>>>
>>> Signed-off-by: Srinivasarao Pathipati <quic_c_spathi@quicinc.com>
>>> ---
>>> Changes since V5:
>>>     - removed configuring with kernel parameters.
>>> Changes since V4:
>>>     - Registering sysctls dynamically for only arm64 as suggested by 
>>> Will
>>>     - Not removed the code to configure with kernel parameters
>>>       as the sysctl's kernel parameter(sysctl.kernel.export_pmu_events)
>>>       is not working at early bootup. pmu_reset() getting called before
>>>       sysctl's kernel parameter is set.
>>> 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/sysctl/kernel.rst | 11 +++++++++++
>>>   arch/arm64/kernel/perf_event.c              | 13 +++++++++++++
>>>   2 files changed, 24 insertions(+)
>>>
>>> diff --git a/Documentation/admin-guide/sysctl/kernel.rst 
>>> b/Documentation/admin-guide/sysctl/kernel.rst
>>> index ddccd10..c2ecd84 100644
>>> --- a/Documentation/admin-guide/sysctl/kernel.rst
>>> +++ b/Documentation/admin-guide/sysctl/kernel.rst
>>> @@ -267,6 +267,17 @@ domain names are in general different. For a 
>>> detailed discussion
>>>   see the ``hostname(1)`` man page.
>>>     +export_pmu_events (arm64 only)
>>> +==============================
>>> +
>>> +Controls the PMU export bit (PMCR_EL0.X), which enables the 
>>> exporting of
>>> +events over an IMPLEMENTATION DEFINED PMU event export bus to 
>>> another device.
>>> +
>>> +0: disables exporting of events (default).
>>> +
>>> +1: enables exporting of events.
>>> +
>>> +
>>>   firmware_config
>>>   ===============
>>>   diff --git a/arch/arm64/kernel/perf_event.c 
>>> b/arch/arm64/kernel/perf_event.c
>>> index cb69ff1..a8c32a0 100644
>>> --- a/arch/arm64/kernel/perf_event.c
>>> +++ b/arch/arm64/kernel/perf_event.c
>>> @@ -298,6 +298,7 @@ PMU_FORMAT_ATTR(long, "config1:0");
>>>   PMU_FORMAT_ATTR(rdpmc, "config1:1");
>>>     static int sysctl_perf_user_access __read_mostly;
>>> +static int sysctl_export_pmu_events __read_mostly;
>>>     static inline bool armv8pmu_event_is_64bit(struct perf_event 
>>> *event)
>>>   {
>>> @@ -1047,6 +1048,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;
>> I think we need to do this in armv8pmu_start() rather than 
>> armv8pmu_reset(),
>> otherwise any changes to the sysctl at runtime won't take effect 
>> unless you
>> do something like re-online the CPU.
>>
>> Will
>
> Hi Will ,
>
> We think it may not work.   Say for example pmu->reset is called which 
> disables the exporting of events,
> and pmu->start not called. This would lead to missing of events.
>
> How about enabling it in both reset and start functions? do you see 
> any issue?
>
> --Srinivas

Hi Will ,

Could you please suggest ,better way to fix this.