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

["Koralahalli Channabasappa, Smita" <skoralah@amd.com>]

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