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

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

[Johnny.Fan]

From: "Johnny.Fan" <Johnny.Fan@cixtech.com>

On heterogeneous ACPI systems, not all CPUs implement SPE, and the
ones that do may use different PPIs. The MADT describes this per
GICC entry: a CPU without SPE reports a zero SPE overflow interrupt,
and the PPTT hetero id identifies CPUs of the same implementation.

The current code registers a single platform device and gives up if
the GICC entries disagree on either the hetero id or the interrupt,
so SPE is unavailable on such systems even though the firmware
describes it.

Group the CPUs by (hetero id, SPE interrupt) and register one
platform device per group. A GSI may be shared between groups with
non-overlapping CPU masks, so only unregister the GSI mapping when
no other group uses the same interrupt, as the mapping is not
reference counted.

These synthetic devices have no fwnode, so the firmware-described
IRQ affinity cannot tell which CPUs implement the feature. Pass the
CPU mask as platform data and have the drivers prefer it over the
IRQ affinity when present.

Apply the same treatment to TRBE, which shares this registration
code. TRBE must also pass a stable copy of the mask to
request_percpu_irq_affinity(), since supported_cpus is pruned as
CPUs are probed.

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/hwtracing/coresight/coresight-trbe.c |  22 +-
 drivers/perf/arm_pmu_acpi.c                  | 211 ++++++++++++-------
 drivers/perf/arm_spe_pmu.c                   |  10 +-
 include/linux/perf/arm_pmu.h                 |  10 +
 4 files changed, 176 insertions(+), 77 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index c7cbca45f2de..0c4bc8fbcaf3 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -158,6 +158,7 @@ struct trbe_drvdata {
 	struct hlist_node hotplug_node;
 	int irq;
 	cpumask_t supported_cpus;
+	cpumask_t irq_affinity;
 	enum cpuhp_state trbe_online;
 	struct platform_device *pdev;
 };
