| Message ID | 166931493266.2104015.8062923429837042172.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Add support for Restricted CXL hosts (RCD mode) | expand |
On 11/24/2022 11:35 AM, Dan Williams wrote: > Unlike a CXL memory expander in a VH topology that has at least one > intervening 'struct cxl_port' instance between itself and the CXL root > device, an RCD attaches one-level higher. For example: > > VH > ┌──────────┐ > │ ACPI0017 │ > │ root0 │ > └─────┬────┘ > │ > ┌─────┴────┐ > │ dport0 │ > ┌─────┤ ACPI0016 ├─────┐ > │ │ port1 │ │ > │ └────┬─────┘ │ > │ │ │ > ┌──┴───┐ ┌──┴───┐ ┌───┴──┐ > │dport0│ │dport1│ │dport2│ > │ RP0 │ │ RP1 │ │ RP2 │ > └──────┘ └──┬───┘ └──────┘ > │ > ┌───┴─────┐ > │endpoint0│ > │ port2 │ > └─────────┘ > > ...vs: > > RCH > ┌──────────┐ > │ ACPI0017 │ > │ root0 │ > └────┬─────┘ > │ > ┌───┴────┐ > │ dport0 │ > │ACPI0016│ > └───┬────┘ > │ > ┌────┴─────┐ > │endpoint0 │ > │ port1 │ > └──────────┘ > > So arrange for endpoint port in the RCH/RCD case to appear directly > connected to the host-bridge in its singular role as a dport. Compare > that to the VH case where the host-bridge serves a dual role as a > 'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port for > the Root Ports in the Root Complex that are modeled as 'cxl_dport' > instances in the CXL topology. > > Another deviation from the VH case is that RCDs may need to look up > their component registers from the Root Complex Register Block (RCRB). > That platform firmware specified RCRB area is cached by the cxl_acpi > driver and conveyed via the host-bridge dport to the cxl_mem driver to > perform the cxl_rcrb_to_component() lookup for the endpoint port > (See 9.11.8 CXL Devices Attached to an RCH for the lookup of the > upstream port component registers). > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/port.c | 11 +++++++++-- > drivers/cxl/cxlmem.h | 2 ++ > drivers/cxl/mem.c | 31 ++++++++++++++++++++++++------- > drivers/cxl/pci.c | 10 ++++++++++ > 4 files changed, 45 insertions(+), 9 deletions(-) > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index c7f58282b2c1..2385ee00eb9a 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -1358,8 +1358,17 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > { > struct device *dev = &cxlmd->dev; > struct device *iter; > + struct cxl_dport *dport; > + struct cxl_port *port; > int rc; > > + /* > + * Skip intermediate port enumeration in the RCH case, there > + * are no ports in between a host bridge and an endpoint. > + */ > + if (cxlmd->cxlds->rcd) > + return 0; > + > rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd); > if (rc) > return rc; > @@ -1373,8 +1382,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > for (iter = dev; iter; iter = grandparent(iter)) { > struct device *dport_dev = grandparent(iter); > struct device *uport_dev; > - struct cxl_dport *dport; > - struct cxl_port *port; > > if (!dport_dev) > return 0; > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index e082991bc58c..35d485d041f0 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -201,6 +201,7 @@ struct cxl_endpoint_dvsec_info { > * @dev: The device associated with this CXL state > * @regs: Parsed register blocks > * @cxl_dvsec: Offset to the PCIe device DVSEC > + * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH) > * @payload_size: Size of space for payload > * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register) > * @lsa_size: Size of Label Storage Area > @@ -235,6 +236,7 @@ struct cxl_dev_state { > struct cxl_regs regs; > int cxl_dvsec; > > + bool rcd; > size_t payload_size; > size_t lsa_size; > struct mutex mbox_mutex; /* Protects device mailbox and firmware */ > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index aa63ce8c7ca6..9a655b4b5e52 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -45,12 +45,13 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data) > return 0; > } > > -static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > +static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd, > struct cxl_dport *parent_dport) > { > struct cxl_port *parent_port = parent_dport->port; > struct cxl_dev_state *cxlds = cxlmd->cxlds; > struct cxl_port *endpoint, *iter, *down; > + resource_size_t component_reg_phys; > int rc; > > /* > @@ -65,8 +66,18 @@ static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > ep->next = down; > } > > - endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev, > - cxlds->component_reg_phys, parent_dport); > + /* > + * The component registers for an RCD might come from the > + * host-bridge RCRB if they are not already mapped via the > + * typical register locator mechanism. > + */ > + if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) > + component_reg_phys = cxl_rcrb_to_component( > + &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_DOWNSTREAM); Should this be CXL_RCRB_UPSTREAM? > + else > + component_reg_phys = cxlds->component_reg_phys; > + endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys, > + parent_dport); > if (IS_ERR(endpoint)) > return PTR_ERR(endpoint); > > @@ -87,6 +98,7 @@ static int cxl_mem_probe(struct device *dev) > { > struct cxl_memdev *cxlmd = to_cxl_memdev(dev); > struct cxl_dev_state *cxlds = cxlmd->cxlds; > + struct device *endpoint_parent; > struct cxl_port *parent_port; > struct cxl_dport *dport; > struct dentry *dentry; > @@ -119,17 +131,22 @@ static int cxl_mem_probe(struct device *dev) > return -ENXIO; > } > > - device_lock(&parent_port->dev); > - if (!parent_port->dev.driver) { > + if (dport->rch) > + endpoint_parent = parent_port->uport; > + else > + endpoint_parent = &parent_port->dev; > + > + device_lock(endpoint_parent); > + if (!endpoint_parent->driver) { > dev_err(dev, "CXL port topology %s not enabled\n", > dev_name(&parent_port->dev)); > rc = -ENXIO; > goto unlock; > } > > - rc = devm_cxl_add_endpoint(cxlmd, dport); > + rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); > unlock: > - device_unlock(&parent_port->dev); > + device_unlock(endpoint_parent); > put_device(&parent_port->dev); > if (rc) > return rc; > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index e15da405b948..73ff6c33a0c0 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -433,6 +433,15 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds) > } > } > > +/* > + * Assume that any RCIEP that emits the CXL memory expander class code > + * is an RCD > + */ > +static bool is_cxl_restricted(struct pci_dev *pdev) > +{ > + return pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END; > +} > + > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > struct cxl_register_map map; > @@ -455,6 +464,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (IS_ERR(cxlds)) > return PTR_ERR(cxlds); > > + cxlds->rcd = is_cxl_restricted(pdev); > cxlds->serial = pci_get_dsn(pdev); > cxlds->cxl_dvsec = pci_find_dvsec_capability( > pdev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE); >
Dave Jiang wrote: > > > On 11/24/2022 11:35 AM, Dan Williams wrote: > > Unlike a CXL memory expander in a VH topology that has at least one > > intervening 'struct cxl_port' instance between itself and the CXL root > > device, an RCD attaches one-level higher. For example: > > > > VH > > ┌──────────┐ > > │ ACPI0017 │ > > │ root0 │ > > └─────┬────┘ > > │ > > ┌─────┴────┐ > > │ dport0 │ > > ┌─────┤ ACPI0016 ├─────┐ > > │ │ port1 │ │ > > │ └────┬─────┘ │ > > │ │ │ > > ┌──┴───┐ ┌──┴───┐ ┌───┴──┐ > > │dport0│ │dport1│ │dport2│ > > │ RP0 │ │ RP1 │ │ RP2 │ > > └──────┘ └──┬───┘ └──────┘ > > │ > > ┌───┴─────┐ > > │endpoint0│ > > │ port2 │ > > └─────────┘ > > > > ...vs: > > > > RCH > > ┌──────────┐ > > │ ACPI0017 │ > > │ root0 │ > > └────┬─────┘ > > │ > > ┌───┴────┐ > > │ dport0 │ > > │ACPI0016│ > > └───┬────┘ > > │ > > ┌────┴─────┐ > > │endpoint0 │ > > │ port1 │ > > └──────────┘ > > > > So arrange for endpoint port in the RCH/RCD case to appear directly > > connected to the host-bridge in its singular role as a dport. Compare > > that to the VH case where the host-bridge serves a dual role as a > > 'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port for > > the Root Ports in the Root Complex that are modeled as 'cxl_dport' > > instances in the CXL topology. > > > > Another deviation from the VH case is that RCDs may need to look up > > their component registers from the Root Complex Register Block (RCRB). > > That platform firmware specified RCRB area is cached by the cxl_acpi > > driver and conveyed via the host-bridge dport to the cxl_mem driver to > > perform the cxl_rcrb_to_component() lookup for the endpoint port > > (See 9.11.8 CXL Devices Attached to an RCH for the lookup of the > > upstream port component registers). > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > drivers/cxl/core/port.c | 11 +++++++++-- > > drivers/cxl/cxlmem.h | 2 ++ > > drivers/cxl/mem.c | 31 ++++++++++++++++++++++++------- > > drivers/cxl/pci.c | 10 ++++++++++ > > 4 files changed, 45 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > > index c7f58282b2c1..2385ee00eb9a 100644 > > --- a/drivers/cxl/core/port.c > > +++ b/drivers/cxl/core/port.c > > @@ -1358,8 +1358,17 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > { > > struct device *dev = &cxlmd->dev; > > struct device *iter; > > + struct cxl_dport *dport; > > + struct cxl_port *port; > > int rc; > > > > + /* > > + * Skip intermediate port enumeration in the RCH case, there > > + * are no ports in between a host bridge and an endpoint. > > + */ > > + if (cxlmd->cxlds->rcd) > > + return 0; > > + > > rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd); > > if (rc) > > return rc; > > @@ -1373,8 +1382,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > for (iter = dev; iter; iter = grandparent(iter)) { > > struct device *dport_dev = grandparent(iter); > > struct device *uport_dev; > > - struct cxl_dport *dport; > > - struct cxl_port *port; > > > > if (!dport_dev) > > return 0; > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > > index e082991bc58c..35d485d041f0 100644 > > --- a/drivers/cxl/cxlmem.h > > +++ b/drivers/cxl/cxlmem.h > > @@ -201,6 +201,7 @@ struct cxl_endpoint_dvsec_info { > > * @dev: The device associated with this CXL state > > * @regs: Parsed register blocks > > * @cxl_dvsec: Offset to the PCIe device DVSEC > > + * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH) > > * @payload_size: Size of space for payload > > * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register) > > * @lsa_size: Size of Label Storage Area > > @@ -235,6 +236,7 @@ struct cxl_dev_state { > > struct cxl_regs regs; > > int cxl_dvsec; > > > > + bool rcd; > > size_t payload_size; > > size_t lsa_size; > > struct mutex mbox_mutex; /* Protects device mailbox and firmware */ > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > index aa63ce8c7ca6..9a655b4b5e52 100644 > > --- a/drivers/cxl/mem.c > > +++ b/drivers/cxl/mem.c > > @@ -45,12 +45,13 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data) > > return 0; > > } > > > > -static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > > +static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd, > > struct cxl_dport *parent_dport) > > { > > struct cxl_port *parent_port = parent_dport->port; > > struct cxl_dev_state *cxlds = cxlmd->cxlds; > > struct cxl_port *endpoint, *iter, *down; > > + resource_size_t component_reg_phys; > > int rc; > > > > /* > > @@ -65,8 +66,18 @@ static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > > ep->next = down; > > } > > > > - endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev, > > - cxlds->component_reg_phys, parent_dport); > > + /* > > + * The component registers for an RCD might come from the > > + * host-bridge RCRB if they are not already mapped via the > > + * typical register locator mechanism. > > + */ > > + if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) > > + component_reg_phys = cxl_rcrb_to_component( > > + &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_DOWNSTREAM); > > Should this be CXL_RCRB_UPSTREAM? Indeed it should, good catch.
On 24.11.22 10:35:32, Dan Williams wrote: > Unlike a CXL memory expander in a VH topology that has at least one > intervening 'struct cxl_port' instance between itself and the CXL root > device, an RCD attaches one-level higher. For example: > > VH > ┌──────────┐ > │ ACPI0017 │ > │ root0 │ > └─────┬────┘ > │ > ┌─────┴────┐ > │ dport0 │ > ┌─────┤ ACPI0016 ├─────┐ > │ │ port1 │ │ > │ └────┬─────┘ │ > │ │ │ > ┌──┴───┐ ┌──┴───┐ ┌───┴──┐ > │dport0│ │dport1│ │dport2│ > │ RP0 │ │ RP1 │ │ RP2 │ > └──────┘ └──┬───┘ └──────┘ > │ > ┌───┴─────┐ > │endpoint0│ > │ port2 │ > └─────────┘ > > ...vs: > > RCH > ┌──────────┐ > │ ACPI0017 │ > │ root0 │ > └────┬─────┘ > │ > ┌───┴────┐ > │ dport0 │ > │ACPI0016│ > └───┬────┘ > │ > ┌────┴─────┐ > │endpoint0 │ > │ port1 │ > └──────────┘ > > So arrange for endpoint port in the RCH/RCD case to appear directly > connected to the host-bridge in its singular role as a dport. Compare > that to the VH case where the host-bridge serves a dual role as a > 'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port for > the Root Ports in the Root Complex that are modeled as 'cxl_dport' > instances in the CXL topology. > > Another deviation from the VH case is that RCDs may need to look up > their component registers from the Root Complex Register Block (RCRB). > That platform firmware specified RCRB area is cached by the cxl_acpi > driver and conveyed via the host-bridge dport to the cxl_mem driver to > perform the cxl_rcrb_to_component() lookup for the endpoint port > (See 9.11.8 CXL Devices Attached to an RCH for the lookup of the > upstream port component registers). > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/port.c | 11 +++++++++-- > drivers/cxl/cxlmem.h | 2 ++ > drivers/cxl/mem.c | 31 ++++++++++++++++++++++++------- > drivers/cxl/pci.c | 10 ++++++++++ > 4 files changed, 45 insertions(+), 9 deletions(-) > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index c7f58282b2c1..2385ee00eb9a 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -1358,8 +1358,17 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > { > struct device *dev = &cxlmd->dev; > struct device *iter; > + struct cxl_dport *dport; > + struct cxl_port *port; There is no direct need to move that code here. If you want to clean that up in this patch too, then leave a comment in the change log? > int rc; > > + /* > + * Skip intermediate port enumeration in the RCH case, there > + * are no ports in between a host bridge and an endpoint. > + */ > + if (cxlmd->cxlds->rcd) > + return 0; > + > rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd); > if (rc) > return rc; > @@ -1373,8 +1382,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > for (iter = dev; iter; iter = grandparent(iter)) { > struct device *dport_dev = grandparent(iter); > struct device *uport_dev; > - struct cxl_dport *dport; > - struct cxl_port *port; > > if (!dport_dev) > return 0; > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index e082991bc58c..35d485d041f0 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -201,6 +201,7 @@ struct cxl_endpoint_dvsec_info { > * @dev: The device associated with this CXL state > * @regs: Parsed register blocks > * @cxl_dvsec: Offset to the PCIe device DVSEC > + * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH) > * @payload_size: Size of space for payload > * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register) > * @lsa_size: Size of Label Storage Area > @@ -235,6 +236,7 @@ struct cxl_dev_state { > struct cxl_regs regs; > int cxl_dvsec; > > + bool rcd; > size_t payload_size; > size_t lsa_size; > struct mutex mbox_mutex; /* Protects device mailbox and firmware */ > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index aa63ce8c7ca6..9a655b4b5e52 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -45,12 +45,13 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data) > return 0; > } > > -static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > +static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd, > struct cxl_dport *parent_dport) > { > struct cxl_port *parent_port = parent_dport->port; > struct cxl_dev_state *cxlds = cxlmd->cxlds; > struct cxl_port *endpoint, *iter, *down; > + resource_size_t component_reg_phys; > int rc; > > /* > @@ -65,8 +66,18 @@ static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > ep->next = down; > } > > - endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev, > - cxlds->component_reg_phys, parent_dport); > + /* > + * The component registers for an RCD might come from the > + * host-bridge RCRB if they are not already mapped via the > + * typical register locator mechanism. > + */ > + if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) > + component_reg_phys = cxl_rcrb_to_component( > + &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_DOWNSTREAM); As already commented: this must be the upstream RCRB here. > + else > + component_reg_phys = cxlds->component_reg_phys; > + endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys, > + parent_dport); Looking at CXL 3.0 spec, table 8-22, there are the various sources of component registers listed. For RCD we need: D1, DP1, UP1 (optional R). D1: endpoint->component_reg_phys; UP1: parent_port-component_reg_phys; (missing in RCH topology) DP1: parent_dport->component_reg_phys; I don't see how all of them could be stored in this data layout as the cxl host port is missing. > if (IS_ERR(endpoint)) > return PTR_ERR(endpoint); > > @@ -87,6 +98,7 @@ static int cxl_mem_probe(struct device *dev) > { > struct cxl_memdev *cxlmd = to_cxl_memdev(dev); > struct cxl_dev_state *cxlds = cxlmd->cxlds; > + struct device *endpoint_parent; > struct cxl_port *parent_port; > struct cxl_dport *dport; > struct dentry *dentry; > @@ -119,17 +131,22 @@ static int cxl_mem_probe(struct device *dev) > return -ENXIO; > } > > - device_lock(&parent_port->dev); > - if (!parent_port->dev.driver) { > + if (dport->rch) > + endpoint_parent = parent_port->uport; > + else > + endpoint_parent = &parent_port->dev; > + > + device_lock(endpoint_parent); > + if (!endpoint_parent->driver) { > dev_err(dev, "CXL port topology %s not enabled\n", > dev_name(&parent_port->dev)); > rc = -ENXIO; > goto unlock; > } > > - rc = devm_cxl_add_endpoint(cxlmd, dport); > + rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); > unlock: > - device_unlock(&parent_port->dev); > + device_unlock(endpoint_parent); > put_device(&parent_port->dev); > if (rc) > return rc; > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index e15da405b948..73ff6c33a0c0 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -433,6 +433,15 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds) > } > } > > +/* > + * Assume that any RCIEP that emits the CXL memory expander class code > + * is an RCD > + */ > +static bool is_cxl_restricted(struct pci_dev *pdev) > +{ > + return pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END; > +} > + > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > struct cxl_register_map map; > @@ -455,6 +464,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (IS_ERR(cxlds)) > return PTR_ERR(cxlds); > > + cxlds->rcd = is_cxl_restricted(pdev); > cxlds->serial = pci_get_dsn(pdev); > cxlds->cxl_dvsec = pci_find_dvsec_capability( > pdev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE); >
Robert Richter wrote: > On 24.11.22 10:35:32, Dan Williams wrote: > > Unlike a CXL memory expander in a VH topology that has at least one > > intervening 'struct cxl_port' instance between itself and the CXL root > > device, an RCD attaches one-level higher. For example: > > > > VH > > ┌──────────┐ > > │ ACPI0017 │ > > │ root0 │ > > └─────┬────┘ > > │ > > ┌─────┴────┐ > > │ dport0 │ > > ┌─────┤ ACPI0016 ├─────┐ > > │ │ port1 │ │ > > │ └────┬─────┘ │ > > │ │ │ > > ┌──┴───┐ ┌──┴───┐ ┌───┴──┐ > > │dport0│ │dport1│ │dport2│ > > │ RP0 │ │ RP1 │ │ RP2 │ > > └──────┘ └──┬───┘ └──────┘ > > │ > > ┌───┴─────┐ > > │endpoint0│ > > │ port2 │ > > └─────────┘ > > > > ...vs: > > > > RCH > > ┌──────────┐ > > │ ACPI0017 │ > > │ root0 │ > > └────┬─────┘ > > │ > > ┌───┴────┐ > > │ dport0 │ > > │ACPI0016│ > > └───┬────┘ > > │ > > ┌────┴─────┐ > > │endpoint0 │ > > │ port1 │ > > └──────────┘ > > > > So arrange for endpoint port in the RCH/RCD case to appear directly > > connected to the host-bridge in its singular role as a dport. Compare > > that to the VH case where the host-bridge serves a dual role as a > > 'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port for > > the Root Ports in the Root Complex that are modeled as 'cxl_dport' > > instances in the CXL topology. > > > > Another deviation from the VH case is that RCDs may need to look up > > their component registers from the Root Complex Register Block (RCRB). > > That platform firmware specified RCRB area is cached by the cxl_acpi > > driver and conveyed via the host-bridge dport to the cxl_mem driver to > > perform the cxl_rcrb_to_component() lookup for the endpoint port > > (See 9.11.8 CXL Devices Attached to an RCH for the lookup of the > > upstream port component registers). > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > drivers/cxl/core/port.c | 11 +++++++++-- > > drivers/cxl/cxlmem.h | 2 ++ > > drivers/cxl/mem.c | 31 ++++++++++++++++++++++++------- > > drivers/cxl/pci.c | 10 ++++++++++ > > 4 files changed, 45 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > > index c7f58282b2c1..2385ee00eb9a 100644 > > --- a/drivers/cxl/core/port.c > > +++ b/drivers/cxl/core/port.c > > @@ -1358,8 +1358,17 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > { > > struct device *dev = &cxlmd->dev; > > struct device *iter; > > + struct cxl_dport *dport; > > + struct cxl_port *port; > > There is no direct need to move that code here. > > If you want to clean that up in this patch too, then leave a comment > in the change log? Oh, good point, must have been left over from an earlier revision of the patch, dropped it. > > > int rc; > > > > + /* > > + * Skip intermediate port enumeration in the RCH case, there > > + * are no ports in between a host bridge and an endpoint. > > + */ > > + if (cxlmd->cxlds->rcd) > > + return 0; > > + > > rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd); > > if (rc) > > return rc; > > @@ -1373,8 +1382,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > for (iter = dev; iter; iter = grandparent(iter)) { > > struct device *dport_dev = grandparent(iter); > > struct device *uport_dev; > > - struct cxl_dport *dport; > > - struct cxl_port *port; > > > > if (!dport_dev) > > return 0; > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > > index e082991bc58c..35d485d041f0 100644 > > --- a/drivers/cxl/cxlmem.h > > +++ b/drivers/cxl/cxlmem.h > > @@ -201,6 +201,7 @@ struct cxl_endpoint_dvsec_info { > > * @dev: The device associated with this CXL state > > * @regs: Parsed register blocks > > * @cxl_dvsec: Offset to the PCIe device DVSEC > > + * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH) > > * @payload_size: Size of space for payload > > * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register) > > * @lsa_size: Size of Label Storage Area > > @@ -235,6 +236,7 @@ struct cxl_dev_state { > > struct cxl_regs regs; > > int cxl_dvsec; > > > > + bool rcd; > > size_t payload_size; > > size_t lsa_size; > > struct mutex mbox_mutex; /* Protects device mailbox and firmware */ > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > index aa63ce8c7ca6..9a655b4b5e52 100644 > > --- a/drivers/cxl/mem.c > > +++ b/drivers/cxl/mem.c > > @@ -45,12 +45,13 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data) > > return 0; > > } > > > > -static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > > +static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd, > > struct cxl_dport *parent_dport) > > { > > struct cxl_port *parent_port = parent_dport->port; > > struct cxl_dev_state *cxlds = cxlmd->cxlds; > > struct cxl_port *endpoint, *iter, *down; > > + resource_size_t component_reg_phys; > > int rc; > > > > /* > > @@ -65,8 +66,18 @@ static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > > ep->next = down; > > } > > > > - endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev, > > - cxlds->component_reg_phys, parent_dport); > > + /* > > + * The component registers for an RCD might come from the > > + * host-bridge RCRB if they are not already mapped via the > > + * typical register locator mechanism. > > + */ > > + if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) > > + component_reg_phys = cxl_rcrb_to_component( > > + &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_DOWNSTREAM); > > As already commented: this must be the upstream RCRB here. > > > + else > > + component_reg_phys = cxlds->component_reg_phys; > > + endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys, > > + parent_dport); > > Looking at CXL 3.0 spec, table 8-22, there are the various sources of > component registers listed. For RCD we need: D1, DP1, UP1 (optional > R). > > D1: endpoint->component_reg_phys; > UP1: parent_port-component_reg_phys; (missing in RCH topology) > DP1: parent_dport->component_reg_phys; > > I don't see how all of them could be stored in this data layout as the > cxl host port is missing. If I am understanding your concern correctly, that's handled here: if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) In the D1 case cxlds->component_reg_phys will be valid since the component registers were visible via the register locator DVSEC and retrieved by cxl_pci. In the UP1 case cxlds->component_reg_phys is invalid and the driver falls back to the RCRB. DP1 is handled in cxl_acpi. I.e. the D1 and UP1 cases do not co-exist.
On 28.11.22 16:25:18, Dan Williams wrote: > Robert Richter wrote: > > On 24.11.22 10:35:32, Dan Williams wrote: > > > Unlike a CXL memory expander in a VH topology that has at least one > > > intervening 'struct cxl_port' instance between itself and the CXL root > > > device, an RCD attaches one-level higher. For example: > > > > > > VH > > > ┌──────────┐ > > > │ ACPI0017 │ > > > │ root0 │ > > > └─────┬────┘ > > > │ > > > ┌─────┴────┐ > > > │ dport0 │ > > > ┌─────┤ ACPI0016 ├─────┐ > > > │ │ port1 │ │ > > > │ └────┬─────┘ │ > > > │ │ │ > > > ┌──┴───┐ ┌──┴───┐ ┌───┴──┐ > > > │dport0│ │dport1│ │dport2│ > > > │ RP0 │ │ RP1 │ │ RP2 │ > > > └──────┘ └──┬───┘ └──────┘ > > > │ > > > ┌───┴─────┐ > > > │endpoint0│ > > > │ port2 │ > > > └─────────┘ > > > > > > ...vs: > > > > > > RCH > > > ┌──────────┐ > > > │ ACPI0017 │ > > > │ root0 │ > > > └────┬─────┘ > > > │ > > > ┌───┴────┐ > > > │ dport0 │ > > > │ACPI0016│ > > > └───┬────┘ > > > │ > > > ┌────┴─────┐ > > > │endpoint0 │ > > > │ port1 │ > > > └──────────┘ > > > > > > So arrange for endpoint port in the RCH/RCD case to appear directly > > > connected to the host-bridge in its singular role as a dport. Compare > > > that to the VH case where the host-bridge serves a dual role as a > > > 'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port for > > > the Root Ports in the Root Complex that are modeled as 'cxl_dport' > > > instances in the CXL topology. > > > > > > Another deviation from the VH case is that RCDs may need to look up > > > their component registers from the Root Complex Register Block (RCRB). > > > That platform firmware specified RCRB area is cached by the cxl_acpi > > > driver and conveyed via the host-bridge dport to the cxl_mem driver to > > > perform the cxl_rcrb_to_component() lookup for the endpoint port > > > (See 9.11.8 CXL Devices Attached to an RCH for the lookup of the > > > upstream port component registers). > > > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > --- > > > drivers/cxl/core/port.c | 11 +++++++++-- > > > drivers/cxl/cxlmem.h | 2 ++ > > > drivers/cxl/mem.c | 31 ++++++++++++++++++++++++------- > > > drivers/cxl/pci.c | 10 ++++++++++ > > > 4 files changed, 45 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > > > index c7f58282b2c1..2385ee00eb9a 100644 > > > --- a/drivers/cxl/core/port.c > > > +++ b/drivers/cxl/core/port.c > > > @@ -1358,8 +1358,17 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > > { > > > struct device *dev = &cxlmd->dev; > > > struct device *iter; > > > + struct cxl_dport *dport; > > > + struct cxl_port *port; > > > > There is no direct need to move that code here. > > > > If you want to clean that up in this patch too, then leave a comment > > in the change log? > > Oh, good point, must have been left over from an earlier revision of the > patch, dropped it. > > > > > > int rc; > > > > > > + /* > > > + * Skip intermediate port enumeration in the RCH case, there > > > + * are no ports in between a host bridge and an endpoint. > > > + */ > > > + if (cxlmd->cxlds->rcd) > > > + return 0; > > > + > > > rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd); > > > if (rc) > > > return rc; > > > @@ -1373,8 +1382,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > > for (iter = dev; iter; iter = grandparent(iter)) { > > > struct device *dport_dev = grandparent(iter); > > > struct device *uport_dev; > > > - struct cxl_dport *dport; > > > - struct cxl_port *port; > > > > > > if (!dport_dev) > > > return 0; > > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > > > index e082991bc58c..35d485d041f0 100644 > > > --- a/drivers/cxl/cxlmem.h > > > +++ b/drivers/cxl/cxlmem.h > > > @@ -201,6 +201,7 @@ struct cxl_endpoint_dvsec_info { > > > * @dev: The device associated with this CXL state > > > * @regs: Parsed register blocks > > > * @cxl_dvsec: Offset to the PCIe device DVSEC > > > + * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH) > > > * @payload_size: Size of space for payload > > > * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register) > > > * @lsa_size: Size of Label Storage Area > > > @@ -235,6 +236,7 @@ struct cxl_dev_state { > > > struct cxl_regs regs; > > > int cxl_dvsec; > > > > > > + bool rcd; > > > size_t payload_size; > > > size_t lsa_size; > > > struct mutex mbox_mutex; /* Protects device mailbox and firmware */ > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > > index aa63ce8c7ca6..9a655b4b5e52 100644 > > > --- a/drivers/cxl/mem.c > > > +++ b/drivers/cxl/mem.c > > > @@ -45,12 +45,13 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data) > > > return 0; > > > } > > > > > > -static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > > > +static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd, > > > struct cxl_dport *parent_dport) > > > { > > > struct cxl_port *parent_port = parent_dport->port; > > > struct cxl_dev_state *cxlds = cxlmd->cxlds; > > > struct cxl_port *endpoint, *iter, *down; > > > + resource_size_t component_reg_phys; > > > int rc; > > > > > > /* > > > @@ -65,8 +66,18 @@ static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > > > ep->next = down; > > > } > > > > > > - endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev, > > > - cxlds->component_reg_phys, parent_dport); > > > + /* > > > + * The component registers for an RCD might come from the > > > + * host-bridge RCRB if they are not already mapped via the > > > + * typical register locator mechanism. > > > + */ > > > + if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) > > > + component_reg_phys = cxl_rcrb_to_component( > > > + &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_DOWNSTREAM); > > > > As already commented: this must be the upstream RCRB here. > > > > > + else > > > + component_reg_phys = cxlds->component_reg_phys; > > > + endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys, > > > + parent_dport); > > > > Looking at CXL 3.0 spec, table 8-22, there are the various sources of > > component registers listed. For RCD we need: D1, DP1, UP1 (optional > > R). > > > > D1: endpoint->component_reg_phys; > > UP1: parent_port-component_reg_phys; (missing in RCH topology) > > DP1: parent_dport->component_reg_phys; > > > > I don't see how all of them could be stored in this data layout as the > > cxl host port is missing. > > If I am understanding your concern correctly, that's handled here: > > if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) > > In the D1 case cxlds->component_reg_phys will be valid since the > component registers were visible via the register locator DVSEC and > retrieved by cxl_pci. In the UP1 case cxlds->component_reg_phys is > invalid and the driver falls back to the RCRB. DP1 is handled in > cxl_acpi. I.e. the D1 and UP1 cases do not co-exist. What I mean is we must store all 3 component reg base addresses for later access. E.g., if there is an AER error of a pci dev or the host, we must (depending on the error details) access CXL RAS status of either D1, UP1 or DP1. So for all 3 of them there must be a way to determine this walking through the port hierarchy. In the above list of locations I don't where UP1's component reg base address is stored. -Robert
Robert Richter wrote: > On 28.11.22 16:25:18, Dan Williams wrote: > > Robert Richter wrote: > > > On 24.11.22 10:35:32, Dan Williams wrote: > > > > Unlike a CXL memory expander in a VH topology that has at least one > > > > intervening 'struct cxl_port' instance between itself and the CXL root > > > > device, an RCD attaches one-level higher. For example: > > > > > > > > VH > > > > ┌──────────┐ > > > > │ ACPI0017 │ > > > > │ root0 │ > > > > └─────┬────┘ > > > > │ > > > > ┌─────┴────┐ > > > > │ dport0 │ > > > > ┌─────┤ ACPI0016 ├─────┐ > > > > │ │ port1 │ │ > > > > │ └────┬─────┘ │ > > > > │ │ │ > > > > ┌──┴───┐ ┌──┴───┐ ┌───┴──┐ > > > > │dport0│ │dport1│ │dport2│ > > > > │ RP0 │ │ RP1 │ │ RP2 │ > > > > └──────┘ └──┬───┘ └──────┘ > > > > │ > > > > ┌───┴─────┐ > > > > │endpoint0│ > > > > │ port2 │ > > > > └─────────┘ > > > > > > > > ...vs: > > > > > > > > RCH > > > > ┌──────────┐ > > > > │ ACPI0017 │ > > > > │ root0 │ > > > > └────┬─────┘ > > > > │ > > > > ┌───┴────┐ > > > > │ dport0 │ > > > > │ACPI0016│ > > > > └───┬────┘ > > > > │ > > > > ┌────┴─────┐ > > > > │endpoint0 │ > > > > │ port1 │ > > > > └──────────┘ > > > > > > > > So arrange for endpoint port in the RCH/RCD case to appear directly > > > > connected to the host-bridge in its singular role as a dport. Compare > > > > that to the VH case where the host-bridge serves a dual role as a > > > > 'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port for > > > > the Root Ports in the Root Complex that are modeled as 'cxl_dport' > > > > instances in the CXL topology. > > > > > > > > Another deviation from the VH case is that RCDs may need to look up > > > > their component registers from the Root Complex Register Block (RCRB). > > > > That platform firmware specified RCRB area is cached by the cxl_acpi > > > > driver and conveyed via the host-bridge dport to the cxl_mem driver to > > > > perform the cxl_rcrb_to_component() lookup for the endpoint port > > > > (See 9.11.8 CXL Devices Attached to an RCH for the lookup of the > > > > upstream port component registers). > > > > > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > > --- > > > > drivers/cxl/core/port.c | 11 +++++++++-- > > > > drivers/cxl/cxlmem.h | 2 ++ > > > > drivers/cxl/mem.c | 31 ++++++++++++++++++++++++------- > > > > drivers/cxl/pci.c | 10 ++++++++++ > > > > 4 files changed, 45 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > > > > index c7f58282b2c1..2385ee00eb9a 100644 > > > > --- a/drivers/cxl/core/port.c > > > > +++ b/drivers/cxl/core/port.c > > > > @@ -1358,8 +1358,17 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > > > { > > > > struct device *dev = &cxlmd->dev; > > > > struct device *iter; > > > > + struct cxl_dport *dport; > > > > + struct cxl_port *port; > > > > > > There is no direct need to move that code here. > > > > > > If you want to clean that up in this patch too, then leave a comment > > > in the change log? > > > > Oh, good point, must have been left over from an earlier revision of the > > patch, dropped it. > > > > > > > > > int rc; > > > > > > > > + /* > > > > + * Skip intermediate port enumeration in the RCH case, there > > > > + * are no ports in between a host bridge and an endpoint. > > > > + */ > > > > + if (cxlmd->cxlds->rcd) > > > > + return 0; > > > > + > > > > rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd); > > > > if (rc) > > > > return rc; > > > > @@ -1373,8 +1382,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > > > for (iter = dev; iter; iter = grandparent(iter)) { > > > > struct device *dport_dev = grandparent(iter); > > > > struct device *uport_dev; > > > > - struct cxl_dport *dport; > > > > - struct cxl_port *port; > > > > > > > > if (!dport_dev) > > > > return 0; > > > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > > > > index e082991bc58c..35d485d041f0 100644 > > > > --- a/drivers/cxl/cxlmem.h > > > > +++ b/drivers/cxl/cxlmem.h > > > > @@ -201,6 +201,7 @@ struct cxl_endpoint_dvsec_info { > > > > * @dev: The device associated with this CXL state > > > > * @regs: Parsed register blocks > > > > * @cxl_dvsec: Offset to the PCIe device DVSEC > > > > + * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH) > > > > * @payload_size: Size of space for payload > > > > * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register) > > > > * @lsa_size: Size of Label Storage Area > > > > @@ -235,6 +236,7 @@ struct cxl_dev_state { > > > > struct cxl_regs regs; > > > > int cxl_dvsec; > > > > > > > > + bool rcd; > > > > size_t payload_size; > > > > size_t lsa_size; > > > > struct mutex mbox_mutex; /* Protects device mailbox and firmware */ > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > > > index aa63ce8c7ca6..9a655b4b5e52 100644 > > > > --- a/drivers/cxl/mem.c > > > > +++ b/drivers/cxl/mem.c > > > > @@ -45,12 +45,13 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data) > > > > return 0; > > > > } > > > > > > > > -static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > > > > +static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd, > > > > struct cxl_dport *parent_dport) > > > > { > > > > struct cxl_port *parent_port = parent_dport->port; > > > > struct cxl_dev_state *cxlds = cxlmd->cxlds; > > > > struct cxl_port *endpoint, *iter, *down; > > > > + resource_size_t component_reg_phys; > > > > int rc; > > > > > > > > /* > > > > @@ -65,8 +66,18 @@ static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > > > > ep->next = down; > > > > } > > > > > > > > - endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev, > > > > - cxlds->component_reg_phys, parent_dport); > > > > + /* > > > > + * The component registers for an RCD might come from the > > > > + * host-bridge RCRB if they are not already mapped via the > > > > + * typical register locator mechanism. > > > > + */ > > > > + if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) > > > > + component_reg_phys = cxl_rcrb_to_component( > > > > + &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_DOWNSTREAM); > > > > > > As already commented: this must be the upstream RCRB here. > > > > > > > + else > > > > + component_reg_phys = cxlds->component_reg_phys; > > > > + endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys, > > > > + parent_dport); > > > > > > Looking at CXL 3.0 spec, table 8-22, there are the various sources of > > > component registers listed. For RCD we need: D1, DP1, UP1 (optional > > > R). > > > > > > D1: endpoint->component_reg_phys; > > > UP1: parent_port-component_reg_phys; (missing in RCH topology) > > > DP1: parent_dport->component_reg_phys; > > > > > > I don't see how all of them could be stored in this data layout as the > > > cxl host port is missing. > > > > If I am understanding your concern correctly, that's handled here: > > > > if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) > > > > In the D1 case cxlds->component_reg_phys will be valid since the > > component registers were visible via the register locator DVSEC and > > retrieved by cxl_pci. In the UP1 case cxlds->component_reg_phys is > > invalid and the driver falls back to the RCRB. DP1 is handled in > > cxl_acpi. I.e. the D1 and UP1 cases do not co-exist. > > What I mean is we must store all 3 component reg base addresses for > later access. E.g., if there is an AER error of a pci dev or the host, > we must (depending on the error details) access CXL RAS status of > either D1, UP1 or DP1. So for all 3 of them there must be a way to > determine this walking through the port hierarchy. In the above list > of locations I don't where UP1's component reg base address is stored. So I think we are reading the specification differently. I am comparing Figure 9-7 "CXL Device Remaps Upstream Port and Component Registers" and Figure Figure 9-8 "CXL Device that Does Not Remap Upstream Port and Component Registers" and noting that there is never a case where three sets of component registers are visible. It is either DP1 connected to UP1 (Figure 9-7) or DP1 connected to D1 (Figure 9-8). There is never a case where the code needs to consider UP1 and D1 component registers at the same time because those things are identical just enumerated differently depending on how the endpoint is implemented.
On 30.11.22 12:39:00, Dan Williams wrote: > Robert Richter wrote: > > On 28.11.22 16:25:18, Dan Williams wrote: > > > Robert Richter wrote: > > > > On 24.11.22 10:35:32, Dan Williams wrote: > > > > > Unlike a CXL memory expander in a VH topology that has at least one > > > > > intervening 'struct cxl_port' instance between itself and the CXL root > > > > > device, an RCD attaches one-level higher. For example: > > > > > > > > > > VH > > > > > ┌──────────┐ > > > > > │ ACPI0017 │ > > > > > │ root0 │ > > > > > └─────┬────┘ > > > > > │ > > > > > ┌─────┴────┐ > > > > > │ dport0 │ > > > > > ┌─────┤ ACPI0016 ├─────┐ > > > > > │ │ port1 │ │ > > > > > │ └────┬─────┘ │ > > > > > │ │ │ > > > > > ┌──┴───┐ ┌──┴───┐ ┌───┴──┐ > > > > > │dport0│ │dport1│ │dport2│ > > > > > │ RP0 │ │ RP1 │ │ RP2 │ > > > > > └──────┘ └──┬───┘ └──────┘ > > > > > │ > > > > > ┌───┴─────┐ > > > > > │endpoint0│ > > > > > │ port2 │ > > > > > └─────────┘ > > > > > > > > > > ...vs: > > > > > > > > > > RCH > > > > > ┌──────────┐ > > > > > │ ACPI0017 │ > > > > > │ root0 │ > > > > > └────┬─────┘ > > > > > │ > > > > > ┌───┴────┐ > > > > > │ dport0 │ > > > > > │ACPI0016│ > > > > > └───┬────┘ > > > > > │ > > > > > ┌────┴─────┐ > > > > > │endpoint0 │ > > > > > │ port1 │ > > > > > └──────────┘ > > > > > > > > > > So arrange for endpoint port in the RCH/RCD case to appear directly > > > > > connected to the host-bridge in its singular role as a dport. Compare > > > > > that to the VH case where the host-bridge serves a dual role as a > > > > > 'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port for > > > > > the Root Ports in the Root Complex that are modeled as 'cxl_dport' > > > > > instances in the CXL topology. > > > > > > > > > > Another deviation from the VH case is that RCDs may need to look up > > > > > their component registers from the Root Complex Register Block (RCRB). > > > > > That platform firmware specified RCRB area is cached by the cxl_acpi > > > > > driver and conveyed via the host-bridge dport to the cxl_mem driver to > > > > > perform the cxl_rcrb_to_component() lookup for the endpoint port > > > > > (See 9.11.8 CXL Devices Attached to an RCH for the lookup of the > > > > > upstream port component registers). > > > > > > > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > > > --- > > > > > drivers/cxl/core/port.c | 11 +++++++++-- > > > > > drivers/cxl/cxlmem.h | 2 ++ > > > > > drivers/cxl/mem.c | 31 ++++++++++++++++++++++++------- > > > > > drivers/cxl/pci.c | 10 ++++++++++ > > > > > 4 files changed, 45 insertions(+), 9 deletions(-) > > > > > > > > > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > > > > > index c7f58282b2c1..2385ee00eb9a 100644 > > > > > --- a/drivers/cxl/core/port.c > > > > > +++ b/drivers/cxl/core/port.c > > > > > @@ -1358,8 +1358,17 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > > > > { > > > > > struct device *dev = &cxlmd->dev; > > > > > struct device *iter; > > > > > + struct cxl_dport *dport; > > > > > + struct cxl_port *port; > > > > > > > > There is no direct need to move that code here. > > > > > > > > If you want to clean that up in this patch too, then leave a comment > > > > in the change log? > > > > > > Oh, good point, must have been left over from an earlier revision of the > > > patch, dropped it. > > > > > > > > > > > > int rc; > > > > > > > > > > + /* > > > > > + * Skip intermediate port enumeration in the RCH case, there > > > > > + * are no ports in between a host bridge and an endpoint. > > > > > + */ > > > > > + if (cxlmd->cxlds->rcd) > > > > > + return 0; > > > > > + > > > > > rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd); > > > > > if (rc) > > > > > return rc; > > > > > @@ -1373,8 +1382,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > > > > for (iter = dev; iter; iter = grandparent(iter)) { > > > > > struct device *dport_dev = grandparent(iter); > > > > > struct device *uport_dev; > > > > > - struct cxl_dport *dport; > > > > > - struct cxl_port *port; > > > > > > > > > > if (!dport_dev) > > > > > return 0; > > > > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > > > > > index e082991bc58c..35d485d041f0 100644 > > > > > --- a/drivers/cxl/cxlmem.h > > > > > +++ b/drivers/cxl/cxlmem.h > > > > > @@ -201,6 +201,7 @@ struct cxl_endpoint_dvsec_info { > > > > > * @dev: The device associated with this CXL state > > > > > * @regs: Parsed register blocks > > > > > * @cxl_dvsec: Offset to the PCIe device DVSEC > > > > > + * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH) > > > > > * @payload_size: Size of space for payload > > > > > * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register) > > > > > * @lsa_size: Size of Label Storage Area > > > > > @@ -235,6 +236,7 @@ struct cxl_dev_state { > > > > > struct cxl_regs regs; > > > > > int cxl_dvsec; > > > > > > > > > > + bool rcd; > > > > > size_t payload_size; > > > > > size_t lsa_size; > > > > > struct mutex mbox_mutex; /* Protects device mailbox and firmware */ > > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > > > > index aa63ce8c7ca6..9a655b4b5e52 100644 > > > > > --- a/drivers/cxl/mem.c > > > > > +++ b/drivers/cxl/mem.c > > > > > @@ -45,12 +45,13 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data) > > > > > return 0; > > > > > } > > > > > > > > > > -static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > > > > > +static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd, > > > > > struct cxl_dport *parent_dport) > > > > > { > > > > > struct cxl_port *parent_port = parent_dport->port; > > > > > struct cxl_dev_state *cxlds = cxlmd->cxlds; > > > > > struct cxl_port *endpoint, *iter, *down; > > > > > + resource_size_t component_reg_phys; > > > > > int rc; > > > > > > > > > > /* > > > > > @@ -65,8 +66,18 @@ static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > > > > > ep->next = down; > > > > > } > > > > > > > > > > - endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev, > > > > > - cxlds->component_reg_phys, parent_dport); > > > > > + /* > > > > > + * The component registers for an RCD might come from the > > > > > + * host-bridge RCRB if they are not already mapped via the > > > > > + * typical register locator mechanism. > > > > > + */ > > > > > + if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) > > > > > + component_reg_phys = cxl_rcrb_to_component( > > > > > + &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_DOWNSTREAM); > > > > > > > > As already commented: this must be the upstream RCRB here. > > > > > > > > > + else > > > > > + component_reg_phys = cxlds->component_reg_phys; > > > > > + endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys, > > > > > + parent_dport); > > > > > > > > Looking at CXL 3.0 spec, table 8-22, there are the various sources of > > > > component registers listed. For RCD we need: D1, DP1, UP1 (optional > > > > R). > > > > > > > > D1: endpoint->component_reg_phys; > > > > UP1: parent_port-component_reg_phys; (missing in RCH topology) > > > > DP1: parent_dport->component_reg_phys; > > > > > > > > I don't see how all of them could be stored in this data layout as the > > > > cxl host port is missing. > > > > > > If I am understanding your concern correctly, that's handled here: > > > > > > if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) > > > > > > In the D1 case cxlds->component_reg_phys will be valid since the > > > component registers were visible via the register locator DVSEC and > > > retrieved by cxl_pci. In the UP1 case cxlds->component_reg_phys is > > > invalid and the driver falls back to the RCRB. DP1 is handled in > > > cxl_acpi. I.e. the D1 and UP1 cases do not co-exist. > > > > What I mean is we must store all 3 component reg base addresses for > > later access. E.g., if there is an AER error of a pci dev or the host, > > we must (depending on the error details) access CXL RAS status of > > either D1, UP1 or DP1. So for all 3 of them there must be a way to > > determine this walking through the port hierarchy. In the above list > > of locations I don't where UP1's component reg base address is stored. > > So I think we are reading the specification differently. I am comparing > Figure 9-7 "CXL Device Remaps Upstream Port and Component Registers" and > Figure Figure 9-8 "CXL Device that Does Not Remap Upstream Port and > Component Registers" and noting that there is never a case where three > sets of component registers are visible. It is either DP1 connected to > UP1 (Figure 9-7) or DP1 connected to D1 (Figure 9-8). There is never a > case where the code needs to consider UP1 and D1 component registers at > the same time because those things are identical just enumerated > differently depending on how the endpoint is implemented. Yes, the spec is ambiguous here. Looking at CXL 3.0, Figure 12-3 I also tend to agree the RCiEP's CXL RAS cap is used for that ("UP Z sends an error message to all CXL.io Functions that are affected by this error."). Unfortunately 12.2.1.2 does not state where the CXL RAS Capability that logs the error resides. -Robert
On 01.12.22 16:23:46, Robert Richter wrote: > On 30.11.22 12:39:00, Dan Williams wrote: > > Robert Richter wrote: > > > On 28.11.22 16:25:18, Dan Williams wrote: > > > > Robert Richter wrote: > > > > > On 24.11.22 10:35:32, Dan Williams wrote: > > > > > > Unlike a CXL memory expander in a VH topology that has at least one > > > > > > intervening 'struct cxl_port' instance between itself and the CXL root > > > > > > device, an RCD attaches one-level higher. For example: > > > > > > > > > > > > VH > > > > > > ┌──────────┐ > > > > > > │ ACPI0017 │ > > > > > > │ root0 │ > > > > > > └─────┬────┘ > > > > > > │ > > > > > > ┌─────┴────┐ > > > > > > │ dport0 │ > > > > > > ┌─────┤ ACPI0016 ├─────┐ > > > > > > │ │ port1 │ │ > > > > > > │ └────┬─────┘ │ > > > > > > │ │ │ > > > > > > ┌──┴───┐ ┌──┴───┐ ┌───┴──┐ > > > > > > │dport0│ │dport1│ │dport2│ > > > > > > │ RP0 │ │ RP1 │ │ RP2 │ > > > > > > └──────┘ └──┬───┘ └──────┘ > > > > > > │ > > > > > > ┌───┴─────┐ > > > > > > │endpoint0│ > > > > > > │ port2 │ > > > > > > └─────────┘ > > > > > > > > > > > > ...vs: > > > > > > > > > > > > RCH > > > > > > ┌──────────┐ > > > > > > │ ACPI0017 │ > > > > > > │ root0 │ > > > > > > └────┬─────┘ > > > > > > │ > > > > > > ┌───┴────┐ > > > > > > │ dport0 │ > > > > > > │ACPI0016│ > > > > > > └───┬────┘ > > > > > > │ > > > > > > ┌────┴─────┐ > > > > > > │endpoint0 │ > > > > > > │ port1 │ > > > > > > └──────────┘ > > > > > > > > > > > > So arrange for endpoint port in the RCH/RCD case to appear directly > > > > > > connected to the host-bridge in its singular role as a dport. Compare > > > > > > that to the VH case where the host-bridge serves a dual role as a > > > > > > 'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port for > > > > > > the Root Ports in the Root Complex that are modeled as 'cxl_dport' > > > > > > instances in the CXL topology. > > > > > > > > > > > > Another deviation from the VH case is that RCDs may need to look up > > > > > > their component registers from the Root Complex Register Block (RCRB). > > > > > > That platform firmware specified RCRB area is cached by the cxl_acpi > > > > > > driver and conveyed via the host-bridge dport to the cxl_mem driver to > > > > > > perform the cxl_rcrb_to_component() lookup for the endpoint port > > > > > > (See 9.11.8 CXL Devices Attached to an RCH for the lookup of the > > > > > > upstream port component registers). > > > > > > > > > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > > > > --- > > > > > > drivers/cxl/core/port.c | 11 +++++++++-- > > > > > > drivers/cxl/cxlmem.h | 2 ++ > > > > > > drivers/cxl/mem.c | 31 ++++++++++++++++++++++++------- > > > > > > drivers/cxl/pci.c | 10 ++++++++++ > > > > > > 4 files changed, 45 insertions(+), 9 deletions(-) > > > > > > > > > > > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > > > > > > index c7f58282b2c1..2385ee00eb9a 100644 > > > > > > --- a/drivers/cxl/core/port.c > > > > > > +++ b/drivers/cxl/core/port.c > > > > > > @@ -1358,8 +1358,17 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > > > > > { > > > > > > struct device *dev = &cxlmd->dev; > > > > > > struct device *iter; > > > > > > + struct cxl_dport *dport; > > > > > > + struct cxl_port *port; > > > > > > > > > > There is no direct need to move that code here. > > > > > > > > > > If you want to clean that up in this patch too, then leave a comment > > > > > in the change log? > > > > > > > > Oh, good point, must have been left over from an earlier revision of the > > > > patch, dropped it. > > > > > > > > > > > > > > > int rc; > > > > > > > > > > > > + /* > > > > > > + * Skip intermediate port enumeration in the RCH case, there > > > > > > + * are no ports in between a host bridge and an endpoint. > > > > > > + */ > > > > > > + if (cxlmd->cxlds->rcd) > > > > > > + return 0; > > > > > > + > > > > > > rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd); > > > > > > if (rc) > > > > > > return rc; > > > > > > @@ -1373,8 +1382,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > > > > > for (iter = dev; iter; iter = grandparent(iter)) { > > > > > > struct device *dport_dev = grandparent(iter); > > > > > > struct device *uport_dev; > > > > > > - struct cxl_dport *dport; > > > > > > - struct cxl_port *port; > > > > > > > > > > > > if (!dport_dev) > > > > > > return 0; > > > > > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > > > > > > index e082991bc58c..35d485d041f0 100644 > > > > > > --- a/drivers/cxl/cxlmem.h > > > > > > +++ b/drivers/cxl/cxlmem.h > > > > > > @@ -201,6 +201,7 @@ struct cxl_endpoint_dvsec_info { > > > > > > * @dev: The device associated with this CXL state > > > > > > * @regs: Parsed register blocks > > > > > > * @cxl_dvsec: Offset to the PCIe device DVSEC > > > > > > + * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH) > > > > > > * @payload_size: Size of space for payload > > > > > > * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register) > > > > > > * @lsa_size: Size of Label Storage Area > > > > > > @@ -235,6 +236,7 @@ struct cxl_dev_state { > > > > > > struct cxl_regs regs; > > > > > > int cxl_dvsec; > > > > > > > > > > > > + bool rcd; > > > > > > size_t payload_size; > > > > > > size_t lsa_size; > > > > > > struct mutex mbox_mutex; /* Protects device mailbox and firmware */ > > > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > > > > > index aa63ce8c7ca6..9a655b4b5e52 100644 > > > > > > --- a/drivers/cxl/mem.c > > > > > > +++ b/drivers/cxl/mem.c > > > > > > @@ -45,12 +45,13 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > -static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > > > > > > +static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd, > > > > > > struct cxl_dport *parent_dport) > > > > > > { > > > > > > struct cxl_port *parent_port = parent_dport->port; > > > > > > struct cxl_dev_state *cxlds = cxlmd->cxlds; > > > > > > struct cxl_port *endpoint, *iter, *down; > > > > > > + resource_size_t component_reg_phys; > > > > > > int rc; > > > > > > > > > > > > /* > > > > > > @@ -65,8 +66,18 @@ static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, > > > > > > ep->next = down; > > > > > > } > > > > > > > > > > > > - endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev, > > > > > > - cxlds->component_reg_phys, parent_dport); > > > > > > + /* > > > > > > + * The component registers for an RCD might come from the > > > > > > + * host-bridge RCRB if they are not already mapped via the > > > > > > + * typical register locator mechanism. > > > > > > + */ > > > > > > + if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) > > > > > > + component_reg_phys = cxl_rcrb_to_component( > > > > > > + &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_DOWNSTREAM); > > > > > > > > > > As already commented: this must be the upstream RCRB here. > > > > > > > > > > > + else > > > > > > + component_reg_phys = cxlds->component_reg_phys; > > > > > > + endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys, > > > > > > + parent_dport); > > > > > > > > > > Looking at CXL 3.0 spec, table 8-22, there are the various sources of > > > > > component registers listed. For RCD we need: D1, DP1, UP1 (optional > > > > > R). > > > > > > > > > > D1: endpoint->component_reg_phys; > > > > > UP1: parent_port-component_reg_phys; (missing in RCH topology) > > > > > DP1: parent_dport->component_reg_phys; > > > > > > > > > > I don't see how all of them could be stored in this data layout as the > > > > > cxl host port is missing. > > > > > > > > If I am understanding your concern correctly, that's handled here: > > > > > > > > if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) > > > > > > > > In the D1 case cxlds->component_reg_phys will be valid since the > > > > component registers were visible via the register locator DVSEC and > > > > retrieved by cxl_pci. In the UP1 case cxlds->component_reg_phys is > > > > invalid and the driver falls back to the RCRB. DP1 is handled in > > > > cxl_acpi. I.e. the D1 and UP1 cases do not co-exist. > > > > > > What I mean is we must store all 3 component reg base addresses for > > > later access. E.g., if there is an AER error of a pci dev or the host, > > > we must (depending on the error details) access CXL RAS status of > > > either D1, UP1 or DP1. So for all 3 of them there must be a way to > > > determine this walking through the port hierarchy. In the above list > > > of locations I don't where UP1's component reg base address is stored. > > > > So I think we are reading the specification differently. I am comparing > > Figure 9-7 "CXL Device Remaps Upstream Port and Component Registers" and > > Figure Figure 9-8 "CXL Device that Does Not Remap Upstream Port and > > Component Registers" and noting that there is never a case where three > > sets of component registers are visible. It is either DP1 connected to > > UP1 (Figure 9-7) or DP1 connected to D1 (Figure 9-8). There is never a > > case where the code needs to consider UP1 and D1 component registers at > > the same time because those things are identical just enumerated > > differently depending on how the endpoint is implemented. > > Yes, the spec is ambiguous here. Looking at CXL 3.0, Figure 12-3 I > also tend to agree the RCiEP's CXL RAS cap is used for that ("UP Z > sends an error message to all CXL.io Functions that are affected by > this error."). Unfortunately 12.2.1.2 does not state where the CXL RAS > Capability that logs the error resides. I found it now in 8.2.2 of the CXL 3.0 spec: For an RCD upstream port the RCRB should be used to locate the component registers. If the UP's RCRB is not implemented, then the Register Locator DVSEC (of the RCiEP) should be used. So right, UP1 and D1 component registers are never used at the same time. Thanks for clarification, -Robert
On 24.11.22 10:35:32, Dan Williams wrote: > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > @@ -119,17 +131,22 @@ static int cxl_mem_probe(struct device *dev) > return -ENXIO; > } > > - device_lock(&parent_port->dev); > - if (!parent_port->dev.driver) { > + if (dport->rch) > + endpoint_parent = parent_port->uport; > + else > + endpoint_parent = &parent_port->dev; > + > + device_lock(endpoint_parent); > + if (!endpoint_parent->driver) { > dev_err(dev, "CXL port topology %s not enabled\n", > dev_name(&parent_port->dev)); This must be dev_name(endpoint_parent) here. > rc = -ENXIO; > goto unlock; > } > > - rc = devm_cxl_add_endpoint(cxlmd, dport); > + rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); > unlock: > - device_unlock(&parent_port->dev); > + device_unlock(endpoint_parent); > put_device(&parent_port->dev); This is correct and the counterpart to cxl_mem_find_port(). > if (rc) > return rc; -Robert
Robert Richter wrote: > On 24.11.22 10:35:32, Dan Williams wrote: > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > > @@ -119,17 +131,22 @@ static int cxl_mem_probe(struct device *dev) > > return -ENXIO; > > } > > > > - device_lock(&parent_port->dev); > > - if (!parent_port->dev.driver) { > > + if (dport->rch) > > + endpoint_parent = parent_port->uport; > > + else > > + endpoint_parent = &parent_port->dev; > > + > > + device_lock(endpoint_parent); > > + if (!endpoint_parent->driver) { > > dev_err(dev, "CXL port topology %s not enabled\n", > > dev_name(&parent_port->dev)); > > This must be dev_name(endpoint_parent) here. Indeed, good catch.
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index c7f58282b2c1..2385ee00eb9a 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1358,8 +1358,17 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) { struct device *dev = &cxlmd->dev; struct device *iter; + struct cxl_dport *dport; + struct cxl_port *port; int rc; + /* + * Skip intermediate port enumeration in the RCH case, there + * are no ports in between a host bridge and an endpoint. + */ + if (cxlmd->cxlds->rcd) + return 0; + rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd); if (rc) return rc; @@ -1373,8 +1382,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) for (iter = dev; iter; iter = grandparent(iter)) { struct device *dport_dev = grandparent(iter); struct device *uport_dev; - struct cxl_dport *dport; - struct cxl_port *port; if (!dport_dev) return 0; diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index e082991bc58c..35d485d041f0 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -201,6 +201,7 @@ struct cxl_endpoint_dvsec_info { * @dev: The device associated with this CXL state * @regs: Parsed register blocks * @cxl_dvsec: Offset to the PCIe device DVSEC + * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH) * @payload_size: Size of space for payload * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register) * @lsa_size: Size of Label Storage Area @@ -235,6 +236,7 @@ struct cxl_dev_state { struct cxl_regs regs; int cxl_dvsec; + bool rcd; size_t payload_size; size_t lsa_size; struct mutex mbox_mutex; /* Protects device mailbox and firmware */ diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index aa63ce8c7ca6..9a655b4b5e52 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -45,12 +45,13 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data) return 0; } -static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, +static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd, struct cxl_dport *parent_dport) { struct cxl_port *parent_port = parent_dport->port; struct cxl_dev_state *cxlds = cxlmd->cxlds; struct cxl_port *endpoint, *iter, *down; + resource_size_t component_reg_phys; int rc; /* @@ -65,8 +66,18 @@ static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd, ep->next = down; } - endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev, - cxlds->component_reg_phys, parent_dport); + /* + * The component registers for an RCD might come from the + * host-bridge RCRB if they are not already mapped via the + * typical register locator mechanism. + */ + if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) + component_reg_phys = cxl_rcrb_to_component( + &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_DOWNSTREAM); + else + component_reg_phys = cxlds->component_reg_phys; + endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys, + parent_dport); if (IS_ERR(endpoint)) return PTR_ERR(endpoint); @@ -87,6 +98,7 @@ static int cxl_mem_probe(struct device *dev) { struct cxl_memdev *cxlmd = to_cxl_memdev(dev); struct cxl_dev_state *cxlds = cxlmd->cxlds; + struct device *endpoint_parent; struct cxl_port *parent_port; struct cxl_dport *dport; struct dentry *dentry; @@ -119,17 +131,22 @@ static int cxl_mem_probe(struct device *dev) return -ENXIO; } - device_lock(&parent_port->dev); - if (!parent_port->dev.driver) { + if (dport->rch) + endpoint_parent = parent_port->uport; + else + endpoint_parent = &parent_port->dev; + + device_lock(endpoint_parent); + if (!endpoint_parent->driver) { dev_err(dev, "CXL port topology %s not enabled\n", dev_name(&parent_port->dev)); rc = -ENXIO; goto unlock; } - rc = devm_cxl_add_endpoint(cxlmd, dport); + rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); unlock: - device_unlock(&parent_port->dev); + device_unlock(endpoint_parent); put_device(&parent_port->dev); if (rc) return rc; diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index e15da405b948..73ff6c33a0c0 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -433,6 +433,15 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds) } } +/* + * Assume that any RCIEP that emits the CXL memory expander class code + * is an RCD + */ +static bool is_cxl_restricted(struct pci_dev *pdev) +{ + return pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END; +} + static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct cxl_register_map map; @@ -455,6 +464,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (IS_ERR(cxlds)) return PTR_ERR(cxlds); + cxlds->rcd = is_cxl_restricted(pdev); cxlds->serial = pci_get_dsn(pdev); cxlds->cxl_dvsec = pci_find_dvsec_capability( pdev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
Unlike a CXL memory expander in a VH topology that has at least one intervening 'struct cxl_port' instance between itself and the CXL root device, an RCD attaches one-level higher. For example: VH ┌──────────┐ │ ACPI0017 │ │ root0 │ └─────┬────┘ │ ┌─────┴────┐ │ dport0 │ ┌─────┤ ACPI0016 ├─────┐ │ │ port1 │ │ │ └────┬─────┘ │ │ │ │ ┌──┴───┐ ┌──┴───┐ ┌───┴──┐ │dport0│ │dport1│ │dport2│ │ RP0 │ │ RP1 │ │ RP2 │ └──────┘ └──┬───┘ └──────┘ │ ┌───┴─────┐ │endpoint0│ │ port2 │ └─────────┘ ...vs: RCH ┌──────────┐ │ ACPI0017 │ │ root0 │ └────┬─────┘ │ ┌───┴────┐ │ dport0 │ │ACPI0016│ └───┬────┘ │ ┌────┴─────┐ │endpoint0 │ │ port1 │ └──────────┘ So arrange for endpoint port in the RCH/RCD case to appear directly connected to the host-bridge in its singular role as a dport. Compare that to the VH case where the host-bridge serves a dual role as a 'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port for the Root Ports in the Root Complex that are modeled as 'cxl_dport' instances in the CXL topology. Another deviation from the VH case is that RCDs may need to look up their component registers from the Root Complex Register Block (RCRB). That platform firmware specified RCRB area is cached by the cxl_acpi driver and conveyed via the host-bridge dport to the cxl_mem driver to perform the cxl_rcrb_to_component() lookup for the endpoint port (See 9.11.8 CXL Devices Attached to an RCH for the lookup of the upstream port component registers). Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/port.c | 11 +++++++++-- drivers/cxl/cxlmem.h | 2 ++ drivers/cxl/mem.c | 31 ++++++++++++++++++++++++------- drivers/cxl/pci.c | 10 ++++++++++ 4 files changed, 45 insertions(+), 9 deletions(-)