Message-ID: <b137dd39-dcf6-4203-adab-8c9ee2b3e6ef@amd.com>
Date: Thu, 29 Jan 2026 13:20:36 -0800
From: "Koralahalli Channabasappa, Smita" <skoralah@....com>
To: dan.j.williams@...el.com,
 Smita Koralahalli <Smita.KoralahalliChannabasappa@....com>,
 linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org,
 nvdimm@...ts.linux.dev, linux-fsdevel@...r.kernel.org,
 linux-pm@...r.kernel.org
Cc: Ard Biesheuvel <ardb@...nel.org>,
 Alison Schofield <alison.schofield@...el.com>,
 Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>,
 Jonathan Cameron <jonathan.cameron@...wei.com>,
 Yazen Ghannam <yazen.ghannam@....com>, Dave Jiang <dave.jiang@...el.com>,
 Davidlohr Bueso <dave@...olabs.net>, Matthew Wilcox <willy@...radead.org>,
 Jan Kara <jack@...e.cz>, "Rafael J . Wysocki" <rafael@...nel.org>,
 Len Brown <len.brown@...el.com>, Pavel Machek <pavel@...nel.org>,
 Li Ming <ming.li@...omail.com>, Jeff Johnson
 <jeff.johnson@....qualcomm.com>, Ying Huang <huang.ying.caritas@...il.com>,
 Yao Xingtao <yaoxt.fnst@...itsu.com>, Peter Zijlstra <peterz@...radead.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Nathan Fontenot <nathan.fontenot@....com>,
 Terry Bowman <terry.bowman@....com>, Robert Richter <rrichter@....com>,
 Benjamin Cheatham <benjamin.cheatham@....com>,
 Zhijian Li <lizhijian@...itsu.com>, Borislav Petkov <bp@...en8.de>,
 Tomasz Wolski <tomasz.wolski@...itsu.com>
Subject: Re: [PATCH v5 6/7] dax/hmem, cxl: Defer and resolve ownership of Soft
 Reserved memory ranges

Hi Dan,

