Re: [PATCH v2 10/15] cxl/region: Use root decoders interleaving parameters to create a region

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 18 Feb 2025 14:23:51 +0100
Robert Richter <rrichter@amd.com> wrote:

> Endpoints requiring address translation might not be aware of the
> system's interleaving configuration. Instead, interleaving can be
> configured on an upper memory domain (from an endpoint view) and thus
> is not visible to the endpoint. For region creation this might cause
> an invalid interleaving config that does not match the CFMWS entries.
> 
> Use the interleaving configuration of the root decoders to create a
> region which bases on CFMWS entries. This always matches the system's
> interleaving configuration and is independent of the underlying memory
> topology.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/core/region.c | 39 ++++++++++++++++++++++++++++++++++-----
>  drivers/cxl/cxl.h         |  2 ++
>  2 files changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6e0434eee6df..3afcc9ca06ae 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1749,6 +1749,15 @@ static int cxl_region_validate_position(struct cxl_region *cxlr,
>  		}
>  	}
>  
> +	if (p->interleave_ways != cxled->interleave_ways ||
> +	    p->interleave_granularity != cxled->interleave_granularity ) {
> +		dev_dbg(&cxlr->dev, "interleaving config mismatch with %s: ways: %d:%d granularity: %d:%d\n",
> +			dev_name(&cxled->cxld.dev), p->interleave_ways,
> +			cxled->interleave_ways, p->interleave_granularity,
> +			cxled->interleave_granularity);
> +		return -ENXIO;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1852,7 +1861,7 @@ static int match_switch_decoder_by_range(struct device *dev,
>  }
>  
>  static int find_pos_and_ways(struct cxl_port *port, struct range *range,
> -			     int *pos, int *ways)
> +			     int *pos, int *ways, int *granularity)
>  {
>  	struct cxl_switch_decoder *cxlsd;
>  	struct cxl_port *parent;
> @@ -1873,6 +1882,7 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range,
>  	}
>  	cxlsd = to_cxl_switch_decoder(dev);
>  	*ways = cxlsd->cxld.interleave_ways;
> +	*granularity = cxlsd->cxld.interleave_granularity;
>  
>  	for (int i = 0; i < *ways; i++) {
>  		if (cxlsd->target[i] == port->parent_dport) {
> @@ -1896,6 +1906,8 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range,
>  struct cxl_interleave_context {
>  	struct range *hpa_range;
>  	int pos;
> +	int interleave_ways;
> +	int interleave_granularity;

Ah. And here is our context expansion

>  };
>  
>  /**
> @@ -1914,13 +1926,17 @@ struct cxl_interleave_context {
>   * the topology from the endpoint to the root decoder and iteratively
>   * applying the function for each port.
>   *
> + * Calculation of interleaving ways:
> + *
> + *    interleave_ways = interleave_ways * parent_ways;
> + *
>   * Return: position >= 0 on success
>   *	   -ENXIO on failure
>   */
>  static int cxl_port_calc_interleave(struct cxl_port *port,
>  				    struct cxl_interleave_context *ctx)
>  {
> -	int parent_ways = 0, parent_pos = 0;
> +	int parent_ways = 0, parent_pos = 0, parent_granularity = 0;
>  	int rc;
>  
>  	/*
> @@ -1955,12 +1971,23 @@ static int cxl_port_calc_interleave(struct cxl_port *port,
>  	if (is_cxl_root(port))
>  		return 0;
>  
> -	rc = find_pos_and_ways(port, ctx->hpa_range, &parent_pos, &parent_ways);
> +	rc = find_pos_and_ways(port, ctx->hpa_range, &parent_pos, &parent_ways,
> +			&parent_granularity);
>  	if (rc)
>  		return rc;
>  
>  	ctx->pos = ctx->pos * parent_ways + parent_pos;
>  
> +	if (ctx->interleave_ways)
> +		ctx->interleave_ways *= parent_ways;
> +	else
> +		ctx->interleave_ways = parent_ways;
> +
> +	if (ctx->interleave_granularity)
> +		ctx->interleave_granularity *= ctx->interleave_ways;
> +	else
> +		ctx->interleave_granularity = parent_granularity;
> +
>  	return ctx->pos;

I think Gregory called this out in earlier patch.  Mixing and matching
between returning pos and use of ctx makes things hard to read.  If
we need to have it in context, then make this return 0 or -ERR instead.

>  }