Re: [PATCH V2 1/6] powerpc/perf: Add HTM PMU driver to expose Hardware Trace Macro data

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

> On 20 Jul 2026, at 4:30 PM, sashiko-bot@kernel.org wrote:
> 
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Hardware Resource Leak on H_LONG_BUSY_* in Teardown Paths
> - [High] Missing Global Target Collision Detection
> - [High] Hypervisor HCALL Flooding via Misunderstood pmu->add Semantics
> --
> 
> --- Patch [1]: [PATCH V2 1/6] powerpc/perf: Add HTM PMU driver to expose Hardware Trace Macro data ---
> commit 5f73062ed0507987b187b59e32b4100e6330618a
> Author: Athira Rajeev <atrajeev@linux.ibm.com>
> 
> powerpc/perf: Add HTM PMU driver to expose Hardware Trace Macro data
> 
> The H_HTM hypervisor call (hcall) provides an interface to the Hardware
> Trace Macro (HTM) function on POWER systems.  HTM captures hardware-level
> trace data for a specific node/chip/core target within a logical partition.
> 
>> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
>> new file mode 100644
>> index 0000000000000..5d3aa9c7fd8d1
>> --- /dev/null
>> +++ b/arch/powerpc/perf/htm-perf.c
> 
> [ ... ]
> 
>> +static ssize_t htm_return_check(int rc)
>> +{
>> + switch (rc) {
>> + case H_SUCCESS:
>> + case H_PARTIAL:
>> + return 1;
>> + case H_NOT_AVAILABLE:
>> + return 0;
>> + case H_BUSY:
>> + /* Transient busy: retry loop will spin up to MAX_RETRIES */
>> + return -EBUSY;
>> + case H_LONG_BUSY_ORDER_1_MSEC:
>> + case H_LONG_BUSY_ORDER_10_MSEC:
>> + case H_LONG_BUSY_ORDER_100_MSEC:
>> + case H_LONG_BUSY_ORDER_1_SEC:
>> + case H_LONG_BUSY_ORDER_10_SEC:
>> + case H_LONG_BUSY_ORDER_100_SEC:
>> + /*
>> + * Hypervisor requests a long delay before retry (1ms-100s).
>> + * Spinning in a kernel retry loop for this duration risks
>> + * deadlocks in atomic contexts.  Return -EAGAIN so callers
>> + * exit immediately; the operation will fail and userspace
>> + * can retry the perf_event_open() call.
>> + * Note: all retry loops check ret == -EBUSY only, so
>> + * -EAGAIN exits without retrying, this is intentional.
>> + */
>> + return -EAGAIN;
> 
> [Severity: High]
> The comment mentions that returning -EAGAIN will cause the perf_event_open()
> call to fail so userspace can retry. Is this accurate for paths called
> during event scheduling?
> 
> When htm_return_check() is called from htm_event_add(), -EAGAIN is returned
> to the perf core's pmu->add() caller (event_sched_in). The perf core treats
> -EAGAIN from pmu->add as a transient scheduling failure and will repeatedly
> attempt to schedule the event on subsequent timer ticks or context switches.
> 
> Will this flood the hypervisor with H_HTM_OP_CONFIGURE hcalls exactly when
> it is requesting a long backoff delay?
> 

Thanks for the feedback
I will be sending a V3 with handles possibility of long repeated attempt by not using
EAGAIN

> [ ... ]
> 
>> +static int htm_event_init(struct perf_event *event)
>> +{
>> + u64 config = event->attr.config;
>> + struct htm_config cfg;
>> +
>> + if (event->attr.inherit)
>> + return -EOPNOTSUPP;
> 
> [ ... ]
> 
>> + parse_htm_config(config, &cfg);
>> + switch (cfg.htmtype) {
>> + case HTM_CORE:
>> + case HTM_NEST:
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + /* Allocate per-event private state; freed via event->destroy */
>> + event->pmu_private = kzalloc(sizeof(struct htm_target_id), GFP_KERNEL);
>> + if (!event->pmu_private)
>> + return -ENOMEM;
> 
> [Severity: High]
> Does this missing global target collision detection allow hardware state
> corruption?
> 
> The driver sets PERF_PMU_CAP_EXCLUSIVE, but the perf core only enforces this
> within the same per-CPU context. If two users concurrently create events for
> the exact same HTM hardware resource (identical node/chip/core) on different
> CPUs, they would bypass that check.
> 
> Without a global registry of active targets here in htm_event_init(), could
> conflicting hcalls be issued to the identical hardware resource, clobbering
> the hypervisor configuration?

I am handling this in patch 2
Will mention this specifically in V3

> 
> [ ... ]
> 
>> +static void htm_event_stop(struct perf_event *event, int flags)
>> +{
>> + int rc, ret, retries = 0;
>> + struct htm_config cfg;
>> + struct htm_target_id *target = event->pmu_private;
>> +
>> + /*
>> + * Ignore context-switch-out stops. The perf core passes
>> + * PERF_EF_UPDATE on context-switch-out. Do not stop the hcall.
>> + */
>> + if (flags & PERF_EF_UPDATE)
>> + return;
>> +
>> + /* Not tracing, nothing to stop */
>> + if (target->tracing_active == HTM_TRACING_INACTIVE)
>> + return;
>> +
>> + cfg = target->cfg;
>> +
>> + /* Only retry on transient H_BUSY; H_LONG_BUSY_* (-EAGAIN) exits immediately */
>> + do {
>> + rc = htm_hcall_wrapper(htmflags, cfg.nodeindex, cfg.nodalchipindex,
>> +       cfg.coreindexonchip, cfg.htmtype,
>> +       H_HTM_OP_STOP, 0, 0, 0);
>> + ret = htm_return_check(rc);
>> + } while (ret == -EBUSY && ++retries < MAX_RETRIES);
> 
> [Severity: High]
> If the hypervisor returns H_LONG_BUSY_* (which maps to -EAGAIN), the loop
> exits immediately. Since htm_event_stop() returns void, it cannot defer
> the cleanup or notify the perf core. Does this permanently leave the trace
> started in the hypervisor?

V3 will address change to:
- best handle the fail and log the error if still any HTM configure/deconfigure/start/stop fails.
- Update the approach in comments too

Thanks
Athira
> 
> [ ... ]
> 
>> +static int htm_event_add(struct perf_event *event, int flags)
>> +{
> 
> [ ... ]
> 
>> + /*
>> + * Start tracing via the .start callback so the standard
>> + * PERF_EF_START / ioctl(ENABLE) path is honoured.
>> + */
>> + if (flags & PERF_EF_START) {
>> + htm_event_start(event, 0); /* flags=0: not a context switch */
>> + if (target->tracing_active == HTM_TRACING_INACTIVE) {
>> + /* Start failed, deconfigure to avoid resource leak */
>> + retries = 0;
>> + do {
>> + rc = htm_hcall_wrapper(htmflags, cfg.nodeindex,
>> +       cfg.nodalchipindex, cfg.coreindexonchip,
>> +       cfg.htmtype, H_HTM_OP_DECONFIGURE, 0, 0, 0);
>> + ret = htm_return_check(rc);
>> + } while (ret == -EBUSY && ++retries < MAX_RETRIES);
>> + return -EIO;
>> + }
>> + }
>> +
>> + return 0;
>> +}
> 
> [Severity: High]
> Similar to htm_event_stop(), if H_HTM_OP_DECONFIGURE returns a long busy
> status here in the error rollback path, the retry loop exits immediately
> and returns -EIO. Does this leak the configured hypervisor resource?
> 
>> +
>> +static void htm_event_del(struct perf_event *event, int flags)
>> +{
>> + int rc, ret, retries = 0;
>> + struct htm_target_id *target = event->pmu_private;
>> + struct htm_config cfg = target->cfg;
>> +
>> + /* Stop tracing, flags=0 so htm_event_stop issues the hcall */
>> + htm_event_stop(event, 0);
>> +
>> + /* Deconfigure */
>> + do {
>> + rc = htm_hcall_wrapper(htmflags, cfg.nodeindex, cfg.nodalchipindex,
>> +       cfg.coreindexonchip, cfg.htmtype,
>> +       H_HTM_OP_DECONFIGURE, 0, 0, 0);
>> + ret = htm_return_check(rc);
>> + } while (ret == -EBUSY && ++retries < MAX_RETRIES);
> 
> [Severity: High]
> If this loop exits on -EAGAIN, the resource is silently abandoned without
> deferring cleanup. The kernel will then free event->pmu_private during
> event->destroy. Could this result in a permanent leak of shared hypervisor
> HTM resources, potentially leading to a denial of service for hardware
> tracing?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260720104447.11843-1-atrajeev@linux.ibm.com?part=1