Re: [PATCH v2 1/6] cxl/region: Validate interleave selector bits

[Alison Schofield <alison.schofield@intel.com>]

On Fri, Jun 26, 2026 at 01:37:17PM +0200, Robert Richter wrote:
> On 11.06.26 10:47:25, Alison Schofield wrote:
> > Multi-level interleaves are valid when each decoder level claims a
> > distinct portion of the region HPA selector. The existing setup is
> > too restrictive as it requires that each level's granularity must
> > equal the parent granularity multiplied by the parent ways. That
> > forces the root to sit at the innermost selector bit and rejects
> > legal layouts where the root claims an outer bit.
> > 
> > Add selector-bit accounting to cxl_port_setup_targets(). Accumulate
> > selector bits from the root toward the current port, reject overlap,
> > and reject any accumulated selector that does not fit within the
> > region selector.

Hi Robert,

The question underneath several of your comments, whether the
implementation should move to selector-bit arithmetic, is answered in my
reply on Patch 3/6.

Below, the local comments on this patch.

> 
> I think this is the main goal of the patch, to introduce a
> selector-bit mask and use that to keep track of ways and
> granularities. We should remove as much as possible from the original
> code without introducing functional changes here. Subject and
> description should emphasize that more.

Agreed. In v3 (I hope that) this patch becomes a no-functional-change
refactor and the log say so explicitly.

