[PATCH 0/3] perf: arm_spe: Add support for a 'software' discard mode

[PATCH 0/3] perf: arm_spe: Add support for a 'software' discard mode

[James Clark]

Add a mode similar to discard mode that we get in SPE V1P2, but without
requiring hardware support. It works by allocating a single page in the
driver when opening the event so the buffer can still be programmed and
SPE enabled. The same page is mapped multiple times to make up a 4MB
buffer so the full interrupt is serviced less frequently.

Unfortunately we can't rely on userspace to do the allocation because
the existing HW discard mode came with the relaxation that an event can
be opened without mapping a buffer. Therefore adding SW discard mode
without doing the allocation in the driver would break old tools. It
will also be useful for very simple tools that only do raw
perf_event_open() calls and don't want to mess around with mmaps. Tools
also can't do the hack to present a 4MB buffer to hardware while only
being backed by a single page.

Discard mode is used to activate SPE related PMU events so that they can
be counted by the PMU, without the overhead of collecting any SPE data.
So it's likely that simple tools are using discard mode with only raw
perf_event_open() calls.

Signed-off-by: James Clark <james.clark@linaro.org>
---
James Clark (3):
      perf: arm_spe: Factor aux output and flags out of buffer fault handler
      perf: arm_spe: Factor HW discard mode out of arm_spe_perf_aux_output_begin()
      perf: arm_spe: Add support for a 'software' discard mode

 drivers/perf/arm_spe_pmu.c | 208 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 168 insertions(+), 40 deletions(-)
---
base-commit: 075b74841bd0065a3bda3440873c747938e69b68
change-id: 20260804-james-spe-software-discard-87691ce1a949

Best regards,
--  
James Clark <james.clark@linaro.org>

[PATCH 1/3] perf: arm_spe: Factor aux output and flags out of buffer fault handler

[James Clark]

Software discard mode will still want to handle buffer faults, but
doesn't have a real aux buffer to set flags or call
perf_aux_output_end() on. Factor the aux buffer parts out of
arm_spe_pmu_buf_get_fault_act() so it only decodes the fault, which
better matches the name and can be used by the new mode later.

This makes the arm_spe_pmu_stop() handling a bit simpler because it
doesn't need to conditionally call arm_spe_perf_aux_output_end() anymore
to handle the case when the fault handler didn't do it.

No functional changes intended.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/perf/arm_spe_pmu.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index dbd0da111639..10d28ad81256 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -677,11 +677,12 @@ static void arm_spe_pmu_disable_and_drain_local(void)
 
 /* IRQ handling */
 static enum arm_spe_pmu_buf_fault_action
-arm_spe_pmu_buf_get_fault_act(struct perf_output_handle *handle)
+arm_spe_pmu_buf_get_fault_act(u64 *aux_flags)
 {
 	const char *err_str;
 	u64 pmbsr;
-	enum arm_spe_pmu_buf_fault_action ret;
+
+	*aux_flags = 0;
 
 	/*
 	 * Ensure new profiling data is visible to the CPU and any external
@@ -703,12 +704,11 @@ arm_spe_pmu_buf_get_fault_act(struct perf_output_handle *handle)
 	 * flag to indicate that the last record is corrupted.
 	 */
 	if (FIELD_GET(PMBSR_EL1_DL, pmbsr))
-		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED |
-					     PERF_AUX_FLAG_PARTIAL);
+		*aux_flags |= PERF_AUX_FLAG_TRUNCATED | PERF_AUX_FLAG_PARTIAL;
 
 	/* Report collisions to userspace so that it can up the period */
 	if (FIELD_GET(PMBSR_EL1_COLL, pmbsr))
-		perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
+		*aux_flags |= PERF_AUX_FLAG_COLLISION;
 
 	/* We only expect buffer management events */
 	switch (FIELD_GET(PMBSR_EL1_EC, pmbsr)) {
@@ -727,8 +727,7 @@ arm_spe_pmu_buf_get_fault_act(struct perf_output_handle *handle)
 	/* Buffer management event */
 	switch (FIELD_GET(PMBSR_EL1_BUF_BSC_MASK, pmbsr)) {
 	case PMBSR_EL1_BUF_BSC_FULL:
-		ret = SPE_PMU_BUF_FAULT_ACT_OK;
-		goto out_stop;
+		return SPE_PMU_BUF_FAULT_ACT_OK;
 	default:
 		err_str = "Unknown buffer status code";
 	}
@@ -738,11 +737,7 @@ arm_spe_pmu_buf_get_fault_act(struct perf_output_handle *handle)
 			   err_str, smp_processor_id(), pmbsr,
 			   read_sysreg_s(SYS_PMBPTR_EL1),
 			   read_sysreg_s(SYS_PMBLIMITR_EL1));
-	ret = SPE_PMU_BUF_FAULT_ACT_FATAL;
-
-out_stop:
-	arm_spe_perf_aux_output_end(handle);
-	return ret;
+	return SPE_PMU_BUF_FAULT_ACT_FATAL;
 }
 
 static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
@@ -750,14 +745,18 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
 	struct perf_output_handle *handle = dev;
 	struct perf_event *event = handle->event;
 	enum arm_spe_pmu_buf_fault_action act;
