[PATCH 0/5] cxl: Some preparation work for type2 accelerators support

[PATCH 0/5] cxl: Some preparation work for type2 accelerators support

[Huang Ying]

This series implement some preparation work for the type2 accelerators
support.

We have tested the series with cxl_test via mocking a type2
accelerator as in [1].  Because we need more type2 accelerators
support to simulate a type2 device, we don't include the cxl_test
patch in the series.

[1] https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/

Changes:

RFC->v1:

- Collected Reviewed-by, Thanks Davidlohr and Gregory.

- Some minor fixes, Thanks Ben and others

- Thanks Jonathan to suggest to revise type2/type3 and coherence naming.

- Rebased to latest upstream

- Link to RFC: https://lore.kernel.org/linux-cxl/20240925024647.46735-1-ying.huang@intel.com/

Huang Ying (5):
  cxl: Rename ACPI_CEDT_CFMWS_RESTRICT_TYPE2/TYPE3
  cxl: Rename CXL_DECODER_HOSTONLYMEM/DEVMEM
  cxl: Separate coherence from target type
  cxl: Set type of region to that of the first endpoint
  cxl: Avoid to create dax regions for type2 accelerators

 drivers/cxl/acpi.c           |  9 +++---
 drivers/cxl/core/hdm.c       | 38 +++++++++++++----------
 drivers/cxl/core/mbox.c      |  1 +
 drivers/cxl/core/port.c      | 17 ++++++-----
 drivers/cxl/core/region.c    | 58 ++++++++++++++++++++++++++++++------
 drivers/cxl/cxl.h            | 29 ++++++++++++------
 drivers/cxl/cxlmem.h         | 12 ++++++++
 include/acpi/actbl1.h        | 10 +++----
 tools/testing/cxl/test/cxl.c | 24 +++++++--------
 9 files changed, 135 insertions(+), 63 deletions(-)

Best Regards,
Huang, Ying

[PATCH 1/5] cxl: Rename ACPI_CEDT_CFMWS_RESTRICT_TYPE2/TYPE3

[Huang Ying]

According to the description of the "Window Restrictions" field of
"CFMWS Structure" in the CXL spec v3.1 section 9.18.1.3: CXL Fixed
Memory Window Structure (CFMWS), the bit 0 of "Window Restrictions" is
formerly known as "CXL Type 2 Memory" and renamed to "Device
Coherent", while the bit 1 is formerly known as "CXL Type 3 Memory"
and renamed to "Host-only Coherent".  Because type 3 memory can only
be host-only coherent before, while it can be host-only coherent or
device coherent with "Back-Invalidate" now.

To avoid confusion about type 2/3 memory and device/host-only coherent
in Linux kernel, the patch renames corresponding bit definition from
ACPI_CEDT_CFMWS_RESTRICT_TYPE2/TYPE3 to
ACPI_CEDT_CFMWS_RESTRICT_DEVCOH/HOSTONLYCOH.  This makes the kernel
code consistent with the spec too.

The patch also renames the corresponding cxl_decoder flags
CXL_DECODER_F_TYPE2/TYPE3 to CXL_DECODER_F_DEVCOH/HOSTONLYCOH.

No functionality change is expected.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Gregory Price <gourry@gourry.net>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Alejandro Lucero <alucerop@amd.com>
Cc: Ben Cheatham <benjamin.cheatham@amd.com>
---
 drivers/cxl/acpi.c           |  8 ++++----
 drivers/cxl/core/port.c      |  8 ++++----
 drivers/cxl/cxl.h            | 14 +++++++-------
 include/acpi/actbl1.h        | 10 +++++-----
 tools/testing/cxl/test/cxl.c | 18 +++++++++---------
 5 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 82b78e331d8e..3115f246273b 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -115,10 +115,10 @@ static unsigned long cfmws_to_decoder_flags(int restrictions)
 {
 	unsigned long flags = CXL_DECODER_F_ENABLE;
 
-	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE2)
-		flags |= CXL_DECODER_F_TYPE2;
-	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE3)
-		flags |= CXL_DECODER_F_TYPE3;
+	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_DEVCOH)
+		flags |= CXL_DECODER_F_DEVCOH;
+	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYCOH)
+		flags |= CXL_DECODER_F_HOSTONLYCOH;
 	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE)
 		flags |= CXL_DECODER_F_RAM;
 	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_PMEM)
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index a5e6f3d23cfb..35b6ad4ea0f9 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -125,8 +125,8 @@ static DEVICE_ATTR_RO(name)
 
 CXL_DECODER_FLAG_ATTR(cap_pmem, CXL_DECODER_F_PMEM);
 CXL_DECODER_FLAG_ATTR(cap_ram, CXL_DECODER_F_RAM);
