[PATCH v7 0/9] Enable PMUs in ACPI systems

[PATCH v7 0/9] Enable PMUs in ACPI systems

[Jeremy Linton]

v7:
Rebase to 4.8rc3

Remove cpu affinity sysfs entry. While providing a CPU mask for
ARMv8 PMU's is really helpful in big/little environments, reworking the PMU
code to support the cpumask attribute for !arm64 PMUs is out of the scope
of this patch set.

Fix CPU miscount problem where an alloc failure followed by successfully
allocating the structure can result in under counting the CPUs associated
with the PMU. This bug was created in v6 with the conversion to a linked list.

Remove initial platform device creation code by Mark Salter, and re-squash
multiple platform device creation code together with helper routines.

Other minor tweakage.

v6:
Added cpu affinity sysfs entry
Converted pmu_types array, to linked list
Restrict use of the armv8_pmu_probe_table to ACPI systems
Rename MADT parsing routines in smp.c
Convert sysfs PMU name to use index rather than partnum
Remove pr_devel statements
Other Minor cleanups
Add Partial Ack-by Will Deacon

v5:
Remove list of CPU types for ACPI systems. We now match a generic
event list, and use the PMCIED[01] to select events which exist on
the given PMU. This avoids the need to update the kernel every time
a new CPU is released.
Update the maintainers list to include the new file.

v4:
Correct build issues with ARM (!ARM64) kernels.
Add ThunderX to list of PMU types.

v3:
Enable ARM performance monitoring units on ACPI/arm64 machines.

This patch expands and reworks the patches published by Mark Salter
in order to clean up a few of the previous review comments, as well as
add support for newer CPUs and big/little configurations.

Jeremy Linton (7):
  arm64: pmu: Probe default hw/cache counters
  arm64: pmu: Hoist pmu platform device name
  arm64: Rename the common MADT parse routine
  arm: arm64: Add routine to determine cpuid of other cpus
  arm: arm64: pmu: Assign platform PMU CPU affinity
  arm64: pmu: Detect and enable multiple PMUs in an ACPI system
  MAINTAINERS: Tweak ARM PMU maintainers

Mark Salter (2):
  arm64: pmu: add fallback probe table
  arm64: pmu: Add support for probing with ACPI

 MAINTAINERS                      |   3 +-
 arch/arm/include/asm/cputype.h   |   2 +
 arch/arm64/include/asm/cputype.h |   3 +
 arch/arm64/kernel/perf_event.c   |  58 ++++++++++-
 arch/arm64/kernel/smp.c          |  18 ++--
 drivers/perf/Kconfig             |   4 +
 drivers/perf/Makefile            |   1 +
 drivers/perf/arm_pmu.c           |  60 +++++++++--
 drivers/perf/arm_pmu_acpi.c      | 215 +++++++++++++++++++++++++++++++++++++++
 include/linux/perf/arm_pmu.h     |  12 +++
 10 files changed, 353 insertions(+), 23 deletions(-)
 create mode 100644 drivers/perf/arm_pmu_acpi.c

-- 
2.5.5

[PATCH v7 0/9] Enable PMUs in ACPI systems

[Jeremy Linton]

v7:
Rebase to 4.8rc3

Remove cpu affinity sysfs entry. While providing a CPU mask for
ARMv8 PMU's is really helpful in big/little environments, reworking the PMU
code to support the cpumask attribute for !arm64 PMUs is out of the scope
of this patch set.

Fix CPU miscount problem where an alloc failure followed by successfully
allocating the structure can result in under counting the CPUs associated
with the PMU. This bug was created in v6 with the conversion to a linked list.

Remove initial platform device creation code by Mark Salter, and re-squash
multiple platform device creation code together with helper routines.

Other minor tweakage.

v6:
Added cpu affinity sysfs entry
Converted pmu_types array, to linked list
Restrict use of the armv8_pmu_probe_table to ACPI systems
Rename MADT parsing routines in smp.c
Convert sysfs PMU name to use index rather than partnum
Remove pr_devel statements
Other Minor cleanups
Add Partial Ack-by Will Deacon

v5:
Remove list of CPU types for ACPI systems. We now match a generic
event list, and use the PMCIED[01] to select events which exist on
the given PMU. This avoids the need to update the kernel every time
a new CPU is released.
Update the maintainers list to include the new file.

v4:
Correct build issues with ARM (!ARM64) kernels.
Add ThunderX to list of PMU types.

v3:
Enable ARM performance monitoring units on ACPI/arm64 machines.

This patch expands and reworks the patches published by Mark Salter
in order to clean up a few of the previous review comments, as well as
add support for newer CPUs and big/little configurations.

Jeremy Linton (7):
  arm64: pmu: Probe default hw/cache counters
  arm64: pmu: Hoist pmu platform device name
  arm64: Rename the common MADT parse routine
  arm: arm64: Add routine to determine cpuid of other cpus
  arm: arm64: pmu: Assign platform PMU CPU affinity
  arm64: pmu: Detect and enable multiple PMUs in an ACPI system
  MAINTAINERS: Tweak ARM PMU maintainers

Mark Salter (2):
  arm64: pmu: add fallback probe table
  arm64: pmu: Add support for probing with ACPI

 MAINTAINERS                      |   3 +-
 arch/arm/include/asm/cputype.h   |   2 +
 arch/arm64/include/asm/cputype.h |   3 +
 arch/arm64/kernel/perf_event.c   |  58 ++++++++++-
 arch/arm64/kernel/smp.c          |  18 ++--
 drivers/perf/Kconfig             |   4 +
 drivers/perf/Makefile            |   1 +
 drivers/perf/arm_pmu.c           |  60 +++++++++--
 drivers/perf/arm_pmu_acpi.c      | 215 +++++++++++++++++++++++++++++++++++++++
 include/linux/perf/arm_pmu.h     |  12 +++
 10 files changed, 353 insertions(+), 23 deletions(-)
 create mode 100644 drivers/perf/arm_pmu_acpi.c

-- 
2.5.5

[PATCH v7 1/9] arm64: pmu: add fallback probe table

[Jeremy Linton]

From: Mark Salter <msalter@redhat.com>

In preparation for ACPI support, add a pmu_probe_info table to
the arm_pmu_device_probe() call. This table gets used when
probing in the absence of a devicetree node for PMU.

Signed-off-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/perf_event.c | 13 ++++++++++++-
 drivers/perf/arm_pmu.c         |  2 +-
 include/linux/perf/arm_pmu.h   |  3 +++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 838ccf1..3aac598 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -24,6 +24,7 @@
 #include <asm/sysreg.h>
 #include <asm/virt.h>
 
+#include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/perf/arm_pmu.h>
 #include <linux/platform_device.h>
@@ -1044,9 +1045,19 @@ static const struct of_device_id armv8_pmu_of_device_ids[] = {
 	{},
 };
 
