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

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

> 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 <atrajeev@linux.ibm.com>
> 
> 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.
> 
>> 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
> 
>> [ ... ]
> 
>> +static ssize_t htm_return_check(int rc)
>> +{
>> + switch (rc) {
>> + case H_SUCCESS:
>> + break;
> 
>> [ ... ]
> 
>> + 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.
> 
>> [ ... ]
> 
>> +static int htm_event_init(struct perf_event *event)
>> +{
>> + struct hw_perf_event *hwc = &event->hw;
>> + u64 config = event->attr.config;
>> + u32 htmtype;
> 
>> [ ... ]
> 
>> + 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?
> 
>> [ ... ]
> 
>> +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.
> 
>> + 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.
> 
>> + 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;
> 
>> [ ... ]
> 
>> + /* 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?
> 
>> [ ... ]
> 
>> +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.
> 
>> +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.
> 
>> +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.
> 
>> + 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