Re: [PATCH v4 1/9] dax/hmem, e820, resource: Defer Soft Reserved insertion until hmem is ready

Koralahalli Channabasappa, Smita
2025-12-11T23:20:43Z

Hi,

Sorry for the delay here. I was on vacation. Responses inline.

On 12/2/2025 2:19 PM, dan.j.williams@intel.com wrote:
> Smita Koralahalli wrote:
>> From: Dan Williams <dan.j.williams@intel.com>
>>
>> Insert Soft Reserved memory into a dedicated soft_reserve_resource tree
>> instead of the iomem_resource tree at boot. Delay publishing these ranges
>> into the iomem hierarchy until ownership is resolved and the HMEM path
>> is ready to consume them.
>>
>> Publishing Soft Reserved ranges into iomem too early conflicts with CXL
>> hotplug and prevents region assembly when those ranges overlap CXL
>> windows.
>>
>> Follow up patches will reinsert Soft Reserved ranges into iomem after CXL
>> window publication is complete and HMEM is ready to claim the memory. This
>> provides a cleaner handoff between EFI-defined memory ranges and CXL
>> resource management without trimming or deleting resources later.
>
> Please, when you modify a patch from an original, add your
> Co-developed-by: and clarify what you changed.

Thanks Dan. Yeah, this was a bit of a gray area for me. I had the
impression or remember reading somewhere that Co-developed-by tags are
typically added only when the modifications are substantial, so I didn’t
include it initially. I will add the Co-developed-by: line.

>
>>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
>> ---
>> arch/x86/kernel/e820.c | 2 +-
>> drivers/cxl/acpi.c | 2 +-
>> drivers/dax/hmem/device.c | 4 +-
>> drivers/dax/hmem/hmem.c | 7 ++-
>> include/linux/ioport.h | 13 +++++-
>> kernel/resource.c | 92 +++++++++++++++++++++++++++++++++------
>> 6 files changed, 100 insertions(+), 20 deletions(-)
>>
> [..]
>> @@ -426,6 +443,26 @@ int walk_iomem_res_desc(unsigned long desc, unsigned long flags, u64 start,
>> }
>> EXPORT_SYMBOL_GPL(walk_iomem_res_desc);
>>
>> +#ifdef CONFIG_EFI_SOFT_RESERVE
>> +struct resource soft_reserve_resource = {
>> + .name = "Soft Reserved",
>> + .start = 0,
>> + .end = -1,
>> + .desc = IORES_DESC_SOFT_RESERVED,
>> + .flags = IORESOURCE_MEM,
>> +};
>> +EXPORT_SYMBOL_GPL(soft_reserve_resource);
>
> It looks like one of the things you changed from my RFC was the addition
> of walk_soft_reserve_res_desc() and region_intersects_soft_reserve().
> With those APIs not only does this symbol not need to be exported, but
> it also can be static / private to resource.c.

I remember these helpers were introduced in your RFC but I think they
weren't yet defined. With them in place, agreed there’s no need to
export soft_reserve_resource. Will fix this in the next revision.

>
>> +
>> +int walk_soft_reserve_res_desc(unsigned long desc, unsigned long flags,
>> + u64 start, u64 end, void *arg,
>> + int (*func)(struct resource *, void *))
>> +{
>> + return walk_res_desc(&soft_reserve_resource, start, end, flags, desc,
>> + arg, func);
>> +}
>> +EXPORT_SYMBOL_GPL(walk_soft_reserve_res_desc);
>> +#endif
>> +
>> /*
>> * This function calls the @func callback against all memory ranges of type
>> * System RAM which are marked as IORESOURCE_SYSTEM_RAM and IORESOUCE_BUSY.
>> @@ -648,6 +685,22 @@ int region_intersects(resource_size_t start, size_t size, unsigned long flags,
>> }
>> EXPORT_SYMBOL_GPL(region_intersects);
>>
>> +#ifdef CONFIG_EFI_SOFT_RESERVE
>> +int region_intersects_soft_reserve(resource_size_t start, size_t size,
>> + unsigned long flags, unsigned long desc)
>> +{
>> + int ret;
>> +
>> + read_lock(&resource_lock);
>> + ret = __region_intersects(&soft_reserve_resource, start, size, flags,
>> + desc);
>> + read_unlock(&resource_lock);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(region_intersects_soft_reserve);
>> +#endif
>> +
>> void __weak arch_remove_reservations(struct resource *avail)
>> {
>> }
>> @@ -966,7 +1019,7 @@ EXPORT_SYMBOL_GPL(insert_resource);
>> * Insert a resource into the resource tree, possibly expanding it in order
>> * to make it encompass any conflicting resources.
>> */
>> -void insert_resource_expand_to_fit(struct resource *root, struct resource *new)
>> +void __insert_resource_expand_to_fit(struct resource *root, struct resource *new)
>> {
>> if (new->parent)
>> return;
>> @@ -997,7 +1050,20 @@ void insert_resource_expand_to_fit(struct resource *root, struct resource *new)
>> * to use this interface. The former are built-in and only the latter,
>> * CXL, is a module.
>> */
>> -EXPORT_SYMBOL_NS_GPL(insert_resource_expand_to_fit, "CXL");
>> +EXPORT_SYMBOL_NS_GPL(__insert_resource_expand_to_fit, "CXL");
>> +
>> +void insert_resource_expand_to_fit(struct resource *new)
>> +{
>> + struct resource *root = &iomem_resource;
>> +
>> +#ifdef CONFIG_EFI_SOFT_RESERVE
>> + if (new->desc == IORES_DESC_SOFT_RESERVED)
>> + root = &soft_reserve_resource;
>> +#endif
>
> I can not say I am entirely happy with this change, I would prefer to
> avoid ifdef in C, and I would prefer not to break the legacy semantics
> of this function, but it meets the spirit of the original RFC without
> introducing a new insert_resource_late(). I assume review feedback
> requested this?

Yeah here,
https://lore.kernel.org/all/20250909161210.GBaMBR2rN8h6eT9JHe@fat_crate.local/

>
>> + __insert_resource_expand_to_fit(root, new);
>> +}
>> +EXPORT_SYMBOL_GPL(insert_resource_expand_to_fit);
>
> There are no consumers for this export, so it can be dropped.

Okay.

Thanks
Smita