All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH v3 0/8] cmd: add efishell for efi environment
@ 2018-12-18  5:05 AKASHI Takahiro
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 1/8] cmd: add efishell command AKASHI Takahiro
                   ` (7 more replies)
  0 siblings, 8 replies; 30+ messages in thread
From: AKASHI Takahiro @ 2018-12-18  5:05 UTC (permalink / raw)
  To: u-boot

# This patch set contains only efishell-specific part of my original
  patch[1]. See also the other patch[2].

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:
=> efishell boot add 1 SHELL mmc 0:1 /Shell.efi ""
=> efishell boot add 2 HELLO mmc 0:1 /hello.efi ""
=> efishell 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: 

=> efishell boot order 1 2
=> efishell 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 "efishell 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

   (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:
=> efishell devices				; print uefi devices
=> efishell drivers				; print uefi drivers
=> efishell images				; print loaded images
=> efishell dh					; print uefi handles
=> efishell memmap				; dump uefi memory map

# I admit there is some room to improve the output from those commands.

[1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
[2] https://lists.denx.de/pipermail/u-boot/2018-December/352403.html

Enjoy!
-Takahiro Akashi

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 (8):
  cmd: add efishell command
  cmd: efishell: add devices command
  cmd: efishell: add drivers command
  cmd: efishell: add images command
  cmd: efishell: add memmap command
  cmd: efishell: add dh command
  cmd: efishell: export uefi variable helper functions
  cmd: env: add "-e" option for handling UEFI variables

 cmd/Kconfig       |   10 +
 cmd/Makefile      |    1 +
 cmd/efishell.c    | 1000 +++++++++++++++++++++++++++++++++++++++++++++
 cmd/nvedit.c      |   61 ++-
 include/command.h |    4 +
 5 files changed, 1074 insertions(+), 2 deletions(-)
 create mode 100644 cmd/efishell.c

-- 
2.19.1

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 1/8] cmd: add efishell command
  2018-12-18  5:05 [U-Boot] [PATCH v3 0/8] cmd: add efishell for efi environment AKASHI Takahiro
@ 2018-12-18  5:05 ` AKASHI Takahiro
  2018-12-30 15:44   ` Heinrich Schuchardt
  2018-12-30 23:47   ` Heinrich Schuchardt
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 2/8] cmd: efishell: add devices command AKASHI Takahiro
                   ` (6 subsequent siblings)
  7 siblings, 2 replies; 30+ messages in thread
From: AKASHI Takahiro @ 2018-12-18  5:05 UTC (permalink / raw)
  To: u-boot

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, efishell, helps address these issues and give us
more friendly interfaces:
 * efishell boot add: add BootXXXX variable
 * efishell boot rm: remove BootXXXX variable
 * efishell boot dump: display all BootXXXX variables
 * efishell boot order: set/display a boot order (BootOrder)
 * efishell setvar: set an UEFI variable (with limited functionality)
 * efishell 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/efishell.c | 673 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 684 insertions(+)
 create mode 100644 cmd/efishell.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index f2f3b5e2b76b..a8a4bf7db45e 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1390,6 +1390,16 @@ config CMD_DISPLAY
 	  displayed on a simple board-specific display. Implement
 	  display_putc() to use it.
 
+config CMD_EFISHELL
+	bool "Enable the 'efishell' command for EFI environment"
+	depends on EFI_LOADER
+	default n
+	help
+	  Enable the 'efishell' 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 5ec2f9e8ebfd..0258d8a373b1 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -49,6 +49,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_EFISHELL) += efishell.o
 obj-$(CONFIG_CMD_ELF) += elf.o
 obj-$(CONFIG_HUSH_PARSER) += exit.o
 obj-$(CONFIG_CMD_EXT4) += ext4.o
diff --git a/cmd/efishell.c b/cmd/efishell.c
new file mode 100644
index 000000000000..5819e52cf575
--- /dev/null
+++ b/cmd/efishell.c
@@ -0,0 +1,673 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  EFI 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] = 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 EFI 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;
+	unsigned long size = 0;
+	u16 *var_name16, *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:
+	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;
+	unsigned long size;
+	int ret;
+
+	if (argc < 6 || argc > 7)
+		return CMD_RET_USAGE;
+
+	id = (int)simple_strtoul(argv[1], &endp, 0);
+	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, 0);
+		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,
+				       EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				       EFI_VARIABLE_RUNTIME_ACCESS, 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;
+	unsigned long 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;
+	unsigned long 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_order(int argc, char * const argv[])
+{
+	u16 *bootorder = NULL;
+	unsigned long 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, 0);
+		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, "order"))
+		return do_efi_boot_order(argc, argv);
+	else
+		return CMD_RET_USAGE;
+}
+
+/* Interpreter command to configure EFI environment */
+static int do_efishell(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 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;
+	}
+
+	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 efishell_help_text[] =
+	"  - EFI Shell-like interface to configure EFI environment\n"
+	"\n"
+	"efishell boot add <bootid> <label> <interface> <device>[:<part>] <file path> <option>\n"
+	"  - set uefi's BOOT variable\n"
+	"efishell boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
+	"  - set/display uefi boot order\n"
+	"efishell boot dump\n"
+	"  - display all uefi's BOOT variables\n"
+	"efishell boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
+	"  - set/display uefi boot order\n"
+	"\n"
+	"efishell dumpvar [<name>]\n"
+	"  - get uefi variable's value\n"
+	"efishell setvar <name> [<value>]\n"
+	"  - set/delete uefi variable's value\n"
+	"    <value> may be \"=\"...\"\", \"=0x...\" (set) or \"=\" (delete)\n";
+#endif
+
+U_BOOT_CMD(
+	efishell, 10, 0, do_efishell,
+	"Configure EFI environment",
+	efishell_help_text
+);
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 2/8] cmd: efishell: add devices command
  2018-12-18  5:05 [U-Boot] [PATCH v3 0/8] cmd: add efishell for efi environment AKASHI Takahiro
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 1/8] cmd: add efishell command AKASHI Takahiro
@ 2018-12-18  5:05 ` AKASHI Takahiro
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 3/8] cmd: efishell: add drivers command AKASHI Takahiro
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 30+ messages in thread
From: AKASHI Takahiro @ 2018-12-18  5:05 UTC (permalink / raw)
  To: u-boot

"devices" command prints all the uefi variables on the system.
=> efishell devices
Device Name
============================================
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
				HD(2,MBR,0x086246ba,0x40800,0x3f800)
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/\
				HD(1,MBR,0x086246ba,0x800,0x40000)

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

diff --git a/cmd/efishell.c b/cmd/efishell.c
index 5819e52cf575..929b6343b1b2 100644
--- a/cmd/efishell.c
+++ b/cmd/efishell.c
@@ -18,6 +18,8 @@
 #include <linux/ctype.h>
 #include <asm/global_data.h>
 
+static const struct efi_boot_services *bs;
+
 static void dump_var_data(char *data, unsigned long len)
 {
 	char *start, *end, *p;
@@ -263,6 +265,84 @@ out:
 	return ret;
 }
 
+static efi_handle_t *efi_get_handles_by_proto(efi_guid_t *guid)
+{
+	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 NULL;
+
+			ret = bs->locate_handle(BY_PROTOCOL, guid, NULL, &size,
+						handles);
+		}
+		if (ret != EFI_SUCCESS) {
+			free(handles);
+			return NULL;
+		}
+	} else {
+		ret = bs->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
+		if (ret == EFI_BUFFER_TOO_SMALL) {
+			handles = calloc(1, size);
+			if (!handles)
+				return NULL;
+
+			ret = bs->locate_handle(ALL_HANDLES, NULL, NULL, &size,
+						handles);
+		}
+		if (ret != EFI_SUCCESS) {
+			free(handles);
+			return NULL;
+		}
+	}
+
+	return handles;
+}
+
+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 = NULL, *handle;
+	u16 *devname;
+
+	handles = efi_get_handles_by_proto(NULL);
+	if (!handles)
+		return CMD_RET_SUCCESS;
+
+	printf("Device Name\n");
+	printf("============================================\n");
+	for (handle = handles; *handle; handle++) {
+		if (!efi_get_device_handle_info(*handle, &devname)) {
+			printf("%ls\n", devname);
+			efi_free_pool(devname);
+		}
+		handle++;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(int argc, char * const argv[])
 {
 	int id;
@@ -625,6 +705,8 @@ static int do_efishell(cmd_tbl_t *cmdtp, int flag,
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
+	bs = systab.boottime;
+
 	argc--; argv++;
 	command = argv[0];
 
@@ -642,6 +724,8 @@ static int do_efishell(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;
 }
@@ -663,7 +747,9 @@ static char efishell_help_text[] =
 	"  - get uefi variable's value\n"
 	"efishell setvar <name> [<value>]\n"
 	"  - set/delete uefi variable's value\n"
-	"    <value> may be \"=\"...\"\", \"=0x...\" (set) or \"=\" (delete)\n";
+	"    <value> may be \"=\"...\"\", \"=0x...\" (set) or \"=\" (delete)\n"
+	"efishell devices\n"
+	"  - show uefi devices\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 3/8] cmd: efishell: add drivers command
  2018-12-18  5:05 [U-Boot] [PATCH v3 0/8] cmd: add efishell for efi environment AKASHI Takahiro
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 1/8] cmd: add efishell command AKASHI Takahiro
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 2/8] cmd: efishell: add devices command AKASHI Takahiro
@ 2018-12-18  5:05 ` AKASHI Takahiro
  2018-12-20  7:51   ` Heinrich Schuchardt
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 4/8] cmd: efishell: add images command AKASHI Takahiro
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 30+ messages in thread
From: AKASHI Takahiro @ 2018-12-18  5:05 UTC (permalink / raw)
  To: u-boot

"drivers" command prints all the uefi drivers on the system.
=> efi drivers
Driver Name     Image Path
============================================
(unknown)       <NULL>+(not found)
    guid: 18a031ab-b443-4d1a-a5c0-0c09261e9f71

Currently, no useful information can be printed.

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

diff --git a/cmd/efishell.c b/cmd/efishell.c
index 929b6343b1b2..ed8de9e0355d 100644
--- a/cmd/efishell.c
+++ b/cmd/efishell.c
@@ -343,6 +343,95 @@ 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_driver_binding_protocol *binding;
+	struct efi_loaded_image *image;
+	efi_status_t ret;
+
+	ret = bs->open_protocol(handle, &efi_guid_driver_binding_protocol,
+				(void **)&binding, NULL, NULL,
+				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+	if (ret != EFI_SUCCESS)
+		return -1;
+
+	ret = bs->open_protocol(binding->image_handle, &efi_guid_loaded_image,
+				(void **)&image, NULL /* FIXME */, NULL,
+				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+	if (ret != EFI_SUCCESS)
+		goto e_out;
+
+	/*
+	 * TODO:
+	 * handle image->device_handle,
+	 * use append_device_path()
+	 */
+	*devname = NULL;
+	*filename = efi_dp_str(image->file_path);
+
+	return 0;
+
+e_out:
+	*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("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)) {
+			printf("%-16s%ls+%ls\n",
+			       "(unknown)", devname, filename);
+			efi_free_pool(devname);
+			efi_free_pool(filename);
+
+			/* TODO: no other info */
+			struct efi_object *efiobj;
+			struct list_head *lhandle;
+			struct efi_handler *protocol;
+
+			efiobj = efi_search_obj(*handle);
+			list_for_each(lhandle, &efiobj->protocols) {
+				protocol = list_entry(lhandle,
+						      struct efi_handler, link);
+				printf("    guid: %pUl\n", protocol->guid);
+			}
+		}
+		handle++;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(int argc, char * const argv[])
 {
 	int id;
@@ -726,6 +815,8 @@ static int do_efishell(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;
 }
@@ -749,7 +840,9 @@ static char efishell_help_text[] =
 	"  - set/delete uefi variable's value\n"
 	"    <value> may be \"=\"...\"\", \"=0x...\" (set) or \"=\" (delete)\n"
 	"efishell devices\n"
-	"  - show uefi devices\n";
+	"  - show uefi devices\n"
+	"efishell drivers\n"
+	"  - show uefi drivers\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 4/8] cmd: efishell: add images command
  2018-12-18  5:05 [U-Boot] [PATCH v3 0/8] cmd: add efishell for efi environment AKASHI Takahiro
                   ` (2 preceding siblings ...)
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 3/8] cmd: efishell: add drivers command AKASHI Takahiro
@ 2018-12-18  5:05 ` AKASHI Takahiro
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 5/8] cmd: efishell: add memmap command AKASHI Takahiro
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 30+ messages in thread
From: AKASHI Takahiro @ 2018-12-18  5:05 UTC (permalink / raw)
  To: u-boot

"images" command prints loaded images-related information.

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

diff --git a/cmd/efishell.c b/cmd/efishell.c
index ed8de9e0355d..010870df63c8 100644
--- a/cmd/efishell.c
+++ b/cmd/efishell.c
@@ -432,6 +432,13 @@ static int do_efi_show_drivers(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;
@@ -817,6 +824,8 @@ static int do_efishell(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, "images"))
+		return do_efi_show_images(argc, argv);
 	else
 		return CMD_RET_USAGE;
 }
@@ -842,7 +851,9 @@ static char efishell_help_text[] =
 	"efishell devices\n"
 	"  - show uefi devices\n"
 	"efishell drivers\n"
-	"  - show uefi drivers\n";
+	"  - show uefi drivers\n"
+	"efishell images\n"
+	"  - show loaded images\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 5/8] cmd: efishell: add memmap command
  2018-12-18  5:05 [U-Boot] [PATCH v3 0/8] cmd: add efishell for efi environment AKASHI Takahiro
                   ` (3 preceding siblings ...)
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 4/8] cmd: efishell: add images command AKASHI Takahiro
@ 2018-12-18  5:05 ` AKASHI Takahiro
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 6/8] cmd: efishell: add dh command AKASHI Takahiro
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 30+ messages in thread
From: AKASHI Takahiro @ 2018-12-18  5:05 UTC (permalink / raw)
  To: u-boot

"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/efishell.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