-CXL_DECODER_FLAG_ATTR(cap_type2, CXL_DECODER_F_TYPE2);
-CXL_DECODER_FLAG_ATTR(cap_type3, CXL_DECODER_F_TYPE3);
+CXL_DECODER_FLAG_ATTR(cap_type2, CXL_DECODER_F_DEVCOH);
+CXL_DECODER_FLAG_ATTR(cap_type3, CXL_DECODER_F_HOSTONLYCOH);
 CXL_DECODER_FLAG_ATTR(locked, CXL_DECODER_F_LOCK);
 
 static ssize_t target_type_show(struct device *dev,
@@ -326,14 +326,14 @@ static struct attribute *cxl_decoder_root_attrs[] = {
 
 static bool can_create_pmem(struct cxl_root_decoder *cxlrd)
 {
-	unsigned long flags = CXL_DECODER_F_TYPE3 | CXL_DECODER_F_PMEM;
+	unsigned long flags = CXL_DECODER_F_HOSTONLYCOH | CXL_DECODER_F_PMEM;
 
 	return (cxlrd->cxlsd.cxld.flags & flags) == flags;
 }
 
 static bool can_create_ram(struct cxl_root_decoder *cxlrd)
 {
-	unsigned long flags = CXL_DECODER_F_TYPE3 | CXL_DECODER_F_RAM;
+	unsigned long flags = CXL_DECODER_F_HOSTONLYCOH | CXL_DECODER_F_RAM;
 
 	return (cxlrd->cxlsd.cxld.flags & flags) == flags;
 }
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 0fc96f8bf15c..a34e4256aa5f 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -315,13 +315,13 @@ resource_size_t cxl_rcd_component_reg_phys(struct device *dev,
  * Additionally indicate whether decoder settings were autodetected,
  * user customized.
  */
-#define CXL_DECODER_F_RAM   BIT(0)
-#define CXL_DECODER_F_PMEM  BIT(1)
-#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_ENABLE    BIT(5)
-#define CXL_DECODER_F_MASK  GENMASK(5, 0)
+#define CXL_DECODER_F_RAM         BIT(0)
+#define CXL_DECODER_F_PMEM        BIT(1)
+#define CXL_DECODER_F_DEVCOH      BIT(2)
+#define CXL_DECODER_F_HOSTONLYCOH BIT(3)
+#define CXL_DECODER_F_LOCK        BIT(4)
+#define CXL_DECODER_F_ENABLE      BIT(5)
+#define CXL_DECODER_F_MASK        GENMASK(5, 0)
 
 enum cxl_decoder_type {
 	CXL_DECODER_DEVMEM = 2,
diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index 199afc2cd122..2b2111035669 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -551,11 +551,11 @@ struct acpi_cedt_cfmws_target_element {
 
 /* Values for Restrictions field above */
 
-#define ACPI_CEDT_CFMWS_RESTRICT_TYPE2      (1)
-#define ACPI_CEDT_CFMWS_RESTRICT_TYPE3      (1<<1)
-#define ACPI_CEDT_CFMWS_RESTRICT_VOLATILE   (1<<2)
-#define ACPI_CEDT_CFMWS_RESTRICT_PMEM       (1<<3)
-#define ACPI_CEDT_CFMWS_RESTRICT_FIXED      (1<<4)
+#define ACPI_CEDT_CFMWS_RESTRICT_DEVCOH        (1)
+#define ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYCOH   (1<<1)
+#define ACPI_CEDT_CFMWS_RESTRICT_VOLATILE      (1<<2)
+#define ACPI_CEDT_CFMWS_RESTRICT_PMEM          (1<<3)
+#define ACPI_CEDT_CFMWS_RESTRICT_FIXED         (1<<4)
 
 /* 2: CXL XOR Interleave Math Structure */
 
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index 90d5afd52dd0..3982d292d286 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -209,7 +209,7 @@ static struct {
 			},
 			.interleave_ways = 0,
 			.granularity = 4,
-			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_TYPE3 |
+			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYCOH |
 					ACPI_CEDT_CFMWS_RESTRICT_VOLATILE,
 			.qtg_id = FAKE_QTG_ID,
 			.window_size = SZ_256M * 4UL,
@@ -224,7 +224,7 @@ static struct {
 			},
 			.interleave_ways = 1,
 			.granularity = 4,
-			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_TYPE3 |
+			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYCOH |
 					ACPI_CEDT_CFMWS_RESTRICT_VOLATILE,
 			.qtg_id = FAKE_QTG_ID,
 			.window_size = SZ_256M * 8UL,
@@ -239,7 +239,7 @@ static struct {
 			},
 			.interleave_ways = 0,
 			.granularity = 4,
-			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_TYPE3 |
+			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYCOH |
 					ACPI_CEDT_CFMWS_RESTRICT_PMEM,
 			.qtg_id = FAKE_QTG_ID,
 			.window_size = SZ_256M * 4UL,
@@ -254,7 +254,7 @@ static struct {
 			},
 			.interleave_ways = 1,
 			.granularity = 4,
-			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_TYPE3 |
+			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYCOH |
 					ACPI_CEDT_CFMWS_RESTRICT_PMEM,
 			.qtg_id = FAKE_QTG_ID,
 			.window_size = SZ_256M * 8UL,
@@ -269,7 +269,7 @@ static struct {
 			},
 			.interleave_ways = 0,
 			.granularity = 4,
-			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_TYPE3 |
+			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYCOH |
 					ACPI_CEDT_CFMWS_RESTRICT_PMEM,
 			.qtg_id = FAKE_QTG_ID,
 			.window_size = SZ_256M * 4UL,
@@ -284,7 +284,7 @@ static struct {
 			},
 			.interleave_ways = 0,
 			.granularity = 4,
-			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_TYPE3 |
+			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYCOH |
 					ACPI_CEDT_CFMWS_RESTRICT_VOLATILE,
 			.qtg_id = FAKE_QTG_ID,
 			.window_size = SZ_256M,
@@ -301,7 +301,7 @@ static struct {
 			.interleave_arithmetic = ACPI_CEDT_CFMWS_ARITHMETIC_XOR,
 			.interleave_ways = 0,
 			.granularity = 4,
-			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_TYPE3 |
+			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYCOH |
 					ACPI_CEDT_CFMWS_RESTRICT_PMEM,
 			.qtg_id = FAKE_QTG_ID,
 			.window_size = SZ_256M * 8UL,
@@ -317,7 +317,7 @@ static struct {
 			.interleave_arithmetic = ACPI_CEDT_CFMWS_ARITHMETIC_XOR,
 			.interleave_ways = 1,
 			.granularity = 0,
-			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_TYPE3 |
+			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYCOH |
 					ACPI_CEDT_CFMWS_RESTRICT_PMEM,
 			.qtg_id = FAKE_QTG_ID,
 			.window_size = SZ_256M * 8UL,
@@ -333,7 +333,7 @@ static struct {
 			.interleave_arithmetic = ACPI_CEDT_CFMWS_ARITHMETIC_XOR,
 			.interleave_ways = 2,
 			.granularity = 0,
-			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_TYPE3 |
+			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYCOH |
 					ACPI_CEDT_CFMWS_RESTRICT_PMEM,
 			.qtg_id = FAKE_QTG_ID,
 			.window_size = SZ_256M * 16UL,
-- 
2.39.2

[PATCH 2/5] cxl: Rename CXL_DECODER_HOSTONLYMEM/DEVMEM

[Huang Ying]

Previously, CXL type3 devices (memory expanders) use host only
coherence (HDM-H), while CXL type2 devices (accelerators) use dev
coherence (HDM-D).  So the name of the target device type of a cxl
decoder is CXL_DECODER_HOSTONLYMEM for type3 devices and
CXL_DECODER_DEVMEM for type2 devices.  However, this isn't true
anymore.  CXL type3 devices can use dev coherence + back
invalidation (HDM-DB) too.

To avoid confusion between the device type and coherence, the patch
renames CXL_DECODER_HOSTONLYMEM/DEVMEM to CXL_DECODER_EXPANDER/ACCEL.

We don't expect any functionality change in this patch.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Gregory Price <gourry@gourry.net>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Alejandro Lucero <alucerop@amd.com>
Cc: Ben Cheatham <benjamin.cheatham@amd.com>
---
 drivers/cxl/acpi.c           |  2 +-
 drivers/cxl/core/hdm.c       | 16 ++++++++--------
 drivers/cxl/core/port.c      |  6 +++---
 drivers/cxl/core/region.c    |  2 +-
 drivers/cxl/cxl.h            |  4 ++--
 tools/testing/cxl/test/cxl.c |  6 +++---
 6 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 3115f246273b..21486e471305 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -382,7 +382,7 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
 
 	cxld = &cxlrd->cxlsd.cxld;
 	cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions);
-	cxld->target_type = CXL_DECODER_HOSTONLYMEM;
+	cxld->target_type = CXL_DECODER_EXPANDER;
 	cxld->hpa_range = (struct range) {
 		.start = cfmws->base_hpa,
 		.end = cfmws->base_hpa + cfmws->window_size - 1,
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 3df10517a327..57b54ecdb000 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -572,7 +572,7 @@ static void cxld_set_interleave(struct cxl_decoder *cxld, u32 *ctrl)
 static void cxld_set_type(struct cxl_decoder *cxld, u32 *ctrl)
 {
 	u32p_replace_bits(ctrl,
-			  !!(cxld->target_type == CXL_DECODER_HOSTONLYMEM),
+			  !!(cxld->target_type == CXL_DECODER_EXPANDER),
 			  CXL_HDM_DECODER0_CTRL_HOSTONLY);
 }
 
@@ -771,7 +771,7 @@ static int cxl_setup_hdm_decoder_from_dvsec(
 	if (!len)
 		return -ENOENT;
 
-	cxld->target_type = CXL_DECODER_HOSTONLYMEM;
+	cxld->target_type = CXL_DECODER_EXPANDER;
 	cxld->commit = NULL;
 	cxld->reset = NULL;
 	cxld->hpa_range = info->dvsec_range[which];
@@ -847,9 +847,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
 			cxld->flags |= CXL_DECODER_F_LOCK;
 		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl))
-			cxld->target_type = CXL_DECODER_HOSTONLYMEM;
+			cxld->target_type = CXL_DECODER_EXPANDER;
 		else
-			cxld->target_type = CXL_DECODER_DEVMEM;
+			cxld->target_type = CXL_DECODER_ACCEL;
 
 		guard(rwsem_write)(&cxl_region_rwsem);
 		if (cxld->id != cxl_num_decoders_committed(port)) {
@@ -876,16 +876,16 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 			 * more precision.
 			 */
 			if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
-				cxld->target_type = CXL_DECODER_HOSTONLYMEM;
+				cxld->target_type = CXL_DECODER_EXPANDER;
 			else
-				cxld->target_type = CXL_DECODER_DEVMEM;
+				cxld->target_type = CXL_DECODER_ACCEL;
 		} else {
 			/* To be overridden by region type at commit time */
-			cxld->target_type = CXL_DECODER_HOSTONLYMEM;
+			cxld->target_type = CXL_DECODER_EXPANDER;
 		}
 
 		if (!FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl) &&
-		    cxld->target_type == CXL_DECODER_HOSTONLYMEM) {
+		    cxld->target_type == CXL_DECODER_EXPANDER) {
 			ctrl |= CXL_HDM_DECODER0_CTRL_HOSTONLY;
 			writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
 		}
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 35b6ad4ea0f9..e80b0af3d812 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -135,9 +135,9 @@ static ssize_t target_type_show(struct device *dev,
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
 
 	switch (cxld->target_type) {
-	case CXL_DECODER_DEVMEM:
+	case CXL_DECODER_ACCEL:
 		return sysfs_emit(buf, "accelerator\n");
-	case CXL_DECODER_HOSTONLYMEM:
+	case CXL_DECODER_EXPANDER:
 		return sysfs_emit(buf, "expander\n");
 	}
 	return -ENXIO;
@@ -1768,7 +1768,7 @@ static int cxl_decoder_init(struct cxl_port *port, struct cxl_decoder *cxld)
 	/* Pre initialize an "empty" decoder */
 	cxld->interleave_ways = 1;
 	cxld->interleave_granularity = PAGE_SIZE;
-	cxld->target_type = CXL_DECODER_HOSTONLYMEM;
+	cxld->target_type = CXL_DECODER_EXPANDER;
 	cxld->hpa_range = (struct range) {
 		.start = 0,
 		.end = -1,
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 7bb79f3f318c..036356bc4204 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2531,7 +2531,7 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
 		return ERR_PTR(-EBUSY);
 	}
 
-	return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_HOSTONLYMEM);
+	return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_EXPANDER);
 }
 
 static ssize_t create_pmem_region_store(struct device *dev,
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index a34e4256aa5f..f95101994238 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -324,8 +324,8 @@ resource_size_t cxl_rcd_component_reg_phys(struct device *dev,
 #define CXL_DECODER_F_MASK        GENMASK(5, 0)
 
 enum cxl_decoder_type {
-	CXL_DECODER_DEVMEM = 2,
-	CXL_DECODER_HOSTONLYMEM = 3,
+	CXL_DECODER_ACCEL = 2,
+	CXL_DECODER_EXPANDER = 3,
 };
 
 /*
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index 3982d292d286..352a62c745c6 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -724,7 +724,7 @@ static void default_mock_decoder(struct cxl_decoder *cxld)
 
 	cxld->interleave_ways = 1;
 	cxld->interleave_granularity = 256;
-	cxld->target_type = CXL_DECODER_HOSTONLYMEM;
+	cxld->target_type = CXL_DECODER_EXPANDER;
 	cxld->commit = mock_decoder_commit;
 	cxld->reset = mock_decoder_reset;
 }
@@ -798,7 +798,7 @@ static void mock_init_hdm_decoder(struct cxl_decoder *cxld)
 
 	cxld->interleave_ways = 2;
 	eig_to_granularity(window->granularity, &cxld->interleave_granularity);
-	cxld->target_type = CXL_DECODER_HOSTONLYMEM;
+	cxld->target_type = CXL_DECODER_EXPANDER;
 	cxld->flags = CXL_DECODER_F_ENABLE;
 	cxled->state = CXL_DECODER_STATE_AUTO;
 	port->commit_end = cxld->id;
@@ -831,7 +831,7 @@ static void mock_init_hdm_decoder(struct cxl_decoder *cxld)
 		} else
 			cxlsd->target[0] = dport;
 		cxld = &cxlsd->cxld;
-		cxld->target_type = CXL_DECODER_HOSTONLYMEM;
+		cxld->target_type = CXL_DECODER_EXPANDER;
 		cxld->flags = CXL_DECODER_F_ENABLE;
 		iter->commit_end = 0;
 		/*
-- 
2.39.2

Re: [PATCH 2/5] cxl: Rename CXL_DECODER_HOSTONLYMEM/DEVMEM

[Dan Williams]

Huang Ying wrote:
> Previously, CXL type3 devices (memory expanders) use host only
> coherence (HDM-H), while CXL type2 devices (accelerators) use dev
> coherence (HDM-D).  So the name of the target device type of a cxl
> decoder is CXL_DECODER_HOSTONLYMEM for type3 devices and
> CXL_DECODER_DEVMEM for type2 devices.  However, this isn't true
> anymore.  CXL type3 devices can use dev coherence + back
> invalidation (HDM-DB) too.
> 
> To avoid confusion between the device type and coherence, the patch
> renames CXL_DECODER_HOSTONLYMEM/DEVMEM to CXL_DECODER_EXPANDER/ACCEL.

This does not look like an improvement to me. Type-3 devices that
support back-invalidate are DEVMEM devices. The device plays a role in
the coherence. 

Your explanation is the reverse of this commit:

5aa39a9165cf cxl/port: Rename CXL_DECODER_{EXPANDER, ACCELERATOR} => {HOSTONLYMEM, DEVMEM}

...so I am confused what motivated this rename?

Re: [PATCH 2/5] cxl: Rename CXL_DECODER_HOSTONLYMEM/DEVMEM

[Huang, Ying]

Hi, Dan,

Dan Williams <dan.j.williams@intel.com> writes:

> Huang Ying wrote:
>> Previously, CXL type3 devices (memory expanders) use host only
>> coherence (HDM-H), while CXL type2 devices (accelerators) use dev
>> coherence (HDM-D).  So the name of the target device type of a cxl
>> decoder is CXL_DECODER_HOSTONLYMEM for type3 devices and
>> CXL_DECODER_DEVMEM for type2 devices.  However, this isn't true
>> anymore.  CXL type3 devices can use dev coherence + back
>> invalidation (HDM-DB) too.
>> 
>> To avoid confusion between the device type and coherence, the patch
>> renames CXL_DECODER_HOSTONLYMEM/DEVMEM to CXL_DECODER_EXPANDER/ACCEL.
>
> This does not look like an improvement to me. Type-3 devices that
> support back-invalidate are DEVMEM devices. The device plays a role in
> the coherence. 
>
> Your explanation is the reverse of this commit:
>
> 5aa39a9165cf cxl/port: Rename CXL_DECODER_{EXPANDER, ACCELERATOR} => {HOSTONLYMEM, DEVMEM}
>
> ...so I am confused what motivated this rename?

Sorry, I am confused about the target_type and coherence and forgot to
check the history.  In some places, current kernel still hints
target_type (CXL_DECODER_HOSTONLYMEM/DEVMEM) as expander/accelerator.
Should we change them to avoid confusion in the future?

$ grep expander -r drivers/cxl/
drivers/cxl/cxl.h:346: * @target_type: accelerator vs expander (type2 vs type3) selector
drivers/cxl/core/region.c:2450: * @type: select whether this is an expander or accelerator (type-2 or type-3)
drivers/cxl/core/port.c:141:		return sysfs_emit(buf, "expander\n");

The last one is

static ssize_t target_type_show(struct device *dev,
				struct device_attribute *attr, char *buf)
{
	struct cxl_decoder *cxld = to_cxl_decoder(dev);

	switch (cxld->target_type) {
	case CXL_DECODER_DEVMEM:
		return sysfs_emit(buf, "accelerator\n");
	case CXL_DECODER_HOSTONLYMEM:
		return sysfs_emit(buf, "expander\n");
	}
	return -ENXIO;
}
static DEVICE_ATTR_RO(target_type);

for decoder device.  This is a testing ABI documented in,

Documentation/ABI/testing/sysfs-bus-cxl

Is it OK to change this?

--
Best Regards,
Huang, Ying

Re: [PATCH 2/5] cxl: Rename CXL_DECODER_HOSTONLYMEM/DEVMEM

[Dan Williams]

Huang, Ying wrote:
> Hi, Dan,
> 
> Dan Williams <dan.j.williams@intel.com> writes:
> 
> > Huang Ying wrote:
> >> Previously, CXL type3 devices (memory expanders) use host only
> >> coherence (HDM-H), while CXL type2 devices (accelerators) use dev
> >> coherence (HDM-D).  So the name of the target device type of a cxl
> >> decoder is CXL_DECODER_HOSTONLYMEM for type3 devices and
> >> CXL_DECODER_DEVMEM for type2 devices.  However, this isn't true
> >> anymore.  CXL type3 devices can use dev coherence + back
> >> invalidation (HDM-DB) too.
> >> 
> >> To avoid confusion between the device type and coherence, the patch
> >> renames CXL_DECODER_HOSTONLYMEM/DEVMEM to CXL_DECODER_EXPANDER/ACCEL.
> >
> > This does not look like an improvement to me. Type-3 devices that
> > support back-invalidate are DEVMEM devices. The device plays a role in
> > the coherence. 
> >
> > Your explanation is the reverse of this commit:
> >
> > 5aa39a9165cf cxl/port: Rename CXL_DECODER_{EXPANDER, ACCELERATOR} => {HOSTONLYMEM, DEVMEM}
> >
> > ...so I am confused what motivated this rename?
> 
> Sorry, I am confused about the target_type and coherence and forgot to
> check the history.  In some places, current kernel still hints
> target_type (CXL_DECODER_HOSTONLYMEM/DEVMEM) as expander/accelerator.
> Should we change them to avoid confusion in the future?
> 
> $ grep expander -r drivers/cxl/
> drivers/cxl/cxl.h:346: * @target_type: accelerator vs expander (type2 vs type3) selector
> drivers/cxl/core/region.c:2450: * @type: select whether this is an expander or accelerator (type-2 or type-3)
> drivers/cxl/core/port.c:141:		return sysfs_emit(buf, "expander\n");
> 
> The last one is
> 
> static ssize_t target_type_show(struct device *dev,
> 				struct device_attribute *attr, char *buf)
> {
> 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
> 
> 	switch (cxld->target_type) {
> 	case CXL_DECODER_DEVMEM:
> 		return sysfs_emit(buf, "accelerator\n");
> 	case CXL_DECODER_HOSTONLYMEM:
> 		return sysfs_emit(buf, "expander\n");
> 	}
> 	return -ENXIO;
> }
> static DEVICE_ATTR_RO(target_type);
> 
> for decoder device.  This is a testing ABI documented in,
> 
> Documentation/ABI/testing/sysfs-bus-cxl
> 
> Is it OK to change this?

No, why does it need to change?

It is unfortunate, but ABI's are forever. The place to clarify that this
decoder is participating in HDM-D[B] vs HDM-H protocol rather than being
an "accelerator" or "expander" device would be in user tooling like
cxl-cli. sysfs is just a transport, not a UI.

Re: [PATCH 2/5] cxl: Rename CXL_DECODER_HOSTONLYMEM/DEVMEM

[Huang, Ying]

Dan Williams <dan.j.williams@intel.com> writes:

> Huang, Ying wrote:
>> Hi, Dan,
>> 
>> Dan Williams <dan.j.williams@intel.com> writes:
>> 
>> > Huang Ying wrote:
>> >> Previously, CXL type3 devices (memory expanders) use host only
>> >> coherence (HDM-H), while CXL type2 devices (accelerators) use dev
>> >> coherence (HDM-D).  So the name of the target device type of a cxl
>> >> decoder is CXL_DECODER_HOSTONLYMEM for type3 devices and
>> >> CXL_DECODER_DEVMEM for type2 devices.  However, this isn't true
>> >> anymore.  CXL type3 devices can use dev coherence + back
>> >> invalidation (HDM-DB) too.
>> >> 
>> >> To avoid confusion between the device type and coherence, the patch
>> >> renames CXL_DECODER_HOSTONLYMEM/DEVMEM to CXL_DECODER_EXPANDER/ACCEL.
>> >
>> > This does not look like an improvement to me. Type-3 devices that
>> > support back-invalidate are DEVMEM devices. The device plays a role in
>> > the coherence. 
>> >
>> > Your explanation is the reverse of this commit:
>> >
>> > 5aa39a9165cf cxl/port: Rename CXL_DECODER_{EXPANDER, ACCELERATOR} => {HOSTONLYMEM, DEVMEM}
>> >
>> > ...so I am confused what motivated this rename?
>> 
>> Sorry, I am confused about the target_type and coherence and forgot to
>> check the history.  In some places, current kernel still hints
>> target_type (CXL_DECODER_HOSTONLYMEM/DEVMEM) as expander/accelerator.
>> Should we change them to avoid confusion in the future?
>> 
>> $ grep expander -r drivers/cxl/
>> drivers/cxl/cxl.h:346: * @target_type: accelerator vs expander (type2 vs type3) selector
>> drivers/cxl/core/region.c:2450: * @type: select whether this is an expander or accelerator (type-2 or type-3)
>> drivers/cxl/core/port.c:141:		return sysfs_emit(buf, "expander\n");
>> 
>> The last one is
>> 
>> static ssize_t target_type_show(struct device *dev,
>> 				struct device_attribute *attr, char *buf)
>> {
>> 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
>> 
>> 	switch (cxld->target_type) {
>> 	case CXL_DECODER_DEVMEM:
>> 		return sysfs_emit(buf, "accelerator\n");
>> 	case CXL_DECODER_HOSTONLYMEM:
>> 		return sysfs_emit(buf, "expander\n");
>> 	}
>> 	return -ENXIO;
>> }
>> static DEVICE_ATTR_RO(target_type);
>> 
>> for decoder device.  This is a testing ABI documented in,
>> 
>> Documentation/ABI/testing/sysfs-bus-cxl
>> 
>> Is it OK to change this?
>
> No, why does it need to change?

For example, if the target_type is CXL_DECODER_DEVMEM, while the device
is a memory expander with HDM-DB protocol.  The sysfs will show it as
"accelerator".  This may make users or developers confusing.  If we can
show "hostonlymem"/"devmem", that may be better.  But apparently, we
cannot change ABI.

> It is unfortunate, but ABI's are forever. The place to clarify that this
> decoder is participating in HDM-D[B] vs HDM-H protocol rather than being
> an "accelerator" or "expander" device would be in user tooling like
> cxl-cli. sysfs is just a transport, not a UI.

Although it's not perfect, this is a solution.  Another way to solve
this is to separate device coherence (HOSTONLY vs. DEV) and device type
(ACCELERATOR vs. EXPANDER).  In this way, if the "target_type" in sysfs
designates device type, it could show "expander" for memory expander
even if HDM-DB protocol is used.

Another possibility, can we just remove this sysfs file?

--
Best Regards,
Huang, Ying

[PATCH 3/5] cxl: Separate coherence from target type

[Huang Ying]

Previously, target type (expander or accelerator) and
coherence (HOSTONLY (HDM-H) or DEV (HDM-D/DB)) are synonym.  So
current kernel uses target type to designate coherence too.  However,
it's possible for expanders to use HDM-DB now.  So, the patch
separates coherence from target type.

Accordingly, the patch sets the HOSTONLY field of decoder ctrl
register (CXL_HDM_DECODER0_CTRL_HOSTONLY) according to the coherence
instead of the target type.

Because we cannot determine the coherence of decoders via target type,
the patch lets accelerator/expander device drivers specify coherence
explicitly via newly added coherence field in struct cxl_dev_state.

The coherence of each end points in a region needs to be same.  So,
the patch records the coherence of the first added end point in struct
region.  Then, it checks whether the coherence of all other end points
is same.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Alejandro Lucero <alucerop@amd.com>
Cc: Ben Cheatham <benjamin.cheatham@amd.com>
---
 drivers/cxl/core/hdm.c    | 22 +++++++++++++++-------
 drivers/cxl/core/mbox.c   |  1 +
 drivers/cxl/core/port.c   |  1 +
 drivers/cxl/core/region.c | 37 ++++++++++++++++++++++++++++++++++---
 drivers/cxl/cxl.h         | 10 ++++++++++
 drivers/cxl/cxlmem.h      | 12 ++++++++++++
 6 files changed, 73 insertions(+), 10 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 57b54ecdb000..478fb6691759 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -569,10 +569,10 @@ static void cxld_set_interleave(struct cxl_decoder *cxld, u32 *ctrl)
 	*ctrl |= CXL_HDM_DECODER0_CTRL_COMMIT;
 }
 
-static void cxld_set_type(struct cxl_decoder *cxld, u32 *ctrl)
+static void cxld_set_coherence(struct cxl_decoder *cxld, u32 *ctrl)
 {
 	u32p_replace_bits(ctrl,
-			  !!(cxld->target_type == CXL_DECODER_EXPANDER),
+			  !!(cxld->coherence == CXL_DECODER_HOSTONLYCOH),
 			  CXL_HDM_DECODER0_CTRL_HOSTONLY);
 }
 
@@ -667,7 +667,7 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
 	/* common decoder settings */
 	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
 	cxld_set_interleave(cxld, &ctrl);
-	cxld_set_type(cxld, &ctrl);
+	cxld_set_coherence(cxld, &ctrl);
 	base = cxld->hpa_range.start;
 	size = range_len(&cxld->hpa_range);
 
@@ -846,10 +846,13 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 		cxld->flags |= CXL_DECODER_F_ENABLE;
 		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
 			cxld->flags |= CXL_DECODER_F_LOCK;
-		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl))
+		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl)) {
 			cxld->target_type = CXL_DECODER_EXPANDER;
-		else
+			cxld->coherence = CXL_DECODER_HOSTONLYCOH;
+		} else {
 			cxld->target_type = CXL_DECODER_ACCEL;
+			cxld->coherence = CXL_DECODER_DEVCOH;
+		}
 
 		guard(rwsem_write)(&cxl_region_rwsem);
 		if (cxld->id != cxl_num_decoders_committed(port)) {
@@ -879,13 +882,18 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 				cxld->target_type = CXL_DECODER_EXPANDER;
 			else
 				cxld->target_type = CXL_DECODER_ACCEL;
+			if (cxlds->coherence == CXL_DEVCOH_HOSTONLY)
+				cxld->coherence = CXL_DECODER_HOSTONLYCOH;
+			else
+				cxld->coherence = CXL_DECODER_DEVCOH;
 		} else {
-			/* To be overridden by region type at commit time */
+			/* To be overridden by region type/coherence at commit time */
 			cxld->target_type = CXL_DECODER_EXPANDER;
+			cxld->coherence = CXL_DECODER_HOSTONLYCOH;
 		}
 
 		if (!FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl) &&
-		    cxld->target_type == CXL_DECODER_EXPANDER) {
+		    cxld->coherence == CXL_DECODER_HOSTONLYCOH) {
 			ctrl |= CXL_HDM_DECODER0_CTRL_HOSTONLY;
 			writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
 		}
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 5175138c4fb7..fb98cd1a8b61 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1450,6 +1450,7 @@ struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev)
 	mds->cxlds.reg_map.host = dev;
 	mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
 	mds->cxlds.type = CXL_DEVTYPE_CLASSMEM;
