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.koj...@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 <p...@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