[PATCH v4 0/7] SRAT/CEDT fixes and updates

[PATCH v4 0/7] SRAT/CEDT fixes and updates

[Robert Richter]

Some fixes and updates for SRAT/CEDT parsing code. Patches can be
applied individually and are independent.

First patch fixes a page fault during boot. It should be marked
stable.

2nd patch reworks the code around numa_fill_memblks() (Dan's
suggestion).

Patches 3 to 5 remove architectural code no longer needed.

Patches 6 to 7 add diagnostic printouts for CEDT (can be dropped if
so).

Changelog:

v4:
 * updated SOB chains and desription (Alison, Dan)
 * added patch "x86/numa: Remove numa_fill_memblks() from sparsemem.h
   using __weak", please check note on authorship (Dan)
 * Reordered patches to move CEDT table printout as an option at the
   end (Dan)
 * split print table patch and added: "ACPI/NUMA: Add log messages for
   memory ranges found in CEDT" (Alison, Dan)

v3:
 * Rebased onto v6.9-rc1
 * Fixing x86 build error in sparsemem.h [Dan/Alison]
 * Added CEDT node info [Alison]
 * Use pr_debug() for table output [Dan]
 * Refactoring split in 3 patches [Dan]
 * Fixed performance regression introduced [kbot]
 * Fixed checkpatch issues [Dan]

Bases on v6.9-rc1.

Robert Richter (7):
  x86/numa: Fix SRAT lookup of CFMWS ranges with numa_fill_memblks()
  x86/numa: Remove numa_fill_memblks() from sparsemem.h using __weak
  ACPI/NUMA: Remove architecture dependent remainings
  ACPI/NUMA: Squash acpi_numa_slit_init() into acpi_parse_slit()
  ACPI/NUMA: Squash acpi_numa_memory_affinity_init() into
    acpi_parse_memory_affinity()
  ACPI/NUMA: Add log messages for memory ranges found in CEDT
  ACPI/NUMA: Print CXL Early Discovery Table (CEDT)

 arch/x86/include/asm/numa.h      |   1 +
 arch/x86/include/asm/sparsemem.h |   2 -
 arch/x86/mm/numa.c               |   4 +-
 drivers/acpi/numa/srat.c         | 203 +++++++++++++++++++++++--------
 include/linux/acpi.h             |   5 -
 include/linux/numa.h             |   7 --
 6 files changed, 155 insertions(+), 67 deletions(-)


base-commit: d37a823e8ac01a2f657eed7aed8ea7e515c5f147
-- 
2.39.2

[PATCH v4 1/7] x86/numa: Fix SRAT lookup of CFMWS ranges with numa_fill_memblks()

[Robert Richter]

For configurations that have the kconfig option NUMA_KEEP_MEMINFO
disabled, the SRAT lookup done with numa_fill_memblks() fails
returning NUMA_NO_MEMBLK (-1). 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.

numa_fill_memblks() is implemented and used in the init section only.
The option NUMA_KEEP_MEMINFO is only for the case when NUMA data will
be used outside of init. So fix the SRAT lookup by moving
numa_fill_memblks() out of the NUMA_KEEP_MEMINFO block to make it
always available in the init section.

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")

Fixes: 8f1004679987 ("ACPI/NUMA: Apply SRAT proximity domain to entire CFMWS window")
Cc: Derick Marks <derick.w.marks@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
---

Also note this patch is intended for stable, please tag it. The next
patch (using __weak instead) fixes the issue too, but is more
complex. So if this patch will not be used for stable it can be
dropped entirely in favour of the next.
---
 arch/x86/include/asm/sparsemem.h | 2 +-
 arch/x86/mm/numa.c               | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/sparsemem.h b/arch/x86/include/asm/sparsemem.h
index 1be13b2dfe8b..1aaa447ef24b 100644
--- a/arch/x86/include/asm/sparsemem.h
+++ b/arch/x86/include/asm/sparsemem.h
@@ -37,9 +37,9 @@ 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
+#endif
 extern int numa_fill_memblks(u64 start, u64 end);
 #define numa_fill_memblks numa_fill_memblks
-#endif
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_X86_SPARSEMEM_H */
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
-- 
2.39.2

