RE: [PATCH v3] x86/apic/flat64: Add back the early_param("apic", parse_apic)

[Dexuan Cui <decui@microsoft.com>]

> From: Dexuan Cui <decui@microsoft.com>
> Sent: Sunday, May 31, 2020 9:49 AM
> To: tglx@linutronix.de; mingo@redhat.com; rdunlap@infradead.org;
> bp@alien8.de; hpa@zytor.com; x86@kernel.org; peterz@infradead.org;
> allison@lohutok.net; alexios.zavras@intel.com; gregkh@linuxfoundation.org;
> Dexuan Cui <decui@microsoft.com>; namit@vmware.com; Michael Kelley
> <mikelley@microsoft.com>; Long Li <longli@microsoft.com>
> Cc: linux-kernel@vger.kernel.org; linux-hyperv@vger.kernel.org
> Subject: [PATCH v3] 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 <decui@microsoft.com>
> ---
> 
> Changes in v2:
>   Updated Documentation/admin-guide/kernel-parameters.txt. [Randy
> Dunlap]
>   Changed apic_set_verbosity().
>   Enhanced the changelog.
> 
> Changes in v3:
>   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 <lkp@intel.com>).
> 
>   Updated the changelog for the __init tag.
> 
>  .../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(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index 7bc83f3d9bdf..c4503fff9348 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -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 }
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index e53dda210cd7..6f7d75b6358b 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -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;
>  }
> diff --git a/arch/x86/kernel/apic/apic_flat_64.c
> b/arch/x86/kernel/apic/apic_flat_64.c
> index 7862b152a052..da8f3640453f 100644
> --- a/arch/x86/kernel/apic/apic_flat_64.c
> +++ b/arch/x86/kernel/apic/apic_flat_64.c
> @@ -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
>  	/*
> --

Hi tglx and all,
Since v5.8-rc1 has been out, I guess you may have some cycles to
take a look at this patch?

Thanks,
-- Dexuan