Linux-EFI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] efi/libstub/x86: fix mixed mode boot issue after macro refactor
@ 2020-05-03 15:45 Ard Biesheuvel
  2020-05-04  0:38 ` [PATCH 0/1] efi/libstub: Fix mixed mode boot on efi/next Arvind Sankar
  0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2020-05-03 15:45 UTC (permalink / raw)
  To: linux-efi; +Cc: linux, 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 0/1] efi/libstub: Fix mixed mode boot on efi/next
  2020-05-03 15:45 [PATCH] efi/libstub/x86: fix mixed mode boot issue after macro refactor Ard Biesheuvel
@ 2020-05-04  0:38 ` Arvind Sankar
  2020-05-04  0:38   ` [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor Arvind Sankar
  2020-05-04  2:25   ` [PATCH 0/1] efi/libstub: Fix mixed mode boot on efi/next Guenter Roeck
  0 siblings, 2 replies; 12+ messages in thread
From: Arvind Sankar @ 2020-05-04  0:38 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: linux-efi, linux

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor
  2020-05-04  0:38 ` [PATCH 0/1] efi/libstub: Fix mixed mode boot on efi/next Arvind Sankar
@ 2020-05-04  0:38   ` Arvind Sankar
  2020-05-04  8:05     ` Ard Biesheuvel
  2020-05-04  2:25   ` [PATCH 0/1] efi/libstub: Fix mixed mode boot on efi/next Guenter Roeck
  1 sibling, 1 reply; 12+ messages in thread
From: Arvind Sankar @ 2020-05-04  0:38 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: linux-efi, linux

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/1] efi/libstub: Fix mixed mode boot on efi/next
  2020-05-04  0:38 ` [PATCH 0/1] efi/libstub: Fix mixed mode boot on efi/next Arvind Sankar
  2020-05-04  0:38   ` [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor Arvind Sankar
@ 2020-05-04  2:25   ` Guenter Roeck
  1 sibling, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2020-05-04  2:25 UTC (permalink / raw)
  To: Arvind Sankar, Ard Biesheuvel; +Cc: linux-efi

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(-)
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor
  2020-05-04  0:38   ` [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor Arvind Sankar
@ 2020-05-04  8:05     ` Ard Biesheuvel
  2020-05-04 14:02       ` Arvind Sankar
  0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2020-05-04  8:05 UTC (permalink / raw)
  To: Arvind Sankar; +Cc: linux-efi, Guenter Roeck

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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor
  2020-05-04  8:05     ` Ard Biesheuvel
@ 2020-05-04 14:02       ` Arvind Sankar
  2020-05-04 14:15         ` Ard Biesheuvel
  0 siblings, 1 reply; 12+ messages in thread
From: Arvind Sankar @ 2020-05-04 14:02 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Arvind Sankar, linux-efi, Guenter Roeck

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor
  2020-05-04 14:02       ` Arvind Sankar
@ 2020-05-04 14:15         ` Ard Biesheuvel
  2020-05-04 14:27           ` Arvind Sankar
  0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2020-05-04 14:15 UTC (permalink / raw)
  To: Arvind Sankar; +Cc: linux-efi, Guenter Roeck

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor
  2020-05-04 14:15         ` Ard Biesheuvel
@ 2020-05-04 14:27           ` Arvind Sankar
  2020-05-04 14:33             ` Ard Biesheuvel
  0 siblings, 1 reply; 12+ messages in thread
From: Arvind Sankar @ 2020-05-04 14:27 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Arvind Sankar, linux-efi, Guenter Roeck

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 {			\

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor
  2020-05-04 14:27           ` Arvind Sankar
@ 2020-05-04 14:33             ` Ard Biesheuvel
  2020-05-04 15:02               ` [PATCH v2] " Arvind Sankar
  0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2020-05-04 14:33 UTC (permalink / raw)
  To: Arvind Sankar; +Cc: linux-efi, Guenter Roeck

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 {                    \

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] efi/libstub: Fix mixed mode boot issue after macro refactor
  2020-05-04 14:33             ` Ard Biesheuvel
@ 2020-05-04 15:02               ` Arvind Sankar
  2020-05-05  7:58                 ` Ard Biesheuvel
  0 siblings, 1 reply; 12+ messages in thread
From: Arvind Sankar @ 2020-05-04 15:02 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: linux-efi, Guenter Roeck

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] efi/libstub: Fix mixed mode boot issue after macro refactor
  2020-05-04 15:02               ` [PATCH v2] " Arvind Sankar
@ 2020-05-05  7:58                 ` Ard Biesheuvel
  0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2020-05-05  7:58 UTC (permalink / raw)
  To: Arvind Sankar; +Cc: linux-efi, Guenter Roeck

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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] efi/libstub/x86: fix mixed mode boot issue after macro refactor
@ 2020-05-03 16:16 Guenter Roeck
  0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2020-05-03 16:16 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: linux-efi

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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-03 15:45 [PATCH] efi/libstub/x86: fix mixed mode boot issue after macro refactor Ard Biesheuvel
2020-05-04  0:38 ` [PATCH 0/1] efi/libstub: Fix mixed mode boot on efi/next Arvind Sankar
2020-05-04  0:38   ` [PATCH 1/1] efi/libstub: Fix mixed mode boot issue after macro refactor Arvind Sankar
2020-05-04  8:05     ` Ard Biesheuvel
2020-05-04 14:02       ` Arvind Sankar
2020-05-04 14:15         ` Ard Biesheuvel
2020-05-04 14:27           ` Arvind Sankar
2020-05-04 14:33             ` Ard Biesheuvel
2020-05-04 15:02               ` [PATCH v2] " Arvind Sankar
2020-05-05  7:58                 ` Ard Biesheuvel
2020-05-04  2:25   ` [PATCH 0/1] efi/libstub: Fix mixed mode boot on efi/next Guenter Roeck
2020-05-03 16:16 [PATCH] efi/libstub/x86: fix mixed mode boot issue after macro refactor Guenter Roeck

Linux-EFI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-efi/0 linux-efi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-efi linux-efi/ https://lore.kernel.org/linux-efi \
		linux-efi@vger.kernel.org
	public-inbox-index linux-efi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-efi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git