[PATCH v2 00/12] cxl: Add support for Restricted CXL hosts (RCD mode)

[PATCH v2 00/12] cxl: Add support for Restricted CXL hosts (RCD mode)

[Robert Richter]

In Restricted CXL Device (RCD) mode (formerly referred to as CXL 1.1)
the PCIe enumeration hierarchy is different from CXL VH Enumeration
(formerly referred to as 2.0, for both modes see CXL spec 3.0: 9.11
and 9.12, [1]). This series adds support for RCD mode. It implements
the detection of Restricted CXL Hosts (RCHs) and its corresponding
Restricted CXL Devices (RCDs). It does the necessary enumeration of
ports and connects the endpoints. With all the plumbing an RCH/RCD
pair is registered at the Linux CXL bus and becomes visible in sysfs
in the same way as CXL VH hosts and devices do already. RCDs are
brought up as CXL endpoints and bound to subsequent drivers such as
cxl_mem.

For CXL VH the host driver (cxl_acpi) starts host bridge discovery
once the ACPI0017 CXL root device is detected and then searches for
ACPI0016 host bridges to enable CXL. This implementation requires that
in RCD mode an ACPI0017 device exists, the host bridge must have the
ACPI0016 ID set. This implementation also expects the host's
downstream and upstream port RCRBs base address being reported by
firmware using the optional CEDT CHBS entry of the host bridge (see
CXL spec 3.0, 9.17.1.2).

RCD mode does not support hot-plug, so host discovery is at boot time
only.

Patches #1 to #5 are prerequisites of the series with fixes needed and
a rework of debug messages for port enumeration. Those are general
patches and could be applied earlier and independently from the rest
assuming there are no objections with them. Patches #6 to #12 contain
the actual implementation of RCD mode support.

[1] https://www.computeexpresslink.org/spec-landing

---

v2:
 * Reworked series to use add_host_bridge_dport() and
   add_host_bridge_uport(). There is a single cxl root device
   (ACPI0017) also for RCHs (must have a ACPI0016 id). (Dan)
 * Rebased onto 6.1-rc1.
 * Added a WARN_ON_ONCE() to CXL_RESOURCE_NONE check. Updated patch
   description with an example case. (Dan, Jonathan)
 * Added wrapper functions to devm_cxl_add_port() and
   devm_cxl_add_dport(). (Dan)
 * Dropped "PCI/ACPI: Link host bridge to its ACPI fw node" patch.
 * Updated spec refs to use 3.0. Added PCIe base spec refs. (Jonathan)
 * Reused UID detect code. (Dan)
 * Dropped "cxl/acpi: Specify module load order dependency for the
   cxl_acpi module" patch. Return -EINVAL if host not yet ready. (Dan)
 * Minor other changes.
 * Note: I haven't included most of the received Reviewed-by tags due
   to the major rework. In any case, thanks to all here.

Robert Richter (12):
  cxl/core: Remove duplicate declaration of devm_cxl_iomap_block()
  cxl/core: Check physical address before mapping it in
    devm_cxl_iomap_block()
  cxl: Unify debug messages when calling devm_cxl_add_port()
  cxl: Unify debug messages when calling devm_cxl_add_dport()
  cxl/acpi: Improve debug messages in cxl_acpi_probe()
  cxl/acpi: Extract component registers of restricted hosts from RCRB
  cxl: Remove dev_is_cxl_root_child() check in
    devm_cxl_enumerate_ports()
  cxl: Factor out code in devm_cxl_enumerate_ports() to
    find_port_attach_ep()
  cxl: Extend devm_cxl_enumerate_ports() to support restricted devices
    (RCDs)
  cxl: Do not ignore PCI config read errors in match_add_dports()
  cxl: Factor out code in match_add_dports() to pci_dev_add_dport()
  cxl: Extend devm_cxl_port_enumerate_dports() to support restricted
    hosts (RCH)

 drivers/cxl/acpi.c           |  98 ++++++++++++----
 drivers/cxl/core/pci.c       |  76 +++++++++---
 drivers/cxl/core/port.c      | 219 +++++++++++++++++++++++++----------
 drivers/cxl/core/regs.c      |   5 +
 drivers/cxl/cxl.h            |   2 -
 tools/testing/cxl/test/cxl.c |   8 +-
 6 files changed, 305 insertions(+), 103 deletions(-)


base-commit: 9abf2313adc1ca1b6180c508c25f22f9395cc780
-- 
2.30.2

[PATCH v2 01/12] cxl/core: Remove duplicate declaration of devm_cxl_iomap_block()

[Robert Richter]

The function devm_cxl_iomap_block() is only used in the core
code. There are two declarations in header files of it, in
drivers/cxl/core/core.h and drivers/cxl/cxl.h. Remove its unused
declaration in drivers/cxl/cxl.h.

Fixing build error in regs.c found by kernel test robot by including
"core.h" there.

Signed-off-by: Robert Richter <rrichter@amd.com>
Reported-by: kernel test robot <lkp@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/regs.c | 2 ++
 drivers/cxl/cxl.h       | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index 39a129c57d40..6522931df3f7 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -7,6 +7,8 @@
 #include <cxlmem.h>
 #include <cxlpci.h>
 
+#include "core.h"
+
 /**
  * DOC: cxl registers
  *
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index f680450f0b16..ac8998b627b5 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -218,8 +218,6 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 enum cxl_regloc_type;
 int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
 		      struct cxl_register_map *map);
-void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
-				   resource_size_t length);
 
 #define CXL_RESOURCE_NONE ((resource_size_t) -1)
 #define CXL_TARGET_STRLEN 20
-- 
2.30.2

[PATCH v2 02/12] cxl/core: Check physical address before mapping it in devm_cxl_iomap_block()

[Robert Richter]

The physical base address of a CXL range can be invalid and is then
set to CXL_RESOURCE_NONE. In general software shall prevent such
situations, but it is hard to proof this may never happen. E.g. in
add_port_attach_ep() there this the following:

      component_reg_phys = find_component_registers(uport_dev);
      port = devm_cxl_add_port(&parent_port->dev, uport_dev,
              component_reg_phys, parent_dport);

find_component_registers() and subsequent functions (e.g.
cxl_regmap_to_base()) may return CXL_RESOURCE_NONE. But it is written
to port without any further check in cxl_port_alloc():

      port->component_reg_phys = component_reg_phys;

It is then later directly used in devm_cxl_setup_hdm() to map io
ranges with devm_cxl_iomap_block(). Just an example...

Check this condition. Also do not fail silently like an ioremap()
failure, use a WARN_ON_ONCE() for it.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/regs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index 6522931df3f7..ec178e69b18f 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -167,6 +167,9 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
 	void __iomem *ret_val;
 	struct resource *res;
 
+	if (WARN_ON_ONCE(addr == CXL_RESOURCE_NONE))
+		return NULL;
+
 	res = devm_request_mem_region(dev, addr, length, dev_name(dev));
 	if (!res) {
 		resource_size_t end = addr + length - 1;
-- 
2.30.2

[PATCH v2 03/12] cxl: Unify debug messages when calling devm_cxl_add_port()

[Robert Richter]

CXL ports are added in a couple of code paths using devm_cxl_add_
port(). Debug messages are individually generated, but are incomplete
and inconsistent. Change this by moving its generation to devm_cxl_
add_port(). This unifies the messages and reduces code duplication.
Also, generate messages on failure. Use a __devm_cxl_add_port()
wrapper to keep the readability of the error exits.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/acpi.c      |  2 --
 drivers/cxl/core/port.c | 51 +++++++++++++++++++++++++++++++----------
 2 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index fb649683dd3a..767a91f44221 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -220,7 +220,6 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 	port = devm_cxl_add_port(host, match, dport->component_reg_phys, dport);
 	if (IS_ERR(port))
 		return PTR_ERR(port);
-	dev_dbg(host, "%s: add: %s\n", dev_name(match), dev_name(&port->dev));
 
 	return 0;
 }
@@ -466,7 +465,6 @@ static int cxl_acpi_probe(struct platform_device *pdev)
 	root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
 	if (IS_ERR(root_port))
 		return PTR_ERR(root_port);
-	dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
 
 	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
 			      add_host_bridge_dport);
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index bffde862de0b..9bfd01b4e5b5 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -655,16 +655,10 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
 	return ERR_PTR(rc);
 }
 
-/**
- * devm_cxl_add_port - register a cxl_port in CXL memory decode hierarchy
- * @host: host device for devm operations
- * @uport: "physical" device implementing this upstream port
- * @component_reg_phys: (optional) for configurable cxl_port instances
- * @parent_dport: next hop up in the CXL memory decode hierarchy
- */
-struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
-				   resource_size_t component_reg_phys,
-				   struct cxl_dport *parent_dport)
+static struct cxl_port *__devm_cxl_add_port(struct device *host,
+					    struct device *uport,
+					    resource_size_t component_reg_phys,
+					    struct cxl_dport *parent_dport)
 {
 	struct cxl_port *port;
 	struct device *dev;
@@ -702,6 +696,41 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
 	put_device(dev);
 	return ERR_PTR(rc);
 }
+
+/**
+ * devm_cxl_add_port - register a cxl_port in CXL memory decode hierarchy
+ * @host: host device for devm operations
+ * @uport: "physical" device implementing this upstream port
+ * @component_reg_phys: (optional) for configurable cxl_port instances
+ * @parent_dport: next hop up in the CXL memory decode hierarchy
+ */
+struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
+				   resource_size_t component_reg_phys,
+				   struct cxl_dport *parent_dport)
+{
+	struct cxl_port *port, *parent_port;
+
+	port = __devm_cxl_add_port(host, uport, component_reg_phys,
+				   parent_dport);
+
+	parent_port = parent_dport ? parent_dport->port : NULL;
+	if (IS_ERR(port)) {
+		dev_dbg(uport, "Failed to add %s%s%s%s: %ld\n",
+			dev_name(&port->dev),
+			parent_port ? " to " : "",
+			parent_port ? dev_name(&parent_port->dev) : "",
+			parent_port ? "" : " (root port)",
+			PTR_ERR(port));
+	} else {
+		dev_dbg(uport, "%s added%s%s%s\n",
+			dev_name(&port->dev),
+			parent_port ? " to " : "",
+			parent_port ? dev_name(&parent_port->dev) : "",
+			parent_port ? "" : " (root port)");
+	}
+
+	return port;
+}
 EXPORT_SYMBOL_NS_GPL(devm_cxl_add_port, CXL);
 
 struct pci_bus *cxl_port_to_pci_bus(struct cxl_port *port)
@@ -1140,8 +1169,6 @@ int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd,
 	if (IS_ERR(endpoint))
 		return PTR_ERR(endpoint);
 
-	dev_dbg(&cxlmd->dev, "add: %s\n", dev_name(&endpoint->dev));
-
 	rc = cxl_endpoint_autoremove(cxlmd, endpoint);
 	if (rc)
 		return rc;
-- 
2.30.2

[PATCH v2 04/12] cxl: Unify debug messages when calling devm_cxl_add_dport()

[Robert Richter]

