Skip to contentSkip to conversation

x86/cacheinfo: Don't use cpu_llc_shared_map for !CONFIG_SMP

PATCH Applied Saurabh Sengar 4 participants 10 messages Aug 10, 2022 – Sep 29, 2022

Subsystems (2)
Stable branch x86
lore
Share & tools
lore

Saurabh Sengar

author
#0 lore raw

PATCH 1/1 x86/cacheinfo: Don't use cpu_llc_shared_map for !CONFIG_SMP

cpu_llc_shared_map is always declared and defined, but populated in arch/x86/kernel/smpboot.c which only builds for CONFIG_SMP=y. For UniProcessor builds this mask is never populated and hence invalid. Current code doesn't handle the case of AMD UniProcessor correctly, which results "shared_cpu_map" and "shared_cpu_list" files missing from sysfs entries for l3 cache. This patch fixes this issue.
This code used to work because of a another bug in 'cpumask_next', where in the CONFIG_SMP=n case the cpumask_next() ignores empty mask that results as if CPU 0 was set. This bug in 'cpumask_next' was fixed by following commit, which exposes the cpu_llc_shared_map bug.

b81dce77ced ("cpumask: Fix invalid uniprocessor mask assumption")

Fixes 2b83809a5e ("x86/cpu/amd: Derive L3 shared_cpu_map from cpu_llc_shared_mask")
Signed-off-by Saurabh Sengar <[email protected]>
---
 arch/x86/kernel/cpu/cacheinfo.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
