[PATCH v3 0/3] perf: Add Arm Network-on-Chip PMU driver

[PATCH v3 0/3] perf: Add Arm Network-on-Chip PMU driver

[Robin Murphy]

v2: https://lore.kernel.org/linux-arm-kernel/cover.1720625639.git.robin.murphy@arm.com/

Hi all,

Hopefully this is good for 6.12 now. I've not managed to double-check
the interaction with userspace PMU aggregation behaviour yet, but I
think that's something to look into separately as it's not specific to
this driver.

Thanks,
Robin.


Robin Murphy (3):
  dt-bindings/perf: Add Arm NI-700 PMU
  perf: Add driver for Arm NI-700 interconnect PMU
  MAINTAINERS: List Arm interconnect PMUs as supported

 Documentation/admin-guide/perf/arm-ni.rst     |  17 +
 Documentation/admin-guide/perf/index.rst      |   1 +
 .../devicetree/bindings/perf/arm,ni.yaml      |  30 +
 MAINTAINERS                                   |  11 +
 drivers/perf/Kconfig                          |   7 +
 drivers/perf/Makefile                         |   1 +
 drivers/perf/arm-ni.c                         | 778 ++++++++++++++++++
 7 files changed, 845 insertions(+)
 create mode 100644 Documentation/admin-guide/perf/arm-ni.rst
 create mode 100644 Documentation/devicetree/bindings/perf/arm,ni.yaml
 create mode 100644 drivers/perf/arm-ni.c

-- 
2.39.2.101.g768bb238c484.dirty

[PATCH v3 1/3] dt-bindings/perf: Add Arm NI-700 PMU

[Robin Murphy]

Add an initial binding for the Arm NI-700 interconnect PMU. As with the
Arm CMN family, there are already future NI products on the roadmap, so
the overall binding is named generically just in case any
non-discoverable incompatibility between generations crops up.

Cc: <devicetree@vger.kernel.org>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v3: No change

 .../devicetree/bindings/perf/arm,ni.yaml      | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/perf/arm,ni.yaml

diff --git a/Documentation/devicetree/bindings/perf/arm,ni.yaml b/Documentation/devicetree/bindings/perf/arm,ni.yaml
new file mode 100644
index 000000000000..d66fffa256d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/perf/arm,ni.yaml
@@ -0,0 +1,30 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/perf/arm,ni.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Arm NI (Network-on-Chip Interconnect) Performance Monitors
+
+maintainers:
+  - Robin Murphy <robin.murphy@arm.com>
+
+properties:
+  compatible:
+    const: arm,ni-700
+
+  reg:
+    items:
+      - description: Complete configuration register space
+
+  interrupts:
+    minItems: 1
+    maxItems: 32
+    description: Overflow interrupts, one per clock domain, in order of domain ID
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH v3 2/3] perf: Add driver for Arm NI-700 interconnect PMU

[Robin Murphy]

The Arm NI-700 Network-on-Chip Interconnect has a relatively
straightforward design with a hierarchy of voltage, power, and clock
domains, where each clock domain then contains a number of interface
units and a PMU which can monitor events thereon. As such, it begets a
relatively straightforward driver to interface those PMUs with perf.

Even more so than with arm-cmn, users will require detailed knowledge of
the wider system topology in order to meaningfully analyse anything,
since the interconnect itself cannot know what lies beyond the boundary
of each inscrutably-numbered interface. Given that, for now they are
also expected to refer to the NI-700 documentation for the relevant
event IDs to provide as well. An identifier is implemented so we can
come back and add jevents if anyone really wants to.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>

---
v2:
 - Add basic usage documentation
 - Use __counted_by attribute
 - Make group validation logic clearer (and drop PMU type check
   which perf_event_open() already takes care of)
 - Add retry limit to arm_ni_read_ccnt()
v3:
 - Update .remove to return void
 - Fix group leader validation and make the naming clearer
 - Drop NUMA_NO_NODE check for CPU online (the only way that could
   actually pass both other migration conditions is if the NUMA info
   is so messed up that it's not worth worrying about anyway)
---
 Documentation/admin-guide/perf/arm-ni.rst |  17 +
 Documentation/admin-guide/perf/index.rst  |   1 +
 drivers/perf/Kconfig                      |   7 +
 drivers/perf/Makefile                     |   1 +
 drivers/perf/arm-ni.c                     | 778 ++++++++++++++++++++++
 5 files changed, 804 insertions(+)
 create mode 100644 Documentation/admin-guide/perf/arm-ni.rst
 create mode 100644 drivers/perf/arm-ni.c

diff --git a/Documentation/admin-guide/perf/arm-ni.rst b/Documentation/admin-guide/perf/arm-ni.rst
new file mode 100644
index 000000000000..3cd7d0f75f0f
--- /dev/null
+++ b/Documentation/admin-guide/perf/arm-ni.rst
@@ -0,0 +1,17 @@
+====================================
+Arm Network-on Chip Interconnect PMU
+====================================
+
+NI-700 and friends implement a distinct PMU for each clock domain within the
+interconnect. Correspondingly, the driver exposes multiple PMU devices named
+arm_ni_<x>_cd_<y>, where <x> is an (abritrary) instance identifier and <y> is
+the clock domain ID within that particular instance. If multiple NI instances
+exist within a system, the PMU devices can be correlated with the underlying
+hardware instance via sysfs parentage.
+
+Each PMU exposes base event aliases for the interface types present in its clock
+domain. These require qualifying with the "eventid" and "nodeid" parameters
+to specify the event code to count and the interface at which to count it
+(per the configured hardware ID as reflected in the xxNI_NODE_INFO register).
+The exception is the "cycles" alias for the PMU cycle counter, which is encoded
+with the PMU node type and needs no further qualification.
diff --git a/Documentation/admin-guide/perf/index.rst b/Documentation/admin-guide/perf/index.rst
index 7eb3dcd6f4da..8502bc174640 100644
--- a/Documentation/admin-guide/perf/index.rst
+++ b/Documentation/admin-guide/perf/index.rst
@@ -16,6 +16,7 @@ Performance monitor support
    starfive_starlink_pmu
    arm-ccn
    arm-cmn
+   arm-ni
    xgene-pmu
    arm_dsu_pmu
    thunderx2-pmu
diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index aa9530b4064f..bab8ba64162f 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -48,6 +48,13 @@ config ARM_CMN
 	  Support for PMU events monitoring on the Arm CMN-600 Coherent Mesh
 	  Network interconnect.
 
+config ARM_NI
+	tristate "Arm NI-700 PMU support"
+	depends on ARM64 || COMPILE_TEST
+	help
+	  Support for PMU events monitoring on the Arm NI-700 Network-on-Chip
+	  interconnect and family.
+
 config ARM_PMU
 	depends on ARM || ARM64
 	bool "ARM PMU framework"
diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
index d43df81d52f7..8268f38e42c5 100644
--- a/drivers/perf/Makefile
+++ b/drivers/perf/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_ARM_CCI_PMU) += arm-cci.o
 obj-$(CONFIG_ARM_CCN) += arm-ccn.o
 obj-$(CONFIG_ARM_CMN) += arm-cmn.o
 obj-$(CONFIG_ARM_DSU_PMU) += arm_dsu_pmu.o
+obj-$(CONFIG_ARM_NI) += arm-ni.o
 obj-$(CONFIG_ARM_PMU) += arm_pmu.o arm_pmu_platform.o
 obj-$(CONFIG_ARM_PMU_ACPI) += arm_pmu_acpi.o
 obj-$(CONFIG_ARM_PMUV3) += arm_pmuv3.o
