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

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

[Prarit Bhargava]

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 <prarit@redhat.com>
Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Len Brown <len.brown@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: dyoung@redhat.com
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: kexec@lists.infradead.org
---
 arch/x86/kernel/smpboot.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 4296beb8fdd3..d1272febc13b 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -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) {
-- 
1.7.9.3

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

[Prarit Bhargava]

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 <prarit@redhat.com>
Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Len Brown <len.brown@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: dyoung@redhat.com
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: kexec@lists.infradead.org
---
 arch/x86/kernel/smpboot.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 4296beb8fdd3..d1272febc13b 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -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) {
-- 
1.7.9.3


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Jiri Olsa]

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

SNIP

> 
> 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 <prarit@redhat.com>
> Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: dyoung@redhat.com
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: kexec@lists.infradead.org

Reviewed-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

> ---
>  arch/x86/kernel/smpboot.c |   15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 4296beb8fdd3..d1272febc13b 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -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) {
> -- 
> 1.7.9.3
> 

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Jiri Olsa]

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

SNIP

> 
> 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 <prarit@redhat.com>
> Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: dyoung@redhat.com
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: kexec@lists.infradead.org

Reviewed-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

> ---
>  arch/x86/kernel/smpboot.c |   15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 4296beb8fdd3..d1272febc13b 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -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) {
> -- 
> 1.7.9.3
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Thomas Gleixner]

On Mon, 3 Oct 2016, Prarit Bhargava wrote:
> BUG: unable to handle kernel paging request at 0000000000841f1f
> IP: [<ffffffff81014ec4>] uncore_change_context+0xd4/0x180
...
>  [<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];
> 
> pmu->boxes[pkg] is garbage because pkg was returned as 0xffff.

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

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

> [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.

> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 4296beb8fdd3..d1272febc13b 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -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;

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

Thanks,

	tglx

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Thomas Gleixner]

On Mon, 3 Oct 2016, Prarit Bhargava wrote:
> BUG: unable to handle kernel paging request at 0000000000841f1f
> IP: [<ffffffff81014ec4>] uncore_change_context+0xd4/0x180
...
>  [<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];
> 
> pmu->boxes[pkg] is garbage because pkg was returned as 0xffff.

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

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

> [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.

> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 4296beb8fdd3..d1272febc13b 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -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;

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

Thanks,

	tglx

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Prarit Bhargava]

On 10/04/2016 06:58 AM, Thomas Gleixner wrote:
> On Mon, 3 Oct 2016, Prarit Bhargava wrote:
>> BUG: unable to handle kernel paging request at 0000000000841f1f
>> IP: [<ffffffff81014ec4>] uncore_change_context+0xd4/0x180
> ...
>>  [<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];
>>
>> pmu->boxes[pkg] is garbage because pkg was returned as 0xffff.
> 
> And that's what needs to be fixed in the first place.
> 
>> 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.

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

> 
>> [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.

> 
> 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
> understandable explanation why prefill_possible_map() is the right place to
> do this.

> 
>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>> index 4296beb8fdd3..d1272febc13b 100644
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -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;
> 
> 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().

P.

> 
> Thanks,
> 
> 	tglx
> 

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Prarit Bhargava]

On 10/04/2016 06:58 AM, Thomas Gleixner wrote:
> On Mon, 3 Oct 2016, Prarit Bhargava wrote:
>> BUG: unable to handle kernel paging request at 0000000000841f1f
>> IP: [<ffffffff81014ec4>] uncore_change_context+0xd4/0x180
> ...
>>  [<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];
>>
>> pmu->boxes[pkg] is garbage because pkg was returned as 0xffff.
> 
> And that's what needs to be fixed in the first place.
> 
>> 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.

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

> 
>> [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.

> 
> 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
> understandable explanation why prefill_possible_map() is the right place to
> do this.

> 
>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>> index 4296beb8fdd3..d1272febc13b 100644
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -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;
> 
> 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().

P.

> 
> Thanks,
> 
> 	tglx
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Jiri Olsa]

