libperf and arm64 userspace counter access support

From: Raphael Gault <[email protected]>

In order to be able to access the counter directly for userspace, we need to provide the index of the counter using the userpage. We thus need to override the event_idx function to retrieve and convert the perf_event index to armv8 hardware index.

Since the arm_pmu driver can be used by any implementation, even if not armv8, two components play a role into making sure the behaviour is correct and consistent with the PMU capabilities:

* the ARMPMU_EL0_RD_CNTR flag which denotes the capability to access counter from userspace.
* the event_idx call back, which is implemented and initialized by the PMU implementation: if no callback is provided, the default behaviour applies, returning 0 as index value.

Signed-off-by Raphael Gault <[email protected]>

Signed-off-by Rob Herring <[email protected]>

---
 arch/arm64/kernel/perf_event.c | 18 ++++++++++++++++++
 include/linux/perf/arm_pmu.h   |  2 ++
 2 files changed, 20 insertions(+)

In order to enable 64-bit counters for userspace when available, a new config1 bit is added for userspace to indicate it wants userspace counter access. This bit allows the kernel to decide if chaining should be disabled and chaining and userspace access are incompatible. The modes for config1 are as follows:

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

From: Raphael Gault <[email protected]>

Keep track of event opened with direct access to the hardware counters and modify permissions while they are open.

The strategy used here is the same which x86 uses: every time an event is mapped, the permissions are set if required. The atomic field added in the mm_context helps keep track of the different event opened and de-activate the permissions when all are unmapped.
We also need to update the permissions in the context switch code so that tasks keep the right permissions.

In order to enable 64-bit counters for userspace when available, a new config1 bit is added for userspace to indicate it wants userspace counter access. This bit allows the kernel to decide if chaining should be disabled and chaining and userspace access are incompatible. The modes for config1 are as follows:

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

User access is enabled with config1 == 0 so that we match x86 behavior and don't need Arm specific code (outside the counter read).

Signed-off-by Raphael Gault <[email protected]>

Signed-off-by Rob Herring <[email protected]>

---
I'm not completely sure if using current->active_mm in an IPI is okay? It seems to work in my testing.

Peter Z says (event->oncpu == smp_processor_id()) in the user page update is always true, but my testing says otherwise[1].

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/

user fix

---
 arch/arm64/include/asm/mmu.h         |  5 ++
 arch/arm64/include/asm/mmu_context.h |  2 +
 arch/arm64/include/asm/perf_event.h  | 14 +++++
 arch/arm64/kernel/perf_event.c       | 86 +++++++++++++++++++++++++++-
 4 files changed, 104 insertions(+), 3 deletions(-)

arch/arm64/include/asm/mmu.h +5 -0

index 75beffe2ee8a..ee08447455da 100644
@@ -18,6 +18,11 @@
 
 typedef struct {
 	atomic64_t	id;
+	/*
+	 * non-zero if userspace have access to hardware
+	 * counters directly.
+	 */
+	atomic_t	pmu_direct_access;
 #ifdef CONFIG_COMPAT
 	void		*sigpage;
 #endif

arch/arm64/include/asm/mmu_context.h +2 -0

index 70ce8c1d2b07..ccb5ff417b42 100644
@@ -21,6 +21,7 @@
 #include <asm/proc-fns.h>
 #include <asm-generic/mm_hooks.h>
 #include <asm/cputype.h>
+#include <asm/perf_event.h>
 #include <asm/sysreg.h>
 #include <asm/tlbflush.h>
 

@@ -230,6 +231,7 @@ static inline void __switch_mm(struct mm_struct *next)
 	}
 
 	check_and_switch_context(next);
+	perf_switch_user_access(next);
 }
 
 static inline void

arch/arm64/include/asm/perf_event.h +1 -0

index 60731f602d3e..112f3f63b79e 100644
@@ -8,6 +8,7 @@
 
 #include <asm/stack_pointer.h>
 #include <asm/ptrace.h>
+#include <linux/mm_types.h>
 
 #define	ARMV8_PMU_MAX_COUNTERS	32
 #define	ARMV8_PMU_COUNTER_MASK	(ARMV8_PMU_MAX_COUNTERS - 1)

arch/arm64/include/asm/perf_event.h +9 -0

+
+	if (atomic_read(&mm->context.pmu_direct_access)) {
+		write_sysreg(ARMV8_PMU_USERENR_ER|ARMV8_PMU_USERENR_CR,
+			     pmuserenr_el0);
+	} else {
+		write_sysreg(0, pmuserenr_el0);
+	}
+}
+
 #endif

arch/arm64/kernel/perf_event.c +11 -0

index 387838496955..9ad3cc523ef4 100644
@@ -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,
 };
 

@@ -356,6 +363,7 @@ static inline bool armv8pmu_event_is_chained(struct perf_event *event)
 	struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
 
 	return !WARN_ON(idx < 0) &&
+	       !armv8pmu_event_want_user_access(event) &&
 	       armv8pmu_event_is_64bit(event) &&
 	       !armv8pmu_has_long_event(cpu_pmu) &&
 	       (idx != ARMV8_IDX_CYCLE_COUNTER);

@@ -849,13 +857,17 @@ 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))

arch/arm64/kernel/perf_event.c +18 -0

 		return	armv8pmu_get_chain_idx(cpuc, cpu_pmu);
 	else
 		return armv8pmu_get_single_idx(cpuc, cpu_pmu);

@@ -887,6 +899,46 @@ static int armv8pmu_access_event_idx(struct perf_event *event)
 	return event->hw.idx;
 }
 
+static void refresh_pmuserenr(void *mm)
+{
+	if (mm == current->active_mm)
+		perf_switch_user_access(mm);
+}
+
+static void armv8pmu_event_mapped(struct perf_event *event, struct mm_struct *mm)
+{
+	struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+
+	if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+		return;
+
+	/*
+	 * This function relies on not being called concurrently in two
+	 * tasks in the same mm.  Otherwise one task could observe
+	 * pmu_direct_access > 1 and return all the way back to
+	 * userspace with user access disabled while another task is still

arch/arm64/kernel/perf_event.c +15 -0

+		/*
+		 * 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_has_long_event(armpmu) ||
+		     hw_event_id == ARMV8_PMUV3_PERFCTR_CPU_CYCLES))
+			event->hw.flags |= ARMPMU_EVT_64BIT;
+	} else if (armv8pmu_event_is_64bit(event))
 		event->hw.flags |= ARMPMU_EVT_64BIT;
 
+
 	/* 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)) {

@@ -1115,6 +1181,8 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name,
 	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->name			= name;
 	cpu_pmu->map_event		= map_event;

@@ -1290,6 +1358,18 @@ void arch_perf_update_userpage(struct perf_event *event,
 	userpg->cap_user_time = 0;

Quoted 18 lines · Rob Herring

From: Raphael Gault <[email protected]>

Keep track of event opened with direct access to the hardware counters and modify permissions while they are open.

The strategy used here is the same which x86 uses: every time an event is mapped, the permissions are set if required. The atomic field added in the mm_context helps keep track of the different event opened and de-activate the permissions when all are unmapped.
We also need to update the permissions in the context switch code so that tasks keep the right permissions.

In order to enable 64-bit counters for userspace when available, a new config1 bit is added for userspace to indicate it wants userspace counter access. This bit allows the kernel to decide if chaining should be disabled and chaining and userspace access are incompatible. The modes for config1 are as follows:

Quoted 3 lines

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

In this last case, how does userspace know whether it got a 32-bit or a 64-bit counter?

Quoted 4 lines

User access is enabled with config1 == 0 so that we match x86 behavior and don't need Arm specific code (outside the counter read).

Quoted 8 lines

Signed-off-by Raphael Gault <[email protected]>

Signed-off-by Rob Herring <[email protected]>

---
I'm not completely sure if using current->active_mm in an IPI is okay? It seems to work in my testing.

Peter Z says (event->oncpu == smp_processor_id()) in the user page update is always true, but my testing says otherwise[1].

Peter? Sounds like there's either a misunderstanding here or we have some fundamental issue elsewhere.

Quoted 77 lines

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/

user fix

---
 arch/arm64/include/asm/mmu.h         |  5 ++
 arch/arm64/include/asm/mmu_context.h |  2 +
 arch/arm64/include/asm/perf_event.h  | 14 +++++
 arch/arm64/kernel/perf_event.c       | 86 +++++++++++++++++++++++++++-
 4 files changed, 104 insertions(+), 3 deletions(-)

arch/arm64/include/asm/mmu.h +5 -0

index 75beffe2ee8a..ee08447455da 100644
@@ -18,6 +18,11 @@
 
 typedef struct {
      atomic64_t      id;
+     /*
+      * non-zero if userspace have access to hardware
+      * counters directly.
+      */
+     atomic_t        pmu_direct_access;
 #ifdef CONFIG_COMPAT
      void            *sigpage;
 #endif

arch/arm64/include/asm/mmu_context.h +2 -0

index 70ce8c1d2b07..ccb5ff417b42 100644
@@ -21,6 +21,7 @@
 #include <asm/proc-fns.h>
 #include <asm-generic/mm_hooks.h>
 #include <asm/cputype.h>
+#include <asm/perf_event.h>
 #include <asm/sysreg.h>
 #include <asm/tlbflush.h>
 

@@ -230,6 +231,7 @@ static inline void __switch_mm(struct mm_struct *next)
      }
 
      check_and_switch_context(next);
+     perf_switch_user_access(next);
 }
 
 static inline void

arch/arm64/include/asm/perf_event.h +1 -0

