Re: [PATCH v4 00/11] enable menu-driven boot device selection

[Masahisa Kojima <masahisa.kojima@linaro.org>]

Hi Akashi-san,

On Fri, 25 Mar 2022 at 10:20, Takahiro Akashi
<takahiro.akashi@linaro.org> wrote:
>
> Kojima-san,
>
> On Thu, Mar 24, 2022 at 10:54:32PM +0900, Masahisa Kojima wrote:
> > This patch series adds the menu-driven boot device selection,
> > by extending the existing "bootmenu" to include UEFI and distro_boot
> > related entries, and supports menu-driven UEFI boot variable
> > maintenance.
> >
> > This patch series also includes the removable media support
> > that UEFI specification requires to support.
> >
> > The menu example is as follows.
>
> Good job done, Kojima-san. I like it.
> Before reviewing each commit, I would suggest a couple of
> improvements on the menu itself.
> They are more or less my opinion and other people may have
> their own preference, though.

Thank you for your comment and verifying the actual behavior.

>
> 1) Top menu (U-Boot Boot Menu)
>  - In general, it's a bit difficult to understand from where
>    each menu item comes and what it means.
>    For instance,
> >      UEFI BOOT0000 : debian
>    is a user-defined boot option, while
> >      UEFI BOOT0002 : mmc0:1
>    is an option for removable media. Correct?

Yes, and I agree that the current menu title is a bit difficult to understand,
especially for automatically listed removable media(e.g. mmc0:1).

>
>  - I'd prefer to categorize items into sub-menus, particularly,
>    UEFI items.
>
>        bootmenu_...
>        distro_boot ...
>        UEFI Boot
>        UEFI Boot Manager Maintenance

Yes, it is a good idea.
On the other hand, if user prefers simpler menu and only uses UEFI Boot
for example, bootmenu_xx and distro boot entries can be removed by
updating the U-Boot environment variable(remove bootmenu_xx
and boot_targets).

>
>    and "UEFI Boot" sub-menu shows
> >      UEFI BOOT0000 : debian
> >      UEFI BOOT0001 : ubuntu
>    in an order of "BootOrder",
>
>    and
>        <removable media> /* not selectable */
> >      UEFI BOOT0002 : mmc0:1
> >      UEFI BOOT0003 : mmc0:2

It is a little difficult.
UEFI BootOrder variable is also automatically updated by bootmenu
to include the removable media, so the entries that user defined
Boot#### and automatically added removable media are mixed in
BootOrder.
The UEFI Boot entry is displayed in the order of BootOrder.

>
>  - For UEFI items, I want to do "e" (edit/modify) directly here
>    to change the option.

It is the same as the Grub menu. I will consider it but let me set
as a low priority now.

>
>  - When "U-Boot console" is selected, the prompt ("=>") is displayed
>    like
>        UEFI Boot Manager Maintenance
>             U-Boot console=>
>    It should be output at the beginning of the next line or
>    the screen be cleaned up before showing the prompt.

Thank you. It should be fixed.

>
>  - What not have "Quit" here?

OK, I will consider to add.

>
> 2) UEFI Boot Manager Maintenance
>  - The title should be "UEFI Boot Manager Maintenance".

"UEFI Boot Manager Maintenance" is the same as the current one.

>  - I want to have "Edit(Modify) Boot Option"

Yes, I agree.

>  - "Add Boot Option"
>    - The menu titles should be "Select a device" and "Select a file".

Current "Add Boot Option" will support the "Select a file" feature.
I think the user will normally use the boot option having FilePath.

>    - Some devices are shown, some are not. Why?
>      Do we have to run, say, "scsi rescan" beforehand?

Current bootmenu enumerates the all devices implementing
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
If some devices are not shown, please run the discover command
like "scsi rescan" then re-enter the bootmenu with "bootmenu" command.
If the all devices listed by "efidebug dh" having
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL are not shown in bootmenu,
something wrong in my code.

>    - How can we specify "removable media" without a file path?

This patch series only supports the auto enumeration.
I think the user will not manually add the "removable media".

>    - At "file selection" menu, "Esc" should let us go back to
>      the "device selection" menu rather than the top, "Add Boot Option".

Yes, I agree.

