[PATCH v7 0/3] cxl: Region bandwidth calculation for targets with shared upstream link

[PATCH v7 0/3] cxl: Region bandwidth calculation for targets with shared upstream link

[Dave Jiang]

v7:
- Add documentation explaining how the bandwidth is calculated. (Dan)
- Fix 2hb2rp2ep case. (Jonathan)
  - Split out the RP and HB level work.
- Rename functions and add kdoc comments to clarify the code. (Dan)
- Move activation point to immediately after cxl_region_setup_targets(). (Dan)
- Added information on testing and results in commit log. (Dan)
- See specific patches for more details.

This series provides recalculation of the CXL region bandwidth when the targets have
shared upstream link by walking the toplogy from bottom up and clamp the bandwdith
as the code traverses up the tree. An example topology:

 An example topology from Jonathan:

                 CFMWS 0
                   |
          _________|_________
         |                   |
     ACPI0017-0            ACPI0017-1
   GP0/HB0/ACPI0016-0   GP1/HB1/ACPI0016-1
     |          |        |           |
    RP0        RP1      RP2         RP3
     |          |        |           |
   SW 0       SW 1     SW 2        SW 3
   |   |      |   |    |   |       |   |
  EP0 EP1    EP2 EP3  EP4  EP5    EP6 EP7

 Computation for the example topology:

 Min (GP0 to CPU BW,
      Min(SW 0 Upstream Link to RP0 BW,
          Min(SW0SSLBIS for SW0DSP0 (EP0), EP0 DSLBIS, EP0 Upstream Link) +
          Min(SW0SSLBIS for SW0DSP1 (EP1), EP1 DSLBIS, EP1 Upstream link)) +
      Min(SW 1 Upstream Link to RP1 BW,
          Min(SW1SSLBIS for SW1DSP0 (EP2), EP2 DSLBIS, EP2 Upstream Link) +
          Min(SW1SSLBIS for SW1DSP1 (EP3), EP3 DSLBIS, EP3 Upstream link))) +
 Min (GP1 to CPU BW,
      Min(SW 2 Upstream Link to RP2 BW,
          Min(SW2SSLBIS for SW2DSP0 (EP4), EP4 DSLBIS, EP4 Upstream Link) +
          Min(SW2SSLBIS for SW2DSP1 (EP5), EP5 DSLBIS, EP5 Upstream link)) +
      Min(SW 3 Upstream Link to RP3 BW,
          Min(SW3SSLBIS for SW3DSP0 (EP6), EP6 DSLBIS, EP6 Upstream Link) +
          Min(SW3SSLBIS for SW3DSP1 (EP7), EP7 DSLBIS, EP7 Upstream link))))

---

Dave Jiang (3):
      cxl: Preserve the CDAT access_coordinate for an endpoint
      cxl: Calculate region bandwidth of targets with shared upstream link
      cxl: Add documentation to explain the shared link bandwidth calculation

 Documentation/driver-api/cxl/access-coordinates.rst |  90 +++++++
 Documentation/driver-api/cxl/index.rst              |   1 +
 MAINTAINERS                                         |   1 +
 drivers/cxl/core/cdat.c                             | 509 +++++++++++++++++++++++++++++++++++++++-
 drivers/cxl/core/core.h                             |   4 +-
 drivers/cxl/core/pci.c                              |  23 ++
 drivers/cxl/core/port.c                             |  20 ++
 drivers/cxl/core/region.c                           |   2 +
 drivers/cxl/cxl.h                                   |   1 +
 drivers/cxl/cxlmem.h                                |   4 +-
 10 files changed, 640 insertions(+), 15 deletions(-)

[PATCH v7 1/3] cxl: Preserve the CDAT access_coordinate for an endpoint

[Dave Jiang]

Keep the access_coordinate from the CDAT tables for region perf
calculations. The region perf calculation requires all participating
endpoints to have arrived in order to determine if there are limitations
of bandwidth data due to shared uplink.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/cdat.c | 10 ++++++----
 drivers/cxl/cxlmem.h    |  4 +++-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index bb83867d9fec..fea214340d4b 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -15,7 +15,7 @@ struct dsmas_entry {
 	struct range dpa_range;
 	u8 handle;
 	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
-
+	struct access_coordinate cdat_coord[ACCESS_COORDINATE_MAX];
 	int entries;
 	int qos_class;
 };
@@ -163,7 +163,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->cdat_coord, dslbis->data_type, val);
 
 	return 0;
 }
@@ -220,7 +220,7 @@ 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->cdat_coord, ep_c);
 		dent->entries = 1;
 		rc = cxl_root->ops->qos_class(cxl_root,
 					      &dent->coord[ACCESS_COORDINATE_CPU],
@@ -241,8 +241,10 @@ 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++)
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
 		dpa_perf->coord[i] = dent->coord[i];
