linux-cxl.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers
@ 2021-07-23 21:06 Ben Widawsky
  2021-07-23 21:06 ` [PATCH 01/23] cxl: Move cxl_core to new directory Ben Widawsky
                   ` (22 more replies)
  0 siblings, 23 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma


I will be going on vacation for the next two weeks so I'm sending out everything
I have, even though it's not fully baked so that folks who might want to
continue it while I'm gone, can do so. Many of these patches have already been
sent as separate series, or RFC.

The ultimate goal of this series is to create two new driver types, cxl_region,
and cxl_memdev. The cxl_region driver is responsible for region creation and
configuration, and the cxl_memdev driver is responsible for handling the CXL
specific (non-PCI) parts of CXL endpoints. In order to establish that in a clean
way, the cxl_core driver needed quite a bit of rework. The next step is to
complete the HDM decoder programming.

What's specifically not fully baked is the cxl_pci driver claims part of the
component register space, but the cxl_memdev driver also wants this resource.
Better coordination here needs to happen, most likely the cxl_pci driver must
relinquish any resources it won't be needing. Some of the patches started to
enable making that easy, but it's not complete and as a result I believe these
patches will OOPS (there may be other issues as well).

If you feel like reviewing any of these patches, that'd be great, but I'm
equally happy to resubmit when I'm back and have things fully working.

Patch breakdown
1-11: cxl_core reorganization
12-18: cxl_region driver
19-23: cxl_memdev driver

These patches can be obtained from my gitlab as well:
https://gitlab.com/bwidawsk/linux/-/tree/cxl_memdrver2

Ben Widawsky (23):
  cxl: Move cxl_core to new directory
  cxl/core: Improve CXL core kernel docs
  cxl/core: Extract register and pmem functionality
  cxl/mem: Move character device region creation
  cxl: Pass fops and shutdown to memdev creation
  cxl/core: Move memdev management to core
  cxl/pci: Ignore unknown register block types
  cxl/pci: Simplify register setup
  cxl/pci: Retain map information in cxl_mem_probe
  cxl/decoder: Support parentless decoders
  cxl: Enable an endpoint decoder type
  cxl/region: Add region creation ABI
  cxl/region: Introduce concept of region configuration
  cxl: Convert driver id to an enum
  cxl/region: Introduce a cxl_region driver
  cxl/core: Convert decoder range to resource
  cxl/region: Handle region's address space allocation
  cxl/region: Only allow CXL capable targets
  cxl/mem: Introduce CXL mem driver
  cxl/memdev: Determine CXL.mem capability
  cxl/mem: Check that the device is CXL.mem capable
  cxl/mem: Add device as a port
  cxl/core: Map component registers for ports

 Documentation/ABI/testing/sysfs-bus-cxl       |  53 ++
 .../driver-api/cxl/memory-devices.rst         |  25 +-
 drivers/cxl/Makefile                          |   9 +-
 drivers/cxl/acpi.c                            |  44 +-
 drivers/cxl/core/Makefile                     |   5 +
 drivers/cxl/{core.c => core/bus.c}            | 711 +++++++-----------
 drivers/cxl/core/core.h                       |  22 +
 drivers/cxl/core/memdev.c                     | 230 ++++++
 drivers/cxl/core/pmem.c                       | 201 +++++
 drivers/cxl/core/region.c                     | 389 ++++++++++
 drivers/cxl/core/regs.c                       | 235 ++++++
 drivers/cxl/cxl.h                             |  67 +-
 drivers/cxl/mem.c                             |  94 +++
 drivers/cxl/mem.h                             |  19 +
 drivers/cxl/pci.c                             | 325 ++------
 drivers/cxl/pci.h                             |   8 +-
 drivers/cxl/region.c                          | 134 ++++
 drivers/cxl/region.h                          |  34 +
 drivers/cxl/trace.h                           |  33 +
 19 files changed, 1867 insertions(+), 771 deletions(-)
 create mode 100644 drivers/cxl/core/Makefile
 rename drivers/cxl/{core.c => core/bus.c} (55%)
 create mode 100644 drivers/cxl/core/core.h
 create mode 100644 drivers/cxl/core/memdev.c
 create mode 100644 drivers/cxl/core/pmem.c
 create mode 100644 drivers/cxl/core/region.c
 create mode 100644 drivers/cxl/core/regs.c
 create mode 100644 drivers/cxl/mem.c
 create mode 100644 drivers/cxl/region.c
 create mode 100644 drivers/cxl/region.h
 create mode 100644 drivers/cxl/trace.h

-- 
2.32.0


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 01/23] cxl: Move cxl_core to new directory
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 02/23] cxl/core: Improve CXL core kernel docs Ben Widawsky
                   ` (21 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

CXL core is growing, and it's already arguably unmanageable. To support
future growth, move core functionality to a new directory and rename the
file to represent just bus support. Future work will remove non-bus
functionality.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 Documentation/driver-api/cxl/memory-devices.rst | 2 +-
 drivers/cxl/Makefile                            | 4 +---
 drivers/cxl/core/Makefile                       | 5 +++++
 drivers/cxl/{core.c => core/bus.c}              | 4 ++--
 4 files changed, 9 insertions(+), 6 deletions(-)
 create mode 100644 drivers/cxl/core/Makefile
 rename drivers/cxl/{core.c => core/bus.c} (99%)

diff --git a/Documentation/driver-api/cxl/memory-devices.rst b/Documentation/driver-api/cxl/memory-devices.rst
index 487ce4f41d77..a86e2c7c551a 100644
--- a/Documentation/driver-api/cxl/memory-devices.rst
+++ b/Documentation/driver-api/cxl/memory-devices.rst
@@ -36,7 +36,7 @@ CXL Core
 .. kernel-doc:: drivers/cxl/cxl.h
    :internal:
 
-.. kernel-doc:: drivers/cxl/core.c
+.. kernel-doc:: drivers/cxl/core/bus.c
    :doc: cxl core
 
 External Interfaces
diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index 32954059b37b..d1aaabc940f3 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -1,11 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_CXL_BUS) += cxl_core.o
+obj-$(CONFIG_CXL_BUS) += core/
 obj-$(CONFIG_CXL_MEM) += cxl_pci.o
 obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
 obj-$(CONFIG_CXL_PMEM) += cxl_pmem.o
 
-ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL
-cxl_core-y := core.o
 cxl_pci-y := pci.o
 cxl_acpi-y := acpi.o
 cxl_pmem-y := pmem.o
diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
new file mode 100644
index 000000000000..c65e9f61abe9
--- /dev/null
+++ b/drivers/cxl/core/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_CXL_BUS) += cxl_core.o
+
+ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL -I./drivers/cxl
+cxl_core-y := bus.o
diff --git a/drivers/cxl/core.c b/drivers/cxl/core/bus.c
similarity index 99%
rename from drivers/cxl/core.c
rename to drivers/cxl/core/bus.c
index a2e4d54fc7bc..6ce04e3976d2 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core/bus.c
@@ -6,8 +6,8 @@
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/idr.h>
-#include "cxl.h"
-#include "mem.h"
+#include <cxl.h>
+#include <mem.h>
 
 /**
  * DOC: cxl core
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 02/23] cxl/core: Improve CXL core kernel docs
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
  2021-07-23 21:06 ` [PATCH 01/23] cxl: Move cxl_core to new directory Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 03/23] cxl/core: Extract register and pmem functionality Ben Widawsky
                   ` (20 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

Now that CXL core's role is well understood, the documentation should
reflect that information.

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

diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index 6ce04e3976d2..647b8a00ab36 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -12,8 +12,15 @@
 /**
  * DOC: cxl core
  *
- * The CXL core provides a sysfs hierarchy for control devices and a rendezvous
- * point for cross-device interleave coordination through cxl ports.
+ * The CXL core provides a set of interfaces that can be consumed by CXL aware
+ * drivers. The interfaces allow for creation, modification, and destruction of
+ * regions, memory devices, ports, and decoders. CXL aware drivers must register
+ * with the CXL core via these interfaces in order to be able to participate in
+ * cross-device interleave coordination. The CXL core also establishes and
+ * maintains the bridge to the nvdimm subsystem.
+ *
+ * CXL core introduces sysfs hierarchy to control the devices that are
+ * instantiated by the core.
  */
 
 static DEFINE_IDA(cxl_port_ida);
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 03/23] cxl/core: Extract register and pmem functionality
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
  2021-07-23 21:06 ` [PATCH 01/23] cxl: Move cxl_core to new directory Ben Widawsky
  2021-07-23 21:06 ` [PATCH 02/23] cxl/core: Improve CXL core kernel docs Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 04/23] cxl/mem: Move character device region creation Ben Widawsky
                   ` (19 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

Register mapping and pmem/nvdimm integration are distinct enough from
basic CXL bus functionality that it warrants being moved out of bus.c
Additionally, this aims to modularize for the sake of reducing the size
of bus.c

pmem and register programming have very clear separation and are done
together for that reason. Other parts of core, like ports and decoders
should be pulled out as well, but those are more integrated with core
and therefore saved for later.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 .../driver-api/cxl/memory-devices.rst         |   6 +
 drivers/cxl/Makefile                          |   1 +
 drivers/cxl/core/Makefile                     |   2 +-
 drivers/cxl/core/bus.c                        | 435 +-----------------
 drivers/cxl/core/core.h                       |  20 +
 drivers/cxl/core/pmem.c                       | 201 ++++++++
 drivers/cxl/core/regs.c                       | 235 ++++++++++
 7 files changed, 467 insertions(+), 433 deletions(-)
 create mode 100644 drivers/cxl/core/core.h
 create mode 100644 drivers/cxl/core/pmem.c
 create mode 100644 drivers/cxl/core/regs.c

diff --git a/Documentation/driver-api/cxl/memory-devices.rst b/Documentation/driver-api/cxl/memory-devices.rst
index a86e2c7c551a..46847d8c70a0 100644
--- a/Documentation/driver-api/cxl/memory-devices.rst
+++ b/Documentation/driver-api/cxl/memory-devices.rst
@@ -39,6 +39,12 @@ CXL Core
 .. kernel-doc:: drivers/cxl/core/bus.c
    :doc: cxl core
 
+.. kernel-doc:: drivers/cxl/core/pmem.c
+   :internal:
+
+.. kernel-doc:: drivers/cxl/core/regs.c
+   :internal:
+
 External Interfaces
 ===================
 
diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index d1aaabc940f3..febb2c3f5fc6 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_CXL_MEM) += cxl_pci.o
 obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
 obj-$(CONFIG_CXL_PMEM) += cxl_pmem.o
 
+ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL
 cxl_pci-y := pci.o
 cxl_acpi-y := acpi.o
 cxl_pmem-y := pmem.o
diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
index c65e9f61abe9..289e8c8deebb 100644
--- a/drivers/cxl/core/Makefile
+++ b/drivers/cxl/core/Makefile
@@ -2,4 +2,4 @@
 obj-$(CONFIG_CXL_BUS) += cxl_core.o
 
 ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL -I./drivers/cxl
