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

[sashiko-bot@kernel.org]

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

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