[PATCH v3 0/9] perf/cxlpmu: Misc sashiko raised issues fixes

[PATCH v3 0/9] perf/cxlpmu: Misc sashiko raised issues fixes

[Dave Jiang]

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 v2:
- 1/9: check event_idx alongside counter_idx (Jonathan).
- 2/9: use FIELD_MODIFY() rather than mask-then-OR, for the four fields
  that can carry a stale value (Jonathan).
- 3/9: rewrite the rationale, no code change (Jonathan).
- 4/9: new patch, split the MSI vector out of info->irq (Robin, Jonathan).
- 5/9: was 4/9, now tests info->msi_vec. Fix unchanged.
- 6/9: was 5/9, retitled. Drop IRQF_SHARED as well as adding
  IRQF_NOBALANCING (Jonathan, Robin). Review tags dropped as the patch
  grew.
- 7/9: was 6/9. Don't unfreeze if the PMU has been disabled meanwhile
  (Robin, Jonathan). Review tag dropped as the patch grew a hunk.
- 8/9: was 7/9, unchanged.
- 9/9: was 8/9, retitled. Drop the cxl_pmu_offline_cpu() hunk, and clear
  the overflow status at probe so a stale bit from firmware cannot reach
  the handler (Robin). Ack dropped as the patch grew a hunk.
- dropped: the old 9/9 (cpumask_of(-1) guard).

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

Two 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
that moot and Robin may have a view on whether it is worth it. And the
on_cpu transient plus dead branch above want the same patch, possibly on
top of the generic PMU hotplug rework Jonathan pointed at [2].

[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 (9):
  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: Split the MSI vector out of info->irq
  perf/cxl: Accept an overflow interrupt on MSI message number 0
  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

 drivers/perf/cxl_pmu.c | 88 +++++++++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 23 deletions(-)


base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff
-- 
2.55.0

[PATCH v3 1/9] 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>
---
v3:
- Check event_idx alongside counter_idx at the top of the scope. (Jonathan)
---
 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.55.0

Re: [PATCH v3 1/9] perf/cxl: Program the requested event group on configurable counters

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Counter configuration fields are not cleared before being modified in cxl_pmu_event_start(), causing corrupted event group IDs when a counter is reused.
--

--- Patch [1]: [PATCH v3 1/9] perf/cxl: Program the requested event group on configurable counters ---
commit 8ade5d557a8fda7df4834e1f36fcc4dfd34e953d
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.

Return 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,
> @@ -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;

[Severity: Medium]
Will this corrupt the hardware counter configuration when a counter is reused?

By populating event_idx here, hwc->event_base will now contain the actual
event group ID rather than 0.

However, in cxl_pmu_event_start(), the configuration is updated using a
read-modify-write without clearing the previous fields:

drivers/perf/cxl_pmu.c:cxl_pmu_event_start() {
    ...
    cfg = readq(base + CXL_PMU_COUNTER_CFG_REG(hwc->idx));
    ...
	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));
	}
    ...
    writeq(cfg, base + CXL_PMU_COUNTER_CFG_REG(hwc->idx));
}

If a counter is freed and reused for a new event with a different event group
ID, won't the bitwise OR merge the old and new event_base values since the
CXL_PMU_COUNTER_CFG_EVENT_GRP_ID_IDX_MSK bits are never cleared from cfg?

>  		return 0;
>  	}
>

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

[PATCH v3 2/9] 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.

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>
---
v3:
- Use FIELD_MODIFY() instead of masking then ORing (Jonathan).
---
 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.55.0

Re: [PATCH v3 2/9] perf/cxl: Clear stale event fields before reprogramming a counter

[sashiko-bot]

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

Pre-existing issues:
- [High] writeq() used on 32-bit CXL PMU Filter Config registers clobbers the adjacent Filter 1 configuration with zeros.
- [High] Pending hardware overflows from stopped events are erroneously attributed to newly started events on the same counter, causing massive spurious counts.
- [High] Two's complement arithmetic misunderstanding causes the driver to overcount by exactly 2^width on almost every overflow interrupt.
--

--- Patch [2]: [PATCH v3 2/9] perf/cxl: Clear stale event fields before reprogramming a counter ---
commit e2d9388d1e46f8effd5efea9988cf263ff2efd2c
Author: Dave Jiang <dave.jiang@intel.com>

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

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.

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 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)

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem earlier in
cxl_pmu_event_start():

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);
        writeq(cfg, base + CXL_PMU_FILTER_CFG_REG(hwc->idx, 0));
    ...
}

