Re: [PATCH v2 1/4] boot: correct the default sequence of boot methods
On 04.04.24 17:18, Mark Kettenis wrote:
From: Heinrich Schuchardt
Date: Thu, 4 Apr 2024 13:47:39 +0200
The default sequence of boot methods is determined by alphabetical sorting
during linkage.
* efi_mgr must run before efi to be UEFI compliant
* pxe should run as last resort
Signed-off-by: Heinrich Schuchardt
This sound very much like you're working around a standard boot design
flaw.
Who does the alphabetical sorting here? Is that just a result of how
the toolchain behaves? If so, that sounds fragile to me and I think
it would be better to make the order explicit in the code.
Thanks for reviewing.
Please, observe the SORT command in our linker script lines
KEEP(*(SORT(__u_boot_list*)));
This ensures that linker generated lists are always alphabetically
sorted. We rely on this in other places too. See
part_driver_lookup_type() where GPT must be tested before FAT (commit
96e5b03c8ab7 ("dm: part: Convert partition API use to linker lists").
Best regards
Heinrich
Cheers,
Mark
---
v2:
no change
---
boot/bootmeth_efi.c | 2 +-
boot/bootmeth_efi_mgr.c | 2 +-
boot/bootmeth_pxe.c | 2 +-
test/boot/bootflow.c| 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
index c4eb331d69e..a46b6c9c805 100644
--- a/boot/bootmeth_efi.c
+++ b/boot/bootmeth_efi.c
@@ -489,7 +489,7 @@ static const struct udevice_id distro_efi_bootmeth_ids[] = {
{ }
};
-U_BOOT_DRIVER(bootmeth_efi) = {
+U_BOOT_DRIVER(bootmeth_4efi) = {
.name = "bootmeth_efi",
.id = UCLASS_BOOTMETH,
.of_match = distro_efi_bootmeth_ids,
diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
index ed29d7ef021..b7d429f2c3d 100644
--- a/boot/bootmeth_efi_mgr.c
+++ b/boot/bootmeth_efi_mgr.c
@@ -114,7 +114,7 @@ static const struct udevice_id efi_mgr_bootmeth_ids[] = {
{ }
};
-U_BOOT_DRIVER(bootmeth_efi_mgr) = {
+U_BOOT_DRIVER(bootmeth_3efi_mgr) = {
.name = "bootmeth_efi_mgr",
.id = UCLASS_BOOTMETH,
.of_match = efi_mgr_bootmeth_ids,
diff --git a/boot/bootmeth_pxe.c b/boot/bootmeth_pxe.c
index 8d489a11aa4..70f693aa239 100644
--- a/boot/bootmeth_pxe.c
+++ b/boot/bootmeth_pxe.c
@@ -184,7 +184,7 @@ static const struct udevice_id extlinux_bootmeth_pxe_ids[]
= {
{ }
};
-U_BOOT_DRIVER(bootmeth_pxe) = {
+U_BOOT_DRIVER(bootmeth_zpxe) = {
.name = "bootmeth_pxe",
.id = UCLASS_BOOTMETH,
.of_match = extlinux_bootmeth_pxe_ids,
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index 4845b7121c8..e60e9309fa9 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -377,7 +377,7 @@ static int bootflow_system(struct unit_test_state *uts)
if (!IS_ENABLED(CONFIG_EFI_BOOTMGR))
return -EAGAIN;
ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));
- ut_assertok(device_bind(bootstd, DM_DRIVER_GET(bootmeth_efi_mgr),
+ ut_assertok(device_bind(bootstd, DM_DRIVER_GET(bootmeth_3efi_mgr),
"efi_mgr", 0, ofnode_null(), &dev));
ut_assertok(device_probe(dev));
sandbox_set_fake_efi_mgr_dev(dev, true);
--
2.43.0
Re: [PATCH v2 1/4] boot: correct the default sequence of boot methods
> From: Heinrich Schuchardt
> Date: Thu, 4 Apr 2024 13:47:39 +0200
>
> The default sequence of boot methods is determined by alphabetical sorting
> during linkage.
>
> * efi_mgr must run before efi to be UEFI compliant
> * pxe should run as last resort
>
> Signed-off-by: Heinrich Schuchardt
This sound very much like you're working around a standard boot design
flaw.
Who does the alphabetical sorting here? Is that just a result of how
the toolchain behaves? If so, that sounds fragile to me and I think
it would be better to make the order explicit in the code.
Cheers,
Mark
> ---
> v2:
> no change
> ---
> boot/bootmeth_efi.c | 2 +-
> boot/bootmeth_efi_mgr.c | 2 +-
> boot/bootmeth_pxe.c | 2 +-
> test/boot/bootflow.c| 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
> index c4eb331d69e..a46b6c9c805 100644
> --- a/boot/bootmeth_efi.c
> +++ b/boot/bootmeth_efi.c
> @@ -489,7 +489,7 @@ static const struct udevice_id distro_efi_bootmeth_ids[]
> = {
> { }
> };
>
> -U_BOOT_DRIVER(bootmeth_efi) = {
> +U_BOOT_DRIVER(bootmeth_4efi) = {
> .name = "bootmeth_efi",
> .id = UCLASS_BOOTMETH,
> .of_match = distro_efi_bootmeth_ids,
> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> index ed29d7ef021..b7d429f2c3d 100644
> --- a/boot/bootmeth_efi_mgr.c
> +++ b/boot/bootmeth_efi_mgr.c
> @@ -114,7 +114,7 @@ static const struct udevice_id efi_mgr_bootmeth_ids[] = {
> { }
> };
>
> -U_BOOT_DRIVER(bootmeth_efi_mgr) = {
> +U_BOOT_DRIVER(bootmeth_3efi_mgr) = {
> .name = "bootmeth_efi_mgr",
> .id = UCLASS_BOOTMETH,
> .of_match = efi_mgr_bootmeth_ids,
> diff --git a/boot/bootmeth_pxe.c b/boot/bootmeth_pxe.c
> index 8d489a11aa4..70f693aa239 100644
> --- a/boot/bootmeth_pxe.c
> +++ b/boot/bootmeth_pxe.c
> @@ -184,7 +184,7 @@ static const struct udevice_id
> extlinux_bootmeth_pxe_ids[] = {
> { }
> };
>
> -U_BOOT_DRIVER(bootmeth_pxe) = {
> +U_BOOT_DRIVER(bootmeth_zpxe) = {
> .name = "bootmeth_pxe",
> .id = UCLASS_BOOTMETH,
> .of_match = extlinux_bootmeth_pxe_ids,
> diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
> index 4845b7121c8..e60e9309fa9 100644
> --- a/test/boot/bootflow.c
> +++ b/test/boot/bootflow.c
> @@ -377,7 +377,7 @@ static int bootflow_system(struct unit_test_state *uts)
> if (!IS_ENABLED(CONFIG_EFI_BOOTMGR))
> return -EAGAIN;
> ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));
> - ut_assertok(device_bind(bootstd, DM_DRIVER_GET(bootmeth_efi_mgr),
> + ut_assertok(device_bind(bootstd, DM_DRIVER_GET(bootmeth_3efi_mgr),
> "efi_mgr", 0, ofnode_null(), &dev));
> ut_assertok(device_probe(dev));
> sandbox_set_fake_efi_mgr_dev(dev, true);
> --
> 2.43.0
>
>
Re: [PATCH v2 1/4] boot: correct the default sequence of boot methods
On Thu, 4 Apr 2024 at 14:49, Heinrich Schuchardt
wrote:
>
> The default sequence of boot methods is determined by alphabetical sorting
> during linkage.
>
> * efi_mgr must run before efi to be UEFI compliant
> * pxe should run as last resort
>
> Signed-off-by: Heinrich Schuchardt
> ---
> v2:
> no change
> ---
> boot/bootmeth_efi.c | 2 +-
> boot/bootmeth_efi_mgr.c | 2 +-
> boot/bootmeth_pxe.c | 2 +-
> test/boot/bootflow.c| 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
> index c4eb331d69e..a46b6c9c805 100644
> --- a/boot/bootmeth_efi.c
> +++ b/boot/bootmeth_efi.c
> @@ -489,7 +489,7 @@ static const struct udevice_id distro_efi_bootmeth_ids[]
> = {
> { }
> };
>
> -U_BOOT_DRIVER(bootmeth_efi) = {
> +U_BOOT_DRIVER(bootmeth_4efi) = {
> .name = "bootmeth_efi",
> .id = UCLASS_BOOTMETH,
> .of_match = distro_efi_bootmeth_ids,
> diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> index ed29d7ef021..b7d429f2c3d 100644
> --- a/boot/bootmeth_efi_mgr.c
> +++ b/boot/bootmeth_efi_mgr.c
> @@ -114,7 +114,7 @@ static const struct udevice_id efi_mgr_bootmeth_ids[] = {
> { }
> };
>
> -U_BOOT_DRIVER(bootmeth_efi_mgr) = {
> +U_BOOT_DRIVER(bootmeth_3efi_mgr) = {
> .name = "bootmeth_efi_mgr",
> .id = UCLASS_BOOTMETH,
> .of_match = efi_mgr_bootmeth_ids,
> diff --git a/boot/bootmeth_pxe.c b/boot/bootmeth_pxe.c
> index 8d489a11aa4..70f693aa239 100644
> --- a/boot/bootmeth_pxe.c
> +++ b/boot/bootmeth_pxe.c
> @@ -184,7 +184,7 @@ static const struct udevice_id
> extlinux_bootmeth_pxe_ids[] = {
> { }
> };
>
> -U_BOOT_DRIVER(bootmeth_pxe) = {
> +U_BOOT_DRIVER(bootmeth_zpxe) = {
> .name = "bootmeth_pxe",
> .id = UCLASS_BOOTMETH,
> .of_match = extlinux_bootmeth_pxe_ids,
> diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
> index 4845b7121c8..e60e9309fa9 100644
> --- a/test/boot/bootflow.c
> +++ b/test/boot/bootflow.c
> @@ -377,7 +377,7 @@ static int bootflow_system(struct unit_test_state *uts)
> if (!IS_ENABLED(CONFIG_EFI_BOOTMGR))
> return -EAGAIN;
> ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));
> - ut_assertok(device_bind(bootstd, DM_DRIVER_GET(bootmeth_efi_mgr),
> + ut_assertok(device_bind(bootstd, DM_DRIVER_GET(bootmeth_3efi_mgr),
> "efi_mgr", 0, ofnode_null(), &dev));
> ut_assertok(device_probe(dev));
> sandbox_set_fake_efi_mgr_dev(dev, true);
> --
> 2.43.0
>
Reviewed-by: Ilias Apalodimas
