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