Re: [PATCH V3 2/6] tools/perf: Add AUXTRACE recording support for powerpc HTM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Concurrent use of VPA DTL and HTM auxtrace events silently drops HTM initialization, producing corrupted trace data.
- [High] System-wide and multi-CPU profiles fail to record CPU mappings for all but the first CPU, causing decoding failures.
- [Medium] Missing explicit `<string.h>` inclusion for `strcmp()` breaks musl libc compilation.
- [Medium] Redundant `pos->name` NULL checks incorrectly skip valid unnamed HTM events.
--

--- Patch [2]: [PATCH V3 2/6] tools/perf: Add AUXTRACE recording support for powerpc HTM ---
commit 00980957c6d482ec39e6aa18c32409446efcd20e
Author: Athira Rajeev <atrajeev@linux.ibm.com>

tools/perf: Add AUXTRACE recording support for powerpc HTM

This commit adds powerpc perf tool support for the HTM PMU AUXTRACE
recording path. It introduces initialization and callbacks to create
PERF_RECORD_AUXTRACE records for HTM data, populating mapping information
for decoding.

> diff --git a/tools/perf/arch/powerpc/util/auxtrace.c b/tools/perf/arch/powerpc/util/auxtrace.c
> index e04a0bd617557..74f0a37236b8f 100644
> --- a/tools/perf/arch/powerpc/util/auxtrace.c
> +++ b/tools/perf/arch/powerpc/util/auxtrace.c

[ ... ]

> @@ -32,11 +34,18 @@ struct auxtrace_record *auxtrace_record__init(struct evlist *evlist,
>  			/* Remember the first matching VPA DTL event */
>  			if (!vpa_dtl_evsel)
>  				vpa_dtl_evsel = pos;
> +		} else if (pos->name && (!strcmp(evsel__pmu_name(pos), "htm"))) {

[Severity: Medium]
Since evsel__pmu_name() accurately identifies the PMU name regardless of
pos->name, does the pos->name check silently ignore valid HTM evsels that
might be created dynamically and lack a name?

Also, this code calls strcmp() but the file doesn't explicitly include
<string.h>. Will this break musl libc builds that strictly enforce POSIX
header inclusions without implicit dependencies?

> +			pos->needs_auxtrace_mmap = true;
> +			/* Remember the first matching HTM event */
> +			if (!htm_evsel)
> +				htm_evsel = pos;
>  		}
>  	}
>  
>  	if (vpa_dtl_evsel)
>  		return vpa_dtl_recording_init(vpa_dtl_evsel, err);
> +	else if (htm_evsel)
> +		return htm_recording_init(htm_evsel, err);

[Severity: High]
What happens if a user requests both VPA DTL and HTM auxtrace PMUs
concurrently?

It looks like the code will initialize VPA DTL and return immediately,
completely skipping HTM initialization. Since both had needs_auxtrace_mmap
set to true, will this result in HTM aux buffers being collected without
writing the required PERF_RECORD_AUXTRACE_INFO metadata, causing decoding
failures?

Should this explicitly reject concurrent auxtrace PMUs instead?

>  	return NULL;
>  }

> diff --git a/tools/perf/arch/powerpc/util/htm.c b/tools/perf/arch/powerpc/util/htm.c
> new file mode 100644
> index 0000000000000..9a12a8fe8027f
> --- /dev/null
> +++ b/tools/perf/arch/powerpc/util/htm.c

[ ... ]

> +static int
> +htm_recording_options(struct auxtrace_record *itr __maybe_unused,
> +		      struct evlist *evlist,
> +		      struct record_opts *opts)
> +{
> +	struct evsel *pos;
> +
> +	opts->full_auxtrace = true;
> +
> +	if (!opts->auxtrace_mmap_pages) {
> +		opts->auxtrace_mmap_pages = KiB(128) / page_size;
> +		if (opts->mmap_pages == UINT_MAX)
> +			opts->mmap_pages = KiB(256) / page_size;
> +	}
> +
> +	evlist__for_each_entry(evlist, pos) {
> +		if (!pos->name || (strcmp(evsel__pmu_name(pos), "htm")))
> +			continue;

[Severity: Medium]
Similar to auxtrace_record__init(), does the pos->name check unnecessarily
skip unnamed events?

Additionally, strcmp() is used here without explicitly including <string.h>.
Could this cause compilation failures on musl libc systems?

> +		pos->core.attr.aux_watermark =
> +			opts->auxtrace_mmap_pages * (size_t)page_size / 2;
> +		pos->core.attr.freq = 0;
> +		pos->core.attr.sample_type |= PERF_SAMPLE_RAW;
> +		pos->needs_auxtrace_mmap = true;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +/* Count htm evsels in the evlist */
> +static int htm_nr_events(struct evlist *evlist)
> +{
> +	struct evsel *pos;
> +	int n = 0;
> +
> +	evlist__for_each_entry(evlist, pos) {
> +		if (pos->name && (!strcmp(evsel__pmu_name(pos), "htm")))
> +			n++;
> +	}
> +	return n;
> +}

[ ... ]

> +static int
> +htm_info_fill(struct auxtrace_record *itr,
> +	      struct perf_session *session,
> +	      struct perf_record_auxtrace_info *auxtrace_info,
> +	      size_t priv_size)
> +{

[ ... ]

> +	evlist__for_each_entry(evlist, pos) {
> +		struct perf_cpu_map *cpus;
> +		int cpu;
> +
> +		if (!pos->name || (strcmp(evsel__pmu_name(pos), "htm")))
> +			continue;
> +
> +		/*
> +		 * Get the CPU this evsel is pinned to.  For events opened
> +		 * with cpu=N, evsel__cpus() returns a single-entry map {N}
> +		 * at record time (not during replay).
> +		 */
> +		cpus = evsel__cpus(pos);
> +		if (cpus && perf_cpu_map__nr(cpus) > 0)
> +			cpu = perf_cpu_map__cpu(cpus, 0).cpu;

[Severity: High]
When an event is opened system-wide or with multiple CPUs, will this loop
miss CPU mappings for all but the first CPU?

By extracting only index 0 from the perf_cpu_map, the resulting metadata
will lack mappings for CPU 1 and beyond. When the auxtrace decoder tries to
decode data from these additional CPUs, will it fail to find the required
event->auxtrace.cpu mapping?

Also, since htm_nr_events() only counts the number of evsels rather than
the total number of CPUs across the maps, would the priv array even be
large enough to hold metadata for all CPUs in a multi-CPU profile?

> +		else
> +			cpu = -1;
> +
> +		auxtrace_info->priv[POWERPC_HTM_EVENT_DATA + n * 2]     = cpu;
> +		auxtrace_info->priv[POWERPC_HTM_EVENT_DATA + n * 2 + 1] =
> +							pos->core.attr.config;
> +		n++;
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725070747.81435-1-atrajeev@linux.ibm.com?part=2