Hi Ilias,
On Fri, Dec 08, 2023 at 08:33:11AM +0200, Ilias Apalodimas wrote:
> Akashi-san,
>
> [...]
>
> > > > > > 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.
>
> Not in board_init_r(). Perhaps we can have that as a Kconfig option
Then where do you want to call the function?
> >
> > > 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.
>
> Yes, but I prefer calling it once.
> Instead of duplicating the init on
> if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR) && ...
> if (IS_ENABLED(CONFIG_CMD_BOOTEFI_SELFTEST) && ...and then at the end
> simply move it to the top of the function. You are trying to call efi
> bootmgr, no one will care if the EFI subsystem comes up regardless of
> ifs and potential failures.
You are in the middle of refactoring here.
I would like you to see the *final* code after refactoring.
Then you will understand why I did so (duplicating efi_init_obj_list()
at each sub command parts).
-Takahiro Akashi
> Thanks
> /Ilias
>
>
>
> >
> > -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