+		dpa_perf->cdat_coord[i] = dent->cdat_coord[i];
+	}
 	dpa_perf->dpa_range = dent->dpa_range;
 	dpa_perf->qos_class = dent->qos_class;
 	dev_dbg(dev,
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 19aba81cdf13..a9c4af4ca78b 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -396,12 +396,14 @@ enum cxl_devtype {
 /**
  * struct cxl_dpa_perf - DPA performance property entry
  * @dpa_range - range for DPA address
- * @coord - QoS performance data (i.e. latency, bandwidth)
+ * @coord - calculated QoS performance data (i.e. latency, bandwidth)
+ * @cdat_coord - raw QoS performance data from CDAT
  * @qos_class - QoS Class cookies
  */
 struct cxl_dpa_perf {
 	struct range dpa_range;
 	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
+	struct access_coordinate cdat_coord[ACCESS_COORDINATE_MAX];
 	int qos_class;
 };
 
-- 
2.45.2

[PATCH v7 2/3] cxl: Calculate region bandwidth of targets with shared upstream link

[Dave Jiang]

The current bandwidth calculation aggregates all the targets. This simple
method does not take into account where multiple targets sharing under
a switch or a root port where the aggregated bandwidth can be greater than
the upstream link of the switch.

To accurately account for the shared upstream uplink cases, a new update
function is introduced by walking from the leaves to the root of the
hierarchy and clamp the bandwidth in the process as needed. This process
is done when all the targets for a region are present but before the
final values are send to the HMAT handling code cached access_coordinate
targets.

The original perf calculation path was kept to calculate the latency
performance data that does not require the shared link consideration.
The shared upstream link calculation is done as a second pass when all
the endpoints have arrived.

Testing is done via qemu with CXL hierachy. run_qemu[1] is modified to
support several CXL hierachy layouts. The following layouts are tested:

HB: Host Bridge
RP: Root Port
SW: Switch
EP: End Point

2 HB 2 RP 2 EP: resulting bandwidth: 624
1 HB 2 RP 2 EP: resulting bandwidth: 624
2 HB 2 RP 2 SW 4 EP: resulting bandwidth: 624

Current testing, perf number from SRAT/HMAT is hacked into the kernel
code. However with new QEMU support of Generic Target Port that's
incoming, the perf data injection is no longer needed.

[1]: https://github.com/pmem/run_qemu

Suggested-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Link: https://lore.kernel.org/linux-cxl/20240501152503.00002e60@Huawei.com/
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v7:
- Add test notes in commit log. (Dan)
- Move cxl_memdev_get_dpa_perf() to cxled_memdev_get_dpa_perf(). (Dan)
- Add a DEFINE_FREE(free_perf_xa). (Dan)
- Address 2hb2rp2ep issue Jonathan reported. (Jonathan)
- Added more kdoc comment headers. (Dan)
- Rename helper functions to be more clear on what they do. (Dan)
- Move activiation point to after cxl_region_setup_targets(). (Dan)
---
 drivers/cxl/core/cdat.c   | 499 +++++++++++++++++++++++++++++++++++++-
 drivers/cxl/core/core.h   |   4 +-
 drivers/cxl/core/pci.c    |  23 ++
 drivers/cxl/core/port.c   |  20 ++
 drivers/cxl/core/region.c |   2 +
 drivers/cxl/cxl.h         |   1 +
 6 files changed, 539 insertions(+), 10 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index fea214340d4b..e14eceb8c55c 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -548,19 +548,37 @@ void cxl_coordinates_combine(struct access_coordinate *out,
 
 MODULE_IMPORT_NS(CXL);
 
-void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
-				    struct cxl_endpoint_decoder *cxled)
+static void cxl_bandwidth_add(struct access_coordinate *coord,
+			      struct access_coordinate *c1,
+			      struct access_coordinate *c2)
+{
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
+		coord[i].read_bandwidth = c1[i].read_bandwidth +
+					  c2[i].read_bandwidth;
+		coord[i].write_bandwidth = c1[i].write_bandwidth +
+					   c2[i].write_bandwidth;
+	}
+}
+
+static bool dpa_perf_contains(struct cxl_dpa_perf *perf,
+			      struct resource *dpa_res)
 {
-	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
-	struct cxl_dev_state *cxlds = cxlmd->cxlds;
-	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
 	struct range dpa = {
-			.start = cxled->dpa_res->start,
-			.end = cxled->dpa_res->end,
+		.start = dpa_res->start,
+		.end = dpa_res->end,
 	};
+
+	return range_contains(&perf->dpa_range, &dpa);
+}
+
+static struct cxl_dpa_perf *cxled_get_dpa_perf(struct cxl_endpoint_decoder *cxled,
+					       enum cxl_decoder_mode mode)
+{
+	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
 	struct cxl_dpa_perf *perf;
 
-	switch (cxlr->mode) {
+	switch (mode) {
 	case CXL_DECODER_RAM:
 		perf = &mds->ram_perf;
 		break;
@@ -568,12 +586,475 @@ void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
 		perf = &mds->pmem_perf;
 		break;
 	default:
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (!dpa_perf_contains(perf, cxled->dpa_res))
+		return ERR_PTR(-EINVAL);
+
+	return perf;
+}
+
+/*
+ * Transient context for containing the current calculation of bandwidth when
+ * doing walking the port hierachy to deal with shared upstream link.
+ */
+struct cxl_perf_ctx {
+	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
+	struct cxl_port *port;
+};
+
+/*
+ * cxl_endpoint_gather_bandwidth - collect all the endpoint bandwidth in an xarray
+ * @cxlr: CXL region for the bandwidth calculation
+ * @cxled: endpoint decoder to start on
+ * @usp_xa: (output) the xarray that collects all the bandwidth coordinates
+ *          indexed by the upstream device with data of 'struct cxl_perf_ctx'.
+ * @gp_is_root: (output) bool of whether the grandparent is cxl root.
+ *
+ * Return: 0 for success or -errno
+ *
+ * Collects aggregated endpoint bandwidth and store the bandwidth in
+ * an xarray indexed by the upstream device of the switch or the RP
+ * device. Each endpoint consists the minimum of the bandwidth from DSLBIS
+ * from the endpoint CDAT, the endpoint upstream link bandwidth, and the
+ * bandwidth from the SSLBIS of the switch CDAT for the switch upstream port to
+ * the downstream port that's associated with the endpoint. If the
+ * device is directly connected to a RP, then no SSLBIS is involved.
+ */
+static int cxl_endpoint_gather_bandwidth(struct cxl_region *cxlr,
+					 struct cxl_endpoint_decoder *cxled,
+					 struct xarray *usp_xa,
+					 bool *gp_is_root)
+{
+	struct cxl_port *endpoint = to_cxl_port(cxled->cxld.dev.parent);
+	struct cxl_port *parent_port = to_cxl_port(endpoint->dev.parent);
+	struct cxl_port *gp_port = to_cxl_port(parent_port->dev.parent);
+	struct access_coordinate pci_coord[ACCESS_COORDINATE_MAX];
+	struct access_coordinate sw_coord[ACCESS_COORDINATE_MAX];
+	struct access_coordinate ep_coord[ACCESS_COORDINATE_MAX];
+	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+	struct cxl_perf_ctx *perf_ctx;
+	struct cxl_dpa_perf *perf;
+	unsigned long index;
+	void *ptr;
+	int rc;
+
+	if (cxlds->rcd)
+		return -ENODEV;
+
+	perf = cxled_get_dpa_perf(cxled, cxlr->mode);
+	if (IS_ERR(perf))
+		return PTR_ERR(perf);
+
+	gp_port = to_cxl_port(parent_port->dev.parent);
+	*gp_is_root = is_cxl_root(gp_port);
+
+	/*
+	 * If the grandparent is cxl root, then index is the root port,
+	 * otherwise it's the parent switch upstream device.
+	 */
+	if (*gp_is_root)
+		index = (unsigned long)endpoint->parent_dport->dport_dev;
+	else
+		index = (unsigned long)parent_port->uport_dev;
+
+	perf_ctx = xa_load(usp_xa, index);
+	if (!perf_ctx) {
+		struct cxl_perf_ctx *c __free(kfree) =
+			kzalloc(sizeof(*perf_ctx), GFP_KERNEL);
+
+		if (!c)
+			return -ENOMEM;
+		ptr = xa_store(usp_xa, index, c, GFP_KERNEL);
+		if (xa_is_err(ptr))
+			return xa_err(ptr);
+		perf_ctx = no_free_ptr(c);
+		perf_ctx->port = parent_port;
+	}
+
+	/* Direct upstream link from EP bandwidth */
+	rc = cxl_pci_get_bandwidth(pdev, pci_coord);
+	if (rc < 0)
+		return rc;
+
+	/*
+	 * Min of upstream link bandwidth and Endpoint CDAT bandwidth from
+	 * DSLBIS.
+	 */
+	cxl_coordinates_combine(ep_coord, pci_coord, perf->cdat_coord);
+
+	/*
+	 * If grandparent port is root, then there's no switch involved and
+	 * the endpoint is connected to a root port.
+	 */
+	if (!*gp_is_root) {
+		/*
+		 * Retrieve the switch SSLBIS for switch downstream port
+		 * associated with the endpoint bandwidth.
+		 */
+		rc = cxl_port_get_switch_dport_bandwidth(endpoint, sw_coord);
+		if (rc)
+			return rc;
+
+		/*
+		 * Min of the earlier coordinates with the switch SSLBIS
+		 * bandwidth
+		 */
+		cxl_coordinates_combine(ep_coord, ep_coord, sw_coord);
+	}
+
+	/*
+	 * Aggregate the computed bandwidth with the current aggregated bandwidth
+	 * of the endpoints with the same switch upstream device or RP.
+	 */
+	cxl_bandwidth_add(perf_ctx->coord, perf_ctx->coord, ep_coord);
+
+	return 0;
+}
+
+static void free_perf_xa(struct xarray *xa)
+{
+	struct cxl_perf_ctx *ctx;
+	unsigned long index;
+
+	if (!xa)
+		return;
+
+	xa_for_each(xa, index, ctx)
+		kfree(ctx);
+	xa_destroy(xa);
+	kfree(xa);
+}
+DEFINE_FREE(free_perf_xa, struct xarray *, if (_T) free_perf_xa(_T))
+
+/*
+ * cxl_switch_gather_bandwidth - collect all the bandwidth at switch level in an xarray
+ * @cxlr: The region being operated on
+ * @input_xa: xarray indexed by upstream device of a switch with data of 'struct
+ *	      cxl_perf_ctx'
+ * @gp_is_root: (output) bool of whether the grandparent is cxl root.
+ *
+ * Return: a xarray of resulting cxl_perf_ctx per parent switch or root port
+ *         or ERR_PTR(-errno)
+ *
+ * Iterate through the xarray. Take the minimum of the downstream calculated
+ * bandwidth, the upstream link bandwidth, and the SSLBIS of the upstream
+ * switch if exists. Sum the resulting bandwidth under the switch upstream
+ * device or a RP device. The function can be iterated over multiple switches
+ * if the switches are present.
+ */
+static struct xarray *cxl_switch_gather_bandwidth(struct cxl_region *cxlr,
+						  struct xarray *input_xa,
+						  bool *gp_is_root)
+{
+	struct xarray *res_xa __free(free_perf_xa) =
+		kzalloc(sizeof(*res_xa), GFP_KERNEL);
+	struct access_coordinate coords[ACCESS_COORDINATE_MAX];
+	struct cxl_perf_ctx *ctx, *us_ctx;
+	unsigned long index, us_index;
+	int dev_count = 0;
+	int gp_count = 0;
+	void *ptr;
+	int rc;
+
+	if (!res_xa)
+		return ERR_PTR(-ENOMEM);
+	xa_init(res_xa);
+
+	xa_for_each(input_xa, index, ctx) {
+		struct device *dev = (struct device *)index;
+		struct cxl_port *port = ctx->port;
+		struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
+		struct cxl_port *gp_port = to_cxl_port(parent_port->dev.parent);
+		struct cxl_dport *dport = port->parent_dport;
+		bool is_root;
+
+		dev_count++;
+		if (is_cxl_root(gp_port)) {
+			is_root = true;
+			gp_count++;
+		}
+
+		/*
+		 * If the grandparent is cxl root, then index is the root port,
+		 * otherwise it's the parent switch upstream device.
+		 */
+		if (is_root)
+			us_index = (unsigned long)port->parent_dport->dport_dev;
+		else
+			us_index = (unsigned long)parent_port->uport_dev;
+
+		us_ctx = xa_load(res_xa, us_index);
+		if (!us_ctx) {
+			struct cxl_perf_ctx *n __free(kfree) =
+				kzalloc(sizeof(*n), GFP_KERNEL);
+
+			if (!n)
+				return ERR_PTR(-ENOMEM);
+
+			ptr = xa_store(res_xa, us_index, n, GFP_KERNEL);
+			if (xa_is_err(ptr))
+				return ERR_PTR(xa_err(ptr));
+			us_ctx = no_free_ptr(n);
+			us_ctx->port = parent_port;
+		}
+
+		/*
+		 * If the device isn't an upstream PCIe port, there's something
+		 * wrong with the topology.
+		 */
+		if (!dev_is_pci(dev))
+			return ERR_PTR(-EINVAL);
+
+		/* Retrieve the upstream link bandwidth */
+		rc = cxl_pci_get_bandwidth(to_pci_dev(dev), coords);
+		if (rc)
+			return ERR_PTR(-ENXIO);
+
+		/*
+		 * Take the min of downstream bandwidth and the upstream link
+		 * bandwidth.
+		 */
+		cxl_coordinates_combine(coords, coords, ctx->coord);
+
+		/*
+		 * Take the min of the calculated bandwdith and the upstream
+		 * switch SSLBIS bandwidth if there's a parent switch
+		 */
+		if (!is_root)
+			cxl_coordinates_combine(coords, coords, dport->coord);
+
+		/*
+		 * Aggregate the calculated bandwidth common to an upstream
+		 * switch.
+		 */
+		cxl_bandwidth_add(us_ctx->coord, us_ctx->coord, coords);
+	}
+
+	/* Asymmetric topology detected. */
+	if (gp_count) {
+		if (gp_count != dev_count) {
+			dev_dbg(&cxlr->dev,
+				"Asymmetric hierachy detected, bandwidth not updated\n");
+			return ERR_PTR(-EOPNOTSUPP);
+		}
+		*gp_is_root = true;
+	}
+
+	return no_free_ptr(res_xa);
+}
+
+/*
+ * cxl_rp_gather_bandwidth - handle the root port level bandwidth collection
+ * @xa: the xarray that holds the cxl_perf_ctx that has the bandwidth calculated
+ *      below each root port device.
+ *
+ * Return: xarray that holds cxl_perf_ctx per host bridge or ERR_PTR(-errno)
+ */
+static struct xarray *cxl_rp_gather_bandwidth(struct xarray *xa)
+{
+	struct xarray *hb_xa __free(free_perf_xa) =
+		kzalloc(sizeof(*hb_xa), GFP_KERNEL);
+	struct cxl_perf_ctx *ctx;
+	unsigned long index;
+
+	if (!hb_xa)
+		return ERR_PTR(-ENOMEM);
+	xa_init(hb_xa);
+
+	xa_for_each(xa, index, ctx) {
+		struct cxl_port *port = ctx->port;
+		unsigned long hb_index = (unsigned long)port->uport_dev;
+		struct cxl_perf_ctx *hb_ctx;
+		void *ptr;
+
+		hb_ctx = xa_load(hb_xa, hb_index);
+		if (!hb_ctx) {
+			struct cxl_perf_ctx *n __free(kfree) =
+				kzalloc(sizeof(*n), GFP_KERNEL);
+
+			if (!n)
+				return ERR_PTR(-ENOMEM);
+			ptr = xa_store(hb_xa, hb_index, n, GFP_KERNEL);
+			if (xa_is_err(ptr))
+				return ERR_PTR(xa_err(ptr));
+			hb_ctx = no_free_ptr(n);
+			hb_ctx->port = port;
+		}
+
+		cxl_bandwidth_add(hb_ctx->coord, hb_ctx->coord, ctx->coord);
+	}
+
+	return no_free_ptr(hb_xa);
+}
+
+/*
+ * cxl_hb_gather_bandwidth - handle the host bridge level bandwidth collection
+ * @xa: the xarray that holds the cxl_perf_ctx that has the bandwidth calculated
+ *      below each host bridge.
+ *
+ * Return: xarray that holds cxl_perf_ctx per ACPI0017 device or ERR_PTR(-errno)
+ */
+static struct xarray *cxl_hb_gather_bandwidth(struct xarray *xa)
+{
+	struct xarray *mw_xa __free(free_perf_xa) =
+		kzalloc(sizeof(*mw_xa), GFP_KERNEL);
+	struct cxl_perf_ctx *ctx;
+	unsigned long index;
+
+	if (!mw_xa)
+		return ERR_PTR(-ENOMEM);
+	xa_init(mw_xa);
+
+	xa_for_each(xa, index, ctx) {
+		struct cxl_port *port = ctx->port;
+		struct cxl_port *parent_port;
+		struct cxl_perf_ctx *mw_ctx;
+		struct cxl_dport *dport;
+		unsigned long mw_index;
+		void *ptr;
+
+		parent_port = to_cxl_port(port->dev.parent);
+		mw_index = (unsigned long)parent_port->uport_dev;
+
+		mw_ctx = xa_load(mw_xa, mw_index);
+		if (!mw_ctx) {
+			struct cxl_perf_ctx *n __free(kfree) =
+				kzalloc(sizeof(*n), GFP_KERNEL);
+
+			if (!n)
+				return ERR_PTR(-ENOMEM);
+			ptr = xa_store(mw_xa, mw_index, n, GFP_KERNEL);
+			if (xa_is_err(ptr))
+				return ERR_PTR(xa_err(ptr));
+			mw_ctx = no_free_ptr(n);
+		}
+
+		dport = port->parent_dport;
+		cxl_coordinates_combine(ctx->coord, ctx->coord, dport->coord);
+		cxl_bandwidth_add(mw_ctx->coord, mw_ctx->coord, ctx->coord);
+	}
+
+	return no_free_ptr(mw_xa);
+}
+
+/*
+ * cxl_region_update_bandwidth - Update the bandwidth access coordinates of a region
+ * @cxlr: The region being operated on
+ * @input_xa: xarray holds cxl_perf_ctx wht calculated bandwidth per ACPI0017 instance
+ */
+static void cxl_region_update_bandwidth(struct cxl_region *cxlr,
+					struct xarray *input_xa)
+{
+	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
+	struct cxl_perf_ctx *ctx;
+	unsigned long index;
+
+	memset(coord, 0, sizeof(coord));
+	xa_for_each(input_xa, index, ctx)
+		cxl_bandwidth_add(coord, coord, ctx->coord);
+
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
+		cxlr->coord[i].read_bandwidth = coord[i].read_bandwidth;
+		cxlr->coord[i].write_bandwidth = coord[i].write_bandwidth;
+	}
+}
+
+/*
+ * cxl_region_shared_upstream_perf_update - Recalculate the bandwidth for the region
+ * @cxl_region: the cxl region to recalculate
+ *
+ * The function walks the topology from bottom up and calculates the bandwidth. It
+ * starts at the endpoints, processes at the switches if any, processes at the rootport
+ * level, at the host bridge level, and finally aggregates at the region.
+ */
+void cxl_region_shared_upstream_bandwidth_update(struct cxl_region *cxlr)
+{
+	struct xarray *usp_xa, *working_xa;
+	int root_count = 0;
+	bool is_root;
+	int rc;
+
+	lockdep_assert_held(&cxl_dpa_rwsem);
+
+	usp_xa = kzalloc(sizeof(*usp_xa), GFP_KERNEL);
+	if (!usp_xa)
 		return;
+
+	xa_init(usp_xa);
+
+	/* Collect bandwidth data from all the endpoints. */
+	for (int i = 0; i < cxlr->params.nr_targets; i++) {
+		struct cxl_endpoint_decoder *cxled = cxlr->params.targets[i];
+
+		is_root = false;
+		rc = cxl_endpoint_gather_bandwidth(cxlr, cxled, usp_xa, &is_root);
+		if (rc) {
+			free_perf_xa(usp_xa);
+			return;
+		}
+		root_count += is_root;
 	}
 
+	/* Detect asymmetric hierachy with some direct attached endpoints. */
+	if (root_count && root_count != cxlr->params.nr_targets) {
+		dev_dbg(&cxlr->dev,
+			"Asymmetric hierachy detected, bandwidth not updated\n");
+		return;
+	}
+
+	/*
+	 * Walk up one or more switches to deal with the bandwidth of the
+	 * switches if they exist. Endpoints directly attached to RPs skip
+	 * over this part.
+	 */
+	if (!root_count) {
+		do {
+			working_xa = cxl_switch_gather_bandwidth(cxlr, usp_xa,
+								 &is_root);
+			if (IS_ERR(working_xa))
+				goto out;
+			free_perf_xa(usp_xa);
+			usp_xa = working_xa;
+		} while (!is_root);
+	}
+
+	/* Handle the bandwidth at the root port of the hierachy */
+	working_xa = cxl_rp_gather_bandwidth(usp_xa);
+	if (rc)
+		goto out;
+	free_perf_xa(usp_xa);
+	usp_xa = working_xa;
+
+	/* Handle the bandwidth at the host bridge of the hierachy */
+	working_xa = cxl_hb_gather_bandwidth(usp_xa);
+	if (rc)
+		goto out;
+	free_perf_xa(usp_xa);
+	usp_xa = working_xa;
+
+	/*
+	 * Aggregate all the bandwidth collected per CFMWS (ACPI0017) and
+	 * update the region bandwidth with the final calculated values.
+	 */
+	cxl_region_update_bandwidth(cxlr, usp_xa);
+
+out:
+	free_perf_xa(usp_xa);
+}
+
+void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
+				    struct cxl_endpoint_decoder *cxled)
+{
+	struct cxl_dpa_perf *perf;
+
 	lockdep_assert_held(&cxl_dpa_rwsem);
 
-	if (!range_contains(&perf->dpa_range, &dpa))
+	perf = cxled_get_dpa_perf(cxled, cxlr->mode);
+	if (IS_ERR(perf))
 		return;
 
 	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 625394486459..c72a7b9f5e21 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -103,9 +103,11 @@ enum cxl_poison_trace_type {
 };
 
 long cxl_pci_get_latency(struct pci_dev *pdev);
-
+int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c);
 int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
 				       enum access_coordinate_class access);
 bool cxl_need_node_perf_attrs_update(int nid);
