x86/apic/flat64: Add back the early_param("apic", parse_apic)

PATCH RESEND In review Dexuan Cui 3 participants 4 messages Jun 26, 2020 – Sep 7, 2020

Subsystems (4)

Documentation x86 Hyper-V/Azure core and drivers Microsoft MANA RDMA driver

lore

Share & tools

lore

Dexuan Cui

author maintainer

2020-06-26 18:21 UTC #0 lore raw

PATCH RESEND v3 1/1 x86/apic/flat64: Add back the early_param("apic", parse_apic)

parse_apic() allows the user to try a different APIC driver than the default one that's automatically chosen. It works for X86-32, but doesn't work for X86-64 because it was removed in 2009 for X86-64 by commit 7b38725318f4 ("x86: remove subarchitecture support code"), whose changelog doesn't explicitly describe the removal for X86-64.

The patch adds back the functionality for X86-64. The intent is mainly to work around an APIC emulation bug in Hyper-V in the case of kdump:
currently Hyper-V does not honor the disabled state of the local APICs, so all the IOAPIC-based interrupts may not be delivered to the correct virtual CPU, if the logical-mode APIC driver is used (the kdump kernel usually uses the logical-mode APIC driver, since typically only 1 CPU is active). Luckily the kdump issue can be worked around by forcing the kdump kernel to use physical mode, before the fix to Hyper-V becomes widely available.

The current algorithm of choosing an APIC driver is:

1. The global pointer "struct apic *apic" has a default value, i.e "apic_default" on X86-32, and "apic_flat" on X86-64.

2. If the early_param "apic=" is specified, parse_apic() is called and the pointer "apic" is changed if a matching APIC driver is found.

3. default_acpi_madt_oem_check() calls the acpi_madt_oem_check() method of all APIC drivers, which may override the "apic" pointer.

4. default_setup_apic_routing() may override the "apic" pointer, e.g. by calling the probe() method of all APIC drivers. Note: refer to the order of the APIC drivers specified in arch/x86/kernel/apic/Makefile.

The patch is safe because if the apic= early param is not specified, the current algorithm of choosing an APIC driver is unchanged; when the param is specified (e.g. on X86-64, "apic=physical flat"), the kernel still tries to find a "more suitable" APIC driver in the above step 3 and 4: e.g. if the BIOS/firmware requires that apic_x2apic_phys should be used, the above step 4 will override the APIC driver to apic_x2apic_phys, even if an early_param "apic=physical flat" is specified.

On Hyper-V, when a Linux VM has <= 8 virtual CPUs, if we use "apic=physical flat", sending IPIs to multiple vCPUs is still fast because Linux VM uses the para-virtualized IPI hypercalls: see hv_apic_init().

The patch adds the __init tag for flat_acpi_madt_oem_check() and physflat_acpi_madt_oem_check() to avoid a warning seen with "make W=1":
flat_acpi_madt_oem_check() accesses cmdline_apic, which has a __initdata tag.

Fixes 7b38725318f4 ("x86: remove subarchitecture support code")

Signed-off-by Dexuan Cui <[email protected]>

---

Changes in v2 (5/28/2020):
Updated Documentation/admin-guide/kernel-parameters.txt. [Randy Dunlap] Changed apic_set_verbosity().
Enhanced the changelog.

Changes in v3 (5/31/2020):
Added the __init tag for flat_acpi_madt_oem_check() and
physflat_acpi_madt_oem_check() to avoid a warning seen with "make W=1".
(Thanks to kbuild test robot <[email protected]>).

Updated the changelog for the __init tag.

Today is 6/26/2020 and this is just a RESEND of v3, which was posted on 5/31 (https://lkml.org/lkml/2020/5/31/198).

 .../admin-guide/kernel-parameters.txt         | 11 +++++--
 arch/x86/kernel/apic/apic.c                   | 11 +++----
 arch/x86/kernel/apic/apic_flat_64.c           | 31 +++++++++++++++++--
 3 files changed, 40 insertions(+), 13 deletions(-)

Documentation/admin-guide/kernel-parameters.txt +8 -3

index 7bc83f3d9bdf..c4503fff9348 100644
@@ -341,10 +341,15 @@
 			Format: { quiet (default) | verbose | debug }
 			Change the amount of debugging information output
 			when initialising the APIC and IO-APIC components.
-			For X86-32, this can also be used to specify an APIC
-			driver name.
+			This can also be used to specify an APIC driver name.
 			Format: apic=driver_name
-			Examples: apic=bigsmp
+			Examples:
+			  On X86-32:  apic=bigsmp
+			  On X86-64: "apic=physical flat"
+			  Note: the available driver names depend on the
+			  architecture and the kernel config; the setting may
+			  be overridden by the acpi_madt_oem_check() and probe()
+			  methods of other APIC drivers.
 
 	apic_extnmi=	[APIC,X86] External NMI delivery setting
 			Format: { bsp (default) | all | none }

arch/x86/kernel/apic/apic.c +4 -7

index e53dda210cd7..6f7d75b6358b 100644
@@ -2855,13 +2855,10 @@ static int __init apic_set_verbosity(char *arg)
 		apic_verbosity = APIC_DEBUG;
 	else if (strcmp("verbose", arg) == 0)
 		apic_verbosity = APIC_VERBOSE;
-#ifdef CONFIG_X86_64
-	else {
-		pr_warn("APIC Verbosity level %s not recognised"
-			" use apic=verbose or apic=debug\n", arg);
-		return -EINVAL;
-	}
-#endif
+
+	/* Ignore unrecognized verbosity level setting. */
+
+	pr_info("APIC Verbosity level is %d\n", apic_verbosity);
 
 	return 0;
 }

