Re: [PATCH v1 16/29] cxl/region: Use translated HPA ranges to calculate the endpoint position

[Ben Cheatham <benjamin.cheatham@amd.com>]

On 1/7/25 8:10 AM, Robert Richter wrote:
> To enable address translation, the calculation of the endpoint
> position must use translated HPA ranges. The function
> cxl_endpoint_initialize() already uses translation which could be
> reused to calculate the endpoint position.
> 
> Use translated HPA address ranges for the calculation of endpoint

s/HPA address ranges/HPA ranges/

> position by moving it to cxl_endpoint_initialize(). Create a function
> cxl_port_calc_pos() for use in the iterator there, but keep a
> simplified version of cxl_calc_interleave_pos() for the
> non-auto-discovery code path without address translation since it is
> not support there.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/core/region.c | 66 ++++++++++++++++++++++++++-------------
>  1 file changed, 44 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 007a2016760d..c1e384e80d10 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1813,26 +1813,29 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range,
>  }
>  
>  /**
> - * cxl_calc_interleave_pos() - calculate an endpoint position in a region
> - * @cxled: endpoint decoder member of given region
> + * cxl_port_calc_pos() - calculate an endpoint position for @port
> + * @port: Port the new position is calculated for.
> + * @range: The HPA address range for the port.

Same as above, but it may be better to write Host Physical address range instead for
the extra context.

> + * @pos: Current position in the topology.
>   *
> - * The endpoint position is calculated by traversing the topology from
> - * the endpoint to the root decoder and iteratively applying this
> - * calculation:
> + * The endpoint position for the next port is calculated by applying
> + * this calculation:
>   *
>   *    position = position * parent_ways + parent_pos;
>   *
>   * ...where @position is inferred from switch and root decoder target lists.
>   *
> + * The endpoint position of region can be calculated by traversing the

I would word this as "The endpoint's position in a region can be..." instead since
I think that's what you are doing below. This currently reads like the region's
position is being found.

> + * topology from the endpoint to the root decoder and iteratively
> + * applying the function for each port.
> + *
>   * Return: position >= 0 on success
>   *	   -ENXIO on failure
>   */
> -static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
> +static int cxl_port_calc_pos(struct cxl_port *port, struct range *range,
> +			     int pos)
>  {
> -	struct cxl_port *iter, *port = cxled_to_port(cxled);
> -	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> -	struct range *range = &cxled->cxld.hpa_range;
> -	int parent_ways = 0, parent_pos = 0, pos = 0;
> +	int parent_ways = 0, parent_pos = 0;
>  	int rc;
>  
>  	/*
> @@ -1864,17 +1867,30 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
>  	 * complex topologies, including those with switches.
>  	 */
>  
> -	/* Iterate from endpoint to root_port refining the position */
> -	for (iter = port; iter; iter = parent_port_of(iter)) {
> -		if (is_cxl_root(iter))
> -			break;
> +	if (is_cxl_root(port))
> +		return pos;
>  
> -		rc = find_pos_and_ways(iter, range, &parent_pos, &parent_ways);
> -		if (rc)
> -			return rc;
> +	rc = find_pos_and_ways(port, range, &parent_pos, &parent_ways);
> +	if (rc)
> +		return rc;
>  
> -		pos = pos * parent_ways + parent_pos;
> -	}
> +	return pos * parent_ways + parent_pos;
> +}
> +
> +static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
> +{
> +	struct cxl_port *iter, *port = cxled_to_port(cxled);
> +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +	struct range *range = &cxled->cxld.hpa_range;
> +	int pos = 0;
> +
> +	/*
> +	 * Address translation is only supported for auto-discovery of
> +	 * decoders. There is no need to support address translation
> +	 * here.
> +	 */
> +	for (iter = cxled_to_port(cxled); iter; iter = parent_port_of(iter))
> +		pos = cxl_port_calc_pos(iter, range, pos);
>  
>  	dev_dbg(&cxlmd->dev,
>  		"decoder:%s parent:%s port:%s range:%#llx-%#llx pos:%d\n",
> @@ -1892,7 +1908,6 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
>  	for (i = 0; i < p->nr_targets; i++) {
>  		struct cxl_endpoint_decoder *cxled = p->targets[i];
>  
> -		cxled->pos = cxl_calc_interleave_pos(cxled);
>  		/*
>  		 * Record that sorting failed, but still continue to calc
>  		 * cxled->pos so that follow-on code paths can reliably
> @@ -3252,6 +3267,7 @@ static int cxl_endpoint_initialize(struct cxl_endpoint_decoder *cxled)
>  	struct cxl_port *parent, *iter = cxled_to_port(cxled);
>  	struct range hpa = cxled->cxld.hpa_range;
>  	struct cxl_decoder *cxld = &cxled->cxld;
> +	int pos = 0;
>  
>  	if (!iter || is_cxl_root(iter))
>  		return -ENXIO;
> @@ -3280,16 +3296,22 @@ static int cxl_endpoint_initialize(struct cxl_endpoint_decoder *cxled)
>  		if (cxl_port_calc_hpa(parent, cxld, &hpa))
>  			return -ENXIO;
>  
> +		/* Iterate from endpoint to root_port refining the position */
> +		pos = cxl_port_calc_pos(iter, &hpa, pos);
> +		if (pos < 0)
> +			return pos;
> +
>  		iter = parent;
>  	}
>  
>  	dev_dbg(cxld->dev.parent,
> -		"%s:%s: range:%#llx-%#llx\n",
> +		"%s:%s: range:%#llx-%#llx pos:%d\n",
>  		dev_name(&cxled->cxld.dev), dev_name(&cxld->dev),
> -		hpa.start, hpa.end);
> +		hpa.start, hpa.end, pos);
>  
>  	cxled->cxlrd = to_cxl_root_decoder(&cxld->dev);
>  	cxled->spa_range = hpa;
> +	cxled->pos = pos;
>  
>  	return 0;
>  }