[U-Boot] [PATCH v4 0/9] cmd: add efitool for efi environment

[U-Boot] [PATCH v4 0/9] cmd: add efitool 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:
=> efitool boot add 1 SHELL mmc 0:1 /Shell.efi ""
=> efitool boot add 2 HELLO mmc 0:1 /hello.efi ""
=> efitool boot dump
Boot0001:
	attributes: A-- (0x00000001)
	label: SHELL
	file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(1,MBR,0x086246ba,0x800,0x40000)/\\Shell.efi
	data: 
Boot0002:
	attributes: A-- (0x00000001)
	label: HELLO
	file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(1,MBR,0x086246ba,0x800,0x40000)/\\hello.efi
	data: 

=> efitool boot order 1 2
=> efitool 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!
   (or you can do "efitool setvar PlatformLang en")
=> env print -e
Boot0001: {boot,run}(blob)
00000000:  01 00 00 00 68 00 53 00  ....h.S.
00000010:  48 00 45 00 4c 00 4c 00  H.E.L.L.
00000020:  00 00 01 04 14 00 b9 73  .......s
00000030:  1d e6 84 a3 cc 4a ae ab  .....J..
00000040:  82 e8 28 f3 62 8b 03 1a  ..(.b...
00000050:  05 00 00 03 1a 05 00 00  ........
00000060:  04 01 2a 00 01 00 00 00  ..*.....
00000070:  00 08 00 00 00 00 00 00  ........
00000080:  00 00 04 00 00 00 00 00  ........
00000090:  ba 46 62 08 00 00 00 00  .Fb.....
000000a0:  00 00 00 00 00 00 00 00  ........
000000b0:  01 01 04 04 1c 00 5c 00  ......\.
000000c0:  5c 00 53 00 68 00 65 00  \.S.h.e.
000000d0:  6c 00 6c 00 2e 00 65 00  l.l...e.
000000e0:  66 00 69 00 00 00 7f ff  f.i....
000000f0:  04 00 00                 ...
Boot0002: {boot,run}(blob)
00000000:  01 00 00 00 68 00 48 00  ....h.H.
00000010:  45 00 4c 00 4c 00 4f 00  E.L.L.O.
00000020:  00 00 01 04 14 00 b9 73  .......s
00000030:  1d e6 84 a3 cc 4a ae ab  .....J..
00000040:  82 e8 28 f3 62 8b 03 1a  ..(.b...
00000050:  05 00 00 03 1a 05 00 00  ........
00000060:  04 01 2a 00 01 00 00 00  ..*.....
00000070:  00 08 00 00 00 00 00 00  ........
00000080:  00 00 04 00 00 00 00 00  ........
00000090:  ba 46 62 08 00 00 00 00  .Fb.....
000000a0:  00 00 00 00 00 00 00 00  ........
000000b0:  01 01 04 04 1c 00 5c 00  ......\.
000000c0:  5c 00 68 00 65 00 6c 00  \.h.e.l.
000000d0:  6c 00 6f 00 2e 00 65 00  l.o...e.
000000e0:  66 00 69 00 00 00 7f ff  f.i....
000000f0:  04 00 00                 ...
BootOrder: {boot,run}(blob)
00000000:  01 00 02 00              ....
OsIndicationsSupported: {ro,boot}(blob)
00000000:  00 00 00 00 00 00 00 00  ........
PlatformLang: {boot,run}(blob)
00000000:  65 6e                    en

=> run -e Boot0001 or bootefi bootmgr

   (UEFI shell ...)

"setvar" command now supports efi shell-like syntax:

=> env set -e foo =S\"akashi\" =0x012345 =Habcdef  
=> env print -e foo                              
foo: {boot,run}(blob)
00000000:  61 6b 61 73 68 69 45 23  akashiE#
00000010:  01 00 ab cd ef           .....


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

Enjoy!
-Takahiro Akashi

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 (9):
  cmd: add efitool command
  cmd: efitool: add devices command
  efi_driver: add name to driver binding protocol
  cmd: efitool: add drivers command
  cmd: efitool: add dh command
  cmd: efitool: add images command
  cmd: efitool: add memmap command
  cmd: efitool: export uefi variable helper functions
  cmd: env: add "-e" option for handling UEFI variables

 cmd/Kconfig                 |   10 +
 cmd/Makefile                |    1 +
 cmd/efitool.c               | 1103 +++++++++++++++++++++++++++++++++++
 cmd/nvedit.c                |   61 +-
 include/command.h           |    4 +
 include/efi_driver.h        |    1 +
 lib/efi_driver/efi_uclass.c |    1 +
 7 files changed, 1179 insertions(+), 2 deletions(-)
 create mode 100644 cmd/efitool.c

-- 
2.19.1

[U-Boot] [PATCH v4 1/9] cmd: add efitool 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, efitool, helps address these issues and give us
more friendly interfaces:
 * efitool boot add: add BootXXXX variable
 * efitool boot rm: remove BootXXXX variable
 * efitool boot dump: display all BootXXXX variables
 * efitool boot order: set/display a boot order (BootOrder)
 * efitool setvar: set an UEFI variable (with limited functionality)
 * efitool dumpvar: display all UEFI variables

As the name suggests, this command basically provides a subset fo UEFI
shell commands with simplified functionality.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/Kconfig   |  10 +
 cmd/Makefile  |   1 +
 cmd/efitool.c | 707 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 718 insertions(+)
 create mode 100644 cmd/efitool.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index ea1a325eb301..4a721b8bb5b8 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1397,6 +1397,16 @@ config CMD_DISPLAY
 	  displayed on a simple board-specific display. Implement
 	  display_putc() to use it.
 
+config CMD_EFITOOL
+	bool "efitool - display/customize UEFI environment"
+	depends on EFI_LOADER
+	default n
+	help
+	  Enable the 'efitool' 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 15ae4d250f50..f103d2e0fbb6 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_EFITOOL) += efitool.o
 obj-$(CONFIG_CMD_ELF) += elf.o
 obj-$(CONFIG_HUSH_PARSER) += exit.o
 obj-$(CONFIG_CMD_EXT4) += ext4.o
diff --git a/cmd/efitool.c b/cmd/efitool.c
new file mode 100644
index 000000000000..c7dc2c11f2e7
--- /dev/null
+++ b/cmd/efitool.c
@@ -0,0 +1,707 @@
+// 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 <errno.h>
+#include <exports.h>
+#include <hexdump.h>
+#include <malloc.h>
+#include <search.h>
+#include <linux/ctype.h>
+#include <asm/global_data.h>
+
+static void dump_var_data(char *data, unsigned long len)
+{
+	char *start, *end, *p;
+	unsigned long pos, count;
+	char hex[3], line[9];
+	int i;
+
+	end = data + len;
+	for (start = data, pos = 0; start < end; start += count, pos += count) {
+		count = end - start;
+		if (count > 16)
+			count = 16;
+
+		/* count should be multiple of two */
+		printf("%08lx: ", pos);
+
+		/* in hex format */
+		p = start;
+		for (i = 0; i < count / 2; p += 2, i++)
+			printf(" %c%c", *p, *(p + 1));
+		for (; i < 8; i++)
+			printf("   ");
+
+		/* in character format */
+		p = start;
+		hex[2] = '\0';
+		for (i = 0; i < count / 2; i++) {
+			hex[0] = *p++;
+			hex[1] = *p++;
+			line[i] = (char)simple_strtoul(hex, 0, 16);
+			if (line[i] < 0x20 || line[i] > 0x7f)
+				line[i] = '.';
+		}
+		line[i] = '\0';
+		printf("  %s\n", line);
+	}
+}
+
+/*
+ * From efi_variable.c,
+ *
+ * Mapping between UEFI variables and u-boot variables:
+ *
+ *   efi_$guid_$varname = {attributes}(type)value
+ */
+static int do_efi_dump_var(int argc, char * const argv[])
+{
+	char regex[256];
+	char * const regexlist[] = {regex};
+	char *res = NULL, *start, *end;
+	int len;
+
+	if (argc > 2)
+		return CMD_RET_USAGE;
+
+	if (argc == 2)
+		snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_%s", argv[1]);
+	else
+		snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*");
+	debug("%s:%d grep uefi var %s\n", __func__, __LINE__, regex);
+
+	len = hexport_r(&env_htab, '\n', H_MATCH_REGEX | H_MATCH_KEY,
+			&res, 0, 1, regexlist);
+
+	if (len < 0)
+		return CMD_RET_FAILURE;
+
+	if (len > 0) {
+		end = res;
+		while (true) {
+			/* variable name */
+			start = strchr(end, '_');
+			if (!start)
+				break;
+			start = strchr(++start, '_');
+			if (!start)
+				break;
+			end = strchr(++start, '=');
+			if (!end)
+				break;
+			printf("%.*s:", (int)(end - start), start);
+			end++;
+
+			/* value */
+			start = end;
+			end = strstr(start, "(blob)");
+			if (!end) {
+				putc('\n');
+				break;
+			}
+			end += 6;
+			printf(" %.*s\n", (int)(end - start), start);
+
+			start = end;
+			end = strchr(start, '\n');
+			if (!end)
+				break;
+			dump_var_data(start,  end - start);
+		}
+		free(res);
+
+		if (len < 2 && argc == 2)
+			printf("%s: not found\n", argv[1]);
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+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 do_efi_set_var(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;
+}
+
+static int do_efi_boot_add(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(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(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]+");
+
+	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(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(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 int do_efi_boot_opt(int argc, char * const argv[])
+{
+	char *sub_command;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	argc--; argv++;
+	sub_command = argv[0];
+
+	if (!strcmp(sub_command, "add"))
+		return do_efi_boot_add(argc, argv);
+	else if (!strcmp(sub_command, "rm"))
+		return do_efi_boot_rm(argc, argv);
+	else if (!strcmp(sub_command, "dump"))
+		return do_efi_boot_dump(argc, argv);
+	else if (!strcmp(sub_command, "next"))
+		return do_efi_boot_next(argc, argv);
+	else if (!strcmp(sub_command, "order"))
+		return do_efi_boot_order(argc, argv);
+	else
+		return CMD_RET_USAGE;
+}
+
+/* Interpreter command to configure UEFI environment */
+static int do_efitool(cmd_tbl_t *cmdtp, int flag,
+		      int argc, char * const argv[])
+{
+	char *command;
+	efi_status_t r;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	argc--; argv++;
+	command = argv[0];
+
+	/* Initialize UEFI drivers */
+	r = efi_init_obj_list();
+	if (r != EFI_SUCCESS) {
+		printf("Error: Cannot set up UEFI drivers, r = %lu\n",
+		       r & ~EFI_ERROR_MASK);
+		return CMD_RET_FAILURE;
+	}
+
+	if (!strcmp(command, "boot"))
+		return do_efi_boot_opt(argc, argv);
+	else if (!strcmp(command, "dumpvar") || !strcmp(command, "dmpstore"))
+		return do_efi_dump_var(argc, argv);
+	else if (!strcmp(command, "setvar"))
+		return do_efi_set_var(argc, argv);
+	else
+		return CMD_RET_USAGE;
+}
+
+#ifdef CONFIG_SYS_LONGHELP
+static char efitool_help_text[] =
+	"  - UEFI Shell-like interface to configure UEFI environment\n"
+	"\n"
+	"efitool 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"
+	"efitool boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
+	"  - delete UEFI BootXXXX variables\n"
+	"efitool boot dump\n"
+	"  - show all UEFI BootXXXX variables\n"
+	"efitool boot next <bootid>\n"
+	"  - set UEFI BootNext variable\n"
+	"efitool boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
+	"  - set/show UEFI boot order\n"
+	"\n"
+	"efitool dumpvar [<name>]\n"
+	"  - show UEFI variable's value\n"
+	"efitool setvar <name> [<value>]\n"
+	"  - set/delete uefi variable's value\n"
+	"    <value> may be \"=\"...\"\", \"=0x...\", \"=H...\", (set) or \"=\" (delete)\n";
+#endif
+
+U_BOOT_CMD(
+	efitool, 10, 0, do_efitool,
+	"Configure UEFI environment",
+	efitool_help_text
+);
-- 
2.19.1

[U-Boot] [PATCH v4 2/9] cmd: efitool: 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
D
e
v                Device Path
================ ====================
        7ef3bfa0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
        7ef3cca0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
        7ef3d070 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
        7ef3d1b0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
        7ef3d3e0 /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/efitool.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 97 insertions(+), 1 deletion(-)

diff --git a/cmd/efitool.c b/cmd/efitool.c
index c7dc2c11f2e7..6b2df1beedb5 100644
--- a/cmd/efitool.c
+++ b/cmd/efitool.c
@@ -18,6 +18,8 @@
 #include <linux/ctype.h>
 #include <asm/global_data.h>
 
+#define BS systab.boottime
+
 static void dump_var_data(char *data, unsigned long len)
 {
 	char *start, *end, *p;
@@ -266,6 +268,96 @@ out:
 	return ret;
 }
 
+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 **name)
+{
+	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) {
+		*name = efi_dp_str(dp);
+		return 0;
+	} else {
+		return -1;
+	}
+}
+
+static int do_efi_show_devices(int argc, char * const argv[])
+{
+	efi_handle_t *handles;
+	u16 *devname;
+	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("D\n");
+	printf("e\n");
+	printf("v                Device Path\n");
+	printf("================ ====================\n");
+	for (i = 0; i < num; i++) {
+		if (!efi_get_device_handle_info(handles[i], &devname)) {
+			printf("%16p %ls\n", handles[i], devname);
+			efi_free_pool(devname);
+		}
+	}
+
+	free(handles);
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(int argc, char * const argv[])
 {
 	int id;
@@ -673,6 +765,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
 		return do_efi_dump_var(argc, argv);
 	else if (!strcmp(command, "setvar"))
 		return do_efi_set_var(argc, argv);
+	else if (!strcmp(command, "devices"))
+		return do_efi_show_devices(argc, argv);
 	else
 		return CMD_RET_USAGE;
 }
@@ -697,7 +791,9 @@ static char efitool_help_text[] =
 	"  - show UEFI variable's value\n"
 	"efitool setvar <name> [<value>]\n"
 	"  - set/delete uefi variable's value\n"
-	"    <value> may be \"=\"...\"\", \"=0x...\", \"=H...\", (set) or \"=\" (delete)\n";
+	"    <value> may be \"=\"...\"\", \"=0x...\", \"=H...\", (set) or \"=\" (delete)\n"
+	"efitool devices\n"
+	"  - show uefi devices\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

[U-Boot] [PATCH v4 3/9] efi_driver: add name to driver binding protocol

[AKASHI Takahiro]

This new field will be shown as a driver's name in "efitool drivers"
command.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/efi_driver.h        | 1 +
 lib/efi_driver/efi_uclass.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/efi_driver.h b/include/efi_driver.h
index 840483a416a4..ee8867816094 100644
--- a/include/efi_driver.h
+++ b/include/efi_driver.h
@@ -34,6 +34,7 @@ struct efi_driver_ops {
  * This structure adds internal fields to the driver binding protocol.
  */
 struct efi_driver_binding_extended_protocol {
+	const char *name;
 	struct efi_driver_binding_protocol bp;
 	const struct efi_driver_ops *ops;
 };
diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c
index bb86ffd399c3..8bbaa02d490e 100644
--- a/lib/efi_driver/efi_uclass.c
+++ b/lib/efi_driver/efi_uclass.c
@@ -271,6 +271,7 @@ static efi_status_t efi_add_driver(struct driver *drv)
 	bp->bp.stop = efi_uc_stop;
 	bp->bp.version = 0xffffffff;
 	bp->ops = drv->ops;
+	bp->name = drv->name;
 
 	ret = efi_create_handle(&bp->bp.driver_binding_handle);
 	if (ret != EFI_SUCCESS) {
-- 
2.19.1

[U-Boot] [PATCH v4 4/9] cmd: efitool: add drivers command

[AKASHI Takahiro]

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

=> efi drivers
D
r
v                Driver Name          Image Path
================ ==================== ==========
        7ef31d30 EFI block driver     <built-in>

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

diff --git a/cmd/efitool.c b/cmd/efitool.c
index 6b2df1beedb5..4d46721fbf91 100644
--- a/cmd/efitool.c
+++ b/cmd/efitool.c
@@ -8,6 +8,7 @@
 #include <charset.h>
 #include <common.h>
 #include <command.h>
+#include <efi_driver.h>
 #include <efi_loader.h>
 #include <environment.h>
 #include <errno.h>
@@ -16,6 +17,7 @@
 #include <malloc.h>
 #include <search.h>
 #include <linux/ctype.h>
+#include <linux/kernel.h>
 #include <asm/global_data.h>
 
 #define BS systab.boottime
@@ -358,6 +360,82 @@ static int do_efi_show_devices(int argc, char * const argv[])
 	return CMD_RET_SUCCESS;
 }
 
+static int efi_get_driver_handle_info(efi_handle_t handle, u16 **name,
+				      u16 **devname, u16 **filename)
+{
+	struct efi_handler *handler;
+	struct efi_driver_binding_extended_protocol *bp;
+	efi_status_t ret;
+
+	ret = efi_search_protocol(handle, &efi_guid_driver_binding_protocol,
+				  &handler);
+	if (ret != EFI_SUCCESS)
+		return -1;
+	bp = handler->protocol_interface;
+
+	*name = malloc((strlen(bp->name) + 1) * sizeof(u16));
+	if (*name)
+		ascii2unicode(*name, bp->name);
+
+	/*
+	 * TODO:
+	 * handle image->device_handle,
+	 * use append_device_path()
+	 */
+	*devname = NULL;
+	*filename = NULL;
+
+	return 0;
+}
+
+static int do_efi_show_drivers(int argc, char * const argv[])
+{
+	efi_handle_t *handles = NULL, *handle;
+	efi_uintn_t size = 0;
+	u16 *drvname, *devname, *filename;
+	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("D\n");
+	printf("r\n");
+	printf("v                Driver Name          Image Path\n");
+	printf("================ ==================== ==========\n");
+	handle = handles;
+	for (i = 0; i < size / sizeof(*handle); i++) {
+		if (!efi_get_driver_handle_info(*handle, &drvname, &devname,
+						&filename)) {
+			if (filename)
+				printf("%16p %-20ls %ls/%ls\n",
+				       *handle, drvname, devname, filename);
+			else
+				printf("%16p %-20ls <built-in>\n",
+				       *handle, drvname);
+			free(drvname);
+			free(devname);
+			free(filename);
+		}
+		handle++;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(int argc, char * const argv[])
 {
 	int id;
@@ -767,6 +845,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
 		return do_efi_set_var(argc, argv);
 	else if (!strcmp(command, "devices"))
 		return do_efi_show_devices(argc, argv);
+	else if (!strcmp(command, "drivers"))
+		return do_efi_show_drivers(argc, argv);
 	else
 		return CMD_RET_USAGE;
 }
@@ -793,7 +873,9 @@ static char efitool_help_text[] =
 	"  - set/delete uefi variable's value\n"
 	"    <value> may be \"=\"...\"\", \"=0x...\", \"=H...\", (set) or \"=\" (delete)\n"
 	"efitool devices\n"
-	"  - show uefi devices\n";
+	"  - show uefi devices\n"
+	"efitool drivers\n"
+	"  - show uefi drivers\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

[U-Boot] [PATCH v4 5/9] cmd: efitool: 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/efitool.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 1 deletion(-)

diff --git a/cmd/efitool.c b/cmd/efitool.c
index 4d46721fbf91..4bd6367b4bd9 100644
--- a/cmd/efitool.c
+++ b/cmd/efitool.c
@@ -436,6 +436,103 @@ static int do_efi_show_drivers(int argc, char * const argv[])
 	return CMD_RET_SUCCESS;
 }
 
+static struct {
+	u16 *name;
+	efi_guid_t guid;
+} guid_list[] = {
+	{
+		L"Device Path",
+		DEVICE_PATH_GUID,
+	},
+	{
+		L"Device Path To Text",
+		EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID,
+	},
+	{
+		L"Device Path Utilities",
+		EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID,
+	},
+	{
+		L"Unicode Collation 2",
+		EFI_UNICODE_COLLATION_PROTOCOL2_GUID,
+	},
+	{
+		L"Driver Binding",
+		EFI_DRIVER_BINDING_PROTOCOL_GUID,
+	},
+	{
+		L"Simple Text Input",
+		EFI_SIMPLE_TEXT_INPUT_PROTOCOL_GUID,
+	},
+	{
+		L"Simple Text Input Ex",
+		EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL_GUID,
+	},
+	{
+		L"Simple Text Output",
+		EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL_GUID,
+	},
+	{
+		L"Block IO",
+		BLOCK_IO_GUID,
+	},
+	{
+		L"Simple File System",
+		EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID,
+	},
+	{
+		NULL,
+		{{0,},},
+	},
+};
+
+static int do_efi_show_handles(int argc, char * const argv[])
+{
+	efi_handle_t *handles;
+	efi_guid_t **guid;
+	efi_uintn_t count;
+	int num, i, j, k;
+	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;
+
+	for (i = 0; i < num; i++) {
+		printf("%16p:", handles[i]);
+		ret = BS->protocols_per_handle(handles[i], &guid, &count);
+		if (ret || !count) {
+			putc('\n');
+			continue;
+		}
+
+		for (j = 0; j < count; j++) {
+			for (k = 0; guid_list[k].name; k++)
+				if (!guidcmp(&guid_list[k].guid, guid[j]))
+					break;
+
+			if (j)
+				printf(", ");
+			else
+				putc(' ');
+
+			if (guid[j])
+				printf("%ls", guid_list[k].name);
+			else
+				printf("%pUl", guid[j]);
+		}
+		putc('\n');
+	}
+
+	free(handles);
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(int argc, char * const argv[])
 {
 	int id;
@@ -847,6 +944,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
 		return do_efi_show_devices(argc, argv);
 	else if (!strcmp(command, "drivers"))
 		return do_efi_show_drivers(argc, argv);
+	else if (!strcmp(command, "dh"))
+		return do_efi_show_handles(argc, argv);
 	else
 		return CMD_RET_USAGE;
 }
@@ -875,7 +974,9 @@ static char efitool_help_text[] =
 	"efitool devices\n"
 	"  - show uefi devices\n"
 	"efitool drivers\n"
-	"  - show uefi drivers\n";
+	"  - show uefi drivers\n"
+	"efitool dh\n"
+	"  - show uefi handles\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

[U-Boot] [PATCH v4 6/9] cmd: efitool: add images command

[AKASHI Takahiro]

"images" command prints loaded images-related information.

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

diff --git a/cmd/efitool.c b/cmd/efitool.c
index 4bd6367b4bd9..e77273fc6e1e 100644
--- a/cmd/efitool.c
+++ b/cmd/efitool.c
@@ -533,6 +533,13 @@ static int do_efi_show_handles(int argc, char * const argv[])
 	return CMD_RET_SUCCESS;
 }
 
+static int do_efi_show_images(int argc, char * const argv[])
+{
+	efi_print_image_infos(NULL);
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(int argc, char * const argv[])
 {
 	int id;
@@ -946,6 +953,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
 		return do_efi_show_drivers(argc, argv);
 	else if (!strcmp(command, "dh"))
 		return do_efi_show_handles(argc, argv);
+	else if (!strcmp(command, "images"))
+		return do_efi_show_images(argc, argv);
 	else
 		return CMD_RET_USAGE;
 }
@@ -976,7 +985,9 @@ static char efitool_help_text[] =
 	"efitool drivers\n"
 	"  - show uefi drivers\n"
 	"efitool dh\n"
-	"  - show uefi handles\n";
+	"  - show uefi handles\n"
+	"efitool images\n"
+	"  - show loaded images\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

[U-Boot] [PATCH v4 7/9] cmd: efitool: 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/efitool.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

diff --git a/cmd/efitool.c b/cmd/efitool.c
index e77273fc6e1e..f06718ea580d 100644
--- a/cmd/efitool.c
+++ b/cmd/efitool.c
@@ -540,6 +540,78 @@ static int do_efi_show_images(int argc, char * const argv[])
 	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",
+};
+
+#define EFI_MEM_ATTR(attribute, bit, string) \
+	if ((attribute) & (bit)) {	\
+		if (sep)		\
+			putc('|');	\
+		sep = 1;		\
+		printf(string);		\
+	}
+
+static int do_efi_show_memmap(int argc, char * const argv[])
+{
+	struct efi_mem_desc *memmap = NULL, *map;
+	efi_uintn_t map_size = 0;
+	int i, sep;
+	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            End              Attributes\n");
+	printf("================ ================ ================ ==========\n");
+	for (i = 0, map = memmap; i < map_size / sizeof(*map); map++, i++) {
+		sep = 0;
+		printf("%-16s %016llx-%016llx ",
+		       efi_mem_type_string[map->type],
+		       map->physical_start,
+		       map->physical_start + map->num_pages * EFI_PAGE_SIZE);
+		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_UC, "UC");
+		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WC, "WC");
+		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WT, "WT");
+		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WB, "WB");
+		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_UCE, "UCE");
+		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WP, "WP");
+		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_RP, "RP");
+		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_XP, "WP");
+		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_NV, "NV");
+		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_MORE_RELIABLE, "REL");
+		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_RO, "RO");
+		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_RUNTIME, "RT");
+		putc('\n');
+	}
+
+	free(memmap);
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(int argc, char * const argv[])
 {
 	int id;
@@ -955,6 +1027,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
 		return do_efi_show_handles(argc, argv);
 	else if (!strcmp(command, "images"))
 		return do_efi_show_images(argc, argv);
+	else if (!strcmp(command, "memmap"))
+		return do_efi_show_memmap(argc, argv);
 	else
 		return CMD_RET_USAGE;
 }
