Re: [PATCH v3 01/19] perf cs-etm: Queue context packets for frontend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Removing `prev_packet_thread` breaks branch sample cross-context instruction decoding when transitions cross machine boundaries (e.g., hypervisor to guest).
- [Medium] `cs_etm__synth_instruction_sample()` ignores its new `packet` argument and hardcodes `tidq->packet`, causing instruction copying to fail during trace flush.
--

commit 588f2ae34f926c74679fbf78b98515da3192e351
Author: James Clark <james.clark@linaro.org>

perf cs-etm: Queue context packets for frontend

This commit adds a CS_ETM_CONTEXT packet carrying the TID and exception
level to the frontend, keeping context changes ordered with the packet stream.
It separates the memory access functions for the frontend and decoder backend,
and removes tracking for the previous packet's thread.

Link: https://lore.kernel.org/linux-perf-users/20260515021135.1729028-1-aaupov@meta.com/

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 40c6ddfa8c8d9..ce570913669c1 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c

[ ... ]

> @@ -1158,6 +1151,30 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
>  	return ret;
>  }
>  
> +static u32 cs_etm__frontend_mem_access(struct cs_etm_queue *etmq,
> +				       struct cs_etm_traceid_queue *tidq,
> +				       struct cs_etm_packet *packet,
> +				       u64 address, size_t size, u8 *buffer)
> +{
> +	return __cs_etm__mem_access(etmq, address, size, buffer, 0, packet->el,
> +				    tidq->frontend_thread);
> +}

[Severity: Medium]
Will this mix the previous packet's exception level with the newly updated
frontend thread?

When tracing virtualized environments, an exception return from a hypervisor
to a guest causes a machine context change. The decoder emits a context
packet which updates the frontend thread to the guest thread before
generating the branch sample for the hypervisor's execution range.

By passing the newly updated frontend thread to the memory access function
while using the previous packet's exception level, the map lookup could
look up the host kernel address in the guest's map group, causing the
lookup to fail and result in missing instruction bytes.

[ ... ]

> @@ -1579,6 +1614,7 @@ static inline u64 cs_etm__resolve_sample_time(struct cs_etm_queue *etmq,
>  
>  static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
>  					    struct cs_etm_traceid_queue *tidq,
> +					    struct cs_etm_packet *packet,
>  					    u64 addr, u64 period)
>  {
>  	int ret = 0;

[ ... ]

> @@ -1604,7 +1640,7 @@ static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
>  	sample.cpu = tidq->packet->cpu;
>  	sample.flags = tidq->prev_packet->flags;
>  	sample.cpumode = event->sample.header.misc;
>  
> -	cs_etm__copy_insn(etmq, tidq->trace_chan_id, tidq->packet, &sample);
> +	cs_etm__copy_insn(etmq, tidq, tidq->packet, &sample);

[Severity: Medium]
Does this ignore the newly added packet argument?

The function signature was updated to accept a packet argument so it can
process the previous packet during a flush. However, the function body
still hardcodes tidq->packet for sample.cpu and in the call to
cs_etm__copy_insn().

When called from a flush, tidq->packet is typically a discontinuity or
empty packet. This could cause cs_etm__copy_insn() to bail out early and
silently drop the instruction bytes for the flushed sample.

[ ... ]

> @@ -1649,15 +1685,15 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq,
>  
>  	event->sample.header.type = PERF_RECORD_SAMPLE;
>  	event->sample.header.misc = cs_etm__cpu_mode(etmq, ip,
> -						     tidq->prev_packet_el);
> +						     tidq->prev_packet->el);
>  	event->sample.header.size = sizeof(struct perf_event_header);
>  
>  	/* Set time field based on etm auxtrace config. */
>  	sample.time = cs_etm__resolve_sample_time(etmq, tidq);
>  
>  	sample.ip = ip;
> -	sample.pid = thread__pid(tidq->prev_packet_thread);
> -	sample.tid = thread__tid(tidq->prev_packet_thread);
> +	sample.pid = thread__pid(tidq->frontend_thread);
> +	sample.tid = thread__tid(tidq->frontend_thread);

[Severity: Medium]
Does the removal of the previous packet thread tracking break cross-context
branch samples?

By using the newly updated frontend thread instead of the previous packet
thread here, it appears a branch sample from the previous context could be
misattributed to the new thread when transitions cross machine boundaries.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-james-cs-context-tracking-fix-v3-0-c392945d9ed5@linaro.org?part=1