+static const struct pmu_probe_info armv8_pmu_probe_table[] = {
+	PMU_PROBE(0, 0, armv8_pmuv3_init), /* if all else fails... */
+	{ /* sentinel value */ }
+};
+
 static int armv8_pmu_device_probe(struct platform_device *pdev)
 {
-	return arm_pmu_device_probe(pdev, armv8_pmu_of_device_ids, NULL);
+	if (acpi_disabled)
+		return arm_pmu_device_probe(pdev, armv8_pmu_of_device_ids,
+					    NULL);
+
+	return arm_pmu_device_probe(pdev, armv8_pmu_of_device_ids,
+				    armv8_pmu_probe_table);
 }
 
 static struct platform_driver armv8_pmu_driver = {
diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index c494613..f1aee26 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -1028,7 +1028,7 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 		ret = of_pmu_irq_cfg(pmu);
 		if (!ret)
 			ret = init_fn(pmu);
-	} else {
+	} else if (probe_table) {
 		cpumask_setall(&pmu->supported_cpus);
 		ret = probe_current_pmu(pmu, probe_table);
 	}
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index e188438..65d8e27 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -147,6 +147,9 @@ struct pmu_probe_info {
 #define XSCALE_PMU_PROBE(_version, _fn) \
 	PMU_PROBE(ARM_CPU_IMP_INTEL << 24 | _version, ARM_PMU_XSCALE_MASK, _fn)
 
+#define ARMV8_PMU_PART_PROBE(_part, _fn) \
+	PMU_PROBE((_part) << MIDR_PARTNUM_SHIFT, MIDR_PARTNUM_MASK, _fn)
+
 int arm_pmu_device_probe(struct platform_device *pdev,
 			 const struct of_device_id *of_table,
 			 const struct pmu_probe_info *probe_table);
-- 
2.5.5

[PATCH v7 1/9] arm64: pmu: add fallback probe table

[Jeremy Linton]

From: Mark Salter <msalter@redhat.com>

In preparation for ACPI support, add a pmu_probe_info table to
the arm_pmu_device_probe() call. This table gets used when
probing in the absence of a devicetree node for PMU.

Signed-off-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/perf_event.c | 13 ++++++++++++-
 drivers/perf/arm_pmu.c         |  2 +-
 include/linux/perf/arm_pmu.h   |  3 +++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 838ccf1..3aac598 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -24,6 +24,7 @@
 #include <asm/sysreg.h>
 #include <asm/virt.h>
 
+#include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/perf/arm_pmu.h>
 #include <linux/platform_device.h>
@@ -1044,9 +1045,19 @@ static const struct of_device_id armv8_pmu_of_device_ids[] = {
 	{},
 };
 
+static const struct pmu_probe_info armv8_pmu_probe_table[] = {
+	PMU_PROBE(0, 0, armv8_pmuv3_init), /* if all else fails... */
+	{ /* sentinel value */ }
+};
+
 static int armv8_pmu_device_probe(struct platform_device *pdev)
 {
-	return arm_pmu_device_probe(pdev, armv8_pmu_of_device_ids, NULL);
+	if (acpi_disabled)
+		return arm_pmu_device_probe(pdev, armv8_pmu_of_device_ids,
+					    NULL);
+
+	return arm_pmu_device_probe(pdev, armv8_pmu_of_device_ids,
+				    armv8_pmu_probe_table);
 }
 
 static struct platform_driver armv8_pmu_driver = {
diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index c494613..f1aee26 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -1028,7 +1028,7 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 		ret = of_pmu_irq_cfg(pmu);
 		if (!ret)
 			ret = init_fn(pmu);
-	} else {
+	} else if (probe_table) {
 		cpumask_setall(&pmu->supported_cpus);
 		ret = probe_current_pmu(pmu, probe_table);
 	}
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index e188438..65d8e27 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -147,6 +147,9 @@ struct pmu_probe_info {
 #define XSCALE_PMU_PROBE(_version, _fn) \
 	PMU_PROBE(ARM_CPU_IMP_INTEL << 24 | _version, ARM_PMU_XSCALE_MASK, _fn)
 
+#define ARMV8_PMU_PART_PROBE(_part, _fn) \
+	PMU_PROBE((_part) << MIDR_PARTNUM_SHIFT, MIDR_PARTNUM_MASK, _fn)
+
 int arm_pmu_device_probe(struct platform_device *pdev,
 			 const struct of_device_id *of_table,
 			 const struct pmu_probe_info *probe_table);
-- 
2.5.5

[PATCH v7 2/9] arm64: pmu: Probe default hw/cache counters

[Jeremy Linton]

ARMv8 machines can identify the micro/arch defined counters
that are available on a machine. Add all these counters to the
default armv8 perf map. At run-time disable the counters which
are not available on the given PMU.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/perf_event.c | 45 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 3aac598..f650548 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -191,13 +191,23 @@
 #define ARMV8_THUNDER_PERFCTR_L1I_CACHE_PREF_MISS		0xED
 
 /* PMUv3 HW events mapping. */
+
+/*
+ * ARMv8 Architectural defined events, not all of these may
+ * be supported on any given implementation. Undefined events will
+ * be disabled at run-time.
+ */
 static const unsigned armv8_pmuv3_perf_map[PERF_COUNT_HW_MAX] = {
 	PERF_MAP_ALL_UNSUPPORTED,
 	[PERF_COUNT_HW_CPU_CYCLES]		= ARMV8_PMUV3_PERFCTR_CPU_CYCLES,
 	[PERF_COUNT_HW_INSTRUCTIONS]		= ARMV8_PMUV3_PERFCTR_INST_RETIRED,
 	[PERF_COUNT_HW_CACHE_REFERENCES]	= ARMV8_PMUV3_PERFCTR_L1D_CACHE,
 	[PERF_COUNT_HW_CACHE_MISSES]		= ARMV8_PMUV3_PERFCTR_L1D_CACHE_REFILL,
+	[PERF_COUNT_HW_BRANCH_INSTRUCTIONS]	= ARMV8_PMUV3_PERFCTR_PC_WRITE_RETIRED,
 	[PERF_COUNT_HW_BRANCH_MISSES]		= ARMV8_PMUV3_PERFCTR_BR_MIS_PRED,
+	[PERF_COUNT_HW_BUS_CYCLES]		= ARMV8_PMUV3_PERFCTR_BUS_CYCLES,
+	[PERF_COUNT_HW_STALLED_CYCLES_FRONTEND]	= ARMV8_PMUV3_PERFCTR_STALL_FRONTEND,
+	[PERF_COUNT_HW_STALLED_CYCLES_BACKEND]	= ARMV8_PMUV3_PERFCTR_STALL_BACKEND,
 };
 
 /* ARM Cortex-A53 HW events mapping. */
@@ -259,6 +269,15 @@ static const unsigned armv8_pmuv3_perf_cache_map[PERF_COUNT_HW_CACHE_MAX]
 	[C(L1D)][C(OP_WRITE)][C(RESULT_ACCESS)]	= ARMV8_PMUV3_PERFCTR_L1D_CACHE,
 	[C(L1D)][C(OP_WRITE)][C(RESULT_MISS)]	= ARMV8_PMUV3_PERFCTR_L1D_CACHE_REFILL,
 
+	[C(L1I)][C(OP_READ)][C(RESULT_ACCESS)]	= ARMV8_PMUV3_PERFCTR_L1I_CACHE,
+	[C(L1I)][C(OP_READ)][C(RESULT_MISS)]	= ARMV8_PMUV3_PERFCTR_L1I_CACHE_REFILL,
+
+	[C(DTLB)][C(OP_READ)][C(RESULT_MISS)]	= ARMV8_PMUV3_PERFCTR_L1D_TLB_REFILL,
+	[C(DTLB)][C(OP_READ)][C(RESULT_ACCESS)]	= ARMV8_PMUV3_PERFCTR_L1D_TLB,
+
+	[C(ITLB)][C(OP_READ)][C(RESULT_MISS)]	= ARMV8_PMUV3_PERFCTR_L1I_TLB_REFILL,
+	[C(ITLB)][C(OP_READ)][C(RESULT_ACCESS)]	= ARMV8_PMUV3_PERFCTR_L1I_TLB,
+
 	[C(BPU)][C(OP_READ)][C(RESULT_ACCESS)]	= ARMV8_PMUV3_PERFCTR_BR_PRED,
 	[C(BPU)][C(OP_READ)][C(RESULT_MISS)]	= ARMV8_PMUV3_PERFCTR_BR_MIS_PRED,
 	[C(BPU)][C(OP_WRITE)][C(RESULT_ACCESS)]	= ARMV8_PMUV3_PERFCTR_BR_PRED,
@@ -906,9 +925,22 @@ static void armv8pmu_reset(void *info)
 
 static int armv8_pmuv3_map_event(struct perf_event *event)
 {
-	return armpmu_map_event(event, &armv8_pmuv3_perf_map,
-				&armv8_pmuv3_perf_cache_map,
-				ARMV8_PMU_EVTYPE_EVENT);
+	int hw_event_id;
+	struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+
+	hw_event_id = armpmu_map_event(event, &armv8_pmuv3_perf_map,
+				       &armv8_pmuv3_perf_cache_map,
+				       ARMV8_PMU_EVTYPE_EVENT);
+	if (hw_event_id < 0)
+		return hw_event_id;
+
+	/* disable micro/arch events not supported by this PMU */
+	if ((hw_event_id < ARMV8_PMUV3_MAX_COMMON_EVENTS) &&
+		!test_bit(hw_event_id, armpmu->pmceid_bitmap)) {
+			return -EOPNOTSUPP;
+	}
+
+	return hw_event_id;
 }
 
 static int armv8_a53_map_event(struct perf_event *event)
@@ -1045,8 +1077,13 @@ static const struct of_device_id armv8_pmu_of_device_ids[] = {
 	{},
 };
 
+/*
+ * Non DT systems have their micro/arch events probed at run-time.
+ * A fairly complete list of generic events are provided and ones that
+ * aren't supported by the current PMU are disabled.
+ */
 static const struct pmu_probe_info armv8_pmu_probe_table[] = {
-	PMU_PROBE(0, 0, armv8_pmuv3_init), /* if all else fails... */
+	PMU_PROBE(0, 0, armv8_pmuv3_init), /* enable all defined counters */
 	{ /* sentinel value */ }
 };
 
-- 
2.5.5

[PATCH v7 2/9] arm64: pmu: Probe default hw/cache counters

[Jeremy Linton]

ARMv8 machines can identify the micro/arch defined counters
that are available on a machine. Add all these counters to the
default armv8 perf map. At run-time disable the counters which
are not available on the given PMU.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/perf_event.c | 45 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 3aac598..f650548 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -191,13 +191,23 @@
 #define ARMV8_THUNDER_PERFCTR_L1I_CACHE_PREF_MISS		0xED
 
 /* PMUv3 HW events mapping. */
+
+/*
+ * ARMv8 Architectural defined events, not all of these may
+ * be supported on any given implementation. Undefined events will
+ * be disabled at run-time.
+ */
 static const unsigned armv8_pmuv3_perf_map[PERF_COUNT_HW_MAX] = {
 	PERF_MAP_ALL_UNSUPPORTED,
 	[PERF_COUNT_HW_CPU_CYCLES]		= ARMV8_PMUV3_PERFCTR_CPU_CYCLES,
 	[PERF_COUNT_HW_INSTRUCTIONS]		= ARMV8_PMUV3_PERFCTR_INST_RETIRED,
 	[PERF_COUNT_HW_CACHE_REFERENCES]	= ARMV8_PMUV3_PERFCTR_L1D_CACHE,
 	[PERF_COUNT_HW_CACHE_MISSES]		= ARMV8_PMUV3_PERFCTR_L1D_CACHE_REFILL,
+	[PERF_COUNT_HW_BRANCH_INSTRUCTIONS]	= ARMV8_PMUV3_PERFCTR_PC_WRITE_RETIRED,
 	[PERF_COUNT_HW_BRANCH_MISSES]		= ARMV8_PMUV3_PERFCTR_BR_MIS_PRED,
+	[PERF_COUNT_HW_BUS_CYCLES]		= ARMV8_PMUV3_PERFCTR_BUS_CYCLES,
+	[PERF_COUNT_HW_STALLED_CYCLES_FRONTEND]	= ARMV8_PMUV3_PERFCTR_STALL_FRONTEND,
+	[PERF_COUNT_HW_STALLED_CYCLES_BACKEND]	= ARMV8_PMUV3_PERFCTR_STALL_BACKEND,
 };
 
 /* ARM Cortex-A53 HW events mapping. */
@@ -259,6 +269,15 @@ static const unsigned armv8_pmuv3_perf_cache_map[PERF_COUNT_HW_CACHE_MAX]
 	[C(L1D)][C(OP_WRITE)][C(RESULT_ACCESS)]	= ARMV8_PMUV3_PERFCTR_L1D_CACHE,
 	[C(L1D)][C(OP_WRITE)][C(RESULT_MISS)]	= ARMV8_PMUV3_PERFCTR_L1D_CACHE_REFILL,
 
+	[C(L1I)][C(OP_READ)][C(RESULT_ACCESS)]	= ARMV8_PMUV3_PERFCTR_L1I_CACHE,
+	[C(L1I)][C(OP_READ)][C(RESULT_MISS)]	= ARMV8_PMUV3_PERFCTR_L1I_CACHE_REFILL,
+
+	[C(DTLB)][C(OP_READ)][C(RESULT_MISS)]	= ARMV8_PMUV3_PERFCTR_L1D_TLB_REFILL,
+	[C(DTLB)][C(OP_READ)][C(RESULT_ACCESS)]	= ARMV8_PMUV3_PERFCTR_L1D_TLB,
+
+	[C(ITLB)][C(OP_READ)][C(RESULT_MISS)]	= ARMV8_PMUV3_PERFCTR_L1I_TLB_REFILL,
+	[C(ITLB)][C(OP_READ)][C(RESULT_ACCESS)]	= ARMV8_PMUV3_PERFCTR_L1I_TLB,
+
 	[C(BPU)][C(OP_READ)][C(RESULT_ACCESS)]	= ARMV8_PMUV3_PERFCTR_BR_PRED,
 	[C(BPU)][C(OP_READ)][C(RESULT_MISS)]	= ARMV8_PMUV3_PERFCTR_BR_MIS_PRED,
 	[C(BPU)][C(OP_WRITE)][C(RESULT_ACCESS)]	= ARMV8_PMUV3_PERFCTR_BR_PRED,
@@ -906,9 +925,22 @@ static void armv8pmu_reset(void *info)
 
 static int armv8_pmuv3_map_event(struct perf_event *event)
 {
-	return armpmu_map_event(event, &armv8_pmuv3_perf_map,
-				&armv8_pmuv3_perf_cache_map,
-				ARMV8_PMU_EVTYPE_EVENT);
+	int hw_event_id;
+	struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+
+	hw_event_id = armpmu_map_event(event, &armv8_pmuv3_perf_map,
+				       &armv8_pmuv3_perf_cache_map,
+				       ARMV8_PMU_EVTYPE_EVENT);
+	if (hw_event_id < 0)
+		return hw_event_id;
+
+	/* disable micro/arch events not supported by this PMU */
+	if ((hw_event_id < ARMV8_PMUV3_MAX_COMMON_EVENTS) &&
+		!test_bit(hw_event_id, armpmu->pmceid_bitmap)) {
+			return -EOPNOTSUPP;
+	}
+
+	return hw_event_id;
 }
 
 static int armv8_a53_map_event(struct perf_event *event)
@@ -1045,8 +1077,13 @@ static const struct of_device_id armv8_pmu_of_device_ids[] = {
 	{},
 };
 
+/*
+ * Non DT systems have their micro/arch events probed at run-time.
+ * A fairly complete list of generic events are provided and ones that
+ * aren't supported by the current PMU are disabled.
+ */
 static const struct pmu_probe_info armv8_pmu_probe_table[] = {
-	PMU_PROBE(0, 0, armv8_pmuv3_init), /* if all else fails... */
+	PMU_PROBE(0, 0, armv8_pmuv3_init), /* enable all defined counters */
 	{ /* sentinel value */ }
 };
 
-- 
2.5.5

[PATCH v7 3/9] arm64: pmu: Hoist pmu platform device name

[Jeremy Linton]

Move the PMU name into a common header file so it may
be referenced by other users.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/perf_event.c | 2 +-
 include/linux/perf/arm_pmu.h   | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index f650548..356fa6c 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -1099,7 +1099,7 @@ static int armv8_pmu_device_probe(struct platform_device *pdev)
 
 static struct platform_driver armv8_pmu_driver = {
 	.driver		= {
-		.name	= "armv8-pmu",
+		.name	= ARMV8_PMU_PDEV_NAME,
 		.of_match_table = armv8_pmu_of_device_ids,
 	},
 	.probe		= armv8_pmu_device_probe,
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index 65d8e27..ef4b760 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -154,6 +154,8 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 			 const struct of_device_id *of_table,
 			 const struct pmu_probe_info *probe_table);
 
+#define ARMV8_PMU_PDEV_NAME "armv8-pmu"
+
 #endif /* CONFIG_ARM_PMU */
 
 #endif /* __ARM_PMU_H__ */
-- 
2.5.5

[PATCH v7 3/9] arm64: pmu: Hoist pmu platform device name

[Jeremy Linton]

Move the PMU name into a common header file so it may
be referenced by other users.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/perf_event.c | 2 +-
 include/linux/perf/arm_pmu.h   | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index f650548..356fa6c 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -1099,7 +1099,7 @@ static int armv8_pmu_device_probe(struct platform_device *pdev)
 
 static struct platform_driver armv8_pmu_driver = {
 	.driver		= {
-		.name	= "armv8-pmu",
+		.name	= ARMV8_PMU_PDEV_NAME,
 		.of_match_table = armv8_pmu_of_device_ids,
 	},
 	.probe		= armv8_pmu_device_probe,
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index 65d8e27..ef4b760 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -154,6 +154,8 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 			 const struct of_device_id *of_table,
 			 const struct pmu_probe_info *probe_table);
 
+#define ARMV8_PMU_PDEV_NAME "armv8-pmu"
+
 #endif /* CONFIG_ARM_PMU */
 
 #endif /* __ARM_PMU_H__ */
-- 
2.5.5

[PATCH v7 4/9] arm64: Rename the common MADT parse routine

[Jeremy Linton]

The MADT parser in smp.c is now being used to parse
out NUMA, PMU and ACPI parking protocol information as
well as the GIC information for which it was originally
created. Rename it to avoid a misleading name.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/smp.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index d93d433..a6552fe 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -506,13 +506,14 @@ static unsigned int cpu_count = 1;
 
 #ifdef CONFIG_ACPI
 /*
- * acpi_map_gic_cpu_interface - parse processor MADT entry
+ * acpi_verify_and_map_madt - parse processor MADT entry
  *
  * Carry out sanity checks on MADT processor entry and initialize
- * cpu_logical_map on success
+ * cpu_logical_map, the ACPI parking protocol, NUMA mapping
+ * and the PMU interrupts on success
  */
 static void __init
-acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
+acpi_verify_and_map_madt(struct acpi_madt_generic_interrupt *processor)
 {
 	u64 hwid = processor->arm_mpidr;
 
@@ -565,7 +566,7 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
 }
 
 static int __init
-acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
+acpi_parse_madt_common(struct acpi_subtable_header *header,
 			     const unsigned long end)
 {
 	struct acpi_madt_generic_interrupt *processor;
@@ -576,7 +577,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
 
 	acpi_table_print_madt_entry(header);
 
-	acpi_map_gic_cpu_interface(processor);
+	acpi_verify_and_map_madt(processor);
 
 	return 0;
 }
@@ -659,7 +660,7 @@ void __init smp_init_cpus(void)
 		 * we need for SMP init
 		 */
 		acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
-				      acpi_parse_gic_cpu_interface, 0);
+				      acpi_parse_madt_common, 0);
 
 	if (cpu_count > nr_cpu_ids)
 		pr_warn("Number of cores (%d) exceeds configured maximum of %d - clipping\n",
-- 
2.5.5

[PATCH v7 4/9] arm64: Rename the common MADT parse routine

[Jeremy Linton]

The MADT parser in smp.c is now being used to parse
out NUMA, PMU and ACPI parking protocol information as
well as the GIC information for which it was originally
created. Rename it to avoid a misleading name.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/smp.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index d93d433..a6552fe 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -506,13 +506,14 @@ static unsigned int cpu_count = 1;
 
 #ifdef CONFIG_ACPI
 /*
- * acpi_map_gic_cpu_interface - parse processor MADT entry
+ * acpi_verify_and_map_madt - parse processor MADT entry
  *
  * Carry out sanity checks on MADT processor entry and initialize
- * cpu_logical_map on success
+ * cpu_logical_map, the ACPI parking protocol, NUMA mapping
+ * and the PMU interrupts on success
  */
 static void __init
-acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
+acpi_verify_and_map_madt(struct acpi_madt_generic_interrupt *processor)
 {
 	u64 hwid = processor->arm_mpidr;
 
@@ -565,7 +566,7 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
 }
 
 static int __init
-acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
+acpi_parse_madt_common(struct acpi_subtable_header *header,
 			     const unsigned long end)
 {
 	struct acpi_madt_generic_interrupt *processor;
@@ -576,7 +577,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
 
 	acpi_table_print_madt_entry(header);
 
-	acpi_map_gic_cpu_interface(processor);
+	acpi_verify_and_map_madt(processor);
 
 	return 0;
 }
@@ -659,7 +660,7 @@ void __init smp_init_cpus(void)
 		 * we need for SMP init
 		 */
 		acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
-				      acpi_parse_gic_cpu_interface, 0);
+				      acpi_parse_madt_common, 0);
 
 	if (cpu_count > nr_cpu_ids)
 		pr_warn("Number of cores (%d) exceeds configured maximum of %d - clipping\n",
-- 
2.5.5

[PATCH v7 5/9] arm64: pmu: Add support for probing with ACPI

[Jeremy Linton]

From: Mark Salter <msalter@redhat.com>

In the case of ACPI, the PMU IRQ information is contained in the
MADT table. Also, since the PMU does not exist as a device in the
ACPI DSDT table, it is necessary to create a platform device so
that the appropriate driver probing is triggered.

Signed-off-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/smp.c      |  5 +++++
 drivers/perf/Kconfig         |  4 ++++
 drivers/perf/Makefile        |  1 +
 drivers/perf/arm_pmu_acpi.c  | 51 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/perf/arm_pmu.h |  7 ++++++
 5 files changed, 68 insertions(+)
 create mode 100644 drivers/perf/arm_pmu_acpi.c

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index a6552fe..dc333c6 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -37,6 +37,7 @@
 #include <linux/completion.h>
 #include <linux/of.h>
 #include <linux/irq_work.h>
+#include <linux/perf/arm_pmu.h>
 
 #include <asm/alternative.h>
 #include <asm/atomic.h>
@@ -540,6 +541,7 @@ acpi_verify_and_map_madt(struct acpi_madt_generic_interrupt *processor)
 			return;
 		}
 		bootcpu_valid = true;
+		arm_pmu_parse_acpi(0, processor);
 		return;
 	}
 
@@ -560,6 +562,9 @@ acpi_verify_and_map_madt(struct acpi_madt_generic_interrupt *processor)
 	 */
 	acpi_set_mailbox_entry(cpu_count, processor);
 
+	/* get PMU irq info */
+	arm_pmu_parse_acpi(cpu_count, processor);
+
 	early_map_cpu_to_node(cpu_count, acpi_numa_get_nid(cpu_count, hwid));
 
 	cpu_count++;
diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index 04e2653..818fa3b 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -12,4 +12,8 @@ config ARM_PMU
 	  Say y if you want to use CPU performance monitors on ARM-based
 	  systems.
 
+config ARM_PMU_ACPI
+	def_bool y
+	depends on ARM_PMU && ACPI
+
 endmenu
diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
index acd2397..fd8090d 100644
--- a/drivers/perf/Makefile
+++ b/drivers/perf/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_ARM_PMU) += arm_pmu.o
+obj-$(CONFIG_ARM_PMU_ACPI) += arm_pmu_acpi.o
diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
new file mode 100644
index 0000000..e784714
--- /dev/null
+++ b/drivers/perf/arm_pmu_acpi.c
@@ -0,0 +1,51 @@
+/*
+ * ARM ACPI PMU support
+ *
+ * Copyright (C) 2015 Red Hat Inc.
+ * Author: Mark Salter <msalter@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include <asm/cpu.h>
+#include <linux/acpi.h>
+#include <linux/irq.h>
+#include <linux/irqdesc.h>
+#include <linux/list.h>
+#include <linux/perf/arm_pmu.h>
+#include <linux/platform_device.h>
+
+struct pmu_irq {
+	int  gsi;
+	int  trigger;
+	bool registered;
+};
+
+static struct pmu_irq pmu_irqs[NR_CPUS] __initdata;
+
+/*
+ * Called from acpi_map_gic_cpu_interface()'s MADT parsing during boot.
+ * This routine saves off the GSI's and their trigger state for use when we are
+ * ready to build the PMU platform device.
+*/
+void __init arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic)
+{
+	pmu_irqs[cpu].gsi = gic->performance_interrupt;
+	if (gic->flags & ACPI_MADT_PERFORMANCE_IRQ_MODE)
+		pmu_irqs[cpu].trigger = ACPI_EDGE_SENSITIVE;
+	else
+		pmu_irqs[cpu].trigger = ACPI_LEVEL_SENSITIVE;
+}
+
+static int __init pmu_acpi_init(void)
+{
+	int err = -ENOMEM;
+
+	if (acpi_disabled)
+		return 0;
+
+	return err;
+}
+arch_initcall(pmu_acpi_init);
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index ef4b760..012deb7 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -158,4 +158,11 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 
 #endif /* CONFIG_ARM_PMU */
 
+#ifdef CONFIG_ARM_PMU_ACPI
+struct acpi_madt_generic_interrupt;
+void arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic);
+#else
+#define arm_pmu_parse_acpi(a, b) do { } while (0)
+#endif /* CONFIG_ARM_PMU_ACPI */
+
 #endif /* __ARM_PMU_H__ */
-- 
2.5.5

[PATCH v7 5/9] arm64: pmu: Add support for probing with ACPI

[Jeremy Linton]

From: Mark Salter <msalter@redhat.com>

In the case of ACPI, the PMU IRQ information is contained in the
MADT table. Also, since the PMU does not exist as a device in the
ACPI DSDT table, it is necessary to create a platform device so
that the appropriate driver probing is triggered.

Signed-off-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/smp.c      |  5 +++++
 drivers/perf/Kconfig         |  4 ++++
 drivers/perf/Makefile        |  1 +
 drivers/perf/arm_pmu_acpi.c  | 51 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/perf/arm_pmu.h |  7 ++++++
 5 files changed, 68 insertions(+)
 create mode 100644 drivers/perf/arm_pmu_acpi.c

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index a6552fe..dc333c6 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -37,6 +37,7 @@
 #include <linux/completion.h>
 #include <linux/of.h>
 #include <linux/irq_work.h>
+#include <linux/perf/arm_pmu.h>
 
 #include <asm/alternative.h>
 #include <asm/atomic.h>
@@ -540,6 +541,7 @@ acpi_verify_and_map_madt(struct acpi_madt_generic_interrupt *processor)
 			return;
 		}
 		bootcpu_valid = true;
+		arm_pmu_parse_acpi(0, processor);
 		return;
 	}
 
@@ -560,6 +562,9 @@ acpi_verify_and_map_madt(struct acpi_madt_generic_interrupt *processor)
 	 */
 	acpi_set_mailbox_entry(cpu_count, processor);
 
+	/* get PMU irq info */
+	arm_pmu_parse_acpi(cpu_count, processor);
+
 	early_map_cpu_to_node(cpu_count, acpi_numa_get_nid(cpu_count, hwid));
 
 	cpu_count++;
diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index 04e2653..818fa3b 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -12,4 +12,8 @@ config ARM_PMU
 	  Say y if you want to use CPU performance monitors on ARM-based
 	  systems.
 
+config ARM_PMU_ACPI
+	def_bool y
+	depends on ARM_PMU && ACPI
+
 endmenu
diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
index acd2397..fd8090d 100644
--- a/drivers/perf/Makefile
+++ b/drivers/perf/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_ARM_PMU) += arm_pmu.o
+obj-$(CONFIG_ARM_PMU_ACPI) += arm_pmu_acpi.o
diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
new file mode 100644
index 0000000..e784714
--- /dev/null
+++ b/drivers/perf/arm_pmu_acpi.c
@@ -0,0 +1,51 @@
+/*
+ * ARM ACPI PMU support
+ *
+ * Copyright (C) 2015 Red Hat Inc.
+ * Author: Mark Salter <msalter@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include <asm/cpu.h>
+#include <linux/acpi.h>
+#include <linux/irq.h>
+#include <linux/irqdesc.h>
+#include <linux/list.h>
+#include <linux/perf/arm_pmu.h>
+#include <linux/platform_device.h>
+
+struct pmu_irq {
+	int  gsi;
+	int  trigger;
+	bool registered;
+};
+
+static struct pmu_irq pmu_irqs[NR_CPUS] __initdata;
+
+/*
+ * Called from acpi_map_gic_cpu_interface()'s MADT parsing during boot.
+ * This routine saves off the GSI's and their trigger state for use when we are
+ * ready to build the PMU platform device.
+*/
+void __init arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic)
+{
+	pmu_irqs[cpu].gsi = gic->performance_interrupt;
+	if (gic->flags & ACPI_MADT_PERFORMANCE_IRQ_MODE)
+		pmu_irqs[cpu].trigger = ACPI_EDGE_SENSITIVE;
+	else
+		pmu_irqs[cpu].trigger = ACPI_LEVEL_SENSITIVE;
+}
+
+static int __init pmu_acpi_init(void)
+{
+	int err = -ENOMEM;
+
+	if (acpi_disabled)
+		return 0;
+
+	return err;
+}
+arch_initcall(pmu_acpi_init);
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index ef4b760..012deb7 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -158,4 +158,11 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 
 #endif /* CONFIG_ARM_PMU */
 
+#ifdef CONFIG_ARM_PMU_ACPI
+struct acpi_madt_generic_interrupt;
+void arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic);
+#else
+#define arm_pmu_parse_acpi(a, b) do { } while (0)
+#endif /* CONFIG_ARM_PMU_ACPI */
+
 #endif /* __ARM_PMU_H__ */
-- 
2.5.5

[PATCH v7 6/9] arm: arm64: Add routine to determine cpuid of other cpus

[Jeremy Linton]

It is helpful if we can read the cpuid/midr of other CPUs
in the system independent of arm/arm64.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm/include/asm/cputype.h   | 2 ++
 arch/arm64/include/asm/cputype.h | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h
index 1ee94c7..d5900a2 100644
--- a/arch/arm/include/asm/cputype.h
+++ b/arch/arm/include/asm/cputype.h
@@ -208,6 +208,8 @@ static inline unsigned int __attribute_const__ read_cpuid_mpidr(void)
 	return read_cpuid(CPUID_MPIDR);
 }
 
+#define read_specific_cpuid(cpu_num) per_cpu_ptr(&cpu_data, cpu_num)->cpuid
+
 /*
  * Intel's XScale3 core supports some v6 features (supersections, L2)
  * but advertises itself as v5 as it does not support the v6 ISA.  For
diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h
index 9d9fd4b..beb95dc 100644
--- a/arch/arm64/include/asm/cputype.h
+++ b/arch/arm64/include/asm/cputype.h
@@ -128,6 +128,9 @@ static inline u32 __attribute_const__ read_cpuid_cachetype(void)
 {
 	return read_cpuid(CTR_EL0);
 }
+
+#define read_specific_cpuid(cpu_num) per_cpu_ptr(&cpu_data, cpu_num)->reg_midr
+
 #endif /* __ASSEMBLY__ */
 
 #endif
-- 
2.5.5

[PATCH v7 6/9] arm: arm64: Add routine to determine cpuid of other cpus

[Jeremy Linton]

It is helpful if we can read the cpuid/midr of other CPUs
in the system independent of arm/arm64.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm/include/asm/cputype.h   | 2 ++
 arch/arm64/include/asm/cputype.h | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h
index 1ee94c7..d5900a2 100644
--- a/arch/arm/include/asm/cputype.h
+++ b/arch/arm/include/asm/cputype.h
@@ -208,6 +208,8 @@ static inline unsigned int __attribute_const__ read_cpuid_mpidr(void)
 	return read_cpuid(CPUID_MPIDR);
 }
 
+#define read_specific_cpuid(cpu_num) per_cpu_ptr(&cpu_data, cpu_num)->cpuid
+
 /*
  * Intel's XScale3 core supports some v6 features (supersections, L2)
  * but advertises itself as v5 as it does not support the v6 ISA.  For
diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h
index 9d9fd4b..beb95dc 100644
--- a/arch/arm64/include/asm/cputype.h
+++ b/arch/arm64/include/asm/cputype.h
@@ -128,6 +128,9 @@ static inline u32 __attribute_const__ read_cpuid_cachetype(void)
 {
 	return read_cpuid(CTR_EL0);
 }
+
+#define read_specific_cpuid(cpu_num) per_cpu_ptr(&cpu_data, cpu_num)->reg_midr
+
 #endif /* __ASSEMBLY__ */
 
 #endif
-- 
2.5.5

[PATCH v7 7/9] arm: arm64: pmu: Assign platform PMU CPU affinity

[Jeremy Linton]

On systems with multiple PMU types the PMU to CPU affinity
needs to be detected and set. The CPU to interrupt affinity
should also be set.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/perf/arm_pmu.c | 53 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index f1aee26..ee9e301 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -11,6 +11,7 @@
  */
 #define pr_fmt(fmt) "hw perfevents: " fmt
 
+#include <linux/acpi.h>
 #include <linux/bitmap.h>
 #include <linux/cpumask.h>
 #include <linux/cpu_pm.h>
@@ -24,6 +25,7 @@
 #include <linux/irq.h>
 #include <linux/irqdesc.h>
 
+#include <asm/cpu.h>
 #include <asm/cputype.h>
 #include <asm/irq_regs.h>
 
@@ -876,25 +878,57 @@ static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
 }
 
 /*
- * CPU PMU identification and probing.
+ * CPU PMU identification and probing. Its possible to have
+ * multiple CPU types in an ARM machine. Assure that we are
+ * picking the right PMU types based on the CPU in question
  */
-static int probe_current_pmu(struct arm_pmu *pmu,
-			     const struct pmu_probe_info *info)
+static int probe_plat_pmu(struct arm_pmu *pmu,
+			     const struct pmu_probe_info *info,
+			     unsigned int pmuid)
 {
-	int cpu = get_cpu();
-	unsigned int cpuid = read_cpuid_id();
 	int ret = -ENODEV;
+	int cpu;
+	int aff_ctr = 0;
+	static int duplicate_pmus;
+	struct platform_device *pdev = pmu->plat_device;
+	int irq = platform_get_irq(pdev, 0);
+
+	if (irq >= 0 && !irq_is_percpu(irq)) {
+		pmu->irq_affinity = kcalloc(pdev->num_resources, sizeof(int),
+					    GFP_KERNEL);
+		if (!pmu->irq_affinity)
+			return -ENOMEM;
+	}
 
-	pr_info("probing PMU on CPU %d\n", cpu);
+	for_each_possible_cpu(cpu) {
+		unsigned int cpuid = read_specific_cpuid(cpu);
 
+		if (cpuid == pmuid) {
+			cpumask_set_cpu(cpu, &pmu->supported_cpus);
+			if (pmu->irq_affinity) {
+				pmu->irq_affinity[aff_ctr] = cpu;
+				aff_ctr++;
+			}
+		}
+	}
+
+	/* find the type of PMU given the CPU */
 	for (; info->init != NULL; info++) {
-		if ((cpuid & info->mask) != info->cpuid)
+		if ((pmuid & info->mask) != info->cpuid)
 			continue;
 		ret = info->init(pmu);
+		if ((!info->cpuid) && (duplicate_pmus)) {
+			pmu->name = kasprintf(GFP_KERNEL, "%s_%d",
+					    pmu->name, duplicate_pmus);
+			if (!pmu->name) {
+				kfree(pmu->irq_affinity);
+				ret = -ENOMEM;
+			}
+		}
+		duplicate_pmus++;
 		break;
 	}
 
-	put_cpu();
 	return ret;
 }
 
@@ -1029,8 +1063,7 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 		if (!ret)
 			ret = init_fn(pmu);
 	} else if (probe_table) {
-		cpumask_setall(&pmu->supported_cpus);
-		ret = probe_current_pmu(pmu, probe_table);
+		ret = probe_plat_pmu(pmu, probe_table, read_cpuid_id());
 	}
 
 	if (ret) {
-- 
2.5.5

[PATCH v7 7/9] arm: arm64: pmu: Assign platform PMU CPU affinity

[Jeremy Linton]

On systems with multiple PMU types the PMU to CPU affinity
needs to be detected and set. The CPU to interrupt affinity
should also be set.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/perf/arm_pmu.c | 53 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index f1aee26..ee9e301 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -11,6 +11,7 @@
  */
 #define pr_fmt(fmt) "hw perfevents: " fmt
 
+#include <linux/acpi.h>
 #include <linux/bitmap.h>
 #include <linux/cpumask.h>
 #include <linux/cpu_pm.h>
@@ -24,6 +25,7 @@
 #include <linux/irq.h>
 #include <linux/irqdesc.h>
 
+#include <asm/cpu.h>
 #include <asm/cputype.h>
 #include <asm/irq_regs.h>
 
@@ -876,25 +878,57 @@ static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
 }
 
 /*
- * CPU PMU identification and probing.
+ * CPU PMU identification and probing. Its possible to have
+ * multiple CPU types in an ARM machine. Assure that we are
+ * picking the right PMU types based on the CPU in question
  */