[PATCH v4 2/7] x86/numa: Remove numa_fill_memblks() from sparsemem.h using __weak

[Robert Richter]

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.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Link: https://lore.kernel.org/all/66271b0072317_69102944c@dwillia2-xfh.jf.intel.com.notmuch/
Signed-off-by: Robert Richter <rrichter@amd.com>
---

Authorship can be changed to Dan's if he wants to but that needs his
Signed-off-by.
---
 arch/x86/include/asm/numa.h      | 1 +
 arch/x86/include/asm/sparsemem.h | 2 --
 drivers/acpi/numa/srat.c         | 5 +++++
 include/linux/numa.h             | 7 -------
 4 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/numa.h b/arch/x86/include/asm/numa.h
index ef2844d69173..12a93a3466c4 100644
--- a/arch/x86/include/asm/numa.h
+++ b/arch/x86/include/asm/numa.h
@@ -26,6 +26,7 @@ extern s16 __apicid_to_node[MAX_LOCAL_APIC];
 extern nodemask_t numa_nodes_parsed __initdata;
 
 extern int __init numa_add_memblk(int nodeid, u64 start, u64 end);
+extern int __init numa_fill_memblks(u64 start, u64 end);
 extern void __init numa_set_distance(int from, int to, int distance);
 
 static inline void set_apicid_to_node(int apicid, s16 node)
diff --git a/arch/x86/include/asm/sparsemem.h b/arch/x86/include/asm/sparsemem.h
index 1aaa447ef24b..64df897c0ee3 100644
--- a/arch/x86/include/asm/sparsemem.h
+++ b/arch/x86/include/asm/sparsemem.h
@@ -38,8 +38,6 @@ extern int phys_to_target_node(phys_addr_t start);
 extern int memory_add_physaddr_to_nid(u64 start);
 #define memory_add_physaddr_to_nid memory_add_physaddr_to_nid
 #endif
-extern int numa_fill_memblks(u64 start, u64 end);
-#define numa_fill_memblks numa_fill_memblks
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_X86_SPARSEMEM_H */
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..8485d98e554d 100644
--- a/include/linux/numa.h
+++ b/include/linux/numa.h
@@ -36,13 +36,6 @@ 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
-
 #else /* !CONFIG_NUMA */
 static inline int numa_nearest_node(int node, unsigned int state)
 {
-- 
2.39.2

[PATCH v4 3/7] ACPI/NUMA: Remove architecture dependent remainings

[Robert Richter]

With the removal of the Itanium architecture [1] the last architecture
dependent functions:

 acpi_numa_slit_init(), acpi_numa_memory_affinity_init()

were removed. Remove its remainings in the header files too and make
them static.

[1] commit cf8e8658100d ("arch: Remove Itanium (IA-64) architecture")

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/acpi/numa/srat.c | 16 ++--------------
 include/linux/acpi.h     |  5 -----
 2 files changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
index 3b09fd39eeb4..e4d53e3660fd 100644
--- a/drivers/acpi/numa/srat.c
+++ b/drivers/acpi/numa/srat.c
@@ -213,13 +213,12 @@ __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
  * I/O localities since SRAT does not list them.  I/O localities are
  * not supported at this point.
  */
-void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
+static void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
 {
 	int i, j;
 
@@ -241,11 +240,7 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
 	}
 }
 
-/*
- * Default callback for parsing of the Proximity Domain <-> Memory
- * Area mappings
- */
-int __init
+static int __init
 acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 {
 	u64 start, end;
@@ -345,13 +340,6 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
 	(*fake_pxm)++;
 	return 0;
 }
-#else
-static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
-				   void *arg, const unsigned long table_end)
-{
-	return 0;
-}
-#endif /* defined(CONFIG_X86) || defined (CONFIG_ARM64) */
 
 static int __init acpi_parse_slit(struct acpi_table_header *table)
 {
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 34829f2c517a..2c227b61a452 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -242,9 +242,6 @@ static inline bool acpi_gicc_is_usable(struct acpi_madt_generic_interrupt *gicc)
 	return gicc->flags & ACPI_MADT_ENABLED;
 }
 
