Re: [PATCH] perf: arm_spe: Support heterogeneous ACPI systems

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] perf: arm_spe: missing heterogeneity check for optional PMUs
- [Medium] perf/arm_spe: spurious probe failure due to CPU hotplug race
--

commit 9ac6f1b22e8955b2543af029311236c9a14923ec
Author: Johnny.Fan <Johnny.Fan@cixtech.com>

perf: arm_spe: Support heterogeneous ACPI systems

The ACPI MADT describes the SPE overflow interrupt on a per-CPU basis
in the GICC structure, where a CPU that does not implement SPE simply
reports 0. Heterogeneous systems therefore only report the interrupt
on the CPUs that implement SPE, but arm_pmu_acpi.c rejects this as a
non-homogeneous mapping and never creates the SPE platform device.

> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
> index ca6bf8f86cfe9..4ece1070827b9 100644
> --- a/drivers/perf/arm_pmu_acpi.c
> +++ b/drivers/perf/arm_pmu_acpi.c
[ ... ]
> @@ -97,6 +100,20 @@ arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
>  			return gsi ? -ENXIO : 0;
>  
>  		this_gsi = parse_gsi(gicc);
> +
> +		if (optional) {
> +			if (!this_gsi)
> +				continue;
> +
> +			if (gsi && gsi != this_gsi) {
> +				pr_warn("ACPI: %s: CPU%d: inconsistent interrupt %u/%u\n",
> +					pdev->name, cpu, gsi, this_gsi);
> +				return -ENXIO;
> +			}
> +			gsi = this_gsi;
> +			continue;
> +		}
> +
>  		this_hetid = find_acpi_cpu_topology_hetero_id(cpu);
>  		if (!gsi) {
>  			hetid = this_hetid;

[Severity: High]
Does skipping find_acpi_cpu_topology_hetero_id() allow grouping CPUs from
different microarchitectural clusters into a single PMU device?

The SPE PMU driver (arm_spe_pmu_dev_init()) assumes a single PMU device is
homogeneous. It probes SPE parameters (e.g., align, max_record_sz, features)
on a single CPU via smp_call_function_any() and applies them to all CPUs in
spe_pmu->supported_cpus.

If heterogeneous CPUs with different SPE capabilities are grouped together,
scheduling a perf event on a CPU from a different cluster could program the
hardware with mismatched constraints or feature flags. Could the this_hetid
check be preserved for the CPUs that actually implement the optional PMU?

> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index 604ae6ba3b076..c1ff875be9577 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
[ ... ]
> @@ -1277,6 +1291,12 @@ static int arm_spe_pmu_cpu_startup(unsigned int cpu, struct hlist_node *node)
>  	if (!cpumask_test_cpu(cpu, &spe_pmu->supported_cpus))
>  		return 0;
>  
> +	/* The CPU was offline during probe; trim it if it lacks SPE */
> +	if (!arm_spe_pmu_pmsver()) {
> +		cpumask_clear_cpu(cpu, &spe_pmu->supported_cpus);
> +		return 0;
> +	}
> +
>  	__arm_spe_pmu_setup_one(spe_pmu);

[Severity: High]
Is it possible for the newly online CPU to have a different version of SPE
than what was cached during the initial probe?

This startup check only verifies that the CPU has some version of SPE
(checking pmsver != 0), but does not verify that the SPE parameters match
the ones cached from the initial probe.

[ ... ]
> @@ -1346,6 +1366,21 @@ static int arm_spe_pmu_irq_probe(struct arm_spe_pmu *spe_pmu)
>  
>  	cpumask_copy(&spe_pmu->supported_cpus, affinity);
>  
> +	/*
> +	 * Heterogeneous systems may only implement SPE on a subset of the
> +	 * CPUs covered by the interrupt affinity (ACPI in particular has
> +	 * no affinity information and falls back to cpu_possible_mask).
> +	 * SPE is self-describing through PMSVer, so drop the online CPUs
> +	 * that do not implement it. CPUs that are offline at probe time
> +	 * are checked when they come up.
> +	 */
> +	for_each_cpu(cpu, &spe_pmu->supported_cpus) {
> +		if (cpu_online(cpu))
> +			smp_call_function_single(cpu,
> +						 __arm_spe_pmu_trim_supported_cpus,
> +						 spe_pmu, 1);
> +	}
> +
>  	spe_pmu->irq = irq;
>  	return 0;
>  }

[Severity: Medium]
Could this cpu_online(cpu) check without holding cpus_read_lock() lead to a
race condition with CPU hotplug?

If an offline CPU lacking SPE comes online immediately after this loop, it
could remain in supported_cpus because the hotplug callback is not yet
registered.

Later, arm_spe_pmu_dev_init() consumes this mask:

	ret = smp_call_function_any(mask,  __arm_spe_pmu_dev_probe, spe_pmu, 1);
	if (ret || !(spe_pmu->features & SPE_PMU_FEAT_DEV_PROBED))
		return -ENXIO;

This could select the newly online CPU that lacks SPE, causing the driver
probe to spuriously fail.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015062908.3565761-1-johnny.fan@cixtech.com?part=1