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

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

Pre-existing issues:
- [High] Incorrect overflow handling in __cxl_pmu_read() double-counts events and invokes undefined behavior via an out-of-bounds shift.
--

--- Patch [2]: [PATCH 2/9] perf/cxl: Clear stale event fields before reprogramming a counter ---
commit 7bcf06641417cf2542677bb2d2607c683f730512
Author: Dave Jiang <[email protected]>

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

This commit clears stale event fields from the configuration register
before reprogramming a counter in the CXL PMU driver. This ensures
the counter does not count the wrong events with old edge or invert
behavior when reused.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=1

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index e77c0650a7eb6..3138514157cde 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
> @@ -641,6 +641,7 @@ static void cxl_pmu_event_start(struct perf_event *event, int flags)
>  	}
>  
>  	cfg = readq(base + CXL_PMU_COUNTER_CFG_REG(hwc->idx));
> +	cfg &= ~(CXL_PMU_COUNTER_CFG_EDGE | CXL_PMU_COUNTER_CFG_INVERT);
>  	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_INT_ON_OVRFLW, 1);

[ ... ]

> @@ -651,6 +652,8 @@ static void cxl_pmu_event_start(struct perf_event *event, int flags)
>  
>  	/* Fixed purpose counters have next two fields RO */
>  	if (test_bit(hwc->idx, info->conf_counter_bm)) {
> +		cfg &= ~(CXL_PMU_COUNTER_CFG_EVENT_GRP_ID_IDX_MSK |
> +			 CXL_PMU_COUNTER_CFG_EVENTS_MSK);
>  		cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_EVENT_GRP_ID_IDX_MSK,
>  				  hwc->event_base);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential problem
in how overflows are handled in __cxl_pmu_read() in this same 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);
    ...
}

Does this double-count the overflow events? Since the subtraction is already
masked by GENMASK_ULL(), the wraparound is correctly accounted for in delta.
Adding (1UL << info->counter_width) appears redundant and might corrupt the
resulting metrics.

Additionally, does this shift operation risk undefined behavior? If
info->counter_width is 64 (the maximum width allowed by the CXL 3.0
specification) on 64-bit systems, or 32 or greater on 32-bit systems,
shifting 1UL by that width would cause an out-of-bounds shift.

Is it safe to rely on this delta calculation when counter_width can be large
enough to trigger this undefined behavior?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2