[PATCH v2 0/8] efi_loader: improve device-tree loading

In U-Boot EFI boot options can already specify both an EFI binary and an initrd. With this series we can additionally define the matching device-tree to be loaded in the boot option.
With the last patch the boot manager will fall back the device-tree specified by $fdtfile in directories '/dtb/', '/', or '/dtb/current/' on the boot device if no device-tree is specified in the boot option or via a bootefi command parameter.
v2: Update efi_dp_concat() instead of new function efi_dp_merge(). Carve out a function efi_load_option_dp_join() which we can use both for the eficonfig and the efidebug command. Rename variables id_dp, final_dp_size. Rename create_initrd_dp() to create_lo_dp_part(). Use enum as parameter for create_lo_dp_part(). Put all related changes into one patch.
Heinrich Schuchardt (8): efi_loader: allow concatenation with contained end node cmd: eficonfig: add support for setting fdt cmd: efidebug: add support for setting fdt efi_loader: load device-tree specified in boot option efi_loader: move distro_efi_get_fdt_name() efi_loader: return binary from efi_dp_from_lo() efi_loader: export efi_load_image_from_path efi_loader: load distro dtb in bootmgr
boot/bootmeth_efi.c | 60 +--------- cmd/eficonfig.c | 83 +++++++++---- cmd/efidebug.c | 130 +++++++++++++++------ include/efi_loader.h | 24 +++- lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_bootbin.c | 2 +- lib/efi_loader/efi_bootmgr.c | 75 +++++++++++- lib/efi_loader/efi_boottime.c | 3 +- lib/efi_loader/efi_device_path.c | 40 ++++--- lib/efi_loader/efi_device_path_utilities.c | 2 +- lib/efi_loader/efi_fdt.c | 117 +++++++++++++++++++ lib/efi_loader/efi_helper.c | 44 +++++++ 12 files changed, 445 insertions(+), 136 deletions(-) create mode 100644 lib/efi_loader/efi_fdt.c

