[PATCH v7 0/5] cxl: access_coordinate validity fixes for 6.9

[PATCH v7 0/5] cxl: access_coordinate validity fixes for 6.9

[Dave Jiang]

v7:
- Add bug fix for not skipping RP coords that don't exist. (Jonathan)
- Remove min_not_zero() as it doesn't apply here. DIV_ROUNDUP() is sufficient. (Jonathan)
- Reflowed patch ordering for easier backport.

A group of fixes to make CXL access_coordinate calculation function
properly.

[1/5] cxl: Remove checking of iter in cxl_endpoint_get_perf_coordinates()
[2/5] cxl: Fix retrieving of access_coordinates in PCIe path
[3/5] cxl: Fix incorrect region perf data calculation
[4/5] cxl: Consolidate dport access_coordinate ->hb_coord and ->sw_coord into ->coord
[5/5] cxl: Add checks to access_coordinate calculation to fail missing data

[PATCH v7 1/5] cxl: Remove checking of iter in cxl_endpoint_get_perf_coordinates()

[Dave Jiang]

The while() loop in cxl_endpoint_get_perf_coordinates() checks to see if
'iter' is valid as part of the condition breaking out of the loop.
is_cxl_root() will stop the loop before the next iteration could go NULL.
Remove the iter check.

The presence of the iter or removing the iter does not impact the behavior
of the code. This is a code clean up and not a bug fix.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 2b0cab556072..6cbde50a742b 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -2197,7 +2197,7 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 	 * port each iteration. If the parent is cxl root then there is
 	 * nothing to gather.
 	 */
