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

[Marc Zyngier <maz@kernel.org>]

On Thu, 15 Oct 2026 07:29:08 +0100,
"Johnny.Fan" <johnny.fan@cixtech.com> wrote:
> 
> From: "Johnny.Fan" <Johnny.Fan@cixtech.com>
> 
> 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.
> 
> Treat the SPE interrupt as optional when walking the MADT: skip the
> GICCs that report 0 and only require that the CPUs which do report it
> agree on the number.
> 
> Note that the CPUs reporting the interrupt must still share a single
> GSI, as the platform device only exposes one IRQ resource. A system
> that assigns a different PPI per cluster cannot be described by this
> single-device model.
> 
> The CPUs without SPE are trimmed from the PMU supported CPUs by
> probing PMSVer on each CPU, both at probe time (for online CPUs) and
> from the hotplug callback (for CPUs that were offline at probe), as
> ACPI conveys no affinity information and the PPI otherwise defaults
> to all possible CPUs.

This is a bit odd: at the beginning, you are stating that the SPE
interrupt being 0 is an indication of SPE not being supported. Then
you check for PMSVer for the same properly. This seems pretty wrong.

It should be possible to build the mask of SPE-enabled CPUs without
IPI-ing the other CPUs, and only rely on the firmware table.

> 
> Tested on a CIX sky1 SoC with heterogeneous SPE support.
> 
> Signed-off-by: Johnny.Fan <Johnny.Fan@cixtech.com>
> 
> Reviewed-by: Fugang Duan <fugang.duan@cixtech.com>
> ---
>  drivers/perf/arm_pmu_acpi.c | 34 +++++++++++++++++++++++-------
>  drivers/perf/arm_spe_pmu.c  | 41 ++++++++++++++++++++++++++++++++++---
>  2 files changed, 65 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
> index ca6bf8f86cfe..4ece1070827b 100644
> --- a/drivers/perf/arm_pmu_acpi.c
> +++ b/drivers/perf/arm_pmu_acpi.c
> @@ -70,7 +70,8 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
>  
>  static int __maybe_unused
>  arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
> -			     u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
> +			     u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *),
> +			     bool optional)
>  {
>  	int cpu, this_hetid, hetid, irq, ret;
>  	u16 this_gsi = 0, gsi = 0;
> @@ -87,7 +88,9 @@ arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
>  
>  	/*
>  	 * Sanity check all the GICC tables for the same interrupt
> -	 * number. For now, only support homogeneous ACPI machines.
> +	 * number. For now, only support homogeneous ACPI machines,
> +	 * unless the interrupt is optional, in which case a CPU that
> +	 * does not implement the feature simply reports 0.
>  	 */
>  	for_each_possible_cpu(cpu) {
>  		struct acpi_madt_generic_interrupt *gicc;
> @@ -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 "optional" parameter feels very wrong. We now have an
infrastructure to get a set of affinities for an interrupt (see
5324fe21ba9b7 and following patches). What this needs is ACPI to be
plumbed into this.

Unfortunately, ACPI is awkward and very badly abstracted in this area,
but that doesn't mean we should make the driver code more bizarre.

Treating SPE and TRBE differently is just as odd, and there is no
reason to do other than suiting your particular platform.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.