diff --git a/cmd/efishell.c b/cmd/efishell.c
index 010870df63c8..5a81a627d616 100644
--- a/cmd/efishell.c
+++ b/cmd/efishell.c
@@ -439,6 +439,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 *map = NULL;
+	efi_uintn_t map_size = 0;
+	int i, sep;
+	efi_status_t ret;
+
+	ret = efi_get_memory_map(&map_size, map, NULL, NULL, NULL);
+	if (ret == EFI_BUFFER_TOO_SMALL) {
+		map = malloc(map_size);
+		if (!map)
+			return CMD_RET_FAILURE;
+		ret = efi_get_memory_map(&map_size, map, NULL, NULL, NULL);
+	}
+	if (ret != EFI_SUCCESS) {
+		free(map);
+		return CMD_RET_FAILURE;
+	}
+
+	printf("Type             Start            End              Attributes\n");
+	printf("================ ================ ================ ==========\n");
+	for (i = 0; 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(map);
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(int argc, char * const argv[])
 {
 	int id;
@@ -826,6 +898,8 @@ static int do_efishell(cmd_tbl_t *cmdtp, int flag,
 		return do_efi_show_drivers(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;
 }
@@ -853,7 +927,9 @@ static char efishell_help_text[] =
 	"efishell drivers\n"
 	"  - show uefi drivers\n"
 	"efishell images\n"
-	"  - show loaded images\n";
+	"  - show loaded images\n"
+	"efishell memmap\n"
+	"  - show uefi memory map\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 6/8] cmd: efishell: add dh command
  2018-12-18  5:05 [U-Boot] [PATCH v3 0/8] cmd: add efishell for efi environment AKASHI Takahiro
                   ` (4 preceding siblings ...)
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 5/8] cmd: efishell: add memmap command AKASHI Takahiro
@ 2018-12-18  5:05 ` AKASHI Takahiro
  2018-12-20  7:49   ` Heinrich Schuchardt
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 7/8] cmd: efishell: export uefi variable helper functions AKASHI Takahiro
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables AKASHI Takahiro
  7 siblings, 1 reply; 30+ messages in thread
From: AKASHI Takahiro @ 2018-12-18  5:05 UTC (permalink / raw)
  To: u-boot

"dh" command prints all the uefi handles used in the system.
=> efishell dh
(T.B.D.)
0: (protocol info not available)
1: (protocol info not available)
2: (protocol info not available)
3: (protocol info not available)
4: (protocol info not available)
5: (protocol info not available)
6: (protocol info not available)
7: (protocol info not available)
8: (protocol info not available)
9: (protocol info not available)
10: (protocol info not available)
11: (protocol info not available)
12: (protocol info not available)
13: (protocol info not available)
14: (protocol info not available)
15: (protocol info not available)

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

diff --git a/cmd/efishell.c b/cmd/efishell.c
index 5a81a627d616..47ad77606062 100644
--- a/cmd/efishell.c
+++ b/cmd/efishell.c
@@ -511,6 +511,33 @@ static int do_efi_show_memmap(int argc, char * const argv[])
 	return CMD_RET_SUCCESS;
 }
 
+static char *efi_get_proto_info(efi_handle_t handle)
+{
+	return strdup("(protocol info not available)");
+}
+
+static int do_efi_show_handles(int argc, char * const argv[])
+{
+	efi_handle_t *handles = NULL, *handle;
+	char *info;
+	int i;
+
+	handles = efi_get_handles_by_proto(NULL);
+	if (!handles)
+		return CMD_RET_SUCCESS;
+
+	for (handle = handles, i = 0; *handle; handle++, i++) {
+		/* TODO: depends on protocols */
+		info = efi_get_proto_info(*handle);
+		printf("%d: %s\n", i, info ?: "");
+		free(info);
+	}
+
+	free(handles);
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_efi_boot_add(int argc, char * const argv[])
 {
 	int id;
@@ -900,6 +927,8 @@ static int do_efishell(cmd_tbl_t *cmdtp, int flag,
 		return do_efi_show_images(argc, argv);
 	else if (!strcmp(command, "memmap"))
 		return do_efi_show_memmap(argc, argv);
+	else if (!strcmp(command, "dh"))
+		return do_efi_show_handles(argc, argv);
 	else
 		return CMD_RET_USAGE;
 }
@@ -929,7 +958,9 @@ static char efishell_help_text[] =
 	"efishell images\n"
 	"  - show loaded images\n"
 	"efishell memmap\n"
-	"  - show uefi memory map\n";
+	"  - show uefi memory map\n"
+	"efishell dh\n"
+	"  - show uefi handles\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 7/8] cmd: efishell: export uefi variable helper functions
  2018-12-18  5:05 [U-Boot] [PATCH v3 0/8] cmd: add efishell for efi environment AKASHI Takahiro
                   ` (5 preceding siblings ...)
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 6/8] cmd: efishell: add dh command AKASHI Takahiro
@ 2018-12-18  5:05 ` AKASHI Takahiro
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables AKASHI Takahiro
  7 siblings, 0 replies; 30+ messages in thread
From: AKASHI Takahiro @ 2018-12-18  5:05 UTC (permalink / raw)
  To: u-boot

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/efishell.c    | 38 ++++++++++++++++++++++++++++++++++----
 include/command.h |  4 ++++
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/cmd/efishell.c b/cmd/efishell.c
index 47ad77606062..42a29de7617c 100644
--- a/cmd/efishell.c
+++ b/cmd/efishell.c
@@ -65,7 +65,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};
@@ -128,6 +128,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;
@@ -225,7 +240,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;
 	unsigned long size = 0;
@@ -265,6 +280,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 efi_handle_t *efi_get_handles_by_proto(efi_guid_t *guid)
 {
 	efi_handle_t *handles = NULL;
@@ -916,9 +946,9 @@ static int do_efishell(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 9b7b876585d9..5b081ae94cf3 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)
 int do_bootefi_bootmgr_exec(int boot_id);
 #endif
+#if defined(CONFIG_CMD_EFISHELL)
+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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables
  2018-12-18  5:05 [U-Boot] [PATCH v3 0/8] cmd: add efishell for efi environment AKASHI Takahiro
                   ` (6 preceding siblings ...)
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 7/8] cmd: efishell: export uefi variable helper functions AKASHI Takahiro
@ 2018-12-18  5:05 ` AKASHI Takahiro
  2018-12-18  6:07   ` Heinrich Schuchardt
  7 siblings, 1 reply; 30+ messages in thread
From: AKASHI Takahiro @ 2018-12-18  5:05 UTC (permalink / raw)
  To: u-boot

"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 c0facabfc4fe..8168c963ac9d 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_EFISHELL)
+	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_EFISHELL)
+	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_EFISHELL)
+	"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_EFISHELL)
+	"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_EFISHELL)
+	"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_EFISHELL)
+	"-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'",
@@ -1343,7 +1400,7 @@ U_BOOT_CMD(
 U_BOOT_CMD_COMPLETE(
 	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
 	"run commands in an environment variable",
-#if defined(CONFIG_CMD_BOOTEFI)
+#if defined(CONFIG_CMD_EFISHELL)
 	"var -e [BootXXXX]\n"
 	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
 #else
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables AKASHI Takahiro
@ 2018-12-18  6:07   ` Heinrich Schuchardt
  2018-12-19  1:49     ` AKASHI Takahiro
  0 siblings, 1 reply; 30+ messages in thread
From: Heinrich Schuchardt @ 2018-12-18  6:07 UTC (permalink / raw)
  To: u-boot

On 12/18/18 6:05 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>

Hello Takahiro,

in several patch series you are implementing multiple interactive
commands that concern

- handling of EFI variables
- executing EFI binaries
- managing boot sequence

I very much appreciate your effort to provide an independent UEFI shell
implementation. What I am worried about is that your current patches
make it part of the monolithic U-Boot binary.

This design has multiple drawbacks:

The memory size available for U-Boot is very limited for many devices.
We already had to disable EFI_LOADER for some boards due to this
limitations. Hence we want to keep everything out of the U-Boot binary
that does not serve the primary goal of loading and executing the next
binary.

The UEFI forum has published a UEFI Shell specification which is very
extensive. We still have a lot of deficiencies in U-Boot's UEFI API
implementation. By merging in parts of an UEFI shell implementation our
project looses focus. There is an EDK2 implementation of said
specification. If we fix the remaining bugs of the EFI API
implementation in U-Boot we could simply run the EDK2 shell which
provides all that is needed for interactive work.

With you monolithic approach your UEFI shell implementation can neither
be used with other UEFI API implementations than U-Boot nor can it be
tested against other API implementations.

Due to these considerations I suggest that you build your UEFI shell
implementation as a separate UEFI binary (like helloworld.efi). You may
offer an embedding of the binary (like the bootefi hello command) into
the finally linked U-Boot binary via a configuration variable. Please,
put the shell implementation into a separate directory. You may want to
designate yourself as maintainer (in file MAINTAINERS).

Best regards

Heinrich


> ---
>  cmd/nvedit.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index c0facabfc4fe..8168c963ac9d 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_EFISHELL)
> +	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_EFISHELL)
> +	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_EFISHELL)
> +	"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_EFISHELL)
> +	"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_EFISHELL)
> +	"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_EFISHELL)
> +	"-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'",
> @@ -1343,7 +1400,7 @@ U_BOOT_CMD(
>  U_BOOT_CMD_COMPLETE(
>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
>  	"run commands in an environment variable",
> -#if defined(CONFIG_CMD_BOOTEFI)
> +#if defined(CONFIG_CMD_EFISHELL)
>  	"var -e [BootXXXX]\n"
>  	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
>  #else
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables
  2018-12-18  6:07   ` Heinrich Schuchardt
@ 2018-12-19  1:49     ` AKASHI Takahiro
  2018-12-19 12:23       ` Heinrich Schuchardt
  0 siblings, 1 reply; 30+ messages in thread
From: AKASHI Takahiro @ 2018-12-19  1:49 UTC (permalink / raw)
  To: u-boot

Heinrich,

On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote:
> On 12/18/18 6:05 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>
> 
> Hello Takahiro,
> 
> in several patch series you are implementing multiple interactive
> commands that concern
> 
> - handling of EFI variables
> - executing EFI binaries
> - managing boot sequence
> 
> I very much appreciate your effort to provide an independent UEFI shell
> implementation. What I am worried about is that your current patches
> make it part of the monolithic U-Boot binary.

First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's
comment on v2. So you can disable efishell command if you don't want it.

Are you still worried?

> This design has multiple drawbacks:
> 
> The memory size available for U-Boot is very limited for many devices.
> We already had to disable EFI_LOADER for some boards due to this
> limitations. Hence we want to keep everything out of the U-Boot binary
> that does not serve the primary goal of loading and executing the next
> binary.

I don't know your point here. If EFI_LOADER is disabled, efishell
will never be compiled in.

> The UEFI forum has published a UEFI Shell specification which is very
> extensive. We still have a lot of deficiencies in U-Boot's UEFI API
> implementation. By merging in parts of an UEFI shell implementation our
> project looses focus.

What is "our project?" What is "focus?"
I'm just asking as I want to share that information with you.

> There is an EDK2 implementation of said
> specification. If we fix the remaining bugs of the EFI API
> implementation in U-Boot we could simply run the EDK2 shell which
> provides all that is needed for interactive work.
> 
> With you monolithic approach your UEFI shell implementation can neither
> be used with other UEFI API implementations than U-Boot nor can it be
> tested against other API implementations.

Let me explain my stance.
My efishell is basically something like a pursuit as well as
a debug/test tool which was and is still quite useful for me.
Without it, I would have completed (most of) my efi-related work so far.
So I believe that it will also be useful for other people who may want
to get involved and play with u-boot's efi environment.

I have never intended to fully implement a shell which is to be compliant
with UEFI specification while I'm trying to mimick some command
interfaces for convenience. UEFI shell, as you know, provides plenty
of "protocols" on which some UEFI applications, including UEFI SCT,
reply. I will never implement it with my efishell.

I hope that my efishell is a quick and easy way of learning more about
u-boot's uefi environment. I will be even happier if more people
get involved there.

> Due to these considerations I suggest that you build your UEFI shell
> implementation as a separate UEFI binary (like helloworld.efi). You may
> offer an embedding of the binary (like the bootefi hello command) into
> the finally linked U-Boot binary via a configuration variable. Please,
> put the shell implementation into a separate directory. You may want to
> designate yourself as maintainer (in file MAINTAINERS).

Yeah, your suggestion is reasonable and I have thought of it before.
There are, however, several reasons that I haven't done so; particularly,
efishell is implemented not only with boottime services but also
other helper functions, say, from device path utilities. Exporting them
as libraries is possible but I don't think that it would be so valuable.

Even if efishell is a separate application, it will not contribute to
reduce the total footprint if it is embedded along with u-boot binary.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> 
> > ---
> >  cmd/nvedit.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 59 insertions(+), 2 deletions(-)
> > 
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index c0facabfc4fe..8168c963ac9d 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_EFISHELL)
> > +	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_EFISHELL)
> > +	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_EFISHELL)
> > +	"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_EFISHELL)
> > +	"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_EFISHELL)
> > +	"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_EFISHELL)
> > +	"-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'",
> > @@ -1343,7 +1400,7 @@ U_BOOT_CMD(
> >  U_BOOT_CMD_COMPLETE(
> >  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
> >  	"run commands in an environment variable",
> > -#if defined(CONFIG_CMD_BOOTEFI)
> > +#if defined(CONFIG_CMD_EFISHELL)
> >  	"var -e [BootXXXX]\n"
> >  	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> >  #else
> > 
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables
  2018-12-19  1:49     ` AKASHI Takahiro
@ 2018-12-19 12:23       ` Heinrich Schuchardt
  2018-12-23  1:56         ` Alexander Graf
  0 siblings, 1 reply; 30+ messages in thread
From: Heinrich Schuchardt @ 2018-12-19 12:23 UTC (permalink / raw)
  To: u-boot

On 12/19/18 2:49 AM, AKASHI Takahiro wrote:
> Heinrich,
> 
> On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote:
>> On 12/18/18 6:05 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>
>>
>> Hello Takahiro,
>>
>> in several patch series you are implementing multiple interactive
>> commands that concern
>>
>> - handling of EFI variables
>> - executing EFI binaries
>> - managing boot sequence
>>
>> I very much appreciate your effort to provide an independent UEFI shell
>> implementation. What I am worried about is that your current patches
>> make it part of the monolithic U-Boot binary.
> 
> First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's
> comment on v2. So you can disable efishell command if you don't want it.
> 
> Are you still worried?
> 
>> This design has multiple drawbacks:
>>
>> The memory size available for U-Boot is very limited for many devices.
>> We already had to disable EFI_LOADER for some boards due to this
>> limitations. Hence we want to keep everything out of the U-Boot binary
>> that does not serve the primary goal of loading and executing the next
>> binary.
> 
> I don't know your point here. If EFI_LOADER is disabled, efishell
> will never be compiled in.
> 
>> The UEFI forum has published a UEFI Shell specification which is very
>> extensive. We still have a lot of deficiencies in U-Boot's UEFI API
>> implementation. By merging in parts of an UEFI shell implementation our
>> project looses focus.
> 
> What is "our project?" What is "focus?"
> I'm just asking as I want to share that information with you.
> 
>> There is an EDK2 implementation of said
>> specification. If we fix the remaining bugs of the EFI API
>> implementation in U-Boot we could simply run the EDK2 shell which
>> provides all that is needed for interactive work.
>>
>> With you monolithic approach your UEFI shell implementation can neither
>> be used with other UEFI API implementations than U-Boot nor can it be
>> tested against other API implementations.
> 
> Let me explain my stance.
> My efishell is basically something like a pursuit as well as
> a debug/test tool which was and is still quite useful for me.
> Without it, I would have completed (most of) my efi-related work so far.
> So I believe that it will also be useful for other people who may want
> to get involved and play with u-boot's efi environment.

On SD-Cards U-Boot is installed between the MBR and the first partition.
On other devices it is put into a very small ROM. Both ways the maximum
size is rather limited.

U-Boot provides all that is needed to load and execute an EFI binary. So
you can put your efishell as file into the EFI partition like you would
install the EDK2 shell.

The only hardshift this approach brings is that you have to implement
your own printf because UEFI does not offer formatted output. But this
can be copied from lib/efi_selftest/efi_selftest_console.c.

The same decision I took for booting from iSCSI. I did not try to put an
iSCSI driver into U-Boot instead I use iPXE as an executable that is
loaded from the EFI partition.

> 
> I have never intended to fully implement a shell which is to be compliant
> with UEFI specification while I'm trying to mimick some command
> interfaces for convenience. UEFI shell, as you know, provides plenty
> of "protocols" on which some UEFI applications, including UEFI SCT,
> reply. I will never implement it with my efishell.
> 
> I hope that my efishell is a quick and easy way of learning more about
> u-boot's uefi environment. I will be even happier if more people
> get involved there.
> 
>> Due to these considerations I suggest that you build your UEFI shell
>> implementation as a separate UEFI binary (like helloworld.efi). You may
>> offer an embedding of the binary (like the bootefi hello command) into
>> the finally linked U-Boot binary via a configuration variable. Please,
>> put the shell implementation into a separate directory. You may want to
>> designate yourself as maintainer (in file MAINTAINERS).
> 
> Yeah, your suggestion is reasonable and I have thought of it before.
> There are, however, several reasons that I haven't done so; particularly,
> efishell is implemented not only with boottime services but also
> other helper functions, say, from device path utilities. Exporting them
> as libraries is possible but I don't think that it would be so valuable.
> 
> Even if efishell is a separate application, it will not contribute to
> reduce the total footprint if it is embedded along with u-boot binary.

That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into
the U-Boot binary - is default no. Same I would do for efishell.efi.

Best regards

Heinrich

> 
> Thanks,
> -Takahiro Akashi
> 
>> Best regards
>>
>> Heinrich
>>
>>
>>> ---
>>>  cmd/nvedit.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 59 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
>>> index c0facabfc4fe..8168c963ac9d 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_EFISHELL)
>>> +	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_EFISHELL)
>>> +	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_EFISHELL)
>>> +	"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_EFISHELL)
>>> +	"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_EFISHELL)
>>> +	"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_EFISHELL)
>>> +	"-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'",
>>> @@ -1343,7 +1400,7 @@ U_BOOT_CMD(
>>>  U_BOOT_CMD_COMPLETE(
>>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
>>>  	"run commands in an environment variable",
>>> -#if defined(CONFIG_CMD_BOOTEFI)
>>> +#if defined(CONFIG_CMD_EFISHELL)
>>>  	"var -e [BootXXXX]\n"
>>>  	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
>>>  #else
>>>
>>
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 6/8] cmd: efishell: add dh command
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 6/8] cmd: efishell: add dh command AKASHI Takahiro
@ 2018-12-20  7:49   ` Heinrich Schuchardt
  2018-12-25  5:32     ` AKASHI Takahiro
  0 siblings, 1 reply; 30+ messages in thread
