Re: [RFC PATCH 14/20] cxl/region: Add cxl pmem region creation routine for region persistency
From: Neeraj Kumar
Date: Sat Jul 19 2025 - 03:18:22 EST
On 10/07/25 08:59AM, Dave Jiang wrote:
On 6/17/25 5:39 AM, Neeraj Kumar wrote:
Added exported cxl_create_pmem_region routine to create cxl pmem region
from LSA parsed cxl region information.
Inspirition for the function is taken from ndctl device attribute
(_store) call. It allocates cxlr and fills information parsed from LSA
and calls device_add(&cxlr->dev) to initiates further region creation
porbes
Signed-off-by: Neeraj Kumar <s.neeraj@xxxxxxxxxxx>
---
drivers/cxl/core/port.c | 6 ++
drivers/cxl/core/region.c | 208 ++++++++++++++++++++++++++++++++++++++
drivers/cxl/cxl.h | 11 ++
3 files changed, 225 insertions(+)
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index bca668193c49..2452f7c15b2d 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -2150,6 +2150,12 @@ void cxl_bus_drain(void)
}
EXPORT_SYMBOL_NS_GPL(cxl_bus_drain, "CXL");
+void cxl_wq_flush(void)
+{
+ flush_workqueue(cxl_bus_wq);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_wq_flush, "CXL");
+
bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd)
{
return queue_work(cxl_bus_wq, &cxlmd->detach_work);
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index b98b1ccffd1c..8990e3c3474d 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2522,6 +2522,214 @@ static ssize_t create_ram_region_show(struct device *dev,
return __create_region_show(to_cxl_root_decoder(dev), buf);
}
+static ssize_t update_region_size(struct cxl_region *cxlr, u64 val)
Maybe call it resize_or_free_region_hpa()?
Also rename 'val' to 'size'
Sure Dave, I will fix it next patch-set.
+{
+ int rc;
+
+ rc = down_write_killable(&cxl_region_rwsem);
+ if (rc)
+ return rc;
+
+ if (val)
+ rc = alloc_hpa(cxlr, val);
+ else
+ rc = free_hpa(cxlr);
+ up_write(&cxl_region_rwsem);
+
+ if (rc)
+ return rc;
+
+ return 0;
+}
Share common code with core/region.c:size_store(). Please use helper function and not duplicate code.
Sure, I will try to minimize duplicate code as much as possible in next patch-set.
+
+static ssize_t update_region_dpa_size(struct cxl_region *cxlr,
resize_or_free_dpa()
Sure, I will fix it in next patch-set
+ struct cxl_decoder *cxld,
+ unsigned long long size)
u64 size
Sure, I will fix it in next patch-set
+{
+ int rc;
+ struct cxl_endpoint_decoder *cxled =
+ to_cxl_endpoint_decoder(&cxld->dev);
+
+ if (!IS_ALIGNED(size, SZ_256M))
+ return -EINVAL;
+
+ rc = cxl_dpa_free(cxled);
+ if (rc)
+ return rc;
+
+ if (size == 0)
+ return 0;
+
+ rc = cxl_dpa_alloc(cxled, size);
+ if (rc)
+ return rc;
+
+ return 0;
+}
Share common code with core/port.c:dpa_size_store(). Please use helper function and not duplicate code.
Sure, I will try minimizing duplicate code as much as possible.
+
+static ssize_t update_region_dpa_mode(struct cxl_region *cxlr,
+ struct cxl_decoder *cxld)
+{
+ int rc;
+ struct cxl_endpoint_decoder *cxled =
+ to_cxl_endpoint_decoder(&cxld->dev);
+
+ rc = cxl_dpa_set_mode(cxled, CXL_DECODER_PMEM);
Don't think CXL_DECODER_PMEM exists any longer. It's CXL_PARTMODE_PMEM. Just beware there have been some changes while you are rebasing to the latest upstream code.
Thanks, I will rebase and fix it with latest change
+ if (rc)
+ return rc;
+
+ return 0;
+}
+
+static size_t attach_region_target(struct cxl_region *cxlr,
+ struct cxl_decoder *cxld, int pos)
+{
+ int rc;
+ struct cxl_endpoint_decoder *cxled =
+ to_cxl_endpoint_decoder(&cxld->dev);
+
+ rc = attach_target(cxlr, cxled, pos, TASK_INTERRUPTIBLE);
+
+ if (rc < 0)
+ return rc;
+
+ return 0;
+}
+
+static ssize_t commit_region(struct cxl_region *cxlr)
+{
+ struct cxl_region_params *p = &cxlr->params;
+ ssize_t rc;
+
+ rc = down_write_killable(&cxl_region_rwsem);
+ if (rc)
+ return rc;
+
+ /* Already in the requested state? */
+ if (p->state >= CXL_CONFIG_COMMIT)
+ goto out;
+
+ /* Not ready to commit? */
+ if (p->state < CXL_CONFIG_ACTIVE) {
+ rc = -ENXIO;
+ goto out;
+ }
+
+ /*
+ * Invalidate caches before region setup to drop any speculative
+ * consumption of this address space
+ */
+ rc = cxl_region_invalidate_memregion(cxlr);
+ if (rc)
+ goto out;
+
+ rc = cxl_region_decode_commit(cxlr);
+ if (rc == 0)
+ p->state = CXL_CONFIG_COMMIT;
+out:
+ up_write(&cxl_region_rwsem);
+ if (rc)
+ return rc;
+ return 0;
+}
Sharing common code with core/region.c:commit_store()?
Sure, I will handle it in next patch-set
+
+static struct cxl_region *
+devm_cxl_pmem_add_region(struct cxl_root_decoder *cxlrd,
+ struct cxl_decoder *cxld,
+ struct cxl_pmem_region_params *params, int id,
+ enum cxl_decoder_mode mode, enum cxl_decoder_type type)
+{
+ struct cxl_port *port;
+ struct cxl_region *cxlr;
+ struct cxl_region_params *p;
+ struct device *dev;
+ int rc;
+
+ if (!cxlrd)
+ return ERR_PTR(-EINVAL);
+
+ port = to_cxl_port(cxlrd->cxlsd.cxld.dev.parent);
+
+ cxlr = cxl_region_alloc(cxlrd, id);
+ if (IS_ERR(cxlr))
+ return cxlr;
+ cxlr->mode = mode;
+ cxlr->type = type;
+
+ dev = &cxlr->dev;
+ rc = dev_set_name(dev, "region%d", id);
+ if (rc)
+ goto err;
+> + p = &cxlr->params;
+ p->uuid = params->uuid;
+ p->interleave_ways = params->nlabel;
+ p->interleave_granularity = params->ig;
+
+ /* Update region size */
Not sure what value this comment adds
Sure, I will remove these comments
+ if (update_region_size(cxlr, params->rawsize))
+ goto err;
+
+ /* Flush cxl wq */
Can you explain here why a flush is needed?
These steps of region creation is taken from how ndctl creates region
using *_store(). I will re-look if its required here.
+ cxl_wq_flush();
+
+ /* Clear DPA Size */
comment provides no value
+ if (update_region_dpa_size(cxlr, cxld, 0))
+ goto err;
+
+ /* Update DPA mode */
same as above
+ if (update_region_dpa_mode(cxlr, cxld))
+ goto err;
+
+ /* Update DPA Size */
same as above
+ if (update_region_dpa_size(cxlr, cxld, params->rawsize))
+ goto err;
+
+ /* Attach region targets */
same as above
+ if (attach_region_target(cxlr, cxld, params->position))
+ goto err;
+
+ /* Commit Region */
same as above
Sure, I will remove unwanted comments
+ if (commit_region(cxlr))
+ goto err;
Can you please provide some verbose explanation as to what all these extra steps are doing for pmem versus devm_cxl_add_region() for dram?
These steps of region creation is taken from how ndctl creates region.
Sure I will update the comments accordingly.
+
+ rc = device_add(dev);
+ if (rc)
+ goto err;
+
+ rc = devm_add_action_or_reset(port->uport_dev, unregister_region, cxlr);
+ if (rc)
+ return ERR_PTR(rc);
+
+ dev_dbg(port->uport_dev, "%s: created %s\n",
+ dev_name(&cxlrd->cxlsd.cxld.dev), dev_name(dev));
+ return cxlr;
+
+err:
+ put_device(dev);
+ return ERR_PTR(rc);
+}
+
+struct cxl_region *cxl_create_pmem_region(struct cxl_root_decoder *cxlrd,
+ struct cxl_decoder *cxld,
+ struct cxl_pmem_region_params *params, int id)
+{
+ int rc;
+
+ rc = memregion_alloc(GFP_KERNEL);
+ if (rc < 0)
+ return ERR_PTR(rc);
+
+ if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) {
+ memregion_free(rc);
+ return ERR_PTR(-EBUSY);
+ }
+
+ return devm_cxl_pmem_add_region(cxlrd, cxld, params, id,
+ CXL_DECODER_PMEM, CXL_DECODER_HOSTONLYMEM);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_create_pmem_region, "CXL");
Can __create_region() be modified to determine whether to create dram region or pmem?
DJ
Sure, I will refactor it accordingly
Regards,
Neeraj