[PATCH v5 0/4] x86/cacheinfo: Set the number of leaves per CPU

[PATCH v5 0/4] x86/cacheinfo: Set the number of leaves per CPU

[Ricardo Neri]

Hi,

The interface /sys/devices/system/cpu/cpuX/cache is broken (not populated)
if CPUs have different numbers of subleaves in CPUID 4. This is the case
of Intel Meteor Lake, which now is out in the world.

This is v5 of a patchset to fix the cache sysfs interface by setting the
number of cache leaves independently for each CPU.

v1, v2, v3, and v4 can be found here[1], here[2], here[3], and here[4].

All the tests described in [5] passed.

Changes since v4:
  * Combined two condition checks into one line. (Sudeep)
  * Added one more Reviewed-by tag from Sudeep. Thanks!

Changes since v3:
  * Fixed another NULL-pointer dereference when checking the validity of
    the last-level cache info.
  * Added the Reviewed-by tags from Radu and Sudeep. Thanks!
  * Rebased on v6.7-rc5.

Changes since v2:
  * This version uncovered a NULL-pointer dereference in recent changes to
    cacheinfo[6]. This dereference is observed when the system does not
    configure cacheinfo early during boot nor makes corrections later
    during CPU hotplug; as is the case in x86. Patch 1 fixes this issue.

Changes since v1:
  * Dave Hansen suggested to use the existing per-CPU ci_cpu_cacheinfo
    variable. Now the global variable num_cache_leaves became useless.
  * While here, I noticed that init_cache_level() also became useless:
    x86 does not need ci_cpu_cacheinfo::num_levels.

Thanks and BR,
Ricardo

[1]. https://lore.kernel.org/lkml/20230314231658.30169-1-ricardo.neri-calderon@linux.intel.com/
[2]. https://lore.kernel.org/all/20230424001956.21434-1-ricardo.neri-calderon@linux.intel.com/
[3]. https://lore.kernel.org/lkml/20230805012421.7002-1-ricardo.neri-calderon@linux.intel.com/
[4]. https://lore.kernel.org/all/20231212222519.12834-1-ricardo.neri-calderon@linux.intel.com/
[5]. https://lore.kernel.org/lkml/20230912032350.GA17008@ranerica-svr.sc.intel.com/
[6]. https://lore.kernel.org/all/20230412185759.755408-1-rrendec@redhat.com/

Ricardo Neri (4):
  cacheinfo: Check for null last-level cache info
  cacheinfo: Allocate memory for memory if not done from the primary CPU
  x86/cacheinfo: Delete global num_cache_leaves
  x86/cacheinfo: Clean out init_cache_level()

 arch/x86/kernel/cpu/cacheinfo.c | 49 +++++++++++++++++----------------
 drivers/base/cacheinfo.c        |  8 ++++--
 2 files changed, 32 insertions(+), 25 deletions(-)

-- 
2.34.1

[PATCH v5 1/4] cacheinfo: Check for null last-level cache info

[Ricardo Neri]

Before determining the validity of the last-level cache info, ensure that
it has been allocated. Simply checking for non-zero cache_leaves() is not
sufficient, as some architectures (e.g., Intel processors) have non-zero
cache_leaves() before allocation.

Dereferencing NULL cacheinfo can occur in update_per_cpu_data_slice_size().
This function iterates over all online CPUs. However, a CPU may have come
online recently, but its cacheinfo may not have been allocated yet.

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Cc: Andreas Herrmann <aherrmann@suse.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Chen Yu <yu.c.chen@intel.com>
Cc: Huang Ying <ying.huang@intel.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Nikolay Borisov <nik.borisov@suse.com>
Cc: Radu Rendec <rrendec@redhat.com>
Cc: Pierre Gondois <Pierre.Gondois@arm.com>
Cc: Pu Wen <puwen@hygon.cn>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Will Deacon <will@kernel.org>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: stable@vger.kernel.org # 6.3+
---
Changes since v4:
 * Combined checks for per_cpu_cacheinfo() and cache_leaves() in a single
   line. (Sudeep)
 * Added Reviewed-by tag from Sudeep. Thanks!

Changes since v3:
 * Introduced this patch.

Changes since v2:
 * N/A

Changes since v1:
 * N/A
