[U-Boot] [PATCH 0/6] efi: make efi and bootmgr more usable

[U-Boot] [PATCH 0/6] efi: make efi and bootmgr more usable

[AKASHI Takahiro]

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

Patch#1 to #4 are for efishell.
Patch#5 and #6 are for bootmgr.

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

=> bootefi bootmgr -2
WARNING: booting without device tree
Booting: HELLO
## Starting EFI application at 000000007db8b040 ...
Hello, world!
## Application terminated, r = 0

=> efishell setvar PlatformLang en        <--- important!
=> efishell bootmgr -1 or efishell bootmgr

   (shell ...)

# The only drawback is that it can be confusing to type
  "bootefi ..." and "efi(shell) boot ..." :)

Enjoy!
-Takahiro Akashi
AKASHI Takahiro (6):
  fs: update fs_dev_part in fs_set_blk_dev_with_part()
  efi_loader: add efi_dp_from_name()
  efi_loader: bootmgr: add load option helper functions
  cmd: add efishell command
  bootefi: carve out fdt parameter handling
  efi_loader: bootmgr: run an EFI application of a given load option

 cmd/Makefile                     |   2 +-
 cmd/bootefi.c                    | 112 +++----
 cmd/efishell.c                   | 531 +++++++++++++++++++++++++++++++
 fs/fs.c                          |   1 +
 include/efi_loader.h             |  32 +-
 lib/efi_loader/efi_bootmgr.c     |  76 +++--
 lib/efi_loader/efi_device_path.c |  47 +++
 7 files changed, 721 insertions(+), 80 deletions(-)
 create mode 100644 cmd/efishell.c

-- 
2.19.0

[U-Boot] [PATCH 1/6] fs: update fs_dev_part in fs_set_blk_dev_with_part()

[AKASHI Takahiro]

As in the case of fs_set_blk_dev(), fs_set_blk_dev_with_part() should
maintain and update fs_dev_part whenever called.

Without this patch, a problem will come up when an efi binary associated
with efi's BOOTxxxx variable is invoked via "bootefi bootmgr".

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/fs.c b/fs/fs.c
index adae98d021ee..cb265174e2f2 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -365,6 +365,7 @@ int fs_set_blk_dev_with_part(struct blk_desc *desc, int part)
 	for (i = 0, info = fstypes; i < ARRAY_SIZE(fstypes); i++, info++) {
 		if (!info->probe(fs_dev_desc, &fs_partition)) {
 			fs_type = info->fstype;
+			fs_dev_part = part;
 			return 0;
 		}
 	}
-- 
2.19.0

[U-Boot] [PATCH 2/6] efi_loader: add efi_dp_from_name()

[AKASHI Takahiro]

Factor out efi_set_bootdev() and extract efi_dp_from_name().
This function will be used to set a boot device in efishell command.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/bootefi.c                    | 42 ++++++----------------------
 include/efi_loader.h             |  4 +++
 lib/efi_loader/efi_device_path.c | 47 ++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 34 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 99f5b2b95706..b3e2845120fe 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -647,45 +647,19 @@ U_BOOT_CMD(
 
 void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
 {
-	char filename[32] = { 0 }; /* dp->str is u16[32] long */
-	char *s;
+	struct efi_device_path *device, *image;
+	efi_status_t ret;
 
 	/* efi_set_bootdev is typically called repeatedly, recover memory */
 	efi_free_pool(bootefi_device_path);
 	efi_free_pool(bootefi_image_path);
-	/* If blk_get_device_part_str fails, avoid duplicate free. */
-	bootefi_device_path = NULL;
-	bootefi_image_path = NULL;
-
-	if (strcmp(dev, "Net")) {
-		struct blk_desc *desc;
-		disk_partition_t fs_partition;
-		int part;
-
-		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
-					       1);
-		if (part < 0)
-			return;
-
-		bootefi_device_path = efi_dp_from_part(desc, part);
-	} else {
-#ifdef CONFIG_NET
-		bootefi_device_path = efi_dp_from_eth();
-#endif
-	}
-
-	if (!path)
-		return;
 
-	if (strcmp(dev, "Net")) {
-		/* Add leading / to fs paths, because they're absolute */
-		snprintf(filename, sizeof(filename), "/%s", path);
+	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
+	if (ret == EFI_SUCCESS) {
+		bootefi_device_path = device;
+		bootefi_image_path = image;
 	} else {
-		snprintf(filename, sizeof(filename), "%s", path);
+		bootefi_device_path = NULL;
+		bootefi_image_path = NULL;
 	}
-	/* DOS style file path: */
-	s = filename;
-	while ((s = strchr(s, '/')))
-		*s++ = '\\';
-	bootefi_image_path = efi_dp_from_file(NULL, 0, filename);
 }
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 74df070316a0..b73fbb6de23f 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -425,6 +425,10 @@ const struct efi_device_path *efi_dp_last_node(
 efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
 				    struct efi_device_path **device_path,
 				    struct efi_device_path **file_path);
+efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
+			      const char *path,
+			      struct efi_device_path **device,
+			      struct efi_device_path **file);
 
 #define EFI_DP_TYPE(_dp, _type, _subtype) \
 	(((_dp)->type == DEVICE_PATH_TYPE_##_type) && \
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 5a61a1c1dcf9..3af444147283 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -942,3 +942,50 @@ efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
 	*file_path = fp;
 	return EFI_SUCCESS;
 }
+
+efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
+			      const char *path,
+			      struct efi_device_path **device,
+			      struct efi_device_path **file)
+{
+	int is_net;
+	struct blk_desc *desc = NULL;
+	disk_partition_t fs_partition;
+	int part = 0;
+	char filename[32] = { 0 }; /* dp->str is u16[32] long */
+	char *s;
+
+	if (!device || (path && !file))
+		return EFI_INVALID_PARAMETER;
+
+	is_net = !strcmp(dev, "Net");
+	if (!is_net) {
+		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
+					       1);
+		if (part < 0)
+			return EFI_INVALID_PARAMETER;
+
+		*device = efi_dp_from_part(desc, part);
+	} else {
+#ifdef CONFIG_NET
+		*device = efi_dp_from_eth();
+#endif
+	}
+
+	if (!path)
+		return EFI_SUCCESS;
+
+	if (!is_net) {
+		/* Add leading / to fs paths, because they're absolute */
+		snprintf(filename, sizeof(filename), "/%s", path);
+	} else {
+		snprintf(filename, sizeof(filename), "%s", path);
+	}
+	/* DOS style file path: */
+	s = filename;
+	while ((s = strchr(s, '/')))
+		*s++ = '\\';
+	*file = efi_dp_from_file(NULL, 0, filename);
+
+	return EFI_SUCCESS;
+}
-- 
2.19.0

[U-Boot] [PATCH 3/6] efi_loader: bootmgr: add load option helper functions

[AKASHI Takahiro]

In this patch, helper functions for an load option variable (BootXXXX)
are added:
* efi_parse_load_option(): parse a string into load_option data
			   (renamed from parse_load_option and exported)
* efi_marshel_load_option(): convert load_option data into a string

Those functions will be used to implement efishell command.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/efi_loader.h         | 25 +++++++++++++
 lib/efi_loader/efi_bootmgr.c | 68 ++++++++++++++++++++++++------------
 2 files changed, 70 insertions(+), 23 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index b73fbb6de23f..1cabb1680d20 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -502,6 +502,31 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_guid_t *vendor,
 				     u32 attributes, efi_uintn_t data_size,
 				     void *data);
 
+/*
+ * See section 3.1.3 in the v2.7 UEFI spec for more details on
+ * the layout of EFI_LOAD_OPTION.  In short it is:
+ *
+ *    typedef struct _EFI_LOAD_OPTION {
+ *        UINT32 Attributes;
+ *        UINT16 FilePathListLength;
+ *        // CHAR16 Description[];   <-- variable length, NULL terminated
+ *        // EFI_DEVICE_PATH_PROTOCOL FilePathList[];
+ *						 <-- FilePathListLength bytes
+ *        // UINT8 OptionalData[];
+ *    } EFI_LOAD_OPTION;
+ */
+struct load_option {
+	u32 attributes;
+	u16 file_path_length;
+	u16 *label;
+	struct efi_device_path *file_path;
+	u8 *optional_data;
+};
+
+void efi_parse_load_option(struct load_option *lo, void *ptr);
+unsigned long efi_marshal_load_option(u32 attr, u16 *label,
+				      struct efi_device_path *file_path,
+				      char *option, void **data);
 void *efi_bootmgr_load(struct efi_device_path **device_path,
 		       struct efi_device_path **file_path);
 
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 0c5764db127b..c2d29f956065 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -30,28 +30,8 @@ static const struct efi_runtime_services *rs;
  */
 
 
-/*
- * See section 3.1.3 in the v2.7 UEFI spec for more details on
- * the layout of EFI_LOAD_OPTION.  In short it is:
- *
- *    typedef struct _EFI_LOAD_OPTION {
- *        UINT32 Attributes;
- *        UINT16 FilePathListLength;
- *        // CHAR16 Description[];   <-- variable length, NULL terminated
- *        // EFI_DEVICE_PATH_PROTOCOL FilePathList[];  <-- FilePathListLength bytes
- *        // UINT8 OptionalData[];
- *    } EFI_LOAD_OPTION;
- */
-struct load_option {
-	u32 attributes;
-	u16 file_path_length;
-	u16 *label;
-	struct efi_device_path *file_path;
-	u8 *optional_data;
-};
-
 /* parse an EFI_LOAD_OPTION, as described above */
-static void parse_load_option(struct load_option *lo, void *ptr)
+void efi_parse_load_option(struct load_option *lo, void *ptr)
 {
 	lo->attributes = *(u32 *)ptr;
 	ptr += sizeof(u32);
@@ -60,7 +40,7 @@ static void parse_load_option(struct load_option *lo, void *ptr)
 	ptr += sizeof(u16);
 
 	lo->label = ptr;
-	ptr += (u16_strlen(lo->label) + 1) * 2;
+	ptr += (u16_strlen(lo->label) + 1) * sizeof(u16);
 
 	lo->file_path = ptr;
 	ptr += lo->file_path_length;
@@ -68,6 +48,48 @@ static void parse_load_option(struct load_option *lo, void *ptr)
 	lo->optional_data = ptr;
 }
 
+unsigned long efi_marshal_load_option(u32 attr, u16 *label,
+				      struct efi_device_path *file_path,
+				      char *option, void **data)
+{
+	unsigned long size;
+	unsigned long label_len, option_len;
+	u16 file_path_len;
+	void *p;
+
+	label_len = (u16_strlen(label) + 1) * sizeof(u16);
+	file_path_len = efi_dp_size(file_path)
+			+ sizeof(struct efi_device_path); /* for END */
+	option_len = strlen(option);
+
+	/* total size */
+	size = sizeof(attr);
+	size += file_path_len;
+	size += label_len;
+	size += option_len + 1;
+	p = malloc(size);
+	if (!p)
+		return 0;
+
+	/* copy data */
+	*data = p;
+	memcpy(p, &attr, sizeof(attr));
+	p += sizeof(attr);
+	memcpy(p, &file_path_len, sizeof(file_path_len));
+	p += sizeof(file_path_len);
+	memcpy(p, label, label_len);
+	p += label_len;
+
+	memcpy(p, file_path, file_path_len);
+	p += file_path_len;
+
+	memcpy(p, option, option_len);
+	p += option_len;
+	*(char *)p = '\0';
+
+	return size;
+}
+
 /* free() the result */
 static void *get_var(u16 *name, const efi_guid_t *vendor,
 		     efi_uintn_t *size)