arch/x86/kernel/cpu/cacheinfo.c +12 -2
index 66556833d7af..8753bf33fec4 100644
@@ -889,10 +889,12 @@ static int __cache_amd_cpumap_setup(unsigned int cpu, int index,
 	int i, sibling;
 
 	/*
-	 * For L3, always use the pre-calculated cpu_llc_shared_mask
-	 * to derive shared_cpu_map.
+	 * For L3, in case of SMP systems use the pre-calculated
+	 * cpu_llc_shared_mask to derive shared_cpu_map. In case
+	 * of UP simply set the only cpu in mask.
 	 */
 	if (index == 3) {
+#ifdef CONFIG_SMP
 		for_each_cpu(i, cpu_llc_shared_mask(cpu)) {
 			this_cpu_ci = get_cpu_cacheinfo(i);
 			if (!this_cpu_ci->info_list)

@@ -905,6 +907,14 @@ static int __cache_amd_cpumap_setup(unsigned int cpu, int index,
 						&this_leaf->shared_cpu_map);
 			}
 		}
+#else
+		this_cpu_ci = get_cpu_cacheinfo(cpu);
+		WARN_ON(!this_cpu_ci->info_list);
+		if (!this_cpu_ci->info_list)
+			return 0;
+		this_leaf = this_cpu_ci->info_list + index;
+		cpumask_set_cpu(cpu, &this_leaf->shared_cpu_map);
+#endif
 	} else if (boot_cpu_has(X86_FEATURE_TOPOEXT)) {
 		unsigned int apicid, nshared, first, last;
Signature · 1 line
2.25.1
Reviewed-by on v1 1/1
#1 lore raw

From: Saurabh Sengar <[email protected]> Sent: Wednesday, August 10, 2022 9:15 AM

Quoted 58 lines

cpu_llc_shared_map is always declared and defined, but populated in arch/x86/kernel/smpboot.c which only builds for CONFIG_SMP=y. For UniProcessor builds this mask is never populated and hence invalid. Current code doesn't handle the case of AMD UniProcessor correctly, which results "shared_cpu_map" and "shared_cpu_list" files missing from sysfs entries for l3 cache. This patch fixes this issue.
This code used to work because of a another bug in 'cpumask_next', where in the CONFIG_SMP=n case the cpumask_next() ignores empty mask that results as if CPU 0 was set. This bug in 'cpumask_next' was fixed by following commit, which exposes the cpu_llc_shared_map bug.

b81dce77ced ("cpumask: Fix invalid uniprocessor mask assumption")

Fixes 2b83809a5e ("x86/cpu/amd: Derive L3 shared_cpu_map from

cpu_llc_shared_mask")

Signed-off-by Saurabh Sengar <[email protected]>
---
 arch/x86/kernel/cpu/cacheinfo.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
arch/x86/kernel/cpu/cacheinfo.c +4 -2
index 66556833d7af..8753bf33fec4 100644
@@ -889,10 +889,12 @@ static int __cache_amd_cpumap_setup(unsigned int cpu, int
 index,
 	int i, sibling;
 
 	/*
-	 * For L3, always use the pre-calculated cpu_llc_shared_mask
-	 * to derive shared_cpu_map.
+	 * For L3, in case of SMP systems use the pre-calculated
+	 * cpu_llc_shared_mask to derive shared_cpu_map. In case
+	 * of UP simply set the only cpu in mask.
 	 */
 	if (index == 3) {
+#ifdef CONFIG_SMP
 		for_each_cpu(i, cpu_llc_shared_mask(cpu)) {
 			this_cpu_ci = get_cpu_cacheinfo(i);
arch/x86/kernel/cpu/cacheinfo.c +8 -1
 			if (!this_cpu_ci->info_list)

@@ -905,6 +907,14 @@ static int __cache_amd_cpumap_setup(unsigned int cpu, int
 index,
 						&this_leaf->shared_cpu_map);
 			}
 		}
+#else
+		this_cpu_ci = get_cpu_cacheinfo(cpu);
+		WARN_ON(!this_cpu_ci->info_list);
+		if (!this_cpu_ci->info_list)
+			return 0;
+		this_leaf = this_cpu_ci->info_list + index;
+		cpumask_set_cpu(cpu, &this_leaf->shared_cpu_map);
+#endif
 	} else if (boot_cpu_has(X86_FEATURE_TOPOEXT)) {
 		unsigned int apicid, nshared, first, last;
 
--
Quoted 1 line

2.25.1

Reviewed-by Michael Kelley <[email protected]>
maintainer
#2 lore raw
In reply to Saurabh Sengar
Quoted 4 lines · Saurabh Sengar

cpu_llc_shared_map is always declared and defined, but populated in arch/x86/kernel/smpboot.c which only builds for CONFIG_SMP=y. For UniProcessor builds this mask is never populated and hence invalid. Current code doesn't handle the case of AMD UniProcessor correctly,

What is "AMD UniProcessor"?

Quoted 2 lines

which results "shared_cpu_map" and "shared_cpu_list" files missing from sysfs entries for l3 cache. This patch fixes this issue.

What issue exactly?

What is the real life use case for this?

Quoted 8 lines

This code used to work because of a another bug in 'cpumask_next', where in the CONFIG_SMP=n case the cpumask_next() ignores empty mask that results as if CPU 0 was set. This bug in 'cpumask_next' was fixed by following commit, which exposes the cpu_llc_shared_map bug.

b81dce77ced ("cpumask: Fix invalid uniprocessor mask assumption")

Fixes 2b83809a5e ("x86/cpu/amd: Derive L3 shared_cpu_map from cpu_llc_shared_mask")

Add

---
[core]

        abbrev = 12

[alias]

        one = show -s --pretty='format:%h (\"%s\")'

---

to your git .config so that when you do

$ git one <commit id>

you can get the proper formatting and abbreviated sha1 for commits.

Thx.

Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
author
#3 lore raw

Thanks for review, please find my comments inline.

Quoted 7 lines · Borislav Petkov
Quoted 4 lines · Saurabh Sengar

cpu_llc_shared_map is always declared and defined, but populated in arch/x86/kernel/smpboot.c which only builds for CONFIG_SMP=y. For UniProcessor builds this mask is never populated and hence invalid. Current code doesn't handle the case of AMD UniProcessor correctly,

What is "AMD UniProcessor"?

Shall I mention here "Non-SMP AMD processor" instead ?

Quoted 7 lines
Quoted 3 lines

which results "shared_cpu_map" and "shared_cpu_list" files missing from sysfs entries for l3 cache. This patch fixes this issue.

What issue exactly?

What is the real life use case for this?

We observed that lscpu version 2.34 was causing segfault with latest kernel, which motivate us to debug this issue. Root cause being shared_cpu_map file missing. There could be more usecases where this file is getting queried for L3 cache information but at the moment I am aware of only lscpu. Do we need to add this info in commit.

Quoted 17 lines
Quoted 9 lines

This code used to work because of a another bug in 'cpumask_next', where in the CONFIG_SMP=n case the cpumask_next() ignores empty mask that results as if CPU 0 was set. This bug in 'cpumask_next' was fixed by following commit, which exposes the cpu_llc_shared_map bug.

b81dce77ced ("cpumask: Fix invalid uniprocessor mask assumption")

Fixes 2b83809a5e ("x86/cpu/amd: Derive L3 shared_cpu_map from cpu_llc_shared_mask")

Add

---
[core]

        abbrev = 12

[alias]

Quoted 8 lines
        one = show -s --pretty='format:%h (\"%s\")'

---

to your git .config so that when you do

$ git one <commit id>

you can get the proper formatting and abbreviated sha1 for commits.

Thanks for suggestion, will fix this in V2.

Quoted 8 lines

Thx.

Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
maintainer
#4 lore raw
Quoted 1 line · Saurabh Singh Sengar

Shall I mention here "Non-SMP AMD processor" instead ?

You should explain what that is.

Is that a CONFIG_SMP=n kernel which you boot on a AMD CPU?

IOW, how do I reproduce the issue you're describing below, here locally?

Send dmesg pls.

Also, what is the use case?

Why would you even build with SMP off?

Feel free to explain more verbosely what you're trying to accomplish.

Thx.

Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
author
#5 lore raw
Quoted 6 lines · Borislav Petkov
Quoted 1 line · Saurabh Singh Sengar

Shall I mention here "Non-SMP AMD processor" instead ?

You should explain what that is.

Is that a CONFIG_SMP=n kernel which you boot on a AMD CPU?

Yes, thats correct.

Quoted 2 lines

IOW, how do I reproduce the issue you're describing below, here locally?

Boot latest linux kernel on AMD CPU, having CONGIG_SMP=n.
Once booted execute lscpu (just lscpu without any argument ), this will cause segfault. Please make a note issue is observed with 2.34 version (default lscpu version with Ubuntu 20.04.4) of lscpu.

Also, if for the above test we take linux kernel code before below commit, shared_cpu_map file gets created and lscpu just works fine. b81dce77ced ("cpumask: Fix invalid uniprocessor mask assumption")

Quoted 3 lines

Send dmesg pls.

dmesg :
We get only 2 lines of dmesg for this segfault.
Step 1: dmesg -c
Step 2: lscpu
Step 3: dmesg
below is the output of step 3.

Dmesg:

[ 6951.530138] lscpu[99034]: segfault at 0 ip 00005557bc9c35ba sp 00007ffced2681c8 error 4 in lscpu[5557bc9c0000+e000]
[ 6951.530145] Code: 4d 39 c2 76 33 48 c1 e9 06 49 89 c8 4c 8d 2c cd 00 00 00 00 44 89 c9 4f 8b 04 c3 83 e1 3f 4d 0f a3 c8 73 14 4c 8b 02 4d 8b 00 <4f> 8b 04 28 49 d3 e8 41 83 e0 01 44 01 c3 48 89 c1 49 39 c4 74 38
Quoted 2 lines

Also, what is the use case?

Explained above, putting here more precise steps:
Step 1: Build linux kernel for AMD processor supporting L3 cache with CONFIG_SMP=n. Step 2: Boot this kernel.
Step 3: Once in command prompt, enter lscpu

If lspu is version 2.34 like the one I have (default with Ubuntu 20.04.4), segfault will be observed.

Quoted 1 line

Why would you even build with SMP off?

This is caught in our testing.

Quoted 10 lines

Feel free to explain more verbosely what you're trying to accomplish.

Thx.

Signature · 4 lines
Regards/Gruss,
    Boris.

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

Please let me know if any thing is unclear or need more info reproducing the scenario.

- Saurabh

maintainer
#6 lore raw
Quoted 4 lines · Saurabh Singh Sengar

Boot latest linux kernel on AMD CPU, having CONGIG_SMP=n. Once booted execute lscpu (just lscpu without any argument ), this will cause segfault. Please make a note issue is observed with 2.34 version (default lscpu version with Ubuntu 20.04.4) of lscpu.

Does that fix it?

---

arch/x86/include/asm/smp.h +15 -10
index 81a0211a372d..a73bced40e24 100644
@@ -21,16 +21,6 @@ DECLARE_PER_CPU_READ_MOSTLY(u16, cpu_llc_id);
 DECLARE_PER_CPU_READ_MOSTLY(u16, cpu_l2c_id);
 DECLARE_PER_CPU_READ_MOSTLY(int, cpu_number);
 
-static inline struct cpumask *cpu_llc_shared_mask(int cpu)
-{
-	return per_cpu(cpu_llc_shared_map, cpu);
-}
-
-static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
-{
-	return per_cpu(cpu_l2c_shared_map, cpu);
-}
-
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_cpu_to_apicid);
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(u32, x86_cpu_to_acpiid);
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_bios_cpu_apicid);

@@ -172,6 +162,16 @@ extern int safe_smp_processor_id(void);
 # define safe_smp_processor_id()	smp_processor_id()
 #endif
 
+static inline struct cpumask *cpu_llc_shared_mask(int cpu)
+{
+	return per_cpu(cpu_llc_shared_map, cpu);
+}
+
+static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
+{
+	return per_cpu(cpu_l2c_shared_map, cpu);
+}
+
 #else /* !CONFIG_SMP */
 #define wbinvd_on_cpu(cpu)     wbinvd()
 static inline int wbinvd_on_all_cpus(void)

@@ -179,6 +179,11 @@ static inline int wbinvd_on_all_cpus(void)
 	wbinvd();
 	return 0;
 }