+	u64 aux_flags;
 
 	if (!perf_get_aux(handle))
 		return IRQ_NONE;
 
-	act = arm_spe_pmu_buf_get_fault_act(handle);
+	act = arm_spe_pmu_buf_get_fault_act(&aux_flags);
 	if (act == SPE_PMU_BUF_FAULT_ACT_SPURIOUS)
 		return IRQ_NONE;
 
+	perf_aux_output_flag(handle, aux_flags);
+	arm_spe_perf_aux_output_end(handle);
+
 	/*
 	 * Ensure perf callbacks have completed, which may disable the
 	 * profiling buffer in response to a TRUNCATION flag.
@@ -927,6 +926,7 @@ static void arm_spe_pmu_stop(struct perf_event *event, int flags)
 	struct arm_spe_pmu *spe_pmu = to_spe_pmu(event->pmu);
 	struct hw_perf_event *hwc = &event->hw;
 	struct perf_output_handle *handle = this_cpu_ptr(spe_pmu->handle);
+	u64 aux_flags;
 
 	/* If we're already stopped, then nothing to do */
 	if (hwc->state & PERF_HES_STOPPED)
@@ -944,10 +944,12 @@ static void arm_spe_pmu_stop(struct perf_event *event, int flags)
 		if (perf_get_aux(handle)) {
 			enum arm_spe_pmu_buf_fault_action act;
 
-			act = arm_spe_pmu_buf_get_fault_act(handle);
-			if (act == SPE_PMU_BUF_FAULT_ACT_SPURIOUS)
-				arm_spe_perf_aux_output_end(handle);
-			else
+			act = arm_spe_pmu_buf_get_fault_act(&aux_flags);
+			perf_aux_output_flag(handle, aux_flags);
+			arm_spe_perf_aux_output_end(handle);
+
+			/* Assume PMBSR only needs clearing for real faults */
+			if (act != SPE_PMU_BUF_FAULT_ACT_SPURIOUS)
 				write_sysreg_s(0, SYS_PMBSR_EL1);
 		}
 

-- 
2.43.0

[PATCH 2/3] perf: arm_spe: Factor HW discard mode out of arm_spe_perf_aux_output_begin()

[James Clark]

Discard mode doesn't call perf_aux_output_begin() or emit aux records,
and enabling it never fails. We'll add a second discard mode in the
next commit so it will be cleaner if there is a separate begin function
just for discard mode.

No functional changes intended.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/perf/arm_spe_pmu.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index 10d28ad81256..562a1d3be39f 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -363,6 +363,11 @@ static const struct attribute_group *arm_spe_pmu_attr_groups[] = {
 	NULL,
 };
 
+static bool arm_spe_discard_mode(struct perf_event *event)
+{
+	return ATTR_CFG_GET_FLD(&event->attr, discard);
+}
+
 /* Convert between user ABI and register values */
 static u64 arm_spe_event_to_pmscr(struct perf_event *event)
 {
@@ -609,18 +614,23 @@ static u64 arm_spe_pmu_next_off(struct perf_output_handle *handle)
 	return limit;
 }
 
