| Message ID | 20181218050510.20308-9-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cmd: add efishell for efi environment | expand |
On 12/18/18 6:05 AM, AKASHI Takahiro wrote: > "env [print|set] -e" allows for handling uefi variables without > knowing details about mapping to corresponding u-boot variables. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> Hello Takahiro, in several patch series you are implementing multiple interactive commands that concern - handling of EFI variables - executing EFI binaries - managing boot sequence I very much appreciate your effort to provide an independent UEFI shell implementation. What I am worried about is that your current patches make it part of the monolithic U-Boot binary. This design has multiple drawbacks: The memory size available for U-Boot is very limited for many devices. We already had to disable EFI_LOADER for some boards due to this limitations. Hence we want to keep everything out of the U-Boot binary that does not serve the primary goal of loading and executing the next binary. The UEFI forum has published a UEFI Shell specification which is very extensive. We still have a lot of deficiencies in U-Boot's UEFI API implementation. By merging in parts of an UEFI shell implementation our project looses focus. There is an EDK2 implementation of said specification. If we fix the remaining bugs of the EFI API implementation in U-Boot we could simply run the EDK2 shell which provides all that is needed for interactive work. With you monolithic approach your UEFI shell implementation can neither be used with other UEFI API implementations than U-Boot nor can it be tested against other API implementations. Due to these considerations I suggest that you build your UEFI shell implementation as a separate UEFI binary (like helloworld.efi). You may offer an embedding of the binary (like the bootefi hello command) into the finally linked U-Boot binary via a configuration variable. Please, put the shell implementation into a separate directory. You may want to designate yourself as maintainer (in file MAINTAINERS). Best regards Heinrich > --- > cmd/nvedit.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 59 insertions(+), 2 deletions(-) > > diff --git a/cmd/nvedit.c b/cmd/nvedit.c > index c0facabfc4fe..8168c963ac9d 100644 > --- a/cmd/nvedit.c > +++ b/cmd/nvedit.c > @@ -27,6 +27,8 @@ > #include <cli.h> > #include <command.h> > #include <console.h> > +#include <efi.h> > +#include <efi_loader.h> > #include <environment.h> > #include <search.h> > #include <errno.h> > @@ -119,6 +121,25 @@ static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc, > int rcode = 0; > int env_flag = H_HIDE_DOT; > > +#if defined(CONFIG_CMD_EFISHELL) > + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') { > + efi_status_t r; > + > + argc--; > + argv++; > + > + /* Initialize EFI drivers */ > + r = efi_init_obj_list(); > + if (r != EFI_SUCCESS) { > + printf("Error: Cannot set up EFI drivers, r = %lu\n", > + r & ~EFI_ERROR_MASK); > + return CMD_RET_FAILURE; > + } > + > + return do_efi_dump_var(argc, argv); > + } > +#endif > + > if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'a') { > argc--; > argv++; > @@ -216,6 +237,26 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag) > ENTRY e, *ep; > > debug("Initial value for argc=%d\n", argc); > + > +#if defined(CONFIG_CMD_EFISHELL) > + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') { > + efi_status_t r; > + > + argc--; > + argv++; > + > + /* Initialize EFI drivers */ > + r = efi_init_obj_list(); > + if (r != EFI_SUCCESS) { > + printf("Error: Cannot set up EFI drivers, r = %lu\n", > + r & ~EFI_ERROR_MASK); > + return CMD_RET_FAILURE; > + } > + > + return do_efi_set_var(argc, argv); > + } > +#endif > + > while (argc > 1 && **(argv + 1) == '-') { > char *arg = *++argv; > > @@ -1262,15 +1303,23 @@ static char env_help_text[] = > #if defined(CONFIG_CMD_IMPORTENV) > "env import [-d] [-t [-r] | -b | -c] addr [size] [var ...] - import environment\n" > #endif > +#if defined(CONFIG_CMD_EFISHELL) > + "env print [-a | -e [name] | name ...] - print environment\n" > +#else > "env print [-a | name ...] - print environment\n" > +#endif > #if defined(CONFIG_CMD_RUN) > "env run var [...] - run commands in an environment variable\n" > #endif > #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE) > "env save - save environment\n" > #endif > +#if defined(CONFIG_CMD_EFISHELL) > + "env set [-e | -f] name [arg ...]\n"; > +#else > "env set [-f] name [arg ...]\n"; > #endif > +#endif > > U_BOOT_CMD( > env, CONFIG_SYS_MAXARGS, 1, do_env, > @@ -1295,6 +1344,10 @@ U_BOOT_CMD_COMPLETE( > printenv, CONFIG_SYS_MAXARGS, 1, do_env_print, > "print environment variables", > "[-a]\n - print [all] values of all environment variables\n" > +#if defined(CONFIG_CMD_EFISHELL) > + "printenv -e [<name>]\n" > + " - print UEFI variable 'name' or all the variables\n" > +#endif > "printenv name ...\n" > " - print value of environment variable 'name'", > var_complete > @@ -1322,7 +1375,11 @@ U_BOOT_CMD_COMPLETE( > U_BOOT_CMD_COMPLETE( > setenv, CONFIG_SYS_MAXARGS, 0, do_env_set, > "set environment variables", > - "[-f] name value ...\n" > +#if defined(CONFIG_CMD_EFISHELL) > + "-e <name> [<value>]\n" > + " - set UEFI variable 'name' to 'value' ...'\n" > +#endif > + "setenv [-f] name value ...\n" > " - [forcibly] set environment variable 'name' to 'value ...'\n" > "setenv [-f] name\n" > " - [forcibly] delete environment variable 'name'", > @@ -1343,7 +1400,7 @@ U_BOOT_CMD( > U_BOOT_CMD_COMPLETE( > run, CONFIG_SYS_MAXARGS, 1, do_run, > "run commands in an environment variable", > -#if defined(CONFIG_CMD_BOOTEFI) > +#if defined(CONFIG_CMD_EFISHELL) > "var -e [BootXXXX]\n" > " - load and run UEFI app based on 'BootXXXX' UEFI variable", > #else >
Heinrich, On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote: > On 12/18/18 6:05 AM, AKASHI Takahiro wrote: > > "env [print|set] -e" allows for handling uefi variables without > > knowing details about mapping to corresponding u-boot variables. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > Hello Takahiro, > > in several patch series you are implementing multiple interactive > commands that concern > > - handling of EFI variables > - executing EFI binaries > - managing boot sequence > > I very much appreciate your effort to provide an independent UEFI shell > implementation. What I am worried about is that your current patches > make it part of the monolithic U-Boot binary. First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's comment on v2. So you can disable efishell command if you don't want it. Are you still worried? > This design has multiple drawbacks: > > The memory size available for U-Boot is very limited for many devices. > We already had to disable EFI_LOADER for some boards due to this > limitations. Hence we want to keep everything out of the U-Boot binary > that does not serve the primary goal of loading and executing the next > binary. I don't know your point here. If EFI_LOADER is disabled, efishell will never be compiled in. > The UEFI forum has published a UEFI Shell specification which is very > extensive. We still have a lot of deficiencies in U-Boot's UEFI API > implementation. By merging in parts of an UEFI shell implementation our > project looses focus. What is "our project?" What is "focus?" I'm just asking as I want to share that information with you. > There is an EDK2 implementation of said > specification. If we fix the remaining bugs of the EFI API > implementation in U-Boot we could simply run the EDK2 shell which > provides all that is needed for interactive work. > > With you monolithic approach your UEFI shell implementation can neither > be used with other UEFI API implementations than U-Boot nor can it be > tested against other API implementations. Let me explain my stance. My efishell is basically something like a pursuit as well as a debug/test tool which was and is still quite useful for me. Without it, I would have completed (most of) my efi-related work so far. So I believe that it will also be useful for other people who may want to get involved and play with u-boot's efi environment. I have never intended to fully implement a shell which is to be compliant with UEFI specification while I'm trying to mimick some command interfaces for convenience. UEFI shell, as you know, provides plenty of "protocols" on which some UEFI applications, including UEFI SCT, reply. I will never implement it with my efishell. I hope that my efishell is a quick and easy way of learning more about u-boot's uefi environment. I will be even happier if more people get involved there. > Due to these considerations I suggest that you build your UEFI shell > implementation as a separate UEFI binary (like helloworld.efi). You may > offer an embedding of the binary (like the bootefi hello command) into > the finally linked U-Boot binary via a configuration variable. Please, > put the shell implementation into a separate directory. You may want to > designate yourself as maintainer (in file MAINTAINERS). Yeah, your suggestion is reasonable and I have thought of it before. There are, however, several reasons that I haven't done so; particularly, efishell is implemented not only with boottime services but also other helper functions, say, from device path utilities. Exporting them as libraries is possible but I don't think that it would be so valuable. Even if efishell is a separate application, it will not contribute to reduce the total footprint if it is embedded along with u-boot binary. Thanks, -Takahiro Akashi > Best regards > > Heinrich > > > > --- > > cmd/nvedit.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 59 insertions(+), 2 deletions(-) > > > > diff --git a/cmd/nvedit.c b/cmd/nvedit.c > > index c0facabfc4fe..8168c963ac9d 100644 > > --- a/cmd/nvedit.c > > +++ b/cmd/nvedit.c > > @@ -27,6 +27,8 @@ > > #include <cli.h> > > #include <command.h> > > #include <console.h> > > +#include <efi.h> > > +#include <efi_loader.h> > > #include <environment.h> > > #include <search.h> > > #include <errno.h> > > @@ -119,6 +121,25 @@ static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc, > > int rcode = 0; > > int env_flag = H_HIDE_DOT; > > > > +#if defined(CONFIG_CMD_EFISHELL) > > + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') { > > + efi_status_t r; > > + > > + argc--; > > + argv++; > > + > > + /* Initialize EFI drivers */ > > + r = efi_init_obj_list(); > > + if (r != EFI_SUCCESS) { > > + printf("Error: Cannot set up EFI drivers, r = %lu\n", > > + r & ~EFI_ERROR_MASK); > > + return CMD_RET_FAILURE; > > + } > > + > > + return do_efi_dump_var(argc, argv); > > + } > > +#endif > > + > > if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'a') { > > argc--; > > argv++; > > @@ -216,6 +237,26 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag) > > ENTRY e, *ep; > > > > debug("Initial value for argc=%d\n", argc); > > + > > +#if defined(CONFIG_CMD_EFISHELL) > > + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') { > > + efi_status_t r; > > + > > + argc--; > > + argv++; > > + > > + /* Initialize EFI drivers */ > > + r = efi_init_obj_list(); > > + if (r != EFI_SUCCESS) { > > + printf("Error: Cannot set up EFI drivers, r = %lu\n", > > + r & ~EFI_ERROR_MASK); > > + return CMD_RET_FAILURE; > > + } > > + > > + return do_efi_set_var(argc, argv); > > + } > > +#endif > > + > > while (argc > 1 && **(argv + 1) == '-') { > > char *arg = *++argv; > > > > @@ -1262,15 +1303,23 @@ static char env_help_text[] = > > #if defined(CONFIG_CMD_IMPORTENV) > > "env import [-d] [-t [-r] | -b | -c] addr [size] [var ...] - import environment\n" > > #endif > > +#if defined(CONFIG_CMD_EFISHELL) > > + "env print [-a | -e [name] | name ...] - print environment\n" > > +#else > > "env print [-a | name ...] - print environment\n" > > +#endif > > #if defined(CONFIG_CMD_RUN) > > "env run var [...] - run commands in an environment variable\n" > > #endif > > #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE) > > "env save - save environment\n" > > #endif > > +#if defined(CONFIG_CMD_EFISHELL) > > + "env set [-e | -f] name [arg ...]\n"; > > +#else > > "env set [-f] name [arg ...]\n"; > > #endif > > +#endif > > > > U_BOOT_CMD( > > env, CONFIG_SYS_MAXARGS, 1, do_env, > > @@ -1295,6 +1344,10 @@ U_BOOT_CMD_COMPLETE( > > printenv, CONFIG_SYS_MAXARGS, 1, do_env_print, > > "print environment variables", > > "[-a]\n - print [all] values of all environment variables\n" > > +#if defined(CONFIG_CMD_EFISHELL) > > + "printenv -e [<name>]\n" > > + " - print UEFI variable 'name' or all the variables\n" > > +#endif > > "printenv name ...\n" > > " - print value of environment variable 'name'", > > var_complete > > @@ -1322,7 +1375,11 @@ U_BOOT_CMD_COMPLETE( > > U_BOOT_CMD_COMPLETE( > > setenv, CONFIG_SYS_MAXARGS, 0, do_env_set, > > "set environment variables", > > - "[-f] name value ...\n" > > +#if defined(CONFIG_CMD_EFISHELL) > > + "-e <name> [<value>]\n" > > + " - set UEFI variable 'name' to 'value' ...'\n" > > +#endif > > + "setenv [-f] name value ...\n" > > " - [forcibly] set environment variable 'name' to 'value ...'\n" > > "setenv [-f] name\n" > > " - [forcibly] delete environment variable 'name'", > > @@ -1343,7 +1400,7 @@ U_BOOT_CMD( > > U_BOOT_CMD_COMPLETE( > > run, CONFIG_SYS_MAXARGS, 1, do_run, > > "run commands in an environment variable", > > -#if defined(CONFIG_CMD_BOOTEFI) > > +#if defined(CONFIG_CMD_EFISHELL) > > "var -e [BootXXXX]\n" > > " - load and run UEFI app based on 'BootXXXX' UEFI variable", > > #else > > >
On 12/19/18 2:49 AM, AKASHI Takahiro wrote: > Heinrich, > > On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote: >> On 12/18/18 6:05 AM, AKASHI Takahiro wrote: >>> "env [print|set] -e" allows for handling uefi variables without >>> knowing details about mapping to corresponding u-boot variables. >>> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> >> Hello Takahiro, >> >> in several patch series you are implementing multiple interactive >> commands that concern >> >> - handling of EFI variables >> - executing EFI binaries >> - managing boot sequence >> >> I very much appreciate your effort to provide an independent UEFI shell >> implementation. What I am worried about is that your current patches >> make it part of the monolithic U-Boot binary. > > First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's > comment on v2. So you can disable efishell command if you don't want it. > > Are you still worried? > >> This design has multiple drawbacks: >> >> The memory size available for U-Boot is very limited for many devices. >> We already had to disable EFI_LOADER for some boards due to this >> limitations. Hence we want to keep everything out of the U-Boot binary >> that does not serve the primary goal of loading and executing the next >> binary. > > I don't know your point here. If EFI_LOADER is disabled, efishell > will never be compiled in. > >> The UEFI forum has published a UEFI Shell specification which is very >> extensive. We still have a lot of deficiencies in U-Boot's UEFI API >> implementation. By merging in parts of an UEFI shell implementation our >> project looses focus. > > What is "our project?" What is "focus?" > I'm just asking as I want to share that information with you. > >> There is an EDK2 implementation of said >> specification. If we fix the remaining bugs of the EFI API >> implementation in U-Boot we could simply run the EDK2 shell which >> provides all that is needed for interactive work. >> >> With you monolithic approach your UEFI shell implementation can neither >> be used with other UEFI API implementations than U-Boot nor can it be >> tested against other API implementations. > > Let me explain my stance. > My efishell is basically something like a pursuit as well as > a debug/test tool which was and is still quite useful for me. > Without it, I would have completed (most of) my efi-related work so far. > So I believe that it will also be useful for other people who may want > to get involved and play with u-boot's efi environment. On SD-Cards U-Boot is installed between the MBR and the first partition. On other devices it is put into a very small ROM. Both ways the maximum size is rather limited. U-Boot provides all that is needed to load and execute an EFI binary. So you can put your efishell as file into the EFI partition like you would install the EDK2 shell. The only hardshift this approach brings is that you have to implement your own printf because UEFI does not offer formatted output. But this can be copied from lib/efi_selftest/efi_selftest_console.c. The same decision I took for booting from iSCSI. I did not try to put an iSCSI driver into U-Boot instead I use iPXE as an executable that is loaded from the EFI partition. > > I have never intended to fully implement a shell which is to be compliant > with UEFI specification while I'm trying to mimick some command > interfaces for convenience. UEFI shell, as you know, provides plenty > of "protocols" on which some UEFI applications, including UEFI SCT, > reply. I will never implement it with my efishell. > > I hope that my efishell is a quick and easy way of learning more about > u-boot's uefi environment. I will be even happier if more people > get involved there. > >> Due to these considerations I suggest that you build your UEFI shell >> implementation as a separate UEFI binary (like helloworld.efi). You may >> offer an embedding of the binary (like the bootefi hello command) into >> the finally linked U-Boot binary via a configuration variable. Please, >> put the shell implementation into a separate directory. You may want to >> designate yourself as maintainer (in file MAINTAINERS). > > Yeah, your suggestion is reasonable and I have thought of it before. > There are, however, several reasons that I haven't done so; particularly, > efishell is implemented not only with boottime services but also > other helper functions, say, from device path utilities. Exporting them > as libraries is possible but I don't think that it would be so valuable. > > Even if efishell is a separate application, it will not contribute to > reduce the total footprint if it is embedded along with u-boot binary. That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into the U-Boot binary - is default no. Same I would do for efishell.efi. Best regards Heinrich > > Thanks, > -Takahiro Akashi > >> Best regards >> >> Heinrich >> >> >>> --- >>> cmd/nvedit.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 59 insertions(+), 2 deletions(-) >>> >>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c >>> index c0facabfc4fe..8168c963ac9d 100644 >>> --- a/cmd/nvedit.c >>> +++ b/cmd/nvedit.c >>> @@ -27,6 +27,8 @@ >>> #include <cli.h> >>> #include <command.h> >>> #include <console.h> >>> +#include <efi.h> >>> +#include <efi_loader.h> >>> #include <environment.h> >>> #include <search.h> >>> #include <errno.h> >>> @@ -119,6 +121,25 @@ static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc, >>> int rcode = 0; >>> int env_flag = H_HIDE_DOT; >>> >>> +#if defined(CONFIG_CMD_EFISHELL) >>> + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') { >>> + efi_status_t r; >>> + >>> + argc--; >>> + argv++; >>> + >>> + /* Initialize EFI drivers */ >>> + r = efi_init_obj_list(); >>> + if (r != EFI_SUCCESS) { >>> + printf("Error: Cannot set up EFI drivers, r = %lu\n", >>> + r & ~EFI_ERROR_MASK); >>> + return CMD_RET_FAILURE; >>> + } >>> + >>> + return do_efi_dump_var(argc, argv); >>> + } >>> +#endif >>> + >>> if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'a') { >>> argc--; >>> argv++; >>> @@ -216,6 +237,26 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag) >>> ENTRY e, *ep; >>> >>> debug("Initial value for argc=%d\n", argc); >>> + >>> +#if defined(CONFIG_CMD_EFISHELL) >>> + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') { >>> + efi_status_t r; >>> + >>> + argc--; >>> + argv++; >>> + >>> + /* Initialize EFI drivers */ >>> + r = efi_init_obj_list(); >>> + if (r != EFI_SUCCESS) { >>> + printf("Error: Cannot set up EFI drivers, r = %lu\n", >>> + r & ~EFI_ERROR_MASK); >>> + return CMD_RET_FAILURE; >>> + } >>> + >>> + return do_efi_set_var(argc, argv); >>> + } >>> +#endif >>> + >>> while (argc > 1 && **(argv + 1) == '-') { >>> char *arg = *++argv; >>> >>> @@ -1262,15 +1303,23 @@ static char env_help_text[] = >>> #if defined(CONFIG_CMD_IMPORTENV) >>> "env import [-d] [-t [-r] | -b | -c] addr [size] [var ...] - import environment\n" >>> #endif >>> +#if defined(CONFIG_CMD_EFISHELL) >>> + "env print [-a | -e [name] | name ...] - print environment\n" >>> +#else >>> "env print [-a | name ...] - print environment\n" >>> +#endif >>> #if defined(CONFIG_CMD_RUN) >>> "env run var [...] - run commands in an environment variable\n" >>> #endif >>> #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE) >>> "env save - save environment\n" >>> #endif >>> +#if defined(CONFIG_CMD_EFISHELL) >>> + "env set [-e | -f] name [arg ...]\n"; >>> +#else >>> "env set [-f] name [arg ...]\n"; >>> #endif >>> +#endif >>> >>> U_BOOT_CMD( >>> env, CONFIG_SYS_MAXARGS, 1, do_env, >>> @@ -1295,6 +1344,10 @@ U_BOOT_CMD_COMPLETE( >>> printenv, CONFIG_SYS_MAXARGS, 1, do_env_print, >>> "print environment variables", >>> "[-a]\n - print [all] values of all environment variables\n" >>> +#if defined(CONFIG_CMD_EFISHELL) >>> + "printenv -e [<name>]\n" >>> + " - print UEFI variable 'name' or all the variables\n" >>> +#endif >>> "printenv name ...\n" >>> " - print value of environment variable 'name'", >>> var_complete >>> @@ -1322,7 +1375,11 @@ U_BOOT_CMD_COMPLETE( >>> U_BOOT_CMD_COMPLETE( >>> setenv, CONFIG_SYS_MAXARGS, 0, do_env_set, >>> "set environment variables", >>> - "[-f] name value ...\n" >>> +#if defined(CONFIG_CMD_EFISHELL) >>> + "-e <name> [<value>]\n" >>> + " - set UEFI variable 'name' to 'value' ...'\n" >>> +#endif >>> + "setenv [-f] name value ...\n" >>> " - [forcibly] set environment variable 'name' to 'value ...'\n" >>> "setenv [-f] name\n" >>> " - [forcibly] delete environment variable 'name'", >>> @@ -1343,7 +1400,7 @@ U_BOOT_CMD( >>> U_BOOT_CMD_COMPLETE( >>> run, CONFIG_SYS_MAXARGS, 1, do_run, >>> "run commands in an environment variable", >>> -#if defined(CONFIG_CMD_BOOTEFI) >>> +#if defined(CONFIG_CMD_EFISHELL) >>> "var -e [BootXXXX]\n" >>> " - load and run UEFI app based on 'BootXXXX' UEFI variable", >>> #else >>> >> >
On 19.12.18 13:23, Heinrich Schuchardt wrote: > On 12/19/18 2:49 AM, AKASHI Takahiro wrote: >> Heinrich, >> >> On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote: >>> On 12/18/18 6:05 AM, AKASHI Takahiro wrote: >>>> "env [print|set] -e" allows for handling uefi variables without >>>> knowing details about mapping to corresponding u-boot variables. >>>> >>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> >>> Hello Takahiro, >>> >>> in several patch series you are implementing multiple interactive >>> commands that concern >>> >>> - handling of EFI variables >>> - executing EFI binaries >>> - managing boot sequence >>> >>> I very much appreciate your effort to provide an independent UEFI shell >>> implementation. What I am worried about is that your current patches >>> make it part of the monolithic U-Boot binary. >> >> First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's >> comment on v2. So you can disable efishell command if you don't want it. >> >> Are you still worried? >> >>> This design has multiple drawbacks: >>> >>> The memory size available for U-Boot is very limited for many devices. >>> We already had to disable EFI_LOADER for some boards due to this >>> limitations. Hence we want to keep everything out of the U-Boot binary >>> that does not serve the primary goal of loading and executing the next >>> binary. >> >> I don't know your point here. If EFI_LOADER is disabled, efishell >> will never be compiled in. >> >>> The UEFI forum has published a UEFI Shell specification which is very >>> extensive. We still have a lot of deficiencies in U-Boot's UEFI API >>> implementation. By merging in parts of an UEFI shell implementation our >>> project looses focus. >> >> What is "our project?" What is "focus?" >> I'm just asking as I want to share that information with you. >> >>> There is an EDK2 implementation of said >>> specification. If we fix the remaining bugs of the EFI API >>> implementation in U-Boot we could simply run the EDK2 shell which >>> provides all that is needed for interactive work. >>> >>> With you monolithic approach your UEFI shell implementation can neither >>> be used with other UEFI API implementations than U-Boot nor can it be >>> tested against other API implementations. >> >> Let me explain my stance. >> My efishell is basically something like a pursuit as well as >> a debug/test tool which was and is still quite useful for me. >> Without it, I would have completed (most of) my efi-related work so far. >> So I believe that it will also be useful for other people who may want >> to get involved and play with u-boot's efi environment. > > On SD-Cards U-Boot is installed between the MBR and the first partition. > On other devices it is put into a very small ROM. Both ways the maximum > size is rather limited. > > U-Boot provides all that is needed to load and execute an EFI binary. So > you can put your efishell as file into the EFI partition like you would > install the EDK2 shell. > > The only hardshift this approach brings is that you have to implement > your own printf because UEFI does not offer formatted output. But this > can be copied from lib/efi_selftest/efi_selftest_console.c. > > The same decision I took for booting from iSCSI. I did not try to put an > iSCSI driver into U-Boot instead I use iPXE as an executable that is > loaded from the EFI partition. > >> >> I have never intended to fully implement a shell which is to be compliant >> with UEFI specification while I'm trying to mimick some command >> interfaces for convenience. UEFI shell, as you know, provides plenty >> of "protocols" on which some UEFI applications, including UEFI SCT, >> reply. I will never implement it with my efishell. >> >> I hope that my efishell is a quick and easy way of learning more about >> u-boot's uefi environment. I will be even happier if more people >> get involved there. >> >>> Due to these considerations I suggest that you build your UEFI shell >>> implementation as a separate UEFI binary (like helloworld.efi). You may >>> offer an embedding of the binary (like the bootefi hello command) into >>> the finally linked U-Boot binary via a configuration variable. Please, >>> put the shell implementation into a separate directory. You may want to >>> designate yourself as maintainer (in file MAINTAINERS). >> >> Yeah, your suggestion is reasonable and I have thought of it before. >> There are, however, several reasons that I haven't done so; particularly, >> efishell is implemented not only with boottime services but also >> other helper functions, say, from device path utilities. Exporting them >> as libraries is possible but I don't think that it would be so valuable. >> >> Even if efishell is a separate application, it will not contribute to >> reduce the total footprint if it is embedded along with u-boot binary. > > That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into > the U-Boot binary - is default no. Same I would do for efishell.efi. One big drawback with a separate binary is the missing command line integration. It becomes quite awkward to execute efi debug commands then, since you'll have to run them through a special bootefi subcommand. If you really want to have a "uefi shell", I think the sanest option is to just provide a built-in copy of the edk2 uefi shell, similar to the hello world binary. The big benefit of this patch set however, is not that we get a shell - it's that we get quick and tiny debug introspectability into efi_loader data structures. I think the biggest problem here really is the name of the code. Why don't we just call it "debugefi"? It would be default N except for debug targets (just like bootefi_hello). That way when someone wants to just quickly introspect internal data structures, they can. I also hope that if the name contains debug, nobody will expect command line compatibility going forward, so we have much more freedom to change internals (which is my biggest concern). So in my opinion, if you fix the 2 other comments from Heinrich and rename everything from "efishell" to "debugefi" (so it aligns with bootefi), we should be good. Alex
On Sun, Dec 23, 2018 at 02:56:40AM +0100, Alexander Graf wrote: > > > On 19.12.18 13:23, Heinrich Schuchardt wrote: > > On 12/19/18 2:49 AM, AKASHI Takahiro wrote: > >> Heinrich, > >> > >> On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote: > >>> On 12/18/18 6:05 AM, AKASHI Takahiro wrote: > >>>> "env [print|set] -e" allows for handling uefi variables without > >>>> knowing details about mapping to corresponding u-boot variables. > >>>> > >>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>> > >>> Hello Takahiro, > >>> > >>> in several patch series you are implementing multiple interactive > >>> commands that concern > >>> > >>> - handling of EFI variables > >>> - executing EFI binaries > >>> - managing boot sequence > >>> > >>> I very much appreciate your effort to provide an independent UEFI shell > >>> implementation. What I am worried about is that your current patches > >>> make it part of the monolithic U-Boot binary. > >> > >> First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's > >> comment on v2. So you can disable efishell command if you don't want it. > >> > >> Are you still worried? > >> > >>> This design has multiple drawbacks: > >>> > >>> The memory size available for U-Boot is very limited for many devices. > >>> We already had to disable EFI_LOADER for some boards due to this > >>> limitations. Hence we want to keep everything out of the U-Boot binary > >>> that does not serve the primary goal of loading and executing the next > >>> binary. > >> > >> I don't know your point here. If EFI_LOADER is disabled, efishell > >> will never be compiled in. > >> > >>> The UEFI forum has published a UEFI Shell specification which is very > >>> extensive. We still have a lot of deficiencies in U-Boot's UEFI API > >>> implementation. By merging in parts of an UEFI shell implementation our > >>> project looses focus. > >> > >> What is "our project?" What is "focus?" > >> I'm just asking as I want to share that information with you. > >> > >>> There is an EDK2 implementation of said > >>> specification. If we fix the remaining bugs of the EFI API > >>> implementation in U-Boot we could simply run the EDK2 shell which > >>> provides all that is needed for interactive work. > >>> > >>> With you monolithic approach your UEFI shell implementation can neither > >>> be used with other UEFI API implementations than U-Boot nor can it be > >>> tested against other API implementations. > >> > >> Let me explain my stance. > >> My efishell is basically something like a pursuit as well as > >> a debug/test tool which was and is still quite useful for me. > >> Without it, I would have completed (most of) my efi-related work so far. > >> So I believe that it will also be useful for other people who may want > >> to get involved and play with u-boot's efi environment. > > > > On SD-Cards U-Boot is installed between the MBR and the first partition. > > On other devices it is put into a very small ROM. Both ways the maximum > > size is rather limited. > > > > U-Boot provides all that is needed to load and execute an EFI binary. So > > you can put your efishell as file into the EFI partition like you would > > install the EDK2 shell. > > > > The only hardshift this approach brings is that you have to implement > > your own printf because UEFI does not offer formatted output. But this > > can be copied from lib/efi_selftest/efi_selftest_console.c. > > > > The same decision I took for booting from iSCSI. I did not try to put an > > iSCSI driver into U-Boot instead I use iPXE as an executable that is > > loaded from the EFI partition. > > > >> > >> I have never intended to fully implement a shell which is to be compliant > >> with UEFI specification while I'm trying to mimick some command > >> interfaces for convenience. UEFI shell, as you know, provides plenty > >> of "protocols" on which some UEFI applications, including UEFI SCT, > >> reply. I will never implement it with my efishell. > >> > >> I hope that my efishell is a quick and easy way of learning more about > >> u-boot's uefi environment. I will be even happier if more people > >> get involved there. > >> > >>> Due to these considerations I suggest that you build your UEFI shell > >>> implementation as a separate UEFI binary (like helloworld.efi). You may > >>> offer an embedding of the binary (like the bootefi hello command) into > >>> the finally linked U-Boot binary via a configuration variable. Please, > >>> put the shell implementation into a separate directory. You may want to > >>> designate yourself as maintainer (in file MAINTAINERS). > >> > >> Yeah, your suggestion is reasonable and I have thought of it before. > >> There are, however, several reasons that I haven't done so; particularly, > >> efishell is implemented not only with boottime services but also > >> other helper functions, say, from device path utilities. Exporting them > >> as libraries is possible but I don't think that it would be so valuable. > >> > >> Even if efishell is a separate application, it will not contribute to > >> reduce the total footprint if it is embedded along with u-boot binary. > > > > That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into > > the U-Boot binary - is default no. Same I would do for efishell.efi. > > One big drawback with a separate binary is the missing command line > integration. It becomes quite awkward to execute efi debug commands > then, since you'll have to run them through a special bootefi subcommand. > > If you really want to have a "uefi shell", I think the sanest option is > to just provide a built-in copy of the edk2 uefi shell, similar to the > hello world binary. The big benefit of this patch set however, is not > that we get a shell - it's that we get quick and tiny debug > introspectability into efi_loader data structures. And my command can be used for simple testing. > I think the biggest problem here really is the name of the code. Why > don't we just call it "debugefi"? It would be default N except for debug > targets (just like bootefi_hello). > > That way when someone wants to just quickly introspect internal data > structures, they can. I also hope that if the name contains debug, > nobody will expect command line compatibility going forward, so we have > much more freedom to change internals (which is my biggest concern). > > So in my opinion, if you fix the 2 other comments from Heinrich and > rename everything from "efishell" to "debugefi" (so it aligns with > bootefi), we should be good. If Heinrich agrees, I will fix the name although I'm not a super fan of this new name :) Thanks, -Takahiro Akashi > > Alex
On 25.12.18 09:44, AKASHI Takahiro wrote: > On Sun, Dec 23, 2018 at 02:56:40AM +0100, Alexander Graf wrote: >> >> >> On 19.12.18 13:23, Heinrich Schuchardt wrote: >>> On 12/19/18 2:49 AM, AKASHI Takahiro wrote: >>>> Heinrich, >>>> >>>> On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote: >>>>> On 12/18/18 6:05 AM, AKASHI Takahiro wrote: >>>>>> "env [print|set] -e" allows for handling uefi variables without >>>>>> knowing details about mapping to corresponding u-boot variables. >>>>>> >>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>>> >>>>> Hello Takahiro, >>>>> >>>>> in several patch series you are implementing multiple interactive >>>>> commands that concern >>>>> >>>>> - handling of EFI variables >>>>> - executing EFI binaries >>>>> - managing boot sequence >>>>> >>>>> I very much appreciate your effort to provide an independent UEFI shell >>>>> implementation. What I am worried about is that your current patches >>>>> make it part of the monolithic U-Boot binary. >>>> >>>> First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's >>>> comment on v2. So you can disable efishell command if you don't want it. >>>> >>>> Are you still worried? >>>> >>>>> This design has multiple drawbacks: >>>>> >>>>> The memory size available for U-Boot is very limited for many devices. >>>>> We already had to disable EFI_LOADER for some boards due to this >>>>> limitations. Hence we want to keep everything out of the U-Boot binary >>>>> that does not serve the primary goal of loading and executing the next >>>>> binary. >>>> >>>> I don't know your point here. If EFI_LOADER is disabled, efishell >>>> will never be compiled in. >>>> >>>>> The UEFI forum has published a UEFI Shell specification which is very >>>>> extensive. We still have a lot of deficiencies in U-Boot's UEFI API >>>>> implementation. By merging in parts of an UEFI shell implementation our >>>>> project looses focus. >>>> >>>> What is "our project?" What is "focus?" >>>> I'm just asking as I want to share that information with you. >>>> >>>>> There is an EDK2 implementation of said >>>>> specification. If we fix the remaining bugs of the EFI API >>>>> implementation in U-Boot we could simply run the EDK2 shell which >>>>> provides all that is needed for interactive work. >>>>> >>>>> With you monolithic approach your UEFI shell implementation can neither >>>>> be used with other UEFI API implementations than U-Boot nor can it be >>>>> tested against other API implementations. >>>> >>>> Let me explain my stance. >>>> My efishell is basically something like a pursuit as well as >>>> a debug/test tool which was and is still quite useful for me. >>>> Without it, I would have completed (most of) my efi-related work so far. >>>> So I believe that it will also be useful for other people who may want >>>> to get involved and play with u-boot's efi environment. >>> >>> On SD-Cards U-Boot is installed between the MBR and the first partition. >>> On other devices it is put into a very small ROM. Both ways the maximum >>> size is rather limited. >>> >>> U-Boot provides all that is needed to load and execute an EFI binary. So >>> you can put your efishell as file into the EFI partition like you would >>> install the EDK2 shell. >>> >>> The only hardshift this approach brings is that you have to implement >>> your own printf because UEFI does not offer formatted output. But this >>> can be copied from lib/efi_selftest/efi_selftest_console.c. >>> >>> The same decision I took for booting from iSCSI. I did not try to put an >>> iSCSI driver into U-Boot instead I use iPXE as an executable that is >>> loaded from the EFI partition. >>> >>>> >>>> I have never intended to fully implement a shell which is to be compliant >>>> with UEFI specification while I'm trying to mimick some command >>>> interfaces for convenience. UEFI shell, as you know, provides plenty >>>> of "protocols" on which some UEFI applications, including UEFI SCT, >>>> reply. I will never implement it with my efishell. >>>> >>>> I hope that my efishell is a quick and easy way of learning more about >>>> u-boot's uefi environment. I will be even happier if more people >>>> get involved there. >>>> >>>>> Due to these considerations I suggest that you build your UEFI shell >>>>> implementation as a separate UEFI binary (like helloworld.efi). You may >>>>> offer an embedding of the binary (like the bootefi hello command) into >>>>> the finally linked U-Boot binary via a configuration variable. Please, >>>>> put the shell implementation into a separate directory. You may want to >>>>> designate yourself as maintainer (in file MAINTAINERS). >>>> >>>> Yeah, your suggestion is reasonable and I have thought of it before. >>>> There are, however, several reasons that I haven't done so; particularly, >>>> efishell is implemented not only with boottime services but also >>>> other helper functions, say, from device path utilities. Exporting them >>>> as libraries is possible but I don't think that it would be so valuable. >>>> >>>> Even if efishell is a separate application, it will not contribute to >>>> reduce the total footprint if it is embedded along with u-boot binary. >>> >>> That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into >>> the U-Boot binary - is default no. Same I would do for efishell.efi. >> >> One big drawback with a separate binary is the missing command line >> integration. It becomes quite awkward to execute efi debug commands >> then, since you'll have to run them through a special bootefi subcommand. >> >> If you really want to have a "uefi shell", I think the sanest option is >> to just provide a built-in copy of the edk2 uefi shell, similar to the >> hello world binary. The big benefit of this patch set however, is not >> that we get a shell - it's that we get quick and tiny debug >> introspectability into efi_loader data structures. > > And my command can be used for simple testing. Exactly, that would give us the best of both worlds. > >> I think the biggest problem here really is the name of the code. Why >> don't we just call it "debugefi"? It would be default N except for debug >> targets (just like bootefi_hello). >> >> That way when someone wants to just quickly introspect internal data >> structures, they can. I also hope that if the name contains debug, >> nobody will expect command line compatibility going forward, so we have >> much more freedom to change internals (which is my biggest concern). >> >> So in my opinion, if you fix the 2 other comments from Heinrich and >> rename everything from "efishell" to "debugefi" (so it aligns with >> bootefi), we should be good. > > If Heinrich agrees, I will fix the name although I'm not a super fan > of this new name :) Well, feel free to come up with a new one, but it definitely must have a ring to it that it's a tiny, debug only feature that is not intended for normal use ;). For normal operation, we need to come up with mechanisms that integrate much deeper into U-Boot's generic command structure. Alex
Heinrich, On Wed, Dec 26, 2018 at 10:20:32PM +0100, Alexander Graf wrote: > > > On 25.12.18 09:44, AKASHI Takahiro wrote: > > On Sun, Dec 23, 2018 at 02:56:40AM +0100, Alexander Graf wrote: > >> > >> > >> On 19.12.18 13:23, Heinrich Schuchardt wrote: > >>> On 12/19/18 2:49 AM, AKASHI Takahiro wrote: > >>>> Heinrich, > >>>> > >>>> On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote: > >>>>> On 12/18/18 6:05 AM, AKASHI Takahiro wrote: > >>>>>> "env [print|set] -e" allows for handling uefi variables without > >>>>>> knowing details about mapping to corresponding u-boot variables. > >>>>>> > >>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>>>> > >>>>> Hello Takahiro, > >>>>> > >>>>> in several patch series you are implementing multiple interactive > >>>>> commands that concern > >>>>> > >>>>> - handling of EFI variables > >>>>> - executing EFI binaries > >>>>> - managing boot sequence > >>>>> > >>>>> I very much appreciate your effort to provide an independent UEFI shell > >>>>> implementation. What I am worried about is that your current patches > >>>>> make it part of the monolithic U-Boot binary. > >>>> > >>>> First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's > >>>> comment on v2. So you can disable efishell command if you don't want it. > >>>> > >>>> Are you still worried? > >>>> > >>>>> This design has multiple drawbacks: > >>>>> > >>>>> The memory size available for U-Boot is very limited for many devices. > >>>>> We already had to disable EFI_LOADER for some boards due to this > >>>>> limitations. Hence we want to keep everything out of the U-Boot binary > >>>>> that does not serve the primary goal of loading and executing the next > >>>>> binary. > >>>> > >>>> I don't know your point here. If EFI_LOADER is disabled, efishell > >>>> will never be compiled in. > >>>> > >>>>> The UEFI forum has published a UEFI Shell specification which is very > >>>>> extensive. We still have a lot of deficiencies in U-Boot's UEFI API > >>>>> implementation. By merging in parts of an UEFI shell implementation our > >>>>> project looses focus. > >>>> > >>>> What is "our project?" What is "focus?" > >>>> I'm just asking as I want to share that information with you. > >>>> > >>>>> There is an EDK2 implementation of said > >>>>> specification. If we fix the remaining bugs of the EFI API > >>>>> implementation in U-Boot we could simply run the EDK2 shell which > >>>>> provides all that is needed for interactive work. > >>>>> > >>>>> With you monolithic approach your UEFI shell implementation can neither > >>>>> be used with other UEFI API implementations than U-Boot nor can it be > >>>>> tested against other API implementations. > >>>> > >>>> Let me explain my stance. > >>>> My efishell is basically something like a pursuit as well as > >>>> a debug/test tool which was and is still quite useful for me. > >>>> Without it, I would have completed (most of) my efi-related work so far. > >>>> So I believe that it will also be useful for other people who may want > >>>> to get involved and play with u-boot's efi environment. > >>> > >>> On SD-Cards U-Boot is installed between the MBR and the first partition. > >>> On other devices it is put into a very small ROM. Both ways the maximum > >>> size is rather limited. > >>> > >>> U-Boot provides all that is needed to load and execute an EFI binary. So > >>> you can put your efishell as file into the EFI partition like you would > >>> install the EDK2 shell. > >>> > >>> The only hardshift this approach brings is that you have to implement > >>> your own printf because UEFI does not offer formatted output. But this > >>> can be copied from lib/efi_selftest/efi_selftest_console.c. > >>> > >>> The same decision I took for booting from iSCSI. I did not try to put an > >>> iSCSI driver into U-Boot instead I use iPXE as an executable that is > >>> loaded from the EFI partition. > >>> > >>>> > >>>> I have never intended to fully implement a shell which is to be compliant > >>>> with UEFI specification while I'm trying to mimick some command > >>>> interfaces for convenience. UEFI shell, as you know, provides plenty > >>>> of "protocols" on which some UEFI applications, including UEFI SCT, > >>>> reply. I will never implement it with my efishell. > >>>> > >>>> I hope that my efishell is a quick and easy way of learning more about > >>>> u-boot's uefi environment. I will be even happier if more people > >>>> get involved there. > >>>> > >>>>> Due to these considerations I suggest that you build your UEFI shell > >>>>> implementation as a separate UEFI binary (like helloworld.efi). You may > >>>>> offer an embedding of the binary (like the bootefi hello command) into > >>>>> the finally linked U-Boot binary via a configuration variable. Please, > >>>>> put the shell implementation into a separate directory. You may want to > >>>>> designate yourself as maintainer (in file MAINTAINERS). > >>>> > >>>> Yeah, your suggestion is reasonable and I have thought of it before. > >>>> There are, however, several reasons that I haven't done so; particularly, > >>>> efishell is implemented not only with boottime services but also > >>>> other helper functions, say, from device path utilities. Exporting them > >>>> as libraries is possible but I don't think that it would be so valuable. > >>>> > >>>> Even if efishell is a separate application, it will not contribute to > >>>> reduce the total footprint if it is embedded along with u-boot binary. > >>> > >>> That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into > >>> the U-Boot binary - is default no. Same I would do for efishell.efi. > >> > >> One big drawback with a separate binary is the missing command line > >> integration. It becomes quite awkward to execute efi debug commands > >> then, since you'll have to run them through a special bootefi subcommand. > >> > >> If you really want to have a "uefi shell", I think the sanest option is > >> to just provide a built-in copy of the edk2 uefi shell, similar to the > >> hello world binary. The big benefit of this patch set however, is not > >> that we get a shell - it's that we get quick and tiny debug > >> introspectability into efi_loader data structures. > > > > And my command can be used for simple testing. > > Exactly, that would give us the best of both worlds. > > > > >> I think the biggest problem here really is the name of the code. Why > >> don't we just call it "debugefi"? It would be default N except for debug > >> targets (just like bootefi_hello). > >> > >> That way when someone wants to just quickly introspect internal data > >> structures, they can. I also hope that if the name contains debug, > >> nobody will expect command line compatibility going forward, so we have > >> much more freedom to change internals (which is my biggest concern). > >> > >> So in my opinion, if you fix the 2 other comments from Heinrich and > >> rename everything from "efishell" to "debugefi" (so it aligns with > >> bootefi), we should be good. > > > > If Heinrich agrees, I will fix the name although I'm not a super fan > > of this new name :) > > Well, feel free to come up with a new one, but it definitely must have a > ring to it that it's a tiny, debug only feature that is not intended for > normal use ;). Do you have any idea/preference about this command's name? -Takahiro Akashi > For normal operation, we need to come up with mechanisms that integrate > much deeper into U-Boot's generic command structure. > > > Alex
On Mon, Jan 07, 2019 at 04:47:13PM +0900, AKASHI Takahiro wrote: > Heinrich, > > On Wed, Dec 26, 2018 at 10:20:32PM +0100, Alexander Graf wrote: > > > > > > On 25.12.18 09:44, AKASHI Takahiro wrote: > > > On Sun, Dec 23, 2018 at 02:56:40AM +0100, Alexander Graf wrote: > > >> > > >> > > >> On 19.12.18 13:23, Heinrich Schuchardt wrote: > > >>> On 12/19/18 2:49 AM, AKASHI Takahiro wrote: > > >>>> Heinrich, > > >>>> > > >>>> On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote: > > >>>>> On 12/18/18 6:05 AM, AKASHI Takahiro wrote: > > >>>>>> "env [print|set] -e" allows for handling uefi variables without > > >>>>>> knowing details about mapping to corresponding u-boot variables. > > >>>>>> > > >>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > >>>>> > > >>>>> Hello Takahiro, > > >>>>> > > >>>>> in several patch series you are implementing multiple interactive > > >>>>> commands that concern > > >>>>> > > >>>>> - handling of EFI variables > > >>>>> - executing EFI binaries > > >>>>> - managing boot sequence > > >>>>> > > >>>>> I very much appreciate your effort to provide an independent UEFI shell > > >>>>> implementation. What I am worried about is that your current patches > > >>>>> make it part of the monolithic U-Boot binary. > > >>>> > > >>>> First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's > > >>>> comment on v2. So you can disable efishell command if you don't want it. > > >>>> > > >>>> Are you still worried? > > >>>> > > >>>>> This design has multiple drawbacks: > > >>>>> > > >>>>> The memory size available for U-Boot is very limited for many devices. > > >>>>> We already had to disable EFI_LOADER for some boards due to this > > >>>>> limitations. Hence we want to keep everything out of the U-Boot binary > > >>>>> that does not serve the primary goal of loading and executing the next > > >>>>> binary. > > >>>> > > >>>> I don't know your point here. If EFI_LOADER is disabled, efishell > > >>>> will never be compiled in. > > >>>> > > >>>>> The UEFI forum has published a UEFI Shell specification which is very > > >>>>> extensive. We still have a lot of deficiencies in U-Boot's UEFI API > > >>>>> implementation. By merging in parts of an UEFI shell implementation our > > >>>>> project looses focus. > > >>>> > > >>>> What is "our project?" What is "focus?" > > >>>> I'm just asking as I want to share that information with you. > > >>>> > > >>>>> There is an EDK2 implementation of said > > >>>>> specification. If we fix the remaining bugs of the EFI API > > >>>>> implementation in U-Boot we could simply run the EDK2 shell which > > >>>>> provides all that is needed for interactive work. > > >>>>> > > >>>>> With you monolithic approach your UEFI shell implementation can neither > > >>>>> be used with other UEFI API implementations than U-Boot nor can it be > > >>>>> tested against other API implementations. > > >>>> > > >>>> Let me explain my stance. > > >>>> My efishell is basically something like a pursuit as well as > > >>>> a debug/test tool which was and is still quite useful for me. > > >>>> Without it, I would have completed (most of) my efi-related work so far. > > >>>> So I believe that it will also be useful for other people who may want > > >>>> to get involved and play with u-boot's efi environment. > > >>> > > >>> On SD-Cards U-Boot is installed between the MBR and the first partition. > > >>> On other devices it is put into a very small ROM. Both ways the maximum > > >>> size is rather limited. > > >>> > > >>> U-Boot provides all that is needed to load and execute an EFI binary. So > > >>> you can put your efishell as file into the EFI partition like you would > > >>> install the EDK2 shell. > > >>> > > >>> The only hardshift this approach brings is that you have to implement > > >>> your own printf because UEFI does not offer formatted output. But this > > >>> can be copied from lib/efi_selftest/efi_selftest_console.c. > > >>> > > >>> The same decision I took for booting from iSCSI. I did not try to put an > > >>> iSCSI driver into U-Boot instead I use iPXE as an executable that is > > >>> loaded from the EFI partition. > > >>> > > >>>> > > >>>> I have never intended to fully implement a shell which is to be compliant > > >>>> with UEFI specification while I'm trying to mimick some command > > >>>> interfaces for convenience. UEFI shell, as you know, provides plenty > > >>>> of "protocols" on which some UEFI applications, including UEFI SCT, > > >>>> reply. I will never implement it with my efishell. > > >>>> > > >>>> I hope that my efishell is a quick and easy way of learning more about > > >>>> u-boot's uefi environment. I will be even happier if more people > > >>>> get involved there. > > >>>> > > >>>>> Due to these considerations I suggest that you build your UEFI shell > > >>>>> implementation as a separate UEFI binary (like helloworld.efi). You may > > >>>>> offer an embedding of the binary (like the bootefi hello command) into > > >>>>> the finally linked U-Boot binary via a configuration variable. Please, > > >>>>> put the shell implementation into a separate directory. You may want to > > >>>>> designate yourself as maintainer (in file MAINTAINERS). > > >>>> > > >>>> Yeah, your suggestion is reasonable and I have thought of it before. > > >>>> There are, however, several reasons that I haven't done so; particularly, > > >>>> efishell is implemented not only with boottime services but also > > >>>> other helper functions, say, from device path utilities. Exporting them > > >>>> as libraries is possible but I don't think that it would be so valuable. > > >>>> > > >>>> Even if efishell is a separate application, it will not contribute to > > >>>> reduce the total footprint if it is embedded along with u-boot binary. > > >>> > > >>> That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into > > >>> the U-Boot binary - is default no. Same I would do for efishell.efi. > > >> > > >> One big drawback with a separate binary is the missing command line > > >> integration. It becomes quite awkward to execute efi debug commands > > >> then, since you'll have to run them through a special bootefi subcommand. > > >> > > >> If you really want to have a "uefi shell", I think the sanest option is > > >> to just provide a built-in copy of the edk2 uefi shell, similar to the > > >> hello world binary. The big benefit of this patch set however, is not > > >> that we get a shell - it's that we get quick and tiny debug > > >> introspectability into efi_loader data structures. > > > > > > And my command can be used for simple testing. > > > > Exactly, that would give us the best of both worlds. > > > > > > > >> I think the biggest problem here really is the name of the code. Why > > >> don't we just call it "debugefi"? It would be default N except for debug > > >> targets (just like bootefi_hello). > > >> > > >> That way when someone wants to just quickly introspect internal data > > >> structures, they can. I also hope that if the name contains debug, > > >> nobody will expect command line compatibility going forward, so we have > > >> much more freedom to change internals (which is my biggest concern). > > >> > > >> So in my opinion, if you fix the 2 other comments from Heinrich and > > >> rename everything from "efishell" to "debugefi" (so it aligns with > > >> bootefi), we should be good. > > > > > > If Heinrich agrees, I will fix the name although I'm not a super fan > > > of this new name :) > > > > Well, feel free to come up with a new one, but it definitely must have a > > ring to it that it's a tiny, debug only feature that is not intended for > > normal use ;). > > Do you have any idea/preference about this command's name? I prefer efidebug/efidbg or efitool so that we can use a shorthand name, efi, at command line in most cases. -Takahiro Akashi > -Takahiro Akashi > > > For normal operation, we need to come up with mechanisms that integrate > > much deeper into U-Boot's generic command structure. > > > > > > Alex
On 08.01.19 08:29, AKASHI Takahiro wrote: > On Mon, Jan 07, 2019 at 04:47:13PM +0900, AKASHI Takahiro wrote: >> Heinrich, >> >> On Wed, Dec 26, 2018 at 10:20:32PM +0100, Alexander Graf wrote: >>> >>> >>> On 25.12.18 09:44, AKASHI Takahiro wrote: >>>> On Sun, Dec 23, 2018 at 02:56:40AM +0100, Alexander Graf wrote: >>>>> >>>>> >>>>> On 19.12.18 13:23, Heinrich Schuchardt wrote: >>>>>> On 12/19/18 2:49 AM, AKASHI Takahiro wrote: >>>>>>> Heinrich, >>>>>>> >>>>>>> On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote: >>>>>>>> On 12/18/18 6:05 AM, AKASHI Takahiro wrote: >>>>>>>>> "env [print|set] -e" allows for handling uefi variables without >>>>>>>>> knowing details about mapping to corresponding u-boot variables. >>>>>>>>> >>>>>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>>>>>> >>>>>>>> Hello Takahiro, >>>>>>>> >>>>>>>> in several patch series you are implementing multiple interactive >>>>>>>> commands that concern >>>>>>>> >>>>>>>> - handling of EFI variables >>>>>>>> - executing EFI binaries >>>>>>>> - managing boot sequence >>>>>>>> >>>>>>>> I very much appreciate your effort to provide an independent UEFI shell >>>>>>>> implementation. What I am worried about is that your current patches >>>>>>>> make it part of the monolithic U-Boot binary. >>>>>>> >>>>>>> First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's >>>>>>> comment on v2. So you can disable efishell command if you don't want it. >>>>>>> >>>>>>> Are you still worried? >>>>>>> >>>>>>>> This design has multiple drawbacks: >>>>>>>> >>>>>>>> The memory size available for U-Boot is very limited for many devices. >>>>>>>> We already had to disable EFI_LOADER for some boards due to this >>>>>>>> limitations. Hence we want to keep everything out of the U-Boot binary >>>>>>>> that does not serve the primary goal of loading and executing the next >>>>>>>> binary. >>>>>>> >>>>>>> I don't know your point here. If EFI_LOADER is disabled, efishell >>>>>>> will never be compiled in. >>>>>>> >>>>>>>> The UEFI forum has published a UEFI Shell specification which is very >>>>>>>> extensive. We still have a lot of deficiencies in U-Boot's UEFI API >>>>>>>> implementation. By merging in parts of an UEFI shell implementation our >>>>>>>> project looses focus. >>>>>>> >>>>>>> What is "our project?" What is "focus?" >>>>>>> I'm just asking as I want to share that information with you. >>>>>>> >>>>>>>> There is an EDK2 implementation of said >>>>>>>> specification. If we fix the remaining bugs of the EFI API >>>>>>>> implementation in U-Boot we could simply run the EDK2 shell which >>>>>>>> provides all that is needed for interactive work. >>>>>>>> >>>>>>>> With you monolithic approach your UEFI shell implementation can neither >>>>>>>> be used with other UEFI API implementations than U-Boot nor can it be >>>>>>>> tested against other API implementations. >>>>>>> >>>>>>> Let me explain my stance. >>>>>>> My efishell is basically something like a pursuit as well as >>>>>>> a debug/test tool which was and is still quite useful for me. >>>>>>> Without it, I would have completed (most of) my efi-related work so far. >>>>>>> So I believe that it will also be useful for other people who may want >>>>>>> to get involved and play with u-boot's efi environment. >>>>>> >>>>>> On SD-Cards U-Boot is installed between the MBR and the first partition. >>>>>> On other devices it is put into a very small ROM. Both ways the maximum >>>>>> size is rather limited. >>>>>> >>>>>> U-Boot provides all that is needed to load and execute an EFI binary. So >>>>>> you can put your efishell as file into the EFI partition like you would >>>>>> install the EDK2 shell. >>>>>> >>>>>> The only hardshift this approach brings is that you have to implement >>>>>> your own printf because UEFI does not offer formatted output. But this >>>>>> can be copied from lib/efi_selftest/efi_selftest_console.c. >>>>>> >>>>>> The same decision I took for booting from iSCSI. I did not try to put an >>>>>> iSCSI driver into U-Boot instead I use iPXE as an executable that is >>>>>> loaded from the EFI partition. >>>>>> >>>>>>> >>>>>>> I have never intended to fully implement a shell which is to be compliant >>>>>>> with UEFI specification while I'm trying to mimick some command >>>>>>> interfaces for convenience. UEFI shell, as you know, provides plenty >>>>>>> of "protocols" on which some UEFI applications, including UEFI SCT, >>>>>>> reply. I will never implement it with my efishell. >>>>>>> >>>>>>> I hope that my efishell is a quick and easy way of learning more about >>>>>>> u-boot's uefi environment. I will be even happier if more people >>>>>>> get involved there. >>>>>>> >>>>>>>> Due to these considerations I suggest that you build your UEFI shell >>>>>>>> implementation as a separate UEFI binary (like helloworld.efi). You may >>>>>>>> offer an embedding of the binary (like the bootefi hello command) into >>>>>>>> the finally linked U-Boot binary via a configuration variable. Please, >>>>>>>> put the shell implementation into a separate directory. You may want to >>>>>>>> designate yourself as maintainer (in file MAINTAINERS). >>>>>>> >>>>>>> Yeah, your suggestion is reasonable and I have thought of it before. >>>>>>> There are, however, several reasons that I haven't done so; particularly, >>>>>>> efishell is implemented not only with boottime services but also >>>>>>> other helper functions, say, from device path utilities. Exporting them >>>>>>> as libraries is possible but I don't think that it would be so valuable. >>>>>>> >>>>>>> Even if efishell is a separate application, it will not contribute to >>>>>>> reduce the total footprint if it is embedded along with u-boot binary. >>>>>> >>>>>> That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into >>>>>> the U-Boot binary - is default no. Same I would do for efishell.efi. >>>>> >>>>> One big drawback with a separate binary is the missing command line >>>>> integration. It becomes quite awkward to execute efi debug commands >>>>> then, since you'll have to run them through a special bootefi subcommand. >>>>> >>>>> If you really want to have a "uefi shell", I think the sanest option is >>>>> to just provide a built-in copy of the edk2 uefi shell, similar to the >>>>> hello world binary. The big benefit of this patch set however, is not >>>>> that we get a shell - it's that we get quick and tiny debug >>>>> introspectability into efi_loader data structures. >>>> >>>> And my command can be used for simple testing. >>> >>> Exactly, that would give us the best of both worlds. >>> >>>> >>>>> I think the biggest problem here really is the name of the code. Why >>>>> don't we just call it "debugefi"? It would be default N except for debug >>>>> targets (just like bootefi_hello). >>>>> >>>>> That way when someone wants to just quickly introspect internal data >>>>> structures, they can. I also hope that if the name contains debug, >>>>> nobody will expect command line compatibility going forward, so we have >>>>> much more freedom to change internals (which is my biggest concern). >>>>> >>>>> So in my opinion, if you fix the 2 other comments from Heinrich and >>>>> rename everything from "efishell" to "debugefi" (so it aligns with >>>>> bootefi), we should be good. >>>> >>>> If Heinrich agrees, I will fix the name although I'm not a super fan >>>> of this new name :) >>> >>> Well, feel free to come up with a new one, but it definitely must have a >>> ring to it that it's a tiny, debug only feature that is not intended for >>> normal use ;). >> >> Do you have any idea/preference about this command's name? > > I prefer efidebug/efidbg or efitool so that we can use a shorthand > name, efi, at command line in most cases. That definitely works for me as well, yes. Alex
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index c0facabfc4fe..8168c963ac9d 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -27,6 +27,8 @@ #include <cli.h> #include <command.h> #include <console.h> +#include <efi.h> +#include <efi_loader.h> #include <environment.h> #include <search.h> #include <errno.h> @@ -119,6 +121,25 @@ static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc, int rcode = 0; int env_flag = H_HIDE_DOT; +#if defined(CONFIG_CMD_EFISHELL) + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') { + efi_status_t r; + + argc--; + argv++; + + /* Initialize EFI drivers */ + r = efi_init_obj_list(); + if (r != EFI_SUCCESS) { + printf("Error: Cannot set up EFI drivers, r = %lu\n", + r & ~EFI_ERROR_MASK); + return CMD_RET_FAILURE; + } + + return do_efi_dump_var(argc, argv); + } +#endif + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'a') { argc--; argv++; @@ -216,6 +237,26 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag) ENTRY e, *ep; debug("Initial value for argc=%d\n", argc); + +#if defined(CONFIG_CMD_EFISHELL) + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') { + efi_status_t r; + + argc--; + argv++; + + /* Initialize EFI drivers */ + r = efi_init_obj_list(); + if (r != EFI_SUCCESS) { + printf("Error: Cannot set up EFI drivers, r = %lu\n", + r & ~EFI_ERROR_MASK); + return CMD_RET_FAILURE; + } + + return do_efi_set_var(argc, argv); + } +#endif + while (argc > 1 && **(argv + 1) == '-') { char *arg = *++argv; @@ -1262,15 +1303,23 @@ static char env_help_text[] = #if defined(CONFIG_CMD_IMPORTENV) "env import [-d] [-t [-r] | -b | -c] addr [size] [var ...] - import environment\n" #endif +#if defined(CONFIG_CMD_EFISHELL) + "env print [-a | -e [name] | name ...] - print environment\n" +#else "env print [-a | name ...] - print environment\n" +#endif #if defined(CONFIG_CMD_RUN) "env run var [...] - run commands in an environment variable\n" #endif #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE) "env save - save environment\n" #endif +#if defined(CONFIG_CMD_EFISHELL) + "env set [-e | -f] name [arg ...]\n"; +#else "env set [-f] name [arg ...]\n"; #endif +#endif U_BOOT_CMD( env, CONFIG_SYS_MAXARGS, 1, do_env, @@ -1295,6 +1344,10 @@ U_BOOT_CMD_COMPLETE( printenv, CONFIG_SYS_MAXARGS, 1, do_env_print, "print environment variables", "[-a]\n - print [all] values of all environment variables\n" +#if defined(CONFIG_CMD_EFISHELL) + "printenv -e [<name>]\n" + " - print UEFI variable 'name' or all the variables\n" +#endif "printenv name ...\n" " - print value of environment variable 'name'", var_complete @@ -1322,7 +1375,11 @@ U_BOOT_CMD_COMPLETE( U_BOOT_CMD_COMPLETE( setenv, CONFIG_SYS_MAXARGS, 0, do_env_set, "set environment variables", - "[-f] name value ...\n" +#if defined(CONFIG_CMD_EFISHELL) + "-e <name> [<value>]\n" + " - set UEFI variable 'name' to 'value' ...'\n" +#endif + "setenv [-f] name value ...\n" " - [forcibly] set environment variable 'name' to 'value ...'\n" "setenv [-f] name\n" " - [forcibly] delete environment variable 'name'", @@ -1343,7 +1400,7 @@ U_BOOT_CMD( U_BOOT_CMD_COMPLETE( run, CONFIG_SYS_MAXARGS, 1, do_run, "run commands in an environment variable", -#if defined(CONFIG_CMD_BOOTEFI) +#if defined(CONFIG_CMD_EFISHELL) "var -e [BootXXXX]\n" " - load and run UEFI app based on 'BootXXXX' UEFI variable", #else
"env [print|set] -e" allows for handling uefi variables without knowing details about mapping to corresponding u-boot variables. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- cmd/nvedit.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 2 deletions(-)