Re: [PATCH v4 07/11] bootmenu: add UEFI and disto_boot entries

On 3/24/22 14:54, 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.

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

  bootmenu_00   : Boot 1. kernel
  bootmenu_01   : Boot 2. kernel
  bootmenu_02   : Reset board
  UEFI BOOT0000 : debian
  UEFI BOOT0001 : ubuntu
  distro_boot   : usb0
  distro_boot   : scsi0
  distro_boot   : virtio0
  distro_boot   : dhcp

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

Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org

Changes in v4:

• add Kconfig option "CMD_BOOTMENU_ENTER_UBOOT_CONSOLE" to disable to enter U-Boot console from bootmenu
• update the menu entry display format
• create local function to create menu entry for bootmenu, UEFI boot option and distro boot command
• handle "BootNext" before showing bootmenu
• call "bootefi bootmgr" instead of "bootefi bootindex"
• move bootmenu_show() into loop
• add default boot behavior(run "bootefi bootmgr" then "run bootcmd") when user quit the bootmenu if U-Boot console is disabled

Changes in v3:

• newly created

  cmd/Kconfig | 10 ++ cmd/bootmenu.c | 393 +++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 360 insertions(+), 43 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig index 5e25e45fd2..5fbeab266f 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -300,6 +300,16 @@ config CMD_BOOTMENU help Add an ANSI terminal boot menu command.

+config CMD_BOOTMENU_ENTER_UBOOT_CONSOLE

• bool "Allow Bootmenu to enter the U-Boot console"
• depends on CMD_BOOTMENU
• default n
• help

•  Add an entry to enter U-Boot console in bootmenu.
  

•  If this option is disabled, user can not enter
  

•  the U-Boot console from bootmenu. It increases
  

•  the system security.
  

This is an unrelated change which is not described in the commit message. As it not specific to UEFI it deserves to live in a separate patch.

config CMD_ADTIMG bool "adtimg" help diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c index d573487272..947b3a49ff 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,27 @@ */ #define MAX_ENV_SIZE (9 + 2 + 1)

+enum bootmenu_ret {

• BOOTMENU_RET_SUCCESS = 0,
• BOOTMENU_RET_FAIL,
• BOOTMENU_RET_QUIT,
• BOOTMENU_RET_UPDATED,

+};

+enum boot_type {

• BOOTMENU_TYPE_NONE = 0,
• BOOTMENU_TYPE_BOOTMENU,
• BOOTMENU_TYPE_UEFI_BOOT_OPTION,
• BOOTMENU_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; /* boot type of entry */
• u16 bootorder; /* order for each boot type */ struct bootmenu_data *menu; /* this bootmenu */ struct bootmenu_entry *next; /* next menu entry (num+1) */ };

@@ -75,7 +94,14 @@ static void bootmenu_print_entry(void *data) if (reverse) puts(ANSI_COLOR_REVERSE);

• puts(entry->title);

• if (entry->type == BOOTMENU_TYPE_BOOTMENU)

• printf("bootmenu_%02d   : %ls", entry->bootorder, entry->title);
  

• else if (entry->type == BOOTMENU_TYPE_UEFI_BOOT_OPTION)

• printf("UEFI BOOT%04X : %ls", entry->bootorder, entry->title);
  

• else if (entry->type == BOOTMENU_TYPE_DISTRO_BOOT)

• printf("distro_boot   : %ls", entry->title);
  

• else

• printf("%ls", entry->title);
  
  

  if (reverse) puts(ANSI_COLOR_RESET);

@@ -87,6 +113,10 @@ static void bootmenu_autoboot_loop(struct bootmenu_data *menu, int i, c;

if (menu->delay > 0) {

• /* flush input */
  

• while (tstc())
  

• 	getchar();
  

This change is not related to UEFI and not described in the commit message.

Put it into a separate patch.

printf(ANSI_CURSOR_POSITION, menu->count + 5, 1);
printf("  Hit any key to stop autoboot: %2d ", menu->delay);

} @@ -275,31 +305,20 @@ static void bootmenu_destroy(struct bootmenu_data *menu) free(menu); }

Please, provide a function description.