From: Heinrich Schuchardt @ 2018-12-20  7:49 UTC (permalink / raw)
  To: u-boot

On 12/18/18 6:05 AM, AKASHI Takahiro wrote:
> "dh" command prints all the uefi handles used in the system.
> => efishell dh
> (T.B.D.)
> 0: (protocol info not available)
> 1: (protocol info not available)
> 2: (protocol info not available)
> 3: (protocol info not available)
> 4: (protocol info not available)
> 5: (protocol info not available)
> 6: (protocol info not available)
> 7: (protocol info not available)
> 8: (protocol info not available)
> 9: (protocol info not available)
> 10: (protocol info not available)
> 11: (protocol info not available)
> 12: (protocol info not available)
> 13: (protocol info not available)
> 14: (protocol info not available)
> 15: (protocol info not available)
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/efishell.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/efishell.c b/cmd/efishell.c
> index 5a81a627d616..47ad77606062 100644
> --- a/cmd/efishell.c
> +++ b/cmd/efishell.c
> @@ -511,6 +511,33 @@ static int do_efi_show_memmap(int argc, char * const argv[])
>  	return CMD_RET_SUCCESS;
>  }
>  
> +static char *efi_get_proto_info(efi_handle_t handle)
> +{
> +	return strdup("(protocol info not available)");
Shouldn't this enumerate all protocol GUIDs installed on the handles by
calling ProtocolsPerHandle()?

Also instances of installed drivers identifiable by the driver binding
protocol might be interesting.

Best regards

Heinrich

> +}
> +
> +static int do_efi_show_handles(int argc, char * const argv[])
> +{
> +	efi_handle_t *handles = NULL, *handle;
> +	char *info;
> +	int i;
> +
> +	handles = efi_get_handles_by_proto(NULL);
> +	if (!handles)
> +		return CMD_RET_SUCCESS;
> +
> +	for (handle = handles, i = 0; *handle; handle++, i++) {
> +		/* TODO: depends on protocols */
> +		info = efi_get_proto_info(*handle);
> +		printf("%d: %s\n", i, info ?: "");
> +		free(info);
> +	}
> +
> +	free(handles);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  static int do_efi_boot_add(int argc, char * const argv[])
>  {
>  	int id;
> @@ -900,6 +927,8 @@ static int do_efishell(cmd_tbl_t *cmdtp, int flag,
>  		return do_efi_show_images(argc, argv);
>  	else if (!strcmp(command, "memmap"))
>  		return do_efi_show_memmap(argc, argv);
> +	else if (!strcmp(command, "dh"))
> +		return do_efi_show_handles(argc, argv);
>  	else
>  		return CMD_RET_USAGE;
>  }
> @@ -929,7 +958,9 @@ static char efishell_help_text[] =
>  	"efishell images\n"
>  	"  - show loaded images\n"
>  	"efishell memmap\n"
> -	"  - show uefi memory map\n";
> +	"  - show uefi memory map\n"
> +	"efishell dh\n"
> +	"  - show uefi handles\n";
>  #endif
>  
>  U_BOOT_CMD(
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 3/8] cmd: efishell: add drivers command
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 3/8] cmd: efishell: add drivers command AKASHI Takahiro
@ 2018-12-20  7:51   ` Heinrich Schuchardt
  2018-12-25  7:22     ` AKASHI Takahiro
  0 siblings, 1 reply; 30+ messages in thread
From: Heinrich Schuchardt @ 2018-12-20  7:51 UTC (permalink / raw)
  To: u-boot

On 12/18/18 6:05 AM, AKASHI Takahiro wrote:
> "drivers" command prints all the uefi drivers on the system.
> => efi drivers
> Driver Name     Image Path
> ============================================
> (unknown)       <NULL>+(not found)
>     guid: 18a031ab-b443-4d1a-a5c0-0c09261e9f71
> 
> Currently, no useful information can be printed.

Why? We have lib/efi_driver/efi_block_device.c. So you should be able to
test the output.

Best regards

Heirnich

> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/efishell.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/efishell.c b/cmd/efishell.c
> index 929b6343b1b2..ed8de9e0355d 100644
> --- a/cmd/efishell.c
> +++ b/cmd/efishell.c
> @@ -343,6 +343,95 @@ 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_driver_binding_protocol *binding;
> +	struct efi_loaded_image *image;
> +	efi_status_t ret;
> +
> +	ret = bs->open_protocol(handle, &efi_guid_driver_binding_protocol,
> +				(void **)&binding, NULL, NULL,
> +				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +	if (ret != EFI_SUCCESS)
> +		return -1;
> +
> +	ret = bs->open_protocol(binding->image_handle, &efi_guid_loaded_image,
> +				(void **)&image, NULL /* FIXME */, NULL,
> +				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +	if (ret != EFI_SUCCESS)
> +		goto e_out;
> +
> +	/*
> +	 * TODO:
> +	 * handle image->device_handle,
> +	 * use append_device_path()
> +	 */
> +	*devname = NULL;
> +	*filename = efi_dp_str(image->file_path);
> +
> +	return 0;
> +
> +e_out:
> +	*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("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)) {
> +			printf("%-16s%ls+%ls\n",
> +			       "(unknown)", devname, filename);
> +			efi_free_pool(devname);
> +			efi_free_pool(filename);
> +
> +			/* TODO: no other info */
> +			struct efi_object *efiobj;
> +			struct list_head *lhandle;
> +			struct efi_handler *protocol;
> +
> +			efiobj = efi_search_obj(*handle);
> +			list_for_each(lhandle, &efiobj->protocols) {
> +				protocol = list_entry(lhandle,
> +						      struct efi_handler, link);
> +				printf("    guid: %pUl\n", protocol->guid);
> +			}
> +		}
> +		handle++;
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  static int do_efi_boot_add(int argc, char * const argv[])
>  {
>  	int id;
> @@ -726,6 +815,8 @@ static int do_efishell(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;
>  }
> @@ -749,7 +840,9 @@ static char efishell_help_text[] =
>  	"  - set/delete uefi variable's value\n"
>  	"    <value> may be \"=\"...\"\", \"=0x...\" (set) or \"=\" (delete)\n"
>  	"efishell devices\n"
> -	"  - show uefi devices\n";
> +	"  - show uefi devices\n"
> +	"efishell drivers\n"
> +	"  - show uefi drivers\n";
>  #endif
>  
>  U_BOOT_CMD(
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables
  2018-12-19 12:23       ` Heinrich Schuchardt
@ 2018-12-23  1:56         ` Alexander Graf
  2018-12-25  8:44           ` AKASHI Takahiro
  0 siblings, 1 reply; 30+ messages in thread
From: Alexander Graf @ 2018-12-23  1:56 UTC (permalink / raw)
  To: u-boot



On 19.12.18 13:23, Heinrich Schuchardt wrote:
> On 12/19/18 2:49 AM, AKASHI Takahiro wrote:
>> Heinrich,
>>
>> On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote:
>>> On 12/18/18 6:05 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>
>>>
>>> Hello Takahiro,
>>>
>>> in several patch series you are implementing multiple interactive
>>> commands that concern
>>>
>>> - handling of EFI variables
>>> - executing EFI binaries
>>> - managing boot sequence
>>>
>>> I very much appreciate your effort to provide an independent UEFI shell
>>> implementation. What I am worried about is that your current patches
>>> make it part of the monolithic U-Boot binary.
>>
>> First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's
>> comment on v2. So you can disable efishell command if you don't want it.
>>
>> Are you still worried?
>>
>>> This design has multiple drawbacks:
>>>
>>> The memory size available for U-Boot is very limited for many devices.
>>> We already had to disable EFI_LOADER for some boards due to this
>>> limitations. Hence we want to keep everything out of the U-Boot binary
>>> that does not serve the primary goal of loading and executing the next
>>> binary.
>>
>> I don't know your point here. If EFI_LOADER is disabled, efishell
>> will never be compiled in.
>>
>>> The UEFI forum has published a UEFI Shell specification which is very
>>> extensive. We still have a lot of deficiencies in U-Boot's UEFI API
>>> implementation. By merging in parts of an UEFI shell implementation our
>>> project looses focus.
>>
>> What is "our project?" What is "focus?"
>> I'm just asking as I want to share that information with you.
>>
>>> There is an EDK2 implementation of said
>>> specification. If we fix the remaining bugs of the EFI API
>>> implementation in U-Boot we could simply run the EDK2 shell which
>>> provides all that is needed for interactive work.
>>>
>>> With you monolithic approach your UEFI shell implementation can neither
>>> be used with other UEFI API implementations than U-Boot nor can it be
>>> tested against other API implementations.
>>
>> Let me explain my stance.
>> My efishell is basically something like a pursuit as well as
>> a debug/test tool which was and is still quite useful for me.
>> Without it, I would have completed (most of) my efi-related work so far.
>> So I believe that it will also be useful for other people who may want
>> to get involved and play with u-boot's efi environment.
> 
> On SD-Cards U-Boot is installed between the MBR and the first partition.
> On other devices it is put into a very small ROM. Both ways the maximum
> size is rather limited.
> 
> U-Boot provides all that is needed to load and execute an EFI binary. So
> you can put your efishell as file into the EFI partition like you would
> install the EDK2 shell.
> 
> The only hardshift this approach brings is that you have to implement
> your own printf because UEFI does not offer formatted output. But this
> can be copied from lib/efi_selftest/efi_selftest_console.c.
> 
> The same decision I took for booting from iSCSI. I did not try to put an
> iSCSI driver into U-Boot instead I use iPXE as an executable that is
> loaded from the EFI partition.
> 
>>
>> I have never intended to fully implement a shell which is to be compliant
>> with UEFI specification while I'm trying to mimick some command
>> interfaces for convenience. UEFI shell, as you know, provides plenty
>> of "protocols" on which some UEFI applications, including UEFI SCT,
>> reply. I will never implement it with my efishell.
>>
>> I hope that my efishell is a quick and easy way of learning more about
>> u-boot's uefi environment. I will be even happier if more people
>> get involved there.
>>
>>> Due to these considerations I suggest that you build your UEFI shell
>>> implementation as a separate UEFI binary (like helloworld.efi). You may
>>> offer an embedding of the binary (like the bootefi hello command) into
>>> the finally linked U-Boot binary via a configuration variable. Please,
>>> put the shell implementation into a separate directory. You may want to
>>> designate yourself as maintainer (in file MAINTAINERS).
>>
>> Yeah, your suggestion is reasonable and I have thought of it before.
>> There are, however, several reasons that I haven't done so; particularly,
>> efishell is implemented not only with boottime services but also
>> other helper functions, say, from device path utilities. Exporting them
>> as libraries is possible but I don't think that it would be so valuable.
>>
>> Even if efishell is a separate application, it will not contribute to
>> reduce the total footprint if it is embedded along with u-boot binary.
> 
> That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into
> the U-Boot binary - is default no. Same I would do for efishell.efi.

One big drawback with a separate binary is the missing command line
integration. It becomes quite awkward to execute efi debug commands
then, since you'll have to run them through a special bootefi subcommand.

If you really want to have a "uefi shell", I think the sanest option is
to just provide a built-in copy of the edk2 uefi shell, similar to the
hello world binary. The big benefit of this patch set however, is not
that we get a shell - it's that we get quick and tiny debug
introspectability into efi_loader data structures.

I think the biggest problem here really is the name of the code. Why
don't we just call it "debugefi"? It would be default N except for debug
targets (just like bootefi_hello).

That way when someone wants to just quickly introspect internal data
structures, they can. I also hope that if the name contains debug,
nobody will expect command line compatibility going forward, so we have
much more freedom to change internals (which is my biggest concern).

So in my opinion, if you fix the 2 other comments from Heinrich and
rename everything from "efishell" to "debugefi" (so it aligns with
bootefi), we should be good.


Alex

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 6/8] cmd: efishell: add dh command
  2018-12-20  7:49   ` Heinrich Schuchardt
@ 2018-12-25  5:32     ` AKASHI Takahiro
  0 siblings, 0 replies; 30+ messages in thread
From: AKASHI Takahiro @ 2018-12-25  5:32 UTC (permalink / raw)
  To: u-boot

On Thu, Dec 20, 2018 at 08:49:25AM +0100, Heinrich Schuchardt wrote:
> On 12/18/18 6:05 AM, AKASHI Takahiro wrote:
> > "dh" command prints all the uefi handles used in the system.
> > => efishell dh
> > (T.B.D.)
> > 0: (protocol info not available)
> > 1: (protocol info not available)
> > 2: (protocol info not available)
> > 3: (protocol info not available)
> > 4: (protocol info not available)
> > 5: (protocol info not available)
> > 6: (protocol info not available)
> > 7: (protocol info not available)
> > 8: (protocol info not available)
> > 9: (protocol info not available)
> > 10: (protocol info not available)
> > 11: (protocol info not available)
> > 12: (protocol info not available)
> > 13: (protocol info not available)
> > 14: (protocol info not available)
> > 15: (protocol info not available)
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/efishell.c | 33 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/efishell.c b/cmd/efishell.c
> > index 5a81a627d616..47ad77606062 100644
> > --- a/cmd/efishell.c
> > +++ b/cmd/efishell.c
> > @@ -511,6 +511,33 @@ static int do_efi_show_memmap(int argc, char * const argv[])
> >  	return CMD_RET_SUCCESS;
> >  }
> >  
> > +static char *efi_get_proto_info(efi_handle_t handle)
> > +{
> > +	return strdup("(protocol info not available)");
> Shouldn't this enumerate all protocol GUIDs installed on the handles by
> calling ProtocolsPerHandle()?

Okay.

> Also instances of installed drivers identifiable by the driver binding
> protocol might be interesting.

Getting a driver name from driver binding protocol can be a bit hard.
See my reply to your comment on "efishell drivers."

Thanks,
-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> > +}
> > +
> > +static int do_efi_show_handles(int argc, char * const argv[])
> > +{
> > +	efi_handle_t *handles = NULL, *handle;
> > +	char *info;
> > +	int i;
> > +
> > +	handles = efi_get_handles_by_proto(NULL);
> > +	if (!handles)
> > +		return CMD_RET_SUCCESS;
> > +
> > +	for (handle = handles, i = 0; *handle; handle++, i++) {
> > +		/* TODO: depends on protocols */
> > +		info = efi_get_proto_info(*handle);
> > +		printf("%d: %s\n", i, info ?: "");
> > +		free(info);
> > +	}
> > +
> > +	free(handles);
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> >  static int do_efi_boot_add(int argc, char * const argv[])
> >  {
> >  	int id;
> > @@ -900,6 +927,8 @@ static int do_efishell(cmd_tbl_t *cmdtp, int flag,
> >  		return do_efi_show_images(argc, argv);
> >  	else if (!strcmp(command, "memmap"))
> >  		return do_efi_show_memmap(argc, argv);
> > +	else if (!strcmp(command, "dh"))
> > +		return do_efi_show_handles(argc, argv);
> >  	else
> >  		return CMD_RET_USAGE;
> >  }
> > @@ -929,7 +958,9 @@ static char efishell_help_text[] =
> >  	"efishell images\n"
> >  	"  - show loaded images\n"
> >  	"efishell memmap\n"
> > -	"  - show uefi memory map\n";
> > +	"  - show uefi memory map\n"
> > +	"efishell dh\n"
> > +	"  - show uefi handles\n";
> >  #endif
> >  
> >  U_BOOT_CMD(
> > 
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 3/8] cmd: efishell: add drivers command
  2018-12-20  7:51   ` Heinrich Schuchardt
@ 2018-12-25  7:22     ` AKASHI Takahiro
  2018-12-25 12:07       ` Heinrich Schuchardt
  0 siblings, 1 reply; 30+ messages in thread
From: AKASHI Takahiro @ 2018-12-25  7:22 UTC (permalink / raw)
  To: u-boot

On Thu, Dec 20, 2018 at 08:51:34AM +0100, Heinrich Schuchardt wrote:
> On 12/18/18 6:05 AM, AKASHI Takahiro wrote:
> > "drivers" command prints all the uefi drivers on the system.
> > => efi drivers
> > Driver Name     Image Path
> > ============================================
> > (unknown)       <NULL>+(not found)
> >     guid: 18a031ab-b443-4d1a-a5c0-0c09261e9f71
> > 
> > Currently, no useful information can be printed.
> 
> Why? We have lib/efi_driver/efi_block_device.c. So you should be able to
> test the output.

There are a couple of reasons that I think there are no useful
information:
1. EFI driver doesn't have any printable name.
   A corresponding u-boot driver, efi_block in efi_block_device.c,
   has a name, but there is not direct link between efi driver
   and this u-boot driver. So it's not easy to find a name for efi driver.
2. I will fix this by adding a "name" field to
   efi_driver_binding_extended_protocol, but if even so, we can only
   say "EFI block driver." We have no chance to know about controller
   -specific, say USB, SCSI or anything, information.

3. More importantly, efi block devices can be defined in two ways,
   via efi_uclass/efi_block_device and efi_disk. The latter supports
   BLOCK_IO_PROTOCOL binding having any associated driver/controller.
   It seems to me that it's quite confusion.

Thanks,
-Takahiro Akashi



> Best regards
> 
> Heirnich
> 
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/efishell.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 94 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/efishell.c b/cmd/efishell.c
> > index 929b6343b1b2..ed8de9e0355d 100644
> > --- a/cmd/efishell.c
> > +++ b/cmd/efishell.c
> > @@ -343,6 +343,95 @@ 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_driver_binding_protocol *binding;
> > +	struct efi_loaded_image *image;
> > +	efi_status_t ret;
> > +
> > +	ret = bs->open_protocol(handle, &efi_guid_driver_binding_protocol,
> > +				(void **)&binding, NULL, NULL,
> > +				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > +	if (ret != EFI_SUCCESS)
> > +		return -1;
> > +
> > +	ret = bs->open_protocol(binding->image_handle, &efi_guid_loaded_image,
> > +				(void **)&image, NULL /* FIXME */, NULL,
> > +				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > +	if (ret != EFI_SUCCESS)
> > +		goto e_out;
> > +
> > +	/*
> > +	 * TODO:
> > +	 * handle image->device_handle,
> > +	 * use append_device_path()
> > +	 */
> > +	*devname = NULL;
> > +	*filename = efi_dp_str(image->file_path);
> > +
> > +	return 0;
> > +
> > +e_out:
> > +	*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("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)) {
> > +			printf("%-16s%ls+%ls\n",
> > +			       "(unknown)", devname, filename);
> > +			efi_free_pool(devname);
> > +			efi_free_pool(filename);
> > +
> > +			/* TODO: no other info */
> > +			struct efi_object *efiobj;
> > +			struct list_head *lhandle;
> > +			struct efi_handler *protocol;
> > +
> > +			efiobj = efi_search_obj(*handle);
> > +			list_for_each(lhandle, &efiobj->protocols) {
> > +				protocol = list_entry(lhandle,
> > +						      struct efi_handler, link);
> > +				printf("    guid: %pUl\n", protocol->guid);
> > +			}
> > +		}
> > +		handle++;
> > +	}
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> >  static int do_efi_boot_add(int argc, char * const argv[])
> >  {
> >  	int id;
> > @@ -726,6 +815,8 @@ static int do_efishell(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;
> >  }
> > @@ -749,7 +840,9 @@ static char efishell_help_text[] =
> >  	"  - set/delete uefi variable's value\n"
> >  	"    <value> may be \"=\"...\"\", \"=0x...\" (set) or \"=\" (delete)\n"
> >  	"efishell devices\n"
> > -	"  - show uefi devices\n";
> > +	"  - show uefi devices\n"
> > +	"efishell drivers\n"
> > +	"  - show uefi drivers\n";
> >  #endif
> >  
> >  U_BOOT_CMD(
> > 
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables
  2018-12-23  1:56         ` Alexander Graf
@ 2018-12-25  8:44           ` AKASHI Takahiro
  2018-12-26 21:20             ` Alexander Graf
  0 siblings, 1 reply; 30+ messages in thread
From: AKASHI Takahiro @ 2018-12-25  8:44 UTC (permalink / raw)
  To: u-boot

On Sun, Dec 23, 2018 at 02:56:40AM +0100, Alexander Graf wrote:
> 
> 
> On 19.12.18 13:23, Heinrich Schuchardt wrote:
> > On 12/19/18 2:49 AM, AKASHI Takahiro wrote:
> >> Heinrich,
> >>
> >> On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote:
> >>> On 12/18/18 6:05 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>
> >>>
> >>> Hello Takahiro,
> >>>
> >>> in several patch series you are implementing multiple interactive
> >>> commands that concern
> >>>
> >>> - handling of EFI variables
> >>> - executing EFI binaries
> >>> - managing boot sequence
> >>>
> >>> I very much appreciate your effort to provide an independent UEFI shell
> >>> implementation. What I am worried about is that your current patches
> >>> make it part of the monolithic U-Boot binary.
> >>
> >> First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's
> >> comment on v2. So you can disable efishell command if you don't want it.
> >>
> >> Are you still worried?
> >>
> >>> This design has multiple drawbacks:
> >>>
> >>> The memory size available for U-Boot is very limited for many devices.
> >>> We already had to disable EFI_LOADER for some boards due to this
> >>> limitations. Hence we want to keep everything out of the U-Boot binary
> >>> that does not serve the primary goal of loading and executing the next
> >>> binary.
> >>
> >> I don't know your point here. If EFI_LOADER is disabled, efishell
> >> will never be compiled in.
> >>
> >>> The UEFI forum has published a UEFI Shell specification which is very
> >>> extensive. We still have a lot of deficiencies in U-Boot's UEFI API
> >>> implementation. By merging in parts of an UEFI shell implementation our
> >>> project looses focus.
> >>
> >> What is "our project?" What is "focus?"
> >> I'm just asking as I want to share that information with you.
> >>
> >>> There is an EDK2 implementation of said
> >>> specification. If we fix the remaining bugs of the EFI API
> >>> implementation in U-Boot we could simply run the EDK2 shell which
> >>> provides all that is needed for interactive work.
> >>>
> >>> With you monolithic approach your UEFI shell implementation can neither
> >>> be used with other UEFI API implementations than U-Boot nor can it be
> >>> tested against other API implementations.
> >>
> >> Let me explain my stance.
> >> My efishell is basically something like a pursuit as well as
> >> a debug/test tool which was and is still quite useful for me.
> >> Without it, I would have completed (most of) my efi-related work so far.
> >> So I believe that it will also be useful for other people who may want
> >> to get involved and play with u-boot's efi environment.
> > 
> > On SD-Cards U-Boot is installed between the MBR and the first partition.
> > On other devices it is put into a very small ROM. Both ways the maximum
> > size is rather limited.
> > 
> > U-Boot provides all that is needed to load and execute an EFI binary. So
> > you can put your efishell as file into the EFI partition like you would
> > install the EDK2 shell.
> > 
> > The only hardshift this approach brings is that you have to implement
> > your own printf because UEFI does not offer formatted output. But this
> > can be copied from lib/efi_selftest/efi_selftest_console.c.
> > 
> > The same decision I took for booting from iSCSI. I did not try to put an
> > iSCSI driver into U-Boot instead I use iPXE as an executable that is
> > loaded from the EFI partition.
> > 
> >>
> >> I have never intended to fully implement a shell which is to be compliant
> >> with UEFI specification while I'm trying to mimick some command
> >> interfaces for convenience. UEFI shell, as you know, provides plenty
> >> of "protocols" on which some UEFI applications, including UEFI SCT,
> >> reply. I will never implement it with my efishell.
> >>
> >> I hope that my efishell is a quick and easy way of learning more about
> >> u-boot's uefi environment. I will be even happier if more people
> >> get involved there.
> >>
> >>> Due to these considerations I suggest that you build your UEFI shell
> >>> implementation as a separate UEFI binary (like helloworld.efi). You may
> >>> offer an embedding of the binary (like the bootefi hello command) into
> >>> the finally linked U-Boot binary via a configuration variable. Please,
> >>> put the shell implementation into a separate directory. You may want to
> >>> designate yourself as maintainer (in file MAINTAINERS).
> >>
> >> Yeah, your suggestion is reasonable and I have thought of it before.
> >> There are, however, several reasons that I haven't done so; particularly,
> >> efishell is implemented not only with boottime services but also
> >> other helper functions, say, from device path utilities. Exporting them
> >> as libraries is possible but I don't think that it would be so valuable.
> >>
> >> Even if efishell is a separate application, it will not contribute to
> >> reduce the total footprint if it is embedded along with u-boot binary.
> > 
> > That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into
> > the U-Boot binary - is default no. Same I would do for efishell.efi.
> 
> One big drawback with a separate binary is the missing command line
> integration. It becomes quite awkward to execute efi debug commands
> then, since you'll have to run them through a special bootefi subcommand.
> 
> If you really want to have a "uefi shell", I think the sanest option is
> to just provide a built-in copy of the edk2 uefi shell, similar to the
> hello world binary. The big benefit of this patch set however, is not
> that we get a shell - it's that we get quick and tiny debug
> introspectability into efi_loader data structures.

And my command can be used for simple testing.

> I think the biggest problem here really is the name of the code. Why
> don't we just call it "debugefi"? It would be default N except for debug
> targets (just like bootefi_hello).
> 
> That way when someone wants to just quickly introspect internal data
> structures, they can. I also hope that if the name contains debug,
> nobody will expect command line compatibility going forward, so we have
> much more freedom to change internals (which is my biggest concern).
> 
> So in my opinion, if you fix the 2 other comments from Heinrich and
> rename everything from "efishell" to "debugefi" (so it aligns with
> bootefi), we should be good.

If Heinrich agrees, I will fix the name although I'm not a super fan
of this new name :)

Thanks,
-Takahiro Akashi

> 
> Alex

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 3/8] cmd: efishell: add drivers command
  2018-12-25  7:22     ` AKASHI Takahiro
@ 2018-12-25 12:07       ` Heinrich Schuchardt
  0 siblings, 0 replies; 30+ messages in thread
From: Heinrich Schuchardt @ 2018-12-25 12:07 UTC (permalink / raw)
  To: u-boot

On 12/25/18 8:22 AM, AKASHI Takahiro wrote:
> On Thu, Dec 20, 2018 at 08:51:34AM +0100, Heinrich Schuchardt wrote:
>> On 12/18/18 6:05 AM, AKASHI Takahiro wrote:
>>> "drivers" command prints all the uefi drivers on the system.
>>> => efi drivers
>>> Driver Name     Image Path
>>> ============================================
>>> (unknown)       <NULL>+(not found)
>>>     guid: 18a031ab-b443-4d1a-a5c0-0c09261e9f71
>>>
>>> Currently, no useful information can be printed.
>>
>> Why? We have lib/efi_driver/efi_block_device.c. So you should be able to
>> test the output.
> 
> There are a couple of reasons that I think there are no useful
> information:
> 1. EFI driver doesn't have any printable name.
>    A corresponding u-boot driver, efi_block in efi_block_device.c,
>    has a name, but there is not direct link between efi driver
>    and this u-boot driver. So it's not easy to find a name for efi driver.
> 2. I will fix this by adding a "name" field to
>    efi_driver_binding_extended_protocol, but if even so, we can only
>    say "EFI block driver." We have no chance to know about controller
>    -specific, say USB, SCSI or anything, information.

I would just stick to the standard. The following information can be
provided in the standard:

Loop over all handles implementing the driver binding protocol and output:

- address of driver handle
- device path (if installed on the handle)

> 
> 3. More importantly, efi block devices can be defined in two ways,
>    via efi_uclass/efi_block_device and efi_disk. The latter supports
>    BLOCK_IO_PROTOCOL binding having any associated driver/controller.
>    It seems to me that it's quite confusion.

lib/efi_driver/efi_block_device.c is used when an EFI binary creates a
new block device. E.g. when iPXE connects to an iSCSI drive it creates a
handle with the the block io protocol and call ConnectControllers. Now
our efi_block_device binds to the handle, identifies the partitions, and
provides the simple file system protocol.

Unfortunately the migration of all block devices to the driver model has
been delayed to July 2019 (cf. doc/driver-model/MIGRATION.txt). Once it
is completed we can clean up the code.

Best regards

Heinrich

> 
> Thanks,
> -Takahiro Akashi
> 
> 
> 
>> Best regards
>>
>> Heirnich
>>
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  cmd/efishell.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 94 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/efishell.c b/cmd/efishell.c
>>> index 929b6343b1b2..ed8de9e0355d 100644
>>> --- a/cmd/efishell.c
>>> +++ b/cmd/efishell.c
>>> @@ -343,6 +343,95 @@ 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_driver_binding_protocol *binding;
>>> +	struct efi_loaded_image *image;
>>> +	efi_status_t ret;
>>> +
>>> +	ret = bs->open_protocol(handle, &efi_guid_driver_binding_protocol,
>>> +				(void **)&binding, NULL, NULL,
>>> +				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>> +	if (ret != EFI_SUCCESS)
>>> +		return -1;
>>> +
>>> +	ret = bs->open_protocol(binding->image_handle, &efi_guid_loaded_image,
>>> +				(void **)&image, NULL /* FIXME */, NULL,
>>> +				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>> +	if (ret != EFI_SUCCESS)
>>> +		goto e_out;
>>> +
>>> +	/*
>>> +	 * TODO:
>>> +	 * handle image->device_handle,
>>> +	 * use append_device_path()
>>> +	 */
>>> +	*devname = NULL;
>>> +	*filename = efi_dp_str(image->file_path);
>>> +
>>> +	return 0;
>>> +
>>> +e_out:
>>> +	*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("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)) {
>>> +			printf("%-16s%ls+%ls\n",
>>> +			       "(unknown)", devname, filename);
>>> +			efi_free_pool(devname);
>>> +			efi_free_pool(filename);
>>> +
>>> +			/* TODO: no other info */
>>> +			struct efi_object *efiobj;
>>> +			struct list_head *lhandle;
>>> +			struct efi_handler *protocol;
>>> +
>>> +			efiobj = efi_search_obj(*handle);
>>> +			list_for_each(lhandle, &efiobj->protocols) {
>>> +				protocol = list_entry(lhandle,
>>> +						      struct efi_handler, link);
>>> +				printf("    guid: %pUl\n", protocol->guid);
>>> +			}
>>> +		}
>>> +		handle++;
>>> +	}
>>> +
>>> +	return CMD_RET_SUCCESS;
>>> +}
>>> +
>>>  static int do_efi_boot_add(int argc, char * const argv[])
>>>  {
>>>  	int id;
>>> @@ -726,6 +815,8 @@ static int do_efishell(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;
>>>  }
>>> @@ -749,7 +840,9 @@ static char efishell_help_text[] =
>>>  	"  - set/delete uefi variable's value\n"
>>>  	"    <value> may be \"=\"...\"\", \"=0x...\" (set) or \"=\" (delete)\n"
>>>  	"efishell devices\n"
>>> -	"  - show uefi devices\n";
>>> +	"  - show uefi devices\n"
>>> +	"efishell drivers\n"
>>> +	"  - show uefi drivers\n";
>>>  #endif
>>>  
>>>  U_BOOT_CMD(
>>>
>>
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables
  2018-12-25  8:44           ` AKASHI Takahiro
@ 2018-12-26 21:20             ` Alexander Graf
  2019-01-07  7:47               ` AKASHI Takahiro
  0 siblings, 1 reply; 30+ messages in thread
From: Alexander Graf @ 2018-12-26 21:20 UTC (permalink / raw)
  To: u-boot



On 25.12.18 09:44, AKASHI Takahiro wrote:
> On Sun, Dec 23, 2018 at 02:56:40AM +0100, Alexander Graf wrote:
>>
>>
>> On 19.12.18 13:23, Heinrich Schuchardt wrote:
>>> On 12/19/18 2:49 AM, AKASHI Takahiro wrote:
>>>> Heinrich,
>>>>
>>>> On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote:
>>>>> On 12/18/18 6:05 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>
>>>>>
>>>>> Hello Takahiro,
>>>>>
>>>>> in several patch series you are implementing multiple interactive
>>>>> commands that concern
>>>>>
>>>>> - handling of EFI variables
>>>>> - executing EFI binaries
>>>>> - managing boot sequence
>>>>>
>>>>> I very much appreciate your effort to provide an independent UEFI shell
>>>>> implementation. What I am worried about is that your current patches
>>>>> make it part of the monolithic U-Boot binary.
>>>>
>>>> First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's
>>>> comment on v2. So you can disable efishell command if you don't want it.
>>>>
>>>> Are you still worried?
>>>>
>>>>> This design has multiple drawbacks:
>>>>>
>>>>> The memory size available for U-Boot is very limited for many devices.
>>>>> We already had to disable EFI_LOADER for some boards due to this
>>>>> limitations. Hence we want to keep everything out of the U-Boot binary
>>>>> that does not serve the primary goal of loading and executing the next
>>>>> binary.
>>>>
>>>> I don't know your point here. If EFI_LOADER is disabled, efishell
>>>> will never be compiled in.
>>>>
>>>>> The UEFI forum has published a UEFI Shell specification which is very
>>>>> extensive. We still have a lot of deficiencies in U-Boot's UEFI API
>>>>> implementation. By merging in parts of an UEFI shell implementation our
>>>>> project looses focus.
>>>>
>>>> What is "our project?" What is "focus?"
>>>> I'm just asking as I want to share that information with you.
>>>>
>>>>> There is an EDK2 implementation of said
>>>>> specification. If we fix the remaining bugs of the EFI API
>>>>> implementation in U-Boot we could simply run the EDK2 shell which
>>>>> provides all that is needed for interactive work.
>>>>>
>>>>> With you monolithic approach your UEFI shell implementation can neither
>>>>> be used with other UEFI API implementations than U-Boot nor can it be
>>>>> tested against other API implementations.
>>>>
>>>> Let me explain my stance.
>>>> My efishell is basically something like a pursuit as well as
>>>> a debug/test tool which was and is still quite useful for me.
>>>> Without it, I would have completed (most of) my efi-related work so far.
>>>> So I believe that it will also be useful for other people who may want
>>>> to get involved and play with u-boot's efi environment.
>>>
>>> On SD-Cards U-Boot is installed between the MBR and the first partition.
>>> On other devices it is put into a very small ROM. Both ways the maximum
>>> size is rather limited.
>>>
>>> U-Boot provides all that is needed to load and execute an EFI binary. So
>>> you can put your efishell as file into the EFI partition like you would
>>> install the EDK2 shell.
>>>
>>> The only hardshift this approach brings is that you have to implement
>>> your own printf because UEFI does not offer formatted output. But this
>>> can be copied from lib/efi_selftest/efi_selftest_console.c.
>>>
>>> The same decision I took for booting from iSCSI. I did not try to put an
>>> iSCSI driver into U-Boot instead I use iPXE as an executable that is
>>> loaded from the EFI partition.
>>>
>>>>
>>>> I have never intended to fully implement a shell which is to be compliant
>>>> with UEFI specification while I'm trying to mimick some command
>>>> interfaces for convenience. UEFI shell, as you know, provides plenty
>>>> of "protocols" on which some UEFI applications, including UEFI SCT,
>>>> reply. I will never implement it with my efishell.
>>>>
>>>> I hope that my efishell is a quick and easy way of learning more about
>>>> u-boot's uefi environment. I will be even happier if more people
>>>> get involved there.
>>>>
>>>>> Due to these considerations I suggest that you build your UEFI shell
>>>>> implementation as a separate UEFI binary (like helloworld.efi). You may
>>>>> offer an embedding of the binary (like the bootefi hello command) into
>>>>> the finally linked U-Boot binary via a configuration variable. Please,
>>>>> put the shell implementation into a separate directory. You may want to
>>>>> designate yourself as maintainer (in file MAINTAINERS).
>>>>
>>>> Yeah, your suggestion is reasonable and I have thought of it before.
>>>> There are, however, several reasons that I haven't done so; particularly,
>>>> efishell is implemented not only with boottime services but also
>>>> other helper functions, say, from device path utilities. Exporting them
>>>> as libraries is possible but I don't think that it would be so valuable.
>>>>
>>>> Even if efishell is a separate application, it will not contribute to
>>>> reduce the total footprint if it is embedded along with u-boot binary.
>>>
>>> That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into
>>> the U-Boot binary - is default no. Same I would do for efishell.efi.
>>
>> One big drawback with a separate binary is the missing command line
>> integration. It becomes quite awkward to execute efi debug commands
>> then, since you'll have to run them through a special bootefi subcommand.
>>
>> If you really want to have a "uefi shell", I think the sanest option is
>> to just provide a built-in copy of the edk2 uefi shell, similar to the
>> hello world binary. The big benefit of this patch set however, is not
>> that we get a shell - it's that we get quick and tiny debug
>> introspectability into efi_loader data structures.
> 
> And my command can be used for simple testing.

Exactly, that would give us the best of both worlds.

> 
>> I think the biggest problem here really is the name of the code. Why
>> don't we just call it "debugefi"? It would be default N except for debug
>> targets (just like bootefi_hello).
>>
>> That way when someone wants to just quickly introspect internal data
>> structures, they can. I also hope that if the name contains debug,
>> nobody will expect command line compatibility going forward, so we have
>> much more freedom to change internals (which is my biggest concern).
>>
>> So in my opinion, if you fix the 2 other comments from Heinrich and
>> rename everything from "efishell" to "debugefi" (so it aligns with
>> bootefi), we should be good.
> 
> If Heinrich agrees, I will fix the name although I'm not a super fan
> of this new name :)

Well, feel free to come up with a new one, but it definitely must have a
ring to it that it's a tiny, debug only feature that is not intended for
normal use ;).

For normal operation, we need to come up with mechanisms that integrate
much deeper into U-Boot's generic command structure.


Alex

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 1/8] cmd: add efishell command
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 1/8] cmd: add efishell command AKASHI Takahiro
@ 2018-12-30 15:44   ` Heinrich Schuchardt
  2018-12-30 17:10     ` Heinrich Schuchardt
  2019-01-07  5:06     ` AKASHI Takahiro
  2018-12-30 23:47   ` Heinrich Schuchardt
  1 sibling, 2 replies; 30+ messages in thread
From: Heinrich Schuchardt @ 2018-12-30 15:44 UTC (permalink / raw)
  To: u-boot

On 12/18/18 6:05 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, efishell, helps address these issues and give us
> more friendly interfaces:
>  * efishell boot add: add BootXXXX variable
>  * efishell boot rm: remove BootXXXX variable
>  * efishell boot dump: display all BootXXXX variables
>  * efishell boot order: set/display a boot order (BootOrder)
>  * efishell setvar: set an UEFI variable (with limited functionality)
>  * efishell 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/efishell.c | 673 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 684 insertions(+)
>  create mode 100644 cmd/efishell.c
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index f2f3b5e2b76b..a8a4bf7db45e 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1390,6 +1390,16 @@ config CMD_DISPLAY
>  	  displayed on a simple board-specific display. Implement
>  	  display_putc() to use it.
>  
> +config CMD_EFISHELL
> +	bool "Enable the 'efishell' command for EFI environment"
> +	depends on EFI_LOADER
> +	default n
> +	help
> +	  Enable the 'efishell' 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 5ec2f9e8ebfd..0258d8a373b1 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -49,6 +49,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_EFISHELL) += efishell.o
>  obj-$(CONFIG_CMD_ELF) += elf.o
>  obj-$(CONFIG_HUSH_PARSER) += exit.o
>  obj-$(CONFIG_CMD_EXT4) += ext4.o
> diff --git a/cmd/efishell.c b/cmd/efishell.c
> new file mode 100644
> index 000000000000..5819e52cf575
> --- /dev/null
> +++ b/cmd/efishell.c
> @@ -0,0 +1,673 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  EFI 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] = 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 EFI 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;
> +	unsigned long size = 0;
> +	u16 *var_name16, *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:
> +	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;
> +	unsigned long size;
> +	int ret;
> +
> +	if (argc < 6 || argc > 7)
> +		return CMD_RET_USAGE;
> +
> +	id = (int)simple_strtoul(argv[1], &endp, 0);
> +	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);

The load options should be converted to a UTF-16 string before calling
efi_serialize_load_option so that we can copy them to the loaded image
protocol.

> +	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, 0);
> +		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,
> +				       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +				       EFI_VARIABLE_RUNTIME_ACCESS, 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;
> +	unsigned long size;

