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

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 <[email protected]>

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/[email protected]?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/[email protected]?part=1