[RESEND PATCH v4 00/11] perf/cxlpmu: Misc sashiko raised issues fixes

[RESEND PATCH v4 00/11] perf/cxlpmu: Misc sashiko raised issues fixes

[Dave Jiang]

[resend, mangled linux-cxl mailing addr]

Teed off of Davidlohr's misc cxlpmu series [1]. I had Claude pick up all
the sashiko raised issues and then continued multiple internal review
iterations to pick up a number of fixes. I'm no CXL PMU or perf expert,
but the fixes look reasonable to me AFAICT.

Since v3:
- 4/11 and 5/11 swapped, so the one-line MSI vector 0 fix comes before the
  info->msi_vec rename and backports on its own (Robin).
- 6/11: new patch, add the PMUs after configuring events.
- 7/11: no code change; the commit message no longer overstates what
  dropping IRQF_SHARED costs a device that shares a vector.
- 8/11: quote the spec sentence that makes the freeze sticky, and correct
  the claim about what the migration window leaves enabled (Richard,
  sashiko-bot).
- 10/11: split the probe-time overflow clear out, so this is back to the
  single hunk Jonathan acked.
- 11/11: new patch. Clear a counter's stale overflow status when starting
  an event, which is where the reuse case bites; the probe clear moved
  here from 10/11 (Richard).

Since v2:
- 1/11: check event_idx alongside counter_idx (Jonathan).
- 2/11: use FIELD_MODIFY() rather than mask-then-OR (Jonathan).
- 3/11: rewrite the rationale, no code change (Jonathan).
- 5/11: new patch, split the MSI vector out of info->irq (Robin, Jonathan).
- 7/11: drop IRQF_SHARED as well as adding IRQF_NOBALANCING, and retitle
  (Jonathan, Robin).
- 8/11: don't unfreeze if the PMU has been disabled meanwhile (Robin,
  Jonathan).
- 10/11: retitle, and drop the cxl_pmu_offline_cpu() hunk since that
  dev_err() cannot be reached (Robin).
- Dropped the old 9/9 that guarded cpumask_show() against cpumask_of(-1).
  The init case was a false positive and neither reviewer liked the shape
  of it (Jonathan, Robin).

Since v1:
- Updated 3/9 to address sashiko issue.
- No other changes as sashiko only raised issue with pre-existing.

Three follow-ups this series does not attempt:

cxl_pmu_read() relies on local64_t being same-CPU, which is the only reason
the interrupt has to be pinned at all. Moving prev_count and event->count
off local64_t would make the affinity question moot, and Robin may have a
view on whether that is worth doing.

cxl_pmu_offline_cpu() still leaves on_cpu at -1 across a sleeping
perf_pmu_migrate_context(), and still has an unreachable no-target branch.
Both want removing, possibly on top of the generic PMU hotplug rework
Jonathan pointed at [2].

Firmware can leave a counter enabled with Global Freeze on Overflow but not
Interrupt on Overflow, which stalls the whole CPMU with no interrupt to say
so. Quiescing the block at probe needs two writes per counter to stay
within the spec's rule about changing an enabled counter, so it belongs in
its own patch.

[1]: https://lore.kernel.org/linux-cxl/20260715191454.459673-1-dave@stgolabs.net/
[2]: https://lore.kernel.org/linux-arm-kernel/cover.1784911757.git.robin.murphy@arm.com/

Dave Jiang (11):
  perf/cxl: Program the requested event group on configurable counters
  perf/cxl: Clear stale event fields before reprogramming a counter
  perf/cxl: Fix the counter overflow delta fixup
  perf/cxl: Accept an overflow interrupt on MSI message number 0
  perf/cxl: Split the MSI vector out of info->irq
  cxl/pci: Add the PMUs after configuring events
  perf/cxl: Don't share the overflow interrupt, and keep it pinned
  perf/cxl: Unfreeze counters after handling an overflow interrupt
  perf/cxl: Validate the hardware-reported counter width
  perf/cxl: Don't log through pmu.dev in the overflow interrupt handler
  perf/cxl: Clear stale overflow status before using a counter

 drivers/cxl/pci.c      |  11 +++--
 drivers/perf/cxl_pmu.c | 107 ++++++++++++++++++++++++++++++++---------
 2 files changed, 90 insertions(+), 28 deletions(-)


base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff
-- 
2.54.0

[RESEND PATCH v4 01/11] perf/cxl: Program the requested event group on configurable counters

[Dave Jiang]

cxl_pmu_get_event_idx() sets *counter_idx for a configurable counter but
never *event_idx. hwc->event_base stays 0, so cxl_pmu_event_start()
programs event group 0 no matter which group the user asked for.

Return the matched capability's event_idx.

Fixes: 5d7107c72796 ("perf: CXL Performance Monitoring Unit driver")
Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1
Assisted-by: Claude:claude-opus-4-8
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/perf/cxl_pmu.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index 68a54d97d2a8..ff01b658e1b4 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -513,7 +513,10 @@ static const struct attribute_group *cxl_pmu_attr_groups[] = {
 	NULL
 };
 