arch/x86/kernel/apic/apic_flat_64.c +28 -3

index 7862b152a052..da8f3640453f 100644
@@ -23,9 +23,34 @@ static struct apic apic_flat;
 struct apic *apic __ro_after_init = &apic_flat;
 EXPORT_SYMBOL_GPL(apic);
 
-static int flat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static int cmdline_apic __initdata;
+static int __init parse_apic(char *arg)
 {
-	return 1;
+	struct apic **drv;
+
+	if (!arg)
+		return -EINVAL;
+
+	for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) {
+		if (!strcmp((*drv)->name, arg)) {
+			apic = *drv;
+			cmdline_apic = 1;
+			return 0;
+		}
+	}
+
+	/* Parsed again by __setup for debug/verbose */
+	return 0;
+}
+early_param("apic", parse_apic);
+
+
+static int __init flat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+{
+	if (!cmdline_apic)
+		return 1;
+
+	return apic == &apic_flat;
 }
 
 /*

@@ -157,7 +182,7 @@ static struct apic apic_flat __ro_after_init = {
  * We cannot use logical delivery in this case because the mask
  * overflows, so use physical mode.
  */
-static int physflat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static int __init physflat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
 #ifdef CONFIG_ACPI
 	/*

Signature · 1 line

2.19.1

Wei Liu

maintainer

2020-07-13 10:31 UTC #1 lore raw

Quoted 50 lines · Dexuan Cui

parse_apic() allows the user to try a different APIC driver than the default one that's automatically chosen. It works for X86-32, but doesn't work for X86-64 because it was removed in 2009 for X86-64 by commit 7b38725318f4 ("x86: remove subarchitecture support code"), whose changelog doesn't explicitly describe the removal for X86-64.
The patch adds back the functionality for X86-64. The intent is mainly to work around an APIC emulation bug in Hyper-V in the case of kdump:
currently Hyper-V does not honor the disabled state of the local APICs, so all the IOAPIC-based interrupts may not be delivered to the correct virtual CPU, if the logical-mode APIC driver is used (the kdump kernel usually uses the logical-mode APIC driver, since typically only 1 CPU is active). Luckily the kdump issue can be worked around by forcing the kdump kernel to use physical mode, before the fix to Hyper-V becomes widely available.
The current algorithm of choosing an APIC driver is:
1. The global pointer "struct apic *apic" has a default value, i.e "apic_default" on X86-32, and "apic_flat" on X86-64.
2. If the early_param "apic=" is specified, parse_apic() is called and the pointer "apic" is changed if a matching APIC driver is found.
3. default_acpi_madt_oem_check() calls the acpi_madt_oem_check() method of all APIC drivers, which may override the "apic" pointer.
4. default_setup_apic_routing() may override the "apic" pointer, e.g. by calling the probe() method of all APIC drivers. Note: refer to the order of the APIC drivers specified in arch/x86/kernel/apic/Makefile.
The patch is safe because if the apic= early param is not specified, the current algorithm of choosing an APIC driver is unchanged; when the param is specified (e.g. on X86-64, "apic=physical flat"), the kernel still tries to find a "more suitable" APIC driver in the above step 3 and 4: e.g. if the BIOS/firmware requires that apic_x2apic_phys should be used, the above step 4 will override the APIC driver to apic_x2apic_phys, even if an early_param "apic=physical flat" is specified.
On Hyper-V, when a Linux VM has <= 8 virtual CPUs, if we use "apic=physical flat", sending IPIs to multiple vCPUs is still fast because Linux VM uses the para-virtualized IPI hypercalls: see hv_apic_init().
The patch adds the __init tag for flat_acpi_madt_oem_check() and physflat_acpi_madt_oem_check() to avoid a warning seen with "make W=1":
flat_acpi_madt_oem_check() accesses cmdline_apic, which has a __initdata tag.
Fixes 7b38725318f4 ("x86: remove subarchitecture support code")
Signed-off-by Dexuan Cui <[email protected]>

[...]

Quoted 11 lines

arch/x86/kernel/apic/apic_flat_64.c +9 -1

+static int cmdline_apic __initdata;
+static int __init parse_apic(char *arg)
 {
-	return 1;
+	struct apic **drv;
+
+	if (!arg)
+		return -EINVAL;
+
+	for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) {
+		if (!strcmp((*drv)->name, arg)) {

arch/x86/kernel/apic/apic_flat_64.c static struct apic apic_flat; · line 45

Quoted 10 lines

arch/x86/kernel/apic/apic_flat_64.c +10 -0

+			apic = *drv;
+			cmdline_apic = 1;
+			return 0;
+		}
+	}
+
+	/* Parsed again by __setup for debug/verbose */
+	return 0;
+}
+early_param("apic", parse_apic);