-static int probe_current_pmu(struct arm_pmu *pmu,
-			     const struct pmu_probe_info *info)
+static int probe_plat_pmu(struct arm_pmu *pmu,
+			     const struct pmu_probe_info *info,
+			     unsigned int pmuid)
 {
-	int cpu = get_cpu();
-	unsigned int cpuid = read_cpuid_id();
 	int ret = -ENODEV;
+	int cpu;
+	int aff_ctr = 0;
+	static int duplicate_pmus;
+	struct platform_device *pdev = pmu->plat_device;
+	int irq = platform_get_irq(pdev, 0);
+
+	if (irq >= 0 && !irq_is_percpu(irq)) {
+		pmu->irq_affinity = kcalloc(pdev->num_resources, sizeof(int),
+					    GFP_KERNEL);
+		if (!pmu->irq_affinity)
+			return -ENOMEM;
+	}
 
-	pr_info("probing PMU on CPU %d\n", cpu);
+	for_each_possible_cpu(cpu) {
+		unsigned int cpuid = read_specific_cpuid(cpu);
 
+		if (cpuid == pmuid) {
+			cpumask_set_cpu(cpu, &pmu->supported_cpus);
+			if (pmu->irq_affinity) {
+				pmu->irq_affinity[aff_ctr] = cpu;
+				aff_ctr++;
+			}
+		}
+	}
+
+	/* find the type of PMU given the CPU */
 	for (; info->init != NULL; info++) {
-		if ((cpuid & info->mask) != info->cpuid)
+		if ((pmuid & info->mask) != info->cpuid)
 			continue;
 		ret = info->init(pmu);
+		if ((!info->cpuid) && (duplicate_pmus)) {
+			pmu->name = kasprintf(GFP_KERNEL, "%s_%d",
+					    pmu->name, duplicate_pmus);
+			if (!pmu->name) {
+				kfree(pmu->irq_affinity);
+				ret = -ENOMEM;
+			}
+		}
+		duplicate_pmus++;
 		break;
 	}
 
-	put_cpu();
 	return ret;
 }
 
@@ -1029,8 +1063,7 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 		if (!ret)
 			ret = init_fn(pmu);
 	} else if (probe_table) {
-		cpumask_setall(&pmu->supported_cpus);
-		ret = probe_current_pmu(pmu, probe_table);
+		ret = probe_plat_pmu(pmu, probe_table, read_cpuid_id());
 	}
 
 	if (ret) {
-- 
2.5.5

[PATCH v7 8/9] arm64: pmu: Detect and enable multiple PMUs in an ACPI system

[Jeremy Linton]

Its possible that an ACPI system has multiple CPU types in it
with differing PMU counters. Iterate the CPU's and make a determination
about how many of each type exist in the system. Then take and create
a PMU platform device for each type, and assign it the interrupts parsed
from the MADT. Creating a platform device is necessary because the PMUs
are not described as devices in the DSDT table.

This code is loosely based on earlier work by Mark Salter.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/perf/arm_pmu.c      |   7 +-
 drivers/perf/arm_pmu_acpi.c | 164 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 170 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index ee9e301..98a037a 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -1063,7 +1063,12 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 		if (!ret)
 			ret = init_fn(pmu);
 	} else if (probe_table) {
-		ret = probe_plat_pmu(pmu, probe_table, read_cpuid_id());
+		if (acpi_disabled) {
+			/* use the current cpu. */
+			ret = probe_plat_pmu(pmu, probe_table,
+					     read_cpuid_id());
+		} else
+			ret = probe_plat_pmu(pmu, probe_table, pdev->id);
 	}
 
 	if (ret) {
diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
index e784714..c0d6888 100644
--- a/drivers/perf/arm_pmu_acpi.c
+++ b/drivers/perf/arm_pmu_acpi.c
@@ -2,13 +2,17 @@
  * ARM ACPI PMU support
  *
  * Copyright (C) 2015 Red Hat Inc.
+ * Copyright (C) 2016 ARM Ltd.
  * Author: Mark Salter <msalter@redhat.com>
+ *         Jeremy Linton <jeremy.linton@arm.com>
  *
  * This work is licensed under the terms of the GNU GPL, version 2.  See
  * the COPYING file in the top-level directory.
  *
  */
 
+#define pr_fmt(fmt) "ACPI-PMU: " fmt
+
 #include <asm/cpu.h>
 #include <linux/acpi.h>
 #include <linux/irq.h>
@@ -23,6 +27,12 @@ struct pmu_irq {
 	bool registered;
 };
 
+struct pmu_types {
+	struct list_head list;
+	int		 cpu_type;
+	int		 cpu_count;
+};
+
 static struct pmu_irq pmu_irqs[NR_CPUS] __initdata;
 
 /*
@@ -39,13 +49,167 @@ void __init arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic)
 		pmu_irqs[cpu].trigger = ACPI_LEVEL_SENSITIVE;
 }
 
+/* Count number and type of CPU cores in the system. */
+static void __init arm_pmu_acpi_determine_cpu_types(struct list_head *pmus)
+{
+	int i;
+	bool alloc_failure = false;
+
+	for_each_possible_cpu(i) {
+		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
+		u32 partnum = MIDR_PARTNUM(cinfo->reg_midr);
+		struct pmu_types *pmu;
+
+		list_for_each_entry(pmu, pmus, list) {
+			if (pmu->cpu_type == partnum) {
+				pmu->cpu_count++;
+				break;
+			}
+		}
+
+		/* we didn't find the CPU type, add an entry to identify it */
+		if ((&pmu->list == pmus) && (!alloc_failure)) {
+			pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL);
+			if (!pmu) {
+				pr_warn("Unable to allocate pmu_types\n");
+				/*
+				 * continue to count cpus for any pmu_types
+				 * already allocated, but don't allocate any
+				 * more pmu_types. This avoids undercounting.
+				 */
+				alloc_failure = true;
+			} else {
+				pmu->cpu_type = partnum;
+				pmu->cpu_count++;
+				list_add_tail(&pmu->list, pmus);
+			}
+		}
+	}
+}
+
+/*
+ * Registers the group of PMU interfaces which correspond to the 'last_cpu_id'.
+ * This group utilizes 'count' resources in the 'res'.
+ */
+static int __init arm_pmu_acpi_register_pmu(int count, struct resource *res,
+					    int last_cpu_id)
+{
+	int i;
+	int err = -ENOMEM;
+	bool free_gsi = false;
+	struct platform_device *pdev;
+
+	if (count) {
+		pdev = platform_device_alloc(ARMV8_PMU_PDEV_NAME, last_cpu_id);
+		if (pdev) {
+			err = platform_device_add_resources(pdev, res, count);
+			if (!err) {
+				err = platform_device_add(pdev);
+				if (err) {
+					pr_warn("Unable to register PMU device\n");
+					free_gsi = true;
+				}
+			} else {
+				pr_warn("Unable to add resources to device\n");
+				free_gsi = true;
+				platform_device_put(pdev);
+			}
+		} else {
+			pr_warn("Unable to allocate platform device\n");
+			free_gsi = true;
+		}
+	}
+
+	/* unmark (and possibly unregister) registered GSIs */
+	for_each_possible_cpu(i) {
+		if (pmu_irqs[i].registered) {
+			if (free_gsi)
+				acpi_unregister_gsi(pmu_irqs[i].gsi);
+			pmu_irqs[i].registered = false;
+		}
+	}
+
+	return err;
+}
+
+/*
+ * For the given cpu/pmu type, walk all known GSIs, register them, and add
+ * them to the resource structure. Return the number of GSI's contained
+ * in the res structure, and the id of the last CPU/PMU we added.
+ */
+static int __init arm_pmu_acpi_gsi_res(struct pmu_types *pmus,
+				       struct resource *res, int *last_cpu_id)
+{
+	int i, count;
+	int irq;
+
+	/* lets group all the PMU's from similar CPU's together */
+	count = 0;
+	for_each_possible_cpu(i) {
+		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
+
+		if (pmus->cpu_type == MIDR_PARTNUM(cinfo->reg_midr)) {
+			if (pmu_irqs[i].gsi == 0)
+				continue;
+
+			irq = acpi_register_gsi(NULL, pmu_irqs[i].gsi,
+						pmu_irqs[i].trigger,
+						ACPI_ACTIVE_HIGH);
+
+			res[count].start = res[count].end = irq;
+			res[count].flags = IORESOURCE_IRQ;
+
+			if (pmu_irqs[i].trigger == ACPI_EDGE_SENSITIVE)
+				res[count].flags |= IORESOURCE_IRQ_HIGHEDGE;
+			else
+				res[count].flags |= IORESOURCE_IRQ_HIGHLEVEL;
+
+			pmu_irqs[i].registered = true;
+			count++;
+			(*last_cpu_id) = cinfo->reg_midr;
+		}
+	}
+	return count;
+}
+
 static int __init pmu_acpi_init(void)
 {
+	struct resource	*res;
 	int err = -ENOMEM;
+	int count, cpu_id;
+	struct pmu_types *pmu, *safe_temp;
+	LIST_HEAD(pmus);
 
 	if (acpi_disabled)
 		return 0;
 
+	arm_pmu_acpi_determine_cpu_types(&pmus);
+
+	list_for_each_entry_safe(pmu, safe_temp, &pmus, list) {
+		res = kcalloc(pmu->cpu_count,
+			      sizeof(struct resource), GFP_KERNEL);
+
+		/* for a given PMU type collect all the GSIs. */
+		if (res) {
+			count = arm_pmu_acpi_gsi_res(pmu, res,
+						     &cpu_id);
+			/*
+			 * register this set of interrupts
+			 * with a new PMU device
+			 */
+			err = arm_pmu_acpi_register_pmu(count, res, cpu_id);
+			if (!err)
+				pr_info("Registered %d devices for %X\n",
+					count, pmu->cpu_type);
+			kfree(res);
+		} else
+			pr_warn("PMU unable to allocate interrupt resource space\n");
+
+		list_del(&pmu->list);
+		kfree(pmu);
+	}
+
 	return err;
 }
+
 arch_initcall(pmu_acpi_init);
-- 
2.5.5

[PATCH v7 8/9] arm64: pmu: Detect and enable multiple PMUs in an ACPI system

[Jeremy Linton]

Its possible that an ACPI system has multiple CPU types in it
with differing PMU counters. Iterate the CPU's and make a determination
about how many of each type exist in the system. Then take and create
a PMU platform device for each type, and assign it the interrupts parsed
from the MADT. Creating a platform device is necessary because the PMUs
are not described as devices in the DSDT table.

This code is loosely based on earlier work by Mark Salter.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/perf/arm_pmu.c      |   7 +-
 drivers/perf/arm_pmu_acpi.c | 164 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 170 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index ee9e301..98a037a 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -1063,7 +1063,12 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 		if (!ret)
 			ret = init_fn(pmu);
 	} else if (probe_table) {
-		ret = probe_plat_pmu(pmu, probe_table, read_cpuid_id());
+		if (acpi_disabled) {
+			/* use the current cpu. */
+			ret = probe_plat_pmu(pmu, probe_table,
+					     read_cpuid_id());
+		} else
+			ret = probe_plat_pmu(pmu, probe_table, pdev->id);
 	}
 
 	if (ret) {
diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
index e784714..c0d6888 100644
--- a/drivers/perf/arm_pmu_acpi.c
+++ b/drivers/perf/arm_pmu_acpi.c
@@ -2,13 +2,17 @@
  * ARM ACPI PMU support
  *
  * Copyright (C) 2015 Red Hat Inc.
+ * Copyright (C) 2016 ARM Ltd.
  * Author: Mark Salter <msalter@redhat.com>
+ *         Jeremy Linton <jeremy.linton@arm.com>
  *
  * This work is licensed under the terms of the GNU GPL, version 2.  See
  * the COPYING file in the top-level directory.
  *
  */
 
+#define pr_fmt(fmt) "ACPI-PMU: " fmt
+
 #include <asm/cpu.h>
 #include <linux/acpi.h>
 #include <linux/irq.h>
@@ -23,6 +27,12 @@ struct pmu_irq {
 	bool registered;
 };
 
+struct pmu_types {
+	struct list_head list;
+	int		 cpu_type;
+	int		 cpu_count;
+};
+
 static struct pmu_irq pmu_irqs[NR_CPUS] __initdata;
 
 /*
@@ -39,13 +49,167 @@ void __init arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic)
 		pmu_irqs[cpu].trigger = ACPI_LEVEL_SENSITIVE;
 }
 
+/* Count number and type of CPU cores in the system. */
+static void __init arm_pmu_acpi_determine_cpu_types(struct list_head *pmus)
+{
+	int i;
+	bool alloc_failure = false;
+
+	for_each_possible_cpu(i) {
+		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
+		u32 partnum = MIDR_PARTNUM(cinfo->reg_midr);
+		struct pmu_types *pmu;
+
+		list_for_each_entry(pmu, pmus, list) {
+			if (pmu->cpu_type == partnum) {
+				pmu->cpu_count++;
+				break;
+			}
+		}
+
+		/* we didn't find the CPU type, add an entry to identify it */
+		if ((&pmu->list == pmus) && (!alloc_failure)) {
+			pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL);
+			if (!pmu) {
+				pr_warn("Unable to allocate pmu_types\n");
+				/*
+				 * continue to count cpus for any pmu_types
+				 * already allocated, but don't allocate any
+				 * more pmu_types. This avoids undercounting.
+				 */
+				alloc_failure = true;
+			} else {
+				pmu->cpu_type = partnum;
+				pmu->cpu_count++;
+				list_add_tail(&pmu->list, pmus);
+			}
+		}
+	}
+}
+
+/*
+ * Registers the group of PMU interfaces which correspond to the 'last_cpu_id'.
+ * This group utilizes 'count' resources in the 'res'.
+ */
+static int __init arm_pmu_acpi_register_pmu(int count, struct resource *res,
+					    int last_cpu_id)
+{
+	int i;
+	int err = -ENOMEM;
+	bool free_gsi = false;
+	struct platform_device *pdev;
+
+	if (count) {
+		pdev = platform_device_alloc(ARMV8_PMU_PDEV_NAME, last_cpu_id);
+		if (pdev) {
+			err = platform_device_add_resources(pdev, res, count);
+			if (!err) {
+				err = platform_device_add(pdev);
+				if (err) {
+					pr_warn("Unable to register PMU device\n");
+					free_gsi = true;
+				}
+			} else {
+				pr_warn("Unable to add resources to device\n");
+				free_gsi = true;
+				platform_device_put(pdev);
+			}
+		} else {
+			pr_warn("Unable to allocate platform device\n");
+			free_gsi = true;
+		}
+	}
+
+	/* unmark (and possibly unregister) registered GSIs */
+	for_each_possible_cpu(i) {
+		if (pmu_irqs[i].registered) {
+			if (free_gsi)
+				acpi_unregister_gsi(pmu_irqs[i].gsi);
+			pmu_irqs[i].registered = false;
+		}
+	}
+
+	return err;
+}
+
+/*
+ * For the given cpu/pmu type, walk all known GSIs, register them, and add
+ * them to the resource structure. Return the number of GSI's contained
+ * in the res structure, and the id of the last CPU/PMU we added.
+ */
+static int __init arm_pmu_acpi_gsi_res(struct pmu_types *pmus,
+				       struct resource *res, int *last_cpu_id)
+{
+	int i, count;
+	int irq;
+
+	/* lets group all the PMU's from similar CPU's together */
+	count = 0;
+	for_each_possible_cpu(i) {
+		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
+
+		if (pmus->cpu_type == MIDR_PARTNUM(cinfo->reg_midr)) {
+			if (pmu_irqs[i].gsi == 0)
+				continue;
+
+			irq = acpi_register_gsi(NULL, pmu_irqs[i].gsi,
+						pmu_irqs[i].trigger,
+						ACPI_ACTIVE_HIGH);
+
+			res[count].start = res[count].end = irq;
+			res[count].flags = IORESOURCE_IRQ;
+
+			if (pmu_irqs[i].trigger == ACPI_EDGE_SENSITIVE)
+				res[count].flags |= IORESOURCE_IRQ_HIGHEDGE;
+			else
+				res[count].flags |= IORESOURCE_IRQ_HIGHLEVEL;
+
+			pmu_irqs[i].registered = true;
+			count++;
+			(*last_cpu_id) = cinfo->reg_midr;
+		}
+	}
+	return count;
+}
+
 static int __init pmu_acpi_init(void)
 {
+	struct resource	*res;
 	int err = -ENOMEM;
+	int count, cpu_id;
+	struct pmu_types *pmu, *safe_temp;
+	LIST_HEAD(pmus);
 
 	if (acpi_disabled)
 		return 0;
 
+	arm_pmu_acpi_determine_cpu_types(&pmus);
+
+	list_for_each_entry_safe(pmu, safe_temp, &pmus, list) {
+		res = kcalloc(pmu->cpu_count,
+			      sizeof(struct resource), GFP_KERNEL);
+
+		/* for a given PMU type collect all the GSIs. */
+		if (res) {
+			count = arm_pmu_acpi_gsi_res(pmu, res,
+						     &cpu_id);
+			/*
+			 * register this set of interrupts
+			 * with a new PMU device
+			 */
+			err = arm_pmu_acpi_register_pmu(count, res, cpu_id);
+			if (!err)
+				pr_info("Registered %d devices for %X\n",
+					count, pmu->cpu_type);
+			kfree(res);
+		} else
+			pr_warn("PMU unable to allocate interrupt resource space\n");
+
+		list_del(&pmu->list);
+		kfree(pmu);
+	}
+
 	return err;
 }
