[U-Boot] [PATCH v6 0/7] cmd: add efidebug for efi environment

[U-Boot] [PATCH v6 0/7] cmd: add efidebug for efi environment

[AKASHI Takahiro]

This patch set is a collection of patches to enhance efi user interfaces
/commands. It will help improve user experience on efi boot and make it
more usable *without* edk2's shell utility.

Let's see how it works:
=> efidebug boot add 1 SHELL scsi 1:1 /Shell.efi ""
=> efidebug boot add 2 HELLO scsi 1:1 /hello.efi ""
=> efidebug boot dump
Boot0001:
	attributes: A-- (0x00000001)
	label: SHELL
	file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)/\\Shell.efi
	data: 
Boot0002:
	attributes: A-- (0x00000001)
	label: HELLO
	file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)/\\hello.efi
	data:

=> efidebug boot order 1 2
=> efidebug boot order
 1: Boot0001: SHELL
 2: Boot0002: HELLO

=> run -e Boot0002 (or bootefi bootmgr - 2)	; '-' means no dtb specified
WARNING: booting without device tree
Booting: HELLO
## Starting EFI application at 000000007db8b040 ...
Hello, world!
## Application terminated, r = 0

=> env set -e PlatformLang en			; important!
=> env print -e
Boot0001: BS|RT, DataSize = 0x79
    00000000: 01 00 00 00 66 00 53 00 48 00 45 00 4c 00 4c 00  ....f.S.H.E.L.L.
    00000010: 00 00 01 04 14 00 b9 73 1d e6 84 a3 cc 4a ae ab  .......s.....J..
    00000020: 82 e8 28 f3 62 8b 03 02 08 00 01 00 00 00 04 01  ..(.b...........
    00000030: 2a 00 01 00 00 00 00 08 00 00 00 00 00 00 00 00  *...............
    00000040: 04 00 00 00 00 00 ba 46 62 08 00 00 00 00 00 00  .......Fb.......
    00000050: 00 00 00 00 00 00 01 01 04 04 1c 00 5c 00 5c 00  ............\.\.
    00000060: 53 00 68 00 65 00 6c 00 6c 00 2e 00 65 00 66 00  S.h.e.l.l...e.f.
    00000070: 69 00 00 00 7f ff 04 00 00                       i........
Boot0002: BS|RT, DataSize = 0x79
    00000000: 01 00 00 00 66 00 48 00 45 00 4c 00 4c 00 4f 00  ....f.H.E.L.L.O.
    00000010: 00 00 01 04 14 00 b9 73 1d e6 84 a3 cc 4a ae ab  .......s.....J..
    00000020: 82 e8 28 f3 62 8b 03 02 08 00 01 00 00 00 04 01  ..(.b...........
    00000030: 2a 00 01 00 00 00 00 08 00 00 00 00 00 00 00 00  *...............
    00000040: 04 00 00 00 00 00 ba 46 62 08 00 00 00 00 00 00  .......Fb.......
    00000050: 00 00 00 00 00 00 01 01 04 04 1c 00 5c 00 5c 00  ............\.\.
    00000060: 68 00 65 00 6c 00 6c 00 6f 00 2e 00 65 00 66 00  h.e.l.l.o...e.f.
    00000070: 69 00 00 00 7f ff 04 00 00                       i........
BootOrder: BS|RT, DataSize = 0x4
    00000000: 01 00 02 00                                      ....
PlatformLang: BS|RT, DataSize = 0x2
    00000000: 65 6e                                            en

=> run -e Boot0001 or bootefi bootmgr

   (UEFI shell ...)

"env set" command now supports UEFI shell-like syntax:

=> env set -e foo =S\"akashi\" =0x012345 =Habcdef
=> env print -e foo
foo: BS|RT, DataSize = 0xd
    00000000: 61 6b 61 73 68 69 45 23 01 00 ab cd ef           akashiE#.....

Other useful sub commands are:
=> efidebug devices				; print uefi devices
=> efidebug drivers				; print uefi drivers
=> efidebug dh					; print uefi handles
=> efidebug images				; print loaded images
=> efidebug memmap				; dump uefi memory map

Enjoy!
-Takahiro Akashi

Changes in v6 (Jan 24, 2019)
* re-organize the code so that we can compile in "env [set|print] -e"
  support with or without CONFIG_CMD_EFIDEBUG
* remove "efidebug setvar" and "efidebug dumpvar" sub-commands
* re-implement "env print -e" using GetNextVariableName API
  and print_hex_dump()
* "env print -e" print more information, including attributes and size
* change a header format at "devices" and "drivers" sub-commands

Changes in v5 (Jan 21, 2019)
* rename the command name from efitool to efidebug again
* add it to MAINTAINERS
* follow a standard way of adding sub commands
* allow "env -e" syntax without enabling CONFIG_CMD_EFIDEBUG
  (merging v4's patch#8 into patch#1)
* change "_"-prefixed function names
* change some internal variables' names
* not print an efi driver's name at "efidebug drivers", yet showing
  a device path
* change protocol name strings from char to u16 at "efidebug dh"
* add a helper function to print efi memory attributes at "efidebug memmap"

Changes in v4 (Jan 15, 2019)
* rename the command name from efishell to efitool
* use efi_uintn_t for "size" if appropriate
* correct a help text for "boot add" sub-command
* "boot" sub-command always takes a hexadecimal number
* use systab.boottime directly instead of a local variable, bs
* fix a bug in "setvar" sub-command
* fix a bug in "devices" and "dh" sub-command
* fix a bug in "memmap" sub-command
* add "boot next" sub-command to set BootNext variable
* "drivers" sub-command prints more useful info, including a driver's
  name which originates from a corresponding u-boot efi driver
* "dh" sub-commands prints more useful info, including a list of
  protocols which are bound to a given handle

Changes in v3 (Dec 18, 2018)
* split v2 into two separate patch series
* add CONFIG_CMD_EFISHELL to enable/disable efishell command
* add missing free() at several places in efishell command

Changes in v2 (Nov 5, 2018)
* modify efi_dp_from_name() for use in efishell
* rename efi_marshal_load_option() to efi_serialize_load_option(),
  taking a "struct efi_load_option" as an argument
* improve a format in dumping uefi variables
* enhance a setvar syntax as efi's shell does
* change a syntax from "bootefi bootmgr -2" to "bootefi bootmgr - 2"
* add -e option to run command
* add -e option to env command
* add more sub-commands

AKASHI Takahiro (7):
  cmd: env: add "-e" option for handling UEFI variables
  cmd: add efidebug command
  cmd: efidebug: add devices command
  cmd: efidebug: add drivers command
  cmd: efidebug: add dh command
  cmd: efidebug: add images command
  cmd: efidebug: add memmap command

 MAINTAINERS       |   2 +
 cmd/Kconfig       |  16 +
 cmd/Makefile      |   2 +
 cmd/efidebug.c    | 869 ++++++++++++++++++++++++++++++++++++++++++++++
 cmd/nvedit.c      |  28 +-
 cmd/nvedit_efi.c  | 319 +++++++++++++++++
 include/command.h |   4 +
 7 files changed, 1239 insertions(+), 1 deletion(-)
 create mode 100644 cmd/efidebug.c
 create mode 100644 cmd/nvedit_efi.c

-- 
2.19.1

[U-Boot] [PATCH v6 1/7] cmd: env: add "-e" option for handling UEFI variables

[AKASHI Takahiro]

"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>
---
 MAINTAINERS       |   1 +
 cmd/Kconfig       |   6 +
 cmd/Makefile      |   1 +
 cmd/nvedit.c      |  28 +++-
 cmd/nvedit_efi.c  | 319 ++++++++++++++++++++++++++++++++++++++++++++++
 include/command.h |   4 +
 6 files changed, 358 insertions(+), 1 deletion(-)
 create mode 100644 cmd/nvedit_efi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ae825014bda9..22ac686ab2d6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -438,6 +438,7 @@ F:	lib/efi*/
 F:	test/py/tests/test_efi*
 F:	test/unicode_ut.c
 F:	cmd/bootefi.c
+F:	cmd/nvedit_efi.c
 F:	tools/file2include.c
 
 FPGA
diff --git a/cmd/Kconfig b/cmd/Kconfig
index ea1a325eb301..812a7eb9b74b 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -53,6 +53,12 @@ config SYS_PROMPT
 	  This string is displayed in the command line to the left of the
 	  cursor.
 
+config CMD_NVEDIT_EFI
+	bool
+	depends on EFI_LOADER
+	default y if EFI_LOADER
+	imply HEXDUMP
+
 menu "Autoboot options"
 
 config AUTOBOOT
diff --git a/cmd/Makefile b/cmd/Makefile
index 15ae4d250f50..142e0ee222ca 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -98,6 +98,7 @@ obj-$(CONFIG_CMD_MTD) += mtd.o
 obj-$(CONFIG_CMD_MTDPARTS) += mtdparts.o
 obj-$(CONFIG_CMD_NAND) += nand.o
 obj-$(CONFIG_CMD_NET) += net.o
+obj-$(CONFIG_CMD_NVEDIT_EFI) += nvedit_efi.o
 obj-$(CONFIG_CMD_ONENAND) += onenand.o
 obj-$(CONFIG_CMD_OSD) += osd.o
 obj-$(CONFIG_CMD_PART) += part.o
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index ce746bbf1b3e..7973d7add5fc 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -119,6 +119,11 @@ 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_NVEDIT_EFI)
+	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
+		return do_env_print_efi(cmdtp, flag, --argc, ++argv);
+#endif
+
 	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'a') {
 		argc--;
 		argv++;
@@ -216,6 +221,12 @@ 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_NVEDIT_EFI)
+	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
+		return do_env_set_efi(NULL, flag, --argc, ++argv);
+#endif
+
 	while (argc > 1 && **(argv + 1) == '-') {
 		char *arg = *++argv;
 
@@ -1263,14 +1274,21 @@ static char env_help_text[] =
 	"env import [-d] [-t [-r] | -b | -c] addr [size] [var ...] - import environment\n"
 #endif
 	"env print [-a | name ...] - print environment\n"
+#if defined(CONFIG_CMD_NVEDIT_EFI)
+	"env print  -e [name ...] - print UEFI 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_NVEDIT_EFI)
+	"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 +1313,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_NVEDIT_EFI)
+	"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 +1344,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_NVEDIT_EFI)
+	"-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'",
diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
new file mode 100644
index 000000000000..58c92009e8db
--- /dev/null
+++ b/cmd/nvedit_efi.c
@@ -0,0 +1,319 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  Integrate UEFI variables to u-boot env interface
+ *
+ *  Copyright (c) 2018 AKASHI Takahiro, Linaro Limited
+ */
+
+#include <charset.h>
+#include <common.h>
+#include <command.h>
+#include <efi_loader.h>
+#include <exports.h>
+#include <hexdump.h>
+#include <malloc.h>
+#include <linux/kernel.h>
+
+static const struct {
+	u32 mask;
+	char *text;
+} efi_var_attrs[] = {
+	{EFI_VARIABLE_NON_VOLATILE, "NV"},
+	{EFI_VARIABLE_BOOTSERVICE_ACCESS, "BS"},
+	{EFI_VARIABLE_RUNTIME_ACCESS, "RT"},
+	{EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS, "AW"},
+	{EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, "AT"},
+};
+
+static int print_variable_info(u16 *name, efi_guid_t *guid)
+{
+	u32 attributes;
+	u8 *data;
+	efi_uintn_t size;
+	int count, i;
+	efi_status_t ret;
+
+	data = NULL;
+	size = 0;
+	ret = efi_get_variable(name, guid, &attributes, &size, data);
+	if (ret == EFI_BUFFER_TOO_SMALL) {
+		data = malloc(size);
+		if (!data)
+			return -1;
+
+		ret = efi_get_variable(name, guid, &attributes, &size, data);
+	}
+	if (ret == EFI_NOT_FOUND) {
+		printf("Error: \"%ls\" not defined\n", name);
+		goto out;
+	}
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	printf("%ls:", name);
+	for (count = 0, i = 0; i < ARRAY_SIZE(efi_var_attrs); i++)
+		if (attributes & efi_var_attrs[i].mask) {
+			if (count)
+				putc('|');
+			else
+				putc(' ');
+			count++;
+			puts(efi_var_attrs[i].text);
+		}
+	printf(", DataSize = 0x%lx\n", size);
+	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1, data, size, true);
+
+	return 0;
+out:
+	free(data);
+	return -1;
+}
+
+/*
+ * From efi_variable.c,
+ *
+ * Mapping between UEFI variables and u-boot variables:
+ *
+ *   efi_$guid_$varname = {attributes}(type)value
+ */
+static int efi_dump_var(cmd_tbl_t *cmdtp, int flag,
+			int argc, char * const argv[])
+{
+	u16 *var_name16, *p;
+	efi_uintn_t buf_size, size;
+	efi_guid_t guid;
+	efi_status_t ret;
+
+	buf_size = 128;
+	var_name16 = malloc(buf_size);
+	if (!var_name16)
+		return CMD_RET_FAILURE;
+
+	if (argc > 1) {
+		argc--;
+		argv++;
+
+		for (; argc > 0; argc--, argv++) {
+			size = (strlen(argv[0]) + 1) * 2;
+			if (buf_size < size) {
+				buf_size = size;
+				p = realloc(var_name16, buf_size);
+				if (!p) {
+					free(var_name16);
+					return CMD_RET_FAILURE;
+				}
+				var_name16 = p;
+			}
+
+			p = var_name16;
+			utf8_utf16_strcpy(&p, argv[0]);
+			guid = efi_global_variable_guid;
+
+			print_variable_info(var_name16, &guid);
+		}
+
+		free(var_name16);
+
+		return CMD_RET_SUCCESS;
+	}
+
+	var_name16[0] = 0;
+	for (;;) {
+		size = buf_size;
+		ret = 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_get_next_variable_name(&size, var_name16,
+							 &guid);
+		}
+		if (ret != EFI_SUCCESS) {
+			free(var_name16);
+			return CMD_RET_FAILURE;
+		}
+
+		print_variable_info(var_name16, &guid);
+	}
+
+	free(var_name16);
+
+	return CMD_RET_SUCCESS;
+}
+
+int do_env_print_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	efi_status_t r;
+
+	/* Initialize EFI drivers */
+	r = efi_init_obj_list();
+	if (r != EFI_SUCCESS) {
+		printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
+		       r & ~EFI_ERROR_MASK);
+		return CMD_RET_FAILURE;
+	}
+
+	return efi_dump_var(cmdtp, flag, argc, argv);
+}
+
+static int append_value(char **bufp, unsigned long *sizep, char *data)
+{
+	char *tmp_buf = NULL, *new_buf = NULL, *value;
+	unsigned long len = 0;
+
+	if (!strncmp(data, "=0x", 2)) { /* hexadecimal number */
+		union {
+			u8 u8;
+			u16 u16;
+			u32 u32;
+			u64 u64;
+		} tmp_data;
+		unsigned long hex_value;
+		void *hex_ptr;
+
+		data += 3;
+		len = strlen(data);
+		if ((len & 0x1)) /* not multiple of two */
+			return -1;
+
+		len /= 2;
+		if (len > 8)
+			return -1;
+		else if (len > 4)
+			len = 8;
+		else if (len > 2)
+			len = 4;
+
+		/* convert hex hexadecimal number */
+		if (strict_strtoul(data, 16, &hex_value) < 0)
+			return -1;
+
+		tmp_buf = malloc(len);
+		if (!tmp_buf)
+			return -1;
+
+		if (len == 1) {
+			tmp_data.u8 = hex_value;
+			hex_ptr = &tmp_data.u8;
+		} else if (len == 2) {
+			tmp_data.u16 = hex_value;
+			hex_ptr = &tmp_data.u16;
+		} else if (len == 4) {
+			tmp_data.u32 = hex_value;
+			hex_ptr = &tmp_data.u32;
+		} else {
+			tmp_data.u64 = hex_value;
+			hex_ptr = &tmp_data.u64;
+		}
+		memcpy(tmp_buf, hex_ptr, len);
+		value = tmp_buf;
+
+	} else if (!strncmp(data, "=H", 2)) { /* hexadecimal-byte array */
+		data += 2;
+		len = strlen(data);
+		if (len & 0x1) /* not multiple of two */
+			return -1;
+
+		len /= 2;
+		tmp_buf = malloc(len);
+		if (!tmp_buf)
+			return -1;
+
+		if (hex2bin((u8 *)tmp_buf, data, len) < 0)
+			return -1;
+
+		value = tmp_buf;
+	} else { /* string */
+		if (!strncmp(data, "=\"", 2) || !strncmp(data, "=S\"", 3)) {
+			if (data[1] == '"')
+				data += 2;
+			else
+				data += 3;
+			value = data;
+			len = strlen(data) - 1;
+			if (data[len] != '"')
+				return -1;
+		} else {
+			value = data;
+			len = strlen(data);
+		}
+	}
+
+	new_buf = realloc(*bufp, *sizep + len);
+	if (!new_buf)
+		goto out;
+
+	memcpy(new_buf + *sizep, value, len);
+	*bufp = new_buf;
+	*sizep += len;
+
+out:
+	free(tmp_buf);
+
+	return 0;
+}
+
+static int efi_set_var(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;
+	efi_guid_t guid;
+	efi_status_t ret;
+
+	if (argc == 1)
+		return CMD_RET_SUCCESS;
+
+	var_name = argv[1];
+	if (argc == 2) {
+		/* remove */
+		value = NULL;
+		size = 0;
+	} else { /* set */
+		argc -= 2;
+		argv += 2;
+
+		for ( ; argc > 0; argc--, argv++)
+			if (append_value(&value, &size, argv[0]) < 0) {
+				ret = CMD_RET_FAILURE;
+				goto out;
+			}
+	}
+
+	var_name16 = malloc((strlen(var_name) + 1) * 2);
+	p = var_name16;
+	utf8_utf16_strncpy(&p, var_name, strlen(var_name) + 1);
+
+	guid = efi_global_variable_guid;
+	ret = efi_set_variable(var_name16, &guid,
+			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
+			       EFI_VARIABLE_RUNTIME_ACCESS, size, value);
+	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
+out:
+	free(value);
+	free(var_name16);
+
+	return ret;
+}
+
+int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	efi_status_t r;
+
+	/* Initialize EFI drivers */
+	r = efi_init_obj_list();
+	if (r != EFI_SUCCESS) {
+		printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
+		       r & ~EFI_ERROR_MASK);
+		return CMD_RET_FAILURE;
+	}
+
+	return efi_set_var(cmdtp, flag, argc, argv);
+}
diff --git a/include/command.h b/include/command.h
index feddef300ccc..efc4dfc457c1 100644
--- a/include/command.h
+++ b/include/command.h
@@ -51,6 +51,10 @@ extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 #if defined(CONFIG_CMD_BOOTEFI)
 extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 #endif
+#if defined(CONFIG_CMD_NVEDIT_EFI)
+int do_env_print_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
+int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
+#endif
 
 /* common/command.c */
 int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
-- 
2.19.1

[U-Boot] [PATCH v6 2/7] cmd: add efidebug command

[AKASHI Takahiro]

Currently, there is no easy way to add or modify UEFI variables.
In particular, bootmgr supports BootOrder/BootXXXX variables, it is
quite hard to define them as u-boot variables because they are represented
in a complicated and encoded format.

The new command, efidebug, helps address these issues and give us
more friendly interfaces:
 * efidebug boot add: add BootXXXX variable
 * efidebug boot rm: remove BootXXXX variable
 * efidebug boot dump: display all BootXXXX variables
 * efidebug boot next: set BootNext variable
 * efidebug boot order: set/display a boot order (BootOrder)

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 MAINTAINERS    |   1 +
 cmd/Kconfig    |  10 ++
 cmd/Makefile   |   1 +
 cmd/efidebug.c | 461 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 473 insertions(+)
 create mode 100644 cmd/efidebug.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 22ac686ab2d6..7ad8c01a3b93 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -438,6 +438,7 @@ F:	lib/efi*/
 F:	test/py/tests/test_efi*
 F:	test/unicode_ut.c
 F:	cmd/bootefi.c
+F:	cmd/efidebug.c
 F:	cmd/nvedit_efi.c
 F:	tools/file2include.c
 
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 812a7eb9b74b..f0405a490635 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1403,6 +1403,16 @@ config CMD_DISPLAY
 	  displayed on a simple board-specific display. Implement
 	  display_putc() to use it.
 
+config CMD_EFIDEBUG
+	bool "efidebug - display/configure UEFI environment"
+	depends on EFI_LOADER
+	default n
+	help
+	  Enable the 'efidebug' command which provides a subset of UEFI
+	  shell utility with simplified functionality. It will be useful
+	  particularly for managing boot parameters as  well as examining
+	  various EFI status for debugging.
+
 config CMD_LED
 	bool "led"
 	default y if LED
diff --git a/cmd/Makefile b/cmd/Makefile
index 142e0ee222ca..48f2742168be 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_CMD_ECHO) += echo.o
 obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o
 obj-$(CONFIG_CMD_EEPROM) += eeprom.o
 obj-$(CONFIG_EFI_STUB) += efi.o
+obj-$(CONFIG_CMD_EFIDEBUG) += efidebug.o
 obj-$(CONFIG_CMD_ELF) += elf.o
 obj-$(CONFIG_HUSH_PARSER) += exit.o
 obj-$(CONFIG_CMD_EXT4) += ext4.o
