Re: [PATCH v2] x86: efi: Turn off efi_enabled after setup on mixed fw/kernel

[Maarten Lankhorst <maarten.lankhorst@canonical.com>]

Op 24-10-12 08:24, Olof Johansson schreef:
> When 32-bit EFI is used with 64-bit kernel (or vice versa), turn off
> efi_enabled once setup is done. Beyond setup, it is normally used to
> determine if runtime services are available and we will have none.
>
> This will resolve issues stemming from efivars modprobe panicking on a
> 32/64-bit setup, as well as some reboot issues on similar setups.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=45991
>
> Reported-by: Marko Kohtala <marko.kohtala@gmail.com>
> Reported-by: Maxim Kammerer <mk@dee.su>
> Signed-off-by: Olof Johansson <olof@lixom.net>
> Cc: stable@kernel.org # 3.4 - 3.6
> Cc: Matthew Garrett <mjg@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> ---
>
> v2: rebase due to context diffs, and simplified efi_is_native() logic.
>
>  arch/x86/kernel/setup.c     | 11 +++++++++++
>  arch/x86/platform/efi/efi.c | 16 +++++++++-------
>  2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 468e98d..ea2c587 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1048,6 +1048,17 @@ void __init setup_arch(char **cmdline_p)
>  	arch_init_ideal_nops();
>  
>  	register_refined_jiffies(CLOCK_TICK_RATE);
> +
> +#ifdef CONFIG_EFI
> +	/* Once setup is done above, disable efi_enabled on mismatched
> +	 * firmware/kernel archtectures since there is no support for
> +	 * runtime services.
> +	 */
> +	if (IS_ENABLED(CONFIG_X86_64) != efi_64bit) {
> +		pr_info("efi: Setup done, disabling due to 32/64-bit mismatch\n");
> +		efi_enabled = 0;
> +	}
> +#endif
>  }

Won't this give a spurious warning if it's already disabled?

And it should probably be moved to before the vga con setup, else it seems you
might not get a vga console when efifb is not used. Unless that's intentional,
but in that case please change the commit message to reflect that. :-)

With those issues fixed in next version.

Acked-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>

>  
>  #ifdef CONFIG_X86_32
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index aded2a9..6e620f1 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -70,11 +70,15 @@ EXPORT_SYMBOL(efi);
>  struct efi_memory_map memmap;
>  
>  bool efi_64bit;
> -static bool efi_native;
>  
>  static struct efi efi_phys __initdata;
>  static efi_system_table_t efi_systab __initdata;
>  
> +static inline bool efi_is_native(void)
> +{
> +	return IS_ENABLED(CONFIG_X86_64) == efi_64bit;
> +}
> +
>  static int __init setup_noefi(char *arg)
>  {
>  	efi_enabled = 0;
> @@ -432,7 +436,7 @@ void __init efi_free_boot_services(void)
>  {
>  	void *p;
>  
> -	if (!efi_native)
> +	if (!efi_is_native())
>  		return;
>  
>  	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
> @@ -684,12 +688,10 @@ void __init efi_init(void)
>  		return;
>  	}
>  	efi_phys.systab = (efi_system_table_t *)boot_params.efi_info.efi_systab;
> -	efi_native = !efi_64bit;
>  #else
>  	efi_phys.systab = (efi_system_table_t *)
>  			  (boot_params.efi_info.efi_systab |
>  			  ((__u64)boot_params.efi_info.efi_systab_hi<<32));
> -	efi_native = efi_64bit;
>  #endif
>  
>  	if (efi_systab_init(efi_phys.systab)) {
> @@ -723,7 +725,7 @@ void __init efi_init(void)
>  	 * that doesn't match the kernel 32/64-bit mode.
>  	 */
>  
> -	if (!efi_native)
> +	if (!efi_is_native())
>  		pr_info("No EFI runtime due to 32/64-bit mismatch with kernel\n");
>  	else if (efi_runtime_init()) {
>  		efi_enabled = 0;
> @@ -735,7 +737,7 @@ void __init efi_init(void)
>  		return;
>  	}
>  #ifdef CONFIG_X86_32
> -	if (efi_native) {
> +	if (efi_is_native()) {
>  		x86_platform.get_wallclock = efi_get_time;
>  		x86_platform.set_wallclock = efi_set_rtc_mmss;
>  	}
> @@ -834,7 +836,7 @@ void __init efi_enter_virtual_mode(void)
>  	 * non-native EFI
>  	 */
>  
> -	if (!efi_native) {
> +	if (!efi_is_native()) {
>  		efi_unmap_memmap();
>  		return;
>  	}