-/* If counter_idx == NULL, don't try to allocate a counter. */
+/*
+ * If counter_idx == NULL, don't try to allocate a counter. Callers that
+ * allocate pass both counter_idx and event_idx.
+ */
 static int cxl_pmu_get_event_idx(struct perf_event *event, int *counter_idx,
 				 int *event_idx)
 {
@@ -541,7 +544,7 @@ static int cxl_pmu_get_event_idx(struct perf_event *event, int *counter_idx,
 
 	pmu_ev = cxl_pmu_find_config_counter_ev_cap(info, vid, gid, mask);
 	if (!IS_ERR(pmu_ev)) {
-		if (!counter_idx)
+		if (!counter_idx || !event_idx)
 			return 0;
 
 		bitmap_andnot(configurable_and_free, info->conf_counter_bm,
@@ -552,6 +555,7 @@ static int cxl_pmu_get_event_idx(struct perf_event *event, int *counter_idx,
 			return -EINVAL;
 
 		*counter_idx = i;
+		*event_idx = pmu_ev->event_idx;
 		return 0;
 	}
 
-- 
2.54.0

[RESEND PATCH v4 02/11] perf/cxl: Clear stale event fields before reprogramming a counter

[Dave Jiang]

cxl_pmu_event_start() ORs the event group id, event mask, edge and invert
selections into a configurable counter's config register without clearing
them first, and cxl_pmu_event_stop() leaves them set. Reuse the counter for
another event and the new selection lands on top of the old one, so the
counter counts the wrong events with stale edge and invert behaviour.

Use FIELD_MODIFY() so each field is replaced rather than accumulated.

Fixes: 5d7107c72796 ("perf: CXL Performance Monitoring Unit driver")
Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/perf/cxl_pmu.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index ff01b658e1b4..b16e2e4090a3 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -646,17 +646,17 @@ static void cxl_pmu_event_start(struct perf_event *event, int flags)
 	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_INT_ON_OVRFLW, 1);
 	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_FREEZE_ON_OVRFLW, 1);
 	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_ENABLE, 1);
-	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_EDGE,
-			  cxl_pmu_config1_get_edge(event) ? 1 : 0);
-	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_INVERT,
-			  cxl_pmu_config1_get_invert(event) ? 1 : 0);
+	FIELD_MODIFY(CXL_PMU_COUNTER_CFG_EDGE, &cfg,
+		     cxl_pmu_config1_get_edge(event) ? 1 : 0);
+	FIELD_MODIFY(CXL_PMU_COUNTER_CFG_INVERT, &cfg,
+		     cxl_pmu_config1_get_invert(event) ? 1 : 0);
 
 	/* Fixed purpose counters have next two fields RO */
 	if (test_bit(hwc->idx, info->conf_counter_bm)) {
-		cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_EVENT_GRP_ID_IDX_MSK,
-				  hwc->event_base);
-		cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_EVENTS_MSK,
-				  cxl_pmu_config_get_mask(event));
+		FIELD_MODIFY(CXL_PMU_COUNTER_CFG_EVENT_GRP_ID_IDX_MSK, &cfg,
+			     hwc->event_base);
+		FIELD_MODIFY(CXL_PMU_COUNTER_CFG_EVENTS_MSK, &cfg,
+			     cxl_pmu_config_get_mask(event));
 	}
 	cfg &= ~CXL_PMU_COUNTER_CFG_THRESHOLD_MSK;
 	/*
-- 
2.54.0

[RESEND PATCH v4 03/11] perf/cxl: Fix the counter overflow delta fixup

[Dave Jiang]

The counter is masked to counter_width, so the subtraction in
__cxl_pmu_read() throws away the bit that records the wrap. A delta of one
whole period reads back as 0, the same as no events at all, and only the
overflow status tells the two apart - which is why __cxl_pmu_read() takes
an overflow argument.

The fixup keys off the delta rather than the operands:

	delta = (new_cnt - prev_cnt) & GENMASK_ULL(counter_width - 1, 0);
	if (overflow && delta < GENMASK_ULL(counter_width - 1, 0))
		delta += (1UL << counter_width);

so it cannot tell which of these it is looking at:

  event_start         polled read          wrap
  ctr = 0 ..........  ctr = P/2 .........  mask -> 0 (+r)
  prev = 0            prev = P/2                 IRQ reads new = r

  prev = 0    (no read yet):  new >= prev, fell short -> add period
  prev = P/2  (polled):       new <  prev, spans wrap -> add nothing

Both rows are the same interrupt and the old guard adds a period in both,
so a mid-period read makes the event over-count. 'perf stat -I' hits that.

Condition the fixup on new_cnt >= prev_cnt, the one case the subtraction
cannot express. Dropping it outright would break the first row. It stays
exact whatever the residual r is, which matters because some events
increment by more than 1 per cycle (CXL r4.0 8.2.7.2.1, Threshold) and can
step past 0 as they wrap.

Use mask + 1 for the period rather than a shift. It is 0 for a 64-bit
counter, and avoids the old 1UL << counter_width - undefined for width 64,
and for >= 32 on 32-bit kernels.

Fixes: 5d7107c72796 ("perf: CXL Performance Monitoring Unit driver")
Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/perf/cxl_pmu.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index b16e2e4090a3..40741e529d9b 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -689,7 +689,7 @@ static void __cxl_pmu_read(struct perf_event *event, bool overflow)
 {
 	struct cxl_pmu_info *info = pmu_to_cxl_pmu_info(event->pmu);
 	struct hw_perf_event *hwc = &event->hw;
-	u64 new_cnt, prev_cnt, delta;
+	u64 new_cnt, prev_cnt, delta, mask;
 
 	do {
 		prev_cnt = local64_read(&hwc->prev_count);
@@ -697,12 +697,16 @@ static void __cxl_pmu_read(struct perf_event *event, bool overflow)
 	} while (local64_cmpxchg(&hwc->prev_count, prev_cnt, new_cnt) != prev_cnt);
 
 	/*
-	 * If we know an overflow occur then take that into account.
-	 * Note counter is not reset as that would lose events
+	 * The mask discards the bit that says the counter wrapped, so a delta of
+	 * one whole period reads back as 0 - the same as no events at all. Only
+	 * the overflow status separates them, and new_cnt >= prev_cnt is that
+	 * case, so add the period back. mask + 1 is 2^counter_width, which comes
+	 * out as 0 for a 64-bit counter and avoids an undefined 1 << 64.
 	 */
-	delta = (new_cnt - prev_cnt) & GENMASK_ULL(info->counter_width - 1, 0);
-	if (overflow && delta < GENMASK_ULL(info->counter_width - 1, 0))
-		delta += (1UL << info->counter_width);
+	mask = GENMASK_ULL(info->counter_width - 1, 0);
+	delta = (new_cnt - prev_cnt) & mask;
+	if (overflow && new_cnt >= prev_cnt)
+		delta += mask + 1;
 
 	local64_add(delta, &event->count);
 }
-- 
2.54.0

[RESEND PATCH v4 04/11] perf/cxl: Accept an overflow interrupt on MSI message number 0

[Dave Jiang]

cxl_pmu_probe() rejects the PMU when info->irq <= 0, but at that point the
field still holds the MSI/MSI-X message number the device signals overflow
on, not a Linux virq. That number is 0-based, and -1 means no interrupt
support. Message number 0 is valid and pci_irq_vector() takes a 0-based
index, so a compliant device signalling on the first vector fails to probe.

Reject only the no-interrupt case, matching how the CXL mailbox and event
interrupts handle it.

Fixes: 5d7107c72796 ("perf: CXL Performance Monitoring Unit driver")
Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1
Assisted-by: Claude:claude-opus-4-8
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Acked-by: Richard Cheng <icheng@nvidia.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v4:
- Move ahead of the info->msi_vec rename so this one-liner stands alone for
  backporting (Robin).
---
 drivers/perf/cxl_pmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index 40741e529d9b..2bebbbe67a0a 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -873,7 +873,7 @@ static int cxl_pmu_probe(struct device *dev)
 		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
 	};
 
-	if (info->irq <= 0)
+	if (info->irq < 0)
 		return -EINVAL;
 
 	rc = pci_irq_vector(pdev, info->irq);
-- 
2.54.0

[RESEND PATCH v4 05/11] perf/cxl: Split the MSI vector out of info->irq

[Dave Jiang]

info->irq means two different things depending on how far probe has got.
cxl_pmu_parse_caps() sets it to the MSI/MSI-X message number from the CPMU
Capability register. cxl_pmu_probe() passes that to pci_irq_vector() and
then overwrites it with the Linux virq, which is what the hotplug callbacks
hand to irq_set_affinity().

Both are ints called irq, so nothing catches a mix-up, and the valid ranges
differ. 0 is a fine message number but never a valid virq.

Give the message number its own field. No functional change.

Assisted-by: Claude:claude-opus-4-8
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v4:
- Move after the one-line fix so that fix has no dependency on this rename
  (Robin).
---
 drivers/perf/cxl_pmu.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index 2bebbbe67a0a..481d32d0c1b0 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -106,6 +106,7 @@ struct cxl_pmu_info {
 	int on_cpu;
 	struct hlist_node node;
 	bool filter_hdm;
+	int msi_vec;
 	int irq;
 };
 
@@ -143,9 +144,9 @@ static int cxl_pmu_parse_caps(struct device *dev, struct cxl_pmu_info *info)
 
 	info->filter_hdm = FIELD_GET(CXL_PMU_CAP_FILTERS_SUP_MSK, val) & CXL_PMU_FILTER_HDM;
 	if (FIELD_GET(CXL_PMU_CAP_INT, val))
-		info->irq = FIELD_GET(CXL_PMU_CAP_MSI_N_MSK, val);
+		info->msi_vec = FIELD_GET(CXL_PMU_CAP_MSI_N_MSK, val);
 	else
-		info->irq = -1;
+		info->msi_vec = -1;
 
 	/* First handle fixed function counters; note if configurable counters found */
 	for (i = 0; i < info->num_counters; i++) {
@@ -873,10 +874,10 @@ static int cxl_pmu_probe(struct device *dev)
 		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
 	};
 
-	if (info->irq < 0)
+	if (info->msi_vec < 0)
 		return -EINVAL;
 
-	rc = pci_irq_vector(pdev, info->irq);
+	rc = pci_irq_vector(pdev, info->msi_vec);
 	if (rc < 0)
 		return rc;
 	irq = rc;
-- 
2.54.0

