RE: [PATCH] cxl: set root decoder granularity based on region params

[Dan Williams <dan.j.williams@intel.com>]

Jim Harris wrote:
> The granularity for downstream targets is all based on descending the
> topology from the root. So we need to set the root decoder's granularity
> based on the region params.
> 
> Test configuration is 1 host bridge * 2 switches * 2 endpoints per switch.
> 
> Region created with 2048 granularity using following command line:
> 
> cxl create-region -m -d decoder0.0 -w 4 mem0 mem2 mem1 mem3 \
> 		  -g 2048 -s 2048M
> 
> Use "cxl list -PDE | grep granularity" to get a view of the granularity
> set at each level of the topology.
> 
> Before:
>         "interleave_granularity":2048,
>         "interleave_granularity":2048,
>     "interleave_granularity":512,
>         "interleave_granularity":2048,
>         "interleave_granularity":2048,
>     "interleave_granularity":512,
> "interleave_granularity":256,
> 
> After:
>         "interleave_granularity":2048,
>         "interleave_granularity":2048,
>     "interleave_granularity":4096,
>         "interleave_granularity":2048,
>         "interleave_granularity":2048,
>     "interleave_granularity":4096,
> "interleave_granularity":2048,
> 
> Signed-off-by: Jim Harris <jim.harris@samsung.com>
> ---
>  drivers/cxl/core/region.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6d63b8798c29..f14110e35f79 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1691,6 +1691,11 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		return -EINVAL;
>  	}
>  
> +	/* Set root decoder's granularity now, so that we can use it to calculate
> +	 * granularity for the downstream targets in cxl_region_setup_targets().
> +	 */

Minor, and I mean minor, nit, the cxl subsystem is using:

/*
 * Comment...
 */

...block-comment style. Yes, it varies across subsystems, yes it's
arbitrary, but general rule is follow local customs.


> +	cxlrd->cxlsd.cxld.interleave_granularity = cxlr->params.interleave_granularity;
> +

So I think this is only valid in the cxlrd->cxlsd.cxld.interleave_ways == 1
case as interleave_granularity_store() forbids regions that do not match
the granularity of the root.

If a BIOS tries to ship such a config in production that's when I expect
that policy needs to be revisited, but outside of being forced to
reconsider that stance the complexity reduction is the benefit.

In the meantime maybe add an "effective granularity" concept for x1 root
decoders and x1 switches so the math can use that value?