Allow appending a device-path to a device-path that contains an end node as separator. We need this feature for creating boot options specifying kernel, initrd, and dtb.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: update efi_dp_concat() instead of new function efi_dp_merge() --- cmd/eficonfig.c | 6 +++--- cmd/efidebug.c | 4 ++-- include/efi_loader.h | 2 +- lib/efi_loader/efi_bootbin.c | 2 +- lib/efi_loader/efi_bootmgr.c | 2 +- lib/efi_loader/efi_boottime.c | 2 +- lib/efi_loader/efi_device_path.c | 20 +++++++++++++------- lib/efi_loader/efi_device_path_utilities.c | 2 +- 8 files changed, 23 insertions(+), 17 deletions(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 0ba92c60e03..b13d9a3d2d9 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -531,7 +531,7 @@ struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_ dp = efi_dp_shorten(dp_volume); if (!dp) dp = dp_volume; - dp = efi_dp_concat(dp, &fp->dp, false); + dp = efi_dp_concat(dp, &fp->dp, 0); free(buf);
return dp; @@ -1485,7 +1485,7 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo goto out; } initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp, - dp, false); + dp, 0); efi_free_pool(dp); }
@@ -1496,7 +1496,7 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo } final_dp_size = efi_dp_size(dp) + sizeof(END); if (initrd_dp) { - final_dp = efi_dp_concat(dp, initrd_dp, true); + final_dp = efi_dp_concat(dp, initrd_dp, 1); final_dp_size += efi_dp_size(initrd_dp) + sizeof(END); } else { final_dp = efi_dp_dup(dp); diff --git a/cmd/efidebug.c b/cmd/efidebug.c index c2c525f2351..762027daf8a 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -697,7 +697,7 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part, short_fp = tmp_fp;
initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp, - short_fp, false); + short_fp, 0);
out: efi_free_pool(tmp_dp); @@ -917,7 +917,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, goto out; }
- final_fp = efi_dp_concat(file_path, initrd_dp, true); + final_fp = efi_dp_concat(file_path, initrd_dp, 1); if (!final_fp) { printf("Cannot create final device path\n"); r = CMD_RET_FAILURE; diff --git a/include/efi_loader.h b/include/efi_loader.h index 9600941aa32..ddf2e41a95c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -946,7 +946,7 @@ struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo, const efi_guid_t *guid); struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, const struct efi_device_path *dp2, - bool split_end_node); + size_t split_end_node); struct efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path); efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data, efi_uintn_t *size); diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index b7910f78fb6..a87006b3c0e 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -150,7 +150,7 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) msg_path = file_path; } else { file_path = efi_dp_concat(bootefi_device_path, - bootefi_image_path, false); + bootefi_image_path, 0); msg_path = bootefi_image_path; log_debug("Loaded from disk\n"); } diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 7da3139f917..b0bf21cf841 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -130,7 +130,7 @@ static efi_status_t try_load_from_file_path(efi_handle_t *fs_handles, if (!dp) continue;
- dp = efi_dp_concat(dp, fp, false); + dp = efi_dp_concat(dp, fp, 0); if (!dp) continue;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 1951291747c..630c5f52c4f 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1816,7 +1816,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, if (device_path) { info->device_handle = efi_dp_find_obj(device_path, NULL, NULL);
- dp = efi_dp_concat(device_path, file_path, false); + dp = efi_dp_concat(device_path, file_path, 0); if (!dp) { ret = EFI_OUT_OF_RESOURCES; goto failure; diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index aec224d8466..c8c8d54f733 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -276,10 +276,11 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp) * * @dp1: First device path * @dp2: Second device path - * @split_end_node: If true the two device paths will be concatenated and - * separated by an end node (DEVICE_PATH_SUB_TYPE_END). - * If false the second device path will be concatenated to the - * first one as-is. + * @split_end_node: + * * 0 to concatenate + * * 1 to concatenate with end node added as separator + * * size of dp1 excluding last end node to concatenate with end node as + * separator in case dp1 contains an end node * * Return: * concatenated device path or NULL. Caller must free the returned value @@ -287,7 +288,7 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp) struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, const struct efi_device_path *dp2, - bool split_end_node) + size_t split_end_node) { struct efi_device_path *ret; size_t end_size; @@ -301,10 +302,15 @@ efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, ret = efi_dp_dup(dp1); } else { /* both dp1 and dp2 are non-null */ - unsigned sz1 = efi_dp_size(dp1); - unsigned sz2 = efi_dp_size(dp2); + size_t sz1; + size_t sz2 = efi_dp_size(dp2); void *p;
+ if (split_end_node < sizeof(struct efi_device_path)) + sz1 = efi_dp_size(dp1); + else + sz1 = split_end_node; + if (split_end_node) end_size = 2 * sizeof(END); else diff --git a/lib/efi_loader/efi_device_path_utilities.c b/lib/efi_loader/efi_device_path_utilities.c index c95dbfa9b5f..ac250bbfcc9 100644 --- a/lib/efi_loader/efi_device_path_utilities.c +++ b/lib/efi_loader/efi_device_path_utilities.c @@ -76,7 +76,7 @@ static struct efi_device_path * EFIAPI append_device_path( const struct efi_device_path *src2) { EFI_ENTRY("%pD, %pD", src1, src2); - return EFI_EXIT(efi_dp_concat(src1, src2, false)); + return EFI_EXIT(efi_dp_concat(src1, src2, 0)); }
/*

On Tue, 28 May 2024 at 17:43, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Allow appending a device-path to a device-path that contains an end node as separator. We need this feature for creating boot options specifying kernel, initrd, and dtb.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: update efi_dp_concat() instead of new function efi_dp_merge()
cmd/eficonfig.c | 6 +++--- cmd/efidebug.c | 4 ++-- include/efi_loader.h | 2 +- lib/efi_loader/efi_bootbin.c | 2 +- lib/efi_loader/efi_bootmgr.c | 2 +- lib/efi_loader/efi_boottime.c | 2 +- lib/efi_loader/efi_device_path.c | 20 +++++++++++++------- lib/efi_loader/efi_device_path_utilities.c | 2 +- 8 files changed, 23 insertions(+), 17 deletions(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 0ba92c60e03..b13d9a3d2d9 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -531,7 +531,7 @@ struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_ dp = efi_dp_shorten(dp_volume); if (!dp) dp = dp_volume;
dp = efi_dp_concat(dp, &fp->dp, false);
dp = efi_dp_concat(dp, &fp->dp, 0); free(buf); return dp;
@@ -1485,7 +1485,7 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo goto out; } initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,
dp, false);
dp, 0); efi_free_pool(dp); }
@@ -1496,7 +1496,7 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo } final_dp_size = efi_dp_size(dp) + sizeof(END); if (initrd_dp) {
final_dp = efi_dp_concat(dp, initrd_dp, true);
final_dp = efi_dp_concat(dp, initrd_dp, 1); final_dp_size += efi_dp_size(initrd_dp) + sizeof(END); } else { final_dp = efi_dp_dup(dp);
diff --git a/cmd/efidebug.c b/cmd/efidebug.c index c2c525f2351..762027daf8a 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -697,7 +697,7 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part, short_fp = tmp_fp;
initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,
short_fp, false);
short_fp, 0);
out: efi_free_pool(tmp_dp); @@ -917,7 +917,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, goto out; }
final_fp = efi_dp_concat(file_path, initrd_dp, true);
final_fp = efi_dp_concat(file_path, initrd_dp, 1); if (!final_fp) { printf("Cannot create final device path\n"); r = CMD_RET_FAILURE;
diff --git a/include/efi_loader.h b/include/efi_loader.h index 9600941aa32..ddf2e41a95c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -946,7 +946,7 @@ struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo, const efi_guid_t *guid); struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, const struct efi_device_path *dp2,
bool split_end_node);
size_t split_end_node);
struct efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path); efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data, efi_uintn_t *size); diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index b7910f78fb6..a87006b3c0e 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -150,7 +150,7 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) msg_path = file_path; } else { file_path = efi_dp_concat(bootefi_device_path,
bootefi_image_path, false);
bootefi_image_path, 0); msg_path = bootefi_image_path; log_debug("Loaded from disk\n"); }
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 7da3139f917..b0bf21cf841 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -130,7 +130,7 @@ static efi_status_t try_load_from_file_path(efi_handle_t *fs_handles, if (!dp) continue;
dp = efi_dp_concat(dp, fp, false);
dp = efi_dp_concat(dp, fp, 0); if (!dp) continue;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 1951291747c..630c5f52c4f 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1816,7 +1816,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, if (device_path) { info->device_handle = efi_dp_find_obj(device_path, NULL, NULL);
dp = efi_dp_concat(device_path, file_path, false);
dp = efi_dp_concat(device_path, file_path, 0); if (!dp) { ret = EFI_OUT_OF_RESOURCES; goto failure;
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index aec224d8466..c8c8d54f733 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -276,10 +276,11 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp)
- @dp1: First device path
- @dp2: Second device path
- @split_end_node: If true the two device paths will be concatenated and
separated by an end node (DEVICE_PATH_SUB_TYPE_END).
If false the second device path will be concatenated to the
first one as-is.
- @split_end_node:
- 0 to concatenate
- 1 to concatenate with end node added as separator
- size of dp1 excluding last end node to concatenate with end node as
- separator in case dp1 contains an end node
- Return:
- concatenated device path or NULL. Caller must free the returned value
@@ -287,7 +288,7 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp) struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, const struct efi_device_path *dp2,
bool split_end_node)
size_t split_end_node)
{ struct efi_device_path *ret; size_t end_size; @@ -301,10 +302,15 @@ efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, ret = efi_dp_dup(dp1); } else { /* both dp1 and dp2 are non-null */
unsigned sz1 = efi_dp_size(dp1);
unsigned sz2 = efi_dp_size(dp2);
size_t sz1;
size_t sz2 = efi_dp_size(dp2); void *p;
if (split_end_node < sizeof(struct efi_device_path))
Can we be more explicit here pls? Someone might misuse this in the future split_end_node < =1 ? And we can document we can use values up to sizeof(struct efi_device_path) if we ever need extra functionality
sz1 = efi_dp_size(dp1);
else
sz1 = split_end_node;
if (split_end_node) end_size = 2 * sizeof(END); else
diff --git a/lib/efi_loader/efi_device_path_utilities.c b/lib/efi_loader/efi_device_path_utilities.c index c95dbfa9b5f..ac250bbfcc9 100644 --- a/lib/efi_loader/efi_device_path_utilities.c +++ b/lib/efi_loader/efi_device_path_utilities.c @@ -76,7 +76,7 @@ static struct efi_device_path * EFIAPI append_device_path( const struct efi_device_path *src2) { EFI_ENTRY("%pD, %pD", src1, src2);
return EFI_EXIT(efi_dp_concat(src1, src2, false));
return EFI_EXIT(efi_dp_concat(src1, src2, 0));
}
/*
2.43.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On 28.05.24 18:02, Ilias Apalodimas wrote:
On Tue, 28 May 2024 at 17:43, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Allow appending a device-path to a device-path that contains an end node as separator. We need this feature for creating boot options specifying kernel, initrd, and dtb.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: update efi_dp_concat() instead of new function efi_dp_merge()
cmd/eficonfig.c | 6 +++--- cmd/efidebug.c | 4 ++-- include/efi_loader.h | 2 +- lib/efi_loader/efi_bootbin.c | 2 +- lib/efi_loader/efi_bootmgr.c | 2 +- lib/efi_loader/efi_boottime.c | 2 +- lib/efi_loader/efi_device_path.c | 20 +++++++++++++------- lib/efi_loader/efi_device_path_utilities.c | 2 +- 8 files changed, 23 insertions(+), 17 deletions(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 0ba92c60e03..b13d9a3d2d9 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -531,7 +531,7 @@ struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_ dp = efi_dp_shorten(dp_volume); if (!dp) dp = dp_volume;
dp = efi_dp_concat(dp, &fp->dp, false);
dp = efi_dp_concat(dp, &fp->dp, 0); free(buf); return dp;
@@ -1485,7 +1485,7 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo goto out; } initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,
dp, false);
dp, 0); efi_free_pool(dp); }
@@ -1496,7 +1496,7 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo } final_dp_size = efi_dp_size(dp) + sizeof(END); if (initrd_dp) {
final_dp = efi_dp_concat(dp, initrd_dp, true);
final_dp = efi_dp_concat(dp, initrd_dp, 1); final_dp_size += efi_dp_size(initrd_dp) + sizeof(END); } else { final_dp = efi_dp_dup(dp);
diff --git a/cmd/efidebug.c b/cmd/efidebug.c index c2c525f2351..762027daf8a 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -697,7 +697,7 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part, short_fp = tmp_fp;
initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,
short_fp, false);
short_fp, 0);
out: efi_free_pool(tmp_dp);
@@ -917,7 +917,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, goto out; }
final_fp = efi_dp_concat(file_path, initrd_dp, true);
final_fp = efi_dp_concat(file_path, initrd_dp, 1); if (!final_fp) { printf("Cannot create final device path\n"); r = CMD_RET_FAILURE;
diff --git a/include/efi_loader.h b/include/efi_loader.h index 9600941aa32..ddf2e41a95c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -946,7 +946,7 @@ struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo, const efi_guid_t *guid); struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, const struct efi_device_path *dp2,
bool split_end_node);
struct efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path); efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data, efi_uintn_t *size);size_t split_end_node);
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index b7910f78fb6..a87006b3c0e 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -150,7 +150,7 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) msg_path = file_path; } else { file_path = efi_dp_concat(bootefi_device_path,
bootefi_image_path, false);
bootefi_image_path, 0); msg_path = bootefi_image_path; log_debug("Loaded from disk\n"); }
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 7da3139f917..b0bf21cf841 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -130,7 +130,7 @@ static efi_status_t try_load_from_file_path(efi_handle_t *fs_handles, if (!dp) continue;
dp = efi_dp_concat(dp, fp, false);
dp = efi_dp_concat(dp, fp, 0); if (!dp) continue;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 1951291747c..630c5f52c4f 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1816,7 +1816,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, if (device_path) { info->device_handle = efi_dp_find_obj(device_path, NULL, NULL);
dp = efi_dp_concat(device_path, file_path, false);
dp = efi_dp_concat(device_path, file_path, 0); if (!dp) { ret = EFI_OUT_OF_RESOURCES; goto failure;
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index aec224d8466..c8c8d54f733 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -276,10 +276,11 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp)
- @dp1: First device path
- @dp2: Second device path
- @split_end_node: If true the two device paths will be concatenated and
separated by an end node (DEVICE_PATH_SUB_TYPE_END).
If false the second device path will be concatenated to the
first one as-is.
- @split_end_node:
- 0 to concatenate
- 1 to concatenate with end node added as separator
- size of dp1 excluding last end node to concatenate with end node as
- separator in case dp1 contains an end node
- Return:
- concatenated device path or NULL. Caller must free the returned value
@@ -287,7 +288,7 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp) struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, const struct efi_device_path *dp2,
bool split_end_node)
{ struct efi_device_path *ret; size_t end_size;size_t split_end_node)
@@ -301,10 +302,15 @@ efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, ret = efi_dp_dup(dp1); } else { /* both dp1 and dp2 are non-null */
unsigned sz1 = efi_dp_size(dp1);
unsigned sz2 = efi_dp_size(dp2);
size_t sz1;
size_t sz2 = efi_dp_size(dp2); void *p;
if (split_end_node < sizeof(struct efi_device_path))
Can we be more explicit here pls? Someone might misuse this in the future split_end_node < =1 ? And we can document we can use values up to sizeof(struct efi_device_path) if we ever need extra functionality
size_t split_end_node cannot be negative.
The case split_end_node == 0 is handled below. What are you missing?
Best regards
Heinrich
sz1 = efi_dp_size(dp1);
else
sz1 = split_end_node;
if (split_end_node) end_size = 2 * sizeof(END); else
diff --git a/lib/efi_loader/efi_device_path_utilities.c b/lib/efi_loader/efi_device_path_utilities.c index c95dbfa9b5f..ac250bbfcc9 100644 --- a/lib/efi_loader/efi_device_path_utilities.c +++ b/lib/efi_loader/efi_device_path_utilities.c @@ -76,7 +76,7 @@ static struct efi_device_path * EFIAPI append_device_path( const struct efi_device_path *src2) { EFI_ENTRY("%pD, %pD", src1, src2);
return EFI_EXIT(efi_dp_concat(src1, src2, false));
return EFI_EXIT(efi_dp_concat(src1, src2, 0));
}
/*
-- 2.43.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

[...]
unsigned sz2 = efi_dp_size(dp2);
size_t sz1;
size_t sz2 = efi_dp_size(dp2); void *p;
if (split_end_node < sizeof(struct efi_device_path))
Can we be more explicit here pls? Someone might misuse this in the future split_end_node < =1 ? And we can document we can use values up to sizeof(struct efi_device_path) if we ever need extra functionality
size_t split_end_node cannot be negative.
The case split_end_node == 0 is handled below. What are you missing?
someone misusing it and passing a value of '3' for example and print an error if the value is 1 < value < sizeof(struct efi_device_path)
Thanks /Ilias
Best regards
Heinrich
sz1 = efi_dp_size(dp1);
else
sz1 = split_end_node;
if (split_end_node) end_size = 2 * sizeof(END); else
diff --git a/lib/efi_loader/efi_device_path_utilities.c b/lib/efi_loader/efi_device_path_utilities.c index c95dbfa9b5f..ac250bbfcc9 100644 --- a/lib/efi_loader/efi_device_path_utilities.c +++ b/lib/efi_loader/efi_device_path_utilities.c @@ -76,7 +76,7 @@ static struct efi_device_path * EFIAPI append_device_path( const struct efi_device_path *src2) { EFI_ENTRY("%pD, %pD", src1, src2);
return EFI_EXIT(efi_dp_concat(src1, src2, false));
return EFI_EXIT(efi_dp_concat(src1, src2, 0));
}
/*
-- 2.43.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On 28.05.24 18:18, Ilias Apalodimas wrote:
[...]
unsigned sz2 = efi_dp_size(dp2);
size_t sz1;
size_t sz2 = efi_dp_size(dp2); void *p;
if (split_end_node < sizeof(struct efi_device_path))
Can we be more explicit here pls? Someone might misuse this in the future split_end_node < =1 ? And we can document we can use values up to sizeof(struct efi_device_path) if we ever need extra functionality
size_t split_end_node cannot be negative.
The case split_end_node == 0 is handled below. What are you missing?
someone misusing it and passing a value of '3' for example and print an error if the value is 1 < value < sizeof(struct efi_device_path)
I would like to avoid over-engineering. How about
- if (split_end_node < sizeof(struct efi_device_path)) + if (split_end_node < 2)
and changing the function description to refer to >= 2?
Best regards
Heinrich
Thanks /Ilias
Best regards
Heinrich
sz1 = efi_dp_size(dp1);
else
sz1 = split_end_node;
if (split_end_node) end_size = 2 * sizeof(END); else
diff --git a/lib/efi_loader/efi_device_path_utilities.c b/lib/efi_loader/efi_device_path_utilities.c index c95dbfa9b5f..ac250bbfcc9 100644 --- a/lib/efi_loader/efi_device_path_utilities.c +++ b/lib/efi_loader/efi_device_path_utilities.c @@ -76,7 +76,7 @@ static struct efi_device_path * EFIAPI append_device_path( const struct efi_device_path *src2) { EFI_ENTRY("%pD, %pD", src1, src2);
return EFI_EXIT(efi_dp_concat(src1, src2, false));
return EFI_EXIT(efi_dp_concat(src1, src2, 0));
}
/*
-- 2.43.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On Tue, 28 May 2024 at 19:59, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 28.05.24 18:18, Ilias Apalodimas wrote:
[...]
unsigned sz2 = efi_dp_size(dp2);
size_t sz1;
size_t sz2 = efi_dp_size(dp2); void *p;
if (split_end_node < sizeof(struct efi_device_path))
Can we be more explicit here pls? Someone might misuse this in the future split_end_node < =1 ? And we can document we can use values up to sizeof(struct efi_device_path) if we ever need extra functionality
size_t split_end_node cannot be negative.
The case split_end_node == 0 is handled below. What are you missing?
someone misusing it and passing a value of '3' for example and print an error if the value is 1 < value < sizeof(struct efi_device_path)
I would like to avoid over-engineering. How about
if (split_end_node < sizeof(struct efi_device_path))
if (split_end_node < 2)
and changing the function description to refer to >= 2?
Nop, in that case, I prefer what's already there, simply because values between 0 < sizeof(struct efi_device_path) will be used for functionality similar to the one we have for 0,1 in the future
Thanks /Ilias
Best regards
Heinrich
Thanks /Ilias
Best regards
Heinrich
sz1 = efi_dp_size(dp1);
else
sz1 = split_end_node;
if (split_end_node) end_size = 2 * sizeof(END); else
diff --git a/lib/efi_loader/efi_device_path_utilities.c b/lib/efi_loader/efi_device_path_utilities.c index c95dbfa9b5f..ac250bbfcc9 100644 --- a/lib/efi_loader/efi_device_path_utilities.c +++ b/lib/efi_loader/efi_device_path_utilities.c @@ -76,7 +76,7 @@ static struct efi_device_path * EFIAPI append_device_path( const struct efi_device_path *src2) { EFI_ENTRY("%pD, %pD", src1, src2);
return EFI_EXIT(efi_dp_concat(src1, src2, false));
return EFI_EXIT(efi_dp_concat(src1, src2, 0));
}
/*
-- 2.43.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

We already support creating a load option where the device-path field contains the concatenation of the binary device-path and optionally the device path of the initrd which we expose via the EFI_LOAD_FILE2_PROTOCOL.
Allow to append another device-path pointing to the device-tree identified by the device-tree GUID.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: Carve out a function efi_load_option_dp_join() which we can use both for the eficonfig and the efidebug command. Rename variables id_dp, final_dp_size. --- cmd/eficonfig.c | 79 ++++++++++++++++++++++++++++--------- include/efi_loader.h | 14 +++++++ lib/efi_loader/efi_helper.c | 44 +++++++++++++++++++++ 3 files changed, 119 insertions(+), 18 deletions(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index b13d9a3d2d9..a12df1e7d59 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -62,6 +62,7 @@ struct eficonfig_filepath_info { struct eficonfig_boot_option { struct eficonfig_select_file_info file_info; struct eficonfig_select_file_info initrd_info; + struct eficonfig_select_file_info fdt_info; unsigned int boot_index; u16 *description; u16 *optional_data; @@ -1308,6 +1309,10 @@ static efi_status_t eficonfig_show_boot_option(struct eficonfig_boot_option *bo, if (ret != EFI_SUCCESS) goto out;
+ ret = prepare_file_selection_entry(efi_menu, "Fdt File: ", &bo->fdt_info); + if (ret != EFI_SUCCESS) + goto out; + ret = create_boot_option_entry(efi_menu, "Optional Data: ", bo->optional_data, eficonfig_boot_add_optional_data, bo); if (ret != EFI_SUCCESS) @@ -1388,27 +1393,44 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo efi_status_t ret; char *tmp = NULL, *p; struct efi_load_option lo = {0}; - efi_uintn_t final_dp_size; + efi_uintn_t dp_size; struct efi_device_path *dp = NULL; efi_uintn_t size = load_option_size; - struct efi_device_path *final_dp = NULL; struct efi_device_path *device_dp = NULL; struct efi_device_path *initrd_dp = NULL; + struct efi_device_path *fdt_dp = NULL; struct efi_device_path *initrd_device_dp = NULL; + struct efi_device_path *fdt_device_dp = NULL;
- const struct efi_initrd_dp id_dp = { + const struct efi_initrd_dp initrd_prefix = { .vendor = { { DEVICE_PATH_TYPE_MEDIA_DEVICE, DEVICE_PATH_SUB_TYPE_VENDOR_PATH, - sizeof(id_dp.vendor), + sizeof(initrd_prefix.vendor), }, EFI_INITRD_MEDIA_GUID, }, .end = { DEVICE_PATH_TYPE_END, DEVICE_PATH_SUB_TYPE_END, - sizeof(id_dp.end), + sizeof(initrd_prefix.end), + } + }; + + const struct efi_initrd_dp fdt_prefix = { + .vendor = { + { + DEVICE_PATH_TYPE_MEDIA_DEVICE, + DEVICE_PATH_SUB_TYPE_VENDOR_PATH, + sizeof(fdt_prefix.vendor), + }, + EFI_FDT_GUID, + }, + .end = { + DEVICE_PATH_TYPE_END, + DEVICE_PATH_SUB_TYPE_END, + sizeof(initrd_prefix.end), } };
@@ -1424,6 +1446,12 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo goto out; }
+ bo->fdt_info.current_path = calloc(1, EFICONFIG_FILE_PATH_BUF_SIZE); + if (!bo->fdt_info.current_path) { + ret = EFI_OUT_OF_RESOURCES; + goto out; + } + bo->description = calloc(1, EFICONFIG_DESCRIPTION_MAX * sizeof(u16)); if (!bo->description) { ret = EFI_OUT_OF_RESOURCES; @@ -1456,13 +1484,20 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo if (lo.file_path) fill_file_info(lo.file_path, &bo->file_info, device_dp);
- /* Initrd file path(optional) is placed at second instance. */ + /* Initrd file path (optional) is placed at second instance. */ initrd_dp = efi_dp_from_lo(&lo, &efi_lf2_initrd_guid); if (initrd_dp) { fill_file_info(initrd_dp, &bo->initrd_info, initrd_device_dp); efi_free_pool(initrd_dp); }
+ /* Fdt file path (optional) is placed as third instance. */ + fdt_dp = efi_dp_from_lo(&lo, &efi_guid_fdt); + if (fdt_dp) { + fill_file_info(fdt_dp, &bo->fdt_info, fdt_device_dp); + efi_free_pool(fdt_dp); + } + if (size > 0) memcpy(bo->optional_data, lo.optional_data, size); } @@ -1484,26 +1519,31 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo ret = EFI_OUT_OF_RESOURCES; goto out; } - initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp, + initrd_dp = efi_dp_concat((const struct efi_device_path *)&initrd_prefix, dp, 0); efi_free_pool(dp); }
+ if (bo->fdt_info.dp_volume) { + dp = eficonfig_create_device_path(bo->fdt_info.dp_volume, + bo->fdt_info.current_path); + if (!dp) { + ret = EFI_OUT_OF_RESOURCES; + goto out; + } + fdt_dp = efi_dp_concat((const struct efi_device_path *)&fdt_prefix, + dp, 0); + efi_free_pool(dp); + } + dp = eficonfig_create_device_path(bo->file_info.dp_volume, bo->file_info.current_path); if (!dp) { ret = EFI_OUT_OF_RESOURCES; goto out; } - final_dp_size = efi_dp_size(dp) + sizeof(END); - if (initrd_dp) { - final_dp = efi_dp_concat(dp, initrd_dp, 1); - final_dp_size += efi_dp_size(initrd_dp) + sizeof(END); - } else { - final_dp = efi_dp_dup(dp); - } - efi_free_pool(dp);
- if (!final_dp) + ret = efi_load_option_dp_join(&dp, &dp_size, initrd_dp, fdt_dp); + if (ret != EFI_SUCCESS) goto out;
if (utf16_utf8_strlen(bo->optional_data)) { @@ -1515,17 +1555,20 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo utf16_utf8_strncpy(&p, bo->optional_data, u16_strlen(bo->optional_data)); }
- ret = eficonfig_set_boot_option(varname, final_dp, final_dp_size, bo->description, tmp); + ret = eficonfig_set_boot_option(varname, dp, dp_size, bo->description, tmp); out: free(tmp); free(bo->optional_data); free(bo->description); free(bo->file_info.current_path); free(bo->initrd_info.current_path); + free(bo->fdt_info.current_path); efi_free_pool(device_dp); efi_free_pool(initrd_device_dp); efi_free_pool(initrd_dp); - efi_free_pool(final_dp); + efi_free_pool(fdt_device_dp); + efi_free_pool(fdt_dp); + efi_free_pool(dp);
return ret; } diff --git a/include/efi_loader.h b/include/efi_loader.h index ddf2e41a95c..1236eecff0f 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -1185,4 +1185,18 @@ efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int */ void efi_add_known_memory(void);
+/** + * efi_load_option_dp_join() - join device-paths for load option + * + * @dp: in: binary device-path, out: joined device-path + * @dp_size: size of joined device-path + * @initrd_dp: initrd device-path or NULL + * @fdt_dp: device-tree device-path or NULL + * Return: status_code + */ +efi_status_t efi_load_option_dp_join(struct efi_device_path **dp, + size_t *dp_size, + struct efi_device_path *initrd_dp, + struct efi_device_path *fdt_dp); + #endif /* _EFI_LOADER_H */ diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c index 73d0279e843..348612c3dad 100644 --- a/lib/efi_loader/efi_helper.c +++ b/lib/efi_loader/efi_helper.c @@ -99,6 +99,50 @@ err: return NULL; }
+/** + * efi_load_option_dp_join() - join device-paths for load option + * + * @dp: in: binary device-path, out: joined device-path + * @dp_size: size of joined device-path + * @initrd_dp: initrd device-path or NULL + * @fdt_dp: device-tree device-path or NULL + * Return: status_code + */ +efi_status_t efi_load_option_dp_join(struct efi_device_path **dp, + size_t *dp_size, + struct efi_device_path *initrd_dp, + struct efi_device_path *fdt_dp) +{ + if (!dp) + return EFI_INVALID_PARAMETER; + + *dp_size = efi_dp_size(*dp); + + if (initrd_dp) { + struct efi_device_path *tmp_dp = *dp; + + *dp = efi_dp_concat(tmp_dp, initrd_dp, *dp_size); + efi_free_pool(tmp_dp); + if (!*dp) + return EFI_OUT_OF_RESOURCES; + *dp_size += efi_dp_size(initrd_dp) + sizeof(END); + } + + if (fdt_dp) { + struct efi_device_path *tmp_dp = *dp; + + *dp = efi_dp_concat(tmp_dp, fdt_dp, *dp_size); + efi_free_pool(tmp_dp); + if (!dp) + return EFI_OUT_OF_RESOURCES; + *dp_size += efi_dp_size(fdt_dp) + sizeof(END); + } + + *dp_size += sizeof(END); + + return EFI_SUCCESS; +} + const struct guid_to_hash_map { efi_guid_t guid; const char algo[32];

Hi Heinrich,
[...]
const struct efi_initrd_dp id_dp = {
const struct efi_initrd_dp initrd_prefix = { .vendor = { { DEVICE_PATH_TYPE_MEDIA_DEVICE, DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
sizeof(id_dp.vendor),
sizeof(initrd_prefix.vendor), }, EFI_INITRD_MEDIA_GUID, }, .end = { DEVICE_PATH_TYPE_END, DEVICE_PATH_SUB_TYPE_END,
sizeof(id_dp.end),
sizeof(initrd_prefix.end),
}
};
const struct efi_initrd_dp fdt_prefix = {
We need to rename efi_initrd_dp to something more generic in the future
.vendor = {
{
DEVICE_PATH_TYPE_MEDIA_DEVICE,
DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
sizeof(fdt_prefix.vendor),
},
EFI_FDT_GUID,
},
.end = {
DEVICE_PATH_TYPE_END,
DEVICE_PATH_SUB_TYPE_END,
sizeof(initrd_prefix.end), } };
[...]
+/**
- efi_load_option_dp_join() - join device-paths for load option
- @dp: in: binary device-path, out: joined device-path
- @dp_size: size of joined device-path
- @initrd_dp: initrd device-path or NULL
- @fdt_dp: device-tree device-path or NULL
- Return: status_code
- */
+efi_status_t efi_load_option_dp_join(struct efi_device_path **dp,
size_t *dp_size,
struct efi_device_path *initrd_dp,
struct efi_device_path *fdt_dp)
+{
if (!dp)
Should we add && !*dp here?
return EFI_INVALID_PARAMETER;
*dp_size = efi_dp_size(*dp);
if (initrd_dp) {
struct efi_device_path *tmp_dp = *dp;
*dp = efi_dp_concat(tmp_dp, initrd_dp, *dp_size);
efi_free_pool(tmp_dp);
if (!*dp)
return EFI_OUT_OF_RESOURCES;
*dp_size += efi_dp_size(initrd_dp) + sizeof(END);
}
if (fdt_dp) {
struct efi_device_path *tmp_dp = *dp;
*dp = efi_dp_concat(tmp_dp, fdt_dp, *dp_size);
efi_free_pool(tmp_dp);
if (!dp)
return EFI_OUT_OF_RESOURCES;
*dp_size += efi_dp_size(fdt_dp) + sizeof(END);
}
*dp_size += sizeof(END);
Why do we have to account for the end node twice if either fdt_dp or initrd_dp are found?
Thanks /Ilias
return EFI_SUCCESS;
+}
const struct guid_to_hash_map { efi_guid_t guid; const char algo[32]; -- 2.43.0

On 28.05.24 17:16, Ilias Apalodimas wrote:
Hi Heinrich,
[...]
const struct efi_initrd_dp id_dp = {
const struct efi_initrd_dp initrd_prefix = { .vendor = { { DEVICE_PATH_TYPE_MEDIA_DEVICE, DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
sizeof(id_dp.vendor),
sizeof(initrd_prefix.vendor), }, EFI_INITRD_MEDIA_GUID, }, .end = { DEVICE_PATH_TYPE_END, DEVICE_PATH_SUB_TYPE_END,
sizeof(id_dp.end),
sizeof(initrd_prefix.end),
}
};
const struct efi_initrd_dp fdt_prefix = {
We need to rename efi_initrd_dp to something more generic in the future
.vendor = {
{
DEVICE_PATH_TYPE_MEDIA_DEVICE,
DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
sizeof(fdt_prefix.vendor),
},
EFI_FDT_GUID,
},
.end = {
DEVICE_PATH_TYPE_END,
DEVICE_PATH_SUB_TYPE_END,
sizeof(initrd_prefix.end), } };
[...]
+/**
- efi_load_option_dp_join() - join device-paths for load option
- @dp: in: binary device-path, out: joined device-path
- @dp_size: size of joined device-path
- @initrd_dp: initrd device-path or NULL
- @fdt_dp: device-tree device-path or NULL
- Return: status_code
- */
+efi_status_t efi_load_option_dp_join(struct efi_device_path **dp,
size_t *dp_size,
struct efi_device_path *initrd_dp,
struct efi_device_path *fdt_dp)
+{
if (!dp)
Should we add && !*dp here?
efi_dp_concat() handles the case of one or both device-paths being NULL.
return EFI_INVALID_PARAMETER;
*dp_size = efi_dp_size(*dp);
if (initrd_dp) {
struct efi_device_path *tmp_dp = *dp;
*dp = efi_dp_concat(tmp_dp, initrd_dp, *dp_size);
efi_free_pool(tmp_dp);
if (!*dp)
return EFI_OUT_OF_RESOURCES;
*dp_size += efi_dp_size(initrd_dp) + sizeof(END);
}
if (fdt_dp) {
struct efi_device_path *tmp_dp = *dp;
*dp = efi_dp_concat(tmp_dp, fdt_dp, *dp_size);
efi_free_pool(tmp_dp);
if (!dp)
return EFI_OUT_OF_RESOURCES;
*dp_size += efi_dp_size(fdt_dp) + sizeof(END);
}
*dp_size += sizeof(END);
Why do we have to account for the end node twice if either fdt_dp or initrd_dp are found?
This is the length of the END node of the binary.
Best regards
Heinrich
Thanks /Ilias
return EFI_SUCCESS;
+}
- const struct guid_to_hash_map { efi_guid_t guid; const char algo[32];
-- 2.43.0

On Tue, 28 May 2024 at 19:08, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 28.05.24 17:16, Ilias Apalodimas wrote:
Hi Heinrich,
[...]
const struct efi_initrd_dp id_dp = {
const struct efi_initrd_dp initrd_prefix = { .vendor = { { DEVICE_PATH_TYPE_MEDIA_DEVICE, DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
sizeof(id_dp.vendor),
sizeof(initrd_prefix.vendor), }, EFI_INITRD_MEDIA_GUID, }, .end = { DEVICE_PATH_TYPE_END, DEVICE_PATH_SUB_TYPE_END,
sizeof(id_dp.end),
sizeof(initrd_prefix.end),
}
};
const struct efi_initrd_dp fdt_prefix = {
We need to rename efi_initrd_dp to something more generic in the future
.vendor = {
{
DEVICE_PATH_TYPE_MEDIA_DEVICE,
DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
sizeof(fdt_prefix.vendor),
},
EFI_FDT_GUID,
},
.end = {
DEVICE_PATH_TYPE_END,
DEVICE_PATH_SUB_TYPE_END,
sizeof(initrd_prefix.end), } };
[...]
+/**
- efi_load_option_dp_join() - join device-paths for load option
- @dp: in: binary device-path, out: joined device-path
- @dp_size: size of joined device-path
- @initrd_dp: initrd device-path or NULL
- @fdt_dp: device-tree device-path or NULL
- Return: status_code
- */
+efi_status_t efi_load_option_dp_join(struct efi_device_path **dp,
size_t *dp_size,
struct efi_device_path *initrd_dp,
struct efi_device_path *fdt_dp)
+{
if (!dp)
Should we add && !*dp here?
efi_dp_concat() handles the case of one or both device-paths being NULL.
Fair enough
return EFI_INVALID_PARAMETER;
*dp_size = efi_dp_size(*dp);
if (initrd_dp) {
struct efi_device_path *tmp_dp = *dp;
*dp = efi_dp_concat(tmp_dp, initrd_dp, *dp_size);
efi_free_pool(tmp_dp);
if (!*dp)
return EFI_OUT_OF_RESOURCES;
*dp_size += efi_dp_size(initrd_dp) + sizeof(END);
}
if (fdt_dp) {
struct efi_device_path *tmp_dp = *dp;
*dp = efi_dp_concat(tmp_dp, fdt_dp, *dp_size);
efi_free_pool(tmp_dp);
if (!dp)
return EFI_OUT_OF_RESOURCES;
*dp_size += efi_dp_size(fdt_dp) + sizeof(END);
}
*dp_size += sizeof(END);
Why do we have to account for the end node twice if either fdt_dp or initrd_dp are found?
This is the length of the END node of the binary.
ah yes correct.
Feel free to add Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org but we need to fix the naming at some point
Best regards
Heinrich
Thanks /Ilias
return EFI_SUCCESS;
+}
- const struct guid_to_hash_map { efi_guid_t guid; const char algo[32];
-- 2.43.0

We already support creating a load option where the device-path field contains the concatenation of the binary device-path and optionally the device path of the initrd which we expose via the EFI_LOAD_FILE2_PROTOCOL.
Allow to append another device-path pointing to the device-tree identified by the device-tree GUID.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: Rename create_initrd_dp() to create_lo_dp_part(). Use enum as parameter for create_lo_dp_part(). Use function efi_load_option_dp_join() to avoid code duplication. --- cmd/efidebug.c | 130 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 93 insertions(+), 37 deletions(-)
diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 762027daf8a..966b4b83361 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -654,38 +654,80 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag, }
/** - * create_initrd_dp() - create a special device for our Boot### option + * enum efi_lo_dp_part - part of device path in load option + */ +enum efi_lo_dp_part { + /** @EFI_LO_DP_PART_BINARY: binary */ + EFI_LO_DP_PART_BINARY, + /** @EFI_LO_DP_PART_INITRD: initial RAM disk */ + EFI_LO_DP_PART_INITRD, + /** EFI_LP_DP_PART_FDT: device-tree */ + EFI_LP_DP_PART_FDT, +}; + +/** + * create_lo_dp() - create a special device path for our Boot### option * * @dev: device * @part: disk partition * @file: filename * @shortform: create short form device path + * @type: part of device path to be created * Return: pointer to the device path or ERR_PTR */ static -struct efi_device_path *create_initrd_dp(const char *dev, const char *part, - const char *file, int shortform) +struct efi_device_path *create_lo_dp_part(const char *dev, const char *part, + const char *file, bool shortform, + enum efi_lo_dp_part type)
{ struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL, *short_fp = NULL; - struct efi_device_path *initrd_dp = NULL; + struct efi_device_path *dp = NULL; + const struct efi_device_path *dp_prefix; efi_status_t ret; - const struct efi_initrd_dp id_dp = { + const struct efi_initrd_dp fdt_dp = { + .vendor = { + { + DEVICE_PATH_TYPE_MEDIA_DEVICE, + DEVICE_PATH_SUB_TYPE_VENDOR_PATH, + sizeof(fdt_dp.vendor), + }, + EFI_FDT_GUID, + }, + .end = { + DEVICE_PATH_TYPE_END, + DEVICE_PATH_SUB_TYPE_END, + sizeof(fdt_dp.end), + } + }; + const struct efi_initrd_dp initrd_dp = { .vendor = { { DEVICE_PATH_TYPE_MEDIA_DEVICE, DEVICE_PATH_SUB_TYPE_VENDOR_PATH, - sizeof(id_dp.vendor), + sizeof(initrd_dp.vendor), }, EFI_INITRD_MEDIA_GUID, }, .end = { DEVICE_PATH_TYPE_END, DEVICE_PATH_SUB_TYPE_END, - sizeof(id_dp.end), + sizeof(initrd_dp.end), } };
+ switch (type) { + case EFI_LO_DP_PART_INITRD: + dp_prefix = &initrd_dp.vendor.dp; + break; + case EFI_LP_DP_PART_FDT: + dp_prefix = &fdt_dp.vendor.dp; + break; + default: + dp_prefix = NULL; + break; + } + ret = efi_dp_from_name(dev, part, file, &tmp_dp, &tmp_fp); if (ret != EFI_SUCCESS) { printf("Cannot create device path for "%s %s"\n", part, file); @@ -696,13 +739,12 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part, if (!short_fp) short_fp = tmp_fp;
- initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp, - short_fp, 0); + dp = efi_dp_concat(dp_prefix, short_fp, 0);
out: efi_free_pool(tmp_dp); efi_free_pool(tmp_fp); - return initrd_dp; + return dp; }
/** @@ -793,9 +835,8 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, efi_guid_t guid; u16 *label; struct efi_device_path *file_path = NULL; - struct efi_device_path *fp_free = NULL; - struct efi_device_path *final_fp = NULL; struct efi_device_path *initrd_dp = NULL; + struct efi_device_path *fdt_dp = NULL; struct efi_load_option lo; void *data = NULL; efi_uintn_t size; @@ -843,22 +884,31 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, lo.label = label; /* label will be changed below */
/* file path */ - ret = efi_dp_from_name(argv[3], argv[4], argv[5], - NULL, &fp_free); - if (ret != EFI_SUCCESS) { - printf("Cannot create device path for "%s %s"\n", - argv[3], argv[4]); + file_path = create_lo_dp_part(argv[3], argv[4], argv[5], + shortform, + EFI_LO_DP_PART_BINARY); + argc -= 5; + argv += 5; + break; + case 'd': + shortform = 1; + fallthrough; + case 'D': + if (argc < 3 || fdt_dp) { + r = CMD_RET_USAGE; + goto out; + } + + fdt_dp = create_lo_dp_part(argv[1], argv[2], argv[3], + shortform, + EFI_LP_DP_PART_FDT); + if (!fdt_dp) { + printf("Cannot add a device-tree\n"); r = CMD_RET_FAILURE; goto out; } - if (shortform) - file_path = efi_dp_shorten(fp_free); - if (!file_path) - file_path = fp_free; - fp_size += efi_dp_size(file_path) + - sizeof(struct efi_device_path); - argc -= 5; - argv += 5; + argc -= 3; + argv += 3; break; case 'i': shortform = 1; @@ -869,8 +919,9 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, goto out; }
- initrd_dp = create_initrd_dp(argv[1], argv[2], argv[3], - shortform); + initrd_dp = create_lo_dp_part(argv[1], argv[2], argv[3], + shortform, + EFI_LO_DP_PART_INITRD); if (!initrd_dp) { printf("Cannot add an initrd\n"); r = CMD_RET_FAILURE; @@ -878,8 +929,6 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, } argc -= 3; argv += 3; - fp_size += efi_dp_size(initrd_dp) + - sizeof(struct efi_device_path); break; case 's': if (argc < 1 || lo.optional_data) { @@ -897,7 +946,6 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, &file_path, &fp_size); if (r != CMD_RET_SUCCESS) goto out; - fp_free = file_path; argc -= 3; argv += 3; } else{ @@ -917,14 +965,14 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, goto out; }
- final_fp = efi_dp_concat(file_path, initrd_dp, 1); - if (!final_fp) { + ret = efi_load_option_dp_join(&file_path, &fp_size, initrd_dp, fdt_dp); + if (ret != EFI_SUCCESS) { printf("Cannot create final device path\n"); r = CMD_RET_FAILURE; goto out; }
- lo.file_path = final_fp; + lo.file_path = file_path; lo.file_path_length = fp_size;
size = efi_serialize_load_option(&lo, (u8 **)&data); @@ -945,9 +993,9 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
out: free(data); - efi_free_pool(final_fp); efi_free_pool(initrd_dp); - efi_free_pool(fp_free); + efi_free_pool(fdt_dp); + efi_free_pool(file_path); free(lo.label);
return r; @@ -1009,7 +1057,8 @@ static int do_efi_boot_rm(struct cmd_tbl *cmdtp, int flag, */ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) { - struct efi_device_path *initrd_path = NULL; + struct efi_device_path *fdt_path; + struct efi_device_path *initrd_path; struct efi_load_option lo; efi_status_t ret;
@@ -1038,6 +1087,12 @@ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) efi_free_pool(initrd_path); }
+ fdt_path = efi_dp_from_lo(&lo, &efi_guid_fdt); + if (fdt_path) { + printf(" device-tree path: %pD\n", fdt_path); + efi_free_pool(fdt_path); + } + printf(" data:\n"); print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, lo.optional_data, *size, true); @@ -1565,8 +1620,9 @@ U_BOOT_LONGHELP(efidebug, "\n" "efidebug boot add - set UEFI BootXXXX variable\n" " -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n" + " -d|-D <interface> <devnum>[:<part>] <device-tree file path>\n" " -i|-I <interface> <devnum>[:<part>] <initrd file path>\n" - " (-b, -i for short form device path)\n" + " (-b, -d, -i for short form device path)\n" #if (IS_ENABLED(CONFIG_EFI_HTTP_BOOT)) " -u <bootid> <label> <uri>\n" #endif

On Tue, 28 May 2024 at 17:43, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
We already support creating a load option where the device-path field contains the concatenation of the binary device-path and optionally the device path of the initrd which we expose via the EFI_LOAD_FILE2_PROTOCOL.
Allow to append another device-path pointing to the device-tree identified by the device-tree GUID.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: Rename create_initrd_dp() to create_lo_dp_part(). Use enum as parameter for create_lo_dp_part(). Use function efi_load_option_dp_join() to avoid code duplication.
cmd/efidebug.c | 130 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 93 insertions(+), 37 deletions(-)
diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 762027daf8a..966b4b83361 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -654,38 +654,80 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag, }
/**
- create_initrd_dp() - create a special device for our Boot### option
- enum efi_lo_dp_part - part of device path in load option
- */
+enum efi_lo_dp_part {
/** @EFI_LO_DP_PART_BINARY: binary */
EFI_LO_DP_PART_BINARY,
/** @EFI_LO_DP_PART_INITRD: initial RAM disk */
EFI_LO_DP_PART_INITRD,
/** EFI_LP_DP_PART_FDT: device-tree */
EFI_LP_DP_PART_FDT,
+};
+/**
- create_lo_dp() - create a special device path for our Boot### option
- @dev: device
- @part: disk partition
- @file: filename
- @shortform: create short form device path
*/
- @type: part of device path to be created
- Return: pointer to the device path or ERR_PTR
static -struct efi_device_path *create_initrd_dp(const char *dev, const char *part,
const char *file, int shortform)
+struct efi_device_path *create_lo_dp_part(const char *dev, const char *part,
const char *file, bool shortform,
enum efi_lo_dp_part type)
{ struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL, *short_fp = NULL;
struct efi_device_path *initrd_dp = NULL;
struct efi_device_path *dp = NULL;
const struct efi_device_path *dp_prefix; efi_status_t ret;
const struct efi_initrd_dp id_dp = {
const struct efi_initrd_dp fdt_dp = {
.vendor = {
{
DEVICE_PATH_TYPE_MEDIA_DEVICE,
DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
sizeof(fdt_dp.vendor),
},
EFI_FDT_GUID,
},
.end = {
DEVICE_PATH_TYPE_END,
DEVICE_PATH_SUB_TYPE_END,
sizeof(fdt_dp.end),
}
};
const struct efi_initrd_dp initrd_dp = { .vendor = { { DEVICE_PATH_TYPE_MEDIA_DEVICE, DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
sizeof(id_dp.vendor),
sizeof(initrd_dp.vendor), }, EFI_INITRD_MEDIA_GUID, }, .end = { DEVICE_PATH_TYPE_END, DEVICE_PATH_SUB_TYPE_END,
sizeof(id_dp.end),
sizeof(initrd_dp.end), } };
switch (type) {
case EFI_LO_DP_PART_INITRD:
dp_prefix = &initrd_dp.vendor.dp;
break;
case EFI_LP_DP_PART_FDT:
dp_prefix = &fdt_dp.vendor.dp;
break;
default:
dp_prefix = NULL;
break;
}
ret = efi_dp_from_name(dev, part, file, &tmp_dp, &tmp_fp); if (ret != EFI_SUCCESS) { printf("Cannot create device path for \"%s %s\"\n", part, file);
@@ -696,13 +739,12 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part, if (!short_fp) short_fp = tmp_fp;
initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,
short_fp, 0);
dp = efi_dp_concat(dp_prefix, short_fp, 0);
out: efi_free_pool(tmp_dp); efi_free_pool(tmp_fp);
return initrd_dp;
return dp;
}
/** @@ -793,9 +835,8 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, efi_guid_t guid; u16 *label; struct efi_device_path *file_path = NULL;
struct efi_device_path *fp_free = NULL;
struct efi_device_path *final_fp = NULL; struct efi_device_path *initrd_dp = NULL;
struct efi_device_path *fdt_dp = NULL; struct efi_load_option lo; void *data = NULL; efi_uintn_t size;
@@ -843,22 +884,31 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, lo.label = label; /* label will be changed below */
/* file path */
ret = efi_dp_from_name(argv[3], argv[4], argv[5],
NULL, &fp_free);
if (ret != EFI_SUCCESS) {
printf("Cannot create device path for \"%s %s\"\n",
argv[3], argv[4]);
file_path = create_lo_dp_part(argv[3], argv[4], argv[5],
shortform,
EFI_LO_DP_PART_BINARY);
argc -= 5;
argv += 5;
break;
case 'd':
shortform = 1;
fallthrough;
case 'D':
if (argc < 3 || fdt_dp) {
r = CMD_RET_USAGE;
goto out;
}
fdt_dp = create_lo_dp_part(argv[1], argv[2], argv[3],
shortform,
EFI_LP_DP_PART_FDT);
if (!fdt_dp) {
printf("Cannot add a device-tree\n"); r = CMD_RET_FAILURE; goto out; }
if (shortform)
file_path = efi_dp_shorten(fp_free);
if (!file_path)
file_path = fp_free;
fp_size += efi_dp_size(file_path) +
sizeof(struct efi_device_path);
argc -= 5;
argv += 5;
argc -= 3;
argv += 3; break; case 'i': shortform = 1;
@@ -869,8 +919,9 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, goto out; }
initrd_dp = create_initrd_dp(argv[1], argv[2], argv[3],
shortform);
initrd_dp = create_lo_dp_part(argv[1], argv[2], argv[3],
shortform,
EFI_LO_DP_PART_INITRD); if (!initrd_dp) { printf("Cannot add an initrd\n"); r = CMD_RET_FAILURE;
@@ -878,8 +929,6 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, } argc -= 3; argv += 3;
fp_size += efi_dp_size(initrd_dp) +
sizeof(struct efi_device_path); break; case 's': if (argc < 1 || lo.optional_data) {
@@ -897,7 +946,6 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, &file_path, &fp_size); if (r != CMD_RET_SUCCESS) goto out;
fp_free = file_path; argc -= 3; argv += 3; } else{
@@ -917,14 +965,14 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, goto out; }
final_fp = efi_dp_concat(file_path, initrd_dp, 1);
if (!final_fp) {
ret = efi_load_option_dp_join(&file_path, &fp_size, initrd_dp, fdt_dp);
if (ret != EFI_SUCCESS) { printf("Cannot create final device path\n"); r = CMD_RET_FAILURE; goto out; }
lo.file_path = final_fp;
lo.file_path = file_path; lo.file_path_length = fp_size; size = efi_serialize_load_option(&lo, (u8 **)&data);
@@ -945,9 +993,9 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
out: free(data);
efi_free_pool(final_fp); efi_free_pool(initrd_dp);
efi_free_pool(fp_free);
efi_free_pool(fdt_dp);
efi_free_pool(file_path); free(lo.label); return r;
@@ -1009,7 +1057,8 @@ static int do_efi_boot_rm(struct cmd_tbl *cmdtp, int flag, */ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) {
struct efi_device_path *initrd_path = NULL;
struct efi_device_path *fdt_path;
struct efi_device_path *initrd_path; struct efi_load_option lo; efi_status_t ret;
@@ -1038,6 +1087,12 @@ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) efi_free_pool(initrd_path); }
fdt_path = efi_dp_from_lo(&lo, &efi_guid_fdt);
if (fdt_path) {
printf(" device-tree path: %pD\n", fdt_path);
efi_free_pool(fdt_path);
}
printf(" data:\n"); print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, lo.optional_data, *size, true);
@@ -1565,8 +1620,9 @@ U_BOOT_LONGHELP(efidebug, "\n" "efidebug boot add - set UEFI BootXXXX variable\n" " -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n"
" -d|-D <interface> <devnum>[:<part>] <device-tree file path>\n" " -i|-I <interface> <devnum>[:<part>] <initrd file path>\n"
" (-b, -i for short form device path)\n"
" (-b, -d, -i for short form device path)\n"
#if (IS_ENABLED(CONFIG_EFI_HTTP_BOOT)) " -u <bootid> <label> <uri>\n"
#endif
2.43.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

We allow to specify the triple of binary, initrd, and device-tree in boot options.
Add the code to actually load the specified device-tree.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: Put all related changes into one patch. Do not boot if device-tree cannot be loaded. --- lib/efi_loader/efi_bootmgr.c | 70 ++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index b0bf21cf841..5ac116a7edf 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -1185,6 +1185,59 @@ out: return ret; }
+/** + * load_fdt_from_load_option - load device-tree from load option + * + * fdt: pointer to loaded device-tree or NULL + * Return: status code + */ +static efi_status_t load_fdt_from_load_option(void **fdt) +{ + struct efi_device_path *dp = NULL; + struct efi_file_handle *f = NULL; + efi_uintn_t filesize; + efi_status_t ret; + + *fdt = NULL; + + dp = efi_get_dp_from_boot(&efi_guid_fdt); + if (!dp) + return EFI_SUCCESS; + + /* Open file */ + f = efi_file_from_path(dp); + if (!f) { + log_err("Can't find %pD specified in Boot####\n", dp); + ret = EFI_NOT_FOUND; + goto out; + } + + /* Get file size */ + ret = efi_file_size(f, &filesize); + if (ret != EFI_SUCCESS) + goto out; + + *fdt = malloc(filesize); + if (!*fdt) { + log_err("Out of memory\n"); + ret = EFI_OUT_OF_RESOURCES; + goto out; + } + ret = EFI_CALL(f->read(f, &filesize, *fdt)); + if (ret != EFI_SUCCESS) { + log_err("Can't read fdt\n"); + free(*fdt); + *fdt = NULL; + } + +out: + efi_free_pool(dp); + if (f) + EFI_CALL(f->close(f)); + + return ret; +} + /** * efi_bootmgr_run() - execute EFI boot manager * @fdt: Flat device tree @@ -1200,6 +1253,7 @@ efi_status_t efi_bootmgr_run(void *fdt) efi_handle_t handle; void *load_options; efi_status_t ret; + void *fdt_lo;
/* Initialize EFI drivers */ ret = efi_init_obj_list(); @@ -1215,7 +1269,23 @@ efi_status_t efi_bootmgr_run(void *fdt) return ret; }
+ if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE)) { + ret = load_fdt_from_load_option(&fdt_lo); + if (ret != EFI_SUCCESS) + return ret; + if (fdt_lo) + fdt = fdt_lo; + } + + /* + * Needed in ACPI case to create reservations based on + * control device-tree. + */ ret = efi_install_fdt(fdt); + + if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE)) + free(fdt_lo); + if (ret != EFI_SUCCESS) { if (EFI_CALL(efi_unload_image(handle)) == EFI_SUCCESS) free(load_options);

[...]
*fdt = malloc(filesize);
Use calloc instead
if (!*fdt) {
log_err("Out of memory\n");
ret = EFI_OUT_OF_RESOURCES;
goto out;
}
ret = EFI_CALL(f->read(f, &filesize, *fdt));
if (ret != EFI_SUCCESS) {
log_err("Can't read fdt\n");
free(*fdt);
*fdt = NULL;
}
+out:
efi_free_pool(dp);
if (f)
EFI_CALL(f->close(f));
return ret;
+}
/**
- efi_bootmgr_run() - execute EFI boot manager
- @fdt: Flat device tree
@@ -1200,6 +1253,7 @@ efi_status_t efi_bootmgr_run(void *fdt) efi_handle_t handle; void *load_options; efi_status_t ret;
void *fdt_lo; /* Initialize EFI drivers */ ret = efi_init_obj_list();
@@ -1215,7 +1269,23 @@ efi_status_t efi_bootmgr_run(void *fdt) return ret; }
if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE)) {
ret = load_fdt_from_load_option(&fdt_lo);
if (ret != EFI_SUCCESS)
return ret;
if (fdt_lo)
fdt = fdt_lo;
}
/*
* Needed in ACPI case to create reservations based on
* control device-tree.
*/ ret = efi_install_fdt(fdt);
if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE))
free(fdt_lo);
if (ret != EFI_SUCCESS) { if (EFI_CALL(efi_unload_image(handle)) == EFI_SUCCESS) free(load_options);
-- 2.43.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

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 --- v2: rebase patch --- 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; - 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 1236eecff0f..1b4bc987a23 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -1199,4 +1199,6 @@ efi_status_t efi_load_option_dp_join(struct efi_device_path **dp, struct efi_device_path *initrd_dp, struct efi_device_path *fdt_dp);
+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; +}