-	while (iter && !is_cxl_root(to_cxl_port(iter->dev.parent))) {
+	while (!is_cxl_root(to_cxl_port(iter->dev.parent))) {
 		cxl_coordinates_combine(&c, &c, &dport->sw_coord);
 		c.write_latency += dport->link_latency;
 		c.read_latency += dport->link_latency;
-- 
2.44.0

Re: [PATCH v7 1/5] cxl: Remove checking of iter in cxl_endpoint_get_perf_coordinates()

[Robert Richter]

On 03.04.24 08:47:12, Dave Jiang wrote:
> The while() loop in cxl_endpoint_get_perf_coordinates() checks to see if
> 'iter' is valid as part of the condition breaking out of the loop.
> is_cxl_root() will stop the loop before the next iteration could go NULL.
> Remove the iter check.
> 
> The presence of the iter or removing the iter does not impact the behavior
> of the code. This is a code clean up and not a bug fix.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/port.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 2b0cab556072..6cbde50a742b 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -2197,7 +2197,7 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>  	 * port each iteration. If the parent is cxl root then there is
>  	 * nothing to gather.
>  	 */
> -	while (iter && !is_cxl_root(to_cxl_port(iter->dev.parent))) {
> +	while (!is_cxl_root(to_cxl_port(iter->dev.parent))) {

I am seeing the following mainline after [1]:

 [   39.815379] cxl_acpi ACPI0017:00: not a cxl_port device
 [   39.827123] WARNING: CPU: 46 PID: 1754 at drivers/cxl/core/port.c:592 to_cxl_port+0x56/0x70 [cxl_core]

... plus some related subsequent NULL pointer dereference:

 [   40.718708] BUG: kernel NULL pointer dereference, address: 00000000000002d8

This changes looks related. I am going to dig deeper here but just a
headsup in advance.

Note this is tested on an RCH topology.

Thanks,

-Robert

[1] commit 586b5dfb51b9 ("Merge tag 'cxl-fixes-6.9-rc4' of
    git://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl"):

>  		cxl_coordinates_combine(&c, &c, &dport->sw_coord);
>  		c.write_latency += dport->link_latency;
>  		c.read_latency += dport->link_latency;
> -- 
> 2.44.0
> 

Re: [PATCH v7 1/5] cxl: Remove checking of iter in cxl_endpoint_get_perf_coordinates()

[Dave Jiang]

On 4/26/24 12:10 PM, Robert Richter wrote:
> On 03.04.24 08:47:12, Dave Jiang wrote:
>> The while() loop in cxl_endpoint_get_perf_coordinates() checks to see if
>> 'iter' is valid as part of the condition breaking out of the loop.
>> is_cxl_root() will stop the loop before the next iteration could go NULL.
>> Remove the iter check.
>>
>> The presence of the iter or removing the iter does not impact the behavior
>> of the code. This is a code clean up and not a bug fix.
>>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
>> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  drivers/cxl/core/port.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 2b0cab556072..6cbde50a742b 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -2197,7 +2197,7 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>>  	 * port each iteration. If the parent is cxl root then there is
>>  	 * nothing to gather.
>>  	 */
>> -	while (iter && !is_cxl_root(to_cxl_port(iter->dev.parent))) {
>> +	while (!is_cxl_root(to_cxl_port(iter->dev.parent))) {
> 
> I am seeing the following mainline after [1]:
> 
>  [   39.815379] cxl_acpi ACPI0017:00: not a cxl_port device
>  [   39.827123] WARNING: CPU: 46 PID: 1754 at drivers/cxl/core/port.c:592 to_cxl_port+0x56/0x70 [cxl_core]
> 
> ... plus some related subsequent NULL pointer dereference:
> 
>  [   40.718708] BUG: kernel NULL pointer dereference, address: 00000000000002d8
> 
> This changes looks related. I am going to dig deeper here but just a
> headsup in advance.
> 
> Note this is tested on an RCH topology.

Hi Robert,
Can you please provide the 'ls -l /sys/bus/cxl/devices/$memX' of the RCH device? Maybe the iterator may need to be tweaked for RCH topology support.

> 
> Thanks,
> 
> -Robert
> 
> [1] commit 586b5dfb51b9 ("Merge tag 'cxl-fixes-6.9-rc4' of
>     git://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl"):
> 
>>  		cxl_coordinates_combine(&c, &c, &dport->sw_coord);
>>  		c.write_latency += dport->link_latency;
>>  		c.read_latency += dport->link_latency;
>> -- 
>> 2.44.0
>>
> 

[PATCH v7 2/5] cxl: Fix retrieving of access_coordinates in PCIe path

[Dave Jiang]

Current loop in cxl_endpoint_get_perf_coordinates() incorrectly assumes
the Root Port (RP) dport is the one with generic port access_coordinate.
However those coordinates are one level up in the Host Bridge (HB).
Current code causes the computation code to pick up 0s as the coordinates
and cause minimal bandwidth to result in 0.

Add check to skip RP when combining coordinates.

Fixes: 4d59ba915318 ("cxl: Fix retrieving of access_coordinates in PCIe path")
Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/port.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 6cbde50a742b..7aadcec4fc64 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -2165,6 +2165,11 @@ int cxl_hb_get_perf_coordinates(struct cxl_port *port,
 	return 0;
 }
 
+static bool parent_port_is_cxl_root(struct cxl_port *port)
+{
+	return is_cxl_root(to_cxl_port(port->dev.parent));
+}
+
 /**
  * cxl_endpoint_get_perf_coordinates - Retrieve performance numbers stored in dports
  *				   of CXL path
@@ -2184,27 +2189,31 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 	struct cxl_dport *dport;
 	struct pci_dev *pdev;
 	unsigned int bw;
+	bool is_cxl_root;
 
 	if (!is_cxl_endpoint(port))
 		return -EINVAL;
 
-	dport = iter->parent_dport;
-
 	/*
-	 * Exit the loop when the parent port of the current port is cxl root.
-	 * The iterative loop starts at the endpoint and gathers the
-	 * latency of the CXL link from the current iter to the next downstream
-	 * port each iteration. If the parent is cxl root then there is
-	 * nothing to gather.
+	 * Exit the loop when the parent port of the current iter port is cxl
+	 * root. The iterative loop starts at the endpoint and gathers the
+	 * latency of the CXL link from the current device/port to the connected
+	 * downstream port each iteration.
 	 */
-	while (!is_cxl_root(to_cxl_port(iter->dev.parent))) {
-		cxl_coordinates_combine(&c, &c, &dport->sw_coord);
+	do {
+		dport = iter->parent_dport;
+		iter = to_cxl_port(iter->dev.parent);
+		is_cxl_root = parent_port_is_cxl_root(iter);
+
+		/*
+		 * There's no valid access_coordinate for a root port since RPs do not
+		 * have CDAT and therefore needs to be skipped.
+		 */
+		if (!is_cxl_root)
+			cxl_coordinates_combine(&c, &c, &dport->sw_coord);
 		c.write_latency += dport->link_latency;
 		c.read_latency += dport->link_latency;
-
-		iter = to_cxl_port(iter->dev.parent);
-		dport = iter->parent_dport;
-	}
+	} while (!is_cxl_root);
 
 	/* Get the calculated PCI paths bandwidth */
 	pdev = to_pci_dev(port->uport_dev->parent);
-- 
2.44.0

Re: [PATCH v7 2/5] cxl: Fix retrieving of access_coordinates in PCIe path

[Jonathan Cameron]

On Wed, 3 Apr 2024 08:47:13 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Current loop in cxl_endpoint_get_perf_coordinates() incorrectly assumes
> the Root Port (RP) dport is the one with generic port access_coordinate.
> However those coordinates are one level up in the Host Bridge (HB).
> Current code causes the computation code to pick up 0s as the coordinates
> and cause minimal bandwidth to result in 0.
> 
> Add check to skip RP when combining coordinates.
> 
> Fixes: 4d59ba915318 ("cxl: Fix retrieving of access_coordinates in PCIe path")
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

I've only tested this with direct connect, but can't see why it wouldn't work
with switches in the path. So

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

> ---
>  drivers/cxl/core/port.c | 35 ++++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 6cbde50a742b..7aadcec4fc64 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -2165,6 +2165,11 @@ int cxl_hb_get_perf_coordinates(struct cxl_port *port,
>  	return 0;
>  }
>  
> +static bool parent_port_is_cxl_root(struct cxl_port *port)
> +{
> +	return is_cxl_root(to_cxl_port(port->dev.parent));
> +}
> +
>  /**
>   * cxl_endpoint_get_perf_coordinates - Retrieve performance numbers stored in dports
>   *				   of CXL path
> @@ -2184,27 +2189,31 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>  	struct cxl_dport *dport;
>  	struct pci_dev *pdev;
>  	unsigned int bw;
> +	bool is_cxl_root;
>  
>  	if (!is_cxl_endpoint(port))
>  		return -EINVAL;
>  
> -	dport = iter->parent_dport;
> -
>  	/*
> -	 * Exit the loop when the parent port of the current port is cxl root.
> -	 * The iterative loop starts at the endpoint and gathers the
> -	 * latency of the CXL link from the current iter to the next downstream
> -	 * port each iteration. If the parent is cxl root then there is
> -	 * nothing to gather.
> +	 * Exit the loop when the parent port of the current iter port is cxl
> +	 * root. The iterative loop starts at the endpoint and gathers the
> +	 * latency of the CXL link from the current device/port to the connected
> +	 * downstream port each iteration.
>  	 */
> -	while (!is_cxl_root(to_cxl_port(iter->dev.parent))) {
> -		cxl_coordinates_combine(&c, &c, &dport->sw_coord);
> +	do {
> +		dport = iter->parent_dport;
> +		iter = to_cxl_port(iter->dev.parent);
> +		is_cxl_root = parent_port_is_cxl_root(iter);
> +
> +		/*
> +		 * There's no valid access_coordinate for a root port since RPs do not
> +		 * have CDAT and therefore needs to be skipped.
> +		 */
> +		if (!is_cxl_root)
> +			cxl_coordinates_combine(&c, &c, &dport->sw_coord);
>  		c.write_latency += dport->link_latency;
>  		c.read_latency += dport->link_latency;
> -
> -		iter = to_cxl_port(iter->dev.parent);
> -		dport = iter->parent_dport;
> -	}
> +	} while (!is_cxl_root);
>  
>  	/* Get the calculated PCI paths bandwidth */
>  	pdev = to_pci_dev(port->uport_dev->parent);

RE: [PATCH v7 2/5] cxl: Fix retrieving of access_coordinates in PCIe path

[Dan Williams]

Dave Jiang wrote:
> Current loop in cxl_endpoint_get_perf_coordinates() incorrectly assumes
> the Root Port (RP) dport is the one with generic port access_coordinate.
> However those coordinates are one level up in the Host Bridge (HB).
> Current code causes the computation code to pick up 0s as the coordinates
> and cause minimal bandwidth to result in 0.
> 
> Add check to skip RP when combining coordinates.
> 
> Fixes: 4d59ba915318 ("cxl: Fix retrieving of access_coordinates in PCIe path")
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Looks ok to me, and I see the Generic Port details are considered right
after this walk to the root-port.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

[PATCH v7 3/5] cxl: Fix incorrect region perf data calculation

[Dave Jiang]

Current math in cxl_region_perf_data_calculate divides the latency by 1000
every time the function gets called. This causes the region latency to be
divided by 1000 per memory device and the math is incorrect. This is user
visible as the latency access_coordinate exposed via sysfs will show
incorrect latency data.

Normalize values from CDAT to nanoseconds. Adjust sub-nanoseconds latency
to at least 1. Remove adjustment of perf numbers from the generic target
since hmat handling code has already normalized those numbers. Now all
computation and stored numbers should be in nanoseconds.

cxl_hb_get_perf_coordinates() is removed and HB coords are calculated
in the port access_coordinate calculation path since it no longer need
to be treated special.

Fixes: 3d9f4a197230 ("cxl/region: Calculate performance data for a region")
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v7:
- Remove min_not_zero(). Incorrectly set everything to 1. DIV_ROUNDUP()
  will ensure sub-nanoseconds values not 0 unless value 0 to begin with.
  (Jonathan)
- Reflowed patch order
- Remove cxl_hb_get_perf_coordinates() as change made function unnessary.
- Add hb access_coordinate back to port caclculation.
---
 drivers/cxl/acpi.c      | 13 +-----
 drivers/cxl/core/cdat.c | 89 ++++++++++++++++-------------------------
 drivers/cxl/core/port.c | 36 ++---------------
 drivers/cxl/cxl.h       |  2 -
 4 files changed, 40 insertions(+), 100 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index af5cb818f84d..566c387d4385 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -525,22 +525,11 @@ static int get_genport_coordinates(struct device *dev, struct cxl_dport *dport)
 {
 	struct acpi_device *hb = to_cxl_host_bridge(NULL, dev);
 	u32 uid;
-	int rc;
 
 	if (kstrtou32(acpi_device_uid(hb), 0, &uid))
 		return -EINVAL;
 
-	rc = acpi_get_genport_coordinates(uid, dport->hb_coord);
-	if (rc < 0)
-		return rc;
-
-	/* Adjust back to picoseconds from nanoseconds */
-	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
-		dport->hb_coord[i].read_latency *= 1000;
-		dport->hb_coord[i].write_latency *= 1000;
-	}
-
-	return 0;
+	return acpi_get_genport_coordinates(uid, dport->hb_coord);
 }
 
 static int add_host_bridge_dport(struct device *match, void *arg)
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index eddbbe21450c..48704976693e 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -20,6 +20,31 @@ struct dsmas_entry {
 	int qos_class;
 };
 
+static u32 cdat_normalize(u16 entry, u64 base, u8 type)
+{
+	u32 value;
+
+	/*
+	 * Check for invalid and overflow values
+	 */
+	if (entry == 0xffff || !entry)
+		return 0;
+	else if (base > (UINT_MAX / (entry)))
+		return 0;
+
+	value = entry * base;
+	switch (type) {
+	case ACPI_HMAT_ACCESS_LATENCY:
+	case ACPI_HMAT_READ_LATENCY:
+	case ACPI_HMAT_WRITE_LATENCY:
+		value = DIV_ROUND_UP(value, 1000);
+		break;
+	default:
+		break;
+	}
+	return value;
+}
+
 static int cdat_dsmas_handler(union acpi_subtable_headers *header, void *arg,
 			      const unsigned long end)
 {
@@ -97,7 +122,6 @@ static int cdat_dslbis_handler(union acpi_subtable_headers *header, void *arg,
 	__le16 le_val;
 	u64 val;
 	u16 len;
-	int rc;
 
 	len = le16_to_cpu((__force __le16)hdr->length);
 	if (len != size || (unsigned long)hdr + len > end) {
@@ -124,10 +148,8 @@ static int cdat_dslbis_handler(union acpi_subtable_headers *header, void *arg,
 
 	le_base = (__force __le64)dslbis->entry_base_unit;
 	le_val = (__force __le16)dslbis->entry[0];
-	rc = check_mul_overflow(le64_to_cpu(le_base),
-				le16_to_cpu(le_val), &val);
-	if (rc)
-		pr_warn("DSLBIS value overflowed.\n");
+	val = cdat_normalize(le16_to_cpu(le_val), le64_to_cpu(le_base),
+			     dslbis->data_type);
 
 	cxl_access_coordinate_set(&dent->coord, dslbis->data_type, val);
 
@@ -164,7 +186,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
 					struct xarray *dsmas_xa)
 {
 	struct access_coordinate ep_c;
-	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
 	struct dsmas_entry *dent;
 	int valid_entries = 0;
 	unsigned long index;
@@ -176,12 +197,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
 		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;
-	}
-
 	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
 
 	if (!cxl_root)
@@ -194,18 +209,9 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
 		int qos_class;
 
 		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_CPU],
-					&coord[ACCESS_COORDINATE_CPU],
-					&dent->coord);
 		dent->entries = 1;
-		rc = cxl_root->ops->qos_class(cxl_root,
-					      &coord[ACCESS_COORDINATE_CPU],
-					      1, &qos_class);
+		rc = cxl_root->ops->qos_class(cxl_root, &dent->coord, 1,
+					      &qos_class);
 		if (rc != 1)
 			continue;
 
@@ -461,10 +467,8 @@ static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
 
 		le_base = (__force __le64)tbl->sslbis_header.entry_base_unit;
 		le_val = (__force __le16)tbl->entries[i].latency_or_bandwidth;
-
-		if (check_mul_overflow(le64_to_cpu(le_base),
-				       le16_to_cpu(le_val), &val))
-			dev_warn(dev, "SSLBIS value overflowed!\n");
+		val = cdat_normalize(le16_to_cpu(le_val), le64_to_cpu(le_base),
+				     sslbis->data_type);
 
 		xa_for_each(&port->dports, index, dport) {
 			if (dsp_id == ACPI_CDAT_SSLBIS_ANY_PORT ||
@@ -521,17 +525,13 @@ void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
 				    struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
-	struct cxl_port *port = cxlmd->endpoint;
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
-	struct access_coordinate hb_coord[ACCESS_COORDINATE_MAX];
-	struct access_coordinate coord;
 	struct range dpa = {
 			.start = cxled->dpa_res->start,
 			.end = cxled->dpa_res->end,
 	};
 	struct cxl_dpa_perf *perf;
-	int rc;
 
 	switch (cxlr->mode) {
 	case CXL_DECODER_RAM:
@@ -549,35 +549,16 @@ void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
 	if (!range_contains(&perf->dpa_range, &dpa))
 		return;
 
-	rc = cxl_hb_get_perf_coordinates(port, hb_coord);
-	if (rc)  {
-		dev_dbg(&port->dev, "Failed to retrieve hb perf coordinates.\n");
-		return;
-	}
-
 	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
-		/* Pickup the host bridge coords */
-		cxl_coordinates_combine(&coord, &hb_coord[i], &perf->coord);
-
 		/* Get total bandwidth and the worst latency for the cxl region */
 		cxlr->coord[i].read_latency = max_t(unsigned int,
 						    cxlr->coord[i].read_latency,
-						    coord.read_latency);
+						    perf->coord.read_latency);
 		cxlr->coord[i].write_latency = max_t(unsigned int,
 						     cxlr->coord[i].write_latency,
-						     coord.write_latency);
-		cxlr->coord[i].read_bandwidth += coord.read_bandwidth;
-		cxlr->coord[i].write_bandwidth += coord.write_bandwidth;
-
-		/*
-		 * Convert latency to nanosec from picosec to be consistent
-		 * with the resulting latency coordinates computed by the
-		 * HMAT_REPORTING code.
-		 */
-		cxlr->coord[i].read_latency =
-			DIV_ROUND_UP(cxlr->coord[i].read_latency, 1000);
-		cxlr->coord[i].write_latency =
-			DIV_ROUND_UP(cxlr->coord[i].write_latency, 1000);
+						     perf->coord.write_latency);
+		cxlr->coord[i].read_bandwidth += perf->coord.read_bandwidth;
+		cxlr->coord[i].write_bandwidth += perf->coord.write_bandwidth;
 	}
 }
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 7aadcec4fc64..c7c00eb373af 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -2133,38 +2133,6 @@ 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 between initiator
- *				 and host bridge
- *
- * @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;
-}
-
 static bool parent_port_is_cxl_root(struct cxl_port *port)
 {
 	return is_cxl_root(to_cxl_port(port->dev.parent));
@@ -2215,6 +2183,10 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 		c.read_latency += dport->link_latency;
 	} while (!is_cxl_root);
 
+	dport = iter->parent_dport;
+	/* Retrieve HB coords */
+	cxl_coordinates_combine(&c, &c, dport->hb_coord);
+
 	/* 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 534e25e2f0a4..ed02373ce3d9 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -884,8 +884,6 @@ 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_region_perf_data_calculate(struct cxl_region *cxlr,
 				    struct cxl_endpoint_decoder *cxled);
 
-- 
2.44.0

Re: [PATCH v7 3/5] cxl: Fix incorrect region perf data calculation

[Jonathan Cameron]

On Wed, 3 Apr 2024 08:47:14 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Current math in cxl_region_perf_data_calculate divides the latency by 1000
> every time the function gets called. This causes the region latency to be
> divided by 1000 per memory device and the math is incorrect. This is user
> visible as the latency access_coordinate exposed via sysfs will show
> incorrect latency data.
> 
> Normalize values from CDAT to nanoseconds. Adjust sub-nanoseconds latency
> to at least 1. Remove adjustment of perf numbers from the generic target
> since hmat handling code has already normalized those numbers. Now all
> computation and stored numbers should be in nanoseconds.
> 
> cxl_hb_get_perf_coordinates() is removed and HB coords are calculated
> in the port access_coordinate calculation path since it no longer need
> to be treated special.
> 
> Fixes: 3d9f4a197230 ("cxl/region: Calculate performance data for a region")
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

I should stop reading this code... 

What happens with the bandwidth if the minimum point on path to EP is shared?
Gets more complex as maybe the shared bit wasn't the minimum bandwidth previously
but when it's 'split' across multiple paths it becomes so.
E.g. HEre the min on each path is 5, but the bottleneck is actually the RP to
switch at 8 once we are interleaving across EP0 and EP1.

     CPU
      |
     HB
      |
     RP
      |
  <min BW here = 8>
      |
    SWITCH
    |    |
<each of these BW 5>
   EP0  EP1


None of this mattered with traditional HMAT entries because they
are point to point so if such interleaving is going on it was
a problem for the BIOS writer...

Not related to what you are fixing here though so
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>



> @@ -521,17 +525,13 @@ void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
>  				    struct cxl_endpoint_decoder *cxled)
>  {
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> -	struct cxl_port *port = cxlmd->endpoint;
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> -	struct access_coordinate hb_coord[ACCESS_COORDINATE_MAX];
> -	struct access_coordinate coord;
>  	struct range dpa = {
>  			.start = cxled->dpa_res->start,
>  			.end = cxled->dpa_res->end,
>  	};
>  	struct cxl_dpa_perf *perf;
> -	int rc;
>  
>  	switch (cxlr->mode) {
>  	case CXL_DECODER_RAM:
> @@ -549,35 +549,16 @@ void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
>  	if (!range_contains(&perf->dpa_range, &dpa))
>  		return;
>  
> -	rc = cxl_hb_get_perf_coordinates(port, hb_coord);
> -	if (rc)  {
> -		dev_dbg(&port->dev, "Failed to retrieve hb perf coordinates.\n");
> -		return;
> -	}
> -
>  	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
> -		/* Pickup the host bridge coords */
> -		cxl_coordinates_combine(&coord, &hb_coord[i], &perf->coord);
> -
>  		/* Get total bandwidth and the worst latency for the cxl region */

Worst latency from what set of choices? Perhaps useful to call that out (multiple EP
paths?)

>  		cxlr->coord[i].read_latency = max_t(unsigned int,
>  						    cxlr->coord[i].read_latency,
> -						    coord.read_latency);
> +						    perf->coord.read_latency);
>  		cxlr->coord[i].write_latency = max_t(unsigned int,
>  						     cxlr->coord[i].write_latency,
> -						     coord.write_latency);
> -		cxlr->coord[i].read_bandwidth += coord.read_bandwidth;
> -		cxlr->coord[i].write_bandwidth += coord.write_bandwidth;
> -
> -		/*
> -		 * Convert latency to nanosec from picosec to be consistent
> -		 * with the resulting latency coordinates computed by the
> -		 * HMAT_REPORTING code.
> -		 */
> -		cxlr->coord[i].read_latency =
> -			DIV_ROUND_UP(cxlr->coord[i].read_latency, 1000);
> -		cxlr->coord[i].write_latency =
> -			DIV_ROUND_UP(cxlr->coord[i].write_latency, 1000);
> +						     perf->coord.write_latency);
> +		cxlr->coord[i].read_bandwidth += perf->coord.read_bandwidth;
> +		cxlr->coord[i].write_bandwidth += perf->coord.write_bandwidth;

As above, this might be the same bandwidth we are double counting...

>  	}
>  }
>  
>

RE: [PATCH v7 3/5] cxl: Fix incorrect region perf data calculation

[Dan Williams]

Dave Jiang wrote:
> Current math in cxl_region_perf_data_calculate divides the latency by 1000
> every time the function gets called. This causes the region latency to be
> divided by 1000 per memory device and the math is incorrect. This is user
> visible as the latency access_coordinate exposed via sysfs will show
> incorrect latency data.
> 
> Normalize values from CDAT to nanoseconds. Adjust sub-nanoseconds latency
> to at least 1. Remove adjustment of perf numbers from the generic target
> since hmat handling code has already normalized those numbers. Now all
> computation and stored numbers should be in nanoseconds.
> 
> cxl_hb_get_perf_coordinates() is removed and HB coords are calculated
> in the port access_coordinate calculation path since it no longer need
> to be treated special.
> 
> Fixes: 3d9f4a197230 ("cxl/region: Calculate performance data for a region")
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v7:
> - Remove min_not_zero(). Incorrectly set everything to 1. DIV_ROUNDUP()
>   will ensure sub-nanoseconds values not 0 unless value 0 to begin with.
>   (Jonathan)
> - Reflowed patch order
> - Remove cxl_hb_get_perf_coordinates() as change made function unnessary.
> - Add hb access_coordinate back to port caclculation.
> ---
>  drivers/cxl/acpi.c      | 13 +-----
>  drivers/cxl/core/cdat.c | 89 ++++++++++++++++-------------------------
>  drivers/cxl/core/port.c | 36 ++---------------
>  drivers/cxl/cxl.h       |  2 -
>  4 files changed, 40 insertions(+), 100 deletions(-)

yum... bug fix that removes more than double the code it adds.

[..]
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index eddbbe21450c..48704976693e 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -20,6 +20,31 @@ struct dsmas_entry {
>  	int qos_class;
>  };
>  
> +static u32 cdat_normalize(u16 entry, u64 base, u8 type)
> +{
> +	u32 value;
> +
> +	/*
> +	 * Check for invalid and overflow values
> +	 */
> +	if (entry == 0xffff || !entry)
> +		return 0;
> +	else if (base > (UINT_MAX / (entry)))
> +		return 0;
> +
> +	value = entry * base;

Might be worth a reminder comment here that CDAT fields follow the
format of HMAT fields when a future reader wonders why these type names
are not CDAT_ACCESS_LATENCY, etc. Bonus points for a "see Table 5
Device Scoped Latency and Bandwidth Information Structure in Coherent
Device Attribute Table (CDAT) Specification v1.01"

Either way you can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH v7 3/5] cxl: Fix incorrect region perf data calculation

[Jonathan Cameron]

On Fri, 5 Apr 2024 15:34:25 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Dave Jiang wrote:
> > Current math in cxl_region_perf_data_calculate divides the latency by 1000
> > every time the function gets called. This causes the region latency to be
> > divided by 1000 per memory device and the math is incorrect. This is user
> > visible as the latency access_coordinate exposed via sysfs will show
> > incorrect latency data.
> > 
> > Normalize values from CDAT to nanoseconds. Adjust sub-nanoseconds latency
> > to at least 1. Remove adjustment of perf numbers from the generic target
> > since hmat handling code has already normalized those numbers. Now all
> > computation and stored numbers should be in nanoseconds.
> > 
> > cxl_hb_get_perf_coordinates() is removed and HB coords are calculated
> > in the port access_coordinate calculation path since it no longer need
> > to be treated special.
> > 
> > Fixes: 3d9f4a197230 ("cxl/region: Calculate performance data for a region")
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > ---
> > v7:
> > - Remove min_not_zero(). Incorrectly set everything to 1. DIV_ROUNDUP()
> >   will ensure sub-nanoseconds values not 0 unless value 0 to begin with.
> >   (Jonathan)
> > - Reflowed patch order
> > - Remove cxl_hb_get_perf_coordinates() as change made function unnessary.
> > - Add hb access_coordinate back to port caclculation.
> > ---
> >  drivers/cxl/acpi.c      | 13 +-----
> >  drivers/cxl/core/cdat.c | 89 ++++++++++++++++-------------------------
> >  drivers/cxl/core/port.c | 36 ++---------------
> >  drivers/cxl/cxl.h       |  2 -
> >  4 files changed, 40 insertions(+), 100 deletions(-)  
> 
> yum... bug fix that removes more than double the code it adds.
> 
> [..]
> > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> > index eddbbe21450c..48704976693e 100644
> > --- a/drivers/cxl/core/cdat.c
> > +++ b/drivers/cxl/core/cdat.c
> > @@ -20,6 +20,31 @@ struct dsmas_entry {
> >  	int qos_class;
> >  };
> >  
> > +static u32 cdat_normalize(u16 entry, u64 base, u8 type)
> > +{
> > +	u32 value;
> > +
> > +	/*
> > +	 * Check for invalid and overflow values
> > +	 */
> > +	if (entry == 0xffff || !entry)
> > +		return 0;
> > +	else if (base > (UINT_MAX / (entry)))
> > +		return 0;
> > +
> > +	value = entry * base;  
> 
> Might be worth a reminder comment here that CDAT fields follow the
> format of HMAT fields when a future reader wonders why these type names
> are not CDAT_ACCESS_LATENCY, etc. Bonus points for a "see Table 5
> Device Scoped Latency and Bandwidth Information Structure in Coherent
> Device Attribute Table (CDAT) Specification v1.01"

Call out a specific HMAT version if you do add such a comment.
We don't want anyone to happen to look at an early version and get
even more confused :(


> 
> Either way you can add:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

[PATCH v7 4/5] cxl: Consolidate dport access_coordinate ->hb_coord and ->sw_coord into ->coord

[Dave Jiang]

The driver stores access_coordinate for host bridge in ->hb_coord and
switch CDAT access_coordinate in ->sw_coord. Since neither of these
access_coordinate clobber each other, the variable name can be consolidated
into ->coord to simplify the code.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v7:
- Adjusted based on code changes from pervious patches
---
 drivers/cxl/acpi.c           |  2 +-
 drivers/cxl/core/cdat.c      | 74 +++++++++++++++++++++++-------------
 drivers/cxl/core/port.c      | 48 +++++++++++++++++------
 drivers/cxl/cxl.h            |  6 +--
 drivers/cxl/cxlmem.h         |  2 +-
 tools/testing/cxl/test/cxl.c | 10 +++--
 6 files changed, 94 insertions(+), 48 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 566c387d4385..cb8c155a2c9b 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -529,7 +529,7 @@ static int get_genport_coordinates(struct device *dev, struct cxl_dport *dport)
 	if (kstrtou32(acpi_device_uid(hb), 0, &uid))
 		return -EINVAL;
 
-	return acpi_get_genport_coordinates(uid, dport->hb_coord);
+	return acpi_get_genport_coordinates(uid, dport->coord);
 }
 
 static int add_host_bridge_dport(struct device *match, void *arg)
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index 48704976693e..4b717d2f5a9d 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -14,7 +14,7 @@
 struct dsmas_entry {
 	struct range dpa_range;
 	u8 handle;
-	struct access_coordinate coord;
+	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
 
 	int entries;
 	int qos_class;
@@ -83,8 +83,8 @@ static int cdat_dsmas_handler(union acpi_subtable_headers *header, void *arg,
 	return 0;
 }
 
-static void cxl_access_coordinate_set(struct access_coordinate *coord,
-				      int access, unsigned int val)
+static void __cxl_access_coordinate_set(struct access_coordinate *coord,
+					int access, unsigned int val)
 {
 	switch (access) {
 	case ACPI_HMAT_ACCESS_LATENCY:
@@ -110,6 +110,13 @@ static void cxl_access_coordinate_set(struct access_coordinate *coord,
 	}
 }
 
+static void cxl_access_coordinate_set(struct access_coordinate *coord,
+				      int access, unsigned int val)
+{
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++)
+		__cxl_access_coordinate_set(&coord[i], access, val);
+}
+
 static int cdat_dslbis_handler(union acpi_subtable_headers *header, void *arg,
 			       const unsigned long end)
 {
@@ -151,7 +158,7 @@ static int cdat_dslbis_handler(union acpi_subtable_headers *header, void *arg,
 	val = cdat_normalize(le16_to_cpu(le_val), le64_to_cpu(le_base),
 			     dslbis->data_type);
 
-	cxl_access_coordinate_set(&dent->coord, dslbis->data_type, val);
+	cxl_access_coordinate_set(dent->coord, dslbis->data_type, val);
 
 	return 0;
 }
@@ -185,13 +192,13 @@ 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 ep_c;
+	struct access_coordinate ep_c[ACCESS_COORDINATE_MAX];
 	struct dsmas_entry *dent;
 	int valid_entries = 0;
 	unsigned long index;
 	int rc;
 
-	rc = cxl_endpoint_get_perf_coordinates(port, &ep_c);
+	rc = cxl_endpoint_get_perf_coordinates(port, ep_c);
 	if (rc) {
 		dev_dbg(&port->dev, "Failed to retrieve ep perf coordinates.\n");
 		return rc;
@@ -208,10 +215,11 @@ 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, &ep_c);
+		cxl_coordinates_combine(dent->coord, dent->coord, ep_c);
 		dent->entries = 1;
-		rc = cxl_root->ops->qos_class(cxl_root, &dent->coord, 1,
-					      &qos_class);
+		rc = cxl_root->ops->qos_class(cxl_root,
+					      &dent->coord[ACCESS_COORDINATE_CPU],
+					      1, &qos_class);
 		if (rc != 1)
 			continue;
 
@@ -228,14 +236,17 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
 static void update_perf_entry(struct device *dev, struct dsmas_entry *dent,
 			      struct cxl_dpa_perf *dpa_perf)
 {
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++)
+		dpa_perf->coord[i] = dent->coord[i];
 	dpa_perf->dpa_range = dent->dpa_range;
-	dpa_perf->coord = dent->coord;
 	dpa_perf->qos_class = dent->qos_class;
 	dev_dbg(dev,
 		"DSMAS: dpa: %#llx qos: %d read_bw: %d write_bw %d read_lat: %d write_lat: %d\n",
 		dent->dpa_range.start, dpa_perf->qos_class,
-		dent->coord.read_bandwidth, dent->coord.write_bandwidth,
-		dent->coord.read_latency, dent->coord.write_latency);
+		dent->coord[ACCESS_COORDINATE_CPU].read_bandwidth,
+		dent->coord[ACCESS_COORDINATE_CPU].write_bandwidth,
+		dent->coord[ACCESS_COORDINATE_CPU].read_latency,
+		dent->coord[ACCESS_COORDINATE_CPU].write_latency);
 }
 
 static void cxl_memdev_set_qos_class(struct cxl_dev_state *cxlds,
@@ -472,10 +483,11 @@ static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
 
 		xa_for_each(&port->dports, index, dport) {
 			if (dsp_id == ACPI_CDAT_SSLBIS_ANY_PORT ||
-			    dsp_id == dport->port_id)
-				cxl_access_coordinate_set(&dport->sw_coord,
+			    dsp_id == dport->port_id) {
+				cxl_access_coordinate_set(dport->coord,
 							  sslbis->data_type,
 							  val);
+			}
 		}
 	}
 
@@ -497,6 +509,21 @@ void cxl_switch_parse_cdat(struct cxl_port *port)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_switch_parse_cdat, CXL);
 
+static void __cxl_coordinates_combine(struct access_coordinate *out,
+				      struct access_coordinate *c1,
+				      struct access_coordinate *c2)
+{
+		if (c1->write_bandwidth && c2->write_bandwidth)
+			out->write_bandwidth = min(c1->write_bandwidth,
+						   c2->write_bandwidth);
+		out->write_latency = c1->write_latency + c2->write_latency;
+
+		if (c1->read_bandwidth && c2->read_bandwidth)
+			out->read_bandwidth = min(c1->read_bandwidth,
+						  c2->read_bandwidth);
+		out->read_latency = c1->read_latency + c2->read_latency;
+}
+
 /**
  * cxl_coordinates_combine - Combine the two input coordinates
  *
@@ -508,15 +535,8 @@ void cxl_coordinates_combine(struct access_coordinate *out,
 			     struct access_coordinate *c1,
 			     struct access_coordinate *c2)
 {
-		if (c1->write_bandwidth && c2->write_bandwidth)
-			out->write_bandwidth = min(c1->write_bandwidth,
-						   c2->write_bandwidth);
-		out->write_latency = c1->write_latency + c2->write_latency;
-
-		if (c1->read_bandwidth && c2->read_bandwidth)
-			out->read_bandwidth = min(c1->read_bandwidth,
-						  c2->read_bandwidth);
-		out->read_latency = c1->read_latency + c2->read_latency;
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++)
+		__cxl_coordinates_combine(&out[i], &c1[i], &c2[i]);
 }
 
 MODULE_IMPORT_NS(CXL);
@@ -553,12 +573,12 @@ void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
 		/* Get total bandwidth and the worst latency for the cxl region */
 		cxlr->coord[i].read_latency = max_t(unsigned int,
 						    cxlr->coord[i].read_latency,
-						    perf->coord.read_latency);
+						    perf->coord[i].read_latency);
 		cxlr->coord[i].write_latency = max_t(unsigned int,
 						     cxlr->coord[i].write_latency,
-						     perf->coord.write_latency);
-		cxlr->coord[i].read_bandwidth += perf->coord.read_bandwidth;
-		cxlr->coord[i].write_bandwidth += perf->coord.write_bandwidth;
+						     perf->coord[i].write_latency);
+		cxlr->coord[i].read_bandwidth += perf->coord[i].read_bandwidth;
+		cxlr->coord[i].write_bandwidth += perf->coord[i].write_bandwidth;
 	}
 }
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index c7c00eb373af..801c4018a1dd 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -2133,6 +2133,29 @@ bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd)
 }
 EXPORT_SYMBOL_NS_GPL(schedule_cxl_memdev_detach, CXL);
 