index 60731f602d3e..112f3f63b79e 100644
@@ -8,6 +8,7 @@
 
 #include <asm/stack_pointer.h>
 #include <asm/ptrace.h>
+#include <linux/mm_types.h>
 
 #define      ARMV8_PMU_MAX_COUNTERS  32
 #define      ARMV8_PMU_COUNTER_MASK  (ARMV8_PMU_MAX_COUNTERS - 1)

arch/arm64/include/asm/perf_event.h extern unsigned long perf_misc_flags(struct pt_regs *regs); · line 261

Quoted 7 lines

arch/arm64/include/asm/perf_event.h +4 -0

@@ -254,4 +255,17 @@ extern unsigned long perf_misc_flags(struct pt_regs *regs);
      (regs)->pstate = PSR_MODE_EL1h; \
 }
 
+static inline void perf_switch_user_access(struct mm_struct *mm)
+{
+     if (!IS_ENABLED(CONFIG_PERF_EVENTS))
+             return;

CONFIG_HW_PERF_EVENTS might be a better fit here.

Quoted 52 lines

arch/arm64/include/asm/perf_event.h +9 -0

+
+     if (atomic_read(&mm->context.pmu_direct_access)) {
+             write_sysreg(ARMV8_PMU_USERENR_ER|ARMV8_PMU_USERENR_CR,
+                          pmuserenr_el0);
+     } else {
+             write_sysreg(0, pmuserenr_el0);
+     }
+}
+
 #endif

arch/arm64/kernel/perf_event.c +11 -0

index 387838496955..9ad3cc523ef4 100644
@@ -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,
 };
 

@@ -356,6 +363,7 @@ static inline bool armv8pmu_event_is_chained(struct perf_event *event)
      struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
 
      return !WARN_ON(idx < 0) &&
+            !armv8pmu_event_want_user_access(event) &&
             armv8pmu_event_is_64bit(event) &&
             !armv8pmu_has_long_event(cpu_pmu) &&
             (idx != ARMV8_IDX_CYCLE_COUNTER);

@@ -849,13 +857,17 @@ 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))

arch/arm64/kernel/perf_event.c static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc, · line 870

Quoted 10 lines

arch/arm64/kernel/perf_event.c +3 -2

+                             return -EAGAIN;
      }
 
      /*
       * Otherwise use events counters
       */
-     if (armv8pmu_event_is_64bit(event) &&
-         !armv8pmu_has_long_event(cpu_pmu))
+     if (armv8pmu_event_is_64bit(event) && !armv8pmu_has_long_event(cpu_pmu) &&
+         !armv8pmu_event_want_user_access(event))

This logic is duplicated in armv8pmu_event_is_chained(); can you split it into a helper, please?

Quoted 30 lines

arch/arm64/kernel/perf_event.c +23 -0

              return  armv8pmu_get_chain_idx(cpuc, cpu_pmu);
      else
              return armv8pmu_get_single_idx(cpuc, cpu_pmu);

@@ -887,6 +899,46 @@ static int armv8pmu_access_event_idx(struct perf_event *event)
      return event->hw.idx;
 }
 
+static void refresh_pmuserenr(void *mm)
+{
+     if (mm == current->active_mm)
+             perf_switch_user_access(mm);
+}
+
+static void armv8pmu_event_mapped(struct perf_event *event, struct mm_struct *mm)
+{
+     struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+
+     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+             return;
+
+     /*
+      * This function relies on not being called concurrently in two
+      * tasks in the same mm.  Otherwise one task could observe
+      * pmu_direct_access > 1 and return all the way back to
+      * userspace with user access disabled while another task is still
+      * doing on_each_cpu_mask() to enable user access.
+      *
+      * For now, this can't happen because all callers hold mmap_lock
+      * for write.  If this changes, we'll need a different solution.
+      */

arch/arm64/kernel/perf_event.c static int armv8pmu_access_event_idx(struct perf_event *event) · line 929

Quoted 5 lines

arch/arm64/kernel/perf_event.c +5 -0

+     lockdep_assert_held_write(&mm->mmap_lock);
+
+     if (atomic_inc_return(&mm->context.pmu_direct_access) == 1)
+             on_each_cpu_mask(&armpmu->supported_cpus, refresh_pmuserenr, mm, 1);
+}

Why do we need to cross-call here? Seems like it would be a tonne simpler to handle the trap. Is there a reason not to do that?

Hmm... this feels pretty fragile since, although we may expect userspace only to trigger this in the context of the specific perf use-case, we don't have a way to detect that, so the ABI we're exposing is that EL0 accesses to non-existent counters will return 0. I don't really think that's something we want to commit to.

Quoted 8 lines

arch/arm64/kernel/perf_event.c +8 -0

+
+static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
+{
+     struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+
+     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+             return;
+

arch/arm64/kernel/perf_event.c static int armv8pmu_access_event_idx(struct perf_event *event) · line 939

Quoted 2 lines

arch/arm64/kernel/perf_event.c +2 -0

+     if (atomic_dec_and_test(&mm->context.pmu_direct_access))
+             on_each_cpu_mask(&armpmu->supported_cpus, refresh_pmuserenr, mm, 1);

Given that the pmu_direct_access field is global per-mm, won't this go wrong if multiple PMUs are opened by the same process but only a subset are exposed to EL0? Perhaps pmu_direct_access should be treated as a mask rather than a counter, so that we can 'and' it with the supported_cpus for the PMU we're dealing with.

arch/arm64/kernel/perf_event.c static int __armv8_pmuv3_map_event(struct perf_event *event, · line 1036

Quoted 7 lines

arch/arm64/kernel/perf_event.c +2 -1

@@ -980,9 +1032,23 @@ 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))
+     if (armv8pmu_event_want_user_access(event) || !armv8pmu_event_is_64bit(event)) {
+             event->hw.flags |= ARMPMU_EL0_RD_CNTR;

Why do you set this for all 32-bit events?

The logic here feels like it
could with a bit of untangling.

Quoted 14 lines

arch/arm64/kernel/perf_event.c +12 -0

+             /*
+              * 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_has_long_event(armpmu) ||
+                  hw_event_id == ARMV8_PMUV3_PERFCTR_CPU_CYCLES))
+                     event->hw.flags |= ARMPMU_EVT_64BIT;
+     } else if (armv8pmu_event_is_64bit(event))
              event->hw.flags |= ARMPMU_EVT_64BIT;

Quoted 23 lines · Will Deacon

Quoted 21 lines · Rob Herring

From: Raphael Gault <[email protected]>

Keep track of event opened with direct access to the hardware counters and modify permissions while they are open.

The strategy used here is the same which x86 uses: every time an event is mapped, the permissions are set if required. The atomic field added in the mm_context helps keep track of the different event opened and de-activate the permissions when all are unmapped.
We also need to update the permissions in the context switch code so that tasks keep the right permissions.

In order to enable 64-bit counters for userspace when available, a new config1 bit is added for userspace to indicate it wants userspace counter access. This bit allows the kernel to decide if chaining should be disabled and chaining and userspace access are incompatible. The modes for config1 are as follows:

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

Quoted 9 lines

arch/arm64/kernel/perf_event.c static int __armv8_pmuv3_map_event(struct perf_event *event, · line 1036

Quoted 7 lines

arch/arm64/kernel/perf_event.c +2 -1

@@ -980,9 +1032,23 @@ 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))
+     if (armv8pmu_event_want_user_access(event) || !armv8pmu_event_is_64bit(event)) {
+             event->hw.flags |= ARMPMU_EL0_RD_CNTR;

Why do you set this for all 32-bit events?

It goes back to the config1 bits as explained in the commit msg. We can always support user access for 32-bit counters, but for 64-bit counters the user has to request both user access and 64-bit counters. We could require explicit user access request for 32-bit access, but I thought it was better to not require userspace to do something Arm specific on open.

Quoted 2 lines

The logic here feels like it
could with a bit of untangling.

Yes, I don't love it, but couldn't come up with anything better. It is complicated by the fact that flags have to be set before we assign the counter and can't set/change them when we assign the counter. It would take a lot of refactoring with armpmu code to fix that.

Quoted 13 lines

Quoted 13 lines

arch/arm64/kernel/perf_event.c +12 -0

+             /*
+              * 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_has_long_event(armpmu) ||
+                  hw_event_id == ARMV8_PMUV3_PERFCTR_CPU_CYCLES))
+                     event->hw.flags |= ARMPMU_EVT_64BIT;
+     } else if (armv8pmu_event_is_64bit(event))
              event->hw.flags |= ARMPMU_EVT_64BIT;

Quoted 8 lines · Rob Herring

Quoted 2 lines · Will Deacon

The logic here feels like it
could with a bit of untangling.

Yes, I don't love it, but couldn't come up with anything better. It is complicated by the fact that flags have to be set before we assign the counter and can't set/change them when we assign the counter. It would take a lot of refactoring with armpmu code to fix that.

How's this instead?:

if (armv8pmu_event_want_user_access(event) || !armv8pmu_event_is_64bit(event))

        event->hw.flags |= ARMPMU_EL0_RD_CNTR;

/*
* 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(cpu_pmu) || (hw_event_id ==
ARMV8_PMUV3_PERFCTR_CPU_CYCLES)))
        event->hw.flags |= ARMPMU_EVT_64BIT;

Quoted 23 lines · Rob Herring

Quoted 8 lines · Rob Herring

Quoted 2 lines · Will Deacon

The logic here feels like it
could with a bit of untangling.

Yes, I don't love it, but couldn't come up with anything better. It is complicated by the fact that flags have to be set before we assign the counter and can't set/change them when we assign the counter. It would take a lot of refactoring with armpmu code to fix that.

How's this instead?:

if (armv8pmu_event_want_user_access(event) || !armv8pmu_event_is_64bit(event))

        event->hw.flags |= ARMPMU_EL0_RD_CNTR;

/*
* 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) &&

Quoted 4 lines

    (!armv8pmu_event_want_user_access(event) ||
     armv8pmu_has_long_event(cpu_pmu) || (hw_event_id ==
ARMV8_PMUV3_PERFCTR_CPU_CYCLES)))
        event->hw.flags |= ARMPMU_EVT_64BIT;

I thought there were some cases where we could assign cycles event to an event counter; does that not happen anymore?

Quoted 30 lines · Will Deacon

Quoted 23 lines · Rob Herring

Quoted 8 lines · Rob Herring

Quoted 2 lines · Will Deacon

The logic here feels like it
could with a bit of untangling.

Yes, I don't love it, but couldn't come up with anything better. It is complicated by the fact that flags have to be set before we assign the counter and can't set/change them when we assign the counter. It would take a lot of refactoring with armpmu code to fix that.

How's this instead?:

if (armv8pmu_event_want_user_access(event) || !armv8pmu_event_is_64bit(event))

        event->hw.flags |= ARMPMU_EL0_RD_CNTR;

/*
* 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) &&

Quoted 4 lines

    (!armv8pmu_event_want_user_access(event) ||
     armv8pmu_has_long_event(cpu_pmu) || (hw_event_id ==
ARMV8_PMUV3_PERFCTR_CPU_CYCLES)))
        event->hw.flags |= ARMPMU_EVT_64BIT;

Quoted 2 lines

I thought there were some cases where we could assign cycles event to an event counter; does that not happen anymore?

Yes, but if we hit that scenario when the user has asked for 64-bit user access, then we return an error later when assigning the counter. I think we can assume if users have gone to the trouble of requesting 64-bit counters, then they can deal with ensuring they don't have multiple users.

Otherwise, the only way I see to simplify this is we only support 64-bit counters in userspace when we have v8.5 PMU.

Quoted 24 lines · Will Deacon

Quoted 21 lines · Rob Herring

From: Raphael Gault <[email protected]>

Keep track of event opened with direct access to the hardware counters and modify permissions while they are open.

The strategy used here is the same which x86 uses: every time an event is mapped, the permissions are set if required. The atomic field added in the mm_context helps keep track of the different event opened and de-activate the permissions when all are unmapped.
We also need to update the permissions in the context switch code so that tasks keep the right permissions.

In order to enable 64-bit counters for userspace when available, a new config1 bit is added for userspace to indicate it wants userspace counter access. This bit allows the kernel to decide if chaining should be disabled and chaining and userspace access are incompatible. The modes for config1 are as follows:

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

Quoted 2 lines

In this last case, how does userspace know whether it got a 32-bit or a 64-bit counter?

pmc_width in the user page. If the read loop is correct, then it doesn't matter to the user.

Quoted 23 lines

Quoted 23 lines

arch/arm64/kernel/perf_event.c +23 -0

+static void refresh_pmuserenr(void *mm)
+{
+     if (mm == current->active_mm)
+             perf_switch_user_access(mm);
+}
+
+static void armv8pmu_event_mapped(struct perf_event *event, struct mm_struct *mm)
+{
+     struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+
+     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+             return;
+
+     /*
+      * This function relies on not being called concurrently in two
+      * tasks in the same mm.  Otherwise one task could observe
+      * pmu_direct_access > 1 and return all the way back to
+      * userspace with user access disabled while another task is still
+      * doing on_each_cpu_mask() to enable user access.
+      *
+      * For now, this can't happen because all callers hold mmap_lock
+      * for write.  If this changes, we'll need a different solution.
+      */

Quoted 8 lines

arch/arm64/kernel/perf_event.c static int armv8pmu_access_event_idx(struct perf_event *event) · line 929

Quoted 5 lines

arch/arm64/kernel/perf_event.c +5 -0

+     lockdep_assert_held_write(&mm->mmap_lock);
+
+     if (atomic_inc_return(&mm->context.pmu_direct_access) == 1)
+             on_each_cpu_mask(&armpmu->supported_cpus, refresh_pmuserenr, mm, 1);
+}

