Re: [PATCH v3 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] writeq() used on 32-bit CXL PMU Filter Config registers clobbers t=
he adjacent Filter 1 configuration with zeros.
- [High] Pending hardware overflows from stopped events are erroneously att=
ributed to newly started events on the same counter, causing massive spurio=
us counts.
- [High] Two's complement arithmetic misunderstanding causes the driver to =
overcount by exactly 2^width on almost every overflow interrupt.
--

--- Patch [2]: [PATCH v3 2/9] perf/cxl: Clear stale event fields before rep=
rogramming a counter ---
commit e2d9388d1e46f8effd5efea9988cf263ff2efd2c
Author: Dave Jiang <[email protected]>

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

cxl_pmu_event_start() ORs the event group id, event mask, edge and invert
selections into a configurable counter's config register without clearing
them first, and cxl_pmu_event_stop() leaves them set. Reuse the counter for
another event and the new selection lands on top of the old one, so the
counter counts the wrong events with stale edge and invert behaviour.

Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolab=
s.net?part=3D1

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

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem earlier in
cxl_pmu_event_start():

drivers/perf/cxl_pmu.c:cxl_pmu_event_start() {
    ...
    if (info->filter_hdm) {
        if (cxl_pmu_config1_hdm_filter_en(event))
            cfg =3D cxl_pmu_config2_get_hdm_decoder(event);
        else
            cfg =3D GENMASK(31, 0);
        writeq(cfg, base + CXL_PMU_FILTER_CFG_REG(hwc->idx, 0));
    ...
}

Does using the 64-bit writeq() here overwrite the adjacent Filter 1
configuration register at offset +4 with the upper 32 bits of cfg (which are
zeros)? This might cause the PMU hardware to silently drop events that do n=
ot
match 0 for Filter 1's property, breaking PMU counting for this counter.

>  	cfg |=3D FIELD_PREP(CXL_PMU_COUNTER_CFG_INT_ON_OVRFLW, 1);
>  	cfg |=3D FIELD_PREP(CXL_PMU_COUNTER_CFG_FREEZE_ON_OVRFLW, 1);
>  	cfg |=3D FIELD_PREP(CXL_PMU_COUNTER_CFG_ENABLE, 1);
> -	cfg |=3D FIELD_PREP(CXL_PMU_COUNTER_CFG_EDGE,
> -			  cxl_pmu_config1_get_edge(event) ? 1 : 0);
> -	cfg |=3D 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);
> =20
>  	/* Fixed purpose counters have next two fields RO */
>  	if (test_bit(hwc->idx, info->conf_counter_bm)) {
> -		cfg |=3D FIELD_PREP(CXL_PMU_COUNTER_CFG_EVENT_GRP_ID_IDX_MSK,
> -				  hwc->event_base);
> -		cfg |=3D 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 &=3D ~CXL_PMU_COUNTER_CFG_THRESHOLD_MSK;
>  	/*

[Severity: High]
This isn't a bug introduced by this patch, but I spotted a potential issue =
with
hardware overflow attribution when stopping and starting events.

If a counter overflows right before being stopped, the hardware sets the
overflow bit and raises an interrupt, but the pending overflow bit in
CXL_PMU_OVERFLOW_REG is not cleared:

drivers/perf/cxl_pmu.c:cxl_pmu_event_stop() {
    ...
    cfg &=3D ~(FIELD_PREP(CXL_PMU_COUNTER_CFG_INT_ON_OVRFLW, 1) |
             FIELD_PREP(CXL_PMU_COUNTER_CFG_ENABLE, 1));
    writeq(cfg, base + CXL_PMU_COUNTER_CFG_REG(hwc->idx));
    ...
}

When a new event is scheduled onto the same counter via cxl_pmu_event_start=
()
and the interrupt fires, will the ISR attribute the overflow to the newly
started event?

drivers/perf/cxl_pmu.c:cxl_pmu_irq() {
    ...
    for_each_set_bit(i, overflowedbm, info->num_counters) {
        struct perf_event *event =3D info->hw_events[i];
        if (!event)
            continue;
        __cxl_pmu_read(event, true);
    }
    ...
}

Could this cause massive spurious event counts to be falsely attributed to =
new
events during normal perf multiplexing?


[Severity: High]
This is a pre-existing issue, but does __cxl_pmu_read() overcount on overfl=
ow
interrupts?

drivers/perf/cxl_pmu.c:__cxl_pmu_read() {
    ...
    delta =3D (new_cnt - prev_cnt) & GENMASK_ULL(info->counter_width - 1, 0=
);
    if (overflow && delta < GENMASK_ULL(info->counter_width - 1, 0))
        delta +=3D (1UL << info->counter_width);
    ...
}

Because these are unsigned 64-bit integers, the masked subtraction natively
accounts for exactly one counter wrap, yielding the correct positive event
delta.=20

Since delta is already correct, does adding (1UL << info->counter_width) on=
 top
of it cause the driver to overcount by exactly 2^width on almost every over=
flow
interrupt?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260731232827.4014=
[email protected]?part=3D2