linux-kernel - Re: [PATCH] perf cs-etm: Fix missing decoder for per-process trace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <71403f39-5894-154d-022a-f42fb344e488@arm.com>
Date:   Tue, 26 Sep 2023 11:56:37 +0100
From:   James Clark <james.clark@....com>
To:     Besar Wicaksono <bwicaksono@...dia.com>, mike.leach@...aro.org,
        suzuki.poulose@....com
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        coresight@...ts.linaro.org, linux-tegra@...r.kernel.org,
        treding@...dia.com, jonathanh@...dia.com, vsethi@...dia.com,
        rwiley@...dia.com, ywan@...dia.com
Subject: Re: [PATCH] perf cs-etm: Fix missing decoder for per-process trace

On 19/09/2023 23:45, Besar Wicaksono wrote:
> The decoder creation for raw trace uses metadata from the first CPU.
> On per-process/per-thread traces, the first CPU is CPU0. If CPU0 trace
> is not enabled, its metadata will be marked unused and the decoder is
> not created. Perf report dump skips the decoding part because the
> decoder is missing.
> 

Hi Besar,

It's not just per-process trace, the bug is also in per-cpu mode but it
means that the metadata from CPU 0 is used for every decoder which is
wrong. Although your change also fixes this issue.

> To fix this, use metadata of the CPU associated with sample object.
> 
> Signed-off-by: Besar Wicaksono <bwicaksono@...dia.com>
> ---
>  tools/perf/util/cs-etm.c | 130 +++++++++++++++++++++++----------------
>  1 file changed, 77 insertions(+), 53 deletions(-)
> 

[...]

> +	if (!formatted) {
> +		/*
> +		 * There is only one decoder when unformatted. Use metadata of
> +		 * sample AUX cpu.
> +		 */
> +		t_param = t_params;
> +		metadata = get_cpu_data(etm, sample_cpu);
> +		if (!metadata) {
> +			pr_err("CS_ETM: invalid sample CPU: %d\n", sample_cpu);
>  			return -EINVAL;
>  		}

Apart from Mike's comments, this looks ok. Thanks for fixing this it has
been on our list for a while.

One issue with calling get_cpu_data() with the sample CPU ID is that it
won't work with old files that don't have the CPU sample flag set. Mike
added the sample flag fairly recently, and I don't think that was a
breaking change for old files at that time. It should be easy to avoid
that by still returning the metadata from CPU 0 when CPU = -1 (Which
isn't correct but is 99% likely to work).

I checked the Coresight tests and they're all passing, at least on a
system without ETE. If you could make sure they're all passing for you
as well that would be great:

  sudo ./perf test coresight

I think they currently only work from an in source build, if you get
stuck there.

Thanks
James