@@ -115,7 +137,7 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
 	if (!load_option)
 		return NULL;
 
-	parse_load_option(&lo, load_option);
+	efi_parse_load_option(&lo, load_option);
 
 	if (lo.attributes & LOAD_OPTION_ACTIVE) {
 		efi_status_t ret;
-- 
2.19.0

[U-Boot] [PATCH 4/6] cmd: add efishell command

[AKASHI Takahiro]

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

The new command, 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/Makefile   |   2 +-
 cmd/efishell.c | 531 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 532 insertions(+), 1 deletion(-)
 create mode 100644 cmd/efishell.c

diff --git a/cmd/Makefile b/cmd/Makefile
index a61fab6583df..e18fb21e4b71 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -24,7 +24,7 @@ obj-$(CONFIG_CMD_BINOP) += binop.o
 obj-$(CONFIG_CMD_BLOCK_CACHE) += blkcache.o
 obj-$(CONFIG_CMD_BMP) += bmp.o
 obj-$(CONFIG_CMD_BOOTCOUNT) += bootcount.o
-obj-$(CONFIG_CMD_BOOTEFI) += bootefi.o
+obj-$(CONFIG_CMD_BOOTEFI) += bootefi.o efishell.o
 obj-$(CONFIG_CMD_BOOTMENU) += bootmenu.o
 obj-$(CONFIG_CMD_BOOTSTAGE) += bootstage.o
 obj-$(CONFIG_CMD_BOOTZ) += bootz.o
diff --git a/cmd/efishell.c b/cmd/efishell.c
new file mode 100644
index 000000000000..7cadb9e47712
--- /dev/null
+++ b/cmd/efishell.c
@@ -0,0 +1,531 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  EFI Shell-like command
+ *
+ *  Copyright (c) 2018 Takahiro AKASHI, 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 <search.h>
+#include <asm/global_data.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+/*
+ * 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;
+	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) {
+		puts(res);
+		free(res);
+		if (len < 2 && argc == 2)
+			printf("%s: not found\n", argv[1]);
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static bool isalnum(char c)
+{
+	if (c >= '0' && c <= '9')
+		return true;
+	if (c >= 'A' && c <= 'F')
+		return true;
+	if (c >= 'a' && c <= 'f')
+		return true;
+
+	return false;
+}
+
+static int do_efi_set_var(int argc, char * const argv[])
+{
+	char *value, *data, *ptr, *ptr2 = NULL, num[3] = {'\0', '\0', '\0'};
+	unsigned long len, size;
+	u16 *var_name16;
+	efi_guid_t guid;
+	efi_status_t ret;
+
+	if (argc > 3)
+		return CMD_RET_USAGE;
+
+	if (argc == 1)
+		return CMD_RET_SUCCESS;
+
+	if (argc == 2) {
+		/* remove */
+		value = NULL;
+		size = 0;
+	} else { /* set */
+		value = argv[2];
+		if (value[0] == '=')
+			value++;
+		len = strlen(value);
+		if ((len > 2) && !strncmp(value, "0x", 2)) {
+			if ((len % 2))
+				return CMD_RET_USAGE;
+
+			data = value + 2;
+			size = (len - 2) / 2;
+			/* FIXME */
+			ptr = malloc(size);
+			ptr2 = ptr;
+			len = 0;
+			while (len < size * 2) {
+				num[0] = data[len++];
+				num[1] = data[len++];
+				if (!isalnum(num[0]) || !isalnum(num[1])) {
+					printf("Invalid format: %s\n\n", value);
+					ret = CMD_RET_USAGE;
+					goto out;
+				}
+				*ptr++ = (char)simple_strtoul(num, NULL, 16);
+			}
+			value = data;
+		} else {
+			size = len;
+		}
+	}
+
+	var_name16 = malloc((strlen(argv[1]) + 1) * 2);
+	utf8_to_utf16(var_name16, (u8 *)argv[1], strlen(argv[1]) + 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:
+	if (ptr2)
+		free(ptr2);
+	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];
+	efi_guid_t guid;
+	u32 attr;
+	size_t label_len, label_len16;
+	u16 *label, *p;
+	char *interface, *device, *file, *option;
+	struct efi_device_path *device_path = NULL, *file_path = NULL;
+	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);
+	utf8_to_utf16(var_name16, (u8 *)var_name, 9);
+	guid = efi_global_variable_guid;
+
+	attr = 0x1; /* always ACTIVE */
+
+	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;
+	p = label;
+	utf8_utf16_strncpy(&p, argv[2], label_len);
+
+	interface = argv[3];
+	device = argv[4];
+	file = argv[5];
+	option = (argc == 6 ? "" : argv[6]);
+
+	ret = efi_dp_from_name(interface, device, file, &device_path,
+			       &file_path);
+	if (ret != EFI_SUCCESS) {
+		ret = CMD_RET_FAILURE;
+		goto out;
+	}
+
+	size = efi_marshal_load_option(attr, label, file_path, option, &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);
+	free(device_path);
+	free(file_path);
+	free(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_to_utf16(var_name16, (u8 *)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 load_option lo;
+	char *label, *p;
+	size_t label_len16, label_len;
+	u16 *dp_str;
+
+	efi_parse_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];
+	efi_guid_t guid;
+	void *data = NULL;
+	unsigned long size;
+	int ret;
+
+	sprintf(var_name, "Boot%04X", id);
+	utf8_to_utf16(var_name16, (u8 *)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);
+		free(data);
+	} else if (ret == EFI_NOT_FOUND) {
+		printf("Boot%04X: not found\n", id);
+	}
+}
+
+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];
+	void *data;
+	struct load_option lo;
+	char *label, *p;
+	size_t label_len16, label_len;
+	efi_status_t ret = CMD_RET_SUCCESS;
+
+	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_SUCCESS) {
+		printf("BootOrder not defined\n");
+		return CMD_RET_SUCCESS;
+	}
+
+	num = size / sizeof(u16);
+	for (i = 0; i < num; i++) {
+		sprintf(var_name, "Boot%04X", bootorder[i]);
+		utf8_to_utf16(var_name16, (u8 *)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_parse_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;
+	/* TODO: declare this somewhere else */
+	extern efi_status_t efi_init_obj_list(void);
+	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"))
+		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.0

[U-Boot] [PATCH 5/6] bootefi: carve out fdt parameter handling

[AKASHI Takahiro]

The current way how command parameters, particularly "fdt addr," are
handled makes it a bit complicated to add a subcommand-specific parameter.
So just refactor the code and extract efi_handle_fdt().

This commit is a preparatory change for enhancing bootmgr sub-command.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/bootefi.c | 51 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index b3e2845120fe..5772f7422e5f 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -349,6 +349,31 @@ static efi_status_t efi_install_fdt(ulong fdt_addr)
 	return ret;
 }
 
+static int efi_handle_fdt(char *fdt_opt)
+{
+	unsigned long fdt_addr;
+	efi_status_t r;
+
+	if (fdt_opt) {
+		fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
+		if (!fdt_addr && *fdt_opt != '0')
+			return CMD_RET_USAGE;
+
+		/* Install device tree */
+		r = efi_install_fdt(fdt_addr);
+		if (r != EFI_SUCCESS) {
+			printf("ERROR: failed to install device tree\n");
+			return CMD_RET_FAILURE;
+		}
+	} else {
+		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
+		efi_install_configuration_table(&efi_guid_fdt, NULL);
+		printf("WARNING: booting without device tree\n");
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
 /**
  * do_bootefi_exec() - execute EFI binary
  *
@@ -516,7 +541,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	unsigned long addr;
 	char *saddr;
 	efi_status_t r;
-	unsigned long fdt_addr;
 
 	/* Allow unaligned memory access */
 	allow_unaligned();
@@ -532,21 +556,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
-	if (argc > 2) {
-		fdt_addr = simple_strtoul(argv[2], NULL, 16);
-		if (!fdt_addr && *argv[2] != '0')
-			return CMD_RET_USAGE;
-		/* Install device tree */
-		r = efi_install_fdt(fdt_addr);
-		if (r != EFI_SUCCESS) {
-			printf("ERROR: failed to install device tree\n");
-			return CMD_RET_FAILURE;
-		}
-	} else {
-		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
-		efi_install_configuration_table(&efi_guid_fdt, NULL);
-		printf("WARNING: booting without device tree\n");
-	}
 #ifdef CONFIG_CMD_BOOTEFI_HELLO
 	if (!strcmp(argv[1], "hello")) {
 		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
@@ -564,6 +573,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		struct efi_loaded_image_obj *image_handle;
 		struct efi_loaded_image *loaded_image_info;
 
+		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
+			return CMD_RET_FAILURE;
+
 		/* Construct a dummy device path. */
 		bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
 						      (uintptr_t)&efi_selftest,
@@ -593,7 +605,10 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	} else
 #endif
 	if (!strcmp(argv[1], "bootmgr")) {
-		return do_bootefi_bootmgr_exec();
+		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
+			return CMD_RET_FAILURE;
+
+		return do_bootefi_bootmgr_exec(boot_id);
 	} else {
 		saddr = argv[1];
 
@@ -602,6 +617,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (!addr && *saddr != '0')
 			return CMD_RET_USAGE;
 
+		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
+			return CMD_RET_FAILURE;
 	}
 
 	printf("## Starting EFI application at %08lx ...\n", addr);
-- 
2.19.0

[U-Boot] [PATCH 6/6] efi_loader: bootmgr: run an EFI application of a given load option

[AKASHI Takahiro]

With this patch applied, we will be able to selectively execute
an EFI application by specifying a load option, say "-1" for Boot0001,
"-2" for Boot0002 and so on.

  => bootefi bootmgr -1 <fdt addr>

Please note that BootXXXX need not be included in "BootOrder".

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/bootefi.c                | 19 ++++++++++++++++---
 include/efi_loader.h         |  3 ++-
 lib/efi_loader/efi_bootmgr.c |  8 +++++++-
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 5772f7422e5f..434a6a07c26a 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -508,7 +508,7 @@ exit:
 	return ret;
 }
 
-static int do_bootefi_bootmgr_exec(void)
+static int do_bootefi_bootmgr_exec(int boot_id)
 {
 	struct efi_device_path *device_path, *file_path;
 	void *addr;
@@ -520,7 +520,7 @@ static int do_bootefi_bootmgr_exec(void)
 	 */
 	efi_save_gd();
 
-	addr = efi_bootmgr_load(&device_path, &file_path);
+	addr = efi_bootmgr_load(boot_id, &device_path, &file_path);
 	if (!addr)
 		return 1;
 
@@ -605,6 +605,19 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	} else
 #endif
 	if (!strcmp(argv[1], "bootmgr")) {
+		char *endp;
+		int boot_id = -1;
+
+		if ((argc > 2) && (argv[2][0] == '-')) {
+			boot_id = (int)simple_strtoul(&argv[2][1], &endp, 0);
+			if ((argv[2] + strlen(argv[2]) != endp) ||
+			    (boot_id > 0xffff))
+				return CMD_RET_USAGE;
+
+			argc--;
+			argv++;
+		}
+
 		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
 			return CMD_RET_FAILURE;
 
@@ -649,7 +662,7 @@ static char bootefi_help_text[] =
 	"    Use environment variable efi_selftest to select a single test.\n"
 	"    Use 'setenv efi_selftest list' to enumerate all tests.\n"
 #endif
-	"bootefi bootmgr [fdt addr]\n"
+	"bootefi bootmgr [-XXXX] [fdt addr]\n"
 	"  - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
 	"\n"
 	"    If specified, the device tree located at <fdt address> gets\n"
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 1cabb1680d20..5804c2b5015d 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -527,7 +527,8 @@ void efi_parse_load_option(struct load_option *lo, void *ptr);
 unsigned long efi_marshal_load_option(u32 attr, u16 *label,
 				      struct efi_device_path *file_path,
 				      char *option, void **data);
-void *efi_bootmgr_load(struct efi_device_path **device_path,
+void *efi_bootmgr_load(int boot_id,
+		       struct efi_device_path **device_path,
 		       struct efi_device_path **file_path);
 
 #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index c2d29f956065..348f99a9ca25 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -165,7 +165,8 @@ error:
  * available load-options, finding and returning the first one that can
  * be loaded successfully.
  */
-void *efi_bootmgr_load(struct efi_device_path **device_path,
+void *efi_bootmgr_load(int boot_id,
+		       struct efi_device_path **device_path,
 		       struct efi_device_path **file_path)
 {
 	uint16_t *bootorder;
@@ -178,6 +179,11 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
 	bs = systab.boottime;
 	rs = systab.runtime;
 
+	if (boot_id != -1) {
+		image = try_load_entry(boot_id, device_path, file_path);
+		goto error;
+	}
+
 	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
 	if (!bootorder)
 		goto error;
-- 
2.19.0

[U-Boot] [PATCH 0/6] efi: make efi and bootmgr more usable

[Alexander Graf]

On 17.10.18 09:32, AKASHI Takahiro wrote:
> This patch set is a collection of patches to enhance efi user interfaces
> /commands. It will help improve user experience on efi boot and make it
> more usable without edk2's shell utility.

That's amazing, thanks a lot :)

> Patch#1 to #4 are for efishell.
> Patch#5 and #6 are for bootmgr.
> 
> 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

IMHO those 3 belong semantically to the "bootmgr" command. I can see how
"bootefi bootmgr add 1 SHELL ..." is terrible UX. But then again it's
not too much more to type than "efishell boot", right? ;)

So at the end of the day, these should probably be

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

Same thing here :).

>  1: Boot0001: SHELL
>  2: Boot0002: HELLO
> 
> => bootefi bootmgr -2
> WARNING: booting without device tree
> Booting: HELLO
> ## Starting EFI application at 000000007db8b040 ...
> Hello, world!
> ## Application terminated, r = 0
> 
> => efishell setvar PlatformLang en        <--- important!

That one is slightly more complicated. How about we introduce a -e flag
to all the env operations? Then this would become

 => setenv -e PlatformLang en

and you could print only EFI variables using

 => printenv -e

maybe one day we could then also just implement partial variable storage
for UEFI variables:

 => saveenv -e

which we could then reuse in the ExitBootServices() call to persist EFI
variables?

> => efishell bootmgr -1 or efishell bootmgr
> 
>    (shell ...)
> 
> # The only drawback is that it can be confusing to type
>   "bootefi ..." and "efi(shell) boot ..." :)

Yes :).


Alex

[U-Boot] [PATCH 3/6] efi_loader: bootmgr: add load option helper functions

[Alexander Graf]

On 17.10.18 09:32, AKASHI Takahiro wrote:
> In this patch, helper functions for an load option variable (BootXXXX)
> are added:
> * efi_parse_load_option(): parse a string into load_option data
> 			   (renamed from parse_load_option and exported)
> * efi_marshel_load_option(): convert load_option data into a string
> 
> Those functions will be used to implement efishell command.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  include/efi_loader.h         | 25 +++++++++++++
>  lib/efi_loader/efi_bootmgr.c | 68 ++++++++++++++++++++++++------------
>  2 files changed, 70 insertions(+), 23 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index b73fbb6de23f..1cabb1680d20 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -502,6 +502,31 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_guid_t *vendor,
>  				     u32 attributes, efi_uintn_t data_size,
>  				     void *data);
>  
> +/*
> + * See section 3.1.3 in the v2.7 UEFI spec for more details on
> + * the layout of EFI_LOAD_OPTION.  In short it is:
> + *
> + *    typedef struct _EFI_LOAD_OPTION {
> + *        UINT32 Attributes;
> + *        UINT16 FilePathListLength;
> + *        // CHAR16 Description[];   <-- variable length, NULL terminated
> + *        // EFI_DEVICE_PATH_PROTOCOL FilePathList[];
> + *						 <-- FilePathListLength bytes
> + *        // UINT8 OptionalData[];
> + *    } EFI_LOAD_OPTION;
> + */
> +struct load_option {
> +	u32 attributes;
> +	u16 file_path_length;
> +	u16 *label;
> +	struct efi_device_path *file_path;
> +	u8 *optional_data;
> +};

If this is part of the spec, shouldn't it rather be in efi_api.h? It
probably also wants an efi_ prefix then :).