CXL dports are added in a couple of code paths using devm_cxl_add_
dport(). Debug messages are individually generated, but are incomplete
and inconsistent. Change this by moving its generation to devm_cxl_
add_dport(). This unifies the messages and reduces code duplication.
Also, generate messages on failure. Use a __devm_cxl_add_dport()
wrapper to keep the readability of the error exits.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/acpi.c           |  7 ++----
 drivers/cxl/core/pci.c       |  2 --
 drivers/cxl/core/port.c      | 48 +++++++++++++++++++++++++-----------
 tools/testing/cxl/test/cxl.c |  8 +-----
 4 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 767a91f44221..31e104f0210f 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -282,12 +282,9 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 	}
 
 	dport = devm_cxl_add_dport(root_port, match, uid, ctx.chbcr);
-	if (IS_ERR(dport)) {
-		dev_err(host, "failed to add downstream port: %s\n",
-			dev_name(match));
+	if (IS_ERR(dport))
 		return PTR_ERR(dport);
-	}
-	dev_dbg(host, "add dport%llu: %s\n", uid, dev_name(match));
+
 	return 0;
 }
 
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 9240df53ed87..0dbbe8d39b07 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -62,8 +62,6 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
 	}
 	ctx->count++;
 
-	dev_dbg(&port->dev, "add dport%d: %s\n", port_num, dev_name(&pdev->dev));
-
 	return 0;
 }
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 9bfd01b4e5b5..0431ed860d8e 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -892,20 +892,10 @@ static void cxl_dport_unlink(void *data)
 	sysfs_remove_link(&port->dev.kobj, link_name);
 }
 
-/**
- * devm_cxl_add_dport - append downstream port data to a cxl_port
- * @port: the cxl_port that references this dport
- * @dport_dev: firmware or PCI device representing the dport
- * @port_id: identifier for this dport in a decoder's target list
- * @component_reg_phys: optional location of CXL component registers
- *
- * Note that dports are appended to the devm release action's of the
- * either the port's host (for root ports), or the port itself (for
- * switch ports)
- */
-struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
-				     struct device *dport_dev, int port_id,
-				     resource_size_t component_reg_phys)
+static struct cxl_dport *__devm_cxl_add_dport(struct cxl_port *port,
+					      struct device *dport_dev,
+					      int port_id,
+					      resource_size_t component_reg_phys)
 {
 	char link_name[CXL_TARGET_STRLEN];
 	struct cxl_dport *dport;
@@ -957,6 +947,36 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
 
 	return dport;
 }
+
+/**
+ * devm_cxl_add_dport - append downstream port data to a cxl_port
+ * @port: the cxl_port that references this dport
+ * @dport_dev: firmware or PCI device representing the dport
+ * @port_id: identifier for this dport in a decoder's target list
+ * @component_reg_phys: optional location of CXL component registers
+ *
+ * Note that dports are appended to the devm release action's of the
+ * either the port's host (for root ports), or the port itself (for
+ * switch ports)
+ */
+struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
+				     struct device *dport_dev, int port_id,
+				     resource_size_t component_reg_phys)
+{
+	struct cxl_dport *dport;
+
+	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
+				     component_reg_phys);
+	if (IS_ERR(dport)) {
+		dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
+			dev_name(&port->dev), PTR_ERR(dport));
+	} else {
+		dev_dbg(dport_dev, "dport added to %s\n",
+			dev_name(&port->dev));
+	}
+
+	return dport;
+}
 EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, CXL);
 
 static int add_ep(struct cxl_ep *new)
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index a072b2d3e726..c610625e8261 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -582,14 +582,8 @@ static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
 		dport = devm_cxl_add_dport(port, &pdev->dev, pdev->id,
 					   CXL_RESOURCE_NONE);
 
-		if (IS_ERR(dport)) {
-			dev_err(dev, "failed to add dport: %s (%ld)\n",
-				dev_name(&pdev->dev), PTR_ERR(dport));
+		if (IS_ERR(dport))
 			return PTR_ERR(dport);
-		}
-
-		dev_dbg(dev, "add dport%d: %s\n", pdev->id,
-			dev_name(&pdev->dev));
 	}
 
 	return 0;
-- 
2.30.2

[PATCH v2 05/12] cxl/acpi: Improve debug messages in cxl_acpi_probe()

[Robert Richter]

In cxl_acpi_probe() the iterator bus_for_each_dev() walks through all
CXL hosts. Since all dev_*() debug messages point to the ACPI0017
device which is the CXL root for all hosts, the device information is
pointless as it is always the same device. Change this to use the host
device for this instead.

Also, add additional host specific information such as CXL support,
UID and CHBCR.

This is an example log:

 acpi ACPI0016:00: UID found: 4
 acpi ACPI0016:00: CHBCR found: 0x28090000000
 acpi ACPI0016:00: dport added to root0
 acpi ACPI0016:00: host-bridge: ACPI0016:00
  pci0000:7f: host supports CXL

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/acpi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 31e104f0210f..fb9f72813067 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -221,6 +221,8 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 	if (IS_ERR(port))
 		return PTR_ERR(port);
 
+	dev_info(pci_root->bus->bridge, "host supports CXL\n");
+
 	return 0;
 }
 
@@ -264,11 +266,12 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 	status = acpi_evaluate_integer(bridge->handle, METHOD_NAME__UID, NULL,
 				       &uid);
 	if (status != AE_OK) {
-		dev_err(host, "unable to retrieve _UID of %s\n",
-			dev_name(match));
+		dev_err(match, "unable to retrieve _UID\n");
 		return -ENODEV;
 	}
 
+	dev_dbg(match, "UID found: %lld\n", uid);
+
 	ctx = (struct cxl_chbs_context) {
 		.dev = host,
 		.uid = uid,
@@ -276,11 +279,12 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbcr, &ctx);
 
 	if (ctx.chbcr == 0) {
-		dev_warn(host, "No CHBS found for Host Bridge: %s\n",
-			 dev_name(match));
+		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
 		return 0;
 	}
 
+	dev_dbg(match, "CHBCR found: 0x%08llx\n", (u64)ctx.chbcr);
+
 	dport = devm_cxl_add_dport(root_port, match, uid, ctx.chbcr);
 	if (IS_ERR(dport))
 		return PTR_ERR(dport);
-- 
2.30.2

[PATCH v2 06/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Robert Richter]

A downstream port must be connected to a component register block.
For restricted hosts the base address is determined from the RCRB. The
RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
get the RCRB and add code to extract the component register block from
it.

RCRB's BAR[0..1] point to the component block containing CXL subsystem
component registers. MEMBAR extraction follows the PCI base spec here,
esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1).

Note: Right now the component register block is used for HDM decoder
capability only which is optional for RCDs. If unsupported by the RCD,
the HDM init will fail. It is future work to bypass it in this case.

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/acpi.c | 79 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 69 insertions(+), 10 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index fb9f72813067..a92d5d7b7a92 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -9,6 +9,8 @@
 #include "cxlpci.h"
 #include "cxl.h"
 
+#define CXL_RCRB_SIZE	SZ_8K
+
 static unsigned long cfmws_to_decoder_flags(int restrictions)
 {
 	unsigned long flags = CXL_DECODER_F_ENABLE;
@@ -229,27 +231,82 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 struct cxl_chbs_context {
 	struct device *dev;
 	unsigned long long uid;
-	resource_size_t chbcr;
+	struct acpi_cedt_chbs chbs;
 };
 
-static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
-			 const unsigned long end)
+static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
+			const unsigned long end)
 {
 	struct cxl_chbs_context *ctx = arg;
 	struct acpi_cedt_chbs *chbs;
 
-	if (ctx->chbcr)
+	if (ctx->chbs.base)
 		return 0;
 
 	chbs = (struct acpi_cedt_chbs *) header;
 
 	if (ctx->uid != chbs->uid)
 		return 0;
-	ctx->chbcr = chbs->base;
+	ctx->chbs = *chbs;
 
 	return 0;
 }
 
+static resource_size_t cxl_get_chbcr(struct cxl_chbs_context *ctx)
+{
+	struct acpi_cedt_chbs *chbs = &ctx->chbs;
+	resource_size_t component_reg_phys, rcrb;
+	u32 bar0, bar1;
+	void *addr;
+
+	if (!chbs->base)
+		return CXL_RESOURCE_NONE;
+
+	if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11)
+		return chbs->base;
+
+	/* Extract RCRB */
+
+	if (chbs->length != CXL_RCRB_SIZE)
+		return CXL_RESOURCE_NONE;
+
+	rcrb = chbs->base;
+
+	dev_dbg(ctx->dev, "RCRB found for UID %lld: 0x%08llx\n",
+		ctx->uid, (u64)rcrb);
+
+	/*
+	 * RCRB's BAR[0..1] point to component block containing CXL
+	 * subsystem component registers. MEMBAR extraction follows
+	 * the PCI Base spec here, esp. 64 bit extraction and memory
+	 * ranges alignment (6.0, 7.5.1.2.1).
+	 */
+	addr = ioremap(rcrb, PCI_BASE_ADDRESS_0 + SZ_8);
+	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
+	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
+	iounmap(addr);
+
+	/* sanity check */
+	if (bar0 & (PCI_BASE_ADDRESS_MEM_TYPE_1M | PCI_BASE_ADDRESS_SPACE_IO))
+		return CXL_RESOURCE_NONE;
+
+	component_reg_phys = bar0 & PCI_BASE_ADDRESS_MEM_MASK;
+	if (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_64)
+		component_reg_phys |= ((u64)bar1) << 32;
+
+	if (!component_reg_phys)
+		return CXL_RESOURCE_NONE;
+
+	/*
+	 * Must be 8k aligned (size of combined CXL 1.1 Downstream and
+	 * Upstream Port RCRBs).
+	 */
+	if (component_reg_phys & (CXL_RCRB_SIZE - 1))
+		return CXL_RESOURCE_NONE;
+
+	return component_reg_phys;
+}
+
 static int add_host_bridge_dport(struct device *match, void *arg)
 {
 	acpi_status status;
@@ -259,6 +316,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 	struct cxl_port *root_port = arg;
 	struct device *host = root_port->dev.parent;
 	struct acpi_device *bridge = to_cxl_host_bridge(host, match);
+	resource_size_t component_reg_phys;
 
 	if (!bridge)
 		return 0;
@@ -273,19 +331,20 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 	dev_dbg(match, "UID found: %lld\n", uid);
 
 	ctx = (struct cxl_chbs_context) {
-		.dev = host,
+		.dev = match,
 		.uid = uid,
 	};
-	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbcr, &ctx);
+	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs, &ctx);
 
-	if (ctx.chbcr == 0) {
+	component_reg_phys = cxl_get_chbcr(&ctx);
+	if (component_reg_phys == CXL_RESOURCE_NONE) {
 		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
 		return 0;
 	}
 
-	dev_dbg(match, "CHBCR found: 0x%08llx\n", (u64)ctx.chbcr);
+	dev_dbg(match, "CHBCR found: 0x%08llx\n", (u64)component_reg_phys);
 
-	dport = devm_cxl_add_dport(root_port, match, uid, ctx.chbcr);
+	dport = devm_cxl_add_dport(root_port, match, uid, component_reg_phys);
 	if (IS_ERR(dport))
 		return PTR_ERR(dport);
 
-- 
2.30.2

[PATCH v2 07/12] cxl: Remove dev_is_cxl_root_child() check in devm_cxl_enumerate_ports()

[Robert Richter]

The dev_is_cxl_root_child() check adds complexity to the control flow
of the iterator loop in devm_cxl_enumerate_ports(). This check is
unnecessary and can safely be removed: If the port of a dport_dev is
connected to the cxl root port, the grandparent of dport_dev will be
null. This is checked early in the iterator loop and the function is
left in this case.

Drop this check to ease the control flow in devm_cxl_enumerate_
ports().

This change is a prerequisite of factoring out parts of the loop.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/port.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 0431ed860d8e..9af2d91db9bf 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1395,6 +1395,7 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 				"found already registered port %s:%s\n",
 				dev_name(&port->dev), dev_name(port->uport));
 			rc = cxl_add_ep(dport, &cxlmd->dev);
