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

[Heinrich Schuchardt]

Am 12. Mai 2022 11:12:47 MESZ schrieb AKASHI Takahiro <takahiro.akashi at 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 at xs4all.nl>
>> > 
>> > > Date: Fri, 29 Apr 2022 19:03:22 +0200
>> > > From: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> > > 
>> > > On 4/28/22 10:09, Masahisa Kojima wrote:
>> > > > From: AKASHI Takahiro <takahiro.akashi at 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 at linaro.org>
>> > > > Signed-off-by: Masahisa Kojima <masahisa.kojima at 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.

TARGET_ARCH would be a better variable name.

Best regards

Heinrich

>
>-Takahiro Akashi