+
 arch_initcall(pmu_acpi_init);
-- 
2.5.5

[PATCH v7 9/9] MAINTAINERS: Tweak ARM PMU maintainers

[Jeremy Linton]

Update the ARM PMU file list, and add the arm mailing list.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 MAINTAINERS | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0bbe4b1..d293270 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -905,12 +905,13 @@ ARM PMU PROFILING AND DEBUGGING
 M:	Will Deacon <will.deacon@arm.com>
 R:	Mark Rutland <mark.rutland@arm.com>
 S:	Maintained
+L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 F:	arch/arm*/kernel/perf_*
 F:	arch/arm/oprofile/common.c
 F:	arch/arm*/kernel/hw_breakpoint.c
 F:	arch/arm*/include/asm/hw_breakpoint.h
 F:	arch/arm*/include/asm/perf_event.h
-F:	drivers/perf/arm_pmu.c
+F:	drivers/perf/arm_pmu*
 F:	include/linux/perf/arm_pmu.h
 
 ARM PORT
-- 
2.5.5

[PATCH v7 9/9] MAINTAINERS: Tweak ARM PMU maintainers

[Jeremy Linton]

Update the ARM PMU file list, and add the arm mailing list.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 MAINTAINERS | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0bbe4b1..d293270 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -905,12 +905,13 @@ ARM PMU PROFILING AND DEBUGGING
 M:	Will Deacon <will.deacon@arm.com>
 R:	Mark Rutland <mark.rutland@arm.com>
 S:	Maintained
+L:	linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
 F:	arch/arm*/kernel/perf_*
 F:	arch/arm/oprofile/common.c
 F:	arch/arm*/kernel/hw_breakpoint.c
 F:	arch/arm*/include/asm/hw_breakpoint.h
 F:	arch/arm*/include/asm/perf_event.h
-F:	drivers/perf/arm_pmu.c
+F:	drivers/perf/arm_pmu*
 F:	include/linux/perf/arm_pmu.h
 
 ARM PORT
-- 
2.5.5

Re: [PATCH v7 1/9] arm64: pmu: add fallback probe table

[Punit Agrawal]

Hi Jeremy,

One comment below.

Jeremy Linton <jeremy.linton@arm.com> writes:

> From: Mark Salter <msalter@redhat.com>
>
> In preparation for ACPI support, add a pmu_probe_info table to
> the arm_pmu_device_probe() call. This table gets used when
> probing in the absence of a devicetree node for PMU.
>
> Signed-off-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---

[...]


> diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
> index e188438..65d8e27 100644
> --- a/include/linux/perf/arm_pmu.h
> +++ b/include/linux/perf/arm_pmu.h
> @@ -147,6 +147,9 @@ struct pmu_probe_info {
>  #define XSCALE_PMU_PROBE(_version, _fn) \
>  	PMU_PROBE(ARM_CPU_IMP_INTEL << 24 | _version, ARM_PMU_XSCALE_MASK, _fn)
>  
> +#define ARMV8_PMU_PART_PROBE(_part, _fn) \
> +	PMU_PROBE((_part) << MIDR_PARTNUM_SHIFT, MIDR_PARTNUM_MASK, _fn)
> +

This hunk could be dropped as it doesn't seem to be used in the
patchset.

With the above change -

FWIW,
        Acked-by: Punit Agrawal <punit.agrawal@arm.com>


>  int arm_pmu_device_probe(struct platform_device *pdev,
>  			 const struct of_device_id *of_table,
>  			 const struct pmu_probe_info *probe_table);

[PATCH v7 1/9] arm64: pmu: add fallback probe table

[Punit Agrawal]

Hi Jeremy,

One comment below.

Jeremy Linton <jeremy.linton@arm.com> writes:

> From: Mark Salter <msalter@redhat.com>
>
> In preparation for ACPI support, add a pmu_probe_info table to
> the arm_pmu_device_probe() call. This table gets used when
> probing in the absence of a devicetree node for PMU.
>
> Signed-off-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---

[...]


> diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
> index e188438..65d8e27 100644
> --- a/include/linux/perf/arm_pmu.h
> +++ b/include/linux/perf/arm_pmu.h
> @@ -147,6 +147,9 @@ struct pmu_probe_info {
>  #define XSCALE_PMU_PROBE(_version, _fn) \
>  	PMU_PROBE(ARM_CPU_IMP_INTEL << 24 | _version, ARM_PMU_XSCALE_MASK, _fn)
>  
> +#define ARMV8_PMU_PART_PROBE(_part, _fn) \
> +	PMU_PROBE((_part) << MIDR_PARTNUM_SHIFT, MIDR_PARTNUM_MASK, _fn)
> +

This hunk could be dropped as it doesn't seem to be used in the
patchset.

With the above change -

FWIW,
        Acked-by: Punit Agrawal <punit.agrawal@arm.com>


>  int arm_pmu_device_probe(struct platform_device *pdev,
>  			 const struct of_device_id *of_table,
>  			 const struct pmu_probe_info *probe_table);

Re: [PATCH v7 5/9] arm64: pmu: Add support for probing with ACPI

[Punit Agrawal]

Hi Jeremy,

A few comments below.

Jeremy Linton <jeremy.linton@arm.com> writes:

> From: Mark Salter <msalter@redhat.com>
>
> In the case of ACPI, the PMU IRQ information is contained in the
> MADT table. Also, since the PMU does not exist as a device in the
> ACPI DSDT table, it is necessary to create a platform device so
> that the appropriate driver probing is triggered.
>

The commit title and message are not true after the re-organisation from
v6. We don't create the platform devices until Patch 8. Maybe something
along the lines of -

"parse the core PMU interrupts from MADT"

More comments below.

> Signed-off-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  arch/arm64/kernel/smp.c      |  5 +++++
>  drivers/perf/Kconfig         |  4 ++++
>  drivers/perf/Makefile        |  1 +
>  drivers/perf/arm_pmu_acpi.c  | 51 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/perf/arm_pmu.h |  7 ++++++
>  5 files changed, 68 insertions(+)
>  create mode 100644 drivers/perf/arm_pmu_acpi.c
>

[...]

> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
> new file mode 100644
> index 0000000..e784714
> --- /dev/null
> +++ b/drivers/perf/arm_pmu_acpi.c
> @@ -0,0 +1,51 @@
> +/*
> + * ARM ACPI PMU support
> + *
> + * Copyright (C) 2015 Red Hat Inc.
> + * Author: Mark Salter <msalter@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <asm/cpu.h>
> +#include <linux/acpi.h>
> +#include <linux/irq.h>
> +#include <linux/irqdesc.h>
> +#include <linux/list.h>
> +#include <linux/perf/arm_pmu.h>
> +#include <linux/platform_device.h>
> +
> +struct pmu_irq {
> +	int  gsi;
> +	int  trigger;
> +	bool registered;
> +};
> +
> +static struct pmu_irq pmu_irqs[NR_CPUS] __initdata;
> +
> +/*
> + * Called from acpi_map_gic_cpu_interface()'s MADT parsing during boot.

The comment needs an update after the rename in Patch 4.


[...]

> +static int __init pmu_acpi_init(void)
> +{
> +	int err = -ENOMEM;
> +
> +	if (acpi_disabled)
> +		return 0;
> +
> +	return err;
> +}
> +arch_initcall(pmu_acpi_init);

The function definition and the arch_initcall don't add any
functionality here and can both be moved to Patch 8.

Thanks,
Punit

> diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
> index ef4b760..012deb7 100644
> --- a/include/linux/perf/arm_pmu.h
> +++ b/include/linux/perf/arm_pmu.h
> @@ -158,4 +158,11 @@ int arm_pmu_device_probe(struct platform_device *pdev,
>  
>  #endif /* CONFIG_ARM_PMU */
>  
> +#ifdef CONFIG_ARM_PMU_ACPI
> +struct acpi_madt_generic_interrupt;
> +void arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic);
> +#else
> +#define arm_pmu_parse_acpi(a, b) do { } while (0)
> +#endif /* CONFIG_ARM_PMU_ACPI */
> +
>  #endif /* __ARM_PMU_H__ */

[PATCH v7 5/9] arm64: pmu: Add support for probing with ACPI

[Punit Agrawal]

Hi Jeremy,

A few comments below.

Jeremy Linton <jeremy.linton@arm.com> writes:

> From: Mark Salter <msalter@redhat.com>
>
> In the case of ACPI, the PMU IRQ information is contained in the
> MADT table. Also, since the PMU does not exist as a device in the
> ACPI DSDT table, it is necessary to create a platform device so
> that the appropriate driver probing is triggered.
>

The commit title and message are not true after the re-organisation from
v6. We don't create the platform devices until Patch 8. Maybe something
along the lines of -

"parse the core PMU interrupts from MADT"

More comments below.

> Signed-off-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  arch/arm64/kernel/smp.c      |  5 +++++
>  drivers/perf/Kconfig         |  4 ++++
>  drivers/perf/Makefile        |  1 +
>  drivers/perf/arm_pmu_acpi.c  | 51 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/perf/arm_pmu.h |  7 ++++++
>  5 files changed, 68 insertions(+)
>  create mode 100644 drivers/perf/arm_pmu_acpi.c
>

[...]

> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
> new file mode 100644
> index 0000000..e784714
> --- /dev/null
> +++ b/drivers/perf/arm_pmu_acpi.c
> @@ -0,0 +1,51 @@
> +/*
> + * ARM ACPI PMU support
> + *
> + * Copyright (C) 2015 Red Hat Inc.
> + * Author: Mark Salter <msalter@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <asm/cpu.h>
> +#include <linux/acpi.h>
> +#include <linux/irq.h>
> +#include <linux/irqdesc.h>
> +#include <linux/list.h>
> +#include <linux/perf/arm_pmu.h>
> +#include <linux/platform_device.h>
> +
> +struct pmu_irq {
> +	int  gsi;
> +	int  trigger;
> +	bool registered;
> +};
> +
> +static struct pmu_irq pmu_irqs[NR_CPUS] __initdata;
> +
> +/*
> + * Called from acpi_map_gic_cpu_interface()'s MADT parsing during boot.

The comment needs an update after the rename in Patch 4.


[...]

> +static int __init pmu_acpi_init(void)
> +{
> +	int err = -ENOMEM;
> +
> +	if (acpi_disabled)
> +		return 0;
> +
> +	return err;
> +}
> +arch_initcall(pmu_acpi_init);

The function definition and the arch_initcall don't add any
functionality here and can both be moved to Patch 8.

Thanks,
Punit

> diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
> index ef4b760..012deb7 100644
> --- a/include/linux/perf/arm_pmu.h
> +++ b/include/linux/perf/arm_pmu.h
> @@ -158,4 +158,11 @@ int arm_pmu_device_probe(struct platform_device *pdev,
>  
>  #endif /* CONFIG_ARM_PMU */
>  
> +#ifdef CONFIG_ARM_PMU_ACPI
> +struct acpi_madt_generic_interrupt;
> +void arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic);
> +#else
> +#define arm_pmu_parse_acpi(a, b) do { } while (0)
> +#endif /* CONFIG_ARM_PMU_ACPI */
> +
>  #endif /* __ARM_PMU_H__ */

Re: [PATCH v7 7/9] arm: arm64: pmu: Assign platform PMU CPU affinity

[Punit Agrawal]

Jeremy Linton <jeremy.linton@arm.com> writes:

> On systems with multiple PMU types the PMU to CPU affinity
> needs to be detected and set. The CPU to interrupt affinity
> should also be set.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  drivers/perf/arm_pmu.c | 53 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index f1aee26..ee9e301 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c

[...]

> @@ -876,25 +878,57 @@ static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
>  }
>  
>  /*
> - * CPU PMU identification and probing.
> + * CPU PMU identification and probing. Its possible to have
> + * multiple CPU types in an ARM machine. Assure that we are
> + * picking the right PMU types based on the CPU in question
>   */
> -static int probe_current_pmu(struct arm_pmu *pmu,
> -			     const struct pmu_probe_info *info)
> +static int probe_plat_pmu(struct arm_pmu *pmu,
> +			     const struct pmu_probe_info *info,
> +			     unsigned int pmuid)
>  {
> -	int cpu = get_cpu();
> -	unsigned int cpuid = read_cpuid_id();
>  	int ret = -ENODEV;
> +	int cpu;
> +	int aff_ctr = 0;
> +	static int duplicate_pmus;
> +	struct platform_device *pdev = pmu->plat_device;
> +	int irq = platform_get_irq(pdev, 0);
> +
> +	if (irq >= 0 && !irq_is_percpu(irq)) {
> +		pmu->irq_affinity = kcalloc(pdev->num_resources, sizeof(int),
> +					    GFP_KERNEL);
> +		if (!pmu->irq_affinity)
> +			return -ENOMEM;
> +	}
>  
> -	pr_info("probing PMU on CPU %d\n", cpu);
> +	for_each_possible_cpu(cpu) {
> +		unsigned int cpuid = read_specific_cpuid(cpu);
>  
> +		if (cpuid == pmuid) {
> +			cpumask_set_cpu(cpu, &pmu->supported_cpus);
> +			if (pmu->irq_affinity) {
> +				pmu->irq_affinity[aff_ctr] = cpu;
> +				aff_ctr++;
> +			}
> +		}
> +	}
> +
> +	/* find the type of PMU given the CPU */
>  	for (; info->init != NULL; info++) {
> -		if ((cpuid & info->mask) != info->cpuid)
> +		if ((pmuid & info->mask) != info->cpuid)
>  			continue;
>  		ret = info->init(pmu);
> +		if ((!info->cpuid) && (duplicate_pmus)) {

It's not obvious what the condition "!info->cpuid" implies. Please add a
comment explaining the rationale.

Also, the parenthesis around the two parts of the logical operation can
be dropped.

> +			pmu->name = kasprintf(GFP_KERNEL, "%s_%d",
> +					    pmu->name, duplicate_pmus);
> +			if (!pmu->name) {
> +				kfree(pmu->irq_affinity);
> +				ret = -ENOMEM;
> +			}
> +		}
> +		duplicate_pmus++;
>  		break;
>  	}
>  
> -	put_cpu();
>  	return ret;
>  }
>  

