[RFC PATCH v3 2/2] bootmenu: add UEFI and disto_boot entries
Takahiro Akashi
takahiro.akashi at linaro.org
Thu Mar 10 03:41:56 CET 2022
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 at 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 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,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
More information about the U-Boot
mailing list