-cxl_core-y := bus.o
+cxl_core-y := bus.o pmem.o regs.o
diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index 647b8a00ab36..33196813ebcb 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -6,8 +6,7 @@
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/idr.h>
-#include <cxl.h>
-#include <mem.h>
+#include "core.h"
 
 /**
  * DOC: cxl core
@@ -37,7 +36,7 @@ static struct attribute *cxl_base_attributes[] = {
 	NULL,
 };
 
-static struct attribute_group cxl_base_attribute_group = {
+struct attribute_group cxl_base_attribute_group = {
 	.attrs = cxl_base_attributes,
 };
 
@@ -514,11 +513,6 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
 	return ERR_PTR(rc);
 }
 
-static void unregister_dev(void *dev)
-{
-	device_unregister(dev);
-}
-
 struct cxl_decoder *
 devm_cxl_add_decoder(struct device *host, struct cxl_port *port, int nr_targets,
 		     resource_size_t base, resource_size_t len,
@@ -543,7 +537,7 @@ devm_cxl_add_decoder(struct device *host, struct cxl_port *port, int nr_targets,
 	if (rc)
 		goto err;
 
-	rc = devm_add_action_or_reset(host, unregister_dev, dev);
+	rc = devm_add_action_or_reset(host, unregister_cxl_dev, dev);
 	if (rc)
 		return ERR_PTR(rc);
 	return cxld;
@@ -554,429 +548,6 @@ devm_cxl_add_decoder(struct device *host, struct cxl_port *port, int nr_targets,
 }
 EXPORT_SYMBOL_GPL(devm_cxl_add_decoder);
 
-/**
- * cxl_probe_component_regs() - Detect CXL Component register blocks
- * @dev: Host device of the @base mapping
- * @base: Mapping containing the HDM Decoder Capability Header
- * @map: Map object describing the register block information found
- *
- * See CXL 2.0 8.2.4 Component Register Layout and Definition
- * See CXL 2.0 8.2.5.5 CXL Device Register Interface
- *
- * Probe for component register information and return it in map object.
- */
-void cxl_probe_component_regs(struct device *dev, void __iomem *base,
-			      struct cxl_component_reg_map *map)
-{
-	int cap, cap_count;
-	u64 cap_array;
-
-	*map = (struct cxl_component_reg_map) { 0 };
-
-	/*
-	 * CXL.cache and CXL.mem registers are at offset 0x1000 as defined in
-	 * CXL 2.0 8.2.4 Table 141.
-	 */
-	base += CXL_CM_OFFSET;
-
-	cap_array = readq(base + CXL_CM_CAP_HDR_OFFSET);
-
-	if (FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, cap_array) != CM_CAP_HDR_CAP_ID) {
-		dev_err(dev,
-			"Couldn't locate the CXL.cache and CXL.mem capability array header./n");
-		return;
-	}
-
-	/* It's assumed that future versions will be backward compatible */
-	cap_count = FIELD_GET(CXL_CM_CAP_HDR_ARRAY_SIZE_MASK, cap_array);
-
-	for (cap = 1; cap <= cap_count; cap++) {
-		void __iomem *register_block;
-		u32 hdr;
-		int decoder_cnt;
-		u16 cap_id, offset;
-		u32 length;
-
-		hdr = readl(base + cap * 0x4);
-
-		cap_id = FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, hdr);
-		offset = FIELD_GET(CXL_CM_CAP_PTR_MASK, hdr);
-		register_block = base + offset;
-
-		switch (cap_id) {
-		case CXL_CM_CAP_CAP_ID_HDM:
-			dev_dbg(dev, "found HDM decoder capability (0x%x)\n",
-				offset);
-
-			hdr = readl(register_block);
-
-			decoder_cnt = cxl_hdm_decoder_count(hdr);
-			length = 0x20 * decoder_cnt + 0x10;
-
-			map->hdm_decoder.valid = true;
-			map->hdm_decoder.offset = CXL_CM_OFFSET + offset;
-			map->hdm_decoder.size = length;
-			break;
-		default:
-			dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,
-				offset);
-			break;
-		}
-	}
-}
-EXPORT_SYMBOL_GPL(cxl_probe_component_regs);
-
-static void cxl_nvdimm_bridge_release(struct device *dev)
-{
-	struct cxl_nvdimm_bridge *cxl_nvb = to_cxl_nvdimm_bridge(dev);
-
-	kfree(cxl_nvb);
-}
-
-static const struct attribute_group *cxl_nvdimm_bridge_attribute_groups[] = {
-	&cxl_base_attribute_group,
-	NULL,
-};
-
-static const struct device_type cxl_nvdimm_bridge_type = {
-	.name = "cxl_nvdimm_bridge",
-	.release = cxl_nvdimm_bridge_release,
-	.groups = cxl_nvdimm_bridge_attribute_groups,
-};
-
-struct cxl_nvdimm_bridge *to_cxl_nvdimm_bridge(struct device *dev)
-{
-	if (dev_WARN_ONCE(dev, dev->type != &cxl_nvdimm_bridge_type,
-			  "not a cxl_nvdimm_bridge device\n"))
-		return NULL;
-	return container_of(dev, struct cxl_nvdimm_bridge, dev);
-}
-EXPORT_SYMBOL_GPL(to_cxl_nvdimm_bridge);
-
-static struct cxl_nvdimm_bridge *
-cxl_nvdimm_bridge_alloc(struct cxl_port *port)
-{
-	struct cxl_nvdimm_bridge *cxl_nvb;
-	struct device *dev;
-
-	cxl_nvb = kzalloc(sizeof(*cxl_nvb), GFP_KERNEL);
-	if (!cxl_nvb)
-		return ERR_PTR(-ENOMEM);
-
-	dev = &cxl_nvb->dev;
-	cxl_nvb->port = port;
-	cxl_nvb->state = CXL_NVB_NEW;
-	device_initialize(dev);
-	device_set_pm_not_required(dev);
-	dev->parent = &port->dev;
-	dev->bus = &cxl_bus_type;
-	dev->type = &cxl_nvdimm_bridge_type;
-
-	return cxl_nvb;
-}
-
-static void unregister_nvb(void *_cxl_nvb)
-{
-	struct cxl_nvdimm_bridge *cxl_nvb = _cxl_nvb;
-	bool flush;
-
-	/*
-	 * If the bridge was ever activated then there might be in-flight state
-	 * work to flush. Once the state has been changed to 'dead' then no new
-	 * work can be queued by user-triggered bind.
-	 */
-	device_lock(&cxl_nvb->dev);
-	flush = cxl_nvb->state != CXL_NVB_NEW;
-	cxl_nvb->state = CXL_NVB_DEAD;
-	device_unlock(&cxl_nvb->dev);
-
-	/*
-	 * Even though the device core will trigger device_release_driver()
-	 * before the unregister, it does not know about the fact that
-	 * cxl_nvdimm_bridge_driver defers ->remove() work. So, do the driver
-	 * release not and flush it before tearing down the nvdimm device
-	 * hierarchy.
-	 */
-	device_release_driver(&cxl_nvb->dev);
-	if (flush)
-		flush_work(&cxl_nvb->state_work);
-	device_unregister(&cxl_nvb->dev);
-}
-
-struct cxl_nvdimm_bridge *devm_cxl_add_nvdimm_bridge(struct device *host,
-						     struct cxl_port *port)
-{
-	struct cxl_nvdimm_bridge *cxl_nvb;
-	struct device *dev;
-	int rc;
-
-	if (!IS_ENABLED(CONFIG_CXL_PMEM))
-		return ERR_PTR(-ENXIO);
-
-	cxl_nvb = cxl_nvdimm_bridge_alloc(port);
-	if (IS_ERR(cxl_nvb))
-		return cxl_nvb;
-
-	dev = &cxl_nvb->dev;
-	rc = dev_set_name(dev, "nvdimm-bridge");
-	if (rc)
-		goto err;
-
-	rc = device_add(dev);
-	if (rc)
-		goto err;
-
-	rc = devm_add_action_or_reset(host, unregister_nvb, cxl_nvb);
-	if (rc)
-		return ERR_PTR(rc);
-
-	return cxl_nvb;
-
-err:
-	put_device(dev);
-	return ERR_PTR(rc);
-}
-EXPORT_SYMBOL_GPL(devm_cxl_add_nvdimm_bridge);
-
-static void cxl_nvdimm_release(struct device *dev)
-{
-	struct cxl_nvdimm *cxl_nvd = to_cxl_nvdimm(dev);
-
-	kfree(cxl_nvd);
-}
-
-static const struct attribute_group *cxl_nvdimm_attribute_groups[] = {
-	&cxl_base_attribute_group,
-	NULL,
-};
-
-static const struct device_type cxl_nvdimm_type = {
-	.name = "cxl_nvdimm",
-	.release = cxl_nvdimm_release,
-	.groups = cxl_nvdimm_attribute_groups,
-};
-
-bool is_cxl_nvdimm(struct device *dev)
-{
-	return dev->type == &cxl_nvdimm_type;
-}
-EXPORT_SYMBOL_GPL(is_cxl_nvdimm);
-
-struct cxl_nvdimm *to_cxl_nvdimm(struct device *dev)
-{
-	if (dev_WARN_ONCE(dev, !is_cxl_nvdimm(dev),
-			  "not a cxl_nvdimm device\n"))
-		return NULL;
-	return container_of(dev, struct cxl_nvdimm, dev);
-}
-EXPORT_SYMBOL_GPL(to_cxl_nvdimm);
-
-static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_memdev *cxlmd)
-{
-	struct cxl_nvdimm *cxl_nvd;
-	struct device *dev;
-
-	cxl_nvd = kzalloc(sizeof(*cxl_nvd), GFP_KERNEL);
-	if (!cxl_nvd)
-		return ERR_PTR(-ENOMEM);
-
-	dev = &cxl_nvd->dev;
-	cxl_nvd->cxlmd = cxlmd;
-	device_initialize(dev);
-	device_set_pm_not_required(dev);
-	dev->parent = &cxlmd->dev;
-	dev->bus = &cxl_bus_type;
-	dev->type = &cxl_nvdimm_type;
-
-	return cxl_nvd;
-}
-
-int devm_cxl_add_nvdimm(struct device *host, struct cxl_memdev *cxlmd)
-{
-	struct cxl_nvdimm *cxl_nvd;
-	struct device *dev;
-	int rc;
-
-	cxl_nvd = cxl_nvdimm_alloc(cxlmd);
-	if (IS_ERR(cxl_nvd))
-		return PTR_ERR(cxl_nvd);
-
-	dev = &cxl_nvd->dev;
-	rc = dev_set_name(dev, "pmem%d", cxlmd->id);
-	if (rc)
-		goto err;
-
-	rc = device_add(dev);
-	if (rc)
-		goto err;
-
-	dev_dbg(host, "%s: register %s\n", dev_name(dev->parent),
-		dev_name(dev));
-
-	return devm_add_action_or_reset(host, unregister_dev, dev);
-
-err:
-	put_device(dev);
-	return rc;
-}
-EXPORT_SYMBOL_GPL(devm_cxl_add_nvdimm);
-
-/**
- * cxl_probe_device_regs() - Detect CXL Device register blocks
- * @dev: Host device of the @base mapping
- * @base: Mapping of CXL 2.0 8.2.8 CXL Device Register Interface
- * @map: Map object describing the register block information found
- *
- * Probe for device register information and return it in map object.
- */
-void cxl_probe_device_regs(struct device *dev, void __iomem *base,
-			   struct cxl_device_reg_map *map)
-{
-	int cap, cap_count;
-	u64 cap_array;
-
-	*map = (struct cxl_device_reg_map){ 0 };
-
-	cap_array = readq(base + CXLDEV_CAP_ARRAY_OFFSET);
-	if (FIELD_GET(CXLDEV_CAP_ARRAY_ID_MASK, cap_array) !=
-	    CXLDEV_CAP_ARRAY_CAP_ID)
-		return;
-
-	cap_count = FIELD_GET(CXLDEV_CAP_ARRAY_COUNT_MASK, cap_array);
-
-	for (cap = 1; cap <= cap_count; cap++) {
-		u32 offset, length;
-		u16 cap_id;
-
-		cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK,
-				   readl(base + cap * 0x10));
-		offset = readl(base + cap * 0x10 + 0x4);
-		length = readl(base + cap * 0x10 + 0x8);
-
-		switch (cap_id) {
-		case CXLDEV_CAP_CAP_ID_DEVICE_STATUS:
-			dev_dbg(dev, "found Status capability (0x%x)\n", offset);
-
-			map->status.valid = true;
-			map->status.offset = offset;
-			map->status.size = length;
-			break;
-		case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX:
-			dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset);
-			map->mbox.valid = true;
-			map->mbox.offset = offset;
-			map->mbox.size = length;
-			break;
-		case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX:
-			dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset);
-			break;
-		case CXLDEV_CAP_CAP_ID_MEMDEV:
-			dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset);
-			map->memdev.valid = true;
-			map->memdev.offset = offset;
-			map->memdev.size = length;
-			break;
-		default:
-			if (cap_id >= 0x8000)
-				dev_dbg(dev, "Vendor cap ID: %#x offset: %#x\n", cap_id, offset);
-			else
-				dev_dbg(dev, "Unknown cap ID: %#x offset: %#x\n", cap_id, offset);
-			break;
-		}
-	}
-}
-EXPORT_SYMBOL_GPL(cxl_probe_device_regs);
-
-static void __iomem *devm_cxl_iomap_block(struct device *dev,
-					  resource_size_t addr,
-					  resource_size_t length)
-{
-	void __iomem *ret_val;
-	struct resource *res;
-
-	res = devm_request_mem_region(dev, addr, length, dev_name(dev));
-	if (!res) {
-		resource_size_t end = addr + length - 1;
-
-		dev_err(dev, "Failed to request region %pa-%pa\n", &addr, &end);
-		return NULL;
-	}
-
-	ret_val = devm_ioremap(dev, addr, length);
-	if (!ret_val)
-		dev_err(dev, "Failed to map region %pr\n", res);
-
-	return ret_val;
-}
-
-int cxl_map_component_regs(struct pci_dev *pdev,
-			   struct cxl_component_regs *regs,
-			   struct cxl_register_map *map)
-{
-	struct device *dev = &pdev->dev;
-	resource_size_t phys_addr;
-	resource_size_t length;
-
-	phys_addr = pci_resource_start(pdev, map->barno);
-	phys_addr += map->block_offset;
-
-	phys_addr += map->component_map.hdm_decoder.offset;
-	length = map->component_map.hdm_decoder.size;
-	regs->hdm_decoder = devm_cxl_iomap_block(dev, phys_addr, length);
-	if (!regs->hdm_decoder)
-		return -ENOMEM;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(cxl_map_component_regs);
-
-int cxl_map_device_regs(struct pci_dev *pdev,
-			struct cxl_device_regs *regs,
-			struct cxl_register_map *map)
-{
-	struct device *dev = &pdev->dev;
-	resource_size_t phys_addr;
-
-	phys_addr = pci_resource_start(pdev, map->barno);
-	phys_addr += map->block_offset;
-
-	if (map->device_map.status.valid) {
-		resource_size_t addr;
-		resource_size_t length;
-
-		addr = phys_addr + map->device_map.status.offset;
-		length = map->device_map.status.size;
-		regs->status = devm_cxl_iomap_block(dev, addr, length);
-		if (!regs->status)
-			return -ENOMEM;
-	}
-
-	if (map->device_map.mbox.valid) {
-		resource_size_t addr;
-		resource_size_t length;
-
-		addr = phys_addr + map->device_map.mbox.offset;
-		length = map->device_map.mbox.size;
-		regs->mbox = devm_cxl_iomap_block(dev, addr, length);
-		if (!regs->mbox)
-			return -ENOMEM;
-	}
-
-	if (map->device_map.memdev.valid) {
-		resource_size_t addr;
-		resource_size_t length;
-
-		addr = phys_addr + map->device_map.memdev.offset;
-		length = map->device_map.memdev.size;
-		regs->memdev = devm_cxl_iomap_block(dev, addr, length);
-		if (!regs->memdev)
-			return -ENOMEM;
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(cxl_map_device_regs);
-
 /**
  * __cxl_driver_register - register a driver for the cxl bus
  * @cxl_drv: cxl driver structure to attach
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
new file mode 100644
index 000000000000..5e5862e4d6af
--- /dev/null
+++ b/drivers/cxl/core/core.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright(c) 2020 Intel Corporation. */
+
+#ifndef __CXL_CORE_H__
+#define __CXL_CORE_H__
+
+#include <cxl.h>
+#include <mem.h>
+
+extern const struct device_type cxl_nvdimm_bridge_type;
+extern const struct device_type cxl_nvdimm_type;
+
+extern struct attribute_group cxl_base_attribute_group;
+
+static inline void unregister_cxl_dev(void *dev)
+{
+	device_unregister(dev);
+}
+
+#endif /* __CXL_CORE_H__ */
diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
new file mode 100644
index 000000000000..b6bf99c1c9bd
--- /dev/null
+++ b/drivers/cxl/core/pmem.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2020 Intel Corporation. */
+
+#include <linux/device.h>
+#include <linux/slab.h>
+#include "core.h"
+
+static void cxl_nvdimm_bridge_release(struct device *dev)
+{
+	struct cxl_nvdimm_bridge *cxl_nvb = to_cxl_nvdimm_bridge(dev);
+
+	kfree(cxl_nvb);
+}
+
+static const struct attribute_group *cxl_nvdimm_bridge_attribute_groups[] = {
+	&cxl_base_attribute_group,
+	NULL,
+};
+
+const struct device_type cxl_nvdimm_bridge_type = {
+	.name = "cxl_nvdimm_bridge",
+	.release = cxl_nvdimm_bridge_release,
+	.groups = cxl_nvdimm_bridge_attribute_groups,
+};
+
+struct cxl_nvdimm_bridge *to_cxl_nvdimm_bridge(struct device *dev)
+{
+	if (dev_WARN_ONCE(dev, dev->type != &cxl_nvdimm_bridge_type,
+			  "not a cxl_nvdimm_bridge device\n"))
+		return NULL;
+	return container_of(dev, struct cxl_nvdimm_bridge, dev);
+}
+EXPORT_SYMBOL_GPL(to_cxl_nvdimm_bridge);
+
+static struct cxl_nvdimm_bridge *
+cxl_nvdimm_bridge_alloc(struct cxl_port *port)
+{
+	struct cxl_nvdimm_bridge *cxl_nvb;
+	struct device *dev;
+
+	cxl_nvb = kzalloc(sizeof(*cxl_nvb), GFP_KERNEL);
+	if (!cxl_nvb)
+		return ERR_PTR(-ENOMEM);
+
+	dev = &cxl_nvb->dev;
+	cxl_nvb->port = port;
+	cxl_nvb->state = CXL_NVB_NEW;
+	device_initialize(dev);
+	device_set_pm_not_required(dev);
+	dev->parent = &port->dev;
+	dev->bus = &cxl_bus_type;
+	dev->type = &cxl_nvdimm_bridge_type;
+
+	return cxl_nvb;
+}
+
+static void unregister_nvb(void *_cxl_nvb)
+{
+	struct cxl_nvdimm_bridge *cxl_nvb = _cxl_nvb;
+	bool flush;
+
+	/*
+	 * If the bridge was ever activated then there might be in-flight state
+	 * work to flush. Once the state has been changed to 'dead' then no new
+	 * work can be queued by user-triggered bind.
+	 */
+	device_lock(&cxl_nvb->dev);
+	flush = cxl_nvb->state != CXL_NVB_NEW;
+	cxl_nvb->state = CXL_NVB_DEAD;
+	device_unlock(&cxl_nvb->dev);
+
+	/*
+	 * Even though the device core will trigger device_release_driver()
+	 * before the unregister, it does not know about the fact that
+	 * cxl_nvdimm_bridge_driver defers ->remove() work. So, do the driver
+	 * release not and flush it before tearing down the nvdimm device
+	 * hierarchy.
+	 */
+	device_release_driver(&cxl_nvb->dev);
+	if (flush)
+		flush_work(&cxl_nvb->state_work);
+	device_unregister(&cxl_nvb->dev);
+}
+
+struct cxl_nvdimm_bridge *devm_cxl_add_nvdimm_bridge(struct device *host,
+						     struct cxl_port *port)
+{
+	struct cxl_nvdimm_bridge *cxl_nvb;
+	struct device *dev;
+	int rc;
+
+	if (!IS_ENABLED(CONFIG_CXL_PMEM))
+		return ERR_PTR(-ENXIO);
+
+	cxl_nvb = cxl_nvdimm_bridge_alloc(port);
+	if (IS_ERR(cxl_nvb))
+		return cxl_nvb;
+
+	dev = &cxl_nvb->dev;
+	rc = dev_set_name(dev, "nvdimm-bridge");
+	if (rc)
+		goto err;
+
+	rc = device_add(dev);
+	if (rc)
+		goto err;
+
+	rc = devm_add_action_or_reset(host, unregister_nvb, cxl_nvb);
+	if (rc)
+		return ERR_PTR(rc);
+
+	return cxl_nvb;
+
+err:
+	put_device(dev);
+	return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_GPL(devm_cxl_add_nvdimm_bridge);
+
+static void cxl_nvdimm_release(struct device *dev)
+{
+	struct cxl_nvdimm *cxl_nvd = to_cxl_nvdimm(dev);
+
+	kfree(cxl_nvd);
+}
+
+static const struct attribute_group *cxl_nvdimm_attribute_groups[] = {
+	&cxl_base_attribute_group,
+	NULL,
+};
+
+const struct device_type cxl_nvdimm_type = {
+	.name = "cxl_nvdimm",
+	.release = cxl_nvdimm_release,
+	.groups = cxl_nvdimm_attribute_groups,
+};
+
+bool is_cxl_nvdimm(struct device *dev)
+{
+	return dev->type == &cxl_nvdimm_type;
+}
+EXPORT_SYMBOL_GPL(is_cxl_nvdimm);
+
+struct cxl_nvdimm *to_cxl_nvdimm(struct device *dev)
+{
+	if (dev_WARN_ONCE(dev, !is_cxl_nvdimm(dev),
+			  "not a cxl_nvdimm device\n"))
+		return NULL;
+	return container_of(dev, struct cxl_nvdimm, dev);
+}
+EXPORT_SYMBOL_GPL(to_cxl_nvdimm);
+
+static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_memdev *cxlmd)
+{
+	struct cxl_nvdimm *cxl_nvd;
+	struct device *dev;
+
+	cxl_nvd = kzalloc(sizeof(*cxl_nvd), GFP_KERNEL);
+	if (!cxl_nvd)
+		return ERR_PTR(-ENOMEM);
+
+	dev = &cxl_nvd->dev;
+	cxl_nvd->cxlmd = cxlmd;
+	device_initialize(dev);
+	device_set_pm_not_required(dev);
+	dev->parent = &cxlmd->dev;
+	dev->bus = &cxl_bus_type;
+	dev->type = &cxl_nvdimm_type;
+
+	return cxl_nvd;
+}
+
+int devm_cxl_add_nvdimm(struct device *host, struct cxl_memdev *cxlmd)
+{
+	struct cxl_nvdimm *cxl_nvd;
+	struct device *dev;
+	int rc;
+
+	cxl_nvd = cxl_nvdimm_alloc(cxlmd);
+	if (IS_ERR(cxl_nvd))
+		return PTR_ERR(cxl_nvd);
+
+	dev = &cxl_nvd->dev;
+	rc = dev_set_name(dev, "pmem%d", cxlmd->id);
+	if (rc)
+		goto err;
+
+	rc = device_add(dev);
+	if (rc)
+		goto err;
+
+	dev_dbg(host, "%s: register %s\n", dev_name(dev->parent),
+		dev_name(dev));
+
+	return devm_add_action_or_reset(host, unregister_cxl_dev, dev);
+
+err:
+	put_device(dev);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(devm_cxl_add_nvdimm);
diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
new file mode 100644
index 000000000000..d05946a6bf53
--- /dev/null
+++ b/drivers/cxl/core/regs.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2020 Intel Corporation. */
+
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/pci.h>
+#include "core.h"
+
+/**
+ * cxl_probe_component_regs() - Detect CXL Component register blocks
+ * @dev: Host device of the @base mapping
+ * @base: Mapping containing the HDM Decoder Capability Header
+ * @map: Map object describing the register block information found
+ *
+ * See CXL 2.0 8.2.4 Component Register Layout and Definition
+ * See CXL 2.0 8.2.5.5 CXL Device Register Interface
+ *
+ * Probe for component register information and return it in map object.
+ */
+void cxl_probe_component_regs(struct device *dev, void __iomem *base,
+			      struct cxl_component_reg_map *map)
+{
+	int cap, cap_count;
+	u64 cap_array;
+
+	*map = (struct cxl_component_reg_map) { 0 };
+
+	/*
+	 * CXL.cache and CXL.mem registers are at offset 0x1000 as defined in
+	 * CXL 2.0 8.2.4 Table 141.
+	 */
+	base += CXL_CM_OFFSET;
+
+	cap_array = readq(base + CXL_CM_CAP_HDR_OFFSET);
+
+	if (FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, cap_array) != CM_CAP_HDR_CAP_ID) {
+		dev_err(dev,
+			"Couldn't locate the CXL.cache and CXL.mem capability array header./n");
+		return;
+	}
+
+	/* It's assumed that future versions will be backward compatible */
+	cap_count = FIELD_GET(CXL_CM_CAP_HDR_ARRAY_SIZE_MASK, cap_array);
+
+	for (cap = 1; cap <= cap_count; cap++) {
+		void __iomem *register_block;
+		u32 hdr;
+		int decoder_cnt;
+		u16 cap_id, offset;
+		u32 length;
+
+		hdr = readl(base + cap * 0x4);
+
+		cap_id = FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, hdr);
+		offset = FIELD_GET(CXL_CM_CAP_PTR_MASK, hdr);
+		register_block = base + offset;
+
+		switch (cap_id) {
+		case CXL_CM_CAP_CAP_ID_HDM:
+			dev_dbg(dev, "found HDM decoder capability (0x%x)\n",
+				offset);
+
+			hdr = readl(register_block);
+
+			decoder_cnt = cxl_hdm_decoder_count(hdr);
+			length = 0x20 * decoder_cnt + 0x10;
+
+			map->hdm_decoder.valid = true;
+			map->hdm_decoder.offset = CXL_CM_OFFSET + offset;
+			map->hdm_decoder.size = length;
+			break;
+		default:
+			dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,
+				offset);
+			break;
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(cxl_probe_component_regs);
+
+/**
+ * cxl_probe_device_regs() - Detect CXL Device register blocks
+ * @dev: Host device of the @base mapping
+ * @base: Mapping of CXL 2.0 8.2.8 CXL Device Register Interface
+ * @map: Map object describing the register block information found
+ *
+ * Probe for device register information and return it in map object.
+ */
+void cxl_probe_device_regs(struct device *dev, void __iomem *base,
+			   struct cxl_device_reg_map *map)
+{
+	int cap, cap_count;
+	u64 cap_array;
+
+	*map = (struct cxl_device_reg_map){ 0 };
+
+	cap_array = readq(base + CXLDEV_CAP_ARRAY_OFFSET);
+	if (FIELD_GET(CXLDEV_CAP_ARRAY_ID_MASK, cap_array) !=
+	    CXLDEV_CAP_ARRAY_CAP_ID)
+		return;
+
+	cap_count = FIELD_GET(CXLDEV_CAP_ARRAY_COUNT_MASK, cap_array);
+
+	for (cap = 1; cap <= cap_count; cap++) {
+		u32 offset, length;
+		u16 cap_id;
+
+		cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK,
+				   readl(base + cap * 0x10));
+		offset = readl(base + cap * 0x10 + 0x4);
+		length = readl(base + cap * 0x10 + 0x8);
+
+		switch (cap_id) {
+		case CXLDEV_CAP_CAP_ID_DEVICE_STATUS:
+			dev_dbg(dev, "found Status capability (0x%x)\n", offset);
+
+			map->status.valid = true;
+			map->status.offset = offset;
+			map->status.size = length;
+			break;
+		case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX:
+			dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset);
+			map->mbox.valid = true;
+			map->mbox.offset = offset;
+			map->mbox.size = length;
+			break;
+		case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX:
+			dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset);
+			break;
+		case CXLDEV_CAP_CAP_ID_MEMDEV:
+			dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset);
+			map->memdev.valid = true;
+			map->memdev.offset = offset;
+			map->memdev.size = length;
+			break;
+		default:
+			if (cap_id >= 0x8000)
+				dev_dbg(dev, "Vendor cap ID: %#x offset: %#x\n", cap_id, offset);
+			else
+				dev_dbg(dev, "Unknown cap ID: %#x offset: %#x\n", cap_id, offset);
+			break;
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(cxl_probe_device_regs);
+
+static void __iomem *devm_cxl_iomap_block(struct device *dev,
+					  resource_size_t addr,
+					  resource_size_t length)
+{
+	void __iomem *ret_val;
+	struct resource *res;
+
+	res = devm_request_mem_region(dev, addr, length, dev_name(dev));
+	if (!res) {
+		resource_size_t end = addr + length - 1;
+
+		dev_err(dev, "Failed to request region %pa-%pa\n", &addr, &end);
+		return NULL;
+	}
+
+	ret_val = devm_ioremap(dev, addr, length);
+	if (!ret_val)
+		dev_err(dev, "Failed to map region %pr\n", res);
+
+	return ret_val;
+}
+
+int cxl_map_component_regs(struct pci_dev *pdev,
+			   struct cxl_component_regs *regs,
+			   struct cxl_register_map *map)
+{
+	struct device *dev = &pdev->dev;
+	resource_size_t phys_addr;
+	resource_size_t length;
+
+	phys_addr = pci_resource_start(pdev, map->barno);
+	phys_addr += map->block_offset;
+
+	phys_addr += map->component_map.hdm_decoder.offset;
+	length = map->component_map.hdm_decoder.size;
+	regs->hdm_decoder = devm_cxl_iomap_block(dev, phys_addr, length);
+	if (!regs->hdm_decoder)
+		return -ENOMEM;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cxl_map_component_regs);
+
+int cxl_map_device_regs(struct pci_dev *pdev,
+			struct cxl_device_regs *regs,
+			struct cxl_register_map *map)
+{
+	struct device *dev = &pdev->dev;
+	resource_size_t phys_addr;
+
+	phys_addr = pci_resource_start(pdev, map->barno);
+	phys_addr += map->block_offset;
+
+	if (map->device_map.status.valid) {
+		resource_size_t addr;
+		resource_size_t length;
+
+		addr = phys_addr + map->device_map.status.offset;
+		length = map->device_map.status.size;
+		regs->status = devm_cxl_iomap_block(dev, addr, length);
+		if (!regs->status)
+			return -ENOMEM;
+	}
+
+	if (map->device_map.mbox.valid) {
+		resource_size_t addr;
+		resource_size_t length;
+
+		addr = phys_addr + map->device_map.mbox.offset;
+		length = map->device_map.mbox.size;
+		regs->mbox = devm_cxl_iomap_block(dev, addr, length);
+		if (!regs->mbox)
+			return -ENOMEM;
+	}
+
+	if (map->device_map.memdev.valid) {
+		resource_size_t addr;
+		resource_size_t length;
+
+		addr = phys_addr + map->device_map.memdev.offset;
+		length = map->device_map.memdev.size;
+		regs->memdev = devm_cxl_iomap_block(dev, addr, length);
+		if (!regs->memdev)
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cxl_map_device_regs);
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 04/23] cxl/mem: Move character device region creation
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (2 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 03/23] cxl/core: Extract register and pmem functionality Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 05/23] cxl: Pass fops and shutdown to memdev creation Ben Widawsky
                   ` (18 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

In order to merge the memdev specific functionality of cxl_pci into
core, the character device creation currently handled by cxl_pci need to
be moved. The rest of the changes are largely cut/paste, the actual
functional change is done here in preparation.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/core/bus.c | 12 ++++++++++++
 drivers/cxl/mem.h      |  2 ++
 drivers/cxl/pci.c      | 14 +-------------
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index 33196813ebcb..8c2351c52d2b 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -22,6 +22,8 @@
  * instantiated by the core.
  */
 
+int cxl_mem_major;
+EXPORT_SYMBOL_GPL(cxl_mem_major);
 static DEFINE_IDA(cxl_port_ida);
 
 static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
@@ -632,12 +634,22 @@ EXPORT_SYMBOL_GPL(cxl_bus_type);
 
 static __init int cxl_core_init(void)
 {
+	dev_t devt;
+	int rc;
+
+	rc = alloc_chrdev_region(&devt, 0, CXL_MEM_MAX_DEVS, "cxl");
+	if (rc)
+		return rc;
+
+	cxl_mem_major = MAJOR(devt);
+
 	return bus_register(&cxl_bus_type);
 }
 
 static void cxl_core_exit(void)
 {
 	bus_unregister(&cxl_bus_type);
+	unregister_chrdev_region(MKDEV(cxl_mem_major, 0), CXL_MEM_MAX_DEVS);
 }
 
 module_init(cxl_core_init);
diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 8f02d02b26b4..2092f86beeb8 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -34,6 +34,8 @@
  */
 #define CXL_MEM_MAX_DEVS 65536
 
+extern int cxl_mem_major;
+
 /**
  * struct cxl_memdev - CXL bus object representing a Type-3 Memory Device
  * @dev: driver core device object
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 4cf351a3cf99..0d9091379963 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -94,7 +94,6 @@ struct mbox_cmd {
 #define CXL_MBOX_SUCCESS 0
 };
 
-static int cxl_mem_major;
 static DEFINE_IDA(cxl_memdev_ida);
 static DECLARE_RWSEM(cxl_memdev_rwsem);
 static struct dentry *cxl_debugfs;
@@ -1640,25 +1639,15 @@ static struct pci_driver cxl_mem_driver = {
 static __init int cxl_mem_init(void)
 {
 	struct dentry *mbox_debugfs;
-	dev_t devt;
 	int rc;
 
 	/* Double check the anonymous union trickery in struct cxl_regs */
 	BUILD_BUG_ON(offsetof(struct cxl_regs, memdev) !=
 		     offsetof(struct cxl_regs, device_regs.memdev));
 
-	rc = alloc_chrdev_region(&devt, 0, CXL_MEM_MAX_DEVS, "cxl");
-	if (rc)
-		return rc;
-
-	cxl_mem_major = MAJOR(devt);
-
 	rc = pci_register_driver(&cxl_mem_driver);
-	if (rc) {
-		unregister_chrdev_region(MKDEV(cxl_mem_major, 0),
-					 CXL_MEM_MAX_DEVS);
+	if (rc)
 		return rc;
-	}
 
 	cxl_debugfs = debugfs_create_dir("cxl", NULL);
 	mbox_debugfs = debugfs_create_dir("mbox", cxl_debugfs);
@@ -1672,7 +1661,6 @@ static __exit void cxl_mem_exit(void)
 {
 	debugfs_remove_recursive(cxl_debugfs);
 	pci_unregister_driver(&cxl_mem_driver);
-	unregister_chrdev_region(MKDEV(cxl_mem_major, 0), CXL_MEM_MAX_DEVS);
 }
 
 MODULE_LICENSE("GPL v2");
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 05/23] cxl: Pass fops and shutdown to memdev creation
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (3 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 04/23] cxl/mem: Move character device region creation Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 06/23] cxl/core: Move memdev management to core Ben Widawsky
                   ` (17 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

Drivers that use cxl_core for registering a cxl_memdev will likely
require synchronization with the core for shutdown so as to not race
against the device going away. The main example currently is with the
ioctl interface. Through the duration of the ioctl it's expected that
the underlying memdev will not disappear.

Additionally, it may be desirable to have the fops be passed along as
well for drivers which do not want the standard handler for the
character device's ioctl.

As memdev is being migrated to core, this separation must be made.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/mem.h |  2 ++
 drivers/cxl/pci.c | 23 +++++++++++++++--------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 2092f86beeb8..2b7481376621 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -42,12 +42,14 @@ extern int cxl_mem_major;
  * @cdev: char dev core object for ioctl operations
  * @cxlm: pointer to the parent device driver data
  * @id: id number of this memdev instance.
+ * @shutdown: Optional function to call on memory device shutdown.
  */
 struct cxl_memdev {
 	struct device dev;
 	struct cdev cdev;
 	struct cxl_mem *cxlm;
 	int id;
+	void (*shutdown)(struct cxl_memdev *cxlmd);
 };
 
 /**
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 0d9091379963..e90b8fb6dc08 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1293,11 +1293,13 @@ static void cxl_memdev_unregister(void *_cxlmd)
 	struct device *dev = &cxlmd->dev;
 
 	cdev_device_del(&cxlmd->cdev, dev);
-	cxl_memdev_shutdown(cxlmd);
+	if (cxlmd->shutdown)
+		cxlmd->shutdown(cxlmd);
 	put_device(dev);
 }
 
-static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm)
+static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm,
+					   const struct file_operations *fops)
 {
 	struct pci_dev *pdev = cxlm->pdev;
 	struct cxl_memdev *cxlmd;
@@ -1323,7 +1325,7 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm)
 	device_set_pm_not_required(dev);
 
 	cdev = &cxlmd->cdev;
-	cdev_init(cdev, &cxl_memdev_fops);
+	cdev_init(cdev, fops);
 	return cxlmd;
 
 err:
@@ -1331,15 +1333,17 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm)
 	return ERR_PTR(rc);
 }
 
-static struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
-					      struct cxl_mem *cxlm)
+static struct cxl_memdev *
+devm_cxl_add_memdev(struct device *host, struct cxl_mem *cxlm,
+		    const struct file_operations *fops,
+		    void (*shutdown)(struct cxl_memdev *cxlmd))
 {
 	struct cxl_memdev *cxlmd;
 	struct device *dev;
 	struct cdev *cdev;
 	int rc;
 
-	cxlmd = cxl_memdev_alloc(cxlm);
+	cxlmd = cxl_memdev_alloc(cxlm, fops);
 	if (IS_ERR(cxlmd))
 		return cxlmd;
 
@@ -1362,6 +1366,7 @@ static struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
 	rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
 	if (rc)
 		return ERR_PTR(rc);
+	cxlmd->shutdown = shutdown;
 	return cxlmd;
 
 err:
@@ -1369,7 +1374,8 @@ static struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
 	 * The cdev was briefly live, shutdown any ioctl operations that
 	 * saw that state.
 	 */
-	cxl_memdev_shutdown(cxlmd);
+	if (shutdown)
+		shutdown(cxlmd);
 	put_device(dev);
 	return ERR_PTR(rc);
 }
@@ -1610,7 +1616,8 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
-	cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlm);
+	cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlm, &cxl_memdev_fops,
+				    cxl_memdev_shutdown);
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 06/23] cxl/core: Move memdev management to core
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (4 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 05/23] cxl: Pass fops and shutdown to memdev creation Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 07/23] cxl/pci: Ignore unknown register block types Ben Widawsky
                   ` (16 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

The CXL core must own memdev management so that CXL drivers may register
for services. At present, only cxl_pci cares about this functionality,
which is why the functionality was implemented there. The immediate need
for this is a memdev driver which will be needed to manage CXL regions.
Potentially, drivers for other types of devices may desire this
functionality as well - an accelerator wishing to present itself as a
memory device for example.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/Makefile      |   1 -
 drivers/cxl/core/Makefile |   2 +-
 drivers/cxl/core/memdev.c | 224 +++++++++++++++++++++++++++++++++++++
 drivers/cxl/mem.h         |   5 +
 drivers/cxl/pci.c         | 228 ++------------------------------------
 5 files changed, 237 insertions(+), 223 deletions(-)
 create mode 100644 drivers/cxl/core/memdev.c

diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index febb2c3f5fc6..d1aaabc940f3 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -4,7 +4,6 @@ obj-$(CONFIG_CXL_MEM) += cxl_pci.o
 obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
 obj-$(CONFIG_CXL_PMEM) += cxl_pmem.o
 
-ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL
 cxl_pci-y := pci.o
 cxl_acpi-y := acpi.o
 cxl_pmem-y := pmem.o
diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
index 289e8c8deebb..1503d8bf5e9a 100644
--- a/drivers/cxl/core/Makefile
+++ b/drivers/cxl/core/Makefile
@@ -2,4 +2,4 @@
 obj-$(CONFIG_CXL_BUS) += cxl_core.o
 
 ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL -I./drivers/cxl
-cxl_core-y := bus.o pmem.o regs.o
+cxl_core-y := bus.o memdev.o pmem.o regs.o
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
new file mode 100644
index 000000000000..dce43e03a05b
--- /dev/null
+++ b/drivers/cxl/core/memdev.c
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2020 Intel Corporation. */
+
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/idr.h>
+#include <linux/pci.h>
+#include "core.h"
+
+static DEFINE_IDA(cxl_memdev_ida);
+
+static struct cxl_memdev *to_cxl_memdev(struct device *dev)
+{
+	return container_of(dev, struct cxl_memdev, dev);
+}
+
+static void cxl_memdev_release(struct device *dev)
+{
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+
+	ida_free(&cxl_memdev_ida, cxlmd->id);
+	kfree(cxlmd);
+}
+
+static char *cxl_memdev_devnode(struct device *dev, umode_t *mode, kuid_t *uid,
+				kgid_t *gid)
+{
+	return kasprintf(GFP_KERNEL, "cxl/%s", dev_name(dev));
+}
+
+static ssize_t firmware_version_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_mem *cxlm = cxlmd->cxlm;
+
+	return sysfs_emit(buf, "%.16s\n", cxlm->firmware_version);
+}
+static DEVICE_ATTR_RO(firmware_version);
+
+static ssize_t payload_max_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_mem *cxlm = cxlmd->cxlm;
+
+	return sysfs_emit(buf, "%zu\n", cxlm->payload_size);
+}
+static DEVICE_ATTR_RO(payload_max);
+
+static ssize_t label_storage_size_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_mem *cxlm = cxlmd->cxlm;
+
+	return sysfs_emit(buf, "%zu\n", cxlm->lsa_size);
+}
+static DEVICE_ATTR_RO(label_storage_size);
+
+static ssize_t ram_size_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_mem *cxlm = cxlmd->cxlm;
+	unsigned long long len = range_len(&cxlm->ram_range);
+
+	return sysfs_emit(buf, "%#llx\n", len);
+}
+
+static struct device_attribute dev_attr_ram_size =
+	__ATTR(size, 0444, ram_size_show, NULL);
+
+static ssize_t pmem_size_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_mem *cxlm = cxlmd->cxlm;
+	unsigned long long len = range_len(&cxlm->pmem_range);
+
+	return sysfs_emit(buf, "%#llx\n", len);
+}
+
+static struct device_attribute dev_attr_pmem_size =
+	__ATTR(size, 0444, pmem_size_show, NULL);
+
+static struct attribute *cxl_memdev_attributes[] = {
+	&dev_attr_firmware_version.attr,
+	&dev_attr_payload_max.attr,
+	&dev_attr_label_storage_size.attr,
+	NULL,
+};
+
+static struct attribute *cxl_memdev_pmem_attributes[] = {
+	&dev_attr_pmem_size.attr,
+	NULL,
+};
+
+static struct attribute *cxl_memdev_ram_attributes[] = {
+	&dev_attr_ram_size.attr,
+	NULL,
+};
+
+static struct attribute_group cxl_memdev_attribute_group = {
+	.attrs = cxl_memdev_attributes,
+};
+
+static struct attribute_group cxl_memdev_ram_attribute_group = {
+	.name = "ram",
+	.attrs = cxl_memdev_ram_attributes,
+};
+
+static struct attribute_group cxl_memdev_pmem_attribute_group = {
+	.name = "pmem",
+	.attrs = cxl_memdev_pmem_attributes,
+};
+
+static const struct attribute_group *cxl_memdev_attribute_groups[] = {
+	&cxl_memdev_attribute_group,
+	&cxl_memdev_ram_attribute_group,
+	&cxl_memdev_pmem_attribute_group,
+	NULL,
+};
+
+static const struct device_type cxl_memdev_type = {
+	.name = "cxl_memdev",
+	.release = cxl_memdev_release,
+	.devnode = cxl_memdev_devnode,
+	.groups = cxl_memdev_attribute_groups,
+};
+
+static void cxl_memdev_unregister(void *_cxlmd)
+{
+	struct cxl_memdev *cxlmd = _cxlmd;
+	struct device *dev = &cxlmd->dev;
+
+	cdev_device_del(&cxlmd->cdev, dev);
+	if (cxlmd->shutdown)
+		cxlmd->shutdown(cxlmd);
+	put_device(dev);
+}
+
+static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm,
+					   const struct file_operations *fops)
+{
+	struct pci_dev *pdev = cxlm->pdev;
+	struct cxl_memdev *cxlmd;
+	struct device *dev;
+	struct cdev *cdev;
+	int rc;
+
+	cxlmd = kzalloc(sizeof(*cxlmd), GFP_KERNEL);
+	if (!cxlmd)
+		return ERR_PTR(-ENOMEM);
+
+	rc = ida_alloc_range(&cxl_memdev_ida, 0, CXL_MEM_MAX_DEVS, GFP_KERNEL);
+	if (rc < 0)
+		goto err;
+	cxlmd->id = rc;
+
+	dev = &cxlmd->dev;
+	device_initialize(dev);
+	dev->parent = &pdev->dev;
+	dev->bus = &cxl_bus_type;
+	dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
+	dev->type = &cxl_memdev_type;
+	device_set_pm_not_required(dev);
+
+	cdev = &cxlmd->cdev;
+	cdev_init(cdev, fops);
+	return cxlmd;
+
+err:
+	kfree(cxlmd);
+	return ERR_PTR(rc);
+}
+
+struct cxl_memdev *
+devm_cxl_add_memdev(struct device *host, struct cxl_mem *cxlm,
+		    const struct file_operations *fops,
+		    void (*shutdown)(struct cxl_memdev *cxlmd))
+{
+	struct cxl_memdev *cxlmd;
+	struct device *dev;
+	struct cdev *cdev;
+	int rc;
+
+	cxlmd = cxl_memdev_alloc(cxlm, fops);
+	if (IS_ERR(cxlmd))
+		return cxlmd;
+
+	dev = &cxlmd->dev;
+	rc = dev_set_name(dev, "mem%d", cxlmd->id);
+	if (rc)
+		goto err;
+
+	/*
+	 * Activate ioctl operations, no cxl_memdev_rwsem manipulation
+	 * needed as this is ordered with cdev_add() publishing the device.
+	 */
+	cxlmd->cxlm = cxlm;
+
+	cdev = &cxlmd->cdev;
+	rc = cdev_device_add(cdev, dev);
+	if (rc)
+		goto err;
+
+	rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
+	if (rc)
+		return ERR_PTR(rc);
+	cxlmd->shutdown = shutdown;
+	return cxlmd;
+
+err:
+	/*
+	 * The cdev was briefly live, shutdown any ioctl operations that
+	 * saw that state.
+	 */
+	if (shutdown)
+		shutdown(cxlmd);
+	put_device(dev);
+	return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_GPL(devm_cxl_add_memdev);
diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 2b7481376621..3730e3509ab6 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -52,6 +52,11 @@ struct cxl_memdev {
 	void (*shutdown)(struct cxl_memdev *cxlmd);
 };
 
+struct cxl_memdev *
+devm_cxl_add_memdev(struct device *host, struct cxl_mem *cxlm,
+		    const struct file_operations *fops,
+		    void (*shutdown)(struct cxl_memdev *cxlmd));
+
 /**
  * struct cxl_mem - A CXL memory device
  * @pdev: The PCI device associated with this CXL device.
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index e90b8fb6dc08..d7da18ebba81 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -94,7 +94,6 @@ struct mbox_cmd {
 #define CXL_MBOX_SUCCESS 0
 };
 
-static DEFINE_IDA(cxl_memdev_ida);
 static DECLARE_RWSEM(cxl_memdev_rwsem);
 static struct dentry *cxl_debugfs;
 static bool cxl_raw_allow_all;
@@ -1160,226 +1159,6 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 	return ret;
 }
 
-static struct cxl_memdev *to_cxl_memdev(struct device *dev)
-{
-	return container_of(dev, struct cxl_memdev, dev);
-}
-
-static void cxl_memdev_release(struct device *dev)
-{
-	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
-
-	ida_free(&cxl_memdev_ida, cxlmd->id);
-	kfree(cxlmd);
-}
-
-static char *cxl_memdev_devnode(struct device *dev, umode_t *mode, kuid_t *uid,
-				kgid_t *gid)
-{
-	return kasprintf(GFP_KERNEL, "cxl/%s", dev_name(dev));
-}
-
-static ssize_t firmware_version_show(struct device *dev,
-				     struct device_attribute *attr, char *buf)
-{
-	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
-	struct cxl_mem *cxlm = cxlmd->cxlm;
-
-	return sysfs_emit(buf, "%.16s\n", cxlm->firmware_version);
-}
-static DEVICE_ATTR_RO(firmware_version);
-
-static ssize_t payload_max_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
-	struct cxl_mem *cxlm = cxlmd->cxlm;
-
-	return sysfs_emit(buf, "%zu\n", cxlm->payload_size);
-}
-static DEVICE_ATTR_RO(payload_max);
-
-static ssize_t label_storage_size_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
-	struct cxl_mem *cxlm = cxlmd->cxlm;
-
-	return sysfs_emit(buf, "%zu\n", cxlm->lsa_size);
-}
-static DEVICE_ATTR_RO(label_storage_size);
-
-static ssize_t ram_size_show(struct device *dev, struct device_attribute *attr,
-			     char *buf)
-{
-	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
-	struct cxl_mem *cxlm = cxlmd->cxlm;
-	unsigned long long len = range_len(&cxlm->ram_range);
-
-	return sysfs_emit(buf, "%#llx\n", len);
-}
-
-static struct device_attribute dev_attr_ram_size =
-	__ATTR(size, 0444, ram_size_show, NULL);
-
-static ssize_t pmem_size_show(struct device *dev, struct device_attribute *attr,
-			      char *buf)
-{
-	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
-	struct cxl_mem *cxlm = cxlmd->cxlm;
-	unsigned long long len = range_len(&cxlm->pmem_range);
-
-	return sysfs_emit(buf, "%#llx\n", len);
-}
-
-static struct device_attribute dev_attr_pmem_size =
-	__ATTR(size, 0444, pmem_size_show, NULL);
-
-static struct attribute *cxl_memdev_attributes[] = {
-	&dev_attr_firmware_version.attr,
-	&dev_attr_payload_max.attr,
-	&dev_attr_label_storage_size.attr,
-	NULL,
-};
-
-static struct attribute *cxl_memdev_pmem_attributes[] = {
-	&dev_attr_pmem_size.attr,
-	NULL,
-};
-
-static struct attribute *cxl_memdev_ram_attributes[] = {
-	&dev_attr_ram_size.attr,
-	NULL,
-};
-
-static struct attribute_group cxl_memdev_attribute_group = {
-	.attrs = cxl_memdev_attributes,
-};
-
-static struct attribute_group cxl_memdev_ram_attribute_group = {
-	.name = "ram",
-	.attrs = cxl_memdev_ram_attributes,
-};
-
-static struct attribute_group cxl_memdev_pmem_attribute_group = {
-	.name = "pmem",
-	.attrs = cxl_memdev_pmem_attributes,
-};
-
-static const struct attribute_group *cxl_memdev_attribute_groups[] = {
-	&cxl_memdev_attribute_group,
-	&cxl_memdev_ram_attribute_group,
-	&cxl_memdev_pmem_attribute_group,
-	NULL,
-};
-
-static const struct device_type cxl_memdev_type = {
-	.name = "cxl_memdev",
-	.release = cxl_memdev_release,
-	.devnode = cxl_memdev_devnode,
-	.groups = cxl_memdev_attribute_groups,
-};
-
-static void cxl_memdev_shutdown(struct cxl_memdev *cxlmd)
-{
-	down_write(&cxl_memdev_rwsem);
-	cxlmd->cxlm = NULL;
-	up_write(&cxl_memdev_rwsem);
-}
-
-static void cxl_memdev_unregister(void *_cxlmd)
-{
-	struct cxl_memdev *cxlmd = _cxlmd;
-	struct device *dev = &cxlmd->dev;
-
-	cdev_device_del(&cxlmd->cdev, dev);
-	if (cxlmd->shutdown)
-		cxlmd->shutdown(cxlmd);
-	put_device(dev);
-}
-
-static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm,
-					   const struct file_operations *fops)
-{
-	struct pci_dev *pdev = cxlm->pdev;
-	struct cxl_memdev *cxlmd;
-	struct device *dev;
-	struct cdev *cdev;
-	int rc;
-
-	cxlmd = kzalloc(sizeof(*cxlmd), GFP_KERNEL);
-	if (!cxlmd)
-		return ERR_PTR(-ENOMEM);
-
-	rc = ida_alloc_range(&cxl_memdev_ida, 0, CXL_MEM_MAX_DEVS, GFP_KERNEL);
-	if (rc < 0)
-		goto err;
-	cxlmd->id = rc;
-
-	dev = &cxlmd->dev;
-	device_initialize(dev);
-	dev->parent = &pdev->dev;
-	dev->bus = &cxl_bus_type;
-	dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
-	dev->type = &cxl_memdev_type;
-	device_set_pm_not_required(dev);
-
-	cdev = &cxlmd->cdev;
-	cdev_init(cdev, fops);
-	return cxlmd;
-
-err:
-	kfree(cxlmd);
-	return ERR_PTR(rc);
-}
-
-static struct cxl_memdev *
-devm_cxl_add_memdev(struct device *host, struct cxl_mem *cxlm,
-		    const struct file_operations *fops,
-		    void (*shutdown)(struct cxl_memdev *cxlmd))
-{
-	struct cxl_memdev *cxlmd;
-	struct device *dev;
-	struct cdev *cdev;
-	int rc;
-
-	cxlmd = cxl_memdev_alloc(cxlm, fops);
-	if (IS_ERR(cxlmd))
-		return cxlmd;
-
-	dev = &cxlmd->dev;
-	rc = dev_set_name(dev, "mem%d", cxlmd->id);
-	if (rc)
-		goto err;
-
-	/*
-	 * Activate ioctl operations, no cxl_memdev_rwsem manipulation
-	 * needed as this is ordered with cdev_add() publishing the device.
-	 */
-	cxlmd->cxlm = cxlm;
-
-	cdev = &cxlmd->cdev;
-	rc = cdev_device_add(cdev, dev);
-	if (rc)
-		goto err;
-
-	rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
-	if (rc)
-		return ERR_PTR(rc);
-	cxlmd->shutdown = shutdown;
-	return cxlmd;
-
-err:
-	/*
-	 * The cdev was briefly live, shutdown any ioctl operations that
-	 * saw that state.
-	 */
-	if (shutdown)
-		shutdown(cxlmd);
-	put_device(dev);
-	return ERR_PTR(rc);
-}
-
 static int cxl_xfer_log(struct cxl_mem *cxlm, uuid_t *uuid, u32 size, u8 *out)
 {
 	u32 remaining = size;
@@ -1586,6 +1365,13 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
 	return 0;
 }
 
+static void cxl_memdev_shutdown(struct cxl_memdev *cxlmd)
+{
+	down_write(&cxl_memdev_rwsem);
+	cxlmd->cxlm = NULL;
+	up_write(&cxl_memdev_rwsem);
+}
+
 static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct cxl_memdev *cxlmd;
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 07/23] cxl/pci: Ignore unknown register block types
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (5 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 06/23] cxl/core: Move memdev management to core Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 08/23] cxl/pci: Simplify register setup Ben Widawsky
                   ` (15 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

In an effort to explicit avoid supporting vendor specific register
blocks (which can happily be mapped from userspace), entirely skip
probing unknown types. The secondary benefit of this will be revealed
in the future with code simplification.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/pci.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index d7da18ebba81..dd0ac89fbdf4 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1106,14 +1106,6 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		u64 offset;
 		u8 bar;
 
-		map = kzalloc(sizeof(*map), GFP_KERNEL);
-		if (!map) {
-			ret = -ENOMEM;
-			goto free_maps;
-		}
-
-		list_add(&map->list, &register_maps);
-
 		pci_read_config_dword(pdev, regloc, &reg_lo);
 		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
 
@@ -1123,6 +1115,18 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		dev_dbg(dev, "Found register block in bar %u @ 0x%llx of type %u\n",
 			bar, offset, reg_type);
 
+		/* Ignore unknown register block types */
+		if (reg_type > CXL_REGLOC_RBI_MEMDEV)
+			continue;
+
+		map = kzalloc(sizeof(*map), GFP_KERNEL);
+		if (!map) {
+			ret = -ENOMEM;
+			goto free_maps;
+		}
+
+		list_add(&map->list, &register_maps);
+
 		base = cxl_mem_map_regblock(cxlm, bar, offset);
 		if (!base) {
 			ret = -ENOMEM;
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 08/23] cxl/pci: Simplify register setup
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (6 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 07/23] cxl/pci: Ignore unknown register block types Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 09/23] cxl/pci: Retain map information in cxl_mem_probe Ben Widawsky
                   ` (14 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

It is desirable to retain the mappings from the calling function. By
simplifying this code, it will be much more straightforward to do that.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/cxl.h |  1 -
 drivers/cxl/pci.c | 38 ++++++++++++--------------------------
 drivers/cxl/pci.h |  1 +
 3 files changed, 13 insertions(+), 27 deletions(-)

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index b6bda39a59e3..53927f9fa77e 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -140,7 +140,6 @@ struct cxl_device_reg_map {
 };
 
 struct cxl_register_map {
-	struct list_head list;
 	u64 block_offset;
 	u8 reg_type;
 	u8 barno;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index dd0ac89fbdf4..8be18daa1420 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1079,9 +1079,8 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 	struct device *dev = &pdev->dev;
 	u32 regloc_size, regblocks;
 	void __iomem *base;
-	int regloc, i;
-	struct cxl_register_map *map, *n;
-	LIST_HEAD(register_maps);
+	int regloc, i, n_maps;
+	struct cxl_register_map *map, maps[CXL_REGLOC_RBI_TYPES];
 	int ret = 0;
 
 	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID);
@@ -1100,7 +1099,7 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 	regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET;
 	regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8;
 
-	for (i = 0; i < regblocks; i++, regloc += 8) {
+	for (i = 0, n_maps = 0; i < regblocks; i++, regloc += 8) {
 		u32 reg_lo, reg_hi;
 		u8 reg_type;
 		u64 offset;
@@ -1119,20 +1118,11 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		if (reg_type > CXL_REGLOC_RBI_MEMDEV)
 			continue;
 
-		map = kzalloc(sizeof(*map), GFP_KERNEL);
-		if (!map) {
-			ret = -ENOMEM;
-			goto free_maps;
-		}
-
-		list_add(&map->list, &register_maps);
-
 		base = cxl_mem_map_regblock(cxlm, bar, offset);
-		if (!base) {
-			ret = -ENOMEM;
-			goto free_maps;
-		}
+		if (!base)
+			return -ENOMEM;
 
+		map = &maps[n_maps];
 		map->barno = bar;
 		map->block_offset = offset;
 		map->reg_type = reg_type;
@@ -1143,21 +1133,17 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		cxl_mem_unmap_regblock(cxlm, base);
 
 		if (ret)
-			goto free_maps;
+			return ret;
+
+		n_maps++;
 	}
 
 	pci_release_mem_regions(pdev);
 
-	list_for_each_entry(map, &register_maps, list) {
-		ret = cxl_map_regs(cxlm, map);
+	for (i = 0; i < n_maps; i++) {
+		ret = cxl_map_regs(cxlm, &maps[i]);
 		if (ret)
-			goto free_maps;
-	}
-
-free_maps:
-	list_for_each_entry_safe(map, n, &register_maps, list) {
-		list_del(&map->list);
-		kfree(map);
+			break;
 	}
 
 	return ret;
diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
index dad7a831f65f..8c1a58813816 100644
--- a/drivers/cxl/pci.h
+++ b/drivers/cxl/pci.h
@@ -25,6 +25,7 @@
 #define CXL_REGLOC_RBI_COMPONENT 1
 #define CXL_REGLOC_RBI_VIRT 2
 #define CXL_REGLOC_RBI_MEMDEV 3
+#define CXL_REGLOC_RBI_TYPES CXL_REGLOC_RBI_MEMDEV + 1
 
 #define CXL_REGLOC_ADDR_MASK GENMASK(31, 16)
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 09/23] cxl/pci: Retain map information in cxl_mem_probe
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (7 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 08/23] cxl/pci: Simplify register setup Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 10/23] cxl/decoder: Support parentless decoders Ben Widawsky
                   ` (13 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

In order for a memdev to participate in cxl_core's port APIs, the
physical address of the memdev's component registers is needed. This is
accomplished by allocating the array of maps in probe so they can be
used after the memdev is created.

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

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 8be18daa1420..f924a8c5a831 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1066,21 +1066,22 @@ static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
 /**
  * cxl_mem_setup_regs() - Setup necessary MMIO.
  * @cxlm: The CXL memory device to communicate with.
+ * @maps: Array of maps populated by this function.
  *
- * Return: 0 if all necessary registers mapped.
+ * Return: 0 if all necessary registers mapped. The results are stored in @maps.
  *
  * A memory device is required by spec to implement a certain set of MMIO
  * regions. The purpose of this function is to enumerate and map those
  * registers.
  */
-static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
+static int cxl_mem_setup_regs(struct cxl_mem *cxlm, struct cxl_register_map maps[])
 {
 	struct pci_dev *pdev = cxlm->pdev;
 	struct device *dev = &pdev->dev;
 	u32 regloc_size, regblocks;
 	void __iomem *base;
 	int regloc, i, n_maps;
-	struct cxl_register_map *map, maps[CXL_REGLOC_RBI_TYPES];
+	struct cxl_register_map *map;
 	int ret = 0;
 
 	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID);
@@ -1364,6 +1365,7 @@ static void cxl_memdev_shutdown(struct cxl_memdev *cxlmd)
 
 static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
+	struct cxl_register_map maps[CXL_REGLOC_RBI_TYPES];
 	struct cxl_memdev *cxlmd;
 	struct cxl_mem *cxlm;
 	int rc;
@@ -1376,7 +1378,7 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (IS_ERR(cxlm))
 		return PTR_ERR(cxlm);
 
-	rc = cxl_mem_setup_regs(cxlm);
+	rc = cxl_mem_setup_regs(cxlm, maps);
 	if (rc)
 		return rc;
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 10/23] cxl/decoder: Support parentless decoders
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (8 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 09/23] cxl/pci: Retain map information in cxl_mem_probe Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-30 21:03   ` Dan Williams
  2021-07-23 21:06 ` [PATCH 11/23] cxl: Enable an endpoint decoder type Ben Widawsky
                   ` (12 subsequent siblings)
  22 siblings, 1 reply; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

Currently, an ACPI0017 device (opaque platform thing) is parent to an
ACPI0016 device (platform host bridge) child. The platform level
decoders don't need a parent child relationship in order to traverse CXL
hierarchy since once you get to these devices, you have a useless ACPI
device instead of a CXL one.

To support an upcoming expansion for CXL endpoints to be able to
enumerate their decoders, support this NULL parent as a way to help
distinguish decoder types.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/acpi.c     |  9 ++++-----
 drivers/cxl/core/bus.c | 44 +++++++++++++++++++++++++++---------------
 drivers/cxl/cxl.h      |  4 ++--
 3 files changed, 34 insertions(+), 23 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 8ae89273f58e..fee56688d797 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -68,8 +68,7 @@ static int cxl_acpi_cfmws_verify(struct device *dev,
 	return 0;
 }
 
-static void cxl_add_cfmws_decoders(struct device *dev,
-				   struct cxl_port *root_port)
+static void cxl_add_cfmws_decoders(struct device *dev)
 {
 	struct acpi_cedt_cfmws *cfmws;
 	struct cxl_decoder *cxld;
@@ -109,7 +108,7 @@ static void cxl_add_cfmws_decoders(struct device *dev,
 		}
 
 		flags = cfmws_to_decoder_flags(cfmws->restrictions);
-		cxld = devm_cxl_add_decoder(dev, root_port,
+		cxld = devm_cxl_add_decoder(dev, NULL,
 					    CFMWS_INTERLEAVE_WAYS(cfmws),
 					    cfmws->base_hpa, cfmws->window_size,
 					    CFMWS_INTERLEAVE_WAYS(cfmws),
@@ -301,7 +300,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 	 * in the CHBCR and a 1:1 passthrough decode is implied.
 	 */
 	if (ctx.count == 1) {
-		cxld = devm_cxl_add_passthrough_decoder(host, port);
+		cxld = devm_cxl_add_passthrough_decoder(host);
 		if (IS_ERR(cxld))
 			return PTR_ERR(cxld);
 
@@ -393,7 +392,7 @@ static int cxl_acpi_probe(struct platform_device *pdev)
 	if (rc)
 		goto out;
 
-	cxl_add_cfmws_decoders(host, root_port);
+	cxl_add_cfmws_decoders(host);
 
 	/*
 	 * Root level scanned with host-bridge as dports, now scan host-bridges
diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index 8c2351c52d2b..7c75ae7f3b8e 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -25,6 +25,7 @@
 int cxl_mem_major;
 EXPORT_SYMBOL_GPL(cxl_mem_major);
 static DEFINE_IDA(cxl_port_ida);
+static DEFINE_IDA(cxl_root_decoder_ida);
 
 static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
@@ -178,9 +179,13 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = {
 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);
+	struct cxl_port *port = dev->parent ? to_cxl_port(dev->parent) : NULL;
+
+	if (port)
+		ida_free(&port->decoder_ida, cxld->id);
+	else
+		ida_free(&cxl_root_decoder_ida, cxld->id);
 
-	ida_free(&port->decoder_ida, cxld->id);
 	kfree(cxld);
 }
 
@@ -466,18 +471,24 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
 	if (interleave_ways < 1)
 		return ERR_PTR(-EINVAL);
 
-	device_lock(&port->dev);
-	if (list_empty(&port->dports))
-		rc = -EINVAL;
-	device_unlock(&port->dev);
-	if (rc)
-		return ERR_PTR(rc);
+	if (port) {
+		device_lock(&port->dev);
+		if (list_empty(&port->dports))
+			rc = -EINVAL;
+		device_unlock(&port->dev);
+		if (rc)
+			return ERR_PTR(rc);
+	}
 
 	cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
 	if (!cxld)
 		return ERR_PTR(-ENOMEM);
 
-	rc = ida_alloc(&port->decoder_ida, GFP_KERNEL);
+	if (port)
+		rc = ida_alloc(&port->decoder_ida, GFP_KERNEL);
+	else
+		rc = ida_alloc(&cxl_root_decoder_ida, GFP_KERNEL);
+
 	if (rc < 0)
 		goto err;
 
@@ -494,17 +505,18 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
 	};
 
 	/* handle implied target_list */
-	if (interleave_ways == 1)
-		cxld->target[0] =
-			list_first_entry(&port->dports, struct cxl_dport, list);
+	if (port)
+		if (interleave_ways == 1)
+			cxld->target[0] =
+				list_first_entry(&port->dports, struct cxl_dport, list);
 	dev = &cxld->dev;
 	device_initialize(dev);
 	device_set_pm_not_required(dev);
-	dev->parent = &port->dev;
+	dev->parent = port ? &port->dev : NULL;
 	dev->bus = &cxl_bus_type;
 
-	/* root ports do not have a cxl_port_type parent */
-	if (port->dev.parent->type == &cxl_port_type)
+	/* platform decoders don't have a parent */
+	if (port)
 		dev->type = &cxl_decoder_switch_type;
 	else
 		dev->type = &cxl_decoder_root_type;
@@ -531,7 +543,7 @@ devm_cxl_add_decoder(struct device *host, struct cxl_port *port, int nr_targets,
 		return cxld;
 
 	dev = &cxld->dev;
-	rc = dev_set_name(dev, "decoder%d.%d", port->id, cxld->id);
+	rc = dev_set_name(dev, "decoder%d.%d", port ? port->id : 0, cxld->id);
 	if (rc)
 		goto err;
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 53927f9fa77e..b9302d3861f0 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -285,9 +285,9 @@ devm_cxl_add_decoder(struct device *host, struct cxl_port *port, int nr_targets,
  * 0-length until the first CXL region is activated.
  */
 static inline struct cxl_decoder *
-devm_cxl_add_passthrough_decoder(struct device *host, struct cxl_port *port)
+devm_cxl_add_passthrough_decoder(struct device *host)
 {
-	return devm_cxl_add_decoder(host, port, 1, 0, 0, 1, PAGE_SIZE,
+	return devm_cxl_add_decoder(host, NULL, 1, 0, 0, 1, PAGE_SIZE,
 				    CXL_DECODER_EXPANDER, 0);
 }
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 11/23] cxl: Enable an endpoint decoder type
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (9 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 10/23] cxl/decoder: Support parentless decoders Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 12/23] cxl/region: Add region creation ABI Ben Widawsky
                   ` (11 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

Endpoints have decoders too (no dports)

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/acpi.c     | 14 +++++------
 drivers/cxl/core/bus.c | 55 ++++++++++++++++++++++++++++++++++++++----
 drivers/cxl/cxl.h      | 28 +++++++++++++--------
 3 files changed, 75 insertions(+), 22 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index fee56688d797..fd1ae2495ab0 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -108,13 +108,13 @@ static void cxl_add_cfmws_decoders(struct device *dev)
 		}
 
 		flags = cfmws_to_decoder_flags(cfmws->restrictions);
