[PATCH v7 0/3] x86/cacheinfo: Set the number of leaves per CPU

[PATCH v7 0/3] x86/cacheinfo: Set the number of leaves per CPU

[Ricardo Neri]

Hi,

This is v7 of a patchset to fix the cache sysfs interface by setting the
number of cache leaves independently for each CPU. This version merges
patches 1 and 2 from v6 into one as Borislav suggested. It looked OK to
me to keep the Reviewed-by and Tested-by collected so far in the merged
patch as all feedback still applies and there were no code changes. I
hope reviewers are OK!

Previous versions can be found in [1], [2], [3], [4], [5], and [6].

Below is the (updated) cover letter from v6 for reference.

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. Tools that rely on
sysfs (e.g., lstopo) fail.

Patches 2 and 3 fix the described issue on Meteor Lake. Patch 1 deals
with prework in the cacheinfo base driver to fix issues uncovered while
updating cacheinfo for x86.

All the tests described in detail in [7] and [8] passed. This is the
summary:

  * /sys/devices/system/cpu/cpuX/cache is populated in Meteor Lake.
  * No inconsistencies are found in /sys/devices/system/cpu/cpuX/cache
    and the tools x86info, lstopo, and lscpu.
  * No splat is observed with and without CONFIG_PREEMPT_RT.
  * No new warnings/errors are seen the kernel log.
  * Tests done on assorted Intel and AMD client and server parts.

Changes since v6:
  * Merged patches 1 and 2 into one. (Borislav)
  * Fixed an formatting issue in allocate_cache_info(). (Borislav)

Changes since v5:
  * Reordered the arguments of set_num_cache_leaves().
  * Fixed wording on the subject of patch 2.
  * Added Reviewed-by tags from Andreas and Nikolay. Thanks!
  * Added Tested-by tags from Andreas. Thanks!

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[9]. 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/all/20240827051635.9114-1-ricardo.neri-calderon@linux.intel.com/
[6]. https://lore.kernel.org/all/20240905060036.5655-1-ricardo.neri-calderon@linux.intel.com/
[7]. https://lore.kernel.org/lkml/20230912032350.GA17008@ranerica-svr.sc.intel.com/
[8]. https://lore.kernel.org/all/20240902074140.GA4179@alberich/
[9]. https://lore.kernel.org/all/20230412185759.755408-1-rrendec@redhat.com/

Ricardo Neri (3):
  cacheinfo: Allocate memory during CPU hotplug 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        | 11 +++++---
 2 files changed, 33 insertions(+), 27 deletions(-)

-- 
2.34.1

[PATCH v7 1/3] cacheinfo: Allocate memory during CPU hotplug 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.

Moreover, 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.

While here, remove an unnecessary indentation in allocate_cache_info().

Reviewed-by: Andreas Herrmann <aherrmann@suse.de>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Reviewed-by: Radu Rendec <rrendec@redhat.com>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Tested-by: Andreas Herrmann <aherrmann@suse.de>
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+
---
Changes since v6:
 * Merged patches 1 and 2 of v6 into one. (Borislav)
 * Merged the history of patches 1 and 2 into this patch.
 * Kept the Reviewed-by and Tested-by tags from the two merged patches.
 * Fixed a formatting issue in allocate_cache_info(). (Borislav)

Changes since v5:
 * Fixed nonsensical subject (Nikolay).
 * Added Reviewed-by and Tested-by tags from Andreas. Thanks!
 * Added Reviewed-by tag from Nikolay. Thanks!

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:
 * Added Reviewed-by tag from Radu and Sudeep. Thanks!

Changes since v2:
 * Introduced this patch.

Changes since v1:
 * N/A