@@ -1533,6 +1534,8 @@ static void arm_trbe_remove_cpuhp(struct trbe_drvdata *drvdata)
 static int arm_trbe_probe_irq(struct platform_device *pdev,
 			      struct trbe_drvdata *drvdata)
 {
+	const struct arm_pmu_acpi_dev_data *acpi_data =
+		dev_get_platdata(&pdev->dev);
 	const struct cpumask *affinity;
 	int ret;
 
@@ -1547,14 +1550,27 @@ static int arm_trbe_probe_irq(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
-	cpumask_copy(&drvdata->supported_cpus, affinity);
+	/*
+	 * The ACPI path has no fwnode describing which CPUs implement TRBE,
+	 * so it passes the mask explicitly. DT devices still use the
+	 * firmware-described IRQ affinity.
+	 */
+	cpumask_copy(&drvdata->supported_cpus,
+		     acpi_data ? &acpi_data->cpus : affinity);
+
+	/*
+	 * supported_cpus gets pruned as CPUs are probed, so keep a stable
+	 * copy for the IRQ action.
+	 */
+	cpumask_copy(&drvdata->irq_affinity, &drvdata->supported_cpus);
 
 	drvdata->handle = alloc_percpu(struct perf_output_handle *);
 	if (!drvdata->handle)
 		return -ENOMEM;
 
-	ret = request_percpu_irq_affinity(drvdata->irq, arm_trbe_irq_handler, DRVNAME,
-					  affinity, drvdata->handle);
+	ret = request_percpu_irq_affinity(drvdata->irq, arm_trbe_irq_handler,
+					  DRVNAME, &drvdata->irq_affinity,
+					  drvdata->handle);
 	if (ret) {
 		free_percpu(drvdata->handle);
 		return ret;
diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
index ca6bf8f86cfe..fdf39673648f 100644
--- a/drivers/perf/arm_pmu_acpi.c
+++ b/drivers/perf/arm_pmu_acpi.c
@@ -12,6 +12,8 @@
 #include <linux/irqdesc.h>
 #include <linux/percpu.h>
 #include <linux/perf/arm_pmu.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
 
 #include <asm/cpu.h>
 #include <asm/cputype.h>
@@ -68,91 +70,165 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
 		acpi_unregister_gsi(gsi);
 }
 
-static int __maybe_unused
-arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
-			     u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
+struct arm_pmu_acpi_group {
+	int hetid;
+	u16 gsi;
+	struct cpumask cpus;
+};
+
+static int arm_acpi_find_group(struct arm_pmu_acpi_group *groups, int nr_groups,
+			       int hetid, u16 gsi)
 {
-	int cpu, this_hetid, hetid, irq, ret;
-	u16 this_gsi = 0, gsi = 0;
+	int i;
 
-	/*
-	 * Ensure that platform device must have IORESOURCE_IRQ
-	 * resource to hold gsi interrupt.
-	 */
-	if (pdev->num_resources != 1)
-		return -ENXIO;
+	for (i = 0; i < nr_groups; i++) {
+		if (groups[i].hetid == hetid && groups[i].gsi == gsi)
+			return i;
+	}
+
+	return -1;
+}
+
+static bool arm_acpi_gsi_is_shared(struct arm_pmu_acpi_group *groups,
+				   int nr_groups, int idx)
+{
+	int i;
+
+	for (i = 0; i < nr_groups; i++) {
+		if (i != idx && groups[i].gsi == groups[idx].gsi)
+			return true;
+	}
+
+	return false;
+}
+
+static int arm_acpi_register_group(struct arm_pmu_acpi_group *group,
+				   const char *name, bool gsi_shared)
+{
+	struct arm_pmu_acpi_dev_data data = { };
+	struct platform_device *pdev;
+	struct resource res;
+	int irq, ret;
+
+	irq = acpi_register_gsi(NULL, group->gsi, ACPI_LEVEL_SENSITIVE,
+				ACPI_ACTIVE_HIGH);
+	if (irq < 0)
+		return irq;
+
+	pdev = platform_device_alloc(name, PLATFORM_DEVID_AUTO);
+	if (!pdev) {
+		ret = -ENOMEM;
+		goto err_unreg;
+	}
 
-	if (pdev->resource[0].flags != IORESOURCE_IRQ)
-		return -ENXIO;
+	res = DEFINE_RES_IRQ(irq);
+	data.cpus = group->cpus;
 
+	ret = platform_device_add_resources(pdev, &res, 1);
+	if (ret)
+		goto err_put;
+
+	ret = platform_device_add_data(pdev, &data, sizeof(data));
+	if (ret)
+		goto err_put;
+
+	ret = platform_device_add(pdev);
+	if (!ret)
+		return 0;
+
+err_put:
+	platform_device_put(pdev);
+err_unreg:
 	/*
-	 * Sanity check all the GICC tables for the same interrupt
-	 * number. For now, only support homogeneous ACPI machines.
+	 * The GSI mapping is not reference counted, so only drop it if no
+	 * other group shares it.
 	 */
+	if (!gsi_shared)
+		acpi_unregister_gsi(group->gsi);
+
+	return ret;
+}
+
+static int __maybe_unused
+arm_acpi_register_pmu_devices(const char *name, u8 len,
+			      u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
+{
+	struct arm_pmu_acpi_group *groups;
+	int cpu, i, nr_cpus = 0, nr_groups = 0, nr_devices = 0;
+	int last_err = 0, ret;
+
 	for_each_possible_cpu(cpu) {
 		struct acpi_madt_generic_interrupt *gicc;
 
 		gicc = acpi_cpu_get_madt_gicc(cpu);
 		if (gicc->header.length < len)
-			return gsi ? -ENXIO : 0;
-
-		this_gsi = parse_gsi(gicc);
-		this_hetid = find_acpi_cpu_topology_hetero_id(cpu);
-		if (!gsi) {
-			hetid = this_hetid;
-			gsi = this_gsi;
-		} else if (hetid != this_hetid || gsi != this_gsi) {
-			pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
-			return -ENXIO;
-		}
+			continue;
+		if (parse_gsi(gicc))
+			nr_cpus++;
 	}
 
-	if (!this_gsi)
+	if (!nr_cpus)
 		return 0;
 
-	irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
-	if (irq < 0) {
-		pr_warn("ACPI: %s Unable to register interrupt: %d\n", pdev->name, gsi);
-		return -ENXIO;
-	}
+	groups = kcalloc(nr_cpus, sizeof(*groups), GFP_KERNEL);
+	if (!groups)
+		return -ENOMEM;
 
-	pdev->resource[0].start = irq;
-	ret = platform_device_register(pdev);
-	if (ret)
-		acpi_unregister_gsi(gsi);
+	for_each_possible_cpu(cpu) {
+		struct acpi_madt_generic_interrupt *gicc;
+		int hetid;
+		u16 gsi;
 
-	return ret;
-}
+		gicc = acpi_cpu_get_madt_gicc(cpu);
+		if (gicc->header.length < len)
+			continue;
 
-#if IS_ENABLED(CONFIG_ARM_SPE_PMU)
-static struct resource spe_resources[] = {
-	{
-		/* irq */
-		.flags          = IORESOURCE_IRQ,
+		gsi = parse_gsi(gicc);
+		if (!gsi)
+			continue;
+
+		hetid = find_acpi_cpu_topology_hetero_id(cpu);
+		i = arm_acpi_find_group(groups, nr_groups, hetid, gsi);
+		if (i < 0) {
+			i = nr_groups++;
+			groups[i].hetid = hetid;
+			groups[i].gsi = gsi;
+		}
+
+		cpumask_set_cpu(cpu, &groups[i].cpus);
 	}
-};
 
-static struct platform_device spe_dev = {
-	.name = ARMV8_SPE_PDEV_NAME,
-	.id = -1,
-	.resource = spe_resources,
-	.num_resources = ARRAY_SIZE(spe_resources)
-};
+	for (i = 0; i < nr_groups; i++) {
+		ret = arm_acpi_register_group(&groups[i], name,
+					      arm_acpi_gsi_is_shared(groups,
+								     nr_groups, i));
+		if (ret) {
+			pr_warn("ACPI: %s: %*pbl: add device failed (%d)\n",
+				name, cpumask_pr_args(&groups[i].cpus), ret);
+			last_err = ret;
+		} else {
+			nr_devices++;
+		}
+	}
+
+	kfree(groups);
+
+	return nr_devices ? 0 : last_err;
+}
 
+#if IS_ENABLED(CONFIG_ARM_SPE_PMU)
 static u16 arm_spe_parse_gsi(struct acpi_madt_generic_interrupt *gicc)
 {
 	return gicc->spe_interrupt;
 }
 
-/*
- * For lack of a better place, hook the normal PMU MADT walk
- * and create a SPE device if we detect a recent MADT with
- * a homogeneous PPI mapping.
- */
 static void arm_spe_acpi_register_device(void)
 {
-	int ret = arm_acpi_register_pmu_device(&spe_dev, ACPI_MADT_GICC_SPE,
-					       arm_spe_parse_gsi);
+	int ret;
+
+	ret = arm_acpi_register_pmu_devices(ARMV8_SPE_PDEV_NAME,
+					    ACPI_MADT_GICC_SPE,
+					    arm_spe_parse_gsi);
 	if (ret)
 		pr_warn("ACPI: SPE: Unable to register device\n");
 }
@@ -163,20 +239,6 @@ static inline void arm_spe_acpi_register_device(void)
 #endif /* CONFIG_ARM_SPE_PMU */
 
 #if IS_ENABLED(CONFIG_CORESIGHT_TRBE)
-static struct resource trbe_resources[] = {
-	{
-		/* irq */
-		.flags          = IORESOURCE_IRQ,
-	}
-};
-
-static struct platform_device trbe_dev = {
-	.name = ARMV8_TRBE_PDEV_NAME,
-	.id = -1,
-	.resource = trbe_resources,
-	.num_resources = ARRAY_SIZE(trbe_resources)
-};
-
 static u16 arm_trbe_parse_gsi(struct acpi_madt_generic_interrupt *gicc)
 {
 	return gicc->trbe_interrupt;
@@ -184,8 +246,11 @@ static u16 arm_trbe_parse_gsi(struct acpi_madt_generic_interrupt *gicc)
 
 static void arm_trbe_acpi_register_device(void)
 {
-	int ret = arm_acpi_register_pmu_device(&trbe_dev, ACPI_MADT_GICC_TRBE,
-					       arm_trbe_parse_gsi);
+	int ret;
+
+	ret = arm_acpi_register_pmu_devices(ARMV8_TRBE_PDEV_NAME,
+					    ACPI_MADT_GICC_TRBE,
+					    arm_trbe_parse_gsi);
 	if (ret)
 		pr_warn("ACPI: TRBE: Unable to register device\n");
 }
diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index b70f3fedf028..712b7f187301 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -1330,6 +1330,8 @@ static void arm_spe_pmu_dev_teardown(struct arm_spe_pmu *spe_pmu)
 static int arm_spe_pmu_irq_probe(struct arm_spe_pmu *spe_pmu)
 {
 	struct platform_device *pdev = spe_pmu->pdev;
+	const struct arm_pmu_acpi_dev_data *acpi_data =
+		dev_get_platdata(&pdev->dev);
 	const struct cpumask *affinity;
 	int irq;
 
@@ -1342,7 +1344,13 @@ static int arm_spe_pmu_irq_probe(struct arm_spe_pmu *spe_pmu)
 		return -EINVAL;
 	}
 
-	cpumask_copy(&spe_pmu->supported_cpus, affinity);
+	/*
+	 * The ACPI path has no fwnode describing which CPUs implement SPE,
+	 * so it passes the mask explicitly. DT devices still use the
+	 * firmware-described IRQ affinity.
+	 */
+	cpumask_copy(&spe_pmu->supported_cpus,
+		     acpi_data ? &acpi_data->cpus : affinity);
 
 	spe_pmu->irq = irq;
 	return 0;
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index 02d2c7f45b52..7f1c03c55025 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -8,6 +8,7 @@
 #ifndef __ARM_PMU_H__
 #define __ARM_PMU_H__
 
+#include <linux/cpumask.h>
 #include <linux/interrupt.h>
 #include <linux/perf_event.h>
 #include <linux/platform_device.h>
@@ -199,6 +200,15 @@ void armpmu_free_irq(struct arm_pmu * __percpu *armpmu, int irq, int cpu);
 #define ARMV8_SPE_PDEV_NAME "arm,spe-v1"
 #define ARMV8_TRBE_PDEV_NAME "arm,trbe"
 
+/*
+ * Per-device CPU mask passed by the ACPI MADT walk. The synthetic
+ * platform devices have no fwnode, so the IRQ affinity cannot describe
+ * which CPUs actually implement the feature.
+ */
+struct arm_pmu_acpi_dev_data {
+	struct cpumask cpus;
+};
+
 /* Why does everything I do descend into this? */
 #define __GEN_PMU_FORMAT_ATTR(cfg, lo, hi)				\
 	(lo) == (hi) ? #cfg ":" #lo "\n" : #cfg ":" #lo "-" #hi
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf: arm_pmu_acpi: GSI leak in arm_acpi_register_group() when shared
--

commit 07d4d2d60385c80c0a1df910cfddbd2fb128123f
Author: Johnny.Fan <Johnny.Fan@cixtech.com>

perf: arm_spe: Support heterogeneous ACPI systems

This commit updates ACPI registration for ARM SPE and TRBE to group CPUs by
hetero ID and interrupt number, registering one platform device per group
and passing the group's CPU mask via platform data.

> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
> index ca6bf8f86cfe9..fdf39673648f3 100644
> --- a/drivers/perf/arm_pmu_acpi.c
> +++ b/drivers/perf/arm_pmu_acpi.c
[ ... ]
> +static bool arm_acpi_gsi_is_shared(struct arm_pmu_acpi_group *groups,
> +				   int nr_groups, int idx)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_groups; i++) {
> +		if (i != idx && groups[i].gsi == groups[idx].gsi)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int arm_acpi_register_group(struct arm_pmu_acpi_group *group,
> +				   const char *name, bool gsi_shared)
> +{
> +	struct arm_pmu_acpi_dev_data data = { };
> +	struct platform_device *pdev;
> +	struct resource res;
> +	int irq, ret;
> +
> +	irq = acpi_register_gsi(NULL, group->gsi, ACPI_LEVEL_SENSITIVE,
> +				ACPI_ACTIVE_HIGH);
> +	if (irq < 0)
> +		return irq;
> +
> +	pdev = platform_device_alloc(name, PLATFORM_DEVID_AUTO);
> +	if (!pdev) {
> +		ret = -ENOMEM;
> +		goto err_unreg;
> +	}
>  
> -	if (pdev->resource[0].flags != IORESOURCE_IRQ)
> -		return -ENXIO;
> +	res = DEFINE_RES_IRQ(irq);
> +	data.cpus = group->cpus;
>  
> +	ret = platform_device_add_resources(pdev, &res, 1);
> +	if (ret)
> +		goto err_put;
> +
> +	ret = platform_device_add_data(pdev, &data, sizeof(data));
> +	if (ret)
> +		goto err_put;
> +
> +	ret = platform_device_add(pdev);
> +	if (!ret)
> +		return 0;
> +
> +err_put:
> +	platform_device_put(pdev);
> +err_unreg:
>  	/*
> -	 * Sanity check all the GICC tables for the same interrupt
> -	 * number. For now, only support homogeneous ACPI machines.
> +	 * The GSI mapping is not reference counted, so only drop it if no
> +	 * other group shares it.
>  	 */
> +	if (!gsi_shared)
> +		acpi_unregister_gsi(group->gsi);
> +
> +	return ret;
> +}
> +
> +static int __maybe_unused
> +arm_acpi_register_pmu_devices(const char *name, u8 len,
> +			      u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
> +{
[ ... ]
> +	for (i = 0; i < nr_groups; i++) {
> +		ret = arm_acpi_register_group(&groups[i], name,
> +					      arm_acpi_gsi_is_shared(groups,
> +								     nr_groups, i));
> +		if (ret) {
> +			pr_warn("ACPI: %s: %*pbl: add device failed (%d)\n",
> +				name, cpumask_pr_args(&groups[i].cpus), ret);
> +			last_err = ret;
> +		} else {
> +			nr_devices++;
> +		}
> +	}

[Severity: Medium]
Can this leak the GSI mapping if all groups sharing an interrupt fail to
register their platform devices?

In drivers/perf/arm_pmu_acpi.c, arm_acpi_register_pmu_devices() calls
arm_acpi_gsi_is_shared(), which checks whether any other entry in groups[]
has the same gsi, regardless of whether any of those groups actually
succeeded in registering a platform device.

When two or more groups share the same gsi, arm_acpi_gsi_is_shared() returns
true for every group in that set. If the first group creates the mapping via
acpi_register_gsi() in arm_acpi_register_group() and then fails in
platform_device_alloc(), platform_device_add_resources(),
platform_device_add_data(), or platform_device_add(), the err_unreg path sees
gsi_shared as true and skips acpi_unregister_gsi(group->gsi). If the
remaining groups sharing that gsi also fail during registration, none of them
will call acpi_unregister_gsi(group->gsi) either, leaving the GSI mapping
registered with no platform device using it.

Would it make sense to track whether another group sharing the gsi has
already registered successfully, or clean up the mapping if all groups
sharing that gsi fail?

[ ... ]

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