Re: [PATCH v5 06/17] efi_loader: bootmgr: add booting from removable media

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Heinrich,

On Thu, May 05, 2022 at 02:47:35PM +0200, Mark Kettenis wrote:
> > Date: Thu, 5 May 2022 14:05:04 +0200 (CEST)
> > From: Mark Kettenis <mark.kettenis@xs4all.nl>
> > 
> > > Date: Fri, 29 Apr 2022 19:03:22 +0200
> > > From: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > 
> > > On 4/28/22 10:09, Masahisa Kojima wrote:
> > > > From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > >
> > > > Under the current implementation, booting from removable media using
> > > > a architecture-specific default image name, say BOOTAA64.EFI, is
> > > > supported only in distro_bootcmd script. See the commit 74522c898b35
> > > > ("efi_loader: Add distro boot script for removable media").
> > > >
> > > > This is, however, half-baked implementation because
> > > > 1) UEFI specification requires this feature to be implemented as part
> > > >     of Boot Manager's responsibility:
> > > >
> > > >    3 - Boot Manager
> > > >    3.5.1 Boot via the Simple File Protocol
> > > >    When booting via the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, the FilePath will
> > > >    start with a device path that points to the device that implements the
> > > >    EFI_SIMPLE_FILE_SYSTEM_PROTOCOL or the EFI_BLOCK_IO_PROTOCOL. The next
> > > >    part of the FilePath may point to the file name, including
> > > >    subdirectories, which contain the bootable image. If the file name is
> > > >    a null device path, the file name must be generated from the rules
> > > >    defined below.
> > > >    ...
> > > >    3.5.1.1 Removable Media Boot Behavior
> > > >    To generate a file name when none is present in the FilePath, the
> > > >    firmware must append a default file name in the form
> > > >    \EFI\BOOT\BOOT{machine type short-name}.EFI ...
> > > >
> > > > 2) So (1) entails the hehavior that the user's preference of boot media
> > > >     order should be determined by Boot#### and BootOrder variables.
> > > >
> > > > With this patch, the semantics mentioned above is fully implemented.
> > > > For example, if you want to boot the system from USB and SCSI in this
> > > > order,
> > > > * define Boot0001 which contains only a device path to the USB device
> > > >    (without any file path/name)
> > > > * define Boot0002 which contains only a device path to the SCSI device,
> > > > and
> > > > * set BootOrder to Boot0001:Boot0002
> > > >
> > > > To avoid build error for sandbox, default file name "BOOTSANDBOX.efi"
> > > > is defined even if it is out of scope of UEFI specification.
> > > >
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > ---
> > > > Changes in v5:
> > > > - add default file name definition for SANDBOX to avoid build error
> > > >
> > > > Changes from original version:
> > > > - create new include file "efi_default_filename.h" to
> > > >    avoid conflict with config_distro_bootcmd.h
> > > > - modify the target pointer of efi_free_pool(), expand_media_path() should
> > > >    only free the pointer allocated by efi_dp_from_file() function.
> > > >   include/config_distro_bootcmd.h | 14 +--------
> > > >   include/efi_default_filename.h  | 33 ++++++++++++++++++++++
> > > >   lib/efi_loader/efi_bootmgr.c    | 50 ++++++++++++++++++++++++++++++++-
> > > >   3 files changed, 83 insertions(+), 14 deletions(-)
> > > >   create mode 100644 include/efi_default_filename.h
> > > >
> > > > diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> > > > index c55023889c..6a3110f27b 100644
> > > > --- a/include/config_distro_bootcmd.h
> > > > +++ b/include/config_distro_bootcmd.h
> > > > @@ -91,19 +91,7 @@
> > > >   #endif
> > > >
> > > >   #ifdef CONFIG_EFI_LOADER
> > > > -#if defined(CONFIG_ARM64)
> > > > -#define BOOTEFI_NAME "bootaa64.efi"
> > > > -#elif defined(CONFIG_ARM)
> > > > -#define BOOTEFI_NAME "bootarm.efi"
> > > > -#elif defined(CONFIG_X86_RUN_32BIT)
> > > > -#define BOOTEFI_NAME "bootia32.efi"
> > > > -#elif defined(CONFIG_X86_RUN_64BIT)
> > > > -#define BOOTEFI_NAME "bootx64.efi"
> > > > -#elif defined(CONFIG_ARCH_RV32I)
> > > > -#define BOOTEFI_NAME "bootriscv32.efi"
> > > > -#elif defined(CONFIG_ARCH_RV64I)
> > > > -#define BOOTEFI_NAME "bootriscv64.efi"
> > > > -#endif
> > > > +#include <efi_default_filename.h>
> > > >   #endif
> > > >
> > > >   #ifdef BOOTEFI_NAME
> > > > diff --git a/include/efi_default_filename.h b/include/efi_default_filename.h
> > > > new file mode 100644
> > > > index 0000000000..cb2ef9e131
> > > > --- /dev/null
> > > > +++ b/include/efi_default_filename.h
> > > > @@ -0,0 +1,33 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > > +/*
> > > > + * Default boot file name when none is present in the FilePath.
> > > > + * This is defined in the UEFI specification.
> > > > + *
> > > > + * Copyright (c) 2022, Linaro Limited
> > > > + */
> > > > +#ifndef _EFI_DEFAULT_FILENAME_H
> > > > +#define _EFI_DEFAULT_FILENAME_H
> > > > +
> > > > +#if defined(CONFIG_ARM64)
> > > > +#define BOOTEFI_NAME "BOOTAA64.EFI"
> > > > +#elif defined(CONFIG_ARM)
> > > > +#define BOOTEFI_NAME "BOOTARM.EFI"
> > > > +#elif defined(CONFIG_X86_64)
> > > > +#define BOOTEFI_NAME "BOOTX64.EFI"
> > > > +#elif defined(CONFIG_X86)
> > > > +#define BOOTEFI_NAME "BOOTIA32.EFI"
> > > > +#elif defined(CONFIG_ARCH_RV32I)
> > > > +#define BOOTEFI_NAME "BOOTRISCV32.EFI"
> > > > +#elif defined(CONFIG_ARCH_RV64I)
> > > > +#define BOOTEFI_NAME "BOOTRISCV64.EFI"
> > > > +#elif defined(CONFIG_SANDBOX)
> > > > +/*
> > > > + * SANDBOX is not defined in UEFI specification, but
> > > > + * this definition avoids build failure for SANDBOX.
> > > > + */
> > > > +#define BOOTEFI_NAME "BOOTSANDBOX.EFI"
> > > 
> > > The sandbox should boot the default binary for the host architecture:
> > > 
> > > #ifndef _EFI_DEFAULT_FILENAME_H
> > > #define _EFI_DEFAULT_FILENAME_H
> > > 
> > > #include <host_arch.h>
> > > 
> > > #undef BOOTEFI_NAME
> > > 
> > > #if HOST_ARCH == HOST_ARCH_X86_64
> > > #define BOOTEFI_NAME "BOOTX64.EFI"
> > > #endif
> > > 
> > > #if HOST_ARCH == HOST_ARCH_X86
> > > #define BOOTEFI_NAME "BOOTIA32.EFI"
> > > #endif
> > > 
> > > #if HOST_ARCH == HOST_ARCH_AARCH64
> > > #define BOOTEFI_NAME "BOOTAA64.EFI"
> > > #endif
> > > 
> > > #if HOST_ARCH == HOST_ARCH_ARM
> > > #define BOOTEFI_NAME "BOOTARM.EFI"
> > > #endif
> > > 
> > > #if HOST_ARCH == HOST_ARCH_RISCV32
> > > #define BOOTEFI_NAME "BOOTRISCV32.EFI"
> > > #endif
> > > 
> > > #if HOST_ARCH == HOST_ARCH_RISCV64
> > > #define BOOTEFI_NAME "BOOTRISCV64.EFI"
> > > #endif
> > > 
> > > #ifndef BOOTEFI_NAME
> > > #error Unsupported UEFI architecture
> > > #endif
> > > 
> > > #endif
> > 
> > Maybe sanbox is special, but using the host architecture for actual
> > boards makes no sense.  I see this has made its way into master
> > already, but when I cross-build for apple_m1_defconfig on an amd64
> > machine I end up with:
> > 
> >     $ strings ./lib/efi_loader/efi_bootmgr.o | grep BOOT
> >     /EFI/BOOT/BOOTX64.EFI
> > 
> > The original diff that used CONFIG_ARM64, CONFIG_ARM, etc, did this
> > right.
> 
> Hmm, forget about that.  The problem is in the sed expression in the
> toplevel Makefile that is used to MK_ARCH that ends up setting
> HOST_ARCH incorrectly.  Using a variable named HOST_ARCH to hold the
> target architecture doesn't help understanding what's going wrong here
> of course...

I can nod to Mark's comment here.
Even though HOST_ARCH might work, it's quite confusing.
Apparently it looks wrong as Mark said.

Please address this comment properly as you modified my patch.

What I basically recommend is:
* In (top-dir)/Makefile
  - s/HOST_ARCH/MK_ARCH/g
  - add
      if (CONFIG_SANDBOX)
          HOST_ARCH = $(MK_ARCH)
  - remove "undefine MK_ARCH"
* In efi_default_filename.h
  - s/HOST_ARCH/MK_ARCH/g

This way, HOST_ARCH is only used in sandbox-related code.

-Takahiro Akashi