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

AKASHI Takahiro takahiro.akashi at linaro.org
Fri Mar 1 01:22:13 UTC 2019 

On Thu, Feb 28, 2019 at 06:13:39AM +0100, Heinrich Schuchardt wrote:
> On 2/28/19 6:06 AM, AKASHI Takahiro wrote:
> > 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
> 
> Your previous mail sounded to me as if you wanted to drop the
> possibility to specify an FDT address in the bootefi command. But maybe
> I got you wrong.

No, even in v2, I didn't change any semantics in do_bootefi()
and hence bootefi command.

> If your idea is that we should use the address specified in command fdt
> and $fdtcontroladdr as fallbacks if no FDT address is specified, that is
> another story.

Exactly for "run -e" case (and probably a new command, efibootmgr, too).

The only concern is an incompatibility with distro_bootcmd.
It has yet another logic regarding fdt.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > 
> >> 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