| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fdc06026-c681-4dae-9202-ad89293931a5@intel.com>
Date: Thu, 10 Jul 2025 08:59:15 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Neeraj Kumar <s.neeraj@...sung.com>, dan.j.williams@...el.com,
dave@...olabs.net, jonathan.cameron@...wei.com, alison.schofield@...el.com,
vishal.l.verma@...el.com, ira.weiny@...el.com
Cc: a.manzanares@...sung.com, nifan.cxl@...il.com, anisa.su@...sung.com,
vishak.g@...sung.com, krish.reddy@...sung.com, arun.george@...sung.com,
alok.rathore@...sung.com, neeraj.kernel@...il.com,
linux-kernel@...r.kernel.org, linux-cxl@...r.kernel.org,
nvdimm@...ts.linux.dev, gost.dev@...sung.com, cpgs@...sung.com
Subject: Re: [RFC PATCH 14/20] cxl/region: Add cxl pmem region creation
routine for region persistency
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@...sung.com>
> ---
> 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'
> +{
> + 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.
> +
> +static ssize_t update_region_dpa_size(struct cxl_region *cxlr,
resize_or_free_dpa()
> + struct cxl_decoder *cxld,
> + unsigned long long size)
u64 size
> +{
> + 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.
> +
> +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.
> + 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()?
> +
> +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
> + if (update_region_size(cxlr, params->rawsize))
> + goto err;
> +
> + /* Flush cxl wq */
Can you explain here why a flush is needed?
> + 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
> + 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?
> +
> + 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
> +
> static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
> enum cxl_decoder_mode mode, int id)
> {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 9423ea3509ad..30c80e04cb27 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -759,6 +759,7 @@ DEFINE_FREE(put_cxl_port, struct cxl_port *, if (!IS_ERR_OR_NULL(_T)) put_device
> int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
> void cxl_bus_rescan(void);
> void cxl_bus_drain(void);
> +void cxl_wq_flush(void);
> struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev,
> struct cxl_dport **dport);
> struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
> @@ -877,6 +878,9 @@ struct cxl_pmem_region *to_cxl_pmem_region(struct device *dev);
> int cxl_add_to_region(struct cxl_port *root,
> struct cxl_endpoint_decoder *cxled);
> struct cxl_dax_region *to_cxl_dax_region(struct device *dev);
> +struct cxl_region *cxl_create_pmem_region(struct cxl_root_decoder *cxlrd,
> + struct cxl_decoder *cxld,
> + struct cxl_pmem_region_params *params, int id);
> #else
> static inline bool is_cxl_pmem_region(struct device *dev)
> {
> @@ -895,6 +899,13 @@ static inline struct cxl_dax_region *to_cxl_dax_region(struct device *dev)
> {
> return NULL;
> }
> +static inline struct cxl_region *cxl_create_pmem_region(
> + struct cxl_root_decoder *cxlrd,
> + struct cxl_decoder *cxld,
> + struct cxl_pmem_region_params *params, int id)
> +{
> + return NULL;
> +}
> #endif
>
> void cxl_endpoint_parse_cdat(struct cxl_port *port);
Powered by blists - more mailing lists