-		cxld = devm_cxl_add_decoder(dev, NULL,
-					    CFMWS_INTERLEAVE_WAYS(cfmws),
-					    cfmws->base_hpa, cfmws->window_size,
-					    CFMWS_INTERLEAVE_WAYS(cfmws),
-					    CFMWS_INTERLEAVE_GRANULARITY(cfmws),
-					    CXL_DECODER_EXPANDER,
-					    flags);
+		cxld = devm_cxl_add_platform_decoder(dev,
+						     CFMWS_INTERLEAVE_WAYS(cfmws),
+						     cfmws->base_hpa,
+						     cfmws->window_size,
+						     CFMWS_INTERLEAVE_WAYS(cfmws),
+						     CFMWS_INTERLEAVE_GRANULARITY(cfmws),
+						     flags);
 
 		if (IS_ERR(cxld)) {
 			dev_err(dev, "Failed to add decoder for %#llx-%#llx\n",
diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index 7c75ae7f3b8e..e2166f43aefc 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -84,6 +84,8 @@ static ssize_t target_type_show(struct device *dev,
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
 
 	switch (cxld->target_type) {
+	case CXL_DECODER_UNKNOWN:
+		return sysfs_emit(buf, "unknown\n");
 	case CXL_DECODER_ACCELERATOR:
 		return sysfs_emit(buf, "accelerator\n");
 	case CXL_DECODER_EXPANDER:
@@ -176,6 +178,12 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = {
 	NULL,
 };
 
+static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = {
+	&cxl_decoder_base_attribute_group,
+	&cxl_base_attribute_group,
+	NULL,
+};
+
 static void cxl_decoder_release(struct device *dev)
 {
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
@@ -189,6 +197,12 @@ static void cxl_decoder_release(struct device *dev)
 	kfree(cxld);
 }
 
+static const struct device_type cxl_decoder_endpoint_type = {
+	.name = "cxl_decoder_endpoint",
+	.release = cxl_decoder_release,
+	.groups = cxl_decoder_endpoint_attribute_groups,
+};
+
 static const struct device_type cxl_decoder_switch_type = {
 	.name = "cxl_decoder_switch",
 	.release = cxl_decoder_release,
@@ -516,10 +530,12 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
 	dev->bus = &cxl_bus_type;
 
 	/* platform decoders don't have a parent */
-	if (port)
-		dev->type = &cxl_decoder_switch_type;
-	else
+	if (!port)
 		dev->type = &cxl_decoder_root_type;
+	else if (flags & CXL_DECODER_F_ENDPOINT)
+		dev->type = &cxl_decoder_endpoint_type;
+	else
+		dev->type = &cxl_decoder_switch_type;
 
 	return cxld;
 err:
@@ -527,7 +543,7 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
 	return ERR_PTR(rc);
 }
 
-struct cxl_decoder *
+static struct cxl_decoder *
 devm_cxl_add_decoder(struct device *host, struct cxl_port *port, int nr_targets,
 		     resource_size_t base, resource_size_t len,
 		     int interleave_ways, int interleave_granularity,
@@ -560,7 +576,36 @@ devm_cxl_add_decoder(struct device *host, struct cxl_port *port, int nr_targets,
 	put_device(dev);
 	return ERR_PTR(rc);
 }
-EXPORT_SYMBOL_GPL(devm_cxl_add_decoder);
+
+struct cxl_decoder *
+devm_cxl_add_platform_decoder(struct device *host, int nr_targets,
+			      resource_size_t base, resource_size_t len,
+			      int interleave_ways, int interleave_granularity,
+			      unsigned long flags)
+{
+	return devm_cxl_add_decoder(host, NULL, nr_targets, base, len,
+				    interleave_ways, interleave_granularity, 0,
+				    flags);
+}
+EXPORT_SYMBOL_GPL(devm_cxl_add_platform_decoder);
+
+struct cxl_decoder *devm_cxl_add_switch_decoder(struct device *host,
+						struct cxl_port *port,
+						enum cxl_decoder_type type)
+{
+	return devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0,
+				    CXL_DECODER_UNKNOWN, 0);
+}
+EXPORT_SYMBOL_GPL(devm_cxl_add_switch_decoder);
+
+struct cxl_decoder *devm_cxl_add_endpoint_decoder(struct device *host,
+						  struct cxl_port *port,
+						  enum cxl_decoder_type type)
+{
+	return devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0, type,
+				    CXL_DECODER_F_ENDPOINT);
+}
+EXPORT_SYMBOL_GPL(devm_cxl_add_endpoint_decoder);
 
 /**
  * __cxl_driver_register - register a driver for the cxl bus
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index b9302d3861f0..dcf2d1a59271 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -173,11 +173,13 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 #define CXL_DECODER_F_TYPE2 BIT(2)
 #define CXL_DECODER_F_TYPE3 BIT(3)
 #define CXL_DECODER_F_LOCK  BIT(4)
-#define CXL_DECODER_F_MASK  GENMASK(4, 0)
+#define CXL_DECODER_F_ENDPOINT BIT(5)
+#define CXL_DECODER_F_MASK GENMASK(5, 0)
 
 enum cxl_decoder_type {
-       CXL_DECODER_ACCELERATOR = 2,
-       CXL_DECODER_EXPANDER = 3,
+	CXL_DECODER_UNKNOWN = 0,
+	CXL_DECODER_ACCELERATOR = 2,
+	CXL_DECODER_EXPANDER = 3,
 };
 
 /**
@@ -271,12 +273,19 @@ int cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
 
 struct cxl_decoder *to_cxl_decoder(struct device *dev);
 bool is_root_decoder(struct device *dev);
-struct cxl_decoder *
-devm_cxl_add_decoder(struct device *host, struct cxl_port *port, int nr_targets,
-		     resource_size_t base, resource_size_t len,
-		     int interleave_ways, int interleave_granularity,
-		     enum cxl_decoder_type type, unsigned long flags);
 
+struct cxl_decoder *
+devm_cxl_add_platform_decoder(struct device *host, int nr_targets,
+			      resource_size_t base, resource_size_t len,
+			      int interleave_ways, int interleave_granularity,
+			      unsigned long flags);
+
+struct cxl_decoder *devm_cxl_add_switch_decoder(struct device *host,
+						struct cxl_port *port,
+						enum cxl_decoder_type type);
+struct cxl_decoder *devm_cxl_add_endpoint_decoder(struct device *host,
+						  struct cxl_port *port,
+						  enum cxl_decoder_type type);
 /*
  * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability Structure)
  * single ported host-bridges need not publish a decoder capability when a
@@ -287,8 +296,7 @@ devm_cxl_add_decoder(struct device *host, struct cxl_port *port, int nr_targets,
 static inline struct cxl_decoder *
 devm_cxl_add_passthrough_decoder(struct device *host)
 {
-	return devm_cxl_add_decoder(host, NULL, 1, 0, 0, 1, PAGE_SIZE,
-				    CXL_DECODER_EXPANDER, 0);
+	return devm_cxl_add_platform_decoder(host, 1, 0, 0, 1, PAGE_SIZE, 0);
 }
 
 extern struct bus_type cxl_bus_type;
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 12/23] cxl/region: Add region creation ABI
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (10 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 11/23] cxl: Enable an endpoint decoder type Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-08-14  2:19   ` Dan Williams
  2021-07-23 21:06 ` [PATCH 13/23] cxl/region: Introduce concept of region configuration Ben Widawsky
                   ` (10 subsequent siblings)
  22 siblings, 1 reply; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

Regions are created as a child of the decoder that encompasses an
address space with constraints. Regions only exist for persistent
capacities.

When regions are created, the number of desired interleave ways must be
known. To enable this, the sysfs attribute will take the desired ways as
input. This interface intentionally allows creation of
impossible-to-enable regions based on interleave constraints in the
topology. The reasoning is to create new regions through the kernel
interfaces which may become possible on reboot under a variety of
circumstances.

As an example, creating a x1 region with:
echo 1 > /sys/bus/cxl/devices/decoder0.0/create_region

Will yield /sys/bus/cxl/devices/decoder0.0/region0.0:0

That region may then be deleted with:
echo region0.0:0 > /sys/bus/cxl/devices/decoder0.0/delete_region

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-cxl       |  21 +++
 .../driver-api/cxl/memory-devices.rst         |  11 ++
 drivers/cxl/Makefile                          |   1 +
 drivers/cxl/core/Makefile                     |   2 +-
 drivers/cxl/core/bus.c                        |  70 ++++++++
 drivers/cxl/core/core.h                       |   1 +
 drivers/cxl/core/region.c                     | 158 ++++++++++++++++++
 drivers/cxl/cxl.h                             |  14 ++
 drivers/cxl/region.h                          |  30 ++++
 9 files changed, 307 insertions(+), 1 deletion(-)
 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 0b6a2e6e8fbb..115a25d2899d 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -127,3 +127,24 @@ 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:		June, 2021
+KernelVersion:	v5.14
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		Creates a new CXL region of N interleaved ways. Writing a value
+		of '2' will create a new uninitialized region with 2x interleave
+		that will be mapped by the CXL decoderX.Y. Reading from this
+		node will return the last created region. 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:		June, 2021
+KernelVersion:	v5.14
+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".
diff --git a/Documentation/driver-api/cxl/memory-devices.rst b/Documentation/driver-api/cxl/memory-devices.rst
index 46847d8c70a0..96a1f8be7940 100644
--- a/Documentation/driver-api/cxl/memory-devices.rst
+++ b/Documentation/driver-api/cxl/memory-devices.rst
@@ -45,6 +45,17 @@ CXL Core
 .. kernel-doc:: drivers/cxl/core/regs.c
    :internal:
 
+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/Makefile b/drivers/cxl/Makefile
index d1aaabc940f3..d19d22a19966 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_CXL_MEM) += cxl_pci.o
 obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
 obj-$(CONFIG_CXL_PMEM) += cxl_pmem.o
 
+cxl_acpi-y := acpi.o
 cxl_pci-y := pci.o
 cxl_acpi-y := acpi.o
 cxl_pmem-y := pmem.o
diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
index 1503d8bf5e9a..4d034900e22c 100644
--- a/drivers/cxl/core/Makefile
+++ b/drivers/cxl/core/Makefile
@@ -2,4 +2,4 @@
 obj-$(CONFIG_CXL_BUS) += cxl_core.o
 
 ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL -I./drivers/cxl
-cxl_core-y := bus.o memdev.o pmem.o regs.o
+cxl_core-y := bus.o memdev.o pmem.o region.o regs.o
diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index e2166f43aefc..3b2bcc091523 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -131,7 +131,68 @@ static ssize_t target_list_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(target_list);
 
+static ssize_t create_region_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+	int rc;
+
+	device_lock(dev);
+	rc = sprintf(buf, "%s\n",
+		     cxld->youngest ? dev_name(&cxld->youngest->dev) : "");
+	device_unlock(dev);
+
+	return rc;
+}
+
+static ssize_t create_region_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t len)
+{
+	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+	struct cxl_region *region;
+	ssize_t rc;
+	int val;
+
+	rc = kstrtoint(buf, 0, &val);
+	if (rc)
+		return rc;
+	if (val < 0 || val > 16)
+		return -EINVAL;
+
+	region = cxl_alloc_region(cxld, val);
+	if (IS_ERR(region))
+		return PTR_ERR(region);
+
+	rc = cxl_add_region(cxld, region);
+	if (rc) {
+		cxl_free_region(cxld, region);
+		return rc;
+	}
+
+	cxld->youngest = region;
+	return len;
+}
+static 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;
+}
+static DEVICE_ATTR_WO(delete_region);
+
 static struct attribute *cxl_decoder_base_attrs[] = {
+	&dev_attr_create_region.attr,
+	&dev_attr_delete_region.attr,
 	&dev_attr_start.attr,
 	&dev_attr_size.attr,
 	&dev_attr_locked.attr,
@@ -188,7 +249,13 @@ static void cxl_decoder_release(struct device *dev)
 {
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
 	struct cxl_port *port = dev->parent ? to_cxl_port(dev->parent) : NULL;
+	struct cxl_region *region;
 
+	list_for_each_entry(region, &cxld->regions, list)
+		cxl_delete_region(cxld, dev_name(&region->dev));
+
+	dev_WARN_ONCE(dev, !ida_is_empty(&cxld->region_ida),
+		      "Lost track of a region");
 	if (port)
 		ida_free(&port->decoder_ida, cxld->id);
 	else
@@ -516,8 +583,11 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
 		.interleave_ways = interleave_ways,
 		.interleave_granularity = interleave_granularity,
 		.target_type = type,
+		.regions = LIST_HEAD_INIT(cxld->regions),
 	};
 
+	ida_init(&cxld->region_ida);
+
 	/* handle implied target_list */
 	if (port)
 		if (interleave_ways == 1)
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 5e5862e4d6af..eb1a17103e5d 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -6,6 +6,7 @@
 
 #include <cxl.h>
 #include <mem.h>
+#include <region.h>
 
 extern const struct device_type cxl_nvdimm_bridge_type;
 extern const struct device_type cxl_nvdimm_type;
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
new file mode 100644
index 000000000000..caa34f59a62d
--- /dev/null
+++ b/drivers/cxl/core/region.c
@@ -0,0 +1,158 @@
+// 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 "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,
+};
+
+bool is_cxl_region(struct device *dev)
+{
+	return dev->type == &cxl_region_type;
+}
+EXPORT_SYMBOL_GPL(is_cxl_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);
+
+void cxl_free_region(struct cxl_decoder *cxld, struct cxl_region *region)
+{
+	ida_free(&cxld->region_ida, region->id);
+	kfree(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);
+
+	cxl_free_region(cxld, region);
+}
+
+struct cxl_region *cxl_alloc_region(struct cxl_decoder *cxld,
+				    int interleave_ways)
+{
+	struct cxl_region *region;
+	int rc;
+
+	region = kzalloc(struct_size(region, targets, interleave_ways),
+			 GFP_KERNEL);
+	if (!region)
+		return ERR_PTR(-ENOMEM);
+
+	region->eniw = interleave_ways;
+
+	rc = ida_alloc(&cxld->region_ida, GFP_KERNEL);
+	if (rc < 0) {
+		dev_err(&cxld->dev, "Couldn't get a new id\n");
+		kfree(region);
+		return ERR_PTR(rc);
+	}
+	region->id = rc;
+
+	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, and that @cxld enforces constraints
+ * upon the region as it is configured. Regions may be added to a @cxld but not
+ * activated and therefore it is possible to have more regions in a @cxld than
+ * there are interleave ways in the @cxld. Regions exist only for persistent
+ * capacities.
+ *
+ * Return: zero if the region was added to the @cxld, else returns negative
+ * error code.
+ */
+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);
+}
+
+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));
+
+	cmpxchg(&cxld->youngest, region, NULL);
+
+	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 dcf2d1a59271..448abc5c7918 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -191,6 +191,9 @@ 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.
+ * @regions: List of regions mapped (may be disabled) by this decoder.
+ * @youngest: Last region created for this decoder.
  * @target: active ordered target list in current decoder configuration
  */
 struct cxl_decoder {
@@ -201,6 +204,9 @@ struct cxl_decoder {
 	int interleave_granularity;
 	enum cxl_decoder_type target_type;
 	unsigned long flags;
+	struct ida region_ida;
+	struct list_head regions;
+	struct cxl_region *youngest;
 	struct cxl_dport *target[];
 };
 
@@ -263,6 +269,14 @@ struct cxl_dport {
 	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);
+void cxl_free_region(struct cxl_decoder *cxld, struct cxl_region *region);
+int cxl_add_region(struct cxl_decoder *cxld, struct cxl_region *region);
+int cxl_delete_region(struct cxl_decoder *cxld, const char *region);
+
 struct cxl_port *to_cxl_port(struct device *dev);
 struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
 				   resource_size_t component_reg_phys,
diff --git a/drivers/cxl/region.h b/drivers/cxl/region.h
new file mode 100644
index 000000000000..c8ed3a8bd1e0
--- /dev/null
+++ b/drivers/cxl/region.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright(c) 2021 Intel Corporation. */
+#ifndef __CXL_REGION_H__
+#define __CXL_REGION_H__
+
+#include <linux/uuid.h>
+
+/**
+ * struct cxl_region - CXL region
+ * @dev: This region's device.
+ * @id: This regions id. Id is globally unique across all regions.
+ * @res: Address space consumed by this region.
+ * @requested_size: Size of the region determined from LSA or userspace.
+ * @uuid: The UUID for this region.
+ * @list: Node in decoders region list.
+ * @eniw: Number of interleave ways this region is configured for.
+ * @targets: The memory devices comprising the region.
+ */
+struct cxl_region {
+	struct device dev;
+	int id;
+	struct resource *res;
+	u64 requested_size;
+	uuid_t uuid;
+	struct list_head list;
+	int eniw;
+	struct cxl_memdev *targets[];
+};
+
+#endif
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 13/23] cxl/region: Introduce concept of region configuration
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (11 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 12/23] cxl/region: Add region creation ABI Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 14/23] cxl: Convert driver id to an enum Ben Widawsky
                   ` (9 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

The region creation APIs leave a region unconfigured. Configuring the
region will work 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 "commit" the
config. What the kernel decides to do after a config is committed is out
of scope at this point.

Introduced here are the most basic attributes needed to configure a
region.

A x1 interleave example is provided below:

decoder0.0
├── create_region
├── delete_region
├── devtype
├── locked
├── region0.0:0
│   ├── offset
│   ├── size
│   ├── subsystem -> ../../../../../../../bus/cxl
│   ├── target0
│   ├── uevent
│   ├── uuid
├── size
├── start
├── subsystem -> ../../../../../../bus/cxl
├── target_list
├── target_type
└── uevent

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-cxl |  32 ++++
 drivers/cxl/core/memdev.c               |   5 -
 drivers/cxl/core/region.c               | 222 ++++++++++++++++++++++++
 drivers/cxl/mem.h                       |   5 +
 4 files changed, 259 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 115a25d2899d..70f9d09385a4 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -148,3 +148,35 @@ 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".
+
+What:		/sys/bus/cxl/devices/decoderX.Y/regionX.Y:Z/offset
+Date:		June, 2021
+KernelVersion:	v5.14
+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/{size,uuid,target[0-15]}
+Date:		June, 2021
+KernelVersion:	v5.14
+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:
+
+		==	========================================================
+		size	Manadatory. Phsyical address space the region will
+			consume.
+		uuid	Optional. A unique identifier for the region. If none is
+			selected, the kernel will create one.
+		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'.
+		==	========================================================
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index dce43e03a05b..bdf811c02b4d 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -9,11 +9,6 @@
 
 static DEFINE_IDA(cxl_memdev_ida);
 
-static struct cxl_memdev *to_cxl_memdev(struct device *dev)
-{
-	return container_of(dev, struct cxl_memdev, dev);
-}
-
 static void cxl_memdev_release(struct device *dev)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index caa34f59a62d..8ed513951730 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3,7 +3,9 @@
 #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 "core.h"
 
@@ -16,11 +18,224 @@
  * (programming the hardware) is handled by a separate region driver.
  */
 
+struct cxl_region *to_cxl_region(struct device *dev);
+
+static bool is_region_active(struct cxl_region *region)
+{
+	/* TODO: Regions can't be activated yet. */
+	return false;
+}
+
+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->range.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);
+
+	if (!region->res)
+		return sysfs_emit(buf, "*%llu\n", region->requested_size);
+
+	return sysfs_emit(buf, "%llu\n", resource_size(region->res));
+}
+
+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->requested_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->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->uuid);
+	device_unlock(&region->dev);
+
+	return rc ? rc : len;
+}
+static DEVICE_ATTR_RW(uuid);
+
+static struct attribute *region_attrs[] = {
+	&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->targets[n])
+		ret = sysfs_emit(buf, "\n");
+	else
+		ret = sysfs_emit(buf, "%s\n",
+				 dev_name(&region->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);
+
+	/* Remove target special case */
+	if (len == 1) {
+		cxlmd = region->targets[n];
+		if (cxlmd)
+			put_device(&cxlmd->dev);
+		region->targets[n] = NULL;
+		device_unlock(&region->dev);
+		return len;
+	}
+
+	memdev_dev = bus_find_device_by_name(&cxl_bus_type, NULL, buf);
+	if (!memdev_dev)
+		return -ENOENT;
+
+	cxlmd = to_cxl_memdev(memdev_dev);
+	get_device(&cxlmd->dev);
+	region->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->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
 };
 
 bool is_cxl_region(struct device *dev)
@@ -41,7 +256,14 @@ EXPORT_SYMBOL_GPL(to_cxl_region);
 
 void cxl_free_region(struct cxl_decoder *cxld, struct cxl_region *region)
 {
+	int i;
+
 	ida_free(&cxld->region_ida, region->id);
+	for (i = 0; i < region->eniw; i++) {
+		if (region->targets[i])
+			put_device(&region->targets[i]->dev);
+	}
+
 	kfree(region);
 }
 
diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 3730e3509ab6..7765f59c890d 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -57,6 +57,11 @@ devm_cxl_add_memdev(struct device *host, struct cxl_mem *cxlm,
 		    const struct file_operations *fops,
 		    void (*shutdown)(struct cxl_memdev *cxlmd));
 
+static inline struct cxl_memdev *to_cxl_memdev(struct device *dev)
+{
+	return container_of(dev, struct cxl_memdev, dev);
+}
+
 /**
  * struct cxl_mem - A CXL memory device
  * @pdev: The PCI device associated with this CXL device.
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 14/23] cxl: Convert driver id to an enum
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (12 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 13/23] cxl/region: Introduce concept of region configuration Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 15/23] cxl/region: Introduce a cxl_region driver Ben Widawsky
                   ` (8 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

CXL drivers can be of only one type. As such, an enum is the most
logical way to represent that singleton nature. By converting to an
enum, it's easy and obvious how to add new driver types.

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

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 448abc5c7918..ecfa5ae082fa 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -315,12 +315,17 @@ devm_cxl_add_passthrough_decoder(struct device *host)
 
 extern struct bus_type cxl_bus_type;
 
+enum cxl_driver_id {
+	CXL_DEVICE_NVDIMM_BRIDGE,
+	CXL_DEVICE_NVDIMM,
+};
+
 struct cxl_driver {
 	const char *name;
 	int (*probe)(struct device *dev);
 	void (*remove)(struct device *dev);
 	struct device_driver drv;
-	int id;
+	enum cxl_driver_id id;
 };
 
 static inline struct cxl_driver *to_cxl_drv(struct device_driver *drv)
@@ -333,9 +338,6 @@ int __cxl_driver_register(struct cxl_driver *cxl_drv, struct module *owner,
 #define cxl_driver_register(x) __cxl_driver_register(x, THIS_MODULE, KBUILD_MODNAME)
 void cxl_driver_unregister(struct cxl_driver *cxl_drv);
 
-#define CXL_DEVICE_NVDIMM_BRIDGE	1
-#define CXL_DEVICE_NVDIMM		2
-
 #define MODULE_ALIAS_CXL(type) MODULE_ALIAS("cxl:t" __stringify(type) "*")
 #define CXL_MODALIAS_FMT "cxl:t%d"
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 15/23] cxl/region: Introduce a cxl_region driver
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (13 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 14/23] cxl: Convert driver id to an enum Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 16/23] cxl/core: Convert decoder range to resource Ben Widawsky
                   ` (7 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

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:

echo 1 > /sys/bus/cxl/devices/decoder0.0/create_region
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 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         |   6 ++
 drivers/cxl/Makefile                          |   3 +-
 drivers/cxl/core/bus.c                        |  15 ++-
 drivers/cxl/core/region.c                     |  24 +++--
 drivers/cxl/cxl.h                             |   1 +
 drivers/cxl/mem.h                             |   1 +
 drivers/cxl/region.c                          | 100 ++++++++++++++++++
 drivers/cxl/region.h                          |   4 +
 drivers/cxl/trace.h                           |  33 ++++++
 9 files changed, 178 insertions(+), 9 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 96a1f8be7940..9e19f90ce7a1 100644
--- a/Documentation/driver-api/cxl/memory-devices.rst
+++ b/Documentation/driver-api/cxl/memory-devices.rst
@@ -47,6 +47,9 @@ CXL Core
 
 CXL Regions
 -----------
+.. kernel-doc:: drivers/cxl/region.c
+   :doc: cxl region
+
 .. kernel-doc:: drivers/cxl/region.h
    :identifiers:
 
@@ -56,6 +59,9 @@ CXL Regions
 .. kernel-doc:: drivers/cxl/core/region.c
    :identifiers:
 
+.. kernel-doc:: drivers/cxl/region.c
+   :identifiers:
+
 External Interfaces
 ===================
 
diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index d19d22a19966..5b8ada0b7c3b 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_CXL_BUS) += core/
-obj-$(CONFIG_CXL_MEM) += cxl_pci.o
+obj-$(CONFIG_CXL_MEM) += cxl_pci.o cxl_region.o
 obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
 obj-$(CONFIG_CXL_PMEM) += cxl_pmem.o
 
@@ -8,3 +8,4 @@ cxl_acpi-y := acpi.o
 cxl_pci-y := pci.o
 cxl_acpi-y := acpi.o
 cxl_pmem-y := pmem.o
+cxl_region-y := region.o
diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index 3b2bcc091523..b3f2d19ab01f 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -722,6 +722,8 @@ static int cxl_device_id(struct device *dev)
 		return CXL_DEVICE_NVDIMM_BRIDGE;
 	if (dev->type == &cxl_nvdimm_type)
 		return CXL_DEVICE_NVDIMM;
+	if (is_cxl_region(dev))
+		return CXL_DEVICE_REGION;
 	return 0;
 }
 
@@ -738,7 +740,18 @@ static int cxl_bus_match(struct device *dev, struct device_driver *drv)
 
 static int cxl_bus_probe(struct device *dev)
 {
-	return to_cxl_drv(dev->driver)->probe(dev);
+	int id = cxl_device_id(dev);
+
+	if (id == CXL_DEVICE_REGION) {
+		struct cxl_region *region = to_cxl_region(dev);
+
+		if (is_cxl_region_configured(region))
+			return to_cxl_drv(dev->driver)->probe(dev);
+	} else {
+		return to_cxl_drv(dev->driver)->probe(dev);
+	}
+
+	return -ENODEV;
 }
 
 static int cxl_bus_remove(struct device *dev)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 8ed513951730..6d5a52091ae2 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -18,13 +18,23 @@
  * (programming the hardware) is handled by a separate region driver.
  */
 
-struct cxl_region *to_cxl_region(struct device *dev);
-
-static bool is_region_active(struct cxl_region *region)
+/*
+ * 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(struct cxl_region *region)
 {
-	/* TODO: Regions can't be activated yet. */
-	return false;
+	/* zero sized regions aren't a thing. */
+	if (region->requested_size <= 0)
+		return false;
+
+	/* all regions have at least 1 target */
+	if (!region->targets[0])
+		return false;
+
+	return true;
 }
+EXPORT_SYMBOL_GPL(is_cxl_region_configured);
 
 static ssize_t offset_show(struct device *dev, struct device_attribute *attr,
 			   char *buf)
@@ -63,7 +73,7 @@ static ssize_t size_store(struct device *dev, struct device_attribute *attr,
 		return rc;
 
 	device_lock(&region->dev);
-	if (is_region_active(region))
+	if (region->active)
 		rc = -EBUSY;
 	else
 		region->requested_size = val;
@@ -91,7 +101,7 @@ static ssize_t uuid_store(struct device *dev, struct device_attribute *attr,
 		return -EINVAL;
 
 	device_lock(&region->dev);
-	if (is_region_active(region))
+	if (region->active)
 		rc = -EBUSY;
 	else
 		rc = uuid_parse(buf, &region->uuid);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index ecfa5ae082fa..c516182eb7bc 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -318,6 +318,7 @@ extern struct bus_type cxl_bus_type;
 enum cxl_driver_id {
 	CXL_DEVICE_NVDIMM_BRIDGE,
 	CXL_DEVICE_NVDIMM,
+	CXL_DEVICE_REGION,
 };
 
 struct cxl_driver {
diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 7765f59c890d..4f3ac1ccee0a 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -92,4 +92,5 @@ struct cxl_mem {
 	struct range pmem_range;
 	struct range ram_range;
 };
+
 #endif /* __CXL_MEM_H__ */
diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
new file mode 100644
index 000000000000..71efe7f29a35
--- /dev/null
+++ b/drivers/cxl/region.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
+#include <linux/device.h>
+#include <linux/module.h>
+#include "region.h"
+#include "mem.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.
+ */
+
+static int bind_region(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->requested_size % (SZ_256M * region->eniw)) {
+		trace_cxl_region_bind(region, "Invalid size. Must be multiple of NIW");
+		return -ENXIO;
+	}
+
+	for (i = 0; i < region->eniw; i++)
+		if (!region->targets[i]) {
+			trace_cxl_region_bind(region, "Missing memory device target");
+			return -ENXIO;
+		}
+
+	/* TODO: Allocate from decoder's address space */
+
+	/* TODO: program HDM decoders */
+
+	if (uuid_is_null(&region->uuid))
+		uuid_gen(&region->uuid);
+
+	trace_cxl_region_bind(region, "Region binding succeeded.");
+	return 0;
+}
+
+static int cxl_region_probe(struct device *dev)
+{
+	struct cxl_region *region = to_cxl_region(dev);
+	int ret;
+
+	device_lock_assert(&region->dev);
+
+	if (region->active)
+		return -EBUSY;
+
+	ret = bind_region(region);
+	if (!ret)
+		region->active = true;
+
+	return ret;
+}
+
+static void cxl_region_remove(struct device *dev)
+{
+	/* Remove region from the decoder's address space */
+}
+
+static struct cxl_driver cxl_region_driver = {
+	.name = "cxl_region",
+	.probe = cxl_region_probe,
+	.remove = cxl_region_remove,
+	.id = CXL_DEVICE_REGION,
+};
+
+static __init int cxl_region_init(void)
+{
+	return cxl_driver_register(&cxl_region_driver);
+}
+
+static __exit void cxl_region_exit(void)
+{
+	cxl_driver_unregister(&cxl_region_driver);
+}
+
+MODULE_LICENSE("GPL v2");
+module_init(cxl_region_init);
+module_exit(cxl_region_exit);
+MODULE_IMPORT_NS(CXL);
+MODULE_ALIAS_CXL(CXL_REGION);
diff --git a/drivers/cxl/region.h b/drivers/cxl/region.h
index c8ed3a8bd1e0..0ee1da323231 100644
--- a/drivers/cxl/region.h
+++ b/drivers/cxl/region.h
@@ -14,6 +14,7 @@
  * @uuid: The UUID for this region.
  * @list: Node in decoders region list.
  * @eniw: Number of interleave ways this region is configured for.
+ * @active: If the region has been activated.
  * @targets: The memory devices comprising the region.
  */
 struct cxl_region {
@@ -24,7 +25,10 @@ struct cxl_region {
 	uuid_t uuid;
 	struct list_head list;
 	int eniw;
+	bool active;
 	struct cxl_memdev *targets[];
 };
 
+bool is_cxl_region_configured(struct cxl_region *region);
+
 #endif
diff --git a/drivers/cxl/trace.h b/drivers/cxl/trace.h
new file mode 100644
index 000000000000..d1a2942c3b82
--- /dev/null
+++ b/drivers/cxl/trace.h
@@ -0,0 +1,33 @@
+/* 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>
+
+TRACE_EVENT(cxl_region_bind,
+	TP_PROTO(struct cxl_region *region, char *status),
+	TP_ARGS(region, status),
+	TP_STRUCT__entry(
+		__field(struct cxl_region *, region)
+		__string(status, status)
+	),
+
+	TP_fast_assign(
+		__entry->region = region;
+		__assign_str(status, status);
+	),
+
+	TP_printk("%s failed to bind (%s)", dev_name(&__entry->region->dev), __get_str(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.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 16/23] cxl/core: Convert decoder range to resource
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (14 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 15/23] cxl/region: Introduce a cxl_region driver Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 17/23] cxl/region: Handle region's address space allocation Ben Widawsky
                   ` (6 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

Regions will use the resource API in order to help manage allocated
space. As regions are children of the decoder, it makes sense that the
parent host the main resource to be suballocated by the region.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/core/bus.c    | 9 +++------
 drivers/cxl/core/region.c | 3 +--
 drivers/cxl/cxl.h         | 4 ++--
 3 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index b3f2d19ab01f..888586768631 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -48,7 +48,7 @@ static ssize_t start_show(struct device *dev, struct device_attribute *attr,
 {
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
 
-	return sysfs_emit(buf, "%#llx\n", cxld->range.start);
+	return sysfs_emit(buf, "%#llx\n", cxld->res.start);
 }
 static DEVICE_ATTR_RO(start);
 
@@ -57,7 +57,7 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr,
 {
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
 
-	return sysfs_emit(buf, "%#llx\n", range_len(&cxld->range));
+	return sysfs_emit(buf, "%#llx\n", resource_size(&cxld->res));
 }
 static DEVICE_ATTR_RO(size);
 
@@ -575,10 +575,7 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
 
 	*cxld = (struct cxl_decoder) {
 		.id = rc,
-		.range = {
-			.start = base,
-			.end = base + len - 1,
-		},
+		.res = DEFINE_RES_MEM_NAMED(base, len, "cxl-decoder"),
 		.flags = flags,
 		.interleave_ways = interleave_ways,
 		.interleave_granularity = interleave_granularity,
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6d5a52091ae2..e9b6549d152a 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -45,8 +45,7 @@ static ssize_t offset_show(struct device *dev, struct device_attribute *attr,
 	if (!region->res)
 		return sysfs_emit(buf, "\n");
 
-	return sysfs_emit(buf, "%#llx\n",
-			  cxld->range.start - region->res->start);
+	return sysfs_emit(buf, "%#llx\n", cxld->res.start - region->res->start);
 }
 static DEVICE_ATTR_RO(offset);
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index c516182eb7bc..217619616d95 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -186,7 +186,7 @@ enum cxl_decoder_type {
  * struct cxl_decoder - CXL address range decode configuration
  * @dev: this decoder's device
  * @id: kernel device name id
- * @range: address range considered by this decoder
+ * @res: address space resources considered by this decoder
  * @interleave_ways: number of cxl_dports in this decode
  * @interleave_granularity: data stride per dport
  * @target_type: accelerator vs expander (type2 vs type3) selector
@@ -199,7 +199,7 @@ enum cxl_decoder_type {
 struct cxl_decoder {
 	struct device dev;
 	int id;
-	struct range range;
+	struct resource res;
 	int interleave_ways;
 	int interleave_granularity;
 	enum cxl_decoder_type target_type;
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 17/23] cxl/region: Handle region's address space allocation
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (15 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 16/23] cxl/core: Convert decoder range to resource Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 18/23] cxl/region: Only allow CXL capable targets Ben Widawsky
                   ` (5 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

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.

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

diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index 888586768631..baf4d4308ae5 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/genalloc.h>
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/pci.h>
@@ -637,6 +638,24 @@ devm_cxl_add_decoder(struct device *host, struct cxl_port *port, int nr_targets,
 	rc = devm_add_action_or_reset(host, unregister_cxl_dev, dev);
 	if (rc)
 		return ERR_PTR(rc);
+
+	if (dev->type == &cxl_decoder_root_type) {
+		struct gen_pool *pool;
+		int order = ilog2(SZ_256M * cxld->interleave_ways);
+
+		pool = devm_gen_pool_create(dev, order, NUMA_NO_NODE,
+					    dev_name(dev));
+		if (IS_ERR(pool))
+			return ERR_CAST(pool);
+
+		cxld->address_space = pool;
+
+		rc = gen_pool_add(cxld->address_space, cxld->res.start,
+				  resource_size(&cxld->res), NUMA_NO_NODE);
+		if (rc)
+			return ERR_PTR(rc);
+	}
+
 	return cxld;
 
 err:
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 217619616d95..9975b4ecf78b 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -194,6 +194,7 @@ enum cxl_decoder_type {
  * @region_ida: allocator for region ids.
  * @regions: List of regions mapped (may be disabled) by this decoder.
  * @youngest: Last region created for this decoder.
+ * @address_space: Used/free address space for regions.
  * @target: active ordered target list in current decoder configuration
  */
 struct cxl_decoder {
@@ -207,6 +208,7 @@ struct cxl_decoder {
 	struct ida region_ida;
 	struct list_head regions;
 	struct cxl_region *youngest;
+	struct gen_pool *address_space;
 	struct cxl_dport *target[];
 };
 
diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
index 71efe7f29a35..1e996ffc0f22 100644
--- a/drivers/cxl/region.c
+++ b/drivers/cxl/region.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2021 Intel Corporation. All rights reserved. */
+#include <linux/genalloc.h>
 #include <linux/device.h>
 #include <linux/module.h>
 #include "region.h"
@@ -23,9 +24,34 @@
  * relationship between decoder and region when the region is interleaved.
  */
 
+static int allocate_region_addr(struct cxl_region *region)
+{
+	struct cxl_decoder *cxld = to_cxl_decoder(region->dev.parent);
+	unsigned long start;
+
+	start = gen_pool_alloc(cxld->address_space, region->requested_size);
+	if (!start) {
+		trace_cxl_region_bind(region,
+				      "Couldn't allocate address space");
+		return -ENOMEM;
+	}
+
+	region->res =
+		__request_region(&cxld->res, start, region->requested_size,
+				 dev_name(&region->dev), IORESOURCE_EXCLUSIVE);
+	if (IS_ERR(region->res)) {
+		trace_cxl_region_bind(region, "Couldn't obtain region");
+		gen_pool_free(cxld->address_space, start,
+			      region->requested_size);
+		return PTR_ERR(region->res);
+	}
+
+	return 0;
+}
+
 static int bind_region(struct cxl_region *region)
 {
-	int i;
+	int i, rc;
 
 	if (dev_WARN_ONCE(&region->dev, !is_cxl_region_configured(region),
 			  "unconfigured regions can't be probed (race?)\n")) {
@@ -43,7 +69,9 @@ static int bind_region(struct cxl_region *region)
 			return -ENXIO;
 		}
 
-	/* TODO: Allocate from decoder's address space */
+	rc = allocate_region_addr(region);
+	if (rc)
+		return rc;
 
 	/* TODO: program HDM decoders */
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 18/23] cxl/region: Only allow CXL capable targets
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (16 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 17/23] cxl/region: Handle region's address space allocation Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 19/23] cxl/mem: Introduce CXL mem driver Ben Widawsky
                   ` (4 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

A cxl_memdev exists for all CXL endpoints that support the CXL.io
protocol. If that device cannot participate in CXL.mem protocol, then it
cannot be part of a region's interleave set.

The ABI allows setting a target which is currently not CXL.mem capable
and only will fail when the binding to the region driver occurs. This is
in line with the other configuration parameters which are only strictly
validated when the driver gets bound to the region.

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

diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 4f3ac1ccee0a..27e142cb0c55 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -93,4 +93,9 @@ struct cxl_mem {
 	struct range ram_range;
 };
 
+static inline bool is_cxl_mem_capable(struct cxl_memdev *cxlmd)
+{
+	return false;
+}
+
 #endif /* __CXL_MEM_H__ */
diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
index 1e996ffc0f22..d625dd81c13a 100644
--- a/drivers/cxl/region.c
+++ b/drivers/cxl/region.c
@@ -63,11 +63,17 @@ static int bind_region(struct cxl_region *region)
 		return -ENXIO;
 	}
 
-	for (i = 0; i < region->eniw; i++)
+	for (i = 0; i < region->eniw; i++) {
 		if (!region->targets[i]) {
 			trace_cxl_region_bind(region, "Missing memory device target");
 			return -ENXIO;
 		}
+		if (!is_cxl_mem_capable(region->targets[i])) {
+			trace_cxl_region_bind(region,
+					      "Target isn't CXL.mem capable");
+			return -ENODEV;
+		}
+	}
 
 	rc = allocate_region_addr(region);
 	if (rc)
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 19/23] cxl/mem: Introduce CXL mem driver
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (17 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 18/23] cxl/region: Only allow CXL capable targets Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 20/23] cxl/memdev: Determine CXL.mem capability Ben Widawsky
                   ` (3 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

CXL endpoints that participate in the CXL.mem protocol require extra
control to ensure architectural constraints are met for device
management.

This driver will implement those controls.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/Makefile      |  3 ++-
 drivers/cxl/core/bus.c    |  2 ++
 drivers/cxl/core/memdev.c |  5 +++++
 drivers/cxl/cxl.h         |  1 +
 drivers/cxl/mem.c         | 45 +++++++++++++++++++++++++++++++++++++++
 drivers/cxl/mem.h         |  2 ++
 6 files changed, 57 insertions(+), 1 deletion(-)
 create mode 100644 drivers/cxl/mem.c

diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index 5b8ada0b7c3b..e5e210369502 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -1,10 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_CXL_BUS) += core/
-obj-$(CONFIG_CXL_MEM) += cxl_pci.o cxl_region.o
+obj-$(CONFIG_CXL_MEM) += cxl_mem.o cxl_pci.o cxl_region.o
 obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
 obj-$(CONFIG_CXL_PMEM) += cxl_pmem.o
 
 cxl_acpi-y := acpi.o
+cxl_mem-y := mem.o
 cxl_pci-y := pci.o
 cxl_acpi-y := acpi.o
 cxl_pmem-y := pmem.o
diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index baf4d4308ae5..ecaa0bcb7fe4 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -740,6 +740,8 @@ static int cxl_device_id(struct device *dev)
 		return CXL_DEVICE_NVDIMM;
 	if (is_cxl_region(dev))
 		return CXL_DEVICE_REGION;
+	if (is_cxl_memdev(dev))
+		return CXL_DEVICE_ENDPOINT;
 	return 0;
 }
 
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index bdf811c02b4d..45894164560b 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -217,3 +217,8 @@ devm_cxl_add_memdev(struct device *host, struct cxl_mem *cxlm,
 	return ERR_PTR(rc);
 }
 EXPORT_SYMBOL_GPL(devm_cxl_add_memdev);
+
+bool is_cxl_memdev(struct device *dev)
+{
+	return dev->type == &cxl_memdev_type;
+}
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 9975b4ecf78b..640006ba457f 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -321,6 +321,7 @@ enum cxl_driver_id {
 	CXL_DEVICE_NVDIMM_BRIDGE,
 	CXL_DEVICE_NVDIMM,
 	CXL_DEVICE_REGION,
+	CXL_DEVICE_ENDPOINT,
 };
 
 struct cxl_driver {
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
new file mode 100644
index 000000000000..2997a03abcb6
--- /dev/null
+++ b/drivers/cxl/mem.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
+#include <linux/device.h>
+#include <linux/module.h>
+#include "mem.h"
+
+/**
+ * DOC: cxl mem
+ *
+ * CXL memory endpoint devices are CXL capable devices that are participating in
+ * CXL.mem protocol. Their functionality builds on top of the CXL.io protocol
+ * that allows enumerating and configuring a CXL endpoint via standard PCI
+ * mechanisms.
+ */
+
+static int cxl_memdev_probe(struct device *dev)
+{
+	return -EOPNOTSUPP;
+}
+
+static void cxl_memdev_remove(struct device *dev)
+{
+}
+
+static struct cxl_driver cxl_memdev_driver = {
+	.name = "cxl_memdev",
+	.probe = cxl_memdev_probe,
+	.remove = cxl_memdev_remove,
+	.id = CXL_DEVICE_ENDPOINT,
+};
+
+static __init int cxl_memdev_init(void)
+{
+	return cxl_driver_register(&cxl_memdev_driver);
+}
+
+static __exit void cxl_memdev_exit(void)
+{
+	cxl_driver_unregister(&cxl_memdev_driver);
+}
+
+MODULE_LICENSE("GPL v2");
+module_init(cxl_memdev_init);
+module_exit(cxl_memdev_exit);
+MODULE_IMPORT_NS(CXL);
diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 27e142cb0c55..d5c0cd541277 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -98,4 +98,6 @@ static inline bool is_cxl_mem_capable(struct cxl_memdev *cxlmd)
 	return false;
 }
 
+bool is_cxl_memdev(struct device *dev);
+
 #endif /* __CXL_MEM_H__ */
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 20/23] cxl/memdev: Determine CXL.mem capability
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (18 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 19/23] cxl/mem: Introduce CXL mem driver Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 21/23] cxl/mem: Check that the device is CXL.mem capable Ben Widawsky
                   ` (2 subsequent siblings)
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

If the "upstream" port of the endpoint is an enumerated downstream CXL
port the memdev driver can bind. This is useful for region
configuration/creation because it provides a way for the region code to
determine if the memdev is actually CXL capable.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/acpi.c        | 23 +++++++----------------
 drivers/cxl/core/bus.c    | 23 +++++++++++++++++++++++
 drivers/cxl/core/memdev.c |  6 ++++++
 drivers/cxl/cxl.h         |  2 ++
 drivers/cxl/mem.c         | 27 ++++++++++++++++++++++++++-
 drivers/cxl/mem.h         |  6 +-----
 6 files changed, 65 insertions(+), 22 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index fd1ae2495ab0..c8486ff273b6 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -7,6 +7,7 @@
 #include <linux/acpi.h>
 #include <linux/pci.h>
 #include "cxl.h"
+#include "mem.h"
 
 static struct acpi_table_header *acpi_cedt;
 
@@ -223,21 +224,6 @@ static int match_add_root_ports(struct pci_dev *pdev, void *data)
 	return 0;
 }
 
-static struct cxl_dport *find_dport_by_dev(struct cxl_port *port, struct device *dev)
-{
-	struct cxl_dport *dport;
-
-	device_lock(&port->dev);
-	list_for_each_entry(dport, &port->dports, list)
-		if (dport->dport == dev) {
-			device_unlock(&port->dev);
-			return dport;
-		}
-
-	device_unlock(&port->dev);
-	return NULL;
-}
-
 static struct acpi_device *to_cxl_host_bridge(struct device *dev)
 {
 	struct acpi_device *adev = to_acpi_device(dev);
@@ -403,9 +389,14 @@ static int cxl_acpi_probe(struct platform_device *pdev)
 	if (rc)
 		goto out;
 
-	if (IS_ENABLED(CONFIG_CXL_PMEM))
+	if (IS_ENABLED(CONFIG_CXL_PMEM)) {
 		rc = device_for_each_child(&root_port->dev, root_port,
 					   add_root_nvdimm_bridge);
+		if (rc)
+			goto out;
+	}
+
+	rc = bus_rescan_devices(&cxl_bus_type);
 
 out:
 	acpi_put_table(acpi_cedt);
diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index ecaa0bcb7fe4..c8c51718f3c7 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -329,6 +329,12 @@ static const struct device_type cxl_port_type = {
 	.groups = cxl_port_attribute_groups,
 };
 
+bool is_cxl_port(struct device *dev)
+{
+	return dev->type == &cxl_port_type;
+}
+EXPORT_SYMBOL_GPL(is_cxl_port);
+
 struct cxl_port *to_cxl_port(struct device *dev)
 {
 	if (dev_WARN_ONCE(dev, dev->type != &cxl_port_type,
@@ -336,6 +342,7 @@ struct cxl_port *to_cxl_port(struct device *dev)
 		return NULL;
 	return container_of(dev, struct cxl_port, dev);
 }
+EXPORT_SYMBOL_GPL(to_cxl_port);
 
 static void unregister_port(void *_port)
 {
@@ -494,6 +501,22 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *new)
 	return dup ? -EEXIST : 0;
 }
 
+struct cxl_dport *find_dport_by_dev(struct cxl_port *port, const struct device *dev)
+{
+	struct cxl_dport *dport;
+
+	device_lock(&port->dev);
+	list_for_each_entry(dport, &port->dports, list)
+		if (dport->dport == dev) {
+			device_unlock(&port->dev);
+			return dport;
+		}
+
+	device_unlock(&port->dev);
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(find_dport_by_dev);
+
 /**
  * cxl_add_dport - append downstream port data to a cxl_port
  * @port: the cxl_port that references this dport
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 45894164560b..2596c3da64a0 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -222,3 +222,9 @@ bool is_cxl_memdev(struct device *dev)
 {
 	return dev->type == &cxl_memdev_type;
 }
+
+bool is_cxl_mem_capable(struct cxl_memdev *cxlmd)
+{
+	return !!cxlmd->dev.driver;
+}
+EXPORT_SYMBOL_GPL(is_cxl_mem_capable);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 640006ba457f..8020af021494 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -284,8 +284,10 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
 				   resource_size_t component_reg_phys,
 				   struct cxl_port *parent_port);
 
+bool is_cxl_port(struct device *dev);
 int cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
 		  resource_size_t component_reg_phys);
+struct cxl_dport *find_dport_by_dev(struct cxl_port *port, const struct device *dev);
 
 struct cxl_decoder *to_cxl_decoder(struct device *dev);
 bool is_root_decoder(struct device *dev);
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 2997a03abcb6..ae2024de7912 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -2,6 +2,7 @@
 /* Copyright(c) 2021 Intel Corporation. All rights reserved. */
 #include <linux/device.h>
 #include <linux/module.h>
+#include <linux/pci.h>
 #include "mem.h"
 
 /**
@@ -13,9 +14,33 @@
  * mechanisms.
  */
 
+static int port_match(struct device *dev, const void *data)
+{
+	struct cxl_port *port;
+
+	if (!is_cxl_port(dev))
+		return 0;
+
+	port = to_cxl_port(dev);
+
+	if (find_dport_by_dev(port, data))
+		return 1;
+
+	return 0;
+}
+
 static int cxl_memdev_probe(struct device *dev)
 {
-	return -EOPNOTSUPP;
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_mem *cxlm = cxlmd->cxlm;
+	struct device *pdev_parent = cxlm->pdev->dev.parent;
+	struct device *port_dev;
+
+	port_dev = bus_find_device(&cxl_bus_type, NULL, pdev_parent, port_match);
+	if (!port_dev)
+		return -ENODEV;
+
+	return 0;
 }
 
 static void cxl_memdev_remove(struct device *dev)
diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index d5c0cd541277..7da1bb48d409 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -93,11 +93,7 @@ struct cxl_mem {
 	struct range ram_range;
 };
 
-static inline bool is_cxl_mem_capable(struct cxl_memdev *cxlmd)
-{
-	return false;
-}
-
+bool is_cxl_mem_capable(struct cxl_memdev *cxlmd);
 bool is_cxl_memdev(struct device *dev);
 
 #endif /* __CXL_MEM_H__ */
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 21/23] cxl/mem: Check that the device is CXL.mem capable
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (19 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 20/23] cxl/memdev: Determine CXL.mem capability Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 22/23] cxl/mem: Add device as a port Ben Widawsky
  2021-07-23 21:06 ` [PATCH 23/23] cxl/core: Map component registers for ports Ben Widawsky
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

CXL.mem capability is required to participate in an interleave set.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/core/bus.c  | 24 ++++++++++++++++++++++++
 drivers/cxl/core/core.h |  1 +
 drivers/cxl/mem.c       | 18 ++++++++++++++++++
 drivers/cxl/pci.c       | 23 -----------------------
 drivers/cxl/pci.h       |  7 ++++++-
 5 files changed, 49 insertions(+), 24 deletions(-)

diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index c8c51718f3c7..75f49fbb8c00 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -716,6 +716,30 @@ struct cxl_decoder *devm_cxl_add_endpoint_decoder(struct device *host,
 }
 EXPORT_SYMBOL_GPL(devm_cxl_add_endpoint_decoder);
 
+int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
+{
+	int pos;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DVSEC);
+	if (!pos)
+		return 0;
+
+	while (pos) {
+		u16 vendor, id;
+
+		pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER1, &vendor);
+		pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER2, &id);
+		if (vendor == PCI_DVSEC_VENDOR_ID_CXL && dvsec == id)
+			return pos;
+
+		pos = pci_find_next_ext_capability(pdev, pos,
+						   PCI_EXT_CAP_ID_DVSEC);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cxl_mem_dvsec);
+
 /**
  * __cxl_driver_register - register a driver for the cxl bus
  * @cxl_drv: cxl driver structure to attach
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index eb1a17103e5d..eab6e6461549 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -6,6 +6,7 @@
 
 #include <cxl.h>
 #include <mem.h>
+#include <pci.h>
 #include <region.h>
 
 extern const struct device_type cxl_nvdimm_bridge_type;
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index ae2024de7912..40281dcc0f3e 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -4,6 +4,7 @@
 #include <linux/module.h>
 #include <linux/pci.h>
 #include "mem.h"
+#include "pci.h"
 
 /**
  * DOC: cxl mem
@@ -33,13 +34,30 @@ static int cxl_memdev_probe(struct device *dev)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
 	struct cxl_mem *cxlm = cxlmd->cxlm;
+	struct pci_dev *pdev = cxlm->pdev;
 	struct device *pdev_parent = cxlm->pdev->dev.parent;
 	struct device *port_dev;
+	int pcie_dvsec;
+	u16 dvsec_ctrl;
 
 	port_dev = bus_find_device(&cxl_bus_type, NULL, pdev_parent, port_match);
 	if (!port_dev)
 		return -ENODEV;
 
+	pcie_dvsec = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_PCIE_DVSEC_CXL_DVSEC_ID);
+	if (!pcie_dvsec) {
+		dev_err(dev, "Unable to determine CXL protocol support");
+		return -ENODEV;
+	}
+
+	pci_read_config_word(pdev,
+			     pcie_dvsec + PCI_DVSEC_ID_CXL_PCIE_CTRL_OFFSET,
+			     &dvsec_ctrl);
+	if (!(dvsec_ctrl & CXL_PCIE_MEM_ENABLE)) {
+		dev_err(dev, "CXL.cache protocol not supported on device");
+		return -ENODEV;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index f924a8c5a831..96837412914d 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -971,29 +971,6 @@ static void cxl_mem_unmap_regblock(struct cxl_mem *cxlm, void __iomem *base)
 	pci_iounmap(cxlm->pdev, base);
 }
 
-static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
-{
-	int pos;
-
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DVSEC);
-	if (!pos)
-		return 0;
-
-	while (pos) {
-		u16 vendor, id;
-
-		pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER1, &vendor);
-		pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER2, &id);
-		if (vendor == PCI_DVSEC_VENDOR_ID_CXL && dvsec == id)
-			return pos;
-
-		pos = pci_find_next_ext_capability(pdev, pos,
-						   PCI_EXT_CAP_ID_DVSEC);
-	}
-
-	return 0;
-}
-
 static int cxl_probe_regs(struct cxl_mem *cxlm, void __iomem *base,
 			  struct cxl_register_map *map)
 {
diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
index 8c1a58813816..c5a4d51b7561 100644
--- a/drivers/cxl/pci.h
+++ b/drivers/cxl/pci.h
@@ -11,7 +11,10 @@
  */
 #define PCI_DVSEC_HEADER1_LENGTH_MASK	GENMASK(31, 20)
 #define PCI_DVSEC_VENDOR_ID_CXL		0x1E98
-#define PCI_DVSEC_ID_CXL		0x0
+
+#define PCI_DVSEC_ID_PCIE_DVSEC_CXL_DVSEC_ID	0x0
+#define PCI_DVSEC_ID_CXL_PCIE_CTRL_OFFSET	0xC
+#define   CXL_PCIE_MEM_ENABLE			BIT(2)
 
 #define PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID	0x8
 #define PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET	0xC
@@ -29,4 +32,6 @@
 
 #define CXL_REGLOC_ADDR_MASK GENMASK(31, 16)
 
+int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec);
+
 #endif /* __CXL_PCI_H__ */
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 22/23] cxl/mem: Add device as a port
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (20 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 21/23] cxl/mem: Check that the device is CXL.mem capable Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  2021-07-23 21:06 ` [PATCH 23/23] cxl/core: Map component registers for ports Ben Widawsky
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/core/bus.c |  5 ++++-
 drivers/cxl/mem.c      | 12 +++++++++---
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index 75f49fbb8c00..4b58b3f1ec99 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -446,7 +446,10 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
 
 	dev = &port->dev;
 	if (parent_port)
-		rc = dev_set_name(dev, "port%d", port->id);
+		if (is_cxl_memdev(uport))
+			rc = dev_set_name(dev, "devport%d", port->id);
+		else
+			rc = dev_set_name(dev, "port%d", port->id);
 	else
 		rc = dev_set_name(dev, "root%d", port->id);
 	if (rc)
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 40281dcc0f3e..4656636acb8a 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -24,7 +24,7 @@ static int port_match(struct device *dev, const void *data)
 
 	port = to_cxl_port(dev);
 
-	if (find_dport_by_dev(port, data))
+	if (find_dport_by_dev(port, (struct device *)data))
 		return 1;
 
 	return 0;
@@ -37,7 +37,7 @@ static int cxl_memdev_probe(struct device *dev)
 	struct pci_dev *pdev = cxlm->pdev;
 	struct device *pdev_parent = cxlm->pdev->dev.parent;
 	struct device *port_dev;
-	int pcie_dvsec;
+	int pcie_dvsec, rc;
 	u16 dvsec_ctrl;
 
 	port_dev = bus_find_device(&cxl_bus_type, NULL, pdev_parent, port_match);
@@ -58,7 +58,13 @@ static int cxl_memdev_probe(struct device *dev)
 		return -ENODEV;
 	}
 
-	return 0;
+	rc = PTR_ERR_OR_ZERO(devm_cxl_add_port(&cxlmd->dev, &cxlmd->dev,
+					       CXL_RESOURCE_NONE,
+					       to_cxl_port(port_dev)));
+	if (rc)
+		dev_err(dev, "Unable to add devices upstream port");
+
+	return rc;
 }
 
 static void cxl_memdev_remove(struct device *dev)
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 23/23] cxl/core: Map component registers for ports
  2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
                   ` (21 preceding siblings ...)
  2021-07-23 21:06 ` [PATCH 22/23] cxl/mem: Add device as a port Ben Widawsky