[...]

[PATCH v7 7/9] arm: arm64: pmu: Assign platform PMU CPU affinity

[Punit Agrawal]

Jeremy Linton <jeremy.linton@arm.com> writes:

> On systems with multiple PMU types the PMU to CPU affinity
> needs to be detected and set. The CPU to interrupt affinity
> should also be set.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  drivers/perf/arm_pmu.c | 53 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index f1aee26..ee9e301 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c

[...]

> @@ -876,25 +878,57 @@ static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
>  }
>  
>  /*
> - * CPU PMU identification and probing.
> + * CPU PMU identification and probing. Its possible to have
> + * multiple CPU types in an ARM machine. Assure that we are
> + * picking the right PMU types based on the CPU in question
>   */
> -static int probe_current_pmu(struct arm_pmu *pmu,
> -			     const struct pmu_probe_info *info)
> +static int probe_plat_pmu(struct arm_pmu *pmu,
> +			     const struct pmu_probe_info *info,
> +			     unsigned int pmuid)
>  {
> -	int cpu = get_cpu();
> -	unsigned int cpuid = read_cpuid_id();
>  	int ret = -ENODEV;
> +	int cpu;
> +	int aff_ctr = 0;
> +	static int duplicate_pmus;
> +	struct platform_device *pdev = pmu->plat_device;
> +	int irq = platform_get_irq(pdev, 0);
> +
> +	if (irq >= 0 && !irq_is_percpu(irq)) {
> +		pmu->irq_affinity = kcalloc(pdev->num_resources, sizeof(int),
> +					    GFP_KERNEL);
> +		if (!pmu->irq_affinity)
> +			return -ENOMEM;
> +	}
>  
> -	pr_info("probing PMU on CPU %d\n", cpu);
> +	for_each_possible_cpu(cpu) {
> +		unsigned int cpuid = read_specific_cpuid(cpu);
>  
> +		if (cpuid == pmuid) {
> +			cpumask_set_cpu(cpu, &pmu->supported_cpus);
> +			if (pmu->irq_affinity) {
> +				pmu->irq_affinity[aff_ctr] = cpu;
> +				aff_ctr++;
> +			}
> +		}
> +	}
> +
> +	/* find the type of PMU given the CPU */
>  	for (; info->init != NULL; info++) {
> -		if ((cpuid & info->mask) != info->cpuid)
> +		if ((pmuid & info->mask) != info->cpuid)
>  			continue;
>  		ret = info->init(pmu);
> +		if ((!info->cpuid) && (duplicate_pmus)) {

It's not obvious what the condition "!info->cpuid" implies. Please add a
comment explaining the rationale.

Also, the parenthesis around the two parts of the logical operation can
be dropped.

> +			pmu->name = kasprintf(GFP_KERNEL, "%s_%d",
> +					    pmu->name, duplicate_pmus);
> +			if (!pmu->name) {
> +				kfree(pmu->irq_affinity);
> +				ret = -ENOMEM;
> +			}
> +		}
> +		duplicate_pmus++;
>  		break;
>  	}
>  
> -	put_cpu();
>  	return ret;
>  }
>  

[...]

Re: [PATCH v7 8/9] arm64: pmu: Detect and enable multiple PMUs in an ACPI system

[Punit Agrawal]

Hi Jeremy,

Jeremy Linton <jeremy.linton@arm.com> writes:

> Its possible that an ACPI system has multiple CPU types in it
> with differing PMU counters. Iterate the CPU's and make a determination
> about how many of each type exist in the system. Then take and create
> a PMU platform device for each type, and assign it the interrupts parsed
> from the MADT. Creating a platform device is necessary because the PMUs
> are not described as devices in the DSDT table.
>
> This code is loosely based on earlier work by Mark Salter.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

Thanks for squashing changes to arm_pmu_acpi.c from different patches in
v6 into one patch. Except for the a function definition in Patch 5 that can
be moved here I think you've got everything. The combined patch is a lot
easier to review.

Some comments below.

> ---
>  drivers/perf/arm_pmu.c      |   7 +-
>  drivers/perf/arm_pmu_acpi.c | 164 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 170 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index ee9e301..98a037a 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -1063,7 +1063,12 @@ int arm_pmu_device_probe(struct platform_device *pdev,
>  		if (!ret)
>  			ret = init_fn(pmu);
>  	} else if (probe_table) {
> -		ret = probe_plat_pmu(pmu, probe_table, read_cpuid_id());
> +		if (acpi_disabled) {
> +			/* use the current cpu. */
> +			ret = probe_plat_pmu(pmu, probe_table,
> +					     read_cpuid_id());
> +		} else
> +			ret = probe_plat_pmu(pmu, probe_table, pdev->id);

Please add matching braces on both sides of the else.

>  	}
>  
>  	if (ret) {
> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
> index e784714..c0d6888 100644
> --- a/drivers/perf/arm_pmu_acpi.c
> +++ b/drivers/perf/arm_pmu_acpi.c

[...]

> @@ -39,13 +49,167 @@ void __init arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic)
>  		pmu_irqs[cpu].trigger = ACPI_LEVEL_SENSITIVE;
>  }
>  
> +/* Count number and type of CPU cores in the system. */
> +static void __init arm_pmu_acpi_determine_cpu_types(struct list_head *pmus)
> +{
> +	int i;
> +	bool alloc_failure = false;
> +
> +	for_each_possible_cpu(i) {
> +		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
> +		u32 partnum = MIDR_PARTNUM(cinfo->reg_midr);
> +		struct pmu_types *pmu;
> +
> +		list_for_each_entry(pmu, pmus, list) {
> +			if (pmu->cpu_type == partnum) {
> +				pmu->cpu_count++;
> +				break;
> +			}
> +		}
> +
> +		/* we didn't find the CPU type, add an entry to identify it */
> +		if ((&pmu->list == pmus) && (!alloc_failure)) {

The parenthesis around the conditions can be dropped.

> +			pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL);
> +			if (!pmu) {
> +				pr_warn("Unable to allocate pmu_types\n");
> +				/*
> +				 * continue to count cpus for any pmu_types
> +				 * already allocated, but don't allocate any
> +				 * more pmu_types. This avoids undercounting.
> +				 */
> +				alloc_failure = true;

Why not just fail probe and return an error? What is the benefit of
having some of the PMUs available?

> +			} else {
> +				pmu->cpu_type = partnum;
> +				pmu->cpu_count++;
> +				list_add_tail(&pmu->list, pmus);
> +			}
> +		}
> +	}
> +}
> +
> +/*
> + * Registers the group of PMU interfaces which correspond to the 'last_cpu_id'.
> + * This group utilizes 'count' resources in the 'res'.
> + */
> +static int __init arm_pmu_acpi_register_pmu(int count, struct resource *res,
> +					    int last_cpu_id)

Please drop the prefix "last_". AFAICS, it doesn't provide any
information.

> +{
> +	int i;
> +	int err = -ENOMEM;
> +	bool free_gsi = false;
> +	struct platform_device *pdev;
> +
> +	if (count) {
> +		pdev = platform_device_alloc(ARMV8_PMU_PDEV_NAME, last_cpu_id);
> +		if (pdev) {
> +			err = platform_device_add_resources(pdev, res, count);
> +			if (!err) {
> +				err = platform_device_add(pdev);
> +				if (err) {
> +					pr_warn("Unable to register PMU device\n");
> +					free_gsi = true;
> +				}
> +			} else {
> +				pr_warn("Unable to add resources to device\n");
> +				free_gsi = true;
> +				platform_device_put(pdev);
> +			}
> +		} else {
> +			pr_warn("Unable to allocate platform device\n");
> +			free_gsi = true;
> +		}
> +	}

This entire "if" block is quite hard to review.

Quoting Documentation/CodingStyle, "if you need more than 3 levels of
indentation, you're screwed anyway, and should fix your program."


> +
> +	/* unmark (and possibly unregister) registered GSIs */
> +	for_each_possible_cpu(i) {
> +		if (pmu_irqs[i].registered) {
> +			if (free_gsi)
> +				acpi_unregister_gsi(pmu_irqs[i].gsi);
> +			pmu_irqs[i].registered = false;
> +		}
> +	}

Moving the for_each_possible_cpu block out of this function should help
makes things simpler. It doesn't have any connection to registering the
platform device and you could then do 

if (!count)
   return count;

which should help reduce a level of indentation. But you can further use
the same approach with other conditions in the block as well.

> +
> +	return err;
> +}
> +
> +/*
> + * For the given cpu/pmu type, walk all known GSIs, register them, and add
> + * them to the resource structure. Return the number of GSI's contained
> + * in the res structure, and the id of the last CPU/PMU we added.
> + */
> +static int __init arm_pmu_acpi_gsi_res(struct pmu_types *pmus,
> +				       struct resource *res, int *last_cpu_id)
> +{
> +	int i, count;
> +	int irq;
> +
> +	/* lets group all the PMU's from similar CPU's together */
> +	count = 0;
> +	for_each_possible_cpu(i) {
> +		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
> +
> +		if (pmus->cpu_type == MIDR_PARTNUM(cinfo->reg_midr)) {
> +			if (pmu_irqs[i].gsi == 0)
> +				continue;

Please don't silently continue if the irq is missing. It deserves a user
visible message. We don't want users complaining about kernel issues
when the firmware fails to provide the required information.

> +
> +			irq = acpi_register_gsi(NULL, pmu_irqs[i].gsi,
> +						pmu_irqs[i].trigger,
> +						ACPI_ACTIVE_HIGH);

Check for the return value of acpi_register_gsi as it can return an
error.

> +
> +			res[count].start = res[count].end = irq;
> +			res[count].flags = IORESOURCE_IRQ;
> +
> +			if (pmu_irqs[i].trigger == ACPI_EDGE_SENSITIVE)
> +				res[count].flags |= IORESOURCE_IRQ_HIGHEDGE;
> +			else
> +				res[count].flags |= IORESOURCE_IRQ_HIGHLEVEL;
> +
> +			pmu_irqs[i].registered = true;
> +			count++;
> +			(*last_cpu_id) = cinfo->reg_midr;

What is the benefit of using the entire MIDR for cpu_id when the
grouping is done on the basis of a subset, i.e., part number.

> +		}
> +	}
> +	return count;
> +}
> +
>  static int __init pmu_acpi_init(void)
>  {
> +	struct resource	*res;
>  	int err = -ENOMEM;
> +	int count, cpu_id;
> +	struct pmu_types *pmu, *safe_temp;
> +	LIST_HEAD(pmus);
>  
>  	if (acpi_disabled)
>  		return 0;
>  
> +	arm_pmu_acpi_determine_cpu_types(&pmus);
> +
> +	list_for_each_entry_safe(pmu, safe_temp, &pmus, list) {
> +		res = kcalloc(pmu->cpu_count,
> +			      sizeof(struct resource), GFP_KERNEL);
> +
> +		/* for a given PMU type collect all the GSIs. */
> +		if (res) {
> +			count = arm_pmu_acpi_gsi_res(pmu, res,
> +						     &cpu_id);
> +			/*
> +			 * register this set of interrupts
> +			 * with a new PMU device
> +			 */
> +			err = arm_pmu_acpi_register_pmu(count, res, cpu_id);
> +			if (!err)
> +				pr_info("Registered %d devices for %X\n",
> +					count, pmu->cpu_type);
> +			kfree(res);
> +		} else
> +			pr_warn("PMU unable to allocate interrupt resource space\n");

Same comment about partial registration as above. It's better to error
out IMO.

Also if this stays, please use matching parenthesis on both sides of the else block.

Thanks,
Punit

> +
> +		list_del(&pmu->list);
> +		kfree(pmu);
> +	}
> +
>  	return err;
>  }
> +
>  arch_initcall(pmu_acpi_init);

[PATCH v7 8/9] arm64: pmu: Detect and enable multiple PMUs in an ACPI system

[Punit Agrawal]

Hi Jeremy,

Jeremy Linton <jeremy.linton@arm.com> writes:

> Its possible that an ACPI system has multiple CPU types in it
> with differing PMU counters. Iterate the CPU's and make a determination
> about how many of each type exist in the system. Then take and create
> a PMU platform device for each type, and assign it the interrupts parsed
> from the MADT. Creating a platform device is necessary because the PMUs
> are not described as devices in the DSDT table.
>
> This code is loosely based on earlier work by Mark Salter.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

Thanks for squashing changes to arm_pmu_acpi.c from different patches in
v6 into one patch. Except for the a function definition in Patch 5 that can
be moved here I think you've got everything. The combined patch is a lot
easier to review.

Some comments below.

> ---
>  drivers/perf/arm_pmu.c      |   7 +-
>  drivers/perf/arm_pmu_acpi.c | 164 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 170 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index ee9e301..98a037a 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -1063,7 +1063,12 @@ int arm_pmu_device_probe(struct platform_device *pdev,
>  		if (!ret)
>  			ret = init_fn(pmu);
>  	} else if (probe_table) {
> -		ret = probe_plat_pmu(pmu, probe_table, read_cpuid_id());
> +		if (acpi_disabled) {
> +			/* use the current cpu. */
> +			ret = probe_plat_pmu(pmu, probe_table,
> +					     read_cpuid_id());
> +		} else
> +			ret = probe_plat_pmu(pmu, probe_table, pdev->id);

Please add matching braces on both sides of the else.

>  	}
>  
>  	if (ret) {
> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
> index e784714..c0d6888 100644
> --- a/drivers/perf/arm_pmu_acpi.c
> +++ b/drivers/perf/arm_pmu_acpi.c

[...]

> @@ -39,13 +49,167 @@ void __init arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic)
>  		pmu_irqs[cpu].trigger = ACPI_LEVEL_SENSITIVE;
>  }
>  
> +/* Count number and type of CPU cores in the system. */
> +static void __init arm_pmu_acpi_determine_cpu_types(struct list_head *pmus)
> +{
> +	int i;
> +	bool alloc_failure = false;
> +
> +	for_each_possible_cpu(i) {
> +		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
> +		u32 partnum = MIDR_PARTNUM(cinfo->reg_midr);
> +		struct pmu_types *pmu;
> +
> +		list_for_each_entry(pmu, pmus, list) {
> +			if (pmu->cpu_type == partnum) {
> +				pmu->cpu_count++;
> +				break;
> +			}
> +		}
> +
> +		/* we didn't find the CPU type, add an entry to identify it */
> +		if ((&pmu->list == pmus) && (!alloc_failure)) {

The parenthesis around the conditions can be dropped.

> +			pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL);
> +			if (!pmu) {
> +				pr_warn("Unable to allocate pmu_types\n");
> +				/*
> +				 * continue to count cpus for any pmu_types
> +				 * already allocated, but don't allocate any
> +				 * more pmu_types. This avoids undercounting.
> +				 */
> +				alloc_failure = true;

Why not just fail probe and return an error? What is the benefit of
having some of the PMUs available?

> +			} else {
> +				pmu->cpu_type = partnum;
> +				pmu->cpu_count++;
> +				list_add_tail(&pmu->list, pmus);
> +			}
> +		}
> +	}
> +}
> +
> +/*
> + * Registers the group of PMU interfaces which correspond to the 'last_cpu_id'.
> + * This group utilizes 'count' resources in the 'res'.
> + */
> +static int __init arm_pmu_acpi_register_pmu(int count, struct resource *res,
> +					    int last_cpu_id)