diff --git a/cmd/efidebug.c b/cmd/efidebug.c
new file mode 100644
index 000000000000..d836576cf6e0
--- /dev/null
+++ b/cmd/efidebug.c
@@ -0,0 +1,461 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  UEFI Shell-like command
+ *
+ *  Copyright (c) 2018 AKASHI Takahiro, Linaro Limited
+ */
+
+#include <charset.h>
+#include <common.h>
+#include <command.h>
+#include <efi_loader.h>
+#include <environment.h>
+#include <exports.h>
+#include <malloc.h>
+#include <search.h>
+#include <linux/ctype.h>
+
+static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
+			   int argc, char * const argv[])
+{
+	int id;
+	char *endp;
+	char var_name[9];
+	u16 var_name16[9], *p;
+	efi_guid_t guid;
+	size_t label_len, label_len16;
+	u16 *label;
+	struct efi_device_path *device_path = NULL, *file_path = NULL;
+	struct efi_load_option lo;
+	void *data = NULL;
+	efi_uintn_t size;
+	int ret;
+
+	if (argc < 6 || argc > 7)
+		return CMD_RET_USAGE;
+
+	id = (int)simple_strtoul(argv[1], &endp, 16);
+	if (*endp != '\0' || id > 0xffff)
+		return CMD_RET_FAILURE;
+
+	sprintf(var_name, "Boot%04X", id);
+	p = var_name16;
+	utf8_utf16_strncpy(&p, var_name, 9);
+
+	guid = efi_global_variable_guid;
+
+	/* attributes */
+	lo.attributes = 0x1; /* always ACTIVE */
+
+	/* label */
+	label_len = strlen(argv[2]);
+	label_len16 = utf8_utf16_strnlen(argv[2], label_len);
+	label = malloc((label_len16 + 1) * sizeof(u16));
+	if (!label)
+		return CMD_RET_FAILURE;
+	lo.label = label; /* label will be changed below */
+	utf8_utf16_strncpy(&label, argv[2], label_len);
+
+	/* file path */
+	ret = efi_dp_from_name(argv[3], argv[4], argv[5], &device_path,
+			       &file_path);
+	if (ret != EFI_SUCCESS) {
+		ret = CMD_RET_FAILURE;
+		goto out;
+	}
+	lo.file_path = file_path;
+	lo.file_path_length = efi_dp_size(file_path)
+				+ sizeof(struct efi_device_path); /* for END */
+
+	/* optional data */
+	lo.optional_data = (u8 *)(argc == 6 ? "" : argv[6]);
+
+	size = efi_serialize_load_option(&lo, (u8 **)&data);
+	if (!size) {
+		ret = CMD_RET_FAILURE;
+		goto out;
+	}
+
+	ret = efi_set_variable(var_name16, &guid,
+			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
+			       EFI_VARIABLE_RUNTIME_ACCESS, size, data);
+	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
+out:
+	free(data);
+	efi_free_pool(device_path);
+	efi_free_pool(file_path);
+	free(lo.label);
+
+	return ret;
+}
+
+static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag,
+			  int argc, char * const argv[])
+{
+	efi_guid_t guid;
+	int id, i;
+	char *endp;
+	char var_name[9];
+	u16 var_name16[9];
+	efi_status_t ret;
+
+	if (argc == 1)
+		return CMD_RET_USAGE;
+
+	guid = efi_global_variable_guid;
+	for (i = 1; i < argc; i++, argv++) {
+		id = (int)simple_strtoul(argv[1], &endp, 16);
+		if (*endp != '\0' || id > 0xffff)
+			return CMD_RET_FAILURE;
+
+		sprintf(var_name, "Boot%04X", id);
+		utf8_utf16_strncpy((u16 **)&var_name16, var_name, 9);
+
+		ret = efi_set_variable(var_name16, &guid, 0, 0, NULL);
+		if (ret) {
+			printf("cannot remove Boot%04X", id);
+			return CMD_RET_FAILURE;
+		}
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static void show_efi_boot_opt_data(int id, void *data)
+{
+	struct efi_load_option lo;
+	char *label, *p;
+	size_t label_len16, label_len;
+	u16 *dp_str;
+
+	efi_deserialize_load_option(&lo, data);
+
+	label_len16 = u16_strlen(lo.label);
+	label_len = utf16_utf8_strnlen(lo.label, label_len16);
+	label = malloc(label_len + 1);
+	if (!label)
+		return;
+	p = label;
+	utf16_utf8_strncpy(&p, lo.label, label_len16);
+
+	printf("Boot%04X:\n", id);
+	printf("\tattributes: %c%c%c (0x%08x)\n",
+	       /* ACTIVE */
+	       lo.attributes & 0x1 ? 'A' : '-',
+	       /* FORCE RECONNECT */
+	       lo.attributes & 0x2 ? 'R' : '-',
+	       /* HIDDEN */
+	       lo.attributes & 0x8 ? 'H' : '-',
+	       lo.attributes);
+	printf("\tlabel: %s\n", label);
+
+	dp_str = efi_dp_str(lo.file_path);
+	printf("\tfile_path: %ls\n", dp_str);
+	efi_free_pool(dp_str);
+
+	printf("\tdata: %s\n", lo.optional_data);
+
+	free(label);
+}
+
+static void show_efi_boot_opt(int id)
+{
+	char var_name[9];
+	u16 var_name16[9], *p;
+	efi_guid_t guid;
+	void *data = NULL;
+	efi_uintn_t size;
+	int ret;
+
+	sprintf(var_name, "Boot%04X", id);
+	p = var_name16;
+	utf8_utf16_strncpy(&p, var_name, 9);
+	guid = efi_global_variable_guid;
+
+	size = 0;
+	ret = efi_get_variable(var_name16, &guid, NULL, &size, NULL);
+	if (ret == (int)EFI_BUFFER_TOO_SMALL) {
+		data = malloc(size);
+		ret = efi_get_variable(var_name16, &guid, NULL, &size, data);
+	}
+	if (ret == EFI_SUCCESS)
+		show_efi_boot_opt_data(id, data);
+	else if (ret == EFI_NOT_FOUND)
+		printf("Boot%04X: not found\n", id);
+
+	free(data);
+}
+
+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;
+
+	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);
+
+	if (!len)
+		return CMD_RET_SUCCESS;
+
+	if (len < 0)
+		return CMD_RET_FAILURE;
+
+	boot = variables;
+	while (*boot) {
+		value = strstr(boot, "Boot") + 4;
+		id = (int)simple_strtoul(value, NULL, 16);
+		show_efi_boot_opt(id);
+		boot = strchr(boot, '\n');
+		if (!*boot)
+			break;
+		boot++;
+	}
+	free(variables);
+
+	return CMD_RET_SUCCESS;
+}
+
+static int show_efi_boot_order(void)
+{
+	efi_guid_t guid;
+	u16 *bootorder = NULL;
+	efi_uintn_t size;
+	int num, i;
+	char var_name[9];
+	u16 var_name16[9], *p16;
+	void *data;
+	struct efi_load_option lo;
+	char *label, *p;
+	size_t label_len16, label_len;
+	efi_status_t ret;
+
+	guid = efi_global_variable_guid;
+	size = 0;
+	ret = efi_get_variable(L"BootOrder", &guid, NULL, &size, NULL);
+	if (ret == EFI_BUFFER_TOO_SMALL) {
+		bootorder = malloc(size);
+		ret = efi_get_variable(L"BootOrder", &guid, NULL, &size,
+				       bootorder);
+	}
+	if (ret == EFI_NOT_FOUND) {
+		printf("BootOrder not defined\n");
+		ret = CMD_RET_SUCCESS;
+		goto out;
+	} else if (ret != EFI_SUCCESS) {
+		ret = CMD_RET_FAILURE;
+		goto out;
+	}
+
+	num = size / sizeof(u16);
+	for (i = 0; i < num; i++) {
+		sprintf(var_name, "Boot%04X", bootorder[i]);
+		p16 = var_name16;
+		utf8_utf16_strncpy(&p16, var_name, 9);
+
+		size = 0;
+		ret = efi_get_variable(var_name16, &guid, NULL, &size, NULL);
+		if (ret != EFI_BUFFER_TOO_SMALL) {
+			printf("%2d: Boot%04X: (not defined)\n",
+			       i + 1, bootorder[i]);
+			continue;
+		}
+
+		data = malloc(size);
+		if (!data) {
+			ret = CMD_RET_FAILURE;
+			goto out;
+		}
+		ret = efi_get_variable(var_name16, &guid, NULL, &size, data);
+		if (ret != EFI_SUCCESS) {
+			free(data);
+			ret = CMD_RET_FAILURE;
+			goto out;
+		}
+
+		efi_deserialize_load_option(&lo, data);
+
+		label_len16 = u16_strlen(lo.label);
+		label_len = utf16_utf8_strnlen(lo.label, label_len16);
+		label = malloc(label_len + 1);
+		if (!label) {
+			free(data);
+			ret = CMD_RET_FAILURE;
+			goto out;
+		}
+		p = label;
+		utf16_utf8_strncpy(&p, lo.label, label_len16);
+		printf("%2d: Boot%04X: %s\n", i + 1, bootorder[i], label);
+		free(label);
+
+		free(data);
+	}
+out:
+	free(bootorder);
+
+	return ret;
+}
+
+static int do_efi_boot_next(cmd_tbl_t *cmdtp, int flag,
+			    int argc, char * const argv[])
+{
+	u16 bootnext;
+	efi_uintn_t size;
+	char *endp;
+	efi_guid_t guid;
+	efi_status_t ret;
+
+	if (argc != 2)
+		return CMD_RET_USAGE;
+
+	bootnext = (u16)simple_strtoul(argv[1], &endp, 16);
+	if (*endp != '\0' || bootnext > 0xffff) {
+		printf("invalid value: %s\n", argv[1]);
+		ret = CMD_RET_FAILURE;
+		goto out;
+	}
+
+	guid = efi_global_variable_guid;
+	size = sizeof(u16);
+	ret = efi_set_variable(L"BootNext", &guid,
+			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
+			       EFI_VARIABLE_RUNTIME_ACCESS, size, &bootnext);
+	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
+out:
+	return ret;
+}
+
+static int do_efi_boot_order(cmd_tbl_t *cmdtp, int flag,
+			     int argc, char * const argv[])
+{
+	u16 *bootorder = NULL;
+	efi_uintn_t size;
+	int id, i;
+	char *endp;
+	efi_guid_t guid;
+	efi_status_t ret;
+
+	if (argc == 1)
+		return show_efi_boot_order();
+
+	argc--;
+	argv++;
+
+	size = argc * sizeof(u16);
+	bootorder = malloc(size);
+	if (!bootorder)
+		return CMD_RET_FAILURE;
+
+	for (i = 0; i < argc; i++) {
+		id = (int)simple_strtoul(argv[i], &endp, 16);
+		if (*endp != '\0' || id > 0xffff) {
+			printf("invalid value: %s\n", argv[i]);
+			ret = CMD_RET_FAILURE;
+			goto out;
+		}
+
+		bootorder[i] = (u16)id;
+	}
+
+	guid = efi_global_variable_guid;
+	ret = efi_set_variable(L"BootOrder", &guid,
+			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
+			       EFI_VARIABLE_RUNTIME_ACCESS, size, bootorder);
+	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
+out:
+	free(bootorder);
+
+	return ret;
+}
+
+static cmd_tbl_t cmd_efidebug_boot_sub[] = {
+	U_BOOT_CMD_MKENT(add, CONFIG_SYS_MAXARGS, 1, do_efi_boot_add, "", ""),
+	U_BOOT_CMD_MKENT(rm, CONFIG_SYS_MAXARGS, 1, do_efi_boot_rm, "", ""),
+	U_BOOT_CMD_MKENT(dump, CONFIG_SYS_MAXARGS, 1, do_efi_boot_dump, "", ""),
+	U_BOOT_CMD_MKENT(next, CONFIG_SYS_MAXARGS, 1, do_efi_boot_next, "", ""),
+	U_BOOT_CMD_MKENT(order, CONFIG_SYS_MAXARGS, 1, do_efi_boot_order,
+			 "", ""),
+};
+
+static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
+			   int argc, char * const argv[])
+{
+	cmd_tbl_t *cp;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	argc--; argv++;
+
+	cp = find_cmd_tbl(argv[0], cmd_efidebug_boot_sub,
+			  ARRAY_SIZE(cmd_efidebug_boot_sub));
+	if (!cp)
+		return CMD_RET_USAGE;
+
+	return cp->cmd(cmdtp, flag, argc, argv);
+}
+
+static cmd_tbl_t cmd_efidebug_sub[] = {
+	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
+};
+
+/* Interpreter command to configure UEFI environment */
+static int do_efidebug(cmd_tbl_t *cmdtp, int flag,
+		       int argc, char * const argv[])
+{
+	cmd_tbl_t *cp;
+	efi_status_t r;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	argc--; argv++;
+
+	/* Initialize UEFI drivers */
+	r = efi_init_obj_list();
+	if (r != EFI_SUCCESS) {
+		printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
+		       r & ~EFI_ERROR_MASK);
+		return CMD_RET_FAILURE;
+	}
+
+	cp = find_cmd_tbl(argv[0], cmd_efidebug_sub,
+			  ARRAY_SIZE(cmd_efidebug_sub));
+	if (!cp)
+		return CMD_RET_USAGE;
+
+	return cp->cmd(cmdtp, flag, argc, argv);
+}
+
+#ifdef CONFIG_SYS_LONGHELP
+static char efidebug_help_text[] =
+	"  - UEFI Shell-like interface to configure UEFI environment\n"
+	"\n"
+	"efidebug boot add <bootid> <label> <interface> <device>[:<part>] <file path> [<load options>]\n"
+	"  - set UEFI BootXXXX variable\n"
+	"    <load options> will be passed to UEFI application\n"
+	"efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
+	"  - delete UEFI BootXXXX variables\n"
+	"efidebug boot dump\n"
+	"  - show all UEFI BootXXXX variables\n"
+	"efidebug boot next <bootid>\n"
+	"  - set UEFI BootNext variable\n"
+	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
+	"  - set/show UEFI boot order\n"
+	"\n";
+#endif
+
+U_BOOT_CMD(
+	efidebug, 10, 0, do_efidebug,
+	"Configure UEFI environment",
+	efidebug_help_text
+);
-- 
2.19.1

[U-Boot] [PATCH v6 3/7] cmd: efidebug: add devices command

[AKASHI Takahiro]

"devices" command prints all the uefi variables on the system.

=> efi devices
Scanning disk ahci_scsi.id0lun0...
Scanning disk ahci_scsi.id1lun0...
Found 4 disks
Device           Device Path
================ ====================
000000007ef07ea0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
000000007ef00c10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
000000007ef00dd0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
000000007ef07be0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
000000007ef07510 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/efidebug.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 101 insertions(+), 1 deletion(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index d836576cf6e0..8a7f775b117a 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -15,6 +15,102 @@
 #include <search.h>
 #include <linux/ctype.h>
 
+#define BS systab.boottime
+
+static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
+				    int *num)
+{
+	efi_handle_t *handles = NULL;
+	efi_uintn_t size = 0;
+	efi_status_t ret;
+
+	if (guid) {
+		ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
+					handles);
+		if (ret == EFI_BUFFER_TOO_SMALL) {
+			handles = calloc(1, size);
+			if (!handles)
+				return -1;
+
+			ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
+						handles);
+		}
+		if (ret != EFI_SUCCESS) {
+			free(handles);
+			return -1;
+		}
+	} else {
+		ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
+		if (ret == EFI_BUFFER_TOO_SMALL) {
+			handles = calloc(1, size);
+			if (!handles)
+				return -1;
+
+			ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size,
+						handles);
+		}
+		if (ret != EFI_SUCCESS) {
+			free(handles);
+			return -1;
+		}
+	}
+
+	*handlesp = handles;
+	*num = size / sizeof(efi_handle_t);
+
+	return 0;
+}
+
+static int efi_get_device_handle_info(efi_handle_t handle, u16 **dev_path_text)
+{
+	struct efi_device_path *dp;
+	efi_status_t ret;
+
+	ret = BS->open_protocol(handle, &efi_guid_device_path,
+				(void **)&dp, NULL /* FIXME */, NULL,
+				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+	if (ret == EFI_SUCCESS) {
+		*dev_path_text = efi_dp_str(dp);
+		return 0;
+	} else {
+		return -1;
+	}
+}
+
+#define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
+
+static const char spc[] = "                ";
+static const char sep[] = "================";
+
+static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
+			       int argc, char * const argv[])
+{
+	efi_handle_t *handles;
+	u16 *dev_path_text;
+	int num, i;
+
+	handles = NULL;
+	num = 0;
+	if (efi_get_handles_by_proto(NULL, &handles, &num))
+		return CMD_RET_FAILURE;
+
+	if (!num)
+		return CMD_RET_SUCCESS;
+
+	printf("Device%.*s Device Path\n", EFI_HANDLE_WIDTH - 6, spc);
+	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
+	for (i = 0; i < num; i++) {
+		if (!efi_get_device_handle_info(handles[i], &dev_path_text)) {
+			printf("%p %ls\n", handles[i], dev_path_text);
+			efi_free_pool(dev_path_text);
+		}
+	}
+
+	free(handles);
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
 			   int argc, char * const argv[])
 {
@@ -406,6 +502,8 @@ static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
 
 static cmd_tbl_t cmd_efidebug_sub[] = {
 	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
+	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
+			 "", ""),
 };
 
 /* Interpreter command to configure UEFI environment */
@@ -451,7 +549,9 @@ static char efidebug_help_text[] =
 	"  - set UEFI BootNext variable\n"
 	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
 	"  - set/show UEFI boot order\n"
-	"\n";
+	"\n"
+	"efidebug devices\n"
+	"  - show uefi devices\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

[U-Boot] [PATCH v6 4/7] cmd: efidebug: add drivers command

[AKASHI Takahiro]

"drivers" command prints all the uefi drivers on the system.

=> efi drivers
Driver           Name                 Image Path
================ ==================== ====================
000000007ef003d0 <NULL>               <built-in>

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/efidebug.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 79 insertions(+), 1 deletion(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 8a7f775b117a..1b788c76a895 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -111,6 +111,80 @@ static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
 	return CMD_RET_SUCCESS;
 }
 
+static int efi_get_driver_handle_info(efi_handle_t handle, u16 **driver_name,
+				      u16 **image_path)
+{
+	struct efi_handler *handler;
+	struct efi_loaded_image *image;
+	efi_status_t ret;
+
+	/*
+	 * driver name
+	 * TODO: support EFI_COMPONENT_NAME2_PROTOCOL
+	 */
+	*driver_name = NULL;
+
+	/* image name */
+	ret = efi_search_protocol(handle, &efi_guid_loaded_image, &handler);
+	if (ret != EFI_SUCCESS) {
+		*image_path = NULL;
+		return 0;
+	}
+
+	image = handler->protocol_interface;
+	*image_path = efi_dp_str(image->file_path);
+
+	return 0;
+}
+
+static int do_efi_show_drivers(cmd_tbl_t *cmdtp, int flag,
+			       int argc, char * const argv[])
+{
+	efi_handle_t *handles = NULL, *handle;
+	efi_uintn_t size = 0;
+	u16 *driver_name, *image_path_text;
+	efi_status_t ret;
+	int i;
+
+	ret = BS->locate_handle(BY_PROTOCOL, &efi_guid_driver_binding_protocol,
+				NULL, &size, NULL);
+	if (ret == EFI_BUFFER_TOO_SMALL) {
+		handles = calloc(1, size);
+		if (!handles)
+			return CMD_RET_FAILURE;
+
+		ret = BS->locate_handle(BY_PROTOCOL,
+					&efi_guid_driver_binding_protocol,
+					NULL, &size, handles);
+	}
+	if (ret != EFI_SUCCESS) {
+		free(handles);
+		return CMD_RET_FAILURE;
+	}
+
+	printf("Driver%.*s Name                 Image Path\n",
+	       EFI_HANDLE_WIDTH - 6, spc);
+	printf("%.*s ==================== ====================\n",
+	       EFI_HANDLE_WIDTH, sep);
+	handle = handles;
+	for (i = 0; i < size / sizeof(*handle); i++) {
+		if (!efi_get_driver_handle_info(*handle, &driver_name,
+						&image_path_text)) {
+			if (image_path_text)
+				printf("%p %-20ls %ls\n",
+				       *handle, driver_name, image_path_text);
+			else
+				printf("%p %-20ls <built-in>\n",
+				       *handle, driver_name);
+			efi_free_pool(driver_name);
+			efi_free_pool(image_path_text);
+		}
+		handle++;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
 			   int argc, char * const argv[])
 {
@@ -504,6 +578,8 @@ static cmd_tbl_t cmd_efidebug_sub[] = {
 	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
 	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
 			 "", ""),
+	U_BOOT_CMD_MKENT(drivers, CONFIG_SYS_MAXARGS, 1, do_efi_show_drivers,
+			 "", ""),
 };
 
 /* Interpreter command to configure UEFI environment */
@@ -551,7 +627,9 @@ static char efidebug_help_text[] =
 	"  - set/show UEFI boot order\n"
 	"\n"
 	"efidebug devices\n"
-	"  - show uefi devices\n";
+	"  - show uefi devices\n"
+	"efidebug drivers\n"
+	"  - show uefi drivers\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

[U-Boot] [PATCH v6 5/7] cmd: efidebug: add dh command

[AKASHI Takahiro]

"dh" command prints all the uefi handles used in the system.

=> efi dh
        7ef3bfa0: Device Path, Device Path To Text, Device Path Utilities,
		  Unicode Collation 2
        7ef31d30: Driver Binding
        7ef31da0: Simple Text Output
        7ef31e10: Simple Text Input, Simple Text Input Ex
        7ef3cca0: Block IO, Device Path
        7ef3d070: Block IO, Device Path
        7ef3d1b0: Block IO, Device Path, Simple File System
        7ef3d3e0: Block IO, Device Path, Simple File System

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/efidebug.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 121 insertions(+), 1 deletion(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 1b788c76a895..a1852e8ea4b9 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -185,6 +185,122 @@ static int do_efi_show_drivers(cmd_tbl_t *cmdtp, int flag,
 	return CMD_RET_SUCCESS;
 }
 
+static const struct {
+	const char *text;
+	const efi_guid_t guid;
+} guid_list[] = {
+	{
+		"Device Path",
+		DEVICE_PATH_GUID,
+	},
+	{
+		"Device Path To Text",
+		EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID,
+	},
+	{
+		"Device Path Utilities",
+		EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID,
+	},
+	{
+		"Unicode Collation 2",
+		EFI_UNICODE_COLLATION_PROTOCOL2_GUID,
+	},
+	{
+		"Driver Binding",
+		EFI_DRIVER_BINDING_PROTOCOL_GUID,
+	},
+	{
+		"Simple Text Input",
+		EFI_SIMPLE_TEXT_INPUT_PROTOCOL_GUID,
+	},
+	{
+		"Simple Text Input Ex",
+		EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL_GUID,
+	},
+	{
+		"Simple Text Output",
+		EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL_GUID,
+	},
+	{
+		"Block IO",
+		BLOCK_IO_GUID,
+	},
+	{
+		"Simple File System",
+		EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID,
+	},
+	{
+		"Loaded Image",
+		LOADED_IMAGE_PROTOCOL_GUID,
+	},
+	{
+		"GOP",
+		EFI_GOP_GUID,
+	},
+};
+
+static const char *get_guid_text(const efi_guid_t *guid)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(guid_list); i++)
+		if (!guidcmp(&guid_list[i].guid, guid))
+			break;
+
+	if (i != ARRAY_SIZE(guid_list))
+		return guid_list[i].text;
+	else
+		return NULL;
+}
+
+static int do_efi_show_handles(cmd_tbl_t *cmdtp, int flag,
+			       int argc, char * const argv[])
+{
+	efi_handle_t *handles;
+	efi_guid_t **guid;
+	efi_uintn_t count;
+	const char *guid_text;
+	int num, i, j;
+	efi_status_t ret;
+
+	handles = NULL;
+	num = 0;
+	if (efi_get_handles_by_proto(NULL, &handles, &num))
+		return CMD_RET_FAILURE;
+
+	if (!num)
+		return CMD_RET_SUCCESS;
+
+	printf("Handle%.*s Protocols\n", EFI_HANDLE_WIDTH - 6, spc);
+	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
+	for (i = 0; i < num; i++) {
+		printf("%p", handles[i]);
+		ret = BS->protocols_per_handle(handles[i], &guid, &count);
+		if (ret || !count) {
+			putc('\n');
+			continue;
+		}
+
+		for (j = 0; j < count; j++) {
+			if (j)
+				printf(", ");
+			else
+				putc(' ');
+
+			guid_text = get_guid_text(guid[j]);
+			if (guid_text)
+				puts(guid_text);
+			else
+				printf("%pUl", guid[j]);
+		}
+		putc('\n');
+	}
+
+	free(handles);
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
 			   int argc, char * const argv[])
 {
@@ -580,6 +696,8 @@ static cmd_tbl_t cmd_efidebug_sub[] = {
 			 "", ""),
 	U_BOOT_CMD_MKENT(drivers, CONFIG_SYS_MAXARGS, 1, do_efi_show_drivers,
 			 "", ""),
+	U_BOOT_CMD_MKENT(dh, CONFIG_SYS_MAXARGS, 1, do_efi_show_handles,
+			 "", ""),
 };
 
 /* Interpreter command to configure UEFI environment */
@@ -629,7 +747,9 @@ static char efidebug_help_text[] =
 	"efidebug devices\n"
 	"  - show uefi devices\n"
 	"efidebug drivers\n"
-	"  - show uefi drivers\n";
+	"  - show uefi drivers\n"
+	"efidebug dh\n"
+	"  - show uefi handles\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

[U-Boot] [PATCH v6 6/7] cmd: efidebug: add images command

[AKASHI Takahiro]

"images" command prints loaded images-related information.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/efidebug.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index a1852e8ea4b9..81ab3654f746 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -301,6 +301,14 @@ static int do_efi_show_handles(cmd_tbl_t *cmdtp, int flag,
 	return CMD_RET_SUCCESS;
 }
 
+static int do_efi_show_images(cmd_tbl_t *cmdtp, int flag,
+			      int argc, char * const argv[])
+{
+	efi_print_image_infos(NULL);
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
 			   int argc, char * const argv[])
 {
@@ -698,6 +706,8 @@ static cmd_tbl_t cmd_efidebug_sub[] = {
 			 "", ""),
 	U_BOOT_CMD_MKENT(dh, CONFIG_SYS_MAXARGS, 1, do_efi_show_handles,
 			 "", ""),
+	U_BOOT_CMD_MKENT(images, CONFIG_SYS_MAXARGS, 1, do_efi_show_images,
+			 "", ""),
 };
 
 /* Interpreter command to configure UEFI environment */
@@ -749,7 +759,9 @@ static char efidebug_help_text[] =
 	"efidebug drivers\n"
 	"  - show uefi drivers\n"
 	"efidebug dh\n"
-	"  - show uefi handles\n";
+	"  - show uefi handles\n"
+	"efidebug images\n"
+	"  - show loaded images\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

[U-Boot] [PATCH v6 7/7] cmd: efidebug: add memmap command

[AKASHI Takahiro]

