Re: [PATCH v2 6/6] efi: Use generic EFI loader for x86_64 and i386

[Ard Biesheuvel]

On Sun, 14 May 2023 at 07:12, Glenn Washburn
<development@efficientek.com> wrote:
>
> On Thu, 11 May 2023 14:06:40 +0200
> Ard Biesheuvel <ardb@kernel.org> wrote:
>
> > Switch the x86 based EFI platform builds to the generic EFI loader,
> > which exposes the initrd via the LoadFile2 protocol instead of the
> > x86-specific setup header. This will launch the Linux kernel via its
> > EFI stub, which performs its own initialization in the EFI boot
> > services context before calling ExitBootServices() and performing the
> > bare metal Linux boot.
> >
> > Given that only Linux kernel versions v5.8 and later support this
> > initrd loading method, the existing x86 loader is retained as a
> > fallback, which will also be used for Linux kernels built without the
> > EFI stub. In this case, GRUB calls ExitBootServices() before entering
> > the Linux kernel, and all EFI related information is provided to the
> > kernel via struct boot_params in the setup header, as before.
> >
> > Note that this means that booting EFI stub kernels older than v5.8 is
> > not supported even when not using an initrd at all. Also, the EFI
> > handover protocol, which has no basis in the UEFI specification, is
> > not implemented.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  grub-core/Makefile.core.def   |  5 +-
> >  grub-core/loader/efi/linux.c  | 51 ++++++++++++++++++--
> >  grub-core/loader/i386/linux.c |  8 +++
> >  include/grub/efi/efi.h        |  2 +-
> >  4 files changed, 56 insertions(+), 10 deletions(-)
> >
> > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > index 8ee2bd89c4205761..bc08955ac791241a 100644
> > --- a/grub-core/Makefile.core.def
> > +++ b/grub-core/Makefile.core.def
> > @@ -1818,11 +1818,8 @@ module = {
> >    powerpc_ieee1275 = loader/powerpc/ieee1275/linux.c;
> >    sparc64_ieee1275 = loader/sparc64/ieee1275/linux.c;
> >    arm_coreboot = loader/arm/linux.c;
> > -  arm_efi = loader/efi/linux.c;
> >    arm_uboot = loader/arm/linux.c;
> > -  arm64 = loader/efi/linux.c;
> > -  riscv32 = loader/efi/linux.c;
> > -  riscv64 = loader/efi/linux.c;
> > +  efi = loader/efi/linux.c;
> >    emu = loader/emu/linux.c;
> >    common = loader/linux.c;
> >    common = lib/cmdline.c;
> > diff --git a/grub-core/loader/efi/linux.c
> > b/grub-core/loader/efi/linux.c index
> > 15e0686549d7ecca..970a4d7a5d4d464f 100644 ---
> > a/grub-core/loader/efi/linux.c +++ b/grub-core/loader/efi/linux.c
> > @@ -69,6 +69,12 @@ static initrd_media_device_path_t
> > initrd_lf2_device_path = { }
> >  };
> >
> > +extern grub_err_t
> > +grub_cmd_linux_x86_legacy (grub_command_t cmd, int argc, char
> > *argv[]); +
> > +extern grub_err_t
> > +grub_cmd_initrd_x86_legacy (grub_command_t cmd, int argc, char
> > *argv[]); +
> >  static grub_efi_status_t __grub_efi_api
> >  grub_efi_initrd_load_file2 (grub_efi_load_file2_t *this,
> >                              grub_efi_device_path_t *device_path,
> > @@ -89,8 +95,7 @@ grub_arch_efi_linux_load_image_header (grub_file_t
> > file, return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read
> > Linux image header");
> >    if ((lh->code0 & 0xffff) != GRUB_PE32_MAGIC)
> > -    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> > -                    N_("plain image kernel not supported -
> > rebuild with CONFIG_(U)EFI_STUB enabled"));
> > +    return GRUB_ERR_NOT_IMPLEMENTED_YET;
> >
> >    grub_dprintf ("linux", "UEFI stub kernel:\n");
> >    grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
> > @@ -117,7 +122,11 @@ grub_arch_efi_linux_load_image_header
> > (grub_file_t file, if
> > (lh->pe_image_header.optional_header.major_image_version >= 1)
> > initrd_use_loadfile2 = true; else
> > +#if defined(__i386__) || defined(__x86_64__)
> > +    return GRUB_ERR_NOT_IMPLEMENTED_YET;
>
> Is this for the unimplemented EFI handover protocol?

No. This forces the caller (below) to take the x86 legacy fallback
path, which we need to use in 2 cases:
- no EFI stub support
- no LoadFile2 support

The former implies the latter for non-EFI stub kernels, which GRUB
supports today, and should remain supported imo. When booting EFI stub
kernels older than v5.8, we fall back to the legacy boot as well, as
it is the only way to boot with EFI support (at runtime) and with
initrd support.

The EFI handover protocol should remain unimplemented.

> Regardless, I think
> a comment above this line indicating what would be implemented here
> would be nice for others looking at this.
>

Nothing should be implemented here. This is just the escape hatch to
fall back to the legacy code. GRUB does not define
GRUB_ERR_NOT_IMPLEMENTED without the _YET, but one could argue that it
applies to the kernel image in this case. Other suggestions for
suitable GRUB_ERR_xxx values are welcome.


>
> > +#else
> >      initrd_use_loadfile2 = false;
> > +#endif
> >
> >    grub_dprintf ("linux", "LoadFile2 initrd loading %sabled\n",
> >                  initrd_use_loadfile2 ? "en" : "dis");
> > @@ -125,6 +134,7 @@ grub_arch_efi_linux_load_image_header
> > (grub_file_t file, return GRUB_ERR_NONE;
> >  }
> >
> > +#if !defined(__i386__) && !defined(__x86_64__)
> >  static grub_err_t
> >  finalize_params_linux (void)
> >  {
> > @@ -169,6 +179,7 @@ failure:
> >    grub_fdt_unload();
> >    return grub_error(GRUB_ERR_BAD_OS, "failed to install/update FDT");
> >  }
> > +#endif
> >
> >  grub_err_t
> >  grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size,
> > char *args) @@ -231,8 +242,10 @@ grub_arch_efi_linux_boot_image
> > (grub_addr_t addr, grub_size_t size, char *args) static grub_err_t
> >  grub_linux_boot (void)
> >  {
> > +#if !defined(__i386__) && !defined(__x86_64__)
> >    if (finalize_params_linux () != GRUB_ERR_NONE)
> >      return grub_errno;
> > +#endif
> >
> >    return (grub_arch_efi_linux_boot_image((grub_addr_t)kernel_addr,
> >                                            kernel_size, linux_args));
> > @@ -253,7 +266,9 @@ grub_linux_unload (void)
> >    if (kernel_addr)
> >      grub_efi_free_pages ((grub_addr_t) kernel_addr,
> >                        GRUB_EFI_BYTES_TO_PAGES (kernel_size));
> > +#if !defined(__i386__) && !defined(__x86_64__)
> >    grub_fdt_unload ();
> > +#endif
> >
> >    if (initrd_lf2_handle != NULL)
> >      {
> > @@ -269,6 +284,7 @@ grub_linux_unload (void)
> >    return GRUB_ERR_NONE;
> >  }
> >
> > +#if !defined(__i386__) && !defined(__x86_64__)
> >  /*
> >   * As per linux/Documentation/arm/Booting
> >   * ARM initrd needs to be covered by kernel linear mapping,
> > @@ -304,6 +320,7 @@ allocate_initrd_mem (int initrd_pages)
> >                                      GRUB_EFI_ALLOCATE_MAX_ADDRESS,
> >                                      GRUB_EFI_LOADER_DATA);
> >  }
> > +#endif
> >
> >  static grub_efi_status_t __grub_efi_api
> >  grub_efi_initrd_load_file2 (grub_efi_load_file2_t *this,
> > @@ -345,7 +362,7 @@ static grub_err_t
> >  grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
> >                int argc, char *argv[])
> >  {
> > -  int initrd_size, initrd_pages;
> > +  int __attribute__ ((unused)) initrd_size, initrd_pages;
> >    void *initrd_mem = NULL;
> >    grub_efi_boot_services_t *b = grub_efi_system_table->boot_services;
> >    grub_efi_status_t status;
> > @@ -356,6 +373,11 @@ grub_cmd_initrd (grub_command_t cmd
> > __attribute__ ((unused)), goto fail;
> >      }
> >
> > +#if defined(__i386__) || defined(__x86_64__)
> > +  if (!initrd_use_loadfile2)
> > +    return grub_cmd_initrd_x86_legacy (cmd, argc, argv);
> > +#endif
> > +
> >    if (!loaded)
> >      {
> >        grub_error (GRUB_ERR_BAD_ARGUMENT,
> > @@ -391,6 +413,7 @@ grub_cmd_initrd (grub_command_t cmd __attribute__
> > ((unused)), return GRUB_ERR_NONE;
> >      }
> >
> > +#if !defined(__i386__) && !defined(__x86_64__)
> >    initrd_size = grub_get_initrd_size (&initrd_ctx);
> >    grub_dprintf ("linux", "Loading initrd\n");
> >
> > @@ -410,6 +433,7 @@ grub_cmd_initrd (grub_command_t cmd __attribute__
> > ((unused)), initrd_end = initrd_start + initrd_size;
> >    grub_dprintf ("linux", "[addr=%p, size=0x%x]\n",
> >               (void *) initrd_start, initrd_size);
> > +#endif
> >
> >   fail:
> >    grub_initrd_close (&initrd_ctx);
> > @@ -441,8 +465,25 @@ grub_cmd_linux (grub_command_t cmd __attribute__
> > ((unused)),
> >    kernel_size = grub_file_size (file);
> >
> > -  if (grub_arch_efi_linux_load_image_header (file, &lh) !=
> > GRUB_ERR_NONE)
> > -    goto fail;
> > +  switch (grub_arch_efi_linux_load_image_header (file, &lh))
> > +    {
> > +    case GRUB_ERR_NONE:
> > +      break;
> > +    case GRUB_ERR_NOT_IMPLEMENTED_YET:
> > +#if defined(__i386__) || defined(__x86_64__)
> > +      /*
> > +       * Retry with the legacy x86 boot code - it can load EFI
> > kernels without
> > +       * the stub, and supports loading initrds without the
> > LoadFile2 protocol
> > +       */
> > +      grub_file_close (file);
> > +      return grub_cmd_linux_x86_legacy (cmd, argc, argv);
> > +#endif
> > +      grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> > +               N_("plain image kernel not supported - rebuild
> > with CONFIG_(U)EFI_STUB enabled"));
> > +      /* fall through */
> > +    default:
> > +      goto fail;
> > +    }
> >
> >    grub_loader_unset();
> >
> > diff --git a/grub-core/loader/i386/linux.c
> > b/grub-core/loader/i386/linux.c index
> > c2385d0a561a8fc4..997647a33326eeb8 100644 ---
> > a/grub-core/loader/i386/linux.c +++ b/grub-core/loader/i386/linux.c
> > @@ -1136,6 +1136,7 @@ grub_cmd_initrd (grub_command_t cmd
> > __attribute__ ((unused)), return grub_errno;
> >  }
> >
> > +#ifndef GRUB_MACHINE_EFI
> >  static grub_command_t cmd_linux, cmd_initrd;
> >
> >  GRUB_MOD_INIT(linux)
> > @@ -1152,3 +1153,10 @@ GRUB_MOD_FINI(linux)
> >    grub_unregister_command (cmd_linux);
> >    grub_unregister_command (cmd_initrd);
> >  }
> > +#else
> > +extern grub_err_t __attribute__((alias("grub_cmd_linux")))
> > +grub_cmd_linux_x86_legacy (grub_command_t cmd, int argc, char
> > *argv[]); +
> > +extern grub_err_t __attribute__((alias("grub_cmd_initrd")))
> > +grub_cmd_initrd_x86_legacy (grub_command_t cmd, int argc, char
> > *argv[]); +#endif
> > diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> > index 444bf5b0b53e31fe..d02abca7da67f787 100644
> > --- a/include/grub/efi/efi.h
> > +++ b/include/grub/efi/efi.h
> > @@ -113,12 +113,12 @@ extern void (*EXPORT_VAR(grub_efi_net_config))
> > (grub_efi_handle_t hnd, #if defined(__arm__) || defined(__aarch64__)
> > || defined(__riscv) void
> > *EXPORT_FUNC(grub_efi_get_firmware_fdt)(void); grub_err_t
> > EXPORT_FUNC(grub_efi_get_ram_base)(grub_addr_t *); +#endif
> >  #include <grub/file.h>
> >  grub_err_t grub_arch_efi_linux_load_image_header(grub_file_t file,
> >                                                  struct
> > linux_arch_kernel_header *lh); grub_err_t
> > grub_arch_efi_linux_boot_image(grub_addr_t addr, grub_size_t size,
> > char *args); -#endif
> >
> >  grub_addr_t grub_efi_modules_addr (void);
> >