[PATCH 0/3] cxl: Support Poison Inject & Clear by HPA

[PATCH 0/3] cxl: Support Poison Inject & Clear by HPA

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

This series allows expert users to inject and clear poison by writing a
Host Physical Address (HPA) to a region debugfs files. At the core of this
new functionality is a helper that translates an HPA into a Device Physical
Address (DPA) and a memdev based on the region's decoder configuration.

The set is not merely a convenience wrapper for these region poison
operations as it enables these operations for XOR interleaved regions
where they were previously impossible.

Patch 1 adds a locked variant of the inject and clear poison ops to
support callers that must hold locks during the entire translation and
operation sequence.

Patch 2 introduces the translation logic capable of returning the memdev
and DPA for a given HPA.

Patch 3 exposes the capability through region debugfs attributes that 
only appear when all participating memdevs support the poison commands.

These patches build on the existing memdev poison and region address
translation infrastructure and target debug and platform validation
scenarios.

Watch for the CXL Unit Test update to cxl-poison.sh posted separately
on this list.


Alison Schofield (3):
  cxl/core: Add locked variants of the poison inject and clear funcs
  cxl/region: Introduce HPA to DPA address translation
  cxl/region: Add inject and clear poison by HPA

 Documentation/ABI/testing/debugfs-cxl |  33 +++++
 drivers/cxl/core/core.h               |   4 +
 drivers/cxl/core/memdev.c             |  88 +++++++----
 drivers/cxl/core/region.c             | 201 ++++++++++++++++++++++++++
 drivers/cxl/cxlmem.h                  |   2 +
 5 files changed, 301 insertions(+), 27 deletions(-)


base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
-- 
2.37.3

[PATCH 1/3] cxl/core: Add locked variants of the poison inject and clear funcs

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

The core functions that validate and send inject and clear commands
to the memdev devices require holding both the dpa_rwsem and the
region_rwsem.

In preparation for another caller of these functions that must hold
the locks upon entry, split the work into a locked and unlocked pair.

Consideration was given to moving the locking to both callers,
however, the existing caller is not in the core (mem.c) and cannot
access the locks.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/memdev.c | 77 +++++++++++++++++++++++++--------------
 drivers/cxl/cxlmem.h      |  2 +
 2 files changed, 52 insertions(+), 27 deletions(-)

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index f88a13adf7fa..b38141761f47 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -280,7 +280,7 @@ static int cxl_validate_poison_dpa(struct cxl_memdev *cxlmd, u64 dpa)
 	return 0;
 }
 
-int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
+int __inject_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
 {
 	struct cxl_mailbox *cxl_mbox = &cxlmd->cxlds->cxl_mbox;
 	struct cxl_mbox_inject_poison inject;
@@ -292,19 +292,12 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
 	if (!IS_ENABLED(CONFIG_DEBUG_FS))
 		return 0;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
-		return rc;
-
-	rc = down_read_interruptible(&cxl_dpa_rwsem);
-	if (rc) {
-		up_read(&cxl_region_rwsem);
-		return rc;
-	}
+	lockdep_assert_held(&cxl_dpa_rwsem);
+	lockdep_assert_held(&cxl_region_rwsem);
 
 	rc = cxl_validate_poison_dpa(cxlmd, dpa);
 	if (rc)
-		goto out;
+		return rc;
 
 	inject.address = cpu_to_le64(dpa);
 	mbox_cmd = (struct cxl_mbox_cmd) {
@@ -314,7 +307,7 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
 	};
 	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
 	if (rc)
-		goto out;
+		return rc;
 
 	cxlr = cxl_dpa_to_region(cxlmd, dpa);
 	if (cxlr)
@@ -327,7 +320,26 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
 		.length = cpu_to_le32(1),
 	};
 	trace_cxl_poison(cxlmd, cxlr, &record, 0, 0, CXL_POISON_TRACE_INJECT);
-out:
+
+	return rc;
+}
+
+int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
+{
+	int rc;
+
+	rc = down_read_interruptible(&cxl_region_rwsem);
+	if (rc)
+		return rc;
+
+	rc = down_read_interruptible(&cxl_dpa_rwsem);
+	if (rc) {
+		up_read(&cxl_region_rwsem);
+		return rc;
+	}
+
+	rc = __inject_poison_locked(cxlmd, dpa);
+
 	up_read(&cxl_dpa_rwsem);
 	up_read(&cxl_region_rwsem);
 
@@ -335,7 +347,7 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_inject_poison, "CXL");
 
-int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
+int __clear_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
 {
 	struct cxl_mailbox *cxl_mbox = &cxlmd->cxlds->cxl_mbox;
 	struct cxl_mbox_clear_poison clear;
@@ -347,20 +359,12 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
 	if (!IS_ENABLED(CONFIG_DEBUG_FS))
 		return 0;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
-		return rc;
-
-	rc = down_read_interruptible(&cxl_dpa_rwsem);
-	if (rc) {
-		up_read(&cxl_region_rwsem);
-		return rc;
-	}
+	lockdep_assert_held(&cxl_dpa_rwsem);
+	lockdep_assert_held(&cxl_region_rwsem);
 
 	rc = cxl_validate_poison_dpa(cxlmd, dpa);
 	if (rc)
-		goto out;
-
+		return rc;
 	/*
 	 * In CXL 3.0 Spec 8.2.9.8.4.3, the Clear Poison mailbox command
 	 * is defined to accept 64 bytes of write-data, along with the
@@ -378,7 +382,7 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
 
 	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
 	if (rc)
-		goto out;
+		return rc;
 
 	cxlr = cxl_dpa_to_region(cxlmd, dpa);
 	if (cxlr)
@@ -391,7 +395,26 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
 		.length = cpu_to_le32(1),
 	};
 	trace_cxl_poison(cxlmd, cxlr, &record, 0, 0, CXL_POISON_TRACE_CLEAR);
-out:
+
+	return rc;
+}
+
+int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
+{
+	int rc;
+
+	rc = down_read_interruptible(&cxl_region_rwsem);
+	if (rc)
+		return rc;
+
+	rc = down_read_interruptible(&cxl_dpa_rwsem);
+	if (rc) {
+		up_read(&cxl_region_rwsem);
+		return rc;
+	}
+
+	rc = __clear_poison_locked(cxlmd, dpa);
+
 	up_read(&cxl_dpa_rwsem);
 	up_read(&cxl_region_rwsem);
 
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 551b0ba2caa1..c0643e8b9d8f 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -861,6 +861,8 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 int cxl_trigger_poison_list(struct cxl_memdev *cxlmd);
 int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa);
 int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa);
+int __inject_poison_locked(struct cxl_memdev *cxlmd, u64 dpa);
+int __clear_poison_locked(struct cxl_memdev *cxlmd, u64 dpa);
 
 #ifdef CONFIG_CXL_EDAC_MEM_FEATURES
 int devm_cxl_memdev_edac_register(struct cxl_memdev *cxlmd);
-- 
2.37.3

[PATCH 2/3] cxl/region: Introduce HPA to DPA address translation

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Add infrastructure to translate Host Physical Addresses (HPA) to Device
Physical Addresses (DPA) within CXL regions. This capability is being
introduced for use by follow-on patches that will add poison inject
and clear operations at the region level.

The HPA-to-DPA translation process involves several steps:
1. Apply root decoder transformations (HPA to SPA) if configured
2. Calculate the relative offset within the region's address space
3. Decode the interleave position using the region's interleave ways
   and granularity settings
4. Identify the target memdev based on the decoded position
5. Compute the final DPA by adding the decoded offset to the memdev's
   DPA base address

It is Step 1 above that makes this a driver level operation and not
work we can push to user space. Rather than exporting the XOR maps for
root decoders configured with XOR interleave, the driver performs this
complex calculation for the user.

While not immediately apparent in this diff, broader examination of
the region.c code shows that this work is basically the reverse of
previous work where a DPA is translated to an HPA. This is notable
because it demonstrates that these calculations reuse existing logic
rather than introducing new algorithms.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/region.c | 85 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6e5e1460068d..d2d904c4b427 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2972,6 +2972,91 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
 	return hpa;
 }
 
+struct hpa_decode_result {
+	u64 dpa_offset;
+	int pos;
+};
+
+struct cxl_dpa_result {
+	u64 dpa;
+	struct cxl_memdev *cxlmd;
+};
+
+static struct hpa_decode_result decode_hpa_to_dpa(u64 hpa_offset, u8 eiw,
+						  u16 eig)
+{
+	struct hpa_decode_result result;
+	u64 bits_upper;
+
+	if (eiw < 8) {
+		result.pos = (hpa_offset >> (eig + 8)) & GENMASK(eiw - 1, 0);
+		hpa_offset &= ~((u64)GENMASK(eiw - 1, 0) << (eig + 8));
+		result.dpa_offset = hpa_offset >> eiw;
+	} else {
+		bits_upper = hpa_offset >> (eig + 8);
+		result.pos = bits_upper % 3;
+		bits_upper /= 3;
+		result.dpa_offset = bits_upper << (eig + 8);
+	}
+
+	result.dpa_offset |= hpa_offset & GENMASK_ULL(eig + 7, 0);
+
+	return result;
+}
+
+static struct cxl_dpa_result __maybe_unused
+cxl_hpa_to_dpa(struct cxl_region *cxlr, u64 hpa)
+{
+	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+	struct cxl_dpa_result result = { .dpa = ULLONG_MAX, .cxlmd = NULL };
+	struct cxl_region_params *p = &cxlr->params;
+	struct hpa_decode_result decode;
+	u64 hpa_offset;
+	u16 eig = 0;
+	u8 eiw = 0;
+
+	lockdep_assert_held(&cxl_region_rwsem);
+	lockdep_assert_held(&cxl_dpa_rwsem);
+
+	if (hpa < p->res->start || hpa > p->res->end) {
+		dev_err_once(&cxlr->dev,
+			     "HPA 0x%llx not in region [0x%llx-0x%llx]\n", hpa,
+			     p->res->start, p->res->end);
+
+		return result;
+	}
+
+	/* Apply root decoder translation */
+	if (cxlrd->hpa_to_spa)
+		hpa = cxlrd->hpa_to_spa(cxlrd, hpa);
+
+	ways_to_eiw(p->interleave_ways, &eiw);
+	granularity_to_eig(p->interleave_granularity, &eig);
+	hpa_offset = hpa - p->res->start;
+
+	decode = decode_hpa_to_dpa(hpa_offset, eiw, eig);
+	if (decode.pos >= p->nr_targets) {
+		dev_err(&cxlr->dev, "Invalid position %d for %d targets\n",
+			decode.pos, p->nr_targets);
+
+		return result;
+	}
+
+	for (int i = 0; i < p->nr_targets; i++) {
+		struct cxl_endpoint_decoder *cxled = p->targets[i];
+
+		if (cxled->pos == decode.pos) {
+			result.cxlmd = cxled_to_memdev(cxled);
+			result.dpa = decode.dpa_offset + cxl_dpa_resource_start(cxled);
+
+			return result;
+		}
+	}
+	dev_err(&cxlr->dev, "No device found for position %d\n", decode.pos);
+
+	return result;
+}
+
 static struct lock_class_key cxl_pmem_region_key;
 
 static int cxl_pmem_region_alloc(struct cxl_region *cxlr)
