[RFC 0/8] boot: run EFI boot manager after block devices

[RFC 0/8] boot: run EFI boot manager after block devices

[Heinrich Schuchardt]

Up to now the EFI boot manager is running as a global boot method before
scanning block devices. This leads to failures to boot from unprobed
devices. Scanning all devices before running the EFI boot manager is
not wanted by some board maintainers.

Introduce a dummy bootdev with a priority between block devices and
network devices.

Add the dummy bootdeb to the QEMU ARM environment.

Do not run the EFI bootdev as we must not run EFI\BOOT\BOOTARCH.EFI
before the boot manager.

Heinrich Schuchardt (8):
  boot: EFI boot manager does not depend on BootOrder
  boot: BOOTMETH_DISTRO should select BOOTMETH_EFI_BOOTMGR
  boot: make BOOTMETH_EFILOADER default=n
  boot: introduce BOOTDEVP_6_EFI
  boot: prevent recursion in bootdev_hunt_drv()
  boot: make EFI boot manager bootmeth non-global
  efi_loader: create EFI BOOTDEV
  board/emulation/qemu-arm: enable EFI boot manager

 board/emulation/qemu-arm/qemu-arm.env   |   2 +-
 board/emulation/qemu-sbsa/qemu-sbsa.env |   2 +-
 boot/Kconfig                            |   6 +-
 boot/bootdev-uclass.c                   |   2 +-
 boot/bootmeth_efi_mgr.c                 |  24 +-----
 include/bootdev.h                       |  10 ++-
 include/dm/uclass-id.h                  |   1 +
 lib/efi_loader/Makefile                 |   1 +
 lib/efi_loader/efi_bootdev.c            | 102 ++++++++++++++++++++++++
 net/eth_bootdev.c                       |   4 +-
 test/boot/bootflow.c                    |   2 +-
 11 files changed, 124 insertions(+), 32 deletions(-)
 create mode 100644 lib/efi_loader/efi_bootdev.c

-- 
2.48.1

[RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Heinrich Schuchardt]

The EFI boot manager bootmeth does not require variable BootOrder to be
preexisting. It creates this variable.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 boot/bootmeth_efi_mgr.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
index 42b8863815e..1669cbed5bd 100644
--- a/boot/bootmeth_efi_mgr.c
+++ b/boot/bootmeth_efi_mgr.c
@@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter)
 
 static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow)
 {
-	struct efi_mgr_priv *priv = dev_get_priv(dev);
-	efi_status_t ret;
-	efi_uintn_t size;
-	u16 *bootorder;
-
-	if (priv->fake_dev) {
-		bflow->state = BOOTFLOWST_READY;
-		return 0;
-	}
+	int ret
 
 	ret = efi_init_obj_list();
 	if (ret)
 		return log_msg_ret("init", ret);
 
-	/* Enable this method if the "BootOrder" UEFI exists. */
-	bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid,
-				&size);
-	if (bootorder) {
-		free(bootorder);
-		bflow->state = BOOTFLOWST_READY;
-		return 0;
-	}
+	bflow->state = BOOTFLOWST_READY;
 
-	return -EINVAL;
+	return 0;
 }
 
 static int efi_mgr_read_file(struct udevice *dev, struct bootflow *bflow,
-- 
2.48.1

[RFC 2/8] boot: BOOTMETH_DISTRO should select BOOTMETH_EFI_BOOTMGR

[Heinrich Schuchardt]

Distros expect the EFI boot manager to run. It falls back to launching
EFI\BOOT\BOOT<ARCH>.EFI from the ESP.

BOOTMETH_EFILOADER is obsolete.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 boot/Kconfig | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/boot/Kconfig b/boot/Kconfig
index fb37d912bc9..71ddb37f6bb 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -597,6 +597,9 @@ config BOOTMETH_EFILOADER
 	imply CMD_TFTPBOOT if CMD_NET
 	default y
 	help
+	  This bootmeth is obsolete. BOOTMETH_EFI_BOOTMGR takes care of
+	  launching EFI\BOOT\BOOT<ARCH>.EFI if not boot option matches.
+
 	  Enables support for EFI boot using bootdevs. This makes the
 	  bootdevs look for a 'boot<arch>.efi' on each filesystem
 	  they scan. The resulting file is booted after enabling U-Boot's
@@ -648,7 +651,7 @@ config BOOTMETH_DISTRO
 	select BOOTMETH_SCRIPT if CMDLINE # E.g. Armbian uses scripts
 	select BOOTMETH_EXTLINUX  # E.g. Debian uses these
 	select BOOTMETH_EXTLINUX_PXE if CMD_PXE && CMD_NET && DM_ETH
-	select BOOTMETH_EFILOADER if EFI_BINARY_EXEC # E.g. Ubuntu uses this
+	select BOOTMETH_EFI_BOOTMGR if EFI_BINARY_EXEC # E.g. Ubuntu uses this
 
 config SPL_BOOTMETH_VBE
 	bool "Bootdev support for Verified Boot for Embedded (SPL)"
-- 
2.48.1

[RFC 3/8] boot: make BOOTMETH_EFILOADER default=n

[Heinrich Schuchardt]

The boot method is deprecated as the EFI boot manager covers this
functionality.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 boot/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/boot/Kconfig b/boot/Kconfig
index 71ddb37f6bb..b973d9eb0a7 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -595,7 +595,6 @@ config BOOTMETH_EFILOADER
 	bool "Bootdev support for EFI boot"
 	depends on EFI_BINARY_EXEC
 	imply CMD_TFTPBOOT if CMD_NET
-	default y
 	help
 	  This bootmeth is obsolete. BOOTMETH_EFI_BOOTMGR takes care of
 	  launching EFI\BOOT\BOOT<ARCH>.EFI if not boot option matches.
-- 
2.48.1

[RFC 4/8] boot: introduce BOOTDEVP_6_EFI

[Heinrich Schuchardt]

The EFI boot manager requires access to all block devices and to network.
So hunting of bootdevs must have been completed. It does not require
DHCP to have completed. So priority-wise it fits best after all bootdevs
except for the 'dummy' network boot devices used for PXE booting.

Create a new bootdev priority for the EFI boot manager which is sorted
after all other bootdevs except network.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 include/bootdev.h | 10 ++++++----
 net/eth_bootdev.c |  4 ++--
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/bootdev.h b/include/bootdev.h
index 12c90c4ec1b..6f46451da73 100644
--- a/include/bootdev.h
+++ b/include/bootdev.h
@@ -36,9 +36,10 @@ struct udevice;
  * @BOOTDEVP_5_SCAN_SLOW: Extenal devices which need scanning or bus
  * enumeration to find. The enumeration takes significant fraction of a second
  * to complete
- * @BOOTDEVP_6_NET_BASE: Basic network devices which are quickly and easily
+ * @BOOTDEVP_6_EFI: EFI boot manager
+ * @BOOTDEVP_7_NET_BASE: Basic network devices which are quickly and easily
  * available. Typically used for an internal Ethernet device
- * @BOOTDEVP_7_NET_FALLBACK: Secondary network devices which require extra time
+ * @BOOTDEVP_8_NET_FALLBACK: Secondary network devices which require extra time
  * to start up, or are less desirable. Typically used for secondary Ethernet
  * devices. Note that USB ethernet devices are found during USB enumeration,
  * so do not use this priority
@@ -50,8 +51,9 @@ enum bootdev_prio_t {
 	BOOTDEVP_3_INTERNAL_SLOW,
 	BOOTDEVP_4_SCAN_FAST,
 	BOOTDEVP_5_SCAN_SLOW,
-	BOOTDEVP_6_NET_BASE,
-	BOOTDEVP_7_NET_FALLBACK,
+	BOOTDEVP_6_EFI,
+	BOOTDEVP_7_NET_BASE,
+	BOOTDEVP_8_NET_FALLBACK,
 
 	BOOTDEVP_COUNT,
 };
diff --git a/net/eth_bootdev.c b/net/eth_bootdev.c
index b0fca6e8313..dd071853886 100644
--- a/net/eth_bootdev.c
+++ b/net/eth_bootdev.c
@@ -59,7 +59,7 @@ static int eth_bootdev_bind(struct udevice *dev)
 {
 	struct bootdev_uc_plat *ucp = dev_get_uclass_plat(dev);
 
-	ucp->prio = BOOTDEVP_6_NET_BASE;
+	ucp->prio = BOOTDEVP_7_NET_BASE;
 
 	return 0;
 }
@@ -117,7 +117,7 @@ U_BOOT_DRIVER(eth_bootdev) = {
 };
 
 BOOTDEV_HUNTER(eth_bootdev_hunt) = {
-	.prio		= BOOTDEVP_6_NET_BASE,
+	.prio		= BOOTDEVP_7_NET_BASE,
 	.uclass		= UCLASS_ETH,
 	.hunt		= eth_bootdev_hunt,
 	.drv		= DM_DRIVER_REF(eth_bootdev),
-- 
2.48.1

[RFC 5/8] boot: prevent recursion in bootdev_hunt_drv()

[Heinrich Schuchardt]

If an implementation of the hunt() method calls bootdev_hunt() itself,
an infinite recursion may occur.

Set the sequence bit before invoking hunt().

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 boot/bootdev-uclass.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/boot/bootdev-uclass.c b/boot/bootdev-uclass.c
index 3791ebfcb42..da757a49481 100644
--- a/boot/bootdev-uclass.c
+++ b/boot/bootdev-uclass.c
@@ -791,6 +791,7 @@ static int bootdev_hunt_drv(struct bootdev_hunter *info, uint seq, bool show)
 		return log_msg_ret("std", ret);
 
 	if (!(std->hunters_used & BIT(seq))) {
+		std->hunters_used |= BIT(seq);
 		if (show)
 			printf("Hunting with: %s\n",
 			       uclass_get_name(info->uclass));
@@ -801,7 +802,6 @@ static int bootdev_hunt_drv(struct bootdev_hunter *info, uint seq, bool show)
 			if (ret && ret != -ENOENT)
 				return ret;
 		}
-		std->hunters_used |= BIT(seq);
 	}
 
 	return 0;
-- 
2.48.1

[RFC 6/8] boot: make EFI boot manager bootmeth non-global

[Heinrich Schuchardt]

Global bootmeths are called before all device specific bootmeths.

To move the EFI boot manager after the block devices it must be non-global.

As sorting will be controlled boot device priority remove the digit 3 from
the name that was only used for sorting reasons.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 boot/bootmeth_efi_mgr.c | 3 +--
 test/boot/bootflow.c    | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
index 1669cbed5bd..8f1d247c942 100644
--- a/boot/bootmeth_efi_mgr.c
+++ b/boot/bootmeth_efi_mgr.c
@@ -82,7 +82,6 @@ static int bootmeth_efi_mgr_bind(struct udevice *dev)
 	struct bootmeth_uc_plat *plat = dev_get_uclass_plat(dev);
 
 	plat->desc = "EFI bootmgr flow";
-	plat->flags = BOOTMETHF_GLOBAL;
 
 	return 0;
 }
@@ -99,7 +98,7 @@ static const struct udevice_id efi_mgr_bootmeth_ids[] = {
 	{ }
 };
 
