On Thu, 4 Apr 2024 at 14:49, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

The default sequence of boot methods is determined by alphabetical sorting during linkage.

• efi_mgr must run before efi to be UEFI compliant
• pxe should run as last resort

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

v2: no change

---

boot/bootmeth_efi.c | 2 +- boot/bootmeth_efi_mgr.c | 2 +- boot/bootmeth_pxe.c | 2 +- test/boot/bootflow.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index c4eb331d69e..a46b6c9c805 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -489,7 +489,7 @@ static const struct udevice_id distro_efi_bootmeth_ids[] = { { } };

-U_BOOT_DRIVER(bootmeth_efi) = { +U_BOOT_DRIVER(bootmeth_4efi) = { .name = "bootmeth_efi", .id = UCLASS_BOOTMETH, .of_match = distro_efi_bootmeth_ids, diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c index ed29d7ef021..b7d429f2c3d 100644 --- a/boot/bootmeth_efi_mgr.c +++ b/boot/bootmeth_efi_mgr.c @@ -114,7 +114,7 @@ static const struct udevice_id efi_mgr_bootmeth_ids[] = { { } };

-U_BOOT_DRIVER(bootmeth_efi_mgr) = { +U_BOOT_DRIVER(bootmeth_3efi_mgr) = { .name = "bootmeth_efi_mgr", .id = UCLASS_BOOTMETH, .of_match = efi_mgr_bootmeth_ids, diff --git a/boot/bootmeth_pxe.c b/boot/bootmeth_pxe.c index 8d489a11aa4..70f693aa239 100644 --- a/boot/bootmeth_pxe.c +++ b/boot/bootmeth_pxe.c @@ -184,7 +184,7 @@ static const struct udevice_id extlinux_bootmeth_pxe_ids[] = { { } };

-U_BOOT_DRIVER(bootmeth_pxe) = { +U_BOOT_DRIVER(bootmeth_zpxe) = { .name = "bootmeth_pxe", .id = UCLASS_BOOTMETH, .of_match = extlinux_bootmeth_pxe_ids, diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index 4845b7121c8..e60e9309fa9 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -377,7 +377,7 @@ static int bootflow_system(struct unit_test_state *uts) if (!IS_ENABLED(CONFIG_EFI_BOOTMGR)) return -EAGAIN; ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));

•   ut_assertok(device_bind(bootstd, DM_DRIVER_GET(bootmeth_efi_mgr),
  

•   ut_assertok(device_bind(bootstd, DM_DRIVER_GET(bootmeth_3efi_mgr),
                            "efi_mgr", 0, ofnode_null(), &dev));
    ut_assertok(device_probe(dev));
    sandbox_set_fake_efi_mgr_dev(dev, true);
  

-- 2.43.0

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On 04.04.24 17:18, Mark Kettenis wrote:

...

From: Heinrich Schuchardt heinrich.schuchardt@canonical.com Date: Thu, 4 Apr 2024 13:47:39 +0200

The default sequence of boot methods is determined by alphabetical sorting during linkage.

• efi_mgr must run before efi to be UEFI compliant
• pxe should run as last resort

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

This sound very much like you're working around a standard boot design flaw.

Who does the alphabetical sorting here? Is that just a result of how the toolchain behaves? If so, that sounds fragile to me and I think it would be better to make the order explicit in the code.

Thanks for reviewing.

Please, observe the SORT command in our linker script lines

KEEP(*(SORT(__u_boot_list*)));

This ensures that linker generated lists are always alphabetically sorted. We rely on this in other places too. See part_driver_lookup_type() where GPT must be tested before FAT (commit 96e5b03c8ab7 ("dm: part: Convert partition API use to linker lists").

Best regards

Heinrich

Cheers,

Mark

...

---

v2: no change

---

boot/bootmeth_efi.c | 2 +- boot/bootmeth_efi_mgr.c | 2 +- boot/bootmeth_pxe.c | 2 +- test/boot/bootflow.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index c4eb331d69e..a46b6c9c805 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -489,7 +489,7 @@ static const struct udevice_id distro_efi_bootmeth_ids[] = { { } };

-U_BOOT_DRIVER(bootmeth_efi) = { +U_BOOT_DRIVER(bootmeth_4efi) = { .name = "bootmeth_efi", .id = UCLASS_BOOTMETH, .of_match = distro_efi_bootmeth_ids, diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c index ed29d7ef021..b7d429f2c3d 100644 --- a/boot/bootmeth_efi_mgr.c +++ b/boot/bootmeth_efi_mgr.c @@ -114,7 +114,7 @@ static const struct udevice_id efi_mgr_bootmeth_ids[] = { { } };

-U_BOOT_DRIVER(bootmeth_efi_mgr) = { +U_BOOT_DRIVER(bootmeth_3efi_mgr) = { .name = "bootmeth_efi_mgr", .id = UCLASS_BOOTMETH, .of_match = efi_mgr_bootmeth_ids, diff --git a/boot/bootmeth_pxe.c b/boot/bootmeth_pxe.c index 8d489a11aa4..70f693aa239 100644 --- a/boot/bootmeth_pxe.c +++ b/boot/bootmeth_pxe.c @@ -184,7 +184,7 @@ static const struct udevice_id extlinux_bootmeth_pxe_ids[] = { { } };

-U_BOOT_DRIVER(bootmeth_pxe) = { +U_BOOT_DRIVER(bootmeth_zpxe) = { .name = "bootmeth_pxe", .id = UCLASS_BOOTMETH, .of_match = extlinux_bootmeth_pxe_ids, diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index 4845b7121c8..e60e9309fa9 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -377,7 +377,7 @@ static int bootflow_system(struct unit_test_state *uts) if (!IS_ENABLED(CONFIG_EFI_BOOTMGR)) return -EAGAIN; ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));

• ut_assertok(device_bind(bootstd, DM_DRIVER_GET(bootmeth_efi_mgr),

• ut_assertok(device_bind(bootstd, DM_DRIVER_GET(bootmeth_3efi_mgr), "efi_mgr", 0, ofnode_null(), &dev)); ut_assertok(device_probe(dev)); sandbox_set_fake_efi_mgr_dev(dev, true);

-- 2.43.0

On Thu, 4 Apr 2024 at 14:49, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

If UEFI is enabled in U-Boot, we want it to conform to the UEFI specification. This requires enabling the boot manager boot method.

Reported-by: E Shattow lucent@gmail.com Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

---

v2: fix building with EFI_BOOT_MGR but w/o BOOTSTD

---

boot/Kconfig | 10 ++++++++++ boot/Makefile | 2 +- lib/efi_loader/Kconfig | 1 - 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/boot/Kconfig b/boot/Kconfig index 3d7aabd27d6..d9a6c270059 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -558,6 +558,16 @@ config BOOTMETH_EFILOADER

      This provides a way to try out standard boot on an existing boot flow.

+config BOOTMETH_EFI_BOOTMGR

•   bool "Bootdev support for EFI boot manager"
  

•   depends on EFI_BOOTMGR
  

•   select BOOTMETH_GLOBAL
  

•   default y
  

•   help
  

•     Enable booting via the UEFI boot manager. Based on the EFI variables
  

•     the EFI binary to be launched is determined. To set the EFI variables
  

•     use the eficonfig command.
  

config BOOTMETH_VBE bool "Bootdev support for Verified Boot for Embedded" depends on FIT diff --git a/boot/Makefile b/boot/Makefile index f0a279cde16..84ccfeaecec 100644 --- a/boot/Makefile +++ b/boot/Makefile @@ -34,8 +34,8 @@ obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_CROS) += bootm.o bootm_os.o bootmeth_cros.o obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SANDBOX) += bootmeth_sandbox.o obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SCRIPT) += bootmeth_script.o obj-$(CONFIG_$(SPL_TPL_)CEDIT) += cedit.o +obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_EFI_BOOTMGR) += bootmeth_efi_mgr.o ifdef CONFIG_$(SPL_TPL_)BOOTSTD_FULL -obj-$(CONFIG_EFI_BOOTMGR) += bootmeth_efi_mgr.o obj-$(CONFIG_$(SPL_TPL_)EXPO) += bootflow_menu.o obj-$(CONFIG_$(SPL_TPL_)BOOTSTD) += bootflow_menu.o endif diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index a7c3e05c13a..e13a6f9f4c3 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -44,7 +44,6 @@ config EFI_BINARY_EXEC config EFI_BOOTMGR bool "UEFI Boot Manager" default y

•   select BOOTMETH_GLOBAL if BOOTSTD
    help
      Select this option if you want to select the UEFI binary to be booted
      via UEFI variables Boot####, BootOrder, and BootNext. You should also
  

-- 2.43.0

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On Thu, 4 Apr 2024 at 14:50, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

efi_default_filename.h requires HOST_ARCH to be defined. Up to now we defined it via a CFLAGS. This does not scale. Add the symbol to version_autogenerated.h instead.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

v2: new patch

---

Makefile | 1 + arch/sandbox/config.mk | 2 -- include/host_arch.h | 2 ++ lib/efi_loader/Makefile | 3 +-- 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile index ba660976547..1069adca91e 100644 --- a/Makefile +++ b/Makefile @@ -1963,6 +1963,7 @@ define filechk_version.h echo #define U_BOOT_VERSION_NUM $(VERSION); \ echo #define U_BOOT_VERSION_NUM_PATCH $$(echo $(PATCHLEVEL) | \ sed -e "s/^0*//"); \

•   echo \#define HOST_ARCH $(HOST_ARCH); \
    echo \#define CC_VERSION_STRING \"$$(LC_ALL=C $(CC) --version | head -n 1)\"; \
    echo \#define LD_VERSION_STRING \"$$(LC_ALL=C $(LD) --version | head -n 1)\"; )
  

endef diff --git a/arch/sandbox/config.mk b/arch/sandbox/config.mk index 1d50991f8d2..405843800e9 100644 --- a/arch/sandbox/config.mk +++ b/arch/sandbox/config.mk @@ -69,5 +69,3 @@ EFI_LDS := ${SRCDIR}/../../../arch/riscv/lib/elf_riscv64_efi.lds endif EFI_CRT0 := crt0_sandbox_efi.o EFI_RELOC := reloc_sandbox_efi.o -AFLAGS_crt0_sandbox_efi.o += -DHOST_ARCH="$(HOST_ARCH)" -CFLAGS_reloc_sandbox_efi.o += -DHOST_ARCH="$(HOST_ARCH)" diff --git a/include/host_arch.h b/include/host_arch.h index 169d4945136..261194bd7c6 100644 --- a/include/host_arch.h +++ b/include/host_arch.h @@ -16,6 +16,8 @@ export HOST_ARCH_X86=0x0386 export HOST_ARCH_X86_64=0x8664 #endif

+#include <version.h>

#define HOST_ARCH_AARCH64 0xaa64 #define HOST_ARCH_ARM 0x00a7 #define HOST_ARCH_RISCV32 0x5032 diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index fcb0af7e7d6..086521fb287 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -6,8 +6,7 @@ # This file only gets included with CONFIG_EFI_LOADER set, so all # object inclusion implicitly depends on it

-asflags-y += -DHOST_ARCH="$(HOST_ARCH)" -I. -ccflags-y += -DHOST_ARCH="$(HOST_ARCH)" +asflags-y += -I.

CFLAGS_efi_boottime.o += \

-DFW_VERSION="0x$(VERSION)" \

2.43.0

Acked-by: Ilias Apalodimas ilias.apalodimas@linaro.org

Hi Heinrich,

FWIW reading at this, I don't see a point of having it overall. It has a nice feature of trying to load external DTBs from a path though. I think in the future we can move the dtb selection in bootmeth_efi_mgr.c and delete this one

On Thu, 4 Apr 2024 at 14:49, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

• The sandbox must not use an arbitrary file name bootsbox.efi but the file name matching the host architecture to properly boot the respective file. We already have an include which provides a macro with the name of the EFI binary. Use it.

• The path to the EFI binary should be absolute.

• The path and the file name must be capitalized to conform to the UEFI specification. This is important when reading from case sensitive file systems.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

v2: new patch

---

boot/bootmeth_efi.c | 44 +++----------------------------------------- test/boot/bootflow.c | 4 +++- 2 files changed, 6 insertions(+), 42 deletions(-)

diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index a46b6c9c805..aebc5207fc0 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -14,6 +14,7 @@ #include <bootmeth.h> #include <command.h> #include <dm.h> +#include <efi_default_filename.h> #include <efi_loader.h> #include <fs.h> #include <malloc.h> @@ -23,43 +24,7 @@ #include <pxe_utils.h> #include <linux/sizes.h>

-#define EFI_DIRNAME "efi/boot/"

-/**

• • get_efi_leafname() - Get the leaf name for the EFI file we expect
• • @str: Place to put leaf name for this architecture, e.g. "bootaa64.efi".

• • Must have at least 16 bytes of space
    

• • @max_len: Length of @str, must be >=16
• */

-static int get_efi_leafname(char *str, int max_len) -{

•   const char *base;
  

•   if (max_len < 16)
  

•           return log_msg_ret("spc", -ENOSPC);
  

•   if (IS_ENABLED(CONFIG_ARM64))
  

•           base = "bootaa64";
  

•   else if (IS_ENABLED(CONFIG_ARM))
  

•           base = "bootarm";
  

•   else if (IS_ENABLED(CONFIG_X86_RUN_32BIT))
  

•           base = "bootia32";
  

•   else if (IS_ENABLED(CONFIG_X86_RUN_64BIT))
  

•           base = "bootx64";
  

•   else if (IS_ENABLED(CONFIG_ARCH_RV32I))
  

•           base = "bootriscv32";
  

•   else if (IS_ENABLED(CONFIG_ARCH_RV64I))
  

•           base = "bootriscv64";
  

•   else if (IS_ENABLED(CONFIG_SANDBOX))
  

•           base = "bootsbox";
  

•   else
  

•           return -EINVAL;
  

•   strcpy(str, base);
  

•   strcat(str, ".efi");
  

•   return 0;
  

-} +#define EFI_DIRNAME "/EFI/BOOT/"

static int get_efi_pxe_arch(void) { @@ -259,10 +224,7 @@ static int distro_efi_try_bootflow_files(struct udevice *dev, return -ENOENT;

    strcpy(fname, EFI_DIRNAME);

•   ret = get_efi_leafname(fname + strlen(fname),
  

•                          sizeof(fname) - strlen(fname));
  

•   if (ret)
  

•           return log_msg_ret("leaf", ret);
  

•   strcat(fname, BOOTEFI_NAME);
  
    if (bflow->blk)
             desc = dev_get_uclass_plat(bflow->blk);
  

diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index e60e9309fa9..674d4c05f83 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -13,6 +13,7 @@ #include <bootstd.h> #include <cli.h> #include <dm.h> +#include <efi_default_filename.h> #include <expo.h> #ifdef CONFIG_SANDBOX #include <asm/test.h> @@ -179,7 +180,8 @@ static int bootflow_cmd_scan_e(struct unit_test_state *uts) ut_assert_nextline(" 3 efi media mmc 0 mmc1.bootdev.whole "); ut_assert_nextline(" ** No partition found, err=-2: No such file or directory"); ut_assert_nextline(" 4 extlinux ready mmc 1 mmc1.bootdev.part_1 /extlinux/extlinux.conf");

•   ut_assert_nextline("  5  efi          fs      mmc          1  mmc1.bootdev.part_1       efi/boot/bootsbox.efi");
  

•   ut_assert_nextline("  5  efi          fs      mmc          1  mmc1.bootdev.part_1       /EFI/BOOT/"
  

•                      BOOTEFI_NAME);
  
    ut_assert_skip_to_line("Scanning bootdev 'mmc0.bootdev':");
    ut_assert_skip_to_line(
  

-- 2.43.0

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org Tested-by: Ilias Apalodimas ilias.apalodimas@linaro.org