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

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

[Olof Johansson]

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.

Signed-off-by: Olof Johansson <olof@lixom.net>
Cc: stable@kernel.org # 3.4 and 3.5
Cc: Matt Fleming <matt.fleming@intel.com>
Cc: Matthew Garrett <mjg@redhat.com>
---
 arch/x86/kernel/setup.c     | 11 +++++++++++
 arch/x86/platform/efi/efi.c | 14 ++++++++------
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f4b9b80..dad38ac 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1034,6 +1034,17 @@ void __init setup_arch(char **cmdline_p)
 	mcheck_init();
 
 	arch_init_ideal_nops();
+
+#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
 }
 
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 2dc29f5..17d7d50 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -69,11 +69,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;
@@ -650,12 +654,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)) {
@@ -689,7 +691,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;
@@ -700,7 +702,7 @@ void __init efi_init(void)
 		efi_enabled = 0;
 		return;
 	}
-	if (efi_native) {
+	if (efi_is_native()) {
 		x86_platform.get_wallclock = efi_get_time;
 		x86_platform.set_wallclock = efi_set_rtc_mmss;
 	}
@@ -765,7 +767,7 @@ void __init efi_enter_virtual_mode(void)
 	 * non-native EFI
 	 */
 
-	if (!efi_native)
+	if (!efi_is_native())
 		goto out;
 
 	/* Merge contiguous regions of the same type and attribute */
-- 
1.7.10.1.488.g05fbf7a

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

[Matt Fleming]

On Sun, 2012-08-19 at 14:48 -0700, Olof Johansson wrote:
> 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.
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> Cc: stable@kernel.org # 3.4 and 3.5
> Cc: Matt Fleming <matt.fleming@intel.com>
> Cc: Matthew Garrett <mjg@redhat.com>
> ---
>  arch/x86/kernel/setup.c     | 11 +++++++++++
>  arch/x86/platform/efi/efi.c | 14 ++++++++------
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index f4b9b80..dad38ac 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1034,6 +1034,17 @@ void __init setup_arch(char **cmdline_p)
>  	mcheck_init();
>  
>  	arch_init_ideal_nops();
> +
> +#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
>  }

Is there a reason we can't just disable efi_enabled at the end of
efi_init() if we're non-native, rather than having this chunk of code
here?

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

[Maarten Lankhorst]

Hey,

Op 19-08-12 23:48, 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.
>
> Signed-off-by: Olof Johansson <olof@lixom.net>
> Cc: stable@kernel.org # 3.4 and 3.5
> Cc: Matt Fleming <matt.fleming@intel.com>
> Cc: Matthew Garrett <mjg@redhat.com>
> ---
>  arch/x86/kernel/setup.c     | 11 +++++++++++
>  arch/x86/platform/efi/efi.c | 14 ++++++++------
>  2 files changed, 19 insertions(+), 6 deletions(-)
> <snip>
>  
> +static inline bool efi_is_native(void)
> +{
> +	return !(IS_ENABLED(CONFIG_X86_64) ^ efi_64bit);
> +}
>
Isn't this just a more complicated way of writing
IS_ENABLED(CONFIG_X86_64) == efi_64bit ?

Also moving the assignment to efi_init will make it no longer call
efi_reserve_boot_services, I don't know if that is intentional or not,
but something to consider at least since it's a behavioral change.

>From a quick glance with some grepping, efi reboot and efifb will
also no longer work, is that intentional?

~Maarten

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

[Olof Johansson]

On Mon, Aug 20, 2012 at 3:13 AM, Maarten Lankhorst
<maarten.lankhorst@canonical.com> wrote:
> Hey,
>
> Op 19-08-12 23:48, 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.
>>
>> Signed-off-by: Olof Johansson <olof@lixom.net>
>> Cc: stable@kernel.org # 3.4 and 3.5
>> Cc: Matt Fleming <matt.fleming@intel.com>
>> Cc: Matthew Garrett <mjg@redhat.com>
>> ---
>>  arch/x86/kernel/setup.c     | 11 +++++++++++
>>  arch/x86/platform/efi/efi.c | 14 ++++++++------
>>  2 files changed, 19 insertions(+), 6 deletions(-)
>> <snip>
>>
>> +static inline bool efi_is_native(void)
>> +{
>> +     return !(IS_ENABLED(CONFIG_X86_64) ^ efi_64bit);
>> +}
>>
> Isn't this just a more complicated way of writing
> IS_ENABLED(CONFIG_X86_64) == efi_64bit ?