diff --git a/drivers/perf/arm-ni.c b/drivers/perf/arm-ni.c
new file mode 100644
index 000000000000..a4f6746e9786
--- /dev/null
+++ b/drivers/perf/arm-ni.c
@@ -0,0 +1,778 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2022-2024 Arm Limited
+// NI-700 Network-on-Chip PMU driver
+
+#include <linux/acpi.h>
+#include <linux/bitfield.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/perf_event.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/* Common registers */
+#define NI_NODE_TYPE		0x000
+#define NI_NODE_TYPE_NODE_ID	GENMASK(31, 16)
+#define NI_NODE_TYPE_NODE_TYPE	GENMASK(15, 0)
+
+#define NI_CHILD_NODE_INFO	0x004
+#define NI_CHILD_PTR(n)		(0x008 + (n) * 4)
+
+#define NI700_PMUSELA		0x00c
+
+/* Config node */
+#define NI_PERIPHERAL_ID0	0xfe0
+#define NI_PIDR0_PART_7_0	GENMASK(7, 0)
+#define NI_PERIPHERAL_ID1	0xfe4
+#define NI_PIDR1_PART_11_8	GENMASK(3, 0)
+#define NI_PERIPHERAL_ID2	0xfe8
+#define NI_PIDR2_VERSION	GENMASK(7, 4)
+
+/* PMU node */
+#define NI_PMEVCNTR(n)		(0x008 + (n) * 8)
+#define NI_PMCCNTR_L		0x0f8
+#define NI_PMCCNTR_U		0x0fc
+#define NI_PMEVTYPER(n)		(0x400 + (n) * 4)
+#define NI_PMEVTYPER_NODE_TYPE	GENMASK(12, 9)
+#define NI_PMEVTYPER_NODE_ID	GENMASK(8, 0)
+#define NI_PMCNTENSET		0xc00
+#define NI_PMCNTENCLR		0xc20
+#define NI_PMINTENSET		0xc40
+#define NI_PMINTENCLR		0xc60
+#define NI_PMOVSCLR		0xc80
+#define NI_PMOVSSET		0xcc0
+#define NI_PMCFGR		0xe00
+#define NI_PMCR			0xe04
+#define NI_PMCR_RESET_CCNT	BIT(2)
+#define NI_PMCR_RESET_EVCNT	BIT(1)
+#define NI_PMCR_ENABLE		BIT(0)
+
+#define NI_NUM_COUNTERS		8
+#define NI_CCNT_IDX		31
+
+/* Event attributes */
+#define NI_CONFIG_TYPE		GENMASK_ULL(15, 0)
+#define NI_CONFIG_NODEID	GENMASK_ULL(31, 16)
+#define NI_CONFIG_EVENTID	GENMASK_ULL(47, 32)
+
+#define NI_EVENT_TYPE(event)	FIELD_GET(NI_CONFIG_TYPE, (event)->attr.config)
+#define NI_EVENT_NODEID(event)	FIELD_GET(NI_CONFIG_NODEID, (event)->attr.config)
+#define NI_EVENT_EVENTID(event)	FIELD_GET(NI_CONFIG_EVENTID, (event)->attr.config)
+
+enum ni_part {
+	PART_NI_700 = 0x43b,
+	PART_NI_710AE = 0x43d,
+};
+
+enum ni_node_type {
+	NI_GLOBAL,
+	NI_VOLTAGE,
+	NI_POWER,
+	NI_CLOCK,
+	NI_ASNI,
+	NI_AMNI,
+	NI_PMU,
+	NI_HSNI,
+	NI_HMNI,
+	NI_PMNI,
+};
+
+struct arm_ni_node {
+	void __iomem *base;
+	enum ni_node_type type;
+	u16 id;
+	u32 num_components;
+};
+
+struct arm_ni_unit {
+	void __iomem *pmusela;
+	enum ni_node_type type;
+	u16 id;
+	bool ns;
+	union {
+		__le64 pmusel;
+		u8 event[8];
+	};
+};
+
+struct arm_ni_cd {
+	void __iomem *pmu_base;
+	u16 id;
+	int num_units;
+	int irq;
+	int cpu;
+	struct hlist_node cpuhp_node;
+	struct pmu pmu;
+	struct arm_ni_unit *units;
+	struct perf_event *evcnt[NI_NUM_COUNTERS];
+	struct perf_event *ccnt;
+};
+
+struct arm_ni {
+	struct device *dev;
+	void __iomem *base;
+	enum ni_part part;
+	int id;
+	int num_cds;
+	struct arm_ni_cd cds[] __counted_by(num_cds);
+};
+
+#define cd_to_ni(cd) container_of((cd), struct arm_ni, cds[(cd)->id])
+#define pmu_to_cd(p) container_of((p), struct arm_ni_cd, pmu)
+
+#define cd_for_each_unit(cd, u) \
+	for (struct arm_ni_unit *u = cd->units; u < cd->units + cd->num_units; u++)
+
+static int arm_ni_hp_state;
+
+struct arm_ni_event_attr {
+	struct device_attribute attr;
+	enum ni_node_type type;
+};
+
+#define NI_EVENT_ATTR(_name, _type)					\
+	(&((struct arm_ni_event_attr[]) {{				\
+		.attr = __ATTR(_name, 0444, arm_ni_event_show, NULL),	\
+		.type = _type,						\
+	}})[0].attr.attr)
+
+static ssize_t arm_ni_event_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct arm_ni_event_attr *eattr = container_of(attr, typeof(*eattr), attr);
+
+	if (eattr->type == NI_PMU)
+		return sysfs_emit(buf, "type=0x%x\n", eattr->type);
+
+	return sysfs_emit(buf, "type=0x%x,eventid=?,nodeid=?\n", eattr->type);
+}
+
+static umode_t arm_ni_event_attr_is_visible(struct kobject *kobj,
+					    struct attribute *attr, int unused)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct arm_ni_cd *cd = pmu_to_cd(dev_get_drvdata(dev));
+	struct arm_ni_event_attr *eattr;
+
+	eattr = container_of(attr, typeof(*eattr), attr.attr);
+
+	cd_for_each_unit(cd, unit) {
+		if (unit->type == eattr->type && unit->ns)
+			return attr->mode;
+	}
+
+	return 0;
+}
+
+static struct attribute *arm_ni_event_attrs[] = {
+	NI_EVENT_ATTR(asni, NI_ASNI),
+	NI_EVENT_ATTR(amni, NI_AMNI),
+	NI_EVENT_ATTR(cycles, NI_PMU),
+	NI_EVENT_ATTR(hsni, NI_HSNI),
+	NI_EVENT_ATTR(hmni, NI_HMNI),
+	NI_EVENT_ATTR(pmni, NI_PMNI),
+	NULL
+};
+
+static const struct attribute_group arm_ni_event_attrs_group = {
+	.name = "events",
+	.attrs = arm_ni_event_attrs,
+	.is_visible = arm_ni_event_attr_is_visible,
+};
+
+struct arm_ni_format_attr {
+	struct device_attribute attr;
+	u64 field;
+};
+
+#define NI_FORMAT_ATTR(_name, _fld)					\
+	(&((struct arm_ni_format_attr[]) {{				\
+		.attr = __ATTR(_name, 0444, arm_ni_format_show, NULL),	\
+		.field = _fld,						\
+	}})[0].attr.attr)
+
+static ssize_t arm_ni_format_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct arm_ni_format_attr *fmt = container_of(attr, typeof(*fmt), attr);
+	int lo = __ffs(fmt->field), hi = __fls(fmt->field);
+
+	return sysfs_emit(buf, "config:%d-%d\n", lo, hi);
+}
+
+static struct attribute *arm_ni_format_attrs[] = {
+	NI_FORMAT_ATTR(type, NI_CONFIG_TYPE),
+	NI_FORMAT_ATTR(nodeid, NI_CONFIG_NODEID),
+	NI_FORMAT_ATTR(eventid, NI_CONFIG_EVENTID),
+	NULL
+};
+
+static const struct attribute_group arm_ni_format_attrs_group = {
+	.name = "format",
+	.attrs = arm_ni_format_attrs,
+};
+
+static ssize_t arm_ni_cpumask_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct arm_ni_cd *cd = pmu_to_cd(dev_get_drvdata(dev));
+
+	return cpumap_print_to_pagebuf(true, buf, cpumask_of(cd->cpu));
+}
+
+static struct device_attribute arm_ni_cpumask_attr =
+		__ATTR(cpumask, 0444, arm_ni_cpumask_show, NULL);
+
+static ssize_t arm_ni_identifier_show(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct arm_ni *ni = cd_to_ni(pmu_to_cd(dev_get_drvdata(dev)));
+	u32 reg = readl_relaxed(ni->base + NI_PERIPHERAL_ID2);
+	int version = FIELD_GET(NI_PIDR2_VERSION, reg);
+
+	return sysfs_emit(buf, "%03x%02x\n", ni->part, version);
+}
+
+static struct device_attribute arm_ni_identifier_attr =
+		__ATTR(identifier, 0444, arm_ni_identifier_show, NULL);
+
+static struct attribute *arm_ni_other_attrs[] = {
+	&arm_ni_cpumask_attr.attr,
+	&arm_ni_identifier_attr.attr,
+	NULL
+};
+
+static const struct attribute_group arm_ni_other_attr_group = {
+	.attrs = arm_ni_other_attrs,
+	NULL
+};
+
+static const struct attribute_group *arm_ni_attr_groups[] = {
+	&arm_ni_event_attrs_group,
+	&arm_ni_format_attrs_group,
+	&arm_ni_other_attr_group,
+	NULL
+};
+
+static void arm_ni_pmu_enable(struct pmu *pmu)
+{
+	writel_relaxed(NI_PMCR_ENABLE, pmu_to_cd(pmu)->pmu_base + NI_PMCR);
+}
+
+static void arm_ni_pmu_disable(struct pmu *pmu)
+{
+	writel_relaxed(0, pmu_to_cd(pmu)->pmu_base + NI_PMCR);
+}
+
+struct arm_ni_val {
+	unsigned int evcnt;
+	unsigned int ccnt;
+};
+
+static bool arm_ni_val_count_event(struct perf_event *evt, struct arm_ni_val *val)
+{
+	if (is_software_event(evt))
+		return true;
+
+	if (NI_EVENT_TYPE(evt) == NI_PMU) {
+		val->ccnt++;
+		return val->ccnt <= 1;
+	}
+
+	val->evcnt++;
+	return val->evcnt <= NI_NUM_COUNTERS;
+}
+
+static int arm_ni_validate_group(struct perf_event *event)
+{
+	struct perf_event *sibling, *leader = event->group_leader;
+	struct arm_ni_val val = { 0 };
+
+	if (leader == event)
+		return 0;
+
+	arm_ni_val_count_event(event, &val);
+	if (!arm_ni_val_count_event(leader, &val))
+		return -EINVAL;
+
+	for_each_sibling_event(sibling, leader) {
+		if (!arm_ni_val_count_event(sibling, &val))
+			return -EINVAL;
+	}
+	return 0;
+}
+
+static int arm_ni_event_init(struct perf_event *event)
+{
+	struct arm_ni_cd *cd = pmu_to_cd(event->pmu);
+
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;
+
+	if (is_sampling_event(event))
+		return -EINVAL;
+
+	event->cpu = cd->cpu;
+	if (NI_EVENT_TYPE(event) == NI_PMU)
+		return arm_ni_validate_group(event);
+
+	cd_for_each_unit(cd, unit) {
+		if (unit->type == NI_EVENT_TYPE(event) &&
+		    unit->id == NI_EVENT_NODEID(event) && unit->ns) {
+			event->hw.config_base = (unsigned long)unit;
+			return arm_ni_validate_group(event);
+		}
+	}
+	return -EINVAL;
+}
+
+static u64 arm_ni_read_ccnt(struct arm_ni_cd *cd)
+{
+	u64 l, u_old, u_new;
+	int retries = 3; /* 1st time unlucky, 2nd improbable, 3rd just broken */
+
+	u_new = readl_relaxed(cd->pmu_base + NI_PMCCNTR_U);
+	do {
+		u_old = u_new;
+		l = readl_relaxed(cd->pmu_base + NI_PMCCNTR_L);
+		u_new = readl_relaxed(cd->pmu_base + NI_PMCCNTR_U);
+	} while (u_new != u_old && --retries);
+	WARN_ON(!retries);
+
+	return (u_new << 32) | l;
+}
+
+static void arm_ni_event_read(struct perf_event *event)
+{
+	struct arm_ni_cd *cd = pmu_to_cd(event->pmu);
+	struct hw_perf_event *hw = &event->hw;
+	u64 count, prev;
+	bool ccnt = hw->idx == NI_CCNT_IDX;
+
+	do {
+		prev = local64_read(&hw->prev_count);
+		if (ccnt)
+			count = arm_ni_read_ccnt(cd);
+		else
+			count = readl_relaxed(cd->pmu_base + NI_PMEVCNTR(hw->idx));
+	} while (local64_cmpxchg(&hw->prev_count, prev, count) != prev);
+
+	count -= prev;
+	if (!ccnt)
+		count = (u32)count;
+	local64_add(count, &event->count);
+}
+
+static void arm_ni_event_start(struct perf_event *event, int flags)
+{
+	struct arm_ni_cd *cd = pmu_to_cd(event->pmu);
+
+	writel_relaxed(1U << event->hw.idx, cd->pmu_base + NI_PMCNTENSET);
+}
+
+static void arm_ni_event_stop(struct perf_event *event, int flags)
+{
+	struct arm_ni_cd *cd = pmu_to_cd(event->pmu);
+
+	writel_relaxed(1U << event->hw.idx, cd->pmu_base + NI_PMCNTENCLR);
+	if (flags & PERF_EF_UPDATE)
+		arm_ni_event_read(event);
+}
+
+static void arm_ni_init_ccnt(struct arm_ni_cd *cd)
+{
+	local64_set(&cd->ccnt->hw.prev_count, S64_MIN);
+	lo_hi_writeq_relaxed(S64_MIN, cd->pmu_base + NI_PMCCNTR_L);
+}
+
+static void arm_ni_init_evcnt(struct arm_ni_cd *cd, int idx)
+{
+	local64_set(&cd->evcnt[idx]->hw.prev_count, S32_MIN);
+	writel_relaxed(S32_MIN, cd->pmu_base + NI_PMEVCNTR(idx));
+}
+
+static int arm_ni_event_add(struct perf_event *event, int flags)
+{
+	struct arm_ni_cd *cd = pmu_to_cd(event->pmu);
+	struct hw_perf_event *hw = &event->hw;
+	struct arm_ni_unit *unit;
+	enum ni_node_type type = NI_EVENT_TYPE(event);
+	u32 reg;
+
+	if (type == NI_PMU) {
+		if (cd->ccnt)
+			return -ENOSPC;
+		hw->idx = NI_CCNT_IDX;
+		cd->ccnt = event;
+		arm_ni_init_ccnt(cd);
+	} else {
+		hw->idx = 0;
+		while (cd->evcnt[hw->idx]) {
+			if (++hw->idx == NI_NUM_COUNTERS)
+				return -ENOSPC;
+		}
+		cd->evcnt[hw->idx] = event;
+		unit = (void *)hw->config_base;
+		unit->event[hw->idx] = NI_EVENT_EVENTID(event);
+		arm_ni_init_evcnt(cd, hw->idx);
+		lo_hi_writeq_relaxed(le64_to_cpu(unit->pmusel), unit->pmusela);
+
+		reg = FIELD_PREP(NI_PMEVTYPER_NODE_TYPE, type) |
+		      FIELD_PREP(NI_PMEVTYPER_NODE_ID, NI_EVENT_NODEID(event));
+		writel_relaxed(reg, cd->pmu_base + NI_PMEVTYPER(hw->idx));
+	}
+	if (flags & PERF_EF_START)
+		arm_ni_event_start(event, 0);
+	return 0;
+}
+
+static void arm_ni_event_del(struct perf_event *event, int flags)
+{
+	struct arm_ni_cd *cd = pmu_to_cd(event->pmu);
+	struct hw_perf_event *hw = &event->hw;
+
+	arm_ni_event_stop(event, PERF_EF_UPDATE);
+
+	if (hw->idx == NI_CCNT_IDX)
+		cd->ccnt = NULL;
+	else
+		cd->evcnt[hw->idx] = NULL;
+}
+
+static irqreturn_t arm_ni_handle_irq(int irq, void *dev_id)
+{
+	struct arm_ni_cd *cd = dev_id;
+	irqreturn_t ret = IRQ_NONE;
+	u32 reg = readl_relaxed(cd->pmu_base + NI_PMOVSCLR);
+
+	if (reg & (1U << NI_CCNT_IDX)) {
+		ret = IRQ_HANDLED;
+		if (!(WARN_ON(!cd->ccnt))) {
+			arm_ni_event_read(cd->ccnt);
+			arm_ni_init_ccnt(cd);
+		}
+	}
+	for (int i = 0; i < NI_NUM_COUNTERS; i++) {
+		if (!(reg & (1U << i)))
+			continue;
+		ret = IRQ_HANDLED;
+		if (!(WARN_ON(!cd->evcnt[i]))) {
+			arm_ni_event_read(cd->evcnt[i]);
+			arm_ni_init_evcnt(cd, i);
+		}
+	}
+	writel_relaxed(reg, cd->pmu_base + NI_PMOVSCLR);
+	return ret;
+}
+
+static int arm_ni_init_cd(struct arm_ni *ni, struct arm_ni_node *node, u64 res_start)
+{
+	struct arm_ni_cd *cd = ni->cds + node->id;
+	const char *name;
+	int err;
+
+	cd->id = node->id;
+	cd->num_units = node->num_components;
+	cd->units = devm_kcalloc(ni->dev, cd->num_units, sizeof(*(cd->units)), GFP_KERNEL);
+	if (!cd->units)
+		return -ENOMEM;
+
+	for (int i = 0; i < cd->num_units; i++) {
+		u32 reg = readl_relaxed(node->base + NI_CHILD_PTR(i));
+		void __iomem *unit_base = ni->base + reg;
+		struct arm_ni_unit *unit = cd->units + i;
+
+		reg = readl_relaxed(unit_base + NI_NODE_TYPE);
+		unit->type = FIELD_GET(NI_NODE_TYPE_NODE_TYPE, reg);
+		unit->id = FIELD_GET(NI_NODE_TYPE_NODE_ID, reg);
+
+		switch (unit->type) {
+		case NI_PMU:
+			reg = readl_relaxed(unit_base + NI_PMCFGR);
+			if (!reg) {
+				dev_info(ni->dev, "No access to PMU %d\n", cd->id);
+				devm_kfree(ni->dev, cd->units);
+				return 0;
+			}
+			unit->ns = true;
+			cd->pmu_base = unit_base;
+			break;
+		case NI_ASNI:
+		case NI_AMNI:
+		case NI_HSNI:
+		case NI_HMNI:
+		case NI_PMNI:
+			unit->pmusela = unit_base + NI700_PMUSELA;
+			writel_relaxed(1, unit->pmusela);
+			if (readl_relaxed(unit->pmusela) != 1)
+				dev_info(ni->dev, "No access to node 0x%04x%04x\n", unit->id, unit->type);
+			else
+				unit->ns = true;
+			break;
+		default:
+			/*
+			 * e.g. FMU - thankfully bits 3:2 of FMU_ERR_FR0 are RES0 so
+			 * can't alias any of the leaf node types we're looking for.
+			 */
+			dev_dbg(ni->dev, "Mystery node 0x%04x%04x\n", unit->id, unit->type);
+			break;
+		}
+	}
+
+	res_start += cd->pmu_base - ni->base;
+	if (!devm_request_mem_region(ni->dev, res_start, SZ_4K, dev_name(ni->dev))) {
+		dev_err(ni->dev, "Failed to request PMU region 0x%llx\n", res_start);
+		return -EBUSY;
+	}
+
+	writel_relaxed(NI_PMCR_RESET_CCNT | NI_PMCR_RESET_EVCNT,
+		       cd->pmu_base + NI_PMCR);
+	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMCNTENCLR);
+	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMOVSCLR);
+	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMINTENSET);
+
+	cd->irq = platform_get_irq(to_platform_device(ni->dev), cd->id);
+	if (cd->irq < 0)
+		return cd->irq;
+
+	err = devm_request_irq(ni->dev, cd->irq, arm_ni_handle_irq,
+			       IRQF_NOBALANCING | IRQF_NO_THREAD,
+			       dev_name(ni->dev), cd);
+	if (err)
+		return err;
+
+	cd->cpu = cpumask_local_spread(0, dev_to_node(ni->dev));
+	cd->pmu = (struct pmu) {
+		.module = THIS_MODULE,
+		.parent = ni->dev,
+		.attr_groups = arm_ni_attr_groups,
+		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
+		.task_ctx_nr = perf_invalid_context,
+		.pmu_enable = arm_ni_pmu_enable,
+		.pmu_disable = arm_ni_pmu_disable,
+		.event_init = arm_ni_event_init,
+		.add = arm_ni_event_add,
+		.del = arm_ni_event_del,
+		.start = arm_ni_event_start,
+		.stop = arm_ni_event_stop,
+		.read = arm_ni_event_read,
+	};
+
+	name = devm_kasprintf(ni->dev, GFP_KERNEL, "arm_ni_%d_cd_%d", ni->id, cd->id);
+	if (!name)
+		return -ENOMEM;
+
+	err = cpuhp_state_add_instance(arm_ni_hp_state, &cd->cpuhp_node);
+	if (err)
+		return err;
+
+	return perf_pmu_register(&cd->pmu, name, -1);
+}
+
+static void arm_ni_probe_domain(void __iomem *base, struct arm_ni_node *node)
+{
+	u32 reg = readl_relaxed(base + NI_NODE_TYPE);
+
+	node->base = base;
+	node->type = FIELD_GET(NI_NODE_TYPE_NODE_TYPE, reg);
+	node->id = FIELD_GET(NI_NODE_TYPE_NODE_ID, reg);
+	node->num_components = readl_relaxed(base + NI_CHILD_NODE_INFO);
+}
+
+static int arm_ni_probe(struct platform_device *pdev)
+{
+	struct arm_ni_node cfg, vd, pd, cd;
+	struct arm_ni *ni;
+	struct resource *res;
+	void __iomem *base;
+	static atomic_t id;
+	int num_cds;
+	u32 reg, part;
+
+	/*
+	 * We want to map the whole configuration space for ease of discovery,
+	 * but the PMU pages are the only ones for which we can honestly claim
+	 * exclusive ownership, so we'll request them explicitly once found.
+	 */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	arm_ni_probe_domain(base, &cfg);
+	if (cfg.type != NI_GLOBAL)
+		return -ENODEV;
+
+	reg = readl_relaxed(cfg.base + NI_PERIPHERAL_ID0);
+	part = FIELD_GET(NI_PIDR0_PART_7_0, reg);
+	reg = readl_relaxed(cfg.base + NI_PERIPHERAL_ID1);
+	part |= FIELD_GET(NI_PIDR1_PART_11_8, reg) << 8;
+
+	switch (part) {
+	case PART_NI_700:
+	case PART_NI_710AE:
+		break;
+	default:
+		dev_WARN(&pdev->dev, "Unknown part number: 0x%03x, this may go badly\n", part);
+		break;
+	}
+
+	num_cds = 0;
+	for (int v = 0; v < cfg.num_components; v++) {
+		reg = readl_relaxed(cfg.base + NI_CHILD_PTR(v));
+		arm_ni_probe_domain(base + reg, &vd);
+		for (int p = 0; p < vd.num_components; p++) {
+			reg = readl_relaxed(vd.base + NI_CHILD_PTR(p));
+			arm_ni_probe_domain(base + reg, &pd);
+			num_cds += pd.num_components;
+		}
+	}
+
+	ni = devm_kzalloc(&pdev->dev, struct_size(ni, cds, num_cds), GFP_KERNEL);
+	if (!ni)
+		return -ENOMEM;
+
+	ni->dev = &pdev->dev;
+	ni->base = base;
+	ni->num_cds = num_cds;
+	ni->part = part;
+	ni->id = atomic_fetch_inc(&id);
+
+	for (int v = 0; v < cfg.num_components; v++) {
+		reg = readl_relaxed(cfg.base + NI_CHILD_PTR(v));
+		arm_ni_probe_domain(base + reg, &vd);
+		for (int p = 0; p < vd.num_components; p++) {
+			reg = readl_relaxed(vd.base + NI_CHILD_PTR(p));
+			arm_ni_probe_domain(base + reg, &pd);
+			for (int c = 0; c < pd.num_components; c++) {
+				int ret;
+
+				reg = readl_relaxed(pd.base + NI_CHILD_PTR(c));
+				arm_ni_probe_domain(base + reg, &cd);
+				ret = arm_ni_init_cd(ni, &cd, res->start);
+				if (ret)
+					return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static void arm_ni_remove(struct platform_device *pdev)
+{
+	struct arm_ni *ni = platform_get_drvdata(pdev);
+
+	for (int i = 0; i < ni->num_cds; i++) {
+		struct arm_ni_cd *cd = ni->cds + i;
+
+		if (!cd->pmu_base)
+			continue;
+
+		writel_relaxed(0, cd->pmu_base + NI_PMCR);
+		writel_relaxed(U32_MAX, cd->pmu_base + NI_PMINTENCLR);
+		perf_pmu_unregister(&cd->pmu);
+		cpuhp_state_remove_instance_nocalls(arm_ni_hp_state, &cd->cpuhp_node);
+	}
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id arm_ni_of_match[] = {
+	{ .compatible = "arm,ni-700" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, arm_ni_of_match);
+#endif
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id arm_ni_acpi_match[] = {
+	{ "ARMHCB70" },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, arm_ni_acpi_match);
+#endif
+
+static struct platform_driver arm_ni_driver = {
+	.driver = {
+		.name = "arm-ni",
+		.of_match_table = of_match_ptr(arm_ni_of_match),
+		.acpi_match_table = ACPI_PTR(arm_ni_acpi_match),
+	},
+	.probe = arm_ni_probe,
+	.remove = arm_ni_remove,
+};
+
+static void arm_ni_pmu_migrate(struct arm_ni_cd *cd, unsigned int cpu)
+{
+	perf_pmu_migrate_context(&cd->pmu, cd->cpu, cpu);
+	irq_set_affinity(cd->irq, cpumask_of(cpu));
+	cd->cpu = cpu;
+}
+
+static int arm_ni_pmu_online_cpu(unsigned int cpu, struct hlist_node *cpuhp_node)
+{
+	struct arm_ni_cd *cd;
+	int node;
+
+	cd = hlist_entry_safe(cpuhp_node, struct arm_ni_cd, cpuhp_node);
+	node = dev_to_node(cd_to_ni(cd)->dev);
+	if (cpu_to_node(cd->cpu) != node && cpu_to_node(cpu) == node)
+		arm_ni_pmu_migrate(cd, cpu);
+	return 0;
+}
+
+static int arm_ni_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_node)
+{
+	struct arm_ni_cd *cd;
+	unsigned int target;
+	int node;
+
+	cd = hlist_entry_safe(cpuhp_node, struct arm_ni_cd, cpuhp_node);
+	if (cpu != cd->cpu)
+		return 0;
+
+	node = dev_to_node(cd_to_ni(cd)->dev);
+	target = cpumask_any_and_but(cpumask_of_node(node), cpu_online_mask, cpu);
+	if (target >= nr_cpu_ids)
+		target = cpumask_any_but(cpu_online_mask, cpu);
+
+	if (target < nr_cpu_ids)
+		arm_ni_pmu_migrate(cd, target);
+	return 0;
+}
+
+static int __init arm_ni_init(void)
+{
+	int ret;
+
+	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
+				      "perf/arm/ni:online",
+				      arm_ni_pmu_online_cpu,
+				      arm_ni_pmu_offline_cpu);
+	if (ret < 0)
+		return ret;
+
+	arm_ni_hp_state = ret;
+
+	ret = platform_driver_register(&arm_ni_driver);
+	if (ret)
+		cpuhp_remove_multi_state(arm_ni_hp_state);
+	return ret;
+}
+
+static void __exit arm_ni_exit(void)
+{
+	platform_driver_unregister(&arm_ni_driver);
+	cpuhp_remove_multi_state(arm_ni_hp_state);
+}
+
+module_init(arm_ni_init);
+module_exit(arm_ni_exit);
+
+MODULE_AUTHOR("Robin Murphy <robin.murphy@arm.com>");
+MODULE_DESCRIPTION("Arm NI-700 PMU driver");
+MODULE_LICENSE("GPL v2");
-- 
2.39.2.101.g768bb238c484.dirty

Re: [PATCH v3 2/3] perf: Add driver for Arm NI-700 interconnect PMU

[Will Deacon]

Hi Robin,

On Fri, Aug 30, 2024 at 06:19:34PM +0100, Robin Murphy wrote:
> The Arm NI-700 Network-on-Chip Interconnect has a relatively
> straightforward design with a hierarchy of voltage, power, and clock
> domains, where each clock domain then contains a number of interface
> units and a PMU which can monitor events thereon. As such, it begets a
> relatively straightforward driver to interface those PMUs with perf.
> 
> Even more so than with arm-cmn, users will require detailed knowledge of
> the wider system topology in order to meaningfully analyse anything,
> since the interconnect itself cannot know what lies beyond the boundary
> of each inscrutably-numbered interface. Given that, for now they are
> also expected to refer to the NI-700 documentation for the relevant
> event IDs to provide as well. An identifier is implemented so we can
> come back and add jevents if anyone really wants to.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> 
> ---
> v2:
>  - Add basic usage documentation
>  - Use __counted_by attribute
>  - Make group validation logic clearer (and drop PMU type check
>    which perf_event_open() already takes care of)
>  - Add retry limit to arm_ni_read_ccnt()
> v3:
>  - Update .remove to return void
>  - Fix group leader validation and make the naming clearer
>  - Drop NUMA_NO_NODE check for CPU online (the only way that could
>    actually pass both other migration conditions is if the NUMA info
>    is so messed up that it's not worth worrying about anyway)

Thanks, this is looking pretty good now. I just have a few random comments
based on another read-through of the code.

> diff --git a/Documentation/admin-guide/perf/arm-ni.rst b/Documentation/admin-guide/perf/arm-ni.rst
> new file mode 100644
> index 000000000000..3cd7d0f75f0f
> --- /dev/null
> +++ b/Documentation/admin-guide/perf/arm-ni.rst
> @@ -0,0 +1,17 @@
> +====================================
> +Arm Network-on Chip Interconnect PMU
> +====================================
> +
> +NI-700 and friends implement a distinct PMU for each clock domain within the
> +interconnect. Correspondingly, the driver exposes multiple PMU devices named
> +arm_ni_<x>_cd_<y>, where <x> is an (abritrary) instance identifier and <y> is

typo: abritrary

> +the clock domain ID within that particular instance. If multiple NI instances
> +exist within a system, the PMU devices can be correlated with the underlying
> +hardware instance via sysfs parentage.
> +
> +Each PMU exposes base event aliases for the interface types present in its clock
> +domain. These require qualifying with the "eventid" and "nodeid" parameters
> +to specify the event code to count and the interface at which to count it
> +(per the configured hardware ID as reflected in the xxNI_NODE_INFO register).
> +The exception is the "cycles" alias for the PMU cycle counter, which is encoded
> +with the PMU node type and needs no further qualification.

[...]

> +static ssize_t arm_ni_format_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct arm_ni_format_attr *fmt = container_of(attr, typeof(*fmt), attr);
> +	int lo = __ffs(fmt->field), hi = __fls(fmt->field);
> +
> +	return sysfs_emit(buf, "config:%d-%d\n", lo, hi);
> +}

Nit: if you end up adding single-bit config fields in the future, this
will quietly do the wrong thing. Maybe safe-guard the 'lo==hi' case (even
if you just warn once and return without doing anything).


[...]

> +static int arm_ni_init_cd(struct arm_ni *ni, struct arm_ni_node *node, u64 res_start)
> +{
> +	struct arm_ni_cd *cd = ni->cds + node->id;
> +	const char *name;
> +	int err;
> +
> +	cd->id = node->id;
> +	cd->num_units = node->num_components;
> +	cd->units = devm_kcalloc(ni->dev, cd->num_units, sizeof(*(cd->units)), GFP_KERNEL);
> +	if (!cd->units)
> +		return -ENOMEM;
> +
> +	for (int i = 0; i < cd->num_units; i++) {
> +		u32 reg = readl_relaxed(node->base + NI_CHILD_PTR(i));
> +		void __iomem *unit_base = ni->base + reg;
> +		struct arm_ni_unit *unit = cd->units + i;
> +
> +		reg = readl_relaxed(unit_base + NI_NODE_TYPE);
> +		unit->type = FIELD_GET(NI_NODE_TYPE_NODE_TYPE, reg);
> +		unit->id = FIELD_GET(NI_NODE_TYPE_NODE_ID, reg);
> +
> +		switch (unit->type) {
> +		case NI_PMU:
> +			reg = readl_relaxed(unit_base + NI_PMCFGR);
> +			if (!reg) {
> +				dev_info(ni->dev, "No access to PMU %d\n", cd->id);
> +				devm_kfree(ni->dev, cd->units);
> +				return 0;
> +			}
> +			unit->ns = true;
> +			cd->pmu_base = unit_base;
> +			break;
> +		case NI_ASNI:
> +		case NI_AMNI:
> +		case NI_HSNI:
> +		case NI_HMNI:
> +		case NI_PMNI:
> +			unit->pmusela = unit_base + NI700_PMUSELA;
> +			writel_relaxed(1, unit->pmusela);
> +			if (readl_relaxed(unit->pmusela) != 1)
> +				dev_info(ni->dev, "No access to node 0x%04x%04x\n", unit->id, unit->type);
> +			else
> +				unit->ns = true;
> +			break;
> +		default:
> +			/*
> +			 * e.g. FMU - thankfully bits 3:2 of FMU_ERR_FR0 are RES0 so
> +			 * can't alias any of the leaf node types we're looking for.
> +			 */
> +			dev_dbg(ni->dev, "Mystery node 0x%04x%04x\n", unit->id, unit->type);
> +			break;
> +		}
> +	}
> +
> +	res_start += cd->pmu_base - ni->base;
> +	if (!devm_request_mem_region(ni->dev, res_start, SZ_4K, dev_name(ni->dev))) {
> +		dev_err(ni->dev, "Failed to request PMU region 0x%llx\n", res_start);
> +		return -EBUSY;
> +	}
> +
> +	writel_relaxed(NI_PMCR_RESET_CCNT | NI_PMCR_RESET_EVCNT,
> +		       cd->pmu_base + NI_PMCR);
> +	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMCNTENCLR);
> +	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMOVSCLR);
> +	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMINTENSET);
> +
> +	cd->irq = platform_get_irq(to_platform_device(ni->dev), cd->id);
> +	if (cd->irq < 0)
> +		return cd->irq;
> +
> +	err = devm_request_irq(ni->dev, cd->irq, arm_ni_handle_irq,
> +			       IRQF_NOBALANCING | IRQF_NO_THREAD,
> +			       dev_name(ni->dev), cd);
> +	if (err)
> +		return err;
> +
> +	cd->cpu = cpumask_local_spread(0, dev_to_node(ni->dev));
> +	cd->pmu = (struct pmu) {
> +		.module = THIS_MODULE,
> +		.parent = ni->dev,
> +		.attr_groups = arm_ni_attr_groups,
> +		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
> +		.task_ctx_nr = perf_invalid_context,
> +		.pmu_enable = arm_ni_pmu_enable,
> +		.pmu_disable = arm_ni_pmu_disable,
> +		.event_init = arm_ni_event_init,
> +		.add = arm_ni_event_add,
> +		.del = arm_ni_event_del,
> +		.start = arm_ni_event_start,
> +		.stop = arm_ni_event_stop,
> +		.read = arm_ni_event_read,
> +	};
> +
> +	name = devm_kasprintf(ni->dev, GFP_KERNEL, "arm_ni_%d_cd_%d", ni->id, cd->id);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	err = cpuhp_state_add_instance(arm_ni_hp_state, &cd->cpuhp_node);
> +	if (err)
> +		return err;

What happens if there's a CPU hotplug operation here? Can we end up calling
perf_pmu_migrate_context() concurrently with perf_pmu_register()?

> +	return perf_pmu_register(&cd->pmu, name, -1);

Clean up the cpuhp instance if this fails?

> +static void arm_ni_remove(struct platform_device *pdev)
> +{
> +	struct arm_ni *ni = platform_get_drvdata(pdev);
> +
> +	for (int i = 0; i < ni->num_cds; i++) {
> +		struct arm_ni_cd *cd = ni->cds + i;
> +
> +		if (!cd->pmu_base)
> +			continue;
> +
> +		writel_relaxed(0, cd->pmu_base + NI_PMCR);
> +		writel_relaxed(U32_MAX, cd->pmu_base + NI_PMINTENCLR);
> +		perf_pmu_unregister(&cd->pmu);

Similarly here, it feels like a CPU hotplug operation could cause problems
here.

> +		cpuhp_state_remove_instance_nocalls(arm_ni_hp_state, &cd->cpuhp_node);
> +	}
> +}

Will

Re: [PATCH v3 2/3] perf: Add driver for Arm NI-700 interconnect PMU

[Robin Murphy]

On 02/09/2024 3:47 pm, Will Deacon wrote:
> Hi Robin,
> 
> On Fri, Aug 30, 2024 at 06:19:34PM +0100, Robin Murphy wrote:
>> The Arm NI-700 Network-on-Chip Interconnect has a relatively
>> straightforward design with a hierarchy of voltage, power, and clock
>> domains, where each clock domain then contains a number of interface
>> units and a PMU which can monitor events thereon. As such, it begets a
>> relatively straightforward driver to interface those PMUs with perf.
>>
>> Even more so than with arm-cmn, users will require detailed knowledge of
>> the wider system topology in order to meaningfully analyse anything,
>> since the interconnect itself cannot know what lies beyond the boundary
>> of each inscrutably-numbered interface. Given that, for now they are
>> also expected to refer to the NI-700 documentation for the relevant
>> event IDs to provide as well. An identifier is implemented so we can
>> come back and add jevents if anyone really wants to.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>
>> ---
>> v2:
>>   - Add basic usage documentation
>>   - Use __counted_by attribute
>>   - Make group validation logic clearer (and drop PMU type check
>>     which perf_event_open() already takes care of)
>>   - Add retry limit to arm_ni_read_ccnt()
>> v3:
>>   - Update .remove to return void
>>   - Fix group leader validation and make the naming clearer
>>   - Drop NUMA_NO_NODE check for CPU online (the only way that could
>>     actually pass both other migration conditions is if the NUMA info
>>     is so messed up that it's not worth worrying about anyway)
> 
> Thanks, this is looking pretty good now. I just have a few random comments
> based on another read-through of the code.
> 
>> diff --git a/Documentation/admin-guide/perf/arm-ni.rst b/Documentation/admin-guide/perf/arm-ni.rst
>> new file mode 100644
>> index 000000000000..3cd7d0f75f0f
>> --- /dev/null
>> +++ b/Documentation/admin-guide/perf/arm-ni.rst
>> @@ -0,0 +1,17 @@
>> +====================================
>> +Arm Network-on Chip Interconnect PMU
>> +====================================
>> +
>> +NI-700 and friends implement a distinct PMU for each clock domain within the
>> +interconnect. Correspondingly, the driver exposes multiple PMU devices named
>> +arm_ni_<x>_cd_<y>, where <x> is an (abritrary) instance identifier and <y> is
> 
> typo: abritrary

Oops, fixed.

>> +the clock domain ID within that particular instance. If multiple NI instances
>> +exist within a system, the PMU devices can be correlated with the underlying
>> +hardware instance via sysfs parentage.
>> +
>> +Each PMU exposes base event aliases for the interface types present in its clock
>> +domain. These require qualifying with the "eventid" and "nodeid" parameters
>> +to specify the event code to count and the interface at which to count it
>> +(per the configured hardware ID as reflected in the xxNI_NODE_INFO register).
>> +The exception is the "cycles" alias for the PMU cycle counter, which is encoded
>> +with the PMU node type and needs no further qualification.
> 
> [...]
> 
>> +static ssize_t arm_ni_format_show(struct device *dev,
>> +				  struct device_attribute *attr, char *buf)
>> +{
>> +	struct arm_ni_format_attr *fmt = container_of(attr, typeof(*fmt), attr);
>> +	int lo = __ffs(fmt->field), hi = __fls(fmt->field);
>> +
>> +	return sysfs_emit(buf, "config:%d-%d\n", lo, hi);
>> +}
> 
> Nit: if you end up adding single-bit config fields in the future, this
> will quietly do the wrong thing. Maybe safe-guard the 'lo==hi' case (even
> if you just warn once and return without doing anything).

The counter-argument is that I don't foresee having any reason to add 
single-bit config fields here in future, nor indeed config1 or config2 
fields, so I intentionally pruned the would-be dead code while 
copy-pasting this implementation from arm-cmn. Yes, if someone were to 
make an incomplete change without paying attention or testing they could 
introduce a bug, but when is that ever not true?

TBH I was originally hoping to have much of this boilerplate stuff 
factored out into a common library beforehand (I also anticipate needing 
the IRQ-sharing shenanigans here), but as ever, priorities got in the way...

> 
> 
> [...]
> 
>> +static int arm_ni_init_cd(struct arm_ni *ni, struct arm_ni_node *node, u64 res_start)
>> +{
>> +	struct arm_ni_cd *cd = ni->cds + node->id;
>> +	const char *name;
>> +	int err;
>> +
>> +	cd->id = node->id;
>> +	cd->num_units = node->num_components;
>> +	cd->units = devm_kcalloc(ni->dev, cd->num_units, sizeof(*(cd->units)), GFP_KERNEL);
>> +	if (!cd->units)
>> +		return -ENOMEM;
>> +
>> +	for (int i = 0; i < cd->num_units; i++) {
>> +		u32 reg = readl_relaxed(node->base + NI_CHILD_PTR(i));
>> +		void __iomem *unit_base = ni->base + reg;
>> +		struct arm_ni_unit *unit = cd->units + i;
>> +
>> +		reg = readl_relaxed(unit_base + NI_NODE_TYPE);
>> +		unit->type = FIELD_GET(NI_NODE_TYPE_NODE_TYPE, reg);
>> +		unit->id = FIELD_GET(NI_NODE_TYPE_NODE_ID, reg);
>> +
>> +		switch (unit->type) {
>> +		case NI_PMU:
>> +			reg = readl_relaxed(unit_base + NI_PMCFGR);
>> +			if (!reg) {
>> +				dev_info(ni->dev, "No access to PMU %d\n", cd->id);
>> +				devm_kfree(ni->dev, cd->units);
>> +				return 0;
>> +			}
>> +			unit->ns = true;
>> +			cd->pmu_base = unit_base;
>> +			break;
>> +		case NI_ASNI:
>> +		case NI_AMNI:
>> +		case NI_HSNI:
>> +		case NI_HMNI:
>> +		case NI_PMNI:
>> +			unit->pmusela = unit_base + NI700_PMUSELA;
>> +			writel_relaxed(1, unit->pmusela);
>> +			if (readl_relaxed(unit->pmusela) != 1)
>> +				dev_info(ni->dev, "No access to node 0x%04x%04x\n", unit->id, unit->type);
>> +			else
>> +				unit->ns = true;
>> +			break;
>> +		default:
>> +			/*
>> +			 * e.g. FMU - thankfully bits 3:2 of FMU_ERR_FR0 are RES0 so
>> +			 * can't alias any of the leaf node types we're looking for.
>> +			 */
>> +			dev_dbg(ni->dev, "Mystery node 0x%04x%04x\n", unit->id, unit->type);
>> +			break;
>> +		}
>> +	}
>> +
>> +	res_start += cd->pmu_base - ni->base;
>> +	if (!devm_request_mem_region(ni->dev, res_start, SZ_4K, dev_name(ni->dev))) {
>> +		dev_err(ni->dev, "Failed to request PMU region 0x%llx\n", res_start);
>> +		return -EBUSY;
>> +	}
>> +
>> +	writel_relaxed(NI_PMCR_RESET_CCNT | NI_PMCR_RESET_EVCNT,
>> +		       cd->pmu_base + NI_PMCR);
>> +	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMCNTENCLR);
>> +	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMOVSCLR);
>> +	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMINTENSET);
>> +
>> +	cd->irq = platform_get_irq(to_platform_device(ni->dev), cd->id);
>> +	if (cd->irq < 0)
>> +		return cd->irq;
>> +
>> +	err = devm_request_irq(ni->dev, cd->irq, arm_ni_handle_irq,
>> +			       IRQF_NOBALANCING | IRQF_NO_THREAD,
>> +			       dev_name(ni->dev), cd);
>> +	if (err)
>> +		return err;
>> +
>> +	cd->cpu = cpumask_local_spread(0, dev_to_node(ni->dev));
>> +	cd->pmu = (struct pmu) {
>> +		.module = THIS_MODULE,
>> +		.parent = ni->dev,
>> +		.attr_groups = arm_ni_attr_groups,
>> +		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
>> +		.task_ctx_nr = perf_invalid_context,
>> +		.pmu_enable = arm_ni_pmu_enable,
>> +		.pmu_disable = arm_ni_pmu_disable,
>> +		.event_init = arm_ni_event_init,
>> +		.add = arm_ni_event_add,
>> +		.del = arm_ni_event_del,
>> +		.start = arm_ni_event_start,
>> +		.stop = arm_ni_event_stop,
>> +		.read = arm_ni_event_read,
>> +	};
>> +
>> +	name = devm_kasprintf(ni->dev, GFP_KERNEL, "arm_ni_%d_cd_%d", ni->id, cd->id);
>> +	if (!name)
>> +		return -ENOMEM;
>> +
>> +	err = cpuhp_state_add_instance(arm_ni_hp_state, &cd->cpuhp_node);
>> +	if (err)
>> +		return err;
> 
> What happens if there's a CPU hotplug operation here? Can we end up calling
> perf_pmu_migrate_context() concurrently with perf_pmu_register()?

Yes. Alternatively we could register the PMU before the hotplug handler, 
then potentially miss a hotplug event and leave a user-visible PMU 
associated with an invalid CPU. This is a known issue for all system PMU 
drivers, and the conclusion 5 years ago was that it's impractical to 
close this race from outside perf core itself[1][2].

>> +	return perf_pmu_register(&cd->pmu, name, -1);
> 
> Clean up the cpuhp instance if this fails?

Indeed, not sure how that managed to escape...

Thanks,
Robin.

[1] 
https://lore.kernel.org/linux-arm-kernel/cover.1549299188.git.robin.murphy@arm.com/
[2] 
https://lore.kernel.org/linux-arm-kernel/cover.1554310292.git.robin.murphy@arm.com/

>> +static void arm_ni_remove(struct platform_device *pdev)
>> +{
>> +	struct arm_ni *ni = platform_get_drvdata(pdev);
>> +
>> +	for (int i = 0; i < ni->num_cds; i++) {
>> +		struct arm_ni_cd *cd = ni->cds + i;
>> +
>> +		if (!cd->pmu_base)
>> +			continue;
>> +
>> +		writel_relaxed(0, cd->pmu_base + NI_PMCR);
>> +		writel_relaxed(U32_MAX, cd->pmu_base + NI_PMINTENCLR);
>> +		perf_pmu_unregister(&cd->pmu);
> 
> Similarly here, it feels like a CPU hotplug operation could cause problems
> here.
> 
>> +		cpuhp_state_remove_instance_nocalls(arm_ni_hp_state, &cd->cpuhp_node);
>> +	}
>> +}
> 
> Will

Re: [PATCH v3 2/3] perf: Add driver for Arm NI-700 interconnect PMU

[Will Deacon]

On Mon, Sep 02, 2024 at 07:47:18PM +0100, Robin Murphy wrote:
> On 02/09/2024 3:47 pm, Will Deacon wrote:
> > > +static ssize_t arm_ni_format_show(struct device *dev,
> > > +				  struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct arm_ni_format_attr *fmt = container_of(attr, typeof(*fmt), attr);
> > > +	int lo = __ffs(fmt->field), hi = __fls(fmt->field);
> > > +
> > > +	return sysfs_emit(buf, "config:%d-%d\n", lo, hi);
> > > +}
> > 
> > Nit: if you end up adding single-bit config fields in the future, this
> > will quietly do the wrong thing. Maybe safe-guard the 'lo==hi' case (even
> > if you just warn once and return without doing anything).
> 
> The counter-argument is that I don't foresee having any reason to add
> single-bit config fields here in future, nor indeed config1 or config2
> fields, so I intentionally pruned the would-be dead code while copy-pasting
> this implementation from arm-cmn. Yes, if someone were to make an incomplete
> change without paying attention or testing they could introduce a bug, but
> when is that ever not true?

I guess I'm just a little more wary when it comes to UAPI. Somebody starts
relying on the broken message and then you're toast. It's also incredibly
easy to avoid by construction and the dead code isn't hurting anybody.

> > > +	name = devm_kasprintf(ni->dev, GFP_KERNEL, "arm_ni_%d_cd_%d", ni->id, cd->id);
> > > +	if (!name)
> > > +		return -ENOMEM;
> > > +
> > > +	err = cpuhp_state_add_instance(arm_ni_hp_state, &cd->cpuhp_node);
> > > +	if (err)
> > > +		return err;
> > 
> > What happens if there's a CPU hotplug operation here? Can we end up calling
> > perf_pmu_migrate_context() concurrently with perf_pmu_register()?
> 
> Yes. Alternatively we could register the PMU before the hotplug handler,
> then potentially miss a hotplug event and leave a user-visible PMU
> associated with an invalid CPU. This is a known issue for all system PMU
> drivers, and the conclusion 5 years ago was that it's impractical to close
> this race from outside perf core itself[1][2].

Ok, I'm going to walk right into the trap you've set me...

Why can't we prevent hotplug (e.g. with cpus_read_lock()) while we're
setting this up?

... and climbing back out of that trap, is the conversation you had with
Thomas written down anywhere?

I don't want to block this patch, but if five years has passed with
nobody looking at this then we probably need to address that at some
point before adding more and more broken drivers.

Will

Re: [PATCH v3 2/3] perf: Add driver for Arm NI-700 interconnect PMU

[Robin Murphy]

On 2024-09-04 1:24 pm, Will Deacon wrote:
> On Mon, Sep 02, 2024 at 07:47:18PM +0100, Robin Murphy wrote:
>> On 02/09/2024 3:47 pm, Will Deacon wrote:
>>>> +static ssize_t arm_ni_format_show(struct device *dev,
>>>> +				  struct device_attribute *attr, char *buf)
>>>> +{
>>>> +	struct arm_ni_format_attr *fmt = container_of(attr, typeof(*fmt), attr);
>>>> +	int lo = __ffs(fmt->field), hi = __fls(fmt->field);
>>>> +
>>>> +	return sysfs_emit(buf, "config:%d-%d\n", lo, hi);
>>>> +}
>>>
>>> Nit: if you end up adding single-bit config fields in the future, this
>>> will quietly do the wrong thing. Maybe safe-guard the 'lo==hi' case (even
>>> if you just warn once and return without doing anything).
>>
>> The counter-argument is that I don't foresee having any reason to add
>> single-bit config fields here in future, nor indeed config1 or config2
>> fields, so I intentionally pruned the would-be dead code while copy-pasting
>> this implementation from arm-cmn. Yes, if someone were to make an incomplete
>> change without paying attention or testing they could introduce a bug, but
>> when is that ever not true?
> 
> I guess I'm just a little more wary when it comes to UAPI. Somebody starts
> relying on the broken message and then you're toast. It's also incredibly
> easy to avoid by construction and the dead code isn't hurting anybody.

Even presuming some theoretical unreviewed broken patch did get merged 
and into real-world kernels without ever being fixed, I still struggle 
to imagine how userspace could somehow grow to *rely* on one PMU driver 
displaying a format attribute in an unexpected manner inconsistent with 
every other PMU driver, as opposed to the far more likely scenario of 
going wrong trying to parse it.

Anyway, after playing with some fun compile-time checks yesterday I've 
just realised there is actually an even simpler solution for doing the 
right thing in general, so I guess thanks for leaning on this :)

