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

AKASHI Takahiro
2018-12-03T07:57:49Z

On Mon, Dec 03, 2018 at 12:53:41AM +0100, Alexander Graf wrote:
>
>
> On 05.11.18 10:06, 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."
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> > cmd/bootefi.c | 2 +-
> > cmd/nvedit.c | 5 +++++
> > common/cli.c | 30 ++++++++++++++++++++++++++++++
> > include/command.h | 3 +++
> > 4 files changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 7580008f691b..3cefb4d0ecaa 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -509,7 +509,7 @@ exit:
> > return ret;
> > }
> >
> > -static int do_bootefi_bootmgr_exec(int boot_id)
> > +int do_bootefi_bootmgr_exec(int boot_id)
> > {
> > struct efi_device_path *device_path, *file_path;
> > void *addr;
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index de16c72c23f2..c0facabfc4fe 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -1343,8 +1343,13 @@ U_BOOT_CMD(
> > U_BOOT_CMD_COMPLETE(
> > run, CONFIG_SYS_MAXARGS, 1, do_run,
> > "run commands in an environment variable",
> > +#if defined(CONFIG_CMD_BOOTEFI)
> > + "var -e [BootXXXX]\n"
> > + " - load and run UEFI app based on 'BootXXXX' UEFI variable",
> > +#else
> > "var [...]\n"
> > " - run the commands in the environment variable(s) 'var'",
> > +#endif
> > var_complete
> > );
> > #endif
> > diff --git a/common/cli.c b/common/cli.c
> > index 51b8d5f85cbb..e4ec35b5eb00 100644
> > --- a/common/cli.c
> > +++ b/common/cli.c
> > @@ -14,6 +14,7 @@
> > #include <console.h>
> > #include <fdtdec.h>
> > #include <malloc.h>
> > +#include "../cmd/efishell.h"
>
> I guess this already shows that something is going wrong ;).

efishell.h here is included solely for efi_init_obj_list() definition
and so

> Please move the depending functions into lib/efi_loader. That way we can
> have the efishell command be optional, but keep this logic around
> regardless.

I can assume that you agree to this function being moved to efi_setup.c.

> >
> > DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -125,6 +126,35 @@ 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")) {
> > + int boot_id = -1;
> > + char *endp;
> > +
> > + if (argc == 3) {
> > + if (!strcmp(argv[2], "BootOrder")) {
> > + /* boot_id = -1 */;
>
> Just write that in code. The compiler will optimize it out.

OK.

> > + } 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);
> > + }
> > +#endif
> > +
> > for (i = 1; i < argc; ++i) {
> > char *arg;
> >
> > diff --git a/include/command.h b/include/command.h
> > index 5b1577f3b477..5bb675122cce 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)
> > +int do_bootefi_bootmgr_exec(int boot_id);
> > +#endif
>
> This needs to be in efi_loader.h which then needs to get included from
> cli.c.

After your comment above, you expect that do_bootefi_bootmgr_exec(),
hence do_bootefi_exec() (and efi_handle_fdt()?) as well, be also moved
to lib/efi_loader/(efi_bootmgr.c)?

Thanks,
-Takahiro Akashi

>
> Alex