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. In RCD mode an ACPI0017 device
might not necessarily exist and the host bridge can have a standard
PCIe host bridge PNP0A08 ID, there aren't any CXL port or switches in
the PCIe hierarchy visible. As such the RCD mode enumeration and host
discovery is very different from CXL VH. See patch #5 for
implementation details.
This implementation 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 #4 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 #5 to #15 contain
the actual implementation of RCD mode support.
[1] https://www.computeexpresslink.org/spec-landing
Robert Richter (15):
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: Add probe function to detect restricted CXL hosts in RCD
mode
PCI/ACPI: Link host bridge to its ACPI fw node
cxl/acpi: Check RCH's PCIe Host Bridge ACPI ID
cxl/acpi: Check RCH's CXL DVSEC capabilities
cxl/acpi: Determine PCI host bridge's ACPI UID
cxl/acpi: Extract the RCH's RCRB base address from CEDT
cxl/acpi: Extract the host's component register base address from RCRB
cxl/acpi: Skip devm_cxl_port_enumerate_dports() when in RCD mode
cxl/acpi: Rework devm_cxl_enumerate_ports() to support RCD mode
cxl/acpi: Enumerate ports in RCD mode to enable RCHs and RCDs
cxl/acpi: Specify module load order dependency for the cxl_acpi module
drivers/acpi/pci_root.c | 1 +
drivers/cxl/acpi.c | 311 ++++++++++++++++++++++++++++++++++-
drivers/cxl/core/pci.c | 22 ++-
drivers/cxl/core/port.c | 103 ++++++++----
drivers/cxl/core/regs.c | 3 +
drivers/cxl/cxl.h | 2 -
drivers/cxl/mem.c | 1 +
tools/testing/cxl/test/cxl.c | 8 +-
8 files changed, 400 insertions(+), 51 deletions(-)
--
2.30.2
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.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/acpi.c | 2 --
drivers/cxl/core/port.c | 39 ++++++++++++++++++++++++++++-----------
2 files changed, 28 insertions(+), 13 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..8604cda88787 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -666,13 +666,17 @@ 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 cxl_port *port, *parent_port;
struct device *dev;
int rc;
+ parent_port = parent_dport ? parent_dport->port : NULL;
+
port = cxl_port_alloc(uport, component_reg_phys, parent_dport);
- if (IS_ERR(port))
- return port;
+ if (IS_ERR(port)) {
+ rc = PTR_ERR(port);
+ goto err_out;
+ }
dev = &port->dev;
if (is_cxl_memdev(uport))
@@ -682,24 +686,39 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
else
rc = dev_set_name(dev, "root%d", port->id);
if (rc)
- goto err;
+ goto err_put;
rc = device_add(dev);
if (rc)
- goto err;
+ goto err_put;
rc = devm_add_action_or_reset(host, unregister_port, port);
if (rc)
- return ERR_PTR(rc);
+ goto err_out;
rc = devm_cxl_link_uport(host, port);
if (rc)
- return ERR_PTR(rc);
+ goto err_out;
- return port;
+ dev_dbg(host, "added %s as%s port of device %s%s%s\n",
+ dev_name(&port->dev),
+ parent_port ? "" : " root",
+ dev_name(uport),
+ parent_port ? " to parent port " : "",
+ parent_port ? dev_name(&parent_port->dev) : "");
-err:
+ return port;
+err_put:
put_device(dev);
+err_out:
+ dev_dbg(host, "failed to add %s as%s port of device %s%s%s: %d\n",
+ dev_name(&port->dev),
+ parent_port ? "" : " root",
+ dev_name(uport),
+ parent_port ? " to parent port " : "",
+ parent_port ? dev_name(&parent_port->dev) : "",
+ rc);
+
return ERR_PTR(rc);
}
EXPORT_SYMBOL_NS_GPL(devm_cxl_add_port, CXL);
@@ -1140,8 +1159,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
A downstream port must be connected to a component register block.
Determine its base address from the RCRB.
The implementation is analog to how cxl_setup_regs() is implemented
for CXL VH mode. A struct cxl_component_reg_map is filled in, mapped
and probed.
Signed-off-by: Terry Bowman <[email protected]>
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/acpi.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 80 insertions(+)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 439df9df2741..88bbd2bb61fc 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -401,12 +401,84 @@ static resource_size_t cxl_get_rcrb(u32 uid)
return ctx.chbcr;
}
+static resource_size_t cxl_get_component_reg_phys(resource_size_t rcrb)
+{
+ resource_size_t component_reg_phys;
+ u32 bar0, bar1;
+ void *addr;
+
+ /*
+ * RCRB's BAR[0..1] point to component block containing CXL subsystem
+ * component registers.
+ * CXL 8.2.4 - Component Register Layout Definition.
+ *
+ * Also, RCRB accesses must use MMIO readl()/readq() to guarantee
+ * 32/64-bit access.
+ * CXL 8.2.2 - CXL 1.1 Upstream and Downstream Port Subsystem Component
+ * Registers
+ */
+ 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 & (SZ_8K - 1))
+ return CXL_RESOURCE_NONE;
+
+ return component_reg_phys;
+}
+
+static int cxl_setup_component_reg(struct device *parent,
+ resource_size_t component_reg_phys)
+{
+ struct cxl_component_reg_map comp_map;
+ void __iomem *base;
+
+ if (component_reg_phys == CXL_RESOURCE_NONE)
+ return -EINVAL;
+
+ base = ioremap(component_reg_phys, SZ_64K);
+ if (!base) {
+ dev_err(parent, "failed to map registers\n");
+ return -ENOMEM;
+ }
+
+ cxl_probe_component_regs(parent, base, &comp_map);
+ iounmap(base);
+
+ if (!comp_map.hdm_decoder.valid) {
+ dev_err(parent, "HDM decoder registers not found\n");
+ return -ENXIO;
+ }
+
+ dev_dbg(parent, "Set up component registers\n");
+
+ return 0;
+}
+
static int __init cxl_restricted_host_probe(struct platform_device *pdev)
{
struct pci_host_bridge *host = NULL;
struct acpi_device *adev;
unsigned long long uid = ~0;
resource_size_t rcrb;
+ resource_size_t component_reg_phys;
+ int rc;
while ((host = cxl_find_next_rch(host)) != NULL) {
adev = ACPI_COMPANION(&host->dev);
@@ -425,10 +497,18 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
dev_dbg(&host->dev, "RCRB found: 0x%08llx\n", (u64)rcrb);
+ component_reg_phys = cxl_get_component_reg_phys(rcrb);
+ rc = cxl_setup_component_reg(&host->dev, component_reg_phys);
+ if (rc)
+ goto fail;
+
dev_info(&host->dev, "host supports CXL\n");
}
return 0;
+fail:
+ dev_err(&host->dev, "failed to initialize CXL host: %d\n", rc);
+ return rc;
}
static struct lock_class_key cxl_root_key;
--
2.30.2
RCD mode has a different enumeration scheme other than in CXL VH mode.
An RCD is directly connected to an RCH without downstream and upstream
ports showing up in between in the PCI hierarchy. Due to the direct
connection of RCD and RCH, the host bridge is always the RCD's parent
instead of the grandparent. Modify devm_cxl_enumerate_ports()
respectively.
Implement this by introducing a function to determine the device's
downstream port. The 'for' loop is adjusted for RCD mode and in this
case find_cxl_port() will always find the host's associated port and
the loop iteration stops.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/core/port.c | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 61e9915162d5..08b99423dbf8 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1084,6 +1084,22 @@ static struct device *grandparent(struct device *dev)
return NULL;
}
+static struct device *cxl_mem_dport_dev(struct cxl_memdev *cxlmd)
+{
+ struct device *dev = cxlmd->dev.parent;
+ struct pci_dev *pdev = to_pci_dev(cxlmd->dev.parent);
+
+ /*
+ * An RCiEP is directly connected to the root bridge without
+ * any PCI bridges/ports in between. Reduce the parent level
+ * for those.
+ */
+ if (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END)
+ return dev;
+
+ return dev->parent;
+}
+
static void delete_endpoint(void *data)
{
struct cxl_memdev *cxlmd = data;
@@ -1339,7 +1355,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
{
struct device *dev = &cxlmd->dev;
- struct device *iter;
+ struct device *dport_dev;
int rc;
rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd);
@@ -1352,25 +1368,21 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
* attempt fails.
*/
retry:
- for (iter = dev; iter; iter = grandparent(iter)) {
- struct device *dport_dev = grandparent(iter);
+ for (dport_dev = cxl_mem_dport_dev(cxlmd); dport_dev;
+ dport_dev = grandparent(dport_dev)) {
struct device *uport_dev;
struct cxl_dport *dport;
struct cxl_port *port;
- if (!dport_dev)
- return 0;
-
uport_dev = dport_dev->parent;
if (!uport_dev) {
- dev_warn(dev, "at %s no parent for dport: %s\n",
- dev_name(iter), dev_name(dport_dev));
+ dev_warn(dev, "no parent for dport: %s\n",
+ dev_name(dport_dev));
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));
+ dev_dbg(dev, "scan: dport_dev: %s parent: %s\n",
+ dev_name(dport_dev), dev_name(uport_dev));
port = find_cxl_port(dport_dev, &dport);
if (port) {
dev_dbg(&cxlmd->dev,
@@ -1418,7 +1430,7 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_ports, CXL);
struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
struct cxl_dport **dport)
{
- return find_cxl_port(grandparent(&cxlmd->dev), dport);
+ return find_cxl_port(cxl_mem_dport_dev(cxlmd), dport);
}
EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL);
--
2.30.2
RCD mode has a different enumeration scheme other than in CXL VH mode.
An RCD is directly connected to an RCH without downstream and upstream
ports showing up in between in the PCI hierarchy. Skip dport
enumeration for RCHs. Upstream and downstream ports of RCH and RCD
will be setup separately in a later patch.
Introduce the function is_rch_uport() to detect an RCH port. For RCHs
the parent root port is not the "ACPI0017" device and instead does not
have a fw node connected to it.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/core/pci.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 0dbbe8d39b07..86ed112eb262 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -65,6 +65,15 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
return 0;
}
+/*
+ * A parent of an RCH (CXL 1.1 host) is a plain platform device while
+ * a 2.0 host links to the ACPI0017 root device.
+ */
+static inline bool is_rch_uport(struct cxl_port *port)
+{
+ return is_cxl_port(&port->dev) && !port->dev.parent->fwnode;
+}
+
/**
* devm_cxl_port_enumerate_dports - enumerate downstream ports of the upstream port
* @port: cxl_port whose ->uport is the upstream of dports to be enumerated
@@ -74,10 +83,19 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
*/
int devm_cxl_port_enumerate_dports(struct cxl_port *port)
{
- struct pci_bus *bus = cxl_port_to_pci_bus(port);
+ struct pci_bus *bus;
struct cxl_walk_context ctx;
int type;
+ /*
+ * Skip enumeration in Restricted CXL Device mode as the
+ * device has been already registered at the host's dport
+ * during host discovery.
+ */
+ if (is_rch_uport(port))
+ return 0;
+
+ bus = cxl_port_to_pci_bus(port);
if (!bus)
return -ENXIO;
--
2.30.2
An RCH is a root bridge and has "PNP0A08" or "ACPI0016" ACPI ID set.
Check this.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/acpi.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index a19e3154dd44..ffdf439adb87 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -312,9 +312,16 @@ static int add_root_nvdimm_bridge(struct device *match, void *data)
return 1;
}
+static const struct acpi_device_id cxl_host_ids[] = {
+ { "ACPI0016", 0 },
+ { "PNP0A08", 0 },
+ { },
+};
+
struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
{
struct pci_bus *bus = host ? host->bus : NULL;
+ struct acpi_device *adev;
while ((bus = pci_find_next_bus(bus)) != NULL) {
host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
@@ -323,6 +330,19 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
dev_dbg(&host->dev, "PCI bridge found\n");
+ /* Must be a root bridge */
+ if (host->bus->parent)
+ continue;
+
+ dev_dbg(&host->dev, "PCI bridge is root bridge\n");
+
+ adev = ACPI_COMPANION(&host->dev);
+ if (acpi_match_device_ids(adev, cxl_host_ids))
+ continue;
+
+ dev_dbg(&host->dev, "PCI ACPI host found: %s\n",
+ acpi_dev_name(adev));
+
return host;
}
--
2.30.2
The downstream and upstream port Root Complex Register Blocks (RCRBs)
are needed to control the ports and CXL devices connected to it. It
also includes the location of the RCH/RCD downstream and upstream port
component registers in MEMBAR0. Extract the RCRB from the host's CEDT
entry.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/acpi.c | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index b3146b7ae922..439df9df2741 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -365,11 +365,48 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
return NULL;
}
+static int __cxl_get_rcrb(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)
+ return 0;
+
+ chbs = (struct acpi_cedt_chbs *)header;
+
+ if (ctx->uid != chbs->uid)
+ return 0;
+
+ if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11)
+ return 0;
+
+ if (chbs->length != SZ_8K)
+ return 0;
+
+ ctx->chbcr = chbs->base;
+
+ return 0;
+}
+
+static resource_size_t cxl_get_rcrb(u32 uid)
+{
+ struct cxl_chbs_context ctx = {
+ .uid = uid,
+ };
+
+ acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, __cxl_get_rcrb, &ctx);
+
+ return ctx.chbcr;
+}
+
static int __init cxl_restricted_host_probe(struct platform_device *pdev)
{
struct pci_host_bridge *host = NULL;
struct acpi_device *adev;
unsigned long long uid = ~0;
+ resource_size_t rcrb;
while ((host = cxl_find_next_rch(host)) != NULL) {
adev = ACPI_COMPANION(&host->dev);
@@ -382,6 +419,12 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
if (uid > U32_MAX)
continue;
+ rcrb = cxl_get_rcrb(uid);
+ if (!rcrb)
+ continue;
+
+ dev_dbg(&host->dev, "RCRB found: 0x%08llx\n", (u64)rcrb);
+
dev_info(&host->dev, "host supports CXL\n");
}
--
2.30.2
In RCD mode the CXL mem dev may be detected on the PCI bus before a
CXL host is brought up. This may cause a CXL mem initialization
failure as it expects the CXL host already detected. Address this by
specifying the module dependencies using MODULE_SOFTDEP().
The following additional dependencies exist:
* cxl_mem depends on cxl_acpi: The CXL hosts must be discovered
before the CXL device is initialized.
* cxl_acpi depends on cxl_port: The acpi driver adds ports to the cxl
bus, the port driver should be loaded before. This might also work
if modules are loaded in different order, but a) it aligns with the
existing cxl_mem/cxl_port softdep and b) it always guarantees a fix
module load order.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/acpi.c | 1 +
drivers/cxl/mem.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 56b2d222afcc..63a1cb295c07 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -834,3 +834,4 @@ module_exit(cxl_host_exit);
MODULE_LICENSE("GPL v2");
MODULE_IMPORT_NS(CXL);
MODULE_IMPORT_NS(ACPI);
+MODULE_SOFTDEP("pre: cxl_port");
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 64ccf053d32c..ae13ec7d9894 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -128,3 +128,4 @@ MODULE_ALIAS_CXL(CXL_DEVICE_MEMORY_EXPANDER);
* endpoint registration.
*/
MODULE_SOFTDEP("pre: cxl_port");
+MODULE_SOFTDEP("pre: cxl_acpi");
--
2.30.2
Restricted CXL device (RCD) mode (formerly CXL 1.1) uses a different
enumeration scheme other than CXL VH (formerly CXL 2.0). In RCD mode a
host/device (RCH-RCD) pair shows up as a legal PCIe hierarchy with an
ACPI host bridge ("PNP0A08" or "ACPI0016" HID) and RCiEP connected to
it with a description of the CXL device.
Add function cxl_restricted_host_probe() to probe RCD enumerated
devices. The function implements a loop that detects all CXL capable
ACPI PCI root bridges in the system (RCD mode only). The iterator
function cxl_find_next_rch() is introduced to walk through all of the
CXL hosts. The loop will then enable all CXL devices connected to the
host. For now, only implement an empty loop with an iterator that
returns all pci host bridges in the system.
The probe function is triggered by adding an own root device for RCHs.
This is different to CXL VH where an ACPI "ACPI0017" root device
exists. Its detection starts the CXL host detection. In RCD mode such
a device does not necessarily exists, so solve this by creating a
plain platform device that is not an ACPI device and is root only for
RCHs.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/acpi.c | 71 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 70 insertions(+), 1 deletion(-)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 31e104f0210f..a19e3154dd44 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -312,6 +312,33 @@ static int add_root_nvdimm_bridge(struct device *match, void *data)
return 1;
}
+struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
+{
+ struct pci_bus *bus = host ? host->bus : NULL;
+
+ while ((bus = pci_find_next_bus(bus)) != NULL) {
+ host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
+ if (!host)
+ continue;
+
+ dev_dbg(&host->dev, "PCI bridge found\n");
+
+ return host;
+ }
+
+ return NULL;
+}
+
+static int __init cxl_restricted_host_probe(struct platform_device *pdev)
+{
+ struct pci_host_bridge *host = NULL;
+
+ while ((host = cxl_find_next_rch(host)) != NULL) {
+ }
+
+ return 0;
+}
+
static struct lock_class_key cxl_root_key;
static void cxl_acpi_lock_reset_class(void *dev)
@@ -445,6 +472,13 @@ static int cxl_acpi_probe(struct platform_device *pdev)
struct acpi_device *adev = ACPI_COMPANION(host);
struct cxl_cfmws_context ctx;
+ /*
+ * For RCH (CXL 1.1 hosts) the probe is triggered by a plain
+ * platform dev which does not have an acpi companion.
+ */
+ if (!adev)
+ return cxl_restricted_host_probe(pdev);
+
device_lock_set_class(&pdev->dev, &cxl_root_key);
rc = devm_add_action_or_reset(&pdev->dev, cxl_acpi_lock_reset_class,
&pdev->dev);
@@ -518,6 +552,7 @@ MODULE_DEVICE_TABLE(acpi, cxl_acpi_ids);
static const struct platform_device_id cxl_test_ids[] = {
{ "cxl_acpi" },
+ { "cxl_root" },
{ },
};
MODULE_DEVICE_TABLE(platform, cxl_test_ids);
@@ -531,7 +566,41 @@ static struct platform_driver cxl_acpi_driver = {
.id_table = cxl_test_ids,
};
-module_platform_driver(cxl_acpi_driver);
+static void cxl_acpi_device_release(struct device *dev) { }
+
+static struct platform_device cxl_acpi_device = {
+ .name = "cxl_root",
+ .id = PLATFORM_DEVID_NONE,
+ .dev = {
+ .release = cxl_acpi_device_release,
+ }
+};
+
+static int __init cxl_host_init(void)
+{
+ int rc;
+
+ /* Kick off restricted host (CXL 1.1) detection */
+ rc = platform_device_register(&cxl_acpi_device);
+ if (rc) {
+ platform_device_put(&cxl_acpi_device);
+ return rc;
+ }
+ rc = platform_driver_register(&cxl_acpi_driver);
+ if (rc)
+ platform_device_unregister(&cxl_acpi_device);
+ return rc;
+}
+
+static void __exit cxl_host_exit(void)
+{
+ platform_driver_unregister(&cxl_acpi_driver);
+ platform_device_unregister(&cxl_acpi_device);
+}
+
+module_init(cxl_host_init);
+module_exit(cxl_host_exit);
+
MODULE_LICENSE("GPL v2");
MODULE_IMPORT_NS(CXL);
MODULE_IMPORT_NS(ACPI);
--
2.30.2
Do the plumbing of ports to enable RCD/RCH pairs.
Do this by enumerating all necessary ports an endpoint needs to
connect to. This includes:
1) A CXL root port with dport links to the RCHs. The port links to
the CXL root platform device for RCH mode.
2) RCH ports with dport links to its endpoints. Port connects to the
pci host bridge device.
3) CXL device endpoint connected to the RCH.
The port creation for the endpoint (3) is already implemented and
works in RCD mode too. Thus, it is not scope of this patch. Only the
endpoints must be registered at the host bridge port.
Implement this by introducing the function cxl_enumerate_rch_ports().
It registers a CXL host at the CXL root device, creates the host's
port and registers the existing CXL memory device endpoint at it. The
port of the CXL root device is created with the first CXL host being
registered.
Once enumerated, CXL restricted hosts show up in sysfs with CXL
devices connected as endpoints to it.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/acpi.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 88bbd2bb61fc..56b2d222afcc 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -471,13 +471,52 @@ static int cxl_setup_component_reg(struct device *parent,
return 0;
}
+static int cxl_enumerate_rch_ports(struct device *root_dev,
+ struct cxl_port *cxl_root,
+ struct pci_host_bridge *host,
+ resource_size_t component_reg_phys,
+ int port_id)
+{
+ struct cxl_dport *dport;
+ struct cxl_port *port;
+ struct pci_dev *pdev;
+
+ dport = devm_cxl_add_dport(cxl_root, &host->dev, port_id,
+ component_reg_phys);
+ if (IS_ERR(dport))
+ return PTR_ERR(dport);
+
+ port = devm_cxl_add_port(root_dev, &host->dev,
+ component_reg_phys, dport);
+ if (IS_ERR(port))
+ return PTR_ERR(port);
+
+ pdev = pci_get_slot(host->bus, PCI_DEVFN(0, 0));
+ if (!pdev)
+ return -ENXIO;
+
+ /* Note: The endpoint provides the component reg base. */
+ dport = devm_cxl_add_dport(port, &pdev->dev, 0,
+ CXL_RESOURCE_NONE);
+
+ pci_dev_put(pdev);
+
+ if (IS_ERR(dport))
+ return PTR_ERR(dport);
+
+ return 0;
+}
+
static int __init cxl_restricted_host_probe(struct platform_device *pdev)
{
+ struct device *root_dev = &pdev->dev;
struct pci_host_bridge *host = NULL;
struct acpi_device *adev;
+ struct cxl_port *cxl_root = NULL;
unsigned long long uid = ~0;
resource_size_t rcrb;
resource_size_t component_reg_phys;
+ int port_id = 0;
int rc;
while ((host = cxl_find_next_rch(host)) != NULL) {
@@ -497,11 +536,30 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
dev_dbg(&host->dev, "RCRB found: 0x%08llx\n", (u64)rcrb);
+ /*
+ * For CXL 1.1 hosts we create a root device other
+ * than the ACPI0017 device to hold the devm data and
+ * the uport ref.
+ */
+ if (!cxl_root) {
+ cxl_root = devm_cxl_add_port(root_dev, root_dev,
+ CXL_RESOURCE_NONE, NULL);
+ if (IS_ERR(cxl_root)) {
+ rc = PTR_ERR(cxl_root);
+ goto fail;
+ }
+ }
+
component_reg_phys = cxl_get_component_reg_phys(rcrb);
rc = cxl_setup_component_reg(&host->dev, component_reg_phys);
if (rc)
goto fail;
+ rc = cxl_enumerate_rch_ports(root_dev, cxl_root, host,
+ component_reg_phys, port_id++);
+ if (rc)
+ goto fail;
+
dev_info(&host->dev, "host supports CXL\n");
}
--
2.30.2
A lookup of a host bridge's corresponding acpi device (struct
acpi_device) is not possible, for example:
adev = ACPI_COMPANION(&host_bridge->dev);
This could be useful to find a host bridge's fwnode handle and to
determine and call additional host bridge ACPI parameters and methods
such as HID/CID or _UID.
Make this work by linking the host bridge to its ACPI fw node.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/acpi/pci_root.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index d57cf8454b93..846c979e4c29 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -1083,6 +1083,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
goto out_release_info;
host_bridge = to_pci_host_bridge(bus->bridge);
+ host_bridge->dev.fwnode = acpi_fwnode_handle(device);
if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
host_bridge->native_pcie_hotplug = 0;
if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
--
2.30.2
The UID is needed to read the RCH's CEDT entry with the RCRB base
address. Determine the host's UID from its ACPI fw node.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/acpi.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index f9cdf23a91a8..b3146b7ae922 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -368,8 +368,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
static int __init cxl_restricted_host_probe(struct platform_device *pdev)
{
struct pci_host_bridge *host = NULL;
+ struct acpi_device *adev;
+ unsigned long long uid = ~0;
while ((host = cxl_find_next_rch(host)) != NULL) {
+ adev = ACPI_COMPANION(&host->dev);
+ if (!adev || !adev->pnp.unique_id ||
+ (kstrtoull(adev->pnp.unique_id, 10, &uid) < 0))
+ continue;
+
+ dev_dbg(&adev->dev, "host uid: %llu\n", uid);
+
+ if (uid > U32_MAX)
+ continue;
+
dev_info(&host->dev, "host supports CXL\n");
}
--
2.30.2
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.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/acpi.c | 7 ++-----
drivers/cxl/core/pci.c | 2 --
drivers/cxl/core/port.c | 28 ++++++++++++++++++++--------
tools/testing/cxl/test/cxl.c | 8 +-------
4 files changed, 23 insertions(+), 22 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 8604cda88787..61e9915162d5 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -914,12 +914,16 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
}
if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >=
- CXL_TARGET_STRLEN)
- return ERR_PTR(-EINVAL);
+ CXL_TARGET_STRLEN) {
+ rc = -EINVAL;
+ goto err;
+ }
dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
- if (!dport)
- return ERR_PTR(-ENOMEM);
+ if (!dport) {
+ rc = -ENOMEM;
+ goto err;
+ }
dport->dport = dport_dev;
dport->port_id = port_id;
@@ -930,22 +934,30 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
rc = add_dport(port, dport);
cond_cxl_root_unlock(port);
if (rc)
- return ERR_PTR(rc);
+ goto err;
get_device(dport_dev);
rc = devm_add_action_or_reset(host, cxl_dport_remove, dport);
if (rc)
- return ERR_PTR(rc);
+ goto err;
rc = sysfs_create_link(&port->dev.kobj, &dport_dev->kobj, link_name);
if (rc)
- return ERR_PTR(rc);
+ goto err;
rc = devm_add_action_or_reset(host, cxl_dport_unlink, dport);
if (rc)
- return ERR_PTR(rc);
+ goto err;
+
+ dev_dbg(&port->dev, "added %s (%s) as dport of device %s\n",
+ dev_name(&port->dev), link_name, dev_name(dport_dev));
return dport;
+err:
+ dev_dbg(&port->dev, "failed to add %s (%s) as dport of device %s: %d\n",
+ dev_name(&port->dev), link_name, dev_name(dport_dev), rc);
+
+ return ERR_PTR(rc);
}
EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, CXL);
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
An RCH has an RCiEP connected to it with CXL DVSEC capabilities
present and the CXL PCIe DVSEC included. Check this.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/acpi.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index ffdf439adb87..f9cdf23a91a8 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -322,6 +322,8 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
{
struct pci_bus *bus = host ? host->bus : NULL;
struct acpi_device *adev;
+ struct pci_dev *pdev;
+ bool is_restricted_host;
while ((bus = pci_find_next_bus(bus)) != NULL) {
host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
@@ -343,6 +345,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
dev_dbg(&host->dev, "PCI ACPI host found: %s\n",
acpi_dev_name(adev));
+ /* 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);
+ pci_dev_put(pdev);
+
+ if (!is_restricted_host)
+ continue;
+
+ dev_dbg(&host->dev, "CXL restricted host found\n");
+
return host;
}
@@ -354,6 +370,7 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
struct pci_host_bridge *host = NULL;
while ((host = cxl_find_next_rch(host)) != NULL) {
+ dev_info(&host->dev, "host supports CXL\n");
}
return 0;
--
2.30.2
The physical base address of a CXL range can be invalid and is then
set to CXL_RESOURCE_NONE. Early check this case before mapping a
memory block in devm_cxl_iomap_block().
Signed-off-by: Robert Richter <[email protected]>
---
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 39a129c57d40..f216c017a474 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -165,6 +165,9 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
void __iomem *ret_val;
struct resource *res;
+ if (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
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.
Signed-off-by: Robert Richter <[email protected]>
---
drivers/cxl/cxl.h | 2 --
1 file changed, 2 deletions(-)
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
On Wed, 31 Aug 2022 10:15:49 +0200
Robert Richter <[email protected]> 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.
>
> Signed-off-by: Robert Richter <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
The wonders of code evolution leaving signs behind ;)
> ---
> drivers/cxl/cxl.h | 2 --
> 1 file changed, 2 deletions(-)
>
> 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
Hi Robert,
I love your patch! Perhaps something to improve:
[auto build test WARNING on helgaas-pci/next]
[also build test WARNING on linus/master v6.0-rc3 next-20220830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Robert-Richter/cxl-Add-support-for-Restricted-CXL-hosts-RCD-mode/20220831-161955
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: alpha-allyesconfig
compiler: alpha-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/cb2fb876ae88418bdae58f974a4905b661792cc5
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Robert-Richter/cxl-Add-support-for-Restricted-CXL-hosts-RCD-mode/20220831-161955
git checkout cb2fb876ae88418bdae58f974a4905b661792cc5
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash drivers/cxl/core/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/cxl/core/regs.c:162:15: warning: no previous prototype for 'devm_cxl_iomap_block' [-Wmissing-prototypes]
162 | void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
| ^~~~~~~~~~~~~~~~~~~~
vim +/devm_cxl_iomap_block +162 drivers/cxl/core/regs.c
0f06157e0135f5 Dan Williams 2021-08-03 161
d17d0540a0dbf1 Dan Williams 2022-02-01 @162 void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
0f06157e0135f5 Dan Williams 2021-08-03 163 resource_size_t length)
0f06157e0135f5 Dan Williams 2021-08-03 164 {
0f06157e0135f5 Dan Williams 2021-08-03 165 void __iomem *ret_val;
0f06157e0135f5 Dan Williams 2021-08-03 166 struct resource *res;
0f06157e0135f5 Dan Williams 2021-08-03 167
0f06157e0135f5 Dan Williams 2021-08-03 168 res = devm_request_mem_region(dev, addr, length, dev_name(dev));
0f06157e0135f5 Dan Williams 2021-08-03 169 if (!res) {
0f06157e0135f5 Dan Williams 2021-08-03 170 resource_size_t end = addr + length - 1;
0f06157e0135f5 Dan Williams 2021-08-03 171
0f06157e0135f5 Dan Williams 2021-08-03 172 dev_err(dev, "Failed to request region %pa-%pa\n", &addr, &end);
0f06157e0135f5 Dan Williams 2021-08-03 173 return NULL;
0f06157e0135f5 Dan Williams 2021-08-03 174 }
0f06157e0135f5 Dan Williams 2021-08-03 175
0f06157e0135f5 Dan Williams 2021-08-03 176 ret_val = devm_ioremap(dev, addr, length);
0f06157e0135f5 Dan Williams 2021-08-03 177 if (!ret_val)
0f06157e0135f5 Dan Williams 2021-08-03 178 dev_err(dev, "Failed to map region %pr\n", res);
0f06157e0135f5 Dan Williams 2021-08-03 179
0f06157e0135f5 Dan Williams 2021-08-03 180 return ret_val;
0f06157e0135f5 Dan Williams 2021-08-03 181 }
0f06157e0135f5 Dan Williams 2021-08-03 182
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Wed, 31 Aug 2022 10:15:51 +0200
Robert Richter <[email protected]> wrote:
> 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.
>
> Signed-off-by: Robert Richter <[email protected]>
This is one for Dan etc as it is mostly a question of how verbose we want
the debug prints to be plus preference for caller or callee being
responsible for outputting this sort of message.
Patch looks good to me if we want to make this sort of change.
> ---
> drivers/cxl/acpi.c | 2 --
> drivers/cxl/core/port.c | 39 ++++++++++++++++++++++++++++-----------
> 2 files changed, 28 insertions(+), 13 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..8604cda88787 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -666,13 +666,17 @@ 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 cxl_port *port, *parent_port;
> struct device *dev;
> int rc;
>
> + parent_port = parent_dport ? parent_dport->port : NULL;
> +
> port = cxl_port_alloc(uport, component_reg_phys, parent_dport);
> - if (IS_ERR(port))
> - return port;
> + if (IS_ERR(port)) {
> + rc = PTR_ERR(port);
> + goto err_out;
> + }
>
> dev = &port->dev;
> if (is_cxl_memdev(uport))
> @@ -682,24 +686,39 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> else
> rc = dev_set_name(dev, "root%d", port->id);
> if (rc)
> - goto err;
> + goto err_put;
>
> rc = device_add(dev);
> if (rc)
> - goto err;
> + goto err_put;
>
> rc = devm_add_action_or_reset(host, unregister_port, port);
> if (rc)
> - return ERR_PTR(rc);
> + goto err_out;
>
> rc = devm_cxl_link_uport(host, port);
> if (rc)
> - return ERR_PTR(rc);
> + goto err_out;
>
> - return port;
> + dev_dbg(host, "added %s as%s port of device %s%s%s\n",
> + dev_name(&port->dev),
> + parent_port ? "" : " root",
> + dev_name(uport),
> + parent_port ? " to parent port " : "",
> + parent_port ? dev_name(&parent_port->dev) : "");
>
> -err:
> + return port;
> +err_put:
> put_device(dev);
> +err_out:
> + dev_dbg(host, "failed to add %s as%s port of device %s%s%s: %d\n",
> + dev_name(&port->dev),
> + parent_port ? "" : " root",
> + dev_name(uport),
> + parent_port ? " to parent port " : "",
> + parent_port ? dev_name(&parent_port->dev) : "",
> + rc);
> +
> return ERR_PTR(rc);
> }
> EXPORT_SYMBOL_NS_GPL(devm_cxl_add_port, CXL);
> @@ -1140,8 +1159,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;
On Wed, 31 Aug 2022 10:15:53 +0200
Robert Richter <[email protected]> wrote:
> Restricted CXL device (RCD) mode (formerly CXL 1.1) uses a different
> enumeration scheme other than CXL VH (formerly CXL 2.0). In RCD mode a
> host/device (RCH-RCD) pair shows up as a legal PCIe hierarchy with an
> ACPI host bridge ("PNP0A08" or "ACPI0016" HID) and RCiEP connected to
> it with a description of the CXL device.
>
> Add function cxl_restricted_host_probe() to probe RCD enumerated
> devices. The function implements a loop that detects all CXL capable
> ACPI PCI root bridges in the system (RCD mode only). The iterator
> function cxl_find_next_rch() is introduced to walk through all of the
> CXL hosts. The loop will then enable all CXL devices connected to the
> host. For now, only implement an empty loop with an iterator that
> returns all pci host bridges in the system.
>
> The probe function is triggered by adding an own root device for RCHs.
> This is different to CXL VH where an ACPI "ACPI0017" root device
> exists. Its detection starts the CXL host detection. In RCD mode such
> a device does not necessarily exists, so solve this by creating a
> plain platform device that is not an ACPI device and is root only for
> RCHs.
If I read this correctly that platform device is created whether or not
there are any cxl devices in the system?
Can we create it only if we find some devices that will be placed
under it later?
Jonathan
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/cxl/acpi.c | 71 +++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 31e104f0210f..a19e3154dd44 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -312,6 +312,33 @@ static int add_root_nvdimm_bridge(struct device *match, void *data)
> return 1;
> }
>
> +struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> +{
> + struct pci_bus *bus = host ? host->bus : NULL;
> +
> + while ((bus = pci_find_next_bus(bus)) != NULL) {
> + host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
> + if (!host)
> + continue;
> +
> + dev_dbg(&host->dev, "PCI bridge found\n");
> +
> + return host;
> + }
> +
> + return NULL;
> +}
> +
> +static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> +{
> + struct pci_host_bridge *host = NULL;
> +
> + while ((host = cxl_find_next_rch(host)) != NULL) {
> + }
> +
> + return 0;
> +}
> +
> static struct lock_class_key cxl_root_key;
>
> static void cxl_acpi_lock_reset_class(void *dev)
> @@ -445,6 +472,13 @@ static int cxl_acpi_probe(struct platform_device *pdev)
> struct acpi_device *adev = ACPI_COMPANION(host);
> struct cxl_cfmws_context ctx;
>
> + /*
> + * For RCH (CXL 1.1 hosts) the probe is triggered by a plain
> + * platform dev which does not have an acpi companion.
> + */
> + if (!adev)
> + return cxl_restricted_host_probe(pdev);
> +
> device_lock_set_class(&pdev->dev, &cxl_root_key);
> rc = devm_add_action_or_reset(&pdev->dev, cxl_acpi_lock_reset_class,
> &pdev->dev);
> @@ -518,6 +552,7 @@ MODULE_DEVICE_TABLE(acpi, cxl_acpi_ids);
>
> static const struct platform_device_id cxl_test_ids[] = {
> { "cxl_acpi" },
> + { "cxl_root" },
> { },
> };
> MODULE_DEVICE_TABLE(platform, cxl_test_ids);
> @@ -531,7 +566,41 @@ static struct platform_driver cxl_acpi_driver = {
> .id_table = cxl_test_ids,
> };
>
> -module_platform_driver(cxl_acpi_driver);
> +static void cxl_acpi_device_release(struct device *dev) { }
Why the empty release? Perhaps introduce this only when it
does something.
> +
> +static struct platform_device cxl_acpi_device = {
> + .name = "cxl_root",
> + .id = PLATFORM_DEVID_NONE,
> + .dev = {
> + .release = cxl_acpi_device_release,
> + }
> +};
> +
> +static int __init cxl_host_init(void)
> +{
> + int rc;
> +
> + /* Kick off restricted host (CXL 1.1) detection */
> + rc = platform_device_register(&cxl_acpi_device);
> + if (rc) {
> + platform_device_put(&cxl_acpi_device);
> + return rc;
> + }
> + rc = platform_driver_register(&cxl_acpi_driver);
> + if (rc)
> + platform_device_unregister(&cxl_acpi_device);
> + return rc;
> +}
> +
> +static void __exit cxl_host_exit(void)
> +{
> + platform_driver_unregister(&cxl_acpi_driver);
> + platform_device_unregister(&cxl_acpi_device);
> +}
> +
> +module_init(cxl_host_init);
> +module_exit(cxl_host_exit);
> +
> MODULE_LICENSE("GPL v2");
> MODULE_IMPORT_NS(CXL);
> MODULE_IMPORT_NS(ACPI);
On Wed, 31 Aug 2022 10:15:54 +0200
Robert Richter <[email protected]> wrote:
> A lookup of a host bridge's corresponding acpi device (struct
> acpi_device) is not possible, for example:
>
> adev = ACPI_COMPANION(&host_bridge->dev);
>
> This could be useful to find a host bridge's fwnode handle and to
> determine and call additional host bridge ACPI parameters and methods
> such as HID/CID or _UID.
>
> Make this work by linking the host bridge to its ACPI fw node.
>
> Signed-off-by: Robert Richter <[email protected]>
Seems sensible to me, though I'm not an expert in this are of the code.
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/acpi/pci_root.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index d57cf8454b93..846c979e4c29 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -1083,6 +1083,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> goto out_release_info;
>
> host_bridge = to_pci_host_bridge(bus->bridge);
> + host_bridge->dev.fwnode = acpi_fwnode_handle(device);
> if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> host_bridge->native_pcie_hotplug = 0;
> if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
On Wed, 31 Aug 2022 10:15:55 +0200
Robert Richter <[email protected]> wrote:
> An RCH is a root bridge and has "PNP0A08" or "ACPI0016" ACPI ID set.
> Check this.
>
> Signed-off-by: Robert Richter <[email protected]>
Hi Robert,
I'm a little uncomfortable with repurposing acpi_device_id
as you have done here. Might be better to do the same
as in pci_root.c where the matches are done more directly.
> ---
> drivers/cxl/acpi.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index a19e3154dd44..ffdf439adb87 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -312,9 +312,16 @@ static int add_root_nvdimm_bridge(struct device *match, void *data)
> return 1;
> }
>
> +static const struct acpi_device_id cxl_host_ids[] = {
> + { "ACPI0016", 0 },
> + { "PNP0A08", 0 },
> + { },
Trivial but no comma after a null terminator. Always good to make
it harder for people to add things where they really shouldn't!
pci_root.c avoids using an acpi_device_id table for similar matching.
I think the point being to separate probe type use of this table
from cases where we aren't using a normal device probe.
So to remain consistent with that, I would just grab the hid
and match it directly in this code.
I don't feel that strongly about this if the ACPI maintainers are
fine with reusing this infrastructure as you have it here.
> +};
> +
> struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> {
> struct pci_bus *bus = host ? host->bus : NULL;
> + struct acpi_device *adev;
>
> while ((bus = pci_find_next_bus(bus)) != NULL) {
> host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
> @@ -323,6 +330,19 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
>
> dev_dbg(&host->dev, "PCI bridge found\n");
>
> + /* Must be a root bridge */
> + if (host->bus->parent)
> + continue;
> +
> + dev_dbg(&host->dev, "PCI bridge is root bridge\n");
> +
> + adev = ACPI_COMPANION(&host->dev);
> + if (acpi_match_device_ids(adev, cxl_host_ids))
> + continue;
> +
> + dev_dbg(&host->dev, "PCI ACPI host found: %s\n",
> + acpi_dev_name(adev));
> +
> return host;
> }
>
On Wed, 31 Aug 2022 10:15:56 +0200
Robert Richter <[email protected]> wrote:
> An RCH has an RCiEP connected to it with CXL DVSEC capabilities
> present and the CXL PCIe DVSEC included. Check this.
>
> Signed-off-by: Robert Richter <[email protected]>
One comment inline. This looks good to me.
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/cxl/acpi.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index ffdf439adb87..f9cdf23a91a8 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -322,6 +322,8 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> {
> struct pci_bus *bus = host ? host->bus : NULL;
> struct acpi_device *adev;
> + struct pci_dev *pdev;
> + bool is_restricted_host;
>
> while ((bus = pci_find_next_bus(bus)) != NULL) {
> host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
> @@ -343,6 +345,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> dev_dbg(&host->dev, "PCI ACPI host found: %s\n",
> acpi_dev_name(adev));
>
> + /* Check CXL DVSEC of dev 0 func 0 */
So assumption here is that the hostbridge has a one or more RCiEPs.
The spec (r3.0 9.11.4) allows for the EP to appear behind a root port
- that case always felt odd to me, so I'm fine with not supporting it until
we see a user.
> + 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);
> + pci_dev_put(pdev);
> +
> + if (!is_restricted_host)
> + continue;
> +
> + dev_dbg(&host->dev, "CXL restricted host found\n");
> +
> return host;
> }
>
> @@ -354,6 +370,7 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> struct pci_host_bridge *host = NULL;
>
> while ((host = cxl_find_next_rch(host)) != NULL) {
> + dev_info(&host->dev, "host supports CXL\n");
> }
>
> return 0;
On Wed, 31 Aug 2022 10:15:58 +0200
Robert Richter <[email protected]> wrote:
> The downstream and upstream port Root Complex Register Blocks (RCRBs)
> are needed to control the ports and CXL devices connected to it. It
> also includes the location of the RCH/RCD downstream and upstream port
> component registers in MEMBAR0. Extract the RCRB from the host's CEDT
> entry.
>
> Signed-off-by: Robert Richter <[email protected]>
Hi Robert,
One trivial comment inline.
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/cxl/acpi.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 43 insertions(+)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index b3146b7ae922..439df9df2741 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -365,11 +365,48 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> return NULL;
> }
>
> +static int __cxl_get_rcrb(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)
> + return 0;
> +
> + chbs = (struct acpi_cedt_chbs *)header;
> +
> + if (ctx->uid != chbs->uid)
> + return 0;
> +
> + if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11)
> + return 0;
> +
> + if (chbs->length != SZ_8K)
> + return 0;
> +
> + ctx->chbcr = chbs->base;
> +
> + return 0;
> +}
> +
> +static resource_size_t cxl_get_rcrb(u32 uid)
> +{
> + struct cxl_chbs_context ctx = {
> + .uid = uid,
> + };
> +
> + acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, __cxl_get_rcrb, &ctx);
> +
> + return ctx.chbcr;
> +}
> +
> static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> {
> struct pci_host_bridge *host = NULL;
> struct acpi_device *adev;
> unsigned long long uid = ~0;
> + resource_size_t rcrb;
Some of these could be made local to the while loop to reduce their scope.
>
> while ((host = cxl_find_next_rch(host)) != NULL) {
> adev = ACPI_COMPANION(&host->dev);
> @@ -382,6 +419,12 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> if (uid > U32_MAX)
> continue;
>
> + rcrb = cxl_get_rcrb(uid);
> + if (!rcrb)
> + continue;
> +
> + dev_dbg(&host->dev, "RCRB found: 0x%08llx\n", (u64)rcrb);
> +
> dev_info(&host->dev, "host supports CXL\n");
> }
>
On Wed, 31 Aug 2022 10:15:57 +0200
Robert Richter <[email protected]> wrote:
> The UID is needed to read the RCH's CEDT entry with the RCRB base
> address. Determine the host's UID from its ACPI fw node.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/cxl/acpi.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index f9cdf23a91a8..b3146b7ae922 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -368,8 +368,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> {
> struct pci_host_bridge *host = NULL;
> + struct acpi_device *adev;
> + unsigned long long uid = ~0;
>
> while ((host = cxl_find_next_rch(host)) != NULL) {
> + adev = ACPI_COMPANION(&host->dev);
> + if (!adev || !adev->pnp.unique_id ||
> + (kstrtoull(adev->pnp.unique_id, 10, &uid) < 0))
There is an acpi_device_uid() accessor function that should probably be
used here.
Also, should a fialure to convert to an integer (or one within
limits) be something we paper over? Feels like we should fail
hard if that happens.
Admittedly I can't immediately find any spec that states that
the _UID should be either integer or under 32 bits...
ACPI allows a string and CXL just says it's 4 bytes long.
> + continue;
> +
> + dev_dbg(&adev->dev, "host uid: %llu\n", uid);
> +
> + if (uid > U32_MAX)
> + continue;
> +
> dev_info(&host->dev, "host supports CXL\n");
> }
>
On Wed, 31 Aug 2022 11:52:24 +0100
Jonathan Cameron <[email protected]> wrote:
> On Wed, 31 Aug 2022 10:15:56 +0200
> Robert Richter <[email protected]> wrote:
>
> > An RCH has an RCiEP connected to it with CXL DVSEC capabilities
> > present and the CXL PCIe DVSEC included. Check this.
> >
> > Signed-off-by: Robert Richter <[email protected]>
> One comment inline. This looks good to me.
>
> Reviewed-by: Jonathan Cameron <[email protected]>
>
> > ---
> > drivers/cxl/acpi.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index ffdf439adb87..f9cdf23a91a8 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -322,6 +322,8 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > {
> > struct pci_bus *bus = host ? host->bus : NULL;
> > struct acpi_device *adev;
> > + struct pci_dev *pdev;
> > + bool is_restricted_host;
> >
> > while ((bus = pci_find_next_bus(bus)) != NULL) {
> > host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
> > @@ -343,6 +345,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > dev_dbg(&host->dev, "PCI ACPI host found: %s\n",
> > acpi_dev_name(adev));
> >
> > + /* Check CXL DVSEC of dev 0 func 0 */
>
> So assumption here is that the hostbridge has a one or more RCiEPs.
> The spec (r3.0 9.11.4) allows for the EP to appear behind a root port
> - that case always felt odd to me, so I'm fine with not supporting it until
> we see a user.
>
> > + 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);
Thinking a bit more on this. I'm not sure this is sufficient.
Nothing in CXL 2.0 or later prevents true RCiEP devices (there are a
few references in CXL 3.0 e.g. 9.12.1 has RCDs or CXL RCiEPs - so just
detecting that there is one on the host bridge might not be sufficient
to distinguish this from a non RCH / RCB.
> > + pci_dev_put(pdev);
> > +
> > + if (!is_restricted_host)
> > + continue;
> > +
> > + dev_dbg(&host->dev, "CXL restricted host found\n");
> > +
> > return host;
> > }
> >
> > @@ -354,6 +370,7 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> > struct pci_host_bridge *host = NULL;
> >
> > while ((host = cxl_find_next_rch(host)) != NULL) {
> > + dev_info(&host->dev, "host supports CXL\n");
> > }
> >
> > return 0;
>
On Wed, 31 Aug 2022 10:15:59 +0200
Robert Richter <[email protected]> wrote:
> A downstream port must be connected to a component register block.
> Determine its base address from the RCRB.
>
> The implementation is analog to how cxl_setup_regs() is implemented
> for CXL VH mode. A struct cxl_component_reg_map is filled in, mapped
> and probed.
>
> Signed-off-by: Terry Bowman <[email protected]>
> Signed-off-by: Robert Richter <[email protected]>
A few comments inline. Mostly requests for references for things
I couldn't find in the specs.
> ---
> drivers/cxl/acpi.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 80 insertions(+)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 439df9df2741..88bbd2bb61fc 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -401,12 +401,84 @@ static resource_size_t cxl_get_rcrb(u32 uid)
> return ctx.chbcr;
> }
>
> +static resource_size_t cxl_get_component_reg_phys(resource_size_t rcrb)
> +{
> + resource_size_t component_reg_phys;
> + u32 bar0, bar1;
> + void *addr;
> +
> + /*
> + * RCRB's BAR[0..1] point to component block containing CXL subsystem
> + * component registers.
> + * CXL 8.2.4 - Component Register Layout Definition.
For references include spec version. Based on discussion other day,
preference is always latest version. So r3.0 8.2.3
is probably right. I think your references are CXL r2.0?
> + *
> + * Also, RCRB accesses must use MMIO readl()/readq() to guarantee
> + * 32/64-bit access.
> + * CXL 8.2.2 - CXL 1.1 Upstream and Downstream Port Subsystem Component
> + * Registers
> + */
> + addr = ioremap(rcrb, PCI_BASE_ADDRESS_0 + SZ_8);
> + bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> + bar1 = readl(addr + PCI_BASE_ADDRESS_1);
The spec is a bit confusing on this, but I think you are reading into
MEMBAR0 of the RCRB, for which there isn't a lot of description other than
it being an address. It's referred to as a 64-bit BAR in places so you
might be right - or it might be intended to be a bare address..
We might want a clarification on this...
Also it's a 64 bit address so we need to read it in one go. However it's
referred to as a being a 64 bit address at 0x10 and 0x14 so who knows...
> + 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).
The rcrb is 8k though I'm not immediately spotting an alignment requirement,
but I'm not sure the component regs have any restrictions do... Add a reference perhaps?
For non RCD devices there is a 64K alignment requirement, but I can't find
anything for RCDs (might just be missing it).
> + */
> + if (component_reg_phys & (SZ_8K - 1))
> + return CXL_RESOURCE_NONE;
> +
> + return component_reg_phys;
> +}
> +
> +static int cxl_setup_component_reg(struct device *parent,
> + resource_size_t component_reg_phys)
> +{
> + struct cxl_component_reg_map comp_map;
> + void __iomem *base;
> +
> + if (component_reg_phys == CXL_RESOURCE_NONE)
> + return -EINVAL;
> +
> + base = ioremap(component_reg_phys, SZ_64K);
Add a spec reference for the size. Table 8-21 perhaps?
> + if (!base) {
> + dev_err(parent, "failed to map registers\n");
> + return -ENOMEM;
> + }
> +
> + cxl_probe_component_regs(parent, base, &comp_map);
> + iounmap(base);
> +
> + if (!comp_map.hdm_decoder.valid) {
> + dev_err(parent, "HDM decoder registers not found\n");
> + return -ENXIO;
Hmm. HDM decoder capability is optional for RCDs - might be using the
range registers. Seems like we'd really want to handle that for
RCDs. Future work I guess.
> + }
> +
> + dev_dbg(parent, "Set up component registers\n");
> +
> + return 0;
> +}
> +
> static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> {
> struct pci_host_bridge *host = NULL;
> struct acpi_device *adev;
> unsigned long long uid = ~0;
> resource_size_t rcrb;
> + resource_size_t component_reg_phys;
Trivial: As before, if we can reduce scope to inside the while loop, slightly cleaner
and more local.
> + int rc;
>
> while ((host = cxl_find_next_rch(host)) != NULL) {
> adev = ACPI_COMPANION(&host->dev);
> @@ -425,10 +497,18 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
>
> dev_dbg(&host->dev, "RCRB found: 0x%08llx\n", (u64)rcrb);
>
> + component_reg_phys = cxl_get_component_reg_phys(rcrb);
> + rc = cxl_setup_component_reg(&host->dev, component_reg_phys);
Perhaps rename to make it clear this is getting the DSP component registers.
Future work would be to add support for the CXL 3.0 feature that lets even an
RCD just put some of these registers in a BAR as per CXL 2.0 devices.
> + if (rc)
> + goto fail;
> +
> dev_info(&host->dev, "host supports CXL\n");
> }
>
> return 0;
> +fail:
Better to have a more specific error message and return directly above.
Note that so far vast majority of CXL error messages are dev_dbg,
so for consistency perhaps this should be as well.
(I prefer dev_err() but not my subsystem ;)
> + dev_err(&host->dev, "failed to initialize CXL host: %d\n", rc);
dev_err_probe() is slightly nicer to use if things can only happen in
probe() paths.
> + return rc;
> }
>
> static struct lock_class_key cxl_root_key;
On Wed, 31 Aug 2022 10:16:01 +0200
Robert Richter <[email protected]> wrote:
> RCD mode has a different enumeration scheme other than in CXL VH mode.
> An RCD is directly connected to an RCH without downstream and upstream
> ports showing up in between in the PCI hierarchy. Due to the direct
> connection of RCD and RCH, the host bridge is always the RCD's parent
> instead of the grandparent.
Mentioned earlier, but that's not quite true. There is a reference in
the spec to allowing it to be under a root port (some sort of virtual structure
- I'm not sure of 'why' you would that though.)(
> Modify devm_cxl_enumerate_ports()
> respectively.
Don't line wrap above.
>
> Implement this by introducing a function to determine the device's
> downstream port. The 'for' loop is adjusted for RCD mode and in this
> case find_cxl_port() will always find the host's associated port and
> the loop iteration stops.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/cxl/core/port.c | 36 ++++++++++++++++++++++++------------
> 1 file changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 61e9915162d5..08b99423dbf8 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1084,6 +1084,22 @@ static struct device *grandparent(struct device *dev)
> return NULL;
> }
>
> +static struct device *cxl_mem_dport_dev(struct cxl_memdev *cxlmd)
> +{
> + struct device *dev = cxlmd->dev.parent;
> + struct pci_dev *pdev = to_pci_dev(cxlmd->dev.parent);
to_pci_dev(dev);
> +
> + /*
> + * An RCiEP is directly connected to the root bridge without
> + * any PCI bridges/ports in between. Reduce the parent level
> + * for those.
> + */
> + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END)
> + return dev;
> +
> + return dev->parent;
Switching from grandparent to this is a little confusing because it's
done in two steps. Perhaps use
return grandparent(cmlmd->dev);
here to keep that connection and rename dev in this function to parent.
Far too many devices in here with similar names for it to be easy
to read.
> +}
> +
> static void delete_endpoint(void *data)
> {
> struct cxl_memdev *cxlmd = data;
> @@ -1339,7 +1355,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
> int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
> {
> struct device *dev = &cxlmd->dev;
> - struct device *iter;
> + struct device *dport_dev;
> int rc;
>
> rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd);
> @@ -1352,25 +1368,21 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
> * attempt fails.
> */
> retry:
> - for (iter = dev; iter; iter = grandparent(iter)) {
> - struct device *dport_dev = grandparent(iter);
> + for (dport_dev = cxl_mem_dport_dev(cxlmd); dport_dev;
> + dport_dev = grandparent(dport_dev)) {
I don't like looping for the RCD case as it relies a bit too
much on subtle relationships between devices and parent.
Perhaps better to factor out the contents of the loop, then handle
the RCD case separately from the main loop.
I haven't tried it, so perhaps that looks even less readable.
> struct device *uport_dev;
> struct cxl_dport *dport;
> struct cxl_port *port;
>
> - if (!dport_dev)
> - return 0;
> -
> uport_dev = dport_dev->parent;
> if (!uport_dev) {
> - dev_warn(dev, "at %s no parent for dport: %s\n",
> - dev_name(iter), dev_name(dport_dev));
> + dev_warn(dev, "no parent for dport: %s\n",
> + dev_name(dport_dev));
> 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));
> + dev_dbg(dev, "scan: dport_dev: %s parent: %s\n",
> + dev_name(dport_dev), dev_name(uport_dev));
> port = find_cxl_port(dport_dev, &dport);
> if (port) {
> dev_dbg(&cxlmd->dev,
> @@ -1418,7 +1430,7 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_ports, CXL);
> struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
> struct cxl_dport **dport)
> {
> - return find_cxl_port(grandparent(&cxlmd->dev), dport);
> + return find_cxl_port(cxl_mem_dport_dev(cxlmd), dport);
> }
> EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL);
>
On Wed, 31 Aug 2022 10:16:00 +0200
Robert Richter <[email protected]> wrote:
> RCD mode has a different enumeration scheme other than in CXL VH mode.
> An RCD is directly connected to an RCH without downstream and upstream
> ports showing up in between in the PCI hierarchy. Skip dport
> enumeration for RCHs. Upstream and downstream ports of RCH and RCD
> will be setup separately in a later patch.
>
> Introduce the function is_rch_uport() to detect an RCH port. For RCHs
> the parent root port is not the "ACPI0017" device and instead does not
> have a fw node connected to it.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/cxl/core/pci.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 0dbbe8d39b07..86ed112eb262 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -65,6 +65,15 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
> return 0;
> }
>
> +/*
> + * A parent of an RCH (CXL 1.1 host) is a plain platform device while
> + * a 2.0 host links to the ACPI0017 root device.
> + */
> +static inline bool is_rch_uport(struct cxl_port *port)
> +{
> + return is_cxl_port(&port->dev) && !port->dev.parent->fwnode;
I'm not keen on the presence of fwnode being used to distinguish anything.
That's the sort of thing that gets 'fixed' by later patches.
Can we check something more specific?
> +}
> +
> /**
> * devm_cxl_port_enumerate_dports - enumerate downstream ports of the upstream port
> * @port: cxl_port whose ->uport is the upstream of dports to be enumerated
> @@ -74,10 +83,19 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
> */
> int devm_cxl_port_enumerate_dports(struct cxl_port *port)
> {
> - struct pci_bus *bus = cxl_port_to_pci_bus(port);
> + struct pci_bus *bus;
> struct cxl_walk_context ctx;
> int type;
>
> + /*
> + * Skip enumeration in Restricted CXL Device mode as the
> + * device has been already registered at the host's dport
> + * during host discovery.
> + */
> + if (is_rch_uport(port))
> + return 0;
> +
> + bus = cxl_port_to_pci_bus(port);
> if (!bus)
> return -ENXIO;
>
On Wed, 31 Aug 2022 10:16:02 +0200
Robert Richter <[email protected]> wrote:
> Do the plumbing of ports to enable RCD/RCH pairs.
>
> Do this by enumerating all necessary ports an endpoint needs to
> connect to. This includes:
>
> 1) A CXL root port with dport links to the RCHs. The port links to
> the CXL root platform device for RCH mode.
>
> 2) RCH ports with dport links to its endpoints. Port connects to the
> pci host bridge device.
>
> 3) CXL device endpoint connected to the RCH.
>
> The port creation for the endpoint (3) is already implemented and
> works in RCD mode too. Thus, it is not scope of this patch. Only the
> endpoints must be registered at the host bridge port.
>
> Implement this by introducing the function cxl_enumerate_rch_ports().
> It registers a CXL host at the CXL root device, creates the host's
> port and registers the existing CXL memory device endpoint at it. The
> port of the CXL root device is created with the first CXL host being
> registered.
>
> Once enumerated, CXL restricted hosts show up in sysfs with CXL
> devices connected as endpoints to it.
>
> Signed-off-by: Robert Richter <[email protected]>
Hi Robert,
A few things inline.
Jonathan
> ---
> drivers/cxl/acpi.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 88bbd2bb61fc..56b2d222afcc 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -471,13 +471,52 @@ static int cxl_setup_component_reg(struct device *parent,
> return 0;
> }
>
> +static int cxl_enumerate_rch_ports(struct device *root_dev,
> + struct cxl_port *cxl_root,
> + struct pci_host_bridge *host,
> + resource_size_t component_reg_phys,
> + int port_id)
> +{
> + struct cxl_dport *dport;
> + struct cxl_port *port;
> + struct pci_dev *pdev;
> +
> + dport = devm_cxl_add_dport(cxl_root, &host->dev, port_id,
> + component_reg_phys);
> + if (IS_ERR(dport))
> + return PTR_ERR(dport);
> +
> + port = devm_cxl_add_port(root_dev, &host->dev,
> + component_reg_phys, dport);
> + if (IS_ERR(port))
> + return PTR_ERR(port);
> +
> + pdev = pci_get_slot(host->bus, PCI_DEVFN(0, 0));
> + if (!pdev)
> + return -ENXIO;
> +
> + /* Note: The endpoint provides the component reg base. */
I'm not sure what this comment means. Which component reg base?
> + dport = devm_cxl_add_dport(port, &pdev->dev, 0,
> + CXL_RESOURCE_NONE);
Trivial: No need to wrap the above.
> +
> + pci_dev_put(pdev);
> +
> + if (IS_ERR(dport))
> + return PTR_ERR(dport);
> +
> + return 0;
> +}
> +
> static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> {
> + struct device *root_dev = &pdev->dev;
> struct pci_host_bridge *host = NULL;
> struct acpi_device *adev;
> + struct cxl_port *cxl_root = NULL;
> unsigned long long uid = ~0;
> resource_size_t rcrb;
> resource_size_t component_reg_phys;
> + int port_id = 0;
> int rc;
>
> while ((host = cxl_find_next_rch(host)) != NULL) {
> @@ -497,11 +536,30 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
>
> dev_dbg(&host->dev, "RCRB found: 0x%08llx\n", (u64)rcrb);
>
> + /*
> + * For CXL 1.1 hosts we create a root device other
> + * than the ACPI0017 device to hold the devm data and
> + * the uport ref.
> + */
> + if (!cxl_root) {
> + cxl_root = devm_cxl_add_port(root_dev, root_dev,
> + CXL_RESOURCE_NONE, NULL);
> + if (IS_ERR(cxl_root)) {
> + rc = PTR_ERR(cxl_root);
> + goto fail;
> + }
> + }
> +
> component_reg_phys = cxl_get_component_reg_phys(rcrb);
> rc = cxl_setup_component_reg(&host->dev, component_reg_phys);
> if (rc)
> goto fail;
>
> + rc = cxl_enumerate_rch_ports(root_dev, cxl_root, host,
> + component_reg_phys, port_id++);
> + if (rc)
> + goto fail;
> +
> dev_info(&host->dev, "host supports CXL\n");
> }
>
On Wed, 31 Aug 2022 10:15:48 +0200
Robert Richter <[email protected]> wrote:
> 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. In RCD mode an ACPI0017 device
> might not necessarily exist and the host bridge can have a standard
> PCIe host bridge PNP0A08 ID, there aren't any CXL port or switches in
> the PCIe hierarchy visible. As such the RCD mode enumeration and host
> discovery is very different from CXL VH. See patch #5 for
> implementation details.
>
> This implementation 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 #4 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 #5 to #15 contain
> the actual implementation of RCD mode support.
>
> [1] https://www.computeexpresslink.org/spec-landing
Hi Robert,
I'm curious on the aims of this work. Given expectation for RCDs is often
that the host firmware has set them up before the OS loads, what functionality
do you want to gain by mapping these into the CXL 2.0+ focused infrastructure?
When I did some analysis a while back on CXL 1.1 I was pretty much assuming
that there was no real reason to let the OS know about it because it
couldn't do much of any use with the information. There are some corners
like RAS where it might be useful or perhaps to enable some of the CXL 3.0
features that are allowed to be EP only and so could be relevant for
an older host (e.g. CPMUs).
With my QEMU hat on I wasn't planning to bother with anything pre 2.0
but it might be worth considering to let us exercise this code...
Jonathan
>
> Robert Richter (15):
> 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: Add probe function to detect restricted CXL hosts in RCD
> mode
> PCI/ACPI: Link host bridge to its ACPI fw node
> cxl/acpi: Check RCH's PCIe Host Bridge ACPI ID
> cxl/acpi: Check RCH's CXL DVSEC capabilities
> cxl/acpi: Determine PCI host bridge's ACPI UID
> cxl/acpi: Extract the RCH's RCRB base address from CEDT
> cxl/acpi: Extract the host's component register base address from RCRB
> cxl/acpi: Skip devm_cxl_port_enumerate_dports() when in RCD mode
> cxl/acpi: Rework devm_cxl_enumerate_ports() to support RCD mode
> cxl/acpi: Enumerate ports in RCD mode to enable RCHs and RCDs
> cxl/acpi: Specify module load order dependency for the cxl_acpi module
>
> drivers/acpi/pci_root.c | 1 +
> drivers/cxl/acpi.c | 311 ++++++++++++++++++++++++++++++++++-
> drivers/cxl/core/pci.c | 22 ++-
> drivers/cxl/core/port.c | 103 ++++++++----
> drivers/cxl/core/regs.c | 3 +
> drivers/cxl/cxl.h | 2 -
> drivers/cxl/mem.c | 1 +
> tools/testing/cxl/test/cxl.c | 8 +-
> 8 files changed, 400 insertions(+), 51 deletions(-)
>
Thanks Jonathan for reviewing this series.
On 31.08.22 09:54:58, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 10:15:49 +0200
> Robert Richter <[email protected]> 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.
> >
> > Signed-off-by: Robert Richter <[email protected]>
> Reviewed-by: Jonathan Cameron <[email protected]>
There is this 0day test bot warning and I will need to either extend
this patch or add another one to fix regs.c.
-Robert
>
> The wonders of code evolution leaving signs behind ;)
>
> > ---
> > drivers/cxl/cxl.h | 2 --
> > 1 file changed, 2 deletions(-)
On 31.08.22 10:59:45, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 10:15:51 +0200
> Robert Richter <[email protected]> wrote:
>
> > 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.
> >
> > Signed-off-by: Robert Richter <[email protected]>
>
> This is one for Dan etc as it is mostly a question of how verbose we want
> the debug prints to be plus preference for caller or callee being
> responsible for outputting this sort of message.
Esp. together with dyndbg this is very useful as you can get a whole
picture of the port enablement.
-Robert
On 31.08.22 11:20:28, Jonathan Cameron wrote:
> Robert Richter <[email protected]> wrote:
> > +static const struct acpi_device_id cxl_host_ids[] = {
> > + { "ACPI0016", 0 },
> > + { "PNP0A08", 0 },
> > + { },
>
> Trivial but no comma after a null terminator. Always good to make
> it harder for people to add things where they really shouldn't!
Can do this.
> pci_root.c avoids using an acpi_device_id table for similar matching.
> I think the point being to separate probe type use of this table
> from cases where we aren't using a normal device probe.
> So to remain consistent with that, I would just grab the hid
> and match it directly in this code.
Grabbing the hid only is actually a violation of the acpi spec as a
cid could be used interchangeable. It must also work then.
It is also not possible to use something like probe or a handler
matching the ids because the hosts must be enabled with the already
existing drivers and handlers. Suppose there are multiple handlers for
the same ids, the first handler wins and all other never get called.
To me it looks sane and simple to use acpi_match_device_ids() here.
-Robert
>
> I don't feel that strongly about this if the ACPI maintainers are
> fine with reusing this infrastructure as you have it here.
On 31.08.22 11:08:04, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 10:15:53 +0200
> Robert Richter <[email protected]> wrote:
>
> > Restricted CXL device (RCD) mode (formerly CXL 1.1) uses a different
> > enumeration scheme other than CXL VH (formerly CXL 2.0). In RCD mode a
> > host/device (RCH-RCD) pair shows up as a legal PCIe hierarchy with an
> > ACPI host bridge ("PNP0A08" or "ACPI0016" HID) and RCiEP connected to
> > it with a description of the CXL device.
> >
> > Add function cxl_restricted_host_probe() to probe RCD enumerated
> > devices. The function implements a loop that detects all CXL capable
> > ACPI PCI root bridges in the system (RCD mode only). The iterator
> > function cxl_find_next_rch() is introduced to walk through all of the
> > CXL hosts. The loop will then enable all CXL devices connected to the
> > host. For now, only implement an empty loop with an iterator that
> > returns all pci host bridges in the system.
> >
> > The probe function is triggered by adding an own root device for RCHs.
> > This is different to CXL VH where an ACPI "ACPI0017" root device
> > exists. Its detection starts the CXL host detection. In RCD mode such
> > a device does not necessarily exists, so solve this by creating a
> > plain platform device that is not an ACPI device and is root only for
> > RCHs.
>
> If I read this correctly that platform device is created whether or not
> there are any cxl devices in the system?
>
> Can we create it only if we find some devices that will be placed
> under it later?
This would move the host detection from probe to init which I wanted
to avoid to better control driver init order dependencies.
I could add a put_device() at the end of a probe so that it will be
released in case no other references use it. This implies the refcount
is maintained for parent devices. Or this needs to be added to. So if
there are no children (hosts) attached to the root device after probe,
it will disappear.
> > @@ -531,7 +566,41 @@ static struct platform_driver cxl_acpi_driver = {
> > .id_table = cxl_test_ids,
> > };
> >
> > -module_platform_driver(cxl_acpi_driver);
> > +static void cxl_acpi_device_release(struct device *dev) { }
>
> Why the empty release? Perhaps introduce this only when it
> does something.
The core device driver requires this in device_release() to be setup.
There is nothing to do as the device is kept in a static struct.
That's why it's empty.
-Robert
On 31.08.22 11:52:24, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 10:15:56 +0200
> Robert Richter <[email protected]> wrote:
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index ffdf439adb87..f9cdf23a91a8 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -322,6 +322,8 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > {
> > struct pci_bus *bus = host ? host->bus : NULL;
> > struct acpi_device *adev;
> > + struct pci_dev *pdev;
> > + bool is_restricted_host;
> >
> > while ((bus = pci_find_next_bus(bus)) != NULL) {
> > host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
> > @@ -343,6 +345,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > dev_dbg(&host->dev, "PCI ACPI host found: %s\n",
> > acpi_dev_name(adev));
> >
> > + /* Check CXL DVSEC of dev 0 func 0 */
>
> So assumption here is that the hostbridge has a one or more RCiEPs.
> The spec (r3.0 9.11.4) allows for the EP to appear behind a root port
> - that case always felt odd to me, so I'm fine with not supporting it until
> we see a user.
The software view of an RCD is always the same, it shows up always as
an RCiEP. See figure 9-12 and 9-13 of the spec.
-Robert
>
> > + 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);
> > + pci_dev_put(pdev);
> > +
> > + if (!is_restricted_host)
> > + continue;
> > +
> > + dev_dbg(&host->dev, "CXL restricted host found\n");
> > +
> > return host;
> > }
On 31.08.22 12:12:22, Jonathan Cameron wrote:
> > On Wed, 31 Aug 2022 10:15:56 +0200
> > Robert Richter <[email protected]> wrote:
> > > @@ -322,6 +322,8 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > > {
> > > struct pci_bus *bus = host ? host->bus : NULL;
> > > struct acpi_device *adev;
> > > + struct pci_dev *pdev;
> > > + bool is_restricted_host;
> > >
> > > while ((bus = pci_find_next_bus(bus)) != NULL) {
> > > host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
> > > @@ -343,6 +345,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > > dev_dbg(&host->dev, "PCI ACPI host found: %s\n",
> > > acpi_dev_name(adev));
> > >
> > > + /* Check CXL DVSEC of dev 0 func 0 */
> >
> > So assumption here is that the hostbridge has a one or more RCiEPs.
> > The spec (r3.0 9.11.4) allows for the EP to appear behind a root port
> > - that case always felt odd to me, so I'm fine with not supporting it until
> > we see a user.
> >
> > > + 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);
>
> Thinking a bit more on this. I'm not sure this is sufficient.
> Nothing in CXL 2.0 or later prevents true RCiEP devices (there are a
> few references in CXL 3.0 e.g. 9.12.1 has RCDs or CXL RCiEPs - so just
> detecting that there is one on the host bridge might not be sufficient
> to distinguish this from a non RCH / RCB.
An RCD has its own host bridge created (software view, not the phys
topology). Host and device are paired in this case. Non-RCDs are
standard endpoints and not RCiEPs, they have their own host. There
cannot be both types connected to the same host.
Again, see figure 9-12 and 9-13.
-Robert
>
> > > + pci_dev_put(pdev);
> > > +
> > > + if (!is_restricted_host)
> > > + continue;
> > > +
> > > + dev_dbg(&host->dev, "CXL restricted host found\n");
> > > +
> > > return host;
> > > }
> > >
> > > @@ -354,6 +370,7 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> > > struct pci_host_bridge *host = NULL;
> > >
> > > while ((host = cxl_find_next_rch(host)) != NULL) {
> > > + dev_info(&host->dev, "host supports CXL\n");
> > > }
> > >
> > > return 0;
> >
>
On 31.08.22 12:09:09, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 10:15:58 +0200
> Robert Richter <[email protected]> wrote:
> > static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> > {
> > struct pci_host_bridge *host = NULL;
> > struct acpi_device *adev;
> > unsigned long long uid = ~0;
> > + resource_size_t rcrb;
>
> Some of these could be made local to the while loop to reduce their scope.
I would need to move most of the vars into the loop then. So I rather
want to keep it as it is.
I also remember a compiler or code checker complaining about block
declaration, though it is in the c standard.
-Robert
> >
> > while ((host = cxl_find_next_rch(host)) != NULL) {
> > adev = ACPI_COMPANION(&host->dev);
> > @@ -382,6 +419,12 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> > if (uid > U32_MAX)
> > continue;
> >
> > + rcrb = cxl_get_rcrb(uid);
> > + if (!rcrb)
> > + continue;
> > +
> > + dev_dbg(&host->dev, "RCRB found: 0x%08llx\n", (u64)rcrb);
> > +
> > dev_info(&host->dev, "host supports CXL\n");
> > }
> >
>
On 31.08.22 12:00:27, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 10:15:57 +0200
> Robert Richter <[email protected]> wrote:
>
> > The UID is needed to read the RCH's CEDT entry with the RCRB base
> > address. Determine the host's UID from its ACPI fw node.
> >
> > Signed-off-by: Robert Richter <[email protected]>
> > ---
> > drivers/cxl/acpi.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index f9cdf23a91a8..b3146b7ae922 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -368,8 +368,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> > {
> > struct pci_host_bridge *host = NULL;
> > + struct acpi_device *adev;
> > + unsigned long long uid = ~0;
> >
> > while ((host = cxl_find_next_rch(host)) != NULL) {
> > + adev = ACPI_COMPANION(&host->dev);
> > + if (!adev || !adev->pnp.unique_id ||
> > + (kstrtoull(adev->pnp.unique_id, 10, &uid) < 0))
>
> There is an acpi_device_uid() accessor function that should probably be
> used here.
That accessor actually does not help really, there is no null pointer
check for adev. Using it actually adds more complexity since another
variable is introduced plus you need to look at the function's
implementation anyway.
The adev->pnp.unique_id access pattern is widely used in the kernel, I
don't expect changes in the data struct here.
> Also, should a fialure to convert to an integer (or one within
> limits) be something we paper over? Feels like we should fail
> hard if that happens.
This is a real corner case and close to a broken firmware
implementation. I think current dbg messages are good to find where
the detection stops.
> Admittedly I can't immediately find any spec that states that
> the _UID should be either integer or under 32 bits...
> ACPI allows a string and CXL just says it's 4 bytes long.
IIRC the UID can be implemented as string or 8 bytes, there is no
limitation then. That's why the range check below.
-Robert
>
> > + continue;
> > +
> > + dev_dbg(&adev->dev, "host uid: %llu\n", uid);
> > +
> > + if (uid > U32_MAX)
> > + continue;
> > +
> > dev_info(&host->dev, "host supports CXL\n");
> > }
> >
>
On 31.08.22 12:58:30, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 10:16:00 +0200
> Robert Richter <[email protected]> wrote:
> > +/*
> > + * A parent of an RCH (CXL 1.1 host) is a plain platform device while
> > + * a 2.0 host links to the ACPI0017 root device.
> > + */
> > +static inline bool is_rch_uport(struct cxl_port *port)
> > +{
> > + return is_cxl_port(&port->dev) && !port->dev.parent->fwnode;
>
> I'm not keen on the presence of fwnode being used to distinguish anything.
> That's the sort of thing that gets 'fixed' by later patches.
>
> Can we check something more specific?
We actually know the parent device is the root device, so we can check
this. Even easier.
Thanks,
-Robert
On 31.08.22 12:56:56, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 10:15:59 +0200
> Robert Richter <[email protected]> wrote:
> A few comments inline. Mostly requests for references for things
> I couldn't find in the specs.
Most of it comes from the pci base spec (5 or 6).
>
> > ---
> > drivers/cxl/acpi.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 80 insertions(+)
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 439df9df2741..88bbd2bb61fc 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -401,12 +401,84 @@ static resource_size_t cxl_get_rcrb(u32 uid)
> > return ctx.chbcr;
> > }
> >
> > +static resource_size_t cxl_get_component_reg_phys(resource_size_t rcrb)
> > +{
> > + resource_size_t component_reg_phys;
> > + u32 bar0, bar1;
> > + void *addr;
> > +
> > + /*
> > + * RCRB's BAR[0..1] point to component block containing CXL subsystem
> > + * component registers.
> > + * CXL 8.2.4 - Component Register Layout Definition.
>
> For references include spec version. Based on discussion other day,
> preference is always latest version. So r3.0 8.2.3
> is probably right. I think your references are CXL r2.0?
Right will update the comment.
>
>
> > + *
> > + * Also, RCRB accesses must use MMIO readl()/readq() to guarantee
> > + * 32/64-bit access.
> > + * CXL 8.2.2 - CXL 1.1 Upstream and Downstream Port Subsystem Component
> > + * Registers
> > + */
> > + addr = ioremap(rcrb, PCI_BASE_ADDRESS_0 + SZ_8);
> > + bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> > + bar1 = readl(addr + PCI_BASE_ADDRESS_1);
>
> The spec is a bit confusing on this, but I think you are reading into
> MEMBAR0 of the RCRB, for which there isn't a lot of description other than
> it being an address. It's referred to as a 64-bit BAR in places so you
> might be right - or it might be intended to be a bare address..
>
> We might want a clarification on this...
>
> Also it's a 64 bit address so we need to read it in one go. However it's
> referred to as a being a 64 bit address at 0x10 and 0x14 so who knows...
This is part of the pci base spec and clearly defined there. There are
also some similar implementation in the kernel already.
>
>
> > + 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).
>
> The rcrb is 8k though I'm not immediately spotting an alignment requirement,
> but I'm not sure the component regs have any restrictions do... Add a reference perhaps?
> For non RCD devices there is a 64K alignment requirement, but I can't find
> anything for RCDs (might just be missing it).
This are the requirements of the pci base spec to membar ranges. The
range size is power of 2 and base must be aligned to its size.
>
> > + */
> > + if (component_reg_phys & (SZ_8K - 1))
> > + return CXL_RESOURCE_NONE;
> > +
> > + return component_reg_phys;
> > +}
> > +
> > +static int cxl_setup_component_reg(struct device *parent,
> > + resource_size_t component_reg_phys)
> > +{
> > + struct cxl_component_reg_map comp_map;
> > + void __iomem *base;
> > +
> > + if (component_reg_phys == CXL_RESOURCE_NONE)
> > + return -EINVAL;
> > +
> > + base = ioremap(component_reg_phys, SZ_64K);
>
> Add a spec reference for the size. Table 8-21 perhaps?
Can add a comment here.
>
> > + if (!base) {
> > + dev_err(parent, "failed to map registers\n");
> > + return -ENOMEM;
> > + }
> > +
> > + cxl_probe_component_regs(parent, base, &comp_map);
> > + iounmap(base);
> > +
> > + if (!comp_map.hdm_decoder.valid) {
> > + dev_err(parent, "HDM decoder registers not found\n");
> > + return -ENXIO;
>
> Hmm. HDM decoder capability is optional for RCDs - might be using the
> range registers. Seems like we'd really want to handle that for
> RCDs. Future work I guess.
I used the same message as for the non-RCD code path. HDM decoding is
just a subset of features handled with component regs. We need to
generalize the code for this in the future.
>
> > + }
> > +
> > + dev_dbg(parent, "Set up component registers\n");
> > +
> > + return 0;
> > +}
> > +
> > static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> > {
> > struct pci_host_bridge *host = NULL;
> > struct acpi_device *adev;
> > unsigned long long uid = ~0;
> > resource_size_t rcrb;
> > + resource_size_t component_reg_phys;
> Trivial: As before, if we can reduce scope to inside the while loop, slightly cleaner
> and more local.
> > + int rc;
> >
> > while ((host = cxl_find_next_rch(host)) != NULL) {
> > adev = ACPI_COMPANION(&host->dev);
> > @@ -425,10 +497,18 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> >
> > dev_dbg(&host->dev, "RCRB found: 0x%08llx\n", (u64)rcrb);
> >
> > + component_reg_phys = cxl_get_component_reg_phys(rcrb);
> > + rc = cxl_setup_component_reg(&host->dev, component_reg_phys);
>
> Perhaps rename to make it clear this is getting the DSP component registers.
>
> Future work would be to add support for the CXL 3.0 feature that lets even an
> RCD just put some of these registers in a BAR as per CXL 2.0 devices.
Yes, this is left out atm.
>
> > + if (rc)
> > + goto fail;
>
> > +
> > dev_info(&host->dev, "host supports CXL\n");
> > }
> >
> > return 0;
> > +fail:
>
> Better to have a more specific error message and return directly above.
> Note that so far vast majority of CXL error messages are dev_dbg,
> so for consistency perhaps this should be as well.
> (I prefer dev_err() but not my subsystem ;)
There is more verbosity on the errors with dbg enabled. Note there are
only a few dev_info/dev_err messages to not polute the logs. dev_err()
is only used if something unexpected happens (e.g. the device exists
but component regs are broken).
>
> > + dev_err(&host->dev, "failed to initialize CXL host: %d\n", rc);
> dev_err_probe() is slightly nicer to use if things can only happen in
> probe() paths.
Will consider that.
-Robert
>
> > + return rc;
> > }
> >
> > static struct lock_class_key cxl_root_key;
>
On 31.08.22 13:16:34, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 10:16:02 +0200
> Robert Richter <[email protected]> wrote:
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 88bbd2bb61fc..56b2d222afcc 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -471,13 +471,52 @@ static int cxl_setup_component_reg(struct device *parent,
> > return 0;
> > }
> >
> > +static int cxl_enumerate_rch_ports(struct device *root_dev,
> > + struct cxl_port *cxl_root,
> > + struct pci_host_bridge *host,
> > + resource_size_t component_reg_phys,
> > + int port_id)
> > +{
> > + struct cxl_dport *dport;
> > + struct cxl_port *port;
> > + struct pci_dev *pdev;
> > +
> > + dport = devm_cxl_add_dport(cxl_root, &host->dev, port_id,
> > + component_reg_phys);
> > + if (IS_ERR(dport))
> > + return PTR_ERR(dport);
> > +
> > + port = devm_cxl_add_port(root_dev, &host->dev,
> > + component_reg_phys, dport);
> > + if (IS_ERR(port))
> > + return PTR_ERR(port);
> > +
> > + pdev = pci_get_slot(host->bus, PCI_DEVFN(0, 0));
> > + if (!pdev)
> > + return -ENXIO;
> > +
> > + /* Note: The endpoint provides the component reg base. */
>
> I'm not sure what this comment means. Which component reg base?
There is no component_reg_phys provided here, so the addr is
CXL_RESOURCE_NONE.
Will improve the comment.
>
> > + dport = devm_cxl_add_dport(port, &pdev->dev, 0,
> > + CXL_RESOURCE_NONE);
>
> Trivial: No need to wrap the above.
Right.
Thanks,
-Robert
Jonathan,
On 31.08.22 13:23:29, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 10:15:48 +0200
> Robert Richter <[email protected]> wrote:
>
> > 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. In RCD mode an ACPI0017 device
> > might not necessarily exist and the host bridge can have a standard
> > PCIe host bridge PNP0A08 ID, there aren't any CXL port or switches in
> > the PCIe hierarchy visible. As such the RCD mode enumeration and host
> > discovery is very different from CXL VH. See patch #5 for
> > implementation details.
> >
> > This implementation 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 #4 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 #5 to #15 contain
> > the actual implementation of RCD mode support.
> >
> > [1] https://www.computeexpresslink.org/spec-landing
>
> Hi Robert,
>
> I'm curious on the aims of this work. Given expectation for RCDs is often
> that the host firmware has set them up before the OS loads, what functionality
> do you want to gain by mapping these into the CXL 2.0+ focused infrastructure?
>
> When I did some analysis a while back on CXL 1.1 I was pretty much assuming
> that there was no real reason to let the OS know about it because it
> couldn't do much of any use with the information. There are some corners
> like RAS where it might be useful or perhaps to enable some of the CXL 3.0
> features that are allowed to be EP only and so could be relevant for
> an older host (e.g. CPMUs).
though CXL RCD works with a legacy kernel or without any CXL
functionality added, a CXL aware kernel can be useful also for RCD
mode. RAS is a topic here but also gathering device information such
as status or topology. Everything where access to the component
register block or mailbox interface is required.
Another plus, the CXL hierarchy becomes visible for RCD mode in sysfs
and the device hierarchy.
Reusing the existing infrastructure for this makes sense. Many
features overlap in both modes (e.g. RAS, mailbox again, or topology
information).
Thanks again for you review.
-Robert
>
> With my QEMU hat on I wasn't planning to bother with anything pre 2.0
> but it might be worth considering to let us exercise this code...
>
> Jonathan
On 31.08.22 13:11:19, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 10:16:01 +0200
> Robert Richter <[email protected]> wrote:
>
> > RCD mode has a different enumeration scheme other than in CXL VH mode.
> > An RCD is directly connected to an RCH without downstream and upstream
> > ports showing up in between in the PCI hierarchy. Due to the direct
> > connection of RCD and RCH, the host bridge is always the RCD's parent
> > instead of the grandparent.
> Mentioned earlier, but that's not quite true. There is a reference in
> the spec to allowing it to be under a root port (some sort of virtual structure
> - I'm not sure of 'why' you would that though.)(
Yes, but software view is still the same, see other mail.
>
> > Modify devm_cxl_enumerate_ports()
> > respectively.
>
> Don't line wrap above.
>
> >
> > Implement this by introducing a function to determine the device's
> > downstream port. The 'for' loop is adjusted for RCD mode and in this
> > case find_cxl_port() will always find the host's associated port and
> > the loop iteration stops.
> >
> > Signed-off-by: Robert Richter <[email protected]>
> > ---
> > drivers/cxl/core/port.c | 36 ++++++++++++++++++++++++------------
> > 1 file changed, 24 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index 61e9915162d5..08b99423dbf8 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -1084,6 +1084,22 @@ static struct device *grandparent(struct device *dev)
> > return NULL;
> > }
> >
> > +static struct device *cxl_mem_dport_dev(struct cxl_memdev *cxlmd)
> > +{
> > + struct device *dev = cxlmd->dev.parent;
> > + struct pci_dev *pdev = to_pci_dev(cxlmd->dev.parent);
>
> to_pci_dev(dev);
Ok.
>
> > +
> > + /*
> > + * An RCiEP is directly connected to the root bridge without
> > + * any PCI bridges/ports in between. Reduce the parent level
> > + * for those.
> > + */
> > + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END)
> > + return dev;
> > +
> > + return dev->parent;
> Switching from grandparent to this is a little confusing because it's
> done in two steps. Perhaps use
> return grandparent(cmlmd->dev);
> here to keep that connection and rename dev in this function to parent.
>
> Far too many devices in here with similar names for it to be easy
> to read.
Can improve this a little.
>
>
> > +}
> > +
> > static void delete_endpoint(void *data)
> > {
> > struct cxl_memdev *cxlmd = data;
> > @@ -1339,7 +1355,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
> > int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
> > {
> > struct device *dev = &cxlmd->dev;
> > - struct device *iter;
> > + struct device *dport_dev;
> > int rc;
> >
> > rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd);
> > @@ -1352,25 +1368,21 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
> > * attempt fails.
> > */
> > retry:
> > - for (iter = dev; iter; iter = grandparent(iter)) {
> > - struct device *dport_dev = grandparent(iter);
> > + for (dport_dev = cxl_mem_dport_dev(cxlmd); dport_dev;
> > + dport_dev = grandparent(dport_dev)) {
>
> I don't like looping for the RCD case as it relies a bit too
> much on subtle relationships between devices and parent.
>
> Perhaps better to factor out the contents of the loop, then handle
> the RCD case separately from the main loop.
> I haven't tried it, so perhaps that looks even less readable.
I see your point here, will take a look.
Thanks,
-Robert
On Thu, 1 Sep 2022 08:01:05 +0200
Robert Richter <[email protected]> wrote:
> On 31.08.22 11:08:04, Jonathan Cameron wrote:
> > On Wed, 31 Aug 2022 10:15:53 +0200
>
> > Robert Richter <[email protected]> wrote:
> >
> > > Restricted CXL device (RCD) mode (formerly CXL 1.1) uses a different
> > > enumeration scheme other than CXL VH (formerly CXL 2.0). In RCD mode a
> > > host/device (RCH-RCD) pair shows up as a legal PCIe hierarchy with an
> > > ACPI host bridge ("PNP0A08" or "ACPI0016" HID) and RCiEP connected to
> > > it with a description of the CXL device.
> > >
> > > Add function cxl_restricted_host_probe() to probe RCD enumerated
> > > devices. The function implements a loop that detects all CXL capable
> > > ACPI PCI root bridges in the system (RCD mode only). The iterator
> > > function cxl_find_next_rch() is introduced to walk through all of the
> > > CXL hosts. The loop will then enable all CXL devices connected to the
> > > host. For now, only implement an empty loop with an iterator that
> > > returns all pci host bridges in the system.
> > >
> > > The probe function is triggered by adding an own root device for RCHs.
> > > This is different to CXL VH where an ACPI "ACPI0017" root device
> > > exists. Its detection starts the CXL host detection. In RCD mode such
> > > a device does not necessarily exists, so solve this by creating a
> > > plain platform device that is not an ACPI device and is root only for
> > > RCHs.
> >
> > If I read this correctly that platform device is created whether or not
> > there are any cxl devices in the system?
> >
> > Can we create it only if we find some devices that will be placed
> > under it later?
>
> This would move the host detection from probe to init which I wanted
> to avoid to better control driver init order dependencies.
It's a bit nasty either way. I can see your reasoning, but
definitely not keen on it if there is a plausible way to avoid.
>
> I could add a put_device() at the end of a probe so that it will be
> released in case no other references use it. This implies the refcount
> is maintained for parent devices. Or this needs to be added to. So if
> there are no children (hosts) attached to the root device after probe,
> it will disappear.
Unless there is precedence for that, it'll be weird enough to be
hard to maintain. I guess I can live with the ugliness if we can't
add something new to ACPI to base this off.
>
> > > @@ -531,7 +566,41 @@ static struct platform_driver cxl_acpi_driver = {
> > > .id_table = cxl_test_ids,
> > > };
> > >
> > > -module_platform_driver(cxl_acpi_driver);
> > > +static void cxl_acpi_device_release(struct device *dev) { }
> >
> > Why the empty release? Perhaps introduce this only when it
> > does something.
>
> The core device driver requires this in device_release() to be setup.
>
> There is nothing to do as the device is kept in a static struct.
> That's why it's empty.
Ah got it. I'd failed to register the static structure.
>
> -Robert
On Thu, 1 Sep 2022 08:30:49 +0200
Robert Richter <[email protected]> wrote:
> On 31.08.22 11:52:24, Jonathan Cameron wrote:
> > On Wed, 31 Aug 2022 10:15:56 +0200
> > Robert Richter <[email protected]> wrote:
>
> > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > index ffdf439adb87..f9cdf23a91a8 100644
> > > --- a/drivers/cxl/acpi.c
> > > +++ b/drivers/cxl/acpi.c
> > > @@ -322,6 +322,8 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > > {
> > > struct pci_bus *bus = host ? host->bus : NULL;
> > > struct acpi_device *adev;
> > > + struct pci_dev *pdev;
> > > + bool is_restricted_host;
> > >
> > > while ((bus = pci_find_next_bus(bus)) != NULL) {
> > > host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
> > > @@ -343,6 +345,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > > dev_dbg(&host->dev, "PCI ACPI host found: %s\n",
> > > acpi_dev_name(adev));
> > >
> > > + /* Check CXL DVSEC of dev 0 func 0 */
> >
> > So assumption here is that the hostbridge has a one or more RCiEPs.
> > The spec (r3.0 9.11.4) allows for the EP to appear behind a root port
> > - that case always felt odd to me, so I'm fine with not supporting it until
> > we see a user.
>
> The software view of an RCD is always the same, it shows up always as
> an RCiEP. See figure 9-12 and 9-13 of the spec.
Ah. I see I misread the following from CXL r3.0 9.11.4
"This ACPI Host Bridge spawns a legal PCIe hierarchy. All PCIe Endpoints located in the
RCD are children of this ACPI Host Bridge. These Endpoints may appear directly on the
Root bus number or may appear behind a Root Port located on the Root bus."
I guess that is allowing 'additional' PCIe endpoints below a root port attached
to the ACPI Host Bridge, but not dev 0 func 0. That's a subtle distinction
I missed.
>
> -Robert
>
> >
> > > + 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);
> > > + pci_dev_put(pdev);
> > > +
> > > + if (!is_restricted_host)
> > > + continue;
> > > +
> > > + dev_dbg(&host->dev, "CXL restricted host found\n");
> > > +
> > > return host;
> > > }
On Thu, 1 Sep 2022 08:16:52 +0200
Robert Richter <[email protected]> wrote:
> On 31.08.22 11:20:28, Jonathan Cameron wrote:
> > Robert Richter <[email protected]> wrote:
>
> > > +static const struct acpi_device_id cxl_host_ids[] = {
> > > + { "ACPI0016", 0 },
> > > + { "PNP0A08", 0 },
> > > + { },
> >
> > Trivial but no comma after a null terminator. Always good to make
> > it harder for people to add things where they really shouldn't!
>
> Can do this.
>
> > pci_root.c avoids using an acpi_device_id table for similar matching.
> > I think the point being to separate probe type use of this table
> > from cases where we aren't using a normal device probe.
> > So to remain consistent with that, I would just grab the hid
> > and match it directly in this code.
>
> Grabbing the hid only is actually a violation of the acpi spec as a
> cid could be used interchangeable. It must also work then.
>
> It is also not possible to use something like probe or a handler
> matching the ids because the hosts must be enabled with the already
> existing drivers and handlers. Suppose there are multiple handlers for
> the same ids, the first handler wins and all other never get called.
>
> To me it looks sane and simple to use acpi_match_device_ids() here.
Ok. One for the ACPI maintainers to comment on if they wish - I'm fine with this
Reviewed-by: Jonathan Cameron <[email protected]>
>
> -Robert
>
> >
> > I don't feel that strongly about this if the ACPI maintainers are
> > fine with reusing this infrastructure as you have it here.
On Thu, 1 Sep 2022 08:38:52 +0200
Robert Richter <[email protected]> wrote:
> On 31.08.22 12:12:22, Jonathan Cameron wrote:
> > > On Wed, 31 Aug 2022 10:15:56 +0200
> > > Robert Richter <[email protected]> wrote:
>
> > > > @@ -322,6 +322,8 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > > > {
> > > > struct pci_bus *bus = host ? host->bus : NULL;
> > > > struct acpi_device *adev;
> > > > + struct pci_dev *pdev;
> > > > + bool is_restricted_host;
> > > >
> > > > while ((bus = pci_find_next_bus(bus)) != NULL) {
> > > > host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
> > > > @@ -343,6 +345,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > > > dev_dbg(&host->dev, "PCI ACPI host found: %s\n",
> > > > acpi_dev_name(adev));
> > > >
> > > > + /* Check CXL DVSEC of dev 0 func 0 */
> > >
> > > So assumption here is that the hostbridge has a one or more RCiEPs.
> > > The spec (r3.0 9.11.4) allows for the EP to appear behind a root port
> > > - that case always felt odd to me, so I'm fine with not supporting it until
> > > we see a user.
> > >
> > > > + 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);
> >
> > Thinking a bit more on this. I'm not sure this is sufficient.
> > Nothing in CXL 2.0 or later prevents true RCiEP devices (there are a
> > few references in CXL 3.0 e.g. 9.12.1 has RCDs or CXL RCiEPs - so just
> > detecting that there is one on the host bridge might not be sufficient
> > to distinguish this from a non RCH / RCB.
>
> An RCD has its own host bridge created (software view, not the phys
> topology). Host and device are paired in this case. Non-RCDs are
> standard endpoints and not RCiEPs, they have their own host.
I disagree. CXL spec does not exclude the possibility of real CXL
RCiEPs. So a CXL 2.0+ device that talks CXL configuration for some
reason but is part of the root complex itself (maybe a chiplet or
something where there isn't necessarily a real CXL bus involved).
Same reason we have RCiEPs in normal PCIe.
Chasing references - there is only one I can find (CXL r3.0 9.12.1)
"If a Host bridge is not associated with RCDs or CXL RCiEPs."
Both listed because they are different things.
(I think it's fine to say here that this has been queried in
appropriate place in the past and is something that is allowed).
So I still don't think the above check is sufficient'. If you
happen to have just one CXL 2.0+ RCiEP on a host bridge with
not root ports, then the check will identify it as a restriced
host. Maybe I'm missing another check that wouldn't though....
> There
> cannot be both types connected to the same host.
>
> Again, see figure 9-12 and 9-13.
Examples - don't show all the crazy things people are allowed to
build - you would need an awful lot of diagrams to do that.
>
> -Robert
>
> >
> > > > + pci_dev_put(pdev);
> > > > +
> > > > + if (!is_restricted_host)
> > > > + continue;
> > > > +
> > > > + dev_dbg(&host->dev, "CXL restricted host found\n");
> > > > +
> > > > return host;
> > > > }
> > > >
> > > > @@ -354,6 +370,7 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> > > > struct pci_host_bridge *host = NULL;
> > > >
> > > > while ((host = cxl_find_next_rch(host)) != NULL) {
> > > > + dev_info(&host->dev, "host supports CXL\n");
> > > > }
> > > >
> > > > return 0;
> > >
> >
On Thu, 1 Sep 2022 09:38:13 +0200
Robert Richter <[email protected]> wrote:
> On 31.08.22 12:56:56, Jonathan Cameron wrote:
> > On Wed, 31 Aug 2022 10:15:59 +0200
> > Robert Richter <[email protected]> wrote:
>
> > A few comments inline. Mostly requests for references for things
> > I couldn't find in the specs.
>
> Most of it comes from the pci base spec (5 or 6).
Ok. Extra references appreciated - these specs are huge, so saving
searching time always good!
>
> >
> >
> > > + *
> > > + * Also, RCRB accesses must use MMIO readl()/readq() to guarantee
> > > + * 32/64-bit access.
> > > + * CXL 8.2.2 - CXL 1.1 Upstream and Downstream Port Subsystem Component
> > > + * Registers
> > > + */
> > > + addr = ioremap(rcrb, PCI_BASE_ADDRESS_0 + SZ_8);
> > > + bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> > > + bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> >
> > The spec is a bit confusing on this, but I think you are reading into
> > MEMBAR0 of the RCRB, for which there isn't a lot of description other than
> > it being an address. It's referred to as a 64-bit BAR in places so you
> > might be right - or it might be intended to be a bare address..
> >
> > We might want a clarification on this...
> >
> > Also it's a 64 bit address so we need to read it in one go. However it's
> > referred to as a being a 64 bit address at 0x10 and 0x14 so who knows...
>
> This is part of the pci base spec and clearly defined there. There are
> also some similar implementation in the kernel already.
There isn't a cross reference from CXL spec and PCI doesn't use
the term membar.
I guess it is fairly obvious though that it's an abbreviation
of Base Address Register for Memory. I might raise the wish to tidy that
up for a future spec revision.
>
> >
> >
> > > + 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).
> >
> > The rcrb is 8k though I'm not immediately spotting an alignment requirement,
> > but I'm not sure the component regs have any restrictions do... Add a reference perhaps?
> > For non RCD devices there is a 64K alignment requirement, but I can't find
> > anything for RCDs (might just be missing it).
>
> This are the requirements of the pci base spec to membar ranges. The
> range size is power of 2 and base must be aligned to its size.
Ok. It feels convoluted to rely on the CXL glossary entry for BAR
to cover MEMBAR0 and hence inherit the restrictions of a PCIe bar.
Maybe just add a comment here so that anyone who hits this can understand
the source of the restriction seeing as it isn't in the CXL spec and this
isn't a PCI BAR.
>
> >
> > > + */
> > > + if (component_reg_phys & (SZ_8K - 1))
> > > + return CXL_RESOURCE_NONE;
> > > +
> > > + return component_reg_phys;
> > > +}
> > > +
> >
> > > + if (!base) {
> > > + dev_err(parent, "failed to map registers\n");
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + cxl_probe_component_regs(parent, base, &comp_map);
> > > + iounmap(base);
> > > +
> > > + if (!comp_map.hdm_decoder.valid) {
> > > + dev_err(parent, "HDM decoder registers not found\n");
> > > + return -ENXIO;
> >
> > Hmm. HDM decoder capability is optional for RCDs - might be using the
> > range registers. Seems like we'd really want to handle that for
> > RCDs. Future work I guess.
>
> I used the same message as for the non-RCD code path. HDM decoding is
> just a subset of features handled with component regs. We need to
> generalize the code for this in the future.
Sure - much more likely to need that generalized code for an RCD.
IIRC a CXL 2.0 device must implement HDM decoders, even though the
other path can be used by software that doesn't understand CXL 2.0.
Our RCD might be because the device is CXL 1.1...
>
> >
> > > + if (rc)
> > > + goto fail;
> >
> > > +
> > > dev_info(&host->dev, "host supports CXL\n");
> > > }
> > >
> > > return 0;
> > > +fail:
> >
> > Better to have a more specific error message and return directly above.
> > Note that so far vast majority of CXL error messages are dev_dbg,
> > so for consistency perhaps this should be as well.
> > (I prefer dev_err() but not my subsystem ;)
>
> There is more verbosity on the errors with dbg enabled. Note there are
> only a few dev_info/dev_err messages to not polute the logs. dev_err()
> is only used if something unexpected happens (e.g. the device exists
> but component regs are broken).
>
Ok. I'll leave the question of balance between the two for CXL maintainers
to comment on if they wish.
Thanks,
Jonathan
On Thu, 1 Sep 2022 08:53:36 +0200
Robert Richter <[email protected]> wrote:
> On 31.08.22 12:00:27, Jonathan Cameron wrote:
> > On Wed, 31 Aug 2022 10:15:57 +0200
> > Robert Richter <[email protected]> wrote:
> >
> > > The UID is needed to read the RCH's CEDT entry with the RCRB base
> > > address. Determine the host's UID from its ACPI fw node.
> > >
> > > Signed-off-by: Robert Richter <[email protected]>
> > > ---
> > > drivers/cxl/acpi.c | 12 ++++++++++++
> > > 1 file changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > index f9cdf23a91a8..b3146b7ae922 100644
> > > --- a/drivers/cxl/acpi.c
> > > +++ b/drivers/cxl/acpi.c
> > > @@ -368,8 +368,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > > static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> > > {
> > > struct pci_host_bridge *host = NULL;
> > > + struct acpi_device *adev;
> > > + unsigned long long uid = ~0;
> > >
> > > while ((host = cxl_find_next_rch(host)) != NULL) {
> > > + adev = ACPI_COMPANION(&host->dev);
> > > + if (!adev || !adev->pnp.unique_id ||
> > > + (kstrtoull(adev->pnp.unique_id, 10, &uid) < 0))
> >
> > There is an acpi_device_uid() accessor function that should probably be
> > used here.
>
> That accessor actually does not help really, there is no null pointer
> check for adev. Using it actually adds more complexity since another
> variable is introduced plus you need to look at the function's
> implementation anyway.
>
> The adev->pnp.unique_id access pattern is widely used in the kernel, I
> don't expect changes in the data struct here.
Ok.
>
> > Also, should a fialure to convert to an integer (or one within
> > limits) be something we paper over? Feels like we should fail
> > hard if that happens.
>
> This is a real corner case and close to a broken firmware
> implementation. I think current dbg messages are good to find where
> the detection stops.
Hmm. I don't like papering over such bugs as it leads to people not
fixing their bios as early as they otherwise would,
but fair enough I guess.
>
> > Admittedly I can't immediately find any spec that states that
> > the _UID should be either integer or under 32 bits...
> > ACPI allows a string and CXL just says it's 4 bytes long.
>
> IIRC the UID can be implemented as string or 8 bytes, there is no
> limitation then. That's why the range check below.
Ok.
All queries answered so
Reviewed-by: Jonathan Cameron <[email protected]>
>
> -Robert
>
> >
> > > + continue;
> > > +
> > > + dev_dbg(&adev->dev, "host uid: %llu\n", uid);
> > > +
> > > + if (uid > U32_MAX)
> > > + continue;
> > > +
> > > dev_info(&host->dev, "host supports CXL\n");
> > > }
> > >
> >
On 31.08.22 10:59:45, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 10:15:51 +0200
> Robert Richter <[email protected]> wrote:
>
> > 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.
> >
> > Signed-off-by: Robert Richter <[email protected]>
>
> This is one for Dan etc as it is mostly a question of how verbose we want
> the debug prints to be plus preference for caller or callee being
> responsible for outputting this sort of message.
>
> Patch looks good to me if we want to make this sort of change.
Should I take this as a Reviewed-by?
Thanks,
-Robert
On 01.09.22 11:10:38, Jonathan Cameron wrote:
> On Thu, 1 Sep 2022 08:01:05 +0200
> Robert Richter <[email protected]> wrote:
> > On 31.08.22 11:08:04, Jonathan Cameron wrote:
> > > On Wed, 31 Aug 2022 10:15:53 +0200
> > > Robert Richter <[email protected]> wrote:
> > > > The probe function is triggered by adding an own root device for RCHs.
> > > > This is different to CXL VH where an ACPI "ACPI0017" root device
> > > > exists. Its detection starts the CXL host detection. In RCD mode such
> > > > a device does not necessarily exists, so solve this by creating a
> > > > plain platform device that is not an ACPI device and is root only for
> > > > RCHs.
> > >
> > > If I read this correctly that platform device is created whether or not
> > > there are any cxl devices in the system?
> > >
> > > Can we create it only if we find some devices that will be placed
> > > under it later?
> >
> > This would move the host detection from probe to init which I wanted
> > to avoid to better control driver init order dependencies.
>
> It's a bit nasty either way. I can see your reasoning, but
> definitely not keen on it if there is a plausible way to avoid.
> >
> > I could add a put_device() at the end of a probe so that it will be
> > released in case no other references use it. This implies the refcount
> > is maintained for parent devices. Or this needs to be added to. So if
> > there are no children (hosts) attached to the root device after probe,
> > it will disappear.
>
> Unless there is precedence for that, it'll be weird enough to be
> hard to maintain. I guess I can live with the ugliness if we can't
> add something new to ACPI to base this off.
Let's stay with a put_device() for now. Then, we wont have a stale cxl
root device in the system in case there are no RCD children.
-Robert
On Tue, 6 Sep 2022 09:19:49 +0200
Robert Richter <[email protected]> wrote:
> On 01.09.22 11:10:38, Jonathan Cameron wrote:
> > On Thu, 1 Sep 2022 08:01:05 +0200
> > Robert Richter <[email protected]> wrote:
> > > On 31.08.22 11:08:04, Jonathan Cameron wrote:
> > > > On Wed, 31 Aug 2022 10:15:53 +0200
> > > > Robert Richter <[email protected]> wrote:
>
> > > > > The probe function is triggered by adding an own root device for RCHs.
> > > > > This is different to CXL VH where an ACPI "ACPI0017" root device
> > > > > exists. Its detection starts the CXL host detection. In RCD mode such
> > > > > a device does not necessarily exists, so solve this by creating a
> > > > > plain platform device that is not an ACPI device and is root only for
> > > > > RCHs.
> > > >
> > > > If I read this correctly that platform device is created whether or not
> > > > there are any cxl devices in the system?
> > > >
> > > > Can we create it only if we find some devices that will be placed
> > > > under it later?
> > >
> > > This would move the host detection from probe to init which I wanted
> > > to avoid to better control driver init order dependencies.
> >
> > It's a bit nasty either way. I can see your reasoning, but
> > definitely not keen on it if there is a plausible way to avoid.
> > >
> > > I could add a put_device() at the end of a probe so that it will be
> > > released in case no other references use it. This implies the refcount
> > > is maintained for parent devices. Or this needs to be added to. So if
> > > there are no children (hosts) attached to the root device after probe,
> > > it will disappear.
> >
> > Unless there is precedence for that, it'll be weird enough to be
> > hard to maintain. I guess I can live with the ugliness if we can't
> > add something new to ACPI to base this off.
>
> Let's stay with a put_device() for now. Then, we wont have a stale cxl
> root device in the system in case there are no RCD children.
>
> -Robert
Kind of obvious, but...
Make sure to call that out as an unusual thing to do via cover letter / patch
description so we hopefully get more eyes on this detail..
Jonathan
On Tue, 6 Sep 2022 09:30:37 +0200
Robert Richter <[email protected]> wrote:
> On 31.08.22 10:59:45, Jonathan Cameron wrote:
> > On Wed, 31 Aug 2022 10:15:51 +0200
> > Robert Richter <[email protected]> wrote:
> >
> > > 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.
> > >
> > > Signed-off-by: Robert Richter <[email protected]>
> >
> > This is one for Dan etc as it is mostly a question of how verbose we want
> > the debug prints to be plus preference for caller or callee being
> > responsible for outputting this sort of message.
> >
> > Patch looks good to me if we want to make this sort of change.
>
> Should I take this as a Reviewed-by?
Hmm. I guess I could go that far as its a policy decision rather than correctness
Reviewed-by: Jonathan Cameron <[email protected]>
>
> Thanks,
>
> -Robert
On 01.09.22 11:37:57, Jonathan Cameron wrote:
> On Thu, 1 Sep 2022 08:38:52 +0200
> Robert Richter <[email protected]> wrote:
>
> > On 31.08.22 12:12:22, Jonathan Cameron wrote:
> > > > On Wed, 31 Aug 2022 10:15:56 +0200
> > > > Robert Richter <[email protected]> wrote:
> >
> > > > > @@ -322,6 +322,8 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > > > > {
> > > > > struct pci_bus *bus = host ? host->bus : NULL;
> > > > > struct acpi_device *adev;
> > > > > + struct pci_dev *pdev;
> > > > > + bool is_restricted_host;
> > > > >
> > > > > while ((bus = pci_find_next_bus(bus)) != NULL) {
> > > > > host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
> > > > > @@ -343,6 +345,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > > > > dev_dbg(&host->dev, "PCI ACPI host found: %s\n",
> > > > > acpi_dev_name(adev));
> > > > >
> > > > > + /* Check CXL DVSEC of dev 0 func 0 */
> > > >
> > > > So assumption here is that the hostbridge has a one or more RCiEPs.
> > > > The spec (r3.0 9.11.4) allows for the EP to appear behind a root port
> > > > - that case always felt odd to me, so I'm fine with not supporting it until
> > > > we see a user.
> > > >
> > > > > + 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);
> > >
> > > Thinking a bit more on this. I'm not sure this is sufficient.
> > > Nothing in CXL 2.0 or later prevents true RCiEP devices (there are a
> > > few references in CXL 3.0 e.g. 9.12.1 has RCDs or CXL RCiEPs - so just
> > > detecting that there is one on the host bridge might not be sufficient
> > > to distinguish this from a non RCH / RCB.
> >
> > An RCD has its own host bridge created (software view, not the phys
> > topology). Host and device are paired in this case. Non-RCDs are
> > standard endpoints and not RCiEPs, they have their own host.
>
> I disagree. CXL spec does not exclude the possibility of real CXL
> RCiEPs. So a CXL 2.0+ device that talks CXL configuration for some
> reason but is part of the root complex itself (maybe a chiplet or
> something where there isn't necessarily a real CXL bus involved).
> Same reason we have RCiEPs in normal PCIe.
>
> Chasing references - there is only one I can find (CXL r3.0 9.12.1)
> "If a Host bridge is not associated with RCDs or CXL RCiEPs."
>
> Both listed because they are different things.
> (I think it's fine to say here that this has been queried in
> appropriate place in the past and is something that is allowed).
>
> So I still don't think the above check is sufficient'. If you
> happen to have just one CXL 2.0+ RCiEP on a host bridge with
> not root ports, then the check will identify it as a restriced
> host. Maybe I'm missing another check that wouldn't though....
>
> > There
> > cannot be both types connected to the same host.
> >
> > Again, see figure 9-12 and 9-13.
> Examples - don't show all the crazy things people are allowed to
> build - you would need an awful lot of diagrams to do that.
Right, there are references to CXL 2.0+ devices implemented as RCiEPs.
"9.12 CXL VH Enumeration" states that for the CXL Host Bridge
identification the CEDT should be used:
"""
CXL Early Discovery Table (CEDT) may be used to differentiate
between the three software concepts listed above.
"""
This check is added in patch #10 where the RCRB is extracted, so we
are good here.
-Robert
On 01.09.22 12:00:03, Jonathan Cameron wrote:
> On Thu, 1 Sep 2022 09:38:13 +0200
> Robert Richter <[email protected]> wrote:
>
> > On 31.08.22 12:56:56, Jonathan Cameron wrote:
> > > On Wed, 31 Aug 2022 10:15:59 +0200
> > > Robert Richter <[email protected]> wrote:
> >
> > > A few comments inline. Mostly requests for references for things
> > > I couldn't find in the specs.
> >
> > Most of it comes from the pci base spec (5 or 6).
>
> Ok. Extra references appreciated - these specs are huge, so saving
> searching time always good!
I will add comments for all ther refs in this patch.
[...]
> > > > + cxl_probe_component_regs(parent, base, &comp_map);
> > > > + iounmap(base);
> > > > +
> > > > + if (!comp_map.hdm_decoder.valid) {
> > > > + dev_err(parent, "HDM decoder registers not found\n");
> > > > + return -ENXIO;
> > >
> > > Hmm. HDM decoder capability is optional for RCDs - might be using the
> > > range registers. Seems like we'd really want to handle that for
> > > RCDs. Future work I guess.
> >
> > I used the same message as for the non-RCD code path. HDM decoding is
> > just a subset of features handled with component regs. We need to
> > generalize the code for this in the future.
>
> Sure - much more likely to need that generalized code for an RCD.
> IIRC a CXL 2.0 device must implement HDM decoders, even though the
> other path can be used by software that doesn't understand CXL 2.0.
> Our RCD might be because the device is CXL 1.1...
Yes, this will be a follow on series.
Thanks,
-Robert
On Wed, 31 Aug 2022, 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.
>
>Signed-off-by: Robert Richter <[email protected]>
>Reported-by: kernel test robot <[email protected]>
>Reviewed-by: Jonathan Cameron <[email protected]>
Reviewed-by: Davidlohr Bueso <[email protected]>
Does this want a
Fixes: d17d0540a0d (cxl/core/hdm: Add CXL standard decoder enumeration to the core)
tag?
>---
> drivers/cxl/cxl.h | 2 --
> 1 file changed, 2 deletions(-)
>
>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
>
On Wed, 31 Aug 2022, 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.
>
>Signed-off-by: Robert Richter <[email protected]>
Reviewed-by: Davidlohr Bueso <[email protected]>
On Wed, 31 Aug 2022, 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 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.
>
>Signed-off-by: Robert Richter <[email protected]>
>Reviewed-by: Jonathan Cameron <[email protected]>
Reviewed-by: Davidlohr Bueso <[email protected]>
On Wed, Aug 31, 2022 at 10:15:53AM +0200, Robert Richter wrote:
> Restricted CXL device (RCD) mode (formerly CXL 1.1) uses a different
> enumeration scheme other than CXL VH (formerly CXL 2.0). In RCD mode a
> host/device (RCH-RCD) pair shows up as a legal PCIe hierarchy with an
> ACPI host bridge ("PNP0A08" or "ACPI0016" HID) and RCiEP connected to
> it with a description of the CXL device.
>
> Add function cxl_restricted_host_probe() to probe RCD enumerated
> devices. The function implements a loop that detects all CXL capable
> ACPI PCI root bridges in the system (RCD mode only). The iterator
> function cxl_find_next_rch() is introduced to walk through all of the
> CXL hosts. The loop will then enable all CXL devices connected to the
> host. For now, only implement an empty loop with an iterator that
> returns all pci host bridges in the system.
>
> The probe function is triggered by adding an own root device for RCHs.
> This is different to CXL VH where an ACPI "ACPI0017" root device
> exists. Its detection starts the CXL host detection. In RCD mode such
> a device does not necessarily exists, so solve this by creating a
> plain platform device that is not an ACPI device and is root only for
> RCHs.
Drive-by nitpicks:
s/PCI root bridges/PCI host bridges/ to match other uses
s/pci host bridges/PCI host bridges/ to match other "PCI" uses
s/does not necessarily exists/does not necessarily exist/
On Wed, Aug 31, 2022 at 10:15:54AM +0200, Robert Richter wrote:
> A lookup of a host bridge's corresponding acpi device (struct
> acpi_device) is not possible, for example:
>
> adev = ACPI_COMPANION(&host_bridge->dev);
>
> This could be useful to find a host bridge's fwnode handle and to
> determine and call additional host bridge ACPI parameters and methods
> such as HID/CID or _UID.
>
> Make this work by linking the host bridge to its ACPI fw node.
s/acpi device/ACPI device/ to match other "ACPI" usage
s/fw node/fwnode/ (if it should match "fwnode handle" above)
I guess this patch makes ACPI_COMPANION() work where it didn't before,
right? E.g., the two ACPI_COMPANION() uses added by this series
(cxl_find_next_rch() and cxl_restricted_host_probe()).
I'm not really clear on the strategy of when we should use acpi_device
vs acpi_handle, but does this mean there's code in places like
pci_root.c that should be reworked to take advantage of this? That
code evaluates _DSM and _OSC, but I think it currently uses
acpi_handle for that.
Bjorn
On Wed, Sep 7, 2022 at 8:37 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Wed, Aug 31, 2022 at 10:15:54AM +0200, Robert Richter wrote:
> > A lookup of a host bridge's corresponding acpi device (struct
> > acpi_device) is not possible, for example:
> >
> > adev = ACPI_COMPANION(&host_bridge->dev);
> >
> > This could be useful to find a host bridge's fwnode handle and to
> > determine and call additional host bridge ACPI parameters and methods
> > such as HID/CID or _UID.
> >
> > Make this work by linking the host bridge to its ACPI fw node.
>
> s/acpi device/ACPI device/ to match other "ACPI" usage
> s/fw node/fwnode/ (if it should match "fwnode handle" above)
>
> I guess this patch makes ACPI_COMPANION() work where it didn't before,
> right? E.g., the two ACPI_COMPANION() uses added by this series
> (cxl_find_next_rch() and cxl_restricted_host_probe()).
>
> I'm not really clear on the strategy of when we should use acpi_device
> vs acpi_handle,
acpi_handle should be used for interactions with the ACPICA code, like
when AML is evaluated, and acpi_device for pretty much everything
else.
> but does this mean there's code in places like
> pci_root.c that should be reworked to take advantage of this? That
> code evaluates _DSM and _OSC, but I think it currently uses
> acpi_handle for that.
That's fine.
Robert Richter wrote:
> The physical base address of a CXL range can be invalid and is then
> set to CXL_RESOURCE_NONE. Early check this case before mapping a
> memory block in devm_cxl_iomap_block().
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> 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 39a129c57d40..f216c017a474 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -165,6 +165,9 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> void __iomem *ret_val;
> struct resource *res;
>
> + if (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
>
devm_request_mem_region() succeeds for you when this happens? More
details about the failure scenario please.
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 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.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/cxl/acpi.c | 2 --
> drivers/cxl/core/port.c | 39 ++++++++++++++++++++++++++++-----------
> 2 files changed, 28 insertions(+), 13 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..8604cda88787 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -666,13 +666,17 @@ 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 cxl_port *port, *parent_port;
> struct device *dev;
> int rc;
>
> + parent_port = parent_dport ? parent_dport->port : NULL;
> +
> port = cxl_port_alloc(uport, component_reg_phys, parent_dport);
> - if (IS_ERR(port))
> - return port;
> + if (IS_ERR(port)) {
> + rc = PTR_ERR(port);
> + goto err_out;
While I agree this unifies the error messaging I am not a fan of what
this does to the readability of the error exits. What about a compromise
of renaming devm_cxl_add_port to __devm_cxl_add_port() and add back a
new devm_cxl_add_port that does the unified debug messaging?
Apologies for the delay in getting to this I had hoped to be able to
finish up some other DAX work to focus on this, but time is getting
short so I will need to do both in parallel.
Robert Richter wrote:
> 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. In RCD mode an ACPI0017 device
> might not necessarily exist
That's a broken BIOS as far as I can see. No ACPI0017 == no OS CXL
services and the CXL aspects of the device need to be 100% managed by
the BIOS. You can still run the cxl_pci driver in that case for mailbox
operation, but error handling must be firmware-first without ACPI0017.
> PCIe host bridge PNP0A08 ID, there aren't any CXL port or switches in
> the PCIe hierarchy visible. As such the RCD mode enumeration and host
> discovery is very different from CXL VH. See patch #5 for
> implementation details.
>
> This implementation 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 #4 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 #5 to #15 contain
> the actual implementation of RCD mode support.
>
> [1] https://www.computeexpresslink.org/spec-landing
>
> Robert Richter (15):
> 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: Add probe function to detect restricted CXL hosts in RCD
> mode
> PCI/ACPI: Link host bridge to its ACPI fw node
> cxl/acpi: Check RCH's PCIe Host Bridge ACPI ID
> cxl/acpi: Check RCH's CXL DVSEC capabilities
> cxl/acpi: Determine PCI host bridge's ACPI UID
> cxl/acpi: Extract the RCH's RCRB base address from CEDT
> cxl/acpi: Extract the host's component register base address from RCRB
> cxl/acpi: Skip devm_cxl_port_enumerate_dports() when in RCD mode
> cxl/acpi: Rework devm_cxl_enumerate_ports() to support RCD mode
> cxl/acpi: Enumerate ports in RCD mode to enable RCHs and RCDs
> cxl/acpi: Specify module load order dependency for the cxl_acpi module
>
> drivers/acpi/pci_root.c | 1 +
> drivers/cxl/acpi.c | 311 ++++++++++++++++++++++++++++++++++-
> drivers/cxl/core/pci.c | 22 ++-
> drivers/cxl/core/port.c | 103 ++++++++----
> drivers/cxl/core/regs.c | 3 +
> drivers/cxl/cxl.h | 2 -
> drivers/cxl/mem.c | 1 +
> tools/testing/cxl/test/cxl.c | 8 +-
> 8 files changed, 400 insertions(+), 51 deletions(-)
>
> --
> 2.30.2
>
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.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/cxl/acpi.c | 7 ++-----
> drivers/cxl/core/pci.c | 2 --
> drivers/cxl/core/port.c | 28 ++++++++++++++++++++--------
> tools/testing/cxl/test/cxl.c | 8 +-------
> 4 files changed, 23 insertions(+), 22 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 8604cda88787..61e9915162d5 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -914,12 +914,16 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> }
>
> if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >=
> - CXL_TARGET_STRLEN)
> - return ERR_PTR(-EINVAL);
> + CXL_TARGET_STRLEN) {
> + rc = -EINVAL;
> + goto err;
> + }
>
> dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
> - if (!dport)
> - return ERR_PTR(-ENOMEM);
> + if (!dport) {
> + rc = -ENOMEM;
> + goto err;
> + }
Similar comment as before of using a goto just to ensure the log message
is called, I suspect a wrapper to do the logging ends up with less code
thrash.
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.
>
> Signed-off-by: Robert Richter <[email protected]>
Looks good,
Reviewed-by: Dan Williams <[email protected]>
Robert Richter wrote:
> An RCH is a root bridge and has "PNP0A08" or "ACPI0016" ACPI ID set.
> Check this.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/cxl/acpi.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index a19e3154dd44..ffdf439adb87 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -312,9 +312,16 @@ static int add_root_nvdimm_bridge(struct device *match, void *data)
> return 1;
> }
>
> +static const struct acpi_device_id cxl_host_ids[] = {
> + { "ACPI0016", 0 },
> + { "PNP0A08", 0 },
> + { },
> +};
> +
> struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> {
> struct pci_bus *bus = host ? host->bus : NULL;
> + struct acpi_device *adev;
>
> while ((bus = pci_find_next_bus(bus)) != NULL) {
> host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
> @@ -323,6 +330,19 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
>
> dev_dbg(&host->dev, "PCI bridge found\n");
>
> + /* Must be a root bridge */
> + if (host->bus->parent)
> + continue;
> +
> + dev_dbg(&host->dev, "PCI bridge is root bridge\n");
> +
> + adev = ACPI_COMPANION(&host->dev);
> + if (acpi_match_device_ids(adev, cxl_host_ids))
> + continue;
> +
> + dev_dbg(&host->dev, "PCI ACPI host found: %s\n",
> + acpi_dev_name(adev));
> +
Again, finding all the ACPI0016 devices is add_host_bridge_dport()
already does.
Robert Richter wrote:
> An RCH has an RCiEP connected to it with CXL DVSEC capabilities
> present and the CXL PCIe DVSEC included. Check this.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/cxl/acpi.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index ffdf439adb87..f9cdf23a91a8 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -322,6 +322,8 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> {
> struct pci_bus *bus = host ? host->bus : NULL;
> struct acpi_device *adev;
> + struct pci_dev *pdev;
> + bool is_restricted_host;
>
> while ((bus = pci_find_next_bus(bus)) != NULL) {
> host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
> @@ -343,6 +345,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> dev_dbg(&host->dev, "PCI ACPI host found: %s\n",
> acpi_dev_name(adev));
>
> + /* 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);
The check looks good, just the matter of integrating it into the
existing ACPI0016 device detection.
Robert Richter wrote:
> A lookup of a host bridge's corresponding acpi device (struct
> acpi_device) is not possible, for example:
>
> adev = ACPI_COMPANION(&host_bridge->dev);
>
> This could be useful to find a host bridge's fwnode handle and to
> determine and call additional host bridge ACPI parameters and methods
> such as HID/CID or _UID.
When is this explicitly needed. "Could be useful" is interesting, but it
needs to have a practical need.
>
> Make this work by linking the host bridge to its ACPI fw node.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/acpi/pci_root.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index d57cf8454b93..846c979e4c29 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -1083,6 +1083,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> goto out_release_info;
>
> host_bridge = to_pci_host_bridge(bus->bridge);
> + host_bridge->dev.fwnode = acpi_fwnode_handle(device);
> if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> host_bridge->native_pcie_hotplug = 0;
> if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
> --
> 2.30.2
>
Robert Richter wrote:
> Restricted CXL device (RCD) mode (formerly CXL 1.1) uses a different
> enumeration scheme other than CXL VH (formerly CXL 2.0). In RCD mode a
> host/device (RCH-RCD) pair shows up as a legal PCIe hierarchy with an
> ACPI host bridge ("PNP0A08" or "ACPI0016" HID) and RCiEP connected to
> it with a description of the CXL device.
>
> Add function cxl_restricted_host_probe() to probe RCD enumerated
> devices. The function implements a loop that detects all CXL capable
> ACPI PCI root bridges in the system (RCD mode only). The iterator
> function cxl_find_next_rch() is introduced to walk through all of the
> CXL hosts. The loop will then enable all CXL devices connected to the
> host. For now, only implement an empty loop with an iterator that
> returns all pci host bridges in the system.
>
> The probe function is triggered by adding an own root device for RCHs.
> This is different to CXL VH where an ACPI "ACPI0017" root device
> exists. Its detection starts the CXL host detection. In RCD mode such
> a device does not necessarily exists, so solve this by creating a
> plain platform device that is not an ACPI device and is root only for
> RCHs.
As I mentioned in the cover letter a BIOS that does not provide an
ACPI0017 device is opting out of the possibility OS first error handling
and other OS CXL services. ACPI0017 is mandatory.
Otherwise, its odd to have a module create the device that its driver is
going to drive. That's a backwards driver model and why we proposed
ACPI0017 in the first instance.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/cxl/acpi.c | 71 +++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 31e104f0210f..a19e3154dd44 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -312,6 +312,33 @@ static int add_root_nvdimm_bridge(struct device *match, void *data)
> return 1;
> }
>
> +struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> +{
> + struct pci_bus *bus = host ? host->bus : NULL;
> +
> + while ((bus = pci_find_next_bus(bus)) != NULL) {
> + host = bus ? to_pci_host_bridge(bus->bridge) : NULL;
> + if (!host)
> + continue;
> +
> + dev_dbg(&host->dev, "PCI bridge found\n");
> +
> + return host;
> + }
> +
> + return NULL;
> +}
> +
> +static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> +{
> + struct pci_host_bridge *host = NULL;
> +
> + while ((host = cxl_find_next_rch(host)) != NULL) {
> + }
> +
> + return 0;
> +}
> +
> static struct lock_class_key cxl_root_key;
>
> static void cxl_acpi_lock_reset_class(void *dev)
> @@ -445,6 +472,13 @@ static int cxl_acpi_probe(struct platform_device *pdev)
> struct acpi_device *adev = ACPI_COMPANION(host);
> struct cxl_cfmws_context ctx;
>
> + /*
> + * For RCH (CXL 1.1 hosts) the probe is triggered by a plain
> + * platform dev which does not have an acpi companion.
> + */
> + if (!adev)
> + return cxl_restricted_host_probe(pdev);
> +
> device_lock_set_class(&pdev->dev, &cxl_root_key);
> rc = devm_add_action_or_reset(&pdev->dev, cxl_acpi_lock_reset_class,
> &pdev->dev);
> @@ -518,6 +552,7 @@ MODULE_DEVICE_TABLE(acpi, cxl_acpi_ids);
>
> static const struct platform_device_id cxl_test_ids[] = {
> { "cxl_acpi" },
> + { "cxl_root" },
> { },
> };
> MODULE_DEVICE_TABLE(platform, cxl_test_ids);
> @@ -531,7 +566,41 @@ static struct platform_driver cxl_acpi_driver = {
> .id_table = cxl_test_ids,
> };
>
> -module_platform_driver(cxl_acpi_driver);
> +static void cxl_acpi_device_release(struct device *dev) { }
> +
> +static struct platform_device cxl_acpi_device = {
> + .name = "cxl_root",
> + .id = PLATFORM_DEVID_NONE,
> + .dev = {
> + .release = cxl_acpi_device_release,
> + }
> +};
> +
> +static int __init cxl_host_init(void)
> +{
> + int rc;
> +
> + /* Kick off restricted host (CXL 1.1) detection */
> + rc = platform_device_register(&cxl_acpi_device);
> + if (rc) {
> + platform_device_put(&cxl_acpi_device);
> + return rc;
> + }
> + rc = platform_driver_register(&cxl_acpi_driver);
> + if (rc)
> + platform_device_unregister(&cxl_acpi_device);
> + return rc;
> +}
> +
> +static void __exit cxl_host_exit(void)
> +{
> + platform_driver_unregister(&cxl_acpi_driver);
> + platform_device_unregister(&cxl_acpi_device);
> +}
> +
> +module_init(cxl_host_init);
> +module_exit(cxl_host_exit);
> +
> MODULE_LICENSE("GPL v2");
> MODULE_IMPORT_NS(CXL);
> MODULE_IMPORT_NS(ACPI);
> --
> 2.30.2
>
Robert Richter wrote:
> Restricted CXL device (RCD) mode (formerly CXL 1.1) uses a different
> enumeration scheme other than CXL VH (formerly CXL 2.0). In RCD mode a
> host/device (RCH-RCD) pair shows up as a legal PCIe hierarchy with an
> ACPI host bridge ("PNP0A08" or "ACPI0016" HID) and RCiEP connected to
> it with a description of the CXL device.
>
> Add function cxl_restricted_host_probe() to probe RCD enumerated
> devices. The function implements a loop that detects all CXL capable
> ACPI PCI root bridges in the system (RCD mode only). The iterator
> function cxl_find_next_rch() is introduced to walk through all of the
> CXL hosts. The loop will then enable all CXL devices connected to the
> host. For now, only implement an empty loop with an iterator that
> returns all pci host bridges in the system.
>
> The probe function is triggered by adding an own root device for RCHs.
> This is different to CXL VH where an ACPI "ACPI0017" root device
> exists. Its detection starts the CXL host detection. In RCD mode such
> a device does not necessarily exists, so solve this by creating a
> plain platform device that is not an ACPI device and is root only for
> RCHs.
These host bridges should be discovered by add_host_bridge_dport(), no?
Unless the BIOS is failing to emit the expected CEDT along with
ACPI0017.
Robert Richter wrote:
> Jonathan,
>
> On 31.08.22 13:23:29, Jonathan Cameron wrote:
> > On Wed, 31 Aug 2022 10:15:48 +0200
> > Robert Richter <[email protected]> wrote:
> >
> > > 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. In RCD mode an ACPI0017 device
> > > might not necessarily exist and the host bridge can have a standard
> > > PCIe host bridge PNP0A08 ID, there aren't any CXL port or switches in
> > > the PCIe hierarchy visible. As such the RCD mode enumeration and host
> > > discovery is very different from CXL VH. See patch #5 for
> > > implementation details.
> > >
> > > This implementation 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 #4 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 #5 to #15 contain
> > > the actual implementation of RCD mode support.
> > >
> > > [1] https://www.computeexpresslink.org/spec-landing
> >
> > Hi Robert,
> >
> > I'm curious on the aims of this work. Given expectation for RCDs is often
> > that the host firmware has set them up before the OS loads, what functionality
> > do you want to gain by mapping these into the CXL 2.0+ focused infrastructure?
> >
> > When I did some analysis a while back on CXL 1.1 I was pretty much assuming
> > that there was no real reason to let the OS know about it because it
> > couldn't do much of any use with the information. There are some corners
> > like RAS where it might be useful or perhaps to enable some of the CXL 3.0
> > features that are allowed to be EP only and so could be relevant for
> > an older host (e.g. CPMUs).
>
> though CXL RCD works with a legacy kernel or without any CXL
> functionality added, a CXL aware kernel can be useful also for RCD
> mode. RAS is a topic here but also gathering device information such
> as status or topology. Everything where access to the component
> register block or mailbox interface is required.
Unless the BIOS is going actively enable the standard CXL topology with
ACPI0017 then I think it should be hands off for the OS. The maintenance
burden of some of the hack to work around missing BIOS descriptions is
non-trivial, and it is still early days to encourage BIOS vendors to
enable what is needed and set end user expectations that these
pre-requisites exist.
As far as I can see this enabling adds an additional CXL "root" device
and I do not think userspace should need to care if a CXL 2.0 device is
attached to an RCH or not.
> Another plus, the CXL hierarchy becomes visible for RCD mode in sysfs
> and the device hierarchy.
>
> Reusing the existing infrastructure for this makes sense. Many
> features overlap in both modes (e.g. RAS, mailbox again, or topology
> information).
RAS only if OS first is supported by the BIOS. Mailbox support happens
with or without a CXL root device. The topology information is certainly
important in OS first error handling, but if its firmware first its
going to have its own FRU id scheme. Much of the common case topology
information for the RCH case (like which RCIEP is hosting which CXL address
range) is covered by this pending lspci update:
https://github.com/pciutils/pciutils/pull/59:
...although that needs some help to get over the goal line.
Otherwise the topology information is mostly for describing all the
degrees of freedom of a full blown CXL 2.0 topoloy with host bridge and
switch interleaving.
Robert Richter wrote:
> The UID is needed to read the RCH's CEDT entry with the RCRB base
> address. Determine the host's UID from its ACPI fw node.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/cxl/acpi.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index f9cdf23a91a8..b3146b7ae922 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -368,8 +368,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> {
> struct pci_host_bridge *host = NULL;
> + struct acpi_device *adev;
> + unsigned long long uid = ~0;
>
> while ((host = cxl_find_next_rch(host)) != NULL) {
> + adev = ACPI_COMPANION(&host->dev);
> + if (!adev || !adev->pnp.unique_id ||
> + (kstrtoull(adev->pnp.unique_id, 10, &uid) < 0))
> + continue;
> +
> + dev_dbg(&adev->dev, "host uid: %llu\n", uid);
> +
> + if (uid > U32_MAX)
> + continue;
Looks redundant with existing _UID matching.
On Wed, Aug 31, 2022 at 10:17 AM Robert Richter <[email protected]> wrote:
>
> A lookup of a host bridge's corresponding acpi device (struct
> acpi_device) is not possible, for example:
>
> adev = ACPI_COMPANION(&host_bridge->dev);
Hmm.
x86 has this code in pcibios_root_bridge_prepare():
if (!bridge->dev.parent) {
struct pci_sysdata *sd = bridge->bus->sysdata;
ACPI_COMPANION_SET(&bridge->dev, sd->companion);
}
which should set the ACPI companion for the host bridge.
Moreover, on my x86 desktop /sys/devices/pci0000\:00/ (which is the
host bridge AFAICS) has
firmware_node -> ../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00
so clearly the ACPI companion is there.
Are we talking about ARM64 here?
> This could be useful to find a host bridge's fwnode handle and to
> determine and call additional host bridge ACPI parameters and methods
> such as HID/CID or _UID.
>
> Make this work by linking the host bridge to its ACPI fw node.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/acpi/pci_root.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index d57cf8454b93..846c979e4c29 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -1083,6 +1083,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> goto out_release_info;
>
> host_bridge = to_pci_host_bridge(bus->bridge);
> + host_bridge->dev.fwnode = acpi_fwnode_handle(device);
So this would be replacing the existing mechanism on x86, right?
> if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> host_bridge->native_pcie_hotplug = 0;
> if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
> --
On Thu, Sep 8, 2022 at 8:05 AM Dan Williams <[email protected]> wrote:
>
> Robert Richter wrote:
> > A lookup of a host bridge's corresponding acpi device (struct
> > acpi_device) is not possible, for example:
> >
> > adev = ACPI_COMPANION(&host_bridge->dev);
> >
> > This could be useful to find a host bridge's fwnode handle and to
> > determine and call additional host bridge ACPI parameters and methods
> > such as HID/CID or _UID.
>
> When is this explicitly needed. "Could be useful" is interesting, but it
> needs to have a practical need.
It is needed and it is present on x86 AFAICS (see my last reply in this thread).
This seems to be addressing an ARM64-specific issue.
> >
> > Make this work by linking the host bridge to its ACPI fw node.
> >
> > Signed-off-by: Robert Richter <[email protected]>
> > ---
> > drivers/acpi/pci_root.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index d57cf8454b93..846c979e4c29 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -1083,6 +1083,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> > goto out_release_info;
> >
> > host_bridge = to_pci_host_bridge(bus->bridge);
> > + host_bridge->dev.fwnode = acpi_fwnode_handle(device);
> > if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> > host_bridge->native_pcie_hotplug = 0;
> > if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
> > --
> > 2.30.2
> >
>
>
On 07.09.22 22:44:01, 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.
> >
> > Signed-off-by: Robert Richter <[email protected]>
>
> Looks good,
>
> Reviewed-by: Dan Williams <[email protected]>
Note there was a 0day build error. So I will add the inclusion of
"core.h" to core/regs.c to this patch. I hope that is ok.
Thanks,
-Robert
> On Sep 7, 2022, at 10:43 PM, Dan Williams <[email protected]> wrote:
>
> Apologies for the delay in getting to this I had hoped to be able to
> finish up some other DAX work to focus on this, but time is getting
> short so I will need to do both in parallel.
>
> Robert Richter wrote:
>> 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. In RCD mode an ACPI0017 device
>> might not necessarily exist
>
> That's a broken BIOS as far as I can see. No ACPI0017 == no OS CXL
> services and the CXL aspects of the device need to be 100% managed by
> the BIOS. You can still run the cxl_pci driver in that case for mailbox
> operation, but error handling must be firmware-first without ACPI0017.
Firmware-first or OS-first applies to CXL protocol error handling. For CXL
memory error handling, the device generates a DRAM error record, the OS
parses such record and act accordingly. According to CXL spec (section
8.2.9.2.1.2 DRAM Event Record), DPA but not HPA is in such record. The OS
needs to translate such DPA into HPA to act on. I am taking this as an example
to show that OS CXL services is needed.
Instead of using ACPI0016 to tell whether the system is under RCH mode,
I suppose one way is to check “CXL version” field of CHBS structure in CEDT?
>
>> PCIe host bridge PNP0A08 ID, there aren't any CXL port or switches in
>> the PCIe hierarchy visible. As such the RCD mode enumeration and host
>> discovery is very different from CXL VH. See patch #5 for
>> implementation details.
>>
>> This implementation 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 #4 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 #5 to #15 contain
>> the actual implementation of RCD mode support.
>>
>> [1] https://www.computeexpresslink.org/spec-landing
>>
>> Robert Richter (15):
>> 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: Add probe function to detect restricted CXL hosts in RCD
>> mode
>> PCI/ACPI: Link host bridge to its ACPI fw node
>> cxl/acpi: Check RCH's PCIe Host Bridge ACPI ID
>> cxl/acpi: Check RCH's CXL DVSEC capabilities
>> cxl/acpi: Determine PCI host bridge's ACPI UID
>> cxl/acpi: Extract the RCH's RCRB base address from CEDT
>> cxl/acpi: Extract the host's component register base address from RCRB
>> cxl/acpi: Skip devm_cxl_port_enumerate_dports() when in RCD mode
>> cxl/acpi: Rework devm_cxl_enumerate_ports() to support RCD mode
>> cxl/acpi: Enumerate ports in RCD mode to enable RCHs and RCDs
>> cxl/acpi: Specify module load order dependency for the cxl_acpi module
>>
>> drivers/acpi/pci_root.c | 1 +
>> drivers/cxl/acpi.c | 311 ++++++++++++++++++++++++++++++++++-
>> drivers/cxl/core/pci.c | 22 ++-
>> drivers/cxl/core/port.c | 103 ++++++++----
>> drivers/cxl/core/regs.c | 3 +
>> drivers/cxl/cxl.h | 2 -
>> drivers/cxl/mem.c | 1 +
>> tools/testing/cxl/test/cxl.c | 8 +-
>> 8 files changed, 400 insertions(+), 51 deletions(-)
>>
>> --
>> 2.30.2
>>
>
>
>
Rafael J. Wysocki wrote:
> On Thu, Sep 8, 2022 at 8:05 AM Dan Williams <[email protected]> wrote:
> >
> > Robert Richter wrote:
> > > A lookup of a host bridge's corresponding acpi device (struct
> > > acpi_device) is not possible, for example:
> > >
> > > adev = ACPI_COMPANION(&host_bridge->dev);
> > >
> > > This could be useful to find a host bridge's fwnode handle and to
> > > determine and call additional host bridge ACPI parameters and methods
> > > such as HID/CID or _UID.
> >
> > When is this explicitly needed. "Could be useful" is interesting, but it
> > needs to have a practical need.
>
> It is needed and it is present on x86 AFAICS (see my last reply in this thread).
>
> This seems to be addressing an ARM64-specific issue.
>
Ah, ok, thanks for pointing that out.
Robert Richter wrote:
> On 07.09.22 22:44:01, 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.
> > >
> > > Signed-off-by: Robert Richter <[email protected]>
> >
> > Looks good,
> >
> > Reviewed-by: Dan Williams <[email protected]>
>
> Note there was a 0day build error. So I will add the inclusion of
> "core.h" to core/regs.c to this patch. I hope that is ok.
Yes, for my Reviewed-by: small fixups that don't touch the core theme of
the patch are ok.
Jonathan Zhang (Infra) wrote:
>
>
> > On Sep 7, 2022, at 10:43 PM, Dan Williams <[email protected]> wrote:
> >
> > Apologies for the delay in getting to this I had hoped to be able to
> > finish up some other DAX work to focus on this, but time is getting
> > short so I will need to do both in parallel.
> >
> > Robert Richter wrote:
> >> 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. In RCD mode an ACPI0017 device
> >> might not necessarily exist
> >
> > That's a broken BIOS as far as I can see. No ACPI0017 == no OS CXL
> > services and the CXL aspects of the device need to be 100% managed by
> > the BIOS. You can still run the cxl_pci driver in that case for mailbox
> > operation, but error handling must be firmware-first without ACPI0017.
> Firmware-first or OS-first applies to CXL protocol error handling. For CXL
> memory error handling, the device generates a DRAM error record, the OS
> parses such record and act accordingly. According to CXL spec (section
> 8.2.9.2.1.2 DRAM Event Record), DPA but not HPA is in such record. The OS
> needs to translate such DPA into HPA to act on. I am taking this as an example
> to show that OS CXL services is needed.
> Instead of using ACPI0016 to tell whether the system is under RCH mode,
> I suppose one way is to check “CXL version” field of CHBS structure in CEDT?
Unless the OS has negotiated CXL _OSC the BIOS owns the event retrieval
and translating it from DPA to HPA. I do want to add OS CXL services to
Linux, but only in the case when the BIOS is actively enabling OS native
address translation which includes populating ACPI0017, CFMWS, and
devices with the HDM decoder capability registers instead of DVSEC range
registers. Everything else is early-gen CXL that is 100% BIOS supported,
similar to DDR where a driver is not expected.
> On Aug 31, 2022, at 1:15 AM, Robert Richter <[email protected]> wrote:
>
> The UID is needed to read the RCH's CEDT entry with the RCRB base
> address. Determine the host's UID from its ACPI fw node.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/cxl/acpi.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index f9cdf23a91a8..b3146b7ae922 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -368,8 +368,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> {
> struct pci_host_bridge *host = NULL;
> + struct acpi_device *adev;
> + unsigned long long uid = ~0;
>
> while ((host = cxl_find_next_rch(host)) != NULL) {
> + adev = ACPI_COMPANION(&host->dev);
> + if (!adev || !adev->pnp.unique_id ||
> + (kstrtoull(adev->pnp.unique_id, 10, &uid) < 0))
The UID field holds 4 bytes of ASCII characters. kstrtoull() would fail
if the UID field has value such as ‘CX03’.
> + continue;
> +
> + dev_dbg(&adev->dev, "host uid: %llu\n", uid);
> +
> + if (uid > U32_MAX)
> + continue;
> +
> dev_info(&host->dev, "host supports CXL\n");
> }
>
> --
> 2.30.2
>
>
> On Aug 31, 2022, at 1:15 AM, Robert Richter <[email protected]> wrote:
>
> A downstream port must be connected to a component register block.
> Determine its base address from the RCRB.
>
> The implementation is analog to how cxl_setup_regs() is implemented
> for CXL VH mode. A struct cxl_component_reg_map is filled in, mapped
> and probed.
>
> Signed-off-by: Terry Bowman <[email protected]>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/cxl/acpi.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 80 insertions(+)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 439df9df2741..88bbd2bb61fc 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -401,12 +401,84 @@ static resource_size_t cxl_get_rcrb(u32 uid)
> return ctx.chbcr;
> }
>
> +static resource_size_t cxl_get_component_reg_phys(resource_size_t rcrb)
> +{
> + resource_size_t component_reg_phys;
> + u32 bar0, bar1;
> + void *addr;
> +
> + /*
> + * RCRB's BAR[0..1] point to component block containing CXL subsystem
> + * component registers.
> + * CXL 8.2.4 - Component Register Layout Definition.
> + *
> + * Also, RCRB accesses must use MMIO readl()/readq() to guarantee
> + * 32/64-bit access.
> + * CXL 8.2.2 - CXL 1.1 Upstream and Downstream Port Subsystem Component
> + * Registers
> + */
> + 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 & (SZ_8K - 1))
> + return CXL_RESOURCE_NONE;
> +
> + return component_reg_phys;
> +}
> +
> +static int cxl_setup_component_reg(struct device *parent,
> + resource_size_t component_reg_phys)
> +{
> + struct cxl_component_reg_map comp_map;
> + void __iomem *base;
> +
> + if (component_reg_phys == CXL_RESOURCE_NONE)
> + return -EINVAL;
> +
> + base = ioremap(component_reg_phys, SZ_64K);
> + if (!base) {
> + dev_err(parent, "failed to map registers\n");
> + return -ENOMEM;
> + }
> +
> + cxl_probe_component_regs(parent, base, &comp_map);
> + iounmap(base);
> +
> + if (!comp_map.hdm_decoder.valid) {
> + dev_err(parent, "HDM decoder registers not found\n");
> + return -ENXIO;
> + }
> +
> + dev_dbg(parent, "Set up component registers\n");
> +
> + return 0;
> +}
> +
> static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> {
> struct pci_host_bridge *host = NULL;
> struct acpi_device *adev;
> unsigned long long uid = ~0;
> resource_size_t rcrb;
> + resource_size_t component_reg_phys;
> + int rc;
>
> while ((host = cxl_find_next_rch(host)) != NULL) {
> adev = ACPI_COMPANION(&host->dev);
> @@ -425,10 +497,18 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
>
> dev_dbg(&host->dev, "RCRB found: 0x%08llx\n", (u64)rcrb);
>
> + component_reg_phys = cxl_get_component_reg_phys(rcrb);
> + rc = cxl_setup_component_reg(&host->dev, component_reg_phys);
cxl_setup_component_reg() calls cxl_probe_component_regs() which depends on the existence
of HDM decoder capability register. Such register does not exist when the device is in RCRB
mode, attached to a RCH.
> + if (rc)
> + goto fail;
> +
> dev_info(&host->dev, "host supports CXL\n");
> }
>
> return 0;
> +fail:
> + dev_err(&host->dev, "failed to initialize CXL host: %d\n", rc);
> + return rc;
> }
>
> static struct lock_class_key cxl_root_key;
> --
> 2.30.2
>
>
> On Sep 8, 2022, at 12:51 PM, Dan Williams <[email protected]> wrote:
>
> !-------------------------------------------------------------------|
> This Message Is From an External Sender
>
> |-------------------------------------------------------------------!
>
> Jonathan Zhang (Infra) wrote:
>>
>>
>>> On Sep 7, 2022, at 10:43 PM, Dan Williams <[email protected]> wrote:
>>>
>>> Apologies for the delay in getting to this I had hoped to be able to
>>> finish up some other DAX work to focus on this, but time is getting
>>> short so I will need to do both in parallel.
>>>
>>> Robert Richter wrote:
>>>> 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. In RCD mode an ACPI0017 device
>>>> might not necessarily exist
>>>
>>> That's a broken BIOS as far as I can see. No ACPI0017 == no OS CXL
>>> services and the CXL aspects of the device need to be 100% managed by
>>> the BIOS. You can still run the cxl_pci driver in that case for mailbox
>>> operation, but error handling must be firmware-first without ACPI0017.
>> Firmware-first or OS-first applies to CXL protocol error handling. For CXL
>> memory error handling, the device generates a DRAM error record, the OS
>> parses such record and act accordingly. According to CXL spec (section
>> 8.2.9.2.1.2 DRAM Event Record), DPA but not HPA is in such record. The OS
>> needs to translate such DPA into HPA to act on. I am taking this as an example
>> to show that OS CXL services is needed.
>> Instead of using ACPI0016 to tell whether the system is under RCH mode,
>> I suppose one way is to check “CXL version” field of CHBS structure in CEDT?
>
> Unless the OS has negotiated CXL _OSC the BIOS owns the event retrieval
> and translating it from DPA to HPA. I do want to add OS CXL services to
> Linux, but only in the case when the BIOS is actively enabling OS native
> address translation which includes populating ACPI0017, CFMWS, and
> devices with the HDM decoder capability registers instead of DVSEC range
> registers. Everything else is early-gen CXL that is 100% BIOS supported,
> similar to DDR where a driver is not expected.
It makes sense that the BIOS and OS need to negotiate CXL _OSC so that OS
would take care of address translation. That being said, only DVSEC range
register (but not HDM decoder capability register) is available when the device is in
RCRB mode (section 9.11.8 figure 9-7) attached to a RCH. This type of
configuration needs to be supported with OS CXL service.
Jonathan Zhang (Infra) wrote:
>
>
> > On Aug 31, 2022, at 1:15 AM, Robert Richter <[email protected]> wrote:
> >
> > The UID is needed to read the RCH's CEDT entry with the RCRB base
> > address. Determine the host's UID from its ACPI fw node.
> >
> > Signed-off-by: Robert Richter <[email protected]>
> > ---
> > drivers/cxl/acpi.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index f9cdf23a91a8..b3146b7ae922 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -368,8 +368,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> > static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> > {
> > struct pci_host_bridge *host = NULL;
> > + struct acpi_device *adev;
> > + unsigned long long uid = ~0;
> >
> > while ((host = cxl_find_next_rch(host)) != NULL) {
> > + adev = ACPI_COMPANION(&host->dev);
> > + if (!adev || !adev->pnp.unique_id ||
> > + (kstrtoull(adev->pnp.unique_id, 10, &uid) < 0))
> The UID field holds 4 bytes of ASCII characters. kstrtoull() would fail
> if the UID field has value such as ‘CX03’.
The UID field is not 4 ASCII characters.
We went through this before in the original code in
drivers/cxl/acpi.c::add_host_bridge_dport().
The CEDT.CHBS defines _UID as an integer so use acpi_evaluate_integer()
to retrieve the UID to perform the comparison. I thought there was an
errata filed to clarify this, but it seems the current spec still just
says "value". The CFMWS also places _UID values in the target list,
those are also handled as integers.
Jonathan Zhang (Infra) wrote:
>
>
> > On Sep 8, 2022, at 12:51 PM, Dan Williams <[email protected]> wrote:
> >
> > !-------------------------------------------------------------------|
> > This Message Is From an External Sender
> >
> > |-------------------------------------------------------------------!
> >
> > Jonathan Zhang (Infra) wrote:
> >>
> >>
> >>> On Sep 7, 2022, at 10:43 PM, Dan Williams <[email protected]> wrote:
> >>>
> >>> Apologies for the delay in getting to this I had hoped to be able to
> >>> finish up some other DAX work to focus on this, but time is getting
> >>> short so I will need to do both in parallel.
> >>>
> >>> Robert Richter wrote:
> >>>> 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. In RCD mode an ACPI0017 device
> >>>> might not necessarily exist
> >>>
> >>> That's a broken BIOS as far as I can see. No ACPI0017 == no OS CXL
> >>> services and the CXL aspects of the device need to be 100% managed by
> >>> the BIOS. You can still run the cxl_pci driver in that case for mailbox
> >>> operation, but error handling must be firmware-first without ACPI0017.
> >> Firmware-first or OS-first applies to CXL protocol error handling. For CXL
> >> memory error handling, the device generates a DRAM error record, the OS
> >> parses such record and act accordingly. According to CXL spec (section
> >> 8.2.9.2.1.2 DRAM Event Record), DPA but not HPA is in such record. The OS
> >> needs to translate such DPA into HPA to act on. I am taking this as an example
> >> to show that OS CXL services is needed.
> >> Instead of using ACPI0016 to tell whether the system is under RCH mode,
> >> I suppose one way is to check “CXL version” field of CHBS structure in CEDT?
> >
> > Unless the OS has negotiated CXL _OSC the BIOS owns the event retrieval
> > and translating it from DPA to HPA. I do want to add OS CXL services to
> > Linux, but only in the case when the BIOS is actively enabling OS native
> > address translation which includes populating ACPI0017, CFMWS, and
> > devices with the HDM decoder capability registers instead of DVSEC range
> > registers. Everything else is early-gen CXL that is 100% BIOS supported,
> > similar to DDR where a driver is not expected.
>
>
> It makes sense that the BIOS and OS need to negotiate CXL _OSC so that OS
> would take care of address translation. That being said, only DVSEC range
> register (but not HDM decoder capability register) is available when the device is in
> RCRB mode (section 9.11.8 figure 9-7) attached to a RCH. This type of
> configuration needs to be supported with OS CXL service.
>
So that figure does have the HDM capabilty pictured in the RCD upstream
port. However, Table 8-22 does seem to incidate that Type 3 D1 devices
are not permitted to have an HDM Decoder Capabilitiy Structure.
However that then leave me confused about figure 9-8 as that shows an
HDM decoder capability in the BAR and not the RCRB. Is that picture
wrong with respect what Table 8-22 indicates?
> On Sep 8, 2022, at 2:10 PM, Dan Williams <[email protected]> wrote:
>
> !-------------------------------------------------------------------|
> This Message Is From an External Sender
>
> |-------------------------------------------------------------------!
>
> Jonathan Zhang (Infra) wrote:
>>
>>
>>> On Aug 31, 2022, at 1:15 AM, Robert Richter <[email protected]> wrote:
>>>
>>> The UID is needed to read the RCH's CEDT entry with the RCRB base
>>> address. Determine the host's UID from its ACPI fw node.
>>>
>>> Signed-off-by: Robert Richter <[email protected]>
>>> ---
>>> drivers/cxl/acpi.c | 12 ++++++++++++
>>> 1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
>>> index f9cdf23a91a8..b3146b7ae922 100644
>>> --- a/drivers/cxl/acpi.c
>>> +++ b/drivers/cxl/acpi.c
>>> @@ -368,8 +368,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
>>> static int __init cxl_restricted_host_probe(struct platform_device *pdev)
>>> {
>>> struct pci_host_bridge *host = NULL;
>>> + struct acpi_device *adev;
>>> + unsigned long long uid = ~0;
>>>
>>> while ((host = cxl_find_next_rch(host)) != NULL) {
>>> + adev = ACPI_COMPANION(&host->dev);
>>> + if (!adev || !adev->pnp.unique_id ||
>>> + (kstrtoull(adev->pnp.unique_id, 10, &uid) < 0))
>> The UID field holds 4 bytes of ASCII characters. kstrtoull() would fail
>> if the UID field has value such as ‘CX03’.
>
> The UID field is not 4 ASCII characters.
>
> We went through this before in the original code in
> drivers/cxl/acpi.c::add_host_bridge_dport().
>
> The CEDT.CHBS defines _UID as an integer so use acpi_evaluate_integer()
> to retrieve the UID to perform the comparison. I thought there was an
> errata filed to clarify this, but it seems the current spec still just
> says "value". The CFMWS also places _UID values in the target list,
> those are also handled as integers.
ACPI 6.4 spec section 6.1.12 describes _UID, it says the return value is:
An Integer or String containing the Unique ID.
In the BIOS I see, the _UIDs of PCIe devices hold ASCII characters (not NULL
terminated).
Jonathan Zhang (Infra) wrote:
>
>
> > On Sep 8, 2022, at 2:10 PM, Dan Williams <[email protected]> wrote:
> >
> > !-------------------------------------------------------------------|
> > This Message Is From an External Sender
> >
> > |-------------------------------------------------------------------!
> >
> > Jonathan Zhang (Infra) wrote:
> >>
> >>
> >>> On Aug 31, 2022, at 1:15 AM, Robert Richter <[email protected]> wrote:
> >>>
> >>> The UID is needed to read the RCH's CEDT entry with the RCRB base
> >>> address. Determine the host's UID from its ACPI fw node.
> >>>
> >>> Signed-off-by: Robert Richter <[email protected]>
> >>> ---
> >>> drivers/cxl/acpi.c | 12 ++++++++++++
> >>> 1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> >>> index f9cdf23a91a8..b3146b7ae922 100644
> >>> --- a/drivers/cxl/acpi.c
> >>> +++ b/drivers/cxl/acpi.c
> >>> @@ -368,8 +368,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> >>> static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> >>> {
> >>> struct pci_host_bridge *host = NULL;
> >>> + struct acpi_device *adev;
> >>> + unsigned long long uid = ~0;
> >>>
> >>> while ((host = cxl_find_next_rch(host)) != NULL) {
> >>> + adev = ACPI_COMPANION(&host->dev);
> >>> + if (!adev || !adev->pnp.unique_id ||
> >>> + (kstrtoull(adev->pnp.unique_id, 10, &uid) < 0))
> >> The UID field holds 4 bytes of ASCII characters. kstrtoull() would fail
> >> if the UID field has value such as ‘CX03’.
> >
> > The UID field is not 4 ASCII characters.
> >
> > We went through this before in the original code in
> > drivers/cxl/acpi.c::add_host_bridge_dport().
> >
> > The CEDT.CHBS defines _UID as an integer so use acpi_evaluate_integer()
> > to retrieve the UID to perform the comparison. I thought there was an
> > errata filed to clarify this, but it seems the current spec still just
> > says "value". The CFMWS also places _UID values in the target list,
> > those are also handled as integers.
>
> ACPI 6.4 spec section 6.1.12 describes _UID, it says the return value is:
> An Integer or String containing the Unique ID.
>
> In the BIOS I see, the _UIDs of PCIe devices hold ASCII characters (not NULL
> terminated).
ASCII characters without NULL termination means that data can be
treated as binary data which is what current CFMWWS parsing code chooses
to do. I think a spec clarification is needed to make resolve the
ambiguity.
> On Sep 8, 2022, at 3:31 PM, Dan Williams <[email protected]> wrote:
>
> !-------------------------------------------------------------------|
> This Message Is From an External Sender
>
> |-------------------------------------------------------------------!
>
> Jonathan Zhang (Infra) wrote:
>>
>>
>>> On Sep 8, 2022, at 2:10 PM, Dan Williams <[email protected]> wrote:
>>>
>>> !-------------------------------------------------------------------|
>>> This Message Is From an External Sender
>>>
>>> |-------------------------------------------------------------------!
>>>
>>> Jonathan Zhang (Infra) wrote:
>>>>
>>>>
>>>>> On Aug 31, 2022, at 1:15 AM, Robert Richter <[email protected]> wrote:
>>>>>
>>>>> The UID is needed to read the RCH's CEDT entry with the RCRB base
>>>>> address. Determine the host's UID from its ACPI fw node.
>>>>>
>>>>> Signed-off-by: Robert Richter <[email protected]>
>>>>> ---
>>>>> drivers/cxl/acpi.c | 12 ++++++++++++
>>>>> 1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
>>>>> index f9cdf23a91a8..b3146b7ae922 100644
>>>>> --- a/drivers/cxl/acpi.c
>>>>> +++ b/drivers/cxl/acpi.c
>>>>> @@ -368,8 +368,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
>>>>> static int __init cxl_restricted_host_probe(struct platform_device *pdev)
>>>>> {
>>>>> struct pci_host_bridge *host = NULL;
>>>>> + struct acpi_device *adev;
>>>>> + unsigned long long uid = ~0;
>>>>>
>>>>> while ((host = cxl_find_next_rch(host)) != NULL) {
>>>>> + adev = ACPI_COMPANION(&host->dev);
>>>>> + if (!adev || !adev->pnp.unique_id ||
>>>>> + (kstrtoull(adev->pnp.unique_id, 10, &uid) < 0))
>>>> The UID field holds 4 bytes of ASCII characters. kstrtoull() would fail
>>>> if the UID field has value such as ‘CX03’.
>>>
>>> The UID field is not 4 ASCII characters.
>>>
>>> We went through this before in the original code in
>>> drivers/cxl/acpi.c::add_host_bridge_dport().
>>>
>>> The CEDT.CHBS defines _UID as an integer so use acpi_evaluate_integer()
>>> to retrieve the UID to perform the comparison. I thought there was an
>>> errata filed to clarify this, but it seems the current spec still just
>>> says "value". The CFMWS also places _UID values in the target list,
>>> those are also handled as integers.
>>
>> ACPI 6.4 spec section 6.1.12 describes _UID, it says the return value is:
>> An Integer or String containing the Unique ID.
>>
>> In the BIOS I see, the _UIDs of PCIe devices hold ASCII characters (not NULL
>> terminated).
>
> ASCII characters without NULL termination means that data can be
> treated as binary data which is what current CFMWWS parsing code chooses
> to do. I think a spec clarification is needed to make resolve the
> ambiguity.
Agreed. In this case, ACPI spec should be referred to.
On 07.09.22 09:11:58, Davidlohr Bueso wrote:
> On Wed, 31 Aug 2022, 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.
> >
> > Signed-off-by: Robert Richter <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
> > Reviewed-by: Jonathan Cameron <[email protected]>
>
> Reviewed-by: Davidlohr Bueso <[email protected]>
>
> Does this want a
>
> Fixes: d17d0540a0d (cxl/core/hdm: Add CXL standard decoder enumeration to the core)
>
> tag?
Looks like a code cleanup to me, nothing worth to backport to stable
which the fixes tag would trigger.
Thanks for review.
-Robert
On 08.09.22 12:45:16, Dan Williams wrote:
> Rafael J. Wysocki wrote:
> > On Thu, Sep 8, 2022 at 8:05 AM Dan Williams <[email protected]> wrote:
> > >
> > > Robert Richter wrote:
> > > > A lookup of a host bridge's corresponding acpi device (struct
> > > > acpi_device) is not possible, for example:
> > > >
> > > > adev = ACPI_COMPANION(&host_bridge->dev);
> > > >
> > > > This could be useful to find a host bridge's fwnode handle and to
> > > > determine and call additional host bridge ACPI parameters and methods
> > > > such as HID/CID or _UID.
> > >
> > > When is this explicitly needed. "Could be useful" is interesting, but it
> > > needs to have a practical need.
> >
> > It is needed and it is present on x86 AFAICS (see my last reply in this thread).
> >
> > This seems to be addressing an ARM64-specific issue.
> >
>
> Ah, ok, thanks for pointing that out.
No, it is x86. And true, it is set. So this series is actually working
without this patch. It can be dropped.
Now, I just checked my logs. The reason I was adding this is that
during code development I modified the code to have bridge->dev.parent
set. Then, the fwnode is not linked. I later dropped that change but
kept this patch.
Thanks for catching this.
-Robert
On 07.09.22 22:48:57, Dan Williams wrote:
> Robert Richter wrote:
> > The physical base address of a CXL range can be invalid and is then
> > set to CXL_RESOURCE_NONE. Early check this case before mapping a
> > memory block in devm_cxl_iomap_block().
> >
> > Signed-off-by: Robert Richter <[email protected]>
> > ---
> > 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 39a129c57d40..f216c017a474 100644
> > --- a/drivers/cxl/core/regs.c
> > +++ b/drivers/cxl/core/regs.c
> > @@ -165,6 +165,9 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> > void __iomem *ret_val;
> > struct resource *res;
> >
> > + if (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
> >
>
> devm_request_mem_region() succeeds for you when this happens? More
> details about the failure scenario please.
No, CXL_RESOURCE_NONE (all FFs) is used as address. A broken range is
calculated that even overflows. I only vaguely remember the exact
error message.
This may happen e.g. if the Component Register Block is missing in the
DVSEC. cxl_find_regblock() may fail then and returns
CXL_RESOURCE_NONE. There are a couple of code paths there
component_reg_phys is set to CXL_RESOURCE_NONE without exiting
immediately.
I saw it during code development, when I pre-inititalized a port with
component_reg_phys set to CXL_RESOURCE_NONE. Since that case can
generally happen, I think it must be checked.
-Robert
On Fri, Sep 09, 2022 at 12:20:45PM +0200, Robert Richter wrote:
> On 08.09.22 12:45:16, Dan Williams wrote:
> > Rafael J. Wysocki wrote:
> > > On Thu, Sep 8, 2022 at 8:05 AM Dan Williams <[email protected]> wrote:
> > > >
> > > > Robert Richter wrote:
> > > > > A lookup of a host bridge's corresponding acpi device (struct
> > > > > acpi_device) is not possible, for example:
> > > > >
> > > > > adev = ACPI_COMPANION(&host_bridge->dev);
> > > > >
> > > > > This could be useful to find a host bridge's fwnode handle and to
> > > > > determine and call additional host bridge ACPI parameters and methods
> > > > > such as HID/CID or _UID.
> ...
> No, it is x86. And true, it is set. So this series is actually working
> without this patch. It can be dropped.
>
> Now, I just checked my logs. The reason I was adding this is that
> during code development I modified the code to have bridge->dev.parent
> set. Then, the fwnode is not linked. I later dropped that change but
> kept this patch.
If this patch does the same thing as the ACPI_COMPANION_SET() in
several pcibios_root_bridge_prepare() implementations, I would love to
keep this patch, which does it in a generic place, and drop the
corresponding code from those arch-specific functions.
But I don't understand the fwnode stuff well enough to know if this is
feasible.
Bjorn
Robert Richter wrote:
> In RCD mode the CXL mem dev may be detected on the PCI bus before a
> CXL host is brought up. This may cause a CXL mem initialization
> failure as it expects the CXL host already detected.
This is not unique to RCD mode. It is mitigated by the cxl_bus_rescan()
at the completion of cxl_acpi_probe().
> Address this by
> specifying the module dependencies using MODULE_SOFTDEP().
>
> The following additional dependencies exist:
>
> * cxl_mem depends on cxl_acpi: The CXL hosts must be discovered
> before the CXL device is initialized.
>
> * cxl_acpi depends on cxl_port: The acpi driver adds ports to the cxl
> bus, the port driver should be loaded before. This might also work
> if modules are loaded in different order, but a) it aligns with the
> existing cxl_mem/cxl_port softdep and b) it always guarantees a fix
> module load order.
Why does a fixed module load order matter?
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> drivers/cxl/acpi.c | 1 +
> drivers/cxl/mem.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 56b2d222afcc..63a1cb295c07 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -834,3 +834,4 @@ module_exit(cxl_host_exit);
> MODULE_LICENSE("GPL v2");
> MODULE_IMPORT_NS(CXL);
> MODULE_IMPORT_NS(ACPI);
> +MODULE_SOFTDEP("pre: cxl_port");
The only reason cxl_acpi would need to preload cxl_port is if it wanted
to do something like:
port = devm_cxl_add_port(...);
if (port->dev.driver)
/* do something with an enabled port */
else
/* do something else if the port failed to enable */
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 64ccf053d32c..ae13ec7d9894 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -128,3 +128,4 @@ MODULE_ALIAS_CXL(CXL_DEVICE_MEMORY_EXPANDER);
> * endpoint registration.
> */
> MODULE_SOFTDEP("pre: cxl_port");
> +MODULE_SOFTDEP("pre: cxl_acpi");
There is no strict requirement that CXL topologies be limited to ACPI
platforms. Per above, the cxl_mem driver will attach when the CXL root
device finally appears, and async out-of-order arrival is ok.
Robert Richter wrote:
> 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. In RCD mode an ACPI0017 device
> might not necessarily exist and the host bridge can have a standard
> PCIe host bridge PNP0A08 ID, there aren't any CXL port or switches in
> the PCIe hierarchy visible. As such the RCD mode enumeration and host
> discovery is very different from CXL VH. See patch #5 for
> implementation details.
>
> This implementation 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 #4 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 #5 to #15 contain
> the actual implementation of RCD mode support.
Hi Robert,
I did not see a response to some of my feedback but wanted to summarize
where I think the next version of this set needs to go:
1/ ACPI0017 is mandatory. If a BIOS does not provide ACPI0017 it is
explicitly opting the OS out of managing anything other than the CXL.io
side of memory expanders.
2/ Per table 8-22 in CXL 3.0 RCDs are not permitted to have HDM decoders
so that assumption in the driver needs to be reworked.
3/ It's not even clear that the Register Locator DVSEC has any role to
play in an RCD as every register the driver needs should be relative to
the RCRB. So the assumptions in the driver need to consider RCRB located
registers as a first class citizen.
Robert Richter wrote:
> On 07.09.22 22:48:57, Dan Williams wrote:
> > Robert Richter wrote:
> > > The physical base address of a CXL range can be invalid and is then
> > > set to CXL_RESOURCE_NONE. Early check this case before mapping a
> > > memory block in devm_cxl_iomap_block().
> > >
> > > Signed-off-by: Robert Richter <[email protected]>
> > > ---
> > > 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 39a129c57d40..f216c017a474 100644
> > > --- a/drivers/cxl/core/regs.c
> > > +++ b/drivers/cxl/core/regs.c
> > > @@ -165,6 +165,9 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> > > void __iomem *ret_val;
> > > struct resource *res;
> > >
> > > + if (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
> > >
> >
> > devm_request_mem_region() succeeds for you when this happens? More
> > details about the failure scenario please.
>
> No, CXL_RESOURCE_NONE (all FFs) is used as address. A broken range is
> calculated that even overflows. I only vaguely remember the exact
> error message.
>
> This may happen e.g. if the Component Register Block is missing in the
> DVSEC. cxl_find_regblock() may fail then and returns
> CXL_RESOURCE_NONE. There are a couple of code paths there
> component_reg_phys is set to CXL_RESOURCE_NONE without exiting
> immediately.
>
> I saw it during code development, when I pre-inititalized a port with
> component_reg_phys set to CXL_RESOURCE_NONE. Since that case can
> generally happen, I think it must be checked.
I think Jonathan had it right when we posited that the code should
probably have failed before getting to this point. For example, the
scenarios where the driver looks for a component register block via the
register locator DVSEC are not valid for RCDs in the first instance.
Bjorn Helgaas wrote:
> On Fri, Sep 09, 2022 at 12:20:45PM +0200, Robert Richter wrote:
> > On 08.09.22 12:45:16, Dan Williams wrote:
> > > Rafael J. Wysocki wrote:
> > > > On Thu, Sep 8, 2022 at 8:05 AM Dan Williams <[email protected]> wrote:
> > > > >
> > > > > Robert Richter wrote:
> > > > > > A lookup of a host bridge's corresponding acpi device (struct
> > > > > > acpi_device) is not possible, for example:
> > > > > >
> > > > > > adev = ACPI_COMPANION(&host_bridge->dev);
> > > > > >
> > > > > > This could be useful to find a host bridge's fwnode handle and to
> > > > > > determine and call additional host bridge ACPI parameters and methods
> > > > > > such as HID/CID or _UID.
>
> > ...
> > No, it is x86. And true, it is set. So this series is actually working
> > without this patch. It can be dropped.
> >
> > Now, I just checked my logs. The reason I was adding this is that
> > during code development I modified the code to have bridge->dev.parent
> > set. Then, the fwnode is not linked. I later dropped that change but
> > kept this patch.
>
> If this patch does the same thing as the ACPI_COMPANION_SET() in
> several pcibios_root_bridge_prepare() implementations, I would love to
> keep this patch, which does it in a generic place, and drop the
> corresponding code from those arch-specific functions.
>
> But I don't understand the fwnode stuff well enough to know if this is
> feasible.
I took a brief look, but I could not convince myself that these lookups:
arch/ia64/pci/pci.c ((struct pci_controller *) bridge->bus->sysdata)->companion
arch/loongarch/pci/acpi.c to_acpi_device(((struct pci_config_window *) bridge->bus->sysdata)->parent)
arch/arm64/kernel/pci.c to_acpi_device(((struct pci_config_window *) bridge->bus->sysdata)->parent)
arch/x86/pci/acpi.c ((struct pci_sysdata *) bridge->bus->sysdata)->companion
...are identical to what acpi_pci_root_add() used to create the
acpi_pci_root.
On 07.09.22 22:53:12, Dan Williams wrote:
> Robert Richter wrote:
> > + parent_port = parent_dport ? parent_dport->port : NULL;
> > +
> > port = cxl_port_alloc(uport, component_reg_phys, parent_dport);
> > - if (IS_ERR(port))
> > - return port;
> > + if (IS_ERR(port)) {
> > + rc = PTR_ERR(port);
> > + goto err_out;
>
> While I agree this unifies the error messaging I am not a fan of what
> this does to the readability of the error exits. What about a compromise
> of renaming devm_cxl_add_port to __devm_cxl_add_port() and add back a
> new devm_cxl_add_port that does the unified debug messaging?
Ok, will add a wrapper.
Thanks,
-Robert
On 16.09.22 11:04:01, Dan Williams wrote:
> Robert Richter wrote:
> > On 07.09.22 22:48:57, Dan Williams wrote:
> > > Robert Richter wrote:
> > > > The physical base address of a CXL range can be invalid and is then
> > > > set to CXL_RESOURCE_NONE. Early check this case before mapping a
> > > > memory block in devm_cxl_iomap_block().
> > > >
> > > > Signed-off-by: Robert Richter <[email protected]>
> > > > ---
> > > > 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 39a129c57d40..f216c017a474 100644
> > > > --- a/drivers/cxl/core/regs.c
> > > > +++ b/drivers/cxl/core/regs.c
> > > > @@ -165,6 +165,9 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> > > > void __iomem *ret_val;
> > > > struct resource *res;
> > > >
> > > > + if (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
> > > >
> > >
> > > devm_request_mem_region() succeeds for you when this happens? More
> > > details about the failure scenario please.
> >
> > No, CXL_RESOURCE_NONE (all FFs) is used as address. A broken range is
> > calculated that even overflows. I only vaguely remember the exact
> > error message.
> >
> > This may happen e.g. if the Component Register Block is missing in the
> > DVSEC. cxl_find_regblock() may fail then and returns
> > CXL_RESOURCE_NONE. There are a couple of code paths there
> > component_reg_phys is set to CXL_RESOURCE_NONE without exiting
> > immediately.
> >
> > I saw it during code development, when I pre-inititalized a port with
> > component_reg_phys set to CXL_RESOURCE_NONE. Since that case can
> > generally happen, I think it must be checked.
>
> I think Jonathan had it right when we posited that the code should
> probably have failed before getting to this point. For example, the
> scenarios where the driver looks for a component register block via the
> register locator DVSEC are not valid for RCDs in the first instance.
I am ok having the code reviewed to prevent such situations. But the
handling of component_reg_phys is by far not trivial as there are many
locations it is initialized and others where it is used. Call chain is
across multiple functions using various data sources for
component_reg_phys, so it is hard to proof this may never happen.
E.g. in add_port_attach_ep() I found this:
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, I did not go through other code paths.
IMO, the check is cheap and prevents weird follow up errors and log
messages. But I would be also ok, to just drop the patch. There are no
dependencies to this change. What do you think?
Thanks,
-Robert
Robert Richter wrote:
> On 16.09.22 11:04:01, Dan Williams wrote:
> > Robert Richter wrote:
> > > On 07.09.22 22:48:57, Dan Williams wrote:
> > > > Robert Richter wrote:
> > > > > The physical base address of a CXL range can be invalid and is then
> > > > > set to CXL_RESOURCE_NONE. Early check this case before mapping a
> > > > > memory block in devm_cxl_iomap_block().
> > > > >
> > > > > Signed-off-by: Robert Richter <[email protected]>
> > > > > ---
> > > > > 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 39a129c57d40..f216c017a474 100644
> > > > > --- a/drivers/cxl/core/regs.c
> > > > > +++ b/drivers/cxl/core/regs.c
> > > > > @@ -165,6 +165,9 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> > > > > void __iomem *ret_val;
> > > > > struct resource *res;
> > > > >
> > > > > + if (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
> > > > >
> > > >
> > > > devm_request_mem_region() succeeds for you when this happens? More
> > > > details about the failure scenario please.
> > >
> > > No, CXL_RESOURCE_NONE (all FFs) is used as address. A broken range is
> > > calculated that even overflows. I only vaguely remember the exact
> > > error message.
> > >
> > > This may happen e.g. if the Component Register Block is missing in the
> > > DVSEC. cxl_find_regblock() may fail then and returns
> > > CXL_RESOURCE_NONE. There are a couple of code paths there
> > > component_reg_phys is set to CXL_RESOURCE_NONE without exiting
> > > immediately.
> > >
> > > I saw it during code development, when I pre-inititalized a port with
> > > component_reg_phys set to CXL_RESOURCE_NONE. Since that case can
> > > generally happen, I think it must be checked.
> >
> > I think Jonathan had it right when we posited that the code should
> > probably have failed before getting to this point. For example, the
> > scenarios where the driver looks for a component register block via the
> > register locator DVSEC are not valid for RCDs in the first instance.
>
> I am ok having the code reviewed to prevent such situations. But the
> handling of component_reg_phys is by far not trivial as there are many
> locations it is initialized and others where it is used. Call chain is
> across multiple functions using various data sources for
> component_reg_phys, so it is hard to proof this may never happen.
> E.g. in add_port_attach_ep() I found this:
>
> 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, I did not go through other code paths.
>
> IMO, the check is cheap and prevents weird follow up errors and log
> messages. But I would be also ok, to just drop the patch. There are no
> dependencies to this change. What do you think?
Good points above...
Yeah, the expectation is that only "broken hardware" fails to provide
component registers, but the driver is currently deficient in that it
assumes everything is CXL 2.0.
Now you have me thinking it should go a bit further and do a WARN_ONCE()
when CXL_RESOURCE_NONE is passed, as those locations need better error
handling than just handling it like an ioremap() failure.