On Mon, Jan 31, 2022 at 4:24 PM Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote:
>
> On 22-01-23 16:30:35, Dan Williams wrote:
> > Unlike the decoder enumeration for "root decoders" described by platform
> > firmware, standard coders can be enumerated from the component registers
> ^ decoders
>
> > space once the base address has been identified (via PCI, ACPI, or
> > another mechanism).
> >
> > Add common infrastructure for HDM (Host-managed-Device-Memory) Decoder
> > enumeration and share it between host-bridge, upstream switch port, and
> > cxl_test defined decoders.
> >
> > The locking model for switch level decoders is to hold the port lock
> > over the enumeration. This facilitates moving the dport and decoder
> > enumeration to a 'port' driver. For now, the only enumerator of decoder
> > resources is the cxl_acpi root driver.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>
> I authored some parts of this patch, not sure how much percentage-wise. If it
> was intentional to drop me, that's fine - just checking.
It was a patch that was not original to the first series, but yeah I
copied some bits out of that series. I'll add you as Co-developed-by
on the resend.
>
> Some comments below.
>
> Reviewed-by: Ben Widawsky <ben.widawsky@xxxxxxxxx>
>
> > ---
> > drivers/cxl/acpi.c | 43 ++-----
> > drivers/cxl/core/Makefile | 1
> > drivers/cxl/core/core.h | 2
> > drivers/cxl/core/hdm.c | 247 +++++++++++++++++++++++++++++++++++++++++
> > drivers/cxl/core/port.c | 65 ++++++++---
> > drivers/cxl/core/regs.c | 5 -
> > drivers/cxl/cxl.h | 33 ++++-
> > drivers/cxl/cxlmem.h | 8 +
> > tools/testing/cxl/Kbuild | 4 +
> > tools/testing/cxl/test/cxl.c | 29 +++++
> > tools/testing/cxl/test/mock.c | 50 ++++++++
> > tools/testing/cxl/test/mock.h | 3
> > 12 files changed, 436 insertions(+), 54 deletions(-)
> > create mode 100644 drivers/cxl/core/hdm.c
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 259441245687..8c2ced91518b 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -168,10 +168,10 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> > struct device *host = root_port->dev.parent;
> > struct acpi_device *bridge = to_cxl_host_bridge(host, match);
> > struct acpi_pci_root *pci_root;
> > - int single_port_map[1], rc;
> > - struct cxl_decoder *cxld;
> > struct cxl_dport *dport;
> > + struct cxl_hdm *cxlhdm;
> > struct cxl_port *port;
> > + int rc;
> >
> > if (!bridge)
> > return 0;
> > @@ -200,37 +200,24 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> > rc = devm_cxl_port_enumerate_dports(host, port);
> > if (rc < 0)
> > return rc;
> > - if (rc > 1)
> > - return 0;
> > -
> > - /* TODO: Scan CHBCR for HDM Decoder resources */
> > -
> > - /*
> > - * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability
> > - * Structure) single ported host-bridges need not publish a decoder
> > - * capability when a passthrough decode can be assumed, i.e. all
> > - * transactions that the uport sees are claimed and passed to the single
> > - * dport. Disable the range until the first CXL region is enumerated /
> > - * activated.
> > - */
> > - cxld = cxl_switch_decoder_alloc(port, 1);
> > - if (IS_ERR(cxld))
> > - return PTR_ERR(cxl);
> > -
> > cxl_device_lock(&port->dev);
> > - dport = list_first_entry(&port->dports, typeof(*dport), list);
> > - cxl_device_unlock(&port->dev);
> > + if (rc == 1) {
> > + rc = devm_cxl_add_passthrough_decoder(host, port);
> > + goto out;
> > + }
> >
> > - single_port_map[0] = dport->port_id;
> > + cxlhdm = devm_cxl_setup_hdm(host, port);
> > + if (IS_ERR(cxlhdm)) {
> > + rc = PTR_ERR(cxlhdm);
> > + goto out;
> > + }
> >
> > - rc = cxl_decoder_add(cxld, single_port_map);
> > + rc = devm_cxl_enumerate_decoders(host, cxlhdm);
> > if (rc)
> > - put_device(&cxld->dev);
> > - else
> > - rc = cxl_decoder_autoremove(host, cxld);
> > + dev_err(&port->dev, "Couldn't enumerate decoders (%d)\n", rc);
> >
> > - if (rc == 0)
> > - dev_dbg(host, "add: %s\n", dev_name(&cxld->dev));
> > +out:
> > + cxl_device_unlock(&port->dev);
> > return rc;
> > }
> >
> > diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> > index 91057f0ec763..6d37cd78b151 100644
> > --- a/drivers/cxl/core/Makefile
> > +++ b/drivers/cxl/core/Makefile
> > @@ -8,3 +8,4 @@ cxl_core-y += regs.o
> > cxl_core-y += memdev.o
> > cxl_core-y += mbox.o
> > cxl_core-y += pci.o
> > +cxl_core-y += hdm.o
> > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> > index e0c9aacc4e9c..1a50c0fc399c 100644
> > --- a/drivers/cxl/core/core.h
> > +++ b/drivers/cxl/core/core.h
> > @@ -14,6 +14,8 @@ struct cxl_mem_query_commands;
> > int cxl_query_cmd(struct cxl_memdev *cxlmd,
> > struct cxl_mem_query_commands __user *q);
> > int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s);
> > +void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> > + resource_size_t length);
> >
> > int cxl_memdev_init(void);
> > void cxl_memdev_exit(void);
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > new file mode 100644
> > index 000000000000..802048dc2046
> > --- /dev/null
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -0,0 +1,247 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> > +#include <linux/io-64-nonatomic-hi-lo.h>
> > +#include <linux/device.h>
> > +#include <linux/delay.h>
> > +
> > +#include "cxlmem.h"
> > +#include "core.h"
> > +
> > +/**
> > + * DOC: cxl core hdm
> > + *
> > + * Compute Express Link Host Managed Device Memory, starting with the
> > + * CXL 2.0 specification, is managed by an array of HDM Decoder register
> > + * instances per CXL port and per CXL endpoint. Define common helpers
> > + * for enumerating these registers and capabilities.
> > + */
> > +
> > +static int add_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> > + int *target_map)
> > +{
> > + int rc;
> > +
> > + rc = cxl_decoder_add_locked(cxld, target_map);
> > + if (rc) {
> > + put_device(&cxld->dev);
> > + dev_err(&port->dev, "Failed to add decoder\n");
> > + return rc;
> > + }
> > +
> > + rc = cxl_decoder_autoremove(&port->dev, cxld);
> > + if (rc)
> > + return rc;
> > +
> > + dev_dbg(&cxld->dev, "Added to port %s\n", dev_name(&port->dev));
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability Structure)
> > + * single ported host-bridges need not publish a decoder capability when a
> > + * passthrough decode can be assumed, i.e. all transactions that the uport sees
> > + * are claimed and passed to the single dport. Disable the range until the first
> > + * CXL region is enumerated / activated.
> > + */
> > +int devm_cxl_add_passthrough_decoder(struct device *host, struct cxl_port *port)
> > +{
> > + struct cxl_decoder *cxld;
> > + struct cxl_dport *dport;
> > + int single_port_map[1];
> > +
> > + cxld = cxl_switch_decoder_alloc(port, 1);
> > + if (IS_ERR(cxld))
> > + return PTR_ERR(cxld);
> > +
> > + device_lock_assert(&port->dev);
> > +
> > + dport = list_first_entry(&port->dports, typeof(*dport), list);
> > + single_port_map[0] = dport->port_id;
> > +
> > + return add_hdm_decoder(port, cxld, single_port_map);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, CXL);
>
> Hmm, this makes me realize I need to modify the region driver to not care about
> finding decoder resources for a passthrough decoder.
Why would a passthrough decoder not have passthrough resources?
> > +
> > +static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
> > +{
> > + u32 hdm_cap;
> > +
> > + hdm_cap = readl(cxlhdm->regs.hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET);
> > + cxlhdm->decoder_count = cxl_hdm_decoder_count(hdm_cap);
> > + cxlhdm->target_count =
> > + FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap);
> > + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap))
> > + cxlhdm->interleave_mask |= GENMASK(11, 8);
> > + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap))
> > + cxlhdm->interleave_mask |= GENMASK(14, 12);
> > +}
> > +
> > +static void __iomem *map_hdm_decoder_regs(struct cxl_port *port,
> > + void __iomem *crb)
> > +{
> > + struct cxl_register_map map;
> > + struct cxl_component_reg_map *comp_map = &map.component_map;
> > +
> > + cxl_probe_component_regs(&port->dev, crb, comp_map);
> > + if (!comp_map->hdm_decoder.valid) {
> > + dev_err(&port->dev, "HDM decoder registers invalid\n");
> > + return IOMEM_ERR_PTR(-ENXIO);
> > + }
> > +
> > + return crb + comp_map->hdm_decoder.offset;
> > +}
> > +
> > +/**
> > + * devm_cxl_setup_hdm - map HDM decoder component registers
> > + * @port: cxl_port to map
> > + */
>
> This got messed up on the fixup. You need @host and @port at this point. It'd be
> pretty cool if we could skip straight to not @host arg.
I'll fixup the inter-patch dpc breakage again, I think I may have
edited a local copy of this file as part of the rebase, and botched
the resend.
I otherwise could not see a way to skip the temporary state without
shipping devm abuse in the middle of series (leaking object
allocations until release)
>
> > +struct cxl_hdm *devm_cxl_setup_hdm(struct device *host, struct cxl_port *port)
> > +{
> > + void __iomem *crb, __iomem *hdm;
> > + struct device *dev = &port->dev;
> > + struct cxl_hdm *cxlhdm;
> > +
> > + cxlhdm = devm_kzalloc(host, sizeof(*cxlhdm), GFP_KERNEL);
> > + if (!cxlhdm)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + cxlhdm->port = port;
> > + crb = devm_cxl_iomap_block(host, port->component_reg_phys,
> > + CXL_COMPONENT_REG_BLOCK_SIZE);
> > + if (!crb) {
> > + dev_err(dev, "No component registers mapped\n");
> > + return ERR_PTR(-ENXIO);
> > + }
>
> Does this work if the port is operating in passthrough decoder mode? Is the idea
> to just not call this thing if so?
Per the spec there are always component registers in a CXL port, there
just may not be an HDM Decoder Capability structure in that set of
component registers. See 8.2.5.12.