@@ -987,7 +1061,9 @@ static char efitool_help_text[] =
 	"efitool dh\n"
 	"  - show uefi handles\n"
 	"efitool images\n"
-	"  - show loaded images\n";
+	"  - show loaded images\n"
+	"efitool memmap\n"
+	"  - show uefi memory map\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

[U-Boot] [PATCH v4 8/9] cmd: efitool: export uefi variable helper functions

[AKASHI Takahiro]

Those function will be used for integration with 'env' command
so as to handle uefi variables.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/efitool.c     | 38 ++++++++++++++++++++++++++++++++++----
 include/command.h |  4 ++++
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/cmd/efitool.c b/cmd/efitool.c
index f06718ea580d..b8fe28c53aaf 100644
--- a/cmd/efitool.c
+++ b/cmd/efitool.c
@@ -67,7 +67,7 @@ static void dump_var_data(char *data, unsigned long len)
  *
  *   efi_$guid_$varname = {attributes}(type)value
  */
-static int do_efi_dump_var(int argc, char * const argv[])
+static int _do_efi_dump_var(int argc, char * const argv[])
 {
 	char regex[256];
 	char * const regexlist[] = {regex};
@@ -130,6 +130,21 @@ static int do_efi_dump_var(int argc, char * const argv[])
 	return CMD_RET_SUCCESS;
 }
 
