Re: [PATCH v2 02/12] cmd: bootefi: re-organize do_bootefi()

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

On Tue, Dec 05, 2023 at 10:54:23AM +0200, Ilias Apalodimas wrote:
> Hello Akashi-san,
> Thanks for taking a shot at the cleanup
> 
> On Tue, 21 Nov 2023 at 06:53, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > Hi Heinrich,
> >
> > On Tue, Nov 21, 2023 at 04:31:40AM +0100, Heinrich Schuchardt wrote:
> > > On 11/21/23 02:29, AKASHI Takahiro wrote:
> > > > Replicate some code and re-organize do_bootefi() into three cases, which
> > > > will be carved out as independent functions in the next two commits.
> > > >
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > ---
> > > >   cmd/Kconfig          | 15 ++++++--
> > > >   cmd/bootefi.c        | 82 ++++++++++++++++++++++++++++++--------------
> > > >   include/efi_loader.h |  2 --
> > > >   3 files changed, 69 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > index 6f636155e5b6..4cf9a210c4a1 100644
> > > > --- a/cmd/Kconfig
> > > > +++ b/cmd/Kconfig
> > > > @@ -362,9 +362,19 @@ config CMD_BOOTEFI
> > > >     help
> > > >       Boot an EFI image from memory.
> > > >
> > > > +if CMD_BOOTEFI
> > > > +config CMD_BOOTEFI_BINARY
> > > > +   bool "Allow booting an EFI binary directly"
> > > > +   depends on BOOTEFI_BOOTMGR
> > >
> > > Why should booting a known binary depend on the boot manager?
> >
> > Because I tried to maintain the meaning of CONFIG_BOOTEFI_BOOTMGR
> > at this point of refactoring.
> > This configuration will eventually be changed to
> > config CMD_BOOTEFI_BINARY
> >         bool "Allow booting an EFI binary directly"
> >         depends on EFI_BINARY_EXEC
> >         default y
> > in patch#9.
> >
> > > > +   default y
> > > > +   help
> > > > +     Select this option to enable direct execution of binary at 'bootefi'.
> > > > +     This subcommand will allow you to load the UEFI binary using
> > > > +     other U-Boot commands or external methods and then run it.
> > > > +
> > > >   config CMD_BOOTEFI_BOOTMGR
> > > >     bool "UEFI Boot Manager command"
> > > > -   depends on BOOTEFI_BOOTMGR && CMD_BOOTEFI
> > > > +   depends on BOOTEFI_BOOTMGR
> > > >     default y
> > > >     help
> > > >       Select this option to enable the 'bootmgr' subcommand of 'bootefi'.
> > > > @@ -373,7 +383,7 @@ config CMD_BOOTEFI_BOOTMGR
> > > >
> > > >   config CMD_BOOTEFI_HELLO_COMPILE
> > > >     bool "Compile a standard EFI hello world binary for testing"
> > > > -   depends on CMD_BOOTEFI && !CPU_V7M
> > > > +   depends on !CPU_V7M
> > >
> > > Why do we have this dependency?
> >
> > CPU_V7M?
> > It was introduced in your commit:
> > ---
> > commit 0ea8741ff65e
> > Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Date:   Sun Dec 30 10:11:14 2018 +0100
> >
> >     efi_loader: CMD_BOOTEFI_HELLO_COMPILE in configs
> > ---
> >
> > > EFI_LOADER cannot be selected for SYS_CPU=armv7m.
> >
> > If not needed, you can delete it, but it is out of scope
> > of this patch series.
> >
> > > >     default y
> > > >     help
> > > >       This compiles a standard EFI hello world application with U-Boot so
> > > > @@ -395,6 +405,7 @@ config CMD_BOOTEFI_HELLO
> > > >       up EFI support on a new architecture.
> > > >
> > > >   source lib/efi_selftest/Kconfig
> > > > +endif
> > > >
> > > >   config CMD_BOOTMENU
> > > >     bool "bootmenu"
> > > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > > > index 190ccba260e0..e9e5ab67a1f5 100644
> > > > --- a/cmd/bootefi.c
> > > > +++ b/cmd/bootefi.c
> > > > @@ -503,7 +503,6 @@ out:
> > > >     return (ret != EFI_SUCCESS) ? ret : ret2;
> > > >   }
> > > >
> > > > -#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> > > >   static efi_status_t bootefi_run_prepare(const char *load_options_path,
> > > >             struct efi_device_path *device_path,
> > > >             struct efi_device_path *image_path,
> > > > @@ -593,7 +592,6 @@ static int do_efi_selftest(void)
> > > >
> > > >     return ret != EFI_SUCCESS;
> > > >   }
> > > > -#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
> > > >
> > > >   /**
> > > >    * do_bootefi() - execute `bootefi` command
> > > > @@ -615,14 +613,6 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> > > >     if (argc < 2)
> > > >             return CMD_RET_USAGE;
> > > >
> > > > -   /* Initialize EFI drivers */
> > > > -   ret = efi_init_obj_list();
> > > > -   if (ret != EFI_SUCCESS) {
> > > > -           log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> > > > -                   ret & ~EFI_ERROR_MASK);
> > > > -           return CMD_RET_FAILURE;
> > > > -   }
> > > > -
> > > >     if (argc > 2) {
> > > >             uintptr_t fdt_addr;
> > > >
> > > > @@ -631,29 +621,54 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> > > >     } else {
> > > >             fdt = EFI_FDT_USE_INTERNAL;
> > > >     }
> > > > -   ret = efi_install_fdt(fdt);
> > > > -   if (ret == EFI_INVALID_PARAMETER)
> > > > -           return CMD_RET_USAGE;
> > > > -   else if (ret != EFI_SUCCESS)
> > > > -           return CMD_RET_FAILURE;
> > > >
> > > > -   if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
> > > > -           if (!strcmp(argv[1], "bootmgr"))
> > > > -                   return do_efibootmgr();
> > > > +   if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR) &&
> > > > +       !strcmp(argv[1], "bootmgr")) {
> > >
> > >
> > > https://docs.u-boot.org/en/latest/develop/commands.html
> > > suggests to use U_BOOT_CMD_MKENT() to define sub-commands.
> >
> > As you know, these "if (!strcmp(argv[1], ...)" code exist since
> > the early days when efi_selftest and bootmgr sub-commands were
> > introduced in bootefi.
> >
> > In my personal preference, I would move bootmgr to a new independent
> > command, efi_selftest to efidebug, leaving only binary-execution
> > syntax in bootefi.
> > (So no sub-command.)
> 
> And that's a good idea, does anything prevent us from doing that ? The
> code works reliably as-is so if we are thinking about refactoring it,
> take a deeper dive and let's do all of it.
> 
> An idea would be to start with patches that move 'bootefi hello' and
> 'bootefi selftest' to the efidebug command?
> 
> >
> > >
> > > > +           /* Initialize EFI drivers */
> > > > +           ret = efi_init_obj_list();
> > >
> > > We should not duplicate this call for each sub-command.
> >
> > Please also take a look at the succeeding commits.
> > A call to efi_init_obj_list() will be included in independent
> > library functions, either efi_bootmgr_run(), efi_binary_run()
> > or do_bootefi() (for efi_selftest) so that a caller of these
> > functions doesn't have to know/care much about detailed APIs.
> 
> I am with Heinrich on this. Despite the further refactoring in
> subsequent patches, we could just initialize the EFI subsystem at the
> beginning. It will eventually be done by some part of u-boot and we

Yes if you want to always call efi_init_obj_list() in, say, board_init_r().
But we didn't take this approach by design because we wanted to initialize
the subsystem only if needed.

> don't clean up the initialization on any failure, so why not move it
> on top of the function right after the argument parsing?

I'm not sure what you mean here, but please remember that either
efi_binary_run() or efi_bootmgr_run() is more or less a utility
function which is convenient in order for other part of u-boot to utilize
efi subsystem easily. Since there is no guarantee that efi_init_obj_list()
is called before, it would be better to always call efi_init_obj_list()
in those functions.

-Takahiro Akashi


> >
> > >
> > > > +           if (ret != EFI_SUCCESS) {
> > > > +                   log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> > > > +                           ret & ~EFI_ERROR_MASK);
> > > > +                   return CMD_RET_FAILURE;
> > > > +           }
> > > > +
> > > > +           ret = efi_install_fdt(fdt);
> > > > +           if (ret == EFI_INVALID_PARAMETER)
> > > > +                   return CMD_RET_USAGE;
> > > > +           else if (ret != EFI_SUCCESS)
> > > > +                   return CMD_RET_FAILURE;
> > >
> > > These lines could be moved into do_efibootmgr.
> >
> > It will be done in patch#3 when carving out bootmgr specific code.
> >
> > > Should we move the translations of the return codes into efi_install_fdt?
> >
> > No, I don't think so. efi_install_fdt() can be called not only from
> > the command (bootefi) but also from other library code (at least,
> > efi_bootmgr_run() and efi_binary_run()).
> >
> > -Takahiro Akashi
> >
> > > Best regards
> > >
> > > Heinrich
> > >
> > > > +
> > > > +           return do_efibootmgr();
> > > >     }
> > > > -#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> > > > -   if (!strcmp(argv[1], "selftest"))
> > > > +
> > > > +   if (IS_ENABLED(CONFIG_CMD_BOOTEFI_SELFTEST) &&
> > > > +       !strcmp(argv[1], "selftest")) {
> > > > +           /* Initialize EFI drivers */
> > > > +           ret = efi_init_obj_list();
> > > > +           if (ret != EFI_SUCCESS) {
> > > > +                   log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> > > > +                           ret & ~EFI_ERROR_MASK);
> > > > +                   return CMD_RET_FAILURE;
> > > > +           }
> > > > +
> > > > +           ret = efi_install_fdt(fdt);
> > > > +           if (ret == EFI_INVALID_PARAMETER)
> > > > +                   return CMD_RET_USAGE;
> > > > +           else if (ret != EFI_SUCCESS)
> > > > +                   return CMD_RET_FAILURE;
> > > > +
> > > >             return do_efi_selftest();
> > > > -#endif
> > > > +   }
> > > >
> > > > -#ifdef CONFIG_CMD_BOOTEFI_HELLO
> > > > -   if (!strcmp(argv[1], "hello")) {
> > > > +   if (!IS_ENABLED(CONFIG_CMD_BOOTEFI_BINARY))
> > > > +           return CMD_RET_SUCCESS;
> > > > +
> > > > +   if (IS_ENABLED(CONFIG_CMD_BOOTEFI_HELLO) &&
> > > > +       !strcmp(argv[1], "hello")) {
> > > >             image_buf = __efi_helloworld_begin;
> > > >             size = __efi_helloworld_end - __efi_helloworld_begin;
> > > >             efi_clear_bootdev();
> > > > -   } else
> > > > -#endif
> > > > -   {
> > > > +   } else {
> > > >             addr = strtoul(argv[1], NULL, 16);
> > > >             /* Check that a numeric value was passed */
> > > >             if (!addr)
> > > > @@ -675,6 +690,21 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> > > >                     size = image_size;
> > > >             }
> > > >     }
> > > > +
> > > > +   /* Initialize EFI drivers */
> > > > +   ret = efi_init_obj_list();
> > > > +   if (ret != EFI_SUCCESS) {
> > > > +           log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> > > > +                   ret & ~EFI_ERROR_MASK);
> > > > +           return CMD_RET_FAILURE;
> > > > +   }
> > > > +
> > > > +   ret = efi_install_fdt(fdt);
> > > > +   if (ret == EFI_INVALID_PARAMETER)
> > > > +           return CMD_RET_USAGE;
> > > > +   else if (ret != EFI_SUCCESS)
> > > > +           return CMD_RET_FAILURE;
> > > > +
> > > >     ret = efi_run_image(image_buf, size);
> > > >
> > > >     if (ret != EFI_SUCCESS)
> > > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > > index 664dae28f882..44436d346286 100644
> > > > --- a/include/efi_loader.h
> > > > +++ b/include/efi_loader.h
> > > > @@ -879,14 +879,12 @@ efi_status_t __efi_runtime EFIAPI efi_get_time(
> > > >
> > > >   efi_status_t __efi_runtime EFIAPI efi_set_time(struct efi_time *time);
> > > >
> > > > -#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> > > >   /*
> > > >    * Entry point for the tests of the EFI API.
> > > >    * It is called by 'bootefi selftest'
> > > >    */
> > > >   efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
> > > >                              struct efi_system_table *systab);
> > > > -#endif
> > > >
> > > >   efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> > > >                                  const efi_guid_t *vendor, u32 *attributes,
> > >
> 
> Thanks
> /Ilias