+int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port,
+					struct access_coordinate *c);
 
 #endif /* __CXL_CORE_H__ */
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 8567dd11eaac..23b3d59c470d 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -1074,3 +1074,26 @@ bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port)
 				     __cxl_endpoint_decoder_reset_detected);
 }
 EXPORT_SYMBOL_NS_GPL(cxl_endpoint_decoder_reset_detected, CXL);
+
+int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c)
+{
+	int speed, bw;
+	u16 lnksta;
+	u32 width;
+
+	speed = pcie_link_speed_mbps(pdev);
+	if (speed < 0)
+		return speed;
+	speed /= BITS_PER_BYTE;
+
+	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnksta);
+	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, lnksta);
+	bw = speed * width;
+
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
+		c[i].read_bandwidth = bw;
+		c[i].write_bandwidth = bw;
+	}
+
+	return 0;
+}
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 887ed6e358fb..054b0db87f6d 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -2259,6 +2259,26 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 }
 EXPORT_SYMBOL_NS_GPL(cxl_endpoint_get_perf_coordinates, CXL);
 
+int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port,
+					struct access_coordinate *c)
+{
+	struct cxl_dport *dport = port->parent_dport;
+
+	/* Check this port is connected to a switch DSP and not an RP */
+	if (parent_port_is_cxl_root(to_cxl_port(port->dev.parent)))
+		return -ENODEV;
+
+	if (!coordinates_valid(dport->coord))
+		return -EINVAL;
+
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
+		c[i].read_bandwidth = dport->coord[i].read_bandwidth;
+		c[i].write_bandwidth = dport->coord[i].write_bandwidth;
+	}
+
+	return 0;
+}
+
 /* for user tooling to ensure port disable work has completed */
 static ssize_t flush_store(const struct bus_type *bus, const char *buf, size_t count)
 {
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 3c2b6144be23..92815481db6b 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1897,6 +1897,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 		 * then the region is already committed.
 		 */
 		p->state = CXL_CONFIG_COMMIT;
+		cxl_region_shared_upstream_bandwidth_update(cxlr);
 
 		return 0;
 	}
@@ -1918,6 +1919,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 		if (rc)
 			return rc;
 		p->state = CXL_CONFIG_ACTIVE;
+		cxl_region_shared_upstream_bandwidth_update(cxlr);
 	}
 
 	cxled->cxld.interleave_ways = p->interleave_ways;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 603c0120cff8..3dfb1e67baad 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -891,6 +891,7 @@ int cxl_endpoint_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);
+void cxl_region_shared_upstream_bandwidth_update(struct cxl_region *cxlr);
 
 void cxl_memdev_update_perf(struct cxl_memdev *cxlmd);
 
-- 
2.45.2

Re: [PATCH v7 2/3] cxl: Calculate region bandwidth of targets with shared upstream link

[Alison Schofield]

On Wed, Jul 10, 2024 at 03:24:01PM -0700, Dave Jiang wrote:
> The current bandwidth calculation aggregates all the targets. This simple
> method does not take into account where multiple targets sharing under
> a switch or a root port where the aggregated bandwidth can be greater than
> the upstream link of the switch.
> 
> To accurately account for the shared upstream uplink cases, a new update
> function is introduced by walking from the leaves to the root of the
> hierarchy and clamp the bandwidth in the process as needed. This process
> is done when all the targets for a region are present but before the
> final values are send to the HMAT handling code cached access_coordinate
> targets.
> 
> The original perf calculation path was kept to calculate the latency
> performance data that does not require the shared link consideration.
> The shared upstream link calculation is done as a second pass when all
> the endpoints have arrived.
> 
> Testing is done via qemu with CXL hierachy. run_qemu[1] is modified to
> support several CXL hierachy layouts. The following layouts are tested:
> 
> HB: Host Bridge
> RP: Root Port
> SW: Switch
> EP: End Point
> 
> 2 HB 2 RP 2 EP: resulting bandwidth: 624
> 1 HB 2 RP 2 EP: resulting bandwidth: 624
> 2 HB 2 RP 2 SW 4 EP: resulting bandwidth: 624
> 
> Current testing, perf number from SRAT/HMAT is hacked into the kernel
> code. However with new QEMU support of Generic Target Port that's
> incoming, the perf data injection is no longer needed.
> 
> [1]: https://github.com/pmem/run_qemu
> 
> Suggested-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Link: https://lore.kernel.org/linux-cxl/20240501152503.00002e60@Huawei.com/
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> ---
> v7:
> - Add test notes in commit log. (Dan)
> - Move cxl_memdev_get_dpa_perf() to cxled_memdev_get_dpa_perf(). (Dan)
> - Add a DEFINE_FREE(free_perf_xa). (Dan)
> - Address 2hb2rp2ep issue Jonathan reported. (Jonathan)
> - Added more kdoc comment headers. (Dan)

Looks like potential kdocs are not annotated w /** 


> - Rename helper functions to be more clear on what they do. (Dan)
> - Move activiation point to after cxl_region_setup_targets(). (Dan)
> ---

snip

> +/*
> + * cxl_region_shared_upstream_perf_update - Recalculate the bandwidth for the region
> + * @cxl_region: the cxl region to recalculate
> + *
> + * The function walks the topology from bottom up and calculates the bandwidth. It
> + * starts at the endpoints, processes at the switches if any, processes at the rootport
> + * level, at the host bridge level, and finally aggregates at the region.
> + */
> +void cxl_region_shared_upstream_bandwidth_update(struct cxl_region *cxlr)
> +{
> +	struct xarray *usp_xa, *working_xa;
> +	int root_count = 0;
> +	bool is_root;
> +	int rc;
> +
> +	lockdep_assert_held(&cxl_dpa_rwsem);
> +
> +	usp_xa = kzalloc(sizeof(*usp_xa), GFP_KERNEL);
> +	if (!usp_xa)
>  		return;
> +
> +	xa_init(usp_xa);
> +
> +	/* Collect bandwidth data from all the endpoints. */
> +	for (int i = 0; i < cxlr->params.nr_targets; i++) {
> +		struct cxl_endpoint_decoder *cxled = cxlr->params.targets[i];
> +
> +		is_root = false;
> +		rc = cxl_endpoint_gather_bandwidth(cxlr, cxled, usp_xa, &is_root);
> +		if (rc) {
> +			free_perf_xa(usp_xa);
> +			return;
> +		}
> +		root_count += is_root;
>  	}
>  
> +	/* Detect asymmetric hierachy with some direct attached endpoints. */
> +	if (root_count && root_count != cxlr->params.nr_targets) {
> +		dev_dbg(&cxlr->dev,
> +			"Asymmetric hierachy detected, bandwidth not updated\n");
> +		return;
> +	}
> +
> +	/*
> +	 * Walk up one or more switches to deal with the bandwidth of the
> +	 * switches if they exist. Endpoints directly attached to RPs skip
> +	 * over this part.
> +	 */
> +	if (!root_count) {
> +		do {
> +			working_xa = cxl_switch_gather_bandwidth(cxlr, usp_xa,
> +								 &is_root);
> +			if (IS_ERR(working_xa))
> +				goto out;
> +			free_perf_xa(usp_xa);
> +			usp_xa = working_xa;
> +		} while (!is_root);
> +	}
> +
> +	/* Handle the bandwidth at the root port of the hierachy */
> +	working_xa = cxl_rp_gather_bandwidth(usp_xa);
> +	if (rc)
> +		goto out;
> +	free_perf_xa(usp_xa);

I was going to say something about getting rid of the goto's
and just freeing and returning in line like was done ~36 lines
above..but then realize something went astray here in the code
movement.  Here and below, rc isn't set.


> +	usp_xa = working_xa;
> +
> +	/* Handle the bandwidth at the host bridge of the hierachy */
> +	working_xa = cxl_hb_gather_bandwidth(usp_xa);
> +	if (rc)
> +		goto out;
> +	free_perf_xa(usp_xa);
> +	usp_xa = working_xa;
> +
> +	/*
> +	 * Aggregate all the bandwidth collected per CFMWS (ACPI0017) and
> +	 * update the region bandwidth with the final calculated values.
> +	 */
> +	cxl_region_update_bandwidth(cxlr, usp_xa);
> +
> +out:
> +	free_perf_xa(usp_xa);
> +}

snip

> 
> 

Re: [PATCH v7 2/3] cxl: Calculate region bandwidth of targets with shared upstream link

[Dave Jiang]

