| Message ID | 20190115025437.11966-6-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: run a specific efi application more easily | expand |
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? 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? 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 >
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? > 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 Boot0001 versus => efidebug boot next 1 => bootefi bootmgr 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. 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 > > >
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. 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. > > 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. 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 >>> >> >
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. > 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. 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 > >>> > >> > > >
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. > >> 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... 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 >>>>> >>>> >>> >> >
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 > >>>>> > >>>> > >>> > >> > > >
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@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 >>>>>>> >>>>>> >>>>> >>>> >>> >> >
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@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 > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > >
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. 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. 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@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 >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
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." > > 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); This function does not invoke: allow_unaligned(); switch_to_non_secure_mode(); Probably these calls should be moved to efi_init_obj_list() anyway. Best regards Heinrich > +} > + > /* 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 >
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@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 > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > >
On Thu, Feb 28, 2019 at 09:59:14PM +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." > > > > 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); > > This function does not invoke: > > allow_unaligned(); > switch_to_non_secure_mode(); I've already fixed it in my pre-v3 patch. > Probably these calls should be moved to efi_init_obj_list() anyway. Can be, but the name no longer represents its function. -Takahiro Akashi > > Best regards > > Heinrich > > > +} > > + > > /* 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 > > >
Heinrich, On Fri, Mar 01, 2019 at 10:22:13AM +0900, AKASHI Takahiro wrote: > 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. I changed my mind a bit. I dropped "run -e" patch, and posted "BootNext" patch on its own. I'd like to focus more on re-working do_bootefi(). See my next RFC. Thanks, -Takahiro Akashi > -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@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 > > >>>>>>>>> > > >>>>>>>> > > >>>>>>> > > >>>>>> > > >>>>> > > >>>> > > >>> > > >> > > > > >
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
"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 | 31 +++++++++++++++++++++++++++++++ cmd/nvedit.c | 9 ++++++++- common/cli.c | 10 ++++++++++ include/command.h | 3 +++ 4 files changed, 52 insertions(+), 1 deletion(-)