---
The dereference of a NULL cacheinfo is not observed today because
cache_leaves(cpu) is zero until after init_cache_level() is called
(during the CPU hotplug callback). A subsequent changeset will set
the number of cache leaves earlier and the NULL-pointer dereference
will be observed.
---
 drivers/base/cacheinfo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index 23b8cba4a2a3..77f2e0f91589 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -58,7 +58,7 @@ bool last_level_cache_is_valid(unsigned int cpu)
 {
 	struct cacheinfo *llc;
 
-	if (!cache_leaves(cpu))
+	if (!cache_leaves(cpu) || !per_cpu_cacheinfo(cpu))
 		return false;
 
 	llc = per_cpu_cacheinfo_idx(cpu, cache_leaves(cpu) - 1);
-- 
2.34.1

[PATCH v5 2/4] cacheinfo: Allocate memory for memory if not done from the primary CPU

[Ricardo Neri]

Commit 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU")
adds functionality that architectures can use to optionally allocate and
build cacheinfo early during boot. Commit 6539cffa9495 ("cacheinfo: Add
arch specific early level initializer") lets secondary CPUs correct (and
reallocate memory) cacheinfo data if needed.

If the early build functionality is not used and cacheinfo does not need
correction, memory for cacheinfo is never allocated. x86 does not use the
early build functionality. Consequently, during the cacheinfo CPU hotplug
callback, last_level_cache_is_valid() attempts to dereference a NULL
pointer:

     BUG: kernel NULL pointer dereference, address: 0000000000000100
     #PF: supervisor read access in kernel mode
     #PF: error_code(0x0000) - not present page
     PGD 0 P4D 0
     Oops: 0000 [#1] PREEPMT SMP NOPTI
     CPU: 0 PID 19 Comm: cpuhp/0 Not tainted 6.4.0-rc2 #1
     RIP: 0010: last_level_cache_is_valid+0x95/0xe0a

Allocate memory for cacheinfo during the cacheinfo CPU hotplug callback if
not done earlier.

Reviewed-by: Radu Rendec <rrendec@redhat.com>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Fixes: 6539cffa9495 ("cacheinfo: Add arch specific early level initializer")
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Cc: Andreas Herrmann <aherrmann@suse.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Chen Yu <yu.c.chen@intel.com>
Cc: Huang Ying <ying.huang@intel.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Nikolay Borisov <nik.borisov@suse.com>
Cc: Radu Rendec <rrendec@redhat.com>
Cc: Pierre Gondois <Pierre.Gondois@arm.com>
Cc: Pu Wen <puwen@hygon.cn>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Will Deacon <will@kernel.org>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: stable@vger.kernel.org # 6.3+
---
The motivation for commit 5944ce092b97 was to prevent a BUG splat in
PREEMPT_RT kernels during memory allocation. This splat is not observed on
x86 because the memory allocation for cacheinfo happens in
detect_cache_attributes() from the cacheinfo CPU hotplug callback.

The dereference of a NULL pointer is not observed today because
cache_leaves(cpu) is zero until after init_cache_level() is called (also
during the CPU hotplug callback). A subsequent changeset will set the
number of cache leaves earlier and the NULL-pointer dereference will be
observed.
---
Changes since v4:
 * None

Changes since v3:
 * Added Reviewed-by tag from Radu and Sudeep. Thanks!

Changes since v2:
 * Introduced this patch.

Changes since v1:
 * N/A
---
 drivers/base/cacheinfo.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index 77f2e0f91589..0332148691f9 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -554,7 +554,11 @@ static inline int init_level_allocate_ci(unsigned int cpu)
 	 */
 	ci_cacheinfo(cpu)->early_ci_levels = false;
 
-	if (cache_leaves(cpu) <= early_leaves)
+	/*
+	 * Some architectures (e.g., x86) do not use early initialization.
+	 * Allocate memory now in such case.
+	 */
+	if (cache_leaves(cpu) <= early_leaves && per_cpu_cacheinfo(cpu))
 		return 0;
 
 	kfree(per_cpu_cacheinfo(cpu));
-- 
2.34.1

[PATCH v5 3/4] x86/cacheinfo: Delete global num_cache_leaves

[Ricardo Neri]

Linux remembers cpu_cachinfo::num_leaves per CPU, but x86 initializes all
CPUs from the same global "num_cache_leaves".

This is erroneous on systems such as Meteor Lake, where each CPU has a
distinct num_leaves value. Delete the global "num_cache_leaves" and
initialize num_leaves on each CPU.

Reviewed-by: Len Brown <len.brown@intel.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Cc: Andreas Herrmann <aherrmann@suse.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Chen Yu <yu.c.chen@intel.com>
Cc: Huang Ying <ying.huang@intel.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Nikolay Borisov <nik.borisov@suse.com>
Cc: Radu Rendec <rrendec@redhat.com>
Cc: Pierre Gondois <Pierre.Gondois@arm.com>
Cc: Pu Wen <puwen@hygon.cn>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Will Deacon <will@kernel.org>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: stable@vger.kernel.org # 6.3+
---
After this change, all CPUs will traverse CPUID leaf 0x4 when booted for
the first time. On systems with symmetric cache topologies this is
useless work.

Creating a list of processor models that have asymmetric cache topologies
was considered. The burden of maintaining such list would outweigh the
performance benefit of skipping this extra step.
---
Changes since v4:
 * None

Changes since v3:
 * Rebased on v6.7-rc5.

Changes since v2:
 * None

Changes since v1:
 * Do not make num_cache_leaves a per-CPU variable. Instead, reuse the
   existing per-CPU ci_cpu_cacheinfo variable. (Dave Hansen)
---
 arch/x86/kernel/cpu/cacheinfo.c | 44 +++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index 392d09c936d6..b5e216677a46 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -178,7 +178,16 @@ struct _cpuid4_info_regs {
 	struct amd_northbridge *nb;
 };
 
-static unsigned short num_cache_leaves;
+static inline unsigned int get_num_cache_leaves(unsigned int cpu)
+{
+	return get_cpu_cacheinfo(cpu)->num_leaves;
+}
+
+static inline void
+set_num_cache_leaves(unsigned int nr_leaves, unsigned int cpu)
+{
+	get_cpu_cacheinfo(cpu)->num_leaves = nr_leaves;
+}
 
 /* AMD doesn't have CPUID4. Emulate it here to report the same
    information to the user.  This makes some assumptions about the machine:
@@ -718,19 +727,21 @@ void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c)
 void init_amd_cacheinfo(struct cpuinfo_x86 *c)
 {
 
+	unsigned int cpu = c->cpu_index;
+
 	if (boot_cpu_has(X86_FEATURE_TOPOEXT)) {
-		num_cache_leaves = find_num_cache_leaves(c);
+		set_num_cache_leaves(find_num_cache_leaves(c), cpu);
 	} else if (c->extended_cpuid_level >= 0x80000006) {
 		if (cpuid_edx(0x80000006) & 0xf000)
-			num_cache_leaves = 4;
+			set_num_cache_leaves(4, cpu);
 		else
-			num_cache_leaves = 3;
+			set_num_cache_leaves(3, cpu);
 	}
 }
 
 void init_hygon_cacheinfo(struct cpuinfo_x86 *c)
 {
-	num_cache_leaves = find_num_cache_leaves(c);
+	set_num_cache_leaves(find_num_cache_leaves(c), c->cpu_index);
 }
 
 void init_intel_cacheinfo(struct cpuinfo_x86 *c)
@@ -742,19 +753,19 @@ void init_intel_cacheinfo(struct cpuinfo_x86 *c)
 	unsigned int l2_id = 0, l3_id = 0, num_threads_sharing, index_msb;
 
 	if (c->cpuid_level > 3) {
-		static int is_initialized;
-
-		if (is_initialized == 0) {
-			/* Init num_cache_leaves from boot CPU */
-			num_cache_leaves = find_num_cache_leaves(c);
-			is_initialized++;
-		}
+		/*
+		 * There should be at least one leaf. A non-zero value means
+		 * that the number of leaves has been initialized.
+		 */
+		if (!get_num_cache_leaves(c->cpu_index))
+			set_num_cache_leaves(find_num_cache_leaves(c),
+					     c->cpu_index);
 
 		/*
 		 * Whenever possible use cpuid(4), deterministic cache
 		 * parameters cpuid leaf to find the cache details
 		 */
-		for (i = 0; i < num_cache_leaves; i++) {
+		for (i = 0; i < get_num_cache_leaves(c->cpu_index); i++) {
 			struct _cpuid4_info_regs this_leaf = {};
 			int retval;
 
@@ -790,14 +801,14 @@ void init_intel_cacheinfo(struct cpuinfo_x86 *c)
 	 * Don't use cpuid2 if cpuid4 is supported. For P4, we use cpuid2 for
 	 * trace cache
 	 */
-	if ((num_cache_leaves == 0 || c->x86 == 15) && c->cpuid_level > 1) {
+	if ((!get_num_cache_leaves(c->cpu_index) || c->x86 == 15) && c->cpuid_level > 1) {
 		/* supports eax=2  call */
 		int j, n;
 		unsigned int regs[4];
 		unsigned char *dp = (unsigned char *)regs;
 		int only_trace = 0;
 
-		if (num_cache_leaves != 0 && c->x86 == 15)
+		if (get_num_cache_leaves(c->cpu_index) && c->x86 == 15)
 			only_trace = 1;
 
 		/* Number of times to iterate */
@@ -993,12 +1004,9 @@ int init_cache_level(unsigned int cpu)
 {
 	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
 
-	if (!num_cache_leaves)
-		return -ENOENT;
 	if (!this_cpu_ci)
 		return -EINVAL;
 	this_cpu_ci->num_levels = 3;
-	this_cpu_ci->num_leaves = num_cache_leaves;
 	return 0;
 }
 
-- 
2.34.1

[PATCH v5 4/4] x86/cacheinfo: Clean out init_cache_level()

[Ricardo Neri]

init_cache_level() no longer has a purpose on x86. It no longer needs to
set num_leaves, and it never had to set num_levels, which was unnecessary
on x86.

Replace it with "return 0" simply to override the weak function, which
would return an error.

Reviewed-by: Len Brown <len.brown@intel.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
Cc: Andreas Herrmann <aherrmann@suse.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Chen Yu <yu.c.chen@intel.com>
CC: Huang Ying <ying.huang@intel.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Nikolay Borisov <nik.borisov@suse.com>
Cc: Radu Rendec <rrendec@redhat.com>
Cc: Pierre Gondois <Pierre.Gondois@arm.com>
Cc: Pu Wen <puwen@hygon.cn>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Will Deacon <will@kernel.org>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: stable@vger.kernel.org # 6.3+
---
Changes since v4:
 * None

Changes since v3:
 * Rebased on v6.7-rc5.

Changes since v2:
 * None

Changes since v1:
 * Introduced this patch.
---
 arch/x86/kernel/cpu/cacheinfo.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index b5e216677a46..d7375328bc1f 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -1002,11 +1002,6 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,
 
 int init_cache_level(unsigned int cpu)
 {
-	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
-
-	if (!this_cpu_ci)
-		return -EINVAL;
-	this_cpu_ci->num_levels = 3;
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH v5 3/4] x86/cacheinfo: Delete global num_cache_leaves

[Nikolay Borisov]

On 27.08.24 г. 8:16 ч., Ricardo Neri wrote:
> Linux remembers cpu_cachinfo::num_leaves per CPU, but x86 initializes all
> CPUs from the same global "num_cache_leaves".
> 
> This is erroneous on systems such as Meteor Lake, where each CPU has a
> distinct num_leaves value. Delete the global "num_cache_leaves" and
> initialize num_leaves on each CPU.
> 
> Reviewed-by: Len Brown <len.brown@intel.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Cc: Andreas Herrmann <aherrmann@suse.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Chen Yu <yu.c.chen@intel.com>
> Cc: Huang Ying <ying.huang@intel.com>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Nikolay Borisov <nik.borisov@suse.com>
> Cc: Radu Rendec <rrendec@redhat.com>
> Cc: Pierre Gondois <Pierre.Gondois@arm.com>
> Cc: Pu Wen <puwen@hygon.cn>
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: stable@vger.kernel.org # 6.3+
> ---
> After this change, all CPUs will traverse CPUID leaf 0x4 when booted for
> the first time. On systems with symmetric cache topologies this is
> useless work.
> 
> Creating a list of processor models that have asymmetric cache topologies
> was considered. The burden of maintaining such list would outweigh the
> performance benefit of skipping this extra step.
> ---
> Changes since v4:
>   * None
> 
> Changes since v3:
>   * Rebased on v6.7-rc5.
> 
> Changes since v2:
>   * None
> 
> Changes since v1:
>   * Do not make num_cache_leaves a per-CPU variable. Instead, reuse the
>     existing per-CPU ci_cpu_cacheinfo variable. (Dave Hansen)
> ---


Overall LGTM, one minor nit below which is not a deal breaker.


Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>

>   arch/x86/kernel/cpu/cacheinfo.c | 44 +++++++++++++++++++--------------
>   1 file changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
> index 392d09c936d6..b5e216677a46 100644
> --- a/arch/x86/kernel/cpu/cacheinfo.c
> +++ b/arch/x86/kernel/cpu/cacheinfo.c
> @@ -178,7 +178,16 @@ struct _cpuid4_info_regs {
>   	struct amd_northbridge *nb;
>   };
>   
> -static unsigned short num_cache_leaves;
> +static inline unsigned int get_num_cache_leaves(unsigned int cpu)
> +{
> +	return get_cpu_cacheinfo(cpu)->num_leaves;
> +}
> +
> +static inline void
> +set_num_cache_leaves(unsigned int nr_leaves, unsigned int cpu)
> +{

nit: I think it's more natural to have the cpu parameter come first.
> +	get_cpu_cacheinfo(cpu)->num_leaves = nr_leaves;
> +}
>   
>   /* AMD doesn't have CPUID4. Emulate it here to report the same
>      information to the user.  This makes some assumptions about the machine:


<snip>

Re: [PATCH v5 2/4] cacheinfo: Allocate memory for memory if not done from the primary CPU

[Nikolay Borisov]

On 27.08.24 г. 8:16 ч., Ricardo Neri wrote:
> Commit 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU")
> adds functionality that architectures can use to optionally allocate and
> build cacheinfo early during boot. Commit 6539cffa9495 ("cacheinfo: Add
> arch specific early level initializer") lets secondary CPUs correct (and
> reallocate memory) cacheinfo data if needed.
> 
> If the early build functionality is not used and cacheinfo does not need
> correction, memory for cacheinfo is never allocated. x86 does not use the
> early build functionality. Consequently, during the cacheinfo CPU hotplug
> callback, last_level_cache_is_valid() attempts to dereference a NULL
> pointer:
> 
>       BUG: kernel NULL pointer dereference, address: 0000000000000100
>       #PF: supervisor read access in kernel mode
>       #PF: error_code(0x0000) - not present page
>       PGD 0 P4D 0
>       Oops: 0000 [#1] PREEPMT SMP NOPTI
>       CPU: 0 PID 19 Comm: cpuhp/0 Not tainted 6.4.0-rc2 #1
>       RIP: 0010: last_level_cache_is_valid+0x95/0xe0a
> 
> Allocate memory for cacheinfo during the cacheinfo CPU hotplug callback if
> not done earlier.

I assume instead of duplicating "memory" in the subject you meant 
"cacheinfo" ?

Otherwise LGTM:

Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>

> 
> Reviewed-by: Radu Rendec <rrendec@redhat.com>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> Fixes: 6539cffa9495 ("cacheinfo: Add arch specific early level initializer")
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Cc: Andreas Herrmann <aherrmann@suse.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Chen Yu <yu.c.chen@intel.com>
> Cc: Huang Ying <ying.huang@intel.com>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Nikolay Borisov <nik.borisov@suse.com>
> Cc: Radu Rendec <rrendec@redhat.com>
> Cc: Pierre Gondois <Pierre.Gondois@arm.com>
> Cc: Pu Wen <puwen@hygon.cn>
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: stable@vger.kernel.org # 6.3+
> ---
> The motivation for commit 5944ce092b97 was to prevent a BUG splat in
> PREEMPT_RT kernels during memory allocation. This splat is not observed on
> x86 because the memory allocation for cacheinfo happens in
> detect_cache_attributes() from the cacheinfo CPU hotplug callback.
> 
> The dereference of a NULL pointer is not observed today because
> cache_leaves(cpu) is zero until after init_cache_level() is called (also
> during the CPU hotplug callback). A subsequent changeset will set the
> number of cache leaves earlier and the NULL-pointer dereference will be
> observed.
> ---
> Changes since v4:
>   * None
> 
> Changes since v3:
>   * Added Reviewed-by tag from Radu and Sudeep. Thanks!
> 
> Changes since v2:
>   * Introduced this patch.
> 
> Changes since v1:
>   * N/A
> ---
>   drivers/base/cacheinfo.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index 77f2e0f91589..0332148691f9 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -554,7 +554,11 @@ static inline int init_level_allocate_ci(unsigned int cpu)
>   	 */
>   	ci_cacheinfo(cpu)->early_ci_levels = false;
>   
> -	if (cache_leaves(cpu) <= early_leaves)
> +	/*
> +	 * Some architectures (e.g., x86) do not use early initialization.
> +	 * Allocate memory now in such case.
> +	 */
> +	if (cache_leaves(cpu) <= early_leaves && per_cpu_cacheinfo(cpu))
>   		return 0;
>   
>   	kfree(per_cpu_cacheinfo(cpu));

Re: [PATCH v5 2/4] cacheinfo: Allocate memory for memory if not done from the primary CPU

[Sudeep Holla]

On Wed, Aug 28, 2024 at 04:16:00PM +0300, Nikolay Borisov wrote:
> 
> On 27.08.24 г. 8:16 ч., Ricardo Neri wrote:
> > Commit 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU")
> > adds functionality that architectures can use to optionally allocate and
> > build cacheinfo early during boot. Commit 6539cffa9495 ("cacheinfo: Add
> > arch specific early level initializer") lets secondary CPUs correct (and
> > reallocate memory) cacheinfo data if needed.
> > 
> > If the early build functionality is not used and cacheinfo does not need
> > correction, memory for cacheinfo is never allocated. x86 does not use the
> > early build functionality. Consequently, during the cacheinfo CPU hotplug
> > callback, last_level_cache_is_valid() attempts to dereference a NULL
> > pointer:
> > 
> >       BUG: kernel NULL pointer dereference, address: 0000000000000100
> >       #PF: supervisor read access in kernel mode
> >       #PF: error_code(0x0000) - not present page
> >       PGD 0 P4D 0
> >       Oops: 0000 [#1] PREEPMT SMP NOPTI
> >       CPU: 0 PID 19 Comm: cpuhp/0 Not tainted 6.4.0-rc2 #1
> >       RIP: 0010: last_level_cache_is_valid+0x95/0xe0a
> > 
> > Allocate memory for cacheinfo during the cacheinfo CPU hotplug callback if
> > not done earlier.
> 
> I assume instead of duplicating "memory" in the subject you meant
> "cacheinfo" ?
>

Good point, +1 for the $subject change. I clearly missed to notice that.

-- 
Regards,
Sudeep

Re: [PATCH v5 4/4] x86/cacheinfo: Clean out init_cache_level()

[Nikolay Borisov]

On 27.08.24 г. 8:16 ч., Ricardo Neri wrote:
> init_cache_level() no longer has a purpose on x86. It no longer needs to
> set num_leaves, and it never had to set num_levels, which was unnecessary
> on x86.
> 
> Replace it with "return 0" simply to override the weak function, which
> would return an error.
> 
> Reviewed-by: Len Brown <len.brown@intel.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>

<snip>

Re: [PATCH v5 3/4] x86/cacheinfo: Delete global num_cache_leaves

[Ricardo Neri]

On Wed, Aug 28, 2024 at 03:57:05PM +0300, Nikolay Borisov wrote:
> 
> 
> On 27.08.24 г. 8:16 ч., Ricardo Neri wrote:
> > Linux remembers cpu_cachinfo::num_leaves per CPU, but x86 initializes all
> > CPUs from the same global "num_cache_leaves".
> > 
> > This is erroneous on systems such as Meteor Lake, where each CPU has a
> > distinct num_leaves value. Delete the global "num_cache_leaves" and
> > initialize num_leaves on each CPU.
> > 
> > Reviewed-by: Len Brown <len.brown@intel.com>
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > ---
> > Cc: Andreas Herrmann <aherrmann@suse.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Chen Yu <yu.c.chen@intel.com>
> > Cc: Huang Ying <ying.huang@intel.com>
> > Cc: Len Brown <len.brown@intel.com>
> > Cc: Nikolay Borisov <nik.borisov@suse.com>
> > Cc: Radu Rendec <rrendec@redhat.com>
> > Cc: Pierre Gondois <Pierre.Gondois@arm.com>
> > Cc: Pu Wen <puwen@hygon.cn>
> > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Zhang Rui <rui.zhang@intel.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: stable@vger.kernel.org # 6.3+
> > ---
> > After this change, all CPUs will traverse CPUID leaf 0x4 when booted for
> > the first time. On systems with symmetric cache topologies this is
> > useless work.
> > 
> > Creating a list of processor models that have asymmetric cache topologies
> > was considered. The burden of maintaining such list would outweigh the
> > performance benefit of skipping this extra step.
> > ---
> > Changes since v4:
> >   * None
> > 
> > Changes since v3:
> >   * Rebased on v6.7-rc5.
> > 
> > Changes since v2:
> >   * None
> > 
> > Changes since v1:
> >   * Do not make num_cache_leaves a per-CPU variable. Instead, reuse the
> >     existing per-CPU ci_cpu_cacheinfo variable. (Dave Hansen)
> > ---
> 
> 
> Overall LGTM, one minor nit below which is not a deal breaker.
> 
> 
> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>

Thank you!

> 
> >   arch/x86/kernel/cpu/cacheinfo.c | 44 +++++++++++++++++++--------------
> >   1 file changed, 26 insertions(+), 18 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
> > index 392d09c936d6..b5e216677a46 100644
> > --- a/arch/x86/kernel/cpu/cacheinfo.c
> > +++ b/arch/x86/kernel/cpu/cacheinfo.c
> > @@ -178,7 +178,16 @@ struct _cpuid4_info_regs {
> >   	struct amd_northbridge *nb;
> >   };
> > -static unsigned short num_cache_leaves;
> > +static inline unsigned int get_num_cache_leaves(unsigned int cpu)
> > +{
> > +	return get_cpu_cacheinfo(cpu)->num_leaves;
> > +}
> > +
> > +static inline void
> > +set_num_cache_leaves(unsigned int nr_leaves, unsigned int cpu)
> > +{
> 
> nit: I think it's more natural to have the cpu parameter come first.

Sure! I will wait a few days for more feedback. Then I will post an
updated version with this change.

Best,
Ricardo

Re: [PATCH v5 2/4] cacheinfo: Allocate memory for memory if not done from the primary CPU

[Ricardo Neri]

On Wed, Aug 28, 2024 at 02:57:19PM +0100, Sudeep Holla wrote:
> On Wed, Aug 28, 2024 at 04:16:00PM +0300, Nikolay Borisov wrote:
> > 
> > On 27.08.24 г. 8:16 ч., Ricardo Neri wrote:
> > > Commit 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU")
> > > adds functionality that architectures can use to optionally allocate and
> > > build cacheinfo early during boot. Commit 6539cffa9495 ("cacheinfo: Add
> > > arch specific early level initializer") lets secondary CPUs correct (and
> > > reallocate memory) cacheinfo data if needed.
> > > 
> > > If the early build functionality is not used and cacheinfo does not need
> > > correction, memory for cacheinfo is never allocated. x86 does not use the
> > > early build functionality. Consequently, during the cacheinfo CPU hotplug
> > > callback, last_level_cache_is_valid() attempts to dereference a NULL
> > > pointer:
> > > 
> > >       BUG: kernel NULL pointer dereference, address: 0000000000000100
> > >       #PF: supervisor read access in kernel mode
> > >       #PF: error_code(0x0000) - not present page
> > >       PGD 0 P4D 0
> > >       Oops: 0000 [#1] PREEPMT SMP NOPTI
> > >       CPU: 0 PID 19 Comm: cpuhp/0 Not tainted 6.4.0-rc2 #1
> > >       RIP: 0010: last_level_cache_is_valid+0x95/0xe0a
> > > 
> > > Allocate memory for cacheinfo during the cacheinfo CPU hotplug callback if
> > > not done earlier.
> > 
> > I assume instead of duplicating "memory" in the subject you meant
> > "cacheinfo" ?
> >
> 
> Good point, +1 for the $subject change. I clearly missed to notice that.

Ah! Yes, it should read cacheinfo: Allocate memory for cacheinfo if not done from the primary CPU.

Thanks for spotting the error. I will update the subject in the next version.

Thanks and BR,
Ricardo

Re: [PATCH v5 4/4] x86/cacheinfo: Clean out init_cache_level()

[Ricardo Neri]

On Wed, Aug 28, 2024 at 05:01:38PM +0300, Nikolay Borisov wrote:
> 
> 
> On 27.08.24 г. 8:16 ч., Ricardo Neri wrote:
> > init_cache_level() no longer has a purpose on x86. It no longer needs to
> > set num_leaves, and it never had to set num_levels, which was unnecessary
> > on x86.
> > 
> > Replace it with "return 0" simply to override the weak function, which
> > would return an error.
> > 
> > Reviewed-by: Len Brown <len.brown@intel.com>
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> 
> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>

Thank you!

Re: [PATCH v5 1/4] cacheinfo: Check for null last-level cache info

[Andreas Herrmann]

On Mon, Aug 26, 2024 at 10:16:32PM -0700, Ricardo Neri wrote:
> Before determining the validity of the last-level cache info, ensure that
> it has been allocated. Simply checking for non-zero cache_leaves() is not
> sufficient, as some architectures (e.g., Intel processors) have non-zero
> cache_leaves() before allocation.
> 
> Dereferencing NULL cacheinfo can occur in update_per_cpu_data_slice_size().
> This function iterates over all online CPUs. However, a CPU may have come
> online recently, but its cacheinfo may not have been allocated yet.
> 
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Reviewed-by: Andreas Herrmann <aherrmann@suse.de>
Tested-by: Andreas Herrmann <aherrmann@suse.de>

Test was done with a system equipped with AMD Phenom II X6 1055T and
test kernels based on v6.11-rc5-176-g20371ba12063.


Thanks,
Andreas

Re: [PATCH v5 2/4] cacheinfo: Allocate memory for memory if not done from the primary CPU

[Andreas Herrmann]

On Mon, Aug 26, 2024 at 10:16:33PM -0700, Ricardo Neri wrote:
> Commit 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU")
> adds functionality that architectures can use to optionally allocate and
> build cacheinfo early during boot. Commit 6539cffa9495 ("cacheinfo: Add
> arch specific early level initializer") lets secondary CPUs correct (and
> reallocate memory) cacheinfo data if needed.
> 
> If the early build functionality is not used and cacheinfo does not need
> correction, memory for cacheinfo is never allocated. x86 does not use the
> early build functionality. Consequently, during the cacheinfo CPU hotplug
> callback, last_level_cache_is_valid() attempts to dereference a NULL
> pointer:
> 
>      BUG: kernel NULL pointer dereference, address: 0000000000000100
>      #PF: supervisor read access in kernel mode
>      #PF: error_code(0x0000) - not present page
>      PGD 0 P4D 0
>      Oops: 0000 [#1] PREEPMT SMP NOPTI
>      CPU: 0 PID 19 Comm: cpuhp/0 Not tainted 6.4.0-rc2 #1
>      RIP: 0010: last_level_cache_is_valid+0x95/0xe0a
> 
> Allocate memory for cacheinfo during the cacheinfo CPU hotplug callback if
> not done earlier.
> 
> Reviewed-by: Radu Rendec <rrendec@redhat.com>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> Fixes: 6539cffa9495 ("cacheinfo: Add arch specific early level initializer")
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Reviewed-by: Andreas Herrmann <aherrmann@suse.de>
Tested-by: Andreas Herrmann <aherrmann@suse.de>

Test was done with a system equipped with AMD Phenom II X6 1055T and
test kernels based on v6.11-rc5-176-g20371ba12063.


Thanks,
Andreas

Re: [PATCH v5 3/4] x86/cacheinfo: Delete global num_cache_leaves

[Andreas Herrmann]

On Mon, Aug 26, 2024 at 10:16:34PM -0700, Ricardo Neri wrote:
> Linux remembers cpu_cachinfo::num_leaves per CPU, but x86 initializes all
> CPUs from the same global "num_cache_leaves".
> 
> This is erroneous on systems such as Meteor Lake, where each CPU has a
> distinct num_leaves value. Delete the global "num_cache_leaves" and
> initialize num_leaves on each CPU.
> 
> Reviewed-by: Len Brown <len.brown@intel.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Reviewed-by: Andreas Herrmann <aherrmann@suse.de>
Tested-by: Andreas Herrmann <aherrmann@suse.de>

Test was done with a system equipped with AMD Phenom II X6 1055T and
test kernels based on v6.11-rc5-176-g20371ba12063.


Thanks,
Andreas

Re: [PATCH v5 4/4] x86/cacheinfo: Clean out init_cache_level()

[Andreas Herrmann]

On Mon, Aug 26, 2024 at 10:16:35PM -0700, Ricardo Neri wrote:
> init_cache_level() no longer has a purpose on x86. It no longer needs to
> set num_leaves, and it never had to set num_levels, which was unnecessary
> on x86.
> 
> Replace it with "return 0" simply to override the weak function, which
> would return an error.
> 
> Reviewed-by: Len Brown <len.brown@intel.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Reviewed-by: Andreas Herrmann <aherrmann@suse.de>
Tested-by: Andreas Herrmann <aherrmann@suse.de>

Test was done with a system equipped with AMD Phenom II X6 1055T and
test kernels based on v6.11-rc5-176-g20371ba12063.


Thanks,
Andreas

Re: [PATCH v5 4/4] x86/cacheinfo: Clean out init_cache_level()

[Andreas Herrmann]

On Sun, Sep 01, 2024 at 08:09:13PM +0200, Andreas Herrmann wrote:
> On Mon, Aug 26, 2024 at 10:16:35PM -0700, Ricardo Neri wrote:
> > init_cache_level() no longer has a purpose on x86. It no longer needs to
> > set num_leaves, and it never had to set num_levels, which was unnecessary
> > on x86.
> > 
> > Replace it with "return 0" simply to override the weak function, which
> > would return an error.
> > 
> > Reviewed-by: Len Brown <len.brown@intel.com>
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> 
> Reviewed-by: Andreas Herrmann <aherrmann@suse.de>
> Tested-by: Andreas Herrmann <aherrmann@suse.de>
> 
> Test was done with a system equipped with AMD Phenom II X6 1055T and
> test kernels based on v6.11-rc5-176-g20371ba12063.

FYI, the test consisted of booting the mainline kernel w/o and w/ your
patches, checking for potential new errors/warnings in kernel log and
checking for changes or incosistencies in information of cache
characteristics as reported in sysfs, and by some tools (lscpu,
lstopo, x86info) -- e.g. x86info does not use sysfs to gather cache
information for CPUs.


Regards,
Andreas

Re: [PATCH v5 4/4] x86/cacheinfo: Clean out init_cache_level()

[Ricardo Neri]

On Mon, Sep 02, 2024 at 09:41:40AM +0200, Andreas Herrmann wrote:
> On Sun, Sep 01, 2024 at 08:09:13PM +0200, Andreas Herrmann wrote:
> > On Mon, Aug 26, 2024 at 10:16:35PM -0700, Ricardo Neri wrote:
> > > init_cache_level() no longer has a purpose on x86. It no longer needs to
> > > set num_leaves, and it never had to set num_levels, which was unnecessary
> > > on x86.
> > > 
> > > Replace it with "return 0" simply to override the weak function, which
> > > would return an error.
> > > 
> > > Reviewed-by: Len Brown <len.brown@intel.com>
> > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > 
> > Reviewed-by: Andreas Herrmann <aherrmann@suse.de>
> > Tested-by: Andreas Herrmann <aherrmann@suse.de>
> > 
> > Test was done with a system equipped with AMD Phenom II X6 1055T and
> > test kernels based on v6.11-rc5-176-g20371ba12063.
> 
> FYI, the test consisted of booting the mainline kernel w/o and w/ your
> patches, checking for potential new errors/warnings in kernel log and
> checking for changes or incosistencies in information of cache
> characteristics as reported in sysfs, and by some tools (lscpu,
> lstopo, x86info) -- e.g. x86info does not use sysfs to gather cache
> information for CPUs.

Thank you very much for you review and testing!

I will post a new version with the feedback from Nikolay and Sudeep and
your tags.

BR,
Ricardo