> 
> > 
> > Preserve the existing same-granularity behavior using values derived
> > from the selector walk.
> > 
> > Block mixed-granularity attachments until position arithmetic is
> > updated by a later patch.
> > 
> > Originally-by: Robert Richter <rrichter@amd.com>
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/core/region.c | 192 +++++++++++++++++++-------------------
> >  1 file changed, 96 insertions(+), 96 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index e50dc716d4e8..c859c8661281 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1423,12 +1423,34 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
> >  	return 0;
> >  }
> >  
> > +/**
> > + * get_selector() - Return the HPA selector mask for one level
> > + * @ways: interleave ways
> > + * @gran: interleave granularity in bytes
> > + *
> > + * Return: ``(ways - 1) * gran`` for power-of-2 interleaves, or the
> > + * selector for the power-of-2 component of a 6-way or 12-way interleave.
> > + *
> > + * A 3-way interleave consumes no HPA selector bits.
> > + */
> > +static inline u64 get_selector(int ways, int gran)
> > +{
> > +	if (!is_power_of_2(ways))
> > +		ways /= 3;
> > +
> > +	if (!is_power_of_2(ways) || !is_power_of_2(gran))
> > +		return 0;
> > +
> > +	return (u64)(ways - 1) * gran;
> > +}
> > +
> >  static int cxl_port_setup_targets(struct cxl_port *port,
> >  				  struct cxl_region *cxlr,
> >  				  struct cxl_endpoint_decoder *cxled)
> >  {
> >  	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> > -	int parent_iw, parent_ig, ig, iw, rc, pos = cxled->pos;
> > +	int distance, parent_distance;
> > +	int ig, iw, rc, pos = cxled->pos;
> >  	struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
> >  	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
> >  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > @@ -1436,9 +1458,10 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >  	struct cxl_region_params *p = &cxlr->params;
> >  	struct cxl_decoder *cxld = cxl_rr->decoder;
> >  	struct cxl_switch_decoder *cxlsd;
> > -	struct cxl_port *iter = port;
> > -	u16 eig, peig;
> > -	u8 eiw, peiw;
> > +	u64 selector, cxlr_sel;
> > +	struct cxl_port *iter;
> > +	u16 eig;
> > +	u8 eiw;
> 
> That list of local vars is very long and we should factor out some of
> the code to helpers.
> 

Agreed. v3 factors the ancestor walk into a helper and moves the per-level
selector computation with it. That takes most of the locals with it.


> >  
> >  	/*
> >  	 * While root level decoders support x3, x6, x12, switch level
> > @@ -1452,28 +1475,66 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >  	}
> >  
> >  	cxlsd = to_cxl_switch_decoder(&cxld->dev);
> 
> That could be moved to the declaration section.

Done in v3.


> 
> > +	iw = cxl_rr->nr_targets;
> > +
> > +	if (iw > 8 || iw > cxlsd->nr_targets) {
> 
> And this can be moved up / merged with the power of 2 check before.
> 

Done in v3.


> > +		dev_dbg(&cxlr->dev,
> > +			"%s:%s:%s: ways: %d overflows targets: %d\n",
> > +			dev_name(port->uport_dev), dev_name(&port->dev),
> > +			dev_name(&cxld->dev), iw, cxlsd->nr_targets);
> > +		return -ENXIO;
> > +	}
> > +
> > +	/* Track selector bits already claimed by ancestor decoders */
> > +	selector = get_selector(cxlrd->cxlsd.cxld.interleave_ways,
> > +				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;
> 
> Better use only one var here. One can derived from the other once
> needed.
> 
> Actually, we do not really need it at all, see below.
> 

v3 keeps a single variable and derives the other form at its one
point of use. On whether we need it at all, deriving distance
purely from the selector bits works for the power-of-2 part but
the selectors cannot carry the factor 3 of a 3/6/12-way root, so a
value-space term survives. See Patch 3/6 reply.


> > +
> > +	for (iter = parent_port; !is_cxl_root(iter);
> 
> That could become a helper function get_parent_selectors() or so.
> 

Agreed, that's the helper mentioned above.




> > +	     iter = to_cxl_port(iter->dev.parent)) {
> > +		struct cxl_region_ref *cxl_rr_iter = cxl_rr_load(iter, cxlr);
> > +		struct cxl_decoder *cxld_iter = cxl_rr_iter->decoder;
> > +		u64 cxld_sel;
> > +
> > +		if (cxld_iter->interleave_ways == 1)
> > +			continue;
> > +
> > +		cxld_sel = get_selector(cxld_iter->interleave_ways,
> > +					cxld_iter->interleave_granularity);
> > +
> > +		if (cxld_sel & selector) {
> > +			dev_dbg(&cxlr->dev,
> > +				"%s:%s: overlapping selectors: %#llx:%#llx\n",
> > +				dev_name(iter->uport_dev),
> > +				dev_name(&iter->dev), cxld_sel, selector);
> > +			return -ENXIO;
> > +		}
> > +
> > +		selector |= cxld_sel;
> > +
> > +		parent_distance *= cxl_rr_iter->nr_targets;
> 
> The parent_distance could be derived from the parent selector and
> moved out of the loop to simplify factoring out this into a helper.
> 

For power-of-2 levels, yes, and v3 does that as part of the
helper split. The x3 caveat above applies here too.



> > +		distance *= cxl_rr_iter->nr_targets;
> > +	}
> > +
> > +	distance *= iw;
> 
> The decoder's selectors are the next lower bits below the parent
> selectors, so iw and ig could be directly calculated from that. I
> think that very much simplifies the code.
> 
> There are those 3 selectors we just need to keep track of: switch
> selector, region selector and parent selector. With that everything
> else can be determined, e.g. iw and ig.
> 
> If we create helper functions to get all 3 selectors, this function
> will be much smaller. That also moves away error checks to that
> helpers.

This is the core of the selector-model question answered in Path 3/6 reply.

Short version: v3 takes the helper structure and adds the root decoder's
selectors to the validation walk (your follow-up), but keeps iw/ig/position
derivation in ways/gran value space, because the selector bits cannot
represent the factor 3.

> 
> > +
> > +	/* Accumulated bits must fit within the region selector */
> > +	cxlr_sel = get_selector(p->interleave_ways, p->interleave_granularity);
> > +	if ((cxlr_sel & selector) != selector) {
> > +		dev_dbg(&cxlr->dev,
> > +			"%s:%s: invalid selectors: cxlr %#llx accum %#llx\n",
> > +			dev_name(port->uport_dev), dev_name(&port->dev),
> > +			cxlr_sel, selector);
> > +		return -ENXIO;
> > +	}
> > +
> >  	if (cxl_rr->nr_targets_set) {
> > -		int i, distance = 1;
> > -		struct cxl_region_ref *cxl_rr_iter;
> > -
> > -		/*
> > -		 * The "distance" between peer downstream ports represents which
> > -		 * endpoint positions in the region interleave a given port can
> > -		 * host.
> > -		 *
> > -		 * For example, at the root of a hierarchy the distance is
> > -		 * always 1 as every index targets a different host-bridge. At
> > -		 * each subsequent switch level those ports map every Nth region
> > -		 * position where N is the width of the switch == distance.
> > -		 */
> > -		do {
> > -			cxl_rr_iter = cxl_rr_load(iter, cxlr);
> > -			distance *= cxl_rr_iter->nr_targets;
> > -			iter = to_cxl_port(iter->dev.parent);
> > -		} while (!is_cxl_root(iter));
> > -		distance *= cxlrd->cxlsd.cxld.interleave_ways;
> > -
> > -		for (i = 0; i < cxl_rr->nr_targets_set; i++)
> > +		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);
> > @@ -1484,84 +1545,18 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >  		goto add_target;
> >  	}
> >  
> > -	if (is_cxl_root(parent_port)) {
> > -		/*
> > -		 * Root decoder IG is always set to value in CFMWS which
> > -		 * may be different than this region's IG.  We can use the
> > -		 * region's IG here since interleave_granularity_store()
> > -		 * does not allow interleaved host-bridges with
> > -		 * root IG != region IG.
> > -		 */
> > -		parent_ig = p->interleave_granularity;
> > -		parent_iw = cxlrd->cxlsd.cxld.interleave_ways;
> > -		/*
> > -		 * For purposes of address bit routing, use power-of-2 math for
> > -		 * switch ports.
> > -		 */
> > -		if (!is_power_of_2(parent_iw))
> > -			parent_iw /= 3;
> > -	} else {
> > -		struct cxl_region_ref *parent_rr;
> > -		struct cxl_decoder *parent_cxld;
> > +	ig = p->interleave_granularity * parent_distance;
> >  
> > -		parent_rr = cxl_rr_load(parent_port, cxlr);
> > -		parent_cxld = parent_rr->decoder;
> > -		parent_ig = parent_cxld->interleave_granularity;
> > -		parent_iw = parent_cxld->interleave_ways;
> > -	}
> > -
> > -	rc = granularity_to_eig(parent_ig, &peig);
> > -	if (rc) {
> > -		dev_dbg(&cxlr->dev, "%s:%s: invalid parent granularity: %d\n",
> > -			dev_name(parent_port->uport_dev),
> > -			dev_name(&parent_port->dev), parent_ig);
> > -		return rc;
> > -	}
> > -
> > -	rc = ways_to_eiw(parent_iw, &peiw);
> > -	if (rc) {
> > -		dev_dbg(&cxlr->dev, "%s:%s: invalid parent interleave: %d\n",
> > -			dev_name(parent_port->uport_dev),
> > -			dev_name(&parent_port->dev), parent_iw);
> > -		return rc;
> > -	}
> > -
> > -	iw = cxl_rr->nr_targets;
> >  	rc = ways_to_eiw(iw, &eiw);
> > -	if (rc) {
> > -		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",
> > -			dev_name(port->uport_dev), dev_name(&port->dev), iw);
> > -		return rc;
> > -	}
> > -
> > -	/*
> > -	 * Interleave granularity is a multiple of @parent_port granularity.
> > -	 * Multiplier is the parent port interleave ways.
> > -	 */
> > -	rc = granularity_to_eig(parent_ig * parent_iw, &eig);
> > +	if (!rc)
> > +		rc = granularity_to_eig(ig, &eig);
> >  	if (rc) {
> >  		dev_dbg(&cxlr->dev,
> > -			"%s: invalid granularity calculation (%d * %d)\n",
> > -			dev_name(&parent_port->dev), parent_ig, parent_iw);
> > +			"%s:%s: derived ig %d not a valid granularity (iw %d)\n",
> > +			dev_name(port->uport_dev), dev_name(&port->dev), ig, iw);
> >  		return rc;
> >  	}
> 
> That is just a value check which could be reworked to check the
> selector instead. Could be a helper function.
> 
> We should consider adding the selector to struct of cxld and only
> check it once it is finally converted to ig and iw after setup is
> done.

