[RFC,1/1] eficonfig: Add a 'scan-only' mode in eficonfig

eficonfig will automatically scan and add Boot#### variables on launch.
It will also perform automatic management of the automatically added
variables (e.g it might decide to delete a boot option if the DP
disappears).  This functionality is useful outside the context of
eficonfig as well.

So let's add a -a flag to eficonfig that will only perform the
automatic variable management and exit.  That would allow users
to define a bootcmd along the lines of 'eficonfig -a && bootefi bootmgr',
that matches the §3.5.1.1 'Removable Media Boot Behavior' behaviour
described in the EFI spec.

Open questions:
- Is this ok to add on eficonfig? Similar functionality is described in the EFI
  spec as part of the efibootmgr
- Having the functionality on the command gives us the flexibility to run the
  run the command when needed.  Alternatively we could move it to the efibootmgr
  and hide it behing a Kconfig option
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 cmd/eficonfig.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

--
2.39.2

Hi Ilias,

On Wed, 1 Mar 2023 at 23:44, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> eficonfig will automatically scan and add Boot#### variables on launch.
> It will also perform automatic management of the automatically added
> variables (e.g it might decide to delete a boot option if the DP
> disappears).  This functionality is useful outside the context of
> eficonfig as well.
>
> So let's add a -a flag to eficonfig that will only perform the
> automatic variable management and exit.  That would allow users
> to define a bootcmd along the lines of 'eficonfig -a && bootefi bootmgr',
> that matches the §3.5.1.1 'Removable Media Boot Behavior' behaviour
> described in the EFI spec.
>
> Open questions:
> - Is this ok to add on eficonfig? Similar functionality is described in the EFI
>   spec as part of the efibootmgr
> - Having the functionality on the command gives us the flexibility to run the
>   run the command when needed.  Alternatively we could move it to the efibootmgr
>   and hide it behing a Kconfig option

In current implementation, "bootmenu" and "eficonfig" invoke this
automatic load option
management function, but I think "bootmenu" should not have responsibility for
load option management.
So I'll vote for moving this automatic load option management into efibootmgr,
then it is invoked in efi subsystem initialization.

Regards,
Masahisa Kojima

> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  cmd/eficonfig.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index 720f52b48b8c..9b6631816997 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -2693,10 +2693,18 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
>  {
>         efi_status_t ret;
>         struct efimenu *efi_menu;
> +       bool exit_on_scan = false;
>
> -       if (argc > 1)
> +       if (argc > 2)
>                 return CMD_RET_USAGE;
>
> +       if (argc > 1) {
> +               if (!strcmp(argv[1], "-a"))
> +                       exit_on_scan = true;
> +               else
> +                       return CMD_RET_FAILURE;
> +       }
> +
>         ret = efi_init_obj_list();
>         if (ret != EFI_SUCCESS) {
>                 log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> @@ -2713,6 +2721,9 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
>         if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
>                 return ret;
>
> +       if (exit_on_scan)
> +               return EFI_SUCCESS;
> +
>         while (1) {
>                 efi_menu = eficonfig_create_fixed_menu(maintenance_menu_items,
>                                                        ARRAY_SIZE(maintenance_menu_items));
> @@ -2734,8 +2745,14 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
>         return CMD_RET_SUCCESS;
>  }
>
> +static char eficonfig_help_text[] =
> +       " - UEFI menu to configure UEFI variables\n"
> +       "\n"
> +       "eficonfig - Spawn the configuration menu\n"
> +       "  -a Scan, configure Boot#### variables and exit\n";
> +
>  U_BOOT_CMD(
> -       eficonfig, 1, 0, do_eficonfig,
> +       eficonfig, 2, 0, do_eficonfig,
>         "provide menu-driven UEFI variable maintenance interface",
> -       ""
> +       eficonfig_help_text
>  );
> --
> 2.39.2
>

On Thu, Mar 02, 2023 at 11:21:05AM +0900, Masahisa Kojima wrote:
> Hi Ilias,
> 
> On Wed, 1 Mar 2023 at 23:44, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > eficonfig will automatically scan and add Boot#### variables on launch.
> > It will also perform automatic management of the automatically added
> > variables (e.g it might decide to delete a boot option if the DP
> > disappears).  This functionality is useful outside the context of
> > eficonfig as well.
> >
> > So let's add a -a flag to eficonfig that will only perform the
> > automatic variable management and exit.  That would allow users
> > to define a bootcmd along the lines of 'eficonfig -a && bootefi bootmgr',
> > that matches the §3.5.1.1 'Removable Media Boot Behavior' behaviour
> > described in the EFI spec.
> >
> > Open questions:
> > - Is this ok to add on eficonfig? Similar functionality is described in the EFI
> >   spec as part of the efibootmgr
> > - Having the functionality on the command gives us the flexibility to run the
> >   run the command when needed.  Alternatively we could move it to the efibootmgr
> >   and hide it behing a Kconfig option
> 
> In current implementation, "bootmenu" and "eficonfig" invoke this
> automatic load option
> management function, but I think "bootmenu" should not have responsibility for
> load option management.
> So I'll vote for moving this automatic load option management into efibootmgr,
> then it is invoked in efi subsystem initialization.

Basically +1, but
I think that we can implement this kind of action by using 'event' hook
as "removable media support" itself is implemented.

-Takahiro Akashi


> Regards,
> Masahisa Kojima
> 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  cmd/eficonfig.c | 23 ++++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index 720f52b48b8c..9b6631816997 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -2693,10 +2693,18 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
> >  {
> >         efi_status_t ret;
> >         struct efimenu *efi_menu;
> > +       bool exit_on_scan = false;
> >
> > -       if (argc > 1)
> > +       if (argc > 2)
> >                 return CMD_RET_USAGE;
> >
> > +       if (argc > 1) {
> > +               if (!strcmp(argv[1], "-a"))
> > +                       exit_on_scan = true;
> > +               else
> > +                       return CMD_RET_FAILURE;
> > +       }
> > +
> >         ret = efi_init_obj_list();
> >         if (ret != EFI_SUCCESS) {
> >                 log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> > @@ -2713,6 +2721,9 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
> >         if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> >                 return ret;
> >
> > +       if (exit_on_scan)
> > +               return EFI_SUCCESS;
> > +
> >         while (1) {
> >                 efi_menu = eficonfig_create_fixed_menu(maintenance_menu_items,
> >                                                        ARRAY_SIZE(maintenance_menu_items));
> > @@ -2734,8 +2745,14 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
> >         return CMD_RET_SUCCESS;
> >  }
> >
> > +static char eficonfig_help_text[] =
> > +       " - UEFI menu to configure UEFI variables\n"
> > +       "\n"
> > +       "eficonfig - Spawn the configuration menu\n"
> > +       "  -a Scan, configure Boot#### variables and exit\n";
> > +
> >  U_BOOT_CMD(
> > -       eficonfig, 1, 0, do_eficonfig,
> > +       eficonfig, 2, 0, do_eficonfig,
> >         "provide menu-driven UEFI variable maintenance interface",
> > -       ""
> > +       eficonfig_help_text
> >  );
> > --
> > 2.39.2
> >

Hi both,

On Thu, 2 Mar 2023 at 08:44, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> On Thu, Mar 02, 2023 at 11:21:05AM +0900, Masahisa Kojima wrote:
> > Hi Ilias,
> >
> > On Wed, 1 Mar 2023 at 23:44, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > eficonfig will automatically scan and add Boot#### variables on launch.
> > > It will also perform automatic management of the automatically added
> > > variables (e.g it might decide to delete a boot option if the DP
> > > disappears).  This functionality is useful outside the context of
> > > eficonfig as well.
> > >
> > > So let's add a -a flag to eficonfig that will only perform the
> > > automatic variable management and exit.  That would allow users
> > > to define a bootcmd along the lines of 'eficonfig -a && bootefi bootmgr',
> > > that matches the §3.5.1.1 'Removable Media Boot Behavior' behaviour
> > > described in the EFI spec.
> > >
> > > Open questions:
> > > - Is this ok to add on eficonfig? Similar functionality is described in the EFI
> > >   spec as part of the efibootmgr
> > > - Having the functionality on the command gives us the flexibility to run the
> > >   run the command when needed.  Alternatively we could move it to the efibootmgr
> > >   and hide it behing a Kconfig option
> >
> > In current implementation, "bootmenu" and "eficonfig" invoke this
> > automatic load option
> > management function, but I think "bootmenu" should not have responsibility for
> > load option management.
> > So I'll vote for moving this automatic load option management into efibootmgr,
> > then it is invoked in efi subsystem initialization.
>
> Basically +1, but
> I think that we can implement this kind of action by using 'event' hook
> as "removable media support" itself is implemented.
>

I don't disagree, hence my "open questions" section in there.
Changing eficonfig was the easiest patch I could come up with to show
what I had in mind.

That being said I like Akashi-san's idea of integrating the automatic
scan as an event.  In that case we can even skip hiding it behind a
Kconfig option since this is supposed to be supported as part of the
efi bootmgr?

Thanks!
/Ilias
> -Takahiro Akashi
>
>
> > Regards,
> > Masahisa Kojima
> >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > >  cmd/eficonfig.c | 23 ++++++++++++++++++++---
> > >  1 file changed, 20 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > > index 720f52b48b8c..9b6631816997 100644
> > > --- a/cmd/eficonfig.c
> > > +++ b/cmd/eficonfig.c
> > > @@ -2693,10 +2693,18 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
> > >  {
> > >         efi_status_t ret;
> > >         struct efimenu *efi_menu;
> > > +       bool exit_on_scan = false;
> > >
> > > -       if (argc > 1)
> > > +       if (argc > 2)
> > >                 return CMD_RET_USAGE;
> > >
> > > +       if (argc > 1) {
> > > +               if (!strcmp(argv[1], "-a"))
> > > +                       exit_on_scan = true;
> > > +               else
> > > +                       return CMD_RET_FAILURE;
> > > +       }
> > > +
> > >         ret = efi_init_obj_list();
> > >         if (ret != EFI_SUCCESS) {
> > >                 log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> > > @@ -2713,6 +2721,9 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
> > >         if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> > >                 return ret;
> > >
> > > +       if (exit_on_scan)
> > > +               return EFI_SUCCESS;
> > > +
> > >         while (1) {
> > >                 efi_menu = eficonfig_create_fixed_menu(maintenance_menu_items,
> > >                                                        ARRAY_SIZE(maintenance_menu_items));
> > > @@ -2734,8 +2745,14 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
> > >         return CMD_RET_SUCCESS;
> > >  }
> > >
> > > +static char eficonfig_help_text[] =
> > > +       " - UEFI menu to configure UEFI variables\n"
> > > +       "\n"
> > > +       "eficonfig - Spawn the configuration menu\n"
> > > +       "  -a Scan, configure Boot#### variables and exit\n";
> > > +
> > >  U_BOOT_CMD(
> > > -       eficonfig, 1, 0, do_eficonfig,
> > > +       eficonfig, 2, 0, do_eficonfig,
> > >         "provide menu-driven UEFI variable maintenance interface",
> > > -       ""
> > > +       eficonfig_help_text
> > >  );
> > > --
> > > 2.39.2
> > >