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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The function `cxl_pmu_config2_get_crb()` returns `u16`, silently truncating the 32-bit CRB filter value.
- [High] Using `writeq()` to program the 32-bit CRB filter register causes unaligned MMIO access and clobbers adjacent registers.
- [Low] The mask `CXL_PMU_ATTR_CONFIG2_CRB_MSK` is defined using `GENMASK(63, 32)`, causing compilation errors and undefined behavior on 32-bit architectures.
--

commit a9330e1fbdf601456995f95b96ae724e328f12a5
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
> index 68a54d97d2a8a..b709b2e6c0d95 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c

[ ... ]

> @@ -246,7 +253,9 @@ static struct attribute *cxl_pmu_format_attr[] = {
>  #define CXL_PMU_ATTR_CONFIG1_INVERT_MSK		BIT(16)
>  #define CXL_PMU_ATTR_CONFIG1_EDGE_MSK		BIT(17)
>  #define CXL_PMU_ATTR_CONFIG1_FILTER_EN_MSK	BIT(18)
> +#define CXL_PMU_ATTR_CONFIG1_CRB_FILTER_EN_MSK	BIT(19)
>  #define CXL_PMU_ATTR_CONFIG2_HDM_MSK		GENMASK(15, 0)
> +#define CXL_PMU_ATTR_CONFIG2_CRB_MSK		GENMASK(63, 32)

[Severity: Low]
Does this code cause compilation errors on 32-bit architectures?

GENMASK generates an unsigned long, which is 32 bits wide on 32-bit
architectures. Shifting by 32 or more results in undefined behavior and
triggers the macro's internal BUILD_BUG_ON_ZERO(h >= BITS_PER_LONG).

Should this use GENMASK_ULL(63, 32) for a 64-bit mask?

[ ... ]

> @@ -319,6 +333,17 @@ static u16 cxl_pmu_config2_get_hdm_decoder(struct perf_event *event)
>  	return FIELD_GET(CXL_PMU_ATTR_CONFIG2_HDM_MSK, event->attr.config2);
>  }
>  
> +static u16 cxl_pmu_config1_crb_filter_en(struct perf_event *event)
> +{
> +	return FIELD_GET(CXL_PMU_ATTR_CONFIG1_CRB_FILTER_EN_MSK,
> +			 event->attr.config1);
> +}
> +
> +static u16 cxl_pmu_config2_get_crb(struct perf_event *event)
> +{
> +	return FIELD_GET(CXL_PMU_ATTR_CONFIG2_CRB_MSK, event->attr.config2);
> +}

[Severity: High]
Does this function truncate the 32-bit CRB filter value?

The CRB filter mask config2:32-63 is 32 bits wide, but
cxl_pmu_config2_get_crb() returns u16. When the 32-bit value is extracted
using FIELD_GET(), the upper 16 bits are silently dropped.

Should the return type be changed to u32 to preserve the full filter value?

[ ... ]

> @@ -627,8 +660,8 @@ 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))
> @@ -638,6 +671,14 @@ static void cxl_pmu_event_start(struct perf_event *event, int flags)
>  		writeq(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 */
> +		writeq(cfg, base + CXL_PMU_FILTER_CFG_REG(hwc->idx, 1));

[Severity: High]
Can this writeq() trigger an unaligned MMIO access fault and clobber
adjacent registers?

The CXL PMU filter configuration registers are 32 bits wide with a 4-byte
stride:

0x400 + 4 * (f + n * 8)

For f=1, CXL_PMU_FILTER_CFG_REG() resolves to an unaligned offset like
0x404. Using a 64-bit writeq() on a 4-byte unaligned device memory
address triggers undefined behavior and hardware alignment faults, such as
a Synchronous External Abort on ARM64. 

Additionally, writing 64 bits to a 32-bit register clobbers the adjacent
register (f=2).

Would writel() be safer here?

> +	}
> +
>  	cfg = readq(base + CXL_PMU_COUNTER_CFG_REG(hwc->idx));
>  	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_INT_ON_OVRFLW, 1);
>  	cfg |= FIELD_PREP(CXL_PMU_COUNTER_CFG_FREEZE_ON_OVRFLW, 1);

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