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

[Mark Kettenis <mark.kettenis@xs4all.nl>]

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