> +
> +void efi_parse_load_option(struct load_option *lo, void *ptr);
> +unsigned long efi_marshal_load_option(u32 attr, u16 *label,
> +				      struct efi_device_path *file_path,
> +				      char *option, void **data);
>  void *efi_bootmgr_load(struct efi_device_path **device_path,
>  		       struct efi_device_path **file_path);
>  
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 0c5764db127b..c2d29f956065 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -30,28 +30,8 @@ static const struct efi_runtime_services *rs;
>   */
>  
>  
> -/*
> - * See section 3.1.3 in the v2.7 UEFI spec for more details on
> - * the layout of EFI_LOAD_OPTION.  In short it is:
> - *
> - *    typedef struct _EFI_LOAD_OPTION {
> - *        UINT32 Attributes;
> - *        UINT16 FilePathListLength;
> - *        // CHAR16 Description[];   <-- variable length, NULL terminated
> - *        // EFI_DEVICE_PATH_PROTOCOL FilePathList[];  <-- FilePathListLength bytes
> - *        // UINT8 OptionalData[];
> - *    } EFI_LOAD_OPTION;
> - */
> -struct load_option {
> -	u32 attributes;
> -	u16 file_path_length;
> -	u16 *label;
> -	struct efi_device_path *file_path;
> -	u8 *optional_data;
> -};
> -
>  /* parse an EFI_LOAD_OPTION, as described above */
> -static void parse_load_option(struct load_option *lo, void *ptr)
> +void efi_parse_load_option(struct load_option *lo, void *ptr)

I think you want to rename this to "deserialize" to better align with ...

>  {
>  	lo->attributes = *(u32 *)ptr;
>  	ptr += sizeof(u32);
> @@ -60,7 +40,7 @@ static void parse_load_option(struct load_option *lo, void *ptr)
>  	ptr += sizeof(u16);
>  
>  	lo->label = ptr;
> -	ptr += (u16_strlen(lo->label) + 1) * 2;
> +	ptr += (u16_strlen(lo->label) + 1) * sizeof(u16);
>  
>  	lo->file_path = ptr;
>  	ptr += lo->file_path_length;
> @@ -68,6 +48,48 @@ static void parse_load_option(struct load_option *lo, void *ptr)
>  	lo->optional_data = ptr;
>  }
>  
> +unsigned long efi_marshal_load_option(u32 attr, u16 *label,

... this function which really should be called "serialize" rather than
"marshal". "Marshalling" is what you do to convert from one API to
another. Here, we want to write an in-memory object to disk.

I also think the API would make more sense if you pass in a struct
efi_load_option *. It's more symmetric to the deserialize one then.

You also want to add comments to the functions to say what they do and
what their parameters are there for. That will make it easier for people
to grasp on how to use this (now public) API.

And I really dislike void * for anything that isn't "opaque". In this
function (and the one above) void * really gets used as byte pointer.
Please mark it as such (u8 *).

The next problem are the casts. If you cast from u8 * to u32 * and
dereference it, the compiler does not know if that pointer is aligned or
not. So on platforms that don't enable caches yet in the bootmgr path,
we may get unaligned exceptions. So you want to use get_unaligned_le32()
and friends or memcpy (as you did in your function) above.


Alex

> +				      struct efi_device_path *file_path,
> +				      char *option, void **data)
> +{
> +	unsigned long size;
> +	unsigned long label_len, option_len;
> +	u16 file_path_len;
> +	void *p;
> +
> +	label_len = (u16_strlen(label) + 1) * sizeof(u16);
> +	file_path_len = efi_dp_size(file_path)
> +			+ sizeof(struct efi_device_path); /* for END */
> +	option_len = strlen(option);
> +
> +	/* total size */
> +	size = sizeof(attr);
> +	size += file_path_len;
> +	size += label_len;
> +	size += option_len + 1;
> +	p = malloc(size);
> +	if (!p)
> +		return 0;
> +
> +	/* copy data */
> +	*data = p;
> +	memcpy(p, &attr, sizeof(attr));
> +	p += sizeof(attr);
> +	memcpy(p, &file_path_len, sizeof(file_path_len));
> +	p += sizeof(file_path_len);
> +	memcpy(p, label, label_len);
> +	p += label_len;
> +
> +	memcpy(p, file_path, file_path_len);
> +	p += file_path_len;
> +
> +	memcpy(p, option, option_len);
> +	p += option_len;
> +	*(char *)p = '\0';
> +
> +	return size;
> +}
> +
>  /* free() the result */
>  static void *get_var(u16 *name, const efi_guid_t *vendor,
>  		     efi_uintn_t *size)
> @@ -115,7 +137,7 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>  	if (!load_option)
>  		return NULL;
>  
> -	parse_load_option(&lo, load_option);
> +	efi_parse_load_option(&lo, load_option);
>  
>  	if (lo.attributes & LOAD_OPTION_ACTIVE) {
>  		efi_status_t ret;
> 

[U-Boot] [PATCH 6/6] efi_loader: bootmgr: run an EFI application of a given load option

[Alexander Graf]

On 17.10.18 09:32, AKASHI Takahiro wrote:
> With this patch applied, we will be able to selectively execute
> an EFI application by specifying a load option, say "-1" for Boot0001,
> "-2" for Boot0002 and so on.
> 
>   => bootefi bootmgr -1 <fdt addr>

I don't think -1 is very good user experience :). How about

  => bootefi bootmgr Boot0001 <fdt addr>


Alex

[U-Boot] [U-Boot,2/6] efi_loader: add efi_dp_from_name()

[Heinrich Schuchardt]

On 10/17/2018 09:32 AM, AKASHI Takahiro wrote:
> Factor out efi_set_bootdev() and extract efi_dp_from_name().
> This function will be used to set a boot device in efishell command.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/bootefi.c                    | 42 ++++++----------------------
>  include/efi_loader.h             |  4 +++
>  lib/efi_loader/efi_device_path.c | 47 ++++++++++++++++++++++++++++++++
>  3 files changed, 59 insertions(+), 34 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 99f5b2b95706..b3e2845120fe 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -647,45 +647,19 @@ U_BOOT_CMD(
>  
>  void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
>  {
> -	char filename[32] = { 0 }; /* dp->str is u16[32] long */
> -	char *s;
> +	struct efi_device_path *device, *image;
> +	efi_status_t ret;
>  
>  	/* efi_set_bootdev is typically called repeatedly, recover memory */
>  	efi_free_pool(bootefi_device_path);
>  	efi_free_pool(bootefi_image_path);
> -	/* If blk_get_device_part_str fails, avoid duplicate free. */
> -	bootefi_device_path = NULL;
> -	bootefi_image_path = NULL;
> -
> -	if (strcmp(dev, "Net")) {
> -		struct blk_desc *desc;
> -		disk_partition_t fs_partition;
> -		int part;
> -
> -		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
> -					       1);
> -		if (part < 0)
> -			return;
> -
> -		bootefi_device_path = efi_dp_from_part(desc, part);
> -	} else {
> -#ifdef CONFIG_NET
> -		bootefi_device_path = efi_dp_from_eth();
> -#endif
> -	}
> -
> -	if (!path)
> -		return;
>  
> -	if (strcmp(dev, "Net")) {
> -		/* Add leading / to fs paths, because they're absolute */
> -		snprintf(filename, sizeof(filename), "/%s", path);
> +	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> +	if (ret == EFI_SUCCESS) {
> +		bootefi_device_path = device;
> +		bootefi_image_path = image;
>  	} else {
> -		snprintf(filename, sizeof(filename), "%s", path);
> +		bootefi_device_path = NULL;
> +		bootefi_image_path = NULL;
>  	}
> -	/* DOS style file path: */
> -	s = filename;
> -	while ((s = strchr(s, '/')))
> -		*s++ = '\\';
> -	bootefi_image_path = efi_dp_from_file(NULL, 0, filename);
>  }
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 74df070316a0..b73fbb6de23f 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -425,6 +425,10 @@ const struct efi_device_path *efi_dp_last_node(
>  efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
>  				    struct efi_device_path **device_path,
>  				    struct efi_device_path **file_path);
> +efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
> +			      const char *path,
> +			      struct efi_device_path **device,
> +			      struct efi_device_path **file);
>  
>  #define EFI_DP_TYPE(_dp, _type, _subtype) \
>  	(((_dp)->type == DEVICE_PATH_TYPE_##_type) && \
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index 5a61a1c1dcf9..3af444147283 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -942,3 +942,50 @@ efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
>  	*file_path = fp;
>  	return EFI_SUCCESS;
>  }
> +
> +efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
> +			      const char *path,
> +			      struct efi_device_path **device,
> +			      struct efi_device_path **file)
> +{
> +	int is_net;
> +	struct blk_desc *desc = NULL;
> +	disk_partition_t fs_partition;
> +	int part = 0;
> +	char filename[32] = { 0 }; /* dp->str is u16[32] long */
> +	char *s;
> +
> +	if (!device || (path && !file))
> +		return EFI_INVALID_PARAMETER;
> +
> +	is_net = !strcmp(dev, "Net");
> +	if (!is_net) {
> +		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
> +					       1);
> +		if (part < 0)
> +			return EFI_INVALID_PARAMETER;
> +
> +		*device = efi_dp_from_part(desc, part);
> +	} else {
> +#ifdef CONFIG_NET
> +		*device = efi_dp_from_eth();
> +#endif
> +	}
> +
> +	if (!path)
> +		return EFI_SUCCESS;
> +
> +	if (!is_net) {
> +		/* Add leading / to fs paths, because they're absolute */
> +		snprintf(filename, sizeof(filename), "/%s", path);

This coding creates undefined behavior. You have to use two different
variables.

> +	} else {
> +		snprintf(filename, sizeof(filename), "%s", path);

Same here.

@Alex:
Please, remove the patch from efi-next until it is fixed.

Best regards

Heinrich

> +	}
> +	/* DOS style file path: */
> +	s = filename;
> +	while ((s = strchr(s, '/')))
> +		*s++ = '\\';
> +	*file = efi_dp_from_file(NULL, 0, filename);
> +
> +	return EFI_SUCCESS;
> +}
> 

