Re: [PATCH V4 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 1 potential issue(s) to consider:
- [High] Unbalanced H_HTM_OP_DECONFIGURE hcalls in htm_event_del can hijack concurrent trace sessions and cause kernel log spam.
--

--- Patch [1]: [PATCH V4 1/6] powerpc/perf: Add HTM PMU driver to expose Hardware Trace Macro data ---
commit ac0e563af1a2f5f6278332191b43b79a4c0d0029
Author: Athira Rajeev <atrajeev@linux.ibm.com>

powerpc/perf: Add HTM PMU driver to expose Hardware Trace Macro data

This commit introduces a new "htm" Performance Monitoring Unit (PMU) driver
for POWER systems. It exposes HTM control via the standard perf interface,
allowing node/chip/core scoped trace configuration.

> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
> new file mode 100644
> index 0000000000000..7ad592f26bb46
> --- /dev/null
> +++ b/arch/powerpc/perf/htm-perf.c

[ ... ]

> +static int htm_event_add(struct perf_event *event, int flags)
> +{

[ ... ]

> +	do {
> +		rc = htm_hcall_wrapper(htmflags, cfg.nodeindex, cfg.nodalchipindex,
> +				       cfg.coreindexonchip, cfg.htmtype,
> +				       H_HTM_OP_CONFIGURE, param1, param2, 0);
> +		ret = htm_return_check(rc);
> +	} while (ret == -EBUSY && ++retries < MAX_RETRIES);
> +
> +	if (ret <= 0) {
> +		perf_event_disable_inatomic(event);
> +		return 0;
> +	}

[Severity: High]
If H_HTM_OP_CONFIGURE fails here (e.g. returning -EAGAIN on a long busy),
does returning 0 trick the perf core into considering the event successfully
added?

Since the perf core assumes the event is successfully added, it will
eventually call htm_event_del(), which unconditionally deconfigures the
resource.

[ ... ]

> +			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);
> +			if (ret <= 0) {
> +				pr_err("DECONFIGURE failed in htm event add (ret=%d) node:%u chip:%u core:%u type:%u;\n",
> +						ret, cfg.nodeindex, cfg.nodalchipindex, cfg.coreindexonchip, cfg.htmtype);
> +			}
> +			perf_event_disable_inatomic(event);
> +			return 0;

[Severity: High]
Similarly, if the start hcall fails, the driver explicitly issues a
DECONFIGURE here but still returns 0. Will this cause htm_event_del() to
incorrectly issue a second DECONFIGURE later?

[ ... ]

> +static void htm_event_del(struct perf_event *event, int flags)
> +{

[ ... ]

> +	htm_event_stop(event, 0);
> +
> +	/*
> +	 * Deconfigure the hardware resource.  Only H_BUSY is retried.
> +	 * If H_LONG_BUSY_* or any other error is returned, the resource
> +	 * cannot be reclaimed in this atomic context; log the failure so it
> +	 * is visible in the kernel log.
> +	 */
> +	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);
> +	if (ret <= 0) {
> +		pr_err("DECONFIGURE failed in htm event del (ret=%d) node:%u chip:%u core:%u type:%u;\n",
> +				ret, cfg.nodeindex, cfg.nodalchipindex, cfg.coreindexonchip, cfg.htmtype);
> +	}
> +	/* pmu_private freed by event->destroy = reset_htm_active */
> +}

[Severity: High]
Can this unconditional DECONFIGURE in htm_event_del() hijack or destroy a
concurrent trace session?

If htm_event_add() previously failed and returned 0, this path is still
reached. Because this patch permits concurrent opens for the same HTM target,
could this double deconfigure silently destroy a valid trace session established
by another CPU, in addition to spamming the kernel log with pr_err() messages?

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