| Message ID | 20220613093853.23865-1-masahisa.kojima@linaro.org |
|---|---|
| Headers | show |
| Series | enable menu-driven UEFI variable maintenance | expand |
On Mon, Jun 13, 2022 at 06:38:44PM +0900, Masahisa Kojima wrote: > This series add the menu-driven UEFI boot variable maintenance > and removable media support in bootmenu. > > Different from previous version, thie series adds a new U-Boot > command "efimenu" to invoke the UEFI boot-related variable > maintenance menu. Thanks. I'd like to give this command a more *specific* name rather than just "menu" :) For example, eficontrol or eficonfig. The followings are my comments mostly from user's perspective (or user-friendliness). So you may take them as improvements. * make menuconfig, "provide menu-driven uefi variables maintenance feature" Please add the command name, so "efimenu - ..." * the command can lead to a segmentation fault. I see it on sandbox. efi_init_obj_list() must always be called at the beginning. * If no boot option is defined yet, "Edit/Order/Delete" menu's simply return to the top menu. I expect some (error) message here. * This may happen even at "Add" if there is no block device. * Boot options without file paths (mostly for removable disks) appear in "Change Order" only if "bootmenu" is executed beforehand. This looks weird. * Some menu's have "Quit", others not. In some menu's, "Quit" means "quit without change" which is equivalent to "Esc". It looks a bit inconsistent. * Some menu's have "Save", others not. For instance, "Change Order" should have explicit "Save" so that user may cancel the change. * "Add" - The header line is "Edit Boot Option". -> Add Boot Option - Users may want to add an additional device path, especially for initrd. * "Edit" - The header line is "Select Boot Order". -> Select Boot Option - If "Description" is selected, I expect the current value is shown at editing screen. - If "File" is selected, I expect I can start with the last component (i.e. a file path). This might be arguable, though. - Users may want to have a short-form path. - Users may want to have a file path without a device media path. Now those variants for device path are acceptable in U-Boot implementation. * "Change Boot Order" - Users may want to remove a boot option (for a removable disk) which is automatically inserted by "bootmenu" from BootOrder. (The given boot option may still exist as a variable.) - As I said above, "Save" and "Quit" should be shown. * "Delete" - The header line is "Select Boot Order". -> Delete Boot Option -Takahiro Akashi > Note that menu-driven UEFI Secure Boot key management patch series > will follow. > > [Major Changes] > - rebased to v2022.07-rc4 > - there is detailed changelog in each commit > > Masahisa Kojima (9): > efi_loader: expose END device path node > efimenu: menu-driven addition of UEFI boot option > efimenu: add "Edit Boot Option" menu entry > menu: add KEY_PLUS and KEY_MINUS handling > efimenu: add "Change Boot Order" menu entry > efimenu: add "Delete Boot Option" menu entry > bootmenu: add removable media entries > doc:bootmenu: add description for UEFI boot support > doc:efimenu: add documentation for efimenu command > > cmd/Kconfig | 7 + > cmd/Makefile | 1 + > cmd/bootmenu.c | 99 +- > cmd/efimenu.c | 1824 ++++++++++++++++++++++++++++++ > common/menu.c | 6 + > doc/usage/cmd/bootmenu.rst | 74 ++ > doc/usage/cmd/efimenu.rst | 50 + > doc/usage/index.rst | 1 + > include/efi_loader.h | 63 ++ > include/efi_menu.h | 91 ++ > include/menu.h | 2 + > lib/efi_loader/efi_boottime.c | 52 +- > lib/efi_loader/efi_console.c | 78 ++ > lib/efi_loader/efi_device_path.c | 2 +- > lib/efi_loader/efi_disk.c | 11 + > lib/efi_loader/efi_file.c | 75 +- > 16 files changed, 2385 insertions(+), 51 deletions(-) > create mode 100644 cmd/efimenu.c > create mode 100644 doc/usage/cmd/efimenu.rst > create mode 100644 include/efi_menu.h > > -- > 2.17.1 >
Hi Akashi-san, On Wed, 15 Jun 2022 at 12:01, Takahiro Akashi <takahiro.akashi@linaro.org> wrote: > > On Mon, Jun 13, 2022 at 06:38:44PM +0900, Masahisa Kojima wrote: > > This series add the menu-driven UEFI boot variable maintenance > > and removable media support in bootmenu. > > > > Different from previous version, thie series adds a new U-Boot > > command "efimenu" to invoke the UEFI boot-related variable > > maintenance menu. > > Thanks. > I'd like to give this command a more *specific* name rather than > just "menu" :) > For example, eficontrol or eficonfig. I will update to "eficonfig". > > The followings are my comments mostly from user's perspective (or > user-friendliness). So you may take them as improvements. > > * make menuconfig, "provide menu-driven uefi variables maintenance feature" > Please add the command name, so "efimenu - ..." OK. > * the command can lead to a segmentation fault. I see it on sandbox. > efi_init_obj_list() must always be called at the beginning. Thank you, it must be called. > * If no boot option is defined yet, "Edit/Order/Delete" menu's simply > return to the top menu. I expect some (error) message here. OK. > * This may happen even at "Add" if there is no block device. OK. > * Boot options without file paths (mostly for removable disks) > appear in "Change Order" only if "bootmenu" is executed beforehand. > This looks weird. This is related to the missing efi_init_obj_list() call. I will fix in the next version. > * Some menu's have "Quit", others not. > In some menu's, "Quit" means "quit without change" which is equivalent > to "Esc". It looks a bit inconsistent. I added "Quit" to change order menu. "Quit" has the same meaning as pressing ESC/Ctrl+C. > * Some menu's have "Save", others not. > For instance, "Change Order" should have explicit "Save" so that user may > cancel the change. OK, I will add in the next vesion. > * "Add" > - The header line is "Edit Boot Option". -> Add Boot Option OK. > - Users may want to add an additional device path, especially for initrd. Let me consider supporting it in the future version. > * "Edit" > - The header line is "Select Boot Order". -> Select Boot Option OK. > - If "Description" is selected, I expect the current value is > shown at editing screen. > - If "File" is selected, I expect I can start with the last component > (i.e. a file path). This might be arguable, though. Let me keep the current implementation in the next version. > - Users may want to have a short-form path. > - Users may want to have a file path without a device media path. > Now those variants for device path are acceptable in U-Boot implementation. Let me consider supporting it in the future version. > * "Change Boot Order" > - Users may want to remove a boot option (for a removable disk) which > is automatically inserted by "bootmenu" from BootOrder. > (The given boot option may still exist as a variable.) Removing the automatically inserted entry is possible, but I don't think of good UI to restore it to the menu again, so I would like to skip this item. > - As I said above, "Save" and "Quit" should be shown. OK. > * "Delete" > - The header line is "Select Boot Order". -> Delete Boot Option OK. Thank you very much for your feedback! Regards, Masahisa Kojima > > -Takahiro Akashi > > > > Note that menu-driven UEFI Secure Boot key management patch series > > will follow. > > > > [Major Changes] > > - rebased to v2022.07-rc4 > > - there is detailed changelog in each commit > > > > Masahisa Kojima (9): > > efi_loader: expose END device path node > > efimenu: menu-driven addition of UEFI boot option > > efimenu: add "Edit Boot Option" menu entry > > menu: add KEY_PLUS and KEY_MINUS handling > > efimenu: add "Change Boot Order" menu entry > > efimenu: add "Delete Boot Option" menu entry > > bootmenu: add removable media entries > > doc:bootmenu: add description for UEFI boot support > > doc:efimenu: add documentation for efimenu command > > > > cmd/Kconfig | 7 + > > cmd/Makefile | 1 + > > cmd/bootmenu.c | 99 +- > > cmd/efimenu.c | 1824 ++++++++++++++++++++++++++++++ > > common/menu.c | 6 + > > doc/usage/cmd/bootmenu.rst | 74 ++ > > doc/usage/cmd/efimenu.rst | 50 + > > doc/usage/index.rst | 1 + > > include/efi_loader.h | 63 ++ > > include/efi_menu.h | 91 ++ > > include/menu.h | 2 + > > lib/efi_loader/efi_boottime.c | 52 +- > > lib/efi_loader/efi_console.c | 78 ++ > > lib/efi_loader/efi_device_path.c | 2 +- > > lib/efi_loader/efi_disk.c | 11 + > > lib/efi_loader/efi_file.c | 75 +- > > 16 files changed, 2385 insertions(+), 51 deletions(-) > > create mode 100644 cmd/efimenu.c > > create mode 100644 doc/usage/cmd/efimenu.rst > > create mode 100644 include/efi_menu.h > > > > -- > > 2.17.1 > >