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.1On 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
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
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
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
On Tue, Nov 25, 2025 at 02:20:36PM +0000, Leo Yan wrote: > 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. From what I can tell, perf_aux_output_end() will call perf_event_disable_inatomic() which should end up invoking perf_pending_disable() via an IPI-to-self to disable the event and put it in the PERF_HES_STOPPED state before we return to userspace. So I still struggle to see the problem here. Will
On Thu, Jan 08, 2026 at 04:23:58PM +0000, Will Deacon wrote:
[...]
> > > 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.
[...]
> > 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.
>
> From what I can tell, perf_aux_output_end() will call
> perf_event_disable_inatomic() which should end up invoking
> perf_pending_disable() via an IPI-to-self to disable the event and put
> it in the PERF_HES_STOPPED state before we return to userspace.
>
> So I still struggle to see the problem here.
The issue is that the SPE driver does not properly propagate errors when
arm_spe_pmu_next_off() fails. Instead, it behaves as if tracing was
enabled successfully, which leads to redundant operations and an
inconsistent state in the perf core.
Let us dig a bit.
arm_spe_pmu_start()
{
hwc->state = 0;
/* Fails inside arm_spe_pmu_next_off() */
arm_spe_perf_aux_output_begin(handle, event);
/* hwc->state remains 0, so execution continues */
if (hwc->state)
return;
reg = arm_spe_event_to_pmsfcr(event);
write_sysreg_s(reg, SYS_PMSFCR_EL1);
...
}
In arm_spe_pmu_start(), a failure in arm_spe_perf_aux_output_begin()
does not set PERF_HES_STOPPED, so hwc->state remains zero and the
function continues to program filters even though has failed.
Moveover, the driver still returns success to the perf core. As a
result, event_sched_in() assumes the event was started correctly and
proceeds to enable other events.
event_sched_in()
{
...
if (event->pmu->add(event, PERF_EF_START)) {
perf_event_set_state(event, PERF_EVENT_STATE_INACTIVE);
event->oncpu = -1;
ret = -EAGAIN;
goto out;
}
...
}
This breaks event group case, for example:
perf record -e '{cs_etm//,cycles}' -- test
The perf core expects all events in a group to start and stop together,
but the SPE driver's incorrect reporting causes misalignment.
Sorry for late reply.
Thanks,
Leo
On Wed, Jan 14, 2026 at 05:52:40PM +0000, Leo Yan wrote:
> > > 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.
> >
> > From what I can tell, perf_aux_output_end() will call
> > perf_event_disable_inatomic() which should end up invoking
> > perf_pending_disable() via an IPI-to-self to disable the event and put
> > it in the PERF_HES_STOPPED state before we return to userspace.
> >
> > So I still struggle to see the problem here.
>
> The issue is that the SPE driver does not properly propagate errors when
> arm_spe_pmu_next_off() fails. Instead, it behaves as if tracing was
> enabled successfully, which leads to redundant operations and an
> inconsistent state in the perf core.
>
> Let us dig a bit.
>
> arm_spe_pmu_start()
> {
> hwc->state = 0;
>
> /* Fails inside arm_spe_pmu_next_off() */
> arm_spe_perf_aux_output_begin(handle, event);
>
> /* hwc->state remains 0, so execution continues */
> if (hwc->state)
> return;
>
> reg = arm_spe_event_to_pmsfcr(event);
> write_sysreg_s(reg, SYS_PMSFCR_EL1);
> ...
> }
>
> In arm_spe_pmu_start(), a failure in arm_spe_perf_aux_output_begin()
> does not set PERF_HES_STOPPED, so hwc->state remains zero and the
> function continues to program filters even though has failed.
>
> Moveover, the driver still returns success to the perf core. As a
> result, event_sched_in() assumes the event was started correctly and
> proceeds to enable other events.
>
> event_sched_in()
> {
> ...
>
> if (event->pmu->add(event, PERF_EF_START)) {
> perf_event_set_state(event, PERF_EVENT_STATE_INACTIVE);
> event->oncpu = -1;
> ret = -EAGAIN;
> goto out;
> }
>
> ...
> }
>
> This breaks event group case, for example:
>
> perf record -e '{cs_etm//,cycles}' -- test
>
> The perf core expects all events in a group to start and stop together,
> but the SPE driver's incorrect reporting causes misalignment.
Ok, so looking at this and the next patch I wonder if we could simplify
things a little by having arm_spe_perf_aux_output_begin() return an 'int'
to indicate success/failure instead of touching 'hwc->state'.
Then arm_spe_pmu_start() and the interrupt handler could call into
arm_spe_pmu_stop() if they get an error code back. Would that work?
Will
On Tue, Jan 20, 2026 at 04:42:39PM +0000, Will Deacon wrote: [...] > Ok, so looking at this and the next patch I wonder if we could simplify > things a little by having arm_spe_perf_aux_output_begin() return an 'int' > to indicate success/failure instead of touching 'hwc->state'. > > Then arm_spe_pmu_start() and the interrupt handler could call into > arm_spe_pmu_stop() if they get an error code back. Would that work? Yes, your suggested approach is cleaner. I will respin patches. Thanks for the suggestions! Leo
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.
© 2016 - 2026 Red Hat, Inc.