From mboxrd@z Thu Jan  1 00:00:00 1970
From: AKASHI Takahiro <takahiro.akashi@linaro.org>
Date: Thu, 14 Jan 2021 14:11:33 +0900
Subject: [RFC PATCH 3/3] efidebug: add multiple device path instances on
 Boot####
In-Reply-To: <20210113111149.64567-4-ilias.apalodimas@linaro.org>
References: <20210113111149.64567-1-ilias.apalodimas@linaro.org>
 <20210113111149.64567-4-ilias.apalodimas@linaro.org>
Message-ID: <20210114051133.GC25252@laputa>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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
>