+static void add_latency(struct access_coordinate *c, long latency)
+{
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
+		c[i].write_latency += latency;
+		c[i].read_latency += latency;
+	}
+}
+
+static void set_min_bandwidth(struct access_coordinate *c, unsigned int bw)
+{
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
+		c[i].write_bandwidth = min(c[i].write_bandwidth, bw);
+		c[i].read_bandwidth = min(c[i].read_bandwidth, bw);
+	}
+}
+
+static void set_access_coordinates(struct access_coordinate *out,
+				   struct access_coordinate *in)
+{
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++)
+		out[i] = in[i];
+}
+
 static bool parent_port_is_cxl_root(struct cxl_port *port)
 {
 	return is_cxl_root(to_cxl_port(port->dev.parent));
@@ -2149,9 +2172,15 @@ static bool parent_port_is_cxl_root(struct cxl_port *port)
 int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 				      struct access_coordinate *coord)
 {
-	struct access_coordinate c = {
-		.read_bandwidth = UINT_MAX,
-		.write_bandwidth = UINT_MAX,
+	struct access_coordinate c[] = {
+		{
+			.read_bandwidth = UINT_MAX,
+			.write_bandwidth = UINT_MAX,
+		},
+		{
+			.read_bandwidth = UINT_MAX,
+			.write_bandwidth = UINT_MAX,
+		},
 	};
 	struct cxl_port *iter = port;
 	struct cxl_dport *dport;
@@ -2178,14 +2207,13 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 		 * have CDAT and therefore needs to be skipped.
 		 */
 		if (!is_cxl_root)
-			cxl_coordinates_combine(&c, &c, &dport->sw_coord);
-		c.write_latency += dport->link_latency;
-		c.read_latency += dport->link_latency;
+			cxl_coordinates_combine(c, c, dport->coord);
+		add_latency(c, dport->link_latency);
 	} while (!is_cxl_root);
 
 	dport = iter->parent_dport;
 	/* Retrieve HB coords */
-	cxl_coordinates_combine(&c, &c, dport->hb_coord);
+	cxl_coordinates_combine(c, c, dport->coord);
 
 	/* Get the calculated PCI paths bandwidth */
 	pdev = to_pci_dev(port->uport_dev->parent);
@@ -2194,10 +2222,8 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 		return -ENXIO;
 	bw /= BITS_PER_BYTE;
 
-	c.write_bandwidth = min(c.write_bandwidth, bw);
-	c.read_bandwidth = min(c.read_bandwidth, bw);
-
-	*coord = c;
+	set_min_bandwidth(c, bw);
+	set_access_coordinates(coord, c);
 
 	return 0;
 }
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index ed02373ce3d9..036d17db68e0 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -663,8 +663,7 @@ struct cxl_rcrb_info {
  * @rch: Indicate whether this dport was enumerated in RCH or VH mode
  * @port: reference to cxl_port that contains this downstream port
  * @regs: Dport parsed register blocks
- * @sw_coord: access coordinates (performance) for switch from CDAT
- * @hb_coord: access coordinates (performance) from ACPI generic port (host bridge)
+ * @coord: access coordinates (bandwidth and latency performance attributes)
  * @link_latency: calculated PCIe downstream latency
  */
 struct cxl_dport {
@@ -675,8 +674,7 @@ struct cxl_dport {
 	bool rch;
 	struct cxl_port *port;
 	struct cxl_regs regs;
-	struct access_coordinate sw_coord;
-	struct access_coordinate hb_coord[ACCESS_COORDINATE_MAX];
+	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
 	long link_latency;
 };
 
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 20fb3b35e89e..36cee9c30ceb 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -401,7 +401,7 @@ enum cxl_devtype {
  */
 struct cxl_dpa_perf {
 	struct range dpa_range;
-	struct access_coordinate coord;
+	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
 	int qos_class;
 };
 
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index 908e0d083936..61c69297e797 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -986,10 +986,12 @@ static void dpa_perf_setup(struct cxl_port *endpoint, struct range *range,
 {
 	dpa_perf->qos_class = FAKE_QTG_ID;
 	dpa_perf->dpa_range = *range;
-	dpa_perf->coord.read_latency = 500;
-	dpa_perf->coord.write_latency = 500;
-	dpa_perf->coord.read_bandwidth = 1000;
-	dpa_perf->coord.write_bandwidth = 1000;
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
+		dpa_perf->coord[i].read_latency = 500;
+		dpa_perf->coord[i].write_latency = 500;
+		dpa_perf->coord[i].read_bandwidth = 1000;
+		dpa_perf->coord[i].write_bandwidth = 1000;
+	}
 }
 
 static void mock_cxl_endpoint_parse_cdat(struct cxl_port *port)
-- 
2.44.0

[PATCH v7 5/5] cxl: Add checks to access_coordinate calculation to fail missing data

[Dave Jiang]

Jonathan noted that when the coordinates for host bridge and switches
can be 0s if no actual data are retrieved and the calculation continues.
The resulting number would be inaccurate. Add checks to ensure that the
calculation would complete only if the numbers are valid.

While not seen in the wild, issue may show up with a BIOS that reported
CXL root ports via Generic Ports (via a PCI handle in the SRAT entry).

Fixes: 14a6960b3e92 ("cxl: Add helper function that calculate performance data for downstream ports")
Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/port.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 801c4018a1dd..762783bb091a 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -2141,6 +2141,18 @@ static void add_latency(struct access_coordinate *c, long latency)
 	}
 }
 
+static bool coordinates_valid(struct access_coordinate *c)
+{
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
+		if (c[i].read_bandwidth && c[i].write_bandwidth &&
+		    c[i].read_latency && c[i].write_latency)
+			continue;
+		return false;
+	}
+
+	return true;
+}
+
 static void set_min_bandwidth(struct access_coordinate *c, unsigned int bw)
 {
 	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
@@ -2206,13 +2218,18 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 		 * There's no valid access_coordinate for a root port since RPs do not
 		 * have CDAT and therefore needs to be skipped.
 		 */
-		if (!is_cxl_root)
+		if (!is_cxl_root) {
+			if (!coordinates_valid(dport->coord))
+				return -EINVAL;
 			cxl_coordinates_combine(c, c, dport->coord);
+		}
 		add_latency(c, dport->link_latency);
 	} while (!is_cxl_root);
 
 	dport = iter->parent_dport;
 	/* Retrieve HB coords */
+	if (!coordinates_valid(dport->coord))
+		return -EINVAL;
 	cxl_coordinates_combine(c, c, dport->coord);
 
 	/* Get the calculated PCI paths bandwidth */
-- 
2.44.0

RE: [PATCH v7 5/5] cxl: Add checks to access_coordinate calculation to fail missing data

[Dan Williams]

Dave Jiang wrote:
> Jonathan noted that when the coordinates for host bridge and switches
> can be 0s if no actual data are retrieved and the calculation continues.
> The resulting number would be inaccurate. Add checks to ensure that the
> calculation would complete only if the numbers are valid.
> 
> While not seen in the wild, issue may show up with a BIOS that reported
> CXL root ports via Generic Ports (via a PCI handle in the SRAT entry).
> 
> Fixes: 14a6960b3e92 ("cxl: Add helper function that calculate performance data for downstream ports")
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Looks good,

Reviewed-by: Dan Williams <dan.j.williams@intel.com>