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@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;