[U-Boot] [U-Boot,2/6] efi_loader: add efi_dp_from_name()

[Heinrich Schuchardt]

On 10/17/2018 12:46 PM, Heinrich Schuchardt wrote:
> On 10/17/2018 09:32 AM, AKASHI Takahiro wrote:
>> Factor out efi_set_bootdev() and extract efi_dp_from_name().
>> This function will be used to set a boot device in efishell command.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>  cmd/bootefi.c                    | 42 ++++++----------------------
>>  include/efi_loader.h             |  4 +++
>>  lib/efi_loader/efi_device_path.c | 47 ++++++++++++++++++++++++++++++++
>>  3 files changed, 59 insertions(+), 34 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 99f5b2b95706..b3e2845120fe 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -647,45 +647,19 @@ U_BOOT_CMD(
>>  
>>  void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
>>  {
>> -	char filename[32] = { 0 }; /* dp->str is u16[32] long */
>> -	char *s;
>> +	struct efi_device_path *device, *image;
>> +	efi_status_t ret;
>>  
>>  	/* efi_set_bootdev is typically called repeatedly, recover memory */
>>  	efi_free_pool(bootefi_device_path);
>>  	efi_free_pool(bootefi_image_path);
>> -	/* If blk_get_device_part_str fails, avoid duplicate free. */
>> -	bootefi_device_path = NULL;
>> -	bootefi_image_path = NULL;
>> -
>> -	if (strcmp(dev, "Net")) {
>> -		struct blk_desc *desc;
>> -		disk_partition_t fs_partition;
>> -		int part;
>> -
>> -		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
>> -					       1);
>> -		if (part < 0)
>> -			return;
>> -
>> -		bootefi_device_path = efi_dp_from_part(desc, part);
>> -	} else {
>> -#ifdef CONFIG_NET
>> -		bootefi_device_path = efi_dp_from_eth();
>> -#endif
>> -	}
>> -
>> -	if (!path)
>> -		return;
>>  
>> -	if (strcmp(dev, "Net")) {
>> -		/* Add leading / to fs paths, because they're absolute */
>> -		snprintf(filename, sizeof(filename), "/%s", path);
>> +	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
>> +	if (ret == EFI_SUCCESS) {
>> +		bootefi_device_path = device;
>> +		bootefi_image_path = image;
>>  	} else {
>> -		snprintf(filename, sizeof(filename), "%s", path);
>> +		bootefi_device_path = NULL;
>> +		bootefi_image_path = NULL;
>>  	}
>> -	/* DOS style file path: */
>> -	s = filename;
>> -	while ((s = strchr(s, '/')))
>> -		*s++ = '\\';
>> -	bootefi_image_path = efi_dp_from_file(NULL, 0, filename);
>>  }
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 74df070316a0..b73fbb6de23f 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -425,6 +425,10 @@ const struct efi_device_path *efi_dp_last_node(
>>  efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
>>  				    struct efi_device_path **device_path,
>>  				    struct efi_device_path **file_path);
>> +efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
>> +			      const char *path,
>> +			      struct efi_device_path **device,
>> +			      struct efi_device_path **file);
>>  
>>  #define EFI_DP_TYPE(_dp, _type, _subtype) \
>>  	(((_dp)->type == DEVICE_PATH_TYPE_##_type) && \
>> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
>> index 5a61a1c1dcf9..3af444147283 100644
>> --- a/lib/efi_loader/efi_device_path.c
>> +++ b/lib/efi_loader/efi_device_path.c
>> @@ -942,3 +942,50 @@ efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
>>  	*file_path = fp;
>>  	return EFI_SUCCESS;
>>  }
>> +
>> +efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
>> +			      const char *path,
>> +			      struct efi_device_path **device,
>> +			      struct efi_device_path **file)
>> +{
>> +	int is_net;
>> +	struct blk_desc *desc = NULL;
>> +	disk_partition_t fs_partition;
>> +	int part = 0;
>> +	char filename[32] = { 0 }; /* dp->str is u16[32] long */
>> +	char *s;
>> +
>> +	if (!device || (path && !file))
>> +		return EFI_INVALID_PARAMETER;
>> +
>> +	is_net = !strcmp(dev, "Net");
>> +	if (!is_net) {
>> +		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
>> +					       1);
>> +		if (part < 0)
>> +			return EFI_INVALID_PARAMETER;
>> +
>> +		*device = efi_dp_from_part(desc, part);
>> +	} else {
>> +#ifdef CONFIG_NET
>> +		*device = efi_dp_from_eth();
>> +#endif
>> +	}
>> +
>> +	if (!path)
>> +		return EFI_SUCCESS;
>> +
>> +	if (!is_net) {
>> +		/* Add leading / to fs paths, because they're absolute */
>> +		snprintf(filename, sizeof(filename), "/%s", path);
> 
> This coding creates undefined behavior. You have to use two different
> variables.

Sorry I must be blind.

Heinrich
> 
>> +	} else {
>> +		snprintf(filename, sizeof(filename), "%s", path);
> 
> Same here.
> 
> @Alex:
> Please, remove the patch from efi-next until it is fixed.
> 
> Best regards
> 
> Heinrich
> 
>> +	}
>> +	/* DOS style file path: */
>> +	s = filename;
>> +	while ((s = strchr(s, '/')))
>> +		*s++ = '\\';
>> +	*file = efi_dp_from_file(NULL, 0, filename);
>> +
>> +	return EFI_SUCCESS;
>> +}
>>
> 
> 

[U-Boot] [PATCH 0/6] efi: make efi and bootmgr more usable

[AKASHI Takahiro]

On Wed, Oct 17, 2018 at 10:06:58AM +0200, Alexander Graf wrote:
> 
> 
> On 17.10.18 09:32, AKASHI Takahiro wrote:
> > This patch set is a collection of patches to enhance efi user interfaces
> > /commands. It will help improve user experience on efi boot and make it
> > more usable without edk2's shell utility.
> 
> That's amazing, thanks a lot :)
> 
> > Patch#1 to #4 are for efishell.
> > Patch#5 and #6 are for bootmgr.
> > 
> > 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
> 
> IMHO those 3 belong semantically to the "bootmgr" command. I can see how
> "bootefi bootmgr add 1 SHELL ..." is terrible UX. But then again it's
> not too much more to type than "efishell boot", right? ;)
> 
> So at the end of the day, these should probably be
> 
>  => bootefi bootmgr add 1 SHELL mmc 0:1 /Shell.efi ""
>  => bootefi bootmgr add 2 HELLO mmc 0:1 /hello.efi ""
>  => bootefi bootmgr dump

To be frank, I hesitate to agree with you for several reasons.
* Boot manager is a sort of boot loader application while adding/showing
  BootXXXX variables can be part of more generic system utility.
  (My interface here mimics uefi shell's bcfg command with slightly
   different syntax.)
* In future, we may want to have another sub command, "driver," to support
  driver loading, namely DriverOrder/DriverXXXX.
* Anyhow, we need another command for "setvar"( and "dumpvar").

> > 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
> 
> Same thing here :).
> 
> >  1: Boot0001: SHELL
> >  2: Boot0002: HELLO
> > 
> > => bootefi bootmgr -2
> > WARNING: booting without device tree
> > Booting: HELLO
> > ## Starting EFI application at 000000007db8b040 ...
> > Hello, world!
> > ## Application terminated, r = 0
> > 
> > => efishell setvar PlatformLang en        <--- important!
> 
> That one is slightly more complicated. How about we introduce a -e flag
> to all the env operations? Then this would become
> 
>  => setenv -e PlatformLang en
> 
> and you could print only EFI variables using
> 
>  => printenv -e
> 
> maybe one day we could then also just implement partial variable storage
> for UEFI variables:
> 
>  => saveenv -e
> 
> which we could then reuse in the ExitBootServices() call to persist EFI
> variables?

I didn't get your point. Can you please elaborate it?