-U_BOOT_DRIVER(bootmeth_3efi_mgr) = {
+U_BOOT_DRIVER(bootmeth_efi_mgr) = {
 	.name		= "bootmeth_efi_mgr",
 	.id		= UCLASS_BOOTMETH,
 	.of_match	= efi_mgr_bootmeth_ids,
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index 5f9c037ff53..db35dad10d3 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -395,7 +395,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_3efi_mgr),
+	ut_assertok(device_bind(bootstd, DM_DRIVER_GET(bootmeth_efi_mgr),
 				"efi_mgr", 0, ofnode_null(), &dev));
 	ut_assertok(device_probe(dev));
 	sandbox_set_fake_efi_mgr_dev(dev, true);
-- 
2.48.1

[RFC 7/8] efi_loader: create EFI BOOTDEV

[Heinrich Schuchardt]

Create a bootdev device for the EFI boot manager.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 include/dm/uclass-id.h       |   1 +
 lib/efi_loader/Makefile      |   1 +
 lib/efi_loader/efi_bootdev.c | 102 +++++++++++++++++++++++++++++++++++
 3 files changed, 104 insertions(+)
 create mode 100644 lib/efi_loader/efi_bootdev.c

diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index 270088ad94f..d0795c13d14 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -59,6 +59,7 @@ enum uclass_id {
 	UCLASS_ECDSA,		/* Elliptic curve cryptographic device */
 	UCLASS_EFI_LOADER,	/* Devices created by UEFI applications */
 	UCLASS_EFI_MEDIA,	/* Devices provided by UEFI firmware */
+	UCLASS_EFI_BOOT_MGR,	/* EFI boot manager */
 	UCLASS_ETH,		/* Ethernet device */
 	UCLASS_ETH_PHY,		/* Ethernet PHY device */
 	UCLASS_EXTCON,		/* External Connector Class */
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index ab50a69e48b..c37f05d70e4 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -25,6 +25,7 @@ apps-$(CONFIG_BOOTEFI_TESTAPP_COMPILE) += testapp
 obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
 obj-$(CONFIG_EFI_BOOTMGR) += efi_bootmgr.o
 obj-$(CONFIG_EFI_BINARY_EXEC) += efi_bootbin.o
+obj-$(CONFIG_BOOTMETH_EFI_BOOTMGR) += efi_bootdev.o
 obj-y += efi_boottime.o
 obj-y += efi_helper.o
 obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
diff --git a/lib/efi_loader/efi_bootdev.c b/lib/efi_loader/efi_bootdev.c
new file mode 100644
index 00000000000..066df9462c8
--- /dev/null
+++ b/lib/efi_loader/efi_bootdev.c
@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Bootdev for boot manager
+ *
+ * Copyright 2025, Heinrich Schuchardt <xypron.glpk@gmx.de>
+ */
+
+#define LOG_CATEGORY UCLASS_BOOTSTD
+
+#include <bootdev.h>
+#include <bootflow.h>
+#include <dm.h>
+#include <dm/lists.h>
+
+static int efi_get_bootflow(struct udevice *dev, struct bootflow_iter *iter,
+			    struct bootflow *bflow)
+{
+	int ret;
+
+	ret = strcmp(iter->method->name, "efi_mgr");
+	if (!ret)
+		bflow->state = BOOTFLOWST_READY;
+
+	return ret;
+}
+
+struct bootdev_ops efi_bootdev_ops = {
+	.get_bootflow	= efi_get_bootflow,
+};
+
+static int efi_bootdev_bind(struct udevice *dev)
+{
+	struct bootdev_uc_plat *ucp = dev_get_uclass_plat(dev);
+
+	ucp->prio = BOOTDEVP_6_EFI;
+
+	return 0;
+}
+
+static const struct udevice_id efi_bootdev_ids[] = {
+	{ .compatible = "u-boot,bootdev-efi" },
+	{ }
+};
+
+U_BOOT_DRIVER(efi_bootdev) = {
+	.name		= "efi_bootdev",
+	.id		= UCLASS_BOOTDEV,
+	.ops		= &efi_bootdev_ops,
+	.bind		= efi_bootdev_bind,
+	.of_match	= efi_bootdev_ids,
+};
+
+/**
+ * efi_bootdev_hunt() - find devices used by EFI boot manager
+ *
+ * Invoke all hunters that have not been invoked by now.
+ * This should only be the network hunter.
+ *
+ * @info:	info structure describing this hunter
+ * @show:	true to show information from the hunter
+ *
+ * Return:	0 if device found, -EINVAL otherwise
+ */
+static int efi_bootdev_hunt(struct bootdev_hunter *info, bool show)
+{
+	return bootdev_hunt(NULL, 0);
+}
+
+BOOTDEV_HUNTER(efi_bootdev_hunt) = {
+	.prio		= BOOTDEVP_6_EFI,
+	.uclass		= UCLASS_EFI_BOOT_MGR,
+	.hunt		= efi_bootdev_hunt,
+	.drv		= DM_DRIVER_REF(efi_bootdev),
+};
+
+static int efi_bootdev_create(void)
+{
+	int ret;
+	struct udevice *dev_mgr;
+	struct udevice *dev;
+
+	ret = device_bind_driver(gd->dm_root, "efimgr",
+				 "efimgr", &dev_mgr);
+	if (ret)
+		return ret;
+
+	ret = device_bind_driver(dev_mgr, "efi_bootdev",
+				 "efimgr.bootdev", &dev);
+	return ret;
+}
+
+EVENT_SPY_SIMPLE(EVT_DM_POST_INIT_R, efi_bootdev_create);
+
+U_BOOT_DRIVER(efimgr) = {
+	.name		= "efimgr",
+	.id		= UCLASS_EFI_BOOT_MGR,
+};
+
+UCLASS_DRIVER(efimgr) = {
+	.id		= UCLASS_EFI_BOOT_MGR,
+	.name		= "efimgr",
+};
-- 
2.48.1

[RFC 8/8] board/emulation/qemu-arm: enable EFI boot manager

[Heinrich Schuchardt]

Add EFI boot manager to boot targets.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 board/emulation/qemu-arm/qemu-arm.env   | 2 +-
 board/emulation/qemu-sbsa/qemu-sbsa.env | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/board/emulation/qemu-arm/qemu-arm.env b/board/emulation/qemu-arm/qemu-arm.env
index fb4adef281e..01d363baf2c 100644
--- a/board/emulation/qemu-arm/qemu-arm.env
+++ b/board/emulation/qemu-arm/qemu-arm.env
@@ -12,4 +12,4 @@ scriptaddr=0x40200000
 pxefile_addr_r=0x40300000
 kernel_addr_r=0x40400000
 ramdisk_addr_r=0x44000000
-boot_targets=qfw usb scsi virtio nvme dhcp
+boot_targets=qfw usb scsi virtio nvme efimgr dhcp
diff --git a/board/emulation/qemu-sbsa/qemu-sbsa.env b/board/emulation/qemu-sbsa/qemu-sbsa.env
index 88fdb0ec1c5..010c6172bdd 100644
--- a/board/emulation/qemu-sbsa/qemu-sbsa.env
+++ b/board/emulation/qemu-sbsa/qemu-sbsa.env
@@ -11,4 +11,4 @@ scriptaddr=0x100000300000
 pxefile_addr_r=0x10000400000
 kernel_addr_r=0x10000200000
 ramdisk_addr_r=0x10001000000
-boot_targets=qfw usb scsi virtio nvme dhcp
+boot_targets=qfw usb scsi virtio nvme efimgr dhcp
-- 
2.48.1

Re: [RFC 2/8] boot: BOOTMETH_DISTRO should select BOOTMETH_EFI_BOOTMGR

[Simon Glass]

Hi Heinrich,

On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Distros expect the EFI boot manager to run. It falls back to launching
> EFI\BOOT\BOOT<ARCH>.EFI from the ESP.
>
> BOOTMETH_EFILOADER is obsolete.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  boot/Kconfig | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/boot/Kconfig b/boot/Kconfig
> index fb37d912bc9..71ddb37f6bb 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -597,6 +597,9 @@ config BOOTMETH_EFILOADER
>         imply CMD_TFTPBOOT if CMD_NET
>         default y
>         help
> +         This bootmeth is obsolete. BOOTMETH_EFI_BOOTMGR takes care of
> +         launching EFI\BOOT\BOOT<ARCH>.EFI if not boot option matches.
> +

I don't agree with marking this as obsolete and your commit message
doesn't explain why it should be.

>           Enables support for EFI boot using bootdevs. This makes the
>           bootdevs look for a 'boot<arch>.efi' on each filesystem
>           they scan. The resulting file is booted after enabling U-Boot's
> @@ -648,7 +651,7 @@ config BOOTMETH_DISTRO
>         select BOOTMETH_SCRIPT if CMDLINE # E.g. Armbian uses scripts
>         select BOOTMETH_EXTLINUX  # E.g. Debian uses these
>         select BOOTMETH_EXTLINUX_PXE if CMD_PXE && CMD_NET && DM_ETH
> -       select BOOTMETH_EFILOADER if EFI_BINARY_EXEC # E.g. Ubuntu uses this
> +       select BOOTMETH_EFI_BOOTMGR if EFI_BINARY_EXEC # E.g. Ubuntu uses this
>
>  config SPL_BOOTMETH_VBE
>         bool "Bootdev support for Verified Boot for Embedded (SPL)"
> --
> 2.48.1
>

Regards,
Simon

Re: [RFC 7/8] efi_loader: create EFI BOOTDEV

[Simon Glass]

Hi Heinrich,

On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Create a bootdev device for the EFI boot manager.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  include/dm/uclass-id.h       |   1 +
>  lib/efi_loader/Makefile      |   1 +
>  lib/efi_loader/efi_bootdev.c | 102 +++++++++++++++++++++++++++++++++++
>  3 files changed, 104 insertions(+)
>  create mode 100644 lib/efi_loader/efi_bootdev.c
>
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 270088ad94f..d0795c13d14 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -59,6 +59,7 @@ enum uclass_id {
>         UCLASS_ECDSA,           /* Elliptic curve cryptographic device */
>         UCLASS_EFI_LOADER,      /* Devices created by UEFI applications */
>         UCLASS_EFI_MEDIA,       /* Devices provided by UEFI firmware */
> +       UCLASS_EFI_BOOT_MGR,    /* EFI boot manager */
>         UCLASS_ETH,             /* Ethernet device */
>         UCLASS_ETH_PHY,         /* Ethernet PHY device */
>         UCLASS_EXTCON,          /* External Connector Class */
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index ab50a69e48b..c37f05d70e4 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -25,6 +25,7 @@ apps-$(CONFIG_BOOTEFI_TESTAPP_COMPILE) += testapp
>  obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
>  obj-$(CONFIG_EFI_BOOTMGR) += efi_bootmgr.o
>  obj-$(CONFIG_EFI_BINARY_EXEC) += efi_bootbin.o
> +obj-$(CONFIG_BOOTMETH_EFI_BOOTMGR) += efi_bootdev.o
>  obj-y += efi_boottime.o
>  obj-y += efi_helper.o
>  obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
> diff --git a/lib/efi_loader/efi_bootdev.c b/lib/efi_loader/efi_bootdev.c
> new file mode 100644
> index 00000000000..066df9462c8
> --- /dev/null
> +++ b/lib/efi_loader/efi_bootdev.c
> @@ -0,0 +1,102 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Bootdev for boot manager
> + *
> + * Copyright 2025, Heinrich Schuchardt <xypron.glpk@gmx.de>
> + */
> +
> +#define LOG_CATEGORY UCLASS_BOOTSTD
> +
> +#include <bootdev.h>
> +#include <bootflow.h>
> +#include <dm.h>
> +#include <dm/lists.h>
> +
> +static int efi_get_bootflow(struct udevice *dev, struct bootflow_iter *iter,
> +                           struct bootflow *bflow)
> +{
> +       int ret;
> +
> +       ret = strcmp(iter->method->name, "efi_mgr");
> +       if (!ret)
> +               bflow->state = BOOTFLOWST_READY;
> +
> +       return ret;
> +}
> +
> +struct bootdev_ops efi_bootdev_ops = {
> +       .get_bootflow   = efi_get_bootflow,
> +};
> +
> +static int efi_bootdev_bind(struct udevice *dev)
> +{
> +       struct bootdev_uc_plat *ucp = dev_get_uclass_plat(dev);
> +
> +       ucp->prio = BOOTDEVP_6_EFI;
> +
> +       return 0;
> +}
> +
> +static const struct udevice_id efi_bootdev_ids[] = {
> +       { .compatible = "u-boot,bootdev-efi" },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(efi_bootdev) = {
> +       .name           = "efi_bootdev",
> +       .id             = UCLASS_BOOTDEV,
> +       .ops            = &efi_bootdev_ops,
> +       .bind           = efi_bootdev_bind,
> +       .of_match       = efi_bootdev_ids,
> +};
> +
> +/**
> + * efi_bootdev_hunt() - find devices used by EFI boot manager
> + *
> + * Invoke all hunters that have not been invoked by now.
> + * This should only be the network hunter.
> + *
> + * @info:      info structure describing this hunter
> + * @show:      true to show information from the hunter
> + *
> + * Return:     0 if device found, -EINVAL otherwise
> + */
> +static int efi_bootdev_hunt(struct bootdev_hunter *info, bool show)
> +{
> +       return bootdev_hunt(NULL, 0);

This recursive calling of hunting is very strange. Since bootmgr seems
to need all the hunting to be done, you could move this line to
somewhere in bootmgr.

> +}
> +
> +BOOTDEV_HUNTER(efi_bootdev_hunt) = {
> +       .prio           = BOOTDEVP_6_EFI,
> +       .uclass         = UCLASS_EFI_BOOT_MGR,
> +       .hunt           = efi_bootdev_hunt,
> +       .drv            = DM_DRIVER_REF(efi_bootdev),
> +};
> +
> +static int efi_bootdev_create(void)
> +{
> +       int ret;
> +       struct udevice *dev_mgr;
> +       struct udevice *dev;
> +
> +       ret = device_bind_driver(gd->dm_root, "efimgr",
> +                                "efimgr", &dev_mgr);
> +       if (ret)
> +               return ret;
> +
> +       ret = device_bind_driver(dev_mgr, "efi_bootdev",
> +                                "efimgr.bootdev", &dev);
> +       return ret;
> +}
> +
> +EVENT_SPY_SIMPLE(EVT_DM_POST_INIT_R, efi_bootdev_create);
> +
> +U_BOOT_DRIVER(efimgr) = {
> +       .name           = "efimgr",
> +       .id             = UCLASS_EFI_BOOT_MGR,
> +};
> +
> +UCLASS_DRIVER(efimgr) = {
> +       .id             = UCLASS_EFI_BOOT_MGR,
> +       .name           = "efimgr",
> +};
> --
> 2.48.1
>

I am not seeing any tests in this series. At least we should update
the bootflow_efi() test or add something similar.

Regards,
SImon

Re: [RFC 7/8] efi_loader: create EFI BOOTDEV

[Heinrich Schuchardt]

On 23.04.25 14:28, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> Create a bootdev device for the EFI boot manager.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>>   include/dm/uclass-id.h       |   1 +
>>   lib/efi_loader/Makefile      |   1 +
>>   lib/efi_loader/efi_bootdev.c | 102 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 104 insertions(+)
>>   create mode 100644 lib/efi_loader/efi_bootdev.c
>>
>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>> index 270088ad94f..d0795c13d14 100644
>> --- a/include/dm/uclass-id.h
>> +++ b/include/dm/uclass-id.h
>> @@ -59,6 +59,7 @@ enum uclass_id {
>>          UCLASS_ECDSA,           /* Elliptic curve cryptographic device */
>>          UCLASS_EFI_LOADER,      /* Devices created by UEFI applications */
>>          UCLASS_EFI_MEDIA,       /* Devices provided by UEFI firmware */
>> +       UCLASS_EFI_BOOT_MGR,    /* EFI boot manager */
>>          UCLASS_ETH,             /* Ethernet device */
>>          UCLASS_ETH_PHY,         /* Ethernet PHY device */
>>          UCLASS_EXTCON,          /* External Connector Class */
>> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
>> index ab50a69e48b..c37f05d70e4 100644
>> --- a/lib/efi_loader/Makefile
>> +++ b/lib/efi_loader/Makefile
>> @@ -25,6 +25,7 @@ apps-$(CONFIG_BOOTEFI_TESTAPP_COMPILE) += testapp
>>   obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
>>   obj-$(CONFIG_EFI_BOOTMGR) += efi_bootmgr.o
>>   obj-$(CONFIG_EFI_BINARY_EXEC) += efi_bootbin.o
>> +obj-$(CONFIG_BOOTMETH_EFI_BOOTMGR) += efi_bootdev.o
>>   obj-y += efi_boottime.o
>>   obj-y += efi_helper.o
>>   obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
>> diff --git a/lib/efi_loader/efi_bootdev.c b/lib/efi_loader/efi_bootdev.c
>> new file mode 100644
>> index 00000000000..066df9462c8
>> --- /dev/null
>> +++ b/lib/efi_loader/efi_bootdev.c
>> @@ -0,0 +1,102 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Bootdev for boot manager
>> + *
>> + * Copyright 2025, Heinrich Schuchardt <xypron.glpk@gmx.de>
>> + */
>> +
>> +#define LOG_CATEGORY UCLASS_BOOTSTD
>> +
>> +#include <bootdev.h>
>> +#include <bootflow.h>
>> +#include <dm.h>
>> +#include <dm/lists.h>
>> +
>> +static int efi_get_bootflow(struct udevice *dev, struct bootflow_iter *iter,
>> +                           struct bootflow *bflow)
>> +{
>> +       int ret;
>> +
>> +       ret = strcmp(iter->method->name, "efi_mgr");
>> +       if (!ret)
>> +               bflow->state = BOOTFLOWST_READY;
>> +
>> +       return ret;
>> +}
>> +
>> +struct bootdev_ops efi_bootdev_ops = {
>> +       .get_bootflow   = efi_get_bootflow,
>> +};
>> +
>> +static int efi_bootdev_bind(struct udevice *dev)
>> +{
>> +       struct bootdev_uc_plat *ucp = dev_get_uclass_plat(dev);
>> +
>> +       ucp->prio = BOOTDEVP_6_EFI;
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct udevice_id efi_bootdev_ids[] = {
>> +       { .compatible = "u-boot,bootdev-efi" },
>> +       { }
>> +};
>> +
>> +U_BOOT_DRIVER(efi_bootdev) = {
>> +       .name           = "efi_bootdev",
>> +       .id             = UCLASS_BOOTDEV,
>> +       .ops            = &efi_bootdev_ops,
>> +       .bind           = efi_bootdev_bind,
>> +       .of_match       = efi_bootdev_ids,
>> +};
>> +
>> +/**
>> + * efi_bootdev_hunt() - find devices used by EFI boot manager
>> + *
>> + * Invoke all hunters that have not been invoked by now.
>> + * This should only be the network hunter.
>> + *
>> + * @info:      info structure describing this hunter
>> + * @show:      true to show information from the hunter
>> + *
>> + * Return:     0 if device found, -EINVAL otherwise
>> + */
>> +static int efi_bootdev_hunt(struct bootdev_hunter *info, bool show)
>> +{
>> +       return bootdev_hunt(NULL, 0);
> 
> This recursive calling of hunting is very strange. Since bootmgr seems
> to need all the hunting to be done, you could move this line to
> somewhere in bootmgr.

Thank you Simon for reviewing.

I added patch 5/8 to avoid infinite recursion.

Maybe we should move this to efi_init_obj_list() because in the 
eficonfig and and efidebug command we also assume that all boot devices 
have been hunted down. If a device has not been probed you cannot create 
a boot option for it.

> 
>> +}
>> +
>> +BOOTDEV_HUNTER(efi_bootdev_hunt) = {
>> +       .prio           = BOOTDEVP_6_EFI,
>> +       .uclass         = UCLASS_EFI_BOOT_MGR,
>> +       .hunt           = efi_bootdev_hunt,
>> +       .drv            = DM_DRIVER_REF(efi_bootdev),
>> +};
>> +
>> +static int efi_bootdev_create(void)
>> +{
>> +       int ret;
>> +       struct udevice *dev_mgr;
>> +       struct udevice *dev;
>> +
>> +       ret = device_bind_driver(gd->dm_root, "efimgr",
>> +                                "efimgr", &dev_mgr);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = device_bind_driver(dev_mgr, "efi_bootdev",
>> +                                "efimgr.bootdev", &dev);
>> +       return ret;
>> +}
>> +
>> +EVENT_SPY_SIMPLE(EVT_DM_POST_INIT_R, efi_bootdev_create);
>> +
>> +U_BOOT_DRIVER(efimgr) = {
>> +       .name           = "efimgr",
>> +       .id             = UCLASS_EFI_BOOT_MGR,
>> +};
>> +
>> +UCLASS_DRIVER(efimgr) = {
>> +       .id             = UCLASS_EFI_BOOT_MGR,
>> +       .name           = "efimgr",
>> +};
>> --
>> 2.48.1
>>
> 
> I am not seeing any tests in this series. At least we should update
> the bootflow_efi() test or add something similar.

I have not looked into the tests because I first wanted to have feedback 
if the general direction is ok.

Unfortunately most bootstd tests are checking console output line by 
line instead of functional tests which makes them fail now.

The bootstd tests should better have been written in a way that would 
allow running them on real boards without making assumptions about the 
board.

Best regards

Heinrich

Re: [RFC 2/8] boot: BOOTMETH_DISTRO should select BOOTMETH_EFI_BOOTMGR

[Heinrich Schuchardt]

On 23.04.25 14:28, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> Distros expect the EFI boot manager to run. It falls back to launching
>> EFI\BOOT\BOOT<ARCH>.EFI from the ESP.
>>
>> BOOTMETH_EFILOADER is obsolete.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>>   boot/Kconfig | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/boot/Kconfig b/boot/Kconfig
>> index fb37d912bc9..71ddb37f6bb 100644
>> --- a/boot/Kconfig
>> +++ b/boot/Kconfig
>> @@ -597,6 +597,9 @@ config BOOTMETH_EFILOADER
>>          imply CMD_TFTPBOOT if CMD_NET
>>          default y
>>          help
>> +         This bootmeth is obsolete. BOOTMETH_EFI_BOOTMGR takes care of
>> +         launching EFI\BOOT\BOOT<ARCH>.EFI if not boot option matches.
>> +
> 
> I don't agree with marking this as obsolete and your commit message
> doesn't explain why it should be.

This method was written in a way that per se violates the UEFI 
specification as it does not set up required UEFI variables.

You must never run this before the boot manager as booting 
EFI/BOOT/BOOT<ARCH>.EFI if there is a boot option violates the UEFI 
specification-

The boot manager covers the functionality of this bootmeth.

We should delete this method.

Best regards

Heinrich

> 
>>            Enables support for EFI boot using bootdevs. This makes the
>>            bootdevs look for a 'boot<arch>.efi' on each filesystem
>>            they scan. The resulting file is booted after enabling U-Boot's
>> @@ -648,7 +651,7 @@ config BOOTMETH_DISTRO
>>          select BOOTMETH_SCRIPT if CMDLINE # E.g. Armbian uses scripts
>>          select BOOTMETH_EXTLINUX  # E.g. Debian uses these
>>          select BOOTMETH_EXTLINUX_PXE if CMD_PXE && CMD_NET && DM_ETH
>> -       select BOOTMETH_EFILOADER if EFI_BINARY_EXEC # E.g. Ubuntu uses this
>> +       select BOOTMETH_EFI_BOOTMGR if EFI_BINARY_EXEC # E.g. Ubuntu uses this
>>
>>   config SPL_BOOTMETH_VBE
>>          bool "Bootdev support for Verified Boot for Embedded (SPL)"
>> --
>> 2.48.1
>>
> 
> Regards,
> Simon

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Ilias Apalodimas]

Hi Heinrich,

On Mon Apr 21, 2025 at 7:25 PM EEST, Heinrich Schuchardt wrote:
> The EFI boot manager bootmeth does not require variable BootOrder to be
> preexisting. It creates this variable.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  boot/bootmeth_efi_mgr.c | 21 +++------------------
>  1 file changed, 3 insertions(+), 18 deletions(-)
>
> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> index 42b8863815e..1669cbed5bd 100644
> --- a/boot/bootmeth_efi_mgr.c
> +++ b/boot/bootmeth_efi_mgr.c
> @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter)
>
>  static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow)
>  {
> -	struct efi_mgr_priv *priv = dev_get_priv(dev);
> -	efi_status_t ret;
> -	efi_uintn_t size;
> -	u16 *bootorder;
> -
> -	if (priv->fake_dev) {
> -		bflow->state = BOOTFLOWST_READY;
> -		return 0;
> -	}

This only seems to be used by sandbox AFAICT. So I think we should remove the
struct members and functions as well with this change

> +	int ret

ret;

>
>  	ret = efi_init_obj_list();
>  	if (ret)
>  		return log_msg_ret("init", ret);
>
> -	/* Enable this method if the "BootOrder" UEFI exists. */
> -	bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid,
> -				&size);
> -	if (bootorder) {
> -		free(bootorder);
> -		bflow->state = BOOTFLOWST_READY;
> -		return 0;
> -	}
> +	bflow->state = BOOTFLOWST_READY;
>
> -	return -EINVAL;
> +	return 0;
>  }
>
>  static int efi_mgr_read_file(struct udevice *dev, struct bootflow *bflow,

Other than that it looks correct

Cheers
/Ilias

Re: [RFC 2/8] boot: BOOTMETH_DISTRO should select BOOTMETH_EFI_BOOTMGR

[Ilias Apalodimas]

Hi Heinrich

On Wed Apr 23, 2025 at 3:53 PM EEST, Heinrich Schuchardt wrote:
> On 23.04.25 14:28, Simon Glass wrote:
>> Hi Heinrich,
>>
>> On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
>> <heinrich.schuchardt@canonical.com> wrote:
>>>
>>> Distros expect the EFI boot manager to run. It falls back to launching
>>> EFI\BOOT\BOOT<ARCH>.EFI from the ESP.
>>>
>>> BOOTMETH_EFILOADER is obsolete.
>>>
>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>> ---
>>>   boot/Kconfig | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/boot/Kconfig b/boot/Kconfig
>>> index fb37d912bc9..71ddb37f6bb 100644
>>> --- a/boot/Kconfig
>>> +++ b/boot/Kconfig
>>> @@ -597,6 +597,9 @@ config BOOTMETH_EFILOADER
>>>          imply CMD_TFTPBOOT if CMD_NET
>>>          default y
>>>          help
>>> +         This bootmeth is obsolete. BOOTMETH_EFI_BOOTMGR takes care of
>>> +         launching EFI\BOOT\BOOT<ARCH>.EFI if not boot option matches.
>>> +
>>
>> I don't agree with marking this as obsolete and your commit message
>> doesn't explain why it should be.
>
> This method was written in a way that per se violates the UEFI
> specification as it does not set up required UEFI variables.
>
> You must never run this before the boot manager as booting
> EFI/BOOT/BOOT<ARCH>.EFI if there is a boot option violates the UEFI
> specification-
>
> The boot manager covers the functionality of this bootmeth.
>
> We should delete this method.

+1. Do you plan to propose a patch to remove it and only leave the bootmanager?
Or you prefer marking it as obsolete for a while?

Thanks
/Ilias

>
> Best regards
>
> Heinrich
>
>>
>>>            Enables support for EFI boot using bootdevs. This makes the
>>>            bootdevs look for a 'boot<arch>.efi' on each filesystem
>>>            they scan. The resulting file is booted after enabling U-Boot's
>>> @@ -648,7 +651,7 @@ config BOOTMETH_DISTRO
>>>          select BOOTMETH_SCRIPT if CMDLINE # E.g. Armbian uses scripts
>>>          select BOOTMETH_EXTLINUX  # E.g. Debian uses these
>>>          select BOOTMETH_EXTLINUX_PXE if CMD_PXE && CMD_NET && DM_ETH
>>> -       select BOOTMETH_EFILOADER if EFI_BINARY_EXEC # E.g. Ubuntu uses this
>>> +       select BOOTMETH_EFI_BOOTMGR if EFI_BINARY_EXEC # E.g. Ubuntu uses this
>>>
>>>   config SPL_BOOTMETH_VBE
>>>          bool "Bootdev support for Verified Boot for Embedded (SPL)"
>>> --
>>> 2.48.1
>>>
>>
>> Regards,
>> Simon

Re: [RFC 3/8] boot: make BOOTMETH_EFILOADER default=n

[Ilias Apalodimas]

On Mon Apr 21, 2025 at 7:25 PM EEST, Heinrich Schuchardt wrote:
> The boot method is deprecated as the EFI boot manager covers this
> functionality.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  boot/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/boot/Kconfig b/boot/Kconfig
> index 71ddb37f6bb..b973d9eb0a7 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -595,7 +595,6 @@ config BOOTMETH_EFILOADER
>  	bool "Bootdev support for EFI boot"
>  	depends on EFI_BINARY_EXEC
>  	imply CMD_TFTPBOOT if CMD_NET
> -	default y
>  	help
>  	  This bootmeth is obsolete. BOOTMETH_EFI_BOOTMGR takes care of
>  	  launching EFI\BOOT\BOOT<ARCH>.EFI if not boot option matches.

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

Re: [RFC 7/8] efi_loader: create EFI BOOTDEV

[Ilias Apalodimas]

Hi Heinrich,

[...]

>>> +static const struct udevice_id efi_bootdev_ids[] = {
>>> +       { .compatible = "u-boot,bootdev-efi" },
>>> +       { }
>>> +};
>>> +
>>> +U_BOOT_DRIVER(efi_bootdev) = {
>>> +       .name           = "efi_bootdev",
>>> +       .id             = UCLASS_BOOTDEV,
>>> +       .ops            = &efi_bootdev_ops,
>>> +       .bind           = efi_bootdev_bind,
>>> +       .of_match       = efi_bootdev_ids,
>>> +};
>>> +
>>> +/**
>>> + * efi_bootdev_hunt() - find devices used by EFI boot manager
>>> + *
>>> + * Invoke all hunters that have not been invoked by now.
>>> + * This should only be the network hunter.
>>> + *
>>> + * @info:      info structure describing this hunter
>>> + * @show:      true to show information from the hunter
>>> + *
>>> + * Return:     0 if device found, -EINVAL otherwise
>>> + */
>>> +static int efi_bootdev_hunt(struct bootdev_hunter *info, bool show)
>>> +{
>>> +       return bootdev_hunt(NULL, 0);
>>
>> This recursive calling of hunting is very strange. Since bootmgr seems
>> to need all the hunting to be done, you could move this line to
>> somewhere in bootmgr.
>
> Thank you Simon for reviewing.
>
> I added patch 5/8 to avoid infinite recursion.
>
> Maybe we should move this to efi_init_obj_list() because in the
> eficonfig and and efidebug command we also assume that all boot devices
> have been hunted down. If a device has not been probed you cannot create
> a boot option for it.

I don't think teaching the EFI subsystem internals of a Kconfig option is the right thing to do.
What happens if someone compiles without that? Can't we have a generic function somewhere that does
that? And then we can decide when to call it, efi_init_obj_list() of the commands themselves
explicitly.

Thanks
/Ilias
>
>>
>>> +}
>>> +
>>> +BOOTDEV_HUNTER(efi_bootdev_hunt) = {
>>> +       .prio           = BOOTDEVP_6_EFI,
>>> +       .uclass         = UCLASS_EFI_BOOT_MGR,
>>> +       .hunt           = efi_bootdev_hunt,
>>> +       .drv            = DM_DRIVER_REF(efi_bootdev),
>>> +};
>>> +
>>> +static int efi_bootdev_create(void)
>>> +{
>>> +       int ret;
>>> +       struct udevice *dev_mgr;
>>> +       struct udevice *dev;
>>> +
>>> +       ret = device_bind_driver(gd->dm_root, "efimgr",
>>> +                                "efimgr", &dev_mgr);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       ret = device_bind_driver(dev_mgr, "efi_bootdev",
>>> +                                "efimgr.bootdev", &dev);
>>> +       return ret;
>>> +}
>>> +
>>> +EVENT_SPY_SIMPLE(EVT_DM_POST_INIT_R, efi_bootdev_create);
>>> +
>>> +U_BOOT_DRIVER(efimgr) = {
>>> +       .name           = "efimgr",
>>> +       .id             = UCLASS_EFI_BOOT_MGR,
>>> +};
>>> +
>>> +UCLASS_DRIVER(efimgr) = {
>>> +       .id             = UCLASS_EFI_BOOT_MGR,
>>> +       .name           = "efimgr",
>>> +};
>>> --
>>> 2.48.1
>>>
>>
>> I am not seeing any tests in this series. At least we should update
>> the bootflow_efi() test or add something similar.
>
> I have not looked into the tests because I first wanted to have feedback
> if the general direction is ok.
>
> Unfortunately most bootstd tests are checking console output line by
> line instead of functional tests which makes them fail now.
>
> The bootstd tests should better have been written in a way that would
> allow running them on real boards without making assumptions about the
> board.
>
> Best regards
>
> Heinrich

Re: [RFC 2/8] boot: BOOTMETH_DISTRO should select BOOTMETH_EFI_BOOTMGR

[Simon Glass]

Hi,

On Fri, 2 May 2025 at 03:28, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Heinrich
>
> On Wed Apr 23, 2025 at 3:53 PM EEST, Heinrich Schuchardt wrote:
> > On 23.04.25 14:28, Simon Glass wrote:
> >> Hi Heinrich,
> >>
> >> On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> >> <heinrich.schuchardt@canonical.com> wrote:
> >>>
> >>> Distros expect the EFI boot manager to run. It falls back to launching
> >>> EFI\BOOT\BOOT<ARCH>.EFI from the ESP.
> >>>
> >>> BOOTMETH_EFILOADER is obsolete.
> >>>
> >>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>> ---
> >>>   boot/Kconfig | 5 ++++-
> >>>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/boot/Kconfig b/boot/Kconfig
> >>> index fb37d912bc9..71ddb37f6bb 100644
> >>> --- a/boot/Kconfig
> >>> +++ b/boot/Kconfig
> >>> @@ -597,6 +597,9 @@ config BOOTMETH_EFILOADER
> >>>          imply CMD_TFTPBOOT if CMD_NET
> >>>          default y
> >>>          help
> >>> +         This bootmeth is obsolete. BOOTMETH_EFI_BOOTMGR takes care of
> >>> +         launching EFI\BOOT\BOOT<ARCH>.EFI if not boot option matches.
> >>> +
> >>
> >> I don't agree with marking this as obsolete and your commit message
> >> doesn't explain why it should be.
> >
> > This method was written in a way that per se violates the UEFI
> > specification as it does not set up required UEFI variables.
> >
> > You must never run this before the boot manager as booting
> > EFI/BOOT/BOOT<ARCH>.EFI if there is a boot option violates the UEFI
> > specification-
> >
> > The boot manager covers the functionality of this bootmeth.
> >
> > We should delete this method.
>
> +1. Do you plan to propose a patch to remove it and only leave the bootmanager?
> Or you prefer marking it as obsolete for a while?

No, neither.

The work-around that Heinrich and I discussed was to insert bootmgr
later in the bootdev sequence, not to disable this bootmeth.

Regards,
Simon

Re: [RFC 2/8] boot: BOOTMETH_DISTRO should select BOOTMETH_EFI_BOOTMGR

[Ilias Apalodimas]

On Fri, 2 May 2025 at 16:23, Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On Fri, 2 May 2025 at 03:28, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Heinrich
> >
> > On Wed Apr 23, 2025 at 3:53 PM EEST, Heinrich Schuchardt wrote:
> > > On 23.04.25 14:28, Simon Glass wrote:
> > >> Hi Heinrich,
> > >>
> > >> On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > >> <heinrich.schuchardt@canonical.com> wrote:
> > >>>
> > >>> Distros expect the EFI boot manager to run. It falls back to launching
> > >>> EFI\BOOT\BOOT<ARCH>.EFI from the ESP.
> > >>>
> > >>> BOOTMETH_EFILOADER is obsolete.
> > >>>
> > >>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > >>> ---
> > >>>   boot/Kconfig | 5 ++++-
> > >>>   1 file changed, 4 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/boot/Kconfig b/boot/Kconfig
> > >>> index fb37d912bc9..71ddb37f6bb 100644
> > >>> --- a/boot/Kconfig
> > >>> +++ b/boot/Kconfig
> > >>> @@ -597,6 +597,9 @@ config BOOTMETH_EFILOADER
> > >>>          imply CMD_TFTPBOOT if CMD_NET
> > >>>          default y
> > >>>          help
> > >>> +         This bootmeth is obsolete. BOOTMETH_EFI_BOOTMGR takes care of
> > >>> +         launching EFI\BOOT\BOOT<ARCH>.EFI if not boot option matches.
> > >>> +
> > >>
> > >> I don't agree with marking this as obsolete and your commit message
> > >> doesn't explain why it should be.
> > >
> > > This method was written in a way that per se violates the UEFI
> > > specification as it does not set up required UEFI variables.
> > >
> > > You must never run this before the boot manager as booting
> > > EFI/BOOT/BOOT<ARCH>.EFI if there is a boot option violates the UEFI
> > > specification-
> > >
> > > The boot manager covers the functionality of this bootmeth.
> > >
> > > We should delete this method.
> >
> > +1. Do you plan to propose a patch to remove it and only leave the bootmanager?
> > Or you prefer marking it as obsolete for a while?
>
> No, neither.
>
> The work-around that Heinrich and I discussed was to insert bootmgr
> later in the bootdev sequence, not to disable this bootmeth.

Can you explain why you want to keep this bootmethod? Both Heinrich
and I explained why the bootmanager is the appropriate one for EFI.
Do you have any use cases in mind that isn't covered by the manager
and you need this?

Thanks
/Ilias
>
> Regards,
> Simon

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Simon Glass]

Hi Heinrich,

On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> The EFI boot manager bootmeth does not require variable BootOrder to be
> preexisting. It creates this variable.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  boot/bootmeth_efi_mgr.c | 21 +++------------------
>  1 file changed, 3 insertions(+), 18 deletions(-)
>
> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> index 42b8863815e..1669cbed5bd 100644
> --- a/boot/bootmeth_efi_mgr.c
> +++ b/boot/bootmeth_efi_mgr.c
> @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter)
>
>  static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow)
>  {
> -       struct efi_mgr_priv *priv = dev_get_priv(dev);
> -       efi_status_t ret;
> -       efi_uintn_t size;
> -       u16 *bootorder;
> -
> -       if (priv->fake_dev) {
> -               bflow->state = BOOTFLOWST_READY;
> -               return 0;
> -       }
> +       int ret
>
>         ret = efi_init_obj_list();
>         if (ret)
>                 return log_msg_ret("init", ret);
>
> -       /* Enable this method if the "BootOrder" UEFI exists. */
> -       bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid,
> -                               &size);
> -       if (bootorder) {
> -               free(bootorder);
> -               bflow->state = BOOTFLOWST_READY;
> -               return 0;
> -       }
> +       bflow->state = BOOTFLOWST_READY;
>
> -       return -EINVAL;
> +       return 0;
>  }
>
>  static int efi_mgr_read_file(struct udevice *dev, struct bootflow *bflow,
> --
> 2.48.1
>

How do we know if the board is using EFI bootmgr? My understanding was
that this was a way to find out?

Regards,
Simon

Re: [RFC 3/8] boot: make BOOTMETH_EFILOADER default=n

[Simon Glass]

Hi Heinrich,

On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> The boot method is deprecated as the EFI boot manager covers this
> functionality.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  boot/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/boot/Kconfig b/boot/Kconfig
> index 71ddb37f6bb..b973d9eb0a7 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -595,7 +595,6 @@ config BOOTMETH_EFILOADER
>         bool "Bootdev support for EFI boot"
>         depends on EFI_BINARY_EXEC
>         imply CMD_TFTPBOOT if CMD_NET
> -       default y
>         help
>           This bootmeth is obsolete. BOOTMETH_EFI_BOOTMGR takes care of
>           launching EFI\BOOT\BOOT<ARCH>.EFI if not boot option matches.
> --
> 2.48.1
>

As mentioned on the other patch, we need to keep this enabled and it
isn't obsolete.

Regards,
Simon

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Heinrich Schuchardt]