Does using the 64-bit writeq() here overwrite the adjacent Filter 1
configuration register at offset +4 with the upper 32 bits of cfg (which are
zeros)? This might cause the PMU hardware to silently drop events that do not
match 0 for Filter 1's property, breaking PMU counting for this counter.

>  	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 I spotted a potential issue with
hardware overflow attribution when stopping and starting events.

If a counter overflows right before being stopped, the hardware sets the
overflow bit and raises an interrupt, but the pending overflow bit in
CXL_PMU_OVERFLOW_REG is not cleared:

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));
    ...
}

When a new event is scheduled onto the same counter via cxl_pmu_event_start()
and the interrupt fires, will the ISR attribute the overflow to the newly
started event?

drivers/perf/cxl_pmu.c:cxl_pmu_irq() {
    ...
    for_each_set_bit(i, overflowedbm, info->num_counters) {
        struct perf_event *event = info->hw_events[i];
        if (!event)
            continue;
        __cxl_pmu_read(event, true);
    }
    ...
}

Could this cause massive spurious event counts to be falsely attributed to new
events during normal perf multiplexing?


[Severity: High]
This is a pre-existing issue, but does __cxl_pmu_read() overcount on overflow
interrupts?

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);
    ...
}

Because these are unsigned 64-bit integers, the masked subtraction natively
accounts for exactly one counter wrap, yielding the correct positive event
delta. 

Since delta is already correct, does adding (1UL << info->counter_width) on top
of it cause the driver to overcount by exactly 2^width on almost every overflow
interrupt?

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

[PATCH v3 3/9] 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. That 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 whether the subtraction needed the period. What decides
that is where prev_count sits when the counter wraps:

  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 overflow 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 instead, the one case the
subtraction cannot express. Dropping it outright would break the first row.
The residual r need not be 0 either, since some events increment by more
than 1 per cycle (CXL r4.0 8.2.7.2.1, Threshold), and keying off the
operands stays exact whatever r is.

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>
---
v3:
- Rewrite the rationale. v2 blamed the counter counting on past the wrap,
  which the spec permits but which was not what made the old code wrong
  (Jonathan).
- Note that events incrementing by more than 1 per cycle make a non-zero
  residual reachable, so the fixup has to hold for any residual (Jonathan).
- Drop the claim that the old code loses a period on the first overflow
  after event_start(). It does not. The subtraction yields 0 there and the
  old guard then adds the period back correctly. The reachable bug is the
  over-count in the other direction.
- No code change.
---
 drivers/perf/cxl_pmu.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index b16e2e4090a3..3511d049aea4 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,17 @@ 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 exactly 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 distinguishes them, and new_cnt >=
+	 * prev_cnt is that case, so add the period back. mask + 1 is
+	 * 2^counter_width, which is 0 for a 64-bit counter - the right value to
+	 * add once the top bit is gone, and no 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.55.0

Re: [PATCH v3 3/9] perf/cxl: Fix the counter overflow delta fixup

[sashiko-bot]

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

Pre-existing issues:
- [Medium] A race condition between a polled read and the overflow IRQ handler causes a double-counting of a full period if the polled read executes after the counter wraps but before the IRQ handler.
- [Medium] Missing validation for `info->counter_width` allows a non-compliant or buggy CXL device to trigger Undefined Behavior when `GENMASK_ULL` is evaluated.
--

--- Patch [3]: [PATCH v3 3/9] perf/cxl: Fix the counter overflow delta fixup ---
commit 333d6be6b59d292e876b5c4888ab33d0abc8cc03
Author: Dave Jiang <dave.jiang@intel.com>

perf/cxl: Fix the counter overflow delta fixup

This commit alters the CXL PMU counter overflow logic to condition the fixup
on new_cnt >= prev_cnt, aiming to correctly handle cases where the subtraction
cannot express the span over a period boundary, while accommodating events
that increment by more than 1 per cycle. It also modifies the period addition
to use mask + 1 to avoid undefined behavior on 64-bit counters.

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 b16e2e4090a39..3511d049aea40 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);

[ ... ]

