[RFC PATCH 0/3] cxl: Auto-create a region for Type-2 memdev attach

[RFC PATCH 0/3] cxl: Auto-create a region for Type-2 memdev attach

[Richard Cheng]

A Type-2 accelerator driver calls devm_cxl_probe_mem() to register its
memdev and get an HPA range, but today only if FW already committed a
region. Real accelerators can have usable memory with no committed decoder,
so get nothing.

If no FW region is mapped, the core picks the device's unused manual
DEVMEM decoder and a compatible x1 Type-2 RAM root, allocates the full
volatile DPA and HPA, commits the decoder, and returns the range. Unbind
resets and removes it. Strict first cut with single decoder, IW=1, minimum
granularity, first-compatible root.

The design intent is that the provider F_LOCKs its region against userspace
but must reset its own software region on detach. A plain flag would also
allow reset on generic kill/delete paths, so we thread a reset context
through teardown and commit rollback. devm_cxl_probe_mem() may now commit
decoders.

Testing result is in the following.
- Built clean with clang/LLVM on arm64
- cxl_test, type2_test=1. accel0 takes the unchanged attach path. accel1
  drives auto_create -> a committed 512 MB RAM region. The test asserts
  the 512 MB HPA range. committed state and 256 byte granularity confirmed
  via sysfs.
- Unbind tears the region down with no orphaned decoder, rebind re-creates
  a fresh committed region.
- Mock test only. Real accelerators whose FW commits a decoder take the
  attach path, and vfio-cxl binds only FW-committed devices, so auto-create
  has no real-HW caller yet.

Best regards,
Richard Cheng.

Richard Cheng (3):
  cxl/region: Reset software-created regions on memdev detach
  cxl/region: Auto-create a region for memdev attach
  cxl/test: Exercise Type-2 automatic region creation

 drivers/cxl/core/region.c      | 422 +++++++++++++++++++++++++++++----
 tools/testing/cxl/test/accel.c |   7 +
 tools/testing/cxl/test/cxl.c   |  61 ++++-
 3 files changed, 439 insertions(+), 51 deletions(-)


base-commit: 1c6b4ceafc3b994871c29340e0c1ddb0af5800e7
-- 
2.43.0

[RFC PATCH 1/3] cxl/region: Reset software-created regions on memdev detach

[Richard Cheng]

A memdev attach provider locks its region to prevent userspace
disposition. Future software-created Type-2 regions inherit that lock,
causing the existing reset path to treat them like FW-owned or HW-locked
regions and leave their decoder programming behind when the provider
detaches.

Pass a managed-detach context through region teardown. In that context,
allow reset only when the region is non-AUTO, owned by a memdev attach
provider, and has no HW-locked decoder in its path. This narrowly
permits the provider to reset its own software-created region without
changing generic teardown behavior.

Generic teardown remains unchanged, while FW-discovered AUTO regions and
genuinely locked decoders remain protected.

Signed-off-by: Richard Cheng <icheng@nvidia.com>
---
 drivers/cxl/core/region.c | 112 +++++++++++++++++++++++++++++++-------
 1 file changed, 91 insertions(+), 21 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 1e211542b6b6..7fcaddc61180 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -39,6 +39,7 @@
 static nodemask_t nodemask_region_seen = NODE_MASK_NONE;
 
 static struct cxl_region *to_cxl_region(struct device *dev);
+static bool cxl_region_has_memdev_attach(struct cxl_region *cxlr);
 
 #define __ACCESS_ATTR_RO(_level, _name) {				\
 	.attr	= { .name = __stringify(_name), .mode = 0444 },		\
@@ -222,6 +223,45 @@ static struct cxl_region_ref *cxl_rr_load(struct cxl_port *port,
 	return xa_load(&port->regions, (unsigned long)cxlr);
 }
 
+static bool cxl_region_has_locked_decoder(struct cxl_region *cxlr)
+{
+	struct cxl_region_params *p = &cxlr->params;
+	int i;
+
+	lockdep_assert_held_write(&cxl_rwsem.region);
+
+	if (cxlr->cxlrd->cxlsd.cxld.flags & CXL_DECODER_F_LOCK)
+		return true;
+
+	for (i = 0; i < p->interleave_ways; i++) {
+		struct cxl_endpoint_decoder *cxled = p->targets[i];
+		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+		struct cxl_port *iter = cxled_to_port(cxled);
+		struct cxl_dev_state *cxlds = cxlmd->cxlds;
+		struct cxl_ep *ep;
+
+		if (cxled->cxld.flags & CXL_DECODER_F_LOCK)
+			return true;
+
+		if (cxlds->rcd)
+			continue;
+
+		while (!is_cxl_root(to_cxl_port(iter->dev.parent)))
+			iter = to_cxl_port(iter->dev.parent);
+
+		for (ep = cxl_ep_load(iter, cxlmd); iter;
+		     iter = ep->next, ep = cxl_ep_load(iter, cxlmd)) {
+			struct cxl_region_ref *cxl_rr;
+
+			cxl_rr = cxl_rr_load(iter, cxlr);
+			if (cxl_rr->decoder->flags & CXL_DECODER_F_LOCK)
+				return true;
+		}
+	}
+
+	return false;
+}
+
 static int cxl_region_invalidate_memregion(struct cxl_region *cxlr)
 {
 	if (!cpu_cache_has_invalidate_memregion()) {
@@ -243,12 +283,27 @@ static int cxl_region_invalidate_memregion(struct cxl_region *cxlr)
 	return 0;
 }
 
-static void cxl_region_decode_reset(struct cxl_region *cxlr, int count)
+enum cxl_region_reset_context {
+	CXL_REGION_RESET_DEFAULT,
+	CXL_REGION_RESET_MANAGED_DETACH,
+};
+
+static void cxl_region_decode_reset(struct cxl_region *cxlr, int count,
+				    enum cxl_region_reset_context context)
 {
 	struct cxl_region_params *p = &cxlr->params;
 	int i;
 
-	if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
+	/*
+	 * An attach provider locks a region against userspace disposition, but
+	 * a non-auto attach-owned region is software-owned and needs reset on
+	 * managed detach, unless its decoder path has a genuine hardware lock.
+	 */
+	if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags) &&
+	    (context != CXL_REGION_RESET_MANAGED_DETACH ||
+	     test_bit(CXL_REGION_F_AUTO, &cxlr->flags) ||
+	     !cxl_region_has_memdev_attach(cxlr) ||
+	     cxl_region_has_locked_decoder(cxlr)))
 		return;
 
 	/*
@@ -350,7 +405,7 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
 
 err:
 	/* undo the targets that were successfully committed */
-	cxl_region_decode_reset(cxlr, i);
+	cxl_region_decode_reset(cxlr, i, CXL_REGION_RESET_DEFAULT);
 	return rc;
 }
 
@@ -449,7 +504,8 @@ static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
 	 * thread already handled this reset.
 	 */
 	if (p->state == CXL_CONFIG_RESET_PENDING) {
-		cxl_region_decode_reset(cxlr, p->interleave_ways);
+		cxl_region_decode_reset(cxlr, p->interleave_ways,
+					CXL_REGION_RESET_DEFAULT);
 		p->state = CXL_CONFIG_ACTIVE;
 	}
 