On Tue, 28 May 2024 at 17:43, Heinrich Schuchardt heinrich.schuchardt@canonical.com 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
v2: rebase patch
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;
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 1236eecff0f..1b4bc987a23 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -1199,4 +1199,6 @@ efi_status_t efi_load_option_dp_join(struct efi_device_path **dp, struct efi_device_path *initrd_dp, struct efi_device_path *fdt_dp);
+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;
+}
2.43.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

For finding distro supplied device-trees we need to know from which device we are booting. This can be identified via the device-path of the binary.
Up to now efi_dp_from_lo() only could return the initrd or fdt device-path. Allow returning the binary device-path, too.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: no change --- lib/efi_loader/efi_device_path.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index c8c8d54f733..52e3313c23a 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -1133,17 +1133,18 @@ ssize_t efi_dp_check_length(const struct efi_device_path *dp, }
/** - * efi_dp_from_lo() - Get the instance of a VenMedia node in a - * multi-instance device path that matches - * a specific GUID. This kind of device paths - * is found in Boot#### options describing an - * initrd location + * efi_dp_from_lo() - get device-path from load option * - * @lo: EFI_LOAD_OPTION containing a valid device path - * @guid: guid to search for + * The load options in U-Boot may contain multiple concatenated device-paths. + * The first device-path indicates the EFI binary to execute. Subsequent + * device-paths start with a VenMedia node where the GUID identifies the + * function (initrd or fdt). + * + * @lo: EFI load option containing a valid device path + * @guid: GUID identifying device-path or NULL for the EFI binary * * Return: - * device path including the VenMedia node or NULL. + * device path excluding the matched VenMedia node or NULL. * Caller must free the returned value. */ struct @@ -1154,6 +1155,9 @@ efi_device_path *efi_dp_from_lo(struct efi_load_option *lo, struct efi_device_path_vendor *vendor; int lo_len = lo->file_path_length;
+ if (!guid) + return efi_dp_dup(fp); + for (; lo_len >= sizeof(struct efi_device_path); lo_len -= fp->length, fp = (void *)fp + fp->length) { if (lo_len < 0 || efi_dp_check_length(fp, lo_len) < 0)

On Tue, 28 May 2024 at 17:43, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
For finding distro supplied device-trees we need to know from which device we are booting. This can be identified via the device-path of the binary.
Up to now efi_dp_from_lo() only could return the initrd or fdt device-path. Allow returning the binary device-path, too.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: no change
lib/efi_loader/efi_device_path.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index c8c8d54f733..52e3313c23a 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -1133,17 +1133,18 @@ ssize_t efi_dp_check_length(const struct efi_device_path *dp, }
/**
- efi_dp_from_lo() - Get the instance of a VenMedia node in a
multi-instance device path that matches
a specific GUID. This kind of device paths
is found in Boot#### options describing an
initrd location
- efi_dp_from_lo() - get device-path from load option
- @lo: EFI_LOAD_OPTION containing a valid device path
- @guid: guid to search for
- The load options in U-Boot may contain multiple concatenated device-paths.
- The first device-path indicates the EFI binary to execute. Subsequent
- device-paths start with a VenMedia node where the GUID identifies the
- function (initrd or fdt).
- @lo: EFI load option containing a valid device path
- @guid: GUID identifying device-path or NULL for the EFI binary
- Return:
- device path including the VenMedia node or NULL.
- device path excluding the matched VenMedia node or NULL.
thanks for catching the typo as well
- Caller must free the returned value.
*/ struct @@ -1154,6 +1155,9 @@ efi_device_path *efi_dp_from_lo(struct efi_load_option *lo, struct efi_device_path_vendor *vendor; int lo_len = lo->file_path_length;
if (!guid)
return efi_dp_dup(fp);
for (; lo_len >= sizeof(struct efi_device_path); lo_len -= fp->length, fp = (void *)fp + fp->length) { if (lo_len < 0 || efi_dp_check_length(fp, lo_len) < 0)
-- 2.43.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

We can reuse this function to load the device-tree.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: Move unrelated changes to different patch. --- include/efi_loader.h | 4 ++++ lib/efi_loader/efi_boottime.c | 1 - 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 1b4bc987a23..ab7bed22971 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -664,6 +664,10 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy, void *source_buffer, efi_uintn_t source_size, efi_handle_t *image_handle); +/* Load image from path */ +efi_status_t efi_load_image_from_path(bool boot_policy, + struct efi_device_path *file_path, + void **buffer, efi_uintn_t *size); /* Start image */ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, efi_uintn_t *exit_data_size, diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 630c5f52c4f..eedc5f39549 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1996,7 +1996,6 @@ error: * @size: size of the loaded image * Return: status code */ -static efi_status_t efi_load_image_from_path(bool boot_policy, struct efi_device_path *file_path, void **buffer, efi_uintn_t *size)

On Tue, 28 May 2024 at 17:43, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
We can reuse this function to load the device-tree.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: Move unrelated changes to different patch.
include/efi_loader.h | 4 ++++ lib/efi_loader/efi_boottime.c | 1 - 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 1b4bc987a23..ab7bed22971 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -664,6 +664,10 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy, void *source_buffer, efi_uintn_t source_size, efi_handle_t *image_handle); +/* Load image from path */ +efi_status_t efi_load_image_from_path(bool boot_policy,
struct efi_device_path *file_path,
void **buffer, efi_uintn_t *size);
/* Start image */ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, efi_uintn_t *exit_data_size, diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 630c5f52c4f..eedc5f39549 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1996,7 +1996,6 @@ error:
- @size: size of the loaded image
- Return: status code
*/ -static efi_status_t efi_load_image_from_path(bool boot_policy, struct efi_device_path *file_path, void **buffer, efi_uintn_t *size) -- 2.43.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

If no device-tree is specified, try to load a device-tree from the boot device use the $fdtfile concatenated to either of the paths '/dtb/', '/', '/dtb/current/'.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: no change --- include/efi_loader.h | 2 ++ lib/efi_loader/efi_bootmgr.c | 13 +++++++++-- lib/efi_loader/efi_fdt.c | 44 ++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 2 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index ab7bed22971..6c993e1a694 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -1205,4 +1205,6 @@ efi_status_t efi_load_option_dp_join(struct efi_device_path **dp,
int efi_get_distro_fdt_name(char *fname, int size, int seq);
+void efi_load_distro_fdt(void **fdt, efi_uintn_t *fdt_size); + #endif /* _EFI_LOADER_H */ diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index b08d6e97ea3..68440542a37 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -1249,7 +1249,8 @@ efi_status_t efi_bootmgr_run(void *fdt) efi_handle_t handle; void *load_options; efi_status_t ret; - void *fdt_lo; + void *fdt_lo, *fdt_distro = NULL; + efi_uintn_t fdt_size;
/* Initialize EFI drivers */ ret = efi_init_obj_list(); @@ -1269,6 +1270,10 @@ efi_status_t efi_bootmgr_run(void *fdt) return ret; if (fdt_lo) fdt = fdt_lo; + if (!fdt) { + efi_load_distro_fdt(&fdt_distro, &fdt_size); + fdt = fdt_distro; + } }
/* @@ -1277,8 +1282,12 @@ efi_status_t efi_bootmgr_run(void *fdt) */ ret = efi_install_fdt(fdt);
- if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE)) + if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE)) { free(fdt_lo); + if (fdt_distro) + efi_free_pages((uintptr_t)fdt_distro, + efi_size_in_pages(fdt_size)); + }
if (ret != EFI_SUCCESS) { if (EFI_CALL(efi_unload_image(handle)) == EFI_SUCCESS) diff --git a/lib/efi_loader/efi_fdt.c b/lib/efi_loader/efi_fdt.c index 0edf0c1e2fc..86ba00c2bdd 100644 --- a/lib/efi_loader/efi_fdt.c +++ b/lib/efi_loader/efi_fdt.c @@ -71,3 +71,47 @@ int efi_get_distro_fdt_name(char *fname, int size, int seq)
return 0; } + +/** + * efi_load_distro_fdt() - load distro device-tree + * + * @fdt: on return device-tree, must be freed via efi_free_pages() + * @fdt_size: buffer size + */ +void efi_load_distro_fdt(void **fdt, efi_uintn_t *fdt_size) +{ + struct efi_device_path *rem, *dp; + efi_status_t ret; + efi_handle_t device; + + *fdt = NULL; + + dp = efi_get_dp_from_boot(NULL); + if (!dp) + return; + device = efi_dp_find_obj(dp, NULL, &rem); + ret = efi_search_protocol(device, &efi_simple_file_system_protocol_guid, + NULL); + if (ret != EFI_SUCCESS) + goto err; + memcpy(rem, &END, sizeof(END)); + + /* try the various available names */ + for (int seq = 0; ; ++seq) { + struct efi_device_path *file; + char buf[255]; + + if (efi_get_distro_fdt_name(buf, sizeof(buf), seq)) + break; + file = efi_dp_from_file(dp, buf); + if (!file) + break; + ret = efi_load_image_from_path(true, file, fdt, fdt_size); + efi_free_pool(file); + if (ret == EFI_SUCCESS) + break; + } + +err: + efi_free_pool(dp); +}

