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.aka...@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.aka...@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.g...@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