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:35:36 +0200
> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> On 5/5/22 14:20, Heinrich Schuchardt wrote:
> > On 5/5/22 14:05, Mark Kettenis wrote:
> >>> 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
> >
> > Thanks for reporting the issue.
> >
> > On Sandbox it should be the host architecture. on other defconfigs the
> > target architecture.
> 
> On Ubuntu 22.04:
> 
> git reset --hard 1739a6db5403d187902dcebca548de0644c8078f
> make apple_m1_defconfig
> CROSS_COMPILE=aarch64-linux-gnu- make -j16
> 
> $ strings ./lib/efi_loader/efi_bootmgr.o | grep BOOT
> /EFI/BOOT/BOOTAA64.EFI
> UCLASS_REBOOT_MODE
> UCLASS_BOOTSTD
> EFI_OBJECT_TYPE_U_BOOT_FIRMWARE
> UCLASS_BOOTDEV
> UCLASS_BOOTMETH
> UCLASS_BOOTCOUN
> 
> $ strings u-boot | grep 'BOOT\S*\.EFI'
> /EFI/BOOT/BOOTAA64.EFI
> 
> How can I reproduce your problem?
> Is this BSD specific? Which distro are you running?

Yes, I'm always building on OpenBSD.  And the probablem is the use of
non-portable GNU sed extensions \s and \S.  Replacing those with
[[:space:]] and [^[:space:]] as in the diff below seems to work.
Decoding chicken scratches is alway fun...

This has been broken for a while, but didn't cause any build issues
until now.  I'll send a proper patch to the mailing list.


diff --git a/Makefile b/Makefile
index ea80f00716..6eceeb35b4 100644
--- a/Makefile
+++ b/Makefile
@@ -21,7 +21,7 @@ include include/host_arch.h
 ifeq ("", "$(CROSS_COMPILE)")
   MK_ARCH="${shell uname -m}"
 else
-  MK_ARCH="${shell echo $(CROSS_COMPILE) | sed -n 's/^\s*\([^\/]*\/\)*\([^-]*\)-\S*/\2/p'}"
+  MK_ARCH="${shell echo $(CROSS_COMPILE) | sed -n 's/^[[:space:]]*\([^\/]*\/\)*\([^-]*\)-[^[:space:]]*/\2/p'}"
 endif
 unexport HOST_ARCH
 ifeq ("x86_64", $(MK_ARCH))