Changes since v1 [1]:
- Add stronger rationale for the 'struct cxl_regs' arrangement that
accommodates devices that compose register blocks mixing "component",
"device", and "memory device" register sets. (Christoph)
- Reorganize root, address space, and port device creation into separate
'alloc' and 'add' steps.
- Rename "upstream_port" to "port_host". This is the PCI or ACPI device
that implements the CXL port capability.
- Rename @parent to @host in init routines to clarify the device that is
the context for devm from the device-model parent of the device being
created.
- Rename port.cxl_regs_phys to port.component_regs_phys
- Add initialization for
cxl_root.port.{target_id,component_regs_phys,port_host}, where
target_id and component_regs_phys are set to invalid values (do not
apply to cxl_root as the first port under root is the first actual
device in the system and port_host is the ACPI0017 device.
[1]: http://lore.kernel.org/r/161662142382.1723715.5934723983022398253.stgit@dwillia2-desk3.amr.corp.intel.com
---
This series is a preview of the proposed infrastructure for enabling
dynamic mapping of Host-managed Device Memory (HDM) Decoders. It
includes a not-for-upstream hack at the tail of the series to stand-in
for the in-flight ACPICA enabling. The goal is to get review of the
proposal in parallel with other in-flight dependencies. The next step
after this is to add dynamic enumeration and assignment of HDM Decoders
in coordination with per-cxl_port driver instances.
---
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 | 553 +++++++++++++++++++++++
drivers/cxl/cxl.h | 146 ++++--
drivers/cxl/mem.c | 97 +---
drivers/cxl/mem.h | 82 +++
9 files changed, 990 insertions(+), 160 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
In preparation for more generic shared functionality across endpoint
consumers of core cxl resources, and platform-firmware producers of
those resources, rename bus.c to core.c. In addition to the central
rendezvous for interleave coordination, the core will also define common
routines like CXL register block mapping.
Acked-by: Ben Widawsky <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
Documentation/driver-api/cxl/memory-devices.rst | 6 ++---
drivers/cxl/Makefile | 4 ++-
drivers/cxl/bus.c | 29 ----------------------
drivers/cxl/core.c | 30 +++++++++++++++++++++++
4 files changed, 35 insertions(+), 34 deletions(-)
delete mode 100644 drivers/cxl/bus.c
create mode 100644 drivers/cxl/core.c
diff --git a/Documentation/driver-api/cxl/memory-devices.rst b/Documentation/driver-api/cxl/memory-devices.rst
index 1bad466f9167..71495ed77069 100644
--- a/Documentation/driver-api/cxl/memory-devices.rst
+++ b/Documentation/driver-api/cxl/memory-devices.rst
@@ -28,10 +28,10 @@ CXL Memory Device
.. kernel-doc:: drivers/cxl/mem.c
:internal:
-CXL Bus
+CXL Core
-------
-.. kernel-doc:: drivers/cxl/bus.c
- :doc: cxl bus
+.. kernel-doc:: drivers/cxl/core.c
+ :doc: cxl core
External Interfaces
===================
diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index a314a1891f4d..3808e39dd31f 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_CXL_BUS) += cxl_bus.o
+obj-$(CONFIG_CXL_BUS) += cxl_core.o
obj-$(CONFIG_CXL_MEM) += cxl_mem.o
ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL
-cxl_bus-y := bus.o
+cxl_core-y := core.o
cxl_mem-y := mem.o
diff --git a/drivers/cxl/bus.c b/drivers/cxl/bus.c
deleted file mode 100644
index 58f74796d525..000000000000
--- a/drivers/cxl/bus.c
+++ /dev/null
@@ -1,29 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright(c) 2020 Intel Corporation. All rights reserved. */
-#include <linux/device.h>
-#include <linux/module.h>
-
-/**
- * DOC: cxl bus
- *
- * The CXL bus provides namespace for control devices and a rendezvous
- * point for cross-device interleave coordination.
- */
-struct bus_type cxl_bus_type = {
- .name = "cxl",
-};
-EXPORT_SYMBOL_GPL(cxl_bus_type);
-
-static __init int cxl_bus_init(void)
-{
- return bus_register(&cxl_bus_type);
-}
-
-static void cxl_bus_exit(void)
-{
- bus_unregister(&cxl_bus_type);
-}
-
-module_init(cxl_bus_init);
-module_exit(cxl_bus_exit);
-MODULE_LICENSE("GPL v2");
diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
new file mode 100644
index 000000000000..7f8d2034038a
--- /dev/null
+++ b/drivers/cxl/core.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2020 Intel Corporation. All rights reserved. */
+#include <linux/device.h>
+#include <linux/module.h>
+
+/**
+ * DOC: cxl core
+ *
+ * The CXL core provides a sysfs hierarchy for control devices and a rendezvous
+ * point for cross-device interleave coordination through cxl ports.
+ */
+
+struct bus_type cxl_bus_type = {
+ .name = "cxl",
+};
+EXPORT_SYMBOL_GPL(cxl_bus_type);
+
+static __init int cxl_core_init(void)
+{
+ return bus_register(&cxl_bus_type);
+}
+
+static void cxl_core_exit(void)
+{
+ bus_unregister(&cxl_bus_type);
+}
+
+module_init(cxl_core_init);
+module_exit(cxl_core_exit);
+MODULE_LICENSE("GPL v2");
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.
Signed-off-by: Dan Williams <[email protected]>
---
drivers/cxl/Kconfig | 14 ++
drivers/cxl/Makefile | 2
drivers/cxl/acpi.c | 39 ++++++
drivers/cxl/core.c | 349 ++++++++++++++++++++++++++++++++++++++++++++++++++
drivers/cxl/cxl.h | 64 +++++++++
5 files changed, 468 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 2ab467ef9909..46c3b2588d2f 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -2,6 +2,8 @@
/* Copyright(c) 2020-2021 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,353 @@
* 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);
+}
+
+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 cbd29650c4e2..559f8343fee4 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -70,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__ */
Hi Dan,
I love your patch! Yet something to improve:
[auto build test ERROR on a38fd8748464831584a19438cbb3082b5a2dab15]
url: https://github.com/0day-ci/linux/commits/Dan-Williams/CXL-Port-Enumeration/20210402-025333
base: a38fd8748464831584a19438cbb3082b5a2dab15
config: ia64-randconfig-r016-20210401 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/84c25f52c3c7f8f20988e98eb9947c4eace11927
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Dan-Williams/CXL-Port-Enumeration/20210402-025333
git checkout 84c25f52c3c7f8f20988e98eb9947c4eace11927
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
In file included from drivers/cxl/core.c:7:
>> drivers/cxl/cxl.h:84:15: error: field 'range' has incomplete type
84 | struct range range;
| ^~~~~
drivers/cxl/core.c: In function 'devm_cxl_add_root':
>> drivers/cxl/core.c:343:8: error: implicit declaration of function 'range_len' [-Werror=implicit-function-declaration]
343 | if (!range_len(&space->range))
| ^~~~~~~~~
drivers/cxl/core.c: In function 'end_show':
drivers/cxl/core.c:56:1: error: control reaches end of non-void function [-Werror=return-type]
56 | }
| ^
drivers/cxl/core.c: In function 'start_show':
drivers/cxl/core.c:47:1: error: control reaches end of non-void function [-Werror=return-type]
47 | }
| ^
cc1: some warnings being treated as errors
--
In file included from drivers/cxl/acpi.c:8:
>> drivers/cxl/cxl.h:84:15: error: field 'range' has incomplete type
84 | struct range range;
| ^~~~~
vim +/range +84 drivers/cxl/cxl.h
82
83 struct cxl_address_space {
> 84 struct range range;
85 int interleave_size;
86 unsigned long flags;
87 unsigned long targets;
88 };
89
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Thu, 1 Apr 2021 07:31:09 -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.
>
> Signed-off-by: Dan Williams <[email protected]>
A few minor comments inline.
> ---
> drivers/cxl/Kconfig | 14 ++
> drivers/cxl/Makefile | 2
> drivers/cxl/acpi.c | 39 ++++++
> drivers/cxl/core.c | 349 ++++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/cxl/cxl.h | 64 +++++++++
> 5 files changed, 468 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>
swap acpi.h that for mod_devicetable.h unless this is going to
need acpi.h later for something else.
> +#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 2ab467ef9909..46c3b2588d2f 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -2,6 +2,8 @@
> /* Copyright(c) 2020-2021 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,353 @@
> * 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
have any corresponding component registers; it is only a
.. or you could use two sentences
> + * 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);
> +}
> +
> +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);
This is unusual. The error handling is undoing something this function
wasn't responsible for. See below for suggested resolution.
> + 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);
> +
Nothing is done between the dev_alloc() and the dev_add()
and this is currently in the odd position of doing put_device() in the
error path of *dev_add() when it wasn't responsible for getting the
reference it is putting, dev_alloc() did that.
That suggests to me that we can clean up the oddity by just combining
cxl_address_space_dev_alloc() and cxl_adress_space_dev_add() into one
alloc_and_add() function (with a better name)
> + rc = cxl_address_space_dev_add(host, cxl_asd, i);
Lifetime management here seems overly complex. Why not use host for both
the alloc and add() devm calls? I guess there is a good reason
though so good to have a comment here saying what it is.
> + 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 cbd29650c4e2..559f8343fee4 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -70,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 Tue, Apr 6, 2021 at 10:47 AM Jonathan Cameron
<[email protected]> wrote:
>
> On Thu, 1 Apr 2021 07:31:09 -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.
> >
> > Signed-off-by: Dan Williams <[email protected]>
>
> A few minor comments inline.
>
> > ---
> > drivers/cxl/Kconfig | 14 ++
> > drivers/cxl/Makefile | 2
> > drivers/cxl/acpi.c | 39 ++++++
> > drivers/cxl/core.c | 349 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > drivers/cxl/cxl.h | 64 +++++++++
> > 5 files changed, 468 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>
>
> swap acpi.h that for mod_devicetable.h unless this is going to
> need acpi.h later for something else.
It will need it after patch7, so I'll just leave it as is for now.
[..]
> > +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
>
> have any corresponding component registers; it is only a
> .. or you could use two sentences
Sure.
>
> > + * 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);
> > +}
> > +
> > +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);
> This is unusual. The error handling is undoing something this function
> wasn't responsible for. See below for suggested resolution.
>
> > + 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);
> > +
>
> Nothing is done between the dev_alloc() and the dev_add()
> and this is currently in the odd position of doing put_device() in the
> error path of *dev_add() when it wasn't responsible for getting the
> reference it is putting, dev_alloc() did that.
No, you missed the back and forth that Jason and I had about proper
device initialization flows:
https://lore.kernel.org/linux-cxl/161714738634.2168142.10860201861152789544.stgit@dwillia2-desk3.amr.corp.intel.com/
The put_device() is not undoing the dev_alloc(), it is undoing the
dev_alloc() + follow on allocations. Specifically it undoes the
dev_set_name() allocation. That is why the alloc and the add are split
into explicit code paths where the recovery shifts from alloc-unwind
to put_device().
> That suggests to me that we can clean up the oddity by just combining
> cxl_address_space_dev_alloc() and cxl_adress_space_dev_add() into one
>
> alloc_and_add() function (with a better name)
It's not an oddity and alloc_and_add() is an anti-pattern that leads to bugs.
>
> > + rc = cxl_address_space_dev_add(host, cxl_asd, i);
>
> Lifetime management here seems overly complex. Why not use host for both
> the alloc and add() devm calls? I guess there is a good reason
> though so good to have a comment here saying what it is.
I'll add a kernel-doc to cxl_address_space_dev_add() to clarify what
is happening here.