>>>> +	name = devm_kasprintf(ni->dev, GFP_KERNEL, "arm_ni_%d_cd_%d", ni->id, cd->id);
>>>> +	if (!name)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	err = cpuhp_state_add_instance(arm_ni_hp_state, &cd->cpuhp_node);
>>>> +	if (err)
>>>> +		return err;
>>>
>>> What happens if there's a CPU hotplug operation here? Can we end up calling
>>> perf_pmu_migrate_context() concurrently with perf_pmu_register()?
>>
>> Yes. Alternatively we could register the PMU before the hotplug handler,
>> then potentially miss a hotplug event and leave a user-visible PMU
>> associated with an invalid CPU. This is a known issue for all system PMU
>> drivers, and the conclusion 5 years ago was that it's impractical to close
>> this race from outside perf core itself[1][2].
> 
> Ok, I'm going to walk right into the trap you've set me...
> 
> Why can't we prevent hotplug (e.g. with cpus_read_lock()) while we're
> setting this up?
> 
> ... and climbing back out of that trap, is the conversation you had with
> Thomas written down anywhere?

I think that must have been the time we talked in person in Cambridge. I 
can't now remember if there is (or was) anything in perf_pmu_register() 
that's actually incompatible with being called under cpus_read_lock(), 
but I do recall that the overall concept of exporting more bits of the 
hotplug machinery in order to copy-paste the same boilerplate bodge in 
every PMU driver was... unpopular.