On 7/10/24 6:39 PM, Alison Schofield wrote:
> On Wed, Jul 10, 2024 at 03:24:01PM -0700, Dave Jiang wrote:
>> The current bandwidth calculation aggregates all the targets. This simple
>> method does not take into account where multiple targets sharing under
>> a switch or a root port where the aggregated bandwidth can be greater than
>> the upstream link of the switch.
>>
>> To accurately account for the shared upstream uplink cases, a new update
>> function is introduced by walking from the leaves to the root of the
>> hierarchy and clamp the bandwidth in the process as needed. This process
>> is done when all the targets for a region are present but before the
>> final values are send to the HMAT handling code cached access_coordinate
>> targets.
>>
>> The original perf calculation path was kept to calculate the latency
>> performance data that does not require the shared link consideration.
>> The shared upstream link calculation is done as a second pass when all
>> the endpoints have arrived.
>>
>> Testing is done via qemu with CXL hierachy. run_qemu[1] is modified to
>> support several CXL hierachy layouts. The following layouts are tested:
>>
>> HB: Host Bridge
>> RP: Root Port
>> SW: Switch
>> EP: End Point
>>
>> 2 HB 2 RP 2 EP: resulting bandwidth: 624
>> 1 HB 2 RP 2 EP: resulting bandwidth: 624
>> 2 HB 2 RP 2 SW 4 EP: resulting bandwidth: 624
>>
>> Current testing, perf number from SRAT/HMAT is hacked into the kernel
>> code. However with new QEMU support of Generic Target Port that's
>> incoming, the perf data injection is no longer needed.
>>
>> [1]: https://github.com/pmem/run_qemu
>>
>> Suggested-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Link: https://lore.kernel.org/linux-cxl/20240501152503.00002e60@Huawei.com/
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>
>> ---
>> v7:
>> - Add test notes in commit log. (Dan)
>> - Move cxl_memdev_get_dpa_perf() to cxled_memdev_get_dpa_perf(). (Dan)
>> - Add a DEFINE_FREE(free_perf_xa). (Dan)
>> - Address 2hb2rp2ep issue Jonathan reported. (Jonathan)
>> - Added more kdoc comment headers. (Dan)
> 
> Looks like potential kdocs are not annotated w /** 

Ooof yeah I missed them all. Will fix.

> 
> 
>> - Rename helper functions to be more clear on what they do. (Dan)
>> - Move activiation point to after cxl_region_setup_targets(). (Dan)
>> ---
> 
> snip
> 
>> +/*
>> + * cxl_region_shared_upstream_perf_update - Recalculate the bandwidth for the region
>> + * @cxl_region: the cxl region to recalculate
>> + *
>> + * The function walks the topology from bottom up and calculates the bandwidth. It
>> + * starts at the endpoints, processes at the switches if any, processes at the rootport
>> + * level, at the host bridge level, and finally aggregates at the region.
>> + */
>> +void cxl_region_shared_upstream_bandwidth_update(struct cxl_region *cxlr)
>> +{
>> +	struct xarray *usp_xa, *working_xa;
>> +	int root_count = 0;
>> +	bool is_root;
>> +	int rc;
>> +
>> +	lockdep_assert_held(&cxl_dpa_rwsem);
>> +
>> +	usp_xa = kzalloc(sizeof(*usp_xa), GFP_KERNEL);
>> +	if (!usp_xa)
>>  		return;
>> +
>> +	xa_init(usp_xa);
>> +
>> +	/* Collect bandwidth data from all the endpoints. */
>> +	for (int i = 0; i < cxlr->params.nr_targets; i++) {
>> +		struct cxl_endpoint_decoder *cxled = cxlr->params.targets[i];
>> +
>> +		is_root = false;
>> +		rc = cxl_endpoint_gather_bandwidth(cxlr, cxled, usp_xa, &is_root);
>> +		if (rc) {
>> +			free_perf_xa(usp_xa);
>> +			return;
>> +		}
>> +		root_count += is_root;
>>  	}
>>  
>> +	/* Detect asymmetric hierachy with some direct attached endpoints. */
>> +	if (root_count && root_count != cxlr->params.nr_targets) {
>> +		dev_dbg(&cxlr->dev,
>> +			"Asymmetric hierachy detected, bandwidth not updated\n");
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * Walk up one or more switches to deal with the bandwidth of the
>> +	 * switches if they exist. Endpoints directly attached to RPs skip
>> +	 * over this part.
>> +	 */
>> +	if (!root_count) {
>> +		do {
>> +			working_xa = cxl_switch_gather_bandwidth(cxlr, usp_xa,
>> +								 &is_root);
>> +			if (IS_ERR(working_xa))
>> +				goto out;
>> +			free_perf_xa(usp_xa);
>> +			usp_xa = working_xa;
>> +		} while (!is_root);
>> +	}
>> +
>> +	/* Handle the bandwidth at the root port of the hierachy */
>> +	working_xa = cxl_rp_gather_bandwidth(usp_xa);
>> +	if (rc)
>> +		goto out;
>> +	free_perf_xa(usp_xa);
> 
> I was going to say something about getting rid of the goto's
> and just freeing and returning in line like was done ~36 lines
> above..but then realize something went astray here in the code
> movement.  Here and below, rc isn't set.

Copy paste mistake looks like. Will fix. Also will remove the gotos. 

> 
> 
>> +	usp_xa = working_xa;
>> +
>> +	/* Handle the bandwidth at the host bridge of the hierachy */
>> +	working_xa = cxl_hb_gather_bandwidth(usp_xa);
>> +	if (rc)
>> +		goto out;
>> +	free_perf_xa(usp_xa);
>> +	usp_xa = working_xa;
>> +
>> +	/*
>> +	 * Aggregate all the bandwidth collected per CFMWS (ACPI0017) and
>> +	 * update the region bandwidth with the final calculated values.
>> +	 */
>> +	cxl_region_update_bandwidth(cxlr, usp_xa);
>> +
>> +out:
>> +	free_perf_xa(usp_xa);
>> +}
> 
> snip
> 
>>
>>
> 

Re: [PATCH v7 2/3] cxl: Calculate region bandwidth of targets with shared upstream link

[kernel test robot]

Hi Dave,

kernel test robot noticed the following build warnings:

[auto build test WARNING on v6.10-rc6]
[cannot apply to cxl/next linus/master cxl/pending v6.10-rc7 next-20240710]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dave-Jiang/cxl-Preserve-the-CDAT-access_coordinate-for-an-endpoint/20240711-092210
base:   v6.10-rc6
patch link:    https://lore.kernel.org/r/20240710222716.797267-3-dave.jiang%40intel.com
patch subject: [PATCH v7 2/3] cxl: Calculate region bandwidth of targets with shared upstream link
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20240711/202407111533.ptNkhsLL-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project a0c6b8aef853eedaa0980f07c0a502a5a8a9740e)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240711/202407111533.ptNkhsLL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407111533.ptNkhsLL-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/cxl/core/cdat.c:3:
   In file included from include/linux/acpi.h:14:
   In file included from include/linux/device.h:32:
   In file included from include/linux/device/driver.h:21:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:173:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2258:
   include/linux/vmstat.h:500:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     500 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     501 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:507:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     507 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     508 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:519:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     519 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     520 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:528:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     528 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     529 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/cxl/core/cdat.c:8:
   In file included from drivers/cxl/cxlpci.h:5:
   In file included from include/linux/pci.h:39:
   In file included from include/linux/io.h:14:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
         |                                                           ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
     102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
         |                                                      ^
   In file included from drivers/cxl/core/cdat.c:8:
   In file included from drivers/cxl/cxlpci.h:5:
   In file included from include/linux/pci.h:39:
   In file included from include/linux/io.h:14:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
         |                                                           ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
     115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
         |                                                      ^
   In file included from drivers/cxl/core/cdat.c:8:
   In file included from drivers/cxl/cxlpci.h:5:
   In file included from include/linux/pci.h:39:
   In file included from include/linux/io.h:14:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     693 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     701 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     709 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     718 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     727 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     736 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