@ 2021-07-23 21:06 ` Ben Widawsky
  22 siblings, 0 replies; 28+ messages in thread
From: Ben Widawsky @ 2021-07-23 21:06 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
	Jonathan Cameron, Vishal Verma

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/core/bus.c  | 37 +++++++++++++++++++++++++++++++++++++
 drivers/cxl/core/regs.c |  6 +++---
 drivers/cxl/cxl.h       |  2 ++
 drivers/cxl/mem.c       |  2 +-
 drivers/cxl/mem.h       |  1 +
 drivers/cxl/pci.c       | 13 ++++++++++++-
 6 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index 4b58b3f1ec99..f3ac899815e8 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -379,6 +379,38 @@ static int devm_cxl_link_uport(struct device *host, struct cxl_port *port)
 	return devm_add_action_or_reset(host, cxl_unlink_uport, port);
 }
 
+static int cxl_port_map_component_registers(struct cxl_port *port,
+					    struct cxl_component_regs *regs)
+{
+	struct cxl_register_map map;
+	struct cxl_component_reg_map *comp_map = &map.component_map;
+	void __iomem *crb;
+
+	if (port->component_reg_phys == CXL_RESOURCE_NONE)
+		return 0;
+
+	crb = devm_cxl_iomap_block(&port->dev,
+				   port->component_reg_phys,
+				   /* CXL_COMPONENT_REG_BLOCK_SIZE */ SZ_64K);
+	if (IS_ERR(crb))
+		return PTR_ERR(crb);
+
+	if (!crb) {
+		dev_err(&port->dev, "No component registers mapped\n");
+		return -ENXIO;
+	}
+
+	cxl_probe_component_regs(&port->dev, crb, comp_map);
+	if (!comp_map->hdm_decoder.valid) {
+		dev_err(&port->dev, "HDM decoder registers invalid\n");
+		return -ENXIO;
+	}
+
+	regs->hdm_decoder = crb + comp_map->hdm_decoder.offset;
+
+	return 0;
+}
+
 static struct cxl_port *cxl_port_alloc(struct device *uport,
 				       resource_size_t component_reg_phys,
 				       struct cxl_port *parent_port)
@@ -436,6 +468,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
 				   resource_size_t component_reg_phys,
 				   struct cxl_port *parent_port)
 {
+	struct cxl_component_regs crb;
 	struct cxl_port *port;
 	struct device *dev;
 	int rc;
@@ -467,6 +500,10 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
 	if (rc)
 		return ERR_PTR(rc);
 
+	rc = cxl_port_map_component_registers(port, &crb);
+	if (rc)
+		return ERR_PTR(rc);
+
 	return port;
 
 err:
diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index d05946a6bf53..6f5baf784d23 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -144,9 +144,8 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
 }
 EXPORT_SYMBOL_GPL(cxl_probe_device_regs);
 
-static void __iomem *devm_cxl_iomap_block(struct device *dev,
-					  resource_size_t addr,
-					  resource_size_t length)
+void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
+				   resource_size_t length)
 {
 	void __iomem *ret_val;
 	struct resource *res;
@@ -165,6 +164,7 @@ static void __iomem *devm_cxl_iomap_block(struct device *dev,
 
 	return ret_val;
 }
+EXPORT_SYMBOL_GPL(devm_cxl_iomap_block);
 
 int cxl_map_component_regs(struct pci_dev *pdev,
 			   struct cxl_component_regs *regs,
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 8020af021494..87fea255a573 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -149,6 +149,8 @@ struct cxl_register_map {
 	};
 };
 
+void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
+				   resource_size_t length);
 void cxl_probe_component_regs(struct device *dev, void __iomem *base,
 			      struct cxl_component_reg_map *map);
 void cxl_probe_device_regs(struct device *dev, void __iomem *base,
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 4656636acb8a..622042b13d24 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -59,7 +59,7 @@ static int cxl_memdev_probe(struct device *dev)
 	}
 
 	rc = PTR_ERR_OR_ZERO(devm_cxl_add_port(&cxlmd->dev, &cxlmd->dev,
-					       CXL_RESOURCE_NONE,
+					       cxlmd->component_reg_phys,
 					       to_cxl_port(port_dev)));
 	if (rc)
 		dev_err(dev, "Unable to add devices upstream port");
diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 7da1bb48d409..b784123ef35d 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -50,6 +50,7 @@ struct cxl_memdev {
 	struct cxl_mem *cxlm;
 	int id;
 	void (*shutdown)(struct cxl_memdev *cxlmd);
+	unsigned long component_reg_phys;
 };
 
 struct cxl_memdev *
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 96837412914d..a557676f509e 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1345,7 +1345,7 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct cxl_register_map maps[CXL_REGLOC_RBI_TYPES];
 	struct cxl_memdev *cxlmd;
 	struct cxl_mem *cxlm;
-	int rc;
+	int rc, i;
 
 	rc = pcim_enable_device(pdev);
 	if (rc)
@@ -1376,6 +1376,17 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);
 
+	for (i = 0; i < ARRAY_SIZE(maps); i++) {
+		struct cxl_register_map *map = &maps[i];
+
+		if (map->reg_type != CXL_REGLOC_RBI_COMPONENT)
+			continue;
+
+		cxlmd->component_reg_phys =
+			pci_resource_start(pdev, map->barno) +
+			map->block_offset;
+	}
+
 	if (range_len(&cxlm->pmem_range) && IS_ENABLED(CONFIG_CXL_PMEM))
 		rc = devm_cxl_add_nvdimm(&pdev->dev, cxlmd);
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH 10/23] cxl/decoder: Support parentless decoders
  2021-07-23 21:06 ` [PATCH 10/23] cxl/decoder: Support parentless decoders Ben Widawsky