+			put_device(&port->dev);
 
 			/*
 			 * If the endpoint already exists in the port's list,
@@ -1403,19 +1404,11 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 			 * the parent_port lock as the current port may be being
 			 * reaped.
 			 */
-			if (rc && rc != -EBUSY) {
-				put_device(&port->dev);
+			if (rc && rc != -EBUSY)
 				return rc;
-			}
 
 			/* Any more ports to add between this one and the root? */
-			if (!dev_is_cxl_root_child(&port->dev)) {
-				put_device(&port->dev);
-				continue;
-			}
-
-			put_device(&port->dev);
-			return 0;
+			continue;
 		}
 
 		rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
-- 
2.30.2

[PATCH v2 08/12] cxl: Factor out code in devm_cxl_enumerate_ports() to find_port_attach_ep()

[Robert Richter]

Factor out the code to attach an EP to an existing port. It will be
reused to implement RCH discovery.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/port.c | 66 +++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 25 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 9af2d91db9bf..4b15481426f7 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1286,6 +1286,36 @@ static resource_size_t find_component_registers(struct device *dev)
 	return cxl_regmap_to_base(pdev, &map);
 }
 
+static int find_port_attach_ep(struct cxl_memdev *cxlmd,
+			      struct device *uport_dev,
+			      struct device *dport_dev,
+			      struct device *origin)
+{
+	struct device *dev = &cxlmd->dev;
+	struct cxl_dport *dport;
+	struct cxl_port *port;
+	int rc;
+
+	dev_dbg(dev, "scan: origin: %s dport_dev: %s parent: %s\n",
+		dev_name(origin), dev_name(dport_dev),
+		uport_dev ? dev_name(uport_dev) : "(null)");
+
+	port = find_cxl_port(dport_dev, &dport);
+	if (!port)
+		return 0;
+
+	dev_dbg(dev, "found already registered port %s:%s\n",
+		dev_name(&port->dev), dev_name(port->uport));
+
+	rc = cxl_add_ep(dport, &cxlmd->dev);
+	put_device(&port->dev);
+	if (rc)
+		return rc;
+
+	/* Continue to add more ports between this one and the root. */
+	return 1;
+}
+
 static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 			      struct device *uport_dev,
 			      struct device *dport_dev)
@@ -1373,8 +1403,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 	for (iter = dev; iter; iter = grandparent(iter)) {
 		struct device *dport_dev = grandparent(iter);
 		struct device *uport_dev;
-		struct cxl_dport *dport;
-		struct cxl_port *port;
 
 		if (!dport_dev)
 			return 0;
@@ -1386,30 +1414,18 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 			return -ENXIO;
 		}
 
-		dev_dbg(dev, "scan: iter: %s dport_dev: %s parent: %s\n",
-			dev_name(iter), dev_name(dport_dev),
-			dev_name(uport_dev));
-		port = find_cxl_port(dport_dev, &dport);
-		if (port) {
-			dev_dbg(&cxlmd->dev,
-				"found already registered port %s:%s\n",
-				dev_name(&port->dev), dev_name(port->uport));
-			rc = cxl_add_ep(dport, &cxlmd->dev);
-			put_device(&port->dev);
-
-			/*
-			 * If the endpoint already exists in the port's list,
-			 * that's ok, it was added on a previous pass.
-			 * Otherwise, retry in add_port_attach_ep() after taking
-			 * the parent_port lock as the current port may be being
-			 * reaped.
-			 */
-			if (rc && rc != -EBUSY)
-				return rc;
-
-			/* Any more ports to add between this one and the root? */
+		rc = find_port_attach_ep(cxlmd, uport_dev, dport_dev, iter);
+		/*
+		 * If the endpoint already exists in the port's list,
+		 * that's ok, it was added on a previous pass.
+		 * Otherwise, retry in add_port_attach_ep() after taking
+		 * the parent_port lock as the current port may be being
+		 * reaped.
+		 */
+		if (rc > 0 || rc == -EBUSY)
 			continue;
-		}
+		if (rc)
+			return rc;
 
 		rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
 		/* port missing, try to add parent */
-- 
2.30.2

[PATCH v2 09/12] cxl: Extend devm_cxl_enumerate_ports() to support restricted devices (RCDs)

[Robert Richter]

The PCIe Software View of an RCH and RCD is different to VH mode. An
RCD is paired with an RCH and shows up as RCiEP. Its downstream and
upstream ports are hidden to the PCI hierarchy. This different PCI
topology requires a different handling of RCHs.

Extend devm_cxl_enumerate_ports() to support restricted devices
(RCDs). If an RCD is detected all to do is to search its corresponding
RCH's port and attach the EP to it. Update cxl_mem_find_port() for
proper removal of the EP in delete_endpoint().

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/port.c | 45 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 4b15481426f7..35f8fa98904e 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1384,16 +1384,56 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 	return rc;
 }
 
+static inline bool is_cxl_restricted(struct cxl_memdev *cxlmd)
+{
+	struct device *parent = cxlmd->dev.parent;
+	if (!dev_is_pci(parent))
+		return false;
+	return pci_pcie_type(to_pci_dev(parent)) == PCI_EXP_TYPE_RC_END;
+}
+
+static int restricted_host_enumerate_port(struct cxl_memdev *cxlmd)
+{
+	struct device *dev, *dport_dev, *uport_dev;
+	int count;
+
+	if (!is_cxl_restricted(cxlmd))
+		return 0;
+
+	/*
+	 * The cxlmd is an RCD, the dport_dev of it is the PCI device
+	 * and the uport_dev is its host bridge which is the parent of
+	 * the PCI device.
+	 */
+	dev = &cxlmd->dev;			/* cxlmd */
+	dport_dev = dev->parent;		/* pci_dev */
+	uport_dev = dev->parent->parent;	/* pci_host_bridge */
+
+	count = find_port_attach_ep(cxlmd, uport_dev, dport_dev, dev);
+
+	/* If missing the host is not yet ready. */
+	if (!count)
+		return -EAGAIN;
+
+	return count;
+}
+
 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 {
 	struct device *dev = &cxlmd->dev;
 	struct device *iter;
-	int rc;
+	int count, rc;
 
 	rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd);
 	if (rc)
 		return rc;
 
