[PATCH v8 0/5] arm64 userspace counter support

[PATCH v8 0/5] arm64 userspace counter support

[Rob Herring]

Another version of arm64 userspace counter access support. This is just the
Arm bits as Arnaldo asked to send the tools separately. The bulk of the
libperf changes landed in 5.13-rc1.

This version departs from the x86 implementation by requiring the user
to always explicitly request user access (via attr.config1) and only
enables access for task bound events. Rather than trying to lock down the
access as the x86 implementation has been doing, we can start with only a
limited use case enabled and later expand it if needed.

This originally resurrected Raphael's series[1] to enable userspace counter
access on arm64. My previous versions are here[2][3][4][5][6][7][8]. A git
branch is here[9].


Changes in v8:
 - Restrict user access to thread bound events which simplifies the
   implementation. A couple of perf core changes (patches 1 and 2) are
   needed to do this.
 - Always require the user to request userspace access.

Changes in v7:
 - Handling of dirty counter leakage and reworking of context switch and
   user access enabling. The .sched_task hook and undef instruction handler
   are now utilized. (Patch 3)
 - Add a userspace disable switch like x86. (Patch 5)

Changes in v6:
 - Reworking of the handling of 64-bit counters and user access. There's
   a new config1 flag to request user access. This takes priority over
   the 64-bit flag and the user will get the maximum size the h/w
   supports without chaining.
 - The libperf evsel mmap struct is stored in its own xyarray
 - New tests for user 64-bit and 32-bit counters
 - Rebase to v5.12-rc2

Changes in v5:
 - Limit enabling/disabling access to CPUs associated with the PMU
   (supported_cpus) and with the mm_struct matching current->active_mm.
   The x86 method of using mm_cpumask doesn't work for arm64 as it is not
   updated.
 - Only set cap_user_rdpmc if event is on current cpu. See patch 2.
 - Create an mmap for every event in an evsel. This results in some changes
   to the libperf mmap API from the last version.
 - Rebase to v5.11-rc2

Changes in v4:
 - Dropped 'arm64: pmu: Add hook to handle pmu-related undefined instructions'.
   The onus is on userspace to pin itself to a homogeneous subset of CPUs
   and avoid any aborts on heterogeneous systems, so the hook is not needed.
 - Make perf_evsel__mmap() take pages rather than bytes for size
 - Fix building arm64 heterogeneous test.

Changes in v3:
 - Dropped removing x86 rdpmc test until libperf tests can run via 'perf test'
 - Added verbose prints for tests
 - Split adding perf_evsel__mmap() to separate patch

The following changes to the arm64 support have been made compared to
Raphael's last version:

The major change is support for heterogeneous systems with some
restrictions. Specifically, userspace must pin itself to like CPUs, open
a specific PMU by type, and use h/w specific events. The tests have been
reworked to demonstrate this.

Chained events are not supported. The problem with supporting chained
events was there's no way to distinguish between a chained event and a
native 64-bit counter. We could add some flag, but do self monitoring
processes really need that? Native 64-bit counters are supported if the
PMU h/w has support. As there's already an explicit ABI to request 64-bit
counters, userspace can request 64-bit counters and if user
access is not enabled, then it must retry with 32-bit counters.

Prior versions broke the build on arm32 (surprisingly never caught by
0-day). As a result, event_mapped and event_unmapped implementations have
been moved into the arm64 code.

There was a bug in that pmc_width was not set in the user page. The tests
now check for this.

The documentation has been converted to rST. I've added sections on
chained events and heterogeneous.

Rob


[1] https://lore.kernel.org/r/20190822144220.27860-1-raphael.gault@arm.com/
[2] https://lore.kernel.org/r/20200707205333.624938-1-robh@kernel.org/
[3] https://lore.kernel.org/r/20200828205614.3391252-1-robh@kernel.org/
[4] https://lore.kernel.org/r/20200911215118.2887710-1-robh@kernel.org/
[5] https://lore.kernel.org/r/20201001140116.651970-1-robh@kernel.org/
[6] https://lore.kernel.org/r/20210114020605.3943992-1-robh@kernel.org/
[7] https://lore.kernel.org/r/20210311000837.3630499-1-robh@kernel.org/
[8] https://lore.kernel.org/r/20210420031511.2348977-1-robh@kernel.org/
[9] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git arm64-user-perf-event-v8


Kan Liang (1):
  perf: Track per-PMU sched_task() callback users

Raphael Gault (1):
  Documentation: arm64: Document PMU counters access from userspace

Rob Herring (3):
  perf: Add a counter for number of user access events in context
  arm64: perf: Enable PMU counter userspace access for perf event
  arm64: perf: Add userspace counter access disable switch

 Documentation/arm64/perf.rst   |  68 ++++++++++-
 arch/arm64/kernel/perf_event.c | 201 +++++++++++++++++++++++++++++++--
 include/linux/perf/arm_pmu.h   |  11 +-
 include/linux/perf_event.h     |   4 +
 kernel/events/core.c           |   8 +-
 5 files changed, 279 insertions(+), 13 deletions(-)

--
2.27.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v8 1/5] perf: Add a counter for number of user access events in context

[Rob Herring]

