| Message ID | 20191004012033.6216-1-takahiro.akashi@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | [U-Boot,v3] cmd: env: extend "env [set|print] -e" to manage UEFI variables | expand |
On 10/4/19 3:20 AM, AKASHI Takahiro wrote: > With this patch, when setting UEFI variable with "env set -e" command, > we will be able to > - specify vendor guid with "-guid guid", > - specify variable attributes, BOOTSERVICE_ACCESS, RUNTIME_ACCESS, > respectively with "-bs" and "-rt", > - append a value instead of overwriting with "-a", > - use memory as variable's value instead of explicit values given > at the command line with "-i address,size" > > If guid is not explicitly given, default value will be used. > > When "-at" is given, a variable should be authenticated with > appropriate signature database before setting or modifying its value. > (Authentication is not supported yet though.) Didn't you remove this in v2? > > Meanwhile, "env print -e," will be modified so that it will dump > a variable's value only if '-v' (verbose) is specified. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> This does not work as expected: => setenv -e -guid 0123456789abcdef0123456789abcdef -bs ARGONAUT hello world => printenv -e OsIndicationsSupported: EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x8 PlatformLang: EFI_GLOBAL_VARIABLE_GUID: NV|BS|RT, DataSize = 0x6 PlatformLangCodes: EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x6 RuntimeServicesSupported: EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x2 Neither do I see an error nor is the variable set. The patch increases the size of u-boot.bin by 2216 bytes for qemu-arm64. Tom is always asking me how we can stop the size increase in the UEFI sub-system. Why do we need this patch? This functionality is already available via the UEFI shell. Just open the UEFI shell and run Shell> setvar ARGONAUT -guid 01234567-89ab-cdef-0123-456789abcdef -rt =L"Hello world" Shell> exit Best regards Heinrich > --- > Changes in v3 (Oct 4, 2019) > * add verbose messages when SetVariable() fails > > Changes in v2 (Sept 6, 2019) > * remove "-at" option > > --- > cmd/nvedit.c | 19 +++-- > cmd/nvedit_efi.c | 212 +++++++++++++++++++++++++++++++++++++++-------- > 2 files changed, 190 insertions(+), 41 deletions(-) > > diff --git a/cmd/nvedit.c b/cmd/nvedit.c > index 1cb0bc1460b9..1e542972db30 100644 > --- a/cmd/nvedit.c > +++ b/cmd/nvedit.c > @@ -1387,7 +1387,7 @@ static char env_help_text[] = > #endif > "env print [-a | name ...] - print environment\n" > #if defined(CONFIG_CMD_NVEDIT_EFI) > - "env print -e [name ...] - print UEFI environment\n" > + "env print -e [-v] [name ...] - print UEFI environment\n" > #endif > #if defined(CONFIG_CMD_RUN) > "env run var [...] - run commands in an environment variable\n" > @@ -1399,7 +1399,8 @@ static char env_help_text[] = > #endif > #endif > #if defined(CONFIG_CMD_NVEDIT_EFI) > - "env set -e name [arg ...] - set UEFI variable; unset if 'arg' not specified\n" > + "env set -e [-nv][-bs][-rt][-a][-i addr,size][-v] name [arg ...]\n" > + " - set UEFI variable; unset if '-i' or 'arg' not specified\n" > #endif > "env set [-f] name [arg ...]\n"; > #endif > @@ -1428,8 +1429,9 @@ U_BOOT_CMD_COMPLETE( > "print environment variables", > "[-a]\n - print [all] values of all environment variables\n" > #if defined(CONFIG_CMD_NVEDIT_EFI) > - "printenv -e [name ...]\n" > + "printenv -e [-v] [name ...]\n" > " - print UEFI variable 'name' or all the variables\n" > + " \"-v\": verbose for signature database\n" > #endif > "printenv name ...\n" > " - print value of environment variable 'name'", > @@ -1459,9 +1461,16 @@ U_BOOT_CMD_COMPLETE( > setenv, CONFIG_SYS_MAXARGS, 0, do_env_set, > "set environment variables", > #if defined(CONFIG_CMD_NVEDIT_EFI) > - "-e [-nv] name [value ...]\n" > + "-e [-guid guid][-nv][-bs][-rt][-a][-v]\n" > + " [-i addr,size name], or [name [value ...]]\n" > " - set UEFI variable 'name' to 'value' ...'\n" > - " 'nv' option makes the variable non-volatile\n" > + " \"-guid\": set vendor guid\n" > + " \"-nv\": set non-volatile attribute\n" > + " \"-bs\": set boot-service attribute\n" > + " \"-rt\": set runtime attribute\n" > + " \"-a\": append-write\n" > + " \"-i addr,size\": use <addr,size> as variable's value\n" > + " \"-v\": verbose print\n" > " - delete UEFI variable 'name' if 'value' not specified\n" > #endif > "setenv [-f] name value ...\n" > diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c > index ed6d09a53046..795c95be0ef7 100644 > --- a/cmd/nvedit_efi.c > +++ b/cmd/nvedit_efi.c > @@ -13,6 +13,7 @@ > #include <exports.h> > #include <hexdump.h> > #include <malloc.h> > +#include <mapmem.h> > #include <linux/kernel.h> > > /* > @@ -34,15 +35,48 @@ static const struct { > {EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, "AT"}, > }; > > +static const struct { > + efi_guid_t guid; > + char *text; > +} efi_guid_text[] = { > + /* signature database */ > + {EFI_GLOBAL_VARIABLE_GUID, "EFI_GLOBAL_VARIABLE_GUID"}, > +}; > + > +/* "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" */ > +static char unknown_guid[37]; > + > +/** > + * efi_guid_to_str() - convert guid to readable name > + * > + * @guid: GUID > + * Return: string for GUID > + * > + * convert guid to readable name > + */ > +static const char *efi_guid_to_str(efi_guid_t *guid) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(efi_guid_text); i++) > + if (!guidcmp(guid, &efi_guid_text[i].guid)) > + return efi_guid_text[i].text; > + > + uuid_bin_to_str(guid->b, unknown_guid, UUID_STR_FORMAT_GUID); > + > + return unknown_guid; > +} > + > /** > * efi_dump_single_var() - show information about a UEFI variable > * > * @name: Name of the variable > * @guid: Vendor GUID > + * @verbose: if true, dump data > * > * Show information encoded in one UEFI variable > */ > -static void efi_dump_single_var(u16 *name, efi_guid_t *guid) > +static void efi_dump_single_var(u16 *name, efi_guid_t *guid, bool verbose) > { > u32 attributes; > u8 *data; > @@ -68,7 +102,7 @@ static void efi_dump_single_var(u16 *name, efi_guid_t *guid) > if (ret != EFI_SUCCESS) > goto out; > > - printf("%ls:", name); > + printf("%ls:\n %s:", name, efi_guid_to_str(guid)); > for (count = 0, i = 0; i < ARRAY_SIZE(efi_var_attrs); i++) > if (attributes & efi_var_attrs[i].mask) { > if (count) > @@ -79,7 +113,9 @@ static void efi_dump_single_var(u16 *name, efi_guid_t *guid) > puts(efi_var_attrs[i].text); > } > printf(", DataSize = 0x%zx\n", size); > - print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, data, size, true); > + if (verbose) > + print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > + data, size, true); > > out: > free(data); > @@ -90,11 +126,12 @@ out: > * > * @argc: Number of arguments (variables) > * @argv: Argument (variable name) array > + * @verbose: if true, dump data > * Return: CMD_RET_SUCCESS on success, or CMD_RET_RET_FAILURE > * > * Show information encoded in named UEFI variables > */ > -static int efi_dump_vars(int argc, char * const argv[]) > +static int efi_dump_vars(int argc, char * const argv[], bool verbose) > { > u16 *var_name16, *p; > efi_uintn_t buf_size, size; > @@ -120,7 +157,8 @@ static int efi_dump_vars(int argc, char * const argv[]) > utf8_utf16_strcpy(&p, argv[0]); > > efi_dump_single_var(var_name16, > - (efi_guid_t *)&efi_global_variable_guid); > + (efi_guid_t *)&efi_global_variable_guid, > + verbose); > } > > free(var_name16); > @@ -131,11 +169,12 @@ static int efi_dump_vars(int argc, char * const argv[]) > /** > * efi_dump_vars() - show information about all the UEFI variables > * > + * @verbose: if true, dump data > * Return: CMD_RET_SUCCESS on success, or CMD_RET_RET_FAILURE > * > * Show information encoded in all the UEFI variables > */ > -static int efi_dump_var_all(void) > +static int efi_dump_var_all(bool verbose) > { > u16 *var_name16, *p; > efi_uintn_t buf_size, size; > @@ -171,7 +210,7 @@ static int efi_dump_var_all(void) > return CMD_RET_FAILURE; > } > > - efi_dump_single_var(var_name16, &guid); > + efi_dump_single_var(var_name16, &guid, verbose); > } > > free(var_name16); > @@ -189,12 +228,13 @@ static int efi_dump_var_all(void) > * Return: CMD_RET_SUCCESS on success, or CMD_RET_RET_FAILURE > * > * This function is for "env print -e" or "printenv -e" command: > - * => env print -e [var [...]] > + * => env print -e [-v] [var [...]] > * If one or more variable names are specified, show information > * named UEFI variables, otherwise show all the UEFI variables. > */ > int do_env_print_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > { > + bool verbose; > efi_status_t ret; > > /* Initialize EFI drivers */ > @@ -205,12 +245,23 @@ int do_env_print_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > return CMD_RET_FAILURE; > } > > - if (argc > 1) > + verbose = false; > + for (argc--, argv++; argc > 0 && argv[0][0] == '-'; argc--, argv++) { > + switch (argv[0][1]) { > + case 'v': > + verbose = true; > + break; > + default: > + return CMD_RET_USAGE; > + } > + } > + > + if (argc) > /* show specified UEFI variables */ > - return efi_dump_vars(--argc, ++argv); > + return efi_dump_vars(argc, argv, verbose); > > /* enumerate and show all UEFI variables */ > - return efi_dump_var_all(); > + return efi_dump_var_all(verbose); > } > > /** > @@ -339,18 +390,22 @@ out: > * Return: CMD_RET_SUCCESS on success, or CMD_RET_RET_FAILURE > * > * This function is for "env set -e" or "setenv -e" command: > - * => env set -e var [value ...]] > + * => env set -e [-guid guid][-nv][-bs][-rt][-a][-v] > + * [-i address,size] var, or > + * var [value ...] > * Encode values specified and set given UEFI variable. > * If no value is specified, delete the variable. > */ > int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > { > - char *var_name, *value = NULL; > - efi_uintn_t size = 0; > - u16 *var_name16 = NULL, *p; > - size_t len; > + char *var_name, *value, *ep; > + ulong addr; > + efi_uintn_t size; > efi_guid_t guid; > u32 attributes; > + bool default_guid, verbose, value_on_memory; > + u16 *var_name16 = NULL, *p; > + size_t len; > efi_status_t ret; > > if (argc == 1) > @@ -364,32 +419,92 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > return CMD_RET_FAILURE; > } > > - attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | > - EFI_VARIABLE_RUNTIME_ACCESS; > - if (!strcmp(argv[1], "-nv")) { > - attributes |= EFI_VARIABLE_NON_VOLATILE; > - argc--; > - argv++; > - if (argc == 1) > - return CMD_RET_SUCCESS; > + /* > + * attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | > + * EFI_VARIABLE_RUNTIME_ACCESS; > + */ > + value = NULL; > + size = 0; > + attributes = 0; > + guid = efi_global_variable_guid; > + default_guid = true; > + verbose = false; > + value_on_memory = false; > + for (argc--, argv++; argc > 0 && argv[0][0] == '-'; argc--, argv++) { > + if (!strcmp(argv[0], "-guid")) { > + if (argc == 1) > + return CMD_RET_USAGE; > + > + argc--; > + argv++; > + if (uuid_str_to_bin(argv[0], guid.b, > + UUID_STR_FORMAT_GUID)) > + return CMD_RET_FAILURE; > + default_guid = false; > + } else if (!strcmp(argv[0], "-bs")) { > + attributes |= EFI_VARIABLE_BOOTSERVICE_ACCESS; > + } else if (!strcmp(argv[0], "-rt")) { > + attributes |= EFI_VARIABLE_RUNTIME_ACCESS; > + } else if (!strcmp(argv[0], "-nv")) { > + attributes |= EFI_VARIABLE_NON_VOLATILE; > + } else if (!strcmp(argv[0], "-a")) { > + attributes |= EFI_VARIABLE_APPEND_WRITE; > + } else if (!strcmp(argv[0], "-i")) { > + /* data comes from memory */ > + if (argc == 1) > + return CMD_RET_USAGE; > + > + argc--; > + argv++; > + addr = simple_strtoul(argv[0], &ep, 16); > + if (*ep != ',') > + return CMD_RET_USAGE; > + > + size = simple_strtoul(++ep, NULL, 16); > + if (!size) > + return CMD_RET_FAILURE; > + value_on_memory = true; > + } else if (!strcmp(argv[0], "-v")) { > + verbose = true; > + } else { > + return CMD_RET_USAGE; > + } > } > + if (!argc) > + return CMD_RET_USAGE; > + > + var_name = argv[0]; > + if (default_guid) > + guid = efi_global_variable_guid; > > - var_name = argv[1]; > - if (argc == 2) { > - /* delete */ > - value = NULL; > - size = 0; > - } else { /* set */ > - argc -= 2; > - argv += 2; > + if (verbose) { > + printf("GUID: %s\n", efi_guid_to_str(&guid)); > + printf("Attributes: 0x%x\n", attributes); > + } > > - for ( ; argc > 0; argc--, argv++) > + /* for value */ > + if (value_on_memory) > + value = map_sysmem(addr, 0); > + else if (argc > 1) > + for (argc--, argv++; argc > 0; argc--, argv++) > if (append_value(&value, &size, argv[0]) < 0) { > printf("## Failed to process an argument, %s\n", > argv[0]); > ret = CMD_RET_FAILURE; > goto out; > } > + > + if (size && !(attributes & (EFI_VARIABLE_BOOTSERVICE_ACCESS | > + EFI_VARIABLE_RUNTIME_ACCESS))) { > + printf("## Attributes (-bs or -rt) must be specified\n"); > + ret = CMD_RET_FAILURE; > + goto out; > + } > + > + if (size && verbose) { > + printf("Value:\n"); > + print_hex_dump(" ", DUMP_PREFIX_OFFSET, > + 16, 1, value, size, true); > } > > len = utf8_utf16_strnlen(var_name, strlen(var_name)); > @@ -402,17 +517,42 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > p = var_name16; > utf8_utf16_strncpy(&p, var_name, len + 1); > > - guid = efi_global_variable_guid; > ret = EFI_CALL(efi_set_variable(var_name16, &guid, attributes, > size, value)); > + unmap_sysmem(value); > if (ret == EFI_SUCCESS) { > ret = CMD_RET_SUCCESS; > } else { > - printf("## Failed to set EFI variable\n"); > + const char *msg; > + > + switch (ret) { > + case EFI_NOT_FOUND: > + msg = " (not found)"; > + break; > + case EFI_WRITE_PROTECTED: > + msg = " (read only)"; > + break; > + case EFI_INVALID_PARAMETER: > + msg = " (invalid parameter)"; > + break; > + case EFI_SECURITY_VIOLATION: > + msg = " (validation failed)"; > + break; > + case EFI_OUT_OF_RESOURCES: > + msg = " (out of memory)"; > + break; > + default: > + msg = ""; > + break; > + } > + printf("## Failed to set EFI variable%s\n", msg); > ret = CMD_RET_FAILURE; > } > out: > - free(value); > + if (value_on_memory) > + unmap_sysmem(value); > + else > + free(value); > free(var_name16); > > return ret; >
On Sat, Oct 05, 2019 at 08:53:39AM +0200, Heinrich Schuchardt wrote: > On 10/4/19 3:20 AM, AKASHI Takahiro wrote: > >With this patch, when setting UEFI variable with "env set -e" command, > >we will be able to > >- specify vendor guid with "-guid guid", > >- specify variable attributes, BOOTSERVICE_ACCESS, RUNTIME_ACCESS, > > respectively with "-bs" and "-rt", > >- append a value instead of overwriting with "-a", > >- use memory as variable's value instead of explicit values given > > at the command line with "-i address,size" > > > >If guid is not explicitly given, default value will be used. > > > >When "-at" is given, a variable should be authenticated with > >appropriate signature database before setting or modifying its value. > >(Authentication is not supported yet though.) > > Didn't you remove this in v2? It was an editorial error, which also existed in v2 commit message. > > > >Meanwhile, "env print -e," will be modified so that it will dump > >a variable's value only if '-v' (verbose) is specified. > > > >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > This does not work as expected: > > => setenv -e -guid 0123456789abcdef0123456789abcdef -bs ARGONAUT hello world > => printenv -e > OsIndicationsSupported: > EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x8 > PlatformLang: > EFI_GLOBAL_VARIABLE_GUID: NV|BS|RT, DataSize = 0x6 > PlatformLangCodes: > EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x6 > RuntimeServicesSupported: > EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x2 > > Neither do I see an error nor is the variable set. You have an incorrect guid format, so env_set_efi() should return CMD_RET_USAGE, but mistakenly returns CMD_RET_FAILURE. > The patch increases the size of u-boot.bin by 2216 bytes for qemu-arm64. > Tom is always asking me how we can stop the size increase in the UEFI > sub-system. > > Why do we need this patch? This functionality is already available via > the UEFI shell. Such a question should have been made before initially "-e" option has been introduced a long time ago. * As far as we have "-e" option, this new patch is a natural extension. * Why do we have to reply on external apps? We should have a minimum self-contained command set to manage UEFI as part of U-Boot. * "-i" is used in secure boot pytest. Thanks, -Takahiro Akashi > Just open the UEFI shell and run > > Shell> setvar ARGONAUT -guid 01234567-89ab-cdef-0123-456789abcdef -rt > =L"Hello world" > Shell> exit > > Best regards > > Heinrich > > > >--- > >Changes in v3 (Oct 4, 2019) > >* add verbose messages when SetVariable() fails > > > >Changes in v2 (Sept 6, 2019) > >* remove "-at" option > > > >--- > > cmd/nvedit.c | 19 +++-- > > cmd/nvedit_efi.c | 212 +++++++++++++++++++++++++++++++++++++++-------- > > 2 files changed, 190 insertions(+), 41 deletions(-) > > > >diff --git a/cmd/nvedit.c b/cmd/nvedit.c > >index 1cb0bc1460b9..1e542972db30 100644 > >--- a/cmd/nvedit.c > >+++ b/cmd/nvedit.c > >@@ -1387,7 +1387,7 @@ static char env_help_text[] = > > #endif > > "env print [-a | name ...] - print environment\n" > > #if defined(CONFIG_CMD_NVEDIT_EFI) > >- "env print -e [name ...] - print UEFI environment\n" > >+ "env print -e [-v] [name ...] - print UEFI environment\n" > > #endif > > #if defined(CONFIG_CMD_RUN) > > "env run var [...] - run commands in an environment variable\n" > >@@ -1399,7 +1399,8 @@ static char env_help_text[] = > > #endif > > #endif > > #if defined(CONFIG_CMD_NVEDIT_EFI) > >- "env set -e name [arg ...] - set UEFI variable; unset if 'arg' not specified\n" > >+ "env set -e [-nv][-bs][-rt][-a][-i addr,size][-v] name [arg ...]\n" > >+ " - set UEFI variable; unset if '-i' or 'arg' not specified\n" > > #endif > > "env set [-f] name [arg ...]\n"; > > #endif > >@@ -1428,8 +1429,9 @@ U_BOOT_CMD_COMPLETE( > > "print environment variables", > > "[-a]\n - print [all] values of all environment variables\n" > > #if defined(CONFIG_CMD_NVEDIT_EFI) > >- "printenv -e [name ...]\n" > >+ "printenv -e [-v] [name ...]\n" > > " - print UEFI variable 'name' or all the variables\n" > >+ " \"-v\": verbose for signature database\n" > > #endif > > "printenv name ...\n" > > " - print value of environment variable 'name'", > >@@ -1459,9 +1461,16 @@ U_BOOT_CMD_COMPLETE( > > setenv, CONFIG_SYS_MAXARGS, 0, do_env_set, > > "set environment variables", > > #if defined(CONFIG_CMD_NVEDIT_EFI) > >- "-e [-nv] name [value ...]\n" > >+ "-e [-guid guid][-nv][-bs][-rt][-a][-v]\n" > >+ " [-i addr,size name], or [name [value ...]]\n" > > " - set UEFI variable 'name' to 'value' ...'\n" > >- " 'nv' option makes the variable non-volatile\n" > >+ " \"-guid\": set vendor guid\n" > >+ " \"-nv\": set non-volatile attribute\n" > >+ " \"-bs\": set boot-service attribute\n" > >+ " \"-rt\": set runtime attribute\n" > >+ " \"-a\": append-write\n" > >+ " \"-i addr,size\": use <addr,size> as variable's value\n" > >+ " \"-v\": verbose print\n" > > " - delete UEFI variable 'name' if 'value' not specified\n" > > #endif > > "setenv [-f] name value ...\n" > >diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c > >index ed6d09a53046..795c95be0ef7 100644 > >--- a/cmd/nvedit_efi.c > >+++ b/cmd/nvedit_efi.c > >@@ -13,6 +13,7 @@ > > #include <exports.h> > > #include <hexdump.h> > > #include <malloc.h> > >+#include <mapmem.h> > > #include <linux/kernel.h> > > > > /* > >@@ -34,15 +35,48 @@ static const struct { > > {EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, "AT"}, > > }; > > > >+static const struct { > >+ efi_guid_t guid; > >+ char *text; > >+} efi_guid_text[] = { > >+ /* signature database */ > >+ {EFI_GLOBAL_VARIABLE_GUID, "EFI_GLOBAL_VARIABLE_GUID"}, > >+}; > >+ > >+/* "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" */ > >+static char unknown_guid[37]; > >+ > >+/** > >+ * efi_guid_to_str() - convert guid to readable name > >+ * > >+ * @guid: GUID > >+ * Return: string for GUID > >+ * > >+ * convert guid to readable name > >+ */ > >+static const char *efi_guid_to_str(efi_guid_t *guid) > >+{ > >+ int i; > >+ > >+ for (i = 0; i < ARRAY_SIZE(efi_guid_text); i++) > >+ if (!guidcmp(guid, &efi_guid_text[i].guid)) > >+ return efi_guid_text[i].text; > >+ > >+ uuid_bin_to_str(guid->b, unknown_guid, UUID_STR_FORMAT_GUID); > >+ > >+ return unknown_guid; > >+} > >+ > > /** > > * efi_dump_single_var() - show information about a UEFI variable > > * > > * @name: Name of the variable > > * @guid: Vendor GUID > >+ * @verbose: if true, dump data > > * > > * Show information encoded in one UEFI variable > > */ > >-static void efi_dump_single_var(u16 *name, efi_guid_t *guid) > >+static void efi_dump_single_var(u16 *name, efi_guid_t *guid, bool verbose) > > { > > u32 attributes; > > u8 *data; > >@@ -68,7 +102,7 @@ static void efi_dump_single_var(u16 *name, efi_guid_t *guid) > > if (ret != EFI_SUCCESS) > > goto out; > > > >- printf("%ls:", name); > >+ printf("%ls:\n %s:", name, efi_guid_to_str(guid)); > > for (count = 0, i = 0; i < ARRAY_SIZE(efi_var_attrs); i++) > > if (attributes & efi_var_attrs[i].mask) { > > if (count) > >@@ -79,7 +113,9 @@ static void efi_dump_single_var(u16 *name, efi_guid_t *guid) > > puts(efi_var_attrs[i].text); > > } > > printf(", DataSize = 0x%zx\n", size); > >- print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, data, size, true); > >+ if (verbose) > >+ print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > >+ data, size, true); > > > > out: > > free(data); > >@@ -90,11 +126,12 @@ out: > > * > > * @argc: Number of arguments (variables) > > * @argv: Argument (variable name) array > >+ * @verbose: if true, dump data > > * Return: CMD_RET_SUCCESS on success, or CMD_RET_RET_FAILURE > > * > > * Show information encoded in named UEFI variables > > */ > >-static int efi_dump_vars(int argc, char * const argv[]) > >+static int efi_dump_vars(int argc, char * const argv[], bool verbose) > > { > > u16 *var_name16, *p; > > efi_uintn_t buf_size, size; > >@@ -120,7 +157,8 @@ static int efi_dump_vars(int argc, char * const argv[]) > > utf8_utf16_strcpy(&p, argv[0]); > > > > efi_dump_single_var(var_name16, > >- (efi_guid_t *)&efi_global_variable_guid); > >+ (efi_guid_t *)&efi_global_variable_guid, > >+ verbose); > > } > > > > free(var_name16); > >@@ -131,11 +169,12 @@ static int efi_dump_vars(int argc, char * const argv[]) > > /** > > * efi_dump_vars() - show information about all the UEFI variables > > * > >+ * @verbose: if true, dump data > > * Return: CMD_RET_SUCCESS on success, or CMD_RET_RET_FAILURE > > * > > * Show information encoded in all the UEFI variables > > */ > >-static int efi_dump_var_all(void) > >+static int efi_dump_var_all(bool verbose) > > { > > u16 *var_name16, *p; > > efi_uintn_t buf_size, size; > >@@ -171,7 +210,7 @@ static int efi_dump_var_all(void) > > return CMD_RET_FAILURE; > > } > > > >- efi_dump_single_var(var_name16, &guid); > >+ efi_dump_single_var(var_name16, &guid, verbose); > > } > > > > free(var_name16); > >@@ -189,12 +228,13 @@ static int efi_dump_var_all(void) > > * Return: CMD_RET_SUCCESS on success, or CMD_RET_RET_FAILURE > > * > > * This function is for "env print -e" or "printenv -e" command: > >- * => env print -e [var [...]] > >+ * => env print -e [-v] [var [...]] > > * If one or more variable names are specified, show information > > * named UEFI variables, otherwise show all the UEFI variables. > > */ > > int do_env_print_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > { > >+ bool verbose; > > efi_status_t ret; > > > > /* Initialize EFI drivers */ > >@@ -205,12 +245,23 @@ int do_env_print_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > return CMD_RET_FAILURE; > > } > > > >- if (argc > 1) > >+ verbose = false; > >+ for (argc--, argv++; argc > 0 && argv[0][0] == '-'; argc--, argv++) { > >+ switch (argv[0][1]) { > >+ case 'v': > >+ verbose = true; > >+ break; > >+ default: > >+ return CMD_RET_USAGE; > >+ } > >+ } > >+ > >+ if (argc) > > /* show specified UEFI variables */ > >- return efi_dump_vars(--argc, ++argv); > >+ return efi_dump_vars(argc, argv, verbose); > > > > /* enumerate and show all UEFI variables */ > >- return efi_dump_var_all(); > >+ return efi_dump_var_all(verbose); > > } > > > > /** > >@@ -339,18 +390,22 @@ out: > > * Return: CMD_RET_SUCCESS on success, or CMD_RET_RET_FAILURE > > * > > * This function is for "env set -e" or "setenv -e" command: > >- * => env set -e var [value ...]] > >+ * => env set -e [-guid guid][-nv][-bs][-rt][-a][-v] > >+ * [-i address,size] var, or > >+ * var [value ...] > > * Encode values specified and set given UEFI variable. > > * If no value is specified, delete the variable. > > */ > > int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > { > >- char *var_name, *value = NULL; > >- efi_uintn_t size = 0; > >- u16 *var_name16 = NULL, *p; > >- size_t len; > >+ char *var_name, *value, *ep; > >+ ulong addr; > >+ efi_uintn_t size; > > efi_guid_t guid; > > u32 attributes; > >+ bool default_guid, verbose, value_on_memory; > >+ u16 *var_name16 = NULL, *p; > >+ size_t len; > > efi_status_t ret; > > > > if (argc == 1) > >@@ -364,32 +419,92 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > return CMD_RET_FAILURE; > > } > > > >- attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | > >- EFI_VARIABLE_RUNTIME_ACCESS; > >- if (!strcmp(argv[1], "-nv")) { > >- attributes |= EFI_VARIABLE_NON_VOLATILE; > >- argc--; > >- argv++; > >- if (argc == 1) > >- return CMD_RET_SUCCESS; > >+ /* > >+ * attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | > >+ * EFI_VARIABLE_RUNTIME_ACCESS; > >+ */ > >+ value = NULL; > >+ size = 0; > >+ attributes = 0; > >+ guid = efi_global_variable_guid; > >+ default_guid = true; > >+ verbose = false; > >+ value_on_memory = false; > >+ for (argc--, argv++; argc > 0 && argv[0][0] == '-'; argc--, argv++) { > >+ if (!strcmp(argv[0], "-guid")) { > >+ if (argc == 1) > >+ return CMD_RET_USAGE; > >+ > >+ argc--; > >+ argv++; > >+ if (uuid_str_to_bin(argv[0], guid.b, > >+ UUID_STR_FORMAT_GUID)) > >+ return CMD_RET_FAILURE; > >+ default_guid = false; > >+ } else if (!strcmp(argv[0], "-bs")) { > >+ attributes |= EFI_VARIABLE_BOOTSERVICE_ACCESS; > >+ } else if (!strcmp(argv[0], "-rt")) { > >+ attributes |= EFI_VARIABLE_RUNTIME_ACCESS; > >+ } else if (!strcmp(argv[0], "-nv")) { > >+ attributes |= EFI_VARIABLE_NON_VOLATILE; > >+ } else if (!strcmp(argv[0], "-a")) { > >+ attributes |= EFI_VARIABLE_APPEND_WRITE; > >+ } else if (!strcmp(argv[0], "-i")) { > >+ /* data comes from memory */ > >+ if (argc == 1) > >+ return CMD_RET_USAGE; > >+ > >+ argc--; > >+ argv++; > >+ addr = simple_strtoul(argv[0], &ep, 16); > >+ if (*ep != ',') > >+ return CMD_RET_USAGE; > >+ > >+ size = simple_strtoul(++ep, NULL, 16); > >+ if (!size) > >+ return CMD_RET_FAILURE; > >+ value_on_memory = true; > >+ } else if (!strcmp(argv[0], "-v")) { > >+ verbose = true; > >+ } else { > >+ return CMD_RET_USAGE; > >+ } > > } > >+ if (!argc) > >+ return CMD_RET_USAGE; > >+ > >+ var_name = argv[0]; > >+ if (default_guid) > >+ guid = efi_global_variable_guid; > > > >- var_name = argv[1]; > >- if (argc == 2) { > >- /* delete */ > >- value = NULL; > >- size = 0; > >- } else { /* set */ > >- argc -= 2; > >- argv += 2; > >+ if (verbose) { > >+ printf("GUID: %s\n", efi_guid_to_str(&guid)); > >+ printf("Attributes: 0x%x\n", attributes); > >+ } > > > >- for ( ; argc > 0; argc--, argv++) > >+ /* for value */ > >+ if (value_on_memory) > >+ value = map_sysmem(addr, 0); > >+ else if (argc > 1) > >+ for (argc--, argv++; argc > 0; argc--, argv++) > > if (append_value(&value, &size, argv[0]) < 0) { > > printf("## Failed to process an argument, %s\n", > > argv[0]); > > ret = CMD_RET_FAILURE; > > goto out; > > } > >+ > >+ if (size && !(attributes & (EFI_VARIABLE_BOOTSERVICE_ACCESS | > >+ EFI_VARIABLE_RUNTIME_ACCESS))) { > >+ printf("## Attributes (-bs or -rt) must be specified\n"); > >+ ret = CMD_RET_FAILURE; > >+ goto out; > >+ } > >+ > >+ if (size && verbose) { > >+ printf("Value:\n"); > >+ print_hex_dump(" ", DUMP_PREFIX_OFFSET, > >+ 16, 1, value, size, true); > > } > > > > len = utf8_utf16_strnlen(var_name, strlen(var_name)); > >@@ -402,17 +517,42 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > p = var_name16; > > utf8_utf16_strncpy(&p, var_name, len + 1); > > > >- guid = efi_global_variable_guid; > > ret = EFI_CALL(efi_set_variable(var_name16, &guid, attributes, > > size, value)); > >+ unmap_sysmem(value); > > if (ret == EFI_SUCCESS) { > > ret = CMD_RET_SUCCESS; > > } else { > >- printf("## Failed to set EFI variable\n"); > >+ const char *msg; > >+ > >+ switch (ret) { > >+ case EFI_NOT_FOUND: > >+ msg = " (not found)"; > >+ break; > >+ case EFI_WRITE_PROTECTED: > >+ msg = " (read only)"; > >+ break; > >+ case EFI_INVALID_PARAMETER: > >+ msg = " (invalid parameter)"; > >+ break; > >+ case EFI_SECURITY_VIOLATION: > >+ msg = " (validation failed)"; > >+ break; > >+ case EFI_OUT_OF_RESOURCES: > >+ msg = " (out of memory)"; > >+ break; > >+ default: > >+ msg = ""; > >+ break; > >+ } > >+ printf("## Failed to set EFI variable%s\n", msg); > > ret = CMD_RET_FAILURE; > > } > > out: > >- free(value); > >+ if (value_on_memory) > >+ unmap_sysmem(value); > >+ else > >+ free(value); > > free(var_name16); > > > > return ret; > > >
On Mon, Oct 07, 2019 at 09:47:46AM +0900, AKASHI Takahiro wrote: > On Sat, Oct 05, 2019 at 08:53:39AM +0200, Heinrich Schuchardt wrote: > > On 10/4/19 3:20 AM, AKASHI Takahiro wrote: > > >With this patch, when setting UEFI variable with "env set -e" command, > > >we will be able to > > >- specify vendor guid with "-guid guid", > > >- specify variable attributes, BOOTSERVICE_ACCESS, RUNTIME_ACCESS, > > > respectively with "-bs" and "-rt", > > >- append a value instead of overwriting with "-a", > > >- use memory as variable's value instead of explicit values given > > > at the command line with "-i address,size" > > > > > >If guid is not explicitly given, default value will be used. > > > > > >When "-at" is given, a variable should be authenticated with > > >appropriate signature database before setting or modifying its value. > > >(Authentication is not supported yet though.) > > > > Didn't you remove this in v2? > > It was an editorial error, which also existed in v2 commit message. > > > > > > >Meanwhile, "env print -e," will be modified so that it will dump > > >a variable's value only if '-v' (verbose) is specified. > > > > > >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > This does not work as expected: > > > > => setenv -e -guid 0123456789abcdef0123456789abcdef -bs ARGONAUT hello world > > => printenv -e > > OsIndicationsSupported: > > EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x8 > > PlatformLang: > > EFI_GLOBAL_VARIABLE_GUID: NV|BS|RT, DataSize = 0x6 > > PlatformLangCodes: > > EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x6 > > RuntimeServicesSupported: > > EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x2 > > > > Neither do I see an error nor is the variable set. > > You have an incorrect guid format, so env_set_efi() should > return CMD_RET_USAGE, but mistakenly returns CMD_RET_FAILURE. > > > The patch increases the size of u-boot.bin by 2216 bytes for qemu-arm64. > > Tom is always asking me how we can stop the size increase in the UEFI > > sub-system. > > > > Why do we need this patch? This functionality is already available via > > the UEFI shell. > > Such a question should have been made before initially "-e" option > has been introduced a long time ago. > > * As far as we have "-e" option, this new patch is a natural extension. > * Why do we have to reply on external apps? We should have a minimum > self-contained command set to manage UEFI as part of U-Boot. > * "-i" is used in secure boot pytest. There is a difficult balance to find here. On the one hand, there is the argument (that I find compelling) which is that we want EFI loader support enabled, by default, on architectures where there is EFI support as it provides a consistent interface between ${random board} and ${off the shelf SW stack}. On the other hand, since this is a feature that's being enabled on a big majority of our platforms we need to be extremely wary of code size increases. So while I do think that some defconfigs (qemu* for example, and sandbox) should be fine to enable everything on (especially sandbox as there's where coverity runs), most configs should only get 'default y' via Kconfig for things required to load the common EFI applications.
On Sun, Oct 06, 2019 at 09:42:30PM -0400, Tom Rini wrote: > On Mon, Oct 07, 2019 at 09:47:46AM +0900, AKASHI Takahiro wrote: > > On Sat, Oct 05, 2019 at 08:53:39AM +0200, Heinrich Schuchardt wrote: > > > On 10/4/19 3:20 AM, AKASHI Takahiro wrote: > > > >With this patch, when setting UEFI variable with "env set -e" command, > > > >we will be able to > > > >- specify vendor guid with "-guid guid", > > > >- specify variable attributes, BOOTSERVICE_ACCESS, RUNTIME_ACCESS, > > > > respectively with "-bs" and "-rt", > > > >- append a value instead of overwriting with "-a", > > > >- use memory as variable's value instead of explicit values given > > > > at the command line with "-i address,size" > > > > > > > >If guid is not explicitly given, default value will be used. > > > > > > > >When "-at" is given, a variable should be authenticated with > > > >appropriate signature database before setting or modifying its value. > > > >(Authentication is not supported yet though.) > > > > > > Didn't you remove this in v2? > > > > It was an editorial error, which also existed in v2 commit message. > > > > > > > > > >Meanwhile, "env print -e," will be modified so that it will dump > > > >a variable's value only if '-v' (verbose) is specified. > > > > > > > >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > This does not work as expected: > > > > > > => setenv -e -guid 0123456789abcdef0123456789abcdef -bs ARGONAUT hello world > > > => printenv -e > > > OsIndicationsSupported: > > > EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x8 > > > PlatformLang: > > > EFI_GLOBAL_VARIABLE_GUID: NV|BS|RT, DataSize = 0x6 > > > PlatformLangCodes: > > > EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x6 > > > RuntimeServicesSupported: > > > EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x2 > > > > > > Neither do I see an error nor is the variable set. > > > > You have an incorrect guid format, so env_set_efi() should > > return CMD_RET_USAGE, but mistakenly returns CMD_RET_FAILURE. > > > > > The patch increases the size of u-boot.bin by 2216 bytes for qemu-arm64. > > > Tom is always asking me how we can stop the size increase in the UEFI > > > sub-system. > > > > > > Why do we need this patch? This functionality is already available via > > > the UEFI shell. > > > > Such a question should have been made before initially "-e" option > > has been introduced a long time ago. > > > > * As far as we have "-e" option, this new patch is a natural extension. > > * Why do we have to reply on external apps? We should have a minimum > > self-contained command set to manage UEFI as part of U-Boot. > > * "-i" is used in secure boot pytest. > > There is a difficult balance to find here. On the one hand, there is > the argument (that I find compelling) which is that we want EFI loader > support enabled, by default, on architectures where there is EFI support > as it provides a consistent interface between ${random board} and ${off > the shelf SW stack}. On the other hand, since this is a feature that's > being enabled on a big majority of our platforms we need to be extremely > wary of code size increases. > > So while I do think that some defconfigs (qemu* for example, and > sandbox) should be fine to enable everything on (especially sandbox as > there's where coverity runs), most configs should only get 'default y' > via Kconfig for things required to load the common EFI applications. What is your definition of *common* EFI applications? Do they include Shell.efi even on tight-resource platforms? BTW, if you don't really like "env [set|print] -e" syntax, we can move all the stuff to "efidebug" command. This was the original place where I put them in my initial patch. Thanks, -Takahiro Akashi > > -- > Tom
On Mon, Oct 07, 2019 at 02:02:26PM +0900, AKASHI Takahiro wrote: > On Sun, Oct 06, 2019 at 09:42:30PM -0400, Tom Rini wrote: > > On Mon, Oct 07, 2019 at 09:47:46AM +0900, AKASHI Takahiro wrote: > > > On Sat, Oct 05, 2019 at 08:53:39AM +0200, Heinrich Schuchardt wrote: > > > > On 10/4/19 3:20 AM, AKASHI Takahiro wrote: > > > > >With this patch, when setting UEFI variable with "env set -e" command, > > > > >we will be able to > > > > >- specify vendor guid with "-guid guid", > > > > >- specify variable attributes, BOOTSERVICE_ACCESS, RUNTIME_ACCESS, > > > > > respectively with "-bs" and "-rt", > > > > >- append a value instead of overwriting with "-a", > > > > >- use memory as variable's value instead of explicit values given > > > > > at the command line with "-i address,size" > > > > > > > > > >If guid is not explicitly given, default value will be used. > > > > > > > > > >When "-at" is given, a variable should be authenticated with > > > > >appropriate signature database before setting or modifying its value. > > > > >(Authentication is not supported yet though.) > > > > > > > > Didn't you remove this in v2? > > > > > > It was an editorial error, which also existed in v2 commit message. > > > > > > > > > > > > >Meanwhile, "env print -e," will be modified so that it will dump > > > > >a variable's value only if '-v' (verbose) is specified. > > > > > > > > > >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > > > This does not work as expected: > > > > > > > > => setenv -e -guid 0123456789abcdef0123456789abcdef -bs ARGONAUT hello world > > > > => printenv -e > > > > OsIndicationsSupported: > > > > EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x8 > > > > PlatformLang: > > > > EFI_GLOBAL_VARIABLE_GUID: NV|BS|RT, DataSize = 0x6 > > > > PlatformLangCodes: > > > > EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x6 > > > > RuntimeServicesSupported: > > > > EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x2 > > > > > > > > Neither do I see an error nor is the variable set. > > > > > > You have an incorrect guid format, so env_set_efi() should > > > return CMD_RET_USAGE, but mistakenly returns CMD_RET_FAILURE. > > > > > > > The patch increases the size of u-boot.bin by 2216 bytes for qemu-arm64. > > > > Tom is always asking me how we can stop the size increase in the UEFI > > > > sub-system. > > > > > > > > Why do we need this patch? This functionality is already available via > > > > the UEFI shell. > > > > > > Such a question should have been made before initially "-e" option > > > has been introduced a long time ago. > > > > > > * As far as we have "-e" option, this new patch is a natural extension. > > > * Why do we have to reply on external apps? We should have a minimum > > > self-contained command set to manage UEFI as part of U-Boot. > > > * "-i" is used in secure boot pytest. > > > > There is a difficult balance to find here. On the one hand, there is > > the argument (that I find compelling) which is that we want EFI loader > > support enabled, by default, on architectures where there is EFI support > > as it provides a consistent interface between ${random board} and ${off > > the shelf SW stack}. On the other hand, since this is a feature that's > > being enabled on a big majority of our platforms we need to be extremely > > wary of code size increases. > > > > So while I do think that some defconfigs (qemu* for example, and > > sandbox) should be fine to enable everything on (especially sandbox as > > there's where coverity runs), most configs should only get 'default y' > > via Kconfig for things required to load the common EFI applications. > > What is your definition of *common* EFI applications? > Do they include Shell.efi even on tight-resource platforms? I'm happy to get more feedback on this but my first pass is GRUB2 and *BSD loaders. Shell, SCT, etc are not. Specific platforms can enable whatever is needed there (even outside of qemu*/sandbox). And focusing on "tight resource platforms" is the wrong thing to focus on. Even on platforms where we have tons of space no one wants a loader that is larger than it needs to be. The bigger it is the longer it takes to read and relocate and get on with the business of running the system. > BTW, if you don't really like "env [set|print] -e" syntax, > we can move all the stuff to "efidebug" command. > This was the original place where I put them in my initial patch. I don't have a strong preference where this goes and will leave that to Heinrich but I do have preferences about binary size (and boot time increases) even when they seem small as they add up over time. Thanks for your understanding here, I do appreciate the time you've been investing here.
On 10/7/19 5:43 PM, Tom Rini wrote: > On Mon, Oct 07, 2019 at 02:02:26PM +0900, AKASHI Takahiro wrote: >> On Sun, Oct 06, 2019 at 09:42:30PM -0400, Tom Rini wrote: >>> On Mon, Oct 07, 2019 at 09:47:46AM +0900, AKASHI Takahiro wrote: >>>> On Sat, Oct 05, 2019 at 08:53:39AM +0200, Heinrich Schuchardt wrote: >>>>> On 10/4/19 3:20 AM, AKASHI Takahiro wrote: >>>>>> With this patch, when setting UEFI variable with "env set -e" command, >>>>>> we will be able to >>>>>> - specify vendor guid with "-guid guid", >>>>>> - specify variable attributes, BOOTSERVICE_ACCESS, RUNTIME_ACCESS, >>>>>> respectively with "-bs" and "-rt", >>>>>> - append a value instead of overwriting with "-a", >>>>>> - use memory as variable's value instead of explicit values given >>>>>> at the command line with "-i address,size" >>>>>> >>>>>> If guid is not explicitly given, default value will be used. >>>>>> >>>>>> When "-at" is given, a variable should be authenticated with >>>>>> appropriate signature database before setting or modifying its value. >>>>>> (Authentication is not supported yet though.) >>>>> >>>>> Didn't you remove this in v2? >>>> >>>> It was an editorial error, which also existed in v2 commit message. >>>> >>>>>> >>>>>> Meanwhile, "env print -e," will be modified so that it will dump >>>>>> a variable's value only if '-v' (verbose) is specified. >>>>>> >>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>>> >>>>> This does not work as expected: >>>>> >>>>> => setenv -e -guid 0123456789abcdef0123456789abcdef -bs ARGONAUT hello world >>>>> => printenv -e >>>>> OsIndicationsSupported: >>>>> EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x8 >>>>> PlatformLang: >>>>> EFI_GLOBAL_VARIABLE_GUID: NV|BS|RT, DataSize = 0x6 >>>>> PlatformLangCodes: >>>>> EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x6 >>>>> RuntimeServicesSupported: >>>>> EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x2 >>>>> >>>>> Neither do I see an error nor is the variable set. >>>> >>>> You have an incorrect guid format, so env_set_efi() should >>>> return CMD_RET_USAGE, but mistakenly returns CMD_RET_FAILURE. >>>> >>>>> The patch increases the size of u-boot.bin by 2216 bytes for qemu-arm64. >>>>> Tom is always asking me how we can stop the size increase in the UEFI >>>>> sub-system. >>>>> >>>>> Why do we need this patch? This functionality is already available via >>>>> the UEFI shell. >>>> >>>> Such a question should have been made before initially "-e" option >>>> has been introduced a long time ago. >>>> >>>> * As far as we have "-e" option, this new patch is a natural extension. >>>> * Why do we have to reply on external apps? We should have a minimum >>>> self-contained command set to manage UEFI as part of U-Boot. >>>> * "-i" is used in secure boot pytest. >>> >>> There is a difficult balance to find here. On the one hand, there is >>> the argument (that I find compelling) which is that we want EFI loader >>> support enabled, by default, on architectures where there is EFI support >>> as it provides a consistent interface between ${random board} and ${off >>> the shelf SW stack}. On the other hand, since this is a feature that's >>> being enabled on a big majority of our platforms we need to be extremely >>> wary of code size increases. >>> >>> So while I do think that some defconfigs (qemu* for example, and >>> sandbox) should be fine to enable everything on (especially sandbox as >>> there's where coverity runs), most configs should only get 'default y' >>> via Kconfig for things required to load the common EFI applications. >> >> What is your definition of *common* EFI applications? >> Do they include Shell.efi even on tight-resource platforms? > > I'm happy to get more feedback on this but my first pass is GRUB2 and > *BSD loaders. Shell, SCT, etc are not. Specific platforms can enable > whatever is needed there (even outside of qemu*/sandbox). And focusing > on "tight resource platforms" is the wrong thing to focus on. Even on > platforms where we have tons of space no one wants a loader that is > larger than it needs to be. The bigger it is the longer it takes to > read and relocate and get on with the business of running the system. > >> BTW, if you don't really like "env [set|print] -e" syntax, >> we can move all the stuff to "efidebug" command. >> This was the original place where I put them in my initial patch. > > I don't have a strong preference where this goes and will leave that to > Heinrich but I do have preferences about binary size (and boot time > increases) even when they seem small as they add up over time. Thanks > for your understanding here, I do appreciate the time you've been > investing here. > With https://lists.denx.de/pipermail/u-boot/2019-October/385626.html [PATCH 1/1] cmd: disable CMD_NVEDIT_EFI by default the code modified by this patch is disabled by default. Best regards Heinrich
On Mon, Oct 07, 2019 at 06:45:08PM +0200, Heinrich Schuchardt wrote: > On 10/7/19 5:43 PM, Tom Rini wrote: > >On Mon, Oct 07, 2019 at 02:02:26PM +0900, AKASHI Takahiro wrote: > >>On Sun, Oct 06, 2019 at 09:42:30PM -0400, Tom Rini wrote: > >>>On Mon, Oct 07, 2019 at 09:47:46AM +0900, AKASHI Takahiro wrote: > >>>>On Sat, Oct 05, 2019 at 08:53:39AM +0200, Heinrich Schuchardt wrote: > >>>>>On 10/4/19 3:20 AM, AKASHI Takahiro wrote: > >>>>>>With this patch, when setting UEFI variable with "env set -e" command, > >>>>>>we will be able to > >>>>>>- specify vendor guid with "-guid guid", > >>>>>>- specify variable attributes, BOOTSERVICE_ACCESS, RUNTIME_ACCESS, > >>>>>> respectively with "-bs" and "-rt", > >>>>>>- append a value instead of overwriting with "-a", > >>>>>>- use memory as variable's value instead of explicit values given > >>>>>> at the command line with "-i address,size" > >>>>>> > >>>>>>If guid is not explicitly given, default value will be used. > >>>>>> > >>>>>>When "-at" is given, a variable should be authenticated with > >>>>>>appropriate signature database before setting or modifying its value. > >>>>>>(Authentication is not supported yet though.) > >>>>> > >>>>>Didn't you remove this in v2? > >>>> > >>>>It was an editorial error, which also existed in v2 commit message. > >>>> > >>>>>> > >>>>>>Meanwhile, "env print -e," will be modified so that it will dump > >>>>>>a variable's value only if '-v' (verbose) is specified. > >>>>>> > >>>>>>Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>>>> > >>>>>This does not work as expected: > >>>>> > >>>>>=> setenv -e -guid 0123456789abcdef0123456789abcdef -bs ARGONAUT hello world > >>>>>=> printenv -e > >>>>>OsIndicationsSupported: > >>>>> EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x8 > >>>>>PlatformLang: > >>>>> EFI_GLOBAL_VARIABLE_GUID: NV|BS|RT, DataSize = 0x6 > >>>>>PlatformLangCodes: > >>>>> EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x6 > >>>>>RuntimeServicesSupported: > >>>>> EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x2 > >>>>> > >>>>>Neither do I see an error nor is the variable set. > >>>> > >>>>You have an incorrect guid format, so env_set_efi() should > >>>>return CMD_RET_USAGE, but mistakenly returns CMD_RET_FAILURE. > >>>> > >>>>>The patch increases the size of u-boot.bin by 2216 bytes for qemu-arm64. > >>>>>Tom is always asking me how we can stop the size increase in the UEFI > >>>>>sub-system. > >>>>> > >>>>>Why do we need this patch? This functionality is already available via > >>>>>the UEFI shell. > >>>> > >>>>Such a question should have been made before initially "-e" option > >>>>has been introduced a long time ago. > >>>> > >>>>* As far as we have "-e" option, this new patch is a natural extension. > >>>>* Why do we have to reply on external apps? We should have a minimum > >>>> self-contained command set to manage UEFI as part of U-Boot. > >>>>* "-i" is used in secure boot pytest. > >>> > >>>There is a difficult balance to find here. On the one hand, there is > >>>the argument (that I find compelling) which is that we want EFI loader > >>>support enabled, by default, on architectures where there is EFI support > >>>as it provides a consistent interface between ${random board} and ${off > >>>the shelf SW stack}. On the other hand, since this is a feature that's > >>>being enabled on a big majority of our platforms we need to be extremely > >>>wary of code size increases. > >>> > >>>So while I do think that some defconfigs (qemu* for example, and > >>>sandbox) should be fine to enable everything on (especially sandbox as > >>>there's where coverity runs), most configs should only get 'default y' > >>>via Kconfig for things required to load the common EFI applications. > >> > >>What is your definition of *common* EFI applications? > >>Do they include Shell.efi even on tight-resource platforms? > > > >I'm happy to get more feedback on this but my first pass is GRUB2 and > >*BSD loaders. Shell, SCT, etc are not. Specific platforms can enable > >whatever is needed there (even outside of qemu*/sandbox). And focusing > >on "tight resource platforms" is the wrong thing to focus on. Even on > >platforms where we have tons of space no one wants a loader that is > >larger than it needs to be. The bigger it is the longer it takes to > >read and relocate and get on with the business of running the system. > > > >>BTW, if you don't really like "env [set|print] -e" syntax, > >>we can move all the stuff to "efidebug" command. > >>This was the original place where I put them in my initial patch. > > > >I don't have a strong preference where this goes and will leave that to > >Heinrich but I do have preferences about binary size (and boot time > >increases) even when they seem small as they add up over time. Thanks > >for your understanding here, I do appreciate the time you've been > >investing here. > > > > With > > https://lists.denx.de/pipermail/u-boot/2019-October/385626.html > [PATCH 1/1] cmd: disable CMD_NVEDIT_EFI by default > > the code modified by this patch is disabled by default. So you have no reason to reject my patch, don't you? -Takahiro Akashi > Best regards > > Heinrich
On 10/8/19 2:05 AM, AKASHI Takahiro wrote: > On Mon, Oct 07, 2019 at 06:45:08PM +0200, Heinrich Schuchardt wrote: >> On 10/7/19 5:43 PM, Tom Rini wrote: >>> On Mon, Oct 07, 2019 at 02:02:26PM +0900, AKASHI Takahiro wrote: >>>> On Sun, Oct 06, 2019 at 09:42:30PM -0400, Tom Rini wrote: >>>>> On Mon, Oct 07, 2019 at 09:47:46AM +0900, AKASHI Takahiro wrote: >>>>>> On Sat, Oct 05, 2019 at 08:53:39AM +0200, Heinrich Schuchardt wrote: >>>>>>> On 10/4/19 3:20 AM, AKASHI Takahiro wrote: >>>>>>>> With this patch, when setting UEFI variable with "env set -e" command, >>>>>>>> we will be able to >>>>>>>> - specify vendor guid with "-guid guid", >>>>>>>> - specify variable attributes, BOOTSERVICE_ACCESS, RUNTIME_ACCESS, >>>>>>>> respectively with "-bs" and "-rt", >>>>>>>> - append a value instead of overwriting with "-a", >>>>>>>> - use memory as variable's value instead of explicit values given >>>>>>>> at the command line with "-i address,size" >>>>>>>> >>>>>>>> If guid is not explicitly given, default value will be used. >>>>>>>> >>>>>>>> When "-at" is given, a variable should be authenticated with >>>>>>>> appropriate signature database before setting or modifying its value. >>>>>>>> (Authentication is not supported yet though.) >>>>>>> >>>>>>> Didn't you remove this in v2? >>>>>> >>>>>> It was an editorial error, which also existed in v2 commit message. >>>>>> >>>>>>>> >>>>>>>> Meanwhile, "env print -e," will be modified so that it will dump >>>>>>>> a variable's value only if '-v' (verbose) is specified. >>>>>>>> >>>>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>>>>> >>>>>>> This does not work as expected: >>>>>>> >>>>>>> => setenv -e -guid 0123456789abcdef0123456789abcdef -bs ARGONAUT hello world >>>>>>> => printenv -e >>>>>>> OsIndicationsSupported: >>>>>>> EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x8 >>>>>>> PlatformLang: >>>>>>> EFI_GLOBAL_VARIABLE_GUID: NV|BS|RT, DataSize = 0x6 >>>>>>> PlatformLangCodes: >>>>>>> EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x6 >>>>>>> RuntimeServicesSupported: >>>>>>> EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x2 >>>>>>> >>>>>>> Neither do I see an error nor is the variable set. >>>>>> >>>>>> You have an incorrect guid format, so env_set_efi() should >>>>>> return CMD_RET_USAGE, but mistakenly returns CMD_RET_FAILURE. >>>>>> >>>>>>> The patch increases the size of u-boot.bin by 2216 bytes for qemu-arm64. >>>>>>> Tom is always asking me how we can stop the size increase in the UEFI >>>>>>> sub-system. >>>>>>> >>>>>>> Why do we need this patch? This functionality is already available via >>>>>>> the UEFI shell. >>>>>> >>>>>> Such a question should have been made before initially "-e" option >>>>>> has been introduced a long time ago. >>>>>> >>>>>> * As far as we have "-e" option, this new patch is a natural extension. >>>>>> * Why do we have to reply on external apps? We should have a minimum >>>>>> self-contained command set to manage UEFI as part of U-Boot. >>>>>> * "-i" is used in secure boot pytest. >>>>> >>>>> There is a difficult balance to find here. On the one hand, there is >>>>> the argument (that I find compelling) which is that we want EFI loader >>>>> support enabled, by default, on architectures where there is EFI support >>>>> as it provides a consistent interface between ${random board} and ${off >>>>> the shelf SW stack}. On the other hand, since this is a feature that's >>>>> being enabled on a big majority of our platforms we need to be extremely >>>>> wary of code size increases. >>>>> >>>>> So while I do think that some defconfigs (qemu* for example, and >>>>> sandbox) should be fine to enable everything on (especially sandbox as >>>>> there's where coverity runs), most configs should only get 'default y' >>>>> via Kconfig for things required to load the common EFI applications. >>>> >>>> What is your definition of *common* EFI applications? >>>> Do they include Shell.efi even on tight-resource platforms? >>> >>> I'm happy to get more feedback on this but my first pass is GRUB2 and >>> *BSD loaders. Shell, SCT, etc are not. Specific platforms can enable >>> whatever is needed there (even outside of qemu*/sandbox). And focusing >>> on "tight resource platforms" is the wrong thing to focus on. Even on >>> platforms where we have tons of space no one wants a loader that is >>> larger than it needs to be. The bigger it is the longer it takes to >>> read and relocate and get on with the business of running the system. >>> >>>> BTW, if you don't really like "env [set|print] -e" syntax, >>>> we can move all the stuff to "efidebug" command. >>>> This was the original place where I put them in my initial patch. >>> >>> I don't have a strong preference where this goes and will leave that to >>> Heinrich but I do have preferences about binary size (and boot time >>> increases) even when they seem small as they add up over time. Thanks >>> for your understanding here, I do appreciate the time you've been >>> investing here. >>> >> >> With >> >> https://lists.denx.de/pipermail/u-boot/2019-October/385626.html >> [PATCH 1/1] cmd: disable CMD_NVEDIT_EFI by default >> >> the code modified by this patch is disabled by default. > > So you have no reason to reject my patch, don't you? setenv -e -guid 12345678123456781234567812345678 foo bar neither sets a value nor gives an error. Please, fix this. Next I tried: => setenv -e -guid 12345678-1234-5678-1234-567812345678 foo bar ## Attributes (-bs or -rt) must be specified So I followed the advice given by the command: => setenv -e -guid 12345678-1234-5678-1234-567812345678 -rt foo bar ## Failed to set EFI variable (invalid parameter) Who will know that he has to specify both -bs and -rt here? As EFI_VARIABLE_RUNTIME_ACCESS always requires EFI_VARIABLE_BOOTSERVICE_ACCESS we do not need a -bs parameter. Just always set EFI_VARIABLE_BOOTSERVICE_ACCESS. printenv -e does not show the content of the variable. This was working before the patch. Best regards Heinrich > > -Takahiro Akashi > >> Best regards >> >> Heinrich >
Heinrich, Thank you for your review. On Sun, Oct 13, 2019 at 05:38:27PM +0200, Heinrich Schuchardt wrote: > On 10/8/19 2:05 AM, AKASHI Takahiro wrote: > >On Mon, Oct 07, 2019 at 06:45:08PM +0200, Heinrich Schuchardt wrote: > >>On 10/7/19 5:43 PM, Tom Rini wrote: > >>>On Mon, Oct 07, 2019 at 02:02:26PM +0900, AKASHI Takahiro wrote: > >>>>On Sun, Oct 06, 2019 at 09:42:30PM -0400, Tom Rini wrote: > >>>>>On Mon, Oct 07, 2019 at 09:47:46AM +0900, AKASHI Takahiro wrote: > >>>>>>On Sat, Oct 05, 2019 at 08:53:39AM +0200, Heinrich Schuchardt wrote: > >>>>>>>On 10/4/19 3:20 AM, AKASHI Takahiro wrote: > >>>>>>>>With this patch, when setting UEFI variable with "env set -e" command, > >>>>>>>>we will be able to > >>>>>>>>- specify vendor guid with "-guid guid", > >>>>>>>>- specify variable attributes, BOOTSERVICE_ACCESS, RUNTIME_ACCESS, > >>>>>>>> respectively with "-bs" and "-rt", > >>>>>>>>- append a value instead of overwriting with "-a", > >>>>>>>>- use memory as variable's value instead of explicit values given > >>>>>>>> at the command line with "-i address,size" > >>>>>>>> > >>>>>>>>If guid is not explicitly given, default value will be used. > >>>>>>>> > >>>>>>>>When "-at" is given, a variable should be authenticated with > >>>>>>>>appropriate signature database before setting or modifying its value. > >>>>>>>>(Authentication is not supported yet though.) > >>>>>>> > >>>>>>>Didn't you remove this in v2? > >>>>>> > >>>>>>It was an editorial error, which also existed in v2 commit message. > >>>>>> > >>>>>>>> > >>>>>>>>Meanwhile, "env print -e," will be modified so that it will dump > >>>>>>>>a variable's value only if '-v' (verbose) is specified. > >>>>>>>> > >>>>>>>>Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>>>>>> > >>>>>>>This does not work as expected: > >>>>>>> > >>>>>>>=> setenv -e -guid 0123456789abcdef0123456789abcdef -bs ARGONAUT hello world > >>>>>>>=> printenv -e > >>>>>>>OsIndicationsSupported: > >>>>>>> EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x8 > >>>>>>>PlatformLang: > >>>>>>> EFI_GLOBAL_VARIABLE_GUID: NV|BS|RT, DataSize = 0x6 > >>>>>>>PlatformLangCodes: > >>>>>>> EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x6 > >>>>>>>RuntimeServicesSupported: > >>>>>>> EFI_GLOBAL_VARIABLE_GUID: BS|RT, DataSize = 0x2 > >>>>>>> > >>>>>>>Neither do I see an error nor is the variable set. > >>>>>> > >>>>>>You have an incorrect guid format, so env_set_efi() should > >>>>>>return CMD_RET_USAGE, but mistakenly returns CMD_RET_FAILURE. > >>>>>> > >>>>>>>The patch increases the size of u-boot.bin by 2216 bytes for qemu-arm64. > >>>>>>>Tom is always asking me how we can stop the size increase in the UEFI > >>>>>>>sub-system. > >>>>>>> > >>>>>>>Why do we need this patch? This functionality is already available via > >>>>>>>the UEFI shell. > >>>>>> > >>>>>>Such a question should have been made before initially "-e" option > >>>>>>has been introduced a long time ago. > >>>>>> > >>>>>>* As far as we have "-e" option, this new patch is a natural extension. > >>>>>>* Why do we have to reply on external apps? We should have a minimum > >>>>>> self-contained command set to manage UEFI as part of U-Boot. > >>>>>>* "-i" is used in secure boot pytest. > >>>>> > >>>>>There is a difficult balance to find here. On the one hand, there is > >>>>>the argument (that I find compelling) which is that we want EFI loader > >>>>>support enabled, by default, on architectures where there is EFI support > >>>>>as it provides a consistent interface between ${random board} and ${off > >>>>>the shelf SW stack}. On the other hand, since this is a feature that's > >>>>>being enabled on a big majority of our platforms we need to be extremely > >>>>>wary of code size increases. > >>>>> > >>>>>So while I do think that some defconfigs (qemu* for example, and > >>>>>sandbox) should be fine to enable everything on (especially sandbox as > >>>>>there's where coverity runs), most configs should only get 'default y' > >>>>>via Kconfig for things required to load the common EFI applications. > >>>> > >>>>What is your definition of *common* EFI applications? > >>>>Do they include Shell.efi even on tight-resource platforms? > >>> > >>>I'm happy to get more feedback on this but my first pass is GRUB2 and > >>>*BSD loaders. Shell, SCT, etc are not. Specific platforms can enable > >>>whatever is needed there (even outside of qemu*/sandbox). And focusing > >>>on "tight resource platforms" is the wrong thing to focus on. Even on > >>>platforms where we have tons of space no one wants a loader that is > >>>larger than it needs to be. The bigger it is the longer it takes to > >>>read and relocate and get on with the business of running the system. > >>> > >>>>BTW, if you don't really like "env [set|print] -e" syntax, > >>>>we can move all the stuff to "efidebug" command. > >>>>This was the original place where I put them in my initial patch. > >>> > >>>I don't have a strong preference where this goes and will leave that to > >>>Heinrich but I do have preferences about binary size (and boot time > >>>increases) even when they seem small as they add up over time. Thanks > >>>for your understanding here, I do appreciate the time you've been > >>>investing here. > >>> > >> > >>With > >> > >>https://lists.denx.de/pipermail/u-boot/2019-October/385626.html > >>[PATCH 1/1] cmd: disable CMD_NVEDIT_EFI by default > >> > >>the code modified by this patch is disabled by default. > > > >So you have no reason to reject my patch, don't you? > > setenv -e -guid 12345678123456781234567812345678 foo bar > > neither sets a value nor gives an error. Please, fix this. Please make sure that you're now using v4. I fixed the issue in v4 as I mentioned in its change log. > Next I tried: > => setenv -e -guid 12345678-1234-5678-1234-567812345678 foo bar > ## Attributes (-bs or -rt) must be specified > > So I followed the advice given by the command: > > => setenv -e -guid 12345678-1234-5678-1234-567812345678 -rt foo bar > ## Failed to set EFI variable (invalid parameter) > > Who will know that he has to specify both -bs and -rt here? UEFI specification clearly says that. > As EFI_VARIABLE_RUNTIME_ACCESS always requires > EFI_VARIABLE_BOOTSERVICE_ACCESS we do not need a -bs parameter. Just > always set EFI_VARIABLE_BOOTSERVICE_ACCESS. Please note that I always try to mimic UEFI's Shell interfaces in my efi-related commands, either efidebug or env -e command, and "-bs" and "-rt" option do exist there. I admit, however, that the message is not quite friendly. Will improve it. > printenv -e does not show the content of the variable. This was working > before the patch. Please use "-v", which is also mentioned in the commit message. I introduced this option as dumping some variables, like PK/KEK/db, may overflow your screen. Thanks, -Takahiro Akashi > Best regards > > Heinrich > > > > >-Takahiro Akashi > > > >>Best regards > >> > >>Heinrich > > >
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 1cb0bc1460b9..1e542972db30 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -1387,7 +1387,7 @@ static char env_help_text[] = #endif "env print [-a | name ...] - print environment\n" #if defined(CONFIG_CMD_NVEDIT_EFI) - "env print -e [name ...] - print UEFI environment\n" + "env print -e [-v] [name ...] - print UEFI environment\n" #endif #if defined(CONFIG_CMD_RUN) "env run var [...] - run commands in an environment variable\n" @@ -1399,7 +1399,8 @@ static char env_help_text[] = #endif #endif #if defined(CONFIG_CMD_NVEDIT_EFI) - "env set -e name [arg ...] - set UEFI variable; unset if 'arg' not specified\n" + "env set -e [-nv][-bs][-rt][-a][-i addr,size][-v] name [arg ...]\n" + " - set UEFI variable; unset if '-i' or 'arg' not specified\n" #endif "env set [-f] name [arg ...]\n"; #endif @@ -1428,8 +1429,9 @@ U_BOOT_CMD_COMPLETE( "print environment variables", "[-a]\n - print [all] values of all environment variables\n" #if defined(CONFIG_CMD_NVEDIT_EFI) - "printenv -e [name ...]\n" + "printenv -e [-v] [name ...]\n" " - print UEFI variable 'name' or all the variables\n" + " \"-v\": verbose for signature database\n" #endif "printenv name ...\n" " - print value of environment variable 'name'", @@ -1459,9 +1461,16 @@ U_BOOT_CMD_COMPLETE( setenv, CONFIG_SYS_MAXARGS, 0, do_env_set, "set environment variables", #if defined(CONFIG_CMD_NVEDIT_EFI) - "-e [-nv] name [value ...]\n" + "-e [-guid guid][-nv][-bs][-rt][-a][-v]\n" + " [-i addr,size name], or [name [value ...]]\n" " - set UEFI variable 'name' to 'value' ...'\n" - " 'nv' option makes the variable non-volatile\n" + " \"-guid\": set vendor guid\n" + " \"-nv\": set non-volatile attribute\n" + " \"-bs\": set boot-service attribute\n" + " \"-rt\": set runtime attribute\n" + " \"-a\": append-write\n" + " \"-i addr,size\": use <addr,size> as variable's value\n" + " \"-v\": verbose print\n" " - delete UEFI variable 'name' if 'value' not specified\n" #endif "setenv [-f] name value ...\n" diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c index ed6d09a53046..795c95be0ef7 100644 --- a/cmd/nvedit_efi.c +++ b/cmd/nvedit_efi.c @@ -13,6 +13,7 @@ #include <exports.h> #include <hexdump.h> #include <malloc.h> +#include <mapmem.h> #include <linux/kernel.h> /* @@ -34,15 +35,48 @@ static const struct { {EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, "AT"}, }; +static const struct { + efi_guid_t guid; + char *text; +} efi_guid_text[] = { + /* signature database */ + {EFI_GLOBAL_VARIABLE_GUID, "EFI_GLOBAL_VARIABLE_GUID"}, +}; + +/* "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" */ +static char unknown_guid[37]; + +/** + * efi_guid_to_str() - convert guid to readable name + * + * @guid: GUID + * Return: string for GUID + * + * convert guid to readable name + */ +static const char *efi_guid_to_str(efi_guid_t *guid) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(efi_guid_text); i++) + if (!guidcmp(guid, &efi_guid_text[i].guid)) + return efi_guid_text[i].text; + + uuid_bin_to_str(guid->b, unknown_guid, UUID_STR_FORMAT_GUID); + + return unknown_guid; +} + /** * efi_dump_single_var() - show information about a UEFI variable * * @name: Name of the variable * @guid: Vendor GUID + * @verbose: if true, dump data * * Show information encoded in one UEFI variable */ -static void efi_dump_single_var(u16 *name, efi_guid_t *guid) +static void efi_dump_single_var(u16 *name, efi_guid_t *guid, bool verbose) { u32 attributes; u8 *data; @@ -68,7 +102,7 @@ static void efi_dump_single_var(u16 *name, efi_guid_t *guid) if (ret != EFI_SUCCESS) goto out; - printf("%ls:", name); + printf("%ls:\n %s:", name, efi_guid_to_str(guid)); for (count = 0, i = 0; i < ARRAY_SIZE(efi_var_attrs); i++) if (attributes & efi_var_attrs[i].mask) { if (count) @@ -79,7 +113,9 @@ static void efi_dump_single_var(u16 *name, efi_guid_t *guid) puts(efi_var_attrs[i].text); } printf(", DataSize = 0x%zx\n", size); - print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, data, size, true); + if (verbose) + print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, + data, size, true); out: free(data); @@ -90,11 +126,12 @@ out: * * @argc: Number of arguments (variables) * @argv: Argument (variable name) array + * @verbose: if true, dump data * Return: CMD_RET_SUCCESS on success, or CMD_RET_RET_FAILURE * * Show information encoded in named UEFI variables */ -static int efi_dump_vars(int argc, char * const argv[]) +static int efi_dump_vars(int argc, char * const argv[], bool verbose) { u16 *var_name16, *p; efi_uintn_t buf_size, size; @@ -120,7 +157,8 @@ static int efi_dump_vars(int argc, char * const argv[]) utf8_utf16_strcpy(&p, argv[0]); efi_dump_single_var(var_name16, - (efi_guid_t *)&efi_global_variable_guid); + (efi_guid_t *)&efi_global_variable_guid, + verbose); } free(var_name16); @@ -131,11 +169,12 @@ static int efi_dump_vars(int argc, char * const argv[]) /** * efi_dump_vars() - show information about all the UEFI variables * + * @verbose: if true, dump data * Return: CMD_RET_SUCCESS on success, or CMD_RET_RET_FAILURE * * Show information encoded in all the UEFI variables */ -static int efi_dump_var_all(void) +static int efi_dump_var_all(bool verbose) { u16 *var_name16, *p; efi_uintn_t buf_size, size; @@ -171,7 +210,7 @@ static int efi_dump_var_all(void) return CMD_RET_FAILURE; } - efi_dump_single_var(var_name16, &guid); + efi_dump_single_var(var_name16, &guid, verbose); } free(var_name16); @@ -189,12 +228,13 @@ static int efi_dump_var_all(void) * Return: CMD_RET_SUCCESS on success, or CMD_RET_RET_FAILURE * * This function is for "env print -e" or "printenv -e" command: - * => env print -e [var [...]] + * => env print -e [-v] [var [...]] * If one or more variable names are specified, show information * named UEFI variables, otherwise show all the UEFI variables. */ int do_env_print_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { + bool verbose; efi_status_t ret; /* Initialize EFI drivers */ @@ -205,12 +245,23 @@ int do_env_print_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return CMD_RET_FAILURE; } - if (argc > 1) + verbose = false; + for (argc--, argv++; argc > 0 && argv[0][0] == '-'; argc--, argv++) { + switch (argv[0][1]) { + case 'v': + verbose = true; + break; + default: + return CMD_RET_USAGE; + } + } + + if (argc) /* show specified UEFI variables */ - return efi_dump_vars(--argc, ++argv); + return efi_dump_vars(argc, argv, verbose); /* enumerate and show all UEFI variables */ - return efi_dump_var_all(); + return efi_dump_var_all(verbose); } /** @@ -339,18 +390,22 @@ out: * Return: CMD_RET_SUCCESS on success, or CMD_RET_RET_FAILURE * * This function is for "env set -e" or "setenv -e" command: - * => env set -e var [value ...]] + * => env set -e [-guid guid][-nv][-bs][-rt][-a][-v] + * [-i address,size] var, or + * var [value ...] * Encode values specified and set given UEFI variable. * If no value is specified, delete the variable. */ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { - char *var_name, *value = NULL; - efi_uintn_t size = 0; - u16 *var_name16 = NULL, *p; - size_t len; + char *var_name, *value, *ep; + ulong addr; + efi_uintn_t size; efi_guid_t guid; u32 attributes; + bool default_guid, verbose, value_on_memory; + u16 *var_name16 = NULL, *p; + size_t len; efi_status_t ret; if (argc == 1) @@ -364,32 +419,92 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return CMD_RET_FAILURE; } - attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | - EFI_VARIABLE_RUNTIME_ACCESS; - if (!strcmp(argv[1], "-nv")) { - attributes |= EFI_VARIABLE_NON_VOLATILE; - argc--; - argv++; - if (argc == 1) - return CMD_RET_SUCCESS; + /* + * attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | + * EFI_VARIABLE_RUNTIME_ACCESS; + */ + value = NULL; + size = 0; + attributes = 0; + guid = efi_global_variable_guid; + default_guid = true; + verbose = false; + value_on_memory = false; + for (argc--, argv++; argc > 0 && argv[0][0] == '-'; argc--, argv++) { + if (!strcmp(argv[0], "-guid")) { + if (argc == 1) + return CMD_RET_USAGE; + + argc--; + argv++; + if (uuid_str_to_bin(argv[0], guid.b, + UUID_STR_FORMAT_GUID)) + return CMD_RET_FAILURE; + default_guid = false; + } else if (!strcmp(argv[0], "-bs")) { + attributes |= EFI_VARIABLE_BOOTSERVICE_ACCESS; + } else if (!strcmp(argv[0], "-rt")) { + attributes |= EFI_VARIABLE_RUNTIME_ACCESS; + } else if (!strcmp(argv[0], "-nv")) { + attributes |= EFI_VARIABLE_NON_VOLATILE; + } else if (!strcmp(argv[0], "-a")) { + attributes |= EFI_VARIABLE_APPEND_WRITE; + } else if (!strcmp(argv[0], "-i")) { + /* data comes from memory */ + if (argc == 1) + return CMD_RET_USAGE; + + argc--; + argv++; + addr = simple_strtoul(argv[0], &ep, 16); + if (*ep != ',') + return CMD_RET_USAGE; + + size = simple_strtoul(++ep, NULL, 16); + if (!size) + return CMD_RET_FAILURE; + value_on_memory = true; + } else if (!strcmp(argv[0], "-v")) { + verbose = true; + } else { + return CMD_RET_USAGE; + } } + if (!argc) + return CMD_RET_USAGE; + + var_name = argv[0]; + if (default_guid) + guid = efi_global_variable_guid; - var_name = argv[1]; - if (argc == 2) { - /* delete */ - value = NULL; - size = 0; - } else { /* set */ - argc -= 2; - argv += 2; + if (verbose) { + printf("GUID: %s\n", efi_guid_to_str(&guid)); + printf("Attributes: 0x%x\n", attributes); + } - for ( ; argc > 0; argc--, argv++) + /* for value */ + if (value_on_memory) + value = map_sysmem(addr, 0); + else if (argc > 1) + for (argc--, argv++; argc > 0; argc--, argv++) if (append_value(&value, &size, argv[0]) < 0) { printf("## Failed to process an argument, %s\n", argv[0]); ret = CMD_RET_FAILURE; goto out; } + + if (size && !(attributes & (EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS))) { + printf("## Attributes (-bs or -rt) must be specified\n"); + ret = CMD_RET_FAILURE; + goto out; + } + + if (size && verbose) { + printf("Value:\n"); + print_hex_dump(" ", DUMP_PREFIX_OFFSET, + 16, 1, value, size, true); } len = utf8_utf16_strnlen(var_name, strlen(var_name)); @@ -402,17 +517,42 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) p = var_name16; utf8_utf16_strncpy(&p, var_name, len + 1); - guid = efi_global_variable_guid; ret = EFI_CALL(efi_set_variable(var_name16, &guid, attributes, size, value)); + unmap_sysmem(value); if (ret == EFI_SUCCESS) { ret = CMD_RET_SUCCESS; } else { - printf("## Failed to set EFI variable\n"); + const char *msg; + + switch (ret) { + case EFI_NOT_FOUND: + msg = " (not found)"; + break; + case EFI_WRITE_PROTECTED: + msg = " (read only)"; + break; + case EFI_INVALID_PARAMETER: + msg = " (invalid parameter)"; + break; + case EFI_SECURITY_VIOLATION: + msg = " (validation failed)"; + break; + case EFI_OUT_OF_RESOURCES: + msg = " (out of memory)"; + break; + default: + msg = ""; + break; + } + printf("## Failed to set EFI variable%s\n", msg); ret = CMD_RET_FAILURE; } out: - free(value); + if (value_on_memory) + unmap_sysmem(value); + else + free(value); free(var_name16); return ret;
With this patch, when setting UEFI variable with "env set -e" command, we will be able to - specify vendor guid with "-guid guid", - specify variable attributes, BOOTSERVICE_ACCESS, RUNTIME_ACCESS, respectively with "-bs" and "-rt", - append a value instead of overwriting with "-a", - use memory as variable's value instead of explicit values given at the command line with "-i address,size" If guid is not explicitly given, default value will be used. When "-at" is given, a variable should be authenticated with appropriate signature database before setting or modifying its value. (Authentication is not supported yet though.) Meanwhile, "env print -e," will be modified so that it will dump a variable's value only if '-v' (verbose) is specified. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- Changes in v3 (Oct 4, 2019) * add verbose messages when SetVariable() fails Changes in v2 (Sept 6, 2019) * remove "-at" option --- cmd/nvedit.c | 19 +++-- cmd/nvedit_efi.c | 212 +++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 190 insertions(+), 41 deletions(-)