>  	/*
> -	 * If we know an overflow occur then take that into account.
> -	 * Note counter is not reset as that would lose events
> +	 * The mask discards exactly 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 distinguishes them, and new_cnt >=
> +	 * prev_cnt is that case, so add the period back. mask + 1 is
> +	 * 2^counter_width, which is 0 for a 64-bit counter - the right value to
> +	 * add once the top bit is gone, and no 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);

[Severity: Medium]
This is a pre-existing issue, but could a buggy or malicious CXL device
reporting a counter width of 0 (or > 64) trigger Undefined Behavior here?

If the device reports 0 for the counter width in the capability registers,
the driver reads this into info->counter_width without bounds validation.

When GENMASK_ULL(info->counter_width - 1, 0) is evaluated, it results in a
negative shift amount which leads to Undefined Behavior and corrupted PMU
masks.

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

[Severity: Medium]
This isn't a bug introduced by this patch, but does this sequence allow a 
race condition between a polled read and the overflow IRQ handler to
double-count a full period?

If the hardware counter wraps, setting the overflow bit, a polled read
(such as via perf stat -I) might read the counter before the hardware IRQ
is delivered.

The polled read would compute the delta, correctly including the period
because new_cnt is wrapped and smaller than prev_cnt, and then update
prev_count to the small post-wrap value.

When the delayed overflow IRQ handler subsequently runs, it reads the
overflow bit and calls __cxl_pmu_read(event, true). Because prev_cnt is
now the small post-wrap value, new_cnt >= prev_cnt evaluates to true.

The handler would then execute delta += mask + 1, incorrectly adding a
second full period because it assumes the wrap wasn't already consumed.

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

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

[PATCH v3 4/9] 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().

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

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- New patch
---
 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 3511d049aea4..56e6ae7bebf7 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++) {
@@ -874,10 +875,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.55.0

Re: [PATCH v3 4/9] perf/cxl: Split the MSI vector out of info->irq

[sashiko-bot]

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

Pre-existing issues:
- [High] The check `info->msi_vec <= 0` incorrectly rejects valid MSI vector index 0.
- [High] Incorrect registration order of the PMU and CPU hotplug instance leads to a Use-After-Free or NULL pointer dereference during hotplug events.
- [High] PMU hardware is not frozen during CPU hotplug migration, and the interrupt is shared, leading to concurrent SMP execution of `local64_t` operations without SMP safety.
- [High] The driver dynamically changes the CPU affinity of an interrupt requested with IRQF_SHARED, forcibly breaking affinity for other devices sharing the IRQ.
- [Medium] The PMU firmware's counter width is used without validation, which can cause undefined behavior via out-of-bounds bit shifts.
- [Medium] Silent truncation of user-provided PMU threshold values.
--

--- Patch [4]: [PATCH v3 4/9] perf/cxl: Split the MSI vector out of info->irq ---
commit ad9898396611ac9af6c4819fa48aba52e6dbeb08
Author: Dave Jiang <dave.jiang@intel.com>
Subject: perf/cxl: Split the MSI vector out of info->irq

The commit separates the MSI/MSI-X message number from the Linux virq by
introducing a new msi_vec field in struct cxl_pmu_info. This clarifies
the use of the field during probe before it gets overwritten with the virq.

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index 3511d049aea40..56e6ae7bebf7c 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c

[ ... ]

> @@ -874,10 +875,10 @@ static int cxl_pmu_probe(struct device *dev)
>  		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
>  	};
>  
> -	if (info->irq <= 0)
> +	if (info->msi_vec <= 0)

[Severity: High]
This is a pre-existing issue, but does this incorrectly reject a valid MSI
vector index of 0?

The CXL PMU capability register provides a 0-based MSI vector number (0-15).
If the firmware specifies MSI message number 0, info->msi_vec becomes 0.
cxl_pmu_probe() then erroneously aborts with -EINVAL, causing valid
devices using MSI vector 0 to fail probing completely.

>  		return -EINVAL;
>  
> -	rc = pci_irq_vector(pdev, info->irq);
> +	rc = pci_irq_vector(pdev, info->msi_vec);
>  	if (rc < 0)
>  		return rc;
>  	irq = rc;

I also noticed a few other pre-existing issues in this file:

[Severity: High]
This isn't a bug introduced by this patch, but could the PMU registration
order cause a NULL pointer dereference or use-after-free during hotplug?

In cxl_pmu_probe(), cpuhp_state_add_instance() is called before
perf_pmu_register(). If a CPU hotplug event triggers immediately:

cxl_pmu_offline_cpu()
  perf_pmu_migrate_context()

Since perf_pmu_register() hasn't allocated pmu->pmu_cpu_context yet,
could this result in a NULL pointer dereference? 

Furthermore, upon module unload, devm executes teardown in reverse order.
perf_pmu_unregister() is called before removing the hotplug instance,
creating a window where a CPU offline event could trigger a use-after-free
on the unregistered PMU context.

[Severity: High]
This is a pre-existing issue, but is it safe to use local64_cmpxchg() when
the PMU interrupt is shared and not disabled during migration?

cxl_pmu_offline_cpu() migrates PMU events without disabling the PMU hardware.
Because the interrupt is shared (IRQF_SHARED), it can fire on any CPU at any
time due to other devices sharing the line:

cxl_pmu_irq()
  __cxl_pmu_read()
    local64_cmpxchg()

Concurrently, the new CPU can start or read events, also invoking
__cxl_pmu_read(). Since local64_t operations are only safe for single-CPU
access, could cross-CPU concurrent access corrupt the performance counters?

[Severity: Medium]
This isn't a bug introduced by this patch, but could an unvalidated counter
width from firmware cause undefined behavior?

In cxl_pmu_parse_caps(), info->counter_width is read directly from the hardware
capability register. If firmware reports a width of 0, __cxl_pmu_read()
computes GENMASK_ULL(0 - 1, 0).

Since 0 - 1 promotes to an int value of -1, passing it to GENMASK_ULL
evaluates to a negative bit shift (~0ULL >> 64), which is undefined behavior.

[Severity: High]
This is a pre-existing issue, but does this driver dynamically alter the
affinity of a shared interrupt?

cxl_pmu_probe() requests the PMU interrupt with the IRQF_SHARED flag.
However, during CPU hotplug events, cxl_pmu_online_cpu() and
cxl_pmu_offline_cpu() unconditionally call irq_set_affinity() to bind the
interrupt to a specific CPU.

Dynamically changing the hardware affinity of a shared interrupt can reroute
interrupts for all other unrelated devices sharing that IRQ line, violating
system isolation and potentially causing performance regressions or timeouts
for other drivers.

[Severity: Medium]
This isn't a bug introduced by this patch, but does the configuration
accessor truncate user-provided threshold values?

The format attribute threshold is exposed via sysfs as config1:0-15 (16 bits).
However, cxl_pmu_config1_get_threshold() uses a u8 return type:

static u8 cxl_pmu_config1_get_threshold(struct perf_event *event)
{
	return FIELD_GET(CXL_PMU_ATTR_CONFIG1_THRESHOLD_MSK, event->attr.config1);
}

This silently truncates any 16-bit user input to 8 bits, resulting in the PMU
being configured with an incorrect threshold without returning any error to
userspace.

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

[PATCH v3 5/9] perf/cxl: Accept an overflow interrupt on MSI message number 0

[Dave Jiang]

cxl_pmu_probe() rejects the PMU when info->msi_vec <= 0, but that field
holds the MSI/MSI-X message number the device signals overflow on. The
field 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>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Rebase on the preceding patch, so this tests info->msi_vec rather than
  info->irq. The fix is unchanged and the hunk is still one line.
- Not guarding against pci_irq_vector()'s INTx fallback for vector 0, which
  it returns when neither MSI nor MSI-X is enabled. A v3 draft added a
  pci_dev_msi_enabled() check for that, but a modern CXL device wiring up
  INTx is not a realistic prospect, and the consequence otherwise is a PMU
  that counts nothing rather than anything unsafe. Left alone to keep this
  a one-liner.
---
 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 56e6ae7bebf7..c9e30cb149df 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -875,7 +875,7 @@ static int cxl_pmu_probe(struct device *dev)
 		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
 	};
 
-	if (info->msi_vec <= 0)
+	if (info->msi_vec < 0)
 		return -EINVAL;
 
 	rc = pci_irq_vector(pdev, info->msi_vec);
-- 
2.55.0

[PATCH v3 6/9] 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 the
interrupt to another CPU. cxl_pmu_irq() then runs local64_cmpxchg() and
local64_add() on hwc->prev_count and event->count there, at the same time
as the managing CPU. local64_t is only atomic against same-CPU access, so
the counts get corrupted.

IRQF_NOBALANCING on its own does not fix that while the line is shared.
__setup_irq() only acts on the flag 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 affinity points, running our handler on the
wrong CPU.

Drop IRQF_SHARED and add IRQF_NOBALANCING. The spec only recommends that a
component give each CPMU instance a distinct Interrupt Message Number (CXL
r4.0 8.2.7.1.1), so a device may put several on one vector. Such a device
now fails to add the second CPMU instead of silently miscounting both.

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>
---
v3:
- Drop IRQF_SHARED as well, and retitle (Jonathan, Robin).
---
 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 c9e30cb149df..6fdc66a01fb6 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -784,7 +784,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;
 
@@ -887,7 +887,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.55.0

Re: [PATCH v3 6/9] perf/cxl: Don't share the overflow interrupt, and keep it pinned

[sashiko-bot]

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

Pre-existing issues:
- [High] CPU hotplug migration race causes cross-CPU data corruption on in-flight interrupts.
--

--- Patch [6]: [PATCH v3 6/9] perf/cxl: Don't share the overflow interrupt, and keep it pinned ---
commit 0c52a628eb5f3fcd46d4c3f0c11a1448687ffd14
Author: Dave Jiang <dave.jiang@intel.com>

perf/cxl: Don't share the overflow interrupt, and keep it pinned

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 the
interrupt to another CPU. cxl_pmu_irq() then runs local64_cmpxchg() and
local64_add() on hwc->prev_count and event->count there, at the same time
as the managing CPU. local64_t is only atomic against same-CPU access, so
the counts get corrupted.

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 c9e30cb149dfa..6fdc66a01fb67 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c

[ ... ]

> @@ -784,7 +784,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;

[Severity: High]
This is a pre-existing issue, but can this IRQ handler race with CPU hotplug
migration and cause cross-CPU data corruption?

Looking at cxl_pmu_offline_cpu(), the context is migrated to a new CPU before
the IRQ affinity is updated:

cxl_pmu_offline_cpu() {
    ...
    perf_pmu_migrate_context(&info->pmu, cpu, target);
    info->on_cpu = target;
    /*
     * CPU HP lock is held so we should be guaranteed that this CPU hasn't
     * yet gone away.
     */
    WARN_ON(irq_set_affinity(info->irq, cpumask_of(target)));
    ...
}

