Skip to contentSkip to conversation

perf/arm-cmn: Add workarounds for CMN-S3 on Graviton5

PATCH Applied Robin Murphy 3 participants 6 messages Jun 30 – Jul 22

Subsystems (2)
Arm PMU profiling and debugging Arm interconnect PMU drivers
lore
Share & tools
lore

Robin Murphy

author maintainer
#0 lore raw

v6 cover perf/arm-cmn: Add workarounds for CMN-S3 on Graviton5

v5: lore.kernel.org/r/[email protected]

Hi all,

I'm reposting Aviv's series partly for visibility, since the previous versions were buried in a thread, but also to respin patch #1 - Aviv had fixed up the bugs in my initial draft, but I also get reports against the original commit on my branch, so I ended up taking a second look, and deciding the improvement was worthwhile.

Thansk,
Robin.

Aviv Bakal (1):
perf/arm-cmn: Add workarounds for CMN-S3 on Graviton5

Robin Murphy (1):
perf/arm-cmn: Move DTM index data out of hw_perf_event

 drivers/perf/arm-cmn.c | 123 ++++++++++++++++++++++++++++++-----------
 1 file changed, 92 insertions(+), 31 deletions(-)
Signature · 1 line
2.54.0.dirty
author maintainer
#1 lore raw

PATCH v6 1/2 perf/arm-cmn: Move DTM index data out of hw_perf_event

The amount of data we need to store all the per-DTM counter and watchpoint allocations is already testing the limits of hw_perf_event, and future CMNs are only likely to keep growing larger, so move these arrays out to separate memory allocations. As part of that we can use an explicit union for allocating cycle counters to dtc_cycles events, which is arguably nicer anyway.

Reviewed-by Ilkka Koskinen <[email protected]>
Signed-off-by Robin Murphy <[email protected]>

---

v6: Use bitmap functions consistently for cleaner allocation/zeroing

 drivers/perf/arm-cmn.c | 91 ++++++++++++++++++++++++++++--------------
 1 file changed, 61 insertions(+), 30 deletions(-)
drivers/perf/arm-cmn.c +61 -30
index 6e5cc4086a9e..9392838408ff 100644
@@ -598,17 +598,14 @@ static void arm_cmn_debugfs_init(struct arm_cmn *cmn, int id) {}
 
 struct arm_cmn_hw_event {
 	struct arm_cmn_node *dn;
-	u64 dtm_idx[DIV_ROUND_UP(CMN_MAX_NODES_PER_EVENT * 2, 64)];
+	union {
+		unsigned long *dtm_idx;
+		int cc_idx;
+	};
+	unsigned long *wp_idx;
 	s8 dtc_idx[CMN_MAX_DTCS];
 	u8 num_dns;
 	u8 dtm_offset;
-
-	/*
-	 * WP config registers are divided to UP and DOWN events. We need to
-	 * keep to track only one of them.
-	 */
-	DECLARE_BITMAP(wp_idx, CMN_MAX_XPS);
-
 	bool wide_sel;
 	enum cmn_filter_select filter_sel;
 };

@@ -626,25 +623,44 @@ static struct arm_cmn_hw_event *to_cmn_hw(struct perf_event *event)
 	return (struct arm_cmn_hw_event *)&event->hw;
 }
 
-static void arm_cmn_set_index(u64 x[], unsigned int pos, unsigned int val)
+#define BPL2 (BITS_PER_LONG / 2)
+
+static void arm_cmn_set_dtm_idx(struct arm_cmn_hw_event *hw, unsigned int pos, unsigned int val)
 {
-	x[pos / 32] |= (u64)val << ((pos % 32) * 2);
+	hw->dtm_idx[pos / BPL2] |= (unsigned long)val << ((pos % BPL2) * 2);
 }
 
-static unsigned int arm_cmn_get_index(u64 x[], unsigned int pos)
+static unsigned int arm_cmn_get_dtm_idx(struct arm_cmn_hw_event *hw, unsigned int pos)
 {
-	return (x[pos / 32] >> ((pos % 32) * 2)) & 3;
+	return (hw->dtm_idx[pos / BPL2] >> ((pos % BPL2) * 2)) & 3;
 }
 
