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

Hi Alison,

On 07.07.26 18:43:38, Alison Schofield wrote:

> 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.

But the endpoint position depends only on the factor-of-2 parts of the
lower address bits and includes the root decoder selectors. The
factor-of-3 can just be dropped for position and selector. "stride"
uses a multiplier to describe the positional bit within the total
selector mask of the root decoders. The term "stride" only becomes
clear from detailed reading of the code and is not obvious. It is then
used for position calculations inline, e.g.

 pos_distance = stride > 1 ? 1 : cxlrd->cxlsd.cxld.interleave_ways;

or

 if (dport != cxlrd->cxlsd.target[pos / stride % iw]) {

That is the complexity I mean, I think the code is hard to maintain.

As said, imo, the position can be just directly calculated from the
factor-of-2 portion of the total selector bits without the stride var.

> 
> 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

Stride does not contain that information either as it is always power
of 2.

> 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.

Finally a combination of 'mod iw' is used, which could apply to both
implementations.

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

That could be an improvement, let's see next version. The
implementation should be clear and easily understandable. Right now I
think the selector approach would be better.

Thanks,

-Robert

> 
> -- Alison
> 
> > 
> > -Robert
> > 
> > > 
> > > 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(-)