During installation on the new CPU, the hardware counters and interrupts are
re-enabled. If the hardware generates a PMU overflow interrupt in this
window before irq_set_affinity() completes, the interrupt is delivered to
the old CPU.

The old CPU's IRQ handler (cxl_pmu_irq()) will then see the newly installed
event in the globally shared info->hw_events[i] array:

cxl_pmu_irq() {
    ...
	for_each_set_bit(i, overflowedbm, info->num_counters) {
		struct perf_event *event = info->hw_events[i];
    ...
		__cxl_pmu_read(event, true);
	}
    ...
}

This causes the old CPU to execute non-atomic local64_cmpxchg() and
local64_add() operations concurrently with the new managing CPU. Since
local64_t operations are only safe for same-CPU access, won't this corrupt
the hardware event counts?

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

[PATCH v3 7/9] 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). cxl_pmu_irq() reads the
overflowed counters and clears the overflow status, but never unfreezes.
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, and
cxl_pmu_offline_cpu() migrates the context before it moves the interrupt's
affinity, so an overflow can still land on the old CPU while the new one
reprograms. Track the enabled state and leave the block frozen in that
case. pmu_enable() unfreezes anyway.

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>
---
v3:
- Don't unfreeze if the PMU has been disabled (Robin, Jonathan). Review tag
  dropped as the patch grew a hunk.
