[U-Boot] [PATCH v2 5/5] cmd: run: add "-e" option to run an EFI application

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
> On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
> > On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
> >> On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
> >>> "run -e" allows for executing EFI application with a specific "BootXXXX"
> >>> variable. If no "BootXXXX" is specified or "BootOrder" is specified,
> >>> it tries to run an EFI application specified in the order of "bootOrder."
> >>>
> >>
> >> If we cannot specify the device tree what would be the use of this for
> >> ARM processors?
> > 
> > I don't get your point. What's the matter with device tree?
> 
> To boot an ARM board on Linux or BSD you need a device tree.

When I discussed with Alex about Boot Manager (and distro_bootcmd?),
he suggested that we should not allow users to specify fdt at a command line.
I believe that it would apply to my case above.

IMO, we should always provide system's fdt to EFI applications.

> So run -e Boot0001 will not allow you to boot into Linux because it does
> not specify a device tree.
> 
> > 
> > 
> >> Why do you add the option to run and not to bootefi?
> >>
> >> You already introduced the capability to set BootNext. Why isn't that
> >> enough?
> > 
> > Simple.
> > 
> > => run -e Boot00>
> > versus
> > 
> > => efidebug boot next 1
> > => bootefi bootmgr
> 
> In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
> 
> So there is no need to go through efidebug.
> 
> I think we should avoid alternative commands.

As I said, I already removed this feature from bootefi.

> > 
> > First of all, efidebug is only recognized as a *debugging* tool.
> > I believe that the former syntax is more intuitive, and it looks
> > a natural extension to "run" command semantics akin to "env -e".
> > 
> > As a result, we don't have to know about bootefi for normal operations.
> 
> But you have to know about 'run' which you might not need otherwise.

"run" is much better known to U-Boot users than bootefi.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> > 
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> ---
> >>>  cmd/bootefi.c     | 31 +++++++++++++++++++++++++++++++
> >>>  cmd/nvedit.c      |  9 ++++++++-
> >>>  common/cli.c      | 10 ++++++++++
> >>>  include/command.h |  3 +++
> >>>  4 files changed, 52 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>> index 241fd0f987ab..ebe149dffa1f 100644
> >>> --- a/cmd/bootefi.c
> >>> +++ b/cmd/bootefi.c
> >>> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
> >>>  	return CMD_RET_SUCCESS;
> >>>  }
> >>>  
> >>> +/* Called by "run" command */
> >>> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>> +{
> >>> +	int boot_id = -1;
> >>> +	char *endp;
> >>> +
> >>> +	if (argc > 2)
> >>> +		return CMD_RET_USAGE;
> >>> +
> >>> +	if (argc == 2) {
> >>> +		if (!strcmp(argv[1], "BootOrder")) {
> >>> +			boot_id = -1;
> >>> +		} else if (!strncmp(argv[2], "Boot", 4)) {
> >>> +			boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0);
> >>> +			if ((argv[2] + strlen(argv[2]) != endp) ||
> >>> +			    boot_id > 0xffff)
> >>> +				return CMD_RET_USAGE;
> >>> +		} else {
> >>> +			return CMD_RET_USAGE;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	if (efi_init_obj_list())
> >>> +		return CMD_RET_FAILURE;
> >>> +
> >>> +	if (efi_handle_fdt(NULL))
> >>> +		return CMD_RET_FAILURE;
> >>> +
> >>> +	return do_bootefi_bootmgr_exec(boot_id);
> >>> +}
> >>> +
> >>>  /* Interpreter command to boot an arbitrary EFI image from memory */
> >>>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>  {
> >>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> >>> index de16c72c23f2..ce746bbf1b3e 100644
> >>> --- a/cmd/nvedit.c
> >>> +++ b/cmd/nvedit.c
> >>> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
> >>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
> >>>  	"run commands in an environment variable",
> >>>  	"var [...]\n"
> >>> -	"    - run the commands in the environment variable(s) 'var'",
> >>> +	"    - run the commands in the environment variable(s) 'var'"
> >>> +#if defined(CONFIG_CMD_BOOTEFI)
> >>> +	"\n"
> >>> +	"run -e [BootXXXX]\n"
> >>> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> >>> +#else
> >>> +	,
> >>> +#endif
> >>>  	var_complete
> >>>  );
> >>>  #endif
> >>> diff --git a/common/cli.c b/common/cli.c
> >>> index 51b8d5f85cbb..fbb09d5049a4 100644
> >>> --- a/common/cli.c
> >>> +++ b/common/cli.c
> >>> @@ -12,6 +12,7 @@
> >>>  #include <cli.h>
> >>>  #include <cli_hush.h>
> >>>  #include <console.h>
> >>> +#include <efi_loader.h>
> >>>  #include <fdtdec.h>
> >>>  #include <malloc.h>
> >>>  
> >>> @@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>  	if (argc < 2)
> >>>  		return CMD_RET_USAGE;
> >>>  
> >>> +#ifdef CONFIG_CMD_BOOTEFI
> >>> +	if (!strcmp(argv[1], "-e")) {
> >>> +		argc--;
> >>> +		argv++;
> >>> +
> >>> +		return do_bootefi_run(cmdtp, flag, argc, argv);
> >>> +	}
> >>> +#endif
> >>> +
> >>>  	for (i = 1; i < argc; ++i) {
> >>>  		char *arg;
> >>>  
> >>> diff --git a/include/command.h b/include/command.h
> >>> index 200c7a5e9f4e..feddef300ccc 100644
> >>> --- a/include/command.h
> >>> +++ b/include/command.h
> >>> @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s	cmd_tbl_t;
> >>>  #if defined(CONFIG_CMD_RUN)
> >>>  extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> >>>  #endif
> >>> +#if defined(CONFIG_CMD_BOOTEFI)
> >>> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> >>> +#endif
> >>>  
> >>>  /* common/command.c */
> >>>  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
> >>>
> >>
> > 
>