[PATCH v2 0/2] perf: arm_spe: Fix trace disabling for invalid limits

[PATCH v2 0/2] perf: arm_spe: Fix trace disabling for invalid limits

[Leo Yan]

This series fixes trace disabling issues that occur when failed to
calculate a valid limit (E.g., due to buffer overflow or insufficient
space during padding).

The first patch improves arm_spe_perf_aux_output_begin() for failure
handling, ensuring that all failure cases set the PERF_HES_STOPPED
flag.

The second patch applies the profiling buffer disable flow in the
interrupt handler when the PERF_HES_STOPPED flag has been set.

This series has been verified on Orion6 board.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
Changes in v2:
- Added the patch 01.
- Did not rely on irq work for disabling as the disable callback will
  skip operations after PERF_HES_STOPPED flag has been set.
- Link to v1: https://lore.kernel.org/r/20251015-arm_spe_fix_truncated_flag-v1-1-555e328cba05@arm.com

---
Leo Yan (2):
      perf: arm_spe: Correct setting the PERF_HES_STOPPED flag
      perf: arm_spe: Ensure profiling buffer is properly disabled

 drivers/perf/arm_spe_pmu.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)
---
base-commit: e9a6fb0bcdd7609be6969112f3fbfcce3b1d4a7c
change-id: 20251015-arm_spe_fix_truncated_flag-1a5b3620a3ab

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

[PATCH v2 1/2] perf: arm_spe: Correct setting the PERF_HES_STOPPED flag

[Leo Yan]

In arm_spe_perf_aux_output_begin(), if the calculation of limit fails
and arm_spe_pmu_next_off() returns zero, the driver misses to set the
PERF_HES_STOPPED flag for the event.  As a result, hwc->state does not
reflect the latest state, which can mislead subsequent operations.

Validate the limit when exiting the function: if the limit is 0,
that tracing is disabled, set the PERF_HES_STOPPED flag accordingly.

Fixes: d5d9696b0380 ("drivers/perf: Add support for ARMv8.2 Statistical Profiling Extension")
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/perf/arm_spe_pmu.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index fa50645feddadbea5dc1e404f80f62cf5aa96fd4..fc8f908c2c3a270f2d1ae574c2badb1fbcf51484 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,15 +607,19 @@ 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 |= PMBLIMITR_EL1_E;
 	base = (u64)buf->base + PERF_IDX2OFF(handle->head, buf);
 	write_sysreg_s(base, SYS_PMBPTR_EL1);
 
 out_write_limit:
 	write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
+
+	if (!limit)
+		event->hw.state |= PERF_HES_STOPPED;
 }
 
 static void arm_spe_perf_aux_output_end(struct perf_output_handle *handle)

-- 
2.34.1

Re: [PATCH v2 1/2] perf: arm_spe: Correct setting the PERF_HES_STOPPED flag

[Will Deacon]

On Mon, Nov 10, 2025 at 04:28:31PM +0000, Leo Yan wrote:
> In arm_spe_perf_aux_output_begin(), if the calculation of limit fails
> and arm_spe_pmu_next_off() returns zero, the driver misses to set the
> PERF_HES_STOPPED flag for the event.  As a result, hwc->state does not
> reflect the latest state, which can mislead subsequent operations.
> 
> Validate the limit when exiting the function: if the limit is 0,
> that tracing is disabled, set the PERF_HES_STOPPED flag accordingly.
> 
> Fixes: d5d9696b0380 ("drivers/perf: Add support for ARMv8.2 Statistical Profiling Extension")
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>  drivers/perf/arm_spe_pmu.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index fa50645feddadbea5dc1e404f80f62cf5aa96fd4..fc8f908c2c3a270f2d1ae574c2badb1fbcf51484 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,15 +607,19 @@ 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;

Is 'limit == 0' always indicative of an error, even in snapshot mode?

If __arm_spe_pmu_next_off() fails, it will call perf_aux_output_end()
with the TRUNCATED flag set, which should then disable the event
via arm_spe_pmu_del() and update the state there.

Is that not happening?

Will

Re: [PATCH v2 1/2] perf: arm_spe: Correct setting the PERF_HES_STOPPED flag

[Leo Yan]

