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

[Heinrich Schuchardt]

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 at 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,