>> drivers/cxl/core/cdat.c:776:7: warning: variable 'is_root' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     776 |                 if (is_cxl_root(gp_port)) {
         |                     ^~~~~~~~~~~~~~~~~~~~
   drivers/cxl/core/cdat.c:785:7: note: uninitialized use occurs here
     785 |                 if (is_root)
         |                     ^~~~~~~
   drivers/cxl/core/cdat.c:776:3: note: remove the 'if' if its condition is always true
     776 |                 if (is_cxl_root(gp_port)) {
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/cxl/core/cdat.c:773:15: note: initialize the variable 'is_root' to silence this warning
     773 |                 bool is_root;
         |                             ^
         |                              = 0
   18 warnings generated.


vim +776 drivers/cxl/core/cdat.c

   732	
   733	/*
   734	 * cxl_switch_gather_bandwidth - collect all the bandwidth at switch level in an xarray
   735	 * @cxlr: The region being operated on
   736	 * @input_xa: xarray indexed by upstream device of a switch with data of 'struct
   737	 *	      cxl_perf_ctx'
   738	 * @gp_is_root: (output) bool of whether the grandparent is cxl root.
   739	 *
   740	 * Return: a xarray of resulting cxl_perf_ctx per parent switch or root port
   741	 *         or ERR_PTR(-errno)
   742	 *
   743	 * Iterate through the xarray. Take the minimum of the downstream calculated
   744	 * bandwidth, the upstream link bandwidth, and the SSLBIS of the upstream
   745	 * switch if exists. Sum the resulting bandwidth under the switch upstream
   746	 * device or a RP device. The function can be iterated over multiple switches
   747	 * if the switches are present.
   748	 */
   749	static struct xarray *cxl_switch_gather_bandwidth(struct cxl_region *cxlr,
   750							  struct xarray *input_xa,
   751							  bool *gp_is_root)
   752	{
   753		struct xarray *res_xa __free(free_perf_xa) =
   754			kzalloc(sizeof(*res_xa), GFP_KERNEL);
   755		struct access_coordinate coords[ACCESS_COORDINATE_MAX];
   756		struct cxl_perf_ctx *ctx, *us_ctx;
   757		unsigned long index, us_index;
   758		int dev_count = 0;
   759		int gp_count = 0;
   760		void *ptr;
   761		int rc;
   762	
   763		if (!res_xa)
   764			return ERR_PTR(-ENOMEM);
   765		xa_init(res_xa);
   766	
   767		xa_for_each(input_xa, index, ctx) {
   768			struct device *dev = (struct device *)index;
   769			struct cxl_port *port = ctx->port;
   770			struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
   771			struct cxl_port *gp_port = to_cxl_port(parent_port->dev.parent);
   772			struct cxl_dport *dport = port->parent_dport;
   773			bool is_root;
   774	
   775			dev_count++;
 > 776			if (is_cxl_root(gp_port)) {
   777				is_root = true;
   778				gp_count++;
   779			}
   780	
   781			/*
   782			 * If the grandparent is cxl root, then index is the root port,
   783			 * otherwise it's the parent switch upstream device.
   784			 */
   785			if (is_root)
   786				us_index = (unsigned long)port->parent_dport->dport_dev;
   787			else
   788				us_index = (unsigned long)parent_port->uport_dev;
   789	
   790			us_ctx = xa_load(res_xa, us_index);
   791			if (!us_ctx) {
   792				struct cxl_perf_ctx *n __free(kfree) =
   793					kzalloc(sizeof(*n), GFP_KERNEL);
   794	
   795				if (!n)
   796					return ERR_PTR(-ENOMEM);
   797	
   798				ptr = xa_store(res_xa, us_index, n, GFP_KERNEL);
   799				if (xa_is_err(ptr))
   800					return ERR_PTR(xa_err(ptr));
   801				us_ctx = no_free_ptr(n);
   802				us_ctx->port = parent_port;
   803			}
   804	
   805			/*
   806			 * If the device isn't an upstream PCIe port, there's something
   807			 * wrong with the topology.
   808			 */
   809			if (!dev_is_pci(dev))
   810				return ERR_PTR(-EINVAL);
   811	
   812			/* Retrieve the upstream link bandwidth */
   813			rc = cxl_pci_get_bandwidth(to_pci_dev(dev), coords);
   814			if (rc)
   815				return ERR_PTR(-ENXIO);
   816	
   817			/*
   818			 * Take the min of downstream bandwidth and the upstream link
   819			 * bandwidth.
   820			 */
   821			cxl_coordinates_combine(coords, coords, ctx->coord);
   822	
   823			/*
   824			 * Take the min of the calculated bandwdith and the upstream
   825			 * switch SSLBIS bandwidth if there's a parent switch
   826			 */
   827			if (!is_root)
   828				cxl_coordinates_combine(coords, coords, dport->coord);
   829	
   830			/*
   831			 * Aggregate the calculated bandwidth common to an upstream
   832			 * switch.
   833			 */
   834			cxl_bandwidth_add(us_ctx->coord, us_ctx->coord, coords);
   835		}
   836	
   837		/* Asymmetric topology detected. */
   838		if (gp_count) {
   839			if (gp_count != dev_count) {
   840				dev_dbg(&cxlr->dev,
   841					"Asymmetric hierachy detected, bandwidth not updated\n");
   842				return ERR_PTR(-EOPNOTSUPP);
   843			}
   844			*gp_is_root = true;
   845		}
   846	
   847		return no_free_ptr(res_xa);
   848	}
   849	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v7 2/3] cxl: Calculate region bandwidth of targets with shared upstream link

[Dan Carpenter]

Hi Dave,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dave-Jiang/cxl-Preserve-the-CDAT-access_coordinate-for-an-endpoint/20240711-092210
base:   v6.10-rc6
patch link:    https://lore.kernel.org/r/20240710222716.797267-3-dave.jiang%40intel.com
patch subject: [PATCH v7 2/3] cxl: Calculate region bandwidth of targets with shared upstream link
config: i386-randconfig-141-20240711 (https://download.01.org/0day-ci/archive/20240711/202407111927.7Q2PF9i0-lkp@intel.com/config)
compiler: gcc-10 (Ubuntu 10.5.0-1ubuntu1) 10.5.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202407111927.7Q2PF9i0-lkp@intel.com/

smatch warnings:
drivers/cxl/core/cdat.c:785 cxl_switch_gather_bandwidth() error: uninitialized symbol 'is_root'.

vim +/is_root +785 drivers/cxl/core/cdat.c

dfbcea34995e70 Dave Jiang 2024-07-10  749  static struct xarray *cxl_switch_gather_bandwidth(struct cxl_region *cxlr,
dfbcea34995e70 Dave Jiang 2024-07-10  750  						  struct xarray *input_xa,
dfbcea34995e70 Dave Jiang 2024-07-10  751  						  bool *gp_is_root)
dfbcea34995e70 Dave Jiang 2024-07-10  752  {
dfbcea34995e70 Dave Jiang 2024-07-10  753  	struct xarray *res_xa __free(free_perf_xa) =
dfbcea34995e70 Dave Jiang 2024-07-10  754  		kzalloc(sizeof(*res_xa), GFP_KERNEL);
dfbcea34995e70 Dave Jiang 2024-07-10  755  	struct access_coordinate coords[ACCESS_COORDINATE_MAX];
dfbcea34995e70 Dave Jiang 2024-07-10  756  	struct cxl_perf_ctx *ctx, *us_ctx;
dfbcea34995e70 Dave Jiang 2024-07-10  757  	unsigned long index, us_index;
dfbcea34995e70 Dave Jiang 2024-07-10  758  	int dev_count = 0;
dfbcea34995e70 Dave Jiang 2024-07-10  759  	int gp_count = 0;
dfbcea34995e70 Dave Jiang 2024-07-10  760  	void *ptr;
dfbcea34995e70 Dave Jiang 2024-07-10  761  	int rc;
dfbcea34995e70 Dave Jiang 2024-07-10  762  
dfbcea34995e70 Dave Jiang 2024-07-10  763  	if (!res_xa)
dfbcea34995e70 Dave Jiang 2024-07-10  764  		return ERR_PTR(-ENOMEM);
dfbcea34995e70 Dave Jiang 2024-07-10  765  	xa_init(res_xa);
dfbcea34995e70 Dave Jiang 2024-07-10  766  
dfbcea34995e70 Dave Jiang 2024-07-10  767  	xa_for_each(input_xa, index, ctx) {
dfbcea34995e70 Dave Jiang 2024-07-10  768  		struct device *dev = (struct device *)index;
dfbcea34995e70 Dave Jiang 2024-07-10  769  		struct cxl_port *port = ctx->port;
dfbcea34995e70 Dave Jiang 2024-07-10  770  		struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
dfbcea34995e70 Dave Jiang 2024-07-10  771  		struct cxl_port *gp_port = to_cxl_port(parent_port->dev.parent);
dfbcea34995e70 Dave Jiang 2024-07-10  772  		struct cxl_dport *dport = port->parent_dport;
dfbcea34995e70 Dave Jiang 2024-07-10  773  		bool is_root;

needs to be initialized to false

dfbcea34995e70 Dave Jiang 2024-07-10  774  
dfbcea34995e70 Dave Jiang 2024-07-10  775  		dev_count++;
dfbcea34995e70 Dave Jiang 2024-07-10  776  		if (is_cxl_root(gp_port)) {
dfbcea34995e70 Dave Jiang 2024-07-10  777  			is_root = true;
dfbcea34995e70 Dave Jiang 2024-07-10  778  			gp_count++;
dfbcea34995e70 Dave Jiang 2024-07-10  779  		}
dfbcea34995e70 Dave Jiang 2024-07-10  780  
dfbcea34995e70 Dave Jiang 2024-07-10  781  		/*
dfbcea34995e70 Dave Jiang 2024-07-10  782  		 * If the grandparent is cxl root, then index is the root port,
dfbcea34995e70 Dave Jiang 2024-07-10  783  		 * otherwise it's the parent switch upstream device.
dfbcea34995e70 Dave Jiang 2024-07-10  784  		 */
dfbcea34995e70 Dave Jiang 2024-07-10 @785  		if (is_root)
dfbcea34995e70 Dave Jiang 2024-07-10  786  			us_index = (unsigned long)port->parent_dport->dport_dev;
dfbcea34995e70 Dave Jiang 2024-07-10  787  		else
dfbcea34995e70 Dave Jiang 2024-07-10  788  			us_index = (unsigned long)parent_port->uport_dev;
dfbcea34995e70 Dave Jiang 2024-07-10  789  
dfbcea34995e70 Dave Jiang 2024-07-10  790  		us_ctx = xa_load(res_xa, us_index);
dfbcea34995e70 Dave Jiang 2024-07-10  791  		if (!us_ctx) {

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v7 3/3] cxl: Add documentation to explain the shared link bandwidth calculation

[Dave Jiang]

Create a kernel documentation to describe how the CXL shared upstream
link bandwidth is calculated.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 .../driver-api/cxl/access-coordinates.rst     | 90 +++++++++++++++++++
 Documentation/driver-api/cxl/index.rst        |  1 +
 MAINTAINERS                                   |  1 +
 3 files changed, 92 insertions(+)
 create mode 100644 Documentation/driver-api/cxl/access-coordinates.rst

diff --git a/Documentation/driver-api/cxl/access-coordinates.rst b/Documentation/driver-api/cxl/access-coordinates.rst
new file mode 100644
index 000000000000..973e63872f06
--- /dev/null
+++ b/Documentation/driver-api/cxl/access-coordinates.rst
@@ -0,0 +1,90 @@
+.. SPDX-License-Identifier: GPL-2.0
+.. include:: <isonum.txt>
+
+==================================
+CXL Access Coordinates Computation
+==================================
+
+Shared Upstream Link Calculation
+================================
+For certain CXL region construction with endpoints behind CXL switches (SW) or
+Root Ports (RP), there is the possibility of the total bandwdith for all
+the endpoints behind a switch being more than the switch upstream link.
+The CXL driver performs an additional pass after all the targets have
+arrived for a region in order to recalculate the bandwidths with possible
+upstream link being a limiting factor in mind.
+
+The algorithm assumes the configuration is a symmetric topology as that
+maximizes performance. When asymmetric topology is detected, the calculation
+is aborted when such topology is detected. An asymmetric topology is detected
+during topology walk where the number of RPs detected as a grandparent is not
+equal to the number of devices iterated in the same iteration loop.
+
+There can be multiple switches under a RP. There can be multiple RPs under
+a CXL Host Bridge (HB). There can be multiple HBs under a CXL Fixed Memory
+Window Structure (CFMWS).
+
+An example hierarchy:
+
+                CFMWS 0
+                  |
+         _________|_________
+        |                   |
+    ACPI0017-0          ACPI0017-1
+ GP0/HB0/ACPI0016-0   GP1/HB1/ACPI0016-1
+    |          |        |           |
+   RP0        RP1      RP2         RP3
+    |          |        |           |
+  SW 0       SW 1     SW 2        SW 3
+  |   |      |   |    |   |       |   |
+ EP0 EP1    EP2 EP3  EP4  EP5    EP6 EP7
+
+Computation for the example hierarchy:
+
+Min (GP0 to CPU BW,
+     Min(SW 0 Upstream Link to RP0 BW,
+         Min(SW0SSLBIS for SW0DSP0 (EP0), EP0 DSLBIS, EP0 Upstream Link) +
+         Min(SW0SSLBIS for SW0DSP1 (EP1), EP1 DSLBIS, EP1 Upstream link)) +
+     Min(SW 1 Upstream Link to RP1 BW,
+         Min(SW1SSLBIS for SW1DSP0 (EP2), EP2 DSLBIS, EP2 Upstream Link) +
+         Min(SW1SSLBIS for SW1DSP1 (EP3), EP3 DSLBIS, EP3 Upstream link))) +
+Min (GP1 to CPU BW,
+     Min(SW 2 Upstream Link to RP2 BW,
+         Min(SW2SSLBIS for SW2DSP0 (EP4), EP4 DSLBIS, EP4 Upstream Link) +
+         Min(SW2SSLBIS for SW2DSP1 (EP5), EP5 DSLBIS, EP5 Upstream link)) +
+     Min(SW 3 Upstream Link to RP3 BW,
+         Min(SW3SSLBIS for SW3DSP0 (EP6), EP6 DSLBIS, EP6 Upstream Link) +
+         Min(SW3SSLBIS for SW3DSP1 (EP7), EP7 DSLBIS, EP7 Upstream link))))
+
+
+The calculation starts at cxl_region_shared_upstream_perf_update(). A xarray
+is created to collect all the endpoint bandwidths via the
+cxl_endpoint_gather_bandwidth() function. The min() of bandwidth from the
+endpoint CDAT and the upstream link bandwidth is calculated. If the endpoint
+has a CXL switch as a parent, then min() of calculated bandwidth and the
+bandwidth from the SSLBIS for the switch downstream port that is associated
+with the endpoint is calculated. The final bandwidth is stored in a
+'struct cxl_perf_ctx' in the xarray indexed by a device pointer. If the
+endpoint is direct attached to a root port (RP), the device pointer would be a
+RP device. If the endpoint is behind a switch, the device pointer would be the
+upstream device of the parent switch.
+
+At the next stage, the code attempts to walk through one or more switches if
+they exist in the topology. For endpoints directly attached to RPs, this
+step is skipped. If there is another switch upstream, the code takes the min()
+of the current gathered bandwidth and the upstream link bandwidth. If there's
+a switch upstream, then the SSLBIS of the upstream switch.
+
+Once the topology walk reaches the RP, whether it's direct attached endpoints
+or walking through the switch(es), cxl_rp_gather_bandwidth() is called. At
+this point all the bandwidths are aggregated per each host bridge, which is
+also the index for the resulting xarray.
+
+The next step is to take the min() of the per host bridge bandwidth and the
+bandwidth from the Generic Port (GP). The bandwidths for the GP is retrieved
+via ACPI tables SRAT/HMAT. The min bandwidth are aggregated under the same
+ACPI0017 device to form a new xarray.
+
+Finally, the cxl_region_update_bandwidth() is called and the aggregated
+bandwidth from all the members of the last xarray is updated for the
+access coordinates residing in the cxl region (cxlr) context.
diff --git a/Documentation/driver-api/cxl/index.rst b/Documentation/driver-api/cxl/index.rst
index 036e49553542..9b0e82a31d89 100644
--- a/Documentation/driver-api/cxl/index.rst
+++ b/Documentation/driver-api/cxl/index.rst
@@ -8,5 +8,6 @@ Compute Express Link
    :maxdepth: 1
 
    memory-devices
+   access-coordinates
 
 .. only::  subproject and html
diff --git a/MAINTAINERS b/MAINTAINERS
index 2ca8f35dfe03..2b3bfc114140 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5484,6 +5484,7 @@ M:	Ira Weiny <ira.weiny@intel.com>
 M:	Dan Williams <dan.j.williams@intel.com>
 L:	linux-cxl@vger.kernel.org
 S:	Maintained
+F:	Documentation/driver-api/cxl/
 F:	drivers/cxl/
 F:	include/linux/einj-cxl.h
 F:	include/linux/cxl-event.h
-- 
2.45.2

Re: [PATCH v7 3/3] cxl: Add documentation to explain the shared link bandwidth calculation

[Jonathan Cameron]

On Wed, 10 Jul 2024 15:24:02 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Create a kernel documentation to describe how the CXL shared upstream
> link bandwidth is calculated.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Oops. Missed this previously.  A few minor things inline.

J
> ---
>  .../driver-api/cxl/access-coordinates.rst     | 90 +++++++++++++++++++
>  Documentation/driver-api/cxl/index.rst        |  1 +
>  MAINTAINERS                                   |  1 +
>  3 files changed, 92 insertions(+)
>  create mode 100644 Documentation/driver-api/cxl/access-coordinates.rst
> 
> diff --git a/Documentation/driver-api/cxl/access-coordinates.rst b/Documentation/driver-api/cxl/access-coordinates.rst
> new file mode 100644
> index 000000000000..973e63872f06
> --- /dev/null
> +++ b/Documentation/driver-api/cxl/access-coordinates.rst
> @@ -0,0 +1,90 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +.. include:: <isonum.txt>
> +
> +==================================
> +CXL Access Coordinates Computation
> +==================================
> +
> +Shared Upstream Link Calculation
> +================================
> +For certain CXL region construction with endpoints behind CXL switches (SW) or
> +Root Ports (RP), there is the possibility of the total bandwdith for all
spell check. bandwidth
> +the endpoints behind a switch being more than the switch upstream link.

Could also be the Generic Port bit of the topology. Mind you I'm still failing
to come up with text for the qemu GP Documentation that the reviewers can follow
so maybe that's just too hard to describe here.  Could use weasel words.

"A similar situation can occur within the host, upstream of the root ports."


> +The CXL driver performs an additional pass after all the targets have
> +arrived for a region in order to recalculate the bandwidths with possible
> +upstream link being a limiting factor in mind.
> +
> +The algorithm assumes the configuration is a symmetric topology as that
> +maximizes performance. When asymmetric topology is detected, the calculation
> +is aborted when such topology is detected. An asymmetric topology is detected

is detected is duplicated.

> +during topology walk where the number of RPs detected as a grandparent is not
> +equal to the number of devices iterated in the same iteration loop.

Maybe make the point that asymmetric in terms of only properties of devices
is not detected. It just uses the first one I think?
> +
> +There can be multiple switches under a RP. There can be multiple RPs under
> +a CXL Host Bridge (HB). There can be multiple HBs under a CXL Fixed Memory
> +Window Structure (CFMWS).
> +
> +An example hierarchy:
> +
> +                CFMWS 0
> +                  |
> +         _________|_________
> +        |                   |
> +    ACPI0017-0          ACPI0017-1
> + GP0/HB0/ACPI0016-0   GP1/HB1/ACPI0016-1
> +    |          |        |           |
> +   RP0        RP1      RP2         RP3
> +    |          |        |           |
> +  SW 0       SW 1     SW 2        SW 3
> +  |   |      |   |    |   |       |   |
> + EP0 EP1    EP2 EP3  EP4  EP5    EP6 EP7
> +
> +Computation for the example hierarchy:
> +
> +Min (GP0 to CPU BW,
> +     Min(SW 0 Upstream Link to RP0 BW,
> +         Min(SW0SSLBIS for SW0DSP0 (EP0), EP0 DSLBIS, EP0 Upstream Link) +
> +         Min(SW0SSLBIS for SW0DSP1 (EP1), EP1 DSLBIS, EP1 Upstream link)) +
> +     Min(SW 1 Upstream Link to RP1 BW,
> +         Min(SW1SSLBIS for SW1DSP0 (EP2), EP2 DSLBIS, EP2 Upstream Link) +
> +         Min(SW1SSLBIS for SW1DSP1 (EP3), EP3 DSLBIS, EP3 Upstream link))) +
> +Min (GP1 to CPU BW,
> +     Min(SW 2 Upstream Link to RP2 BW,
> +         Min(SW2SSLBIS for SW2DSP0 (EP4), EP4 DSLBIS, EP4 Upstream Link) +
> +         Min(SW2SSLBIS for SW2DSP1 (EP5), EP5 DSLBIS, EP5 Upstream link)) +
> +     Min(SW 3 Upstream Link to RP3 BW,
> +         Min(SW3SSLBIS for SW3DSP0 (EP6), EP6 DSLBIS, EP6 Upstream Link) +
> +         Min(SW3SSLBIS for SW3DSP1 (EP7), EP7 DSLBIS, EP7 Upstream link))))
> +
> +
> +The calculation starts at cxl_region_shared_upstream_perf_update(). A xarray
> +is created to collect all the endpoint bandwidths via the
> +cxl_endpoint_gather_bandwidth() function. The min() of bandwidth from the
> +endpoint CDAT and the upstream link bandwidth is calculated. If the endpoint
> +has a CXL switch as a parent, then min() of calculated bandwidth and the
> +bandwidth from the SSLBIS for the switch downstream port that is associated
> +with the endpoint is calculated. The final bandwidth is stored in a
> +'struct cxl_perf_ctx' in the xarray indexed by a device pointer. If the
> +endpoint is direct attached to a root port (RP), the device pointer would be a
> +RP device. If the endpoint is behind a switch, the device pointer would be the
an RP 
> +upstream device of the parent switch.
> +
> +At the next stage, the code attempts to walk through one or more switches if
the code walks through
(I doubt it ever fails and if it does that's detail you don't need here)
> +they exist in the topology. For endpoints directly attached to RPs, this
> +step is skipped. If there is another switch upstream, the code takes the min()
> +of the current gathered bandwidth and the upstream link bandwidth. If there's
> +a switch upstream, then the SSLBIS of the upstream switch.
> +
> +Once the topology walk reaches the RP, whether it's direct attached endpoints
> +or walking through the switch(es), cxl_rp_gather_bandwidth() is called. At
> +this point all the bandwidths are aggregated per each host bridge, which is
> +also the index for the resulting xarray.
> +
> +The next step is to take the min() of the per host bridge bandwidth and the
> +bandwidth from the Generic Port (GP). The bandwidths for the GP is retrieved
> +via ACPI tables SRAT/HMAT. The min bandwidth are aggregated under the same
> +ACPI0017 device to form a new xarray.
> +
> +Finally, the cxl_region_update_bandwidth() is called and the aggregated
> +bandwidth from all the members of the last xarray is updated for the
> +access coordinates residing in the cxl region (cxlr) context.

Re: [PATCH v7 3/3] cxl: Add documentation to explain the shared link bandwidth calculation

[Dave Jiang]

On 8/27/24 9:06 AM, Jonathan Cameron wrote:
> On Wed, 10 Jul 2024 15:24:02 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Create a kernel documentation to describe how the CXL shared upstream
>> link bandwidth is calculated.
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Oops. Missed this previously.  A few minor things inline.
> 
> J
>> ---
>>  .../driver-api/cxl/access-coordinates.rst     | 90 +++++++++++++++++++
>>  Documentation/driver-api/cxl/index.rst        |  1 +
>>  MAINTAINERS                                   |  1 +
>>  3 files changed, 92 insertions(+)
>>  create mode 100644 Documentation/driver-api/cxl/access-coordinates.rst
>>
>> diff --git a/Documentation/driver-api/cxl/access-coordinates.rst b/Documentation/driver-api/cxl/access-coordinates.rst
>> new file mode 100644
>> index 000000000000..973e63872f06
>> --- /dev/null
>> +++ b/Documentation/driver-api/cxl/access-coordinates.rst
>> @@ -0,0 +1,90 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +.. include:: <isonum.txt>
>> +
>> +==================================
>> +CXL Access Coordinates Computation
>> +==================================
>> +
>> +Shared Upstream Link Calculation
>> +================================
>> +For certain CXL region construction with endpoints behind CXL switches (SW) or
>> +Root Ports (RP), there is the possibility of the total bandwdith for all
> spell check. bandwidth
>> +the endpoints behind a switch being more than the switch upstream link.
> 
> Could also be the Generic Port bit of the topology. Mind you I'm still failing
> to come up with text for the qemu GP Documentation that the reviewers can follow
> so maybe that's just too hard to describe here.  Could use weasel words.
> 
> "A similar situation can occur within the host, upstream of the root ports."
> 
> 
>> +The CXL driver performs an additional pass after all the targets have
>> +arrived for a region in order to recalculate the bandwidths with possible
>> +upstream link being a limiting factor in mind.
>> +
>> +The algorithm assumes the configuration is a symmetric topology as that
>> +maximizes performance. When asymmetric topology is detected, the calculation
>> +is aborted when such topology is detected. An asymmetric topology is detected
> 
> is detected is duplicated.

I don't follow here.

> 
>> +during topology walk where the number of RPs detected as a grandparent is not
>> +equal to the number of devices iterated in the same iteration loop.
> 
> Maybe make the point that asymmetric in terms of only properties of devices
> is not detected. It just uses the first one I think?

I also don't follow here.

I also wonder if I should use "unbalanced" topology rather than "unsymmetric"?

DJ

>> +
>> +There can be multiple switches under a RP. There can be multiple RPs under
>> +a CXL Host Bridge (HB). There can be multiple HBs under a CXL Fixed Memory
>> +Window Structure (CFMWS).
>> +
>> +An example hierarchy:
>> +
>> +                CFMWS 0
>> +                  |
>> +         _________|_________
>> +        |                   |
>> +    ACPI0017-0          ACPI0017-1
>> + GP0/HB0/ACPI0016-0   GP1/HB1/ACPI0016-1
>> +    |          |        |           |
>> +   RP0        RP1      RP2         RP3
>> +    |          |        |           |
>> +  SW 0       SW 1     SW 2        SW 3
>> +  |   |      |   |    |   |       |   |
>> + EP0 EP1    EP2 EP3  EP4  EP5    EP6 EP7
>> +
>> +Computation for the example hierarchy:
>> +
>> +Min (GP0 to CPU BW,
>> +     Min(SW 0 Upstream Link to RP0 BW,
>> +         Min(SW0SSLBIS for SW0DSP0 (EP0), EP0 DSLBIS, EP0 Upstream Link) +
>> +         Min(SW0SSLBIS for SW0DSP1 (EP1), EP1 DSLBIS, EP1 Upstream link)) +
>> +     Min(SW 1 Upstream Link to RP1 BW,
>> +         Min(SW1SSLBIS for SW1DSP0 (EP2), EP2 DSLBIS, EP2 Upstream Link) +
>> +         Min(SW1SSLBIS for SW1DSP1 (EP3), EP3 DSLBIS, EP3 Upstream link))) +
>> +Min (GP1 to CPU BW,
>> +     Min(SW 2 Upstream Link to RP2 BW,
>> +         Min(SW2SSLBIS for SW2DSP0 (EP4), EP4 DSLBIS, EP4 Upstream Link) +
>> +         Min(SW2SSLBIS for SW2DSP1 (EP5), EP5 DSLBIS, EP5 Upstream link)) +
>> +     Min(SW 3 Upstream Link to RP3 BW,
>> +         Min(SW3SSLBIS for SW3DSP0 (EP6), EP6 DSLBIS, EP6 Upstream Link) +
>> +         Min(SW3SSLBIS for SW3DSP1 (EP7), EP7 DSLBIS, EP7 Upstream link))))
>> +
>> +
>> +The calculation starts at cxl_region_shared_upstream_perf_update(). A xarray
>> +is created to collect all the endpoint bandwidths via the
>> +cxl_endpoint_gather_bandwidth() function. The min() of bandwidth from the
>> +endpoint CDAT and the upstream link bandwidth is calculated. If the endpoint
>> +has a CXL switch as a parent, then min() of calculated bandwidth and the
>> +bandwidth from the SSLBIS for the switch downstream port that is associated
>> +with the endpoint is calculated. The final bandwidth is stored in a
>> +'struct cxl_perf_ctx' in the xarray indexed by a device pointer. If the
>> +endpoint is direct attached to a root port (RP), the device pointer would be a
>> +RP device. If the endpoint is behind a switch, the device pointer would be the
> an RP 
>> +upstream device of the parent switch.
>> +
>> +At the next stage, the code attempts to walk through one or more switches if
> the code walks through
> (I doubt it ever fails and if it does that's detail you don't need here)
>> +they exist in the topology. For endpoints directly attached to RPs, this
>> +step is skipped. If there is another switch upstream, the code takes the min()
>> +of the current gathered bandwidth and the upstream link bandwidth. If there's
>> +a switch upstream, then the SSLBIS of the upstream switch.
>> +
>> +Once the topology walk reaches the RP, whether it's direct attached endpoints
>> +or walking through the switch(es), cxl_rp_gather_bandwidth() is called. At
>> +this point all the bandwidths are aggregated per each host bridge, which is
>> +also the index for the resulting xarray.
>> +
>> +The next step is to take the min() of the per host bridge bandwidth and the
>> +bandwidth from the Generic Port (GP). The bandwidths for the GP is retrieved
>> +via ACPI tables SRAT/HMAT. The min bandwidth are aggregated under the same
>> +ACPI0017 device to form a new xarray.
>> +
>> +Finally, the cxl_region_update_bandwidth() is called and the aggregated
>> +bandwidth from all the members of the last xarray is updated for the
>> +access coordinates residing in the cxl region (cxlr) context.
> 

Re: [PATCH v7 3/3] cxl: Add documentation to explain the shared link bandwidth calculation

[Jonathan Cameron]

On Tue, 27 Aug 2024 15:38:27 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 8/27/24 9:06 AM, Jonathan Cameron wrote:
> > On Wed, 10 Jul 2024 15:24:02 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> >   
> >> Create a kernel documentation to describe how the CXL shared upstream
> >> link bandwidth is calculated.
> >>
> >> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>  
> > Oops. Missed this previously.  A few minor things inline.
> > 
> > J  
> >> ---
> >>  .../driver-api/cxl/access-coordinates.rst     | 90 +++++++++++++++++++
> >>  Documentation/driver-api/cxl/index.rst        |  1 +
> >>  MAINTAINERS                                   |  1 +
> >>  3 files changed, 92 insertions(+)
> >>  create mode 100644 Documentation/driver-api/cxl/access-coordinates.rst
> >>
> >> diff --git a/Documentation/driver-api/cxl/access-coordinates.rst b/Documentation/driver-api/cxl/access-coordinates.rst
> >> new file mode 100644
> >> index 000000000000..973e63872f06
> >> --- /dev/null
> >> +++ b/Documentation/driver-api/cxl/access-coordinates.rst
> >> @@ -0,0 +1,90 @@
> >> +.. SPDX-License-Identifier: GPL-2.0
> >> +.. include:: <isonum.txt>
> >> +
> >> +==================================
> >> +CXL Access Coordinates Computation
> >> +==================================
> >> +
> >> +Shared Upstream Link Calculation
> >> +================================
> >> +For certain CXL region construction with endpoints behind CXL switches (SW) or
> >> +Root Ports (RP), there is the possibility of the total bandwdith for all  
> > spell check. bandwidth  
> >> +the endpoints behind a switch being more than the switch upstream link.  
> > 
> > Could also be the Generic Port bit of the topology. Mind you I'm still failing
> > to come up with text for the qemu GP Documentation that the reviewers can follow
> > so maybe that's just too hard to describe here.  Could use weasel words.
> > 
> > "A similar situation can occur within the host, upstream of the root ports."
> > 
> >   
> >> +The CXL driver performs an additional pass after all the targets have
> >> +arrived for a region in order to recalculate the bandwidths with possible
> >> +upstream link being a limiting factor in mind.
> >> +
> >> +The algorithm assumes the configuration is a symmetric topology as that
> >> +maximizes performance. When asymmetric topology is detected, the calculation
> >> +is aborted when such topology is detected. An asymmetric topology is detected  
> > 
> > is detected is duplicated.  
> 
> I don't follow here.
> 
>+ When asymmetric topology is detected, the calculation
                             ^^^^^^^^^^^
>+is aborted when such topology is detected.
                                ^^^^^^^^^^^^
Delete the second one.

> >   
> >> +during topology walk where the number of RPs detected as a grandparent is not
> >> +equal to the number of devices iterated in the same iteration loop.  
> > 
> > Maybe make the point that asymmetric in terms of only properties of devices
> > is not detected. It just uses the first one I think?  
> 
> I also don't follow here.
> 
> I also wonder if I should use "unbalanced" topology rather than "unsymmetric"?

Difference between detecting a topology with different number of links and one
where say one type 3 device in an interleave set has lower bandwidth in it's CDAT.

IIRC this does the first, but not the second.

> 
> DJ

Re: [PATCH v7 3/3] cxl: Add documentation to explain the shared link bandwidth calculation

[Dave Jiang]

On 8/28/24 2:00 AM, Jonathan Cameron wrote:
> On Tue, 27 Aug 2024 15:38:27 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> On 8/27/24 9:06 AM, Jonathan Cameron wrote:
>>> On Wed, 10 Jul 2024 15:24:02 -0700
>>> Dave Jiang <dave.jiang@intel.com> wrote:
>>>   
>>>> Create a kernel documentation to describe how the CXL shared upstream
>>>> link bandwidth is calculated.
>>>>
>>>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>  
>>> Oops. Missed this previously.  A few minor things inline.
>>>
>>> J  
>>>> ---
>>>>  .../driver-api/cxl/access-coordinates.rst     | 90 +++++++++++++++++++
>>>>  Documentation/driver-api/cxl/index.rst        |  1 +
>>>>  MAINTAINERS                                   |  1 +
>>>>  3 files changed, 92 insertions(+)
>>>>  create mode 100644 Documentation/driver-api/cxl/access-coordinates.rst
>>>>
>>>> diff --git a/Documentation/driver-api/cxl/access-coordinates.rst b/Documentation/driver-api/cxl/access-coordinates.rst
>>>> new file mode 100644
>>>> index 000000000000..973e63872f06
>>>> --- /dev/null
>>>> +++ b/Documentation/driver-api/cxl/access-coordinates.rst
>>>> @@ -0,0 +1,90 @@
>>>> +.. SPDX-License-Identifier: GPL-2.0
>>>> +.. include:: <isonum.txt>
>>>> +
>>>> +==================================
>>>> +CXL Access Coordinates Computation
>>>> +==================================
>>>> +
>>>> +Shared Upstream Link Calculation
>>>> +================================
>>>> +For certain CXL region construction with endpoints behind CXL switches (SW) or
>>>> +Root Ports (RP), there is the possibility of the total bandwdith for all  
>>> spell check. bandwidth  
>>>> +the endpoints behind a switch being more than the switch upstream link.  
>>>
>>> Could also be the Generic Port bit of the topology. Mind you I'm still failing
>>> to come up with text for the qemu GP Documentation that the reviewers can follow
>>> so maybe that's just too hard to describe here.  Could use weasel words.
>>>
>>> "A similar situation can occur within the host, upstream of the root ports."
>>>
>>>   
>>>> +The CXL driver performs an additional pass after all the targets have
>>>> +arrived for a region in order to recalculate the bandwidths with possible
>>>> +upstream link being a limiting factor in mind.
>>>> +
>>>> +The algorithm assumes the configuration is a symmetric topology as that
>>>> +maximizes performance. When asymmetric topology is detected, the calculation
>>>> +is aborted when such topology is detected. An asymmetric topology is detected  
>>>
>>> is detected is duplicated.  
>>
>> I don't follow here.
>>
>> + When asymmetric topology is detected, the calculation
>                              ^^^^^^^^^^^
>> +is aborted when such topology is detected.
>                                 ^^^^^^^^^^^^
> Delete the second one.

Thanks. The brain does funny tricks sometimes when reading the stuff you write yourself. 

> 
>>>   
>>>> +during topology walk where the number of RPs detected as a grandparent is not
>>>> +equal to the number of devices iterated in the same iteration loop.  
>>>
>>> Maybe make the point that asymmetric in terms of only properties of devices
>>> is not detected. It just uses the first one I think?  
>>
>> I also don't follow here.
>>
>> I also wonder if I should use "unbalanced" topology rather than "unsymmetric"?
> 
> Difference between detecting a topology with different number of links and one
> where say one type 3 device in an interleave set has lower bandwidth in it's CDAT.
> 
> IIRC this does the first, but not the second.

I think here we attempt to detect whether one root port has different level of switch(s) vs another. For example a topology configuration like RP0 is direct attached and RP1 has a switch underneath. 

> 
>>
>> DJ
> 

Re: [PATCH v7 3/3] cxl: Add documentation to explain the shared link bandwidth calculation

[Jonathan Cameron]

On Wed, 28 Aug 2024 08:35:07 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 8/28/24 2:00 AM, Jonathan Cameron wrote:
> > On Tue, 27 Aug 2024 15:38:27 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> >   
> >> On 8/27/24 9:06 AM, Jonathan Cameron wrote:  
> >>> On Wed, 10 Jul 2024 15:24:02 -0700
> >>> Dave Jiang <dave.jiang@intel.com> wrote:
> >>>     
> >>>> Create a kernel documentation to describe how the CXL shared upstream
> >>>> link bandwidth is calculated.
> >>>>
> >>>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> >>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>    
> >>> Oops. Missed this previously.  A few minor things inline.
> >>>
> >>> J    
> >>>> ---
> >>>>  .../driver-api/cxl/access-coordinates.rst     | 90 +++++++++++++++++++
> >>>>  Documentation/driver-api/cxl/index.rst        |  1 +
> >>>>  MAINTAINERS                                   |  1 +
> >>>>  3 files changed, 92 insertions(+)
> >>>>  create mode 100644 Documentation/driver-api/cxl/access-coordinates.rst
> >>>>
> >>>> diff --git a/Documentation/driver-api/cxl/access-coordinates.rst b/Documentation/driver-api/cxl/access-coordinates.rst
> >>>> new file mode 100644
> >>>> index 000000000000..973e63872f06
> >>>> --- /dev/null
> >>>> +++ b/Documentation/driver-api/cxl/access-coordinates.rst
> >>>> @@ -0,0 +1,90 @@
> >>>> +.. SPDX-License-Identifier: GPL-2.0
> >>>> +.. include:: <isonum.txt>
> >>>> +
> >>>> +==================================
> >>>> +CXL Access Coordinates Computation
> >>>> +==================================
> >>>> +
> >>>> +Shared Upstream Link Calculation
> >>>> +================================
> >>>> +For certain CXL region construction with endpoints behind CXL switches (SW) or
> >>>> +Root Ports (RP), there is the possibility of the total bandwdith for all    
> >>> spell check. bandwidth    
> >>>> +the endpoints behind a switch being more than the switch upstream link.    
> >>>
> >>> Could also be the Generic Port bit of the topology. Mind you I'm still failing
> >>> to come up with text for the qemu GP Documentation that the reviewers can follow
> >>> so maybe that's just too hard to describe here.  Could use weasel words.
> >>>
> >>> "A similar situation can occur within the host, upstream of the root ports."
> >>>
> >>>     
> >>>> +The CXL driver performs an additional pass after all the targets have
> >>>> +arrived for a region in order to recalculate the bandwidths with possible
> >>>> +upstream link being a limiting factor in mind.
> >>>> +
> >>>> +The algorithm assumes the configuration is a symmetric topology as that
> >>>> +maximizes performance. When asymmetric topology is detected, the calculation
> >>>> +is aborted when such topology is detected. An asymmetric topology is detected    
> >>>
> >>> is detected is duplicated.    
> >>
> >> I don't follow here.
> >>
> >> + When asymmetric topology is detected, the calculation  
> >                              ^^^^^^^^^^^  
> >> +is aborted when such topology is detected.  
> >                                 ^^^^^^^^^^^^
> > Delete the second one.  
> 
> Thanks. The brain does funny tricks sometimes when reading the stuff you write yourself. 
> 
> >   
> >>>     
> >>>> +during topology walk where the number of RPs detected as a grandparent is not
> >>>> +equal to the number of devices iterated in the same iteration loop.    
> >>>
> >>> Maybe make the point that asymmetric in terms of only properties of devices
> >>> is not detected. It just uses the first one I think?    
> >>
> >> I also don't follow here.
> >>
> >> I also wonder if I should use "unbalanced" topology rather than "unsymmetric"?  
> > 
> > Difference between detecting a topology with different number of links and one
> > where say one type 3 device in an interleave set has lower bandwidth in it's CDAT.
> > 
> > IIRC this does the first, but not the second.  
> 
> I think here we attempt to detect whether one root port has different level of switch(s) vs another. For example a topology configuration like RP0 is direct attached and RP1 has a switch underneath. 

I'd just add a line to say that we assume that
subtle asymmetry in properties doesn't happen, so compute
bandwidth on assumption all paths to EPs are equivalent.

Jonathan

> 
> >   
> >>
> >> DJ  
> >   
> 

Re: [PATCH v7 2/3] cxl: Calculate region bandwidth of targets with shared upstream link

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240710222716.797267-3-dave.jiang@intel.com>
References: <20240710222716.797267-3-dave.jiang@intel.com>
TO: Dave Jiang <dave.jiang@intel.com>

Hi Dave,

kernel test robot noticed the following build warnings:

[auto build test WARNING on v6.10-rc6]
[cannot apply to cxl/next linus/master cxl/pending v6.10-rc7 next-20240711]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dave-Jiang/cxl-Preserve-the-CDAT-access_coordinate-for-an-endpoint/20240711-092210
base:   v6.10-rc6
patch link:    https://lore.kernel.org/r/20240710222716.797267-3-dave.jiang%40intel.com
patch subject: [PATCH v7 2/3] cxl: Calculate region bandwidth of targets with shared upstream link
:::::: branch date: 11 hours ago
:::::: commit date: 11 hours ago
config: i386-randconfig-141-20240711 (https://download.01.org/0day-ci/archive/20240711/202407111927.7Q2PF9i0-lkp@intel.com/config)
compiler: gcc-10 (Ubuntu 10.5.0-1ubuntu1) 10.5.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202407111927.7Q2PF9i0-lkp@intel.com/

smatch warnings:
drivers/cxl/core/cdat.c:785 cxl_switch_gather_bandwidth() error: uninitialized symbol 'is_root'.

vim +/is_root +785 drivers/cxl/core/cdat.c

dfbcea34995e70 Dave Jiang 2024-07-10  732  
dfbcea34995e70 Dave Jiang 2024-07-10  733  /*
dfbcea34995e70 Dave Jiang 2024-07-10  734   * cxl_switch_gather_bandwidth - collect all the bandwidth at switch level in an xarray
dfbcea34995e70 Dave Jiang 2024-07-10  735   * @cxlr: The region being operated on
dfbcea34995e70 Dave Jiang 2024-07-10  736   * @input_xa: xarray indexed by upstream device of a switch with data of 'struct
dfbcea34995e70 Dave Jiang 2024-07-10  737   *	      cxl_perf_ctx'
dfbcea34995e70 Dave Jiang 2024-07-10  738   * @gp_is_root: (output) bool of whether the grandparent is cxl root.
dfbcea34995e70 Dave Jiang 2024-07-10  739   *
dfbcea34995e70 Dave Jiang 2024-07-10  740   * Return: a xarray of resulting cxl_perf_ctx per parent switch or root port
dfbcea34995e70 Dave Jiang 2024-07-10  741   *         or ERR_PTR(-errno)
dfbcea34995e70 Dave Jiang 2024-07-10  742   *
dfbcea34995e70 Dave Jiang 2024-07-10  743   * Iterate through the xarray. Take the minimum of the downstream calculated
dfbcea34995e70 Dave Jiang 2024-07-10  744   * bandwidth, the upstream link bandwidth, and the SSLBIS of the upstream
dfbcea34995e70 Dave Jiang 2024-07-10  745   * switch if exists. Sum the resulting bandwidth under the switch upstream
dfbcea34995e70 Dave Jiang 2024-07-10  746   * device or a RP device. The function can be iterated over multiple switches
dfbcea34995e70 Dave Jiang 2024-07-10  747   * if the switches are present.
dfbcea34995e70 Dave Jiang 2024-07-10  748   */
dfbcea34995e70 Dave Jiang 2024-07-10  749  static struct xarray *cxl_switch_gather_bandwidth(struct cxl_region *cxlr,
dfbcea34995e70 Dave Jiang 2024-07-10  750  						  struct xarray *input_xa,
dfbcea34995e70 Dave Jiang 2024-07-10  751  						  bool *gp_is_root)
dfbcea34995e70 Dave Jiang 2024-07-10  752  {
dfbcea34995e70 Dave Jiang 2024-07-10  753  	struct xarray *res_xa __free(free_perf_xa) =
dfbcea34995e70 Dave Jiang 2024-07-10  754  		kzalloc(sizeof(*res_xa), GFP_KERNEL);
dfbcea34995e70 Dave Jiang 2024-07-10  755  	struct access_coordinate coords[ACCESS_COORDINATE_MAX];
dfbcea34995e70 Dave Jiang 2024-07-10  756  	struct cxl_perf_ctx *ctx, *us_ctx;
dfbcea34995e70 Dave Jiang 2024-07-10  757  	unsigned long index, us_index;
dfbcea34995e70 Dave Jiang 2024-07-10  758  	int dev_count = 0;
dfbcea34995e70 Dave Jiang 2024-07-10  759  	int gp_count = 0;
dfbcea34995e70 Dave Jiang 2024-07-10  760  	void *ptr;
dfbcea34995e70 Dave Jiang 2024-07-10  761  	int rc;
dfbcea34995e70 Dave Jiang 2024-07-10  762  
dfbcea34995e70 Dave Jiang 2024-07-10  763  	if (!res_xa)
dfbcea34995e70 Dave Jiang 2024-07-10  764  		return ERR_PTR(-ENOMEM);
dfbcea34995e70 Dave Jiang 2024-07-10  765  	xa_init(res_xa);
dfbcea34995e70 Dave Jiang 2024-07-10  766  
dfbcea34995e70 Dave Jiang 2024-07-10  767  	xa_for_each(input_xa, index, ctx) {
dfbcea34995e70 Dave Jiang 2024-07-10  768  		struct device *dev = (struct device *)index;
dfbcea34995e70 Dave Jiang 2024-07-10  769  		struct cxl_port *port = ctx->port;
dfbcea34995e70 Dave Jiang 2024-07-10  770  		struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
dfbcea34995e70 Dave Jiang 2024-07-10  771  		struct cxl_port *gp_port = to_cxl_port(parent_port->dev.parent);
dfbcea34995e70 Dave Jiang 2024-07-10  772  		struct cxl_dport *dport = port->parent_dport;
dfbcea34995e70 Dave Jiang 2024-07-10  773  		bool is_root;
dfbcea34995e70 Dave Jiang 2024-07-10  774  
dfbcea34995e70 Dave Jiang 2024-07-10  775  		dev_count++;
dfbcea34995e70 Dave Jiang 2024-07-10  776  		if (is_cxl_root(gp_port)) {
dfbcea34995e70 Dave Jiang 2024-07-10  777  			is_root = true;
dfbcea34995e70 Dave Jiang 2024-07-10  778  			gp_count++;
dfbcea34995e70 Dave Jiang 2024-07-10  779  		}
dfbcea34995e70 Dave Jiang 2024-07-10  780  
dfbcea34995e70 Dave Jiang 2024-07-10  781  		/*
dfbcea34995e70 Dave Jiang 2024-07-10  782  		 * If the grandparent is cxl root, then index is the root port,
dfbcea34995e70 Dave Jiang 2024-07-10  783  		 * otherwise it's the parent switch upstream device.
dfbcea34995e70 Dave Jiang 2024-07-10  784  		 */
dfbcea34995e70 Dave Jiang 2024-07-10 @785  		if (is_root)
dfbcea34995e70 Dave Jiang 2024-07-10  786  			us_index = (unsigned long)port->parent_dport->dport_dev;
dfbcea34995e70 Dave Jiang 2024-07-10  787  		else
dfbcea34995e70 Dave Jiang 2024-07-10  788  			us_index = (unsigned long)parent_port->uport_dev;
dfbcea34995e70 Dave Jiang 2024-07-10  789  
dfbcea34995e70 Dave Jiang 2024-07-10  790  		us_ctx = xa_load(res_xa, us_index);
dfbcea34995e70 Dave Jiang 2024-07-10  791  		if (!us_ctx) {
dfbcea34995e70 Dave Jiang 2024-07-10  792  			struct cxl_perf_ctx *n __free(kfree) =
dfbcea34995e70 Dave Jiang 2024-07-10  793  				kzalloc(sizeof(*n), GFP_KERNEL);
dfbcea34995e70 Dave Jiang 2024-07-10  794  
dfbcea34995e70 Dave Jiang 2024-07-10  795  			if (!n)
dfbcea34995e70 Dave Jiang 2024-07-10  796  				return ERR_PTR(-ENOMEM);
dfbcea34995e70 Dave Jiang 2024-07-10  797  
dfbcea34995e70 Dave Jiang 2024-07-10  798  			ptr = xa_store(res_xa, us_index, n, GFP_KERNEL);
dfbcea34995e70 Dave Jiang 2024-07-10  799  			if (xa_is_err(ptr))
dfbcea34995e70 Dave Jiang 2024-07-10  800  				return ERR_PTR(xa_err(ptr));
dfbcea34995e70 Dave Jiang 2024-07-10  801  			us_ctx = no_free_ptr(n);
dfbcea34995e70 Dave Jiang 2024-07-10  802  			us_ctx->port = parent_port;
dfbcea34995e70 Dave Jiang 2024-07-10  803  		}
dfbcea34995e70 Dave Jiang 2024-07-10  804  
dfbcea34995e70 Dave Jiang 2024-07-10  805  		/*
dfbcea34995e70 Dave Jiang 2024-07-10  806  		 * If the device isn't an upstream PCIe port, there's something
dfbcea34995e70 Dave Jiang 2024-07-10  807  		 * wrong with the topology.
dfbcea34995e70 Dave Jiang 2024-07-10  808  		 */
dfbcea34995e70 Dave Jiang 2024-07-10  809  		if (!dev_is_pci(dev))
dfbcea34995e70 Dave Jiang 2024-07-10  810  			return ERR_PTR(-EINVAL);
dfbcea34995e70 Dave Jiang 2024-07-10  811  
dfbcea34995e70 Dave Jiang 2024-07-10  812  		/* Retrieve the upstream link bandwidth */
dfbcea34995e70 Dave Jiang 2024-07-10  813  		rc = cxl_pci_get_bandwidth(to_pci_dev(dev), coords);
dfbcea34995e70 Dave Jiang 2024-07-10  814  		if (rc)
dfbcea34995e70 Dave Jiang 2024-07-10  815  			return ERR_PTR(-ENXIO);
dfbcea34995e70 Dave Jiang 2024-07-10  816  
dfbcea34995e70 Dave Jiang 2024-07-10  817  		/*
dfbcea34995e70 Dave Jiang 2024-07-10  818  		 * Take the min of downstream bandwidth and the upstream link
dfbcea34995e70 Dave Jiang 2024-07-10  819  		 * bandwidth.
dfbcea34995e70 Dave Jiang 2024-07-10  820  		 */
dfbcea34995e70 Dave Jiang 2024-07-10  821  		cxl_coordinates_combine(coords, coords, ctx->coord);
dfbcea34995e70 Dave Jiang 2024-07-10  822  
dfbcea34995e70 Dave Jiang 2024-07-10  823  		/*
dfbcea34995e70 Dave Jiang 2024-07-10  824  		 * Take the min of the calculated bandwdith and the upstream
dfbcea34995e70 Dave Jiang 2024-07-10  825  		 * switch SSLBIS bandwidth if there's a parent switch
dfbcea34995e70 Dave Jiang 2024-07-10  826  		 */
dfbcea34995e70 Dave Jiang 2024-07-10  827  		if (!is_root)
dfbcea34995e70 Dave Jiang 2024-07-10  828  			cxl_coordinates_combine(coords, coords, dport->coord);
dfbcea34995e70 Dave Jiang 2024-07-10  829  
dfbcea34995e70 Dave Jiang 2024-07-10  830  		/*
dfbcea34995e70 Dave Jiang 2024-07-10  831  		 * Aggregate the calculated bandwidth common to an upstream
dfbcea34995e70 Dave Jiang 2024-07-10  832  		 * switch.
dfbcea34995e70 Dave Jiang 2024-07-10  833  		 */
dfbcea34995e70 Dave Jiang 2024-07-10  834  		cxl_bandwidth_add(us_ctx->coord, us_ctx->coord, coords);
dfbcea34995e70 Dave Jiang 2024-07-10  835  	}
dfbcea34995e70 Dave Jiang 2024-07-10  836  
dfbcea34995e70 Dave Jiang 2024-07-10  837  	/* Asymmetric topology detected. */
dfbcea34995e70 Dave Jiang 2024-07-10  838  	if (gp_count) {
dfbcea34995e70 Dave Jiang 2024-07-10  839  		if (gp_count != dev_count) {
dfbcea34995e70 Dave Jiang 2024-07-10  840  			dev_dbg(&cxlr->dev,
dfbcea34995e70 Dave Jiang 2024-07-10  841  				"Asymmetric hierachy detected, bandwidth not updated\n");
dfbcea34995e70 Dave Jiang 2024-07-10  842  			return ERR_PTR(-EOPNOTSUPP);
dfbcea34995e70 Dave Jiang 2024-07-10  843  		}
dfbcea34995e70 Dave Jiang 2024-07-10  844  		*gp_is_root = true;
dfbcea34995e70 Dave Jiang 2024-07-10  845  	}
dfbcea34995e70 Dave Jiang 2024-07-10  846  
dfbcea34995e70 Dave Jiang 2024-07-10  847  	return no_free_ptr(res_xa);
dfbcea34995e70 Dave Jiang 2024-07-10  848  }
dfbcea34995e70 Dave Jiang 2024-07-10  849  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki