Re: [PATCH v5 10/17] bootmenu: add distro boot entry

[Mark Kettenis <mark.kettenis@xs4all.nl>]

> Date: Thu, 12 May 2022 12:39:39 +0200
> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> Am 12. Mai 2022 10:44:28 MESZ schrieb Takahiro Akashi <takahiro.akashi@linaro.org>:
> >On Sun, May 01, 2022 at 11:48:40PM +0200, Heinrich Schuchardt wrote:
> >> On 4/28/22 10:09, Masahisa Kojima wrote:
> >> > This commit adds the distro_boot entries into the bootmenu.
> >> > The bootmenu read the "boot_targets" U-Boot environment variable
> >> > and enumerate it.
> >> > User can select the distro boot entry, then bootmenu executes
> >> > "run bootcmd_xxx" command.
> >> > 
> >> > The bootmenu also checks the existing block devices and network
> >> > option("dhcp" and "pxe") availability, then filter out
> >> > the "boot_targets" appeared in bootmenu.
> >> > 
> >> > The bootmenu example is as follows, distro boot entry has the
> >> > "distro_boot" prefix.
> >> > 
> >> >    *** U-Boot Boot Menu ***
> >> > 
> >> >       distro_boot   : usb0
> >> >       distro_boot   : scsi0
> >> >       distro_boot   : virtio0
> >> >       distro_boot   : dhcp
> >> 
> >> You will be creating UEFI boot options for all block devices anyway.
> >> We should avoid duplicate entries. Please, drop this patch.
> >
> >Nak
> >A distro entry should work, searching not only for a default UEFI file
> >but also sysboot (extlinux.conf) and boot scripts.
> 
> Nobody expects this in an UEFI boot process.

Not sure about that.  Some x86 firmware implementations present a boot
menu that displays both UEFI and "legacy" boot entries.

> Simon's work on bootflows tries to separate different bootflows.

And I'm still not convinced it does so in a sensible way.  I can't
really test the bootflows since the implementation is incomplete.