-/* the following numa functions are architecture-dependent */
-void acpi_numa_slit_init (struct acpi_table_slit *slit);
-
 #if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH)
 void acpi_numa_processor_affinity_init (struct acpi_srat_cpu_affinity *pa);
 #else
@@ -267,8 +264,6 @@ static inline void
 acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa) { }
 #endif
 
-int acpi_numa_memory_affinity_init (struct acpi_srat_mem_affinity *ma);
-
 #ifndef PHYS_CPUID_INVALID
 typedef u32 phys_cpuid_t;
 #define PHYS_CPUID_INVALID (phys_cpuid_t)(-1)
-- 
2.39.2

[PATCH v4 4/7] ACPI/NUMA: Squash acpi_numa_slit_init() into acpi_parse_slit()

[Robert Richter]

After removing architectural code the helper function
acpi_numa_slit_init() is no longer needed. Squash it into
acpi_parse_slit(). No functional changes intended.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/acpi/numa/srat.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
index e4d53e3660fd..430ddcfb8312 100644
--- a/drivers/acpi/numa/srat.c
+++ b/drivers/acpi/numa/srat.c
@@ -218,10 +218,16 @@ __weak int __init numa_fill_memblks(u64 start, u64 end)
  * I/O localities since SRAT does not list them.  I/O localities are
  * not supported at this point.
  */
-static void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
+static int __init acpi_parse_slit(struct acpi_table_header *table)
 {
+	struct acpi_table_slit *slit = (struct acpi_table_slit *)table;
 	int i, j;
 
+	if (!slit_valid(slit)) {
+		pr_info("SLIT table looks invalid. Not used.\n");
+		return -EINVAL;
+	}
+
 	for (i = 0; i < slit->locality_count; i++) {
 		const int from_node = pxm_to_node(i);
 
@@ -238,6 +244,8 @@ static void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
 				slit->entry[slit->locality_count * i + j]);
 		}
 	}
+
+	return 0;
 }
 
 static int __init
@@ -341,19 +349,6 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
 	return 0;
 }
 
-static int __init acpi_parse_slit(struct acpi_table_header *table)
-{
-	struct acpi_table_slit *slit = (struct acpi_table_slit *)table;
-
-	if (!slit_valid(slit)) {
-		pr_info("SLIT table looks invalid. Not used.\n");
-		return -EINVAL;
-	}
-	acpi_numa_slit_init(slit);
-
-	return 0;
-}
-
 void __init __weak
 acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 {
-- 
2.39.2

[PATCH v4 5/7] ACPI/NUMA: Squash acpi_numa_memory_affinity_init() into acpi_parse_memory_affinity()

[Robert Richter]

After removing architectural code the helper function
acpi_numa_memory_affinity_init() is no longer needed. Squash it into
acpi_parse_memory_affinity(). No functional changes intended.

While at it, fixing checkpatch complaints in code moved.

Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202403220943.96dde419-oliver.sang@intel.com
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/acpi/numa/srat.c | 40 +++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
index 430ddcfb8312..e3f26e71637a 100644
--- a/drivers/acpi/numa/srat.c
+++ b/drivers/acpi/numa/srat.c
@@ -248,22 +248,30 @@ static int __init acpi_parse_slit(struct acpi_table_header *table)
 	return 0;
 }
 
+static int parsed_numa_memblks __initdata;
+
 static int __init
-acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
+acpi_parse_memory_affinity(union acpi_subtable_headers *header,
+			   const unsigned long table_end)
 {
+	struct acpi_srat_mem_affinity *ma;
 	u64 start, end;
 	u32 hotpluggable;
 	int node, pxm;
 
+	ma = (struct acpi_srat_mem_affinity *)header;
+
+	acpi_table_print_srat_entry(&header->common);
+
 	if (srat_disabled())
-		goto out_err;
+		return 0;
 	if (ma->header.length < sizeof(struct acpi_srat_mem_affinity)) {
 		pr_err("SRAT: Unexpected header length: %d\n",
 		       ma->header.length);
 		goto out_err_bad_srat;
 	}
 	if ((ma->flags & ACPI_SRAT_MEM_ENABLED) == 0)
-		goto out_err;
+		return 0;
 	hotpluggable = IS_ENABLED(CONFIG_MEMORY_HOTPLUG) &&
 		(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE);
 
@@ -301,11 +309,15 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 
 	max_possible_pfn = max(max_possible_pfn, PFN_UP(end - 1));
 
+	parsed_numa_memblks++;
+
 	return 0;
+
 out_err_bad_srat:
+	/* Just disable SRAT, but do not fail and ignore errors. */
 	bad_srat();
-out_err:
-	return -EINVAL;
+
+	return 0;
 }
 
 static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
