Message-ID: <20251217120550.00003325@huawei.com>
Date: Wed, 17 Dec 2025 12:05:50 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Smita Koralahalli <Smita.KoralahalliChannabasappa@....com>
CC: <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<nvdimm@...ts.linux.dev>, <linux-fsdevel@...r.kernel.org>,
	<linux-pm@...r.kernel.org>, Alison Schofield <alison.schofield@...el.com>,
	Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>, Dan
 Williams <dan.j.williams@...el.com>, Yazen Ghannam <yazen.ghannam@....com>,
	Dave Jiang <dave.jiang@...el.com>, Davidlohr Bueso <dave@...olabs.net>,
	Matthew Wilcox <willy@...radead.org>, Jan Kara <jack@...e.cz>, "Rafael J .
 Wysocki" <rafael@...nel.org>, Len Brown <len.brown@...el.com>, Pavel Machek
	<pavel@...nel.org>, Li Ming <ming.li@...omail.com>, Jeff Johnson
	<jeff.johnson@....qualcomm.com>, "Ying Huang" <huang.ying.caritas@...il.com>,
	Yao Xingtao <yaoxt.fnst@...itsu.com>, Peter Zijlstra <peterz@...radead.org>,
	Greg KH <gregkh@...uxfoundation.org>, Nathan Fontenot
	<nathan.fontenot@....com>, Terry Bowman <terry.bowman@....com>, Robert
 Richter <rrichter@....com>, Benjamin Cheatham <benjamin.cheatham@....com>,
	Zhijian Li <lizhijian@...itsu.com>, Borislav Petkov <bp@...en8.de>, Ard
 Biesheuvel <ardb@...nel.org>
Subject: Re: [PATCH v4 1/9] dax/hmem, e820, resource: Defer Soft Reserved
 insertion until hmem is ready

On Thu, 20 Nov 2025 03:19:17 +0000
Smita Koralahalli <Smita.KoralahalliChannabasappa@....com> wrote:

> From: Dan Williams <dan.j.williams@...el.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@...el.com>
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@....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

Powered by blists - more mailing lists