[PATCH 00/13] CXL Region driver

[PATCH 00/13] CXL Region driver

[Ben Widawsky]

This patch series introduces the CXL region driver as well as associated APIs in
CXL core. The region driver enables the creation of "regions" which is a concept
defined by the CXL 2.0 specification [1]. Region verification and programming
state are owned by the cxl_region driver (implemented in the cxl_region module).
It relies on cxl_mem to determine if devices are CXL routed, and cxl_port to
actually handle the programming of the HDM decoders. Much of the region driver
is an implementation of algorithms described in the CXL Type 3 Memory Device
Software Guide [2].

The region driver will be responsible for configuring regions found on
persistent capacities in the Label Storage Area (LSA), it will also enumerate
regions configured by BIOS, usually volatile capacities, and will allow for
dynamic region creation (which can then be stored in the LSA). It is the primary
consumer of the CXL Port [3] and CXL Mem drivers introduced previously [4]. Dan
as reworked some of this which is required for this branch. A cached copy is
included in the gitlab for this project [5]. Those patches will be posted
shortly.

The patches for the region driver could be squashed. They're broken out to aid
review and because that's the order they were implemented in. My preference is
to keep those as they are.

Some things are still missing and will be worked on while these are reviewed (in
priority order):
1. Connection to libnvdimm labels (No plan)
2. Volatile regions/LSA regions (Have a plan)
3. Switch ports (Have a plan)
4. Decoder programming restrictions (No plan). The one know restriction I've
   missed is to disallow programming HDM decoders that aren't in incremental
   system physical address ranges.
5. Stress testing

Here is an example of output when programming a x2 interleave region:
./cxlctl create-region -i2 -n -a -s $((512<<20)) /sys/bus/cxl/devices/decoder0.0
[   42.971496][  T644] cxl_core:cxl_bus_probe:1384: cxl_region region0.0:0: probe: -19
[   42.972400][  T644] cxl_core:cxl_add_region:478: cxl region0.0:0: Added region0.0:0 to decoder0.0
[   42.979388][  T644] cxl_core:cxl_commit_decoder:394: cxl_port port1: decoder1.0
[   42.979388][  T644] 	Base 0x0000004c00000000
[   42.979388][  T644] 	Size 536870912
[   42.979388][  T644] 	IG 8
[   42.979388][  T644] 	IW 2
[   42.979388][  T644] 	TargetList: 0 1 -1 -1 -1 -1 -1 -1
[   42.982410][  T644] cxl_core:cxl_commit_decoder:394: cxl_port endpoint3: decoder3.0
[   42.982410][  T644] 	Base 0x0000004c00000000
[   42.982410][  T644] 	Size 536870912
[   42.982410][  T644] 	IG 8
[   42.982410][  T644] 	IW 2
[   42.982410][  T644] 	TargetList: -1 -1 -1 -1 -1 -1 -1 -1
[   42.985427][  T644] cxl_core:cxl_commit_decoder:394: cxl_port endpoint2: decoder2.0
[   42.985427][  T644] 	Base 0x0000004c00000000
[   42.985427][  T644] 	Size 536870912
[   42.985427][  T644] 	IG 8
[   42.985427][  T644] 	IW 2
[   42.985427][  T644] 	TargetList: -1 -1 -1 -1 -1 -1 -1 -1
[   42.987937][  T644] cxl_core:cxl_bus_probe:1384: cxl_region region0.0:0: probe: 0

If you're wondering how I tested this, I've baked it into my cxlctl app [6] and
lib [7]. Eventually this will get absorbed by ndctl/cxl-cli/libcxl.

To get the detailed errors, trace-cmd can be utilized. Until a region device
exists, the region module will not be loaded, which means the region tracepoints
will not exist. To get around this, modprobe cxl_region before anything.

trace-cmd record -e cxl ./cxlctl create-region -n -a -s $((256<<20)) /sys/bus/cxl/devices/decoder0.0

Branch can be found at gitlab [8].

---

[1]: https://www.computeexpresslink.org/download-the-specification
[2]: https://cdrdv2.intel.com/v1/dl/getContent/643805?wapkw=CXL%20memory%20device%20sw%20guide
[3]: https://lore.kernel.org/linux-cxl/20211022183709.1199701-9-ben.widawsky@intel.com/
[4]: https://lore.kernel.org/linux-cxl/20211022183709.1199701-17-ben.widawsky@intel.com/
[5]: https://gitlab.com/bwidawsk/linux/-/commit/126793e22427f7975a8f2fca373764be78012e88
[6]: https://gitlab.com/bwidawsk-cxl/cxlctl
[7]: https://gitlab.com/bwidawsk-cxl/cxl_rs
[8]: https://gitlab.com/bwidawsk/linux/-/tree/cxl_region

---

Ben Widawsky (13):
  cxl/core: Rename find_cxl_port
  cxl/core: Track port depth
  cxl/region: Add region creation ABI
  cxl/region: Introduce concept of region configuration
  cxl/mem: Cache port created by the mem dev
  cxl/region: Introduce a cxl_region driver
  cxl/acpi: Handle address space allocation
  cxl/region: Address space allocation
  cxl/region: Implement XHB verification
  cxl/region: HB port config verification
  cxl/region: Add infrastructure for decoder programming
  cxl/region: Record host bridge target list
  cxl: Program decoders for regions

 .clang-format                                 |   3 +
 Documentation/ABI/testing/sysfs-bus-cxl       |  63 ++
 .../driver-api/cxl/memory-devices.rst         |  14 +
 drivers/cxl/Makefile                          |   2 +
 drivers/cxl/acpi.c                            |  30 +
 drivers/cxl/core/Makefile                     |   1 +
 drivers/cxl/core/core.h                       |   4 +
 drivers/cxl/core/hdm.c                        | 198 +++++
 drivers/cxl/core/port.c                       | 132 +++-
 drivers/cxl/core/region.c                     | 525 +++++++++++++
 drivers/cxl/cxl.h                             |  32 +
 drivers/cxl/cxlmem.h                          |   9 +
 drivers/cxl/mem.c                             |  16 +-
 drivers/cxl/port.c                            |  42 +-
 drivers/cxl/region.c                          | 695 ++++++++++++++++++
 drivers/cxl/region.h                          |  47 ++
 drivers/cxl/trace.h                           |  54 ++
 tools/testing/cxl/Kbuild                      |   1 +
 18 files changed, 1849 insertions(+), 19 deletions(-)
 create mode 100644 drivers/cxl/core/region.c
 create mode 100644 drivers/cxl/region.c
 create mode 100644 drivers/cxl/region.h
 create mode 100644 drivers/cxl/trace.h


base-commit: 03716ce2db3c17ba38f26a88d75049c0472a629e
prerequisite-patch-id: af6a0315e22bfc1099d4f58610b8b897e6e5a060
-- 
2.34.1

[PATCH 01/13] cxl/core: Rename find_cxl_port

[Ben Widawsky]

Needed for other things.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/core/port.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 29b0722dc6eb..5a1ffadd5d0d 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -666,7 +666,7 @@ static int match_port_by_dport(struct device *dev, const void *data)
 	return cxl_find_dport_by_dev(port, data) != NULL;
 }
 
-static struct cxl_port *find_cxl_port(struct device *dport_dev)
+static struct cxl_port *dport_find_cxl_port(struct device *dport_dev)
 {
 	struct device *port_dev;
 
@@ -699,7 +699,7 @@ struct cxl_port *find_cxl_root(struct cxl_memdev *cxlmd)
 		if (!dport_dev)
 			break;
 
-		port = find_cxl_port(dport_dev);
+		port = dport_find_cxl_port(dport_dev);
 		if (!port)
 			continue;
 
@@ -728,7 +728,7 @@ static void cxl_remove_ep(void *data)
 		if (!dport_dev)
 			break;
 
-		port = find_cxl_port(dport_dev);
+		port = dport_find_cxl_port(dport_dev);
 		if (!port || is_cxl_root(port))
 			continue;
 
@@ -787,7 +787,7 @@ static int add_port_register_ep(struct cxl_memdev *cxlmd,
 	resource_size_t component_reg_phys;
 	int rc;
 
-	parent_port = find_cxl_port(grandparent(dport_dev));
+	parent_port = dport_find_cxl_port(grandparent(dport_dev));
 	if (!parent_port) {
 		/*
 		 * The root CXL port is added by the CXL platform driver, fail
@@ -811,7 +811,7 @@ static int add_port_register_ep(struct cxl_memdev *cxlmd,
 		goto out;
 	}
 
-	port = find_cxl_port(dport_dev);
+	port = dport_find_cxl_port(dport_dev);
 	if (!port) {
 		component_reg_phys = find_component_registers(uport_dev);
 		port = devm_cxl_add_port(&parent_port->dev, uport_dev,
@@ -876,7 +876,7 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 			break;
 		}
 
-		port = find_cxl_port(dport_dev);
+		port = dport_find_cxl_port(dport_dev);
 		if (port) {
 			dev_dbg(&cxlmd->dev,
 				"found already registered port %s:%s\n",
@@ -922,7 +922,7 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_ports, CXL);
 
 struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd)
 {
-	return find_cxl_port(grandparent(&cxlmd->dev));
+	return dport_find_cxl_port(grandparent(&cxlmd->dev));
 }
 EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL);
 
-- 
2.34.1

[PATCH 02/13] cxl/core: Track port depth

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/core/port.c | 7 ++++++-
 drivers/cxl/cxl.h       | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 5a1ffadd5d0d..ecab7cfa88f0 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -436,13 +436,18 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
 {
 	struct cxl_port *port;
 	struct device *dev;
-	int rc;
+	int rc, depth = parent_port ? parent_port->depth + 1 : 0;
 
 	port = cxl_port_alloc(uport, component_reg_phys, parent_port);
 	if (IS_ERR(port))
 		return port;
 
+	if (dev_WARN_ONCE(&port->dev, parent_port && !depth,
+			  "Invalid parent port depth\n"))
+		return ERR_PTR(-ENODEV);
+
 	port->host = host;
+	port->depth = depth;
 	dev = &port->dev;
 	if (is_cxl_memdev(uport))
 		rc = dev_set_name(dev, "endpoint%d", port->id);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 276b93316e7f..6eeb82711443 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -272,6 +272,7 @@ struct cxl_walk_context {
  * @decoder_ida: allocator for decoder ids
  * @component_reg_phys: component register capability base address (optional)
  * @dead: last ep has been removed, force port re-creation
+ * @depth: How deep this port is relative to the root. depth 0 is the root.
  */
 struct cxl_port {
 	struct device dev;
@@ -283,6 +284,7 @@ struct cxl_port {
 	struct ida decoder_ida;
 	resource_size_t component_reg_phys;
 	bool dead;
+	unsigned int depth;
 };
 
 /**
-- 
2.34.1

[PATCH 03/13] cxl/region: Add region creation ABI

[Ben Widawsky]

Regions are created as a child of the decoder that encompasses an
address space with constraints. Regions have a number of attributes that
must be configured before the region can be activated.

The ABI is not meant to be secure, but is meant to avoid accidental
races. As a result, a buggy process may create a region by name that was
allocated by a different process. However, multiple processes which are
trying not to race with each other shouldn't need special
synchronization to do so.

// Allocate a new region name
region=$(cat /sys/bus/cxl/devices/decoder0.0/create_region)

// Create a new region by name
echo $region > /sys/bus/cxl/devices/decoder0.0/create_region

// Region now exists in sysfs
stat -t /sys/bus/cxl/devices/decoder0.0/$region

// Delete the region, and name
echo $region > /sys/bus/cxl/devices/decoder0.0/delete_region

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-cxl       |  23 ++
 .../driver-api/cxl/memory-devices.rst         |  11 +
 drivers/cxl/core/Makefile                     |   1 +
 drivers/cxl/core/core.h                       |   3 +
 drivers/cxl/core/port.c                       |  16 ++
 drivers/cxl/core/region.c                     | 209 ++++++++++++++++++
 drivers/cxl/cxl.h                             |   9 +
 drivers/cxl/region.h                          |  38 ++++
 tools/testing/cxl/Kbuild                      |   1 +
 9 files changed, 311 insertions(+)
 create mode 100644 drivers/cxl/core/region.c
 create mode 100644 drivers/cxl/region.h

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 498ae288e143..0fbdd8613654 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -136,3 +136,26 @@ Description:
 		memory (type-3). The 'target_type' attribute indicates the
 		current setting which may dynamically change based on what
 		memory regions are activated in this decode hierarchy.
+
+What:		/sys/bus/cxl/devices/decoderX.Y/create_region
+Date:		November, 2021
+KernelVersion:	v5.17
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		Creates a new CXL region. Writing a value of the form
+		"regionX.Y:Z" will create a new uninitialized region that will
+		be mapped by the CXL decoderX.Y. Reading from this node will
+		return a newly allocated region name. In order to create a
+		region (writing) you must use a value returned from reading the
+		node. Regions must be subsequently configured and bound to a
+		region driver before they can be used.
+
+What:		/sys/bus/cxl/devices/decoderX.Y/delete_region
+Date:		November, 2021
+KernelVersion:	v5.17
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		Deletes the named region. A region must be unbound from the
+		region driver before being deleted. The attributes expects a
+		region in the form "regionX.Y:Z". The region's name, allocated
+		by reading create_region, will also be released.
diff --git a/Documentation/driver-api/cxl/memory-devices.rst b/Documentation/driver-api/cxl/memory-devices.rst
index e101ef02b547..dc756ed23a3a 100644
--- a/Documentation/driver-api/cxl/memory-devices.rst
+++ b/Documentation/driver-api/cxl/memory-devices.rst
@@ -71,6 +71,17 @@ CXL Core
 .. kernel-doc:: drivers/cxl/core/mbox.c
    :doc: cxl mbox
 
+CXL Regions
+-----------
+.. kernel-doc:: drivers/cxl/region.h
+   :identifiers:
+
+.. kernel-doc:: drivers/cxl/core/region.c
+   :doc: cxl core region
+
+.. kernel-doc:: drivers/cxl/core/region.c
+   :identifiers:
+
 External Interfaces
 ===================
 
diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
index 6d37cd78b151..39ce8f2f2373 100644
--- a/drivers/cxl/core/Makefile
+++ b/drivers/cxl/core/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_CXL_BUS) += cxl_core.o
 ccflags-y += -I$(srctree)/drivers/cxl
 cxl_core-y := port.o
 cxl_core-y += pmem.o
+cxl_core-y += region.o
 cxl_core-y += regs.o
 cxl_core-y += memdev.o
 cxl_core-y += mbox.o
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 19d1f9d8ceba..1d4d1699b479 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -10,6 +10,9 @@ extern const struct device_type cxl_memdev_type;
 
 extern struct attribute_group cxl_base_attribute_group;
 
+extern struct device_attribute dev_attr_create_region;
+extern struct device_attribute dev_attr_delete_region;
+
 struct cxl_send_command;
 struct cxl_mem_query_commands;
 int cxl_query_cmd(struct cxl_memdev *cxlmd,
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index ecab7cfa88f0..ef3840c50e3e 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -196,6 +196,8 @@ static struct attribute_group cxl_decoder_base_attribute_group = {
 };
 
 static struct attribute *cxl_decoder_root_attrs[] = {
+	&dev_attr_create_region.attr,
+	&dev_attr_delete_region.attr,
 	&dev_attr_cap_pmem.attr,
 	&dev_attr_cap_ram.attr,
 	&dev_attr_cap_type2.attr,
@@ -236,11 +238,23 @@ static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = {
 	NULL,
 };
 
+static int delete_region(struct device *dev, void *arg)
+{
+	struct cxl_decoder *cxld = to_cxl_decoder(dev->parent);
+
+	return cxl_delete_region(cxld, dev_name(dev));
+}
+
 static void cxl_decoder_release(struct device *dev)
 {
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
 	struct cxl_port *port = to_cxl_port(dev->parent);
 
+	device_for_each_child(&cxld->dev, cxld, delete_region);
+
+	dev_WARN_ONCE(dev, !ida_is_empty(&cxld->region_ida),
+		      "Lost track of a region");
+
 	ida_free(&port->decoder_ida, cxld->id);
 	kfree(cxld);
 }
@@ -1021,6 +1035,8 @@ static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
 	cxld->target_type = CXL_DECODER_EXPANDER;
 	cxld->platform_res = (struct resource)DEFINE_RES_MEM(0, 0);
 
+	ida_init(&cxld->region_ida);
+
 	return cxld;
 err:
 	kfree(cxld);
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
new file mode 100644
index 000000000000..e3a82f3c118e
--- /dev/null
+++ b/drivers/cxl/core/region.c
@@ -0,0 +1,209 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/idr.h>
+#include <region.h>
+#include <cxl.h>
+#include "core.h"
+
+/**
+ * DOC: cxl core region
+ *
+ * Regions are managed through the Linux device model. Each region instance is a
+ * unique struct device. CXL core provides functionality to create, destroy, and
+ * configure regions. This is all implemented here. Binding a region
+ * (programming the hardware) is handled by a separate region driver.
+ */
+
+static void cxl_region_release(struct device *dev);
+
+static const struct device_type cxl_region_type = {
+	.name = "cxl_region",
+	.release = cxl_region_release,
+};
+
+static ssize_t create_region_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct cxl_port *port = to_cxl_port(dev->parent);
+	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+	int rc;
+
+	if (dev_WARN_ONCE(dev, !is_root_decoder(dev),
+			  "Invalid decoder selected for region.")) {
+		return -ENODEV;
+	}
+
+	rc = ida_alloc(&cxld->region_ida, GFP_KERNEL);
+	if (rc < 0) {
+		dev_err(&cxld->dev, "Couldn't get a new id\n");
+		return rc;
+	}
+
+	return sysfs_emit(buf, "region%d.%d:%d\n", port->id, cxld->id, rc);
+}
+
+static ssize_t create_region_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t len)
+{
+	struct cxl_port *port = to_cxl_port(dev->parent);
+	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+	int decoder_id, port_id, region_id;
+	struct cxl_region *region;
+	ssize_t rc;
+
+	if (sscanf(buf, "region%d.%d:%d", &port_id, &decoder_id, &region_id) != 3)
+		return -EINVAL;
+
+	if (decoder_id != cxld->id)
+		return -EINVAL;
+
+	if (port_id != port->id)
+		return -EINVAL;
+
+	region = cxl_alloc_region(cxld, region_id);
+	if (IS_ERR(region))
+		return PTR_ERR(region);
+
+	rc = cxl_add_region(cxld, region);
+	if (rc) {
+		kfree(region);
+		return rc;
+	}
+
+	return len;
+}
+DEVICE_ATTR_RW(create_region);
+
+static ssize_t delete_region_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t len)
+{
+	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+	int rc;
+
+	rc = cxl_delete_region(cxld, buf);
+	if (rc)
+		return rc;
+
+	return len;
+}
+DEVICE_ATTR_WO(delete_region);
+
+struct cxl_region *to_cxl_region(struct device *dev)
+{
+	if (dev_WARN_ONCE(dev, dev->type != &cxl_region_type,
+			  "not a cxl_region device\n"))
+		return NULL;
+
+	return container_of(dev, struct cxl_region, dev);
+}
+EXPORT_SYMBOL_GPL(to_cxl_region);
+
+static void cxl_region_release(struct device *dev)
+{
+	struct cxl_decoder *cxld = to_cxl_decoder(dev->parent);
+	struct cxl_region *region = to_cxl_region(dev);
+
+	ida_free(&cxld->region_ida, region->id);
+	kfree(region);
+}
+
+struct cxl_region *cxl_alloc_region(struct cxl_decoder *cxld, int id)
+{
+	struct cxl_region *region;
+
+	region = kzalloc(sizeof(*region), GFP_KERNEL);
+	if (!region)
+		return ERR_PTR(-ENOMEM);
+
+	region->id = id;
+
+	return region;
+}
+
+/**
+ * cxl_add_region - Adds a region to a decoder
+ * @cxld: Parent decoder.
+ * @region: Region to be added to the decoder.
+ *
+ * This is the second step of region initialization. Regions exist within an
+ * address space which is mapped by a @cxld. That @cxld must be a root decoder,
+ * and it enforces constraints upon the region as it is configured.
+ *
+ * Return: 0 if the region was added to the @cxld, else returns negative error
+ * code. The region will be named "regionX.Y.Z" where X is the port, Y is the
+ * decoder id, and Z is the region number.
+ */
+int cxl_add_region(struct cxl_decoder *cxld, struct cxl_region *region)
+{
+	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
+	struct device *dev = &region->dev;
+	int rc;
+
+	device_initialize(dev);
+	dev->parent = &cxld->dev;
+	device_set_pm_not_required(dev);
+	dev->bus = &cxl_bus_type;
+	dev->type = &cxl_region_type;
+	rc = dev_set_name(dev, "region%d.%d:%d", port->id, cxld->id,
+			  region->id);
+	if (rc)
+		goto err;
+
+	rc = device_add(dev);
+	if (rc)
+		goto err;
+
+	dev_dbg(dev, "Added %s to %s\n", dev_name(dev), dev_name(&cxld->dev));
+
+	return 0;
+
+err:
+	put_device(dev);
+	return rc;
+}
+
+static struct cxl_region *cxl_find_region_by_name(struct cxl_decoder *cxld,
+						  const char *name)
+{
+	struct device *region_dev;
+
+	region_dev = device_find_child_by_name(&cxld->dev, name);
+	if (!region_dev)
+		return ERR_PTR(-ENOENT);
+
+	return to_cxl_region(region_dev);
+}
+
+/**
+ * cxl_delete_region - Deletes a region
+ * @cxld: Parent decoder
+ * @region_name: Named region, ie. regionX.Y:Z
+ */
+int cxl_delete_region(struct cxl_decoder *cxld, const char *region_name)
+{
+	struct cxl_region *region;
+
+	device_lock(&cxld->dev);
+
+	region = cxl_find_region_by_name(cxld, region_name);
+	if (IS_ERR(region)) {
+		device_unlock(&cxld->dev);
+		return PTR_ERR(region);
+	}
+
+	dev_dbg(&cxld->dev, "Requested removal of %s from %s\n",
+		dev_name(&region->dev), dev_name(&cxld->dev));
+
+	device_unregister(&region->dev);
+	device_unlock(&cxld->dev);
+
+	put_device(&region->dev);
+
+	return 0;
+}
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 6eeb82711443..79c5781b6173 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -202,6 +202,7 @@ enum cxl_decoder_type {
  * @interleave_granularity: data stride per dport
  * @target_type: accelerator vs expander (type2 vs type3) selector
  * @flags: memory type capabilities and locking
+ * @region_ida: allocator for region ids.
  * @nr_targets: number of elements in @target
  * @target: active ordered target list in current decoder configuration
  */
@@ -216,6 +217,7 @@ struct cxl_decoder {
 	int interleave_granularity;
 	enum cxl_decoder_type target_type;
 	unsigned long flags;
+	struct ida region_ida;
 	const int nr_targets;
 	struct cxl_dport *target[];
 };
@@ -315,6 +317,13 @@ struct cxl_ep {
 	struct list_head list;
 };
 
+bool is_cxl_region(struct device *dev);
+struct cxl_region *to_cxl_region(struct device *dev);
+struct cxl_region *cxl_alloc_region(struct cxl_decoder *cxld,
+				    int interleave_ways);
+int cxl_add_region(struct cxl_decoder *cxld, struct cxl_region *region);
+int cxl_delete_region(struct cxl_decoder *cxld, const char *region);
+
 static inline bool is_cxl_root(struct cxl_port *port)
 {
 	return port->uport == port->dev.parent;
diff --git a/drivers/cxl/region.h b/drivers/cxl/region.h
new file mode 100644
index 000000000000..3e6e5fb35822
--- /dev/null
+++ b/drivers/cxl/region.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright(c) 2021 Intel Corporation. */
+#ifndef __CXL_REGION_H__
+#define __CXL_REGION_H__
+
+#include <linux/uuid.h>
+
+#include "cxl.h"
+
+/**
+ * struct cxl_region - CXL region
+ * @dev: This region's device.
+ * @id: This regions id. Id is globally unique across all regions.
+ * @list: Node in decoder's region list.
+ * @res: Resource this region carves out of the platform decode range.
+ * @config: HDM decoder program config
+ * @config.size: Size of the region determined from LSA or userspace.
+ * @config.uuid: The UUID for this region.
+ * @config.eniw: Number of interleave ways this region is configured for.
+ * @config.ig: Interleave granularity of region
+ * @config.targets: The memory devices comprising the region.
+ */
+struct cxl_region {
+	struct device dev;
+	int id;
+	struct list_head list;
+	struct resource *res;
+
+	struct {
+		u64 size;
+		uuid_t uuid;
+		int eniw;
+		int ig;
+		struct cxl_memdev *targets[CXL_DECODER_MAX_INTERLEAVE];
+	} config;
+};
+
+#endif
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 8b20e34090f7..73735f561c89 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -43,6 +43,7 @@ cxl_core-y += $(CXL_CORE_SRC)/memdev.o
 cxl_core-y += $(CXL_CORE_SRC)/mbox.o
 cxl_core-y += $(CXL_CORE_SRC)/pci.o
 cxl_core-y += $(CXL_CORE_SRC)/hdm.o
+cxl_core-y += $(CXL_CORE_SRC)/region.o
 cxl_core-y += config_check.o
 
 obj-m += test/
-- 
2.34.1

[PATCH 04/13] cxl/region: Introduce concept of region configuration

[Ben Widawsky]

The region creation APIs create a vacant region. Configuring the region
works in the same way as similar subsystems such as devdax. Sysfs attrs
will be provided to allow userspace to configure the region.  Finally
once all configuration is complete, userspace may activate the region.

Introduced here are the most basic attributes needed to configure a
region. Details of these attribute are described in the ABI
Documentation.

A example is provided below:

/sys/bus/cxl/devices/region0.0:0
├── interleave_granularity
├── interleave_ways
├── offset
├── size
├── subsystem -> ../../../../../../bus/cxl
├── target0
├── uevent
└── uuid

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-cxl |  40 ++++
 drivers/cxl/core/region.c               | 295 ++++++++++++++++++++++++
 2 files changed, 335 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 0fbdd8613654..1a938ad26621 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -159,3 +159,43 @@ Description:
 		region driver before being deleted. The attributes expects a
 		region in the form "regionX.Y:Z". The region's name, allocated
 		by reading create_region, will also be released.
+
+What:		/sys/bus/cxl/devices/decoderX.Y/regionX.Y:Z/offset
+Date:		November, 2021
+KernelVersion:	v5.17
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(RO) A region resides within an address space that is claimed by
+		a decoder. Region space allocation is handled by the driver, but
+		the offset may be read by userspace tooling in order to
+		determine fragmentation, and available size for new regions.
+
+What:
+/sys/bus/cxl/devices/decoderX.Y/regionX.Y:Z/{interleave,size,uuid,target[0-15]}
+Date:		November, 2021
+KernelVersion:	v5.17
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(RW) Configuring regions requires a minimal set of parameters in
+		order for the subsequent bind operation to succeed. The
+		following parameters are defined:
+
+		==	========================================================
+		interleave_granularity Mandatory. Number of consecutive bytes
+			each device in the interleave set will claim. The
+			possible interleave granularity values are determined by
+			the CXL spec and the participating devices.
+		interleave_ways Mandatory. Number of devices participating in the
+			region. Each device will provide 1/interleave of storage
+			for the region.
+		size	Manadatory. Phsyical address space the region will
+			consume.
+		target  Mandatory. Memory devices are the backing storage for a
+			region. There will be N targets based on the number of
+			interleave ways that the top level decoder is configured
+			for. Each target must be set with a memdev device ie.
+			'mem1'. This attribute only becomes available after
+			setting the 'interleave' attribute.
+		uuid	Optional. A unique identifier for the region. If none is
+			selected, the kernel will create one.
+		==	========================================================
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index e3a82f3c118e..26b5ad389cd2 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3,9 +3,12 @@
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/device.h>
 #include <linux/module.h>
+#include <linux/sizes.h>
 #include <linux/slab.h>
+#include <linux/uuid.h>
 #include <linux/idr.h>
 #include <region.h>
+#include <cxlmem.h>
 #include <cxl.h>
 #include "core.h"
 
@@ -18,11 +21,300 @@
  * (programming the hardware) is handled by a separate region driver.
  */
 
+struct cxl_region *to_cxl_region(struct device *dev);
+static const struct attribute_group region_interleave_group;
+
+static bool is_region_active(struct cxl_region *region)
+{
+	/* TODO: Regions can't be activated yet. */
+	return false;
+}
+
+static void remove_target(struct cxl_region *region, int target)
+{
+	struct cxl_memdev *cxlmd;
+
+	cxlmd = region->config.targets[target];
+	if (cxlmd)
+		put_device(&cxlmd->dev);
+	region->config.targets[target] = NULL;
+}
+
+static ssize_t interleave_ways_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct cxl_region *region = to_cxl_region(dev);
+
+	return sysfs_emit(buf, "%d\n", region->config.eniw);
+}
+
+static ssize_t interleave_ways_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t len)
+{
+	struct cxl_region *region = to_cxl_region(dev);
+	int ret, prev_eniw;
+	int val;
+
+	prev_eniw = region->config.eniw;
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+	if (ret < 0 || ret > CXL_DECODER_MAX_INTERLEAVE)
+		return -EINVAL;
+
+	region->config.eniw = val;
+
+	ret = sysfs_update_group(&dev->kobj, &region_interleave_group);
+	if (ret < 0)
+		goto err;
+
+	sysfs_notify(&dev->kobj, NULL, "target_interleave");
+
+	while (prev_eniw > region->config.eniw)
+		remove_target(region, --prev_eniw);
+
+	return len;
+
+err:
+	region->config.eniw = prev_eniw;
+	return ret;
+}
+static DEVICE_ATTR_RW(interleave_ways);
+
+static ssize_t interleave_granularity_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct cxl_region *region = to_cxl_region(dev);
+
+	return sysfs_emit(buf, "%d\n", region->config.ig);
+}
+
+static ssize_t interleave_granularity_store(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf, size_t len)
+{
+	struct cxl_region *region = to_cxl_region(dev);
+	int val, ret;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+	region->config.ig = val;
+
+	return len;
+}
+static DEVICE_ATTR_RW(interleave_granularity);
+
+static ssize_t offset_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
+{
+	struct cxl_region *region = to_cxl_region(dev);
+	struct cxl_decoder *cxld = to_cxl_decoder(dev->parent);
+
+	if (!region->res)
+		return sysfs_emit(buf, "\n");
+
+	return sysfs_emit(buf, "%#llx\n", cxld->platform_res.start - region->res->start);
+}
+static DEVICE_ATTR_RO(offset);
+
+static ssize_t size_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct cxl_region *region = to_cxl_region(dev);
+
+	return sysfs_emit(buf, "%llu\n", region->config.size);
+}
+
+static ssize_t size_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t len)
+{
+	struct cxl_region *region = to_cxl_region(dev);
+	unsigned long long val;
+	ssize_t rc;
+
+	rc = kstrtoull(buf, 0, &val);
+	if (rc)
+		return rc;
+
+	device_lock(&region->dev);
+	if (is_region_active(region))
+		rc = -EBUSY;
+	else
+		region->config.size = val;
+	device_unlock(&region->dev);
+
+	return rc ? rc : len;
+}
+static DEVICE_ATTR_RW(size);
+
+static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct cxl_region *region = to_cxl_region(dev);
+
+	return sysfs_emit(buf, "%pUb\n", &region->config.uuid);
+}
+
+static ssize_t uuid_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t len)
+{
+	struct cxl_region *region = to_cxl_region(dev);
+	ssize_t rc;
+
+	if (len != UUID_STRING_LEN + 1)
+		return -EINVAL;
+
+	device_lock(&region->dev);
+	if (is_region_active(region))
+		rc = -EBUSY;
+	else
+		rc = uuid_parse(buf, &region->config.uuid);
+	device_unlock(&region->dev);
+
+	return rc ? rc : len;
+}
+static DEVICE_ATTR_RW(uuid);
+
+static struct attribute *region_attrs[] = {
+	&dev_attr_interleave_ways.attr,
+	&dev_attr_interleave_granularity.attr,
+	&dev_attr_offset.attr,
+	&dev_attr_size.attr,
+	&dev_attr_uuid.attr,
+	NULL,
+};
+
+static const struct attribute_group region_group = {
+	.attrs = region_attrs,
+};
+
+static size_t show_targetN(struct cxl_region *region, char *buf, int n)
+{
+	int ret;
+
+	device_lock(&region->dev);
+	if (!region->config.targets[n])
+		ret = sysfs_emit(buf, "\n");
+	else
+		ret = sysfs_emit(buf, "%s\n",
+				 dev_name(&region->config.targets[n]->dev));
+	device_unlock(&region->dev);
+
+	return ret;
+}
+
+static size_t set_targetN(struct cxl_region *region, const char *buf, int n,
+			  size_t len)
+{
+	struct device *memdev_dev;
+	struct cxl_memdev *cxlmd;
+
+	device_lock(&region->dev);
+
+	if (len == 1 || region->config.targets[n])
+		remove_target(region, n);
+
+	/* Remove target special case */
+	if (len == 1) {
+		device_unlock(&region->dev);
+		return len;
+	}
+
+	memdev_dev = bus_find_device_by_name(&cxl_bus_type, NULL, buf);
+	if (!memdev_dev)
+		return -ENOENT;
+
+	/* reference to memdev held until target is unset or region goes away */
+
+	cxlmd = to_cxl_memdev(memdev_dev);
+	region->config.targets[n] = cxlmd;
+
+	device_unlock(&region->dev);
+
+	return len;
+}
+
+#define TARGET_ATTR_RW(n)                                                      \
+	static ssize_t target##n##_show(                                       \
+		struct device *dev, struct device_attribute *attr, char *buf)  \
+	{                                                                      \
+		return show_targetN(to_cxl_region(dev), buf, (n));             \
+	}                                                                      \
+	static ssize_t target##n##_store(struct device *dev,                   \
+					 struct device_attribute *attr,        \
+					 const char *buf, size_t len)          \
+	{                                                                      \
+		return set_targetN(to_cxl_region(dev), buf, (n), len);         \
+	}                                                                      \
+	static DEVICE_ATTR_RW(target##n)
+
+TARGET_ATTR_RW(0);
+TARGET_ATTR_RW(1);
+TARGET_ATTR_RW(2);
+TARGET_ATTR_RW(3);
+TARGET_ATTR_RW(4);
+TARGET_ATTR_RW(5);
+TARGET_ATTR_RW(6);
+TARGET_ATTR_RW(7);
+TARGET_ATTR_RW(8);
+TARGET_ATTR_RW(9);
+TARGET_ATTR_RW(10);
+TARGET_ATTR_RW(11);
+TARGET_ATTR_RW(12);
+TARGET_ATTR_RW(13);
+TARGET_ATTR_RW(14);
+TARGET_ATTR_RW(15);
+
+static struct attribute *interleave_attrs[] = {
+	&dev_attr_target0.attr,
+	&dev_attr_target1.attr,
+	&dev_attr_target2.attr,
+	&dev_attr_target3.attr,
+	&dev_attr_target4.attr,
+	&dev_attr_target5.attr,
+	&dev_attr_target6.attr,
+	&dev_attr_target7.attr,
+	&dev_attr_target8.attr,
+	&dev_attr_target9.attr,
+	&dev_attr_target10.attr,
+	&dev_attr_target11.attr,
+	&dev_attr_target12.attr,
+	&dev_attr_target13.attr,
+	&dev_attr_target14.attr,
+	&dev_attr_target15.attr,
+	NULL,
+};
+
+static umode_t visible_targets(struct kobject *kobj, struct attribute *a, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct cxl_region *region = to_cxl_region(dev);
+
+	if (n < region->config.eniw)
+		return a->mode;
+	return 0;
+}
+
+static const struct attribute_group region_interleave_group = {
+	.attrs = interleave_attrs,
+	.is_visible = visible_targets,
+};
+
+static const struct attribute_group *region_groups[] = {
+	&region_group,
+	&region_interleave_group,
+	NULL,
+};
+
 static void cxl_region_release(struct device *dev);
 
 static const struct device_type cxl_region_type = {
 	.name = "cxl_region",
 	.release = cxl_region_release,
+	.groups = region_groups
 };
 
 static ssize_t create_region_show(struct device *dev,
@@ -108,8 +400,11 @@ static void cxl_region_release(struct device *dev)
 {
 	struct cxl_decoder *cxld = to_cxl_decoder(dev->parent);
 	struct cxl_region *region = to_cxl_region(dev);
+	int i;
 
 	ida_free(&cxld->region_ida, region->id);
+	for (i = 0; i < region->config.eniw; i++)
+		remove_target(region, i);
 	kfree(region);
 }
 
-- 
2.34.1

[PATCH 05/13] cxl/mem: Cache port created by the mem dev

[Ben Widawsky]

Since region programming sees all components in the topology as a port,
it's required that endpoints are treated equally. The easiest way to go
from endpoint to port is to simply cache it at creation time.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/cxlmem.h |  2 ++
 drivers/cxl/mem.c    | 16 ++++++++++------
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 4ea0686e5f84..38d6129499c8 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -36,12 +36,14 @@
  * @cxlds: The device state backing this device
  * @id: id number of this memdev instance.
  * @component_reg_phys: register base of component registers
+ * @port: The port created by this device
  */
 struct cxl_memdev {
 	struct device dev;
 	struct cdev cdev;
 	struct cxl_dev_state *cxlds;
 	int id;
+	struct cxl_port *port;
 };
 
 static inline struct cxl_memdev *to_cxl_memdev(struct device *dev)
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 9e6e98e5ea06..2ed7554155d2 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -45,8 +45,8 @@ static int wait_for_media(struct cxl_memdev *cxlmd)
 	return 0;
 }
 
-static int create_endpoint(struct cxl_memdev *cxlmd,
-			   struct cxl_port *parent_port)
+static struct cxl_port *create_endpoint(struct cxl_memdev *cxlmd,
+					struct cxl_port *parent_port)
 {
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_port *endpoint;
@@ -54,10 +54,10 @@ static int create_endpoint(struct cxl_memdev *cxlmd,
 	endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev,
 				     cxlds->component_reg_phys, parent_port);
 	if (IS_ERR(endpoint))
-		return PTR_ERR(endpoint);
+		return endpoint;
 
 	dev_dbg(&cxlmd->dev, "add: %s\n", dev_name(&endpoint->dev));
-	return 0;
+	return endpoint;
 }
 
 /**
@@ -123,7 +123,7 @@ static int cxl_mem_probe(struct device *dev)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
-	struct cxl_port *parent_port;
+	struct cxl_port *parent_port, *ep_port;
 	int rc;
 
 	rc = wait_for_media(cxlmd);
@@ -182,7 +182,11 @@ static int cxl_mem_probe(struct device *dev)
 		goto out;
 	}
 
-	rc = create_endpoint(cxlmd, parent_port);
+	ep_port = create_endpoint(cxlmd, parent_port);
+	if (IS_ERR(ep_port))
+		rc = PTR_ERR(ep_port);
+	else
+		cxlmd->port = ep_port;
 out:
 	device_unlock(&parent_port->dev);
 	put_device(&parent_port->dev);
-- 
2.34.1

[PATCH 06/13] cxl/region: Introduce a cxl_region driver

[Ben Widawsky]

The cxl_region driver is responsible for managing the HDM decoder
programming in the CXL topology. Once a region is created it must be
configured and bound to the driver in order to activate it.

The following is a sample of how such controls might work:

region=$(cat /sys/bus/cxl/devices/decoder0.0/create_region)
echo $region > /sys/bus/cxl/devices/decoder0.0/create_region
echo 2 > /sys/bus/cxl/devices/decoder0.0/region0.0:0/interleave
echo $((256<<20)) > /sys/bus/cxl/devices/decoder0.0/region0.0:0/size
echo mem0 > /sys/bus/cxl/devices/decoder0.0/region0.0:0/target0
echo mem1 > /sys/bus/cxl/devices/decoder0.0/region0.0:0/target1
echo region0.0:0 > /sys/bus/cxl/drivers/cxl_region/bind

In order to handle the eventual rise in failure modes of binding a
region, a new trace event is created to help track these failures for
debug and reconfiguration paths in userspace.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 .../driver-api/cxl/memory-devices.rst         |   3 +
 drivers/cxl/Makefile                          |   2 +
 drivers/cxl/core/core.h                       |   1 +
 drivers/cxl/core/port.c                       |  21 +-
 drivers/cxl/core/region.c                     |  25 +-
 drivers/cxl/cxl.h                             |   6 +
 drivers/cxl/region.c                          | 331 ++++++++++++++++++
 drivers/cxl/region.h                          |   4 +
 drivers/cxl/trace.h                           |  45 +++
 9 files changed, 433 insertions(+), 5 deletions(-)
 create mode 100644 drivers/cxl/region.c
 create mode 100644 drivers/cxl/trace.h

diff --git a/Documentation/driver-api/cxl/memory-devices.rst b/Documentation/driver-api/cxl/memory-devices.rst
index dc756ed23a3a..6734939b7136 100644
--- a/Documentation/driver-api/cxl/memory-devices.rst
+++ b/Documentation/driver-api/cxl/memory-devices.rst
@@ -73,6 +73,9 @@ CXL Core
 
 CXL Regions
 -----------
+.. kernel-doc:: drivers/cxl/region.c
+   :doc: cxl region
+
 .. kernel-doc:: drivers/cxl/region.h
    :identifiers:
 
diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index ce267ef11d93..677a04528b22 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -5,9 +5,11 @@ obj-$(CONFIG_CXL_MEM) += cxl_mem.o
 obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
 obj-$(CONFIG_CXL_PMEM) += cxl_pmem.o
 obj-$(CONFIG_CXL_PORT) += cxl_port.o
+obj-$(CONFIG_CXL_MEM) += cxl_region.o
 
 cxl_mem-y := mem.o
 cxl_pci-y := pci.o
 cxl_acpi-y := acpi.o
 cxl_pmem-y := pmem.o
 cxl_port-y := port.o
+cxl_region-y := region.o
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 1d4d1699b479..bd47e1b59f8b 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -7,6 +7,7 @@
 extern const struct device_type cxl_nvdimm_bridge_type;
 extern const struct device_type cxl_nvdimm_type;
 extern const struct device_type cxl_memdev_type;
+extern const struct device_type cxl_region_type;
 
 extern struct attribute_group cxl_base_attribute_group;
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index ef3840c50e3e..67f3345d44ef 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -8,6 +8,7 @@
 #include <linux/slab.h>
 #include <linux/idr.h>
 #include <cxlmem.h>
+#include <region.h>
 #include <cxl.h>
 #include <pci.h>
 #include "core.h"
@@ -29,6 +30,8 @@
 static DEFINE_IDA(cxl_port_ida);
 static DEFINE_XARRAY(cxl_root_buses);
 
+static void cxl_decoder_release(struct device *dev);
+
 static bool is_cxl_decoder(struct device *dev);
 
 static int decoder_match(struct device *dev, void *data)
@@ -732,6 +735,7 @@ struct cxl_port *find_cxl_root(struct cxl_memdev *cxlmd)
 	}
 	return NULL;
 }
+EXPORT_SYMBOL_NS_GPL(find_cxl_root, CXL);
 
 static void cxl_remove_ep(void *data)
 {
@@ -1276,6 +1280,8 @@ static int cxl_device_id(struct device *dev)
 	}
 	if (dev->type == &cxl_memdev_type)
 		return CXL_DEVICE_MEMORY_EXPANDER;
+	if (dev->type == &cxl_region_type)
+		return CXL_DEVICE_REGION;
 	return 0;
 }
 
@@ -1292,10 +1298,21 @@ static int cxl_bus_match(struct device *dev, struct device_driver *drv)
 
 static int cxl_bus_probe(struct device *dev)
 {
-	int rc;
+	int id = cxl_device_id(dev);
+	int rc = -ENODEV;
+
+	if (id == CXL_DEVICE_REGION) {
+		/* Regions cannot bind until parameters are set */
+		struct cxl_region *region = to_cxl_region(dev);
+
+		if (is_cxl_region_configured(region))
+			rc = to_cxl_drv(dev->driver)->probe(dev);
+	} else {
+		rc = to_cxl_drv(dev->driver)->probe(dev);
+	}
 
