arch/x86: Fix kdump on x86 with physically hotadded CPUs

PATCH In review Prarit Bhargava 4 participants 13 messages Oct 3, 2016 – Oct 7, 2016

Subsystems (4)

x86 perf events Live update kexec handover (KHO)

lore

Share & tools

lore

Prarit Bhargava

author

2016-10-03 17:07 UTC #0 lore raw

PATCH 1/1 arch/x86: Fix kdump on x86 with physically hotadded CPUs

When kdump'ing on a system that has had a socket (package) physically hotadded, the following panic is occasionally seen:

BUG: unable to handle kernel paging request at 0000000000841f1f
IP: [<ffffffff81014ec4>] uncore_change_context+0xd4/0x180
PGD 0
Oops: 0000 [#1] SMP
Modules linked in:
CPU: 0 PID: 12 Comm: cpuhp/0 Not tainted 4.8.0-rc8+ #3
Hardware name: FUJITSU PRIMEQUEST 2800E3/D3752, BIOS PRIMEQUEST 2000 Series BIOS Version 01.17 05/16/2016
task: ffff88002daf1680 task.stack: ffff88002dafc000
RIP: 0010:[<ffffffff81014ec4>]  [<ffffffff81014ec4>] uncore_change_context+0xd4/0x180
RSP: 0000:ffff88002daffdc8  EFLAGS: 00010286
RAX: ffff88002c069c00 RBX: 0000000000841f0f RCX: ffffffffffffffff
RDX: 000000000000a020 RSI: 00000000ffffffff RDI: ffffffff81c18fa0
RBP: ffff88002daffe10 R08: 0000000000000000 R09: 0000000000000000
R10: 000000000007fff8 R11: 00000000a585a840 R12: ffff88002c0a4400
R13: 0000000000000000 R14: 0000000000000000 R15: ffffffff81c19a20
FS:  0000000000000000(0000) GS:ffff880032c00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000841f1f CR3: 0000000031c06000 CR4: 00000000003406b0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Stack:
 000000000000a020 ffffffff81c18fa0 ffff88002daf28c0 ffff88002daffdf0
 0000000000000000 0000000000000000 000000000000004a ffffffff81015a60
 0000000000000000 ffff88002daffe30 ffffffff81015acc ffff880032c0dda0
Call Trace:
 [<ffffffff81015a60>] ? uncore_cpu_starting+0x130/0x130
 [<ffffffff81015acc>] uncore_event_cpu_online+0x6c/0x80
 [<ffffffff8108e819>] cpuhp_invoke_callback+0x49/0x100
 [<ffffffff8108ead1>] cpuhp_thread_fun+0x41/0x100
 [<ffffffff810b054f>] smpboot_thread_fn+0x10f/0x160
 [<ffffffff810b0440>] ? sort_range+0x30/0x30
 [<ffffffff810accd8>] kthread+0xd8/0xf0
 [<ffffffff816ff4bf>] ret_from_fork+0x1f/0x40
 [<ffffffff810acc00>] ? kthread_park+0x60/0x60
Code: c8 44 89 73 10 41 83 c5 01 49 81 c4 48 01 00 00 45 3b 6f 0c 7d 21 49 8b 84 24 40 01 00 00 4a 8b 1c 10 48 85 db 74 de 85 c9 79 96 <83> 7b 10 ff 75 63 44 89 73 10 eb ce 48 83 45 c0 08 48 8b 45 c0
RIP  [<ffffffff81014ec4>] uncore_change_context+0xd4/0x180
 RSP <ffff88002daffdc8>
CR2: 0000000000841f1f
---[ end trace 2ce4e89368333d22 ]---
Kernel panic - not syncing: Fatal exception

Rebooting in 10 seconds..
ACPI MEMORY or I/O RESET_REG.

The panic shows what the problem is:

arch/x86/events/intel/uncore.c:
1137 static void uncore_change_type_ctx(struct intel_uncore_type *type, int old_ cpu,
1138 int new_cpu)
1139 {

1140         struct intel_uncore_pmu *pmu = type->pmus;
1141         struct intel_uncore_box *box;
1142         int i, pkg;

1143
1144 pkg = topology_logical_package_id(old_cpu < 0 ? new_cpu : old_cpu);
1145 for (i = 0; i < type->num_boxes; i++, pmu++) {
1146 box = pmu->boxes[pkg];

pmu->boxes[pkg] is garbage because pkg was returned as 0xffff. topology_logical_package_id() is defined as

|#define topology_logical_package_id(cpu) (cpu_data(cpu).logical_proc_id

which means that logical_proc_id was not defined. logical_proc_id is set in arch/x86/kernel/smpboot.c:topology_update_package_map(), which is called in arch/x86/kernel/smpboot.c:smp_init_package_map.

smp_init_package_map() was introduced in 1f12e32f4cd5 ("x86/topology:
Create logical package id"), and does

arch/x86/kernel/smpboot.c:
358 for_each_present_cpu(cpu) {
359 unsigned int apicid = apic->cpu_present_to_apicid(cpu);
360
361 if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))
362 continue;
363 if (!topology_update_package_map(apicid, cpu))
364 continue;

which means that apic->cpu_present_to_apicid(cpu) is returning BAD_APICID (experimentally verified that it is not the acpi_id_valid() that is the problem) so that topology_update_package_map() is not called for the cpu, and the cpu's pkg value will remain the default value of 0xffff.

Following through function pointers, cpu_present_to_apicid() resolves as default_cpu_present_to_apicid() which is __default_cpu_present_to_apicid() for x86_64.

arch/x86/include/asm/apic.h:
605 static inline int __default_cpu_present_to_apicid(int mps_cpu) 606 {
607 if (mps_cpu < nr_cpu_ids && cpu_present(mps_cpu))
608 return (int)per_cpu(x86_bios_cpu_apicid, mps_cpu);
609 else
610 return BAD_APICID;
611 }

The per_cpu field x86_bios_cpu_apicid is set in generic_processor_info(). After verifying that the mps_cpu was 0 and the cpu was in the present map, the only way that x86_bios_cpu_apicid is BAD_APICID for a valid cpu is if the cpu initialization function generic_processor_info() was not called on the cpu.

As part of acpi_boot_init(), the acpi_register_lapic() calls generic_processor_info() and is called for all APIC entries in the MADT table. The ACPI 6.0 Specification states that the ACPI X2APIC tables does not have to update on a cpu hotplug event:

"5.2.12.12 Processor Local x2APIC Structure

OSPM does not expect the information provided in this table to be updated if the processor information changes during the lifespan of an OS boot."

and that explains why generic_processor_info() was not called on a hotplugged cpu during the kdump kernel boot.

Hot adding a cpu to a system and testing kdump [1] with

taskset -c {hotadded thread id} echo c > /proc/sysrq-trigger

makes the panic occur 100% of the time. Targetting a cpu that is present in the MADT results in a valid kdump 100% of time. These two combined explain the occasional nature of the panic.

The boot log also contains evidence that generic_processor_info() wasn't called on the boot cpu, and that was the problem:

smpboot: weird, boot CPU (#507) not listed by the BIOS

and

APIC: NR_CPUS/possible_cpus limit of 1 almost reached. Keeping one slot for boot cpu. Processor 1/0x2 ignored.

entries are listed for each cpu but there is no indication that the boot cpu was enumerated in ACPI. Adding a debug printk shows num_processors is 0 after the ACPI enumeration is complete.

After the ACPI enumeration is complete, prefill_possible_map() [2] checks if num_processors is 0 and sets it to 1 to account for a boot cpu that wasn't enumerated. However, prefill_possible_map() does not call generic_processor_info() on the boot cpu which leaves the boot cpu with partially uninitialized data.

This patch adds the missing generic_processor_info() to prefill_possible_map() to ensure the initialization of the boot cpu is correct. This results in smp_init_package_map() having correct data and properly setting the package map for the hotplugged boot cpu, which in turn resolves the kdump kernel panic on physically hotplugged cpus.

[1] This can be simulated in a KVM environment by hot adding a CPU and using taskset to force the dump on the newly added CPU.
[2] prefill_possible_map() is called before smp_store_boot_cpu_info(). The comment beside the call to smp_store_boot_cpu_info() states that the completed call results in "Final full version of the data".

Signed-off-by Prarit Bhargava <[email protected]>

Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc [email protected]

Cc Peter Zijlstra <[email protected]>

Cc Len Brown <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Andi Kleen <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Juergen Gross <[email protected]>

Cc [email protected]

Cc Eric Biederman <[email protected]>

Cc [email protected]

---
 arch/x86/kernel/smpboot.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

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

index 4296beb8fdd3..d1272febc13b 100644
@@ -1406,9 +1406,18 @@ __init void prefill_possible_map(void)
 {
 	int i, possible;
 
-	/* no processor from mptable or madt */
-	if (!num_processors)
-		num_processors = 1;
+	/* No boot processor was found in mptable or ACPI MADT */
+	if (!num_processors) {
+		/* Make sure boot cpu is enumerated */
+		if (apic->cpu_present_to_apicid(0) == BAD_APICID &&
+		    apic->apic_id_valid(boot_cpu_physical_apicid))
+			generic_processor_info(boot_cpu_physical_apicid,
+					apic_version[boot_cpu_physical_apicid]);
+		if (!num_processors) {
+			pr_warn("CPU 0 not enumerated in mptable or ACPI MADT\n");
+			num_processors = 1;
+		}
+	}
 
 	i = setup_max_cpus ?: 1;
 	if (setup_possible_cpus == -1) {

Signature · 1 line

1.7.9.3

Jiri Olsa

Reviewed-by on v1 1/1

2016-10-03 22:23 UTC #1 lore raw

On Mon, Oct 03, 2016 at 01:07:12PM -0400, Prarit Bhargava wrote:

SNIP

Quoted 13 lines

This patch adds the missing generic_processor_info() to prefill_possible_map() to ensure the initialization of the boot cpu is correct. This results in smp_init_package_map() having correct data and properly setting the package map for the hotplugged boot cpu, which in turn resolves the kdump kernel panic on physically hotplugged cpus.
[1] This can be simulated in a KVM environment by hot adding a CPU and using taskset to force the dump on the newly added CPU.
[2] prefill_possible_map() is called before smp_store_boot_cpu_info(). The comment beside the call to smp_store_boot_cpu_info() states that the completed call results in "Final full version of the data".

Quoted 15 lines

Signed-off-by Prarit Bhargava <[email protected]>
Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc [email protected]
Cc Peter Zijlstra <[email protected]>
Cc Len Brown <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Juergen Gross <[email protected]>
Cc [email protected]
Cc Eric Biederman <[email protected]>
Cc [email protected]

Reviewed-by Jiri Olsa <[email protected]>

thanks,
jirka

Quoted 33 lines

---
 arch/x86/kernel/smpboot.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

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

index 4296beb8fdd3..d1272febc13b 100644
@@ -1406,9 +1406,18 @@ __init void prefill_possible_map(void)
 {
 	int i, possible;
 
-	/* no processor from mptable or madt */
-	if (!num_processors)
-		num_processors = 1;
+	/* No boot processor was found in mptable or ACPI MADT */
+	if (!num_processors) {
+		/* Make sure boot cpu is enumerated */
+		if (apic->cpu_present_to_apicid(0) == BAD_APICID &&
+		    apic->apic_id_valid(boot_cpu_physical_apicid))
+			generic_processor_info(boot_cpu_physical_apicid,
+					apic_version[boot_cpu_physical_apicid]);
+		if (!num_processors) {
+			pr_warn("CPU 0 not enumerated in mptable or ACPI MADT\n");
+			num_processors = 1;
+		}
+	}
 
 	i = setup_max_cpus ?: 1;
 	if (setup_possible_cpus == -1) {

Signature · 1 line

1.7.9.3

Thomas Gleixner

2016-10-04 11:00 UTC #2 lore raw

In reply to Prarit Bhargava

Quoted 2 lines · Prarit Bhargava

BUG: unable to handle kernel paging request at 0000000000841f1f
IP: [<ffffffff81014ec4>] uncore_change_context+0xd4/0x180

...

Quoted 17 lines

[<ffffffff81015a60>] ? uncore_cpu_starting+0x130/0x130
 [<ffffffff81015acc>] uncore_event_cpu_online+0x6c/0x80
 [<ffffffff8108e819>] cpuhp_invoke_callback+0x49/0x100
 [<ffffffff8108ead1>] cpuhp_thread_fun+0x41/0x100
 [<ffffffff810b054f>] smpboot_thread_fn+0x10f/0x160
 [<ffffffff810b0440>] ? sort_range+0x30/0x30
 [<ffffffff810accd8>] kthread+0xd8/0xf0
 [<ffffffff816ff4bf>] ret_from_fork+0x1f/0x40
 [<ffffffff810acc00>] ? kthread_park+0x60/0x60
arch/x86/events/intel/uncore.c:
1137 static void uncore_change_type_ctx(struct intel_uncore_type *type, int old_ cpu,
1138 int new_cpu)
1139 {
1140         struct intel_uncore_pmu *pmu = type->pmus;
1141         struct intel_uncore_box *box;
1142         int i, pkg;

Quoted 6 lines

1143
1144 pkg = topology_logical_package_id(old_cpu < 0 ? new_cpu : old_cpu);
1145 for (i = 0; i < type->num_boxes; i++, pmu++) {
1146 box = pmu->boxes[pkg];
pmu->boxes[pkg] is garbage because pkg was returned as 0xffff.

And that's what needs to be fixed in the first place.

Quoted 7 lines

This patch adds the missing generic_processor_info() to prefill_possible_map() to ensure the initialization of the boot cpu is correct.
This results in smp_init_package_map() having correct data and properly setting the package map for the hotplugged boot cpu, which in turn resolves the kdump kernel panic on physically hotplugged cpus.

While it is the right thing to initialize the package map in that case, it still papers over a robustness issue in the uncore code, which needs to be fixed first.

Quoted 3 lines

[2] prefill_possible_map() is called before smp_store_boot_cpu_info(). The comment beside the call to smp_store_boot_cpu_info() states that the completed call results in "Final full version of the data".

I'm not sure what that [2] here means and I cannot figure out the meaning of this sentence either.

This changelog is incomprehensible in general and more a "oh look how I decoded this problem" report than something which clearly describes the problem at hand, the root cause and the fix. The latter wants a understandable explanation why prefill_possible_map() is the right place to do this.

Quoted 11 lines

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

index 4296beb8fdd3..d1272febc13b 100644
@@ -1406,9 +1406,18 @@ __init void prefill_possible_map(void)
 {
 	int i, possible;
 
-	/* no processor from mptable or madt */
-	if (!num_processors)
-		num_processors = 1;

arch/x86/kernel/smpboot.c __init void prefill_possible_map(void) · line 1418

Quoted 10 lines

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

+	/* No boot processor was found in mptable or ACPI MADT */
+	if (!num_processors) {
+		/* Make sure boot cpu is enumerated */
+		if (apic->cpu_present_to_apicid(0) == BAD_APICID &&
+		    apic->apic_id_valid(boot_cpu_physical_apicid))
+			generic_processor_info(boot_cpu_physical_apicid,
+					apic_version[boot_cpu_physical_apicid]);
+		if (!num_processors) {
+			pr_warn("CPU 0 not enumerated in mptable or ACPI MADT\n");
+			num_processors = 1;

And in this case we end up with the same problem, right?

Thanks,

tglx

Prarit Bhargava

author

2016-10-04 12:09 UTC #3 lore raw

Quoted 34 lines · Thomas Gleixner

Quoted 2 lines · Prarit Bhargava
BUG: unable to handle kernel paging request at 0000000000841f1f
IP: [<ffffffff81014ec4>] uncore_change_context+0xd4/0x180
...
Quoted 17 lines
[<ffffffff81015a60>] ? uncore_cpu_starting+0x130/0x130
 [<ffffffff81015acc>] uncore_event_cpu_online+0x6c/0x80
 [<ffffffff8108e819>] cpuhp_invoke_callback+0x49/0x100
 [<ffffffff8108ead1>] cpuhp_thread_fun+0x41/0x100
 [<ffffffff810b054f>] smpboot_thread_fn+0x10f/0x160
 [<ffffffff810b0440>] ? sort_range+0x30/0x30
 [<ffffffff810accd8>] kthread+0xd8/0xf0
 [<ffffffff816ff4bf>] ret_from_fork+0x1f/0x40
 [<ffffffff810acc00>] ? kthread_park+0x60/0x60
arch/x86/events/intel/uncore.c:
1137 static void uncore_change_type_ctx(struct intel_uncore_type *type, int old_ cpu,
1138 int new_cpu)
1139 {
1140         struct intel_uncore_pmu *pmu = type->pmus;
1141         struct intel_uncore_box *box;
1142         int i, pkg;
Quoted 6 lines
1143
1144 pkg = topology_logical_package_id(old_cpu < 0 ? new_cpu : old_cpu);
1145 for (i = 0; i < type->num_boxes; i++, pmu++) {
1146 box = pmu->boxes[pkg];
pmu->boxes[pkg] is garbage because pkg was returned as 0xffff.
And that's what needs to be fixed in the first place.
Quoted 3 lines
This patch adds the missing generic_processor_info() to prefill_possible_map() to ensure the initialization of the boot cpu is correct.

Quoted 7 lines

Quoted 3 lines
This results in smp_init_package_map() having correct data and properly setting the package map for the hotplugged boot cpu, which in turn resolves the kdump kernel panic on physically hotplugged cpus.
While it is the right thing to initialize the package map in that case, it still papers over a robustness issue in the uncore code, which needs to be fixed first.

I will include a separate patch with an error check for pkg == 0xffff in the uncore code.

Quoted 7 lines

Quoted 4 lines
[2] prefill_possible_map() is called before smp_store_boot_cpu_info(). The comment beside the call to smp_store_boot_cpu_info() states that the completed call results in "Final full version of the data".
I'm not sure what that [2] here means and I cannot figure out the meaning of this sentence either.

My understanding is that after the call to smp_store_boot_cpu_info(), that for the rest of the initial bringup the cpu_data structs, etc., are complete.

Quoted 4 lines

This changelog is incomprehensible in general and more a "oh look how I decoded this problem" report than something which clearly describes the problem at hand, the root cause and the fix.

That wasn't my intention.

I had to figure it out and it took quite a while to get through it. I had doubts that all of the others on the cc list would understand this and I expected questions of "Can you prove that is the case?".

My changelog addresses those expected questions and I have no problem with a shortened changelog.

The latter wants a

Quoted 12 lines

understandable explanation why prefill_possible_map() is the right place to do this.
Quoted 8 lines
arch/x86/kernel/smpboot.c +0 -0
index 4296beb8fdd3..d1272febc13b 100644
@@ -1406,9 +1406,18 @@ __init void prefill_possible_map(void)
 {
 	int i, possible;
 

Quoted 15 lines

arch/x86/kernel/smpboot.c __init void prefill_possible_map(void) · line 1418

Quoted 13 lines

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

-	/* no processor from mptable or madt */
-	if (!num_processors)
-		num_processors = 1;
+	/* No boot processor was found in mptable or ACPI MADT */
+	if (!num_processors) {
+		/* Make sure boot cpu is enumerated */
+		if (apic->cpu_present_to_apicid(0) == BAD_APICID &&
+		    apic->apic_id_valid(boot_cpu_physical_apicid))
+			generic_processor_info(boot_cpu_physical_apicid,
+					apic_version[boot_cpu_physical_apicid]);
+		if (!num_processors) {
+			pr_warn("CPU 0 not enumerated in mptable or ACPI MADT\n");
+			num_processors = 1;

And in this case we end up with the same problem, right?

It occurs to me that I over thought this: I was thinking that there might exist a pre-ACPI (or at least a system without an MADT) x86 system that wold boot such that num_processors = 0. But in that case, the cpu should be listed in the mptables so the above should not happen. I'll change that to a BUG().

Quoted 5 lines

Thanks,
tglx

Thomas Gleixner

2016-10-04 14:40 UTC #4 lore raw

Quoted 7 lines · Prarit Bhargava

Quoted 3 lines · Thomas Gleixner
While it is the right thing to initialize the package map in that case, it still papers over a robustness issue in the uncore code, which needs to be fixed first.
I will include a separate patch with an error check for pkg == 0xffff in the uncore code.

0xffff? That won't help. The id returned is -1 if the entry is not initialized. And aside of that just patching that particular place is not helping as the uncore code and also rapl is relying on the package map being populated.

So we need a sanity check in the initialization code which prevents any of this being executed.

Quoted 10 lines

Quoted 5 lines
arch/x86/kernel/smpboot.c __init void prefill_possible_map(void) · line 1418
Quoted 3 lines
arch/x86/kernel/smpboot.c +3 -0
+		if (!num_processors) {
+			pr_warn("CPU 0 not enumerated in mptable or ACPI MADT\n");
+			num_processors = 1;
And in this case we end up with the same problem, right?
It occurs to me that I over thought this: I was thinking that there might exist a pre-ACPI (or at least a system without an MADT) x86 system that wold boot such that num_processors = 0. But in that case, the cpu should be listed in the mptables so the above should not happen. I'll change that to a BUG().

No. That's the wrong thing to do. Think SMP kernel on UP machines ...

Thanks,

tglx

Prarit Bhargava

author

2016-10-04 16:01 UTC #5 lore raw

Quoted 6 lines · Thomas Gleixner

Quoted 5 lines · Prarit Bhargava
Quoted 3 lines · Thomas Gleixner
While it is the right thing to initialize the package map in that case, it still papers over a robustness issue in the uncore code, which needs to be fixed first.

Quoted 7 lines

Quoted 2 lines
I will include a separate patch with an error check for pkg == 0xffff in the uncore code.
0xffff? That won't help. The id returned is -1 if the entry is not initialized. And aside of that just patching that particular place is not helping as the uncore code and also rapl is relying on the package map being populated.

Yes, I noticed that after I started digging into it this morning. Not only what you pointed out but there's init that occurs in the uncore code that would have to be undone.

There is a similar issue in the rapl code, but that code inadvertently protects itself with for loops that end up never running (and that's why the rapl code doesn't panic).

Quoted 3 lines

So we need a sanity check in the initialization code which prevents any of this being executed.

Ok, should this be done only for logical_proc_id or for logical_proc_id, phys_proc_id, and cpu_core_id? What do you think of adding that to the end of smp_init_package_map() or smp_store_cpu_info()?

Quoted 7 lines

Quoted 7 lines
Quoted 6 lines
arch/x86/kernel/smpboot.c __init void prefill_possible_map(void) · line 1418
Quoted 4 lines
arch/x86/kernel/smpboot.c +3 -0
+		if (!num_processors) {
+			pr_warn("CPU 0 not enumerated in mptable or ACPI MADT\n");
+			num_processors = 1;
And in this case we end up with the same problem, right?

Quoted 7 lines

Quoted 4 lines
It occurs to me that I over thought this: I was thinking that there might exist a pre-ACPI (or at least a system without an MADT) x86 system that wold boot such that num_processors = 0. But in that case, the cpu should be listed in the mptables so the above should not happen. I'll change that to a BUG().
No. That's the wrong thing to do. Think SMP kernel on UP machines ...

Sorry Thomas, but my history with real UP hardware is limited. I think you might be saying I should call generic_processor_info(0, apic_version[0]) to populate cpu 0 but I'm not sure.

Jiri Olsa

2016-10-05 16:15 UTC #6 lore raw

In reply to Thomas Gleixner

Quoted 9 lines · Thomas Gleixner

Quoted 7 lines · Prarit Bhargava
Quoted 3 lines · Thomas Gleixner
While it is the right thing to initialize the package map in that case, it still papers over a robustness issue in the uncore code, which needs to be fixed first.
I will include a separate patch with an error check for pkg == 0xffff in the uncore code.

Quoted 7 lines

0xffff? That won't help. The id returned is -1 if the entry is not initialized. And aside of that just patching that particular place is not helping as the uncore code and also rapl is relying on the package map being populated.
So we need a sanity check in the initialization code which prevents any of this being executed.

I still need to test this, but how about something like this?

thanks,
jirka

---

arch/x86/events/intel/rapl.c +7 -2

index 28865938aadf..61d087a2f25d 100644
@@ -598,8 +598,13 @@ static int rapl_cpu_online(unsigned int cpu)
 
 static int rapl_cpu_prepare(unsigned int cpu)
 {
-	struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);
+	struct rapl_pmu *pmu;
+	int pkg = topology_logical_package_id(cpu);
+
+	if (WARN_ON(pkg == -1))
+		return -EINVAL;
 
+	pmu = cpu_to_rapl_pmu(cpu);
 	if (pmu)
 		return 0;

@@ -613,7 +618,7 @@ static int rapl_cpu_prepare(unsigned int cpu)
 	pmu->timer_interval = ms_to_ktime(rapl_timer_ms);
 	pmu->cpu = -1;
 	rapl_hrtimer_init(pmu);
-	rapl_pmus->pmus[topology_logical_package_id(cpu)] = pmu;
+	rapl_pmus->pmus[pkg] = pmu;
 	return 0;
 }

arch/x86/events/intel/uncore.c +8 -1

index 463dc7a5a6c3..3f657fbbf787 100644
@@ -1352,7 +1352,7 @@ static int __init intel_uncore_init(void)
 {
 	const struct x86_cpu_id *id;
 	struct intel_uncore_init_fun *uncore_init;
-	int pret = 0, cret = 0, ret;
+	int pret = 0, cret = 0, ret, cpu;
 
 	id = x86_match_cpu(intel_uncore_match);
 	if (!id)

@@ -1363,6 +1363,13 @@ static int __init intel_uncore_init(void)
 
 	max_packages = topology_max_packages();
 
+	for_each_present_cpu(cpu) {
+		int pkg = topology_logical_package_id(cpu);
+
+		if (WARN_ON(pkg == -1))
+			return -EINVAL;
+	}
+
 	uncore_init = (struct intel_uncore_init_fun *)id->driver_data;
 	if (uncore_init->pci_init) {
 		pret = uncore_init->pci_init();

Prarit Bhargava

author

2016-10-06 15:25 UTC #7 lore raw

Quoted 33 lines · Jiri Olsa

Quoted 9 lines · Thomas Gleixner
Quoted 7 lines · Prarit Bhargava
Quoted 3 lines · Thomas Gleixner
While it is the right thing to initialize the package map in that case, it still papers over a robustness issue in the uncore code, which needs to be fixed first.
I will include a separate patch with an error check for pkg == 0xffff in the uncore code.
Quoted 7 lines
0xffff? That won't help. The id returned is -1 if the entry is not initialized. And aside of that just patching that particular place is not helping as the uncore code and also rapl is relying on the package map being populated.
So we need a sanity check in the initialization code which prevents any of this being executed.
I still need to test this, but how about something like this?
thanks,
jirka
---
arch/x86/events/intel/rapl.c +0 -0
index 28865938aadf..61d087a2f25d 100644
@@ -598,8 +598,13 @@ static int rapl_cpu_online(unsigned int cpu)
 
 static int rapl_cpu_prepare(unsigned int cpu)
 {

arch/x86/events/intel/rapl.c static int rapl_cpu_online(unsigned int cpu) · line 609

Quoted 10 lines

arch/x86/events/intel/rapl.c +6 -1

-	struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);
+	struct rapl_pmu *pmu;
+	int pkg = topology_logical_package_id(cpu);
+
+	if (WARN_ON(pkg == -1))
+		return -EINVAL;
 
+	pmu = cpu_to_rapl_pmu(cpu);
 	if (pmu)
 		return 0;

I thought about doing this but it seems like every time some driver uses topology_logical_package_id() the driver would have to replicate the error checking code.

Jiri Olsa

2016-10-07 06:49 UTC #8 lore raw

Quoted 43 lines · Prarit Bhargava

Quoted 40 lines · Jiri Olsa
Quoted 9 lines · Thomas Gleixner
Quoted 7 lines · Prarit Bhargava
Quoted 3 lines · Thomas Gleixner
While it is the right thing to initialize the package map in that case, it still papers over a robustness issue in the uncore code, which needs to be fixed first.
I will include a separate patch with an error check for pkg == 0xffff in the uncore code.
Quoted 7 lines
0xffff? That won't help. The id returned is -1 if the entry is not initialized. And aside of that just patching that particular place is not helping as the uncore code and also rapl is relying on the package map being populated.
So we need a sanity check in the initialization code which prevents any of this being executed.
I still need to test this, but how about something like this?
thanks,
jirka
---
arch/x86/events/intel/rapl.c +5 -1
index 28865938aadf..61d087a2f25d 100644
@@ -598,8 +598,13 @@ static int rapl_cpu_online(unsigned int cpu)
 
 static int rapl_cpu_prepare(unsigned int cpu)
 {
-	struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);
+	struct rapl_pmu *pmu;
+	int pkg = topology_logical_package_id(cpu);
+
+	if (WARN_ON(pkg == -1))
+		return -EINVAL;
 

Quoted 7 lines

arch/x86/events/intel/rapl.c static int rapl_cpu_online(unsigned int cpu) · line 609
Quoted 3 lines
arch/x86/events/intel/rapl.c +1 -0
+	pmu = cpu_to_rapl_pmu(cpu);
 	if (pmu)
 		return 0;
I thought about doing this but it seems like every time some driver uses topology_logical_package_id() the driver would have to replicate the error checking code.

hm, unless we guarantee topology_logical_package_id always returns sane values I dont see another way

jirka

Thomas Gleixner

2016-10-07 08:05 UTC #9 lore raw

Quoted 7 lines · Jiri Olsa

Quoted 3 lines · Prarit Bhargava
I thought about doing this but it seems like every time some driver uses topology_logical_package_id() the driver would have to replicate the error checking code.
hm, unless we guarantee topology_logical_package_id always returns sane values I dont see another way

I did some deeper investigation. So with prarits patch there is only one possibility left to end up with an empty package id:

SMP Kernel on UP Machine, which has neither ACPI nor MADT. But those machines are not a problem because they don't use any drivers which would make use of it. Aside of that for UP we just can set the node of the cpu to 0 and be done with it. I'll pick up Prarits patch and amend it.

Thanks,

tglx

Jiri Olsa

2016-10-04 12:27 UTC #10 lore raw

In reply to Thomas Gleixner

Quoted 26 lines · Thomas Gleixner

Quoted 2 lines · Prarit Bhargava
BUG: unable to handle kernel paging request at 0000000000841f1f
IP: [<ffffffff81014ec4>] uncore_change_context+0xd4/0x180
...
Quoted 22 lines
[<ffffffff81015a60>] ? uncore_cpu_starting+0x130/0x130
 [<ffffffff81015acc>] uncore_event_cpu_online+0x6c/0x80
 [<ffffffff8108e819>] cpuhp_invoke_callback+0x49/0x100
 [<ffffffff8108ead1>] cpuhp_thread_fun+0x41/0x100
 [<ffffffff810b054f>] smpboot_thread_fn+0x10f/0x160
 [<ffffffff810b0440>] ? sort_range+0x30/0x30
 [<ffffffff810accd8>] kthread+0xd8/0xf0
 [<ffffffff816ff4bf>] ret_from_fork+0x1f/0x40
 [<ffffffff810acc00>] ? kthread_park+0x60/0x60
arch/x86/events/intel/uncore.c:
1137 static void uncore_change_type_ctx(struct intel_uncore_type *type, int old_ cpu,
1138 int new_cpu)
1139 {
1140         struct intel_uncore_pmu *pmu = type->pmus;
1141         struct intel_uncore_box *box;
1142         int i, pkg;
1143
1144 pkg = topology_logical_package_id(old_cpu < 0 ? new_cpu : old_cpu);
1145 for (i = 0; i < type->num_boxes; i++, pmu++) {
1146 box = pmu->boxes[pkg];

Quoted 3 lines

Quoted 1 line
pmu->boxes[pkg] is garbage because pkg was returned as 0xffff.
And that's what needs to be fixed in the first place.

right, I'll check on that.. but I think we need this fix as well

Quoted 17 lines

Quoted 8 lines
This patch adds the missing generic_processor_info() to prefill_possible_map() to ensure the initialization of the boot cpu is correct.
This results in smp_init_package_map() having correct data and properly setting the package map for the hotplugged boot cpu, which in turn resolves the kdump kernel panic on physically hotplugged cpus.
While it is the right thing to initialize the package map in that case, it still papers over a robustness issue in the uncore code, which needs to be fixed first.
Quoted 3 lines
[2] prefill_possible_map() is called before smp_store_boot_cpu_info(). The comment beside the call to smp_store_boot_cpu_info() states that the completed call results in "Final full version of the data".

Quoted 8 lines

I'm not sure what that [2] here means and I cannot figure out the meaning of this sentence either.
This changelog is incomprehensible in general and more a "oh look how I decoded this problem" report than something which clearly describes the problem at hand, the root cause and the fix. The latter wants a understandable explanation why prefill_possible_map() is the right place to do this.

I was wondering if acpi_boot_init was a better place for that, but then Prarit suggested in our discussion that the prefill_possible_map() call seems to be a hotplug cleanup.. so it seemed to fit

however it's difficult to say with complex code like this, so any ideas are welcome ;-)

thanks,
jirka

Thomas Gleixner

2016-10-04 14:22 UTC #11 lore raw

Quoted 6 lines · Jiri Olsa

Quoted 3 lines · Thomas Gleixner
Quoted 1 line
pmu->boxes[pkg] is garbage because pkg was returned as 0xffff.
And that's what needs to be fixed in the first place.
right, I'll check on that.. but I think we need this fix as well

That's right.

Quoted 3 lines

I was wondering if acpi_boot_init was a better place for that, but then Prarit suggested in our discussion that the prefill_possible_map() call seems to be a hotplug cleanup.. so it seemed to fit

It's the right place for it.

Thanks,

tglx

tip-bot for Prarit Bhargava

merge bot

2016-10-07 13:28 UTC #12 lore raw

[tip:x86/urgent] arch/x86: Handle non enumerated CPU after physical hotplug

In reply to Prarit Bhargava

arch/x86: Handle non enumerated CPU after physical hotplug

Commit-ID:  2a51fe083eba7f99cbda72f5ef90cdf2f4df882c
Gitweb:     http://git.kernel.org/tip/2a51fe083eba7f99cbda72f5ef90cdf2f4df882c
Author:     Prarit Bhargava <[email protected]>
AuthorDate: Mon, 3 Oct 2016 13:07:12 -0400
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Fri, 7 Oct 2016 15:22:15 +0200

arch/x86: Handle non enumerated CPU after physical hotplug

When a CPU is physically added to a system then the MADT table is not updated.

If subsequently a kdump kernel is started on that physically added CPU then the ACPI enumeration fails to provide the information for this CPU which is now the boot CPU of the kdump kernel.

As a consequence, generic_processor_info() is not invoked for that CPU so the number of enumerated processors is 0 and none of the initializations, including the logical package id management, are performed.

We have code which relies on the correctness of the logical package map and other information which is initialized via generic_processor_info(). Executing such code will result in undefined behaviour or kernel crashes.

This problem applies only to the kdump kernel because a normal kexec will switch to the original boot CPU, which is enumerated in MADT, before jumping into the kexec kernel.

The boot code already has a check for num_processors equal 0 in prefill_possible_map(). We can use that check as an indicator that the enumeration of the boot CPU did not happen and invoke generic_processor_info() for it. That initializes the relevant data for the boot CPU and therefore prevents subsequent failure.

[ tglx: Refined the code and rewrote the changelog ]

Signed-off-by Prarit Bhargava <[email protected]>

Fixes 1f12e32f4cd5 ("x86/topology: Create logical package id")

Cc Peter Zijlstra <[email protected]>

Cc Len Brown <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Andi Kleen <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Juergen Gross <[email protected]>

Cc [email protected]

Cc Eric Biederman <[email protected]>

Cc [email protected]

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/kernel/smpboot.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

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

index 42a9362..951f093 100644
@@ -1407,9 +1407,21 @@ __init void prefill_possible_map(void)
 {
 	int i, possible;
 
-	/* no processor from mptable or madt */
-	if (!num_processors)
-		num_processors = 1;
+	/* No boot processor was found in mptable or ACPI MADT */
+	if (!num_processors) {
+		int apicid = boot_cpu_physical_apicid;
+		int cpu = hard_smp_processor_id();
+
+		pr_warn("Boot CPU (id %d) not listed by BIOS\n", cpu);
+
+		/* Make sure boot cpu is enumerated */
+		if (apic->cpu_present_to_apicid(0) == BAD_APICID &&
+		    apic->apic_id_valid(apicid))
+			generic_processor_info(apicid, boot_cpu_apic_version);
+
+		if (!num_processors)
+			num_processors = 1;
+	}
 
 	i = setup_max_cpus ?: 1;
 	if (setup_possible_cpus == -1) {