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

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

On Mon, Jun 29, 2026 at 08:41:06AM +0200, Robert Richter wrote:
> Alison,
> 
> On 11.06.26 10:47:27, 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.
> 
> I don't see why root decoders need special handling here (apart from
> the 3-way factor, but this is already handled). See my earlier
> comments to the other patches, but if root decoders were just included
> in the pos calculation, that should give correct results. And, instead
> of introducing the "stride" term, why not just continue to work with
> the selector mask also for the root decoders?
> 
> This "special case" handling unnecessarily introduces complex logic
> and code. We should avoid that.

> I don't see why root decoders need special handling here (apart
> from the 3-way factor, but this is already handled). See my
> earlier comments to the other patches, but if root decoders were
> just included in the pos calculation, that should give correct
> results. And, instead of introducing the "stride" term, why not
> just continue to work with the selector mask also for the root
> decoders?
>
> This "special case" handling unnecessarily introduces complex
> logic and code. We should avoid that.


Thanks for the review Robert,

It's not added complexity. It is the root decoder's contribution
to the position, the piece you identified as missing from the
walk in your follow-up on 1/6. Root decoders *are* "just included
in the pos calculation" here: stride = root_gran / region_gran is
the place value of the root's selector bits in position space, so
the root step deposits the root's target index at the root's
place value, same as every other level deposits its index at its
own. It is the same information as the root decoder selectors from
your follow-up on 1/6, just written as a multiplier instead of a
bit mask, which matches the multiply/add arithmetic this function
already uses. Without it, the endpoints get wrong positions while
every selector check still passes.

Nor is it a "special case" of the model. It is the general per-level
rule "each level contributes target_idx * level_gran / region_gran,
which same-granularity levels reduce to trivially. The root is simply
the one level whose granularity can differ from the region's in this
series, so it's the one place the general term is visible.

I did check your selector-bit proposal. A selector-derived position
formula gave identical results to the current code on every pow2 config.
That was equivalent but not simpler. Issue is that it cannot express the
factor of 3 of the 3/6/12-way roots, which consumes no HPA bits and is
recovered by divide/modulo above the pow2 selector bits. So switching to
selector-bit position math would actually require two models: selectors
for pow2, plus the value math kept anyway for 3/6/12-way. The value math
we use now handles both in one model, so position calculation stays there.
Happy to post the comparison traces if useful.

For this patch: stride is renamed and commented as the root's position
place value to make its connection to the selector layout explicit.

-- Alison

> 
> -Robert
> 
> > 
> > Remove the temporary mixed-granularity attach reject now that position
> > arithmetic matches the selector layout.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/core/region.c | 79 ++++++++++++++++++++++++++++-----------
> >  1 file changed, 58 insertions(+), 21 deletions(-)