[PATCH] efi/libstub/x86: fix mixed mode boot issue after macro refactor

[PATCH] efi/libstub/x86: fix mixed mode boot issue after macro refactor

[Ard Biesheuvel]

Commit

  22090f84bc3f8081 ("efi/libstub: Unify EFI call wrappers for non-x86")

refactored some macros that are used to wrap EFI service calls, and
allow us to boot the 64-bit x86 kernel from 32-bit firmware. Sadly, due
to an oversight, this caused a boot issue on mixed mode, due to the fact
that efi_is_native() is not a macro on x86, and so #ifndef will not
detect that it is already defined.

Fix this by defining the macro as well.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/include/asm/efi.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index cd0c3fbf6156..42b2fd123a2f 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -240,6 +240,7 @@ static inline bool efi_is_native(void)
 		return true;
 	return efi_is_64bit();
 }
+#define efi_is_native efi_is_native
 
 #define efi_mixed_mode_cast(attr)					\
 	__builtin_choose_expr(						\
-- 
2.26.2

[PATCH 0/1] efi/libstub: Fix mixed mode boot on efi/next

[Arvind Sankar]

How about this instead -- just get rid of the macros on x86 as well if
mixed mode isn't enabled, and condition the generic definition on
CONFIG_EFI_MIXED?

Arvind Sankar (1):
  efi/libstub: Fix mixed mode boot issue after macro refactor

 arch/x86/include/asm/efi.h             | 17 +++++++++++++----
 drivers/firmware/efi/libstub/efistub.h | 14 ++++----------
 2 files changed, 17 insertions(+), 14 deletions(-)

-- 
2.26.2

[PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor

[Arvind Sankar]

Commit
  22090f84bc3f ("efi/libstub: unify EFI call wrappers for non-x86")

refactored the macros that are used to provide wrappers for mixed-mode
calls on x86, allowing us to boot a 64-bit kernel on 32-bit firmware.

Unfortunately, this broke mixed mode boot due to the fact that
efi_is_native() is not a macro on x86.

Fix this by conditioning the generic macro definitions on
CONFIG_EFI_MIXED, and removing the wrapper definitions on x86 if
CONFIG_EFI_MIXED is not enabled.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/include/asm/efi.h             | 17 +++++++++++++----
 drivers/firmware/efi/libstub/efistub.h | 14 ++++----------
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index cd0c3fbf6156..88f29f84a4d0 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -225,13 +225,13 @@ efi_status_t efi_set_virtual_address_map(unsigned long memory_map_size,
 
 /* arch specific definitions used by the stub code */
 
-extern const bool efi_is64;
+#ifdef CONFIG_EFI_MIXED
 
 static inline bool efi_is_64bit(void)
 {
-	if (IS_ENABLED(CONFIG_EFI_MIXED))
-		return efi_is64;
-	return IS_ENABLED(CONFIG_X86_64);
+	extern const bool efi_is64;
+
+	return efi_is64;
 }
 
 static inline bool efi_is_native(void)
@@ -356,6 +356,15 @@ static inline u32 efi64_convert_status(efi_status_t status)
 						   runtime),		\
 				    func, __VA_ARGS__))
 
+#else /* CONFIG_EFI_MIXED */
+
+static inline bool efi_is_64bit(void)
+{
+	return IS_ENABLED(CONFIG_X86_64);
+}
+
+#endif /* CONFIG_EFI_MIXED */
+
 extern bool efi_reboot_required(void);
 extern bool efi_is_table_address(unsigned long phys_addr);
 
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 874233cf8820..37ca286a40c6 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -33,20 +33,14 @@ extern bool efi_novamap;
 
 extern const efi_system_table_t *efi_system_table;
 
-#ifndef efi_bs_call
+#ifndef CONFIG_EFI_MIXED
+
+#define efi_is_native()		(true)
 #define efi_bs_call(func, ...)	efi_system_table->boottime->func(__VA_ARGS__)
-#endif
-#ifndef efi_rt_call
 #define efi_rt_call(func, ...)	efi_system_table->runtime->func(__VA_ARGS__)
-#endif
-#ifndef efi_is_native
-#define efi_is_native()		(true)
-#endif
-#ifndef efi_table_attr
 #define efi_table_attr(inst, attr)	(inst->attr)
-#endif
-#ifndef efi_call_proto
 #define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