On 5/2/25 16:49, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> The EFI boot manager bootmeth does not require variable BootOrder to be
>> preexisting. It creates this variable.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>>   boot/bootmeth_efi_mgr.c | 21 +++------------------
>>   1 file changed, 3 insertions(+), 18 deletions(-)
>>
>> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
>> index 42b8863815e..1669cbed5bd 100644
>> --- a/boot/bootmeth_efi_mgr.c
>> +++ b/boot/bootmeth_efi_mgr.c
>> @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter)
>>
>>   static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow)
>>   {
>> -       struct efi_mgr_priv *priv = dev_get_priv(dev);
>> -       efi_status_t ret;
>> -       efi_uintn_t size;
>> -       u16 *bootorder;
>> -
>> -       if (priv->fake_dev) {
>> -               bflow->state = BOOTFLOWST_READY;
>> -               return 0;
>> -       }
>> +       int ret
>>
>>          ret = efi_init_obj_list();
>>          if (ret)
>>                  return log_msg_ret("init", ret);
>>
>> -       /* Enable this method if the "BootOrder" UEFI exists. */
>> -       bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid,
>> -                               &size);
>> -       if (bootorder) {
>> -               free(bootorder);
>> -               bflow->state = BOOTFLOWST_READY;
>> -               return 0;
>> -       }
>> +       bflow->state = BOOTFLOWST_READY;
>>
>> -       return -EINVAL;
>> +       return 0;
>>   }
>>
>>   static int efi_mgr_read_file(struct udevice *dev, struct bootflow *bflow,
>> --
>> 2.48.1
>>
> 
> How do we know if the board is using EFI bootmgr? My understanding was
> that this was a way to find out?

The boot manager must always run.

The check for the BootOrder variable introduced in commit f2bfa0cb1794 
is a bug.

The boot manager handles in sequence:

* Try to boot as indicated by BootNext.
* Try to boot as indicated by BootOrder.
* Try to boot default path for available media.
   This will add Boot#### entries and update BootOrder.

Best regards

Heinrich

Re: [RFC 3/8] boot: make BOOTMETH_EFILOADER default=n

[Heinrich Schuchardt]

On 5/2/25 16:49, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> The boot method is deprecated as the EFI boot manager covers this
>> functionality.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>>   boot/Kconfig | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/boot/Kconfig b/boot/Kconfig
>> index 71ddb37f6bb..b973d9eb0a7 100644
>> --- a/boot/Kconfig
>> +++ b/boot/Kconfig
>> @@ -595,7 +595,6 @@ config BOOTMETH_EFILOADER
>>          bool "Bootdev support for EFI boot"
>>          depends on EFI_BINARY_EXEC
>>          imply CMD_TFTPBOOT if CMD_NET
>> -       default y
>>          help
>>            This bootmeth is obsolete. BOOTMETH_EFI_BOOTMGR takes care of
>>            launching EFI\BOOT\BOOT<ARCH>.EFI if not boot option matches.
>> --
>> 2.48.1
>>
> 
> As mentioned on the other patch, we need to keep this enabled and it
> isn't obsolete.
> 
> Regards,
> Simon

Please, read the UEFI specification especially chapter 3 "Boot Manager".

* The functionality is fully covered by the boot manager.
* The current implementation of the method does not comply with the UEFI 
specification. Especially if run before the boot manager.

Best regards

Heinrich

Re: [RFC 3/8] boot: make BOOTMETH_EFILOADER default=n

[Simon Glass]

Hi Heinrich,

On Fri, 2 May 2025 at 10:16, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 5/2/25 16:49, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> The boot method is deprecated as the EFI boot manager covers this
> >> functionality.
> >>
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >> ---
> >>   boot/Kconfig | 1 -
> >>   1 file changed, 1 deletion(-)
> >>
> >> diff --git a/boot/Kconfig b/boot/Kconfig
> >> index 71ddb37f6bb..b973d9eb0a7 100644
> >> --- a/boot/Kconfig
> >> +++ b/boot/Kconfig
> >> @@ -595,7 +595,6 @@ config BOOTMETH_EFILOADER
> >>          bool "Bootdev support for EFI boot"
> >>          depends on EFI_BINARY_EXEC
> >>          imply CMD_TFTPBOOT if CMD_NET
> >> -       default y
> >>          help
> >>            This bootmeth is obsolete. BOOTMETH_EFI_BOOTMGR takes care of
> >>            launching EFI\BOOT\BOOT<ARCH>.EFI if not boot option matches.
> >> --
> >> 2.48.1
> >>
> >
> > As mentioned on the other patch, we need to keep this enabled and it
> > isn't obsolete.
> >
> > Regards,
> > Simon
>
> Please, read the UEFI specification especially chapter 3 "Boot Manager".
>
> * The functionality is fully covered by the boot manager.
> * The current implementation of the method does not comply with the UEFI
> specification. Especially if run before the boot manager.

We're going to have to go through this together sometime as it seems
like chicken & egg to me. We don't want Boot Manager first because it
is slow and stuffs up FEL on sunxi etc., we don't want it last because
then it has to wait for networking, we don't know whether bootmgr is
being used or not, so we just have to put it in the middle, but then
we don't know whether it will succeed, at which point we have to go
back and try the whole boot again..?

Regards,
Simon

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Simon Glass]

Hi Heinrich,