When scheduling a task's context, we need to know if the task has any
user accessible events in order to enable or disable user access.
Walking the context's list of events would be slow, so add a counter
to track this.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 include/linux/perf_event.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index f5a6a2f069ed..4cf081e22b76 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -822,6 +822,7 @@ struct perf_event_context {
 	int				nr_stat;
 	int				nr_freq;
 	int				rotate_disable;
+	atomic_t			nr_user;
 	/*
 	 * Set when nr_events != nr_active, except tolerant to events not
 	 * necessary to be active due to scheduling constraints, such as cgroups.
-- 
2.27.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v8 2/5] perf: Track per-PMU sched_task() callback users

[Rob Herring]

From: Kan Liang <kan.liang@linux.intel.com>

Current perf only tracks the per-CPU sched_task() callback users, which
doesn't work if a callback user is a task. For example, the dirty
counters have to be cleared to prevent data leakage when a new userspace
access task is scheduled in. The task may be created on one CPU but
running on another CPU. It cannot be tracked by the per-CPU variable. A
global variable is not going to work either because of the hybrid PMUs.
Add a per-PMU variable to track the callback users.

Suggested-by: Rob Herring <robh@kernel.org>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
[robh: Also call sched_task() got sched out cases]
Signed-off-by: Rob Herring <robh@kernel.org>
---
 include/linux/perf_event.h | 3 +++
 kernel/events/core.c       | 8 +++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 4cf081e22b76..a88d52e80864 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -300,6 +300,9 @@ struct pmu {
 	/* number of address filters this PMU can do */
 	unsigned int			nr_addr_filters;
 
+	/* Track the per PMU sched_task() callback users */
+	atomic_t			sched_cb_usage;
+
 	/*
 	 * Fully disable/enable this PMU, can be used to protect from the PMI
 	 * as well as for lazy/batch writing of the MSRs.
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 2e947a485898..6d0507c23240 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3448,7 +3448,8 @@ static void perf_event_context_sched_out(struct task_struct *task, int ctxn,
 
 			perf_pmu_disable(pmu);
 
-			if (cpuctx->sched_cb_usage && pmu->sched_task)
+			if (pmu->sched_task &&
+			    (cpuctx->sched_cb_usage || atomic_read(&pmu->sched_cb_usage)))
 				pmu->sched_task(ctx, false);
 
 			/*
@@ -3488,7 +3489,8 @@ static void perf_event_context_sched_out(struct task_struct *task, int ctxn,
 		raw_spin_lock(&ctx->lock);
 		perf_pmu_disable(pmu);
 
-		if (cpuctx->sched_cb_usage && pmu->sched_task)
+		if (pmu->sched_task &&
+		    (cpuctx->sched_cb_usage || atomic_read(&pmu->sched_cb_usage)))
 			pmu->sched_task(ctx, false);
 		task_ctx_sched_out(cpuctx, ctx, EVENT_ALL);
 
@@ -3851,7 +3853,7 @@ static void perf_event_context_sched_in(struct perf_event_context *ctx,
 		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
 	perf_event_sched_in(cpuctx, ctx, task);
 
-	if (cpuctx->sched_cb_usage && pmu->sched_task)
+	if (pmu->sched_task && (cpuctx->sched_cb_usage || atomic_read(&pmu->sched_cb_usage)))
 		pmu->sched_task(cpuctx->task_ctx, true);
 
 	perf_pmu_enable(pmu);
-- 
2.27.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v8 3/5] arm64: perf: Enable PMU counter userspace access for perf event

[Rob Herring]

Arm PMUs can support direct userspace access of counters which allows for
low overhead (i.e. no syscall) self-monitoring of tasks. The same feature
exists on x86 called 'rdpmc'. Unlike x86, userspace access will only be
enabled for thread bound events. This could be extended if needed, but
simplifies the implementation and reduces the chances for any
information leaks (which the x86 implementation suffers from).

When an event is capable of userspace access and has been mmapped, userspace
access is enabled when the event is scheduled on a CPU's PMU. There's some
additional overhead clearing counters when disabled in order to prevent
leaking disabled counter data from other tasks.

Unlike x86, enabling of userspace access must be requested with a new
attr bit: config1:1. If the user requests userspace access and 64-bit
counters, then chaining will be disabled and the user will get the
maximum size counter the underlying h/w can support. The modes for
config1 are as follows:

config1 = 0 : user access disabled and always 32-bit
config1 = 1 : user access disabled and always 64-bit (using chaining if needed)
config1 = 2 : user access enabled and always 32-bit
config1 = 3 : user access enabled and counter size matches underlying counter.

Based on work by Raphael Gault <raphael.gault@arm.com>, but has been
completely re-written.

Signed-off-by: Rob Herring <robh@kernel.org>
---
v8:
 - Rework user access tracking and enabling to be done on task
   context changes using sched_task() hook. This avoids the need for any
   IPIs, mm_switch hooks or undef instr handler.
 - Only support user access when explicitly requested on open and
   only for a thread bound events. This avoids some of the information
   leaks x86 has and simplifies the implementation.

v7:
 - Clear disabled counters when user access is enabled for a task to
   avoid leaking other tasks counter data.
 - Rework context switch handling utilizing sched_task callback
 - Add armv8pmu_event_can_chain() helper
 - Rework config1 flags handling structure
 - Use ARMV8_IDX_CYCLE_COUNTER_USER define for remapped user cycle
   counter index

v6:
 - Add new attr.config1 rdpmc bit for userspace to hint it wants
   userspace access when also requesting 64-bit counters.

v5:
 - Only set cap_user_rdpmc if event is on current cpu
 - Limit enabling/disabling access to CPUs associated with the PMU
   (supported_cpus) and with the mm_struct matching current->active_mm.

v2:
 - Move mapped/unmapped into arm64 code. Fixes arm32.
 - Rebase on cap_user_time_short changes

Changes from Raphael's v4:
  - Drop homogeneous check
  - Disable access for chained counters
  - Set pmc_width in user page

[1] https://lore.kernel.org/lkml/CAL_JsqK+eKef5NaVnBfARCjRE3MYhfBfe54F9YHKbsTnWqLmLw@mail.gmail.com/
---
 arch/arm64/kernel/perf_event.c | 141 +++++++++++++++++++++++++++++++--
 include/linux/perf/arm_pmu.h   |   7 ++
 2 files changed, 140 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index f594957e29bd..dc79cf7b58ee 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -288,15 +288,22 @@ static const struct attribute_group armv8_pmuv3_events_attr_group = {
 
 PMU_FORMAT_ATTR(event, "config:0-15");
 PMU_FORMAT_ATTR(long, "config1:0");
+PMU_FORMAT_ATTR(rdpmc, "config1:1");
 
 static inline bool armv8pmu_event_is_64bit(struct perf_event *event)
 {
 	return event->attr.config1 & 0x1;
 }
 
+static inline bool armv8pmu_event_want_user_access(struct perf_event *event)
+{
+	return event->attr.config1 & 0x2;
+}
+
 static struct attribute *armv8_pmuv3_format_attrs[] = {
 	&format_attr_event.attr,
 	&format_attr_long.attr,
+	&format_attr_rdpmc.attr,
 	NULL,
 };
 
@@ -332,7 +339,7 @@ static const struct attribute_group armv8_pmuv3_caps_attr_group = {
  */
 #define	ARMV8_IDX_CYCLE_COUNTER	0
 #define	ARMV8_IDX_COUNTER0	1
-
+#define	ARMV8_IDX_CYCLE_COUNTER_USER	32
 
 /*
  * We unconditionally enable ARMv8.5-PMU long event counter support
@@ -344,6 +351,15 @@ static bool armv8pmu_has_long_event(struct arm_pmu *cpu_pmu)
 	return (cpu_pmu->pmuver >= ID_AA64DFR0_PMUVER_8_5);
 }
 
+static inline bool armv8pmu_event_can_chain(struct perf_event *event)
+{
+	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
+
+	return !(event->hw.flags & ARMPMU_EL0_RD_CNTR) &&
+	       armv8pmu_event_is_64bit(event) &&
+	       !armv8pmu_has_long_event(cpu_pmu);
+}
+
 /*
  * We must chain two programmable counters for 64 bit events,
  * except when we have allocated the 64bit cycle counter (for CPU
@@ -353,11 +369,9 @@ static bool armv8pmu_has_long_event(struct arm_pmu *cpu_pmu)
 static inline bool armv8pmu_event_is_chained(struct perf_event *event)
 {
 	int idx = event->hw.idx;
-	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
 
 	return !WARN_ON(idx < 0) &&
-	       armv8pmu_event_is_64bit(event) &&
-	       !armv8pmu_has_long_event(cpu_pmu) &&
+	       armv8pmu_event_can_chain(event) &&
 	       (idx != ARMV8_IDX_CYCLE_COUNTER);
 }
 
@@ -688,6 +702,32 @@ static inline u32 armv8pmu_getreset_flags(void)
 	return value;
 }
 
+static void armv8pmu_disable_user_access(void)
+{
+	write_sysreg(0, pmuserenr_el0);
+}
+
+static void armv8pmu_enable_user_access(struct arm_pmu *cpu_pmu)
+{
+	struct pmu_hw_events *cpuc = this_cpu_ptr(cpu_pmu->hw_events);
+
+	if (!bitmap_empty(cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS)) {
+		int i;
+		/* Don't need to clear assigned counters. */
+		bitmap_xor(cpuc->dirty_mask, cpuc->dirty_mask, cpuc->used_mask, ARMPMU_MAX_HWEVENTS);
+
+		for_each_set_bit(i, cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS) {
+			if (i == ARMV8_IDX_CYCLE_COUNTER)
+				write_sysreg(0, pmccntr_el0);
+			else
+				armv8pmu_write_evcntr(i, 0);
+		}
+		bitmap_zero(cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS);
+	}
+
+	write_sysreg(ARMV8_PMU_USERENR_ER | ARMV8_PMU_USERENR_CR, pmuserenr_el0);
+}
+
 static void armv8pmu_enable_event(struct perf_event *event)
 {
 	/*
@@ -848,13 +888,16 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc,
 	if (evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) {
 		if (!test_and_set_bit(ARMV8_IDX_CYCLE_COUNTER, cpuc->used_mask))
 			return ARMV8_IDX_CYCLE_COUNTER;
+		else if (armv8pmu_event_is_64bit(event) &&
+			   armv8pmu_event_want_user_access(event) &&
+			   !armv8pmu_has_long_event(cpu_pmu))
+				return -EAGAIN;
 	}
 
 	/*
 	 * Otherwise use events counters
 	 */
-	if (armv8pmu_event_is_64bit(event) &&
-	    !armv8pmu_has_long_event(cpu_pmu))
+	if (armv8pmu_event_can_chain(event))
 		return	armv8pmu_get_chain_idx(cpuc, cpu_pmu);
 	else
 		return armv8pmu_get_single_idx(cpuc, cpu_pmu);
@@ -866,8 +909,60 @@ static void armv8pmu_clear_event_idx(struct pmu_hw_events *cpuc,
 	int idx = event->hw.idx;
 
 	clear_bit(idx, cpuc->used_mask);
-	if (armv8pmu_event_is_chained(event))
+	set_bit(idx, cpuc->dirty_mask);
+	if (armv8pmu_event_is_chained(event)) {
 		clear_bit(idx - 1, cpuc->used_mask);
+		set_bit(idx - 1, cpuc->dirty_mask);
+	}
+}
+
+static int armv8pmu_access_event_idx(struct perf_event *event)
+{
+	if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+		return 0;
+
+	/*
+	 * We remap the cycle counter index to 32 to
+	 * match the offset applied to the rest of
+	 * the counter indices.
+	 */
+	if (event->hw.idx == ARMV8_IDX_CYCLE_COUNTER)
+		return ARMV8_IDX_CYCLE_COUNTER_USER;
+
+	return event->hw.idx;
+}
+
+void armv8pmu_sched_task(struct perf_event_context *ctx, bool sched_in)
+{
+	if (sched_in && atomic_read(&ctx->nr_user))
+		armv8pmu_enable_user_access(to_arm_pmu(ctx->pmu));
+	else
+		armv8pmu_disable_user_access();
+}
+
+static void armv8pmu_event_mapped(struct perf_event *event, struct mm_struct *mm)
+{
+	if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR) || (atomic_read(&event->mmap_count) != 1))
+		return;
+
+	if (atomic_inc_return(&event->ctx->nr_user) == 1) {
+		unsigned long flags;
+		atomic_inc(&event->pmu->sched_cb_usage);
+		local_irq_save(flags);
+		armv8pmu_enable_user_access(to_arm_pmu(event->pmu));
+		local_irq_restore(flags);
+	}
+}
+
+static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
+{
+	if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR) || (atomic_read(&event->mmap_count) != 1))
+		return;
+
+	if (atomic_dec_and_test(&event->ctx->nr_user)) {
+		atomic_dec(&event->pmu->sched_cb_usage);
+		armv8pmu_disable_user_access();
+	}
 }
 
 /*
@@ -963,9 +1058,22 @@ static int __armv8_pmuv3_map_event(struct perf_event *event,
 				       &armv8_pmuv3_perf_cache_map,
 				       ARMV8_PMU_EVTYPE_EVENT);
 
-	if (armv8pmu_event_is_64bit(event))
+	/*
+	 * At this point, the counter is not assigned. If a 64-bit counter is
+	 * requested, we must make sure the h/w has 64-bit counters if we set
+	 * the event size to 64-bit because chaining is not supported with
+	 * userspace access. This may still fail later on if the CPU cycle
+	 * counter is in use.
+	 */
+	if (armv8pmu_event_is_64bit(event) &&
+	    (!armv8pmu_event_want_user_access(event) ||
+	     armv8pmu_has_long_event(armpmu) || (hw_event_id == ARMV8_PMUV3_PERFCTR_CPU_CYCLES)))
 		event->hw.flags |= ARMPMU_EVT_64BIT;
 