-- 
2.37.3

[PATCH 3/3] cxl/region: Add inject and clear poison by HPA

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Add CXL region debugfs attributes to inject and clear poison based
on Host Physical Addresses (HPA). These new interfaces allow users
to operate on poison at the region level without needing to resolve
Device Physical Addresses (DPA) or target individual memdevs.

The implementation leverages an internal HPA-to-DPA helper, which
applies decoder interleave logic, including XOR-based address decoding
when applicable. Note that XOR decodes rely on driver internal xormaps
which are not exposed to userspace. So, this support is not only a
simplification of poison operations that could be done using existing
per memdev operations, but also it enables the functionality for XOR
interleaved regions for the first time.

The new debugfs attributes are added under /sys/kernel/debug/regionX/:
inject_poison and clear_poison. These are only exposed if all memdevs
participating in the region support both inject and clear commands,
ensuring consistent and reliable behavior across multi-device regions.

If tracing is enabled, these operations are logged as cxl_poison
events in /sys/kernel/tracing/trace.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 Documentation/ABI/testing/debugfs-cxl |  33 +++++++
 drivers/cxl/core/core.h               |   4 +
 drivers/cxl/core/memdev.c             |  11 +++
 drivers/cxl/core/region.c             | 120 +++++++++++++++++++++++++-
 4 files changed, 166 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/debugfs-cxl b/Documentation/ABI/testing/debugfs-cxl
index 12488c14be64..c784885cdf07 100644
--- a/Documentation/ABI/testing/debugfs-cxl
+++ b/Documentation/ABI/testing/debugfs-cxl
@@ -35,6 +35,39 @@ Description:
 		The clear_poison attribute is only visible for devices
 		supporting the capability.
 
+
+What:		/sys/kernel/debug/regionX/inject_poison
+Date:		August, 2025
+KernelVersion:	v6.17
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(WO) When a Host Physical Address (HPA) is written to this
+		attribute, the region driver translates it to a Device
+		Physical Address (DPA) and identifies the corresponding
+		memdev. It then sends an inject poison command to that memdev
+		at the translated DPA. Refer to the memdev ABI entry at:
+		/sys/kernel/debug/cxl/memX/inject_poison for the detailed
+		behavior. This attribute is only visible if all memdevs
+		participating in the region support both inject and clear
+		poison commands.
+
+
+What:		/sys/kernel/debug/regionX/clear_poison
+Date:		August, 2025
+KernelVersion:	v6.17
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(WO) When a Host Physical Address (HPA) is written to this
+		attribute, the region driver translates it to a Device
+		Physical Address (DPA) and identifies the corresponding
+		memdev. It then sends a clear poison command to that memdev
+		at the translated DPA. Refer to the memdev ABI entry at:
+		/sys/kernel/debug/cxl/memX/clear_poison for the detailed
+		behavior. This attribute is only visible if all memdevs
+		participating in the region support both inject and clear
+		poison commands.
+
+
 What:		/sys/kernel/debug/cxl/einj_types
 Date:		January, 2024
 KernelVersion:	v6.9
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 29b61828a847..66ca5f899fd6 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -109,6 +109,10 @@ enum cxl_poison_trace_type {
 	CXL_POISON_TRACE_CLEAR,
 };
 
