[PATCH] perf: arm_spe: Ensure trace is disabled when TRUNCATED flag is set

[PATCH] perf: arm_spe: Ensure trace is disabled when TRUNCATED flag is set

[Leo Yan]

The TRUNCATED flag can be set in the following cases:

 1. When data loss is detected (PMBSR_EL1.DL == 0b1).
 2. When arm_spe_perf_aux_output_begin() fails to find a valid limit
    because it runs out of free space.

Currently, only the first case invokes irq_work_run() to execute the
event disable callback, while the second case does not.

Move the call to irq_work_run() later with checking the TRUNCATED flag,
so that both cases are handled and redundant calls are avoided.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/perf/arm_spe_pmu.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index fa50645feddadbea5dc1e404f80f62cf5aa96fd4..05ba977f75d607a1d524f68694db9ea18a6ef20c 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -731,12 +731,6 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
 	if (act == SPE_PMU_BUF_FAULT_ACT_SPURIOUS)
 		return IRQ_NONE;
 
-	/*
-	 * Ensure perf callbacks have completed, which may disable the
-	 * profiling buffer in response to a TRUNCATION flag.
-	 */
-	irq_work_run();
-
 	switch (act) {
 	case SPE_PMU_BUF_FAULT_ACT_FATAL:
 		/*
@@ -765,6 +759,15 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
 		break;
 	}
 
+	/*
+	 * The TRUNCATED flag is set when data loss is detected by PMBSR_EL1.DL,
+	 * or arm_spe_perf_aux_output_begin() sets the flag if runs out of free
+	 * space. Ensure that all perf callbacks have completed for disabling
+	 * the profiling buffer.
+	 */
+	if (handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)
+		irq_work_run();
+
 	/* The buffer pointers are now sane, so resume profiling. */
 	write_sysreg_s(0, SYS_PMBSR_EL1);
 	return IRQ_HANDLED;

---
base-commit: 1f4a222b0e334540343fbb5d3eac4584a6bfe180
change-id: 20251015-arm_spe_fix_truncated_flag-1a5b3620a3ab

Best regards,
-- 
Leo Yan <leo.yan@arm.com>

Re: [PATCH] perf: arm_spe: Ensure trace is disabled when TRUNCATED flag is set

[Will Deacon]

On Wed, Oct 15, 2025 at 07:23:37PM +0100, Leo Yan wrote:
> The TRUNCATED flag can be set in the following cases:
> 
>  1. When data loss is detected (PMBSR_EL1.DL == 0b1).
>  2. When arm_spe_perf_aux_output_begin() fails to find a valid limit
>     because it runs out of free space.
> 
> Currently, only the first case invokes irq_work_run() to execute the
> event disable callback, while the second case does not.
> 
> Move the call to irq_work_run() later with checking the TRUNCATED flag,
> so that both cases are handled and redundant calls are avoided.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>  drivers/perf/arm_spe_pmu.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index fa50645feddadbea5dc1e404f80f62cf5aa96fd4..05ba977f75d607a1d524f68694db9ea18a6ef20c 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -731,12 +731,6 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
>  	if (act == SPE_PMU_BUF_FAULT_ACT_SPURIOUS)
>  		return IRQ_NONE;
>  
> -	/*
> -	 * Ensure perf callbacks have completed, which may disable the
> -	 * profiling buffer in response to a TRUNCATION flag.
> -	 */
> -	irq_work_run();
> -
>  	switch (act) {
>  	case SPE_PMU_BUF_FAULT_ACT_FATAL:
>  		/*
> @@ -765,6 +759,15 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
>  		break;
>  	}
>  
> +	/*
> +	 * The TRUNCATED flag is set when data loss is detected by PMBSR_EL1.DL,
> +	 * or arm_spe_perf_aux_output_begin() sets the flag if runs out of free
> +	 * space. Ensure that all perf callbacks have completed for disabling
> +	 * the profiling buffer.
> +	 */
> +	if (handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)
> +		irq_work_run();

I'm not sure about this. afaict, perf_aux_output_begin() can fail
(return NULL) without setting the TRUNCATED flag but with a disable
pending in irq work. It also feels a little fragile moving the existing
irq_work_run() call as that could easily break in future if e.g. irq
work is used for other manipulation of the buffer controls.

Given that this isn't a fast path, isn't it simpler and safer just to
do something like the below?

Will

--->8

diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index fa50645fedda..8a38d69e9600 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -758,6 +758,10 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
                if (!(handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)) {
                        arm_spe_perf_aux_output_begin(handle, event);
                        isb();
+
+                       /* Insightful comment here */
+                       if (event->hw.state)
+                               irq_work_run();
                }
                break;
        case SPE_PMU_BUF_FAULT_ACT_SPURIOUS:

Re: [PATCH] perf: arm_spe: Ensure trace is disabled when TRUNCATED flag is set

[Leo Yan]

On Mon, Nov 03, 2025 at 02:14:56PM +0000, Will Deacon wrote:

[...]

> > @@ -765,6 +759,15 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
> >  		break;
> >  	}
> >  
> > +	/*
> > +	 * The TRUNCATED flag is set when data loss is detected by PMBSR_EL1.DL,
> > +	 * or arm_spe_perf_aux_output_begin() sets the flag if runs out of free
> > +	 * space. Ensure that all perf callbacks have completed for disabling
> > +	 * the profiling buffer.
> > +	 */
> > +	if (handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)
> > +		irq_work_run();
> 
> I'm not sure about this. afaict, perf_aux_output_begin() can fail
> (return NULL) without setting the TRUNCATED flag but with a disable
> pending in irq work. It also feels a little fragile moving the existing
> irq_work_run() call as that could easily break in future if e.g. irq
> work is used for other manipulation of the buffer controls.
> 
> Given that this isn't a fast path, isn't it simpler and safer just to
> do something like the below?

This is fine for me.  However, arm_spe_perf_aux_output_begin() misses to
set the 'hw.state' if arm_spe_pmu_next_off() fails to calculate a valid
limit. We need an extra fix for this (see the pasted code piece).

As a result, we will have two fixes:

  1) Fix 'hw.state' setting in arm_spe_perf_aux_output_begin();
  2) Invoke irq_work_run() if 'hw.state' is non-zero.

