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

Heinrich Schuchardt xypron.glpk at gmx.de
Thu Feb 28 04:53:17 UTC 2019 

On 2/28/19 5:45 AM, AKASHI Takahiro wrote:
> On Wed, Feb 27, 2019 at 08:10:36AM +0100, Heinrich Schuchardt wrote:
>> 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.
> 
> Why don't you use "fdt" command in that case?
> IMO, we don't need <fdt> argument at bootefi (and run -e).
> Obviously, I have one assumption that we need change the code
> to utilize "fdtaddr" variable in do_bootefi().

Such a change would mean that after an upgrade of U-Boot all boards
running on Suse and Fedora suddenly will not boot again.

We should not change existing commands in an incompatible way.

> 
> Under the current implementation, a similar behavior is achieved
> only via distro_bootcmd. IMO, this should be corrected.
> If you agree, I will add another patch to my current patchset
> for this purpose.

I suggest to drop patch 5/5 from your series.

Best regards

Heinrich

> 
>>>
>>>> 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...
> 
> What I meant is that people need not learn more commands.
> 
> # Relating to secure boot, I'm now thinking of pulling bootmgr out of
> # bootefi and making it as a single command. In that case,
> # bootefi does exist only for hello and selftest.
> 
> -Takahiro Akashi
> 
>>
>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

More information about the U-Boot mailing list