On Tue, 30 Apr 2024 11:21:54 +0200
Robert Richter <rrichter@xxxxxxx> wrote:
> For configurations that have the kconfig option NUMA_KEEP_MEMINFO
> disabled numa_fill_memblks() only returns with NUMA_NO_MEMBLK (-1).
> SRAT lookup fails then because an existing SRAT memory range cannot be
> found for a CFMWS address range. This causes the addition of a
> duplicate numa_memblk with a different node id and a subsequent page
> fault and kernel crash during boot.
>
> Fix this by making numa_fill_memblks() always available regardless of
> NUMA_KEEP_MEMINFO.
>
> The fix also removes numa_fill_memblks() from sparsemem.h using
> __weak.
>
> From Dan:
>
> """
> It just feels like numa_fill_memblks() has absolutely no business being
> defined in arch/x86/include/asm/sparsemem.h.
>
> The only use for numa_fill_memblks() is to arrange for NUMA nodes to be
> applied to memory ranges hot-onlined by the CXL driver.
>
> It belongs right next to numa_add_memblk(), and I suspect
> arch/x86/include/asm/sparsemem.h was only chosen to avoid figuring out
> what to do about the fact that linux/numa.h does not include asm/numa.h
> and that all implementations either provide numa_add_memblk() or select
> the generic implementation.
>
> So I would prefer that this do the proper fix and get
> numa_fill_memblks() completely out of the sparsemem.h path.
>
> Something like the following which boots for me.
> """
>
> Note that the issue was initially introduced with [1]. But since
> phys_to_target_node() was originally used that returned the valid node
> 0, an additional numa_memblk was not added. Though, the node id was
> wrong too, a message is seen then in the logs:
>
> kernel/numa.c: pr_info_once("Unknown target node for memory at 0x%llx, assuming node 0\n",
>
> [1] commit fd49f99c1809 ("ACPI: NUMA: Add a node and memblk for each
> CFMWS not in SRAT")
>
> Suggested-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> Link: https://lore.kernel.org/all/66271b0072317_69102944c@xxxxxxxxxxxxxxxxxxxxxxxxx.notmuch/
> Fixes: 8f1004679987 ("ACPI/NUMA: Apply SRAT proximity domain to entire CFMWS window")
> Cc: Derick Marks <derick.w.marks@xxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Alison Schofield <alison.schofield@xxxxxxxxx>
> Signed-off-by: Robert Richter <rrichter@xxxxxxx>
Whilst I'm not particularly keen on an arch specific solution for this
and the stub is effectively pointless beyond making the build work, I guess
this works well enough for now.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
I was aiming to post the ARM64 handling this cycle but it hasn't quite happened yet :(
Maybe we can look at whether there is a better level share at than
the whole function once that is done.
Jonathan
> ---
> Authorship can be changed to Dan's if he wants to but that needs his
> Signed-off-by.
> Signed-off-by: Robert Richter <rrichter@xxxxxxx>
> ---
> arch/x86/include/asm/sparsemem.h | 2 --
> arch/x86/mm/numa.c | 4 ++--
> drivers/acpi/numa/srat.c | 5 +++++
> include/linux/numa.h | 7 +------
> 4 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/include/asm/sparsemem.h b/arch/x86/include/asm/sparsemem.h
> index 1be13b2dfe8b..64df897c0ee3 100644
> --- a/arch/x86/include/asm/sparsemem.h
> +++ b/arch/x86/include/asm/sparsemem.h
> @@ -37,8 +37,6 @@ extern int phys_to_target_node(phys_addr_t start);
> #define phys_to_target_node phys_to_target_node
> extern int memory_add_physaddr_to_nid(u64 start);
> #define memory_add_physaddr_to_nid memory_add_physaddr_to_nid
> -extern int numa_fill_memblks(u64 start, u64 end);
> -#define numa_fill_memblks numa_fill_memblks
> #endif
> #endif /* __ASSEMBLY__ */
>
> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index 65e9a6e391c0..ce84ba86e69e 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -929,6 +929,8 @@ int memory_add_physaddr_to_nid(u64 start)
> }
> EXPORT_SYMBOL_GPL(memory_add_physaddr_to_nid);
>
> +#endif
> +
> static int __init cmp_memblk(const void *a, const void *b)
> {
> const struct numa_memblk *ma = *(const struct numa_memblk **)a;
> @@ -1001,5 +1003,3 @@ int __init numa_fill_memblks(u64 start, u64 end)
> }
> return 0;
> }
> -
> -#endif
> diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> index e45e64993c50..3b09fd39eeb4 100644
> --- a/drivers/acpi/numa/srat.c
> +++ b/drivers/acpi/numa/srat.c
> @@ -208,6 +208,11 @@ int __init srat_disabled(void)
> return acpi_numa < 0;
> }
>
> +__weak int __init numa_fill_memblks(u64 start, u64 end)
> +{
> + return NUMA_NO_MEMBLK;
> +}
> +
> #if defined(CONFIG_X86) || defined(CONFIG_ARM64) || defined(CONFIG_LOONGARCH)
> /*
> * Callback for SLIT parsing. pxm_to_node() returns NUMA_NO_NODE for
> diff --git a/include/linux/numa.h b/include/linux/numa.h
> index 915033a75731..1d43371fafd2 100644
> --- a/include/linux/numa.h
> +++ b/include/linux/numa.h
> @@ -36,12 +36,7 @@ int memory_add_physaddr_to_nid(u64 start);
> int phys_to_target_node(u64 start);
> #endif
>
> -#ifndef numa_fill_memblks
> -static inline int __init numa_fill_memblks(u64 start, u64 end)
> -{
> - return NUMA_NO_MEMBLK;
> -}
> -#endif
> +int numa_fill_memblks(u64 start, u64 end);
>
> #else /* !CONFIG_NUMA */
> static inline int numa_nearest_node(int node, unsigned int state)
