
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) &&
image_buf = __efi_helloworld_begin; size = __efi_helloworld_end - __efi_helloworld_begin; efi_clear_bootdev();!strcmp(argv[1], "hello")) {
- } 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,