-static void arm_cmn_set_wp_idx(unsigned long *wp_idx, unsigned int pos, bool val)
+static unsigned long *arm_cmn_alloc_dtm_idx(void)
+{
+	return bitmap_zalloc(CMN_MAX_NODES_PER_EVENT * 2, GFP_KERNEL);
+}
+
+static void arm_cmn_set_wp_idx(struct arm_cmn_hw_event *hw, unsigned int pos, bool val)
 {
 	if (val)
-		set_bit(pos, wp_idx);
+		set_bit(pos, hw->wp_idx);
 }
 
-static unsigned int arm_cmn_get_wp_idx(unsigned long *wp_idx, unsigned int pos)
+static unsigned int arm_cmn_get_wp_idx(struct arm_cmn_hw_event *hw, unsigned int pos)
 {
-	return test_bit(pos, wp_idx);
+	return test_bit(pos, hw->wp_idx);
+}
+
+static unsigned long *arm_cmn_alloc_wp_idx(void)
+{
+	return bitmap_zalloc(CMN_MAX_XPS, GFP_KERNEL);
+}
+
+static void arm_cmn_clear_idx(struct arm_cmn_hw_event *hw)
+{
+	bitmap_zero(hw->dtm_idx, CMN_MAX_NODES_PER_EVENT * 2);
+	if (hw->wp_idx)
+		bitmap_zero(hw->wp_idx, CMN_MAX_XPS);
 }
 
 struct arm_cmn_event_attr {

@@ -1377,7 +1393,7 @@ static int arm_cmn_get_assigned_wp_idx(struct perf_event *event,
 				       struct arm_cmn_hw_event *hw,
 				       unsigned int pos)
 {
-	return CMN_EVENT_EVENTID(event) + arm_cmn_get_wp_idx(hw->wp_idx, pos);
+	return CMN_EVENT_EVENTID(event) + arm_cmn_get_wp_idx(hw, pos);
 }
 
 static void arm_cmn_claim_wp_idx(struct arm_cmn_dtm *dtm,

@@ -1388,7 +1404,7 @@ static void arm_cmn_claim_wp_idx(struct arm_cmn_dtm *dtm,
 	struct arm_cmn_hw_event *hw = to_cmn_hw(event);
 
 	dtm->wp_event[wp_idx] = hw->dtc_idx[dtc];
-	arm_cmn_set_wp_idx(hw->wp_idx, pos, wp_idx - CMN_EVENT_EVENTID(event));
+	arm_cmn_set_wp_idx(hw, pos, wp_idx - CMN_EVENT_EVENTID(event));
 }
 
 static u32 arm_cmn_wp_config(struct perf_event *event, int wp_idx)

@@ -1459,7 +1475,7 @@ static u64 arm_cmn_read_dtm(struct arm_cmn *cmn, struct arm_cmn_hw_event *hw,
 			dtm = &cmn->dtms[dn->dtm] + hw->dtm_offset;
 			reg = readq_relaxed(dtm->base + offset);
 		}
-		dtm_idx = arm_cmn_get_index(hw->dtm_idx, i);
+		dtm_idx = arm_cmn_get_dtm_idx(hw, i);
 		count += (u16)(reg >> (dtm_idx * 16));
 	}
 	return count;

@@ -1506,7 +1522,7 @@ static void arm_cmn_event_read(struct perf_event *event)
 	unsigned long flags;
 
 	if (CMN_EVENT_TYPE(event) == CMN_TYPE_DTC) {
-		delta = arm_cmn_read_cc(cmn->dtc + hw->dtc_idx[0]);
+		delta = arm_cmn_read_cc(cmn->dtc + hw->cc_idx);
 		local64_add(delta, &event->count);
 		return;
 	}

@@ -1573,7 +1589,7 @@ static void arm_cmn_event_start(struct perf_event *event, int flags)
 	int i;
 
 	if (type == CMN_TYPE_DTC) {
-		struct arm_cmn_dtc *dtc = cmn->dtc + hw->dtc_idx[0];
+		struct arm_cmn_dtc *dtc = cmn->dtc + hw->cc_idx;
 
 		writel_relaxed(CMN_DT_DTC_CTL_DT_EN | CMN_DT_DTC_CTL_CG_DISABLE,
 			       dtc->base + CMN_DT_DTC_CTL);

@@ -1591,7 +1607,7 @@ static void arm_cmn_event_start(struct perf_event *event, int flags)
 			writeq_relaxed(mask, base + CMN_DTM_WPn_MASK(wp_idx));
 		}
 	} else for_each_hw_dn(hw, dn, i) {
-		int dtm_idx = arm_cmn_get_index(hw->dtm_idx, i);
+		int dtm_idx = arm_cmn_get_dtm_idx(hw, i);
 
 		arm_cmn_set_event_sel_lo(dn, dtm_idx, CMN_EVENT_EVENTID(event),
 					 hw->wide_sel);

@@ -1607,7 +1623,7 @@ static void arm_cmn_event_stop(struct perf_event *event, int flags)
 	int i;
 
 	if (type == CMN_TYPE_DTC) {
-		struct arm_cmn_dtc *dtc = cmn->dtc + hw->dtc_idx[0];
+		struct arm_cmn_dtc *dtc = cmn->dtc + hw->cc_idx;
 
 		dtc->cc_active = false;
 		writel_relaxed(CMN_DT_DTC_CTL_DT_EN, dtc->base + CMN_DT_DTC_CTL);

@@ -1620,7 +1636,7 @@ static void arm_cmn_event_stop(struct perf_event *event, int flags)
 			writeq_relaxed(~0ULL, base + CMN_DTM_WPn_VAL(wp_idx));
 		}
 	} else for_each_hw_dn(hw, dn, i) {
-		int dtm_idx = arm_cmn_get_index(hw->dtm_idx, i);
+		int dtm_idx = arm_cmn_get_dtm_idx(hw, i);
 
 		arm_cmn_set_event_sel_lo(dn, dtm_idx, 0, hw->wide_sel);
 	}