Ah yes, of course.

> Also moving the assignment to efi_init will make it no longer call
> efi_reserve_boot_services, I don't know if that is intentional or not,
> but something to consider at least since it's a behavioral change.

That's why I kept it in setup.c instead of masking off at the end of
efi_init(). Either way works for me since I don't have firmware that
uses boot services at all, but I didn't want to risk regressing anyone
else.

> From a quick glance with some grepping, efi reboot and efifb will
> also no longer work, is that intentional?

That's the very point of this patch, the EFI services won't work since
there are no runtime services in this state, just boot time setup. If
efi_enabled is left on, the reboot will panic.


-Olof

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

[Matt Fleming]

On Mon, 2012-08-20 at 14:59 -0700, Olof Johansson wrote:
> > From a quick glance with some grepping, efi reboot and efifb will
> > also no longer work, is that intentional?
> 
> That's the very point of this patch, the EFI services won't work since
> there are no runtime services in this state, just boot time setup. If
> efi_enabled is left on, the reboot will panic.

But efifb should still work without EFI runtime services, no? I see this
in setup_arch(),

#ifdef CONFIG_VT                                                                
#if defined(CONFIG_VGA_CONSOLE)                                                 
        if (!efi_enabled || (efi_mem_type(0xa0000) != EFI_CONVENTIONAL_MEMORY)) 
                conswitchp = &vga_con;                                          
#elif defined(CONFIG_DUMMY_CONSOLE)                                             
        conswitchp = &dummy_con;                                                
#endif                                                                          
#endif                                   

but efi_enabled check looks bogus now that efi_enabled has come to mean
"EFI services available?". If we've been passed the dimensions of the
EFI framebuffer I'm unaware of a reason we can't use it.

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

[H. Peter Anvin]

On 08/21/2012 07:39 AM, Matt Fleming wrote:
> On Mon, 2012-08-20 at 14:59 -0700, Olof Johansson wrote:
>>>  From a quick glance with some grepping, efi reboot and efifb will
>>> also no longer work, is that intentional?
>>
>> That's the very point of this patch, the EFI services won't work since
>> there are no runtime services in this state, just boot time setup. If
>> efi_enabled is left on, the reboot will panic.
>
> But efifb should still work without EFI runtime services, no? I see this
> in setup_arch(),
>
> #ifdef CONFIG_VT
> #if defined(CONFIG_VGA_CONSOLE)
>          if (!efi_enabled || (efi_mem_type(0xa0000) != EFI_CONVENTIONAL_MEMORY))
>                  conswitchp = &vga_con;
> #elif defined(CONFIG_DUMMY_CONSOLE)
>          conswitchp = &dummy_con;
> #endif
> #endif
>
> but efi_enabled check looks bogus now that efi_enabled has come to mean
> "EFI services available?". If we've been passed the dimensions of the
> EFI framebuffer I'm unaware of a reason we can't use it.
>

Yes, this should be conditional on the parameters being available. 
However, efi_mem_type() is probably also ill-defined in this case.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

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

[Matt Fleming]

On Sun, 2012-08-19 at 14:48 -0700, Olof Johansson wrote:
> 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.
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> Cc: stable@kernel.org # 3.4 and 3.5
> Cc: Matt Fleming <matt.fleming@intel.com>
> Cc: Matthew Garrett <mjg@redhat.com>
> ---
>  arch/x86/kernel/setup.c     | 11 +++++++++++
>  arch/x86/platform/efi/efi.c | 14 ++++++++------
>  2 files changed, 19 insertions(+), 6 deletions(-)

Olof,

Would you mind resubmitting this patch and addressing Maarten's comment
about efi_is_native()?

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Olof Johansson]

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
 }
 
 #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;
 	}
-- 
1.7.10.1.488.g05fbf7a

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

[Maarten Lankhorst]

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;
>  	}

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

[Olof Johansson]

Hi,

On Wed, Oct 24, 2012 at 1:40 AM, Maarten Lankhorst
<maarten.lankhorst@canonical.com> wrote:
> 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?