@ 2021-07-30 21:03   ` Dan Williams
  0 siblings, 0 replies; 28+ messages in thread
From: Dan Williams @ 2021-07-30 21:03 UTC (permalink / raw)
  To: Ben Widawsky
  Cc: linux-cxl, Alison Schofield, Ira Weiny, Jonathan Cameron, Vishal Verma

On Fri, Jul 23, 2021 at 2:06 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> Currently, an ACPI0017 device (opaque platform thing) is parent to an
> ACPI0016 device (platform host bridge) child.

ACPI0017 is not a parent of ACPI0016 in the device hierarchy described
in the DSDT.

Linux does not even see ACPI0016 devices because the PCIE host-bridge
identifier dominates.

> The platform level
> decoders don't need a parent child relationship in order to traverse CXL
> hierarchy since once you get to these devices, you have a useless ACPI
> device instead of a CXL one.

The relationship is important for sysfs otherwise the root decoders
are floating in the sysfs hierarchy. For example, with this patch:

# ls /sys/devices/
breakpoint  decoder0.1  LNXSYSTM:00  pci0000:00  platform  software
tracepoint  virtual
decoder0.0  kprobe      msr          pci0000:34  pnp0      system    uprobe

...yuck, CXL decoders floating at the root of /sys/devices

> To support an upcoming expansion for CXL endpoints to be able to
> enumerate their decoders, support this NULL parent as a way to help
> distinguish decoder types.

Need to find a better way to make that distinction...

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 12/23] cxl/region: Add region creation ABI
  2021-07-23 21:06 ` [PATCH 12/23] cxl/region: Add region creation ABI Ben Widawsky
