[RFC PATCH 3/3] efidebug: add multiple device path instances on Boot####
AKASHI Takahiro
takahiro.akashi at linaro.org
Thu Jan 14 06:11:33 CET 2021
Ilias,
On Wed, Jan 13, 2021 at 01:11:49PM +0200, Ilias Apalodimas wrote:
> The UEFI spec allow a packed array of UEFI device paths in the
> FilePathList[] of an EFI_LOAD_OPTION. The first file path must
> describe the laoded image but the rest are OS specific.
> Previous patches parse the device path and try to use the second
> member of the array as an initrd. So let's modify efidebug slightly
> and install the second file described in the command line as the
> initrd device path.
I have a concern about your proposed command line syntax.
It takes a lot of parameters as a whole which makes it
hard to understand it at a glance, easily overflowing
the width of terminal window.
It will even get worse if we want to take dtb as a third
device path, and what if we want to specify dtb, but not initrd?
Moreover, if we want to add support for non-linux executabes which
utilize extra device paths (neither initrd nor dtb) in a boot
load option, the syntax will be problematic.
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> ---
> cmd/efidebug.c | 89 +++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 81 insertions(+), 8 deletions(-)
>
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 5fb7b1e3c6a9..8d62981aca92 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -8,6 +8,7 @@
> #include <charset.h>
> #include <common.h>
> #include <command.h>
> +#include <efi_helper.h>
> #include <efi_loader.h>
> #include <efi_rng.h>
> #include <exports.h>
> @@ -17,6 +18,7 @@
> #include <mapmem.h>
> #include <search.h>
> #include <linux/ctype.h>
> +#include <linux/err.h>
>
> #define BS systab.boottime
> #define RT systab.runtime
> @@ -782,6 +784,42 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag,
> return CMD_RET_SUCCESS;
> }
>
> +/**
> + * add_initrd_instance() - Append a device path to load_options pointing to an
> + * inirtd
> + *
> + * @argc: Number of arguments
> + * @argv: Argument array
> + * @file_path Existing device path, the new instance will be appended
> + * Return: Pointer to the device path or ERR_PTR
> + *
> + */
> +static struct efi_device_path *add_initrd_instance(int argc, char *const argv[],
> + struct efi_device_path *file_path)
> +{
> + struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL;
> + struct efi_device_path *final_fp = NULL;
> + efi_status_t ret;
> +
> + if (argc < 8)
> + return ERR_PTR(-EINVAL);
> +
> + ret = efi_dp_from_name(argv[6], argv[7], argv[8], &tmp_dp,
> + &tmp_fp);
> + if (ret != EFI_SUCCESS) {
> + printf("Cannot create device path for \"%s %s\"\n",
> + argv[6], argv[7]);
> + goto out;
> + }
> +
> + final_fp = efi_dp_append_instance(file_path, tmp_fp);
> +
> +out:
> + efi_free_pool(tmp_dp);
> + efi_free_pool(tmp_fp);
> + return final_fp ? final_fp : ERR_PTR(-EINVAL);
> +}
> +
> /**
> * do_efi_boot_add() - set UEFI load option
> *
> @@ -794,7 +832,11 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag,
> *
> * Implement efidebug "boot add" sub-command. Create or change UEFI load option.
> *
> - * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options>
> + * Without initrd:
> + * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options>
> + *
> + * With initrd:
> + * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <interface2> <devnum2>[:<part>] <initrd> <options>
> */
> static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
> int argc, char *const argv[])
> @@ -807,13 +849,14 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
> size_t label_len, label_len16;
> u16 *label;
> struct efi_device_path *device_path = NULL, *file_path = NULL;
> + struct efi_device_path *final_fp = NULL;
> struct efi_load_option lo;
> void *data = NULL;
> efi_uintn_t size;
> efi_status_t ret;
> int r = CMD_RET_SUCCESS;
>
> - if (argc < 6 || argc > 7)
> + if (argc < 6 || argc > 9)
> return CMD_RET_USAGE;
>
> id = (int)simple_strtoul(argv[1], &endp, 16);
> @@ -829,6 +872,12 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
> /* attributes */
> lo.attributes = LOAD_OPTION_ACTIVE; /* always ACTIVE */
>
> + /* optional data */
> + if (argc == 6)
> + lo.optional_data = NULL;
> + else
> + lo.optional_data = (const u8 *)argv[6];
> +
> /* label */
> label_len = strlen(argv[2]);
> label_len16 = utf8_utf16_strnlen(argv[2], label_len);
> @@ -847,15 +896,30 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
> r = CMD_RET_FAILURE;
> goto out;
> }
> +
> + /* add initrd instance in device path */
> + if (argc >= 9) {
> + final_fp = add_initrd_instance(argc, argv, file_path);
We'd better pass argv[6], argv[7] and argv[8] explicitly here,
which will make the code readable and easily extended to support
dtb in the future.
then,
argc -= 3;
argv += 3;
> + if (IS_ERR(final_fp)) {
> + r = CMD_RET_FAILURE;
> + goto out;
> + }
> +
> + /* add_initrd_instance allocates a new device path */
> + efi_free_pool(file_path);
> + file_path = final_fp;
> +
> + /* update optional data */
Then, the code should be moved out of this 'if' clause.
> + if (argc == 9)
> + lo.optional_data = NULL;
> + else
> + lo.optional_data = (const u8 *)argv[9];
> + }
> +
> lo.file_path = file_path;
> lo.file_path_length = efi_dp_size(file_path)
> + sizeof(struct efi_device_path); /* for END */
>
> - /* optional data */
> - if (argc == 6)
> - lo.optional_data = NULL;
> - else
> - lo.optional_data = (const u8 *)argv[6];
The code should remain here for non-initrd case.
-Takahiro Akashi
>
> size = efi_serialize_load_option(&lo, (u8 **)&data);
> if (!size) {
> @@ -939,11 +1003,13 @@ 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_load_option lo;
> char *label, *p;
> size_t label_len16, label_len;
> u16 *dp_str;
> efi_status_t ret;
> + efi_uintn_t initrd_dp_size;
>
> ret = efi_deserialize_load_option(&lo, data, size);
> if (ret != EFI_SUCCESS) {
> @@ -973,6 +1039,13 @@ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size)
> dp_str = efi_dp_str(lo.file_path);
> printf(" file_path: %ls\n", dp_str);
> efi_free_pool(dp_str);
> + initrd_path = efi_dp_instance_by_idx(lo.file_path, &initrd_dp_size, INITRD_DP);
> + if (initrd_path) {
> + dp_str = efi_dp_str(initrd_path);
> + printf(" initrd_path: %ls\n", dp_str);
> + efi_free_pool(dp_str);
> + efi_free_pool(initrd_path);
> + }
>
> printf(" data:\n");
> print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1,
> @@ -1583,7 +1656,7 @@ static char efidebug_help_text[] =
> #endif
>
> U_BOOT_CMD(
> - efidebug, 10, 0, do_efidebug,
> + efidebug, CONFIG_SYS_MAXARGS, 0, do_efidebug,
> "Configure UEFI environment",
> efidebug_help_text
> );
> --
> 2.30.0.rc2
>
More information about the U-Boot
mailing list