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

Jonathan Cameron
2025-12-17T12:06:04Z

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