[RESEND PATCH v4 06/11] cxl/pci: Add the PMUs after configuring events

[Dave Jiang]

cxl_pci_probe() adds the CPMU instances before configuring the event
interrupts. Both take an MSI/MSI-X vector the device names in its own
capability registers, and the spec lets a device point several capabilities
at one message number, so the two can collide.

They do not fail the same way. A failed devm_cxl_pmu_add() only breaks out
of the loop, while a failed cxl_event_config() aborts probe and the memory
device never appears. Going PMUs-first therefore lets a peripheral facility
take the vector and deny it to one that matters.

Configure events first, so a collision costs the PMU rather than the
memdev. Return 0 explicitly afterwards, since rc still holds any PMU error
that cxl_event_config() used to overwrite on the way past.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v4:
- New patch, ahead of the PMU dropping IRQF_SHARED. Without this that
  change can turn a shared vector into a probe failure for the whole
  memdev rather than a missing PMU (sashiko-bot).
---
 drivers/cxl/pci.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 267c679b0b3c..975d32716762 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -894,6 +894,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		dev_dbg(&pdev->dev, "No CXL FWCTL setup\n");
 
+	rc = cxl_event_config(host_bridge, mds, irq_avail);
+	if (rc)
+		return rc;
+
 	pmu_count = cxl_count_regblock(pdev, CXL_REGLOC_RBI_PMU);
 	if (pmu_count < 0)
 		return pmu_count;
@@ -920,13 +924,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		}
 	}
 
-	rc = cxl_event_config(host_bridge, mds, irq_avail);
-	if (rc)
-		return rc;
-
 	pci_save_state(pdev);
 
-	return rc;
+	/* A missing PMU is not fatal, the memdev is still usable */
+	return 0;
 }
 
 static const struct pci_device_id cxl_mem_pci_tbl[] = {
-- 
2.54.0

[RESEND PATCH v4 07/11] perf/cxl: Don't share the overflow interrupt, and keep it pinned

[Dave Jiang]

The PMU pins its overflow interrupt to info->on_cpu in the hotplug
callbacks, but requests it with only IRQF_SHARED | IRQF_NO_THREAD. Without
IRQF_NOBALANCING, irqbalance or a userspace smp_affinity write can move it
elsewhere, and cxl_pmu_irq() then runs local64_cmpxchg() and local64_add()
on hwc->prev_count and event->count at the same time as the managing CPU.
local64_t is only atomic against same-CPU access, so the counts corrupt.

The flag alone does not fix that while the line is shared. __setup_irq()
only acts on it for the first action on a line, and a co-owner has no
reason to want our affinity - it would keep taking the interrupt wherever
its own points, running our handler on the wrong CPU.

Drop IRQF_SHARED and add IRQF_NOBALANCING. The spec only recommends a
distinct Interrupt Message Number per CPMU instance (CXL r4.0 8.2.7.1.1),
so a device may put several on one vector, or share with the mailbox or
event log. Such a device now loses a PMU rather than silently miscounting -
and only the PMU, since the preceding patch adds the CPMUs after the event
configuration.

Fixes: 5d7107c72796 ("perf: CXL Performance Monitoring Unit driver")
Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1
Assisted-by: Claude:claude-opus-4-8
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/perf/cxl_pmu.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index 481d32d0c1b0..448e1da3d59f 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -783,7 +783,7 @@ static irqreturn_t cxl_pmu_irq(int irq, void *data)
 
 	overflowed = readq(base + CXL_PMU_OVERFLOW_REG);
 
-	/* Interrupt may be shared, so maybe it isn't ours */
+	/* Nothing overflowed, so the device did not raise this */
 	if (!overflowed)
 		return IRQ_NONE;
 
@@ -886,7 +886,14 @@ static int cxl_pmu_probe(struct device *dev)
 	if (!irq_name)
 		return -ENOMEM;
 
-	rc = devm_request_irq(dev, irq, cxl_pmu_irq, IRQF_SHARED | IRQF_NO_THREAD,
+	/*
+	 * The handler must run on info->on_cpu, so the interrupt cannot be
+	 * shared - IRQF_NOBALANCING is only honoured for the first action on a
+	 * line, and a co-owner would keep taking the interrupt wherever its own
+	 * affinity points.
+	 */
+	rc = devm_request_irq(dev, irq, cxl_pmu_irq,
+			      IRQF_NO_THREAD | IRQF_NOBALANCING,
 			      irq_name, info);
 	if (rc)
 		return rc;
-- 
2.54.0

[RESEND PATCH v4 08/11] perf/cxl: Unfreeze counters after handling an overflow interrupt

[Dave Jiang]

The counters run with Freeze on Overflow set, so one overflow freezes every
counter in the block (CXL r4.0 8.2.7.2.1), and a frozen counter "remains
frozen until explicitly unfrozen by software" (8.2.7.1.3, Table 8-183).
cxl_pmu_irq() reads the overflowed counters and clears the overflow status
but never unfreezes, so everything stays frozen until the next pmu_enable()
and events in that window are lost.

Unfreeze after clearing the status, unless the PMU has been disabled in the
meantime - cxl_pmu_disable() freezes the block deliberately. Track the
enabled state and leave it frozen in that case.

Fixes: 5d7107c72796 ("perf: CXL Performance Monitoring Unit driver")
Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1
Assisted-by: Claude:claude-opus-4-8
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/perf/cxl_pmu.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index 448e1da3d59f..a081fcba6991 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -108,6 +108,8 @@ struct cxl_pmu_info {
 	bool filter_hdm;
 	int msi_vec;
 	int irq;
+	/* Set between pmu_enable() and pmu_disable(), read by the IRQ handler */
+	bool enabled;
 };
 
 #define pmu_to_cxl_pmu_info(_pmu) container_of(_pmu, struct cxl_pmu_info, pmu)
@@ -596,6 +598,7 @@ static void cxl_pmu_enable(struct pmu *pmu)
 	void __iomem *base = info->base;
 
 	/* Can assume frozen at this stage */
+	WRITE_ONCE(info->enabled, true);
 	writeq(0, base + CXL_PMU_FREEZE_REG);
 }
 
@@ -604,6 +607,7 @@ static void cxl_pmu_disable(struct pmu *pmu)
 	struct cxl_pmu_info *info = pmu_to_cxl_pmu_info(pmu);
 	void __iomem *base = info->base;
 
+	WRITE_ONCE(info->enabled, false);
 	/*
 	 * Whilst bits above number of counters are RsvdZ
 	 * they are unlikely to be repurposed given
@@ -802,6 +806,21 @@ static irqreturn_t cxl_pmu_irq(int irq, void *data)
 
 	writeq(overflowed, base + CXL_PMU_OVERFLOW_REG);
 
+	/*
+	 * An overflow freezes every counter in the CPMU, so unfreeze once the
+	 * overflowed ones have been read and their status cleared. Otherwise
+	 * they stay frozen until the next pmu_enable() and events are lost.
+	 *
+	 * Not while the PMU is disabled, so as not to undo an intentional freeze.
+	 * The check is advisory, not exclusive: pmu_disable() normally runs on
+	 * info->on_cpu with interrupts off, where the pinned handler cannot
+	 * preempt it. In the one window where it does not - the migration in
+	 * cxl_pmu_offline_cpu() - the counters are legitimately running again,
+	 * so unfreezing is correct there anyway.
+	 */
+	if (READ_ONCE(info->enabled))
+		writeq(0, base + CXL_PMU_FREEZE_REG);
+
 	return IRQ_HANDLED;
 }
 
-- 
2.54.0

[RESEND PATCH v4 09/11] perf/cxl: Validate the hardware-reported counter width

[Dave Jiang]