+
+static inline struct cpumask *cpu_llc_shared_mask(int cpu)
+{
+	return (struct cpumask *)cpumask_of(0);
+}
 #endif /* CONFIG_SMP */
 
 extern unsigned disabled_cpus;
Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
author
#7 lore raw
Quoted 7 lines · Borislav Petkov
Quoted 4 lines · Saurabh Singh Sengar

Boot latest linux kernel on AMD CPU, having CONGIG_SMP=n. Once booted execute lscpu (just lscpu without any argument ), this will cause segfault. Please make a note issue is observed with 2.34 version (default lscpu version with Ubuntu 20.04.4) of lscpu.

Does that fix it?

Yes, this patch fixes this segfault.

- Saurabh

Quoted 58 lines

---

arch/x86/include/asm/smp.h +15 -10
index 81a0211a372d..a73bced40e24 100644
@@ -21,16 +21,6 @@ DECLARE_PER_CPU_READ_MOSTLY(u16, cpu_llc_id);
 DECLARE_PER_CPU_READ_MOSTLY(u16, cpu_l2c_id);
 DECLARE_PER_CPU_READ_MOSTLY(int, cpu_number);
 
-static inline struct cpumask *cpu_llc_shared_mask(int cpu)
-{
-	return per_cpu(cpu_llc_shared_map, cpu);
-}
-
-static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
-{
-	return per_cpu(cpu_l2c_shared_map, cpu);
-}
-
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_cpu_to_apicid);
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(u32, x86_cpu_to_acpiid);
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_bios_cpu_apicid);

