Re: [PATCH v5 06/12] cxl: Split out host bridge access coordinates

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

On Tue, 6 Feb 2024 15:28:34 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> The difference between access class 0 and access class 1 for 'struct
> access_coordinate', if any, is that class 0 is for the distance from
> the target to the closest initiator and that class 1 is for the distance
> from the target to the cloest CPU. For CXL memory, the nearest initiator

spell check.

> may not necessarily be a CPU node. The performance path from the CXL
> endpoint to the host bridge should remain the same. However, the numbers
> extracted and stored from HMAT is the difference for the two access
> classes. Split out the performance numbers for the host bridge (generic
> target) from the calculation of the entire path in order to allow
> calculation of both access classes for a CXL region.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

one suggestion on function comment.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/cdat.c | 28 ++++++++++++++++++++++------
>  drivers/cxl/core/port.c | 34 +++++++++++++++++++++++++++++++---
>  drivers/cxl/cxl.h       |  2 ++
>  3 files changed, 55 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 096320390ad9..79844874a34b 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -162,15 +162,22 @@ static int cxl_cdat_endpoint_process(struct cxl_port *port,
>  static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  					struct xarray *dsmas_xa)
>  {
> -	struct access_coordinate c;
> +	struct access_coordinate ep_c;
> +	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
>  	struct dsmas_entry *dent;
>  	int valid_entries = 0;
>  	unsigned long index;
>  	int rc;
>  
> -	rc = cxl_endpoint_get_perf_coordinates(port, &c);
> +	rc = cxl_endpoint_get_perf_coordinates(port, &ep_c);
>  	if (rc) {
> -		dev_dbg(&port->dev, "Failed to retrieve perf coordinates.\n");
> +		dev_dbg(&port->dev, "Failed to retrieve ep perf coordinates.\n");
> +		return rc;
> +	}
> +
> +	rc = cxl_hb_get_perf_coordinates(port, coord);
> +	if (rc)  {
> +		dev_dbg(&port->dev, "Failed to retrieve hb perf coordinates.\n");
>  		return rc;
>  	}
>  
> @@ -185,10 +192,19 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  	xa_for_each(dsmas_xa, index, dent) {
>  		int qos_class;
>  
> -		cxl_coordinates_combine(&dent->coord, &dent->coord, &c);
> +		cxl_coordinates_combine(&dent->coord, &dent->coord, &ep_c);
> +		/*
> +		 * Keeping the host bridge coordinates separate from the dsmas
> +		 * coordinates in order to allow calculation of access class
> +		 * 0 and 1 for region later.
> +		 */
> +		cxl_coordinates_combine(&coord[ACCESS_COORDINATE_LOCAL],
> +					&coord[ACCESS_COORDINATE_LOCAL],
> +					&dent->coord);
>  		dent->entries = 1;
> -		rc = cxl_root->ops->qos_class(cxl_root, &dent->coord, 1,
> -					      &qos_class);
> +		rc = cxl_root->ops->qos_class(cxl_root,
> +					      &coord[ACCESS_COORDINATE_LOCAL],
> +					      1, &qos_class);
>  		if (rc != 1)
>  			continue;
>  
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index af9458b2678c..e8029170b8c6 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -2096,6 +2096,37 @@ bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd)
>  }
>  EXPORT_SYMBOL_NS_GPL(schedule_cxl_memdev_detach, CXL);
>  
> +/**
> + * cxl_hb_get_perf_coordinates - Retrieve performance numbers from host bridge

This description confused me.  What numbers does the host bridge have?

between initiator and host bridge
perhaps?

> + *
> + * @port: endpoint cxl_port
> + * @coord: output access coordinates
> + *
> + * Return: errno on failure, 0 on success.
> + */
> +int cxl_hb_get_perf_coordinates(struct cxl_port *port,
> +				struct access_coordinate *coord)
> +{
> +	struct cxl_port *iter = port;
> +	struct cxl_dport *dport;
> +
> +	if (!is_cxl_endpoint(port))
> +		return -EINVAL;
> +
> +	dport = iter->parent_dport;
> +	while (iter && !is_cxl_root(to_cxl_port(iter->dev.parent))) {
> +		iter = to_cxl_port(iter->dev.parent);
> +		dport = iter->parent_dport;
> +	}
> +
> +	coord[ACCESS_COORDINATE_LOCAL] =
> +		dport->hb_coord[ACCESS_COORDINATE_LOCAL];
> +	coord[ACCESS_COORDINATE_CPU] =
> +		dport->hb_coord[ACCESS_COORDINATE_CPU];
> +
> +	return 0;
> +}
> +
>  /**
>   * cxl_endpoint_get_perf_coordinates - Retrieve performance numbers stored in dports
>   *				   of CXL path
> @@ -2137,9 +2168,6 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>  		dport = iter->parent_dport;
>  	}
>  
> -	/* Augment with the generic port (host bridge) perf data */
> -	cxl_coordinates_combine(&c, &c, &dport->hb_coord[ACCESS_COORDINATE_LOCAL]);
> -
>  	/* Get the calculated PCI paths bandwidth */
>  	pdev = to_pci_dev(port->uport_dev->parent);
>  	bw = pcie_bandwidth_available(pdev, NULL, NULL, NULL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index fab2da4b1f04..de477eb7f5d5 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -879,6 +879,8 @@ void cxl_switch_parse_cdat(struct cxl_port *port);
>  
>  int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>  				      struct access_coordinate *coord);
> +int cxl_hb_get_perf_coordinates(struct cxl_port *port,
> +				struct access_coordinate *coord);
>  
>  void cxl_memdev_update_perf(struct cxl_memdev *cxlmd);
>