Please let me know if this works for you.  Thanks!

Leo

diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index fa50645fedda..ff7b1447db0a 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -597,7 +597,6 @@ static void arm_spe_perf_aux_output_begin(struct perf_output_handle *handle,
 	/* Start a new aux session */
 	buf = perf_aux_output_begin(handle, event);
 	if (!buf) {
-		event->hw.state |= PERF_HES_STOPPED;
 		/*
 		 * We still need to clear the limit pointer, since the
 		 * profiler might only be disabled by virtue of a fault.
@@ -608,14 +607,18 @@ static void arm_spe_perf_aux_output_begin(struct perf_output_handle *handle,
 
 	limit = buf->snapshot ? arm_spe_pmu_next_snapshot_off(handle)
 			      : arm_spe_pmu_next_off(handle);
-	if (limit)
-		limit |= PMBLIMITR_EL1_E;
+	if (!limit)
+		goto out_write_limit;
 
-	limit += (u64)buf->base;
+	limit |= (u64)buf->base | PMBLIMITR_EL1_E;
 	base = (u64)buf->base + PERF_IDX2OFF(handle->head, buf);
 	write_sysreg_s(base, SYS_PMBPTR_EL1);
 
 out_write_limit:
+	/* Stop tracing due to an invalid limit pointer */
+	if (!limit)
+		event->hw.state |= PERF_HES_STOPPED;
 	write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
 }
 

> Will
> 
> --->8
> 
> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index fa50645fedda..8a38d69e9600 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -758,6 +758,10 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
>                 if (!(handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)) {
>                         arm_spe_perf_aux_output_begin(handle, event);
>                         isb();
> +
> +                       /* Insightful comment here */
> +                       if (event->hw.state)
> +                               irq_work_run();
>                 }
>                 break;
>         case SPE_PMU_BUF_FAULT_ACT_SPURIOUS:
>