mbox series

[v7,0/9] enable menu-driven UEFI variable maintenance

Message ID 20220613093853.23865-1-masahisa.kojima@linaro.org
Headers show
Series enable menu-driven UEFI variable maintenance | expand

Message

Masahisa Kojima June 13, 2022, 9:38 a.m. UTC
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.

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

Comments

AKASHI Takahiro June 15, 2022, 3:01 a.m. UTC | #1
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
>
Masahisa Kojima June 19, 2022, 4:16 a.m. UTC | #2
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
> >