This is a verbatim copy of the code in probe_32.c. Can you avoid the duplication by moving the snippet from probe_32.c to apic.c?

You can declare cmdline_apic in local.h if you do that.

Wei.

Thomas Gleixner

2020-07-17 13:03 UTC #2 lore raw

In reply to Dexuan Cui

Dexuan Cui <[email protected]> writes:

What has the early param to do with apic/flat64?

Quoted 5 lines

parse_apic() allows the user to try a different APIC driver than the default one that's automatically chosen. It works for X86-32, but doesn't work for X86-64 because it was removed in 2009 for X86-64 by commit 7b38725318f4 ("x86: remove subarchitecture support code"), whose changelog doesn't explicitly describe the removal for X86-64.

Well, the patches leading to this had the clear intent to simplify the whole APIC code and the removal of that command line override was part of that cleanup. Up to now nothing needed it at all.

Quoted 1 line

The patch adds back the functionality for X86-64. The intent is mainly

git grep 'This patch' Documentation/process

Quoted 8 lines

to work around an APIC emulation bug in Hyper-V in the case of kdump:
currently Hyper-V does not honor the disabled state of the local APICs, so all the IOAPIC-based interrupts may not be delivered to the correct virtual CPU, if the logical-mode APIC driver is used (the kdump kernel usually uses the logical-mode APIC driver, since typically only 1 CPU is active). Luckily the kdump issue can be worked around by forcing the kdump kernel to use physical mode, before the fix to Hyper-V becomes widely available.

This is just another proof that virtualization creates more problems than it solves.

Quoted 8 lines

The current algorithm of choosing an APIC driver is:
1. The global pointer "struct apic *apic" has a default value, i.e "apic_default" on X86-32, and "apic_flat" on X86-64.
2. If the early_param "apic=" is specified, parse_apic() is called and the pointer "apic" is changed if a matching APIC driver is found.

Quoted 6 lines

3. default_acpi_madt_oem_check() calls the acpi_madt_oem_check() method of all APIC drivers, which may override the "apic" pointer.
4. default_setup_apic_routing() may override the "apic" pointer, e.g. by calling the probe() method of all APIC drivers. Note: refer to the order of the APIC drivers specified in arch/x86/kernel/apic/Makefile.

What's the 'current algorithm'? #2 is not in use on 64bit currently.

And looking at 32bit then the above decription is wrong. Once a command line parameter is specified and matched then #3 and #4 do not override anything. See the probe code in 32 bit.

Quoted 3 lines

On Hyper-V, when a Linux VM has <= 8 virtual CPUs, if we use "apic=physical flat", sending IPIs to multiple vCPUs is still fast because Linux VM uses the para-virtualized IPI hypercalls: see hv_apic_init().

And this is relevant because?

Quoted 4 lines

The patch adds the __init tag for flat_acpi_madt_oem_check() and physflat_acpi_madt_oem_check() to avoid a warning seen with "make W=1":
flat_acpi_madt_oem_check() accesses cmdline_apic, which has a __initdata tag.

And both function have pointers stored in the corresponding apic structs, i.e. after init these pointers become dangling. No...

Aside of that the only function which accesses cmdline_apic is flat_acpi_madt_oem_check() according to the patch below.

Quoted 1 line

Fixes 7b38725318f4 ("x86: remove subarchitecture support code")

This fixes tag is blantantly wrong. You want to add:

Fixes Broken hypervisor

When this mess was removed hyperv still wore diapers.

Quoted 5 lines

(quoted diff) +0 -2

 			Format: { quiet (default) | verbose | debug }
 			Change the amount of debugging information output
 			when initialising the APIC and IO-APIC components.
-			For X86-32, this can also be used to specify an APIC
-			driver name.

Review

Quoted 10 lines

(quoted diff) +8 -1

