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

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