Re: [PATCH v2 3/6] cxl/region: Account for mixed-granularity in position calculations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect uniform `pos_distance` assumption in mixed-granularity regions leads to a kernel panic in `check_last_peer()`.
- [High] Flawed arithmetic in `cxl_calc_interleave_pos()` breaks auto-discovery for topologies with multiple interleave passes or interleaved downstream switches.
--

commit 9cd5598bf3a699e6fbc48008f0af90a4640e7218
Author: Alison Schofield <alison.schofield@intel.com>

cxl/region: Account for mixed-granularity in position calculations

Mixed-granularity regions place the root decoder at outer HPA bits. A root
target then owns root_gran / region_gran inner positions before advancing.
This patch accounts for that stride in endpoint placement, position
calculations, and peer-distance checks.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index d5d19ed621713..9fff538a9ce16 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c

[ ... ]

> @@ -1490,10 +1512,11 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  				cxlrd->cxlsd.cxld.interleave_granularity);
>  
>  	/* Track distances used in passthrough granularity and peer checks */
> -	parent_distance = cxlrd->cxlsd.cxld.interleave_ways;
> -	if (!is_power_of_2(parent_distance))
> -		parent_distance /= 3;
> -	distance = cxlrd->cxlsd.cxld.interleave_ways;
> +	stride = root_pos_stride(cxlr);
> +	sel_distance = cxlrd->cxlsd.cxld.interleave_ways;
> +	if (!is_power_of_2(sel_distance))
> +		sel_distance /= 3;
> +	pos_distance = stride > 1 ? 1 : cxlrd->cxlsd.cxld.interleave_ways;
>  
>  	for (iter = parent_port; !is_cxl_root(iter);
>  	     iter = to_cxl_port(iter->dev.parent)) {

[ ... ]

> @@ -1540,7 +1563,7 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
>  			if (ep->dport == cxlsd->target[i]) {
>  				rc = check_last_peer(cxled, ep, cxl_rr,
> -						     distance);
> +						     pos_distance);

[Severity: High]
If stride > 1, pos_distance is initially 1 and then scaled by intermediate
target counts. Does this uniform distance correctly account for the
non-uniform gap between positions routed to the same downstream port in
mixed-granularity topologies?

In check_last_peer(), it subtracts this distance to locate a peer endpoint:

drivers/cxl/core/region.c:check_last_peer() {
	...
	cxled_peer = p->targets[pos - distance];
	cxlmd_peer = cxled_to_memdev(cxled_peer);
	ep_peer = cxl_ep_load(port, cxlmd_peer);
	if (ep->dport != ep_peer->dport) {
	...
}

If the calculated distance is flawed, could this inspect a position
belonging to a completely different root target's block? If so,
cxl_ep_load() might return NULL since the peer endpoint isn't a
descendant of the current intermediate switch port. This would lead
to a NULL pointer dereference when accessing ep_peer->dport.

>  				if (rc)
>  					return rc;
>  				goto out_target_set;

[ ... ]

> @@ -2010,6 +2042,11 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled,
>  		if (rc)
>  			return rc;
>  
> +		if (stride > 1 && is_cxl_root(parent_port_of(iter))) {
> +			pos = pos + stride * parent_pos;
> +			break;
> +		}
> +
>  		pos = pos * parent_ways + parent_pos;
>  	}

[Severity: High]
Does this position calculation properly handle interleave wrap-around and
higher-order switch interleaving?

By using pos = pos + stride * parent_pos, it appears to assume that the root
target always provides the highest-order bits of the region position. In
topologies where a root target owns more endpoints than the stride, or where
downstream switches decode bits higher than the root decoder, could this
result in local position collisions?

For example, if the 5th endpoint on a host bridge has a local pos of 4 and
a stride of 4, this would calculate 4 + 4 * 0 = 4 instead of the correct
region position 8. Would this cause cxl_region_attach_position() to reject
otherwise valid auto-discovered CXL topologies?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781199122.git.alison.schofield@intel.com?part=3