+
 #endif
 
 #define efi_info(msg)		do {			\
-- 
2.26.2

Re: [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor

[Ard Biesheuvel]

On Mon, 4 May 2020 at 02:38, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Commit
>   22090f84bc3f ("efi/libstub: unify EFI call wrappers for non-x86")
>
> refactored the macros that are used to provide wrappers for mixed-mode
> calls on x86, allowing us to boot a 64-bit kernel on 32-bit firmware.
>
> Unfortunately, this broke mixed mode boot due to the fact that
> efi_is_native() is not a macro on x86.
>
> Fix this by conditioning the generic macro definitions on
> CONFIG_EFI_MIXED, and removing the wrapper definitions on x86 if
> CONFIG_EFI_MIXED is not enabled.
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Thanks Arvind.

Currently, CONFIG_EFI_MIXED is never referenced outside of arch/x86,
and I prefer to keep it that way.

Also, I fail to see where the definition of efi_is_native() comes from
after applying this patch.



> ---
>  arch/x86/include/asm/efi.h             | 17 +++++++++++++----
>  drivers/firmware/efi/libstub/efistub.h | 14 ++++----------
>  2 files changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index cd0c3fbf6156..88f29f84a4d0 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -225,13 +225,13 @@ efi_status_t efi_set_virtual_address_map(unsigned long memory_map_size,
>
>  /* arch specific definitions used by the stub code */
>
> -extern const bool efi_is64;
> +#ifdef CONFIG_EFI_MIXED
>
>  static inline bool efi_is_64bit(void)
>  {
> -       if (IS_ENABLED(CONFIG_EFI_MIXED))
> -               return efi_is64;
> -       return IS_ENABLED(CONFIG_X86_64);
> +       extern const bool efi_is64;
> +
> +       return efi_is64;
>  }
>
>  static inline bool efi_is_native(void)
> @@ -356,6 +356,15 @@ static inline u32 efi64_convert_status(efi_status_t status)
>                                                    runtime),            \
>                                     func, __VA_ARGS__))
>
> +#else /* CONFIG_EFI_MIXED */
> +
> +static inline bool efi_is_64bit(void)
> +{
> +       return IS_ENABLED(CONFIG_X86_64);
> +}
> +
> +#endif /* CONFIG_EFI_MIXED */
> +
>  extern bool efi_reboot_required(void);
>  extern bool efi_is_table_address(unsigned long phys_addr);
>
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 874233cf8820..37ca286a40c6 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -33,20 +33,14 @@ extern bool efi_novamap;
>
>  extern const efi_system_table_t *efi_system_table;
>
> -#ifndef efi_bs_call
> +#ifndef CONFIG_EFI_MIXED
> +
> +#define efi_is_native()                (true)
>  #define efi_bs_call(func, ...) efi_system_table->boottime->func(__VA_ARGS__)
> -#endif
> -#ifndef efi_rt_call
>  #define efi_rt_call(func, ...) efi_system_table->runtime->func(__VA_ARGS__)
> -#endif
> -#ifndef efi_is_native
> -#define efi_is_native()                (true)
> -#endif
> -#ifndef efi_table_attr
>  #define efi_table_attr(inst, attr)     (inst->attr)
> -#endif
> -#ifndef efi_call_proto
>  #define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
> +
>  #endif
>
>  #define efi_info(msg)          do {                    \
> --
> 2.26.2
>