@@ -172,6 +162,16 @@ extern int safe_smp_processor_id(void);
 # define safe_smp_processor_id()	smp_processor_id()
 #endif
 
+static inline struct cpumask *cpu_llc_shared_mask(int cpu)
+{
+	return per_cpu(cpu_llc_shared_map, cpu);
+}
+
+static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
+{
+	return per_cpu(cpu_l2c_shared_map, cpu);
+}
+
 #else /* !CONFIG_SMP */
 #define wbinvd_on_cpu(cpu)     wbinvd()
 static inline int wbinvd_on_all_cpus(void)

@@ -179,6 +179,11 @@ static inline int wbinvd_on_all_cpus(void)
 	wbinvd();
 	return 0;
 }
+
+static inline struct cpumask *cpu_llc_shared_mask(int cpu)
+{
+	return (struct cpumask *)cpumask_of(0);
+}
 #endif /* CONFIG_SMP */
 
 extern unsigned disabled_cpus;
Signature · 5 lines
-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
maintainer
#8 lore raw

PATCH 1/1 x86/cacheinfo: Add a cpu_llc_shared_mask() UP variant

Quoted 1 line · Saurabh Singh Sengar

Yes, this patch fixes this segfault.

Thx!

---
From: Borislav Petkov <[email protected]>

On a CONFIG_SMP=n kernel, the LLC shared mask is 0, which prevents __cache_amd_cpumap_setup() from doing the L3 masks setup, and more specifically from setting up the shared_cpu_map and shared_cpu_list files in sysfs, leading to lscpu from util-linux getting confused and segfaulting.

Add a cpu_llc_shared_mask() UP variant which returns a mask with a single bit set, i.e., for CPU0.

Fixes 2b83809a5e6d ("x86/cpu/amd: Derive L3 shared_cpu_map from cpu_llc_shared_mask")
Reported-by Saurabh Sengar <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
Cc <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
---
 arch/x86/include/asm/smp.h | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)
arch/x86/include/asm/smp.h +15 -10
index 81a0211a372d..a73bced40e24 100644
@@ -21,16 +21,6 @@ DECLARE_PER_CPU_READ_MOSTLY(u16, cpu_llc_id);
 DECLARE_PER_CPU_READ_MOSTLY(u16, cpu_l2c_id);
 DECLARE_PER_CPU_READ_MOSTLY(int, cpu_number);
 
