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

[Glenn Washburn]

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? Regardless, I think
a comment above this line indicating what would be implemented here
would be nice for others looking at this.

Glenn

> +#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);
>