x86/smp: Fix __max_logical_packages value setup

Frank reported kernel panic when he disabled several cores in BIOS via following option:

Core Disable Bitmap(Hex) [0]

with number 0xFFE, which leaves 16 CPUs in system (out of 48).

The reason for the panic is wrong value of __max_logical_packages, which lets logical_package_map uninitialized and the uncore code relying on this map being properly initialized (maybe we should add some safety checks there as well).

The __max_logical_packages is computed as:

DIV_ROUND_UP(total_cpus, ncpus);
- ncpus being number of cores

With above BIOS setup we get total_cpus == 16 which set __max_logical_packages to 2 (ncpus is 12).

Once topology_update_package_map processes CPU with logical pkg over 2 we display above messages and fail to initialize the physical_to_logical_pkg map, which makes the uncore code crash.

The fix is to set __max_logical_packages directly to total_cpus, which should be the maximum possible logical ID of the pkg in any case.

Reported-by Frank Ramsay <[email protected]>

Signed-off-by Jiri Olsa <[email protected]>

---

arch/x86/kernel/smpboot.c +0 -0

index 157bf0957219..484f7d357c77 100644

On Wed, Aug 03, 2016 at 06:23:58PM +0200, Jiri Olsa wrote:

Quoted 9 lines

---

arch/x86/kernel/smpboot.c +0 -0

index 157bf0957219..484f7d357c77 100644
@@ -340,7 +340,7 @@ static void __init smp_init_package_map(void)
 		ncpus = 1;
 	}
 

arch/x86/kernel/smpboot.c static void __init smp_init_package_map(void) · line 343

Quoted 2 lines

arch/x86/kernel/smpboot.c +1 -1

-	__max_logical_packages = DIV_ROUND_UP(total_cpus, ncpus);
+	__max_logical_packages = total_cpus;

This seems undesirable.. it would grow the bitmap unnecessarily big on most setups.

Is there no way to detect the brain damage inflicted by that BIOS option and fudge ncpus in that case?

Quoted 1 line · Peter Zijlstra

On Wed, Aug 03, 2016 at 06:23:58PM +0200, Jiri Olsa wrote:

SNIP

Quoted 15 lines

arch/x86/kernel/smpboot.c static void __init smp_init_package_map(void) · line 343

Quoted 11 lines

---

arch/x86/kernel/smpboot.c +1 -1

index 157bf0957219..484f7d357c77 100644
@@ -340,7 +340,7 @@ static void __init smp_init_package_map(void)
 		ncpus = 1;
 	}
 
-	__max_logical_packages = DIV_ROUND_UP(total_cpus, ncpus);
+	__max_logical_packages = total_cpus;

This seems undesirable.. it would grow the bitmap unnecessarily big on most setups.

Quoted 2 lines

Is there no way to detect the brain damage inflicted by that BIOS option and fudge ncpus in that case?

yea, I have no idea.. that's why this is RFC ;-)

maybe we could also gradually allocate this bitmap in topology_update_package_map.. I'll check on that

it's very likely I'm missing something, but seems to me that attached patch (untested) might work as well

---

arch/x86/kernel/smpboot.c +3 -0

index 157bf0957219..003208bc8e44 100644
@@ -277,14 +276,8 @@ int topology_update_package_map(unsigned int apicid, unsigned int cpu)
 	if (test_and_set_bit(pkg, physical_package_map))
 		goto found;
 
+	new = __max_logical_packages++;
+
 	pr_info("APIC(%x) Converting physical %u to logical package %u\n",
 		apicid, pkg, new);
 	physical_to_logical_pkg[pkg] = new;

@@ -340,7 +333,7 @@ static void __init smp_init_package_map(void)
 		ncpus = 1;
 	}
 
