[U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables

[Heinrich Schuchardt]

On 12/18/18 6:05 AM, AKASHI Takahiro wrote:
> "env [print|set] -e" allows for handling uefi variables without
> knowing details about mapping to corresponding u-boot variables.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>

Hello Takahiro,

in several patch series you are implementing multiple interactive
commands that concern

- handling of EFI variables
- executing EFI binaries
- managing boot sequence

I very much appreciate your effort to provide an independent UEFI shell
implementation. What I am worried about is that your current patches
make it part of the monolithic U-Boot binary.

This design has multiple drawbacks:

The memory size available for U-Boot is very limited for many devices.
We already had to disable EFI_LOADER for some boards due to this
limitations. Hence we want to keep everything out of the U-Boot binary
that does not serve the primary goal of loading and executing the next
binary.

The UEFI forum has published a UEFI Shell specification which is very
extensive. We still have a lot of deficiencies in U-Boot's UEFI API
implementation. By merging in parts of an UEFI shell implementation our
project looses focus. There is an EDK2 implementation of said
specification. If we fix the remaining bugs of the EFI API
implementation in U-Boot we could simply run the EDK2 shell which
provides all that is needed for interactive work.

With you monolithic approach your UEFI shell implementation can neither
be used with other UEFI API implementations than U-Boot nor can it be
tested against other API implementations.

Due to these considerations I suggest that you build your UEFI shell
implementation as a separate UEFI binary (like helloworld.efi). You may
offer an embedding of the binary (like the bootefi hello command) into
the finally linked U-Boot binary via a configuration variable. Please,
put the shell implementation into a separate directory. You may want to
designate yourself as maintainer (in file MAINTAINERS).

Best regards

Heinrich


> ---
>  cmd/nvedit.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index c0facabfc4fe..8168c963ac9d 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -27,6 +27,8 @@
>  #include <cli.h>
>  #include <command.h>
>  #include <console.h>
> +#include <efi.h>
> +#include <efi_loader.h>
>  #include <environment.h>
>  #include <search.h>
>  #include <errno.h>
> @@ -119,6 +121,25 @@ static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc,
>  	int rcode = 0;
>  	int env_flag = H_HIDE_DOT;
>  
> +#if defined(CONFIG_CMD_EFISHELL)
> +	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') {
> +		efi_status_t r;
> +
> +		argc--;
> +		argv++;
> +
> +		/* Initialize EFI drivers */
> +		r = efi_init_obj_list();
> +		if (r != EFI_SUCCESS) {
> +			printf("Error: Cannot set up EFI drivers, r = %lu\n",
> +			       r & ~EFI_ERROR_MASK);
> +			return CMD_RET_FAILURE;
> +		}
> +
> +		return do_efi_dump_var(argc, argv);
> +	}
> +#endif
> +
>  	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'a') {
>  		argc--;
>  		argv++;
> @@ -216,6 +237,26 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
>  	ENTRY e, *ep;
>  
>  	debug("Initial value for argc=%d\n", argc);
> +
> +#if defined(CONFIG_CMD_EFISHELL)
> +	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') {
> +		efi_status_t r;
> +
> +		argc--;
> +		argv++;
> +
> +		/* Initialize EFI drivers */
> +		r = efi_init_obj_list();
> +		if (r != EFI_SUCCESS) {
> +			printf("Error: Cannot set up EFI drivers, r = %lu\n",
> +			       r & ~EFI_ERROR_MASK);
> +			return CMD_RET_FAILURE;
> +		}
> +
> +		return do_efi_set_var(argc, argv);
> +	}
> +#endif
> +
>  	while (argc > 1 && **(argv + 1) == '-') {
>  		char *arg = *++argv;
>  
> @@ -1262,15 +1303,23 @@ static char env_help_text[] =
>  #if defined(CONFIG_CMD_IMPORTENV)
>  	"env import [-d] [-t [-r] | -b | -c] addr [size] [var ...] - import environment\n"
>  #endif
> +#if defined(CONFIG_CMD_EFISHELL)
> +	"env print [-a | -e [name] | name ...] - print environment\n"
> +#else
>  	"env print [-a | name ...] - print environment\n"
> +#endif
>  #if defined(CONFIG_CMD_RUN)
>  	"env run var [...] - run commands in an environment variable\n"
>  #endif
>  #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
>  	"env save - save environment\n"
>  #endif
> +#if defined(CONFIG_CMD_EFISHELL)
> +	"env set [-e | -f] name [arg ...]\n";
> +#else
>  	"env set [-f] name [arg ...]\n";
>  #endif
> +#endif
>  
>  U_BOOT_CMD(
>  	env, CONFIG_SYS_MAXARGS, 1, do_env,
> @@ -1295,6 +1344,10 @@ U_BOOT_CMD_COMPLETE(
>  	printenv, CONFIG_SYS_MAXARGS, 1,	do_env_print,
>  	"print environment variables",
>  	"[-a]\n    - print [all] values of all environment variables\n"
> +#if defined(CONFIG_CMD_EFISHELL)
> +	"printenv -e [<name>]\n"
> +	"    - print UEFI variable 'name' or all the variables\n"
> +#endif
>  	"printenv name ...\n"
>  	"    - print value of environment variable 'name'",
>  	var_complete
> @@ -1322,7 +1375,11 @@ U_BOOT_CMD_COMPLETE(
>  U_BOOT_CMD_COMPLETE(
>  	setenv, CONFIG_SYS_MAXARGS, 0,	do_env_set,
>  	"set environment variables",
> -	"[-f] name value ...\n"
> +#if defined(CONFIG_CMD_EFISHELL)
> +	"-e <name> [<value>]\n"
> +	"    - set UEFI variable 'name' to 'value' ...'\n"
> +#endif
> +	"setenv [-f] name value ...\n"
>  	"    - [forcibly] set environment variable 'name' to 'value ...'\n"
>  	"setenv [-f] name\n"
>  	"    - [forcibly] delete environment variable 'name'",
> @@ -1343,7 +1400,7 @@ U_BOOT_CMD(
>  U_BOOT_CMD_COMPLETE(
>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
>  	"run commands in an environment variable",
> -#if defined(CONFIG_CMD_BOOTEFI)
> +#if defined(CONFIG_CMD_EFISHELL)
>  	"var -e [BootXXXX]\n"
>  	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
>  #else
>