@@ -438,24 +450,6 @@ acpi_parse_gi_affinity(union acpi_subtable_headers *header,
 }
 #endif /* defined(CONFIG_X86) || defined (CONFIG_ARM64) */
 
-static int __initdata parsed_numa_memblks;
-
-static int __init
-acpi_parse_memory_affinity(union acpi_subtable_headers * header,
-			   const unsigned long end)
-{
-	struct acpi_srat_mem_affinity *memory_affinity;
-
-	memory_affinity = (struct acpi_srat_mem_affinity *)header;
-
-	acpi_table_print_srat_entry(&header->common);
-
-	/* let architecture-dependent part to do it */
-	if (!acpi_numa_memory_affinity_init(memory_affinity))
-		parsed_numa_memblks++;
-	return 0;
-}
-
 static int __init acpi_parse_srat(struct acpi_table_header *table)
 {
 	struct acpi_table_srat *srat = (struct acpi_table_srat *)table;
-- 
2.39.2

[PATCH v4 6/7] ACPI/NUMA: Add log messages for memory ranges found in CEDT

[Robert Richter]

Adding a pr_info() when successfully adding a CFMWS memory range.

Suggested-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/acpi/numa/srat.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
index e3f26e71637a..c62e4636e472 100644
--- a/drivers/acpi/numa/srat.c
+++ b/drivers/acpi/numa/srat.c
@@ -338,8 +338,11 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
 	 * found for any portion of the window to cover the entire
 	 * window.
 	 */
-	if (!numa_fill_memblks(start, end))
+	if (!numa_fill_memblks(start, end)) {
+		pr_info("CEDT: memblk extended [mem %#010Lx-%#010Lx]\n",
+			(unsigned long long) start, (unsigned long long) end - 1);
 		return 0;
+	}
 
 	/* No SRAT description. Create a new node. */
 	node = acpi_map_pxm_to_node(*fake_pxm);
@@ -354,8 +357,13 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
 		pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n",
 			node, start, end);
 	}
+
 	node_set(node, numa_nodes_parsed);
 
+	pr_info("CEDT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n",
+		node, *fake_pxm,
+		(unsigned long long) start, (unsigned long long) end - 1);
+
 	/* Set the next available fake_pxm value */
 	(*fake_pxm)++;
 	return 0;
-- 
2.39.2

Re: [PATCH v4 6/7] ACPI/NUMA: Add log messages for memory ranges found in CEDT

[Dan Williams]

Robert Richter wrote:
> Adding a pr_info() when successfully adding a CFMWS memory range.
> 
> Suggested-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/acpi/numa/srat.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> index e3f26e71637a..c62e4636e472 100644
> --- a/drivers/acpi/numa/srat.c
> +++ b/drivers/acpi/numa/srat.c
> @@ -338,8 +338,11 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
>  	 * found for any portion of the window to cover the entire
>  	 * window.
>  	 */
> -	if (!numa_fill_memblks(start, end))
> +	if (!numa_fill_memblks(start, end)) {
> +		pr_info("CEDT: memblk extended [mem %#010Lx-%#010Lx]\n",
> +			(unsigned long long) start, (unsigned long long) end - 1);

This looks like pr_debug() material to me.

>  		return 0;
> +	}
>  
>  	/* No SRAT description. Create a new node. */
>  	node = acpi_map_pxm_to_node(*fake_pxm);
> @@ -354,8 +357,13 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
>  		pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n",
>  			node, start, end);
>  	}
> +
>  	node_set(node, numa_nodes_parsed);
>  
> +	pr_info("CEDT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n",
> +		node, *fake_pxm,
> +		(unsigned long long) start, (unsigned long long) end - 1);
> +

