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

On Tue, 16 Jun 2026 20:10:23 -0700
Alison Schofield <[email protected]> wrote:

> On Fri, Jun 12, 2026 at 02:21:07PM +0800, Richard Cheng wrote:
> > On Thu, Jun 11, 2026 at 10:47:27AM +0800, Alison Schofield wrote:  
> > > Mixed-granularity regions place the root decoder at outer HPA bits.
> > > A root target then owns root_gran / region_gran inner positions
> > > before the root advances to the next target. That ratio becomes the
> > > root target stride in region positions.
> > > 
> > > Account for that stride in endpoint placement, position calculation,
> > > and peer-distance checks.
> > > 
> > > Remove the temporary mixed-granularity attach reject now that position
> > > arithmetic matches the selector layout.
> > > 
> > > Signed-off-by: Alison Schofield <[email protected]>
> > > ---
> > >  drivers/cxl/core/region.c | 79 ++++++++++++++++++++++++++++-----------
> > >  1 file changed, 58 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > > index d5d19ed62171..9fff538a9ce1 100644
> > > --- a/drivers/cxl/core/region.c
> > > +++ b/drivers/cxl/core/region.c
> > > @@ -1444,12 +1444,34 @@ static inline u64 get_selector(int ways, int gran)
> > >  	return (u64)(ways - 1) * gran;
> > >  }
> > >  
> > > +/**
> > > + * root_pos_stride() - Return root target stride in region positions
> > > + * @cxlr: region
> > > + *
> > > + * Return: root_gran / region_gran when the root granularity is larger
> > > + * than the region granularity, or 1 otherwise.
> > > + */
> > > +static inline int root_pos_stride(struct cxl_region *cxlr)
> > > +{
> > > +	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> > > +	int root_gran = cxlrd->cxlsd.cxld.interleave_granularity;
> > > +	int region_gran = cxlr->params.interleave_granularity;
> > > +
> > > +	if (cxlrd->cxlsd.cxld.interleave_ways <= 1)
> > > +		return 1;
> > > +
> > > +	if (region_gran == 0 || root_gran <= region_gran)
> > > +		return 1;
> > > +
> > > +	return root_gran / region_gran;
> > > +}
> > > +  
> > 
> > Hi Alison,
> > 
> > This helper computes stride from gran ratio, but nothing from downstream
> > checks that the stride equals the number of endpoints actually fanned out
> > under one root target.
> > The patch assumes each root target each own stride consecutive region position,
> > when that does not hold, the additive recurrence below and peer-distance seed
> > both mis-account, and we walk off into a foreign endpoint.
> >  
> 
> addressed below...
> 
> 
> snip
> 
> > > @@ -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;
> > >  	}
> > >   
> > 
> > I have a question here, on AUTO path, the check are bitwise and order-independent,
> > so they accept it, then pos comes out d*2+u+4*hb where HPA decodes reads d+2*u+4*hb,
> > so 6 of 12 endpoints get the wrong pos and the region is rejected or mis-mapped.
> > 
> > If my inference is true, it contradicts the cover-letter's "every arrangement" claim.
> > 
> > --Richard  
> 
> Hi Richard,
> 
> (Oh, this got long! Spoiler, I'm proposing we limit to configs where
> root->endpoint gran is only coarser->finer, ie. monotonically
> decreasing. No coarser->finer->coarser(again))
> 
> Thanks for digging into the position arithmetic!  You are right, and your
> two points land on the same root cause that Sashiko raised from the
> peer-distance side. The position calculator assumes every level slices
> inside the chunk of the level above it, root carving the biggest pieces
> and each switch below it slicing finer. When that is true, the math is
> correct. So courser->finer granularity as we go from root to endpoints
> all is good!
> 
> However, when a switch slices at a coarser granularity than the level
> above it, the bits are out of order, the calculator places those
> endpoints wrong, and the region is either rejected or mapped wrong. The
> selector checks don't catch it because they only look at which bits are
> used, not their order, exactly as you described.
> 
> First I revisited the CXL spec. The spec does not explicitly forbid
> courser->finer->courser? It doesn't. Section 9.13.1 only requires that
> the levels use different, consecutive HPA bits. It doesn't actually mandate
> an ordering, so the out-of-order layouts are legal.
> 
> I looked at the CXL Spec and the Driver Writers Guide examples. Every
> example is coarse->fine:
> CXL Spec Figures 9-16, 9-17, 9-18, and SW Guide 2.7.2, 2.7.3, 2.7.4
> 
> The CXL spec enumeration tables go further. Table 9-16 (a 12 way) lists an
> out of order ordering, but that has an equivalent coarse->fine config that
> produces the same results.

Same result in some sense, but not for what pattern goes down a given
link.

> 
> The gist of all that spec talk, is that I cannot say we won't add the
> suppport based on what the spec says.
> 
> Next, I considered teaching the calculator to handle arbitrary bit
> ordering, but I don't think it's the right thing for this series. It
> would mean reworking the core position calculation to gain a case that
> nothing, no spec figure, no table, no firmware I'm aware of, actually
> produces, and that users can already reach another way.
> 
> So what I'd like to do instead is define the scope explicitly as support
> for coarse-to-fine mixed-granularity. That is everything this series set
> out to enable, and a clear superset of today's behavior, where any
> region granularity different from the root's was rejected outright.
> Gracefully fail anything out-of-order with a clear error rather than
> letting it mis-map or oops.
> 
> I also believe it meets Robert Richters (AMD) needs because that original
> patch did not add that support in the selector bit accounting.
> Find link to Roberts original patch in cover letter.
> 
> Does a clean rejection of the out-of-order case work for you?
> 
> Other folks?

To cut to the chase, I don't personally care if we limit the
non fine to coarse cases.

It's not hard to come up with a hardware design where say
128 / 64 / 256 makes sense.

The coarse at the top would be the host wanting to use most efficient
size for the impdef interconnect pre CXL.

With that in mind the system designer may then want to get the overall
granularity down as quick as possible to reduce hot spots on the root
ports where contention is likely to be highest. So splits things
down to 64 bytes.

When we reach the switch, 64 and 128 are gone, so best it can do is 256.

Anyhow, this is one of those - for completeness - messages, if anyone
really really cares then can rework things again!

Jonathan



> 
> 
> -- Alison
> 
>