Changes since v2 [1]:
- Add some rationale for moving definitions out of mem.c into mem.h
(Jonathan)
- Fixup CXL_DEVICE_REGS kernel doc and declare the fixup for the
struct cxl_mem kernel-doc in the changelog (Jonathan)
- Fixup cxl_setup_device_regs() kernel-doc (Jonathan)
- Cleanup comment in cxl_root_alloc() (Jonathan)
- [not changed] refactor device creation into alloc_and_add()
anti-pattern.
- Add kernel doc to cxl_address_space_dev_add() to clarify why @host is
needed as an arg. (Jonathan)
- Describe what the port devices are representing in "cxl/port:
Introduce cxl_port objects" (Jonathan)
- Explain the rationale for /sys/bus/cxl (Bjorn)
[1]: http://lore.kernel.org/r/161728744224.2474040.12854720917440712854.stgit@dwillia2-desk3.amr.corp.intel.com
---
Plans for v5.14:
This series is a starting point for the persistent memory and dynamic
HDM decoder manipulation support targeting the v5.14 kernel. Among the
tasks to complete in that timeframe are:
- Region creation including CXL label support
- LIBNVDIMM integration for surfacing /dev/pmemX and /dev/daxX.Y devices
on CXL resources
- HDM decoder enumeration and programming for setting up PMEM mappings
alongside any "System RAM" mappings established by platform firmware
- CDAT-DOE support in support of dynamically created NUMA nodes
- CXL PMEM Shutdown semantics (global persistence flush coordination)
Contributions to cxl.git targeting the next merge window require a
non-author Reviewed-by. A patch with a Reviewed-by, no outstanding
objections, and a 3-5 day quiet period on the list is subject to be
applied to a non-rebasing cxl.git branch and merged into cxl.git/next.
Contributions targeting a current -rc (fixes) may go in on an expedited
schedule with only an Acked-by.
---
CXL Port Topology:
The enumeration starts with the ACPI0017 driver registering a 'struct
cxl_root' object to establish the top of a cxl_port topology. It then
scans the ACPI bus looking for ACPI0016 instances. The cxl_root object
is a singleton* anchor to hang "address-space" objects and be a parent
device for the downstream 'struct cxl_port' instances. An address-space
has a 1:1 relationship with a platform defined memory resource range,
like _CRS for PCIE Host Bridges. Use module parameters to model a
root-level HDM decoder that all downstream ports further decode, to be
replaced with a Code First ECN to do the same.
Each address space is modeled as a sysfs object that also shows up in
/proc/iomem as "CXL Address Space". That iomem resource is functionally
equivalent to the root-level 'PCI Bus' resources for PCIE.mmio while
'CXL Address Space' indicates space for CXL.mem to be mapped. "System
RAM" and "Persistent Memory", when mapped by HDM decoders, will appear
as child CXL.mem resources.
Once a 'struct cxl_root' is established the host bridge is modeled as 1
upstream 'struct cxl_port' and N downstream 'struct cxl_port' instances
(one per Root Port), just like a PCIE switch. The host-bridge upstream
port optionally has the HDM decoder registers from the CHBCR if the
host-bridge has multiple PCIE/CXL root ports. Single-ported host bridges
will not have HDM decoders in the CHBCR space (see CHBCR note in
8.2.5.12 CXL HDM Decoder Capability Structure), but the 'struct
cxl_port' object is still needed to represent other CXL capabilities and
access port-specific component registers outside of HDM decoders.
Each 'struct cxl_port' has a 'target_id' attribute that answers the
question "what port am I in my upstream port's HDM decoder target
list?". For the host-bridge struct cxl_port, the first tier of ports
below cxl_root.port, the id is derived from the ordinal mapping of the
ACPI0016 id (instance id, _UID, or other handle TBD), for all other
ports the id is the PCIE Root Port ID from the Link Capabilities
register [1]. The mapping of ordinal port identifiers relative to their
parent may change once libcxl and cxl-cli prove out region creation, or
a better option is found to establish a static device path / persistent
naming scheme. System software must not assume that 'struct cxl_port'
device names will be static from one boot to the next.
See patch7 for a tree(1) topology picture of what QEMU is producing
today with this enabling.
* cxl_root is singleton only by convention. A given cxl_root could
represent 1 to N address spaces, this patch set chooses to implement 1
cxl_root for all address spaces.
[1]: CXL 2.0 8.2.5.12.8 CXL HDM Decoder 0 Target List Low Register
(Offset 24h) ...The Target Port Identifier for a given Downstream Port
is reported via Port Number field in Link Capabilities Register. (See
PCI Express Base Specification).
---
Dan Williams (8):
cxl/mem: Move some definitions to mem.h
cxl/mem: Introduce 'struct cxl_regs' for "composable" CXL devices
cxl/core: Rename bus.c to core.c
cxl/core: Refactor CXL register lookup for bridge reuse
cxl/acpi: Introduce ACPI0017 driver and cxl_root
cxl/Kconfig: Default drivers to CONFIG_CXL_BUS
cxl/port: Introduce cxl_port objects
cxl/acpi: Add module parameters to stand in for ACPI tables
Documentation/driver-api/cxl/memory-devices.rst | 6
drivers/cxl/Kconfig | 16 +
drivers/cxl/Makefile | 6
drivers/cxl/acpi.c | 215 +++++++++
drivers/cxl/bus.c | 29 -
drivers/cxl/core.c | 561 +++++++++++++++++++++++
drivers/cxl/cxl.h | 148 ++++--
drivers/cxl/mem.c | 97 +---
drivers/cxl/mem.h | 82 +++
9 files changed, 999 insertions(+), 161 deletions(-)
create mode 100644 drivers/cxl/acpi.c
delete mode 100644 drivers/cxl/bus.c
create mode 100644 drivers/cxl/core.c
create mode 100644 drivers/cxl/mem.h
base-commit: a38fd8748464831584a19438cbb3082b5a2dab15
CONFIG_CXL_BUS is default 'n' as expected for new functionality. When
that is enabled do not make the end user hunt for all the expected
sub-options to enable. For example CONFIG_CXL_BUS without CONFIG_CXL_MEM
is an odd/expert configuration, so is CONFIG_CXL_MEM without
CONFIG_CXL_ACPI (on ACPI capable platforms). Default CONFIG_CXL_MEM and
CONFIG_CXL_ACPI to CONFIG_CXL_BUS.
Acked-by: Ben Widawsky <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
drivers/cxl/Kconfig | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
index fb282af84afd..1da7970a5e55 100644
--- a/drivers/cxl/Kconfig
+++ b/drivers/cxl/Kconfig
@@ -15,6 +15,7 @@ if CXL_BUS
config CXL_MEM
tristate "CXL.mem: Memory Devices"
+ default CXL_BUS
help
The CXL.mem protocol allows a device to act as a provider of
"System RAM" and/or "Persistent Memory" that is fully coherent
@@ -54,6 +55,7 @@ config CXL_MEM_RAW_COMMANDS
config CXL_ACPI
tristate "CXL ACPI: Platform Support"
depends on ACPI
+ default CXL_BUS
help
Enable support for host managed device memory (HDM) resources
published by a platform's ACPI CXL memory layout description.
Once the cxl_root is established then other ports in the hierarchy can
be attached. The cxl_port object, unlike cxl_root that is associated
with host bridges, is associated with PCIe Root Ports or PCIe Switch
Ports. Add cxl_port instances for all PCIe Root Ports in an ACPI0016
host bridge. The cxl_port instances for PCIe Switch Ports are not
included here as those are to be modeled as another service device
registered on the pcie_port_bus_type.
A sample sysfs topology for a single-host-bridge with
single-PCIe/CXL-port follows:
/sys/bus/cxl/devices/root0
├── address_space0
│ ├── devtype
│ ├── end
│ ├── start
│ ├── supports_ram
│ ├── supports_type2
│ ├── supports_type3
│ └── uevent
├── address_space1
│ ├── devtype
│ ├── end
│ ├── start
│ ├── supports_pmem
│ ├── supports_type2
│ ├── supports_type3
│ └── uevent
├── devtype
├── port1
│ ├── devtype
│ ├── host -> ../../../../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
│ ├── port2
│ │ ├── devtype
│ │ ├── host -> ../../../../../pci0000:34/0000:34:00.0
│ │ ├── subsystem -> ../../../../../../bus/cxl
│ │ ├── target_id
│ │ └── uevent
│ ├── subsystem -> ../../../../../bus/cxl
│ ├── target_id
│ └── uevent
├── subsystem -> ../../../../bus/cxl
├── target_id
└── uevent
In this listing the system-wide-singleton root0 has 2 address spaces, 1
PMEM and 1 RAM. Those address spaces are accessed through port1 which
represents the upstream port of an ACPI0016 host-bridge. A
multi-host-bridge system would have other ports as peers to port1 to
additionally decode root level address spaces. Port2 in this diagram
represents the single downstream port of the host-bridge. Were it to be
a multi-ported-host-bridge there would be peers / siblings of port2 with
port1 as their common ancestor.
The rationale for this port hierarchy is to be able to walk the HDM
decoder register sets that each port implements. Additionally it
provides a representation of host-bridge interleave which will be
necessary for follow-on work that adds CXL region devices.
The details in the /sys/bus/cxl hierarchy that are not suitable to be
represented in the /sys/bus/pci hierarchy are:
- memory address spaces that are interleaved across host bridges
- common sub-device functionality represented by CXL component + device
registers (enumerated via DVSEC or platform firmware (ACPI CEDT)).
Signed-off-by: Dan Williams <[email protected]>
---
drivers/cxl/acpi.c | 99 +++++++++++++++++++++++++++++++++++++++++++
drivers/cxl/core.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++
drivers/cxl/cxl.h | 5 ++
3 files changed, 224 insertions(+), 1 deletion(-)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index d54c2d5de730..bc2a35ae880b 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -5,18 +5,117 @@
#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/acpi.h>
+#include <linux/pci.h>
#include "cxl.h"
+static int match_ACPI0016(struct device *dev, const void *host)
+{
+ struct acpi_device *adev = to_acpi_device(dev);
+ const char *hid = acpi_device_hid(adev);
+
+ return strcmp(hid, "ACPI0016") == 0;
+}
+
+struct cxl_walk_context {
+ struct device *dev;
+ struct pci_bus *root;
+ struct cxl_port *port;
+ int error;
+ int count;
+};
+
+static int match_add_root_ports(struct pci_dev *pdev, void *data)
+{
+ struct cxl_walk_context *ctx = data;
+ struct pci_bus *root_bus = ctx->root;
+ struct cxl_port *port = ctx->port;
+ int type = pci_pcie_type(pdev);
+ struct device *dev = ctx->dev;
+ resource_size_t cxl_regs_phys;
+ int target_id = ctx->count;
+
+ if (pdev->bus != root_bus)
+ return 0;
+ if (!pci_is_pcie(pdev))
+ return 0;
+ if (type != PCI_EXP_TYPE_ROOT_PORT)
+ return 0;
+
+ ctx->count++;
+
+ /* TODO walk DVSEC to find component register base */
+ cxl_regs_phys = -1;
+
+ port = devm_cxl_add_port(dev, port, &pdev->dev, target_id,
+ cxl_regs_phys);
+ if (IS_ERR(port)) {
+ ctx->error = PTR_ERR(port);
+ return ctx->error;
+ }
+
+ dev_dbg(dev, "%s: register: %s\n", dev_name(&pdev->dev),
+ dev_name(&port->dev));
+
+ return 0;
+}
+
+/*
+ * A host bridge may contain one or more root ports. Register each port
+ * as a child of the cxl_root.
+ */
+static int cxl_acpi_register_ports(struct device *dev, struct acpi_device *root,
+ struct cxl_port *port, int idx)
+{
+ struct acpi_pci_root *pci_root = acpi_pci_find_root(root->handle);
+ struct cxl_walk_context ctx;
+
+ if (!pci_root)
+ return -ENXIO;
+
+ /* TODO: fold in CEDT.CHBS retrieval */
+ port = devm_cxl_add_port(dev, port, &root->dev, idx, ~0ULL);
+ if (IS_ERR(port))
+ return PTR_ERR(port);
+ dev_dbg(dev, "%s: register: %s\n", dev_name(&root->dev),
+ dev_name(&port->dev));
+
+ ctx = (struct cxl_walk_context) {
+ .dev = dev,
+ .root = pci_root->bus,
+ .port = port,
+ };
+ pci_walk_bus(pci_root->bus, match_add_root_ports, &ctx);
+
+ if (ctx.count == 0)
+ return -ENODEV;
+ return ctx.error;
+}
+
static int cxl_acpi_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
+ struct acpi_device *adev = ACPI_COMPANION(dev);
+ struct device *bridge = NULL;
struct cxl_root *cxl_root;
+ int rc, i = 0;
cxl_root = devm_cxl_add_root(dev, NULL, 0);
if (IS_ERR(cxl_root))
return PTR_ERR(cxl_root);
dev_dbg(dev, "register: %s\n", dev_name(&cxl_root->port.dev));
+ while (true) {
+ bridge = bus_find_device(adev->dev.bus, bridge, dev,
+ match_ACPI0016);
+ if (!bridge)
+ break;
+
+ rc = cxl_acpi_register_ports(dev, to_acpi_device(bridge),
+ &cxl_root->port, i++);
+ if (rc)
+ return rc;
+ }
+
return 0;
}
diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index 347824a62a66..cc9af9033292 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -148,6 +148,15 @@ static void cxl_root_release(struct device *dev)
kfree(cxl_root);
}
+static void cxl_port_release(struct device *dev)
+{
+ struct cxl_port *port = to_cxl_port(dev);
+
+ ida_free(&cxl_port_ida, port->id);
+ put_device(port->port_host);
+ kfree(port);
+}
+
static ssize_t target_id_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
@@ -178,6 +187,12 @@ static const struct device_type cxl_root_type = {
.groups = cxl_port_attribute_groups,
};
+static const struct device_type cxl_port_type = {
+ .name = "cxl_port",
+ .release = cxl_port_release,
+ .groups = cxl_port_attribute_groups,
+};
+
struct cxl_root *to_cxl_root(struct device *dev)
{
if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
@@ -188,7 +203,9 @@ struct cxl_root *to_cxl_root(struct device *dev)
struct cxl_port *to_cxl_port(struct device *dev)
{
- if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
+ if (dev_WARN_ONCE(dev,
+ dev->type != &cxl_root_type &&
+ dev->type != &cxl_port_type,
"not a cxl_port device\n"))
return NULL;
return container_of(dev, struct cxl_port, dev);
@@ -367,6 +384,108 @@ struct cxl_root *devm_cxl_add_root(struct device *host,
}
EXPORT_SYMBOL_GPL(devm_cxl_add_root);
+static void cxl_unlink_port(void *_port)
+{
+ struct cxl_port *port = _port;
+
+ sysfs_remove_link(&port->dev.kobj, "host");
+}
+
+static int devm_cxl_link_port(struct device *dev, struct cxl_port *port)
+{
+ int rc;
+
+ rc = sysfs_create_link(&port->dev.kobj, &port->port_host->kobj, "host");
+ if (rc)
+ return rc;
+ return devm_add_action_or_reset(dev, cxl_unlink_port, port);
+}
+
+static struct cxl_port *cxl_port_alloc(struct cxl_port *parent_port,
+ struct device *port_dev, int target_id,
+ resource_size_t component_regs_phys)
+{
+ struct cxl_port *port;
+ struct device *dev;
+ int rc;
+
+ if (!port_dev)
+ return ERR_PTR(-EINVAL);
+
+ port = kzalloc(sizeof(*port), GFP_KERNEL);
+ if (!port)
+ return ERR_PTR(-ENOMEM);
+
+ rc = ida_alloc(&cxl_port_ida, GFP_KERNEL);
+ if (rc < 0)
+ goto err;
+
+ port->id = rc;
+ port->target_id = target_id;
+ port->port_host = get_device(port_dev);
+ port->component_regs_phys = component_regs_phys;
+
+ dev = &port->dev;
+ device_initialize(dev);
+ device_set_pm_not_required(dev);
+ dev->parent = &parent_port->dev;
+ dev->bus = &cxl_bus_type;
+ dev->type = &cxl_port_type;
+
+ return port;
+
+err:
+ kfree(port);
+ return ERR_PTR(rc);
+}
+
+/**
+ * devm_cxl_add_port() - add a cxl_port to the topology
+ * @host: devm context / discovery agent
+ * @parent_port: immediate ancestor towards cxl_root
+ * @port_host: PCI or platform-firmware device hosting this port
+ * @target_id: ordinal id relative to other siblings under @parent_port
+ * @component_regs_phys: CXL component register base address
+ */
+struct cxl_port *devm_cxl_add_port(struct device *host,
+ struct cxl_port *parent_port,
+ struct device *port_host, int target_id,
+ resource_size_t component_regs_phys)
+{
+ struct cxl_port *port;
+ struct device *dev;
+ int rc;
+
+ port = cxl_port_alloc(parent_port, port_host, target_id,
+ component_regs_phys);
+ if (IS_ERR(port))
+ return port;
+
+ dev = &port->dev;
+ rc = dev_set_name(dev, "port%d", port->id);
+ if (rc)
+ goto err;
+
+ rc = device_add(dev);
+ if (rc)
+ goto err;
+
+ rc = devm_add_action_or_reset(host, unregister_dev, dev);
+ if (rc)
+ return ERR_PTR(rc);
+
+ rc = devm_cxl_link_port(host, port);
+ if (rc)
+ return ERR_PTR(rc);
+
+ return port;
+
+err:
+ put_device(dev);
+ return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_GPL(devm_cxl_add_port);
+
/**
* cxl_setup_device_regs() - Detect CXL Device register blocks
* @dev: Host device of the @base mapping
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 5cd1173151e5..71a991bdacb7 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -134,5 +134,10 @@ struct cxl_address_space_dev *to_cxl_address_space(struct device *dev);
struct cxl_root *devm_cxl_add_root(struct device *parent,
struct cxl_address_space *cxl_space,
int nr_spaces);
+struct cxl_port *devm_cxl_add_port(struct device *host,
+ struct cxl_port *parent_port,
+ struct device *port_host, int target_id,
+ resource_size_t component_regs_phys);
+
extern struct bus_type cxl_bus_type;
#endif /* __CXL_H__ */
While CXL Memory Device endpoints locate the CXL MMIO registers in a PCI
BAR, CXL root bridges have their MMIO base address described by platform
firmware. Refactor the existing register lookup into a generic facility
for endpoints and bridges to share.
Reviewed-by: Ben Widawsky <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
drivers/cxl/core.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
drivers/cxl/cxl.h | 3 +++
drivers/cxl/mem.c | 50 ++++++----------------------------------------
3 files changed, 65 insertions(+), 44 deletions(-)
diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index 7f8d2034038a..d94c4c2dee8f 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -2,6 +2,7 @@
/* Copyright(c) 2020 Intel Corporation. All rights reserved. */
#include <linux/device.h>
#include <linux/module.h>
+#include "cxl.h"
/**
* DOC: cxl core
@@ -10,6 +11,61 @@
* point for cross-device interleave coordination through cxl ports.
*/
+/**
+ * cxl_setup_device_regs() - Detect CXL Device register blocks
+ * @dev: Host device of the @base mapping
+ * @base: Mapping of CXL 2.0 8.2.8 CXL Device Register Interface
+ * @regs: Base pointers for device register blocks (see CXL_DEVICE_REGS())
+ */
+void cxl_setup_device_regs(struct device *dev, void __iomem *base,
+ struct cxl_device_regs *regs)
+{
+ int cap, cap_count;
+ u64 cap_array;
+
+ *regs = (struct cxl_device_regs) { 0 };
+
+ cap_array = readq(base + CXLDEV_CAP_ARRAY_OFFSET);
+ if (FIELD_GET(CXLDEV_CAP_ARRAY_ID_MASK, cap_array) !=
+ CXLDEV_CAP_ARRAY_CAP_ID)
+ return;
+
+ cap_count = FIELD_GET(CXLDEV_CAP_ARRAY_COUNT_MASK, cap_array);
+
+ for (cap = 1; cap <= cap_count; cap++) {
+ void __iomem *register_block;
+ u32 offset;
+ u16 cap_id;
+
+ cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK,
+ readl(base + cap * 0x10));
+ offset = readl(base + cap * 0x10 + 0x4);
+ register_block = base + offset;
+
+ switch (cap_id) {
+ case CXLDEV_CAP_CAP_ID_DEVICE_STATUS:
+ dev_dbg(dev, "found Status capability (0x%x)\n", offset);
+ regs->status = register_block;
+ break;
+ case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX:
+ dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset);
+ regs->mbox = register_block;
+ break;
+ case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX:
+ dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset);
+ break;
+ case CXLDEV_CAP_CAP_ID_MEMDEV:
+ dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset);
+ regs->memdev = register_block;
+ break;
+ default:
+ dev_dbg(dev, "Unknown cap ID: %d (0x%x)\n", cap_id, offset);
+ break;
+ }
+ }
+}
+EXPORT_SYMBOL_GPL(cxl_setup_device_regs);
+
struct bus_type cxl_bus_type = {
.name = "cxl",
};
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 1f3434f89ef2..d49e0cb679fa 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -66,5 +66,8 @@ struct cxl_regs {
};
};
+void cxl_setup_device_regs(struct device *dev, void __iomem *base,
+ struct cxl_device_regs *regs);
+
extern struct bus_type cxl_bus_type;
#endif /* __CXL_H__ */
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 6951243d128e..ee55abfa147e 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -865,53 +865,15 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, u16 opcode,
static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
{
struct device *dev = &cxlm->pdev->dev;
- int cap, cap_count;
- u64 cap_array;
+ struct cxl_regs *regs = &cxlm->regs;
- cap_array = readq(cxlm->base + CXLDEV_CAP_ARRAY_OFFSET);
- if (FIELD_GET(CXLDEV_CAP_ARRAY_ID_MASK, cap_array) !=
- CXLDEV_CAP_ARRAY_CAP_ID)
- return -ENODEV;
-
- cap_count = FIELD_GET(CXLDEV_CAP_ARRAY_COUNT_MASK, cap_array);
-
- for (cap = 1; cap <= cap_count; cap++) {
- void __iomem *register_block;
- u32 offset;
- u16 cap_id;
-
- cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK,
- readl(cxlm->base + cap * 0x10));
- offset = readl(cxlm->base + cap * 0x10 + 0x4);
- register_block = cxlm->base + offset;
-
- switch (cap_id) {
- case CXLDEV_CAP_CAP_ID_DEVICE_STATUS:
- dev_dbg(dev, "found Status capability (0x%x)\n", offset);
- cxlm->regs.status = register_block;
- break;
- case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX:
- dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset);
- cxlm->regs.mbox = register_block;
- break;
- case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX:
- dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset);
- break;
- case CXLDEV_CAP_CAP_ID_MEMDEV:
- dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset);
- cxlm->regs.memdev = register_block;
- break;
- default:
- dev_dbg(dev, "Unknown cap ID: %d (0x%x)\n", cap_id, offset);
- break;
- }
- }
+ cxl_setup_device_regs(dev, cxlm->base, ®s->device_regs);
- if (!cxlm->regs.status || !cxlm->regs.mbox || !cxlm->regs.memdev) {
+ if (!regs->status || !regs->mbox || !regs->memdev) {
dev_err(dev, "registers not found: %s%s%s\n",
- !cxlm->regs.status ? "status " : "",
- !cxlm->regs.mbox ? "mbox " : "",
- !cxlm->regs.memdev ? "memdev" : "");
+ !regs->status ? "status " : "",
+ !regs->mbox ? "mbox " : "",
+ !regs->memdev ? "memdev" : "");
return -ENXIO;
}
While CXL builds upon the PCI software model for dynamic enumeration and
control, a static platform component is required to bootstrap the CXL
memory layout. In addition to identifying the host bridges ACPI is
responsible for enumerating the CXL memory space that can be addressed
by decoders. This is similar to the requirement for ACPI to publish
resources reported by _CRS for PCI host bridges.
Introduce the cxl_root object as an abstract "port" into the CXL.mem
address space described by HDM decoders identified by the ACPI
CEDT.CHBS.
For now just establish the initial boilerplate and sysfs attributes, to
be followed by enumeration of the ports within the host bridge.
Note the allocation of CXL core device objects is split into separate
alloc and add steps in order to separate the alloc error path (kfree())
from the device add error path (put_device()).
Signed-off-by: Dan Williams <[email protected]>
---
drivers/cxl/Kconfig | 14 ++
drivers/cxl/Makefile | 2
drivers/cxl/acpi.c | 39 +++++
drivers/cxl/core.c | 356 ++++++++++++++++++++++++++++++++++++++++++++++++++
drivers/cxl/cxl.h | 65 +++++++++
5 files changed, 476 insertions(+)
create mode 100644 drivers/cxl/acpi.c
diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
index 97dc4d751651..fb282af84afd 100644
--- a/drivers/cxl/Kconfig
+++ b/drivers/cxl/Kconfig
@@ -50,4 +50,18 @@ config CXL_MEM_RAW_COMMANDS
potential impact to memory currently in use by the kernel.
If developing CXL hardware or the driver say Y, otherwise say N.
+
+config CXL_ACPI
+ tristate "CXL ACPI: Platform Support"
+ depends on ACPI
+ help
+ Enable support for host managed device memory (HDM) resources
+ published by a platform's ACPI CXL memory layout description.
+ See Chapter 9.14.1 CXL Early Discovery Table (CEDT) in the CXL
+ 2.0 specification. The CXL core consumes these resource to
+ publish port and address_space objects used to map regions
+ that represent System RAM, or Persistent Memory regions to be
+ managed by LIBNVDIMM.
+
+ If unsure say 'm'.
endif
diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index 3808e39dd31f..f429ca6b59d9 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -1,7 +1,9 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_CXL_BUS) += cxl_core.o
obj-$(CONFIG_CXL_MEM) += cxl_mem.o
+obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL
cxl_core-y := core.o
cxl_mem-y := mem.o
+cxl_acpi-y := acpi.o
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
new file mode 100644
index 000000000000..d54c2d5de730
--- /dev/null
+++ b/drivers/cxl/acpi.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/acpi.h>
+#include "cxl.h"
+
+static int cxl_acpi_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cxl_root *cxl_root;
+
+ cxl_root = devm_cxl_add_root(dev, NULL, 0);
+ if (IS_ERR(cxl_root))
+ return PTR_ERR(cxl_root);
+ dev_dbg(dev, "register: %s\n", dev_name(&cxl_root->port.dev));
+
+ return 0;
+}
+
+static const struct acpi_device_id cxl_acpi_ids[] = {
+ { "ACPI0017", 0 },
+ { "", 0 },
+};
+MODULE_DEVICE_TABLE(acpi, cxl_acpi_ids);
+
+static struct platform_driver cxl_acpi_driver = {
+ .probe = cxl_acpi_probe,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .acpi_match_table = cxl_acpi_ids,
+ },
+};
+
+module_platform_driver(cxl_acpi_driver);
+MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(CXL);
diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index d94c4c2dee8f..347824a62a66 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -2,6 +2,8 @@
/* Copyright(c) 2020 Intel Corporation. All rights reserved. */
#include <linux/device.h>
#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/idr.h>
#include "cxl.h"
/**
@@ -11,6 +13,360 @@
* point for cross-device interleave coordination through cxl ports.
*/
+static DEFINE_IDA(cxl_port_ida);
+
+static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ return sysfs_emit(buf, "%s\n", dev->type->name);
+}
+static DEVICE_ATTR_RO(devtype);
+
+static struct attribute *cxl_base_attributes[] = {
+ &dev_attr_devtype.attr,
+ NULL,
+};
+
+static struct attribute_group cxl_base_attribute_group = {
+ .attrs = cxl_base_attributes,
+};
+
+static struct cxl_address_space *dev_to_address_space(struct device *dev)
+{
+ struct cxl_address_space_dev *cxl_asd = to_cxl_address_space(dev);
+
+ return cxl_asd->address_space;
+}
+
+static ssize_t start_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct cxl_address_space *space = dev_to_address_space(dev);
+
+ return sysfs_emit(buf, "%#llx\n", space->range.start);
+}
+static DEVICE_ATTR_RO(start);
+
+static ssize_t end_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct cxl_address_space *space = dev_to_address_space(dev);
+
+ return sysfs_emit(buf, "%#llx\n", space->range.end);
+}
+static DEVICE_ATTR_RO(end);
+
+#define CXL_ATTR_SUPPORTS(name, flag) \
+static ssize_t supports_##name##_show( \
+ struct device *dev, struct device_attribute *attr, char *buf) \
+{ \
+ struct cxl_address_space *space = dev_to_address_space(dev); \
+ \
+ return sysfs_emit(buf, "%s\n", \
+ (space->flags & (flag)) ? "1" : "0"); \
+} \
+static DEVICE_ATTR_RO(supports_##name)
+
+CXL_ATTR_SUPPORTS(pmem, CXL_ADDRSPACE_PMEM);
+CXL_ATTR_SUPPORTS(ram, CXL_ADDRSPACE_RAM);
+CXL_ATTR_SUPPORTS(type2, CXL_ADDRSPACE_TYPE2);
+CXL_ATTR_SUPPORTS(type3, CXL_ADDRSPACE_TYPE3);
+
+static struct attribute *cxl_address_space_attributes[] = {
+ &dev_attr_start.attr,
+ &dev_attr_end.attr,
+ &dev_attr_supports_pmem.attr,
+ &dev_attr_supports_ram.attr,
+ &dev_attr_supports_type2.attr,
+ &dev_attr_supports_type3.attr,
+ NULL,
+};
+
+static umode_t cxl_address_space_visible(struct kobject *kobj,
+ struct attribute *a, int n)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct cxl_address_space *space = dev_to_address_space(dev);
+
+ if (a == &dev_attr_supports_pmem.attr &&
+ !(space->flags & CXL_ADDRSPACE_PMEM))
+ return 0;
+
+ if (a == &dev_attr_supports_ram.attr &&
+ !(space->flags & CXL_ADDRSPACE_RAM))
+ return 0;
+
+ if (a == &dev_attr_supports_type2.attr &&
+ !(space->flags & CXL_ADDRSPACE_TYPE2))
+ return 0;
+
+ if (a == &dev_attr_supports_type3.attr &&
+ !(space->flags & CXL_ADDRSPACE_TYPE3))
+ return 0;
+
+ return a->mode;
+}
+
+static struct attribute_group cxl_address_space_attribute_group = {
+ .attrs = cxl_address_space_attributes,
+ .is_visible = cxl_address_space_visible,
+};
+
+static const struct attribute_group *cxl_address_space_attribute_groups[] = {
+ &cxl_address_space_attribute_group,
+ &cxl_base_attribute_group,
+ NULL,
+};
+
+static void cxl_address_space_release(struct device *dev)
+{
+ struct cxl_address_space_dev *cxl_asd = to_cxl_address_space(dev);
+
+ remove_resource(&cxl_asd->res);
+ kfree(cxl_asd);
+}
+
+static const struct device_type cxl_address_space_type = {
+ .name = "cxl_address_space",
+ .release = cxl_address_space_release,
+ .groups = cxl_address_space_attribute_groups,
+};
+
+struct cxl_address_space_dev *to_cxl_address_space(struct device *dev)
+{
+ if (dev_WARN_ONCE(dev, dev->type != &cxl_address_space_type,
+ "not a cxl_address_space device\n"))
+ return NULL;
+ return container_of(dev, struct cxl_address_space_dev, dev);
+}
+
+static void cxl_root_release(struct device *dev)
+{
+ struct cxl_root *cxl_root = to_cxl_root(dev);
+
+ ida_free(&cxl_port_ida, cxl_root->port.id);
+ kfree(cxl_root);
+}
+
+static ssize_t target_id_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct cxl_port *cxl_port = to_cxl_port(dev);
+
+ return sysfs_emit(buf, "%d\n", cxl_port->target_id);
+}
+static DEVICE_ATTR_RO(target_id);
+
+static struct attribute *cxl_port_attributes[] = {
+ &dev_attr_target_id.attr,
+ NULL,
+};
+
+static struct attribute_group cxl_port_attribute_group = {
+ .attrs = cxl_port_attributes,
+};
+
+static const struct attribute_group *cxl_port_attribute_groups[] = {
+ &cxl_port_attribute_group,
+ &cxl_base_attribute_group,
+ NULL,
+};
+
+static const struct device_type cxl_root_type = {
+ .name = "cxl_root",
+ .release = cxl_root_release,
+ .groups = cxl_port_attribute_groups,
+};
+
+struct cxl_root *to_cxl_root(struct device *dev)
+{
+ if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
+ "not a cxl_root device\n"))
+ return NULL;
+ return container_of(dev, struct cxl_root, port.dev);
+}
+
+struct cxl_port *to_cxl_port(struct device *dev)
+{
+ if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
+ "not a cxl_port device\n"))
+ return NULL;
+ return container_of(dev, struct cxl_port, dev);
+}
+
+static void unregister_dev(void *dev)
+{
+ device_unregister(dev);
+}
+
+static struct cxl_root *cxl_root_alloc(struct device *parent,
+ struct cxl_address_space *cxl_space,
+ int nr_spaces)
+{
+ struct cxl_root *cxl_root;
+ struct cxl_port *port;
+ struct device *dev;
+ int rc;
+
+ cxl_root = kzalloc(struct_size(cxl_root, address_space, nr_spaces),
+ GFP_KERNEL);
+ if (!cxl_root)
+ return ERR_PTR(-ENOMEM);
+
+ memcpy(cxl_root->address_space, cxl_space,
+ flex_array_size(cxl_root, address_space, nr_spaces));
+ cxl_root->nr_spaces = nr_spaces;
+
+ rc = ida_alloc(&cxl_port_ida, GFP_KERNEL);
+ if (rc < 0)
+ goto err;
+ port = &cxl_root->port;
+ port->id = rc;
+
+ /*
+ * Root does not have a cxl_port as its parent and it does not
+ * have any corresponding component registers. It is only a
+ * logical anchor to the first level of actual ports that decode
+ * the root address spaces.
+ */
+ port->port_host = parent;
+ port->target_id = -1;
+ port->component_regs_phys = -1;
+
+ dev = &port->dev;
+ device_initialize(dev);
+ device_set_pm_not_required(dev);
+ dev->parent = parent;
+ dev->bus = &cxl_bus_type;
+ dev->type = &cxl_root_type;
+
+ return cxl_root;
+
+err:
+ kfree(cxl_root);
+ return ERR_PTR(rc);
+}
+
+static struct cxl_address_space_dev *
+cxl_address_space_dev_alloc(struct device *parent,
+ struct cxl_address_space *space)
+{
+ struct cxl_address_space_dev *cxl_asd;
+ struct resource *res;
+ struct device *dev;
+ int rc;
+
+ cxl_asd = kzalloc(sizeof(*cxl_asd), GFP_KERNEL);
+ if (!cxl_asd)
+ return ERR_PTR(-ENOMEM);
+
+ res = &cxl_asd->res;
+ res->name = "CXL Address Space";
+ res->start = space->range.start;
+ res->end = space->range.end;
+ res->flags = IORESOURCE_MEM;
+
+ rc = insert_resource(&iomem_resource, res);
+ if (rc)
+ goto err;
+
+ cxl_asd->address_space = space;
+ dev = &cxl_asd->dev;
+ device_initialize(dev);
+ device_set_pm_not_required(dev);
+ dev->parent = parent;
+ dev->type = &cxl_address_space_type;
+
+ return cxl_asd;
+
+err:
+ kfree(cxl_asd);
+ return ERR_PTR(rc);
+}
+
+
+/**
+ * cxl_address_space_dev_add() - Name and add the cxl address space device
+ * @host: devm context to trigger device removal at @host ->remove()
+ * @cxl_asd: address space device to add
+ * @id: index in the root array of address spaces and name component
+ */
+static int cxl_address_space_dev_add(struct device *host,
+ struct cxl_address_space_dev *cxl_asd,
+ int id)
+{
+ struct device *dev = &cxl_asd->dev;
+ int rc;
+
+ rc = dev_set_name(dev, "address_space%d", id);
+ if (rc)
+ goto err;
+
+ rc = device_add(dev);
+ if (rc)
+ goto err;
+
+ dev_dbg(host, "%s: register %s\n", dev_name(dev->parent),
+ dev_name(dev));
+
+ return devm_add_action_or_reset(host, unregister_dev, dev);
+
+err:
+ put_device(dev);
+ return rc;
+}
+
+struct cxl_root *devm_cxl_add_root(struct device *host,
+ struct cxl_address_space *cxl_space,
+ int nr_spaces)
+{
+ struct cxl_root *cxl_root;
+ struct cxl_port *port;
+ struct device *dev;
+ int i, rc;
+
+ cxl_root = cxl_root_alloc(host, cxl_space, nr_spaces);
+ if (IS_ERR(cxl_root))
+ return cxl_root;
+
+ port = &cxl_root->port;
+ dev = &port->dev;
+ rc = dev_set_name(dev, "root%d", port->id);
+ if (rc)
+ goto err;
+
+ rc = device_add(dev);
+ if (rc)
+ goto err;
+
+ rc = devm_add_action_or_reset(host, unregister_dev, dev);
+ if (rc)
+ return ERR_PTR(rc);
+
+ for (i = 0; i < nr_spaces; i++) {
+ struct cxl_address_space *space = &cxl_root->address_space[i];
+ struct cxl_address_space_dev *cxl_asd;
+
+ if (!range_len(&space->range))
+ continue;
+
+ cxl_asd = cxl_address_space_dev_alloc(dev, space);
+ if (IS_ERR(cxl_asd))
+ return ERR_CAST(cxl_asd);
+
+ rc = cxl_address_space_dev_add(host, cxl_asd, i);
+ if (rc)
+ return ERR_PTR(rc);
+ }
+
+ return cxl_root;
+
+err:
+ put_device(dev);
+ return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_GPL(devm_cxl_add_root);
+
/**
* cxl_setup_device_regs() - Detect CXL Device register blocks
* @dev: Host device of the @base mapping
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index d49e0cb679fa..5cd1173151e5 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -6,6 +6,7 @@
#include <linux/bitfield.h>
#include <linux/bitops.h>
+#include <linux/range.h>
#include <linux/io.h>
/* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
@@ -69,5 +70,69 @@ struct cxl_regs {
void cxl_setup_device_regs(struct device *dev, void __iomem *base,
struct cxl_device_regs *regs);
+/*
+ * Address space properties derived from:
+ * CXL 2.0 8.2.5.12.7 CXL HDM Decoder 0 Control Register
+ */
+#define CXL_ADDRSPACE_RAM BIT(0)
+#define CXL_ADDRSPACE_PMEM BIT(1)
+#define CXL_ADDRSPACE_TYPE2 BIT(2)
+#define CXL_ADDRSPACE_TYPE3 BIT(3)
+#define CXL_ADDRSPACE_MASK GENMASK(3, 0)
+
+struct cxl_address_space {
+ struct range range;
+ int interleave_size;
+ unsigned long flags;
+ unsigned long targets;
+};
+
+struct cxl_address_space_dev {
+ struct device dev;
+ struct resource res;
+ struct cxl_address_space *address_space;
+};
+
+/**
+ * struct cxl_port - object representing a root, upstream, or downstream port
+ * @dev: this port's device
+ * @port_host: PCI or platform device host of the CXL capability
+ * @id: id for port device-name
+ * @target_id: this port's HDM decoder id in the parent port
+ * @component_regs_phys: component register capability array base address
+ */
+struct cxl_port {
+ struct device dev;
+ struct device *port_host;
+ int id;
+ int target_id;
+ resource_size_t component_regs_phys;
+};
+
+/*
+ * struct cxl_root - platform object parent of CXL host bridges
+ *
+ * A cxl_root object represents a set of address spaces that are
+ * interleaved across a set of child host bridges, but never interleaved
+ * to another cxl_root object. It contains a cxl_port that is a special
+ * case in that it does not have a parent port and related HDMs, instead
+ * its decode is derived from the root (platform firmware defined)
+ * address space description. Not to be confused with CXL Root Ports
+ * that are the PCIE Root Ports within PCIE Host Bridges that are
+ * flagged by platform firmware (ACPI0016 on ACPI platforms) as having
+ * CXL capabilities.
+ */
+struct cxl_root {
+ struct cxl_port port;
+ int nr_spaces;
+ struct cxl_address_space address_space[];
+};
+
+struct cxl_root *to_cxl_root(struct device *dev);
+struct cxl_port *to_cxl_port(struct device *dev);
+struct cxl_address_space_dev *to_cxl_address_space(struct device *dev);
+struct cxl_root *devm_cxl_add_root(struct device *parent,
+ struct cxl_address_space *cxl_space,
+ int nr_spaces);
extern struct bus_type cxl_bus_type;
#endif /* __CXL_H__ */
[debug / to-be-replaced / not-for-upstream]
Given ACPICA support is needed before drivers can integrate ACPI
functionality add some module parameters as proxies.
---
drivers/cxl/acpi.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 79 insertions(+), 2 deletions(-)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index bc2a35ae880b..2a48a728f3e0 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -4,10 +4,84 @@
#include <linux/module.h>
#include <linux/device.h>
#include <linux/kernel.h>
+#include <linux/range.h>
#include <linux/acpi.h>
#include <linux/pci.h>
#include "cxl.h"
+/*
+ * TODO: Replace all of the below module parameters with ACPI CXL
+ * resource descriptions once ACPICA makes them available.
+ */
+static unsigned long chbcr[4];
+module_param_named(chbcr0, chbcr[0], ulong, 0400);
+module_param_named(chbcr1, chbcr[1], ulong, 0400);
+module_param_named(chbcr2, chbcr[2], ulong, 0400);
+module_param_named(chbcr3, chbcr[3], ulong, 0400);
+
+/* TODO: cross-bridge interleave */
+static struct cxl_address_space cxl_space[] = {
+ [0] = { .range = { 0, -1 }, .targets = 0x1, },
+ [1] = { .range = { 0, -1 }, .targets = 0x1, },
+ [2] = { .range = { 0, -1 }, .targets = 0x1, },
+ [3] = { .range = { 0, -1 }, .targets = 0x1, },
+};
+
+static int set_range(const char *val, const struct kernel_param *kp)
+{
+ unsigned long long size, base;
+ struct cxl_address_space *space;
+ unsigned long flags;
+ char *p;
+ int rc;
+
+ size = memparse(val, &p);
+ if (*p != '@')
+ return -EINVAL;
+
+ base = memparse(p + 1, &p);
+ if (*p != ':')
+ return -EINVAL;
+
+ rc = kstrtoul(p + 1, 0, &flags);
+ if (rc)
+ return rc;
+ if (!flags || flags > CXL_ADDRSPACE_MASK)
+ return rc;
+
+ space = kp->arg;
+ *space = (struct cxl_address_space) {
+ .range = {
+ .start = base,
+ .end = base + size - 1,
+ },
+ .flags = flags,
+ };
+
+ return 0;
+}
+
+static int get_range(char *buf, const struct kernel_param *kp)
+{
+ struct cxl_address_space *space = kp->arg;
+
+ if (!range_len(&space->range))
+ return -EINVAL;
+
+ return sysfs_emit(buf, "%#llx@%#llx :%s%s%s%s\n",
+ (unsigned long long)range_len(&space->range),
+ (unsigned long long)space->range.start,
+ space->flags & CXL_ADDRSPACE_RAM ? " ram" : "",
+ space->flags & CXL_ADDRSPACE_PMEM ? " pmem" : "",
+ space->flags & CXL_ADDRSPACE_TYPE2 ? " type2" : "",
+ space->flags & CXL_ADDRSPACE_TYPE3 ? " type3" : "");
+}
+
+module_param_call(range0, set_range, get_range, &cxl_space[0], 0400);
+module_param_call(range1, set_range, get_range, &cxl_space[1], 0400);
+module_param_call(range2, set_range, get_range, &cxl_space[2], 0400);
+module_param_call(range3, set_range, get_range, &cxl_space[3], 0400);
+
static int match_ACPI0016(struct device *dev, const void *host)
{
struct acpi_device *adev = to_acpi_device(dev);
@@ -67,13 +141,16 @@ static int cxl_acpi_register_ports(struct device *dev, struct acpi_device *root,
struct cxl_port *port, int idx)
{
struct acpi_pci_root *pci_root = acpi_pci_find_root(root->handle);
+ resource_size_t chbcr_base = ~0ULL;
struct cxl_walk_context ctx;
if (!pci_root)
return -ENXIO;
/* TODO: fold in CEDT.CHBS retrieval */
- port = devm_cxl_add_port(dev, port, &root->dev, idx, ~0ULL);
+ if (idx < ARRAY_SIZE(chbcr))
+ chbcr_base = chbcr[idx];
+ port = devm_cxl_add_port(dev, port, &root->dev, idx, chbcr_base);
if (IS_ERR(port))
return PTR_ERR(port);
dev_dbg(dev, "%s: register: %s\n", dev_name(&root->dev),
@@ -99,7 +176,7 @@ static int cxl_acpi_probe(struct platform_device *pdev)
struct cxl_root *cxl_root;
int rc, i = 0;
- cxl_root = devm_cxl_add_root(dev, NULL, 0);
+ cxl_root = devm_cxl_add_root(dev, cxl_space, ARRAY_SIZE(cxl_space));
if (IS_ERR(cxl_root))
return PTR_ERR(cxl_root);
dev_dbg(dev, "register: %s\n", dev_name(&cxl_root->port.dev));
Hi Dan,
I love your patch! Perhaps something to improve:
[auto build test WARNING on a38fd8748464831584a19438cbb3082b5a2dab15]
url: https://github.com/0day-ci/linux/commits/Dan-Williams/CXL-Port-Enumeration-and-Plans-for-v5-14/20210508-065317
base: a38fd8748464831584a19438cbb3082b5a2dab15
config: i386-randconfig-a003-20210507 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/10f83390eae24effd86455f46429d03ae7c35f53
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Dan-Williams/CXL-Port-Enumeration-and-Plans-for-v5-14/20210508-065317
git checkout 10f83390eae24effd86455f46429d03ae7c35f53
# save the attached .config to linux build tree
make W=1 W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/cxl/acpi.c: In function 'cxl_acpi_register_ports':
>> drivers/cxl/acpi.c:76:55: warning: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '18446744073709551615' to '4294967295' [-Woverflow]
76 | port = devm_cxl_add_port(dev, port, &root->dev, idx, ~0ULL);
| ^~~~~
vim +76 drivers/cxl/acpi.c
61
62 /*
63 * A host bridge may contain one or more root ports. Register each port
64 * as a child of the cxl_root.
65 */
66 static int cxl_acpi_register_ports(struct device *dev, struct acpi_device *root,
67 struct cxl_port *port, int idx)
68 {
69 struct acpi_pci_root *pci_root = acpi_pci_find_root(root->handle);
70 struct cxl_walk_context ctx;
71
72 if (!pci_root)
73 return -ENXIO;
74
75 /* TODO: fold in CEDT.CHBS retrieval */
> 76 port = devm_cxl_add_port(dev, port, &root->dev, idx, ~0ULL);
77 if (IS_ERR(port))
78 return PTR_ERR(port);
79 dev_dbg(dev, "%s: register: %s\n", dev_name(&root->dev),
80 dev_name(&port->dev));
81
82 ctx = (struct cxl_walk_context) {
83 .dev = dev,
84 .root = pci_root->bus,
85 .port = port,
86 };
87 pci_walk_bus(pci_root->bus, match_add_root_ports, &ctx);
88
89 if (ctx.count == 0)
90 return -ENODEV;
91 return ctx.error;
92 }
93
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Fri, 7 May 2021 15:51:47 -0700
Dan Williams <[email protected]> wrote:
> While CXL builds upon the PCI software model for dynamic enumeration and
> control, a static platform component is required to bootstrap the CXL
> memory layout. In addition to identifying the host bridges ACPI is
> responsible for enumerating the CXL memory space that can be addressed
> by decoders. This is similar to the requirement for ACPI to publish
> resources reported by _CRS for PCI host bridges.
>
> Introduce the cxl_root object as an abstract "port" into the CXL.mem
> address space described by HDM decoders identified by the ACPI
> CEDT.CHBS.
>
> For now just establish the initial boilerplate and sysfs attributes, to
> be followed by enumeration of the ports within the host bridge.
>
> Note the allocation of CXL core device objects is split into separate
> alloc and add steps in order to separate the alloc error path (kfree())
> from the device add error path (put_device()).
>
> Signed-off-by: Dan Williams <[email protected]>
Hi Dan
Just one bit in here that confused me (assuming I'm reading the code correctly).
You have is_visible for the dev_attr_supports_pmem etc to only show them if
the particular space supports that memory type. That's fine. You also have
the actual sysfs function checking the same flag to decide to return "1" or "0"
which would also be fine, but in combination it's rather odd as the sysfs
read function can never return "0" (sysfs attribute isn't visible in that
condition). Probably deserves at least a comment.
This also needs some documentation for the new sysfs ABI
(Documentation/ABI/...) but that can be in a separate patch.
Otherwise looks good to me.
Jonathan
> ---
> drivers/cxl/Kconfig | 14 ++
> drivers/cxl/Makefile | 2
> drivers/cxl/acpi.c | 39 +++++
> drivers/cxl/core.c | 356 ++++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/cxl/cxl.h | 65 +++++++++
> 5 files changed, 476 insertions(+)
> create mode 100644 drivers/cxl/acpi.c
>
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 97dc4d751651..fb282af84afd 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -50,4 +50,18 @@ config CXL_MEM_RAW_COMMANDS
> potential impact to memory currently in use by the kernel.
>
> If developing CXL hardware or the driver say Y, otherwise say N.
> +
> +config CXL_ACPI
> + tristate "CXL ACPI: Platform Support"
> + depends on ACPI
> + help
> + Enable support for host managed device memory (HDM) resources
> + published by a platform's ACPI CXL memory layout description.
> + See Chapter 9.14.1 CXL Early Discovery Table (CEDT) in the CXL
> + 2.0 specification. The CXL core consumes these resource to
> + publish port and address_space objects used to map regions
> + that represent System RAM, or Persistent Memory regions to be
> + managed by LIBNVDIMM.
> +
> + If unsure say 'm'.
> endif
> diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> index 3808e39dd31f..f429ca6b59d9 100644
> --- a/drivers/cxl/Makefile
> +++ b/drivers/cxl/Makefile
> @@ -1,7 +1,9 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_CXL_BUS) += cxl_core.o
> obj-$(CONFIG_CXL_MEM) += cxl_mem.o
> +obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
>
> ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL
> cxl_core-y := core.o
> cxl_mem-y := mem.o
> +cxl_acpi-y := acpi.o
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> new file mode 100644
> index 000000000000..d54c2d5de730
> --- /dev/null
> +++ b/drivers/cxl/acpi.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/acpi.h>
> +#include "cxl.h"
> +
> +static int cxl_acpi_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct cxl_root *cxl_root;
> +
> + cxl_root = devm_cxl_add_root(dev, NULL, 0);
> + if (IS_ERR(cxl_root))
> + return PTR_ERR(cxl_root);
> + dev_dbg(dev, "register: %s\n", dev_name(&cxl_root->port.dev));
> +
> + return 0;
> +}
> +
> +static const struct acpi_device_id cxl_acpi_ids[] = {
> + { "ACPI0017", 0 },
> + { "", 0 },
> +};
> +MODULE_DEVICE_TABLE(acpi, cxl_acpi_ids);
> +
> +static struct platform_driver cxl_acpi_driver = {
> + .probe = cxl_acpi_probe,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .acpi_match_table = cxl_acpi_ids,
> + },
> +};
> +
> +module_platform_driver(cxl_acpi_driver);
> +MODULE_LICENSE("GPL v2");
> +MODULE_IMPORT_NS(CXL);
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index d94c4c2dee8f..347824a62a66 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -2,6 +2,8 @@
> /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
> #include <linux/device.h>
> #include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/idr.h>
> #include "cxl.h"
>
> /**
> @@ -11,6 +13,360 @@
> * point for cross-device interleave coordination through cxl ports.
> */
>
> +static DEFINE_IDA(cxl_port_ida);
> +
> +static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + return sysfs_emit(buf, "%s\n", dev->type->name);
> +}
> +static DEVICE_ATTR_RO(devtype);
> +
> +static struct attribute *cxl_base_attributes[] = {
> + &dev_attr_devtype.attr,
> + NULL,
> +};
> +
> +static struct attribute_group cxl_base_attribute_group = {
> + .attrs = cxl_base_attributes,
> +};
> +
> +static struct cxl_address_space *dev_to_address_space(struct device *dev)
> +{
> + struct cxl_address_space_dev *cxl_asd = to_cxl_address_space(dev);
> +
> + return cxl_asd->address_space;
> +}
> +
> +static ssize_t start_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct cxl_address_space *space = dev_to_address_space(dev);
> +
> + return sysfs_emit(buf, "%#llx\n", space->range.start);
> +}
> +static DEVICE_ATTR_RO(start);
> +
> +static ssize_t end_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct cxl_address_space *space = dev_to_address_space(dev);
> +
> + return sysfs_emit(buf, "%#llx\n", space->range.end);
> +}
> +static DEVICE_ATTR_RO(end);
> +
> +#define CXL_ATTR_SUPPORTS(name, flag) \
> +static ssize_t supports_##name##_show( \
> + struct device *dev, struct device_attribute *attr, char *buf) \
> +{ \
> + struct cxl_address_space *space = dev_to_address_space(dev); \
> + \
> + return sysfs_emit(buf, "%s\n", \
> + (space->flags & (flag)) ? "1" : "0"); \
> +} \
> +static DEVICE_ATTR_RO(supports_##name)
I think this can only return 1 if it is possible to read it. See below.
> +
> +CXL_ATTR_SUPPORTS(pmem, CXL_ADDRSPACE_PMEM);
> +CXL_ATTR_SUPPORTS(ram, CXL_ADDRSPACE_RAM);
> +CXL_ATTR_SUPPORTS(type2, CXL_ADDRSPACE_TYPE2);
> +CXL_ATTR_SUPPORTS(type3, CXL_ADDRSPACE_TYPE3);
> +
> +static struct attribute *cxl_address_space_attributes[] = {
> + &dev_attr_start.attr,
> + &dev_attr_end.attr,
> + &dev_attr_supports_pmem.attr,
> + &dev_attr_supports_ram.attr,
> + &dev_attr_supports_type2.attr,
> + &dev_attr_supports_type3.attr,
> + NULL,
> +};
> +
> +static umode_t cxl_address_space_visible(struct kobject *kobj,
> + struct attribute *a, int n)
> +{
> + struct device *dev = container_of(kobj, struct device, kobj);
> + struct cxl_address_space *space = dev_to_address_space(dev);
> +
> + if (a == &dev_attr_supports_pmem.attr &&
> + !(space->flags & CXL_ADDRSPACE_PMEM))
> + return 0;
> +
> + if (a == &dev_attr_supports_ram.attr &&
> + !(space->flags & CXL_ADDRSPACE_RAM))
> + return 0;
> +
> + if (a == &dev_attr_supports_type2.attr &&
> + !(space->flags & CXL_ADDRSPACE_TYPE2))
> + return 0;
> +
> + if (a == &dev_attr_supports_type3.attr &&
> + !(space->flags & CXL_ADDRSPACE_TYPE3))
> + return 0;
Why control visibility for these given they are boolean sysfs attributes
that say if the thing is supported or not? Hiding them if not supported
seems an odd thing to do whilst also allowing them to take a value of 0.
> +
> + return a->mode;
> +}
> +
> +static struct attribute_group cxl_address_space_attribute_group = {
> + .attrs = cxl_address_space_attributes,
> + .is_visible = cxl_address_space_visible,
> +};
> +
> +static const struct attribute_group *cxl_address_space_attribute_groups[] = {
> + &cxl_address_space_attribute_group,
> + &cxl_base_attribute_group,
> + NULL,
> +};
> +
> +static void cxl_address_space_release(struct device *dev)
> +{
> + struct cxl_address_space_dev *cxl_asd = to_cxl_address_space(dev);
> +
> + remove_resource(&cxl_asd->res);
> + kfree(cxl_asd);
> +}
> +
> +static const struct device_type cxl_address_space_type = {
> + .name = "cxl_address_space",
> + .release = cxl_address_space_release,
> + .groups = cxl_address_space_attribute_groups,
> +};
> +
> +struct cxl_address_space_dev *to_cxl_address_space(struct device *dev)
> +{
> + if (dev_WARN_ONCE(dev, dev->type != &cxl_address_space_type,
> + "not a cxl_address_space device\n"))
> + return NULL;
> + return container_of(dev, struct cxl_address_space_dev, dev);
> +}
> +
> +static void cxl_root_release(struct device *dev)
> +{
> + struct cxl_root *cxl_root = to_cxl_root(dev);
> +
> + ida_free(&cxl_port_ida, cxl_root->port.id);
> + kfree(cxl_root);
> +}
> +
> +static ssize_t target_id_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct cxl_port *cxl_port = to_cxl_port(dev);
> +
> + return sysfs_emit(buf, "%d\n", cxl_port->target_id);
> +}
> +static DEVICE_ATTR_RO(target_id);
> +
> +static struct attribute *cxl_port_attributes[] = {
> + &dev_attr_target_id.attr,
> + NULL,
> +};
> +
> +static struct attribute_group cxl_port_attribute_group = {
> + .attrs = cxl_port_attributes,
> +};
> +
> +static const struct attribute_group *cxl_port_attribute_groups[] = {
> + &cxl_port_attribute_group,
> + &cxl_base_attribute_group,
> + NULL,
> +};
> +
> +static const struct device_type cxl_root_type = {
> + .name = "cxl_root",
> + .release = cxl_root_release,
> + .groups = cxl_port_attribute_groups,
> +};
> +
> +struct cxl_root *to_cxl_root(struct device *dev)
> +{
> + if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
> + "not a cxl_root device\n"))
> + return NULL;
> + return container_of(dev, struct cxl_root, port.dev);
> +}
> +
> +struct cxl_port *to_cxl_port(struct device *dev)
> +{
> + if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
> + "not a cxl_port device\n"))
> + return NULL;
> + return container_of(dev, struct cxl_port, dev);
> +}
> +
> +static void unregister_dev(void *dev)
> +{
> + device_unregister(dev);
> +}
> +
> +static struct cxl_root *cxl_root_alloc(struct device *parent,
> + struct cxl_address_space *cxl_space,
> + int nr_spaces)
> +{
> + struct cxl_root *cxl_root;
> + struct cxl_port *port;
> + struct device *dev;
> + int rc;
> +
> + cxl_root = kzalloc(struct_size(cxl_root, address_space, nr_spaces),
> + GFP_KERNEL);
> + if (!cxl_root)
> + return ERR_PTR(-ENOMEM);
> +
> + memcpy(cxl_root->address_space, cxl_space,
> + flex_array_size(cxl_root, address_space, nr_spaces));
> + cxl_root->nr_spaces = nr_spaces;
> +
> + rc = ida_alloc(&cxl_port_ida, GFP_KERNEL);
> + if (rc < 0)
> + goto err;
> + port = &cxl_root->port;
> + port->id = rc;
> +
> + /*
> + * Root does not have a cxl_port as its parent and it does not
> + * have any corresponding component registers. It is only a
> + * logical anchor to the first level of actual ports that decode
> + * the root address spaces.
> + */
> + port->port_host = parent;
> + port->target_id = -1;
> + port->component_regs_phys = -1;
> +
> + dev = &port->dev;
> + device_initialize(dev);
> + device_set_pm_not_required(dev);
> + dev->parent = parent;
> + dev->bus = &cxl_bus_type;
> + dev->type = &cxl_root_type;
> +
> + return cxl_root;
> +
> +err:
> + kfree(cxl_root);
> + return ERR_PTR(rc);
> +}
> +
> +static struct cxl_address_space_dev *
> +cxl_address_space_dev_alloc(struct device *parent,
> + struct cxl_address_space *space)
> +{
> + struct cxl_address_space_dev *cxl_asd;
> + struct resource *res;
> + struct device *dev;
> + int rc;
> +
> + cxl_asd = kzalloc(sizeof(*cxl_asd), GFP_KERNEL);
> + if (!cxl_asd)
> + return ERR_PTR(-ENOMEM);
> +
> + res = &cxl_asd->res;
> + res->name = "CXL Address Space";
> + res->start = space->range.start;
> + res->end = space->range.end;
> + res->flags = IORESOURCE_MEM;
> +
> + rc = insert_resource(&iomem_resource, res);
> + if (rc)
> + goto err;
> +
> + cxl_asd->address_space = space;
> + dev = &cxl_asd->dev;
> + device_initialize(dev);
> + device_set_pm_not_required(dev);
> + dev->parent = parent;
> + dev->type = &cxl_address_space_type;
> +
> + return cxl_asd;
> +
> +err:
> + kfree(cxl_asd);
> + return ERR_PTR(rc);
> +}
> +
> +
> +/**
> + * cxl_address_space_dev_add() - Name and add the cxl address space device
> + * @host: devm context to trigger device removal at @host ->remove()
> + * @cxl_asd: address space device to add
> + * @id: index in the root array of address spaces and name component
> + */
> +static int cxl_address_space_dev_add(struct device *host,
> + struct cxl_address_space_dev *cxl_asd,
> + int id)
> +{
> + struct device *dev = &cxl_asd->dev;
> + int rc;
> +
> + rc = dev_set_name(dev, "address_space%d", id);
> + if (rc)
> + goto err;
> +
> + rc = device_add(dev);
> + if (rc)
> + goto err;
> +
> + dev_dbg(host, "%s: register %s\n", dev_name(dev->parent),
> + dev_name(dev));
> +
> + return devm_add_action_or_reset(host, unregister_dev, dev);
> +
> +err:
> + put_device(dev);
> + return rc;
> +}
> +
> +struct cxl_root *devm_cxl_add_root(struct device *host,
> + struct cxl_address_space *cxl_space,
> + int nr_spaces)
> +{
> + struct cxl_root *cxl_root;
> + struct cxl_port *port;
> + struct device *dev;
> + int i, rc;
> +
> + cxl_root = cxl_root_alloc(host, cxl_space, nr_spaces);
> + if (IS_ERR(cxl_root))
> + return cxl_root;
> +
> + port = &cxl_root->port;
> + dev = &port->dev;
> + rc = dev_set_name(dev, "root%d", port->id);
> + if (rc)
> + goto err;
> +
> + rc = device_add(dev);
> + if (rc)
> + goto err;
> +
> + rc = devm_add_action_or_reset(host, unregister_dev, dev);
> + if (rc)
> + return ERR_PTR(rc);
> +
> + for (i = 0; i < nr_spaces; i++) {
> + struct cxl_address_space *space = &cxl_root->address_space[i];
> + struct cxl_address_space_dev *cxl_asd;
> +
> + if (!range_len(&space->range))
> + continue;
> +
> + cxl_asd = cxl_address_space_dev_alloc(dev, space);
> + if (IS_ERR(cxl_asd))
> + return ERR_CAST(cxl_asd);
> +
> + rc = cxl_address_space_dev_add(host, cxl_asd, i);
> + if (rc)
> + return ERR_PTR(rc);
> + }
> +
> + return cxl_root;
> +
> +err:
> + put_device(dev);
> + return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_GPL(devm_cxl_add_root);
> +
> /**
> * cxl_setup_device_regs() - Detect CXL Device register blocks
> * @dev: Host device of the @base mapping
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index d49e0cb679fa..5cd1173151e5 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -6,6 +6,7 @@
>
> #include <linux/bitfield.h>
> #include <linux/bitops.h>
> +#include <linux/range.h>
> #include <linux/io.h>
>
> /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
> @@ -69,5 +70,69 @@ struct cxl_regs {
> void cxl_setup_device_regs(struct device *dev, void __iomem *base,
> struct cxl_device_regs *regs);
>
> +/*
> + * Address space properties derived from:
> + * CXL 2.0 8.2.5.12.7 CXL HDM Decoder 0 Control Register
> + */
> +#define CXL_ADDRSPACE_RAM BIT(0)
> +#define CXL_ADDRSPACE_PMEM BIT(1)
> +#define CXL_ADDRSPACE_TYPE2 BIT(2)
> +#define CXL_ADDRSPACE_TYPE3 BIT(3)
> +#define CXL_ADDRSPACE_MASK GENMASK(3, 0)
> +
> +struct cxl_address_space {
> + struct range range;
> + int interleave_size;
> + unsigned long flags;
> + unsigned long targets;
> +};
> +
> +struct cxl_address_space_dev {
> + struct device dev;
> + struct resource res;
> + struct cxl_address_space *address_space;
> +};
> +
> +/**
> + * struct cxl_port - object representing a root, upstream, or downstream port
> + * @dev: this port's device
> + * @port_host: PCI or platform device host of the CXL capability
> + * @id: id for port device-name
> + * @target_id: this port's HDM decoder id in the parent port
> + * @component_regs_phys: component register capability array base address
> + */
> +struct cxl_port {
> + struct device dev;
> + struct device *port_host;
> + int id;
> + int target_id;
> + resource_size_t component_regs_phys;
> +};
> +
> +/*
> + * struct cxl_root - platform object parent of CXL host bridges
> + *
> + * A cxl_root object represents a set of address spaces that are
> + * interleaved across a set of child host bridges, but never interleaved
> + * to another cxl_root object. It contains a cxl_port that is a special
> + * case in that it does not have a parent port and related HDMs, instead
> + * its decode is derived from the root (platform firmware defined)
> + * address space description. Not to be confused with CXL Root Ports
> + * that are the PCIE Root Ports within PCIE Host Bridges that are
> + * flagged by platform firmware (ACPI0016 on ACPI platforms) as having
> + * CXL capabilities.
> + */
> +struct cxl_root {
> + struct cxl_port port;
> + int nr_spaces;
> + struct cxl_address_space address_space[];
> +};
> +
> +struct cxl_root *to_cxl_root(struct device *dev);
> +struct cxl_port *to_cxl_port(struct device *dev);
> +struct cxl_address_space_dev *to_cxl_address_space(struct device *dev);
> +struct cxl_root *devm_cxl_add_root(struct device *parent,
> + struct cxl_address_space *cxl_space,
> + int nr_spaces);
> extern struct bus_type cxl_bus_type;
> #endif /* __CXL_H__ */
>
On Fri, 7 May 2021 15:51:53 -0700
Dan Williams <[email protected]> wrote:
> CONFIG_CXL_BUS is default 'n' as expected for new functionality. When
> that is enabled do not make the end user hunt for all the expected
> sub-options to enable. For example CONFIG_CXL_BUS without CONFIG_CXL_MEM
> is an odd/expert configuration, so is CONFIG_CXL_MEM without
> CONFIG_CXL_ACPI (on ACPI capable platforms). Default CONFIG_CXL_MEM and
> CONFIG_CXL_ACPI to CONFIG_CXL_BUS.
>
> Acked-by: Ben Widawsky <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
Acked-by: Jonathan Cameron <[email protected]>
> ---
> drivers/cxl/Kconfig | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index fb282af84afd..1da7970a5e55 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -15,6 +15,7 @@ if CXL_BUS
>
> config CXL_MEM
> tristate "CXL.mem: Memory Devices"
> + default CXL_BUS
> help
> The CXL.mem protocol allows a device to act as a provider of
> "System RAM" and/or "Persistent Memory" that is fully coherent
> @@ -54,6 +55,7 @@ config CXL_MEM_RAW_COMMANDS
> config CXL_ACPI
> tristate "CXL ACPI: Platform Support"
> depends on ACPI
> + default CXL_BUS
> help
> Enable support for host managed device memory (HDM) resources
> published by a platform's ACPI CXL memory layout description.
>
On Fri, 7 May 2021 15:51:58 -0700
Dan Williams <[email protected]> wrote:
> Once the cxl_root is established then other ports in the hierarchy can
> be attached. The cxl_port object, unlike cxl_root that is associated
> with host bridges, is associated with PCIe Root Ports or PCIe Switch
> Ports. Add cxl_port instances for all PCIe Root Ports in an ACPI0016
> host bridge. The cxl_port instances for PCIe Switch Ports are not
> included here as those are to be modeled as another service device
> registered on the pcie_port_bus_type.
>
> A sample sysfs topology for a single-host-bridge with
> single-PCIe/CXL-port follows:
>
> /sys/bus/cxl/devices/root0
> ├── address_space0
> │ ├── devtype
> │ ├── end
> │ ├── start
> │ ├── supports_ram
> │ ├── supports_type2
> │ ├── supports_type3
> │ └── uevent
> ├── address_space1
> │ ├── devtype
> │ ├── end
> │ ├── start
> │ ├── supports_pmem
> │ ├── supports_type2
> │ ├── supports_type3
> │ └── uevent
> ├── devtype
> ├── port1
> │ ├── devtype
> │ ├── host -> ../../../../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
> │ ├── port2
> │ │ ├── devtype
> │ │ ├── host -> ../../../../../pci0000:34/0000:34:00.0
> │ │ ├── subsystem -> ../../../../../../bus/cxl
> │ │ ├── target_id
> │ │ └── uevent
> │ ├── subsystem -> ../../../../../bus/cxl
> │ ├── target_id
> │ └── uevent
> ├── subsystem -> ../../../../bus/cxl
> ├── target_id
> └── uevent
>
> In this listing the system-wide-singleton root0 has 2 address spaces, 1
> PMEM and 1 RAM. Those address spaces are accessed through port1 which
> represents the upstream port of an ACPI0016 host-bridge. A
> multi-host-bridge system would have other ports as peers to port1 to
> additionally decode root level address spaces. Port2 in this diagram
> represents the single downstream port of the host-bridge. Were it to be
> a multi-ported-host-bridge there would be peers / siblings of port2 with
> port1 as their common ancestor.
I guess it would be a pain to emulate a system that actually had
multiple ports at the last level. Pity as would have made your
explanation here a little easier to follow.
>
> The rationale for this port hierarchy is to be able to walk the HDM
> decoder register sets that each port implements. Additionally it
> provides a representation of host-bridge interleave which will be
> necessary for follow-on work that adds CXL region devices.
>
> The details in the /sys/bus/cxl hierarchy that are not suitable to be
> represented in the /sys/bus/pci hierarchy are:
> - memory address spaces that are interleaved across host bridges
> - common sub-device functionality represented by CXL component + device
> registers (enumerated via DVSEC or platform firmware (ACPI CEDT)).
I'm sold :)
>
> Signed-off-by: Dan Williams <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/cxl/acpi.c | 99 +++++++++++++++++++++++++++++++++++++++++++
> drivers/cxl/core.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/cxl/cxl.h | 5 ++
> 3 files changed, 224 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index d54c2d5de730..bc2a35ae880b 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -5,18 +5,117 @@
> #include <linux/device.h>
> #include <linux/kernel.h>
> #include <linux/acpi.h>
> +#include <linux/pci.h>
> #include "cxl.h"
>
> +static int match_ACPI0016(struct device *dev, const void *host)
> +{
> + struct acpi_device *adev = to_acpi_device(dev);
> + const char *hid = acpi_device_hid(adev);
> +
> + return strcmp(hid, "ACPI0016") == 0;
> +}
> +
> +struct cxl_walk_context {
> + struct device *dev;
> + struct pci_bus *root;
> + struct cxl_port *port;
> + int error;
> + int count;
> +};
> +
> +static int match_add_root_ports(struct pci_dev *pdev, void *data)
> +{
> + struct cxl_walk_context *ctx = data;
> + struct pci_bus *root_bus = ctx->root;
> + struct cxl_port *port = ctx->port;
> + int type = pci_pcie_type(pdev);
> + struct device *dev = ctx->dev;
> + resource_size_t cxl_regs_phys;
> + int target_id = ctx->count;
> +
> + if (pdev->bus != root_bus)
> + return 0;
> + if (!pci_is_pcie(pdev))
> + return 0;
> + if (type != PCI_EXP_TYPE_ROOT_PORT)
> + return 0;
> +
> + ctx->count++;
> +
> + /* TODO walk DVSEC to find component register base */
> + cxl_regs_phys = -1;
> +
> + port = devm_cxl_add_port(dev, port, &pdev->dev, target_id,
> + cxl_regs_phys);
> + if (IS_ERR(port)) {
> + ctx->error = PTR_ERR(port);
> + return ctx->error;
> + }
> +
> + dev_dbg(dev, "%s: register: %s\n", dev_name(&pdev->dev),
> + dev_name(&port->dev));
> +
> + return 0;
> +}
> +
> +/*
> + * A host bridge may contain one or more root ports. Register each port
> + * as a child of the cxl_root.
> + */
> +static int cxl_acpi_register_ports(struct device *dev, struct acpi_device *root,
> + struct cxl_port *port, int idx)
> +{
> + struct acpi_pci_root *pci_root = acpi_pci_find_root(root->handle);
> + struct cxl_walk_context ctx;
> +
> + if (!pci_root)
> + return -ENXIO;
> +
> + /* TODO: fold in CEDT.CHBS retrieval */
> + port = devm_cxl_add_port(dev, port, &root->dev, idx, ~0ULL);
> + if (IS_ERR(port))
> + return PTR_ERR(port);
> + dev_dbg(dev, "%s: register: %s\n", dev_name(&root->dev),
> + dev_name(&port->dev));
> +
> + ctx = (struct cxl_walk_context) {
> + .dev = dev,
> + .root = pci_root->bus,
> + .port = port,
> + };
> + pci_walk_bus(pci_root->bus, match_add_root_ports, &ctx);
> +
> + if (ctx.count == 0)
> + return -ENODEV;
> + return ctx.error;
> +}
> +
> static int cxl_acpi_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> + struct device *bridge = NULL;
> struct cxl_root *cxl_root;
> + int rc, i = 0;
>
> cxl_root = devm_cxl_add_root(dev, NULL, 0);
> if (IS_ERR(cxl_root))
> return PTR_ERR(cxl_root);
> dev_dbg(dev, "register: %s\n", dev_name(&cxl_root->port.dev));
>
> + while (true) {
> + bridge = bus_find_device(adev->dev.bus, bridge, dev,
> + match_ACPI0016);
> + if (!bridge)
> + break;
> +
> + rc = cxl_acpi_register_ports(dev, to_acpi_device(bridge),
> + &cxl_root->port, i++);
> + if (rc)
> + return rc;
> + }
> +
> return 0;
> }
>
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index 347824a62a66..cc9af9033292 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -148,6 +148,15 @@ static void cxl_root_release(struct device *dev)
> kfree(cxl_root);
> }
>
> +static void cxl_port_release(struct device *dev)
> +{
> + struct cxl_port *port = to_cxl_port(dev);
> +
> + ida_free(&cxl_port_ida, port->id);
> + put_device(port->port_host);
> + kfree(port);
> +}
> +
> static ssize_t target_id_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> @@ -178,6 +187,12 @@ static const struct device_type cxl_root_type = {
> .groups = cxl_port_attribute_groups,
> };
>
> +static const struct device_type cxl_port_type = {
> + .name = "cxl_port",
> + .release = cxl_port_release,
> + .groups = cxl_port_attribute_groups,
> +};
> +
> struct cxl_root *to_cxl_root(struct device *dev)
> {
> if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
> @@ -188,7 +203,9 @@ struct cxl_root *to_cxl_root(struct device *dev)
>
> struct cxl_port *to_cxl_port(struct device *dev)
> {
> - if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
> + if (dev_WARN_ONCE(dev,
> + dev->type != &cxl_root_type &&
> + dev->type != &cxl_port_type,
> "not a cxl_port device\n"))
> return NULL;
> return container_of(dev, struct cxl_port, dev);
> @@ -367,6 +384,108 @@ struct cxl_root *devm_cxl_add_root(struct device *host,
> }
> EXPORT_SYMBOL_GPL(devm_cxl_add_root);
>
> +static void cxl_unlink_port(void *_port)
> +{
> + struct cxl_port *port = _port;
> +
> + sysfs_remove_link(&port->dev.kobj, "host");
> +}
> +
> +static int devm_cxl_link_port(struct device *dev, struct cxl_port *port)
> +{
> + int rc;
> +
> + rc = sysfs_create_link(&port->dev.kobj, &port->port_host->kobj, "host");
> + if (rc)
> + return rc;
> + return devm_add_action_or_reset(dev, cxl_unlink_port, port);
> +}
> +
> +static struct cxl_port *cxl_port_alloc(struct cxl_port *parent_port,
> + struct device *port_dev, int target_id,
> + resource_size_t component_regs_phys)
> +{
> + struct cxl_port *port;
> + struct device *dev;
> + int rc;
> +
> + if (!port_dev)
> + return ERR_PTR(-EINVAL);
> +
> + port = kzalloc(sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return ERR_PTR(-ENOMEM);
> +
> + rc = ida_alloc(&cxl_port_ida, GFP_KERNEL);
> + if (rc < 0)
> + goto err;
> +
> + port->id = rc;
> + port->target_id = target_id;
> + port->port_host = get_device(port_dev);
> + port->component_regs_phys = component_regs_phys;
> +
> + dev = &port->dev;
> + device_initialize(dev);
> + device_set_pm_not_required(dev);
> + dev->parent = &parent_port->dev;
> + dev->bus = &cxl_bus_type;
> + dev->type = &cxl_port_type;
> +
> + return port;
> +
> +err:
> + kfree(port);
> + return ERR_PTR(rc);
> +}
> +
> +/**
> + * devm_cxl_add_port() - add a cxl_port to the topology
> + * @host: devm context / discovery agent
> + * @parent_port: immediate ancestor towards cxl_root
> + * @port_host: PCI or platform-firmware device hosting this port
> + * @target_id: ordinal id relative to other siblings under @parent_port
> + * @component_regs_phys: CXL component register base address
> + */
> +struct cxl_port *devm_cxl_add_port(struct device *host,
> + struct cxl_port *parent_port,
> + struct device *port_host, int target_id,
> + resource_size_t component_regs_phys)
> +{
> + struct cxl_port *port;
> + struct device *dev;
> + int rc;
> +
> + port = cxl_port_alloc(parent_port, port_host, target_id,
> + component_regs_phys);
> + if (IS_ERR(port))
> + return port;
> +
> + dev = &port->dev;
> + rc = dev_set_name(dev, "port%d", port->id);
> + if (rc)
> + goto err;
> +
> + rc = device_add(dev);
> + if (rc)
> + goto err;
> +
> + rc = devm_add_action_or_reset(host, unregister_dev, dev);
> + if (rc)
> + return ERR_PTR(rc);
> +
> + rc = devm_cxl_link_port(host, port);
> + if (rc)
> + return ERR_PTR(rc);
> +
> + return port;
> +
> +err:
> + put_device(dev);
> + return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_GPL(devm_cxl_add_port);
> +
> /**
> * cxl_setup_device_regs() - Detect CXL Device register blocks
> * @dev: Host device of the @base mapping
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 5cd1173151e5..71a991bdacb7 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -134,5 +134,10 @@ struct cxl_address_space_dev *to_cxl_address_space(struct device *dev);
> struct cxl_root *devm_cxl_add_root(struct device *parent,
> struct cxl_address_space *cxl_space,
> int nr_spaces);
> +struct cxl_port *devm_cxl_add_port(struct device *host,
> + struct cxl_port *parent_port,
> + struct device *port_host, int target_id,
> + resource_size_t component_regs_phys);
> +
> extern struct bus_type cxl_bus_type;
> #endif /* __CXL_H__ */
>
On Fri, 7 May 2021 15:51:14 -0700
Dan Williams <[email protected]> wrote:
> Changes since v2 [1]:
> - Add some rationale for moving definitions out of mem.c into mem.h
> (Jonathan)
>
> - Fixup CXL_DEVICE_REGS kernel doc and declare the fixup for the
> struct cxl_mem kernel-doc in the changelog (Jonathan)
>
> - Fixup cxl_setup_device_regs() kernel-doc (Jonathan)
>
> - Cleanup comment in cxl_root_alloc() (Jonathan)
>
> - [not changed] refactor device creation into alloc_and_add()
> anti-pattern.
>
> - Add kernel doc to cxl_address_space_dev_add() to clarify why @host is
> needed as an arg. (Jonathan)
>
> - Describe what the port devices are representing in "cxl/port:
> Introduce cxl_port objects" (Jonathan)
>
> - Explain the rationale for /sys/bus/cxl (Bjorn)
>
> [1]: http://lore.kernel.org/r/161728744224.2474040.12854720917440712854.stgit@dwillia2-desk3.amr.corp.intel.com
Hi Dan,
What's the base for this series? Given it was nearly ready to go
(as far as I'm concerned anyway), I thought I'd give it a spin but
seems it doesn't have some changes from fixes that have gone upstream.
Thanks,
Jonathan
>
> ---
>
> Plans for v5.14:
>
> This series is a starting point for the persistent memory and dynamic
> HDM decoder manipulation support targeting the v5.14 kernel. Among the
> tasks to complete in that timeframe are:
>
> - Region creation including CXL label support
> - LIBNVDIMM integration for surfacing /dev/pmemX and /dev/daxX.Y devices
> on CXL resources
> - HDM decoder enumeration and programming for setting up PMEM mappings
> alongside any "System RAM" mappings established by platform firmware
> - CDAT-DOE support in support of dynamically created NUMA nodes
> - CXL PMEM Shutdown semantics (global persistence flush coordination)
>
> Contributions to cxl.git targeting the next merge window require a
> non-author Reviewed-by. A patch with a Reviewed-by, no outstanding
> objections, and a 3-5 day quiet period on the list is subject to be
> applied to a non-rebasing cxl.git branch and merged into cxl.git/next.
> Contributions targeting a current -rc (fixes) may go in on an expedited
> schedule with only an Acked-by.
>
> ---
>
> CXL Port Topology:
>
> The enumeration starts with the ACPI0017 driver registering a 'struct
> cxl_root' object to establish the top of a cxl_port topology. It then
> scans the ACPI bus looking for ACPI0016 instances. The cxl_root object
> is a singleton* anchor to hang "address-space" objects and be a parent
> device for the downstream 'struct cxl_port' instances. An address-space
> has a 1:1 relationship with a platform defined memory resource range,
> like _CRS for PCIE Host Bridges. Use module parameters to model a
> root-level HDM decoder that all downstream ports further decode, to be
> replaced with a Code First ECN to do the same.
>
> Each address space is modeled as a sysfs object that also shows up in
> /proc/iomem as "CXL Address Space". That iomem resource is functionally
> equivalent to the root-level 'PCI Bus' resources for PCIE.mmio while
> 'CXL Address Space' indicates space for CXL.mem to be mapped. "System
> RAM" and "Persistent Memory", when mapped by HDM decoders, will appear
> as child CXL.mem resources.
>
> Once a 'struct cxl_root' is established the host bridge is modeled as 1
> upstream 'struct cxl_port' and N downstream 'struct cxl_port' instances
> (one per Root Port), just like a PCIE switch. The host-bridge upstream
> port optionally has the HDM decoder registers from the CHBCR if the
> host-bridge has multiple PCIE/CXL root ports. Single-ported host bridges
> will not have HDM decoders in the CHBCR space (see CHBCR note in
> 8.2.5.12 CXL HDM Decoder Capability Structure), but the 'struct
> cxl_port' object is still needed to represent other CXL capabilities and
> access port-specific component registers outside of HDM decoders.
>
> Each 'struct cxl_port' has a 'target_id' attribute that answers the
> question "what port am I in my upstream port's HDM decoder target
> list?". For the host-bridge struct cxl_port, the first tier of ports
> below cxl_root.port, the id is derived from the ordinal mapping of the
> ACPI0016 id (instance id, _UID, or other handle TBD), for all other
> ports the id is the PCIE Root Port ID from the Link Capabilities
> register [1]. The mapping of ordinal port identifiers relative to their
> parent may change once libcxl and cxl-cli prove out region creation, or
> a better option is found to establish a static device path / persistent
> naming scheme. System software must not assume that 'struct cxl_port'
> device names will be static from one boot to the next.
>
> See patch7 for a tree(1) topology picture of what QEMU is producing
> today with this enabling.
>
> * cxl_root is singleton only by convention. A given cxl_root could
> represent 1 to N address spaces, this patch set chooses to implement 1
> cxl_root for all address spaces.
>
> [1]: CXL 2.0 8.2.5.12.8 CXL HDM Decoder 0 Target List Low Register
> (Offset 24h) ...The Target Port Identifier for a given Downstream Port
> is reported via Port Number field in Link Capabilities Register. (See
> PCI Express Base Specification).
>
> ---
>
> Dan Williams (8):
> cxl/mem: Move some definitions to mem.h
> cxl/mem: Introduce 'struct cxl_regs' for "composable" CXL devices
> cxl/core: Rename bus.c to core.c
> cxl/core: Refactor CXL register lookup for bridge reuse
> cxl/acpi: Introduce ACPI0017 driver and cxl_root
> cxl/Kconfig: Default drivers to CONFIG_CXL_BUS
> cxl/port: Introduce cxl_port objects
> cxl/acpi: Add module parameters to stand in for ACPI tables
>
>
> Documentation/driver-api/cxl/memory-devices.rst | 6
> drivers/cxl/Kconfig | 16 +
> drivers/cxl/Makefile | 6
> drivers/cxl/acpi.c | 215 +++++++++
> drivers/cxl/bus.c | 29 -
> drivers/cxl/core.c | 561 +++++++++++++++++++++++
> drivers/cxl/cxl.h | 148 ++++--
> drivers/cxl/mem.c | 97 +---
> drivers/cxl/mem.h | 82 +++
> 9 files changed, 999 insertions(+), 161 deletions(-)
> create mode 100644 drivers/cxl/acpi.c
> delete mode 100644 drivers/cxl/bus.c
> create mode 100644 drivers/cxl/core.c
> create mode 100644 drivers/cxl/mem.h
>
> base-commit: a38fd8748464831584a19438cbb3082b5a2dab15
On Mon, May 10, 2021 at 10:24 AM Jonathan Cameron
<[email protected]> wrote:
>
> On Fri, 7 May 2021 15:51:14 -0700
> Dan Williams <[email protected]> wrote:
>
> > Changes since v2 [1]:
> > - Add some rationale for moving definitions out of mem.c into mem.h
> > (Jonathan)
> >
> > - Fixup CXL_DEVICE_REGS kernel doc and declare the fixup for the
> > struct cxl_mem kernel-doc in the changelog (Jonathan)
> >
> > - Fixup cxl_setup_device_regs() kernel-doc (Jonathan)
> >
> > - Cleanup comment in cxl_root_alloc() (Jonathan)
> >
> > - [not changed] refactor device creation into alloc_and_add()
> > anti-pattern.
> >
> > - Add kernel doc to cxl_address_space_dev_add() to clarify why @host is
> > needed as an arg. (Jonathan)
> >
> > - Describe what the port devices are representing in "cxl/port:
> > Introduce cxl_port objects" (Jonathan)
> >
> > - Explain the rationale for /sys/bus/cxl (Bjorn)
> >
> > [1]: http://lore.kernel.org/r/161728744224.2474040.12854720917440712854.stgit@dwillia2-desk3.amr.corp.intel.com
>
> Hi Dan,
>
> What's the base for this series? Given it was nearly ready to go
> (as far as I'm concerned anyway), I thought I'd give it a spin but
> seems it doesn't have some changes from fixes that have gone upstream.
I did fail to rebase, will rectify, but I did not fail to include a
base-commit entry at the bottom of this cover letter. Will resend
based on -rc1 now that it is out.
On Mon, May 10, 2021 at 7:58 AM Jonathan Cameron
<[email protected]> wrote:
>
> On Fri, 7 May 2021 15:51:47 -0700
> Dan Williams <[email protected]> wrote:
>
> > While CXL builds upon the PCI software model for dynamic enumeration and
> > control, a static platform component is required to bootstrap the CXL
> > memory layout. In addition to identifying the host bridges ACPI is
> > responsible for enumerating the CXL memory space that can be addressed
> > by decoders. This is similar to the requirement for ACPI to publish
> > resources reported by _CRS for PCI host bridges.
> >
> > Introduce the cxl_root object as an abstract "port" into the CXL.mem
> > address space described by HDM decoders identified by the ACPI
> > CEDT.CHBS.
> >
> > For now just establish the initial boilerplate and sysfs attributes, to
> > be followed by enumeration of the ports within the host bridge.
> >
> > Note the allocation of CXL core device objects is split into separate
> > alloc and add steps in order to separate the alloc error path (kfree())
> > from the device add error path (put_device()).
> >
> > Signed-off-by: Dan Williams <[email protected]>
>
> Hi Dan
>
> Just one bit in here that confused me (assuming I'm reading the code correctly).
> You have is_visible for the dev_attr_supports_pmem etc to only show them if
> the particular space supports that memory type. That's fine. You also have
> the actual sysfs function checking the same flag to decide to return "1" or "0"
> which would also be fine, but in combination it's rather odd as the sysfs
> read function can never return "0" (sysfs attribute isn't visible in that
> condition). Probably deserves at least a comment.
Ok.
That was deliberate since it's trivial to code and allows the
visibility policy to change without needing to go audit the attributes
that assumed invisibility. However, yes, it deserves a comment to save
brain cycles with that "hmm, that's odd" in the future.
> This also needs some documentation for the new sysfs ABI
> (Documentation/ABI/...) but that can be in a separate patch.
True.
>
> Otherwise looks good to me.
Thanks.
On Mon, May 10, 2021 at 8:23 AM Jonathan Cameron
<[email protected]> wrote:
>
> On Fri, 7 May 2021 15:51:58 -0700
> Dan Williams <[email protected]> wrote:
>
> > Once the cxl_root is established then other ports in the hierarchy can
> > be attached. The cxl_port object, unlike cxl_root that is associated
> > with host bridges, is associated with PCIe Root Ports or PCIe Switch
> > Ports. Add cxl_port instances for all PCIe Root Ports in an ACPI0016
> > host bridge. The cxl_port instances for PCIe Switch Ports are not
> > included here as those are to be modeled as another service device
> > registered on the pcie_port_bus_type.
> >
> > A sample sysfs topology for a single-host-bridge with
> > single-PCIe/CXL-port follows:
> >
> > /sys/bus/cxl/devices/root0
> > ├── address_space0
> > │ ├── devtype
> > │ ├── end
> > │ ├── start
> > │ ├── supports_ram
> > │ ├── supports_type2
> > │ ├── supports_type3
> > │ └── uevent
> > ├── address_space1
> > │ ├── devtype
> > │ ├── end
> > │ ├── start
> > │ ├── supports_pmem
> > │ ├── supports_type2
> > │ ├── supports_type3
> > │ └── uevent
> > ├── devtype
> > ├── port1
> > │ ├── devtype
> > │ ├── host -> ../../../../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
> > │ ├── port2
> > │ │ ├── devtype
> > │ │ ├── host -> ../../../../../pci0000:34/0000:34:00.0
> > │ │ ├── subsystem -> ../../../../../../bus/cxl
> > │ │ ├── target_id
> > │ │ └── uevent
> > │ ├── subsystem -> ../../../../../bus/cxl
> > │ ├── target_id
> > │ └── uevent
> > ├── subsystem -> ../../../../bus/cxl
> > ├── target_id
> > └── uevent
> >
> > In this listing the system-wide-singleton root0 has 2 address spaces, 1
> > PMEM and 1 RAM. Those address spaces are accessed through port1 which
> > represents the upstream port of an ACPI0016 host-bridge. A
> > multi-host-bridge system would have other ports as peers to port1 to
> > additionally decode root level address spaces. Port2 in this diagram
> > represents the single downstream port of the host-bridge. Were it to be
> > a multi-ported-host-bridge there would be peers / siblings of port2 with
> > port1 as their common ancestor.
>
> I guess it would be a pain to emulate a system that actually had
> multiple ports at the last level. Pity as would have made your
> explanation here a little easier to follow.
>
A pain in QEMU, but maybe not with a mocked implementation similar to
what gets injected for the nvdimm "nfit_test". I'll take a look.
> > The rationale for this port hierarchy is to be able to walk the HDM
> > decoder register sets that each port implements. Additionally it
> > provides a representation of host-bridge interleave which will be
> > necessary for follow-on work that adds CXL region devices.
> >
> > The details in the /sys/bus/cxl hierarchy that are not suitable to be
> > represented in the /sys/bus/pci hierarchy are:
> > - memory address spaces that are interleaved across host bridges
> > - common sub-device functionality represented by CXL component + device
> > registers (enumerated via DVSEC or platform firmware (ACPI CEDT)).
>
> I'm sold :)
Thanks for the vote of confidence. It sounded like Bjorn was sold too
at the end of our last thread... I'll Cc him on this patch directly in
the resend.
>
> >
> > Signed-off-by: Dan Williams <[email protected]>
>
> Reviewed-by: Jonathan Cameron <[email protected]>
Thanks for the review Jonathan, appreciate it.