cxl_pmu_parse_caps() takes the Counter Width straight from the CPMU
Capability register without a bounds check. The Counter Data register is 64
bits wide, so a device reporting 0 or more than 64 is reporting nonsense,
and GENMASK_ULL(width - 1, 0) in the read path then shifts out of range.
That is undefined behaviour, and a UBSAN splat where it is enabled.

Reject a counter width outside 1..64 at probe.

Fixes: 5d7107c72796 ("perf: CXL Performance Monitoring Unit driver")
Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1
Assisted-by: Claude:claude-opus-4-8
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/perf/cxl_pmu.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index a081fcba6991..84ebc7da181d 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -142,6 +142,15 @@ static int cxl_pmu_parse_caps(struct device *dev, struct cxl_pmu_info *info)
 
 	info->num_counters = FIELD_GET(CXL_PMU_CAP_NUM_COUNTERS_MSK, val) + 1;
 	info->counter_width = FIELD_GET(CXL_PMU_CAP_COUNTER_WIDTH_MSK, val);
+	/*
+	 * The Counter Data register is 64 bits wide, so a Counter Width of 0 or
+	 * >64 is invalid. Reject it rather than let GENMASK_ULL(width - 1, 0) in
+	 * the read path shift out of range.
+	 */
+	if (info->counter_width == 0 || info->counter_width > 64) {
+		dev_err(dev, "Invalid counter width %d\n", info->counter_width);
+		return -ENODEV;
+	}
 	info->num_event_capabilities = FIELD_GET(CXL_PMU_CAP_NUM_EVN_CAP_REG_SUP_MSK, val) + 1;
 
 	info->filter_hdm = FIELD_GET(CXL_PMU_CAP_FILTERS_SUP_MSK, val) & CXL_PMU_FILTER_HDM;
-- 
2.54.0

[RESEND PATCH v4 10/11] perf/cxl: Don't log through pmu.dev in the overflow interrupt handler

[Dave Jiang]

perf_pmu_unregister() frees pmu->dev without clearing the pointer, and
cxl_pmu_probe() orders its devm actions so that teardown runs it before
free_irq(). Nothing in between masks the interrupt, and
cxl_pmu_event_stop() leaves the overflow status bit set, so an overflow
taken just before teardown is still delivered and still reaches the
dev_dbg() in the !event branch - by which point info->pmu.dev is freed.

Log through info->pmu.parent instead, which is devm-managed and outlives
every teardown action.

Fixes: 5d7107c72796 ("perf: CXL Performance Monitoring Unit driver")
Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1
Assisted-by: Claude:claude-opus-4-8
Acked-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v4:
- Split the probe-time overflow clear out, so this is back to the single
  hunk Jonathan acked in v2.
---
 drivers/perf/cxl_pmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index 84ebc7da181d..45edef53a983 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -805,7 +805,7 @@ static irqreturn_t cxl_pmu_irq(int irq, void *data)
 		struct perf_event *event = info->hw_events[i];
 
 		if (!event) {
-			dev_dbg(info->pmu.dev,
+			dev_dbg(info->pmu.parent,
 				"overflow but on non enabled counter %d\n", i);
 			continue;
 		}
-- 
2.54.0

[RESEND PATCH v4 11/11] perf/cxl: Clear stale overflow status before using a counter

[Dave Jiang]

cxl_pmu_event_stop() leaves the counter's bit set in the overflow status
register and only cxl_pmu_irq() ever clears it, so a counter handed from
one event to the next starts with the previous owner's overflow pending.
The perf core stops and starts back to back with interrupts off, so an
interrupt latched over the handover lands on the new event. prev_count is 0
by then, so the read path adds a full period: 2^counter_width phantom
counts.

Clear the bit in cxl_pmu_event_start(), before the config write arms
Interrupt on Overflow. Clear the whole register at probe too, for counters
no event ever owns - the driver never touched it, so firmware or a previous
kernel can leave a bit set there.

Fixes: 5d7107c72796 ("perf: CXL Performance Monitoring Unit driver")
Reported-by: Richard Cheng <icheng@nvidia.com>
Closes: https://lore.kernel.org/linux-cxl/anAVpO--i5heVfz9@MWDK4CY14F/
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v4:
- New patch. The event_start() clear is Richard's; the probe clear was a
  second hunk on the pmu.dev patch in v3.
- Clear before the config write rather than after it, so the question of
  whether arming the interrupt over a set status bit can raise an MSI does
  not arise (sashiko-bot).
---
 drivers/perf/cxl_pmu.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index 45edef53a983..3ec8933b04e2 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -683,6 +683,15 @@ static void cxl_pmu_event_start(struct perf_event *event, int flags)
 	 */
 	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_THRESHOLD_MSK,
 			  cxl_pmu_config1_get_threshold(event));
+
+	/*
+	 * Drop any overflow the previous owner of this counter left pending;
+	 * cxl_pmu_event_stop() does not. Otherwise an interrupt latched over
+	 * the handover gets charged to this event. Do it before arming Interrupt
+	 * on Overflow below, and note RW1C leaves the other counters alone.
+	 */
+	writeq(BIT_ULL(hwc->idx), base + CXL_PMU_OVERFLOW_REG);
+
 	writeq(cfg, base + CXL_PMU_COUNTER_CFG_REG(hwc->idx));
 
 	local64_set(&hwc->prev_count, 0);
@@ -914,6 +923,14 @@ static int cxl_pmu_probe(struct device *dev)
 	if (!irq_name)
 		return -ENOMEM;
 
+	/*
+	 * Same for counters no event owns yet: clear whatever firmware or a
+	 * previous kernel left set before the handler goes live. Bits above the
+	 * implemented counters are reserved, so only write those.
+	 */
+	writeq(GENMASK_ULL(info->num_counters - 1, 0),
+	       info->base + CXL_PMU_OVERFLOW_REG);
+
 	/*
 	 * The handler must run on info->on_cpu, so the interrupt cannot be
 	 * shared - IRQF_NOBALANCING is only honoured for the first action on a
-- 
2.54.0

Re: [RESEND PATCH v4 02/11] perf/cxl: Clear stale event fields before reprogramming a counter

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect 64-bit write (writeq) to a 32-bit hardware filter configuration register.
- [High] Incorrect PMU counter overflow handling logic leads to massive overcounting and Undefined Behavior (UB) via out-of-bounds bitwise shift.
--

--- Patch [2]: [RESEND PATCH v4 02/11] perf/cxl: Clear stale event fields before reprogramming a counter ---
commit 215e20dd2e223d030442e8b7a911140cd3ebb0bf
Author: Dave Jiang <dave.jiang@intel.com>

perf/cxl: Clear stale event fields before reprogramming a counter

This patch fixes an issue in cxl_pmu_event_start() where previous event
settings were accumulated instead of cleared. It uses FIELD_MODIFY() to
ensure event group ID, mask, edge, and invert selections replace the old
values, preventing counters from using stale configuration.

Link: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index ff01b658e1b45..b16e2e4090a39 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
> @@ -646,17 +646,17 @@ static void cxl_pmu_event_start(struct perf_event *event, int flags)
>  	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_INT_ON_OVRFLW, 1);
>  	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_FREEZE_ON_OVRFLW, 1);
>  	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_ENABLE, 1);
> -	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_EDGE,
> -			  cxl_pmu_config1_get_edge(event) ? 1 : 0);
> -	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_INVERT,
> -			  cxl_pmu_config1_get_invert(event) ? 1 : 0);
> +	FIELD_MODIFY(CXL_PMU_COUNTER_CFG_EDGE, &cfg,
> +		     cxl_pmu_config1_get_edge(event) ? 1 : 0);
> +	FIELD_MODIFY(CXL_PMU_COUNTER_CFG_INVERT, &cfg,
> +		     cxl_pmu_config1_get_invert(event) ? 1 : 0);
>  
>  	/* Fixed purpose counters have next two fields RO */
>  	if (test_bit(hwc->idx, info->conf_counter_bm)) {
> -		cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_EVENT_GRP_ID_IDX_MSK,
> -				  hwc->event_base);
> -		cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_EVENTS_MSK,
> -				  cxl_pmu_config_get_mask(event));
> +		FIELD_MODIFY(CXL_PMU_COUNTER_CFG_EVENT_GRP_ID_IDX_MSK, &cfg,
> +			     hwc->event_base);
> +		FIELD_MODIFY(CXL_PMU_COUNTER_CFG_EVENTS_MSK, &cfg,
> +			     cxl_pmu_config_get_mask(event));
>  	}
>  	cfg &= ~CXL_PMU_COUNTER_CFG_THRESHOLD_MSK;
>  	/*

[Severity: High]
This isn't a bug introduced by this patch, but while looking at
cxl_pmu_event_start(), does this writeq() overwrite the configuration
for the adjacent filter?

drivers/perf/cxl_pmu.c:cxl_pmu_event_start() {
    ...
	if (info->filter_hdm) {
		if (cxl_pmu_config1_hdm_filter_en(event))
			cfg = cxl_pmu_config2_get_hdm_decoder(event);
		else
			cfg = GENMASK(31, 0); /* No filtering if 0xFFFF_FFFF */
		writeq(cfg, base + CXL_PMU_FILTER_CFG_REG(hwc->idx, 0));
	}
    ...
}