-static struct bootmenu_data *bootmenu_create(int delay) +static int prepare_bootmenu_entry(struct bootmenu_data *menu,

• 		  struct bootmenu_entry **current,
  

• 		  unsigned short int *index)
  

  {

• unsigned short int i = 0;
• const char *option;
• struct bootmenu_data *menu;
• struct bootmenu_entry *iter = NULL;
• int len; char *sep;
• char *default_str;
• struct bootmenu_entry *entry;
• menu = malloc(sizeof(struct bootmenu_data));
• if (!menu)

• return NULL;
  

• menu->delay = delay;
• menu->active = 0;
• menu->first = NULL;
• default_str = env_get("bootmenu_default");
• if (default_str)

• menu->active = (int)simple_strtol(default_str, NULL, 10);
  

• const char *option;

• unsigned short int i = *index;

• struct bootmenu_entry *entry = NULL;

• struct bootmenu_entry *iter = *current;

  while ((option = bootmenu_getoption(i))) {

• u16 *buf;
  

• sep = strchr(option, '='); if (!sep) { printf("Invalid bootmenu entry: %s\n", option);

@@ -308,23 +327,23 @@ static struct bootmenu_data *bootmenu_create(int delay)

entry = malloc(sizeof(struct bootmenu_entry));
if (!entry)

• 	goto cleanup;
  

• 	return -ENOMEM;
  
  

  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;
  

• 	return -ENOMEM;
  

  }

• memcpy(entry->title, option, len);
  

• entry->title[len] = 0;
  

• utf8_utf16_strncpy(&buf, option, len);
  
  

  len = strlen(sep + 1); entry->command = malloc(len + 1); if (!entry->command) { free(entry->title); free(entry);

• 	goto cleanup;
  

• 	return -ENOMEM;
  

  } memcpy(entry->command, sep + 1, len); entry->command[len] = 0;

@@ -333,6 +352,158 @@ static struct bootmenu_data *bootmenu_create(int delay)

entry->num = i;
entry->menu = menu;

• entry->type = BOOTMENU_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;
  

• }
• *index = i;
• *current = iter;
• return 1;

+}

This is an unrelated change not described in the commit message.

Put it into a separate patch.

Please, add a function description.

+static int prepare_uefi_bootorder_entry(struct bootmenu_data *menu,

• 			struct bootmenu_entry **current,
  

• 			unsigned short int *index)
  

+{

• 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####";
• unsigned short int i = *index;
• struct bootmenu_entry *entry = NULL;
• struct bootmenu_entry *iter = *current;
• bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
• if (!bootorder)

• return -ENOENT;
  

• num = size / sizeof(u16);
• for (j = 0; j < num; j++) {

• entry = malloc(sizeof(struct bootmenu_entry));
  

• if (!entry)
  

• 	return -ENOMEM;
  

• 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);
  

• 	free(entry);
  

• 	continue;
  

• }
  

• if (lo.attributes & LOAD_OPTION_ACTIVE) {
  

• 	entry->title = u16_strdup(lo.label);
  

• 	if (!entry->title) {
  

• 		free(load_option);
  

• 		free(entry);
  

• 		return -ENOMEM;
  

• 	}
  

• 	entry->command = strdup("bootefi bootmgr");
  

• 	sprintf(entry->key, "%d", i);
  

• 	entry->num = i;
  

• 	entry->menu = menu;
  

• 	entry->type = BOOTMENU_TYPE_UEFI_BOOT_OPTION;
  

• 	entry->bootorder = bootorder[j];
  

• 	entry->next = NULL;
  

• 	if (!iter)
  

• 		menu->first = entry;
  

• 	else
  

• 		iter->next = entry;
  

• 	iter = entry;
  

• 	i++;
  

• }
  

• free(load_option);
  

• if (i == MAX_COUNT - 1)
  

• 	break;
  

• }
• free(bootorder);
• *index = i;
• *current = iter;
• return 1;

+}

Please, provide a Sphinx style function description explaining what this function is meant to do.

+static int prepare_distro_boot_entry(struct bootmenu_data *menu,

• 		     struct bootmenu_entry **current,
  

• 		     unsigned short int *index)
  

Please, put distro boot entry generation and UEFI boot entry generation into separate patches. This will make reviewing much easier.

How does this fit to Simon's work to overhaul distroboot?

