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

[Ard Biesheuvel <ardb@kernel.org>]

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