+	count = restricted_host_enumerate_port(cxlmd);
+	if (count < 0)
+		return count;
+	if (count)
+		return 0;
+
 	/*
 	 * Scan for and add all cxl_ports in this device's ancestry.
 	 * Repeat until no more ports are added. Abort if a port add
@@ -1445,6 +1485,9 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_ports, CXL);
 struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
 				   struct cxl_dport **dport)
 {
+	if (is_cxl_restricted(cxlmd))
+		return find_cxl_port(cxlmd->dev.parent, dport);
+
 	return find_cxl_port(grandparent(&cxlmd->dev), dport);
 }
 EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL);
-- 
2.30.2

[PATCH v2 10/12] cxl: Do not ignore PCI config read errors in match_add_dports()

[Robert Richter]

The link capabilities of a PCI device are read when enumerating its
dports. This is done by reading the PCI config space. If that fails
port enumeration ignores that error. However, reading the PCI config
space should reliably work.

To reduce some complexity to the code flow when factoring out parts of
the code in match_add_dports() for later reuse, change this to throw
an error.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 0dbbe8d39b07..8271b8abde7a 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -47,7 +47,7 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
 		return 0;
 	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
 				  &lnkcap))
-		return 0;
+		return -ENXIO;
 
 	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
 	if (rc)
-- 
2.30.2

[PATCH v2 11/12] cxl: Factor out code in match_add_dports() to pci_dev_add_dport()

[Robert Richter]

Factor out the code to register a PCI device's dport to a port. It
will be reused to implement RCD mode.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/pci.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 8271b8abde7a..667de4f125f6 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -29,14 +29,32 @@ struct cxl_walk_context {
 	int count;
 };
 
+static int pci_dev_add_dport(struct pci_dev *pdev, struct cxl_port *port,
+			      resource_size_t component_reg_phys)
+{
+	struct cxl_dport *dport;
+	u32 lnkcap, port_num;
+
+	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
+				  &lnkcap))
+		return -ENXIO;
+
+	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
+	dport = devm_cxl_add_dport(port, &pdev->dev, port_num,
+				   component_reg_phys);
+	if (IS_ERR(dport))
+		return PTR_ERR(dport);
+
+	return 0;
+}
+
 static int match_add_dports(struct pci_dev *pdev, void *data)
 {
 	struct cxl_walk_context *ctx = data;
 	struct cxl_port *port = ctx->port;
 	int type = pci_pcie_type(pdev);
 	struct cxl_register_map map;
-	struct cxl_dport *dport;
-	u32 lnkcap, port_num;
+	resource_size_t component_reg_phys;
 	int rc;
 
 	if (pdev->bus != ctx->bus)
@@ -45,21 +63,18 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
 		return 0;
 	if (type != ctx->type)
 		return 0;
-	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
-				  &lnkcap))
-		return -ENXIO;
 
 	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
 	if (rc)
 		dev_dbg(&port->dev, "failed to find component registers\n");
 
-	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
-	dport = devm_cxl_add_dport(port, &pdev->dev, port_num,
-				   cxl_regmap_to_base(pdev, &map));
-	if (IS_ERR(dport)) {
-		ctx->error = PTR_ERR(dport);
-		return PTR_ERR(dport);
+	component_reg_phys = cxl_regmap_to_base(pdev, &map);
+	rc = pci_dev_add_dport(pdev, port, component_reg_phys);
+	if (rc) {
+		ctx->error = rc;
+		return rc;
 	}
+
 	ctx->count++;
 
 	return 0;
-- 
2.30.2

[PATCH v2 12/12] cxl: Extend devm_cxl_port_enumerate_dports() to support restricted hosts (RCH)

[Robert Richter]

The PCIe Software View of an RCH and RCD is different to VH mode. An
RCD is paired with an RCH and shows up as RCiEP. Its downstream and
upstream ports are hidden to the PCI hierarchy. This different PCI
topology requires a different handling of RCHs.

Extend devm_cxl_port_enumerate_dports() to support restricted hosts
(RCH). If an RCH is detected, register its port as dport to the
device. An RCH is found if the host's dev 0 func 0 devices is an RCiEP
with an existing PCIe DVSEC for CXL Devices (ID 0000h).

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/pci.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 667de4f125f6..a6b1a1501db3 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -48,6 +48,37 @@ static int pci_dev_add_dport(struct pci_dev *pdev, struct cxl_port *port,
 	return 0;
 }
 
+static int restricted_host_enumerate_dport(struct cxl_port *port,
+					   struct pci_bus *bus)
+{
+	struct pci_dev *pdev;
+	bool is_restricted_host;
+	int rc;
+
+	/* Check CXL DVSEC of dev 0 func 0 */
+	pdev = pci_get_slot(bus, PCI_DEVFN(0, 0));
+
+	is_restricted_host = pdev
+		&& (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END)
+		&& pci_find_dvsec_capability(pdev,
+					PCI_DVSEC_VENDOR_ID_CXL,
+					CXL_DVSEC_PCIE_DEVICE);
+	if (is_restricted_host)
+		rc = pci_dev_add_dport(pdev, port, CXL_RESOURCE_NONE);
+
+	pci_dev_put(pdev);
+
+	if (!is_restricted_host)
+		return 0;
+
+	dev_dbg(bus->bridge, "CXL restricted host found\n");
+
+	if (rc)
+		return rc;
+
+	return 1;
+}
+
 static int match_add_dports(struct pci_dev *pdev, void *data)
 {
 	struct cxl_walk_context *ctx = data;
@@ -91,11 +122,15 @@ int devm_cxl_port_enumerate_dports(struct cxl_port *port)
 {
 	struct pci_bus *bus = cxl_port_to_pci_bus(port);
 	struct cxl_walk_context ctx;
-	int type;
+	int type, count;
 
 	if (!bus)
 		return -ENXIO;
 
+	count = restricted_host_enumerate_dport(port, bus);
+	if (count)
+		return count;
+
 	if (pci_is_root_bus(bus))
 		type = PCI_EXP_TYPE_ROOT_PORT;
 	else
-- 
2.30.2

Re: [PATCH v2 06/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Rafael J. Wysocki]

On Tue, Oct 18, 2022 at 3:24 PM Robert Richter <rrichter@amd.com> wrote:
>
> A downstream port must be connected to a component register block.
> For restricted hosts the base address is determined from the RCRB. The
> RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> get the RCRB and add code to extract the component register block from
> it.
>
> RCRB's BAR[0..1] point to the component block containing CXL subsystem
> component registers. MEMBAR extraction follows the PCI base spec here,
> esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1).
>
> Note: Right now the component register block is used for HDM decoder
> capability only which is optional for RCDs. If unsupported by the RCD,
> the HDM init will fail. It is future work to bypass it in this case.
>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>

What does this S-o-B mean?  If this person is your co-developer, you
need to add a Co-developed-by tag to clarify that.

> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/acpi.c | 79 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 69 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index fb9f72813067..a92d5d7b7a92 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -9,6 +9,8 @@
>  #include "cxlpci.h"
>  #include "cxl.h"
>
> +#define CXL_RCRB_SIZE  SZ_8K
> +
>  static unsigned long cfmws_to_decoder_flags(int restrictions)
>  {
>         unsigned long flags = CXL_DECODER_F_ENABLE;
> @@ -229,27 +231,82 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  struct cxl_chbs_context {
>         struct device *dev;
>         unsigned long long uid;
> -       resource_size_t chbcr;
> +       struct acpi_cedt_chbs chbs;
>  };
>
> -static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> -                        const unsigned long end)
> +static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> +                       const unsigned long end)
>  {
>         struct cxl_chbs_context *ctx = arg;
>         struct acpi_cedt_chbs *chbs;
>
> -       if (ctx->chbcr)
> +       if (ctx->chbs.base)
>                 return 0;
>
>         chbs = (struct acpi_cedt_chbs *) header;
>
>         if (ctx->uid != chbs->uid)
>                 return 0;
> -       ctx->chbcr = chbs->base;
> +       ctx->chbs = *chbs;
>
>         return 0;
>  }
>
> +static resource_size_t cxl_get_chbcr(struct cxl_chbs_context *ctx)
> +{
> +       struct acpi_cedt_chbs *chbs = &ctx->chbs;
> +       resource_size_t component_reg_phys, rcrb;
> +       u32 bar0, bar1;
> +       void *addr;
> +
> +       if (!chbs->base)
> +               return CXL_RESOURCE_NONE;
> +
> +       if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11)
> +               return chbs->base;
> +
> +       /* Extract RCRB */
> +
> +       if (chbs->length != CXL_RCRB_SIZE)
> +               return CXL_RESOURCE_NONE;
> +
> +       rcrb = chbs->base;
> +
> +       dev_dbg(ctx->dev, "RCRB found for UID %lld: 0x%08llx\n",
> +               ctx->uid, (u64)rcrb);
> +
> +       /*
> +        * RCRB's BAR[0..1] point to component block containing CXL
> +        * subsystem component registers. MEMBAR extraction follows
> +        * the PCI Base spec here, esp. 64 bit extraction and memory
> +        * ranges alignment (6.0, 7.5.1.2.1).
> +        */
> +       addr = ioremap(rcrb, PCI_BASE_ADDRESS_0 + SZ_8);
> +       bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> +       bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> +       iounmap(addr);
> +
> +       /* sanity check */
> +       if (bar0 & (PCI_BASE_ADDRESS_MEM_TYPE_1M | PCI_BASE_ADDRESS_SPACE_IO))
> +               return CXL_RESOURCE_NONE;
> +
> +       component_reg_phys = bar0 & PCI_BASE_ADDRESS_MEM_MASK;
> +       if (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_64)
> +               component_reg_phys |= ((u64)bar1) << 32;
> +
> +       if (!component_reg_phys)
> +               return CXL_RESOURCE_NONE;
> +
> +       /*
> +        * Must be 8k aligned (size of combined CXL 1.1 Downstream and
> +        * Upstream Port RCRBs).
> +        */
> +       if (component_reg_phys & (CXL_RCRB_SIZE - 1))
> +               return CXL_RESOURCE_NONE;
> +
> +       return component_reg_phys;
> +}
> +
>  static int add_host_bridge_dport(struct device *match, void *arg)
>  {
>         acpi_status status;
> @@ -259,6 +316,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>         struct cxl_port *root_port = arg;
>         struct device *host = root_port->dev.parent;
>         struct acpi_device *bridge = to_cxl_host_bridge(host, match);
> +       resource_size_t component_reg_phys;
>
>         if (!bridge)
>                 return 0;
> @@ -273,19 +331,20 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>         dev_dbg(match, "UID found: %lld\n", uid);
>
>         ctx = (struct cxl_chbs_context) {
> -               .dev = host,
> +               .dev = match,
>                 .uid = uid,
>         };
> -       acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbcr, &ctx);
> +       acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs, &ctx);
>
> -       if (ctx.chbcr == 0) {
> +       component_reg_phys = cxl_get_chbcr(&ctx);
> +       if (component_reg_phys == CXL_RESOURCE_NONE) {
>                 dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
>                 return 0;
>         }
>
> -       dev_dbg(match, "CHBCR found: 0x%08llx\n", (u64)ctx.chbcr);
> +       dev_dbg(match, "CHBCR found: 0x%08llx\n", (u64)component_reg_phys);
>
> -       dport = devm_cxl_add_dport(root_port, match, uid, ctx.chbcr);
> +       dport = devm_cxl_add_dport(root_port, match, uid, component_reg_phys);
>         if (IS_ERR(dport))
>                 return PTR_ERR(dport);
>
> --
> 2.30.2
>

Re: [PATCH v2 06/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Robert Richter]

On 18.10.22 15:31:16, Rafael J. Wysocki wrote:
> On Tue, Oct 18, 2022 at 3:24 PM Robert Richter <rrichter@amd.com> wrote:
> >
> > A downstream port must be connected to a component register block.
> > For restricted hosts the base address is determined from the RCRB. The
> > RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> > get the RCRB and add code to extract the component register block from
> > it.
> >
> > RCRB's BAR[0..1] point to the component block containing CXL subsystem
> > component registers. MEMBAR extraction follows the PCI base spec here,
> > esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1).
> >
> > Note: Right now the component register block is used for HDM decoder
> > capability only which is optional for RCDs. If unsupported by the RCD,
> > the HDM init will fail. It is future work to bypass it in this case.
> >
> > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> 
> What does this S-o-B mean?  If this person is your co-developer, you
> need to add a Co-developed-by tag to clarify that.
> 
> > Signed-off-by: Robert Richter <rrichter@amd.com>

I picked up an early patch and modified it significantly, so I just
left the S-o-B. I could change this to a Co-developed-by tag. IMO, the
S-o-B is ok, but could be wrong here.

-Robert

Re: [PATCH v2 06/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Rafael J. Wysocki]

On Tue, Oct 18, 2022 at 8:42 PM Robert Richter <rrichter@amd.com> wrote:
>
> On 18.10.22 15:31:16, Rafael J. Wysocki wrote:
> > On Tue, Oct 18, 2022 at 3:24 PM Robert Richter <rrichter@amd.com> wrote:
> > >
> > > A downstream port must be connected to a component register block.
> > > For restricted hosts the base address is determined from the RCRB. The
> > > RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> > > get the RCRB and add code to extract the component register block from
> > > it.
> > >
> > > RCRB's BAR[0..1] point to the component block containing CXL subsystem
> > > component registers. MEMBAR extraction follows the PCI base spec here,
> > > esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1).
> > >
> > > Note: Right now the component register block is used for HDM decoder
> > > capability only which is optional for RCDs. If unsupported by the RCD,
> > > the HDM init will fail. It is future work to bypass it in this case.
> > >
> > > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> >
> > What does this S-o-B mean?  If this person is your co-developer, you
> > need to add a Co-developed-by tag to clarify that.
> >
> > > Signed-off-by: Robert Richter <rrichter@amd.com>
>
> I picked up an early patch and modified it significantly, so I just
> left the S-o-B.

In that case the right thing to do is to mention the original author
in the changelog instead of retaining the S-o-b.

> I could change this to a Co-developed-by tag.

Co-developed-by should be used in addition to and not instead of S-o-b
when one of the authors is sending a patch.  However, all of the
authors need to be familiar with the patch in the form in which it is
being sent then.

> IMO, the S-o-B is ok, but could be wrong here.

It isn't, at least not without a Co-developed-by tag.

There are 3 cases in which S-o-b is OK AFAICS:

1. When it matches the From: address.
2. When there is a matching Co-developed-by.
3. When maintainers pick up patches and add their own S-o-b.

This case is none of the above.

Re: [PATCH v2 06/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Robert Richter]

On 18.10.22 20:57:02, Rafael J. Wysocki wrote:
> On Tue, Oct 18, 2022 at 8:42 PM Robert Richter <rrichter@amd.com> wrote:
> >
> > On 18.10.22 15:31:16, Rafael J. Wysocki wrote:
> > > On Tue, Oct 18, 2022 at 3:24 PM Robert Richter <rrichter@amd.com> wrote:
> > > >
> > > > A downstream port must be connected to a component register block.
> > > > For restricted hosts the base address is determined from the RCRB. The
> > > > RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> > > > get the RCRB and add code to extract the component register block from
> > > > it.
> > > >
> > > > RCRB's BAR[0..1] point to the component block containing CXL subsystem
> > > > component registers. MEMBAR extraction follows the PCI base spec here,
> > > > esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1).
> > > >
> > > > Note: Right now the component register block is used for HDM decoder
> > > > capability only which is optional for RCDs. If unsupported by the RCD,
> > > > the HDM init will fail. It is future work to bypass it in this case.
> > > >
> > > > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > >
> > > What does this S-o-B mean?  If this person is your co-developer, you
> > > need to add a Co-developed-by tag to clarify that.
> > >
> > > > Signed-off-by: Robert Richter <rrichter@amd.com>
> >
> > I picked up an early patch and modified it significantly, so I just
> > left the S-o-B.
> 
> In that case the right thing to do is to mention the original author
> in the changelog instead of retaining the S-o-b.
> 
> > I could change this to a Co-developed-by tag.
> 
> Co-developed-by should be used in addition to and not instead of S-o-b
> when one of the authors is sending a patch.  However, all of the
> authors need to be familiar with the patch in the form in which it is
> being sent then.
> 
> > IMO, the S-o-B is ok, but could be wrong here.
> 
> It isn't, at least not without a Co-developed-by tag.
> 
> There are 3 cases in which S-o-b is OK AFAICS:
> 
> 1. When it matches the From: address.
> 2. When there is a matching Co-developed-by.
> 3. When maintainers pick up patches and add their own S-o-b.
> 
> This case is none of the above.