+	mds->cxlds.coherence = CXL_DEVCOH_HOSTONLY;
 	mds->ram_perf.qos_class = CXL_QOS_CLASS_INVALID;
 	mds->pmem_perf.qos_class = CXL_QOS_CLASS_INVALID;
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index e80b0af3d812..9ebbffcea26a 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1769,6 +1769,7 @@ static int cxl_decoder_init(struct cxl_port *port, struct cxl_decoder *cxld)
 	cxld->interleave_ways = 1;
 	cxld->interleave_granularity = PAGE_SIZE;
 	cxld->target_type = CXL_DECODER_EXPANDER;
+	cxld->coherence = CXL_DECODER_HOSTONLYCOH;
 	cxld->hpa_range = (struct range) {
 		.start = 0,
 		.end = -1,
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 036356bc4204..21b877d8582f 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1005,9 +1005,10 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
 	}
 
 	/*
-	 * Endpoints should already match the region type, but backstop that
-	 * assumption with an assertion. Switch-decoders change mapping-type
-	 * based on what is mapped when they are assigned to a region.
+	 * Endpoints should already match the region type/coherence, but
+	 * backstop that assumption with an assertion. Switch-decoders change
+	 * mapping-type/coherence based on what is mapped when they are assigned
+	 * to a region.
 	 */
 	dev_WARN_ONCE(&cxlr->dev,
 		      port == cxled_to_port(cxled) &&
@@ -1016,6 +1017,13 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
 		      dev_name(&cxled_to_memdev(cxled)->dev),
 		      dev_name(&cxld->dev), cxld->target_type, cxlr->type);
 	cxld->target_type = cxlr->type;
