Re: [PATCH v5 6/7] dax/hmem, cxl: Defer and resolve ownership of Soft Reserved memory ranges

[Alejandro Lucero Palau <alucerop@amd.com>]

Hi Smita,


On 1/25/26 03:17, Koralahalli Channabasappa, Smita wrote:
> Hi Alejandro,
>
> On 1/23/2026 3:59 AM, Alejandro Lucero Palau wrote:
>>
>> On 1/22/26 04:55, 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.
>>
>>
>> I was not sure if I was understanding this properly, but after 
>> looking at the code I think I do ... but then I do not understand the 
>> reason behind. If I'm right, there could be two devices and therefore 
>> different soft reserved ranges, with one getting an automatic cxl 
>> region for all the range and the other without that, and the outcome 
>> would be the first one getting its region removed and added to hmem. 
>> Maybe I'm missing something obvious but, why? If there is a good 
>> reason, I think it should be documented in the commit and somewhere 
>> else.
>
> Yeah, if I understood Dan correctly, that's exactly the intended 
> behavior.


OK


>
> I'm trying to restate the "why" behind this based on Dan's earlier 
> guidance. Please correct me if I'm misrepresenting it Dan.
>
> The policy is meant to be coarse: If all SR ranges that intersect CXL 
> windows are fully contained by committed CXL regions, then we have 
> high confidence that the platform descriptions line up and CXL owns 
> the memory.
>
> If any SR range that intersects a CXL window is not fully covered by 
> committed regions then we treat that as unexpected platform 
> shenanigans. In that situation the intent is to give up on CXL 
> entirely for those SR ranges because partial ownership becomes ambiguous.
>
> This is why the fallback is global and not per range. The goal is to
> leave no room for mixed some SR to CXL, some SR to HMEM 
> configurations. Any mismatch should push the platform issue back to 
> the vendor to fix the description (ideally preserving the simplifying 
> assumption of a 1:1 correlation between CXL Regions and SR).


I guess it is a good policy but my concern is with Type2, and at the 
time this check is done the Type2 driver could have not been probed 
(dynamic module), so a soft reserved range could not have a cxl region 
and that not being an error. I do not think this is a problem for your 
patch but something I should add to mine. I'm not sure how to do so yet 
because I will need to do it using some information from the PCI device 
struct while all this patchset relies on "anonymous" soft reserved range.


>
> Thanks for pointing this out. I will update the why in the next revision.
>
>>
>>
>> I have also problems understanding the concurrency when handling the 
>> global dax_cxl_mode variable. It is modified inside 
>> process_defer_work() which I think can have different instances for 
>> different devices executed concurrently in different cores/workers 
>> (the system_wq used is not ordered). If I'm right race conditions are 
>> likely.
>
> Yeah, this is something I spent sometime thinking on. My rationale 
> behind not having it and where I'm still unsure:
>
> My assumption was that after wait_for_device_probe(), CXL topology 
> discovery and region commit are complete and stable. And each deferred 
> worker should observe the same CXL state and therefore compute the 
> same final policy (either DROP or REGISTER).
>

I think so as well.


> Also, I was assuming that even if multiple process_defer_work() 
> instances run, the operations they perform are effectively safe to 
> repeat.. though I'm not sure on this.
>

No if they run in parallel, what I think it could be the case. If there 
is just one soft reserved range, that is fine, but concurrent ones 
executing the code in process_defer_work() can trigger race conditions 
when updating the global variable. Although the code inside this 
function does the right thing, the walk when calling to 
hmem_register_device() will not.

I would say a global spinlock should be enough.


Thank you,

Alejandro