+int do_efi_dump_var(int argc, char * const argv[])
+{
+	efi_status_t r;
+
+	/* Initialize EFI drivers */
+	r = efi_init_obj_list();
+	if (r != EFI_SUCCESS) {
+		printf("Error: Cannot set up EFI drivers, r = %lu\n",
+		       r & ~EFI_ERROR_MASK);
+		return CMD_RET_FAILURE;
+	}
+
+	return _do_efi_dump_var(argc, argv);
+}
+
 static int append_value(char **bufp, unsigned long *sizep, char *data)
 {
 	char *tmp_buf = NULL, *new_buf = NULL, *value;
@@ -227,7 +242,7 @@ out:
 	return 0;
 }
 
-static int do_efi_set_var(int argc, char * const argv[])
+static int _do_efi_set_var(int argc, char * const argv[])
 {
 	char *var_name, *value = NULL;
 	efi_uintn_t size = 0;
@@ -270,6 +285,21 @@ out:
 	return ret;
 }
 
+int do_efi_set_var(int argc, char * const argv[])
+{
+	efi_status_t r;
+
+	/* Initialize EFI drivers */
+	r = efi_init_obj_list();
+	if (r != EFI_SUCCESS) {
+		printf("Error: Cannot set up EFI drivers, r = %lu\n",
+		       r & ~EFI_ERROR_MASK);
+		return CMD_RET_FAILURE;
+	}
+
+	return _do_efi_set_var(argc, argv);
+}
+
 static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
 				    int *num)
 {
@@ -1016,9 +1046,9 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
 	if (!strcmp(command, "boot"))
 		return do_efi_boot_opt(argc, argv);
 	else if (!strcmp(command, "dumpvar") || !strcmp(command, "dmpstore"))
-		return do_efi_dump_var(argc, argv);
+		return _do_efi_dump_var(argc, argv);
 	else if (!strcmp(command, "setvar"))
-		return do_efi_set_var(argc, argv);
+		return _do_efi_set_var(argc, argv);
 	else if (!strcmp(command, "devices"))
 		return do_efi_show_devices(argc, argv);
 	else if (!strcmp(command, "drivers"))
diff --git a/include/command.h b/include/command.h
index feddef300ccc..315e4b9aabfb 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_EFITOOL)
+int do_efi_dump_var(int argc, char * const argv[]);
+int do_efi_set_var(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 v4 9/9] 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>
---
 cmd/nvedit.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 59 insertions(+), 2 deletions(-)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index ce746bbf1b3e..44f6c3759253 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -27,6 +27,8 @@
 #include <cli.h>
 #include <command.h>
 #include <console.h>
+#include <efi.h>
+#include <efi_loader.h>
 #include <environment.h>
 #include <search.h>
 #include <errno.h>
@@ -119,6 +121,25 @@ static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc,
 	int rcode = 0;
 	int env_flag = H_HIDE_DOT;
 
