diff mbox series

[2/5] cxl: Rename CXL_DECODER_HOSTONLYMEM/DEVMEM

Message ID 20241015065713.308671-3-ying.huang@intel.com
State New
Headers show
Series cxl: Some preparation work for type2 accelerators support | expand

Commit Message

Huang, Ying Oct. 15, 2024, 6:57 a.m. UTC 
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(-)

Comments

Dan Williams Oct. 17, 2024, 10:21 p.m. UTC | #1 
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?
Huang, Ying Oct. 18, 2024, 6:18 a.m. UTC | #2 
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
Dan Williams Oct. 18, 2024, 9:17 p.m. UTC | #3 
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.
Huang, Ying Oct. 21, 2024, 4:40 a.m. UTC | #4 
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
diff mbox series

Patch

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;
 		/*