> > => efishell bootmgr -1 or efishell bootmgr
> > 
> >    (shell ...)
> > 
> > # The only drawback is that it can be confusing to type
> >   "bootefi ..." and "efi(shell) boot ..." :)
> 
> Yes :).

The compromise I can imagine is that efishell be also aliased
to bootefi so that we can do:
 => efi(shell) boot add 1 ...
 => efi(shell) bootmgr -1 ( in my current syntax)
Yet we still maintain an old/boot*-style interface:
 => bootefi bootmgr or <appl addr>

Thanks,
-Takahiro Akashi

> 
> Alex

[U-Boot] [PATCH 6/6] efi_loader: bootmgr: run an EFI application of a given load option

[AKASHI Takahiro]

On Wed, Oct 17, 2018 at 10:43:22AM +0200, Alexander Graf wrote:
> 
> 
> On 17.10.18 09:32, AKASHI Takahiro wrote:
> > With this patch applied, we will be able to selectively execute
> > an EFI application by specifying a load option, say "-1" for Boot0001,
> > "-2" for Boot0002 and so on.
> > 
> >   => bootefi bootmgr -1 <fdt addr>
> 
> I don't think -1 is very good user experience :). How about
>   => bootefi bootmgr Boot0001 <fdt addr>

It looks like u-boot's run command with six more characters!
How about this:

 => bootefi bootmgr #1 <fdt addr>

or allowing "-" as empty fdt,

 => bootefi bootmgr - 1

Otherwise, a new sub command?

 => bootefi run 1, or
 => efi(shell) run 1

# Discussing UI is a fun or mess.

Thanks,
-Takahiro Akashi

> 
> Alex

[U-Boot] [PATCH 3/6] efi_loader: bootmgr: add load option helper functions

[AKASHI Takahiro]

On Wed, Oct 17, 2018 at 10:40:26AM +0200, Alexander Graf wrote:
> 
> 
> On 17.10.18 09:32, AKASHI Takahiro wrote:
> > In this patch, helper functions for an load option variable (BootXXXX)
> > are added:
> > * efi_parse_load_option(): parse a string into load_option data
> > 			   (renamed from parse_load_option and exported)
> > * efi_marshel_load_option(): convert load_option data into a string
> > 
> > Those functions will be used to implement efishell command.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  include/efi_loader.h         | 25 +++++++++++++
> >  lib/efi_loader/efi_bootmgr.c | 68 ++++++++++++++++++++++++------------
> >  2 files changed, 70 insertions(+), 23 deletions(-)
> > 
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index b73fbb6de23f..1cabb1680d20 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -502,6 +502,31 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_guid_t *vendor,
> >  				     u32 attributes, efi_uintn_t data_size,
> >  				     void *data);
> >  
> > +/*
> > + * See section 3.1.3 in the v2.7 UEFI spec for more details on
> > + * the layout of EFI_LOAD_OPTION.  In short it is:
> > + *
> > + *    typedef struct _EFI_LOAD_OPTION {
> > + *        UINT32 Attributes;
> > + *        UINT16 FilePathListLength;
> > + *        // CHAR16 Description[];   <-- variable length, NULL terminated
> > + *        // EFI_DEVICE_PATH_PROTOCOL FilePathList[];
> > + *						 <-- FilePathListLength bytes
> > + *        // UINT8 OptionalData[];
> > + *    } EFI_LOAD_OPTION;
> > + */
> > +struct load_option {
> > +	u32 attributes;
> > +	u16 file_path_length;
> > +	u16 *label;
> > +	struct efi_device_path *file_path;
> > +	u8 *optional_data;
> > +};
> 
> If this is part of the spec, shouldn't it rather be in efi_api.h?

While uefi spec mentions this structure, I don't have good confidence
that I can say that it is part of spec or API because there is no interface
(or function) that handles this structure.

> It
> probably also wants an efi_ prefix then :).

OK.

> > +
> > +void efi_parse_load_option(struct load_option *lo, void *ptr);
> > +unsigned long efi_marshal_load_option(u32 attr, u16 *label,
> > +				      struct efi_device_path *file_path,
> > +				      char *option, void **data);
> >  void *efi_bootmgr_load(struct efi_device_path **device_path,
> >  		       struct efi_device_path **file_path);
> >  
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 0c5764db127b..c2d29f956065 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -30,28 +30,8 @@ static const struct efi_runtime_services *rs;
> >   */
> >  
> >  
> > -/*
> > - * See section 3.1.3 in the v2.7 UEFI spec for more details on
> > - * the layout of EFI_LOAD_OPTION.  In short it is:
> > - *
> > - *    typedef struct _EFI_LOAD_OPTION {
> > - *        UINT32 Attributes;
> > - *        UINT16 FilePathListLength;
> > - *        // CHAR16 Description[];   <-- variable length, NULL terminated
> > - *        // EFI_DEVICE_PATH_PROTOCOL FilePathList[];  <-- FilePathListLength bytes
> > - *        // UINT8 OptionalData[];
> > - *    } EFI_LOAD_OPTION;
> > - */
> > -struct load_option {
> > -	u32 attributes;
> > -	u16 file_path_length;
> > -	u16 *label;
> > -	struct efi_device_path *file_path;
> > -	u8 *optional_data;
> > -};
> > -
> >  /* parse an EFI_LOAD_OPTION, as described above */
> > -static void parse_load_option(struct load_option *lo, void *ptr)
> > +void efi_parse_load_option(struct load_option *lo, void *ptr)
> 
> I think you want to rename this to "deserialize" to better align with ...
> 
> >  {
> >  	lo->attributes = *(u32 *)ptr;
> >  	ptr += sizeof(u32);
> > @@ -60,7 +40,7 @@ static void parse_load_option(struct load_option *lo, void *ptr)
> >  	ptr += sizeof(u16);
> >  
> >  	lo->label = ptr;
> > -	ptr += (u16_strlen(lo->label) + 1) * 2;
> > +	ptr += (u16_strlen(lo->label) + 1) * sizeof(u16);
> >  
> >  	lo->file_path = ptr;
> >  	ptr += lo->file_path_length;
> > @@ -68,6 +48,48 @@ static void parse_load_option(struct load_option *lo, void *ptr)
> >  	lo->optional_data = ptr;
> >  }
> >  
> > +unsigned long efi_marshal_load_option(u32 attr, u16 *label,
> 
> ... this function which really should be called "serialize" rather than
> "marshal". "Marshalling" is what you do to convert from one API to
> another. Here, we want to write an in-memory object to disk.

Well, as you know, I borrow this word from RPC world.
While I believe that those two are interchangeable in most contexts,
I don't care either way if you prefer 'serialize'.

> I also think the API would make more sense if you pass in a struct
> efi_load_option *. It's more symmetric to the deserialize one then.

Absolutely agree.

> You also want to add comments to the functions to say what they do and
> what their parameters are there for. That will make it easier for people
> to grasp on how to use this (now public) API.

OK

> And I really dislike void * for anything that isn't "opaque". In this
> function (and the one above) void * really gets used as byte pointer.
> Please mark it as such (u8 *).

Right, but there are quite a few of places where "void *" is used
instead of "u8 *" for a string. For instance, get_var() in bootmgr.c
or more in other files.

It's quite painful to change all the places for consistency.

> The next problem are the casts. If you cast from u8 * to u32 * and
> dereference it, the compiler does not know if that pointer is aligned or
> not. So on platforms that don't enable caches yet in the bootmgr path,
> we may get unaligned exceptions. So you want to use get_unaligned_le32()
> and friends or memcpy (as you did in your function) above.

Good point. I will fix them.
(This reveals other bugs :)

-Takahiro Akashi

> 
> Alex
> 
> > +				      struct efi_device_path *file_path,
> > +				      char *option, void **data)
> > +{
> > +	unsigned long size;
> > +	unsigned long label_len, option_len;
> > +	u16 file_path_len;
> > +	void *p;
> > +
> > +	label_len = (u16_strlen(label) + 1) * sizeof(u16);
> > +	file_path_len = efi_dp_size(file_path)
> > +			+ sizeof(struct efi_device_path); /* for END */
> > +	option_len = strlen(option);
> > +
> > +	/* total size */
> > +	size = sizeof(attr);
> > +	size += file_path_len;
> > +	size += label_len;
> > +	size += option_len + 1;
> > +	p = malloc(size);
> > +	if (!p)
> > +		return 0;
> > +
> > +	/* copy data */
> > +	*data = p;
> > +	memcpy(p, &attr, sizeof(attr));
> > +	p += sizeof(attr);
> > +	memcpy(p, &file_path_len, sizeof(file_path_len));
> > +	p += sizeof(file_path_len);
> > +	memcpy(p, label, label_len);
> > +	p += label_len;
> > +
> > +	memcpy(p, file_path, file_path_len);
> > +	p += file_path_len;
> > +
> > +	memcpy(p, option, option_len);
> > +	p += option_len;
> > +	*(char *)p = '\0';
> > +
> > +	return size;
> > +}
> > +
> >  /* free() the result */
> >  static void *get_var(u16 *name, const efi_guid_t *vendor,
> >  		     efi_uintn_t *size)
> > @@ -115,7 +137,7 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> >  	if (!load_option)
> >  		return NULL;
> >  
> > -	parse_load_option(&lo, load_option);
> > +	efi_parse_load_option(&lo, load_option);
> >  
> >  	if (lo.attributes & LOAD_OPTION_ACTIVE) {
> >  		efi_status_t ret;
> > 

[U-Boot] [PATCH 3/6] efi_loader: bootmgr: add load option helper functions

[Alexander Graf]

On 18.10.18 09:57, AKASHI Takahiro wrote:
> On Wed, Oct 17, 2018 at 10:40:26AM +0200, Alexander Graf wrote:
>>
>>
>> On 17.10.18 09:32, AKASHI Takahiro wrote:
>>> In this patch, helper functions for an load option variable (BootXXXX)
>>> are added:
>>> * efi_parse_load_option(): parse a string into load_option data
>>> 			   (renamed from parse_load_option and exported)
>>> * efi_marshel_load_option(): convert load_option data into a string
>>>
>>> Those functions will be used to implement efishell command.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  include/efi_loader.h         | 25 +++++++++++++
>>>  lib/efi_loader/efi_bootmgr.c | 68 ++++++++++++++++++++++++------------
>>>  2 files changed, 70 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index b73fbb6de23f..1cabb1680d20 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -502,6 +502,31 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_guid_t *vendor,
>>>  				     u32 attributes, efi_uintn_t data_size,
>>>  				     void *data);
>>>  
>>> +/*
>>> + * See section 3.1.3 in the v2.7 UEFI spec for more details on
>>> + * the layout of EFI_LOAD_OPTION.  In short it is:
>>> + *
>>> + *    typedef struct _EFI_LOAD_OPTION {
>>> + *        UINT32 Attributes;
>>> + *        UINT16 FilePathListLength;
>>> + *        // CHAR16 Description[];   <-- variable length, NULL terminated
>>> + *        // EFI_DEVICE_PATH_PROTOCOL FilePathList[];
>>> + *						 <-- FilePathListLength bytes
>>> + *        // UINT8 OptionalData[];
>>> + *    } EFI_LOAD_OPTION;
>>> + */
>>> +struct load_option {
>>> +	u32 attributes;
>>> +	u16 file_path_length;
>>> +	u16 *label;
>>> +	struct efi_device_path *file_path;
>>> +	u8 *optional_data;
>>> +};
>>
>> If this is part of the spec, shouldn't it rather be in efi_api.h?
> 
> While uefi spec mentions this structure, I don't have good confidence
> that I can say that it is part of spec or API because there is no interface
> (or function) that handles this structure.

