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

[sashiko-bot@kernel.org]

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?

[ ... ]

> +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?

[ ... ]

> +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?

[ ... ]

> +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