+{

• char *p;

• int len;

• char *token;

• char *boot_targets;

• unsigned short int i = *index;

• struct bootmenu_entry *entry = NULL;

• struct bootmenu_entry *iter = *current;

• /* list the distro boot "boot_targets" */

• boot_targets = env_get("boot_targets");

• if (!boot_targets)

• return -ENOENT;
  

• len = strlen(boot_targets);

• p = calloc(1, len + 1);

• strlcpy(p, boot_targets, len);

• token = strtok(p, " ");

• do {

• u16 *buf;
  

• char *command;
  

• int command_size;
  

• entry = malloc(sizeof(struct bootmenu_entry));
  

• if (!entry)
  

• 	return -ENOMEM;
  

• len = strlen(token);
  

• buf = calloc(1, (len + 1) * sizeof(u16));
  

• entry->title = buf;
  

• if (!entry->title) {
  

• 	free(entry);
  

• 	return -ENOMEM;
  

• }
  

• utf8_utf16_strncpy(&buf, token, len);
  

• sprintf(entry->key, "%d", i);
  

• entry->num = i;
  

• entry->menu = menu;
  

• command_size = sizeof("run bootcmd_") + len;
  

• command = calloc(1, command_size);
  

• if (!command) {
  

• 	free(entry->title);
  

• 	free(entry);
  

• 	return -ENOMEM;
  

• }
  

• snprintf(command, command_size, "run bootcmd_%s", token);
  

• entry->command = command;
  

• entry->type = BOOTMENU_TYPE_DISTRO_BOOT;
  

  entry->next = NULL;

  if (!iter)

@@ -341,10 +512,59 @@ static struct bootmenu_data *bootmenu_create(int delay) iter->next = entry;

iter = entry;

• ++i;
  

• i++;
  
  

  if (i == MAX_COUNT - 1) break;

• token = strtok(NULL, " ");
  

• } while (token);

• free(p);

• *index = i;

• *current = iter;

• return 1;

+}

+static struct bootmenu_data *bootmenu_create(int delay) +{

• int ret;

• unsigned short int i = 0;

• struct bootmenu_data *menu;

• struct bootmenu_entry *iter = NULL;

• struct bootmenu_entry *entry;

• char *default_str;

• menu = malloc(sizeof(struct bootmenu_data));

• if (!menu)

• return NULL;
  

• menu->delay = delay;

• menu->active = 0;

• menu->first = NULL;

• default_str = env_get("bootmenu_default");

• if (default_str)

• menu->active = (int)simple_strtol(default_str, NULL, 10);
  

• ret = prepare_bootmenu_entry(menu, &iter, &i);

• if (ret < 0)

• goto cleanup;
  

• if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {

• if (i < MAX_COUNT - 1) {
  

• 	ret = prepare_uefi_bootorder_entry(menu, &iter, &i);
  

• 	if (ret < 0 && ret != -ENOENT)
  

• 		goto cleanup;
  

• }
  

• }

• if (i < MAX_COUNT - 1) {

• ret = prepare_distro_boot_entry(menu, &iter, &i);
  

• if (ret < 0 && ret != -ENOENT)
  

• 	goto cleanup;
  

  }

  /* Add U-Boot console entry at the end */

@@ -353,7 +573,12 @@ static struct bootmenu_data *bootmenu_create(int delay) if (!entry) goto cleanup;

• entry->title = strdup("U-Boot console");
  

• /* Add dummy entry if entering U-Boot console is disabled */
  

• if (IS_ENABLED(CONFIG_CMD_BOOTMENU_ENTER_UBOOT_CONSOLE))
  

• 	entry->title = u16_strdup(u"U-Boot console");
  

• else
  

• 	entry->title = u16_strdup(u"");
  

• if (!entry->title) { free(entry); goto cleanup;

@@ -370,6 +595,7 @@ static struct bootmenu_data *bootmenu_create(int delay)

entry->num = i;
entry->menu = menu;

• entry->type = BOOTMENU_TYPE_NONE;
  

  entry->next = NULL;

  if (!iter)

@@ -378,7 +604,7 @@ static struct bootmenu_data *bootmenu_create(int delay) iter->next = entry;

iter = entry;

• ++i;
  

• i++;
  

  }

  menu->count = i;

@@ -423,43 +649,73 @@ static void menu_display_statusline(struct menu *m) puts(ANSI_CLEAR_LINE); }

-static void bootmenu_show(int delay) +static void handle_uefi_bootnext(void) {

• u16 bootnext;
• efi_status_t ret;
• efi_uintn_t size;
• /* 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);
  

• return;
  

• }
• /* If UEFI BootNext variable is set, boot the BootNext load option */
• size = sizeof(u16);
• ret = efi_get_variable_int(u"BootNext",

• 		   &efi_global_variable_guid,
  

• 		   NULL, &size, &bootnext, NULL);
  

• if (ret == EFI_SUCCESS)

• /* BootNext does exist here, try to boot */
  

• run_command("bootefi bootmgr", 0);
  

+}

+static enum bootmenu_ret bootmenu_show(int delay) +{

• int cmd_ret; int init = 0; void *choice = NULL;

• char *title = NULL;

• u16 *title = NULL; char *command = NULL; struct menu *menu; struct bootmenu_data *bootmenu; struct bootmenu_entry *iter;

• efi_status_t efi_ret = EFI_SUCCESS; char *option, *sep;

• if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR))

