Re: [RESEND 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] Incorrect 64-bit write (writeq) to a 32-bit hardware filter configuration register.
- [High] Incorrect PMU counter overflow handling logic leads to massive overcounting and Undefined Behavior (UB) via out-of-bounds bitwise shift.
--

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

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

This patch fixes an issue in cxl_pmu_event_start() where previous event
settings were accumulated instead of cleared. It uses FIELD_MODIFY() to
ensure event group ID, mask, edge, and invert selections replace the old
values, preventing counters from using stale configuration.

Link: https://sashiko.dev/#/patchset/[email protected]?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)
>  	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 while looking at
cxl_pmu_event_start(), does this writeq() overwrite the configuration
for the adjacent filter?

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

CXL_PMU_FILTER_CFG_REG has a 4-byte stride, making it a 32-bit hardware
register. Using the 64-bit writeq() here spills into filter 1. If filter 0
is disabled (cfg = 0xFFFFFFFF), writeq() writes 0x00000000 to filter 1,
which would inadvertently enable filter 1 with a 0-match condition.


[Severity: High]
This is a pre-existing issue, but I also noticed a potential undefined
behavior in the overflow handling logic nearby:

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

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

Since delta already accounts for one wraparound via the bitwise AND when
new_cnt is less than prev_cnt, does unconditionally adding 2^width on
overflow cause massive overcounting? 

Also, can (1UL << info->counter_width) cause an out-of-bounds bitwise
shift resulting in undefined behavior? The CXL specification allows
counter_width to be 64, which would shift beyond the width of an unsigned
long, and on 32-bit architectures this could happen for a width of 32 or
greater.

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