'size' must be of type efi_uint_t. Otherwise you get warnings like this one:

cmd/efishell.c:729:50: warning: passing argument 4 of ‘efi_get_variable’
from incompatible pointer type [-Wincompatible-pointer-types]
  ret = efi_get_variable(var_name16, &guid, NULL, &size, NULL);


> +	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;
> +	unsigned long 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_order(int argc, char * const argv[])
> +{
> +	u16 *bootorder = NULL;
> +	unsigned long 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, 0);
> +		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, "order"))
> +		return do_efi_boot_order(argc, argv);
> +	else
> +		return CMD_RET_USAGE;
> +}
> +
> +/* Interpreter command to configure EFI environment */
> +static int do_efishell(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 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;
> +	}
> +
> +	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 efishell_help_text[] =
> +	"  - EFI Shell-like interface to configure EFI environment\n"
> +	"\n"
> +	"efishell boot add <bootid> <label> <interface> <device>[:<part>] <file path> <option>\n"

The 'option' parameter is optional. Please, add brackets ([]).

The parameter 'option' is misleading. Please call it 'load_options'.

Please, add a text explaining that 'load_options' is meant to be the
command line string passed to the EFI binary.

Best regards

Heinrich

> +	"  - set uefi's BOOT variable\n"
> +	"efishell boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
> +	"  - set/display uefi boot order\n"
> +	"efishell boot dump\n"
> +	"  - display all uefi's BOOT variables\n"
> +	"efishell boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
> +	"  - set/display uefi boot order\n"
> +	"\n"
> +	"efishell dumpvar [<name>]\n"
> +	"  - get uefi variable's value\n"
> +	"efishell setvar <name> [<value>]\n"
> +	"  - set/delete uefi variable's value\n"
> +	"    <value> may be \"=\"...\"\", \"=0x...\" (set) or \"=\" (delete)\n";
> +#endif
> +
> +U_BOOT_CMD(
> +	efishell, 10, 0, do_efishell,
> +	"Configure EFI environment",
> +	efishell_help_text
> +);
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 1/8] cmd: add efishell command
  2018-12-30 15:44   ` Heinrich Schuchardt
@ 2018-12-30 17:10     ` Heinrich Schuchardt
  2019-01-07  5:08       ` AKASHI Takahiro
  2019-01-07  5:06     ` AKASHI Takahiro
  1 sibling, 1 reply; 30+ messages in thread