On Fri, 2 May 2025 at 10:09, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 5/2/25 16:49, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> The EFI boot manager bootmeth does not require variable BootOrder to be
> >> preexisting. It creates this variable.
> >>
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >> ---
> >>   boot/bootmeth_efi_mgr.c | 21 +++------------------
> >>   1 file changed, 3 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> >> index 42b8863815e..1669cbed5bd 100644
> >> --- a/boot/bootmeth_efi_mgr.c
> >> +++ b/boot/bootmeth_efi_mgr.c
> >> @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter)
> >>
> >>   static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow)
> >>   {
> >> -       struct efi_mgr_priv *priv = dev_get_priv(dev);
> >> -       efi_status_t ret;
> >> -       efi_uintn_t size;
> >> -       u16 *bootorder;
> >> -
> >> -       if (priv->fake_dev) {
> >> -               bflow->state = BOOTFLOWST_READY;
> >> -               return 0;
> >> -       }
> >> +       int ret
> >>
> >>          ret = efi_init_obj_list();
> >>          if (ret)
> >>                  return log_msg_ret("init", ret);
> >>
> >> -       /* Enable this method if the "BootOrder" UEFI exists. */
> >> -       bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid,
> >> -                               &size);
> >> -       if (bootorder) {
> >> -               free(bootorder);
> >> -               bflow->state = BOOTFLOWST_READY;
> >> -               return 0;
> >> -       }
> >> +       bflow->state = BOOTFLOWST_READY;
> >>
> >> -       return -EINVAL;
> >> +       return 0;
> >>   }
> >>
> >>   static int efi_mgr_read_file(struct udevice *dev, struct bootflow *bflow,
> >> --
> >> 2.48.1
> >>
> >
> > How do we know if the board is using EFI bootmgr? My understanding was
> > that this was a way to find out?
>
> The boot manager must always run.

Are you sure that is true? It seems false to me, e.g.

- if we are booting FEL on suxi, it must not run.
- none of the boards in my lab use it
- the Ubuntu 24.04 test in my lab doesn't use it

So how can we say that it must always run?

>
> The check for the BootOrder variable introduced in commit f2bfa0cb1794
> is a bug.
>
> The boot manager handles in sequence:
>
> * Try to boot as indicated by BootNext.
> * Try to boot as indicated by BootOrder.
> * Try to boot default path for available media.
>    This will add Boot#### entries and update BootOrder.

Regards,
Simon

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Heinrich Schuchardt]

Simon Glass <sjg@chromium.org> schrieb am Sa., 3. Mai 2025, 04:10:

> Hi Heinrich,
>
> On Fri, 2 May 2025 at 10:09, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
> >
> > On 5/2/25 16:49, Simon Glass wrote:
> > > Hi Heinrich,
> > >
> > > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > > <heinrich.schuchardt@canonical.com> wrote:
> > >>
> > >> The EFI boot manager bootmeth does not require variable BootOrder to
> be
> > >> preexisting. It creates this variable.
> > >>
> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com
> >
> > >> ---
> > >>   boot/bootmeth_efi_mgr.c | 21 +++------------------
> > >>   1 file changed, 3 insertions(+), 18 deletions(-)
> > >>
> > >> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> > >> index 42b8863815e..1669cbed5bd 100644
> > >> --- a/boot/bootmeth_efi_mgr.c
> > >> +++ b/boot/bootmeth_efi_mgr.c
> > >> @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev,
> struct bootflow_iter *iter)
> > >>
> > >>   static int efi_mgr_read_bootflow(struct udevice *dev, struct
> bootflow *bflow)
> > >>   {
> > >> -       struct efi_mgr_priv *priv = dev_get_priv(dev);
> > >> -       efi_status_t ret;
> > >> -       efi_uintn_t size;
> > >> -       u16 *bootorder;
> > >> -
> > >> -       if (priv->fake_dev) {
> > >> -               bflow->state = BOOTFLOWST_READY;
> > >> -               return 0;
> > >> -       }
> > >> +       int ret
> > >>
> > >>          ret = efi_init_obj_list();
> > >>          if (ret)
> > >>                  return log_msg_ret("init", ret);
> > >>
> > >> -       /* Enable this method if the "BootOrder" UEFI exists. */
> > >> -       bootorder = efi_get_var(u"BootOrder",
> &efi_global_variable_guid,
> > >> -                               &size);
> > >> -       if (bootorder) {
> > >> -               free(bootorder);
> > >> -               bflow->state = BOOTFLOWST_READY;
> > >> -               return 0;
> > >> -       }
> > >> +       bflow->state = BOOTFLOWST_READY;
> > >>
> > >> -       return -EINVAL;
> > >> +       return 0;
> > >>   }
> > >>
> > >>   static int efi_mgr_read_file(struct udevice *dev, struct bootflow
> *bflow,
> > >> --
> > >> 2.48.1
> > >>
> > >
> > > How do we know if the board is using EFI bootmgr? My understanding was
> > > that this was a way to find out?
> >
> > The boot manager must always run.
>
> Are you sure that is true? It seems false to me, e.g.
>
> - if we are booting FEL on suxi, it must not run.
> - none of the boards in my lab use it
> - the Ubuntu 24.04 test in my lab doesn't use it
>
> So how can we say that it must always run?
>

If a boot method that is earlier in the sequence boots the boot manager
will not be reached. But the existence of BootOrder or any other EFI
variable shall not decide if the boot manager runs or not.


> >
> > The check for the BootOrder variable introduced in commit f2bfa0cb1794
> > is a bug.
> >
> > The boot manager handles in sequence:
> >
> > * Try to boot as indicated by BootNext.
> > * Try to boot as indicated by BootOrder.
> > * Try to boot default path for available media.
> >    This will add Boot#### entries and update BootOrder.
>
> Regards,
> Simon
>

Re: [RFC 3/8] boot: make BOOTMETH_EFILOADER default=n

[Heinrich Schuchardt]

Simon Glass <sjg@chromium.org> schrieb am Sa., 3. Mai 2025, 04:10:

> Hi Heinrich,
>
> On Fri, 2 May 2025 at 10:16, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
> >
> > On 5/2/25 16:49, Simon Glass wrote:
> > > Hi Heinrich,
> > >
> > > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > > <heinrich.schuchardt@canonical.com> wrote:
> > >>
> > >> The boot method is deprecated as the EFI boot manager covers this
> > >> functionality.
> > >>
> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com
> >
> > >> ---
> > >>   boot/Kconfig | 1 -
> > >>   1 file changed, 1 deletion(-)
> > >>
> > >> diff --git a/boot/Kconfig b/boot/Kconfig
> > >> index 71ddb37f6bb..b973d9eb0a7 100644
> > >> --- a/boot/Kconfig
> > >> +++ b/boot/Kconfig
> > >> @@ -595,7 +595,6 @@ config BOOTMETH_EFILOADER
> > >>          bool "Bootdev support for EFI boot"
> > >>          depends on EFI_BINARY_EXEC
> > >>          imply CMD_TFTPBOOT if CMD_NET
> > >> -       default y
> > >>          help
> > >>            This bootmeth is obsolete. BOOTMETH_EFI_BOOTMGR takes care
> of
> > >>            launching EFI\BOOT\BOOT<ARCH>.EFI if not boot option
> matches.
> > >> --
> > >> 2.48.1
> > >>
> > >
> > > As mentioned on the other patch, we need to keep this enabled and it
> > > isn't obsolete.
> > >
> > > Regards,
> > > Simon
> >
> > Please, read the UEFI specification especially chapter 3 "Boot Manager".
> >
> > * The functionality is fully covered by the boot manager.
> > * The current implementation of the method does not comply with the UEFI
> > specification. Especially if run before the boot manager.
>
> We're going to have to go through this together sometime as it seems
> like chicken & egg to me. We don't want Boot Manager first because it
> is slow and stuffs up FEL on sunxi etc., we don't want it last because
> then it has to wait for networking, we don't know whether bootmgr is
> being used or not, so we just have to put it in the middle, but then
> we don't know whether it will succeed, at which point we have to go
> back and try the whole boot again..?
>

Sure there can be a situation where none of the boot methods will succeed.
E.g. if there are no boot media. This has been the case before the series
and won't be different with the series.

But if we reach the boot manager after trying FEL, extlinux, script and
none of these booted retrying these would not make any sense as they would
fail again.

The boot manager already comprises booting via EFI/BOOT/BOOT<ARCH>.EFI. And
this must not be tried before booting according to BootNext and BootOrder.
So we can safely delete that non-compliant bootmeth_efi.

Best regards

Heinrich





> Regards,
> Simon
>

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Simon Glass]

HI Heinrich,

On Sat, 3 May 2025 at 01:04, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> Simon Glass <sjg@chromium.org> schrieb am Sa., 3. Mai 2025, 04:10:
>>
>> Hi Heinrich,
>>
>> On Fri, 2 May 2025 at 10:09, Heinrich Schuchardt
>> <heinrich.schuchardt@canonical.com> wrote:
>> >
>> > On 5/2/25 16:49, Simon Glass wrote:
>> > > Hi Heinrich,
>> > >
>> > > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
>> > > <heinrich.schuchardt@canonical.com> wrote:
>> > >>
>> > >> The EFI boot manager bootmeth does not require variable BootOrder to be
>> > >> preexisting. It creates this variable.
>> > >>
>> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> > >> ---
>> > >>   boot/bootmeth_efi_mgr.c | 21 +++------------------
>> > >>   1 file changed, 3 insertions(+), 18 deletions(-)
>> > >>
>> > >> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
>> > >> index 42b8863815e..1669cbed5bd 100644
>> > >> --- a/boot/bootmeth_efi_mgr.c
>> > >> +++ b/boot/bootmeth_efi_mgr.c
>> > >> @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter)
>> > >>
>> > >>   static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow)
>> > >>   {
>> > >> -       struct efi_mgr_priv *priv = dev_get_priv(dev);
>> > >> -       efi_status_t ret;
>> > >> -       efi_uintn_t size;
>> > >> -       u16 *bootorder;
>> > >> -
>> > >> -       if (priv->fake_dev) {
>> > >> -               bflow->state = BOOTFLOWST_READY;
>> > >> -               return 0;
>> > >> -       }
>> > >> +       int ret
>> > >>
>> > >>          ret = efi_init_obj_list();
>> > >>          if (ret)
>> > >>                  return log_msg_ret("init", ret);
>> > >>
>> > >> -       /* Enable this method if the "BootOrder" UEFI exists. */
>> > >> -       bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid,
>> > >> -                               &size);
>> > >> -       if (bootorder) {
>> > >> -               free(bootorder);
>> > >> -               bflow->state = BOOTFLOWST_READY;
>> > >> -               return 0;
>> > >> -       }
>> > >> +       bflow->state = BOOTFLOWST_READY;
>> > >>
>> > >> -       return -EINVAL;
>> > >> +       return 0;
>> > >>   }
>> > >>
>> > >>   static int efi_mgr_read_file(struct udevice *dev, struct bootflow *bflow,
>> > >> --
>> > >> 2.48.1
>> > >>
>> > >
>> > > How do we know if the board is using EFI bootmgr? My understanding was
>> > > that this was a way to find out?
>> >
>> > The boot manager must always run.
>>
>> Are you sure that is true? It seems false to me, e.g.
>>
>> - if we are booting FEL on suxi, it must not run.
>> - none of the boards in my lab use it
>> - the Ubuntu 24.04 test in my lab doesn't use it
>>
>> So how can we say that it must always run?
>
>
> If a boot method that is earlier in the sequence boots the boot manager will not be reached. But the existence of BootOrder or any other EFI variable shall not decide if the boot manager runs or not.

So is there *any* way to know whether it should run or not? Should we
use the Kconfig to control it? It feels like disappearing into the
abyss when we run bootmgr, at present. I'm wondering if we can improve
this.

Regards,
Simon

Re: [RFC 3/8] boot: make BOOTMETH_EFILOADER default=n

[Simon Glass]

Hi Heinrich,

On Sat, 3 May 2025 at 01:16, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> Simon Glass <sjg@chromium.org> schrieb am Sa., 3. Mai 2025, 04:10:
>>
>> Hi Heinrich,
>>
>> On Fri, 2 May 2025 at 10:16, Heinrich Schuchardt
>> <heinrich.schuchardt@canonical.com> wrote:
>> >
>> > On 5/2/25 16:49, Simon Glass wrote:
>> > > Hi Heinrich,
>> > >
>> > > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
>> > > <heinrich.schuchardt@canonical.com> wrote:
>> > >>
>> > >> The boot method is deprecated as the EFI boot manager covers this
>> > >> functionality.
>> > >>
>> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> > >> ---
>> > >>   boot/Kconfig | 1 -
>> > >>   1 file changed, 1 deletion(-)
>> > >>
>> > >> diff --git a/boot/Kconfig b/boot/Kconfig
>> > >> index 71ddb37f6bb..b973d9eb0a7 100644
>> > >> --- a/boot/Kconfig
>> > >> +++ b/boot/Kconfig
>> > >> @@ -595,7 +595,6 @@ config BOOTMETH_EFILOADER
>> > >>          bool "Bootdev support for EFI boot"
>> > >>          depends on EFI_BINARY_EXEC
>> > >>          imply CMD_TFTPBOOT if CMD_NET
>> > >> -       default y
>> > >>          help
>> > >>            This bootmeth is obsolete. BOOTMETH_EFI_BOOTMGR takes care of
>> > >>            launching EFI\BOOT\BOOT<ARCH>.EFI if not boot option matches.
>> > >> --
>> > >> 2.48.1
>> > >>
>> > >
>> > > As mentioned on the other patch, we need to keep this enabled and it
>> > > isn't obsolete.
>> > >
>> > > Regards,
>> > > Simon
>> >
>> > Please, read the UEFI specification especially chapter 3 "Boot Manager".
>> >
>> > * The functionality is fully covered by the boot manager.
>> > * The current implementation of the method does not comply with the UEFI
>> > specification. Especially if run before the boot manager.
>>
>> We're going to have to go through this together sometime as it seems
>> like chicken & egg to me. We don't want Boot Manager first because it
>> is slow and stuffs up FEL on sunxi etc., we don't want it last because
>> then it has to wait for networking, we don't know whether bootmgr is
>> being used or not, so we just have to put it in the middle, but then
>> we don't know whether it will succeed, at which point we have to go
>> back and try the whole boot again..?
>
>
> Sure there can be a situation where none of the boot methods will succeed. E.g. if there are no boot media. This has been the case before the series and won't be different with the series.
>
> But if we reach the boot manager after trying FEL, extlinux, script and none of these booted retrying these would not make any sense as they would fail again.
>
> The boot manager already comprises booting via EFI/BOOT/BOOT<ARCH>.EFI. And this must not be tried before booting according to BootNext and BootOrder. So we can safely delete that non-compliant bootmeth_efi.

OK let's discuss this and see what we can figure out. Perhaps we could
come up with an env var which controls this, or similar.

Regards,
SImon

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Heinrich Schuchardt]

Simon Glass <sjg@chromium.org> schrieb am Sa., 3. Mai 2025, 23:26:

> HI Heinrich,
>
> On Sat, 3 May 2025 at 01:04, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
> >
> >
> >
> > Simon Glass <sjg@chromium.org> schrieb am Sa., 3. Mai 2025, 04:10:
> >>
> >> Hi Heinrich,
> >>
> >> On Fri, 2 May 2025 at 10:09, Heinrich Schuchardt
> >> <heinrich.schuchardt@canonical.com> wrote:
> >> >
> >> > On 5/2/25 16:49, Simon Glass wrote:
> >> > > Hi Heinrich,
> >> > >
> >> > > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> >> > > <heinrich.schuchardt@canonical.com> wrote:
> >> > >>
> >> > >> The EFI boot manager bootmeth does not require variable BootOrder
> to be
> >> > >> preexisting. It creates this variable.
> >> > >>
> >> > >> Signed-off-by: Heinrich Schuchardt <
> heinrich.schuchardt@canonical.com>
> >> > >> ---
> >> > >>   boot/bootmeth_efi_mgr.c | 21 +++------------------
> >> > >>   1 file changed, 3 insertions(+), 18 deletions(-)
> >> > >>
> >> > >> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> >> > >> index 42b8863815e..1669cbed5bd 100644
> >> > >> --- a/boot/bootmeth_efi_mgr.c
> >> > >> +++ b/boot/bootmeth_efi_mgr.c
> >> > >> @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev,
> struct bootflow_iter *iter)
> >> > >>
> >> > >>   static int efi_mgr_read_bootflow(struct udevice *dev, struct
> bootflow *bflow)
> >> > >>   {
> >> > >> -       struct efi_mgr_priv *priv = dev_get_priv(dev);
> >> > >> -       efi_status_t ret;
> >> > >> -       efi_uintn_t size;
> >> > >> -       u16 *bootorder;
> >> > >> -
> >> > >> -       if (priv->fake_dev) {
> >> > >> -               bflow->state = BOOTFLOWST_READY;
> >> > >> -               return 0;
> >> > >> -       }
> >> > >> +       int ret
> >> > >>
> >> > >>          ret = efi_init_obj_list();
> >> > >>          if (ret)
> >> > >>                  return log_msg_ret("init", ret);
> >> > >>
> >> > >> -       /* Enable this method if the "BootOrder" UEFI exists. */
> >> > >> -       bootorder = efi_get_var(u"BootOrder",
> &efi_global_variable_guid,
> >> > >> -                               &size);
> >> > >> -       if (bootorder) {
> >> > >> -               free(bootorder);
> >> > >> -               bflow->state = BOOTFLOWST_READY;
> >> > >> -               return 0;
> >> > >> -       }
> >> > >> +       bflow->state = BOOTFLOWST_READY;
> >> > >>
> >> > >> -       return -EINVAL;
> >> > >> +       return 0;
> >> > >>   }
> >> > >>
> >> > >>   static int efi_mgr_read_file(struct udevice *dev, struct
> bootflow *bflow,
> >> > >> --
> >> > >> 2.48.1
> >> > >>
> >> > >
> >> > > How do we know if the board is using EFI bootmgr? My understanding
> was
> >> > > that this was a way to find out?
> >> >
> >> > The boot manager must always run.
> >>
> >> Are you sure that is true? It seems false to me, e.g.
> >>
> >> - if we are booting FEL on suxi, it must not run.
> >> - none of the boards in my lab use it
> >> - the Ubuntu 24.04 test in my lab doesn't use it
> >>
> >> So how can we say that it must always run?
> >
> >
> > If a boot method that is earlier in the sequence boots the boot manager
> will not be reached. But the existence of BootOrder or any other EFI
> variable shall not decide if the boot manager runs or not.
>
> So is there *any* way to know whether it should run or not? Should we
> use the Kconfig to control it?


We already have a Kconfig variable for the bootmeth. If that is chosen and
FEL, script, extlinux fail the boot manager should run.

It feels like disappearing into the
> abyss when we run bootmgr, at present.


All boot methods may fail to find anything to boot. The EFI boot manager is
not special in that regard.

Best regards

Heinrich

I'm wondering if we can improve
> this.
>
> Regards,
> Simon
>

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Mark Kettenis]