-static inline struct cpumask *cpu_llc_shared_mask(int cpu)
-{
-	return per_cpu(cpu_llc_shared_map, cpu);
-}
-
-static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
-{
-	return per_cpu(cpu_l2c_shared_map, cpu);
-}
-
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_cpu_to_apicid);
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(u32, x86_cpu_to_acpiid);
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_bios_cpu_apicid);

@@ -172,6 +162,16 @@ extern int safe_smp_processor_id(void);
 # define safe_smp_processor_id()	smp_processor_id()
 #endif
 
+static inline struct cpumask *cpu_llc_shared_mask(int cpu)
+{
+	return per_cpu(cpu_llc_shared_map, cpu);
+}
+
+static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
+{
+	return per_cpu(cpu_l2c_shared_map, cpu);
+}
+
 #else /* !CONFIG_SMP */
 #define wbinvd_on_cpu(cpu)     wbinvd()
 static inline int wbinvd_on_all_cpus(void)

@@ -179,6 +179,11 @@ static inline int wbinvd_on_all_cpus(void)
 	wbinvd();
 	return 0;
 }
+
+static inline struct cpumask *cpu_llc_shared_mask(int cpu)
+{
+	return (struct cpumask *)cpumask_of(0);
+}
 #endif /* CONFIG_SMP */
 
 extern unsigned disabled_cpus;
Signature · 7 lines
2.35.1

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
merge bot Applied
#9 lore raw

[tip: x86/urgent] x86/cacheinfo: Add a cpu_llc_shared_mask() UP variant

In reply to Saurabh Sengar
The following commit has been merged into the x86/urgent branch of tip:

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     df5b035b5683d6a25f077af889fb88e09827f8bc
Gitweb:        https://git.kernel.org/tip/df5b035b5683d6a25f077af889fb88e09827f8bc
Author:        Borislav Petkov <[email protected]>
AuthorDate:    Fri, 19 Aug 2022 19:47:44 +02:00
Committer:     Borislav Petkov <[email protected]>
CommitterDate: Wed, 28 Sep 2022 18:35:37 +02:00

x86/cacheinfo: Add a cpu_llc_shared_mask() UP variant

On a CONFIG_SMP=n kernel, the LLC shared mask is 0, which prevents __cache_amd_cpumap_setup() from doing the L3 masks setup, and more specifically from setting up the shared_cpu_map and shared_cpu_list files in sysfs, leading to lscpu from util-linux getting confused and segfaulting.

Add a cpu_llc_shared_mask() UP variant which returns a mask with a single bit set, i.e., for CPU0.

Fixes 2b83809a5e6d ("x86/cpu/amd: Derive L3 shared_cpu_map from cpu_llc_shared_mask")
Reported-by Saurabh Sengar <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
Cc <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
---
 arch/x86/include/asm/smp.h | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)
arch/x86/include/asm/smp.h +15 -10
index 81a0211..a73bced 100644
@@ -21,16 +21,6 @@ DECLARE_PER_CPU_READ_MOSTLY(u16, cpu_llc_id);
 DECLARE_PER_CPU_READ_MOSTLY(u16, cpu_l2c_id);
 DECLARE_PER_CPU_READ_MOSTLY(int, cpu_number);
 
-static inline struct cpumask *cpu_llc_shared_mask(int cpu)
-{
-	return per_cpu(cpu_llc_shared_map, cpu);
-}
-
-static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
-{
-	return per_cpu(cpu_l2c_shared_map, cpu);
-}
-
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_cpu_to_apicid);
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(u32, x86_cpu_to_acpiid);
 DECLARE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_bios_cpu_apicid);

@@ -172,6 +162,16 @@ extern int safe_smp_processor_id(void);
 # define safe_smp_processor_id()	smp_processor_id()
 #endif
 
+static inline struct cpumask *cpu_llc_shared_mask(int cpu)
+{
+	return per_cpu(cpu_llc_shared_map, cpu);
+}
+
+static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
+{
+	return per_cpu(cpu_l2c_shared_map, cpu);
+}
+
 #else /* !CONFIG_SMP */
 #define wbinvd_on_cpu(cpu)     wbinvd()
 static inline int wbinvd_on_all_cpus(void)

@@ -179,6 +179,11 @@ static inline int wbinvd_on_all_cpus(void)
 	wbinvd();
 	return 0;
 }
+
+static inline struct cpumask *cpu_llc_shared_mask(int cpu)
+{
+	return (struct cpumask *)cpumask_of(0);
+}
 #endif /* CONFIG_SMP */
 
 extern unsigned disabled_cpus;