Re: [PATCH 1/9] perf/cxl: Program the requested event group on configurable counters

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

Pre-existing issues:
- [High] In cxl_pmu_event_start(), configuration bitfields are bitwise-ORed with the existing register value without being cleared first, leading to corrupted register state.
--

--- Patch [1]: [PATCH 1/9] perf/cxl: Program the requested event group on configurable counters ---
commit a96f996812668f5b85e51cf8c46d25fbd3781fce
Author: Dave Jiang <[email protected]>

perf/cxl: Program the requested event group on configurable counters

cxl_pmu_get_event_idx() sets the counter index for a configurable counter but
never sets the event index, causing the event base to stay 0. This change
returns the matched capability's event index to correctly program the group.

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
> @@ -552,6 +552,8 @@ static int cxl_pmu_get_event_idx(struct perf_event *event, int *counter_idx,
>  			return -EINVAL;
>  
>  		*counter_idx = i;
> +		if (event_idx)
> +			*event_idx = pmu_ev->event_idx;

[Severity: High]
This is a pre-existing issue, but does assigning non-zero event indices here
expose a bit accumulation bug in cxl_pmu_event_start()?

When the event starts, cxl_pmu_event_start() updates the configuration
register using a bitwise-OR against the existing value:

drivers/perf/cxl_pmu.c:cxl_pmu_event_start() {
    cfg = readq(base + CXL_PMU_COUNTER_CFG_REG(hwc->idx));
    ...
    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));
    }
    ...
}

Since cxl_pmu_event_stop() only clears the ENABLE and INT_ON_OVRFLW fields,
the old group ID bits remain in the register. Because the event base is
now correctly populated by this patch instead of remaining 0, won't
consecutive events reusing the same counter cause EVENT_GRP_ID_IDX_MSK
and other fields to accumulate stale bits from previous events?

Could this lead to corrupted hardware state over time, and should the
relevant register fields be explicitly cleared before applying the new
configuration?

>  		return 0;
>  	}

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