On Thu, 20 Nov 2025 03:19:17 +0000 Smita Koralahalli <[email protected]> wrote:
> From: Dan Williams <[email protected]> > > 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 <[email protected]> > Signed-off-by: Smita Koralahalli <[email protected]> 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