> I don't want to block this patch, but if five years has passed with
> nobody looking at this then we probably need to address that at some
> point before adding more and more broken drivers.

Looking at it again, is there actually a problem with the current state 
of things? Following through the call path:

   perf_pmu_migrate_context()
     __perf_pmu_remove()
       perf_event_groups_first()
         __group_cmp()
           perf_event_groups_cmp()

the "pmu" pointer only seems to be used as a key to match events in the 
current CPU context, which obviously won't find anything at this point. 
AFAICS it's never even dereferenced, unless any events *are* found for 
__perf_pmu_install() to do something with, which necessarily implies an 
initialised and registered PMU for them to have been opened in the first 
place.

Thanks,
Robin.

Re: [PATCH v3 2/3] perf: Add driver for Arm NI-700 interconnect PMU

[Will Deacon]

On Wed, Sep 04, 2024 at 06:15:24PM +0100, Robin Murphy wrote:
> On 2024-09-04 1:24 pm, Will Deacon wrote:
> > On Mon, Sep 02, 2024 at 07:47:18PM +0100, Robin Murphy wrote:
> > > On 02/09/2024 3:47 pm, Will Deacon wrote:
> > > > > +static ssize_t arm_ni_format_show(struct device *dev,
> > > > > +				  struct device_attribute *attr, char *buf)
> > > > > +{
> > > > > +	struct arm_ni_format_attr *fmt = container_of(attr, typeof(*fmt), attr);
> > > > > +	int lo = __ffs(fmt->field), hi = __fls(fmt->field);
> > > > > +
> > > > > +	return sysfs_emit(buf, "config:%d-%d\n", lo, hi);
> > > > > +}
> > > > 
> > > > Nit: if you end up adding single-bit config fields in the future, this
> > > > will quietly do the wrong thing. Maybe safe-guard the 'lo==hi' case (even
> > > > if you just warn once and return without doing anything).
> > > 
> > > The counter-argument is that I don't foresee having any reason to add
> > > single-bit config fields here in future, nor indeed config1 or config2
> > > fields, so I intentionally pruned the would-be dead code while copy-pasting
> > > this implementation from arm-cmn. Yes, if someone were to make an incomplete
> > > change without paying attention or testing they could introduce a bug, but
> > > when is that ever not true?
> > 
> > I guess I'm just a little more wary when it comes to UAPI. Somebody starts
> > relying on the broken message and then you're toast. It's also incredibly
> > easy to avoid by construction and the dead code isn't hurting anybody.
> 
> Even presuming some theoretical unreviewed broken patch did get merged and
> into real-world kernels without ever being fixed, I still struggle to
> imagine how userspace could somehow grow to *rely* on one PMU driver
> displaying a format attribute in an unexpected manner inconsistent with
> every other PMU driver, as opposed to the far more likely scenario of going
> wrong trying to parse it.

