linux-kernel - Re: [PATCH V4] arm64: perf: Set PMCR.X of PMCR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220517130822.GA3369@willie-the-truck>
Date:   Tue, 17 May 2022 14:08:22 +0100
From:   Will Deacon <will@...nel.org>
To:     Srinivasarao Pathipati <quic_c_spathi@...cinc.com>
Cc:     mark.rutland@....com, peterz@...radead.org, mingo@...hat.com,
        acme@...nel.org, alexander.shishkin@...ux.intel.com,
        jolsa@...nel.org, namhyung@...nel.org, catalin.marinas@....com,
        linux-arm-kernel@...ts.infradead.org,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V4] arm64: perf: Set PMCR.X of PMCR_EL0 during pmu reset

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@...cinc.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().

> +
>  	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