> Date: Fri, 2 May 2025 18:08:56 +0200
> From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> 
> On 5/2/25 16:49, Simon Glass wrote:
> > Hi Heinrich,
> > 
> > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> The EFI boot manager bootmeth does not require variable BootOrder to be
> >> preexisting. It creates this variable.
> >>
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >> ---
> >>   boot/bootmeth_efi_mgr.c | 21 +++------------------
> >>   1 file changed, 3 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> >> index 42b8863815e..1669cbed5bd 100644
> >> --- a/boot/bootmeth_efi_mgr.c
> >> +++ b/boot/bootmeth_efi_mgr.c
> >> @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter)
> >>
> >>   static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow)
> >>   {
> >> -       struct efi_mgr_priv *priv = dev_get_priv(dev);
> >> -       efi_status_t ret;
> >> -       efi_uintn_t size;
> >> -       u16 *bootorder;
> >> -
> >> -       if (priv->fake_dev) {
> >> -               bflow->state = BOOTFLOWST_READY;
> >> -               return 0;
> >> -       }
> >> +       int ret
> >>
> >>          ret = efi_init_obj_list();
> >>          if (ret)
> >>                  return log_msg_ret("init", ret);
> >>
> >> -       /* Enable this method if the "BootOrder" UEFI exists. */
> >> -       bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid,
> >> -                               &size);
> >> -       if (bootorder) {
> >> -               free(bootorder);
> >> -               bflow->state = BOOTFLOWST_READY;
> >> -               return 0;
> >> -       }
> >> +       bflow->state = BOOTFLOWST_READY;
> >>
> >> -       return -EINVAL;
> >> +       return 0;
> >>   }
> >>
> >>   static int efi_mgr_read_file(struct udevice *dev, struct bootflow *bflow,
> >> --
> >> 2.48.1
> >>
> > 
> > How do we know if the board is using EFI bootmgr? My understanding was
> > that this was a way to find out?
> 
> The boot manager must always run.
> 
> The check for the BootOrder variable introduced in commit f2bfa0cb1794 
> is a bug.

Well, at the time the boot manager did not attempt to boot the default
path.  So there was no point in running the boot manager code unless
BootOrder (or BootNext) was set.  And of course before that commit the
boot manager didn't run at all on non-sandbox builds that had the
standard boot stuff enebaled.

Anyway, I believe the thinking behind that commit is still sound.  As
I explained earlier, I think that...

> The boot manager handles in sequence:
> 
> * Try to boot as indicated by BootNext.
> * Try to boot as indicated by BootOrder.
> * Try to boot default path for available media.
>    This will add Boot#### entries and update BootOrder.

...doing this all in a monolithic sequence isn't the best way to
handle EFI boot in the u-boot ecosystem.

Your series moves the boot manager further down the list because the
third step in the sequence has to happen late.  But as a result
BootNext and BootOrder processing will also happen late.  So what
happens if you have a board with two OS installations:

1. A generic Linux distro that boots via EFI.

2. Something like Armbian that provides an extlinux.conf file.

Currently such a system will probably boot OS #1.  But after your
changes it will probably boot OS #2.  And if OS #1 sets BootOrder or
BootNext that will not change anything.

So I think we need a solution where BootNext and BootOrder processing
happens early, like we do now.


> 
> Best regards
> 
> Heinrich
> 

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Heinrich Schuchardt]

Mark Kettenis <mark.kettenis@xs4all.nl> schrieb am Mo., 5. Mai 2025, 21:03:

> > Date: Fri, 2 May 2025 18:08:56 +0200
> > From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >
> > On 5/2/25 16:49, Simon Glass wrote:
> > > Hi Heinrich,
> > >
> > > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > > <heinrich.schuchardt@canonical.com> wrote:
> > >>
> > >> The EFI boot manager bootmeth does not require variable BootOrder to
> be
> > >> preexisting. It creates this variable.
> > >>
> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com
> >
> > >> ---
> > >>   boot/bootmeth_efi_mgr.c | 21 +++------------------
> > >>   1 file changed, 3 insertions(+), 18 deletions(-)
> > >>
> > >> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> > >> index 42b8863815e..1669cbed5bd 100644
> > >> --- a/boot/bootmeth_efi_mgr.c
> > >> +++ b/boot/bootmeth_efi_mgr.c
> > >> @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev,
> struct bootflow_iter *iter)
> > >>
> > >>   static int efi_mgr_read_bootflow(struct udevice *dev, struct
> bootflow *bflow)
> > >>   {
> > >> -       struct efi_mgr_priv *priv = dev_get_priv(dev);
> > >> -       efi_status_t ret;
> > >> -       efi_uintn_t size;
> > >> -       u16 *bootorder;
> > >> -
> > >> -       if (priv->fake_dev) {
> > >> -               bflow->state = BOOTFLOWST_READY;
> > >> -               return 0;
> > >> -       }
> > >> +       int ret
> > >>
> > >>          ret = efi_init_obj_list();
> > >>          if (ret)
> > >>                  return log_msg_ret("init", ret);
> > >>
> > >> -       /* Enable this method if the "BootOrder" UEFI exists. */
> > >> -       bootorder = efi_get_var(u"BootOrder",
> &efi_global_variable_guid,
> > >> -                               &size);
> > >> -       if (bootorder) {
> > >> -               free(bootorder);
> > >> -               bflow->state = BOOTFLOWST_READY;
> > >> -               return 0;
> > >> -       }
> > >> +       bflow->state = BOOTFLOWST_READY;
> > >>
> > >> -       return -EINVAL;
> > >> +       return 0;
> > >>   }
> > >>
> > >>   static int efi_mgr_read_file(struct udevice *dev, struct bootflow
> *bflow,
> > >> --
> > >> 2.48.1
> > >>
> > >
> > > How do we know if the board is using EFI bootmgr? My understanding was
> > > that this was a way to find out?
> >
> > The boot manager must always run.
> >
> > The check for the BootOrder variable introduced in commit f2bfa0cb1794
> > is a bug.
>
> Well, at the time the boot manager did not attempt to boot the default
> path.  So there was no point in running the boot manager code unless
> BootOrder (or BootNext) was set.  And of course before that commit the
> boot manager didn't run at all on non-sandbox builds that had the
> standard boot stuff enebaled.
>
> Anyway, I believe the thinking behind that commit is still sound.  As
> I explained earlier, I think that...
>
> > The boot manager handles in sequence:
> >
> > * Try to boot as indicated by BootNext.
> > * Try to boot as indicated by BootOrder.
> > * Try to boot default path for available media.
> >    This will add Boot#### entries and update BootOrder.
>
> ...doing this all in a monolithic sequence isn't the best way to
> handle EFI boot in the u-boot ecosystem.
>
> Your series moves the boot manager further down the list because the
> third step in the sequence has to happen late.  But as a result
> BootNext and BootOrder processing will also happen late.  So what
> happens if you have a board with two OS installations:
>
> 1. A generic Linux distro that boots via EFI.
>
> 2. Something like Armbian that provides an extlinux.conf file.
>
> Currently such a system will probably boot OS #1.  But after your
>

This did not happen with distoboot. So migration from distroboot to
standard boot results in a change that you want to avoid.

changes it will probably boot OS #2.  And if OS #1 sets BootOrder or
> BootNext that will not change anything.
>
> So I think we need a solution where BootNext and BootOrder processing
> happens early, like we do now.
>

As of today BootNext does not invoke the boot manager.

If BootOrder is set the boot manager may fail because not all devices are
detected as "hunters" have not been running. E.g. nvme scan and usb start
are only invoked after the EFI boot manager.

I originally suggested to probe all boot devices before the boot manager
runs, but users complained that this slows down their non-EFI boot flows.
This is why I now move it after all boot methods but PXE.

Best regards

Heinrich

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5049 bytes --]

On Mon, May 05, 2025 at 09:51:52PM +0200, Heinrich Schuchardt wrote:
> Mark Kettenis <mark.kettenis@xs4all.nl> schrieb am Mo., 5. Mai 2025, 21:03:
> 
> > > Date: Fri, 2 May 2025 18:08:56 +0200
> > > From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > >
> > > On 5/2/25 16:49, Simon Glass wrote:
> > > > Hi Heinrich,
> > > >
> > > > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > > > <heinrich.schuchardt@canonical.com> wrote:
> > > >>
> > > >> The EFI boot manager bootmeth does not require variable BootOrder to
> > be
> > > >> preexisting. It creates this variable.
> > > >>
> > > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com
> > >
> > > >> ---
> > > >>   boot/bootmeth_efi_mgr.c | 21 +++------------------
> > > >>   1 file changed, 3 insertions(+), 18 deletions(-)
> > > >>
> > > >> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> > > >> index 42b8863815e..1669cbed5bd 100644
> > > >> --- a/boot/bootmeth_efi_mgr.c
> > > >> +++ b/boot/bootmeth_efi_mgr.c
> > > >> @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev,
> > struct bootflow_iter *iter)
> > > >>
> > > >>   static int efi_mgr_read_bootflow(struct udevice *dev, struct
> > bootflow *bflow)
> > > >>   {
> > > >> -       struct efi_mgr_priv *priv = dev_get_priv(dev);
> > > >> -       efi_status_t ret;
> > > >> -       efi_uintn_t size;
> > > >> -       u16 *bootorder;
> > > >> -
> > > >> -       if (priv->fake_dev) {
> > > >> -               bflow->state = BOOTFLOWST_READY;
> > > >> -               return 0;
> > > >> -       }
> > > >> +       int ret
> > > >>
> > > >>          ret = efi_init_obj_list();
> > > >>          if (ret)
> > > >>                  return log_msg_ret("init", ret);
> > > >>
> > > >> -       /* Enable this method if the "BootOrder" UEFI exists. */
> > > >> -       bootorder = efi_get_var(u"BootOrder",
> > &efi_global_variable_guid,
> > > >> -                               &size);
> > > >> -       if (bootorder) {
> > > >> -               free(bootorder);
> > > >> -               bflow->state = BOOTFLOWST_READY;
> > > >> -               return 0;
> > > >> -       }
> > > >> +       bflow->state = BOOTFLOWST_READY;
> > > >>
> > > >> -       return -EINVAL;
> > > >> +       return 0;
> > > >>   }
> > > >>
> > > >>   static int efi_mgr_read_file(struct udevice *dev, struct bootflow
> > *bflow,
> > > >> --
> > > >> 2.48.1
> > > >>
> > > >
> > > > How do we know if the board is using EFI bootmgr? My understanding was
> > > > that this was a way to find out?
> > >
> > > The boot manager must always run.
> > >
> > > The check for the BootOrder variable introduced in commit f2bfa0cb1794
> > > is a bug.
> >
> > Well, at the time the boot manager did not attempt to boot the default
> > path.  So there was no point in running the boot manager code unless
> > BootOrder (or BootNext) was set.  And of course before that commit the
> > boot manager didn't run at all on non-sandbox builds that had the
> > standard boot stuff enebaled.
> >
> > Anyway, I believe the thinking behind that commit is still sound.  As
> > I explained earlier, I think that...
> >
> > > The boot manager handles in sequence:
> > >
> > > * Try to boot as indicated by BootNext.
> > > * Try to boot as indicated by BootOrder.
> > > * Try to boot default path for available media.
> > >    This will add Boot#### entries and update BootOrder.
> >
> > ...doing this all in a monolithic sequence isn't the best way to
> > handle EFI boot in the u-boot ecosystem.
> >
> > Your series moves the boot manager further down the list because the
> > third step in the sequence has to happen late.  But as a result
> > BootNext and BootOrder processing will also happen late.  So what
> > happens if you have a board with two OS installations:
> >
> > 1. A generic Linux distro that boots via EFI.
> >
> > 2. Something like Armbian that provides an extlinux.conf file.
> >
> > Currently such a system will probably boot OS #1.  But after your
> >
> 
> This did not happen with distoboot. So migration from distroboot to
> standard boot results in a change that you want to avoid.
> 
> changes it will probably boot OS #2.  And if OS #1 sets BootOrder or
> > BootNext that will not change anything.
> >
> > So I think we need a solution where BootNext and BootOrder processing
> > happens early, like we do now.
> >
> 
> As of today BootNext does not invoke the boot manager.
> 
> If BootOrder is set the boot manager may fail because not all devices are
> detected as "hunters" have not been running. E.g. nvme scan and usb start
> are only invoked after the EFI boot manager.
> 
> I originally suggested to probe all boot devices before the boot manager
> runs, but users complained that this slows down their non-EFI boot flows.
> This is why I now move it after all boot methods but PXE.

Can we not see what BootOrder is and then ensure it's been "hunted" ?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Heinrich Schuchardt]

Tom Rini <trini@konsulko.com> schrieb am Mo., 5. Mai 2025, 21:54:

> On Mon, May 05, 2025 at 09:51:52PM +0200, Heinrich Schuchardt wrote:
> > Mark Kettenis <mark.kettenis@xs4all.nl> schrieb am Mo., 5. Mai 2025,
> 21:03:
> >
> > > > Date: Fri, 2 May 2025 18:08:56 +0200
> > > > From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > >
> > > > On 5/2/25 16:49, Simon Glass wrote:
> > > > > Hi Heinrich,
> > > > >
> > > > > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > > > > <heinrich.schuchardt@canonical.com> wrote:
> > > > >>
> > > > >> The EFI boot manager bootmeth does not require variable BootOrder
> to
> > > be
> > > > >> preexisting. It creates this variable.
> > > > >>
> > > > >> Signed-off-by: Heinrich Schuchardt <
> heinrich.schuchardt@canonical.com
> > > >
> > > > >> ---
> > > > >>   boot/bootmeth_efi_mgr.c | 21 +++------------------
> > > > >>   1 file changed, 3 insertions(+), 18 deletions(-)
> > > > >>
> > > > >> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> > > > >> index 42b8863815e..1669cbed5bd 100644
> > > > >> --- a/boot/bootmeth_efi_mgr.c
> > > > >> +++ b/boot/bootmeth_efi_mgr.c
> > > > >> @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev,
> > > struct bootflow_iter *iter)
> > > > >>
> > > > >>   static int efi_mgr_read_bootflow(struct udevice *dev, struct
> > > bootflow *bflow)
> > > > >>   {
> > > > >> -       struct efi_mgr_priv *priv = dev_get_priv(dev);
> > > > >> -       efi_status_t ret;
> > > > >> -       efi_uintn_t size;
> > > > >> -       u16 *bootorder;
> > > > >> -
> > > > >> -       if (priv->fake_dev) {
> > > > >> -               bflow->state = BOOTFLOWST_READY;
> > > > >> -               return 0;
> > > > >> -       }
> > > > >> +       int ret
> > > > >>
> > > > >>          ret = efi_init_obj_list();
> > > > >>          if (ret)
> > > > >>                  return log_msg_ret("init", ret);
> > > > >>
> > > > >> -       /* Enable this method if the "BootOrder" UEFI exists. */
> > > > >> -       bootorder = efi_get_var(u"BootOrder",
> > > &efi_global_variable_guid,
> > > > >> -                               &size);
> > > > >> -       if (bootorder) {
> > > > >> -               free(bootorder);
> > > > >> -               bflow->state = BOOTFLOWST_READY;
> > > > >> -               return 0;
> > > > >> -       }
> > > > >> +       bflow->state = BOOTFLOWST_READY;
> > > > >>
> > > > >> -       return -EINVAL;
> > > > >> +       return 0;
> > > > >>   }
> > > > >>
> > > > >>   static int efi_mgr_read_file(struct udevice *dev, struct
> bootflow
> > > *bflow,
> > > > >> --
> > > > >> 2.48.1
> > > > >>
> > > > >
> > > > > How do we know if the board is using EFI bootmgr? My understanding
> was
> > > > > that this was a way to find out?
> > > >
> > > > The boot manager must always run.
> > > >
> > > > The check for the BootOrder variable introduced in commit
> f2bfa0cb1794
> > > > is a bug.
> > >
> > > Well, at the time the boot manager did not attempt to boot the default
> > > path.  So there was no point in running the boot manager code unless
> > > BootOrder (or BootNext) was set.  And of course before that commit the
> > > boot manager didn't run at all on non-sandbox builds that had the
> > > standard boot stuff enebaled.
> > >
> > > Anyway, I believe the thinking behind that commit is still sound.  As
> > > I explained earlier, I think that...
> > >
> > > > The boot manager handles in sequence:
> > > >
> > > > * Try to boot as indicated by BootNext.
> > > > * Try to boot as indicated by BootOrder.
> > > > * Try to boot default path for available media.
> > > >    This will add Boot#### entries and update BootOrder.
> > >
> > > ...doing this all in a monolithic sequence isn't the best way to
> > > handle EFI boot in the u-boot ecosystem.
> > >
> > > Your series moves the boot manager further down the list because the
> > > third step in the sequence has to happen late.  But as a result
> > > BootNext and BootOrder processing will also happen late.  So what
> > > happens if you have a board with two OS installations:
> > >
> > > 1. A generic Linux distro that boots via EFI.
> > >
> > > 2. Something like Armbian that provides an extlinux.conf file.
> > >
> > > Currently such a system will probably boot OS #1.  But after your
> > >
> >
> > This did not happen with distoboot. So migration from distroboot to
> > standard boot results in a change that you want to avoid.
> >
> > changes it will probably boot OS #2.  And if OS #1 sets BootOrder or
> > > BootNext that will not change anything.
> > >
> > > So I think we need a solution where BootNext and BootOrder processing
> > > happens early, like we do now.
> > >
> >
> > As of today BootNext does not invoke the boot manager.
> >
> > If BootOrder is set the boot manager may fail because not all devices are
> > detected as "hunters" have not been running. E.g. nvme scan and usb start
> > are only invoked after the EFI boot manager.
> >
> > I originally suggested to probe all boot devices before the boot manager
> > runs, but users complained that this slows down their non-EFI boot flows.
> > This is why I now move it after all boot methods but PXE.
>
> Can we not see what BootOrder is and then ensure it's been "hunted" ?
>
> --
> Tom
>

A load option may only contain the partition GUID and the file path. In
this case you wouldn't be able to tell whether it is for an NVMe drive, or
a USB stick.

As numbering of devices cannot be expected to be stable, it is preferable
to use this short form of device paths for load options.

Best regards

Heinrich

>

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6256 bytes --]