On Tue, 28 May 2024 at 17:43, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
If no device-tree is specified, try to load a device-tree from the boot device use the $fdtfile concatenated to either of the paths '/dtb/', '/', '/dtb/current/'.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: no change
include/efi_loader.h | 2 ++ lib/efi_loader/efi_bootmgr.c | 13 +++++++++-- lib/efi_loader/efi_fdt.c | 44 ++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 2 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index ab7bed22971..6c993e1a694 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -1205,4 +1205,6 @@ efi_status_t efi_load_option_dp_join(struct efi_device_path **dp,
int efi_get_distro_fdt_name(char *fname, int size, int seq);
+void efi_load_distro_fdt(void **fdt, efi_uintn_t *fdt_size);
#endif /* _EFI_LOADER_H */ diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index b08d6e97ea3..68440542a37 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -1249,7 +1249,8 @@ efi_status_t efi_bootmgr_run(void *fdt) efi_handle_t handle; void *load_options; efi_status_t ret;
void *fdt_lo;
void *fdt_lo, *fdt_distro = NULL;
efi_uintn_t fdt_size; /* Initialize EFI drivers */ ret = efi_init_obj_list();
@@ -1269,6 +1270,10 @@ efi_status_t efi_bootmgr_run(void *fdt) return ret; if (fdt_lo) fdt = fdt_lo;
if (!fdt) {
efi_load_distro_fdt(&fdt_distro, &fdt_size);
fdt = fdt_distro;
} } /*
@@ -1277,8 +1282,12 @@ efi_status_t efi_bootmgr_run(void *fdt) */ ret = efi_install_fdt(fdt);
if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE))
if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE)) { free(fdt_lo);
if (fdt_distro)
efi_free_pages((uintptr_t)fdt_distro,
efi_size_in_pages(fdt_size));
} if (ret != EFI_SUCCESS) { if (EFI_CALL(efi_unload_image(handle)) == EFI_SUCCESS)
diff --git a/lib/efi_loader/efi_fdt.c b/lib/efi_loader/efi_fdt.c index 0edf0c1e2fc..86ba00c2bdd 100644 --- a/lib/efi_loader/efi_fdt.c +++ b/lib/efi_loader/efi_fdt.c @@ -71,3 +71,47 @@ int efi_get_distro_fdt_name(char *fname, int size, int seq)
return 0;
}
+/**
- efi_load_distro_fdt() - load distro device-tree
- @fdt: on return device-tree, must be freed via efi_free_pages()
- @fdt_size: buffer size
- */
+void efi_load_distro_fdt(void **fdt, efi_uintn_t *fdt_size) +{
struct efi_device_path *rem, *dp;
efi_status_t ret;
efi_handle_t device;
*fdt = NULL;
dp = efi_get_dp_from_boot(NULL);
if (!dp)
return;
device = efi_dp_find_obj(dp, NULL, &rem);
ret = efi_search_protocol(device, &efi_simple_file_system_protocol_guid,
NULL);
if (ret != EFI_SUCCESS)
goto err;
memcpy(rem, &END, sizeof(END));
/* try the various available names */
for (int seq = 0; ; ++seq) {
struct efi_device_path *file;
char buf[255];
if (efi_get_distro_fdt_name(buf, sizeof(buf), seq))
break;
file = efi_dp_from_file(dp, buf);
if (!file)
break;
ret = efi_load_image_from_path(true, file, fdt, fdt_size);
efi_free_pool(file);
if (ret == EFI_SUCCESS)
break;
}
+err:
efi_free_pool(dp);
+}
2.43.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