Will add a Co-developed-by tag in my next version. Thanks for pointing
that out.

-Robert

RE: [PATCH v2 01/12] cxl/core: Remove duplicate declaration of devm_cxl_iomap_block()

[Dan Williams]

Robert Richter wrote:
> The function devm_cxl_iomap_block() is only used in the core
> code. There are two declarations in header files of it, in
> drivers/cxl/core/core.h and drivers/cxl/cxl.h. Remove its unused
> declaration in drivers/cxl/cxl.h.
> 
> Fixing build error in regs.c found by kernel test robot by including
> "core.h" there.

Fixes a build error only with the new code in this series, or do you
have a config where this fails with current mainline?

Assuming the former, this is applied for v6.2.

RE: [PATCH v2 02/12] cxl/core: Check physical address before mapping it in devm_cxl_iomap_block()

[Dan Williams]

Robert Richter wrote:
> The physical base address of a CXL range can be invalid and is then
> set to CXL_RESOURCE_NONE. In general software shall prevent such
> situations, but it is hard to proof this may never happen. E.g. in
> add_port_attach_ep() there this the following:
> 
>       component_reg_phys = find_component_registers(uport_dev);
>       port = devm_cxl_add_port(&parent_port->dev, uport_dev,
>               component_reg_phys, parent_dport);
> 
> find_component_registers() and subsequent functions (e.g.
> cxl_regmap_to_base()) may return CXL_RESOURCE_NONE. But it is written
> to port without any further check in cxl_port_alloc():
> 
>       port->component_reg_phys = component_reg_phys;
> 
> It is then later directly used in devm_cxl_setup_hdm() to map io
> ranges with devm_cxl_iomap_block(). Just an example...
> 
> Check this condition. Also do not fail silently like an ioremap()
> failure, use a WARN_ON_ONCE() for it.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>

Looks good to me, applied for v6.2

RE: [PATCH v2 03/12] cxl: Unify debug messages when calling devm_cxl_add_port()

[Dan Williams]

Robert Richter wrote:
> CXL ports are added in a couple of code paths using devm_cxl_add_
> port(). Debug messages are individually generated, but are incomplete

Hmm, devm_cxl_add_port() got wrapped in a few places here. I fixed it up
locally, but maybe take a look at your word wrap to keep function
symbols unbroken in future changelogs.

> and inconsistent. Change this by moving its generation to devm_cxl_
> add_port(). This unifies the messages and reduces code duplication.
> Also, generate messages on failure. Use a __devm_cxl_add_port()
> wrapper to keep the readability of the error exits.

Otherwise looks good, applied for v6.2.

RE: [PATCH v2 04/12] cxl: Unify debug messages when calling devm_cxl_add_dport()

[Dan Williams]

Robert Richter wrote:
> CXL dports are added in a couple of code paths using devm_cxl_add_
> dport(). Debug messages are individually generated, but are incomplete
> and inconsistent. Change this by moving its generation to devm_cxl_
> add_dport(). This unifies the messages and reduces code duplication.
> Also, generate messages on failure. Use a __devm_cxl_add_dport()
> wrapper to keep the readability of the error exits.

Looks good, I fixed up the wrapping of devm_cxl_add_dport() and one more
fixup below, but applied for v6.2.

> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/acpi.c           |  7 ++----
>  drivers/cxl/core/pci.c       |  2 --
>  drivers/cxl/core/port.c      | 48 +++++++++++++++++++++++++-----------
>  tools/testing/cxl/test/cxl.c |  8 +-----
>  4 files changed, 37 insertions(+), 28 deletions(-)
> 
[..]
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index a072b2d3e726..c610625e8261 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -582,14 +582,8 @@ static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
>  		dport = devm_cxl_add_dport(port, &pdev->dev, pdev->id,
>  					   CXL_RESOURCE_NONE);
>  
> -		if (IS_ERR(dport)) {
> -			dev_err(dev, "failed to add dport: %s (%ld)\n",
> -				dev_name(&pdev->dev), PTR_ERR(dport));
> +		if (IS_ERR(dport))
>  			return PTR_ERR(dport);
> -		}
> -
> -		dev_dbg(dev, "add dport%d: %s\n", pdev->id,
> -			dev_name(&pdev->dev));

This hunk causes this new compile error:

tools/testing/cxl/test/cxl.c: In function ‘mock_cxl_port_enumerate_dports’:
tools/testing/cxl/test/cxl.c:559:24: error: unused variable ‘dev’ [-Werror=unused-variable]
  559 |         struct device *dev = &port->dev;
      |                        ^~~
cc1: all warnings being treated as errors

I fixed it up locally, but just double check that you are building
cxl_test if you touch tools/testing/cxl/:

    make M=tools/testing/cxl

RE: [PATCH v2 05/12] cxl/acpi: Improve debug messages in cxl_acpi_probe()

[Dan Williams]

Robert Richter wrote:
> In cxl_acpi_probe() the iterator bus_for_each_dev() walks through all
> CXL hosts. Since all dev_*() debug messages point to the ACPI0017
> device which is the CXL root for all hosts, the device information is
> pointless as it is always the same device. Change this to use the host
> device for this instead.
> 
> Also, add additional host specific information such as CXL support,
> UID and CHBCR.
> 
> This is an example log:
> 
>  acpi ACPI0016:00: UID found: 4
>  acpi ACPI0016:00: CHBCR found: 0x28090000000
>  acpi ACPI0016:00: dport added to root0
>  acpi ACPI0016:00: host-bridge: ACPI0016:00
>   pci0000:7f: host supports CXL
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/acpi.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 31e104f0210f..fb9f72813067 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -221,6 +221,8 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  	if (IS_ERR(port))
>  		return PTR_ERR(port);
>  
> +	dev_info(pci_root->bus->bridge, "host supports CXL\n");
> +

I generally advocate for drivers to be silent, but this is a fundamental
message in a place that is helpful to indicate whether platform-firmware
is doing its job correctly, and it's only once per host bridge.

Applied for v6.2.

RE: [PATCH v2 06/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Dan Williams]

Robert Richter wrote:
> A downstream port must be connected to a component register block.
> For restricted hosts the base address is determined from the RCRB. The
> RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> get the RCRB and add code to extract the component register block from
> it.
> 
> RCRB's BAR[0..1] point to the component block containing CXL subsystem
> component registers. MEMBAR extraction follows the PCI base spec here,
> esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1).
> 
> Note: Right now the component register block is used for HDM decoder
> capability only which is optional for RCDs. If unsupported by the RCD,
> the HDM init will fail. It is future work to bypass it in this case.
> 
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/acpi.c | 79 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 69 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index fb9f72813067..a92d5d7b7a92 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -9,6 +9,8 @@
>  #include "cxlpci.h"
>  #include "cxl.h"
>  
> +#define CXL_RCRB_SIZE	SZ_8K
> +
>  static unsigned long cfmws_to_decoder_flags(int restrictions)
>  {
>  	unsigned long flags = CXL_DECODER_F_ENABLE;
> @@ -229,27 +231,82 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  struct cxl_chbs_context {
>  	struct device *dev;
>  	unsigned long long uid;
> -	resource_size_t chbcr;
> +	struct acpi_cedt_chbs chbs;
>  };
>  
> -static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> -			 const unsigned long end)
> +static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> +			const unsigned long end)
>  {
>  	struct cxl_chbs_context *ctx = arg;
>  	struct acpi_cedt_chbs *chbs;
>  
> -	if (ctx->chbcr)
> +	if (ctx->chbs.base)
>  		return 0;
>  
>  	chbs = (struct acpi_cedt_chbs *) header;
>  
>  	if (ctx->uid != chbs->uid)
>  		return 0;
> -	ctx->chbcr = chbs->base;
> +	ctx->chbs = *chbs;
>  
>  	return 0;
>  }
>  
> +static resource_size_t cxl_get_chbcr(struct cxl_chbs_context *ctx)
> +{

The core logic of this looks good, but this wants to be shared with the
upstream port component register discovery.

Full disclosure I am reconciling these patches with an attempt that Dave
Jiang made at this topic. Since your series hit the list first I will
let it take the lead, but then fill it in with comments and learnings
from Dave's effort.

So in this case Dave moved this into the drivers/cxl/core/regs.c with a
function signature like:

enum cxl_rcrb {
       CXL_RCRB_DOWNSTREAM,
       CXL_RCRB_UPSTREAM,
};

resource_size_t cxl_rcrb_to_component(struct device *dev,
                                      resource_size_t rcrb_base, int len,
                                      enum cxl_rcrb which);

...where @which alternates when called by cxl_acpi for the downstream
case, or cxl_mem for the upstream case.


> +	struct acpi_cedt_chbs *chbs = &ctx->chbs;
> +	resource_size_t component_reg_phys, rcrb;
> +	u32 bar0, bar1;
> +	void *addr;
> +
> +	if (!chbs->base)
> +		return CXL_RESOURCE_NONE;
> +
> +	if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11)
> +		return chbs->base;
> +
> +	/* Extract RCRB */
> +
> +	if (chbs->length != CXL_RCRB_SIZE)
> +		return CXL_RESOURCE_NONE;
> +
> +	rcrb = chbs->base;
> +
> +	dev_dbg(ctx->dev, "RCRB found for UID %lld: 0x%08llx\n",
> +		ctx->uid, (u64)rcrb);
> +
> +	/*
> +	 * RCRB's BAR[0..1] point to component block containing CXL
> +	 * subsystem component registers. MEMBAR extraction follows
> +	 * the PCI Base spec here, esp. 64 bit extraction and memory
> +	 * ranges alignment (6.0, 7.5.1.2.1).
> +	 */
> +	addr = ioremap(rcrb, PCI_BASE_ADDRESS_0 + SZ_8);

No failure check? This also only needs to map 4K at a time.

> +	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> +	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> +	iounmap(addr);
> +
> +	/* sanity check */
> +	if (bar0 & (PCI_BASE_ADDRESS_MEM_TYPE_1M | PCI_BASE_ADDRESS_SPACE_IO))
> +		return CXL_RESOURCE_NONE;
> +
> +	component_reg_phys = bar0 & PCI_BASE_ADDRESS_MEM_MASK;
> +	if (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_64)
> +		component_reg_phys |= ((u64)bar1) << 32;
> +
> +	if (!component_reg_phys)
> +		return CXL_RESOURCE_NONE;
> +
> +	/*
> +	 * Must be 8k aligned (size of combined CXL 1.1 Downstream and
> +	 * Upstream Port RCRBs).
> +	 */
> +	if (component_reg_phys & (CXL_RCRB_SIZE - 1))
> +		return CXL_RESOURCE_NONE;

This is open-coding the IS_ALIGNED() macro. More importantly, why is it
using RCRB size for the component register block alignment? The
component lock is 64K, and at least for CXL 2.0 devices it is 64K
aligned (8.1.9.1 Register Block Offset Low), so I am not sure what this
check is for?

---

