[PATCH v4 07/11] bootmenu: add UEFI and disto_boot entries
Masahisa Kojima
masahisa.kojima at linaro.org
Mon Apr 4 10:10:29 CEST 2022
On Sat, 2 Apr 2022 at 15:33, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> 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 at 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 at 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.
Thank you for your review.
All above comments(patch separation and adding comment) will be done
in the next version.
>
> How does this fit to Simon's work to overhaul distroboot?
The current implementation targets to support existing distroboot.
If user selects one of the distroboot entry in bootmenu, the bootmenu
runs "run bootcmd_[selected device]" command.
We can integrate Simon's work by replacing this command invocation
to bootmeth style.
>
> > +{
> > + 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?
Yes, it is possible.
The saved non-volatile BootNext is handled before showing bootmenu,
so the above U-Boot internal use case of BootNext can be volatile.
One concern is that UEFI spec says BootNext is an NV variable.
Thanks,
Masahisa Kojima
>
> 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
>
More information about the U-Boot
mailing list