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

[Takahiro Akashi <takahiro.akashi@linaro.org>]

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