> cxl_region_teardown_all(): this ultimately triggers the 
> devm_release_action(... unregister_region ...) path. My expectation 
> was that these devm actions are single shot per device lifecycle, so 
> repeated teardown attempts should become noops. And 
> cxl_region_teardown_all() ultimately leads to cxl_decoder_detach(), 
> which takes "cxl_rwsem.region". That should serialize decoder detach 
> and region teardown.
>
> bus_rescan_devices(&cxl_bus_type): I assumed repeated rescans during 
> boot are fine as the rescan path will simply rediscover already 
> present devices..
>
> walk_hmem_resources(.., hmem_register_device): in the DROP case,I 
> thought running the walk multiple times is safe because devm managed 
> platform devices and memregion allocations should prevent duplicate 
> lifetime issues.
>
> So, even if multiple process_defer_work() instances execute 
> concurrently, the CXL operations involved in containment evaluation 
> (cxl_region_contains_soft_reserve()) and teardown are already guarded.
>
> But I'm still trying to understand if 
> bus_rescan_devices(&cxl_bus_type) is not safe when invoked concurrently?
>
> Or is the primary issue that dax_cxl_mode is a global updated from one 
> context and read from others, and should be synchronized even if the 
> computed final value will always be the same?
>
> Happy to correct if my understanding is incorrect.
>
> Thanks
> Smita
>
>>
>>
>>>
>>> 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@intel.com>
>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>> Signed-off-by: Smita Koralahalli 
>>> <Smita.KoralahalliChannabasappa@amd.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;
>>> +    case DAX_CXL_MODE_REGISTER:
>>> +        return -ENODEV;
>>> +    case DAX_CXL_MODE_DROP:
>>> +        break;
>>> +    }
>>> +
>>>       if (nid == NUMA_NO_NODE)
>>>           nid = memory_add_physaddr_to_nid(cxlr_dax->hpa_range.start);
>>> diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
>>> index 1e3424358490..bcb57d8678d7 100644
>>> --- a/drivers/dax/hmem/hmem.c
>>> +++ b/drivers/dax/hmem/hmem.c
>>> @@ -3,6 +3,7 @@
>>>   #include <linux/memregion.h>
>>>   #include <linux/module.h>
>>>   #include <linux/dax.h>
>>> +#include "../../cxl/cxl.h"
>>>   #include "../bus.h"
>>>   static bool region_idle;
>>> @@ -58,9 +59,15 @@ static void release_hmem(void *pdev)
>>>       platform_device_unregister(pdev);
>>>   }
>>> +struct dax_defer_work {
>>> +    struct platform_device *pdev;
>>> +    struct work_struct work;
>>> +};
>>> +
>>>   static int hmem_register_device(struct device *host, int target_nid,
>>>                   const struct resource *res)
>>>   {
>>> +    struct dax_defer_work *work = dev_get_drvdata(host);
>>>       struct platform_device *pdev;
>>>       struct memregion_info info;
>>>       long id;
>>> @@ -69,8 +76,18 @@ static int hmem_register_device(struct device 
>>> *host, int target_nid,
>>>       if (IS_ENABLED(CONFIG_DEV_DAX_CXL) &&
>>>           region_intersects(res->start, resource_size(res), 
>>> IORESOURCE_MEM,
>>>                     IORES_DESC_CXL) != REGION_DISJOINT) {
>>> -        dev_dbg(host, "deferring range to CXL: %pr\n", res);
>>> -        return 0;
>>> +        switch (dax_cxl_mode) {
>>> +        case DAX_CXL_MODE_DEFER:
>>> +            dev_dbg(host, "deferring range to CXL: %pr\n", res);
>>> +            schedule_work(&work->work);
>>> +            return 0;
>>> +        case DAX_CXL_MODE_REGISTER:
>>> +            dev_dbg(host, "registering CXL range: %pr\n", res);
>>> +            break;
>>> +        case DAX_CXL_MODE_DROP:
>>> +            dev_dbg(host, "dropping CXL range: %pr\n", res);
>>> +            return 0;
>>> +        }
>>>       }
>>>       rc = region_intersects_soft_reserve(res->start, 
>>> resource_size(res));
>>> @@ -123,8 +140,67 @@ static int hmem_register_device(struct device 
>>> *host, int target_nid,
>>>       return rc;
>>>   }
>>> +static int cxl_contains_soft_reserve(struct device *host, int 
>>> target_nid,
>>> +                     const struct resource *res)
>>> +{
>>> +    if (region_intersects(res->start, resource_size(res), 
>>> IORESOURCE_MEM,
>>> +                  IORES_DESC_CXL) != REGION_DISJOINT) {
>>> +        if (!cxl_region_contains_soft_reserve(res))
>>> +            return 1;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +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);
>>> +
>>> +    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();
>>> +    }
>>> +
>>> +    walk_hmem_resources(&pdev->dev, hmem_register_device);
>>> +}
>>> +
>>> +static void kill_defer_work(void *_work)
>>> +{
>>> +    struct dax_defer_work *work = container_of(_work, 
>>> typeof(*work), work);
>>> +
>>> +    cancel_work_sync(&work->work);
>>> +    kfree(work);
>>> +}
>>> +
>>>   static int dax_hmem_platform_probe(struct platform_device *pdev)
>>>   {
>>> +    struct dax_defer_work *work = kzalloc(sizeof(*work), GFP_KERNEL);
>>> +    int rc;
>>> +
>>> +    if (!work)
>>> +        return -ENOMEM;
>>> +
>>> +    work->pdev = pdev;
>>> +    INIT_WORK(&work->work, process_defer_work);
>>> +
>>> +    rc = devm_add_action_or_reset(&pdev->dev, kill_defer_work, work);
>>> +    if (rc)
>>> +        return rc;
>>> +
>>> +    platform_set_drvdata(pdev, work);
>>> +
>>>       return walk_hmem_resources(&pdev->dev, hmem_register_device);
>>>   }
>>> @@ -174,3 +250,4 @@ MODULE_ALIAS("platform:hmem_platform*");
>>>   MODULE_DESCRIPTION("HMEM DAX: direct access to 'specific purpose' 
>>> memory");
>>>   MODULE_LICENSE("GPL v2");
>>>   MODULE_AUTHOR("Intel Corporation");
>>> +MODULE_IMPORT_NS("CXL");
>