Re: [PATCH 1/5] powerpc/htm: Add interface to expose HTM trace data via perf

On 1 Jul 2026, at 2:20 PM, sashiko-bot@kernel.org wrote:

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Missing Firmware Feature Check allows PMU to register on bare-metal systems.
- [High] Improper Error Handling treats unhandled hypervisor errors as success.
- [High] Resource leak when `H_HTM_OP_START` fails.
- [High] Missing cpumask attribute leads to concurrent identical hypercalls.
- [Medium] Retry loops spin endlessly on deterministic errors.
- [Medium] PMU API violation for start/stop operations.
- [Medium] Improper handling of `sample_freq` corrupts the sampling logic.

Thanks for the valuable review comments.

I will be sending a V2 with changes to address:
- missing firmware feature check
- Error handling in status check
- Handling concurrent hupervisor calls
- Fixing retry loop
- Adding start/stop callbacks
- Remove the sample freq
- Resource leak in H_HTM_OP_START fail cases

Thanks
Athira

--

commit 400fb092e994f5871c30e25b796b6ab855238ea4
Author: Athira Rajeev [off-list ref]

powerpc/htm: Add interface to expose HTM trace data via perf

H_HTM (Hardware Trace Macro) hypervisor call is an HCALL to export data
from Hardware Trace Macro (HTM) function. Add support for setup,
configuration and control of HTM function via PMU.

quoted

diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
new file mode 100644
--- /dev/null
+++ b/arch/powerpc/perf/htm-perf.c

quoted

[ ... ]

quoted

+static ssize_t htm_return_check(int rc)
+{
+ switch (rc) {
+ case H_SUCCESS:
+ break;

quoted

[ ... ]

quoted

+ case H_AUTHORITY:
+ return -EPERM;
+ }
+
+ /*
+ * Return 1 for H_SUCCESS/H_PARTIAL
+ */
+ return 1;
+out:
+ return -EINVAL;
+}

[Severity: High]
Will unhandled hypercall return codes fall through the switch statement
and return 1 (success)? If the hypervisor returns an unhandled error code,
this code might silently interpret critical failures as successes.

quoted

[ ... ]

quoted

+static int htm_event_init(struct perf_event *event)
+{
+ struct hw_perf_event *hwc = &event->hw;
+ u64 config = event->attr.config;
+ u32 htmtype;

quoted

[ ... ]

quoted

+ if (event->attr.freq) {
+ hwc->sample_period = event->attr.sample_period;
+ local64_set(&hwc->period_left, hwc->sample_period);
+ hwc->last_period = hwc->sample_period;
+ event->attr.freq = 0;
+ }
+
+ return 0;
+}

[Severity: Medium]
Does copying the target frequency into the sample period break the sampling
logic? Since frequency-based sampling does not appear to be supported, should
this return -EINVAL instead of disabling dynamic frequency adjustment?

quoted

[ ... ]

quoted