Good point, it's internal only. Then efi_loader.h is probably a good fit.

> 
>> It
>> probably also wants an efi_ prefix then :).
> 
> OK.
> 
>>> +
>>> +void efi_parse_load_option(struct load_option *lo, void *ptr);
>>> +unsigned long efi_marshal_load_option(u32 attr, u16 *label,
>>> +				      struct efi_device_path *file_path,
>>> +				      char *option, void **data);
>>>  void *efi_bootmgr_load(struct efi_device_path **device_path,
>>>  		       struct efi_device_path **file_path);
>>>  
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index 0c5764db127b..c2d29f956065 100644
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -30,28 +30,8 @@ static const struct efi_runtime_services *rs;
>>>   */
>>>  
>>>  
>>> -/*
>>> - * See section 3.1.3 in the v2.7 UEFI spec for more details on
>>> - * the layout of EFI_LOAD_OPTION.  In short it is:
>>> - *
>>> - *    typedef struct _EFI_LOAD_OPTION {
>>> - *        UINT32 Attributes;
>>> - *        UINT16 FilePathListLength;
>>> - *        // CHAR16 Description[];   <-- variable length, NULL terminated
>>> - *        // EFI_DEVICE_PATH_PROTOCOL FilePathList[];  <-- FilePathListLength bytes
>>> - *        // UINT8 OptionalData[];
>>> - *    } EFI_LOAD_OPTION;
>>> - */
>>> -struct load_option {
>>> -	u32 attributes;
>>> -	u16 file_path_length;
>>> -	u16 *label;
>>> -	struct efi_device_path *file_path;
>>> -	u8 *optional_data;
>>> -};
>>> -
>>>  /* parse an EFI_LOAD_OPTION, as described above */
>>> -static void parse_load_option(struct load_option *lo, void *ptr)
>>> +void efi_parse_load_option(struct load_option *lo, void *ptr)
>>
>> I think you want to rename this to "deserialize" to better align with ...
>>
>>>  {
>>>  	lo->attributes = *(u32 *)ptr;
>>>  	ptr += sizeof(u32);
>>> @@ -60,7 +40,7 @@ static void parse_load_option(struct load_option *lo, void *ptr)
>>>  	ptr += sizeof(u16);
>>>  
>>>  	lo->label = ptr;
>>> -	ptr += (u16_strlen(lo->label) + 1) * 2;
>>> +	ptr += (u16_strlen(lo->label) + 1) * sizeof(u16);
>>>  
>>>  	lo->file_path = ptr;
>>>  	ptr += lo->file_path_length;
>>> @@ -68,6 +48,48 @@ static void parse_load_option(struct load_option *lo, void *ptr)
>>>  	lo->optional_data = ptr;
>>>  }
>>>  
>>> +unsigned long efi_marshal_load_option(u32 attr, u16 *label,
>>
>> ... this function which really should be called "serialize" rather than
>> "marshal". "Marshalling" is what you do to convert from one API to
>> another. Here, we want to write an in-memory object to disk.
> 
> Well, as you know, I borrow this word from RPC world.
> While I believe that those two are interchangeable in most contexts,
> I don't care either way if you prefer 'serialize'.

Yeah, I read up on marshalling afterwards too because I got curious and
I guess it really boils down to which world you come from. The Windows
DOM world always calls everything marshalling, while the Java world
calls it serialize usually.

Either way, I personally find serialize the more obvious name :).

> 
>> I also think the API would make more sense if you pass in a struct
>> efi_load_option *. It's more symmetric to the deserialize one then.
> 
> Absolutely agree.
> 
>> You also want to add comments to the functions to say what they do and
>> what their parameters are there for. That will make it easier for people
>> to grasp on how to use this (now public) API.
> 
> OK
> 
>> And I really dislike void * for anything that isn't "opaque". In this
>> function (and the one above) void * really gets used as byte pointer.
>> Please mark it as such (u8 *).
> 
> Right, but there are quite a few of places where "void *" is used
> instead of "u8 *" for a string. For instance, get_var() in bootmgr.c
> or more in other files.
> 
> It's quite painful to change all the places for consistency.

Yeah, let's focus on the serialize/deserialize functions for now and
just pass u8* there ;). We can tackle the others when we get to them.

Sorry to make you go through this - I should've caught that back when
Rob submitted the patches.


Alex

> 
>> The next problem are the casts. If you cast from u8 * to u32 * and
>> dereference it, the compiler does not know if that pointer is aligned or
>> not. So on platforms that don't enable caches yet in the bootmgr path,
>> we may get unaligned exceptions. So you want to use get_unaligned_le32()
>> and friends or memcpy (as you did in your function) above.
> 
> Good point. I will fix them.
> (This reveals other bugs :)
> 
> -Takahiro Akashi
> 
>>
>> Alex
>>
>>> +				      struct efi_device_path *file_path,
>>> +				      char *option, void **data)
>>> +{
>>> +	unsigned long size;
>>> +	unsigned long label_len, option_len;
>>> +	u16 file_path_len;
>>> +	void *p;
>>> +
>>> +	label_len = (u16_strlen(label) + 1) * sizeof(u16);
>>> +	file_path_len = efi_dp_size(file_path)
>>> +			+ sizeof(struct efi_device_path); /* for END */
>>> +	option_len = strlen(option);
>>> +
>>> +	/* total size */
>>> +	size = sizeof(attr);
>>> +	size += file_path_len;
>>> +	size += label_len;
>>> +	size += option_len + 1;
>>> +	p = malloc(size);
>>> +	if (!p)
>>> +		return 0;
>>> +
>>> +	/* copy data */
>>> +	*data = p;
>>> +	memcpy(p, &attr, sizeof(attr));
>>> +	p += sizeof(attr);
>>> +	memcpy(p, &file_path_len, sizeof(file_path_len));
>>> +	p += sizeof(file_path_len);
>>> +	memcpy(p, label, label_len);
>>> +	p += label_len;
>>> +
>>> +	memcpy(p, file_path, file_path_len);
>>> +	p += file_path_len;
>>> +
>>> +	memcpy(p, option, option_len);
>>> +	p += option_len;
>>> +	*(char *)p = '\0';
>>> +
>>> +	return size;
>>> +}
>>> +
>>>  /* free() the result */
>>>  static void *get_var(u16 *name, const efi_guid_t *vendor,
>>>  		     efi_uintn_t *size)
>>> @@ -115,7 +137,7 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>>>  	if (!load_option)
>>>  		return NULL;
>>>  
>>> -	parse_load_option(&lo, load_option);
>>> +	efi_parse_load_option(&lo, load_option);
>>>  
>>>  	if (lo.attributes & LOAD_OPTION_ACTIVE) {
>>>  		efi_status_t ret;
>>>

[U-Boot] [PATCH 6/6] efi_loader: bootmgr: run an EFI application of a given load option

[Alexander Graf]

On 18.10.18 07:48, AKASHI Takahiro wrote:
> On Wed, Oct 17, 2018 at 10:43:22AM +0200, Alexander Graf wrote:
>>
>>
>> On 17.10.18 09:32, AKASHI Takahiro wrote:
>>> With this patch applied, we will be able to selectively execute
>>> an EFI application by specifying a load option, say "-1" for Boot0001,
>>> "-2" for Boot0002 and so on.
>>>
>>>   => bootefi bootmgr -1 <fdt addr>
>>
>> I don't think -1 is very good user experience :). How about
>>   => bootefi bootmgr Boot0001 <fdt addr>
> 
> It looks like u-boot's run command with six more characters!
> How about this:
> 
>  => bootefi bootmgr #1 <fdt addr>

So what is the problem with making it Boot0001? That way at least the
variable name is consistent across the board ;).

> or allowing "-" as empty fdt,
> 
>  => bootefi bootmgr - 1
> 
> Otherwise, a new sub command?
> 
>  => bootefi run 1, or
>  => efi(shell) run 1
> 
> # Discussing UI is a fun or mess.

Yeah :(. What we really need would be that "bootefi bootmgr" becomes
"efiboot". But that would be even more confusing ;).

So the whole rationale of why "bootefi" is the way it is today is that
it's trying to lean on the existing "bootm", "booti", "bootz" etc syntax
as much as it can. In other words, it's trying to fit into the U-Boot
ecosystem much rather than the existing edk2 one.

I would like to keep following that path going forward. Whenever there
is an option between "U-Boot like" and "edk2 like" I would always opt
for the "U-Boot like" user experience, because if they want edk2 they
may as well use edk2 ;).


Alex

[U-Boot] [PATCH 0/6] efi: make efi and bootmgr more usable

[Alexander Graf]

On 18.10.18 07:24, AKASHI Takahiro wrote:
> On Wed, Oct 17, 2018 at 10:06:58AM +0200, Alexander Graf wrote:
>>
>>
>> On 17.10.18 09:32, AKASHI Takahiro wrote:
>>> This patch set is a collection of patches to enhance efi user interfaces
>>> /commands. It will help improve user experience on efi boot and make it
>>> more usable without edk2's shell utility.
>>
>> That's amazing, thanks a lot :)
>>
>>> Patch#1 to #4 are for efishell.
>>> Patch#5 and #6 are for bootmgr.
>>>
>>> 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
>>
>> IMHO those 3 belong semantically to the "bootmgr" command. I can see how
>> "bootefi bootmgr add 1 SHELL ..." is terrible UX. But then again it's
>> not too much more to type than "efishell boot", right? ;)
>>
>> So at the end of the day, these should probably be
>>
>>  => bootefi bootmgr add 1 SHELL mmc 0:1 /Shell.efi ""
>>  => bootefi bootmgr add 2 HELLO mmc 0:1 /hello.efi ""
>>  => bootefi bootmgr dump
> 
> To be frank, I hesitate to agree with you for several reasons.
> * Boot manager is a sort of boot loader application while adding/showing
>   BootXXXX variables can be part of more generic system utility.
>   (My interface here mimics uefi shell's bcfg command with slightly
>    different syntax.)

If you look at it from a U-Boot perspective, the only consumer of
BootXXXX variables will realistically be bootmgr. So I don't quite see
how setting BootXXXX variables doesn't belong to the same command line
syntax?

> * In future, we may want to have another sub command, "driver," to support
>   driver loading, namely DriverOrder/DriverXXXX.

Hm. Who would read that order? When would it get parsed? Would that be
part of the script (and thus a command again) or would we want to have
them get loaded at an earlier stage?

My current stance would be that driver loading would happen very similar
to application loading: Using a command from a script / the shell. So
the same command could again be responsible for changing the order and
adding/removing driver entries.

> * Anyhow, we need another command for "setvar"( and "dumpvar").

I would much much rather like to integrate with existing U-Boot commands
(setenv, printenv, print) than invent new commands. The UEFI bits in
U-Boot shouldn't be alien - they should just seamlessly integrate :).

> 
>>> 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
>>
>> Same thing here :).
>>
>>>  1: Boot0001: SHELL
>>>  2: Boot0002: HELLO
>>>
>>> => bootefi bootmgr -2
>>> WARNING: booting without device tree
>>> Booting: HELLO
>>> ## Starting EFI application at 000000007db8b040 ...
>>> Hello, world!
>>> ## Application terminated, r = 0
>>>
>>> => efishell setvar PlatformLang en        <--- important!
>>
>> That one is slightly more complicated. How about we introduce a -e flag
>> to all the env operations? Then this would become
>>
>>  => setenv -e PlatformLang en
>>
>> and you could print only EFI variables using
>>
>>  => printenv -e
>>
>> maybe one day we could then also just implement partial variable storage
>> for UEFI variables:
>>
>>  => saveenv -e
>>
>> which we could then reuse in the ExitBootServices() call to persist EFI
>> variables?
> 
> I didn't get your point. Can you please elaborate it?

