Re: [PATCH v10 3/5] arm64: perf: Add userspace counter access disable switch
From: Mark Rutland <mark.rutland@arm.com>
Date: 2021-10-13 17:30:29
Also in:
linux-perf-users, lkml
On Tue, Sep 14, 2021 at 03:47:58PM -0500, Rob Herring wrote:
Like x86, some users may want to disable userspace PMU counter altogether. Add a sysctl 'perf_user_access' file to control userspace counter access. The default is '0' which is disabled. Writing '1' enables access. Note that x86 also supports writing '2' to globally enable user access.
For clarity it might be worth mentioning that on x86 this is controlled by the PMU's `rdpmc` sysfs attribute, i.e. Note that x86 supports globally enabling user access by writing '2' to /sys/bus/event_source/devices/cpu/rdpmc
quoted hunk ↗ jump to hunk
As there's not existing userspace support to worry about, this shouldn't be necessary for Arm. It could be added later if the need arises. Cc: Mark Rutland <mark.rutland@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 <redacted> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: linux-perf-users@vger.kernel.org Acked-by: Will Deacon <will@kernel.org> Signed-off-by: Rob Herring <robh@kernel.org> --- v10: - Add documentation - Use a custom handler (needed on the next patch) v9: - Use sysctl instead of sysfs attr - Default to disabled v8: - New patch --- Documentation/admin-guide/sysctl/kernel.rst | 11 +++++++++ arch/arm64/kernel/perf_event.c | 27 +++++++++++++++++++++ 2 files changed, 38 insertions(+)diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst index 426162009ce9..346a0dba5703 100644 --- a/Documentation/admin-guide/sysctl/kernel.rst +++ b/Documentation/admin-guide/sysctl/kernel.rst@@ -905,6 +905,17 @@ enabled, otherwise writing to this file will return ``-EBUSY``. The default value is 8. +perf_user_access (arm64 only) +================================= + +Controls user space access for reading perf event counters. When set to 1, +user space can read performance monitor counter registers directly. + +The default value is 0 (access disabled). + +See Documentation/arm64/perf.rst for more information.
Looking at the existing perf sysctls: # ls /proc/sys/kernel/perf* /proc/sys/kernel/perf_cpu_time_max_percent /proc/sys/kernel/perf_event_max_contexts_per_stack /proc/sys/kernel/perf_event_max_sample_rate /proc/sys/kernel/perf_event_max_stack /proc/sys/kernel/perf_event_mlock_kb /proc/sys/kernel/perf_event_paranoid I see that other than `perf_cpu_time_max_percent`, we've used `perf_event_` as the prefix, and I suspect we should do the same here, but I guess it may not matter either way.
quoted hunk ↗ jump to hunk
+ + pid_max =======diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c index b4044469527e..a8f8dd741aeb 100644 --- a/arch/arm64/kernel/perf_event.c +++ b/arch/arm64/kernel/perf_event.c@@ -286,6 +286,8 @@ static const struct attribute_group armv8_pmuv3_events_attr_group = { PMU_FORMAT_ATTR(event, "config:0-15"); PMU_FORMAT_ATTR(long, "config1:0"); +static int sysctl_perf_user_access __read_mostly; + static inline bool armv8pmu_event_is_64bit(struct perf_event *event) { return event->attr.config1 & 0x1;@@ -1104,6 +1106,29 @@ static int armv8pmu_probe_pmu(struct arm_pmu *cpu_pmu) return probe.present ? 0 : -ENODEV; } +int armv8pmu_proc_user_access_handler(struct ctl_table *table, int write, + void *buffer, size_t *lenp, loff_t *ppos) +{ + int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos); + if (ret || !write || sysctl_perf_user_access) + return ret; + + return 0; +}
Maybe this is needed in the next patch, but the if statement is entirely
redundant on this patch and looks really odd.
Can we please either:
1) Use proc_dointvec_minmax() directly in this patch (which is what Will
Acked in v9) and add the wrapper in the next patch when we need it.
2) make this:
| int armv8pmu_proc_user_access_handler(struct ctl_table *table, int write,
| void *buffer, size_t *lenp, loff_t *ppos)
| {
| return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
| }
... and flesh it out in the next patch.
With either of those two options (and regardless of whether the
attribute is renamed):
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Thanks,
Mark.
quoted hunk ↗ jump to hunk
+ + return 0; +} + +static struct ctl_table armv8_pmu_sysctl_table[] = { + { + .procname = "perf_user_access", + .data = &sysctl_perf_user_access, + .maxlen = sizeof(unsigned int), + .mode = 0644, + .proc_handler = armv8pmu_proc_user_access_handler, + .extra1 = SYSCTL_ZERO, + .extra2 = SYSCTL_ONE, + }, + { } +}; + static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name, int (*map_event)(struct perf_event *event), const struct attribute_group *events,@@ -1136,6 +1161,8 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name, cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_CAPS] = caps ? caps : &armv8_pmuv3_caps_attr_group; + register_sysctl("kernel", armv8_pmu_sysctl_table); + return 0; }-- 2.30.2
_______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel