| Message ID | 20231121012950.156539-3-takahiro.akashi@linaro.org |
|---|---|
| State | Accepted |
| Commit | 296faf4f7ef15a3f9d5920b8dd247b4744e3e255 |
| Headers | show |
| Series | cmd: bootefi: refactor the code for bootmgr | expand |
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? > + 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? EFI_LOADER cannot be selected for SYS_CPU=armv7m. > 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. > + /* Initialize EFI drivers */ > + ret = efi_init_obj_list(); We should not duplicate this call for each sub-command. > + 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. Should we move the translations of the return codes into efi_install_fdt? 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,
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.) > > > + /* 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. > > > + 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, >
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 don't clean up the initialization on any failure, so why not move it on top of the function right after the argument parsing? > > > > > > + 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
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
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 > > > 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. 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
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
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 > + 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 CMD_BOOTEFI_HELLO_COMPILE makes no sense without a command to run the hello command. The CPU_V7M dependency is not needed as described in [PATCH 1/1] cmd: CONFIG_CMD_BOOTEFI_HELLO_COMPILE dependencies https://lore.kernel.org/u-boot/20231217114457.70128-1-heinrich.schuchardt@canonical.com/T/#u Best regards Heinrich > 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")) { > + /* 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_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,
On 12/17/23 13:00, 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 >> + 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 > > CMD_BOOTEFI_HELLO_COMPILE makes no sense without a command to run the > hello command. The CPU_V7M dependency is not needed as described in Missed that you pulled that CMD_BOOTEFI into the if above. > > [PATCH 1/1] cmd: CONFIG_CMD_BOOTEFI_HELLO_COMPILE dependencies > https://lore.kernel.org/u-boot/20231217114457.70128-1-heinrich.schuchardt@canonical.com/T/#u > > Best regards > > Heinrich > >> 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")) { >> + /* 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_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( >>CMD_BOOTEFI >> 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, >
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 + 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 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")) { + /* 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_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,
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(-)