CXL_PMU_FILTER_CFG_REG has a 4-byte stride, making it a 32-bit hardware
register. Using the 64-bit writeq() here spills into filter 1. If filter 0
is disabled (cfg = 0xFFFFFFFF), writeq() writes 0x00000000 to filter 1,
which would inadvertently enable filter 1 with a 0-match condition.


[Severity: High]
This is a pre-existing issue, but I also noticed a potential undefined
behavior in the overflow handling logic nearby:

drivers/perf/cxl_pmu.c:__cxl_pmu_read() {
    ...
	delta = (new_cnt - prev_cnt) & GENMASK_ULL(info->counter_width - 1, 0);
	if (overflow && delta < GENMASK_ULL(info->counter_width - 1, 0))
		delta += (1UL << info->counter_width);

	local64_add(delta, &event->count);
}

Since delta already accounts for one wraparound via the bitwise AND when
new_cnt is less than prev_cnt, does unconditionally adding 2^width on
overflow cause massive overcounting? 

Also, can (1UL << info->counter_width) cause an out-of-bounds bitwise
shift resulting in undefined behavior? The CXL specification allows
counter_width to be 64, which would shift beyond the width of an unsigned
long, and on 32-bit architectures this could happen for a width of 32 or
greater.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805155911.1304807-1-dave.jiang@intel.com?part=2

Re: [RESEND PATCH v4 08/11] perf/cxl: Unfreeze counters after handling an overflow interrupt

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] A cross-CPU TOCTOU race condition during CPU migration can leave the PMU permanently unfrozen when it is supposed to be disabled, leading to a specification violation where a running counter is written to.

Pre-existing issues:
- [High] `__cxl_pmu_read` is vulnerable to a cross-CPU data race on `hwc->prev_count` during CPU migration because it relies on `local64_cmpxchg`.
- [High] Pending overflow interrupts can be incorrectly attributed to newly scheduled events on the same counter, corrupting their counts by adding a spurious full period.
- [High] Writing 1s to RsvdZ (Reserved, must be zero) bits in the Freeze register violates the CXL specification and risks fatal hardware errors.
- [High] Concurrent PMU reads and overflows cause massive double-counting or loss of events due to stateless overflow handling.
--

--- Patch [8]: [RESEND PATCH v4 08/11] perf/cxl: Unfreeze counters after handling an overflow interrupt ---
commit 77054eb72889cfc4fca68e72d632bb6f5cfbe02d
Author: Dave Jiang <dave.jiang@intel.com>

perf/cxl: Unfreeze counters after handling an overflow interrupt

The counters run with Freeze on Overflow set, so one overflow freezes every
counter in the block. cxl_pmu_irq() reads the overflowed counters and clears
the overflow status but never unfreezes, so everything stays frozen until the
next pmu_enable(). This patch unfreezes after clearing the status, unless the
PMU has been disabled in the meantime.

Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index 448e1da3d59f9..a081fcba69917 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c

[ ... ]

> @@ -604,6 +607,7 @@ static void cxl_pmu_disable(struct pmu *pmu)
>  	struct cxl_pmu_info *info = pmu_to_cxl_pmu_info(pmu);
>  	void __iomem *base = info->base;
>  
> +	WRITE_ONCE(info->enabled, false);
>  	/*
>  	 * Whilst bits above number of counters are RsvdZ
>  	 * they are unlikely to be repurposed given
>  	 * number of counters is allowed to be 64 leaving
>  	 * no reserved bits.  Hence this is only slightly
>  	 * naughty.
>  	 */
>  	writeq(GENMASK_ULL(63, 0), base + CXL_PMU_FREEZE_REG);

[Severity: High]
This isn't a bug introduced by this patch, but does writing a full 64-bit mask
to the freeze register risk fatal hardware errors on strict CXL implementations?

In cxl_pmu_disable(), the driver writes GENMASK_ULL(63, 0) to
CXL_PMU_FREEZE_REG to freeze all counters.

According to the CXL 3.0 specification, bits above the implemented number
of counters (info->num_counters) are RsvdZ, and software is strictly
required to write 0 to these fields.

Writing 1s to reserved fields could cause AER faults or Poisoned transactions.

[ ... ]

> @@ -802,6 +806,21 @@ static irqreturn_t cxl_pmu_irq(int irq, void *data)
>  
>  	writeq(overflowed, base + CXL_PMU_OVERFLOW_REG);
>  
> +	/*
> +	 * An overflow freezes every counter in the CPMU, so unfreeze once the
> +	 * overflowed ones have been read and their status cleared. Otherwise
> +	 * they stay frozen until the next pmu_enable() and events are lost.
> +	 *
> +	 * Not while the PMU is disabled, so as not to undo an intentional freeze.
> +	 * The check is advisory, not exclusive: pmu_disable() normally runs on
> +	 * info->on_cpu with interrupts off, where the pinned handler cannot
> +	 * preempt it. In the one window where it does not - the migration in
> +	 * cxl_pmu_offline_cpu() - the counters are legitimately running again,
> +	 * so unfreezing is correct there anyway.
> +	 */
> +	if (READ_ONCE(info->enabled))

[Severity: High]
Could this advisory check on info->enabled cause a cross-CPU race condition
during CPU migration?

During CPU hotplug migration, perf_pmu_migrate_context() in
cxl_pmu_offline_cpu() moves events to the target CPU, but the IRQ
affinity is not updated until afterwards.

If the IRQ fires on the old CPU, it can read a stale true for info->enabled
concurrently with the target CPU executing cxl_pmu_disable(), which sets
it to false and freezes the PMU.

The old CPU would then write 0 to unfreeze the PMU here. The target CPU,
expecting the PMU to be frozen, proceeds to cxl_pmu_event_start(), enabling
the counter via CXL_PMU_COUNTER_CFG_ENABLE and writing 0 to
CXL_PMU_COUNTER_REG.

Because the PMU was incorrectly unfrozen, the counter starts immediately,
leading to a write to a running counter, which violates the CXL 3.0 spec.

> +		writeq(0, base + CXL_PMU_FREEZE_REG);
> +
>  	return IRQ_HANDLED;
>  }

I also noticed a few other pre-existing issues in this file while reviewing.

[Severity: High]
This isn't a bug introduced by this patch, but is the use of local64_cmpxchg
in __cxl_pmu_read() safe across multiple CPUs?