+	/* Userspace counter access only enabled if requested and a per task event */
+	if (armv8pmu_event_want_user_access(event) && event->hw.target)
+		event->hw.flags |= ARMPMU_EL0_RD_CNTR;
+
 	/* Only expose micro/arch events supported by this PMU */
 	if ((hw_event_id > 0) && (hw_event_id < ARMV8_PMUV3_MAX_COMMON_EVENTS)
 	    && test_bit(hw_event_id, armpmu->pmceid_bitmap)) {
@@ -1097,6 +1205,11 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name,
 	cpu_pmu->set_event_filter	= armv8pmu_set_event_filter;
 	cpu_pmu->filter_match		= armv8pmu_filter_match;
 
+	cpu_pmu->pmu.event_idx		= armv8pmu_access_event_idx;
+	cpu_pmu->pmu.event_mapped	= armv8pmu_event_mapped;
+	cpu_pmu->pmu.event_unmapped	= armv8pmu_event_unmapped;
+	cpu_pmu->pmu.sched_task		= armv8pmu_sched_task;
+
 	cpu_pmu->name			= name;
 	cpu_pmu->map_event		= map_event;
 	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_EVENTS] = events ?
@@ -1271,6 +1384,18 @@ void arch_perf_update_userpage(struct perf_event *event,
 	userpg->cap_user_time = 0;
 	userpg->cap_user_time_zero = 0;
 	userpg->cap_user_time_short = 0;
+	userpg->cap_user_rdpmc = !!userpg->index;
+
+	if (userpg->cap_user_rdpmc) {
+		struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
+
+		if (armv8pmu_event_is_64bit(event) &&
+		    (armv8pmu_has_long_event(cpu_pmu) ||
+		     (userpg->index == ARMV8_IDX_CYCLE_COUNTER_USER)))
+			userpg->pmc_width = 64;
+		else
+			userpg->pmc_width = 32;
+	}
 
 	do {
 		rd = sched_clock_read_begin(&seq);
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index 505480217cf1..02ab0010c6d0 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -26,6 +26,7 @@
  */
 /* Event uses a 64bit counter */
 #define ARMPMU_EVT_64BIT		1
+#define ARMPMU_EL0_RD_CNTR		2
 
 #define HW_OP_UNSUPPORTED		0xFFFF
 #define C(_x)				PERF_COUNT_HW_CACHE_##_x
@@ -54,6 +55,12 @@ struct pmu_hw_events {
 	 */
 	DECLARE_BITMAP(used_mask, ARMPMU_MAX_HWEVENTS);
 
+	/*
+	 * A 1 bit for an index indicates that the counter has been used for
+	 * an event and has not been cleared.
+	 */
+	DECLARE_BITMAP(dirty_mask, ARMPMU_MAX_HWEVENTS);
+
 	/*
 	 * Hardware lock to serialize accesses to PMU registers. Needed for the
 	 * read/modify/write sequences.
-- 
2.27.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v8 4/5] arm64: perf: Add userspace counter access disable switch

[Rob Herring]

Like x86, some users may want to disable userspace PMU counter
altogether. Add a sysfs 'rdpmc' file to control userspace counter
access. The default is '1' which is enabled. Writing '0' disables
access.

In the case of multiple PMUs (i.e. big.LITTLE), the control is per PMU
and userspace must disable access on each PMU.

Note that x86 also supports writing '2' to globally enable user access.
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.

Signed-off-by: Rob Herring <robh@kernel.org>
---
v8:
 - Adjust due to patch 3 changes
v7:
 - New patch
---
 arch/arm64/kernel/perf_event.c | 64 ++++++++++++++++++++++++++++++++--
 include/linux/perf/arm_pmu.h   |  4 ++-
 2 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index dc79cf7b58ee..4e7b93c23db9 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -334,6 +334,60 @@ static const struct attribute_group armv8_pmuv3_caps_attr_group = {
 	.attrs = armv8_pmuv3_caps_attrs,
 };

+static void armv8pmu_disable_user_access(void);
+
+static void armv8pmu_disable_user_access_ipi(void *unused)
+{
+	armv8pmu_disable_user_access();
+}
+
+static ssize_t get_attr_rdpmc(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct pmu *pmu = dev_get_drvdata(dev);
+	struct arm_pmu *cpu_pmu = container_of(pmu, struct arm_pmu, pmu);
+
+	return snprintf(buf, 40, "%d\n", cpu_pmu->attr_rdpmc);
+}
+
+static ssize_t set_attr_rdpmc(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct pmu *pmu = dev_get_drvdata(dev);
+	struct arm_pmu *cpu_pmu = container_of(pmu, struct arm_pmu, pmu);
+	unsigned long val;
+	ssize_t ret;
+
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val > 1)
+		return -EINVAL;
+
+	if (!!val != cpu_pmu->attr_rdpmc) {
+		cpu_pmu->attr_rdpmc = !!val;
+		if (!val)
+			on_each_cpu_mask(&cpu_pmu->supported_cpus,
+				armv8pmu_disable_user_access_ipi, NULL, 1);
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR(rdpmc, S_IRUSR | S_IWUSR, get_attr_rdpmc, set_attr_rdpmc);
+
+static struct attribute *armv8_pmuv3_rdpmc_attrs[] = {
+	&dev_attr_rdpmc.attr,
+	NULL,
+};
+
+static const struct attribute_group armv8_pmuv3_rdpmc_attr_group = {
+	.attrs = armv8_pmuv3_rdpmc_attrs,
+};
+
 /*
  * Perf Events' indices
  */
@@ -711,6 +765,9 @@ static void armv8pmu_enable_user_access(struct arm_pmu *cpu_pmu)
 {
 	struct pmu_hw_events *cpuc = this_cpu_ptr(cpu_pmu->hw_events);

+	if (!cpu_pmu->attr_rdpmc)
+		return;
+
 	if (!bitmap_empty(cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS)) {
 		int i;
 		/* Don't need to clear assigned counters. */
@@ -918,7 +975,7 @@ static void armv8pmu_clear_event_idx(struct pmu_hw_events *cpuc,

 static int armv8pmu_access_event_idx(struct perf_event *event)
 {
-	if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+	if (!to_arm_pmu(event->pmu)->attr_rdpmc || !(event->hw.flags & ARMPMU_EL0_RD_CNTR))
 		return 0;

 	/*
@@ -1071,7 +1128,7 @@ static int __armv8_pmuv3_map_event(struct perf_event *event,
 		event->hw.flags |= ARMPMU_EVT_64BIT;

 	/* Userspace counter access only enabled if requested and a per task event */
-	if (armv8pmu_event_want_user_access(event) && event->hw.target)
+	if (to_arm_pmu(event->pmu)->attr_rdpmc && armv8pmu_event_want_user_access(event) && event->hw.target)
 		event->hw.flags |= ARMPMU_EL0_RD_CNTR;

 	/* Only expose micro/arch events supported by this PMU */
@@ -1219,6 +1276,9 @@ 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;

+	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_RDPMC] = &armv8_pmuv3_rdpmc_attr_group;
+	cpu_pmu->attr_rdpmc = true;
+
 	return 0;
 }

diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index 02ab0010c6d0..2ecd41df29dd 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -81,6 +81,7 @@ enum armpmu_attr_groups {
 	ARMPMU_ATTR_GROUP_EVENTS,
 	ARMPMU_ATTR_GROUP_FORMATS,
 	ARMPMU_ATTR_GROUP_CAPS,
+	ARMPMU_ATTR_GROUP_RDPMC,
 	ARMPMU_NR_ATTR_GROUPS
 };

@@ -106,7 +107,8 @@ struct arm_pmu {
 	int		(*map_event)(struct perf_event *event);
 	int		(*filter_match)(struct perf_event *event);
 	int		num_events;
-	bool		secure_access; /* 32-bit ARM only */
+	bool		secure_access:1; /* 32-bit ARM only */
+	bool		attr_rdpmc:1;
 #define ARMV8_PMUV3_MAX_COMMON_EVENTS		0x40
 	DECLARE_BITMAP(pmceid_bitmap, ARMV8_PMUV3_MAX_COMMON_EVENTS);
 #define ARMV8_PMUV3_EXT_COMMON_EVENT_BASE	0x4000
--
2.27.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v8 5/5] Documentation: arm64: Document PMU counters access from userspace

[Rob Herring]

From: Raphael Gault <raphael.gault@arm.com>

Add documentation to describe the access to the pmu hardware counters from
userspace.

Signed-off-by: Raphael Gault <raphael.gault@arm.com>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v8:
 - Reword that config1:1 must always be set to request user access
v7:
 - Merge into existing arm64 perf.rst
v6:
  - Update the chained event section with attr.config1 details
v2:
  - Update links to test examples

Changes from Raphael's v4:
  - Convert to rSt
  - Update chained event status
  - Add section for heterogeneous systems
---
 Documentation/arm64/perf.rst | 68 +++++++++++++++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/Documentation/arm64/perf.rst b/Documentation/arm64/perf.rst
index b567f177d385..5dcbb508586f 100644
--- a/Documentation/arm64/perf.rst
+++ b/Documentation/arm64/perf.rst
@@ -2,7 +2,10 @@
 
 .. _perf_index:
 
-=====================
+====
+Perf
+====
+
 Perf Event Attributes
 =====================
 
@@ -88,3 +91,66 @@ exclude_host. However when using !exclude_hv there is a small blackout
 window at the guest entry/exit where host events are not captured.
 
 On VHE systems there are no blackout windows.
+
+Perf Userspace PMU Hardware Counter Access
+==========================================
+
+Overview
+--------
+The perf userspace tool relies on the PMU to monitor events. It offers an
+abstraction layer over the hardware counters since the underlying
+implementation is cpu-dependent.
+Arm64 allows userspace tools to have access to the registers storing the
+hardware counters' values directly.
+
+This targets specifically self-monitoring tasks in order to reduce the overhead
+by directly accessing the registers without having to go through the kernel.
+
+How-to
+------
+The focus is set on the armv8 PMUv3 which makes sure that the access to the pmu
+registers is enabled and that the userspace has access to the relevant
+information in order to use them.
+
+In order to have access to the hardware counter it is necessary to open the 
+event using the perf tool interface with config1:1 attr bit set: the 
+sys_perf_event_open syscall returns a fd which can subsequently be used 
+with the mmap syscall in order to retrieve a page of memory containing 
+information about the event. The PMU driver uses this page to expose to 
+the user the hardware counter's index and other necessary data. Using 
+this index enables the user to access the PMU registers using the `mrs` 
+instruction.
+
+The userspace access is supported in libperf using the perf_evsel__mmap()
+and perf_evsel__read() functions. See `tools/lib/perf/tests/test-evsel.c`_ for
+an example.
+
+About heterogeneous systems
+---------------------------
+On heterogeneous systems such as big.LITTLE, userspace PMU counter access can
+only be enabled when the tasks are pinned to a homogeneous subset of cores and
+the corresponding PMU instance is opened by specifying the 'type' attribute.
+The use of generic event types is not supported in this case.
+
+Have a look at `tools/perf/arch/arm64/tests/user-events.c`_ for an example. It
+can be run using the perf tool to check that the access to the registers works
+correctly from userspace:
+
+.. code-block:: sh
+
+  perf test -v user
+
+About chained events and 64-bit counters
+----------------------------------------
+Chained events are not supported in conjunction with userspace counter
+access. If a 64-bit counter is requested (attr.config1:0) with userspace 
+access (attr.config1:1 set), then counter chaining will be disabled. The 
+'pmc_width' in the user page will indicate the actual width of the 
+counter which could be only 32-bits depending on the event and PMU 
+features.
+
+.. Links
+.. _tools/perf/arch/arm64/tests/user-events.c:
+   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/arch/arm64/tests/user-events.c
+.. _tools/lib/perf/tests/test-evsel.c:
+   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/perf/tests/test-evsel.c
-- 
2.27.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 4/5] arm64: perf: Add userspace counter access disable switch

[Will Deacon]

Hi Rob,

On Mon, May 17, 2021 at 02:54:04PM -0500, Rob Herring wrote:
> Like x86, some users may want to disable userspace PMU counter
> altogether. Add a sysfs 'rdpmc' file to control userspace counter
> access. The default is '1' which is enabled. Writing '0' disables
> access.
> 
> In the case of multiple PMUs (i.e. big.LITTLE), the control is per PMU
> and userspace must disable access on each PMU.
> 
> Note that x86 also supports writing '2' to globally enable user access.
> 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.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v8:
>  - Adjust due to patch 3 changes
> v7:
>  - New patch
> ---
>  arch/arm64/kernel/perf_event.c | 64 ++++++++++++++++++++++++++++++++--
>  include/linux/perf/arm_pmu.h   |  4 ++-
>  2 files changed, 65 insertions(+), 3 deletions(-)

I understand you've tried to follow the x86 behaviour here, but I think it
might be better to implement this as a sysctl on arm64, with the default
behaviour being that userspace access is _disabled_. Having the attribute
per-PMU doesn't really make a lot of sense to me and we don't have any
compatibility issues to worry about given that we've not exposed this to
userspace yet.

That should also be straightforward to implement (famous last words... yell
if it isn't).

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 3/5] arm64: perf: Enable PMU counter userspace access for perf event

[Mark Rutland]

On Mon, May 17, 2021 at 02:54:03PM -0500, Rob Herring wrote:
> Arm PMUs can support direct userspace access of counters which allows for
> low overhead (i.e. no syscall) self-monitoring of tasks. The same feature
> exists on x86 called 'rdpmc'. Unlike x86, userspace access will only be
> enabled for thread bound events. This could be extended if needed, but
> simplifies the implementation and reduces the chances for any
> information leaks (which the x86 implementation suffers from).
> 
> When an event is capable of userspace access and has been mmapped, userspace
> access is enabled when the event is scheduled on a CPU's PMU. There's some
> additional overhead clearing counters when disabled in order to prevent
> leaking disabled counter data from other tasks.
> 
> Unlike x86, enabling of userspace access must be requested with a new
> attr bit: config1:1. If the user requests userspace access and 64-bit
> counters, then chaining will be disabled and the user will get the
> maximum size counter the underlying h/w can support. The modes for
> config1 are as follows:
> 
> config1 = 0 : user access disabled and always 32-bit
> config1 = 1 : user access disabled and always 64-bit (using chaining if needed)
> config1 = 2 : user access enabled and always 32-bit
> config1 = 3 : user access enabled and counter size matches underlying counter.
> 
> Based on work by Raphael Gault <raphael.gault@arm.com>, but has been
> completely re-written.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>

[...]

> +static void armv8pmu_enable_user_access(struct arm_pmu *cpu_pmu)
> +{
> +	struct pmu_hw_events *cpuc = this_cpu_ptr(cpu_pmu->hw_events);
> +
> +	if (!bitmap_empty(cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS)) {
> +		int i;
> +		/* Don't need to clear assigned counters. */
> +		bitmap_xor(cpuc->dirty_mask, cpuc->dirty_mask, cpuc->used_mask, ARMPMU_MAX_HWEVENTS);
> +
> +		for_each_set_bit(i, cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS) {
> +			if (i == ARMV8_IDX_CYCLE_COUNTER)
> +				write_sysreg(0, pmccntr_el0);
> +			else
> +				armv8pmu_write_evcntr(i, 0);
> +		}
> +		bitmap_zero(cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS);
> +	}
> +
> +	write_sysreg(ARMV8_PMU_USERENR_ER | ARMV8_PMU_USERENR_CR, pmuserenr_el0);
> +}

This still leaks the values of CPU-bound events, or task-bound events
owned by others, right?

[...]

> +static void armv8pmu_event_mapped(struct perf_event *event, struct mm_struct *mm)
> +{
> +	if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR) || (atomic_read(&event->mmap_count) != 1))
> +		return;
> +
> +	if (atomic_inc_return(&event->ctx->nr_user) == 1) {
> +		unsigned long flags;
> +		atomic_inc(&event->pmu->sched_cb_usage);
> +		local_irq_save(flags);
> +		armv8pmu_enable_user_access(to_arm_pmu(event->pmu));
> +		local_irq_restore(flags);
> +	}
> +}
> +
> +static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
> +{
> +	if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR) || (atomic_read(&event->mmap_count) != 1))
> +		return;
> +
> +	if (atomic_dec_and_test(&event->ctx->nr_user)) {
> +		atomic_dec(&event->pmu->sched_cb_usage);
> +		armv8pmu_disable_user_access();
> +	}
>  }