"memmap" command prints uefi-specific memory map information.
=> efi memmap
Type             Start            End              Attributes
================ ================ ================ ==========
CONVENTIONAL     0000000040000000-000000007de27000 WB
RUNTIME DATA     000000007de27000-000000007de28000 WB|RT
RESERVED         000000007de28000-000000007de2a000 WB
RUNTIME DATA     000000007de2a000-000000007de2b000 WB|RT
RESERVED         000000007de2b000-000000007de2c000 WB
RUNTIME DATA     000000007de2c000-000000007de2d000 WB|RT
LOADER DATA      000000007de2d000-000000007ff37000 WB
RUNTIME CODE     000000007ff37000-000000007ff38000 WB|RT
LOADER DATA      000000007ff38000-0000000080000000 WB

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/efidebug.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 1 deletion(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 81ab3654f746..39398669e18f 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -309,6 +309,100 @@ static int do_efi_show_images(cmd_tbl_t *cmdtp, int flag,
 	return CMD_RET_SUCCESS;
 }
 
+static const char * const efi_mem_type_string[] = {
+	[EFI_RESERVED_MEMORY_TYPE] = "RESERVED",
+	[EFI_LOADER_CODE] = "LOADER CODE",
+	[EFI_LOADER_DATA] = "LOADER DATA",
+	[EFI_BOOT_SERVICES_CODE] = "BOOT CODE",
+	[EFI_BOOT_SERVICES_DATA] = "BOOT DATA",
+	[EFI_RUNTIME_SERVICES_CODE] = "RUNTIME CODE",
+	[EFI_RUNTIME_SERVICES_DATA] = "RUNTIME DATA",
+	[EFI_CONVENTIONAL_MEMORY] = "CONVENTIONAL",
+	[EFI_UNUSABLE_MEMORY] = "UNUSABLE MEM",
+	[EFI_ACPI_RECLAIM_MEMORY] = "ACPI RECLAIM MEM",
+	[EFI_ACPI_MEMORY_NVS] = "ACPI NVS",
+	[EFI_MMAP_IO] = "IO",
+	[EFI_MMAP_IO_PORT] = "IO PORT",
+	[EFI_PAL_CODE] = "PAL",
+};
+
+static const struct efi_mem_attrs {
+	const u64 bit;
+	const char *text;
+} efi_mem_attrs[] = {
+	{EFI_MEMORY_UC, "UC"},
+	{EFI_MEMORY_UC, "UC"},
+	{EFI_MEMORY_WC, "WC"},
+	{EFI_MEMORY_WT, "WT"},
+	{EFI_MEMORY_WB, "WB"},
+	{EFI_MEMORY_UCE, "UCE"},
+	{EFI_MEMORY_WP, "WP"},
+	{EFI_MEMORY_RP, "RP"},
+	{EFI_MEMORY_XP, "WP"},
+	{EFI_MEMORY_NV, "NV"},
+	{EFI_MEMORY_MORE_RELIABLE, "REL"},
+	{EFI_MEMORY_RO, "RO"},
+	{EFI_MEMORY_RUNTIME, "RT"},
+};
+
+static void print_memory_attributes(u64 attributes)
+{
+	int sep, i;
+
+	for (sep = 0, i = 0; i < ARRAY_SIZE(efi_mem_attrs); i++)
+		if (attributes & efi_mem_attrs[i].bit) {
+			if (sep) {
+				putc('|');
+			} else {
+				putc(' ');
+				sep = 1;
+			}
+			puts(efi_mem_attrs[i].text);
+		}
+}
+
+static int do_efi_show_memmap(cmd_tbl_t *cmdtp, int flag,
+			      int argc, char * const argv[])
+{
+	struct efi_mem_desc *memmap = NULL, *map;
+	efi_uintn_t map_size = 0;
+	const char *type;
+	int i;
+	efi_status_t ret;
+
+	ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
+	if (ret == EFI_BUFFER_TOO_SMALL) {
+		memmap = malloc(map_size);
+		if (!memmap)
+			return CMD_RET_FAILURE;
+		ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
+	}
+	if (ret != EFI_SUCCESS) {
+		free(memmap);
+		return CMD_RET_FAILURE;
+	}
+
+	printf("Type             Start%.*s End%.*s Attributes\n",
+	       EFI_HANDLE_WIDTH - 5, spc, EFI_HANDLE_WIDTH - 3, spc);
+	printf("================ %.*s %.*s ==========\n",
+	       EFI_HANDLE_WIDTH, sep, EFI_HANDLE_WIDTH, sep);
+	for (i = 0, map = memmap; i < map_size / sizeof(*map); map++, i++) {
+		if (map->type < EFI_MAX_MEMORY_TYPE)
+			type = efi_mem_type_string[map->type];
+		else
+			type = "(unknown)";
+		printf("%-16s %016llx-%016llx", type, map->physical_start,
+		       map->physical_start + map->num_pages * EFI_PAGE_SIZE);
+
+		print_memory_attributes(map->attribute);
+		putc('\n');
+	}
+
+	free(memmap);
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
 			   int argc, char * const argv[])
 {
@@ -708,6 +802,8 @@ static cmd_tbl_t cmd_efidebug_sub[] = {
 			 "", ""),
 	U_BOOT_CMD_MKENT(images, CONFIG_SYS_MAXARGS, 1, do_efi_show_images,
 			 "", ""),
+	U_BOOT_CMD_MKENT(memmap, CONFIG_SYS_MAXARGS, 1, do_efi_show_memmap,
+			 "", ""),
 };
 
 /* Interpreter command to configure UEFI environment */
@@ -761,7 +857,9 @@ static char efidebug_help_text[] =
 	"efidebug dh\n"
 	"  - show uefi handles\n"
 	"efidebug images\n"
-	"  - show loaded images\n";
+	"  - show loaded images\n"
+	"efidebug memmap\n"
+	"  - show uefi memory map\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

[U-Boot] [PATCH v6 0/7] cmd: add efidebug for efi environment

[Alexander Graf]

On 24.01.19 12:04, AKASHI Takahiro wrote:
> This patch set is a collection of patches to enhance efi user interfaces
> /commands. It will help improve user experience on efi boot and make it
> more usable *without* edk2's shell utility.
> 
> Let's see how it works:
> => efidebug boot add 1 SHELL scsi 1:1 /Shell.efi ""
> => efidebug boot add 2 HELLO scsi 1:1 /hello.efi ""
> => efidebug boot dump
> Boot0001:
> 	attributes: A-- (0x00000001)
> 	label: SHELL
> 	file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)/\\Shell.efi
> 	data: 
> Boot0002:
> 	attributes: A-- (0x00000001)
> 	label: HELLO
> 	file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)/\\hello.efi
> 	data:
> 
> => efidebug boot order 1 2
> => efidebug boot order
>  1: Boot0001: SHELL
>  2: Boot0002: HELLO
> 
> => run -e Boot0002 (or bootefi bootmgr - 2)	; '-' means no dtb specified
> WARNING: booting without device tree
> Booting: HELLO
> ## Starting EFI application at 000000007db8b040 ...
> Hello, world!
> ## Application terminated, r = 0
> 
> => env set -e PlatformLang en			; important!
> => env print -e
> Boot0001: BS|RT, DataSize = 0x79
>     00000000: 01 00 00 00 66 00 53 00 48 00 45 00 4c 00 4c 00  ....f.S.H.E.L.L.
>     00000010: 00 00 01 04 14 00 b9 73 1d e6 84 a3 cc 4a ae ab  .......s.....J..
>     00000020: 82 e8 28 f3 62 8b 03 02 08 00 01 00 00 00 04 01  ..(.b...........
>     00000030: 2a 00 01 00 00 00 00 08 00 00 00 00 00 00 00 00  *...............
>     00000040: 04 00 00 00 00 00 ba 46 62 08 00 00 00 00 00 00  .......Fb.......
>     00000050: 00 00 00 00 00 00 01 01 04 04 1c 00 5c 00 5c 00  ............\.\.
>     00000060: 53 00 68 00 65 00 6c 00 6c 00 2e 00 65 00 66 00  S.h.e.l.l...e.f.
>     00000070: 69 00 00 00 7f ff 04 00 00                       i........
> Boot0002: BS|RT, DataSize = 0x79
>     00000000: 01 00 00 00 66 00 48 00 45 00 4c 00 4c 00 4f 00  ....f.H.E.L.L.O.
>     00000010: 00 00 01 04 14 00 b9 73 1d e6 84 a3 cc 4a ae ab  .......s.....J..
>     00000020: 82 e8 28 f3 62 8b 03 02 08 00 01 00 00 00 04 01  ..(.b...........
>     00000030: 2a 00 01 00 00 00 00 08 00 00 00 00 00 00 00 00  *...............
>     00000040: 04 00 00 00 00 00 ba 46 62 08 00 00 00 00 00 00  .......Fb.......
>     00000050: 00 00 00 00 00 00 01 01 04 04 1c 00 5c 00 5c 00  ............\.\.
>     00000060: 68 00 65 00 6c 00 6c 00 6f 00 2e 00 65 00 66 00  h.e.l.l.o...e.f.
>     00000070: 69 00 00 00 7f ff 04 00 00                       i........
> BootOrder: BS|RT, DataSize = 0x4
>     00000000: 01 00 02 00                                      ....
> PlatformLang: BS|RT, DataSize = 0x2
>     00000000: 65 6e                                            en
> 
> => run -e Boot0001 or bootefi bootmgr
> 
>    (UEFI shell ...)
> 
> "env set" command now supports UEFI shell-like syntax:
> 
> => env set -e foo =S\"akashi\" =0x012345 =Habcdef
> => env print -e foo
> foo: BS|RT, DataSize = 0xd
>     00000000: 61 6b 61 73 68 69 45 23 01 00 ab cd ef           akashiE#.....
> 
> Other useful sub commands are:
> => efidebug devices				; print uefi devices
> => efidebug drivers				; print uefi drivers
> => efidebug dh					; print uefi handles
> => efidebug images				; print loaded images
> => efidebug memmap				; dump uefi memory map
> 
> Enjoy!

Did this patch set successfully pass Travis tests? Could you please
point me to the results?


Thanks,

Alex

[U-Boot] [PATCH v6 1/7] cmd: env: add "-e" option for handling UEFI variables

[Alexander Graf]

On 24.01.19 12:04, 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>
> ---
>  MAINTAINERS       |   1 +
>  cmd/Kconfig       |   6 +
>  cmd/Makefile      |   1 +
>  cmd/nvedit.c      |  28 +++-
>  cmd/nvedit_efi.c  | 319 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/command.h |   4 +
>  6 files changed, 358 insertions(+), 1 deletion(-)
>  create mode 100644 cmd/nvedit_efi.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ae825014bda9..22ac686ab2d6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -438,6 +438,7 @@ F:	lib/efi*/
>  F:	test/py/tests/test_efi*
>  F:	test/unicode_ut.c
>  F:	cmd/bootefi.c
> +F:	cmd/nvedit_efi.c
>  F:	tools/file2include.c
>  
>  FPGA
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index ea1a325eb301..812a7eb9b74b 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -53,6 +53,12 @@ config SYS_PROMPT
>  	  This string is displayed in the command line to the left of the
>  	  cursor.
>  
> +config CMD_NVEDIT_EFI
> +	bool
> +	depends on EFI_LOADER
> +	default y if EFI_LOADER

No need for the "if EFI_LOADER" here. You only ever get to run the
default path when EFI_LOADER is set due to the depends on line before.

> +	imply HEXDUMP
> +
>  menu "Autoboot options"
>  
>  config AUTOBOOT
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 15ae4d250f50..142e0ee222ca 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_CMD_MTD) += mtd.o
>  obj-$(CONFIG_CMD_MTDPARTS) += mtdparts.o
>  obj-$(CONFIG_CMD_NAND) += nand.o
>  obj-$(CONFIG_CMD_NET) += net.o
> +obj-$(CONFIG_CMD_NVEDIT_EFI) += nvedit_efi.o
>  obj-$(CONFIG_CMD_ONENAND) += onenand.o
>  obj-$(CONFIG_CMD_OSD) += osd.o
>  obj-$(CONFIG_CMD_PART) += part.o
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index ce746bbf1b3e..7973d7add5fc 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -119,6 +119,11 @@ 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_NVEDIT_EFI)
> +	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> +		return do_env_print_efi(cmdtp, flag, --argc, ++argv);
> +#endif
> +
>  	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'a') {
>  		argc--;
>  		argv++;
> @@ -216,6 +221,12 @@ 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_NVEDIT_EFI)
> +	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> +		return do_env_set_efi(NULL, flag, --argc, ++argv);
> +#endif
> +
>  	while (argc > 1 && **(argv + 1) == '-') {
>  		char *arg = *++argv;
>  
> @@ -1263,14 +1274,21 @@ static char env_help_text[] =
>  	"env import [-d] [-t [-r] | -b | -c] addr [size] [var ...] - import environment\n"
>  #endif
>  	"env print [-a | name ...] - print environment\n"
> +#if defined(CONFIG_CMD_NVEDIT_EFI)
> +	"env print  -e [name ...] - print UEFI 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_NVEDIT_EFI)
> +	"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 +1313,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_NVEDIT_EFI)
> +	"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 +1344,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_NVEDIT_EFI)
> +	"-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'",
> diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
> new file mode 100644
> index 000000000000..58c92009e8db
> --- /dev/null
> +++ b/cmd/nvedit_efi.c
> @@ -0,0 +1,319 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  Integrate UEFI variables to u-boot env interface
> + *
> + *  Copyright (c) 2018 AKASHI Takahiro, Linaro Limited
> + */
> +
> +#include <charset.h>
> +#include <common.h>
> +#include <command.h>
> +#include <efi_loader.h>
> +#include <exports.h>
> +#include <hexdump.h>
> +#include <malloc.h>
> +#include <linux/kernel.h>
> +
> +static const struct {
> +	u32 mask;
> +	char *text;
> +} efi_var_attrs[] = {
> +	{EFI_VARIABLE_NON_VOLATILE, "NV"},
> +	{EFI_VARIABLE_BOOTSERVICE_ACCESS, "BS"},
> +	{EFI_VARIABLE_RUNTIME_ACCESS, "RT"},
> +	{EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS, "AW"},
> +	{EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, "AT"},
> +};
> +
> +static int print_variable_info(u16 *name, efi_guid_t *guid)
> +{
> +	u32 attributes;
> +	u8 *data;
> +	efi_uintn_t size;
> +	int count, i;
> +	efi_status_t ret;
> +
> +	data = NULL;
> +	size = 0;
> +	ret = efi_get_variable(name, guid, &attributes, &size, data);
> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		data = malloc(size);
> +		if (!data)
> +			return -1;

Just goto out.

> +
> +		ret = efi_get_variable(name, guid, &attributes, &size, data);
> +	}
> +	if (ret == EFI_NOT_FOUND) {
> +		printf("Error: \"%ls\" not defined\n", name);
> +		goto out;
> +	}
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	printf("%ls:", name);
> +	for (count = 0, i = 0; i < ARRAY_SIZE(efi_var_attrs); i++)
> +		if (attributes & efi_var_attrs[i].mask) {
> +			if (count)
> +				putc('|');
> +			else
> +				putc(' ');
> +			count++;
> +			puts(efi_var_attrs[i].text);
> +		}
> +	printf(", DataSize = 0x%lx\n", size);
> +	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1, data, size, true);
> +
> +	return 0;
> +out:
> +	free(data);
> +	return -1;
> +}
> +
> +/*
> + * From efi_variable.c,
> + *
> + * Mapping between UEFI variables and u-boot variables:
> + *
> + *   efi_$guid_$varname = {attributes}(type)value
> + */
> +static int efi_dump_var(cmd_tbl_t *cmdtp, int flag,
> +			int argc, char * const argv[])
> +{
> +	u16 *var_name16, *p;
> +	efi_uintn_t buf_size, size;
> +	efi_guid_t guid;
> +	efi_status_t ret;
> +
> +	buf_size = 128;
> +	var_name16 = malloc(buf_size);
> +	if (!var_name16)
> +		return CMD_RET_FAILURE;
> +
> +	if (argc > 1) {

Missing a comment on what this does. Also, I think it makes sense to
extract the 2 cases (dump 1 var, dump all vars) into separate functions.
They share almost no code and just make indentation hard.

> +		argc--;
> +		argv++;
> +
> +		for (; argc > 0; argc--, argv++) {
> +			size = (strlen(argv[0]) + 1) * 2;

(utf8_utf16_strlen() + 1) * sizeof(u16)?

> +			if (buf_size < size) {
> +				buf_size = size;
> +				p = realloc(var_name16, buf_size);
> +				if (!p) {
> +					free(var_name16);
> +					return CMD_RET_FAILURE;
> +				}
> +				var_name16 = p;
> +			}
> +
> +			p = var_name16;
> +			utf8_utf16_strcpy(&p, argv[0]);
> +			guid = efi_global_variable_guid;

Why do you need to copy the guid into a stack variable? Can't you just
pass the pointer?

> +
> +			print_variable_info(var_name16, &guid);

Not checking return value?

> +		}
> +
> +		free(var_name16);
> +
> +		return CMD_RET_SUCCESS;
> +	}
> +
> +	var_name16[0] = 0;
> +	for (;;) {
> +		size = buf_size;
> +		ret = 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_get_next_variable_name(&size, var_name16,
> +							 &guid);
> +		}
> +		if (ret != EFI_SUCCESS) {
> +			free(var_name16);
> +			return CMD_RET_FAILURE;
> +		}
> +
> +		print_variable_info(var_name16, &guid);

Not checking return value?


Alex

[U-Boot] [PATCH v6 0/7] cmd: add efidebug for efi environment

[AKASHI Takahiro]

Alex,

On Fri, Jan 25, 2019 at 12:56:52PM +0100, Alexander Graf wrote:
> 
> 
> On 24.01.19 12:04, AKASHI Takahiro wrote:
> > This patch set is a collection of patches to enhance efi user interfaces
> > /commands. It will help improve user experience on efi boot and make it
> > more usable *without* edk2's shell utility.
> > 
> > Let's see how it works:
> > => efidebug boot add 1 SHELL scsi 1:1 /Shell.efi ""
> > => efidebug boot add 2 HELLO scsi 1:1 /hello.efi ""
> > => efidebug boot dump
> > Boot0001:
> > 	attributes: A-- (0x00000001)
> > 	label: SHELL
> > 	file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)/\\Shell.efi
> > 	data: 
> > Boot0002:
> > 	attributes: A-- (0x00000001)
> > 	label: HELLO
> > 	file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)/\\hello.efi
> > 	data:
> > 
> > => efidebug boot order 1 2
> > => efidebug boot order
> >  1: Boot0001: SHELL
> >  2: Boot0002: HELLO
> > 
> > => run -e Boot0002 (or bootefi bootmgr - 2)	; '-' means no dtb specified
> > WARNING: booting without device tree
> > Booting: HELLO
> > ## Starting EFI application at 000000007db8b040 ...
> > Hello, world!
> > ## Application terminated, r = 0
> > 
> > => env set -e PlatformLang en			; important!
> > => env print -e
> > Boot0001: BS|RT, DataSize = 0x79
> >     00000000: 01 00 00 00 66 00 53 00 48 00 45 00 4c 00 4c 00  ....f.S.H.E.L.L.
> >     00000010: 00 00 01 04 14 00 b9 73 1d e6 84 a3 cc 4a ae ab  .......s.....J..
> >     00000020: 82 e8 28 f3 62 8b 03 02 08 00 01 00 00 00 04 01  ..(.b...........
> >     00000030: 2a 00 01 00 00 00 00 08 00 00 00 00 00 00 00 00  *...............
> >     00000040: 04 00 00 00 00 00 ba 46 62 08 00 00 00 00 00 00  .......Fb.......
> >     00000050: 00 00 00 00 00 00 01 01 04 04 1c 00 5c 00 5c 00  ............\.\.
> >     00000060: 53 00 68 00 65 00 6c 00 6c 00 2e 00 65 00 66 00  S.h.e.l.l...e.f.
> >     00000070: 69 00 00 00 7f ff 04 00 00                       i........
> > Boot0002: BS|RT, DataSize = 0x79
> >     00000000: 01 00 00 00 66 00 48 00 45 00 4c 00 4c 00 4f 00  ....f.H.E.L.L.O.
> >     00000010: 00 00 01 04 14 00 b9 73 1d e6 84 a3 cc 4a ae ab  .......s.....J..
> >     00000020: 82 e8 28 f3 62 8b 03 02 08 00 01 00 00 00 04 01  ..(.b...........
> >     00000030: 2a 00 01 00 00 00 00 08 00 00 00 00 00 00 00 00  *...............
> >     00000040: 04 00 00 00 00 00 ba 46 62 08 00 00 00 00 00 00  .......Fb.......
> >     00000050: 00 00 00 00 00 00 01 01 04 04 1c 00 5c 00 5c 00  ............\.\.
> >     00000060: 68 00 65 00 6c 00 6c 00 6f 00 2e 00 65 00 66 00  h.e.l.l.o...e.f.
> >     00000070: 69 00 00 00 7f ff 04 00 00                       i........
> > BootOrder: BS|RT, DataSize = 0x4
> >     00000000: 01 00 02 00                                      ....
> > PlatformLang: BS|RT, DataSize = 0x2
> >     00000000: 65 6e                                            en
> > 
> > => run -e Boot0001 or bootefi bootmgr
> > 
> >    (UEFI shell ...)
> > 
> > "env set" command now supports UEFI shell-like syntax:
> > 
> > => env set -e foo =S\"akashi\" =0x012345 =Habcdef
> > => env print -e foo
> > foo: BS|RT, DataSize = 0xd
> >     00000000: 61 6b 61 73 68 69 45 23 01 00 ab cd ef           akashiE#.....
> > 
> > Other useful sub commands are:
> > => efidebug devices				; print uefi devices
> > => efidebug drivers				; print uefi drivers
> > => efidebug dh					; print uefi handles
> > => efidebug images				; print loaded images
> > => efidebug memmap				; dump uefi memory map
> > 
> > Enjoy!
> 
> Did this patch set successfully pass Travis tests? Could you please
> point me to the results?

Not yet, probably next time?
It may take some time for me to set up travis.

-Takahiro Akashi

> 
> Thanks,
> 
> Alex

[U-Boot] [PATCH v6 1/7] cmd: env: add "-e" option for handling UEFI variables

[AKASHI Takahiro]

On Fri, Jan 25, 2019 at 01:42:17PM +0100, Alexander Graf wrote:
> 
> 
> On 24.01.19 12:04, 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>
> > ---
> >  MAINTAINERS       |   1 +
> >  cmd/Kconfig       |   6 +
> >  cmd/Makefile      |   1 +
> >  cmd/nvedit.c      |  28 +++-
> >  cmd/nvedit_efi.c  | 319 ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/command.h |   4 +
> >  6 files changed, 358 insertions(+), 1 deletion(-)
> >  create mode 100644 cmd/nvedit_efi.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ae825014bda9..22ac686ab2d6 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -438,6 +438,7 @@ F:	lib/efi*/
> >  F:	test/py/tests/test_efi*
> >  F:	test/unicode_ut.c
> >  F:	cmd/bootefi.c
> > +F:	cmd/nvedit_efi.c
> >  F:	tools/file2include.c
> >  
> >  FPGA
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index ea1a325eb301..812a7eb9b74b 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -53,6 +53,12 @@ config SYS_PROMPT
> >  	  This string is displayed in the command line to the left of the
> >  	  cursor.
> >  
> > +config CMD_NVEDIT_EFI
> > +	bool
> > +	depends on EFI_LOADER
> > +	default y if EFI_LOADER
> 
> No need for the "if EFI_LOADER" here. You only ever get to run the
> default path when EFI_LOADER is set due to the depends on line before.

Right.

> > +	imply HEXDUMP
> > +
> >  menu "Autoboot options"
> >  
> >  config AUTOBOOT
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index 15ae4d250f50..142e0ee222ca 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -98,6 +98,7 @@ obj-$(CONFIG_CMD_MTD) += mtd.o
> >  obj-$(CONFIG_CMD_MTDPARTS) += mtdparts.o
> >  obj-$(CONFIG_CMD_NAND) += nand.o
> >  obj-$(CONFIG_CMD_NET) += net.o
> > +obj-$(CONFIG_CMD_NVEDIT_EFI) += nvedit_efi.o
> >  obj-$(CONFIG_CMD_ONENAND) += onenand.o
> >  obj-$(CONFIG_CMD_OSD) += osd.o
> >  obj-$(CONFIG_CMD_PART) += part.o
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index ce746bbf1b3e..7973d7add5fc 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -119,6 +119,11 @@ 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_NVEDIT_EFI)
> > +	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> > +		return do_env_print_efi(cmdtp, flag, --argc, ++argv);
> > +#endif
> > +
> >  	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'a') {
> >  		argc--;
> >  		argv++;
> > @@ -216,6 +221,12 @@ 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_NVEDIT_EFI)
> > +	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> > +		return do_env_set_efi(NULL, flag, --argc, ++argv);
> > +#endif
> > +
> >  	while (argc > 1 && **(argv + 1) == '-') {
> >  		char *arg = *++argv;
> >  
> > @@ -1263,14 +1274,21 @@ static char env_help_text[] =
> >  	"env import [-d] [-t [-r] | -b | -c] addr [size] [var ...] - import environment\n"
> >  #endif
> >  	"env print [-a | name ...] - print environment\n"
> > +#if defined(CONFIG_CMD_NVEDIT_EFI)
> > +	"env print  -e [name ...] - print UEFI 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_NVEDIT_EFI)
> > +	"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 +1313,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_NVEDIT_EFI)
> > +	"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 +1344,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_NVEDIT_EFI)
> > +	"-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'",
> > diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
> > new file mode 100644
> > index 000000000000..58c92009e8db
> > --- /dev/null
> > +++ b/cmd/nvedit_efi.c
> > @@ -0,0 +1,319 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  Integrate UEFI variables to u-boot env interface
> > + *
> > + *  Copyright (c) 2018 AKASHI Takahiro, Linaro Limited
> > + */
> > +
> > +#include <charset.h>
> > +#include <common.h>
> > +#include <command.h>
> > +#include <efi_loader.h>
> > +#include <exports.h>
> > +#include <hexdump.h>
> > +#include <malloc.h>
> > +#include <linux/kernel.h>
> > +
> > +static const struct {
> > +	u32 mask;
> > +	char *text;
> > +} efi_var_attrs[] = {
> > +	{EFI_VARIABLE_NON_VOLATILE, "NV"},
> > +	{EFI_VARIABLE_BOOTSERVICE_ACCESS, "BS"},
> > +	{EFI_VARIABLE_RUNTIME_ACCESS, "RT"},
> > +	{EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS, "AW"},
> > +	{EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, "AT"},
> > +};
> > +
> > +static int print_variable_info(u16 *name, efi_guid_t *guid)
> > +{
> > +	u32 attributes;
> > +	u8 *data;
> > +	efi_uintn_t size;
> > +	int count, i;
> > +	efi_status_t ret;
> > +
> > +	data = NULL;
> > +	size = 0;
> > +	ret = efi_get_variable(name, guid, &attributes, &size, data);
> > +	if (ret == EFI_BUFFER_TOO_SMALL) {
> > +		data = malloc(size);
> > +		if (!data)
> > +			return -1;
> 
> Just goto out.

Sure.

> > +
> > +		ret = efi_get_variable(name, guid, &attributes, &size, data);
> > +	}
> > +	if (ret == EFI_NOT_FOUND) {
> > +		printf("Error: \"%ls\" not defined\n", name);
> > +		goto out;
> > +	}
> > +	if (ret != EFI_SUCCESS)
> > +		goto out;
> > +
> > +	printf("%ls:", name);
> > +	for (count = 0, i = 0; i < ARRAY_SIZE(efi_var_attrs); i++)
> > +		if (attributes & efi_var_attrs[i].mask) {
> > +			if (count)
> > +				putc('|');
> > +			else
> > +				putc(' ');
> > +			count++;
> > +			puts(efi_var_attrs[i].text);
> > +		}
> > +	printf(", DataSize = 0x%lx\n", size);
> > +	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1, data, size, true);
> > +
> > +	return 0;
> > +out:
> > +	free(data);
> > +	return -1;
> > +}
> > +
> > +/*
> > + * From efi_variable.c,
> > + *
> > + * Mapping between UEFI variables and u-boot variables:
> > + *
> > + *   efi_$guid_$varname = {attributes}(type)value
> > + */
> > +static int efi_dump_var(cmd_tbl_t *cmdtp, int flag,
> > +			int argc, char * const argv[])
> > +{
> > +	u16 *var_name16, *p;
> > +	efi_uintn_t buf_size, size;
> > +	efi_guid_t guid;
> > +	efi_status_t ret;
> > +
> > +	buf_size = 128;
> > +	var_name16 = malloc(buf_size);
> > +	if (!var_name16)
> > +		return CMD_RET_FAILURE;
> > +
> > +	if (argc > 1) {
> 
> Missing a comment on what this does. Also, I think it makes sense to
> extract the 2 cases (dump 1 var, dump all vars) into separate functions.
> They share almost no code and just make indentation hard.

Okay.

> > +		argc--;
> > +		argv++;
> > +
> > +		for (; argc > 0; argc--, argv++) {
> > +			size = (strlen(argv[0]) + 1) * 2;
> 
> (utf8_utf16_strlen() + 1) * sizeof(u16)?

Okay.

> > +			if (buf_size < size) {
> > +				buf_size = size;
> > +				p = realloc(var_name16, buf_size);
> > +				if (!p) {
> > +					free(var_name16);
> > +					return CMD_RET_FAILURE;
> > +				}
> > +				var_name16 = p;
> > +			}
> > +
> > +			p = var_name16;
> > +			utf8_utf16_strcpy(&p, argv[0]);
> > +			guid = efi_global_variable_guid;
> 
> Why do you need to copy the guid into a stack variable? Can't you just
> pass the pointer?

You're right.

> > +
> > +			print_variable_info(var_name16, &guid);
> 
> Not checking return value?

This is in an iteration loop and the case that a variable doesn't exist
is also handled in print_variable_info(). So we don't care a return value.
I will change a return value to void to clarify this.

> > +		}
> > +
> > +		free(var_name16);
> > +
> > +		return CMD_RET_SUCCESS;
> > +	}
> > +
> > +	var_name16[0] = 0;
> > +	for (;;) {
> > +		size = buf_size;
> > +		ret = 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_get_next_variable_name(&size, var_name16,
> > +							 &guid);
> > +		}
> > +		if (ret != EFI_SUCCESS) {
> > +			free(var_name16);
> > +			return CMD_RET_FAILURE;
> > +		}
> > +
> > +		print_variable_info(var_name16, &guid);
> 
> Not checking return value?

ditto.

Unless you have any comments on other patches (than patch#1),
I'd like to send out only this one as v6.1 (or v7) since changes
here don't affect other patches.

Thanks,
-Takahiro Akashi

> 
> Alex

[U-Boot] [PATCH v6 6/7] cmd: efidebug: add images command

[Alexander Graf]

On 01/24/2019 12:04 PM, AKASHI Takahiro wrote:
> "images" command prints loaded images-related information.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   cmd/efidebug.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index a1852e8ea4b9..81ab3654f746 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -301,6 +301,14 @@ static int do_efi_show_handles(cmd_tbl_t *cmdtp, int flag,
>   	return CMD_RET_SUCCESS;
>   }
>   
> +static int do_efi_show_images(cmd_tbl_t *cmdtp, int flag,
> +			      int argc, char * const argv[])
> +{
> +	efi_print_image_infos(NULL);

Where does this function get defined?


Alex

[U-Boot] [PATCH v6 6/7] cmd: efidebug: add images command

[AKASHI Takahiro]

On Tue, Jan 29, 2019 at 04:35:32PM +0100, Alexander Graf wrote:
> On 01/24/2019 12:04 PM, AKASHI Takahiro wrote:
> >"images" command prints loaded images-related information.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  cmd/efidebug.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> >diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> >index a1852e8ea4b9..81ab3654f746 100644
> >--- a/cmd/efidebug.c
> >+++ b/cmd/efidebug.c
> >@@ -301,6 +301,14 @@ static int do_efi_show_handles(cmd_tbl_t *cmdtp, int flag,
> >  	return CMD_RET_SUCCESS;
> >  }
> >+static int do_efi_show_images(cmd_tbl_t *cmdtp, int flag,
> >+			      int argc, char * const argv[])
> >+{
> >+	efi_print_image_infos(NULL);
> 
> Where does this function get defined?

in efi_image_loader.c by Heinrich.

-Takahiro Akashi

> 
> Alex
> 

[U-Boot] [PATCH v6 7/7] cmd: efidebug: add memmap command

[Heinrich Schuchardt]

On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
> "memmap" command prints uefi-specific memory map information.
> => efi memmap
> Type             Start            End              Attributes
> ================ ================ ================ ==========
> CONVENTIONAL     0000000040000000-000000007de27000 WB
> RUNTIME DATA     000000007de27000-000000007de28000 WB|RT
> RESERVED         000000007de28000-000000007de2a000 WB
> RUNTIME DATA     000000007de2a000-000000007de2b000 WB|RT
> RESERVED         000000007de2b000-000000007de2c000 WB
> RUNTIME DATA     000000007de2c000-000000007de2d000 WB|RT
> LOADER DATA      000000007de2d000-000000007ff37000 WB
> RUNTIME CODE     000000007ff37000-000000007ff38000 WB|RT
> LOADER DATA      000000007ff38000-0000000080000000 WB

On qemu_arm_defconfig I got this ouptut:

=> efidebug memmap
Type             Start    End      Attributes
================ ======== ======== ==========
CONVENTIONAL     0000000040000000-000000007ddf8000 WB
BOOT DATA        000000007ddf8000-000000007ddfd000 WB
RUNTIME DATA     000000007ddfd000-000000007ddfe000 WB|RT
RESERVED         000000007ddfe000-000000007ddff000 WB
RUNTIME DATA     000000007ddff000-000000007de00000 WB|RT
LOADER DATA      000000007de00000-000000007ff42000 WB
RUNTIME CODE     000000007ff42000-000000007ff43000 WB|RT
LOADER DATA      000000007ff43000-0000000080000000 WB

Something is wrong with you format codes.

> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/efidebug.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 99 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 81ab3654f746..39398669e18f 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -309,6 +309,100 @@ static int do_efi_show_images(cmd_tbl_t *cmdtp, int flag,
>  	return CMD_RET_SUCCESS;
>  }
>  
> +static const char * const efi_mem_type_string[] = {
> +	[EFI_RESERVED_MEMORY_TYPE] = "RESERVED",
> +	[EFI_LOADER_CODE] = "LOADER CODE",
> +	[EFI_LOADER_DATA] = "LOADER DATA",
> +	[EFI_BOOT_SERVICES_CODE] = "BOOT CODE",
> +	[EFI_BOOT_SERVICES_DATA] = "BOOT DATA",
> +	[EFI_RUNTIME_SERVICES_CODE] = "RUNTIME CODE",
> +	[EFI_RUNTIME_SERVICES_DATA] = "RUNTIME DATA",
> +	[EFI_CONVENTIONAL_MEMORY] = "CONVENTIONAL",
> +	[EFI_UNUSABLE_MEMORY] = "UNUSABLE MEM",
> +	[EFI_ACPI_RECLAIM_MEMORY] = "ACPI RECLAIM MEM",
> +	[EFI_ACPI_MEMORY_NVS] = "ACPI NVS",
> +	[EFI_MMAP_IO] = "IO",
> +	[EFI_MMAP_IO_PORT] = "IO PORT",
> +	[EFI_PAL_CODE] = "PAL",
> +};
> +
> +static const struct efi_mem_attrs {
> +	const u64 bit;
> +	const char *text;
> +} efi_mem_attrs[] = {
> +	{EFI_MEMORY_UC, "UC"},
> +	{EFI_MEMORY_UC, "UC"},
> +	{EFI_MEMORY_WC, "WC"},
> +	{EFI_MEMORY_WT, "WT"},
> +	{EFI_MEMORY_WB, "WB"},
> +	{EFI_MEMORY_UCE, "UCE"},
> +	{EFI_MEMORY_WP, "WP"},
> +	{EFI_MEMORY_RP, "RP"},
> +	{EFI_MEMORY_XP, "WP"},
> +	{EFI_MEMORY_NV, "NV"},
> +	{EFI_MEMORY_MORE_RELIABLE, "REL"},
> +	{EFI_MEMORY_RO, "RO"},
> +	{EFI_MEMORY_RUNTIME, "RT"},
> +};
> +
> +static void print_memory_attributes(u64 attributes)
> +{
> +	int sep, i;
> +
> +	for (sep = 0, i = 0; i < ARRAY_SIZE(efi_mem_attrs); i++)
> +		if (attributes & efi_mem_attrs[i].bit) {
> +			if (sep) {
> +				putc('|');
> +			} else {
> +				putc(' ');
> +				sep = 1;
> +			}
> +			puts(efi_mem_attrs[i].text);
> +		}
> +}
> +
> +static int do_efi_show_memmap(cmd_tbl_t *cmdtp, int flag,
> +			      int argc, char * const argv[])
> +{
> +	struct efi_mem_desc *memmap = NULL, *map;
> +	efi_uintn_t map_size = 0;
> +	const char *type;
> +	int i;
> +	efi_status_t ret;
> +
> +	ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		memmap = malloc(map_size);
> +		if (!memmap)
> +			return CMD_RET_FAILURE;
> +		ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
> +	}
> +	if (ret != EFI_SUCCESS) {
> +		free(memmap);
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	printf("Type             Start%.*s End%.*s Attributes\n",
> +	       EFI_HANDLE_WIDTH - 5, spc, EFI_HANDLE_WIDTH - 3, spc);
> +	printf("================ %.*s %.*s ==========\n",
> +	       EFI_HANDLE_WIDTH, sep, EFI_HANDLE_WIDTH, sep);
> +	for (i = 0, map = memmap; i < map_size / sizeof(*map); map++, i++) {
> +		if (map->type < EFI_MAX_MEMORY_TYPE)
> +			type = efi_mem_type_string[map->type];
> +		else
> +			type = "(unknown)";
> +		printf("%-16s %016llx-%016llx", type, map->physical_start,

Width 16 is ok on 64bit systems but not on 32bit ones.

Best regards

Heinrich

> +		       map->physical_start + map->num_pages * EFI_PAGE_SIZE);
> +
> +		print_memory_attributes(map->attribute);
> +		putc('\n');
> +	}
> +
> +	free(memmap);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
>  			   int argc, char * const argv[])
>  {
> @@ -708,6 +802,8 @@ static cmd_tbl_t cmd_efidebug_sub[] = {
>  			 "", ""),
>  	U_BOOT_CMD_MKENT(images, CONFIG_SYS_MAXARGS, 1, do_efi_show_images,
>  			 "", ""),
> +	U_BOOT_CMD_MKENT(memmap, CONFIG_SYS_MAXARGS, 1, do_efi_show_memmap,
> +			 "", ""),
>  };
>  
>  /* Interpreter command to configure UEFI environment */
> @@ -761,7 +857,9 @@ static char efidebug_help_text[] =
>  	"efidebug dh\n"
>  	"  - show uefi handles\n"
>  	"efidebug images\n"
> -	"  - show loaded images\n";
> +	"  - show loaded images\n"
> +	"efidebug memmap\n"
> +	"  - show uefi memory map\n";
>  #endif
>  
>  U_BOOT_CMD(
> 

[U-Boot] [PATCH v6 2/7] cmd: add efidebug command

[Heinrich Schuchardt]

On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
> Currently, there is no easy way to add or modify UEFI variables.
> In particular, bootmgr supports BootOrder/BootXXXX variables, it is
> quite hard to define them as u-boot variables because they are represented
> in a complicated and encoded format.
> 
> The new command, efidebug, helps address these issues and give us
> more friendly interfaces:
>  * efidebug boot add: add BootXXXX variable
>  * efidebug boot rm: remove BootXXXX variable
>  * efidebug boot dump: display all BootXXXX variables
>  * efidebug boot next: set BootNext variable
>  * efidebug boot order: set/display a boot order (BootOrder)
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
I could not make this work on qemu_arm_defconfig:

Usage:
efidebug boot add <bootid> <label> <interface> <device>[:<part>] <file
path> [<load options>]

=> efidebug boot add 00AA 'fancy label' mmc 0:1 wonder.efi '--do-it'
=> efidebug boot dump
=>

I would expect either an error or an output of `dump`.

As the UEFI spec teaches: "Each Boot####  variable is the name “Boot”
appended with a unique four digit hexadecimal number." So 00AA should be
a valid id.

Best regards

Heinrich

> ---
>  MAINTAINERS    |   1 +
>  cmd/Kconfig    |  10 ++
>  cmd/Makefile   |   1 +
>  cmd/efidebug.c | 461 +++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 473 insertions(+)
>  create mode 100644 cmd/efidebug.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 22ac686ab2d6..7ad8c01a3b93 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -438,6 +438,7 @@ F:	lib/efi*/
>  F:	test/py/tests/test_efi*
>  F:	test/unicode_ut.c
>  F:	cmd/bootefi.c
> +F:	cmd/efidebug.c
>  F:	cmd/nvedit_efi.c
>  F:	tools/file2include.c
>  
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 812a7eb9b74b..f0405a490635 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1403,6 +1403,16 @@ config CMD_DISPLAY
>  	  displayed on a simple board-specific display. Implement
>  	  display_putc() to use it.
>  
> +config CMD_EFIDEBUG
> +	bool "efidebug - display/configure UEFI environment"
> +	depends on EFI_LOADER
> +	default n
> +	help
> +	  Enable the 'efidebug' command which provides a subset of UEFI
> +	  shell utility with simplified functionality. It will be useful
> +	  particularly for managing boot parameters as  well as examining
> +	  various EFI status for debugging.
> +
>  config CMD_LED
>  	bool "led"
>  	default y if LED
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 142e0ee222ca..48f2742168be 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_CMD_ECHO) += echo.o
>  obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o
>  obj-$(CONFIG_CMD_EEPROM) += eeprom.o
>  obj-$(CONFIG_EFI_STUB) += efi.o
> +obj-$(CONFIG_CMD_EFIDEBUG) += efidebug.o
>  obj-$(CONFIG_CMD_ELF) += elf.o
>  obj-$(CONFIG_HUSH_PARSER) += exit.o
>  obj-$(CONFIG_CMD_EXT4) += ext4.o
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> new file mode 100644
> index 000000000000..d836576cf6e0
> --- /dev/null
> +++ b/cmd/efidebug.c
> @@ -0,0 +1,461 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  UEFI Shell-like command
> + *
> + *  Copyright (c) 2018 AKASHI Takahiro, Linaro Limited
> + */
> +
> +#include <charset.h>
> +#include <common.h>
> +#include <command.h>
> +#include <efi_loader.h>
> +#include <environment.h>
> +#include <exports.h>
> +#include <malloc.h>
> +#include <search.h>
> +#include <linux/ctype.h>
> +
> +static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
> +			   int argc, char * const argv[])
> +{
> +	int id;
> +	char *endp;
> +	char var_name[9];
> +	u16 var_name16[9], *p;
> +	efi_guid_t guid;
> +	size_t label_len, label_len16;
> +	u16 *label;
> +	struct efi_device_path *device_path = NULL, *file_path = NULL;
> +	struct efi_load_option lo;
> +	void *data = NULL;
> +	efi_uintn_t size;
> +	int ret;
> +
> +	if (argc < 6 || argc > 7)
> +		return CMD_RET_USAGE;
> +
> +	id = (int)simple_strtoul(argv[1], &endp, 16);
> +	if (*endp != '\0' || id > 0xffff)
> +		return CMD_RET_FAILURE;
> +
> +	sprintf(var_name, "Boot%04X", id);
> +	p = var_name16;
> +	utf8_utf16_strncpy(&p, var_name, 9);
> +
> +	guid = efi_global_variable_guid;
> +
> +	/* attributes */
> +	lo.attributes = 0x1; /* always ACTIVE */
> +
> +	/* label */
> +	label_len = strlen(argv[2]);
> +	label_len16 = utf8_utf16_strnlen(argv[2], label_len);
> +	label = malloc((label_len16 + 1) * sizeof(u16));
> +	if (!label)
> +		return CMD_RET_FAILURE;
> +	lo.label = label; /* label will be changed below */
> +	utf8_utf16_strncpy(&label, argv[2], label_len);
> +
> +	/* file path */
> +	ret = efi_dp_from_name(argv[3], argv[4], argv[5], &device_path,
> +			       &file_path);
> +	if (ret != EFI_SUCCESS) {
> +		ret = CMD_RET_FAILURE;
> +		goto out;
> +	}
> +	lo.file_path = file_path;
> +	lo.file_path_length = efi_dp_size(file_path)
> +				+ sizeof(struct efi_device_path); /* for END */
> +
> +	/* optional data */
> +	lo.optional_data = (u8 *)(argc == 6 ? "" : argv[6]);
> +
> +	size = efi_serialize_load_option(&lo, (u8 **)&data);
> +	if (!size) {
> +		ret = CMD_RET_FAILURE;
> +		goto out;
> +	}
> +
> +	ret = efi_set_variable(var_name16, &guid,
> +			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +			       EFI_VARIABLE_RUNTIME_ACCESS, size, data);
> +	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
> +out:
> +	free(data);
> +	efi_free_pool(device_path);
> +	efi_free_pool(file_path);
> +	free(lo.label);
> +
> +	return ret;
> +}
> +
> +static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag,
> +			  int argc, char * const argv[])
> +{
> +	efi_guid_t guid;
> +	int id, i;
> +	char *endp;
> +	char var_name[9];
> +	u16 var_name16[9];
> +	efi_status_t ret;
> +
> +	if (argc == 1)
> +		return CMD_RET_USAGE;
> +
> +	guid = efi_global_variable_guid;
> +	for (i = 1; i < argc; i++, argv++) {
> +		id = (int)simple_strtoul(argv[1], &endp, 16);
> +		if (*endp != '\0' || id > 0xffff)
> +			return CMD_RET_FAILURE;
> +
> +		sprintf(var_name, "Boot%04X", id);
> +		utf8_utf16_strncpy((u16 **)&var_name16, var_name, 9);
> +
> +		ret = efi_set_variable(var_name16, &guid, 0, 0, NULL);
> +		if (ret) {
> +			printf("cannot remove Boot%04X", id);
> +			return CMD_RET_FAILURE;
> +		}
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
> +static void show_efi_boot_opt_data(int id, void *data)
> +{
> +	struct efi_load_option lo;
> +	char *label, *p;
> +	size_t label_len16, label_len;
> +	u16 *dp_str;
> +
> +	efi_deserialize_load_option(&lo, data);
> +
> +	label_len16 = u16_strlen(lo.label);
> +	label_len = utf16_utf8_strnlen(lo.label, label_len16);
> +	label = malloc(label_len + 1);
> +	if (!label)
> +		return;
> +	p = label;
> +	utf16_utf8_strncpy(&p, lo.label, label_len16);
> +
> +	printf("Boot%04X:\n", id);
> +	printf("\tattributes: %c%c%c (0x%08x)\n",
> +	       /* ACTIVE */
> +	       lo.attributes & 0x1 ? 'A' : '-',
> +	       /* FORCE RECONNECT */
> +	       lo.attributes & 0x2 ? 'R' : '-',
> +	       /* HIDDEN */
> +	       lo.attributes & 0x8 ? 'H' : '-',
> +	       lo.attributes);
> +	printf("\tlabel: %s\n", label);
> +
> +	dp_str = efi_dp_str(lo.file_path);
> +	printf("\tfile_path: %ls\n", dp_str);
> +	efi_free_pool(dp_str);
> +
> +	printf("\tdata: %s\n", lo.optional_data);
> +
> +	free(label);
> +}
> +
> +static void show_efi_boot_opt(int id)
> +{
> +	char var_name[9];
> +	u16 var_name16[9], *p;
> +	efi_guid_t guid;
> +	void *data = NULL;
> +	efi_uintn_t size;
> +	int ret;
> +
> +	sprintf(var_name, "Boot%04X", id);
> +	p = var_name16;
> +	utf8_utf16_strncpy(&p, var_name, 9);
> +	guid = efi_global_variable_guid;
> +
> +	size = 0;
> +	ret = efi_get_variable(var_name16, &guid, NULL, &size, NULL);
> +	if (ret == (int)EFI_BUFFER_TOO_SMALL) {
> +		data = malloc(size);
> +		ret = efi_get_variable(var_name16, &guid, NULL, &size, data);
> +	}
> +	if (ret == EFI_SUCCESS)
> +		show_efi_boot_opt_data(id, data);
> +	else if (ret == EFI_NOT_FOUND)
> +		printf("Boot%04X: not found\n", id);
> +
> +	free(data);
> +}
> +
> +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;
> +
> +	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);
> +
> +	if (!len)
> +		return CMD_RET_SUCCESS;
> +
> +	if (len < 0)
> +		return CMD_RET_FAILURE;
> +
> +	boot = variables;
> +	while (*boot) {
> +		value = strstr(boot, "Boot") + 4;
> +		id = (int)simple_strtoul(value, NULL, 16);
> +		show_efi_boot_opt(id);
> +		boot = strchr(boot, '\n');
> +		if (!*boot)
> +			break;
> +		boot++;
> +	}
> +	free(variables);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
> +static int show_efi_boot_order(void)
> +{
> +	efi_guid_t guid;
> +	u16 *bootorder = NULL;
> +	efi_uintn_t size;
> +	int num, i;
> +	char var_name[9];
> +	u16 var_name16[9], *p16;
> +	void *data;
> +	struct efi_load_option lo;
> +	char *label, *p;
> +	size_t label_len16, label_len;
> +	efi_status_t ret;
> +
> +	guid = efi_global_variable_guid;
> +	size = 0;
> +	ret = efi_get_variable(L"BootOrder", &guid, NULL, &size, NULL);
> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		bootorder = malloc(size);
> +		ret = efi_get_variable(L"BootOrder", &guid, NULL, &size,
> +				       bootorder);
> +	}
> +	if (ret == EFI_NOT_FOUND) {
> +		printf("BootOrder not defined\n");
> +		ret = CMD_RET_SUCCESS;
> +		goto out;
> +	} else if (ret != EFI_SUCCESS) {
> +		ret = CMD_RET_FAILURE;
> +		goto out;
> +	}
> +
> +	num = size / sizeof(u16);
> +	for (i = 0; i < num; i++) {
> +		sprintf(var_name, "Boot%04X", bootorder[i]);
> +		p16 = var_name16;
> +		utf8_utf16_strncpy(&p16, var_name, 9);
> +
> +		size = 0;
> +		ret = efi_get_variable(var_name16, &guid, NULL, &size, NULL);
> +		if (ret != EFI_BUFFER_TOO_SMALL) {
> +			printf("%2d: Boot%04X: (not defined)\n",
> +			       i + 1, bootorder[i]);
> +			continue;
> +		}
> +
> +		data = malloc(size);
> +		if (!data) {
> +			ret = CMD_RET_FAILURE;
> +			goto out;
> +		}
> +		ret = efi_get_variable(var_name16, &guid, NULL, &size, data);
> +		if (ret != EFI_SUCCESS) {
> +			free(data);
> +			ret = CMD_RET_FAILURE;
> +			goto out;
> +		}
> +
> +		efi_deserialize_load_option(&lo, data);
> +
> +		label_len16 = u16_strlen(lo.label);
> +		label_len = utf16_utf8_strnlen(lo.label, label_len16);
> +		label = malloc(label_len + 1);
> +		if (!label) {
> +			free(data);
> +			ret = CMD_RET_FAILURE;
> +			goto out;
> +		}
> +		p = label;
> +		utf16_utf8_strncpy(&p, lo.label, label_len16);
> +		printf("%2d: Boot%04X: %s\n", i + 1, bootorder[i], label);
> +		free(label);
> +
> +		free(data);
> +	}
> +out:
> +	free(bootorder);
> +
> +	return ret;
> +}
> +
> +static int do_efi_boot_next(cmd_tbl_t *cmdtp, int flag,
> +			    int argc, char * const argv[])
> +{
> +	u16 bootnext;
> +	efi_uintn_t size;
> +	char *endp;
> +	efi_guid_t guid;
> +	efi_status_t ret;
> +
> +	if (argc != 2)
> +		return CMD_RET_USAGE;
> +
> +	bootnext = (u16)simple_strtoul(argv[1], &endp, 16);
> +	if (*endp != '\0' || bootnext > 0xffff) {
> +		printf("invalid value: %s\n", argv[1]);
> +		ret = CMD_RET_FAILURE;
> +		goto out;
> +	}
> +
> +	guid = efi_global_variable_guid;
> +	size = sizeof(u16);
> +	ret = efi_set_variable(L"BootNext", &guid,
> +			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +			       EFI_VARIABLE_RUNTIME_ACCESS, size, &bootnext);
> +	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
> +out:
> +	return ret;
> +}
> +
> +static int do_efi_boot_order(cmd_tbl_t *cmdtp, int flag,
> +			     int argc, char * const argv[])
> +{
> +	u16 *bootorder = NULL;
> +	efi_uintn_t size;
> +	int id, i;
> +	char *endp;
> +	efi_guid_t guid;
> +	efi_status_t ret;
> +
> +	if (argc == 1)
> +		return show_efi_boot_order();
> +
> +	argc--;
> +	argv++;
> +
> +	size = argc * sizeof(u16);
> +	bootorder = malloc(size);
> +	if (!bootorder)
> +		return CMD_RET_FAILURE;
> +
> +	for (i = 0; i < argc; i++) {
> +		id = (int)simple_strtoul(argv[i], &endp, 16);
> +		if (*endp != '\0' || id > 0xffff) {
> +			printf("invalid value: %s\n", argv[i]);
> +			ret = CMD_RET_FAILURE;
> +			goto out;
> +		}
> +
> +		bootorder[i] = (u16)id;
> +	}
> +
> +	guid = efi_global_variable_guid;
> +	ret = efi_set_variable(L"BootOrder", &guid,
> +			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +			       EFI_VARIABLE_RUNTIME_ACCESS, size, bootorder);
> +	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
> +out:
> +	free(bootorder);
> +
> +	return ret;
> +}
> +
> +static cmd_tbl_t cmd_efidebug_boot_sub[] = {
> +	U_BOOT_CMD_MKENT(add, CONFIG_SYS_MAXARGS, 1, do_efi_boot_add, "", ""),
> +	U_BOOT_CMD_MKENT(rm, CONFIG_SYS_MAXARGS, 1, do_efi_boot_rm, "", ""),
> +	U_BOOT_CMD_MKENT(dump, CONFIG_SYS_MAXARGS, 1, do_efi_boot_dump, "", ""),
> +	U_BOOT_CMD_MKENT(next, CONFIG_SYS_MAXARGS, 1, do_efi_boot_next, "", ""),
> +	U_BOOT_CMD_MKENT(order, CONFIG_SYS_MAXARGS, 1, do_efi_boot_order,
> +			 "", ""),
> +};
> +
> +static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
> +			   int argc, char * const argv[])
> +{
> +	cmd_tbl_t *cp;
> +
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +
> +	argc--; argv++;
> +
> +	cp = find_cmd_tbl(argv[0], cmd_efidebug_boot_sub,
> +			  ARRAY_SIZE(cmd_efidebug_boot_sub));
> +	if (!cp)
> +		return CMD_RET_USAGE;
> +
> +	return cp->cmd(cmdtp, flag, argc, argv);
> +}
> +
> +static cmd_tbl_t cmd_efidebug_sub[] = {
> +	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
> +};
> +
> +/* Interpreter command to configure UEFI environment */
> +static int do_efidebug(cmd_tbl_t *cmdtp, int flag,
> +		       int argc, char * const argv[])
> +{
> +	cmd_tbl_t *cp;
> +	efi_status_t r;
> +
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +
> +	argc--; argv++;
> +
> +	/* Initialize UEFI drivers */
> +	r = efi_init_obj_list();
> +	if (r != EFI_SUCCESS) {
> +		printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> +		       r & ~EFI_ERROR_MASK);
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	cp = find_cmd_tbl(argv[0], cmd_efidebug_sub,
> +			  ARRAY_SIZE(cmd_efidebug_sub));
> +	if (!cp)
> +		return CMD_RET_USAGE;
> +
> +	return cp->cmd(cmdtp, flag, argc, argv);
> +}
> +
> +#ifdef CONFIG_SYS_LONGHELP
> +static char efidebug_help_text[] =
> +	"  - UEFI Shell-like interface to configure UEFI environment\n"
> +	"\n"
> +	"efidebug boot add <bootid> <label> <interface> <device>[:<part>] <file path> [<load options>]\n"
> +	"  - set UEFI BootXXXX variable\n"
> +	"    <load options> will be passed to UEFI application\n"
> +	"efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
> +	"  - delete UEFI BootXXXX variables\n"
> +	"efidebug boot dump\n"
> +	"  - show all UEFI BootXXXX variables\n"
> +	"efidebug boot next <bootid>\n"
> +	"  - set UEFI BootNext variable\n"
> +	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
> +	"  - set/show UEFI boot order\n"
> +	"\n";
> +#endif
> +
> +U_BOOT_CMD(
> +	efidebug, 10, 0, do_efidebug,
> +	"Configure UEFI environment",
> +	efidebug_help_text
> +);
> 

[U-Boot] [PATCH v6 3/7] cmd: efidebug: add devices command

[Heinrich Schuchardt]

On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
> "devices" command prints all the uefi variables on the system.
> 
> => efi devices
> Scanning disk ahci_scsi.id0lun0...
> Scanning disk ahci_scsi.id1lun0...
> Found 4 disks
> Device           Device Path
> ================ ====================
> 000000007ef07ea0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> 000000007ef00c10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
> 000000007ef00dd0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
> 000000007ef07be0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
> 000000007ef07510 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/efidebug.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 101 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index d836576cf6e0..8a7f775b117a 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -15,6 +15,102 @@
>  #include <search.h>
>  #include <linux/ctype.h>
>  
> +#define BS systab.boottime
> +

Please, add Sphinx style descriptions to all functions, their
parameters, and return values.

Why do you need this function? Can't you call LocateHandleBuffer() which
takes care of the allocation of the buffer?

> +static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
> +				    int *num)
> +{
> +	efi_handle_t *handles = NULL;
> +	efi_uintn_t size = 0;
> +	efi_status_t ret;
> +
> +	if (guid) {
> +		ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
> +					handles);

This will not work on i386. Please, always use EFI_CALL when invoking a
function which calls EFI_ENTRY().

> +		if (ret == EFI_BUFFER_TOO_SMALL) {
> +			handles = calloc(1, size);
> +			if (!handles)
> +				return -1;
> +
> +			ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
> +						handles);
> +		}
> +		if (ret != EFI_SUCCESS) {
> +			free(handles);

The handles are not allocated by malloc(). Use FreePool() here.

> +			return -1;
> +		}
> +	} else {
> +		ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
> +		if (ret == EFI_BUFFER_TOO_SMALL) {
> +			handles = calloc(1, size);
> +			if (!handles)
> +				return -1;
> +
> +			ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size,
> +						handles);
> +		}
> +		if (ret != EFI_SUCCESS) {
> +			free(handles);

Same here.

> +			return -1;
> +		}
> +	}
> +
> +	*handlesp = handles;
> +	*num = size / sizeof(efi_handle_t);
> +
> +	return 0;
> +}
> +
> +static int efi_get_device_handle_info(efi_handle_t handle, u16 **dev_path_text)
> +{
> +	struct efi_device_path *dp;
> +	efi_status_t ret;
> +
> +	ret = BS->open_protocol(handle, &efi_guid_device_path,
> +				(void **)&dp, NULL /* FIXME */, NULL,
> +				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +	if (ret == EFI_SUCCESS) {
> +		*dev_path_text = efi_dp_str(dp);
> +		return 0;
> +	} else {
> +		return -1;
> +	}
> +}
> +
> +#define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
> +
> +static const char spc[] = "                ";
> +static const char sep[] = "================";
> +
> +static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
> +			       int argc, char * const argv[])
> +{
> +	efi_handle_t *handles;
> +	u16 *dev_path_text;
> +	int num, i;
> +
> +	handles = NULL;
> +	num = 0;
> +	if (efi_get_handles_by_proto(NULL, &handles, &num))
> +		return CMD_RET_FAILURE;
> +
> +	if (!num)
> +		return CMD_RET_SUCCESS;
> +
> +	printf("Device%.*s Device Path\n", EFI_HANDLE_WIDTH - 6, spc);
> +	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
> +	for (i = 0; i < num; i++) {
> +		if (!efi_get_device_handle_info(handles[i], &dev_path_text)) {
> +			printf("%p %ls\n", handles[i], dev_path_text);
> +			efi_free_pool(dev_path_text);
> +		}
> +	}
> +
> +	free(handles);

FreePool()!

Best regards

Heinrich

> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
>  			   int argc, char * const argv[])
>  {
> @@ -406,6 +502,8 @@ static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
>  
>  static cmd_tbl_t cmd_efidebug_sub[] = {
>  	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
> +	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
> +			 "", ""),
>  };
>  
>  /* Interpreter command to configure UEFI environment */
> @@ -451,7 +549,9 @@ static char efidebug_help_text[] =
>  	"  - set UEFI BootNext variable\n"
>  	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
>  	"  - set/show UEFI boot order\n"
> -	"\n";
> +	"\n"
> +	"efidebug devices\n"
> +	"  - show uefi devices\n";
>  #endif
>  
>  U_BOOT_CMD(
> 

[U-Boot] [PATCH v6 4/7] cmd: efidebug: add drivers command

[Heinrich Schuchardt]

On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
> "drivers" command prints all the uefi drivers on the system.
> 
> => efi drivers
> Driver           Name                 Image Path
> ================ ==================== ====================
> 000000007ef003d0 <NULL>               <built-in>
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/efidebug.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 79 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 8a7f775b117a..1b788c76a895 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -111,6 +111,80 @@ static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
>  	return CMD_RET_SUCCESS;
>  }
>  
> +static int efi_get_driver_handle_info(efi_handle_t handle, u16 **driver_name,
> +				      u16 **image_path)
> +{
> +	struct efi_handler *handler;
> +	struct efi_loaded_image *image;
> +	efi_status_t ret;
> +
> +	/*
> +	 * driver name
> +	 * TODO: support EFI_COMPONENT_NAME2_PROTOCOL
> +	 */
> +	*driver_name = NULL;
> +
> +	/* image name */
> +	ret = efi_search_protocol(handle, &efi_guid_loaded_image, &handler);
> +	if (ret != EFI_SUCCESS) {
> +		*image_path = NULL;
> +		return 0;
> +	}
> +
> +	image = handler->protocol_interface;
> +	*image_path = efi_dp_str(image->file_path);
> +
> +	return 0;
> +}
> +
> +static int do_efi_show_drivers(cmd_tbl_t *cmdtp, int flag,
> +			       int argc, char * const argv[])
> +{
> +	efi_handle_t *handles = NULL, *handle;
> +	efi_uintn_t size = 0;
> +	u16 *driver_name, *image_path_text;
> +	efi_status_t ret;
> +	int i;
> +

You are duplicating the logic of LocateHandleBuffer(). Why?

> +	ret = BS->locate_handle(BY_PROTOCOL, &efi_guid_driver_binding_protocol,
> +				NULL, &size, NULL);
> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		handles = calloc(1, size);
> +		if (!handles)
> +			return CMD_RET_FAILURE;
> +
> +		ret = BS->locate_handle(BY_PROTOCOL,
> +					&efi_guid_driver_binding_protocol,
> +					NULL, &size, handles);
> +	}
> +	if (ret != EFI_SUCCESS) {
> +		free(handles);
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	printf("Driver%.*s Name                 Image Path\n",
> +	       EFI_HANDLE_WIDTH - 6, spc);
> +	printf("%.*s ==================== ====================\n",
> +	       EFI_HANDLE_WIDTH, sep);
> +	handle = handles;
> +	for (i = 0; i < size / sizeof(*handle); i++) {
> +		if (!efi_get_driver_handle_info(*handle, &driver_name,
> +						&image_path_text)) {
> +			if (image_path_text)
> +				printf("%p %-20ls %ls\n",
> +				       *handle, driver_name, image_path_text);
> +			else
> +				printf("%p %-20ls <built-in>\n",
> +				       *handle, driver_name);
> +			efi_free_pool(driver_name);
> +			efi_free_pool(image_path_text);
> +		}
> +		handle++;
> +	}
> +

You are leaking 'handles' here.

Best regards

Heinrich

> +	return CMD_RET_SUCCESS;
> +}
> +
>  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
>  			   int argc, char * const argv[])
>  {
> @@ -504,6 +578,8 @@ static cmd_tbl_t cmd_efidebug_sub[] = {
>  	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
>  	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
>  			 "", ""),
> +	U_BOOT_CMD_MKENT(drivers, CONFIG_SYS_MAXARGS, 1, do_efi_show_drivers,
> +			 "", ""),
>  };
>  
>  /* Interpreter command to configure UEFI environment */
> @@ -551,7 +627,9 @@ static char efidebug_help_text[] =
>  	"  - set/show UEFI boot order\n"
>  	"\n"
>  	"efidebug devices\n"
> -	"  - show uefi devices\n";
> +	"  - show uefi devices\n"
> +	"efidebug drivers\n"
> +	"  - show uefi drivers\n";
>  #endif
>  
>  U_BOOT_CMD(
> 

[U-Boot] [PATCH v6 6/7] cmd: efidebug: add images command

[Heinrich Schuchardt]

On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
> "images" command prints loaded images-related information.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>  cmd/efidebug.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index a1852e8ea4b9..81ab3654f746 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -301,6 +301,14 @@ static int do_efi_show_handles(cmd_tbl_t *cmdtp, int flag,
>  	return CMD_RET_SUCCESS;
>  }
>  
> +static int do_efi_show_images(cmd_tbl_t *cmdtp, int flag,
> +			      int argc, char * const argv[])
> +{
> +	efi_print_image_infos(NULL);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
>  			   int argc, char * const argv[])
>  {
> @@ -698,6 +706,8 @@ static cmd_tbl_t cmd_efidebug_sub[] = {
>  			 "", ""),
>  	U_BOOT_CMD_MKENT(dh, CONFIG_SYS_MAXARGS, 1, do_efi_show_handles,
>  			 "", ""),
> +	U_BOOT_CMD_MKENT(images, CONFIG_SYS_MAXARGS, 1, do_efi_show_images,
> +			 "", ""),
>  };
>  
>  /* Interpreter command to configure UEFI environment */
> @@ -749,7 +759,9 @@ static char efidebug_help_text[] =
>  	"efidebug drivers\n"
>  	"  - show uefi drivers\n"
>  	"efidebug dh\n"
> -	"  - show uefi handles\n";
> +	"  - show uefi handles\n"
> +	"efidebug images\n"
> +	"  - show loaded images\n";
>  #endif
>  
>  U_BOOT_CMD(
> 

[U-Boot] [PATCH v6 3/7] cmd: efidebug: add devices command

[Heinrich Schuchardt]

On 2/19/19 8:46 PM, Heinrich Schuchardt wrote:
> On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
>> "devices" command prints all the uefi variables on the system.
>>
>> => efi devices
>> Scanning disk ahci_scsi.id0lun0...
>> Scanning disk ahci_scsi.id1lun0...
>> Found 4 disks
>> Device           Device Path
>> ================ ====================
>> 000000007ef07ea0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
>> 000000007ef00c10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
>> 000000007ef00dd0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
>> 000000007ef07be0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
>> 000000007ef07510 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>  cmd/efidebug.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 101 insertions(+), 1 deletion(-)
>>
>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
>> index d836576cf6e0..8a7f775b117a 100644
>> --- a/cmd/efidebug.c
>> +++ b/cmd/efidebug.c
>> @@ -15,6 +15,102 @@
>>  #include <search.h>
>>  #include <linux/ctype.h>
>>  
>> +#define BS systab.boottime
>> +
> 
> Please, add Sphinx style descriptions to all functions, their
> parameters, and return values.
> 
> Why do you need this function? Can't you call LocateHandleBuffer() which
> takes care of the allocation of the buffer?
> 
>> +static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
>> +				    int *num)
>> +{
>> +	efi_handle_t *handles = NULL;
>> +	efi_uintn_t size = 0;
>> +	efi_status_t ret;
>> +
>> +	if (guid) {
>> +		ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
>> +					handles);
> 
> This will not work on i386. Please, always use EFI_CALL when invoking a
> function which calls EFI_ENTRY().
> 
>> +		if (ret == EFI_BUFFER_TOO_SMALL) {
>> +			handles = calloc(1, size);
>> +			if (!handles)
>> +				return -1;
>> +
>> +			ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
>> +						handles);
>> +		}
>> +		if (ret != EFI_SUCCESS) {
>> +			free(handles);
> 
> The handles are not allocated by malloc(). Use FreePool() here.

Sorry I read this wrong. If you calloc(), free() is ok. But I would
rather use LocateHandleBuffer() and FreePool() further down.

Regards

Heinrich

> 
>> +			return -1;
>> +		}
>> +	} else {
>> +		ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
>> +		if (ret == EFI_BUFFER_TOO_SMALL) {
>> +			handles = calloc(1, size);
>> +			if (!handles)
>> +				return -1;
>> +
>> +			ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size,
>> +						handles);
>> +		}
>> +		if (ret != EFI_SUCCESS) {
>> +			free(handles);
> 
> Same here.
> 
>> +			return -1;
>> +		}
>> +	}
>> +
>> +	*handlesp = handles;
>> +	*num = size / sizeof(efi_handle_t);
>> +
>> +	return 0;
>> +}
>> +
>> +static int efi_get_device_handle_info(efi_handle_t handle, u16 **dev_path_text)
>> +{
>> +	struct efi_device_path *dp;
>> +	efi_status_t ret;
>> +
>> +	ret = BS->open_protocol(handle, &efi_guid_device_path,
>> +				(void **)&dp, NULL /* FIXME */, NULL,
>> +				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>> +	if (ret == EFI_SUCCESS) {
>> +		*dev_path_text = efi_dp_str(dp);
>> +		return 0;
>> +	} else {
>> +		return -1;
>> +	}
>> +}
>> +
>> +#define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
>> +
>> +static const char spc[] = "                ";
>> +static const char sep[] = "================";
>> +
>> +static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
>> +			       int argc, char * const argv[])
>> +{
>> +	efi_handle_t *handles;
>> +	u16 *dev_path_text;
>> +	int num, i;
>> +
>> +	handles = NULL;
>> +	num = 0;
>> +	if (efi_get_handles_by_proto(NULL, &handles, &num))
>> +		return CMD_RET_FAILURE;
>> +
>> +	if (!num)
>> +		return CMD_RET_SUCCESS;
>> +
>> +	printf("Device%.*s Device Path\n", EFI_HANDLE_WIDTH - 6, spc);
>> +	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
>> +	for (i = 0; i < num; i++) {
>> +		if (!efi_get_device_handle_info(handles[i], &dev_path_text)) {
>> +			printf("%p %ls\n", handles[i], dev_path_text);
>> +			efi_free_pool(dev_path_text);
>> +		}
>> +	}
>> +
>> +	free(handles);
> 
> FreePool()!
> 
> Best regards
> 
> Heinrich
> 
>> +
>> +	return CMD_RET_SUCCESS;
>> +}
>> +
>>  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
>>  			   int argc, char * const argv[])
>>  {
>> @@ -406,6 +502,8 @@ static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
>>  
>>  static cmd_tbl_t cmd_efidebug_sub[] = {
>>  	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
>> +	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
>> +			 "", ""),
>>  };
>>  
>>  /* Interpreter command to configure UEFI environment */
>> @@ -451,7 +549,9 @@ static char efidebug_help_text[] =
>>  	"  - set UEFI BootNext variable\n"
>>  	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
>>  	"  - set/show UEFI boot order\n"
>> -	"\n";
>> +	"\n"
>> +	"efidebug devices\n"
>> +	"  - show uefi devices\n";
>>  #endif
>>  
>>  U_BOOT_CMD(
>>
> 
> 

[U-Boot] [PATCH v6 7/7] cmd: efidebug: add memmap command

[AKASHI Takahiro]

On Tue, Feb 19, 2019 at 08:11:05PM +0100, Heinrich Schuchardt wrote:
> On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
> > "memmap" command prints uefi-specific memory map information.
> > => efi memmap
> > Type             Start            End              Attributes
> > ================ ================ ================ ==========
> > CONVENTIONAL     0000000040000000-000000007de27000 WB
> > RUNTIME DATA     000000007de27000-000000007de28000 WB|RT
> > RESERVED         000000007de28000-000000007de2a000 WB
> > RUNTIME DATA     000000007de2a000-000000007de2b000 WB|RT
> > RESERVED         000000007de2b000-000000007de2c000 WB
> > RUNTIME DATA     000000007de2c000-000000007de2d000 WB|RT
> > LOADER DATA      000000007de2d000-000000007ff37000 WB
> > RUNTIME CODE     000000007ff37000-000000007ff38000 WB|RT
> > LOADER DATA      000000007ff38000-0000000080000000 WB
> 
> On qemu_arm_defconfig I got this ouptut:
> 
> => efidebug memmap
> Type             Start    End      Attributes
> ================ ======== ======== ==========
> CONVENTIONAL     0000000040000000-000000007ddf8000 WB
> BOOT DATA        000000007ddf8000-000000007ddfd000 WB
> RUNTIME DATA     000000007ddfd000-000000007ddfe000 WB|RT
> RESERVED         000000007ddfe000-000000007ddff000 WB
> RUNTIME DATA     000000007ddff000-000000007de00000 WB|RT
> LOADER DATA      000000007de00000-000000007ff42000 WB
> RUNTIME CODE     000000007ff42000-000000007ff43000 WB|RT
> LOADER DATA      000000007ff43000-0000000080000000 WB
> 
> Something is wrong with you format codes.
> 
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/efidebug.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 99 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> > index 81ab3654f746..39398669e18f 100644
> > --- a/cmd/efidebug.c
> > +++ b/cmd/efidebug.c
> > @@ -309,6 +309,100 @@ static int do_efi_show_images(cmd_tbl_t *cmdtp, int flag,
> >  	return CMD_RET_SUCCESS;
> >  }
> >  
> > +static const char * const efi_mem_type_string[] = {
> > +	[EFI_RESERVED_MEMORY_TYPE] = "RESERVED",
> > +	[EFI_LOADER_CODE] = "LOADER CODE",
> > +	[EFI_LOADER_DATA] = "LOADER DATA",
> > +	[EFI_BOOT_SERVICES_CODE] = "BOOT CODE",
> > +	[EFI_BOOT_SERVICES_DATA] = "BOOT DATA",
> > +	[EFI_RUNTIME_SERVICES_CODE] = "RUNTIME CODE",
> > +	[EFI_RUNTIME_SERVICES_DATA] = "RUNTIME DATA",
> > +	[EFI_CONVENTIONAL_MEMORY] = "CONVENTIONAL",
> > +	[EFI_UNUSABLE_MEMORY] = "UNUSABLE MEM",
> > +	[EFI_ACPI_RECLAIM_MEMORY] = "ACPI RECLAIM MEM",
> > +	[EFI_ACPI_MEMORY_NVS] = "ACPI NVS",
> > +	[EFI_MMAP_IO] = "IO",
> > +	[EFI_MMAP_IO_PORT] = "IO PORT",
> > +	[EFI_PAL_CODE] = "PAL",
> > +};
> > +
> > +static const struct efi_mem_attrs {
> > +	const u64 bit;
> > +	const char *text;
> > +} efi_mem_attrs[] = {
> > +	{EFI_MEMORY_UC, "UC"},
> > +	{EFI_MEMORY_UC, "UC"},
> > +	{EFI_MEMORY_WC, "WC"},
> > +	{EFI_MEMORY_WT, "WT"},
> > +	{EFI_MEMORY_WB, "WB"},
> > +	{EFI_MEMORY_UCE, "UCE"},
> > +	{EFI_MEMORY_WP, "WP"},
> > +	{EFI_MEMORY_RP, "RP"},
> > +	{EFI_MEMORY_XP, "WP"},
> > +	{EFI_MEMORY_NV, "NV"},
> > +	{EFI_MEMORY_MORE_RELIABLE, "REL"},
> > +	{EFI_MEMORY_RO, "RO"},
> > +	{EFI_MEMORY_RUNTIME, "RT"},
> > +};
> > +
> > +static void print_memory_attributes(u64 attributes)
> > +{
> > +	int sep, i;
> > +
> > +	for (sep = 0, i = 0; i < ARRAY_SIZE(efi_mem_attrs); i++)
> > +		if (attributes & efi_mem_attrs[i].bit) {
> > +			if (sep) {
> > +				putc('|');
> > +			} else {
> > +				putc(' ');
> > +				sep = 1;
> > +			}
> > +			puts(efi_mem_attrs[i].text);
> > +		}
> > +}
> > +
> > +static int do_efi_show_memmap(cmd_tbl_t *cmdtp, int flag,
> > +			      int argc, char * const argv[])
> > +{
> > +	struct efi_mem_desc *memmap = NULL, *map;
> > +	efi_uintn_t map_size = 0;
> > +	const char *type;
> > +	int i;
> > +	efi_status_t ret;
> > +
> > +	ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
> > +	if (ret == EFI_BUFFER_TOO_SMALL) {
> > +		memmap = malloc(map_size);
> > +		if (!memmap)
> > +			return CMD_RET_FAILURE;
> > +		ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
> > +	}
> > +	if (ret != EFI_SUCCESS) {
> > +		free(memmap);
> > +		return CMD_RET_FAILURE;
> > +	}
> > +
> > +	printf("Type             Start%.*s End%.*s Attributes\n",
> > +	       EFI_HANDLE_WIDTH - 5, spc, EFI_HANDLE_WIDTH - 3, spc);
> > +	printf("================ %.*s %.*s ==========\n",
> > +	       EFI_HANDLE_WIDTH, sep, EFI_HANDLE_WIDTH, sep);
> > +	for (i = 0, map = memmap; i < map_size / sizeof(*map); map++, i++) {
> > +		if (map->type < EFI_MAX_MEMORY_TYPE)
> > +			type = efi_mem_type_string[map->type];
> > +		else
> > +			type = "(unknown)";
> > +		printf("%-16s %016llx-%016llx", type, map->physical_start,
> 
> Width 16 is ok on 64bit systems but not on 32bit ones.

It sounds reasonable, but the reality is not so trivial.
In struct efi_mem_desc, physical_start is defined as efi_physical_addr_t,
and efi_physical_addr_t is defined as u64 whatever the arch is.

So how do we know the system has 64-bit address space?

There is a config, CONFIG_PHYS_64BIT, but it is never defined on x86(_64).

So workable but ugly solution to meet your requirement would be
a)
        if (sizeof(phys_addr_t) >= 8)
		printf("%-16s %016llx-%016llx", ...);
        else
		printf("%-16s %08llx-%08llx", ...);
   or
b)
        printf("%-16s %p-%p", type, (void *)map->physical_start, ...);
        (I don't think "void *" always reflects *physical* bit width
        on LPAE arch though.)

Which do you like better?

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +		       map->physical_start + map->num_pages * EFI_PAGE_SIZE);
> > +
> > +		print_memory_attributes(map->attribute);
> > +		putc('\n');
> > +	}
> > +
> > +	free(memmap);
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> >  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
> >  			   int argc, char * const argv[])
> >  {
> > @@ -708,6 +802,8 @@ static cmd_tbl_t cmd_efidebug_sub[] = {
> >  			 "", ""),
> >  	U_BOOT_CMD_MKENT(images, CONFIG_SYS_MAXARGS, 1, do_efi_show_images,
> >  			 "", ""),
> > +	U_BOOT_CMD_MKENT(memmap, CONFIG_SYS_MAXARGS, 1, do_efi_show_memmap,
> > +			 "", ""),
> >  };
> >  
> >  /* Interpreter command to configure UEFI environment */
> > @@ -761,7 +857,9 @@ static char efidebug_help_text[] =
> >  	"efidebug dh\n"
> >  	"  - show uefi handles\n"
> >  	"efidebug images\n"
> > -	"  - show loaded images\n";
> > +	"  - show loaded images\n"
> > +	"efidebug memmap\n"
> > +	"  - show uefi memory map\n";
> >  #endif
> >  
> >  U_BOOT_CMD(
> > 
> 

[U-Boot] [PATCH v6 3/7] cmd: efidebug: add devices command

[AKASHI Takahiro]

On Tue, Feb 19, 2019 at 08:46:52PM +0100, Heinrich Schuchardt wrote:
> On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
> > "devices" command prints all the uefi variables on the system.
> > 
> > => efi devices
> > Scanning disk ahci_scsi.id0lun0...
> > Scanning disk ahci_scsi.id1lun0...
> > Found 4 disks
> > Device           Device Path
> > ================ ====================
> > 000000007ef07ea0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> > 000000007ef00c10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
> > 000000007ef00dd0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
> > 000000007ef07be0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
> > 000000007ef07510 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/efidebug.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 101 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> > index d836576cf6e0..8a7f775b117a 100644
> > --- a/cmd/efidebug.c
> > +++ b/cmd/efidebug.c
> > @@ -15,6 +15,102 @@
> >  #include <search.h>
> >  #include <linux/ctype.h>
> >  
> > +#define BS systab.boottime
> > +
> 
> Please, add Sphinx style descriptions to all functions, their
> parameters, and return values.

Sure, but please clearify what you mean by "all."
Do you expect descriptions for exactly all the functions,
even for some helper functions of a few lines of code?

> Why do you need this function? Can't you call LocateHandleBuffer() which
> takes care of the allocation of the buffer?
> 
> > +static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
> > +				    int *num)