On Mon, Nov 24, 2025 at 04:14:23PM +0000, Will Deacon wrote:
> On Mon, Nov 10, 2025 at 04:28:31PM +0000, Leo Yan wrote:
> > In arm_spe_perf_aux_output_begin(), if the calculation of limit fails
> > and arm_spe_pmu_next_off() returns zero, the driver misses to set the
> > PERF_HES_STOPPED flag for the event.  As a result, hwc->state does not
> > reflect the latest state, which can mislead subsequent operations.
> > 
> > Validate the limit when exiting the function: if the limit is 0,
> > that tracing is disabled, set the PERF_HES_STOPPED flag accordingly.
> > 
> > Fixes: d5d9696b0380 ("drivers/perf: Add support for ARMv8.2 Statistical Profiling Extension")
> > Signed-off-by: Leo Yan <leo.yan@arm.com>
> > ---
> >  drivers/perf/arm_spe_pmu.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> > index fa50645feddadbea5dc1e404f80f62cf5aa96fd4..fc8f908c2c3a270f2d1ae574c2badb1fbcf51484 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,15 +607,19 @@ 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;
> 
> Is 'limit == 0' always indicative of an error, even in snapshot mode?

Yes, the 'limit' would never be zero unless an error occurs.

> If __arm_spe_pmu_next_off() fails, it will call perf_aux_output_end()
> with the TRUNCATED flag set, which should then disable the event
> via arm_spe_pmu_del() and update the state there.
> 
> Is that not happening?

Correct.  However, this patch is not for the flow you mentioned.

If an error is returned from arm_spe_pmu_next_off(), because hw.state
is not set to PERF_HES_STOPPED, the caller arm_spe_pmu_start() cannot
detect error properly:

    arm_spe_pmu_start()
    {
        ...

        hwc->state = 0;
        arm_spe_perf_aux_output_begin(handle, event); // Miss to set hwc->state
                                                      // for error case.
        if (hwc->state)    // Fail to detect error.
                return;

        reg = arm_spe_event_to_pmsfcr(event);
        write_sysreg_s(reg, SYS_PMSFCR_EL1);

        ...
     }

This patch consolidates arm_spe_perf_aux_output_begin() to always set
the PERF_HES_STOPPED flag for errors, ensuring consistent error
handling.

A side effect is that arm_spe_pmu_del() will skip disabling buffer unit
when "hwc->state==PERF_HES_STOPPED".  This is why we need the patch 02
to explicitly disable the buffer unit in interrupt handler.

Thanks,
Leo

Re: [PATCH v2 1/2] perf: arm_spe: Correct setting the PERF_HES_STOPPED flag

[Leo Yan]

On Mon, Nov 24, 2025 at 06:48:15PM +0000, Leo Yan wrote:

[...]


> > If __arm_spe_pmu_next_off() fails, it will call perf_aux_output_end()
> > with the TRUNCATED flag set, which should then disable the event
> > via arm_spe_pmu_del() and update the state there.
> > 
> > Is that not happening?
> 
> Correct.  However, this patch is not for the flow you mentioned.

"Correct" might not be clear enough - I meant that the flow you
mentioned does happen.

Re: [PATCH v2 1/2] perf: arm_spe: Correct setting the PERF_HES_STOPPED flag

[Will Deacon]

On Mon, Nov 24, 2025 at 06:48:15PM +0000, Leo Yan wrote:
> On Mon, Nov 24, 2025 at 04:14:23PM +0000, Will Deacon wrote:
> > On Mon, Nov 10, 2025 at 04:28:31PM +0000, Leo Yan wrote:
> > > In arm_spe_perf_aux_output_begin(), if the calculation of limit fails
> > > and arm_spe_pmu_next_off() returns zero, the driver misses to set the
> > > PERF_HES_STOPPED flag for the event.  As a result, hwc->state does not
> > > reflect the latest state, which can mislead subsequent operations.
> > > 
> > > Validate the limit when exiting the function: if the limit is 0,
> > > that tracing is disabled, set the PERF_HES_STOPPED flag accordingly.
> > > 
> > > Fixes: d5d9696b0380 ("drivers/perf: Add support for ARMv8.2 Statistical Profiling Extension")
> > > Signed-off-by: Leo Yan <leo.yan@arm.com>
> > > ---
> > >  drivers/perf/arm_spe_pmu.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> > > index fa50645feddadbea5dc1e404f80f62cf5aa96fd4..fc8f908c2c3a270f2d1ae574c2badb1fbcf51484 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,15 +607,19 @@ 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;
> > 
> > Is 'limit == 0' always indicative of an error, even in snapshot mode?
> 
> Yes, the 'limit' would never be zero unless an error occurs.
> 
> > If __arm_spe_pmu_next_off() fails, it will call perf_aux_output_end()
> > with the TRUNCATED flag set, which should then disable the event
> > via arm_spe_pmu_del() and update the state there.
> > 
> > Is that not happening?
> 
> Correct.  However, this patch is not for the flow you mentioned.