On Tue, Oct 04, 2016 at 12:58:04PM +0200, Thomas Gleixner wrote:
> On Mon, 3 Oct 2016, Prarit Bhargava wrote:
> > BUG: unable to handle kernel paging request at 0000000000841f1f
> > IP: [<ffffffff81014ec4>] uncore_change_context+0xd4/0x180
> ...
> >  [<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];
> > 
> > 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

> 
> > 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.
> 
> > [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.

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

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Jiri Olsa]

On Tue, Oct 04, 2016 at 12:58:04PM +0200, Thomas Gleixner wrote:
> On Mon, 3 Oct 2016, Prarit Bhargava wrote:
> > BUG: unable to handle kernel paging request at 0000000000841f1f
> > IP: [<ffffffff81014ec4>] uncore_change_context+0xd4/0x180
> ...
> >  [<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];
> > 
> > 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

> 
> > 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.
> 
> > [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.

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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Thomas Gleixner]

On Tue, 4 Oct 2016, Jiri Olsa wrote:
> On Tue, Oct 04, 2016 at 12:58:04PM +0200, Thomas Gleixner wrote:
> > > 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.
 
> 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

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Thomas Gleixner]

On Tue, 4 Oct 2016, Jiri Olsa wrote:
> On Tue, Oct 04, 2016 at 12:58:04PM +0200, Thomas Gleixner wrote:
> > > 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.
 
> 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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Thomas Gleixner]

On Tue, 4 Oct 2016, Prarit Bhargava wrote:
> On 10/04/2016 06:58 AM, Thomas Gleixner wrote:
> > 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.
 
> >> +		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

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Thomas Gleixner]

On Tue, 4 Oct 2016, Prarit Bhargava wrote:
> On 10/04/2016 06:58 AM, Thomas Gleixner wrote:
> > 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.
 
> >> +		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



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Prarit Bhargava]

On 10/04/2016 10:38 AM, Thomas Gleixner wrote:
> On Tue, 4 Oct 2016, Prarit Bhargava wrote:
>> On 10/04/2016 06:58 AM, Thomas Gleixner wrote:
>>> 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.

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).

> 
> 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()?

>  
>>>> +		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 ...
> 

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.

P.

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Prarit Bhargava]

On 10/04/2016 10:38 AM, Thomas Gleixner wrote:
> On Tue, 4 Oct 2016, Prarit Bhargava wrote:
>> On 10/04/2016 06:58 AM, Thomas Gleixner wrote:
>>> 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.

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).

> 
> 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()?

>  
>>>> +		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 ...
> 

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.

P.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Jiri Olsa]

On Tue, Oct 04, 2016 at 04:38:16PM +0200, Thomas Gleixner wrote:
> On Tue, 4 Oct 2016, Prarit Bhargava wrote:
> > On 10/04/2016 06:58 AM, Thomas Gleixner wrote:
> > > 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.

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

thanks,
jirka


---
diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
index 28865938aadf..61d087a2f25d 100644
--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/intel/rapl.c
@@ -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;
 }
 
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 463dc7a5a6c3..3f657fbbf787 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -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();

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Jiri Olsa]

On Tue, Oct 04, 2016 at 04:38:16PM +0200, Thomas Gleixner wrote:
> On Tue, 4 Oct 2016, Prarit Bhargava wrote:
> > On 10/04/2016 06:58 AM, Thomas Gleixner wrote:
> > > 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.

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

thanks,
jirka


---
diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
index 28865938aadf..61d087a2f25d 100644
--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/intel/rapl.c
@@ -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;
 }
 
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 463dc7a5a6c3..3f657fbbf787 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -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();

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Prarit Bhargava]

On 10/05/2016 12:14 PM, Jiri Olsa wrote:
> On Tue, Oct 04, 2016 at 04:38:16PM +0200, Thomas Gleixner wrote:
>> On Tue, 4 Oct 2016, Prarit Bhargava wrote:
>>> On 10/04/2016 06:58 AM, Thomas Gleixner wrote:
>>>> 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.
> 
> I still need to test this, but how about something like this?
> 
> thanks,
> jirka
> 
> 
> ---
> diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
> index 28865938aadf..61d087a2f25d 100644
> --- a/arch/x86/events/intel/rapl.c
> +++ b/arch/x86/events/intel/rapl.c
> @@ -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;

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.

