Hello,
This patch deals with the issue of handling boot_cpu_physical_apicid
in boot process I avoided in disable_cpu_apicid patch because I
cannot guess how long it needs to take for the review of this fix.
This patch is made on top of today's x86/apic branch of tip tree.
Its commit hash is 5b4d1dbc24bb6fd7179ada0f47be34e27e64decb
I tested this patch in each combination of the following table:
BIOS table boot cpu
-------------+--------------
ACPI MADT AP
MP Table BSP
>From 10946038252fc91f3ae2740953b2484ea0076ed4 Mon Sep 17 00:00:00 2001
From: HATAYAMA Daisuke <[email protected]>
Date: Thu, 16 Jan 2014 15:47:25 +0900
Subject: [PATCH] x86, apic: clean up handling of boot_cpu_physical_apicid in
boot process
boot_cpu_physical_apicid variable is intended to have the apicid of
the processor that is doing the boot up by read_apic_id() that picks
up the value from APIC_ID register of local APIC.
boot_cpu_physical_apicid variable could be initialized with the value
from MP table if there's no acpi support on the system or acpi=off is
manually specified.
The first issue is that if kexec boots the 2nd kernel on some AP, the
processor that is doing the boot up of the 2nd kernel is no longer
BSP. This means that boot_cpu_physical_apicid is modified to
unintended BSP's apicid.
The second issue is that in the current code, once the
boot_cpu_physical_apicid variable is modified by the value from MP
table, it is again modified back to read_apic_id().
Concretely, they are processed as follows in setup_arch():
1) boot_cpu_physical_apicid is first initialized by read_apic_id()
in acpi_boot_init(), then
2) re-initialized by get_smp_config() and finally
3) modified back by read_apic_id() in init_apic_mappings().
In source code:
/*
* Read APIC and some other early information from ACPI tables.
*/
acpi_boot_init(); <== 1)
sfi_init();
x86_dtb_init();
/*
* get boot-time SMP configuration:
*/
if (smp_found_config)
get_smp_config(); <== 2)
prefill_possible_map();
init_cpu_to_node();
init_apic_mappings(); <== 3)
It is confusing that meaning of the variable changes in the middle of
boot up.
To fix these issues, this patch adds bios_bsp_physical_apicid variable
to store the apicid value reported by BIOS via ACPI MADT or MP table,
by which boot_cpu_physical_apicid variable is cleanly handled in boot
up.
This patch also fixes the workaround done in generic_processor_info()
that directly calls read_apic_id() to avoid the first issue.
Signed-off-by: HATAYAMA Daisuke <[email protected]>
---
arch/x86/include/asm/mpspec.h | 1 +
arch/x86/kernel/acpi/boot.c | 8 ++++++++
arch/x86/kernel/apic/apic.c | 31 ++++++++++---------------------
arch/x86/kernel/mpparse.c | 2 +-
arch/x86/platform/visws/visws_quirks.c | 2 +-
5 files changed, 21 insertions(+), 23 deletions(-)
diff --git a/arch/x86/include/asm/mpspec.h b/arch/x86/include/asm/mpspec.h
index 3142a94..724853f 100644
--- a/arch/x86/include/asm/mpspec.h
+++ b/arch/x86/include/asm/mpspec.h
@@ -47,6 +47,7 @@ extern int mp_bus_id_to_type[MAX_MP_BUSSES];
extern DECLARE_BITMAP(mp_bus_not_pci, MAX_MP_BUSSES);
extern unsigned int boot_cpu_physical_apicid;
+extern unsigned int bios_bsp_physical_apicid;
extern unsigned int max_physical_apicid;
extern int mpc_default_type;
extern unsigned long mp_lapic_addr;
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 6c0b43b..4f460e2 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -213,6 +213,14 @@ static int acpi_register_lapic(int id, u8 enabled)
if (boot_cpu_physical_apicid != -1U)
ver = apic_version[boot_cpu_physical_apicid];
+ /*
+ * ACPI specification describes that platform firmware should
+ * list the BSP processor as the first LAPIC entry in the
+ * MADT.
+ */
+ if (!num_processors && !disabled_cpus)
+ bios_bsp_physical_apicid = id;
+
return generic_processor_info(id, ver);
}
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 7f26c9a..107614f 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -64,6 +64,15 @@ unsigned disabled_cpus;
unsigned int boot_cpu_physical_apicid = -1U;
EXPORT_SYMBOL_GPL(boot_cpu_physical_apicid);
+/* Processor with BP flag on IP32_APIC_BASE MSR to be initialized with
+ * the value reported by BIOS tables such as ACPI MADT or MP table.
+ *
+ * If kexec boots the 2nd kernel on some AP, this differs from the
+ * processor that is doing the boot up: boot_cpu_physical_apicid !=
+ * bios_bsp_physical_apicid.
+ */
+unsigned int bios_bsp_physical_apicid = BAD_APICID;
+
/*
* The highest APIC ID seen during enumeration.
*/
@@ -2120,28 +2129,8 @@ int generic_processor_info(int apicid, int version)
bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
phys_cpu_present_map);
- /*
- * boot_cpu_physical_apicid is designed to have the apicid
- * returned by read_apic_id(), i.e, the apicid of the
- * currently booting-up processor. However, on some platforms,
- * it is temporarily modified by the apicid reported as BSP
- * through MP table. Concretely:
- *
- * - arch/x86/kernel/mpparse.c: MP_processor_info()
- * - arch/x86/mm/amdtopology.c: amd_numa_init()
- * - arch/x86/platform/visws/visws_quirks.c: MP_processor_info()
- *
- * This function is executed with the modified
- * boot_cpu_physical_apicid. So, disabled_cpu_apicid kernel
- * parameter doesn't work to disable APs on kdump 2nd kernel.
- *
- * Since fixing handling of boot_cpu_physical_apicid requires
- * another discussion and tests on each platform, we leave it
- * for now and here we use read_apic_id() directly in this
- * function, generic_processor_info().
- */
if (disabled_cpu_apicid != BAD_APICID &&
- disabled_cpu_apicid != read_apic_id() &&
+ disabled_cpu_apicid != boot_cpu_physical_apicid &&
disabled_cpu_apicid == apicid) {
int thiscpu = num_processors + disabled_cpus;
diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
index d2b5648..5bb5220 100644
--- a/arch/x86/kernel/mpparse.c
+++ b/arch/x86/kernel/mpparse.c
@@ -64,7 +64,7 @@ static void __init MP_processor_info(struct mpc_cpu *m)
if (m->cpuflag & CPU_BOOTPROCESSOR) {
bootup_cpu = " (Bootup-CPU)";
- boot_cpu_physical_apicid = m->apicid;
+ bios_bsp_physical_apicid = m->apicid;
}
printk(KERN_INFO "Processor #%d%s\n", m->apicid, bootup_cpu);
diff --git a/arch/x86/platform/visws/visws_quirks.c b/arch/x86/platform/visws/visws_quirks.c
index 94d8a39..069ed83 100644
--- a/arch/x86/platform/visws/visws_quirks.c
+++ b/arch/x86/platform/visws/visws_quirks.c
@@ -166,7 +166,7 @@ static void __init MP_processor_info(struct mpc_cpu *m)
(m->cpufeature & CPU_MODEL_MASK) >> 4, m->apicver);
if (m->cpuflag & CPU_BOOTPROCESSOR)
- boot_cpu_physical_apicid = m->apicid;
+ bios_cpu_physical_apicid = m->apicid;
ver = m->apicver;
if ((ver >= 0x14 && m->apicid >= 0xff) || m->apicid >= 0xf) {
--
1.8.4.2
--
Thanks.
HATAYAMA, Daisuke
On Thu, 16 Jan 2014, HATAYAMA Daisuke wrote:
> Hello,
>
> This patch deals with the issue of handling boot_cpu_physical_apicid
> in boot process I avoided in disable_cpu_apicid patch because I
> cannot guess how long it needs to take for the review of this fix.
>
> This patch is made on top of today's x86/apic branch of tip tree.
> Its commit hash is 5b4d1dbc24bb6fd7179ada0f47be34e27e64decb
>
This breaks with SGI 320/540:
arch/x86/platform/visws/visws_quirks.c: In function ‘MP_processor_info’:
arch/x86/platform/visws/visws_quirks.c:169:3: error: ‘bios_cpu_physical_apicid’ undeclared (first use in this function)
arch/x86/platform/visws/visws_quirks.c:169:3: note: each undeclared identifier is reported only once for each function it appears in
It makes it pretty apparent that you want a different name for the new
variable, bios_bsp_physical_apicid is just too close to
boot_cpu_physical_apicid that even the author of the patch missed a
conversion.
(It's also really sad that we can't make it __initdata since it's only
valid at boot because of the hotplug stuff.)
(2014/01/26 13:02), David Rientjes wrote:
> On Thu, 16 Jan 2014, HATAYAMA Daisuke wrote:
>
>> Hello,
>>
>> This patch deals with the issue of handling boot_cpu_physical_apicid
>> in boot process I avoided in disable_cpu_apicid patch because I
>> cannot guess how long it needs to take for the review of this fix.
>>
>> This patch is made on top of today's x86/apic branch of tip tree.
>> Its commit hash is 5b4d1dbc24bb6fd7179ada0f47be34e27e64decb
>>
>
> This breaks with SGI 320/540:
>
> arch/x86/platform/visws/visws_quirks.c: In function ‘MP_processor_info’:
> arch/x86/platform/visws/visws_quirks.c:169:3: error: ‘bios_cpu_physical_apicid’ undeclared (first use in this function)
> arch/x86/platform/visws/visws_quirks.c:169:3: note: each undeclared identifier is reported only once for each function it appears in
>
Thanks. I'll retry with CONFIG_X86_VISWS...
obj-$(CONFIG_X86_VISWS) += visws_quirks.o
> It makes it pretty apparent that you want a different name for the new
> variable, bios_bsp_physical_apicid is just too close to
> boot_cpu_physical_apicid that even the author of the patch missed a
> conversion.
>
``bsp'' and ``boot cpu'' are different, but certainly they are close
for most of people who don't need to focus the difference.
One idea is not to add bios_bsp_physical_apicid, just removing
assignment to boot_cpu_physical_apicid from MP_processor_info().
I guess currently no one uses apicid provided by MP table
since boot_cpu_physical_apicid is finally initialized in
init_apic_mappings().
> (It's also really sad that we can't make it __initdata since it's only
> valid at boot because of the hotplug stuff.)
>
--
Thanks.
HATAYAMA, Daisuke
On Mon, 27 Jan 2014, HATAYAMA Daisuke wrote:
> One idea is not to add bios_bsp_physical_apicid, just removing
> assignment to boot_cpu_physical_apicid from MP_processor_info().
> I guess currently no one uses apicid provided by MP table
> since boot_cpu_physical_apicid is finally initialized in
> init_apic_mappings().
>
Yeah, following generic_processor_info() it looks like it can be dropped.
On Mon, Jan 27, 2014 at 11:55:09AM +0900, HATAYAMA Daisuke wrote:
[..]
> ``bsp'' and ``boot cpu'' are different, but certainly they are close
> for most of people who don't need to focus the difference.
I agree. Especially this difference is becoming visible in kdump
context where we skip BIOS and can trigger a fresh boot on an AP.
Hatayama, I think distinguishing BSP and boot cpu makes sense and
we also need to find a way to export this information to user
space. So that we can figure out who is BSP and tell second kernel
not to bring up that cpu.
Initially we thought that cpu with "initial apicid" 0 is BSP. But
Jerry from HP is reporting that on some of the machines he has, BSP
does not have to have apic id 0.
If that's the case, we don't have a reliable way to figure out which is
BSP in the system. Or am I missing something?
Thanks
Vivek
On Thu, Jan 16, 2014 at 06:40:00PM +0900, HATAYAMA Daisuke wrote:
[..]
> diff --git a/arch/x86/include/asm/mpspec.h b/arch/x86/include/asm/mpspec.h
> index 3142a94..724853f 100644
> --- a/arch/x86/include/asm/mpspec.h
> +++ b/arch/x86/include/asm/mpspec.h
> @@ -47,6 +47,7 @@ extern int mp_bus_id_to_type[MAX_MP_BUSSES];
> extern DECLARE_BITMAP(mp_bus_not_pci, MAX_MP_BUSSES);
>
> extern unsigned int boot_cpu_physical_apicid;
> +extern unsigned int bios_bsp_physical_apicid;
I think put some comments here to explain clearly what is
boot_cpu_physical_apicid and what is bios_bsp_physical_apicid and what's
the difference. So that anybody making use of these variables or changing
code understands the difference better.
Thanks
Vivek
On Wed, Feb 05, 2014 at 11:38:24AM -0500, Vivek Goyal wrote:
[..]
> Initially we thought that cpu with "initial apicid" 0 is BSP. But
> Jerry from HP is reporting that on some of the machines he has, BSP
> does not have to have apic id 0.
>
> If that's the case, we don't have a reliable way to figure out which is
> BSP in the system. Or am I missing something?
Or, can I look at "processor" field in /proc/cpuinfo. If this is 0, will
this always mean it represents the cpu on which kernel booted.
If yes, then in first kernel one can safely assume that boot cpu is bios
designated BSP also. And kdump scripts should be able to automate that.
Though this assumption can be broken in second kernel but as of today
nobody needs to know bios designated BSP in second kernel.
Thanks
Vivek