Why do we need to cross-call here? Seems like it would be a tonne simpler to handle the trap. Is there a reason not to do that?

Alignment with x86? You didn't like the trap handler either:

Quoted 5 lines

Hmm... this feels pretty fragile since, although we may expect userspace only to trigger this in the context of the specific perf use-case, we don't have a way to detect that, so the ABI we're exposing is that EL0 accesses to non-existent counters will return 0. I don't really think that's something we want to commit to.

Full mail: https://lore.kernel.org/r/20200928182601.GA11974@willie-the-truck

The handler would be different, but we'd still have the problem of not being able to detect the use case, right?

Quoted 8 lines

Quoted 8 lines

arch/arm64/kernel/perf_event.c +8 -0

+
+static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
+{
+     struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+
+     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+             return;
+

Quoted 8 lines

arch/arm64/kernel/perf_event.c static int armv8pmu_access_event_idx(struct perf_event *event) · line 939

Quoted 2 lines

arch/arm64/kernel/perf_event.c +2 -0

+     if (atomic_dec_and_test(&mm->context.pmu_direct_access))
+             on_each_cpu_mask(&armpmu->supported_cpus, refresh_pmuserenr, mm, 1);

Given that the pmu_direct_access field is global per-mm, won't this go wrong if multiple PMUs are opened by the same process but only a subset are exposed to EL0? Perhaps pmu_direct_access should be treated as a mask rather than a counter, so that we can 'and' it with the supported_cpus for the PMU we're dealing with.

It needs to be a count to support multiple events on the same PMU. If the event is not enabled for EL0, then we'd exit out on the ARMPMU_EL0_RD_CNTR check. So I think we're fine.

Quoted 9 lines

arch/arm64/kernel/perf_event.c static int __armv8_pmuv3_map_event(struct perf_event *event, · line 1036

Quoted 7 lines

arch/arm64/kernel/perf_event.c +2 -1

@@ -980,9 +1032,23 @@ 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))
+     if (armv8pmu_event_want_user_access(event) || !armv8pmu_event_is_64bit(event)) {
+             event->hw.flags |= ARMPMU_EL0_RD_CNTR;

Why do you set this for all 32-bit events?

It goes back to the config1 bits as explained in the commit msg. We can always support user access for 32-bit counters, but for 64-bit counters the user has to request both user access and 64-bit counters. We could require explicit user access request for 32-bit access, but I thought it was better to not require userspace to do something Arm specific on open.

Quoted 25 lines · Rob Herring

Quoted 24 lines · Will Deacon

Quoted 21 lines · Rob Herring

From: Raphael Gault <[email protected]>

Keep track of event opened with direct access to the hardware counters and modify permissions while they are open.

The strategy used here is the same which x86 uses: every time an event is mapped, the permissions are set if required. The atomic field added in the mm_context helps keep track of the different event opened and de-activate the permissions when all are unmapped.
We also need to update the permissions in the context switch code so that tasks keep the right permissions.

In order to enable 64-bit counters for userspace when available, a new config1 bit is added for userspace to indicate it wants userspace counter access. This bit allows the kernel to decide if chaining should be disabled and chaining and userspace access are incompatible. The modes for config1 are as follows:

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

Quoted 5 lines

Quoted 2 lines

In this last case, how does userspace know whether it got a 32-bit or a 64-bit counter?

pmc_width in the user page. If the read loop is correct, then it doesn't matter to the user.

Gotcha; please mention that in this comment,

Quoted 40 lines

Quoted 23 lines

Quoted 23 lines

arch/arm64/kernel/perf_event.c +23 -0

+static void refresh_pmuserenr(void *mm)
+{
+     if (mm == current->active_mm)
+             perf_switch_user_access(mm);
+}
+
+static void armv8pmu_event_mapped(struct perf_event *event, struct mm_struct *mm)
+{
+     struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+
+     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+             return;
+
+     /*
+      * This function relies on not being called concurrently in two
+      * tasks in the same mm.  Otherwise one task could observe
+      * pmu_direct_access > 1 and return all the way back to
+      * userspace with user access disabled while another task is still
+      * doing on_each_cpu_mask() to enable user access.
+      *
+      * For now, this can't happen because all callers hold mmap_lock
+      * for write.  If this changes, we'll need a different solution.
+      */

Quoted 8 lines

arch/arm64/kernel/perf_event.c static int armv8pmu_access_event_idx(struct perf_event *event) · line 929

Quoted 5 lines

arch/arm64/kernel/perf_event.c +5 -0

+     lockdep_assert_held_write(&mm->mmap_lock);
+
+     if (atomic_inc_return(&mm->context.pmu_direct_access) == 1)
+             on_each_cpu_mask(&armpmu->supported_cpus, refresh_pmuserenr, mm, 1);
+}

Why do we need to cross-call here? Seems like it would be a tonne simpler to handle the trap. Is there a reason not to do that?

Alignment with x86? You didn't like the trap handler either:

Quoted 5 lines

Hmm... this feels pretty fragile since, although we may expect userspace only to trigger this in the context of the specific perf use-case, we don't have a way to detect that, so the ABI we're exposing is that EL0 accesses to non-existent counters will return 0. I don't really think that's something we want to commit to.

Quoted 4 lines

Full mail: https://lore.kernel.org/r/20200928182601.GA11974@willie-the-truck

The handler would be different, but we'd still have the problem of not being able to detect the use case, right?

Well hang on, the thing you link to was about _emulating_ the access. I'm not saying we should do that, but rather just enable EL0 PMU access lazily instead of using the IPI. We're going to enable it when we context-switch anyway.

The alternative is enabling EL0 access unconditionally and leaving it enabled.

Quoted 17 lines

Quoted 16 lines

arch/arm64/kernel/perf_event.c static int armv8pmu_access_event_idx(struct perf_event *event) · line 939

Quoted 10 lines

arch/arm64/kernel/perf_event.c +10 -0

+
+static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
+{
+     struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+
+     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+             return;
+
+     if (atomic_dec_and_test(&mm->context.pmu_direct_access))
+             on_each_cpu_mask(&armpmu->supported_cpus, refresh_pmuserenr, mm, 1);

Given that the pmu_direct_access field is global per-mm, won't this go wrong if multiple PMUs are opened by the same process but only a subset are exposed to EL0? Perhaps pmu_direct_access should be treated as a mask rather than a counter, so that we can 'and' it with the supported_cpus for the PMU we're dealing with.

Quoted 3 lines

It needs to be a count to support multiple events on the same PMU. If the event is not enabled for EL0, then we'd exit out on the ARMPMU_EL0_RD_CNTR check. So I think we're fine.

I'm still not convinced; pmu_direct_access is shared between PMUs, so testing the result of atomic_dec_and_test() just doesn't make sense to me, as another PMU could be playing with the count.

Quoted 80 lines · Will Deacon

Quoted 25 lines · Rob Herring

Quoted 24 lines · Will Deacon

Quoted 21 lines · Rob Herring

From: Raphael Gault <[email protected]>

Keep track of event opened with direct access to the hardware counters and modify permissions while they are open.

The strategy used here is the same which x86 uses: every time an event is mapped, the permissions are set if required. The atomic field added in the mm_context helps keep track of the different event opened and de-activate the permissions when all are unmapped.
We also need to update the permissions in the context switch code so that tasks keep the right permissions.

In order to enable 64-bit counters for userspace when available, a new config1 bit is added for userspace to indicate it wants userspace counter access. This bit allows the kernel to decide if chaining should be disabled and chaining and userspace access are incompatible. The modes for config1 are as follows:

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

Quoted 5 lines

Quoted 2 lines

In this last case, how does userspace know whether it got a 32-bit or a 64-bit counter?

pmc_width in the user page. If the read loop is correct, then it doesn't matter to the user.

Gotcha; please mention that in this comment,

Quoted 40 lines

Quoted 23 lines

Quoted 23 lines

arch/arm64/kernel/perf_event.c +23 -0

+static void refresh_pmuserenr(void *mm)
+{
+     if (mm == current->active_mm)
+             perf_switch_user_access(mm);
+}
+
+static void armv8pmu_event_mapped(struct perf_event *event, struct mm_struct *mm)
+{
+     struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+
+     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+             return;
+
+     /*
+      * This function relies on not being called concurrently in two
+      * tasks in the same mm.  Otherwise one task could observe
+      * pmu_direct_access > 1 and return all the way back to
+      * userspace with user access disabled while another task is still
+      * doing on_each_cpu_mask() to enable user access.
+      *
+      * For now, this can't happen because all callers hold mmap_lock
+      * for write.  If this changes, we'll need a different solution.
+      */

Quoted 8 lines

arch/arm64/kernel/perf_event.c static int armv8pmu_access_event_idx(struct perf_event *event) · line 929

Quoted 5 lines

arch/arm64/kernel/perf_event.c +5 -0

+     lockdep_assert_held_write(&mm->mmap_lock);
+
+     if (atomic_inc_return(&mm->context.pmu_direct_access) == 1)
+             on_each_cpu_mask(&armpmu->supported_cpus, refresh_pmuserenr, mm, 1);
+}

Why do we need to cross-call here? Seems like it would be a tonne simpler to handle the trap. Is there a reason not to do that?

Alignment with x86? You didn't like the trap handler either:

Quoted 5 lines

Hmm... this feels pretty fragile since, although we may expect userspace only to trigger this in the context of the specific perf use-case, we don't have a way to detect that, so the ABI we're exposing is that EL0 accesses to non-existent counters will return 0. I don't really think that's something we want to commit to.

Quoted 4 lines

Full mail: https://lore.kernel.org/r/20200928182601.GA11974@willie-the-truck

The handler would be different, but we'd still have the problem of not being able to detect the use case, right?

Quoted 7 lines

Well hang on, the thing you link to was about _emulating_ the access. I'm not saying we should do that, but rather just enable EL0 PMU access lazily instead of using the IPI. We're going to enable it when we context-switch anyway.

The alternative is enabling EL0 access unconditionally and leaving it enabled.

That BTW is what x86 originally did. This pmu_direct_access stuff came about as part of locking down access a bit.

In any case, I've modified this to trap. It avoids the mmap lock dependency too, so that's nice.

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

arch/arm64/kernel/perf_event.c static int armv8pmu_access_event_idx(struct perf_event *event) · line 939

Quoted 10 lines

arch/arm64/kernel/perf_event.c +10 -0

+
+static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
+{
+     struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+
+     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+             return;
+
+     if (atomic_dec_and_test(&mm->context.pmu_direct_access))
+             on_each_cpu_mask(&armpmu->supported_cpus, refresh_pmuserenr, mm, 1);

Given that the pmu_direct_access field is global per-mm, won't this go wrong if multiple PMUs are opened by the same process but only a subset are exposed to EL0? Perhaps pmu_direct_access should be treated as a mask rather than a counter, so that we can 'and' it with the supported_cpus for the PMU we're dealing with.

Quoted 7 lines

Quoted 3 lines

It needs to be a count to support multiple events on the same PMU. If the event is not enabled for EL0, then we'd exit out on the ARMPMU_EL0_RD_CNTR check. So I think we're fine.

I'm still not convinced; pmu_direct_access is shared between PMUs, so testing the result of atomic_dec_and_test() just doesn't make sense to me, as another PMU could be playing with the count.

How is that a problem? Let's make a concrete example:

map PMU1:event2 -> pmu_direct_access = 1 -> enable access map PMU2:event3 -> pmu_direct_access = 2
map PMU1:event4 -> pmu_direct_access = 3
unmap PMU2:event3 -> pmu_direct_access = 2
unmap PMU1:event2 -> pmu_direct_access = 1
unmap PMU1:event4 -> pmu_direct_access = 0 -> disable access

The only issue I can see is PMU2 remains enabled for user access until the last unmap. But we're sharing the mm, so who cares? Also, in this scenario it is the user's problem to pin themselves to cores sharing a PMU. If the user doesn't do that, they get to keep the pieces.

[Moving Mark to To: since I'd like his view on this]

Quoted 41 lines · Rob Herring

Quoted 23 lines · Will Deacon

Quoted 21 lines · Rob Herring

Quoted 19 lines · Will Deacon

arch/arm64/kernel/perf_event.c static int armv8pmu_access_event_idx(struct perf_event *event) · line 939

Quoted 11 lines · Rob Herring

From: Raphael Gault <[email protected]>

arch/arm64/kernel/perf_event.c +9 -0

+static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
+{
+     struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+
+     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+             return;
+
+     if (atomic_dec_and_test(&mm->context.pmu_direct_access))
+             on_each_cpu_mask(&armpmu->supported_cpus, refresh_pmuserenr, mm, 1);

Given that the pmu_direct_access field is global per-mm, won't this go wrong if multiple PMUs are opened by the same process but only a subset are exposed to EL0? Perhaps pmu_direct_access should be treated as a mask rather than a counter, so that we can 'and' it with the supported_cpus for the PMU we're dealing with.

Quoted 7 lines

Quoted 3 lines

It needs to be a count to support multiple events on the same PMU. If the event is not enabled for EL0, then we'd exit out on the ARMPMU_EL0_RD_CNTR check. So I think we're fine.

I'm still not convinced; pmu_direct_access is shared between PMUs, so testing the result of atomic_dec_and_test() just doesn't make sense to me, as another PMU could be playing with the count.

How is that a problem? Let's make a concrete example:

map PMU1:event2 -> pmu_direct_access = 1 -> enable access map PMU2:event3 -> pmu_direct_access = 2
map PMU1:event4 -> pmu_direct_access = 3
unmap PMU2:event3 -> pmu_direct_access = 2
unmap PMU1:event2 -> pmu_direct_access = 1
unmap PMU1:event4 -> pmu_direct_access = 0 -> disable access

Quoted 4 lines

The only issue I can see is PMU2 remains enabled for user access until the last unmap. But we're sharing the mm, so who cares? Also, in this scenario it is the user's problem to pin themselves to cores sharing a PMU. If the user doesn't do that, they get to keep the pieces.

I guess I'm just worried about exposing the counters to userspace after the PMU driver (or perf core?) thinks that they're no longer exposed in case we leak other events.

However, I'm not sure how this is supposed to work normally: what happens if e.g. a privileged user has a per-cpu counter for a kernel event while a task has a counter with direct access -- can that task read the kernel event out of the PMU registers from userspace?

Quoted 44 lines · Will Deacon

[Moving Mark to To: since I'd like his view on this]

Quoted 41 lines · Rob Herring

Quoted 23 lines · Will Deacon

Quoted 21 lines · Rob Herring

Quoted 19 lines · Will Deacon

Review

Quoted 11 lines · Rob Herring

From: Raphael Gault <[email protected]>

(quoted diff) +9 -0

+static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
+{
+     struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+
+     if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+             return;
+
+     if (atomic_dec_and_test(&mm->context.pmu_direct_access))
+             on_each_cpu_mask(&armpmu->supported_cpus, refresh_pmuserenr, mm, 1);

Given that the pmu_direct_access field is global per-mm, won't this go wrong if multiple PMUs are opened by the same process but only a subset are exposed to EL0? Perhaps pmu_direct_access should be treated as a mask rather than a counter, so that we can 'and' it with the supported_cpus for the PMU we're dealing with.

Quoted 7 lines

Quoted 3 lines

It needs to be a count to support multiple events on the same PMU. If the event is not enabled for EL0, then we'd exit out on the ARMPMU_EL0_RD_CNTR check. So I think we're fine.

I'm still not convinced; pmu_direct_access is shared between PMUs, so testing the result of atomic_dec_and_test() just doesn't make sense to me, as another PMU could be playing with the count.

How is that a problem? Let's make a concrete example:

map PMU1:event2 -> pmu_direct_access = 1 -> enable access map PMU2:event3 -> pmu_direct_access = 2
map PMU1:event4 -> pmu_direct_access = 3
unmap PMU2:event3 -> pmu_direct_access = 2
unmap PMU1:event2 -> pmu_direct_access = 1
unmap PMU1:event4 -> pmu_direct_access = 0 -> disable access

Quoted 8 lines

Quoted 4 lines

The only issue I can see is PMU2 remains enabled for user access until the last unmap. But we're sharing the mm, so who cares? Also, in this scenario it is the user's problem to pin themselves to cores sharing a PMU. If the user doesn't do that, they get to keep the pieces.

I guess I'm just worried about exposing the counters to userspace after the PMU driver (or perf core?) thinks that they're no longer exposed in case we leak other events.

IMO that's not practically different from the single-PMU case (i.e. multi-PMU isn't material, either we have a concern with leaking or we don't); more on that below.

While it looks odd to place this on the mm, I don't think it's the end of the world.

Quoted 4 lines

However, I'm not sure how this is supposed to work normally: what happens if e.g. a privileged user has a per-cpu counter for a kernel event while a task has a counter with direct access -- can that task read the kernel event out of the PMU registers from userspace?

Yes -- userspace could go read any counters even though it isn't supposed to, and could potentially infer information from those. It won't have access to the config registers or kernel data structures, so it isn't guaranteed to know what the even is or when it is context-switched/reprogrammed/etc.

If we believe that's a problem, then it's difficult to do anything robust other than denying userspace access entirely, since disabling userspace access while in use would surprise applications, and denying privileged events would need some global state that we consult at event creation time (in addition to being an inversion of privilege).

IIRC there was some fuss about this a while back on x86; I'll go dig and see what I can find, unless Peter has a memory...

Quoted 10 lines · Mark Rutland

Quoted 5 lines · Will Deacon

Quoted 1 line · Rob Herring

On Wed, Mar 31, 2021 at 11:01 AM Will Deacon <[email protected]> wrote:

I guess I'm just worried about exposing the counters to userspace after the PMU driver (or perf core?) thinks that they're no longer exposed in case we leak other events.

IMO that's not practically different from the single-PMU case (i.e. multi-PMU isn't material, either we have a concern with leaking or we don't); more on that below.

Quoted 20 lines

While it looks odd to place this on the mm, I don't think it's the end of the world.

Quoted 4 lines

However, I'm not sure how this is supposed to work normally: what happens if e.g. a privileged user has a per-cpu counter for a kernel event while a task has a counter with direct access -- can that task read the kernel event out of the PMU registers from userspace?

Yes -- userspace could go read any counters even though it isn't supposed to, and could potentially infer information from those. It won't have access to the config registers or kernel data structures, so it isn't guaranteed to know what the even is or when it is context-switched/reprogrammed/etc.

If we believe that's a problem, then it's difficult to do anything robust other than denying userspace access entirely, since disabling userspace access while in use would surprise applications, and denying privileged events would need some global state that we consult at event creation time (in addition to being an inversion of privilege).

Quoted 2 lines

IIRC there was some fuss about this a while back on x86; I'll go dig and see what I can find, unless Peter has a memory...

Maybe this one[1].

Rob

[1] https://lore.kernel.org/lkml/[email protected]/

Quoted 11 lines · Rob Herring

Quoted 10 lines · Mark Rutland

Quoted 5 lines · Will Deacon

Quoted 1 line · Rob Herring

On Wed, Mar 31, 2021 at 11:01 AM Will Deacon <[email protected]> wrote:

I guess I'm just worried about exposing the counters to userspace after the PMU driver (or perf core?) thinks that they're no longer exposed in case we leak other events.

IMO that's not practically different from the single-PMU case (i.e. multi-PMU isn't material, either we have a concern with leaking or we don't); more on that below.

Well, maybe. It looks the single-PMU case is exposed to the same issue, but I think a solution needs to take into account the multi-PMU situation.

Quoted 20 lines

Quoted 20 lines

While it looks odd to place this on the mm, I don't think it's the end of the world.

Quoted 4 lines

However, I'm not sure how this is supposed to work normally: what happens if e.g. a privileged user has a per-cpu counter for a kernel event while a task has a counter with direct access -- can that task read the kernel event out of the PMU registers from userspace?

Yes -- userspace could go read any counters even though it isn't supposed to, and could potentially infer information from those. It won't have access to the config registers or kernel data structures, so it isn't guaranteed to know what the even is or when it is context-switched/reprogrammed/etc.

If we believe that's a problem, then it's difficult to do anything robust other than denying userspace access entirely, since disabling userspace access while in use would surprise applications, and denying privileged events would need some global state that we consult at event creation time (in addition to being an inversion of privilege).

Quoted 8 lines

Quoted 2 lines

IIRC there was some fuss about this a while back on x86; I'll go dig and see what I can find, unless Peter has a memory...

Maybe this one[1].

Rob

[1] https://lore.kernel.org/lkml/[email protected]/

Going through the archives and talking to Peter, it looks like this is still an active area of concern:

- There are patches to clear "dirty" counters on context-switch. They were queued for 5.13 but broke -tip on Friday:

https://lore.kernel.org/lkml/YHm%[email protected]/

- Per-cpu events cannot be protected in software:

https://lore.kernel.org/lkml/CALCETrVVPzUd_hQ8xoomHn_wWRQJUvROeCt2do4_D4ROZoAVMg@mail.gmail.com/

so without hardware support, we need a way to disable user access for people that care about this leakage

x86 has an "rdpmc" file exposed for the PMU device in sysfs which allows access to be disabled. I don't think these patches add such a thing, and that's where the fun with multi-PMU machines would come into play.

tools/lib/perf/evsel.c +3 -0

+
 static int
 sys_perf_event_open(struct perf_event_attr *attr,
 		    pid_t pid, int cpu, int group_fd,

@@ -137,6 +147,8 @@ void perf_evsel__free_fd(struct perf_evsel *evsel)
 {
 	xyarray__delete(evsel->fd);
 	evsel->fd = NULL;
+	xyarray__delete(evsel->mmap);
+	evsel->mmap = NULL;
 }
 
 void perf_evsel__close(struct perf_evsel *evsel)

tools/lib/perf/evsel.c +24 -0

+
+	if (evsel->mmap == NULL &&
+	    perf_evsel__alloc_mmap(evsel, xyarray__max_x(evsel->fd), xyarray__max_y(evsel->fd)) < 0)
+		return -ENOMEM;
+
+	for (cpu = 0; cpu < xyarray__max_x(evsel->fd); cpu++) {
+		for (thread = 0; thread < xyarray__max_y(evsel->fd); thread++) {
+			int fd = FD(evsel, cpu, thread);
+			struct perf_mmap *map = MMAP(evsel, cpu, thread);
+
+			if (fd < 0)
+				continue;
+
+			perf_mmap__init(map, NULL, false, NULL);
+
+			ret = perf_mmap__mmap(map, &mp, fd, cpu);
+			if (ret)
+				return -1;
+		}
+	}
+
+	return 0;
+}
+

On Wed, Mar 10, 2021 at 05:08:31PM -0700, Rob Herring wrote:

SNIP

Quoted 17 lines

tools/lib/perf/evsel.c +3 -0

+
 static int
 sys_perf_event_open(struct perf_event_attr *attr,
                  pid_t pid, int cpu, int group_fd,

@@ -137,6 +147,8 @@ void perf_evsel__free_fd(struct perf_evsel *evsel)
 {
      xyarray__delete(evsel->fd);
      evsel->fd = NULL;
+     xyarray__delete(evsel->mmap);
+     evsel->mmap = NULL;
 }
 
 void perf_evsel__close(struct perf_evsel *evsel)

@@ -156,6 +168,45 @@ void perf_evsel__close_cpu(struct perf_evsel *evsel, int cpu)
      perf_evsel__close_fd_cpu(evsel, cpu);
 }
 

tools/lib/perf/evsel.c void perf_evsel__close_cpu(struct perf_evsel *evsel, int cpu) · line 177

Quoted 7 lines

tools/lib/perf/evsel.c +7 -0

+int perf_evsel__mmap(struct perf_evsel *evsel, int pages)
+{
+     int ret, cpu, thread;
+     struct perf_mmap_param mp = {
+             .prot = PROT_READ | PROT_WRITE,
+             .mask = (pages * page_size) - 1,
+     };

I don't mind using evsel->fd for dimensions below, but we need to check in here that it's defined, that perf_evsel__open was called

jirka

Quoted 24 lines

tools/lib/perf/evsel.c +24 -0

+
+     if (evsel->mmap == NULL &&
+         perf_evsel__alloc_mmap(evsel, xyarray__max_x(evsel->fd), xyarray__max_y(evsel->fd)) < 0)
+             return -ENOMEM;
+
+     for (cpu = 0; cpu < xyarray__max_x(evsel->fd); cpu++) {
+             for (thread = 0; thread < xyarray__max_y(evsel->fd); thread++) {
+                     int fd = FD(evsel, cpu, thread);
+                     struct perf_mmap *map = MMAP(evsel, cpu, thread);
+
+                     if (fd < 0)
+                             continue;
+
+                     perf_mmap__init(map, NULL, false, NULL);
+
+                     ret = perf_mmap__mmap(map, &mp, fd, cpu);
+                     if (ret)
+                             return -1;
+             }
+     }
+
+     return 0;
+}
+

SNIP

Quoted 22 lines · Jiri Olsa

On Wed, Mar 10, 2021 at 05:08:31PM -0700, Rob Herring wrote:

SNIP

Quoted 17 lines

tools/lib/perf/evsel.c +3 -0

+
 static int
 sys_perf_event_open(struct perf_event_attr *attr,
                  pid_t pid, int cpu, int group_fd,

@@ -137,6 +147,8 @@ void perf_evsel__free_fd(struct perf_evsel *evsel)
 {
      xyarray__delete(evsel->fd);
      evsel->fd = NULL;
+     xyarray__delete(evsel->mmap);
+     evsel->mmap = NULL;
 }
 
 void perf_evsel__close(struct perf_evsel *evsel)

@@ -156,6 +168,45 @@ void perf_evsel__close_cpu(struct perf_evsel *evsel, int cpu)
      perf_evsel__close_fd_cpu(evsel, cpu);
 }
 

Quoted 11 lines

tools/lib/perf/evsel.c void perf_evsel__close_cpu(struct perf_evsel *evsel, int cpu) · line 177

Quoted 7 lines

tools/lib/perf/evsel.c +7 -0

+int perf_evsel__mmap(struct perf_evsel *evsel, int pages)
+{
+     int ret, cpu, thread;
+     struct perf_mmap_param mp = {
+             .prot = PROT_READ | PROT_WRITE,
+             .mask = (pages * page_size) - 1,
+     };

I don't mind using evsel->fd for dimensions below, but we need to check in here that it's defined, that perf_evsel__open was called

Right, so I'll add this here:

if (evsel->fd == NULL)

    return -EINVAL;

Note that struct evsel has dimensions in it, but they are only set in the evlist code. I couldn't tell if that was by design or mistake.

BTW, I just noticed perf_evsel__open is leaking memory on most of its error paths.

Quoted 31 lines

jirka

Quoted 24 lines

tools/lib/perf/evsel.c +24 -0

+
+     if (evsel->mmap == NULL &&
+         perf_evsel__alloc_mmap(evsel, xyarray__max_x(evsel->fd), xyarray__max_y(evsel->fd)) < 0)
+             return -ENOMEM;
+
+     for (cpu = 0; cpu < xyarray__max_x(evsel->fd); cpu++) {
+             for (thread = 0; thread < xyarray__max_y(evsel->fd); thread++) {
+                     int fd = FD(evsel, cpu, thread);
+                     struct perf_mmap *map = MMAP(evsel, cpu, thread);
+
+                     if (fd < 0)
+                             continue;
+
+                     perf_mmap__init(map, NULL, false, NULL);
+
+                     ret = perf_mmap__mmap(map, &mp, fd, cpu);
+                     if (ret)
+                             return -1;
+             }
+     }
+
+     return 0;
+}
+

SNIP

Quoted 35 lines · Rob Herring

Quoted 33 lines · Jiri Olsa

On Wed, Mar 10, 2021 at 05:08:31PM -0700, Rob Herring wrote:

SNIP

Quoted 14 lines

tools/lib/perf/evsel.c +3 -0

+
 static int
 sys_perf_event_open(struct perf_event_attr *attr,
                  pid_t pid, int cpu, int group_fd,

@@ -137,6 +147,8 @@ void perf_evsel__free_fd(struct perf_evsel *evsel)
 {
      xyarray__delete(evsel->fd);
      evsel->fd = NULL;
+     xyarray__delete(evsel->mmap);
+     evsel->mmap = NULL;
 }
 
 void perf_evsel__close(struct perf_evsel *evsel)

tools/lib/perf/evsel.c void perf_evsel__close_cpu(struct perf_evsel *evsel, int cpu) · line 177

Quoted 10 lines

tools/lib/perf/evsel.c +7 -0

@@ -156,6 +168,45 @@ void perf_evsel__close_cpu(struct perf_evsel *evsel, int cpu)
      perf_evsel__close_fd_cpu(evsel, cpu);
 }
 
+int perf_evsel__mmap(struct perf_evsel *evsel, int pages)
+{
+     int ret, cpu, thread;
+     struct perf_mmap_param mp = {
+             .prot = PROT_READ | PROT_WRITE,
+             .mask = (pages * page_size) - 1,
+     };

I don't mind using evsel->fd for dimensions below, but we need to check in here that it's defined, that perf_evsel__open was called

Quoted 4 lines

Right, so I'll add this here:

if (evsel->fd == NULL)

    return -EINVAL;

Quoted 2 lines

Note that struct evsel has dimensions in it, but they are only set in the evlist code. I couldn't tell if that was by design or mistake.

we do? we have the cpus and threads in perf_evsel no?

also you'd need perf_evsel not evsel, right?

Quoted 3 lines

BTW, I just noticed perf_evsel__open is leaking memory on most of its error paths.

nice.. let's fix it ;-)

x86 and arm64 can both support direct access of event counters in userspace. The access sequence is less than trivial and currently exists in perf test code (tools/perf/arch/x86/tests/rdpmc.c) with copies in projects such as PAPI and libpfm4.

In order to support usersapce access, an event must be mmapped first with perf_evsel__mmap(). Then subsequent calls to perf_evsel__read() will use the fast path (assuming the arch supports it).

Signed-off-by Rob Herring <[email protected]>

---
v6:
- Adapt to mmap changes adding MMAP NULL check
v5:
- Make raw count s64 instead of u64 so that counter width shifting works
- Adapt to mmap changes
v4:
- Update perf_evsel__mmap size to pages
v3:
- Split out perf_evsel__mmap() to separate patch

---
 tools/lib/perf/evsel.c                 |  4 ++
 tools/lib/perf/include/internal/mmap.h |  3 +
 tools/lib/perf/mmap.c                  | 88 ++++++++++++++++++++++++++
 tools/lib/perf/tests/test-evsel.c      | 65 +++++++++++++++++++
 4 files changed, 160 insertions(+)

tools/lib/perf/evsel.c +4 -0

index 1057e9b15528..4d67343d36c9 100644
@@ -242,6 +242,10 @@ int perf_evsel__read(struct perf_evsel *evsel, int cpu, int thread,
        if (FD(evsel, cpu, thread) < 0)
                return -EINVAL;
 
+       if (MMAP(evsel, cpu, thread) &&
+           !perf_mmap__read_self(MMAP(evsel, cpu, thread), count))
+               return 0;
+
        if (readn(FD(evsel, cpu, thread), count->values, size) <= 0)
                return -errno;
 

tools/lib/perf/include/internal/mmap.h +3 -0

index be7556e0a2b2..5e3422f40ed5 100644
@@ -11,6 +11,7 @@
 #define PERF_SAMPLE_MAX_SIZE (1 << 16)
 
 struct perf_mmap;
+struct perf_counts_values;
 
 typedef void (*libperf_unmap_cb_t)(struct perf_mmap *map);
 

@@ -52,4 +53,6 @@ void perf_mmap__put(struct perf_mmap *map);
 
 u64 perf_mmap__read_head(struct perf_mmap *map);
 
+int perf_mmap__read_self(struct perf_mmap *map, struct perf_counts_values *count);
+
 #endif /* __LIBPERF_INTERNAL_MMAP_H */

tools/lib/perf/mmap.c +42 -0

index 79d5ed6c38cc..915469f00cf4 100644
@@ -8,9 +8,11 @@
 #include <linux/perf_event.h>
 #include <perf/mmap.h>
 #include <perf/event.h>
+#include <perf/evsel.h>
 #include <internal/mmap.h>
 #include <internal/lib.h>
 #include <linux/kernel.h>
+#include <linux/math64.h>
 #include "internal.h"
 
 void perf_mmap__init(struct perf_mmap *map, struct perf_mmap *prev,

@@ -273,3 +275,89 @@ union perf_event *perf_mmap__read_event(struct perf_mmap *map)
 
        return event;
 }
+
+#if defined(__i386__) || defined(__x86_64__)
+static u64 read_perf_counter(unsigned int counter)
+{
+       unsigned int low, high;
+
+       asm volatile("rdpmc" : "=a" (low), "=d" (high) : "c" (counter));
+
+       return low | ((u64)high) << 32;
+}
+
+static u64 read_timestamp(void)
+{
+       unsigned int low, high;
+
+       asm volatile("rdtsc" : "=a" (low), "=d" (high));
+
+       return low | ((u64)high) << 32;
+}
+#else
+static u64 read_perf_counter(unsigned int counter) { return 0; }
+static u64 read_timestamp(void) { return 0; }
+#endif
+
+int perf_mmap__read_self(struct perf_mmap *map, struct perf_counts_values *count)
+{
+       struct perf_event_mmap_page *pc = map->base;
+       u32 seq, idx, time_mult = 0, time_shift = 0;
+       u64 cnt, cyc = 0, time_offset = 0, time_cycles = 0, time_mask = ~0ULL;
+
+       if (!pc || !pc->cap_user_rdpmc)
+               return -1;
+
+       do {
+               seq = READ_ONCE(pc->lock);
+               barrier();
+
+               count->ena = READ_ONCE(pc->time_enabled);
+               count->run = READ_ONCE(pc->time_running);
+

tools/lib/perf/mmap.c +20 -0

+               }
+
+               idx = READ_ONCE(pc->index);
+               cnt = READ_ONCE(pc->offset);
+               if (pc->cap_user_rdpmc && idx) {
+                       s64 evcnt = read_perf_counter(idx - 1);
+                       u16 width = READ_ONCE(pc->pmc_width);
+
+                       evcnt <<= 64 - width;
+                       evcnt >>= 64 - width;
+                       cnt += evcnt;
+               } else
+                       return -1;
+
+               barrier();
+       } while (READ_ONCE(pc->lock) != seq);
+
+       if (count->ena != count->run) {
+               u64 delta;
+

tools/lib/perf/mmap.c +6 -0

+       }
+
+       count->val = cnt;
+
+       return 0;
+}

tools/lib/perf/tests/test-evsel.c +0 -0

index 0ad82d7a2a51..54fb4809b9ee 100644
@@ -120,6 +120,69 @@ static int test_stat_thread_enable(void)
        return 0;
 }
 

tools/lib/perf/tests/test-evsel.c +55 -1

+       int err, i;
+
+       threads = perf_thread_map__new_dummy();
+       __T("failed to create threads", threads);
+
+       perf_thread_map__set_pid(threads, 0, 0);
+
+       evsel = perf_evsel__new(&attr);
+       __T("failed to create evsel", evsel);
+
+       err = perf_evsel__open(evsel, NULL, threads);
+       __T("failed to open evsel", err == 0);
+
+       err = perf_evsel__mmap(evsel, 0);
+       __T("failed to mmap evsel", err == 0);
+
+       pc = perf_evsel__mmap_base(evsel, 0, 0);
+
+#if defined(__i386__) || defined(__x86_64__) || defined(__aarch64__)
+       __T("userspace counter access not supported", pc->cap_user_rdpmc);
+       __T("userspace counter access not enabled", pc->index);
+       __T("userspace counter width not set", pc->pmc_width >= 32);
+#endif
+
+       perf_evsel__read(evsel, 0, 0, &counts);
+       __T("failed to read value for evsel", counts.val != 0);
+
+       for (i = 0; i < 5; i++) {
+               volatile int count = 0x10000 << i;
+               __u64 start, end, last = 0;
+
+               __T_VERBOSE("\tloop = %u, ", count);
+
+               perf_evsel__read(evsel, 0, 0, &counts);
+               start = counts.val;
+
+               while (count--) ;
+
+               perf_evsel__read(evsel, 0, 0, &counts);
+               end = counts.val;
+
+               __T("invalid counter data", (end - start) > last);
+               last = end - start;
+               __T_VERBOSE("count = %llu\n", end - start);
+       }
+
+       perf_evsel__close(evsel);
+       perf_evsel__delete(evsel);
+
+       perf_thread_map__put(threads);
+       return 0;
+}
+
 int main(int argc, char **argv)
 {
        __T_START;

@@ -129,6 +192,8 @@ int main(int argc, char **argv)
        test_stat_cpu();
        test_stat_thread();
        test_stat_thread_enable();
+       test_stat_user_read(PERF_COUNT_HW_INSTRUCTIONS);
+       test_stat_user_read(PERF_COUNT_HW_CPU_CYCLES);
 
        __T_END;
        return tests_failed == 0 ? 0 : -1;
--

2.27.0

Quoted 127 lines · Rob Herring

x86 and arm64 can both support direct access of event counters in userspace. The access sequence is less than trivial and currently exists in perf test code (tools/perf/arch/x86/tests/rdpmc.c) with copies in projects such as PAPI and libpfm4.

In order to support usersapce access, an event must be mmapped first with perf_evsel__mmap(). Then subsequent calls to perf_evsel__read() will use the fast path (assuming the arch supports it).

Signed-off-by Rob Herring <[email protected]>

---
v6:
- Adapt to mmap changes adding MMAP NULL check
v5:
- Make raw count s64 instead of u64 so that counter width shifting works
- Adapt to mmap changes
v4:
- Update perf_evsel__mmap size to pages
v3:
- Split out perf_evsel__mmap() to separate patch

---
 tools/lib/perf/evsel.c                 |  4 ++
 tools/lib/perf/include/internal/mmap.h |  3 +
 tools/lib/perf/mmap.c                  | 88 ++++++++++++++++++++++++++
 tools/lib/perf/tests/test-evsel.c      | 65 +++++++++++++++++++
 4 files changed, 160 insertions(+)

tools/lib/perf/evsel.c +4 -0

index 1057e9b15528..4d67343d36c9 100644
@@ -242,6 +242,10 @@ int perf_evsel__read(struct perf_evsel *evsel, int cpu, int thread,
        if (FD(evsel, cpu, thread) < 0)
                return -EINVAL;
 
+       if (MMAP(evsel, cpu, thread) &&
+           !perf_mmap__read_self(MMAP(evsel, cpu, thread), count))
+               return 0;
+
        if (readn(FD(evsel, cpu, thread), count->values, size) <= 0)
                return -errno;
 

tools/lib/perf/include/internal/mmap.h +3 -0

index be7556e0a2b2..5e3422f40ed5 100644
@@ -11,6 +11,7 @@
 #define PERF_SAMPLE_MAX_SIZE (1 << 16)
 
 struct perf_mmap;
+struct perf_counts_values;
 
 typedef void (*libperf_unmap_cb_t)(struct perf_mmap *map);
 

@@ -52,4 +53,6 @@ void perf_mmap__put(struct perf_mmap *map);
 
 u64 perf_mmap__read_head(struct perf_mmap *map);
 
+int perf_mmap__read_self(struct perf_mmap *map, struct perf_counts_values *count);
+
 #endif /* __LIBPERF_INTERNAL_MMAP_H */

tools/lib/perf/mmap.c +46 -0

index 79d5ed6c38cc..915469f00cf4 100644
@@ -8,9 +8,11 @@
 #include <linux/perf_event.h>
 #include <perf/mmap.h>
 #include <perf/event.h>
+#include <perf/evsel.h>
 #include <internal/mmap.h>
 #include <internal/lib.h>
 #include <linux/kernel.h>
+#include <linux/math64.h>
 #include "internal.h"
 
 void perf_mmap__init(struct perf_mmap *map, struct perf_mmap *prev,

@@ -273,3 +275,89 @@ union perf_event *perf_mmap__read_event(struct perf_mmap *map)
 
        return event;
 }
+
+#if defined(__i386__) || defined(__x86_64__)
+static u64 read_perf_counter(unsigned int counter)
+{
+       unsigned int low, high;
+
+       asm volatile("rdpmc" : "=a" (low), "=d" (high) : "c" (counter));
+
+       return low | ((u64)high) << 32;
+}
+
+static u64 read_timestamp(void)
+{
+       unsigned int low, high;
+
+       asm volatile("rdtsc" : "=a" (low), "=d" (high));
+
+       return low | ((u64)high) << 32;
+}
+#else
+static u64 read_perf_counter(unsigned int counter) { return 0; }
+static u64 read_timestamp(void) { return 0; }
+#endif
+
+int perf_mmap__read_self(struct perf_mmap *map, struct perf_counts_values *count)
+{
+       struct perf_event_mmap_page *pc = map->base;
+       u32 seq, idx, time_mult = 0, time_shift = 0;
+       u64 cnt, cyc = 0, time_offset = 0, time_cycles = 0, time_mask = ~0ULL;
+
+       if (!pc || !pc->cap_user_rdpmc)
+               return -1;
+
+       do {
+               seq = READ_ONCE(pc->lock);
+               barrier();
+
+               count->ena = READ_ONCE(pc->time_enabled);
+               count->run = READ_ONCE(pc->time_running);
+
+               if (pc->cap_user_time && count->ena != count->run) {
+                       cyc = read_timestamp();
+                       time_mult = READ_ONCE(pc->time_mult);
+                       time_shift = READ_ONCE(pc->time_shift);

tools/lib/perf/mmap.c union perf_event *perf_mmap__read_event(struct perf_mmap *map) · line 327

Quoted 6 lines

tools/lib/perf/mmap.c +6 -0

+                       time_offset = READ_ONCE(pc->time_offset);
+
+                       if (pc->cap_user_time_short) {
+                               time_cycles = READ_ONCE(pc->time_cycles);
+                               time_mask = READ_ONCE(pc->time_mask);
+                       }

Nit, this is now out of sync with the comment code in perf_event.h.

Quoted 26 lines

tools/lib/perf/mmap.c +26 -0

+               }
+
+               idx = READ_ONCE(pc->index);
+               cnt = READ_ONCE(pc->offset);
+               if (pc->cap_user_rdpmc && idx) {
+                       s64 evcnt = read_perf_counter(idx - 1);
+                       u16 width = READ_ONCE(pc->pmc_width);
+
+                       evcnt <<= 64 - width;
+                       evcnt >>= 64 - width;
+                       cnt += evcnt;
+               } else
+                       return -1;
+
+               barrier();
+       } while (READ_ONCE(pc->lock) != seq);
+
+       if (count->ena != count->run) {
+               u64 delta;
+
+               /* Adjust for cap_usr_time_short, a nop if not */
+               cyc = time_cycles + ((cyc - time_cycles) & time_mask);
+
+               delta = time_offset + mul_u64_u32_shr(cyc, time_mult, time_shift);
+
+               count->ena += delta;

tools/lib/perf/mmap.c union perf_event *perf_mmap__read_event(struct perf_mmap *map) · line 357

Quoted 4 lines

tools/lib/perf/mmap.c +4 -0

+               if (idx)
+                       count->run += delta;
+
+               cnt = mul_u64_u64_div64(cnt, count->ena, count->run);

Does this still suffer the divide by zero if multiplexing hasn't run the counter? If so, we still need to add something like:
https://lore.kernel.org/lkml/CAP-5=fVRdqvswtyQMg5cB+ntTGda+SAYskjTQednEH-AeZo13g@mail.gmail.com/

Quoted 15 lines

tools/lib/perf/mmap.c +6 -0

+       }
+
+       count->val = cnt;
+
+       return 0;
+}

tools/lib/perf/tests/test-evsel.c +0 -0

index 0ad82d7a2a51..54fb4809b9ee 100644
@@ -120,6 +120,69 @@ static int test_stat_thread_enable(void)
        return 0;
 }
 

tools/lib/perf/tests/test-evsel.c static int test_stat_thread_enable(void) · line 132

Quoted 10 lines

tools/lib/perf/tests/test-evsel.c +10 -0

+static int test_stat_user_read(int event)
+{
+       struct perf_counts_values counts = { .val = 0 };
+       struct perf_thread_map *threads;
+       struct perf_evsel *evsel;
+       struct perf_event_mmap_page *pc;
+       struct perf_event_attr attr = {
+               .type   = PERF_TYPE_HARDWARE,
+               .config = event,
+       };

A nit, previously test-evsel was able to run and pass on a hypervisor. As now there is a reliance on hardware events the evsel open fails on a hypervisor. It'd be nice if we could detect running on a hypervisor and test software events in that case.

Userspace counter access only works on heterogeneous systems with some restrictions. The userspace process must be pinned to a homogeneous subset of CPUs and must open the corresponding PMU for those CPUs. This commit adds a test implementing these requirements.

Signed-off-by Rob Herring <[email protected]>

---
v6:
- Add a check on cap_user_rdpmc
v5:
- Adapt to libperf mmap API changes
v4:
- Update perf_evsel__mmap params
v2:
- Drop all but heterogeneous test as others covered by libperf tests
- Rework to use libperf

---
 tools/perf/arch/arm64/include/arch-tests.h |   7 +
 tools/perf/arch/arm64/tests/Build          |   1 +
 tools/perf/arch/arm64/tests/arch-tests.c   |   4 +
 tools/perf/arch/arm64/tests/user-events.c  | 177 +++++++++++++++++++++
 4 files changed, 189 insertions(+)
 create mode 100644 tools/perf/arch/arm64/tests/user-events.c

tools/perf/arch/arm64/include/arch-tests.h +0 -0

index 90ec4c8cb880..380ad34a3f09 100644
@@ -2,11 +2,18 @@
 #ifndef ARCH_TESTS_H

tools/perf/arch/arm64/include/arch-tests.h +3 -0

 extern struct test arch_tests[];
+int test__rd_pinned(struct test __maybe_unused *test,
+		       int __maybe_unused subtest);
+
 
 #endif

tools/perf/arch/arm64/tests/Build +1 -0

index a61c06bdb757..3f9a20c17fc6 100644
@@ -1,4 +1,5 @@
 perf-y += regs_load.o
 perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o
 
+perf-y += user-events.o
 perf-y += arch-tests.o

tools/perf/arch/arm64/tests/arch-tests.c +4 -0

index 5b1543c98022..80ce7bd3c16d 100644
@@ -10,6 +10,10 @@ struct test arch_tests[] = {
 		.func = test__dwarf_unwind,
 	},
 #endif
+	{
+		.desc = "Pinned CPU user counter access",
+		.func = test__rd_pinned,
+	},
 	{
 		.func = NULL,
 	},

tools/perf/arch/arm64/tests/user-events.c new file +177 -0

new file mode 100644
index 000000000000..c8efc6b369e6
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <unistd.h>
+#include <sched.h>
+#include <cpumap.h>
+
+#include <perf/core.h>
+#include <perf/threadmap.h>
+#include <perf/evsel.h>
+
+#include "pmu.h"
+#include "debug.h"
+#include "tests/tests.h"
+#include "arch-tests.h"
+
+static int run_test(struct perf_evsel *evsel)
+{
+	int n;
+	volatile int tmp = 0;
+	u64 delta, i, loops = 1000;
+	struct perf_counts_values counts = { .val = 0 };
+
+	for (n = 0; n < 6; n++) {
+		u64 stamp, now;
+
+		perf_evsel__read(evsel, 0, 0, &counts);
+		stamp = counts.val;
+
+		for (i = 0; i < loops; i++)
+			tmp++;
+
+		perf_evsel__read(evsel, 0, 0, &counts);
+		now = counts.val;
+		loops *= 10;
+
+		delta = now - stamp;
+		pr_debug("%14d: %14llu\n", n, (long long)delta);
+
+		if (!delta)
+			break;
+	}
+	return delta ? 0 : -1;
+}
+
+static struct perf_pmu *pmu_for_cpu(int cpu)
+{
+	int acpu, idx;
+	struct perf_pmu *pmu = NULL;
+
+	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
+		if (pmu->is_uncore)
+			continue;
+		perf_cpu_map__for_each_cpu(acpu, idx, pmu->cpus)
+			if (acpu == cpu)
+				return pmu;
+	}
+	return NULL;
+}
+
+static bool pmu_is_homogeneous(void)
+{
+	int core_cnt = 0;
+	struct perf_pmu *pmu = NULL;
+
+	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
+		if (!pmu->is_uncore && !perf_cpu_map__empty(pmu->cpus))
+			core_cnt++;
+	}
+	return core_cnt == 1;
+}
+
+static int libperf_print(enum libperf_print_level level,
+			 const char *fmt, va_list ap)
+{
+	(void)level;
+	return vfprintf(stderr, fmt, ap);
+}
+
+static struct perf_evsel *perf_init(struct perf_event_attr *attr)
+{
+	int err;
+	struct perf_thread_map *threads;
+	struct perf_evsel *evsel;
+	struct perf_event_mmap_page *pc;
+
+	libperf_init(libperf_print);
+
+	threads = perf_thread_map__new_dummy();
+	if (!threads) {
+		pr_err("failed to create threads\n");
+		return NULL;
+	}
+
+	perf_thread_map__set_pid(threads, 0, 0);
+
+	evsel = perf_evsel__new(attr);
+	if (!evsel) {
+		pr_err("failed to create evsel\n");
+		goto out_thread;
+	}
+
+	err = perf_evsel__open(evsel, NULL, threads);
+	if (err) {
+		pr_err("failed to open evsel\n");
+		goto out_open;
+	}
+
+	if (perf_evsel__mmap(evsel, 0)) {
+		pr_err("failed to mmap evsel\n");
+		goto out_mmap;
+	}
+
+	pc = perf_evsel__mmap_base(evsel, 0, 0);
+	if (!pc->cap_user_rdpmc) {
+		pr_err("userspace access not enabled\n");
+		goto out_mmap;
+	}
+
+	return evsel;
+
+out_mmap:
+	perf_evsel__close(evsel);
+out_open:
+	perf_evsel__delete(evsel);
+out_thread:
+	perf_thread_map__put(threads);
+	return NULL;
+}
+
+int test__rd_pinned(struct test __maybe_unused *test,
+		    int __maybe_unused subtest)
+{
+	int cpu, cputmp, ret = -1;
+	struct perf_evsel *evsel;
+	struct perf_event_attr attr = {
+		.config = 0x8, /* Instruction count */
+		.config1 = 0, /* 32-bit counter */
+		.exclude_kernel = 1,
+	};
+	cpu_set_t cpu_set;
+	struct perf_pmu *pmu;
+
+	if (pmu_is_homogeneous())
+		return TEST_SKIP;
+
+	cpu = sched_getcpu();
+	pmu = pmu_for_cpu(cpu);
+	if (!pmu)
+		return -1;
+	attr.type = pmu->type;
+
+	CPU_ZERO(&cpu_set);
+	perf_cpu_map__for_each_cpu(cpu, cputmp, pmu->cpus)
+		CPU_SET(cpu, &cpu_set);
+	if (sched_setaffinity(0, sizeof(cpu_set), &cpu_set) < 0)
+		pr_err("Could not set affinity\n");
+
+	evsel = perf_init(&attr);
+	if (!evsel)
+		return -1;
+
+	perf_cpu_map__for_each_cpu(cpu, cputmp, pmu->cpus) {
+		CPU_ZERO(&cpu_set);
+		CPU_SET(cpu, &cpu_set);
+		if (sched_setaffinity(0, sizeof(cpu_set), &cpu_set) < 0)
+			pr_err("Could not set affinity\n");
+
+		pr_debug("Running on CPU %d\n", cpu);
+
+		ret = run_test(evsel);
+		if (ret)
+			break;
+	}
+
+	perf_evsel__close(evsel);
+	perf_evsel__delete(evsel);
+	return ret;
+}

Signature · 7 lines

2.27.0


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Raphael Gault <[email protected]>

Add a documentation file to describe the access to the pmu hardware counters from userspace

Signed-off-by Raphael Gault <[email protected]>

Signed-off-by Rob Herring <[email protected]>

---
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/index.rst                 |  1 +
 .../arm64/perf_counter_user_access.rst        | 60 +++++++++++++++++++

Changes in v6:
...