[U-Boot] [PATCH v2 03/11] cmd: efidebug: rework "boot dump" sub-command using GetNextVariableName()
AKASHI Takahiro
takahiro.akashi at linaro.org
Thu Apr 25 00:30:37 UTC 2019
On Wed, Apr 24, 2019 at 10:13:37PM +0200, Heinrich Schuchardt wrote:
> On 4/24/19 8:30 AM, AKASHI Takahiro wrote:
> >Efidebug command should be implemented using well-defined EFI interfaces,
> >rather than using internal functions/data. This change will be needed in
> >a later patch where UEFI variables are re-implemented.
>
> I had trouble to get the point. In the next version I suggest to write:
>
> Currently in do_efi_boot_dump() we directly read EFI variables from the
> related environment variables. To accomodate alternative storage
> backends we should switch to using the UEFI API instead.
Okay.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> >---
> > cmd/efidebug.c | 92 ++++++++++++++++++++++++++++++++++++--------------
> > 1 file changed, 66 insertions(+), 26 deletions(-)
> >
> >diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> >index a40c4f4be286..8890dd7268f1 100644
> >--- a/cmd/efidebug.c
> >+++ b/cmd/efidebug.c
> >@@ -509,7 +509,7 @@ static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
> > if (argc < 6 || argc > 7)
> > return CMD_RET_USAGE;
> >
> >- id = (int)simple_strtoul(argv[1], &endp, 16);
> >+ id = simple_strtoul(argv[1], &endp, 16);
>
> This change is correct but unrelated. Please, put it into a separate
> patch. Or at least mention it in the commit message.
Sure.
> > if (*endp != '\0' || id > 0xffff)
> > return CMD_RET_USAGE;
> >
> >@@ -595,7 +595,7 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag,
> >
> > guid = efi_global_variable_guid;
> > for (i = 1; i < argc; i++, argv++) {
> >- id = (int)simple_strtoul(argv[1], &endp, 16);
> >+ id = simple_strtoul(argv[1], &endp, 16);
>
> Same here.
>
> > if (*endp != '\0' || id > 0xffff)
> > return CMD_RET_FAILURE;
> >
> >@@ -693,6 +693,27 @@ static void show_efi_boot_opt(int id)
> > free(data);
> > }
> >
> >+static bool u16_isxdigit(u16 c)
> >+{
> >+ if (c & 0xff00)
> >+ return false;
> >+
> >+ return isxdigit((u8)c);
> >+}
> >+
> >+static int u16_tohex(u16 c)
> >+{
> >+ if (c >= '0' && c < '9')
> >+ return c - '0';
> >+ if (c >= 'A' && c < 'F')
> >+ return c - 'A' + 10;
> >+ if (c >= 'a' && c < 'f')
> >+ return c - 'a' + 10;
>
> Does the UEFI spec really allow lower case hexadecimal numbers here?
> I only found an example with capital numbers. Would this imply that I
> could have variables Boot00a0 and Boot00A0 side by side? Which one would
> be selected by boot option 00a0?
>
> Or should SetVariable() make a case insensitive search for variable names?
Good point. I found the description below in UEFI section 3.1.1:
Each load option entry resides in a Boot####, Driver####,
SysPrep####, OsRecovery####
or PlatformRecovery#### variable where #### is replaced by
a unique option number in
printable hexadecimal representation using the digits 0-9,
and the upper case versions of the
characters A-F (0000-FFFF).
> In EDK2 function FindVariableEx() in
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> uses CompareMem() to compare variable names.
>
> Function BmCharToUint() is used to check the digits of boot options and
> has this comment:
>
> "It assumes the input Char is in the scope of L'0' ~ L'9'
> and L'A' ~ L'F'"
>
> So let's us assume that variable names are case sensitive and Boot
> entries can only have capital hexadecimal digits.
>
> So u16_isxdigit() is incorrect.
>
> >+
> >+ /* dummy */
> >+ return -1;
>
> As we do not check bounds here the function could be reduced to:
>
> return c > '9' ? c - ('A' - 0xa) : c - '9';
>
> But I would prefer one library function instead of the two static
> functions which returns -1 for a non-digit and 0 - 15 for a digit.
> And a unit test in test/unicoode_ut.c
Okay.
> >+}
> >+
> > /**
> > * show_efi_boot_dump() - dump all UEFI load options
> > *
> >@@ -709,38 +730,57 @@ static void show_efi_boot_opt(int id)
> > static int do_efi_boot_dump(cmd_tbl_t *cmdtp, int flag,
> > int argc, char * const argv[])
> > {
> >- char regex[256];
> >- char * const regexlist[] = {regex};
> >- char *variables = NULL, *boot, *value;
> >- int len;
> >- int id;
> >+ u16 *var_name16, *p;
> >+ efi_uintn_t buf_size, size;
> >+ efi_guid_t guid;
> >+ int id, i;
> >+ efi_status_t ret;
> >
> > if (argc > 1)
> > return CMD_RET_USAGE;
> >
> >- snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_Boot[0-9A-F]+");
> >-
> >- /* TODO: use GetNextVariableName? */
> >- len = hexport_r(&env_htab, '\n', H_MATCH_REGEX | H_MATCH_KEY,
> >- &variables, 0, 1, regexlist);
> >+ buf_size = 128;
> >+ var_name16 = malloc(buf_size);
> >+ if (!var_name16)
> >+ return CMD_RET_FAILURE;
> >
> >- if (!len)
> >- return CMD_RET_SUCCESS;
> >+ var_name16[0] = 0;
> >+ for (;;) {
> >+ size = buf_size;
> >+ ret = EFI_CALL(efi_get_next_variable_name(&size, var_name16,
> >+ &guid));
> >+ if (ret == EFI_NOT_FOUND)
> >+ break;
> >+ if (ret == EFI_BUFFER_TOO_SMALL) {
> >+ buf_size = size;
> >+ p = realloc(var_name16, buf_size);
> >+ if (!p) {
> >+ free(var_name16);
> >+ return CMD_RET_FAILURE;
> >+ }
> >+ var_name16 = p;
> >+ ret = EFI_CALL(efi_get_next_variable_name(&size,
> >+ var_name16,
> >+ &guid));
> >+ }
> >+ if (ret != EFI_SUCCESS) {
> >+ free(var_name16);
> >+ return CMD_RET_FAILURE;
> >+ }
> >
> >- if (len < 0)
> >- return CMD_RET_FAILURE;
> >+ if (u16_strncmp(var_name16, L"Boot", 4) || var_name16[8] ||
>
> We can use memcmp() here. This avoids introducing u16_strncmp().
Okay.
> >+ !u16_isxdigit(var_name16[4]) ||
> >+ !u16_isxdigit(var_name16[5]) ||
> >+ !u16_isxdigit(var_name16[6]) ||
> >+ !u16_isxdigit(var_name16[7]))
> >+ continue;
> >
> >- boot = variables;
> >- while (*boot) {
> >- value = strstr(boot, "Boot") + 4;
> >- id = (int)simple_strtoul(value, NULL, 16);
> >+ for (id = 0, i = 0; i < 4; i++)
> >+ id = (id << 4) + u16_tohex(var_name16[4 + i]);
>
> This all can be simplified if we unify u16_isxdigit() and u16_tohex().
> Just one function returning -1 if not a hexadecimal and 0 - 15 otherwise.
Will manage that.
> > show_efi_boot_opt(id);
> >- boot = strchr(boot, '\n');
> >- if (!*boot)
> >- break;
> >- boot++;
> > }
> >- free(variables);
> >+
> >+ free(var_name16);
> >
> > return CMD_RET_SUCCESS;
> > }
> >@@ -914,7 +954,7 @@ static int do_efi_boot_order(cmd_tbl_t *cmdtp, int flag,
> > return CMD_RET_FAILURE;
> >
> > for (i = 0; i < argc; i++) {
> >- id = (int)simple_strtoul(argv[i], &endp, 16);
> >+ id = simple_strtoul(argv[i], &endp, 16);
>
> This change is correct but unrelated. Please, put it into a separate
> patch. Or at least mention it in the commit message.
Okay
Thanks,
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> > if (*endp != '\0' || id > 0xffff) {
> > printf("invalid value: %s\n", argv[i]);
> > ret = CMD_RET_FAILURE;
> >
>
More information about the U-Boot
mailing list