+#if defined(CONFIG_CMD_EFITOOL)
+	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') {
+		efi_status_t r;
+
+		argc--;
+		argv++;
+
+		/* Initialize EFI drivers */
+		r = efi_init_obj_list();
+		if (r != EFI_SUCCESS) {
+			printf("Error: Cannot set up EFI drivers, r = %lu\n",
+			       r & ~EFI_ERROR_MASK);
+			return CMD_RET_FAILURE;
+		}
+
+		return do_efi_dump_var(argc, argv);
+	}
+#endif
+
 	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'a') {
 		argc--;
 		argv++;
@@ -216,6 +237,26 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
 	ENTRY e, *ep;
 
 	debug("Initial value for argc=%d\n", argc);
+
+#if defined(CONFIG_CMD_EFITOOL)
+	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') {
+		efi_status_t r;
+
+		argc--;
+		argv++;
+
+		/* Initialize EFI drivers */
+		r = efi_init_obj_list();
+		if (r != EFI_SUCCESS) {
+			printf("Error: Cannot set up EFI drivers, r = %lu\n",
+			       r & ~EFI_ERROR_MASK);
+			return CMD_RET_FAILURE;
+		}
+
+		return do_efi_set_var(argc, argv);
+	}
+#endif
+
 	while (argc > 1 && **(argv + 1) == '-') {
 		char *arg = *++argv;
 
@@ -1262,15 +1303,23 @@ static char env_help_text[] =
 #if defined(CONFIG_CMD_IMPORTENV)
 	"env import [-d] [-t [-r] | -b | -c] addr [size] [var ...] - import environment\n"
 #endif
+#if defined(CONFIG_CMD_EFITOOL)
+	"env print [-a | -e [name] | name ...] - print environment\n"
+#else
 	"env print [-a | name ...] - print environment\n"
+#endif
 #if defined(CONFIG_CMD_RUN)
 	"env run var [...] - run commands in an environment variable\n"
 #endif
 #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
 	"env save - save environment\n"
 #endif
+#if defined(CONFIG_CMD_EFITOOL)
+	"env set [-e | -f] name [arg ...]\n";
+#else
 	"env set [-f] name [arg ...]\n";
 #endif
+#endif
 
 U_BOOT_CMD(
 	env, CONFIG_SYS_MAXARGS, 1, do_env,
@@ -1295,6 +1344,10 @@ U_BOOT_CMD_COMPLETE(
 	printenv, CONFIG_SYS_MAXARGS, 1,	do_env_print,
 	"print environment variables",
 	"[-a]\n    - print [all] values of all environment variables\n"
+#if defined(CONFIG_CMD_EFITOOL)
+	"printenv -e [<name>]\n"
+	"    - print UEFI variable 'name' or all the variables\n"
+#endif
 	"printenv name ...\n"
 	"    - print value of environment variable 'name'",
 	var_complete
@@ -1322,7 +1375,11 @@ U_BOOT_CMD_COMPLETE(
 U_BOOT_CMD_COMPLETE(
 	setenv, CONFIG_SYS_MAXARGS, 0,	do_env_set,
 	"set environment variables",
-	"[-f] name value ...\n"
+#if defined(CONFIG_CMD_EFITOOL)
+	"-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'",
@@ -1345,7 +1402,7 @@ U_BOOT_CMD_COMPLETE(
 	"run commands in an environment variable",
 	"var [...]\n"
 	"    - run the commands in the environment variable(s) 'var'"
-#if defined(CONFIG_CMD_BOOTEFI)
+#if defined(CONFIG_CMD_EFITOOL)
 	"\n"
 	"run -e [BootXXXX]\n"
 	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
-- 
2.19.1

[U-Boot] [PATCH v4 1/9] cmd: add efitool command

[Heinrich Schuchardt]

On 1/15/19 3:55 AM, 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, efitool, helps address these issues and give us
> more friendly interfaces:
>  * efitool boot add: add BootXXXX variable
>  * efitool boot rm: remove BootXXXX variable
>  * efitool boot dump: display all BootXXXX variables
>  * efitool boot order: set/display a boot order (BootOrder)
>  * efitool setvar: set an UEFI variable (with limited functionality)
>  * efitool dumpvar: display all UEFI variables
> 
> As the name suggests, this command basically provides a subset fo UEFI
> shell commands with simplified functionality.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Hello Takahiro,

thanks a lot for your patch series. The additional command is really useful.

Unfortunately the implementation of sub-commands does not follow the
coding style of other sub-commands. Could you please have a look at the
current doc/README.commands

http://git.denx.de/?p=u-boot.git;a=blob;f=doc/README.commands

I think it should be easy to convert your code to follow the
implementation style suggested there and used all over U-Boot.

Sorry that I did not put this into an earlier comment. I only learnt
about this recently by a review from Simon and then updated the README.

Please, add the new file in MAINTAINERS to the EFI PAYLOAD section.

Best regards

Heinrich

[U-Boot] [PATCH v4 4/9] cmd: efitool: add drivers command

[Heinrich Schuchardt]

On 1/15/19 3:55 AM, AKASHI Takahiro wrote:
> "drivers" command prints all the uefi drivers on the system.
> 
> => efi drivers
> D
> r
> v                Driver Name          Image Path
> ================ ==================== ==========
>         7ef31d30 EFI block driver     <built-in>
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/efitool.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/efitool.c b/cmd/efitool.c
> index 6b2df1beedb5..4d46721fbf91 100644
> --- a/cmd/efitool.c
> +++ b/cmd/efitool.c
> @@ -8,6 +8,7 @@
>  #include <charset.h>
>  #include <common.h>
>  #include <command.h>
> +#include <efi_driver.h>
>  #include <efi_loader.h>
>  #include <environment.h>
>  #include <errno.h>
> @@ -16,6 +17,7 @@
>  #include <malloc.h>
>  #include <search.h>
>  #include <linux/ctype.h>
> +#include <linux/kernel.h>
>  #include <asm/global_data.h>
>  
>  #define BS systab.boottime
> @@ -358,6 +360,82 @@ static int do_efi_show_devices(int argc, char * const argv[])
>  	return CMD_RET_SUCCESS;
>  }
>  
> +static int efi_get_driver_handle_info(efi_handle_t handle, u16 **name,
> +				      u16 **devname, u16 **filename)
> +{
> +	struct efi_handler *handler;
> +	struct efi_driver_binding_extended_protocol *bp;

We can have drivers supplied by U-Boot and drivers supplied by an EFI
binary that we recently installed via the bootefi command.

A driver installed via the bootefi command will not have allocated
memory for the extra fields. So the rest of the code accessing the name
field will produce an illegal memory access.

Best regards

Heinrich

> +	efi_status_t ret;
> +
> +	ret = efi_search_protocol(handle, &efi_guid_driver_binding_protocol,
> +				  &handler);
> +	if (ret != EFI_SUCCESS)
> +		return -1;
> +	bp = handler->protocol_interface;
> +
> +	*name = malloc((strlen(bp->name) + 1) * sizeof(u16));
> +	if (*name)
> +		ascii2unicode(*name, bp->name);
> +
> +	/*
> +	 * TODO:
> +	 * handle image->device_handle,
> +	 * use append_device_path()
> +	 */
> +	*devname = NULL;
> +	*filename = NULL;
> +
> +	return 0;
> +}
> +
> +static int do_efi_show_drivers(int argc, char * const argv[])
> +{
> +	efi_handle_t *handles = NULL, *handle;
> +	efi_uintn_t size = 0;
> +	u16 *drvname, *devname, *filename;
> +	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("D\n");
> +	printf("r\n");
> +	printf("v                Driver Name          Image Path\n");
> +	printf("================ ==================== ==========\n");
> +	handle = handles;
> +	for (i = 0; i < size / sizeof(*handle); i++) {
> +		if (!efi_get_driver_handle_info(*handle, &drvname, &devname,
> +						&filename)) {
> +			if (filename)
> +				printf("%16p %-20ls %ls/%ls\n",
> +				       *handle, drvname, devname, filename);
> +			else
> +				printf("%16p %-20ls <built-in>\n",
> +				       *handle, drvname);
> +			free(drvname);
> +			free(devname);
> +			free(filename);
> +		}
> +		handle++;
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  static int do_efi_boot_add(int argc, char * const argv[])
>  {
>  	int id;
> @@ -767,6 +845,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
>  		return do_efi_set_var(argc, argv);
>  	else if (!strcmp(command, "devices"))
>  		return do_efi_show_devices(argc, argv);
> +	else if (!strcmp(command, "drivers"))
> +		return do_efi_show_drivers(argc, argv);
>  	else
>  		return CMD_RET_USAGE;
>  }
> @@ -793,7 +873,9 @@ static char efitool_help_text[] =
>  	"  - set/delete uefi variable's value\n"
>  	"    <value> may be \"=\"...\"\", \"=0x...\", \"=H...\", (set) or \"=\" (delete)\n"
>  	"efitool devices\n"
> -	"  - show uefi devices\n";
> +	"  - show uefi devices\n"
> +	"efitool drivers\n"
> +	"  - show uefi drivers\n";
>  #endif
>  
>  U_BOOT_CMD(
> 

[U-Boot] [PATCH v4 3/9] efi_driver: add name to driver binding protocol

[Heinrich Schuchardt]

On 1/15/19 3:55 AM, AKASHI Takahiro wrote:
> This new field will be shown as a driver's name in "efitool drivers"
> command.

We can have drivers supplied by U-Boot and drivers supplied by an EFI
binary that we recently installed via the bootefi command.

A driver installed via the bootefi command will not have allocated
memory for the extra fields.

So you cannot use the name field in your "efitool drivers" command.

Best regards

Heinrich

> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  include/efi_driver.h        | 1 +
>  lib/efi_driver/efi_uclass.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/include/efi_driver.h b/include/efi_driver.h
> index 840483a416a4..ee8867816094 100644
> --- a/include/efi_driver.h
> +++ b/include/efi_driver.h
> @@ -34,6 +34,7 @@ struct efi_driver_ops {
>   * This structure adds internal fields to the driver binding protocol.
>   */
>  struct efi_driver_binding_extended_protocol {
> +	const char *name;
>  	struct efi_driver_binding_protocol bp;
>  	const struct efi_driver_ops *ops;
>  };
> diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c
> index bb86ffd399c3..8bbaa02d490e 100644
> --- a/lib/efi_driver/efi_uclass.c
> +++ b/lib/efi_driver/efi_uclass.c
> @@ -271,6 +271,7 @@ static efi_status_t efi_add_driver(struct driver *drv)
>  	bp->bp.stop = efi_uc_stop;
>  	bp->bp.version = 0xffffffff;
>  	bp->ops = drv->ops;
> +	bp->name = drv->name;
>  
>  	ret = efi_create_handle(&bp->bp.driver_binding_handle);
>  	if (ret != EFI_SUCCESS) {
> 

[U-Boot] [PATCH v4 9/9] cmd: env: add "-e" option for handling UEFI variables

[Heinrich Schuchardt]

On 1/15/19 3:55 AM, AKASHI Takahiro wrote:
> "env [print|set] -e" allows for handling uefi variables without
> knowing details about mapping to corresponding u-boot variables.

Why do we need two alternative ways to achieve the same result?
We already have 'efitool setvar' and 'efitool dumpvar'.

Best regards

Heinrich

> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---

[U-Boot] [PATCH v4 7/9] cmd: efitool: add memmap command

[Heinrich Schuchardt]

On 1/15/19 3:55 AM, 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
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/efitool.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 77 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/efitool.c b/cmd/efitool.c
> index e77273fc6e1e..f06718ea580d 100644
> --- a/cmd/efitool.c
> +++ b/cmd/efitool.c
> @@ -540,6 +540,78 @@ static int do_efi_show_images(int argc, char * const argv[])
>  	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",
> +};
> +
> +#define EFI_MEM_ATTR(attribute, bit, string) \
> +	if ((attribute) & (bit)) {	\
> +		if (sep)		\
> +			putc('|');	\
> +		sep = 1;		\
> +		printf(string);		\
> +	}
> +
> +static int do_efi_show_memmap(int argc, char * const argv[])
> +{
> +	struct efi_mem_desc *memmap = NULL, *map;
> +	efi_uintn_t map_size = 0;
> +	int i, sep;
> +	efi_status_t ret;
> +
> +	ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);

ret = EFI_SUCCCESS will only occur if the memory has not been setup yet.

So shouldn't we error out if ret != EFI_BUFFER_TOO_SMALL ?

> +	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            End              Attributes\n");
> +	printf("================ ================ ================ ==========\n");
> +	for (i = 0, map = memmap; i < map_size / sizeof(*map); map++, i++) {
> +		sep = 0;
> +		printf("%-16s %016llx-%016llx ",
> +		       efi_mem_type_string[map->type],
> +		       map->physical_start,
> +		       map->physical_start + map->num_pages * EFI_PAGE_SIZE);
> +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_UC, "UC");
> +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WC, "WC");
> +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WT, "WT");
> +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WB, "WB");
> +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_UCE, "UCE");
> +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WP, "WP");
> +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_RP, "RP");
> +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_XP, "WP");
> +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_NV, "NV");
> +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_MORE_RELIABLE, "REL");
> +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_RO, "RO");
> +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_RUNTIME, "RT");

Please, put that logic into a separate function which returns a const
char *. Then we need only a single printf().

I suggest to use an array for the mapping:

struct memory_attribute_name {
	u64 attribute;
	const char *short_name;
};

struct memory_attribute_name memory_attribute_names[] = {
	{EFI_MEMORY_UC, "UC"},
	...
};

For looping you can use ARRAY_SIZE(memory_attribute_names).