Hi,
On Tue, May 28, 2024 at 7:43 AM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
In U-Boot EFI boot options can already specify both an EFI binary and an initrd. With this series we can additionally define the matching device-tree to be loaded in the boot option.
With the last patch the boot manager will fall back the device-tree specified by $fdtfile in directories '/dtb/', '/', or '/dtb/current/' on the boot device if no device-tree is specified in the boot option or via a bootefi command parameter.
As tested the $fdtfile environment variable has no effect on global EFI boot when i.e. EFI/BOOT/BOOTRISCV64.EFI on EFI System Partition and no user-added boot option; $fdtfile env variable is not used with "mmc 0" or whichever global boot option is enabled by default in the boot order.
Adding a boot option for EFI/BOOT/BOOTRISCV64.EFI and giving this priority in the boot order allows $fdtfile to be effective here. This is consistent with what is described by the series. Would the global EFI boot also get support for $fdtfile either with this or a later series?
v2: Update efi_dp_concat() instead of new function efi_dp_merge(). Carve out a function efi_load_option_dp_join() which we can use both for the eficonfig and the efidebug command. Rename variables id_dp, final_dp_size. Rename create_initrd_dp() to create_lo_dp_part(). Use enum as parameter for create_lo_dp_part(). Put all related changes into one patch.
Heinrich Schuchardt (8): efi_loader: allow concatenation with contained end node cmd: eficonfig: add support for setting fdt cmd: efidebug: add support for setting fdt efi_loader: load device-tree specified in boot option efi_loader: move distro_efi_get_fdt_name() efi_loader: return binary from efi_dp_from_lo() efi_loader: export efi_load_image_from_path efi_loader: load distro dtb in bootmgr
boot/bootmeth_efi.c | 60 +--------- cmd/eficonfig.c | 83 +++++++++---- cmd/efidebug.c | 130 +++++++++++++++------ include/efi_loader.h | 24 +++- lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_bootbin.c | 2 +- lib/efi_loader/efi_bootmgr.c | 75 +++++++++++- lib/efi_loader/efi_boottime.c | 3 +- lib/efi_loader/efi_device_path.c | 40 ++++--- lib/efi_loader/efi_device_path_utilities.c | 2 +- lib/efi_loader/efi_fdt.c | 117 +++++++++++++++++++ lib/efi_loader/efi_helper.c | 44 +++++++ 12 files changed, 445 insertions(+), 136 deletions(-) create mode 100644 lib/efi_loader/efi_fdt.c
-- 2.43.0
Tested-by: E Shattow lucent@gmail.com

