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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] htm_info_fill() and htm_nr_events() only record the mapping data for the first CPU in a multi-CPU evsel map, causing AUX buffer decoding to fail for all other CPUs.
- [High] The aux_watermark calculation can silently overflow a 32-bit integer when large AUX buffer sizes are requested, corrupting the synchronization watermark.
- [Low] Missing explicit inclusion of POSIX <string.h> header for strcmp() usage, violating musl libc compatibility guidelines.
--

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

tools/perf: Add AUXTRACE recording support for powerpc HTM

Add powerpc perf tool support for the HTM PMU AUXTRACE recording path.
Introduce htm_recording_init() and the associated auxtrace callbacks so
perf record can create PERF_RECORD_AUXTRACE records for HTM data.

> diff --git a/tools/perf/arch/powerpc/util/auxtrace.c b/tools/perf/arch/powerpc/util/auxtrace.c
> index e04a0bd617557..c77ffc4c6a9d7 100644
> --- a/tools/perf/arch/powerpc/util/auxtrace.c
> +++ b/tools/perf/arch/powerpc/util/auxtrace.c
[ ... ]
> @@ -32,11 +34,30 @@ 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 (!strcmp(evsel__pmu_name(pos), "htm")) {

[Severity: Low]
Should the POSIX <string.h> header be explicitly included in this file
to provide the declaration for strcmp()?

> +			pos->needs_auxtrace_mmap = true;
> +			/* Remember the first matching HTM event */
> +			if (!htm_evsel)
> +				htm_evsel = pos;
> +		}
>  	}
>  

> diff --git a/tools/perf/arch/powerpc/util/htm.c b/tools/perf/arch/powerpc/util/htm.c
> new file mode 100644
> index 0000000000000..7757f548ef22c
> --- /dev/null
> +++ b/tools/perf/arch/powerpc/util/htm.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * HTM AUX tracing support
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/string.h>

[Severity: Low]
Should the POSIX <string.h> header be explicitly included here to avoid
relying on implicit inclusion via the kernel wrapper for musl compatibility?

> +#include <linux/zalloc.h>
[ ... ]
> +	evlist__for_each_entry(evlist, pos) {
> +		if (strcmp(evsel__pmu_name(pos), "htm"))
> +			continue;
> +		pos->core.attr.aux_watermark =
> +			opts->auxtrace_mmap_pages * (size_t)page_size / 2;

[Severity: High]
Can this calculation silently overflow when large AUX buffer sizes are
requested?

The result of this 64-bit size calculation is assigned directly to the 32-bit
aux_watermark field without being clamped to UINT_MAX.

> +		pos->core.attr.sample_type |= PERF_SAMPLE_RAW;
> +		pos->core.attr.freq = 0;
[ ... ]
> +	evlist__for_each_entry(evlist, pos) {
> +		struct perf_cpu_map *cpus;
> +		int cpu;
> +
> +		if (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]
Does this logic silently discard the configuration mapping for all CPUs
in the map beyond the 0th index?

This function explicitly extracts only perf_cpu_map__cpu(cpus, 0). Earlier in
htm_nr_events(), the array is sized based on the number of evsels rather than
the total number of CPUs:

tools/perf/arch/powerpc/util/htm.c:htm_nr_events() {
    ...
	evlist__for_each_entry(evlist, pos) {
		if (!strcmp(evsel__pmu_name(pos), "htm"))
			n++;
	}
	return n;
}

Are multi-CPU mappings missing the necessary space and data to be fully
recorded?

> +		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++;
> +	}

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