I think you lose the game when you try to imagine what userspace could do :)

But c'mon, this is simple to address and then we don't have to imagine
anything.

> Anyway, after playing with some fun compile-time checks yesterday I've just
> realised there is actually an even simpler solution for doing the right
> thing in general, so I guess thanks for leaning on this :)

Hurrah! Thanks.

> > > > > +	name = devm_kasprintf(ni->dev, GFP_KERNEL, "arm_ni_%d_cd_%d", ni->id, cd->id);
> > > > > +	if (!name)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	err = cpuhp_state_add_instance(arm_ni_hp_state, &cd->cpuhp_node);
> > > > > +	if (err)
> > > > > +		return err;
> > > > 
> > > > What happens if there's a CPU hotplug operation here? Can we end up calling
> > > > perf_pmu_migrate_context() concurrently with perf_pmu_register()?
> > > 
> > > Yes. Alternatively we could register the PMU before the hotplug handler,
> > > then potentially miss a hotplug event and leave a user-visible PMU
> > > associated with an invalid CPU. This is a known issue for all system PMU
> > > drivers, and the conclusion 5 years ago was that it's impractical to close
> > > this race from outside perf core itself[1][2].
> > 
> > Ok, I'm going to walk right into the trap you've set me...
> > 
> > Why can't we prevent hotplug (e.g. with cpus_read_lock()) while we're
> > setting this up?
> > 
> > ... and climbing back out of that trap, is the conversation you had with
> > Thomas written down anywhere?
> 
> I think that must have been the time we talked in person in Cambridge. I
> can't now remember if there is (or was) anything in perf_pmu_register()
> that's actually incompatible with being called under cpus_read_lock(), but I
> do recall that the overall concept of exporting more bits of the hotplug
> machinery in order to copy-paste the same boilerplate bodge in every PMU
> driver was... unpopular.