Right. I will use locate_handle_buffer here.
But why does UEFI spec define functionally-duplicated functions,
LocateHandle() and LocateHandleBuffer()?

> > +{
> > +	efi_handle_t *handles = NULL;
> > +	efi_uintn_t size = 0;
> > +	efi_status_t ret;
> > +
> > +	if (guid) {
> > +		ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
> > +					handles);
> 
> This will not work on i386. Please, always use EFI_CALL when invoking a
> function which calls EFI_ENTRY().

OK, but let me make sure.
We don't have to use EFI_CALL() for any of (internal) efi_xxx() functions,
say, efi_add_protocol(). Right?

> > +		if (ret == EFI_BUFFER_TOO_SMALL) {
> > +			handles = calloc(1, size);
> > +			if (!handles)
> > +				return -1;
> > +
> > +			ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
> > +						handles);
> > +		}
> > +		if (ret != EFI_SUCCESS) {
> > +			free(handles);
> 
> The handles are not allocated by malloc(). Use FreePool() here.

Will take care of that along with the change above.

-Takahiro Akashi


> > +			return -1;
> > +		}
> > +	} else {
> > +		ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
> > +		if (ret == EFI_BUFFER_TOO_SMALL) {
> > +			handles = calloc(1, size);
> > +			if (!handles)
> > +				return -1;
> > +
> > +			ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size,
> > +						handles);
> > +		}
> > +		if (ret != EFI_SUCCESS) {
> > +			free(handles);
> 
> Same here.
> 
> > +			return -1;
> > +		}
> > +	}
> > +
> > +	*handlesp = handles;
> > +	*num = size / sizeof(efi_handle_t);
> > +
> > +	return 0;
> > +}
> > +
> > +static int efi_get_device_handle_info(efi_handle_t handle, u16 **dev_path_text)
> > +{
> > +	struct efi_device_path *dp;
> > +	efi_status_t ret;
> > +
> > +	ret = BS->open_protocol(handle, &efi_guid_device_path,
> > +				(void **)&dp, NULL /* FIXME */, NULL,
> > +				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > +	if (ret == EFI_SUCCESS) {
> > +		*dev_path_text = efi_dp_str(dp);
> > +		return 0;
> > +	} else {
> > +		return -1;
> > +	}
> > +}
> > +
> > +#define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
> > +
> > +static const char spc[] = "                ";
> > +static const char sep[] = "================";
> > +
> > +static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
> > +			       int argc, char * const argv[])
> > +{
> > +	efi_handle_t *handles;
> > +	u16 *dev_path_text;
> > +	int num, i;
> > +
> > +	handles = NULL;
> > +	num = 0;
> > +	if (efi_get_handles_by_proto(NULL, &handles, &num))
> > +		return CMD_RET_FAILURE;
> > +
> > +	if (!num)
> > +		return CMD_RET_SUCCESS;
> > +
> > +	printf("Device%.*s Device Path\n", EFI_HANDLE_WIDTH - 6, spc);
> > +	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
> > +	for (i = 0; i < num; i++) {
> > +		if (!efi_get_device_handle_info(handles[i], &dev_path_text)) {
> > +			printf("%p %ls\n", handles[i], dev_path_text);
> > +			efi_free_pool(dev_path_text);
> > +		}
> > +	}
> > +
> > +	free(handles);
> 
> FreePool()!
> 
> Best regards
> 
> Heinrich
> 
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> >  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
> >  			   int argc, char * const argv[])
> >  {
> > @@ -406,6 +502,8 @@ static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
> >  
> >  static cmd_tbl_t cmd_efidebug_sub[] = {
> >  	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
> > +	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
> > +			 "", ""),
> >  };
> >  
> >  /* Interpreter command to configure UEFI environment */
> > @@ -451,7 +549,9 @@ static char efidebug_help_text[] =
> >  	"  - set UEFI BootNext variable\n"
> >  	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
> >  	"  - set/show UEFI boot order\n"
> > -	"\n";
> > +	"\n"
> > +	"efidebug devices\n"
> > +	"  - show uefi devices\n";
> >  #endif
> >  
> >  U_BOOT_CMD(
> > 
> 

[U-Boot] [PATCH v6 4/7] cmd: efidebug: add drivers command

[AKASHI Takahiro]

On Tue, Feb 19, 2019 at 08:49:30PM +0100, Heinrich Schuchardt wrote:
> On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
> > "drivers" command prints all the uefi drivers on the system.
> > 
> > => efi drivers
> > Driver           Name                 Image Path
> > ================ ==================== ====================
> > 000000007ef003d0 <NULL>               <built-in>
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/efidebug.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 79 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> > index 8a7f775b117a..1b788c76a895 100644
> > --- a/cmd/efidebug.c
> > +++ b/cmd/efidebug.c
> > @@ -111,6 +111,80 @@ static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
> >  	return CMD_RET_SUCCESS;
> >  }
> >  
> > +static int efi_get_driver_handle_info(efi_handle_t handle, u16 **driver_name,
> > +				      u16 **image_path)
> > +{
> > +	struct efi_handler *handler;
> > +	struct efi_loaded_image *image;
> > +	efi_status_t ret;
> > +
> > +	/*
> > +	 * driver name
> > +	 * TODO: support EFI_COMPONENT_NAME2_PROTOCOL
> > +	 */
> > +	*driver_name = NULL;
> > +
> > +	/* image name */
> > +	ret = efi_search_protocol(handle, &efi_guid_loaded_image, &handler);
> > +	if (ret != EFI_SUCCESS) {
> > +		*image_path = NULL;
> > +		return 0;
> > +	}
> > +
> > +	image = handler->protocol_interface;
> > +	*image_path = efi_dp_str(image->file_path);
> > +
> > +	return 0;
> > +}
> > +
> > +static int do_efi_show_drivers(cmd_tbl_t *cmdtp, int flag,
> > +			       int argc, char * const argv[])
> > +{
> > +	efi_handle_t *handles = NULL, *handle;
> > +	efi_uintn_t size = 0;
> > +	u16 *driver_name, *image_path_text;
> > +	efi_status_t ret;
> > +	int i;
> > +
> 
> You are duplicating the logic of LocateHandleBuffer(). Why?

Will fix.

> > +	ret = BS->locate_handle(BY_PROTOCOL, &efi_guid_driver_binding_protocol,
> > +				NULL, &size, NULL);
> > +	if (ret == EFI_BUFFER_TOO_SMALL) {
> > +		handles = calloc(1, size);
> > +		if (!handles)
> > +			return CMD_RET_FAILURE;
> > +
> > +		ret = BS->locate_handle(BY_PROTOCOL,
> > +					&efi_guid_driver_binding_protocol,
> > +					NULL, &size, handles);
> > +	}
> > +	if (ret != EFI_SUCCESS) {
> > +		free(handles);
> > +		return CMD_RET_FAILURE;
> > +	}
> > +
> > +	printf("Driver%.*s Name                 Image Path\n",
> > +	       EFI_HANDLE_WIDTH - 6, spc);
> > +	printf("%.*s ==================== ====================\n",
> > +	       EFI_HANDLE_WIDTH, sep);
> > +	handle = handles;
> > +	for (i = 0; i < size / sizeof(*handle); i++) {
> > +		if (!efi_get_driver_handle_info(*handle, &driver_name,
> > +						&image_path_text)) {
> > +			if (image_path_text)
> > +				printf("%p %-20ls %ls\n",
> > +				       *handle, driver_name, image_path_text);
> > +			else
> > +				printf("%p %-20ls <built-in>\n",
> > +				       *handle, driver_name);
> > +			efi_free_pool(driver_name);
> > +			efi_free_pool(image_path_text);
> > +		}
> > +		handle++;
> > +	}
> > +
> 
> You are leaking 'handles' here.

Will fix.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> >  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
> >  			   int argc, char * const argv[])
> >  {
> > @@ -504,6 +578,8 @@ static cmd_tbl_t cmd_efidebug_sub[] = {
> >  	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
> >  	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
> >  			 "", ""),
> > +	U_BOOT_CMD_MKENT(drivers, CONFIG_SYS_MAXARGS, 1, do_efi_show_drivers,
> > +			 "", ""),
> >  };
> >  
> >  /* Interpreter command to configure UEFI environment */
> > @@ -551,7 +627,9 @@ static char efidebug_help_text[] =
> >  	"  - set/show UEFI boot order\n"
> >  	"\n"
> >  	"efidebug devices\n"
> > -	"  - show uefi devices\n";
> > +	"  - show uefi devices\n"
> > +	"efidebug drivers\n"
> > +	"  - show uefi drivers\n";
> >  #endif
> >  
> >  U_BOOT_CMD(
> > 
> 

[U-Boot] [PATCH v6 2/7] cmd: add efidebug command

[AKASHI Takahiro]

On Tue, Feb 19, 2019 at 08:32:52PM +0100, Heinrich Schuchardt wrote:
> On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
> > Currently, there is no easy way to add or modify UEFI variables.
> > In particular, bootmgr supports BootOrder/BootXXXX variables, it is
> > quite hard to define them as u-boot variables because they are represented
> > in a complicated and encoded format.
> > 
> > The new command, efidebug, helps address these issues and give us
> > more friendly interfaces:
> >  * efidebug boot add: add BootXXXX variable
> >  * efidebug boot rm: remove BootXXXX variable
> >  * efidebug boot dump: display all BootXXXX variables
> >  * efidebug boot next: set BootNext variable
> >  * efidebug boot order: set/display a boot order (BootOrder)
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> I could not make this work on qemu_arm_defconfig:
> 
> Usage:
> efidebug boot add <bootid> <label> <interface> <device>[:<part>] <file
> path> [<load options>]
> 
> => efidebug boot add 00AA 'fancy label' mmc 0:1 wonder.efi '--do-it'
> => efidebug boot dump
> =>
> 
> I would expect either an error or an output of `dump`.
> 
> As the UEFI spec teaches: "Each Boot####  variable is the name “Boot”
> appended with a unique four digit hexadecimal number." So 00AA should be
> a valid id.

It's not a problem.
The real issue is that you don't have "mmc 0:1" on you system
when typing this command.

This error take places because this command internally uses
efi_dp_from_name(), hence blk_get_device_part_str(), which
requires that a block device does exist and it can be accessible
to see a partition table.

I think that this is one of major issues in current device path
implementation. We never know how to convert "mmc 0:1" to a device
path without having the named  device.

Anyhow, I will add some error message in such a case.

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> > ---
> >  MAINTAINERS    |   1 +
> >  cmd/Kconfig    |  10 ++
> >  cmd/Makefile   |   1 +
> >  cmd/efidebug.c | 461 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 473 insertions(+)
> >  create mode 100644 cmd/efidebug.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 22ac686ab2d6..7ad8c01a3b93 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -438,6 +438,7 @@ F:	lib/efi*/
> >  F:	test/py/tests/test_efi*
> >  F:	test/unicode_ut.c
> >  F:	cmd/bootefi.c
> > +F:	cmd/efidebug.c
> >  F:	cmd/nvedit_efi.c
> >  F:	tools/file2include.c
> >  
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 812a7eb9b74b..f0405a490635 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -1403,6 +1403,16 @@ config CMD_DISPLAY
> >  	  displayed on a simple board-specific display. Implement
> >  	  display_putc() to use it.
> >  
> > +config CMD_EFIDEBUG
> > +	bool "efidebug - display/configure UEFI environment"
> > +	depends on EFI_LOADER
> > +	default n
> > +	help
> > +	  Enable the 'efidebug' command which provides a subset of UEFI
> > +	  shell utility with simplified functionality. It will be useful
> > +	  particularly for managing boot parameters as  well as examining
> > +	  various EFI status for debugging.
> > +
> >  config CMD_LED
> >  	bool "led"
> >  	default y if LED
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index 142e0ee222ca..48f2742168be 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -51,6 +51,7 @@ obj-$(CONFIG_CMD_ECHO) += echo.o
> >  obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o
> >  obj-$(CONFIG_CMD_EEPROM) += eeprom.o
> >  obj-$(CONFIG_EFI_STUB) += efi.o
> > +obj-$(CONFIG_CMD_EFIDEBUG) += efidebug.o
> >  obj-$(CONFIG_CMD_ELF) += elf.o
> >  obj-$(CONFIG_HUSH_PARSER) += exit.o
> >  obj-$(CONFIG_CMD_EXT4) += ext4.o
> > diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> > new file mode 100644
> > index 000000000000..d836576cf6e0
> > --- /dev/null
> > +++ b/cmd/efidebug.c
> > @@ -0,0 +1,461 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  UEFI Shell-like command
> > + *
> > + *  Copyright (c) 2018 AKASHI Takahiro, Linaro Limited
> > + */
> > +
> > +#include <charset.h>
> > +#include <common.h>
> > +#include <command.h>
> > +#include <efi_loader.h>
> > +#include <environment.h>
> > +#include <exports.h>
> > +#include <malloc.h>
> > +#include <search.h>
> > +#include <linux/ctype.h>
> > +
> > +static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
> > +			   int argc, char * const argv[])
> > +{
> > +	int id;
> > +	char *endp;
> > +	char var_name[9];
> > +	u16 var_name16[9], *p;
> > +	efi_guid_t guid;
> > +	size_t label_len, label_len16;
> > +	u16 *label;
> > +	struct efi_device_path *device_path = NULL, *file_path = NULL;
> > +	struct efi_load_option lo;
> > +	void *data = NULL;
> > +	efi_uintn_t size;
> > +	int ret;
> > +
> > +	if (argc < 6 || argc > 7)
> > +		return CMD_RET_USAGE;
> > +
> > +	id = (int)simple_strtoul(argv[1], &endp, 16);
> > +	if (*endp != '\0' || id > 0xffff)
> > +		return CMD_RET_FAILURE;
> > +
> > +	sprintf(var_name, "Boot%04X", id);
> > +	p = var_name16;
> > +	utf8_utf16_strncpy(&p, var_name, 9);
> > +
> > +	guid = efi_global_variable_guid;
> > +
> > +	/* attributes */
> > +	lo.attributes = 0x1; /* always ACTIVE */
> > +
> > +	/* label */
> > +	label_len = strlen(argv[2]);
> > +	label_len16 = utf8_utf16_strnlen(argv[2], label_len);
> > +	label = malloc((label_len16 + 1) * sizeof(u16));
> > +	if (!label)
> > +		return CMD_RET_FAILURE;
> > +	lo.label = label; /* label will be changed below */
> > +	utf8_utf16_strncpy(&label, argv[2], label_len);
> > +
> > +	/* file path */
> > +	ret = efi_dp_from_name(argv[3], argv[4], argv[5], &device_path,
> > +			       &file_path);
> > +	if (ret != EFI_SUCCESS) {
> > +		ret = CMD_RET_FAILURE;
> > +		goto out;
> > +	}
> > +	lo.file_path = file_path;
> > +	lo.file_path_length = efi_dp_size(file_path)
> > +				+ sizeof(struct efi_device_path); /* for END */
> > +
> > +	/* optional data */
> > +	lo.optional_data = (u8 *)(argc == 6 ? "" : argv[6]);
> > +
> > +	size = efi_serialize_load_option(&lo, (u8 **)&data);
> > +	if (!size) {
> > +		ret = CMD_RET_FAILURE;
> > +		goto out;
> > +	}
> > +
> > +	ret = efi_set_variable(var_name16, &guid,
> > +			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +			       EFI_VARIABLE_RUNTIME_ACCESS, size, data);
> > +	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
> > +out:
> > +	free(data);
> > +	efi_free_pool(device_path);
> > +	efi_free_pool(file_path);
> > +	free(lo.label);
> > +
> > +	return ret;
> > +}
> > +
> > +static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag,
> > +			  int argc, char * const argv[])
> > +{
> > +	efi_guid_t guid;
> > +	int id, i;
> > +	char *endp;
> > +	char var_name[9];
> > +	u16 var_name16[9];
> > +	efi_status_t ret;
> > +
> > +	if (argc == 1)
> > +		return CMD_RET_USAGE;
> > +
> > +	guid = efi_global_variable_guid;
> > +	for (i = 1; i < argc; i++, argv++) {
> > +		id = (int)simple_strtoul(argv[1], &endp, 16);
> > +		if (*endp != '\0' || id > 0xffff)
> > +			return CMD_RET_FAILURE;
> > +
> > +		sprintf(var_name, "Boot%04X", id);
> > +		utf8_utf16_strncpy((u16 **)&var_name16, var_name, 9);
> > +
> > +		ret = efi_set_variable(var_name16, &guid, 0, 0, NULL);
> > +		if (ret) {
> > +			printf("cannot remove Boot%04X", id);
> > +			return CMD_RET_FAILURE;
> > +		}
> > +	}
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> > +static void show_efi_boot_opt_data(int id, void *data)
> > +{
> > +	struct efi_load_option lo;
> > +	char *label, *p;
> > +	size_t label_len16, label_len;
> > +	u16 *dp_str;
> > +
> > +	efi_deserialize_load_option(&lo, data);
> > +
> > +	label_len16 = u16_strlen(lo.label);
> > +	label_len = utf16_utf8_strnlen(lo.label, label_len16);
> > +	label = malloc(label_len + 1);
> > +	if (!label)
> > +		return;
> > +	p = label;
> > +	utf16_utf8_strncpy(&p, lo.label, label_len16);
> > +
> > +	printf("Boot%04X:\n", id);
> > +	printf("\tattributes: %c%c%c (0x%08x)\n",
> > +	       /* ACTIVE */
> > +	       lo.attributes & 0x1 ? 'A' : '-',
> > +	       /* FORCE RECONNECT */
> > +	       lo.attributes & 0x2 ? 'R' : '-',
> > +	       /* HIDDEN */
> > +	       lo.attributes & 0x8 ? 'H' : '-',
> > +	       lo.attributes);
> > +	printf("\tlabel: %s\n", label);
> > +
> > +	dp_str = efi_dp_str(lo.file_path);
> > +	printf("\tfile_path: %ls\n", dp_str);
> > +	efi_free_pool(dp_str);
> > +
> > +	printf("\tdata: %s\n", lo.optional_data);
> > +
> > +	free(label);
> > +}
> > +
> > +static void show_efi_boot_opt(int id)
> > +{
> > +	char var_name[9];
> > +	u16 var_name16[9], *p;
> > +	efi_guid_t guid;
> > +	void *data = NULL;
> > +	efi_uintn_t size;
> > +	int ret;
> > +
> > +	sprintf(var_name, "Boot%04X", id);
> > +	p = var_name16;
> > +	utf8_utf16_strncpy(&p, var_name, 9);
> > +	guid = efi_global_variable_guid;
> > +
> > +	size = 0;
> > +	ret = efi_get_variable(var_name16, &guid, NULL, &size, NULL);
> > +	if (ret == (int)EFI_BUFFER_TOO_SMALL) {
> > +		data = malloc(size);
> > +		ret = efi_get_variable(var_name16, &guid, NULL, &size, data);
> > +	}
> > +	if (ret == EFI_SUCCESS)
> > +		show_efi_boot_opt_data(id, data);
> > +	else if (ret == EFI_NOT_FOUND)
> > +		printf("Boot%04X: not found\n", id);
> > +
> > +	free(data);
> > +}
> > +
> > +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;
> > +
> > +	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);
> > +
> > +	if (!len)
> > +		return CMD_RET_SUCCESS;
> > +
> > +	if (len < 0)
> > +		return CMD_RET_FAILURE;
> > +
> > +	boot = variables;
> > +	while (*boot) {
> > +		value = strstr(boot, "Boot") + 4;
> > +		id = (int)simple_strtoul(value, NULL, 16);
> > +		show_efi_boot_opt(id);
> > +		boot = strchr(boot, '\n');
> > +		if (!*boot)
> > +			break;
> > +		boot++;
> > +	}
> > +	free(variables);
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> > +static int show_efi_boot_order(void)
> > +{
> > +	efi_guid_t guid;
> > +	u16 *bootorder = NULL;
> > +	efi_uintn_t size;
> > +	int num, i;
> > +	char var_name[9];
> > +	u16 var_name16[9], *p16;
> > +	void *data;
> > +	struct efi_load_option lo;
> > +	char *label, *p;
> > +	size_t label_len16, label_len;
> > +	efi_status_t ret;
> > +
> > +	guid = efi_global_variable_guid;
> > +	size = 0;
> > +	ret = efi_get_variable(L"BootOrder", &guid, NULL, &size, NULL);
> > +	if (ret == EFI_BUFFER_TOO_SMALL) {
> > +		bootorder = malloc(size);
> > +		ret = efi_get_variable(L"BootOrder", &guid, NULL, &size,
> > +				       bootorder);
> > +	}
> > +	if (ret == EFI_NOT_FOUND) {
> > +		printf("BootOrder not defined\n");
> > +		ret = CMD_RET_SUCCESS;
> > +		goto out;
> > +	} else if (ret != EFI_SUCCESS) {
> > +		ret = CMD_RET_FAILURE;
> > +		goto out;
> > +	}
> > +
> > +	num = size / sizeof(u16);
> > +	for (i = 0; i < num; i++) {
> > +		sprintf(var_name, "Boot%04X", bootorder[i]);
> > +		p16 = var_name16;
> > +		utf8_utf16_strncpy(&p16, var_name, 9);
> > +
> > +		size = 0;
> > +		ret = efi_get_variable(var_name16, &guid, NULL, &size, NULL);
> > +		if (ret != EFI_BUFFER_TOO_SMALL) {
> > +			printf("%2d: Boot%04X: (not defined)\n",
> > +			       i + 1, bootorder[i]);
> > +			continue;
> > +		}
> > +
> > +		data = malloc(size);
> > +		if (!data) {
> > +			ret = CMD_RET_FAILURE;
> > +			goto out;
> > +		}
> > +		ret = efi_get_variable(var_name16, &guid, NULL, &size, data);
> > +		if (ret != EFI_SUCCESS) {
> > +			free(data);
> > +			ret = CMD_RET_FAILURE;
> > +			goto out;
> > +		}
> > +
> > +		efi_deserialize_load_option(&lo, data);
> > +
> > +		label_len16 = u16_strlen(lo.label);
> > +		label_len = utf16_utf8_strnlen(lo.label, label_len16);
> > +		label = malloc(label_len + 1);
> > +		if (!label) {
> > +			free(data);
> > +			ret = CMD_RET_FAILURE;
> > +			goto out;
> > +		}
> > +		p = label;
> > +		utf16_utf8_strncpy(&p, lo.label, label_len16);
> > +		printf("%2d: Boot%04X: %s\n", i + 1, bootorder[i], label);
> > +		free(label);
> > +
> > +		free(data);
> > +	}
> > +out:
> > +	free(bootorder);
> > +
> > +	return ret;
> > +}
> > +
> > +static int do_efi_boot_next(cmd_tbl_t *cmdtp, int flag,
> > +			    int argc, char * const argv[])
> > +{
> > +	u16 bootnext;
> > +	efi_uintn_t size;
> > +	char *endp;
> > +	efi_guid_t guid;
> > +	efi_status_t ret;
> > +
> > +	if (argc != 2)
> > +		return CMD_RET_USAGE;
> > +
> > +	bootnext = (u16)simple_strtoul(argv[1], &endp, 16);
> > +	if (*endp != '\0' || bootnext > 0xffff) {
> > +		printf("invalid value: %s\n", argv[1]);
> > +		ret = CMD_RET_FAILURE;
> > +		goto out;
> > +	}
> > +
> > +	guid = efi_global_variable_guid;
> > +	size = sizeof(u16);
> > +	ret = efi_set_variable(L"BootNext", &guid,
> > +			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +			       EFI_VARIABLE_RUNTIME_ACCESS, size, &bootnext);
> > +	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
> > +out:
> > +	return ret;
> > +}
> > +
> > +static int do_efi_boot_order(cmd_tbl_t *cmdtp, int flag,
> > +			     int argc, char * const argv[])
> > +{
> > +	u16 *bootorder = NULL;
> > +	efi_uintn_t size;
> > +	int id, i;
> > +	char *endp;
> > +	efi_guid_t guid;
> > +	efi_status_t ret;
> > +
> > +	if (argc == 1)
> > +		return show_efi_boot_order();
> > +
> > +	argc--;
> > +	argv++;
> > +
> > +	size = argc * sizeof(u16);
> > +	bootorder = malloc(size);
> > +	if (!bootorder)
> > +		return CMD_RET_FAILURE;
> > +
> > +	for (i = 0; i < argc; i++) {
> > +		id = (int)simple_strtoul(argv[i], &endp, 16);
> > +		if (*endp != '\0' || id > 0xffff) {
> > +			printf("invalid value: %s\n", argv[i]);
> > +			ret = CMD_RET_FAILURE;
> > +			goto out;
> > +		}
> > +
> > +		bootorder[i] = (u16)id;
> > +	}
> > +
> > +	guid = efi_global_variable_guid;
> > +	ret = efi_set_variable(L"BootOrder", &guid,
> > +			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +			       EFI_VARIABLE_RUNTIME_ACCESS, size, bootorder);
> > +	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
> > +out:
> > +	free(bootorder);
> > +
> > +	return ret;
> > +}
> > +
> > +static cmd_tbl_t cmd_efidebug_boot_sub[] = {
> > +	U_BOOT_CMD_MKENT(add, CONFIG_SYS_MAXARGS, 1, do_efi_boot_add, "", ""),
> > +	U_BOOT_CMD_MKENT(rm, CONFIG_SYS_MAXARGS, 1, do_efi_boot_rm, "", ""),
> > +	U_BOOT_CMD_MKENT(dump, CONFIG_SYS_MAXARGS, 1, do_efi_boot_dump, "", ""),
> > +	U_BOOT_CMD_MKENT(next, CONFIG_SYS_MAXARGS, 1, do_efi_boot_next, "", ""),
> > +	U_BOOT_CMD_MKENT(order, CONFIG_SYS_MAXARGS, 1, do_efi_boot_order,
> > +			 "", ""),
> > +};
> > +
> > +static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
> > +			   int argc, char * const argv[])
> > +{
> > +	cmd_tbl_t *cp;
> > +
> > +	if (argc < 2)
> > +		return CMD_RET_USAGE;
> > +
> > +	argc--; argv++;
> > +
> > +	cp = find_cmd_tbl(argv[0], cmd_efidebug_boot_sub,
> > +			  ARRAY_SIZE(cmd_efidebug_boot_sub));
> > +	if (!cp)
> > +		return CMD_RET_USAGE;
> > +
> > +	return cp->cmd(cmdtp, flag, argc, argv);
> > +}
> > +
> > +static cmd_tbl_t cmd_efidebug_sub[] = {
> > +	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
> > +};
> > +
> > +/* Interpreter command to configure UEFI environment */
> > +static int do_efidebug(cmd_tbl_t *cmdtp, int flag,
> > +		       int argc, char * const argv[])
> > +{
> > +	cmd_tbl_t *cp;
> > +	efi_status_t r;
> > +
> > +	if (argc < 2)
> > +		return CMD_RET_USAGE;
> > +
> > +	argc--; argv++;
> > +
> > +	/* Initialize UEFI drivers */
> > +	r = efi_init_obj_list();
> > +	if (r != EFI_SUCCESS) {
> > +		printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> > +		       r & ~EFI_ERROR_MASK);
> > +		return CMD_RET_FAILURE;
> > +	}
> > +
> > +	cp = find_cmd_tbl(argv[0], cmd_efidebug_sub,
> > +			  ARRAY_SIZE(cmd_efidebug_sub));
> > +	if (!cp)
> > +		return CMD_RET_USAGE;
> > +
> > +	return cp->cmd(cmdtp, flag, argc, argv);
> > +}
> > +
> > +#ifdef CONFIG_SYS_LONGHELP
> > +static char efidebug_help_text[] =
> > +	"  - UEFI Shell-like interface to configure UEFI environment\n"
> > +	"\n"
> > +	"efidebug boot add <bootid> <label> <interface> <device>[:<part>] <file path> [<load options>]\n"
> > +	"  - set UEFI BootXXXX variable\n"
> > +	"    <load options> will be passed to UEFI application\n"
> > +	"efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
> > +	"  - delete UEFI BootXXXX variables\n"
> > +	"efidebug boot dump\n"
> > +	"  - show all UEFI BootXXXX variables\n"
> > +	"efidebug boot next <bootid>\n"
> > +	"  - set UEFI BootNext variable\n"
> > +	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
> > +	"  - set/show UEFI boot order\n"
> > +	"\n";
> > +#endif
> > +
> > +U_BOOT_CMD(
> > +	efidebug, 10, 0, do_efidebug,
> > +	"Configure UEFI environment",
> > +	efidebug_help_text
> > +);
> > 
> 