Re: [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor

[Arvind Sankar]

On Mon, May 04, 2020 at 10:05:23AM +0200, Ard Biesheuvel wrote:
> On Mon, 4 May 2020 at 02:38, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > Commit
> >   22090f84bc3f ("efi/libstub: unify EFI call wrappers for non-x86")
> >
> > refactored the macros that are used to provide wrappers for mixed-mode
> > calls on x86, allowing us to boot a 64-bit kernel on 32-bit firmware.
> >
> > Unfortunately, this broke mixed mode boot due to the fact that
> > efi_is_native() is not a macro on x86.
> >
> > Fix this by conditioning the generic macro definitions on
> > CONFIG_EFI_MIXED, and removing the wrapper definitions on x86 if
> > CONFIG_EFI_MIXED is not enabled.
> >
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> 
> Thanks Arvind.
> 
> Currently, CONFIG_EFI_MIXED is never referenced outside of arch/x86,
> and I prefer to keep it that way.

All these macros go together though -- they should either all be defined
or none, so it makes sense to put them under a single #if. If you think
it's possible that a future architecture might need the wrappers but not
be mixed, we could maybe add an ARCH_NEEDS_EFISTUB_WRAPPERS?

> 
> Also, I fail to see where the definition of efi_is_native() comes from
> after applying this patch.

The generic definition is in the same place -- I just moved it to the
top as it's a predicate rather than a wrapper, and kept all the actual
wrappers together.

> > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> > index 874233cf8820..37ca286a40c6 100644
> > --- a/drivers/firmware/efi/libstub/efistub.h
> > +++ b/drivers/firmware/efi/libstub/efistub.h
> > @@ -33,20 +33,14 @@ extern bool efi_novamap;
> >
> >  extern const efi_system_table_t *efi_system_table;
> >
> > -#ifndef efi_bs_call
> > +#ifndef CONFIG_EFI_MIXED
> > +
> > +#define efi_is_native()                (true)
> >  #define efi_bs_call(func, ...) efi_system_table->boottime->func(__VA_ARGS__)
> > -#endif
> > -#ifndef efi_rt_call
> >  #define efi_rt_call(func, ...) efi_system_table->runtime->func(__VA_ARGS__)
> > -#endif
> > -#ifndef efi_is_native
> > -#define efi_is_native()                (true)
> > -#endif
> > -#ifndef efi_table_attr
> >  #define efi_table_attr(inst, attr)     (inst->attr)
> > -#endif
> > -#ifndef efi_call_proto
> >  #define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
> > +
> >  #endif
> >
> >  #define efi_info(msg)          do {                    \
> > --
> > 2.26.2
> >

Re: [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor

[Ard Biesheuvel]

On Mon, 4 May 2020 at 16:02, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Mon, May 04, 2020 at 10:05:23AM +0200, Ard Biesheuvel wrote:
> > On Mon, 4 May 2020 at 02:38, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > Commit
> > >   22090f84bc3f ("efi/libstub: unify EFI call wrappers for non-x86")
> > >
> > > refactored the macros that are used to provide wrappers for mixed-mode
> > > calls on x86, allowing us to boot a 64-bit kernel on 32-bit firmware.
> > >
> > > Unfortunately, this broke mixed mode boot due to the fact that
> > > efi_is_native() is not a macro on x86.
> > >
> > > Fix this by conditioning the generic macro definitions on
> > > CONFIG_EFI_MIXED, and removing the wrapper definitions on x86 if
> > > CONFIG_EFI_MIXED is not enabled.
> > >
> > > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> >
> > Thanks Arvind.
> >
> > Currently, CONFIG_EFI_MIXED is never referenced outside of arch/x86,
> > and I prefer to keep it that way.
>
> All these macros go together though -- they should either all be defined
> or none, so it makes sense to put them under a single #if.

True.

> If you think
> it's possible that a future architecture might need the wrappers but not
> be mixed, we could maybe add an ARCH_NEEDS_EFISTUB_WRAPPERS?
>

Well, remember that x86 used wrappers for native invocations only two
releases ago, but that was mainly because of the SysV vs MS ABI issue,
so the issue could emerge again, but it is unlikely.

> >
> > Also, I fail to see where the definition of efi_is_native() comes from
> > after applying this patch.
>
> The generic definition is in the same place -- I just moved it to the
> top as it's a predicate rather than a wrapper, and kept all the actual
> wrappers together.
>

Ah yes, I see it now.


> > > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> > > index 874233cf8820..37ca286a40c6 100644
> > > --- a/drivers/firmware/efi/libstub/efistub.h
> > > +++ b/drivers/firmware/efi/libstub/efistub.h
> > > @@ -33,20 +33,14 @@ extern bool efi_novamap;
> > >
> > >  extern const efi_system_table_t *efi_system_table;
> > >
> > > -#ifndef efi_bs_call
> > > +#ifndef CONFIG_EFI_MIXED
> > > +
> > > +#define efi_is_native()                (true)
> > >  #define efi_bs_call(func, ...) efi_system_table->boottime->func(__VA_ARGS__)
> > > -#endif
> > > -#ifndef efi_rt_call
> > >  #define efi_rt_call(func, ...) efi_system_table->runtime->func(__VA_ARGS__)
> > > -#endif
> > > -#ifndef efi_is_native
> > > -#define efi_is_native()                (true)
> > > -#endif
> > > -#ifndef efi_table_attr
> > >  #define efi_table_attr(inst, attr)     (inst->attr)
> > > -#endif
> > > -#ifndef efi_call_proto
> > >  #define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
> > > +
> > >  #endif
> > >
> > >  #define efi_info(msg)          do {                    \
> > > --
> > > 2.26.2
> > >

Re: [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor

[Arvind Sankar]

On Mon, May 04, 2020 at 04:15:59PM +0200, Ard Biesheuvel wrote:
> On Mon, 4 May 2020 at 16:02, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Mon, May 04, 2020 at 10:05:23AM +0200, Ard Biesheuvel wrote:
> > > On Mon, 4 May 2020 at 02:38, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > Commit
> > > >   22090f84bc3f ("efi/libstub: unify EFI call wrappers for non-x86")
> > > >
> > > > refactored the macros that are used to provide wrappers for mixed-mode
> > > > calls on x86, allowing us to boot a 64-bit kernel on 32-bit firmware.
> > > >
> > > > Unfortunately, this broke mixed mode boot due to the fact that
> > > > efi_is_native() is not a macro on x86.
> > > >
> > > > Fix this by conditioning the generic macro definitions on
> > > > CONFIG_EFI_MIXED, and removing the wrapper definitions on x86 if
> > > > CONFIG_EFI_MIXED is not enabled.
> > > >
> > > > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > >
> > > Thanks Arvind.
> > >
> > > Currently, CONFIG_EFI_MIXED is never referenced outside of arch/x86,
> > > and I prefer to keep it that way.
> >
> > All these macros go together though -- they should either all be defined
> > or none, so it makes sense to put them under a single #if.
> 
> True.
> 
> > If you think
> > it's possible that a future architecture might need the wrappers but not
> > be mixed, we could maybe add an ARCH_NEEDS_EFISTUB_WRAPPERS?
> >
> 
> Well, remember that x86 used wrappers for native invocations only two
> releases ago, but that was mainly because of the SysV vs MS ABI issue,
> so the issue could emerge again, but it is unlikely.
> 

Yep.

Would the below be more palatable?

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index cd0c3fbf6156..6b9ab0d8b2a7 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -225,13 +225,15 @@ efi_status_t efi_set_virtual_address_map(unsigned long memory_map_size,
 
 /* arch specific definitions used by the stub code */
 
-extern const bool efi_is64;
+#ifdef CONFIG_EFI_MIXED
+
+#define ARCH_HAS_EFISTUB_WRAPPERS
 
 static inline bool efi_is_64bit(void)
 {
-	if (IS_ENABLED(CONFIG_EFI_MIXED))
-		return efi_is64;
-	return IS_ENABLED(CONFIG_X86_64);
+	extern const bool efi_is64;
+
+	return efi_is64;
 }
 
 static inline bool efi_is_native(void)
@@ -356,6 +358,15 @@ static inline u32 efi64_convert_status(efi_status_t status)
 						   runtime),		\
 				    func, __VA_ARGS__))
 
+#else /* CONFIG_EFI_MIXED */
+
+static inline bool efi_is_64bit(void)
+{
+	return IS_ENABLED(CONFIG_X86_64);
+}
+
+#endif /* CONFIG_EFI_MIXED */
+
 extern bool efi_reboot_required(void);
 extern bool efi_is_table_address(unsigned long phys_addr);
 
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 874233cf8820..4f10a09563f3 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -33,20 +33,14 @@ extern bool efi_novamap;
 
 extern const efi_system_table_t *efi_system_table;
 
-#ifndef efi_bs_call
+#ifndef ARCH_HAS_EFISTUB_WRAPPERS
+
+#define efi_is_native()		(true)
 #define efi_bs_call(func, ...)	efi_system_table->boottime->func(__VA_ARGS__)
-#endif
-#ifndef efi_rt_call
 #define efi_rt_call(func, ...)	efi_system_table->runtime->func(__VA_ARGS__)
-#endif
-#ifndef efi_is_native
-#define efi_is_native()		(true)
-#endif
-#ifndef efi_table_attr
 #define efi_table_attr(inst, attr)	(inst->attr)
-#endif
-#ifndef efi_call_proto
 #define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
+
 #endif
 
 #define efi_info(msg)		do {			\

Re: [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor

[Ard Biesheuvel]

On Mon, 4 May 2020 at 16:27, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Mon, May 04, 2020 at 04:15:59PM +0200, Ard Biesheuvel wrote:
> > On Mon, 4 May 2020 at 16:02, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Mon, May 04, 2020 at 10:05:23AM +0200, Ard Biesheuvel wrote:
> > > > On Mon, 4 May 2020 at 02:38, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > >
> > > > > Commit
> > > > >   22090f84bc3f ("efi/libstub: unify EFI call wrappers for non-x86")
> > > > >
> > > > > refactored the macros that are used to provide wrappers for mixed-mode
> > > > > calls on x86, allowing us to boot a 64-bit kernel on 32-bit firmware.
> > > > >
> > > > > Unfortunately, this broke mixed mode boot due to the fact that
> > > > > efi_is_native() is not a macro on x86.
> > > > >
> > > > > Fix this by conditioning the generic macro definitions on
> > > > > CONFIG_EFI_MIXED, and removing the wrapper definitions on x86 if
> > > > > CONFIG_EFI_MIXED is not enabled.
> > > > >
> > > > > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > > > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > > >
> > > > Thanks Arvind.
> > > >
> > > > Currently, CONFIG_EFI_MIXED is never referenced outside of arch/x86,
> > > > and I prefer to keep it that way.
> > >
> > > All these macros go together though -- they should either all be defined
> > > or none, so it makes sense to put them under a single #if.
> >
> > True.
> >
> > > If you think
> > > it's possible that a future architecture might need the wrappers but not
> > > be mixed, we could maybe add an ARCH_NEEDS_EFISTUB_WRAPPERS?
> > >
> >
> > Well, remember that x86 used wrappers for native invocations only two
> > releases ago, but that was mainly because of the SysV vs MS ABI issue,
> > so the issue could emerge again, but it is unlikely.
> >
>
> Yep.
>
> Would the below be more palatable?
>

Yes that looks better. Could you please spin it as a proper patch?


> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index cd0c3fbf6156..6b9ab0d8b2a7 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -225,13 +225,15 @@ efi_status_t efi_set_virtual_address_map(unsigned long memory_map_size,
>
>  /* arch specific definitions used by the stub code */
>
> -extern const bool efi_is64;
> +#ifdef CONFIG_EFI_MIXED
> +
> +#define ARCH_HAS_EFISTUB_WRAPPERS
>
>  static inline bool efi_is_64bit(void)
>  {
> -       if (IS_ENABLED(CONFIG_EFI_MIXED))
> -               return efi_is64;
> -       return IS_ENABLED(CONFIG_X86_64);
> +       extern const bool efi_is64;
> +
> +       return efi_is64;
>  }
>
>  static inline bool efi_is_native(void)
> @@ -356,6 +358,15 @@ static inline u32 efi64_convert_status(efi_status_t status)
>                                                    runtime),            \
>                                     func, __VA_ARGS__))
>
> +#else /* CONFIG_EFI_MIXED */
> +
> +static inline bool efi_is_64bit(void)
> +{
> +       return IS_ENABLED(CONFIG_X86_64);
> +}
> +
> +#endif /* CONFIG_EFI_MIXED */
> +
>  extern bool efi_reboot_required(void);
>  extern bool efi_is_table_address(unsigned long phys_addr);
>
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 874233cf8820..4f10a09563f3 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -33,20 +33,14 @@ extern bool efi_novamap;
>
>  extern const efi_system_table_t *efi_system_table;
>
> -#ifndef efi_bs_call
> +#ifndef ARCH_HAS_EFISTUB_WRAPPERS
> +
> +#define efi_is_native()                (true)
>  #define efi_bs_call(func, ...) efi_system_table->boottime->func(__VA_ARGS__)
> -#endif
> -#ifndef efi_rt_call
>  #define efi_rt_call(func, ...) efi_system_table->runtime->func(__VA_ARGS__)
> -#endif
> -#ifndef efi_is_native
> -#define efi_is_native()                (true)
> -#endif
> -#ifndef efi_table_attr
>  #define efi_table_attr(inst, attr)     (inst->attr)
> -#endif
> -#ifndef efi_call_proto
>  #define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
> +
>  #endif
>
>  #define efi_info(msg)          do {                    \