+static void arm_spe_pmu_begin_discard(struct perf_output_handle *handle,
+				      struct perf_event *event)
+{
+	u64 limit;
+
+	limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
+	limit |= PMBLIMITR_EL1_E;
+
+	write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
+}
+
 static int arm_spe_perf_aux_output_begin(struct perf_output_handle *handle,
 					 struct perf_event *event)
 {
 	u64 base, limit;
 	struct arm_spe_pmu_buf *buf;
 
-	if (ATTR_CFG_GET_FLD(&event->attr, discard)) {
-		limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
-		limit |= PMBLIMITR_EL1_E;
-		goto out_write_limit;
-	}
-
 	/* Start a new aux session */
 	buf = perf_aux_output_begin(handle, event);
 	if (!buf) {
@@ -775,6 +785,12 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
 		arm_spe_pmu_disable_and_drain_local();
 		break;
 	case SPE_PMU_BUF_FAULT_ACT_OK:
+		if (arm_spe_discard_mode(event)) {
+			arm_spe_pmu_begin_discard(handle, event);
+			isb();
+			break;
+		}
+
 		/*
 		 * We handled the fault (the buffer was full), so resume
 		 * profiling as long as we didn't detect truncation.
@@ -864,7 +880,7 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
 	    !(spe_pmu->features & SPE_PMU_FEAT_EFT))
 		return -EOPNOTSUPP;
 
-	if (ATTR_CFG_GET_FLD(&event->attr, discard) &&
+	if (arm_spe_discard_mode(event) &&
 	    !(spe_pmu->features & SPE_PMU_FEAT_DISCARD))
 		return -EOPNOTSUPP;
 
@@ -884,7 +900,9 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
 	struct perf_output_handle *handle = this_cpu_ptr(spe_pmu->handle);
 
 	hwc->state = 0;
-	if (arm_spe_perf_aux_output_begin(handle, event)) {
+	if (arm_spe_discard_mode(event)) {
+		arm_spe_pmu_begin_discard(handle, event);
+	} else if (arm_spe_perf_aux_output_begin(handle, event)) {
 		arm_spe_pmu_stop(event, 0);
 		return;
 	}

-- 
2.43.0

[PATCH 3/3] perf: arm_spe: Add support for a 'software' discard mode

[James Clark]

Currently discard mode needs hardware support, but we can get something
that appears pretty similar to userspace by writing to a single page
allocated by the driver and not emitting aux records.

Remove the -EOPNOTSUPP, as now we use the same discard mode format attr
as with HW support but fall back to the software version.

Don't call perf_get_aux() in discard mode as it unconditionally
dereferences a NULL event->rb when handle->event exists, which can now
be true.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/perf/arm_spe_pmu.c | 160 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 134 insertions(+), 26 deletions(-)

diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index 562a1d3be39f..093fa3cc5084 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -60,6 +60,7 @@ static bool get_spe_event_has_cx(struct perf_event *event)
 }
 
 #define ARM_SPE_BUF_PAD_BYTE			0
+#define ARM_SPE_DISCARD_BUF_SIZE		SZ_4M
 
 struct arm_spe_pmu_buf {
 	int					nr_pages;
@@ -67,6 +68,12 @@ struct arm_spe_pmu_buf {
 	void					*base;
 };
 
+struct arm_spe_pmu_sw_discard_buf {
+	struct page				*page;
+	void					*base;
+	refcount_t				refcount;
+};
+
 struct arm_spe_pmu {
 	struct pmu				pmu;
 	struct platform_device			*pdev;
@@ -85,7 +92,7 @@ struct arm_spe_pmu {
 #define SPE_PMU_FEAT_LDS			(1UL << 4)
 #define SPE_PMU_FEAT_ERND			(1UL << 5)
 #define SPE_PMU_FEAT_INV_FILT_EVT		(1UL << 6)
-#define SPE_PMU_FEAT_DISCARD			(1UL << 7)
+#define SPE_PMU_FEAT_HW_DISCARD			(1UL << 7)
 #define SPE_PMU_FEAT_EFT			(1UL << 8)
 #define SPE_PMU_FEAT_FDS			(1UL << 9)
 #define SPE_PMU_FEAT_DEV_PROBED			(1UL << 63)
@@ -218,7 +225,8 @@ static const struct attribute_group arm_spe_pmu_cap_group = {
 #define ATTR_CFG_FLD_store_filter_CFG		config	/* PMSFCR_EL1.ST */
 #define ATTR_CFG_FLD_store_filter_LO		34
 #define ATTR_CFG_FLD_store_filter_HI		34
-#define ATTR_CFG_FLD_discard_CFG		config	/* PMBLIMITR_EL1.FM = DISCARD */
+/* PMBLIMITR_EL1.FM = DISCARD, or scratch buffer if no HW support */
+#define ATTR_CFG_FLD_discard_CFG		config
 #define ATTR_CFG_FLD_discard_LO			35
 #define ATTR_CFG_FLD_discard_HI			35
 #define ATTR_CFG_FLD_branch_filter_mask_CFG	config	/* PMSFCR_EL1.Bm */
@@ -309,9 +317,6 @@ static umode_t arm_spe_pmu_format_attr_is_visible(struct kobject *kobj,
 	struct device *dev = kobj_to_dev(kobj);
 	struct arm_spe_pmu *spe_pmu = dev_get_drvdata(dev);
 
-	if (attr == &format_attr_discard.attr && !(spe_pmu->features & SPE_PMU_FEAT_DISCARD))
-		return 0;
-
 	if (attr == &format_attr_inv_event_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT))
 		return 0;
 
@@ -368,6 +373,82 @@ static bool arm_spe_discard_mode(struct perf_event *event)
 	return ATTR_CFG_GET_FLD(&event->attr, discard);
 }
 
+static bool arm_spe_uses_sw_discard_buf(struct perf_event *event)
+{
+	struct arm_spe_pmu *spe_pmu = to_spe_pmu(event->pmu);
+
+	return arm_spe_discard_mode(event) &&
+	       !(spe_pmu->features & SPE_PMU_FEAT_HW_DISCARD);
+}
+
+static void arm_spe_pmu_free_discard_buf(struct perf_event *event)
+{
+	struct arm_spe_pmu_sw_discard_buf *buf = event->pmu_private;
+
+	event->pmu_private = NULL;
+	if (!refcount_dec_and_test(&buf->refcount))
+		return;
+
+	vunmap(buf->base);
+	__free_page(buf->page);
+	kfree(buf);
+}
+
+/*
+ * Map a single page multiple times to make up ARM_SPE_DISCARD_BUF_SIZE.
+ *
+ * This reduces the number of interrupts that have to be serviced in software
+ * discard mode, but at the same time only uses a page of memory. We don't need
+ * to worry about samples being overwritten because they're never read.
+ */
+static int arm_spe_pmu_alloc_discard_buf(struct perf_event *event)
+{
+	int node = (event->cpu == -1) ? -1 : cpu_to_node(event->cpu);
+	int nr_pages = ARM_SPE_DISCARD_BUF_SIZE / PAGE_SIZE;
+	struct arm_spe_pmu_sw_discard_buf *buf;
+	struct page **pglist;
+	int i;
+
+	if (event->parent) {
+		buf = event->parent->pmu_private;
+		refcount_inc(&buf->refcount);
+		event->pmu_private = buf;
+		event->destroy = arm_spe_pmu_free_discard_buf;
+		return 0;
+	}
+
+	buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, node);
+	if (!buf)
+		return -ENOMEM;
+
+	buf->page = alloc_pages_node(node, GFP_KERNEL, 0);
+	if (!buf->page)
+		goto out_free_buf;
+
+	pglist = kvmalloc_array(nr_pages, sizeof(*pglist), GFP_KERNEL);
+	if (!pglist)
+		goto out_free_page;
+
+	for (i = 0; i < nr_pages; i++)
+		pglist[i] = buf->page;
+
+	buf->base = vmap(pglist, nr_pages, VM_MAP, PAGE_KERNEL);
+	kvfree(pglist);
+	if (!buf->base)
+		goto out_free_page;
+
+	refcount_set(&buf->refcount, 1);
+	event->pmu_private = buf;
+	event->destroy = arm_spe_pmu_free_discard_buf;
+	return 0;
+
+out_free_page:
+	__free_page(buf->page);
+out_free_buf:
+	kfree(buf);
+	return -ENOMEM;
+}
+
 /* Convert between user ABI and register values */
 static u64 arm_spe_event_to_pmscr(struct perf_event *event)
 {
@@ -617,10 +698,26 @@ static u64 arm_spe_pmu_next_off(struct perf_output_handle *handle)
 static void arm_spe_pmu_begin_discard(struct perf_output_handle *handle,
 				      struct perf_event *event)
 {
-	u64 limit;
+	struct arm_spe_pmu *spe_pmu = to_spe_pmu(event->pmu);
+	struct arm_spe_pmu_sw_discard_buf *discard_buf = event->pmu_private;
+	u64 base, limit;
+
+	if (spe_pmu->features & SPE_PMU_FEAT_HW_DISCARD) {
+		limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
+		limit |= PMBLIMITR_EL1_E;
+	} else {
+		base = (u64) discard_buf->base;
+		limit = base + ARM_SPE_DISCARD_BUF_SIZE;
+		limit |= PMBLIMITR_EL1_E;
+		write_sysreg_s(base, SYS_PMBPTR_EL1);
 
-	limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
-	limit |= PMBLIMITR_EL1_E;
+		/*
+		 * Track the event so we can access the discard buffer through
+		 * event->pmu_private. perf_aux_output_begin() normally handles
+		 * this, but we can do it manually if that's not being used.
+		 */
+		handle->event = event;
+	}
 
 	write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
 }
@@ -757,15 +854,20 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
 	enum arm_spe_pmu_buf_fault_action act;
 	u64 aux_flags;
 
-	if (!perf_get_aux(handle))
+	if (!event)
+		return IRQ_NONE;
+
+	if (!arm_spe_discard_mode(event) && !perf_get_aux(handle))
 		return IRQ_NONE;
 
 	act = arm_spe_pmu_buf_get_fault_act(&aux_flags);
 	if (act == SPE_PMU_BUF_FAULT_ACT_SPURIOUS)
 		return IRQ_NONE;
 
-	perf_aux_output_flag(handle, aux_flags);
-	arm_spe_perf_aux_output_end(handle);
+	if (!arm_spe_discard_mode(event)) {
+		perf_aux_output_flag(handle, aux_flags);
+		arm_spe_perf_aux_output_end(handle);
+	}
 
 	/*
 	 * Ensure perf callbacks have completed, which may disable the
@@ -819,6 +921,7 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
 static int arm_spe_pmu_event_init(struct perf_event *event)
 {
 	u64 reg;
+	int ret;
 	struct perf_event_attr *attr = &event->attr;
 	struct arm_spe_pmu *spe_pmu = to_spe_pmu(event->pmu);
 
@@ -880,14 +983,16 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
 	    !(spe_pmu->features & SPE_PMU_FEAT_EFT))
 		return -EOPNOTSUPP;
 
-	if (arm_spe_discard_mode(event) &&
-	    !(spe_pmu->features & SPE_PMU_FEAT_DISCARD))
-		return -EOPNOTSUPP;
-
 	set_spe_event_has_cx(event);
 	reg = arm_spe_event_to_pmscr(event);
-	if (reg & (PMSCR_EL1_PA | PMSCR_EL1_PCT))
-		return perf_allow_kernel();
+	if (reg & (PMSCR_EL1_PA | PMSCR_EL1_PCT)) {
+		ret = perf_allow_kernel();
+		if (ret)
+			return ret;
+	}
+
+	if (arm_spe_uses_sw_discard_buf(event))
+		return arm_spe_pmu_alloc_discard_buf(event);
 
 	return 0;
 }
@@ -954,23 +1059,26 @@ static void arm_spe_pmu_stop(struct perf_event *event, int flags)
 	arm_spe_pmu_disable_and_drain_local();
 
 	if (flags & PERF_EF_UPDATE) {
+		enum arm_spe_pmu_buf_fault_action act;
+
 		/*
 		 * If there's a fault pending then ensure we contain it
 		 * to this buffer, since we might be on the context-switch
 		 * path.
 		 */
-		if (perf_get_aux(handle)) {
-			enum arm_spe_pmu_buf_fault_action act;
-
-			act = arm_spe_pmu_buf_get_fault_act(&aux_flags);
+		act = arm_spe_pmu_buf_get_fault_act(&aux_flags);
+		if (!arm_spe_discard_mode(event) && perf_get_aux(handle)) {
 			perf_aux_output_flag(handle, aux_flags);
 			arm_spe_perf_aux_output_end(handle);
-
-			/* Assume PMBSR only needs clearing for real faults */
-			if (act != SPE_PMU_BUF_FAULT_ACT_SPURIOUS)
-				write_sysreg_s(0, SYS_PMBSR_EL1);
+		} else {
+			/* Discard mode tracks event outside of perf_aux_output_X() */
+			handle->event = NULL;
 		}
 
+		/* Assume PMBSR only needs clearing for real faults */
+		if (act != SPE_PMU_BUF_FAULT_ACT_SPURIOUS)
+			write_sysreg_s(0, SYS_PMBSR_EL1);
+
 		/*
 		 * This may also contain ECOUNT, but nobody else should
 		 * be looking at period_left, since we forbid frequency
@@ -1180,7 +1288,7 @@ static void __arm_spe_pmu_dev_probe(void *info)
 		spe_pmu->features |= SPE_PMU_FEAT_ERND;
 
 	if (spe_pmu->pmsver >= ID_AA64DFR0_EL1_PMSVer_V1P2)
-		spe_pmu->features |= SPE_PMU_FEAT_DISCARD;
+		spe_pmu->features |= SPE_PMU_FEAT_HW_DISCARD;
 
 	if (FIELD_GET(PMSIDR_EL1_EFT, reg))
 		spe_pmu->features |= SPE_PMU_FEAT_EFT;

-- 
2.43.0

Re: [PATCH 1/3] perf: arm_spe: Factor aux output and flags out of buffer fault handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing immediate `isb()` after writing to control-plane system register `SYS_PMBLIMITR_EL1`.
- [High] Missing immediate `isb()` after writing to control-plane system register `SYS_PMSCR_EL1`.
--

--- Patch [1]: [PATCH 1/3] perf: arm_spe: Factor aux output and flags out of buffer fault handler ---
commit 454b555e21bef7b51b93cb02a78486de3845b1b9
Author: James Clark <james.clark@linaro.org>
Subject: perf: arm_spe: Factor aux output and flags out of buffer fault handler

This patch refactors the ARM SPE PMU buffer fault handling by separating the
auxiliary buffer output management and flags from the fault decoding logic.
This simplifies the stop handling routine as it no longer conditionally
calls the aux output end function. No functional changes are intended.

> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index dbd0da1116390..10d28ad81256f 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -677,11 +677,12 @@ static void arm_spe_pmu_disable_and_drain_local(void)
>  
>  /* IRQ handling */
>  static enum arm_spe_pmu_buf_fault_action
> -arm_spe_pmu_buf_get_fault_act(struct perf_output_handle *handle)
> +arm_spe_pmu_buf_get_fault_act(u64 *aux_flags)
>  {

[ ... ]

> out_write_limit:
> 	write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
> 	return (limit & PMBLIMITR_EL1_E) ? 0 : -EIO;

[Severity: High]
This is a pre-existing issue, but is an immediate isb() required here?

The ARM64 architecture guidelines state that writes to control-plane system
registers like SYS_PMBLIMITR_EL1 must be followed by an isb() as the very
next instruction. In arm_spe_perf_aux_output_begin() in 
drivers/perf/arm_spe_pmu.c, the function returns immediately after the write,
which could leave the CPU pipeline in an architecturally undefined state for
subsequent instructions.

[ ... ]

> 	reg = arm_spe_event_to_pmscr(event);
> 	isb();
> 	write_sysreg_s(reg, SYS_PMSCR_EL1);
> }

[Severity: High]
This isn't a bug introduced by this patch, but shouldn't there be an isb()
after this system register write?

In arm_spe_pmu_start() in drivers/perf/arm_spe_pmu.c, an isb() is issued
before writing to SYS_PMSCR_EL1, but it appears missing immediately after
the write. Missing context synchronization after a system register write
might leave the pipeline exposed to a stale configuration state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-james-spe-software-discard-v1-0-0ff0e44c4c92@linaro.org?part=1

Re: [PATCH 2/3] perf: arm_spe: Factor HW discard mode out of arm_spe_perf_aux_output_begin()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unhandled Interrupt Storm / Unreachable Code due to uninitialized perf_output_handle in discard mode

Pre-existing issues:
- [High] Missing Context Synchronization Event (ISB) after writing to SYS_PMBLIMITR_EL1
--

--- Patch [2]: [PATCH 2/3] perf: arm_spe: Factor HW discard mode out of arm_spe_perf_aux_output_begin() ---
commit 61201ea6d6b4e69de62f9788efe1db4fd64b4fd9
Author: James Clark <james.clark@linaro.org>

perf: arm_spe: Factor HW discard mode out of arm_spe_perf_aux_output_begin()

Discard mode doesn't call perf_aux_output_begin() or emit aux records,
and enabling it never fails. We'll add a second discard mode in the
next commit so it will be cleaner if there is a separate begin function
just for discard mode.

> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
[ ... ]
> @@ -609,18 +614,23 @@ static u64 arm_spe_pmu_next_off(struct perf_output_handle *handle)
>  	return limit;
>  }
>  
> +static void arm_spe_pmu_begin_discard(struct perf_output_handle *handle,
> +				      struct perf_event *event)
> +{
> +	u64 limit;
> +
> +	limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
> +	limit |= PMBLIMITR_EL1_E;
> +
> +	write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
> +}

[Severity: High]
This isn't a bug introduced by this patch, but writing to a control-plane
system register like SYS_PMBLIMITR_EL1 requires an immediate isb() to avoid
leaving the CPU pipeline in an architecturally undefined state.

Looking at the caller in arm_spe_pmu_irq_handler(), the isb() is issued after
this function returns, which means the CPU executes the function return
instructions before the barrier. In arm_spe_pmu_start(), execution proceeds to
even more instructions before an isb() is eventually issued.

Should the isb() be placed immediately after the write_sysreg_s() directly
inside arm_spe_pmu_begin_discard() to guarantee architectural compliance?

[ ... ]
> @@ -884,7 +900,9 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
>  	struct perf_output_handle *handle = this_cpu_ptr(spe_pmu->handle);
>  
>  	hwc->state = 0;
> -	if (arm_spe_perf_aux_output_begin(handle, event)) {
> +	if (arm_spe_discard_mode(event)) {
> +		arm_spe_pmu_begin_discard(handle, event);
> +	} else if (arm_spe_perf_aux_output_begin(handle, event)) {
>  		arm_spe_pmu_stop(event, 0);
>  		return;
>  	}

[Severity: High]
Because discard mode explicitly bypasses perf_aux_output_begin() here, the
per-CPU perf_output_handle is never initialized with the ring buffer or event.

If the PMU hardware asserts an interrupt during this mode, wouldn't
arm_spe_pmu_irq_handler() see an uninitialized (or previously NULLed) handle?

It looks like early checks in the IRQ handler, such as checking if the event
is present or if perf_get_aux(handle) is valid, would evaluate to true and
return IRQ_NONE immediately without clearing the hardware interrupt syndrome.
Could this lead to an unhandled interrupt storm that locks up the CPU?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-james-spe-software-discard-v1-0-0ff0e44c4c92@linaro.org?part=2

Re: [PATCH 3/3] perf: arm_spe: Add support for a 'software' discard mode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Writes to control-plane system registers (`SYS_PMBPTR_EL1` and `SYS_PMBLIMITR_EL1`) lack an immediate `isb()` synchronization barrier, violating ARM64 architectural invariants.
- [Medium] User-triggerable allocations for the software discard buffer lack `__GFP_ACCOUNT`, allowing a user to bypass memory cgroup limits.
--

--- Patch [3]: [PATCH 3/3] perf: arm_spe: Add support for a 'software' discard mode ---
commit af3311701234ae1a3b87d9c00cb9f6f3748862b7
Author: James Clark <james.clark@linaro.org>

perf: arm_spe: Add support for a 'software' discard mode

This patch implements a software discard mode for ARM SPE. It falls back
to writing to a single driver-allocated page and not emitting aux records 
when hardware discard support is unavailable.

> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c

[ ... ]

> +static int arm_spe_pmu_alloc_discard_buf(struct perf_event *event)
> +{
> +	int node = (event->cpu == -1) ? -1 : cpu_to_node(event->cpu);
> +	int nr_pages = ARM_SPE_DISCARD_BUF_SIZE / PAGE_SIZE;
> +	struct arm_spe_pmu_sw_discard_buf *buf;
> +	struct page **pglist;
> +	int i;
> +
> +	if (event->parent) {

[ ... ]

> +		return 0;
> +	}
> +
> +	buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, node);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	buf->page = alloc_pages_node(node, GFP_KERNEL, 0);
> +	if (!buf->page)
> +		goto out_free_buf;
> +
> +	pglist = kvmalloc_array(nr_pages, sizeof(*pglist), GFP_KERNEL);

[Severity: Medium]
Should these GFP_KERNEL allocations include __GFP_ACCOUNT? 

Since this memory is allocated on behalf of a user process creating an 
SPE event, omitting __GFP_ACCOUNT could allow a container to allocate 
kernel memory without being charged, potentially bypassing memory limits.

> +	if (!pglist)
> +		goto out_free_page;

[ ... ]

> @@ -617,10 +698,26 @@ static u64 arm_spe_pmu_next_off(struct perf_output_handle *handle)
> -static void arm_spe_pmu_begin_discard(struct perf_output_handle *handle,
> -				      struct perf_event *event)
> +static void arm_spe_pmu_begin_discard(struct perf_output_handle *handle,
> +				      struct perf_event *event)
>  {
> -	u64 limit;
> +	struct arm_spe_pmu *spe_pmu = to_spe_pmu(event->pmu);
> +	struct arm_spe_pmu_sw_discard_buf *discard_buf = event->pmu_private;
> +	u64 base, limit;
> +
> +	if (spe_pmu->features & SPE_PMU_FEAT_HW_DISCARD) {
> +		limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
> +		limit |= PMBLIMITR_EL1_E;
> +	} else {
> +		base = (u64) discard_buf->base;
> +		limit = base + ARM_SPE_DISCARD_BUF_SIZE;
> +		limit |= PMBLIMITR_EL1_E;
> +		write_sysreg_s(base, SYS_PMBPTR_EL1);
> +
> +		/*
> +		 * Track the event so we can access the discard buffer through
> +		 * event->pmu_private. perf_aux_output_begin() normally handles
> +		 * this, but we can do it manually if that's not being used.
> +		 */
> +		handle->event = event;

[Severity: High]
Does this write to SYS_PMBPTR_EL1 require an immediate isb() synchronization
barrier? 

Without it, the subsequent memory store (handle->event = event) might execute
while the CPU pipeline is operating under an architecturally undefined state.

> +	}
>  
> -	limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
> -	limit |= PMBLIMITR_EL1_E;
> +	write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
> +}

[Severity: High]
Is an isb() barrier also missing after this write to SYS_PMBLIMITR_EL1? 

Because it is at the very end of arm_spe_pmu_begin_discard(), the function
return and any subsequent conditional branches in the caller will execute 
before the pipeline state is synchronized.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-james-spe-software-discard-v1-0-0ff0e44c4c92@linaro.org?part=3

Re: [PATCH 2/3] perf: arm_spe: Factor HW discard mode out of arm_spe_perf_aux_output_begin()

[James Clark]

On 05/08/2026 16:37, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Unhandled Interrupt Storm / Unreachable Code due to uninitialized perf_output_handle in discard mode
> 
> Pre-existing issues:
> - [High] Missing Context Synchronization Event (ISB) after writing to SYS_PMBLIMITR_EL1
> --
> 
> --- Patch [2]: [PATCH 2/3] perf: arm_spe: Factor HW discard mode out of arm_spe_perf_aux_output_begin() ---
> commit 61201ea6d6b4e69de62f9788efe1db4fd64b4fd9
> Author: James Clark <james.clark@linaro.org>
> 
> perf: arm_spe: Factor HW discard mode out of arm_spe_perf_aux_output_begin()
> 
> Discard mode doesn't call perf_aux_output_begin() or emit aux records,
> and enabling it never fails. We'll add a second discard mode in the
> next commit so it will be cleaner if there is a separate begin function
> just for discard mode.
> 
>> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
>> --- a/drivers/perf/arm_spe_pmu.c
>> +++ b/drivers/perf/arm_spe_pmu.c
> [ ... ]
>> @@ -609,18 +614,23 @@ static u64 arm_spe_pmu_next_off(struct perf_output_handle *handle)
>>   	return limit;
>>   }
>>   
>> +static void arm_spe_pmu_begin_discard(struct perf_output_handle *handle,
>> +				      struct perf_event *event)
>> +{
>> +	u64 limit;
>> +
>> +	limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
>> +	limit |= PMBLIMITR_EL1_E;
>> +
>> +	write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
>> +}
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but writing to a control-plane
> system register like SYS_PMBLIMITR_EL1 requires an immediate isb() to avoid
> leaving the CPU pipeline in an architecturally undefined state.
> 
> Looking at the caller in arm_spe_pmu_irq_handler(), the isb() is issued after
> this function returns, which means the CPU executes the function return
> instructions before the barrier. In arm_spe_pmu_start(), execution proceeds to
> even more instructions before an isb() is eventually issued.
> 
> Should the isb() be placed immediately after the write_sysreg_s() directly
> inside arm_spe_pmu_begin_discard() to guarantee architectural compliance?
> 
> [ ... ]
>> @@ -884,7 +900,9 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
>>   	struct perf_output_handle *handle = this_cpu_ptr(spe_pmu->handle);
>>   
>>   	hwc->state = 0;
>> -	if (arm_spe_perf_aux_output_begin(handle, event)) {
>> +	if (arm_spe_discard_mode(event)) {
>> +		arm_spe_pmu_begin_discard(handle, event);
>> +	} else if (arm_spe_perf_aux_output_begin(handle, event)) {
>>   		arm_spe_pmu_stop(event, 0);
>>   		return;
>>   	}
> 
> [Severity: High]
> Because discard mode explicitly bypasses perf_aux_output_begin() here, the
> per-CPU perf_output_handle is never initialized with the ring buffer or event.
> 

If this is an issue then it's an existing one because this patch is just 
a refactor.

> If the PMU hardware asserts an interrupt during this mode, wouldn't
> arm_spe_pmu_irq_handler() see an uninitialized (or previously NULLed) handle?
> 
> It looks like early checks in the IRQ handler, such as checking if the event
> is present or if perf_get_aux(handle) is valid, would evaluate to true and
> return IRQ_NONE immediately without clearing the hardware interrupt syndrome.
> Could this lead to an unhandled interrupt storm that locks up the CPU?
> 

HW discard mode doesn't trigger interrupts. If it does then a separate 
fixes commit is required unrelated to these patches.

Re: [PATCH 3/3] perf: arm_spe: Add support for a 'software' discard mode

[James Clark]

On 05/08/2026 16:58, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Writes to control-plane system registers (`SYS_PMBPTR_EL1` and `SYS_PMBLIMITR_EL1`) lack an immediate `isb()` synchronization barrier, violating ARM64 architectural invariants.
> - [Medium] User-triggerable allocations for the software discard buffer lack `__GFP_ACCOUNT`, allowing a user to bypass memory cgroup limits.
> --
> 
> --- Patch [3]: [PATCH 3/3] perf: arm_spe: Add support for a 'software' discard mode ---
> commit af3311701234ae1a3b87d9c00cb9f6f3748862b7
> Author: James Clark <james.clark@linaro.org>
> 
> perf: arm_spe: Add support for a 'software' discard mode
> 
> This patch implements a software discard mode for ARM SPE. It falls back
> to writing to a single driver-allocated page and not emitting aux records
> when hardware discard support is unavailable.
> 
>> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
>> --- a/drivers/perf/arm_spe_pmu.c
>> +++ b/drivers/perf/arm_spe_pmu.c
> 
> [ ... ]
> 
>> +static int arm_spe_pmu_alloc_discard_buf(struct perf_event *event)
>> +{
>> +	int node = (event->cpu == -1) ? -1 : cpu_to_node(event->cpu);
>> +	int nr_pages = ARM_SPE_DISCARD_BUF_SIZE / PAGE_SIZE;
>> +	struct arm_spe_pmu_sw_discard_buf *buf;
>> +	struct page **pglist;
>> +	int i;
>> +
>> +	if (event->parent) {
> 
> [ ... ]
> 
>> +		return 0;
>> +	}
>> +
>> +	buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, node);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	buf->page = alloc_pages_node(node, GFP_KERNEL, 0);
>> +	if (!buf->page)
>> +		goto out_free_buf;
>> +
>> +	pglist = kvmalloc_array(nr_pages, sizeof(*pglist), GFP_KERNEL);
> 
> [Severity: Medium]
> Should these GFP_KERNEL allocations include __GFP_ACCOUNT?
> 
> Since this memory is allocated on behalf of a user process creating an
> SPE event, omitting __GFP_ACCOUNT could allow a container to allocate
> kernel memory without being charged, potentially bypassing memory limits.
> 

I can't find any examples of __GFP_ACCOUNT being used in a similar way. 
Even the existing arm_spe_pmu_setup_aux() doesn't use it. So I'm not 
sure what the precedent is.

>> +	if (!pglist)
>> +		goto out_free_page;
> 
> [ ... ]
> 
>> @@ -617,10 +698,26 @@ static u64 arm_spe_pmu_next_off(struct perf_output_handle *handle)
>> -static void arm_spe_pmu_begin_discard(struct perf_output_handle *handle,
>> -				      struct perf_event *event)
>> +static void arm_spe_pmu_begin_discard(struct perf_output_handle *handle,
>> +				      struct perf_event *event)
>>   {
>> -	u64 limit;
>> +	struct arm_spe_pmu *spe_pmu = to_spe_pmu(event->pmu);
>> +	struct arm_spe_pmu_sw_discard_buf *discard_buf = event->pmu_private;
>> +	u64 base, limit;
>> +
>> +	if (spe_pmu->features & SPE_PMU_FEAT_HW_DISCARD) {
>> +		limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
>> +		limit |= PMBLIMITR_EL1_E;
>> +	} else {
>> +		base = (u64) discard_buf->base;
>> +		limit = base + ARM_SPE_DISCARD_BUF_SIZE;
>> +		limit |= PMBLIMITR_EL1_E;
>> +		write_sysreg_s(base, SYS_PMBPTR_EL1);
>> +
>> +		/*
>> +		 * Track the event so we can access the discard buffer through
>> +		 * event->pmu_private. perf_aux_output_begin() normally handles
>> +		 * this, but we can do it manually if that's not being used.
>> +		 */
>> +		handle->event = event;
> 
> [Severity: High]
> Does this write to SYS_PMBPTR_EL1 require an immediate isb() synchronization
> barrier?
> 
> Without it, the subsequent memory store (handle->event = event) might execute
> while the CPU pipeline is operating under an architecturally undefined state.
> 
>> +	}
>>   
>> -	limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
>> -	limit |= PMBLIMITR_EL1_E;
>> +	write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
>> +}
> 
> [Severity: High]
> Is an isb() barrier also missing after this write to SYS_PMBLIMITR_EL1?
> 
> Because it is at the very end of arm_spe_pmu_begin_discard(), the function
> return and any subsequent conditional branches in the caller will execute
> before the pipeline state is synchronized.
> 

The register write orderings are identical to the old 
arm_spe_perf_aux_output_begin(). So this would be an existing issue if 
it even was an issue.