From: Heinrich Schuchardt @ 2018-12-30 17:10 UTC (permalink / raw)
  To: u-boot

On 12/30/18 4:44 PM, Heinrich Schuchardt wrote:
> On 12/18/18 6:05 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, efishell, helps address these issues and give us
>> more friendly interfaces:
>>  * efishell boot add: add BootXXXX variable
>>  * efishell boot rm: remove BootXXXX variable
>>  * efishell boot dump: display all BootXXXX variables
>>  * efishell boot order: set/display a boot order (BootOrder)
>>  * efishell setvar: set an UEFI variable (with limited functionality)
>>  * efishell 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>

The behavior is a bit unexpected:

=> efishell boot order 200
=> efishell boot order
 1: Boot00C8: (not defined)
exit not allowed from main input shell.

I would expect 'efishell boot order' to take a 4 digit hexadecimal
number and to do no conversion from decimal to hexadecimal.

I was also surprised to see 'exit not allowed from main input shell.'

Best regards

Heinrich

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 1/8] cmd: add efishell command
  2018-12-18  5:05 ` [U-Boot] [PATCH v3 1/8] cmd: add efishell command AKASHI Takahiro
  2018-12-30 15:44   ` Heinrich Schuchardt
@ 2018-12-30 23:47   ` Heinrich Schuchardt
  2019-01-07  5:13     ` AKASHI Takahiro
  1 sibling, 1 reply; 30+ messages in thread