[PATCH v2] efi/libstub: Fix mixed mode boot issue after macro refactor

[Arvind Sankar]

Commit
  22090f84bc3f ("efi/libstub: unify EFI call wrappers for non-x86")

refactored the macros that are used to provide wrappers for mixed-mode
calls on x86, allowing us to boot a 64-bit kernel on 32-bit firmware.

Unfortunately, this broke mixed mode boot due to the fact that
efi_is_native() is not a macro on x86.

All of these macros should go together, so rather than testing each one
to see if it is defined, condition the generic macro definitions on a
new ARCH_HAS_EFISTUB_WRAPPERS, and remove the wrapper definitions on x86
as well if CONFIG_EFI_MIXED is not enabled.

Fixes: 22090f84bc3f ("efi/libstub: unify EFI call wrappers for non-x86")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/include/asm/efi.h             | 19 +++++++++++++++----
 drivers/firmware/efi/libstub/efistub.h | 14 ++++----------
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index cd0c3fbf6156..6b9ab0d8b2a7 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -225,13 +225,15 @@ efi_status_t efi_set_virtual_address_map(unsigned long memory_map_size,
 
 /* arch specific definitions used by the stub code */
 
-extern const bool efi_is64;
+#ifdef CONFIG_EFI_MIXED
+
+#define ARCH_HAS_EFISTUB_WRAPPERS
 
 static inline bool efi_is_64bit(void)
 {
-	if (IS_ENABLED(CONFIG_EFI_MIXED))
-		return efi_is64;
-	return IS_ENABLED(CONFIG_X86_64);
+	extern const bool efi_is64;
+
+	return efi_is64;
 }
 
 static inline bool efi_is_native(void)
@@ -356,6 +358,15 @@ static inline u32 efi64_convert_status(efi_status_t status)
 						   runtime),		\
 				    func, __VA_ARGS__))
 