Given that there are actual CXL RCH platforms in the wild I want this
topic branch to be the first thing queued for v6.2. To help us
coordinate I pushed:

https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=rch

...with the patches from this set accepted so far. You can use that as
the baseline for the next spin.

RE: [PATCH v2 07/12] cxl: Remove dev_is_cxl_root_child() check in devm_cxl_enumerate_ports()

[Dan Williams]

Robert Richter wrote:
> The dev_is_cxl_root_child() check adds complexity to the control flow
> of the iterator loop in devm_cxl_enumerate_ports(). This check is
> unnecessary and can safely be removed: If the port of a dport_dev is
> connected to the cxl root port, the grandparent of dport_dev will be
> null. This is checked early in the iterator loop and the function is
> left in this case.
> 
> Drop this check to ease the control flow in devm_cxl_enumerate_
> ports().
> 
> This change is a prerequisite of factoring out parts of the loop.

Ok, so this seems to be where we diverge on how an RCH topology maps
into the CXL subsystem object hierarchy. The main observation going
through this with Dave before this set came out is that
devm_cxl_enumerate_ports() is a nop and should be skipped in the RCH
case. devm_cxl_enumerate_ports() is only for discovering intermediate
ports between a host bridge and an endpoint.

In a CXL VH topology a direct attached endpoint is downstream of a
host-bridge's root port. In the CXL RCH topology the
Root-Complex-Integrated-Endpoint is a *peer* of a host-bridge's root
port. So one level of the hierarchy is removed and
devm_cxl_enumerate_ports() can be skipped.

The proposal is to have cxl_mem_probe() do something like:

       if (!cxlds->is_rcd) {
               rc = devm_cxl_enumerate_ports(cxlmd);
               if (rc)
                       return rc;
       }

The existing:

       parent_port = cxl_mem_find_port(cxlmd, &dport);

...should do the right thing as long as cxl_acpi registers the host
bridge as a dport with this change:

-       dport = devm_cxl_add_dport(root_port, match, uid, ...
+       dport = devm_cxl_add_dport(root_port, pci_root->bus->bridge, uid, ... 

That way the dport device is not the ACPI device but the 'struct
pci_dev' for the host-bridge.

With that scheme in place and some cxl-cli fixups from Vishal we are
seeing:

# cxl list -BEMPTu
{
  "bus":"root0",
  "provider":"ACPI.CXL",
  "nr_dports":1,
  "dports":[
    {
      "dport":"pci0000:38",
      "id":"0x31"
    }
  ],
  "endpoints:root0":[
    {
      "endpoint":"endpoint1",
      "host":"mem0",
      "depth":1,
      "memdev":{
        "memdev":"mem0",
        "pmem_size":0,
        "ram_size":"16.00 GiB (17.18 GB)",
        "serial":"0",
        "numa_node":0,
        "host":"0000:38:00.0"
      }
    }
  ]
}

Does that make sense? I think this patchset gets a lot simpler if it
does not try to make devm_cxl_enumerate_ports() understand the RCH
topology.

Re: [PATCH v2 07/12] cxl: Remove dev_is_cxl_root_child() check in devm_cxl_enumerate_ports()

[Verma, Vishal L]

On Thu, 2022-10-20 at 22:38 -0700, Dan Williams wrote:
> Robert Richter wrote:
> > 

<..>

> 
> With that scheme in place and some cxl-cli fixups from Vishal we are
> seeing:
> 
> # cxl list -BEMPTu
> {
>   "bus":"root0",
>   "provider":"ACPI.CXL",
>   "nr_dports":1,
>   "dports":[
>     {
>       "dport":"pci0000:38",
>       "id":"0x31"
>     }
>   ],
>   "endpoints:root0":[
>     {
>       "endpoint":"endpoint1",
>       "host":"mem0",
>       "depth":1,
>       "memdev":{
>         "memdev":"mem0",
>         "pmem_size":0,
>         "ram_size":"16.00 GiB (17.18 GB)",
>         "serial":"0",
>         "numa_node":0,
>         "host":"0000:38:00.0"
>       }
>     }
>   ]
> }

I was waiting to post the cxl-cli patches for this until the kernel
discussion settles - but maybe this topology layout is settled enough?

In the meanwhile I've pushed a branch with these fixups here:
https://github.com/pmem/ndctl/tree/vv/rch-support

> 
> Does that make sense? I think this patchset gets a lot simpler if it
> does not try to make devm_cxl_enumerate_ports() understand the RCH
> topology.

Re: [PATCH v2 01/12] cxl/core: Remove duplicate declaration of devm_cxl_iomap_block()

[Robert Richter]

On 20.10.22 16:56:51, Dan Williams wrote:
> Robert Richter wrote:
> > The function devm_cxl_iomap_block() is only used in the core
> > code. There are two declarations in header files of it, in
> > drivers/cxl/core/core.h and drivers/cxl/cxl.h. Remove its unused
> > declaration in drivers/cxl/cxl.h.
> > 
> > Fixing build error in regs.c found by kernel test robot by including
> > "core.h" there.
> 
> Fixes a build error only with the new code in this series, or do you
> have a config where this fails with current mainline?
> 
> Assuming the former, this is applied for v6.2.

Yes, a build error in v1.

Thanks,

-Robert

Re: [PATCH v2 03/12] cxl: Unify debug messages when calling devm_cxl_add_port()

[Robert Richter]

On 20.10.22 17:20:10, Dan Williams wrote:
> Robert Richter wrote:
> > CXL ports are added in a couple of code paths using devm_cxl_add_
> > port(). Debug messages are individually generated, but are incomplete
> 
> Hmm, devm_cxl_add_port() got wrapped in a few places here. I fixed it up
> locally, but maybe take a look at your word wrap to keep function
> symbols unbroken in future changelogs.

Ok, will do. Thanks for the note.

> 
> > and inconsistent. Change this by moving its generation to devm_cxl_
> > add_port(). This unifies the messages and reduces code duplication.
> > Also, generate messages on failure. Use a __devm_cxl_add_port()
> > wrapper to keep the readability of the error exits.
> 
> Otherwise looks good, applied for v6.2.

Thanks,

-Robert

Re: [PATCH v2 04/12] cxl: Unify debug messages when calling devm_cxl_add_dport()

[Robert Richter]

On 20.10.22 17:32:58, Dan Williams wrote:
> Robert Richter wrote:

> > diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> > index a072b2d3e726..c610625e8261 100644
> > --- a/tools/testing/cxl/test/cxl.c
> > +++ b/tools/testing/cxl/test/cxl.c
> > @@ -582,14 +582,8 @@ static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
> >  		dport = devm_cxl_add_dport(port, &pdev->dev, pdev->id,
> >  					   CXL_RESOURCE_NONE);
> >  
> > -		if (IS_ERR(dport)) {
> > -			dev_err(dev, "failed to add dport: %s (%ld)\n",
> > -				dev_name(&pdev->dev), PTR_ERR(dport));
> > +		if (IS_ERR(dport))
> >  			return PTR_ERR(dport);
> > -		}
> > -
> > -		dev_dbg(dev, "add dport%d: %s\n", pdev->id,
> > -			dev_name(&pdev->dev));
> 
> This hunk causes this new compile error:
> 
> tools/testing/cxl/test/cxl.c: In function ‘mock_cxl_port_enumerate_dports’:
> tools/testing/cxl/test/cxl.c:559:24: error: unused variable ‘dev’ [-Werror=unused-variable]
>   559 |         struct device *dev = &port->dev;
>       |                        ^~~
> cc1: all warnings being treated as errors
> 
> I fixed it up locally, but just double check that you are building
> cxl_test if you touch tools/testing/cxl/:
> 
>     make M=tools/testing/cxl

Right. Thanks for fixing. Will build test in the future.

-Robert

Re: [PATCH v2 06/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Robert Richter]

On 20.10.22 22:17:07, Dan Williams wrote:
> Robert Richter wrote:
> > A downstream port must be connected to a component register block.
> > For restricted hosts the base address is determined from the RCRB. The
> > RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> > get the RCRB and add code to extract the component register block from
> > it.
> > 
> > RCRB's BAR[0..1] point to the component block containing CXL subsystem
> > component registers. MEMBAR extraction follows the PCI base spec here,
> > esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1).
> > 
> > Note: Right now the component register block is used for HDM decoder
> > capability only which is optional for RCDs. If unsupported by the RCD,
> > the HDM init will fail. It is future work to bypass it in this case.
> > 
> > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > ---
> >  drivers/cxl/acpi.c | 79 ++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 69 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index fb9f72813067..a92d5d7b7a92 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -9,6 +9,8 @@
> >  #include "cxlpci.h"
> >  #include "cxl.h"
> >  
> > +#define CXL_RCRB_SIZE	SZ_8K
> > +
> >  static unsigned long cfmws_to_decoder_flags(int restrictions)
> >  {
> >  	unsigned long flags = CXL_DECODER_F_ENABLE;
> > @@ -229,27 +231,82 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> >  struct cxl_chbs_context {
> >  	struct device *dev;
> >  	unsigned long long uid;
> > -	resource_size_t chbcr;
> > +	struct acpi_cedt_chbs chbs;
> >  };
> >  
> > -static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> > -			 const unsigned long end)
> > +static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> > +			const unsigned long end)
> >  {
> >  	struct cxl_chbs_context *ctx = arg;
> >  	struct acpi_cedt_chbs *chbs;
> >  
> > -	if (ctx->chbcr)
> > +	if (ctx->chbs.base)
> >  		return 0;
> >  
> >  	chbs = (struct acpi_cedt_chbs *) header;
> >  
> >  	if (ctx->uid != chbs->uid)
> >  		return 0;
> > -	ctx->chbcr = chbs->base;
> > +	ctx->chbs = *chbs;
> >  
> >  	return 0;
> >  }
> >  
> > +static resource_size_t cxl_get_chbcr(struct cxl_chbs_context *ctx)
> > +{
> 
> The core logic of this looks good, but this wants to be shared with the
> upstream port component register discovery.
> 
> Full disclosure I am reconciling these patches with an attempt that Dave
> Jiang made at this topic. Since your series hit the list first I will
> let it take the lead, but then fill it in with comments and learnings
> from Dave's effort.
> 
> So in this case Dave moved this into the drivers/cxl/core/regs.c with a
> function signature like:
> 
> enum cxl_rcrb {
>        CXL_RCRB_DOWNSTREAM,
>        CXL_RCRB_UPSTREAM,
> };
> 
> resource_size_t cxl_rcrb_to_component(struct device *dev,
>                                       resource_size_t rcrb_base, int len,
>                                       enum cxl_rcrb which);
> 
> ...where @which alternates when called by cxl_acpi for the downstream
> case, or cxl_mem for the upstream case.

Ok, I see where to go here. Could you point me to Dave's postings you
are referring to? I checked linux-cxl and could not find anything
related to RCRB or that changes regs.c.

> 
> 
> > +	struct acpi_cedt_chbs *chbs = &ctx->chbs;
> > +	resource_size_t component_reg_phys, rcrb;
> > +	u32 bar0, bar1;
> > +	void *addr;
> > +
> > +	if (!chbs->base)
> > +		return CXL_RESOURCE_NONE;
> > +
> > +	if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11)
> > +		return chbs->base;
> > +
> > +	/* Extract RCRB */
> > +
> > +	if (chbs->length != CXL_RCRB_SIZE)
> > +		return CXL_RESOURCE_NONE;
> > +
> > +	rcrb = chbs->base;
> > +
> > +	dev_dbg(ctx->dev, "RCRB found for UID %lld: 0x%08llx\n",
> > +		ctx->uid, (u64)rcrb);
> > +
> > +	/*
> > +	 * RCRB's BAR[0..1] point to component block containing CXL
> > +	 * subsystem component registers. MEMBAR extraction follows
> > +	 * the PCI Base spec here, esp. 64 bit extraction and memory
> > +	 * ranges alignment (6.0, 7.5.1.2.1).
> > +	 */
> > +	addr = ioremap(rcrb, PCI_BASE_ADDRESS_0 + SZ_8);
> 
> No failure check? This also only needs to map 4K at a time.