From: Heinrich Schuchardt @ 2018-12-30 23:47 UTC (permalink / raw)
  To: u-boot

On 12/18/18 6:05 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, efishell, helps address these issues and give us
> more friendly interfaces:
>  * efishell boot add: add BootXXXX variable
>  * efishell boot rm: remove BootXXXX variable
>  * efishell boot dump: display all BootXXXX variables
>  * efishell boot order: set/display a boot order (BootOrder)
>  * efishell setvar: set an UEFI variable (with limited functionality)
>  * efishell 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/efishell.c | 673 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 684 insertions(+)
>  create mode 100644 cmd/efishell.c
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index f2f3b5e2b76b..a8a4bf7db45e 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1390,6 +1390,16 @@ config CMD_DISPLAY
>  	  displayed on a simple board-specific display. Implement
>  	  display_putc() to use it.
>  

<snip>

> +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;
> +	unsigned long size;
> +	int ret;
> +
> +	if (argc < 6 || argc > 7)
> +		return CMD_RET_USAGE;
> +
> +	id = (int)simple_strtoul(argv[1], &endp, 0);
> +	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);

This will create a full device path like

/VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8)/HD(1,MBR,0xd1535d21,0x1,0x7f)/\<file_path>

This is unlike what the Linux program efibootmgr would do. Efibootmgr
will create a shortened device path where the first node is the
partition, e.g.

HD(1,MBR,0xd1535d21,0x1,0x7f)/\<file_path>

The advantage of this shortened device path is that it only depends on
the disk content and not on the firmware. With a full device path
approach adding a node (e.g. the disk controller) in front of the
partition would invalidate the boot entry. Furthermore the operating
system will not be aware of the full device path.

EDK2 uses the following logic in the boot manager to expand the device
path (BmGetNextLoadOptionDevicePath()):

Check if the file path is a full device path.
Check if the device path matches a partition on any drive.
Check if the file path matches a file on any partition.

I think in U-Boot we will have to support shortened device paths to
collaborate with operating systems.

Best regards

Heinrich

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 1/8] cmd: add efishell command
  2018-12-30 15:44   ` Heinrich Schuchardt
  2018-12-30 17:10     ` Heinrich Schuchardt
@ 2019-01-07  5:06     ` AKASHI Takahiro
  1 sibling, 0 replies; 30+ messages in thread
From: AKASHI Takahiro @ 2019-01-07  5:06 UTC (permalink / raw)
  To: u-boot

On Sun, Dec 30, 2018 at 04:44:53PM +0100, Heinrich Schuchardt wrote:
> On 12/18/18 6:05 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, efishell, helps address these issues and give us
> > more friendly interfaces:
> >  * efishell boot add: add BootXXXX variable
> >  * efishell boot rm: remove BootXXXX variable
> >  * efishell boot dump: display all BootXXXX variables
> >  * efishell boot order: set/display a boot order (BootOrder)
> >  * efishell setvar: set an UEFI variable (with limited functionality)
> >  * efishell 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/efishell.c | 673 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 684 insertions(+)
> >  create mode 100644 cmd/efishell.c
> > 
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index f2f3b5e2b76b..a8a4bf7db45e 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -1390,6 +1390,16 @@ config CMD_DISPLAY
> >  	  displayed on a simple board-specific display. Implement
> >  	  display_putc() to use it.
> >  
> > +config CMD_EFISHELL
> > +	bool "Enable the 'efishell' command for EFI environment"
> > +	depends on EFI_LOADER
> > +	default n
> > +	help
> > +	  Enable the 'efishell' 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 5ec2f9e8ebfd..0258d8a373b1 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -49,6 +49,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_EFISHELL) += efishell.o
> >  obj-$(CONFIG_CMD_ELF) += elf.o
> >  obj-$(CONFIG_HUSH_PARSER) += exit.o
> >  obj-$(CONFIG_CMD_EXT4) += ext4.o
> > diff --git a/cmd/efishell.c b/cmd/efishell.c
> > new file mode 100644
> > index 000000000000..5819e52cf575
> > --- /dev/null
> > +++ b/cmd/efishell.c
> > @@ -0,0 +1,673 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  EFI 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] = 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 EFI 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;
> > +	unsigned long size = 0;
> > +	u16 *var_name16, *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:
> > +	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;
> > +	unsigned long size;
> > +	int ret;
> > +
> > +	if (argc < 6 || argc > 7)
> > +		return CMD_RET_USAGE;
> > +
> > +	id = (int)simple_strtoul(argv[1], &endp, 0);
> > +	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);
> 
> The load options should be converted to a UTF-16 string before calling
> efi_serialize_load_option so that we can copy them to the loaded image
> protocol.

I don't get your point.
What do you mean by "load options," lo or optional_data?
The latter is defined as a u8 string in efi_loader.h, which was
originally located in efi_bootmgr.c.

> > +	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, 0);
> > +		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,
> > +				       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +				       EFI_VARIABLE_RUNTIME_ACCESS, 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;
> > +	unsigned long size;
> 
> 'size' must be of type efi_uint_t. Otherwise you get warnings like this one:
> 
> cmd/efishell.c:729:50: warning: passing argument 4 of ‘efi_get_variable’
> from incompatible pointer type [-Wincompatible-pointer-types]
>   ret = efi_get_variable(var_name16, &guid, NULL, &size, NULL);

OK.

> 
> > +	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;
> > +	unsigned long 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_order(int argc, char * const argv[])
> > +{
> > +	u16 *bootorder = NULL;
> > +	unsigned long 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, 0);
> > +		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, "order"))
> > +		return do_efi_boot_order(argc, argv);
> > +	else
> > +		return CMD_RET_USAGE;
> > +}
> > +
> > +/* Interpreter command to configure EFI environment */
> > +static int do_efishell(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 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;
> > +	}
> > +
> > +	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 efishell_help_text[] =
> > +	"  - EFI Shell-like interface to configure EFI environment\n"
> > +	"\n"
> > +	"efishell boot add <bootid> <label> <interface> <device>[:<part>] <file path> <option>\n"
> 
> The 'option' parameter is optional. Please, add brackets ([]).

OK.

> The parameter 'option' is misleading. Please call it 'load_options'.

OK.

> Please, add a text explaining that 'load_options' is meant to be the
> command line string passed to the EFI binary.

OK.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +	"  - set uefi's BOOT variable\n"
> > +	"efishell boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
> > +	"  - set/display uefi boot order\n"
> > +	"efishell boot dump\n"
> > +	"  - display all uefi's BOOT variables\n"
> > +	"efishell boot order [<bootid#1> [<bootid#2> [<bootid#3> [...]]]]\n"
> > +	"  - set/display uefi boot order\n"
> > +	"\n"
> > +	"efishell dumpvar [<name>]\n"
> > +	"  - get uefi variable's value\n"
> > +	"efishell setvar <name> [<value>]\n"
> > +	"  - set/delete uefi variable's value\n"
> > +	"    <value> may be \"=\"...\"\", \"=0x...\" (set) or \"=\" (delete)\n";
> > +#endif
> > +
> > +U_BOOT_CMD(
> > +	efishell, 10, 0, do_efishell,
> > +	"Configure EFI environment",
> > +	efishell_help_text
> > +);
> > 
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 1/8] cmd: add efishell command
  2018-12-30 17:10     ` Heinrich Schuchardt
@ 2019-01-07  5:08       ` AKASHI Takahiro
  2019-01-08  9:57         ` Alexander Graf
  0 siblings, 1 reply; 30+ messages in thread
From: AKASHI Takahiro @ 2019-01-07  5:08 UTC (permalink / raw)
  To: u-boot

On Sun, Dec 30, 2018 at 06:10:51PM +0100, Heinrich Schuchardt wrote:
> On 12/30/18 4:44 PM, Heinrich Schuchardt wrote:
> > On 12/18/18 6:05 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, efishell, helps address these issues and give us
> >> more friendly interfaces:
> >>  * efishell boot add: add BootXXXX variable
> >>  * efishell boot rm: remove BootXXXX variable
> >>  * efishell boot dump: display all BootXXXX variables
> >>  * efishell boot order: set/display a boot order (BootOrder)
> >>  * efishell setvar: set an UEFI variable (with limited functionality)
> >>  * efishell 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>
> 
> The behavior is a bit unexpected:
> 
> => efishell boot order 200
> => efishell boot order
>  1: Boot00C8: (not defined)
> exit not allowed from main input shell.
> 
> I would expect 'efishell boot order' to take a 4 digit hexadecimal
> number and to do no conversion from decimal to hexadecimal.

OK, but we should allow a less-than-4-digit number. 

> I was also surprised to see 'exit not allowed from main input shell.'

I cannot reproduce this problem.

-Takahiro Akashi

> Best regards
> 
> Heinrich

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 1/8] cmd: add efishell command
  2018-12-30 23:47   ` Heinrich Schuchardt
@ 2019-01-07  5:13     ` AKASHI Takahiro
  0 siblings, 0 replies; 30+ messages in thread
From: AKASHI Takahiro @ 2019-01-07  5:13 UTC (permalink / raw)
  To: u-boot

