Re: [PATCH v2 1/2] perf/cxlpmu: Support Channel/Rank/Bank filter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PMU filters (like the newly added CRB filter) are silently ignored if the requested event is assigned to a fixed counter.
--

commit efebea58e23a015bdb87cae3df5f0d067aa37706
Author: Harshal Thakkar <harshal.t@samsung.com>

perf/cxlpmu: Support Channel/Rank/Bank filter

Implement CRB filtering per CXL 4.0 8.2.7.2.2, and extend the current
filtering support beyond HDM. These filters are only for DDR Interface
events.

> 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
[ ... ]
> @@ -627,15 +660,23 @@ static void cxl_pmu_event_start(struct perf_event *event, int flags)
>  	hwc->state = 0;
>  
>  	/*
> -	 * Currently only hdm filter control is implemented, this code will
> -	 * want generalizing when more filters are added.
> +	 * Filter ID=0: HDM decoder filter
> +	 * Filter ID=1: Channel/Rank/Bank (CRB) filter
>  	 */
>  	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));
> +		writel(cfg, base + CXL_PMU_FILTER_CFG_REG(hwc->idx, 0));
> +	}
> +
> +	if (info->filter_chan_rank_bank) {
> +		if (cxl_pmu_config1_crb_filter_en(event))
> +			cfg = cxl_pmu_config2_get_crb(event);
> +		else
> +			cfg = GENMASK(31, 0); /* no filtering if 0xFFFF_FFFF */
> +		writel(cfg, base + CXL_PMU_FILTER_CFG_REG(hwc->idx, 1));

[Severity: Medium]
Will this filter configuration be silently ignored if the event is assigned
to a fixed counter?

In cxl_pmu_get_event_idx(), fixed counters are prioritized for assignment 
without checking if PMU filters were requested:

cxl_pmu_get_event_idx() {
	...
	pmu_ev = cxl_pmu_find_fixed_counter_ev_cap(info, vid, gid, mask);
	if (!IS_ERR(pmu_ev)) {
		if (!counter_idx)
			return 0;
		if (!test_bit(pmu_ev->counter_idx, info->used_counter_bm)) {
			*counter_idx = pmu_ev->counter_idx;
			return 0;
		}
	}
	...
}

Since fixed counters do not support filtering per the CXL specification, does
writing the CRB filter configuration to a fixed counter's register result
in the hardware silently ignoring it? This could lead to unfiltered counts
being returned to the user when they expected filtered statistics.

>  	}
>  
>  	cfg = readq(base + CXL_PMU_COUNTER_CFG_REG(hwc->idx));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630235002.253297-1-dave@stgolabs.net?part=1