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

AKASHI Takahiro takahiro.akashi at linaro.org
Thu Feb 28 05:06:10 UTC 2019 

On Thu, Feb 28, 2019 at 05:53:17AM +0100, Heinrich Schuchardt wrote:
> 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.

Why do you think so?
Unless people intentionally run "fdt" command before bootefi,
the system will behave in the exact same way.

How many people really expect that, in the case below,
  => load ... <addr> <file>
  => fdt addr <addr>
  => bootefi bootmgr
bootefi will start EFI application *without* fdt?

-Takahiro Akashi

> 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