Right, will add that.

> 
> > +	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> > +	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> > +	iounmap(addr);
> > +
> > +	/* sanity check */
> > +	if (bar0 & (PCI_BASE_ADDRESS_MEM_TYPE_1M | PCI_BASE_ADDRESS_SPACE_IO))
> > +		return CXL_RESOURCE_NONE;
> > +
> > +	component_reg_phys = bar0 & PCI_BASE_ADDRESS_MEM_MASK;
> > +	if (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_64)
> > +		component_reg_phys |= ((u64)bar1) << 32;
> > +
> > +	if (!component_reg_phys)
> > +		return CXL_RESOURCE_NONE;
> > +
> > +	/*
> > +	 * Must be 8k aligned (size of combined CXL 1.1 Downstream and
> > +	 * Upstream Port RCRBs).
> > +	 */
> > +	if (component_reg_phys & (CXL_RCRB_SIZE - 1))
> > +		return CXL_RESOURCE_NONE;
> 
> This is open-coding the IS_ALIGNED() macro. More importantly, why is it
> using RCRB size for the component register block alignment? The
> component lock is 64K, and at least for CXL 2.0 devices it is 64K
> aligned (8.1.9.1 Register Block Offset Low), so I am not sure what this
> check is for?

True, this is a mistake and needs to be corrected. It is the component
reg range which is 64k. Will also use IS_ALIGNED().

> 
> ---
> 
> Given that there are actual CXL RCH platforms in the wild I want this
> topic branch to be the first thing queued for v6.2. To help us
> coordinate I pushed:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=rch
> 
> ...with the patches from this set accepted so far. You can use that as
> the baseline for the next spin.

Yes, thanks for that branch and applying the first part.

-Robert

Re: [PATCH v2 06/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Dan Williams]

Robert Richter wrote:
> On 20.10.22 22:17:07, Dan Williams wrote:
> > Robert Richter wrote:
> > > A downstream port must be connected to a component register block.
> > > For restricted hosts the base address is determined from the RCRB. The
> > > RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> > > get the RCRB and add code to extract the component register block from
> > > it.
> > > 
> > > RCRB's BAR[0..1] point to the component block containing CXL subsystem
> > > component registers. MEMBAR extraction follows the PCI base spec here,
> > > esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1).
> > > 
> > > Note: Right now the component register block is used for HDM decoder
> > > capability only which is optional for RCDs. If unsupported by the RCD,
> > > the HDM init will fail. It is future work to bypass it in this case.
> > > 
> > > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > ---
> > >  drivers/cxl/acpi.c | 79 ++++++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 69 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > index fb9f72813067..a92d5d7b7a92 100644
> > > --- a/drivers/cxl/acpi.c
> > > +++ b/drivers/cxl/acpi.c
> > > @@ -9,6 +9,8 @@
> > >  #include "cxlpci.h"
> > >  #include "cxl.h"
> > >  
> > > +#define CXL_RCRB_SIZE	SZ_8K
> > > +
> > >  static unsigned long cfmws_to_decoder_flags(int restrictions)
> > >  {
> > >  	unsigned long flags = CXL_DECODER_F_ENABLE;
> > > @@ -229,27 +231,82 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> > >  struct cxl_chbs_context {
> > >  	struct device *dev;
> > >  	unsigned long long uid;
> > > -	resource_size_t chbcr;
> > > +	struct acpi_cedt_chbs chbs;
> > >  };
> > >  
> > > -static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> > > -			 const unsigned long end)
> > > +static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> > > +			const unsigned long end)
> > >  {
> > >  	struct cxl_chbs_context *ctx = arg;
> > >  	struct acpi_cedt_chbs *chbs;
> > >  
> > > -	if (ctx->chbcr)
> > > +	if (ctx->chbs.base)
> > >  		return 0;
> > >  
> > >  	chbs = (struct acpi_cedt_chbs *) header;
> > >  
> > >  	if (ctx->uid != chbs->uid)
> > >  		return 0;
> > > -	ctx->chbcr = chbs->base;
> > > +	ctx->chbs = *chbs;
> > >  
> > >  	return 0;
> > >  }
> > >  
> > > +static resource_size_t cxl_get_chbcr(struct cxl_chbs_context *ctx)
> > > +{
> > 
> > The core logic of this looks good, but this wants to be shared with the
> > upstream port component register discovery.
> > 
> > Full disclosure I am reconciling these patches with an attempt that Dave
> > Jiang made at this topic. Since your series hit the list first I will
> > let it take the lead, but then fill it in with comments and learnings
> > from Dave's effort.
> > 
> > So in this case Dave moved this into the drivers/cxl/core/regs.c with a
> > function signature like:
> > 
> > enum cxl_rcrb {
> >        CXL_RCRB_DOWNSTREAM,
> >        CXL_RCRB_UPSTREAM,
> > };
> > 
> > resource_size_t cxl_rcrb_to_component(struct device *dev,
> >                                       resource_size_t rcrb_base, int len,
> >                                       enum cxl_rcrb which);
> > 
> > ...where @which alternates when called by cxl_acpi for the downstream
> > case, or cxl_mem for the upstream case.
> 
> Ok, I see where to go here. Could you point me to Dave's postings you
> are referring to? I checked linux-cxl and could not find anything
> related to RCRB or that changes regs.c.

He was in the middle of tidying them when you posted your series, but I
think it would not hurt to push them to a git tree so you can grab the
bits and pieces you want.

Dave?

Re: [PATCH v2 06/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Dan Williams]

Dan Williams wrote:
> Robert Richter wrote:
> > On 20.10.22 22:17:07, Dan Williams wrote:
> > > Robert Richter wrote:
> > > > A downstream port must be connected to a component register block.
> > > > For restricted hosts the base address is determined from the RCRB. The
> > > > RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> > > > get the RCRB and add code to extract the component register block from
> > > > it.
> > > > 
> > > > RCRB's BAR[0..1] point to the component block containing CXL subsystem
> > > > component registers. MEMBAR extraction follows the PCI base spec here,
> > > > esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1).
> > > > 
> > > > Note: Right now the component register block is used for HDM decoder
> > > > capability only which is optional for RCDs. If unsupported by the RCD,
> > > > the HDM init will fail. It is future work to bypass it in this case.
> > > > 
> > > > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > > ---
> > > >  drivers/cxl/acpi.c | 79 ++++++++++++++++++++++++++++++++++++++++------
> > > >  1 file changed, 69 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > > index fb9f72813067..a92d5d7b7a92 100644
> > > > --- a/drivers/cxl/acpi.c
> > > > +++ b/drivers/cxl/acpi.c
> > > > @@ -9,6 +9,8 @@
> > > >  #include "cxlpci.h"
> > > >  #include "cxl.h"
> > > >  
> > > > +#define CXL_RCRB_SIZE	SZ_8K
> > > > +
> > > >  static unsigned long cfmws_to_decoder_flags(int restrictions)
> > > >  {
> > > >  	unsigned long flags = CXL_DECODER_F_ENABLE;
> > > > @@ -229,27 +231,82 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> > > >  struct cxl_chbs_context {
> > > >  	struct device *dev;
> > > >  	unsigned long long uid;
> > > > -	resource_size_t chbcr;
> > > > +	struct acpi_cedt_chbs chbs;
> > > >  };
> > > >  
> > > > -static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> > > > -			 const unsigned long end)
> > > > +static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> > > > +			const unsigned long end)
> > > >  {
> > > >  	struct cxl_chbs_context *ctx = arg;
> > > >  	struct acpi_cedt_chbs *chbs;
> > > >  
> > > > -	if (ctx->chbcr)
> > > > +	if (ctx->chbs.base)
> > > >  		return 0;
> > > >  
> > > >  	chbs = (struct acpi_cedt_chbs *) header;
> > > >  
> > > >  	if (ctx->uid != chbs->uid)
> > > >  		return 0;
> > > > -	ctx->chbcr = chbs->base;
> > > > +	ctx->chbs = *chbs;
> > > >  
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static resource_size_t cxl_get_chbcr(struct cxl_chbs_context *ctx)
> > > > +{
> > > 
> > > The core logic of this looks good, but this wants to be shared with the
> > > upstream port component register discovery.
> > > 
> > > Full disclosure I am reconciling these patches with an attempt that Dave
> > > Jiang made at this topic. Since your series hit the list first I will
> > > let it take the lead, but then fill it in with comments and learnings
> > > from Dave's effort.
> > > 
> > > So in this case Dave moved this into the drivers/cxl/core/regs.c with a
> > > function signature like:
> > > 
> > > enum cxl_rcrb {
> > >        CXL_RCRB_DOWNSTREAM,
> > >        CXL_RCRB_UPSTREAM,
> > > };
> > > 
> > > resource_size_t cxl_rcrb_to_component(struct device *dev,
> > >                                       resource_size_t rcrb_base, int len,
> > >                                       enum cxl_rcrb which);
> > > 
> > > ...where @which alternates when called by cxl_acpi for the downstream
> > > case, or cxl_mem for the upstream case.
> > 
> > Ok, I see where to go here. Could you point me to Dave's postings you
> > are referring to? I checked linux-cxl and could not find anything
> > related to RCRB or that changes regs.c.
> 
> He was in the middle of tidying them when you posted your series, but I
> think it would not hurt to push them to a git tree so you can grab the
> bits and pieces you want.
> 
> Dave?

Looks like the list delivery is backed up, so I added Dave to the Cc:.

He pushed:

https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch

...which was his original attempt and:

https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch-robert

...which was an attempt to rebase on top of your bits.

The common RCRB mapping function is here:

https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/commit/?h=cxl-rch-robert&id=5be44cad37972517dae6a79001080ccfbdb67c49

I think the path forward is to build on that common RCRB code, fix
cxl_acpi to register the pci host bridge device instead of the APCI
device as the dport device, and then rely on a flag to skip over
devm_enumerate_cxl_ports() in favor of just calling cxl_mem_find_port()
directly in the RCIEP / RCH case. Then we can figure out what to do
about RCDs that choose not to implement the HDM decoder capability which
was forbidden in CXL 2.0, but now allowed in CXL 3.0.

Re: [PATCH v2 06/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Robert Richter]

On 24.10.22 16:23:39, Dave Jiang wrote:
> On 10/24/2022 3:37 PM, Dan Williams wrote:
>     Dan Williams wrote:
>         Robert Richter wrote:

>             Ok, I see where to go here. Could you point me to Dave's postings you
>             are referring to? I checked linux-cxl and could not find anything
>             related to RCRB or that changes regs.c.
> 
>         He was in the middle of tidying them when you posted your series, but I
>         think it would not hurt to push them to a git tree so you can grab the
>         bits and pieces you want.
> 
>         Dave?
> 
>     Looks like the list delivery is backed up, so I added Dave to the Cc:.
> 
>     He pushed:
> 
>     https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch
> 
>     ...which was his original attempt and:
> 
>     https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch-robert
> 
>     ...which was an attempt to rebase on top of your bits.
> 
>     The common RCRB mapping function is here:
> 
>     https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/commit/?h=cxl-rch-robert&id=5be44cad37972517dae6a79001080ccfbdb67c49

Thanks for the pointers.

> 
>     I think the path forward is to build on that common RCRB code, fix
>     cxl_acpi to register the pci host bridge device instead of the APCI
>     device as the dport device, and then rely on a flag to skip over
>     devm_enumerate_cxl_ports() in favor of just calling cxl_mem_find_port()
>     directly in the RCIEP / RCH case.

Yes, we can completely skip devm_enumerate_cxl_ports() now. Though, I
am not convinced on using the pci host bridge as dport_dev as RCD and
non-RCD mode will diverge too much then. Looking into details here.

>     Then we can figure out what to do
>     about RCDs that choose not to implement the HDM decoder capability which
>     was forbidden in CXL 2.0, but now allowed in CXL 3.0.
> 
> Hi Robert. As follow on to your work, I'm also working on reworking the hdm
> decoder enumeration. I have this table from Dan where rr - range register exist
> but not setup, RR - range register exist and setup, hdm - HDM decoder exist but
> not programmed, HDM - HDM decoders exist and programmed. And I'm trying to
> refactor the current code to cover all those scenarios:
> 
>          rr             RR               rr hdm          rr HDM          RR hdm         RR HDM
> D2       unsupported    emulate RR       enable HDM      keep HDM        enable HDM     keep HDM
> D1       unsupported    emulate RR       enable HDM      keep HDM        enable HDM     keep HDM
> 
> The current test device I have that's attached to RCH host, I'm seeing the RR
> has setup a single range, but none of the HDM decoders are programmed.
> 

Right, HDM decoder init need to be changed next.

Thanks,

-Robert

Re: [PATCH v2 06/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Dan Williams]

Robert Richter wrote:
> On 24.10.22 16:23:39, Dave Jiang wrote:
> > On 10/24/2022 3:37 PM, Dan Williams wrote:
> >     Dan Williams wrote:
> >         Robert Richter wrote:
> 
> >             Ok, I see where to go here. Could you point me to Dave's postings you
> >             are referring to? I checked linux-cxl and could not find anything
> >             related to RCRB or that changes regs.c.
> > 
> >         He was in the middle of tidying them when you posted your series, but I
> >         think it would not hurt to push them to a git tree so you can grab the
> >         bits and pieces you want.
> > 
> >         Dave?
> > 
> >     Looks like the list delivery is backed up, so I added Dave to the Cc:.
> > 
> >     He pushed:
> > 
> >     https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch
> > 
> >     ...which was his original attempt and:
> > 
> >     https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch-robert
> > 
> >     ...which was an attempt to rebase on top of your bits.
> > 
> >     The common RCRB mapping function is here:
> > 
> >     https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/commit/?h=cxl-rch-robert&id=5be44cad37972517dae6a79001080ccfbdb67c49
> 
> Thanks for the pointers.
> 
> > 
> >     I think the path forward is to build on that common RCRB code, fix
> >     cxl_acpi to register the pci host bridge device instead of the APCI
> >     device as the dport device, and then rely on a flag to skip over
> >     devm_enumerate_cxl_ports() in favor of just calling cxl_mem_find_port()
> >     directly in the RCIEP / RCH case.
> 
> Yes, we can completely skip devm_enumerate_cxl_ports() now. Though, I
> am not convinced on using the pci host bridge as dport_dev as RCD and
> non-RCD mode will diverge too much then. Looking into details here.

Oh, I disagree with the initial implementation Dave had here. Both cases
should be specifying the bridge device as the dport. That's a fixup that
can go in now even without the RCD support.

As it is the tooling needs to jump through the physical_node attribute
to provide the useful information in cxl list:

# cxl list -BTu -b ACPI.CXL
{
  "bus":"root0",
  "provider":"ACPI.CXL",
  "nr_dports":1,
  "dports":[
    {
      "dport":"ACPI0016:00",
      "alias":"pci0000:34",
      "id":"0x34"
    }
  ]
}

...and I think that should just swap to this in all cases:

# cxl list -BTu -b ACPI.CXL
{
  "bus":"root0",
  "provider":"ACPI.CXL",
  "nr_dports":1,
  "dports":[
    {
      "dport":"pci0000:34",
      "alias":"ACPI0016:00",
      "id":"0x34"
    }
  ]
}

Re: [PATCH v2 07/12] cxl: Remove dev_is_cxl_root_child() check in devm_cxl_enumerate_ports()

[Robert Richter]

Vishal,

On 21.10.22 06:32:33, Verma, Vishal L wrote:
> I was waiting to post the cxl-cli patches for this until the kernel
> discussion settles - but maybe this topology layout is settled enough?
> 
> In the meanwhile I've pushed a branch with these fixups here:
> https://github.com/pmem/ndctl/tree/vv/rch-support

I sent out the v3 series today, patch #1 contains the change of the
port's reference device. You need to adopt the ndctl patches for this.
To find the alias in sysfs, you need to lookup the firmware_node
instead of the physical_node. I would suggest to change the
implemenation to just check for both entries.

Thanks,

-Robert

[PATCH] tools/testing/cxl: Add bridge mocking support

[Dan Williams]

In preparation for cxl_acpi walking pci_root->bus->bridge, add that
association to the mock pci_root instances.

Note that the missing 3rd entry in mock_pci_root[] was not noticed until
now given that the test version of to_cxl_host_bridge()
(tools/testing/cxl/mock_acpi.c), obviated the need for that entry.
However, "cxl/acpi: Improve debug messages in cxl_acpi_probe()" [1]
needs pci_root->bus->bridge to be populated.

Link: https://lore.kernel.org/r/20221018132341.76259-6-rrichter@amd.com [1]
Cc: Robert Richter <rrichter@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 tools/testing/cxl/test/cxl.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index a5146d80ecc4..facfcd11cb67 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -439,14 +439,18 @@ mock_acpi_evaluate_integer(acpi_handle handle, acpi_string pathname,
 	return AE_OK;
 }
 
-static struct pci_bus mock_pci_bus[NR_CXL_HOST_BRIDGES];
-static struct acpi_pci_root mock_pci_root[NR_CXL_HOST_BRIDGES] = {
+static struct pci_bus mock_pci_bus[NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST];
+static struct acpi_pci_root mock_pci_root[ARRAY_SIZE(mock_pci_bus)] = {
 	[0] = {
 		.bus = &mock_pci_bus[0],
 	},
 	[1] = {
 		.bus = &mock_pci_bus[1],
 	},
+	[2] = {
+		.bus = &mock_pci_bus[2],
+	},
+
 };
 
 static bool is_mock_bus(struct pci_bus *bus)
@@ -744,6 +748,7 @@ static __init int cxl_single_init(void)
 		}
 
 		cxl_hb_single[i] = pdev;
+		mock_pci_bus[i + NR_CXL_HOST_BRIDGES].bridge = &pdev->dev;
 		rc = sysfs_create_link(&pdev->dev.kobj, &pdev->dev.kobj,
 				       "physical_node");
 		if (rc)
@@ -910,6 +915,7 @@ static __init int cxl_test_init(void)
 		}
 
 		cxl_host_bridge[i] = pdev;
+		mock_pci_bus[i].bridge = &pdev->dev;
 		rc = sysfs_create_link(&pdev->dev.kobj, &pdev->dev.kobj,
 				       "physical_node");
 		if (rc)

Re: [PATCH] tools/testing/cxl: Add bridge mocking support

[Robert Richter]

On 14.11.22 12:11:13, Dan Williams wrote:
> In preparation for cxl_acpi walking pci_root->bus->bridge, add that
> association to the mock pci_root instances.
> 
> Note that the missing 3rd entry in mock_pci_root[] was not noticed until
> now given that the test version of to_cxl_host_bridge()
> (tools/testing/cxl/mock_acpi.c), obviated the need for that entry.
> However, "cxl/acpi: Improve debug messages in cxl_acpi_probe()" [1]
> needs pci_root->bus->bridge to be populated.

Patch looks good to me, though we might want to move the
mock_pci_root[] fix into e41c8452b9b2 or on top as a separate change.

For patch logistics, could you merge that changes into the rch tree as
a new base? Right now I have cherry picked e41c8452b9b2 and this one
as pre-requisites of my series.

-Robert

> 
> Link: https://lore.kernel.org/r/20221018132341.76259-6-rrichter@amd.com [1]
> Cc: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH] tools/testing/cxl: Add bridge mocking support