> +		putc('\n');
> +	}
> +
> +	free(memmap);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  static int do_efi_boot_add(int argc, char * const argv[])
>  {

Please, refer to doc/README.commands for the standard way of adding
sub-commands.

Best regards

Heinrich

>  	int id;
> @@ -955,6 +1027,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
>  		return do_efi_show_handles(argc, argv);
>  	else if (!strcmp(command, "images"))
>  		return do_efi_show_images(argc, argv);
> +	else if (!strcmp(command, "memmap"))
> +		return do_efi_show_memmap(argc, argv);
>  	else
>  		return CMD_RET_USAGE;
>  }
> @@ -987,7 +1061,9 @@ static char efitool_help_text[] =
>  	"efitool dh\n"
>  	"  - show uefi handles\n"
>  	"efitool images\n"
> -	"  - show loaded images\n";
> +	"  - show loaded images\n"
> +	"efitool memmap\n"
> +	"  - show uefi memory map\n";
>  #endif
>  
>  U_BOOT_CMD(
> 

[U-Boot] [PATCH v4 5/9] cmd: efitool: add dh command

[Heinrich Schuchardt]

On 1/15/19 3:55 AM, AKASHI Takahiro wrote:
> "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/efitool.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 102 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/efitool.c b/cmd/efitool.c
> index 4d46721fbf91..4bd6367b4bd9 100644
> --- a/cmd/efitool.c
> +++ b/cmd/efitool.c
> @@ -436,6 +436,103 @@ static int do_efi_show_drivers(int argc, char * const argv[])
>  	return CMD_RET_SUCCESS;
>  }
>  
> +static struct {
> +	u16 *name;

Please, reduce the memory size by using char *.

> +	efi_guid_t guid;
> +} guid_list[] = {
> +	{
> +		L"Device Path",
> +		DEVICE_PATH_GUID,
> +	},
> +	{
> +		L"Device Path To Text",
> +		EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID,
> +	},
> +	{
> +		L"Device Path Utilities",
> +		EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID,
> +	},
> +	{
> +		L"Unicode Collation 2",
> +		EFI_UNICODE_COLLATION_PROTOCOL2_GUID,
> +	},
> +	{
> +		L"Driver Binding",
> +		EFI_DRIVER_BINDING_PROTOCOL_GUID,
> +	},
> +	{
> +		L"Simple Text Input",
> +		EFI_SIMPLE_TEXT_INPUT_PROTOCOL_GUID,
> +	},
> +	{
> +		L"Simple Text Input Ex",
> +		EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL_GUID,
> +	},
> +	{
> +		L"Simple Text Output",
> +		EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL_GUID,
> +	},
> +	{
> +		L"Block IO",
> +		BLOCK_IO_GUID,
> +	},
> +	{
> +		L"Simple File System",
> +		EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID,
> +	},

We implement a few more protocols, e.g.

include/efi_api.h:283:#define LOADED_IMAGE_PROTOCOL_GUID \
include/efi_api.h:467:#define BLOCK_IO_GUID \
include/efi_api.h:700:#define EFI_GOP_GUID \
include/efi_api.h:977:#define EFI_DRIVER_BINDING_PROTOCOL_GUID \
include/efi_api.h:999:#define EFI_UNICODE_COLLATION_PROTOCOL2_GUID \

> +	{
> +		NULL,
> +		{{0,},},
> +	}
You could use the ARRAY_SIZE() macro instead of the NULL entry.

> +};
> +
> +static int do_efi_show_handles(int argc, char * const argv[])
> +{
> +	efi_handle_t *handles;
> +	efi_guid_t **guid;
> +	efi_uintn_t count;
> +	int num, i, j, k;
> +	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;
> +
> +	for (i = 0; i < num; i++) {
> +		printf("%16p:", handles[i]);
> +		ret = BS->protocols_per_handle(handles[i], &guid, &count);
> +		if (ret || !count) {
> +			putc('\n');
> +			continue;
> +		}
> +
> +		for (j = 0; j < count; j++) {
> +			for (k = 0; guid_list[k].name; k++)
> +				if (!guidcmp(&guid_list[k].guid, guid[j]))
> +					break;

Please, put this logic into a separate function returning const char *.
Return NULL if not found.

> +
> +			if (j)
> +				printf(", ");
> +			else
> +				putc(' ');
> +
> +			if (guid[j])
> +				printf("%ls", guid_list[k].name);
> +			else
> +				printf("%pUl", guid[j]);
> +		}
> +		putc('\n');
> +	}
> +
> +	free(handles);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  static int do_efi_boot_add(int argc, char * const argv[])
>  {

Please, use the standard way of adding sub-commands. See
doc/README.commands.

Best regards

Heinrich

>  	int id;
> @@ -847,6 +944,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
>  		return do_efi_show_devices(argc, argv);
>  	else if (!strcmp(command, "drivers"))
>  		return do_efi_show_drivers(argc, argv);
> +	else if (!strcmp(command, "dh"))
> +		return do_efi_show_handles(argc, argv);
>  	else
>  		return CMD_RET_USAGE;
>  }
> @@ -875,7 +974,9 @@ static char efitool_help_text[] =
>  	"efitool devices\n"
>  	"  - show uefi devices\n"
>  	"efitool drivers\n"
> -	"  - show uefi drivers\n";
> +	"  - show uefi drivers\n"
> +	"efitool dh\n"
> +	"  - show uefi handles\n";
>  #endif
>  
>  U_BOOT_CMD(
> 

[U-Boot] [PATCH v4 6/9] cmd: efitool: add images command

[Heinrich Schuchardt]

On 1/15/19 3:55 AM, AKASHI Takahiro wrote:
> "images" command prints loaded images-related information.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/efitool.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/efitool.c b/cmd/efitool.c
> index 4bd6367b4bd9..e77273fc6e1e 100644
> --- a/cmd/efitool.c
> +++ b/cmd/efitool.c
> @@ -533,6 +533,13 @@ static int do_efi_show_handles(int argc, char * const argv[])
>  	return CMD_RET_SUCCESS;
>  }
>  
> +static int do_efi_show_images(int argc, char * const argv[])
> +{
> +	efi_print_image_infos(NULL);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  static int do_efi_boot_add(int argc, char * const argv[])
>  {

Please, use the standard way of adding sub-commands. See
doc/README.commands.

Best regards

Heinrich

>  	int id;
> @@ -946,6 +953,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
>  		return do_efi_show_drivers(argc, argv);
>  	else if (!strcmp(command, "dh"))
>  		return do_efi_show_handles(argc, argv);
> +	else if (!strcmp(command, "images"))
> +		return do_efi_show_images(argc, argv);
>  	else
>  		return CMD_RET_USAGE;
>  }
> @@ -976,7 +985,9 @@ static char efitool_help_text[] =
>  	"efitool drivers\n"
>  	"  - show uefi drivers\n"
>  	"efitool dh\n"
> -	"  - show uefi handles\n";
> +	"  - show uefi handles\n"
> +	"efitool images\n"
> +	"  - show loaded images\n";
>  #endif
>  
>  U_BOOT_CMD(
> 

[U-Boot] [PATCH v4 2/9] cmd: efitool: add devices command

[Heinrich Schuchardt]

On 1/15/19 3:55 AM, 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
> D
> e
> v                Device Path
> ================ ====================
>         7ef3bfa0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
>         7ef3cca0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
>         7ef3d070 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
>         7ef3d1b0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
>         7ef3d3e0 /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/efitool.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 97 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/efitool.c b/cmd/efitool.c
> index c7dc2c11f2e7..6b2df1beedb5 100644
> --- a/cmd/efitool.c
> +++ b/cmd/efitool.c
> @@ -18,6 +18,8 @@
>  #include <linux/ctype.h>
>  #include <asm/global_data.h>
>  
> +#define BS systab.boottime
> +
>  static void dump_var_data(char *data, unsigned long len)
>  {
>  	char *start, *end, *p;
> @@ -266,6 +268,96 @@ out:
>  	return ret;
>  }
>  
> +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 **name)
> +{
> +	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) {
> +		*name = efi_dp_str(dp);
> +		return 0;
> +	} else {
> +		return -1;
> +	}
> +}
> +
> +static int do_efi_show_devices(int argc, char * const argv[])
> +{
> +	efi_handle_t *handles;
> +	u16 *devname;

This is the device path. So why not call it dev_path?

> +	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("D\n");
> +	printf("e\n");
> +	printf("v                Device Path\n");
> +	printf("================ ====================\n");
> +	for (i = 0; i < num; i++) {
> +		if (!efi_get_device_handle_info(handles[i], &devname)) {
> +			printf("%16p %ls\n", handles[i], devname);
> +			efi_free_pool(devname);
> +		}
> +	}
> +
> +	free(handles);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  static int do_efi_boot_add(int argc, char * const argv[])
>  {

Please, use the standard way of adding sub-commands, cf.
doc/README.commands.

Thanks a lot for your contribution.

Regards

Heinrich

>  	int id;
> @@ -673,6 +765,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
>  		return do_efi_dump_var(argc, argv);
>  	else if (!strcmp(command, "setvar"))
>  		return do_efi_set_var(argc, argv);
> +	else if (!strcmp(command, "devices"))
> +		return do_efi_show_devices(argc, argv);
>  	else
>  		return CMD_RET_USAGE;
>  }
> @@ -697,7 +791,9 @@ static char efitool_help_text[] =
>  	"  - show UEFI variable's value\n"
>  	"efitool setvar <name> [<value>]\n"
>  	"  - set/delete uefi variable's value\n"
> -	"    <value> may be \"=\"...\"\", \"=0x...\", \"=H...\", (set) or \"=\" (delete)\n";
> +	"    <value> may be \"=\"...\"\", \"=0x...\", \"=H...\", (set) or \"=\" (delete)\n"
> +	"efitool devices\n"
> +	"  - show uefi devices\n";
>  #endif
>  
>  U_BOOT_CMD(
> 

[U-Boot] [PATCH v4 8/9] cmd: efitool: export uefi variable helper functions

[Heinrich Schuchardt]

On 1/15/19 3:55 AM, AKASHI Takahiro wrote:
> Those function will be used for integration with 'env' command
> so as to handle uefi variables.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/efitool.c     | 38 ++++++++++++++++++++++++++++++++++----
>  include/command.h |  4 ++++
>  2 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/cmd/efitool.c b/cmd/efitool.c
> index f06718ea580d..b8fe28c53aaf 100644
> --- a/cmd/efitool.c
> +++ b/cmd/efitool.c
> @@ -67,7 +67,7 @@ static void dump_var_data(char *data, unsigned long len)
>   *
>   *   efi_$guid_$varname = {attributes}(type)value
>   */
> -static int do_efi_dump_var(int argc, char * const argv[])
> +static int _do_efi_dump_var(int argc, char * const argv[])

Please, do not use two function names only distinguished by and
underscore. A a reader I will always wonder which does what.

Please, use names that describe what the difference is.

>  {
>  	char regex[256];
>  	char * const regexlist[] = {regex};
> @@ -130,6 +130,21 @@ static int do_efi_dump_var(int argc, char * const argv[])
>  	return CMD_RET_SUCCESS;
>  }
>  
> +int do_efi_dump_var(int argc, char * const argv[])
> +{
> +	efi_status_t r;
> +
> +	/* Initialize EFI drivers */
> +	r = efi_init_obj_list();
> +	if (r != EFI_SUCCESS) {
> +		printf("Error: Cannot set up EFI drivers, r = %lu\n",
> +		       r & ~EFI_ERROR_MASK);

You duplicate this code below.

So, please, put this into a separate function.

That could also help to avoid having separate do_efi_set_var and
_do_efi_set_var.

> +		return CMD_RET_FAILURE;
> +	}
> +
> +	return _do_efi_dump_var(argc, argv);
> +}
> +
>  static int append_value(char **bufp, unsigned long *sizep, char *data)
>  {
>  	char *tmp_buf = NULL, *new_buf = NULL, *value;
> @@ -227,7 +242,7 @@ out:
>  	return 0;
>  }
>  
> -static int do_efi_set_var(int argc, char * const argv[])
> +static int _do_efi_set_var(int argc, char * const argv[])
>  {
>  	char *var_name, *value = NULL;
>  	efi_uintn_t size = 0;
> @@ -270,6 +285,21 @@ out:
>  	return ret;
>  }
>  
> +int do_efi_set_var(int argc, char * const argv[])
> +{
> +	efi_status_t r;
> +
> +	/* Initialize EFI drivers */
> +	r = efi_init_obj_list();
> +	if (r != EFI_SUCCESS) {
> +		printf("Error: Cannot set up EFI drivers, r = %lu\n",

It is more then drivers that we setup. So perhaps

"Cannot initialize UEFI sub-system."

> +		       r & ~EFI_ERROR_MASK);
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	return _do_efi_set_var(argc, argv);
> +}
> +
>  static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
>  				    int *num)
>  {
> @@ -1016,9 +1046,9 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
>  	if (!strcmp(command, "boot"))
>  		return do_efi_boot_opt(argc, argv);
>  	else if (!strcmp(command, "dumpvar") || !strcmp(command, "dmpstore"))
> -		return do_efi_dump_var(argc, argv);
> +		return _do_efi_dump_var(argc, argv);
>  	else if (!strcmp(command, "setvar"))
> -		return do_efi_set_var(argc, argv);
> +		return _do_efi_set_var(argc, argv);
>  	else if (!strcmp(command, "devices"))
>  		return do_efi_show_devices(argc, argv);
>  	else if (!strcmp(command, "drivers"))
> diff --git a/include/command.h b/include/command.h
> index feddef300ccc..315e4b9aabfb 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_EFITOOL)

The definition has not dependencies. No need for an #if here.

> +int do_efi_dump_var(int argc, char * const argv[]);
> +int do_efi_set_var(int argc, char * const argv[]);
> +#endi

Shouldn't we put this into some include/efi*?

Best regards

Heinrich

>  
>  /* common/command.c */
>  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
> 

[U-Boot] [PATCH v4 0/9] cmd: add efitool for efi environment

[Alexander Graf]

> Am 15.01.2019 um 03:55 schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
> 
> 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.

Thanks a lot again. This is invaluable for efi_loader debugging. I found myself so many times adding printfs to enumarate bits this command would have just told me.

> 
> Let's see how it works:
> => efitool boot add 1 SHELL mmc 0:1 /Shell.efi ""

efidebug please :). I thought we agreed that this is an optional command, people can not expect to have around with no command line syntax guarantees? The name must reflect that. Efitool does not.


Alex

[U-Boot] [PATCH v4 9/9] cmd: env: add "-e" option for handling UEFI variables

[Alexander Graf]

On 01/15/2019 04:47 AM, Heinrich Schuchardt wrote:
> On 1/15/19 3:55 AM, AKASHI Takahiro wrote:
>> "env [print|set] -e" allows for handling uefi variables without
>> knowing details about mapping to corresponding u-boot variables.
> Why do we need two alternative ways to achieve the same result?
> We already have 'efitool setvar' and 'efitool dumpvar'.

I asked for it.

If it was me, I would just drop the "efitool" versions. The more we can 
have in common code / CLI, the better for everyone.

But if Takahiro is passionate about having everything accessible through 
a single efi helper command for his debugging, I wouldn't oppose to it :).


Alex

[U-Boot] [PATCH v4 9/9] cmd: env: add "-e" option for handling UEFI variables

[Alexander Graf]

On 01/15/2019 03:55 AM, AKASHI Takahiro wrote:
> "env [print|set] -e" allows for handling uefi variables without
> knowing details about mapping to corresponding u-boot variables.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   cmd/nvedit.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index ce746bbf1b3e..44f6c3759253 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -27,6 +27,8 @@
>   #include <cli.h>
>   #include <command.h>
>   #include <console.h>
> +#include <efi.h>
> +#include <efi_loader.h>
>   #include <environment.h>
>   #include <search.h>
>   #include <errno.h>
> @@ -119,6 +121,25 @@ static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc,
>   	int rcode = 0;
>   	int env_flag = H_HIDE_DOT;
>   
> +#if defined(CONFIG_CMD_EFITOOL)

This needs to be available without your EFITOOL (which really is a DEBUG 
command). So this needs to be accessible using only CONFIG_EFI_LOADER I 
think. Maybe we can add a CONFIG_CMD_EFI_SUBCMD or so that these bits 
are guarded by if you think that someone really wants to live without.


Alex

[U-Boot] [PATCH v4 1/9] cmd: add efitool command

[AKASHI Takahiro]

On Tue, Jan 15, 2019 at 04:31:35AM +0100, Heinrich Schuchardt wrote:
> On 1/15/19 3:55 AM, 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, efitool, helps address these issues and give us
> > more friendly interfaces:
> >  * efitool boot add: add BootXXXX variable
> >  * efitool boot rm: remove BootXXXX variable
> >  * efitool boot dump: display all BootXXXX variables
> >  * efitool boot order: set/display a boot order (BootOrder)
> >  * efitool setvar: set an UEFI variable (with limited functionality)
> >  * efitool dumpvar: display all UEFI variables
> > 
> > As the name suggests, this command basically provides a subset fo UEFI
> > shell commands with simplified functionality.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> Hello Takahiro,
> 
> thanks a lot for your patch series. The additional command is really useful.
> 
> Unfortunately the implementation of sub-commands does not follow the
> coding style of other sub-commands. Could you please have a look at the
> current doc/README.commands
> 
> http://git.denx.de/?p=u-boot.git;a=blob;f=doc/README.commands

OK.

> I think it should be easy to convert your code to follow the
> implementation style suggested there and used all over U-Boot.
>
> Sorry that I did not put this into an earlier comment. I only learnt
> about this recently by a review from Simon and then updated the README.
> 
> Please, add the new file in MAINTAINERS to the EFI PAYLOAD section.

Sure

Thanks,
-Takahiro Akashi


> Best regards
> 
> Heinrich

[U-Boot] [PATCH v4 2/9] cmd: efitool: add devices command

[AKASHI Takahiro]

On Tue, Jan 15, 2019 at 06:09:31AM +0100, Heinrich Schuchardt wrote:
> On 1/15/19 3:55 AM, 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
> > D
> > e
> > v                Device Path
> > ================ ====================
> >         7ef3bfa0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> >         7ef3cca0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)
> >         7ef3d070 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)
> >         7ef3d1b0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)
> >         7ef3d3e0 /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/efitool.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 97 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/efitool.c b/cmd/efitool.c
> > index c7dc2c11f2e7..6b2df1beedb5 100644
> > --- a/cmd/efitool.c
> > +++ b/cmd/efitool.c
> > @@ -18,6 +18,8 @@
> >  #include <linux/ctype.h>
> >  #include <asm/global_data.h>
> >  
> > +#define BS systab.boottime
> > +
> >  static void dump_var_data(char *data, unsigned long len)
> >  {
> >  	char *start, *end, *p;
> > @@ -266,6 +268,96 @@ out:
> >  	return ret;
> >  }
> >  
> > +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 **name)
> > +{
> > +	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) {
> > +		*name = efi_dp_str(dp);
> > +		return 0;
> > +	} else {
> > +		return -1;
> > +	}
> > +}
> > +
> > +static int do_efi_show_devices(int argc, char * const argv[])
> > +{
> > +	efi_handle_t *handles;
> > +	u16 *devname;
> 
> This is the device path. So why not call it dev_path?

dev_path is normally used for a device path object itself.
I prefer dp_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("D\n");
> > +	printf("e\n");
> > +	printf("v                Device Path\n");
> > +	printf("================ ====================\n");
> > +	for (i = 0; i < num; i++) {
> > +		if (!efi_get_device_handle_info(handles[i], &devname)) {
> > +			printf("%16p %ls\n", handles[i], devname);
> > +			efi_free_pool(devname);
> > +		}
> > +	}
> > +
> > +	free(handles);
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> >  static int do_efi_boot_add(int argc, char * const argv[])
> >  {
> 
> Please, use the standard way of adding sub-commands, cf.
> doc/README.commands.

OK

Thanks,
-Takahiro Akashi

> Thanks a lot for your contribution.
> 
> Regards
> 
> Heinrich
> 
> >  	int id;
> > @@ -673,6 +765,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
> >  		return do_efi_dump_var(argc, argv);
> >  	else if (!strcmp(command, "setvar"))
> >  		return do_efi_set_var(argc, argv);
> > +	else if (!strcmp(command, "devices"))
> > +		return do_efi_show_devices(argc, argv);
> >  	else
> >  		return CMD_RET_USAGE;
> >  }
> > @@ -697,7 +791,9 @@ static char efitool_help_text[] =
> >  	"  - show UEFI variable's value\n"
> >  	"efitool setvar <name> [<value>]\n"
> >  	"  - set/delete uefi variable's value\n"
> > -	"    <value> may be \"=\"...\"\", \"=0x...\", \"=H...\", (set) or \"=\" (delete)\n";
> > +	"    <value> may be \"=\"...\"\", \"=0x...\", \"=H...\", (set) or \"=\" (delete)\n"
> > +	"efitool devices\n"
> > +	"  - show uefi devices\n";
> >  #endif
> >  
> >  U_BOOT_CMD(
> > 
> 

[U-Boot] [PATCH v4 3/9] efi_driver: add name to driver binding protocol

[AKASHI Takahiro]

You raised a couple of questions to me.

On Tue, Jan 15, 2019 at 04:41:08AM +0100, Heinrich Schuchardt wrote:
> On 1/15/19 3:55 AM, AKASHI Takahiro wrote:
> > This new field will be shown as a driver's name in "efitool drivers"
> > command.
> 
> We can have drivers supplied by U-Boot

I assume that what you mention here is a UCLASS_EFI driver.

What's the problem is;
efi_add_driver() adds EFI_DRIVER_BINDING_PROTOCOL with
*efi_driver_binding_extended_protocol* interface, which is NOT compatible
with EFI_DRIVER_BINDING_PROTOCOL.
On the other hand, for example, in your efi_selftest_controller test
a test driver is set up by installing EFI_DRIVER_BINDING_PROTOCOL
with EFI_DRIVER_BINDING_PROTOCOL interface.

So we have no way to distinguish the two cases(handles) and cannot
deal with them properly.

> and drivers supplied by an EFI
> binary that we recently installed via the bootefi command.
> 
> A driver installed via the bootefi command will not have allocated
> memory for the extra fields.

There is no good example of driver of such kind.
I don't know how we can retrieve a meaningful "driver name."

> So you cannot use the name field in your "efitool drivers" command.

Any suggestion?

Thanks,
-Takahiro Akashi

> 
> Best regards
> 
> Heinrich
> 
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  include/efi_driver.h        | 1 +
> >  lib/efi_driver/efi_uclass.c | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/include/efi_driver.h b/include/efi_driver.h
> > index 840483a416a4..ee8867816094 100644
> > --- a/include/efi_driver.h
> > +++ b/include/efi_driver.h
> > @@ -34,6 +34,7 @@ struct efi_driver_ops {
> >   * This structure adds internal fields to the driver binding protocol.
> >   */
> >  struct efi_driver_binding_extended_protocol {
> > +	const char *name;
> >  	struct efi_driver_binding_protocol bp;
> >  	const struct efi_driver_ops *ops;
> >  };
> > diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c
> > index bb86ffd399c3..8bbaa02d490e 100644
> > --- a/lib/efi_driver/efi_uclass.c
> > +++ b/lib/efi_driver/efi_uclass.c
> > @@ -271,6 +271,7 @@ static efi_status_t efi_add_driver(struct driver *drv)
> >  	bp->bp.stop = efi_uc_stop;
> >  	bp->bp.version = 0xffffffff;
> >  	bp->ops = drv->ops;
> > +	bp->name = drv->name;
> >  
> >  	ret = efi_create_handle(&bp->bp.driver_binding_handle);
> >  	if (ret != EFI_SUCCESS) {
> > 
> 

[U-Boot] [PATCH v4 5/9] cmd: efitool: add dh command

[AKASHI Takahiro]

On Tue, Jan 15, 2019 at 05:55:26AM +0100, Heinrich Schuchardt wrote:
> On 1/15/19 3:55 AM, AKASHI Takahiro wrote:
> > "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/efitool.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 102 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/efitool.c b/cmd/efitool.c
> > index 4d46721fbf91..4bd6367b4bd9 100644
> > --- a/cmd/efitool.c
> > +++ b/cmd/efitool.c
> > @@ -436,6 +436,103 @@ static int do_efi_show_drivers(int argc, char * const argv[])
> >  	return CMD_RET_SUCCESS;
> >  }
> >  
> > +static struct {
> > +	u16 *name;
> 
> Please, reduce the memory size by using char *.

OK

> > +	efi_guid_t guid;
> > +} guid_list[] = {
> > +	{
> > +		L"Device Path",
> > +		DEVICE_PATH_GUID,
> > +	},
> > +	{
> > +		L"Device Path To Text",
> > +		EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID,
> > +	},
> > +	{
> > +		L"Device Path Utilities",
> > +		EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID,
> > +	},
> > +	{
> > +		L"Unicode Collation 2",
> > +		EFI_UNICODE_COLLATION_PROTOCOL2_GUID,
> > +	},
> > +	{
> > +		L"Driver Binding",
> > +		EFI_DRIVER_BINDING_PROTOCOL_GUID,
> > +	},
> > +	{
> > +		L"Simple Text Input",
> > +		EFI_SIMPLE_TEXT_INPUT_PROTOCOL_GUID,
> > +	},
> > +	{
> > +		L"Simple Text Input Ex",
> > +		EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL_GUID,
> > +	},
> > +	{
> > +		L"Simple Text Output",
> > +		EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL_GUID,
> > +	},
> > +	{
> > +		L"Block IO",
> > +		BLOCK_IO_GUID,
> > +	},
> > +	{
> > +		L"Simple File System",
> > +		EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID,
> > +	},
> 
> We implement a few more protocols, e.g.
> 
> include/efi_api.h:467:#define BLOCK_IO_GUID \
> include/efi_api.h:977:#define EFI_DRIVER_BINDING_PROTOCOL_GUID \
> include/efi_api.h:999:#define EFI_UNICODE_COLLATION_PROTOCOL2_GUID \

They are already in there.

> include/efi_api.h:283:#define LOADED_IMAGE_PROTOCOL_GUID \
> include/efi_api.h:700:#define EFI_GOP_GUID \

Added.

> > +	{
> > +		NULL,
> > +		{{0,},},
> > +	}
> You could use the ARRAY_SIZE() macro instead of the NULL entry.

OK

> > +};
> > +
> > +static int do_efi_show_handles(int argc, char * const argv[])
> > +{
> > +	efi_handle_t *handles;
> > +	efi_guid_t **guid;
> > +	efi_uintn_t count;
> > +	int num, i, j, k;
> > +	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;
> > +
> > +	for (i = 0; i < num; i++) {
> > +		printf("%16p:", handles[i]);
> > +		ret = BS->protocols_per_handle(handles[i], &guid, &count);
> > +		if (ret || !count) {
> > +			putc('\n');
> > +			continue;
> > +		}
> > +
> > +		for (j = 0; j < count; j++) {
> > +			for (k = 0; guid_list[k].name; k++)
> > +				if (!guidcmp(&guid_list[k].guid, guid[j]))
> > +					break;
> 
> Please, put this logic into a separate function returning const char *.
> Return NULL if not found.

OK

> > +
> > +			if (j)
> > +				printf(", ");
> > +			else
> > +				putc(' ');
> > +
> > +			if (guid[j])
> > +				printf("%ls", guid_list[k].name);
> > +			else
> > +				printf("%pUl", guid[j]);
> > +		}
> > +		putc('\n');
> > +	}
> > +
> > +	free(handles);
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> >  static int do_efi_boot_add(int argc, char * const argv[])
> >  {
> 
> Please, use the standard way of adding sub-commands. See
> doc/README.commands.

OK

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >  	int id;
> > @@ -847,6 +944,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
> >  		return do_efi_show_devices(argc, argv);
> >  	else if (!strcmp(command, "drivers"))
> >  		return do_efi_show_drivers(argc, argv);
> > +	else if (!strcmp(command, "dh"))
> > +		return do_efi_show_handles(argc, argv);
> >  	else
> >  		return CMD_RET_USAGE;
> >  }
> > @@ -875,7 +974,9 @@ static char efitool_help_text[] =
> >  	"efitool devices\n"
> >  	"  - show uefi devices\n"
> >  	"efitool drivers\n"
> > -	"  - show uefi drivers\n";
> > +	"  - show uefi drivers\n"
> > +	"efitool dh\n"
> > +	"  - show uefi handles\n";
> >  #endif
> >  
> >  U_BOOT_CMD(
> > 
> 

[U-Boot] [PATCH v4 6/9] cmd: efitool: add images command

[AKASHI Takahiro]

On Tue, Jan 15, 2019 at 05:58:57AM +0100, Heinrich Schuchardt wrote:
> On 1/15/19 3:55 AM, AKASHI Takahiro wrote:
> > "images" command prints loaded images-related information.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/efitool.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/efitool.c b/cmd/efitool.c
> > index 4bd6367b4bd9..e77273fc6e1e 100644
> > --- a/cmd/efitool.c
> > +++ b/cmd/efitool.c
> > @@ -533,6 +533,13 @@ static int do_efi_show_handles(int argc, char * const argv[])
> >  	return CMD_RET_SUCCESS;
> >  }
> >  
> > +static int do_efi_show_images(int argc, char * const argv[])
> > +{
> > +	efi_print_image_infos(NULL);
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> >  static int do_efi_boot_add(int argc, char * const argv[])
> >  {
> 
> Please, use the standard way of adding sub-commands. See
> doc/README.commands.

OK

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >  	int id;
> > @@ -946,6 +953,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
> >  		return do_efi_show_drivers(argc, argv);
> >  	else if (!strcmp(command, "dh"))
> >  		return do_efi_show_handles(argc, argv);
> > +	else if (!strcmp(command, "images"))
> > +		return do_efi_show_images(argc, argv);
> >  	else
> >  		return CMD_RET_USAGE;
> >  }
> > @@ -976,7 +985,9 @@ static char efitool_help_text[] =
> >  	"efitool drivers\n"
> >  	"  - show uefi drivers\n"
> >  	"efitool dh\n"
> > -	"  - show uefi handles\n";
> > +	"  - show uefi handles\n"
> > +	"efitool images\n"
> > +	"  - show loaded images\n";
> >  #endif
> >  
> >  U_BOOT_CMD(
> > 
> 

[U-Boot] [PATCH v4 3/9] efi_driver: add name to driver binding protocol

[Heinrich Schuchardt]

On 1/17/19 6:33 AM, AKASHI Takahiro wrote:
> You raised a couple of questions to me.
> 
> On Tue, Jan 15, 2019 at 04:41:08AM +0100, Heinrich Schuchardt wrote:
>> On 1/15/19 3:55 AM, AKASHI Takahiro wrote:
>>> This new field will be shown as a driver's name in "efitool drivers"
>>> command.
>>
>> We can have drivers supplied by U-Boot
> 
> I assume that what you mention here is a UCLASS_EFI driver.
> 
> What's the problem is;
> efi_add_driver() adds EFI_DRIVER_BINDING_PROTOCOL with
> *efi_driver_binding_extended_protocol* interface, which is NOT compatible
> with EFI_DRIVER_BINDING_PROTOCOL.
> On the other hand, for example, in your efi_selftest_controller test
> a test driver is set up by installing EFI_DRIVER_BINDING_PROTOCOL
> with EFI_DRIVER_BINDING_PROTOCOL interface.
> 
> So we have no way to distinguish the two cases(handles) and cannot
> deal with them properly.

Correct. It is allowable to add private fields to a protocol. EDK2 does
the same. But we cannot make any assumptions about the private fields here.

> 
>> and drivers supplied by an EFI
>> binary that we recently installed via the bootefi command.
>>
>> A driver installed via the bootefi command will not have allocated
>> memory for the extra fields.
> 
> There is no good example of driver of such kind.
> I don't know how we can retrieve a meaningful "driver name."

The UEFI spec does not foresee any name field.

The interesting information is: on which handle did the driver install
which protocol.

This information could be gathered by looping over all protocols on all
handles and looking for an OpenProtocolInformation where the driver is
the controller.

For me it would be fine if we leave that to a later patch.

Best regards

Heinrich

> 
>> So you cannot use the name field in your "efitool drivers" command.
> 
> Any suggestion?
> 
> Thanks,
> -Takahiro Akashi
> 
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  include/efi_driver.h        | 1 +
>>>  lib/efi_driver/efi_uclass.c | 1 +
>>>  2 files changed, 2 insertions(+)
>>>
>>> diff --git a/include/efi_driver.h b/include/efi_driver.h
>>> index 840483a416a4..ee8867816094 100644
>>> --- a/include/efi_driver.h
>>> +++ b/include/efi_driver.h
>>> @@ -34,6 +34,7 @@ struct efi_driver_ops {
>>>   * This structure adds internal fields to the driver binding protocol.
>>>   */
>>>  struct efi_driver_binding_extended_protocol {
>>> +	const char *name;
>>>  	struct efi_driver_binding_protocol bp;
>>>  	const struct efi_driver_ops *ops;
>>>  };
>>> diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c
>>> index bb86ffd399c3..8bbaa02d490e 100644
>>> --- a/lib/efi_driver/efi_uclass.c
>>> +++ b/lib/efi_driver/efi_uclass.c
>>> @@ -271,6 +271,7 @@ static efi_status_t efi_add_driver(struct driver *drv)
>>>  	bp->bp.stop = efi_uc_stop;
>>>  	bp->bp.version = 0xffffffff;
>>>  	bp->ops = drv->ops;
>>> +	bp->name = drv->name;
>>>  
>>>  	ret = efi_create_handle(&bp->bp.driver_binding_handle);
>>>  	if (ret != EFI_SUCCESS) {
>>>
>>
> 

[U-Boot] [PATCH v4 7/9] cmd: efitool: add memmap command

[AKASHI Takahiro]

On Tue, Jan 15, 2019 at 05:26:42AM +0100, Heinrich Schuchardt wrote:
> On 1/15/19 3:55 AM, 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
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/efitool.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 77 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/efitool.c b/cmd/efitool.c
> > index e77273fc6e1e..f06718ea580d 100644
> > --- a/cmd/efitool.c
> > +++ b/cmd/efitool.c
> > @@ -540,6 +540,78 @@ static int do_efi_show_images(int argc, char * const argv[])
> >  	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",
> > +};
> > +
> > +#define EFI_MEM_ATTR(attribute, bit, string) \
> > +	if ((attribute) & (bit)) {	\
> > +		if (sep)		\
> > +			putc('|');	\
> > +		sep = 1;		\
> > +		printf(string);		\
> > +	}
> > +
> > +static int do_efi_show_memmap(int argc, char * const argv[])
> > +{
> > +	struct efi_mem_desc *memmap = NULL, *map;
> > +	efi_uintn_t map_size = 0;
> > +	int i, sep;
> > +	efi_status_t ret;
> > +
> > +	ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
> 
> ret = EFI_SUCCCESS will only occur if the memory has not been setup yet.

I don't think so.

> So shouldn't we error out if ret != EFI_BUFFER_TOO_SMALL ?
> 
> > +	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            End              Attributes\n");
> > +	printf("================ ================ ================ ==========\n");
> > +	for (i = 0, map = memmap; i < map_size / sizeof(*map); map++, i++) {
> > +		sep = 0;
> > +		printf("%-16s %016llx-%016llx ",
> > +		       efi_mem_type_string[map->type],
> > +		       map->physical_start,
> > +		       map->physical_start + map->num_pages * EFI_PAGE_SIZE);
> > +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_UC, "UC");
> > +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WC, "WC");
> > +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WT, "WT");
> > +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WB, "WB");
> > +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_UCE, "UCE");
> > +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WP, "WP");
> > +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_RP, "RP");
> > +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_XP, "WP");
> > +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_NV, "NV");
> > +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_MORE_RELIABLE, "REL");
> > +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_RO, "RO");
> > +		EFI_MEM_ATTR(map->attribute, EFI_MEMORY_RUNTIME, "RT");
> 
> Please, put that logic into a separate function which returns a const
> char *. Then we need only a single printf().

I don't think it's worth generating a one-time string only for this
specific use.
If 'printf' is a matter, I will simply replace it to puts() here.

> I suggest to use an array for the mapping:
> 
> struct memory_attribute_name {
> 	u64 attribute;
> 	const char *short_name;
> };
> 
> struct memory_attribute_name memory_attribute_names[] = {
> 	{EFI_MEMORY_UC, "UC"},
> 	...
> };
>
> For looping you can use ARRAY_SIZE(memory_attribute_names).
> 
> > +		putc('\n');
> > +	}
> > +
> > +	free(memmap);
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> >  static int do_efi_boot_add(int argc, char * const argv[])
> >  {
> 
> Please, refer to doc/README.commands for the standard way of adding
> sub-commands.

OK

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >  	int id;
> > @@ -955,6 +1027,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
> >  		return do_efi_show_handles(argc, argv);
> >  	else if (!strcmp(command, "images"))
> >  		return do_efi_show_images(argc, argv);
> > +	else if (!strcmp(command, "memmap"))
> > +		return do_efi_show_memmap(argc, argv);
> >  	else
> >  		return CMD_RET_USAGE;
> >  }
> > @@ -987,7 +1061,9 @@ static char efitool_help_text[] =
> >  	"efitool dh\n"
> >  	"  - show uefi handles\n"
> >  	"efitool images\n"
> > -	"  - show loaded images\n";
> > +	"  - show loaded images\n"
> > +	"efitool memmap\n"
> > +	"  - show uefi memory map\n";
> >  #endif
> >  
> >  U_BOOT_CMD(
> > 
> 

[U-Boot] [PATCH v4 8/9] cmd: efitool: export uefi variable helper functions

[AKASHI Takahiro]

On Tue, Jan 15, 2019 at 06:28:38AM +0100, Heinrich Schuchardt wrote:
> On 1/15/19 3:55 AM, AKASHI Takahiro wrote:
> > Those function will be used for integration with 'env' command
> > so as to handle uefi variables.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/efitool.c     | 38 ++++++++++++++++++++++++++++++++++----
> >  include/command.h |  4 ++++
> >  2 files changed, 38 insertions(+), 4 deletions(-)
> > 
> > diff --git a/cmd/efitool.c b/cmd/efitool.c
> > index f06718ea580d..b8fe28c53aaf 100644
> > --- a/cmd/efitool.c
> > +++ b/cmd/efitool.c
> > @@ -67,7 +67,7 @@ static void dump_var_data(char *data, unsigned long len)
> >   *
> >   *   efi_$guid_$varname = {attributes}(type)value
> >   */
> > -static int do_efi_dump_var(int argc, char * const argv[])
> > +static int _do_efi_dump_var(int argc, char * const argv[])
> 
> Please, do not use two function names only distinguished by and
> underscore. A a reader I will always wonder which does what.

While I believe that "_" and "__" are a very common practice to
show an internal version of function,

> Please, use names that describe what the difference is.

I will revert _do_efi_dump_var() to do_efi_dump_var() and
change do_efi_dump_var() to do_efi_dump_var_ext().

> >  {
> >  	char regex[256];
> >  	char * const regexlist[] = {regex};
> > @@ -130,6 +130,21 @@ static int do_efi_dump_var(int argc, char * const argv[])
> >  	return CMD_RET_SUCCESS;
> >  }
> >  
> > +int do_efi_dump_var(int argc, char * const argv[])
> > +{
> > +	efi_status_t r;
> > +
> > +	/* Initialize EFI drivers */
> > +	r = efi_init_obj_list();
> > +	if (r != EFI_SUCCESS) {
> > +		printf("Error: Cannot set up EFI drivers, r = %lu\n",
> > +		       r & ~EFI_ERROR_MASK);
> 
> You duplicate this code below.

Do you want another function for just calling one function?
I don't think it's worth doing so.

> So, please, put this into a separate function.
> 
> That could also help to avoid having separate do_efi_set_var and
> _do_efi_set_var.

I don't get your point.
Do you want to call efi_init_obj_list() at every do_efi_xxx()?
It just doesn't make sense.

> > +		return CMD_RET_FAILURE;
> > +	}
> > +
> > +	return _do_efi_dump_var(argc, argv);
> > +}
> > +
> >  static int append_value(char **bufp, unsigned long *sizep, char *data)
> >  {
> >  	char *tmp_buf = NULL, *new_buf = NULL, *value;
> > @@ -227,7 +242,7 @@ out:
> >  	return 0;
> >  }
> >  
> > -static int do_efi_set_var(int argc, char * const argv[])
> > +static int _do_efi_set_var(int argc, char * const argv[])
> >  {
> >  	char *var_name, *value = NULL;
> >  	efi_uintn_t size = 0;
> > @@ -270,6 +285,21 @@ out:
> >  	return ret;
> >  }
> >  
> > +int do_efi_set_var(int argc, char * const argv[])
> > +{
> > +	efi_status_t r;
> > +
> > +	/* Initialize EFI drivers */
> > +	r = efi_init_obj_list();
> > +	if (r != EFI_SUCCESS) {
> > +		printf("Error: Cannot set up EFI drivers, r = %lu\n",
> 
> It is more then drivers that we setup. So perhaps
> 
> "Cannot initialize UEFI sub-system."

I don't mind, but that string directly comes from the original code
in bootefi.c.

> > +		       r & ~EFI_ERROR_MASK);
> > +		return CMD_RET_FAILURE;
> > +	}
> > +
> > +	return _do_efi_set_var(argc, argv);
> > +}
> > +
> >  static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
> >  				    int *num)
> >  {
> > @@ -1016,9 +1046,9 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
> >  	if (!strcmp(command, "boot"))
> >  		return do_efi_boot_opt(argc, argv);
> >  	else if (!strcmp(command, "dumpvar") || !strcmp(command, "dmpstore"))
> > -		return do_efi_dump_var(argc, argv);
> > +		return _do_efi_dump_var(argc, argv);
> >  	else if (!strcmp(command, "setvar"))
> > -		return do_efi_set_var(argc, argv);
> > +		return _do_efi_set_var(argc, argv);
> >  	else if (!strcmp(command, "devices"))
> >  		return do_efi_show_devices(argc, argv);
> >  	else if (!strcmp(command, "drivers"))
> > diff --git a/include/command.h b/include/command.h
> > index feddef300ccc..315e4b9aabfb 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_EFITOOL)
> 
> The definition has not dependencies. No need for an #if here.