drivers/perf/cxl_pmu.c:__cxl_pmu_read() {
    ...
    do {
        prev_cnt = local64_read(&hwc->prev_count);
        new_cnt = cxl_pmu_read_counter(event);
    } while (local64_cmpxchg(&hwc->prev_count, prev_cnt, new_cnt) != prev_cnt);
    ...
}

During the CPU hotplug migration window in cxl_pmu_offline_cpu(), events
are migrated to the target CPU before IRQ affinity is updated via
irq_set_affinity().

If an overflow IRQ fires in this window, it runs on the old CPU while the
target CPU can concurrently execute cxl_pmu_read().

Since local64_t only guarantees atomicity for local CPU accesses, both CPUs
iterating over the same event and concurrently executing local64_cmpxchg()
could cause a data race and silent counter corruption.

[Severity: High]
This isn't a bug introduced by this patch, but can pending overflow interrupts
be incorrectly attributed to newly scheduled events?

When a counter overflows, the hardware asserts an interrupt and sets the
corresponding bit in CXL_PMU_OVERFLOW_REG.

If the current event is deleted before the IRQ is serviced, neither
cxl_pmu_event_del() nor cxl_pmu_event_start() clears the overflow bit:

drivers/perf/cxl_pmu.c:cxl_pmu_event_start() {
    ...
    local64_set(&hwc->prev_count, 0);
    writeq(0, base + CXL_PMU_COUNTER_REG(hwc->idx));
    ...
}

When cxl_pmu_event_add() maps a new event to the same counter index, and
the pending IRQ is serviced, cxl_pmu_irq() calls __cxl_pmu_read(event, true).

For the new event, both new_cnt and prev_cnt are near 0, meaning the
following check is satisfied:

drivers/perf/cxl_pmu.c:__cxl_pmu_read() {
    ...
    if (overflow && new_cnt >= prev_cnt)
        delta += mask + 1;
    ...
}

Could this cause the driver to falsely believe a wrap occurred and corrupt
the count by adding a spurious full period?

[Severity: High]
This isn't a bug introduced by this patch, but does the stateless overflow
handling in __cxl_pmu_read() cause double-counting or lost events?

drivers/perf/cxl_pmu.c:__cxl_pmu_read() {
    ...
    delta = (new_cnt - prev_cnt) & mask;
    if (overflow && new_cnt >= prev_cnt)
        delta += mask + 1;
    ...
}

If a polled read (overflow = false) occurs after the counter wraps and
freezes, but before the IRQ handler runs, it adds the correct delta and
updates prev_cnt.

When the IRQ handler subsequently runs, new_cnt and prev_cnt will be equal,
satisfying new_cnt >= prev_cnt.

Wouldn't the IRQ handler then erroneously add a full period on top of the
already correct count? Conversely, if the event is deleted before the IRQ
handler runs, could the full period be permanently lost?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805155911.1304807-1-dave.jiang@intel.com?part=8

Re: [RESEND PATCH v4 09/11] perf/cxl: Validate the hardware-reported counter width

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition between manual perf event reads and the PMU overflow interrupt handler causes spurious full-period additions.
- [High] Missing cleanup of the hardware overflow status bit when stopping or starting a counter leads to massive overcounting for subsequently scheduled events.
--

--- Patch [9]: [RESEND PATCH v4 09/11] perf/cxl: Validate the hardware-reported counter width ---
commit 194e381f35d23ddbc6cc781235e1bc6b38fb673a
Author: Dave Jiang <dave.jiang@intel.com>

perf/cxl: Validate the hardware-reported counter width

cxl_pmu_parse_caps() takes the Counter Width straight from the CPMU
Capability register without a bounds check. Reject a counter width
outside 1..64 at probe to prevent shifting out of range.

Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index a081fcba69917..84ebc7da181d5 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
> @@ -142,6 +142,15 @@ static int cxl_pmu_parse_caps(struct device *dev, struct cxl_pmu_info *info)
>  
>  	info->num_counters = FIELD_GET(CXL_PMU_CAP_NUM_COUNTERS_MSK, val) + 1;
>  	info->counter_width = FIELD_GET(CXL_PMU_CAP_COUNTER_WIDTH_MSK, val);
> +	/*
> +	 * The Counter Data register is 64 bits wide, so a Counter Width of 0 or
> +	 * >64 is invalid. Reject it rather than let GENMASK_ULL(width - 1, 0) in
> +	 * the read path shift out of range.
> +	 */
> +	if (info->counter_width == 0 || info->counter_width > 64) {
> +		dev_err(dev, "Invalid counter width %d\n", info->counter_width);
> +		return -ENODEV;
> +	}
>  	info->num_event_capabilities = FIELD_GET(CXL_PMU_CAP_NUM_EVN_CAP_REG_SUP_MSK, val) + 1;
>  
>  	info->filter_hdm = FIELD_GET(CXL_PMU_CAP_FILTERS_SUP_MSK, val) & CXL_PMU_FILTER_HDM;

[Severity: High]
This is a pre-existing issue, but can a race condition between manual perf
event reads and the PMU overflow interrupt handler cause spurious additions?

drivers/perf/cxl_pmu.c:__cxl_pmu_read() {
    ...
	mask = GENMASK_ULL(info->counter_width - 1, 0);
	delta = (new_cnt - prev_cnt) & mask;
	if (overflow && new_cnt >= prev_cnt)
		delta += mask + 1;
    ...
}

If a hardware counter wraps and asserts the PMU interrupt, and a manual read
occurs before the interrupt handler executes, the manual read handles the wrap
natively via 2's complement subtraction and updates prev_count.

When the IRQ handler subsequently runs, it sees the stale hardware overflow
bit still set and evaluates new_cnt >= prev_cnt. Will it then spuriously
add a full period (mask + 1) to the event count?


[Severity: High]
This is a pre-existing issue, but does missing cleanup of the hardware overflow
status bit when stopping or starting a counter lead to overcounting for
subsequently scheduled events?

drivers/perf/cxl_pmu.c:cxl_pmu_event_stop() {
    ...
	cfg &= ~(FIELD_PREP(CXL_PMU_COUNTER_CFG_INT_ON_OVRFLW, 1) |
		 FIELD_PREP(CXL_PMU_COUNTER_CFG_ENABLE, 1));
	writeq(cfg, base + CXL_PMU_COUNTER_CFG_REG(hwc->idx));
    ...
}

If cxl_pmu_event_stop disables a counter's interrupt but does not clear the
hardware overflow status bit in CXL_PMU_OVERFLOW_REG, the bit remains set.

drivers/perf/cxl_pmu.c:cxl_pmu_event_start() {
    ...
	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_INT_ON_OVRFLW, 1);
	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_FREEZE_ON_OVRFLW, 1);
	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_ENABLE, 1);
    ...
}

When cxl_pmu_event_start later reuses the counter index for a new event, it
initializes the counter to 0 and re-enables the interrupt without clearing the
pending overflow bit.

Could the IRQ handler process the stale overflow bit and incorrectly add mask + 1
to the new event's count?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805155911.1304807-1-dave.jiang@intel.com?part=9

Re: [RESEND PATCH v4 11/11] perf/cxl: Clear stale overflow status before using a counter

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Flawed overflow handling adds phantom counts to events when `cxl_pmu_read()` races with the overflow interrupt.
--

--- Patch [11]: [RESEND PATCH v4 11/11] perf/cxl: Clear stale overflow status before using a counter ---
commit 7eae9f25fd541ecbfdfed643b25d0495c2ec7444
Author: Dave Jiang <dave.jiang@intel.com>
perf/cxl: Clear stale overflow status before using a counter

This commit clears the overflow status register in cxl_pmu_event_start() and
cxl_pmu_probe(). This ensures that a counter handed from one event to the
next does not start with a previous owner's pending overflow, preventing
phantom counts from being added on handover.