[U-Boot] [PATCH v6 2/7] cmd: add efidebug command

[Heinrich Schuchardt]

On 2/20/19 5:58 AM, AKASHI Takahiro wrote:
> On Tue, Feb 19, 2019 at 08:32:52PM +0100, Heinrich Schuchardt wrote:
>> On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
>>> Currently, there is no easy way to add or modify UEFI variables.
>>> In particular, bootmgr supports BootOrder/BootXXXX variables, it is
>>> quite hard to define them as u-boot variables because they are represented
>>> in a complicated and encoded format.
>>>
>>> The new command, efidebug, helps address these issues and give us
>>> more friendly interfaces:
>>>  * efidebug boot add: add BootXXXX variable
>>>  * efidebug boot rm: remove BootXXXX variable
>>>  * efidebug boot dump: display all BootXXXX variables
>>>  * efidebug boot next: set BootNext variable
>>>  * efidebug boot order: set/display a boot order (BootOrder)
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> I could not make this work on qemu_arm_defconfig:
>>
>> Usage:
>> efidebug boot add <bootid> <label> <interface> <device>[:<part>] <file
>> path> [<load options>]
>>
>> => efidebug boot add 00AA 'fancy label' mmc 0:1 wonder.efi '--do-it'
>> => efidebug boot dump
>> =>
>>
>> I would expect either an error or an output of `dump`.
>>
>> As the UEFI spec teaches: "Each Boot####  variable is the name “Boot”
>> appended with a unique four digit hexadecimal number." So 00AA should be
>> a valid id.
> 
> It's not a problem.
> The real issue is that you don't have "mmc 0:1" on you system
> when typing this command.
> 
> This error take places because this command internally uses
> efi_dp_from_name(), hence blk_get_device_part_str(), which
> requires that a block device does exist and it can be accessible
> to see a partition table.
> 
> I think that this is one of major issues in current device path
> implementation. We never know how to convert "mmc 0:1" to a device
> path without having the named  device.
> 
> Anyhow, I will add some error message in such a case.
> 
> -Takahiro Akashi