Ah, of course, my bad. v3 forthcoming.

> 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. :-)

Hmm. I think the current logic and flow is valid -- it's likely a bad
idea to enable VGA console if the memory isn't set aside in the memory
map, since that possibly means that EFI didn't POST graphics for VGA
text mode in the first place.

> With those issues fixed in next version.
>
> Acked-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>

Please confirm if you agree with my reasoning above, I'll post v3
right after if so.

Thanks,

-Olof

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

[Maarten Lankhorst]

Op 24-10-12 17:21, Olof Johansson schreef:
> Hi,
>
> On Wed, Oct 24, 2012 at 1:40 AM, Maarten Lankhorst
> <maarten.lankhorst@canonical.com> wrote:
>> 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?
> Ah, of course, my bad. v3 forthcoming.
>
>> 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. :-)
> Hmm. I think the current logic and flow is valid -- it's likely a bad
> idea to enable VGA console if the memory isn't set aside in the memory
> map, since that possibly means that EFI didn't POST graphics for VGA
> text mode in the first place.
>
>> With those issues fixed in next version.
>>
>> Acked-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> Please confirm if you agree with my reasoning above, I'll post v3
> right after if so.
>
Ack

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

[Olof Johansson]

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>
Acked-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Cc: stable@kernel.org # 3.4 - 3.6
Cc: Matthew Garrett <mjg@redhat.com>
---

v3: Don't print disable warning if EFI was never enabled in the first place
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..745d68f 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 (efi_enabled && IS_ENABLED(CONFIG_X86_64) != efi_64bit) {
+		pr_info("efi: Setup done, disabling due to 32/64-bit mismatch\n");
+		efi_enabled = 0;
+	}
+#endif
 }
 
 #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;
 	}
-- 
1.7.10.1.488.g05fbf7a

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

[Matt Fleming]

On Wed, 2012-10-24 at 10:00 -0700, Olof Johansson wrote:
> 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>
> Acked-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> Cc: stable@kernel.org # 3.4 - 3.6
> Cc: Matthew Garrett <mjg@redhat.com>

Applied, thanks!

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Matt Fleming]

On Wed, 2012-10-24 at 10:00 -0700, Olof Johansson wrote:
> 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>
> Acked-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> Cc: stable@kernel.org # 3.4 - 3.6
> Cc: Matthew Garrett <mjg@redhat.com>
> ---

Running with this patch results in the following,

[    3.486943] ------------[ cut here ]------------
[    3.488455] WARNING: at /home/matt/src/kernels/linux-2.6/arch/x86/mm/ioremap.c:586 check_early_ioremap_leak+0x3b/0x50()
[    3.490043] Hardware name: MacBook2,1
[    3.491600] Debug warning: early ioremap leak of 1 areas detected.
[    3.493156] Modules linked in:
[    3.493973] usb 1-4: new high-speed USB device number 3 using ehci_hcd
[    3.496340] Pid: 1, comm: swapper/0 Not tainted 3.7.0-rc1+ #26
[    3.497949] Call Trace:
[    3.499558]  [<ffffffff81d022ab>] ? check_early_ioremap_leak+0x3b/0x50
[    3.501217]  [<ffffffff81032970>] warn_slowpath_common+0x85/0x9d
[    3.502872]  [<ffffffff81d02270>] ? early_ioremap_debug_setup+0x12/0x12
[    3.504516]  [<ffffffff81032a2b>] warn_slowpath_fmt+0x46/0x48
[    3.506161]  [<ffffffff81cf89df>] ? mcheck_debugfs_init+0x3b/0x3b
[    3.507790]  [<ffffffff81d022ab>] check_early_ioremap_leak+0x3b/0x50
[    3.509418]  [<ffffffff8100023b>] do_one_initcall+0x7f/0x134
[    3.511040]  [<ffffffff8163cc3c>] kernel_init+0x10c/0x275
[    3.512638]  [<ffffffff81cef4c7>] ? loglevel+0x31/0x31
[    3.514241]  [<ffffffff8163cb30>] ? rest_init+0x74/0x74
[    3.515824]  [<ffffffff8165ca9c>] ret_from_fork+0x7c/0xb0
[    3.517393]  [<ffffffff8163cb30>] ? rest_init+0x74/0x74
[    3.518958] ---[ end trace afc7d1c216dac6e1 ]---
[    3.520479] please boot with early_ioremap_debug and report the dmesg.