+	__max_logical_packages = 0;
 
 	/*
 	 * Possibly larger than what we need as the number of apic ids per

Quoted 2 lines · Jiri Olsa

it's very likely I'm missing something, but seems to me that attached patch (untested) might work as well

Hmm, I think you're right. Earlier code (which hasn't survived) tried to match logical and physical if possible, but that was busted and got removed.

The code as is should indeed allow for this.

Maybe one nit, the variable is no longer used for a max, so maybe rename it too?

Quoted 2 lines

Maybe one nit, the variable is no longer used for a max, so maybe rename it too?

arch/x86/include/asm/topology.h +0 -0

index cf75871d2f81..c28010088651 100644

Quoted 8 lines · Jiri Olsa

Quoted 2 lines

Maybe one nit, the variable is no longer used for a max, so maybe rename it too?

arch/x86/include/asm/topology.h +0 -0

index cf75871d2f81..c28010088651 100644

arch/x86/include/asm/topology.h extern const struct cpumask *cpu_coregroup_mask(int cpu); · line 125

Quoted 10 lines

arch/x86/include/asm/topology.h +2 -2

@@ -118,8 +118,8 @@ extern const struct cpumask *cpu_coregroup_mask(int cpu);
 #define topology_core_cpumask(cpu)		(per_cpu(cpu_core_map, cpu))
 #define topology_sibling_cpumask(cpu)		(per_cpu(cpu_sibling_map, cpu))
 
-extern unsigned int __max_logical_packages;
-#define topology_max_packages()			(__max_logical_packages)
+extern unsigned int logical_packages;
+#define topology_max_packages()			(logical_packages)
 
 extern int __max_smt_threads;
 

Aaahh, I missed this bit yesterday..

Imagine a machine with physical hotplug, where we boot with half the sockets populated (say 2), then topology_max_packages() will return 2 when we run the uncore driver init.

That driver will allocate resources based on 2.

Then we physically hotplug the remaining sockets (another 2), which will result in topology_max_packages() to now return 4.

When we run the cpuhotplug hook, it will try and access resources based on 4, which were not allocated.

hum, so we either need some acpi solution to get number of all sockets or

fix the uncore code to initialize pmu boxes on cpu hotplug as well

Quoted 2 lines · Jiri Olsa

hum, so we either need some acpi solution to get number of all sockets or

This.. So the problem here is that the BIOS completely screws us over.

It wrecks the ACPI-ID table with that option to limit the number of CPUs exposed to the OS (note that it didn't need to do that, it could have enumerated them as empty, instead of not there at all) while keeping the CPUID of the CPUs as reporting they have many (12? was it) cores.

This results in inconsistent state, and we're left with nothing useful.

Quoted 1 line

fix the uncore code to initialize pmu boxes on cpu hotplug as well

Can't.. it uses the boxes at STARTING time, and we can't do allocs there. Not can we alloc earlier, because we don't know max_packages is going to increase.

Quoted 13 lines · Peter Zijlstra

Quoted 2 lines · Jiri Olsa

hum, so we either need some acpi solution to get number of all sockets or

This.. So the problem here is that the BIOS completely screws us over.

It wrecks the ACPI-ID table with that option to limit the number of CPUs exposed to the OS (note that it didn't need to do that, it could have enumerated them as empty, instead of not there at all) while keeping the CPUID of the CPUs as reporting they have many (12? was it) cores.

This results in inconsistent state, and we're left with nothing useful.

Quoted 5 lines

Quoted 1 line

fix the uncore code to initialize pmu boxes on cpu hotplug as well

Can't.. it uses the boxes at STARTING time, and we can't do allocs there. Not can we alloc earlier, because we don't know max_packages is going to increase.

I still need to test this, but would this be something like you proposed on irc?

I still need to test this, but would this be something like you proposed on irc?

Quoted 2 lines · Jiri Olsa

I still need to test this, but would this be something like you proposed on irc?

Yep, looks good. Please post with Changelog etc..

Quoted 5 lines · Peter Zijlstra

Quoted 2 lines · Jiri Olsa

I still need to test this, but would this be something like you proposed on irc?

Yep, looks good. Please post with Changelog etc..

attached,

thanks,
jirka

---
Frank reported kernel panic when he disabled several cores in BIOS via following option:

Core Disable Bitmap(Hex) [0]

with number 0xFFE, which leaves 16 CPUs in system (out of 48).

The kernel panic below goes along with following messages:

smpboot: Max logical packages: 2^M
smpboot: APIC(0) Converting physical 0 to logical package 0^M
smpboot: APIC(20) Converting physical 1 to logical package 1^M
smpboot: APIC(40) Package 2 exceeds logical package map^M
smpboot: CPU 8 APICId 40 disabled^M
smpboot: APIC(60) Package 3 exceeds logical package map^M
smpboot: CPU 12 APICId 60 disabled^M
...

 general protection fault: 0000 [#1] SMP^M
 Modules linked in:^M
 CPU: 15 PID: 1 Comm: swapper/0 Not tainted 4.7.0-rc5+ #1^M
 Hardware name: SGI UV300/UV300, BIOS SGI UV 300 series BIOS 05/25/2016^M
 task: ffff8801673e0000 ti: ffff8801673ac000 task.ti: ffff8801673ac000^M
 RIP: 0010:[<ffffffff81014d54>]  [<ffffffff81014d54>] uncore_change_context+0xd4/0x180^M
 ...
  [<ffffffff810158ac>] uncore_event_init_cpu+0x6c/0x70^M
  [<ffffffff81d8c91c>] intel_uncore_init+0x1c2/0x2dd^M
  [<ffffffff81d8c75a>] ? uncore_cpu_setup+0x17/0x17^M
  [<ffffffff81002190>] do_one_initcall+0x50/0x190^M
  [<ffffffff810ab193>] ? parse_args+0x293/0x480^M
  [<ffffffff81d87365>] kernel_init_freeable+0x1a5/0x249^M
  [<ffffffff81d86a35>] ? set_debug_rodata+0x12/0x12^M
  [<ffffffff816dc19e>] kernel_init+0xe/0x110^M
  [<ffffffff816e93bf>] ret_from_fork+0x1f/0x40^M
  [<ffffffff816dc190>] ? rest_init+0x80/0x80^M

The reason for the panic is wrong value of __max_logical_packages, which lets logical_package_map uninitialized and the uncore code relying on this map being properly initialized (maybe we should add some safety checks there as well).

The __max_logical_packages is computed as:

DIV_ROUND_UP(total_cpus, ncpus);
- ncpus being number of cores

With above BIOS setup we get total_cpus == 16 which set __max_logical_packages to 2 (ncpus is 12).

Once topology_update_package_map processes CPU with logical pkg over 2 we display above messages and fail to initialize the physical_to_logical_pkg map, which makes the uncore code crash.

The fix is to remove logical_package_map bitmap completely and keep and update the logical_packages number instead.

After we enumerate all the present cpus, we check if the enumerated logical packages count is within its computed maximum from BIOS data.

If it's not the case, we set this maximum to the new enumerated value and freeze any new addition of logical packages.

The freeze is because lot of init code like uncore/rapl/cqm depends on having maximum logical package value set to allocate their data, so we can't change it later on.

Suggested-by Peter Zijlstra <[email protected]>

Reported-by Frank Ramsay <[email protected]>

Signed-off-by Jiri Olsa <[email protected]>

From dmidecode:

Frank reported kernel panic when he disabled several cores in BIOS via following option:

Core Disable Bitmap(Hex) [0]

with number 0xFFE, which leaves 16 CPUs in system (out of 48).

The kernel panic below goes along with following messages:

smpboot: Max logical packages: 2^M
smpboot: APIC(0) Converting physical 0 to logical package 0^M
smpboot: APIC(20) Converting physical 1 to logical package 1^M
smpboot: APIC(40) Package 2 exceeds logical package map^M
smpboot: CPU 8 APICId 40 disabled^M
smpboot: APIC(60) Package 3 exceeds logical package map^M
smpboot: CPU 12 APICId 60 disabled^M
...

 general protection fault: 0000 [#1] SMP^M
 Modules linked in:^M
 CPU: 15 PID: 1 Comm: swapper/0 Not tainted 4.7.0-rc5+ #1^M
 Hardware name: SGI UV300/UV300, BIOS SGI UV 300 series BIOS 05/25/2016^M
 task: ffff8801673e0000 ti: ffff8801673ac000 task.ti: ffff8801673ac000^M
 RIP: 0010:[<ffffffff81014d54>]  [<ffffffff81014d54>] uncore_change_context+0xd4/0x180^M
 ...
  [<ffffffff810158ac>] uncore_event_init_cpu+0x6c/0x70^M
  [<ffffffff81d8c91c>] intel_uncore_init+0x1c2/0x2dd^M
  [<ffffffff81d8c75a>] ? uncore_cpu_setup+0x17/0x17^M
  [<ffffffff81002190>] do_one_initcall+0x50/0x190^M
  [<ffffffff810ab193>] ? parse_args+0x293/0x480^M
  [<ffffffff81d87365>] kernel_init_freeable+0x1a5/0x249^M
  [<ffffffff81d86a35>] ? set_debug_rodata+0x12/0x12^M
  [<ffffffff816dc19e>] kernel_init+0xe/0x110^M
  [<ffffffff816e93bf>] ret_from_fork+0x1f/0x40^M
  [<ffffffff816dc190>] ? rest_init+0x80/0x80^M

The reason for the panic is wrong value of __max_logical_packages, which lets logical_package_map uninitialized and the uncore code relying on this map being properly initialized (maybe we should add some safety checks there as well).

The __max_logical_packages is computed as:

DIV_ROUND_UP(total_cpus, ncpus);
- ncpus being number of cores

With above BIOS setup we get total_cpus == 16 which set __max_logical_packages to 2 (ncpus is 12).

Once topology_update_package_map processes CPU with logical pkg over 2 we display above messages and fail to initialize the physical_to_logical_pkg map, which makes the uncore code crash.

The fix is to set __max_logical_packages directly to total_cpus, which should be the maximum possible logical ID of the pkg in any case.

Reported-by Frank Ramsay <[email protected]>

Signed-off-by Jiri Olsa <[email protected]>

---

arch/x86/kernel/smpboot.c +1 -1

index 157bf0957219..484f7d357c77 100644
@@ -340,7 +340,7 @@ static void __init smp_init_package_map(void)
 		ncpus = 1;
 	}
 
-	__max_logical_packages = DIV_ROUND_UP(total_cpus, ncpus);
+	__max_logical_packages = total_cpus;
 
 	/*
 	 * Possibly larger than what we need as the number of apic ids per