Please drop the prefix "last_". AFAICS, it doesn't provide any
information.

> +{
> +	int i;
> +	int err = -ENOMEM;
> +	bool free_gsi = false;
> +	struct platform_device *pdev;
> +
> +	if (count) {
> +		pdev = platform_device_alloc(ARMV8_PMU_PDEV_NAME, last_cpu_id);
> +		if (pdev) {
> +			err = platform_device_add_resources(pdev, res, count);
> +			if (!err) {
> +				err = platform_device_add(pdev);
> +				if (err) {
> +					pr_warn("Unable to register PMU device\n");
> +					free_gsi = true;
> +				}
> +			} else {
> +				pr_warn("Unable to add resources to device\n");
> +				free_gsi = true;
> +				platform_device_put(pdev);
> +			}
> +		} else {
> +			pr_warn("Unable to allocate platform device\n");
> +			free_gsi = true;
> +		}
> +	}

This entire "if" block is quite hard to review.

Quoting Documentation/CodingStyle, "if you need more than 3 levels of
indentation, you're screwed anyway, and should fix your program."


> +
> +	/* unmark (and possibly unregister) registered GSIs */
> +	for_each_possible_cpu(i) {
> +		if (pmu_irqs[i].registered) {
> +			if (free_gsi)
> +				acpi_unregister_gsi(pmu_irqs[i].gsi);
> +			pmu_irqs[i].registered = false;
> +		}
> +	}

Moving the for_each_possible_cpu block out of this function should help
makes things simpler. It doesn't have any connection to registering the
platform device and you could then do 

if (!count)
   return count;

which should help reduce a level of indentation. But you can further use
the same approach with other conditions in the block as well.

> +
> +	return err;
> +}
> +
> +/*
> + * For the given cpu/pmu type, walk all known GSIs, register them, and add
> + * them to the resource structure. Return the number of GSI's contained
> + * in the res structure, and the id of the last CPU/PMU we added.
> + */
> +static int __init arm_pmu_acpi_gsi_res(struct pmu_types *pmus,
> +				       struct resource *res, int *last_cpu_id)
> +{
> +	int i, count;
> +	int irq;
> +
> +	/* lets group all the PMU's from similar CPU's together */
> +	count = 0;
> +	for_each_possible_cpu(i) {
> +		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
> +
> +		if (pmus->cpu_type == MIDR_PARTNUM(cinfo->reg_midr)) {
> +			if (pmu_irqs[i].gsi == 0)
> +				continue;

Please don't silently continue if the irq is missing. It deserves a user
visible message. We don't want users complaining about kernel issues
when the firmware fails to provide the required information.

> +
> +			irq = acpi_register_gsi(NULL, pmu_irqs[i].gsi,
> +						pmu_irqs[i].trigger,
> +						ACPI_ACTIVE_HIGH);

Check for the return value of acpi_register_gsi as it can return an
error.

> +
> +			res[count].start = res[count].end = irq;
> +			res[count].flags = IORESOURCE_IRQ;
> +
> +			if (pmu_irqs[i].trigger == ACPI_EDGE_SENSITIVE)
> +				res[count].flags |= IORESOURCE_IRQ_HIGHEDGE;
> +			else
> +				res[count].flags |= IORESOURCE_IRQ_HIGHLEVEL;
> +
> +			pmu_irqs[i].registered = true;
> +			count++;
> +			(*last_cpu_id) = cinfo->reg_midr;

What is the benefit of using the entire MIDR for cpu_id when the
grouping is done on the basis of a subset, i.e., part number.

> +		}
> +	}
> +	return count;
> +}
> +
>  static int __init pmu_acpi_init(void)
>  {
> +	struct resource	*res;
>  	int err = -ENOMEM;
> +	int count, cpu_id;
> +	struct pmu_types *pmu, *safe_temp;
> +	LIST_HEAD(pmus);
>  
>  	if (acpi_disabled)
>  		return 0;
>  
> +	arm_pmu_acpi_determine_cpu_types(&pmus);
> +
> +	list_for_each_entry_safe(pmu, safe_temp, &pmus, list) {
> +		res = kcalloc(pmu->cpu_count,
> +			      sizeof(struct resource), GFP_KERNEL);
> +
> +		/* for a given PMU type collect all the GSIs. */
> +		if (res) {
> +			count = arm_pmu_acpi_gsi_res(pmu, res,
> +						     &cpu_id);
> +			/*
> +			 * register this set of interrupts
> +			 * with a new PMU device
> +			 */
> +			err = arm_pmu_acpi_register_pmu(count, res, cpu_id);
> +			if (!err)
> +				pr_info("Registered %d devices for %X\n",
> +					count, pmu->cpu_type);
> +			kfree(res);
> +		} else
> +			pr_warn("PMU unable to allocate interrupt resource space\n");

Same comment about partial registration as above. It's better to error
out IMO.

Also if this stays, please use matching parenthesis on both sides of the else block.

Thanks,
Punit

> +
> +		list_del(&pmu->list);
> +		kfree(pmu);
> +	}
> +
>  	return err;
>  }
> +
>  arch_initcall(pmu_acpi_init);

Re: [PATCH v7 8/9] arm64: pmu: Detect and enable multiple PMUs in an ACPI system

[Jeremy Linton]

Hi,

