[RFC 0/5] cxl: Preparation of type2 accelerators support

[RFC 0/5] cxl: Preparation of type2 accelerators support

[Huang Ying]

There have been 2 series to add type 2 accelerators support in [1] and [2].

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

Both provide relative complete support, but both are long too.  To
make it easier to review, some preparation of type2 accelerators
support is implemented in this series.  More complete support can be
implemented based on it.

This series has been tested with cxl_test via mocking a type2
accelerator as in [1] above.  Because more type2 accelerators support
than that provided by this series is needed to simulate the device,
the cxl_test patch isn't included in the series.

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            | 28 +++++++++++------
 drivers/cxl/cxlmem.h         | 11 +++++++
 include/acpi/actbl1.h        | 10 +++----
 tools/testing/cxl/test/cxl.c | 24 +++++++--------
 9 files changed, 133 insertions(+), 63 deletions(-)

Best Regards,
Huang, Ying

[RFC 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 confusing about type 3 memory and host-only coherent in Linux
kernel, we rename 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.

Also rename the corresponding cxl_decoder flags
CXL_DECODER_F_TYPE2/TYPE3 to CXL_DECODER_F_DEVCOH/HOSTONLYCOH.

No functionality change is expected, because we just rename the flags
constant definition.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
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>
---
 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 1d5007e3795a..67a8dc4d7868 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 9afb407d438f..28c8783d3c14 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 841ef9f22795..2b38455e0f13 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

Re: [RFC 1/5] cxl: Rename ACPI_CEDT_CFMWS_RESTRICT_TYPE2/TYPE3

[Davidlohr Bueso]

On Wed, 25 Sep 2024, Huang Ying wrote:

>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 confusing about type 3 memory and host-only coherent in Linux
>kernel, we rename 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.
>
>Also rename the corresponding cxl_decoder flags
>CXL_DECODER_F_TYPE2/TYPE3 to CXL_DECODER_F_DEVCOH/HOSTONLYCOH.
>
>No functionality change is expected, because we just rename the flags
>constant definition.
>
>Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>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>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

Re: [RFC 1/5] cxl: Rename ACPI_CEDT_CFMWS_RESTRICT_TYPE2/TYPE3

[Gregory Price]

On Wed, Sep 25, 2024 at 10:46:43AM +0800, Huang Ying wrote:
> 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 confusing about type 3 memory and host-only coherent in Linux
> kernel, we rename 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.
> 
> Also rename the corresponding cxl_decoder flags
> CXL_DECODER_F_TYPE2/TYPE3 to CXL_DECODER_F_DEVCOH/HOSTONLYCOH.
> 
> No functionality change is expected, because we just rename the flags
> constant definition.
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 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>
> ---
>  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(-)
> 

Reviewed-by: Gregory Price <gourry@gourry.net>

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

[Huang Ying]

Previously, CXL type3 devices (memory expanders) use hostonly
coherence (HDM-H), while CXL type2 devices (accelerators) use dev
coherence (HDM-D).  So the target device type of a cxl decoder is
named as 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 confusing between the device type and coherence, in this
patch, CXL_DECODER_HOSTONLYMEM/DEVMEM is renamed to
CXL_DECODER_EXPANDER/ACCEL.

No functionality change is expected in this patch.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
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>
---
 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 67a8dc4d7868..47ad6d9329db 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 21ad5f242875..8229e8a0072d 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2545,7 +2545,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 28c8783d3c14..55b8c32f8d72 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: [RFC 2/5] cxl: Rename CXL_DECODER_HOSTONLYMEM/DEVMEM

[Davidlohr Bueso]

On Wed, 25 Sep 2024, Huang Ying wrote:

>Previously, CXL type3 devices (memory expanders) use hostonly
>coherence (HDM-H), while CXL type2 devices (accelerators) use dev
>coherence (HDM-D).  So the target device type of a cxl decoder is
>named as 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 confusing between the device type and coherence, in this
>patch, CXL_DECODER_HOSTONLYMEM/DEVMEM is renamed to
>CXL_DECODER_EXPANDER/ACCEL.
>
>No functionality change is expected in this patch.

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

>Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>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>

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

[Gregory Price]

On Wed, Sep 25, 2024 at 10:46:44AM +0800, Huang Ying wrote:
> Previously, CXL type3 devices (memory expanders) use hostonly
> coherence (HDM-H), while CXL type2 devices (accelerators) use dev
> coherence (HDM-D).  So the target device type of a cxl decoder is
> named as 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 confusing between the device type and coherence, in this
> patch, CXL_DECODER_HOSTONLYMEM/DEVMEM is renamed to
> CXL_DECODER_EXPANDER/ACCEL.
> 
> No functionality change is expected in this patch.
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 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>
> ---
>  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(-)
> 

Reviewed-by: Gregory Price <gourry@gourry.net>

[RFC 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 target
type is used to designate coherence too.  However, it's possible for
expanders to use HDM-DB now.  So, we need to separate coherence from
target type.

Accordingly, the HOSTONLY field of decoder ctrl
register (CXL_HDM_DECODER0_CTRL_HOSTONLY) need to be set according to
coherence.

The coherence of decoders can not be determined via target type too.
So, accelerator/expander device drivers need to specify coherence
explicitly via newly added coherence field in struct cxl_dev_state.

The coherence of each end points in a region need to be same.  So, the
coherence of the first end point is recorded in struct region.  Which
will be checked against the coherence of all other end points.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
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>
---
 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         |  9 +++++++++
 drivers/cxl/cxlmem.h      | 11 +++++++++++
 6 files changed, 71 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 e5cdeafdf76e..3635a0a402b7 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1424,6 +1424,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 47ad6d9329db..2dee78e9b90c 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 8229e8a0072d..cec7d08b6f44 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 55b8c32f8d72..99398c868d82 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
@@ -356,6 +362,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 +524,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 +538,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 afb53d058d62..cc4880286134 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -393,6 +393,16 @@ enum cxl_devtype {
 	CXL_DEVTYPE_CLASSMEM,
 };
 
+/*
+ * enum cxl_devcoherence - the coherence of the cxl device
+ * @CXL_DEVCOH_DEV      - HDM-D or HDM-DB
+ * @CXL_DEVCOH_HOSTONLY - HDM-H
+ */
+enum cxl_devcoherence {
+	CXL_DEVCOH_DEV,
+	CXL_DEVCOH_HOSTONLY,
+};
+
 /**
  * struct cxl_dpa_perf - DPA performance property entry
  * @dpa_range: range for DPA address
@@ -438,6 +448,7 @@ struct cxl_dev_state {
 	struct resource ram_res;
 	u64 serial;
 	enum cxl_devtype type;
+	enum cxl_devcoherence coherence;
 };
 
 /**
-- 
2.39.2

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

[Gregory Price]

On Wed, Sep 25, 2024 at 10:46:45AM +0800, Huang Ying wrote:
> Previously, target type (expander or accelerator) and
> coherence (HOSTONLY (HDM-H) or DEV (HDM-D/DB)) are synonym.  So target
> type is used to designate coherence too.  However, it's possible for
> expanders to use HDM-DB now.  So, we need to separate coherence from
> target type.
> 
> Accordingly, the HOSTONLY field of decoder ctrl
> register (CXL_HDM_DECODER0_CTRL_HOSTONLY) need to be set according to
> coherence.
> 
> The coherence of decoders can not be determined via target type too.
> So, accelerator/expander device drivers need to specify coherence
> explicitly via newly added coherence field in struct cxl_dev_state.
> 
> The coherence of each end points in a region need to be same.  So, the
> coherence of the first end point is recorded in struct region.  Which
> will be checked against the coherence of all other end points.
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 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>
> ---
>  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         |  9 +++++++++
>  drivers/cxl/cxlmem.h      | 11 +++++++++++
>  6 files changed, 71 insertions(+), 10 deletions(-)
> 

Reviewed-by: Gregory Price <gourry@gourry.net>

> @@ -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))) {

silly nit but my gut tells me we can make this less ugly.
Not a blocker though.

> +			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;
> +		}

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

[Huang, Ying]

Gregory Price <gourry@gourry.net> writes:

> On Wed, Sep 25, 2024 at 10:46:45AM +0800, Huang Ying wrote:
>> Previously, target type (expander or accelerator) and
>> coherence (HOSTONLY (HDM-H) or DEV (HDM-D/DB)) are synonym.  So target
>> type is used to designate coherence too.  However, it's possible for
>> expanders to use HDM-DB now.  So, we need to separate coherence from
>> target type.
>> 
>> Accordingly, the HOSTONLY field of decoder ctrl
>> register (CXL_HDM_DECODER0_CTRL_HOSTONLY) need to be set according to
>> coherence.
>> 
>> The coherence of decoders can not be determined via target type too.
>> So, accelerator/expander device drivers need to specify coherence
>> explicitly via newly added coherence field in struct cxl_dev_state.
>> 
>> The coherence of each end points in a region need to be same.  So, the
>> coherence of the first end point is recorded in struct region.  Which
>> will be checked against the coherence of all other end points.
>> 
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> 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>
>> ---
>>  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         |  9 +++++++++
>>  drivers/cxl/cxlmem.h      | 11 +++++++++++
>>  6 files changed, 71 insertions(+), 10 deletions(-)
>> 
>
> Reviewed-by: Gregory Price <gourry@gourry.net>

Thanks!

>> @@ -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))) {
>
> silly nit but my gut tells me we can make this less ugly.
> Not a blocker though.

Yes.  This looks urgly.  Will think about how to improve it.

>> +			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;
>> +		}

--
Best Regards,
Huang, Ying

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

[Huang, Ying]

"Huang, Ying" <ying.huang@intel.com> writes:

> Gregory Price <gourry@gourry.net> writes:
>
>> On Wed, Sep 25, 2024 at 10:46:45AM +0800, Huang Ying wrote:

[snip]

>
>>> @@ -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))) {
>>
>> silly nit but my gut tells me we can make this less ugly.
>> Not a blocker though.
>
> Yes.  This looks urgly.  Will think about how to improve it.

Reviewed the code again.  However, I don't have any idea to improve
this.  Do you have some suggestion?

>>> +			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;
>>> +		}
>

--
Best Regards,
Huang, Ying

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

[Ben Cheatham]

Hi Huang, quick comments in this patch and the next.

On 9/24/24 9:46 PM, Huang Ying wrote:
> Previously, target type (expander or accelerator) and
> coherence (HOSTONLY (HDM-H) or DEV (HDM-D/DB)) are synonym.  So target
> type is used to designate coherence too.  However, it's possible for
> expanders to use HDM-DB now.  So, we need to separate coherence from
> target type.
> 
> Accordingly, the HOSTONLY field of decoder ctrl
> register (CXL_HDM_DECODER0_CTRL_HOSTONLY) need to be set according to
> coherence.
> 
> The coherence of decoders can not be determined via target type too.
> So, accelerator/expander device drivers need to specify coherence
> explicitly via newly added coherence field in struct cxl_dev_state.
> 
> The coherence of each end points in a region need to be same.  So, the
> coherence of the first end point is recorded in struct region.  Which
> will be checked against the coherence of all other end points.
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 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>
> ---
>  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         |  9 +++++++++
>  drivers/cxl/cxlmem.h      | 11 +++++++++++
>  6 files changed, 71 insertions(+), 10 deletions(-)
> 

[snip]

>  
> +/*
> + * enum cxl_devcoherence - the coherence of the cxl device
> + * @CXL_DEVCOH_DEV      - HDM-D or HDM-DB
> + * @CXL_DEVCOH_HOSTONLY - HDM-H
> + */

Could I suggest mapping the coherence type to the expected device type(s) in this
comment? My thinking here is that the coherence types aren't exactly straightforward
and having the device types they correspond to would help ease any confusion, especially
since it looks like we are expecting type 2 driver writers to fill this in manually. I'm
thinking something along the lines of:
/*
 * 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
> @@ -438,6 +448,7 @@ struct cxl_dev_state {
>  	struct resource ram_res;
>  	u64 serial;
>  	enum cxl_devtype type;
> +	enum cxl_devcoherence coherence;
>  };
>  
>  /**

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

[Huang, Ying]

Ben Cheatham <benjamin.cheatham@amd.com> writes:

> Hi Huang, quick comments in this patch and the next.
>
> On 9/24/24 9:46 PM, Huang Ying wrote:
>> Previously, target type (expander or accelerator) and
>> coherence (HOSTONLY (HDM-H) or DEV (HDM-D/DB)) are synonym.  So target
>> type is used to designate coherence too.  However, it's possible for
>> expanders to use HDM-DB now.  So, we need to separate coherence from
>> target type.
>> 
>> Accordingly, the HOSTONLY field of decoder ctrl
>> register (CXL_HDM_DECODER0_CTRL_HOSTONLY) need to be set according to
>> coherence.
>> 
>> The coherence of decoders can not be determined via target type too.
>> So, accelerator/expander device drivers need to specify coherence
>> explicitly via newly added coherence field in struct cxl_dev_state.
>> 
>> The coherence of each end points in a region need to be same.  So, the
>> coherence of the first end point is recorded in struct region.  Which
>> will be checked against the coherence of all other end points.
>> 
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> 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>
>> ---
>>  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         |  9 +++++++++
>>  drivers/cxl/cxlmem.h      | 11 +++++++++++
>>  6 files changed, 71 insertions(+), 10 deletions(-)
>> 
>
> [snip]
>
>>  
>> +/*
>> + * enum cxl_devcoherence - the coherence of the cxl device
>> + * @CXL_DEVCOH_DEV      - HDM-D or HDM-DB
>> + * @CXL_DEVCOH_HOSTONLY - HDM-H
>> + */
>
> Could I suggest mapping the coherence type to the expected device type(s) in this
> comment? My thinking here is that the coherence types aren't exactly straightforward
> and having the device types they correspond to would help ease any confusion, especially
> since it looks like we are expecting type 2 driver writers to fill this in manually. I'm
> thinking something along the lines of:
> /*
>  * 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)
>  */

This looks good to me!  Thanks!  Will do this in the next version.

>> +enum cxl_devcoherence {
>> +	CXL_DEVCOH_DEV,
>> +	CXL_DEVCOH_HOSTONLY,
>> +};
>> +
>>  /**
>>   * struct cxl_dpa_perf - DPA performance property entry
>>   * @dpa_range: range for DPA address
>> @@ -438,6 +448,7 @@ struct cxl_dev_state {
>>  	struct resource ram_res;
>>  	u64 serial;
>>  	enum cxl_devtype type;
>> +	enum cxl_devcoherence coherence;
>>  };
>>  
>>  /**

--
Best Regards,
Huang, Ying

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

[Huang Ying]

The type of region is hard-coded as type 3 expander now, because this
is the only supported device type.  As a preparation to support type 2
accelerators, we set the type of region to that of the first endpoint.
Then, we will check 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.

The target type of endpoint devices comes from expander/accelerator
device drivers via struct cxl_dev_state.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
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>
---
 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 2dee78e9b90c..5633b7316cb3 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 cec7d08b6f44..9c68ec445128 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);
@@ -2482,7 +2485,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.
@@ -2492,8 +2494,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;
@@ -2504,7 +2505,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);
@@ -2576,7 +2577,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 99398c868d82..2a2d2c483654 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,
 	CXL_DECODER_ACCEL = 2,
 	CXL_DECODER_EXPANDER = 3,
 };
-- 
2.39.2

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

[Gregory Price]

On Wed, Sep 25, 2024 at 10:46:46AM +0800, Huang Ying wrote:
> The type of region is hard-coded as type 3 expander now, because this
> is the only supported device type.  As a preparation to support type 2
> accelerators, we set the type of region to that of the first endpoint.
> Then, we will check 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.
> 
> The target type of endpoint devices comes from expander/accelerator
> device drivers via struct cxl_dev_state.
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 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>
> ---
>  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(-)
> 

Reviewed-by: Gregory Price <gourry@gourry.net>

>  static ssize_t create_pmem_region_store(struct device *dev,
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 99398c868d82..2a2d2c483654 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,

nit - should this be an explicit value?

>  	CXL_DECODER_ACCEL = 2,
>  	CXL_DECODER_EXPANDER = 3,
>  };
> -- 
> 2.39.2
> 

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

[Huang, Ying]

Gregory Price <gourry@gourry.net> writes:

> On Wed, Sep 25, 2024 at 10:46:46AM +0800, Huang Ying wrote:
>> The type of region is hard-coded as type 3 expander now, because this
>> is the only supported device type.  As a preparation to support type 2
>> accelerators, we set the type of region to that of the first endpoint.
>> Then, we will check 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.
>> 
>> The target type of endpoint devices comes from expander/accelerator
>> device drivers via struct cxl_dev_state.
>> 
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> 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>
>> ---
>>  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(-)
>> 
>
> Reviewed-by: Gregory Price <gourry@gourry.net>

Thanks!

>>  static ssize_t create_pmem_region_store(struct device *dev,
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 99398c868d82..2a2d2c483654 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,
>
> nit - should this be an explicit value?

Sure.  Will fix this in the future version!

>>  	CXL_DECODER_ACCEL = 2,
>>  	CXL_DECODER_EXPANDER = 3,
>>  };
>> -- 
>> 2.39.2
>> 

--
Best Regards,
Huang, Ying

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

[Ben Cheatham]

On 9/24/24 9:46 PM, Huang Ying wrote:
> The type of region is hard-coded as type 3 expander now, because this
> is the only supported device type.  As a preparation to support type 2
> accelerators, we set the type of region to that of the first endpoint.
> Then, we will check 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.
> 
> The target type of endpoint devices comes from expander/accelerator
> device drivers via struct cxl_dev_state.
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 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>
> ---
>  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 2dee78e9b90c..5633b7316cb3 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;

You can drop this imo. It doesn't change anything functionally considering
the break is immediately followed by a return.

Thanks,
Ben

>  	}
>  	return -ENXIO;
>  }
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index cec7d08b6f44..9c68ec445128 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);
> @@ -2482,7 +2485,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.
> @@ -2492,8 +2494,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;
> @@ -2504,7 +2505,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);
> @@ -2576,7 +2577,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 99398c868d82..2a2d2c483654 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,
>  	CXL_DECODER_ACCEL = 2,
>  	CXL_DECODER_EXPANDER = 3,
>  };

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

[Huang, Ying]

Hi, Ben,

Ben Cheatham <benjamin.cheatham@amd.com> writes:

> On 9/24/24 9:46 PM, Huang Ying wrote:

[snip]

>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 2dee78e9b90c..5633b7316cb3 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;
>
> You can drop this imo. It doesn't change anything functionally considering
> the break is immediately followed by a return.

Sorry, I cannot do that.  Otherwise, there will be build error.

> Thanks,
> Ben
>
>>  	}
>>  	return -ENXIO;
>>  }

[snip]

--
Best Regards,
Huang, Ying

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

[Ben Cheatham]

On 10/2/24 8:12 PM, Huang, Ying wrote:
> Hi, Ben,
> 
> Ben Cheatham <benjamin.cheatham@amd.com> writes:
> 
>> On 9/24/24 9:46 PM, Huang Ying wrote:
> 
> [snip]
> 
>>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>>> index 2dee78e9b90c..5633b7316cb3 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;
>>
>> You can drop this imo. It doesn't change anything functionally considering
>> the break is immediately followed by a return.
> 
> Sorry, I cannot do that.  Otherwise, there will be build error.
> 
I thought that might be the case, but figured I'd suggest it anyway. Oh well!

Thanks,
Ben

>> Thanks,
>> Ben
>>
>>>  	}
>>>  	return -ENXIO;
>>>  }
> 
> [snip]
> 
> --
> Best Regards,
> Huang, Ying

[RFC 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, in this patch, we skip 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>
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>
---
 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 9c68ec445128..b276752c38da 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3466,6 +3466,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: [RFC 5/5] cxl: Avoid to create dax regions for type2 accelerators

[Gregory Price]

On Wed, Sep 25, 2024 at 10:46:47AM +0800, 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, in this patch, we skip 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>
> 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>
> ---
>  drivers/cxl/core/region.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

Reviewed-by: Gregory Price <gourry@gourry.net>

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 9c68ec445128..b276752c38da 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3466,6 +3466,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
>