Closes: https://lore.kernel.org/linux-cxl/anAVpO--i5heVfz9@MWDK4CY14F/

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
> @@ -683,6 +683,15 @@ static void cxl_pmu_event_start(struct perf_event *event, int flags)
>  	 */
>  	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_THRESHOLD_MSK,
>  			  cxl_pmu_config1_get_threshold(event));
> +
> +	/*
> +	 * Drop any overflow the previous owner of this counter left pending;
> +	 * cxl_pmu_event_stop() does not. Otherwise an interrupt latched over
> +	 * the handover gets charged to this event. Do it before arming Interrupt
> +	 * on Overflow below, and note RW1C leaves the other counters alone.
> +	 */
> +	writeq(BIT_ULL(hwc->idx), base + CXL_PMU_OVERFLOW_REG);

[Severity: High]
This is a pre-existing issue, but while we are addressing phantom counts from
stale overflows here, is there still a race condition between cxl_pmu_read()
and the overflow interrupt that can also result in phantom periods?

If the hardware counter wraps, it sets the overflow bit and raises an MSI.
Before the ISR can run, cxl_pmu_read() might be called (for instance, via a
system call or a periodic timer) while local IRQs are disabled:

cxl_pmu_read()
  __cxl_pmu_read(event, false)

This calculates the correct delta, adds it to the event count, and updates
prev_count to match new_cnt.

Once IRQs are re-enabled, the pending MSI fires. cxl_pmu_irq() runs and calls:

cxl_pmu_irq()
  __cxl_pmu_read(event, true)

Because prev_count was just updated, prev_count and new_cnt are now equal or
nearly equal. In __cxl_pmu_read(), we have:

	if (overflow && new_cnt >= prev_cnt)
		delta += mask + 1;

Since new_cnt is greater than or equal to prev_cnt and the overflow flag is
passed as true by the ISR, does this logic erroneously add a full mask + 1
phantom period to the performance counter?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805155911.1304807-1-dave.jiang@intel.com?part=11

Re: [RESEND PATCH v4 01/11] perf/cxl: Program the requested event group on configurable counters

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing unfreeze of CPMU counters in the IRQ handler causes the PMU to permanently stop counting after the first overflow.
- [High] Incorrect wrap-around arithmetic in `__cxl_pmu_read` double-counts events on overflow and triggers undefined behavior.
--

--- Patch [1]: [RESEND PATCH v4 01/11] perf/cxl: Program the requested event group on configurable counters ---
commit eb034a4b2ed07ff56c863ab7711fcb1eb6012a8b
Author: Dave Jiang <dave.jiang@intel.com>

perf/cxl: Program the requested event group on configurable counters

cxl_pmu_get_event_idx() sets *counter_idx for a configurable counter but
never *event_idx. hwc->event_base stays 0, so cxl_pmu_event_start() programs
event group 0 no matter which group the user asked for. This commit fixes the
issue by returning the matched capability's event_idx.

Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index 68a54d97d2a8a..ff01b658e1b45 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c

[ ... ]