@ 2021-08-14  2:19   ` Dan Williams
  2021-08-26 21:01     ` Ben Widawsky
  0 siblings, 1 reply; 28+ messages in thread
From: Dan Williams @ 2021-08-14  2:19 UTC (permalink / raw)
  To: Ben Widawsky
  Cc: linux-cxl, Alison Schofield, Ira Weiny, Jonathan Cameron, Vishal Verma

On Fri, Jul 23, 2021 at 2:06 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> Regions are created as a child of the decoder that encompasses an
> address space with constraints. Regions only exist for persistent
> capacities.

Maybe I am misinterpreting, but regions need to exist for volatile
capacities too, otherwise what's the interface for enumerating and
reconfiguring volatile interleaves? The only difference is one is
recorded in labels.

Perhaps before creating regions we should write the code to enumerate
volatile regions that might be present since boot. We'll need that
anyway to determine how much CFMWS space is available for dynamic
creation. Once happy with the read side then proceed to the write
side. Thoughts?

>
> When regions are created, the number of desired interleave ways must be
> known. To enable this, the sysfs attribute will take the desired ways as
> input. This interface intentionally allows creation of
> impossible-to-enable regions based on interleave constraints in the
> topology. The reasoning is to create new regions through the kernel
> interfaces which may become possible on reboot under a variety of
> circumstances.
>
> As an example, creating a x1 region with:
> echo 1 > /sys/bus/cxl/devices/decoder0.0/create_region

