* [PATCH V2 1/6] tools/perf: Move powerpc VPA-DTL auxtrace init into a separate file
2026-07-20 10:52 [PATCH V2 0/6] tools/perf: Add powerpc HTM auxtrace support Athira Rajeev
@ 2026-07-20 10:52 ` Athira Rajeev
2026-07-20 11:15 ` sashiko-bot
2026-07-20 10:52 ` [PATCH V2 2/6] tools/perf: Add AUXTRACE recording support for powerpc HTM Athira Rajeev
` (4 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Athira Rajeev @ 2026-07-20 10:52 UTC (permalink / raw)
To: acme, jolsa, adrian.hunter, maddy, irogers, namhyung
Cc: linux-perf-users, linuxppc-dev, atrajeev, hbathini, tejas05,
tshah, venkat88, usha.r2
The powerpc auxtrace dispatch lives entirely in
arch/powerpc/util/auxtrace.c. As new PMUs such as HTM are added, this
file would grow to contain the recording logic for all of them.
Factor out the VPA-DTL recording initializer into its own file,
arch/powerpc/util/vpa-dtl.c, and reduce auxtrace.c to a thin dispatch
layer. auxtrace_record__init() now detects the PMU by name and calls
the appropriate per-PMU init function:
- vpa_dtl_recording_init() for VPA-DTL events (unchanged behaviour)
- Further PMU entries will follow in subsequent patches
This makes room in auxtrace_record__init() for the HTM recording path
added in the next patch without growing a single monolithic file.
Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
---
Changes in V2:
- Renamed the destination file from arch/powerpc/util/vpa-dtl.c (same
name, unchanged) but the subject and commit message are reworded to
clearly state that the goal is to make auxtrace_record__init() a thin
per-PMU dispatcher, not merely to "allow multiple PMUs to use auxtrace".
- Handle failure from memory allocation
- Included stdlib and limits.h
- No functional change to the VPA-DTL path itself.
- Patch is now 1/6 instead of 1/9.
tools/perf/arch/powerpc/util/Build | 1 +
tools/perf/arch/powerpc/util/auxtrace.c | 84 +++-------------------
tools/perf/arch/powerpc/util/vpa-dtl.c | 95 +++++++++++++++++++++++++
tools/perf/util/powerpc-vpadtl.h | 1 +
4 files changed, 105 insertions(+), 76 deletions(-)
create mode 100644 tools/perf/arch/powerpc/util/vpa-dtl.c
diff --git a/tools/perf/arch/powerpc/util/Build b/tools/perf/arch/powerpc/util/Build
index ae928050e07a..7819c8f5af2d 100644
--- a/tools/perf/arch/powerpc/util/Build
+++ b/tools/perf/arch/powerpc/util/Build
@@ -7,3 +7,4 @@ perf-util-y += evsel.o
perf-util-$(CONFIG_LIBDW) += skip-callchain-idx.o
perf-util-y += auxtrace.o
+perf-util-y += vpa-dtl.o
diff --git a/tools/perf/arch/powerpc/util/auxtrace.c b/tools/perf/arch/powerpc/util/auxtrace.c
index 4600a1661b4f..e04a0bd61755 100644
--- a/tools/perf/arch/powerpc/util/auxtrace.c
+++ b/tools/perf/arch/powerpc/util/auxtrace.c
@@ -13,63 +13,12 @@
#include "../../util/auxtrace.h"
#include "../../util/powerpc-vpadtl.h"
#include "../../util/record.h"
-#include <internal/lib.h> // page_size
-
-#define KiB(x) ((x) * 1024)
-
-static int
-powerpc_vpadtl_recording_options(struct auxtrace_record *ar __maybe_unused,
- struct evlist *evlist __maybe_unused,
- struct record_opts *opts)
-{
- opts->full_auxtrace = true;
-
- /*
- * Set auxtrace_mmap_pages to minimum
- * two pages
- */
- 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;
- }
-
- return 0;
-}
-
-static size_t powerpc_vpadtl_info_priv_size(struct auxtrace_record *itr __maybe_unused,
- struct evlist *evlist __maybe_unused)
-{
- return VPADTL_AUXTRACE_PRIV_SIZE;
-}
-
-static int
-powerpc_vpadtl_info_fill(struct auxtrace_record *itr __maybe_unused,
- struct perf_session *session __maybe_unused,
- struct perf_record_auxtrace_info *auxtrace_info,
- size_t priv_size __maybe_unused)
-{
- auxtrace_info->type = PERF_AUXTRACE_VPA_DTL;
-
- return 0;
-}
-
-static void powerpc_vpadtl_free(struct auxtrace_record *itr)
-{
- free(itr);
-}
-
-static u64 powerpc_vpadtl_reference(struct auxtrace_record *itr __maybe_unused)
-{
- return 0;
-}
struct auxtrace_record *auxtrace_record__init(struct evlist *evlist,
int *err)
{
- struct auxtrace_record *aux;
struct evsel *pos;
- int found = 0;
+ struct evsel *vpa_dtl_evsel = NULL;
/*
* Set err value to zero here. Any fail later
@@ -78,33 +27,16 @@ struct auxtrace_record *auxtrace_record__init(struct evlist *evlist,
*err = 0;
evlist__for_each_entry(evlist, pos) {
- if (strstarts(pos->name, "vpa_dtl")) {
- found = 1;
+ if (pos->name && strstarts(pos->name, "vpa_dtl")) {
pos->needs_auxtrace_mmap = true;
- break;
+ /* Remember the first matching VPA DTL event */
+ if (!vpa_dtl_evsel)
+ vpa_dtl_evsel = pos;
}
}
- if (!found)
- return NULL;
-
- /*
- * To obtain the auxtrace buffer file descriptor, the auxtrace event
- * must come first.
- */
- evlist__to_front(pos->evlist, pos);
-
- aux = zalloc(sizeof(*aux));
- if (aux == NULL) {
- pr_debug("aux record is NULL\n");
- *err = -ENOMEM;
- return NULL;
- }
+ if (vpa_dtl_evsel)
+ return vpa_dtl_recording_init(vpa_dtl_evsel, err);
- aux->recording_options = powerpc_vpadtl_recording_options;
- aux->info_priv_size = powerpc_vpadtl_info_priv_size;
- aux->info_fill = powerpc_vpadtl_info_fill;
- aux->free = powerpc_vpadtl_free;
- aux->reference = powerpc_vpadtl_reference;
- return aux;
+ return NULL;
}
diff --git a/tools/perf/arch/powerpc/util/vpa-dtl.c b/tools/perf/arch/powerpc/util/vpa-dtl.c
new file mode 100644
index 000000000000..fde388d87477
--- /dev/null
+++ b/tools/perf/arch/powerpc/util/vpa-dtl.c
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * VPA DTL AUX tracing support
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/string.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <limits.h>
+#include "../../util/cpumap.h"
+#include "../../util/evsel.h"
+#include "../../util/evlist.h"
+#include "../../util/session.h"
+#include "../../util/util.h"
+#include "../../util/debug.h"
+#include "../../util/auxtrace.h"
+#include "../../util/powerpc-vpadtl.h"
+#include "../../util/record.h"
+#include <internal/lib.h> // page_size
+
+#define KiB(x) ((x) * 1024)
+
+static int
+powerpc_vpadtl_recording_options(struct auxtrace_record *ar __maybe_unused,
+ struct evlist *evlist __maybe_unused,
+ struct record_opts *opts)
+{
+ opts->full_auxtrace = true;
+
+ /*
+ * Set auxtrace_mmap_pages to minimum
+ * two pages
+ */
+ 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;
+ }
+
+ return 0;
+}
+
+static size_t powerpc_vpadtl_info_priv_size(struct auxtrace_record *itr __maybe_unused,
+ struct evlist *evlist __maybe_unused)
+{
+ return VPADTL_AUXTRACE_PRIV_SIZE;
+}
+
+static int
+powerpc_vpadtl_info_fill(struct auxtrace_record *itr __maybe_unused,
+ struct perf_session *session __maybe_unused,
+ struct perf_record_auxtrace_info *auxtrace_info,
+ size_t priv_size __maybe_unused)
+{
+ auxtrace_info->type = PERF_AUXTRACE_VPA_DTL;
+
+ return 0;
+}
+
+static void powerpc_vpadtl_free(struct auxtrace_record *itr)
+{
+ free(itr);
+}
+
+static u64 powerpc_vpadtl_reference(struct auxtrace_record *itr __maybe_unused)
+{
+ return 0;
+}
+
+struct auxtrace_record *vpa_dtl_recording_init(struct evsel *pos, int *err)
+{
+ struct auxtrace_record *aux;
+
+ /*
+ * To obtain the auxtrace buffer file descriptor, the auxtrace event
+ * must come first.
+ */
+ evlist__to_front(pos->evlist, pos);
+
+ aux = zalloc(sizeof(*aux));
+ if (aux == NULL) {
+ pr_debug("aux record allocation failed (-ENOMEM)\n");
+ *err = -ENOMEM;
+ return NULL;
+ }
+
+ aux->recording_options = powerpc_vpadtl_recording_options;
+ aux->info_priv_size = powerpc_vpadtl_info_priv_size;
+ aux->info_fill = powerpc_vpadtl_info_fill;
+ aux->free = powerpc_vpadtl_free;
+ aux->reference = powerpc_vpadtl_reference;
+ return aux;
+}
diff --git a/tools/perf/util/powerpc-vpadtl.h b/tools/perf/util/powerpc-vpadtl.h
index ca809660b9bb..41e09b17a353 100644
--- a/tools/perf/util/powerpc-vpadtl.h
+++ b/tools/perf/util/powerpc-vpadtl.h
@@ -20,4 +20,5 @@ struct perf_pmu;
int powerpc_vpadtl_process_auxtrace_info(union perf_event *event,
struct perf_session *session);
+struct auxtrace_record *vpa_dtl_recording_init(struct evsel *pos, int *err);
#endif
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH V2 1/6] tools/perf: Move powerpc VPA-DTL auxtrace init into a separate file
2026-07-20 10:52 ` [PATCH V2 1/6] tools/perf: Move powerpc VPA-DTL auxtrace init into a separate file Athira Rajeev
@ 2026-07-20 11:15 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-07-20 11:15 UTC (permalink / raw)
To: Athira Rajeev; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added file `vpa-dtl.c` uses `zalloc()` but fails to include `<linux/zalloc.h>`. On 64-bit platforms like PowerPC, this implicit declaration causes the compiler to assume `zalloc` returns `int`, leading to the truncation of the upper 32 bits of the returned 64-bit pointer.
--
--- Patch [1]: [PATCH V2 1/6] tools/perf: Move powerpc VPA-DTL auxtrace init into a separate file ---
commit 36d571f8cc56fc84bd8c942791167ff968206898
Author: Athira Rajeev <atrajeev@linux.ibm.com>
tools/perf: Move powerpc VPA-DTL auxtrace init into a separate file
This commit extracts the VPA-DTL auxtrace recording initializer into a new
file, vpa-dtl.c. It reduces auxtrace.c to a thin dispatch layer to prepare
for HTM recording.
> diff --git a/tools/perf/arch/powerpc/util/vpa-dtl.c b/tools/perf/arch/powerpc/util/vpa-dtl.c
> new file mode 100644
> index 0000000000000..fde388d874775
> --- /dev/null
> +++ b/tools/perf/arch/powerpc/util/vpa-dtl.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * VPA DTL AUX tracing support
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/string.h>
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <limits.h>
> +#include "../../util/cpumap.h"
> +#include "../../util/evsel.h"
> +#include "../../util/evlist.h"
> +#include "../../util/session.h"
> +#include "../../util/util.h"
> +#include "../../util/debug.h"
> +#include "../../util/auxtrace.h"
> +#include "../../util/powerpc-vpadtl.h"
> +#include "../../util/record.h"
> +#include <internal/lib.h> // page_size
[ ... ]
> +struct auxtrace_record *vpa_dtl_recording_init(struct evsel *pos, int *err)
> +{
> + struct auxtrace_record *aux;
> +
> + /*
> + * To obtain the auxtrace buffer file descriptor, the auxtrace event
> + * must come first.
> + */
> + evlist__to_front(pos->evlist, pos);
> +
> + aux = zalloc(sizeof(*aux));
[Severity: Medium]
Is the <linux/zalloc.h> include missing from the headers at the top of the
file?
Since zalloc is used here without a declaration, the compiler will treat it
as implicitly declared and returning an int. On 64-bit platforms like PowerPC,
this truncates the upper 32 bits of the returned pointer, which could result
in a segmentation fault at runtime.
> + if (aux == NULL) {
> + pr_debug("aux record allocation failed (-ENOMEM)\n");
> + *err = -ENOMEM;
> + return NULL;
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720105218.14277-1-atrajeev@linux.ibm.com?part=1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V2 2/6] tools/perf: Add AUXTRACE recording support for powerpc HTM
2026-07-20 10:52 [PATCH V2 0/6] tools/perf: Add powerpc HTM auxtrace support Athira Rajeev
2026-07-20 10:52 ` [PATCH V2 1/6] tools/perf: Move powerpc VPA-DTL auxtrace init into a separate file Athira Rajeev
@ 2026-07-20 10:52 ` Athira Rajeev
2026-07-20 11:20 ` sashiko-bot
2026-07-20 10:52 ` [PATCH V2 3/6] tools/perf: Add arch hook to drain remaining data before event close Athira Rajeev
` (3 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Athira Rajeev @ 2026-07-20 10:52 UTC (permalink / raw)
To: acme, jolsa, adrian.hunter, maddy, irogers, namhyung
Cc: linux-perf-users, linuxppc-dev, atrajeev, hbathini, tejas05,
tshah, venkat88, usha.r2
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>
---
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 | 9 ++
tools/perf/arch/powerpc/util/htm.c | 169 ++++++++++++++++++++++++
tools/perf/util/powerpc-htm.h | 43 ++++++
4 files changed, 222 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..fd9215e8d3f2 100644
--- a/tools/perf/arch/powerpc/util/auxtrace.c
+++ b/tools/perf/arch/powerpc/util/auxtrace.c
@@ -12,6 +12,7 @@
#include "../../util/debug.h"
#include "../../util/auxtrace.h"
#include "../../util/powerpc-vpadtl.h"
+#include "../../util/powerpc-htm.h"
#include "../../util/record.h"
struct auxtrace_record *auxtrace_record__init(struct evlist *evlist,
@@ -19,6 +20,7 @@ struct auxtrace_record *auxtrace_record__init(struct evlist *evlist,
{
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 +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 && strstarts(pos->name, "htm")) {
+ 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);
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..07b496e4be2f
--- /dev/null
+++ b/tools/perf/arch/powerpc/util/htm.c
@@ -0,0 +1,169 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HTM AUX tracing support
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/string.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) {
+ if (!pos->name || !strstarts(pos->name, "htm"))
+ continue;
+ pos->core.attr.aux_watermark =
+ opts->auxtrace_mmap_pages * (size_t)page_size;
+ 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 && strstarts(pos->name, "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] = number of htm evsels
+ * priv[POWERPC_HTM_EVENT_DATA + n*2] = CPU for nth htm evsel
+ * priv[POWERPC_HTM_EVENT_DATA + n*2 + 1] = attr.config for nth htm evsel
+ *
+ * The CPU is the first CPU in the evsel's cpu map; for events opened with
+ * cpu=N there is exactly one CPU. 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;
+
+ // To set: auxtrace_info->type
+ 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 cpu;
+
+ if (!pos->name || !strstarts(pos->name, "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;
+ 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;
+}
+
+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/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);
+
+#endif /* __POWERPC_HTM_H */
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH V2 2/6] tools/perf: Add AUXTRACE recording support for powerpc HTM
2026-07-20 10:52 ` [PATCH V2 2/6] tools/perf: Add AUXTRACE recording support for powerpc HTM Athira Rajeev
@ 2026-07-20 11:20 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-07-20 11:20 UTC (permalink / raw)
To: Athira Rajeev; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized auxtrace_info->type causes perf.data corruption and breaks bisectability.
- [High] aux_watermark is set to the full size of the ring buffer, risking trace data loss.
--
--- Patch [2]: [PATCH V2 2/6] tools/perf: Add AUXTRACE recording support for powerpc HTM ---
commit 90e77eef6274098986791263120e8c79ee34c0b9
Author: Athira Rajeev <atrajeev@linux.ibm.com>
tools/perf: Add AUXTRACE recording support for powerpc HTM
Adds powerpc perf tool support for HTM PMU AUXTRACE recording. Introduces
htm_recording_init() and related auxtrace callbacks to create
PERF_RECORD_AUXTRACE records for HTM data. Updates auxtrace_record__init()
to detect HTM events.
> diff --git a/tools/perf/arch/powerpc/util/htm.c b/tools/perf/arch/powerpc/util/htm.c
> new file mode 100644
> index 0000000000000..07b496e4be2fb
> --- /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 || !strstarts(pos->name, "htm"))
> + continue;
> + pos->core.attr.aux_watermark =
> + opts->auxtrace_mmap_pages * (size_t)page_size;
[Severity: High]
Does setting the watermark to the full size of the ring buffer risk hardware
trace data loss?
By setting the watermark equal to the entire buffer size, the wakeup only
fires when the buffer is 100% full. Because scheduling the user-space perf
tool and reading the buffer introduces latency, any hardware trace data
generated during this window might be dropped due to a lack of buffer
headroom.
> + pos->core.attr.freq = 0;
> + pos->core.attr.sample_type |= PERF_SAMPLE_RAW;
> + pos->needs_auxtrace_mmap = true;
> + }
> +
> + return 0;
> +}
[ ... ]
> +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;
> +
> + // To set: auxtrace_info->type
> + auxtrace_info->priv[POWERPC_HTM_PMU_TYPE] = htm_r->evsel->core.attr.type;
[Severity: High]
Does leaving auxtrace_info->type uninitialized break bisectability by
generating corrupted perf.data files?
Because the type is left uninitialized (defaulting to PERF_AUXTRACE_UNKNOWN)
before the auxtrace info data is written to the stream, this might create
trace files that cannot be read by `perf report` at this commit.
> + auxtrace_info->priv[POWERPC_HTM_NUM_EVENTS] = expected_n;
> +
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720105218.14277-1-atrajeev@linux.ibm.com?part=2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V2 3/6] tools/perf: Add arch hook to drain remaining data before event close
2026-07-20 10:52 [PATCH V2 0/6] tools/perf: Add powerpc HTM auxtrace support Athira Rajeev
2026-07-20 10:52 ` [PATCH V2 1/6] tools/perf: Move powerpc VPA-DTL auxtrace init into a separate file Athira Rajeev
2026-07-20 10:52 ` [PATCH V2 2/6] tools/perf: Add AUXTRACE recording support for powerpc HTM Athira Rajeev
@ 2026-07-20 10:52 ` Athira Rajeev
2026-07-20 11:16 ` sashiko-bot
2026-07-20 10:52 ` [PATCH V2 4/6] tools/perf: Add powerpc callback support for arch_perf_record__need_read Athira Rajeev
` (2 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Athira Rajeev @ 2026-07-20 10:52 UTC (permalink / raw)
To: acme, jolsa, adrian.hunter, maddy, irogers, namhyung
Cc: linux-perf-users, linuxppc-dev, atrajeev, hbathini, tejas05,
tshah, venkat88, usha.r2
While collecting samples using perf record, __cmd_record() disables the
evlist once recording is complete. After that, event fds are no longer
read and any remaining PMU-specific data cannot be drained.
Add a weak arch_perf_record__need_read() hook so architecture code can
indicate that more data remains to be collected before events are
disabled and closed. When the hook reports pending data, perf record
performs another read pass.
This allows architectures such as powerpc HTM to drain trace data and
associated metadata before the event is closed.
Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
---
Changes in V2:
- V1's callback was responsible for driving the read loop
including evlist__enable cycling. Removed that logic
- Use bytes written to check if session needs to be continued.
- Patch is now 3/6 instead of 3/9.
tools/perf/builtin-record.c | 45 +++++++++++++++++++++++++++++++++++++
tools/perf/util/record.h | 3 +++
2 files changed, 48 insertions(+)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index f58d7e3c7879..ae44e452d148 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -2437,6 +2437,45 @@ static unsigned long record__waking(struct record *rec)
return waking;
}
+/*
+ * Weak symbol - architecture can override to indicate if more
+ * data needs to be collected before finishing output.
+ *
+ * Returns: 1 if more data exists, 0 if collection is complete
+ */
+__weak int arch_perf_record__need_read(struct evlist *evlist __maybe_unused)
+{
+ return 0; /* Default: no arch-specific data to collect */
+}
+
+static void record__final_data(struct record *rec)
+{
+ u64 last_bytes_written = 0;
+ /*
+ * Collect any remaining architecture-specific data.
+ * The arch code checks if more data exists, and we do the actual
+ * reading here since we have access to record__mmap_read_all().
+ * This code performs the additional read pass while events are
+ * still live.
+ */
+ while (arch_perf_record__need_read(rec->evlist)) {
+ /* If user presses Ctrl+C again during draining, abort cleanly */
+ if (done > 1)
+ break;
+
+ last_bytes_written = rec->bytes_written;
+
+ if (record__mmap_read_all(rec, true) < 0)
+ break;
+
+ if (rec->bytes_written == last_bytes_written) {
+ pr_warning("Final data drain made no forward progress.\n");
+ break;
+ }
+ usleep(100);
+ }
+}
+
static int __cmd_record(struct record *rec, int argc, const char **argv)
{
int err;
@@ -2451,6 +2490,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
float ratio = 0;
enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
struct perf_env *env;
+ bool final_data_drained = false;
atexit(record__sig_exit);
signal(SIGCHLD, sig_handler);
@@ -2857,6 +2897,11 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
done = 1;
}
+ if (done && !disabled && !final_data_drained) {
+ record__final_data(rec);
+ final_data_drained = true;
+ }
+
/*
* When perf is starting the traced process, at the end events
* die with the process and we wait for that. Thus no need to
diff --git a/tools/perf/util/record.h b/tools/perf/util/record.h
index 93627c9a7338..56de4f95a836 100644
--- a/tools/perf/util/record.h
+++ b/tools/perf/util/record.h
@@ -10,6 +10,7 @@
#include "util/target.h"
struct option;
+struct evlist;
struct record_opts {
struct target target;
@@ -95,4 +96,6 @@ static inline bool record_opts__no_switch_events(const struct record_opts *opts)
return opts->record_switch_events_set && !opts->record_switch_events;
}
+int arch_perf_record__need_read(struct evlist *evlist);
+
#endif // _PERF_RECORD_H
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH V2 3/6] tools/perf: Add arch hook to drain remaining data before event close
2026-07-20 10:52 ` [PATCH V2 3/6] tools/perf: Add arch hook to drain remaining data before event close Athira Rajeev
@ 2026-07-20 11:16 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-07-20 11:16 UTC (permalink / raw)
To: Athira Rajeev; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] `record__final_data()` fails to drain architecture-specific data and falsely aborts in parallel trace streaming mode due to incorrect byte tracking and thread-local mmap limitations.
- [Medium] The final data drain hook is completely bypassed on normal exit if the normal event ring buffer is empty when the termination signal is evaluated.
- [Medium] The final data drain loop aborts on the first empty read pass, failing to wait for asynchronous PMU data to arrive.
- [Medium] The force-abort mechanism `if (done > 1)` inside `record__final_data()` is mathematically impossible and will never successfully abort a stuck drain loop.
--
--- Patch [3]: [PATCH V2 3/6] tools/perf: Add arch hook to drain remaining data before event close ---
commit 912498764cf4d44940ef92ba0d9b5b3fa62c31b4
Author: Athira Rajeev <atrajeev@linux.ibm.com>
tools/perf: Add arch hook to drain remaining data before event close
This patch adds a weak architecture hook to allow collecting remaining
PMU-specific data before events are disabled and closed. It introduces
a loop in the record command to perform additional read passes when the
architecture reports pending data.
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
[ ... ]
> @@ -2425,6 +2425,45 @@ static unsigned long record__waking(struct record *rec)
> return waking;
> }
>
> +/*
> + * Weak symbol - architecture can override to indicate if more
> + * data needs to be collected before finishing output.
> + *
> + * Returns: 1 if more data exists, 0 if collection is complete
> + */
> +__weak int arch_perf_record__need_read(struct evlist *evlist __maybe_unused)
> +{
> + return 0; /* Default: no arch-specific data to collect */
> +}
> +
> +static void record__final_data(struct record *rec)
> +{
> + u64 last_bytes_written = 0;
> + /*
> + * Collect any remaining architecture-specific data.
> + * The arch code checks if more data exists, and we do the actual
> + * reading here since we have access to record__mmap_read_all().
> + * This code performs the additional read pass while events are
> + * still live.
> + */
> + while (arch_perf_record__need_read(rec->evlist)) {
> + /* If user presses Ctrl+C again during draining, abort cleanly */
> + if (done > 1)
> + break;
[Severity: Medium]
Is it possible for this abort condition to trigger? Looking at sig_handler()
for SIGINT, the variable done is set to 1 directly:
sig_handler() {
...
done = 1;
...
}
Since done is never incremented, could this block of code fail to force-abort
if the PMU gets stuck during the drain?
> +
> + last_bytes_written = rec->bytes_written;
> +
> + if (record__mmap_read_all(rec, true) < 0)
> + break;
> +
> + if (rec->bytes_written == last_bytes_written) {
> + pr_warning("Final data drain made no forward progress.\n");
> + break;
> + }
[Severity: Medium]
Does this check properly account for multithreaded mode? In parallel trace
streaming mode, worker threads maintain their own mmaps, write to their own
files, and increment rec->thread_bytes_written instead of rec->bytes_written.
Additionally, when record__mmap_read_all() is invoked from the main thread,
it only drains mmaps assigned to the main thread. Will this cause the loop
to abort prematurely with a false warning when running with multiple threads?
Also, if the first non-blocking read pass yields no data because the hardware
needs a moment to flush asynchronous data, the loop breaks immediately here.
Does this bypass the usleep(100) and defeat the purpose of waiting for the
pending data?
> + usleep(100);
> + }
> +}
> +
[ ... ]
> @@ -2845,6 +2885,11 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
> done = 1;
> }
>
> + if (done && !disabled && !final_data_drained) {
> + record__final_data(rec);
> + final_data_drained = true;
> + }
> +
> /*
> * When perf is starting the traced process, at the end events
> * die with the process and we wait for that. Thus no need to
[Severity: Medium]
If done is set to 1 by a termination signal (like SIGINT) and the subsequent
read pass yields no normal events, will we ever reach this newly added code?
Looking earlier in the polling loop of __cmd_record():
if (hits == thread->samples) {
if (done || draining)
break;
...
The break statement exits the loop completely. Does this mean that for
low-frequency workloads, if the ring buffer happens to be empty when the
signal is evaluated, the final data drain hook is completely bypassed?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720105218.14277-1-atrajeev@linux.ibm.com?part=3
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V2 4/6] tools/perf: Add powerpc callback support for arch_perf_record__need_read
2026-07-20 10:52 [PATCH V2 0/6] tools/perf: Add powerpc HTM auxtrace support Athira Rajeev
` (2 preceding siblings ...)
2026-07-20 10:52 ` [PATCH V2 3/6] tools/perf: Add arch hook to drain remaining data before event close Athira Rajeev
@ 2026-07-20 10:52 ` Athira Rajeev
2026-07-20 11:18 ` sashiko-bot
2026-07-20 10:52 ` [PATCH V2 5/6] tools/perf: Add powerpc HTM auxtrace event processing support Athira Rajeev
2026-07-20 10:52 ` [PATCH V2 6/6] tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records Athira Rajeev
5 siblings, 1 reply; 13+ messages in thread
From: Athira Rajeev @ 2026-07-20 10:52 UTC (permalink / raw)
To: acme, jolsa, adrian.hunter, maddy, irogers, namhyung
Cc: linux-perf-users, linuxppc-dev, atrajeev, hbathini, tejas05,
tshah, venkat88, usha.r2
Implement the arch_perf_record__need_read() architecture-specific hook
for powerpc in arch/powerpc/util/evsel.c.
The HTM kernel driver sets event->count to 1 while data is staged in
its internal buffers and to 0 once the stream is exhausted. This hook
reads that count for every open htm evsel via perf_evsel__read() and
accumulates the values. A non-zero total means at least one HTM target
still has data pending; the recording loop added in the previous patch
will perform another mmap-read pass.
The implementation scans the evlist for htm events, iterates the fd/
sample-id xyarray, and skips any evsel whose fd and sample-id arrays are
mismatched to avoid reading stale state. When the accumulated count
reaches zero the hook returns 0 and the recording loop proceeds to
disable and close the events.
Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
---
Changes in V2:
- Implements the renamed arch_perf_record__need_read() hook (V1
implemented arch_record__collect_final_data()).
- Skips evsels whose fd and sample-id xyarrays are mismatched, avoiding
stale-state reads. V1 had no such guard.
- evlist__enable cycling is removed; that responsibility now belongs to
the drain loop in builtin-record.c added in patch 3.
- File location changed to arch/powerpc/util/evsel.c (V1 used
arch/powerpc/util/powerpc-htm.c).
- Patch is now 4/6 instead of 4/9.
tools/perf/arch/powerpc/util/evsel.c | 69 ++++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
diff --git a/tools/perf/arch/powerpc/util/evsel.c b/tools/perf/arch/powerpc/util/evsel.c
index 2f733cdc8dbb..8f2805a68026 100644
--- a/tools/perf/arch/powerpc/util/evsel.c
+++ b/tools/perf/arch/powerpc/util/evsel.c
@@ -1,8 +1,77 @@
// SPDX-License-Identifier: GPL-2.0
#include <stdio.h>
+#include <linux/string.h>
#include "util/evsel.h"
+#include "util/record.h"
+#include "util/evlist.h"
+#include "util/debug.h"
+#include <internal/xyarray.h>
void arch_evsel__set_sample_weight(struct evsel *evsel)
{
evsel__set_sample_bit(evsel, WEIGHT_STRUCT);
}
+
+/*
+ * Check if HTM events have more data to collect.
+ *
+ * This function reads the HTM event counts. When the kernel driver
+ * has more data available, it returns a non-zero count. When all
+ * data has been collected, it returns zero.
+ *
+ * Returns: 1 if more data exists, 0 if collection is complete
+ */
+int arch_perf_record__need_read(struct evlist *evlist)
+{
+ struct evsel *evsel;
+ bool found_htm = false;
+ u64 total_pending_bytes = 0;
+ int x, y;
+
+ /* there was an error during record__open */
+ if (!evlist)
+ return 0;
+
+ /* First, check if any HTM events exist */
+ evlist__for_each_entry(evlist, evsel) {
+ if (evsel->name && strstarts(evsel->name, "htm")) {
+ found_htm = true;
+ break;
+ }
+ }
+
+ if (!found_htm)
+ return 0;
+
+ /* Read HTM event counts to check if more data is available */
+ evlist__for_each_entry(evlist, evsel) {
+ struct xyarray *xy = evsel->core.sample_id;
+
+ if (!evsel->name || !strstarts(evsel->name, "htm"))
+ continue;
+
+ if (!strcmp(evsel->name, "dummy:u"))
+ continue;
+
+ if (xy == NULL || evsel->core.fd == NULL)
+ continue;
+
+ if (xyarray__max_x(evsel->core.fd) != xyarray__max_x(xy) ||
+ xyarray__max_y(evsel->core.fd) != xyarray__max_y(xy)) {
+ pr_debug("Unmatched FD vs sample ID array for HTM event\n");
+ continue;
+ }
+
+ for (x = 0; x < xyarray__max_x(xy); x++) {
+ for (y = 0; y < xyarray__max_y(xy); y++) {
+ struct perf_counts_values count = { .val = 0 };
+
+ if (perf_evsel__read(&evsel->core, x, y, &count) == 0)
+ total_pending_bytes += count.val;
+ }
+ }
+ }
+
+ /* Collection is complete only when ALL hardware queues match 0 */
+ return (total_pending_bytes > 0) ? 1 : 0;
+}
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH V2 4/6] tools/perf: Add powerpc callback support for arch_perf_record__need_read
2026-07-20 10:52 ` [PATCH V2 4/6] tools/perf: Add powerpc callback support for arch_perf_record__need_read Athira Rajeev
@ 2026-07-20 11:18 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-07-20 11:18 UTC (permalink / raw)
To: Athira Rajeev; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The function identifies HTM events using a highly brittle prefix match on the event name (`strstarts(evsel->name, "htm")`), which will erroneously match unrelated user-named events or tracepoints.
- [Low] The file uses `strcmp()` and `bool` but fails to explicitly include the standard POSIX headers `<string.h>` and `<stdbool.h>`, violating the subsystem musl compatibility guidelines.
- [Low] The string comparison against 'dummy:u' is mathematically unreachable and constitutes dead code.
--
--- Patch [4]: [PATCH V2 4/6] tools/perf: Add powerpc callback support for arch_perf_record__need_read ---
commit 08efcd22d3e33aa6575dbd818f05eb3b46a80a92
Author: Athira Rajeev <atrajeev@linux.ibm.com>
tools/perf: Add powerpc callback support for arch_perf_record__need_read
Implement the arch_perf_record__need_read() architecture-specific hook
for powerpc in arch/powerpc/util/evsel.c. The hook reads event counts for
open htm evsels to determine if data is pending.
> diff --git a/tools/perf/arch/powerpc/util/evsel.c b/tools/perf/arch/powerpc/util/evsel.c
> index 2f733cdc8dbbe..8f2805a680261 100644
> --- a/tools/perf/arch/powerpc/util/evsel.c
> +++ b/tools/perf/arch/powerpc/util/evsel.c
> @@ -1,8 +1,77 @@
> // SPDX-License-Identifier: GPL-2.0
> #include <stdio.h>
> +#include <linux/string.h>
> #include "util/evsel.h"
> +#include "util/record.h"
> +#include "util/evlist.h"
> +#include "util/debug.h"
> +#include <internal/xyarray.h>
[Severity: Low]
Does this file need to explicitly include <string.h> and <stdbool.h>?
This file uses bool and strcmp(), but relies on implicit inclusion or
<linux/string.h>. Compiling perf against musl libc requires direct inclusion
of standard POSIX headers for these types and functions to avoid build
failures.
[ ... ]
> +int arch_perf_record__need_read(struct evlist *evlist)
> +{
> + struct evsel *evsel;
[ ... ]
> + /* Read HTM event counts to check if more data is available */
> + evlist__for_each_entry(evlist, evsel) {
> + struct xyarray *xy = evsel->core.sample_id;
> +
> + if (!evsel->name || !strstarts(evsel->name, "htm"))
> + continue;
[Severity: Medium]
Is it safe to identify HTM events using a prefix match on the event name?
If a user traces a standard event and names it with an "htm" prefix (e.g.,
perf record -e cpu-cycles/name=htm_cycles/), this hook would falsely
identify it as an HTM event and read its event count.
For standard events, this count contains the actual accumulated hits, not the
0/1 driver staging flag, causing total_pending_bytes to always be > 0. Would
this force arch_perf_record__need_read() to always return 1 and cause the
drain loop in record__final_data() to instantly abort with a spurious warning
about making no forward progress?
Would it be more robust to identify these events by checking the PMU type
instead of the name?
> +
> + if (!strcmp(evsel->name, "dummy:u"))
> + continue;
[Severity: Low]
Is this string comparison reachable?
The loop explicitly filters events above with:
if (!evsel->name || !strstarts(evsel->name, "htm")) continue;
Because "dummy:u" does not start with "htm", any "dummy:u" event is already
caught and skipped by the first condition, making this check dead code.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720105218.14277-1-atrajeev@linux.ibm.com?part=4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V2 5/6] tools/perf: Add powerpc HTM auxtrace event processing support
2026-07-20 10:52 [PATCH V2 0/6] tools/perf: Add powerpc HTM auxtrace support Athira Rajeev
` (3 preceding siblings ...)
2026-07-20 10:52 ` [PATCH V2 4/6] tools/perf: Add powerpc callback support for arch_perf_record__need_read Athira Rajeev
@ 2026-07-20 10:52 ` Athira Rajeev
2026-07-20 11:18 ` sashiko-bot
2026-07-20 10:52 ` [PATCH V2 6/6] tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records Athira Rajeev
5 siblings, 1 reply; 13+ messages in thread
From: Athira Rajeev @ 2026-07-20 10:52 UTC (permalink / raw)
To: acme, jolsa, adrian.hunter, maddy, irogers, namhyung
Cc: linux-perf-users, linuxppc-dev, atrajeev, hbathini, tejas05,
tshah, venkat88, usha.r2
Add the PERF_AUXTRACE_POWERPC_HTM enum value to auxtrace.h and wire
the dispatch in perf_event__process_auxtrace_info() to call
powerpc_htm_process_auxtrace_info().
This patch sets auxtrace_info->type = PERF_AUXTRACE_POWERPC_HTM in
htm_info_fill() (introduced in the previous patch) now that the
constant is defined, completing the record-side setup.
Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
---
Changes in V2:
- Scope narrowed: this patch now only adds the PERF_AUXTRACE_POWERPC_HTM
enum constant to auxtrace.h and wires the dispatch in
perf_event__process_auxtrace_info() to call
powerpc_htm_process_auxtrace_info(). It also amends htm_info_fill()
(from patch 2) to set auxtrace_info->type now that the constant is
defined.
- All file-writing and decoding logic is moved to patch 6.
- Patch is now 5/6 instead of 5/9.
tools/perf/arch/powerpc/util/htm.c | 2 +-
tools/perf/util/Build | 1 +
tools/perf/util/auxtrace.c | 4 +
tools/perf/util/auxtrace.h | 1 +
tools/perf/util/powerpc-htm.c | 115 +++++++++++++++++++++++++++++
5 files changed, 122 insertions(+), 1 deletion(-)
create mode 100644 tools/perf/util/powerpc-htm.c
diff --git a/tools/perf/arch/powerpc/util/htm.c b/tools/perf/arch/powerpc/util/htm.c
index 07b496e4be2f..9555cf356617 100644
--- a/tools/perf/arch/powerpc/util/htm.c
+++ b/tools/perf/arch/powerpc/util/htm.c
@@ -99,7 +99,7 @@ htm_info_fill(struct auxtrace_record *itr,
if (priv_size != HTM_AUXTRACE_PRIV_SIZE(expected_n))
return -EINVAL;
- // To set: auxtrace_info->type
+ 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;
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 330311cac550..7fa354853d2a 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -141,6 +141,7 @@ perf-util-y += hisi-ptt.o
perf-util-y += hisi-ptt-decoder/
perf-util-y += s390-cpumsf.o
perf-util-y += powerpc-vpadtl.o
+perf-util-y += powerpc-htm.o
ifdef CONFIG_LIBOPENCSD
perf-util-y += cs-etm.o
diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index 0b851f32e98c..9f32f54fad43 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"
@@ -1427,6 +1428,9 @@ 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:
+ err = powerpc_htm_process_auxtrace_info(event, session);
+ break;
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.c b/tools/perf/util/powerpc-htm.c
new file mode 100644
index 000000000000..8d28d02031d6
--- /dev/null
+++ b/tools/perf/util/powerpc-htm.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <stdio.h>
+#include <stdlib.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <string.h>
+#include <errno.h>
+#include <endian.h>
+#include "util/evsel.h"
+#include "util/evlist.h"
+#include "util/session.h"
+#include "color.h"
+#include "powerpc-htm.h"
+#include "debug.h"
+#include "sample.h"
+
+struct powerpc_htm {
+ struct auxtrace auxtrace;
+ struct auxtrace_queues queues;
+ struct auxtrace_heap heap;
+ u32 auxtrace_type;
+ struct perf_session *session;
+ struct machine *machine;
+};
+
+static void powerpc_htm_dump_event(size_t len)
+{
+ const char *color = PERF_COLOR_BLUE;
+
+ if (dump_trace) {
+ color_fprintf(stdout, color,
+ ". ... HTM PMU data: size %zu bytes\n", len);
+ }
+}
+
+static int powerpc_htm_process_event(struct perf_session *session __maybe_unused,
+ union perf_event *event __maybe_unused,
+ struct perf_sample *sample __maybe_unused,
+ const struct perf_tool *tool __maybe_unused)
+{
+ return 0;
+}
+
+static int powerpc_htm_process_auxtrace_event(struct perf_session *session __maybe_unused,
+ union perf_event *event,
+ const struct perf_tool *tool __maybe_unused)
+{
+ if (dump_trace)
+ powerpc_htm_dump_event(event->auxtrace.size);
+
+ return 0;
+}
+
+static int powerpc_htm_flush(struct perf_session *session __maybe_unused,
+ const struct perf_tool *tool __maybe_unused)
+{
+ return 0;
+}
+
+static void powerpc_htm_free_events(struct perf_session *session)
+{
+ struct powerpc_htm *htm;
+
+ if (!session || !session->auxtrace)
+ return;
+
+ htm = container_of(session->auxtrace, struct powerpc_htm, auxtrace);
+ auxtrace_queues__free(&htm->queues);
+}
+
+static void powerpc_htm_free(struct perf_session *session)
+{
+ struct powerpc_htm *htm;
+
+ if (!session || !session->auxtrace)
+ return;
+
+ htm = container_of(session->auxtrace, struct powerpc_htm, auxtrace);
+ powerpc_htm_free_events(session);
+ session->auxtrace = NULL;
+ free(htm);
+}
+
+int powerpc_htm_process_auxtrace_info(union perf_event *event,
+ struct perf_session *session)
+{
+ struct perf_record_auxtrace_info *auxtrace_info = &event->auxtrace_info;
+ struct powerpc_htm *htm;
+ int err;
+
+ if (auxtrace_info->header.size < sizeof(struct perf_record_auxtrace_info) +
+ HTM_AUXTRACE_PRIV_FIXED)
+ return -EINVAL;
+
+ htm = zalloc(sizeof(struct powerpc_htm));
+ if (!htm)
+ return -ENOMEM;
+
+ err = auxtrace_queues__init(&htm->queues);
+ if (err) {
+ free(htm);
+ return err;
+ }
+
+ htm->session = session;
+ htm->machine = &session->machines.host;
+ htm->auxtrace.process_event = powerpc_htm_process_event;
+ htm->auxtrace.process_auxtrace_event = powerpc_htm_process_auxtrace_event;
+ htm->auxtrace.flush_events = powerpc_htm_flush;
+ htm->auxtrace.free_events = powerpc_htm_free_events;
+ htm->auxtrace.free = powerpc_htm_free;
+ session->auxtrace = &htm->auxtrace;
+
+ return 0;
+}
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH V2 5/6] tools/perf: Add powerpc HTM auxtrace event processing support
2026-07-20 10:52 ` [PATCH V2 5/6] tools/perf: Add powerpc HTM auxtrace event processing support Athira Rajeev
@ 2026-07-20 11:18 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-07-20 11:18 UTC (permalink / raw)
To: Athira Rajeev; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Implicit truncation of `__u64` to `size_t` on 32-bit architectures.
- [Low] The `auxtrace_type` field in `struct powerpc_htm` is declared but never initialized.
--
--- Patch [5]: [PATCH V2 5/6] tools/perf: Add powerpc HTM auxtrace event processing support ---
commit 87b8929025f59e2555a76806a3d33736b3cb7f18
Author: Athira Rajeev <atrajeev@linux.ibm.com>
tools/perf: Add powerpc HTM auxtrace event processing support
This commit adds the PERF_AUXTRACE_POWERPC_HTM enum value to auxtrace.h and
wires the dispatch in perf_event__process_auxtrace_info() to call
powerpc_htm_process_auxtrace_info(). It also sets the auxtrace_info->type
in htm_info_fill().
> diff --git a/tools/perf/util/powerpc-htm.c b/tools/perf/util/powerpc-htm.c
> new file mode 100644
> index 0000000000000..8d28d02031d6e
> --- /dev/null
> +++ b/tools/perf/util/powerpc-htm.c
> @@ -0,0 +1,115 @@
[ ... ]
> +static void powerpc_htm_dump_event(size_t len)
> +{
> + const char *color = PERF_COLOR_BLUE;
> +
> + if (dump_trace) {
> + color_fprintf(stdout, color,
> + ". ... HTM PMU data: size %zu bytes\n", len);
> + }
> +}
[ ... ]
> +static int powerpc_htm_process_auxtrace_event(struct perf_session *session __maybe_unused,
> + union perf_event *event,
> + const struct perf_tool *tool __maybe_unused)
> +{
> + if (dump_trace)
> + powerpc_htm_dump_event(event->auxtrace.size);
[Severity: Low]
Does this implicitly truncate event->auxtrace.size on 32-bit architectures?
event->auxtrace.size is a __u64, but powerpc_htm_dump_event() accepts a
size_t. This isn't a critical bug since it's only used for debug output,
but could it result in printing a truncated size if the trace exceeds 4GB?
> +
> + return 0;
> +}
[ ... ]
> +int powerpc_htm_process_auxtrace_info(union perf_event *event,
> + struct perf_session *session)
> +{
> + struct perf_record_auxtrace_info *auxtrace_info = &event->auxtrace_info;
> + struct powerpc_htm *htm;
> + int err;
> +
> + if (auxtrace_info->header.size < sizeof(struct perf_record_auxtrace_info) +
> + HTM_AUXTRACE_PRIV_FIXED)
> + return -EINVAL;
> +
> + htm = zalloc(sizeof(struct powerpc_htm));
> + if (!htm)
> + return -ENOMEM;
> +
> + err = auxtrace_queues__init(&htm->queues);
> + if (err) {
> + free(htm);
> + return err;
> + }
> +
> + htm->session = session;
> + htm->machine = &session->machines.host;
[Severity: Low]
Is htm->auxtrace_type intentionally left uninitialized here?
It is declared in struct powerpc_htm, but unlike other fields, it is not
assigned auxtrace_info->type after the allocation.
> + htm->auxtrace.process_event = powerpc_htm_process_event;
> + htm->auxtrace.process_auxtrace_event = powerpc_htm_process_auxtrace_event;
> + htm->auxtrace.flush_events = powerpc_htm_flush;
> + htm->auxtrace.free_events = powerpc_htm_free_events;
> + htm->auxtrace.free = powerpc_htm_free;
> + session->auxtrace = &htm->auxtrace;
> +
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720105218.14277-1-atrajeev@linux.ibm.com?part=5
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V2 6/6] tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records
2026-07-20 10:52 [PATCH V2 0/6] tools/perf: Add powerpc HTM auxtrace support Athira Rajeev
` (4 preceding siblings ...)
2026-07-20 10:52 ` [PATCH V2 5/6] tools/perf: Add powerpc HTM auxtrace event processing support Athira Rajeev
@ 2026-07-20 10:52 ` Athira Rajeev
2026-07-20 11:25 ` sashiko-bot
5 siblings, 1 reply; 13+ messages in thread
From: Athira Rajeev @ 2026-07-20 10:52 UTC (permalink / raw)
To: acme, jolsa, adrian.hunter, maddy, irogers, namhyung
Cc: linux-perf-users, linuxppc-dev, atrajeev, hbathini, tejas05,
tshah, venkat88, usha.r2
powerpc_htm_process_auxtrace_info() reads the (cpu, attr.config) pairs
stored by htm_info_fill() at record time and builds a cpu_configs[]
table. This lets process_auxtrace_event() map each AUX buffer, which
carries event->auxtrace.cpu, to the correct (node, chip, core) target
and write the raw trace data to htm.bin.nX.pX.cX immediately.
PERF_RECORD_SAMPLE events carrying PERF_SAMPLE_RAW data are handled by
process_event(), which extracts memory configuration records and writes
them to translation.nX.pX.cX.
Per-run first-write tracking (htm_target_seen()) ensures each output
file is opened with O_TRUNC on the first write and O_APPEND on
subsequent writes, correctly handling multiple AUX chunks from the same
target and stale files from prior runs.
For perf report -D the AUX buffer size is printed.
Example:
# perf record -C 9 -m,256 \
-e htm/nodalchipindex=2,nodeindex=0,htm_type=1/ sleep 3
# perf report
# ls htm.bin.* translation.*
htm.bin.n0.p2.c0 translation.n0.p2.c0
Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
---
Changes in V2:
- Consolidated: the file-writing, memory-configuration processing, and
address-mapping logic that was spread across old patches 5 to 9 is
rationalised into a single, focused implementation in
tools/perf/util/powerpc-htm.c.
- Synthetic sample generation (old patch 8: PERF_TYPE_SYNTH "htm" event
with logical addresses) and the separate logical-address mapping output
file (old patch 9: .out.l file) are not included in V2. These features
require further discussion on the appropriate abstraction and will be
posted as a follow-on series.
- Physical-to-logical address mapping (old patch 7) is not included in
V2 for the same reason.
- Memory configuration records are now written via process_event()
handling PERF_RECORD_SAMPLE with PERF_SAMPLE_RAW, keyed on the "htm"
PMU name from evsel__pmu_name(). V1 used PERF_SAMPLE_RAW boundary
markers inside the AUX buffer to locate the configuration data.
- AUX buffer processing uses htm_config_for_cpu() to map
event->auxtrace.cpu to the correct (node, chip, core) target using
the (cpu, attr.config) table populated from AUXTRACE_INFO priv[] in
powerpc_htm_process_auxtrace_info(). V1 iterated the evlist at
report time.
- Per-run first-write tracking (htm_target_seen()) uses O_TRUNC on the
first write and O_APPEND on subsequent writes for both htm.bin.* and
translation.* files, correctly handling multiple AUX chunks per target
and stale files from prior runs. V1 always opened with O_TRUNC.
- HTM_MAX_TARGETS compile-time constant is replaced by dynamic allocation
sized to num_events read from AUXTRACE_INFO priv[POWERPC_HTM_NUM_EVENTS].
- Patch is now 6/6 instead of spanning patches 5 to 9.
tools/perf/util/powerpc-htm.c | 312 +++++++++++++++++++++++++++++++++-
1 file changed, 306 insertions(+), 6 deletions(-)
diff --git a/tools/perf/util/powerpc-htm.c b/tools/perf/util/powerpc-htm.c
index 8d28d02031d6..409ab563a0a7 100644
--- a/tools/perf/util/powerpc-htm.c
+++ b/tools/perf/util/powerpc-htm.c
@@ -11,9 +11,12 @@
#include "util/session.h"
#include "color.h"
#include "powerpc-htm.h"
+#include <errno.h>
#include "debug.h"
#include "sample.h"
+struct perf_session;
+
struct powerpc_htm {
struct auxtrace auxtrace;
struct auxtrace_queues queues;
@@ -21,8 +24,52 @@ struct powerpc_htm {
u32 auxtrace_type;
struct perf_session *session;
struct machine *machine;
+
+ /*
+ * Capacity: number of distinct HTM targets (node/chip/core tuples)
+ * recorded, read from auxtrace_info->priv[POWERPC_HTM_NUM_EVENTS] at
+ * init time. All three arrays below are allocated to this size.
+ */
+ int nr_targets;
+
+ /*
+ * Per-run first-write tracking for htm.bin.* and translation.* files.
+ * Each entry is a packed u32: node<<16 | chip<<8 | core.
+ * First write for a given key -> O_TRUNC; subsequent writes -> O_APPEND.
+ */
+ u32 *htm_bin_targets;
+ int nr_htm_bin_targets;
+ u32 *translation_targets;
+ int nr_translation_targets;
+
+ /*
+ * CPU -> attr.config table, populated from auxtrace_info->priv[] at
+ * init time. htm_info_fill() (record side) stores the CPU and config
+ * for each htm evsel; we read them back here to map each
+ * event->auxtrace.cpu to the correct (node, chip, core) config.
+ */
+ struct {
+ int cpu;
+ u64 config;
+ } *cpu_configs;
+ int nr_cpu_configs;
};
+/*
+ * Look up attr.config by the CPU number carried in event->auxtrace.cpu.
+ * Returns 0 if not found.
+ */
+static u64 htm_config_for_cpu(struct powerpc_htm *htm, int cpu)
+{
+ int i;
+
+ for (i = 0; i < htm->nr_cpu_configs; i++) {
+ if (htm->cpu_configs[i].cpu == cpu)
+ return htm->cpu_configs[i].config;
+ }
+ return 0;
+}
+
static void powerpc_htm_dump_event(size_t len)
{
const char *color = PERF_COLOR_BLUE;
@@ -33,22 +80,234 @@ static void powerpc_htm_dump_event(size_t len)
}
}
-static int powerpc_htm_process_event(struct perf_session *session __maybe_unused,
- union perf_event *event __maybe_unused,
- struct perf_sample *sample __maybe_unused,
+#define HTM_MEM_ENTRY_SIZE 32
+
+static inline u32 htm_pack_target(u32 node, u32 chip, u32 core)
+{
+ return (node << 16) | (chip << 8) | core;
+}
+
+static bool htm_target_seen(u32 *targets, int *nr, int capacity, u32 key)
+{
+ int i;
+
+ for (i = 0; i < *nr; i++) {
+ if (targets[i] == key)
+ return true;
+ }
+
+ if (*nr < capacity)
+ targets[(*nr)++] = key;
+ else
+ pr_warning("htm: too many targets (max %d), appending to existing file\n",
+ capacity);
+
+ return false;
+}
+
+/*
+ * Write HTM data to a file.
+ *
+ * mem_maps == 0: AUX bus-trace path -> htm.bin.nX.pX.cX
+ * mem_maps != 0: memory config path -> translation.nX.pX.cX
+ *
+ * htm_target_seen() decides O_TRUNC (first write this run) vs O_APPEND
+ * (subsequent writes), keyed on what this process has already written,
+ * not on whether the file exists on disk.
+ */
+static int write_htm(struct powerpc_htm *htm, void *data, size_t size,
+ u32 node, u32 chip, u32 core, int mem_maps)
+{
+ u32 target_key = htm_pack_target(node, chip, core);
+ char target_file[128];
+ size_t written;
+ int flags;
+ int fd;
+
+ if (!data || !size)
+ return -EINVAL;
+
+ flags = O_CREAT | O_WRONLY | O_NOFOLLOW | O_CLOEXEC;
+
+ if (mem_maps) {
+ uint8_t *byte_ptr = (uint8_t *)data;
+ __be64 *num_entries_ptr;
+ size_t entries;
+ size_t payload;
+
+ if (size < HTM_MEM_ENTRY_SIZE) {
+ pr_err("Malformed memory mapping entry trace segment\n");
+ return -EINVAL;
+ }
+
+ /* Entry count is at offset 0x10; add 1 for the 32-byte header */
+ num_entries_ptr = (__be64 *)(byte_ptr + 0x10);
+ entries = be64_to_cpu(*num_entries_ptr) + 1;
+ payload = entries * HTM_MEM_ENTRY_SIZE;
+
+ if (payload != size) {
+ pr_err("Bad memory mapping data, invalid number of entries\n");
+ return -EINVAL;
+ }
+
+ snprintf(target_file, sizeof(target_file),
+ "translation.n%d.p%d.c%d", node, chip, core);
+ flags |= htm_target_seen(htm->translation_targets,
+ &htm->nr_translation_targets,
+ htm->nr_targets,
+ target_key) ? O_APPEND : O_TRUNC;
+ fd = open(target_file, flags, 0644);
+ if (fd == -1) {
+ pr_err("Failed to open %s: %s\n", target_file, strerror(errno));
+ return -errno;
+ }
+
+ written = write(fd, data, payload);
+ close(fd);
+
+ if (written != payload) {
+ pr_err("Failed to write memory config: expected %zu bytes, wrote %zu\n",
+ payload, written);
+ return -EIO;
+ }
+
+ return 0;
+ }
+
+ /* AUX bus-trace path */
+ snprintf(target_file, sizeof(target_file),
+ "htm.bin.n%d.p%d.c%d", node, chip, core);
+ flags |= htm_target_seen(htm->htm_bin_targets,
+ &htm->nr_htm_bin_targets,
+ htm->nr_targets,
+ target_key) ? O_APPEND : O_TRUNC;
+ fd = open(target_file, flags, 0644);
+ if (fd == -1) {
+ pr_err("Failed to open %s: %s\n", target_file, strerror(errno));
+ return -errno;
+ }
+
+ written = write(fd, data, size);
+ close(fd);
+
+ if (written != size) {
+ pr_err("Failed to write htm trace data: expected %zu bytes, wrote %zu\n",
+ size, written);
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int powerpc_htm_process_event(struct perf_session *session,
+ union perf_event *event,
+ struct perf_sample *sample,
const struct perf_tool *tool __maybe_unused)
{
+ struct powerpc_htm *htm;
+ struct evsel *evsel;
+ u32 node, chip, core;
+ u64 ev_config;
+
+ if (!session || !session->auxtrace || !event || !sample)
+ return 0;
+
+ if (event->header.type != PERF_RECORD_SAMPLE || !sample->raw_data)
+ return 0;
+
+ htm = container_of(session->auxtrace, struct powerpc_htm, auxtrace);
+ evsel = evlist__event2evsel(session->evlist, event);
+
+ if (!evsel || strcmp(evsel__pmu_name(evsel), "htm") != 0)
+ return 0;
+
+ ev_config = evsel->core.attr.config;
+ node = (ev_config >> 4) & 0xff;
+ chip = (ev_config >> 12) & 0xff;
+ core = (ev_config >> 20) & 0xff;
+
+ /*
+ * raw_size includes 4 bytes of u64 alignment padding added by the
+ * kernel. Subtract sizeof(u32) to recover the true payload byte count.
+ */
+ if (write_htm(htm, sample->raw_data,
+ sample->raw_size - sizeof(uint32_t),
+ node, chip, core, 1) < 0) {
+ pr_err("Failed to write memory translation block\n");
+ return -EIO;
+ }
+
return 0;
}
-static int powerpc_htm_process_auxtrace_event(struct perf_session *session __maybe_unused,
+static int powerpc_htm_process_auxtrace_event(struct perf_session *session,
union perf_event *event,
const struct perf_tool *tool __maybe_unused)
{
+ struct powerpc_htm *htm;
+ struct auxtrace_buffer *buffer;
+ off_t data_offset;
+ u32 node, chip, core;
+ u64 ev_config;
+ int fd;
+ int err;
+
+ if (!session || !session->auxtrace)
+ return 0;
+
+ htm = container_of(session->auxtrace, struct powerpc_htm, auxtrace);
+ fd = perf_data__fd(session->data);
+
+ if (perf_data__is_pipe(session->data)) {
+ data_offset = 0;
+ } else {
+ data_offset = lseek(fd, 0, SEEK_CUR);
+ if (data_offset == -1)
+ return -errno;
+ }
+
+ /*
+ * Queue the buffer and get back a pointer to it. We immediately load
+ * and write the data so htm.bin.* exists on disk before subsequent
+ * patches invoke htmdecode during the same session pass.
+ */
+ err = auxtrace_queues__add_event(&htm->queues, session, event,
+ data_offset, &buffer);
+ if (err)
+ return err;
+
+ if (!buffer)
+ return 0;
+
+ /*
+ * Map event->auxtrace.cpu -> attr.config using the table built from
+ * auxtrace_info->priv[] at init time. This is reliable because
+ * htm_info_fill() stored the exact (cpu, config) pair for each evsel
+ * at record time, no CPU map or evlist iteration needed here.
+ */
+ ev_config = htm_config_for_cpu(htm, (int)event->auxtrace.cpu);
+ if (!ev_config) {
+ pr_err("htm: no config found for auxtrace cpu %u\n",
+ event->auxtrace.cpu);
+ return 0;
+ }
+
+ node = (ev_config >> 4) & 0xff;
+ chip = (ev_config >> 12) & 0xff;
+ core = (ev_config >> 20) & 0xff;
+
+ if (!auxtrace_buffer__get_data(buffer, fd)) {
+ pr_err("Failed to read AUX buffer data\n");
+ return -ENOMEM;
+ }
+
if (dump_trace)
- powerpc_htm_dump_event(event->auxtrace.size);
+ powerpc_htm_dump_event(buffer->size);
- return 0;
+ err = write_htm(htm, buffer->data, buffer->size, node, chip, core, 0);
+ auxtrace_buffer__put_data(buffer);
+
+ return err < 0 ? err : 0;
}
static int powerpc_htm_flush(struct perf_session *session __maybe_unused,
@@ -78,6 +337,9 @@ static void powerpc_htm_free(struct perf_session *session)
htm = container_of(session->auxtrace, struct powerpc_htm, auxtrace);
powerpc_htm_free_events(session);
session->auxtrace = NULL;
+ free(htm->cpu_configs);
+ free(htm->htm_bin_targets);
+ free(htm->translation_targets);
free(htm);
}
@@ -86,6 +348,8 @@ int powerpc_htm_process_auxtrace_info(union perf_event *event,
{
struct perf_record_auxtrace_info *auxtrace_info = &event->auxtrace_info;
struct powerpc_htm *htm;
+ u64 num_events;
+ u64 i;
int err;
if (auxtrace_info->header.size < sizeof(struct perf_record_auxtrace_info) +
@@ -102,6 +366,42 @@ int powerpc_htm_process_auxtrace_info(union perf_event *event,
return err;
}
+ htm->auxtrace_type = auxtrace_info->priv[POWERPC_HTM_PMU_TYPE];
+ num_events = auxtrace_info->priv[POWERPC_HTM_NUM_EVENTS];
+
+ /*
+ * All three arrays are sized to num_events, the exact count of HTM
+ * targets written by htm_info_fill() at record time. This replaces the
+ * old compile-time HTM_MAX_TARGETS constant so the limit is always
+ * consistent with what was actually recorded.
+ */
+ htm->nr_targets = (int)num_events;
+ htm->cpu_configs = calloc(num_events, sizeof(*htm->cpu_configs));
+ htm->htm_bin_targets = calloc(num_events, sizeof(*htm->htm_bin_targets));
+ htm->translation_targets = calloc(num_events, sizeof(*htm->translation_targets));
+ if (!htm->cpu_configs || !htm->htm_bin_targets || !htm->translation_targets) {
+ free(htm->cpu_configs);
+ free(htm->htm_bin_targets);
+ free(htm->translation_targets);
+ auxtrace_queues__free(&htm->queues);
+ free(htm);
+ return -ENOMEM;
+ }
+
+ /*
+ * Read (cpu, config) pairs from priv[]. These were written by
+ * htm_info_fill() at record time, one pair per htm evsel in evlist
+ * order. Keying by CPU lets process_auxtrace_event() look up the
+ * correct attr.config for each AUX buffer using event->auxtrace.cpu.
+ */
+ for (i = 0; i < num_events; i++) {
+ htm->cpu_configs[i].cpu =
+ (int)auxtrace_info->priv[POWERPC_HTM_EVENT_DATA + i * 2];
+ htm->cpu_configs[i].config =
+ auxtrace_info->priv[POWERPC_HTM_EVENT_DATA + i * 2 + 1];
+ htm->nr_cpu_configs++;
+ }
+
htm->session = session;
htm->machine = &session->machines.host;
htm->auxtrace.process_event = powerpc_htm_process_event;
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH V2 6/6] tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records
2026-07-20 10:52 ` [PATCH V2 6/6] tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records Athira Rajeev
@ 2026-07-20 11:25 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-07-20 11:25 UTC (permalink / raw)
To: Athira Rajeev; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Incorrect return value in `htm_target_seen()` causes trace file truncation instead of appending when target capacity is exceeded.
- [High] Missing bounds check on dynamic array portion of `auxtrace_info` leads to out-of-bounds read.
- [High] Integer underflow when processing `sample->raw_size` leads to bypassed length checks and out-of-bounds read.
- [High] Direct dereferencing of a 64-bit pointer causes unaligned memory access.
- [High] Integer truncation on 32-bit platforms leads to a severe heap buffer overflow.
- [Medium] Processing multiple AUXTRACE_INFO events overwrites `session->auxtrace`, leaking memory.
- [Low] Missing explicit POSIX libc header inclusions causes musl libc build failures.
--
--- Patch [6]: [PATCH V2 6/6] tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records ---
commit 30554a3a06e5c4703030155cf5ee8abbdc6f50e6
Author: Athira Rajeev <atrajeev@linux.ibm.com>
tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records
This commit adds perf tool support for processing powerpc HTM AUXTRACE records.
It introduces logic to map CPU configurations to trace targets and writes HTM
data directly to files. Memory configuration records are also extracted from
raw sample data and processed.
> diff --git a/tools/perf/util/powerpc-htm.c b/tools/perf/util/powerpc-htm.c
> index 8d28d02031d6e..28166f7f7463b 100644
> --- a/tools/perf/util/powerpc-htm.c
> +++ b/tools/perf/util/powerpc-htm.c
> @@ -11,9 +11,12 @@
> #include "util/session.h"
> #include "color.h"
> #include "powerpc-htm.h"
> +#include <errno.h>
> #include "debug.h"
> #include "sample.h"
>
> +struct perf_session;
> +
[Severity: Low]
Will this fail to compile on musl libc? The code uses POSIX standard functions
like open, write, close, calloc, and snprintf, but standard headers like
<fcntl.h>, <unistd.h>, <stdlib.h>, <stdio.h>, and <string.h> are not included.
[ ... ]
> +static bool htm_target_seen(u32 *targets, int *nr, int capacity, u32 key)
> +{
> + int i;
> +
> + for (i = 0; i < *nr; i++) {
> + if (targets[i] == key)
> + return true;
> + }
> +
> + if (*nr < capacity)
> + targets[(*nr)++] = key;
> + else
> + pr_warning("htm: too many targets (max %d), appending to existing file\n",
> + capacity);
> +
> + return false;
> +}
[Severity: High]
If we exceed the target capacity, does this return false and inadvertently
truncate the file? The warning message states it is appending, but a false
return causes write_htm() to select O_TRUNC instead of O_APPEND.
[ ... ]
> +static int write_htm(struct powerpc_htm *htm, void *data, size_t size,
> + u32 node, u32 chip, u32 core, int mem_maps)
> +{
> + u32 target_key = htm_pack_target(node, chip, core);
> + char target_file[128];
> + size_t written;
> + int flags;
> + int fd;
> +
> + if (!data || !size)
> + return -EINVAL;
> +
> + flags = O_CREAT | O_WRONLY | O_NOFOLLOW | O_CLOEXEC;
> +
> + if (mem_maps) {
> + uint8_t *byte_ptr = (uint8_t *)data;
> + __be64 *num_entries_ptr;
> + size_t entries;
> + size_t payload;
> +
> + if (size < HTM_MEM_ENTRY_SIZE) {
> + pr_err("Malformed memory mapping entry trace segment\n");
> + return -EINVAL;
> + }
> +
> + /* Entry count is at offset 0x10; add 1 for the 32-byte header */
> + num_entries_ptr = (__be64 *)(byte_ptr + 0x10);
> + entries = be64_to_cpu(*num_entries_ptr) + 1;
[Severity: High]
Can this dereference cause a SIGBUS on architectures with strict alignment
requirements? Since byte_ptr comes from sample->raw_data, it might only be
4-byte aligned due to the preceding u32 raw_size in the header. Should this
use get_unaligned_be64() instead?
[ ... ]
> +static int powerpc_htm_process_event(struct perf_session *session,
> + union perf_event *event,
> + struct perf_sample *sample,
> const struct perf_tool *tool __maybe_unused)
> {
> + struct powerpc_htm *htm;
> + struct evsel *evsel;
> + u32 node, chip, core;
> + u64 ev_config;
> +
> + if (!session || !session->auxtrace || !event || !sample)
> + return 0;
> +
> + if (event->header.type != PERF_RECORD_SAMPLE || !sample->raw_data)
> + return 0;
> +
> + htm = container_of(session->auxtrace, struct powerpc_htm, auxtrace);
> + evsel = evlist__event2evsel(session->evlist, event);
> +
> + if (!evsel || strcmp(evsel__pmu_name(evsel), "htm") != 0)
> + return 0;
> +
> + ev_config = evsel->core.attr.config;
> + node = (ev_config >> 4) & 0xff;
> + chip = (ev_config >> 12) & 0xff;
> + core = (ev_config >> 20) & 0xff;
> +
> + /*
> + * raw_size includes 4 bytes of u64 alignment padding added by the
> + * kernel. Subtract sizeof(u32) to recover the true payload byte count.
> + */
> + if (write_htm(htm, sample->raw_data,
> + sample->raw_size - sizeof(uint32_t),
> + node, chip, core, 1) < 0) {
[Severity: High]
If sample->raw_size is smaller than 4 (e.g., 0), does this unchecked
subtraction underflow to a massive positive size_t value? If so, this massive
size will bypass the boundary check in write_htm(), leading to out-of-bounds
reads when evaluating the entry count.
[ ... ]
> int powerpc_htm_process_auxtrace_info(union perf_event *event,
> struct perf_session *session)
> {
> struct perf_record_auxtrace_info *auxtrace_info = &event->auxtrace_info;
> struct powerpc_htm *htm;
> + u64 num_events;
> + u64 i;
> int err;
>
> if (auxtrace_info->header.size < sizeof(struct perf_record_auxtrace_info) +
> HTM_AUXTRACE_PRIV_FIXED)
> return -EINVAL;
[Severity: High]
Does this size check fail to validate the variable-length dynamic array of
num_events pairs? If the payload provides a small header.size but a large
num_events value, the subsequent initialization loop might read completely out
of bounds of the memory mapped event buffer.
>
> + htm = zalloc(sizeof(struct powerpc_htm));
> + if (!htm)
> + return -ENOMEM;
[Severity: Medium]
If a perf.data file contains multiple POWERPC_HTM auxtrace info events, does
this blindly allocate a new powerpc_htm structure and overwrite
session->auxtrace later? This would lead to a memory leak of the previous
context during session teardown.
>
> err = auxtrace_queues__init(&htm->queues);
> if (err) {
> free(htm);
> return err;
> }
>
> + htm->auxtrace_type = auxtrace_info->priv[POWERPC_HTM_PMU_TYPE];
> + num_events = auxtrace_info->priv[POWERPC_HTM_NUM_EVENTS];
> +
> + /*
> + * All three arrays are sized to num_events, the exact count of HTM
> + * targets written by htm_info_fill() at record time. This replaces the
> + * old compile-time HTM_MAX_TARGETS constant so the limit is always
> + * consistent with what was actually recorded.
> + */
> + htm->nr_targets = (int)num_events;
> + htm->cpu_configs = calloc(num_events, sizeof(*htm->cpu_configs));
> + htm->htm_bin_targets = calloc(num_events, sizeof(*htm->htm_bin_targets));
> + htm->translation_targets = calloc(num_events, sizeof(*htm->translation_targets));
[Severity: High]
On 32-bit platforms, does the 64-bit num_events value get implicitly
truncated by the 32-bit size_t argument in calloc()? If truncated, the
subsequent loop using the full 64-bit num_events limit could cause a severe
heap buffer overflow.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720105218.14277-1-atrajeev@linux.ibm.com?part=6
^ permalink raw reply [flat|nested] 13+ messages in thread