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

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

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.

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?

 - I'd prefer to categorize items into sub-menus, particularly,
   UEFI items.

       bootmenu_...
       distro_boot ...
       UEFI Boot
       UEFI Boot Manager Maintenance

   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
       
 - For UEFI items, I want to do "e" (edit/modify) directly here
   to change the option.

 - 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.

 - What not have "Quit" here?

2) UEFI Boot Manager Maintenance
 - The title should be "UEFI Boot Manager Maintenance".
 - I want to have "Edit(Modify) Boot Option"
 - "Add Boot Option"
   - The menu titles should be "Select a device" and "Select a file".
   - Some devices are shown, some are not. Why?
     Do we have to run, say, "scsi rescan" beforehand?
   - How can we specify "removable media" without a file path?
   - At "file selection" menu, "Esc" should let us go back to
     the "device selection" menu rather than the top, "Add Boot Option".
   - 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 like a more intuitive operation here. Say,
     select an item with "Enter" and then use "Up" and "Down" to move it
     around.
 - Probably, it would be better to have the final confirmation, like
       "Do you want to save the change?"

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
>