On 1/28/2026 3:35 PM, dan.j.williams@...el.com wrote:
> Smita Koralahalli wrote:
>> The current probe time ownership check for Soft Reserved memory based
>> solely on CXL window intersection is insufficient. dax_hmem probing is not
>> always guaranteed to run after CXL enumeration and region assembly, which
>> can lead to incorrect ownership decisions before the CXL stack has
>> finished publishing windows and assembling committed regions.
>>
>> Introduce deferred ownership handling for Soft Reserved ranges that
>> intersect CXL windows at probe time by scheduling deferred work from
>> dax_hmem and waiting for the CXL stack to complete enumeration and region
>> assembly before deciding ownership.
>>
>> Evaluate ownership of Soft Reserved ranges based on CXL region
>> containment.
>>
>>     - If all Soft Reserved ranges are fully contained within committed CXL
>>       regions, DROP handling Soft Reserved ranges from dax_hmem and allow
>>       dax_cxl to bind.
>>
>>     - If any Soft Reserved range is not fully claimed by committed CXL
>>       region, tear down all CXL regions and REGISTER the Soft Reserved
>>       ranges with dax_hmem instead.
>>
>> While ownership resolution is pending, gate dax_cxl probing to avoid
>> binding prematurely.
>>
>> This enforces a strict ownership. Either CXL fully claims the Soft
>> Reserved ranges or it relinquishes it entirely.
>>
>> Co-developed-by: Dan Williams <dan.j.williams@...el.com>
>> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@....com>
>> ---
>>   drivers/cxl/core/region.c | 25 ++++++++++++
>>   drivers/cxl/cxl.h         |  2 +
>>   drivers/dax/cxl.c         |  9 +++++
>>   drivers/dax/hmem/hmem.c   | 81 ++++++++++++++++++++++++++++++++++++++-
>>   4 files changed, 115 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 9827a6dd3187..6c22a2d4abbb 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -3875,6 +3875,31 @@ static int cxl_region_debugfs_poison_clear(void *data, u64 offset)
>>   DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_clear_fops, NULL,
>>   			 cxl_region_debugfs_poison_clear, "%llx\n");
>>   
>> +static int cxl_region_teardown_cb(struct device *dev, void *data)
>> +{
>> +	struct cxl_root_decoder *cxlrd;
>> +	struct cxl_region *cxlr;
>> +	struct cxl_port *port;
>> +
>> +	if (!is_cxl_region(dev))
>> +		return 0;
>> +
>> +	cxlr = to_cxl_region(dev);
>> +
>> +	cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
>> +	port = cxlrd_to_port(cxlrd);
>> +
>> +	devm_release_action(port->uport_dev, unregister_region, cxlr);
>> +
>> +	return 0;
>> +}
>> +
>> +void cxl_region_teardown_all(void)
>> +{
>> +	bus_for_each_dev(&cxl_bus_type, NULL, NULL, cxl_region_teardown_cb);
>> +}
>> +EXPORT_SYMBOL_GPL(cxl_region_teardown_all);
>> +
>>   static int cxl_region_contains_sr_cb(struct device *dev, void *data)
>>   {
>>   	struct resource *res = data;
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index b0ff6b65ea0b..1864d35d5f69 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -907,6 +907,7 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled);
>>   struct cxl_dax_region *to_cxl_dax_region(struct device *dev);
>>   u64 cxl_port_get_spa_cache_alias(struct cxl_port *endpoint, u64 spa);
>>   bool cxl_region_contains_soft_reserve(const struct resource *res);
>> +void cxl_region_teardown_all(void);
>>   #else
>>   static inline bool is_cxl_pmem_region(struct device *dev)
>>   {
>> @@ -933,6 +934,7 @@ static inline bool cxl_region_contains_soft_reserve(const struct resource *res)
>>   {
>>   	return false;
>>   }
>> +static inline void cxl_region_teardown_all(void) { }
>>   #endif
>>   
>>   void cxl_endpoint_parse_cdat(struct cxl_port *port);
>> diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c
>> index 13cd94d32ff7..b7e90d6dd888 100644
>> --- a/drivers/dax/cxl.c
>> +++ b/drivers/dax/cxl.c
>> @@ -14,6 +14,15 @@ static int cxl_dax_region_probe(struct device *dev)
>>   	struct dax_region *dax_region;
>>   	struct dev_dax_data data;
>>   
>> +	switch (dax_cxl_mode) {
>> +	case DAX_CXL_MODE_DEFER:
>> +		return -EPROBE_DEFER;
> 
> So, I think this causes a mess because now you have 2 workqueues (driver
> core defer-queue and hmem work) competing to disposition this device.
> What this seems to want is to only run in the post "soft reserve
> dispositioned" world. Something like (untested!)
> 
> diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c
> index 13cd94d32ff7..1162495eb317 100644
> --- a/drivers/dax/cxl.c
> +++ b/drivers/dax/cxl.c
> @@ -14,6 +14,9 @@ static int cxl_dax_region_probe(struct device *dev)
>          struct dax_region *dax_region;
>          struct dev_dax_data data;
>   
> +       /* Make sure that dax_cxl_mode is stable, only runs once at boot */
> +       flush_hmem_work();
> +
>          if (nid == NUMA_NO_NODE)
>                  nid = memory_add_physaddr_to_nid(cxlr_dax->hpa_range.start);
>   
> @@ -38,6 +41,7 @@ static struct cxl_driver cxl_dax_region_driver = {
>          .id = CXL_DEVICE_DAX_REGION,
>          .drv = {
>                  .suppress_bind_attrs = true,
> +               .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>          },
>   };
>   
> ...where that flush_hmem_work() is something provided by
> drivers/dax/bus.c. The asynchronous probe is to make sure that the wait
> is always out-of-line of any other synchronous probing.
> 
> You could probably drop the work item from being a per hmem_platform
> drvdata and just make it a singleton work item in bus.c that hmem.c
> queues and cxl.c flushes.
> 
> Probably also need to make sure that hmem_init() always runs before
> dax_cxl module init with something like this for the built-in case:
> 
> diff --git a/drivers/dax/Makefile b/drivers/dax/Makefile
> index 5ed5c39857c8..70e996bf1526 100644
> --- a/drivers/dax/Makefile
> +++ b/drivers/dax/Makefile
> @@ -1,4 +1,5 @@
>   # SPDX-License-Identifier: GPL-2.0
> +obj-y += hmem/
>   obj-$(CONFIG_DAX) += dax.o
>   obj-$(CONFIG_DEV_DAX) += device_dax.o
>   obj-$(CONFIG_DEV_DAX_KMEM) += kmem.o
> @@ -10,5 +11,3 @@ dax-y += bus.o
>   device_dax-y := device.o
>   dax_pmem-y := pmem.o
>   dax_cxl-y := cxl.o
> -
> -obj-y += hmem/
> 
> [..]
>> +static void process_defer_work(struct work_struct *_work)
>> +{
>> +	struct dax_defer_work *work = container_of(_work, typeof(*work), work);
>> +	struct platform_device *pdev = work->pdev;
>> +	int rc;
>> +
>> +	/* relies on cxl_acpi and cxl_pci having had a chance to load */
>> +	wait_for_device_probe();
>> +
>> +	rc = walk_hmem_resources(&pdev->dev, cxl_contains_soft_reserve);
> 
> Like I said before this probably wants to be named something like
> soft_reserve_has_cxl_match() to make it clear what is happening.
> 
>> +
>> +	if (!rc) {
>> +		dax_cxl_mode = DAX_CXL_MODE_DROP;
>> +		rc = bus_rescan_devices(&cxl_bus_type);
>> +		if (rc)
>> +			dev_warn(&pdev->dev, "CXL bus rescan failed: %d\n", rc);
>> +	} else {
>> +		dax_cxl_mode = DAX_CXL_MODE_REGISTER;
>> +		cxl_region_teardown_all();
> 
> I was thinking through what Alison asked about what to do later in boot
> when other regions are being dynamically created. It made me wonder if
> this safety can be achieved more easily by just making sure that the
> alloc_dax_region() call fails.

Agreed with all the points above, including making alloc_dax_region() 
fail as the safety mechanism. This also cleanly avoids the no Soft 
Reserved case Alison pointed out, where dax_cxl_mode can remain stuck in 
DEFER and return -EPROBE_DEFER.

What I’m still trying to understand is the case of “other regions being 
dynamically created.” Once HMEM has claimed the relevant HPA range, any 
later userspace attempts to create regions (via cxl create-region) 
should naturally fail due to the existing HPA allocation. This already 
shows up as an HPA allocation failure currently.

#cxl create-region -d decoder0.0 -m mem2 -w 1 -g256
cxl region: create_region: region0: set_size failed: Numerical result 
out of range
cxl region: cmd_create_region: created 0 regions

And in the dmesg:
[  466.819353] alloc_hpa: cxl region0: HPA allocation error (-34) for 
size:0x0000002000000000 in CXL Window 0 [mem 0x850000000-0x284fffffff 
flags 0x200]

Also, at this point, with the probe-ordering fixes and the use of 
wait_for_device_probe(), region probing should have fully completed.

Am I missing any other scenario where regions could still be created 
dynamically beyond this?

> 
> Something like (untested / incomplete, needs cleanup handling!)
> 
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index fde29e0ad68b..fd18343e0538 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -10,6 +10,7 @@
>   #include "dax-private.h"
>   #include "bus.h"
>   
> +static struct resource dax_regions = DEFINE_RES_MEM_NAMED(0, -1, "DAX Regions");
>   static DEFINE_MUTEX(dax_bus_lock);
>   
>   /*
> @@ -661,11 +662,7 @@ struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>          dax_region->dev = parent;
>          dax_region->target_node = target_node;
>          ida_init(&dax_region->ida);
> -       dax_region->res = (struct resource) {
> -               .start = range->start,
> -               .end = range->end,
> -               .flags = IORESOURCE_MEM | flags,
> -       };
> +       dax_region->res = __request_region(&dax_regions, range->start, range->end, flags);
>   
>          if (sysfs_create_groups(&parent->kobj, dax_region_attribute_groups)) {
>                  kfree(dax_region);
> 
> ...which will result in enforcing only one of dax_hmem or dax_cxl being
> able to register a dax_region.
> 
> Yes, this would leave a mess of disabled cxl_dax_region devices lying
> around, but it would leave more breadcrumbs for debug, and reduce the
> number of races you need to worry about.
> 
> In other words, I thought total teardown would be simpler, but as the
> feedback keeps coming in, I think that brings a different set of
> complexity. So just inject failures for dax_cxl to trip over and then we
> can go further later to effect total teardown if that proves to not be
> enough.

One concern with the approach of not tearing down CXL regions is the 
state it leaves behind in /proc/iomem. Soft Reserved ranges are 
REGISTERed to HMEM while CXL regions remain present. The resulting 
nesting (dax under region, region under window and window under SR) 
visually suggests a coherent CXL hierarchy, even though ownership has 
effectively moved to HMEM. When users, then attempt to tear regions down 
and recreate them from userspace, they hit the same HPA allocation 
failures described above.

If we decide not to tear down regions in the REGISTER case, should we 
gate decoder resets during user initiated region teardown? Today, 
decoders are reset when regions are torn down dynamically, and 
subsequent attempts to recreate regions can trigger a large amount of 
mailbox traffic. Much of what shows up as repeated “Reading event logs/ 
Clearing …” messages which ends up interleaved with the HPA allocation 
failure, which can be confusing.

Thanks
Smita

Powered by blists - more mailing lists