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

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Thu, 20 Nov 2025 03:19:17 +0000
Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> 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.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>

A couple of general comments below.  I don't feel particularly strongly
about any of them however if you disagree! (other than the ever important
number of blank lines!) :)

Jonathan


> diff --git a/drivers/dax/hmem/device.c b/drivers/dax/hmem/device.c
> index f9e1a76a04a9..22732b729017 100644
> --- a/drivers/dax/hmem/device.c
> +++ b/drivers/dax/hmem/device.c
> @@ -83,8 +83,8 @@ static __init int hmem_register_one(struct resource *res, void *data)
>  
>  static __init int hmem_init(void)
>  {
> -	walk_iomem_res_desc(IORES_DESC_SOFT_RESERVED,
> -			IORESOURCE_MEM, 0, -1, NULL, hmem_register_one);
> +	walk_soft_reserve_res_desc(IORES_DESC_SOFT_RESERVED, IORESOURCE_MEM, 0,

Similar to below. If we are only putting MEM of type SOFT_RESERVED in here
can we drop those two as parameters?

> +				   -1, NULL, hmem_register_one);
>  	return 0;
>  }
>  
> diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
> index c18451a37e4f..48f4642f4bb8 100644
> --- a/drivers/dax/hmem/hmem.c
> +++ b/drivers/dax/hmem/hmem.c
> @@ -73,11 +73,14 @@ static int hmem_register_device(struct device *host, int target_nid,
>  		return 0;
>  	}
>  
> -	rc = region_intersects(res->start, resource_size(res), IORESOURCE_MEM,
> -			       IORES_DESC_SOFT_RESERVED);
> +	rc = region_intersects_soft_reserve(res->start, resource_size(res),
> +					    IORESOURCE_MEM,
> +					    IORES_DESC_SOFT_RESERVED);

The flags seem perhaps redundant.  Trade off between matching the more complex
existing functions and simplfying this.  Maybe push them down into the
call and just have
	rc = region_intersects_soft_reserved(res->start, resource_size(res));
here?

>  	if (rc != REGION_INTERSECTS)
>  		return 0;
>  
> +	/* TODO: Add Soft-Reserved memory back to iomem */
> +
>  	id = memregion_alloc(GFP_KERNEL);
>  	if (id < 0) {
>  		dev_err(host, "memregion allocation failure for %pr\n", res);

> diff --git a/kernel/resource.c b/kernel/resource.c
> index b9fa2a4ce089..208eaafcc681 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c

> @@ -402,6 +410,15 @@ static int __walk_iomem_res_desc(resource_size_t start, resource_size_t end,
>  	return ret;
>  }
>  
> +static int __walk_iomem_res_desc(resource_size_t start, resource_size_t end,
> +				 unsigned long flags, unsigned long desc,
> +				 void *arg,
> +				 int (*func)(struct resource *, void *))
> +{
> +	return walk_res_desc(&iomem_resource, start, end, flags, desc, arg, func);
> +}
> +

Local style seems to be single line breaks - stick to that unless I'm missing
some reason this one is special.

> +
>  /**

>  /*
>   * 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;
Perhaps the shortening of code makes it worth implementing this as:

	guard(read_lock)(&resource_lock);
	return __region_intersects();

Or ignore that until someone feels like a more general use of that
infrastructure in this file.  Looks like there are a bunch of places
where I'd argue it is worth doing.

Jonathan


> +}
> +EXPORT_SYMBOL_GPL(region_intersects_soft_reserve);
> +#endif