+	dev_WARN_ONCE(&cxlr->dev,
+		      port == cxled_to_port(cxled) &&
+			      cxld->coherence != cxlr->coherence,
+		      "%s:%s mismatch decoder coherence %d -> %d\n",
+		      dev_name(&cxled_to_memdev(cxled)->dev),
+		      dev_name(&cxld->dev), cxld->coherence, cxlr->coherence);
+	cxld->coherence = cxlr->coherence;
 	cxl_rr->decoder = cxld;
 	return 0;
 }
@@ -1925,6 +1933,29 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 		return -ENXIO;
 	}
 
+	/* Set the coherence of region to that of the first endpoint */
+	if (cxlr->coherence == CXL_DECODER_INVALIDCOH) {
+		unsigned long flags = cxlrd->cxlsd.cxld.flags;
+		enum cxl_decoder_coherence coherence = cxled->cxld.coherence;
+
+		cxlr->coherence = coherence;
+		if ((coherence == CXL_DECODER_HOSTONLYCOH &&
+		     !(flags & CXL_DECODER_F_HOSTONLYCOH)) ||
+		    (coherence == CXL_DECODER_DEVCOH &&
+		     !(flags & CXL_DECODER_F_DEVCOH))) {
+			dev_dbg(&cxlr->dev,
+"%s:%s endpoint coherence: %d isn't supported by root decoder: %#lx\n",
+				dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
+				coherence, flags);
+			return -ENXIO;
+		}
+	} else if (cxled->cxld.coherence != cxlr->coherence) {
+		dev_dbg(&cxlr->dev, "%s:%s coherence mismatch: %d vs %d\n",
+			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
+			cxled->cxld.coherence, cxlr->coherence);
+		return -ENXIO;
+	}
+
 	if (!cxled->dpa_res) {
 		dev_dbg(&cxlr->dev, "%s:%s: missing DPA allocation.\n",
 			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev));
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index f95101994238..1927a1849d82 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -328,6 +328,12 @@ enum cxl_decoder_type {
 	CXL_DECODER_EXPANDER = 3,
 };
 
+enum cxl_decoder_coherence {
+	CXL_DECODER_INVALIDCOH,
+	CXL_DECODER_HOSTONLYCOH,
+	CXL_DECODER_DEVCOH,
+};
+
 /*
  * Current specification goes up to 8, double that seems a reasonable
  * software max for the foreseeable future
@@ -344,6 +350,7 @@ enum cxl_decoder_type {
  * @interleave_ways: number of cxl_dports in this decode
  * @interleave_granularity: data stride per dport
  * @target_type: accelerator vs expander (type2 vs type3) selector
+ * @coherence: host only vs device coherence selector
  * @region: currently assigned region for this decoder
  * @flags: memory type capabilities and locking
  * @commit: device/decoder-type specific callback to commit settings to hw
@@ -356,6 +363,7 @@ struct cxl_decoder {
 	int interleave_ways;
 	int interleave_granularity;
 	enum cxl_decoder_type target_type;
+	enum cxl_decoder_coherence coherence;
 	struct cxl_region *region;
 	unsigned long flags;
 	int (*commit)(struct cxl_decoder *cxld);
@@ -517,6 +525,7 @@ struct cxl_region_params {
  * @id: This region's id. Id is globally unique across all regions
  * @mode: Endpoint decoder allocation / access mode
  * @type: Endpoint decoder target type
+ * @coherence: Endpoint decoder coherence
  * @cxl_nvb: nvdimm bridge for coordinating @cxlr_pmem setup / shutdown
  * @cxlr_pmem: (for pmem regions) cached copy of the nvdimm bridge
  * @flags: Region state flags
@@ -530,6 +539,7 @@ struct cxl_region {
 	int id;
 	enum cxl_decoder_mode mode;
 	enum cxl_decoder_type type;
+	enum cxl_decoder_coherence coherence;
 	struct cxl_nvdimm_bridge *cxl_nvb;
 	struct cxl_pmem_region *cxlr_pmem;
 	unsigned long flags;
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 2a25d1957ddb..f9156c578bde 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -394,6 +394,16 @@ enum cxl_devtype {
 	CXL_DEVTYPE_CLASSMEM,
 };
 
+/*
+ * enum cxl_devcoherence - the coherence of the cxl device
+ * @CXL_DEVCOH_DEV      - HDM-D (type 2) or HDM-DB (type 2/3)
+ * @CXL_DEVCOH_HOSTONLY - HDM-H (type 3)
+ */
+enum cxl_devcoherence {
+	CXL_DEVCOH_DEV,
+	CXL_DEVCOH_HOSTONLY,
+};
+
 /**
  * struct cxl_dpa_perf - DPA performance property entry
  * @dpa_range: range for DPA address
@@ -427,6 +437,7 @@ struct cxl_dpa_perf {
  * @ram_res: Active Volatile memory capacity configuration
  * @serial: PCIe Device Serial Number
  * @type: Generic Memory Class device or Vendor Specific Memory device
+ * @coherence: Device or Host only coherence
  * @cxl_mbox: CXL mailbox context
  */
 struct cxl_dev_state {
@@ -442,6 +453,7 @@ struct cxl_dev_state {
 	struct resource ram_res;
 	u64 serial;
 	enum cxl_devtype type;
+	enum cxl_devcoherence coherence;
 	struct cxl_mailbox cxl_mbox;
 };
 
-- 
2.39.2

Re: [PATCH 3/5] cxl: Separate coherence from target type

[Dan Williams]

Huang Ying wrote:
> Previously, target type (expander or accelerator) and
> coherence (HOSTONLY (HDM-H) or DEV (HDM-D/DB)) are synonym.  So
> current kernel uses target type to designate coherence too.  However,
> it's possible for expanders to use HDM-DB now.  So, the patch
> separates coherence from target type.
> 
> Accordingly, the patch sets the HOSTONLY field of decoder ctrl
> register (CXL_HDM_DECODER0_CTRL_HOSTONLY) according to the coherence
> instead of the target type.
> 
> Because we cannot determine the coherence of decoders via target type,
> the patch lets accelerator/expander device drivers specify coherence
> explicitly via newly added coherence field in struct cxl_dev_state.
> 
> The coherence of each end points in a region needs to be same.  So,
> the patch records the coherence of the first added end point in struct
> region.  Then, it checks whether the coherence of all other end points
> is same.

The target type is the coherence type, I am not sure what is motivating
this separation?

The decoder must match the mode of window it is joining because both
HDM-D and HDM-DB speak a different protocol than HDM.

[PATCH 4/5] cxl: Set type of region to that of the first endpoint

[Huang Ying]

Current kernel hard-codes the type of region to type 3 expander now,
because this is the only supported device type.  As a preparation to
support type 2 accelerators, the patch sets the type of region to that
of the first endpoint.  Then, the patch checks whether the type of
region is same as the type of other endpoints of the region.  Because
what we really need is to make sure the type of all endpoints of a
region is same.  And, the patch lets expander/accelerator device
drivers specify the target type of endpoint devices via struct
cxl_dev_state.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Alejandro Lucero <alucerop@amd.com>
Cc: Ben Cheatham <benjamin.cheatham@amd.com>
---
 drivers/cxl/acpi.c        |  1 -
 drivers/cxl/core/hdm.c    | 28 +++++++++++++---------------
 drivers/cxl/core/port.c   |  2 ++
 drivers/cxl/core/region.c | 13 +++++++------
 drivers/cxl/cxl.h         |  1 +
 5 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 21486e471305..29c2a44b122c 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -382,7 +382,6 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
 
 	cxld = &cxlrd->cxlsd.cxld;
 	cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions);
-	cxld->target_type = CXL_DECODER_EXPANDER;
 	cxld->hpa_range = (struct range) {
 		.start = cfmws->base_hpa,
 		.end = cfmws->base_hpa + cfmws->window_size - 1,
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 478fb6691759..c9accf8be71f 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -841,18 +841,25 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 		.end = base + size - 1,
 	};
 
+	if (cxled) {
+		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+		struct cxl_dev_state *cxlds = cxlmd->cxlds;
+
+		if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
+			cxld->target_type = CXL_DECODER_EXPANDER;
+		else
+			cxld->target_type = CXL_DECODER_ACCEL;
+	}
+
 	/* decoders are enabled if committed */
 	if (committed) {
 		cxld->flags |= CXL_DECODER_F_ENABLE;
 		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
 			cxld->flags |= CXL_DECODER_F_LOCK;
-		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl)) {
-			cxld->target_type = CXL_DECODER_EXPANDER;
+		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl))
 			cxld->coherence = CXL_DECODER_HOSTONLYCOH;