Hi,
On Tue, 28 May 2024 at 18:38, E Shattow lucent@gmail.com wrote:
Hi,
On Tue, May 28, 2024 at 7:43 AM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
In U-Boot EFI boot options can already specify both an EFI binary and an initrd. With this series we can additionally define the matching device-tree to be loaded in the boot option.
With the last patch the boot manager will fall back the device-tree specified by $fdtfile in directories '/dtb/', '/', or '/dtb/current/' on the boot device if no device-tree is specified in the boot option or via a bootefi command parameter.
As tested the $fdtfile environment variable has no effect on global EFI boot when i.e. EFI/BOOT/BOOTRISCV64.EFI on EFI System Partition and no user-added boot option; $fdtfile env variable is not used with "mmc 0" or whichever global boot option is enabled by default in the boot order.
Adding a boot option for EFI/BOOT/BOOTRISCV64.EFI and giving this priority in the boot order allows $fdtfile to be effective here. This is consistent with what is described by the series. Would the global EFI boot also get support for $fdtfile either with this or a later series?
v2: Update efi_dp_concat() instead of new function efi_dp_merge(). Carve out a function efi_load_option_dp_join() which we can use both for the eficonfig and the efidebug command. Rename variables id_dp, final_dp_size. Rename create_initrd_dp() to create_lo_dp_part(). Use enum as parameter for create_lo_dp_part(). Put all related changes into one patch.
Heinrich Schuchardt (8): efi_loader: allow concatenation with contained end node cmd: eficonfig: add support for setting fdt cmd: efidebug: add support for setting fdt efi_loader: load device-tree specified in boot option efi_loader: move distro_efi_get_fdt_name() efi_loader: return binary from efi_dp_from_lo() efi_loader: export efi_load_image_from_path efi_loader: load distro dtb in bootmgr
boot/bootmeth_efi.c | 60 +--------- cmd/eficonfig.c | 83 +++++++++---- cmd/efidebug.c | 130 +++++++++++++++------ include/efi_loader.h | 24 +++- lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_bootbin.c | 2 +- lib/efi_loader/efi_bootmgr.c | 75 +++++++++++- lib/efi_loader/efi_boottime.c | 3 +- lib/efi_loader/efi_device_path.c | 40 ++++--- lib/efi_loader/efi_device_path_utilities.c | 2 +- lib/efi_loader/efi_fdt.c | 117 +++++++++++++++++++ lib/efi_loader/efi_helper.c | 44 +++++++ 12 files changed, 445 insertions(+), 136 deletions(-) create mode 100644 lib/efi_loader/efi_fdt.c
-- 2.43.0
Can we use the best-match compatible approach as expected by the new 'make image.fit' in Linux?
Filenames should be deprecated IMO. I am happy to help work on how to do that if you agree.
Regards, Simon
Tested-by: E Shattow lucent@gmail.com

