Re: [RFC PATCH 3/3] efidebug: add multiple device path instances on Boot####

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@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