This makes sense to mirror the SRAT pr_info().

Re: [PATCH v4 6/7] ACPI/NUMA: Add log messages for memory ranges found in CEDT

[Robert Richter]

On 24.04.24 10:54:22, Dan Williams wrote:
> Robert Richter wrote:
> > Adding a pr_info() when successfully adding a CFMWS memory range.
> > 
> > Suggested-by: Alison Schofield <alison.schofield@intel.com>
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > ---
> >  drivers/acpi/numa/srat.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> > index e3f26e71637a..c62e4636e472 100644
> > --- a/drivers/acpi/numa/srat.c
> > +++ b/drivers/acpi/numa/srat.c
> > @@ -338,8 +338,11 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> >  	 * found for any portion of the window to cover the entire
> >  	 * window.
> >  	 */
> > -	if (!numa_fill_memblks(start, end))
> > +	if (!numa_fill_memblks(start, end)) {
> > +		pr_info("CEDT: memblk extended [mem %#010Lx-%#010Lx]\n",
> > +			(unsigned long long) start, (unsigned long long) end - 1);
> 
> This looks like pr_debug() material to me.

This should have the same log level as the message below and/or its
corresponding SRAT message. CEDT mem blocks wouldn't be reported
otherwise only because a smaller (overlapping) entry was registered
before. That is why I used pr_info here.

> 
> >  		return 0;
> > +	}
> >  
> >  	/* No SRAT description. Create a new node. */
> >  	node = acpi_map_pxm_to_node(*fake_pxm);
> > @@ -354,8 +357,13 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> >  		pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n",
> >  			node, start, end);
> >  	}
> > +
> >  	node_set(node, numa_nodes_parsed);
> >  
> > +	pr_info("CEDT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n",
> > +		node, *fake_pxm,
> > +		(unsigned long long) start, (unsigned long long) end - 1);
> > +
> 
> This makes sense to mirror the SRAT pr_info().

I evaluated this.

SRAT shows this:

	pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s%s\n",
		node, pxm,
		(unsigned long long) start, (unsigned long long) end - 1,
		hotpluggable ? " hotplug" : "",
		ma->flags & ACPI_SRAT_MEM_NON_VOLATILE ? " non-volatile" : "");

There is no direct mapping of SRAT memory affinity flags (acpi-6.5
spec, table 5.59) and something in the CFMWS entry (cxl-3.1, table
9-22). There is no "hotplug" flag and the "non-volatile" part would be
ambiguous, as some logic must be defined to handle the "volatile" and
"persistent" Window Restrictions. Since the CFMWS restrictions are not
used at all by the kernel my conclusion was to just dropped the flag
for the CEDT info.

Note there is a mapping defined for CDAT DSMAS and SRAT entries, see
CDAT 1.03 spec, Table 4.

-Robert

Re: [PATCH v4 6/7] ACPI/NUMA: Add log messages for memory ranges found in CEDT

[Alison Schofield]

