On 26.04.24 16:52, Caleb Connolly wrote:
Hi Heinrich,
On 26/04/2024 16:13, Heinrich Schuchardt wrote:
Move distro_efi_get_fdt_name() to a separate C module and rename it to efi_get_distro_fdt_name().
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
boot/bootmeth_efi.c | 60 ++------------------------------- include/efi_loader.h | 2 ++ lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_fdt.c | 73 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 58 deletions(-) create mode 100644 lib/efi_loader/efi_fdt.c
diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index aebc5207fc0..40da77c497b 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -144,62 +144,6 @@ static int distro_efi_check(struct udevice *dev, struct bootflow_iter *iter) return 0; } -/**
- distro_efi_get_fdt_name() - Get the filename for reading the .dtb
file
- @fname: Place to put filename
- @size: Max size of filename
- @seq: Sequence number, to cycle through options (0=first)
- Returns: 0 on success, -ENOENT if the "fdtfile" env var does not
exist,
- -EINVAL if there are no more options, -EALREADY if the control FDT
should be
- used
- */
-static int distro_efi_get_fdt_name(char *fname, int size, int seq) -{ - const char *fdt_fname; - const char *prefix;
- /* select the prefix */ - switch (seq) { - case 0: - /* this is the default */ - prefix = "/dtb"; - break; - case 1: - prefix = ""; - break; - case 2: - prefix = "/dtb/current"; - break;
This is a bit of a tangential point (and shouldn't block this series at all). But where do we find a balance with this search algorithm? The distro I work on (postmarketOS) actually installs DTB files to "/dtbs" (not "/dtb").
No doubt we can come up with a bunch of clever ideas for optimising this with wildcards or whatever, but is that something we want to implement?
What about distros that support falling back to previous kernel versions (and consequently have the DTB dir named after the kernel version).
Presumably in these situations the distro would have systemd-boot, GRUB, etc. Would it make sense to just inform the bootloader what the .dtb file name is so that they can handle the path building?
(I understand there
We have been carrying the logic for scanning for DTBs in U-Boot since commit Fixes: 74522c898b35 ("efi_loader: Add distro boot script for removable media") merged in 2016.
The script based version allowed to modify variable efi_dtb_prefixes to scan other directories.
Now Simon has started with the 2024.01 release to move the logic from scripts in header files with script fragments to C files in /boot/.
This series is cleaning up the changes for bootmethod_efi_mgr.c.
Debian's flash-kernel package installs device-trees in dtb. I do not know if there are users for '/dtb/current/' or '/'. The more directories we scan the slower booting.
This specific patch does not change the current logic. If we need the flexibility back, that was provided by environment variable efi_dtb_prefixes we should look at it in a future change.
Best regards
Heinrich
are other usecases and reasons to load the DTB ahead
of time in U-Boot).
- default: - return log_msg_ret("pref", -EINVAL); - }
- fdt_fname = env_get("fdtfile"); - if (fdt_fname) { - snprintf(fname, size, "%s/%s", prefix, fdt_fname); - log_debug("Using device tree: %s\n", fname); - } else if (IS_ENABLED(CONFIG_OF_HAS_PRIOR_STAGE)) { - strcpy(fname, "<prior>"); - return log_msg_ret("pref", -EALREADY); - /* Use this fallback only for 32-bit ARM */ - } else if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64)) { - const char *soc = env_get("soc"); - const char *board = env_get("board"); - const char *boardver = env_get("boardver");
- /* cf the code in label_boot() which seems very complex */ - snprintf(fname, size, "%s/%s%s%s%s.dtb", prefix, - soc ? soc : "", soc ? "-" : "", board ? board : "", - boardver ? boardver : ""); - log_debug("Using default device tree: %s\n", fname); - } else { - return log_msg_ret("env", -ENOENT); - }
- return 0; -}
/* * distro_efi_try_bootflow_files() - Check that files are present * @@ -241,7 +185,7 @@ static int distro_efi_try_bootflow_files(struct udevice *dev, ret = -ENOENT; *fname = '\0'; for (seq = 0; ret == -ENOENT; seq++) { - ret = distro_efi_get_fdt_name(fname, sizeof(fname), seq); + ret = efi_get_distro_fdt_name(fname, sizeof(fname), seq); if (ret == -EALREADY) bflow->flags = BOOTFLOWF_USE_PRIOR_FDT; if (!ret) { @@ -340,7 +284,7 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) sprintf(file_addr, "%lx", fdt_addr); /* We only allow the first prefix with PXE */ - ret = distro_efi_get_fdt_name(fname, sizeof(fname), 0); + ret = efi_get_distro_fdt_name(fname, sizeof(fname), 0); if (ret) return log_msg_ret("nam", ret); diff --git a/include/efi_loader.h b/include/efi_loader.h index 0ac04990b8e..ed2b517b130 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -1187,4 +1187,6 @@ efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int */ void efi_add_known_memory(void); +int efi_get_distro_fdt_name(char *fname, int size, int seq);
#endif /* _EFI_LOADER_H */ diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index 034e366967f..2af6f2066b5 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -59,6 +59,7 @@ obj-y += efi_device_path.o obj-$(CONFIG_EFI_DEVICE_PATH_TO_TEXT) += efi_device_path_to_text.o obj-$(CONFIG_EFI_DEVICE_PATH_UTIL) += efi_device_path_utilities.o obj-y += efi_dt_fixup.o +obj-y += efi_fdt.o obj-y += efi_file.o obj-$(CONFIG_EFI_LOADER_HII) += efi_hii.o obj-y += efi_image_loader.o diff --git a/lib/efi_loader/efi_fdt.c b/lib/efi_loader/efi_fdt.c new file mode 100644 index 00000000000..0edf0c1e2fc --- /dev/null +++ b/lib/efi_loader/efi_fdt.c @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Bootmethod for distro boot via EFI
- Copyright 2021 Google LLC
- Written by Simon Glass sjg@chromium.org
- */
+#include <efi_loader.h> +#include <env.h> +#include <errno.h> +#include <log.h> +#include <string.h> +#include <vsprintf.h>
+/**
- distro_efi_get_fdt_name() - get the filename for reading the .dtb
file
- @fname: buffer for filename
- @size: buffer size
- @seq: sequence number, to cycle through options (0=first)
- Returns:
- 0 on success,
- -ENOENT if the "fdtfile" env var does not exist,
- -EINVAL if there are no more options,
- -EALREADY if the control FDT should be used
- */
+int efi_get_distro_fdt_name(char *fname, int size, int seq) +{ + const char *fdt_fname; + const char *prefix;
+ /* select the prefix */ + switch (seq) { + case 0: + /* this is the default */ + prefix = "/dtb"; + break; + case 1: + prefix = ""; + break; + case 2: + prefix = "/dtb/current"; + break; + default: + return log_msg_ret("pref", -EINVAL); + }
+ fdt_fname = env_get("fdtfile"); + if (fdt_fname) { + snprintf(fname, size, "%s/%s", prefix, fdt_fname); + log_debug("Using device tree: %s\n", fname); + } else if (IS_ENABLED(CONFIG_OF_HAS_PRIOR_STAGE)) { + strcpy(fname, "<prior>"); + return log_msg_ret("pref", -EALREADY); + /* Use this fallback only for 32-bit ARM */ + } else if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64)) { + const char *soc = env_get("soc"); + const char *board = env_get("board"); + const char *boardver = env_get("boardver");
+ /* cf the code in label_boot() which seems very complex */ + snprintf(fname, size, "%s/%s%s%s%s.dtb", prefix, + soc ? soc : "", soc ? "-" : "", board ? board : "", + boardver ? boardver : ""); + log_debug("Using default device tree: %s\n", fname); + } else { + return log_msg_ret("env", -ENOENT); + }
+ return 0; +}