[RFC PATCH 0/5] Introduce memdev driver

[RFC PATCH 0/5] Introduce memdev driver

[Ben Widawsky]

The concept of the memdev has existed since the initial support for CXL.io
landed in 5.12. Here, supported is furthered by adding a driver that is capable
of reporting whether or not the device is also CXL.mem capable. With this, the
region driver is able to consume these devices for programming interleave (or
x1) sets. Unlike the region driver, no explicit sysfs interaction is needed to
utilize this driver.

The logic encapsulated here checks two things:
1. The device itself is CXL.mem enabled.
2. The device's upstream is CXL.mem enabled [1].

What's currently missing is for the cxlmem driver to add the device as an
upstream port (since it has HDM decoders). I'm still working out those details.
HDM decoder programming still remains undone as well, and isn't pertinent to
this series perse.

The patches are based on top of my region patches [2].

The code itself is pretty rough for now, and so I'm mostly looking for feedback
as to whether or not the memdev driver is serving its purpose and checking what
needs to be checked on bind. If however you come along something glaringly bad,
or feel like reviewing not fully tested code (I know it builds), by all means...

[1]: This series doesn't actually add real support for switches which would also
need to make the determination of CXL.mem enabling.
[2]: https://lore.kernel.org/linux-cxl/20210617173655.430424-1-ben.widawsky@intel.com/


Ben Widawsky (5):
  cxl/region: Only allow CXL capable targets
  cxl/mem: Introduce CXL mem driver
  cxl/memdev: Determine CXL.mem capability
  cxl/pci: Export CXL DVSEC functionality
  cxl/mem: Check that the device is CXL.mem capable

 drivers/cxl/Makefile |   3 +-
 drivers/cxl/acpi.c   |   9 +++-
 drivers/cxl/core.c   |   7 +++
 drivers/cxl/cxl.h    |   2 +
 drivers/cxl/mem.c    | 102 +++++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/mem.h    |   3 ++
 drivers/cxl/pci.c    |   7 ++-
 drivers/cxl/pci.h    |   7 ++-
 drivers/cxl/region.c |  12 ++++-
 9 files changed, 147 insertions(+), 5 deletions(-)
 create mode 100644 drivers/cxl/mem.c

-- 
2.32.0

[RFC PATCH 1/5] cxl/region: Only allow CXL capable targets

[Ben Widawsky]

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 | 12 +++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index ff1f9c57e089..3d51bf6c090f 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -84,4 +84,9 @@ struct cxl_mem {
 	struct range ram_range;
 };
 
+static inline bool is_cxl_capable(struct cxl_memdev *cxlmd)
+{
+	return false;
+}
+
 #endif /* __CXL_MEM_H__ */
diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
index 2e73ece001ec..837f4314ffcc 100644
--- a/drivers/cxl/region.c
+++ b/drivers/cxl/region.c
@@ -176,6 +176,10 @@ static size_t set_targetN(struct cxl_region *region, const char *buf, int n, siz
 		return -ENOENT;
 
 	cxlmd = to_cxl_memdev(memdev_dev);
+	if (!is_cxl_capable(cxlmd))
+		dev_dbg(&region->dev,
+			"Setting a target which doesn't support CXL.mem");
+
 	get_device(&cxlmd->dev);
 	region->targets[n] = cxlmd;
 
@@ -432,11 +436,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_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

[RFC PATCH 2/5] cxl/mem: Introduce CXL mem driver

[Ben Widawsky]

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.c   |  2 ++
 drivers/cxl/cxl.h    |  1 +
 drivers/cxl/mem.c    | 45 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/mem.h    |  1 +
 drivers/cxl/pci.c    |  5 +++++
 6 files changed, 56 insertions(+), 1 deletion(-)
 create mode 100644 drivers/cxl/mem.c

diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index f35077c073b8..1fc2836d4f12 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_CXL_BUS) += cxl_core.o
-obj-$(CONFIG_CXL_MEM) += cxl_pci.o cxl_region.o
+obj-$(CONFIG_CXL_MEM) += cxl_pci.o cxl_region.o mem.o
 obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
 obj-$(CONFIG_CXL_PMEM) += cxl_pmem.o
 