On Tue, May 06, 2025 at 12:07:20AM +0200, Heinrich Schuchardt wrote:
> Tom Rini <trini@konsulko.com> schrieb am Mo., 5. Mai 2025, 21:54:
> 
> > On Mon, May 05, 2025 at 09:51:52PM +0200, Heinrich Schuchardt wrote:
> > > Mark Kettenis <mark.kettenis@xs4all.nl> schrieb am Mo., 5. Mai 2025,
> > 21:03:
> > >
> > > > > Date: Fri, 2 May 2025 18:08:56 +0200
> > > > > From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > >
> > > > > On 5/2/25 16:49, Simon Glass wrote:
> > > > > > Hi Heinrich,
> > > > > >
> > > > > > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > > > > > <heinrich.schuchardt@canonical.com> wrote:
> > > > > >>
> > > > > >> The EFI boot manager bootmeth does not require variable BootOrder
> > to
> > > > be
> > > > > >> preexisting. It creates this variable.
> > > > > >>
> > > > > >> Signed-off-by: Heinrich Schuchardt <
> > heinrich.schuchardt@canonical.com
> > > > >
> > > > > >> ---
> > > > > >>   boot/bootmeth_efi_mgr.c | 21 +++------------------
> > > > > >>   1 file changed, 3 insertions(+), 18 deletions(-)
> > > > > >>
> > > > > >> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> > > > > >> index 42b8863815e..1669cbed5bd 100644
> > > > > >> --- a/boot/bootmeth_efi_mgr.c
> > > > > >> +++ b/boot/bootmeth_efi_mgr.c
> > > > > >> @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev,
> > > > struct bootflow_iter *iter)
> > > > > >>
> > > > > >>   static int efi_mgr_read_bootflow(struct udevice *dev, struct
> > > > bootflow *bflow)
> > > > > >>   {
> > > > > >> -       struct efi_mgr_priv *priv = dev_get_priv(dev);
> > > > > >> -       efi_status_t ret;
> > > > > >> -       efi_uintn_t size;
> > > > > >> -       u16 *bootorder;
> > > > > >> -
> > > > > >> -       if (priv->fake_dev) {
> > > > > >> -               bflow->state = BOOTFLOWST_READY;
> > > > > >> -               return 0;
> > > > > >> -       }
> > > > > >> +       int ret
> > > > > >>
> > > > > >>          ret = efi_init_obj_list();
> > > > > >>          if (ret)
> > > > > >>                  return log_msg_ret("init", ret);
> > > > > >>
> > > > > >> -       /* Enable this method if the "BootOrder" UEFI exists. */
> > > > > >> -       bootorder = efi_get_var(u"BootOrder",
> > > > &efi_global_variable_guid,
> > > > > >> -                               &size);
> > > > > >> -       if (bootorder) {
> > > > > >> -               free(bootorder);
> > > > > >> -               bflow->state = BOOTFLOWST_READY;
> > > > > >> -               return 0;
> > > > > >> -       }
> > > > > >> +       bflow->state = BOOTFLOWST_READY;
> > > > > >>
> > > > > >> -       return -EINVAL;
> > > > > >> +       return 0;
> > > > > >>   }
> > > > > >>
> > > > > >>   static int efi_mgr_read_file(struct udevice *dev, struct
> > bootflow
> > > > *bflow,
> > > > > >> --
> > > > > >> 2.48.1
> > > > > >>
> > > > > >
> > > > > > How do we know if the board is using EFI bootmgr? My understanding
> > was
> > > > > > that this was a way to find out?
> > > > >
> > > > > The boot manager must always run.
> > > > >
> > > > > The check for the BootOrder variable introduced in commit
> > f2bfa0cb1794
> > > > > is a bug.
> > > >
> > > > Well, at the time the boot manager did not attempt to boot the default
> > > > path.  So there was no point in running the boot manager code unless
> > > > BootOrder (or BootNext) was set.  And of course before that commit the
> > > > boot manager didn't run at all on non-sandbox builds that had the
> > > > standard boot stuff enebaled.
> > > >
> > > > Anyway, I believe the thinking behind that commit is still sound.  As
> > > > I explained earlier, I think that...
> > > >
> > > > > The boot manager handles in sequence:
> > > > >
> > > > > * Try to boot as indicated by BootNext.
> > > > > * Try to boot as indicated by BootOrder.
> > > > > * Try to boot default path for available media.
> > > > >    This will add Boot#### entries and update BootOrder.
> > > >
> > > > ...doing this all in a monolithic sequence isn't the best way to
> > > > handle EFI boot in the u-boot ecosystem.
> > > >
> > > > Your series moves the boot manager further down the list because the
> > > > third step in the sequence has to happen late.  But as a result
> > > > BootNext and BootOrder processing will also happen late.  So what
> > > > happens if you have a board with two OS installations:
> > > >
> > > > 1. A generic Linux distro that boots via EFI.
> > > >
> > > > 2. Something like Armbian that provides an extlinux.conf file.
> > > >
> > > > Currently such a system will probably boot OS #1.  But after your
> > > >
> > >
> > > This did not happen with distoboot. So migration from distroboot to
> > > standard boot results in a change that you want to avoid.
> > >
> > > changes it will probably boot OS #2.  And if OS #1 sets BootOrder or
> > > > BootNext that will not change anything.
> > > >
> > > > So I think we need a solution where BootNext and BootOrder processing
> > > > happens early, like we do now.
> > > >
> > >
> > > As of today BootNext does not invoke the boot manager.
> > >
> > > If BootOrder is set the boot manager may fail because not all devices are
> > > detected as "hunters" have not been running. E.g. nvme scan and usb start
> > > are only invoked after the EFI boot manager.
> > >
> > > I originally suggested to probe all boot devices before the boot manager
> > > runs, but users complained that this slows down their non-EFI boot flows.
> > > This is why I now move it after all boot methods but PXE.
> >
> > Can we not see what BootOrder is and then ensure it's been "hunted" ?
> >
> > --
> > Tom
> >
> 
> A load option may only contain the partition GUID and the file path. In
> this case you wouldn't be able to tell whether it is for an NVMe drive, or
> a USB stick.
> 
> As numbering of devices cannot be expected to be stable, it is preferable
> to use this short form of device paths for load options.

Ah, OK. So could we check and if not found print a human understandable
error message, and continue on? I want to figure out some path that does
not change the pre-bootstd behavior.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Heinrich Schuchardt]

On 5/6/25 00:10, Tom Rini wrote:
> On Tue, May 06, 2025 at 12:07:20AM +0200, Heinrich Schuchardt wrote:
>> Tom Rini <trini@konsulko.com> schrieb am Mo., 5. Mai 2025, 21:54:
>>
>>> On Mon, May 05, 2025 at 09:51:52PM +0200, Heinrich Schuchardt wrote:
>>>> Mark Kettenis <mark.kettenis@xs4all.nl> schrieb am Mo., 5. Mai 2025,
>>> 21:03:
>>>>
>>>>>> Date: Fri, 2 May 2025 18:08:56 +0200
>>>>>> From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>>>>
>>>>>> On 5/2/25 16:49, Simon Glass wrote:
>>>>>>> Hi Heinrich,
>>>>>>>
>>>>>>> On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
>>>>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>>>>
>>>>>>>> The EFI boot manager bootmeth does not require variable BootOrder
>>> to
>>>>> be
>>>>>>>> preexisting. It creates this variable.
>>>>>>>>
>>>>>>>> Signed-off-by: Heinrich Schuchardt <
>>> heinrich.schuchardt@canonical.com
>>>>>>
>>>>>>>> ---
>>>>>>>>    boot/bootmeth_efi_mgr.c | 21 +++------------------
>>>>>>>>    1 file changed, 3 insertions(+), 18 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
>>>>>>>> index 42b8863815e..1669cbed5bd 100644
>>>>>>>> --- a/boot/bootmeth_efi_mgr.c
>>>>>>>> +++ b/boot/bootmeth_efi_mgr.c
>>>>>>>> @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev,
>>>>> struct bootflow_iter *iter)
>>>>>>>>
>>>>>>>>    static int efi_mgr_read_bootflow(struct udevice *dev, struct
>>>>> bootflow *bflow)
>>>>>>>>    {
>>>>>>>> -       struct efi_mgr_priv *priv = dev_get_priv(dev);
>>>>>>>> -       efi_status_t ret;
>>>>>>>> -       efi_uintn_t size;
>>>>>>>> -       u16 *bootorder;
>>>>>>>> -
>>>>>>>> -       if (priv->fake_dev) {
>>>>>>>> -               bflow->state = BOOTFLOWST_READY;
>>>>>>>> -               return 0;
>>>>>>>> -       }
>>>>>>>> +       int ret
>>>>>>>>
>>>>>>>>           ret = efi_init_obj_list();
>>>>>>>>           if (ret)
>>>>>>>>                   return log_msg_ret("init", ret);
>>>>>>>>
>>>>>>>> -       /* Enable this method if the "BootOrder" UEFI exists. */
>>>>>>>> -       bootorder = efi_get_var(u"BootOrder",
>>>>> &efi_global_variable_guid,
>>>>>>>> -                               &size);
>>>>>>>> -       if (bootorder) {
>>>>>>>> -               free(bootorder);
>>>>>>>> -               bflow->state = BOOTFLOWST_READY;
>>>>>>>> -               return 0;
>>>>>>>> -       }
>>>>>>>> +       bflow->state = BOOTFLOWST_READY;
>>>>>>>>
>>>>>>>> -       return -EINVAL;
>>>>>>>> +       return 0;
>>>>>>>>    }
>>>>>>>>
>>>>>>>>    static int efi_mgr_read_file(struct udevice *dev, struct
>>> bootflow
>>>>> *bflow,
>>>>>>>> --
>>>>>>>> 2.48.1
>>>>>>>>
>>>>>>>
>>>>>>> How do we know if the board is using EFI bootmgr? My understanding
>>> was
>>>>>>> that this was a way to find out?
>>>>>>
>>>>>> The boot manager must always run.
>>>>>>
>>>>>> The check for the BootOrder variable introduced in commit
>>> f2bfa0cb1794
>>>>>> is a bug.
>>>>>
>>>>> Well, at the time the boot manager did not attempt to boot the default
>>>>> path.  So there was no point in running the boot manager code unless
>>>>> BootOrder (or BootNext) was set.  And of course before that commit the
>>>>> boot manager didn't run at all on non-sandbox builds that had the
>>>>> standard boot stuff enebaled.
>>>>>
>>>>> Anyway, I believe the thinking behind that commit is still sound.  As
>>>>> I explained earlier, I think that...
>>>>>
>>>>>> The boot manager handles in sequence:
>>>>>>
>>>>>> * Try to boot as indicated by BootNext.
>>>>>> * Try to boot as indicated by BootOrder.
>>>>>> * Try to boot default path for available media.
>>>>>>     This will add Boot#### entries and update BootOrder.
>>>>>
>>>>> ...doing this all in a monolithic sequence isn't the best way to
>>>>> handle EFI boot in the u-boot ecosystem.
>>>>>
>>>>> Your series moves the boot manager further down the list because the
>>>>> third step in the sequence has to happen late.  But as a result
>>>>> BootNext and BootOrder processing will also happen late.  So what
>>>>> happens if you have a board with two OS installations:
>>>>>
>>>>> 1. A generic Linux distro that boots via EFI.
>>>>>
>>>>> 2. Something like Armbian that provides an extlinux.conf file.
>>>>>
>>>>> Currently such a system will probably boot OS #1.  But after your
>>>>>
>>>>
>>>> This did not happen with distoboot. So migration from distroboot to
>>>> standard boot results in a change that you want to avoid.
>>>>
>>>> changes it will probably boot OS #2.  And if OS #1 sets BootOrder or
>>>>> BootNext that will not change anything.
>>>>>
>>>>> So I think we need a solution where BootNext and BootOrder processing
>>>>> happens early, like we do now.
>>>>>
>>>>
>>>> As of today BootNext does not invoke the boot manager.
>>>>
>>>> If BootOrder is set the boot manager may fail because not all devices are
>>>> detected as "hunters" have not been running. E.g. nvme scan and usb start
>>>> are only invoked after the EFI boot manager.
>>>>
>>>> I originally suggested to probe all boot devices before the boot manager
>>>> runs, but users complained that this slows down their non-EFI boot flows.
>>>> This is why I now move it after all boot methods but PXE.
>>>
>>> Can we not see what BootOrder is and then ensure it's been "hunted" ?
>>>
>>> --
>>> Tom
>>>
>>
>> A load option may only contain the partition GUID and the file path. In
>> this case you wouldn't be able to tell whether it is for an NVMe drive, or
>> a USB stick.
>>
>> As numbering of devices cannot be expected to be stable, it is preferable
>> to use this short form of device paths for load options.
> 
> Ah, OK. So could we check and if not found print a human understandable
> error message, and continue on? I want to figure out some path that does
> not change the pre-bootstd behavior.
> 

In distroboot scan_dev_for_efi is invoked for every boot device and each 
time calls the boot manager. As usb start is only called in usb_boot 
this will result in behavior that is as non-compliant with the UEFI 
specification.

We should reach a behavior that complies with the UEFI specification.
Not changing former distro boot behavior is not a valid option.

Best regards

Heinrich

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 7821 bytes --]

On Tue, May 06, 2025 at 01:18:16AM +0200, Heinrich Schuchardt wrote:
> On 5/6/25 00:10, Tom Rini wrote:
> > On Tue, May 06, 2025 at 12:07:20AM +0200, Heinrich Schuchardt wrote:
> > > Tom Rini <trini@konsulko.com> schrieb am Mo., 5. Mai 2025, 21:54:
> > > 
> > > > On Mon, May 05, 2025 at 09:51:52PM +0200, Heinrich Schuchardt wrote:
> > > > > Mark Kettenis <mark.kettenis@xs4all.nl> schrieb am Mo., 5. Mai 2025,
> > > > 21:03:
> > > > > 
> > > > > > > Date: Fri, 2 May 2025 18:08:56 +0200
> > > > > > > From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > > > > 
> > > > > > > On 5/2/25 16:49, Simon Glass wrote:
> > > > > > > > Hi Heinrich,
> > > > > > > > 
> > > > > > > > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > > > > > > > <heinrich.schuchardt@canonical.com> wrote:
> > > > > > > > > 
> > > > > > > > > The EFI boot manager bootmeth does not require variable BootOrder
> > > > to
> > > > > > be
> > > > > > > > > preexisting. It creates this variable.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Heinrich Schuchardt <
> > > > heinrich.schuchardt@canonical.com
> > > > > > > 
> > > > > > > > > ---
> > > > > > > > >    boot/bootmeth_efi_mgr.c | 21 +++------------------
> > > > > > > > >    1 file changed, 3 insertions(+), 18 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> > > > > > > > > index 42b8863815e..1669cbed5bd 100644
> > > > > > > > > --- a/boot/bootmeth_efi_mgr.c
> > > > > > > > > +++ b/boot/bootmeth_efi_mgr.c
> > > > > > > > > @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev,
> > > > > > struct bootflow_iter *iter)
> > > > > > > > > 
> > > > > > > > >    static int efi_mgr_read_bootflow(struct udevice *dev, struct
> > > > > > bootflow *bflow)
> > > > > > > > >    {
> > > > > > > > > -       struct efi_mgr_priv *priv = dev_get_priv(dev);
> > > > > > > > > -       efi_status_t ret;
> > > > > > > > > -       efi_uintn_t size;
> > > > > > > > > -       u16 *bootorder;
> > > > > > > > > -
> > > > > > > > > -       if (priv->fake_dev) {
> > > > > > > > > -               bflow->state = BOOTFLOWST_READY;
> > > > > > > > > -               return 0;
> > > > > > > > > -       }
> > > > > > > > > +       int ret
> > > > > > > > > 
> > > > > > > > >           ret = efi_init_obj_list();
> > > > > > > > >           if (ret)
> > > > > > > > >                   return log_msg_ret("init", ret);
> > > > > > > > > 
> > > > > > > > > -       /* Enable this method if the "BootOrder" UEFI exists. */
> > > > > > > > > -       bootorder = efi_get_var(u"BootOrder",
> > > > > > &efi_global_variable_guid,
> > > > > > > > > -                               &size);
> > > > > > > > > -       if (bootorder) {
> > > > > > > > > -               free(bootorder);
> > > > > > > > > -               bflow->state = BOOTFLOWST_READY;
> > > > > > > > > -               return 0;
> > > > > > > > > -       }
> > > > > > > > > +       bflow->state = BOOTFLOWST_READY;
> > > > > > > > > 
> > > > > > > > > -       return -EINVAL;
> > > > > > > > > +       return 0;
> > > > > > > > >    }
> > > > > > > > > 
> > > > > > > > >    static int efi_mgr_read_file(struct udevice *dev, struct
> > > > bootflow
> > > > > > *bflow,
> > > > > > > > > --
> > > > > > > > > 2.48.1
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > How do we know if the board is using EFI bootmgr? My understanding
> > > > was
> > > > > > > > that this was a way to find out?
> > > > > > > 
> > > > > > > The boot manager must always run.
> > > > > > > 
> > > > > > > The check for the BootOrder variable introduced in commit
> > > > f2bfa0cb1794
> > > > > > > is a bug.
> > > > > > 
> > > > > > Well, at the time the boot manager did not attempt to boot the default
> > > > > > path.  So there was no point in running the boot manager code unless
> > > > > > BootOrder (or BootNext) was set.  And of course before that commit the
> > > > > > boot manager didn't run at all on non-sandbox builds that had the
> > > > > > standard boot stuff enebaled.
> > > > > > 
> > > > > > Anyway, I believe the thinking behind that commit is still sound.  As
> > > > > > I explained earlier, I think that...
> > > > > > 
> > > > > > > The boot manager handles in sequence:
> > > > > > > 
> > > > > > > * Try to boot as indicated by BootNext.
> > > > > > > * Try to boot as indicated by BootOrder.
> > > > > > > * Try to boot default path for available media.
> > > > > > >     This will add Boot#### entries and update BootOrder.
> > > > > > 
> > > > > > ...doing this all in a monolithic sequence isn't the best way to
> > > > > > handle EFI boot in the u-boot ecosystem.
> > > > > > 
> > > > > > Your series moves the boot manager further down the list because the
> > > > > > third step in the sequence has to happen late.  But as a result
> > > > > > BootNext and BootOrder processing will also happen late.  So what
> > > > > > happens if you have a board with two OS installations:
> > > > > > 
> > > > > > 1. A generic Linux distro that boots via EFI.
> > > > > > 
> > > > > > 2. Something like Armbian that provides an extlinux.conf file.
> > > > > > 
> > > > > > Currently such a system will probably boot OS #1.  But after your
> > > > > > 
> > > > > 
> > > > > This did not happen with distoboot. So migration from distroboot to
> > > > > standard boot results in a change that you want to avoid.
> > > > > 
> > > > > changes it will probably boot OS #2.  And if OS #1 sets BootOrder or
> > > > > > BootNext that will not change anything.
> > > > > > 
> > > > > > So I think we need a solution where BootNext and BootOrder processing
> > > > > > happens early, like we do now.
> > > > > > 
> > > > > 
> > > > > As of today BootNext does not invoke the boot manager.
> > > > > 
> > > > > If BootOrder is set the boot manager may fail because not all devices are
> > > > > detected as "hunters" have not been running. E.g. nvme scan and usb start
> > > > > are only invoked after the EFI boot manager.
> > > > > 
> > > > > I originally suggested to probe all boot devices before the boot manager
> > > > > runs, but users complained that this slows down their non-EFI boot flows.
> > > > > This is why I now move it after all boot methods but PXE.
> > > > 
> > > > Can we not see what BootOrder is and then ensure it's been "hunted" ?
> > > > 
> > > > --
> > > > Tom
> > > > 
> > > 
> > > A load option may only contain the partition GUID and the file path. In
> > > this case you wouldn't be able to tell whether it is for an NVMe drive, or
> > > a USB stick.
> > > 
> > > As numbering of devices cannot be expected to be stable, it is preferable
> > > to use this short form of device paths for load options.
> > 
> > Ah, OK. So could we check and if not found print a human understandable
> > error message, and continue on? I want to figure out some path that does
> > not change the pre-bootstd behavior.
> > 
> 
> In distroboot scan_dev_for_efi is invoked for every boot device and each
> time calls the boot manager. As usb start is only called in usb_boot this
> will result in behavior that is as non-compliant with the UEFI
> specification.
> 
> We should reach a behavior that complies with the UEFI specification.
> Not changing former distro boot behavior is not a valid option.

Can we also not figure out some way to boot promptly? I was thinking
something along the lines of if we know BootOrder (or similar), we try
it. If we don't find it, we try again a bit later on when we can run all
the hunting that might be slow. If the only option is slow-but-compliant
I feel like a lot of use cases will opt out of using EFI boot instead.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Mark Kettenis]

