Re: [RFC PATCH v3 1/2] efi_loader: introduce "bootefi bootindex" command

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Kojima-san

On Wed, Mar 09, 2022 at 11:35:57AM +0900, Takahiro Akashi wrote:
> On Wed, Mar 09, 2022 at 09:47:25AM +0900, Masahisa Kojima wrote:
> > On Tue, 8 Mar 2022 at 23:17, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Tue, Mar 08, 2022 at 11:07:44PM +0900, Masahisa Kojima wrote:
> > > > This commit introduces the new command "bootefi bootindex".
> > > > With this command, user can select which "Boot####" option
> > > > to load and execute.
> > >
> > > You can do the same thing with:
> > > $ efidebug boot next 1 (for BOOT0001)
> > > $ bootefi bootmgr
> > 
> > Thank you for the information, it is good to know.
> > My only concern is that user needs to enable "efidebug" command
> > for this case, since efidebug implies that it is for debug purpose.
> 
> Yeah, that is the point where I and ex-maintainer have never agreed.
> So we have no standard UI for UEFI subsystem on U-Boot until now.
> 
> Just FYI, we can do the same thing in yet another way :)
>   => env set -e -nv -bs -rt BootNext =0x0001
> 
> Similarly, BootOrder as well.
> 
> -Takahiro Akashi

I also think it's safe to drop this patch.  It's indeed an easier way to
load a specific boot index, however the use of bootnext is clearly defined
in the spec.  I don't think we need to increase the EFI code more as long
as it's clear to anyone that setting bootnext (note here it has to be set
on each reboot) would have the same result.

Cheers
/Ilias
> 
> > Thanks,
> > Masahisa Kojima
> > 
> > >
> > > -Takahiro Akashi
> > >
> > >
> > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > ---
> > > > Changes in v3:
> > > > - newly created
> > > >
> > > >  cmd/bootefi.c                | 42 ++++++++++++++++++++++++++++++++++++
> > > >  include/efi_loader.h         |  1 +
> > > >  lib/efi_loader/efi_bootmgr.c |  7 +++---
> > > >  3 files changed, 46 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > > > index 46eebd5ee2..df86438fec 100644
> > > > --- a/cmd/bootefi.c
> > > > +++ b/cmd/bootefi.c
> > > > @@ -416,6 +416,30 @@ static int do_efibootmgr(void)
> > > >       return CMD_RET_SUCCESS;
> > > >  }
> > > >
> > > > +/**
> > > > + * do_efibootindex() - load and execute the specified Boot#### option
> > > > + *
> > > > + * Return:   status code
> > > > + */
> > > > +static int do_efibootindex(u16 boot_index)
> > > > +{
> > > > +     efi_handle_t handle;
> > > > +     efi_status_t ret;
> > > > +     void *load_options;
> > > > +
> > > > +     ret = efi_try_load_entry(boot_index, &handle, &load_options);
> > > > +     if (ret != EFI_SUCCESS) {
> > > > +             log_notice("EFI boot manager: failed to load Boot%04X\n", boot_index);
> > > > +             return CMD_RET_FAILURE;
> > > > +     }
> > > > +
> > > > +     ret = do_bootefi_exec(handle, load_options);
> > > > +
> > > > +     if (ret != EFI_SUCCESS)
> > > > +             return CMD_RET_FAILURE;
> > > > +
> > > > +     return CMD_RET_SUCCESS;
> > > > +}
> > > >  /**
> > > >   * do_bootefi_image() - execute EFI binary
> > > >   *
> > > > @@ -654,6 +678,22 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> > > >               return CMD_RET_FAILURE;
> > > >       }
> > > >
> > > > +     if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
> > > > +             if (!strcmp(argv[1], "bootindex")) {
> > > > +                     char *endp;
> > > > +                     int boot_index;
> > > > +
> > > > +                     if (argc < 3)
> > > > +                             return CMD_RET_USAGE;
> > > > +
> > > > +                     boot_index = (int)hextoul(argv[2], &endp);
> > > > +                     if (*endp != '\0' || boot_index > 0xffff)
> > > > +                             return CMD_RET_USAGE;
> > > > +
> > > > +                     return do_efibootindex((u16)boot_index);
> > > > +             }
> > > > +     }
> > > > +
> > > >       if (argc > 2) {
> > > >               uintptr_t fdt_addr;
> > > >
> > > > @@ -702,6 +742,8 @@ static char bootefi_help_text[] =
> > > >       "\n"
> > > >       "    If specified, the device tree located at <fdt address> gets\n"
> > > >       "    exposed as EFI configuration table.\n"
> > > > +     "bootefi bootindex <boot option>\n"
> > > > +     "  - load and boot EFI payload based on the specified BootXXXX variable.\n"
> > > >  #endif
> > > >       ;
> > > >  #endif
> > > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > > index 80a5f1ec01..e5972f5fee 100644
> > > > --- a/include/efi_loader.h
> > > > +++ b/include/efi_loader.h
> > > > @@ -861,6 +861,7 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
> > > >                                 efi_uintn_t load_options_size,
> > > >                                 void *load_options);
> > > >  efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
> > > > +efi_status_t efi_try_load_entry(u16 n, efi_handle_t *handle, void **load_options);
> > > >
> > > >  /**
> > > >   * struct efi_image_regions - A list of memory regions
> > > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > > > index 8c04ecbdc8..a3060b5c62 100644
> > > > --- a/lib/efi_loader/efi_bootmgr.c
> > > > +++ b/lib/efi_loader/efi_bootmgr.c
> > > > @@ -42,8 +42,7 @@ static const struct efi_runtime_services *rs;
> > > >   * @load_options:    load options set on the loaded image protocol
> > > >   * Return:           status code
> > > >   */
> > > > -static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> > > > -                                void **load_options)
> > > > +efi_status_t efi_try_load_entry(u16 n, efi_handle_t *handle, void **load_options)
> > > >  {
> > > >       struct efi_load_option lo;
> > > >       u16 varname[] = u"Boot0000";
> > > > @@ -165,7 +164,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
> > > >               /* load BootNext */
> > > >               if (ret == EFI_SUCCESS) {
> > > >                       if (size == sizeof(u16)) {
> > > > -                             ret = try_load_entry(bootnext, handle,
> > > > +                             ret = efi_try_load_entry(bootnext, handle,
> > > >                                                    load_options);
> > > >                               if (ret == EFI_SUCCESS)
> > > >                                       return ret;
> > > > @@ -189,7 +188,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
> > > >       for (i = 0; i < num; i++) {
> > > >               log_debug("%s trying to load Boot%04X\n", __func__,
> > > >                         bootorder[i]);
> > > > -             ret = try_load_entry(bootorder[i], handle, load_options);
> > > > +             ret = efi_try_load_entry(bootorder[i], handle, load_options);
> > > >               if (ret == EFI_SUCCESS)
> > > >                       break;
> > > >       }
> > > > --
> > > > 2.17.1
> > > >