+#else /* CONFIG_EFI_MIXED */
+
+static inline bool efi_is_64bit(void)
+{
+	return IS_ENABLED(CONFIG_X86_64);
+}
+
+#endif /* CONFIG_EFI_MIXED */
+
 extern bool efi_reboot_required(void);
 extern bool efi_is_table_address(unsigned long phys_addr);
 
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 874233cf8820..4f10a09563f3 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -33,20 +33,14 @@ extern bool efi_novamap;
 
 extern const efi_system_table_t *efi_system_table;
 
-#ifndef efi_bs_call
+#ifndef ARCH_HAS_EFISTUB_WRAPPERS
+
+#define efi_is_native()		(true)
 #define efi_bs_call(func, ...)	efi_system_table->boottime->func(__VA_ARGS__)
-#endif
-#ifndef efi_rt_call
 #define efi_rt_call(func, ...)	efi_system_table->runtime->func(__VA_ARGS__)
-#endif
-#ifndef efi_is_native
-#define efi_is_native()		(true)
-#endif
-#ifndef efi_table_attr
 #define efi_table_attr(inst, attr)	(inst->attr)
-#endif
-#ifndef efi_call_proto
 #define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
+
 #endif
 
 #define efi_info(msg)		do {			\
-- 
2.26.2

Re: [PATCH v2] efi/libstub: Fix mixed mode boot issue after macro refactor