because we're now leaking the mapping from efi_memmap_init(). 

Maybe it should be handled like so?

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index c9dcc18..029189d 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -98,6 +98,7 @@ extern void efi_set_executable(efi_memory_desc_t *md, bool executable);
 extern int efi_memblock_x86_reserve_range(void);
 extern void efi_call_phys_prelog(void);
 extern void efi_call_phys_epilog(void);
+extern void efi_unmap_memmap(void);
 
 #ifndef CONFIG_EFI
 /*
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 392dae3..6fd7752 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1043,6 +1043,7 @@ void __init setup_arch(char **cmdline_p)
 	 */
 	if (efi_enabled && IS_ENABLED(CONFIG_X86_64) != efi_64bit) {
 		pr_info("efi: Setup done, disabling due to 32/64-bit mismatch\n");
+		efi_unmap_memmap();
 		efi_enabled = 0;
 	}
 #endif
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 6e620f1..9bae3b7 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -424,7 +424,7 @@ void __init efi_reserve_boot_services(void)
 	}
 }
 
-static void __init efi_unmap_memmap(void)
+void __init efi_unmap_memmap(void)
 {
 	if (memmap.map) {
 		early_iounmap(memmap.map, memmap.nr_map * memmap.desc_size);


-- 
Matt Fleming, Intel Open Source Technology Center

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

[Olof Johansson]

On Thu, Oct 25, 2012 at 6:20 AM, Matt Fleming <matt@console-pimps.org> wrote:
> On Wed, 2012-10-24 at 10:00 -0700, Olof Johansson wrote:
>> 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>
>> Acked-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
>> Cc: stable@kernel.org # 3.4 - 3.6
>> Cc: Matthew Garrett <mjg@redhat.com>
>> ---
>
> Running with this patch results in the following,
>
> [    3.486943] ------------[ cut here ]------------
> [    3.488455] WARNING: at /home/matt/src/kernels/linux-2.6/arch/x86/mm/ioremap.c:586 check_early_ioremap_leak+0x3b/0x50()
> [    3.490043] Hardware name: MacBook2,1
> [    3.491600] Debug warning: early ioremap leak of 1 areas detected.
> [    3.493156] Modules linked in:
> [    3.493973] usb 1-4: new high-speed USB device number 3 using ehci_hcd
> [    3.496340] Pid: 1, comm: swapper/0 Not tainted 3.7.0-rc1+ #26
> [    3.497949] Call Trace:
> [    3.499558]  [<ffffffff81d022ab>] ? check_early_ioremap_leak+0x3b/0x50
> [    3.501217]  [<ffffffff81032970>] warn_slowpath_common+0x85/0x9d
> [    3.502872]  [<ffffffff81d02270>] ? early_ioremap_debug_setup+0x12/0x12
> [    3.504516]  [<ffffffff81032a2b>] warn_slowpath_fmt+0x46/0x48
> [    3.506161]  [<ffffffff81cf89df>] ? mcheck_debugfs_init+0x3b/0x3b
> [    3.507790]  [<ffffffff81d022ab>] check_early_ioremap_leak+0x3b/0x50
> [    3.509418]  [<ffffffff8100023b>] do_one_initcall+0x7f/0x134
> [    3.511040]  [<ffffffff8163cc3c>] kernel_init+0x10c/0x275
> [    3.512638]  [<ffffffff81cef4c7>] ? loglevel+0x31/0x31
> [    3.514241]  [<ffffffff8163cb30>] ? rest_init+0x74/0x74
> [    3.515824]  [<ffffffff8165ca9c>] ret_from_fork+0x7c/0xb0
> [    3.517393]  [<ffffffff8163cb30>] ? rest_init+0x74/0x74
> [    3.518958] ---[ end trace afc7d1c216dac6e1 ]---
> [    3.520479] please boot with early_ioremap_debug and report the dmesg.
>
> because we're now leaking the mapping from efi_memmap_init().

Odd, I didn't see it here. Thanks for catching it.

> Maybe it should be handled like so?

Looks good, since the memory should still be reserved and not reused.

Acked-by: Olof Johansson <olof@lixom.net>
(or just amend the original patch)


-Olof