-		} else {
-			cxld->target_type = CXL_DECODER_ACCEL;
+		else
 			cxld->coherence = CXL_DECODER_DEVCOH;
-		}
 
 		guard(rwsem_write)(&cxl_region_rwsem);
 		if (cxld->id != cxl_num_decoders_committed(port)) {
@@ -874,21 +881,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 			struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 			struct cxl_dev_state *cxlds = cxlmd->cxlds;
 
-			/*
-			 * Default by devtype until a device arrives that needs
-			 * more precision.
-			 */
-			if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
-				cxld->target_type = CXL_DECODER_EXPANDER;
-			else
-				cxld->target_type = CXL_DECODER_ACCEL;
 			if (cxlds->coherence == CXL_DEVCOH_HOSTONLY)
 				cxld->coherence = CXL_DECODER_HOSTONLYCOH;
 			else
 				cxld->coherence = CXL_DECODER_DEVCOH;
 		} else {
-			/* To be overridden by region type/coherence at commit time */
-			cxld->target_type = CXL_DECODER_EXPANDER;
+			/* To be overridden by region coherence at commit time */
 			cxld->coherence = CXL_DECODER_HOSTONLYCOH;
 		}
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 9ebbffcea26a..d1bc6aed6509 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -139,6 +139,8 @@ static ssize_t target_type_show(struct device *dev,
 		return sysfs_emit(buf, "accelerator\n");
 	case CXL_DECODER_EXPANDER:
 		return sysfs_emit(buf, "expander\n");
+	default:
+		break;
 	}
 	return -ENXIO;
 }
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 21b877d8582f..d709738ada61 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 		return -ENXIO;
 	}
 
-	if (cxled->cxld.target_type != cxlr->type) {
+	/* Set the type of region to that of the first endpoint */
+	if (cxlr->type == CXL_DECODER_INVALID) {
+		cxlr->type = cxled->cxld.target_type;
+	} else if (cxled->cxld.target_type != cxlr->type) {
 		dev_dbg(&cxlr->dev, "%s:%s type mismatch: %d vs %d\n",
 			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
 			cxled->cxld.target_type, cxlr->type);
@@ -2476,7 +2479,6 @@ static int cxl_region_calculate_adistance(struct notifier_block *nb,
  * @cxlrd: root decoder
  * @id: memregion id to create, or memregion_free() on failure
  * @mode: mode for the endpoint decoders of this region
- * @type: select whether this is an expander or accelerator (type-2 or type-3)
  *
  * This is the second step of region initialization. Regions exist within an
  * address space which is mapped by a @cxlrd.
@@ -2486,8 +2488,7 @@ static int cxl_region_calculate_adistance(struct notifier_block *nb,
  */
 static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
 					      int id,
-					      enum cxl_decoder_mode mode,
-					      enum cxl_decoder_type type)
+					      enum cxl_decoder_mode mode)
 {
 	struct cxl_port *port = to_cxl_port(cxlrd->cxlsd.cxld.dev.parent);
 	struct cxl_region *cxlr;
@@ -2498,7 +2499,7 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
 	if (IS_ERR(cxlr))
 		return cxlr;
 	cxlr->mode = mode;
-	cxlr->type = type;
+	cxlr->type = CXL_DECODER_INVALID;
 
 	dev = &cxlr->dev;
 	rc = dev_set_name(dev, "region%d", id);
@@ -2562,7 +2563,7 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
 		return ERR_PTR(-EBUSY);
 	}
 
-	return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_EXPANDER);
+	return devm_cxl_add_region(cxlrd, id, mode);
 }
 
 static ssize_t create_pmem_region_store(struct device *dev,
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 1927a1849d82..f4cbe5c292ea 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -324,6 +324,7 @@ resource_size_t cxl_rcd_component_reg_phys(struct device *dev,
 #define CXL_DECODER_F_MASK        GENMASK(5, 0)
 
 enum cxl_decoder_type {
+	CXL_DECODER_INVALID = 0,
 	CXL_DECODER_ACCEL = 2,
 	CXL_DECODER_EXPANDER = 3,
 };
-- 
2.39.2

Re: [PATCH 4/5] cxl: Set type of region to that of the first endpoint

[Dan Williams]

Huang Ying wrote:
> Current kernel hard-codes the type of region to type 3 expander now,
> because this is the only supported device type.  As a preparation to
> support type 2 accelerators, the patch sets the type of region to that
> of the first endpoint.  Then, the patch checks whether the type of
> region is same as the type of other endpoints of the region.  Because
> what we really need is to make sure the type of all endpoints of a
> region is same.  And, the patch lets expander/accelerator device
> drivers specify the target type of endpoint devices via struct
> cxl_dev_state.
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Alejandro Lucero <alucerop@amd.com>
> Cc: Ben Cheatham <benjamin.cheatham@amd.com>
> ---
>  drivers/cxl/acpi.c        |  1 -
>  drivers/cxl/core/hdm.c    | 28 +++++++++++++---------------
>  drivers/cxl/core/port.c   |  2 ++
>  drivers/cxl/core/region.c | 13 +++++++------
>  drivers/cxl/cxl.h         |  1 +
>  5 files changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 21486e471305..29c2a44b122c 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -382,7 +382,6 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
>  
>  	cxld = &cxlrd->cxlsd.cxld;
>  	cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions);
> -	cxld->target_type = CXL_DECODER_EXPANDER;
>  	cxld->hpa_range = (struct range) {
>  		.start = cfmws->base_hpa,
>  		.end = cfmws->base_hpa + cfmws->window_size - 1,
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 478fb6691759..c9accf8be71f 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -841,18 +841,25 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  		.end = base + size - 1,
>  	};
>  
> +	if (cxled) {
> +		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +		struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +
> +		if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
> +			cxld->target_type = CXL_DECODER_EXPANDER;
> +		else
> +			cxld->target_type = CXL_DECODER_ACCEL;

This looks broken there is no way to know the target type of the decoder
from the cxl_dev_state. An "accelerator" can have HDM and an "expander"
can have HDM-DB.

> +	}
> +
>  	/* decoders are enabled if committed */
>  	if (committed) {
>  		cxld->flags |= CXL_DECODER_F_ENABLE;
>  		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
>  			cxld->flags |= CXL_DECODER_F_LOCK;
> -		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl)) {
> -			cxld->target_type = CXL_DECODER_EXPANDER;
> +		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl))
>  			cxld->coherence = CXL_DECODER_HOSTONLYCOH;
> -		} else {
> -			cxld->target_type = CXL_DECODER_ACCEL;
> +		else
>  			cxld->coherence = CXL_DECODER_DEVCOH;
> -		}
>  
>  		guard(rwsem_write)(&cxl_region_rwsem);
>  		if (cxld->id != cxl_num_decoders_committed(port)) {
> @@ -874,21 +881,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  			struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>  			struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  
> -			/*
> -			 * Default by devtype until a device arrives that needs
> -			 * more precision.
> -			 */
> -			if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
> -				cxld->target_type = CXL_DECODER_EXPANDER;
> -			else
> -				cxld->target_type = CXL_DECODER_ACCEL;
>  			if (cxlds->coherence == CXL_DEVCOH_HOSTONLY)
>  				cxld->coherence = CXL_DECODER_HOSTONLYCOH;
>  			else
>  				cxld->coherence = CXL_DECODER_DEVCOH;
>  		} else {
> -			/* To be overridden by region type/coherence at commit time */
> -			cxld->target_type = CXL_DECODER_EXPANDER;
> +			/* To be overridden by region coherence at commit time */
>  			cxld->coherence = CXL_DECODER_HOSTONLYCOH;
>  		}
>  
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 9ebbffcea26a..d1bc6aed6509 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -139,6 +139,8 @@ static ssize_t target_type_show(struct device *dev,
>  		return sysfs_emit(buf, "accelerator\n");
>  	case CXL_DECODER_EXPANDER:
>  		return sysfs_emit(buf, "expander\n");
> +	default:
> +		break;
>  	}
>  	return -ENXIO;
>  }
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 21b877d8582f..d709738ada61 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		return -ENXIO;
>  	}
>  
> -	if (cxled->cxld.target_type != cxlr->type) {
> +	/* Set the type of region to that of the first endpoint */
> +	if (cxlr->type == CXL_DECODER_INVALID) {
> +		cxlr->type = cxled->cxld.target_type;
> +	} else if (cxled->cxld.target_type != cxlr->type) {

No, the type of the region is determined by the caller and should be
gated by the region capability. For type-2 region creation I doubt
userspace is going to be creating those vs the accelerator so this all
seems backwards to me.

Re: [PATCH 4/5] cxl: Set type of region to that of the first endpoint

[Huang, Ying]

Dan Williams <dan.j.williams@intel.com> writes:

> Huang Ying wrote:

[snip]

>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 21b877d8582f..d709738ada61 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>>  		return -ENXIO;
>>  	}
>>  
>> -	if (cxled->cxld.target_type != cxlr->type) {
>> +	/* Set the type of region to that of the first endpoint */
>> +	if (cxlr->type == CXL_DECODER_INVALID) {
>> +		cxlr->type = cxled->cxld.target_type;
>> +	} else if (cxled->cxld.target_type != cxlr->type) {
>
> No, the type of the region is determined by the caller and should be
> gated by the region capability. For type-2 region creation I doubt
> userspace is going to be creating those vs the accelerator so this all
> seems backwards to me.

How do we determine the type of the endpoint?  Specify it in type2/type3
device driver?

If so, we will specify the type of both the endpoint and the region in
type2/type3 device driver.  Then, why not only specify the type of the
endpoint?  The type of region can be determined from the type of the
endpoint.

--
Best Regards,
Huang, Ying

Re: [PATCH 4/5] cxl: Set type of region to that of the first endpoint

[Dan Williams]

Huang, Ying wrote:
> Dan Williams <dan.j.williams@intel.com> writes:
> 
> > Huang Ying wrote:
> 
> [snip]
> 
> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> >> index 21b877d8582f..d709738ada61 100644
> >> --- a/drivers/cxl/core/region.c
> >> +++ b/drivers/cxl/core/region.c
> >> @@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> >>  		return -ENXIO;
> >>  	}
> >>  
> >> -	if (cxled->cxld.target_type != cxlr->type) {
> >> +	/* Set the type of region to that of the first endpoint */
> >> +	if (cxlr->type == CXL_DECODER_INVALID) {
> >> +		cxlr->type = cxled->cxld.target_type;
> >> +	} else if (cxled->cxld.target_type != cxlr->type) {
> >
> > No, the type of the region is determined by the caller and should be
> > gated by the region capability. For type-2 region creation I doubt
> > userspace is going to be creating those vs the accelerator so this all
> > seems backwards to me.
> 
> How do we determine the type of the endpoint?  Specify it in type2/type3
> device driver?

Why does the endpoint type matter? Memory expansion can be supported by
HDM-D[B], and an accelerator could have one or more HDM-H decoders.

> If so, we will specify the type of both the endpoint and the region in
> type2/type3 device driver.  Then, why not only specify the type of the
> endpoint?  The type of region can be determined from the type of the
> endpoint.

Because CXL HDM protocol is per decoder not per device.

Re: [PATCH 4/5] cxl: Set type of region to that of the first endpoint

[Huang, Ying]

Dan Williams <dan.j.williams@intel.com> writes:

> Huang, Ying wrote:
>> Dan Williams <dan.j.williams@intel.com> writes:
>> 
>> > Huang Ying wrote:
>> 
>> [snip]
>> 
>> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> >> index 21b877d8582f..d709738ada61 100644
>> >> --- a/drivers/cxl/core/region.c
>> >> +++ b/drivers/cxl/core/region.c
>> >> @@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>> >>  		return -ENXIO;
>> >>  	}
>> >>  
>> >> -	if (cxled->cxld.target_type != cxlr->type) {
>> >> +	/* Set the type of region to that of the first endpoint */
>> >> +	if (cxlr->type == CXL_DECODER_INVALID) {
>> >> +		cxlr->type = cxled->cxld.target_type;
>> >> +	} else if (cxled->cxld.target_type != cxlr->type) {
>> >
>> > No, the type of the region is determined by the caller and should be
>> > gated by the region capability. For type-2 region creation I doubt
>> > userspace is going to be creating those vs the accelerator so this all
>> > seems backwards to me.
>> 
>> How do we determine the type of the endpoint?  Specify it in type2/type3
>> device driver?
>
> Why does the endpoint type matter? Memory expansion can be supported by
> HDM-D[B], and an accelerator could have one or more HDM-H decoders.
>
>> If so, we will specify the type of both the endpoint and the region in
>> type2/type3 device driver.  Then, why not only specify the type of the
>> endpoint?  The type of region can be determined from the type of the
>> endpoint.
>
> Because CXL HDM protocol is per decoder not per device.

Sorry for confusing.  When I said endpoint, I wanted to say "endpoint
decoder" actually.  IIUC, the coherence type of region should be same as
that of all endpoint decoders participating the region.  If we specify
the coherence type of the endpoint decoders, the coherence type of the
region should be same, so we don't need to specify it again?  We need to
check root decoder capability as you pointed out.

Best Regards,
Huang, Ying

Re: [PATCH 4/5] cxl: Set type of region to that of the first endpoint

[Alejandro Lucero Palau]

On 10/17/24 23:33, Dan Williams wrote:
> Huang Ying wrote:
>> Current kernel hard-codes the type of region to type 3 expander now,
>> because this is the only supported device type.  As a preparation to
>> support type 2 accelerators, the patch sets the type of region to that
>> of the first endpoint.  Then, the patch checks whether the type of
>> region is same as the type of other endpoints of the region.  Because
>> what we really need is to make sure the type of all endpoints of a
>> region is same.  And, the patch lets expander/accelerator device
>> drivers specify the target type of endpoint devices via struct
>> cxl_dev_state.
>>
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Reviewed-by: Gregory Price <gourry@gourry.net>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Cc: Dave Jiang <dave.jiang@intel.com>
>> Cc: Alison Schofield <alison.schofield@intel.com>
>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>> Cc: Alejandro Lucero <alucerop@amd.com>
>> Cc: Ben Cheatham <benjamin.cheatham@amd.com>
>> ---
>>   drivers/cxl/acpi.c        |  1 -
>>   drivers/cxl/core/hdm.c    | 28 +++++++++++++---------------
>>   drivers/cxl/core/port.c   |  2 ++
>>   drivers/cxl/core/region.c | 13 +++++++------
>>   drivers/cxl/cxl.h         |  1 +
>>   5 files changed, 23 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
>> index 21486e471305..29c2a44b122c 100644
>> --- a/drivers/cxl/acpi.c
>> +++ b/drivers/cxl/acpi.c
>> @@ -382,7 +382,6 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
>>   
>>   	cxld = &cxlrd->cxlsd.cxld;
>>   	cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions);
>> -	cxld->target_type = CXL_DECODER_EXPANDER;
>>   	cxld->hpa_range = (struct range) {
>>   		.start = cfmws->base_hpa,
>>   		.end = cfmws->base_hpa + cfmws->window_size - 1,
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 478fb6691759..c9accf8be71f 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -841,18 +841,25 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>   		.end = base + size - 1,
>>   	};
>>   
>> +	if (cxled) {
>> +		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>> +		struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +
>> +		if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
>> +			cxld->target_type = CXL_DECODER_EXPANDER;
>> +		else
>> +			cxld->target_type = CXL_DECODER_ACCEL;
> This looks broken there is no way to know the target type of the decoder
> from the cxl_dev_state. An "accelerator" can have HDM and an "expander"
> can have HDM-DB.
>
>> +	}
>> +
>>   	/* decoders are enabled if committed */
>>   	if (committed) {
>>   		cxld->flags |= CXL_DECODER_F_ENABLE;
>>   		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
>>   			cxld->flags |= CXL_DECODER_F_LOCK;
>> -		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl)) {
>> -			cxld->target_type = CXL_DECODER_EXPANDER;
>> +		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl))
>>   			cxld->coherence = CXL_DECODER_HOSTONLYCOH;
>> -		} else {
>> -			cxld->target_type = CXL_DECODER_ACCEL;
>> +		else
>>   			cxld->coherence = CXL_DECODER_DEVCOH;
>> -		}
>>   
>>   		guard(rwsem_write)(&cxl_region_rwsem);
>>   		if (cxld->id != cxl_num_decoders_committed(port)) {
>> @@ -874,21 +881,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>   			struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>>   			struct cxl_dev_state *cxlds = cxlmd->cxlds;
>>   
>> -			/*
>> -			 * Default by devtype until a device arrives that needs
>> -			 * more precision.
>> -			 */
>> -			if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
>> -				cxld->target_type = CXL_DECODER_EXPANDER;
>> -			else
>> -				cxld->target_type = CXL_DECODER_ACCEL;
>>   			if (cxlds->coherence == CXL_DEVCOH_HOSTONLY)
>>   				cxld->coherence = CXL_DECODER_HOSTONLYCOH;
>>   			else
>>   				cxld->coherence = CXL_DECODER_DEVCOH;
>>   		} else {
>> -			/* To be overridden by region type/coherence at commit time */
>> -			cxld->target_type = CXL_DECODER_EXPANDER;
>> +			/* To be overridden by region coherence at commit time */
>>   			cxld->coherence = CXL_DECODER_HOSTONLYCOH;
>>   		}
>>   
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 9ebbffcea26a..d1bc6aed6509 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -139,6 +139,8 @@ static ssize_t target_type_show(struct device *dev,
>>   		return sysfs_emit(buf, "accelerator\n");
>>   	case CXL_DECODER_EXPANDER:
>>   		return sysfs_emit(buf, "expander\n");
>> +	default:
>> +		break;
>>   	}
>>   	return -ENXIO;
>>   }
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 21b877d8582f..d709738ada61 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>>   		return -ENXIO;
>>   	}
>>   
>> -	if (cxled->cxld.target_type != cxlr->type) {
>> +	/* Set the type of region to that of the first endpoint */
>> +	if (cxlr->type == CXL_DECODER_INVALID) {
>> +		cxlr->type = cxled->cxld.target_type;
>> +	} else if (cxled->cxld.target_type != cxlr->type) {
> No, the type of the region is determined by the caller and should be
> gated by the region capability. For type-2 region creation I doubt
> userspace is going to be creating those vs the accelerator so this all
> seems backwards to me.


FWIW, path 19 of last type support patchset does add the type to be set 
at region creation time by the caller:

https://lore.kernel.org/linux-cxl/4ce8cc04-71fd-424a-9831-86f89fcd7d2f@amd.com/T/#m420ea86f7b9193519e3226c377612ad3ea546633

[PATCH 5/5] cxl: Avoid to create dax regions for type2 accelerators

[Huang Ying]

The memory range of a type2 accelerator should be managed by the type2
accelerator specific driver instead of the common dax region drivers,
as discussed in [1].

[1] https://lore.kernel.org/linux-cxl/66469ff1b8fbc_2c2629427@dwillia2-xfh.jf.intel.com.notmuch/

So, the patch skips dax regions creation for type2 accelerator device
memory regions.

Based on: https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Co-developed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Alejandro Lucero <alucerop@amd.com>
Cc: Ben Cheatham <benjamin.cheatham@amd.com>
---
 drivers/cxl/core/region.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index d709738ada61..708be236c9a2 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3473,6 +3473,14 @@ static int cxl_region_probe(struct device *dev)
 					p->res->start, p->res->end, cxlr,
 					is_system_ram) > 0)
 			return 0;