Currently we have:

=> help printenv
printenv - print environment variables

Usage:
printenv [-a]
    - print [all] values of all environment variables
printenv name ...
    - print value of environment variable 'name'
=> help setenv
setenv - set environment variables

Usage:
setenv [-f] name value ...
    - [forcibly] set environment variable 'name' to 'value ...'
setenv [-f] name
    - [forcibly] delete environment variable 'name'


What if we add UEFI variable awareness to those commands? So instead of

  => efishell setvar PlatformLang en

you would get

  => setenv -e PlatformLang en

The same would apply for printenv. Something like

  => printenv -e

would just show UEFI variables with potential additional decoding?


Or was your question on saveenv? The saveenv command is used to make the
variable storage persistent. One problem we have with variable storage
is that you want to only persist variables that are

  a) marked to persist
  b) UEFI variables

So for example if you're in the middle of a script and you want to
change the boot order, you want to say "please make the UEFI variable
store persistent now", but you definitely do now want to make the U-Boot
variable store persistent, because your variables are in the middle of
your script execution with helper variables.

That same problem also occurs with booting. When calling
ExitBootServices() we ideally want to persist the UEFI variable storage
(well, the non-temporary variables in it). But for that we currently
don't have any logic implemented to differentiate between UEFI variables
and U-Boot variables on save.

> 
>>> => efishell bootmgr -1 or efishell bootmgr
>>>
>>>    (shell ...)
>>>
>>> # The only drawback is that it can be confusing to type
>>>   "bootefi ..." and "efi(shell) boot ..." :)
>>
>> Yes :).
> 
> The compromise I can imagine is that efishell be also aliased
> to bootefi so that we can do:
>  => efi(shell) boot add 1 ...
>  => efi(shell) bootmgr -1 ( in my current syntax)
> Yet we still maintain an old/boot*-style interface:
>  => bootefi bootmgr or <appl addr>

In a nutshell, I want to have UEFI commands be first class U-Boot
commands :). The whole fact that we have "bootefi bootmgr" is bad. It
shouldn't have been a bootefi subcommand really :/.

Now the really big question is what an alternative *good* naming scheme
is. And there I'm not sure yet...


Alex

[U-Boot] [PATCH 6/6] efi_loader: bootmgr: run an EFI application of a given load option

[AKASHI Takahiro]

On Thu, Oct 18, 2018 at 10:46:36AM +0200, Alexander Graf wrote:
> 
> 
> On 18.10.18 07:48, AKASHI Takahiro wrote:
> > On Wed, Oct 17, 2018 at 10:43:22AM +0200, Alexander Graf wrote:
> >>
> >>
> >> On 17.10.18 09:32, AKASHI Takahiro wrote:
> >>> With this patch applied, we will be able to selectively execute
> >>> an EFI application by specifying a load option, say "-1" for Boot0001,
> >>> "-2" for Boot0002 and so on.
> >>>
> >>>   => bootefi bootmgr -1 <fdt addr>
> >>
> >> I don't think -1 is very good user experience :). How about
> >>   => bootefi bootmgr Boot0001 <fdt addr>
> > 
> > It looks like u-boot's run command with six more characters!
> > How about this:
> > 
> >  => bootefi bootmgr #1 <fdt addr>
> 
> So what is the problem with making it Boot0001? That way at least the
> variable name is consistent across the board ;).

More typing!

> > or allowing "-" as empty fdt,
> > 
> >  => bootefi bootmgr - 1

(Please notice that this is NOT "-1.")
I also like this one as it maintains upward-compatibility:
    bootefi bootmgr [<fdt addr> [<boot id>]]

> > Otherwise, a new sub command?
> > 
> >  => bootefi run 1, or
> >  => efi(shell) run 1

Well, if you stick to "setenv -e"-like syntax, how about
    => run -e Boot0001

Given that "run" takes an environment variable, this syntax
is perfectly fit to U-boot, isn't it?

> > # Discussing UI is a fun or mess.

# I hope that this is not fruitless discussion.

> Yeah :(. What we really need would be that "bootefi bootmgr" becomes
> "efiboot". But that would be even more confusing ;).

So I think that we should not add anything more to "bootefi bootmgr"
to extend functionality.

> So the whole rationale of why "bootefi" is the way it is today is that
> it's trying to lean on the existing "bootm", "booti", "bootz" etc syntax
> as much as it can. In other words, it's trying to fit into the U-Boot
> ecosystem much rather than the existing edk2 one.

IMO, "boot*" variants are already a mess.

> I would like to keep following that path going forward. Whenever there
> is an option between "U-Boot like" and "edk2 like" I would always opt
> for the "U-Boot like" user experience, because if they want edk2 they
> may as well use edk2 ;).

Well, BootXXXX is quite UEFI-specific and people who are not familiar
with UEFI need to consult UEFI specification anyway and this means, to me,
that UEFI shell's similarity would be favorable.
(See "setvar" syntax, not mine but UEFI shell's, which can be
quite different and complicated.)

Does anybody else have any opinions?

Thanks,
-Takahiro Akashi

> 
> Alex

[U-Boot] [PATCH 3/6] efi_loader: bootmgr: add load option helper functions

[AKASHI Takahiro]

On Thu, Oct 18, 2018 at 10:39:30AM +0200, Alexander Graf wrote:
> 
> 
> On 18.10.18 09:57, AKASHI Takahiro wrote:
> > On Wed, Oct 17, 2018 at 10:40:26AM +0200, Alexander Graf wrote:
> >>
> >>
> >> On 17.10.18 09:32, AKASHI Takahiro wrote:
> >>> In this patch, helper functions for an load option variable (BootXXXX)
> >>> are added:
> >>> * efi_parse_load_option(): parse a string into load_option data
> >>> 			   (renamed from parse_load_option and exported)
> >>> * efi_marshel_load_option(): convert load_option data into a string
> >>>
> >>> Those functions will be used to implement efishell command.
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> ---
> >>>  include/efi_loader.h         | 25 +++++++++++++
> >>>  lib/efi_loader/efi_bootmgr.c | 68 ++++++++++++++++++++++++------------
> >>>  2 files changed, 70 insertions(+), 23 deletions(-)
> >>>
> >>> diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>> index b73fbb6de23f..1cabb1680d20 100644
> >>> --- a/include/efi_loader.h
> >>> +++ b/include/efi_loader.h
> >>> @@ -502,6 +502,31 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_guid_t *vendor,
> >>>  				     u32 attributes, efi_uintn_t data_size,
> >>>  				     void *data);
> >>>  
> >>> +/*
> >>> + * See section 3.1.3 in the v2.7 UEFI spec for more details on
> >>> + * the layout of EFI_LOAD_OPTION.  In short it is:
> >>> + *
> >>> + *    typedef struct _EFI_LOAD_OPTION {
> >>> + *        UINT32 Attributes;
> >>> + *        UINT16 FilePathListLength;
> >>> + *        // CHAR16 Description[];   <-- variable length, NULL terminated
> >>> + *        // EFI_DEVICE_PATH_PROTOCOL FilePathList[];
> >>> + *						 <-- FilePathListLength bytes
> >>> + *        // UINT8 OptionalData[];
> >>> + *    } EFI_LOAD_OPTION;
> >>> + */
> >>> +struct load_option {
> >>> +	u32 attributes;
> >>> +	u16 file_path_length;
> >>> +	u16 *label;
> >>> +	struct efi_device_path *file_path;
> >>> +	u8 *optional_data;
> >>> +};
> >>
> >> If this is part of the spec, shouldn't it rather be in efi_api.h?
> > 
> > While uefi spec mentions this structure, I don't have good confidence
> > that I can say that it is part of spec or API because there is no interface
> > (or function) that handles this structure.
> 
> Good point, it's internal only. Then efi_loader.h is probably a good fit.

OK.

> > 
> >> It
> >> probably also wants an efi_ prefix then :).
> > 
> > OK.
> > 
> >>> +
> >>> +void efi_parse_load_option(struct load_option *lo, void *ptr);
> >>> +unsigned long efi_marshal_load_option(u32 attr, u16 *label,
> >>> +				      struct efi_device_path *file_path,
> >>> +				      char *option, void **data);
> >>>  void *efi_bootmgr_load(struct efi_device_path **device_path,
> >>>  		       struct efi_device_path **file_path);
> >>>  
> >>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >>> index 0c5764db127b..c2d29f956065 100644
> >>> --- a/lib/efi_loader/efi_bootmgr.c
> >>> +++ b/lib/efi_loader/efi_bootmgr.c
> >>> @@ -30,28 +30,8 @@ static const struct efi_runtime_services *rs;
> >>>   */
> >>>  
> >>>  
> >>> -/*
> >>> - * See section 3.1.3 in the v2.7 UEFI spec for more details on
> >>> - * the layout of EFI_LOAD_OPTION.  In short it is:
> >>> - *
> >>> - *    typedef struct _EFI_LOAD_OPTION {
> >>> - *        UINT32 Attributes;
> >>> - *        UINT16 FilePathListLength;
> >>> - *        // CHAR16 Description[];   <-- variable length, NULL terminated
> >>> - *        // EFI_DEVICE_PATH_PROTOCOL FilePathList[];  <-- FilePathListLength bytes
> >>> - *        // UINT8 OptionalData[];
> >>> - *    } EFI_LOAD_OPTION;
> >>> - */
> >>> -struct load_option {
> >>> -	u32 attributes;
> >>> -	u16 file_path_length;
> >>> -	u16 *label;
> >>> -	struct efi_device_path *file_path;
> >>> -	u8 *optional_data;
> >>> -};
> >>> -
> >>>  /* parse an EFI_LOAD_OPTION, as described above */
> >>> -static void parse_load_option(struct load_option *lo, void *ptr)
> >>> +void efi_parse_load_option(struct load_option *lo, void *ptr)
> >>
> >> I think you want to rename this to "deserialize" to better align with ...
> >>
> >>>  {
> >>>  	lo->attributes = *(u32 *)ptr;
> >>>  	ptr += sizeof(u32);
> >>> @@ -60,7 +40,7 @@ static void parse_load_option(struct load_option *lo, void *ptr)
> >>>  	ptr += sizeof(u16);
> >>>  
> >>>  	lo->label = ptr;
> >>> -	ptr += (u16_strlen(lo->label) + 1) * 2;
> >>> +	ptr += (u16_strlen(lo->label) + 1) * sizeof(u16);
> >>>  
> >>>  	lo->file_path = ptr;
> >>>  	ptr += lo->file_path_length;
> >>> @@ -68,6 +48,48 @@ static void parse_load_option(struct load_option *lo, void *ptr)
> >>>  	lo->optional_data = ptr;
> >>>  }
> >>>  
> >>> +unsigned long efi_marshal_load_option(u32 attr, u16 *label,
> >>
> >> ... this function which really should be called "serialize" rather than
> >> "marshal". "Marshalling" is what you do to convert from one API to
> >> another. Here, we want to write an in-memory object to disk.
> > 
> > Well, as you know, I borrow this word from RPC world.
> > While I believe that those two are interchangeable in most contexts,
> > I don't care either way if you prefer 'serialize'.
> 
> Yeah, I read up on marshalling afterwards too because I got curious and
> I guess it really boils down to which world you come from. The Windows
> DOM world always calls everything marshalling, while the Java world
> calls it serialize usually.
> 
> Either way, I personally find serialize the more obvious name :).