The check moves into a helper in v3.

About a stored selector in struct cxl_decoder, I don't think it fits
the model. When a consumer show up w need, then consider it.

> 
> >  
> > -	rc = eig_to_granularity(eig, &ig);
> > -	if (rc) {
> > -		dev_dbg(&cxlr->dev, "%s:%s: invalid interleave: %d\n",
> > -			dev_name(port->uport_dev), dev_name(&port->dev),
> > -			256 << eig);
> > -		return rc;
> > -	}
> > -
> > -	if (iw > 8 || iw > cxlsd->nr_targets) {
> > -		dev_dbg(&cxlr->dev,
> > -			"%s:%s:%s: ways: %d overflows targets: %d\n",
> > -			dev_name(port->uport_dev), dev_name(&port->dev),
> > -			dev_name(&cxld->dev), iw, cxlsd->nr_targets);
> > -		return -ENXIO;
> > -	}
> > -
> >  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> >  		if (cxld->interleave_ways != iw ||
> >  		    (iw > 1 && cxld->interleave_granularity != ig) ||
> > @@ -2067,6 +2062,11 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> >  		return -ENXIO;
> >  	}
> >  
> > +	/* Keep this patch bisectable until position arithmetic is updated */
> > +	if (cxlrd->cxlsd.cxld.interleave_granularity >
> > +	    p->interleave_granularity)
> > +		return -ENXIO;
> > +
> 
> Hmm, that looks odd, even if it's only temporary. Could you explain
> why that is needed here.

Your follow-up answered most of this. This patch makes the selector
walk *pass* for mixed-granularity layouts that the old equality rule
rejected, while position arithmetic doesn't handle them until 3/6.
Without the temporary gate, a bisect landing between the two patches
could assemble a mixed-gran region with wrong endpoint positions. The
gate keeps every commit in the series correct on its own. It is removed
in 3/6 when the position math catches up.

I think that's all on this patch.

-- Alison

> 
> Thanks,
> 
> -Robert
> 
> >  	if (p->nr_targets >= p->interleave_ways) {
> >  		dev_dbg(&cxlr->dev, "region already has %d endpoints\n",
> >  			p->nr_targets);
> > -- 
> > 2.37.3
> >