+static int htm_event_add(struct perf_event *event, int flags)
+{
+ int rc, ret;
+ unsigned long param1 = -1, param2 = -1;
+ int retries = 0;
+ u64 config = event->attr.config;
+ u32 htmtype, nodeindex, nodalchipindex, coreindexonchip;
+
+ /*
+ * Invoke H_HTM call with:
+ * operation as htm configure (H_HTM_OP_CONFIGURE)
+ * last three values are unused, hence set to zero
+ */
+ htmtype = config & 0xf;
+ nodeindex = (config >> 4) & 0xff;
+ nodalchipindex = (config >> 12) & 0xff;
+ coreindexonchip = (config >> 20) & 0xff;
+ do {
+ rc = htm_hcall_wrapper(htmflags, nodeindex, nodalchipindex, coreindexonchip,
+ htmtype, H_HTM_OP_CONFIGURE, param1, param2, 0);
+ ret = htm_return_check(rc);
+ } while (ret <= 0 && ++retries < 100);

[Severity: Medium]
Does this loop retry 100 times for deterministic errors like -EINVAL and
-EPERM? The condition ret <= 0 causes it to spin needlessly on errors that
will not succeed on retry.

quoted

+ if (ret <= 0)
+ return -1;
+
+ /* Reset retries */
+ retries = 0;
+
+ /*
+ * Invoke H_HTM call with:
+ * operation as htm  start (H_HTM_OP_START)
+ * last three values are unused, hence set to zero
+ */
+ do {
+ rc = htm_hcall_wrapper(htmflags, nodeindex, nodalchipindex, coreindexonchip,
+ htmtype, H_HTM_OP_START, 0, 0, 0);
+ ret = htm_return_check(rc);
+ } while (ret == -EBUSY && ++retries < 100);
+
+ if (htm_return_check(rc) <= 0)
+ return -1;

[Severity: High]
If H_HTM_OP_START fails, does this return without cleaning up the
configuration? Because event_add fails, the perf core aborts event creation
and won't call event_del, potentially leaving the hypervisor state permanently
configured and leaking tracing resources.

[Severity: Medium]
Is it expected to execute the start operation unconditionally during event_add
instead of using the standard pmu->start callback? If the event is created in
a disabled state, it will immediately start tracing anyway.

quoted

+ return 0;
+}
+
+static void htm_event_del(struct perf_event *event, int flags)
+{
+ long rc;
+ int ret;
+ int retries = 0;
+ u64 config = event->attr.config;
+ u32 htmtype, nodeindex, nodalchipindex, coreindexonchip;

quoted

[ ... ]

quoted

+ /* Reset retries */
+ retries = 0;
+
+ /*
+ * Invoke H_HTM call with:
+ * operation as htm configure (H_HTM_OP_DECONFIGURE)
+ * last three values are unused, hence set to zero
+ */
+ do {
+ rc = htm_hcall_wrapper(htmflags, nodeindex, nodalchipindex, coreindexonchip,
+ htmtype, H_HTM_OP_DECONFIGURE, 0, 0, 0);
+ ret = htm_return_check(rc);
+ } while (ret <= 0 && ++retries < 100);
+}

[Severity: Medium]
Similar to the configure path, does this loop needlessly spin on deterministic
errors like -EINVAL and -EPERM?

quoted

[ ... ]

quoted

+static void htm_event_stop(struct perf_event *event, int flags)
+{
+}

[Severity: Medium]
Does providing an empty stop callback prevent users from disabling tracing via
the standard perf API? ioctl(PERF_EVENT_IOC_DISABLE) relies on this callback
to pause the hardware.

quoted

+static struct pmu htm_pmu = {
+ .task_ctx_nr = perf_invalid_context,
+
+ .name = "htm",
+ .attr_groups = attr_groups,
+ .event_init  = htm_event_init,
+ .add         = htm_event_add,
+ .del         = htm_event_del,
+ .read        = htm_event_read,
+ .start     = htm_event_start,
+ .stop     = htm_event_stop,
+ .capabilities = PERF_PMU_CAP_NO_EXCLUDE | PERF_PMU_CAP_EXCLUSIVE,
+};

[Severity: High]
Does this system-wide PMU require a cpumask attribute to prevent concurrent
access? Without a cpumask, running perf stat -a might attempt to open and
start the HTM event on every CPU simultaneously, leading to race conditions
on the hypervisor side for the same node/chip target.

quoted

+static int htm_init(void)
+{
+ int r;
+
+ /* This driver is intended only for L1 host. */
+ if (is_kvm_guest()) {
+ pr_debug("Only supported for L1 host system\n");
+ return -ENODEV;
+ }
+
+ r = perf_pmu_register(&htm_pmu, htm_pmu.name, -1);

[Severity: High]
Can this PMU register on bare-metal systems (PowerNV/OPAL) where the PAPR
hypervisor isn't present? It might be necessary to verify the presence of the
hypervisor using firmware_has_feature(FW_FEATURE_LPAR) to prevent system
traps or panics when executing H_HTM.

quoted

+ if (r)
+ return r;
+
+ return 0;
+}
+
+device_initcall(htm_init);

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