On Thu, Apr 25, 2024 at 09:30:15AM +0200, Robert Richter wrote:
> On 24.04.24 10:54:22, Dan Williams wrote:
> > Robert Richter wrote:
> > > Adding a pr_info() when successfully adding a CFMWS memory range.
> > > 
> > > Suggested-by: Alison Schofield <alison.schofield@intel.com>
> > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > ---
> > >  drivers/acpi/numa/srat.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> > > index e3f26e71637a..c62e4636e472 100644
> > > --- a/drivers/acpi/numa/srat.c
> > > +++ b/drivers/acpi/numa/srat.c
> > > @@ -338,8 +338,11 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> > >  	 * found for any portion of the window to cover the entire
> > >  	 * window.
> > >  	 */
> > > -	if (!numa_fill_memblks(start, end))
> > > +	if (!numa_fill_memblks(start, end)) {
> > > +		pr_info("CEDT: memblk extended [mem %#010Lx-%#010Lx]\n",
> > > +			(unsigned long long) start, (unsigned long long) end - 1);
> > 
> > This looks like pr_debug() material to me.
> 
> This should have the same log level as the message below and/or its
> corresponding SRAT message. CEDT mem blocks wouldn't be reported
> otherwise only because a smaller (overlapping) entry was registered
> before. That is why I used pr_info here.

It does feel like this message belongs but maybe it should also 
mimic the SRAT define message and emit what node maps this range
if memblocks were extended.

But...seems this will emit a message for every CFMWS range, even those
where no memblk needed to be extended - ie the range was fully described
in the SRAT.

Sadly, numa_fill_memblks() return of 'success' has double meaning.
It can mean memblks were extended, or that (start, end) was found fully
described. I don't see an good place to insert the message in
numa_fill_memblks(). 

Sorry, just stirring the pot here, with no clear suggestion on how
to emit info.

> 
> > 
> > >  		return 0;
> > > +	}
> > >  
> > >  	/* No SRAT description. Create a new node. */
> > >  	node = acpi_map_pxm_to_node(*fake_pxm);
> > > @@ -354,8 +357,13 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> > >  		pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n",
> > >  			node, start, end);
> > >  	}
> > > +
> > >  	node_set(node, numa_nodes_parsed);
> > >  
> > > +	pr_info("CEDT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n",
> > > +		node, *fake_pxm,
> > > +		(unsigned long long) start, (unsigned long long) end - 1);
> > > +
> > 
> > This makes sense to mirror the SRAT pr_info().
> 
> I evaluated this.
> 

I read Dan's comment as simple acceptance. Like, yeah this one is good
because it mimics the SRAT pr_info.


> SRAT shows this:
> 
> 	pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s%s\n",
> 		node, pxm,
> 		(unsigned long long) start, (unsigned long long) end - 1,
> 		hotpluggable ? " hotplug" : "",
> 		ma->flags & ACPI_SRAT_MEM_NON_VOLATILE ? " non-volatile" : "");
> 
> There is no direct mapping of SRAT memory affinity flags (acpi-6.5
> spec, table 5.59) and something in the CFMWS entry (cxl-3.1, table
> 9-22). There is no "hotplug" flag and the "non-volatile" part would be
> ambiguous, as some logic must be defined to handle the "volatile" and
> "persistent" Window Restrictions. Since the CFMWS restrictions are not
> used at all by the kernel my conclusion was to just dropped the flag
> for the CEDT info.
> 
> Note there is a mapping defined for CDAT DSMAS and SRAT entries, see
> CDAT 1.03 spec, Table 4.
> 
> -Robert

Re: [PATCH v4 6/7] ACPI/NUMA: Add log messages for memory ranges found in CEDT

[Robert Richter]

On 25.04.24 11:56:44, Alison Schofield wrote:
> On Thu, Apr 25, 2024 at 09:30:15AM +0200, Robert Richter wrote:
> > On 24.04.24 10:54:22, Dan Williams wrote:
> > > Robert Richter wrote:
> > > > Adding a pr_info() when successfully adding a CFMWS memory range.
> > > > 
> > > > Suggested-by: Alison Schofield <alison.schofield@intel.com>
> > > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > > ---
> > > >  drivers/acpi/numa/srat.c | 10 +++++++++-
> > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> > > > index e3f26e71637a..c62e4636e472 100644
> > > > --- a/drivers/acpi/numa/srat.c
> > > > +++ b/drivers/acpi/numa/srat.c
> > > > @@ -338,8 +338,11 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> > > >  	 * found for any portion of the window to cover the entire
> > > >  	 * window.
> > > >  	 */
> > > > -	if (!numa_fill_memblks(start, end))
> > > > +	if (!numa_fill_memblks(start, end)) {
> > > > +		pr_info("CEDT: memblk extended [mem %#010Lx-%#010Lx]\n",
> > > > +			(unsigned long long) start, (unsigned long long) end - 1);
> > > 
> > > This looks like pr_debug() material to me.
> > 
> > This should have the same log level as the message below and/or its
> > corresponding SRAT message. CEDT mem blocks wouldn't be reported
> > otherwise only because a smaller (overlapping) entry was registered
> > before. That is why I used pr_info here.
> 
> It does feel like this message belongs but maybe it should also 
> mimic the SRAT define message and emit what node maps this range
> if memblocks were extended.
> 
> But...seems this will emit a message for every CFMWS range, even those
> where no memblk needed to be extended - ie the range was fully described
> in the SRAT.
> 
> Sadly, numa_fill_memblks() return of 'success' has double meaning.
> It can mean memblks were extended, or that (start, end) was found fully
> described. I don't see an good place to insert the message in
> numa_fill_memblks(). 
> 
> Sorry, just stirring the pot here, with no clear suggestion on how
> to emit info.

Ok, I have changed numa_fill_memblks() to also return if memblks have
been modified. That information is used to print the message.

> 
> > 
> > > 
> > > >  		return 0;
> > > > +	}
> > > >  
> > > >  	/* No SRAT description. Create a new node. */
> > > >  	node = acpi_map_pxm_to_node(*fake_pxm);
> > > > @@ -354,8 +357,13 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> > > >  		pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n",
> > > >  			node, start, end);
> > > >  	}
> > > > +
> > > >  	node_set(node, numa_nodes_parsed);
> > > >  
> > > > +	pr_info("CEDT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n",
> > > > +		node, *fake_pxm,
> > > > +		(unsigned long long) start, (unsigned long long) end - 1);
> > > > +
> > > 
> > > This makes sense to mirror the SRAT pr_info().
> > 
> > I evaluated this.
> > 
> 
> I read Dan's comment as simple acceptance. Like, yeah this one is good
> because it mimics the SRAT pr_info.