We can open an event for task A, but call mmap()/munmap() for that event
from task B, which will do the enable/disable on task B rather than task
A. The core doesn't enforce that the mmap is performed on the same core,
so I don't think this is quite right, unfortunately.

I reckon we need to do something with task_function_call() to make this
happen in the context of the expected task.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 3/5] arm64: perf: Enable PMU counter userspace access for perf event

[Rob Herring]

On Tue, Jun 1, 2021 at 8:55 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Mon, May 17, 2021 at 02:54:03PM -0500, Rob Herring wrote:
> > Arm PMUs can support direct userspace access of counters which allows for
> > low overhead (i.e. no syscall) self-monitoring of tasks. The same feature
> > exists on x86 called 'rdpmc'. Unlike x86, userspace access will only be
> > enabled for thread bound events. This could be extended if needed, but
> > simplifies the implementation and reduces the chances for any
> > information leaks (which the x86 implementation suffers from).
> >
> > When an event is capable of userspace access and has been mmapped, userspace
> > access is enabled when the event is scheduled on a CPU's PMU. There's some
> > additional overhead clearing counters when disabled in order to prevent
> > leaking disabled counter data from other tasks.
> >
> > Unlike x86, enabling of userspace access must be requested with a new
> > attr bit: config1:1. If the user requests userspace access and 64-bit
> > counters, then chaining will be disabled and the user will get the
> > maximum size counter the underlying h/w can support. The modes for
> > config1 are as follows:
> >
> > config1 = 0 : user access disabled and always 32-bit
> > config1 = 1 : user access disabled and always 64-bit (using chaining if needed)
> > config1 = 2 : user access enabled and always 32-bit
> > config1 = 3 : user access enabled and counter size matches underlying counter.
> >
> > Based on work by Raphael Gault <raphael.gault@arm.com>, but has been
> > completely re-written.
> >
> > Signed-off-by: Rob Herring <robh@kernel.org>
>
> [...]
>
> > +static void armv8pmu_enable_user_access(struct arm_pmu *cpu_pmu)
> > +{
> > +     struct pmu_hw_events *cpuc = this_cpu_ptr(cpu_pmu->hw_events);
> > +
> > +     if (!bitmap_empty(cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS)) {
> > +             int i;
> > +             /* Don't need to clear assigned counters. */
> > +             bitmap_xor(cpuc->dirty_mask, cpuc->dirty_mask, cpuc->used_mask, ARMPMU_MAX_HWEVENTS);
> > +
> > +             for_each_set_bit(i, cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS) {
> > +                     if (i == ARMV8_IDX_CYCLE_COUNTER)
> > +                             write_sysreg(0, pmccntr_el0);
> > +                     else
> > +                             armv8pmu_write_evcntr(i, 0);
> > +             }
> > +             bitmap_zero(cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS);
> > +     }
> > +
> > +     write_sysreg(ARMV8_PMU_USERENR_ER | ARMV8_PMU_USERENR_CR, pmuserenr_el0);
> > +}
>
> This still leaks the values of CPU-bound events, or task-bound events
> owned by others, right?