On Mon, Dec 31, 2018 at 12:47:07AM +0100, Heinrich Schuchardt wrote:
> On 12/18/18 6:05 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, efishell, helps address these issues and give us
> > more friendly interfaces:
> >  * efishell boot add: add BootXXXX variable
> >  * efishell boot rm: remove BootXXXX variable
> >  * efishell boot dump: display all BootXXXX variables
> >  * efishell boot order: set/display a boot order (BootOrder)
> >  * efishell setvar: set an UEFI variable (with limited functionality)
> >  * efishell 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/efishell.c | 673 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 684 insertions(+)
> >  create mode 100644 cmd/efishell.c
> > 
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index f2f3b5e2b76b..a8a4bf7db45e 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -1390,6 +1390,16 @@ config CMD_DISPLAY
> >  	  displayed on a simple board-specific display. Implement
> >  	  display_putc() to use it.
> >  
> 
> <snip>
> 
> > +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;
> > +	unsigned long size;
> > +	int ret;
> > +
> > +	if (argc < 6 || argc > 7)
> > +		return CMD_RET_USAGE;
> > +
> > +	id = (int)simple_strtoul(argv[1], &endp, 0);
> > +	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);
> 
> This will create a full device path like
> 
> /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8)/HD(1,MBR,0xd1535d21,0x1,0x7f)/\<file_path>
> 
> This is unlike what the Linux program efibootmgr would do. Efibootmgr
> will create a shortened device path where the first node is the
> partition, e.g.
> 
> HD(1,MBR,0xd1535d21,0x1,0x7f)/\<file_path>
> 
> The advantage of this shortened device path is that it only depends on
> the disk content and not on the firmware. With a full device path
> approach adding a node (e.g. the disk controller) in front of the
> partition would invalidate the boot entry. Furthermore the operating
> system will not be aware of the full device path.
> 
> EDK2 uses the following logic in the boot manager to expand the device
> path (BmGetNextLoadOptionDevicePath()):
> 
> Check if the file path is a full device path.
> Check if the device path matches a partition on any drive.
> Check if the file path matches a file on any partition.
> 
> I think in U-Boot we will have to support shortened device paths to
> collaborate with operating systems.

So I assume that your comment here is not on my patch, but u-boot's
bootmgr. Once bootmgr is modified, I will be able to make my code
aligned.

-Takahiro Akashi

> Best regards
> 
> Heinrich

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables
  2018-12-26 21:20             ` Alexander Graf
@ 2019-01-07  7:47               ` AKASHI Takahiro
  2019-01-08  7:29                 ` AKASHI Takahiro
  0 siblings, 1 reply; 30+ messages in thread
From: AKASHI Takahiro @ 2019-01-07  7:47 UTC (permalink / raw)
  To: u-boot

Heinrich,

On Wed, Dec 26, 2018 at 10:20:32PM +0100, Alexander Graf wrote:
> 
> 
> On 25.12.18 09:44, AKASHI Takahiro wrote:
> > On Sun, Dec 23, 2018 at 02:56:40AM +0100, Alexander Graf wrote:
> >>
> >>
> >> On 19.12.18 13:23, Heinrich Schuchardt wrote:
> >>> On 12/19/18 2:49 AM, AKASHI Takahiro wrote:
> >>>> Heinrich,
> >>>>
> >>>> On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote:
> >>>>> On 12/18/18 6:05 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>
> >>>>>
> >>>>> Hello Takahiro,
> >>>>>
> >>>>> in several patch series you are implementing multiple interactive
> >>>>> commands that concern
> >>>>>
> >>>>> - handling of EFI variables
> >>>>> - executing EFI binaries
> >>>>> - managing boot sequence
> >>>>>
> >>>>> I very much appreciate your effort to provide an independent UEFI shell
> >>>>> implementation. What I am worried about is that your current patches
> >>>>> make it part of the monolithic U-Boot binary.
> >>>>
> >>>> First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's
> >>>> comment on v2. So you can disable efishell command if you don't want it.
> >>>>
> >>>> Are you still worried?
> >>>>
> >>>>> This design has multiple drawbacks:
> >>>>>
> >>>>> The memory size available for U-Boot is very limited for many devices.
> >>>>> We already had to disable EFI_LOADER for some boards due to this
> >>>>> limitations. Hence we want to keep everything out of the U-Boot binary
> >>>>> that does not serve the primary goal of loading and executing the next
> >>>>> binary.
> >>>>
> >>>> I don't know your point here. If EFI_LOADER is disabled, efishell
> >>>> will never be compiled in.
> >>>>
> >>>>> The UEFI forum has published a UEFI Shell specification which is very
> >>>>> extensive. We still have a lot of deficiencies in U-Boot's UEFI API
> >>>>> implementation. By merging in parts of an UEFI shell implementation our
> >>>>> project looses focus.
> >>>>
> >>>> What is "our project?" What is "focus?"
> >>>> I'm just asking as I want to share that information with you.
> >>>>
> >>>>> There is an EDK2 implementation of said
> >>>>> specification. If we fix the remaining bugs of the EFI API
> >>>>> implementation in U-Boot we could simply run the EDK2 shell which
> >>>>> provides all that is needed for interactive work.
> >>>>>
> >>>>> With you monolithic approach your UEFI shell implementation can neither
> >>>>> be used with other UEFI API implementations than U-Boot nor can it be
> >>>>> tested against other API implementations.
> >>>>
> >>>> Let me explain my stance.
> >>>> My efishell is basically something like a pursuit as well as
> >>>> a debug/test tool which was and is still quite useful for me.
> >>>> Without it, I would have completed (most of) my efi-related work so far.
> >>>> So I believe that it will also be useful for other people who may want
> >>>> to get involved and play with u-boot's efi environment.
> >>>
> >>> On SD-Cards U-Boot is installed between the MBR and the first partition.
> >>> On other devices it is put into a very small ROM. Both ways the maximum
> >>> size is rather limited.
> >>>
> >>> U-Boot provides all that is needed to load and execute an EFI binary. So
> >>> you can put your efishell as file into the EFI partition like you would
> >>> install the EDK2 shell.
> >>>
> >>> The only hardshift this approach brings is that you have to implement
> >>> your own printf because UEFI does not offer formatted output. But this
> >>> can be copied from lib/efi_selftest/efi_selftest_console.c.
> >>>
> >>> The same decision I took for booting from iSCSI. I did not try to put an
> >>> iSCSI driver into U-Boot instead I use iPXE as an executable that is
> >>> loaded from the EFI partition.
> >>>
> >>>>
> >>>> I have never intended to fully implement a shell which is to be compliant
> >>>> with UEFI specification while I'm trying to mimick some command
> >>>> interfaces for convenience. UEFI shell, as you know, provides plenty
> >>>> of "protocols" on which some UEFI applications, including UEFI SCT,
> >>>> reply. I will never implement it with my efishell.
> >>>>
> >>>> I hope that my efishell is a quick and easy way of learning more about
> >>>> u-boot's uefi environment. I will be even happier if more people
> >>>> get involved there.
> >>>>
> >>>>> Due to these considerations I suggest that you build your UEFI shell
> >>>>> implementation as a separate UEFI binary (like helloworld.efi). You may
> >>>>> offer an embedding of the binary (like the bootefi hello command) into
> >>>>> the finally linked U-Boot binary via a configuration variable. Please,
> >>>>> put the shell implementation into a separate directory. You may want to
> >>>>> designate yourself as maintainer (in file MAINTAINERS).
> >>>>
> >>>> Yeah, your suggestion is reasonable and I have thought of it before.
> >>>> There are, however, several reasons that I haven't done so; particularly,
> >>>> efishell is implemented not only with boottime services but also
> >>>> other helper functions, say, from device path utilities. Exporting them
> >>>> as libraries is possible but I don't think that it would be so valuable.
> >>>>
> >>>> Even if efishell is a separate application, it will not contribute to
> >>>> reduce the total footprint if it is embedded along with u-boot binary.
> >>>
> >>> That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into
> >>> the U-Boot binary - is default no. Same I would do for efishell.efi.
> >>
> >> One big drawback with a separate binary is the missing command line
> >> integration. It becomes quite awkward to execute efi debug commands
> >> then, since you'll have to run them through a special bootefi subcommand.
> >>
> >> If you really want to have a "uefi shell", I think the sanest option is
> >> to just provide a built-in copy of the edk2 uefi shell, similar to the
> >> hello world binary. The big benefit of this patch set however, is not
> >> that we get a shell - it's that we get quick and tiny debug
> >> introspectability into efi_loader data structures.
> > 
> > And my command can be used for simple testing.
> 
> Exactly, that would give us the best of both worlds.
> 
> > 
> >> I think the biggest problem here really is the name of the code. Why
> >> don't we just call it "debugefi"? It would be default N except for debug
> >> targets (just like bootefi_hello).
> >>
> >> That way when someone wants to just quickly introspect internal data
> >> structures, they can. I also hope that if the name contains debug,
> >> nobody will expect command line compatibility going forward, so we have
> >> much more freedom to change internals (which is my biggest concern).
> >>
> >> So in my opinion, if you fix the 2 other comments from Heinrich and
> >> rename everything from "efishell" to "debugefi" (so it aligns with
> >> bootefi), we should be good.
> > 
> > If Heinrich agrees, I will fix the name although I'm not a super fan
> > of this new name :)
> 
> Well, feel free to come up with a new one, but it definitely must have a
> ring to it that it's a tiny, debug only feature that is not intended for
> normal use ;).

Do you have any idea/preference about this command's name?

-Takahiro Akashi

> For normal operation, we need to come up with mechanisms that integrate
> much deeper into U-Boot's generic command structure.
> 
> 
> Alex

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables
  2019-01-07  7:47               ` AKASHI Takahiro
@ 2019-01-08  7:29                 ` AKASHI Takahiro
  2019-01-08  8:58                   ` Alexander Graf
  0 siblings, 1 reply; 30+ messages in thread
From: AKASHI Takahiro @ 2019-01-08  7:29 UTC (permalink / raw)
  To: u-boot

On Mon, Jan 07, 2019 at 04:47:13PM +0900, AKASHI Takahiro wrote:
> Heinrich,
> 
> On Wed, Dec 26, 2018 at 10:20:32PM +0100, Alexander Graf wrote:
> > 
> > 
> > On 25.12.18 09:44, AKASHI Takahiro wrote:
> > > On Sun, Dec 23, 2018 at 02:56:40AM +0100, Alexander Graf wrote:
> > >>
> > >>
> > >> On 19.12.18 13:23, Heinrich Schuchardt wrote:
> > >>> On 12/19/18 2:49 AM, AKASHI Takahiro wrote:
> > >>>> Heinrich,
> > >>>>
> > >>>> On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote:
> > >>>>> On 12/18/18 6:05 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>
> > >>>>>
> > >>>>> Hello Takahiro,
> > >>>>>
> > >>>>> in several patch series you are implementing multiple interactive
> > >>>>> commands that concern
> > >>>>>
> > >>>>> - handling of EFI variables
> > >>>>> - executing EFI binaries
> > >>>>> - managing boot sequence
> > >>>>>
> > >>>>> I very much appreciate your effort to provide an independent UEFI shell
> > >>>>> implementation. What I am worried about is that your current patches
> > >>>>> make it part of the monolithic U-Boot binary.
> > >>>>
> > >>>> First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's
> > >>>> comment on v2. So you can disable efishell command if you don't want it.
> > >>>>
> > >>>> Are you still worried?
> > >>>>
> > >>>>> This design has multiple drawbacks:
> > >>>>>
> > >>>>> The memory size available for U-Boot is very limited for many devices.
> > >>>>> We already had to disable EFI_LOADER for some boards due to this
> > >>>>> limitations. Hence we want to keep everything out of the U-Boot binary
> > >>>>> that does not serve the primary goal of loading and executing the next
> > >>>>> binary.
> > >>>>
> > >>>> I don't know your point here. If EFI_LOADER is disabled, efishell
> > >>>> will never be compiled in.
> > >>>>
> > >>>>> The UEFI forum has published a UEFI Shell specification which is very
> > >>>>> extensive. We still have a lot of deficiencies in U-Boot's UEFI API
> > >>>>> implementation. By merging in parts of an UEFI shell implementation our
> > >>>>> project looses focus.
> > >>>>
> > >>>> What is "our project?" What is "focus?"
> > >>>> I'm just asking as I want to share that information with you.
> > >>>>
> > >>>>> There is an EDK2 implementation of said
> > >>>>> specification. If we fix the remaining bugs of the EFI API
> > >>>>> implementation in U-Boot we could simply run the EDK2 shell which
> > >>>>> provides all that is needed for interactive work.
> > >>>>>
> > >>>>> With you monolithic approach your UEFI shell implementation can neither
> > >>>>> be used with other UEFI API implementations than U-Boot nor can it be
> > >>>>> tested against other API implementations.
> > >>>>
> > >>>> Let me explain my stance.
> > >>>> My efishell is basically something like a pursuit as well as
> > >>>> a debug/test tool which was and is still quite useful for me.
> > >>>> Without it, I would have completed (most of) my efi-related work so far.
> > >>>> So I believe that it will also be useful for other people who may want
> > >>>> to get involved and play with u-boot's efi environment.
> > >>>
> > >>> On SD-Cards U-Boot is installed between the MBR and the first partition.
> > >>> On other devices it is put into a very small ROM. Both ways the maximum
> > >>> size is rather limited.
> > >>>
> > >>> U-Boot provides all that is needed to load and execute an EFI binary. So
> > >>> you can put your efishell as file into the EFI partition like you would
> > >>> install the EDK2 shell.
> > >>>
> > >>> The only hardshift this approach brings is that you have to implement
> > >>> your own printf because UEFI does not offer formatted output. But this
> > >>> can be copied from lib/efi_selftest/efi_selftest_console.c.
> > >>>
> > >>> The same decision I took for booting from iSCSI. I did not try to put an
> > >>> iSCSI driver into U-Boot instead I use iPXE as an executable that is
> > >>> loaded from the EFI partition.
> > >>>
> > >>>>
> > >>>> I have never intended to fully implement a shell which is to be compliant
> > >>>> with UEFI specification while I'm trying to mimick some command
> > >>>> interfaces for convenience. UEFI shell, as you know, provides plenty
> > >>>> of "protocols" on which some UEFI applications, including UEFI SCT,
> > >>>> reply. I will never implement it with my efishell.
> > >>>>
> > >>>> I hope that my efishell is a quick and easy way of learning more about
> > >>>> u-boot's uefi environment. I will be even happier if more people
> > >>>> get involved there.
> > >>>>
> > >>>>> Due to these considerations I suggest that you build your UEFI shell
> > >>>>> implementation as a separate UEFI binary (like helloworld.efi). You may
> > >>>>> offer an embedding of the binary (like the bootefi hello command) into
> > >>>>> the finally linked U-Boot binary via a configuration variable. Please,
> > >>>>> put the shell implementation into a separate directory. You may want to
> > >>>>> designate yourself as maintainer (in file MAINTAINERS).
> > >>>>
> > >>>> Yeah, your suggestion is reasonable and I have thought of it before.
> > >>>> There are, however, several reasons that I haven't done so; particularly,
> > >>>> efishell is implemented not only with boottime services but also
> > >>>> other helper functions, say, from device path utilities. Exporting them
> > >>>> as libraries is possible but I don't think that it would be so valuable.
> > >>>>
> > >>>> Even if efishell is a separate application, it will not contribute to
> > >>>> reduce the total footprint if it is embedded along with u-boot binary.
> > >>>
> > >>> That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into
> > >>> the U-Boot binary - is default no. Same I would do for efishell.efi.
> > >>
> > >> One big drawback with a separate binary is the missing command line
> > >> integration. It becomes quite awkward to execute efi debug commands
> > >> then, since you'll have to run them through a special bootefi subcommand.
> > >>
> > >> If you really want to have a "uefi shell", I think the sanest option is
> > >> to just provide a built-in copy of the edk2 uefi shell, similar to the
> > >> hello world binary. The big benefit of this patch set however, is not
> > >> that we get a shell - it's that we get quick and tiny debug
> > >> introspectability into efi_loader data structures.
> > > 
> > > And my command can be used for simple testing.
> > 
> > Exactly, that would give us the best of both worlds.
> > 
> > > 
> > >> I think the biggest problem here really is the name of the code. Why
> > >> don't we just call it "debugefi"? It would be default N except for debug
> > >> targets (just like bootefi_hello).
> > >>
> > >> That way when someone wants to just quickly introspect internal data
> > >> structures, they can. I also hope that if the name contains debug,
> > >> nobody will expect command line compatibility going forward, so we have
> > >> much more freedom to change internals (which is my biggest concern).
> > >>
> > >> So in my opinion, if you fix the 2 other comments from Heinrich and
> > >> rename everything from "efishell" to "debugefi" (so it aligns with
> > >> bootefi), we should be good.
> > > 
> > > If Heinrich agrees, I will fix the name although I'm not a super fan
> > > of this new name :)
> > 
> > Well, feel free to come up with a new one, but it definitely must have a
> > ring to it that it's a tiny, debug only feature that is not intended for
> > normal use ;).
> 
> Do you have any idea/preference about this command's name?