- Robin also asked whether this can release a counter that overflowed after
  the status read. It cannot. Global Freeze on Overflow stops every
  non-free-running counter and they stay frozen until software unfreezes
  them, so nothing the driver owns can advance, let alone newly overflow,
  between the readq and here. Two counters overflowing together both show
  up in the same pass. Only free-running counters move while frozen, and
  the driver never programs those.
- The enabled check is not a full exclusion, since the flag is read outside
  any lock. Say so in the comment rather than implying otherwise. A lock
  would buy nothing: pmu_disable() normally runs on the pinned CPU with
  interrupts off, and in the one window where it does not - the migration
  in cxl_pmu_offline_cpu() - event_stop() has already cleared Counter
  Enable, so a stray unfreeze resumes nothing (sashiko-bot).
---
 drivers/perf/cxl_pmu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index 6fdc66a01fb6..580a75bc9210 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
@@ -803,6 +807,9 @@ static irqreturn_t cxl_pmu_irq(int irq, void *data)
 
 	writeq(overflowed, base + CXL_PMU_OVERFLOW_REG);
 
+	if (READ_ONCE(info->enabled))
+		writeq(0, base + CXL_PMU_FREEZE_REG);
+
 	return IRQ_HANDLED;
 }
 
-- 
2.55.0

