On 28.11.22 13:58:55, Dan Williams wrote:
> 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.
Yes, should work as is.
>
> >
> > > +
> > > 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.
Yes, will try to send tomorrow. Need to rework, still doing some
testing.
>
> >
> > > 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;
Change looks good.
I was even thinking to even use ->rcrb to check for rch:
static inline bool is_rch(struct cxl_dport *dport)
{
return (dport->rcrb != CXL_RESOURCE_NONE);
}
But "if (dport->rch) ..." looks nice too, esp. with its counterpart
cxlds->rcd.
>
>
> >
> > > + 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
> + */
Looks good. The component_reg_phys arg could be used for Register
Locator DVSEC in case of a VH device layout (9.11.8).
> 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) {
U32_MAX? Or, cheating there: ((u32)~0U).
> + 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))
>