• handle_uefi_bootnext();
  

• /* If delay is 0 do not create menu, just run first entry */ if (delay == 0) { option = bootmenu_getoption(0); if (!option) { puts("bootmenu option 0 was not found\n");

• 	return;
  

• 	return BOOTMENU_RET_FAIL;
  

  } sep = strchr(option, '='); if (!sep) { puts("bootmenu option 0 is invalid\n");

• 	return;
  

• 	return BOOTMENU_RET_FAIL;
  

  }

• run_command(sep+1, 0);
  

• return;
  

• cmd_ret = run_command(sep + 1, 0);
  

• return (cmd_ret == CMD_RET_SUCCESS ? BOOTMENU_RET_SUCCESS : BOOTMENU_RET_FAIL);
  

  }

  bootmenu = bootmenu_create(delay); if (!bootmenu)

• return;
  

• return BOOTMENU_RET_FAIL;
  
  

  menu = menu_create(NULL, bootmenu->delay, 1, menu_display_statusline, bootmenu_print_entry, bootmenu_choice_entry, bootmenu); if (!menu) { bootmenu_destroy(bootmenu);

• return;
  

• return BOOTMENU_RET_FAIL;
  

  }

  for (iter = bootmenu->first; iter; iter = iter->next) {

@@ -478,8 +734,33 @@ static void bootmenu_show(int delay)

if (menu_get_choice(menu, &choice) == 1) { iter = choice;

• title = strdup(iter->title);
  

• /* last entry is U-Boot console or Quit */
  

• if (iter->num == iter->menu->count - 1) {
  

• 	menu_destroy(menu);
  

• 	bootmenu_destroy(bootmenu);
  

• 	return BOOTMENU_RET_QUIT;
  

• }
  

• title = u16_strdup(iter->title);
  

  command = strdup(iter->command);
• } else {

• goto cleanup;
  

• }
• /*

• * If the selected entry is UEFI BOOT####, set the BootNext variable.
  

• * Then uefi bootmgr is invoked by the preset command in iter->command.
  

• */
  

• if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {

• if (iter->type == BOOTMENU_TYPE_UEFI_BOOT_OPTION) {
  

• 	efi_ret = efi_set_variable_int(u"BootNext", &efi_global_variable_guid,
  

We should avoid needlessly writing to flash. Can't we avoid the non-volatile flag here?

Best regards

Heinrich

• 				       EFI_VARIABLE_NON_VOLATILE |
  

• 				       EFI_VARIABLE_BOOTSERVICE_ACCESS |
  

• 				       EFI_VARIABLE_RUNTIME_ACCESS,
  

• 				       sizeof(u16), &iter->bootorder, false);
  

• 	if (efi_ret != EFI_SUCCESS)
  

• 		goto cleanup;
  

• }
  

  }

  cleanup:

@@ -493,21 +774,47 @@ cleanup: }

if (title && command) {

• debug("Starting entry '%s'\n", title);
  

• debug("Starting entry '%ls'\n", title);
  

  free(title);

• run_command(command, 0);
  

• if (efi_ret == EFI_SUCCESS)
  

• 	cmd_ret = run_command(command, 0);
  

  free(command); }

  #ifdef CONFIG_POSTBOOTMENU run_command(CONFIG_POSTBOOTMENU, 0); #endif

• if (efi_ret == EFI_SUCCESS && cmd_ret == CMD_RET_SUCCESS)

• return BOOTMENU_RET_SUCCESS;
  

• return BOOTMENU_RET_FAIL; }

  #ifdef CONFIG_AUTOBOOT_MENU_SHOW int menu_show(int bootdelay) {

• bootmenu_show(bootdelay);

• int ret;
• while (1) {

• ret = bootmenu_show(bootdelay);
  

• bootdelay = -1;
  

• if (ret == BOOTMENU_RET_UPDATED)
  

• 	continue;
  

• if (!IS_ENABLED(CONFIG_CMD_BOOTMENU_ENTER_UBOOT_CONSOLE)) {
  

• 	if (ret == BOOTMENU_RET_QUIT) {
  

• 		/* default boot process */
  

• 		if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR))
  

• 			run_command("bootefi bootmgr", 0);
  

• 		run_command("run bootcmd", 0);
  

• 	}
  

• } else {
  

• 	break;
  

• }
  

• }
• return -1; /* -1 - abort boot and run monitor code */ } #endif