On 08/26/2016 10:04 AM, Punit Agrawal wrote:
(trimming)
>> +			pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL);
>> +			if (!pmu) {
>> +				pr_warn("Unable to allocate pmu_types\n");
>> +				/*
>> +				 * continue to count cpus for any pmu_types
>> +				 * already allocated, but don't allocate any
>> +				 * more pmu_types. This avoids undercounting.
>> +				 */
>> +				alloc_failure = true;
>
> Why not just fail probe and return an error? What is the benefit of
> having some of the PMUs available?

AFAIC, there isn't a good reason for penalizing PMU's which we can get 
working if a subset of the system PMUs can't be created. But this is per 
PMU type, so with current systems the kzalloc will be called a max of 2 
times (there is the potential of a 3rd time, due to some other error 
handling, but that doesn't change the argument much). AKA, this doesn't 
result in "partial registration" of a PMU.

So, really the only question in my mind is does it work if one of the 
allocations fails and the other succeeds, and the answer is yes, the 
remaining interrupts/etc get associated with the correct PMU, and it 
gets created and should work as well as perf currently works in systems 
with heterogeneous PMUs (cue discussion about CPU process migration).

So, since this is early boot, and we are taking a tiny allocation, if 
this fails i suspect that the machine will probably die anyway, not due 
to the choice of whether the PMU is counted properly or not. I would 
guess the platform allocation or similar will die..

(trimming)

>> +/*
>> + * For the given cpu/pmu type, walk all known GSIs, register them, and add
>> + * them to the resource structure. Return the number of GSI's contained
>> + * in the res structure, and the id of the last CPU/PMU we added.
>> + */
>> +static int __init arm_pmu_acpi_gsi_res(struct pmu_types *pmus,
>> +				       struct resource *res, int *last_cpu_id)
>> +{
>> +	int i, count;
>> +	int irq;
>> +
>> +	/* lets group all the PMU's from similar CPU's together */
>> +	count = 0;
>> +	for_each_possible_cpu(i) {
>> +		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
>> +
>> +		if (pmus->cpu_type == MIDR_PARTNUM(cinfo->reg_midr)) {
>> +			if (pmu_irqs[i].gsi == 0)
>> +				continue;
>
> Please don't silently continue if the irq is missing. It deserves a user
> visible message. We don't want users complaining about kernel issues
> when the firmware fails to provide the required information.

See below.

>
>> +
>> +			irq = acpi_register_gsi(NULL, pmu_irqs[i].gsi,
>> +						pmu_irqs[i].trigger,
>> +						ACPI_ACTIVE_HIGH);
>
> Check for the return value of acpi_register_gsi as it can return an
> error.

Ok, so this is probably a little subtle, but IIRC from a few months back 
when I was testing this/reworking it, duplicate GSI registrations for 
exclusive PPI's result in errors (see any random discussion about PPI's 
not being ACPI GSI's). As the code to handle SPI vs PPI exists in the 
platform code, I decided to ignore registration errors until such a 
determination can be made. AKA, i'm potentially tossing invalid irq's 
into the irq list for PPIs, but it doesn't matter because they are 
temporary ignored. The core ARM PMU code, has a much better handle on 
what is a correct interrupt binding, so the decision about whether these 
failures need to be worried about are delayed until then. This results 
in a large simplification because we handle the irq deregistration 
necessarily for any further errors together, AKA you will notice a 
complete lack of differing code paths for PPI vs SPI in this module.

As far as gsi=0, in cases where there are placeholder GICC entries in 
the MADT (think disabled CPU) then i've seen gsi=0. I'm not sure that is 
incorrect, so again it gets skipped, the pmu mask doesn't have it 
listed, and everything "works". So if i'm going to add a message here 
i'm, going to wrap it in a reg_midr!=0 check.

>
>> +
>> +			res[count].start = res[count].end = irq;
>> +			res[count].flags = IORESOURCE_IRQ;
>> +
>> +			if (pmu_irqs[i].trigger == ACPI_EDGE_SENSITIVE)
>> +				res[count].flags |= IORESOURCE_IRQ_HIGHEDGE;
>> +			else
>> +				res[count].flags |= IORESOURCE_IRQ_HIGHLEVEL;
>> +
>> +			pmu_irqs[i].registered = true;
>> +			count++;
>> +			(*last_cpu_id) = cinfo->reg_midr;
>
> What is the benefit of using the entire MIDR for cpu_id when the
> grouping is done on the basis of a subset, i.e., part number.

Because the platform code isn't partnum specific, unless the the 
ARMV8_PMU_PART_PROBE() macro is utilized. Delaying any decisions about 
what part of the MIDR happens to be used (if any) to the part probe 
table is probably a good idea. Especially if someone happens to think 
that they want a ARMV8_PMU_PART_REVISION_PROBE() or similar macro..


If anything It might be worth removing the partnum checks in this module 
as well. That way should someone ship a machine with the same CPU only 
differing by revision (or pick your favorite !partnum field) they get 
different PMUs definitions. Why anyone would do that I cannot guess, but 
I do see that apparently the xscale version was important at one point.

[PATCH v7 8/9] arm64: pmu: Detect and enable multiple PMUs in an ACPI system

[Jeremy Linton]

Hi,

On 08/26/2016 10:04 AM, Punit Agrawal wrote:
(trimming)
>> +			pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL);
>> +			if (!pmu) {
>> +				pr_warn("Unable to allocate pmu_types\n");
>> +				/*
>> +				 * continue to count cpus for any pmu_types
>> +				 * already allocated, but don't allocate any
>> +				 * more pmu_types. This avoids undercounting.
>> +				 */
>> +				alloc_failure = true;
>
> Why not just fail probe and return an error? What is the benefit of
> having some of the PMUs available?

AFAIC, there isn't a good reason for penalizing PMU's which we can get 
working if a subset of the system PMUs can't be created. But this is per 
PMU type, so with current systems the kzalloc will be called a max of 2 
times (there is the potential of a 3rd time, due to some other error 
handling, but that doesn't change the argument much). AKA, this doesn't 
result in "partial registration" of a PMU.

So, really the only question in my mind is does it work if one of the 
allocations fails and the other succeeds, and the answer is yes, the 
remaining interrupts/etc get associated with the correct PMU, and it 
gets created and should work as well as perf currently works in systems 
with heterogeneous PMUs (cue discussion about CPU process migration).

So, since this is early boot, and we are taking a tiny allocation, if 
this fails i suspect that the machine will probably die anyway, not due 
to the choice of whether the PMU is counted properly or not. I would 
guess the platform allocation or similar will die..

(trimming)

>> +/*
>> + * For the given cpu/pmu type, walk all known GSIs, register them, and add
>> + * them to the resource structure. Return the number of GSI's contained
>> + * in the res structure, and the id of the last CPU/PMU we added.
>> + */
>> +static int __init arm_pmu_acpi_gsi_res(struct pmu_types *pmus,
>> +				       struct resource *res, int *last_cpu_id)
>> +{
>> +	int i, count;
>> +	int irq;
>> +
>> +	/* lets group all the PMU's from similar CPU's together */
>> +	count = 0;
>> +	for_each_possible_cpu(i) {
>> +		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
>> +
>> +		if (pmus->cpu_type == MIDR_PARTNUM(cinfo->reg_midr)) {
>> +			if (pmu_irqs[i].gsi == 0)
>> +				continue;
>
> Please don't silently continue if the irq is missing. It deserves a user
> visible message. We don't want users complaining about kernel issues
> when the firmware fails to provide the required information.

See below.

>
>> +
>> +			irq = acpi_register_gsi(NULL, pmu_irqs[i].gsi,
>> +						pmu_irqs[i].trigger,
>> +						ACPI_ACTIVE_HIGH);
>
> Check for the return value of acpi_register_gsi as it can return an
> error.

Ok, so this is probably a little subtle, but IIRC from a few months back 
when I was testing this/reworking it, duplicate GSI registrations for 
exclusive PPI's result in errors (see any random discussion about PPI's 
not being ACPI GSI's). As the code to handle SPI vs PPI exists in the 
platform code, I decided to ignore registration errors until such a 
determination can be made. AKA, i'm potentially tossing invalid irq's 
into the irq list for PPIs, but it doesn't matter because they are 
temporary ignored. The core ARM PMU code, has a much better handle on 
what is a correct interrupt binding, so the decision about whether these 
failures need to be worried about are delayed until then. This results 
in a large simplification because we handle the irq deregistration 
necessarily for any further errors together, AKA you will notice a 
complete lack of differing code paths for PPI vs SPI in this module.

As far as gsi=0, in cases where there are placeholder GICC entries in 
the MADT (think disabled CPU) then i've seen gsi=0. I'm not sure that is 
incorrect, so again it gets skipped, the pmu mask doesn't have it 
listed, and everything "works". So if i'm going to add a message here 
i'm, going to wrap it in a reg_midr!=0 check.

>
>> +
>> +			res[count].start = res[count].end = irq;
>> +			res[count].flags = IORESOURCE_IRQ;
>> +
>> +			if (pmu_irqs[i].trigger == ACPI_EDGE_SENSITIVE)
>> +				res[count].flags |= IORESOURCE_IRQ_HIGHEDGE;
>> +			else
>> +				res[count].flags |= IORESOURCE_IRQ_HIGHLEVEL;
>> +
>> +			pmu_irqs[i].registered = true;
>> +			count++;
>> +			(*last_cpu_id) = cinfo->reg_midr;
>
> What is the benefit of using the entire MIDR for cpu_id when the
> grouping is done on the basis of a subset, i.e., part number.

Because the platform code isn't partnum specific, unless the the 
ARMV8_PMU_PART_PROBE() macro is utilized. Delaying any decisions about 
what part of the MIDR happens to be used (if any) to the part probe 
table is probably a good idea. Especially if someone happens to think 
that they want a ARMV8_PMU_PART_REVISION_PROBE() or similar macro..


If anything It might be worth removing the partnum checks in this module 
as well. That way should someone ship a machine with the same CPU only 
differing by revision (or pick your favorite !partnum field) they get 
different PMUs definitions. Why anyone would do that I cannot guess, but 
I do see that apparently the xscale version was important at one point.

Re: [PATCH v7 8/9] arm64: pmu: Detect and enable multiple PMUs in an ACPI system

[Punit Agrawal]

Hi Jeremy,

Jeremy Linton <jeremy.linton@arm.com> writes:

> Hi,
>
> On 08/26/2016 10:04 AM, Punit Agrawal wrote:
> (trimming)
>>> +			pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL);
>>> +			if (!pmu) {
>>> +				pr_warn("Unable to allocate pmu_types\n");
>>> +				/*
>>> +				 * continue to count cpus for any pmu_types
>>> +				 * already allocated, but don't allocate any
>>> +				 * more pmu_types. This avoids undercounting.
>>> +				 */
>>> +				alloc_failure = true;
>>
>> Why not just fail probe and return an error? What is the benefit of
>> having some of the PMUs available?
>
> AFAIC, there isn't a good reason for penalizing PMU's which we can get
> working if a subset of the system PMUs can't be created. But this is
> per PMU type, so with current systems the kzalloc will be called a max
> of 2 times (there is the potential of a 3rd time, due to some other
> error handling, but that doesn't change the argument much). AKA, this
> doesn't result in "partial registration" of a PMU.

If it wasn't clear, by partial registration I was referring to a strict
subset of core PMUs being registered.

>
> So, really the only question in my mind is does it work if one of the
> allocations fails and the other succeeds, and the answer is yes, the
> remaining interrupts/etc get associated with the correct PMU, and it
> gets created and should work as well as perf currently works in
> systems with heterogeneous PMUs (cue discussion about CPU process
> migration).
>
> So, since this is early boot, and we are taking a tiny allocation, if
> this fails i suspect that the machine will probably die anyway, not
> due to the choice of whether the PMU is counted properly or not. I
> would guess the platform allocation or similar will die..

As you have pointed out earlier, if the allocation fails something is
seriously wrong with the system.

In such a situation, I don't see the justification of additional code
complexity (as evidenced in the patch) - when having a subset of PMUs
available isn't even going to be high on the users' priority.

>
> (trimming)
>
>>> +/*
>>> + * For the given cpu/pmu type, walk all known GSIs, register them, and add
>>> + * them to the resource structure. Return the number of GSI's contained
>>> + * in the res structure, and the id of the last CPU/PMU we added.
>>> + */
>>> +static int __init arm_pmu_acpi_gsi_res(struct pmu_types *pmus,
>>> +				       struct resource *res, int *last_cpu_id)
>>> +{
>>> +	int i, count;
>>> +	int irq;
>>> +
>>> +	/* lets group all the PMU's from similar CPU's together */
>>> +	count = 0;
>>> +	for_each_possible_cpu(i) {
>>> +		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
>>> +
>>> +		if (pmus->cpu_type == MIDR_PARTNUM(cinfo->reg_midr)) {
>>> +			if (pmu_irqs[i].gsi == 0)
>>> +				continue;
>>
>> Please don't silently continue if the irq is missing. It deserves a user
>> visible message. We don't want users complaining about kernel issues
>> when the firmware fails to provide the required information.
>
> See below.
>
>>
>>> +
>>> +			irq = acpi_register_gsi(NULL, pmu_irqs[i].gsi,
>>> +						pmu_irqs[i].trigger,
>>> +						ACPI_ACTIVE_HIGH);
>>
>> Check for the return value of acpi_register_gsi as it can return an
>> error.
>
> Ok, so this is probably a little subtle, but IIRC from a few months
> back when I was testing this/reworking it, duplicate GSI registrations
> for exclusive PPI's result in errors (see any random discussion about
> PPI's not being ACPI GSI's).

Is this - improper handling of PPIs in ACPI - something we should be
looking at improving? The current behaviour seems like a bodge we are
working around. I don't think this work should be part of this series.

> As the code to handle SPI vs PPI exists
> in the platform code, I decided to ignore registration errors until
> such a determination can be made. AKA, i'm potentially tossing invalid
> irq's into the irq list for PPIs, but it doesn't matter because they
> are temporary ignored. The core ARM PMU code, has a much better handle
> on what is a correct interrupt binding, so the decision about whether
> these failures need to be worried about are delayed until then. This
> results in a large simplification because we handle the irq
> deregistration necessarily for any further errors together, AKA you
> will notice a complete lack of differing code paths for PPI vs SPI in
> this module.
>
> As far as gsi=0, in cases where there are placeholder GICC entries in
> the MADT (think disabled CPU) then i've seen gsi=0. I'm not sure that
> is incorrect, so again it gets skipped, the pmu mask doesn't have it
> listed, and everything "works". So if i'm going to add a message here
> i'm, going to wrap it in a reg_midr!=0 check.
>
>>
>>> +
>>> +			res[count].start = res[count].end = irq;
>>> +			res[count].flags = IORESOURCE_IRQ;
>>> +
>>> +			if (pmu_irqs[i].trigger == ACPI_EDGE_SENSITIVE)
>>> +				res[count].flags |= IORESOURCE_IRQ_HIGHEDGE;
>>> +			else
>>> +				res[count].flags |= IORESOURCE_IRQ_HIGHLEVEL;
>>> +
>>> +			pmu_irqs[i].registered = true;
>>> +			count++;
>>> +			(*last_cpu_id) = cinfo->reg_midr;
>>
>> What is the benefit of using the entire MIDR for cpu_id when the
>> grouping is done on the basis of a subset, i.e., part number.
>
> Because the platform code isn't partnum specific, unless the the
> ARMV8_PMU_PART_PROBE() macro is utilized. Delaying any decisions about
> what part of the MIDR happens to be used (if any) to the part probe
> table is probably a good idea. Especially if someone happens to think
> that they want a ARMV8_PMU_PART_REVISION_PROBE() or similar macro..
>
>
> If anything It might be worth removing the partnum checks in this
> module as well. That way should someone ship a machine with the same
> CPU only differing by revision (or pick your favorite !partnum field)
> they get different PMUs definitions. Why anyone would do that I cannot
> guess, but I do see that apparently the xscale version was important
> at one point.

It is generally a hard problem to guess what implementations might do in
the future. ;)

Punit

>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 8/9] arm64: pmu: Detect and enable multiple PMUs in an ACPI system

[Punit Agrawal]

Hi Jeremy,

Jeremy Linton <jeremy.linton@arm.com> writes:

> Hi,
>
> On 08/26/2016 10:04 AM, Punit Agrawal wrote:
> (trimming)
>>> +			pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL);
>>> +			if (!pmu) {
>>> +				pr_warn("Unable to allocate pmu_types\n");
>>> +				/*
>>> +				 * continue to count cpus for any pmu_types
>>> +				 * already allocated, but don't allocate any
>>> +				 * more pmu_types. This avoids undercounting.
>>> +				 */
>>> +				alloc_failure = true;
>>
>> Why not just fail probe and return an error? What is the benefit of
>> having some of the PMUs available?
>
> AFAIC, there isn't a good reason for penalizing PMU's which we can get
> working if a subset of the system PMUs can't be created. But this is
> per PMU type, so with current systems the kzalloc will be called a max
> of 2 times (there is the potential of a 3rd time, due to some other
> error handling, but that doesn't change the argument much). AKA, this
> doesn't result in "partial registration" of a PMU.

If it wasn't clear, by partial registration I was referring to a strict
subset of core PMUs being registered.

>
> So, really the only question in my mind is does it work if one of the
> allocations fails and the other succeeds, and the answer is yes, the
> remaining interrupts/etc get associated with the correct PMU, and it
> gets created and should work as well as perf currently works in
> systems with heterogeneous PMUs (cue discussion about CPU process
> migration).
>
> So, since this is early boot, and we are taking a tiny allocation, if
> this fails i suspect that the machine will probably die anyway, not
> due to the choice of whether the PMU is counted properly or not. I
> would guess the platform allocation or similar will die..

As you have pointed out earlier, if the allocation fails something is
seriously wrong with the system.

In such a situation, I don't see the justification of additional code
complexity (as evidenced in the patch) - when having a subset of PMUs
available isn't even going to be high on the users' priority.

>
> (trimming)
>
>>> +/*
>>> + * For the given cpu/pmu type, walk all known GSIs, register them, and add
>>> + * them to the resource structure. Return the number of GSI's contained
>>> + * in the res structure, and the id of the last CPU/PMU we added.
>>> + */
>>> +static int __init arm_pmu_acpi_gsi_res(struct pmu_types *pmus,
>>> +				       struct resource *res, int *last_cpu_id)
>>> +{
>>> +	int i, count;
>>> +	int irq;
>>> +
>>> +	/* lets group all the PMU's from similar CPU's together */
>>> +	count = 0;
>>> +	for_each_possible_cpu(i) {
>>> +		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
>>> +
>>> +		if (pmus->cpu_type == MIDR_PARTNUM(cinfo->reg_midr)) {
>>> +			if (pmu_irqs[i].gsi == 0)
>>> +				continue;
>>
>> Please don't silently continue if the irq is missing. It deserves a user
>> visible message. We don't want users complaining about kernel issues
>> when the firmware fails to provide the required information.
>
> See below.
>
>>
>>> +
>>> +			irq = acpi_register_gsi(NULL, pmu_irqs[i].gsi,
>>> +						pmu_irqs[i].trigger,
>>> +						ACPI_ACTIVE_HIGH);
>>
>> Check for the return value of acpi_register_gsi as it can return an
>> error.
>
> Ok, so this is probably a little subtle, but IIRC from a few months
> back when I was testing this/reworking it, duplicate GSI registrations
> for exclusive PPI's result in errors (see any random discussion about
> PPI's not being ACPI GSI's).

Is this - improper handling of PPIs in ACPI - something we should be
looking at improving? The current behaviour seems like a bodge we are
working around. I don't think this work should be part of this series.

> As the code to handle SPI vs PPI exists
> in the platform code, I decided to ignore registration errors until
> such a determination can be made. AKA, i'm potentially tossing invalid
> irq's into the irq list for PPIs, but it doesn't matter because they
> are temporary ignored. The core ARM PMU code, has a much better handle
> on what is a correct interrupt binding, so the decision about whether
> these failures need to be worried about are delayed until then. This
> results in a large simplification because we handle the irq
> deregistration necessarily for any further errors together, AKA you
> will notice a complete lack of differing code paths for PPI vs SPI in
> this module.
>
> As far as gsi=0, in cases where there are placeholder GICC entries in
> the MADT (think disabled CPU) then i've seen gsi=0. I'm not sure that
> is incorrect, so again it gets skipped, the pmu mask doesn't have it
> listed, and everything "works". So if i'm going to add a message here
> i'm, going to wrap it in a reg_midr!=0 check.
>
>>
>>> +
>>> +			res[count].start = res[count].end = irq;
>>> +			res[count].flags = IORESOURCE_IRQ;
>>> +
>>> +			if (pmu_irqs[i].trigger == ACPI_EDGE_SENSITIVE)
>>> +				res[count].flags |= IORESOURCE_IRQ_HIGHEDGE;
>>> +			else
>>> +				res[count].flags |= IORESOURCE_IRQ_HIGHLEVEL;
>>> +
>>> +			pmu_irqs[i].registered = true;
>>> +			count++;
>>> +			(*last_cpu_id) = cinfo->reg_midr;
>>
>> What is the benefit of using the entire MIDR for cpu_id when the
>> grouping is done on the basis of a subset, i.e., part number.
>
> Because the platform code isn't partnum specific, unless the the
> ARMV8_PMU_PART_PROBE() macro is utilized. Delaying any decisions about
> what part of the MIDR happens to be used (if any) to the part probe
> table is probably a good idea. Especially if someone happens to think
> that they want a ARMV8_PMU_PART_REVISION_PROBE() or similar macro..
>
>
> If anything It might be worth removing the partnum checks in this
> module as well. That way should someone ship a machine with the same
> CPU only differing by revision (or pick your favorite !partnum field)
> they get different PMUs definitions. Why anyone would do that I cannot
> guess, but I do see that apparently the xscale version was important
> at one point.

It is generally a hard problem to guess what implementations might do in
the future. ;)

Punit

>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 8/9] arm64: pmu: Detect and enable multiple PMUs in an ACPI system

[Will Deacon]

On Fri, Aug 26, 2016 at 05:44:59PM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 08/26/2016 10:04 AM, Punit Agrawal wrote:
> (trimming)
> >>+			pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL);
> >>+			if (!pmu) {
> >>+				pr_warn("Unable to allocate pmu_types\n");
> >>+				/*
> >>+				 * continue to count cpus for any pmu_types
> >>+				 * already allocated, but don't allocate any
> >>+				 * more pmu_types. This avoids undercounting.
> >>+				 */
> >>+				alloc_failure = true;
> >
> >Why not just fail probe and return an error? What is the benefit of
> >having some of the PMUs available?
> 
> AFAIC, there isn't a good reason for penalizing PMU's which we can get
> working if a subset of the system PMUs can't be created. But this is per PMU
> type, so with current systems the kzalloc will be called a max of 2 times
> (there is the potential of a 3rd time, due to some other error handling, but
> that doesn't change the argument much). AKA, this doesn't result in "partial
> registration" of a PMU.

... but this will look mighty confusing to userspace, where things will
appear to "half-work", if for some reason the machine makes it that far
at all.

I think we should stick with the KISS approach and just fail the probe
as Punit is suggesting.

Will

[PATCH v7 8/9] arm64: pmu: Detect and enable multiple PMUs in an ACPI system

[Will Deacon]

On Fri, Aug 26, 2016 at 05:44:59PM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 08/26/2016 10:04 AM, Punit Agrawal wrote:
> (trimming)
> >>+			pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL);
> >>+			if (!pmu) {
> >>+				pr_warn("Unable to allocate pmu_types\n");
> >>+				/*
> >>+				 * continue to count cpus for any pmu_types
> >>+				 * already allocated, but don't allocate any
> >>+				 * more pmu_types. This avoids undercounting.
> >>+				 */
> >>+				alloc_failure = true;
> >
> >Why not just fail probe and return an error? What is the benefit of
> >having some of the PMUs available?
> 
> AFAIC, there isn't a good reason for penalizing PMU's which we can get
> working if a subset of the system PMUs can't be created. But this is per PMU
> type, so with current systems the kzalloc will be called a max of 2 times
> (there is the potential of a 3rd time, due to some other error handling, but
> that doesn't change the argument much). AKA, this doesn't result in "partial
> registration" of a PMU.

... but this will look mighty confusing to userspace, where things will
appear to "half-work", if for some reason the machine makes it that far
at all.

I think we should stick with the KISS approach and just fail the probe
as Punit is suggesting.

Will