linux-kernel - Re: [RESEND][PATCH v3] x86/apic/flat64: Add back the early_param("apic", parse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200713103140.xesdtvpczlfv6edb@liuwe-devbox-debian-v2>
Date:   Mon, 13 Jul 2020 10:31:40 +0000
From:   Wei Liu <wei.liu@...nel.org>
To:     Dexuan Cui <decui@...rosoft.com>
Cc:     tglx@...utronix.de, mingo@...hat.com, rdunlap@...radead.org,
        bp@...en8.de, hpa@...or.com, x86@...nel.org, peterz@...radead.org,
        allison@...utok.net, alexios.zavras@...el.com,
        gregkh@...uxfoundation.org, namit@...are.com,
        mikelley@...rosoft.com, longli@...rosoft.com,
        linux-kernel@...r.kernel.org, linux-hyperv@...r.kernel.org,
        Wei Liu <wei.liu@...nel.org>
Subject: Re: [RESEND][PATCH v3] x86/apic/flat64: Add back the
 early_param("apic", parse_apic)

On Fri, Jun 26, 2020 at 11:21:06AM -0700, Dexuan Cui wrote:
> 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@...rosoft.com>

[...]

> +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);

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.