Re: [PATCH v2 19/23] coresight: trbe: Reload the AUX buffer on overflow restart

[James Clark]

On 11/11/2026 17:46, Leo Yan wrote:
> On Tue, Nov 10, 2026 at 03:12:06PM +0000, James Clark wrote:
> > On 16/10/2026 17:55, Leo Yan wrote:
> > > PERF_EVENT_IOC_SET_OUTPUT lets events share another event's mapped output
> > > buffer. For AUX events, this also selects the associated AUX area and
> > > sink configuration. Perf normally establishes this sharing during setup.
> > >
> > > Consider the following hypothetical live-redirection sequence, permitted
> > > by the inspected perf-core paths. P is a parent event and C is its
> > > inherited event, with C->parent = P. A and B are mapped AUX destination
> > > buffers owned by separate, disabled cs_etm events in different task
> > > contexts. All three are per-task events restricted to the same CPU, with
> > > compatible output settings. P has no mmap of its own, allowing its output
> > > to be redirected while A and B remain mapped through their owner events.
> > >
> > > 1. P's output is redirected to A. C inherits from P and begins tracing
> > >      on the selected CPU, so its open AUX transaction and TRBE hardware
> > >      both use A. P can be inactive while C is running because their
> > >      events are scheduled with their respective tasks.
> > > 2. A userspace controller calls PERF_EVENT_IOC_SET_OUTPUT on P to select
> > >      B. ring_buffer_attach() calls perf_event_stop(P, 0), which returns for
> > >      inactive P without stopping C. P->rb now selects B, but C's existing
> > >      handle and hardware still use A.
> > > 3. C's next buffer overflow invokes trbe_handle_overflow() from the IRQ
> > >      handler. It saves buf for A, ends that AUX transaction, then calls
> > >      perf_aux_output_begin(handle, C). AUX begin follows C->parent and
> > >      reads P's current rb, rebinding the same handle to B.
> > >
> > > This is a hypothetical API sequence, not a reproduced TRBE failure or
> > > an observed buffer switch in normal perf record operation.
> > >
> > > Without reloading buf, __arm_trbe_enable() calculates the next buffer
> > > using B through the handle, but its workaround and hardware setup still
> > > receive A. If B has never been enabled, its missing CPU association can
> > > cause the normal-offset calculation to dereference NULL. With a snapshot
> > > buffer, hardware may instead keep using A while perf accounts output to B.
> > >
> > > Reload the sink configuration after perf_aux_output_begin() so the buffer
> > > and handle refer to the same AUX transaction. If the buffer is missing or
> > > its CPU association does not match the interrupt's CPU data, truncate the
> > > new transaction and leave TRBE disabled. Truncation requests perf to stop
> > > the event; a subsequent normal restart installs the buffer and its CPU
> > > association through the sink enable path.
> > >
> > > Stopping inherited writers during output redirection remains a separate
> > > perf-core fix.
> >
> > Shouldn't we do that instead of this? It seems much better to fix this in
> > perf core rather than here which is hard to understand and appears to be a
> > bit hacky, and doesn't apply to all users of aux buffers. I would expect
> > that even when using PERF_EVENT_IOC_SET_OUTPUT, the buffer should be
> > consistent between start and stops from the point of view of individual
> > drivers.
>
> This is only a hypothetical scenario. Fixing common issues in perf core
> is right thing to do, but I'd rather not spend time on this until we
> can identify a real-world case.

Can we drop the patch then? Doesn't seem like there's much point in 
fixing a hypothetical when we know the real fix should be elsewhere.

> > It makes sense to add a workaround in the driver if we can't
> > control that behavior, but the code is in the same repo. Maybe we could even
> > just leave this patch out and do the core fix later?
>
> Let's see the simplified flow:
>
>    trbe_handle_overflow() {
>
>      buf = etm_perf_sink_config(handle);
>
>      ...
>
>      perf_aux_output_end(handle, size);
>      event_data = perf_aux_output_begin(handle, event);
>
>      buf = etm_perf_sink_config(handle);
>
>      ...
>    }
>
> The overflow handler ends one AUX transaction and starts another.
> Although perf_aux_output_begin() returns updated event_data,
> the buf pointer still refers to the old buffer, potentially causing
> a mismatch in the hypothetical scenario.
>
> In contrast, Intel PT and SPE use the updated buffer pointer returned
> by perf_aux_output_begin() to program the hardware, so they don't
> have the same issue.

Why not do it the same way? Get the updated pointer from 
perf_aux_output_begin() and if it's different, reprogram the TRBE.

> > Unrelated to this, the sashiko report that says there is an out of bounds
> > read if the output is switched to an ETF buffer seems to be pre-existing.
> > But it's something worth thinking about.
>
> I roughly remembered the comment mentioned a race condtion due to swap
> TRBE and ETF buffer _within_ the same session. I don't think this will
> happen. (I will double check Sashiko comment).

What do you mean by session? Do you mean multiple events owned by the 
same user? AFAIK all you need is access to an ETF event fd and a TRBE 
event fd, anyone can get those two things by opening events and use 
PERF_EVENT_IOC_SET_OUTPUT to connect them together. But it's a 
pre-existing issue.

> Thanks,
> Leo