I prefer efidebug/efidbg or efitool so that we can use a shorthand
name, efi, at command line in most cases.

-Takahiro Akashi


> -Takahiro Akashi
> 
> > For normal operation, we need to come up with mechanisms that integrate
> > much deeper into U-Boot's generic command structure.
> > 
> > 
> > Alex

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables
  2019-01-08  7:29                 ` AKASHI Takahiro
@ 2019-01-08  8:58                   ` Alexander Graf
  0 siblings, 0 replies; 30+ messages in thread
From: Alexander Graf @ 2019-01-08  8:58 UTC (permalink / raw)
  To: u-boot



On 08.01.19 08:29, AKASHI Takahiro wrote:
> On Mon, Jan 07, 2019 at 04:47:13PM +0900, AKASHI Takahiro wrote:
>> Heinrich,
>>
>> On Wed, Dec 26, 2018 at 10:20:32PM +0100, Alexander Graf wrote:
>>>
>>>
>>> On 25.12.18 09:44, AKASHI Takahiro wrote:
>>>> On Sun, Dec 23, 2018 at 02:56:40AM +0100, Alexander Graf wrote:
>>>>>
>>>>>
>>>>> On 19.12.18 13:23, Heinrich Schuchardt wrote:
>>>>>> On 12/19/18 2:49 AM, AKASHI Takahiro wrote:
>>>>>>> Heinrich,
>>>>>>>
>>>>>>> On Tue, Dec 18, 2018 at 07:07:02AM +0100, Heinrich Schuchardt wrote:
>>>>>>>> On 12/18/18 6:05 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>
>>>>>>>>
>>>>>>>> Hello Takahiro,
>>>>>>>>
>>>>>>>> in several patch series you are implementing multiple interactive
>>>>>>>> commands that concern
>>>>>>>>
>>>>>>>> - handling of EFI variables
>>>>>>>> - executing EFI binaries
>>>>>>>> - managing boot sequence
>>>>>>>>
>>>>>>>> I very much appreciate your effort to provide an independent UEFI shell
>>>>>>>> implementation. What I am worried about is that your current patches
>>>>>>>> make it part of the monolithic U-Boot binary.
>>>>>>>
>>>>>>> First of all, in v3, CONFIG_CMD_EFISHELL was introduced after Alex's
>>>>>>> comment on v2. So you can disable efishell command if you don't want it.
>>>>>>>
>>>>>>> Are you still worried?
>>>>>>>
>>>>>>>> This design has multiple drawbacks:
>>>>>>>>
>>>>>>>> The memory size available for U-Boot is very limited for many devices.
>>>>>>>> We already had to disable EFI_LOADER for some boards due to this
>>>>>>>> limitations. Hence we want to keep everything out of the U-Boot binary
>>>>>>>> that does not serve the primary goal of loading and executing the next
>>>>>>>> binary.
>>>>>>>
>>>>>>> I don't know your point here. If EFI_LOADER is disabled, efishell
>>>>>>> will never be compiled in.
>>>>>>>
>>>>>>>> The UEFI forum has published a UEFI Shell specification which is very
>>>>>>>> extensive. We still have a lot of deficiencies in U-Boot's UEFI API
>>>>>>>> implementation. By merging in parts of an UEFI shell implementation our
>>>>>>>> project looses focus.
>>>>>>>
>>>>>>> What is "our project?" What is "focus?"
>>>>>>> I'm just asking as I want to share that information with you.
>>>>>>>
>>>>>>>> There is an EDK2 implementation of said
>>>>>>>> specification. If we fix the remaining bugs of the EFI API
>>>>>>>> implementation in U-Boot we could simply run the EDK2 shell which
>>>>>>>> provides all that is needed for interactive work.
>>>>>>>>
>>>>>>>> With you monolithic approach your UEFI shell implementation can neither
>>>>>>>> be used with other UEFI API implementations than U-Boot nor can it be
>>>>>>>> tested against other API implementations.
>>>>>>>
>>>>>>> Let me explain my stance.
>>>>>>> My efishell is basically something like a pursuit as well as
>>>>>>> a debug/test tool which was and is still quite useful for me.
>>>>>>> Without it, I would have completed (most of) my efi-related work so far.
>>>>>>> So I believe that it will also be useful for other people who may want
>>>>>>> to get involved and play with u-boot's efi environment.
>>>>>>
>>>>>> On SD-Cards U-Boot is installed between the MBR and the first partition.
>>>>>> On other devices it is put into a very small ROM. Both ways the maximum
>>>>>> size is rather limited.
>>>>>>
>>>>>> U-Boot provides all that is needed to load and execute an EFI binary. So
>>>>>> you can put your efishell as file into the EFI partition like you would
>>>>>> install the EDK2 shell.
>>>>>>
>>>>>> The only hardshift this approach brings is that you have to implement
>>>>>> your own printf because UEFI does not offer formatted output. But this
>>>>>> can be copied from lib/efi_selftest/efi_selftest_console.c.
>>>>>>
>>>>>> The same decision I took for booting from iSCSI. I did not try to put an
>>>>>> iSCSI driver into U-Boot instead I use iPXE as an executable that is
>>>>>> loaded from the EFI partition.
>>>>>>
>>>>>>>
>>>>>>> I have never intended to fully implement a shell which is to be compliant
>>>>>>> with UEFI specification while I'm trying to mimick some command
>>>>>>> interfaces for convenience. UEFI shell, as you know, provides plenty
>>>>>>> of "protocols" on which some UEFI applications, including UEFI SCT,
>>>>>>> reply. I will never implement it with my efishell.
>>>>>>>
>>>>>>> I hope that my efishell is a quick and easy way of learning more about
>>>>>>> u-boot's uefi environment. I will be even happier if more people
>>>>>>> get involved there.
>>>>>>>
>>>>>>>> Due to these considerations I suggest that you build your UEFI shell
>>>>>>>> implementation as a separate UEFI binary (like helloworld.efi). You may
>>>>>>>> offer an embedding of the binary (like the bootefi hello command) into
>>>>>>>> the finally linked U-Boot binary via a configuration variable. Please,
>>>>>>>> put the shell implementation into a separate directory. You may want to
>>>>>>>> designate yourself as maintainer (in file MAINTAINERS).
>>>>>>>
>>>>>>> Yeah, your suggestion is reasonable and I have thought of it before.
>>>>>>> There are, however, several reasons that I haven't done so; particularly,
>>>>>>> efishell is implemented not only with boottime services but also
>>>>>>> other helper functions, say, from device path utilities. Exporting them
>>>>>>> as libraries is possible but I don't think that it would be so valuable.
>>>>>>>
>>>>>>> Even if efishell is a separate application, it will not contribute to
>>>>>>> reduce the total footprint if it is embedded along with u-boot binary.
>>>>>>
>>>>>> That is why CONFIG_CMD_BOOTEFI_HELLO - which embeds helloworld.efi into
>>>>>> the U-Boot binary - is default no. Same I would do for efishell.efi.
>>>>>
>>>>> One big drawback with a separate binary is the missing command line
>>>>> integration. It becomes quite awkward to execute efi debug commands
>>>>> then, since you'll have to run them through a special bootefi subcommand.
>>>>>
>>>>> If you really want to have a "uefi shell", I think the sanest option is
>>>>> to just provide a built-in copy of the edk2 uefi shell, similar to the
>>>>> hello world binary. The big benefit of this patch set however, is not
>>>>> that we get a shell - it's that we get quick and tiny debug
>>>>> introspectability into efi_loader data structures.
>>>>
>>>> And my command can be used for simple testing.
>>>
>>> Exactly, that would give us the best of both worlds.
>>>
>>>>
>>>>> I think the biggest problem here really is the name of the code. Why
>>>>> don't we just call it "debugefi"? It would be default N except for debug
>>>>> targets (just like bootefi_hello).
>>>>>
>>>>> That way when someone wants to just quickly introspect internal data
>>>>> structures, they can. I also hope that if the name contains debug,
>>>>> nobody will expect command line compatibility going forward, so we have
>>>>> much more freedom to change internals (which is my biggest concern).
>>>>>
>>>>> So in my opinion, if you fix the 2 other comments from Heinrich and
>>>>> rename everything from "efishell" to "debugefi" (so it aligns with
>>>>> bootefi), we should be good.
>>>>
>>>> If Heinrich agrees, I will fix the name although I'm not a super fan
>>>> of this new name :)
>>>
>>> Well, feel free to come up with a new one, but it definitely must have a
>>> ring to it that it's a tiny, debug only feature that is not intended for
>>> normal use ;).
>>
>> Do you have any idea/preference about this command's name?
> 
> I prefer efidebug/efidbg or efitool so that we can use a shorthand
> name, efi, at command line in most cases.

That definitely works for me as well, yes.


Alex

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [U-Boot] [PATCH v3 1/8] cmd: add efishell command
  2019-01-07  5:08       ` AKASHI Takahiro
@ 2019-01-08  9:57         ` Alexander Graf
  0 siblings, 0 replies; 30+ messages in thread
From: Alexander Graf @ 2019-01-08  9:57 UTC (permalink / raw)
  To: u-boot



On 07.01.19 06:08, AKASHI Takahiro wrote:
> On Sun, Dec 30, 2018 at 06:10:51PM +0100, Heinrich Schuchardt wrote:
>> On 12/30/18 4:44 PM, Heinrich Schuchardt wrote:
>>> On 12/18/18 6:05 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, efishell, helps address these issues and give us
>>>> more friendly interfaces:
>>>>  * efishell boot add: add BootXXXX variable
>>>>  * efishell boot rm: remove BootXXXX variable
>>>>  * efishell boot dump: display all BootXXXX variables
>>>>  * efishell boot order: set/display a boot order (BootOrder)
>>>>  * efishell setvar: set an UEFI variable (with limited functionality)
>>>>  * efishell 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>
>>
>> The behavior is a bit unexpected:
>>
>> => efishell boot order 200
>> => efishell boot order
>>  1: Boot00C8: (not defined)
>> exit not allowed from main input shell.
>>
>> I would expect 'efishell boot order' to take a 4 digit hexadecimal
>> number and to do no conversion from decimal to hexadecimal.
> 
> OK, but we should allow a less-than-4-digit number. 

Yes, but definitely stick to Hex regardless. Hex is the default number
space in U-Boot - and it just happens to fit quite well with the
variable definition too :).


Alex

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2019-01-08  9:57 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-18  5:05 [U-Boot] [PATCH v3 0/8] cmd: add efishell for efi environment AKASHI Takahiro
2018-12-18  5:05 ` [U-Boot] [PATCH v3 1/8] cmd: add efishell command AKASHI Takahiro
2018-12-30 15:44   ` Heinrich Schuchardt
2018-12-30 17:10     ` Heinrich Schuchardt
2019-01-07  5:08       ` AKASHI Takahiro
2019-01-08  9:57         ` Alexander Graf
2019-01-07  5:06     ` AKASHI Takahiro
2018-12-30 23:47   ` Heinrich Schuchardt
2019-01-07  5:13     ` AKASHI Takahiro
2018-12-18  5:05 ` [U-Boot] [PATCH v3 2/8] cmd: efishell: add devices command AKASHI Takahiro
2018-12-18  5:05 ` [U-Boot] [PATCH v3 3/8] cmd: efishell: add drivers command AKASHI Takahiro
2018-12-20  7:51   ` Heinrich Schuchardt
2018-12-25  7:22     ` AKASHI Takahiro
2018-12-25 12:07       ` Heinrich Schuchardt
2018-12-18  5:05 ` [U-Boot] [PATCH v3 4/8] cmd: efishell: add images command AKASHI Takahiro
2018-12-18  5:05 ` [U-Boot] [PATCH v3 5/8] cmd: efishell: add memmap command AKASHI Takahiro
2018-12-18  5:05 ` [U-Boot] [PATCH v3 6/8] cmd: efishell: add dh command AKASHI Takahiro
2018-12-20  7:49   ` Heinrich Schuchardt
2018-12-25  5:32     ` AKASHI Takahiro
2018-12-18  5:05 ` [U-Boot] [PATCH v3 7/8] cmd: efishell: export uefi variable helper functions AKASHI Takahiro
2018-12-18  5:05 ` [U-Boot] [PATCH v3 8/8] cmd: env: add "-e" option for handling UEFI variables AKASHI Takahiro
2018-12-18  6:07   ` Heinrich Schuchardt
2018-12-19  1:49     ` AKASHI Takahiro
2018-12-19 12:23       ` Heinrich Schuchardt
2018-12-23  1:56         ` Alexander Graf
2018-12-25  8:44           ` AKASHI Takahiro
2018-12-26 21:20             ` Alexander Graf
2019-01-07  7:47               ` AKASHI Takahiro
2019-01-08  7:29                 ` AKASHI Takahiro
2019-01-08  8:58                   ` Alexander Graf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.