> @@ -541,7 +544,7 @@ static int cxl_pmu_get_event_idx(struct perf_event *event, int *counter_idx,
>  
>  	pmu_ev = cxl_pmu_find_config_counter_ev_cap(info, vid, gid, mask);
>  	if (!IS_ERR(pmu_ev)) {
> -		if (!counter_idx)
> +		if (!counter_idx || !event_idx)
>  			return 0;
>  
>  		bitmap_andnot(configurable_and_free, info->conf_counter_bm,

While reviewing this patch, I noticed a couple of pre-existing issues in
this file:

drivers/perf/cxl_pmu.c:__cxl_pmu_read() {
    ...
	delta = (new_cnt - prev_cnt) & GENMASK_ULL(info->counter_width - 1, 0);
	if (overflow && delta < GENMASK_ULL(info->counter_width - 1, 0))
		delta += (1UL << info->counter_width);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this wrap-around
arithmetic double-count events on overflow and trigger undefined behavior?

Because the wrap-around is already accounted for in delta during the
unsigned subtraction, adding 2^width erroneously double-counts the
overflow, injecting an enormous spurious value into event->count every
time an interrupt fires.

Furthermore, if the hardware specifies a 64-bit counter
(info->counter_width == 64), evaluating 1UL << 64 invokes undefined
behavior in C.


drivers/perf/cxl_pmu.c:cxl_pmu_irq() {
    ...
		__cxl_pmu_read(event, true);
	}

	writeq(overflowed, base + CXL_PMU_OVERFLOW_REG);

	return IRQ_HANDLED;
}

[Severity: High]
This is a pre-existing issue, but does this interrupt handler need to write 0
to CXL_PMU_FREEZE_REG to unfreeze the counters?

In cxl_pmu_event_start(), the configuration sets
CXL_PMU_COUNTER_CFG_FREEZE_ON_OVRFLW. According to the CXL 3.0
specification, when this is set, an overflow causes the hardware to
freeze counters by setting bits in the Freeze Register.

The IRQ handler clears the overflow register (CXL_PMU_OVERFLOW_REG) but
fails to explicitly clear the freeze register (CXL_PMU_FREEZE_REG).
Consequently, the counters remain permanently frozen and stop generating
events after processing the first overflow.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805155911.1304807-1-dave.jiang@intel.com?part=1

Re: [RESEND PATCH v4 08/11] perf/cxl: Unfreeze counters after handling an overflow interrupt

[Dave Jiang]

On 8/5/26 9:15 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> 
> New issues:
> - [High] A cross-CPU TOCTOU race condition during CPU migration can leave the PMU permanently unfrozen when it is supposed to be disabled, leading to a specification violation where a running counter is written to.

The window is real but the spec conclusion is not. CXL r4.0 8.2.7.1.1,
Counters Writable while Frozen, ties writability to enabled or frozen,
not to running:

  1 = ... software is permitted to write and modify any Fixed-function
      Counter Data register or any Configurable Counter Data register while
      it is frozen.

cxl_pmu_parse_caps() refuses to probe unless that bit is set, so the write
event_start() does is explicitly permitted. The stray unfreeze takes away
the frozen state, which the grant does not cover. We get a garbage initial
count on one event, on a CPU that is going away, and the destination's
pmu_enable() unfreezes a moment later anyway.

A lock would close the gap between the flag read and the freeze write, but
the handler still races event_start() on the other CPU, so it narrows the
race rather than removing it.

The real fix is not depending on same-CPU semantics at all. Moving
prev_count and event->count off local64_t makes this class of question go
away. Needs follow changes if that is the direction we want to go.

DJ

> 
> Pre-existing issues:
> - [High] `__cxl_pmu_read` is vulnerable to a cross-CPU data race on `hwc->prev_count` during CPU migration because it relies on `local64_cmpxchg`.
> - [High] Pending overflow interrupts can be incorrectly attributed to newly scheduled events on the same counter, corrupting their counts by adding a spurious full period.
> - [High] Writing 1s to RsvdZ (Reserved, must be zero) bits in the Freeze register violates the CXL specification and risks fatal hardware errors.
> - [High] Concurrent PMU reads and overflows cause massive double-counting or loss of events due to stateless overflow handling.
> --
> 
> --- Patch [8]: [RESEND PATCH v4 08/11] perf/cxl: Unfreeze counters after handling an overflow interrupt ---
> commit 77054eb72889cfc4fca68e72d632bb6f5cfbe02d
> Author: Dave Jiang <dave.jiang@intel.com>
> 
> perf/cxl: Unfreeze counters after handling an overflow interrupt
> 
> The counters run with Freeze on Overflow set, so one overflow freezes every
> counter in the block. cxl_pmu_irq() reads the overflowed counters and clears
> the overflow status but never unfreezes, so everything stays frozen until the
> next pmu_enable(). This patch unfreezes after clearing the status, unless the
> PMU has been disabled in the meantime.
> 
> Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1
> 
>> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
>> index 448e1da3d59f9..a081fcba69917 100644
>> --- a/drivers/perf/cxl_pmu.c
>> +++ b/drivers/perf/cxl_pmu.c
> 
> [ ... ]
> 
>> @@ -604,6 +607,7 @@ static void cxl_pmu_disable(struct pmu *pmu)
>>  	struct cxl_pmu_info *info = pmu_to_cxl_pmu_info(pmu);
>>  	void __iomem *base = info->base;
>>  
>> +	WRITE_ONCE(info->enabled, false);
>>  	/*
>>  	 * Whilst bits above number of counters are RsvdZ
>>  	 * they are unlikely to be repurposed given
>>  	 * number of counters is allowed to be 64 leaving
>>  	 * no reserved bits.  Hence this is only slightly
>>  	 * naughty.
>>  	 */
>>  	writeq(GENMASK_ULL(63, 0), base + CXL_PMU_FREEZE_REG);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does writing a full 64-bit mask
> to the freeze register risk fatal hardware errors on strict CXL implementations?
> 
> In cxl_pmu_disable(), the driver writes GENMASK_ULL(63, 0) to
> CXL_PMU_FREEZE_REG to freeze all counters.
> 
> According to the CXL 3.0 specification, bits above the implemented number
> of counters (info->num_counters) are RsvdZ, and software is strictly
> required to write 0 to these fields.
> 
> Writing 1s to reserved fields could cause AER faults or Poisoned transactions.
> 
> [ ... ]
> 
>> @@ -802,6 +806,21 @@ static irqreturn_t cxl_pmu_irq(int irq, void *data)
>>  
>>  	writeq(overflowed, base + CXL_PMU_OVERFLOW_REG);
>>  
>> +	/*
>> +	 * An overflow freezes every counter in the CPMU, so unfreeze once the
>> +	 * overflowed ones have been read and their status cleared. Otherwise
>> +	 * they stay frozen until the next pmu_enable() and events are lost.
>> +	 *
>> +	 * Not while the PMU is disabled, so as not to undo an intentional freeze.
>> +	 * The check is advisory, not exclusive: pmu_disable() normally runs on
>> +	 * info->on_cpu with interrupts off, where the pinned handler cannot
>> +	 * preempt it. In the one window where it does not - the migration in
>> +	 * cxl_pmu_offline_cpu() - the counters are legitimately running again,
>> +	 * so unfreezing is correct there anyway.
>> +	 */
>> +	if (READ_ONCE(info->enabled))
> 
> [Severity: High]
> Could this advisory check on info->enabled cause a cross-CPU race condition
> during CPU migration?
> 
> During CPU hotplug migration, perf_pmu_migrate_context() in
> cxl_pmu_offline_cpu() moves events to the target CPU, but the IRQ
> affinity is not updated until afterwards.
> 
> If the IRQ fires on the old CPU, it can read a stale true for info->enabled
> concurrently with the target CPU executing cxl_pmu_disable(), which sets
> it to false and freezes the PMU.
> 
> The old CPU would then write 0 to unfreeze the PMU here. The target CPU,
> expecting the PMU to be frozen, proceeds to cxl_pmu_event_start(), enabling
> the counter via CXL_PMU_COUNTER_CFG_ENABLE and writing 0 to
> CXL_PMU_COUNTER_REG.
> 
> Because the PMU was incorrectly unfrozen, the counter starts immediately,
> leading to a write to a running counter, which violates the CXL 3.0 spec.
> 
>> +		writeq(0, base + CXL_PMU_FREEZE_REG);
>> +
>>  	return IRQ_HANDLED;
>>  }
> 
> I also noticed a few other pre-existing issues in this file while reviewing.
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but is the use of local64_cmpxchg
> in __cxl_pmu_read() safe across multiple CPUs?
> 
> drivers/perf/cxl_pmu.c:__cxl_pmu_read() {
>     ...
>     do {
>         prev_cnt = local64_read(&hwc->prev_count);
>         new_cnt = cxl_pmu_read_counter(event);
>     } while (local64_cmpxchg(&hwc->prev_count, prev_cnt, new_cnt) != prev_cnt);
>     ...
> }
> 
> During the CPU hotplug migration window in cxl_pmu_offline_cpu(), events
> are migrated to the target CPU before IRQ affinity is updated via
> irq_set_affinity().
> 
> If an overflow IRQ fires in this window, it runs on the old CPU while the
> target CPU can concurrently execute cxl_pmu_read().
> 
> Since local64_t only guarantees atomicity for local CPU accesses, both CPUs
> iterating over the same event and concurrently executing local64_cmpxchg()
> could cause a data race and silent counter corruption.
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but can pending overflow interrupts
> be incorrectly attributed to newly scheduled events?
> 
> When a counter overflows, the hardware asserts an interrupt and sets the
> corresponding bit in CXL_PMU_OVERFLOW_REG.
> 
> If the current event is deleted before the IRQ is serviced, neither
> cxl_pmu_event_del() nor cxl_pmu_event_start() clears the overflow bit:
> 
> drivers/perf/cxl_pmu.c:cxl_pmu_event_start() {
>     ...
>     local64_set(&hwc->prev_count, 0);
>     writeq(0, base + CXL_PMU_COUNTER_REG(hwc->idx));
>     ...
> }
> 
> When cxl_pmu_event_add() maps a new event to the same counter index, and
> the pending IRQ is serviced, cxl_pmu_irq() calls __cxl_pmu_read(event, true).
> 
> For the new event, both new_cnt and prev_cnt are near 0, meaning the
> following check is satisfied:
> 
> drivers/perf/cxl_pmu.c:__cxl_pmu_read() {
>     ...
>     if (overflow && new_cnt >= prev_cnt)
>         delta += mask + 1;
>     ...
> }
> 
> Could this cause the driver to falsely believe a wrap occurred and corrupt
> the count by adding a spurious full period?
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does the stateless overflow
> handling in __cxl_pmu_read() cause double-counting or lost events?
> 
> drivers/perf/cxl_pmu.c:__cxl_pmu_read() {
>     ...
>     delta = (new_cnt - prev_cnt) & mask;
>     if (overflow && new_cnt >= prev_cnt)
>         delta += mask + 1;
>     ...
> }
> 
> If a polled read (overflow = false) occurs after the counter wraps and
> freezes, but before the IRQ handler runs, it adds the correct delta and
> updates prev_cnt.
> 
> When the IRQ handler subsequently runs, new_cnt and prev_cnt will be equal,
> satisfying new_cnt >= prev_cnt.
> 
> Wouldn't the IRQ handler then erroneously add a full period on top of the
> already correct count? Conversely, if the event is deleted before the IRQ
> handler runs, could the full period be permanently lost?
> 

Re: [RESEND PATCH v4 06/11] cxl/pci: Add the PMUs after configuring events

[Alison Schofield]

On Wed, Aug 05, 2026 at 08:59:06AM -0700, Dave Jiang wrote:
> cxl_pci_probe() adds the CPMU instances before configuring the event
> interrupts. Both take an MSI/MSI-X vector the device names in its own
> capability registers, and the spec lets a device point several capabilities
> at one message number, so the two can collide.
> 
> They do not fail the same way. A failed devm_cxl_pmu_add() only breaks out
> of the loop, while a failed cxl_event_config() aborts probe and the memory
> device never appears. Going PMUs-first therefore lets a peripheral facility
> take the vector and deny it to one that matters.
> 
> Configure events first, so a collision costs the PMU rather than the
> memdev. Return 0 explicitly afterwards, since rc still holds any PMU error
> that cxl_event_config() used to overwrite on the way past.

Reviewed-by: Alison Schofield <alison.schofield@intel.com>