Re: [PATCH v3 10/14] perf cs-etm: Synthesize exception entries separately from branches

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf cs-etm: missing protocol check in cs_etm_decoder__buffer_exception()
--

commit 1b56f1522800c89c4b421c00a6430a476535232e
Author: Leo Yan <leo.yan@arm.com>

perf cs-etm: Synthesize exception entries separately from branches

This commit synthesizes exception entry samples separately from preceding
instruction ranges by deriving A32 and A64 exception source addresses from
the preferred exception return address and retaining the exception packet in
prev_packet until its destination is resolved.

> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> index ccae10e6cdad5..b9397151748b2 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -514,6 +514,7 @@ cs_etm_decoder__buffer_exception(struct cs_etm_queue *etmq,
>  {
>  	int ret = 0;
>  	struct cs_etm_packet *packet;
> +	u8 last_instr_size = 0;
>  
>  	ret = cs_etm_decoder__buffer_packet(etmq, queue, elem, trace_chan_id,
>  					    CS_ETM_EXCEPTION);
> @@ -524,8 +525,34 @@ cs_etm_decoder__buffer_exception(struct cs_etm_queue *etmq,
>  	packet->exception_number = elem->exception_number;
>  	if (elem->context.el_valid)
>  		packet->el = elem->context.exception_level;
> -	if (elem->excep_ret_addr)
> -		packet->pref_ret_addr = elem->en_addr;
> +
> +	/* RESET has no defined preferred return address. */
> +	if (!elem->excep_ret_addr ||
> +	    packet->exception_number == CS_ETMV4_EXC_RESET)
> +		return ret;
> +
> +	packet->pref_ret_addr = elem->en_addr;
> +
> +	/*
> +	 * T32 calls can be two or four bytes, requiring an instruction read
> +	 * before pref_ret_addr to determine their size. Keep it for the
> +	 * frontend's SVC check and leave start_addr unknown for its fallback.
> +	 */
> +	if (packet->isa != CS_ETM_ISA_A32 && packet->isa != CS_ETM_ISA_A64)
> +		return ret;
> +
> +	/*
> +	 * For A32 and A64, exception calls execute a four-byte SVC, HVC or
> +	 * SMC and save the following PC. Other exceptions use the interrupted
> +	 * or faulting PC, including traps on those instructions.
> +	 *
> +	 * With a shared branch target, no instruction has completed
> +	 * at the target, so last_instr_size remains zero.
> +	 */
> +	if (elem->exception_number == CS_ETMV4_EXC_CALL)
> +		last_instr_size = 4;
> +	packet->start_addr = packet->pref_ret_addr - last_instr_size;
> +	packet->last_instr_size = last_instr_size;

[Severity: Medium]
As reported by Sashiko on v2 [1], does cs_etm_decoder__buffer_exception()
misinterpret exception_number when decoding ETMv3 traces in A32 state?

Unlike cs_etm__is_syscall(), cs_etm__is_async_exception(), and
cs_etm__is_sync_exception() in tools/perf/util/cs-etm.c, which check the
trace protocol before matching exception numbers:

tools/perf/util/cs-etm.c:cs_etm__is_syscall() {
	...
	if (magic == __perf_cs_etmv3_magic)
		if (packet->exception_number == CS_ETMV3_EXC_SVC)
			return true;

	/*
	 * ETMv4 has a single exception number for SVC, SMC and HVC; need to
	 * test the assembly code to identify if it is a SVC instruction.
	 */
	if (magic == __perf_cs_etmv4_magic || magic == __perf_cs_ete_magic) {
		if (packet->exception_number == CS_ETMV4_EXC_CALL &&
		    cs_etm__is_svc_instr(etmq, tidq, packet, end_addr))
			return true;
	}
	...
}

cs_etm_decoder__buffer_exception() unconditionally compares exception_number
against the ETMv4 constants CS_ETMV4_EXC_RESET (0) and CS_ETMV4_EXC_CALL (2).

In ETMv3, PE reset is CS_ETMV3_EXC_PE_RESET (8), SVC is CS_ETMV3_EXC_SVC
(10), and Hypervisor call is CS_ETMV3_EXC_HYP (3), while only
CS_ETMV3_EXC_SMC (2) happens to match CS_ETMV4_EXC_CALL. When decoding an
ETMv3 A32 trace with elem->excep_ret_addr set, SVC and HYP calls leave
last_instr_size as 0 and set packet->start_addr to packet->pref_ret_addr
instead of packet->pref_ret_addr - 4, and PE reset exceptions bypass the
reset fallback check.

Should cs_etm_decoder__buffer_exception() check the decoder protocol (or
corresponding ETMv3 exception numbers) before matching reset and call
exceptions?

[1] https://lore.kernel.org/r/20260923163605.3962C1F000FF@smtp.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-perf_cs_etm_fix_non_taken-v3-0-7629b52ab047@arm.com?part=10