Re: [PATCH v3 7/9] perf/cxl: Unfreeze counters after handling an overflow interrupt

[sashiko-bot]

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

Pre-existing issues:
- [High] Cross-CPU data race on local64_t variables and spurious counter increments during CPU hotplug migration.
- [High] Unregistering the PMU before removing the CPU hotplug instance creates a Use-After-Free window during driver removal.
--

--- Patch [7]: [PATCH v3 7/9] perf/cxl: Unfreeze counters after handling an overflow interrupt ---
commit d6decd45c5a5019eddbbf261ba3f712c1ad75535
Author: Dave Jiang <dave.jiang@intel.com>

perf/cxl: Unfreeze counters after handling an overflow interrupt

This patch ensures CXL PMU counters are unfrozen after clearing an overflow
interrupt. Previously, counters remained frozen until the next pmu_enable(),
causing events to be lost during that window. It tracks the PMU enabled
state to avoid unfreezing if the PMU was disabled concurrently.

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 6fdc66a01fb67..580a75bc9210a 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c

[ ... ]

> @@ -803,6 +807,9 @@ static irqreturn_t cxl_pmu_irq(int irq, void *data)
>  
>  	writeq(overflowed, base + CXL_PMU_OVERFLOW_REG);
>  
> +	if (READ_ONCE(info->enabled))
> +		writeq(0, base + CXL_PMU_FREEZE_REG);
> +
>  	return IRQ_HANDLED;
>  }

[Severity: High]
This is a pre-existing issue, but can this sequence corrupt performance
counters during CPU hotplug?

Looking at cxl_pmu_offline_cpu(), perf_pmu_migrate_context() is called
before the IRQ affinity is updated:

drivers/perf/cxl_pmu.c:cxl_pmu_offline_cpu() {
	...
	perf_pmu_migrate_context(&info->pmu, cpu, target);
	info->on_cpu = target;
	/*
	 * CPU HP lock is held so we should be guaranteed that this CPU hasn't
	 * yet gone away.
	 */
	WARN_ON(irq_set_affinity(info->irq, cpumask_of(target)));
	...
}

If an overflow MSI is delivered to the old CPU in this narrow window,
cxl_pmu_irq() runs on the old CPU. If the new CPU has already re-added and
started the event, it has cleared hwc->prev_count to 0.

Because the hardware overflow register is only cleared by the IRQ handler,
the old CPU sees a stale overflow flag and invokes __cxl_pmu_read(). The
condition new_cnt >= prev_cnt passes (since prev_cnt is 0), causing it to
erroneously add a full period (mask + 1) to the counter:

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);

	/* ... */
	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);
}

Does this also cause a data race since local64_t operations lack cross-CPU
atomic guarantees?

[Severity: High]
This isn't a bug introduced by this patch, but does the devm teardown order
create a use-after-free window during driver removal?

In cxl_pmu_probe(), the CPU hotplug instance is registered before the PMU:

drivers/perf/cxl_pmu.c:cxl_pmu_probe() {
	...
	rc = cpuhp_state_add_instance(cxl_pmu_cpuhp_state_num, &info->node);
	if (rc)
		return rc;

	rc = devm_add_action_or_reset(dev, cxl_pmu_cpuhp_remove, info);
	if (rc)
		return rc;

	rc = perf_pmu_register(&info->pmu, info->pmu.name, -1);
	if (rc)
		return rc;

	rc = devm_add_action_or_reset(dev, cxl_pmu_perf_unregister, info);
	...
}

During driver removal, devm executes actions in LIFO order.
cxl_pmu_perf_unregister() runs first, delegating to perf_pmu_unregister(),
which frees pmu->cpu_pmu_context.

If a CPU is offlined concurrently before cxl_pmu_cpuhp_remove() runs,
cxl_pmu_offline_cpu() is invoked:

drivers/perf/cxl_pmu.c:cxl_pmu_offline_cpu() {
	...
	perf_pmu_migrate_context(&info->pmu, cpu, target);
	...
}

Will perf_pmu_migrate_context() access the freed cpu_pmu_context when it
calls find_get_pmu_context()?

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