Ah, I misread this, thanks for clarification.

-Robert

[PATCH v4 7/7] ACPI/NUMA: Print CXL Early Discovery Table (CEDT)

[Robert Richter]

The CEDT contains similar entries as the SRAT. For diagnostic reasons
print the CEDT same style as the SRAT.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/acpi/numa/srat.c | 111 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
index c62e4636e472..a7285d23387f 100644
--- a/drivers/acpi/numa/srat.c
+++ b/drivers/acpi/numa/srat.c
@@ -320,6 +320,114 @@ acpi_parse_memory_affinity(union acpi_subtable_headers *header,
 	return 0;
 }
 
+static int __init
+__acpi_table_print_cedt_entry(union acpi_subtable_headers *__header,
+			      void *arg, const unsigned long table_end)
+{
+	struct acpi_cedt_header *header = (struct acpi_cedt_header *)__header;
+
+	switch (header->type) {
+	case ACPI_CEDT_TYPE_CHBS:
+		{
+			struct acpi_cedt_chbs *p =
+				(struct acpi_cedt_chbs *)header;
+
+			if (header->length < sizeof(struct acpi_cedt_chbs)) {
+				pr_warn("CEDT: unsupported CHBS entry: size %d\n",
+					 header->length);
+				break;
+			}
+
+			pr_debug("CEDT: CHBS (0x%llx length 0x%llx uid %lu) %s (%d)\n",
+				(unsigned long long)p->base,
+				(unsigned long long)p->length,
+				(unsigned long)p->uid,
+				(p->cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) ?
+				"cxl11" :
+				(p->cxl_version == ACPI_CEDT_CHBS_VERSION_CXL20) ?
+				"cxl20" :
+				"unsupported version",
+				p->cxl_version);
+		}
+		break;
+	case ACPI_CEDT_TYPE_CFMWS:
+		{
+			struct acpi_cedt_cfmws *p =
+				(struct acpi_cedt_cfmws *)header;
+			int eiw_to_ways[] = {1, 2, 4, 8, 16, 3, 6, 12};
+			int targets = -1;
+
+			if (header->length < sizeof(struct acpi_cedt_cfmws)) {
+				pr_warn("CEDT: unsupported CFMWS entry: size %d\n",
+					header->length);
+				break;
+			}
+
+			if (p->interleave_ways < ARRAY_SIZE(eiw_to_ways))
+				targets = eiw_to_ways[p->interleave_ways];
+			if (header->length < struct_size(
+					p, interleave_targets, targets))
+				targets = -1;
+
+			pr_debug("CEDT: CFMWS (0x%llx length 0x%llx) with %d target%s",
+				(unsigned long long)p->base_hpa,
+				(unsigned long long)p->window_size,
+				targets, targets > 1 ? "s" : "");
+			for (int i = 0; i < targets; i++)
+				pr_cont("%s%lu", i ? ", " : " (",
+					(unsigned long)p->interleave_targets[i]);
+			pr_cont("%s%s%s%s%s%s\n",
+				targets > 0 ? ")" : "",
+				(p->restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) ?
+				" type2" : "",
+				(p->restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE3) ?
+				" type3" : "",
+				(p->restrictions & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE) ?
+				" volatile" : "",
+				(p->restrictions & ACPI_CEDT_CFMWS_RESTRICT_PMEM) ?
+				" pmem" : "",
+				(p->restrictions & ACPI_CEDT_CFMWS_RESTRICT_FIXED) ?
+				" fixed" : "");
+		}
+		break;
+	case ACPI_CEDT_TYPE_CXIMS:
+		{
+			struct acpi_cedt_cxims *p =
+				(struct acpi_cedt_cxims *)header;
+
+			if (header->length < sizeof(struct acpi_cedt_cxims)) {
+				pr_warn("CEDT: unsupported CXIMS entry: size %d\n",
+					header->length);
+				break;
+			}
+
+			pr_debug("CEDT: CXIMS (hbig %u nr_xormaps %u)\n",
+				(unsigned int)p->hbig,
+				(unsigned int)p->nr_xormaps);
+		}
+		break;
+	default:
+		pr_warn("CEDT: Found unsupported entry (type = 0x%x)\n",
+			header->type);
+		break;
+	}
+
+	return 0;
+}
+
+static void __init acpi_table_print_cedt_entry(enum acpi_cedt_type id)
+{
+	acpi_table_parse_cedt(id, __acpi_table_print_cedt_entry, NULL);
+}
+
+static void __init acpi_table_print_cedt(void)
+{
+	/* Print only implemented CEDT types */
+	acpi_table_print_cedt_entry(ACPI_CEDT_TYPE_CHBS);
+	acpi_table_print_cedt_entry(ACPI_CEDT_TYPE_CFMWS);
+	acpi_table_print_cedt_entry(ACPI_CEDT_TYPE_CXIMS);
+}
+
 static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
 				   void *arg, const unsigned long table_end)
 {
@@ -516,6 +624,9 @@ int __init acpi_numa_init(void)
 	/* SLIT: System Locality Information Table */
 	acpi_table_parse(ACPI_SIG_SLIT, acpi_parse_slit);
 
+	/* CEDT: CXL Early Discovery Table */
+	acpi_table_print_cedt();
+
 	/*
 	 * CXL Fixed Memory Window Structures (CFMWS) must be parsed
 	 * after the SRAT. Create NUMA Nodes for CXL memory ranges that
-- 
2.39.2

Re: [PATCH v4 0/7] SRAT/CEDT fixes and updates

[Dan Williams]

Robert Richter wrote:
> Some fixes and updates for SRAT/CEDT parsing code. Patches can be
> applied individually and are independent.
> 
> First patch fixes a page fault during boot. It should be marked
> stable.
> 
> 2nd patch reworks the code around numa_fill_memblks() (Dan's
> suggestion).

Just squash these 2 together. The -stable maintainers continue to assert
that fixes should do the right thing by mainline mainline standards and
let the -stable backport process decide if a different change needs to
be made for older kernels. I see no benefit for tracking 2 changes for
how numa_fill_memblks() is defined.

Re: [PATCH v4 0/7] SRAT/CEDT fixes and updates

[Robert Richter]

On 24.04.24 10:46:44, Dan Williams wrote:
> Robert Richter wrote:
> > Some fixes and updates for SRAT/CEDT parsing code. Patches can be
> > applied individually and are independent.
> > 
> > First patch fixes a page fault during boot. It should be marked
> > stable.
> > 
> > 2nd patch reworks the code around numa_fill_memblks() (Dan's
> > suggestion).
> 
> Just squash these 2 together. The -stable maintainers continue to assert
> that fixes should do the right thing by mainline mainline standards and
> let the -stable backport process decide if a different change needs to
> be made for older kernels. I see no benefit for tracking 2 changes for
> how numa_fill_memblks() is defined.

Ok, will drop #1 in a v5.

Thanks,

-Robert