Robert Richter wrote:
> On 24.11.22 10:35:21, Dan Williams wrote:
> > From: Robert Richter <rrich...@amd.com>
> >
> > A downstream port must be connected to a component register block.
> > For restricted hosts the base address is determined from the RCRB. The
> > RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> > get the RCRB and add code to extract the component register block from
> > it.
> >
> > RCRB's BAR[0..1] point to the component block containing CXL subsystem
> > component registers. MEMBAR extraction follows the PCI base spec here,
> > esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1). The
> > RCRB base address is cached in the cxl_dport per-host bridge so that the
> > upstream port component registers can be retrieved later by an RCD
> > (RCIEP) associated with the host bridge.
> >
> > Note: Right now the component register block is used for HDM decoder
> > capability only which is optional for RCDs. If unsupported by the RCD,
> > the HDM init will fail. It is future work to bypass it in this case.
> >
> > Co-developed-by: Terry Bowman <terry.bow...@amd.com>
> > Signed-off-by: Terry Bowman <terry.bow...@amd.com>
> > Signed-off-by: Robert Richter <rrich...@amd.com>
> > [djbw: introduce devm_cxl_add_rch_dport()]
> > Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
> > ---
> > drivers/cxl/acpi.c | 54
> > ++++++++++++++++++++++++++++++++--------
> > drivers/cxl/core/port.c | 42 +++++++++++++++++++++++++++----
> > drivers/cxl/core/regs.c | 56
> > +++++++++++++++++++++++++++++++++++++++++
> > drivers/cxl/cxl.h | 16 ++++++++++++
> > tools/testing/cxl/Kbuild | 1 +
> > tools/testing/cxl/test/cxl.c | 10 +++++++
> > tools/testing/cxl/test/mock.c | 19 ++++++++++++++
> > tools/testing/cxl/test/mock.h | 3 ++
> > 8 files changed, 186 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 50d82376097c..1224add13529 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -9,6 +9,8 @@
> > #include "cxlpci.h"
> > #include "cxl.h"
> >
> > +#define CXL_RCRB_SIZE SZ_8K
> > +
> > static unsigned long cfmws_to_decoder_flags(int restrictions)
> > {
> > unsigned long flags = CXL_DECODER_F_ENABLE;
> > @@ -215,6 +217,11 @@ static int add_host_bridge_uport(struct device *match,
> > void *arg)
> > if (rc)
> > return rc;
> >
> > + if (dport->rch) {
> > + dev_info(bridge, "host supports CXL (restricted)\n");
> > + return 0;
> > + }
>
> This change comes after devm_cxl_register_pci_bus() to serve the
> cxl_port_to_pci_bus() in devm_cxl_port_enumerate_dports() in
> cxl_port_probe(). A root port is not probed and
> devm_cxl_port_enumerate_dports() will be never called, so we could
> jump out before devm_cxl_register_pci_bus().
Good point.
> On the other side we might want to be ready to use
> cxl_port_to_pci_bus() elsewhere in later changes. RCHs would not work
> then.
At least cxl_port_to_pci_bus() as is will work fine for the RCD
endpoint-cxl-port, so I think it is ok to leave this alone for now.
>
> > +
> > port = devm_cxl_add_port(host, bridge, dport->component_reg_phys,
> > dport);
> > if (IS_ERR(port))
> > @@ -228,27 +235,46 @@ static int add_host_bridge_uport(struct device
> > *match, void *arg)
> > struct cxl_chbs_context {
> > struct device *dev;
> > unsigned long long uid;
> > - resource_size_t chbcr;
> > + struct acpi_cedt_chbs chbs;
> > };
> >
> > -static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> > - const unsigned long end)
> > +static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> > + const unsigned long end)
> > {
> > struct cxl_chbs_context *ctx = arg;
> > struct acpi_cedt_chbs *chbs;
> >
> > - if (ctx->chbcr)
> > + if (ctx->chbs.base)
> > return 0;
> >
> > chbs = (struct acpi_cedt_chbs *) header;
> >
> > if (ctx->uid != chbs->uid)
> > return 0;
> > - ctx->chbcr = chbs->base;
> > + ctx->chbs = *chbs;
> >
> > return 0;
> > }
> >
> > +static resource_size_t cxl_get_chbcr(struct cxl_chbs_context *ctx)
> > +{
> > + struct acpi_cedt_chbs *chbs = &ctx->chbs;
> > +
> > + if (!chbs->base)
> > + return CXL_RESOURCE_NONE;
> > +
> > + if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11)
> > + return chbs->base;
> > +
> > + if (chbs->length != CXL_RCRB_SIZE)
> > + return CXL_RESOURCE_NONE;
> > +
> > + dev_dbg(ctx->dev, "RCRB found for UID %lld: %pa\n", ctx->uid,
> > + &chbs->base);
> > +
> > + return cxl_rcrb_to_component(ctx->dev, chbs->base, CXL_RCRB_DOWNSTREAM);
> > +}
> > +
>
> I have an improved version of this code which squashes cxl_get_chbcr()
> into cxl_get_chbs() (basically extends the original cxl_get_chbcr()
> function).
Care to send it? If I see it before the next posting I can fold it in,
otherwise it can be a follow-on cleanup.
>
> > static int add_host_bridge_dport(struct device *match, void *arg)
> > {
> > acpi_status status;
> > @@ -258,6 +284,7 @@ static int add_host_bridge_dport(struct device *match,
> > void *arg)
> > struct cxl_chbs_context ctx;
> > struct acpi_pci_root *pci_root;
> > struct cxl_port *root_port = arg;
> > + resource_size_t component_reg_phys;
> > struct device *host = root_port->dev.parent;
> > struct acpi_device *hb = to_cxl_host_bridge(host, match);
> >
> > @@ -274,21 +301,28 @@ static int add_host_bridge_dport(struct device
> > *match, void *arg)
> > dev_dbg(match, "UID found: %lld\n", uid);
> >
> > ctx = (struct cxl_chbs_context) {
> > - .dev = host,
> > + .dev = match,
> > .uid = uid,
> > };
> > - acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbcr, &ctx);
> > + acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs, &ctx);
> >
> > - if (ctx.chbcr == 0) {
> > + component_reg_phys = cxl_get_chbcr(&ctx);
> > + if (component_reg_phys == CXL_RESOURCE_NONE) {
> > dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n",
> > uid);
> > return 0;
> > }
> >
> > - dev_dbg(match, "CHBCR found: 0x%08llx\n", (u64)ctx.chbcr);
> > + dev_dbg(match, "CHBCR found: %pa\n", &component_reg_phys);
> >
> > pci_root = acpi_pci_find_root(hb->handle);
> > bridge = pci_root->bus->bridge;
> > - dport = devm_cxl_add_dport(root_port, bridge, uid, ctx.chbcr);
> > + if (ctx.chbs.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11)
> > + dport = devm_cxl_add_rch_dport(root_port, bridge, uid,
> > + component_reg_phys,
> > + ctx.chbs.base);
>
> Yes, this new function makes the rcrb handling much more simpler.
>
> > + else
> > + dport = devm_cxl_add_dport(root_port, bridge, uid,
> > + component_reg_phys);
> > if (IS_ERR(dport))
> > return PTR_ERR(dport);
> >
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index d225267c69bb..d9fe06e1462f 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -628,6 +628,8 @@ static struct cxl_port *cxl_port_alloc(struct device
> > *uport,
> > iter = to_cxl_port(iter->dev.parent);
> > if (iter->host_bridge)
> > port->host_bridge = iter->host_bridge;
> > + else if (parent_dport->rch)
> > + port->host_bridge = parent_dport->dport;
>
> Yes, looks good. This makes the endpoint a child of a CXL root port,
> not the ACPI0017 the root device.
>
> > else
> > port->host_bridge = iter->uport;
> > dev_dbg(uport, "host-bridge: %s\n",
> > dev_name(port->host_bridge));
> > @@ -899,10 +901,15 @@ static void cxl_dport_unlink(void *data)
> > sysfs_remove_link(&port->dev.kobj, link_name);
> > }
> >
> > -static struct cxl_dport *__devm_cxl_add_dport(struct cxl_port *port,
> > - struct device *dport_dev,
> > - int port_id,
> > - resource_size_t
> > component_reg_phys)
> > +enum cxl_dport_mode {
> > + CXL_DPORT_VH,
> > + CXL_DPORT_RCH,
> > +};
> > +
> > +static struct cxl_dport *
> > +__devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> > + int port_id, resource_size_t component_reg_phys,
> > + enum cxl_dport_mode mode, resource_size_t rcrb)
> > {
> > char link_name[CXL_TARGET_STRLEN];
> > struct cxl_dport *dport;
> > @@ -932,6 +939,9 @@ static struct cxl_dport *__devm_cxl_add_dport(struct
> > cxl_port *port,
> > dport->port_id = port_id;
> > dport->component_reg_phys = component_reg_phys;
> > dport->port = port;
> > + if (mode == CXL_DPORT_RCH)
> > + dport->rch = true;
>
> Alternatively an inline function could be added which checks
> dport->rcrb for a valid address.
I like it. Especially because a valid RCRB is needed to register the
dport in the first instance. I think it looks ok without an inline
function:
@@ -901,15 +901,10 @@ static void cxl_dport_unlink(void *data)
sysfs_remove_link(&port->dev.kobj, link_name);
}
-enum cxl_dport_mode {
- CXL_DPORT_VH,
- CXL_DPORT_RCH,
-};
-
static struct cxl_dport *
__devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
int port_id, resource_size_t component_reg_phys,
- enum cxl_dport_mode mode, resource_size_t rcrb)
+ resource_size_t rcrb)
{
char link_name[CXL_TARGET_STRLEN];
struct cxl_dport *dport;
@@ -939,7 +934,7 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device
*dport_dev,
dport->port_id = port_id;
dport->component_reg_phys = component_reg_phys;
dport->port = port;
- if (mode == CXL_DPORT_RCH)
+ if (rcrb != CXL_RESOURCE_NONE)
dport->rch = true;
dport->rcrb = rcrb;
>
> > + dport->rcrb = rcrb;
> >
> > cond_cxl_root_lock(port);
> > rc = add_dport(port, dport);
> > @@ -973,7 +983,8 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port
> > *port,
> > struct cxl_dport *dport;
> >
> > dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> > - component_reg_phys);
> > + component_reg_phys, CXL_DPORT_VH,
> > + CXL_RESOURCE_NONE);
> > if (IS_ERR(dport)) {
> > dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
> > dev_name(&port->dev), PTR_ERR(dport));
> > @@ -986,6 +997,27 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port
> > *port,
> > }
> > EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, CXL);
> >
> > +struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
> > + struct device *dport_dev, int port_id,
> > + resource_size_t component_reg_phys,
> > + resource_size_t rcrb)
>
> The documentation header is missing for that.
Added the following and clarified that @rcrb is mandatory:
@@ -966,7 +961,7 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device
*dport_dev,
}
/**
- * devm_cxl_add_dport - append downstream port data to a cxl_port
+ * devm_cxl_add_dport - append VH downstream port data to a cxl_port
* @port: the cxl_port that references this dport
* @dport_dev: firmware or PCI device representing the dport
* @port_id: identifier for this dport in a decoder's target list
@@ -997,6 +991,16 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
}
EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, CXL);
+/**
+ * devm_cxl_add_rch_dport - append RCH downstream port data to a cxl_port
+ * @port: the cxl_port that references this dport
+ * @dport_dev: firmware or PCI device representing the dport
+ * @port_id: identifier for this dport in a decoder's target list
+ * @component_reg_phys: optional location of CXL component registers
+ * @rcrb: mandatory location of a Root Complex Register Block
+ *
+ * See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
+ */
struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
struct device *dport_dev, int port_id,
resource_size_t component_reg_phys,
>
> > +{
> > + struct cxl_dport *dport;
> > +
> > + dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> > + component_reg_phys, CXL_DPORT_RCH, rcrb);
> > + if (IS_ERR(dport)) {
> > + dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
> > + dev_name(&port->dev), PTR_ERR(dport));
> > + } else {
> > + dev_dbg(dport_dev, "RCH dport added to %s\n",
> > + dev_name(&port->dev));
> > + }
> > +
> > + return dport;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(devm_cxl_add_rch_dport, CXL);
> > +
> > static int add_ep(struct cxl_ep *new)
> > {
> > struct cxl_port *port = new->dport->port;
> > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> > index ec178e69b18f..7c2a85dc4125 100644
> > --- a/drivers/cxl/core/regs.c
> > +++ b/drivers/cxl/core/regs.c
> > @@ -307,3 +307,59 @@ int cxl_find_regblock(struct pci_dev *pdev, enum
> > cxl_regloc_type type,
> > return -ENODEV;
> > }
> > EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
> > +
> > +resource_size_t cxl_rcrb_to_component(struct device *dev,
> > + resource_size_t rcrb,
> > + enum cxl_rcrb which)
> > +{
> > + resource_size_t component_reg_phys;
> > + u32 bar0, bar1;
> > + void *addr;
> > + u16 cmd;
> > +
> > + if (which == CXL_RCRB_UPSTREAM)
> > + rcrb += SZ_4K;
> > +
> > + /*
> > + * RCRB's BAR[0..1] point to component block containing CXL
> > + * subsystem component registers. MEMBAR extraction follows
> > + * the PCI Base spec here, esp. 64 bit extraction and memory
> > + * ranges alignment (6.0, 7.5.1.2.1).
> > + */
> > + if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))
> > + return CXL_RESOURCE_NONE;
> > + addr = ioremap(rcrb, SZ_4K);
> > + if (!addr) {
> > + dev_err(dev, "Failed to map region %pr\n", addr);
> > + release_mem_region(rcrb, SZ_4K);
> > + return CXL_RESOURCE_NONE;
> > + }
> > +
> > + cmd = readw(addr + PCI_COMMAND);
> > + bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> > + bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> > + iounmap(addr);
> > + release_mem_region(rcrb, SZ_4K);
> > +
> > + /* sanity check */
> > + if (cmd == 0xffff)
> > + return CXL_RESOURCE_NONE;
>
> The spec says offset 0 should be checked (32 bit) which is always
> non-FF if implemented. This requires another read.
>
> cmd is most of the cases also non-zero, so probably checking cmd
> instead will have the same effect. Still worth changing that.
>
> If the downstream port's rcrb is all FFs, it is a FW bug. Could be
> worth a message.
Ok, makes sense, added:
@@ -335,15 +336,22 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
return CXL_RESOURCE_NONE;
}
+ id = readl(addr + PCI_VENDOR_ID);
cmd = readw(addr + PCI_COMMAND);
bar0 = readl(addr + PCI_BASE_ADDRESS_0);
bar1 = readl(addr + PCI_BASE_ADDRESS_1);
iounmap(addr);
release_mem_region(rcrb, SZ_4K);
- /* sanity check */
- if (cmd == 0xffff)
+ /*
+ * Sanity check, see CXL 3.0 Figure 9-8 CXL Device that Does Not
+ * Remap Upstream Port and Component Registers
+ */
+ if (id == (u32) -1) {
+ if (which == CXL_RCRB_DOWNSTREAM)
+ dev_err(dev, "Failed to access Downstream Port RCRB\n");
return CXL_RESOURCE_NONE;
+ }
if ((cmd & PCI_COMMAND_MEMORY) == 0)
return CXL_RESOURCE_NONE;
if (bar0 & (PCI_BASE_ADDRESS_MEM_TYPE_1M | PCI_BASE_ADDRESS_SPACE_IO))
