Re: [PATCH v3 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:
- [Medium] Counter configuration fields are not cleared before being modified in cxl_pmu_event_start(), causing corrupted event group IDs when a counter is reused.
--

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

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

cxl_pmu_get_event_idx() sets *counter_idx for a configurable counter but
never *event_idx. hwc->event_base stays 0, so cxl_pmu_event_start()
programs event group 0 no matter which group the user asked for.

Return the matched capability's event_idx.

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

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index 68a54d97d2a8a..ff01b658e1b45 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
[ ... ]
> @@ -541,7 +544,7 @@ static int cxl_pmu_get_event_idx(struct perf_event *event, int *counter_idx,
>  
>  	pmu_ev = cxl_pmu_find_config_counter_ev_cap(info, vid, gid, mask);
>  	if (!IS_ERR(pmu_ev)) {
> -		if (!counter_idx)
> +		if (!counter_idx || !event_idx)
>  			return 0;
>  
>  		bitmap_andnot(configurable_and_free, info->conf_counter_bm,
> @@ -552,6 +555,7 @@ static int cxl_pmu_get_event_idx(struct perf_event *event, int *counter_idx,
>  			return -EINVAL;
>  
>  		*counter_idx = i;
> +		*event_idx = pmu_ev->event_idx;

[Severity: Medium]
Will this corrupt the hardware counter configuration when a counter is reused?

By populating event_idx here, hwc->event_base will now contain the actual
event group ID rather than 0.

However, in cxl_pmu_event_start(), the configuration is updated using a
read-modify-write without clearing the previous fields:

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));
	}
    ...
    writeq(cfg, base + CXL_PMU_COUNTER_CFG_REG(hwc->idx));
}

If a counter is freed and reused for a new event with a different event group
ID, won't the bitwise OR merge the old and new event_base values since the
CXL_PMU_COUNTER_CFG_EVENT_GRP_ID_IDX_MSK bits are never cleared from cfg?

>  		return 0;
>  	}
>

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