| 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: <4fc3ee3e-d764-46c7-ae45-a835e0080c98@amd.com>
Date: Mon, 12 Jan 2026 15:10:29 -0600
From: "Cheatham, Benjamin" <benjamin.cheatham@....com>
To: Gregory Price <gourry@...rry.net>, <linux-cxl@...r.kernel.org>
CC: <linux-kernel@...r.kernel.org>, <kernel-team@...a.com>,
<dave@...olabs.net>, <jonathan.cameron@...wei.com>, <dave.jiang@...el.com>,
<alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
<ira.weiny@...el.com>, <dan.j.williams@...el.com>
Subject: Re: [PATCH 1/6] drivers/cxl: add cxl_memctrl_mode and region->memctrl
On 1/12/2026 10:35 AM, Gregory Price wrote:
> The CXL driver presently hands policy management over to DAX subsystem
> for sysram regions, which makes building policy around the entire region
> clunky and at time difficult (e.g. multiple actions to offline and
> hot-unplug memory reliably).
>
> To support multiple backend controllers for memory regions (for example
> dax vs direct hotplug), implement a memctrl field in cxl_region allows
> switching uncomitted regions between different "memory controllers".
>
> CXL_CONTROL_NONE: No selected controller, probe will fail.
> CXL_CONTROL_AUTO: If memory is already online as SysRAM, no controller
> otherwise register a dax_region
I think you can streamline this to get rid of CXL_CONTROL_AUTO. If BIOS set up
the memory you can just set the mode to CXL_CONTROL_NONE, otherwise use CXL_CONTROL_DAX.
This patch would be a bit less complicated at the very least, and I don't think it
would require much reworking of later patches.
> CXL_CONTROL_DAX : register a dax_region
>
> Auto regions will either be static sysram (BIOS-onlined) and has no
> region controller associated with it - or if the SP bit was set a
> DAX device will be created.
>
> Rather than default all regions to the auto-controller, only default
> auto-regions to the auto controller.
>
> Non-auto regions will be defaulted to CXL_CONTROL_NONE, which will cause
> a failure to probe unless a controller is selected.
>
> Signed-off-by: Gregory Price <gourry@...rry.net>
> ---
> drivers/cxl/core/Makefile | 1 +
> drivers/cxl/core/core.h | 2 +
> drivers/cxl/core/memctrl/Makefile | 4 +
> drivers/cxl/core/memctrl/dax_region.c | 79 +++++++++++++++
> drivers/cxl/core/memctrl/memctrl.c | 42 ++++++++
> drivers/cxl/core/region.c | 136 ++++++++++----------------
> drivers/cxl/cxl.h | 14 +++
> 7 files changed, 192 insertions(+), 86 deletions(-)
> create mode 100644 drivers/cxl/core/memctrl/Makefile
> create mode 100644 drivers/cxl/core/memctrl/dax_region.c
> create mode 100644 drivers/cxl/core/memctrl/memctrl.c
>
> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> index 5ad8fef210b5..79de20e3f8aa 100644
> --- a/drivers/cxl/core/Makefile
> +++ b/drivers/cxl/core/Makefile
> @@ -17,6 +17,7 @@ cxl_core-y += cdat.o
> cxl_core-y += ras.o
> cxl_core-$(CONFIG_TRACING) += trace.o
> cxl_core-$(CONFIG_CXL_REGION) += region.o
> +include $(src)/memctrl/Makefile
> cxl_core-$(CONFIG_CXL_MCE) += mce.o
> cxl_core-$(CONFIG_CXL_FEATURES) += features.o
> cxl_core-$(CONFIG_CXL_EDAC_MEM_FEATURES) += edac.o
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 1fb66132b777..1156a4bd0080 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -42,6 +42,8 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port);
> struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa);
> u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> u64 dpa);
> +int cxl_enable_memctrl(struct cxl_region *cxlr);
> +int devm_cxl_add_dax_region(struct cxl_region *cxlr);
>
> #else
> static inline u64 cxl_dpa_to_hpa(struct cxl_region *cxlr,
> diff --git a/drivers/cxl/core/memctrl/Makefile b/drivers/cxl/core/memctrl/Makefile
> new file mode 100644
> index 000000000000..8165aad5a52a
> --- /dev/null
> +++ b/drivers/cxl/core/memctrl/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +cxl_core-$(CONFIG_CXL_REGION) += memctrl/memctrl.o
> +cxl_core-$(CONFIG_CXL_REGION) += memctrl/dax_region.o
> diff --git a/drivers/cxl/core/memctrl/dax_region.c b/drivers/cxl/core/memctrl/dax_region.c
> new file mode 100644
> index 000000000000..90d7fdb97013
> --- /dev/null
> +++ b/drivers/cxl/core/memctrl/dax_region.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <cxlmem.h>
> +#include <cxl.h>
> +#include "../core.h"
> +
> +static struct lock_class_key cxl_dax_region_key;
> +
> +static struct cxl_dax_region *cxl_dax_region_alloc(struct cxl_region *cxlr)
> +{
> + struct cxl_region_params *p = &cxlr->params;
> + struct cxl_dax_region *cxlr_dax;
> + struct device *dev;
> +
> + guard(rwsem_read)(&cxl_rwsem.region);
> + if (p->state != CXL_CONFIG_COMMIT)
> + return ERR_PTR(-ENXIO);
> +
> + cxlr_dax = kzalloc(sizeof(*cxlr_dax), GFP_KERNEL);
> + if (!cxlr_dax)
> + return ERR_PTR(-ENOMEM);
> +
> + cxlr_dax->hpa_range.start = p->res->start;
> + cxlr_dax->hpa_range.end = p->res->end;
> +
> + dev = &cxlr_dax->dev;
> + cxlr_dax->cxlr = cxlr;
> + device_initialize(dev);
> + lockdep_set_class(&dev->mutex, &cxl_dax_region_key);
> + device_set_pm_not_required(dev);
> + dev->parent = &cxlr->dev;
> + dev->bus = &cxl_bus_type;
> + dev->type = &cxl_dax_region_type;
> +
> + return cxlr_dax;
> +}
> +
> +static void cxlr_dax_unregister(void *_cxlr_dax)
> +{
> + struct cxl_dax_region *cxlr_dax = _cxlr_dax;
> +
> + device_unregister(&cxlr_dax->dev);
> +}
> +
> +/*
> + * The dax controller is the default controller and simply hands the
> + * control pattern over to the dax driver. It does with a dax_region
> + * built by dax/cxl.c
> + */
> +int devm_cxl_add_dax_region(struct cxl_region *cxlr)
> +{
> + struct cxl_dax_region *cxlr_dax;
> + struct device *dev;
> + int rc;
> +
> + cxlr_dax = cxl_dax_region_alloc(cxlr);
> + if (IS_ERR(cxlr_dax))
> + return PTR_ERR(cxlr_dax);
> +
> + dev = &cxlr_dax->dev;
> + rc = dev_set_name(dev, "dax_region%d", cxlr->id);
> + if (rc)
> + goto err;
> +
> + rc = device_add(dev);
> + if (rc)
> + goto err;
> +
> + dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent),
> + dev_name(dev));
> +
> + return devm_add_action_or_reset(&cxlr->dev, cxlr_dax_unregister,
> + cxlr_dax);
> +err:
> + put_device(dev);
> + return rc;
> +}
> diff --git a/drivers/cxl/core/memctrl/memctrl.c b/drivers/cxl/core/memctrl/memctrl.c
> new file mode 100644
> index 000000000000..24e0e14b39c7
> --- /dev/null
> +++ b/drivers/cxl/core/memctrl/memctrl.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> +/* Copyright(c) 2026 Meta Inc. All rights reserved. */
> +#include <linux/device.h>
> +#include <linux/ioport.h>
> +#include <cxlmem.h>
> +#include <cxl.h>
> +#include "../core.h"
> +
> +static int is_system_ram(struct resource *res, void *arg)
> +{
> + struct cxl_region *cxlr = arg;
> + struct cxl_region_params *p = &cxlr->params;
> +
> + dev_dbg(&cxlr->dev, "%pr has System RAM: %pr\n", p->res, res);
> + return 1;
> +}
> +
> +int cxl_enable_memctrl(struct cxl_region *cxlr)
> +{
> + struct cxl_region_params *p = &cxlr->params;
> +
> + switch (cxlr->memctrl) {
> + case CXL_MEMCTRL_AUTO:
> + /*
> + * The region can not be manged by CXL if any portion of
s/manged/managed. May as well fix it since it's being moved.
> + * it is already online as 'System RAM'
> + */
> + if (walk_iomem_res_desc(IORES_DESC_NONE,
> + IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY,
> + p->res->start, p->res->end, cxlr,
> + is_system_ram) > 0)
> + return 0;
> + return devm_cxl_add_dax_region(cxlr);
If you take my suggestion at the top about removing MEMCTRL_AUTO, this case become
the MEMCTRL_DAX case.
> + case CXL_MEMCTRL_DAX:
> + return devm_cxl_add_dax_region(cxlr);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index ae899f68551f..02d7d9ae0252 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -626,6 +626,50 @@ static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
> }
> static DEVICE_ATTR_RO(mode);
>
> +static ssize_t ctrl_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct cxl_region *cxlr = to_cxl_region(dev);
> + const char *desc;
> +
> + switch (cxlr->memctrl) {
> + case CXL_MEMCTRL_AUTO:
> + desc = "auto";
> + break;
> + case CXL_MEMCTRL_DAX:
> + desc = "dax";
> + break;
> + default:
> + desc = "";
Nit: I would prefer this say "none" instead of being blank to match the code.
> + break;
> + }
> +
> + return sysfs_emit(buf, "%s\n", desc);
> +}
> +
> +static ssize_t ctrl_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct cxl_region *cxlr = to_cxl_region(dev);
> + struct cxl_region_params *p = &cxlr->params;
> + int rc;
> +
> + ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
> + if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
> + return rc;
> +
> + if (p->state >= CXL_CONFIG_COMMIT)
> + return -EBUSY;
> +
> + if (sysfs_streq(buf, "dax"))
> + cxlr->memctrl = CXL_MEMCTRL_DAX;
> + else
> + return -EINVAL;
> +
> + return len;
> +}
> +static DEVICE_ATTR_RW(ctrl);
> +
> static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size)
> {
> struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> @@ -772,6 +816,7 @@ static struct attribute *cxl_region_attrs[] = {
> &dev_attr_size.attr,
> &dev_attr_mode.attr,
> &dev_attr_extended_linear_cache_size.attr,
> + &dev_attr_ctrl.attr,
> NULL,
> };
>
> @@ -2598,6 +2643,7 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
> return cxlr;
> cxlr->mode = mode;
> cxlr->type = type;
> + cxlr->memctrl = CXL_MEMCTRL_NONE;
>
> dev = &cxlr->dev;
> rc = dev_set_name(dev, "region%d", id);
> @@ -3307,37 +3353,6 @@ struct cxl_dax_region *to_cxl_dax_region(struct device *dev)
> }
> EXPORT_SYMBOL_NS_GPL(to_cxl_dax_region, "CXL");
>
> -static struct lock_class_key cxl_dax_region_key;
> -
> -static struct cxl_dax_region *cxl_dax_region_alloc(struct cxl_region *cxlr)
> -{
> - struct cxl_region_params *p = &cxlr->params;
> - struct cxl_dax_region *cxlr_dax;
> - struct device *dev;
> -
> - guard(rwsem_read)(&cxl_rwsem.region);
> - if (p->state != CXL_CONFIG_COMMIT)
> - return ERR_PTR(-ENXIO);
> -
> - cxlr_dax = kzalloc(sizeof(*cxlr_dax), GFP_KERNEL);
> - if (!cxlr_dax)
> - return ERR_PTR(-ENOMEM);
> -
> - cxlr_dax->hpa_range.start = p->res->start;
> - cxlr_dax->hpa_range.end = p->res->end;
> -
> - dev = &cxlr_dax->dev;
> - cxlr_dax->cxlr = cxlr;
> - device_initialize(dev);
> - lockdep_set_class(&dev->mutex, &cxl_dax_region_key);
> - device_set_pm_not_required(dev);
> - dev->parent = &cxlr->dev;
> - dev->bus = &cxl_bus_type;
> - dev->type = &cxl_dax_region_type;
> -
> - return cxlr_dax;
> -}
> -
> static void cxlr_pmem_unregister(void *_cxlr_pmem)
> {
> struct cxl_pmem_region *cxlr_pmem = _cxlr_pmem;
> @@ -3424,42 +3439,6 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
> return rc;
> }
>
> -static void cxlr_dax_unregister(void *_cxlr_dax)
> -{
> - struct cxl_dax_region *cxlr_dax = _cxlr_dax;
> -
> - device_unregister(&cxlr_dax->dev);
> -}
> -
> -static int devm_cxl_add_dax_region(struct cxl_region *cxlr)
> -{
> - struct cxl_dax_region *cxlr_dax;
> - struct device *dev;
> - int rc;
> -
> - cxlr_dax = cxl_dax_region_alloc(cxlr);
> - if (IS_ERR(cxlr_dax))
> - return PTR_ERR(cxlr_dax);
> -
> - dev = &cxlr_dax->dev;
> - rc = dev_set_name(dev, "dax_region%d", cxlr->id);
> - if (rc)
> - goto err;
> -
> - rc = device_add(dev);
> - if (rc)
> - goto err;
> -
> - dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent),
> - dev_name(dev));
> -
> - return devm_add_action_or_reset(&cxlr->dev, cxlr_dax_unregister,
> - cxlr_dax);
> -err:
> - put_device(dev);
> - return rc;
> -}
> -
> static int match_decoder_by_range(struct device *dev, const void *data)
> {
> const struct range *r1, *r2 = data;
> @@ -3579,6 +3558,9 @@ static int __construct_region(struct cxl_region *cxlr,
>
> set_bit(CXL_REGION_F_AUTO, &cxlr->flags);
>
> + /* Auto-regions will either be static sysram (onlined by BIOS) or DAX */
> + cxlr->memctrl = CXL_MEMCTRL_AUTO;
And this would be set to CXL_MEM_CTRL_DAX instead.
> +
> res = kmalloc(sizeof(*res), GFP_KERNEL);
> if (!res)
> return -ENOMEM;
> @@ -3755,15 +3737,6 @@ u64 cxl_port_get_spa_cache_alias(struct cxl_port *endpoint, u64 spa)
> }
> EXPORT_SYMBOL_NS_GPL(cxl_port_get_spa_cache_alias, "CXL");
>
> -static int is_system_ram(struct resource *res, void *arg)
> -{
> - struct cxl_region *cxlr = arg;
> - struct cxl_region_params *p = &cxlr->params;
> -
> - dev_dbg(&cxlr->dev, "%pr has System RAM: %pr\n", p->res, res);
> - return 1;
> -}
> -
> static void shutdown_notifiers(void *_cxlr)
> {
> struct cxl_region *cxlr = _cxlr;
> @@ -3965,16 +3938,7 @@ static int cxl_region_probe(struct device *dev)
> dev_dbg(&cxlr->dev, "CXL EDAC registration for region_id=%d failed\n",
> cxlr->id);
>
> - /*
> - * The region can not be manged by CXL if any portion of
> - * it is already online as 'System RAM'
> - */
> - if (walk_iomem_res_desc(IORES_DESC_NONE,
> - IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY,
> - p->res->start, p->res->end, cxlr,
> - is_system_ram) > 0)
> - return 0;
> - return devm_cxl_add_dax_region(cxlr);
> + return cxl_enable_memctrl(cxlr);
> default:
> dev_dbg(&cxlr->dev, "unsupported region mode: %d\n",
> cxlr->mode);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index ba17fa86d249..b8fabaa77262 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -502,6 +502,19 @@ enum cxl_partition_mode {
> CXL_PARTMODE_PMEM,
> };
>
> +
> +/*
> + * Memory Controller modes:
> + * None - No controller selected
> + * Auto - either BIOS-configured as SysRAM, or default to DAX
> + * DAX - creates a dax_region controller for the cxl_region
> + */
> +enum cxl_memctrl_mode {
> + CXL_MEMCTRL_NONE,
> + CXL_MEMCTRL_AUTO,
> + CXL_MEMCTRL_DAX,
> +};
> +
> /*
> * Indicate whether this region has been assembled by autodetection or
> * userspace assembly. Prevent endpoint decoders outside of automatic
> @@ -543,6 +556,7 @@ struct cxl_region {
> struct device dev;
> int id;
> enum cxl_partition_mode mode;
> + enum cxl_memctrl_mode memctrl;
> enum cxl_decoder_type type;
> struct cxl_nvdimm_bridge *cxl_nvb;
> struct cxl_pmem_region *cxlr_pmem;
Powered by blists - more mailing lists