Currently has, but as you and Alex suggested in a separate thread,
we may want to enable it (and do_efi_dump/set_var() as well)
whether or not CMD_BOOTEFI and CMD_EFIDEBUG, respectively, are defined.

Thanks,
-Takahiro Akashi

> > +int do_efi_dump_var(int argc, char * const argv[]);
> > +int do_efi_set_var(int argc, char * const argv[]);
> > +#endi
> 
> Shouldn't we put this into some include/efi*?
> 
> Best regards
> 
> Heinrich
> 
> >  
> >  /* common/command.c */
> >  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
> > 
> 

[U-Boot] [PATCH v4 3/9] efi_driver: add name to driver binding protocol

[AKASHI Takahiro]

On Thu, Jan 17, 2019 at 07:58:17AM +0100, Heinrich Schuchardt wrote:
> On 1/17/19 6:33 AM, AKASHI Takahiro wrote:
> > You raised a couple of questions to me.
> > 
> > On Tue, Jan 15, 2019 at 04:41:08AM +0100, Heinrich Schuchardt wrote:
> >> On 1/15/19 3:55 AM, AKASHI Takahiro wrote:
> >>> This new field will be shown as a driver's name in "efitool drivers"
> >>> command.
> >>
> >> We can have drivers supplied by U-Boot
> > 
> > I assume that what you mention here is a UCLASS_EFI driver.
> > 
> > What's the problem is;
> > efi_add_driver() adds EFI_DRIVER_BINDING_PROTOCOL with
> > *efi_driver_binding_extended_protocol* interface, which is NOT compatible
> > with EFI_DRIVER_BINDING_PROTOCOL.
> > On the other hand, for example, in your efi_selftest_controller test
> > a test driver is set up by installing EFI_DRIVER_BINDING_PROTOCOL
> > with EFI_DRIVER_BINDING_PROTOCOL interface.
> > 
> > So we have no way to distinguish the two cases(handles) and cannot
> > deal with them properly.
> 
> Correct. It is allowable to add private fields to a protocol. EDK2 does
> the same. But we cannot make any assumptions about the private fields here.