@@ -10,3 +10,4 @@ cxl_pci-y := pci.o
 cxl_acpi-y := acpi.o
 cxl_pmem-y := pmem.o
 cxl_region-y := region.o
+cxl_mem-y := mem.o
diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index 5d81fba787e9..16a671722d4e 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -1098,6 +1098,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/cxl.h b/drivers/cxl/cxl.h
index b5b728155d86..ce4b241c5dda 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -328,6 +328,7 @@ void cxl_driver_unregister(struct cxl_driver *cxl_drv);
 #define CXL_DEVICE_NVDIMM_BRIDGE	1
 #define CXL_DEVICE_NVDIMM		2
 #define CXL_DEVICE_REGION		3
+#define CXL_DEVICE_ENDPOINT		4
 
 #define MODULE_ALIAS_CXL(type) MODULE_ALIAS("cxl:t" __stringify(type) "*")
 #define CXL_MODALIAS_FMT "cxl:t%d"
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 3d51bf6c090f..2c20c1ccd6b8 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -88,5 +88,6 @@ static inline bool is_cxl_capable(struct cxl_memdev *cxlmd)
 {
 	return false;
 }
+bool is_cxl_memdev(struct device *dev);
 
 #endif /* __CXL_MEM_H__ */
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 379a106ada94..f9c0eaf3ff4e 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1276,6 +1276,11 @@ static const struct device_type cxl_memdev_type = {
 	.groups = cxl_memdev_attribute_groups,
 };
 
