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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing state reset in htm_event_del() causes spurious STOP and DECONFIGURE hcalls on subsequent htm_event_add() failures.
--

--- Patch [1]: [PATCH V5 1/6] powerpc/perf: Add HTM PMU driver to expose Hardware Trace Macro data ---
commit 482e76cb25a86207519e7500e8ca689e3b600e86
Author: Athira Rajeev <[email protected]>

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

This commit adds a new PMU driver for the Hardware Trace Macro (HTM) on
POWER systems. It maps the PMU lifecycle to HTM hypervisor calls and 
implements state tracking to handle hcall retries and error conditions.

> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
> new file mode 100644
> index 0000000000000..cbe4f62813ee0
> --- /dev/null
> +++ b/arch/powerpc/perf/htm-perf.c
[ ... ]
> +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;
[ ... ]
> +	htm_event_stop(event, 0);
> +
> +	/*
> +	 * Only issue H_HTM_OP_DECONFIGURE if this driver successfully issued
> +	 * the paired H_HTM_OP_CONFIGURE.  If htm_event_add() failed before or
> +	 * during CONFIGURE (configured == 0), there is nothing to tear down.
> +	 * Issuing DECONFIGURE without a prior CONFIGURE would silently destroy
> +	 * a concurrent trace session that owns the same hardware target and
> +	 * produce spurious pr_err() noise for an expected H_STATE / H_NOT_AVAILABLE
> +	 * response.
> +	 */
> +	if (!target->configured)
> +		return;
> +
> +	/*
> +	 * 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]
Does this code need to reset target->configured to 0 after issuing the
H_HTM_OP_DECONFIGURE hcall?

If Process A disables an event via ioctl(PERF_EVENT_IOC_DISABLE),
htm_event_del() correctly deconfigures the hardware but leaves
target->configured as 1.

If Process B then opens and enables the same HTM target, it takes ownership.
If Process A subsequently re-enables its event, the H_HTM_OP_CONFIGURE in
htm_event_add() will fail. When Process A's htm_event_del() runs on the
error path, it will see the stale configured == 1 state and spuriously issue
H_HTM_OP_DECONFIGURE. 

Can this silently destroy Process B's active trace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1