The bootmenu I saw when I tested things a few weeks ago made sense to
me.  It presented all the relevant options (UEFI and distroboot for
the relevant boot devices) on a system without any of the UEFI
BootXXXX options set.  Some Linux distros still don't support the UEFI
bootflow (or at least don't support it very well), so distro_boot is
still important.

> So adding entries should depend on the configured bootflow.

But in my opinion, U-Boot should not depend on explicitly configured
bootflows.

Cheers,

Mark

> >> > 
> >> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >> > ---
> >> > Changes in v5:
> >> > - split into the separate patch
> >> > - add function description comment
> >> > - handle the case boot_targets variable is empty
> >> > - filter out the non-exist device entry
> >> > 
> >> >   cmd/bootmenu.c | 177 +++++++++++++++++++++++++++++++++++++++++++++++++
> >> >   1 file changed, 177 insertions(+)
> >> > 
> >> > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> >> > index da688e6213..afe42b8041 100644
> >> > --- a/cmd/bootmenu.c
> >> > +++ b/cmd/bootmenu.c
> >> > @@ -7,6 +7,7 @@
> >> >   #include <common.h>
> >> >   #include <command.h>
> >> >   #include <ansi.h>
> >> > +#include <dm.h>
> >> >   #include <efi_loader.h>
> >> >   #include <efi_variable.h>
> >> >   #include <env.h>
> >> > @@ -14,6 +15,7 @@
> >> >   #include <menu.h>
> >> >   #include <watchdog.h>
> >> >   #include <malloc.h>
> >> > +#include <linux/ctype.h>
> >> >   #include <linux/delay.h>
> >> >   #include <linux/string.h>
> >> > 
> >> > @@ -31,6 +33,7 @@ enum boot_type {
> >> >   	BOOTMENU_TYPE_NONE = 0,
> >> >   	BOOTMENU_TYPE_BOOTMENU,
> >> >   	BOOTMENU_TYPE_UEFI_BOOT_OPTION,
> >> > +	BOOTMENU_TYPE_DISTRO_BOOT,
> >> >   };
> >> > 
> >> >   struct bootmenu_entry {
> >> > @@ -90,6 +93,8 @@ static void bootmenu_print_entry(void *data)
> >> >   		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);
> >> > 
> >> > @@ -465,6 +470,172 @@ static int prepare_uefi_bootorder_entry(struct bootmenu_data *menu,
> >> >   	return 1;
> >> >   }
> >> > 
> >> > +static int is_blk_device_available(char *token)
> >> > +{
> >> > +	struct driver *d = ll_entry_start(struct driver, driver);
> >> > +	const int n_ents = ll_entry_count(struct driver, driver);
> >> > +	struct driver *entry;
> >> > +	struct udevice *udev;
> >> > +	struct uclass *uc;
> >> > +	struct blk_desc *desc;
> >> > +	int ret, i;
> >> > +	const char *if_type_name;
> >> > +
> >> > +	ret = uclass_get(UCLASS_BLK, &uc);
> >> > +	if (ret)
> >> > +		return -ENODEV;
> >> > +
> >> > +	for (entry = d; entry < d + n_ents; entry++) {
> >> > +		if (entry->id != UCLASS_BLK)
> >> > +			continue;
> >> > +		i = 0;
> >> > +		uclass_foreach_dev(udev, uc) {
> >> > +			if (udev->driver != entry)
> >> > +				continue;
> >> > +			desc = dev_get_uclass_plat(udev);
> >> > +			if_type_name = blk_get_if_type_name(desc->if_type);
> >> > +			if (strncmp(token, if_type_name, strlen(if_type_name)) == 0) {
> >> > +				char *p;
> >> > +				int j, len;
> >> > +				int devnum = 0;
> >> > +
> >> > +				p = token + strlen(if_type_name);
> >> > +				len = strlen(p);
> >> > +				if (!len)
> >> > +					continue; /* no device number */
> >> > +
> >> > +				for (j = 0; j < len; j++) {
> >> > +					if (!isdigit(*p)) {
> >> > +						/* invalid device number */
> >> > +						devnum = INT_MAX;
> >> > +						break;
> >> > +					}
> >> > +					devnum = (devnum * 10) + (*p++ - '0');
> >> > +				}
> >> > +
> >> > +				if (devnum == INT_MAX)
> >> > +					continue;
> >> > +
> >> > +				if (devnum == desc->devnum)
> >> > +					return 1;
> >> > +			}
> >> > +		}
> >> > +	}
> >> > +
> >> > +	if (strncmp(token, "dhcp", strlen("dhcp")) == 0) {
> >> > +		if (IS_ENABLED(CONFIG_CMD_DHCP))
> >> > +			return 1;
> >> > +	}
> >> > +
> >> > +	if (strncmp(token, "pxe", strlen("pxe")) == 0) {
> >> > +		if (IS_ENABLED(CONFIG_CMD_PXE))
> >> > +			return 1;
> >> > +	}
> >> > +
> >> > +	return -ENODEV;
> >> > +}
> >> > +
> >> > +/**
> >> > + * prepare_distro_boot_entry() - generate the distro boot entries
> >> > + *
> >> > + * This function read the "boot_targets" U-Boot environment variable
> >> > + * and generate the bootmenu entries.
> >> > + *
> >> > + * @menu:	pointer to the bootmenu structure
> >> > + * @current:	pointer to the last bootmenu entry list
> >> > + * @index:	pointer to the index of the last bootmenu entry,
> >> > + *		the number of uefi entry is added by this function
> >> > + * Return:	1 on success, negative value on error
> >> > + */
> >> > +static int prepare_distro_boot_entry(struct bootmenu_data *menu,
> >> > +				     struct bootmenu_entry **current,
> >> > +				     unsigned short int *index)
> >> > +{
> >> > +	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);
> >> > +	if (!len)
> >> > +		return -ENOENT;
> >> > +
> >> > +	p = calloc(1, len + 1);
> >> > +	strlcpy(p, boot_targets, len);
> >> > +
> >> > +	token = strtok(p, " ");
> >> > +
> >> > +	do {
> >> > +		u16 *buf;
> >> > +		char *command;
> >> > +		int command_size;
> >> > +
> >> > +		if (is_blk_device_available(token) != 1) {
> >> > +			token = strtok(NULL, " ");
> >> > +			continue;
> >> > +		}
> >> > +
> >> > +		entry = malloc(sizeof(struct bootmenu_entry));
> >> > +		if (!entry) {
> >> > +			free(p);
> >> > +			return -ENOMEM;
> >> > +		}
> >> > +
> >> > +		len = strlen(token);
> >> > +		buf = calloc(1, (len + 1) * sizeof(u16));
> >> > +		entry->title = buf;
> >> > +		if (!entry->title) {
> >> > +			free(entry);
> >> > +			free(p);
> >> > +			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);
> >> > +			free(p);
> >> > +			return -ENOMEM;
> >> > +		}
> >> > +		snprintf(command, command_size, "run bootcmd_%s", token);
> >> > +		entry->command = command;
> >> > +		entry->type = BOOTMENU_TYPE_DISTRO_BOOT;
> >> > +		entry->next = NULL;
> >> > +
> >> > +		if (!iter)
> >> > +			menu->first = entry;
> >> > +		else
> >> > +			iter->next = entry;
> >> > +
> >> > +		iter = entry;
> >> > +		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;
> >> > @@ -498,6 +669,12 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >> >   		}
> >> >   	}
> >> > 
> >> > +	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 */
> >> >   	if (i <= MAX_COUNT - 1) {
> >> >   		entry = malloc(sizeof(struct bootmenu_entry));
> >> 
> 
>