> Date: Mon, 5 May 2025 17:43:19 -0600
> From: Tom Rini <trini@konsulko.com>
> 
> On Tue, May 06, 2025 at 01:18:16AM +0200, Heinrich Schuchardt wrote:
> > On 5/6/25 00:10, Tom Rini wrote:
> > > On Tue, May 06, 2025 at 12:07:20AM +0200, Heinrich Schuchardt wrote:
> > > > Tom Rini <trini@konsulko.com> schrieb am Mo., 5. Mai 2025, 21:54:
> > > > 
> > > > > On Mon, May 05, 2025 at 09:51:52PM +0200, Heinrich Schuchardt wrote:
> > > > > > Mark Kettenis <mark.kettenis@xs4all.nl> schrieb am Mo., 5. Mai 2025,
> > > > > 21:03:
> > > > > > 
> > > > > > > > Date: Fri, 2 May 2025 18:08:56 +0200
> > > > > > > > From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > > > > > 
> > > > > > > > On 5/2/25 16:49, Simon Glass wrote:
> > > > > > > > > Hi Heinrich,
> > > > > > > > > 
> > > > > > > > > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > > > > > > > > <heinrich.schuchardt@canonical.com> wrote:
> > > > > > > > > > 
> > > > > > > > > > The EFI boot manager bootmeth does not require variable BootOrder
> > > > > to
> > > > > > > be
> > > > > > > > > > preexisting. It creates this variable.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Heinrich Schuchardt <
> > > > > heinrich.schuchardt@canonical.com
> > > > > > > > 
> > > > > > > > > > ---
> > > > > > > > > >    boot/bootmeth_efi_mgr.c | 21 +++------------------
> > > > > > > > > >    1 file changed, 3 insertions(+), 18 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> > > > > > > > > > index 42b8863815e..1669cbed5bd 100644
> > > > > > > > > > --- a/boot/bootmeth_efi_mgr.c
> > > > > > > > > > +++ b/boot/bootmeth_efi_mgr.c
> > > > > > > > > > @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev,
> > > > > > > struct bootflow_iter *iter)
> > > > > > > > > > 
> > > > > > > > > >    static int efi_mgr_read_bootflow(struct udevice *dev, struct
> > > > > > > bootflow *bflow)
> > > > > > > > > >    {
> > > > > > > > > > -       struct efi_mgr_priv *priv = dev_get_priv(dev);
> > > > > > > > > > -       efi_status_t ret;
> > > > > > > > > > -       efi_uintn_t size;
> > > > > > > > > > -       u16 *bootorder;
> > > > > > > > > > -
> > > > > > > > > > -       if (priv->fake_dev) {
> > > > > > > > > > -               bflow->state = BOOTFLOWST_READY;
> > > > > > > > > > -               return 0;
> > > > > > > > > > -       }
> > > > > > > > > > +       int ret
> > > > > > > > > > 
> > > > > > > > > >           ret = efi_init_obj_list();
> > > > > > > > > >           if (ret)
> > > > > > > > > >                   return log_msg_ret("init", ret);
> > > > > > > > > > 
> > > > > > > > > > -       /* Enable this method if the "BootOrder" UEFI exists. */
> > > > > > > > > > -       bootorder = efi_get_var(u"BootOrder",
> > > > > > > &efi_global_variable_guid,
> > > > > > > > > > -                               &size);
> > > > > > > > > > -       if (bootorder) {
> > > > > > > > > > -               free(bootorder);
> > > > > > > > > > -               bflow->state = BOOTFLOWST_READY;
> > > > > > > > > > -               return 0;
> > > > > > > > > > -       }
> > > > > > > > > > +       bflow->state = BOOTFLOWST_READY;
> > > > > > > > > > 
> > > > > > > > > > -       return -EINVAL;
> > > > > > > > > > +       return 0;
> > > > > > > > > >    }
> > > > > > > > > > 
> > > > > > > > > >    static int efi_mgr_read_file(struct udevice *dev, struct
> > > > > bootflow
> > > > > > > *bflow,
> > > > > > > > > > --
> > > > > > > > > > 2.48.1
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > How do we know if the board is using EFI bootmgr? My understanding
> > > > > was
> > > > > > > > > that this was a way to find out?
> > > > > > > > 
> > > > > > > > The boot manager must always run.
> > > > > > > > 
> > > > > > > > The check for the BootOrder variable introduced in commit
> > > > > f2bfa0cb1794
> > > > > > > > is a bug.
> > > > > > > 
> > > > > > > Well, at the time the boot manager did not attempt to boot the default
> > > > > > > path.  So there was no point in running the boot manager code unless
> > > > > > > BootOrder (or BootNext) was set.  And of course before that commit the
> > > > > > > boot manager didn't run at all on non-sandbox builds that had the
> > > > > > > standard boot stuff enebaled.
> > > > > > > 
> > > > > > > Anyway, I believe the thinking behind that commit is still sound.  As
> > > > > > > I explained earlier, I think that...
> > > > > > > 
> > > > > > > > The boot manager handles in sequence:
> > > > > > > > 
> > > > > > > > * Try to boot as indicated by BootNext.
> > > > > > > > * Try to boot as indicated by BootOrder.
> > > > > > > > * Try to boot default path for available media.
> > > > > > > >     This will add Boot#### entries and update BootOrder.
> > > > > > > 
> > > > > > > ...doing this all in a monolithic sequence isn't the best way to
> > > > > > > handle EFI boot in the u-boot ecosystem.
> > > > > > > 
> > > > > > > Your series moves the boot manager further down the list because the
> > > > > > > third step in the sequence has to happen late.  But as a result
> > > > > > > BootNext and BootOrder processing will also happen late.  So what
> > > > > > > happens if you have a board with two OS installations:
> > > > > > > 
> > > > > > > 1. A generic Linux distro that boots via EFI.
> > > > > > > 
> > > > > > > 2. Something like Armbian that provides an extlinux.conf file.
> > > > > > > 
> > > > > > > Currently such a system will probably boot OS #1.  But after your
> > > > > > > 
> > > > > > 
> > > > > > This did not happen with distoboot. So migration from distroboot to
> > > > > > standard boot results in a change that you want to avoid.
> > > > > > 
> > > > > > changes it will probably boot OS #2.  And if OS #1 sets BootOrder or
> > > > > > > BootNext that will not change anything.
> > > > > > > 
> > > > > > > So I think we need a solution where BootNext and BootOrder processing
> > > > > > > happens early, like we do now.
> > > > > > > 
> > > > > > 
> > > > > > As of today BootNext does not invoke the boot manager.
> > > > > > 
> > > > > > If BootOrder is set the boot manager may fail because not all devices are
> > > > > > detected as "hunters" have not been running. E.g. nvme scan and usb start
> > > > > > are only invoked after the EFI boot manager.
> > > > > > 
> > > > > > I originally suggested to probe all boot devices before the boot manager
> > > > > > runs, but users complained that this slows down their non-EFI boot flows.
> > > > > > This is why I now move it after all boot methods but PXE.
> > > > > 
> > > > > Can we not see what BootOrder is and then ensure it's been "hunted" ?
> > > > > 
> > > > > --
> > > > > Tom
> > > > > 
> > > > 
> > > > A load option may only contain the partition GUID and the file path. In
> > > > this case you wouldn't be able to tell whether it is for an NVMe drive, or
> > > > a USB stick.
> > > > 
> > > > As numbering of devices cannot be expected to be stable, it is preferable
> > > > to use this short form of device paths for load options.
> > > 
> > > Ah, OK. So could we check and if not found print a human understandable
> > > error message, and continue on? I want to figure out some path that does
> > > not change the pre-bootstd behavior.
> > > 
> > 
> > In distroboot scan_dev_for_efi is invoked for every boot device and each
> > time calls the boot manager. As usb start is only called in usb_boot this
> > will result in behavior that is as non-compliant with the UEFI
> > specification.
> > 
> > We should reach a behavior that complies with the UEFI specification.
> > Not changing former distro boot behavior is not a valid option.
> 
> Can we also not figure out some way to boot promptly? I was thinking
> something along the lines of if we know BootOrder (or similar), we try
> it. If we don't find it, we try again a bit later on when we can run all
> the hunting that might be slow. If the only option is slow-but-compliant
> I feel like a lot of use cases will opt out of using EFI boot instead.

I think that is possible.  But it will require tighter integration
integration between the EFI loader and the standard boot device
hunting code such that devices are probed as we walk to options
specified by the BootOrder.

A complication here is that a Boot#### option doesn't necessearily
have the device encoded into the boot option.  An option can reference
just the GUID of a partition and many OS installers create such
options.  So if we see such an option we do need to probe devices
until we find the right partition.

Another complication is that currently the
efi_bootmgr_update_media_device_boot_option() function will append
Boot#### options it creates to the BootOrder variable.

This is used to implement the following requirement in the spec:

  The boot manager must also support booting from a short-form device
  path that starts with the first element being a File Path Media
  Device Path (File Path Media Device Path ). When the boot manager
  attempts to boot a short-form File Path Media Device Path, it will
  enumerate all removable media devices, followed by all fixed media
  devices, creating boot options for each device.

However, a the spec also says that:

  These new boot options must not be saved to non volatile storage,
  and may not be added to BootOrder.

But we we do add those options to BootOrder and we I believe we will
save those new Boot#### options as well.

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Simon Glass]

Hi Mark, Tom,

On Tue, 6 May 2025 at 12:24, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > Date: Mon, 5 May 2025 17:43:19 -0600
> > From: Tom Rini <trini@konsulko.com>
> >
> > On Tue, May 06, 2025 at 01:18:16AM +0200, Heinrich Schuchardt wrote:
> > > On 5/6/25 00:10, Tom Rini wrote:
> > > > On Tue, May 06, 2025 at 12:07:20AM +0200, Heinrich Schuchardt wrote:
> > > > > Tom Rini <trini@konsulko.com> schrieb am Mo., 5. Mai 2025, 21:54:
> > > > >
> > > > > > On Mon, May 05, 2025 at 09:51:52PM +0200, Heinrich Schuchardt wrote:
> > > > > > > Mark Kettenis <mark.kettenis@xs4all.nl> schrieb am Mo., 5. Mai 2025,
> > > > > > 21:03:
> > > > > > >
> > > > > > > > > Date: Fri, 2 May 2025 18:08:56 +0200
> > > > > > > > > From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > > > > > >
> > > > > > > > > On 5/2/25 16:49, Simon Glass wrote:
> > > > > > > > > > Hi Heinrich,
> > > > > > > > > >
> > > > > > > > > > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > > > > > > > > > <heinrich.schuchardt@canonical.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > The EFI boot manager bootmeth does not require variable BootOrder
> > > > > > to
> > > > > > > > be
> > > > > > > > > > > preexisting. It creates this variable.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Heinrich Schuchardt <
> > > > > > heinrich.schuchardt@canonical.com
> > > > > > > > >
> > > > > > > > > > > ---
> > > > > > > > > > >    boot/bootmeth_efi_mgr.c | 21 +++------------------
> > > > > > > > > > >    1 file changed, 3 insertions(+), 18 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> > > > > > > > > > > index 42b8863815e..1669cbed5bd 100644
> > > > > > > > > > > --- a/boot/bootmeth_efi_mgr.c
> > > > > > > > > > > +++ b/boot/bootmeth_efi_mgr.c
> > > > > > > > > > > @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev,
> > > > > > > > struct bootflow_iter *iter)
> > > > > > > > > > >
> > > > > > > > > > >    static int efi_mgr_read_bootflow(struct udevice *dev, struct
> > > > > > > > bootflow *bflow)
> > > > > > > > > > >    {
> > > > > > > > > > > -       struct efi_mgr_priv *priv = dev_get_priv(dev);
> > > > > > > > > > > -       efi_status_t ret;
> > > > > > > > > > > -       efi_uintn_t size;
> > > > > > > > > > > -       u16 *bootorder;
> > > > > > > > > > > -
> > > > > > > > > > > -       if (priv->fake_dev) {
> > > > > > > > > > > -               bflow->state = BOOTFLOWST_READY;
> > > > > > > > > > > -               return 0;
> > > > > > > > > > > -       }
> > > > > > > > > > > +       int ret
> > > > > > > > > > >
> > > > > > > > > > >           ret = efi_init_obj_list();
> > > > > > > > > > >           if (ret)
> > > > > > > > > > >                   return log_msg_ret("init", ret);
> > > > > > > > > > >
> > > > > > > > > > > -       /* Enable this method if the "BootOrder" UEFI exists. */
> > > > > > > > > > > -       bootorder = efi_get_var(u"BootOrder",
> > > > > > > > &efi_global_variable_guid,
> > > > > > > > > > > -                               &size);
> > > > > > > > > > > -       if (bootorder) {
> > > > > > > > > > > -               free(bootorder);
> > > > > > > > > > > -               bflow->state = BOOTFLOWST_READY;
> > > > > > > > > > > -               return 0;
> > > > > > > > > > > -       }
> > > > > > > > > > > +       bflow->state = BOOTFLOWST_READY;
> > > > > > > > > > >
> > > > > > > > > > > -       return -EINVAL;
> > > > > > > > > > > +       return 0;
> > > > > > > > > > >    }
> > > > > > > > > > >
> > > > > > > > > > >    static int efi_mgr_read_file(struct udevice *dev, struct
> > > > > > bootflow
> > > > > > > > *bflow,
> > > > > > > > > > > --
> > > > > > > > > > > 2.48.1
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > How do we know if the board is using EFI bootmgr? My understanding
> > > > > > was
> > > > > > > > > > that this was a way to find out?
> > > > > > > > >
> > > > > > > > > The boot manager must always run.
> > > > > > > > >
> > > > > > > > > The check for the BootOrder variable introduced in commit
> > > > > > f2bfa0cb1794
> > > > > > > > > is a bug.
> > > > > > > >
> > > > > > > > Well, at the time the boot manager did not attempt to boot the default
> > > > > > > > path.  So there was no point in running the boot manager code unless
> > > > > > > > BootOrder (or BootNext) was set.  And of course before that commit the
> > > > > > > > boot manager didn't run at all on non-sandbox builds that had the
> > > > > > > > standard boot stuff enebaled.
> > > > > > > >
> > > > > > > > Anyway, I believe the thinking behind that commit is still sound.  As
> > > > > > > > I explained earlier, I think that...
> > > > > > > >
> > > > > > > > > The boot manager handles in sequence:
> > > > > > > > >
> > > > > > > > > * Try to boot as indicated by BootNext.
> > > > > > > > > * Try to boot as indicated by BootOrder.
> > > > > > > > > * Try to boot default path for available media.
> > > > > > > > >     This will add Boot#### entries and update BootOrder.
> > > > > > > >
> > > > > > > > ...doing this all in a monolithic sequence isn't the best way to
> > > > > > > > handle EFI boot in the u-boot ecosystem.
> > > > > > > >
> > > > > > > > Your series moves the boot manager further down the list because the
> > > > > > > > third step in the sequence has to happen late.  But as a result
> > > > > > > > BootNext and BootOrder processing will also happen late.  So what
> > > > > > > > happens if you have a board with two OS installations:
> > > > > > > >
> > > > > > > > 1. A generic Linux distro that boots via EFI.
> > > > > > > >
> > > > > > > > 2. Something like Armbian that provides an extlinux.conf file.
> > > > > > > >
> > > > > > > > Currently such a system will probably boot OS #1.  But after your
> > > > > > > >
> > > > > > >
> > > > > > > This did not happen with distoboot. So migration from distroboot to
> > > > > > > standard boot results in a change that you want to avoid.
> > > > > > >
> > > > > > > changes it will probably boot OS #2.  And if OS #1 sets BootOrder or
> > > > > > > > BootNext that will not change anything.
> > > > > > > >
> > > > > > > > So I think we need a solution where BootNext and BootOrder processing
> > > > > > > > happens early, like we do now.
> > > > > > > >
> > > > > > >
> > > > > > > As of today BootNext does not invoke the boot manager.
> > > > > > >
> > > > > > > If BootOrder is set the boot manager may fail because not all devices are
> > > > > > > detected as "hunters" have not been running. E.g. nvme scan and usb start
> > > > > > > are only invoked after the EFI boot manager.
> > > > > > >
> > > > > > > I originally suggested to probe all boot devices before the boot manager
> > > > > > > runs, but users complained that this slows down their non-EFI boot flows.
> > > > > > > This is why I now move it after all boot methods but PXE.
> > > > > >
> > > > > > Can we not see what BootOrder is and then ensure it's been "hunted" ?
> > > > > >
> > > > > > --
> > > > > > Tom
> > > > > >
> > > > >
> > > > > A load option may only contain the partition GUID and the file path. In
> > > > > this case you wouldn't be able to tell whether it is for an NVMe drive, or
> > > > > a USB stick.
> > > > >
> > > > > As numbering of devices cannot be expected to be stable, it is preferable
> > > > > to use this short form of device paths for load options.
> > > >
> > > > Ah, OK. So could we check and if not found print a human understandable
> > > > error message, and continue on? I want to figure out some path that does
> > > > not change the pre-bootstd behavior.
> > > >
> > >
> > > In distroboot scan_dev_for_efi is invoked for every boot device and each
> > > time calls the boot manager. As usb start is only called in usb_boot this
> > > will result in behavior that is as non-compliant with the UEFI
> > > specification.
> > >
> > > We should reach a behavior that complies with the UEFI specification.
> > > Not changing former distro boot behavior is not a valid option.
> >
> > Can we also not figure out some way to boot promptly? I was thinking
> > something along the lines of if we know BootOrder (or similar), we try
> > it. If we don't find it, we try again a bit later on when we can run all
> > the hunting that might be slow. If the only option is slow-but-compliant
> > I feel like a lot of use cases will opt out of using EFI boot instead.

BTW this is what I have been proposing for a year or so. But my
understanding was that you were happy enough with Heinrich's
workaround for now.

>
> I think that is possible.  But it will require tighter integration
> integration between the EFI loader and the standard boot device
> hunting code such that devices are probed as we walk to options
> specified by the BootOrder.
>
> A complication here is that a Boot#### option doesn't necessearily
> have the device encoded into the boot option.  An option can reference
> just the GUID of a partition and many OS installers create such
> options.  So if we see such an option we do need to probe devices
> until we find the right partition.

Right. It might be feasible to just use bootstd to produce partitions
one by one, e.g. by calling iter_incr(), then looking at what was
received and seeing if it matches the first bootorder var, booting if
so. That might be enough for a first pass?

Somewhat related, it would be nice if bootmgr could move to using expo
and show its menu while it is still scanning things.

>
> Another complication is that currently the
> efi_bootmgr_update_media_device_boot_option() function will append
> Boot#### options it creates to the BootOrder variable.
>
> This is used to implement the following requirement in the spec:
>
>   The boot manager must also support booting from a short-form device
>   path that starts with the first element being a File Path Media
>   Device Path (File Path Media Device Path ). When the boot manager
>   attempts to boot a short-form File Path Media Device Path, it will
>   enumerate all removable media devices, followed by all fixed media
>   devices, creating boot options for each device.
>
> However, a the spec also says that:
>
>   These new boot options must not be saved to non volatile storage,
>   and may not be added to BootOrder.
>
> But we we do add those options to BootOrder and we I believe we will
> save those new Boot#### options as well.

Regards,
Simon

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 9427 bytes --]

