Re: [PATCH 13/15] cxl/cache: Add snoop filter allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] cxl: cxl_cachedev_find_snoop_gid() loop prematurely ignores root dports
- [Medium] cxl: memory bloat from XArray with kernel pointer as index
--

commit 0d6bed69d638badd54cfffcfb59d4229114673a5
Author: Ben Cheatham <Benjamin.Cheatham@amd.com>

cxl/cache: Add snoop filter allocation

This commit adds snoop filter capacity allocation for CXL.cache devices.
Devices are expected to allocate snoop filter capacity equal to the size of
the address range they will cache before using the protocol. It also allows
devices to strictly fail allocations if the filter is full.

> diff --git a/drivers/cxl/cache.c b/drivers/cxl/cache.c
> index 40d1e8330df78..b4574a76ac0b8 100644
> --- a/drivers/cxl/cache.c
> +++ b/drivers/cxl/cache.c
> @@ -179,6 +179,20 @@ struct cxl_cachedev *devm_cxl_add_cachedev(struct cxl_dev_state *cxlds)
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_add_cachedev, "CXL");
>  
> +static int cxl_cachedev_find_snoop_gid(struct cxl_cachedev *cxlcd)
> +{
> +	struct cxl_dport *iter;
> +
> +	for (iter = cxlcd->endpoint->parent_dport;
> +	     iter && !is_cxl_root(iter->port);
> +	     iter = iter->port->parent_dport) {

[Severity: High]
Does this loop prematurely ignore snoop filters located on root ports?

The termination condition !is_cxl_root(iter->port) is evaluated before the
loop body. When iter reaches the cxl_dport of a host bridge (which is a
downstream port of the cxl_root port), iter->port points to the cxl_root
port. This makes is_cxl_root(iter->port) evaluate to true, causing the loop
to terminate immediately before checking iter->snoop.

Consequently, snoop filters located on host bridges are completely ignored.
If no intermediate switch has a snoop filter, will this cause
cxl_cache_probe() to unconditionally fail with -ENXIO for direct attached
devices?

> +		if (iter->snoop != CXL_SNOOP_FILTER_NO_GROUP_ID)
> +			return iter->snoop;
> +	}
> +
> +	return -ENXIO;
> +}

[ ... ]

> diff --git a/drivers/cxl/core/cache.c b/drivers/cxl/core/cache.c
> index f07c49cfacec2..429c997b2ada6 100644
> --- a/drivers/cxl/core/cache.c
> +++ b/drivers/cxl/core/cache.c

[ ... ]

> @@ -638,6 +671,94 @@ static struct cxl_snoop_filter *find_or_add_snoop_filter(u64 size, int id)
>  	return sf;
>  }
>  
> +static struct snoop_allocation *
> +snoop_alloc_capacity(struct cxl_snoop_filter *sf, struct cxl_cachedev *cxlcd,
> +		     u64 size)
> +{
> +	struct cxl_cache_state *cstate = &cxlcd->cxlds->cstate;
> +	struct snoop_allocation *alloc;
> +	int rc;
> +
> +	guard(mutex)(&sf->lock);
> +	if (sf->avail < size) {
> +		/*
> +		 * Either this device or a device already using the filter
> +		 * can't use an oversubscribed filter
> +		 */
> +		if (cstate->strict_snoop || sf->strict > 0)
> +			return ERR_PTR(-ENOSPC);
> +
> +		size = sf->avail;
> +	}
> +
> +	alloc = kmalloc_obj(*alloc);
> +	if (!alloc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	*alloc = (struct snoop_allocation) {
> +		.cxlcd = cxlcd,
> +		.size = size,
> +		.strict = cstate->strict_snoop,
> +		.id = sf->id,
> +	};
> +
> +	rc = xa_insert(&sf->allocations, (unsigned long)alloc, alloc,
> +		       GFP_KERNEL);

[Severity: Medium]
Does using a kernel pointer as an XArray index cause memory bloat here?

On 64-bit architectures, kernel pointers have their high bits set. Using a
dense array structure like XArray for a highly sparse, massive index forces
it to allocate its maximum depth of internal xa_node radix tree levels
(typically 11 levels). This wastes thousands of bytes of memory per
allocation to track a single pointer.

Since the XArray is never used for index-based lookups (elements are only
erased by pointer during cleanup), would a standard struct list_head provide
the same functionality with better performance and no memory bloat?

> +	if (rc) {
> +		kfree(alloc);
> +		return ERR_PTR(rc);
> +	}
> +
> +	if (cstate->strict_snoop)
> +		sf->strict++;
> +
> +	sf->avail -= size;
> +	return alloc;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923173342.5584-1-Benjamin.Cheatham@amd.com?part=13