@@ -1764,6 +1780,14 @@ static enum cmn_filter_select arm_cmn_filter_sel(const struct arm_cmn *cmn,
 }
 
 
+static void arm_cmn_event_destroy(struct perf_event *event)
+{
+	struct arm_cmn_hw_event *hw = to_cmn_hw(event);
+
+	bitmap_free(hw->dtm_idx);
+	bitmap_free(hw->wp_idx);
+}
+
 static int arm_cmn_event_init(struct perf_event *event)
 {
 	struct arm_cmn *cmn = to_cmn(event->pmu);

@@ -1788,6 +1812,11 @@ static int arm_cmn_event_init(struct perf_event *event)
 	if (type == CMN_TYPE_DTC)
 		return arm_cmn_validate_group(cmn, event);
 
+	event->destroy = arm_cmn_event_destroy;
+	hw->dtm_idx = arm_cmn_alloc_dtm_idx();
+	if (!hw->dtm_idx)
+		return -ENOMEM;
+
 	eventid = CMN_EVENT_EVENTID(event);
 	/* For watchpoints we need the actual XP node here */
 	if (type == CMN_TYPE_WP) {

@@ -1798,6 +1827,9 @@ static int arm_cmn_event_init(struct perf_event *event)
 		/* ...but the DTM may depend on which port we're watching */
 		if (cmn->multi_dtm)
 			hw->dtm_offset = CMN_EVENT_WP_DEV_SEL(event) / 2;
+		hw->wp_idx = arm_cmn_alloc_wp_idx();
+		if (!hw->wp_idx)
+			return -ENOMEM;
 	} else if (type == CMN_TYPE_XP &&
 		   (cmn->part == PART_CMN700 || cmn->part == PART_CMN_S3)) {
 		hw->wide_sel = true;

@@ -1848,7 +1880,7 @@ static void arm_cmn_event_clear(struct arm_cmn *cmn, struct perf_event *event,
 
 	while (i--) {
 		struct arm_cmn_dtm *dtm = &cmn->dtms[hw->dn[i].dtm] + hw->dtm_offset;
-		unsigned int dtm_idx = arm_cmn_get_index(hw->dtm_idx, i);
+		unsigned int dtm_idx = arm_cmn_get_dtm_idx(hw, i);
 
 		if (type == CMN_TYPE_WP) {
 			int wp_idx = arm_cmn_get_assigned_wp_idx(event, hw, i);

@@ -1862,8 +1894,7 @@ static void arm_cmn_event_clear(struct arm_cmn *cmn, struct perf_event *event,
 		dtm->pmu_config_low &= ~CMN__PMEVCNT_PAIRED(dtm_idx);
 		writel_relaxed(dtm->pmu_config_low, dtm->base + CMN_DTM_PMU_CONFIG);
 	}
-	memset(hw->dtm_idx, 0, sizeof(hw->dtm_idx));
-	memset(hw->wp_idx, 0, sizeof(hw->wp_idx));
+	arm_cmn_clear_idx(hw);
 
 	for_each_hw_dtc_idx(hw, j, idx)
 		cmn->dtc[j].counters[idx] = NULL;

@@ -1883,7 +1914,7 @@ static int arm_cmn_event_add(struct perf_event *event, int flags)
 				return -ENOSPC;
 
 		cmn->dtc[i].cycles = event;
-		hw->dtc_idx[0] = i;
+		hw->cc_idx = i;
 
 		if (flags & PERF_EF_START)
 			arm_cmn_event_start(event, 0);

@@ -1948,7 +1979,7 @@ static int arm_cmn_event_add(struct perf_event *event, int flags)
 				goto free_dtms;
 		}
 
-		arm_cmn_set_index(hw->dtm_idx, i, dtm_idx);
+		arm_cmn_set_dtm_idx(hw, i, dtm_idx);
 
 		dtm->input_sel[dtm_idx] = input_sel;
 		shift = CMN__PMEVCNTn_GLOBAL_NUM_SHIFT(dtm_idx);

@@ -1981,7 +2012,7 @@ static void arm_cmn_event_del(struct perf_event *event, int flags)
 	arm_cmn_event_stop(event, PERF_EF_UPDATE);
 
 	if (type == CMN_TYPE_DTC)
-		cmn->dtc[hw->dtc_idx[0]].cycles = NULL;
+		cmn->dtc[hw->cc_idx].cycles = NULL;
 	else
 		arm_cmn_event_clear(cmn, event, hw->num_dns);
 }
Signature · 1 line
2.54.0.dirty
author maintainer
#2 lore raw

PATCH v6 2/2 perf/arm-cmn: Add workarounds for CMN-S3 on Graviton5

In reply to Robin Murphy

From: Aviv Bakal <[email protected]>

Graviton5 uses a customised CMN-S3 implementation where certain discovery registers report zeroed fields. Add the following workarounds:

- Introduce a dedicated ACPI HID to identify the Graviton5 CMN variant.
- Derive the DTC domain from the XP node ID, since the unit info register reports it as zero.
- Set the DTC logical ID from the XP's logical ID, since the node info register's logical ID field is also zeroed.

Signed-off-by Aviv Bakal <[email protected]>
Reviewed-by Robin Murphy <[email protected]>
Reviewed-by Ilkka Koskinen <[email protected]>
Signed-off-by Robin Murphy <[email protected]>

---

v6: No change

 drivers/perf/arm-cmn.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)
drivers/perf/arm-cmn.c +31 -1
index 9392838408ff..50402bc4a21d 100644
@@ -31,7 +31,8 @@
 #define CMN_CHILD_NODE_ADDR		GENMASK(29, 0)
 #define CMN_CHILD_NODE_EXTERNAL		BIT(31)
 
-#define CMN_MAX_DIMENSION		12
+/* Some implementations use a mesh larger than the architectural max of 12 */
+#define CMN_MAX_DIMENSION		14
 #define CMN_MAX_XPS			(CMN_MAX_DIMENSION * CMN_MAX_DIMENSION)
 #define CMN_MAX_DTMS			(CMN_MAX_XPS + (CMN_MAX_DIMENSION - 1) * 4)
 

@@ -215,6 +216,8 @@ enum cmn_part {
 	PART_CMN700 = 0x43c,
 	PART_CI700 = 0x43a,
 	PART_CMN_S3 = 0x43e,
+	/* Synthetic part number, overridden to PART_CMN_S3 during discovery */
+	PART_GRAVITON5 = 0xa5,
 };
 
 /* CMN-600 r0px shouldn't exist in silicon, thankfully */

@@ -2253,6 +2256,18 @@ static unsigned int arm_cmn_dtc_domain(struct arm_cmn *cmn, void __iomem *xp_reg
 	return FIELD_GET(CMN_DTM_UNIT_INFO_DTC_DOMAIN, readl_relaxed(xp_region + offset));
 }
 
+static unsigned int arm_cmn_graviton5_dtc_domain(u16 xp_id)
+{
+	unsigned int x = (xp_id >> 7) & 0xf;
+	unsigned int y = (xp_id >> 3) & 0xf;
+
+	/*
+	 * The unit info register reads as zero; derive the DTC domain from
+	 * the XP's mesh coordinates over the 10x14 mesh.
+	 */
+	return (x / 5) + (y / 7) * 2;
+}
+
 static void arm_cmn_init_node_info(struct arm_cmn *cmn, u32 offset, struct arm_cmn_node *node)
 {
 	int level;

@@ -2298,6 +2313,7 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
 	u64 reg;
 	int i, j;
 	size_t sz;
+	bool graviton5_workaround = false;
 
 	arm_cmn_init_node_info(cmn, rgn_offset, &cfg);
 	if (cfg.type != CMN_TYPE_CFG)

@@ -2308,6 +2324,13 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
 	reg = readq_relaxed(cfg_region + CMN_CFGM_PERIPH_ID_01);
 	part = FIELD_GET(CMN_CFGM_PID0_PART_0, reg);
 	part |= FIELD_GET(CMN_CFGM_PID1_PART_1, reg) << 8;
+
+	/* Graviton5 has a customised CMN-S3 which needs some fixups */
+	if (cmn->part == PART_GRAVITON5) {
+		cmn->part = PART_CMN_S3;
+		graviton5_workaround = true;
+	}
+
 	/* 600AE is close enough that it's not really worth more complexity */
 	if (part == PART_CMN600AE)
 		part = PART_CMN600;

@@ -2397,6 +2420,8 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
 
 		if (cmn->part == PART_CMN600)
 			xp->dtc = -1;
+		else if (graviton5_workaround)
+			xp->dtc = arm_cmn_graviton5_dtc_domain(xp->id);
 		else
 			xp->dtc = arm_cmn_dtc_domain(cmn, xp_region);
 

@@ -2475,6 +2500,10 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
 
 			switch (dn->type) {
 			case CMN_TYPE_DTC:
+				if (graviton5_workaround) {
+					/* Node info logical ID is zeroed; use the XP's */
+					dn->logid = xp->logid;
+				}
 				cmn->num_dtcs++;
 				dn++;
 				break;

@@ -2690,6 +2719,7 @@ static const struct acpi_device_id arm_cmn_acpi_match[] = {
 	{ "ARMHC650" },
 	{ "ARMHC700" },
 	{ "ARMHC003" },
+	{ "AMZN0070", PART_GRAVITON5 },
 	{}
 };
 MODULE_DEVICE_TABLE(acpi, arm_cmn_acpi_match);
Signature · 1 line
2.54.0.dirty
AI review
#3 lore raw
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The Graviton5 workaround sets the DTC node's logical ID to the XP's mesh logical ID instead of…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The Graviton5 workaround sets the DTC node's logical ID to the XP's mesh logical ID instead of the DTC domain ID, which can cross-wire PMU event routing. --

commit d22a8571a1d86b4f3c00bde161d5fb2ce99fe43f
Author: Aviv Bakal <[email protected]>

perf/arm-cmn: Add workarounds for CMN-S3 on Graviton5

This patch adds workarounds for the CMN-S3 implementation on Graviton5, which reports zeroed fields in discovery registers. It introduces an ACPI HID for identification, derives the DTC domain from the XP node ID, and sets the DTC logical ID from the XP's logical ID.

Quoted 3 lines
drivers/perf/arm-cmn.c +0 -0

[ ... ]

drivers/perf/arm-cmn.c static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset) · line 2505

Quoted 7 lines
drivers/perf/arm-cmn.c +3 -0
@@ -2475,6 +2500,10 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
 
 			switch (dn->type) {
 			case CMN_TYPE_DTC:
+				if (graviton5_workaround) {
+					/* Node info logical ID is zeroed; use the XP's */
+					dn->logid = xp->logid;

[Severity: Medium]
Does this code assign the host XP's mesh logical ID to the DTC node instead of the computed domain ID (xp->dtc)?

If the DTC nodes' mesh logical IDs do not sort into the exact same order as their domain IDs, will the array indices be cross-wired, potentially misrouting PMU events?

Event configuration looks up the cmn->dtc[] array using the node's domain ID (dn->dtc).

Could this instead be dn->logid = xp->dtc; to restore the correct semantic meaning of a DTC's logical ID as its domain index?

Quoted 4 lines
drivers/perf/arm-cmn.c +1 -0
+				}
 				cmn->num_dtcs++;
 				dn++;
 				break;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
maintainer Applied
#4 lore raw
In reply to Robin Murphy
Quoted 11 lines · Robin Murphy

v5: lore.kernel.org/r/[email protected]

Hi all,

I'm reposting Aviv's series partly for visibility, since the previous versions were buried in a thread, but also to respin patch #1 - Aviv had fixed up the bugs in my initial draft, but I also get reports against the original commit on my branch, so I ended up taking a second look, and deciding the improvement was worthwhile.

[...]

Applied to arm64 (for-next/perf), thanks!

[1/2] perf/arm-cmn: Move DTM index data out of hw_perf_event

      https://git.kernel.org/arm64/c/56603c28d916
[2/2] perf/arm-cmn: Add workarounds for CMN-S3 on Graviton5
      https://git.kernel.org/arm64/c/9d0b1714e1af

If you get a chance, please can you take a look at Sashiko's comment on the second patch? It said something similar on Aviv's v5:

https://sashiko.dev/#/patchset/[email protected]

Cheers,

Signature · 5 lines
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
author maintainer
#5 lore raw
Quoted 16 lines · Will Deacon
Quoted 11 lines · Robin Murphy

v5: lore.kernel.org/r/[email protected]

Hi all,

I'm reposting Aviv's series partly for visibility, since the previous versions were buried in a thread, but also to respin patch #1 - Aviv had fixed up the bugs in my initial draft, but I also get reports against the original commit on my branch, so I ended up taking a second look, and deciding the improvement was worthwhile.

[...]

Applied to arm64 (for-next/perf), thanks!

[1/2] perf/arm-cmn: Move DTM index data out of hw_perf_event

Quoted 8 lines
       https://git.kernel.org/arm64/c/56603c28d916
[2/2] perf/arm-cmn: Add workarounds for CMN-S3 on Graviton5
       https://git.kernel.org/arm64/c/9d0b1714e1af

If you get a chance, please can you take a look at Sashiko's comment on the second patch? It said something similar on Aviv's v5:

   https://sashiko.dev/#/patchset/[email protected]

I did already respond to that back on v3 or so - the code is correct compared to what the standard CMN tooling generates. The main DTC always has a logical ID of 0, while secondary DTCs inherit the logical ID of whatever XP they are attached to (and in this case the main DTC is on XP 0 anyway so it works out neatly).

Subsequently I did also spend a while trying to make sense of the generation scripts themselves, and while I couldn't find any obvious proof that the DTC domain numbers (other than 0) will always line up with the logical ID order (although there definitely looks to be some sorting of things before the XP logical IDs are assigned), apparently it's not been an issue for anything anyone's built so far, and if it did become one, then it's a problem for arm_cmn_init_dtcs() to solve in general.

Cheers,
Robin.