P.

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Prarit Bhargava]

On 10/05/2016 12:14 PM, Jiri Olsa wrote:
> On Tue, Oct 04, 2016 at 04:38:16PM +0200, Thomas Gleixner wrote:
>> On Tue, 4 Oct 2016, Prarit Bhargava wrote:
>>> On 10/04/2016 06:58 AM, Thomas Gleixner wrote:
>>>> 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.
> 
> I still need to test this, but how about something like this?
> 
> thanks,
> jirka
> 
> 
> ---
> diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
> index 28865938aadf..61d087a2f25d 100644
> --- a/arch/x86/events/intel/rapl.c
> +++ b/arch/x86/events/intel/rapl.c
> @@ -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;

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.

P.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Jiri Olsa]

On Thu, Oct 06, 2016 at 11:25:43AM -0400, Prarit Bhargava wrote:
> 
> 
> On 10/05/2016 12:14 PM, Jiri Olsa wrote:
> > On Tue, Oct 04, 2016 at 04:38:16PM +0200, Thomas Gleixner wrote:
> >> On Tue, 4 Oct 2016, Prarit Bhargava wrote:
> >>> On 10/04/2016 06:58 AM, Thomas Gleixner wrote:
> >>>> 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.
> > 
> > I still need to test this, but how about something like this?
> > 
> > thanks,
> > jirka
> > 
> > 
> > ---
> > diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
> > index 28865938aadf..61d087a2f25d 100644
> > --- a/arch/x86/events/intel/rapl.c
> > +++ b/arch/x86/events/intel/rapl.c
> > @@ -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;
> 
> 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

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Jiri Olsa]

On Thu, Oct 06, 2016 at 11:25:43AM -0400, Prarit Bhargava wrote:
> 
> 
> On 10/05/2016 12:14 PM, Jiri Olsa wrote:
> > On Tue, Oct 04, 2016 at 04:38:16PM +0200, Thomas Gleixner wrote:
> >> On Tue, 4 Oct 2016, Prarit Bhargava wrote:
> >>> On 10/04/2016 06:58 AM, Thomas Gleixner wrote:
> >>>> 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.
> > 
> > I still need to test this, but how about something like this?
> > 
> > thanks,
> > jirka
> > 
> > 
> > ---
> > diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
> > index 28865938aadf..61d087a2f25d 100644
> > --- a/arch/x86/events/intel/rapl.c
> > +++ b/arch/x86/events/intel/rapl.c
> > @@ -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;
> 
> 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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Thomas Gleixner]

On Fri, 7 Oct 2016, Jiri Olsa wrote:
> On Thu, Oct 06, 2016 at 11:25:43AM -0400, Prarit Bhargava wrote:
> > 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

Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Thomas Gleixner]

On Fri, 7 Oct 2016, Jiri Olsa wrote:
> On Thu, Oct 06, 2016 at 11:25:43AM -0400, Prarit Bhargava wrote:
> > 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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[tip-bot for Prarit Bhargava]

Commit-ID:  2a51fe083eba7f99cbda72f5ef90cdf2f4df882c
Gitweb:     http://git.kernel.org/tip/2a51fe083eba7f99cbda72f5ef90cdf2f4df882c
Author:     Prarit Bhargava <prarit@redhat.com>
AuthorDate: Mon, 3 Oct 2016 13:07:12 -0400
Committer:  Thomas Gleixner <tglx@linutronix.de>
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 <prarit@redhat.com>
Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Len Brown <len.brown@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: dyoung@redhat.com
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: kexec@lists.infradead.org
Link: http://lkml.kernel.org/r/1475514432-27682-1-git-send-email-prarit@redhat.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

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

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 42a9362..951f093 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -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) {