+		/*
+		 * Accelerator regions have specific usage, skip
+		 * device-dax registration.
+		 */
+		if (cxlr->type == CXL_DECODER_ACCEL)
+			return 0;
+
+		/* Expander routes to device-dax */
 		return devm_cxl_add_dax_region(cxlr);
 	default:
 		dev_dbg(&cxlr->dev, "unsupported region mode: %d\n",
-- 
2.39.2

Re: [PATCH 5/5] cxl: Avoid to create dax regions for type2 accelerators

[Alejandro Lucero Palau]

I did comment on this some time ago and I'm doing it again.


This is originally part of the type2 patchset, and I'm keeping it in V4. 
I do not understand why you pick code changes (you explicitly said that 
in the first RFC) from there and use it here, and without previous 
discussion about this necessity in the list. I do not think this is 
usual, at least in other kernel subsystems I'm more familiar with, so I 
will raise this in today's cxl open source collaboration sync.


On 10/15/24 07:57, Huang Ying wrote:
> The memory range of a type2 accelerator should be managed by the type2
> accelerator specific driver instead of the common dax region drivers,
> as discussed in [1].
>
> [1] https://lore.kernel.org/linux-cxl/66469ff1b8fbc_2c2629427@dwillia2-xfh.jf.intel.com.notmuch/
>
> So, the patch skips dax regions creation for type2 accelerator device
> memory regions.
>
> Based on: https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
>
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Alejandro Lucero <alucerop@amd.com>
> Cc: Ben Cheatham <benjamin.cheatham@amd.com>
> ---
>   drivers/cxl/core/region.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index d709738ada61..708be236c9a2 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3473,6 +3473,14 @@ static int cxl_region_probe(struct device *dev)
>   					p->res->start, p->res->end, cxlr,
>   					is_system_ram) > 0)
>   			return 0;
> +		/*
> +		 * Accelerator regions have specific usage, skip
> +		 * device-dax registration.
> +		 */
> +		if (cxlr->type == CXL_DECODER_ACCEL)
> +			return 0;
> +
> +		/* Expander routes to device-dax */
>   		return devm_cxl_add_dax_region(cxlr);
>   	default:
>   		dev_dbg(&cxlr->dev, "unsupported region mode: %d\n",

Re: [PATCH 5/5] cxl: Avoid to create dax regions for type2 accelerators

[Huang, Ying]

Hi, Alejandro,

Alejandro Lucero Palau <alucerop@amd.com> writes:

> I did comment on this some time ago and I'm doing it again.
>
>
> This is originally part of the type2 patchset, and I'm keeping it in
> V4. I do not understand why you pick code changes (you explicitly said
> that in the first RFC) from there and use it here, and without
> previous discussion about this necessity in the list. I do not think
> this is usual, at least in other kernel subsystems I'm more familiar
> with, so I will raise this in today's cxl open source collaboration
> sync.

No.  I picked this change from Dan's series as follows,

https://eclists.intel.com/sympa//arc/linux-bkc/2024-10/msg00018.html

So, I added co-developed-by and signed-off-by of Dan.

IIUC, your picked this change from Dan's series too?

Feel free to include this change in your series.  If your patchset is
merged firstly, I will rebase on yours and drop this change.

[snip]

--
Best Regards,
Huang, Ying

Re: [PATCH 5/5] cxl: Avoid to create dax regions for type2 accelerators

[Alejandro Lucero Palau]

On 10/17/24 07:29, Huang, Ying wrote:
> Hi, Alejandro,
>
> Alejandro Lucero Palau <alucerop@amd.com> writes:
>
>> I did comment on this some time ago and I'm doing it again.
>>
>>
>> This is originally part of the type2 patchset, and I'm keeping it in
>> V4. I do not understand why you pick code changes (you explicitly said
>> that in the first RFC) from there and use it here, and without
>> previous discussion about this necessity in the list. I do not think
>> this is usual, at least in other kernel subsystems I'm more familiar
>> with, so I will raise this in today's cxl open source collaboration
>> sync.
> No.  I picked this change from Dan's series as follows,
>
> https://eclists.intel.com/sympa//arc/linux-bkc/2024-10/msg00018.html
>
> So, I added co-developed-by and signed-off-by of Dan.
>
> IIUC, your picked this change from Dan's series too?


Look, this is not going well.


You specifically said in your first patchset you considered the type2 
support patchset complete but too large or complex, so you were taking 
parts of it as a prelude for making it easier to review/accept. Just 
face that and not twist the argument.


FWIW, I'm against you doing so because:


1) You should have commented in the type2 patchset about your concern, 
and gave advice about doing such a prelude (by me) or offer yourself for 
doing it.

