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

AKASHI Takahiro
2019-02-28T04:45:16Z

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

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.

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