+			This can also be used to specify an APIC driver name.
 			Format: apic=driver_name
-			Examples: apic=bigsmp
+			Examples:
+			  On X86-32:  apic=bigsmp
+			  On X86-64: "apic=physical flat"
+			  Note: the available driver names depend on the
+			  architecture and the kernel config; the setting may
+			  be overridden by the acpi_madt_oem_check() and probe()
+			  methods of other APIC drivers.

Which is wrong because that's not true for 32bit.

Quoted 5 lines

(quoted diff) +0 -1

@@ -2855,13 +2855,10 @@ static int __init apic_set_verbosity(char *arg)
 		apic_verbosity = APIC_DEBUG;
 	else if (strcmp("verbose", arg) == 0)
 		apic_verbosity = APIC_VERBOSE;
-#ifdef CONFIG_X86_64

Review static int __init apic_set_verbosity(char *arg) · line 2861

Quoted 10 lines

(quoted diff) +4 -6

-	else {
-		pr_warn("APIC Verbosity level %s not recognised"
-			" use apic=verbose or apic=debug\n", arg);
-		return -EINVAL;
-	}
-#endif
+
+	/* Ignore unrecognized verbosity level setting. */
+
+	pr_info("APIC Verbosity level is %d\n", apic_verbosity);

We need that printk on every boot because?

Quoted 23 lines

(quoted diff) +0 -0

 	return 0;
 }

arch/x86/kernel/apic/apic_flat_64.c +9 -2

index 7862b152a052..da8f3640453f 100644
@@ -23,9 +23,34 @@ static struct apic apic_flat;
 struct apic *apic __ro_after_init = &apic_flat;
 EXPORT_SYMBOL_GPL(apic);
 
-static int flat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static int cmdline_apic __initdata;
+static int __init parse_apic(char *arg)
 {
-	return 1;
+	struct apic **drv;
+
+	if (!arg)
+		return -EINVAL;
+
+	for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) {
+		if (!strcmp((*drv)->name, arg)) {

arch/x86/kernel/apic/apic_flat_64.c static struct apic apic_flat; · line 45

Quoted 10 lines

arch/x86/kernel/apic/apic_flat_64.c +10 -0

+			apic = *drv;
+			cmdline_apic = 1;
+			return 0;
+		}
+	}
+
+	/* Parsed again by __setup for debug/verbose */
+	return 0;
+}
+early_param("apic", parse_apic);

We surely need yet another copy of the code in probe_32.c

arch/x86/kernel/apic/apic_flat_64.c static struct apic apic_flat; · line 51

Quoted 6 lines

arch/x86/kernel/apic/apic_flat_64.c +6 -0

+static int __init flat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+{
+	if (!cmdline_apic)
+		return 1;
+
+	return apic == &apic_flat;

Comments are overrated...

arch/x86/kernel/apic/apic_flat_64.c static struct apic apic_flat __ro_after_init = { · line 186

Quoted 10 lines

arch/x86/kernel/apic/apic_flat_64.c +1 -1

 }
 
 /*

@@ -157,7 +182,7 @@ static struct apic apic_flat __ro_after_init = {
  * We cannot use logical delivery in this case because the mask
  * overflows, so use physical mode.
  */
-static int physflat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static int __init physflat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {

See above ...

But why do you need all this if it is only relevant for a crash dump kernel? Can't this be simply cured by using 'nolapic' on the kernel command line?

Even if you must have the local apic for whatever reason in the kdump kernel then this can be completely done w/o command line hackery simply because its only required for the mshypervers + kdump case.

Thanks,

tglx
---

arch/x86/kernel/apic/apic_flat_64.c +7 -0

@@ -176,6 +176,13 @@ static int physflat_acpi_madt_oem_check(
 		return 1;
 	}
 #endif
+	/*
+	 * Workaround for a bug in the MS hypervisor which fails to reset
+	 * the APIC properly when a kernel crash jumps into the kdump
+	 * kernel.
+	 */
+	if (hypervisor_is_type(X86_HYPER_MS_HYPERV) && is_kdump_kernel())
+		return 1;
 
 	return 0;
 }

Dexuan Cui

author maintainer

2020-09-07 06:31 UTC #3 lore raw

Quoted 3 lines

From: Thomas Gleixner <[email protected]>
Sent: Friday, July 17, 2020 6:03 AM
[...]

Hi, I'm very sorry for this extremely late reply -- I was sidetracked by something else and I just had a chance to revisit the issue. Thank you tglx for the comments and suggestions, which I think are reasonable. I realized this patch is problematic. The good news is that the LAPIC emulation bug has been fixed in the latest version of Hyper-V and now kdump can work reliably. I think the hypervisor fix would be backported to old Hyper-V versions, so hopefully this won't be an issue over time.

Thanks,
-- Dexuan