Unpopular, maybe, but you only have to look at a few PMU drivers to see
we're already in that situation for other aspects of the code.
Consolidation would be welcome, but I'd sooner have boilerplate code
than bugs.

> > I don't want to block this patch, but if five years has passed with
> > nobody looking at this then we probably need to address that at some
> > point before adding more and more broken drivers.
> 
> Looking at it again, is there actually a problem with the current state of
> things? Following through the call path:
> 
>   perf_pmu_migrate_context()
>     __perf_pmu_remove()
>       perf_event_groups_first()
>         __group_cmp()
>           perf_event_groups_cmp()
> 
> the "pmu" pointer only seems to be used as a key to match events in the
> current CPU context, which obviously won't find anything at this point.
> AFAICS it's never even dereferenced, unless any events *are* found for
> __perf_pmu_install() to do something with, which necessarily implies an
> initialised and registered PMU for them to have been opened in the first
> place.

You might well be right, but this could will change in future and I don't
think it's been written to work safely under this race. I also pointed
out that a similar race exists on the unregister path.

Will

[PATCH v3 3/3] MAINTAINERS: List Arm interconnect PMUs as supported

[Robin Murphy]

Whatever I may or may not have hoped for, looking after these drivers
seems to have firmly stuck as one of the responsibilities of the job Arm
pays me for, and I would still like to be aware of any other patches, so
make it official.

CC: Ilkka Koskinen <ilkka@os.amperecomputing.com>
CC: Jing Zhang <renyu.zj@linux.alibaba.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v3: No change

 MAINTAINERS | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f328373463b0..1e1cd7414f06 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1737,6 +1737,17 @@ F:	drivers/mtd/maps/physmap-versatile.*
 F:	drivers/power/reset/arm-versatile-reboot.c
 F:	drivers/soc/versatile/
 
+ARM INTERCONNECT PMU DRIVERS
+M:	Robin Murphy <robin.murphy@arm.com>
+S:	Supported
+F:	Documentation/admin-guide/perf/arm-cmn.rst
+F:	Documentation/admin-guide/perf/arm-ni.rst
+F:	Documentation/devicetree/bindings/perf/arm,cmn.yaml
+F:	Documentation/devicetree/bindings/perf/arm,ni.yaml
+F:	drivers/perf/arm-cmn.c
+F:	drivers/perf/arm-ni.c
+F:	tools/perf/pmu-events/arch/arm64/arm/cmn/
+
 ARM KOMEDA DRM-KMS DRIVER
 M:	Liviu Dudau <liviu.dudau@arm.com>
 S:	Supported
-- 
2.39.2.101.g768bb238c484.dirty