On 29.05.24 18:30, Simon Glass wrote:
Hi,
On Tue, 28 May 2024 at 18:38, E Shattow lucent@gmail.com wrote:
Hi,
On Tue, May 28, 2024 at 7:43 AM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
In U-Boot EFI boot options can already specify both an EFI binary and an initrd. With this series we can additionally define the matching device-tree to be loaded in the boot option.
With the last patch the boot manager will fall back the device-tree specified by $fdtfile in directories '/dtb/', '/', or '/dtb/current/' on the boot device if no device-tree is specified in the boot option or via a bootefi command parameter.
As tested the $fdtfile environment variable has no effect on global EFI boot when i.e. EFI/BOOT/BOOTRISCV64.EFI on EFI System Partition and no user-added boot option; $fdtfile env variable is not used with "mmc 0" or whichever global boot option is enabled by default in the boot order.
Adding a boot option for EFI/BOOT/BOOTRISCV64.EFI and giving this priority in the boot order allows $fdtfile to be effective here. This is consistent with what is described by the series. Would the global EFI boot also get support for $fdtfile either with this or a later series?
v2: Update efi_dp_concat() instead of new function efi_dp_merge(). Carve out a function efi_load_option_dp_join() which we can use both for the eficonfig and the efidebug command. Rename variables id_dp, final_dp_size. Rename create_initrd_dp() to create_lo_dp_part(). Use enum as parameter for create_lo_dp_part(). Put all related changes into one patch.
Heinrich Schuchardt (8): efi_loader: allow concatenation with contained end node cmd: eficonfig: add support for setting fdt cmd: efidebug: add support for setting fdt efi_loader: load device-tree specified in boot option efi_loader: move distro_efi_get_fdt_name() efi_loader: return binary from efi_dp_from_lo() efi_loader: export efi_load_image_from_path efi_loader: load distro dtb in bootmgr
boot/bootmeth_efi.c | 60 +--------- cmd/eficonfig.c | 83 +++++++++---- cmd/efidebug.c | 130 +++++++++++++++------ include/efi_loader.h | 24 +++- lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_bootbin.c | 2 +- lib/efi_loader/efi_bootmgr.c | 75 +++++++++++- lib/efi_loader/efi_boottime.c | 3 +- lib/efi_loader/efi_device_path.c | 40 ++++--- lib/efi_loader/efi_device_path_utilities.c | 2 +- lib/efi_loader/efi_fdt.c | 117 +++++++++++++++++++ lib/efi_loader/efi_helper.c | 44 +++++++ 12 files changed, 445 insertions(+), 136 deletions(-) create mode 100644 lib/efi_loader/efi_fdt.c
-- 2.43.0
Can we use the best-match compatible approach as expected by the new 'make image.fit' in Linux?
Filenames should be deprecated IMO. I am happy to help work on how to do that if you agree.
Hello Simon,
It is the OS that creates boot options. The OS can determine the exact dtb file based on the compatible string and the kernel version once per kernel upgrade. This is much more efficient than doing the same on every boot.
Replacing $fdtfile by a matching logic could make sense. But please consider the effect on boot time if have to read through more than 1000 arm64 dtbs with U-Boot's non-caching file-system drivers.
Best regards
Heinrich