OK.

> > 
> >> I also think the API would make more sense if you pass in a struct
> >> efi_load_option *. It's more symmetric to the deserialize one then.
> > 
> > Absolutely agree.
> > 
> >> You also want to add comments to the functions to say what they do and
> >> what their parameters are there for. That will make it easier for people
> >> to grasp on how to use this (now public) API.
> > 
> > OK
> > 
> >> And I really dislike void * for anything that isn't "opaque". In this
> >> function (and the one above) void * really gets used as byte pointer.
> >> Please mark it as such (u8 *).
> > 
> > Right, but there are quite a few of places where "void *" is used
> > instead of "u8 *" for a string. For instance, get_var() in bootmgr.c
> > or more in other files.
> > 
> > It's quite painful to change all the places for consistency.
> 
> Yeah, let's focus on the serialize/deserialize functions for now and
> just pass u8* there ;). We can tackle the others when we get to them.

Sure.

-Takahiro Akashi

> Sorry to make you go through this - I should've caught that back when
> Rob submitted the patches.
> 
> 
> Alex
> 
> > 
> >> The next problem are the casts. If you cast from u8 * to u32 * and
> >> dereference it, the compiler does not know if that pointer is aligned or
> >> not. So on platforms that don't enable caches yet in the bootmgr path,
> >> we may get unaligned exceptions. So you want to use get_unaligned_le32()
> >> and friends or memcpy (as you did in your function) above.
> > 
> > Good point. I will fix them.
> > (This reveals other bugs :)
> > 
> > -Takahiro Akashi
> > 
> >>
> >> Alex
> >>
> >>> +				      struct efi_device_path *file_path,
> >>> +				      char *option, void **data)
> >>> +{
> >>> +	unsigned long size;
> >>> +	unsigned long label_len, option_len;
> >>> +	u16 file_path_len;
> >>> +	void *p;
> >>> +
> >>> +	label_len = (u16_strlen(label) + 1) * sizeof(u16);
> >>> +	file_path_len = efi_dp_size(file_path)
> >>> +			+ sizeof(struct efi_device_path); /* for END */
> >>> +	option_len = strlen(option);
> >>> +
> >>> +	/* total size */
> >>> +	size = sizeof(attr);
> >>> +	size += file_path_len;
> >>> +	size += label_len;
> >>> +	size += option_len + 1;
> >>> +	p = malloc(size);
> >>> +	if (!p)
> >>> +		return 0;
> >>> +
> >>> +	/* copy data */
> >>> +	*data = p;
> >>> +	memcpy(p, &attr, sizeof(attr));
> >>> +	p += sizeof(attr);
> >>> +	memcpy(p, &file_path_len, sizeof(file_path_len));
> >>> +	p += sizeof(file_path_len);
> >>> +	memcpy(p, label, label_len);
> >>> +	p += label_len;
> >>> +
> >>> +	memcpy(p, file_path, file_path_len);
> >>> +	p += file_path_len;
> >>> +
> >>> +	memcpy(p, option, option_len);
> >>> +	p += option_len;
> >>> +	*(char *)p = '\0';
> >>> +
> >>> +	return size;
> >>> +}
> >>> +
> >>>  /* free() the result */
> >>>  static void *get_var(u16 *name, const efi_guid_t *vendor,
> >>>  		     efi_uintn_t *size)
> >>> @@ -115,7 +137,7 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> >>>  	if (!load_option)
> >>>  		return NULL;
> >>>  
> >>> -	parse_load_option(&lo, load_option);
> >>> +	efi_parse_load_option(&lo, load_option);
> >>>  
> >>>  	if (lo.attributes & LOAD_OPTION_ACTIVE) {
> >>>  		efi_status_t ret;
> >>>

[U-Boot] [PATCH 6/6] efi_loader: bootmgr: run an EFI application of a given load option

[Alexander Graf]

On 22.10.18 06:37, AKASHI Takahiro wrote:
> On Thu, Oct 18, 2018 at 10:46:36AM +0200, Alexander Graf wrote:
>>
>>
>> On 18.10.18 07:48, AKASHI Takahiro wrote:
>>> On Wed, Oct 17, 2018 at 10:43:22AM +0200, Alexander Graf wrote:
>>>>
>>>>
>>>> On 17.10.18 09:32, AKASHI Takahiro wrote:
>>>>> With this patch applied, we will be able to selectively execute
>>>>> an EFI application by specifying a load option, say "-1" for Boot0001,
>>>>> "-2" for Boot0002 and so on.
>>>>>
>>>>>   => bootefi bootmgr -1 <fdt addr>
>>>>
>>>> I don't think -1 is very good user experience :). How about
>>>>   => bootefi bootmgr Boot0001 <fdt addr>
>>>
>>> It looks like u-boot's run command with six more characters!
>>> How about this:
>>>
>>>  => bootefi bootmgr #1 <fdt addr>
>>
>> So what is the problem with making it Boot0001? That way at least the
>> variable name is consistent across the board ;).
> 
> More typing!
> 
>>> or allowing "-" as empty fdt,
>>>
>>>  => bootefi bootmgr - 1
> 
> (Please notice that this is NOT "-1.")
> I also like this one as it maintains upward-compatibility:
>     bootefi bootmgr [<fdt addr> [<boot id>]]
> 
>>> Otherwise, a new sub command?
>>>
>>>  => bootefi run 1, or
>>>  => efi(shell) run 1
> 
> Well, if you stick to "setenv -e"-like syntax, how about
>     => run -e Boot0001
> 
> Given that "run" takes an environment variable, this syntax
> is perfectly fit to U-boot, isn't it?

Ooooh, that is an amazing suggestion! And "run -e" without an explicit
target could just invoke efibootmgr directly, looping through the BootOrder.

> 
>>> # Discussing UI is a fun or mess.
> 
> # I hope that this is not fruitless discussion.
> 
>> Yeah :(. What we really need would be that "bootefi bootmgr" becomes
>> "efiboot". But that would be even more confusing ;).
> 
> So I think that we should not add anything more to "bootefi bootmgr"
> to extend functionality.
> 
>> So the whole rationale of why "bootefi" is the way it is today is that
>> it's trying to lean on the existing "bootm", "booti", "bootz" etc syntax
>> as much as it can. In other words, it's trying to fit into the U-Boot
>> ecosystem much rather than the existing edk2 one.
> 
> IMO, "boot*" variants are already a mess.
> 
>> I would like to keep following that path going forward. Whenever there
>> is an option between "U-Boot like" and "edk2 like" I would always opt
>> for the "U-Boot like" user experience, because if they want edk2 they
>> may as well use edk2 ;).
> 
> Well, BootXXXX is quite UEFI-specific and people who are not familiar
> with UEFI need to consult UEFI specification anyway and this means, to me,
> that UEFI shell's similarity would be favorable.
> (See "setvar" syntax, not mine but UEFI shell's, which can be
> quite different and complicated.)

Well my thinking there is that if someone likes the UEFI Shell, they may
as well just run it :).


Alex

> 
> Does anybody else have any opinions?
> 
> Thanks,
> -Takahiro Akashi
> 
>>
>> Alex

[U-Boot] [PATCH 6/6] efi_loader: bootmgr: run an EFI application of a given load option

[AKASHI Takahiro]

On Mon, Oct 22, 2018 at 07:58:29AM +0100, Alexander Graf wrote:
> 
> 
> On 22.10.18 06:37, AKASHI Takahiro wrote:
> > On Thu, Oct 18, 2018 at 10:46:36AM +0200, Alexander Graf wrote:
> >>
> >>
> >> On 18.10.18 07:48, AKASHI Takahiro wrote:
> >>> On Wed, Oct 17, 2018 at 10:43:22AM +0200, Alexander Graf wrote:
> >>>>
> >>>>
> >>>> On 17.10.18 09:32, AKASHI Takahiro wrote:
> >>>>> With this patch applied, we will be able to selectively execute
> >>>>> an EFI application by specifying a load option, say "-1" for Boot0001,
> >>>>> "-2" for Boot0002 and so on.
> >>>>>
> >>>>>   => bootefi bootmgr -1 <fdt addr>
> >>>>
> >>>> I don't think -1 is very good user experience :). How about
> >>>>   => bootefi bootmgr Boot0001 <fdt addr>
> >>>
> >>> It looks like u-boot's run command with six more characters!
> >>> How about this:
> >>>
> >>>  => bootefi bootmgr #1 <fdt addr>
> >>
> >> So what is the problem with making it Boot0001? That way at least the
> >> variable name is consistent across the board ;).
> > 
> > More typing!
> > 
> >>> or allowing "-" as empty fdt,
> >>>
> >>>  => bootefi bootmgr - 1
> > 
> > (Please notice that this is NOT "-1.")
> > I also like this one as it maintains upward-compatibility:
> >     bootefi bootmgr [<fdt addr> [<boot id>]]
> > 
> >>> Otherwise, a new sub command?
> >>>
> >>>  => bootefi run 1, or
> >>>  => efi(shell) run 1
> > 
> > Well, if you stick to "setenv -e"-like syntax, how about
> >     => run -e Boot0001
> > 
> > Given that "run" takes an environment variable, this syntax
> > is perfectly fit to U-boot, isn't it?
> 
> Ooooh, that is an amazing suggestion! And "run -e" without an explicit
> target could just invoke efibootmgr directly, looping through the BootOrder.

We agree here :)

> > 
> >>> # Discussing UI is a fun or mess.
> > 
> > # I hope that this is not fruitless discussion.
> > 
> >> Yeah :(. What we really need would be that "bootefi bootmgr" becomes
> >> "efiboot". But that would be even more confusing ;).
> > 
> > So I think that we should not add anything more to "bootefi bootmgr"
> > to extend functionality.
> > 
> >> So the whole rationale of why "bootefi" is the way it is today is that
> >> it's trying to lean on the existing "bootm", "booti", "bootz" etc syntax
> >> as much as it can. In other words, it's trying to fit into the U-Boot
> >> ecosystem much rather than the existing edk2 one.
> > 
> > IMO, "boot*" variants are already a mess.
> > 
> >> I would like to keep following that path going forward. Whenever there
> >> is an option between "U-Boot like" and "edk2 like" I would always opt
> >> for the "U-Boot like" user experience, because if they want edk2 they
> >> may as well use edk2 ;).
> > 
> > Well, BootXXXX is quite UEFI-specific and people who are not familiar
> > with UEFI need to consult UEFI specification anyway and this means, to me,
> > that UEFI shell's similarity would be favorable.
> > (See "setvar" syntax, not mine but UEFI shell's, which can be
> > quite different and complicated.)
> 
> Well my thinking there is that if someone likes the UEFI Shell, they may
> as well just run it :).

My aim here is to provide poor man's uefi shell!
For example, I think there are a few useful commands to be supported:
* devices
* devtree
* dh
* (dis)connect
* drivers
* memmap
* unload
They must be useful even now for debugging and understanding
internal status of UEFI environment.
# Please note that some of those commands in edk2's shell
  will still cause a panic.

That's is why I want to have efi(shell) command.

Thanks,
-Takahiro Akashi

> 
> Alex
> 
> > 
> > Does anybody else have any opinions?
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> >>
> >> Alex