+bool is_cxl_memdev(struct device *dev)
+{
+	return dev->type == &cxl_memdev_type;
+}
+
 static void cxl_memdev_shutdown(struct cxl_memdev *cxlmd)
 {
 	down_write(&cxl_memdev_rwsem);
-- 
2.32.0

[RFC PATCH 3/5] cxl/memdev: Determine CXL.mem capability

[Ben Widawsky]

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 |  9 ++++++++-
 drivers/cxl/core.c |  5 +++++
 drivers/cxl/cxl.h  |  1 +
 drivers/cxl/mem.c  | 41 ++++++++++++++++++++++++++++++++++++++++-
 drivers/cxl/mem.h  |  5 +----
 5 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index cf7c26fb2578..6192739fcf43 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"
 
 /* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */
 #define CFMWS_INTERLEAVE_WAYS(x)	(1 << (x)->interleave_ways)
@@ -398,9 +399,15 @@ 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(cedt_table);
 	return rc;
diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index 16a671722d4e..0a54c6eb84eb 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -303,6 +303,11 @@ 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;
+}
+
 struct cxl_port *to_cxl_port(struct device *dev)
 {
 	if (dev_WARN_ONCE(dev, dev->type != &cxl_port_type,
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index ce4b241c5dda..1a3800616f4a 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -285,6 +285,7 @@ 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);
+bool is_cxl_port(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,
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 2997a03abcb6..cbf18df24109 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,42 @@
  * mechanisms.
  */
 
+static int port_match(struct device *dev, const void *data)
+{
+	struct cxl_dport *dport;
+	struct cxl_port *port;
+	int ret = 0;
+
+	if (!is_cxl_port(dev))
+		return 0;
+
+	port = to_cxl_port(dev);
+
+	device_lock(&port->dev);
+	list_for_each_entry(dport, &port->dports, list)
+		if (dport->dport == data) {
+			ret = 1;
+			break;
+		}
+
+	device_unlock(&port->dev);
+
+	return ret;
+}
+
 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)
@@ -29,6 +63,11 @@ static struct cxl_driver cxl_memdev_driver = {
 	.id = CXL_DEVICE_ENDPOINT,
 };
 
+bool is_cxl_capable(struct cxl_memdev *cxlmd)
+{
+	return cxlmd->dev.driver == &cxl_memdev_driver.drv;
+}
+
 static __init int cxl_memdev_init(void)
 {
 	return cxl_driver_register(&cxl_memdev_driver);
diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 2c20c1ccd6b8..e9333c2ea745 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -84,10 +84,7 @@ struct cxl_mem {
 	struct range ram_range;
 };
 
-static inline bool is_cxl_capable(struct cxl_memdev *cxlmd)
-{
-	return false;
-}
+bool is_cxl_capable(struct cxl_memdev *cxlmd);
 bool is_cxl_memdev(struct device *dev);
 
 #endif /* __CXL_MEM_H__ */
-- 
2.32.0

[RFC PATCH 4/5] cxl/pci: Export CXL DVSEC functionality

[Ben Widawsky]

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

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index f9c0eaf3ff4e..b1a5a18dba92 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -973,7 +973,7 @@ 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 cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
 {
 	int pos;
 
diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
index dad7a831f65f..0d6f50f725bc 100644
--- a/drivers/cxl/pci.h
+++ b/drivers/cxl/pci.h
@@ -28,4 +28,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

[RFC PATCH 5/5] cxl/mem: Check that the device is CXL.mem capable

[Ben Widawsky]

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

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index cbf18df24109..7f26937c7151 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
@@ -41,14 +42,31 @@ 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.h b/drivers/cxl/pci.h
index 0d6f50f725bc..ee26bc8c2ec8 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
-- 
2.32.0

Re: [RFC PATCH 1/5] cxl/region: Only allow CXL capable targets

[Jonathan Cameron]

On Thu, 17 Jun 2021 17:51:56 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> 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 | 12 +++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> index ff1f9c57e089..3d51bf6c090f 100644
> --- a/drivers/cxl/mem.h
> +++ b/drivers/cxl/mem.h
> @@ -84,4 +84,9 @@ struct cxl_mem {
>  	struct range ram_range;
>  };
>  
> +static inline bool is_cxl_capable(struct cxl_memdev *cxlmd)

is_cxl_mem_capable()?

Can be cxl capable in many other senses!

> +{
> +	return false;
> +}
> +
>  #endif /* __CXL_MEM_H__ */
> diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> index 2e73ece001ec..837f4314ffcc 100644
> --- a/drivers/cxl/region.c
> +++ b/drivers/cxl/region.c
> @@ -176,6 +176,10 @@ static size_t set_targetN(struct cxl_region *region, const char *buf, int n, siz
>  		return -ENOENT;
>  
>  	cxlmd = to_cxl_memdev(memdev_dev);
> +	if (!is_cxl_capable(cxlmd))
> +		dev_dbg(&region->dev,
> +			"Setting a target which doesn't support CXL.mem");
> +
>  	get_device(&cxlmd->dev);
>  	region->targets[n] = cxlmd;
>  
> @@ -432,11 +436,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_capable(region->targets[i])) {
> +			trace_cxl_region_bind(region,
> +					      "Target isn't CXL.mem capable");
> +			return -ENODEV;
> +		}
> +	}
>  
>  	rc = allocate_region_addr(region);
>  	if (rc)

Re: [RFC PATCH 3/5] cxl/memdev: Determine CXL.mem capability

[Jonathan Cameron]

On Thu, 17 Jun 2021 17:51:58 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> 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>
I can't resist commenting even when mostly looking at the basic
form a patch. So some trivial stuff in here ;)

Jonathan

> ---
>  drivers/cxl/acpi.c |  9 ++++++++-
>  drivers/cxl/core.c |  5 +++++
>  drivers/cxl/cxl.h  |  1 +
>  drivers/cxl/mem.c  | 41 ++++++++++++++++++++++++++++++++++++++++-
>  drivers/cxl/mem.h  |  5 +----
>  5 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index cf7c26fb2578..6192739fcf43 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"
>  
>  /* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */
>  #define CFMWS_INTERLEAVE_WAYS(x)	(1 << (x)->interleave_ways)
> @@ -398,9 +399,15 @@ 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(cedt_table);
>  	return rc;
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index 16a671722d4e..0a54c6eb84eb 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -303,6 +303,11 @@ 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;

Reuse this in to_cxl_port() for the sanity check..

> +}
> +
>  struct cxl_port *to_cxl_port(struct device *dev)
>  {
>  	if (dev_WARN_ONCE(dev, dev->type != &cxl_port_type,
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index ce4b241c5dda..1a3800616f4a 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -285,6 +285,7 @@ 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);
> +bool is_cxl_port(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,
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 2997a03abcb6..cbf18df24109 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,42 @@
>   * mechanisms.
>   */
>  
> +static int port_match(struct device *dev, const void *data)
> +{
> +	struct cxl_dport *dport;
> +	struct cxl_port *port;
> +	int ret = 0;
> +
> +	if (!is_cxl_port(dev))
> +		return 0;
> +
> +	port = to_cxl_port(dev);
> +
> +	device_lock(&port->dev);
> +	list_for_each_entry(dport, &port->dports, list)
> +		if (dport->dport == data) {
> +			ret = 1;
> +			break;
> +		}
> +
> +	device_unlock(&port->dev);
> +
> +	return ret;
> +}
> +
>  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)
> @@ -29,6 +63,11 @@ static struct cxl_driver cxl_memdev_driver = {
>  	.id = CXL_DEVICE_ENDPOINT,
>  };
>  
> +bool is_cxl_capable(struct cxl_memdev *cxlmd)
> +{
> +	return cxlmd->dev.driver == &cxl_memdev_driver.drv;
> +}

I'll just assume this will end up here directly in the non rfc
version.  In general, I think you've broken this series into
too many small patches, making it harder to see overall shape.

> +
>  static __init int cxl_memdev_init(void)
>  {
>  	return cxl_driver_register(&cxl_memdev_driver);
> diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> index 2c20c1ccd6b8..e9333c2ea745 100644
> --- a/drivers/cxl/mem.h
> +++ b/drivers/cxl/mem.h
> @@ -84,10 +84,7 @@ struct cxl_mem {
>  	struct range ram_range;
>  };
>  
> -static inline bool is_cxl_capable(struct cxl_memdev *cxlmd)
> -{
> -	return false;
> -}
> +bool is_cxl_capable(struct cxl_memdev *cxlmd);
>  bool is_cxl_memdev(struct device *dev);
>  
>  #endif /* __CXL_MEM_H__ */

Re: [RFC PATCH 5/5] cxl/mem: Check that the device is CXL.mem capable

[Jonathan Cameron]

On Thu, 17 Jun 2021 17:52:00 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  drivers/cxl/mem.c | 18 ++++++++++++++++++
>  drivers/cxl/pci.h |  5 ++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index cbf18df24109..7f26937c7151 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
> @@ -41,14 +42,31 @@ 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)) {

Checking control rather than capability?  If you want to know if it supports
it read the bit in register at offset 0x0a

If there is a good reason to see if it's turned on, then document that
with a comment here somewhere and change the error message appropriately.

> +		dev_err(dev, "CXL.cache protocol not supported on device");
> +		return -ENODEV;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
> index 0d6f50f725bc..ee26bc8c2ec8 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

????  That's a non obvious bit of naming!

> +#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

Re: [RFC PATCH 0/5] Introduce memdev driver

[Jonathan Cameron]

On Thu, 17 Jun 2021 17:51:55 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> The concept of the memdev has existed since the initial support for CXL.io
> landed in 5.12. Here, supported is furthered by adding a driver that is capable
> of reporting whether or not the device is also CXL.mem capable. With this, the
> region driver is able to consume these devices for programming interleave (or
> x1) sets. Unlike the region driver, no explicit sysfs interaction is needed to
> utilize this driver.
> 
> The logic encapsulated here checks two things:
> 1. The device itself is CXL.mem enabled.

Need comments in relevant places to say checking if it is enabled,
not capable.

> 2. The device's upstream is CXL.mem enabled [1].
> 
> What's currently missing is for the cxlmem driver to add the device as an
> upstream port (since it has HDM decoders). I'm still working out those details.
> HDM decoder programming still remains undone as well, and isn't pertinent to
> this series perse.
> 
> The patches are based on top of my region patches [2].
> 
> The code itself is pretty rough for now, and so I'm mostly looking for feedback
> as to whether or not the memdev driver is serving its purpose and checking what
> needs to be checked on bind. If however you come along something glaringly bad,
> or feel like reviewing not fully tested code (I know it builds), by all means...

:) 

> 
> [1]: This series doesn't actually add real support for switches which would also
> need to make the determination of CXL.mem enabling.

Any plans to do the QEMU stuff needed for a switch?  I guess it's going to get
messy if you want the hdm decoders to 'work' but should be fairly trivial to make them look
plausible from an interfaces point of view.

> [2]: https://lore.kernel.org/linux-cxl/20210617173655.430424-1-ben.widawsky@intel.com/
> 
> 
> Ben Widawsky (5):
>   cxl/region: Only allow CXL capable targets
>   cxl/mem: Introduce CXL mem driver
>   cxl/memdev: Determine CXL.mem capability
>   cxl/pci: Export CXL DVSEC functionality
>   cxl/mem: Check that the device is CXL.mem capable
> 
>  drivers/cxl/Makefile |   3 +-
>  drivers/cxl/acpi.c   |   9 +++-
>  drivers/cxl/core.c   |   7 +++
>  drivers/cxl/cxl.h    |   2 +
>  drivers/cxl/mem.c    | 102 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/mem.h    |   3 ++
>  drivers/cxl/pci.c    |   7 ++-
>  drivers/cxl/pci.h    |   7 ++-
>  drivers/cxl/region.c |  12 ++++-
>  9 files changed, 147 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/cxl/mem.c
> 

Re: [RFC PATCH 4/5] cxl/pci: Export CXL DVSEC functionality

[Dan Williams]

On Thu, Jun 17, 2021 at 5:52 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  drivers/cxl/pci.c | 2 +-
>  drivers/cxl/pci.h | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)

Fold this with the patch that needs it.

>
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index f9c0eaf3ff4e..b1a5a18dba92 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -973,7 +973,7 @@ 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 cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
>  {
>         int pos;
>
> diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
> index dad7a831f65f..0d6f50f725bc 100644
> --- a/drivers/cxl/pci.h
> +++ b/drivers/cxl/pci.h
> @@ -28,4 +28,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
>

Re: [RFC PATCH 0/5] Introduce memdev driver

[Dan Williams]

On Fri, Jun 18, 2021 at 7:27 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu, 17 Jun 2021 17:51:55 -0700
> Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> > The concept of the memdev has existed since the initial support for CXL.io
> > landed in 5.12. Here, supported is furthered by adding a driver that is capable
> > of reporting whether or not the device is also CXL.mem capable. With this, the
> > region driver is able to consume these devices for programming interleave (or
> > x1) sets. Unlike the region driver, no explicit sysfs interaction is needed to
> > utilize this driver.
> >
> > The logic encapsulated here checks two things:
> > 1. The device itself is CXL.mem enabled.
>
> Need comments in relevant places to say checking if it is enabled,
> not capable.
>
> > 2. The device's upstream is CXL.mem enabled [1].
> >
> > What's currently missing is for the cxlmem driver to add the device as an
> > upstream port (since it has HDM decoders). I'm still working out those details.
> > HDM decoder programming still remains undone as well, and isn't pertinent to
> > this series perse.
> >
> > The patches are based on top of my region patches [2].
> >
> > The code itself is pretty rough for now, and so I'm mostly looking for feedback
> > as to whether or not the memdev driver is serving its purpose and checking what
> > needs to be checked on bind. If however you come along something glaringly bad,
> > or feel like reviewing not fully tested code (I know it builds), by all means...
>
> :)
>
> >
> > [1]: This series doesn't actually add real support for switches which would also
> > need to make the determination of CXL.mem enabling.
>
> Any plans to do the QEMU stuff needed for a switch?  I guess it's going to get
> messy if you want the hdm decoders to 'work' but should be fairly trivial to make them look
> plausible from an interfaces point of view.

It's already the case that the stack is modeling host bridges as
devtype 'cxl_decoder_switch' objects. So, I was hoping we, linux-cxl@
community, could convince ourselves that the algorithm that works for
the default 'cxl_decoder_switch' in the hierarchy can apply generally
to N layers of those. That said, it still would be nice to exercise
that case, but I don't see QEMU getting there anytime soon. I'm taking
a look at modeling a CXL hierarchy with kernel mocked resources for
this purpose.