[Dan Williams]

Robert Richter wrote:
> On 14.11.22 12:11:13, Dan Williams wrote:
> > In preparation for cxl_acpi walking pci_root->bus->bridge, add that
> > association to the mock pci_root instances.
> > 
> > Note that the missing 3rd entry in mock_pci_root[] was not noticed until
> > now given that the test version of to_cxl_host_bridge()
> > (tools/testing/cxl/mock_acpi.c), obviated the need for that entry.
> > However, "cxl/acpi: Improve debug messages in cxl_acpi_probe()" [1]
> > needs pci_root->bus->bridge to be populated.
> 
> Patch looks good to me, though we might want to move the
> mock_pci_root[] fix into e41c8452b9b2 or on top as a separate change.
> 
> For patch logistics, could you merge that changes into the rch tree as
> a new base? Right now I have cherry picked e41c8452b9b2 and this one
> as pre-requisites of my series.

Yes, I re-rolled the series on -rc4. Will post a new baseline once I
have the unit test topology ready later today.

Re: [PATCH] tools/testing/cxl: Add bridge mocking support

[Jonathan Cameron]

On Mon, 14 Nov 2022 12:11:13 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> In preparation for cxl_acpi walking pci_root->bus->bridge, add that
> association to the mock pci_root instances.
> 
> Note that the missing 3rd entry in mock_pci_root[] was not noticed until
> now given that the test version of to_cxl_host_bridge()
> (tools/testing/cxl/mock_acpi.c), obviated the need for that entry.
> However, "cxl/acpi: Improve debug messages in cxl_acpi_probe()" [1]
> needs pci_root->bus->bridge to be populated.
> 
> Link: https://lore.kernel.org/r/20221018132341.76259-6-rrichter@amd.com [1]
> Cc: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  tools/testing/cxl/test/cxl.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index a5146d80ecc4..facfcd11cb67 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -439,14 +439,18 @@ mock_acpi_evaluate_integer(acpi_handle handle, acpi_string pathname,
>  	return AE_OK;
>  }
>  
> -static struct pci_bus mock_pci_bus[NR_CXL_HOST_BRIDGES];
> -static struct acpi_pci_root mock_pci_root[NR_CXL_HOST_BRIDGES] = {
> +static struct pci_bus mock_pci_bus[NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST];
> +static struct acpi_pci_root mock_pci_root[ARRAY_SIZE(mock_pci_bus)] = {
>  	[0] = {
>  		.bus = &mock_pci_bus[0],
>  	},
>  	[1] = {
>  		.bus = &mock_pci_bus[1],
>  	},
> +	[2] = {
> +		.bus = &mock_pci_bus[2],
> +	},
> +
Ahah. Here's the stray space that gets cleaned up in the RCH series.  Guessing you
don't want to rebase to fix it here? 

>  };