-	rc = to_cxl_drv(dev->driver)->probe(dev);
 	dev_dbg(dev, "probe: %d\n", rc);
+
 	return rc;
 }
 
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 26b5ad389cd2..de0514a85cf1 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -12,6 +12,8 @@
 #include <cxl.h>
 #include "core.h"
 
+#include "core.h"
+
 /**
  * DOC: cxl core region
  *
@@ -26,10 +28,27 @@ static const struct attribute_group region_interleave_group;
 
 static bool is_region_active(struct cxl_region *region)
 {
-	/* TODO: Regions can't be activated yet. */
-	return false;
+	return region->active;
 }
 
+/*
+ * Most sanity checking is left up to region binding. This does the most basic
+ * check to determine whether or not the core should try probing the driver.
+ */
+bool is_cxl_region_configured(const struct cxl_region *region)
+{
+	/* zero sized regions aren't a thing. */
+	if (region->config.size <= 0)
+		return false;
+
+	/* all regions have at least 1 target */
+	if (!region->config.targets[0])
+		return false;
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(is_cxl_region_configured);
+
 static void remove_target(struct cxl_region *region, int target)
 {
 	struct cxl_memdev *cxlmd;
@@ -311,7 +330,7 @@ static const struct attribute_group *region_groups[] = {
 
 static void cxl_region_release(struct device *dev);
 
-static const struct device_type cxl_region_type = {
+const struct device_type cxl_region_type = {
 	.name = "cxl_region",
 	.release = cxl_region_release,
 	.groups = region_groups
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 79c5781b6173..b318cabfc4a2 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -181,6 +181,10 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
 #define CXL_DECODER_F_ENABLE    BIT(5)
 #define CXL_DECODER_F_MASK  GENMASK(5, 0)
 
+#define cxl_is_pmem_t3(flags)                                                  \
+	(((flags) & (CXL_DECODER_F_TYPE3 | CXL_DECODER_F_PMEM)) ==             \
+	 (CXL_DECODER_F_TYPE3 | CXL_DECODER_F_PMEM))
+
 enum cxl_decoder_type {
        CXL_DECODER_ACCELERATOR = 2,
        CXL_DECODER_EXPANDER = 3,
@@ -348,6 +352,7 @@ int devm_cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
 		       resource_size_t component_reg_phys);
 struct cxl_dport *cxl_find_dport_by_dev(struct cxl_port *port,
 					const struct device *dev);
+struct cxl_port *ep_find_cxl_port(struct cxl_memdev *cxlmd, unsigned int depth);
 
 struct cxl_decoder *to_cxl_decoder(struct device *dev);
 bool is_root_decoder(struct device *dev);
@@ -388,6 +393,7 @@ void cxl_driver_unregister(struct cxl_driver *cxl_drv);
 #define CXL_DEVICE_PORT			3
 #define CXL_DEVICE_MEMORY_EXPANDER	4
 #define CXL_DEVICE_ROOT			5
+#define CXL_DEVICE_REGION		6
 
 #define MODULE_ALIAS_CXL(type) MODULE_ALIAS("cxl:t" __stringify(type) "*")
 #define CXL_MODALIAS_FMT "cxl:t%d"
diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
new file mode 100644
index 000000000000..9174925b67e3
--- /dev/null
+++ b/drivers/cxl/region.c
@@ -0,0 +1,331 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
+#include <linux/platform_device.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include "cxlmem.h"
+#include "region.h"
+#include "cxl.h"
+
+#define CREATE_TRACE_POINTS
+#include "trace.h"
+
+/**
+ * DOC: cxl region
+ *
+ * This module implements a region driver that is capable of programming CXL
+ * hardware to setup regions.
+ *
+ * A CXL region encompasses a chunk of host physical address space that may be
+ * consumed by a single device (x1 interleave aka linear) or across multiple
+ * devices (xN interleaved). A region is a child device of a &struct
+ * cxl_decoder. There may be multiple active regions under a single &struct
+ * cxl_decoder. The common case for multiple regions would be several linear,
+ * contiguous regions under a single decoder. Generally, there will be a 1:1
+ * relationship between decoder and region when the region is interleaved.
+ */
+
+#define region_ways(region) ((region)->config.eniw)
+
+static struct cxl_decoder *rootd_from_region(struct cxl_region *r)
+{
+	struct device *d = r->dev.parent;
+
+	if (WARN_ONCE(!is_root_decoder(d), "Corrupt topology for root region\n"))
+		return NULL;
+
+	return to_cxl_decoder(d);
+}
+
+static struct cxl_port *get_hostbridge(const struct cxl_memdev *ep)
+{
+	struct cxl_port *port = ep->port;
+
+	while (!is_cxl_root(port)) {
+		port = to_cxl_port(port->dev.parent);
+		if (port->depth == 1)
+			return port;
+	}
+
+	BUG();
+	return NULL;
+}
+
+static struct cxl_port *get_root_decoder(const struct cxl_memdev *endpoint)
+{
+	struct cxl_port *hostbridge = get_hostbridge(endpoint);
+
+	if (hostbridge)
+		return to_cxl_port(hostbridge->dev.parent);
+
+	return NULL;
+}
+
+/**
+ * sanitize_region() - Check is region is reasonably configured
+ * @region: The region to check
+ *
+ * Determination as to whether or not a region can possibly be configured is
+ * described in CXL Memory Device SW Guide. In order to implement the algorithms
+ * described there, certain more basic configuration parameters must first need
+ * to be validated. That is accomplished by this function.
+ *
+ * Returns 0 if the region is reasonably configured, else returns a negative
+ * error code.
+ */
+static int sanitize_region(const struct cxl_region *region)
+{
+	int i;
+
+	if (dev_WARN_ONCE(&region->dev, !is_cxl_region_configured(region),
+			  "unconfigured regions can't be probed (race?)\n")) {
+		return -ENXIO;
+	}
+
+	if (region->config.size % (SZ_256M * region_ways(region))) {
+		trace_sanitize_failed(region,
+				      "Invalid size. Must be multiple of NIW");
+		return -ENXIO;
+	}
+
+	for (i = 0; i < region_ways(region); i++) {
+		if (!region->config.targets[i]) {
+			trace_sanitize_failed(region,
+					      "Missing memory device target");
+			return -ENXIO;
+		}
+		if (!region->config.targets[i]->dev.driver) {
+			trace_sanitize_failed(region,
+					      "Target isn't CXL.mem capable");
+			return -ENODEV;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * allocate_address_space() - Gets address space for the region.
+ * @region: The region that will consume the address space
+ */
+static int allocate_address_space(struct cxl_region *region)
+{
+	/* TODO */
+	return 0;
+}
+
+/**
+ * find_cdat_dsmas() - Find a valid DSMAS for the region
+ * @region: The region
+ */
+static bool find_cdat_dsmas(const struct cxl_region *region)
+{
+	return true;
+}
+
+/**
+ * qtg_match() - Does this CFMWS have desirable QTG for the endpoint
+ * @cfmws: The CFMWS for the region
+ * @endpoint: Endpoint whose QTG is being compared
+ *
+ * Prior to calling this function, the caller should verify that all endpoints
+ * in the region have the same QTG ID.
+ *
+ * Returns true if the QTG ID of the CFMWS matches the endpoint
+ */
+static bool qtg_match(const struct cxl_decoder *cfmws,
+		      const struct cxl_memdev *endpoint)
+{
+	/* TODO: */
+	return true;
+}
+
+/**
+ * region_xhb_config_valid() - determine cross host bridge validity
+ * @cfmws: The CFMWS to check against
+ * @region: The region being programmed
+ *
+ * The algorithm is outlined in 2.13.14 "Verify XHB configuration sequence" of
+ * the CXL Memory Device SW Guide (Rev1p0).
+ *
+ * Returns true if the configuration is valid.
+ */
+static bool region_xhb_config_valid(const struct cxl_region *region,
+				    const struct cxl_decoder *cfmws)
+{
+	/* TODO: */
+	return true;
+}
+
+/**
+ * region_hb_rp_config_valid() - determine root port ordering is correct
+ * @cfmws: CFMWS decoder for this @region
+ * @region: Region to validate
+ *
+ * The algorithm is outlined in 2.13.15 "Verify HB root port configuration
+ * sequence" of the CXL Memory Device SW Guide (Rev1p0).
+ *
+ * Returns true if the configuration is valid.
+ */
+static bool region_hb_rp_config_valid(const struct cxl_region *region,
+				      const struct cxl_decoder *cfmws)
+{
+	/* TODO: */
+	return true;
+}
+
+/**
+ * rootd_contains() - determine if this region can exist in the root decoder
+ * @rootd: CFMWS that potentially decodes to this region
+ * @region: region to be routed by the @rootd
+ */
+static bool rootd_contains(const struct cxl_region *region,
+			   const struct cxl_decoder *rootd)
+{
+	/* TODO: */
+	return true;
+}
+
+static bool rootd_valid(const struct cxl_region *region,
+			const struct cxl_decoder *rootd)
+{
+	const struct cxl_memdev *endpoint = region->config.targets[0];
+
+	if (!qtg_match(rootd, endpoint))
+		return false;
+
+	if (!cxl_is_pmem_t3(rootd->flags))
+		return false;
+
+	if (!region_xhb_config_valid(region, rootd))
+		return false;
+
+	if (!region_hb_rp_config_valid(region, rootd))
+		return false;
+
+	if (!rootd_contains(region, rootd))
+		return false;
+
+	return true;
+}
+
+struct rootd_context {
+	const struct cxl_region *region;
+	struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
+	int count;
+};
+
+static int rootd_match(struct device *dev, void *data)
+{
+	struct rootd_context *ctx = (struct rootd_context *)data;
+	const struct cxl_region *region = ctx->region;
+
+	if (!is_root_decoder(dev))
+		return 0;
+
+	return !!rootd_valid(region, to_cxl_decoder(dev));
+}
+
+/*
+ * This is a roughly equivalent implementation to "Figure 45 - High-level
+ * sequence: Finding CFMWS for region" from the CXL Memory Device SW Guide
+ * Rev1p0.
+ */
+static struct cxl_decoder *find_rootd(const struct cxl_region *region,
+				      const struct cxl_port *root)
+{
+	struct rootd_context ctx;
+	struct device *ret;
+
+	ctx.region = region;
+
+	ret = device_find_child((struct device *)&root->dev, &ctx, rootd_match);
+	if (ret)
+		return to_cxl_decoder(ret);
+
+	return NULL;
+}
+
+static int collect_ep_decoders(const struct cxl_region *region)
+{
+	/* TODO: */
+	return 0;
+}
+
+static int bind_region(const struct cxl_region *region)
+{
+	/* TODO: */
+	return 0;
+}
+
+static int cxl_region_probe(struct device *dev)
+{
+	struct cxl_region *region = to_cxl_region(dev);
+	struct cxl_port *root_port;
+	struct cxl_decoder *rootd, *ours;
+	int ret;
+
+	device_lock_assert(&region->dev);
+
+	if (region->active)
+		return 0;
+
+	if (uuid_is_null(&region->config.uuid))
+		uuid_gen(&region->config.uuid);
+
+	/* TODO: What about volatile, and LSA generated regions? */
+
+	ret = sanitize_region(region);
+	if (ret)
+		return ret;
+
+	ret = allocate_address_space(region);
+	if (ret)
+		return ret;
+
+	if (!find_cdat_dsmas(region))
+		return -ENXIO;
+
+	rootd = rootd_from_region(region);
+	if (!rootd) {
+		dev_err(dev, "Couldn't find root decoder\n");
+		return -ENXIO;
+	}
+
+	if (!rootd_valid(region, rootd)) {
+		dev_err(dev, "Picked invalid rootd\n");
+		return -ENXIO;
+	}
+
+	root_port = get_root_decoder(region->config.targets[0]);
+	ours = find_rootd(region, root_port);
+	if (ours != rootd)
+		dev_warn(dev, "Picked different rootd %s %s\n",
+			 dev_name(&rootd->dev), dev_name(&ours->dev));
+	if (ours)
+		put_device(&ours->dev);
+
+	ret = collect_ep_decoders(region);
+	if (ret)
+		return ret;
+
+	ret = bind_region(region);
+	if (!ret) {
+		region->active = true;
+		trace_region_activated(region, "");
+	}
+
+	return ret;
+}
+
+static struct cxl_driver cxl_region_driver = {
+	.name = "cxl_region",
+	.probe = cxl_region_probe,
+	.id = CXL_DEVICE_REGION,
+};
+module_cxl_driver(cxl_region_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(CXL);
+MODULE_ALIAS_CXL(CXL_DEVICE_REGION);
diff --git a/drivers/cxl/region.h b/drivers/cxl/region.h
index 3e6e5fb35822..97bb3f12f2fc 100644
--- a/drivers/cxl/region.h
+++ b/drivers/cxl/region.h
@@ -13,6 +13,7 @@
  * @id: This regions id. Id is globally unique across all regions.
  * @list: Node in decoder's region list.
  * @res: Resource this region carves out of the platform decode range.
+ * @active: If the region has been activated.
  * @config: HDM decoder program config
  * @config.size: Size of the region determined from LSA or userspace.
  * @config.uuid: The UUID for this region.
@@ -25,6 +26,7 @@ struct cxl_region {
 	int id;
 	struct list_head list;
 	struct resource *res;
+	bool active;
 
 	struct {
 		u64 size;
@@ -35,4 +37,6 @@ struct cxl_region {
 	} config;
 };
 
+bool is_cxl_region_configured(const struct cxl_region *region);
+
 #endif
diff --git a/drivers/cxl/trace.h b/drivers/cxl/trace.h
new file mode 100644
index 000000000000..8f7f471e15b8
--- /dev/null
+++ b/drivers/cxl/trace.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM cxl
+
+#if !defined(__CXL_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ)
+#define __CXL_TRACE_H__
+
+#include <linux/tracepoint.h>
+
+DECLARE_EVENT_CLASS(cxl_region_template,
+
+	TP_PROTO(const struct cxl_region *region, char *status),
+
+	TP_ARGS(region, status),
+
+	TP_STRUCT__entry(
+		__string(dev_name, dev_name(&region->dev))
+		__field(const struct cxl_region *, region)
+		__string(status, status)
+	),
+
+	TP_fast_assign(
+		__assign_str(dev_name, dev_name(&region->dev));
+		__entry->region = (const struct cxl_region *)region;
+		__assign_str(status, status);
+	),
+
+	TP_printk("%s: (%s)\n", __get_str(dev_name), __get_str(status))
+);
+
+DEFINE_EVENT(cxl_region_template, region_activated,
+	     TP_PROTO(const struct cxl_region *region, char *status),
+	     TP_ARGS(region, status));
+DEFINE_EVENT(cxl_region_template, sanitize_failed,
+	     TP_PROTO(const struct cxl_region *region, char *status),
+	     TP_ARGS(region, status));
+
+#endif /* if !defined (__CXL_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ) */
+
+/* This part must be outside protection */
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH ../../../drivers/cxl
+#define TRACE_INCLUDE_FILE trace
+#include <trace/define_trace.h>
-- 
2.34.1

[PATCH 07/13] cxl/acpi: Handle address space allocation

[Ben Widawsky]

Regions are carved out of an addresses space which is claimed by top
level decoders, and subsequently their children decoders. Regions are
created with a size and therefore must fit, with proper alignment, in
that address space. The support for doing this fitting is handled by the
driver automatically.

As an example, a platform might configure a top level decoder to claim
1TB of address space @ 0x800000000 -> 0x10800000000; it would be
possible to create M regions with appropriate alignment to occupy that
address space. Each of those regions would have a host physical address
somewhere in the range between 32G and 1.3TB, and the location will be
determined by the logic added here.

The request_region() usage is not strictly mandatory at this point as
the actual handling of the address space is done with genpools. It is
highly likely however that the resource/region APIs will become useful
in the not too distant future.

All decoders manage a host physical address space while active. Only the
root decoder has constraints on location and size. As a result, it makes
most sense for the root decoder to be responsible for managing the
entire address space, and mid-level decoders and endpoints can ask the
root decoder for suballocations.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/acpi.c   | 30 ++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h    |  2 ++
 drivers/cxl/region.c | 12 ++++++------
 3 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 4c746a6ef48c..a7ce0d660b34 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2021 Intel Corporation. All rights reserved. */
 #include <linux/platform_device.h>
+#include <linux/genalloc.h>
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
@@ -73,6 +74,27 @@ static int cxl_acpi_cfmws_verify(struct device *dev,
 	return 0;
 }
 
+/*
+ * Every decoder while active has an address space that it is decoding. However,
+ * only the root level decoders have fixed host physical address space ranges.
+ */
+static int cxl_create_cfmws_address_space(struct cxl_decoder *cxld,
+					  struct acpi_cedt_cfmws *cfmws)
+{
+	const int order = ilog2(SZ_256M * cxld->interleave_ways);
+	struct device *dev = &cxld->dev;
+	struct gen_pool *pool;
+
+	pool = devm_gen_pool_create(dev, order, NUMA_NO_NODE, dev_name(dev));
+	if (IS_ERR(pool))
+		return PTR_ERR(pool);
+
+	cxld->address_space = pool;
+
+	return gen_pool_add(cxld->address_space, cfmws->base_hpa,
+			    cfmws->window_size, NUMA_NO_NODE);
+}
+
 struct cxl_cfmws_context {
 	struct device *dev;
 	struct cxl_port *root_port;
@@ -113,6 +135,14 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
 	cxld->interleave_ways = CFMWS_INTERLEAVE_WAYS(cfmws);
 	cxld->interleave_granularity = CFMWS_INTERLEAVE_GRANULARITY(cfmws);
 
+	rc = cxl_create_cfmws_address_space(cxld, cfmws);
+	if (rc) {
+		dev_err(dev,
+			"Failed to create CFMWS address space for decoder\n");
+		put_device(&cxld->dev);
+		return 0;
+	}
+
 	rc = cxl_decoder_add(cxld, target_map);
 	if (rc)
 		put_device(&cxld->dev);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index b318cabfc4a2..19e65ed35796 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -207,6 +207,7 @@ enum cxl_decoder_type {
  * @target_type: accelerator vs expander (type2 vs type3) selector
  * @flags: memory type capabilities and locking
  * @region_ida: allocator for region ids.
+ * @address_space: Used/free address space for regions.
  * @nr_targets: number of elements in @target
  * @target: active ordered target list in current decoder configuration
  */
@@ -222,6 +223,7 @@ struct cxl_decoder {
 	enum cxl_decoder_type target_type;
 	unsigned long flags;
 	struct ida region_ida;
+	struct gen_pool *address_space;
 	const int nr_targets;
 	struct cxl_dport *target[];
 };
diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
index 9174925b67e3..b458d7b97120 100644
--- a/drivers/cxl/region.c
+++ b/drivers/cxl/region.c
@@ -126,7 +126,7 @@ static bool find_cdat_dsmas(const struct cxl_region *region)
 
 /**
  * qtg_match() - Does this CFMWS have desirable QTG for the endpoint
- * @cfmws: The CFMWS for the region
+ * @rootd: The root decoder for the region
  * @endpoint: Endpoint whose QTG is being compared
  *
  * Prior to calling this function, the caller should verify that all endpoints
@@ -134,7 +134,7 @@ static bool find_cdat_dsmas(const struct cxl_region *region)
  *
  * Returns true if the QTG ID of the CFMWS matches the endpoint
  */
-static bool qtg_match(const struct cxl_decoder *cfmws,
+static bool qtg_match(const struct cxl_decoder *rootd,
 		      const struct cxl_memdev *endpoint)
 {
 	/* TODO: */
@@ -143,7 +143,7 @@ static bool qtg_match(const struct cxl_decoder *cfmws,
 
 /**
  * region_xhb_config_valid() - determine cross host bridge validity
- * @cfmws: The CFMWS to check against
+ * @rootd: The root decoder to check against
  * @region: The region being programmed
  *
  * The algorithm is outlined in 2.13.14 "Verify XHB configuration sequence" of
@@ -152,7 +152,7 @@ static bool qtg_match(const struct cxl_decoder *cfmws,
  * Returns true if the configuration is valid.
  */
 static bool region_xhb_config_valid(const struct cxl_region *region,
-				    const struct cxl_decoder *cfmws)
+				    const struct cxl_decoder *rootd)
 {
 	/* TODO: */
 	return true;
@@ -160,7 +160,7 @@ static bool region_xhb_config_valid(const struct cxl_region *region,
 
 /**
  * region_hb_rp_config_valid() - determine root port ordering is correct
- * @cfmws: CFMWS decoder for this @region
+ * @rootd: root decoder for this @region
  * @region: Region to validate
  *
  * The algorithm is outlined in 2.13.15 "Verify HB root port configuration
@@ -169,7 +169,7 @@ static bool region_xhb_config_valid(const struct cxl_region *region,
  * Returns true if the configuration is valid.
  */
 static bool region_hb_rp_config_valid(const struct cxl_region *region,
-				      const struct cxl_decoder *cfmws)
+				      const struct cxl_decoder *rootd)
 {
 	/* TODO: */
 	return true;
-- 
2.34.1

[PATCH 08/13] cxl/region: Address space allocation

[Ben Widawsky]

When a region is not assigned a host physical address, one is picked by
the driver. As the address will determine which CFMWS contains the
region, it's usually a better idea to let the driver make this
determination.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/region.c | 38 ++++++++++++++++++++++++++++++++++++--
 drivers/cxl/trace.h  |  3 +++
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
index b458d7b97120..c8e3c48dfbb9 100644
--- a/drivers/cxl/region.c
+++ b/drivers/cxl/region.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2021 Intel Corporation. All rights reserved. */
 #include <linux/platform_device.h>
+#include <linux/genalloc.h>
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/pci.h>
@@ -62,6 +63,20 @@ static struct cxl_port *get_root_decoder(const struct cxl_memdev *endpoint)
 	return NULL;
 }
 
+static void release_cxl_region(void *r)
+{
+	struct cxl_region *region = (struct cxl_region *)r;
+	struct cxl_decoder *rootd = rootd_from_region(region);
+	struct resource *res = &rootd->platform_res;
+	resource_size_t start, size;
+
+	start = region->res->start;
+	size = resource_size(region->res);
+
+	__release_region(res, start, size);
+	gen_pool_free(rootd->address_space, start, size);
+}
+
 /**
  * sanitize_region() - Check is region is reasonably configured
  * @region: The region to check
@@ -111,8 +126,27 @@ static int sanitize_region(const struct cxl_region *region)
  */
 static int allocate_address_space(struct cxl_region *region)
 {
-	/* TODO */
-	return 0;
+	struct cxl_decoder *rootd = rootd_from_region(region);
+	unsigned long start;
+
+	start = gen_pool_alloc(rootd->address_space, region->config.size);
+	if (!start) {
+		trace_allocation_failed(region,
+					"Couldn't allocate address space");
+		return -ENOMEM;
+	}
+
+	region->res = __request_region(&rootd->platform_res, start,
+				       region->config.size,
+				       dev_name(&region->dev), IORESOURCE_MEM);
+	if (!region->res) {
+		trace_allocation_failed(region, "Couldn't obtain region");
+		gen_pool_free(rootd->address_space, start, region->config.size);
+		return -ENOMEM;
+	}
+
+	return devm_add_action_or_reset(&region->dev, release_cxl_region,
+					region);
 }
 
 /**
diff --git a/drivers/cxl/trace.h b/drivers/cxl/trace.h
index 8f7f471e15b8..a53f00ba5d0e 100644
--- a/drivers/cxl/trace.h
+++ b/drivers/cxl/trace.h
@@ -35,6 +35,9 @@ DEFINE_EVENT(cxl_region_template, region_activated,
 DEFINE_EVENT(cxl_region_template, sanitize_failed,
 	     TP_PROTO(const struct cxl_region *region, char *status),
 	     TP_ARGS(region, status));
+DEFINE_EVENT(cxl_region_template, allocation_failed,
+	     TP_PROTO(const struct cxl_region *region, char *status),
+	     TP_ARGS(region, status));
 
 #endif /* if !defined (__CXL_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ) */
 
-- 
2.34.1

[PATCH 09/13] cxl/region: Implement XHB verification

[Ben Widawsky]

Cross host bridge verification primarily determines if the requested
interleave ordering can be achieved by the root decoder, which isn't as
programmable as other decoders.

The algorithm implemented here is based on the CXL Type 3 Memory Device
Software Guide, chapter 2.13.14

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 .clang-format        |  2 +
 drivers/cxl/region.c | 89 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/cxl/trace.h  |  3 ++
 3 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/.clang-format b/.clang-format
index 15d4eaabc6b5..55f628f21722 100644
--- a/.clang-format
+++ b/.clang-format
@@ -169,6 +169,8 @@ ForEachMacros:
   - 'for_each_cpu_and'
   - 'for_each_cpu_not'
   - 'for_each_cpu_wrap'
+  - 'for_each_cxl_decoder_target'
+  - 'for_each_cxl_endpoint'
   - 'for_each_dapm_widgets'
   - 'for_each_dev_addr'
   - 'for_each_dev_scope'
diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
index c8e3c48dfbb9..ca559a4b5347 100644
--- a/drivers/cxl/region.c
+++ b/drivers/cxl/region.c
@@ -28,6 +28,17 @@
  */
 
 #define region_ways(region) ((region)->config.eniw)
+#define region_ig(region) (ilog2((region)->config.ig))
+
+#define for_each_cxl_endpoint(ep, region, idx)                                 \
+	for (idx = 0, ep = (region)->config.targets[idx];                      \
+	     idx < region_ways(region);                                        \
+	     idx++, ep = (region)->config.targets[idx])
+
+#define for_each_cxl_decoder_target(target, decoder, idx)                      \
+	for (idx = 0, target = (decoder)->target[idx];                         \
+	     idx < (decoder)->nr_targets;                                      \
+	     idx++, target++)
 
 static struct cxl_decoder *rootd_from_region(struct cxl_region *r)
 {
@@ -175,6 +186,30 @@ static bool qtg_match(const struct cxl_decoder *rootd,
 	return true;
 }
 
+static int get_unique_hostbridges(const struct cxl_region *region,
+				  struct cxl_port **hbs)
+{
+	struct cxl_memdev *ep;
+	int i, hb_count = 0;
+
+	for_each_cxl_endpoint(ep, region, i) {
+		struct cxl_port *hb = get_hostbridge(ep);
+		bool found = false;
+		int j;
+
+		BUG_ON(!hb);
+
+		for (j = 0; j < hb_count; j++) {
+			if (hbs[j] == hb)
+				found = true;
+		}
+		if (!found)
+			hbs[hb_count++] = hb;
+	}
+
+	return hb_count;
+}
+
 /**
  * region_xhb_config_valid() - determine cross host bridge validity
  * @rootd: The root decoder to check against
@@ -188,7 +223,59 @@ static bool qtg_match(const struct cxl_decoder *rootd,
 static bool region_xhb_config_valid(const struct cxl_region *region,
 				    const struct cxl_decoder *rootd)
 {
-	/* TODO: */
+	struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
+	int rootd_ig, i;
+	struct cxl_dport *target;
+
+	/* Are all devices in this region on the same CXL host bridge */
+	if (get_unique_hostbridges(region, hbs) == 1)
+		return true;
+
+	rootd_ig = rootd->interleave_granularity;
+
+	/* CFMWS.HBIG >= Device.Label.IG */
+	if (rootd_ig < region_ig(region)) {
+		trace_xhb_valid(region,
+				"granularity does not support the region interleave granularity\n");
+		return false;
+	}
+
+	/* ((2^(CFMWS.HBIG - Device.RLabel.IG) * (2^CFMWS.ENIW)) > Device.RLabel.NLabel) */
+	if (1 << (rootd_ig - region_ig(region)) * (1 << rootd->interleave_ways) >
+	    region_ways(region)) {
+		trace_xhb_valid(region,
+				"granularity to device granularity ratio requires a larger number of devices than currently configured");
+		return false;
+	}
+
+	/* Check that endpoints are hooked up in the correct order */
+	for_each_cxl_decoder_target(target, rootd, i) {
+		struct cxl_memdev *endpoint = region->config.targets[i];
+
+		if (get_hostbridge(endpoint) != target->port) {
+			trace_xhb_valid(region, "device ordering bad\n");
+			return false;
+		}
+	}
+
+	/*
+	 * CFMWS.InterleaveTargetList[n] must contain all devices, x where:
+	 *	(Device[x],RegionLabel.Position >> (CFMWS.HBIG -
+	 *	Device[x].RegionLabel.InterleaveGranularity)) &
+	 *	((2^CFMWS.ENIW) - 1) = n
+	 *
+	 * Linux notes: All devices are known to have the same interleave
+	 * granularity at this point.
+	 */
+	for_each_cxl_decoder_target(target, rootd, i) {
+		if (((i >> (rootd_ig - region_ig(region)))) &
+		    (((1 << rootd->interleave_ways) - 1) != target->port_id)) {
+			trace_xhb_valid(region,
+					"One or more devices are not connected to the correct hostbridge.");
+			return false;
+		}
+	}
+
 	return true;
 }
 
diff --git a/drivers/cxl/trace.h b/drivers/cxl/trace.h
index a53f00ba5d0e..4de47d1111ac 100644
--- a/drivers/cxl/trace.h
+++ b/drivers/cxl/trace.h
@@ -38,6 +38,9 @@ DEFINE_EVENT(cxl_region_template, sanitize_failed,
 DEFINE_EVENT(cxl_region_template, allocation_failed,
 	     TP_PROTO(const struct cxl_region *region, char *status),
 	     TP_ARGS(region, status));
+DEFINE_EVENT(cxl_region_template, xhb_valid,
+	     TP_PROTO(const struct cxl_region *region, char *status),
+	     TP_ARGS(region, status));
 
 #endif /* if !defined (__CXL_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ) */
 
-- 
2.34.1

[PATCH 10/13] cxl/region: HB port config verification

[Ben Widawsky]

Host bridge root port verification determines if the device ordering in
an interleave set can be programmed through the host bridges and
switches.

The algorithm implemented here is based on the CXL Type 3 Memory Device
Software Guide, chapter 2.13.15

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 .clang-format           |   1 +
 drivers/cxl/core/port.c |   1 +
 drivers/cxl/cxl.h       |   2 +
 drivers/cxl/region.c    | 122 +++++++++++++++++++++++++++++++++++++++-
 drivers/cxl/trace.h     |   3 +
 5 files changed, 128 insertions(+), 1 deletion(-)

diff --git a/.clang-format b/.clang-format
index 55f628f21722..96c282b63e7b 100644
--- a/.clang-format
+++ b/.clang-format
@@ -171,6 +171,7 @@ ForEachMacros:
   - 'for_each_cpu_wrap'
   - 'for_each_cxl_decoder_target'
   - 'for_each_cxl_endpoint'
+  - 'for_each_cxl_endpoint_hb'
   - 'for_each_dapm_widgets'
   - 'for_each_dev_addr'
   - 'for_each_dev_scope'
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 67f3345d44ef..99589f23f1ff 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -585,6 +585,7 @@ struct cxl_dport *cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
 		return ERR_PTR(-ENOMEM);
 
 	INIT_LIST_HEAD(&dport->list);
+	INIT_LIST_HEAD(&dport->verify_link);
 	dport->dport = get_device(dport_dev);
 	dport->port_id = port_id;
 	dport->component_reg_phys = component_reg_phys;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 19e65ed35796..762da3621ca4 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -303,6 +303,7 @@ struct cxl_port {
  * @port: reference to cxl_port that contains this downstream port
  * @list: node for a cxl_port's list of cxl_dport instances
  * @link_name: the name of the sysfs link from @port to @dport
+ * @verify_link: node used for hb root port verification
  */
 struct cxl_dport {
 	struct device *dport;
@@ -311,6 +312,7 @@ struct cxl_dport {
 	struct cxl_port *port;
 	struct list_head list;
 	char link_name[CXL_TARGET_STRLEN];
+	struct list_head verify_link;
 };
 
 /**
diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
index ca559a4b5347..87fdcf0049d8 100644
--- a/drivers/cxl/region.c
+++ b/drivers/cxl/region.c
@@ -4,6 +4,7 @@
 #include <linux/genalloc.h>
 #include <linux/device.h>
 #include <linux/module.h>
+#include <linux/sort.h>
 #include <linux/pci.h>
 #include "cxlmem.h"
 #include "region.h"
@@ -35,6 +36,12 @@
 	     idx < region_ways(region);                                        \
 	     idx++, ep = (region)->config.targets[idx])
 
+#define for_each_cxl_endpoint_hb(ep, region, hb, idx)                          \
+	for (idx = 0, (ep) = (region)->config.targets[idx];                    \
+	     idx < region_ways(region);                                        \
+	     idx++, (ep) = (region)->config.targets[idx])                      \
+		if (get_hostbridge(ep) == (hb))
+
 #define for_each_cxl_decoder_target(target, decoder, idx)                      \
 	for (idx = 0, target = (decoder)->target[idx];                         \
 	     idx < (decoder)->nr_targets;                                      \
@@ -279,6 +286,59 @@ static bool region_xhb_config_valid(const struct cxl_region *region,
 	return true;
 }
 
+static struct cxl_dport *get_rp(struct cxl_memdev *ep)
+{
+	struct cxl_port *port, *parent_port = port = ep->port;
+	struct cxl_dport *dport;
+
+	while (!is_cxl_root(port)) {
+		parent_port = to_cxl_port(port->dev.parent);
+		if (parent_port->depth == 1)
+			list_for_each_entry(dport, &parent_port->dports, list)
+				if (dport->dport == port->uport->parent->parent)
+					return dport;
+		port = parent_port;
+	}
+
+	BUG();
+	return NULL;
+}
+
+static int get_num_root_ports(const struct cxl_region *region)
+{
+	struct cxl_memdev *endpoint;
+	struct cxl_dport *dport, *tmp;
+	int num_root_ports = 0;
+	LIST_HEAD(root_ports);
+	int idx;
+
+	for_each_cxl_endpoint(endpoint, region, idx) {
+		struct cxl_dport *root_port = get_rp(endpoint);
+
+		if (list_empty(&root_port->verify_link)) {
+			list_add_tail(&root_port->verify_link, &root_ports);
+			num_root_ports++;
+		}
+	}
+
+	list_for_each_entry_safe(dport, tmp, &root_ports, verify_link)
+		list_del_init(&dport->verify_link);
+
+	return num_root_ports;
+}
+
+static bool has_switch(const struct cxl_region *region)
+{
+	struct cxl_memdev *ep;
+	int i;
+
+	for_each_cxl_endpoint(ep, region, i)
+		if (ep->port->depth > 2)
+			return true;
+
+	return false;
+}
+
 /**
  * region_hb_rp_config_valid() - determine root port ordering is correct
  * @rootd: root decoder for this @region
@@ -292,7 +352,67 @@ static bool region_xhb_config_valid(const struct cxl_region *region,
 static bool region_hb_rp_config_valid(const struct cxl_region *region,
 				      const struct cxl_decoder *rootd)
 {
-	/* TODO: */
+	const int num_root_ports = get_num_root_ports(region);
+	struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
+	int hb_count, i;
+
+	hb_count = get_unique_hostbridges(region, hbs);
+
+	/*
+	 * Are all devices in this region on the same CXL Host Bridge
+	 * Root Port?
+	 */
+	if (num_root_ports == 1 && !has_switch(region))
+		return true;
+
+	for (i = 0; i < hb_count; i++) {
+		int idx, position_mask;
+		struct cxl_dport *rp;
+		struct cxl_port *hb;
+
+		/* Get next CXL Host Bridge this region spans */
+		hb = hbs[i];
+
+		/*
+		 * Calculate the position mask: NumRootPorts = 2^PositionMask
+		 * for this region.
+		 *
+		 * XXX: pos_mask is actually (1 << PositionMask)  - 1
+		 */
+		position_mask = (1 << (ilog2(num_root_ports))) - 1;
+
+		/*
+		 * Calculate the PortGrouping for each device on this CXL Host
+		 * Bridge Root Port:
+		 * PortGrouping = RegionLabel.Position & PositionMask
+		 */
+		list_for_each_entry(rp, &hb->dports, list) {
+			struct cxl_memdev *ep;
+			int port_grouping = -1;
+
+			for_each_cxl_endpoint_hb(ep, region, hb, idx) {
+				/* Only endpoints under the same root port */
+				if (get_rp(ep) != rp)
+					continue;
+
+				if (port_grouping == -1) {
+					port_grouping = idx & position_mask;
+					continue;
+				}
+
+				/*
+				 * Do all devices in the region connected to this CXL
+				 * Host Bridge Root Port have the same PortGrouping?
+				 */
+				if ((idx & position_mask) != port_grouping) {
+					trace_hb_rp_valid(region,
+							  "One or more devices are not connected to the correct Host Bridge Root Port\n");
+					return false;
+				}
+			}
+		}
+	}
+
 	return true;
 }
 
diff --git a/drivers/cxl/trace.h b/drivers/cxl/trace.h
index 4de47d1111ac..57fe9342817c 100644
--- a/drivers/cxl/trace.h
+++ b/drivers/cxl/trace.h
@@ -41,6 +41,9 @@ DEFINE_EVENT(cxl_region_template, allocation_failed,
 DEFINE_EVENT(cxl_region_template, xhb_valid,
 	     TP_PROTO(const struct cxl_region *region, char *status),
 	     TP_ARGS(region, status));
+DEFINE_EVENT(cxl_region_template, hb_rp_valid,
+	     TP_PROTO(const struct cxl_region *region, char *status),
+	     TP_ARGS(region, status));
 
 #endif /* if !defined (__CXL_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ) */
 
-- 
2.34.1

[PATCH 11/13] cxl/region: Add infrastructure for decoder programming

[Ben Widawsky]

There are 3 steps in handling region programming once it has been
configured by userspace.
1. Sanitize the parameters against the system.
2. Collect decoder resources from the topology
3. Program decoder resources

The infrastructure added here addresses #2. Two new APIs are introduced
to allow collecting and returning decoder resources. Additionally the
infrastructure includes two lists managed by the region driver, a staged
list, and a commit list. The staged list contains those collected in
step #2, and the commit list are all the decoders programmed in step #3.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/core/port.c   | 75 ++++++++++++++++++++++++++++--
 drivers/cxl/core/region.c |  2 +
 drivers/cxl/cxl.h         |  8 ++++
 drivers/cxl/cxlmem.h      |  7 +++
 drivers/cxl/port.c        | 42 ++++++++++++++++-
 drivers/cxl/region.c      | 97 +++++++++++++++++++++++++++++++++------
 drivers/cxl/region.h      |  5 ++
 7 files changed, 218 insertions(+), 18 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 99589f23f1ff..c9fd5bab1498 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -32,8 +32,6 @@ static DEFINE_XARRAY(cxl_root_buses);
 
 static void cxl_decoder_release(struct device *dev);
 
-static bool is_cxl_decoder(struct device *dev);
-
 static int decoder_match(struct device *dev, void *data)
 {
 	struct resource *theirs = (struct resource *)data;
@@ -291,10 +289,11 @@ bool is_root_decoder(struct device *dev)
 }
 EXPORT_SYMBOL_NS_GPL(is_root_decoder, CXL);
 
-static bool is_cxl_decoder(struct device *dev)
+bool is_cxl_decoder(struct device *dev)
 {
 	return dev->type->release == cxl_decoder_release;
 }
+EXPORT_SYMBOL_NS_GPL(is_cxl_decoder, CXL);
 
 struct cxl_decoder *to_cxl_decoder(struct device *dev)
 {
@@ -1040,6 +1039,8 @@ static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
 	cxld->target_type = CXL_DECODER_EXPANDER;
 	cxld->platform_res = (struct resource)DEFINE_RES_MEM(0, 0);
 
+	INIT_LIST_HEAD(&cxld->region_link);
+
 	ida_init(&cxld->region_ida);
 
 	return cxld;
@@ -1200,6 +1201,74 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_decoder_add, CXL);
 
+/**
+ * cxl_get_decoder() - Get an unused decoder from the port.
+ * @port: The port to obtain a decoder from.
+ *
+ * Region programming requires obtaining decoder resources from all ports that
+ * participate in the interleave set. This function shall be used to pull the
+ * decoder resource out of the list of available.
+ *
+ * Context: Process context. Takes and releases the device lock of the port.
+ *
+ * Return: A cxl_decoder that can be used for programming if successful, else a
+ * 	   negative error code.
+ */
+struct cxl_decoder *cxl_get_decoder(struct cxl_port *port)
+{
+	struct cxl_port_state *cxlps;
+	int dec;
+
+	cxlps = dev_get_drvdata(&port->dev);
+	if (dev_WARN_ONCE(&port->dev, !cxlps, "No port drvdata\n"))
+		return ERR_PTR(-ENXIO);
+
+	device_lock(&port->dev);
+	dec = find_first_bit(cxlps->decoders.free_mask, cxlps->decoders.count);
+	if (dec == cxlps->decoders.count) {
+		device_unlock(&port->dev);
+		return ERR_PTR(-ENODEV);
+	}
+
+	clear_bit(dec, cxlps->decoders.free_mask);
+	device_unlock(&port->dev);
+
+	return cxlps->decoders.cxld[dec];
+}
+EXPORT_SYMBOL_NS_GPL(cxl_get_decoder, CXL);
+
+/**
+ * cxl_put_decoder() - Return an inactive decoder to the port.
+ * @cxld: The decoder being returned.
+ */
+void cxl_put_decoder(struct cxl_decoder *cxld)
+{
+	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
+	struct cxl_port_state *cxlps;
+	int i;
+
+	cxlps = dev_get_drvdata(&port->dev);
+	if (dev_WARN_ONCE(&port->dev, !cxlps, "No port drvdata\n"))
+		return;
+
+	device_lock(&port->dev);
+
+	for (i = 0; i < CXL_DECODER_MAX_INSTANCES; i++) {
+		struct cxl_decoder *d = cxlps->decoders.cxld[i];
+
+		if (!d)
+			break;
+
+		if (d == cxld) {
+			set_bit(i, cxlps->decoders.free_mask);
+			break;
+		}
+	}
+
+	device_unlock(&port->dev);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_put_decoder, CXL);
+
 static void cxld_unregister(void *dev)
 {
 	struct cxl_decoder *plat_decoder, *cxld = to_cxl_decoder(dev);
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index de0514a85cf1..b15202732d55 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -435,6 +435,8 @@ struct cxl_region *cxl_alloc_region(struct cxl_decoder *cxld, int id)
 	if (!region)
 		return ERR_PTR(-ENOMEM);
 
+	INIT_LIST_HEAD(&region->staged_list);
+	INIT_LIST_HEAD(&region->commit_list);
 	region->id = id;
 
 	return region;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 762da3621ca4..79bca9a8246c 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -35,6 +35,8 @@
 #define   CXL_CM_CAP_CAP_ID_HDM 0x5
 #define   CXL_CM_CAP_CAP_HDM_VERSION 1
 
+#define CXL_DECODER_MAX_INSTANCES 10
+
 /* HDM decoders CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure */
 #define CXL_HDM_DECODER_CAP_OFFSET 0x0
 #define   CXL_HDM_DECODER_COUNT_MASK GENMASK(3, 0)
@@ -208,6 +210,7 @@ enum cxl_decoder_type {
  * @flags: memory type capabilities and locking
  * @region_ida: allocator for region ids.
  * @address_space: Used/free address space for regions.
+ * @region_link: This decoder's place on either the staged, or commit list.
  * @nr_targets: number of elements in @target
  * @target: active ordered target list in current decoder configuration
  */
@@ -224,6 +227,7 @@ struct cxl_decoder {
 	unsigned long flags;
 	struct ida region_ida;
 	struct gen_pool *address_space;
+	struct list_head region_link;
 	const int nr_targets;
 	struct cxl_dport *target[];
 };
@@ -277,6 +281,7 @@ struct cxl_walk_context {
  * @id: id for port device-name
  * @dports: cxl_dport instances referenced by decoders
  * @endpoints: cxl_ep instances, endpoints that are a descendant of this port
+ * @region_link: this port's node on the region's list of ports
  * @decoder_ida: allocator for decoder ids
  * @component_reg_phys: component register capability base address (optional)
  * @dead: last ep has been removed, force port re-creation
@@ -347,6 +352,8 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
 				   struct cxl_port *parent_port);
 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
 struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd);
+struct cxl_decoder *cxl_get_decoder(struct cxl_port *port);
+void cxl_put_decoder(struct cxl_decoder *cxld);
 bool schedule_cxl_rescan(void);
 
 struct cxl_dport *cxl_add_dport(struct cxl_port *port, struct device *dport,
@@ -359,6 +366,7 @@ struct cxl_dport *cxl_find_dport_by_dev(struct cxl_port *port,
 struct cxl_port *ep_find_cxl_port(struct cxl_memdev *cxlmd, unsigned int depth);
 
 struct cxl_decoder *to_cxl_decoder(struct device *dev);
+bool is_cxl_decoder(struct device *dev);
 bool is_root_decoder(struct device *dev);
 struct cxl_decoder *cxl_root_decoder_alloc(struct cxl_port *port,
 					   unsigned int nr_targets);
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 38d6129499c8..e4793e5f25bc 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -301,6 +301,13 @@ struct cxl_port_state {
 		unsigned int interleave11_8;
 		unsigned int interleave14_12;
 	} caps;
+
+	struct port_decoders {
+		unsigned long *free_mask;
+		int count;
+
+		struct cxl_decoder *cxld[CXL_DECODER_MAX_INSTANCES];
+	} decoders;
 };
 
 int devm_cxl_setup_hdm(struct cxl_port *port);
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index c10b462373db..ddf6e78189ee 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -41,10 +41,39 @@ static bool is_cxl_endpoint(struct cxl_port *port)
 	return is_cxl_memdev(port->uport);
 }
 
+static int count_decoders(struct device *dev, void *data)
+{
+	if (is_cxl_decoder(dev))
+		(*(int *)data)++;
+
+	return 0;
+}
+
+static int set_decoders(struct device *dev, void *data)
+{
+	struct cxl_port_state *cxlps;
+	int dec;
+
+	if (!is_cxl_decoder(dev))
+		return 0;
+
+	cxlps = data;
+	dec = find_first_zero_bit(cxlps->decoders.free_mask, cxlps->decoders.count);
+	if (dev_WARN_ONCE(dev, dec == cxlps->decoders.count,
+			  "Impossible decoder bitmap state\n"))
+		return 1;
+
+	set_bit(dec, cxlps->decoders.free_mask);
+	cxlps->decoders.cxld[dec] = to_cxl_decoder(dev);
+
+	return 0;
+}
+
 static int cxl_port_probe(struct device *dev)
 {
 	struct cxl_port *port = to_cxl_port(dev);
-	int rc;
+	struct cxl_port_state *cxlps;
+	int rc, decoder_count = 0;
 
 	if (!is_cxl_endpoint(port)) {
 		rc = cxl_port_enumerate_dports(port);
@@ -59,6 +88,8 @@ static int cxl_port_probe(struct device *dev)
 	if (rc)
 		return rc;
 
+	cxlps = dev_get_drvdata(dev);
+
 	if (is_cxl_endpoint(port))
 		rc = devm_cxl_enumerate_endpoint_decoders(port);
 	else
@@ -68,6 +99,15 @@ static int cxl_port_probe(struct device *dev)
 		return rc;
 	}
 
+	device_for_each_child(&port->dev, &decoder_count, count_decoders);
+
+	cxlps->decoders.free_mask =
+		devm_bitmap_zalloc(&port->dev, decoder_count, GFP_KERNEL);
+	cxlps->decoders.count = decoder_count;
+
+	if (device_for_each_child(&port->dev, cxlps, set_decoders))
+		return -ENXIO;
+
 	schedule_cxl_rescan();
 
 	return 0;
diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
index 87fdcf0049d8..eafd95419895 100644
--- a/drivers/cxl/region.c
+++ b/drivers/cxl/region.c
@@ -343,17 +343,20 @@ static bool has_switch(const struct cxl_region *region)
  * region_hb_rp_config_valid() - determine root port ordering is correct
  * @rootd: root decoder for this @region
  * @region: Region to validate
+ * @state_update: Whether or not to update port state
  *
  * The algorithm is outlined in 2.13.15 "Verify HB root port configuration
  * sequence" of the CXL Memory Device SW Guide (Rev1p0).
  *
  * Returns true if the configuration is valid.
  */
-static bool region_hb_rp_config_valid(const struct cxl_region *region,
-				      const struct cxl_decoder *rootd)
+static bool region_hb_rp_config_valid(struct cxl_region *region,
+				      const struct cxl_decoder *rootd,
+				      bool state_update)
 {
 	const int num_root_ports = get_num_root_ports(region);
 	struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
+	struct cxl_decoder *cxld, *c;
 	int hb_count, i;
 
 	hb_count = get_unique_hostbridges(region, hbs);
@@ -362,8 +365,24 @@ static bool region_hb_rp_config_valid(const struct cxl_region *region,
 	 * Are all devices in this region on the same CXL Host Bridge
 	 * Root Port?
 	 */
-	if (num_root_ports == 1 && !has_switch(region))
+	if (num_root_ports == 1 && !has_switch(region)) {
+		struct cxl_decoder *cxld;
+
+		if (!state_update)
+			return true;
+
+		cxld = cxl_get_decoder(hbs[0]);
+		if (!cxld) {
+			trace_hb_rp_valid(region, "Couldn't get decoder for region");
+			return false;
+		}
+
+		cxld->interleave_ways = 1;
+		cxld->interleave_granularity = region_ig(region);
+		cxld->target[0] = get_rp(region->config.targets[0]);
+		list_add_tail(&cxld->region_link, (struct list_head *)&region->staged_list);
 		return true;
+	}
 
 	for (i = 0; i < hb_count; i++) {
 		int idx, position_mask;
@@ -373,6 +392,14 @@ static bool region_hb_rp_config_valid(const struct cxl_region *region,
 		/* Get next CXL Host Bridge this region spans */
 		hb = hbs[i];
 
+		if (state_update) {
+			cxld = cxl_get_decoder(hbs[i]);
+			if (IS_ERR(cxld))
+				goto err;
+			cxld->interleave_ways = 0;
+		} else
+			cxld = NULL;
+
 		/*
 		 * Calculate the position mask: NumRootPorts = 2^PositionMask
 		 * for this region.
@@ -411,9 +438,18 @@ static bool region_hb_rp_config_valid(const struct cxl_region *region,
 				}
 			}
 		}
+		if (state_update)
+			list_add_tail(&cxld->region_link, &region->staged_list);
 	}
 
 	return true;
+
+err:
+	trace_hb_rp_valid(region, "Couldn't get decoder for region");
+	list_for_each_entry_safe(cxld, c, &region->staged_list, region_link)
+		cxl_put_decoder(cxld);
+
+	return false;
 }
 
 /**
@@ -429,7 +465,8 @@ static bool rootd_contains(const struct cxl_region *region,
 }
 
 static bool rootd_valid(const struct cxl_region *region,
-			const struct cxl_decoder *rootd)
+			const struct cxl_decoder *rootd,
+			bool state_update)
 {
 	const struct cxl_memdev *endpoint = region->config.targets[0];
 
@@ -442,7 +479,7 @@ static bool rootd_valid(const struct cxl_region *region,
 	if (!region_xhb_config_valid(region, rootd))
 		return false;
 
-	if (!region_hb_rp_config_valid(region, rootd))
+	if (!region_hb_rp_config_valid((struct cxl_region *)region, rootd, state_update))
 		return false;
 
 	if (!rootd_contains(region, rootd))
@@ -465,7 +502,7 @@ static int rootd_match(struct device *dev, void *data)
 	if (!is_root_decoder(dev))
 		return 0;
 
-	return !!rootd_valid(region, to_cxl_decoder(dev));
+	return !!rootd_valid(region, to_cxl_decoder(dev), false);
 }
 
 /*
@@ -488,12 +525,40 @@ static struct cxl_decoder *find_rootd(const struct cxl_region *region,
 	return NULL;
 }
 
-static int collect_ep_decoders(const struct cxl_region *region)
+static int collect_ep_decoders(struct cxl_region *region)
 {
-	/* TODO: */
+	struct cxl_memdev *ep;
+	int i;
+
+	for_each_cxl_endpoint(ep, region, i) {
+		struct cxl_decoder *cxld;
+
+		cxld = cxl_get_decoder(ep->port);
+		if (IS_ERR(cxld))
+			return PTR_ERR(cxld);
+
+		cxld->decoder_range = (struct range) {
+			.start = region->res->start,
+			.end = region->res->end
+		};
+		cxld->interleave_granularity = region_ig(region);
+		cxld->interleave_ways = region_ways(region);
+		list_add_tail(&cxld->region_link, &region->staged_list);
+	}
+
 	return 0;
 }
 
+static void cleanup_staged_decoders(struct cxl_region *region)
+{
+	struct cxl_decoder *cxld, *d;
+
+	list_for_each_entry_safe(cxld, d, &region->staged_list, region_link) {
+		cxl_put_decoder(cxld);
+		list_del_init(&cxld->region_link);
+	}
+}
+
 static int bind_region(const struct cxl_region *region)
 {
 	/* TODO: */
@@ -534,7 +599,7 @@ static int cxl_region_probe(struct device *dev)
 		return -ENXIO;
 	}
 
-	if (!rootd_valid(region, rootd)) {
+	if (!rootd_valid(region, rootd, true)) {
 		dev_err(dev, "Picked invalid rootd\n");
 		return -ENXIO;
 	}
@@ -549,14 +614,18 @@ static int cxl_region_probe(struct device *dev)
 
 	ret = collect_ep_decoders(region);
 	if (ret)
-		return ret;
+		goto err;
 
 	ret = bind_region(region);
-	if (!ret) {
-		region->active = true;
-		trace_region_activated(region, "");
-	}
+	if (ret)
+		goto err;
 
+	region->active = true;
+	trace_region_activated(region, "");
+	return 0;
+
+err:
+	cleanup_staged_decoders(region);
 	return ret;
 }
 
diff --git a/drivers/cxl/region.h b/drivers/cxl/region.h
index 97bb3f12f2fc..e865b8d6af2e 100644
--- a/drivers/cxl/region.h
+++ b/drivers/cxl/region.h
@@ -14,6 +14,9 @@
  * @list: Node in decoder's region list.
  * @res: Resource this region carves out of the platform decode range.
  * @active: If the region has been activated.
+ * @staged_list: All decoders staged for programming.
+ * @commit_list: All decoders programmed for this region's parameters.
+ *
  * @config: HDM decoder program config
  * @config.size: Size of the region determined from LSA or userspace.
  * @config.uuid: The UUID for this region.
@@ -27,6 +30,8 @@ struct cxl_region {
 	struct list_head list;
 	struct resource *res;
 	bool active;
+	struct list_head staged_list;
+	struct list_head commit_list;
 
 	struct {
 		u64 size;
-- 
2.34.1

[PATCH 12/13] cxl/region: Record host bridge target list

[Ben Widawsky]

Part of host bridge verification in the CXL Type 3 Memory Device
Software Guide calculates the host bridge interleave target list (6th
step in the flow chart). With host bridge verification already done, it
is trivial to store away the configuration information.

TODO: Needs support for switches (7th step in the flow chart).

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/region.c | 41 +++++++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
index eafd95419895..3120b65b0bc5 100644
--- a/drivers/cxl/region.c
+++ b/drivers/cxl/region.c
@@ -385,6 +385,7 @@ static bool region_hb_rp_config_valid(struct cxl_region *region,
 	}
 
 	for (i = 0; i < hb_count; i++) {
+		struct cxl_decoder *cxld;
 		int idx, position_mask;
 		struct cxl_dport *rp;
 		struct cxl_port *hb;
@@ -422,10 +423,8 @@ static bool region_hb_rp_config_valid(struct cxl_region *region,
 				if (get_rp(ep) != rp)
 					continue;
 
-				if (port_grouping == -1) {
+				if (port_grouping == -1)
 					port_grouping = idx & position_mask;
-					continue;
-				}
 
 				/*
 				 * Do all devices in the region connected to this CXL
@@ -436,10 +435,32 @@ static bool region_hb_rp_config_valid(struct cxl_region *region,
 							  "One or more devices are not connected to the correct Host Bridge Root Port\n");
 					return false;
 				}
+
+				if (!state_update)
+					continue;
+
+				if (dev_WARN_ONCE(&cxld->dev,
+						  port_grouping >= cxld->nr_targets,
+						  "Invalid port grouping %d/%d\n",
+						  port_grouping, cxld->nr_targets))
+					return false;
+
+				cxld->interleave_ways++;
+				cxld->target[port_grouping] = get_rp(ep);
 			}
 		}
-		if (state_update)
+
+		if (state_update) {
+			/* IG doesn't change across host bridges */
+			cxld->interleave_granularity = region_ig(region);
+
+			cxld->decoder_range = (struct range) {
+				.start = region->res->start,
+				.end = region->res->end
+			};
+
 			list_add_tail(&cxld->region_link, &region->staged_list);
+		}
 	}
 
 	return true;
@@ -464,7 +485,7 @@ static bool rootd_contains(const struct cxl_region *region,
 	return true;
 }
 
-static bool rootd_valid(const struct cxl_region *region,
+static bool rootd_valid(struct cxl_region *region,
 			const struct cxl_decoder *rootd,
 			bool state_update)
 {
@@ -489,20 +510,20 @@ static bool rootd_valid(const struct cxl_region *region,
 }
 
 struct rootd_context {
-	const struct cxl_region *region;
-	struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
+	struct cxl_region *region;
+	const struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
 	int count;
 };
 
 static int rootd_match(struct device *dev, void *data)
 {
 	struct rootd_context *ctx = (struct rootd_context *)data;
-	const struct cxl_region *region = ctx->region;
+	struct cxl_region *region = ctx->region;
 
 	if (!is_root_decoder(dev))
 		return 0;
 
-	return !!rootd_valid(region, to_cxl_decoder(dev), false);
+	return rootd_valid(region, to_cxl_decoder(dev), false);
 }
 
 /*
@@ -516,7 +537,7 @@ static struct cxl_decoder *find_rootd(const struct cxl_region *region,
 	struct rootd_context ctx;
 	struct device *ret;
 
-	ctx.region = region;
+	ctx.region = (struct cxl_region *)region;
 
 	ret = device_find_child((struct device *)&root->dev, &ctx, rootd_match);
 	if (ret)
-- 
2.34.1

[PATCH 13/13] cxl: Program decoders for regions

[Ben Widawsky]

Do the HDM decoder programming for all endpoints and host bridges in a
region. Switches are currently unimplemented.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/core/hdm.c | 198 +++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h      |   3 +
 drivers/cxl/region.c   |  39 +++++++-
 3 files changed, 237 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 44e48cea8cd4..c7293cbd8547 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -242,3 +242,201 @@ int devm_cxl_enumerate_switch_decoders(struct cxl_port *port)
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_switch_decoders, CXL);
+
+#define COMMIT_TIMEOUT_MS 10
+static int wait_for_commit(struct cxl_decoder *cxld)
+{
+	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
+	const unsigned long start = jiffies;
+	struct cxl_port_state *cxlps;
+	void __iomem *hdm_decoder;
+	unsigned long end = start;
+	u32 ctrl;
+
+	cxlps = dev_get_drvdata(&port->dev);
+	hdm_decoder = cxlps->regs.hdm_decoder;
+
+	do {
+		end = jiffies;
+		ctrl = readl(hdm_decoder +
+			     CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
+		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
+			break;
+
+		if (time_after(end, start + COMMIT_TIMEOUT_MS)) {
+			dev_err(&cxld->dev, "HDM decoder commit timeout %x\n", ctrl);
+			return -ETIMEDOUT;
+		}
+		if ((ctrl & CXL_HDM_DECODER0_CTRL_COMMIT_ERROR) != 0) {
+			dev_err(&cxld->dev, "HDM decoder commit error %x\n", ctrl);
+			return -ENXIO;
+		}
+	} while (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl));
+
+	return 0;
+}
+
+/**
+ * cxl_commit_decoder() - Program a configured cxl_decoder
+ * @cxld: The preconfigured cxl decoder.
+ *
+ * A cxl decoder that is to be committed should have been earmarked as enabled.
+ * This mechanism acts as a soft reservation on the decoder.
+ *
+ * Returns 0 if commit was successful, negative error code otherwise.
+ */
+int cxl_commit_decoder(struct cxl_decoder *cxld)
+{
+	u32 ctrl, tl_lo, tl_hi, base_lo, base_hi, size_lo, size_hi;
+	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
+	struct cxl_port_state *cxlps;
+	void __iomem *hdm_decoder;
+	int rc;
+
+	/*
+	 * Decoder flags are entirely software controlled and therefore this
+	 * case is purely a driver bug.
+	 */
+	if (dev_WARN_ONCE(&port->dev, (cxld->flags & CXL_DECODER_F_ENABLE) != 0,
+			  "Invalid %s enable state\n", dev_name(&cxld->dev)))
+		return -ENXIO;
+
+	cxlps = dev_get_drvdata(&port->dev);
+	hdm_decoder = cxlps->regs.hdm_decoder;
+	ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
+
+	/*
+	 * A decoder that's currently active cannot be changed without the
+	 * system being quiesced. While the driver should prevent against this,
+	 * for a variety of reasons the hardware might not be in sync with the
+	 * hardware and so, do not splat on error.
+	 */
+	size_hi = readl(hdm_decoder +
+			CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
+	size_lo =
+		readl(hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
+	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl) &&
+	    (size_lo + size_hi)) {
+		dev_err(&port->dev, "Tried to change an active decoder (%s)\n",
+			dev_name(&cxld->dev));
+		return -EBUSY;
+	}
+
+	u32p_replace_bits(&ctrl, 8 - ilog2(cxld->interleave_granularity),
+			  CXL_HDM_DECODER0_CTRL_IG_MASK);
+	u32p_replace_bits(&ctrl, ilog2(cxld->interleave_ways),
+			  CXL_HDM_DECODER0_CTRL_IW_MASK);
+	u32p_replace_bits(&ctrl, 1, CXL_HDM_DECODER0_CTRL_COMMIT);
+
+	/* TODO: set based on type */
+	u32p_replace_bits(&ctrl, 1, CXL_HDM_DECODER0_CTRL_TYPE);
+
+	base_lo = FIELD_PREP(GENMASK(31, 28),
+			     (u32)(cxld->decoder_range.start & 0xffffffff));
+	base_hi = FIELD_PREP(~0, (u32)(cxld->decoder_range.start >> 32));
+
+	size_lo = (u32)(range_len(&cxld->decoder_range)) & GENMASK(31, 28);
+	size_hi = (u32)((range_len(&cxld->decoder_range) >> 32));
+
+	if (cxld->nr_targets > 0) {
+		tl_lo |= FIELD_PREP(GENMASK(7, 0), cxld->target[0]->port_id);
+		if (cxld->interleave_ways > 1)
+			tl_lo |= FIELD_PREP(GENMASK(15, 8),
+					    cxld->target[1]->port_id);
+		if (cxld->interleave_ways > 2)
+			tl_lo |= FIELD_PREP(GENMASK(23, 16),
+					    cxld->target[2]->port_id);
+		if (cxld->interleave_ways > 3)
+			tl_lo |= FIELD_PREP(GENMASK(31, 24),
+					    cxld->target[3]->port_id);
+		if (cxld->interleave_ways > 4)
+			tl_hi |= FIELD_PREP(GENMASK(7, 0),
+					    cxld->target[4]->port_id);
+		if (cxld->interleave_ways > 5)
+			tl_hi |= FIELD_PREP(GENMASK(15, 8),
+					    cxld->target[5]->port_id);
+		if (cxld->interleave_ways > 6)
+			tl_hi |= FIELD_PREP(GENMASK(23, 16),
+					    cxld->target[6]->port_id);
+		if (cxld->interleave_ways > 7)
+			tl_hi |= FIELD_PREP(GENMASK(31, 24),
+					    cxld->target[7]->port_id);
+
+		writel(tl_hi, hdm_decoder + CXL_HDM_DECODER0_TL_HIGH(cxld->id));
+		writel(tl_lo, hdm_decoder + CXL_HDM_DECODER0_TL_LOW(cxld->id));
+	}
+
+	writel(size_hi,
+	       hdm_decoder + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
+	writel(size_lo,
+	       hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
+	writel(base_hi,
+	       hdm_decoder + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(cxld->id));
+	writel(base_lo,
+	       hdm_decoder + CXL_HDM_DECODER0_BASE_LOW_OFFSET(cxld->id));
+	writel(ctrl, hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
+
+	rc = wait_for_commit(cxld);
+	if (rc)
+		return rc;
+
+	cxld->flags |= CXL_DECODER_F_ENABLE;
+
+#define DPORT_TL_STR "%d %d %d %d %d %d %d %d"
+#define DPORT(i)                                                               \
+	(cxld->nr_targets && cxld->interleave_ways > (i)) ?                    \
+		      cxld->target[(i)]->port_id :                                   \
+		      -1
+#define DPORT_TL                                                               \
+	DPORT(0), DPORT(1), DPORT(2), DPORT(3), DPORT(4), DPORT(5), DPORT(6),  \
+		DPORT(7)
+
+	dev_dbg(&port->dev,
+		"%s\n\tBase %pa\n\tSize %llu\n\tIG %u\n\tIW %u\n\tTargetList: " DPORT_TL_STR,
+		dev_name(&cxld->dev), &cxld->decoder_range.start,
+		range_len(&cxld->decoder_range), cxld->interleave_granularity,
+		cxld->interleave_ways, DPORT_TL);
+#undef DPORT_TL
+#undef DPORT
+#undef DPORT_TL_STR
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cxl_commit_decoder);
+
+/**
+ * cxl_disable_decoder() - Disables a decoder
+ * @cxld: The active cxl decoder.
+ *
+ * CXL decoders (as of 2.0 spec) have no way to deactivate them other than to
+ * set the size of the HDM to 0. This function will clear all registers, and if
+ * the decoder is active, commit the 0'd out registers.
+ */
+void cxl_disable_decoder(struct cxl_decoder *cxld)
+{
+	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
+	struct cxl_port_state *cxlps;
+	void __iomem *hdm_decoder;
+	u32 ctrl;
+
+	cxlps = dev_get_drvdata(&port->dev);
+	hdm_decoder = cxlps->regs.hdm_decoder;
+	ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
+
+	if (dev_WARN_ONCE(&port->dev, (cxld->flags & CXL_DECODER_F_ENABLE) == 0,
+			  "Invalid decoder enable state\n"))
+		return;
+
+	/* There's no way to "uncommit" a committed decoder, only 0 size it */
+	writel(0, hdm_decoder + CXL_HDM_DECODER0_TL_HIGH(cxld->id));
+	writel(0, hdm_decoder + CXL_HDM_DECODER0_TL_LOW(cxld->id));
+	writel(0, hdm_decoder + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
+	writel(0, hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
+	writel(0, hdm_decoder + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(cxld->id));
+	writel(0, hdm_decoder + CXL_HDM_DECODER0_BASE_LOW_OFFSET(cxld->id));
+
+	/* If the device isn't actually active, just zero out all the fields */
+	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
+		writel(CXL_HDM_DECODER0_CTRL_COMMIT,
+		       hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
+}
+EXPORT_SYMBOL_GPL(cxl_disable_decoder);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 79bca9a8246c..587b75f7fa53 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -54,6 +54,7 @@
 #define   CXL_HDM_DECODER0_CTRL_IW_MASK GENMASK(7, 4)
 #define   CXL_HDM_DECODER0_CTRL_COMMIT BIT(9)
 #define   CXL_HDM_DECODER0_CTRL_COMMITTED BIT(10)
+#define   CXL_HDM_DECODER0_CTRL_COMMIT_ERROR BIT(11)
 #define   CXL_HDM_DECODER0_CTRL_TYPE BIT(12)
 #define CXL_HDM_DECODER0_TL_LOW(i) (0x20 * (i) + 0x24)
 #define CXL_HDM_DECODER0_TL_HIGH(i) (0x20 * (i) + 0x28)
@@ -364,6 +365,8 @@ int devm_cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
 struct cxl_dport *cxl_find_dport_by_dev(struct cxl_port *port,
 					const struct device *dev);
 struct cxl_port *ep_find_cxl_port(struct cxl_memdev *cxlmd, unsigned int depth);
+int cxl_commit_decoder(struct cxl_decoder *cxld);
+void cxl_disable_decoder(struct cxl_decoder *cxld);
 
 struct cxl_decoder *to_cxl_decoder(struct device *dev);
 bool is_cxl_decoder(struct device *dev);
diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
index 3120b65b0bc5..fd82d37ba573 100644
--- a/drivers/cxl/region.c
+++ b/drivers/cxl/region.c
@@ -580,10 +580,30 @@ static void cleanup_staged_decoders(struct cxl_region *region)
 	}
 }
 
-static int bind_region(const struct cxl_region *region)
+static int bind_region(struct cxl_region *region)
 {
-	/* TODO: */
-	return 0;
+	struct cxl_decoder *cxld, *d;
+	int rc;
+
+	list_for_each_entry_safe(cxld, d, &region->staged_list, region_link) {
+		rc = cxl_commit_decoder(cxld);
+		if (!rc)
+			list_move_tail(&cxld->region_link, &region->commit_list);
+		else
+			break;
+	}
+
+	list_for_each_entry_safe(cxld, d, &region->commit_list, region_link) {
+		if (rc)
+			cxl_disable_decoder(cxld);
+		list_del(&cxld->region_link);
+	}
+
+	if (rc)
+		cleanup_staged_decoders((struct cxl_region *)region);
+
+	BUG_ON(!list_empty(&region->staged_list));
+	return rc;
 }
 
 static int cxl_region_probe(struct device *dev)
@@ -650,9 +670,22 @@ static int cxl_region_probe(struct device *dev)
 	return ret;
 }
 
+static void cxl_region_remove(struct device *dev)
+{
+	struct cxl_region *region = to_cxl_region(dev);
+	struct cxl_decoder *cxld, *d;
+
+	list_for_each_entry_safe(cxld, d, &region->commit_list, region_link) {
+		cxl_disable_decoder(cxld);
+		list_del(&cxld->region_link);
+		cxl_put_decoder(cxld);
+	}
+}
+
 static struct cxl_driver cxl_region_driver = {
 	.name = "cxl_region",
 	.probe = cxl_region_probe,
+	.remove = cxl_region_remove,
 	.id = CXL_DEVICE_REGION,
 };
 module_cxl_driver(cxl_region_driver);
-- 
2.34.1

Re: [PATCH 09/13] cxl/region: Implement XHB verification

[Jonathan Cameron]

On Thu,  6 Jan 2022 16:37:52 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> Cross host bridge verification primarily determines if the requested
> interleave ordering can be achieved by the root decoder, which isn't as
> programmable as other decoders.
> 
> The algorithm implemented here is based on the CXL Type 3 Memory Device
> Software Guide, chapter 2.13.14
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

Hi Ben,

A few things I'm carrying 'fixes' for in here.

Jonathan

> ---
>  .clang-format        |  2 +
>  drivers/cxl/region.c | 89 +++++++++++++++++++++++++++++++++++++++++++-
>  drivers/cxl/trace.h  |  3 ++
>  3 files changed, 93 insertions(+), 1 deletion(-)
> 
> diff --git a/.clang-format b/.clang-format
> index 15d4eaabc6b5..55f628f21722 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -169,6 +169,8 @@ ForEachMacros:
>    - 'for_each_cpu_and'
>    - 'for_each_cpu_not'
>    - 'for_each_cpu_wrap'
> +  - 'for_each_cxl_decoder_target'
> +  - 'for_each_cxl_endpoint'
>    - 'for_each_dapm_widgets'
>    - 'for_each_dev_addr'
>    - 'for_each_dev_scope'
> diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> index c8e3c48dfbb9..ca559a4b5347 100644
> --- a/drivers/cxl/region.c
> +++ b/drivers/cxl/region.c
> @@ -28,6 +28,17 @@
>   */
>  
>  #define region_ways(region) ((region)->config.eniw)
> +#define region_ig(region) (ilog2((region)->config.ig))
> +
> +#define for_each_cxl_endpoint(ep, region, idx)                                 \
> +	for (idx = 0, ep = (region)->config.targets[idx];                      \
> +	     idx < region_ways(region);                                        \
> +	     idx++, ep = (region)->config.targets[idx])
> +
> +#define for_each_cxl_decoder_target(target, decoder, idx)                      \
> +	for (idx = 0, target = (decoder)->target[idx];                         \
> +	     idx < (decoder)->nr_targets;                                      \
> +	     idx++, target++)
>  
>  static struct cxl_decoder *rootd_from_region(struct cxl_region *r)
>  {
> @@ -175,6 +186,30 @@ static bool qtg_match(const struct cxl_decoder *rootd,
>  	return true;
>  }
>  
> +static int get_unique_hostbridges(const struct cxl_region *region,
> +				  struct cxl_port **hbs)
> +{
> +	struct cxl_memdev *ep;
> +	int i, hb_count = 0;
> +
> +	for_each_cxl_endpoint(ep, region, i) {
> +		struct cxl_port *hb = get_hostbridge(ep);
> +		bool found = false;
> +		int j;
> +
> +		BUG_ON(!hb);
> +
> +		for (j = 0; j < hb_count; j++) {
> +			if (hbs[j] == hb)
> +				found = true;
> +		}
> +		if (!found)
> +			hbs[hb_count++] = hb;
> +	}
> +
> +	return hb_count;
> +}
> +
>  /**
>   * region_xhb_config_valid() - determine cross host bridge validity
>   * @rootd: The root decoder to check against
> @@ -188,7 +223,59 @@ static bool qtg_match(const struct cxl_decoder *rootd,
>  static bool region_xhb_config_valid(const struct cxl_region *region,
>  				    const struct cxl_decoder *rootd)
>  {
> -	/* TODO: */
> +	struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
> +	int rootd_ig, i;
> +	struct cxl_dport *target;
> +
> +	/* Are all devices in this region on the same CXL host bridge */
> +	if (get_unique_hostbridges(region, hbs) == 1)
> +		return true;
> +
> +	rootd_ig = rootd->interleave_granularity;
> +
> +	/* CFMWS.HBIG >= Device.Label.IG */
> +	if (rootd_ig < region_ig(region)) {
> +		trace_xhb_valid(region,
> +				"granularity does not support the region interleave granularity\n");
> +		return false;
> +	}
> +
> +	/* ((2^(CFMWS.HBIG - Device.RLabel.IG) * (2^CFMWS.ENIW)) > Device.RLabel.NLabel) */
> +	if (1 << (rootd_ig - region_ig(region)) * (1 << rootd->interleave_ways) >

This maths isn't what the comment says it is.
((1 << (rootd_ig - region_ig(region))) * rootd->interleaveways)
so brackets needed to avoid 2^( all the rest) and rootd->interleave_ways seems to the
actual number of ways not the log2 of it.

That feeds through below.


> +	    region_ways(region)) {
> +		trace_xhb_valid(region,
> +				"granularity to device granularity ratio requires a larger number of devices than currently configured");
> +		return false;
> +	}
> +
> +	/* Check that endpoints are hooked up in the correct order */
> +	for_each_cxl_decoder_target(target, rootd, i) {
> +		struct cxl_memdev *endpoint = region->config.targets[i];
> +
> +		if (get_hostbridge(endpoint) != target->port) {

I think this should be
get_hostbridge(endpoint)->uport != target->dport

As it stands you are comparing the host bridge with the root object.

> +			trace_xhb_valid(region, "device ordering bad\n");
> +			return false;
> +		}
> +	}
> +
> +	/*
> +	 * CFMWS.InterleaveTargetList[n] must contain all devices, x where:
> +	 *	(Device[x],RegionLabel.Position >> (CFMWS.HBIG -
> +	 *	Device[x].RegionLabel.InterleaveGranularity)) &
> +	 *	((2^CFMWS.ENIW) - 1) = n
> +	 *
> +	 * Linux notes: All devices are known to have the same interleave
> +	 * granularity at this point.
> +	 */
> +	for_each_cxl_decoder_target(target, rootd, i) {
> +		if (((i >> (rootd_ig - region_ig(region)))) &
> +		    (((1 << rootd->interleave_ways) - 1) != target->port_id)) {
> +			trace_xhb_valid(region,
> +					"One or more devices are not connected to the correct hostbridge.");
> +			return false;
> +		}
> +	}
> +
>  	return true;
>  }
>  
> diff --git a/drivers/cxl/trace.h b/drivers/cxl/trace.h
> index a53f00ba5d0e..4de47d1111ac 100644
> --- a/drivers/cxl/trace.h
> +++ b/drivers/cxl/trace.h
> @@ -38,6 +38,9 @@ DEFINE_EVENT(cxl_region_template, sanitize_failed,
>  DEFINE_EVENT(cxl_region_template, allocation_failed,
>  	     TP_PROTO(const struct cxl_region *region, char *status),
>  	     TP_ARGS(region, status));
> +DEFINE_EVENT(cxl_region_template, xhb_valid,
> +	     TP_PROTO(const struct cxl_region *region, char *status),
> +	     TP_ARGS(region, status));
>  
>  #endif /* if !defined (__CXL_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ) */
>  

Re: [PATCH 09/13] cxl/region: Implement XHB verification

[Jonathan Cameron]

On Thu,  6 Jan 2022 16:37:52 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> Cross host bridge verification primarily determines if the requested
> interleave ordering can be achieved by the root decoder, which isn't as
> programmable as other decoders.
> 
> The algorithm implemented here is based on the CXL Type 3 Memory Device
> Software Guide, chapter 2.13.14
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

Trivial thing inline.

> diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> index c8e3c48dfbb9..ca559a4b5347 100644
> --- a/drivers/cxl/region.c
> +++ b/drivers/cxl/region.c
> @@ -28,6 +28,17 @@
>   */
>  
>  #define region_ways(region) ((region)->config.eniw)
> +#define region_ig(region) (ilog2((region)->config.ig))
> +
> +#define for_each_cxl_endpoint(ep, region, idx)                                 \
> +	for (idx = 0, ep = (region)->config.targets[idx];                      \
> +	     idx < region_ways(region);                                        \
> +	     idx++, ep = (region)->config.targets[idx])
> +
> +#define for_each_cxl_decoder_target(target, decoder, idx)                      \
> +	for (idx = 0, target = (decoder)->target[idx];                         \

As target is used too often in here, you'll replace it in ->target[idx] as well.
It happens to work today because the parameter always happens to be target

> +	     idx < (decoder)->nr_targets;                                      \
> +	     idx++, target++)
>  

Re: [PATCH 09/13] cxl/region: Implement XHB verification

[Jonathan Cameron]

On Fri, 7 Jan 2022 10:30:52 +0000
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Thu,  6 Jan 2022 16:37:52 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
> 
> > Cross host bridge verification primarily determines if the requested
> > interleave ordering can be achieved by the root decoder, which isn't as
> > programmable as other decoders.
> > 
> > The algorithm implemented here is based on the CXL Type 3 Memory Device
> > Software Guide, chapter 2.13.14
> > 
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>  
> 
> Trivial thing inline.
> 
> > diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> > index c8e3c48dfbb9..ca559a4b5347 100644
> > --- a/drivers/cxl/region.c
> > +++ b/drivers/cxl/region.c
> > @@ -28,6 +28,17 @@
> >   */
> >  
> >  #define region_ways(region) ((region)->config.eniw)
> > +#define region_ig(region) (ilog2((region)->config.ig))
> > +
> > +#define for_each_cxl_endpoint(ep, region, idx)                                 \
> > +	for (idx = 0, ep = (region)->config.targets[idx];                      \
> > +	     idx < region_ways(region);                                        \
> > +	     idx++, ep = (region)->config.targets[idx])
> > +
> > +#define for_each_cxl_decoder_target(target, decoder, idx)                      \
> > +	for (idx = 0, target = (decoder)->target[idx];                         \  
> 
> As target is used too often in here, you'll replace it in ->target[idx] as well.
> It happens to work today because the parameter always happens to be target
> 
> > +	     idx < (decoder)->nr_targets;                                      \
> > +	     idx++, target++)
I should have read the next few lines :)

target++ doesn't get (decoder)->target[idx] which is what we want - it indexes
off the end of a particular instance rather than through the array.

I'm guessing this was from my unclear comment yesterday. I should have spent
a little more time being explicit there.

Jonathan

> >    

Re: [PATCH 09/13] cxl/region: Implement XHB verification

[Jonathan Cameron]

On Fri, 7 Jan 2022 10:07:14 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Thu,  6 Jan 2022 16:37:52 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
> 
> > Cross host bridge verification primarily determines if the requested
> > interleave ordering can be achieved by the root decoder, which isn't as
> > programmable as other decoders.
> > 
> > The algorithm implemented here is based on the CXL Type 3 Memory Device
> > Software Guide, chapter 2.13.14
> > 
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>  
> 
> Hi Ben,
> 
> A few things I'm carrying 'fixes' for in here.
> 
> Jonathan
> 
> > ---
> >  .clang-format        |  2 +
> >  drivers/cxl/region.c | 89 +++++++++++++++++++++++++++++++++++++++++++-
> >  drivers/cxl/trace.h  |  3 ++
> >  3 files changed, 93 insertions(+), 1 deletion(-)
> > 
> > diff --git a/.clang-format b/.clang-format
> > index 15d4eaabc6b5..55f628f21722 100644
> > --- a/.clang-format
> > +++ b/.clang-format
> > @@ -169,6 +169,8 @@ ForEachMacros:
> >    - 'for_each_cpu_and'
> >    - 'for_each_cpu_not'
> >    - 'for_each_cpu_wrap'
> > +  - 'for_each_cxl_decoder_target'
> > +  - 'for_each_cxl_endpoint'
> >    - 'for_each_dapm_widgets'
> >    - 'for_each_dev_addr'
> >    - 'for_each_dev_scope'
> > diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> > index c8e3c48dfbb9..ca559a4b5347 100644
> > --- a/drivers/cxl/region.c
> > +++ b/drivers/cxl/region.c
> > @@ -28,6 +28,17 @@
> >   */
> >  
> >  #define region_ways(region) ((region)->config.eniw)
> > +#define region_ig(region) (ilog2((region)->config.ig))
> > +
> > +#define for_each_cxl_endpoint(ep, region, idx)                                 \
> > +	for (idx = 0, ep = (region)->config.targets[idx];                      \
> > +	     idx < region_ways(region);                                        \
> > +	     idx++, ep = (region)->config.targets[idx])
> > +
> > +#define for_each_cxl_decoder_target(target, decoder, idx)                      \
> > +	for (idx = 0, target = (decoder)->target[idx];                         \
> > +	     idx < (decoder)->nr_targets;                                      \
> > +	     idx++, target++)
> >  
> >  static struct cxl_decoder *rootd_from_region(struct cxl_region *r)
> >  {
> > @@ -175,6 +186,30 @@ static bool qtg_match(const struct cxl_decoder *rootd,
> >  	return true;
> >  }
> >  
> > +static int get_unique_hostbridges(const struct cxl_region *region,
> > +				  struct cxl_port **hbs)
> > +{
> > +	struct cxl_memdev *ep;
> > +	int i, hb_count = 0;
> > +
> > +	for_each_cxl_endpoint(ep, region, i) {
> > +		struct cxl_port *hb = get_hostbridge(ep);
> > +		bool found = false;
> > +		int j;
> > +
> > +		BUG_ON(!hb);
> > +
> > +		for (j = 0; j < hb_count; j++) {
> > +			if (hbs[j] == hb)
> > +				found = true;
> > +		}
> > +		if (!found)
> > +			hbs[hb_count++] = hb;
> > +	}
> > +
> > +	return hb_count;
> > +}
> > +
> >  /**
> >   * region_xhb_config_valid() - determine cross host bridge validity
> >   * @rootd: The root decoder to check against
> > @@ -188,7 +223,59 @@ static bool qtg_match(const struct cxl_decoder *rootd,
> >  static bool region_xhb_config_valid(const struct cxl_region *region,
> >  				    const struct cxl_decoder *rootd)
> >  {
> > -	/* TODO: */
> > +	struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
> > +	int rootd_ig, i;
> > +	struct cxl_dport *target;
> > +
> > +	/* Are all devices in this region on the same CXL host bridge */
> > +	if (get_unique_hostbridges(region, hbs) == 1)
> > +		return true;
> > +
> > +	rootd_ig = rootd->interleave_granularity;
> > +
> > +	/* CFMWS.HBIG >= Device.Label.IG */
> > +	if (rootd_ig < region_ig(region)) {
> > +		trace_xhb_valid(region,
> > +				"granularity does not support the region interleave granularity\n");
> > +		return false;
> > +	}
> > +
> > +	/* ((2^(CFMWS.HBIG - Device.RLabel.IG) * (2^CFMWS.ENIW)) > Device.RLabel.NLabel) */
> > +	if (1 << (rootd_ig - region_ig(region)) * (1 << rootd->interleave_ways) >  
> 
> This maths isn't what the comment says it is.
> ((1 << (rootd_ig - region_ig(region))) * rootd->interleaveways)
> so brackets needed to avoid 2^( all the rest) and rootd->interleave_ways seems to the
> actual number of ways not the log2 of it.
> 
> That feeds through below.
> 
> 
> > +	    region_ways(region)) {
> > +		trace_xhb_valid(region,
> > +				"granularity to device granularity ratio requires a larger number of devices than currently configured");
> > +		return false;
> > +	}
> > +
> > +	/* Check that endpoints are hooked up in the correct order */
> > +	for_each_cxl_decoder_target(target, rootd, i) {
> > +		struct cxl_memdev *endpoint = region->config.targets[i];
> > +
> > +		if (get_hostbridge(endpoint) != target->port) {  
> 
> I think this should be
> get_hostbridge(endpoint)->uport != target->dport
> 
> As it stands you are comparing the host bridge with the root object.

On closer inspection this code doesn't do what it is meant to do at all
if there are multiple EP below a given root bridge.

You'd expect multiple endpoints to match to each target->port.
Something along the lines of this should work:

        {
                struct cxl_memdev **epgroupstart = region->config.targets;
                struct cxl_memdev **endpoint;

                for_each_cxl_decoder_target(target, rootd, i) {
                        /* Find start of next endpoint group */
                        endpoint = epgroupstart;
                        if (*endpoint == NULL) {
                                printk("No endpoints under decoder target\n");
                                return false;
                        }
                        while (*epgroupstart &&
                                get_hostbridge(*endpoint) == get_hostbridge(*epgroupstart))
                                epgroupstart++;
                }
                if (*epgroupstart) {
                        printk("still some entries left. boom\n");
                        return false;
                }
        }

Only lightly tested with correct inputs...

Next up is figuring out why the EP HDM decoder won't commit. :)

Jonathan


> 
> > +			trace_xhb_valid(region, "device ordering bad\n");
> > +			return false;
> > +		}
> > +	}
> > +
> > +	/*
> > +	 * CFMWS.InterleaveTargetList[n] must contain all devices, x where:
> > +	 *	(Device[x],RegionLabel.Position >> (CFMWS.HBIG -
> > +	 *	Device[x].RegionLabel.InterleaveGranularity)) &
> > +	 *	((2^CFMWS.ENIW) - 1) = n
> > +	 *
> > +	 * Linux notes: All devices are known to have the same interleave
> > +	 * granularity at this point.
> > +	 */
> > +	for_each_cxl_decoder_target(target, rootd, i) {
> > +		if (((i >> (rootd_ig - region_ig(region)))) &
> > +		    (((1 << rootd->interleave_ways) - 1) != target->port_id)) {
> > +			trace_xhb_valid(region,
> > +					"One or more devices are not connected to the correct hostbridge.");
> > +			return false;
> > +		}
> > +	}
> > +
> >  	return true;
> >  }
> >  
> > diff --git a/drivers/cxl/trace.h b/drivers/cxl/trace.h
> > index a53f00ba5d0e..4de47d1111ac 100644
> > --- a/drivers/cxl/trace.h
> > +++ b/drivers/cxl/trace.h
> > @@ -38,6 +38,9 @@ DEFINE_EVENT(cxl_region_template, sanitize_failed,
> >  DEFINE_EVENT(cxl_region_template, allocation_failed,
> >  	     TP_PROTO(const struct cxl_region *region, char *status),
> >  	     TP_ARGS(region, status));
> > +DEFINE_EVENT(cxl_region_template, xhb_valid,
> > +	     TP_PROTO(const struct cxl_region *region, char *status),
> > +	     TP_ARGS(region, status));
> >  
> >  #endif /* if !defined (__CXL_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ) */
> >    
> 

Re: [PATCH 13/13] cxl: Program decoders for regions

[Jonathan Cameron]

On Thu,  6 Jan 2022 16:37:56 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> Do the HDM decoder programming for all endpoints and host bridges in a
> region. Switches are currently unimplemented.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  drivers/cxl/core/hdm.c | 198 +++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h      |   3 +
>  drivers/cxl/region.c   |  39 +++++++-
>  3 files changed, 237 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 44e48cea8cd4..c7293cbd8547 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -242,3 +242,201 @@ int devm_cxl_enumerate_switch_decoders(struct cxl_port *port)
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_switch_decoders, CXL);
> +
> +#define COMMIT_TIMEOUT_MS 10
> +static int wait_for_commit(struct cxl_decoder *cxld)
> +{
> +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> +	const unsigned long start = jiffies;
> +	struct cxl_port_state *cxlps;
> +	void __iomem *hdm_decoder;
> +	unsigned long end = start;
> +	u32 ctrl;
> +
> +	cxlps = dev_get_drvdata(&port->dev);
> +	hdm_decoder = cxlps->regs.hdm_decoder;
> +
> +	do {
> +		end = jiffies;
> +		ctrl = readl(hdm_decoder +
> +			     CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> +		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> +			break;
> +
> +		if (time_after(end, start + COMMIT_TIMEOUT_MS)) {
> +			dev_err(&cxld->dev, "HDM decoder commit timeout %x\n", ctrl);
> +			return -ETIMEDOUT;
> +		}
> +		if ((ctrl & CXL_HDM_DECODER0_CTRL_COMMIT_ERROR) != 0) {
> +			dev_err(&cxld->dev, "HDM decoder commit error %x\n", ctrl);
> +			return -ENXIO;
> +		}
> +	} while (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl));

Hi Ben,

Thanks for posting this btw - it's very helpful indeed for hammering out bugs
in the qemu code and to identify what isn't there yet.

Took me a while to work out how this seemed to succeed on QEMU with no
implementation of the commit for the host bridges (just adding that now ;)
Reason is this condition is inverse of what I think you intend.

Given you have an exit condition above this could probably just be a while (1)
loop.

Jonathan

> +
> +	return 0;
> +}
> +
> +/**
> + * cxl_commit_decoder() - Program a configured cxl_decoder
> + * @cxld: The preconfigured cxl decoder.
> + *
> + * A cxl decoder that is to be committed should have been earmarked as enabled.
> + * This mechanism acts as a soft reservation on the decoder.
> + *
> + * Returns 0 if commit was successful, negative error code otherwise.
> + */
> +int cxl_commit_decoder(struct cxl_decoder *cxld)
> +{
> +	u32 ctrl, tl_lo, tl_hi, base_lo, base_hi, size_lo, size_hi;
> +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> +	struct cxl_port_state *cxlps;
> +	void __iomem *hdm_decoder;
> +	int rc;
> +
> +	/*
> +	 * Decoder flags are entirely software controlled and therefore this
> +	 * case is purely a driver bug.
> +	 */
> +	if (dev_WARN_ONCE(&port->dev, (cxld->flags & CXL_DECODER_F_ENABLE) != 0,
> +			  "Invalid %s enable state\n", dev_name(&cxld->dev)))
> +		return -ENXIO;
> +
> +	cxlps = dev_get_drvdata(&port->dev);
> +	hdm_decoder = cxlps->regs.hdm_decoder;
> +	ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> +
> +	/*
> +	 * A decoder that's currently active cannot be changed without the
> +	 * system being quiesced. While the driver should prevent against this,
> +	 * for a variety of reasons the hardware might not be in sync with the
> +	 * hardware and so, do not splat on error.
> +	 */
> +	size_hi = readl(hdm_decoder +
> +			CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> +	size_lo =
> +		readl(hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> +	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl) &&
> +	    (size_lo + size_hi)) {
> +		dev_err(&port->dev, "Tried to change an active decoder (%s)\n",
> +			dev_name(&cxld->dev));
> +		return -EBUSY;
> +	}
> +
> +	u32p_replace_bits(&ctrl, 8 - ilog2(cxld->interleave_granularity),
> +			  CXL_HDM_DECODER0_CTRL_IG_MASK);
> +	u32p_replace_bits(&ctrl, ilog2(cxld->interleave_ways),
> +			  CXL_HDM_DECODER0_CTRL_IW_MASK);
> +	u32p_replace_bits(&ctrl, 1, CXL_HDM_DECODER0_CTRL_COMMIT);
> +
> +	/* TODO: set based on type */
> +	u32p_replace_bits(&ctrl, 1, CXL_HDM_DECODER0_CTRL_TYPE);
> +
> +	base_lo = FIELD_PREP(GENMASK(31, 28),
> +			     (u32)(cxld->decoder_range.start & 0xffffffff));
> +	base_hi = FIELD_PREP(~0, (u32)(cxld->decoder_range.start >> 32));
> +
> +	size_lo = (u32)(range_len(&cxld->decoder_range)) & GENMASK(31, 28);
> +	size_hi = (u32)((range_len(&cxld->decoder_range) >> 32));
> +
> +	if (cxld->nr_targets > 0) {
> +		tl_lo |= FIELD_PREP(GENMASK(7, 0), cxld->target[0]->port_id);
> +		if (cxld->interleave_ways > 1)
> +			tl_lo |= FIELD_PREP(GENMASK(15, 8),
> +					    cxld->target[1]->port_id);
> +		if (cxld->interleave_ways > 2)
> +			tl_lo |= FIELD_PREP(GENMASK(23, 16),
> +					    cxld->target[2]->port_id);
> +		if (cxld->interleave_ways > 3)
> +			tl_lo |= FIELD_PREP(GENMASK(31, 24),
> +					    cxld->target[3]->port_id);
> +		if (cxld->interleave_ways > 4)
> +			tl_hi |= FIELD_PREP(GENMASK(7, 0),
> +					    cxld->target[4]->port_id);
> +		if (cxld->interleave_ways > 5)
> +			tl_hi |= FIELD_PREP(GENMASK(15, 8),
> +					    cxld->target[5]->port_id);
> +		if (cxld->interleave_ways > 6)
> +			tl_hi |= FIELD_PREP(GENMASK(23, 16),
> +					    cxld->target[6]->port_id);
> +		if (cxld->interleave_ways > 7)
> +			tl_hi |= FIELD_PREP(GENMASK(31, 24),
> +					    cxld->target[7]->port_id);
> +
> +		writel(tl_hi, hdm_decoder + CXL_HDM_DECODER0_TL_HIGH(cxld->id));
> +		writel(tl_lo, hdm_decoder + CXL_HDM_DECODER0_TL_LOW(cxld->id));
> +	}
> +
> +	writel(size_hi,
> +	       hdm_decoder + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> +	writel(size_lo,
> +	       hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> +	writel(base_hi,
> +	       hdm_decoder + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(cxld->id));
> +	writel(base_lo,
> +	       hdm_decoder + CXL_HDM_DECODER0_BASE_LOW_OFFSET(cxld->id));
> +	writel(ctrl, hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> +
> +	rc = wait_for_commit(cxld);
> +	if (rc)
> +		return rc;
> +
> +	cxld->flags |= CXL_DECODER_F_ENABLE;
> +
> +#define DPORT_TL_STR "%d %d %d %d %d %d %d %d"
> +#define DPORT(i)                                                               \
> +	(cxld->nr_targets && cxld->interleave_ways > (i)) ?                    \
> +		      cxld->target[(i)]->port_id :                                   \
> +		      -1
> +#define DPORT_TL                                                               \
> +	DPORT(0), DPORT(1), DPORT(2), DPORT(3), DPORT(4), DPORT(5), DPORT(6),  \
> +		DPORT(7)
> +
> +	dev_dbg(&port->dev,
> +		"%s\n\tBase %pa\n\tSize %llu\n\tIG %u\n\tIW %u\n\tTargetList: " DPORT_TL_STR,
> +		dev_name(&cxld->dev), &cxld->decoder_range.start,
> +		range_len(&cxld->decoder_range), cxld->interleave_granularity,
> +		cxld->interleave_ways, DPORT_TL);
> +#undef DPORT_TL
> +#undef DPORT
> +#undef DPORT_TL_STR
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cxl_commit_decoder);
> +
> +/**
> + * cxl_disable_decoder() - Disables a decoder
> + * @cxld: The active cxl decoder.
> + *
> + * CXL decoders (as of 2.0 spec) have no way to deactivate them other than to
> + * set the size of the HDM to 0. This function will clear all registers, and if
> + * the decoder is active, commit the 0'd out registers.
> + */
> +void cxl_disable_decoder(struct cxl_decoder *cxld)
> +{
> +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> +	struct cxl_port_state *cxlps;
> +	void __iomem *hdm_decoder;
> +	u32 ctrl;
> +
> +	cxlps = dev_get_drvdata(&port->dev);
> +	hdm_decoder = cxlps->regs.hdm_decoder;
> +	ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> +
> +	if (dev_WARN_ONCE(&port->dev, (cxld->flags & CXL_DECODER_F_ENABLE) == 0,
> +			  "Invalid decoder enable state\n"))
> +		return;
> +
> +	/* There's no way to "uncommit" a committed decoder, only 0 size it */
> +	writel(0, hdm_decoder + CXL_HDM_DECODER0_TL_HIGH(cxld->id));
> +	writel(0, hdm_decoder + CXL_HDM_DECODER0_TL_LOW(cxld->id));
> +	writel(0, hdm_decoder + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> +	writel(0, hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> +	writel(0, hdm_decoder + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(cxld->id));
> +	writel(0, hdm_decoder + CXL_HDM_DECODER0_BASE_LOW_OFFSET(cxld->id));
> +
> +	/* If the device isn't actually active, just zero out all the fields */
> +	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> +		writel(CXL_HDM_DECODER0_CTRL_COMMIT,
> +		       hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> +}
> +EXPORT_SYMBOL_GPL(cxl_disable_decoder);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 79bca9a8246c..587b75f7fa53 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -54,6 +54,7 @@
>  #define   CXL_HDM_DECODER0_CTRL_IW_MASK GENMASK(7, 4)
>  #define   CXL_HDM_DECODER0_CTRL_COMMIT BIT(9)
>  #define   CXL_HDM_DECODER0_CTRL_COMMITTED BIT(10)
> +#define   CXL_HDM_DECODER0_CTRL_COMMIT_ERROR BIT(11)
>  #define   CXL_HDM_DECODER0_CTRL_TYPE BIT(12)
>  #define CXL_HDM_DECODER0_TL_LOW(i) (0x20 * (i) + 0x24)
>  #define CXL_HDM_DECODER0_TL_HIGH(i) (0x20 * (i) + 0x28)
> @@ -364,6 +365,8 @@ int devm_cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
>  struct cxl_dport *cxl_find_dport_by_dev(struct cxl_port *port,
>  					const struct device *dev);
>  struct cxl_port *ep_find_cxl_port(struct cxl_memdev *cxlmd, unsigned int depth);
> +int cxl_commit_decoder(struct cxl_decoder *cxld);
> +void cxl_disable_decoder(struct cxl_decoder *cxld);
>  
>  struct cxl_decoder *to_cxl_decoder(struct device *dev);
>  bool is_cxl_decoder(struct device *dev);
> diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> index 3120b65b0bc5..fd82d37ba573 100644
> --- a/drivers/cxl/region.c
> +++ b/drivers/cxl/region.c
> @@ -580,10 +580,30 @@ static void cleanup_staged_decoders(struct cxl_region *region)
>  	}
>  }
>  
> -static int bind_region(const struct cxl_region *region)
> +static int bind_region(struct cxl_region *region)
>  {
> -	/* TODO: */
> -	return 0;
> +	struct cxl_decoder *cxld, *d;
> +	int rc;
> +
> +	list_for_each_entry_safe(cxld, d, &region->staged_list, region_link) {
> +		rc = cxl_commit_decoder(cxld);
> +		if (!rc)
> +			list_move_tail(&cxld->region_link, &region->commit_list);
> +		else
> +			break;
> +	}
> +
> +	list_for_each_entry_safe(cxld, d, &region->commit_list, region_link) {
> +		if (rc)
> +			cxl_disable_decoder(cxld);
> +		list_del(&cxld->region_link);
> +	}
> +
> +	if (rc)
> +		cleanup_staged_decoders((struct cxl_region *)region);
> +
> +	BUG_ON(!list_empty(&region->staged_list));
> +	return rc;
>  }
>  
>  static int cxl_region_probe(struct device *dev)
> @@ -650,9 +670,22 @@ static int cxl_region_probe(struct device *dev)
>  	return ret;
>  }
>  
> +static void cxl_region_remove(struct device *dev)
> +{
> +	struct cxl_region *region = to_cxl_region(dev);
> +	struct cxl_decoder *cxld, *d;
> +
> +	list_for_each_entry_safe(cxld, d, &region->commit_list, region_link) {
> +		cxl_disable_decoder(cxld);
> +		list_del(&cxld->region_link);
> +		cxl_put_decoder(cxld);
> +	}
> +}
> +
>  static struct cxl_driver cxl_region_driver = {
>  	.name = "cxl_region",
>  	.probe = cxl_region_probe,
> +	.remove = cxl_region_remove,
>  	.id = CXL_DEVICE_REGION,
>  };
>  module_cxl_driver(cxl_region_driver);

Re: [PATCH 09/13] cxl/region: Implement XHB verification

[Ben Widawsky]

On 22-01-07 10:38:27, Jonathan Cameron wrote:
> On Fri, 7 Jan 2022 10:30:52 +0000
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> > On Thu,  6 Jan 2022 16:37:52 -0800
> > Ben Widawsky <ben.widawsky@intel.com> wrote:
> > 
> > > Cross host bridge verification primarily determines if the requested
> > > interleave ordering can be achieved by the root decoder, which isn't as
> > > programmable as other decoders.
> > > 
> > > The algorithm implemented here is based on the CXL Type 3 Memory Device
> > > Software Guide, chapter 2.13.14
> > > 
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>  
> > 
> > Trivial thing inline.
> > 
> > > diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> > > index c8e3c48dfbb9..ca559a4b5347 100644
> > > --- a/drivers/cxl/region.c
> > > +++ b/drivers/cxl/region.c
> > > @@ -28,6 +28,17 @@
> > >   */
> > >  
> > >  #define region_ways(region) ((region)->config.eniw)
> > > +#define region_ig(region) (ilog2((region)->config.ig))
> > > +
> > > +#define for_each_cxl_endpoint(ep, region, idx)                                 \
> > > +	for (idx = 0, ep = (region)->config.targets[idx];                      \
> > > +	     idx < region_ways(region);                                        \
> > > +	     idx++, ep = (region)->config.targets[idx])
> > > +
> > > +#define for_each_cxl_decoder_target(target, decoder, idx)                      \
> > > +	for (idx = 0, target = (decoder)->target[idx];                         \  
> > 
> > As target is used too often in here, you'll replace it in ->target[idx] as well.
> > It happens to work today because the parameter always happens to be target
> > 
> > > +	     idx < (decoder)->nr_targets;                                      \
> > > +	     idx++, target++)
> I should have read the next few lines :)
> 
> target++ doesn't get (decoder)->target[idx] which is what we want - it indexes
> off the end of a particular instance rather than through the array.
> 
> I'm guessing this was from my unclear comment yesterday. I should have spent
> a little more time being explicit there.
> 
> Jonathan

Yeah. I was working quickly because I ended up losing childcare for this week
and wanted to get this out ASAP. I'll fix it up on the next round.

> 
> > >    
> 

Re: [PATCH 13/13] cxl: Program decoders for regions

[Ben Widawsky]

On 22-01-07 16:18:12, Jonathan Cameron wrote:
> On Thu,  6 Jan 2022 16:37:56 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
> 
> > Do the HDM decoder programming for all endpoints and host bridges in a
> > region. Switches are currently unimplemented.
> > 
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> >  drivers/cxl/core/hdm.c | 198 +++++++++++++++++++++++++++++++++++++++++
> >  drivers/cxl/cxl.h      |   3 +
> >  drivers/cxl/region.c   |  39 +++++++-
> >  3 files changed, 237 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 44e48cea8cd4..c7293cbd8547 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -242,3 +242,201 @@ int devm_cxl_enumerate_switch_decoders(struct cxl_port *port)
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_switch_decoders, CXL);
> > +
> > +#define COMMIT_TIMEOUT_MS 10
> > +static int wait_for_commit(struct cxl_decoder *cxld)
> > +{
> > +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > +	const unsigned long start = jiffies;
> > +	struct cxl_port_state *cxlps;
> > +	void __iomem *hdm_decoder;
> > +	unsigned long end = start;
> > +	u32 ctrl;
> > +
> > +	cxlps = dev_get_drvdata(&port->dev);
> > +	hdm_decoder = cxlps->regs.hdm_decoder;
> > +
> > +	do {
> > +		end = jiffies;
> > +		ctrl = readl(hdm_decoder +
> > +			     CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > +		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> > +			break;
> > +
> > +		if (time_after(end, start + COMMIT_TIMEOUT_MS)) {
> > +			dev_err(&cxld->dev, "HDM decoder commit timeout %x\n", ctrl);
> > +			return -ETIMEDOUT;
> > +		}
> > +		if ((ctrl & CXL_HDM_DECODER0_CTRL_COMMIT_ERROR) != 0) {
> > +			dev_err(&cxld->dev, "HDM decoder commit error %x\n", ctrl);
> > +			return -ENXIO;
> > +		}
> > +	} while (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl));
> 
> Hi Ben,
> 
> Thanks for posting this btw - it's very helpful indeed for hammering out bugs
> in the qemu code and to identify what isn't there yet.
> 
> Took me a while to work out how this seemed to succeed on QEMU with no
> implementation of the commit for the host bridges (just adding that now ;)
> Reason is this condition is inverse of what I think you intend.
> 
> Given you have an exit condition above this could probably just be a while (1)
> loop.
> 
> Jonathan

Quick response without looking at code. I thought I did implement this in QEMU,
but you're correct that the logic is inverted. If you have a QEMU branch for me
to use when I fix this, please let me know.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * cxl_commit_decoder() - Program a configured cxl_decoder
> > + * @cxld: The preconfigured cxl decoder.
> > + *
> > + * A cxl decoder that is to be committed should have been earmarked as enabled.
> > + * This mechanism acts as a soft reservation on the decoder.
> > + *
> > + * Returns 0 if commit was successful, negative error code otherwise.
> > + */
> > +int cxl_commit_decoder(struct cxl_decoder *cxld)
> > +{
> > +	u32 ctrl, tl_lo, tl_hi, base_lo, base_hi, size_lo, size_hi;
> > +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > +	struct cxl_port_state *cxlps;
> > +	void __iomem *hdm_decoder;
> > +	int rc;
> > +
> > +	/*
> > +	 * Decoder flags are entirely software controlled and therefore this
> > +	 * case is purely a driver bug.
> > +	 */
> > +	if (dev_WARN_ONCE(&port->dev, (cxld->flags & CXL_DECODER_F_ENABLE) != 0,
> > +			  "Invalid %s enable state\n", dev_name(&cxld->dev)))
> > +		return -ENXIO;
> > +
> > +	cxlps = dev_get_drvdata(&port->dev);
> > +	hdm_decoder = cxlps->regs.hdm_decoder;
> > +	ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > +
> > +	/*
> > +	 * A decoder that's currently active cannot be changed without the
> > +	 * system being quiesced. While the driver should prevent against this,
> > +	 * for a variety of reasons the hardware might not be in sync with the
> > +	 * hardware and so, do not splat on error.
> > +	 */
> > +	size_hi = readl(hdm_decoder +
> > +			CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> > +	size_lo =
> > +		readl(hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> > +	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl) &&
> > +	    (size_lo + size_hi)) {
> > +		dev_err(&port->dev, "Tried to change an active decoder (%s)\n",
> > +			dev_name(&cxld->dev));
> > +		return -EBUSY;
> > +	}
> > +
> > +	u32p_replace_bits(&ctrl, 8 - ilog2(cxld->interleave_granularity),
> > +			  CXL_HDM_DECODER0_CTRL_IG_MASK);
> > +	u32p_replace_bits(&ctrl, ilog2(cxld->interleave_ways),
> > +			  CXL_HDM_DECODER0_CTRL_IW_MASK);
> > +	u32p_replace_bits(&ctrl, 1, CXL_HDM_DECODER0_CTRL_COMMIT);
> > +
> > +	/* TODO: set based on type */
> > +	u32p_replace_bits(&ctrl, 1, CXL_HDM_DECODER0_CTRL_TYPE);
> > +
> > +	base_lo = FIELD_PREP(GENMASK(31, 28),
> > +			     (u32)(cxld->decoder_range.start & 0xffffffff));
> > +	base_hi = FIELD_PREP(~0, (u32)(cxld->decoder_range.start >> 32));
> > +
> > +	size_lo = (u32)(range_len(&cxld->decoder_range)) & GENMASK(31, 28);
> > +	size_hi = (u32)((range_len(&cxld->decoder_range) >> 32));
> > +
> > +	if (cxld->nr_targets > 0) {
> > +		tl_lo |= FIELD_PREP(GENMASK(7, 0), cxld->target[0]->port_id);
> > +		if (cxld->interleave_ways > 1)
> > +			tl_lo |= FIELD_PREP(GENMASK(15, 8),
> > +					    cxld->target[1]->port_id);
> > +		if (cxld->interleave_ways > 2)
> > +			tl_lo |= FIELD_PREP(GENMASK(23, 16),
> > +					    cxld->target[2]->port_id);
> > +		if (cxld->interleave_ways > 3)
> > +			tl_lo |= FIELD_PREP(GENMASK(31, 24),
> > +					    cxld->target[3]->port_id);
> > +		if (cxld->interleave_ways > 4)
> > +			tl_hi |= FIELD_PREP(GENMASK(7, 0),
> > +					    cxld->target[4]->port_id);
> > +		if (cxld->interleave_ways > 5)
> > +			tl_hi |= FIELD_PREP(GENMASK(15, 8),
> > +					    cxld->target[5]->port_id);
> > +		if (cxld->interleave_ways > 6)
> > +			tl_hi |= FIELD_PREP(GENMASK(23, 16),
> > +					    cxld->target[6]->port_id);
> > +		if (cxld->interleave_ways > 7)
> > +			tl_hi |= FIELD_PREP(GENMASK(31, 24),
> > +					    cxld->target[7]->port_id);
> > +
> > +		writel(tl_hi, hdm_decoder + CXL_HDM_DECODER0_TL_HIGH(cxld->id));
> > +		writel(tl_lo, hdm_decoder + CXL_HDM_DECODER0_TL_LOW(cxld->id));
> > +	}
> > +
> > +	writel(size_hi,
> > +	       hdm_decoder + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> > +	writel(size_lo,
> > +	       hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> > +	writel(base_hi,
> > +	       hdm_decoder + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(cxld->id));
> > +	writel(base_lo,
> > +	       hdm_decoder + CXL_HDM_DECODER0_BASE_LOW_OFFSET(cxld->id));
> > +	writel(ctrl, hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > +
> > +	rc = wait_for_commit(cxld);
> > +	if (rc)
> > +		return rc;
> > +
> > +	cxld->flags |= CXL_DECODER_F_ENABLE;
> > +
> > +#define DPORT_TL_STR "%d %d %d %d %d %d %d %d"
> > +#define DPORT(i)                                                               \
> > +	(cxld->nr_targets && cxld->interleave_ways > (i)) ?                    \
> > +		      cxld->target[(i)]->port_id :                                   \
> > +		      -1
> > +#define DPORT_TL                                                               \
> > +	DPORT(0), DPORT(1), DPORT(2), DPORT(3), DPORT(4), DPORT(5), DPORT(6),  \
> > +		DPORT(7)
> > +
> > +	dev_dbg(&port->dev,
> > +		"%s\n\tBase %pa\n\tSize %llu\n\tIG %u\n\tIW %u\n\tTargetList: " DPORT_TL_STR,
> > +		dev_name(&cxld->dev), &cxld->decoder_range.start,
> > +		range_len(&cxld->decoder_range), cxld->interleave_granularity,
> > +		cxld->interleave_ways, DPORT_TL);
> > +#undef DPORT_TL
> > +#undef DPORT
> > +#undef DPORT_TL_STR
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(cxl_commit_decoder);
> > +
> > +/**
> > + * cxl_disable_decoder() - Disables a decoder
> > + * @cxld: The active cxl decoder.
> > + *
> > + * CXL decoders (as of 2.0 spec) have no way to deactivate them other than to
> > + * set the size of the HDM to 0. This function will clear all registers, and if
> > + * the decoder is active, commit the 0'd out registers.
> > + */
> > +void cxl_disable_decoder(struct cxl_decoder *cxld)
> > +{
> > +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > +	struct cxl_port_state *cxlps;
> > +	void __iomem *hdm_decoder;
> > +	u32 ctrl;
> > +
> > +	cxlps = dev_get_drvdata(&port->dev);
> > +	hdm_decoder = cxlps->regs.hdm_decoder;
> > +	ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > +
> > +	if (dev_WARN_ONCE(&port->dev, (cxld->flags & CXL_DECODER_F_ENABLE) == 0,
> > +			  "Invalid decoder enable state\n"))
> > +		return;
> > +
> > +	/* There's no way to "uncommit" a committed decoder, only 0 size it */
> > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_TL_HIGH(cxld->id));
> > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_TL_LOW(cxld->id));
> > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(cxld->id));
> > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_BASE_LOW_OFFSET(cxld->id));
> > +
> > +	/* If the device isn't actually active, just zero out all the fields */
> > +	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> > +		writel(CXL_HDM_DECODER0_CTRL_COMMIT,
> > +		       hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > +}
> > +EXPORT_SYMBOL_GPL(cxl_disable_decoder);
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 79bca9a8246c..587b75f7fa53 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -54,6 +54,7 @@
> >  #define   CXL_HDM_DECODER0_CTRL_IW_MASK GENMASK(7, 4)
> >  #define   CXL_HDM_DECODER0_CTRL_COMMIT BIT(9)
> >  #define   CXL_HDM_DECODER0_CTRL_COMMITTED BIT(10)
> > +#define   CXL_HDM_DECODER0_CTRL_COMMIT_ERROR BIT(11)
> >  #define   CXL_HDM_DECODER0_CTRL_TYPE BIT(12)
> >  #define CXL_HDM_DECODER0_TL_LOW(i) (0x20 * (i) + 0x24)
> >  #define CXL_HDM_DECODER0_TL_HIGH(i) (0x20 * (i) + 0x28)
> > @@ -364,6 +365,8 @@ int devm_cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
> >  struct cxl_dport *cxl_find_dport_by_dev(struct cxl_port *port,
> >  					const struct device *dev);
> >  struct cxl_port *ep_find_cxl_port(struct cxl_memdev *cxlmd, unsigned int depth);
> > +int cxl_commit_decoder(struct cxl_decoder *cxld);
> > +void cxl_disable_decoder(struct cxl_decoder *cxld);
> >  
> >  struct cxl_decoder *to_cxl_decoder(struct device *dev);
> >  bool is_cxl_decoder(struct device *dev);
> > diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> > index 3120b65b0bc5..fd82d37ba573 100644
> > --- a/drivers/cxl/region.c
> > +++ b/drivers/cxl/region.c
> > @@ -580,10 +580,30 @@ static void cleanup_staged_decoders(struct cxl_region *region)
> >  	}
> >  }
> >  
> > -static int bind_region(const struct cxl_region *region)
> > +static int bind_region(struct cxl_region *region)
> >  {
> > -	/* TODO: */
> > -	return 0;
> > +	struct cxl_decoder *cxld, *d;
> > +	int rc;
> > +
> > +	list_for_each_entry_safe(cxld, d, &region->staged_list, region_link) {
> > +		rc = cxl_commit_decoder(cxld);
> > +		if (!rc)
> > +			list_move_tail(&cxld->region_link, &region->commit_list);
> > +		else
> > +			break;
> > +	}
> > +
> > +	list_for_each_entry_safe(cxld, d, &region->commit_list, region_link) {
> > +		if (rc)
> > +			cxl_disable_decoder(cxld);
> > +		list_del(&cxld->region_link);
> > +	}
> > +
> > +	if (rc)
> > +		cleanup_staged_decoders((struct cxl_region *)region);
> > +
> > +	BUG_ON(!list_empty(&region->staged_list));
> > +	return rc;
> >  }
> >  
> >  static int cxl_region_probe(struct device *dev)
> > @@ -650,9 +670,22 @@ static int cxl_region_probe(struct device *dev)
> >  	return ret;
> >  }
> >  
> > +static void cxl_region_remove(struct device *dev)
> > +{
> > +	struct cxl_region *region = to_cxl_region(dev);
> > +	struct cxl_decoder *cxld, *d;
> > +
> > +	list_for_each_entry_safe(cxld, d, &region->commit_list, region_link) {
> > +		cxl_disable_decoder(cxld);
> > +		list_del(&cxld->region_link);
> > +		cxl_put_decoder(cxld);
> > +	}
> > +}
> > +
> >  static struct cxl_driver cxl_region_driver = {
> >  	.name = "cxl_region",
> >  	.probe = cxl_region_probe,
> > +	.remove = cxl_region_remove,
> >  	.id = CXL_DEVICE_REGION,
> >  };
> >  module_cxl_driver(cxl_region_driver);
> 

Re: [PATCH 13/13] cxl: Program decoders for regions

[Jonathan Cameron]

On Fri, 7 Jan 2022 08:33:41 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> On 22-01-07 16:18:12, Jonathan Cameron wrote:
> > On Thu,  6 Jan 2022 16:37:56 -0800
> > Ben Widawsky <ben.widawsky@intel.com> wrote:
> >   
> > > Do the HDM decoder programming for all endpoints and host bridges in a
> > > region. Switches are currently unimplemented.
> > > 
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > ---
> > >  drivers/cxl/core/hdm.c | 198 +++++++++++++++++++++++++++++++++++++++++
> > >  drivers/cxl/cxl.h      |   3 +
> > >  drivers/cxl/region.c   |  39 +++++++-
> > >  3 files changed, 237 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > index 44e48cea8cd4..c7293cbd8547 100644
> > > --- a/drivers/cxl/core/hdm.c
> > > +++ b/drivers/cxl/core/hdm.c
> > > @@ -242,3 +242,201 @@ int devm_cxl_enumerate_switch_decoders(struct cxl_port *port)
> > >  	return 0;
> > >  }
> > >  EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_switch_decoders, CXL);
> > > +
> > > +#define COMMIT_TIMEOUT_MS 10
> > > +static int wait_for_commit(struct cxl_decoder *cxld)
> > > +{
> > > +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > > +	const unsigned long start = jiffies;
> > > +	struct cxl_port_state *cxlps;
> > > +	void __iomem *hdm_decoder;
> > > +	unsigned long end = start;
> > > +	u32 ctrl;
> > > +
> > > +	cxlps = dev_get_drvdata(&port->dev);
> > > +	hdm_decoder = cxlps->regs.hdm_decoder;
> > > +
> > > +	do {
> > > +		end = jiffies;
> > > +		ctrl = readl(hdm_decoder +
> > > +			     CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > > +		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> > > +			break;
> > > +
> > > +		if (time_after(end, start + COMMIT_TIMEOUT_MS)) {
> > > +			dev_err(&cxld->dev, "HDM decoder commit timeout %x\n", ctrl);
> > > +			return -ETIMEDOUT;
> > > +		}
> > > +		if ((ctrl & CXL_HDM_DECODER0_CTRL_COMMIT_ERROR) != 0) {
> > > +			dev_err(&cxld->dev, "HDM decoder commit error %x\n", ctrl);
> > > +			return -ENXIO;
> > > +		}
> > > +	} while (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl));  
> > 
> > Hi Ben,
> > 
> > Thanks for posting this btw - it's very helpful indeed for hammering out bugs
> > in the qemu code and to identify what isn't there yet.
> > 
> > Took me a while to work out how this seemed to succeed on QEMU with no
> > implementation of the commit for the host bridges (just adding that now ;)
> > Reason is this condition is inverse of what I think you intend.
> > 
> > Given you have an exit condition above this could probably just be a while (1)
> > loop.
> > 
> > Jonathan  
> 
> Quick response without looking at code. I thought I did implement this in QEMU,
> but you're correct that the logic is inverted. If you have a QEMU branch for me
> to use when I fix this, please let me know.

The branch I got from your gitlab has it specifically for the type3 device, but
not more generally.

Qemu wise will see if I can hammer something into a shape to share but might take
a little while to hammer out remaining bugs.  I've made a few fiddly changes to
try and simplify the setup + avoid fixed PA address ranges anywhere.

Jonathan

> 
> >   
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * cxl_commit_decoder() - Program a configured cxl_decoder
> > > + * @cxld: The preconfigured cxl decoder.
> > > + *
> > > + * A cxl decoder that is to be committed should have been earmarked as enabled.
> > > + * This mechanism acts as a soft reservation on the decoder.
> > > + *
> > > + * Returns 0 if commit was successful, negative error code otherwise.
> > > + */
> > > +int cxl_commit_decoder(struct cxl_decoder *cxld)
> > > +{
> > > +	u32 ctrl, tl_lo, tl_hi, base_lo, base_hi, size_lo, size_hi;
> > > +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > > +	struct cxl_port_state *cxlps;
> > > +	void __iomem *hdm_decoder;
> > > +	int rc;
> > > +
> > > +	/*
> > > +	 * Decoder flags are entirely software controlled and therefore this
> > > +	 * case is purely a driver bug.
> > > +	 */
> > > +	if (dev_WARN_ONCE(&port->dev, (cxld->flags & CXL_DECODER_F_ENABLE) != 0,
> > > +			  "Invalid %s enable state\n", dev_name(&cxld->dev)))
> > > +		return -ENXIO;
> > > +
> > > +	cxlps = dev_get_drvdata(&port->dev);
> > > +	hdm_decoder = cxlps->regs.hdm_decoder;
> > > +	ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > > +
> > > +	/*
> > > +	 * A decoder that's currently active cannot be changed without the
> > > +	 * system being quiesced. While the driver should prevent against this,
> > > +	 * for a variety of reasons the hardware might not be in sync with the
> > > +	 * hardware and so, do not splat on error.
> > > +	 */
> > > +	size_hi = readl(hdm_decoder +
> > > +			CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> > > +	size_lo =
> > > +		readl(hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> > > +	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl) &&
> > > +	    (size_lo + size_hi)) {
> > > +		dev_err(&port->dev, "Tried to change an active decoder (%s)\n",
> > > +			dev_name(&cxld->dev));
> > > +		return -EBUSY;
> > > +	}
> > > +
> > > +	u32p_replace_bits(&ctrl, 8 - ilog2(cxld->interleave_granularity),
> > > +			  CXL_HDM_DECODER0_CTRL_IG_MASK);
> > > +	u32p_replace_bits(&ctrl, ilog2(cxld->interleave_ways),
> > > +			  CXL_HDM_DECODER0_CTRL_IW_MASK);
> > > +	u32p_replace_bits(&ctrl, 1, CXL_HDM_DECODER0_CTRL_COMMIT);
> > > +
> > > +	/* TODO: set based on type */
> > > +	u32p_replace_bits(&ctrl, 1, CXL_HDM_DECODER0_CTRL_TYPE);
> > > +
> > > +	base_lo = FIELD_PREP(GENMASK(31, 28),
> > > +			     (u32)(cxld->decoder_range.start & 0xffffffff));
> > > +	base_hi = FIELD_PREP(~0, (u32)(cxld->decoder_range.start >> 32));
> > > +
> > > +	size_lo = (u32)(range_len(&cxld->decoder_range)) & GENMASK(31, 28);
> > > +	size_hi = (u32)((range_len(&cxld->decoder_range) >> 32));
> > > +
> > > +	if (cxld->nr_targets > 0) {
> > > +		tl_lo |= FIELD_PREP(GENMASK(7, 0), cxld->target[0]->port_id);
> > > +		if (cxld->interleave_ways > 1)
> > > +			tl_lo |= FIELD_PREP(GENMASK(15, 8),
> > > +					    cxld->target[1]->port_id);
> > > +		if (cxld->interleave_ways > 2)
> > > +			tl_lo |= FIELD_PREP(GENMASK(23, 16),
> > > +					    cxld->target[2]->port_id);
> > > +		if (cxld->interleave_ways > 3)
> > > +			tl_lo |= FIELD_PREP(GENMASK(31, 24),
> > > +					    cxld->target[3]->port_id);
> > > +		if (cxld->interleave_ways > 4)
> > > +			tl_hi |= FIELD_PREP(GENMASK(7, 0),
> > > +					    cxld->target[4]->port_id);
> > > +		if (cxld->interleave_ways > 5)
> > > +			tl_hi |= FIELD_PREP(GENMASK(15, 8),
> > > +					    cxld->target[5]->port_id);
> > > +		if (cxld->interleave_ways > 6)
> > > +			tl_hi |= FIELD_PREP(GENMASK(23, 16),
> > > +					    cxld->target[6]->port_id);
> > > +		if (cxld->interleave_ways > 7)
> > > +			tl_hi |= FIELD_PREP(GENMASK(31, 24),
> > > +					    cxld->target[7]->port_id);
> > > +
> > > +		writel(tl_hi, hdm_decoder + CXL_HDM_DECODER0_TL_HIGH(cxld->id));
> > > +		writel(tl_lo, hdm_decoder + CXL_HDM_DECODER0_TL_LOW(cxld->id));
> > > +	}
> > > +
> > > +	writel(size_hi,
> > > +	       hdm_decoder + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> > > +	writel(size_lo,
> > > +	       hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> > > +	writel(base_hi,
> > > +	       hdm_decoder + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(cxld->id));
> > > +	writel(base_lo,
> > > +	       hdm_decoder + CXL_HDM_DECODER0_BASE_LOW_OFFSET(cxld->id));
> > > +	writel(ctrl, hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > > +
> > > +	rc = wait_for_commit(cxld);
> > > +	if (rc)
> > > +		return rc;
> > > +
> > > +	cxld->flags |= CXL_DECODER_F_ENABLE;
> > > +
> > > +#define DPORT_TL_STR "%d %d %d %d %d %d %d %d"
> > > +#define DPORT(i)                                                               \
> > > +	(cxld->nr_targets && cxld->interleave_ways > (i)) ?                    \
> > > +		      cxld->target[(i)]->port_id :                                   \
> > > +		      -1
> > > +#define DPORT_TL                                                               \
> > > +	DPORT(0), DPORT(1), DPORT(2), DPORT(3), DPORT(4), DPORT(5), DPORT(6),  \
> > > +		DPORT(7)
> > > +
> > > +	dev_dbg(&port->dev,
> > > +		"%s\n\tBase %pa\n\tSize %llu\n\tIG %u\n\tIW %u\n\tTargetList: " DPORT_TL_STR,
> > > +		dev_name(&cxld->dev), &cxld->decoder_range.start,
> > > +		range_len(&cxld->decoder_range), cxld->interleave_granularity,
> > > +		cxld->interleave_ways, DPORT_TL);
> > > +#undef DPORT_TL
> > > +#undef DPORT
> > > +#undef DPORT_TL_STR
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cxl_commit_decoder);
> > > +
> > > +/**
> > > + * cxl_disable_decoder() - Disables a decoder
> > > + * @cxld: The active cxl decoder.
> > > + *
> > > + * CXL decoders (as of 2.0 spec) have no way to deactivate them other than to
> > > + * set the size of the HDM to 0. This function will clear all registers, and if
> > > + * the decoder is active, commit the 0'd out registers.
> > > + */
> > > +void cxl_disable_decoder(struct cxl_decoder *cxld)
> > > +{
> > > +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > > +	struct cxl_port_state *cxlps;
> > > +	void __iomem *hdm_decoder;
> > > +	u32 ctrl;
> > > +
> > > +	cxlps = dev_get_drvdata(&port->dev);
> > > +	hdm_decoder = cxlps->regs.hdm_decoder;
> > > +	ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > > +
> > > +	if (dev_WARN_ONCE(&port->dev, (cxld->flags & CXL_DECODER_F_ENABLE) == 0,
> > > +			  "Invalid decoder enable state\n"))
> > > +		return;
> > > +
> > > +	/* There's no way to "uncommit" a committed decoder, only 0 size it */
> > > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_TL_HIGH(cxld->id));
> > > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_TL_LOW(cxld->id));
> > > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> > > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> > > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(cxld->id));
> > > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_BASE_LOW_OFFSET(cxld->id));
> > > +
> > > +	/* If the device isn't actually active, just zero out all the fields */
> > > +	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> > > +		writel(CXL_HDM_DECODER0_CTRL_COMMIT,
> > > +		       hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > > +}
> > > +EXPORT_SYMBOL_GPL(cxl_disable_decoder);
> > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > > index 79bca9a8246c..587b75f7fa53 100644
> > > --- a/drivers/cxl/cxl.h
> > > +++ b/drivers/cxl/cxl.h
> > > @@ -54,6 +54,7 @@
> > >  #define   CXL_HDM_DECODER0_CTRL_IW_MASK GENMASK(7, 4)
> > >  #define   CXL_HDM_DECODER0_CTRL_COMMIT BIT(9)
> > >  #define   CXL_HDM_DECODER0_CTRL_COMMITTED BIT(10)
> > > +#define   CXL_HDM_DECODER0_CTRL_COMMIT_ERROR BIT(11)
> > >  #define   CXL_HDM_DECODER0_CTRL_TYPE BIT(12)
> > >  #define CXL_HDM_DECODER0_TL_LOW(i) (0x20 * (i) + 0x24)
> > >  #define CXL_HDM_DECODER0_TL_HIGH(i) (0x20 * (i) + 0x28)
> > > @@ -364,6 +365,8 @@ int devm_cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
> > >  struct cxl_dport *cxl_find_dport_by_dev(struct cxl_port *port,
> > >  					const struct device *dev);
> > >  struct cxl_port *ep_find_cxl_port(struct cxl_memdev *cxlmd, unsigned int depth);
> > > +int cxl_commit_decoder(struct cxl_decoder *cxld);
> > > +void cxl_disable_decoder(struct cxl_decoder *cxld);
> > >  
> > >  struct cxl_decoder *to_cxl_decoder(struct device *dev);
> > >  bool is_cxl_decoder(struct device *dev);
> > > diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> > > index 3120b65b0bc5..fd82d37ba573 100644
> > > --- a/drivers/cxl/region.c
> > > +++ b/drivers/cxl/region.c
> > > @@ -580,10 +580,30 @@ static void cleanup_staged_decoders(struct cxl_region *region)
> > >  	}
> > >  }
> > >  
> > > -static int bind_region(const struct cxl_region *region)
> > > +static int bind_region(struct cxl_region *region)
> > >  {
> > > -	/* TODO: */
> > > -	return 0;
> > > +	struct cxl_decoder *cxld, *d;
> > > +	int rc;
> > > +
> > > +	list_for_each_entry_safe(cxld, d, &region->staged_list, region_link) {
> > > +		rc = cxl_commit_decoder(cxld);
> > > +		if (!rc)
> > > +			list_move_tail(&cxld->region_link, &region->commit_list);
> > > +		else
> > > +			break;
> > > +	}
> > > +
> > > +	list_for_each_entry_safe(cxld, d, &region->commit_list, region_link) {
> > > +		if (rc)
> > > +			cxl_disable_decoder(cxld);
> > > +		list_del(&cxld->region_link);
> > > +	}
> > > +
> > > +	if (rc)
> > > +		cleanup_staged_decoders((struct cxl_region *)region);
> > > +
> > > +	BUG_ON(!list_empty(&region->staged_list));
> > > +	return rc;
> > >  }
> > >  
> > >  static int cxl_region_probe(struct device *dev)
> > > @@ -650,9 +670,22 @@ static int cxl_region_probe(struct device *dev)
> > >  	return ret;
> > >  }
> > >  
> > > +static void cxl_region_remove(struct device *dev)
> > > +{
> > > +	struct cxl_region *region = to_cxl_region(dev);
> > > +	struct cxl_decoder *cxld, *d;
> > > +
> > > +	list_for_each_entry_safe(cxld, d, &region->commit_list, region_link) {
> > > +		cxl_disable_decoder(cxld);
> > > +		list_del(&cxld->region_link);
> > > +		cxl_put_decoder(cxld);
> > > +	}
> > > +}
> > > +
> > >  static struct cxl_driver cxl_region_driver = {
> > >  	.name = "cxl_region",
> > >  	.probe = cxl_region_probe,
> > > +	.remove = cxl_region_remove,
> > >  	.id = CXL_DEVICE_REGION,
> > >  };
> > >  module_cxl_driver(cxl_region_driver);  
> >   

Re: [PATCH 12/13] cxl/region: Record host bridge target list

[Jonathan Cameron]

On Thu,  6 Jan 2022 16:37:55 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> Part of host bridge verification in the CXL Type 3 Memory Device
> Software Guide calculates the host bridge interleave target list (6th
> step in the flow chart). With host bridge verification already done, it
> is trivial to store away the configuration information.
> 
> TODO: Needs support for switches (7th step in the flow chart).
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  drivers/cxl/region.c | 41 +++++++++++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> index eafd95419895..3120b65b0bc5 100644
> --- a/drivers/cxl/region.c
> +++ b/drivers/cxl/region.c
> @@ -385,6 +385,7 @@ static bool region_hb_rp_config_valid(struct cxl_region *region,
>  	}
>  
>  	for (i = 0; i < hb_count; i++) {
> +		struct cxl_decoder *cxld;
>  		int idx, position_mask;
>  		struct cxl_dport *rp;
>  		struct cxl_port *hb;
> @@ -422,10 +423,8 @@ static bool region_hb_rp_config_valid(struct cxl_region *region,
>  				if (get_rp(ep) != rp)
>  					continue;
>  
> -				if (port_grouping == -1) {
> +				if (port_grouping == -1)
>  					port_grouping = idx & position_mask;
> -					continue;
> -				}
>  
>  				/*
>  				 * Do all devices in the region connected to this CXL
> @@ -436,10 +435,32 @@ static bool region_hb_rp_config_valid(struct cxl_region *region,
>  							  "One or more devices are not connected to the correct Host Bridge Root Port\n");
>  					return false;
>  				}
> +
> +				if (!state_update)
> +					continue;
> +
> +				if (dev_WARN_ONCE(&cxld->dev,
> +						  port_grouping >= cxld->nr_targets,
> +						  "Invalid port grouping %d/%d\n",
> +						  port_grouping, cxld->nr_targets))
> +					return false;
> +
> +				cxld->interleave_ways++;
> +				cxld->target[port_grouping] = get_rp(ep);

Hi Ben,

Just one more based on debug rather than review.

The reason is across 2 patches so not necessary obvious from what is visible here,
but port_grouping here for a 2hb, 2rp on each and 1 ep on each of those
case goes 0,1,2,3 resulting in us setting one of the host bridges to have
a decoder with targets 2 and 3 rather than 0 and 1 set.

I haven't figured out a particularly good solution yet...  If everything is nice and symmetric
and power of 2 then you can simply change the mask on the index to reflect num_root_ports / num_host_bridges

With that change in place my decoders all look good on this particular configuration :)
Note this is eyeball based testing only and on just one configuration so far.

I'll have to try your tool as it is really annoying that the mem devices change order on every
boot as my script is dumb currently so have to edit it every run.

Jonathan

>  			}
>  		}
> -		if (state_update)
> +
> +		if (state_update) {
> +			/* IG doesn't change across host bridges */
> +			cxld->interleave_granularity = region_ig(region);
> +
> +			cxld->decoder_range = (struct range) {
> +				.start = region->res->start,
> +				.end = region->res->end
> +			};
> +
>  			list_add_tail(&cxld->region_link, &region->staged_list);
> +		}
>  	}
>  
>  	return true;
> @@ -464,7 +485,7 @@ static bool rootd_contains(const struct cxl_region *region,
>  	return true;
>  }
>  
> -static bool rootd_valid(const struct cxl_region *region,
> +static bool rootd_valid(struct cxl_region *region,
>  			const struct cxl_decoder *rootd,
>  			bool state_update)
>  {
> @@ -489,20 +510,20 @@ static bool rootd_valid(const struct cxl_region *region,
>  }
>  
>  struct rootd_context {
> -	const struct cxl_region *region;
> -	struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
> +	struct cxl_region *region;
> +	const struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
>  	int count;
>  };
>  
>  static int rootd_match(struct device *dev, void *data)
>  {
>  	struct rootd_context *ctx = (struct rootd_context *)data;
> -	const struct cxl_region *region = ctx->region;
> +	struct cxl_region *region = ctx->region;
>  
>  	if (!is_root_decoder(dev))
>  		return 0;
>  
> -	return !!rootd_valid(region, to_cxl_decoder(dev), false);
> +	return rootd_valid(region, to_cxl_decoder(dev), false);
>  }
>  
>  /*
> @@ -516,7 +537,7 @@ static struct cxl_decoder *find_rootd(const struct cxl_region *region,
>  	struct rootd_context ctx;
>  	struct device *ret;
>  
> -	ctx.region = region;
> +	ctx.region = (struct cxl_region *)region;
>  
>  	ret = device_find_child((struct device *)&root->dev, &ctx, rootd_match);
>  	if (ret)

Re: [PATCH 12/13] cxl/region: Record host bridge target list

[Dan Williams]

On Fri, Jan 7, 2022 at 10:15 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu,  6 Jan 2022 16:37:55 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> > Part of host bridge verification in the CXL Type 3 Memory Device
> > Software Guide calculates the host bridge interleave target list (6th
> > step in the flow chart). With host bridge verification already done, it
> > is trivial to store away the configuration information.
> >
> > TODO: Needs support for switches (7th step in the flow chart).
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> >  drivers/cxl/region.c | 41 +++++++++++++++++++++++++++++++----------
> >  1 file changed, 31 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> > index eafd95419895..3120b65b0bc5 100644
> > --- a/drivers/cxl/region.c
> > +++ b/drivers/cxl/region.c
> > @@ -385,6 +385,7 @@ static bool region_hb_rp_config_valid(struct cxl_region *region,
> >       }
> >
> >       for (i = 0; i < hb_count; i++) {
> > +             struct cxl_decoder *cxld;
> >               int idx, position_mask;
> >               struct cxl_dport *rp;
> >               struct cxl_port *hb;
> > @@ -422,10 +423,8 @@ static bool region_hb_rp_config_valid(struct cxl_region *region,
> >                               if (get_rp(ep) != rp)
> >                                       continue;
> >
> > -                             if (port_grouping == -1) {
> > +                             if (port_grouping == -1)
> >                                       port_grouping = idx & position_mask;
> > -                                     continue;
> > -                             }
> >
> >                               /*
> >                                * Do all devices in the region connected to this CXL
> > @@ -436,10 +435,32 @@ static bool region_hb_rp_config_valid(struct cxl_region *region,
> >                                                         "One or more devices are not connected to the correct Host Bridge Root Port\n");
> >                                       return false;
> >                               }
> > +
> > +                             if (!state_update)
> > +                                     continue;
> > +
> > +                             if (dev_WARN_ONCE(&cxld->dev,
> > +                                               port_grouping >= cxld->nr_targets,
> > +                                               "Invalid port grouping %d/%d\n",
> > +                                               port_grouping, cxld->nr_targets))
> > +                                     return false;
> > +
> > +                             cxld->interleave_ways++;
> > +                             cxld->target[port_grouping] = get_rp(ep);
>
> Hi Ben,
>
> Just one more based on debug rather than review.
>
> The reason is across 2 patches so not necessary obvious from what is visible here,
> but port_grouping here for a 2hb, 2rp on each and 1 ep on each of those
> case goes 0,1,2,3 resulting in us setting one of the host bridges to have
> a decoder with targets 2 and 3 rather than 0 and 1 set.
>
> I haven't figured out a particularly good solution yet...  If everything is nice and symmetric
> and power of 2 then you can simply change the mask on the index to reflect num_root_ports / num_host_bridges
>
> With that change in place my decoders all look good on this particular configuration :)
> Note this is eyeball based testing only and on just one configuration so far.
>
> I'll have to try your tool as it is really annoying that the mem devices change order on every
> boot as my script is dumb currently so have to edit it every run.

I have support pending for cxl-cli that lets memdevs be filtered by
serial number so you don't need to worry about dynamic kernel device
names.

Should be posted later today.

Re: [PATCH 13/13] cxl: Program decoders for regions

[Ben Widawsky]

On 22-01-07 17:22:22, Jonathan Cameron wrote:
> On Fri, 7 Jan 2022 08:33:41 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
> 
> > On 22-01-07 16:18:12, Jonathan Cameron wrote:
> > > On Thu,  6 Jan 2022 16:37:56 -0800
> > > Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >   
> > > > Do the HDM decoder programming for all endpoints and host bridges in a
> > > > region. Switches are currently unimplemented.
> > > > 
> > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > > ---
> > > >  drivers/cxl/core/hdm.c | 198 +++++++++++++++++++++++++++++++++++++++++
> > > >  drivers/cxl/cxl.h      |   3 +
> > > >  drivers/cxl/region.c   |  39 +++++++-
> > > >  3 files changed, 237 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > > index 44e48cea8cd4..c7293cbd8547 100644
> > > > --- a/drivers/cxl/core/hdm.c
> > > > +++ b/drivers/cxl/core/hdm.c
> > > > @@ -242,3 +242,201 @@ int devm_cxl_enumerate_switch_decoders(struct cxl_port *port)
> > > >  	return 0;
> > > >  }
> > > >  EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_switch_decoders, CXL);
> > > > +
> > > > +#define COMMIT_TIMEOUT_MS 10
> > > > +static int wait_for_commit(struct cxl_decoder *cxld)
> > > > +{
> > > > +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > > > +	const unsigned long start = jiffies;
> > > > +	struct cxl_port_state *cxlps;
> > > > +	void __iomem *hdm_decoder;
> > > > +	unsigned long end = start;
> > > > +	u32 ctrl;
> > > > +
> > > > +	cxlps = dev_get_drvdata(&port->dev);
> > > > +	hdm_decoder = cxlps->regs.hdm_decoder;
> > > > +
> > > > +	do {
> > > > +		end = jiffies;
> > > > +		ctrl = readl(hdm_decoder +
> > > > +			     CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > > > +		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> > > > +			break;
> > > > +
> > > > +		if (time_after(end, start + COMMIT_TIMEOUT_MS)) {
> > > > +			dev_err(&cxld->dev, "HDM decoder commit timeout %x\n", ctrl);
> > > > +			return -ETIMEDOUT;
> > > > +		}
> > > > +		if ((ctrl & CXL_HDM_DECODER0_CTRL_COMMIT_ERROR) != 0) {
> > > > +			dev_err(&cxld->dev, "HDM decoder commit error %x\n", ctrl);
> > > > +			return -ENXIO;
> > > > +		}
> > > > +	} while (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl));  
> > > 
> > > Hi Ben,
> > > 
> > > Thanks for posting this btw - it's very helpful indeed for hammering out bugs
> > > in the qemu code and to identify what isn't there yet.
> > > 
> > > Took me a while to work out how this seemed to succeed on QEMU with no
> > > implementation of the commit for the host bridges (just adding that now ;)
> > > Reason is this condition is inverse of what I think you intend.
> > > 
> > > Given you have an exit condition above this could probably just be a while (1)
> > > loop.
> > > 
> > > Jonathan  
> > 
> > Quick response without looking at code. I thought I did implement this in QEMU,
> > but you're correct that the logic is inverted. If you have a QEMU branch for me
> > to use when I fix this, please let me know.
> 
> The branch I got from your gitlab has it specifically for the type3 device, but
> not more generally.
> 
> Qemu wise will see if I can hammer something into a shape to share but might take
> a little while to hammer out remaining bugs.  I've made a few fiddly changes to
> try and simplify the setup + avoid fixed PA address ranges anywhere.
> 
> Jonathan
> 

Just pushed a bugfix to my v4 branch for this (I've also updated the kernel
patch).

> > 
> > >   
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * cxl_commit_decoder() - Program a configured cxl_decoder
> > > > + * @cxld: The preconfigured cxl decoder.
> > > > + *
> > > > + * A cxl decoder that is to be committed should have been earmarked as enabled.
> > > > + * This mechanism acts as a soft reservation on the decoder.
> > > > + *
> > > > + * Returns 0 if commit was successful, negative error code otherwise.
> > > > + */
> > > > +int cxl_commit_decoder(struct cxl_decoder *cxld)
> > > > +{
> > > > +	u32 ctrl, tl_lo, tl_hi, base_lo, base_hi, size_lo, size_hi;
> > > > +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > > > +	struct cxl_port_state *cxlps;
> > > > +	void __iomem *hdm_decoder;
> > > > +	int rc;
> > > > +
> > > > +	/*
> > > > +	 * Decoder flags are entirely software controlled and therefore this
> > > > +	 * case is purely a driver bug.
> > > > +	 */
> > > > +	if (dev_WARN_ONCE(&port->dev, (cxld->flags & CXL_DECODER_F_ENABLE) != 0,
> > > > +			  "Invalid %s enable state\n", dev_name(&cxld->dev)))
> > > > +		return -ENXIO;
> > > > +
> > > > +	cxlps = dev_get_drvdata(&port->dev);
> > > > +	hdm_decoder = cxlps->regs.hdm_decoder;
> > > > +	ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > > > +
> > > > +	/*
> > > > +	 * A decoder that's currently active cannot be changed without the
> > > > +	 * system being quiesced. While the driver should prevent against this,
> > > > +	 * for a variety of reasons the hardware might not be in sync with the
> > > > +	 * hardware and so, do not splat on error.
> > > > +	 */
> > > > +	size_hi = readl(hdm_decoder +
> > > > +			CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> > > > +	size_lo =
> > > > +		readl(hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> > > > +	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl) &&
> > > > +	    (size_lo + size_hi)) {
> > > > +		dev_err(&port->dev, "Tried to change an active decoder (%s)\n",
> > > > +			dev_name(&cxld->dev));
> > > > +		return -EBUSY;
> > > > +	}
> > > > +
> > > > +	u32p_replace_bits(&ctrl, 8 - ilog2(cxld->interleave_granularity),
> > > > +			  CXL_HDM_DECODER0_CTRL_IG_MASK);
> > > > +	u32p_replace_bits(&ctrl, ilog2(cxld->interleave_ways),
> > > > +			  CXL_HDM_DECODER0_CTRL_IW_MASK);
> > > > +	u32p_replace_bits(&ctrl, 1, CXL_HDM_DECODER0_CTRL_COMMIT);
> > > > +
> > > > +	/* TODO: set based on type */
> > > > +	u32p_replace_bits(&ctrl, 1, CXL_HDM_DECODER0_CTRL_TYPE);
> > > > +
> > > > +	base_lo = FIELD_PREP(GENMASK(31, 28),
> > > > +			     (u32)(cxld->decoder_range.start & 0xffffffff));
> > > > +	base_hi = FIELD_PREP(~0, (u32)(cxld->decoder_range.start >> 32));
> > > > +
> > > > +	size_lo = (u32)(range_len(&cxld->decoder_range)) & GENMASK(31, 28);
> > > > +	size_hi = (u32)((range_len(&cxld->decoder_range) >> 32));
> > > > +
> > > > +	if (cxld->nr_targets > 0) {
> > > > +		tl_lo |= FIELD_PREP(GENMASK(7, 0), cxld->target[0]->port_id);
> > > > +		if (cxld->interleave_ways > 1)
> > > > +			tl_lo |= FIELD_PREP(GENMASK(15, 8),
> > > > +					    cxld->target[1]->port_id);
> > > > +		if (cxld->interleave_ways > 2)
> > > > +			tl_lo |= FIELD_PREP(GENMASK(23, 16),
> > > > +					    cxld->target[2]->port_id);
> > > > +		if (cxld->interleave_ways > 3)
> > > > +			tl_lo |= FIELD_PREP(GENMASK(31, 24),
> > > > +					    cxld->target[3]->port_id);
> > > > +		if (cxld->interleave_ways > 4)
> > > > +			tl_hi |= FIELD_PREP(GENMASK(7, 0),
> > > > +					    cxld->target[4]->port_id);
> > > > +		if (cxld->interleave_ways > 5)
> > > > +			tl_hi |= FIELD_PREP(GENMASK(15, 8),
> > > > +					    cxld->target[5]->port_id);
> > > > +		if (cxld->interleave_ways > 6)
> > > > +			tl_hi |= FIELD_PREP(GENMASK(23, 16),
> > > > +					    cxld->target[6]->port_id);
> > > > +		if (cxld->interleave_ways > 7)
> > > > +			tl_hi |= FIELD_PREP(GENMASK(31, 24),
> > > > +					    cxld->target[7]->port_id);
> > > > +
> > > > +		writel(tl_hi, hdm_decoder + CXL_HDM_DECODER0_TL_HIGH(cxld->id));
> > > > +		writel(tl_lo, hdm_decoder + CXL_HDM_DECODER0_TL_LOW(cxld->id));
> > > > +	}
> > > > +
> > > > +	writel(size_hi,
> > > > +	       hdm_decoder + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> > > > +	writel(size_lo,
> > > > +	       hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> > > > +	writel(base_hi,
> > > > +	       hdm_decoder + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(cxld->id));
> > > > +	writel(base_lo,
> > > > +	       hdm_decoder + CXL_HDM_DECODER0_BASE_LOW_OFFSET(cxld->id));
> > > > +	writel(ctrl, hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > > > +
> > > > +	rc = wait_for_commit(cxld);
> > > > +	if (rc)
> > > > +		return rc;
> > > > +
> > > > +	cxld->flags |= CXL_DECODER_F_ENABLE;
> > > > +
> > > > +#define DPORT_TL_STR "%d %d %d %d %d %d %d %d"
> > > > +#define DPORT(i)                                                               \
> > > > +	(cxld->nr_targets && cxld->interleave_ways > (i)) ?                    \
> > > > +		      cxld->target[(i)]->port_id :                                   \
> > > > +		      -1
> > > > +#define DPORT_TL                                                               \
> > > > +	DPORT(0), DPORT(1), DPORT(2), DPORT(3), DPORT(4), DPORT(5), DPORT(6),  \
> > > > +		DPORT(7)
> > > > +
> > > > +	dev_dbg(&port->dev,
> > > > +		"%s\n\tBase %pa\n\tSize %llu\n\tIG %u\n\tIW %u\n\tTargetList: " DPORT_TL_STR,
> > > > +		dev_name(&cxld->dev), &cxld->decoder_range.start,
> > > > +		range_len(&cxld->decoder_range), cxld->interleave_granularity,
> > > > +		cxld->interleave_ways, DPORT_TL);
> > > > +#undef DPORT_TL
> > > > +#undef DPORT
> > > > +#undef DPORT_TL_STR
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(cxl_commit_decoder);
> > > > +
> > > > +/**
> > > > + * cxl_disable_decoder() - Disables a decoder
> > > > + * @cxld: The active cxl decoder.
> > > > + *
> > > > + * CXL decoders (as of 2.0 spec) have no way to deactivate them other than to
> > > > + * set the size of the HDM to 0. This function will clear all registers, and if
> > > > + * the decoder is active, commit the 0'd out registers.
> > > > + */
> > > > +void cxl_disable_decoder(struct cxl_decoder *cxld)
> > > > +{
> > > > +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > > > +	struct cxl_port_state *cxlps;
> > > > +	void __iomem *hdm_decoder;
> > > > +	u32 ctrl;
> > > > +
> > > > +	cxlps = dev_get_drvdata(&port->dev);
> > > > +	hdm_decoder = cxlps->regs.hdm_decoder;
> > > > +	ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > > > +
> > > > +	if (dev_WARN_ONCE(&port->dev, (cxld->flags & CXL_DECODER_F_ENABLE) == 0,
> > > > +			  "Invalid decoder enable state\n"))
> > > > +		return;
> > > > +
> > > > +	/* There's no way to "uncommit" a committed decoder, only 0 size it */
> > > > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_TL_HIGH(cxld->id));
> > > > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_TL_LOW(cxld->id));
> > > > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> > > > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> > > > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(cxld->id));
> > > > +	writel(0, hdm_decoder + CXL_HDM_DECODER0_BASE_LOW_OFFSET(cxld->id));
> > > > +
> > > > +	/* If the device isn't actually active, just zero out all the fields */
> > > > +	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> > > > +		writel(CXL_HDM_DECODER0_CTRL_COMMIT,
> > > > +		       hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(cxl_disable_decoder);
> > > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > > > index 79bca9a8246c..587b75f7fa53 100644
> > > > --- a/drivers/cxl/cxl.h
> > > > +++ b/drivers/cxl/cxl.h
> > > > @@ -54,6 +54,7 @@
> > > >  #define   CXL_HDM_DECODER0_CTRL_IW_MASK GENMASK(7, 4)
> > > >  #define   CXL_HDM_DECODER0_CTRL_COMMIT BIT(9)
> > > >  #define   CXL_HDM_DECODER0_CTRL_COMMITTED BIT(10)
> > > > +#define   CXL_HDM_DECODER0_CTRL_COMMIT_ERROR BIT(11)
> > > >  #define   CXL_HDM_DECODER0_CTRL_TYPE BIT(12)
> > > >  #define CXL_HDM_DECODER0_TL_LOW(i) (0x20 * (i) + 0x24)
> > > >  #define CXL_HDM_DECODER0_TL_HIGH(i) (0x20 * (i) + 0x28)
> > > > @@ -364,6 +365,8 @@ int devm_cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
> > > >  struct cxl_dport *cxl_find_dport_by_dev(struct cxl_port *port,
> > > >  					const struct device *dev);
> > > >  struct cxl_port *ep_find_cxl_port(struct cxl_memdev *cxlmd, unsigned int depth);
> > > > +int cxl_commit_decoder(struct cxl_decoder *cxld);
> > > > +void cxl_disable_decoder(struct cxl_decoder *cxld);
> > > >  
> > > >  struct cxl_decoder *to_cxl_decoder(struct device *dev);
> > > >  bool is_cxl_decoder(struct device *dev);
> > > > diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> > > > index 3120b65b0bc5..fd82d37ba573 100644
> > > > --- a/drivers/cxl/region.c
> > > > +++ b/drivers/cxl/region.c
> > > > @@ -580,10 +580,30 @@ static void cleanup_staged_decoders(struct cxl_region *region)
> > > >  	}
> > > >  }
> > > >  
> > > > -static int bind_region(const struct cxl_region *region)
> > > > +static int bind_region(struct cxl_region *region)
> > > >  {
> > > > -	/* TODO: */
> > > > -	return 0;
> > > > +	struct cxl_decoder *cxld, *d;
> > > > +	int rc;
> > > > +
> > > > +	list_for_each_entry_safe(cxld, d, &region->staged_list, region_link) {
> > > > +		rc = cxl_commit_decoder(cxld);
> > > > +		if (!rc)
> > > > +			list_move_tail(&cxld->region_link, &region->commit_list);
> > > > +		else
> > > > +			break;
> > > > +	}
> > > > +
> > > > +	list_for_each_entry_safe(cxld, d, &region->commit_list, region_link) {
> > > > +		if (rc)
> > > > +			cxl_disable_decoder(cxld);
> > > > +		list_del(&cxld->region_link);
> > > > +	}
> > > > +
> > > > +	if (rc)
> > > > +		cleanup_staged_decoders((struct cxl_region *)region);
> > > > +
> > > > +	BUG_ON(!list_empty(&region->staged_list));
> > > > +	return rc;
> > > >  }
> > > >  
> > > >  static int cxl_region_probe(struct device *dev)
> > > > @@ -650,9 +670,22 @@ static int cxl_region_probe(struct device *dev)
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +static void cxl_region_remove(struct device *dev)
> > > > +{
> > > > +	struct cxl_region *region = to_cxl_region(dev);
> > > > +	struct cxl_decoder *cxld, *d;
> > > > +
> > > > +	list_for_each_entry_safe(cxld, d, &region->commit_list, region_link) {
> > > > +		cxl_disable_decoder(cxld);
> > > > +		list_del(&cxld->region_link);
> > > > +		cxl_put_decoder(cxld);
> > > > +	}
> > > > +}
> > > > +
> > > >  static struct cxl_driver cxl_region_driver = {
> > > >  	.name = "cxl_region",
> > > >  	.probe = cxl_region_probe,
> > > > +	.remove = cxl_region_remove,
> > > >  	.id = CXL_DEVICE_REGION,
> > > >  };
> > > >  module_cxl_driver(cxl_region_driver);  
> > >   
> 

Re: [PATCH 09/13] cxl/region: Implement XHB verification

[Ben Widawsky]

On 22-01-07 10:38:27, Jonathan Cameron wrote:
> On Fri, 7 Jan 2022 10:30:52 +0000
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> > On Thu,  6 Jan 2022 16:37:52 -0800
> > Ben Widawsky <ben.widawsky@intel.com> wrote:
> > 
> > > Cross host bridge verification primarily determines if the requested
> > > interleave ordering can be achieved by the root decoder, which isn't as
> > > programmable as other decoders.
> > > 
> > > The algorithm implemented here is based on the CXL Type 3 Memory Device
> > > Software Guide, chapter 2.13.14
> > > 
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>  
> > 
> > Trivial thing inline.
> > 
> > > diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> > > index c8e3c48dfbb9..ca559a4b5347 100644
> > > --- a/drivers/cxl/region.c
> > > +++ b/drivers/cxl/region.c
> > > @@ -28,6 +28,17 @@
> > >   */
> > >  
> > >  #define region_ways(region) ((region)->config.eniw)
> > > +#define region_ig(region) (ilog2((region)->config.ig))
> > > +
> > > +#define for_each_cxl_endpoint(ep, region, idx)                                 \
> > > +	for (idx = 0, ep = (region)->config.targets[idx];                      \
> > > +	     idx < region_ways(region);                                        \
> > > +	     idx++, ep = (region)->config.targets[idx])
> > > +
> > > +#define for_each_cxl_decoder_target(target, decoder, idx)                      \
> > > +	for (idx = 0, target = (decoder)->target[idx];                         \  
> > 
> > As target is used too often in here, you'll replace it in ->target[idx] as well.
> > It happens to work today because the parameter always happens to be target
> > 
> > > +	     idx < (decoder)->nr_targets;                                      \
> > > +	     idx++, target++)
> I should have read the next few lines :)
> 
> target++ doesn't get (decoder)->target[idx] which is what we want - it indexes
> off the end of a particular instance rather than through the array.
> 
> I'm guessing this was from my unclear comment yesterday. I should have spent
> a little more time being explicit there.
> 
> Jonathan
> 
> > >    
> 

Gotcha. I combined the idx increment as well, what do you think about this (just
typed, not tested):

#define for_each_cxl_decoder_target(dport, decoder, idx)                      \
        for (idx = 0, dport = (decoder)->target[idx];                         \
             idx < (decoder)->nr_targets;                                     \
             dport = (decoder)->target[++idx])

Re: [PATCH 09/13] cxl/region: Implement XHB verification

[Ben Widawsky]

On 22-01-07 11:55:24, Jonathan Cameron wrote:
> On Fri, 7 Jan 2022 10:07:14 +0000
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Thu,  6 Jan 2022 16:37:52 -0800
> > Ben Widawsky <ben.widawsky@intel.com> wrote:
> > 
> > > Cross host bridge verification primarily determines if the requested
> > > interleave ordering can be achieved by the root decoder, which isn't as
> > > programmable as other decoders.
> > > 
> > > The algorithm implemented here is based on the CXL Type 3 Memory Device
> > > Software Guide, chapter 2.13.14
> > > 
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>  
> > 
> > Hi Ben,
> > 
> > A few things I'm carrying 'fixes' for in here.
> > 
> > Jonathan
> > 
> > > ---
> > >  .clang-format        |  2 +
> > >  drivers/cxl/region.c | 89 +++++++++++++++++++++++++++++++++++++++++++-
> > >  drivers/cxl/trace.h  |  3 ++
> > >  3 files changed, 93 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/.clang-format b/.clang-format
> > > index 15d4eaabc6b5..55f628f21722 100644
> > > --- a/.clang-format
> > > +++ b/.clang-format
> > > @@ -169,6 +169,8 @@ ForEachMacros:
> > >    - 'for_each_cpu_and'
> > >    - 'for_each_cpu_not'
> > >    - 'for_each_cpu_wrap'
> > > +  - 'for_each_cxl_decoder_target'
> > > +  - 'for_each_cxl_endpoint'
> > >    - 'for_each_dapm_widgets'
> > >    - 'for_each_dev_addr'
> > >    - 'for_each_dev_scope'
> > > diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> > > index c8e3c48dfbb9..ca559a4b5347 100644
> > > --- a/drivers/cxl/region.c
> > > +++ b/drivers/cxl/region.c
> > > @@ -28,6 +28,17 @@
> > >   */
> > >  
> > >  #define region_ways(region) ((region)->config.eniw)
> > > +#define region_ig(region) (ilog2((region)->config.ig))
> > > +
> > > +#define for_each_cxl_endpoint(ep, region, idx)                                 \
> > > +	for (idx = 0, ep = (region)->config.targets[idx];                      \
> > > +	     idx < region_ways(region);                                        \
> > > +	     idx++, ep = (region)->config.targets[idx])
> > > +
> > > +#define for_each_cxl_decoder_target(target, decoder, idx)                      \
> > > +	for (idx = 0, target = (decoder)->target[idx];                         \
> > > +	     idx < (decoder)->nr_targets;                                      \
> > > +	     idx++, target++)
> > >  
> > >  static struct cxl_decoder *rootd_from_region(struct cxl_region *r)
> > >  {
> > > @@ -175,6 +186,30 @@ static bool qtg_match(const struct cxl_decoder *rootd,
> > >  	return true;
> > >  }
> > >  
> > > +static int get_unique_hostbridges(const struct cxl_region *region,
> > > +				  struct cxl_port **hbs)
> > > +{
> > > +	struct cxl_memdev *ep;
> > > +	int i, hb_count = 0;
> > > +
> > > +	for_each_cxl_endpoint(ep, region, i) {
> > > +		struct cxl_port *hb = get_hostbridge(ep);
> > > +		bool found = false;
> > > +		int j;
> > > +
> > > +		BUG_ON(!hb);
> > > +
> > > +		for (j = 0; j < hb_count; j++) {
> > > +			if (hbs[j] == hb)
> > > +				found = true;
> > > +		}
> > > +		if (!found)
> > > +			hbs[hb_count++] = hb;
> > > +	}
> > > +
> > > +	return hb_count;
> > > +}
> > > +
> > >  /**
> > >   * region_xhb_config_valid() - determine cross host bridge validity
> > >   * @rootd: The root decoder to check against
> > > @@ -188,7 +223,59 @@ static bool qtg_match(const struct cxl_decoder *rootd,
> > >  static bool region_xhb_config_valid(const struct cxl_region *region,
> > >  				    const struct cxl_decoder *rootd)
> > >  {
> > > -	/* TODO: */
> > > +	struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
> > > +	int rootd_ig, i;
> > > +	struct cxl_dport *target;
> > > +
> > > +	/* Are all devices in this region on the same CXL host bridge */
> > > +	if (get_unique_hostbridges(region, hbs) == 1)
> > > +		return true;
> > > +
> > > +	rootd_ig = rootd->interleave_granularity;
> > > +
> > > +	/* CFMWS.HBIG >= Device.Label.IG */
> > > +	if (rootd_ig < region_ig(region)) {
> > > +		trace_xhb_valid(region,
> > > +				"granularity does not support the region interleave granularity\n");
> > > +		return false;
> > > +	}
> > > +
> > > +	/* ((2^(CFMWS.HBIG - Device.RLabel.IG) * (2^CFMWS.ENIW)) > Device.RLabel.NLabel) */
> > > +	if (1 << (rootd_ig - region_ig(region)) * (1 << rootd->interleave_ways) >  
> > 
> > This maths isn't what the comment says it is.
> > ((1 << (rootd_ig - region_ig(region))) * rootd->interleaveways)
> > so brackets needed to avoid 2^( all the rest) and rootd->interleave_ways seems to the
> > actual number of ways not the log2 of it.
> > 
> > That feeds through below.

You're correct on both counts. Nice catch.

> > 
> > 
> > > +	    region_ways(region)) {
> > > +		trace_xhb_valid(region,
> > > +				"granularity to device granularity ratio requires a larger number of devices than currently configured");
> > > +		return false;
> > > +	}
> > > +
> > > +	/* Check that endpoints are hooked up in the correct order */
> > > +	for_each_cxl_decoder_target(target, rootd, i) {
> > > +		struct cxl_memdev *endpoint = region->config.targets[i];
> > > +
> > > +		if (get_hostbridge(endpoint) != target->port) {  
> > 
> > I think this should be
> > get_hostbridge(endpoint)->uport != target->dport
> > 
> > As it stands you are comparing the host bridge with the root object.
> 
> On closer inspection this code doesn't do what it is meant to do at all
> if there are multiple EP below a given root bridge.
> 
> You'd expect multiple endpoints to match to each target->port.
> Something along the lines of this should work:
> 
>         {
>                 struct cxl_memdev **epgroupstart = region->config.targets;
>                 struct cxl_memdev **endpoint;
> 
>                 for_each_cxl_decoder_target(target, rootd, i) {
>                         /* Find start of next endpoint group */
>                         endpoint = epgroupstart;
>                         if (*endpoint == NULL) {
>                                 printk("No endpoints under decoder target\n");
>                                 return false;
>                         }
>                         while (*epgroupstart &&
>                                 get_hostbridge(*endpoint) == get_hostbridge(*epgroupstart))
>                                 epgroupstart++;
>                 }
>                 if (*epgroupstart) {
>                         printk("still some entries left. boom\n");
>                         return false;
>                 }
>         }
> 
> Only lightly tested with correct inputs...
> 
> Next up is figuring out why the EP HDM decoder won't commit. :)
> 
> Jonathan
> 

You're correct that what I have isn't correct. However, I think this was just
bogus leftover from an aborted attempt to try to implement this. See below...

> 
> > 
> > > +			trace_xhb_valid(region, "device ordering bad\n");
> > > +			return false;
> > > +		}
> > > +	}
> > > +
> > > +	/*
> > > +	 * CFMWS.InterleaveTargetList[n] must contain all devices, x where:
> > > +	 *	(Device[x],RegionLabel.Position >> (CFMWS.HBIG -
> > > +	 *	Device[x].RegionLabel.InterleaveGranularity)) &
> > > +	 *	((2^CFMWS.ENIW) - 1) = n
> > > +	 *
> > > +	 * Linux notes: All devices are known to have the same interleave
> > > +	 * granularity at this point.
> > > +	 */
> > > +	for_each_cxl_decoder_target(target, rootd, i) {
> > > +		if (((i >> (rootd_ig - region_ig(region)))) &
> > > +		    (((1 << rootd->interleave_ways) - 1) != target->port_id)) {
> > > +			trace_xhb_valid(region,
> > > +					"One or more devices are not connected to the correct hostbridge.");
> > > +			return false;
> > > +		}
> > > +	}
> > > +

I think this does the correct XHB calculation. So I think I can just remove the
top hunk and we're good. Do you agree?

> > >  	return true;
> > >  }
> > >  
> > > diff --git a/drivers/cxl/trace.h b/drivers/cxl/trace.h
> > > index a53f00ba5d0e..4de47d1111ac 100644
> > > --- a/drivers/cxl/trace.h
> > > +++ b/drivers/cxl/trace.h
> > > @@ -38,6 +38,9 @@ DEFINE_EVENT(cxl_region_template, sanitize_failed,
> > >  DEFINE_EVENT(cxl_region_template, allocation_failed,
> > >  	     TP_PROTO(const struct cxl_region *region, char *status),
> > >  	     TP_ARGS(region, status));
> > > +DEFINE_EVENT(cxl_region_template, xhb_valid,
> > > +	     TP_PROTO(const struct cxl_region *region, char *status),
> > > +	     TP_ARGS(region, status));
> > >  
> > >  #endif /* if !defined (__CXL_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ) */
> > >    
> > 
> 

Re: [PATCH 13/13] cxl: Program decoders for regions

[Jonathan Cameron]

On Thu,  6 Jan 2022 16:37:56 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> Do the HDM decoder programming for all endpoints and host bridges in a
> region. Switches are currently unimplemented.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
Hi Ben,

Minor bug in the maths below that I'd missed eyeballing the registers
because it happened to give nearly the write value for my normal test config
by complete coincidence...

You may well have already caught this one - I've not checked your latest
tree.

> +/**
> + * cxl_commit_decoder() - Program a configured cxl_decoder
> + * @cxld: The preconfigured cxl decoder.
> + *
> + * A cxl decoder that is to be committed should have been earmarked as enabled.
> + * This mechanism acts as a soft reservation on the decoder.
> + *
> + * Returns 0 if commit was successful, negative error code otherwise.
> + */
> +int cxl_commit_decoder(struct cxl_decoder *cxld)
> +{
> +	u32 ctrl, tl_lo, tl_hi, base_lo, base_hi, size_lo, size_hi;
> +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> +	struct cxl_port_state *cxlps;
> +	void __iomem *hdm_decoder;
> +	int rc;
> +
> +	/*
> +	 * Decoder flags are entirely software controlled and therefore this
> +	 * case is purely a driver bug.
> +	 */
> +	if (dev_WARN_ONCE(&port->dev, (cxld->flags & CXL_DECODER_F_ENABLE) != 0,
> +			  "Invalid %s enable state\n", dev_name(&cxld->dev)))
> +		return -ENXIO;
> +
> +	cxlps = dev_get_drvdata(&port->dev);
> +	hdm_decoder = cxlps->regs.hdm_decoder;
> +	ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> +
> +	/*
> +	 * A decoder that's currently active cannot be changed without the
> +	 * system being quiesced. While the driver should prevent against this,
> +	 * for a variety of reasons the hardware might not be in sync with the
> +	 * hardware and so, do not splat on error.
> +	 */
> +	size_hi = readl(hdm_decoder +
> +			CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> +	size_lo =
> +		readl(hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> +	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl) &&
> +	    (size_lo + size_hi)) {
> +		dev_err(&port->dev, "Tried to change an active decoder (%s)\n",
> +			dev_name(&cxld->dev));
> +		return -EBUSY;
> +	}
> +
> +	u32p_replace_bits(&ctrl, 8 - ilog2(cxld->interleave_granularity),

This maths is wrong.   interleave_granularity is stored here as log2() anyway
and should be cxld->interleave_granularity - 8;



> +			  CXL_HDM_DECODER0_CTRL_IG_MASK);
> +	u32p_replace_bits(&ctrl, ilog2(cxld->interleave_ways),
> +			  CXL_HDM_DECODER0_CTRL_IW_MASK);

Re: [PATCH 13/13] cxl: Program decoders for regions

[Ben Widawsky]

On 22-01-12 14:53:28, Jonathan Cameron wrote:
> On Thu,  6 Jan 2022 16:37:56 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
> 
> > Do the HDM decoder programming for all endpoints and host bridges in a
> > region. Switches are currently unimplemented.
> > 
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> Hi Ben,
> 
> Minor bug in the maths below that I'd missed eyeballing the registers
> because it happened to give nearly the write value for my normal test config
> by complete coincidence...
> 
> You may well have already caught this one - I've not checked your latest
> tree.
> 
> > +/**
> > + * cxl_commit_decoder() - Program a configured cxl_decoder
> > + * @cxld: The preconfigured cxl decoder.
> > + *
> > + * A cxl decoder that is to be committed should have been earmarked as enabled.
> > + * This mechanism acts as a soft reservation on the decoder.
> > + *
> > + * Returns 0 if commit was successful, negative error code otherwise.
> > + */
> > +int cxl_commit_decoder(struct cxl_decoder *cxld)
> > +{
> > +	u32 ctrl, tl_lo, tl_hi, base_lo, base_hi, size_lo, size_hi;
> > +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > +	struct cxl_port_state *cxlps;
> > +	void __iomem *hdm_decoder;
> > +	int rc;
> > +
> > +	/*
> > +	 * Decoder flags are entirely software controlled and therefore this
> > +	 * case is purely a driver bug.
> > +	 */
> > +	if (dev_WARN_ONCE(&port->dev, (cxld->flags & CXL_DECODER_F_ENABLE) != 0,
> > +			  "Invalid %s enable state\n", dev_name(&cxld->dev)))
> > +		return -ENXIO;
> > +
> > +	cxlps = dev_get_drvdata(&port->dev);
> > +	hdm_decoder = cxlps->regs.hdm_decoder;
> > +	ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > +
> > +	/*
> > +	 * A decoder that's currently active cannot be changed without the
> > +	 * system being quiesced. While the driver should prevent against this,
> > +	 * for a variety of reasons the hardware might not be in sync with the
> > +	 * hardware and so, do not splat on error.
> > +	 */
> > +	size_hi = readl(hdm_decoder +
> > +			CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> > +	size_lo =
> > +		readl(hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> > +	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl) &&
> > +	    (size_lo + size_hi)) {
> > +		dev_err(&port->dev, "Tried to change an active decoder (%s)\n",
> > +			dev_name(&cxld->dev));
> > +		return -EBUSY;
> > +	}
> > +
> > +	u32p_replace_bits(&ctrl, 8 - ilog2(cxld->interleave_granularity),
> 
> This maths is wrong.   interleave_granularity is stored here as log2() anyway
> and should be cxld->interleave_granularity - 8;

Thanks. interleave_granularity was supposed to move to the absolute value and so
the math here should be correct (although in my current rev I have extracted the
math to a separate function).

I'll fix the meaning of granularity...

> 
> 
> 
> > +			  CXL_HDM_DECODER0_CTRL_IG_MASK);
> > +	u32p_replace_bits(&ctrl, ilog2(cxld->interleave_ways),
> > +			  CXL_HDM_DECODER0_CTRL_IW_MASK);

Re: [PATCH 00/13] CXL Region driver

[Ben Widawsky]

Hi all.

I found some last minute bugs which I've fixed. Please ignore this series and
wait until I send out v3.

Thanks.
Ben

On 22-01-06 16:37:43, Ben Widawsky wrote:
> This patch series introduces the CXL region driver as well as associated APIs in
> CXL core. The region driver enables the creation of "regions" which is a concept
> defined by the CXL 2.0 specification [1]. Region verification and programming
> state are owned by the cxl_region driver (implemented in the cxl_region module).
> It relies on cxl_mem to determine if devices are CXL routed, and cxl_port to
> actually handle the programming of the HDM decoders. Much of the region driver
> is an implementation of algorithms described in the CXL Type 3 Memory Device
> Software Guide [2].
> 
> The region driver will be responsible for configuring regions found on
> persistent capacities in the Label Storage Area (LSA), it will also enumerate
> regions configured by BIOS, usually volatile capacities, and will allow for
> dynamic region creation (which can then be stored in the LSA). It is the primary
> consumer of the CXL Port [3] and CXL Mem drivers introduced previously [4]. Dan
> as reworked some of this which is required for this branch. A cached copy is
> included in the gitlab for this project [5]. Those patches will be posted
> shortly.
> 
> The patches for the region driver could be squashed. They're broken out to aid
> review and because that's the order they were implemented in. My preference is
> to keep those as they are.
> 
> Some things are still missing and will be worked on while these are reviewed (in
> priority order):
> 1. Connection to libnvdimm labels (No plan)
> 2. Volatile regions/LSA regions (Have a plan)
> 3. Switch ports (Have a plan)
> 4. Decoder programming restrictions (No plan). The one know restriction I've
>    missed is to disallow programming HDM decoders that aren't in incremental
>    system physical address ranges.
> 5. Stress testing
> 
> Here is an example of output when programming a x2 interleave region:
> ./cxlctl create-region -i2 -n -a -s $((512<<20)) /sys/bus/cxl/devices/decoder0.0
> [   42.971496][  T644] cxl_core:cxl_bus_probe:1384: cxl_region region0.0:0: probe: -19
> [   42.972400][  T644] cxl_core:cxl_add_region:478: cxl region0.0:0: Added region0.0:0 to decoder0.0
> [   42.979388][  T644] cxl_core:cxl_commit_decoder:394: cxl_port port1: decoder1.0
> [   42.979388][  T644] 	Base 0x0000004c00000000
> [   42.979388][  T644] 	Size 536870912
> [   42.979388][  T644] 	IG 8
> [   42.979388][  T644] 	IW 2
> [   42.979388][  T644] 	TargetList: 0 1 -1 -1 -1 -1 -1 -1
> [   42.982410][  T644] cxl_core:cxl_commit_decoder:394: cxl_port endpoint3: decoder3.0
> [   42.982410][  T644] 	Base 0x0000004c00000000
> [   42.982410][  T644] 	Size 536870912
> [   42.982410][  T644] 	IG 8
> [   42.982410][  T644] 	IW 2
> [   42.982410][  T644] 	TargetList: -1 -1 -1 -1 -1 -1 -1 -1
> [   42.985427][  T644] cxl_core:cxl_commit_decoder:394: cxl_port endpoint2: decoder2.0
> [   42.985427][  T644] 	Base 0x0000004c00000000
> [   42.985427][  T644] 	Size 536870912
> [   42.985427][  T644] 	IG 8
> [   42.985427][  T644] 	IW 2
> [   42.985427][  T644] 	TargetList: -1 -1 -1 -1 -1 -1 -1 -1
> [   42.987937][  T644] cxl_core:cxl_bus_probe:1384: cxl_region region0.0:0: probe: 0
> 
> If you're wondering how I tested this, I've baked it into my cxlctl app [6] and
> lib [7]. Eventually this will get absorbed by ndctl/cxl-cli/libcxl.
> 
> To get the detailed errors, trace-cmd can be utilized. Until a region device
> exists, the region module will not be loaded, which means the region tracepoints
> will not exist. To get around this, modprobe cxl_region before anything.
> 
> trace-cmd record -e cxl ./cxlctl create-region -n -a -s $((256<<20)) /sys/bus/cxl/devices/decoder0.0
> 
> Branch can be found at gitlab [8].
> 
> ---
> 
> [1]: https://www.computeexpresslink.org/download-the-specification
> [2]: https://cdrdv2.intel.com/v1/dl/getContent/643805?wapkw=CXL%20memory%20device%20sw%20guide
> [3]: https://lore.kernel.org/linux-cxl/20211022183709.1199701-9-ben.widawsky@intel.com/
> [4]: https://lore.kernel.org/linux-cxl/20211022183709.1199701-17-ben.widawsky@intel.com/
> [5]: https://gitlab.com/bwidawsk/linux/-/commit/126793e22427f7975a8f2fca373764be78012e88
> [6]: https://gitlab.com/bwidawsk-cxl/cxlctl
> [7]: https://gitlab.com/bwidawsk-cxl/cxl_rs
> [8]: https://gitlab.com/bwidawsk/linux/-/tree/cxl_region
> 
> ---
> 
> Ben Widawsky (13):
>   cxl/core: Rename find_cxl_port
>   cxl/core: Track port depth
>   cxl/region: Add region creation ABI
>   cxl/region: Introduce concept of region configuration
>   cxl/mem: Cache port created by the mem dev
>   cxl/region: Introduce a cxl_region driver
>   cxl/acpi: Handle address space allocation
>   cxl/region: Address space allocation
>   cxl/region: Implement XHB verification
>   cxl/region: HB port config verification
>   cxl/region: Add infrastructure for decoder programming
>   cxl/region: Record host bridge target list
>   cxl: Program decoders for regions
> 
>  .clang-format                                 |   3 +
>  Documentation/ABI/testing/sysfs-bus-cxl       |  63 ++
>  .../driver-api/cxl/memory-devices.rst         |  14 +
>  drivers/cxl/Makefile                          |   2 +
>  drivers/cxl/acpi.c                            |  30 +
>  drivers/cxl/core/Makefile                     |   1 +
>  drivers/cxl/core/core.h                       |   4 +
>  drivers/cxl/core/hdm.c                        | 198 +++++
>  drivers/cxl/core/port.c                       | 132 +++-
>  drivers/cxl/core/region.c                     | 525 +++++++++++++
>  drivers/cxl/cxl.h                             |  32 +
>  drivers/cxl/cxlmem.h                          |   9 +
>  drivers/cxl/mem.c                             |  16 +-
>  drivers/cxl/port.c                            |  42 +-
>  drivers/cxl/region.c                          | 695 ++++++++++++++++++
>  drivers/cxl/region.h                          |  47 ++
>  drivers/cxl/trace.h                           |  54 ++
>  tools/testing/cxl/Kbuild                      |   1 +
>  18 files changed, 1849 insertions(+), 19 deletions(-)
>  create mode 100644 drivers/cxl/core/region.c
>  create mode 100644 drivers/cxl/region.c
>  create mode 100644 drivers/cxl/region.h
>  create mode 100644 drivers/cxl/trace.h
> 
> 
> base-commit: 03716ce2db3c17ba38f26a88d75049c0472a629e
> prerequisite-patch-id: af6a0315e22bfc1099d4f58610b8b897e6e5a060
> -- 
> 2.34.1
>