Re: [PATCH v4 8/9] cxl/region, dax/hmem: Tear down CXL regions when HMEM reclaims Soft Reserved

Koralahalli Channabasappa, Smita
2025-12-12T22:12:18Z

On 12/3/2025 4:50 PM, dan.j.williams@intel.com wrote:
> Smita Koralahalli wrote:
>> If CXL regions do not fully cover a Soft Reserved span, HMEM takes
>> ownership. Tear down overlapping CXL regions before allowing HMEM to
>> register and online the memory.
>>
>> Add cxl_region_teardown() to walk CXL regions overlapping a span and
>> unregister them via devm_release_action() and unregister_region().
>>
>> Force the region state back to CXL_CONFIG_ACTIVE before unregistering to
>> prevent the teardown path from resetting decoders HMEM still relies on
>> to create its dax and online memory.
>>
>> Co-developed-by: Alison Schofield <alison.schofield@intel.com>
>> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
>> ---
>> drivers/cxl/core/region.c | 38 ++++++++++++++++++++++++++++++++++++++
>> drivers/cxl/cxl.h | 5 +++++
>> drivers/dax/hmem/hmem.c | 4 +++-
>> 3 files changed, 46 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 38e7ec6a087b..266b24028df0 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -3784,6 +3784,44 @@ struct cxl_range_ctx {
>> bool found;
>> };
>>
>> +static int cxl_region_teardown_cb(struct device *dev, void *data)
>> +{
>> + struct cxl_range_ctx *ctx = data;
>> + struct cxl_root_decoder *cxlrd;
>> + struct cxl_region_params *p;
>> + struct cxl_region *cxlr;
>> + struct cxl_port *port;
>> +
>> + cxlr = cxlr_overlapping_range(dev, ctx->start, ctx->end);
>> + if (!cxlr)
>> + return 0;
>> +
>> + cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
>> + port = cxlrd_to_port(cxlrd);
>> + p = &cxlr->params;
>> +
>> + /* Force the region state back to CXL_CONFIG_ACTIVE so that
>
> Minor, and moot given the follow on comments below, but please keep
> consistent comment-style and lead with a /*, i.e.:
>
> /*
> * Force the region...
>
>> + * unregister_region() does not run the full decoder reset path
>> + * which would invalidate the decoder programming that HMEM
>> + * relies on to create its DAX device and online the underlying
>> + * memory.
>> + */
>> + scoped_guard(rwsem_write, &cxl_rwsem.region)
>> + p->state = min(p->state, CXL_CONFIG_ACTIVE);
>
> I think the thickness of the above comment belies that this is too much
> of a layering violation and likely to cause problems. For minimizing the
> mental load of analyzing future bug reports, I want all regions gone
> when any handshake with the platform firmware and dax-hmem occurs. When
> that happens it may mean destroying regions that were dynamically
> created while waiting the wait_for_initial_probe() to timeout, who
> knows. The simple policy is "CXL subsystem understands everything, or
> touches nothing."
>
> For this reset determination, what I think makes more sense, and is
> generally useful for shutting down CXL even outside of the hmem deferral
> trickery, is to always record whether decoders were idle or not at the
> time of region creation. In fact we already have that flag, it is called
> CXL_REGION_F_AUTO.
>
> If CXL_REGION_F_AUTO is still set at detach_target() time, it means that
> we are giving up on auto-assembly and leaving the decoders alone.
>
> If the administrator actually wants to destroy and reclaim that
> physical address space then they need to forcefully de-commit that
> auto-assembled region via the @commit sysfs attribute. So that means
> commit_store() needs to clear CXL_REGION_F_AUTO to get the decoder reset
> to happen.
>
> [..]
>> void cxl_endpoint_parse_cdat(struct cxl_port *port);
>> diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
>> index b9312e0f2e62..7d874ee169ac 100644
>> --- a/drivers/dax/hmem/hmem.c
>> +++ b/drivers/dax/hmem/hmem.c
>> @@ -158,8 +158,10 @@ static int handle_deferred_cxl(struct device *host, int target_nid,
>> if (cxl_regions_fully_map(res->start, res->end)) {
>> dax_cxl_mode = DAX_CXL_MODE_DROP;
>> cxl_register_dax(res->start, res->end);
>> - } else
>> + } else {
>> dax_cxl_mode = DAX_CXL_MODE_REGISTER;
>> + cxl_region_teardown(res->start, res->end);
>> + }
>
> Like I alluded to above, I am not on board with making a range-by range
> decision on teardown. The check for "all clear" vs "abort" should be a
> global event before proceeding with either allowing cxl_region instances
> to attach or all of them get destroyed. Recall that if
> cxl_dax_region_probe() is globally rejecting all cxl_dax_region devices
> until dax_cxl_mode moves to DAX_CXL_MODE_DROP then it keeps a consistent
> behavior of all regions attach or none attach.

Thanks. I will restructure this and rely on CXL_REGION_F_AUTO semantics
for teardown.

I wanted to check one nuance to make sure Im applying this correctly.

Consider a case like:

Soft Reserved spans (HMAT-visible):
SR1: 0x085000–0x284fff
SR2: 0x285000–0x484fff
SR3: 0x485000–0x684fff

CXL regions:
R1: 0x085000–0x274fff
R2: 0x285000–0x484fff
R3: 0x485000–0x684fff

Here SR1 is not fully contained by any committed CXL region, even though
SR2 and SR3 are. In this scenario, my understanding is that the global
check should fail, and we abort CXL ownership entirely (tear down all
auto-assembled regions) and proceed with pure HMEM, rather than allowing
partial CXL ownership. Is that correct?

Related to that, the flow should be something like:

Inside process_defer_work()

process_defer_work()
wait_for_device_probe();
if (cxl_contains_soft_reserve())
dax_cxl_mode = DROP
rescan cxl-bus (bus_rescan_devices(&cxl_bus_type);)
else
dax_cxl_mode = REGISTER
teardown all regions.
walk_hmem_resources(&pdev->dev, hmem_register_device);
(Here we walk all hmem resources second time)..

and as you mentioned

cxl_contains_soft_reserve()
for_each_cxl_intersecting_hmem_resource()
(Here we walk all hmem resources first time)..
found = false
for_each_region()
if (resource_contains(cxl_region_resource, hmem_resource))
found = true
if (!found)
return false
return true

Thanks
Smita