[Ard Biesheuvel]

On Mon, 4 May 2020 at 17:02, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Commit
>   22090f84bc3f ("efi/libstub: unify EFI call wrappers for non-x86")
>
> refactored the macros that are used to provide wrappers for mixed-mode
> calls on x86, allowing us to boot a 64-bit kernel on 32-bit firmware.
>
> Unfortunately, this broke mixed mode boot due to the fact that
> efi_is_native() is not a macro on x86.
>
> All of these macros should go together, so rather than testing each one
> to see if it is defined, condition the generic macro definitions on a
> new ARCH_HAS_EFISTUB_WRAPPERS, and remove the wrapper definitions on x86
> as well if CONFIG_EFI_MIXED is not enabled.
>
> Fixes: 22090f84bc3f ("efi/libstub: unify EFI call wrappers for non-x86")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Thanks Arvind, I've queued this up now.

> ---
>  arch/x86/include/asm/efi.h             | 19 +++++++++++++++----
>  drivers/firmware/efi/libstub/efistub.h | 14 ++++----------
>  2 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index cd0c3fbf6156..6b9ab0d8b2a7 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -225,13 +225,15 @@ efi_status_t efi_set_virtual_address_map(unsigned long memory_map_size,
>
>  /* arch specific definitions used by the stub code */
>
> -extern const bool efi_is64;
> +#ifdef CONFIG_EFI_MIXED
> +
> +#define ARCH_HAS_EFISTUB_WRAPPERS
>
>  static inline bool efi_is_64bit(void)
>  {
> -       if (IS_ENABLED(CONFIG_EFI_MIXED))
> -               return efi_is64;
> -       return IS_ENABLED(CONFIG_X86_64);
> +       extern const bool efi_is64;
> +
> +       return efi_is64;
>  }
>
>  static inline bool efi_is_native(void)
> @@ -356,6 +358,15 @@ static inline u32 efi64_convert_status(efi_status_t status)
>                                                    runtime),            \
>                                     func, __VA_ARGS__))
>
> +#else /* CONFIG_EFI_MIXED */
> +
> +static inline bool efi_is_64bit(void)
> +{
> +       return IS_ENABLED(CONFIG_X86_64);
> +}
> +
> +#endif /* CONFIG_EFI_MIXED */
> +
>  extern bool efi_reboot_required(void);
>  extern bool efi_is_table_address(unsigned long phys_addr);
>
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 874233cf8820..4f10a09563f3 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -33,20 +33,14 @@ extern bool efi_novamap;
>
>  extern const efi_system_table_t *efi_system_table;
>
> -#ifndef efi_bs_call
> +#ifndef ARCH_HAS_EFISTUB_WRAPPERS
> +
> +#define efi_is_native()                (true)
>  #define efi_bs_call(func, ...) efi_system_table->boottime->func(__VA_ARGS__)
> -#endif
> -#ifndef efi_rt_call
>  #define efi_rt_call(func, ...) efi_system_table->runtime->func(__VA_ARGS__)
> -#endif
> -#ifndef efi_is_native
> -#define efi_is_native()                (true)
> -#endif
> -#ifndef efi_table_attr
>  #define efi_table_attr(inst, attr)     (inst->attr)
> -#endif
> -#ifndef efi_call_proto
>  #define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
> +
>  #endif
>
>  #define efi_info(msg)          do {                    \
> --
> 2.26.2
>