On Tue, May 06, 2025 at 12:24:24PM +0200, Mark Kettenis wrote:
> > Date: Mon, 5 May 2025 17:43:19 -0600
> > From: Tom Rini <trini@konsulko.com>
> > 
> > On Tue, May 06, 2025 at 01:18:16AM +0200, Heinrich Schuchardt wrote:
> > > On 5/6/25 00:10, Tom Rini wrote:
> > > > On Tue, May 06, 2025 at 12:07:20AM +0200, Heinrich Schuchardt wrote:
> > > > > Tom Rini <trini@konsulko.com> schrieb am Mo., 5. Mai 2025, 21:54:
> > > > > 
> > > > > > On Mon, May 05, 2025 at 09:51:52PM +0200, Heinrich Schuchardt wrote:
> > > > > > > Mark Kettenis <mark.kettenis@xs4all.nl> schrieb am Mo., 5. Mai 2025,
> > > > > > 21:03:
> > > > > > > 
> > > > > > > > > Date: Fri, 2 May 2025 18:08:56 +0200
> > > > > > > > > From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > > > > > > 
> > > > > > > > > On 5/2/25 16:49, Simon Glass wrote:
> > > > > > > > > > Hi Heinrich,
> > > > > > > > > > 
> > > > > > > > > > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > > > > > > > > > <heinrich.schuchardt@canonical.com> wrote:
> > > > > > > > > > > 
> > > > > > > > > > > The EFI boot manager bootmeth does not require variable BootOrder
> > > > > > to
> > > > > > > > be
> > > > > > > > > > > preexisting. It creates this variable.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Heinrich Schuchardt <
> > > > > > heinrich.schuchardt@canonical.com
> > > > > > > > > 
> > > > > > > > > > > ---
> > > > > > > > > > >    boot/bootmeth_efi_mgr.c | 21 +++------------------
> > > > > > > > > > >    1 file changed, 3 insertions(+), 18 deletions(-)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> > > > > > > > > > > index 42b8863815e..1669cbed5bd 100644
> > > > > > > > > > > --- a/boot/bootmeth_efi_mgr.c
> > > > > > > > > > > +++ b/boot/bootmeth_efi_mgr.c
> > > > > > > > > > > @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev,
> > > > > > > > struct bootflow_iter *iter)
> > > > > > > > > > > 
> > > > > > > > > > >    static int efi_mgr_read_bootflow(struct udevice *dev, struct
> > > > > > > > bootflow *bflow)
> > > > > > > > > > >    {
> > > > > > > > > > > -       struct efi_mgr_priv *priv = dev_get_priv(dev);
> > > > > > > > > > > -       efi_status_t ret;
> > > > > > > > > > > -       efi_uintn_t size;
> > > > > > > > > > > -       u16 *bootorder;
> > > > > > > > > > > -
> > > > > > > > > > > -       if (priv->fake_dev) {
> > > > > > > > > > > -               bflow->state = BOOTFLOWST_READY;
> > > > > > > > > > > -               return 0;
> > > > > > > > > > > -       }
> > > > > > > > > > > +       int ret
> > > > > > > > > > > 
> > > > > > > > > > >           ret = efi_init_obj_list();
> > > > > > > > > > >           if (ret)
> > > > > > > > > > >                   return log_msg_ret("init", ret);
> > > > > > > > > > > 
> > > > > > > > > > > -       /* Enable this method if the "BootOrder" UEFI exists. */
> > > > > > > > > > > -       bootorder = efi_get_var(u"BootOrder",
> > > > > > > > &efi_global_variable_guid,
> > > > > > > > > > > -                               &size);
> > > > > > > > > > > -       if (bootorder) {
> > > > > > > > > > > -               free(bootorder);
> > > > > > > > > > > -               bflow->state = BOOTFLOWST_READY;
> > > > > > > > > > > -               return 0;
> > > > > > > > > > > -       }
> > > > > > > > > > > +       bflow->state = BOOTFLOWST_READY;
> > > > > > > > > > > 
> > > > > > > > > > > -       return -EINVAL;
> > > > > > > > > > > +       return 0;
> > > > > > > > > > >    }
> > > > > > > > > > > 
> > > > > > > > > > >    static int efi_mgr_read_file(struct udevice *dev, struct
> > > > > > bootflow
> > > > > > > > *bflow,
> > > > > > > > > > > --
> > > > > > > > > > > 2.48.1
> > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > How do we know if the board is using EFI bootmgr? My understanding
> > > > > > was
> > > > > > > > > > that this was a way to find out?
> > > > > > > > > 
> > > > > > > > > The boot manager must always run.
> > > > > > > > > 
> > > > > > > > > The check for the BootOrder variable introduced in commit
> > > > > > f2bfa0cb1794
> > > > > > > > > is a bug.
> > > > > > > > 
> > > > > > > > Well, at the time the boot manager did not attempt to boot the default
> > > > > > > > path.  So there was no point in running the boot manager code unless
> > > > > > > > BootOrder (or BootNext) was set.  And of course before that commit the
> > > > > > > > boot manager didn't run at all on non-sandbox builds that had the
> > > > > > > > standard boot stuff enebaled.
> > > > > > > > 
> > > > > > > > Anyway, I believe the thinking behind that commit is still sound.  As
> > > > > > > > I explained earlier, I think that...
> > > > > > > > 
> > > > > > > > > The boot manager handles in sequence:
> > > > > > > > > 
> > > > > > > > > * Try to boot as indicated by BootNext.
> > > > > > > > > * Try to boot as indicated by BootOrder.
> > > > > > > > > * Try to boot default path for available media.
> > > > > > > > >     This will add Boot#### entries and update BootOrder.
> > > > > > > > 
> > > > > > > > ...doing this all in a monolithic sequence isn't the best way to
> > > > > > > > handle EFI boot in the u-boot ecosystem.
> > > > > > > > 
> > > > > > > > Your series moves the boot manager further down the list because the
> > > > > > > > third step in the sequence has to happen late.  But as a result
> > > > > > > > BootNext and BootOrder processing will also happen late.  So what
> > > > > > > > happens if you have a board with two OS installations:
> > > > > > > > 
> > > > > > > > 1. A generic Linux distro that boots via EFI.
> > > > > > > > 
> > > > > > > > 2. Something like Armbian that provides an extlinux.conf file.
> > > > > > > > 
> > > > > > > > Currently such a system will probably boot OS #1.  But after your
> > > > > > > > 
> > > > > > > 
> > > > > > > This did not happen with distoboot. So migration from distroboot to
> > > > > > > standard boot results in a change that you want to avoid.
> > > > > > > 
> > > > > > > changes it will probably boot OS #2.  And if OS #1 sets BootOrder or
> > > > > > > > BootNext that will not change anything.
> > > > > > > > 
> > > > > > > > So I think we need a solution where BootNext and BootOrder processing
> > > > > > > > happens early, like we do now.
> > > > > > > > 
> > > > > > > 
> > > > > > > As of today BootNext does not invoke the boot manager.
> > > > > > > 
> > > > > > > If BootOrder is set the boot manager may fail because not all devices are
> > > > > > > detected as "hunters" have not been running. E.g. nvme scan and usb start
> > > > > > > are only invoked after the EFI boot manager.
> > > > > > > 
> > > > > > > I originally suggested to probe all boot devices before the boot manager
> > > > > > > runs, but users complained that this slows down their non-EFI boot flows.
> > > > > > > This is why I now move it after all boot methods but PXE.
> > > > > > 
> > > > > > Can we not see what BootOrder is and then ensure it's been "hunted" ?
> > > > > > 
> > > > > > --
> > > > > > Tom
> > > > > > 
> > > > > 
> > > > > A load option may only contain the partition GUID and the file path. In
> > > > > this case you wouldn't be able to tell whether it is for an NVMe drive, or
> > > > > a USB stick.
> > > > > 
> > > > > As numbering of devices cannot be expected to be stable, it is preferable
> > > > > to use this short form of device paths for load options.
> > > > 
> > > > Ah, OK. So could we check and if not found print a human understandable
> > > > error message, and continue on? I want to figure out some path that does
> > > > not change the pre-bootstd behavior.
> > > > 
> > > 
> > > In distroboot scan_dev_for_efi is invoked for every boot device and each
> > > time calls the boot manager. As usb start is only called in usb_boot this
> > > will result in behavior that is as non-compliant with the UEFI
> > > specification.
> > > 
> > > We should reach a behavior that complies with the UEFI specification.
> > > Not changing former distro boot behavior is not a valid option.
> > 
> > Can we also not figure out some way to boot promptly? I was thinking
> > something along the lines of if we know BootOrder (or similar), we try
> > it. If we don't find it, we try again a bit later on when we can run all
> > the hunting that might be slow. If the only option is slow-but-compliant
> > I feel like a lot of use cases will opt out of using EFI boot instead.
> 
> I think that is possible.  But it will require tighter integration
> integration between the EFI loader and the standard boot device
> hunting code such that devices are probed as we walk to options
> specified by the BootOrder.
> 
> A complication here is that a Boot#### option doesn't necessearily
> have the device encoded into the boot option.  An option can reference
> just the GUID of a partition and many OS installers create such
> options.  So if we see such an option we do need to probe devices
> until we find the right partition.

Right, I recall Heinrich mentioned it could (might often be?) a GUID
instead and so we might need to hunt, check what we found for a GUID,
repeat. I am assuming that would not be an expensive operation however.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [RFC 1/8] boot: EFI boot manager does not depend on BootOrder

[Ilias Apalodimas]

On Tue, 6 May 2025 at 17:29, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, May 06, 2025 at 12:24:24PM +0200, Mark Kettenis wrote:
> > > Date: Mon, 5 May 2025 17:43:19 -0600
> > > From: Tom Rini <trini@konsulko.com>
> > >
> > > On Tue, May 06, 2025 at 01:18:16AM +0200, Heinrich Schuchardt wrote:
> > > > On 5/6/25 00:10, Tom Rini wrote:
> > > > > On Tue, May 06, 2025 at 12:07:20AM +0200, Heinrich Schuchardt wrote:
> > > > > > Tom Rini <trini@konsulko.com> schrieb am Mo., 5. Mai 2025, 21:54:
> > > > > >
> > > > > > > On Mon, May 05, 2025 at 09:51:52PM +0200, Heinrich Schuchardt wrote:
> > > > > > > > Mark Kettenis <mark.kettenis@xs4all.nl> schrieb am Mo., 5. Mai 2025,
> > > > > > > 21:03:
> > > > > > > >
> > > > > > > > > > Date: Fri, 2 May 2025 18:08:56 +0200
> > > > > > > > > > From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > > > > > > >
> > > > > > > > > > On 5/2/25 16:49, Simon Glass wrote:
> > > > > > > > > > > Hi Heinrich,
> > > > > > > > > > >
> > > > > > > > > > > On Mon, 21 Apr 2025 at 10:26, Heinrich Schuchardt
> > > > > > > > > > > <heinrich.schuchardt@canonical.com> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > The EFI boot manager bootmeth does not require variable BootOrder
> > > > > > > to
> > > > > > > > > be
> > > > > > > > > > > > preexisting. It creates this variable.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Heinrich Schuchardt <
> > > > > > > heinrich.schuchardt@canonical.com
> > > > > > > > > >
> > > > > > > > > > > > ---
> > > > > > > > > > > >    boot/bootmeth_efi_mgr.c | 21 +++------------------
> > > > > > > > > > > >    1 file changed, 3 insertions(+), 18 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> > > > > > > > > > > > index 42b8863815e..1669cbed5bd 100644
> > > > > > > > > > > > --- a/boot/bootmeth_efi_mgr.c
> > > > > > > > > > > > +++ b/boot/bootmeth_efi_mgr.c
> > > > > > > > > > > > @@ -47,30 +47,15 @@ static int efi_mgr_check(struct udevice *dev,
> > > > > > > > > struct bootflow_iter *iter)
> > > > > > > > > > > >
> > > > > > > > > > > >    static int efi_mgr_read_bootflow(struct udevice *dev, struct
> > > > > > > > > bootflow *bflow)
> > > > > > > > > > > >    {
> > > > > > > > > > > > -       struct efi_mgr_priv *priv = dev_get_priv(dev);
> > > > > > > > > > > > -       efi_status_t ret;
> > > > > > > > > > > > -       efi_uintn_t size;
> > > > > > > > > > > > -       u16 *bootorder;
> > > > > > > > > > > > -
> > > > > > > > > > > > -       if (priv->fake_dev) {
> > > > > > > > > > > > -               bflow->state = BOOTFLOWST_READY;
> > > > > > > > > > > > -               return 0;
> > > > > > > > > > > > -       }
> > > > > > > > > > > > +       int ret
> > > > > > > > > > > >
> > > > > > > > > > > >           ret = efi_init_obj_list();
> > > > > > > > > > > >           if (ret)
> > > > > > > > > > > >                   return log_msg_ret("init", ret);
> > > > > > > > > > > >
> > > > > > > > > > > > -       /* Enable this method if the "BootOrder" UEFI exists. */
> > > > > > > > > > > > -       bootorder = efi_get_var(u"BootOrder",
> > > > > > > > > &efi_global_variable_guid,
> > > > > > > > > > > > -                               &size);
> > > > > > > > > > > > -       if (bootorder) {
> > > > > > > > > > > > -               free(bootorder);
> > > > > > > > > > > > -               bflow->state = BOOTFLOWST_READY;
> > > > > > > > > > > > -               return 0;
> > > > > > > > > > > > -       }
> > > > > > > > > > > > +       bflow->state = BOOTFLOWST_READY;
> > > > > > > > > > > >
> > > > > > > > > > > > -       return -EINVAL;
> > > > > > > > > > > > +       return 0;
> > > > > > > > > > > >    }
> > > > > > > > > > > >
> > > > > > > > > > > >    static int efi_mgr_read_file(struct udevice *dev, struct
> > > > > > > bootflow
> > > > > > > > > *bflow,
> > > > > > > > > > > > --
> > > > > > > > > > > > 2.48.1
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > How do we know if the board is using EFI bootmgr? My understanding
> > > > > > > was
> > > > > > > > > > > that this was a way to find out?
> > > > > > > > > >
> > > > > > > > > > The boot manager must always run.
> > > > > > > > > >
> > > > > > > > > > The check for the BootOrder variable introduced in commit
> > > > > > > f2bfa0cb1794
> > > > > > > > > > is a bug.
> > > > > > > > >
> > > > > > > > > Well, at the time the boot manager did not attempt to boot the default
> > > > > > > > > path.  So there was no point in running the boot manager code unless
> > > > > > > > > BootOrder (or BootNext) was set.  And of course before that commit the
> > > > > > > > > boot manager didn't run at all on non-sandbox builds that had the
> > > > > > > > > standard boot stuff enebaled.
> > > > > > > > >
> > > > > > > > > Anyway, I believe the thinking behind that commit is still sound.  As
> > > > > > > > > I explained earlier, I think that...
> > > > > > > > >
> > > > > > > > > > The boot manager handles in sequence:
> > > > > > > > > >
> > > > > > > > > > * Try to boot as indicated by BootNext.
> > > > > > > > > > * Try to boot as indicated by BootOrder.
> > > > > > > > > > * Try to boot default path for available media.
> > > > > > > > > >     This will add Boot#### entries and update BootOrder.
> > > > > > > > >
> > > > > > > > > ...doing this all in a monolithic sequence isn't the best way to
> > > > > > > > > handle EFI boot in the u-boot ecosystem.
> > > > > > > > >
> > > > > > > > > Your series moves the boot manager further down the list because the
> > > > > > > > > third step in the sequence has to happen late.  But as a result
> > > > > > > > > BootNext and BootOrder processing will also happen late.  So what
> > > > > > > > > happens if you have a board with two OS installations:
> > > > > > > > >
> > > > > > > > > 1. A generic Linux distro that boots via EFI.
> > > > > > > > >
> > > > > > > > > 2. Something like Armbian that provides an extlinux.conf file.
> > > > > > > > >
> > > > > > > > > Currently such a system will probably boot OS #1.  But after your
> > > > > > > > >
> > > > > > > >
> > > > > > > > This did not happen with distoboot. So migration from distroboot to
> > > > > > > > standard boot results in a change that you want to avoid.
> > > > > > > >
> > > > > > > > changes it will probably boot OS #2.  And if OS #1 sets BootOrder or
> > > > > > > > > BootNext that will not change anything.
> > > > > > > > >
> > > > > > > > > So I think we need a solution where BootNext and BootOrder processing
> > > > > > > > > happens early, like we do now.
> > > > > > > > >
> > > > > > > >
> > > > > > > > As of today BootNext does not invoke the boot manager.
> > > > > > > >
> > > > > > > > If BootOrder is set the boot manager may fail because not all devices are
> > > > > > > > detected as "hunters" have not been running. E.g. nvme scan and usb start
> > > > > > > > are only invoked after the EFI boot manager.
> > > > > > > >
> > > > > > > > I originally suggested to probe all boot devices before the boot manager
> > > > > > > > runs, but users complained that this slows down their non-EFI boot flows.
> > > > > > > > This is why I now move it after all boot methods but PXE.
> > > > > > >
> > > > > > > Can we not see what BootOrder is and then ensure it's been "hunted" ?
> > > > > > >
> > > > > > > --
> > > > > > > Tom
> > > > > > >
> > > > > >
> > > > > > A load option may only contain the partition GUID and the file path. In
> > > > > > this case you wouldn't be able to tell whether it is for an NVMe drive, or
> > > > > > a USB stick.
> > > > > >
> > > > > > As numbering of devices cannot be expected to be stable, it is preferable
> > > > > > to use this short form of device paths for load options.
> > > > >
> > > > > Ah, OK. So could we check and if not found print a human understandable
> > > > > error message, and continue on? I want to figure out some path that does
> > > > > not change the pre-bootstd behavior.
> > > > >
> > > >
> > > > In distroboot scan_dev_for_efi is invoked for every boot device and each
> > > > time calls the boot manager. As usb start is only called in usb_boot this
> > > > will result in behavior that is as non-compliant with the UEFI
> > > > specification.
> > > >
> > > > We should reach a behavior that complies with the UEFI specification.
> > > > Not changing former distro boot behavior is not a valid option.
> > >
> > > Can we also not figure out some way to boot promptly? I was thinking
> > > something along the lines of if we know BootOrder (or similar), we try
> > > it. If we don't find it, we try again a bit later on when we can run all
> > > the hunting that might be slow. If the only option is slow-but-compliant
> > > I feel like a lot of use cases will opt out of using EFI boot instead.
> >
> > I think that is possible.  But it will require tighter integration
> > integration between the EFI loader and the standard boot device
> > hunting code such that devices are probed as we walk to options
> > specified by the BootOrder.
> >
> > A complication here is that a Boot#### option doesn't necessearily
> > have the device encoded into the boot option.  An option can reference
> > just the GUID of a partition and many OS installers create such
> > options.  So if we see such an option we do need to probe devices
> > until we find the right partition.
>
> Right, I recall Heinrich mentioned it could (might often be?) a GUID
> instead and so we might need to hunt, check what we found for a GUID,
> repeat. I am assuming that would not be an expensive operation however.

Yes and that's the main problem here.
In EFI you have long and short options. The long one is the full
device path, which we can easily map to a device. The short is a bit
trickier as Heinrich described -- but because it's more flexible
that's what people tend to use.

Thanks
/Ilias

>
> --
> Tom