For CPU-bound events, yes. There's not any way to prevent that without
per counter access controls.

It is clearing other's task-bound events.

> [...]
>
> > +static void armv8pmu_event_mapped(struct perf_event *event, struct mm_struct *mm)
> > +{
> > +     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR) || (atomic_read(&event->mmap_count) != 1))
> > +             return;
> > +
> > +     if (atomic_inc_return(&event->ctx->nr_user) == 1) {
> > +             unsigned long flags;
> > +             atomic_inc(&event->pmu->sched_cb_usage);
> > +             local_irq_save(flags);
> > +             armv8pmu_enable_user_access(to_arm_pmu(event->pmu));
> > +             local_irq_restore(flags);
> > +     }
> > +}
> > +
> > +static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
> > +{
> > +     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR) || (atomic_read(&event->mmap_count) != 1))
> > +             return;
> > +
> > +     if (atomic_dec_and_test(&event->ctx->nr_user)) {
> > +             atomic_dec(&event->pmu->sched_cb_usage);
> > +             armv8pmu_disable_user_access();
> > +     }
> >  }
>
> We can open an event for task A, but call mmap()/munmap() for that event
> from task B, which will do the enable/disable on task B rather than task
> A. The core doesn't enforce that the mmap is performed on the same core,
> so I don't think this is quite right, unfortunately.