Hi Heinrich,
On Wed, 5 Jun 2024 at 02:40, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 29.05.24 18:30, Simon Glass wrote:
Hi,
On Tue, 28 May 2024 at 18:38, E Shattow lucent@gmail.com wrote:
Hi,
On Tue, May 28, 2024 at 7:43 AM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
In U-Boot EFI boot options can already specify both an EFI binary and an initrd. With this series we can additionally define the matching device-tree to be loaded in the boot option.
With the last patch the boot manager will fall back the device-tree specified by $fdtfile in directories '/dtb/', '/', or '/dtb/current/' on the boot device if no device-tree is specified in the boot option or via a bootefi command parameter.
As tested the $fdtfile environment variable has no effect on global EFI boot when i.e. EFI/BOOT/BOOTRISCV64.EFI on EFI System Partition and no user-added boot option; $fdtfile env variable is not used with "mmc 0" or whichever global boot option is enabled by default in the boot order.
Adding a boot option for EFI/BOOT/BOOTRISCV64.EFI and giving this priority in the boot order allows $fdtfile to be effective here. This is consistent with what is described by the series. Would the global EFI boot also get support for $fdtfile either with this or a later series?
v2: Update efi_dp_concat() instead of new function efi_dp_merge(). Carve out a function efi_load_option_dp_join() which we can use both for the eficonfig and the efidebug command. Rename variables id_dp, final_dp_size. Rename create_initrd_dp() to create_lo_dp_part(). Use enum as parameter for create_lo_dp_part(). Put all related changes into one patch.
Heinrich Schuchardt (8): efi_loader: allow concatenation with contained end node cmd: eficonfig: add support for setting fdt cmd: efidebug: add support for setting fdt efi_loader: load device-tree specified in boot option efi_loader: move distro_efi_get_fdt_name() efi_loader: return binary from efi_dp_from_lo() efi_loader: export efi_load_image_from_path efi_loader: load distro dtb in bootmgr
boot/bootmeth_efi.c | 60 +--------- cmd/eficonfig.c | 83 +++++++++---- cmd/efidebug.c | 130 +++++++++++++++------ include/efi_loader.h | 24 +++- lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_bootbin.c | 2 +- lib/efi_loader/efi_bootmgr.c | 75 +++++++++++- lib/efi_loader/efi_boottime.c | 3 +- lib/efi_loader/efi_device_path.c | 40 ++++--- lib/efi_loader/efi_device_path_utilities.c | 2 +- lib/efi_loader/efi_fdt.c | 117 +++++++++++++++++++ lib/efi_loader/efi_helper.c | 44 +++++++ 12 files changed, 445 insertions(+), 136 deletions(-) create mode 100644 lib/efi_loader/efi_fdt.c
-- 2.43.0
Can we use the best-match compatible approach as expected by the new 'make image.fit' in Linux?
Filenames should be deprecated IMO. I am happy to help work on how to do that if you agree.
Hello Simon,
It is the OS that creates boot options. The OS can determine the exact dtb file based on the compatible string and the kernel version once per kernel upgrade. This is much more efficient than doing the same on every boot.
Replacing $fdtfile by a matching logic could make sense. But please consider the effect on boot time if have to read through more than 1000 arm64 dtbs with U-Boot's non-caching file-system drivers.
The suggested option here is to use FIT, which is very fast at scanning the files. Please see [1]
Failing that, we could implement a way (in FIT) of specifying that the FDT is in an external file. In that case FIT would become a mapping from compatible strings to filenames.
Regards, Simon
[1] https://github.com/open-source-firmware/flat-image-tree/blob/main/source/cha...

On 05.06.24 15:17, Simon Glass wrote:
Hi Heinrich,
On Wed, 5 Jun 2024 at 02:40, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 29.05.24 18:30, Simon Glass wrote:
Hi,
On Tue, 28 May 2024 at 18:38, E Shattow lucent@gmail.com wrote:
Hi,
On Tue, May 28, 2024 at 7:43 AM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
In U-Boot EFI boot options can already specify both an EFI binary and an initrd. With this series we can additionally define the matching device-tree to be loaded in the boot option.
With the last patch the boot manager will fall back the device-tree specified by $fdtfile in directories '/dtb/', '/', or '/dtb/current/' on the boot device if no device-tree is specified in the boot option or via a bootefi command parameter.
As tested the $fdtfile environment variable has no effect on global EFI boot when i.e. EFI/BOOT/BOOTRISCV64.EFI on EFI System Partition and no user-added boot option; $fdtfile env variable is not used with "mmc 0" or whichever global boot option is enabled by default in the boot order.
Adding a boot option for EFI/BOOT/BOOTRISCV64.EFI and giving this priority in the boot order allows $fdtfile to be effective here. This is consistent with what is described by the series. Would the global EFI boot also get support for $fdtfile either with this or a later series?
v2: Update efi_dp_concat() instead of new function efi_dp_merge(). Carve out a function efi_load_option_dp_join() which we can use both for the eficonfig and the efidebug command. Rename variables id_dp, final_dp_size. Rename create_initrd_dp() to create_lo_dp_part(). Use enum as parameter for create_lo_dp_part(). Put all related changes into one patch.
Heinrich Schuchardt (8): efi_loader: allow concatenation with contained end node cmd: eficonfig: add support for setting fdt cmd: efidebug: add support for setting fdt efi_loader: load device-tree specified in boot option efi_loader: move distro_efi_get_fdt_name() efi_loader: return binary from efi_dp_from_lo() efi_loader: export efi_load_image_from_path efi_loader: load distro dtb in bootmgr
boot/bootmeth_efi.c | 60 +--------- cmd/eficonfig.c | 83 +++++++++---- cmd/efidebug.c | 130 +++++++++++++++------ include/efi_loader.h | 24 +++- lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_bootbin.c | 2 +- lib/efi_loader/efi_bootmgr.c | 75 +++++++++++- lib/efi_loader/efi_boottime.c | 3 +- lib/efi_loader/efi_device_path.c | 40 ++++--- lib/efi_loader/efi_device_path_utilities.c | 2 +- lib/efi_loader/efi_fdt.c | 117 +++++++++++++++++++ lib/efi_loader/efi_helper.c | 44 +++++++ 12 files changed, 445 insertions(+), 136 deletions(-) create mode 100644 lib/efi_loader/efi_fdt.c
-- 2.43.0
Can we use the best-match compatible approach as expected by the new 'make image.fit' in Linux?
Filenames should be deprecated IMO. I am happy to help work on how to do that if you agree.
Hello Simon,
It is the OS that creates boot options. The OS can determine the exact dtb file based on the compatible string and the kernel version once per kernel upgrade. This is much more efficient than doing the same on every boot.
Replacing $fdtfile by a matching logic could make sense. But please consider the effect on boot time if have to read through more than 1000 arm64 dtbs with U-Boot's non-caching file-system drivers.
The suggested option here is to use FIT, which is very fast at scanning the files. Please see [1]
Failing that, we could implement a way (in FIT) of specifying that the FDT is in an external file. In that case FIT would become a mapping from compatible strings to filenames.
FIT has no place in UEFI.
Best regards
Heinrich

Hi Simon,
[...]
Can we use the best-match compatible approach as expected by the new 'make image.fit' in Linux?
Filenames should be deprecated IMO. I am happy to help work on how to do that if you agree.
Hello Simon,
It is the OS that creates boot options. The OS can determine the exact dtb file based on the compatible string and the kernel version once per kernel upgrade. This is much more efficient than doing the same on every boot.
Replacing $fdtfile by a matching logic could make sense. But please consider the effect on boot time if have to read through more than 1000 arm64 dtbs with U-Boot's non-caching file-system drivers.
The suggested option here is to use FIT, which is very fast at scanning the files. Please see [1]
Failing that, we could implement a way (in FIT) of specifying that the FDT is in an external file. In that case FIT would become a mapping from compatible strings to filenames.
This is kind of irrelevant to FIT. This is how the efibootmgr configures the files it has to load. commit 76e8acce12fe, commit 53f6a5aa8626, and doc/develop/uefi/uefi.rst, Chapter Load file 2 protocol has enough information of how the initrd is implemented. What Heinrich is doing here is extending the existing code to load a DT
Regards /Ilias
Regards, Simon
[1] https://github.com/open-source-firmware/flat-image-tree/blob/main/source/cha...
participants (4)
-
E Shattow
-
Heinrich Schuchardt
-
Ilias Apalodimas
-
Simon Glass