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

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

Pre-existing issues:
- [High] Writing to the 32-bit filter configuration register with `writeq` (64-bit) overwrites the adjacent filter register, causing silent event drops on hardware with multiple filters.
- [High] The overflow calculation in `__cxl_pmu_read()` double-counts events by erroneously adding `2^width` to a correctly computed delta.
--

--- Patch [2]: [PATCH v4 02/11] perf/cxl: Clear stale event fields before reprogramming a counter ---
commit 54f01a971c5fe0d0d8f7face1960b3232e30fafb
Author: Dave Jiang <[email protected]>

perf/cxl: Clear stale event fields before reprogramming a counter
    
This commit uses FIELD_MODIFY() to clear stale event fields before
reprogramming a counter. This ensures new selections don't land on top of
old ones when reusing counters.

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

> 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

[ ... ]

> @@ -638,30 +638,30 @@ static void cxl_pmu_event_start(struct perf_event *event, int flags)
>  	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));

[Severity: High]
This isn't a bug introduced by this patch, but could this writeq()
unintentionally overwrite the adjacent filter register?

Since CXL_PMU_FILTER_CFG_REG is a 32-bit register and writeq() performs a
64-bit write, the variable cfg (which has 0 in its upper 32 bits) will
inadvertently zero out the adjacent 32 bits corresponding to Filter 1.

Because writing 0 enables a filter and forces an exact match of 0, any
hardware implementing Filter 1 will logically AND it with Filter 0,
effectively dropping all non-zero events.

>  	}
>  
>  	cfg = readq(base + CXL_PMU_COUNTER_CFG_REG(hwc->idx));
>  	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);

[ ... ]

> @@ -700,10 +700,10 @@ static void __cxl_pmu_read(struct perf_event *event, bool overflow)
>  	/*
>  	 * If we know an overflow occur then take that into account.
>  	 * Note counter is not reset as that would lose events
>  	 */
>  	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 is a pre-existing issue, but is it possible this addition double-counts
events?

The 2's complement arithmetic delta = (new_cnt - prev_cnt) & GENMASK_ULL(...)
already naturally calculates the delta across a single wrap-around. For
example, with a 32-bit counter, 0x10 - 0xFFFFFFF0 masked strictly yields 0x20.

If this condition evaluates to true, it injects an additional 2^width into
the delta, reporting billions of extraneous events per normal overflow.

Additionally, if info->counter_width is 64, does 1UL << 64 result in
undefined behavior in C?

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

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