Why do we care and who wants to do that? It all seems like a
convoluted scenario that isn't really going to happen. I prefer to not
support that until someone asks for it. Maybe we should check for the
condition (event->ctx->task != current) though.

> I reckon we need to do something with task_function_call() to make this
> happen in the context of the expected task.

Following the x86 way using the mm_context and IPI handled that case,
but Will didn't like either part of that implementation. His
suggestion then was to enable access in an undef instr handler rather
than an IPI. I think that would still work with this version.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 3/5] arm64: perf: Enable PMU counter userspace access for perf event

[Mark Rutland]

On Tue, Jun 01, 2021 at 10:00:53AM -0500, Rob Herring wrote:
> On Tue, Jun 1, 2021 at 8:55 AM Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, May 17, 2021 at 02:54:03PM -0500, Rob Herring wrote:
> > > +static void armv8pmu_enable_user_access(struct arm_pmu *cpu_pmu)
> > > +{
> > > +     struct pmu_hw_events *cpuc = this_cpu_ptr(cpu_pmu->hw_events);
> > > +
> > > +     if (!bitmap_empty(cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS)) {
> > > +             int i;
> > > +             /* Don't need to clear assigned counters. */
> > > +             bitmap_xor(cpuc->dirty_mask, cpuc->dirty_mask, cpuc->used_mask, ARMPMU_MAX_HWEVENTS);
> > > +
> > > +             for_each_set_bit(i, cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS) {
> > > +                     if (i == ARMV8_IDX_CYCLE_COUNTER)
> > > +                             write_sysreg(0, pmccntr_el0);
> > > +                     else
> > > +                             armv8pmu_write_evcntr(i, 0);
> > > +             }
> > > +             bitmap_zero(cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS);
> > > +     }
> > > +
> > > +     write_sysreg(ARMV8_PMU_USERENR_ER | ARMV8_PMU_USERENR_CR, pmuserenr_el0);
> > > +}
> >
> > This still leaks the values of CPU-bound events, or task-bound events
> > owned by others, right?
> 
> For CPU-bound events, yes. There's not any way to prevent that without
> per counter access controls.
> 
> It is clearing other's task-bound events.