2) Just following your approach, anyone could do the same for any 
patchset sent to the list. This is not a good precedent.

3) If this is going to be allowed/approved, I'm not going to be 
comfortable within this community. If it is just me, I guess it will not 
be a big loss.


None has commented yet except you and me, what I do not know if it is 
because this is a nasty discussion they do not want to get entangle 
with, or because they just think your approach is OK. If not further 
comment and your patchset is accepted, nothing else will be needed to say.


> Feel free to include this change in your series.  If your patchset is
> merged firstly, I will rebase on yours and drop this change.
>
> [snip]
>
> --
> Best Regards,
> Huang, Ying

Re: [PATCH 5/5] cxl: Avoid to create dax regions for type2 accelerators

[Huang, Ying]

Alejandro Lucero Palau <alucerop@amd.com> writes:

> On 10/17/24 07:29, Huang, Ying wrote:
>> Hi, Alejandro,
>>
>> Alejandro Lucero Palau <alucerop@amd.com> writes:
>>
>>> I did comment on this some time ago and I'm doing it again.
>>>
>>>
>>> This is originally part of the type2 patchset, and I'm keeping it in
>>> V4. I do not understand why you pick code changes (you explicitly said
>>> that in the first RFC) from there and use it here, and without
>>> previous discussion about this necessity in the list. I do not think
>>> this is usual, at least in other kernel subsystems I'm more familiar
>>> with, so I will raise this in today's cxl open source collaboration
>>> sync.
>> No.  I picked this change from Dan's series as follows,
>>
>> https://eclists.intel.com/sympa//arc/linux-bkc/2024-10/msg00018.html
>>
>> So, I added co-developed-by and signed-off-by of Dan.
>>
>> IIUC, your picked this change from Dan's series too?
>
>
> Look, this is not going well.
>
>
> You specifically said in your first patchset you considered the type2
> support patchset complete but too large or complex, so you were taking
> parts of it as a prelude for making it easier to review/accept. Just
> face that and not twist the argument.

Although I listed your patchset in my cover letter.  All changes I
picked was from Dan's patchset instead of yours.  And, I kept Dan's
co-developed-by and signed-off-by.  If you will pick changes from Dan,
please do that too.

> FWIW, I'm against you doing so because:
>
>
> 1) You should have commented in the type2 patchset about your concern,
> and gave advice about doing such a prelude (by me) or offer yourself
> for doing it.
>
> 2) Just following your approach, anyone could do the same for any
> patchset sent to the list. This is not a good precedent.
>
> 3) If this is going to be allowed/approved, I'm not going to be
> comfortable within this community. If it is just me, I guess it will
> not be a big loss.
>
>
> None has commented yet except you and me, what I do not know if it is
> because this is a nasty discussion they do not want to get entangle
> with, or because they just think your approach is OK. If not further
> comment and your patchset is accepted, nothing else will be needed to
> say.
>
>
>> Feel free to include this change in your series.  If your patchset is
>> merged firstly, I will rebase on yours and drop this change.
>>
>> [snip]

--
Best Regards,
Huang, Ying

Re: [PATCH 5/5] cxl: Avoid to create dax regions for type2 accelerators

[Jonathan Cameron]

On Thu, 17 Oct 2024 15:48:04 +0800
"Huang, Ying" <ying.huang@intel.com> wrote:

> Alejandro Lucero Palau <alucerop@amd.com> writes:
> 
> > On 10/17/24 07:29, Huang, Ying wrote:  
> >> Hi, Alejandro,
> >>
> >> Alejandro Lucero Palau <alucerop@amd.com> writes:
> >>  
> >>> I did comment on this some time ago and I'm doing it again.
> >>>
> >>>
> >>> This is originally part of the type2 patchset, and I'm keeping it in
> >>> V4. I do not understand why you pick code changes (you explicitly said
> >>> that in the first RFC) from there and use it here, and without
> >>> previous discussion about this necessity in the list. I do not think
> >>> this is usual, at least in other kernel subsystems I'm more familiar
> >>> with, so I will raise this in today's cxl open source collaboration
> >>> sync.  
> >> No.  I picked this change from Dan's series as follows,
> >>
> >> https://eclists.intel.com/sympa//arc/linux-bkc/2024-10/msg00018.html
> >>
> >> So, I added co-developed-by and signed-off-by of Dan.
> >>
> >> IIUC, your picked this change from Dan's series too?  
> >
> >
> > Look, this is not going well.

Hi Alejandro + Huang, Ying

This seems to be an unfortunate case of disconnected work on the same
large problem and shows the need for more coordination.
Note these are my personal responses to this, other maintainers and
community members may well disagree!

Alejandro had already clearly adopted the patches from Dan and taken a
number of them forwards as part of his patch set.  That had
happened before Huang, Ying posted the RFC (which referenced
Alejandro's work along side Dan's original series).

The idea of trying to accelerate the process of upstreaming type 2
support by merging a few low hanging fruit is certainly one I think
we can all get behind.  However, it needs some coordination to avoid
actually slowing down overall progress by both causing spread out
reviews and divergence in direction + churn in the base on which
the fuller sets are built. So Huang, Ying please work with Alejandro
rather than continuing to evolve this set independently.

Perhaps an initial step would be to figure out how to reorder Alejandro's
series so that any work duplicated by this set is pulled to the front.
That should make it easy to identify and discuss differences that
have resulted from review of this series. At that point we can focus
the review on those patches as the rest of the set continues to evolve.
However, I would  strongly suggest coordinating that work in order to
avoid churning the code when Alejandro may be near to posting a new
version of his fuller series.

Whilst the precise way we have ended up with two sets changing the same
code is unusual it is extremely common for multiple people to be working
on the same code and coordination to be needed.  Many of the regular
sync calls / discord channels / irc etc are used to figure out how
this can be done efficiently.  Please use those channels here.

If it would be useful to have an additional call or similar to ensure
a fruitful collaboration then we can set one up.

Finally I'll note that I'd have expected to see explicit discussion of
how this series relates to Alejandro's set and a suggestion of how to
move forwards in the cover letter and that would perhaps have either
resolved Alejandro's concerns or at least publicly shown awareness of the
issues this would cause for his work.

Irrespective of the other reasons for such an intro text, whilst the
CXL maintainers were at least somewhat aware, we always appreciate
a reminder in a cover letter!

Jonathan


> >
> >
> > You specifically said in your first patchset you considered the type2
> > support patchset complete but too large or complex, so you were taking
> > parts of it as a prelude for making it easier to review/accept. Just
> > face that and not twist the argument.  
> 
> Although I listed your patchset in my cover letter.  All changes I
> picked was from Dan's patchset instead of yours.  And, I kept Dan's
> co-developed-by and signed-off-by.  If you will pick changes from Dan,
> please do that too.
> 
> > FWIW, I'm against you doing so because:
> >
> >
> > 1) You should have commented in the type2 patchset about your concern,
> > and gave advice about doing such a prelude (by me) or offer yourself
> > for doing it.
> >
> > 2) Just following your approach, anyone could do the same for any
> > patchset sent to the list. This is not a good precedent.
> >
> > 3) If this is going to be allowed/approved, I'm not going to be
> > comfortable within this community. If it is just me, I guess it will
> > not be a big loss.
> >
> >
> > None has commented yet except you and me, what I do not know if it is
> > because this is a nasty discussion they do not want to get entangle
> > with, or because they just think your approach is OK. If not further
> > comment and your patchset is accepted, nothing else will be needed to
> > say.
> >
> >  
> >> Feel free to include this change in your series.  If your patchset is
> >> merged firstly, I will rebase on yours and drop this change.
> >>
> >> [snip]  
> 
> --
> Best Regards,
> Huang, Ying
> 

Re: [PATCH 5/5] cxl: Avoid to create dax regions for type2 accelerators

[Huang, Ying]

Hi, Jonathan,

Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:

> On Thu, 17 Oct 2024 15:48:04 +0800
> "Huang, Ying" <ying.huang@intel.com> wrote:
>
>> Alejandro Lucero Palau <alucerop@amd.com> writes:
>> 
>> > On 10/17/24 07:29, Huang, Ying wrote:  
>> >> Hi, Alejandro,
>> >>
>> >> Alejandro Lucero Palau <alucerop@amd.com> writes:
>> >>  
>> >>> I did comment on this some time ago and I'm doing it again.
>> >>>
>> >>>
>> >>> This is originally part of the type2 patchset, and I'm keeping it in
>> >>> V4. I do not understand why you pick code changes (you explicitly said
>> >>> that in the first RFC) from there and use it here, and without
>> >>> previous discussion about this necessity in the list. I do not think
>> >>> this is usual, at least in other kernel subsystems I'm more familiar
>> >>> with, so I will raise this in today's cxl open source collaboration
>> >>> sync.  
>> >> No.  I picked this change from Dan's series as follows,
>> >>
>> >> https://eclists.intel.com/sympa//arc/linux-bkc/2024-10/msg00018.html
>> >>
>> >> So, I added co-developed-by and signed-off-by of Dan.
>> >>
>> >> IIUC, your picked this change from Dan's series too?  
>> >
>> >
>> > Look, this is not going well.
>
> Hi Alejandro + Huang, Ying
>
> This seems to be an unfortunate case of disconnected work on the same
> large problem and shows the need for more coordination.
> Note these are my personal responses to this, other maintainers and
> community members may well disagree!
>
> Alejandro had already clearly adopted the patches from Dan and taken a
> number of them forwards as part of his patch set.  That had
> happened before Huang, Ying posted the RFC (which referenced
> Alejandro's work along side Dan's original series).
>
> The idea of trying to accelerate the process of upstreaming type 2
> support by merging a few low hanging fruit is certainly one I think
> we can all get behind.  However, it needs some coordination to avoid
> actually slowing down overall progress by both causing spread out
> reviews and divergence in direction + churn in the base on which
> the fuller sets are built. So Huang, Ying please work with Alejandro
> rather than continuing to evolve this set independently.