---
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 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 | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index 23b8cba4a2a3..5f35a76cba2a 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);
@@ -481,8 +481,7 @@ int __weak populate_cache_leaves(unsigned int cpu)
 static inline
 int allocate_cache_info(int cpu)
 {
-	per_cpu_cacheinfo(cpu) = kcalloc(cache_leaves(cpu),
-					 sizeof(struct cacheinfo), GFP_ATOMIC);
+	per_cpu_cacheinfo(cpu) = kcalloc(cache_leaves(cpu), sizeof(struct cacheinfo), GFP_ATOMIC);
 	if (!per_cpu_cacheinfo(cpu)) {
 		cache_leaves(cpu) = 0;
 		return -ENOMEM;
@@ -554,7 +553,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 v7 2/3] 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: Andreas Herrmann <aherrmann@suse.de>
Reviewed-by: Len Brown <len.brown@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Tested-by: Andreas Herrmann <aherrmann@suse.de>
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 v5:
 * Reordered the arguments of set_num_cache_leaves() for readability.
   (Nikolay)
 * Added Reviewed-by tag from Nikolay and Andreas. Thanks!
 * Added Tested-by tag from Andreas. Thanks!

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..182cacd772b8 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 cpu, unsigned int nr_leaves)
+{
+	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(cpu, find_num_cache_leaves(c));
 	} else if (c->extended_cpuid_level >= 0x80000006) {
 		if (cpuid_edx(0x80000006) & 0xf000)
-			num_cache_leaves = 4;
+			set_num_cache_leaves(cpu, 4);
 		else
-			num_cache_leaves = 3;
+			set_num_cache_leaves(cpu, 3);
 	}
 }
 
 void init_hygon_cacheinfo(struct cpuinfo_x86 *c)
 {
-	num_cache_leaves = find_num_cache_leaves(c);
+	set_num_cache_leaves(c->cpu_index, find_num_cache_leaves(c));
 }
 
 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(c->cpu_index,
+					     find_num_cache_leaves(c));
 
 		/*
 		 * 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 v7 3/3] 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: Andreas Herrmann <aherrmann@suse.de>
Reviewed-by: Len Brown <len.brown@intel.com>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Tested-by: Andreas Herrmann <aherrmann@suse.de>
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 v5:
 * Added Reviewed-by tag from Nikolay and Andreas. Thanks!

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 182cacd772b8..2a37f14cc6b1 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 v7 1/3] cacheinfo: Allocate memory during CPU hotplug if not done from the primary CPU

[Borislav Petkov]

On Fri, Sep 13, 2024 at 01:31:53AM -0700, Ricardo Neri wrote:
> 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 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.

Lemme get this straight: this NULL ptr deref will happen after your changes so
we don't really need this patch in stable and thus no Fixes: tag at all,
right?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v7 1/3] cacheinfo: Allocate memory during CPU hotplug if not done from the primary CPU

[Ricardo Neri]

On Tue, Oct 08, 2024 at 05:51:06PM +0200, Borislav Petkov wrote:
> On Fri, Sep 13, 2024 at 01:31:53AM -0700, Ricardo Neri wrote:
> > 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 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.
> 
> Lemme get this straight: this NULL ptr deref will happen after your changes

Yes. It does happen only after my changes.

> so we don't really need this patch in stable and thus no Fixes: tag at all,
> right?

IMHO, this patchset is needed in stable kernel because /sys/devices/system/cpu
cpu0/cache would be broken on Meteor Lake and processors like it.

This patch alone is not needed in stable kernels.

Thanks and BR,
Ricardo

Re: [PATCH v7 1/3] cacheinfo: Allocate memory during CPU hotplug if not done from the primary CPU

[Borislav Petkov]

On Tue, Oct 08, 2024 at 10:00:15AM -0700, Ricardo Neri wrote:
> IMHO, this patchset is needed in stable kernel because
> /sys/devices/system/cpu cpu0/cache would be broken on Meteor Lake and
> processors like it.

Is Meteor Lake and processors like it an already released product?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v7 1/3] cacheinfo: Allocate memory during CPU hotplug if not done from the primary CPU

[Ricardo Neri]

On Tue, Oct 08, 2024 at 08:41:45PM +0200, Borislav Petkov wrote:
> On Tue, Oct 08, 2024 at 10:00:15AM -0700, Ricardo Neri wrote:
> > IMHO, this patchset is needed in stable kernel because
> > /sys/devices/system/cpu cpu0/cache would be broken on Meteor Lake and
> > processors like it.
> 
> Is Meteor Lake and processors like it an already released product?

Yes, Meteor Lake was released in December 2023. It can be found in various
laptops.

Re: [PATCH v7 2/3] x86/cacheinfo: Delete global num_cache_leaves

[Borislav Petkov]

On Fri, Sep 13, 2024 at 01:31:54AM -0700, Ricardo Neri wrote:
> diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
> index 392d09c936d6..182cacd772b8 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;
> +}

There already is

#define cache_leaves(cpu)       (ci_cacheinfo(cpu)->num_leaves)

And there's also get_cpu_cacheinfo().

And now you're adding more silly wrappers. Yuck.

Can we pls use *one* of those things and work with it everywhere?

> @@ -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(c->cpu_index,
> +					     find_num_cache_leaves(c));

Ugly linebreak.

>  
>  		/*
>  		 * 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;

Why not

	if (!cache_leaves(cpu))
		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
> 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v7 2/3] x86/cacheinfo: Delete global num_cache_leaves

[Ricardo Neri]

On Tue, Oct 22, 2024 at 03:20:50PM +0200, Borislav Petkov wrote:
> On Fri, Sep 13, 2024 at 01:31:54AM -0700, Ricardo Neri wrote:
> > diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
> > index 392d09c936d6..182cacd772b8 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;
> > +}
> 
> There already is
> 
> #define cache_leaves(cpu)       (ci_cacheinfo(cpu)->num_leaves)
> 
> And there's also get_cpu_cacheinfo().
> 
> And now you're adding more silly wrappers. Yuck.
> 
> Can we pls use *one* of those things and work with it everywhere?

I agree. Another wrapper is not needed. I did not use cache_leaves() because
it was internal to drivers/base/cacheinfo.c I can convert it to a function
and expose it in include/linux/cacheinfo.h. I can rename it as
get_cacheinfo_leaves(unsigned int cpu).

Would that make sense?

> 
> > @@ -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(c->cpu_index,
> > +					     find_num_cache_leaves(c));
> 
> Ugly linebreak.

I will make it a single line.

> 
> >  
> >  		/*
> >  		 * 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;
> 
> Why not
> 
> 	if (!cache_leaves(cpu))
> 		return -ENOENT;
> 
> ?

The only caller of init_cache_level() also checks for !cache_leaves(cpu). I
saw no need to repeat the check here.

Also, I understand that the purpose of the function is to initialize
cpu_cacheinfo::num_levels, which is not used on x86. Moreover,
cpu_cacheinfo::num_levels do not depend on num_leaves.

Having said that, I see other architectures initializing both num_levels
and num_leaves in this function.

Adding this check probably makes the x86 implementation more future-proof
in case callers change their behavior.

Thanks and BR,
Ricardo

Re: [PATCH v7 2/3] x86/cacheinfo: Delete global num_cache_leaves

[Borislav Petkov]

On Tue, Oct 22, 2024 at 08:50:22PM -0700, Ricardo Neri wrote:
> I agree. Another wrapper is not needed. I did not use cache_leaves() because
> it was internal to drivers/base/cacheinfo.c I can convert it to a function
> and expose it in include/linux/cacheinfo.h. I can rename it as
> get_cacheinfo_leaves(unsigned int cpu).
> 
> Would that make sense?

I think you should use get_cpu_cacheinfo() everywhere and simply access the
struct members like ->num_leaves where you need it. No need for a bunch of
other silly one-liners.

> The only caller of init_cache_level() also checks for !cache_leaves(cpu). I
> saw no need to repeat the check here.
> 
> Also, I understand that the purpose of the function is to initialize
> cpu_cacheinfo::num_levels, which is not used on x86. Moreover,
> cpu_cacheinfo::num_levels do not depend on num_leaves.
> 
> Having said that, I see other architectures initializing both num_levels
> and num_leaves in this function.
> 
> Adding this check probably makes the x86 implementation more future-proof
> in case callers change their behavior.

But you're practically zapping its body in the next patch. So why does patch
3 even exist as a separate patch instead of being part of patch 2?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v7 2/3] x86/cacheinfo: Delete global num_cache_leaves

[Ricardo Neri]

On Fri, Nov 08, 2024 at 12:58:25PM +0100, Borislav Petkov wrote:
> On Tue, Oct 22, 2024 at 08:50:22PM -0700, Ricardo Neri wrote:
> > I agree. Another wrapper is not needed. I did not use cache_leaves() because
> > it was internal to drivers/base/cacheinfo.c I can convert it to a function
> > and expose it in include/linux/cacheinfo.h. I can rename it as
> > get_cacheinfo_leaves(unsigned int cpu).
> > 
> > Would that make sense?
> 
> I think you should use get_cpu_cacheinfo() everywhere and simply access the
> struct members like ->num_leaves where you need it. No need for a bunch of
> other silly one-liners.

Sure, I can do this.

> 
> > The only caller of init_cache_level() also checks for !cache_leaves(cpu). I
> > saw no need to repeat the check here.
> > 
> > Also, I understand that the purpose of the function is to initialize
> > cpu_cacheinfo::num_levels, which is not used on x86. Moreover,
> > cpu_cacheinfo::num_levels do not depend on num_leaves.
> > 
> > Having said that, I see other architectures initializing both num_levels
> > and num_leaves in this function.
> > 
> > Adding this check probably makes the x86 implementation more future-proof
> > in case callers change their behavior.
> 
> But you're practically zapping its body in the next patch. So why does patch
> 3 even exist as a separate patch instead of being part of patch 2?

Because patch 2 deals with cpu_cacheinfo::num_leaves whereas patch 3 deals
with cpu_cacheinfo:::num_levels.

I think I see your point: it can be argued that both patches deal with
init_cache_level(). I can merge these two patches together.

Thanks and BR,
Ricardo