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

Wed Feb 27 07:10:36 UTC 2019

On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
> 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.

With current Linux development practice this unfortunately does not
work. Linux device trees sometimes see incompatible changes between
versions. Booting may fail with a device tree that is either too old or
too new for your Linux version.

E.g. for the Odroid C2 some reserved memory regions were removed from
the device tree and replaced by a logic that determines them on the fly
due to changes in ARM trusted firmware location.

The Wandboard rev B1 device tree was moved to a new file when a new
board revision appeared and the new revision changed the old file (sic).

U-Boot is also not perfect at keeping its device tree in sync with the
newest Linux device tree.

> 
>> 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.

Do you have a statistic ;)

Up to now booting always required a command starting on boot...

Best regards

Heinrich

> 
> Thanks,
> -Takahiro Akashi
> 
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Thanks,
>>> -Takahiro Akashi
>>>
>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at 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
>>>>>
>>>>
>>>
>>
> 