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

[Athira Rajeev <atrajeev@linux.ibm.com>]

> On 12 Aug 2026, at 3:57 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
> On 07/08/2026 17:41, Athira Rajeev wrote:
>> 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.
>> 
>> htm_info_fill() stores the PMU type, number of HTM evsels, and a
>> (cpu, attr.config) pair for each evsel into PERF_RECORD_AUXTRACE_INFO
>> priv[].  The decode side reads these back to map each AUX buffer
>> (identified by event->auxtrace.cpu) to the correct (node, chip, core)
>> target when writing htm.bin.* output files.
>> 
>> Update auxtrace_record__init() to detect HTM events and dispatch to the
>> HTM-specific recording initializer.
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
> 
> A couple of very minor comments.
> 
> Nevertheless:
> 
> Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

Hi Adrian

Thanks for reviewing and sharing comments

I will include changes in the next version.
I will wait for feedback on the remaining patches and post a v6 incorporating all the changes together.

Thanks
Athira

> 
>> ---
>> Changes in V5:
>> - htm_recording_options(): clamp aux_watermark with min_t(size_t, wm,
>>  UINT_MAX) before assigning to pos->core.attr.aux_watermark (u32).
>>  On a system with large auxtrace_mmap_pages the unclamped product can
>>  exceed UINT_MAX and silently truncate on 32-bit platforms.
>> - htm_info_fill(): iterate all CPUs in each evsel's CPU map and write
>>  one (cpu, config) entry per CPU instead of only the first CPU.  An
>>  evsel opened with -C 0,1,2 now contributes three priv[] entries, one
>>  per CPU, each carrying the same attr.config.  The decode side
>>  (powerpc_htm_process_auxtrace_event) uses event->auxtrace.cpu to look
>>  up the matching entry, so every CPU's AUX buffer is correctly mapped
>>  to its (node, chip, core, type) target and written to the right output
>>  file.  An evsel with no CPU map records cpu=-1 as before.
>> 
>> Changes in V4:
>> - Add explicit mutual-exclusion check in auxtrace_record__init(): if
>>  both vpa_dtl_evsel and htm_evsel are set after scanning the evlist,
>>  print an error, set *err = -EINVAL, and return NULL.  V3 silently
>>  fell through to prefer VPA-DTL when both PMU types were present,
>>  leaving HTM AUX buffers collected without a
>>  PERF_RECORD_AUXTRACE_INFO record and the trace undecodable.
>> - Drop the superfluous pos->name && null-guard from the HTM detection
>>  condition in auxtrace_record__init(): the condition becomes
>>  !strcmp(evsel__pmu_name(pos), "htm") without the leading check.
>>  evsel__pmu_name() handles a NULL name internally so the guard is
>>  redundant.
>> - Apply the same simplification in htm_recording_options() and
>>  htm_nr_events() in htm.c: remove the !pos->name || prefix from
>>  the evsel__pmu_name() comparisons, consistent with the dispatcher
>>  change above.
>> - Set pos->core.attr.sample_period = 1 for each HTM evsel inside
>>  htm_recording_options().  V3 set freq=0 and aux_watermark but did
>>  not set sample_period, leaving the kernel to use whatever default
>>  value was in the attr, which could cause the HTM event to fire at
>>  an unintended rate.
>> 
>> Changes in V3:
>> - Switch HTM event detection in auxtrace_record__init() from
>> strstarts(pos->name, "htm") to !strcmp(evsel__pmu_name(pos), "htm"),
>> matching the kernel-assigned PMU name and preventing false
>> matches on user-named events.
>> - Switch htm_recording_options(), htm_nr_events(), and the
>> loop in htm_info_fill() from strstarts(pos->name, "htm")
>> to evsel__pmu_name(), consistent with the above and with
>> patches 4 and 6.
>> - Add #include <linux/zalloc.h> to htm.c for zalloc().
>> - Set auxtrace_info->type = PERF_AUXTRACE_POWERPC_HTM directly
>> in htm_info_fill(), removing the placeholder comment. (V2
>> deferred this to patch 5; it is cleaner to set it in the same
>> patch that defines the enum.)
>> - Set aux_watermark to half the AUX ring-buffer size
>> (auxtrace_mmap_pages * page_size / 2) so there is always
>> headroom for hardware to write while userspace drains. V2 set
>> it to the full buffer size, risking data loss under back-pressure.
>> - Add PERF_AUXTRACE_POWERPC_HTM to the enum auxtrace_type in
>> util/auxtrace.h and add #include "powerpc-htm.h" plus a stub
>> case PERF_AUXTRACE_POWERPC_HTM: (falls through to
>> PERF_AUXTRACE_UNKNOWN) in util/auxtrace.c in this patch, so
>> the enum is defined before it is used in htm_info_fill().
>> (V2 did this in patch 5.)
>> 
>> Changes in V2:
>> - htm_info_fill() now stores a (cpu, attr.config) pair for every HTM
>>  evsel into PERF_RECORD_AUXTRACE_INFO priv[].  V1 stored only the PMU
>>  type and a single config value; there was no per-CPU mapping.
>> - The priv[] layout is formalised in util/powerpc-htm.h with enum
>>  constants POWERPC_HTM_PMU_TYPE, POWERPC_HTM_NUM_EVENTS, and
>>  POWERPC_HTM_EVENT_DATA and the helper macros HTM_AUXTRACE_PRIV_FIXED
>>  and HTM_AUXTRACE_PRIV_SIZE(n).  V1 used bare numeric offsets.
>> - PERF_SAMPLE_RAW is enabled in the recording options so that memory
>>  configuration records emitted by the kernel driver are captured
>>  alongside the AUX stream.  V1 added this in a later patch.
>> - PERF_AUXTRACE_POWERPC_HTM type constant is set in htm_info_fill()
>>  (wired up in patch 5 once the enum is defined).
>> - Patch is now 2/6 instead of 2/9.
>> 
>> tools/perf/arch/powerpc/util/Build      |   1 +
>> tools/perf/arch/powerpc/util/auxtrace.c |  22 +++
>> tools/perf/arch/powerpc/util/htm.c      | 185 ++++++++++++++++++++++++
>> tools/perf/util/auxtrace.c              |   2 +
>> tools/perf/util/auxtrace.h              |   1 +
>> tools/perf/util/powerpc-htm.h           |  43 ++++++
>> 6 files changed, 254 insertions(+)
>> create mode 100644 tools/perf/arch/powerpc/util/htm.c
>> create mode 100644 tools/perf/util/powerpc-htm.h
>> 
>> diff --git a/tools/perf/arch/powerpc/util/Build b/tools/perf/arch/powerpc/util/Build
>> index 7819c8f5af2d..297152591046 100644
>> --- a/tools/perf/arch/powerpc/util/Build
>> +++ b/tools/perf/arch/powerpc/util/Build
>> @@ -8,3 +8,4 @@ perf-util-$(CONFIG_LIBDW) += skip-callchain-idx.o
>> 
>> perf-util-y += auxtrace.o
>> perf-util-y += vpa-dtl.o
>> +perf-util-y += htm.o
>> diff --git a/tools/perf/arch/powerpc/util/auxtrace.c b/tools/perf/arch/powerpc/util/auxtrace.c
>> index e04a0bd61755..9936d1910256 100644
>> --- a/tools/perf/arch/powerpc/util/auxtrace.c
>> +++ b/tools/perf/arch/powerpc/util/auxtrace.c
>> @@ -12,13 +12,16 @@
>> #include "../../util/debug.h"
>> #include "../../util/auxtrace.h"
>> #include "../../util/powerpc-vpadtl.h"
>> +#include "../../util/powerpc-htm.h"
>> #include "../../util/record.h"
>> +#include <string.h>
> 
> There is "#include <errno.h>" further up.  Maybe nicer to put them
> together as standard library includes
> 
>> 
>> struct auxtrace_record *auxtrace_record__init(struct evlist *evlist,
>> int *err)
>> {
>> struct evsel *pos;
>> struct evsel *vpa_dtl_evsel = NULL;
>> + struct evsel *htm_evsel = NULL;
>> 
>> /*
>>  * Set err value to zero here. Any fail later
>> @@ -32,11 +35,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")) {
>> + pos->needs_auxtrace_mmap = true;
>> + /* Remember the first matching HTM event */
>> + if (!htm_evsel)
>> + htm_evsel = pos;
>> }
>> }
>> 
>> + /*
>> +  * Only one auxtrace PMU can be initialised per session.  Reject
>> +  * concurrent VPA DTL and HTM events: HTM AUX buffers would be
>> +  * collected without a PERF_RECORD_AUXTRACE_INFO record, making
>> +  * the trace undecodable.
>> +  */
>> + if (vpa_dtl_evsel && htm_evsel) {
>> + pr_err("Cannot record VPA DTL and HTM auxtrace events simultaneously\n");
>> + *err = -EINVAL;
>> + return NULL;
>> + }
>> +
>> if (vpa_dtl_evsel)
>> return vpa_dtl_recording_init(vpa_dtl_evsel, err);
>> + else if (htm_evsel)
>> + return htm_recording_init(htm_evsel, err);
>> 
>> 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 000000000000..d2fdef488032
>> --- /dev/null
>> +++ b/tools/perf/arch/powerpc/util/htm.c
>> @@ -0,0 +1,185 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * HTM AUX tracing support
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/types.h>
>> +#include <linux/string.h>
>> +#include <linux/zalloc.h>
>> +#include <stdlib.h>
>> +#include <limits.h>
>> +#include "../../util/evsel.h"
>> +#include "../../util/evlist.h"
>> +#include "../../util/session.h"
>> +#include "../../util/debug.h"
>> +#include "../../util/auxtrace.h"
>> +#include "../../util/powerpc-htm.h"
>> +#include "../../util/record.h"
>> +#include <internal/lib.h> /* page_size */
>> +#include <errno.h>
>> +
>> +#define KiB(x) ((x) * 1024)
>> +
>> +struct htm_recording {
>> + struct auxtrace_record itr;
>> + struct evsel *evsel;
>> +};
>> +
>> +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) {
>> + size_t wm;
>> +
>> + if (strcmp(evsel__pmu_name(pos), "htm"))
>> + continue;
>> + wm = opts->auxtrace_mmap_pages * (size_t)page_size / 2;
>> + pos->core.attr.aux_watermark = min_t(size_t, wm, UINT_MAX);
>> + pos->core.attr.sample_type |= PERF_SAMPLE_RAW;
>> + pos->core.attr.freq = 0;
>> + pos->core.attr.sample_period = 1;
>> + 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 (!strcmp(evsel__pmu_name(pos), "htm"))
>> + n++;
>> + }
>> + return n;
>> +}
>> +
>> +static size_t htm_info_priv_size(struct auxtrace_record *itr __maybe_unused,
>> +  struct evlist *evlist)
>> +{
>> + return HTM_AUXTRACE_PRIV_SIZE(htm_nr_events(evlist));
>> +}
>> +
>> +/*
>> + * Fill the PERF_RECORD_AUXTRACE_INFO private data with:
>> + *   priv[POWERPC_HTM_PMU_TYPE]   = pmu->type of the first htm evsel
>> + *   priv[POWERPC_HTM_NUM_EVENTS] = total number of HTM CPU entries
>> + *   priv[POWERPC_HTM_EVENT_DATA + n*2]     = CPU number for nth entry
>> + *   priv[POWERPC_HTM_EVENT_DATA + n*2 + 1] = attr.config for nth entry
>> + *
>> + * One entry is written per CPU in each evsel's cpu map.  An evsel opened
>> + * with -C 0,1,2 contributes three entries (one per CPU), each carrying
>> + * the same attr.config.  The decode side uses event->auxtrace.cpu to look
>> + * up the matching config and derive (node, chip, core) for the output
>> + * file name.
>> + */
>> +static int
>> +htm_info_fill(struct auxtrace_record *itr,
>> +       struct perf_session *session,
>> +       struct perf_record_auxtrace_info *auxtrace_info,
>> +       size_t priv_size)
>> +{
>> + struct htm_recording *htm_r = container_of(itr, struct htm_recording, itr);
>> + struct evlist *evlist = session->evlist;
>> + struct evsel *pos;
>> + int n = 0;
>> + int expected_n = htm_nr_events(evlist);
>> +
>> + if (priv_size != HTM_AUXTRACE_PRIV_SIZE(expected_n))
>> + return -EINVAL;
>> +
>> + auxtrace_info->type = PERF_AUXTRACE_POWERPC_HTM;
>> + auxtrace_info->priv[POWERPC_HTM_PMU_TYPE] = htm_r->evsel->core.attr.type;
>> + auxtrace_info->priv[POWERPC_HTM_NUM_EVENTS] = expected_n;
>> +
>> + evlist__for_each_entry(evlist, pos) {
>> + struct perf_cpu_map *cpus;
>> + int i, nr;
>> +
>> + if (strcmp(evsel__pmu_name(pos), "htm"))
>> + continue;
>> +
>> + /*
>> +  * Emit one (cpu, config) entry for every CPU in this evsel's
>> +  * map.  perf record -C 0,1,2 creates one evsel with a
>> +  * three-entry cpu map; each CPU gets its own AUX buffer and
>> +  * must be individually mapped so the decoder can match
>> +  * event->auxtrace.cpu to the correct (node, chip, core).
>> +  */
>> + cpus = evsel__cpus(pos);
>> + nr = cpus ? perf_cpu_map__nr(cpus) : 0;
>> +
>> + if (nr > 0) {
>> + for (i = 0; i < nr; i++) {
>> + int cpu = perf_cpu_map__cpu(cpus, i).cpu;
>> +
>> + 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++;
>> + }
>> + } else {
>> + /* cpu-agnostic evsel: record cpu = -1 */
>> + auxtrace_info->priv[POWERPC_HTM_EVENT_DATA + n * 2]     = (u64)-1;
>> + auxtrace_info->priv[POWERPC_HTM_EVENT_DATA + n * 2 + 1] =
>> + pos->core.attr.config;
>> + n++;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static u64 htm_reference(struct auxtrace_record *itr __maybe_unused)
>> +{
>> + return 0;
>> +}
>> +
>> +static void htm_free(struct auxtrace_record *itr)
>> +{
>> + struct htm_recording *htm_r = container_of(itr, struct htm_recording, itr);
>> +
>> + free(htm_r);
>> +}
>> +
>> +struct auxtrace_record *htm_recording_init(struct evsel *pos, int *err)
>> +{
>> + struct htm_recording *htm_r;
>> +
>> + /*
>> +  * To obtain the auxtrace buffer file descriptor, the auxtrace event
>> +  * must come first.
>> +  */
>> + evlist__to_front(pos->evlist, pos);
>> +
>> + htm_r = zalloc(sizeof(*htm_r));
>> + if (!htm_r) {
>> + pr_debug("htm_recording allocation failed (-ENOMEM)\n");
>> + *err = -ENOMEM;
>> + return NULL;
>> + }
>> +
>> + htm_r->evsel = pos;
>> + htm_r->itr.recording_options = htm_recording_options;
>> + htm_r->itr.info_priv_size    = htm_info_priv_size;
>> + htm_r->itr.info_fill         = htm_info_fill;
>> + htm_r->itr.free              = htm_free;
>> + htm_r->itr.reference         = htm_reference;
>> + return &htm_r->itr;
>> +}
>> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
>> index aa749e1c3036..bf08f41d623f 100644
>> --- a/tools/perf/util/auxtrace.c
>> +++ b/tools/perf/util/auxtrace.c
>> @@ -56,6 +56,7 @@
>> #include "s390-cpumsf.h"
>> #include "util/mmap.h"
>> #include "powerpc-vpadtl.h"
>> +#include "powerpc-htm.h"
>> 
>> #include <linux/ctype.h>
>> #include "symbol/kallsyms.h"
>> @@ -1432,6 +1433,7 @@ int perf_event__process_auxtrace_info(const struct perf_tool *tool __maybe_unuse
>> case PERF_AUXTRACE_VPA_DTL:
>> err = powerpc_vpadtl_process_auxtrace_info(event, session);
>> break;
>> + case PERF_AUXTRACE_POWERPC_HTM:
>> case PERF_AUXTRACE_UNKNOWN:
>> default:
>> return -EINVAL;
>> diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
>> index 6947f3f284c0..68b17802a419 100644
>> --- a/tools/perf/util/auxtrace.h
>> +++ b/tools/perf/util/auxtrace.h
>> @@ -46,6 +46,7 @@ enum auxtrace_type {
>> PERF_AUXTRACE_S390_CPUMSF,
>> PERF_AUXTRACE_HISI_PTT,
>> PERF_AUXTRACE_VPA_DTL,
>> + PERF_AUXTRACE_POWERPC_HTM,
>> };
>> 
>> enum itrace_period_type {
>> diff --git a/tools/perf/util/powerpc-htm.h b/tools/perf/util/powerpc-htm.h
>> new file mode 100644
>> index 000000000000..18e39417d556
>> --- /dev/null
>> +++ b/tools/perf/util/powerpc-htm.h
>> @@ -0,0 +1,43 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __POWERPC_HTM_H
>> +#define __POWERPC_HTM_H
>> +
>> +#include <linux/types.h>
>> +
>> +/*
>> + * Layout of the private data in PERF_RECORD_AUXTRACE_INFO for HTM.
>> + *
>> + * priv[POWERPC_HTM_PMU_TYPE]   = htm PMU type ID (pmu->type from kernel)
>> + * priv[POWERPC_HTM_NUM_EVENTS] = number of htm evsels recorded (N)
>> + *
>> + * Followed by N pairs (2 u64 each):
>> + *   priv[POWERPC_HTM_EVENT_DATA + n*2 + 0] = CPU number for nth htm evsel
>> + *   priv[POWERPC_HTM_EVENT_DATA + n*2 + 1] = attr.config for nth htm evsel
>> + *
>> + * Total priv entries: POWERPC_HTM_EVENT_DATA + N * 2
>> + */
>> +enum {
>> + POWERPC_HTM_PMU_TYPE  = 0,
>> + POWERPC_HTM_NUM_EVENTS,
>> + POWERPC_HTM_EVENT_DATA, /* variable-length: 2 u64 per event */
>> +};
>> +
>> +/* Fixed header size (everything before the per-event data) */
>> +#define HTM_AUXTRACE_PRIV_FIXED  (POWERPC_HTM_EVENT_DATA * sizeof(u64))
>> +
>> +/* Total priv size for N htm evsels */
>> +#define HTM_AUXTRACE_PRIV_SIZE(n) \
>> + (HTM_AUXTRACE_PRIV_FIXED + (n) * 2 * sizeof(u64))
>> +
>> +struct evsel;
>> +struct evlist;
>> +union perf_event;
>> +struct perf_session;
>> +struct auxtrace_record;
>> +
>> +struct auxtrace_record *htm_recording_init(struct evsel *pos, int *err);
>> +
>> +int powerpc_htm_process_auxtrace_info(union perf_event *event,
>> +       struct perf_session *session);
> 
> Ideally, should only add a declaration for powerpc_htm_process_auxtrace_info()
> when it actually exists
> 
>> +
>> +#endif /* __POWERPC_HTM_H */