This proposal has lost its luster for me since I last saw it, and I
must belatedly apologize to you and Jonathan for not internalizing his
critique of this. My previous concern [1] was that tooling would be
surprised by sysfs_update_group() causing attributes to pop into
existence. However, I think the asymmetry is more surprising,
especially when it comes to reconfiguring an existing interleave to be
a different width. There should just be one sysfs attribute that
controls the width of a region.

This allows for additional symmetry where userspace must explicitly
request the next region device name, like so:

# region=$(cat /sys/bus/cxl/devices/decoder0.0/create_region)
# echo $region
region0.0:0
# echo $region > /sys/bus/cxl/devices/decoder0.0/create_region

If userspace races itself 2 threads will attempt
# echo region0.0:0 > /sys/bus/cxl/devices/decoder0.0/create_region
...and one thread will win. This solves the long standing "next seed"
problem (in libnvdimm and device-dax) with a way to atomically get the
next region, and send colliding threads to create another region.


[1]: https://lore.kernel.org/r/CAPcyv4iF1m+xGMaph+K8VJ0+tCvUML9-pUuAWymXoOrvY1jV1w@mail.gmail.com


>
> Will yield /sys/bus/cxl/devices/decoder0.0/region0.0:0
>
> That region may then be deleted with:
> echo region0.0:0 > /sys/bus/cxl/devices/decoder0.0/delete_region
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl       |  21 +++
>  .../driver-api/cxl/memory-devices.rst         |  11 ++
>  drivers/cxl/Makefile                          |   1 +
>  drivers/cxl/core/Makefile                     |   2 +-
>  drivers/cxl/core/bus.c                        |  70 ++++++++
>  drivers/cxl/core/core.h                       |   1 +
>  drivers/cxl/core/region.c                     | 158 ++++++++++++++++++
>  drivers/cxl/cxl.h                             |  14 ++
>  drivers/cxl/region.h                          |  30 ++++
>  9 files changed, 307 insertions(+), 1 deletion(-)
>  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 0b6a2e6e8fbb..115a25d2899d 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -127,3 +127,24 @@ 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:          June, 2021
> +KernelVersion: v5.14
> +Contact:       linux-cxl@vger.kernel.org
> +Description:
> +               Creates a new CXL region of N interleaved ways. Writing a value
> +               of '2' will create a new uninitialized region with 2x interleave
> +               that will be mapped by the CXL decoderX.Y. Reading from this
> +               node will return the last created region. 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:          June, 2021
> +KernelVersion: v5.14
> +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".
> diff --git a/Documentation/driver-api/cxl/memory-devices.rst b/Documentation/driver-api/cxl/memory-devices.rst
> index 46847d8c70a0..96a1f8be7940 100644
> --- a/Documentation/driver-api/cxl/memory-devices.rst
> +++ b/Documentation/driver-api/cxl/memory-devices.rst
> @@ -45,6 +45,17 @@ CXL Core
>  .. kernel-doc:: drivers/cxl/core/regs.c
>     :internal:
>
> +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/Makefile b/drivers/cxl/Makefile
> index d1aaabc940f3..d19d22a19966 100644
> --- a/drivers/cxl/Makefile
> +++ b/drivers/cxl/Makefile
> @@ -4,6 +4,7 @@ obj-$(CONFIG_CXL_MEM) += cxl_pci.o
>  obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
>  obj-$(CONFIG_CXL_PMEM) += cxl_pmem.o
>
> +cxl_acpi-y := acpi.o
>  cxl_pci-y := pci.o
>  cxl_acpi-y := acpi.o
>  cxl_pmem-y := pmem.o
> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> index 1503d8bf5e9a..4d034900e22c 100644
> --- a/drivers/cxl/core/Makefile
> +++ b/drivers/cxl/core/Makefile
> @@ -2,4 +2,4 @@
>  obj-$(CONFIG_CXL_BUS) += cxl_core.o
>
>  ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL -I./drivers/cxl
> -cxl_core-y := bus.o memdev.o pmem.o regs.o
> +cxl_core-y := bus.o memdev.o pmem.o region.o regs.o
> diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> index e2166f43aefc..3b2bcc091523 100644
> --- a/drivers/cxl/core/bus.c
> +++ b/drivers/cxl/core/bus.c
> @@ -131,7 +131,68 @@ static ssize_t target_list_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(target_list);
>
> +static ssize_t create_region_show(struct device *dev,
> +                                 struct device_attribute *attr, char *buf)
> +{
> +       struct cxl_decoder *cxld = to_cxl_decoder(dev);
> +       int rc;
> +
> +       device_lock(dev);
> +       rc = sprintf(buf, "%s\n",
> +                    cxld->youngest ? dev_name(&cxld->youngest->dev) : "");

Youngest goes away if userspace explicitly knows which region it
atomically created.

> +       device_unlock(dev);
> +
> +       return rc;
> +}
> +
> +static ssize_t create_region_store(struct device *dev,
> +                                  struct device_attribute *attr,
> +                                  const char *buf, size_t len)
> +{
> +       struct cxl_decoder *cxld = to_cxl_decoder(dev);
> +       struct cxl_region *region;
> +       ssize_t rc;
> +       int val;
> +
> +       rc = kstrtoint(buf, 0, &val);
> +       if (rc)
> +               return rc;
> +       if (val < 0 || val > 16)
> +               return -EINVAL;
> +
> +       region = cxl_alloc_region(cxld, val);
> +       if (IS_ERR(region))
> +               return PTR_ERR(region);
> +
> +       rc = cxl_add_region(cxld, region);
> +       if (rc) {
> +               cxl_free_region(cxld, region);
> +               return rc;
> +       }
> +
> +       cxld->youngest = region;
> +       return len;
> +}
> +static 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;
> +}
> +static DEVICE_ATTR_WO(delete_region);
> +
>  static struct attribute *cxl_decoder_base_attrs[] = {
> +       &dev_attr_create_region.attr,
> +       &dev_attr_delete_region.attr,
>         &dev_attr_start.attr,
>         &dev_attr_size.attr,
>         &dev_attr_locked.attr,
> @@ -188,7 +249,13 @@ static void cxl_decoder_release(struct device *dev)
>  {
>         struct cxl_decoder *cxld = to_cxl_decoder(dev);
>         struct cxl_port *port = dev->parent ? to_cxl_port(dev->parent) : NULL;
> +       struct cxl_region *region;
>
> +       list_for_each_entry(region, &cxld->regions, list)
> +               cxl_delete_region(cxld, dev_name(&region->dev));

The decoder already has a list of child devices, no need to duplicate
that, i.e. do something like:

device_for_each_child(&cxld->dev, cxld, cxl_delete_region);

> +
> +       dev_WARN_ONCE(dev, !ida_is_empty(&cxld->region_ida),
> +                     "Lost track of a region");
>         if (port)
>                 ida_free(&port->decoder_ida, cxld->id);
>         else
> @@ -516,8 +583,11 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
>                 .interleave_ways = interleave_ways,
>                 .interleave_granularity = interleave_granularity,
>                 .target_type = type,
> +               .regions = LIST_HEAD_INIT(cxld->regions),

This goes away...

>         };
>
> +       ida_init(&cxld->region_ida);
> +
>         /* handle implied target_list */
>         if (port)
>                 if (interleave_ways == 1)
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 5e5862e4d6af..eb1a17103e5d 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -6,6 +6,7 @@
>
>  #include <cxl.h>
>  #include <mem.h>
> +#include <region.h>
>
>  extern const struct device_type cxl_nvdimm_bridge_type;
>  extern const struct device_type cxl_nvdimm_type;
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> new file mode 100644
> index 000000000000..caa34f59a62d
> --- /dev/null
> +++ b/drivers/cxl/core/region.c
> @@ -0,0 +1,158 @@
> +// 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 "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,
> +};
> +
> +bool is_cxl_region(struct device *dev)
> +{
> +       return dev->type == &cxl_region_type;
> +}
> +EXPORT_SYMBOL_GPL(is_cxl_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);
> +
> +void cxl_free_region(struct cxl_decoder *cxld, struct cxl_region *region)
> +{
> +       ida_free(&cxld->region_ida, region->id);
> +       kfree(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);
> +
> +       cxl_free_region(cxld, region);
> +}
> +
> +struct cxl_region *cxl_alloc_region(struct cxl_decoder *cxld,
> +                                   int interleave_ways)
> +{
> +       struct cxl_region *region;
> +       int rc;
> +
> +       region = kzalloc(struct_size(region, targets, interleave_ways),
> +                        GFP_KERNEL);
> +       if (!region)
> +               return ERR_PTR(-ENOMEM);
> +
> +       region->eniw = interleave_ways;
> +
> +       rc = ida_alloc(&cxld->region_ida, GFP_KERNEL);
> +       if (rc < 0) {
> +               dev_err(&cxld->dev, "Couldn't get a new id\n");
> +               kfree(region);
> +               return ERR_PTR(rc);
> +       }
> +       region->id = rc;
> +
> +       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, and that @cxld enforces constraints
> + * upon the region as it is configured. Regions may be added to a @cxld but not
> + * activated and therefore it is possible to have more regions in a @cxld than
> + * there are interleave ways in the @cxld. Regions exist only for persistent
> + * capacities.
> + *
> + * Return: zero if the region was added to the @cxld, else returns negative
> + * error code.
> + */
> +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);
> +}
> +
> +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));
> +
> +       cmpxchg(&cxld->youngest, region, NULL);
> +
> +       device_unregister(&region->dev);
> +       device_unlock(&cxld->dev);
> +
> +       put_device(&region->dev);

What get_device() does this pair with?

