Re: [RFC 05/14] cmd: efidebug: add support for setting fdt

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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