Re: [PATCH] cxl: Update Soft Reserved resources upon region creation

["Fontenot, Nathan" <nafonten@amd.com>]

responses below...

On 12/11/2024 5:31 AM, Andy Shevchenko wrote:
> On Mon, Dec 02, 2024 at 09:55:42AM -0600, Nathan Fontenot wrote:
>> Update handling of SOFT RESERVE iomem resources that intersect with
>> CXL region resources to remove the intersections from the SOFT RESERVE
>> resources. The current approach of leaving the SOFT RESERVE
>> resource as is can cause failures during hotplug replace of CXL
>> devices because the resource is not available for reuse after
>> teardown of the CXL device.
>>
>> The approach is to trim out any pieces of SOFT RESERVE resources
>> that intersect CXL regions. To do this, first set aside any SOFT RESERVE
>> resources that intersect with a CFMWS into a separate resource tree
>> during e820__reserve_resources_late() that would have been otherwise
>> added to the iomem resource tree.
>>
>> As CXL regions are created the cxl resource created for the new
>> region is used to trim intersections from the SOFT RESERVE
>> resources that were previously set aside.
>>
>> Once CXL device probe has completed ant remaining SOFT RESERVE resources
>> remaining are added to the iomem resource tree. As each resource
>> is added to the oiomem resource tree a new notifier chain is invoked
>> to notify the dax driver of newly added SOFT RESERVE resources so that
>> the dax driver can consume them.
> 
> ...
> 
>>  void __init e820__reserve_resources_late(void)
>>  {
>> -	int i;
>>  	struct resource *res;
>> +	int i;
> 
> Unrelated change.
> 
> ...
> 
>> -	for (i = 0; i < e820_table->nr_entries; i++) {
> 
>> +	for (i = 0; i < e820_table->nr_entries; i++, res++) {
> 
> 
>> -		res++;
> 
> Unrelated change.

I can remove these unrelated changes.

> 
> 
>>  	}
> 
> ...
> 
>> +static struct notifier_block hmem_nb = {
>> +	.notifier_call = dax_hmem_cb
> 
> It's better to leave trailing comma as it reduces churn in the future is
> anything to add here.
> 
>> +};
> 
> ...
> 
>> +++ b/drivers/dax/hmem/hmem.h
>> @@ -0,0 +1,11 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#ifndef _HMEM_H
>> +#define _HMEM_H
> 
> This needs a few forward declarations
> 
> struct device;
> struct platform_device;
> struct resource;

The next version of the patch removes this newly created .h file but good point
for any new headers created.
 
> 
>> +typedef int (*walk_hmem_fn)(struct device *dev, int target_nid,
>> +			    const struct resource *res);
>> +int walk_hmem_resources(struct device *dev, walk_hmem_fn fn);
>> +
>> +extern struct platform_device *hmem_pdev;
>> +
>> +#endif
> 
> ...
> 
>>  #include <linux/compiler.h>
>>  #include <linux/minmax.h>
>>  #include <linux/types.h>
>> +#include <linux/notifier.h>
> 
> I would put it before types.h to have more ordered piece.

will do.

> 
> ...
> 
>> +extern void trim_soft_reserve_resources(const struct resource *res);
>> +extern void merge_soft_reserve_resources(void);
>> +extern int insert_soft_reserve_resource(struct resource *res);
>> +extern int register_soft_reserve_notifier(struct notifier_block *nb);
>> +extern int unregister_soft_reserve_notifier(struct notifier_block *nb);
> 
> Why extern?

You're correct, extern isn't needed. I used it to follow what is done for other
declarations in the file only.

> 
> ...
> 
>> +++ b/kernel/resource.c
>> @@ -30,7 +30,7 @@
>>  #include <linux/string.h>
>>  #include <linux/vmalloc.h>
>>  #include <asm/io.h>
>> -
>> +#include <linux/acpi.h>
> 
> We don't usually interleave linux and asm headers, moreover the list seems to
> be sorted (ordered, please preserve the ordering).
> 

Good point, I'll correct this.

> ...
> 
>> +struct resource srmem_resource = {
>> +	.name	= "Soft Reserved mem",
>> +	.start	= 0,
>> +	.end	= -1,
>> +	.flags	= IORESOURCE_MEM,
> 
> This can use DEFINE_RES_MEM_NAMED() as well.

Yes. I went with this format since it is what is used for the other two
struct resource definitions currently in the file.

> 
>> +};
> 
> ...
> 
>> +	if (sr_res->start == res->start && sr_res->end == res->end) {
> 
> Wondering if we have a helper to exact match the resource by range...

I don't remember seeing one but will investigate.

> 
>> +		release_resource(sr_res);
>> +		free_resource(sr_res);
>> +	} else if (sr_res->start == res->start) {
>> +		WARN_ON(adjust_resource(sr_res, res->end + 1,
>> +					sr_res->end - res->end));
>> +	} else if (sr_res->end == res->end) {
>> +		WARN_ON(adjust_resource(sr_res, sr_res->start,
>> +					res->start - sr_res->start));
>> +	} else {
>> +		/*
>> +		 * Adjust existing resource to cover the resource
>> +		 * range prior to the range to be trimmed.
>> +		 */
>> +		adjust_resource(sr_res, sr_res->start,
>> +				res->start - sr_res->start);
>> +
>> +		/*
>> +		 * Add new resource to cover the resource range for
>> +		 * the range after the range to be trimmed.
>> +		 */
>> +		new_res = alloc_resource(GFP_KERNEL);
>> +		if (!new_res)
>> +			return;
>> +
>> +		*new_res = DEFINE_RES_NAMED(res->end + 1, sr_res->end - res->end,
>> +					    "Soft Reserved", sr_res->flags);
>> +		new_res->desc = IORES_DESC_SOFT_RESERVED;
>> +		insert_resource(&srmem_resource, new_res);
>> +	}
> 
> ...
> 
>> +void trim_soft_reserve_resources(const struct resource *res)
>> +{
>> +	struct resource *sr_res;
>> +
>> +	write_lock(&srmem_resource_lock);
>> +	for (sr_res = srmem_resource.child; sr_res; sr_res = sr_res->sibling) {
> 
> Can this utilise for_each_resource*()?
> Ditto for the rest of open coded for each type of loops.

This one could use for_each_resource(). There is one loop that cannot since it
moves struct resources from one list to another.

> 
>> +		if (resource_contains(sr_res, res)) {
>> +			trim_soft_reserve(sr_res, res);
>> +			break;
>> +		}
>> +	}
>> +	write_unlock(&srmem_resource_lock);
>> +}
> 
> ...
> 
>> +	cfmws_res = DEFINE_RES_MEM(cfmws->base_hpa,
>> +				   cfmws->base_hpa + cfmws->window_size);
> 
> Can be one line.
> But is this correct? The parameters are start,size, and here it seems like start,end.

Good catch. I'll fix this.

> 
> ...
> 
>> +static bool resource_overlaps_cfmws(struct resource *res)
>> +{
>> +	struct srmem_arg arg = {
>> +		.res = res,
> 
>> +		.overlaps = 0
> Keep trailing comma.

will do.

> 
>> +	};
>> +
>> +	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS, srmem_parse_cfmws, &arg);
> 
>> +	if (arg.overlaps)
>> +		return true;
>> +
>> +	return false;
> 
> 	return arg.overlaps;
> 
>> +}
> 
> ...
> 
>> +int insert_soft_reserve_resource(struct resource *res)
>> +{
>> +	if (resource_overlaps_cfmws(res)) {
>> +		pr_info("Reserving Soft Reserve %pr\n", res);
> 
> Btw, do we have pr_fmt() defined in this file?

Yes, there is a pr_fmt for kernel/resource.c

Thanks for the review.

-Nathan

> 
>> +		return insert_resource(&srmem_resource, res);
>> +	}
>> +
>> +	return insert_resource(&iomem_resource, res);
>> +}
>