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

[James Clark <james.clark@linaro.org>]

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