| 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
| ||
|
Message-ID: <7c9074ef-46c2-08b8-ab1d-410953145570@quicinc.com>
Date: Wed, 18 May 2022 19:47:05 +0530
From: Srinivasarao Pathipati <quic_c_spathi@...cinc.com>
To: Will Deacon <will@...nel.org>
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 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@...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().
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
Powered by blists - more mailing lists