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

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

Pre-existing issues:
- [High] A 64-bit `writeq()` is incorrectly used to write a 32-bit filter configuration register, corrupting the adjacent filter register.
- [High] The PMU event read logic incorrectly adds `2^width` to the event delta on typical overflows, causing massive overcounting and undefined behavior.
- [Medium] The PMU sysfs format attribute for `threshold` accepts a 16-bit value, but the accessor silently truncates it to 8 bits.
--

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

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

This commit fixes an issue where reprogramming a performance counter leaves
stale values in the configuration register. It ensures the event group id,
event mask, edge, and invert fields are cleared before being reprogrammed.
This prevents the counter from accumulating statistics with old selections.

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);
>  	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_FREEZE_ON_OVRFLW, 1);
>  	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_ENABLE, 1);

[Severity: High]
This isn't a bug introduced by this patch, but does the filter configuration
overwrite the adjacent Filter 1 register?

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

Because CXL_PMU_FILTER_CFG_REG uses a 4-byte stride, the registers are
expected to be 32-bit wide. A 64-bit writeq() will write 8 bytes, which
on a little-endian system clobbers the adjacent Filter 1 register when
writing to Filter 0.

> @@ -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);
>  		cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_EVENTS_MSK,

[Severity: Medium]
This is a pre-existing issue, but could the threshold accessor silently
truncate user-provided values?

drivers/perf/cxl_pmu.c:cxl_pmu_config1_get_threshold()
static u8 cxl_pmu_config1_get_threshold(struct perf_event *event)
{
	return FIELD_GET(CXL_PMU_ATTR_CONFIG1_THRESHOLD_MSK,
			 event->attr.config1);
}

The accessor returns a u8, but the PMU sysfs format attribute for threshold
accepts a 16-bit value (config1:0-15). If a user requests an event with a
threshold configuration greater than 255, the value gets silently truncated
before being programmed into the hardware.

[Severity: High]
This isn't a bug introduced by this patch, but does the event read logic
cause massive overcounting when a counter overflows?

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

The calculation (new_cnt - prev_cnt) & mask correctly handles a single
wrap-around when new_cnt < prev_cnt due to two's complement arithmetic.
Because delta < mask is almost always true during an overflow, the subsequent
addition double counts the wrap-around.

Additionally, if info->counter_width is 64, shifting 1UL by 64 will invoke
undefined behavior.

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