Re: [PATCH v3 2/9] cxl/region: Validate interleave selector bits

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

On Tue, Aug 18, 2026 at 11:13:18AM +0200, Robert Richter wrote:
> On 30.07.26 15:20:22, Alison Schofield wrote:
> > Each decoder level in an interleave uses a field of host physical
> > address (HPA) bits, called a selector, to choose which of its
> > downstream targets a given address routes to. In a multi-level
> > interleave the selectors of the root, the switches, and the endpoints
> > must occupy distinct address bits so that every level makes an
> > independent routing decision.
> > 
> > The existing setup does not check the selectors directly. It instead
> > requires each level's granularity to equal the parent granularity
> > multiplied by the parent ways, which only holds for a subset of the
> > legal selector layouts. Layouts that place non-overlapping selectors
> > in a different order, as mixed-granularity regions do, are rejected
> > even though they are valid.
> > 
> > Add selector-bit accounting to cxl_port_setup_targets(). Accumulate
> > the selector of each level from the root toward the current port,
> > reject any selector that overlaps a bit already claimed by another
> > level, and reject an accumulated selector that does not fit within
> > the region selector. The root decoder's selector is walked alongside
> > the switch levels rather than tracked separately.
> > 
> > This patch adds selector validation but does not yet enable the
> > mixed-granularity layouts. A temporary gate rejects region granularity
> > finer than the root granularity until the position arithmetic is
> > updated.

Thanks for the review Robert,
I took the simplification comments here more broadly in v4. The selector
accumulation, fanout tracking, and the associated single-use helpers are
all gone. Port decoder setup now derives the needed geometry directly from
the parent granularity and target count.

Replies to the individual comments below.


snip

> > +/**
> > + * get_parent_selectors() - Collect selectors and fan-out above a port
> >   * @parent_port: first ancestor port
> >   * @cxlr: region under construction
> > + * @cxlrd: region root decoder
> > + * @accum: filled with the selectors claimed by the ancestors
> >   * @fanout: filled with the product of the ancestor switch ways
> >   *
> > - * Walk from @parent_port to the root and multiply the interleave ways of
> > - * each switch decoder. Root decoder ways are not included.
> > + * Start with the root decoder selector and walk the ancestor switch
> > + * decoders. Reject selectors that overlap. Passthrough decoders contribute
> > + * neither selector bits nor fan-out.
> >   *
> > - * Return: 0 on success.
> > + * Root decoder ways are not included in @fanout.
> > + *
> > + * Return: 0 on success, -ENXIO on selector overlap.
> >   */
> 
> See my comment on comments in the previous patch. It should not be
> documented.


Agreed. This helper and its kernel-doc are gone in v4.

snip
> > +/**
> > + * region_selectors_fit() - Check ancestor selectors against the region
> > + * @port: port being configured
> > + * @cxlr: region under construction
> > + * @accum: selectors used by the ancestor decoders
> > + *
> > + * Return: true when every accumulated selector bit is present in the region
> > + * selector.
> > + */
> > +static bool region_selectors_fit(struct cxl_port *port,
> > +				 struct cxl_region *cxlr, u64 accum)
> 
> Better have this inline in the code, this functions does not help
> much.

This helper is gone in v4. The selector-fit validation it was wrapping
is gone as well with the selector-walk approach.

snip

> > @@ -1517,6 +1594,7 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >  	int ig, iw = cxl_rr->nr_targets;
> >  	int fanout, rc;
> >  	int pos = cxled->pos;
> > +	u64 accum;
> 
> rename: "total_sel", "parent_sel", ...?

Agree that accum was not very descriptive. The accumulated selector
state is gone in v4, so the variable disappears with it.


> 
> >  	u16 eig;
> >  	u8 eiw;
> >  
> > @@ -1538,10 +1616,13 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >  		return -ENXIO;
> >  	}
> >  
> > -	rc = get_parent_fanout(parent_port, cxlr, &fanout);
> > +	rc = get_parent_selectors(parent_port, cxlr, cxlrd, &accum, &fanout);
> 
> An easy and straight for loop could be a good alternative.

Yes. This was another sign that the helper structure was getting in the
way. v4 drops this ancestor selector walk entirely rather than moving it
back inline.


> 
> fanout can be dropped from the function interface. Weight of the
> selector can be used instead.
> 

The fanout state is gone in v4, along with the selector accumulation.
I did not replace it with selector weight, though. The v4 approach
derives each decoder's granularity directly from its parent granularity
and target count, which also accounts for the mod3 configurations.


> >  	if (rc)
> >  		return rc;
> >  
> > +	if (!region_selectors_fit(port, cxlr, accum))
> > +		return -ENXIO;
> > +
> 
> Without a helper it is actually better readable:
> 
> 	cxlr_sel = ...;
> 	total_sel = ...;
> 
> 	if ((cxlr_sel & total_sel) != total_sel) {
> 		...
> 
> So, only use helpers if really needed. And, assign values directly.


Agreed on the larger point. v4 removes these single-use helpers and the
intermediate selector state rather than trying to reorganize them. The
target setup is now much closer to the original flow.


> 
> >  	if (cxl_rr->nr_targets_set) {
> >  		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
> >  			if (ep->dport == cxlsd->target[i]) {
> > @@ -2087,6 +2168,14 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> >  		return -ENXIO;
> >  	}
> >  
> > +	/*
> > +	 * Mixed-granularity position calculation is added by the next patch.
> > +	 * Reject it until then so this intermediate state remains bisectable.
> > +	 */
> > +	if (cxlrd->cxlsd.cxld.interleave_granularity >
> > +	    p->interleave_granularity)
> > +		return -ENXIO;
> > +
> 
> That change did not remove anything that would make existing code
> break, right? It just adds selector calculation. I don't see a need
> for this additional check.

The check was needed to keep this intermediate commit bisectable.
This patch relaxes validation enough that mixed-granularity attach can
proceed before the position calculation supports it.

The v4 set avoids that intermediate state, so the gate is no longer
needed.

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