[RFC PATCH v3 0/2] enable menu-driven boot device selection

[RFC PATCH v3 0/2] enable menu-driven boot device selection

[Masahisa Kojima]

This patch series adds the menu-driven boot device selection,
by extending the existing "bootmenu" to include UEFI and distro_boot
related entries.

The menu example shown with this patch series is as follows.


  *** U-Boot Boot Menu ***

     Boot 1. kernel (bootmenu_0)
     Boot 2. kernel (bootmenu_1)
     Reset board (bootmenu_2)
     debian (BOOT0000)
     ubuntu (BOOT0001)
     UEFI Boot Manager
     usb0
     scsi0
     virtio0
     dhcp
     UEFI Boot Manager Maintenance
     U-Boot console

  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit


This example shows three "bootmenu_0..2" entries, UEFI "Boot0000" and "Boot0001"
entries, distro_boot "boot_targets" entries(usb0, scici0, virtio0 and dhcp)
and some pre-defined entries(UEFI Boot Manager, UEFI Boot Manager Maintenance
and U-Boot console).

Note that this patch series aims to propose the above menu structure,
the code quality is very low and requires much cleanup/refactoring.


[How to run on QEMU(arm64)]
1) clone source code
 $ git clone https://git.linaro.org/people/masahisa.kojima/u-boot.git \
-b kojima/efibootmenu_v3_upstream --depth 2

2) update U-Boot .config
 $ make qemu_arm64_menuconfig
  then, enable CONFIG_CMD_BOOTMENU and CONFIG_AUTOBOOT_MENU_SHOW

3) run on QEMU(arm64) example
 $ qemu-system-aarch64 -machine virt,gic-version=3 -cpu cortex-a57 -m 4G -nographic \
   -no-acpi -bios ./u-boot.bin -hda xxx.img

[Changes in v3]

- Major difference from previous version is that this version
  extends the existing bootmenu capability instead of updating
  efi bootmgr.
- include distro_boot entries
- "bootefi bootindex" command is newly added


Masahisa Kojima (2):
  efi_loader: introduce "bootefi bootindex" command
  bootmenu: add UEFI and disto_boot entries

 cmd/bootefi.c                |  42 ++++++
 cmd/bootmenu.c               | 268 +++++++++++++++++++++++++++++++++--
 include/efi_loader.h         |   1 +
 lib/efi_loader/efi_bootmgr.c |   7 +-
 4 files changed, 305 insertions(+), 13 deletions(-)

-- 
2.17.1

[RFC PATCH v3 1/2] efi_loader: introduce "bootefi bootindex" command

[Masahisa Kojima]

This commit introduces the new command "bootefi bootindex".
With this command, user can select which "Boot####" option
to load and execute.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Changes in v3:
- newly created

 cmd/bootefi.c                | 42 ++++++++++++++++++++++++++++++++++++
 include/efi_loader.h         |  1 +
 lib/efi_loader/efi_bootmgr.c |  7 +++---
 3 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 46eebd5ee2..df86438fec 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -416,6 +416,30 @@ static int do_efibootmgr(void)
 	return CMD_RET_SUCCESS;
 }
 
