Re: [PATCH v4 07/19] perf test cs-etm: Remove asm_pure_loop test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Removing `tidq->prev_packet_thread` causes samples belonging to `prev_packet` to be attributed to the wrong thread if a context packet was processed in between.
--

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

perf cs-etm: Queue context packets for frontend

PE_CONTEXT elements update the context ID and exception level, but the
decoder may still have prior packets cached for frontend processing.
Updating the context immediately in the decoder backend can make those
cached packets get consumed with the wrong thread or EL state. Add a
CS_ETM_CONTEXT packet carrying the TID and EL to the frontend.

Closes: 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..5e92359f51a7c 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c

[ ... ]

> @@ -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]
If a context packet was processed just before this, frontend_thread has
already been updated to the new thread. Does using frontend_thread here
instead of tracking the thread that actually executed prev_packet cause the
branch sample to be attributed to the wrong thread?

>  	sample.addr = cs_etm__first_executed_instr(tidq->packet);
>  	sample.id = etmq->etm->branches_id;
>  	sample.stream_id = etmq->etm->branches_id;

[ ... ]

> @@ -1959,7 +1993,7 @@ static int cs_etm__flush(struct cs_etm_queue *etmq,
>  		addr = cs_etm__last_executed_instr(tidq->prev_packet);
>  
>  		err = cs_etm__synth_instruction_sample(
> -			etmq, tidq, addr,
> +			etmq, tidq, tidq->prev_packet, addr,
>  			tidq->period_instructions);

[Severity: Medium]
When flushing the trace, an instruction sample is generated for prev_packet.
However, cs_etm__synth_instruction_sample unconditionally uses
frontend_thread. If a context packet was the last processed packet, could
this lead to misattribution of the sample to the new thread?

>  		if (err)
>  			return err;

[ ... ]

> @@ -2469,6 +2501,19 @@ static int cs_etm__process_traceid_queue(struct cs_etm_queue *etmq,
>  			 */
>  			cs_etm__sample(etmq, tidq);
>  			break;
> +		case CS_ETM_CONTEXT:
> +			/*
> +			 * Update context but don't swap packet. Keep the
> +			 * previous one for branch source address info, if
> +			 * tracing the kernel the context packet will be emitted
> +			 * between two ranges.
> +			 */
> +			ret = cs_etm__etmq_update_thread(etmq, tidq->packet->el,
> +							 tidq->packet->tid,
> +							 &tidq->frontend_thread);

[Severity: Medium]
When CS_ETM_CONTEXT updates frontend_thread without swapping packets, won't
prev_packet still contain the execution range of the old thread? Since the
frontend_thread is updated immediately, could any subsequent samples
generated for prev_packet be attributed to the wrong thread?

> +			if (ret)
> +				goto out;
> +			break;
>  		case CS_ETM_EXCEPTION:
>  		case CS_ETM_EXCEPTION_RET:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-james-cs-context-tracking-fix-v4-0-44f9fb9e5c42@linaro.org?part=7