[PATCH v3 8/9] 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>
---
v3:
- No change; picked up review tag.
---
 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 580a75bc9210..37742ce43d9f 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.55.0

[PATCH v3 9/9] 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() registers its devm actions so that teardown runs
perf_pmu_unregister() first, then the hotplug instance removal, then
free_irq(). Nothing before free_irq() masks the interrupt, so the handler
stays live across a window where info->pmu.dev is freed and its dev_dbg()
walks that pointer.

Unsharing the interrupt does not close that window. cxl_pmu_event_stop()
leaves the counter's overflow status bit set, and only the handler clears
it, so an overflow taken just before teardown is still delivered and still
gets past the "did anything overflow" early-out. It lands in the !event
branch, where the dev_dbg() is.

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

Clear the overflow status before requesting the interrupt too, since the
driver never touched it at probe and a counter left enabled with
INT_ON_OVRFLW by firmware or a previous kernel can raise an interrupt at
any point.

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>
---
v3:
- Clear CXL_PMU_OVERFLOW_REG at probe, before the handler is armed. A stale
  status bit from firmware can cause overflow interrupt. (Robin)
- Ack dropped as the patch grew a hunk.
- Drop the cxl_pmu_offline_cpu() hunk. That dev_err() cannot be reached.
---
 drivers/perf/cxl_pmu.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index 37742ce43d9f..3683427fbb7e 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -806,7 +806,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;
 		}
@@ -903,6 +903,15 @@ static int cxl_pmu_probe(struct device *dev)
 	if (!irq_name)
 		return -ENOMEM;
 
+	/*
+	 * Clear any overflow status left set by firmware or a previous kernel
+	 * before the handler goes live, so it cannot mistake a stale bit for an
+	 * overflow on a counter no event owns yet. The register is RW1C, and
+	 * bits above the implemented counters are RsvdZ, 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.55.0

Re: [PATCH v3 9/9] perf/cxl: Don't log through pmu.dev in the overflow interrupt handler

[sashiko-bot]

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

Pre-existing issues:
- [High] Double counting and cross-event corruption of performance events due to uncleared pending overflows.
--

--- Patch [9]: [PATCH v3 9/9] perf/cxl: Don't log through pmu.dev in the overflow interrupt handler ---
commit d6b58846ef576f998be916a54f3a5a31363d2d44
Author: Dave Jiang <dave.jiang@intel.com>

perf/cxl: Don't log through pmu.dev in the overflow interrupt handler

This commit fixes a use-after-free in the CXL PMU overflow interrupt handler
by changing the logging device to the parent and clears any stale overflow
status before requesting the IRQ.

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

[ ... ]

> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c

[ ... ]

> @@ -903,6 +903,15 @@ static int cxl_pmu_probe(struct device *dev)
>  	if (!irq_name)
>  		return -ENOMEM;
>  
> +	/*
> +	 * Clear any overflow status left set by firmware or a previous kernel
> +	 * before the handler goes live, so it cannot mistake a stale bit for an
> +	 * overflow on a counter no event owns yet. The register is RW1C, and
> +	 * bits above the implemented counters are RsvdZ, so only write those.
> +	 */
> +	writeq(GENMASK_ULL(info->num_counters - 1, 0),
> +	       info->base + CXL_PMU_OVERFLOW_REG);
> +

[Severity: High]
This isn't a bug introduced by this patch, but since we are clearing stale 
overflow bits here, does cxl_pmu_event_stop() also need to clear the overflow 
bit when stopping an event?

Currently, cxl_pmu_event_stop() in drivers/perf/cxl_pmu.c disables the counter 
but doesn't clear any pending overflow bit in CXL_PMU_OVERFLOW_REG. If an 
event overflows just before being stopped (e.g., during PMU multiplexing), 
and a new event is started on the same counter or reset via 
PERF_EVENT_IOC_RESET, the hardware overflow bit remains set.

When IRQs are re-enabled, cxl_pmu_irq() handles the pending IRQ and calls
__cxl_pmu_read() with overflow=true. Because the counter was just reset
by cxl_pmu_event_start(), both new_cnt and prev_cnt will be 0. 

Since new_cnt (0) >= prev_cnt (0), would __cxl_pmu_read() falsely apply the 
wraparound fixup:

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

Could this erroneously add a full period (e.g., 2^64 ticks) to the newly
started or reset event and corrupt the counter statistics?

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