Hi Heinrich
On Fri, Apr 26, 2024 at 04:13:12PM +0200, Heinrich Schuchardt 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.schucha...@canonical.com>
> ---
> cmd/efidebug.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 63 insertions(+), 5 deletions(-)
>
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 93ba16efc7d..32c64711b6c 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -660,16 +660,33 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp,
> int flag,
> * @part: disk partition
> * @file: filename
> * @shortform: create short form device path
> + * @fdt: create path for device-tree
> * 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)
> + const char *file, bool shortform,
> + bool fdt)
Overall the patch looks correct. Can we pick a better name instead of
create_initrd_dp() & efi_initrd_dp? Do you have in mind any extra uses
cases apart from the initrd and dtb? If yes, then we could convert the i
'bool fdt' to an enum.
Since all these are encoded in the load option perhaps we can rename it to
create_lo_dp and efi_lo_dp?
>
> {
> struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL, *short_fp = NULL;
> struct efi_device_path *initrd_dp = NULL;
> efi_status_t ret;
> + 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 id_dp = {
> .vendor = {
> {
> @@ -696,7 +713,8 @@ 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,
> + initrd_dp = efi_dp_concat((const struct efi_device_path *)
> + (fdt ? &fdt_dp : &id_dp),
> short_fp);
>
> out:
> @@ -796,6 +814,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int
> flag,
> 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;
> @@ -860,6 +879,27 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int
> flag,
> 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_initrd_dp(argv[1], argv[2], argv[3],
> + shortform, true);
> + if (!fdt_dp) {
> + printf("Cannot add a device-tree\n");
> + r = CMD_RET_FAILURE;
> + goto out;
> + }
> + argc -= 3;
> + argv += 3;
> + fp_size += efi_dp_size(fdt_dp) +
> + sizeof(struct efi_device_path);
> + break;
> case 'i':
> shortform = 1;
> /* fallthrough */
> @@ -870,7 +910,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int
> flag,
> }
>
> initrd_dp = create_initrd_dp(argv[1], argv[2], argv[3],
> - shortform);
> + shortform, false);
> if (!initrd_dp) {
> printf("Cannot add an initrd\n");
> r = CMD_RET_FAILURE;
> @@ -929,6 +969,15 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int
> flag,
> }
> }
>
> + if (fdt_dp) {
> + final_fp = efi_dp_merge(final_fp, &fp_size, fdt_dp);
> + if (!final_fp) {
> + printf("Cannot create final device path\n");
> + r = CMD_RET_FAILURE;
> + goto out;
> + }
> + }
> +
> lo.file_path = final_fp;
> lo.file_path_length = fp_size;
>
> @@ -952,6 +1001,7 @@ out:
> free(data);
> efi_free_pool(final_fp);
> efi_free_pool(initrd_dp);
> + efi_free_pool(fdt_dp);
> efi_free_pool(fp_free);
> free(lo.label);
>
> @@ -1014,7 +1064,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;
>
> @@ -1043,6 +1094,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);
> @@ -1570,8 +1627,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
>
Regards
/Ilias