IMHO, it may be better to continue to review this small series and make
it ready to be merged (or dropped) ASAP?  I can put it as a high
priority task.  After all, this series begins with code or ideas from
Dan.

However, this is only my personal opinion, if community thinks that it
isn't a good idea, I can drop this series, at least the part that
duplicates with Alejandro's series.

> Perhaps an initial step would be to figure out how to reorder Alejandro's
> series so that any work duplicated by this set is pulled to the front.
> That should make it easy to identify and discuss differences that
> have resulted from review of this series. At that point we can focus
> the review on those patches as the rest of the set continues to evolve.
> However, I would  strongly suggest coordinating that work in order to
> avoid churning the code when Alejandro may be near to posting a new
> version of his fuller series.
> 
> Whilst the precise way we have ended up with two sets changing the same
> code is unusual it is extremely common for multiple people to be working
> on the same code and coordination to be needed.  Many of the regular
> sync calls / discord channels / irc etc are used to figure out how
> this can be done efficiently.  Please use those channels here.

Yes.  More coordination is good.

> If it would be useful to have an additional call or similar to ensure
> a fruitful collaboration then we can set one up.
>
> Finally I'll note that I'd have expected to see explicit discussion of
> how this series relates to Alejandro's set and a suggestion of how to
> move forwards in the cover letter and that would perhaps have either
> resolved Alejandro's concerns or at least publicly shown awareness of the
> issues this would cause for his work.
>
> Irrespective of the other reasons for such an intro text, whilst the
> CXL maintainers were at least somewhat aware, we always appreciate
> a reminder in a cover letter!
>
> Jonathan
>
>
>> >
>> >
>> > You specifically said in your first patchset you considered the type2
>> > support patchset complete but too large or complex, so you were taking
>> > parts of it as a prelude for making it easier to review/accept. Just
>> > face that and not twist the argument.  
>> 
>> Although I listed your patchset in my cover letter.  All changes I
>> picked was from Dan's patchset instead of yours.  And, I kept Dan's
>> co-developed-by and signed-off-by.  If you will pick changes from Dan,
>> please do that too.
>> 
>> > FWIW, I'm against you doing so because:
>> >
>> >
>> > 1) You should have commented in the type2 patchset about your concern,
>> > and gave advice about doing such a prelude (by me) or offer yourself
>> > for doing it.
>> >
>> > 2) Just following your approach, anyone could do the same for any
>> > patchset sent to the list. This is not a good precedent.
>> >
>> > 3) If this is going to be allowed/approved, I'm not going to be
>> > comfortable within this community. If it is just me, I guess it will
>> > not be a big loss.
>> >
>> >
>> > None has commented yet except you and me, what I do not know if it is
>> > because this is a nasty discussion they do not want to get entangle
>> > with, or because they just think your approach is OK. If not further
>> > comment and your patchset is accepted, nothing else will be needed to
>> > say.
>> >
>> >  
>> >> Feel free to include this change in your series.  If your patchset is
>> >> merged firstly, I will rebase on yours and drop this change.
>> >>
>> >> [snip]  
>> 

--
Best Regards,
Huang, Ying

Re: [PATCH 5/5] cxl: Avoid to create dax regions for type2 accelerators

[Dan Williams]

Huang Ying wrote:
> The memory range of a type2 accelerator should be managed by the type2
> accelerator specific driver instead of the common dax region drivers,
> as discussed in [1].
> 
> [1] https://lore.kernel.org/linux-cxl/66469ff1b8fbc_2c2629427@dwillia2-xfh.jf.intel.com.notmuch/
> 
> So, the patch skips dax regions creation for type2 accelerator device
> memory regions.
> 
> Based on: https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Alejandro Lucero <alucerop@amd.com>
> Cc: Ben Cheatham <benjamin.cheatham@amd.com>
> ---
>  drivers/cxl/core/region.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index d709738ada61..708be236c9a2 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3473,6 +3473,14 @@ static int cxl_region_probe(struct device *dev)
>  					p->res->start, p->res->end, cxlr,
>  					is_system_ram) > 0)
>  			return 0;
> +		/*
> +		 * Accelerator regions have specific usage, skip
> +		 * device-dax registration.
> +		 */
> +		if (cxlr->type == CXL_DECODER_ACCEL)
> +			return 0;
> +
> +		/* Expander routes to device-dax */

So Linux is this weird career choice where you get to argue with
yourself months later. I understand why I took this shortcut of assuming
that the coherence mode determines device-dax routing, but that is not
sufficient.

An HDM-DB region could want the device-dax uAPI, and an HDM-H range
could want to do its own uAPI besides device-dax.

So, in retrospect, I think it is a mistake to assume uAPI from coherence
mode. It really is a property of the region decided by the region
creator independent of the coherence mode or the device type.

I am thinking that 'struct cxl_region' grows something like:

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 5406e3ab3d4a..4cf1d030404d 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -511,12 +511,25 @@ struct cxl_region_params {
  */
 #define CXL_REGION_F_NEEDS_RESET 1
 
+/*
+ * enum cxl_mem_api - where to route this cxl region
+ * @CXL_MEM_API_DAX: application specific / soft-reserved memory
+ * @CXL_MEM_API_PMEM: direct region to the NVDIMM subsystem
+ * @CXL_MEM_API_NONE: accelerator specific / hard-reserved memory
+ */
+enum cxl_mem_api {
+	CXL_MEM_API_DAX,
+	CXL_MEM_API_PMEM,
+	CXL_MEM_API_NONE,
+};
+
 /**
  * struct cxl_region - CXL region
  * @dev: This region's device
  * @id: This region's id. Id is globally unique across all regions
  * @mode: Endpoint decoder allocation / access mode
  * @type: Endpoint decoder target type
+ * @api: What if any subsystem will present this region to consumers
  * @cxl_nvb: nvdimm bridge for coordinating @cxlr_pmem setup / shutdown
  * @cxlr_pmem: (for pmem regions) cached copy of the nvdimm bridge
  * @flags: Region state flags
@@ -530,6 +543,7 @@ struct cxl_region {
 	int id;
 	enum cxl_decoder_mode mode;
 	enum cxl_decoder_type type;
+	enum cxl_mem_api api;
 	struct cxl_nvdimm_bridge *cxl_nvb;
 	struct cxl_pmem_region *cxlr_pmem;
 	unsigned long flags;

Now, I have not seen how Alejandro's series handles this, but this
type-2 series was shorter so I started here first.

Re: [PATCH 5/5] cxl: Avoid to create dax regions for type2 accelerators

[Huang, Ying]

Dan Williams <dan.j.williams@intel.com> writes:

> Huang Ying wrote:
>> The memory range of a type2 accelerator should be managed by the type2
>> accelerator specific driver instead of the common dax region drivers,
>> as discussed in [1].
>> 
>> [1] https://lore.kernel.org/linux-cxl/66469ff1b8fbc_2c2629427@dwillia2-xfh.jf.intel.com.notmuch/
>> 
>> So, the patch skips dax regions creation for type2 accelerator device
>> memory regions.
>> 
>> Based on: https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
>> 
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> Reviewed-by: Gregory Price <gourry@gourry.net>
>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Cc: Dave Jiang <dave.jiang@intel.com>
>> Cc: Alison Schofield <alison.schofield@intel.com>
>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>> Cc: Alejandro Lucero <alucerop@amd.com>
>> Cc: Ben Cheatham <benjamin.cheatham@amd.com>
>> ---
>>  drivers/cxl/core/region.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>> 
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index d709738ada61..708be236c9a2 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -3473,6 +3473,14 @@ static int cxl_region_probe(struct device *dev)
>>  					p->res->start, p->res->end, cxlr,
>>  					is_system_ram) > 0)
>>  			return 0;
>> +		/*
>> +		 * Accelerator regions have specific usage, skip
>> +		 * device-dax registration.
>> +		 */
>> +		if (cxlr->type == CXL_DECODER_ACCEL)
>> +			return 0;
>> +
>> +		/* Expander routes to device-dax */
>
> So Linux is this weird career choice where you get to argue with
> yourself months later. I understand why I took this shortcut of assuming
> that the coherence mode determines device-dax routing, but that is not
> sufficient.
>
> An HDM-DB region could want the device-dax uAPI, and an HDM-H range
> could want to do its own uAPI besides device-dax.
>
> So, in retrospect, I think it is a mistake to assume uAPI from coherence
> mode. It really is a property of the region decided by the region
> creator independent of the coherence mode or the device type.

Yes.  It isn't a good idea to determine the region usage based on coherence
mode.  In this series, clxr->type is used to designate
expander/accelerator instead of hostonly/dev, it makes more sense to
create device-dax for expander only by default.

> I am thinking that 'struct cxl_region' grows something like:
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 5406e3ab3d4a..4cf1d030404d 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -511,12 +511,25 @@ struct cxl_region_params {
>   */
>  #define CXL_REGION_F_NEEDS_RESET 1
>  
> +/*
> + * enum cxl_mem_api - where to route this cxl region
> + * @CXL_MEM_API_DAX: application specific / soft-reserved memory
> + * @CXL_MEM_API_PMEM: direct region to the NVDIMM subsystem
> + * @CXL_MEM_API_NONE: accelerator specific / hard-reserved memory
> + */
> +enum cxl_mem_api {
> +	CXL_MEM_API_DAX,
> +	CXL_MEM_API_PMEM,
> +	CXL_MEM_API_NONE,
> +};
> +
>  /**
>   * struct cxl_region - CXL region
>   * @dev: This region's device
>   * @id: This region's id. Id is globally unique across all regions
>   * @mode: Endpoint decoder allocation / access mode
>   * @type: Endpoint decoder target type
> + * @api: What if any subsystem will present this region to consumers
>   * @cxl_nvb: nvdimm bridge for coordinating @cxlr_pmem setup / shutdown
>   * @cxlr_pmem: (for pmem regions) cached copy of the nvdimm bridge
>   * @flags: Region state flags
> @@ -530,6 +543,7 @@ struct cxl_region {
>  	int id;
>  	enum cxl_decoder_mode mode;
>  	enum cxl_decoder_type type;
> +	enum cxl_mem_api api;
>  	struct cxl_nvdimm_bridge *cxl_nvb;
>  	struct cxl_pmem_region *cxlr_pmem;
>  	unsigned long flags;

This looks good to me.  The usage is specified by the device driver explicitly.

> Now, I have not seen how Alejandro's series handles this, but this
> type-2 series was shorter so I started here first.

--
Best Regards,
Huang, Ying