@@ -2268,7 +2324,8 @@ static void cxl_cancel_auto_attach(struct cxl_endpoint_decoder *cxled)
 static struct cxl_region *
 __cxl_decoder_detach(struct cxl_region *cxlr,
 		     struct cxl_endpoint_decoder *cxled, int pos,
-		     enum cxl_detach_mode mode)
+		     enum cxl_detach_mode mode,
+		     enum cxl_region_reset_context context)
 {
 	struct cxl_region_params *p;
 
@@ -2299,7 +2356,8 @@ __cxl_decoder_detach(struct cxl_region *cxlr,
 		cxled->part = -1;
 
 	if (p->state > CXL_CONFIG_ACTIVE) {
-		cxl_region_decode_reset(cxlr, p->interleave_ways);
+		cxl_region_decode_reset(cxlr, p->interleave_ways,
+					context);
 		p->state = CXL_CONFIG_ACTIVE;
 	}
 
@@ -2340,23 +2398,25 @@ __cxl_decoder_detach(struct cxl_region *cxlr,
  *
  * When the detachment finds a region release the region driver.
  */
-int cxl_decoder_detach(struct cxl_region *cxlr,
-		       struct cxl_endpoint_decoder *cxled, int pos,
-		       enum cxl_detach_mode mode)
+static int cxl_decoder_detach_context(
+	struct cxl_region *cxlr, struct cxl_endpoint_decoder *cxled, int pos,
+	enum cxl_detach_mode mode, enum cxl_region_reset_context context)
 {
 	struct cxl_region *detach;
 
 	/* when the decoder is being destroyed lock unconditionally */
 	if (mode == DETACH_INVALIDATE) {
 		guard(rwsem_write)(&cxl_rwsem.region);
-		detach = __cxl_decoder_detach(cxlr, cxled, pos, mode);
+		detach = __cxl_decoder_detach(cxlr, cxled, pos, mode,
+					      context);
 	} else {
 		int rc;
 
 		ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
 		if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
 			return rc;
-		detach = __cxl_decoder_detach(cxlr, cxled, pos, mode);
+		detach = __cxl_decoder_detach(cxlr, cxled, pos, mode,
+					      context);
 	}
 
 	if (detach) {
@@ -2366,6 +2426,14 @@ int cxl_decoder_detach(struct cxl_region *cxlr,
 	return 0;
 }
 
+int cxl_decoder_detach(struct cxl_region *cxlr,
+		       struct cxl_endpoint_decoder *cxled, int pos,
+		       enum cxl_detach_mode mode)
+{
+	return cxl_decoder_detach_context(cxlr, cxled, pos, mode,
+					  CXL_REGION_RESET_DEFAULT);
+}
+
 static int __attach_target(struct cxl_region *cxlr,
 			   struct cxl_endpoint_decoder *cxled, int pos,
 			   unsigned int state)
@@ -2398,9 +2466,10 @@ static int attach_target(struct cxl_region *cxlr,
 	return rc;
 }
 
-static int detach_target(struct cxl_region *cxlr, int pos)
+static int detach_target(struct cxl_region *cxlr, int pos,
+			 enum cxl_region_reset_context context)
 {
-	return cxl_decoder_detach(cxlr, NULL, pos, DETACH_ONLY);
+	return cxl_decoder_detach_context(cxlr, NULL, pos, DETACH_ONLY, context);
 }
 
 static size_t store_targetN(struct cxl_region *cxlr, const char *buf, int pos,
@@ -2409,7 +2478,7 @@ static size_t store_targetN(struct cxl_region *cxlr, const char *buf, int pos,
 	int rc;
 
 	if (sysfs_streq(buf, "\n"))
-		rc = detach_target(cxlr, pos);
+		rc = detach_target(cxlr, pos, CXL_REGION_RESET_DEFAULT);
 	else {
 		struct device *dev;
 
@@ -2559,7 +2628,8 @@ static struct cxl_region *to_cxl_region(struct device *dev)
 	return container_of(dev, struct cxl_region, dev);
 }
 
-static void unregister_region(struct cxl_region *cxlr)
+static void unregister_region(struct cxl_region *cxlr,
+			      enum cxl_region_reset_context context)
 {
 	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
 	struct cxl_region_params *p = &cxlr->params;
@@ -2574,7 +2644,7 @@ static void unregister_region(struct cxl_region *cxlr)
 	 * region parameters.
 	 */
 	for (i = 0; i < p->interleave_ways; i++)
-		detach_target(cxlr, i);
+		detach_target(cxlr, i, context);
 
 	cxlr->hpa_range = DEFINE_RANGE(0, -1);
 
@@ -2589,7 +2659,7 @@ static void endpoint_unregister_region(void *_cxlr)
 
 	guard(mutex)(&cxlrd->regions_lock);
 	if (xa_load(&cxlrd->regions, cxlr->id))
-		unregister_region(cxlr);
+		unregister_region(cxlr, CXL_REGION_RESET_MANAGED_DETACH);
 	put_device(&cxlr->dev);
 }
 
@@ -2717,7 +2787,7 @@ void kill_regions(struct cxl_root_decoder *cxlrd)
 	/* no more region creation */
 	cxlrd->dead = true;
 	xa_for_each(&cxlrd->regions, index, cxlr)
-		unregister_region(cxlr);
+		unregister_region(cxlr, CXL_REGION_RESET_DEFAULT);
 }
 
 /**
@@ -2760,7 +2830,7 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
 
 	rc = xa_insert(&cxlrd->regions, cxlr->id, cxlr, GFP_KERNEL);
 	if (rc) {
-		unregister_region(cxlr);
+		unregister_region(cxlr, CXL_REGION_RESET_DEFAULT);
 		return ERR_PTR(rc);
 	}
 
@@ -2893,7 +2963,7 @@ static ssize_t delete_region_store(struct device *dev,
 	if (!cxlr || !sysfs_streq(buf, dev_name(&cxlr->dev)))
 		return -ENODEV;
 
-	unregister_region(cxlr);
+	unregister_region(cxlr, CXL_REGION_RESET_DEFAULT);
 
 	return len;
 }
@@ -3781,7 +3851,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 
 	rc = __construct_region(cxlr, ctx);
 	if (rc) {
-		unregister_region(cxlr);
+		unregister_region(cxlr, CXL_REGION_RESET_DEFAULT);
 		return ERR_PTR(rc);
 	}
 
-- 
2.43.0

[RFC PATCH 2/3] cxl/region: Auto-create a region for memdev attach

[Richard Cheng]

devm_cxl_probe_mem() currently fails when FW has not committed a region,
even when a Type-2 accelerator has usable CXL.mem capacity.

When no mapped decoder exists, select a pristine manual DEVMEM decoder
and the first compatible unlocked Type-2 RAM root decoder. Create a
non-AUTO, single-target region, allocate HPA and the full volatile DPA
partition, attach and commit the decoder path, then return the resulting
HPA range.

Use provider-managed reset for partial-commit rollback and unwind
region, HPA, DPA, and partition state in reverse order on failure.
Preserve the existing FW-precommitted path.

This support is limited to decoder 0, IW=1, and first-compatible root
selection.

Signed-off-by: Richard Cheng <icheng@nvidia.com>
---
 drivers/cxl/core/region.c | 312 ++++++++++++++++++++++++++++++++++----
 1 file changed, 284 insertions(+), 28 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 7fcaddc61180..4ceabdfdd3b6 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -363,7 +363,8 @@ static int commit_decoder(struct cxl_decoder *cxld)
 	return 0;
 }
 
-static int cxl_region_decode_commit(struct cxl_region *cxlr)
+static int cxl_region_decode_commit(
+	struct cxl_region *cxlr, enum cxl_region_reset_context context)
 {
 	struct cxl_region_params *p = &cxlr->params;
 	int i, rc = 0;
@@ -405,7 +406,7 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
 
 err:
 	/* undo the targets that were successfully committed */
-	cxl_region_decode_reset(cxlr, i, CXL_REGION_RESET_DEFAULT);
+	cxl_region_decode_reset(cxlr, i, context);
 	return rc;
 }
 
@@ -427,7 +428,8 @@ static int queue_reset(struct cxl_region *cxlr)
 	return 0;
 }
 
-static int __commit(struct cxl_region *cxlr)
+static int __commit_context(struct cxl_region *cxlr,
+			    enum cxl_region_reset_context context)
 {
 	struct cxl_region_params *p = &cxlr->params;
 	int rc;
@@ -452,7 +454,7 @@ static int __commit(struct cxl_region *cxlr)
 	if (rc)
 		return rc;
 
-	rc = cxl_region_decode_commit(cxlr);
+	rc = cxl_region_decode_commit(cxlr, context);
 	if (rc)
 		return rc;
 
@@ -461,6 +463,11 @@ static int __commit(struct cxl_region *cxlr)
 	return 0;
 }
 
+static int __commit(struct cxl_region *cxlr)
+{
+	return __commit_context(cxlr, CXL_REGION_RESET_DEFAULT);
+}
+
 static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t len)
 {
@@ -4177,45 +4184,271 @@ static int first_mapped_decoder(struct device *dev, const void *data)
 	return 0;
 }
 
+static int first_attach_decoder(struct device *dev, const void *data)
+{
+	struct cxl_port *endpoint = (struct cxl_port *)data;
+	struct cxl_endpoint_decoder *cxled;
+	struct cxl_decoder *cxld;
+
+	if (!is_endpoint_decoder(dev))
+		return 0;
+
+	cxled = to_cxl_endpoint_decoder(dev);
+	cxld = &cxled->cxld;
+	if (cxld->id != 0 || cxled->state != CXL_DECODER_STATE_MANUAL ||
+	    cxld->target_type != CXL_DECODER_DEVMEM || cxld->region ||
+	    cxled->dpa_res ||
+	    (cxld->flags & (CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK)))
+		return 0;
+
+	if (endpoint->hdm_end != -1 || cxl_num_decoders_committed(endpoint))
+		return 0;
+
+	return 1;
+}
+
+static int first_attach_root_decoder(struct device *dev, const void *data)
+{
+	struct cxl_port *endpoint = (struct cxl_port *)data;
+	unsigned long required = CXL_DECODER_F_TYPE2 | CXL_DECODER_F_RAM |
+				 CXL_DECODER_F_ENABLE;
+	struct cxl_root_decoder *cxlrd;
+	struct cxl_switch_decoder *cxlsd;
+	struct cxl_decoder *cxld;
+	struct cxl_dport *dport;
+
+	if (!is_root_decoder(dev) || !device_is_registered(dev))
+		return 0;
+
+	cxlrd = to_cxl_root_decoder(dev);
+	cxlsd = &cxlrd->cxlsd;
+	cxld = &cxlsd->cxld;
+	if (cxlrd->dead || !cxlrd->res ||
+	    (cxld->flags & required) != required ||
+	    (cxld->flags & CXL_DECODER_F_LOCK) ||
+	    cxld->interleave_ways != 1 || cxlsd->nr_targets < 1)
+		return 0;
+
+	dport = cxl_find_dport_by_dev(cxlrd_to_port(cxlrd),
+				       endpoint->host_bridge);
+	return dport && cxlsd->target[0] == dport;
+}
+
+static struct cxl_root_decoder *
+find_attach_root_decoder(struct cxl_endpoint_decoder *cxled)
+{
+	struct cxl_port *endpoint = cxled_to_port(cxled);
+	struct cxl_root *root __free(put_cxl_root) = find_cxl_root(endpoint);
+	struct device *dev;
+
+	if (!root)
+		return ERR_PTR(-ENXIO);
+
+	/* First compatible x1 Type-2 window is strict v1 policy. */
+	dev = device_find_child(&root->port.dev, endpoint,
+				first_attach_root_decoder);
+	if (!dev)
+		return ERR_PTR(-ENXIO);
+
+	return to_cxl_root_decoder(dev);
+}
+
+static void restore_attach_decoder_part(struct cxl_endpoint_decoder *cxled,
+					int old_part)
+{
+	guard(rwsem_write)(&cxl_rwsem.dpa);
+	cxled->part = old_part;
+}
+
+static int select_attach_ram(struct cxl_endpoint_decoder *cxled,
+			     int *old_part, resource_size_t *size)
+{
+	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	struct resource *res;
+	int part, rc;
+
+	scoped_guard(rwsem_read, &cxl_rwsem.dpa)
+		*old_part = cxled->part;
+
+	rc = cxl_dpa_set_part(cxled, CXL_PARTMODE_RAM);
+	if (rc)
+		return rc;
+
+	guard(rwsem_read)(&cxl_rwsem.dpa);
+	part = cxled->part;
+	if (part < 0 || part >= cxlds->nr_partitions)
+		return -ENXIO;
+
+	res = &cxlds->part[part].res;
+	if (res->child)
+		return -EBUSY;
+
+	*size = resource_size(res);
+	if (!*size || !IS_ALIGNED(*size, SZ_256M))
+		return -EINVAL;
+
+	return 0;
+}
+
+static struct cxl_region *
+create_attach_region(struct cxl_endpoint_decoder *cxled,
+		     struct cxl_root_decoder *cxlrd, resource_size_t size)
+{
+	struct cxl_region *cxlr;
+	int rc;
+
+	guard(mutex)(&cxlrd->regions_lock);
+	do {
+		cxlr = __create_region(cxlrd, CXL_PARTMODE_RAM,
+				       atomic_read(&cxlrd->region_id),
+				       CXL_DECODER_DEVMEM);
+	} while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY);
+	if (IS_ERR(cxlr))
+		return cxlr;
+
+	scoped_guard(rwsem_write, &cxl_rwsem.region) {
+		/* Single-target IW=1 is strict v1 policy. */
+		rc = set_interleave_ways(cxlr, 1);
+		if (!rc)
+			rc = set_interleave_granularity(
+				cxlr, CXL_DECODER_MIN_GRANULARITY);
+		if (!rc)
+			rc = alloc_hpa(cxlr, size);
+	}
+	if (rc)
+		goto err_unregister;
+
+	rc = cxl_dpa_alloc(cxled, size);
+	if (rc)
+		goto err_unregister;
+
+	rc = attach_target(cxlr, cxled, 0, TASK_UNINTERRUPTIBLE);
+	if (rc)
+		goto err_unregister;
+
+	rc = __commit_context(cxlr, CXL_REGION_RESET_MANAGED_DETACH);
+	if (rc)
+		goto err_unregister;
+
+	rc = device_attach(&cxlr->dev);
+	if (rc <= 0) {
+		if (!rc)
+			rc = -ENXIO;
+		goto err_unregister;
+	}
+
+	get_device(&cxlr->dev);
+	return cxlr;
+
+err_unregister:
+	unregister_region(cxlr, CXL_REGION_RESET_MANAGED_DETACH);
+	return ERR_PTR(rc);
+}
+
+static void cleanup_attach_dpa(struct cxl_endpoint_decoder *cxled,
+			       int old_part, int setup_rc)
+{
+	int rc;
+
+	rc = cxl_dpa_free(cxled);
+	if (rc)
+		dev_err(&cxled->cxld.dev,
+			"failed to clean up DPA after attach error %d: %d\n",
+			setup_rc, rc);
+	restore_attach_decoder_part(cxled, old_part);
+}
+
+static int create_memdev_attach_region(struct cxl_memdev *cxlmd,
+				       struct cxl_attach_region *attach)
+{
+	struct cxl_port *endpoint = cxlmd->endpoint;
+	struct device *decoder_dev __free(put_device) = NULL;
+	struct cxl_endpoint_decoder *cxled;
+	struct cxl_root_decoder *cxlrd;
+	struct cxl_region *cxlr;
+	struct range hpa_range;
+	resource_size_t size;
+	int old_part, rc;
+
+	scoped_guard(rwsem_read, &cxl_rwsem.region) {
+		guard(rwsem_read)(&cxl_rwsem.dpa);
+		decoder_dev = device_find_child(&endpoint->dev, endpoint,
+						first_attach_decoder);
+	}
+	if (!decoder_dev) {
+		dev_dbg(cxlmd->cxlds->dev,
+			"no free manual DEVMEM decoder to auto-create a region for %s\n",
+			dev_name(&cxlmd->dev));
+		return -ENXIO;
+	}
+	cxled = to_cxl_endpoint_decoder(decoder_dev);
+
+	rc = select_attach_ram(cxled, &old_part, &size);
+	if (rc) {
+		restore_attach_decoder_part(cxled, old_part);
+		return rc;
+	}
+
+	cxlrd = find_attach_root_decoder(cxled);
+	if (IS_ERR(cxlrd)) {
+		rc = PTR_ERR(cxlrd);
+		dev_dbg(cxlmd->cxlds->dev,
+			"no compatible Type-2 root decoder to auto-create a region for %s: %d\n",
+			dev_name(&cxlmd->dev), rc);
+		goto err_cleanup_dpa;
+	}
+
+	cxlr = create_attach_region(cxled, cxlrd, size);
+	put_device(&cxlrd->cxlsd.cxld.dev);
+	if (IS_ERR(cxlr)) {
+		rc = PTR_ERR(cxlr);
+		goto err_cleanup_dpa;
+	}
+
+	hpa_range = (struct range) {
+		.start = cxlr->params.res->start,
+		.end = cxlr->params.res->end,
+	};
+	rc = devm_add_action_or_reset(&endpoint->dev,
+				      endpoint_unregister_region, cxlr);
+	if (rc)
+		goto err_cleanup_dpa;
+
+	attach->hpa_range = hpa_range;
+	return 0;
+
+err_cleanup_dpa:
+	cleanup_attach_dpa(cxled, old_part, rc);
+	return rc;
+}
+
 /*
- * Runs in cxl_mem_probe context after successful endpoint probe, assumes the
- * simple case of single mapped decoder per memdev.
+ * Attach to a firmware-precommitted region already mapped to the endpoint.
+ * Return 0 on success, -ENODEV when no region is present (the caller then
+ * auto-creates one), or a negative errno for a present-but-unusable region.
  */
-int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
+static int find_committed_attach_region(struct cxl_memdev *cxlmd,
+					struct cxl_attach_region *attach)
 {
-	struct cxl_attach_region *attach =
-		container_of(cxlmd->attach, typeof(*attach), attach);
 	struct cxl_port *endpoint = cxlmd->endpoint;
 	struct cxl_endpoint_decoder *cxled;
 	struct cxl_region *cxlr;
 	int rc;
 
-	/* hold endpoint lock to setup autoremove of the region */
-	guard(device)(&endpoint->dev);
-	if (!endpoint->dev.driver)
-		return -ENXIO;
 	guard(rwsem_read)(&cxl_rwsem.region);
 	guard(rwsem_read)(&cxl_rwsem.dpa);
-
-	/*
-	 * TODO auto-instantiate a region, for now assume this will find an
-	 * auto-region
-	 */
 	struct device *dev __free(put_device) =
 		device_find_child(&endpoint->dev, NULL, first_mapped_decoder);
 
-	if (!dev) {
-		dev_dbg(cxlmd->cxlds->dev, "no region found for memdev %s\n",
-			dev_name(&cxlmd->dev));
-		return -ENXIO;
-	}
+	if (!dev)
+		return -ENODEV;
 
 	cxled = to_cxl_endpoint_decoder(dev);
 	cxlr = cxled->cxld.region;
 
 	if (cxlr->params.state < CXL_CONFIG_COMMIT) {
-		dev_dbg(cxlmd->cxlds->dev,
-			"region %s not committed for memdev %s\n",
+		dev_dbg(cxlmd->cxlds->dev, "region %s not committed for memdev %s\n",
 			dev_name(&cxlr->dev), dev_name(&cxlmd->dev));
 		return -ENXIO;
 	}
@@ -4226,10 +4459,10 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
 		return -ENXIO;
 	}
 
-	/* Only teardown regions that pass validation, ignore the rest */
+	/* Only teardown regions that pass validation. */
 	get_device(&cxlr->dev);
-	rc = devm_add_action_or_reset(&endpoint->dev,
-				      endpoint_unregister_region, cxlr);
+	rc = devm_add_action_or_reset(&endpoint->dev, endpoint_unregister_region,
+				      cxlr);
 	if (rc)
 		return rc;
 
@@ -4239,6 +4472,29 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
 	};
 	return 0;
 }
+
+/*
+ * Runs in cxl_mem_probe context after successful endpoint probe, assumes the
+ * simple case of single mapped decoder per memdev.
+ */
+int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
+{
+	struct cxl_attach_region *attach =
+		container_of(cxlmd->attach, typeof(*attach), attach);
+	struct cxl_port *endpoint = cxlmd->endpoint;
+	int rc;
+
+	/* hold endpoint lock to setup autoremove of the region */
+	guard(device)(&endpoint->dev);
+	if (!endpoint->dev.driver)
+		return -ENXIO;
+
+	rc = find_committed_attach_region(cxlmd, attach);
+	if (rc != -ENODEV)
+		return rc;
+
+	return create_memdev_attach_region(cxlmd, attach);
+}
 EXPORT_SYMBOL_FOR_MODULES(cxl_memdev_attach_region, "cxl_mem");
 
 /*
-- 
2.43.0

[RFC PATCH 3/3] cxl/test: Exercise Type-2 automatic region creation

[Richard Cheng]

Add a second mock Type-2 accelerator with an independent single-target
CFMWS and an uncommitted manual DEVMEM decoder. Keep the existing
FW-precomitted accelerator unchanged.

Verify that devm_cxl_probe_mem() creates the missing region and returns
a valid 512 MB HPA range. Preserve the manual decoder config after reset
so the fallback remains available across unbind and rebind.

Signed-off-by: Richard Cheng <icheng@nvidia.com>
---
 tools/testing/cxl/test/accel.c |  7 ++++
 tools/testing/cxl/test/cxl.c   | 61 ++++++++++++++++++++++++++++++++--
 2 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/tools/testing/cxl/test/accel.c b/tools/testing/cxl/test/accel.c
index 8e6f4687ca02..7e5d76a7f8c4 100644
--- a/tools/testing/cxl/test/accel.c
+++ b/tools/testing/cxl/test/accel.c
@@ -38,6 +38,13 @@ static int cxl_mock_accel_probe(struct platform_device *pdev)
 	cxlmd = devm_cxl_probe_mem(cxlds, &mock_range);
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);
+	if (mock_range.start > mock_range.end ||
+	    range_len(&mock_range) != SZ_512M) {
+		dev_err(dev,
+			"accelerator%d returned invalid HPA range %pra (expected 512 MiB)\n",
+			pdev->id, &mock_range);
+		return -ERANGE;
+	}
 	cxl_accel->cxlmd = cxlmd;
 
 	dev_dbg(dev, "Probed mock accelerator with range %pra\n", &mock_range);
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index 8ab2ce1262f3..305a0c3705da 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -28,7 +28,7 @@ static bool type2_test;
 #define NR_CXL_SWITCH_PORTS 2
 #define NR_CXL_PORT_DECODERS 8
 #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH)
-#define NR_CXL_TYPE2_ACCEL 1
+#define NR_CXL_TYPE2_ACCEL 2
 
 #define MOCK_AUTO_REGION_SIZE_DEFAULT SZ_512M
 static int mock_auto_region_size = MOCK_AUTO_REGION_SIZE_DEFAULT;
@@ -493,7 +493,16 @@ static void cfmws_elc_update(struct acpi_cedt_cfmws *window, int index)
 
 static void update_type2_cfmws(void)
 {
+	struct acpi_cedt_cfmws *window = &mock_cedt.cfmws1.cfmws;
+
 	memcpy(&mock_cedt.cfmws0.cfmws, &type2_cfmws0, sizeof(type2_cfmws0));
+
+	window->header.length = sizeof(*window) +
+				sizeof(mock_cedt.cfmws1.target[0]);
+	window->interleave_ways = 0;
+	window->restrictions = ACPI_CEDT_CFMWS_RESTRICT_DEVMEM |
+			       ACPI_CEDT_CFMWS_RESTRICT_VOLATILE;
+	mock_cedt.cfmws1.target[0] = 1;
 }
 
 static int populate_cedt(void)
@@ -814,6 +823,32 @@ static int mock_decoder_commit(struct cxl_decoder *cxld);
 static void mock_decoder_reset(struct cxl_decoder *cxld);
 static void init_disabled_mock_decoder(struct cxl_decoder *cxld);
 
+static bool is_type2_manual_decoder(struct cxl_decoder *cxld,
+				    struct platform_device *pdev)
+{
+	return type2_test && is_endpoint_decoder(&cxld->dev) && pdev &&
+		pdev->id == 1 && !strcmp(pdev->name, "cxl_type2_accel") &&
+		cxld->id == 0;
+}
+
+static void init_type2_manual_decoder(struct cxl_endpoint_decoder *cxled)
+{
+	struct cxl_decoder *cxld = &cxled->cxld;
+
+	cxld->hpa_range = (struct range) {
+		.start = 0,
+		.end = -1,
+	};
+	cxld->interleave_ways = 1;
+	cxld->interleave_granularity = CXL_DECODER_MIN_GRANULARITY;
+	cxld->target_type = CXL_DECODER_DEVMEM;
+	cxld->flags = 0;
+	cxled->state = CXL_DECODER_STATE_MANUAL;
+	cxled->skip = 0;
+	cxld->commit = mock_decoder_commit;
+	cxld->reset = mock_decoder_reset;
+}
+
 static void cxld_copy(struct cxl_decoder *a, struct cxl_decoder *b)
 {
 	a->id = b->id;
@@ -1089,6 +1124,7 @@ enum cxld_init_type {
 	MOCK_DECODER_INIT_SAVED,
 	MOCK_DECODER_INIT_TYPE3_AUTO,
 	MOCK_DECODER_INIT_TYPE2_AUTO,
+	MOCK_DECODER_INIT_TYPE2_MANUAL,
 };
 
 static enum cxld_init_type get_decoder_init_type(struct cxl_decoder *cxld,
@@ -1104,6 +1140,8 @@ static enum cxld_init_type get_decoder_init_type(struct cxl_decoder *cxld,
 	}
 
 	*td = NULL;
+	if (is_type2_manual_decoder(cxld, pdev))
+		return MOCK_DECODER_INIT_TYPE2_MANUAL;
 
 	/*
 	 * The first decoder on the first 2 devices on the first switch
@@ -1121,7 +1159,9 @@ static enum cxld_init_type get_decoder_init_type(struct cxl_decoder *cxld,
 			    MOCK_DECODER_INIT_TYPE3_AUTO;
 }
 
-static bool mock_decoder_handle_saved(struct cxl_decoder *cxld, struct cxl_test_decoder *td)
+static bool mock_decoder_handle_saved(struct cxl_decoder *cxld,
+				      struct cxl_test_decoder *td,
+				      struct platform_device *pdev)
 {
 	bool enabled;
 
@@ -1133,6 +1173,11 @@ static bool mock_decoder_handle_saved(struct cxl_decoder *cxld, struct cxl_test_
 	if (enabled)
 		return !cxld_registry_restore(cxld, td);
 
+	if (is_type2_manual_decoder(cxld, pdev)) {
+		init_type2_manual_decoder(to_cxl_endpoint_decoder(&cxld->dev));
+		return false;
+	}
+
 	init_disabled_mock_decoder(cxld);
 	return false;
 }
@@ -1209,6 +1254,13 @@ static void mock_init_hdm_type2_cxled(struct cxl_endpoint_decoder *cxled,
 	put_device(dev);
 }
 
+static void mock_init_hdm_type2_manual(struct cxl_endpoint_decoder *cxled)
+{
+	init_type2_manual_decoder(cxled);
+
+	WARN_ON_ONCE(!cxld_registry_new(&cxled->cxld));
+}
+
 static void mock_init_hdm_type3_cxled(struct cxl_endpoint_decoder *cxled,
 				      struct cxl_port *port,
 				      struct platform_device *pdev,
@@ -1361,7 +1413,7 @@ static bool mock_init_hdm_decoder(struct cxl_decoder *cxld)
 	case MOCK_DECODER_INIT_SAVED:
 		if (WARN_ON(!td))
 			return false;
-		return mock_decoder_handle_saved(cxld, td);
+		return mock_decoder_handle_saved(cxld, td, pdev);
 	case MOCK_DECODER_INIT_DEFAULT:
 		/*
 		 * The default path picks up all the decoders that are not
@@ -1375,6 +1427,9 @@ static bool mock_init_hdm_decoder(struct cxl_decoder *cxld)
 	case MOCK_DECODER_INIT_TYPE2_AUTO:
 		mock_init_hdm_type2_cxled(cxled, port);
 		return false;
+	case MOCK_DECODER_INIT_TYPE2_MANUAL:
+		mock_init_hdm_type2_manual(cxled);
+		return false;
 	default:
 		return false;
 	}
-- 
2.43.0

Re: [RFC PATCH 3/3] cxl/test: Exercise Type-2 automatic region creation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Failure to restore a previously-enabled manual decoder permanently destroys its manual configuration across unbind/rebind cycles.
--

--- Patch [3]: [RFC PATCH 3/3] cxl/test: Exercise Type-2 automatic region creation ---
commit e8d600bcac1235041afc295954c1b2f226a9c8d7
Author: Richard Cheng <icheng@nvidia.com>

cxl/test: Exercise Type-2 automatic region creation

This patch adds a second mock Type-2 accelerator with an independent
single-target CFMWS and an uncommitted manual DEVMEM decoder. It verifies
that devm_cxl_probe_mem() creates the missing region and preserves the manual
decoder config after reset so the fallback remains available across unbind and
rebind.

> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index 8ab2ce1262f30..305a0c3705da4 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c

[ ... ]

> @@ -1121,7 +1159,9 @@ static enum cxld_init_type get_decoder_init_type(struct cxl_decoder *cxld,
>  			    MOCK_DECODER_INIT_TYPE3_AUTO;
>  }
>  
> -static bool mock_decoder_handle_saved(struct cxl_decoder *cxld, struct cxl_test_decoder *td)
> +static bool mock_decoder_handle_saved(struct cxl_decoder *cxld,
> +				      struct cxl_test_decoder *td,
> +				      struct platform_device *pdev)
>  {
>  	bool enabled;
>  
> @@ -1133,6 +1173,11 @@ static bool mock_decoder_handle_saved(struct cxl_decoder *cxld, struct cxl_test_
>  	if (enabled)
>  		return !cxld_registry_restore(cxld, td);
>  
> +	if (is_type2_manual_decoder(cxld, pdev)) {

[Severity: Medium]
If cxld_registry_restore() fails, does this bypass the manual fallback logic?

Looking at the preceding lines:

	if (enabled)
		return !cxld_registry_restore(cxld, td);

If the decoder was enabled but cxld_registry_restore() fails (e.g., due to a
DPA reservation error), this function returns immediately. 

Since cxld_registry_restore() internally calls init_disabled_mock_decoder()
on failure, the decoder is forcefully reverted to a HOSTONLYMEM state instead
of the intended DEVMEM fallback state.

Would it be better to fall through to the is_type2_manual_decoder() check
when cxld_registry_restore() fails, so the manual configuration isn't lost
across unbind and rebind cycles?

> +		init_type2_manual_decoder(to_cxl_endpoint_decoder(&cxld->dev));
> +		return false;
> +	}
> +
>  	init_disabled_mock_decoder(cxld);
>  	return false;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805074042.30173-1-icheng@nvidia.com?part=3

Re: [RFC PATCH 1/3] cxl/region: Reset software-created regions on memdev detach

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Userspace can bypass the F_LOCK protection intended to prevent userspace disposition, resulting in permanently leaked hardware decoders and potential memory corruption.
--

--- Patch [1]: [RFC PATCH 1/3] cxl/region: Reset software-created regions on memdev detach ---
commit f2541005d7a5430e1aac6ecade8931768b282855
Author: Richard Cheng <icheng@nvidia.com>

cxl/region: Reset software-created regions on memdev detach

This patch modifies region teardown to pass a reset context (DEFAULT vs
MANAGED_DETACH). It allows a memdev attach provider to reset its own
software-created regions without changing generic teardown behavior.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 1e211542b6b64..7fcaddc61180d 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c

[ ... ]

> @@ -2893,7 +2963,7 @@ static ssize_t delete_region_store(struct device *dev,
>  	if (!cxlr || !sysfs_streq(buf, dev_name(&cxlr->dev)))
>  		return -ENODEV;
>  
> -	unregister_region(cxlr);
> +	unregister_region(cxlr, CXL_REGION_RESET_DEFAULT);
>  
>  	return len;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does this sysfs path allow
userspace to bypass the F_LOCK protection and corrupt memory?

When a user writes to the delete_region sysfs file, delete_region_store()
unconditionally calls unregister_region() with CXL_REGION_RESET_DEFAULT,
even if CXL_REGION_F_LOCK is set.

This propagates down to cxl_region_decode_reset(), where the combination
of F_LOCK and CXL_REGION_RESET_DEFAULT causes an early return, skipping
the hardware reset:

drivers/cxl/core/region.c:cxl_region_decode_reset() {
        if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags) &&
            (context != CXL_REGION_RESET_MANAGED_DETACH || ...))
                return;
        ...
}

However, the software teardown still completes in __cxl_decoder_detach()
and the region is unregistered, freeing the HPA range for reallocation:

drivers/cxl/core/region.c:__cxl_decoder_detach() {
        ...
        if (p->state == CXL_CONFIG_ACTIVE) {
                p->state = CXL_CONFIG_INTERLEAVE_ACTIVE;
                cxl_region_teardown_targets(cxlr);
        }
        p->targets[cxled->pos] = NULL;
        p->nr_targets--;
        ...
}

Could this leave the hardware decoders permanently programmed while the
software structures are destroyed? If the kernel reallocates this HPA range
to a new region, would it create overlapping hardware decoders and lead
to memory corruption?

The same bypass pattern appears to be preserved in store_targetN() when
detaching targets. Should these sysfs paths verify CXL_REGION_F_LOCK
before initiating teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805074042.30173-1-icheng@nvidia.com?part=1

Re: [RFC PATCH 2/3] cxl/region: Auto-create a region for memdev attach

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Mixing goto-based error handling with __free() or guard() cleanup in the same function.

Pre-existing issues:
- [High] Self-deadlock in find_committed_attach_region() on the devm_add_action_or_reset() error path due to function-scoped read lock on cxl_rwsem.region.
--

--- Patch [2]: [RFC PATCH 2/3] cxl/region: Auto-create a region for memdev attach ---
commit 5faea7eff07e5a585a3f3dc3abd38f41084338fd
Author: Richard Cheng <icheng@nvidia.com>

cxl/region: Auto-create a region for memdev attach

devm_cxl_probe_mem() currently fails when FW has not committed a region,
even when a Type-2 accelerator has usable CXL.mem capacity. This patch
auto-creates a region when no mapped decoder exists by selecting a manual
DEVMEM decoder and a compatible root decoder.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 7fcaddc61180d..4ceabdfdd3b6a 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
[ ... ]
> +static struct cxl_region *
> +create_attach_region(struct cxl_endpoint_decoder *cxled,
> +		     struct cxl_root_decoder *cxlrd, resource_size_t size)
> +{
> +	struct cxl_region *cxlr;
> +	int rc;
> +
> +	guard(mutex)(&cxlrd->regions_lock);
> +	do {
[ ... ]
> +	rc = device_attach(&cxlr->dev);
> +	if (rc <= 0) {
> +		if (!rc)
> +			rc = -ENXIO;
> +		goto err_unregister;
> +	}
> +
> +	get_device(&cxlr->dev);
> +	return cxlr;
> +
> +err_unregister:
> +	unregister_region(cxlr, CXL_REGION_RESET_MANAGED_DETACH);
> +	return ERR_PTR(rc);
> +}

[Severity: Medium]
Is it acceptable to mix guard() based cleanup with goto-based error handling
in this function? According to the cleanup subsystem guidelines, functions
should ideally use scope-based cleanup exclusively or goto-based cleanup
exclusively to prevent complex ownership and LIFO ordering bugs.

[ ... ]
> +static int create_memdev_attach_region(struct cxl_memdev *cxlmd,
> +				       struct cxl_attach_region *attach)
> +{
> +	struct cxl_port *endpoint = cxlmd->endpoint;
> +	struct device *decoder_dev __free(put_device) = NULL;
> +	struct cxl_endpoint_decoder *cxled;
[ ... ]
> +	cxlrd = find_attach_root_decoder(cxled);
> +	if (IS_ERR(cxlrd)) {
> +		rc = PTR_ERR(cxlrd);
> +		dev_dbg(cxlmd->cxlds->dev,
> +			"no compatible Type-2 root decoder to auto-create a region for %s: %d\n",
> +			dev_name(&cxlmd->dev), rc);
> +		goto err_cleanup_dpa;
> +	}

[Severity: Medium]
Similar to create_attach_region() above, does mixing __free() and goto labels
in the same function conflict with the cleanup subsystem expectations?

[ ... ]
>  /*
> - * Runs in cxl_mem_probe context after successful endpoint probe, assumes the
> - * simple case of single mapped decoder per memdev.
> + * Attach to a firmware-precommitted region already mapped to the endpoint.
> + * Return 0 on success, -ENODEV when no region is present (the caller then
> + * auto-creates one), or a negative errno for a present-but-unusable region.
>   */
> -int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
> +static int find_committed_attach_region(struct cxl_memdev *cxlmd,
> +					struct cxl_attach_region *attach)
>  {
> -	struct cxl_attach_region *attach =
> -		container_of(cxlmd->attach, typeof(*attach), attach);
>  	struct cxl_port *endpoint = cxlmd->endpoint;
>  	struct cxl_endpoint_decoder *cxled;
>  	struct cxl_region *cxlr;
>  	int rc;
>  
> -	/* hold endpoint lock to setup autoremove of the region */
> -	guard(device)(&endpoint->dev);
> -	if (!endpoint->dev.driver)
> -		return -ENXIO;
>  	guard(rwsem_read)(&cxl_rwsem.region);
>  	guard(rwsem_read)(&cxl_rwsem.dpa);
[ ... ]
> -	/* Only teardown regions that pass validation, ignore the rest */
> +	/* Only teardown regions that pass validation. */
>  	get_device(&cxlr->dev);
> -	rc = devm_add_action_or_reset(&endpoint->dev,
> -				      endpoint_unregister_region, cxlr);
> +	rc = devm_add_action_or_reset(&endpoint->dev, endpoint_unregister_region,
> +				      cxlr);
>  	if (rc)
>  		return rc;

[Severity: High]
This is a pre-existing issue, but can this devm_add_action_or_reset() trigger
a self-deadlock on failure?

Because the function scope is guarded by a read lock on cxl_rwsem.region:

    guard(rwsem_read)(&cxl_rwsem.region);

If devm_add_action_or_reset() fails, it immediately executes its callback
endpoint_unregister_region() inline. This callback eventually calls
unregister_region() and detach_target(), which attempts to acquire a write
lock on the same semaphore:

    ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region)

Since the current thread already holds the function-scoped read lock, wouldn't
this result in an ABBA-style self-deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805074042.30173-1-icheng@nvidia.com?part=2

Re: [RFC PATCH 0/3] cxl: Auto-create a region for Type-2 memdev attach

[Alejandro Lucero Palau]

Hi Richard,


Some comments below.


Thanks!


On 8/5/26 08:40, Richard Cheng wrote:
> A Type-2 accelerator driver calls devm_cxl_probe_mem() to register its
> memdev and get an HPA range, but today only if FW already committed a
> region. Real accelerators can have usable memory with no committed decoder,
> so get nothing.


The previous paragraph describes the current situation and the next one 
is about what the patchset tries to address. Maybe to explicitly make 
the difference would help people not so used to the subject.


> If no FW region is mapped, the core picks the device's unused manual
> DEVMEM decoder and a compatible x1 Type-2 RAM root


I had to look for this x1 reference ... and I would say it creates 
confusion. At least it does to me. Not sure if you meant interleaving, 
because I do not think you are referring to link lanes here ...


> , allocates the full
> volatile DPA and HPA, commits the decoder, and returns the range.


This is something requiring discussion or clarification. I think it 
would make sense the provider/driver specifying a DPA size instead of 
using the default full size. I think there is a good reason for this 
non-default size use: why would the kernel create a region from a CXL 
Type2 device using the full DPA size when the FW/BIOS did not do so?


This leads us to wondering why the FW/BIOS would not do so, the use 
case. Current Intel/AMD BIOS (I think you have the aim at ARM servers) 
are not allowing this case ... for a Type2 device having all the bits in 
place. If something requires to be specifically configured, would not 
the driver do so before using the CXL mem? If this logic makes sense, 
the auto-creation should not be the way to go.


The first 15 Type2 basic support patchset versions supported the case of 
a driver specifying the size for the cxl region to be created. And it 
was through a specific API call after the memdev was created. Last Type2 
patchset and the functionality finally merged only supported the case of 
auto-create regions from committed decoders, and using this final 
agreement for region attachment by the Type2 memdev/driver. I think it 
makes sense in that supported case to have the auto-create region but I 
can not see the reason for the case you are addressing now.


>   Unbind
> resets and removes it. Strict first cut with single decoder, IW=1, minimum
> granularity, first-compatible root.


I'm lost here.


>
> The design intent is that the provider F_LOCKs its region against userspace
> but must reset its own software region on detach. A plain flag would also
> allow reset on generic kill/delete paths, so we thread a reset context
> through teardown and commit rollback. devm_cxl_probe_mem() may now commit
> decoders.


If there is a real use case for this auto-create region from 
non-committed decoders, I think your patchset makes sense. But I'm 
afraid we need to discuss this further.


Thank you,

Alejandro.


> Testing result is in the following.
> - Built clean with clang/LLVM on arm64
> - cxl_test, type2_test=1. accel0 takes the unchanged attach path. accel1
>    drives auto_create -> a committed 512 MB RAM region. The test asserts
>    the 512 MB HPA range. committed state and 256 byte granularity confirmed
>    via sysfs.
> - Unbind tears the region down with no orphaned decoder, rebind re-creates
>    a fresh committed region.
> - Mock test only. Real accelerators whose FW commits a decoder take the
>    attach path, and vfio-cxl binds only FW-committed devices, so auto-create
>    has no real-HW caller yet.
>
> Best regards,
> Richard Cheng.
>
> Richard Cheng (3):
>    cxl/region: Reset software-created regions on memdev detach
>    cxl/region: Auto-create a region for memdev attach
>    cxl/test: Exercise Type-2 automatic region creation
>
>   drivers/cxl/core/region.c      | 422 +++++++++++++++++++++++++++++----
>   tools/testing/cxl/test/accel.c |   7 +
>   tools/testing/cxl/test/cxl.c   |  61 ++++-
>   3 files changed, 439 insertions(+), 51 deletions(-)
>
>
> base-commit: 1c6b4ceafc3b994871c29340e0c1ddb0af5800e7

Re: [RFC PATCH 2/3] cxl/region: Auto-create a region for memdev attach

[Alejandro Lucero Palau]

Hi Richard,


Some comments below. Just conceptual ones, except maybe a bug at the end.


Thanks!


On 8/5/26 08:40, Richard Cheng wrote:
> devm_cxl_probe_mem() currently fails when FW has not committed a region,
> even when a Type-2 accelerator has usable CXL.mem capacity.
>
> When no mapped decoder exists, select a pristine manual DEVMEM decoder
> and the first compatible unlocked Type-2 RAM root decoder. Create a
> non-AUTO, single-target region, allocate HPA and the full volatile DPA
> partition, attach and commit the decoder path, then return the resulting
> HPA range.
>
> Use provider-managed reset for partial-commit rollback and unwind
> region, HPA, DPA, and partition state in reverse order on failure.
> Preserve the existing FW-precommitted path.
>
> This support is limited to decoder 0, IW=1, and first-compatible root
> selection.
>
> Signed-off-by: Richard Cheng <icheng@nvidia.com>
> ---
>   drivers/cxl/core/region.c | 312 ++++++++++++++++++++++++++++++++++----
>   1 file changed, 284 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 7fcaddc61180..4ceabdfdd3b6 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -363,7 +363,8 @@ static int commit_decoder(struct cxl_decoder *cxld)
>   	return 0;
>   }
>   
> -static int cxl_region_decode_commit(struct cxl_region *cxlr)
> +static int cxl_region_decode_commit(
> +	struct cxl_region *cxlr, enum cxl_region_reset_context context)
>   {
>   	struct cxl_region_params *p = &cxlr->params;
>   	int i, rc = 0;
> @@ -405,7 +406,7 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
>   
>   err:
>   	/* undo the targets that were successfully committed */
> -	cxl_region_decode_reset(cxlr, i, CXL_REGION_RESET_DEFAULT);
> +	cxl_region_decode_reset(cxlr, i, context);
>   	return rc;
>   }
>   
> @@ -427,7 +428,8 @@ static int queue_reset(struct cxl_region *cxlr)
>   	return 0;
>   }
>   
> -static int __commit(struct cxl_region *cxlr)
> +static int __commit_context(struct cxl_region *cxlr,
> +			    enum cxl_region_reset_context context)
>   {
>   	struct cxl_region_params *p = &cxlr->params;
>   	int rc;
> @@ -452,7 +454,7 @@ static int __commit(struct cxl_region *cxlr)
>   	if (rc)
>   		return rc;
>   
> -	rc = cxl_region_decode_commit(cxlr);
> +	rc = cxl_region_decode_commit(cxlr, context);
>   	if (rc)
>   		return rc;
>   
> @@ -461,6 +463,11 @@ static int __commit(struct cxl_region *cxlr)
>   	return 0;
>   }
>   
> +static int __commit(struct cxl_region *cxlr)
> +{
> +	return __commit_context(cxlr, CXL_REGION_RESET_DEFAULT);
> +}
> +
>   static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
>   			    const char *buf, size_t len)
>   {
> @@ -4177,45 +4184,271 @@ static int first_mapped_decoder(struct device *dev, const void *data)
>   	return 0;
>   }
>   
> +static int first_attach_decoder(struct device *dev, const void *data)
> +{
> +	struct cxl_port *endpoint = (struct cxl_port *)data;
> +	struct cxl_endpoint_decoder *cxled;
> +	struct cxl_decoder *cxld;
> +
> +	if (!is_endpoint_decoder(dev))
> +		return 0;
> +
> +	cxled = to_cxl_endpoint_decoder(dev);
> +	cxld = &cxled->cxld;
> +	if (cxld->id != 0 || cxled->state != CXL_DECODER_STATE_MANUAL ||
> +	    cxld->target_type != CXL_DECODER_DEVMEM || cxld->region ||
> +	    cxled->dpa_res ||
> +	    (cxld->flags & (CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK)))
> +		return 0;
> +
> +	if (endpoint->hdm_end != -1 || cxl_num_decoders_committed(endpoint))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static int first_attach_root_decoder(struct device *dev, const void *data)
> +{
> +	struct cxl_port *endpoint = (struct cxl_port *)data;
> +	unsigned long required = CXL_DECODER_F_TYPE2 | CXL_DECODER_F_RAM |
> +				 CXL_DECODER_F_ENABLE;
> +	struct cxl_root_decoder *cxlrd;
> +	struct cxl_switch_decoder *cxlsd;
> +	struct cxl_decoder *cxld;
> +	struct cxl_dport *dport;
> +
> +	if (!is_root_decoder(dev) || !device_is_registered(dev))
> +		return 0;
> +
> +	cxlrd = to_cxl_root_decoder(dev);
> +	cxlsd = &cxlrd->cxlsd;
> +	cxld = &cxlsd->cxld;
> +	if (cxlrd->dead || !cxlrd->res ||
> +	    (cxld->flags & required) != required ||
> +	    (cxld->flags & CXL_DECODER_F_LOCK) ||
> +	    cxld->interleave_ways != 1 || cxlsd->nr_targets < 1)
> +		return 0;
> +
> +	dport = cxl_find_dport_by_dev(cxlrd_to_port(cxlrd),
> +				       endpoint->host_bridge);
> +	return dport && cxlsd->target[0] == dport;
> +}
> +
> +static struct cxl_root_decoder *
> +find_attach_root_decoder(struct cxl_endpoint_decoder *cxled)
> +{
> +	struct cxl_port *endpoint = cxled_to_port(cxled);
> +	struct cxl_root *root __free(put_cxl_root) = find_cxl_root(endpoint);
> +	struct device *dev;
> +
> +	if (!root)
> +		return ERR_PTR(-ENXIO);
> +
> +	/* First compatible x1 Type-2 window is strict v1 policy. */
> +	dev = device_find_child(&root->port.dev, endpoint,
> +				first_attach_root_decoder);
> +	if (!dev)
> +		return ERR_PTR(-ENXIO);
> +
> +	return to_cxl_root_decoder(dev);
> +}
> +


All these new functions are what v15 and older ones did but a bit 
different. Likely you did look at them, but for what is worth: 
https://lore.kernel.org/linux-cxl/20250514132743.523469-12-alejandro.lucero-palau@amd.com/


My main concern here is what I mentioned about the need for this being 
generic expecting other clients requiring same/similar functionality. 
Although these previous ones seem generic enough, I pointed to this 
specifics below.


> +static void restore_attach_decoder_part(struct cxl_endpoint_decoder *cxled,
> +					int old_part)
> +{
> +	guard(rwsem_write)(&cxl_rwsem.dpa);
> +	cxled->part = old_part;
> +}
> +
> +static int select_attach_ram(struct cxl_endpoint_decoder *cxled,
> +			     int *old_part, resource_size_t *size)
> +{
> +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct resource *res;
> +	int part, rc;
> +
> +	scoped_guard(rwsem_read, &cxl_rwsem.dpa)
> +		*old_part = cxled->part;
> +
> +	rc = cxl_dpa_set_part(cxled, CXL_PARTMODE_RAM);
> +	if (rc)
> +		return rc;
> +
> +	guard(rwsem_read)(&cxl_rwsem.dpa);
> +	part = cxled->part;
> +	if (part < 0 || part >= cxlds->nr_partitions)
> +		return -ENXIO;
> +
> +	res = &cxlds->part[part].res;
> +	if (res->child)
> +		return -EBUSY;
> +
> +	*size = resource_size(res);
> +	if (!*size || !IS_ALIGNED(*size, SZ_256M))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static struct cxl_region *
> +create_attach_region(struct cxl_endpoint_decoder *cxled,
> +		     struct cxl_root_decoder *cxlrd, resource_size_t size)
> +{
> +	struct cxl_region *cxlr;
> +	int rc;
> +
> +	guard(mutex)(&cxlrd->regions_lock);
> +	do {
> +		cxlr = __create_region(cxlrd, CXL_PARTMODE_RAM,
> +				       atomic_read(&cxlrd->region_id),
> +				       CXL_DECODER_DEVMEM);
> +	} while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY);
> +	if (IS_ERR(cxlr))
> +		return cxlr;
> +
> +	scoped_guard(rwsem_write, &cxl_rwsem.region) {
> +		/* Single-target IW=1 is strict v1 policy. */
> +		rc = set_interleave_ways(cxlr, 1);
> +		if (!rc)
> +			rc = set_interleave_granularity(
> +				cxlr, CXL_DECODER_MIN_GRANULARITY);
> +		if (!rc)
> +			rc = alloc_hpa(cxlr, size);
> +	}
> +	if (rc)
> +		goto err_unregister;
> +
> +	rc = cxl_dpa_alloc(cxled, size);
> +	if (rc)
> +		goto err_unregister;
> +
> +	rc = attach_target(cxlr, cxled, 0, TASK_UNINTERRUPTIBLE);
> +	if (rc)
> +		goto err_unregister;
> +
> +	rc = __commit_context(cxlr, CXL_REGION_RESET_MANAGED_DETACH);
> +	if (rc)
> +		goto err_unregister;
> +
> +	rc = device_attach(&cxlr->dev);
> +	if (rc <= 0) {
> +		if (!rc)
> +			rc = -ENXIO;
> +		goto err_unregister;
> +	}
> +
> +	get_device(&cxlr->dev);
> +	return cxlr;
> +
> +err_unregister:
> +	unregister_region(cxlr, CXL_REGION_RESET_MANAGED_DETACH);
> +	return ERR_PTR(rc);
> +}
> +
> +static void cleanup_attach_dpa(struct cxl_endpoint_decoder *cxled,
> +			       int old_part, int setup_rc)
> +{
> +	int rc;
> +
> +	rc = cxl_dpa_free(cxled);
> +	if (rc)
> +		dev_err(&cxled->cxld.dev,
> +			"failed to clean up DPA after attach error %d: %d\n",
> +			setup_rc, rc);
> +	restore_attach_decoder_part(cxled, old_part);
> +}
> +
> +static int create_memdev_attach_region(struct cxl_memdev *cxlmd,
> +				       struct cxl_attach_region *attach)
> +{
> +	struct cxl_port *endpoint = cxlmd->endpoint;
> +	struct device *decoder_dev __free(put_device) = NULL;
> +	struct cxl_endpoint_decoder *cxled;
> +	struct cxl_root_decoder *cxlrd;
> +	struct cxl_region *cxlr;
> +	struct range hpa_range;
> +	resource_size_t size;
> +	int old_part, rc;
> +
> +	scoped_guard(rwsem_read, &cxl_rwsem.region) {
> +		guard(rwsem_read)(&cxl_rwsem.dpa);
> +		decoder_dev = device_find_child(&endpoint->dev, endpoint,
> +						first_attach_decoder);
> +	}
> +	if (!decoder_dev) {
> +		dev_dbg(cxlmd->cxlds->dev,
> +			"no free manual DEVMEM decoder to auto-create a region for %s\n",
> +			dev_name(&cxlmd->dev));
> +		return -ENXIO;
> +	}
> +	cxled = to_cxl_endpoint_decoder(decoder_dev);
> +
> +	rc = select_attach_ram(cxled, &old_part, &size);


Previous partition/type selection is likely good enough for only Type2 
auto-creation, but a generic solution for this happening not at probe 
time, therefore not using the attach option, would need to support pmem 
as well ... and the label management.


> +	if (rc) {
> +		restore_attach_decoder_part(cxled, old_part);
> +		return rc;
> +	}
> +
> +	cxlrd = find_attach_root_decoder(cxled);
> +	if (IS_ERR(cxlrd)) {
> +		rc = PTR_ERR(cxlrd);
> +		dev_dbg(cxlmd->cxlds->dev,
> +			"no compatible Type-2 root decoder to auto-create a region for %s: %d\n",
> +			dev_name(&cxlmd->dev), rc);
> +		goto err_cleanup_dpa;
> +	}
> +
> +	cxlr = create_attach_region(cxled, cxlrd, size);
> +	put_device(&cxlrd->cxlsd.cxld.dev);
> +	if (IS_ERR(cxlr)) {
> +		rc = PTR_ERR(cxlr);
> +		goto err_cleanup_dpa;
> +	}
> +
> +	hpa_range = (struct range) {
> +		.start = cxlr->params.res->start,
> +		.end = cxlr->params.res->end,
> +	};
> +	rc = devm_add_action_or_reset(&endpoint->dev,
> +				      endpoint_unregister_region, cxlr);
> +	if (rc)
> +		goto err_cleanup_dpa;
> +
> +	attach->hpa_range = hpa_range;
> +	return 0;
> +
> +err_cleanup_dpa:
> +	cleanup_attach_dpa(cxled, old_part, rc);
> +	return rc;
> +}
> +
>   /*
> - * Runs in cxl_mem_probe context after successful endpoint probe, assumes the
> - * simple case of single mapped decoder per memdev.
> + * Attach to a firmware-precommitted region already mapped to the endpoint.
> + * Return 0 on success, -ENODEV when no region is present (the caller then
> + * auto-creates one), or a negative errno for a present-but-unusable region.
>    */
> -int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
> +static int find_committed_attach_region(struct cxl_memdev *cxlmd,
> +					struct cxl_attach_region *attach)
>   {
> -	struct cxl_attach_region *attach =
> -		container_of(cxlmd->attach, typeof(*attach), attach);
>   	struct cxl_port *endpoint = cxlmd->endpoint;
>   	struct cxl_endpoint_decoder *cxled;
>   	struct cxl_region *cxlr;
>   	int rc;
>   
> -	/* hold endpoint lock to setup autoremove of the region */
> -	guard(device)(&endpoint->dev);
> -	if (!endpoint->dev.driver)
> -		return -ENXIO;
>   	guard(rwsem_read)(&cxl_rwsem.region);
>   	guard(rwsem_read)(&cxl_rwsem.dpa);
> -
> -	/*
> -	 * TODO auto-instantiate a region, for now assume this will find an
> -	 * auto-region
> -	 */
>   	struct device *dev __free(put_device) =
>   		device_find_child(&endpoint->dev, NULL, first_mapped_decoder);
>   
> -	if (!dev) {
> -		dev_dbg(cxlmd->cxlds->dev, "no region found for memdev %s\n",
> -			dev_name(&cxlmd->dev));
> -		return -ENXIO;
> -	}
> +	if (!dev)
> +		return -ENODEV;
>   
>   	cxled = to_cxl_endpoint_decoder(dev);
>   	cxlr = cxled->cxld.region;
>   
>   	if (cxlr->params.state < CXL_CONFIG_COMMIT) {
> -		dev_dbg(cxlmd->cxlds->dev,
> -			"region %s not committed for memdev %s\n",
> +		dev_dbg(cxlmd->cxlds->dev, "region %s not committed for memdev %s\n",
>   			dev_name(&cxlr->dev), dev_name(&cxlmd->dev));
>   		return -ENXIO;
>   	}
> @@ -4226,10 +4459,10 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>   		return -ENXIO;
>   	}
>   
> -	/* Only teardown regions that pass validation, ignore the rest */
> +	/* Only teardown regions that pass validation. */
>   	get_device(&cxlr->dev);
> -	rc = devm_add_action_or_reset(&endpoint->dev,
> -				      endpoint_unregister_region, cxlr);
> +	rc = devm_add_action_or_reset(&endpoint->dev, endpoint_unregister_region,
> +				      cxlr);
>   	if (rc)
>   		return rc;
>   
> @@ -4239,6 +4472,29 @@ int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
>   	};
>   	return 0;
>   }
> +
> +/*
> + * Runs in cxl_mem_probe context after successful endpoint probe, assumes the
> + * simple case of single mapped decoder per memdev.
> + */
> +int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
> +{
> +	struct cxl_attach_region *attach =
> +		container_of(cxlmd->attach, typeof(*attach), attach);
> +	struct cxl_port *endpoint = cxlmd->endpoint;
> +	int rc;
> +
> +	/* hold endpoint lock to setup autoremove of the region */
> +	guard(device)(&endpoint->dev);
> +	if (!endpoint->dev.driver)
> +		return -ENXIO;
> +
> +	rc = find_committed_attach_region(cxlmd, attach);
> +	if (rc != -ENODEV)
> +		return rc;


If I'm not wrong, the previous call could find a region therefore not 
needing the next call. Does it? If so the function name should change 
covering the two possibilities.


Thank you,

Alejandro.


> +
> +	return create_memdev_attach_region(cxlmd, attach);
> +}
>   EXPORT_SYMBOL_FOR_MODULES(cxl_memdev_attach_region, "cxl_mem");
>   
>   /*