Sorry, I misspoke. When I said "task-bound events owned by others", I
had meant events bounds to this task, but owned by someone else (e.g.
the system administrator).

Thanks for confirming!

> > > +static void armv8pmu_event_mapped(struct perf_event *event, struct mm_struct *mm)
> > > +{
> > > +     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR) || (atomic_read(&event->mmap_count) != 1))
> > > +             return;
> > > +
> > > +     if (atomic_inc_return(&event->ctx->nr_user) == 1) {
> > > +             unsigned long flags;
> > > +             atomic_inc(&event->pmu->sched_cb_usage);
> > > +             local_irq_save(flags);
> > > +             armv8pmu_enable_user_access(to_arm_pmu(event->pmu));
> > > +             local_irq_restore(flags);
> > > +     }
> > > +}
> > > +
> > > +static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
> > > +{
> > > +     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR) || (atomic_read(&event->mmap_count) != 1))
> > > +             return;
> > > +
> > > +     if (atomic_dec_and_test(&event->ctx->nr_user)) {
> > > +             atomic_dec(&event->pmu->sched_cb_usage);
> > > +             armv8pmu_disable_user_access();
> > > +     }
> > >  }
> >
> > We can open an event for task A, but call mmap()/munmap() for that event
> > from task B, which will do the enable/disable on task B rather than task
> > A. The core doesn't enforce that the mmap is performed on the same core,
> > so I don't think this is quite right, unfortunately.
> 
> Why do we care and who wants to do that? It all seems like a
> convoluted scenario that isn't really going to happen. I prefer to not
> support that until someone asks for it. Maybe we should check for the
> condition (event->ctx->task != current) though.

My reason for caring is that it means our accounting structures aren't
aligned with the actual CPU state, and it's going to be very easy for
this to go wrong as things get reworked in future.

If we're saying someone shouldn't do this, then we should enforce that
they can't. If they can, then I'd prefer to have deterministic behaviour
that isn't going to cause us issues in future.

I think that we should treat this like changing other event properties
(e.g. the period, or enabling/disabling the event), with an
event_function_call, since that will do the right thing regardless. I
also expect that people will want to do setup/teardown in a single
thread, and this would make that work too.

I reckon we can get the core logic for that in kernel/events/core.c.

> > I reckon we need to do something with task_function_call() to make this
> > happen in the context of the expected task.
> 
> Following the x86 way using the mm_context and IPI handled that case,
> but Will didn't like either part of that implementation. His
> suggestion then was to enable access in an undef instr handler rather
> than an IPI. I think that would still work with this version.

IIUC the main objection there was that we'd have to signal all tasks
associated with the mm or all CPUs, neither of which were attractive.

Now that we're saying this is strictly task-bound, I think this is
similar to enabling/disabling an event or changing its period, for which
we already do a (single) event_function_call() to ensure the CPU state
change happens on the correct CPU.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 3/5] arm64: perf: Enable PMU counter userspace access for perf event

[Rob Herring]

On Tue, Jun 1, 2021 at 12:12 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Tue, Jun 01, 2021 at 10:00:53AM -0500, Rob Herring wrote:
> > On Tue, Jun 1, 2021 at 8:55 AM Mark Rutland <mark.rutland@arm.com> wrote:
> > > On Mon, May 17, 2021 at 02:54:03PM -0500, Rob Herring wrote:
> > > > +static void armv8pmu_enable_user_access(struct arm_pmu *cpu_pmu)
> > > > +{
> > > > +     struct pmu_hw_events *cpuc = this_cpu_ptr(cpu_pmu->hw_events);
> > > > +
> > > > +     if (!bitmap_empty(cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS)) {
> > > > +             int i;
> > > > +             /* Don't need to clear assigned counters. */
> > > > +             bitmap_xor(cpuc->dirty_mask, cpuc->dirty_mask, cpuc->used_mask, ARMPMU_MAX_HWEVENTS);
> > > > +
> > > > +             for_each_set_bit(i, cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS) {
> > > > +                     if (i == ARMV8_IDX_CYCLE_COUNTER)
> > > > +                             write_sysreg(0, pmccntr_el0);
> > > > +                     else
> > > > +                             armv8pmu_write_evcntr(i, 0);
> > > > +             }
> > > > +             bitmap_zero(cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS);
> > > > +     }
> > > > +
> > > > +     write_sysreg(ARMV8_PMU_USERENR_ER | ARMV8_PMU_USERENR_CR, pmuserenr_el0);
> > > > +}
> > >
> > > This still leaks the values of CPU-bound events, or task-bound events
> > > owned by others, right?
> >
> > For CPU-bound events, yes. There's not any way to prevent that without
> > per counter access controls.
> >
> > It is clearing other's task-bound events.
>
> Sorry, I misspoke. When I said "task-bound events owned by others", I
> had meant events bounds to this task, but owned by someone else (e.g.
> the system administrator).

Ah yeah, those would still be exposed, but that would only tell the
task someone is watching them. Though I guess the task could write
those counters and corrupt the data.

> Thanks for confirming!
>
> > > > +static void armv8pmu_event_mapped(struct perf_event *event, struct mm_struct *mm)
> > > > +{
> > > > +     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR) || (atomic_read(&event->mmap_count) != 1))
> > > > +             return;
> > > > +
> > > > +     if (atomic_inc_return(&event->ctx->nr_user) == 1) {
> > > > +             unsigned long flags;
> > > > +             atomic_inc(&event->pmu->sched_cb_usage);
> > > > +             local_irq_save(flags);
> > > > +             armv8pmu_enable_user_access(to_arm_pmu(event->pmu));
> > > > +             local_irq_restore(flags);
> > > > +     }
> > > > +}
> > > > +
> > > > +static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
> > > > +{
> > > > +     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR) || (atomic_read(&event->mmap_count) != 1))
> > > > +             return;
> > > > +
> > > > +     if (atomic_dec_and_test(&event->ctx->nr_user)) {
> > > > +             atomic_dec(&event->pmu->sched_cb_usage);
> > > > +             armv8pmu_disable_user_access();
> > > > +     }
> > > >  }
> > >
> > > We can open an event for task A, but call mmap()/munmap() for that event
> > > from task B, which will do the enable/disable on task B rather than task
> > > A. The core doesn't enforce that the mmap is performed on the same core,
> > > so I don't think this is quite right, unfortunately.
> >
> > Why do we care and who wants to do that? It all seems like a
> > convoluted scenario that isn't really going to happen. I prefer to not
> > support that until someone asks for it. Maybe we should check for the
> > condition (event->ctx->task != current) though.
>
> My reason for caring is that it means our accounting structures aren't
> aligned with the actual CPU state, and it's going to be very easy for
> this to go wrong as things get reworked in future.
>
> If we're saying someone shouldn't do this, then we should enforce that
> they can't. If they can, then I'd prefer to have deterministic behaviour
> that isn't going to cause us issues in future.
>
> I think that we should treat this like changing other event properties
> (e.g. the period, or enabling/disabling the event), with an
> event_function_call, since that will do the right thing regardless. I
> also expect that people will want to do setup/teardown in a single
> thread, and this would make that work too.