>    - We should be able to specify initrd path, i.e. the second device path
>      in a boot option.
>    - We should be able to specify "optional data" in a boot option.
>  - "Change Boot Order"

I consider supporting these two items.

>    - I like a more intuitive operation here. Say,
>      select an item with "Enter" and then use "Up" and "Down" to move it
>      around.

Yes, I agree and try to improve.

>  - Probably, it would be better to have the final confirmation, like
>        "Do you want to save the change?"

Yes, same for "Delete Boot Option".

Thanks,
Masahisa Kojima

>
> Thanks,
> -Takahiro Akashi
>
> >   *** U-Boot Boot Menu ***
> >
> >      bootmenu_00   : Boot 1. kernel
> >      bootmenu_01   : Boot 2. kernel
> >      bootmenu_02   : Reset board
> >      UEFI BOOT0000 : debian
> >      UEFI BOOT0001 : ubuntu
> >      UEFI BOOT0002 : mmc0:1
> >      UEFI BOOT0003 : mmc0:2
> >      UEFI BOOT0004 : nvme0:1
> >      UEFI BOOT0005 : nvme0:2
> >      UEFI BOOT0006 : usb0:2
> >      UEFI BOOT0007 : usb1:1
> >      UEFI BOOT0008 : usb1:2
> >      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
> >
> > [Major changes from RFC v3]
> > - add Kconfig option to disable U-Boot console
> > - add UEFI boot variable maintenance feature
> > - support removable media support and user selection
> > - app bootmenu enhancement documentation
> >
> > [How to run on QEMU(arm64)]
> > 1) clone source code
> >  $ git clone https://git.linaro.org/people/masahisa.kojima/u-boot.git \
> > -b kojima/bootmenu_v4_upstream_0324 --depth 1
> >
> > 2) prepare U-Boot .config
> >  $ make qemu_arm64_menuconfig
> >   then, enable CONFIG_CMD_BOOTMENU and CONFIG_AUTOBOOT_MENU_SHOW
> >
> > 3) run on QEMU(arm64) example
> >  $ qemu-system-aarch64 -machine virt,gic-version=3 -cpu cortex-a57 -m 4G -nographic \
> >    -no-acpi -bios ./u-boot.bin -hda xxx.img
> >
> >
> > AKASHI Takahiro (2):
> >   efi_loader: export efi_locate_device_handle()
> >   efi_loader: bootmgr: add booting from removable media
> >
> > Masahisa Kojima (9):
> >   bootmenu: fix menu API error handling
> >   lib/charset: add u16_strlcat() function
> >   test: unit test for u16_strlcat()
> >   menu: always show the menu regardless of the number or entry
> >   bootmenu: add UEFI and disto_boot entries
> >   bootmenu: factor out the user input handling
> >   efi_loader: add menu-driven UEFI Boot Variable maintenance
> >   bootmenu: add removable media entries
> >   doc:bootmenu: add UEFI boot variable and distro boot support
> >
> >  cmd/Kconfig                               |   10 +
> >  cmd/bootmenu.c                            |  678 +++++++----
> >  common/menu.c                             |  139 ++-
> >  doc/usage/bootmenu.rst                    |   65 ++
> >  include/charset.h                         |   15 +
> >  include/config_distro_bootcmd.h           |   14 +-
> >  include/efi_default_filename.h            |   26 +
> >  include/efi_loader.h                      |   63 ++
> >  include/menu.h                            |   20 +
> >  lib/charset.c                             |   21 +
> >  lib/efi_loader/Makefile                   |    1 +
> >  lib/efi_loader/efi_bootmenu_maintenance.c | 1244 +++++++++++++++++++++
> >  lib/efi_loader/efi_bootmgr.c              |   50 +-
> >  lib/efi_loader/efi_boottime.c             |   59 +-
> >  lib/efi_loader/efi_console.c              |   81 ++
> >  lib/efi_loader/efi_disk.c                 |   11 +
> >  lib/efi_loader/efi_file.c                 |   75 +-
> >  test/unicode_ut.c                         |   45 +
> >  18 files changed, 2357 insertions(+), 260 deletions(-)
> >  create mode 100644 include/efi_default_filename.h
> >  create mode 100644 lib/efi_loader/efi_bootmenu_maintenance.c
> >
> > --
> > 2.17.1
> >