> +
> +       return 0;
> +}
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index dcf2d1a59271..448abc5c7918 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -191,6 +191,9 @@ 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.
> + * @regions: List of regions mapped (may be disabled) by this decoder.
> + * @youngest: Last region created for this decoder.
>   * @target: active ordered target list in current decoder configuration
>   */
>  struct cxl_decoder {
> @@ -201,6 +204,9 @@ struct cxl_decoder {
>         int interleave_granularity;
>         enum cxl_decoder_type target_type;
>         unsigned long flags;
> +       struct ida region_ida;
> +       struct list_head regions;
> +       struct cxl_region *youngest;
>         struct cxl_dport *target[];
>  };
>
> @@ -263,6 +269,14 @@ struct cxl_dport {
>         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);
> +void cxl_free_region(struct cxl_decoder *cxld, struct cxl_region *region);
> +int cxl_add_region(struct cxl_decoder *cxld, struct cxl_region *region);
> +int cxl_delete_region(struct cxl_decoder *cxld, const char *region);
> +
>  struct cxl_port *to_cxl_port(struct device *dev);
>  struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
>                                    resource_size_t component_reg_phys,
> diff --git a/drivers/cxl/region.h b/drivers/cxl/region.h
> new file mode 100644
> index 000000000000..c8ed3a8bd1e0
> --- /dev/null
> +++ b/drivers/cxl/region.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2021 Intel Corporation. */
> +#ifndef __CXL_REGION_H__
> +#define __CXL_REGION_H__
> +
> +#include <linux/uuid.h>
> +
> +/**
> + * struct cxl_region - CXL region
> + * @dev: This region's device.
> + * @id: This regions id. Id is globally unique across all regions.
> + * @res: Address space consumed by this region.
> + * @requested_size: Size of the region determined from LSA or userspace.
> + * @uuid: The UUID for this region.
> + * @list: Node in decoders region list.
> + * @eniw: Number of interleave ways this region is configured for.
> + * @targets: The memory devices comprising the region.
> + */
> +struct cxl_region {
> +       struct device dev;
> +       int id;
> +       struct resource *res;
> +       u64 requested_size;

Why requested_size, and not size? Perhaps this instead should be a
pointer to a 'struct cxl_region_label' where the LSA data is cached.

I think we'll want struct cxl_volatile_region and struct
cxl_persistent_region wrapping a common struct cxl_region.

> +       uuid_t uuid;
> +       struct list_head list;
> +       int eniw;
> +       struct cxl_memdev *targets[];
> +};
> +
> +#endif
> --
> 2.32.0
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 12/23] cxl/region: Add region creation ABI
  2021-08-14  2:19   ` Dan Williams
@ 2021-08-26 21:01     ` Ben Widawsky
  2021-08-26 21:44       ` Dan Williams
  0 siblings, 1 reply; 28+ messages in thread
From: Ben Widawsky @ 2021-08-26 21:01 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-cxl, Alison Schofield, Ira Weiny, Jonathan Cameron, Vishal Verma

On 21-08-13 19:19:28, Dan Williams wrote:
> On Fri, Jul 23, 2021 at 2:06 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > Regions are created as a child of the decoder that encompasses an
> > address space with constraints. Regions only exist for persistent
> > capacities.
> 
> Maybe I am misinterpreting, but regions need to exist for volatile
> capacities too, otherwise what's the interface for enumerating and
> reconfiguring volatile interleaves? The only difference is one is
> recorded in labels.

You mean for hotplugged volatile regions? I thought for static volatile regions,
BIOS will always set it up and we'll get just a chunk of address space via EFI
memory map. What would we be reconfiguring?

> 
> Perhaps before creating regions we should write the code to enumerate
> volatile regions that might be present since boot. We'll need that
> anyway to determine how much CFMWS space is available for dynamic
> creation. Once happy with the read side then proceed to the write
> side. Thoughts?

I'm fine either way. I started writing a tool to generate fake labels for this,
but then we switched course. I think since you sent this email, we've kind of
decided to do both at the same time.

> 
> >
> > When regions are created, the number of desired interleave ways must be
> > known. To enable this, the sysfs attribute will take the desired ways as
> > input. This interface intentionally allows creation of
> > impossible-to-enable regions based on interleave constraints in the
> > topology. The reasoning is to create new regions through the kernel
> > interfaces which may become possible on reboot under a variety of
> > circumstances.
> >
> > As an example, creating a x1 region with:
> > echo 1 > /sys/bus/cxl/devices/decoder0.0/create_region
> 
> This proposal has lost its luster for me since I last saw it, and I
> must belatedly apologize to you and Jonathan for not internalizing his
> critique of this. My previous concern [1] was that tooling would be
> surprised by sysfs_update_group() causing attributes to pop into
> existence. However, I think the asymmetry is more surprising,
> especially when it comes to reconfiguring an existing interleave to be
> a different width. There should just be one sysfs attribute that
> controls the width of a region.
> 
> This allows for additional symmetry where userspace must explicitly
> request the next region device name, like so:
> 
> # region=$(cat /sys/bus/cxl/devices/decoder0.0/create_region)
> # echo $region
> region0.0:0
> # echo $region > /sys/bus/cxl/devices/decoder0.0/create_region
> 
> If userspace races itself 2 threads will attempt
> # echo region0.0:0 > /sys/bus/cxl/devices/decoder0.0/create_region
> ...and one thread will win. This solves the long standing "next seed"
> problem (in libnvdimm and device-dax) with a way to atomically get the
> next region, and send colliding threads to create another region.
> 

Apology accepted.

> 
> [1]: https://lore.kernel.org/r/CAPcyv4iF1m+xGMaph+K8VJ0+tCvUML9-pUuAWymXoOrvY1jV1w@mail.gmail.com
> 
> 
> >
> > Will yield /sys/bus/cxl/devices/decoder0.0/region0.0:0
> >
> > That region may then be deleted with:
> > echo region0.0:0 > /sys/bus/cxl/devices/decoder0.0/delete_region
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-cxl       |  21 +++
> >  .../driver-api/cxl/memory-devices.rst         |  11 ++
> >  drivers/cxl/Makefile                          |   1 +
> >  drivers/cxl/core/Makefile                     |   2 +-
> >  drivers/cxl/core/bus.c                        |  70 ++++++++
> >  drivers/cxl/core/core.h                       |   1 +
> >  drivers/cxl/core/region.c                     | 158 ++++++++++++++++++
> >  drivers/cxl/cxl.h                             |  14 ++
> >  drivers/cxl/region.h                          |  30 ++++
> >  9 files changed, 307 insertions(+), 1 deletion(-)
> >  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 0b6a2e6e8fbb..115a25d2899d 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-cxl
> > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > @@ -127,3 +127,24 @@ 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:          June, 2021
> > +KernelVersion: v5.14
> > +Contact:       linux-cxl@vger.kernel.org
> > +Description:
> > +               Creates a new CXL region of N interleaved ways. Writing a value
> > +               of '2' will create a new uninitialized region with 2x interleave
> > +               that will be mapped by the CXL decoderX.Y. Reading from this
> > +               node will return the last created region. 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:          June, 2021
> > +KernelVersion: v5.14
> > +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".
> > diff --git a/Documentation/driver-api/cxl/memory-devices.rst b/Documentation/driver-api/cxl/memory-devices.rst
> > index 46847d8c70a0..96a1f8be7940 100644
> > --- a/Documentation/driver-api/cxl/memory-devices.rst
> > +++ b/Documentation/driver-api/cxl/memory-devices.rst
> > @@ -45,6 +45,17 @@ CXL Core
> >  .. kernel-doc:: drivers/cxl/core/regs.c
> >     :internal:
> >
> > +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/Makefile b/drivers/cxl/Makefile
> > index d1aaabc940f3..d19d22a19966 100644
> > --- a/drivers/cxl/Makefile
> > +++ b/drivers/cxl/Makefile
> > @@ -4,6 +4,7 @@ obj-$(CONFIG_CXL_MEM) += cxl_pci.o
> >  obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
> >  obj-$(CONFIG_CXL_PMEM) += cxl_pmem.o
> >
> > +cxl_acpi-y := acpi.o
> >  cxl_pci-y := pci.o
> >  cxl_acpi-y := acpi.o
> >  cxl_pmem-y := pmem.o
> > diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> > index 1503d8bf5e9a..4d034900e22c 100644
> > --- a/drivers/cxl/core/Makefile
> > +++ b/drivers/cxl/core/Makefile
> > @@ -2,4 +2,4 @@
> >  obj-$(CONFIG_CXL_BUS) += cxl_core.o
> >
> >  ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL -I./drivers/cxl
> > -cxl_core-y := bus.o memdev.o pmem.o regs.o
> > +cxl_core-y := bus.o memdev.o pmem.o region.o regs.o
> > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > index e2166f43aefc..3b2bcc091523 100644
> > --- a/drivers/cxl/core/bus.c
> > +++ b/drivers/cxl/core/bus.c
> > @@ -131,7 +131,68 @@ static ssize_t target_list_show(struct device *dev,
> >  }
> >  static DEVICE_ATTR_RO(target_list);
> >
> > +static ssize_t create_region_show(struct device *dev,
> > +                                 struct device_attribute *attr, char *buf)
> > +{
> > +       struct cxl_decoder *cxld = to_cxl_decoder(dev);
> > +       int rc;
> > +
> > +       device_lock(dev);
> > +       rc = sprintf(buf, "%s\n",
> > +                    cxld->youngest ? dev_name(&cxld->youngest->dev) : "");
> 
> Youngest goes away if userspace explicitly knows which region it
> atomically created.
> 
> > +       device_unlock(dev);
> > +
> > +       return rc;
> > +}
> > +
> > +static ssize_t create_region_store(struct device *dev,
> > +                                  struct device_attribute *attr,
> > +                                  const char *buf, size_t len)
> > +{
> > +       struct cxl_decoder *cxld = to_cxl_decoder(dev);
> > +       struct cxl_region *region;
> > +       ssize_t rc;
> > +       int val;
> > +
> > +       rc = kstrtoint(buf, 0, &val);
> > +       if (rc)
> > +               return rc;
> > +       if (val < 0 || val > 16)
> > +               return -EINVAL;
> > +
> > +       region = cxl_alloc_region(cxld, val);
> > +       if (IS_ERR(region))
> > +               return PTR_ERR(region);
> > +
> > +       rc = cxl_add_region(cxld, region);
> > +       if (rc) {
> > +               cxl_free_region(cxld, region);
> > +               return rc;
> > +       }
> > +
> > +       cxld->youngest = region;
> > +       return len;
> > +}
> > +static 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;
> > +}
> > +static DEVICE_ATTR_WO(delete_region);
> > +
> >  static struct attribute *cxl_decoder_base_attrs[] = {
> > +       &dev_attr_create_region.attr,
> > +       &dev_attr_delete_region.attr,
> >         &dev_attr_start.attr,
> >         &dev_attr_size.attr,
> >         &dev_attr_locked.attr,
> > @@ -188,7 +249,13 @@ static void cxl_decoder_release(struct device *dev)
> >  {
> >         struct cxl_decoder *cxld = to_cxl_decoder(dev);
> >         struct cxl_port *port = dev->parent ? to_cxl_port(dev->parent) : NULL;
> > +       struct cxl_region *region;
> >
> > +       list_for_each_entry(region, &cxld->regions, list)
> > +               cxl_delete_region(cxld, dev_name(&region->dev));
> 
> The decoder already has a list of child devices, no need to duplicate
> that, i.e. do something like:
> 
> device_for_each_child(&cxld->dev, cxld, cxl_delete_region);
> 
> > +
> > +       dev_WARN_ONCE(dev, !ida_is_empty(&cxld->region_ida),
> > +                     "Lost track of a region");
> >         if (port)
> >                 ida_free(&port->decoder_ida, cxld->id);
> >         else
> > @@ -516,8 +583,11 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
> >                 .interleave_ways = interleave_ways,
> >                 .interleave_granularity = interleave_granularity,
> >                 .target_type = type,
> > +               .regions = LIST_HEAD_INIT(cxld->regions),
> 
> This goes away...
> 
> >         };
> >
> > +       ida_init(&cxld->region_ida);
> > +
> >         /* handle implied target_list */
> >         if (port)
> >                 if (interleave_ways == 1)
> > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> > index 5e5862e4d6af..eb1a17103e5d 100644
> > --- a/drivers/cxl/core/core.h
> > +++ b/drivers/cxl/core/core.h
> > @@ -6,6 +6,7 @@
> >
> >  #include <cxl.h>
> >  #include <mem.h>
> > +#include <region.h>
> >
> >  extern const struct device_type cxl_nvdimm_bridge_type;
> >  extern const struct device_type cxl_nvdimm_type;
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > new file mode 100644
> > index 000000000000..caa34f59a62d
> > --- /dev/null
> > +++ b/drivers/cxl/core/region.c
> > @@ -0,0 +1,158 @@
> > +// 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 "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,
> > +};
> > +
> > +bool is_cxl_region(struct device *dev)
> > +{
> > +       return dev->type == &cxl_region_type;
> > +}
> > +EXPORT_SYMBOL_GPL(is_cxl_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);
> > +
> > +void cxl_free_region(struct cxl_decoder *cxld, struct cxl_region *region)
> > +{
> > +       ida_free(&cxld->region_ida, region->id);
> > +       kfree(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);
> > +
> > +       cxl_free_region(cxld, region);
> > +}
> > +
> > +struct cxl_region *cxl_alloc_region(struct cxl_decoder *cxld,
> > +                                   int interleave_ways)
> > +{
> > +       struct cxl_region *region;
> > +       int rc;
> > +
> > +       region = kzalloc(struct_size(region, targets, interleave_ways),
> > +                        GFP_KERNEL);
> > +       if (!region)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       region->eniw = interleave_ways;
> > +
> > +       rc = ida_alloc(&cxld->region_ida, GFP_KERNEL);
> > +       if (rc < 0) {
> > +               dev_err(&cxld->dev, "Couldn't get a new id\n");
> > +               kfree(region);
> > +               return ERR_PTR(rc);
> > +       }
> > +       region->id = rc;
> > +
> > +       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, and that @cxld enforces constraints
> > + * upon the region as it is configured. Regions may be added to a @cxld but not
> > + * activated and therefore it is possible to have more regions in a @cxld than
> > + * there are interleave ways in the @cxld. Regions exist only for persistent
> > + * capacities.
> > + *
> > + * Return: zero if the region was added to the @cxld, else returns negative
> > + * error code.
> > + */
> > +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);
> > +}
> > +
> > +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));
> > +
> > +       cmpxchg(&cxld->youngest, region, NULL);
> > +
> > +       device_unregister(&region->dev);
> > +       device_unlock(&cxld->dev);
> > +
> > +       put_device(&region->dev);
> 
> What get_device() does this pair with?
> 
> > +
> > +       return 0;
> > +}
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index dcf2d1a59271..448abc5c7918 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -191,6 +191,9 @@ 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.
> > + * @regions: List of regions mapped (may be disabled) by this decoder.
> > + * @youngest: Last region created for this decoder.
> >   * @target: active ordered target list in current decoder configuration
> >   */
> >  struct cxl_decoder {
> > @@ -201,6 +204,9 @@ struct cxl_decoder {
> >         int interleave_granularity;
> >         enum cxl_decoder_type target_type;
> >         unsigned long flags;
> > +       struct ida region_ida;
> > +       struct list_head regions;
> > +       struct cxl_region *youngest;
> >         struct cxl_dport *target[];
> >  };
> >
> > @@ -263,6 +269,14 @@ struct cxl_dport {
> >         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);
> > +void cxl_free_region(struct cxl_decoder *cxld, struct cxl_region *region);
> > +int cxl_add_region(struct cxl_decoder *cxld, struct cxl_region *region);
> > +int cxl_delete_region(struct cxl_decoder *cxld, const char *region);
> > +
> >  struct cxl_port *to_cxl_port(struct device *dev);
> >  struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> >                                    resource_size_t component_reg_phys,
> > diff --git a/drivers/cxl/region.h b/drivers/cxl/region.h
> > new file mode 100644
> > index 000000000000..c8ed3a8bd1e0
> > --- /dev/null
> > +++ b/drivers/cxl/region.h
> > @@ -0,0 +1,30 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/* Copyright(c) 2021 Intel Corporation. */
> > +#ifndef __CXL_REGION_H__
> > +#define __CXL_REGION_H__
> > +
> > +#include <linux/uuid.h>
> > +
> > +/**
> > + * struct cxl_region - CXL region
> > + * @dev: This region's device.
> > + * @id: This regions id. Id is globally unique across all regions.
> > + * @res: Address space consumed by this region.
> > + * @requested_size: Size of the region determined from LSA or userspace.
> > + * @uuid: The UUID for this region.
> > + * @list: Node in decoders region list.
> > + * @eniw: Number of interleave ways this region is configured for.
> > + * @targets: The memory devices comprising the region.
> > + */
> > +struct cxl_region {
> > +       struct device dev;
> > +       int id;
> > +       struct resource *res;
> > +       u64 requested_size;
> 
> Why requested_size, and not size? Perhaps this instead should be a
> pointer to a 'struct cxl_region_label' where the LSA data is cached.

I'd been thinking that regions could be rounded up in size by the driver. It
could point to a region label too. I hadn't been touching any label creation yet
because I thought you're bringing that up from the other side. If there's some
pointer I should use for this, I'm game. Which one?

> 
> I think we'll want struct cxl_volatile_region and struct
> cxl_persistent_region wrapping a common struct cxl_region.
> 

Reserving the right to change my mind, I think one region struct should be fine,
it's just one needs to be serialized to the LSA, and the other is ephemeral.

> > +       uuid_t uuid;
> > +       struct list_head list;
> > +       int eniw;
> > +       struct cxl_memdev *targets[];
> > +};
> > +
> > +#endif
> > --
> > 2.32.0
> >

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 12/23] cxl/region: Add region creation ABI
  2021-08-26 21:01     ` Ben Widawsky
@ 2021-08-26 21:44       ` Dan Williams
  0 siblings, 0 replies; 28+ messages in thread
From: Dan Williams @ 2021-08-26 21:44 UTC (permalink / raw)
  To: Ben Widawsky
  Cc: linux-cxl, Alison Schofield, Ira Weiny, Jonathan Cameron, Vishal Verma

On Thu, Aug 26, 2021 at 2:01 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-08-13 19:19:28, Dan Williams wrote:
> > On Fri, Jul 23, 2021 at 2:06 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > Regions are created as a child of the decoder that encompasses an
> > > address space with constraints. Regions only exist for persistent
> > > capacities.
> >
> > Maybe I am misinterpreting, but regions need to exist for volatile
> > capacities too, otherwise what's the interface for enumerating and
> > reconfiguring volatile interleaves? The only difference is one is
> > recorded in labels.
>
> You mean for hotplugged volatile regions? I thought for static volatile regions,
> BIOS will always set it up and we'll get just a chunk of address space via EFI
> memory map. What would we be reconfiguring?

While the BIOS must set up CXL memory that it advertises as
Conventional Memory to the OS, there is no requirement that the BIOS
sets up *all* volatile memory present at boot. I expect that the BIOS
might do things like setup direct host-bridge attached CXL, or
whatever the OEM expects to be present. I also expect there will be
BIOS implementations that punt on configuring after-market add-on
devices, or ones that are deeper in a switch topology. Regardless
there will be CXL regions that the BIOS sets up that consume CFMWS
space and the driver needs to account for those and provide address
translation. The mechanism for kernel services and resource management
for CXL to operate is to establish CXL regions for them.



> > Perhaps before creating regions we should write the code to enumerate
> > volatile regions that might be present since boot. We'll need that
> > anyway to determine how much CFMWS space is available for dynamic
> > creation. Once happy with the read side then proceed to the write
> > side. Thoughts?
>
> I'm fine either way. I started writing a tool to generate fake labels for this,
> but then we switched course. I think since you sent this email, we've kind of
> decided to do both at the same time.

Yeah, both at the same time works for me.

[..]
> > > +/**
> > > + * struct cxl_region - CXL region
> > > + * @dev: This region's device.
> > > + * @id: This regions id. Id is globally unique across all regions.
> > > + * @res: Address space consumed by this region.
> > > + * @requested_size: Size of the region determined from LSA or userspace.
> > > + * @uuid: The UUID for this region.
> > > + * @list: Node in decoders region list.
> > > + * @eniw: Number of interleave ways this region is configured for.
> > > + * @targets: The memory devices comprising the region.
> > > + */
> > > +struct cxl_region {
> > > +       struct device dev;
> > > +       int id;
> > > +       struct resource *res;
> > > +       u64 requested_size;
> >
> > Why requested_size, and not size? Perhaps this instead should be a
> > pointer to a 'struct cxl_region_label' where the LSA data is cached.
>
> I'd been thinking that regions could be rounded up in size by the driver. It
> could point to a region label too. I hadn't been touching any label creation yet
> because I thought you're bringing that up from the other side. If there's some
> pointer I should use for this, I'm game. Which one?

'struct cxl_region_label' is currently in drivers/nvdimm/label.h it
would need to be moved to common location in include/. I am roughly
expecting that drivers/cxl/ calls into drivers/nvdimm/ to manipulate
the label area.

>
> >
> > I think we'll want struct cxl_volatile_region and struct
> > cxl_persistent_region wrapping a common struct cxl_region.
> >
>
> Reserving the right to change my mind, I think one region struct should be fine,
> it's just one needs to be serialized to the LSA, and the other is ephemeral.

It can be like nvdimm nd_region's where it's one data-structure object
with varying 'struct device_type' types.

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2021-08-26 21:44 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
2021-07-23 21:06 ` [PATCH 01/23] cxl: Move cxl_core to new directory Ben Widawsky
2021-07-23 21:06 ` [PATCH 02/23] cxl/core: Improve CXL core kernel docs Ben Widawsky
2021-07-23 21:06 ` [PATCH 03/23] cxl/core: Extract register and pmem functionality Ben Widawsky
2021-07-23 21:06 ` [PATCH 04/23] cxl/mem: Move character device region creation Ben Widawsky
2021-07-23 21:06 ` [PATCH 05/23] cxl: Pass fops and shutdown to memdev creation Ben Widawsky
2021-07-23 21:06 ` [PATCH 06/23] cxl/core: Move memdev management to core Ben Widawsky
2021-07-23 21:06 ` [PATCH 07/23] cxl/pci: Ignore unknown register block types Ben Widawsky
2021-07-23 21:06 ` [PATCH 08/23] cxl/pci: Simplify register setup Ben Widawsky
2021-07-23 21:06 ` [PATCH 09/23] cxl/pci: Retain map information in cxl_mem_probe Ben Widawsky
2021-07-23 21:06 ` [PATCH 10/23] cxl/decoder: Support parentless decoders Ben Widawsky
2021-07-30 21:03   ` Dan Williams
2021-07-23 21:06 ` [PATCH 11/23] cxl: Enable an endpoint decoder type Ben Widawsky
2021-07-23 21:06 ` [PATCH 12/23] cxl/region: Add region creation ABI Ben Widawsky
2021-08-14  2:19   ` Dan Williams
2021-08-26 21:01     ` Ben Widawsky
2021-08-26 21:44       ` Dan Williams
2021-07-23 21:06 ` [PATCH 13/23] cxl/region: Introduce concept of region configuration Ben Widawsky
2021-07-23 21:06 ` [PATCH 14/23] cxl: Convert driver id to an enum Ben Widawsky
2021-07-23 21:06 ` [PATCH 15/23] cxl/region: Introduce a cxl_region driver Ben Widawsky
2021-07-23 21:06 ` [PATCH 16/23] cxl/core: Convert decoder range to resource Ben Widawsky
2021-07-23 21:06 ` [PATCH 17/23] cxl/region: Handle region's address space allocation Ben Widawsky
2021-07-23 21:06 ` [PATCH 18/23] cxl/region: Only allow CXL capable targets Ben Widawsky
2021-07-23 21:06 ` [PATCH 19/23] cxl/mem: Introduce CXL mem driver Ben Widawsky
2021-07-23 21:06 ` [PATCH 20/23] cxl/memdev: Determine CXL.mem capability Ben Widawsky
2021-07-23 21:06 ` [PATCH 21/23] cxl/mem: Check that the device is CXL.mem capable Ben Widawsky
2021-07-23 21:06 ` [PATCH 22/23] cxl/mem: Add device as a port Ben Widawsky
2021-07-23 21:06 ` [PATCH 23/23] cxl/core: Map component registers for ports Ben Widawsky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).