+/**
+ * do_efibootindex() - load and execute the specified Boot#### option
+ *
+ * Return:	status code
+ */
+static int do_efibootindex(u16 boot_index)
+{
+	efi_handle_t handle;
+	efi_status_t ret;
+	void *load_options;
+
+	ret = efi_try_load_entry(boot_index, &handle, &load_options);
+	if (ret != EFI_SUCCESS) {
+		log_notice("EFI boot manager: failed to load Boot%04X\n", boot_index);
+		return CMD_RET_FAILURE;
+	}
+
+	ret = do_bootefi_exec(handle, load_options);
+
+	if (ret != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
+
+	return CMD_RET_SUCCESS;
+}
 /**
  * do_bootefi_image() - execute EFI binary
  *
@@ -654,6 +678,22 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
 		return CMD_RET_FAILURE;
 	}
 
+	if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
+		if (!strcmp(argv[1], "bootindex")) {
+			char *endp;
+			int boot_index;
+
+			if (argc < 3)
+				return CMD_RET_USAGE;
+
+			boot_index = (int)hextoul(argv[2], &endp);
+			if (*endp != '\0' || boot_index > 0xffff)
+				return CMD_RET_USAGE;
+
+			return do_efibootindex((u16)boot_index);
+		}
+	}
+
 	if (argc > 2) {
 		uintptr_t fdt_addr;
 
@@ -702,6 +742,8 @@ static char bootefi_help_text[] =
 	"\n"
 	"    If specified, the device tree located at <fdt address> gets\n"
 	"    exposed as EFI configuration table.\n"
+	"bootefi bootindex <boot option>\n"
+	"  - load and boot EFI payload based on the specified BootXXXX variable.\n"
 #endif
 	;
 #endif
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 80a5f1ec01..e5972f5fee 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -861,6 +861,7 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
 				  efi_uintn_t load_options_size,
 				  void *load_options);
 efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
+efi_status_t efi_try_load_entry(u16 n, efi_handle_t *handle, void **load_options);
 
 /**
  * struct efi_image_regions - A list of memory regions
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 8c04ecbdc8..a3060b5c62 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -42,8 +42,7 @@ static const struct efi_runtime_services *rs;
  * @load_options:	load options set on the loaded image protocol
  * Return:		status code
  */
-static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
-				   void **load_options)
+efi_status_t efi_try_load_entry(u16 n, efi_handle_t *handle, void **load_options)
 {
 	struct efi_load_option lo;
 	u16 varname[] = u"Boot0000";
@@ -165,7 +164,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
 		/* load BootNext */
 		if (ret == EFI_SUCCESS) {
 			if (size == sizeof(u16)) {
-				ret = try_load_entry(bootnext, handle,
+				ret = efi_try_load_entry(bootnext, handle,
 						     load_options);
 				if (ret == EFI_SUCCESS)
 					return ret;
@@ -189,7 +188,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
 	for (i = 0; i < num; i++) {
 		log_debug("%s trying to load Boot%04X\n", __func__,
 			  bootorder[i]);
-		ret = try_load_entry(bootorder[i], handle, load_options);
+		ret = efi_try_load_entry(bootorder[i], handle, load_options);
 		if (ret == EFI_SUCCESS)
 			break;
 	}
-- 
2.17.1

[RFC PATCH v3 2/2] bootmenu: add UEFI and disto_boot entries

[Masahisa Kojima]

This commit adds the UEFI related menu entries and
distro_boot entries into the bootmenu.

For UEFI, user can select which UEFI "Boot####" option
to execute, call UEFI bootmgr and UEFI boot variable
maintenance menu. UEFI bootmgr entry is required to
correctly handle "BootNext" variable.

For distro_boot, user can select the boot device
included in "boot_targets" u-boot environment variable.

The menu example is as follows.

  *** U-Boot Boot Menu ***

     Boot 1. kernel (bootmenu_0)
     Boot 2. kernel (bootmenu_1)
     Reset board (bootmenu_2)
     debian (BOOT0000)
     ubuntu (BOOT0001)
     UEFI Boot Manager
     usb0
     scsi0
     virtio0
     dhcp
     UEFI Boot Manager Maintenance
     U-Boot console

  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Changes in v3:
- newly created

 cmd/bootmenu.c | 268 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 259 insertions(+), 9 deletions(-)

diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
index 409ef9a848..a8dc50dcaa 100644
--- a/cmd/bootmenu.c
+++ b/cmd/bootmenu.c
@@ -3,9 +3,12 @@
  * (C) Copyright 2011-2013 Pali Rohár <pali@kernel.org>
  */
 
+#include <charset.h>
 #include <common.h>
 #include <command.h>
 #include <ansi.h>
+#include <efi_loader.h>
+#include <efi_variable.h>
 #include <env.h>
 #include <log.h>
 #include <menu.h>
@@ -24,11 +27,20 @@
  */
 #define MAX_ENV_SIZE	(9 + 2 + 1)
 
+enum boot_type {
+	BOOT_TYPE_NONE = 0,
+	BOOT_TYPE_BOOTMENU,
+	BOOT_TYPE_UEFI,
+	BOOT_TYPE_DISTRO_BOOT,
+};
+
 struct bootmenu_entry {
 	unsigned short int num;		/* unique number 0 .. MAX_COUNT */
 	char key[3];			/* key identifier of number */
-	char *title;			/* title of entry */
+	u16 *title;			/* title of entry */
 	char *command;			/* hush command of entry */
+	enum boot_type type;
+	u16 bootorder;
 	struct bootmenu_data *menu;	/* this bootmenu */
 	struct bootmenu_entry *next;	/* next menu entry (num+1) */
 };
@@ -75,7 +87,12 @@ static void bootmenu_print_entry(void *data)
 	if (reverse)
 		puts(ANSI_COLOR_REVERSE);
 
-	puts(entry->title);
+	if (entry->type == BOOT_TYPE_BOOTMENU)
+		printf("%ls (bootmenu_%d)", entry->title, entry->bootorder);
+	else if (entry->type == BOOT_TYPE_UEFI)
+		printf("%ls (BOOT%04X)", entry->title, entry->bootorder);
+	else
+		printf("%ls", entry->title);
 
 	if (reverse)
 		puts(ANSI_COLOR_RESET);
@@ -87,6 +104,10 @@ static void bootmenu_autoboot_loop(struct bootmenu_data *menu,
 	int i, c;
 
 	if (menu->delay > 0) {
+		/* flush input */
+		while (tstc())
+			getchar();
+
 		printf(ANSI_CURSOR_POSITION, menu->count + 5, 1);
 		printf("  Hit any key to stop autoboot: %2d ", menu->delay);
 	}
@@ -300,6 +321,8 @@ static struct bootmenu_data *bootmenu_create(int delay)
 		menu->active = (int)simple_strtol(default_str, NULL, 10);
 
 	while ((option = bootmenu_getoption(i))) {
+		u16 *buf;
+
 		sep = strchr(option, '=');
 		if (!sep) {
 			printf("Invalid bootmenu entry: %s\n", option);
@@ -311,13 +334,13 @@ static struct bootmenu_data *bootmenu_create(int delay)
 			goto cleanup;
 
 		len = sep-option;
-		entry->title = malloc(len + 1);
+		buf = calloc(1, (len + 1) * sizeof(u16));
+		entry->title = buf;
 		if (!entry->title) {
 			free(entry);
 			goto cleanup;
 		}
-		memcpy(entry->title, option, len);
-		entry->title[len] = 0;
+		utf8_utf16_strncpy(&buf, option, len);
 
 		len = strlen(sep + 1);
 		entry->command = malloc(len + 1);
@@ -333,6 +356,190 @@ static struct bootmenu_data *bootmenu_create(int delay)
 
 		entry->num = i;
 		entry->menu = menu;
+		entry->type = BOOT_TYPE_BOOTMENU;
+		entry->bootorder = i;
+		entry->next = NULL;
+
+		if (!iter)
+			menu->first = entry;
+		else
+			iter->next = entry;
+
+		iter = entry;
+		++i;
+
+		if (i == MAX_COUNT - 1)
+			break;
+	}
+
+{
+	u16 *bootorder;
+	efi_status_t ret;
+	unsigned short j;
+	efi_uintn_t num, size;
+	void *load_option;
+	struct efi_load_option lo;
+	u16 varname[] = u"Boot####";
+
+	/* Initialize EFI drivers */
+	ret = efi_init_obj_list();
+	if (ret != EFI_SUCCESS) {
+		log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
+			ret & ~EFI_ERROR_MASK);
+		goto cleanup;
+	}
+
+	bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
+	if (!bootorder)
+		goto bootmgr;
+
+	num = size / sizeof(u16);
+	for (j = 0; j < num; j++) {
+		entry = malloc(sizeof(struct bootmenu_entry));
+		if (!entry)
+			goto cleanup;
+
+		efi_create_indexed_name(varname, sizeof(varname),
+					"Boot", bootorder[j]);
+		load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
+		if (!load_option)
+			continue;
+
+		ret = efi_deserialize_load_option(&lo, load_option, &size);
+		if (ret != EFI_SUCCESS) {
+			log_warning("Invalid load option for %ls\n", varname);
+			free(load_option);
+			continue;
+		}
+
+		if (lo.attributes & LOAD_OPTION_ACTIVE) {
+			char *command;
+			int command_size;
+
+			entry->title = u16_strdup(lo.label);
+			if (!entry->title) {
+				free(load_option);
+				free(entry);
+				goto cleanup;
+			}
+			command_size = strlen("bootefi bootindex XXXX") + 1;
+			command = calloc(1, command_size);
+			if (!command) {
+				free(entry->title);
+				free(load_option);
+				free(entry);
+				goto cleanup;
+			}
+			snprintf(command, command_size, "bootefi bootindex %X", bootorder[j]);
+			entry->command = command;
+			sprintf(entry->key, "%d", i);
+			entry->num = i;
+			entry->menu = menu;
+			entry->type = BOOT_TYPE_UEFI;
+			entry->bootorder = bootorder[j];
+			entry->next = NULL;
+
+			if (!iter)
+				menu->first = entry;
+			else
+				iter->next = entry;
+
+			iter = entry;
+			++i;
+		}
+
+		if (i == MAX_COUNT - 1)
+			break;
+	}
+	free(bootorder);
+}
+
+bootmgr:
+	/* Add UEFI Boot Manager entry if available */
+	if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
+		if (i <= MAX_COUNT - 1) {
+			entry = malloc(sizeof(struct bootmenu_entry));
+			if (!entry)
+				goto cleanup;
+
+			entry->title = u16_strdup(u"UEFI Boot Manager");
+			if (!entry->title) {
+				free(entry);
+				goto cleanup;
+			}
+
+			entry->command = strdup("bootefi bootmgr");
+			if (!entry->command) {
+				free(entry->title);
+				free(entry);
+				goto cleanup;
+			}
+
+			sprintf(entry->key, "%d", i);
+
+			entry->num = i;
+			entry->menu = menu;
+			entry->type = BOOT_TYPE_NONE;
+			entry->next = NULL;
+
+			if (!iter)
+				menu->first = entry;
+			else
+				iter->next = entry;
+
+			iter = entry;
+			++i;
+		}
+	}
+
+{
+	char *p;
+	char *token;
+	char *boot_targets;
+	int len;
+
+	/* list the distro boot "boot_targets" */
+	boot_targets = env_get("boot_targets");
+	if (!boot_targets)
+		goto exit_boot_targets;
+
+	len = strlen(boot_targets);
+	p = calloc(1, len + 1);
+	strncpy(p, boot_targets, len);
+
+	token = strtok(p, " ");
+
+	do {
+		u16 *buf;
+		char *command;
+		int command_size;
+
+		entry = malloc(sizeof(struct bootmenu_entry));
+		if (!entry)
+			goto cleanup;
+
+		len = strlen(token);
+		buf = calloc(1, (len + 1) * sizeof(u16));
+		entry->title = buf;
+		if (!entry->title) {
+			free(entry);
+			goto cleanup;
+		}
+		utf8_utf16_strncpy(&buf, token,len);
+		sprintf(entry->key, "%d", i);
+		entry->num = i;
+		entry->menu = menu;
+
+		command_size = strlen("run bootcmd_") + len + 1;
+		command = calloc(1, command_size);
+		if (!command) {
+			free(entry->title);
+			free(entry);
+			goto cleanup;
+		}
+		snprintf(command, command_size, "run bootcmd_%s", token);
+		entry->command = command;
+		entry->type = BOOT_TYPE_DISTRO_BOOT;
 		entry->next = NULL;
 
 		if (!iter)
@@ -345,6 +552,48 @@ static struct bootmenu_data *bootmenu_create(int delay)
 
 		if (i == MAX_COUNT - 1)
 			break;
+
+		token = strtok(NULL, " ");
+	} while (token);
+
+	free(p);
+}
+
+exit_boot_targets:
+
+	/* Add UEFI Boot Manager Maintenance entry */
+	if (i <= MAX_COUNT - 1) {
+		entry = malloc(sizeof(struct bootmenu_entry));
+		if (!entry)
+			goto cleanup;
+
+		entry->title = u16_strdup(u"UEFI Boot Manager Maintenance");
+		if (!entry->title) {
+			free(entry);
+			goto cleanup;
+		}
+
+		entry->command = strdup("echo TODO: Not implemented");
+		if (!entry->command) {
+			free(entry->title);
+			free(entry);
+			goto cleanup;
+		}
+
+		sprintf(entry->key, "%d", i);
+
+		entry->num = i;
+		entry->menu = menu;
+		entry->type = BOOT_TYPE_NONE;
+		entry->next = NULL;
+
+		if (!iter)
+			menu->first = entry;
+		else
+			iter->next = entry;
+
+		iter = entry;
+		++i;
 	}
 
 	/* Add U-Boot console entry at the end */
@@ -353,7 +602,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
 		if (!entry)
 			goto cleanup;
 
-		entry->title = strdup("U-Boot console");
+		entry->title = u16_strdup(u"U-Boot console");
 		if (!entry->title) {
 			free(entry);
 			goto cleanup;
@@ -370,6 +619,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
 
 		entry->num = i;
 		entry->menu = menu;
+		entry->type = BOOT_TYPE_NONE;
 		entry->next = NULL;
 
 		if (!iter)
@@ -427,7 +677,7 @@ static void bootmenu_show(int delay)
 {
 	int init = 0;
 	void *choice = NULL;
-	char *title = NULL;
+	u16 *title = NULL;
 	char *command = NULL;
 	struct menu *menu;
 	struct bootmenu_data *bootmenu;
@@ -478,7 +728,7 @@ static void bootmenu_show(int delay)
 
 	if (menu_get_choice(menu, &choice)) {
 		iter = choice;
-		title = strdup(iter->title);
+		title = u16_strdup(iter->title);
 		command = strdup(iter->command);
 	}
 
@@ -493,7 +743,7 @@ cleanup:
 	}
 
 	if (title && command) {
-		debug("Starting entry '%s'\n", title);
+		debug("Starting entry '%ls'\n", title);
 		free(title);
 		run_command(command, 0);
 		free(command);
-- 
2.17.1

Re: [RFC PATCH v3 1/2] efi_loader: introduce "bootefi bootindex" command

[Takahiro Akashi]

On Tue, Mar 08, 2022 at 11:07:44PM +0900, Masahisa Kojima wrote:
> This commit introduces the new command "bootefi bootindex".
> With this command, user can select which "Boot####" option
> to load and execute.

You can do the same thing with:
$ efidebug boot next 1 (for BOOT0001)
$ bootefi bootmgr

-Takahiro Akashi


> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v3:
> - newly created
> 
>  cmd/bootefi.c                | 42 ++++++++++++++++++++++++++++++++++++
>  include/efi_loader.h         |  1 +
>  lib/efi_loader/efi_bootmgr.c |  7 +++---
>  3 files changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 46eebd5ee2..df86438fec 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -416,6 +416,30 @@ static int do_efibootmgr(void)
>  	return CMD_RET_SUCCESS;
>  }
>  
> +/**
> + * do_efibootindex() - load and execute the specified Boot#### option
> + *
> + * Return:	status code
> + */
> +static int do_efibootindex(u16 boot_index)
> +{
> +	efi_handle_t handle;
> +	efi_status_t ret;
> +	void *load_options;
> +
> +	ret = efi_try_load_entry(boot_index, &handle, &load_options);
> +	if (ret != EFI_SUCCESS) {
> +		log_notice("EFI boot manager: failed to load Boot%04X\n", boot_index);
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	ret = do_bootefi_exec(handle, load_options);
> +
> +	if (ret != EFI_SUCCESS)
> +		return CMD_RET_FAILURE;
> +
> +	return CMD_RET_SUCCESS;
> +}
>  /**
>   * do_bootefi_image() - execute EFI binary
>   *
> @@ -654,6 +678,22 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
>  		return CMD_RET_FAILURE;
>  	}
>  
> +	if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
> +		if (!strcmp(argv[1], "bootindex")) {
> +			char *endp;
> +			int boot_index;
> +
> +			if (argc < 3)
> +				return CMD_RET_USAGE;
> +
> +			boot_index = (int)hextoul(argv[2], &endp);
> +			if (*endp != '\0' || boot_index > 0xffff)
> +				return CMD_RET_USAGE;
> +
> +			return do_efibootindex((u16)boot_index);
> +		}
> +	}
> +
>  	if (argc > 2) {
>  		uintptr_t fdt_addr;
>  
> @@ -702,6 +742,8 @@ static char bootefi_help_text[] =
>  	"\n"
>  	"    If specified, the device tree located at <fdt address> gets\n"
>  	"    exposed as EFI configuration table.\n"
> +	"bootefi bootindex <boot option>\n"
> +	"  - load and boot EFI payload based on the specified BootXXXX variable.\n"
>  #endif
>  	;
>  #endif
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 80a5f1ec01..e5972f5fee 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -861,6 +861,7 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
>  				  efi_uintn_t load_options_size,
>  				  void *load_options);
>  efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
> +efi_status_t efi_try_load_entry(u16 n, efi_handle_t *handle, void **load_options);
>  
>  /**
>   * struct efi_image_regions - A list of memory regions
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 8c04ecbdc8..a3060b5c62 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -42,8 +42,7 @@ static const struct efi_runtime_services *rs;
>   * @load_options:	load options set on the loaded image protocol
>   * Return:		status code
>   */
> -static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> -				   void **load_options)
> +efi_status_t efi_try_load_entry(u16 n, efi_handle_t *handle, void **load_options)
>  {
>  	struct efi_load_option lo;
>  	u16 varname[] = u"Boot0000";
> @@ -165,7 +164,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
>  		/* load BootNext */
>  		if (ret == EFI_SUCCESS) {
>  			if (size == sizeof(u16)) {
> -				ret = try_load_entry(bootnext, handle,
> +				ret = efi_try_load_entry(bootnext, handle,
>  						     load_options);
>  				if (ret == EFI_SUCCESS)
>  					return ret;
> @@ -189,7 +188,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
>  	for (i = 0; i < num; i++) {
>  		log_debug("%s trying to load Boot%04X\n", __func__,
>  			  bootorder[i]);
> -		ret = try_load_entry(bootorder[i], handle, load_options);
> +		ret = efi_try_load_entry(bootorder[i], handle, load_options);
>  		if (ret == EFI_SUCCESS)
>  			break;
>  	}
> -- 
> 2.17.1
> 

Re: [RFC PATCH v3 1/2] efi_loader: introduce "bootefi bootindex" command

[Masahisa Kojima]

On Tue, 8 Mar 2022 at 23:17, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
>
> On Tue, Mar 08, 2022 at 11:07:44PM +0900, Masahisa Kojima wrote:
> > This commit introduces the new command "bootefi bootindex".
> > With this command, user can select which "Boot####" option
> > to load and execute.
>
> You can do the same thing with:
> $ efidebug boot next 1 (for BOOT0001)
> $ bootefi bootmgr

Thank you for the information, it is good to know.
My only concern is that user needs to enable "efidebug" command
for this case, since efidebug implies that it is for debug purpose.

Thanks,
Masahisa Kojima

>
> -Takahiro Akashi
>
>
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Changes in v3:
> > - newly created
> >
> >  cmd/bootefi.c                | 42 ++++++++++++++++++++++++++++++++++++
> >  include/efi_loader.h         |  1 +
> >  lib/efi_loader/efi_bootmgr.c |  7 +++---
> >  3 files changed, 46 insertions(+), 4 deletions(-)
> >
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 46eebd5ee2..df86438fec 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -416,6 +416,30 @@ static int do_efibootmgr(void)
> >       return CMD_RET_SUCCESS;
> >  }
> >
> > +/**
> > + * do_efibootindex() - load and execute the specified Boot#### option
> > + *
> > + * Return:   status code
> > + */
> > +static int do_efibootindex(u16 boot_index)
> > +{
> > +     efi_handle_t handle;
> > +     efi_status_t ret;
> > +     void *load_options;
> > +
> > +     ret = efi_try_load_entry(boot_index, &handle, &load_options);
> > +     if (ret != EFI_SUCCESS) {
> > +             log_notice("EFI boot manager: failed to load Boot%04X\n", boot_index);
> > +             return CMD_RET_FAILURE;
> > +     }
> > +
> > +     ret = do_bootefi_exec(handle, load_options);
> > +
> > +     if (ret != EFI_SUCCESS)
> > +             return CMD_RET_FAILURE;
> > +
> > +     return CMD_RET_SUCCESS;
> > +}
> >  /**
> >   * do_bootefi_image() - execute EFI binary
> >   *
> > @@ -654,6 +678,22 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> >               return CMD_RET_FAILURE;
> >       }
> >
> > +     if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
> > +             if (!strcmp(argv[1], "bootindex")) {
> > +                     char *endp;
> > +                     int boot_index;
> > +
> > +                     if (argc < 3)
> > +                             return CMD_RET_USAGE;
> > +
> > +                     boot_index = (int)hextoul(argv[2], &endp);
> > +                     if (*endp != '\0' || boot_index > 0xffff)
> > +                             return CMD_RET_USAGE;
> > +
> > +                     return do_efibootindex((u16)boot_index);
> > +             }
> > +     }
> > +
> >       if (argc > 2) {
> >               uintptr_t fdt_addr;
> >
> > @@ -702,6 +742,8 @@ static char bootefi_help_text[] =
> >       "\n"
> >       "    If specified, the device tree located at <fdt address> gets\n"
> >       "    exposed as EFI configuration table.\n"
> > +     "bootefi bootindex <boot option>\n"
> > +     "  - load and boot EFI payload based on the specified BootXXXX variable.\n"
> >  #endif
> >       ;
> >  #endif
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 80a5f1ec01..e5972f5fee 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -861,6 +861,7 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
> >                                 efi_uintn_t load_options_size,
> >                                 void *load_options);
> >  efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
> > +efi_status_t efi_try_load_entry(u16 n, efi_handle_t *handle, void **load_options);
> >
> >  /**
> >   * struct efi_image_regions - A list of memory regions
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 8c04ecbdc8..a3060b5c62 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -42,8 +42,7 @@ static const struct efi_runtime_services *rs;
> >   * @load_options:    load options set on the loaded image protocol
> >   * Return:           status code
> >   */
> > -static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> > -                                void **load_options)
> > +efi_status_t efi_try_load_entry(u16 n, efi_handle_t *handle, void **load_options)
> >  {
> >       struct efi_load_option lo;
> >       u16 varname[] = u"Boot0000";
> > @@ -165,7 +164,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
> >               /* load BootNext */
> >               if (ret == EFI_SUCCESS) {
> >                       if (size == sizeof(u16)) {
> > -                             ret = try_load_entry(bootnext, handle,
> > +                             ret = efi_try_load_entry(bootnext, handle,
> >                                                    load_options);
> >                               if (ret == EFI_SUCCESS)
> >                                       return ret;
> > @@ -189,7 +188,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
> >       for (i = 0; i < num; i++) {
> >               log_debug("%s trying to load Boot%04X\n", __func__,
> >                         bootorder[i]);
> > -             ret = try_load_entry(bootorder[i], handle, load_options);
> > +             ret = efi_try_load_entry(bootorder[i], handle, load_options);
> >               if (ret == EFI_SUCCESS)
> >                       break;
> >       }
> > --
> > 2.17.1
> >

Re: [RFC PATCH v3 1/2] efi_loader: introduce "bootefi bootindex" command

[Takahiro Akashi]

On Wed, Mar 09, 2022 at 09:47:25AM +0900, Masahisa Kojima wrote:
> On Tue, 8 Mar 2022 at 23:17, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
> >
> > On Tue, Mar 08, 2022 at 11:07:44PM +0900, Masahisa Kojima wrote:
> > > This commit introduces the new command "bootefi bootindex".
> > > With this command, user can select which "Boot####" option
> > > to load and execute.
> >
> > You can do the same thing with:
> > $ efidebug boot next 1 (for BOOT0001)
> > $ bootefi bootmgr
> 
> Thank you for the information, it is good to know.
> My only concern is that user needs to enable "efidebug" command
> for this case, since efidebug implies that it is for debug purpose.

Yeah, that is the point where I and ex-maintainer have never agreed.
So we have no standard UI for UEFI subsystem on U-Boot until now.

Just FYI, we can do the same thing in yet another way :)
  => env set -e -nv -bs -rt BootNext =0x0001

Similarly, BootOrder as well.

-Takahiro Akashi

> Thanks,
> Masahisa Kojima
> 
> >
> > -Takahiro Akashi
> >
> >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > ---
> > > Changes in v3:
> > > - newly created
> > >
> > >  cmd/bootefi.c                | 42 ++++++++++++++++++++++++++++++++++++
> > >  include/efi_loader.h         |  1 +
> > >  lib/efi_loader/efi_bootmgr.c |  7 +++---
> > >  3 files changed, 46 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > > index 46eebd5ee2..df86438fec 100644
> > > --- a/cmd/bootefi.c
> > > +++ b/cmd/bootefi.c
> > > @@ -416,6 +416,30 @@ static int do_efibootmgr(void)
> > >       return CMD_RET_SUCCESS;
> > >  }
> > >
> > > +/**
> > > + * do_efibootindex() - load and execute the specified Boot#### option
> > > + *
> > > + * Return:   status code
> > > + */
> > > +static int do_efibootindex(u16 boot_index)
> > > +{
> > > +     efi_handle_t handle;
> > > +     efi_status_t ret;
> > > +     void *load_options;
> > > +
> > > +     ret = efi_try_load_entry(boot_index, &handle, &load_options);
> > > +     if (ret != EFI_SUCCESS) {
> > > +             log_notice("EFI boot manager: failed to load Boot%04X\n", boot_index);
> > > +             return CMD_RET_FAILURE;
> > > +     }
> > > +
> > > +     ret = do_bootefi_exec(handle, load_options);
> > > +
> > > +     if (ret != EFI_SUCCESS)
> > > +             return CMD_RET_FAILURE;
> > > +
> > > +     return CMD_RET_SUCCESS;
> > > +}
> > >  /**
> > >   * do_bootefi_image() - execute EFI binary
> > >   *
> > > @@ -654,6 +678,22 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> > >               return CMD_RET_FAILURE;
> > >       }
> > >
> > > +     if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
> > > +             if (!strcmp(argv[1], "bootindex")) {
> > > +                     char *endp;
> > > +                     int boot_index;
> > > +
> > > +                     if (argc < 3)
> > > +                             return CMD_RET_USAGE;
> > > +
> > > +                     boot_index = (int)hextoul(argv[2], &endp);
> > > +                     if (*endp != '\0' || boot_index > 0xffff)
> > > +                             return CMD_RET_USAGE;
> > > +
> > > +                     return do_efibootindex((u16)boot_index);
> > > +             }
> > > +     }
> > > +
> > >       if (argc > 2) {
> > >               uintptr_t fdt_addr;
> > >
> > > @@ -702,6 +742,8 @@ static char bootefi_help_text[] =
> > >       "\n"
> > >       "    If specified, the device tree located at <fdt address> gets\n"
> > >       "    exposed as EFI configuration table.\n"
> > > +     "bootefi bootindex <boot option>\n"
> > > +     "  - load and boot EFI payload based on the specified BootXXXX variable.\n"
> > >  #endif
> > >       ;
> > >  #endif
> > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > index 80a5f1ec01..e5972f5fee 100644
> > > --- a/include/efi_loader.h
> > > +++ b/include/efi_loader.h
> > > @@ -861,6 +861,7 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
> > >                                 efi_uintn_t load_options_size,
> > >                                 void *load_options);
> > >  efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
> > > +efi_status_t efi_try_load_entry(u16 n, efi_handle_t *handle, void **load_options);
> > >
> > >  /**
> > >   * struct efi_image_regions - A list of memory regions
> > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > > index 8c04ecbdc8..a3060b5c62 100644
> > > --- a/lib/efi_loader/efi_bootmgr.c
> > > +++ b/lib/efi_loader/efi_bootmgr.c
> > > @@ -42,8 +42,7 @@ static const struct efi_runtime_services *rs;
> > >   * @load_options:    load options set on the loaded image protocol
> > >   * Return:           status code
> > >   */
> > > -static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> > > -                                void **load_options)
> > > +efi_status_t efi_try_load_entry(u16 n, efi_handle_t *handle, void **load_options)
> > >  {
> > >       struct efi_load_option lo;
> > >       u16 varname[] = u"Boot0000";
> > > @@ -165,7 +164,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
> > >               /* load BootNext */
> > >               if (ret == EFI_SUCCESS) {
> > >                       if (size == sizeof(u16)) {
> > > -                             ret = try_load_entry(bootnext, handle,
> > > +                             ret = efi_try_load_entry(bootnext, handle,
> > >                                                    load_options);
> > >                               if (ret == EFI_SUCCESS)
> > >                                       return ret;
> > > @@ -189,7 +188,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
> > >       for (i = 0; i < num; i++) {
> > >               log_debug("%s trying to load Boot%04X\n", __func__,
> > >                         bootorder[i]);
> > > -             ret = try_load_entry(bootorder[i], handle, load_options);
> > > +             ret = efi_try_load_entry(bootorder[i], handle, load_options);
> > >               if (ret == EFI_SUCCESS)
> > >                       break;
> > >       }
> > > --
> > > 2.17.1
> > >

Re: [RFC PATCH v3 1/2] efi_loader: introduce "bootefi bootindex" command

[Ilias Apalodimas]

Hi Kojima-san

On Wed, Mar 09, 2022 at 11:35:57AM +0900, Takahiro Akashi wrote:
> On Wed, Mar 09, 2022 at 09:47:25AM +0900, Masahisa Kojima wrote:
> > On Tue, 8 Mar 2022 at 23:17, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Tue, Mar 08, 2022 at 11:07:44PM +0900, Masahisa Kojima wrote:
> > > > This commit introduces the new command "bootefi bootindex".
> > > > With this command, user can select which "Boot####" option
> > > > to load and execute.
> > >
> > > You can do the same thing with:
> > > $ efidebug boot next 1 (for BOOT0001)
> > > $ bootefi bootmgr
> > 
> > Thank you for the information, it is good to know.
> > My only concern is that user needs to enable "efidebug" command
> > for this case, since efidebug implies that it is for debug purpose.
> 
> Yeah, that is the point where I and ex-maintainer have never agreed.
> So we have no standard UI for UEFI subsystem on U-Boot until now.
> 
> Just FYI, we can do the same thing in yet another way :)
>   => env set -e -nv -bs -rt BootNext =0x0001
> 
> Similarly, BootOrder as well.
> 
> -Takahiro Akashi

I also think it's safe to drop this patch.  It's indeed an easier way to
load a specific boot index, however the use of bootnext is clearly defined
in the spec.  I don't think we need to increase the EFI code more as long
as it's clear to anyone that setting bootnext (note here it has to be set
on each reboot) would have the same result.

Cheers
/Ilias
> 
> > Thanks,
> > Masahisa Kojima
> > 
> > >
> > > -Takahiro Akashi
> > >
> > >
> > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > ---
> > > > Changes in v3:
> > > > - newly created
> > > >
> > > >  cmd/bootefi.c                | 42 ++++++++++++++++++++++++++++++++++++
> > > >  include/efi_loader.h         |  1 +
> > > >  lib/efi_loader/efi_bootmgr.c |  7 +++---
> > > >  3 files changed, 46 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > > > index 46eebd5ee2..df86438fec 100644
> > > > --- a/cmd/bootefi.c
> > > > +++ b/cmd/bootefi.c
> > > > @@ -416,6 +416,30 @@ static int do_efibootmgr(void)
> > > >       return CMD_RET_SUCCESS;
> > > >  }
> > > >
> > > > +/**
> > > > + * do_efibootindex() - load and execute the specified Boot#### option
> > > > + *
> > > > + * Return:   status code
> > > > + */
> > > > +static int do_efibootindex(u16 boot_index)
> > > > +{
> > > > +     efi_handle_t handle;
> > > > +     efi_status_t ret;
> > > > +     void *load_options;
> > > > +
> > > > +     ret = efi_try_load_entry(boot_index, &handle, &load_options);
> > > > +     if (ret != EFI_SUCCESS) {
> > > > +             log_notice("EFI boot manager: failed to load Boot%04X\n", boot_index);
> > > > +             return CMD_RET_FAILURE;
> > > > +     }
> > > > +
> > > > +     ret = do_bootefi_exec(handle, load_options);
> > > > +
> > > > +     if (ret != EFI_SUCCESS)
> > > > +             return CMD_RET_FAILURE;
> > > > +
> > > > +     return CMD_RET_SUCCESS;
> > > > +}
> > > >  /**
> > > >   * do_bootefi_image() - execute EFI binary
> > > >   *
> > > > @@ -654,6 +678,22 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> > > >               return CMD_RET_FAILURE;
> > > >       }
> > > >
> > > > +     if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
> > > > +             if (!strcmp(argv[1], "bootindex")) {
> > > > +                     char *endp;
> > > > +                     int boot_index;
> > > > +
> > > > +                     if (argc < 3)
> > > > +                             return CMD_RET_USAGE;
> > > > +
> > > > +                     boot_index = (int)hextoul(argv[2], &endp);
> > > > +                     if (*endp != '\0' || boot_index > 0xffff)
> > > > +                             return CMD_RET_USAGE;
> > > > +
> > > > +                     return do_efibootindex((u16)boot_index);
> > > > +             }
> > > > +     }
> > > > +
> > > >       if (argc > 2) {
> > > >               uintptr_t fdt_addr;
> > > >
> > > > @@ -702,6 +742,8 @@ static char bootefi_help_text[] =
> > > >       "\n"
> > > >       "    If specified, the device tree located at <fdt address> gets\n"
> > > >       "    exposed as EFI configuration table.\n"
> > > > +     "bootefi bootindex <boot option>\n"
> > > > +     "  - load and boot EFI payload based on the specified BootXXXX variable.\n"
> > > >  #endif
> > > >       ;
> > > >  #endif
> > > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > > index 80a5f1ec01..e5972f5fee 100644
> > > > --- a/include/efi_loader.h
> > > > +++ b/include/efi_loader.h
> > > > @@ -861,6 +861,7 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
> > > >                                 efi_uintn_t load_options_size,
> > > >                                 void *load_options);
> > > >  efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
> > > > +efi_status_t efi_try_load_entry(u16 n, efi_handle_t *handle, void **load_options);
> > > >
> > > >  /**
> > > >   * struct efi_image_regions - A list of memory regions
> > > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > > > index 8c04ecbdc8..a3060b5c62 100644
> > > > --- a/lib/efi_loader/efi_bootmgr.c
> > > > +++ b/lib/efi_loader/efi_bootmgr.c
> > > > @@ -42,8 +42,7 @@ static const struct efi_runtime_services *rs;
> > > >   * @load_options:    load options set on the loaded image protocol
> > > >   * Return:           status code
> > > >   */
> > > > -static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> > > > -                                void **load_options)
> > > > +efi_status_t efi_try_load_entry(u16 n, efi_handle_t *handle, void **load_options)
> > > >  {
> > > >       struct efi_load_option lo;
> > > >       u16 varname[] = u"Boot0000";
> > > > @@ -165,7 +164,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
> > > >               /* load BootNext */
> > > >               if (ret == EFI_SUCCESS) {
> > > >                       if (size == sizeof(u16)) {
> > > > -                             ret = try_load_entry(bootnext, handle,
> > > > +                             ret = efi_try_load_entry(bootnext, handle,
> > > >                                                    load_options);
> > > >                               if (ret == EFI_SUCCESS)
> > > >                                       return ret;
> > > > @@ -189,7 +188,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
> > > >       for (i = 0; i < num; i++) {
> > > >               log_debug("%s trying to load Boot%04X\n", __func__,
> > > >                         bootorder[i]);
> > > > -             ret = try_load_entry(bootorder[i], handle, load_options);
> > > > +             ret = efi_try_load_entry(bootorder[i], handle, load_options);
> > > >               if (ret == EFI_SUCCESS)
> > > >                       break;
> > > >       }
> > > > --
> > > > 2.17.1
> > > >

Re: [RFC PATCH v3 2/2] bootmenu: add UEFI and disto_boot entries

[Ilias Apalodimas]

Hi Kojima-san

On Tue, Mar 08, 2022 at 11:07:45PM +0900, Masahisa Kojima wrote:
> This commit adds the UEFI related menu entries and
> distro_boot entries into the bootmenu.
> 
> For UEFI, user can select which UEFI "Boot####" option
> to execute, call UEFI bootmgr and UEFI boot variable
> maintenance menu. UEFI bootmgr entry is required to
> correctly handle "BootNext" variable.

Hmm why? (some more info further down)

> 
> For distro_boot, user can select the boot device
> included in "boot_targets" u-boot environment variable.
> 
> The menu example is as follows.
> 
>   *** U-Boot Boot Menu ***
> 
>      Boot 1. kernel (bootmenu_0)
>      Boot 2. kernel (bootmenu_1)
>      Reset board (bootmenu_2)
>      debian (BOOT0000)
>      ubuntu (BOOT0001)
>      UEFI Boot Manager
>      usb0
>      scsi0
>      virtio0
>      dhcp
>      UEFI Boot Manager Maintenance
>      U-Boot console
> 
>   Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v3:
> - newly created
> 
>  cmd/bootmenu.c | 268 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 259 insertions(+), 9 deletions(-)
> 
> diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> index 409ef9a848..a8dc50dcaa 100644
> --- a/cmd/bootmenu.c
> +++ b/cmd/bootmenu.c
> @@ -3,9 +3,12 @@
>   * (C) Copyright 2011-2013 Pali Rohár <pali@kernel.org>
>   */
>  
> +#include <charset.h>
>  #include <common.h>
>  #include <command.h>
>  #include <ansi.h>
> +#include <efi_loader.h>
> +#include <efi_variable.h>
>  #include <env.h>
>  #include <log.h>
>  #include <menu.h>
> @@ -24,11 +27,20 @@
>   */
>  #define MAX_ENV_SIZE	(9 + 2 + 1)
>  
> +enum boot_type {
> +	BOOT_TYPE_NONE = 0,
> +	BOOT_TYPE_BOOTMENU,
> +	BOOT_TYPE_UEFI,
> +	BOOT_TYPE_DISTRO_BOOT,
> +};
> +
>  struct bootmenu_entry {
>  	unsigned short int num;		/* unique number 0 .. MAX_COUNT */
>  	char key[3];			/* key identifier of number */
> -	char *title;			/* title of entry */
> +	u16 *title;			/* title of entry */
>  	char *command;			/* hush command of entry */
> +	enum boot_type type;
> +	u16 bootorder;
>  	struct bootmenu_data *menu;	/* this bootmenu */
>  	struct bootmenu_entry *next;	/* next menu entry (num+1) */
>  };
> @@ -75,7 +87,12 @@ static void bootmenu_print_entry(void *data)
>  	if (reverse)
>  		puts(ANSI_COLOR_REVERSE);
>  
> -	puts(entry->title);
> +	if (entry->type == BOOT_TYPE_BOOTMENU)
> +		printf("%ls (bootmenu_%d)", entry->title, entry->bootorder);
> +	else if (entry->type == BOOT_TYPE_UEFI)
> +		printf("%ls (BOOT%04X)", entry->title, entry->bootorder);
> +	else
> +		printf("%ls", entry->title);
>  
>  	if (reverse)
>  		puts(ANSI_COLOR_RESET);
> @@ -87,6 +104,10 @@ static void bootmenu_autoboot_loop(struct bootmenu_data *menu,
>  	int i, c;
>  
>  	if (menu->delay > 0) {
> +		/* flush input */
> +		while (tstc())
> +			getchar();
> +
>  		printf(ANSI_CURSOR_POSITION, menu->count + 5, 1);
>  		printf("  Hit any key to stop autoboot: %2d ", menu->delay);
>  	}
> @@ -300,6 +321,8 @@ static struct bootmenu_data *bootmenu_create(int delay)
>  		menu->active = (int)simple_strtol(default_str, NULL, 10);
>  
>  	while ((option = bootmenu_getoption(i))) {
> +		u16 *buf;
> +
>  		sep = strchr(option, '=');
>  		if (!sep) {
>  			printf("Invalid bootmenu entry: %s\n", option);
> @@ -311,13 +334,13 @@ static struct bootmenu_data *bootmenu_create(int delay)
>  			goto cleanup;
>  
>  		len = sep-option;
> -		entry->title = malloc(len + 1);
> +		buf = calloc(1, (len + 1) * sizeof(u16));
> +		entry->title = buf;
>  		if (!entry->title) {
>  			free(entry);
>  			goto cleanup;
>  		}
> -		memcpy(entry->title, option, len);
> -		entry->title[len] = 0;
> +		utf8_utf16_strncpy(&buf, option, len);
>  
>  		len = strlen(sep + 1);
>  		entry->command = malloc(len + 1);
> @@ -333,6 +356,190 @@ static struct bootmenu_data *bootmenu_create(int delay)
>  
>  		entry->num = i;
>  		entry->menu = menu;
> +		entry->type = BOOT_TYPE_BOOTMENU;
> +		entry->bootorder = i;
> +		entry->next = NULL;
> +
> +		if (!iter)
> +			menu->first = entry;
> +		else
> +			iter->next = entry;
> +
> +		iter = entry;
> +		++i;
> +
> +		if (i == MAX_COUNT - 1)
> +			break;
> +	}
> +
> +{
> +	u16 *bootorder;
> +	efi_status_t ret;
> +	unsigned short j;
> +	efi_uintn_t num, size;
> +	void *load_option;
> +	struct efi_load_option lo;
> +	u16 varname[] = u"Boot####";
> +
> +	/* Initialize EFI drivers */
> +	ret = efi_init_obj_list();
> +	if (ret != EFI_SUCCESS) {
> +		log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> +			ret & ~EFI_ERROR_MASK);
> +		goto cleanup;
> +	}
> +
> +	bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
> +	if (!bootorder)
> +		goto bootmgr;
> +
> +	num = size / sizeof(u16);
> +	for (j = 0; j < num; j++) {
> +		entry = malloc(sizeof(struct bootmenu_entry));
> +		if (!entry)
> +			goto cleanup;
> +
> +		efi_create_indexed_name(varname, sizeof(varname),
> +					"Boot", bootorder[j]);
> +		load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
> +		if (!load_option)
> +			continue;
> +
> +		ret = efi_deserialize_load_option(&lo, load_option, &size);
> +		if (ret != EFI_SUCCESS) {
> +			log_warning("Invalid load option for %ls\n", varname);
> +			free(load_option);
> +			continue;
> +		}
> +
> +		if (lo.attributes & LOAD_OPTION_ACTIVE) {
> +			char *command;
> +			int command_size;
> +
> +			entry->title = u16_strdup(lo.label);
> +			if (!entry->title) {
> +				free(load_option);
> +				free(entry);
> +				goto cleanup;
> +			}
> +			command_size = strlen("bootefi bootindex XXXX") + 1;

As we commented on patch 2/2 you can simply set BootNext variable here and
fire up the efibootmgr

> +			command = calloc(1, command_size);
> +			if (!command) {
> +				free(entry->title);
> +				free(load_option);
> +				free(entry);
> +				goto cleanup;
> +			}
> +			snprintf(command, command_size, "bootefi bootindex %X", bootorder[j]);
> +			entry->command = command;
> +			sprintf(entry->key, "%d", i);
> +			entry->num = i;
> +			entry->menu = menu;
> +			entry->type = BOOT_TYPE_UEFI;
> +			entry->bootorder = bootorder[j];
> +			entry->next = NULL;
> +
> +			if (!iter)
> +				menu->first = entry;
> +			else
> +				iter->next = entry;
> +
> +			iter = entry;
> +			++i;
> +		}
> +
> +		if (i == MAX_COUNT - 1)
> +			break;
> +	}
> +	free(bootorder);
> +}
> +
> +bootmgr:

Why do we need an entire menu entry if the bootorder is not defined?
Currently there's no logic in the efibootmgr to look for the standard
filenames in the ESP (eg bootarm.efi, bootaa64.efi etc) if no boot option
is defined.  Instead this is implement in distro_bootcmd.

I was thinking of something along the lines of:
1. bootmenu comes up
2. We read all the Boot#### variables that are defined and add them on the
   menu 
2a. If the user doesn't explicitly choose a boot option we run 'bootefi bootmgr'
2b. If the user does select a different option we set BootNext and still
run 'bootefi bootmgr'
3. If there's not Boot#### option defined,  we should in the future add the
   functionality of searching for bootaa64.efi etc in ESP and still just 
   launch the efi bootmgr

> +	/* Add UEFI Boot Manager entry if available */
> +	if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
> +		if (i <= MAX_COUNT - 1) {
> +			entry = malloc(sizeof(struct bootmenu_entry));
> +			if (!entry)
> +				goto cleanup;
> +
> +			entry->title = u16_strdup(u"UEFI Boot Manager");
> +			if (!entry->title) {
> +				free(entry);
> +				goto cleanup;
> +			}
> +
> +			entry->command = strdup("bootefi bootmgr");
> +			if (!entry->command) {
> +				free(entry->title);
> +				free(entry);
> +				goto cleanup;
> +			}
> +
> +			sprintf(entry->key, "%d", i);
> +
> +			entry->num = i;
> +			entry->menu = menu;
> +			entry->type = BOOT_TYPE_NONE;
> +			entry->next = NULL;
> +
> +			if (!iter)
> +				menu->first = entry;
> +			else
> +				iter->next = entry;
> +
> +			iter = entry;
> +			++i;
> +		}
> +	}
> +
> +{
> +	char *p;
> +	char *token;
> +	char *boot_targets;
> +	int len;
> +
> +	/* list the distro boot "boot_targets" */
> +	boot_targets = env_get("boot_targets");
> +	if (!boot_targets)
> +		goto exit_boot_targets;
> +
> +	len = strlen(boot_targets);
> +	p = calloc(1, len + 1);
> +	strncpy(p, boot_targets, len);
> +
> +	token = strtok(p, " ");
> +
> +	do {
> +		u16 *buf;
> +		char *command;
> +		int command_size;
> +
> +		entry = malloc(sizeof(struct bootmenu_entry));
> +		if (!entry)
> +			goto cleanup;
> +
> +		len = strlen(token);
> +		buf = calloc(1, (len + 1) * sizeof(u16));
> +		entry->title = buf;
> +		if (!entry->title) {
> +			free(entry);
> +			goto cleanup;
> +		}
> +		utf8_utf16_strncpy(&buf, token,len);
> +		sprintf(entry->key, "%d", i);
> +		entry->num = i;
> +		entry->menu = menu;
> +
> +		command_size = strlen("run bootcmd_") + len + 1;

I think we are better of here with a sizeof() instead of strlen since the
'run bootcmd_' string is not expected to change

> +		command = calloc(1, command_size);
> +		if (!command) {
> +			free(entry->title);
> +			free(entry);
> +			goto cleanup;
> +		}
> +		snprintf(command, command_size, "run bootcmd_%s", token);
> +		entry->command = command;
> +		entry->type = BOOT_TYPE_DISTRO_BOOT;
>  		entry->next = NULL;
>  
>  		if (!iter)
> @@ -345,6 +552,48 @@ static struct bootmenu_data *bootmenu_create(int delay)
>  
>  		if (i == MAX_COUNT - 1)
>  			break;
> +
> +		token = strtok(NULL, " ");
> +	} while (token);
> +
> +	free(p);
> +}
> +
> +exit_boot_targets:
> +
> +	/* Add UEFI Boot Manager Maintenance entry */
> +	if (i <= MAX_COUNT - 1) {
> +		entry = malloc(sizeof(struct bootmenu_entry));
> +		if (!entry)
> +			goto cleanup;
> +
> +		entry->title = u16_strdup(u"UEFI Boot Manager Maintenance");
> +		if (!entry->title) {
> +			free(entry);
> +			goto cleanup;
> +		}
> +
> +		entry->command = strdup("echo TODO: Not implemented");
> +		if (!entry->command) {
> +			free(entry->title);
> +			free(entry);
> +			goto cleanup;
> +		}

Any idea of how we'll tackle that?  We could export the efibootmgr
functions that deal with this and use them on the edit menu ?

> +
> +		sprintf(entry->key, "%d", i);
> +
> +		entry->num = i;
> +		entry->menu = menu;
> +		entry->type = BOOT_TYPE_NONE;
> +		entry->next = NULL;
> +
> +		if (!iter)
> +			menu->first = entry;
> +		else
> +			iter->next = entry;
> +
> +		iter = entry;
> +		++i;
>  	}
>  
>  	/* Add U-Boot console entry at the end */
> @@ -353,7 +602,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
>  		if (!entry)
>  			goto cleanup;
>  
> -		entry->title = strdup("U-Boot console");
> +		entry->title = u16_strdup(u"U-Boot console");

Can we add an option to prohibit the user from going back to the console?

>  		if (!entry->title) {
>  			free(entry);
>  			goto cleanup;
> @@ -370,6 +619,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
>  
>  		entry->num = i;
>  		entry->menu = menu;
> +		entry->type = BOOT_TYPE_NONE;
>  		entry->next = NULL;
>  
>  		if (!iter)
> @@ -427,7 +677,7 @@ static void bootmenu_show(int delay)
>  {
>  	int init = 0;
>  	void *choice = NULL;
> -	char *title = NULL;
> +	u16 *title = NULL;
>  	char *command = NULL;
>  	struct menu *menu;
>  	struct bootmenu_data *bootmenu;
> @@ -478,7 +728,7 @@ static void bootmenu_show(int delay)
>  
>  	if (menu_get_choice(menu, &choice)) {
>  		iter = choice;
> -		title = strdup(iter->title);
> +		title = u16_strdup(iter->title);
>  		command = strdup(iter->command);
>  	}
>  
> @@ -493,7 +743,7 @@ cleanup:
>  	}
>  
>  	if (title && command) {
> -		debug("Starting entry '%s'\n", title);
> +		debug("Starting entry '%ls'\n", title);
>  		free(title);
>  		run_command(command, 0);
>  		free(command);
> -- 
> 2.17.1
> 

Cheers
/Ilias

Re: [RFC PATCH v3 1/2] efi_loader: introduce "bootefi bootindex" command

[Masahisa Kojima]

Hi Akashi-san, Ilias,

On Wed, 9 Mar 2022 at 22:50, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Kojima-san
>
> On Wed, Mar 09, 2022 at 11:35:57AM +0900, Takahiro Akashi wrote:
> > On Wed, Mar 09, 2022 at 09:47:25AM +0900, Masahisa Kojima wrote:
> > > On Tue, 8 Mar 2022 at 23:17, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > On Tue, Mar 08, 2022 at 11:07:44PM +0900, Masahisa Kojima wrote:
> > > > > This commit introduces the new command "bootefi bootindex".
> > > > > With this command, user can select which "Boot####" option
> > > > > to load and execute.
> > > >
> > > > You can do the same thing with:
> > > > $ efidebug boot next 1 (for BOOT0001)
> > > > $ bootefi bootmgr
> > >
> > > Thank you for the information, it is good to know.
> > > My only concern is that user needs to enable "efidebug" command
> > > for this case, since efidebug implies that it is for debug purpose.
> >
> > Yeah, that is the point where I and ex-maintainer have never agreed.
> > So we have no standard UI for UEFI subsystem on U-Boot until now.
> >
> > Just FYI, we can do the same thing in yet another way :)
> >   => env set -e -nv -bs -rt BootNext =0x0001
> >
> > Similarly, BootOrder as well.
> >
> > -Takahiro Akashi
>
> I also think it's safe to drop this patch.  It's indeed an easier way to
> load a specific boot index, however the use of bootnext is clearly defined
> in the spec.  I don't think we need to increase the EFI code more as long
> as it's clear to anyone that setting bootnext (note here it has to be set
> on each reboot) would have the same result.

Thank you for your comment.
I will drop this patch and will use BootNext for the same purpose.

Thanks,
Masahisa Kojima

>
> Cheers
> /Ilias
> >
> > > Thanks,
> > > Masahisa Kojima
> > >
> > > >
> > > > -Takahiro Akashi
> > > >
> > > >
> > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > > ---
> > > > > Changes in v3:
> > > > > - newly created
> > > > >
> > > > >  cmd/bootefi.c                | 42 ++++++++++++++++++++++++++++++++++++
> > > > >  include/efi_loader.h         |  1 +
> > > > >  lib/efi_loader/efi_bootmgr.c |  7 +++---
> > > > >  3 files changed, 46 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > > > > index 46eebd5ee2..df86438fec 100644
> > > > > --- a/cmd/bootefi.c
> > > > > +++ b/cmd/bootefi.c
> > > > > @@ -416,6 +416,30 @@ static int do_efibootmgr(void)
> > > > >       return CMD_RET_SUCCESS;
> > > > >  }
> > > > >
> > > > > +/**
> > > > > + * do_efibootindex() - load and execute the specified Boot#### option
> > > > > + *
> > > > > + * Return:   status code
> > > > > + */
> > > > > +static int do_efibootindex(u16 boot_index)
> > > > > +{
> > > > > +     efi_handle_t handle;
> > > > > +     efi_status_t ret;
> > > > > +     void *load_options;
> > > > > +
> > > > > +     ret = efi_try_load_entry(boot_index, &handle, &load_options);
> > > > > +     if (ret != EFI_SUCCESS) {
> > > > > +             log_notice("EFI boot manager: failed to load Boot%04X\n", boot_index);
> > > > > +             return CMD_RET_FAILURE;
> > > > > +     }
> > > > > +
> > > > > +     ret = do_bootefi_exec(handle, load_options);
> > > > > +
> > > > > +     if (ret != EFI_SUCCESS)
> > > > > +             return CMD_RET_FAILURE;
> > > > > +
> > > > > +     return CMD_RET_SUCCESS;
> > > > > +}
> > > > >  /**
> > > > >   * do_bootefi_image() - execute EFI binary
> > > > >   *
> > > > > @@ -654,6 +678,22 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > >               return CMD_RET_FAILURE;
> > > > >       }
> > > > >
> > > > > +     if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
> > > > > +             if (!strcmp(argv[1], "bootindex")) {
> > > > > +                     char *endp;
> > > > > +                     int boot_index;
> > > > > +
> > > > > +                     if (argc < 3)
> > > > > +                             return CMD_RET_USAGE;
> > > > > +
> > > > > +                     boot_index = (int)hextoul(argv[2], &endp);
> > > > > +                     if (*endp != '\0' || boot_index > 0xffff)
> > > > > +                             return CMD_RET_USAGE;
> > > > > +
> > > > > +                     return do_efibootindex((u16)boot_index);
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > >       if (argc > 2) {
> > > > >               uintptr_t fdt_addr;
> > > > >
> > > > > @@ -702,6 +742,8 @@ static char bootefi_help_text[] =
> > > > >       "\n"
> > > > >       "    If specified, the device tree located at <fdt address> gets\n"
> > > > >       "    exposed as EFI configuration table.\n"
> > > > > +     "bootefi bootindex <boot option>\n"
> > > > > +     "  - load and boot EFI payload based on the specified BootXXXX variable.\n"
> > > > >  #endif
> > > > >       ;
> > > > >  #endif
> > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > > > index 80a5f1ec01..e5972f5fee 100644
> > > > > --- a/include/efi_loader.h
> > > > > +++ b/include/efi_loader.h
> > > > > @@ -861,6 +861,7 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
> > > > >                                 efi_uintn_t load_options_size,
> > > > >                                 void *load_options);
> > > > >  efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
> > > > > +efi_status_t efi_try_load_entry(u16 n, efi_handle_t *handle, void **load_options);
> > > > >
> > > > >  /**
> > > > >   * struct efi_image_regions - A list of memory regions
> > > > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > > > > index 8c04ecbdc8..a3060b5c62 100644
> > > > > --- a/lib/efi_loader/efi_bootmgr.c
> > > > > +++ b/lib/efi_loader/efi_bootmgr.c
> > > > > @@ -42,8 +42,7 @@ static const struct efi_runtime_services *rs;
> > > > >   * @load_options:    load options set on the loaded image protocol
> > > > >   * Return:           status code
> > > > >   */
> > > > > -static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> > > > > -                                void **load_options)
> > > > > +efi_status_t efi_try_load_entry(u16 n, efi_handle_t *handle, void **load_options)
> > > > >  {
> > > > >       struct efi_load_option lo;
> > > > >       u16 varname[] = u"Boot0000";
> > > > > @@ -165,7 +164,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
> > > > >               /* load BootNext */
> > > > >               if (ret == EFI_SUCCESS) {
> > > > >                       if (size == sizeof(u16)) {
> > > > > -                             ret = try_load_entry(bootnext, handle,
> > > > > +                             ret = efi_try_load_entry(bootnext, handle,
> > > > >                                                    load_options);
> > > > >                               if (ret == EFI_SUCCESS)
> > > > >                                       return ret;
> > > > > @@ -189,7 +188,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
> > > > >       for (i = 0; i < num; i++) {
> > > > >               log_debug("%s trying to load Boot%04X\n", __func__,
> > > > >                         bootorder[i]);
> > > > > -             ret = try_load_entry(bootorder[i], handle, load_options);
> > > > > +             ret = efi_try_load_entry(bootorder[i], handle, load_options);
> > > > >               if (ret == EFI_SUCCESS)
> > > > >                       break;
> > > > >       }
> > > > > --
> > > > > 2.17.1
> > > > >

Re: [RFC PATCH v3 2/2] bootmenu: add UEFI and disto_boot entries

[Takahiro Akashi]

On Wed, Mar 09, 2022 at 04:34:42PM +0200, Ilias Apalodimas wrote:
> Hi Kojima-san
> 
> On Tue, Mar 08, 2022 at 11:07:45PM +0900, Masahisa Kojima wrote:
> > This commit adds the UEFI related menu entries and
> > distro_boot entries into the bootmenu.
> > 
> > For UEFI, user can select which UEFI "Boot####" option
> > to execute, call UEFI bootmgr and UEFI boot variable
> > maintenance menu. UEFI bootmgr entry is required to
> > correctly handle "BootNext" variable.
> 
> Hmm why? (some more info further down)
> 
> > 
> > For distro_boot, user can select the boot device
> > included in "boot_targets" u-boot environment variable.
> > 
> > The menu example is as follows.
> > 
> >   *** U-Boot Boot Menu ***
> > 
> >      Boot 1. kernel (bootmenu_0)
> >      Boot 2. kernel (bootmenu_1)
> >      Reset board (bootmenu_2)
> >      debian (BOOT0000)
> >      ubuntu (BOOT0001)
> >      UEFI Boot Manager
> >      usb0
> >      scsi0
> >      virtio0
> >      dhcp
> >      UEFI Boot Manager Maintenance
> >      U-Boot console
> > 
> >   Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit
> > 
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Changes in v3:
> > - newly created
> > 
> >  cmd/bootmenu.c | 268 +++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 259 insertions(+), 9 deletions(-)
> > 
> > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> > index 409ef9a848..a8dc50dcaa 100644
> > --- a/cmd/bootmenu.c
> > +++ b/cmd/bootmenu.c
> > @@ -3,9 +3,12 @@
> >   * (C) Copyright 2011-2013 Pali Rohár <pali@kernel.org>
> >   */
> >  
> > +#include <charset.h>
> >  #include <common.h>
> >  #include <command.h>
> >  #include <ansi.h>
> > +#include <efi_loader.h>
> > +#include <efi_variable.h>
> >  #include <env.h>
> >  #include <log.h>
> >  #include <menu.h>
> > @@ -24,11 +27,20 @@
> >   */
> >  #define MAX_ENV_SIZE	(9 + 2 + 1)
> >  
> > +enum boot_type {
> > +	BOOT_TYPE_NONE = 0,
> > +	BOOT_TYPE_BOOTMENU,
> > +	BOOT_TYPE_UEFI,
> > +	BOOT_TYPE_DISTRO_BOOT,
> > +};
> > +
> >  struct bootmenu_entry {
> >  	unsigned short int num;		/* unique number 0 .. MAX_COUNT */
> >  	char key[3];			/* key identifier of number */
> > -	char *title;			/* title of entry */
> > +	u16 *title;			/* title of entry */
> >  	char *command;			/* hush command of entry */
> > +	enum boot_type type;
> > +	u16 bootorder;
> >  	struct bootmenu_data *menu;	/* this bootmenu */
> >  	struct bootmenu_entry *next;	/* next menu entry (num+1) */
> >  };
> > @@ -75,7 +87,12 @@ static void bootmenu_print_entry(void *data)
> >  	if (reverse)
> >  		puts(ANSI_COLOR_REVERSE);
> >  
> > -	puts(entry->title);
> > +	if (entry->type == BOOT_TYPE_BOOTMENU)
> > +		printf("%ls (bootmenu_%d)", entry->title, entry->bootorder);
> > +	else if (entry->type == BOOT_TYPE_UEFI)
> > +		printf("%ls (BOOT%04X)", entry->title, entry->bootorder);
> > +	else
> > +		printf("%ls", entry->title);
> >  
> >  	if (reverse)
> >  		puts(ANSI_COLOR_RESET);
> > @@ -87,6 +104,10 @@ static void bootmenu_autoboot_loop(struct bootmenu_data *menu,
> >  	int i, c;
> >  
> >  	if (menu->delay > 0) {
> > +		/* flush input */
> > +		while (tstc())
> > +			getchar();
> > +
> >  		printf(ANSI_CURSOR_POSITION, menu->count + 5, 1);
> >  		printf("  Hit any key to stop autoboot: %2d ", menu->delay);
> >  	}
> > @@ -300,6 +321,8 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >  		menu->active = (int)simple_strtol(default_str, NULL, 10);
> >  
> >  	while ((option = bootmenu_getoption(i))) {
> > +		u16 *buf;
> > +
> >  		sep = strchr(option, '=');
> >  		if (!sep) {
> >  			printf("Invalid bootmenu entry: %s\n", option);
> > @@ -311,13 +334,13 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >  			goto cleanup;
> >  
> >  		len = sep-option;
> > -		entry->title = malloc(len + 1);
> > +		buf = calloc(1, (len + 1) * sizeof(u16));
> > +		entry->title = buf;
> >  		if (!entry->title) {
> >  			free(entry);
> >  			goto cleanup;
> >  		}
> > -		memcpy(entry->title, option, len);
> > -		entry->title[len] = 0;
> > +		utf8_utf16_strncpy(&buf, option, len);
> >  
> >  		len = strlen(sep + 1);
> >  		entry->command = malloc(len + 1);
> > @@ -333,6 +356,190 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >  
> >  		entry->num = i;
> >  		entry->menu = menu;
> > +		entry->type = BOOT_TYPE_BOOTMENU;
> > +		entry->bootorder = i;
> > +		entry->next = NULL;
> > +
> > +		if (!iter)
> > +			menu->first = entry;
> > +		else
> > +			iter->next = entry;
> > +
> > +		iter = entry;
> > +		++i;
> > +
> > +		if (i == MAX_COUNT - 1)
> > +			break;
> > +	}
> > +
> > +{
> > +	u16 *bootorder;
> > +	efi_status_t ret;
> > +	unsigned short j;
> > +	efi_uintn_t num, size;
> > +	void *load_option;
> > +	struct efi_load_option lo;
> > +	u16 varname[] = u"Boot####";
> > +
> > +	/* Initialize EFI drivers */
> > +	ret = efi_init_obj_list();
> > +	if (ret != EFI_SUCCESS) {
> > +		log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> > +			ret & ~EFI_ERROR_MASK);
> > +		goto cleanup;
> > +	}
> > +
> > +	bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
> > +	if (!bootorder)
> > +		goto bootmgr;
> > +
> > +	num = size / sizeof(u16);
> > +	for (j = 0; j < num; j++) {
> > +		entry = malloc(sizeof(struct bootmenu_entry));
> > +		if (!entry)
> > +			goto cleanup;
> > +
> > +		efi_create_indexed_name(varname, sizeof(varname),
> > +					"Boot", bootorder[j]);
> > +		load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
> > +		if (!load_option)
> > +			continue;
> > +
> > +		ret = efi_deserialize_load_option(&lo, load_option, &size);
> > +		if (ret != EFI_SUCCESS) {
> > +			log_warning("Invalid load option for %ls\n", varname);
> > +			free(load_option);
> > +			continue;
> > +		}
> > +
> > +		if (lo.attributes & LOAD_OPTION_ACTIVE) {
> > +			char *command;
> > +			int command_size;
> > +
> > +			entry->title = u16_strdup(lo.label);
> > +			if (!entry->title) {
> > +				free(load_option);
> > +				free(entry);
> > +				goto cleanup;
> > +			}
> > +			command_size = strlen("bootefi bootindex XXXX") + 1;
> 
> As we commented on patch 2/2 you can simply set BootNext variable here and
> fire up the efibootmgr
> 
> > +			command = calloc(1, command_size);
> > +			if (!command) {
> > +				free(entry->title);
> > +				free(load_option);
> > +				free(entry);
> > +				goto cleanup;
> > +			}
> > +			snprintf(command, command_size, "bootefi bootindex %X", bootorder[j]);
> > +			entry->command = command;
> > +			sprintf(entry->key, "%d", i);
> > +			entry->num = i;
> > +			entry->menu = menu;
> > +			entry->type = BOOT_TYPE_UEFI;
> > +			entry->bootorder = bootorder[j];
> > +			entry->next = NULL;
> > +
> > +			if (!iter)
> > +				menu->first = entry;
> > +			else
> > +				iter->next = entry;
> > +
> > +			iter = entry;
> > +			++i;
> > +		}
> > +
> > +		if (i == MAX_COUNT - 1)
> > +			break;
> > +	}
> > +	free(bootorder);
> > +}
> > +
> > +bootmgr:
> 
> Why do we need an entire menu entry if the bootorder is not defined?
> Currently there's no logic in the efibootmgr to look for the standard
> filenames in the ESP (eg bootarm.efi, bootaa64.efi etc) if no boot option
> is defined.  Instead this is implement in distro_bootcmd.

To be clear, "if no boot option is defined" is wrong.
What the specification requires is that, if a file name is missing
in a device path defined in a boot option, a default path/name must be
appended to the path before trying to load an image from that media.

> I was thinking of something along the lines of:
> 1. bootmenu comes up
> 2. We read all the Boot#### variables that are defined and add them on the
>    menu 
> 2a. If the user doesn't explicitly choose a boot option we run 'bootefi bootmgr'
> 2b. If the user does select a different option we set BootNext and still
> run 'bootefi bootmgr'
> 3. If there's not Boot#### option defined,  we should in the future add the
>    functionality of searching for bootaa64.efi etc in ESP and still just 
>    launch the efi bootmgr
> 
> > +	/* Add UEFI Boot Manager entry if available */
> > +	if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
> > +		if (i <= MAX_COUNT - 1) {
> > +			entry = malloc(sizeof(struct bootmenu_entry));
> > +			if (!entry)
> > +				goto cleanup;
> > +
> > +			entry->title = u16_strdup(u"UEFI Boot Manager");
> > +			if (!entry->title) {
> > +				free(entry);
> > +				goto cleanup;
> > +			}
> > +
> > +			entry->command = strdup("bootefi bootmgr");
> > +			if (!entry->command) {
> > +				free(entry->title);
> > +				free(entry);
> > +				goto cleanup;
> > +			}
> > +
> > +			sprintf(entry->key, "%d", i);
> > +
> > +			entry->num = i;
> > +			entry->menu = menu;
> > +			entry->type = BOOT_TYPE_NONE;
> > +			entry->next = NULL;
> > +
> > +			if (!iter)
> > +				menu->first = entry;
> > +			else
> > +				iter->next = entry;
> > +
> > +			iter = entry;
> > +			++i;
> > +		}
> > +	}
> > +
> > +{
> > +	char *p;
> > +	char *token;
> > +	char *boot_targets;
> > +	int len;
> > +
> > +	/* list the distro boot "boot_targets" */
> > +	boot_targets = env_get("boot_targets");
> > +	if (!boot_targets)
> > +		goto exit_boot_targets;
> > +
> > +	len = strlen(boot_targets);
> > +	p = calloc(1, len + 1);
> > +	strncpy(p, boot_targets, len);
> > +
> > +	token = strtok(p, " ");
> > +
> > +	do {
> > +		u16 *buf;
> > +		char *command;
> > +		int command_size;
> > +
> > +		entry = malloc(sizeof(struct bootmenu_entry));
> > +		if (!entry)
> > +			goto cleanup;
> > +
> > +		len = strlen(token);
> > +		buf = calloc(1, (len + 1) * sizeof(u16));
> > +		entry->title = buf;
> > +		if (!entry->title) {
> > +			free(entry);
> > +			goto cleanup;
> > +		}
> > +		utf8_utf16_strncpy(&buf, token,len);
> > +		sprintf(entry->key, "%d", i);
> > +		entry->num = i;
> > +		entry->menu = menu;
> > +
> > +		command_size = strlen("run bootcmd_") + len + 1;
> 
> I think we are better of here with a sizeof() instead of strlen since the
> 'run bootcmd_' string is not expected to change
> 
> > +		command = calloc(1, command_size);
> > +		if (!command) {
> > +			free(entry->title);
> > +			free(entry);
> > +			goto cleanup;
> > +		}
> > +		snprintf(command, command_size, "run bootcmd_%s", token);
> > +		entry->command = command;
> > +		entry->type = BOOT_TYPE_DISTRO_BOOT;
> >  		entry->next = NULL;
> >  
> >  		if (!iter)
> > @@ -345,6 +552,48 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >  
> >  		if (i == MAX_COUNT - 1)
> >  			break;
> > +
> > +		token = strtok(NULL, " ");
> > +	} while (token);
> > +
> > +	free(p);
> > +}
> > +
> > +exit_boot_targets:
> > +
> > +	/* Add UEFI Boot Manager Maintenance entry */
> > +	if (i <= MAX_COUNT - 1) {
> > +		entry = malloc(sizeof(struct bootmenu_entry));
> > +		if (!entry)
> > +			goto cleanup;
> > +
> > +		entry->title = u16_strdup(u"UEFI Boot Manager Maintenance");
> > +		if (!entry->title) {
> > +			free(entry);
> > +			goto cleanup;
> > +		}
> > +
> > +		entry->command = strdup("echo TODO: Not implemented");
> > +		if (!entry->command) {
> > +			free(entry->title);
> > +			free(entry);
> > +			goto cleanup;
> > +		}
> 
> Any idea of how we'll tackle that?  We could export the efibootmgr
> functions that deal with this and use them on the edit menu ?

# efibootmgr, if it means efi_bootmgr.c, doesn't have such a function.

I hope that it is just a matter of time :)

I think that the display format of the menu should be improved so that users
easily understand that the list of UEFI boot options are listed in the order
that "BootOrder" specifies.

-Takahiro Akashi

> 
> > +
> > +		sprintf(entry->key, "%d", i);
> > +
> > +		entry->num = i;
> > +		entry->menu = menu;
> > +		entry->type = BOOT_TYPE_NONE;
> > +		entry->next = NULL;
> > +
> > +		if (!iter)
> > +			menu->first = entry;
> > +		else
> > +			iter->next = entry;
> > +
> > +		iter = entry;
> > +		++i;
> >  	}
> >  
> >  	/* Add U-Boot console entry at the end */
> > @@ -353,7 +602,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >  		if (!entry)
> >  			goto cleanup;
> >  
> > -		entry->title = strdup("U-Boot console");
> > +		entry->title = u16_strdup(u"U-Boot console");
> 
> Can we add an option to prohibit the user from going back to the console?
> 
> >  		if (!entry->title) {
> >  			free(entry);
> >  			goto cleanup;
> > @@ -370,6 +619,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >  
> >  		entry->num = i;
> >  		entry->menu = menu;
> > +		entry->type = BOOT_TYPE_NONE;
> >  		entry->next = NULL;
> >  
> >  		if (!iter)
> > @@ -427,7 +677,7 @@ static void bootmenu_show(int delay)
> >  {
> >  	int init = 0;
> >  	void *choice = NULL;
> > -	char *title = NULL;
> > +	u16 *title = NULL;
> >  	char *command = NULL;
> >  	struct menu *menu;
> >  	struct bootmenu_data *bootmenu;
> > @@ -478,7 +728,7 @@ static void bootmenu_show(int delay)
> >  
> >  	if (menu_get_choice(menu, &choice)) {
> >  		iter = choice;
> > -		title = strdup(iter->title);
> > +		title = u16_strdup(iter->title);
> >  		command = strdup(iter->command);
> >  	}
> >  
> > @@ -493,7 +743,7 @@ cleanup:
> >  	}
> >  
> >  	if (title && command) {
> > -		debug("Starting entry '%s'\n", title);
> > +		debug("Starting entry '%ls'\n", title);
> >  		free(title);
> >  		run_command(command, 0);
> >  		free(command);
> > -- 
> > 2.17.1
> > 
> 
> Cheers
> /Ilias

Re: [RFC PATCH v3 2/2] bootmenu: add UEFI and disto_boot entries

[Takahiro Akashi]

On Thu, Mar 10, 2022 at 10:50:57AM +0900, Takahiro Akashi wrote:
> On Wed, Mar 09, 2022 at 04:34:42PM +0200, Ilias Apalodimas wrote:
> > Hi Kojima-san
> > 
> > On Tue, Mar 08, 2022 at 11:07:45PM +0900, Masahisa Kojima wrote:
> > > This commit adds the UEFI related menu entries and
> > > distro_boot entries into the bootmenu.
> > > 
> > > For UEFI, user can select which UEFI "Boot####" option
> > > to execute, call UEFI bootmgr and UEFI boot variable
> > > maintenance menu. UEFI bootmgr entry is required to
> > > correctly handle "BootNext" variable.
> > 
> > Hmm why? (some more info further down)
> > 
> > > 
> > > For distro_boot, user can select the boot device
> > > included in "boot_targets" u-boot environment variable.
> > > 
> > > The menu example is as follows.
> > > 
> > >   *** U-Boot Boot Menu ***
> > > 
> > >      Boot 1. kernel (bootmenu_0)
> > >      Boot 2. kernel (bootmenu_1)
> > >      Reset board (bootmenu_2)
> > >      debian (BOOT0000)
> > >      ubuntu (BOOT0001)
> > >      UEFI Boot Manager
> > >      usb0
> > >      scsi0
> > >      virtio0
> > >      dhcp
> > >      UEFI Boot Manager Maintenance
> > >      U-Boot console
> > > 
> > >   Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit
> > > 
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > ---
> > > Changes in v3:
> > > - newly created
> > > 
> > >  cmd/bootmenu.c | 268 +++++++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 259 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> > > index 409ef9a848..a8dc50dcaa 100644
> > > --- a/cmd/bootmenu.c
> > > +++ b/cmd/bootmenu.c
> > > @@ -3,9 +3,12 @@
> > >   * (C) Copyright 2011-2013 Pali Rohár <pali@kernel.org>
> > >   */
> > >  
> > > +#include <charset.h>
> > >  #include <common.h>
> > >  #include <command.h>
> > >  #include <ansi.h>
> > > +#include <efi_loader.h>
> > > +#include <efi_variable.h>
> > >  #include <env.h>
> > >  #include <log.h>
> > >  #include <menu.h>
> > > @@ -24,11 +27,20 @@
> > >   */
> > >  #define MAX_ENV_SIZE	(9 + 2 + 1)
> > >  
> > > +enum boot_type {
> > > +	BOOT_TYPE_NONE = 0,
> > > +	BOOT_TYPE_BOOTMENU,
> > > +	BOOT_TYPE_UEFI,
> > > +	BOOT_TYPE_DISTRO_BOOT,
> > > +};
> > > +
> > >  struct bootmenu_entry {
> > >  	unsigned short int num;		/* unique number 0 .. MAX_COUNT */
> > >  	char key[3];			/* key identifier of number */
> > > -	char *title;			/* title of entry */
> > > +	u16 *title;			/* title of entry */
> > >  	char *command;			/* hush command of entry */
> > > +	enum boot_type type;
> > > +	u16 bootorder;
> > >  	struct bootmenu_data *menu;	/* this bootmenu */
> > >  	struct bootmenu_entry *next;	/* next menu entry (num+1) */
> > >  };
> > > @@ -75,7 +87,12 @@ static void bootmenu_print_entry(void *data)
> > >  	if (reverse)
> > >  		puts(ANSI_COLOR_REVERSE);
> > >  
> > > -	puts(entry->title);
> > > +	if (entry->type == BOOT_TYPE_BOOTMENU)
> > > +		printf("%ls (bootmenu_%d)", entry->title, entry->bootorder);
> > > +	else if (entry->type == BOOT_TYPE_UEFI)
> > > +		printf("%ls (BOOT%04X)", entry->title, entry->bootorder);
> > > +	else
> > > +		printf("%ls", entry->title);
> > >  
> > >  	if (reverse)
> > >  		puts(ANSI_COLOR_RESET);
> > > @@ -87,6 +104,10 @@ static void bootmenu_autoboot_loop(struct bootmenu_data *menu,
> > >  	int i, c;
> > >  
> > >  	if (menu->delay > 0) {
> > > +		/* flush input */
> > > +		while (tstc())
> > > +			getchar();
> > > +
> > >  		printf(ANSI_CURSOR_POSITION, menu->count + 5, 1);
> > >  		printf("  Hit any key to stop autoboot: %2d ", menu->delay);
> > >  	}
> > > @@ -300,6 +321,8 @@ static struct bootmenu_data *bootmenu_create(int delay)
> > >  		menu->active = (int)simple_strtol(default_str, NULL, 10);
> > >  
> > >  	while ((option = bootmenu_getoption(i))) {
> > > +		u16 *buf;
> > > +
> > >  		sep = strchr(option, '=');
> > >  		if (!sep) {
> > >  			printf("Invalid bootmenu entry: %s\n", option);
> > > @@ -311,13 +334,13 @@ static struct bootmenu_data *bootmenu_create(int delay)
> > >  			goto cleanup;
> > >  
> > >  		len = sep-option;
> > > -		entry->title = malloc(len + 1);
> > > +		buf = calloc(1, (len + 1) * sizeof(u16));
> > > +		entry->title = buf;
> > >  		if (!entry->title) {
> > >  			free(entry);
> > >  			goto cleanup;
> > >  		}
> > > -		memcpy(entry->title, option, len);
> > > -		entry->title[len] = 0;
> > > +		utf8_utf16_strncpy(&buf, option, len);
> > >  
> > >  		len = strlen(sep + 1);
> > >  		entry->command = malloc(len + 1);
> > > @@ -333,6 +356,190 @@ static struct bootmenu_data *bootmenu_create(int delay)
> > >  
> > >  		entry->num = i;
> > >  		entry->menu = menu;
> > > +		entry->type = BOOT_TYPE_BOOTMENU;
> > > +		entry->bootorder = i;
> > > +		entry->next = NULL;
> > > +
> > > +		if (!iter)
> > > +			menu->first = entry;
> > > +		else
> > > +			iter->next = entry;
> > > +
> > > +		iter = entry;
> > > +		++i;
> > > +
> > > +		if (i == MAX_COUNT - 1)
> > > +			break;
> > > +	}
> > > +
> > > +{
> > > +	u16 *bootorder;
> > > +	efi_status_t ret;
> > > +	unsigned short j;
> > > +	efi_uintn_t num, size;
> > > +	void *load_option;
> > > +	struct efi_load_option lo;
> > > +	u16 varname[] = u"Boot####";
> > > +
> > > +	/* Initialize EFI drivers */
> > > +	ret = efi_init_obj_list();
> > > +	if (ret != EFI_SUCCESS) {
> > > +		log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> > > +			ret & ~EFI_ERROR_MASK);
> > > +		goto cleanup;
> > > +	}
> > > +
> > > +	bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
> > > +	if (!bootorder)
> > > +		goto bootmgr;
> > > +
> > > +	num = size / sizeof(u16);
> > > +	for (j = 0; j < num; j++) {
> > > +		entry = malloc(sizeof(struct bootmenu_entry));
> > > +		if (!entry)
> > > +			goto cleanup;
> > > +
> > > +		efi_create_indexed_name(varname, sizeof(varname),
> > > +					"Boot", bootorder[j]);
> > > +		load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
> > > +		if (!load_option)
> > > +			continue;
> > > +
> > > +		ret = efi_deserialize_load_option(&lo, load_option, &size);
> > > +		if (ret != EFI_SUCCESS) {
> > > +			log_warning("Invalid load option for %ls\n", varname);
> > > +			free(load_option);
> > > +			continue;
> > > +		}
> > > +
> > > +		if (lo.attributes & LOAD_OPTION_ACTIVE) {
> > > +			char *command;
> > > +			int command_size;
> > > +
> > > +			entry->title = u16_strdup(lo.label);
> > > +			if (!entry->title) {
> > > +				free(load_option);
> > > +				free(entry);
> > > +				goto cleanup;
> > > +			}
> > > +			command_size = strlen("bootefi bootindex XXXX") + 1;
> > 
> > As we commented on patch 2/2 you can simply set BootNext variable here and
> > fire up the efibootmgr
> > 
> > > +			command = calloc(1, command_size);
> > > +			if (!command) {
> > > +				free(entry->title);
> > > +				free(load_option);
> > > +				free(entry);
> > > +				goto cleanup;
> > > +			}
> > > +			snprintf(command, command_size, "bootefi bootindex %X", bootorder[j]);
> > > +			entry->command = command;
> > > +			sprintf(entry->key, "%d", i);
> > > +			entry->num = i;
> > > +			entry->menu = menu;
> > > +			entry->type = BOOT_TYPE_UEFI;
> > > +			entry->bootorder = bootorder[j];
> > > +			entry->next = NULL;
> > > +
> > > +			if (!iter)
> > > +				menu->first = entry;
> > > +			else
> > > +				iter->next = entry;
> > > +
> > > +			iter = entry;
> > > +			++i;
> > > +		}
> > > +
> > > +		if (i == MAX_COUNT - 1)
> > > +			break;
> > > +	}
> > > +	free(bootorder);
> > > +}
> > > +
> > > +bootmgr:
> > 
> > Why do we need an entire menu entry if the bootorder is not defined?
> > Currently there's no logic in the efibootmgr to look for the standard
> > filenames in the ESP (eg bootarm.efi, bootaa64.efi etc) if no boot option
> > is defined.  Instead this is implement in distro_bootcmd.
> 
> To be clear, "if no boot option is defined" is wrong.
> What the specification requires is that, if a file name is missing
> in a device path defined in a boot option, a default path/name must be
> appended to the path before trying to load an image from that media.

One more thing:
What the current distro_bootcmd does is to try to a binary with the default
file name in an order specified by "boot_targets" variable.
Here is another inconsistency with UEFI specification.

-Takahiro Akashi


> > I was thinking of something along the lines of:
> > 1. bootmenu comes up
> > 2. We read all the Boot#### variables that are defined and add them on the
> >    menu 
> > 2a. If the user doesn't explicitly choose a boot option we run 'bootefi bootmgr'
> > 2b. If the user does select a different option we set BootNext and still
> > run 'bootefi bootmgr'
> > 3. If there's not Boot#### option defined,  we should in the future add the
> >    functionality of searching for bootaa64.efi etc in ESP and still just 
> >    launch the efi bootmgr
> > 
> > > +	/* Add UEFI Boot Manager entry if available */
> > > +	if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
> > > +		if (i <= MAX_COUNT - 1) {
> > > +			entry = malloc(sizeof(struct bootmenu_entry));
> > > +			if (!entry)
> > > +				goto cleanup;
> > > +
> > > +			entry->title = u16_strdup(u"UEFI Boot Manager");
> > > +			if (!entry->title) {
> > > +				free(entry);
> > > +				goto cleanup;
> > > +			}
> > > +
> > > +			entry->command = strdup("bootefi bootmgr");
> > > +			if (!entry->command) {
> > > +				free(entry->title);
> > > +				free(entry);
> > > +				goto cleanup;
> > > +			}
> > > +
> > > +			sprintf(entry->key, "%d", i);
> > > +
> > > +			entry->num = i;
> > > +			entry->menu = menu;
> > > +			entry->type = BOOT_TYPE_NONE;
> > > +			entry->next = NULL;
> > > +
> > > +			if (!iter)
> > > +				menu->first = entry;
> > > +			else
> > > +				iter->next = entry;
> > > +
> > > +			iter = entry;
> > > +			++i;
> > > +		}
> > > +	}
> > > +
> > > +{
> > > +	char *p;
> > > +	char *token;
> > > +	char *boot_targets;
> > > +	int len;
> > > +
> > > +	/* list the distro boot "boot_targets" */
> > > +	boot_targets = env_get("boot_targets");
> > > +	if (!boot_targets)
> > > +		goto exit_boot_targets;
> > > +
> > > +	len = strlen(boot_targets);
> > > +	p = calloc(1, len + 1);
> > > +	strncpy(p, boot_targets, len);
> > > +
> > > +	token = strtok(p, " ");
> > > +
> > > +	do {
> > > +		u16 *buf;
> > > +		char *command;
> > > +		int command_size;
> > > +
> > > +		entry = malloc(sizeof(struct bootmenu_entry));
> > > +		if (!entry)
> > > +			goto cleanup;
> > > +
> > > +		len = strlen(token);
> > > +		buf = calloc(1, (len + 1) * sizeof(u16));
> > > +		entry->title = buf;
> > > +		if (!entry->title) {
> > > +			free(entry);
> > > +			goto cleanup;
> > > +		}
> > > +		utf8_utf16_strncpy(&buf, token,len);
> > > +		sprintf(entry->key, "%d", i);
> > > +		entry->num = i;
> > > +		entry->menu = menu;
> > > +
> > > +		command_size = strlen("run bootcmd_") + len + 1;
> > 
> > I think we are better of here with a sizeof() instead of strlen since the
> > 'run bootcmd_' string is not expected to change
> > 
> > > +		command = calloc(1, command_size);
> > > +		if (!command) {
> > > +			free(entry->title);
> > > +			free(entry);
> > > +			goto cleanup;
> > > +		}
> > > +		snprintf(command, command_size, "run bootcmd_%s", token);
> > > +		entry->command = command;
> > > +		entry->type = BOOT_TYPE_DISTRO_BOOT;
> > >  		entry->next = NULL;
> > >  
> > >  		if (!iter)
> > > @@ -345,6 +552,48 @@ static struct bootmenu_data *bootmenu_create(int delay)
> > >  
> > >  		if (i == MAX_COUNT - 1)
> > >  			break;
> > > +
> > > +		token = strtok(NULL, " ");
> > > +	} while (token);
> > > +
> > > +	free(p);
> > > +}
> > > +
> > > +exit_boot_targets:
> > > +
> > > +	/* Add UEFI Boot Manager Maintenance entry */
> > > +	if (i <= MAX_COUNT - 1) {
> > > +		entry = malloc(sizeof(struct bootmenu_entry));
> > > +		if (!entry)
> > > +			goto cleanup;
> > > +
> > > +		entry->title = u16_strdup(u"UEFI Boot Manager Maintenance");
> > > +		if (!entry->title) {
> > > +			free(entry);
> > > +			goto cleanup;
> > > +		}
> > > +
> > > +		entry->command = strdup("echo TODO: Not implemented");
> > > +		if (!entry->command) {
> > > +			free(entry->title);
> > > +			free(entry);
> > > +			goto cleanup;
> > > +		}
> > 
> > Any idea of how we'll tackle that?  We could export the efibootmgr
> > functions that deal with this and use them on the edit menu ?
> 
> # efibootmgr, if it means efi_bootmgr.c, doesn't have such a function.
> 
> I hope that it is just a matter of time :)
> 
> I think that the display format of the menu should be improved so that users
> easily understand that the list of UEFI boot options are listed in the order
> that "BootOrder" specifies.
> 
> -Takahiro Akashi
> 
> > 
> > > +
> > > +		sprintf(entry->key, "%d", i);
> > > +
> > > +		entry->num = i;
> > > +		entry->menu = menu;
> > > +		entry->type = BOOT_TYPE_NONE;
> > > +		entry->next = NULL;
> > > +
> > > +		if (!iter)
> > > +			menu->first = entry;
> > > +		else
> > > +			iter->next = entry;
> > > +
> > > +		iter = entry;
> > > +		++i;
> > >  	}
> > >  
> > >  	/* Add U-Boot console entry at the end */
> > > @@ -353,7 +602,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> > >  		if (!entry)
> > >  			goto cleanup;
> > >  
> > > -		entry->title = strdup("U-Boot console");
> > > +		entry->title = u16_strdup(u"U-Boot console");
> > 
> > Can we add an option to prohibit the user from going back to the console?
> > 
> > >  		if (!entry->title) {
> > >  			free(entry);
> > >  			goto cleanup;
> > > @@ -370,6 +619,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> > >  
> > >  		entry->num = i;
> > >  		entry->menu = menu;
> > > +		entry->type = BOOT_TYPE_NONE;
> > >  		entry->next = NULL;
> > >  
> > >  		if (!iter)
> > > @@ -427,7 +677,7 @@ static void bootmenu_show(int delay)
> > >  {
> > >  	int init = 0;
> > >  	void *choice = NULL;
> > > -	char *title = NULL;
> > > +	u16 *title = NULL;
> > >  	char *command = NULL;
> > >  	struct menu *menu;
> > >  	struct bootmenu_data *bootmenu;
> > > @@ -478,7 +728,7 @@ static void bootmenu_show(int delay)
> > >  
> > >  	if (menu_get_choice(menu, &choice)) {
> > >  		iter = choice;
> > > -		title = strdup(iter->title);
> > > +		title = u16_strdup(iter->title);
> > >  		command = strdup(iter->command);
> > >  	}
> > >  
> > > @@ -493,7 +743,7 @@ cleanup:
> > >  	}
> > >  
> > >  	if (title && command) {
> > > -		debug("Starting entry '%s'\n", title);
> > > +		debug("Starting entry '%ls'\n", title);
> > >  		free(title);
> > >  		run_command(command, 0);
> > >  		free(command);
> > > -- 
> > > 2.17.1
> > > 
> > 
> > Cheers
> > /Ilias

Re: [RFC PATCH v3 2/2] bootmenu: add UEFI and disto_boot entries

[Masahisa Kojima]

Hi Ilias,

On Wed, 9 Mar 2022 at 23:34, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Kojima-san
>
> On Tue, Mar 08, 2022 at 11:07:45PM +0900, Masahisa Kojima wrote:
> > This commit adds the UEFI related menu entries and
> > distro_boot entries into the bootmenu.
> >
> > For UEFI, user can select which UEFI "Boot####" option
> > to execute, call UEFI bootmgr and UEFI boot variable
> > maintenance menu. UEFI bootmgr entry is required to
> > correctly handle "BootNext" variable.
>
> Hmm why? (some more info further down)
>
> >
> > For distro_boot, user can select the boot device
> > included in "boot_targets" u-boot environment variable.
> >
> > The menu example is as follows.
> >
> >   *** U-Boot Boot Menu ***
> >
> >      Boot 1. kernel (bootmenu_0)
> >      Boot 2. kernel (bootmenu_1)
> >      Reset board (bootmenu_2)
> >      debian (BOOT0000)
> >      ubuntu (BOOT0001)
> >      UEFI Boot Manager
> >      usb0
> >      scsi0
> >      virtio0
> >      dhcp
> >      UEFI Boot Manager Maintenance
> >      U-Boot console
> >
> >   Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Changes in v3:
> > - newly created
> >
> >  cmd/bootmenu.c | 268 +++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 259 insertions(+), 9 deletions(-)
> >
> > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> > index 409ef9a848..a8dc50dcaa 100644
> > --- a/cmd/bootmenu.c
> > +++ b/cmd/bootmenu.c
> > @@ -3,9 +3,12 @@
> >   * (C) Copyright 2011-2013 Pali Rohár <pali@kernel.org>
> >   */
> >
> > +#include <charset.h>
> >  #include <common.h>
> >  #include <command.h>
> >  #include <ansi.h>
> > +#include <efi_loader.h>
> > +#include <efi_variable.h>
> >  #include <env.h>
> >  #include <log.h>
> >  #include <menu.h>
> > @@ -24,11 +27,20 @@
> >   */
> >  #define MAX_ENV_SIZE (9 + 2 + 1)
> >
> > +enum boot_type {
> > +     BOOT_TYPE_NONE = 0,
> > +     BOOT_TYPE_BOOTMENU,
> > +     BOOT_TYPE_UEFI,
> > +     BOOT_TYPE_DISTRO_BOOT,
> > +};
> > +
> >  struct bootmenu_entry {
> >       unsigned short int num;         /* unique number 0 .. MAX_COUNT */
> >       char key[3];                    /* key identifier of number */
> > -     char *title;                    /* title of entry */
> > +     u16 *title;                     /* title of entry */
> >       char *command;                  /* hush command of entry */
> > +     enum boot_type type;
> > +     u16 bootorder;
> >       struct bootmenu_data *menu;     /* this bootmenu */
> >       struct bootmenu_entry *next;    /* next menu entry (num+1) */
> >  };
> > @@ -75,7 +87,12 @@ static void bootmenu_print_entry(void *data)
> >       if (reverse)
> >               puts(ANSI_COLOR_REVERSE);
> >
> > -     puts(entry->title);
> > +     if (entry->type == BOOT_TYPE_BOOTMENU)
> > +             printf("%ls (bootmenu_%d)", entry->title, entry->bootorder);
> > +     else if (entry->type == BOOT_TYPE_UEFI)
> > +             printf("%ls (BOOT%04X)", entry->title, entry->bootorder);
> > +     else
> > +             printf("%ls", entry->title);
> >
> >       if (reverse)
> >               puts(ANSI_COLOR_RESET);
> > @@ -87,6 +104,10 @@ static void bootmenu_autoboot_loop(struct bootmenu_data *menu,
> >       int i, c;
> >
> >       if (menu->delay > 0) {
> > +             /* flush input */
> > +             while (tstc())
> > +                     getchar();
> > +
> >               printf(ANSI_CURSOR_POSITION, menu->count + 5, 1);
> >               printf("  Hit any key to stop autoboot: %2d ", menu->delay);
> >       }
> > @@ -300,6 +321,8 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >               menu->active = (int)simple_strtol(default_str, NULL, 10);
> >
> >       while ((option = bootmenu_getoption(i))) {
> > +             u16 *buf;
> > +
> >               sep = strchr(option, '=');
> >               if (!sep) {
> >                       printf("Invalid bootmenu entry: %s\n", option);
> > @@ -311,13 +334,13 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >                       goto cleanup;
> >
> >               len = sep-option;
> > -             entry->title = malloc(len + 1);
> > +             buf = calloc(1, (len + 1) * sizeof(u16));
> > +             entry->title = buf;
> >               if (!entry->title) {
> >                       free(entry);
> >                       goto cleanup;
> >               }
> > -             memcpy(entry->title, option, len);
> > -             entry->title[len] = 0;
> > +             utf8_utf16_strncpy(&buf, option, len);
> >
> >               len = strlen(sep + 1);
> >               entry->command = malloc(len + 1);
> > @@ -333,6 +356,190 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >
> >               entry->num = i;
> >               entry->menu = menu;
> > +             entry->type = BOOT_TYPE_BOOTMENU;
> > +             entry->bootorder = i;
> > +             entry->next = NULL;
> > +
> > +             if (!iter)
> > +                     menu->first = entry;
> > +             else
> > +                     iter->next = entry;
> > +
> > +             iter = entry;
> > +             ++i;
> > +
> > +             if (i == MAX_COUNT - 1)
> > +                     break;
> > +     }
> > +
> > +{
> > +     u16 *bootorder;
> > +     efi_status_t ret;
> > +     unsigned short j;
> > +     efi_uintn_t num, size;
> > +     void *load_option;
> > +     struct efi_load_option lo;
> > +     u16 varname[] = u"Boot####";
> > +
> > +     /* Initialize EFI drivers */
> > +     ret = efi_init_obj_list();
> > +     if (ret != EFI_SUCCESS) {
> > +             log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> > +                     ret & ~EFI_ERROR_MASK);
> > +             goto cleanup;
> > +     }
> > +
> > +     bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
> > +     if (!bootorder)
> > +             goto bootmgr;
> > +
> > +     num = size / sizeof(u16);
> > +     for (j = 0; j < num; j++) {
> > +             entry = malloc(sizeof(struct bootmenu_entry));
> > +             if (!entry)
> > +                     goto cleanup;
> > +
> > +             efi_create_indexed_name(varname, sizeof(varname),
> > +                                     "Boot", bootorder[j]);
> > +             load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
> > +             if (!load_option)
> > +                     continue;
> > +
> > +             ret = efi_deserialize_load_option(&lo, load_option, &size);
> > +             if (ret != EFI_SUCCESS) {
> > +                     log_warning("Invalid load option for %ls\n", varname);
> > +                     free(load_option);
> > +                     continue;
> > +             }
> > +
> > +             if (lo.attributes & LOAD_OPTION_ACTIVE) {
> > +                     char *command;
> > +                     int command_size;
> > +
> > +                     entry->title = u16_strdup(lo.label);
> > +                     if (!entry->title) {
> > +                             free(load_option);
> > +                             free(entry);
> > +                             goto cleanup;
> > +                     }
> > +                     command_size = strlen("bootefi bootindex XXXX") + 1;
>
> As we commented on patch 2/2 you can simply set BootNext variable here and
> fire up the efibootmgr

Yes, I will do this.

>
> > +                     command = calloc(1, command_size);
> > +                     if (!command) {
> > +                             free(entry->title);
> > +                             free(load_option);
> > +                             free(entry);
> > +                             goto cleanup;
> > +                     }
> > +                     snprintf(command, command_size, "bootefi bootindex %X", bootorder[j]);
> > +                     entry->command = command;
> > +                     sprintf(entry->key, "%d", i);
> > +                     entry->num = i;
> > +                     entry->menu = menu;
> > +                     entry->type = BOOT_TYPE_UEFI;
> > +                     entry->bootorder = bootorder[j];
> > +                     entry->next = NULL;
> > +
> > +                     if (!iter)
> > +                             menu->first = entry;
> > +                     else
> > +                             iter->next = entry;
> > +
> > +                     iter = entry;
> > +                     ++i;
> > +             }
> > +
> > +             if (i == MAX_COUNT - 1)
> > +                     break;
> > +     }
> > +     free(bootorder);
> > +}
> > +
> > +bootmgr:
>
> Why do we need an entire menu entry if the bootorder is not defined?
> Currently there's no logic in the efibootmgr to look for the standard
> filenames in the ESP (eg bootarm.efi, bootaa64.efi etc) if no boot option
> is defined.  Instead this is implement in distro_bootcmd.

In this version, newly added command "bootefi bootindex XXXX" is called
if the user selects one of the "Boot####" option, and "bootefi bootindex"
does not handle "BootNext". That's why I think the menu entry simply
call "bootefi bootmgr" is required.
Anyway, I will modify to set "BootNext" and call "bootefi bootmgr" if
the user selects a "Boot####" option.

I think we need to consider the case that "BootNext" is already set
when bootmenu comes up.
In this case, bootmenu must automatically try to boot the load option
of "BootNext" without any user interaction.

>
> I was thinking of something along the lines of:
> 1. bootmenu comes up
> 2. We read all the Boot#### variables that are defined and add them on the
>    menu
> 2a. If the user doesn't explicitly choose a boot option we run 'bootefi bootmgr'
> 2b. If the user does select a different option we set BootNext and still
> run 'bootefi bootmgr'
> 3. If there's not Boot#### option defined,  we should in the future add the
>    functionality of searching for bootaa64.efi etc in ESP and still just
>    launch the efi bootmgr

Thank you very much for summarizing.
I would like to update as follows.

1. bootmenu comes up
2. If the BootNext is already defined, try to boot with BootNext
without showing bootmenu
3. We read the BootOrder, then read Boot#### with the order specified
by BootOrder and add it on the menu (UEFI spec requires to honor the
priorities set in BootOrder variable)
3a. If the user doesn't explicitly choose a boot option we run 'bootefi bootmgr'
3b. If the user does select a different option we set BootNext and still
run 'bootefi bootmgr'
4. If there's not Boot#### option defined,  we should in the future add the
   functionality of searching for bootaa64.efi etc in ESP and still just
   launch the efi bootmgr
4a. If "bootefi bootmgr" returns, run U-Boot "bootcmd" environment variable.

>
> > +     /* Add UEFI Boot Manager entry if available */
> > +     if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
> > +             if (i <= MAX_COUNT - 1) {
> > +                     entry = malloc(sizeof(struct bootmenu_entry));
> > +                     if (!entry)
> > +                             goto cleanup;
> > +
> > +                     entry->title = u16_strdup(u"UEFI Boot Manager");
> > +                     if (!entry->title) {
> > +                             free(entry);
> > +                             goto cleanup;
> > +                     }
> > +
> > +                     entry->command = strdup("bootefi bootmgr");
> > +                     if (!entry->command) {
> > +                             free(entry->title);
> > +                             free(entry);
> > +                             goto cleanup;
> > +                     }
> > +
> > +                     sprintf(entry->key, "%d", i);
> > +
> > +                     entry->num = i;
> > +                     entry->menu = menu;
> > +                     entry->type = BOOT_TYPE_NONE;
> > +                     entry->next = NULL;
> > +
> > +                     if (!iter)
> > +                             menu->first = entry;
> > +                     else
> > +                             iter->next = entry;
> > +
> > +                     iter = entry;
> > +                     ++i;
> > +             }
> > +     }
> > +
> > +{
> > +     char *p;
> > +     char *token;
> > +     char *boot_targets;
> > +     int len;
> > +
> > +     /* list the distro boot "boot_targets" */
> > +     boot_targets = env_get("boot_targets");
> > +     if (!boot_targets)
> > +             goto exit_boot_targets;
> > +
> > +     len = strlen(boot_targets);
> > +     p = calloc(1, len + 1);
> > +     strncpy(p, boot_targets, len);
> > +
> > +     token = strtok(p, " ");
> > +
> > +     do {
> > +             u16 *buf;
> > +             char *command;
> > +             int command_size;
> > +
> > +             entry = malloc(sizeof(struct bootmenu_entry));
> > +             if (!entry)
> > +                     goto cleanup;
> > +
> > +             len = strlen(token);
> > +             buf = calloc(1, (len + 1) * sizeof(u16));
> > +             entry->title = buf;
> > +             if (!entry->title) {
> > +                     free(entry);
> > +                     goto cleanup;
> > +             }
> > +             utf8_utf16_strncpy(&buf, token,len);
> > +             sprintf(entry->key, "%d", i);
> > +             entry->num = i;
> > +             entry->menu = menu;
> > +
> > +             command_size = strlen("run bootcmd_") + len + 1;
>
> I think we are better of here with a sizeof() instead of strlen since the
> 'run bootcmd_' string is not expected to change

OK.

>
> > +             command = calloc(1, command_size);
> > +             if (!command) {
> > +                     free(entry->title);
> > +                     free(entry);
> > +                     goto cleanup;
> > +             }
> > +             snprintf(command, command_size, "run bootcmd_%s", token);
> > +             entry->command = command;
> > +             entry->type = BOOT_TYPE_DISTRO_BOOT;
> >               entry->next = NULL;
> >
> >               if (!iter)
> > @@ -345,6 +552,48 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >
> >               if (i == MAX_COUNT - 1)
> >                       break;
> > +
> > +             token = strtok(NULL, " ");
> > +     } while (token);
> > +
> > +     free(p);
> > +}
> > +
> > +exit_boot_targets:
> > +
> > +     /* Add UEFI Boot Manager Maintenance entry */
> > +     if (i <= MAX_COUNT - 1) {
> > +             entry = malloc(sizeof(struct bootmenu_entry));
> > +             if (!entry)
> > +                     goto cleanup;
> > +
> > +             entry->title = u16_strdup(u"UEFI Boot Manager Maintenance");
> > +             if (!entry->title) {
> > +                     free(entry);
> > +                     goto cleanup;
> > +             }
> > +
> > +             entry->command = strdup("echo TODO: Not implemented");
> > +             if (!entry->command) {
> > +                     free(entry->title);
> > +                     free(entry);
> > +                     goto cleanup;
> > +             }
>
> Any idea of how we'll tackle that?  We could export the efibootmgr
> functions that deal with this and use them on the edit menu ?

I guess you are referring the RFC patch[*1] I sent before?
If yes, yes I will reuse them.
Or do you mean the "efidebug" command?

[*1] https://lore.kernel.org/u-boot/20220225013257.15674-5-masahisa.kojima@linaro.org/

>
> > +
> > +             sprintf(entry->key, "%d", i);
> > +
> > +             entry->num = i;
> > +             entry->menu = menu;
> > +             entry->type = BOOT_TYPE_NONE;
> > +             entry->next = NULL;
> > +
> > +             if (!iter)
> > +                     menu->first = entry;
> > +             else
> > +                     iter->next = entry;
> > +
> > +             iter = entry;
> > +             ++i;
> >       }
> >
> >       /* Add U-Boot console entry at the end */
> > @@ -353,7 +602,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >               if (!entry)
> >                       goto cleanup;
> >
> > -             entry->title = strdup("U-Boot console");
> > +             entry->title = u16_strdup(u"U-Boot console");
>
> Can we add an option to prohibit the user from going back to the console?

Yes, I will add this option.

Thanks,
Masahisa Kojima

>
> >               if (!entry->title) {
> >                       free(entry);
> >                       goto cleanup;
> > @@ -370,6 +619,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >
> >               entry->num = i;
> >               entry->menu = menu;
> > +             entry->type = BOOT_TYPE_NONE;
> >               entry->next = NULL;
> >
> >               if (!iter)
> > @@ -427,7 +677,7 @@ static void bootmenu_show(int delay)
> >  {
> >       int init = 0;
> >       void *choice = NULL;
> > -     char *title = NULL;
> > +     u16 *title = NULL;
> >       char *command = NULL;
> >       struct menu *menu;
> >       struct bootmenu_data *bootmenu;
> > @@ -478,7 +728,7 @@ static void bootmenu_show(int delay)
> >
> >       if (menu_get_choice(menu, &choice)) {
> >               iter = choice;
> > -             title = strdup(iter->title);
> > +             title = u16_strdup(iter->title);
> >               command = strdup(iter->command);
> >       }
> >
> > @@ -493,7 +743,7 @@ cleanup:
> >       }
> >
> >       if (title && command) {
> > -             debug("Starting entry '%s'\n", title);
> > +             debug("Starting entry '%ls'\n", title);
> >               free(title);
> >               run_command(command, 0);
> >               free(command);
> > --
> > 2.17.1
> >
>
> Cheers
> /Ilias

Re: [RFC PATCH v3 2/2] bootmenu: add UEFI and disto_boot entries

[Masahisa Kojima]

On Thu, 10 Mar 2022 at 11:42, Takahiro Akashi
<takahiro.akashi@linaro.org> wrote:
>
> On Thu, Mar 10, 2022 at 10:50:57AM +0900, Takahiro Akashi wrote:
> > On Wed, Mar 09, 2022 at 04:34:42PM +0200, Ilias Apalodimas wrote:
> > > Hi Kojima-san
> > >
> > > On Tue, Mar 08, 2022 at 11:07:45PM +0900, Masahisa Kojima wrote:
> > > > This commit adds the UEFI related menu entries and
> > > > distro_boot entries into the bootmenu.
> > > >
> > > > For UEFI, user can select which UEFI "Boot####" option
> > > > to execute, call UEFI bootmgr and UEFI boot variable
> > > > maintenance menu. UEFI bootmgr entry is required to
> > > > correctly handle "BootNext" variable.
> > >
> > > Hmm why? (some more info further down)
> > >
> > > >
> > > > For distro_boot, user can select the boot device
> > > > included in "boot_targets" u-boot environment variable.
> > > >
> > > > The menu example is as follows.
> > > >
> > > >   *** U-Boot Boot Menu ***
> > > >
> > > >      Boot 1. kernel (bootmenu_0)
> > > >      Boot 2. kernel (bootmenu_1)
> > > >      Reset board (bootmenu_2)
> > > >      debian (BOOT0000)
> > > >      ubuntu (BOOT0001)
> > > >      UEFI Boot Manager
> > > >      usb0
> > > >      scsi0
> > > >      virtio0
> > > >      dhcp
> > > >      UEFI Boot Manager Maintenance
> > > >      U-Boot console
> > > >
> > > >   Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit
> > > >
> > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > ---
> > > > Changes in v3:
> > > > - newly created
> > > >
> > > >  cmd/bootmenu.c | 268 +++++++++++++++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 259 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> > > > index 409ef9a848..a8dc50dcaa 100644
> > > > --- a/cmd/bootmenu.c
> > > > +++ b/cmd/bootmenu.c
> > > > @@ -3,9 +3,12 @@
> > > >   * (C) Copyright 2011-2013 Pali Rohár <pali@kernel.org>
> > > >   */
> > > >
> > > > +#include <charset.h>
> > > >  #include <common.h>
> > > >  #include <command.h>
> > > >  #include <ansi.h>
> > > > +#include <efi_loader.h>
> > > > +#include <efi_variable.h>
> > > >  #include <env.h>
> > > >  #include <log.h>
> > > >  #include <menu.h>
> > > > @@ -24,11 +27,20 @@
> > > >   */
> > > >  #define MAX_ENV_SIZE     (9 + 2 + 1)
> > > >
> > > > +enum boot_type {
> > > > + BOOT_TYPE_NONE = 0,
> > > > + BOOT_TYPE_BOOTMENU,
> > > > + BOOT_TYPE_UEFI,
> > > > + BOOT_TYPE_DISTRO_BOOT,
> > > > +};
> > > > +
> > > >  struct bootmenu_entry {
> > > >   unsigned short int num;         /* unique number 0 .. MAX_COUNT */
> > > >   char key[3];                    /* key identifier of number */
> > > > - char *title;                    /* title of entry */
> > > > + u16 *title;                     /* title of entry */
> > > >   char *command;                  /* hush command of entry */
> > > > + enum boot_type type;
> > > > + u16 bootorder;
> > > >   struct bootmenu_data *menu;     /* this bootmenu */
> > > >   struct bootmenu_entry *next;    /* next menu entry (num+1) */
> > > >  };
> > > > @@ -75,7 +87,12 @@ static void bootmenu_print_entry(void *data)
> > > >   if (reverse)
> > > >           puts(ANSI_COLOR_REVERSE);
> > > >
> > > > - puts(entry->title);
> > > > + if (entry->type == BOOT_TYPE_BOOTMENU)
> > > > +         printf("%ls (bootmenu_%d)", entry->title, entry->bootorder);
> > > > + else if (entry->type == BOOT_TYPE_UEFI)
> > > > +         printf("%ls (BOOT%04X)", entry->title, entry->bootorder);
> > > > + else
> > > > +         printf("%ls", entry->title);
> > > >
> > > >   if (reverse)
> > > >           puts(ANSI_COLOR_RESET);
> > > > @@ -87,6 +104,10 @@ static void bootmenu_autoboot_loop(struct bootmenu_data *menu,
> > > >   int i, c;
> > > >
> > > >   if (menu->delay > 0) {
> > > > +         /* flush input */
> > > > +         while (tstc())
> > > > +                 getchar();
> > > > +
> > > >           printf(ANSI_CURSOR_POSITION, menu->count + 5, 1);
> > > >           printf("  Hit any key to stop autoboot: %2d ", menu->delay);
> > > >   }
> > > > @@ -300,6 +321,8 @@ static struct bootmenu_data *bootmenu_create(int delay)
> > > >           menu->active = (int)simple_strtol(default_str, NULL, 10);
> > > >
> > > >   while ((option = bootmenu_getoption(i))) {
> > > > +         u16 *buf;
> > > > +
> > > >           sep = strchr(option, '=');
> > > >           if (!sep) {
> > > >                   printf("Invalid bootmenu entry: %s\n", option);
> > > > @@ -311,13 +334,13 @@ static struct bootmenu_data *bootmenu_create(int delay)
> > > >                   goto cleanup;
> > > >
> > > >           len = sep-option;
> > > > -         entry->title = malloc(len + 1);
> > > > +         buf = calloc(1, (len + 1) * sizeof(u16));
> > > > +         entry->title = buf;
> > > >           if (!entry->title) {
> > > >                   free(entry);
> > > >                   goto cleanup;
> > > >           }
> > > > -         memcpy(entry->title, option, len);
> > > > -         entry->title[len] = 0;
> > > > +         utf8_utf16_strncpy(&buf, option, len);
> > > >
> > > >           len = strlen(sep + 1);
> > > >           entry->command = malloc(len + 1);
> > > > @@ -333,6 +356,190 @@ static struct bootmenu_data *bootmenu_create(int delay)
> > > >
> > > >           entry->num = i;
> > > >           entry->menu = menu;
> > > > +         entry->type = BOOT_TYPE_BOOTMENU;
> > > > +         entry->bootorder = i;
> > > > +         entry->next = NULL;
> > > > +
> > > > +         if (!iter)
> > > > +                 menu->first = entry;
> > > > +         else
> > > > +                 iter->next = entry;
> > > > +
> > > > +         iter = entry;
> > > > +         ++i;
> > > > +
> > > > +         if (i == MAX_COUNT - 1)
> > > > +                 break;
> > > > + }
> > > > +
> > > > +{
> > > > + u16 *bootorder;
> > > > + efi_status_t ret;
> > > > + unsigned short j;
> > > > + efi_uintn_t num, size;
> > > > + void *load_option;
> > > > + struct efi_load_option lo;
> > > > + u16 varname[] = u"Boot####";
> > > > +
> > > > + /* Initialize EFI drivers */
> > > > + ret = efi_init_obj_list();
> > > > + if (ret != EFI_SUCCESS) {
> > > > +         log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> > > > +                 ret & ~EFI_ERROR_MASK);
> > > > +         goto cleanup;
> > > > + }
> > > > +
> > > > + bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
> > > > + if (!bootorder)
> > > > +         goto bootmgr;
> > > > +
> > > > + num = size / sizeof(u16);
> > > > + for (j = 0; j < num; j++) {
> > > > +         entry = malloc(sizeof(struct bootmenu_entry));
> > > > +         if (!entry)
> > > > +                 goto cleanup;
> > > > +
> > > > +         efi_create_indexed_name(varname, sizeof(varname),
> > > > +                                 "Boot", bootorder[j]);
> > > > +         load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
> > > > +         if (!load_option)
> > > > +                 continue;
> > > > +
> > > > +         ret = efi_deserialize_load_option(&lo, load_option, &size);
> > > > +         if (ret != EFI_SUCCESS) {
> > > > +                 log_warning("Invalid load option for %ls\n", varname);
> > > > +                 free(load_option);
> > > > +                 continue;
> > > > +         }
> > > > +
> > > > +         if (lo.attributes & LOAD_OPTION_ACTIVE) {
> > > > +                 char *command;
> > > > +                 int command_size;
> > > > +
> > > > +                 entry->title = u16_strdup(lo.label);
> > > > +                 if (!entry->title) {
> > > > +                         free(load_option);
> > > > +                         free(entry);
> > > > +                         goto cleanup;
> > > > +                 }
> > > > +                 command_size = strlen("bootefi bootindex XXXX") + 1;
> > >
> > > As we commented on patch 2/2 you can simply set BootNext variable here and
> > > fire up the efibootmgr
> > >
> > > > +                 command = calloc(1, command_size);
> > > > +                 if (!command) {
> > > > +                         free(entry->title);
> > > > +                         free(load_option);
> > > > +                         free(entry);
> > > > +                         goto cleanup;
> > > > +                 }
> > > > +                 snprintf(command, command_size, "bootefi bootindex %X", bootorder[j]);
> > > > +                 entry->command = command;
> > > > +                 sprintf(entry->key, "%d", i);
> > > > +                 entry->num = i;
> > > > +                 entry->menu = menu;
> > > > +                 entry->type = BOOT_TYPE_UEFI;
> > > > +                 entry->bootorder = bootorder[j];
> > > > +                 entry->next = NULL;
> > > > +
> > > > +                 if (!iter)
> > > > +                         menu->first = entry;
> > > > +                 else
> > > > +                         iter->next = entry;
> > > > +
> > > > +                 iter = entry;
> > > > +                 ++i;
> > > > +         }
> > > > +
> > > > +         if (i == MAX_COUNT - 1)
> > > > +                 break;
> > > > + }
> > > > + free(bootorder);
> > > > +}
> > > > +
> > > > +bootmgr:
> > >
> > > Why do we need an entire menu entry if the bootorder is not defined?
> > > Currently there's no logic in the efibootmgr to look for the standard
> > > filenames in the ESP (eg bootarm.efi, bootaa64.efi etc) if no boot option
> > > is defined.  Instead this is implement in distro_bootcmd.
> >
> > To be clear, "if no boot option is defined" is wrong.
> > What the specification requires is that, if a file name is missing
> > in a device path defined in a boot option, a default path/name must be
> > appended to the path before trying to load an image from that media.
>
> One more thing:
> What the current distro_bootcmd does is to try to a binary with the default
> file name in an order specified by "boot_targets" variable.
> Here is another inconsistency with UEFI specification.

So it is better to create entries in bootmenu indicating
each removable device. (It is what the UiApp in EDK2 does)
Then it can be integrated with the supporting removal media patch
Akashi-san sent[*1].

[*1] https://lore.kernel.org/u-boot/20211109013233.72902-3-takahiro.akashi@linaro.org/#t

Thanks,
Masahisa Kojima


>
> -Takahiro Akashi
>
>
> > > I was thinking of something along the lines of:
> > > 1. bootmenu comes up
> > > 2. We read all the Boot#### variables that are defined and add them on the
> > >    menu
> > > 2a. If the user doesn't explicitly choose a boot option we run 'bootefi bootmgr'
> > > 2b. If the user does select a different option we set BootNext and still
> > > run 'bootefi bootmgr'
> > > 3. If there's not Boot#### option defined,  we should in the future add the
> > >    functionality of searching for bootaa64.efi etc in ESP and still just
> > >    launch the efi bootmgr
> > >
> > > > + /* Add UEFI Boot Manager entry if available */
> > > > + if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
> > > > +         if (i <= MAX_COUNT - 1) {
> > > > +                 entry = malloc(sizeof(struct bootmenu_entry));
> > > > +                 if (!entry)
> > > > +                         goto cleanup;
> > > > +
> > > > +                 entry->title = u16_strdup(u"UEFI Boot Manager");
> > > > +                 if (!entry->title) {
> > > > +                         free(entry);
> > > > +                         goto cleanup;
> > > > +                 }
> > > > +
> > > > +                 entry->command = strdup("bootefi bootmgr");
> > > > +                 if (!entry->command) {
> > > > +                         free(entry->title);
> > > > +                         free(entry);
> > > > +                         goto cleanup;
> > > > +                 }
> > > > +
> > > > +                 sprintf(entry->key, "%d", i);
> > > > +
> > > > +                 entry->num = i;
> > > > +                 entry->menu = menu;
> > > > +                 entry->type = BOOT_TYPE_NONE;
> > > > +                 entry->next = NULL;
> > > > +
> > > > +                 if (!iter)
> > > > +                         menu->first = entry;
> > > > +                 else
> > > > +                         iter->next = entry;
> > > > +
> > > > +                 iter = entry;
> > > > +                 ++i;
> > > > +         }
> > > > + }
> > > > +
> > > > +{
> > > > + char *p;
> > > > + char *token;
> > > > + char *boot_targets;
> > > > + int len;
> > > > +
> > > > + /* list the distro boot "boot_targets" */
> > > > + boot_targets = env_get("boot_targets");
> > > > + if (!boot_targets)
> > > > +         goto exit_boot_targets;
> > > > +
> > > > + len = strlen(boot_targets);
> > > > + p = calloc(1, len + 1);
> > > > + strncpy(p, boot_targets, len);
> > > > +
> > > > + token = strtok(p, " ");
> > > > +
> > > > + do {
> > > > +         u16 *buf;
> > > > +         char *command;
> > > > +         int command_size;
> > > > +
> > > > +         entry = malloc(sizeof(struct bootmenu_entry));
> > > > +         if (!entry)
> > > > +                 goto cleanup;
> > > > +
> > > > +         len = strlen(token);
> > > > +         buf = calloc(1, (len + 1) * sizeof(u16));
> > > > +         entry->title = buf;
> > > > +         if (!entry->title) {
> > > > +                 free(entry);
> > > > +                 goto cleanup;
> > > > +         }
> > > > +         utf8_utf16_strncpy(&buf, token,len);
> > > > +         sprintf(entry->key, "%d", i);
> > > > +         entry->num = i;
> > > > +         entry->menu = menu;
> > > > +
> > > > +         command_size = strlen("run bootcmd_") + len + 1;
> > >
> > > I think we are better of here with a sizeof() instead of strlen since the
> > > 'run bootcmd_' string is not expected to change
> > >
> > > > +         command = calloc(1, command_size);
> > > > +         if (!command) {
> > > > +                 free(entry->title);
> > > > +                 free(entry);
> > > > +                 goto cleanup;
> > > > +         }
> > > > +         snprintf(command, command_size, "run bootcmd_%s", token);
> > > > +         entry->command = command;
> > > > +         entry->type = BOOT_TYPE_DISTRO_BOOT;
> > > >           entry->next = NULL;
> > > >
> > > >           if (!iter)
> > > > @@ -345,6 +552,48 @@ static struct bootmenu_data *bootmenu_create(int delay)
> > > >
> > > >           if (i == MAX_COUNT - 1)
> > > >                   break;
> > > > +
> > > > +         token = strtok(NULL, " ");
> > > > + } while (token);
> > > > +
> > > > + free(p);
> > > > +}
> > > > +
> > > > +exit_boot_targets:
> > > > +
> > > > + /* Add UEFI Boot Manager Maintenance entry */
> > > > + if (i <= MAX_COUNT - 1) {
> > > > +         entry = malloc(sizeof(struct bootmenu_entry));
> > > > +         if (!entry)
> > > > +                 goto cleanup;
> > > > +
> > > > +         entry->title = u16_strdup(u"UEFI Boot Manager Maintenance");
> > > > +         if (!entry->title) {
> > > > +                 free(entry);
> > > > +                 goto cleanup;
> > > > +         }
> > > > +
> > > > +         entry->command = strdup("echo TODO: Not implemented");
> > > > +         if (!entry->command) {
> > > > +                 free(entry->title);
> > > > +                 free(entry);
> > > > +                 goto cleanup;
> > > > +         }
> > >
> > > Any idea of how we'll tackle that?  We could export the efibootmgr
> > > functions that deal with this and use them on the edit menu ?
> >
> > # efibootmgr, if it means efi_bootmgr.c, doesn't have such a function.
> >
> > I hope that it is just a matter of time :)
> >
> > I think that the display format of the menu should be improved so that users
> > easily understand that the list of UEFI boot options are listed in the order
> > that "BootOrder" specifies.
> >
> > -Takahiro Akashi
> >
> > >
> > > > +
> > > > +         sprintf(entry->key, "%d", i);
> > > > +
> > > > +         entry->num = i;
> > > > +         entry->menu = menu;
> > > > +         entry->type = BOOT_TYPE_NONE;
> > > > +         entry->next = NULL;
> > > > +
> > > > +         if (!iter)
> > > > +                 menu->first = entry;
> > > > +         else
> > > > +                 iter->next = entry;
> > > > +
> > > > +         iter = entry;
> > > > +         ++i;
> > > >   }
> > > >
> > > >   /* Add U-Boot console entry at the end */
> > > > @@ -353,7 +602,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> > > >           if (!entry)
> > > >                   goto cleanup;
> > > >
> > > > -         entry->title = strdup("U-Boot console");
> > > > +         entry->title = u16_strdup(u"U-Boot console");
> > >
> > > Can we add an option to prohibit the user from going back to the console?
> > >
> > > >           if (!entry->title) {
> > > >                   free(entry);
> > > >                   goto cleanup;
> > > > @@ -370,6 +619,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> > > >
> > > >           entry->num = i;
> > > >           entry->menu = menu;
> > > > +         entry->type = BOOT_TYPE_NONE;
> > > >           entry->next = NULL;
> > > >
> > > >           if (!iter)
> > > > @@ -427,7 +677,7 @@ static void bootmenu_show(int delay)
> > > >  {
> > > >   int init = 0;
> > > >   void *choice = NULL;
> > > > - char *title = NULL;
> > > > + u16 *title = NULL;
> > > >   char *command = NULL;
> > > >   struct menu *menu;
> > > >   struct bootmenu_data *bootmenu;
> > > > @@ -478,7 +728,7 @@ static void bootmenu_show(int delay)
> > > >
> > > >   if (menu_get_choice(menu, &choice)) {
> > > >           iter = choice;
> > > > -         title = strdup(iter->title);
> > > > +         title = u16_strdup(iter->title);
> > > >           command = strdup(iter->command);
> > > >   }
> > > >
> > > > @@ -493,7 +743,7 @@ cleanup:
> > > >   }
> > > >
> > > >   if (title && command) {
> > > > -         debug("Starting entry '%s'\n", title);
> > > > +         debug("Starting entry '%ls'\n", title);
> > > >           free(title);
> > > >           run_command(command, 0);
> > > >           free(command);
> > > > --
> > > > 2.17.1
> > > >
> > >
> > > Cheers
> > > /Ilias

Re: [RFC PATCH v3 2/2] bootmenu: add UEFI and disto_boot entries

[Ilias Apalodimas]

[...]

> > > +                     command = calloc(1, command_size);
> > > +                     if (!command) {
> > > +                             free(entry->title);
> > > +                             free(load_option);
> > > +                             free(entry);
> > > +                             goto cleanup;
> > > +                     }
> > > +                     snprintf(command, command_size, "bootefi bootindex %X", bootorder[j]);
> > > +                     entry->command = command;
> > > +                     sprintf(entry->key, "%d", i);
> > > +                     entry->num = i;
> > > +                     entry->menu = menu;
> > > +                     entry->type = BOOT_TYPE_UEFI;
> > > +                     entry->bootorder = bootorder[j];
> > > +                     entry->next = NULL;
> > > +
> > > +                     if (!iter)
> > > +                             menu->first = entry;
> > > +                     else
> > > +                             iter->next = entry;
> > > +
> > > +                     iter = entry;
> > > +                     ++i;
> > > +             }
> > > +
> > > +             if (i == MAX_COUNT - 1)
> > > +                     break;
> > > +     }
> > > +     free(bootorder);
> > > +}
> > > +
> > > +bootmgr:
> >
> > Why do we need an entire menu entry if the bootorder is not defined?
> > Currently there's no logic in the efibootmgr to look for the standard
> > filenames in the ESP (eg bootarm.efi, bootaa64.efi etc) if no boot option
> > is defined.  Instead this is implement in distro_bootcmd.
> 
> In this version, newly added command "bootefi bootindex XXXX" is called
> if the user selects one of the "Boot####" option, and "bootefi bootindex"
> does not handle "BootNext". That's why I think the menu entry simply
> call "bootefi bootmgr" is required.
> Anyway, I will modify to set "BootNext" and call "bootefi bootmgr" if
> the user selects a "Boot####" option.
> 
> I think we need to consider the case that "BootNext" is already set
> when bootmenu comes up.
> In this case, bootmenu must automatically try to boot the load option
> of "BootNext" without any user interaction.

Good point.  Especially if we fix setvariable at runtime for specific
boards, capsuleupdates will need this

> 
> >
> > I was thinking of something along the lines of:
> > 1. bootmenu comes up
> > 2. We read all the Boot#### variables that are defined and add them on the
> >    menu
> > 2a. If the user doesn't explicitly choose a boot option we run 'bootefi bootmgr'
> > 2b. If the user does select a different option we set BootNext and still
> > run 'bootefi bootmgr'
> > 3. If there's not Boot#### option defined,  we should in the future add the
> >    functionality of searching for bootaa64.efi etc in ESP and still just
> >    launch the efi bootmgr
> 
> Thank you very much for summarizing.
> I would like to update as follows.
> 
> 1. bootmenu comes up
> 2. If the BootNext is already defined, try to boot with BootNext
> without showing bootmenu
> 3. We read the BootOrder, then read Boot#### with the order specified
> by BootOrder and add it on the menu (UEFI spec requires to honor the
> priorities set in BootOrder variable)
> 3a. If the user doesn't explicitly choose a boot option we run 'bootefi bootmgr'
> 3b. If the user does select a different option we set BootNext and still
> run 'bootefi bootmgr'
> 4. If there's not Boot#### option defined,  we should in the future add the
>    functionality of searching for bootaa64.efi etc in ESP and still just
>    launch the efi bootmgr
> 4a. If "bootefi bootmgr" returns, run U-Boot "bootcmd" environment variable.
> 

Yep, that looks correct

> >
> > > +     /* Add UEFI Boot Manager entry if available */
> > > +     if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {

[...]

> > > +                     free(entry);
> > > +                     goto cleanup;
> > > +             }
> >
> > Any idea of how we'll tackle that?  We could export the efibootmgr
> > functions that deal with this and use them on the edit menu ?
> 
> I guess you are referring the RFC patch[*1] I sent before?
> If yes, yes I will reuse them.

Yes

> Or do you mean the "efidebug" command?
> 
> [*1] https://lore.kernel.org/u-boot/20220225013257.15674-5-masahisa.kojima@linaro.org/
> 
> >
> > > +
> > > +             sprintf(entry->key, "%d", i);
> > > +
> > > +             entry->num = i;
> > > +             entry->menu = menu;
> > > +             entry->type = BOOT_TYPE_NONE;
> > > +             entry->next = NULL;
> > > +
> > > +             if (!iter)
> > > +                     menu->first = entry;
> > > +             else
> > > +                     iter->next = entry;
> > > +
> > > +             iter = entry;
> > > +             ++i;
> > >       }
> > >
> > >       /* Add U-Boot console entry at the end */
> > > @@ -353,7 +602,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
> > >               if (!entry)
> > >                       goto cleanup;
> > >
> > > -             entry->title = strdup("U-Boot console");
> > > +             entry->title = u16_strdup(u"U-Boot console");
> >
> > Can we add an option to prohibit the user from going back to the console?
> 
> Yes, I will add this option.
> 

[...]

Thanks
/Ilias