Re: [PATCH 0/1] efi/libstub: Fix mixed mode boot on efi/next

[Guenter Roeck]

On 5/3/20 5:38 PM, Arvind Sankar wrote:
> How about this instead -- just get rid of the macros on x86 as well if
> mixed mode isn't enabled, and condition the generic definition on
> CONFIG_EFI_MIXED?
> 

Not my call to make, but I prefer simple solutions. So for me the other patch
is a clear winner.

Guenter

> Arvind Sankar (1):
>   efi/libstub: Fix mixed mode boot issue after macro refactor
> 
>  arch/x86/include/asm/efi.h             | 17 +++++++++++++----
>  drivers/firmware/efi/libstub/efistub.h | 14 ++++----------
>  2 files changed, 17 insertions(+), 14 deletions(-)
> 

Re: [PATCH] efi/libstub/x86: fix mixed mode boot issue after macro refactor

[Guenter Roeck]

On Sun, May 03, 2020 at 05:45:07PM +0200, Ard Biesheuvel wrote:
> Commit
> 
>   22090f84bc3f8081 ("efi/libstub: Unify EFI call wrappers for non-x86")
> 
> refactored some macros that are used to wrap EFI service calls, and
> allow us to boot the 64-bit x86 kernel from 32-bit firmware. Sadly, due
> to an oversight, this caused a boot issue on mixed mode, due to the fact
> that efi_is_native() is not a macro on x86, and so #ifndef will not
> detect that it is already defined.
> 
> Fix this by defining the macro as well.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  arch/x86/include/asm/efi.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index cd0c3fbf6156..42b2fd123a2f 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -240,6 +240,7 @@ static inline bool efi_is_native(void)
>  		return true;
>  	return efi_is_64bit();
>  }
> +#define efi_is_native efi_is_native
>  
>  #define efi_mixed_mode_cast(attr)					\
>  	__builtin_choose_expr(						\
> -- 
> 2.26.2
>