Looks like that will take some work in the core to get
event_function_call to be called in this case.

A much more simple solution is simply not enabling/disabling user
access on mmap/unmap. That was added along the way for x86, but I
don't think that buys us much given the departure from x86
implementation. We already have to enable the feature via sysctl (as
Will wants it default off) and the open has to request user access. If
a task succeeded in opening the event, is there any scenario where it
can't mmap its event? If so, could that condition not also be checked
in open?

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 3/5] arm64: perf: Enable PMU counter userspace access for perf event

[Rob Herring]

On Tue, Jun 1, 2021 at 11:12 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Tue, Jun 01, 2021 at 10:00:53AM -0500, Rob Herring wrote:
> > On Tue, Jun 1, 2021 at 8:55 AM Mark Rutland <mark.rutland@arm.com> wrote:
> > > On Mon, May 17, 2021 at 02:54:03PM -0500, Rob Herring wrote:
> > > > +static void armv8pmu_enable_user_access(struct arm_pmu *cpu_pmu)
> > > > +{
> > > > +     struct pmu_hw_events *cpuc = this_cpu_ptr(cpu_pmu->hw_events);
> > > > +
> > > > +     if (!bitmap_empty(cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS)) {
> > > > +             int i;
> > > > +             /* Don't need to clear assigned counters. */
> > > > +             bitmap_xor(cpuc->dirty_mask, cpuc->dirty_mask, cpuc->used_mask, ARMPMU_MAX_HWEVENTS);
> > > > +
> > > > +             for_each_set_bit(i, cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS) {
> > > > +                     if (i == ARMV8_IDX_CYCLE_COUNTER)
> > > > +                             write_sysreg(0, pmccntr_el0);
> > > > +                     else
> > > > +                             armv8pmu_write_evcntr(i, 0);
> > > > +             }
> > > > +             bitmap_zero(cpuc->dirty_mask, ARMPMU_MAX_HWEVENTS);
> > > > +     }
> > > > +
> > > > +     write_sysreg(ARMV8_PMU_USERENR_ER | ARMV8_PMU_USERENR_CR, pmuserenr_el0);
> > > > +}
> > >
> > > This still leaks the values of CPU-bound events, or task-bound events
> > > owned by others, right?
> >
> > For CPU-bound events, yes. There's not any way to prevent that without
> > per counter access controls.
> >
> > It is clearing other's task-bound events.
>
> Sorry, I misspoke. When I said "task-bound events owned by others", I
> had meant events bounds to this task, but owned by someone else (e.g.
> the system administrator).
>
> Thanks for confirming!
>
> > > > +static void armv8pmu_event_mapped(struct perf_event *event, struct mm_struct *mm)
> > > > +{
> > > > +     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR) || (atomic_read(&event->mmap_count) != 1))
> > > > +             return;
> > > > +
> > > > +     if (atomic_inc_return(&event->ctx->nr_user) == 1) {
> > > > +             unsigned long flags;
> > > > +             atomic_inc(&event->pmu->sched_cb_usage);
> > > > +             local_irq_save(flags);
> > > > +             armv8pmu_enable_user_access(to_arm_pmu(event->pmu));
> > > > +             local_irq_restore(flags);
> > > > +     }
> > > > +}
> > > > +
> > > > +static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
> > > > +{
> > > > +     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR) || (atomic_read(&event->mmap_count) != 1))
> > > > +             return;
> > > > +
> > > > +     if (atomic_dec_and_test(&event->ctx->nr_user)) {
> > > > +             atomic_dec(&event->pmu->sched_cb_usage);
> > > > +             armv8pmu_disable_user_access();
> > > > +     }
> > > >  }
> > >
> > > We can open an event for task A, but call mmap()/munmap() for that event
> > > from task B, which will do the enable/disable on task B rather than task
> > > A. The core doesn't enforce that the mmap is performed on the same core,
> > > so I don't think this is quite right, unfortunately.
> >
> > Why do we care and who wants to do that? It all seems like a
> > convoluted scenario that isn't really going to happen. I prefer to not
> > support that until someone asks for it. Maybe we should check for the
> > condition (event->ctx->task != current) though.
>
> My reason for caring is that it means our accounting structures aren't
> aligned with the actual CPU state, and it's going to be very easy for
> this to go wrong as things get reworked in future.
>
> If we're saying someone shouldn't do this, then we should enforce that
> they can't. If they can, then I'd prefer to have deterministic behaviour
> that isn't going to cause us issues in future.
>
> I think that we should treat this like changing other event properties
> (e.g. the period, or enabling/disabling the event), with an
> event_function_call, since that will do the right thing regardless. I
> also expect that people will want to do setup/teardown in a single
> thread, and this would make that work too.

The big difference here is we're dealing with task context state, not
just event state.

> I reckon we can get the core logic for that in kernel/events/core.c.

I assume you don't mean copy it out of there, but add it there. So
we'd need an event_function_call on mmap/munmap and a new PMU callback
as the PMU mmap callbacks are defined to be in context of the mmap,
not the task context. And then we'd need to not do that on x86.

Or we could change the definition of event_mapped/event_unmapped to be
called on every active context. After all, that is what x86 needs in
the end. Though I'm sure I don't know all the implications of such a
change.

> > > I reckon we need to do something with task_function_call() to make this
> > > happen in the context of the expected task.
> >
> > Following the x86 way using the mm_context and IPI handled that case,
> > but Will didn't like either part of that implementation. His
> > suggestion then was to enable access in an undef instr handler rather
> > than an IPI. I think that would still work with this version.
>
> IIUC the main objection there was that we'd have to signal all tasks
> associated with the mm or all CPUs, neither of which were attractive.

Yes, and the undef handler avoided that.

If we go back to utilizing the undef handler, I think we could make
things a bit more secure where the counter data is not exposed when
the user event is not scheduled.
This would enable or disable user access when a user event is
scheduled on the PMU or not. The undef handler will then return 0 on
counter reads if the context has any user event or abort otherwise.
The check of the context events will need to walk the event list which
I think? should be safe to do from the undef handler.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel