On Mon, Dec 02, 2024 at 09:55:42AM -0600, Nathan Fontenot wrote:
... snip ...
> diff --git a/kernel/resource.c b/kernel/resource.c
> index a83040fde236..8fc4121a1887 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
... snip ...
> +static int srmem_parse_cfmws(union acpi_subtable_headers *hdr,
> + void *arg, const unsigned long unused)
> +{
Chiming in a little late to the party here
I don't think encoding CXL-specific terminology and functionality
directly into kernel/resource.c is wise by any measure.
The abstraction here is completely inverted, and this is probably
in line with Dan's comments.
The comments in e820.c allude to a similar issue
* Prior to inserting SOFT_RESERVED resources we want to check
* for an intersection with potential CXL resources.
This is similarly inverted - e820 doesn't know anthing about CXL
and it shouldn't have to be made aware of CXL. Mucking with
e820 is *begging* to be bitten elsewhere in parts of the system
that depend on it to be a relatively stable source of truth.
This tells me that this patch is trying to solve the wrong problem.
Your changelog alludes to supporting hotplug replace
"""
The current approach of leaving the SOFT RESERVE resource as is can
cause failure during hotplug replace of CXL devices because the
resource is not available for reuse after teardown of the CXL device.
"""
It sounds like we should be making the SR resource available for
re-use through proper teardown and cleanup of the resource tree,
rather than trying to change fundamental components like e820.
If the driver was capable of using the SOFT_RESERVED region on
initial setup, it should be capable of re-using that region.
Is the issue here that the hotplug-replaced component has a
different capacity? It it being assigned a new region entirely?
Is it exactly the same, but the resource isn't being cleaned up?
Can you provide more specifics about the exact hotplug interaction
that is happening? That might help understand the issue a bit better.
Much of this sounds like we need additional better tear-down
management and possibly additional cxl/acpi features to handle
hotplug of these devices - rather than changing resource.c.
~Gregory
> + struct acpi_cedt_cfmws *cfmws;
> + struct srmem_arg *args = arg;
> + struct resource cfmws_res;
> + struct resource *res;
> +
> + res = args->res;
> +
> + cfmws = (struct acpi_cedt_cfmws *)hdr;
> + cfmws_res = DEFINE_RES_MEM(cfmws->base_hpa,
> + cfmws->base_hpa + cfmws->window_size);
> +
> + if (resource_overlaps(&cfmws_res, res)) {
> + args->overlaps += 1;
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static bool resource_overlaps_cfmws(struct resource *res)
> +{
> + struct srmem_arg arg = {
> + .res = res,
> + .overlaps = 0
> + };
> +
> + acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS, srmem_parse_cfmws, &arg);
> +
> + if (arg.overlaps)
> + return true;
> +
> + return false;
> +}
> +
> +int insert_soft_reserve_resource(struct resource *res)
> +{
> + if (resource_overlaps_cfmws(res)) {
> + pr_info("Reserving Soft Reserve %pr\n", res);
> + return insert_resource(&srmem_resource, res);
> + }
> +
> + return insert_resource(&iomem_resource, res);
> +}
> +EXPORT_SYMBOL(insert_soft_reserve_resource);
> +
> static void __init
> __reserve_region_with_split(struct resource *root, resource_size_t start,
> resource_size_t end, const char *name)
> --
> 2.43.0
>