=> scsi scan
scanning bus for devices...
Target spinup took 0 ms.
SATA link 1 timeout.
SATA link 2 timeout.
SATA link 3 timeout.
SATA link 4 timeout.
SATA link 5 timeout.
AHCI 0001.0000 32 slots 6 ports 1.5 Gbps 0x3f impl SATA mode
flags: 64bit ncq only
  Device 0: (0:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+
            Type: Hard Disk
            Capacity: 128.9 MB = 0.1 GB (264190 x 512)
=> efidebug boot add 00AA 'fancy label' scsi 0:1 wonder.efi '--do-it'
Scanning disk ahci_scsi.id0lun0...
Found 2 disks
e1000: 52:54:00:12:34:56

Warning: e1000#0 using MAC address from ROM
=> efidebug boot dump
Boot00AA:
        attributes: A-- (0x00000001)
        label: fancy label
        file_path:
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/HD(1,MBR,0xcc8b40b9,0x800,0x3fffe)/\wonder.efi
        data: --do-it
=> env print -e Boot00AA
Boot00AA: BS|RT, DataSize = 0x8c
    00000000: 01 00 00 00 66 00 66 00 61 00 6e 00 63 00 79 00
....f.f.a.n.c.y.
    00000010: 20 00 6c 00 61 00 62 00 65 00 6c 00 00 00 01 04
.l.a.b.e.l.....
    00000020: 14 00 b9 73 1d e6 84 a3 cc 4a ae ab 82 e8 28 f3
...s.....J....(.
    00000030: 62 8b 03 02 08 00 00 00 00 00 04 01 2a 00 01 00
b...........*...
    00000040: 00 00 00 08 00 00 00 00 00 00 fe ff 03 00 00 00
................
    00000050: 00 00 b9 40 8b cc 00 00 00 00 00 00 00 00 00 00
... at ............
    00000060: 00 00 01 01 04 04 1c 00 5c 00 77 00 6f 00 6e 00
........\.w.o.n.
    00000070: 64 00 65 00 72 00 2e 00 65 00 66 00 69 00 00 00
d.e.r...e.f.i...
    00000080: 7f ff 04 00 2d 2d 64 6f 2d 69 74 00              ....--do-it.

worked on qemu_arm64_config.

Yes, please, add the error message for an invalid device. That matches
the behavior of the load command that will not work without prior device
scanning.

Best regards

Heinrich

[U-Boot] [PATCH v6 3/7] cmd: efidebug: add devices command

[Heinrich Schuchardt]

On 2/20/19 3:20 AM, AKASHI Takahiro wrote:
> On Tue, Feb 19, 2019 at 08:46:52PM +0100, Heinrich Schuchardt wrote:
>> On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
>>> "devices" command prints all the uefi variables on the system.
>>>
>>> => efi devices
>>> Scanning disk ahci_scsi.id0lun0...
>>> Scanning disk ahci_scsi.id1lun0...
>>> Found 4 disks
>>> Device           Device Path
>>> ================ ====================
>>> 000000007ef07ea0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
>>> 000000007ef00c10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
>>> 000000007ef00dd0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
>>> 000000007ef07be0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
>>> 000000007ef07510 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  cmd/efidebug.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 101 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
>>> index d836576cf6e0..8a7f775b117a 100644
>>> --- a/cmd/efidebug.c
>>> +++ b/cmd/efidebug.c
>>> @@ -15,6 +15,102 @@
>>>  #include <search.h>
>>>  #include <linux/ctype.h>
>>>  
>>> +#define BS systab.boottime
>>> +
>>
>> Please, add Sphinx style descriptions to all functions, their
>> parameters, and return values.
> 
> Sure, but please clearify what you mean by "all."
> Do you expect descriptions for exactly all the functions,
> even for some helper functions of a few lines of code?
> 
>> Why do you need this function? Can't you call LocateHandleBuffer() which
>> takes care of the allocation of the buffer?
>>
>>> +static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
>>> +				    int *num)
> 
> Right. I will use locate_handle_buffer here.
> But why does UEFI spec define functionally-duplicated functions,
> LocateHandle() and LocateHandleBuffer()?
> 
>>> +{
>>> +	efi_handle_t *handles = NULL;
>>> +	efi_uintn_t size = 0;
>>> +	efi_status_t ret;
>>> +
>>> +	if (guid) {
>>> +		ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
>>> +					handles);
>>
>> This will not work on i386. Please, always use EFI_CALL when invoking a
>> function which calls EFI_ENTRY().
> 
> OK, but let me make sure.
> We don't have to use EFI_CALL() for any of (internal) efi_xxx() functions,
> say, efi_add_protocol(). Right?

EFI_ENTRY() and EFI_EXIT() change the content of the register to which
gd is mapped and executes an assert. So only functions starting with
EFI_ENTRY() should be called via EFI_CALL. Please, do not call other
functions via EFI_CALL().

Please, check that efi_save_gd() is called (e.g. via
efi_init_obj_list()) before the first EFI_CALL().

To get rid of the systab.boottime dereference it might be preferable to
change static functions to global once.

Best regards

Heinrich

> 
>>> +		if (ret == EFI_BUFFER_TOO_SMALL) {
>>> +			handles = calloc(1, size);
>>> +			if (!handles)
>>> +				return -1;
>>> +
>>> +			ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
>>> +						handles);
>>> +		}
>>> +		if (ret != EFI_SUCCESS) {
>>> +			free(handles);
>>
>> The handles are not allocated by malloc(). Use FreePool() here.
> 
> Will take care of that along with the change above.
> 
> -Takahiro Akashi
> 
> 
>>> +			return -1;
>>> +		}
>>> +	} else {
>>> +		ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
>>> +		if (ret == EFI_BUFFER_TOO_SMALL) {
>>> +			handles = calloc(1, size);
>>> +			if (!handles)
>>> +				return -1;
>>> +
>>> +			ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size,
>>> +						handles);
>>> +		}
>>> +		if (ret != EFI_SUCCESS) {
>>> +			free(handles);
>>
>> Same here.
>>
>>> +			return -1;
>>> +		}
>>> +	}
>>> +
>>> +	*handlesp = handles;
>>> +	*num = size / sizeof(efi_handle_t);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int efi_get_device_handle_info(efi_handle_t handle, u16 **dev_path_text)
>>> +{
>>> +	struct efi_device_path *dp;
>>> +	efi_status_t ret;
>>> +
>>> +	ret = BS->open_protocol(handle, &efi_guid_device_path,
>>> +				(void **)&dp, NULL /* FIXME */, NULL,
>>> +				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>> +	if (ret == EFI_SUCCESS) {
>>> +		*dev_path_text = efi_dp_str(dp);
>>> +		return 0;
>>> +	} else {
>>> +		return -1;
>>> +	}
>>> +}
>>> +
>>> +#define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
>>> +
>>> +static const char spc[] = "                ";
>>> +static const char sep[] = "================";
>>> +
>>> +static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
>>> +			       int argc, char * const argv[])
>>> +{
>>> +	efi_handle_t *handles;
>>> +	u16 *dev_path_text;
>>> +	int num, i;
>>> +
>>> +	handles = NULL;
>>> +	num = 0;
>>> +	if (efi_get_handles_by_proto(NULL, &handles, &num))
>>> +		return CMD_RET_FAILURE;
>>> +
>>> +	if (!num)
>>> +		return CMD_RET_SUCCESS;
>>> +
>>> +	printf("Device%.*s Device Path\n", EFI_HANDLE_WIDTH - 6, spc);
>>> +	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
>>> +	for (i = 0; i < num; i++) {
>>> +		if (!efi_get_device_handle_info(handles[i], &dev_path_text)) {
>>> +			printf("%p %ls\n", handles[i], dev_path_text);
>>> +			efi_free_pool(dev_path_text);
>>> +		}
>>> +	}
>>> +
>>> +	free(handles);
>>
>> FreePool()!
>>
>> Best regards
>>
>> Heinrich
>>
>>> +
>>> +	return CMD_RET_SUCCESS;
>>> +}
>>> +
>>>  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
>>>  			   int argc, char * const argv[])
>>>  {
>>> @@ -406,6 +502,8 @@ static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
>>>  
>>>  static cmd_tbl_t cmd_efidebug_sub[] = {
>>>  	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
>>> +	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
>>> +			 "", ""),
>>>  };
>>>  
>>>  /* Interpreter command to configure UEFI environment */
>>> @@ -451,7 +549,9 @@ static char efidebug_help_text[] =
>>>  	"  - set UEFI BootNext variable\n"
>>>  	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
>>>  	"  - set/show UEFI boot order\n"
>>> -	"\n";
>>> +	"\n"
>>> +	"efidebug devices\n"
>>> +	"  - show uefi devices\n";
>>>  #endif
>>>  
>>>  U_BOOT_CMD(
>>>
>>
> 

[U-Boot] [PATCH v6 7/7] cmd: efidebug: add memmap command

[Heinrich Schuchardt]

On 2/20/19 1:53 AM, AKASHI Takahiro wrote:
> On Tue, Feb 19, 2019 at 08:11:05PM +0100, Heinrich Schuchardt wrote:
>> On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
>>> "memmap" command prints uefi-specific memory map information.
>>> => efi memmap
>>> Type             Start            End              Attributes
>>> ================ ================ ================ ==========
>>> CONVENTIONAL     0000000040000000-000000007de27000 WB
>>> RUNTIME DATA     000000007de27000-000000007de28000 WB|RT
>>> RESERVED         000000007de28000-000000007de2a000 WB
>>> RUNTIME DATA     000000007de2a000-000000007de2b000 WB|RT
>>> RESERVED         000000007de2b000-000000007de2c000 WB
>>> RUNTIME DATA     000000007de2c000-000000007de2d000 WB|RT
>>> LOADER DATA      000000007de2d000-000000007ff37000 WB
>>> RUNTIME CODE     000000007ff37000-000000007ff38000 WB|RT
>>> LOADER DATA      000000007ff38000-0000000080000000 WB
>>
>> On qemu_arm_defconfig I got this ouptut:
>>
>> => efidebug memmap
>> Type             Start    End      Attributes
>> ================ ======== ======== ==========
>> CONVENTIONAL     0000000040000000-000000007ddf8000 WB
>> BOOT DATA        000000007ddf8000-000000007ddfd000 WB
>> RUNTIME DATA     000000007ddfd000-000000007ddfe000 WB|RT
>> RESERVED         000000007ddfe000-000000007ddff000 WB
>> RUNTIME DATA     000000007ddff000-000000007de00000 WB|RT
>> LOADER DATA      000000007de00000-000000007ff42000 WB
>> RUNTIME CODE     000000007ff42000-000000007ff43000 WB|RT
>> LOADER DATA      000000007ff43000-0000000080000000 WB
>>
>> Something is wrong with you format codes.
>>
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  cmd/efidebug.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 99 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
>>> index 81ab3654f746..39398669e18f 100644
>>> --- a/cmd/efidebug.c
>>> +++ b/cmd/efidebug.c
>>> @@ -309,6 +309,100 @@ static int do_efi_show_images(cmd_tbl_t *cmdtp, int flag,
>>>  	return CMD_RET_SUCCESS;
>>>  }
>>>  
>>> +static const char * const efi_mem_type_string[] = {
>>> +	[EFI_RESERVED_MEMORY_TYPE] = "RESERVED",
>>> +	[EFI_LOADER_CODE] = "LOADER CODE",
>>> +	[EFI_LOADER_DATA] = "LOADER DATA",
>>> +	[EFI_BOOT_SERVICES_CODE] = "BOOT CODE",
>>> +	[EFI_BOOT_SERVICES_DATA] = "BOOT DATA",
>>> +	[EFI_RUNTIME_SERVICES_CODE] = "RUNTIME CODE",
>>> +	[EFI_RUNTIME_SERVICES_DATA] = "RUNTIME DATA",
>>> +	[EFI_CONVENTIONAL_MEMORY] = "CONVENTIONAL",
>>> +	[EFI_UNUSABLE_MEMORY] = "UNUSABLE MEM",
>>> +	[EFI_ACPI_RECLAIM_MEMORY] = "ACPI RECLAIM MEM",
>>> +	[EFI_ACPI_MEMORY_NVS] = "ACPI NVS",
>>> +	[EFI_MMAP_IO] = "IO",
>>> +	[EFI_MMAP_IO_PORT] = "IO PORT",
>>> +	[EFI_PAL_CODE] = "PAL",
>>> +};
>>> +
>>> +static const struct efi_mem_attrs {
>>> +	const u64 bit;
>>> +	const char *text;
>>> +} efi_mem_attrs[] = {
>>> +	{EFI_MEMORY_UC, "UC"},
>>> +	{EFI_MEMORY_UC, "UC"},
>>> +	{EFI_MEMORY_WC, "WC"},
>>> +	{EFI_MEMORY_WT, "WT"},
>>> +	{EFI_MEMORY_WB, "WB"},
>>> +	{EFI_MEMORY_UCE, "UCE"},
>>> +	{EFI_MEMORY_WP, "WP"},
>>> +	{EFI_MEMORY_RP, "RP"},
>>> +	{EFI_MEMORY_XP, "WP"},
>>> +	{EFI_MEMORY_NV, "NV"},
>>> +	{EFI_MEMORY_MORE_RELIABLE, "REL"},
>>> +	{EFI_MEMORY_RO, "RO"},
>>> +	{EFI_MEMORY_RUNTIME, "RT"},
>>> +};
>>> +
>>> +static void print_memory_attributes(u64 attributes)
>>> +{
>>> +	int sep, i;
>>> +
>>> +	for (sep = 0, i = 0; i < ARRAY_SIZE(efi_mem_attrs); i++)
>>> +		if (attributes & efi_mem_attrs[i].bit) {
>>> +			if (sep) {
>>> +				putc('|');
>>> +			} else {
>>> +				putc(' ');
>>> +				sep = 1;
>>> +			}
>>> +			puts(efi_mem_attrs[i].text);
>>> +		}
>>> +}
>>> +
>>> +static int do_efi_show_memmap(cmd_tbl_t *cmdtp, int flag,
>>> +			      int argc, char * const argv[])
>>> +{
>>> +	struct efi_mem_desc *memmap = NULL, *map;
>>> +	efi_uintn_t map_size = 0;
>>> +	const char *type;
>>> +	int i;
>>> +	efi_status_t ret;
>>> +
>>> +	ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
>>> +	if (ret == EFI_BUFFER_TOO_SMALL) {
>>> +		memmap = malloc(map_size);
>>> +		if (!memmap)
>>> +			return CMD_RET_FAILURE;
>>> +		ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
>>> +	}
>>> +	if (ret != EFI_SUCCESS) {
>>> +		free(memmap);
>>> +		return CMD_RET_FAILURE;
>>> +	}
>>> +
>>> +	printf("Type             Start%.*s End%.*s Attributes\n",
>>> +	       EFI_HANDLE_WIDTH - 5, spc, EFI_HANDLE_WIDTH - 3, spc);
>>> +	printf("================ %.*s %.*s ==========\n",
>>> +	       EFI_HANDLE_WIDTH, sep, EFI_HANDLE_WIDTH, sep);
>>> +	for (i = 0, map = memmap; i < map_size / sizeof(*map); map++, i++) {
>>> +		if (map->type < EFI_MAX_MEMORY_TYPE)
>>> +			type = efi_mem_type_string[map->type];
>>> +		else
>>> +			type = "(unknown)";
>>> +		printf("%-16s %016llx-%016llx", type, map->physical_start,
>>
>> Width 16 is ok on 64bit systems but not on 32bit ones.
> 
> It sounds reasonable, but the reality is not so trivial.
> In struct efi_mem_desc, physical_start is defined as efi_physical_addr_t,
> and efi_physical_addr_t is defined as u64 whatever the arch is.
> 
> So how do we know the system has 64-bit address space?
> 
> There is a config, CONFIG_PHYS_64BIT, but it is never defined on x86(_64).
> 
> So workable but ugly solution to meet your requirement would be
> a)
>         if (sizeof(phys_addr_t) >= 8)
> 		printf("%-16s %016llx-%016llx", ...);
>         else
> 		printf("%-16s %08llx-%08llx", ...);
>    or
> b)
>         printf("%-16s %p-%p", type, (void *)map->physical_start, ...);
>         (I don't think "void *" always reflects *physical* bit width
>         on LPAE arch though.)
> 
> Which do you like better?

Please, use the same constant for both the header line and the list output.

The UEFI spec uses
typedef UINT64 EFI_PHYSICAL_ADDRESS;

So it would be appropriate to always print 16 characters.

Best regards

Heinrich


> 
> Thanks,
> -Takahiro Akashi
> 
>> Best regards
>>
>> Heinrich
>>
>>> +		       map->physical_start + map->num_pages * EFI_PAGE_SIZE);
>>> +
>>> +		print_memory_attributes(map->attribute);
>>> +		putc('\n');
>>> +	}
>>> +
>>> +	free(memmap);
>>> +
>>> +	return CMD_RET_SUCCESS;
>>> +}
>>> +
>>>  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
>>>  			   int argc, char * const argv[])
>>>  {
>>> @@ -708,6 +802,8 @@ static cmd_tbl_t cmd_efidebug_sub[] = {
>>>  			 "", ""),
>>>  	U_BOOT_CMD_MKENT(images, CONFIG_SYS_MAXARGS, 1, do_efi_show_images,
>>>  			 "", ""),
>>> +	U_BOOT_CMD_MKENT(memmap, CONFIG_SYS_MAXARGS, 1, do_efi_show_memmap,
>>> +			 "", ""),
>>>  };
>>>  
>>>  /* Interpreter command to configure UEFI environment */
>>> @@ -761,7 +857,9 @@ static char efidebug_help_text[] =
>>>  	"efidebug dh\n"
>>>  	"  - show uefi handles\n"
>>>  	"efidebug images\n"
>>> -	"  - show loaded images\n";
>>> +	"  - show loaded images\n"
>>> +	"efidebug memmap\n"
>>> +	"  - show uefi memory map\n";
>>>  #endif
>>>  
>>>  U_BOOT_CMD(
>>>
>>
> 

[U-Boot] [PATCH v6 3/7] cmd: efidebug: add devices command

[AKASHI Takahiro]

On Wed, Feb 20, 2019 at 07:49:01AM +0100, Heinrich Schuchardt wrote:
> On 2/20/19 3:20 AM, AKASHI Takahiro wrote:
> > On Tue, Feb 19, 2019 at 08:46:52PM +0100, Heinrich Schuchardt wrote:
> >> On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
> >>> "devices" command prints all the uefi variables on the system.
> >>>
> >>> => efi devices
> >>> Scanning disk ahci_scsi.id0lun0...
> >>> Scanning disk ahci_scsi.id1lun0...
> >>> Found 4 disks
> >>> Device           Device Path
> >>> ================ ====================
> >>> 000000007ef07ea0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> >>> 000000007ef00c10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
> >>> 000000007ef00dd0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
> >>> 000000007ef07be0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
> >>> 000000007ef07510 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> ---
> >>>  cmd/efidebug.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>  1 file changed, 101 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> >>> index d836576cf6e0..8a7f775b117a 100644
> >>> --- a/cmd/efidebug.c
> >>> +++ b/cmd/efidebug.c
> >>> @@ -15,6 +15,102 @@
> >>>  #include <search.h>
> >>>  #include <linux/ctype.h>
> >>>  
> >>> +#define BS systab.boottime
> >>> +
> >>
> >> Please, add Sphinx style descriptions to all functions, their
> >> parameters, and return values.
> > 
> > Sure, but please clearify what you mean by "all."
> > Do you expect descriptions for exactly all the functions,
> > even for some helper functions of a few lines of code?
> > 
> >> Why do you need this function? Can't you call LocateHandleBuffer() which
> >> takes care of the allocation of the buffer?
> >>
> >>> +static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
> >>> +				    int *num)
> > 
> > Right. I will use locate_handle_buffer here.
> > But why does UEFI spec define functionally-duplicated functions,
> > LocateHandle() and LocateHandleBuffer()?
> > 
> >>> +{
> >>> +	efi_handle_t *handles = NULL;
> >>> +	efi_uintn_t size = 0;
> >>> +	efi_status_t ret;
> >>> +
> >>> +	if (guid) {
> >>> +		ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
> >>> +					handles);
> >>
> >> This will not work on i386. Please, always use EFI_CALL when invoking a
> >> function which calls EFI_ENTRY().
> > 
> > OK, but let me make sure.
> > We don't have to use EFI_CALL() for any of (internal) efi_xxx() functions,
> > say, efi_add_protocol(). Right?
> 
> EFI_ENTRY() and EFI_EXIT() change the content of the register to which
> gd is mapped and executes an assert. So only functions starting with
> EFI_ENTRY() should be called via EFI_CALL. Please, do not call other
> functions via EFI_CALL().

Thank you for explanation.

> Please, check that efi_save_gd() is called (e.g. via
> efi_init_obj_list()) before the first EFI_CALL().

I didn't notice that.

Can you please leave some document about this kind of usage
so that future UEFI developers be aware of it.

> To get rid of the systab.boottime dereference it might be preferable to
> change static functions to global once.

Do you mean, for example, we should call efi_open_protocol()
instead of BS->open_protocol()?
I hesitate to agree with you as we should try to call exported *API's*
wherever possible, in particular, in EFI-application-like code, like efidebug.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > 
> >>> +		if (ret == EFI_BUFFER_TOO_SMALL) {
> >>> +			handles = calloc(1, size);
> >>> +			if (!handles)
> >>> +				return -1;
> >>> +
> >>> +			ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
> >>> +						handles);
> >>> +		}
> >>> +		if (ret != EFI_SUCCESS) {
> >>> +			free(handles);
> >>
> >> The handles are not allocated by malloc(). Use FreePool() here.
> > 
> > Will take care of that along with the change above.
> > 
> > -Takahiro Akashi
> > 
> > 
> >>> +			return -1;
> >>> +		}
> >>> +	} else {
> >>> +		ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
> >>> +		if (ret == EFI_BUFFER_TOO_SMALL) {
> >>> +			handles = calloc(1, size);
> >>> +			if (!handles)
> >>> +				return -1;
> >>> +
> >>> +			ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size,
> >>> +						handles);
> >>> +		}
> >>> +		if (ret != EFI_SUCCESS) {
> >>> +			free(handles);
> >>
> >> Same here.
> >>
> >>> +			return -1;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	*handlesp = handles;
> >>> +	*num = size / sizeof(efi_handle_t);
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int efi_get_device_handle_info(efi_handle_t handle, u16 **dev_path_text)
> >>> +{
> >>> +	struct efi_device_path *dp;
> >>> +	efi_status_t ret;
> >>> +
> >>> +	ret = BS->open_protocol(handle, &efi_guid_device_path,
> >>> +				(void **)&dp, NULL /* FIXME */, NULL,
> >>> +				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> >>> +	if (ret == EFI_SUCCESS) {
> >>> +		*dev_path_text = efi_dp_str(dp);
> >>> +		return 0;
> >>> +	} else {
> >>> +		return -1;
> >>> +	}
> >>> +}
> >>> +
> >>> +#define EFI_HANDLE_WIDTH ((int)sizeof(efi_handle_t) * 2)
> >>> +
> >>> +static const char spc[] = "                ";
> >>> +static const char sep[] = "================";
> >>> +
> >>> +static int do_efi_show_devices(cmd_tbl_t *cmdtp, int flag,
> >>> +			       int argc, char * const argv[])
> >>> +{
> >>> +	efi_handle_t *handles;
> >>> +	u16 *dev_path_text;
> >>> +	int num, i;
> >>> +
> >>> +	handles = NULL;
> >>> +	num = 0;
> >>> +	if (efi_get_handles_by_proto(NULL, &handles, &num))
> >>> +		return CMD_RET_FAILURE;
> >>> +
> >>> +	if (!num)
> >>> +		return CMD_RET_SUCCESS;
> >>> +
> >>> +	printf("Device%.*s Device Path\n", EFI_HANDLE_WIDTH - 6, spc);
> >>> +	printf("%.*s ====================\n", EFI_HANDLE_WIDTH, sep);
> >>> +	for (i = 0; i < num; i++) {
> >>> +		if (!efi_get_device_handle_info(handles[i], &dev_path_text)) {
> >>> +			printf("%p %ls\n", handles[i], dev_path_text);
> >>> +			efi_free_pool(dev_path_text);
> >>> +		}
> >>> +	}
> >>> +
> >>> +	free(handles);
> >>
> >> FreePool()!
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> +
> >>> +	return CMD_RET_SUCCESS;
> >>> +}
> >>> +
> >>>  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
> >>>  			   int argc, char * const argv[])
> >>>  {
> >>> @@ -406,6 +502,8 @@ static int do_efi_boot_opt(cmd_tbl_t *cmdtp, int flag,
> >>>  
> >>>  static cmd_tbl_t cmd_efidebug_sub[] = {
> >>>  	U_BOOT_CMD_MKENT(boot, CONFIG_SYS_MAXARGS, 1, do_efi_boot_opt, "", ""),
> >>> +	U_BOOT_CMD_MKENT(devices, CONFIG_SYS_MAXARGS, 1, do_efi_show_devices,
> >>> +			 "", ""),
> >>>  };
> >>>  
> >>>  /* Interpreter command to configure UEFI environment */
> >>> @@ -451,7 +549,9 @@ static char efidebug_help_text[] =
> >>>  	"  - set UEFI BootNext variable\n"
> >>>  	"efidebug boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
> >>>  	"  - set/show UEFI boot order\n"
> >>> -	"\n";
> >>> +	"\n"
> >>> +	"efidebug devices\n"
> >>> +	"  - show uefi devices\n";
> >>>  #endif
> >>>  
> >>>  U_BOOT_CMD(
> >>>
> >>
> > 
> 

[U-Boot] [PATCH v6 7/7] cmd: efidebug: add memmap command

[AKASHI Takahiro]

On Wed, Feb 20, 2019 at 07:53:48AM +0100, Heinrich Schuchardt wrote:
> On 2/20/19 1:53 AM, AKASHI Takahiro wrote:
> > On Tue, Feb 19, 2019 at 08:11:05PM +0100, Heinrich Schuchardt wrote:
> >> On 1/24/19 12:04 PM, AKASHI Takahiro wrote:
> >>> "memmap" command prints uefi-specific memory map information.
> >>> => efi memmap
> >>> Type             Start            End              Attributes
> >>> ================ ================ ================ ==========
> >>> CONVENTIONAL     0000000040000000-000000007de27000 WB
> >>> RUNTIME DATA     000000007de27000-000000007de28000 WB|RT
> >>> RESERVED         000000007de28000-000000007de2a000 WB
> >>> RUNTIME DATA     000000007de2a000-000000007de2b000 WB|RT
> >>> RESERVED         000000007de2b000-000000007de2c000 WB
> >>> RUNTIME DATA     000000007de2c000-000000007de2d000 WB|RT
> >>> LOADER DATA      000000007de2d000-000000007ff37000 WB
> >>> RUNTIME CODE     000000007ff37000-000000007ff38000 WB|RT
> >>> LOADER DATA      000000007ff38000-0000000080000000 WB
> >>
> >> On qemu_arm_defconfig I got this ouptut:
> >>
> >> => efidebug memmap
> >> Type             Start    End      Attributes
> >> ================ ======== ======== ==========
> >> CONVENTIONAL     0000000040000000-000000007ddf8000 WB
> >> BOOT DATA        000000007ddf8000-000000007ddfd000 WB
> >> RUNTIME DATA     000000007ddfd000-000000007ddfe000 WB|RT
> >> RESERVED         000000007ddfe000-000000007ddff000 WB
> >> RUNTIME DATA     000000007ddff000-000000007de00000 WB|RT
> >> LOADER DATA      000000007de00000-000000007ff42000 WB
> >> RUNTIME CODE     000000007ff42000-000000007ff43000 WB|RT
> >> LOADER DATA      000000007ff43000-0000000080000000 WB
> >>
> >> Something is wrong with you format codes.
> >>
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> ---
> >>>  cmd/efidebug.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>  1 file changed, 99 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> >>> index 81ab3654f746..39398669e18f 100644
> >>> --- a/cmd/efidebug.c
> >>> +++ b/cmd/efidebug.c
> >>> @@ -309,6 +309,100 @@ static int do_efi_show_images(cmd_tbl_t *cmdtp, int flag,
> >>>  	return CMD_RET_SUCCESS;
> >>>  }
> >>>  
> >>> +static const char * const efi_mem_type_string[] = {
> >>> +	[EFI_RESERVED_MEMORY_TYPE] = "RESERVED",
> >>> +	[EFI_LOADER_CODE] = "LOADER CODE",
> >>> +	[EFI_LOADER_DATA] = "LOADER DATA",
> >>> +	[EFI_BOOT_SERVICES_CODE] = "BOOT CODE",
> >>> +	[EFI_BOOT_SERVICES_DATA] = "BOOT DATA",
> >>> +	[EFI_RUNTIME_SERVICES_CODE] = "RUNTIME CODE",
> >>> +	[EFI_RUNTIME_SERVICES_DATA] = "RUNTIME DATA",
> >>> +	[EFI_CONVENTIONAL_MEMORY] = "CONVENTIONAL",
> >>> +	[EFI_UNUSABLE_MEMORY] = "UNUSABLE MEM",
> >>> +	[EFI_ACPI_RECLAIM_MEMORY] = "ACPI RECLAIM MEM",
> >>> +	[EFI_ACPI_MEMORY_NVS] = "ACPI NVS",
> >>> +	[EFI_MMAP_IO] = "IO",
> >>> +	[EFI_MMAP_IO_PORT] = "IO PORT",
> >>> +	[EFI_PAL_CODE] = "PAL",
> >>> +};
> >>> +
> >>> +static const struct efi_mem_attrs {
> >>> +	const u64 bit;
> >>> +	const char *text;
> >>> +} efi_mem_attrs[] = {
> >>> +	{EFI_MEMORY_UC, "UC"},
> >>> +	{EFI_MEMORY_UC, "UC"},
> >>> +	{EFI_MEMORY_WC, "WC"},
> >>> +	{EFI_MEMORY_WT, "WT"},
> >>> +	{EFI_MEMORY_WB, "WB"},
> >>> +	{EFI_MEMORY_UCE, "UCE"},
> >>> +	{EFI_MEMORY_WP, "WP"},
> >>> +	{EFI_MEMORY_RP, "RP"},
> >>> +	{EFI_MEMORY_XP, "WP"},
> >>> +	{EFI_MEMORY_NV, "NV"},
> >>> +	{EFI_MEMORY_MORE_RELIABLE, "REL"},
> >>> +	{EFI_MEMORY_RO, "RO"},
> >>> +	{EFI_MEMORY_RUNTIME, "RT"},
> >>> +};
> >>> +
> >>> +static void print_memory_attributes(u64 attributes)
> >>> +{
> >>> +	int sep, i;
> >>> +
> >>> +	for (sep = 0, i = 0; i < ARRAY_SIZE(efi_mem_attrs); i++)
> >>> +		if (attributes & efi_mem_attrs[i].bit) {
> >>> +			if (sep) {
> >>> +				putc('|');
> >>> +			} else {
> >>> +				putc(' ');
> >>> +				sep = 1;
> >>> +			}
> >>> +			puts(efi_mem_attrs[i].text);
> >>> +		}
> >>> +}
> >>> +
> >>> +static int do_efi_show_memmap(cmd_tbl_t *cmdtp, int flag,
> >>> +			      int argc, char * const argv[])
> >>> +{
> >>> +	struct efi_mem_desc *memmap = NULL, *map;
> >>> +	efi_uintn_t map_size = 0;
> >>> +	const char *type;
> >>> +	int i;
> >>> +	efi_status_t ret;
> >>> +
> >>> +	ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
> >>> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> >>> +		memmap = malloc(map_size);
> >>> +		if (!memmap)
> >>> +			return CMD_RET_FAILURE;
> >>> +		ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
> >>> +	}
> >>> +	if (ret != EFI_SUCCESS) {
> >>> +		free(memmap);
> >>> +		return CMD_RET_FAILURE;
> >>> +	}
> >>> +
> >>> +	printf("Type             Start%.*s End%.*s Attributes\n",
> >>> +	       EFI_HANDLE_WIDTH - 5, spc, EFI_HANDLE_WIDTH - 3, spc);
> >>> +	printf("================ %.*s %.*s ==========\n",
> >>> +	       EFI_HANDLE_WIDTH, sep, EFI_HANDLE_WIDTH, sep);
> >>> +	for (i = 0, map = memmap; i < map_size / sizeof(*map); map++, i++) {
> >>> +		if (map->type < EFI_MAX_MEMORY_TYPE)
> >>> +			type = efi_mem_type_string[map->type];
> >>> +		else
> >>> +			type = "(unknown)";
> >>> +		printf("%-16s %016llx-%016llx", type, map->physical_start,
> >>
> >> Width 16 is ok on 64bit systems but not on 32bit ones.
> > 
> > It sounds reasonable, but the reality is not so trivial.
> > In struct efi_mem_desc, physical_start is defined as efi_physical_addr_t,
> > and efi_physical_addr_t is defined as u64 whatever the arch is.
> > 
> > So how do we know the system has 64-bit address space?
> > 
> > There is a config, CONFIG_PHYS_64BIT, but it is never defined on x86(_64).
> > 
> > So workable but ugly solution to meet your requirement would be
> > a)
> >         if (sizeof(phys_addr_t) >= 8)
> > 		printf("%-16s %016llx-%016llx", ...);
> >         else
> > 		printf("%-16s %08llx-%08llx", ...);
> >    or
> > b)
> >         printf("%-16s %p-%p", type, (void *)map->physical_start, ...);
> >         (I don't think "void *" always reflects *physical* bit width
> >         on LPAE arch though.)
> > 
> > Which do you like better?
> 
> Please, use the same constant for both the header line and the list output.
> 
> The UEFI spec uses
> typedef UINT64 EFI_PHYSICAL_ADDRESS;
> 
> So it would be appropriate to always print 16 characters.

Okay, so you changed your mind.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> 
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> +		       map->physical_start + map->num_pages * EFI_PAGE_SIZE);
> >>> +
> >>> +		print_memory_attributes(map->attribute);
> >>> +		putc('\n');
> >>> +	}
> >>> +
> >>> +	free(memmap);
> >>> +
> >>> +	return CMD_RET_SUCCESS;
> >>> +}
> >>> +
> >>>  static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
> >>>  			   int argc, char * const argv[])
> >>>  {
> >>> @@ -708,6 +802,8 @@ static cmd_tbl_t cmd_efidebug_sub[] = {
> >>>  			 "", ""),
> >>>  	U_BOOT_CMD_MKENT(images, CONFIG_SYS_MAXARGS, 1, do_efi_show_images,
> >>>  			 "", ""),
> >>> +	U_BOOT_CMD_MKENT(memmap, CONFIG_SYS_MAXARGS, 1, do_efi_show_memmap,
> >>> +			 "", ""),
> >>>  };
> >>>  
> >>>  /* Interpreter command to configure UEFI environment */
> >>> @@ -761,7 +857,9 @@ static char efidebug_help_text[] =
> >>>  	"efidebug dh\n"
> >>>  	"  - show uefi handles\n"
> >>>  	"efidebug images\n"
> >>> -	"  - show loaded images\n";
> >>> +	"  - show loaded images\n"
> >>> +	"efidebug memmap\n"
> >>> +	"  - show uefi memory map\n";
> >>>  #endif
> >>>  
> >>>  U_BOOT_CMD(
> >>>
> >>
> > 
>