How is it not for this flow? You're talking about:

arm_spe_pmu_start
	=> arm_spe_perf_aux_output_begin
		=> arm_spe_pmu_next_off // Returns error

The only way arm_spe_pmu_next_off() returns an error is if
__arm_spe_pmu_next_off() fails, and that's the flow I'm talking about.

> If an error is returned from arm_spe_pmu_next_off(), because hw.state
> is not set to PERF_HES_STOPPED, the caller arm_spe_pmu_start() cannot
> detect error properly:

But why isn't PERF_HES_STOPPED set by the sequence I described?

I have a feeling you're right, but I can't piece it together from the
information here.

Will

Re: [PATCH v2 1/2] perf: arm_spe: Correct setting the PERF_HES_STOPPED flag

[Leo Yan]

On Mon, Nov 24, 2025 at 07:02:06PM +0000, Will Deacon wrote:

[...]

> > > If __arm_spe_pmu_next_off() fails, it will call perf_aux_output_end()
> > > with the TRUNCATED flag set, which should then disable the event
> > > via arm_spe_pmu_del() and update the state there.
> > > 
> > > Is that not happening?
> > 
> > Correct.  However, this patch is not for the flow you mentioned.
> 
> How is it not for this flow? You're talking about:
> 
> arm_spe_pmu_start
> 	=> arm_spe_perf_aux_output_begin
> 		=> arm_spe_pmu_next_off // Returns error
> 
> The only way arm_spe_pmu_next_off() returns an error is if
> __arm_spe_pmu_next_off() fails, and that's the flow I'm talking about.

My bad.  Because you mentioned the TRUNCATED flag, I incorrectly assumed
it had to be used in interrupt handler with the disable irq work.

> > If an error is returned from arm_spe_pmu_next_off(), because hw.state
> > is not set to PERF_HES_STOPPED, the caller arm_spe_pmu_start() cannot
> > detect error properly:
> 
> But why isn't PERF_HES_STOPPED set by the sequence I described?

Fair point.  I can confirm after settting the TRUNCATED flag,
arm_spe_pmu_del() will be invoked to disable the trace unit and state
will be updated to PERF_HES_STOPPED.

> I have a feeling you're right, but I can't piece it together from the
> information here.

Let me explain in another way:

The issue is a mismatch between the state machine and the hardware
state.  When arm_spe_perf_aux_output_begin() detects an error and does
not set PMBLIMITR_EL1_E, the trace unit is effectively stopped, but
the state machine is not updated to PERF_HES_STOPPED. This causes
callers to handle errors incorrectly [1][2].

It is arguable that the disable IRQ work will eventually disable the
trace unit and update hw.state, but the state should be updated in the
first place by the PMU driver to notify even core layer.

Thanks,
Leo

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/arm_spe_pmu.c?//h=v6.18-rc7#n855
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/events/core.c#n2742

[PATCH v2 2/2] perf: arm_spe: Ensure profiling buffer is properly disabled

[Leo Yan]

During interrupt handling, if arm_spe_perf_aux_output_begin() fails to
calculate a valid limit, it queues the disable callback in the IRQ work
and sets the PERF_HES_STOPPED flag.

Afterwards, the disable callback arm_spe_pmu_stop() is invoked, but
since the PERF_HES_STOPPED flag has already been set, arm_spe_pmu_stop()
exits immediately. As a result, the profiling buffer is not properly
stopped — profiling on exception levels is not disabled, and buffered
data is not drained.

To fix this, explicitly disable the profiling buffer for stopped events
in the interrupt handler.

Fixes: d5d9696b0380 ("drivers/perf: Add support for ARMv8.2 Statistical Profiling Extension")
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/perf/arm_spe_pmu.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index fc8f908c2c3a270f2d1ae574c2badb1fbcf51484..4f51510d5f8a660c12e3934eae8a66f1a1416617 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -761,6 +761,20 @@ 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();
+
+			/*
+			 * A non-zero state indicates that an error occurred in
+			 * arm_spe_perf_aux_output_begin(), for example, if the
+			 * buffer overflowed and failed to get a valid limit.
+			 *
+			 * Since the PERF_HES_STOPPED flag has been set, the
+			 * afterwards disable callback exits immediately and
+			 * the buffer disable flow is skipped (see
+			 * arm_spe_pmu_stop()). Explicitly disable the profiling
+			 * buffer here.
+			 */
+			if (event->hw.state)
+				arm_spe_pmu_disable_and_drain_local();
 		}
 		break;
 	case SPE_PMU_BUF_FAULT_ACT_SPURIOUS:

-- 
2.34.1