+enum poison_cmd_enabled_bits;
+bool cxl_memdev_has_poison_cmd(struct cxl_memdev *cxlmd,
+			       enum poison_cmd_enabled_bits cmd);
+
 long cxl_pci_get_latency(struct pci_dev *pdev);
 int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c);
 int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index b38141761f47..7616e68fa79e 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -200,6 +200,17 @@ static ssize_t security_erase_store(struct device *dev,
 static struct device_attribute dev_attr_security_erase =
 	__ATTR(erase, 0200, NULL, security_erase_store);
 
+bool cxl_memdev_has_poison_cmd(struct cxl_memdev *cxlmd,
+			       enum poison_cmd_enabled_bits cmd)
+{
+	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
+
+	if (test_bit(cmd, mds->poison.enabled_cmds))
+		return true;
+
+	return false;
+}
+
 static int cxl_get_poison_by_memdev(struct cxl_memdev *cxlmd)
 {
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index d2d904c4b427..e9ed5ac7309e 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2,6 +2,7 @@
 /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
 #include <linux/memregion.h>
 #include <linux/genalloc.h>
+#include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/memory.h>
@@ -3004,8 +3005,7 @@ static struct hpa_decode_result decode_hpa_to_dpa(u64 hpa_offset, u8 eiw,
 	return result;
 }
 
-static struct cxl_dpa_result __maybe_unused
-cxl_hpa_to_dpa(struct cxl_region *cxlr, u64 hpa)
+static struct cxl_dpa_result cxl_hpa_to_dpa(struct cxl_region *cxlr, u64 hpa)
 {
 	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
 	struct cxl_dpa_result result = { .dpa = ULLONG_MAX, .cxlmd = NULL };
@@ -3616,10 +3616,101 @@ static void shutdown_notifiers(void *_cxlr)
 	unregister_mt_adistance_algorithm(&cxlr->adist_notifier);
 }
 
+static void remove_debugfs(void *dentry)
+{
+	debugfs_remove_recursive(dentry);
+}
+
+static int cxl_region_debugfs_poison_inject(void *data, u64 hpa)
+{
+	struct cxl_region *cxlr = data;
+	struct cxl_region_params *p = &cxlr->params;
+	struct cxl_dpa_result result;
+	int rc;
+
+	rc = down_read_interruptible(&cxl_region_rwsem);
+	if (rc)
+		return rc;
+
+	rc = down_read_interruptible(&cxl_dpa_rwsem);
+	if (rc) {
+		up_read(&cxl_region_rwsem);
+		return rc;
+	}
+
+	if (hpa < p->res->start || hpa > p->res->end) {
+		dev_err_once(&cxlr->dev, "HPA 0x%llx not in region %pr\n", hpa,
+			     p->res);
+		rc = -EINVAL;
+		goto out;
+	}
+	result = cxl_hpa_to_dpa(cxlr, hpa);
+	if (!result.cxlmd || result.dpa == ULLONG_MAX) {
+		dev_err(&cxlr->dev, "Failed to resolve DPA from HPA 0x%llx\n",
+			hpa);
+
+		rc = -EINVAL;
+		goto out;
+	}
+	rc = __inject_poison_locked(result.cxlmd, result.dpa);
+
+out:
+	up_read(&cxl_dpa_rwsem);
+	up_read(&cxl_region_rwsem);
+
+	return rc;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_inject_fops, NULL,
+			 cxl_region_debugfs_poison_inject, "%llx\n");
+
+static int cxl_region_debugfs_poison_clear(void *data, u64 hpa)
+{
+	struct cxl_region *cxlr = data;
+	struct cxl_region_params *p = &cxlr->params;
+	struct cxl_dpa_result result;
+	int rc;
+
+	rc = down_read_interruptible(&cxl_region_rwsem);
+	if (rc)
+		return rc;
+
+	rc = down_read_interruptible(&cxl_dpa_rwsem);
+	if (rc) {
+		up_read(&cxl_region_rwsem);
+		return rc;
+	}
+
+	if (hpa < p->res->start || hpa > p->res->end) {
+		dev_err_once(&cxlr->dev, "HPA 0x%llx not in region %pr\n", hpa,
+			     p->res);
+		rc = -EINVAL;
+		goto out;
+	}
+	result = cxl_hpa_to_dpa(cxlr, hpa);
+	if (!result.cxlmd || result.dpa == ULLONG_MAX) {
+		dev_err(&cxlr->dev, "Failed to resolve DPA from HPA 0x%llx\n",
+			hpa);
+
+		rc = -EINVAL;
+		goto out;
+	}
+	rc = __clear_poison_locked(result.cxlmd, result.dpa);
+out:
+	up_read(&cxl_dpa_rwsem);
+	up_read(&cxl_region_rwsem);
+
+	return rc;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_clear_fops, NULL,
+			 cxl_region_debugfs_poison_clear, "%llx\n");
+
 static int cxl_region_probe(struct device *dev)
 {
 	struct cxl_region *cxlr = to_cxl_region(dev);
 	struct cxl_region_params *p = &cxlr->params;
+	bool poison_supported = true;
 	int rc;
 
 	rc = down_read_interruptible(&cxl_region_rwsem);
@@ -3663,6 +3754,31 @@ static int cxl_region_probe(struct device *dev)
 	if (rc)
 		return rc;
 
+	/* Create poison attributes if all memdevs support the capabilities */
+	for (int i = 0; i < p->nr_targets; i++) {
+		struct cxl_endpoint_decoder *cxled = p->targets[i];
+		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+
+		if (!cxl_memdev_has_poison_cmd(cxlmd, CXL_POISON_ENABLED_INJECT) ||
+		    !cxl_memdev_has_poison_cmd(cxlmd, CXL_POISON_ENABLED_CLEAR)) {
+			poison_supported = false;
+			break;
+		}
+	}
+
+	if (poison_supported) {
+		struct dentry *dentry;
+
+		dentry = cxl_debugfs_create_dir(dev_name(dev));
+		debugfs_create_file("inject_poison", 0200, dentry, cxlr,
+				    &cxl_poison_inject_fops);
+		debugfs_create_file("clear_poison", 0200, dentry, cxlr,
+				    &cxl_poison_clear_fops);
+		rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
+		if (rc)
+			return rc;
+	}
+
 	switch (cxlr->mode) {
 	case CXL_PARTMODE_PMEM:
 		rc = devm_cxl_region_edac_register(cxlr);
-- 
2.37.3

Re: [PATCH 3/3] cxl/region: Add inject and clear poison by HPA

[kernel test robot]

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on 19272b37aa4f83ca52bdf9c16d5d81bdd1354494]

url:    https://github.com/intel-lab-lkp/linux/commits/alison-schofield-intel-com/cxl-core-Add-locked-variants-of-the-poison-inject-and-clear-funcs/20250624-085810
base:   19272b37aa4f83ca52bdf9c16d5d81bdd1354494
patch link:    https://lore.kernel.org/r/50bd42e0db1ed5979a00e6f5a43147320a1d5b9b.1750725512.git.alison.schofield%40intel.com
patch subject: [PATCH 3/3] cxl/region: Add inject and clear poison by HPA
config: arm-randconfig-003-20250624 (https://download.01.org/0day-ci/archive/20250624/202506241504.qzHRxclP-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.4.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250624/202506241504.qzHRxclP-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506241504.qzHRxclP-lkp@intel.com/

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: drivers/cxl/core/region.o: in function `cxl_hpa_to_dpa':
>> region.c:(.text.cxl_hpa_to_dpa+0x3dc): undefined reference to `__aeabi_uldivmod'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] cxl/core: Add locked variants of the poison inject and clear funcs

[Jonathan Cameron]

On Mon, 23 Jun 2025 17:53:34 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> The core functions that validate and send inject and clear commands
> to the memdev devices require holding both the dpa_rwsem and the
> region_rwsem.
> 
> In preparation for another caller of these functions that must hold
> the locks upon entry, split the work into a locked and unlocked pair.
> 
> Consideration was given to moving the locking to both callers,
> however, the existing caller is not in the core (mem.c) and cannot
> access the locks.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

This will clash with Dan's ACQUIRE() set.  Up to Dave on how
to handle that but my gut feeling is that if we want to queue much
else this cycle and that we should ask for patches on top of the
ACQUIRE() series.  The risk being that gets some later push back.

Ah well I'll apply the someone else's problem field.

Given reuse of these in patch 3 this looks sensible to me.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>


> ---
>  drivers/cxl/core/memdev.c | 77 +++++++++++++++++++++++++--------------
>  drivers/cxl/cxlmem.h      |  2 +
>  2 files changed, 52 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index f88a13adf7fa..b38141761f47 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -280,7 +280,7 @@ static int cxl_validate_poison_dpa(struct cxl_memdev *cxlmd, u64 dpa)
>  	return 0;
>  }
>  
> -int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
> +int __inject_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
>  {
>  	struct cxl_mailbox *cxl_mbox = &cxlmd->cxlds->cxl_mbox;
>  	struct cxl_mbox_inject_poison inject;
> @@ -292,19 +292,12 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
>  	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>  		return 0;
>  
> -	rc = down_read_interruptible(&cxl_region_rwsem);
> -	if (rc)
> -		return rc;
> -
> -	rc = down_read_interruptible(&cxl_dpa_rwsem);
> -	if (rc) {
> -		up_read(&cxl_region_rwsem);
> -		return rc;
> -	}
> +	lockdep_assert_held(&cxl_dpa_rwsem);
> +	lockdep_assert_held(&cxl_region_rwsem);
>  
>  	rc = cxl_validate_poison_dpa(cxlmd, dpa);
>  	if (rc)
> -		goto out;
> +		return rc;
>  
>  	inject.address = cpu_to_le64(dpa);
>  	mbox_cmd = (struct cxl_mbox_cmd) {
> @@ -314,7 +307,7 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
>  	};
>  	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
>  	if (rc)
> -		goto out;
> +		return rc;
>  
>  	cxlr = cxl_dpa_to_region(cxlmd, dpa);
>  	if (cxlr)
> @@ -327,7 +320,26 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
>  		.length = cpu_to_le32(1),
>  	};
>  	trace_cxl_poison(cxlmd, cxlr, &record, 0, 0, CXL_POISON_TRACE_INJECT);
> -out:
> +
> +	return rc;
> +}
> +
> +int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
> +{
> +	int rc;
> +
> +	rc = down_read_interruptible(&cxl_region_rwsem);
> +	if (rc)
> +		return rc;
> +
> +	rc = down_read_interruptible(&cxl_dpa_rwsem);
> +	if (rc) {
> +		up_read(&cxl_region_rwsem);
> +		return rc;
> +	}
> +
> +	rc = __inject_poison_locked(cxlmd, dpa);
> +
>  	up_read(&cxl_dpa_rwsem);
>  	up_read(&cxl_region_rwsem);
>  
> @@ -335,7 +347,7 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_inject_poison, "CXL");
>  
> -int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
> +int __clear_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
>  {
>  	struct cxl_mailbox *cxl_mbox = &cxlmd->cxlds->cxl_mbox;
>  	struct cxl_mbox_clear_poison clear;
> @@ -347,20 +359,12 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
>  	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>  		return 0;
>  
> -	rc = down_read_interruptible(&cxl_region_rwsem);
> -	if (rc)
> -		return rc;
> -
> -	rc = down_read_interruptible(&cxl_dpa_rwsem);
> -	if (rc) {
> -		up_read(&cxl_region_rwsem);
> -		return rc;
> -	}
> +	lockdep_assert_held(&cxl_dpa_rwsem);
> +	lockdep_assert_held(&cxl_region_rwsem);
>  
>  	rc = cxl_validate_poison_dpa(cxlmd, dpa);
>  	if (rc)
> -		goto out;
> -
> +		return rc;
>  	/*
>  	 * In CXL 3.0 Spec 8.2.9.8.4.3, the Clear Poison mailbox command
>  	 * is defined to accept 64 bytes of write-data, along with the
> @@ -378,7 +382,7 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
>  
>  	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
>  	if (rc)
> -		goto out;
> +		return rc;
>  
>  	cxlr = cxl_dpa_to_region(cxlmd, dpa);
>  	if (cxlr)
> @@ -391,7 +395,26 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
>  		.length = cpu_to_le32(1),
>  	};
>  	trace_cxl_poison(cxlmd, cxlr, &record, 0, 0, CXL_POISON_TRACE_CLEAR);
> -out:
> +
> +	return rc;
> +}
> +
> +int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
> +{
> +	int rc;
> +
> +	rc = down_read_interruptible(&cxl_region_rwsem);
> +	if (rc)
> +		return rc;
> +
> +	rc = down_read_interruptible(&cxl_dpa_rwsem);
> +	if (rc) {
> +		up_read(&cxl_region_rwsem);
> +		return rc;
> +	}
> +
> +	rc = __clear_poison_locked(cxlmd, dpa);
> +
>  	up_read(&cxl_dpa_rwsem);
>  	up_read(&cxl_region_rwsem);
>  
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 551b0ba2caa1..c0643e8b9d8f 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -861,6 +861,8 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>  int cxl_trigger_poison_list(struct cxl_memdev *cxlmd);
>  int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa);
>  int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa);
> +int __inject_poison_locked(struct cxl_memdev *cxlmd, u64 dpa);
> +int __clear_poison_locked(struct cxl_memdev *cxlmd, u64 dpa);
>  
>  #ifdef CONFIG_CXL_EDAC_MEM_FEATURES
>  int devm_cxl_memdev_edac_register(struct cxl_memdev *cxlmd);

Re: [PATCH 2/3] cxl/region: Introduce HPA to DPA address translation

[Jonathan Cameron]

On Mon, 23 Jun 2025 17:53:35 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> Add infrastructure to translate Host Physical Addresses (HPA) to Device
> Physical Addresses (DPA) within CXL regions. This capability is being
> introduced for use by follow-on patches that will add poison inject
> and clear operations at the region level.
> 
> The HPA-to-DPA translation process involves several steps:
> 1. Apply root decoder transformations (HPA to SPA) if configured
> 2. Calculate the relative offset within the region's address space
> 3. Decode the interleave position using the region's interleave ways
>    and granularity settings
> 4. Identify the target memdev based on the decoded position
> 5. Compute the final DPA by adding the decoded offset to the memdev's
>    DPA base address
> 
> It is Step 1 above that makes this a driver level operation and not
> work we can push to user space. Rather than exporting the XOR maps for
> root decoders configured with XOR interleave, the driver performs this
> complex calculation for the user.
> 
> While not immediately apparent in this diff, broader examination of
> the region.c code shows that this work is basically the reverse of
> previous work where a DPA is translated to an HPA. This is notable
> because it demonstrates that these calculations reuse existing logic
> rather than introducing new algorithms.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Hi Alison,

This maths still gives me a headache, but I don't follow at least some of it.
Also I'm missing the application of the XOR Map referred to above.

> ---
>  drivers/cxl/core/region.c | 85 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6e5e1460068d..d2d904c4b427 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2972,6 +2972,91 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
>  	return hpa;
>  }
>  
> +struct hpa_decode_result {
> +	u64 dpa_offset;
> +	int pos;
> +};
> +
> +struct cxl_dpa_result {
> +	u64 dpa;
> +	struct cxl_memdev *cxlmd;
> +};
> +
> +static struct hpa_decode_result decode_hpa_to_dpa(u64 hpa_offset, u8 eiw,
Having a function called hpa_to_dpa that takes an hpa_offset seems inconsistent.

decode_hpa_offset_to_dpa() maybe?

> +						  u16 eig)
> +{
> +	struct hpa_decode_result result;
> +	u64 bits_upper;
> +
> +	if (eiw < 8) {
Could add a spec reference. Other than that, this block looks fine to me.
> +		result.pos = (hpa_offset >> (eig + 8)) & GENMASK(eiw - 1, 0);
> +		hpa_offset &= ~((u64)GENMASK(eiw - 1, 0) << (eig + 8));
> +		result.dpa_offset = hpa_offset >> eiw;
> +	} else {

This block isn't lining up for me with what is documented in step 2 of the
implementation note on decoding.

> +		bits_upper = hpa_offset >> (eig + 8);
ignoring pos calc as that is confusingly in a very different place
in the spec, we should have
DPA_OFFSET[51:IG + 8] = HPA_OFFSET[51:IG + IW] / 3 

So I'm missing a shift by IW somewhere.
Perhaps some more comments would help relate this to the maths.

> +		result.pos = bits_upper % 3;
> +		bits_upper /= 3;
> +		result.dpa_offset = bits_upper << (eig + 8);



> +	}
> +
> +	result.dpa_offset |= hpa_offset & GENMASK_ULL(eig + 7, 0);

I wonder if it is simpler to keep a copy of these bottom bits from
before all the manipulation above.  They make it through untouched
but we could avoid the reader having to figure that out.


> +
> +	return result;
> +}
> +
> +static struct cxl_dpa_result __maybe_unused
> +cxl_hpa_to_dpa(struct cxl_region *cxlr, u64 hpa)
> +{
> +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> +	struct cxl_dpa_result result = { .dpa = ULLONG_MAX, .cxlmd = NULL };
> +	struct cxl_region_params *p = &cxlr->params;
> +	struct hpa_decode_result decode;
> +	u64 hpa_offset;
> +	u16 eig = 0;
> +	u8 eiw = 0;
> +
> +	lockdep_assert_held(&cxl_region_rwsem);
> +	lockdep_assert_held(&cxl_dpa_rwsem);
> +
> +	if (hpa < p->res->start || hpa > p->res->end) {
> +		dev_err_once(&cxlr->dev,
> +			     "HPA 0x%llx not in region [0x%llx-0x%llx]\n", hpa,
> +			     p->res->start, p->res->end);
> +
> +		return result;
> +	}
> +
> +	/* Apply root decoder translation */
> +	if (cxlrd->hpa_to_spa)
> +		hpa = cxlrd->hpa_to_spa(cxlrd, hpa);

Is this in the right direction?  I was rather expecting opposite of what
is going on in cxl_dpa_to_hpa()

Perhaps separate spa and hpa in here to improve readability with
spa == hpa for the case where we don't have a transaltion.

> +
> +	ways_to_eiw(p->interleave_ways, &eiw);
> +	granularity_to_eig(p->interleave_granularity, &eig);
> +	hpa_offset = hpa - p->res->start;

Given people will line this code up with cxl_dpa_to_hpa() maybe a
comment to rule out subtracting cache_size?

> +
> +	decode = decode_hpa_to_dpa(hpa_offset, eiw, eig);
> +	if (decode.pos >= p->nr_targets) {
> +		dev_err(&cxlr->dev, "Invalid position %d for %d targets\n",
> +			decode.pos, p->nr_targets);
> +
> +		return result;
> +	}
> +
> +	for (int i = 0; i < p->nr_targets; i++) {
> +		struct cxl_endpoint_decoder *cxled = p->targets[i];
> +
> +		if (cxled->pos == decode.pos) {
> +			result.cxlmd = cxled_to_memdev(cxled);
> +			result.dpa = decode.dpa_offset + cxl_dpa_resource_start(cxled);
> +
> +			return result;
> +		}
> +	}
> +	dev_err(&cxlr->dev, "No device found for position %d\n", decode.pos);
> +
> +	return result;
> +}
> +
>  static struct lock_class_key cxl_pmem_region_key;
>  
>  static int cxl_pmem_region_alloc(struct cxl_region *cxlr)

Re: [PATCH 3/3] cxl/region: Add inject and clear poison by HPA

[Jonathan Cameron]

On Mon, 23 Jun 2025 17:53:36 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> Add CXL region debugfs attributes to inject and clear poison based
> on Host Physical Addresses (HPA). These new interfaces allow users
> to operate on poison at the region level without needing to resolve
> Device Physical Addresses (DPA) or target individual memdevs.
> 
> The implementation leverages an internal HPA-to-DPA helper, which
> applies decoder interleave logic, including XOR-based address decoding
> when applicable. Note that XOR decodes rely on driver internal xormaps
> which are not exposed to userspace. So, this support is not only a
> simplification of poison operations that could be done using existing
> per memdev operations, but also it enables the functionality for XOR
> interleaved regions for the first time.
> 
> The new debugfs attributes are added under /sys/kernel/debug/regionX/:
> inject_poison and clear_poison. These are only exposed if all memdevs
> participating in the region support both inject and clear commands,
> ensuring consistent and reliable behavior across multi-device regions.
> 
> If tracing is enabled, these operations are logged as cxl_poison
> events in /sys/kernel/tracing/trace.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

A few things inline.

> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index b38141761f47..7616e68fa79e 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -200,6 +200,17 @@ static ssize_t security_erase_store(struct device *dev,
>  static struct device_attribute dev_attr_security_erase =
>  	__ATTR(erase, 0200, NULL, security_erase_store);
>  
> +bool cxl_memdev_has_poison_cmd(struct cxl_memdev *cxlmd,
> +			       enum poison_cmd_enabled_bits cmd)
> +{
> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> +
> +	if (test_bit(cmd, mds->poison.enabled_cmds))
> +		return true;
> +
> +	return false;

	return test_bit(cmd, ...);

> +}
> +
>  static int cxl_get_poison_by_memdev(struct cxl_memdev *cxlmd)
>  {
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index d2d904c4b427..e9ed5ac7309e 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2,6 +2,7 @@


> +DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_inject_fops, NULL,
> +			 cxl_region_debugfs_poison_inject, "%llx\n");
> +
> +static int cxl_region_debugfs_poison_clear(void *data, u64 hpa)
> +{
> +	struct cxl_region *cxlr = data;
> +	struct cxl_region_params *p = &cxlr->params;
> +	struct cxl_dpa_result result;
> +	int rc;
> +
> +	rc = down_read_interruptible(&cxl_region_rwsem);

This can use the new shiny ACQUIRE() tool letting us do
early returns and generally making it more readable.

> +	if (rc)
> +		return rc;
> +
> +	rc = down_read_interruptible(&cxl_dpa_rwsem);
> +	if (rc) {
> +		up_read(&cxl_region_rwsem);
> +		return rc;
> +	}
> +
> +	if (hpa < p->res->start || hpa > p->res->end) {
> +		dev_err_once(&cxlr->dev, "HPA 0x%llx not in region %pr\n", hpa,
> +			     p->res);
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +	result = cxl_hpa_to_dpa(cxlr, hpa);
> +	if (!result.cxlmd || result.dpa == ULLONG_MAX) {
> +		dev_err(&cxlr->dev, "Failed to resolve DPA from HPA 0x%llx\n",
> +			hpa);
> +
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +	rc = __clear_poison_locked(result.cxlmd, result.dpa);
> +out:
> +	up_read(&cxl_dpa_rwsem);
> +	up_read(&cxl_region_rwsem);
> +
> +	return rc;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_clear_fops, NULL,
> +			 cxl_region_debugfs_poison_clear, "%llx\n");
> +
>  static int cxl_region_probe(struct device *dev)
>  {
>  	struct cxl_region *cxlr = to_cxl_region(dev);
>  	struct cxl_region_params *p = &cxlr->params;
> +	bool poison_supported = true;
>  	int rc;
>  
>  	rc = down_read_interruptible(&cxl_region_rwsem);
> @@ -3663,6 +3754,31 @@ static int cxl_region_probe(struct device *dev)
>  	if (rc)
>  		return rc;
>  
> +	/* Create poison attributes if all memdevs support the capabilities */
> +	for (int i = 0; i < p->nr_targets; i++) {
> +		struct cxl_endpoint_decoder *cxled = p->targets[i];
> +		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +
> +		if (!cxl_memdev_has_poison_cmd(cxlmd, CXL_POISON_ENABLED_INJECT) ||
> +		    !cxl_memdev_has_poison_cmd(cxlmd, CXL_POISON_ENABLED_CLEAR)) {
> +			poison_supported = false;
You could drop i out of the loop and simply use the early break to detect
that a match occurred.

	if (i < p->nr_targets) {
		struct dentry *dentry;

but maybe that's a bit too subtle.  I don't mind having a variable for this.
	}
> +			break;
> +		}
> +	}
> +
> +	if (poison_supported) {
> +		struct dentry *dentry;
> +
> +		dentry = cxl_debugfs_create_dir(dev_name(dev));
> +		debugfs_create_file("inject_poison", 0200, dentry, cxlr,
> +				    &cxl_poison_inject_fops);
> +		debugfs_create_file("clear_poison", 0200, dentry, cxlr,
> +				    &cxl_poison_clear_fops);
> +		rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	switch (cxlr->mode) {
>  	case CXL_PARTMODE_PMEM:
>  		rc = devm_cxl_region_edac_register(cxlr);

Re: [PATCH 1/3] cxl/core: Add locked variants of the poison inject and clear funcs

[Dave Jiang]

On 6/24/25 6:38 AM, Jonathan Cameron wrote:
> On Mon, 23 Jun 2025 17:53:34 -0700
> alison.schofield@intel.com wrote:
> 
>> From: Alison Schofield <alison.schofield@intel.com>
>>
>> The core functions that validate and send inject and clear commands
>> to the memdev devices require holding both the dpa_rwsem and the
>> region_rwsem.
>>
>> In preparation for another caller of these functions that must hold
>> the locks upon entry, split the work into a locked and unlocked pair.
>>
>> Consideration was given to moving the locking to both callers,
>> however, the existing caller is not in the core (mem.c) and cannot
>> access the locks.
>>
>> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> 
> This will clash with Dan's ACQUIRE() set.  Up to Dave on how
> to handle that but my gut feeling is that if we want to queue much
> else this cycle and that we should ask for patches on top of the
> ACQUIRE() series.  The risk being that gets some later push back.

I think this patch may simplify more with ACQUIRE() if Dan hasn't already refactored the function. We can wait for a rev based off of that given there's an immutable branch with the ACQUIRE() patch on cxl.git already.

DJ

> 
> Ah well I'll apply the someone else's problem field.
> 
> Given reuse of these in patch 3 this looks sensible to me.
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> 
> 
>> ---
>>  drivers/cxl/core/memdev.c | 77 +++++++++++++++++++++++++--------------
>>  drivers/cxl/cxlmem.h      |  2 +
>>  2 files changed, 52 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index f88a13adf7fa..b38141761f47 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -280,7 +280,7 @@ static int cxl_validate_poison_dpa(struct cxl_memdev *cxlmd, u64 dpa)
>>  	return 0;
>>  }
>>  
>> -int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
>> +int __inject_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
>>  {
>>  	struct cxl_mailbox *cxl_mbox = &cxlmd->cxlds->cxl_mbox;
>>  	struct cxl_mbox_inject_poison inject;
>> @@ -292,19 +292,12 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
>>  	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>>  		return 0;
>>  
>> -	rc = down_read_interruptible(&cxl_region_rwsem);
>> -	if (rc)
>> -		return rc;
>> -
>> -	rc = down_read_interruptible(&cxl_dpa_rwsem);
>> -	if (rc) {
>> -		up_read(&cxl_region_rwsem);
>> -		return rc;
>> -	}
>> +	lockdep_assert_held(&cxl_dpa_rwsem);
>> +	lockdep_assert_held(&cxl_region_rwsem);
>>  
>>  	rc = cxl_validate_poison_dpa(cxlmd, dpa);
>>  	if (rc)
>> -		goto out;
>> +		return rc;
>>  
>>  	inject.address = cpu_to_le64(dpa);
>>  	mbox_cmd = (struct cxl_mbox_cmd) {
>> @@ -314,7 +307,7 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
>>  	};
>>  	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
>>  	if (rc)
>> -		goto out;
>> +		return rc;
>>  
>>  	cxlr = cxl_dpa_to_region(cxlmd, dpa);
>>  	if (cxlr)
>> @@ -327,7 +320,26 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
>>  		.length = cpu_to_le32(1),
>>  	};
>>  	trace_cxl_poison(cxlmd, cxlr, &record, 0, 0, CXL_POISON_TRACE_INJECT);
>> -out:
>> +
>> +	return rc;
>> +}
>> +
>> +int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
>> +{
>> +	int rc;
>> +
>> +	rc = down_read_interruptible(&cxl_region_rwsem);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = down_read_interruptible(&cxl_dpa_rwsem);
>> +	if (rc) {
>> +		up_read(&cxl_region_rwsem);
>> +		return rc;
>> +	}
>> +
>> +	rc = __inject_poison_locked(cxlmd, dpa);
>> +
>>  	up_read(&cxl_dpa_rwsem);
>>  	up_read(&cxl_region_rwsem);
>>  
>> @@ -335,7 +347,7 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
>>  }
>>  EXPORT_SYMBOL_NS_GPL(cxl_inject_poison, "CXL");
>>  
>> -int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
>> +int __clear_poison_locked(struct cxl_memdev *cxlmd, u64 dpa)
>>  {
>>  	struct cxl_mailbox *cxl_mbox = &cxlmd->cxlds->cxl_mbox;
>>  	struct cxl_mbox_clear_poison clear;
>> @@ -347,20 +359,12 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
>>  	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>>  		return 0;
>>  
>> -	rc = down_read_interruptible(&cxl_region_rwsem);
>> -	if (rc)
>> -		return rc;
>> -
>> -	rc = down_read_interruptible(&cxl_dpa_rwsem);
>> -	if (rc) {
>> -		up_read(&cxl_region_rwsem);
>> -		return rc;
>> -	}
>> +	lockdep_assert_held(&cxl_dpa_rwsem);
>> +	lockdep_assert_held(&cxl_region_rwsem);
>>  
>>  	rc = cxl_validate_poison_dpa(cxlmd, dpa);
>>  	if (rc)
>> -		goto out;
>> -
>> +		return rc;
>>  	/*
>>  	 * In CXL 3.0 Spec 8.2.9.8.4.3, the Clear Poison mailbox command
>>  	 * is defined to accept 64 bytes of write-data, along with the
>> @@ -378,7 +382,7 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
>>  
>>  	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
>>  	if (rc)
>> -		goto out;
>> +		return rc;
>>  
>>  	cxlr = cxl_dpa_to_region(cxlmd, dpa);
>>  	if (cxlr)
>> @@ -391,7 +395,26 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
>>  		.length = cpu_to_le32(1),
>>  	};
>>  	trace_cxl_poison(cxlmd, cxlr, &record, 0, 0, CXL_POISON_TRACE_CLEAR);
>> -out:
>> +
>> +	return rc;
>> +}
>> +
>> +int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
>> +{
>> +	int rc;
>> +
>> +	rc = down_read_interruptible(&cxl_region_rwsem);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = down_read_interruptible(&cxl_dpa_rwsem);
>> +	if (rc) {
>> +		up_read(&cxl_region_rwsem);
>> +		return rc;
>> +	}
>> +
>> +	rc = __clear_poison_locked(cxlmd, dpa);
>> +
>>  	up_read(&cxl_dpa_rwsem);
>>  	up_read(&cxl_region_rwsem);
>>  
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index 551b0ba2caa1..c0643e8b9d8f 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -861,6 +861,8 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>>  int cxl_trigger_poison_list(struct cxl_memdev *cxlmd);
>>  int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa);
>>  int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa);
>> +int __inject_poison_locked(struct cxl_memdev *cxlmd, u64 dpa);
>> +int __clear_poison_locked(struct cxl_memdev *cxlmd, u64 dpa);
>>  
>>  #ifdef CONFIG_CXL_EDAC_MEM_FEATURES
>>  int devm_cxl_memdev_edac_register(struct cxl_memdev *cxlmd);
> 

Re: [PATCH 2/3] cxl/region: Introduce HPA to DPA address translation

[Dave Jiang]

On 6/23/25 5:53 PM, alison.schofield@intel.com wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Add infrastructure to translate Host Physical Addresses (HPA) to Device
> Physical Addresses (DPA) within CXL regions. This capability is being
> introduced for use by follow-on patches that will add poison inject
> and clear operations at the region level.
> 
> The HPA-to-DPA translation process involves several steps:
> 1. Apply root decoder transformations (HPA to SPA) if configured
> 2. Calculate the relative offset within the region's address space
> 3. Decode the interleave position using the region's interleave ways
>    and granularity settings
> 4. Identify the target memdev based on the decoded position
> 5. Compute the final DPA by adding the decoded offset to the memdev's
>    DPA base address
> 
> It is Step 1 above that makes this a driver level operation and not
> work we can push to user space. Rather than exporting the XOR maps for
> root decoders configured with XOR interleave, the driver performs this
> complex calculation for the user.
> 
> While not immediately apparent in this diff, broader examination of
> the region.c code shows that this work is basically the reverse of
> previous work where a DPA is translated to an HPA. This is notable
> because it demonstrates that these calculations reuse existing logic
> rather than introducing new algorithms.

Are there any public documentations (or spec section) on explaining the translation you can point to for each of the function segments?

> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/core/region.c | 85 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6e5e1460068d..d2d904c4b427 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2972,6 +2972,91 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
>  	return hpa;
>  }
>  
> +struct hpa_decode_result {
> +	u64 dpa_offset;
> +	int pos;
> +};
> +
> +struct cxl_dpa_result {
> +	u64 dpa;
> +	struct cxl_memdev *cxlmd;
> +};
> +
> +static struct hpa_decode_result decode_hpa_to_dpa(u64 hpa_offset, u8 eiw,

While this is totally valid in C, I'm not big fan on returning a struct. The compiler copies the result from the function's stack to the variable of the caller function. Can we just pass in the addr of the caller's struct to write back and avoid that copy? We are not writing Rust code in CXL yet. :) 

> +						  u16 eig)
> +{
> +	struct hpa_decode_result result;
> +	u64 bits_upper;
> +
> +	if (eiw < 8) {
> +		result.pos = (hpa_offset >> (eig + 8)) & GENMASK(eiw - 1, 0);
> +		hpa_offset &= ~((u64)GENMASK(eiw - 1, 0) << (eig + 8));
> +		result.dpa_offset = hpa_offset >> eiw;
> +	} else {
> +		bits_upper = hpa_offset >> (eig + 8);
> +		result.pos = bits_upper % 3;
> +		bits_upper /= 3;
> +		result.dpa_offset = bits_upper << (eig + 8);
> +	}
> +
> +	result.dpa_offset |= hpa_offset & GENMASK_ULL(eig + 7, 0);
> +
> +	return result;
> +}
> +
> +static struct cxl_dpa_result __maybe_unused

Same comment as above

DJ

> +cxl_hpa_to_dpa(struct cxl_region *cxlr, u64 hpa)
> +{
> +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> +	struct cxl_dpa_result result = { .dpa = ULLONG_MAX, .cxlmd = NULL };
> +	struct cxl_region_params *p = &cxlr->params;
> +	struct hpa_decode_result decode;
> +	u64 hpa_offset;
> +	u16 eig = 0;
> +	u8 eiw = 0;
> +
> +	lockdep_assert_held(&cxl_region_rwsem);
> +	lockdep_assert_held(&cxl_dpa_rwsem);
> +
> +	if (hpa < p->res->start || hpa > p->res->end) {
> +		dev_err_once(&cxlr->dev,
> +			     "HPA 0x%llx not in region [0x%llx-0x%llx]\n", hpa,
> +			     p->res->start, p->res->end);
> +
> +		return result;
> +	}
> +
> +	/* Apply root decoder translation */
> +	if (cxlrd->hpa_to_spa)
> +		hpa = cxlrd->hpa_to_spa(cxlrd, hpa);
> +
> +	ways_to_eiw(p->interleave_ways, &eiw);
> +	granularity_to_eig(p->interleave_granularity, &eig);
> +	hpa_offset = hpa - p->res->start;
> +
> +	decode = decode_hpa_to_dpa(hpa_offset, eiw, eig);
> +	if (decode.pos >= p->nr_targets) {
> +		dev_err(&cxlr->dev, "Invalid position %d for %d targets\n",
> +			decode.pos, p->nr_targets);
> +
> +		return result;
> +	}
> +
> +	for (int i = 0; i < p->nr_targets; i++) {
> +		struct cxl_endpoint_decoder *cxled = p->targets[i];
> +
> +		if (cxled->pos == decode.pos) {
> +			result.cxlmd = cxled_to_memdev(cxled);
> +			result.dpa = decode.dpa_offset + cxl_dpa_resource_start(cxled);
> +
> +			return result;
> +		}
> +	}
> +	dev_err(&cxlr->dev, "No device found for position %d\n", decode.pos);
> +
> +	return result;
> +}
> +
>  static struct lock_class_key cxl_pmem_region_key;
>  
>  static int cxl_pmem_region_alloc(struct cxl_region *cxlr)

Re: [PATCH 3/3] cxl/region: Add inject and clear poison by HPA

[dan.j.williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Add CXL region debugfs attributes to inject and clear poison based
> on Host Physical Addresses (HPA). These new interfaces allow users
> to operate on poison at the region level without needing to resolve
> Device Physical Addresses (DPA) or target individual memdevs.
> 
> The implementation leverages an internal HPA-to-DPA helper, which
> applies decoder interleave logic, including XOR-based address decoding
> when applicable. Note that XOR decodes rely on driver internal xormaps
> which are not exposed to userspace. So, this support is not only a
> simplification of poison operations that could be done using existing
> per memdev operations, but also it enables the functionality for XOR
> interleaved regions for the first time.
> 
> The new debugfs attributes are added under /sys/kernel/debug/regionX/:
> inject_poison and clear_poison. These are only exposed if all memdevs
> participating in the region support both inject and clear commands,
> ensuring consistent and reliable behavior across multi-device regions.
> 
> If tracing is enabled, these operations are logged as cxl_poison
> events in /sys/kernel/tracing/trace.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  Documentation/ABI/testing/debugfs-cxl |  33 +++++++
>  drivers/cxl/core/core.h               |   4 +
>  drivers/cxl/core/memdev.c             |  11 +++
>  drivers/cxl/core/region.c             | 120 +++++++++++++++++++++++++-
>  4 files changed, 166 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/debugfs-cxl b/Documentation/ABI/testing/debugfs-cxl
> index 12488c14be64..c784885cdf07 100644
> --- a/Documentation/ABI/testing/debugfs-cxl
> +++ b/Documentation/ABI/testing/debugfs-cxl
> @@ -35,6 +35,39 @@ Description:
>  		The clear_poison attribute is only visible for devices
>  		supporting the capability.
>  
> +
> +What:		/sys/kernel/debug/regionX/inject_poison
> +Date:		August, 2025
> +KernelVersion:	v6.17

I do not find the KernelVersion line all that useful because git blame
can get you that and sometimes the merged kernel version changes.

> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(WO) When a Host Physical Address (HPA) is written to this
> +		attribute, the region driver translates it to a Device
> +		Physical Address (DPA) and identifies the corresponding
> +		memdev. It then sends an inject poison command to that memdev
> +		at the translated DPA. Refer to the memdev ABI entry at:
> +		/sys/kernel/debug/cxl/memX/inject_poison for the detailed
> +		behavior. This attribute is only visible if all memdevs
> +		participating in the region support both inject and clear
> +		poison commands.
> +
> +
> +What:		/sys/kernel/debug/regionX/clear_poison
> +Date:		August, 2025
> +KernelVersion:	v6.17
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(WO) When a Host Physical Address (HPA) is written to this
> +		attribute, the region driver translates it to a Device
> +		Physical Address (DPA) and identifies the corresponding
> +		memdev. It then sends a clear poison command to that memdev
> +		at the translated DPA. Refer to the memdev ABI entry at:
> +		/sys/kernel/debug/cxl/memX/clear_poison for the detailed
> +		behavior. This attribute is only visible if all memdevs
> +		participating in the region support both inject and clear
> +		poison commands.

A few food for thought comments here:

In the nvdimm subsystem all of the poison addressing is object relative.
I.e. instead of absolute HPA it would be a 0-based region offset. That
might help when we get to platforms that have additional Host Bridge
translation because HPA != SPA in all cases. The existing DPA interface
for memX injection just happens to comply because DPA is 0-based
mem-object relative address. Lets use region-offset values for this new
capability.

This documentation probably needs to be clearer about the data loss
danger here especially when this error inject can permanently destroy
data in the case of CXL PMEM, and crash kernels if this is just memory.
Again, nvdimm was careful to separate "uninject" from "data
repair/recovery" because they are not equivalent. The documentation can
note that this interface is test-only not repair

Re: [PATCH 1/3] cxl/core: Add locked variants of the poison inject and clear funcs

[Alison Schofield]

On Tue, Jun 24, 2025 at 02:38:22PM +0100, Jonathan Cameron wrote:
> On Mon, 23 Jun 2025 17:53:34 -0700
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > The core functions that validate and send inject and clear commands
> > to the memdev devices require holding both the dpa_rwsem and the
> > region_rwsem.
> > 
> > In preparation for another caller of these functions that must hold
> > the locks upon entry, split the work into a locked and unlocked pair.
> > 
> > Consideration was given to moving the locking to both callers,
> > however, the existing caller is not in the core (mem.c) and cannot
> > access the locks.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> 
> This will clash with Dan's ACQUIRE() set.  Up to Dave on how
> to handle that but my gut feeling is that if we want to queue much
> else this cycle and that we should ask for patches on top of the
> ACQUIRE() series.  The risk being that gets some later push back.
> 
> Ah well I'll apply the someone else's problem field.
> 
> Given reuse of these in patch 3 this looks sensible to me.
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Thanks Jonathan - but I'm going to omit your tag and ask you
to take a peek at v2, which I'm rebasing on for-6.17/cxl-acquire.
(I'm not pushing to get this queued for 6.17.)

Re: [PATCH 1/3] cxl/core: Add locked variants of the poison inject and clear funcs

[Alison Schofield]

On Wed, Jun 25, 2025 at 03:15:45PM -0700, Dave Jiang wrote:
> 
> 
> On 6/24/25 6:38 AM, Jonathan Cameron wrote:
> > On Mon, 23 Jun 2025 17:53:34 -0700
> > alison.schofield@intel.com wrote:
> > 
> >> From: Alison Schofield <alison.schofield@intel.com>
> >>
> >> The core functions that validate and send inject and clear commands
> >> to the memdev devices require holding both the dpa_rwsem and the
> >> region_rwsem.
> >>
> >> In preparation for another caller of these functions that must hold
> >> the locks upon entry, split the work into a locked and unlocked pair.
> >>
> >> Consideration was given to moving the locking to both callers,
> >> however, the existing caller is not in the core (mem.c) and cannot
> >> access the locks.
> >>
> >> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > 
> > This will clash with Dan's ACQUIRE() set.  Up to Dave on how
> > to handle that but my gut feeling is that if we want to queue much
> > else this cycle and that we should ask for patches on top of the
> > ACQUIRE() series.  The risk being that gets some later push back.
> 
> I think this patch may simplify more with ACQUIRE() if Dan hasn't already refactored the function. We can wait for a rev based off of that given there's an immutable branch with the ACQUIRE() patch on cxl.git already.
> 
> DJ

rebase coming soon in v2 ;)

Re: [PATCH 2/3] cxl/region: Introduce HPA to DPA address translation

[Alison Schofield]

On Tue, Jun 24, 2025 at 03:27:16PM +0100, Jonathan Cameron wrote:
> On Mon, 23 Jun 2025 17:53:35 -0700
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Add infrastructure to translate Host Physical Addresses (HPA) to Device
> > Physical Addresses (DPA) within CXL regions. This capability is being
> > introduced for use by follow-on patches that will add poison inject
> > and clear operations at the region level.
> > 
> > The HPA-to-DPA translation process involves several steps:
> > 1. Apply root decoder transformations (HPA to SPA) if configured
> > 2. Calculate the relative offset within the region's address space
> > 3. Decode the interleave position using the region's interleave ways
> >    and granularity settings
> > 4. Identify the target memdev based on the decoded position
> > 5. Compute the final DPA by adding the decoded offset to the memdev's
> >    DPA base address
> > 
> > It is Step 1 above that makes this a driver level operation and not
> > work we can push to user space. Rather than exporting the XOR maps for
> > root decoders configured with XOR interleave, the driver performs this
> > complex calculation for the user.
> > 
> > While not immediately apparent in this diff, broader examination of
> > the region.c code shows that this work is basically the reverse of
> > previous work where a DPA is translated to an HPA. This is notable
> > because it demonstrates that these calculations reuse existing logic
> > rather than introducing new algorithms.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Hi Alison,
> 
> This maths still gives me a headache, but I don't follow at least some of it.
> Also I'm missing the application of the XOR Map referred to above.

It's there. You mention it again below, so I'll comment there.

> 
> > ---
> >  drivers/cxl/core/region.c | 85 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 85 insertions(+)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 6e5e1460068d..d2d904c4b427 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -2972,6 +2972,91 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> >  	return hpa;
> >  }
> >  
> > +struct hpa_decode_result {
> > +	u64 dpa_offset;
> > +	int pos;
> > +};
> > +
> > +struct cxl_dpa_result {
> > +	u64 dpa;
> > +	struct cxl_memdev *cxlmd;
> > +};
> > +
> > +static struct hpa_decode_result decode_hpa_to_dpa(u64 hpa_offset, u8 eiw,
> Having a function called hpa_to_dpa that takes an hpa_offset seems inconsistent.
> 
> decode_hpa_offset_to_dpa() maybe?

The naming and the layering didn't survive my v2.
I replaced the whole shbang with region_offset_to_dpa_result()
and collect the pos and dpa_offset pieces inline.

Let me know how that flows when you see it.

> 
> > +						  u16 eig)
> > +{
> > +	struct hpa_decode_result result;
> > +	u64 bits_upper;
> > +
> > +	if (eiw < 8) {
> Could add a spec reference. Other than that, this block looks fine to me.
> > +		result.pos = (hpa_offset >> (eig + 8)) & GENMASK(eiw - 1, 0);
> > +		hpa_offset &= ~((u64)GENMASK(eiw - 1, 0) << (eig + 8));
> > +		result.dpa_offset = hpa_offset >> eiw;
> > +	} else {
> 
> This block isn't lining up for me with what is documented in step 2 of the
> implementation note on decoding.
> 
> > +		bits_upper = hpa_offset >> (eig + 8);
> ignoring pos calc as that is confusingly in a very different place
> in the spec, we should have
> DPA_OFFSET[51:IG + 8] = HPA_OFFSET[51:IG + IW] / 3 
> 
> So I'm missing a shift by IW somewhere.
> Perhaps some more comments would help relate this to the maths.

A couple of responses here -
- yes this shift "bits_upper = hpa_offset >> (eig + 8);"
  should have be (eig + eiw). I've added a MOD3 region to the
  unit test to catch this case.

- in v2 in addition to fixing that up I did a couple of changes
  that I hope make it easier to align with the spec.

  I move all the work inline, well that just makes it easier
  to read, and separated the work of getting the 'pos' and
  the work of getting the dpa_offset.

  I also spit out the spec comments verbatim as I do each step.
  I discarded my original mindset and recommendation to look
  at the dpa-to-hpa translation and see that this reverses it,
  because that is actually harder to do!  This is the translation
  that is documented in the spec - so no need for any reverse
  thinking.

> 
> > +		result.pos = bits_upper % 3;
> > +		bits_upper /= 3;
> > +		result.dpa_offset = bits_upper << (eig + 8);
> 
> 
> 
> > +	}
> > +
> > +	result.dpa_offset |= hpa_offset & GENMASK_ULL(eig + 7, 0);
> 
> I wonder if it is simpler to keep a copy of these bottom bits from
> before all the manipulation above.  They make it through untouched
> but we could avoid the reader having to figure that out.

Done.

> 
> 
> > +
> > +	return result;
> > +}
> > +
> > +static struct cxl_dpa_result __maybe_unused
> > +cxl_hpa_to_dpa(struct cxl_region *cxlr, u64 hpa)
> > +{
> > +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> > +	struct cxl_dpa_result result = { .dpa = ULLONG_MAX, .cxlmd = NULL };
> > +	struct cxl_region_params *p = &cxlr->params;
> > +	struct hpa_decode_result decode;
> > +	u64 hpa_offset;
> > +	u16 eig = 0;
> > +	u8 eiw = 0;
> > +
> > +	lockdep_assert_held(&cxl_region_rwsem);
> > +	lockdep_assert_held(&cxl_dpa_rwsem);
> > +
> > +	if (hpa < p->res->start || hpa > p->res->end) {
> > +		dev_err_once(&cxlr->dev,
> > +			     "HPA 0x%llx not in region [0x%llx-0x%llx]\n", hpa,
> > +			     p->res->start, p->res->end);
> > +
> > +		return result;
> > +	}
> > +
> > +	/* Apply root decoder translation */
> > +	if (cxlrd->hpa_to_spa)
> > +		hpa = cxlrd->hpa_to_spa(cxlrd, hpa);
> 
> Is this in the right direction?  I was rather expecting opposite of what
> is going on in cxl_dpa_to_hpa()
> 

For the XOR root decoder translation, the existing function is
self-inverting, so this just works. However, the root decoder
callback is not defined as such, so I've added a complentary
root decoder callback in v2, that looks like this:
in struct cxl_root_decoder
	cxl_hpa_to_spa_fn hpa_to_spa;
+	cxl_spa_to_hpa_fn spa_to_hpa;


> Perhaps separate spa and hpa in here to improve readability with
> spa == hpa for the case where we don't have a transaltion.
> 
> > +
> > +	ways_to_eiw(p->interleave_ways, &eiw);
> > +	granularity_to_eig(p->interleave_granularity, &eig);
> > +	hpa_offset = hpa - p->res->start;
> 
> Given people will line this code up with cxl_dpa_to_hpa() maybe a
> comment to rule out subtracting cache_size?
> 
> > +
> > +	decode = decode_hpa_to_dpa(hpa_offset, eiw, eig);
> > +	if (decode.pos >= p->nr_targets) {
> > +		dev_err(&cxlr->dev, "Invalid position %d for %d targets\n",
> > +			decode.pos, p->nr_targets);
> > +
> > +		return result;
> > +	}
> > +
> > +	for (int i = 0; i < p->nr_targets; i++) {
> > +		struct cxl_endpoint_decoder *cxled = p->targets[i];
> > +
> > +		if (cxled->pos == decode.pos) {
> > +			result.cxlmd = cxled_to_memdev(cxled);
> > +			result.dpa = decode.dpa_offset + cxl_dpa_resource_start(cxled);
> > +
> > +			return result;
> > +		}
> > +	}
> > +	dev_err(&cxlr->dev, "No device found for position %d\n", decode.pos);
> > +
> > +	return result;
> > +}
> > +
> >  static struct lock_class_key cxl_pmem_region_key;
> >  
> >  static int cxl_pmem_region_alloc(struct cxl_region *cxlr)
> 

Re: [PATCH 2/3] cxl/region: Introduce HPA to DPA address translation

[Alison Schofield]

On Wed, Jun 25, 2025 at 03:49:37PM -0700, Dave Jiang wrote:
> 
> 
> On 6/23/25 5:53 PM, alison.schofield@intel.com wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Add infrastructure to translate Host Physical Addresses (HPA) to Device
> > Physical Addresses (DPA) within CXL regions. This capability is being
> > introduced for use by follow-on patches that will add poison inject
> > and clear operations at the region level.
> > 
> > The HPA-to-DPA translation process involves several steps:
> > 1. Apply root decoder transformations (HPA to SPA) if configured
> > 2. Calculate the relative offset within the region's address space
> > 3. Decode the interleave position using the region's interleave ways
> >    and granularity settings
> > 4. Identify the target memdev based on the decoded position
> > 5. Compute the final DPA by adding the decoded offset to the memdev's
> >    DPA base address
> > 
> > It is Step 1 above that makes this a driver level operation and not
> > work we can push to user space. Rather than exporting the XOR maps for
> > root decoders configured with XOR interleave, the driver performs this
> > complex calculation for the user.
> > 
> > While not immediately apparent in this diff, broader examination of
> > the region.c code shows that this work is basically the reverse of
> > previous work where a DPA is translated to an HPA. This is notable
> > because it demonstrates that these calculations reuse existing logic
> > rather than introducing new algorithms.
> 
> Are there any public documentations (or spec section) on explaining the translation you can point to for each of the function segments?

Yes. Will include in the v2 update.  Also, as I mentioned in reply to
Jonathan, I've backed away from the just reverse the dpa->hpa
messaging. The reason is that the spec defines exactly what to do
for hpa->dpa - which is what we are doing here. It's simpler to just
look at that, rather than to reverse what we reversed for dpa->hpa.
Clear as mud ?!

> 
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/core/region.c | 85 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 85 insertions(+)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 6e5e1460068d..d2d904c4b427 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -2972,6 +2972,91 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> >  	return hpa;
> >  }
> >  
> > +struct hpa_decode_result {
> > +	u64 dpa_offset;
> > +	int pos;
> > +};
> > +
> > +struct cxl_dpa_result {
> > +	u64 dpa;
> > +	struct cxl_memdev *cxlmd;
> > +};
> > +
> > +static struct hpa_decode_result decode_hpa_to_dpa(u64 hpa_offset, u8 eiw,
> 
> While this is totally valid in C, I'm not big fan on returning a struct. The compiler copies the result from the function's stack to the variable of the caller function. Can we just pass in the addr of the caller's struct to write back and avoid that copy? We are not writing Rust code in CXL yet. :) 
> 

Done with the func that remains in v2. (One got folded into the other)

Re: [PATCH 3/3] cxl/region: Add inject and clear poison by HPA

[Alison Schofield]

On Wed, Jun 25, 2025 at 04:05:35PM -0700, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Add CXL region debugfs attributes to inject and clear poison based
> > on Host Physical Addresses (HPA). These new interfaces allow users
> > to operate on poison at the region level without needing to resolve
> > Device Physical Addresses (DPA) or target individual memdevs.
> > 
> > The implementation leverages an internal HPA-to-DPA helper, which
> > applies decoder interleave logic, including XOR-based address decoding
> > when applicable. Note that XOR decodes rely on driver internal xormaps
> > which are not exposed to userspace. So, this support is not only a
> > simplification of poison operations that could be done using existing
> > per memdev operations, but also it enables the functionality for XOR
> > interleaved regions for the first time.
> > 
> > The new debugfs attributes are added under /sys/kernel/debug/regionX/:
> > inject_poison and clear_poison. These are only exposed if all memdevs
> > participating in the region support both inject and clear commands,
> > ensuring consistent and reliable behavior across multi-device regions.
> > 
> > If tracing is enabled, these operations are logged as cxl_poison
> > events in /sys/kernel/tracing/trace.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  Documentation/ABI/testing/debugfs-cxl |  33 +++++++
> >  drivers/cxl/core/core.h               |   4 +
> >  drivers/cxl/core/memdev.c             |  11 +++
> >  drivers/cxl/core/region.c             | 120 +++++++++++++++++++++++++-
> >  4 files changed, 166 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/debugfs-cxl b/Documentation/ABI/testing/debugfs-cxl
> > index 12488c14be64..c784885cdf07 100644
> > --- a/Documentation/ABI/testing/debugfs-cxl
> > +++ b/Documentation/ABI/testing/debugfs-cxl
> > @@ -35,6 +35,39 @@ Description:
> >  		The clear_poison attribute is only visible for devices
> >  		supporting the capability.
> >  
> > +
> > +What:		/sys/kernel/debug/regionX/inject_poison
> > +Date:		August, 2025
> > +KernelVersion:	v6.17
> 
> I do not find the KernelVersion line all that useful because git blame
> can get you that and sometimes the merged kernel version changes.

Removed it.

> 
> > +Contact:	linux-cxl@vger.kernel.org
> > +Description:
> > +		(WO) When a Host Physical Address (HPA) is written to this
> > +		attribute, the region driver translates it to a Device
> > +		Physical Address (DPA) and identifies the corresponding
> > +		memdev. It then sends an inject poison command to that memdev
> > +		at the translated DPA. Refer to the memdev ABI entry at:
> > +		/sys/kernel/debug/cxl/memX/inject_poison for the detailed
> > +		behavior. This attribute is only visible if all memdevs
> > +		participating in the region support both inject and clear
> > +		poison commands.
> > +
> > +
> > +What:		/sys/kernel/debug/regionX/clear_poison
> > +Date:		August, 2025
> > +KernelVersion:	v6.17
> > +Contact:	linux-cxl@vger.kernel.org
> > +Description:
> > +		(WO) When a Host Physical Address (HPA) is written to this
> > +		attribute, the region driver translates it to a Device
> > +		Physical Address (DPA) and identifies the corresponding
> > +		memdev. It then sends a clear poison command to that memdev
> > +		at the translated DPA. Refer to the memdev ABI entry at:
> > +		/sys/kernel/debug/cxl/memX/clear_poison for the detailed
> > +		behavior. This attribute is only visible if all memdevs
> > +		participating in the region support both inject and clear
> > +		poison commands.
> 
> A few food for thought comments here:
> 
> In the nvdimm subsystem all of the poison addressing is object relative.
> I.e. instead of absolute HPA it would be a 0-based region offset. That
> might help when we get to platforms that have additional Host Bridge
> translation because HPA != SPA in all cases. The existing DPA interface
> for memX injection just happens to comply because DPA is 0-based
> mem-object relative address. Lets use region-offset values for this new
> capability.

Done.

> 
> This documentation probably needs to be clearer about the data loss
> danger here especially when this error inject can permanently destroy
> data in the case of CXL PMEM, and crash kernels if this is just memory.
> Again, nvdimm was careful to separate "uninject" from "data
> repair/recovery" because they are not equivalent. The documentation can
> note that this interface is test-only not repair

We talked thru much of this when we introduced inject/clear poison by
memdev, because they allow the operations in active regions. This new
method also allows the operation in active regions with the added
benefit of finding the correct DPA in XOR cases. 

The clear, as implemented previously and here, is not an uninject.
The user may clear any address they'd like. We do not restrict the
clear operation to addresses that have poison injected, and the
spec doesn't require that an address have poison at all. It just
quietly does nothing if the clear is sent to an address with no
poison.

I can make this documentation louder, and also be more shouty
in the original by memdev docs that I reference here.

If we wanted to offer 'uninject' then we'd read the poison list
and confirm that there is poison of type 'Injected' before 
sending the clear. That is not this patchset.

Does anyone think it should be?  Which btw would mean we'd
backtrack on existing by memdev offering.

Re: [PATCH 3/3] cxl/region: Add inject and clear poison by HPA

[Alison Schofield]

On Tue, Jun 24, 2025 at 03:33:59PM +0100, Jonathan Cameron wrote:
> On Mon, 23 Jun 2025 17:53:36 -0700
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 

snip

> > +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> > +
> > +	if (test_bit(cmd, mds->poison.enabled_cmds))
> > +		return true;
> > +
> > +	return false;
> 
> 	return test_bit(cmd, ...);
> 

Done.


> > +
> > +	rc = down_read_interruptible(&cxl_region_rwsem);
> 
> This can use the new shiny ACQUIRE() tool letting us do
> early returns and generally making it more readable.

Done.

snip
> >  
> > +	/* Create poison attributes if all memdevs support the capabilities */
> > +	for (int i = 0; i < p->nr_targets; i++) {
> > +		struct cxl_endpoint_decoder *cxled = p->targets[i];
> > +		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +
> > +		if (!cxl_memdev_has_poison_cmd(cxlmd, CXL_POISON_ENABLED_INJECT) ||
> > +		    !cxl_memdev_has_poison_cmd(cxlmd, CXL_POISON_ENABLED_CLEAR)) {
> > +			poison_supported = false;
> You could drop i out of the loop and simply use the early break to detect
> that a match occurred.
> 
> 	if (i < p->nr_targets) {
> 		struct dentry *dentry;
> 
> but maybe that's a bit too subtle.  I don't mind having a variable for this.

Left it as is. (I have gotten similar feedback on this pattern
elsewhere, but here I really like the clarity of it.)

Re: [PATCH 2/3] cxl/region: Introduce HPA to DPA address translation

[Alison Schofield]

On Tue, Jun 24, 2025 at 03:27:16PM +0100, Jonathan Cameron wrote:
> On Mon, 23 Jun 2025 17:53:35 -0700
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 

snip --

Another response to your review that I cut from previous reply - 

> > +
> > +	ways_to_eiw(p->interleave_ways, &eiw);
> > +	granularity_to_eig(p->interleave_granularity, &eig);
> > +	hpa_offset = hpa - p->res->start;
> 
> Given people will line this code up with cxl_dpa_to_hpa() maybe a
> comment to rule out subtracting cache_size?
> 

In v2 we make sure that the offset is not within the extended linear
cache.  Not sure about a comment here, since I change 'here' in v2.
Please take another look in v2.