I misunderstood something here, but anyhow,

> > 
> >> and drivers supplied by an EFI
> >> binary that we recently installed via the bootefi command.
> >>
> >> A driver installed via the bootefi command will not have allocated
> >> memory for the extra fields.
> > 
> > There is no good example of driver of such kind.
> > I don't know how we can retrieve a meaningful "driver name."
> 
> The UEFI spec does not foresee any name field.

Edk2's shell does get a driver's name by using COMPONENT_NAME2_PROTOCOL,
and we'd better do the same way.
So "<NULL>" will be printed with my patch for now.

-Takahiro Akashi

> The interesting information is: on which handle did the driver install
> which protocol.
> 
> This information could be gathered by looping over all protocols on all
> handles and looking for an OpenProtocolInformation where the driver is
> the controller.
> 
> For me it would be fine if we leave that to a later patch.
> 
> Best regards
> 
> Heinrich
> 
> > 
> >> So you cannot use the name field in your "efitool drivers" command.
> > 
> > Any suggestion?
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> ---
> >>>  include/efi_driver.h        | 1 +
> >>>  lib/efi_driver/efi_uclass.c | 1 +
> >>>  2 files changed, 2 insertions(+)
> >>>
> >>> diff --git a/include/efi_driver.h b/include/efi_driver.h
> >>> index 840483a416a4..ee8867816094 100644
> >>> --- a/include/efi_driver.h
> >>> +++ b/include/efi_driver.h
> >>> @@ -34,6 +34,7 @@ struct efi_driver_ops {
> >>>   * This structure adds internal fields to the driver binding protocol.
> >>>   */
> >>>  struct efi_driver_binding_extended_protocol {
> >>> +	const char *name;
> >>>  	struct efi_driver_binding_protocol bp;
> >>>  	const struct efi_driver_ops *ops;
> >>>  };
> >>> diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c
> >>> index bb86ffd399c3..8bbaa02d490e 100644
> >>> --- a/lib/efi_driver/efi_uclass.c
> >>> +++ b/lib/efi_driver/efi_uclass.c
> >>> @@ -271,6 +271,7 @@ static efi_status_t efi_add_driver(struct driver *drv)
> >>>  	bp->bp.stop = efi_uc_stop;
> >>>  	bp->bp.version = 0xffffffff;
> >>>  	bp->ops = drv->ops;
> >>> +	bp->name = drv->name;
> >>>  
> >>>  	ret = efi_create_handle(&bp->bp.driver_binding_handle);
> >>>  	if (ret != EFI_SUCCESS) {
> >>>
> >>
> > 
>