[U-Boot] [PATCH v2 00/11] efi_loader: non-volatile variables support

[U-Boot] [PATCH v2 00/11] efi_loader: non-volatile variables support

[AKASHI Takahiro]

This patch set is an attempt to implement non-volatile attribute for
UEFI variables. Under the current implementation,
* SetVariable API doesn't recognize non-volatile attribute
* While some variables are defined non-volatile in UEFI specification,
  they are NOT marked as non-volatile in the code.
* env_save() (or "env save" command) allows us to save all the variables
  into persistent storage, but it may cause volatile UEFI variables,
  along with irrelevant U-Boot variables, to be saved unconditionally.

Those observation rationalizes that the implementation of UEFI variables
should be revamped utilizing dedicated storage for them.

This patch set is yet experimental and rough-edged(See known issues below),
but shows how UEFI variables can be split from U-Boot environment.
This enhancement will also be vital when we introduce UEFI secure boot
where secure and tamper-resistant storage (with authentication) is
required.

Usage:
To enable this feature, the following configs must be enabled:
  CONFIG_ENV_IS_IN_FAT
  CONFIG_ENV_FAT_INTERFACE
  CONFIG_ENV_EFI_FAT_DEVICE_AND_PART
  CONFIG_ENV_EFI_FAT_FILE

You can also define a non-volatile variable from command interface:
=> setenv -e -nv FOO baa

Known issues/restriction:
* UEFI spec defines "globally defined variables" with specific
  attributes, but with this patch, we don't check against the user-supplied
  attribute for any variable.
* Only FAT can be enabled for persistent storage for UEFI non-volatile
  variables.
* The whole area of storage will be saved at every update of one variable.
  It can be optimized.
* An error during saving may cause inconsistency between cache (hash table)
  and the storage.
* Cache is of fixed size and can be quite big for normal usage.

Patch#1 to #4 are preparatory so that we won't rely on U-Boot environment,
that is, env_get/set() helper functions.
Patch#5 to #8 are core part of changes.
Patch#9 to #11 are for modifying variable attributes.

Changes in v2 (Apr 24, 2019)
* rebased on efi-2019-07
* revamp the implementation

v1 (Nov 28, 2018)
* initial

AKASHI Takahiro (11):
  lib: charset: add u16_strcmp()
  lib: charset: add u16_strncmp()
  cmd: efidebug: rework "boot dump" sub-command using
    GetNextVariableName()
  efi_loader: set OsIndicationsSupported at init
  env: save UEFI non-volatile variables in dedicated storage
  efi_loader: variable: support non-volatile attribute
  efi_loader: variable: split UEFI variables from U-Boot environment
  efi_loader: load saved non-volatile variables at init
  efi_loader: bootmgr: handle BootNext as non-volatile
  cmd: env: add -nv option for UEFI non-volatile variable
  cmd: efidebug: make some boot variables non-volatile

 cmd/bootefi.c                     |   4 -
 cmd/efidebug.c                    |  95 +++++++++++-----
 cmd/nvedit.c                      |   3 +-
 cmd/nvedit_efi.c                  |  15 ++-
 env/Kconfig                       |  34 ++++++
 env/env.c                         |  98 ++++++++++++++++-
 env/fat.c                         | 109 +++++++++++++++++++
 include/asm-generic/global_data.h |   1 +
 include/charset.h                 |  10 ++
 include/environment.h             |  24 +++++
 lib/charset.c                     |  23 ++++
 lib/efi_loader/efi_bootmgr.c      |   3 +-
 lib/efi_loader/efi_setup.c        |  13 +++
 lib/efi_loader/efi_variable.c     | 174 ++++++++++++++++++++++++++++--
 14 files changed, 560 insertions(+), 46 deletions(-)

-- 
2.20.1

[U-Boot] [PATCH v2 01/11] lib: charset: add u16_strcmp()

[AKASHI Takahiro]

u16 version of strcmp()

AUTHER: Patrick Wildt <patrick@blueri.se>
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/charset.h |  5 +++++
 lib/charset.c     | 10 ++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/charset.h b/include/charset.h
index 65087f76d1fc..747a9b376c03 100644
--- a/include/charset.h
+++ b/include/charset.h
@@ -166,6 +166,11 @@ s32 utf_to_lower(const s32 code);
  */
 s32 utf_to_upper(const s32 code);
 
+/*
+ * u16_strcmp() - strcmp() for u16 strings
+ */
+int u16_strcmp(const u16 *s1, const u16 *s2);
+
 /**
  * u16_strlen - count non-zero words
  *
diff --git a/lib/charset.c b/lib/charset.c
index 5e349ed5ee45..4a25ac0bdb9c 100644
--- a/lib/charset.c
+++ b/lib/charset.c
@@ -335,6 +335,16 @@ s32 utf_to_upper(const s32 code)
 	return ret;
 }
 
+int u16_strcmp(const u16 *s1, const u16 *s2)
+{
+	while (*s1 == *s2++)
+		if (*s1++ == 0)
+			return (0);
+	--s2;
+
+	return (*(uint16_t *)s1 - *(uint16_t *)s2);
+}
+
 size_t u16_strlen(const u16 *in)
 {
 	size_t i;
-- 
2.20.1

[U-Boot] [PATCH v2 02/11] lib: charset: add u16_strncmp()

[AKASHI Takahiro]

u16_strncmp() works like u16_strcmp() but only at most n characters
(in u16) are compared.
This function will be used in a later patch.

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

diff --git a/include/charset.h b/include/charset.h
index 747a9b376c03..49842a88bc8b 100644
--- a/include/charset.h
+++ b/include/charset.h
@@ -171,6 +171,11 @@ s32 utf_to_upper(const s32 code);
  */
 int u16_strcmp(const u16 *s1, const u16 *s2);
 
+/*
+ * u16_strncmp() - strncmp() for u16 strings
+ */
+int u16_strncmp(const u16 *s1, const u16 *s2, size_t n);
+
 /**
  * u16_strlen - count non-zero words
  *
diff --git a/lib/charset.c b/lib/charset.c
index 4a25ac0bdb9c..85f08db68fe2 100644
--- a/lib/charset.c
+++ b/lib/charset.c
@@ -345,6 +345,19 @@ int u16_strcmp(const u16 *s1, const u16 *s2)
 	return (*(uint16_t *)s1 - *(uint16_t *)s2);
 }
 
+int u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
+{
+	while ((n-- > 0) && (*s1 == *s2++)) {
+		if (*s1++ == 0)
+			return 0;
+		if (!n)
+			return 0;
+	}
+	--s2;
+
+	return (*(uint16_t *)s1 - *(uint16_t *)s2);
+}
+
 size_t u16_strlen(const u16 *in)
 {
 	size_t i;
-- 
2.20.1

[U-Boot] [PATCH v2 03/11] cmd: efidebug: rework "boot dump" sub-command using GetNextVariableName()

[AKASHI Takahiro]

Efidebug command should be implemented using well-defined EFI interfaces,
rather than using internal functions/data. This change will be needed in
a later patch where UEFI variables are re-implemented.

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

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index a40c4f4be286..8890dd7268f1 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -509,7 +509,7 @@ static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
 	if (argc < 6 || argc > 7)
 		return CMD_RET_USAGE;
 
-	id = (int)simple_strtoul(argv[1], &endp, 16);
+	id = simple_strtoul(argv[1], &endp, 16);
 	if (*endp != '\0' || id > 0xffff)
 		return CMD_RET_USAGE;
 
@@ -595,7 +595,7 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag,
 
 	guid = efi_global_variable_guid;
 	for (i = 1; i < argc; i++, argv++) {
-		id = (int)simple_strtoul(argv[1], &endp, 16);
+		id = simple_strtoul(argv[1], &endp, 16);
 		if (*endp != '\0' || id > 0xffff)
 			return CMD_RET_FAILURE;
 
@@ -693,6 +693,27 @@ static void show_efi_boot_opt(int id)
 	free(data);
 }
 
+static bool u16_isxdigit(u16 c)
+{
+	if (c & 0xff00)
+		return false;
+
+	return isxdigit((u8)c);
+}
+
+static int u16_tohex(u16 c)
+{
+	if (c >= '0' && c < '9')
+		return c - '0';
+	if (c >= 'A' && c < 'F')
+		return c - 'A' + 10;
+	if (c >= 'a' && c < 'f')
+		return c - 'a' + 10;
+
+	/* dummy */
+	return -1;
+}
+
 /**
  * show_efi_boot_dump() - dump all UEFI load options
  *
@@ -709,38 +730,57 @@ static void show_efi_boot_opt(int id)
 static int do_efi_boot_dump(cmd_tbl_t *cmdtp, int flag,
 			    int argc, char * const argv[])
 {
-	char regex[256];
-	char * const regexlist[] = {regex};
-	char *variables = NULL, *boot, *value;
-	int len;
-	int id;
+	u16 *var_name16, *p;
+	efi_uintn_t buf_size, size;
+	efi_guid_t guid;
+	int id, i;
+	efi_status_t ret;
 
 	if (argc > 1)
 		return CMD_RET_USAGE;
 
-	snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_Boot[0-9A-F]+");
-
-	/* TODO: use GetNextVariableName? */
-	len = hexport_r(&env_htab, '\n', H_MATCH_REGEX | H_MATCH_KEY,
-			&variables, 0, 1, regexlist);
+	buf_size = 128;
+	var_name16 = malloc(buf_size);
+	if (!var_name16)
+		return CMD_RET_FAILURE;
 
-	if (!len)
-		return CMD_RET_SUCCESS;
+	var_name16[0] = 0;
+	for (;;) {
+		size = buf_size;
+		ret = EFI_CALL(efi_get_next_variable_name(&size, var_name16,
+							  &guid));
+		if (ret == EFI_NOT_FOUND)
+			break;
+		if (ret == EFI_BUFFER_TOO_SMALL) {
+			buf_size = size;
+			p = realloc(var_name16, buf_size);
+			if (!p) {
+				free(var_name16);
+				return CMD_RET_FAILURE;
+			}
+			var_name16 = p;
+			ret = EFI_CALL(efi_get_next_variable_name(&size,
+								  var_name16,
+								  &guid));
+		}
+		if (ret != EFI_SUCCESS) {
+			free(var_name16);
+			return CMD_RET_FAILURE;
+		}
 
-	if (len < 0)
-		return CMD_RET_FAILURE;
+		if (u16_strncmp(var_name16, L"Boot", 4) || var_name16[8] ||
+		    !u16_isxdigit(var_name16[4]) ||
+		    !u16_isxdigit(var_name16[5]) ||
+		    !u16_isxdigit(var_name16[6]) ||
+		    !u16_isxdigit(var_name16[7]))
+			continue;
 
-	boot = variables;
-	while (*boot) {
-		value = strstr(boot, "Boot") + 4;
-		id = (int)simple_strtoul(value, NULL, 16);
+		for (id = 0, i = 0; i < 4; i++)
+			id = (id << 4) + u16_tohex(var_name16[4 + i]);
 		show_efi_boot_opt(id);
-		boot = strchr(boot, '\n');
-		if (!*boot)
-			break;
-		boot++;
 	}
-	free(variables);
+
+	free(var_name16);
 
 	return CMD_RET_SUCCESS;
 }
@@ -914,7 +954,7 @@ static int do_efi_boot_order(cmd_tbl_t *cmdtp, int flag,
 		return CMD_RET_FAILURE;
 
 	for (i = 0; i < argc; i++) {
-		id = (int)simple_strtoul(argv[i], &endp, 16);
+		id = simple_strtoul(argv[i], &endp, 16);
 		if (*endp != '\0' || id > 0xffff) {
 			printf("invalid value: %s\n", argv[i]);
 			ret = CMD_RET_FAILURE;
-- 
2.20.1

[U-Boot] [PATCH v2 04/11] efi_loader: set OsIndicationsSupported at init

[AKASHI Takahiro]

UEFI variable should be installed using well-defined API.
Currently we don't support much, but the value fo OsIndicationsSupported
will be updated once some features are added in the future.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/bootefi.c              | 4 ----
 lib/efi_loader/efi_setup.c | 9 +++++++++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index efaa548be4d8..b93d8c6a32cd 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -303,10 +303,6 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle)
 	if (ret != EFI_SUCCESS)
 		return ret;
 
-	/* we don't support much: */
-	env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
-		"{ro,boot}(blob)0000000000000000");
-
 	/* Call our payload! */
 	ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
 
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index 7d67a5506335..05d8d754f4c7 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -89,6 +89,7 @@ out:
  */
 efi_status_t efi_init_obj_list(void)
 {
+	u64 val = 0;
 	efi_status_t ret = EFI_SUCCESS;
 
 	/* Initialize once only */
@@ -100,6 +101,14 @@ efi_status_t efi_init_obj_list(void)
 	if (ret != EFI_SUCCESS)
 		goto out;
 
+	ret = EFI_CALL(efi_set_variable(L"OsIndicationsSupported",
+					&efi_global_variable_guid,
+					EFI_VARIABLE_BOOTSERVICE_ACCESS |
+					EFI_VARIABLE_RUNTIME_ACCESS,
+					sizeof(val), &val));
+	if (ret != EFI_SUCCESS)
+		goto out;
+
 	/* Initialize system table */
 	ret = efi_initialize_system_table();
 	if (ret != EFI_SUCCESS)
-- 
2.20.1

[U-Boot] [PATCH v2 05/11] env: save UEFI non-volatile variables in dedicated storage

[AKASHI Takahiro]

We need a variant of env_save()/env_load() to handle dedicated storage
for UEFI variables.
It is assumed that env_efi_load() will be called only ince at init
and that env_efi_save() will be called at every SetVariable.

In this patch, new parameters will be expected to be configured:
  CONFIG_ENV_EFI_FAT_DEVICE_AND_PART
  CONFIG_ENV_EFI_FAT_FILE
in case of CONFIG_ENV_IS_IN_FAT.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 env/Kconfig                       |  34 ++++++++++
 env/env.c                         |  98 ++++++++++++++++++++++++++-
 env/fat.c                         | 109 ++++++++++++++++++++++++++++++
 include/asm-generic/global_data.h |   1 +
 include/environment.h             |  24 +++++++
 5 files changed, 265 insertions(+), 1 deletion(-)

diff --git a/env/Kconfig b/env/Kconfig
index 78300660c720..8f59e9347d4b 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -438,6 +438,34 @@ config ENV_FAT_FILE
 	  It's a string of the FAT file name. This file use to store the
 	  environment.
 
+config ENV_EFI_FAT_DEVICE_AND_PART
+	string "Device and partition for where to store the UEFI non-volatile variables in FAT"
+	depends on ENV_IS_IN_FAT
+	depends on EFI_LOADER
+	help
+	  Define this to a string to specify the partition of the device. It can
+	  be as following:
+
+	    "D:P", "D:0", "D", "D:" or "D:auto" (D, P are integers. And P >= 1)
+	       - "D:P": device D partition P. Error occurs if device D has no
+	                partition table.
+	       - "D:0": device D.
+	       - "D" or "D:": device D partition 1 if device D has partition
+	                      table, or the whole device D if has no partition
+	                      table.
+	       - "D:auto": first partition in device D with bootable flag set.
+	                   If none, first valid partition in device D. If no
+	                   partition table then means device D.
+
+config ENV_EFI_FAT_FILE
+	string "Name of the FAT file to use for the UEFI non-volatile variables"
+	depends on ENV_IS_IN_FAT
+	depends on EFI_LOADER
+	default "uboot_efi.env"
+	help
+	  It's a string of the FAT file name. This file use to store the
+	  UEFI non-volatile variables.
+
 config ENV_EXT4_INTERFACE
 	string "Name of the block device for the environment"
 	depends on ENV_IS_IN_EXT4
@@ -470,6 +498,12 @@ config ENV_EXT4_FILE
 	  It's a string of the EXT4 file name. This file use to store the
 	  environment (explicit path to the file)
 
+config ENV_EFI_SIZE
+	hex "UEFI Variables Environment Size"
+	default 0x20000
+	help
+	  Size of the UEFI variables storage area
+
 if ARCH_ROCKCHIP || ARCH_SUNXI || ARCH_ZYNQ || ARCH_ZYNQMP || ARCH_VERSAL || ARC
 
 config ENV_OFFSET
diff --git a/env/env.c b/env/env.c
index 4b417b90a291..d5af761ba57e 100644
--- a/env/env.c
+++ b/env/env.c
@@ -24,6 +24,10 @@ void env_fix_drivers(void)
 			entry->load += gd->reloc_off;
 		if (entry->save)
 			entry->save += gd->reloc_off;
+		if (entry->efi_load)
+			entry->efi_load += gd->reloc_off;
+		if (entry->efi_save)
+			entry->efi_save += gd->reloc_off;
 		if (entry->init)
 			entry->init += gd->reloc_off;
 	}
@@ -125,7 +129,8 @@ __weak enum env_location env_get_location(enum env_operation op, int prio)
 	if (prio >= ARRAY_SIZE(env_locations))
 		return ENVL_UNKNOWN;
 
-	gd->env_load_prio = prio;
+	if (op != ENVOP_EFI)
+		gd->env_load_prio = prio;
 
 	return env_locations[prio];
 }
@@ -280,3 +285,94 @@ int env_init(void)
 
 	return ret;
 }
+
+#ifdef CONFIG_EFI_LOADER
+extern struct hsearch_data efi_var_htab;
+extern struct hsearch_data efi_nv_var_htab;
+
+/* TODO: experimental */
+int env_efi_save(void)
+{
+	struct env_driver *drv = NULL;
+	int ret;
+
+	if (!efi_nv_var_htab.table)
+		return 0;
+
+	if (gd->env_efi_prio == -1) {
+		printf("Cannot save UEFI non-volatile variable\n");
+		return -1;
+	}
+
+	drv = _env_driver_lookup(env_get_location(ENVOP_EFI, gd->env_efi_prio));
+	if (!drv) {
+		printf("Cannot save UEFI non-volatile variable\n");
+		return -1;
+	}
+
+	ret = drv->efi_save();
+	if (ret)
+		printf("Saving UEFI non-volatile variable failed\n");
+
+	return ret;
+}
+
+/* TODO: experimental */
+/* This function should be called only once at init */
+int env_efi_load(void)
+{
+	struct env_driver *drv = NULL;
+	int prio, ret;
+	enum env_location loc;
+
+	gd->env_efi_prio = -1;
+
+	/* volatile variables */
+	if (!efi_var_htab.table) {
+		ret = himport_r(&efi_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
+		if (!ret) {
+			printf("Creating default UEFI variables failed\n");
+			return -1;
+		}
+	}
+
+	/* non-loratile variables */
+	if (efi_nv_var_htab.table)
+		return 0;
+
+	for (prio = 0; prio < ARRAY_SIZE(env_locations); prio++) {
+		loc = env_get_location(ENVOP_EFI, prio);
+		drv = _env_driver_lookup(loc);
+		if (!drv)
+			continue;
+
+		if (drv->efi_load && drv->efi_save)
+			break;
+	}
+	if (!drv || prio == ARRAY_SIZE(env_locations)) {
+		printf("No driver for UEFI storage is available\n");
+		return -1;
+	}
+
+	gd->env_efi_prio = prio;
+
+	ret = drv->efi_load();
+	if (ret) {
+		printf("Loading UEFI non-volatile variables failed\n");
+		return -1;
+	}
+
+	/* FIXME: init needed here? */
+	if (!efi_nv_var_htab.table) {
+		ret = himport_r(&efi_nv_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
+		if (!ret) {
+			printf("Creating default UEFI non-volatile variables failed\n");
+			return -1;
+		}
+
+		return 0;
+	}
+
+	return 0;
+}
+#endif /* CONFIG_EFI_LOADER */
diff --git a/env/fat.c b/env/fat.c
index 7f74c64dfe7e..e2b050d4553d 100644
--- a/env/fat.c
+++ b/env/fat.c
@@ -14,6 +14,7 @@
 #include <malloc.h>
 #include <memalign.h>
 #include <search.h>
+#include <efi_loader.h>
 #include <errno.h>
 #include <fat.h>
 #include <mmc.h>
@@ -128,6 +129,110 @@ err_env_relocate:
 }
 #endif /* LOADENV */
 
+#ifdef CONFIG_EFI_LOADER
+#if 1
+extern int efi_variable_import(const char *buf, int check);
+extern int efi_variable_export(env_t *env_out);
+#endif
+static int env_fat_efi_save(void)
+{
+	env_t __aligned(ARCH_DMA_MINALIGN) env_new;
+	struct blk_desc *dev_desc = NULL;
+	disk_partition_t info;
+	int dev, part;
+	int err;
+	loff_t size;
+
+	err = efi_variable_export(&env_new);
+	if (err)
+		return err;
+
+	part = blk_get_device_part_str(CONFIG_ENV_FAT_INTERFACE,
+				       CONFIG_ENV_EFI_FAT_DEVICE_AND_PART,
+				       &dev_desc, &info, 1);
+	if (part < 0)
+		return 1;
+
+	dev = dev_desc->devnum;
+	if (fat_set_blk_dev(dev_desc, &info) != 0) {
+		/*
+		 * This printf is embedded in the messages from env_save that
+		 * will calling it. The missing \n is intentional.
+		 */
+		printf("Unable to use %s %d:%d... ",
+		       CONFIG_ENV_FAT_INTERFACE, dev, part);
+		return 1;
+	}
+
+	err = file_fat_write(CONFIG_ENV_EFI_FAT_FILE,
+			     (void *)&env_new, 0, sizeof(env_t), &size);
+	if (err < 0) {
+		/*
+		 * This printf is embedded in the messages from env_save that
+		 * will calling it. The missing \n is intentional.
+		 */
+		printf("Unable to write \"%s\" from %s%d:%d... ",
+		       CONFIG_ENV_EFI_FAT_FILE, CONFIG_ENV_FAT_INTERFACE,
+		       dev, part);
+		return 1;
+	}
+
+	return 0;
+}
+
+static int env_fat_efi_load(void)
+{
+	ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_EFI_SIZE);
+	struct blk_desc *dev_desc = NULL;
+	disk_partition_t info;
+	int dev, part;
+	int err;
+
+#ifdef CONFIG_MMC
+	if (!strcmp(CONFIG_ENV_FAT_INTERFACE, "mmc"))
+		mmc_initialize(NULL);
+#endif
+
+	part = blk_get_device_part_str(CONFIG_ENV_FAT_INTERFACE,
+				       CONFIG_ENV_EFI_FAT_DEVICE_AND_PART,
+				       &dev_desc, &info, 1);
+	if (part < 0)
+		goto err_env_relocate;
+
+	dev = dev_desc->devnum;
+	if (fat_set_blk_dev(dev_desc, &info) != 0) {
+		/*
+		 * This printf is embedded in the messages from env_save that
+		 * will calling it. The missing \n is intentional.
+		 */
+		printf("Unable to use %s %d:%d...\n",
+		       CONFIG_ENV_FAT_INTERFACE, dev, part);
+		goto err_env_relocate;
+	}
+
+	err = file_fat_read(CONFIG_ENV_EFI_FAT_FILE, buf, CONFIG_ENV_EFI_SIZE);
+	if (err <= 0 && (err != -ENOENT)) {
+		/*
+		 * This printf is embedded in the messages from env_save that
+		 * will calling it. The missing \n is intentional.
+		 */
+		printf("Unable to read \"%s\" from %s %d:%d...\n",
+		       CONFIG_ENV_EFI_FAT_FILE, CONFIG_ENV_FAT_INTERFACE,
+		       dev, part);
+		goto err_env_relocate;
+	}
+
+	if (err > 0)
+		return efi_variable_import(buf, 1);
+	else
+		return 0;
+
+err_env_relocate:
+
+	return -EIO;
+}
+#endif /* CONFIG_EFI_LOADER */
+
 U_BOOT_ENV_LOCATION(fat) = {
 	.location	= ENVL_FAT,
 	ENV_NAME("FAT")
@@ -137,4 +242,8 @@ U_BOOT_ENV_LOCATION(fat) = {
 #ifdef CMD_SAVEENV
 	.save		= env_save_ptr(env_fat_save),
 #endif
+#ifdef CONFIG_EFI_LOADER
+	.efi_load	= env_fat_efi_load,
+	.efi_save	= env_fat_efi_save,
+#endif
 };
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index 78dcf40bff48..8c4d56c346f3 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -51,6 +51,7 @@ typedef struct global_data {
 	unsigned long env_valid;	/* Environment valid? enum env_valid */
 	unsigned long env_has_init;	/* Bitmask of boolean of struct env_location offsets */
 	int env_load_prio;		/* Priority of the loaded environment */
+	int env_efi_prio;		/* Priority of the UEFI variables */
 
 	unsigned long ram_base;		/* Base address of RAM used by U-Boot */
 	unsigned long ram_top;		/* Top address of RAM used by U-Boot */
diff --git a/include/environment.h b/include/environment.h
index cd966761416e..f62399863ef9 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -200,6 +200,7 @@ enum env_operation {
 	ENVOP_INIT,	/* we want to call the init function */
 	ENVOP_LOAD,	/* we want to call the load function */
 	ENVOP_SAVE,	/* we want to call the save function */
+	ENVOP_EFI,	/* we want to call the efi_load/save function */
 };
 
 struct env_driver {
@@ -225,6 +226,24 @@ struct env_driver {
 	 */
 	int (*save)(void);
 
+	/**
+	 * efi_load() - Load UEFI non-volatile variables from storage
+	 *
+	 * This method is required for UEFI non-volatile variables
+	 *
+	 * @return 0 if OK, -ve on error
+	 */
+	int (*efi_load)(void);
+
+	/**
+	 * efi_save() - Save UEFI non-volatile variables to storage
+	 *
+	 * This method is required for UEFI non-volatile variables
+	 *
+	 * @return 0 if OK, -ve on error
+	 */
+	int (*efi_save)(void);
+
 	/**
 	 * init() - Set up the initial pre-relocation environment
 	 *
@@ -312,6 +331,11 @@ void eth_parse_enetaddr(const char *addr, uint8_t *enetaddr);
 int eth_env_get_enetaddr(const char *name, uint8_t *enetaddr);
 int eth_env_set_enetaddr(const char *name, const uint8_t *enetaddr);
 
+#ifdef CONFIG_EFI_LOADER
+int env_efi_load(void);
+int env_efi_save(void);
+#endif
+
 #endif /* DO_DEPS_ONLY */
 
 #endif /* _ENVIRONMENT_H_ */
-- 
2.20.1

[U-Boot] [PATCH v2 06/11] efi_loader: variable: support non-volatile attribute

[AKASHI Takahiro]

The attribute, EFI_VARIABLE_NON_VOLATILE, should be encoded as "nv" flag
in U-Boot variable if specified.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_variable.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 37728c3c165d..2f489ab9db97 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -125,6 +125,8 @@ static const char *parse_attr(const char *str, u32 *attrp)
 
 		if ((s = prefix(str, "ro"))) {
 			attr |= READ_ONLY;
+		} else if ((s = prefix(str, "nv"))) {
+			attr |= EFI_VARIABLE_NON_VOLATILE;
 		} else if ((s = prefix(str, "boot"))) {
 			attr |= EFI_VARIABLE_BOOTSERVICE_ACCESS;
 		} else if ((s = prefix(str, "run"))) {
@@ -452,7 +454,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
 		}
 	}
 
-	val = malloc(2 * data_size + strlen("{ro,run,boot}(blob)") + 1);
+	val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1);
 	if (!val) {
 		ret = EFI_OUT_OF_RESOURCES;
 		goto out;
@@ -464,12 +466,16 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
 	 * store attributes
 	 * TODO: several attributes are not supported
 	 */
-	attributes &= (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS);
+	attributes &= (EFI_VARIABLE_NON_VOLATILE |
+		       EFI_VARIABLE_BOOTSERVICE_ACCESS |
+		       EFI_VARIABLE_RUNTIME_ACCESS);
 	s += sprintf(s, "{");
 	while (attributes) {
 		u32 attr = 1 << (ffs(attributes) - 1);
 
-		if (attr == EFI_VARIABLE_BOOTSERVICE_ACCESS)
+		if (attr == EFI_VARIABLE_NON_VOLATILE)
+			s += sprintf(s, "nv");
+		else if (attr == EFI_VARIABLE_BOOTSERVICE_ACCESS)
 			s += sprintf(s, "boot");
 		else if (attr == EFI_VARIABLE_RUNTIME_ACCESS)
 			s += sprintf(s, "run");
-- 
2.20.1

[U-Boot] [PATCH v2 07/11] efi_loader: variable: split UEFI variables from U-Boot environment

[AKASHI Takahiro]

UEFI volatile variables are managed in efi_var_htab while UEFI non-volatile
variables are in efi_nv_var_htab. At every SetVariable API, env_efi_save()
will also be called to save data cache (hash table) to persistent storage.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_variable.c | 162 ++++++++++++++++++++++++++++++++--
 1 file changed, 155 insertions(+), 7 deletions(-)

diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 2f489ab9db97..f7b1ce2f3350 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -48,6 +48,115 @@
  * converted to utf16?
  */
 
+/*
+ * We will maintain two variable database: one for volatile variables,
+ * the other for non-volatile variables. The former exists only in memory
+ * and will go away at re-boot. The latter is currently backed up by the same
+ * device as U-Boot environment and also works as variables cache.
+ */
+
+enum efi_var_type {
+	EFI_VAR_TYPE_VOLATILE,
+	EFI_VAR_TYPE_NON_VOLATILE,
+};
+
+struct hsearch_data efi_var_htab;
+struct hsearch_data efi_nv_var_htab;
+
+static char *env_efi_get(const char *name, int type)
+{
+	struct hsearch_data *htab;
+	ENTRY e, *ep;
+
+	/* WATCHDOG_RESET(); */
+
+	if (type == EFI_VAR_TYPE_VOLATILE)
+		htab = &efi_var_htab;
+	else
+		htab = &efi_nv_var_htab;
+
+	e.key   = name;
+	e.data  = NULL;
+	hsearch_r(e, FIND, &ep, htab, 0);
+
+	return ep ? ep->data : NULL;
+}
+
+static int env_efi_set(const char *name, const char *value, int type)
+{
+	struct hsearch_data *htab;
+	ENTRY e, *ep;
+	int ret;
+
+	if (type == EFI_VAR_TYPE_VOLATILE)
+		htab = &efi_var_htab;
+	else
+		htab = &efi_nv_var_htab;
+
+	/* delete */
+	if (!value || *value == '\0') {
+		ret = hdelete_r(name, htab, H_PROGRAMMATIC);
+		return !ret;
+	}
+
+	/* set */
+	e.key   = name;
+	e.data  = (char *)value;
+	hsearch_r(e, ENTER, &ep, htab, H_PROGRAMMATIC);
+	if (!ep) {
+		printf("## Error inserting \"%s\" variable, errno=%d\n",
+		       name, errno);
+		return 1;
+	}
+
+	return 0;
+}
+
+int efi_variable_import(const char *buf, int check)
+{
+	env_t *ep = (env_t *)buf;
+
+	if (check) {
+		u32 crc;
+
+		memcpy(&crc, &ep->crc, sizeof(crc));
+
+		if (crc32(0, ep->data, CONFIG_ENV_EFI_SIZE) != crc) {
+			pr_err("bad CRC of UEFI variables\n");
+			return -ENOMSG; /* needed for env_load() */
+		}
+	}
+
+	if (himport_r(&efi_nv_var_htab, (char *)ep->data, CONFIG_ENV_EFI_SIZE,
+		      '\0', 0, 0, 0, NULL))
+		return 0;
+
+	pr_err("Cannot import environment: errno = %d\n", errno);
+
+	/* set_default_env("import failed", 0); */
+
+	return -EIO;
+}
+
+/* Export the environment and generate CRC for it. */
+int efi_variable_export(env_t *env_out)
+{
+	char *res;
+	ssize_t	len;
+
+	res = (char *)env_out->data;
+	len = hexport_r(&efi_nv_var_htab, '\0', 0, &res, CONFIG_ENV_EFI_SIZE,
+			0, NULL);
+	if (len < 0) {
+		pr_err("Cannot export environment: errno = %d\n", errno);
+		return 1;
+	}
+
+	env_out->crc = crc32(0, env_out->data, CONFIG_ENV_EFI_SIZE);
+
+	return 0;
+}
+
 #define PREFIX_LEN (strlen("efi_xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx_"))
 
 /**
@@ -184,7 +293,9 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
 
 	EFI_PRINT("get '%s'\n", native_name);
 
-	val = env_get(native_name);
+	val = env_efi_get(native_name, EFI_VAR_TYPE_VOLATILE);
+	if (!val)
+		val = env_efi_get(native_name, EFI_VAR_TYPE_NON_VOLATILE);
 	free(native_name);
 	if (!val)
 		return EFI_EXIT(EFI_NOT_FOUND);
@@ -326,7 +437,7 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
 					       u16 *variable_name,
 					       const efi_guid_t *vendor)
 {
-	char *native_name, *variable;
+	char *native_name, *variable, *tmp_list, *merged_list;
 	ssize_t name_len, list_len;
 	char regex[256];
 	char * const regexlist[] = {regex};
@@ -382,10 +493,39 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
 		efi_cur_variable = NULL;
 
 		snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*");
-		list_len = hexport_r(&env_htab, '\n',
+		list_len = hexport_r(&efi_var_htab, '\n',
 				     H_MATCH_REGEX | H_MATCH_KEY,
 				     &efi_variables_list, 0, 1, regexlist);
-		/* 1 indicates that no match was found */
+		/*
+		 * Note: '1' indicates that nothing is matched
+		 */
+		if (list_len <= 1) {
+			free(efi_variables_list);
+			efi_variables_list = NULL;
+			list_len = hexport_r(&efi_nv_var_htab, '\n',
+					     H_MATCH_REGEX | H_MATCH_KEY,
+					     &efi_variables_list, 0, 1,
+					     regexlist);
+		} else {
+			tmp_list = NULL;
+			list_len = hexport_r(&efi_nv_var_htab, '\n',
+					     H_MATCH_REGEX | H_MATCH_KEY,
+					     &tmp_list, 0, 1,
+					     regexlist);
+			if (list_len <= 1) {
+				list_len = 2; /* don't care actual number */
+			} else {
+				/* merge two variables lists */
+				merged_list = malloc(strlen(efi_variables_list)
+							+ strlen(tmp_list) + 1);
+				strcpy(merged_list, efi_variables_list);
+				strcat(merged_list, tmp_list);
+				free(efi_variables_list);
+				free(tmp_list);
+				efi_variables_list = merged_list;
+			}
+		}
+
 		if (list_len <= 1)
 			return EFI_EXIT(EFI_NOT_FOUND);
 
@@ -420,6 +560,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
 	char *native_name = NULL, *val = NULL, *s;
 	efi_status_t ret = EFI_SUCCESS;
 	u32 attr;
+	int type;
 
 	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
 		  data_size, data);
@@ -435,14 +576,18 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
 
 #define ACCESS_ATTR (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)
 
+	type = (attributes & EFI_VARIABLE_NON_VOLATILE) ?
+		EFI_VAR_TYPE_NON_VOLATILE : EFI_VAR_TYPE_VOLATILE;
 	if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
 		/* delete the variable: */
-		env_set(native_name, NULL);
+		env_efi_set(native_name, NULL, type);
 		ret = EFI_SUCCESS;
 		goto out;
 	}
 
-	val = env_get(native_name);
+	val = env_efi_get(native_name, EFI_VAR_TYPE_VOLATILE);
+	if (!val)
+		val = env_efi_get(native_name, EFI_VAR_TYPE_NON_VOLATILE);
 	if (val) {
 		parse_attr(val, &attr);
 
@@ -493,9 +638,12 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
 
 	EFI_PRINT("setting: %s=%s\n", native_name, val);
 
-	if (env_set(native_name, val))
+	if (env_efi_set(native_name, val, type))
 		ret = EFI_DEVICE_ERROR;
 
+	/* FIXME: what if save failed? */
+	env_efi_save();
+
 out:
 	free(native_name);
 	free(val);
-- 
2.20.1

[U-Boot] [PATCH v2 08/11] efi_loader: load saved non-volatile variables at init

[AKASHI Takahiro]

Data cache will be read in from persistent storage after (re)boot
to restore UEFI non-volatile variables.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_setup.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index 05d8d754f4c7..490e3c5eb81a 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -7,6 +7,7 @@
 
 #include <common.h>
 #include <efi_loader.h>
+#include <environment.h>
 
 #if 1 /* TEMPORARILY */
 #define DXE_SERVICES_TABLE_GUID \
@@ -96,6 +97,9 @@ efi_status_t efi_init_obj_list(void)
 	if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED)
 		return efi_obj_list_initialized;
 
+	/* Load non-volatile variables */
+	env_efi_load();
+
 	/* Define supported languages */
 	ret = efi_init_platform_lang();
 	if (ret != EFI_SUCCESS)
-- 
2.20.1

[U-Boot] [PATCH v2 09/11] efi_loader: bootmgr: handle BootNext as non-volatile

[AKASHI Takahiro]

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_bootmgr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 4ccba2287572..e8f48684257f 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -206,7 +206,8 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle)
 		ret = EFI_CALL(efi_set_variable(
 					L"BootNext",
 					(efi_guid_t *)&efi_global_variable_guid,
-					0, 0, &bootnext));
+					EFI_VARIABLE_NON_VOLATILE, 0,
+					&bootnext));
 
 		/* load BootNext */
 		if (ret == EFI_SUCCESS) {
-- 
2.20.1

[U-Boot] [PATCH v2 10/11] cmd: env: add -nv option for UEFI non-volatile variable

[AKASHI Takahiro]

With this option, -nv, at "setenv -e" command, a variable will be defined
as non-volatile.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/nvedit.c     |  3 ++-
 cmd/nvedit_efi.c | 15 ++++++++++++---
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 24a6cf7824ad..52c242b4f622 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -1344,8 +1344,9 @@ U_BOOT_CMD_COMPLETE(
 	setenv, CONFIG_SYS_MAXARGS, 0,	do_env_set,
 	"set environment variables",
 #if defined(CONFIG_CMD_NVEDIT_EFI)
-	"-e name [value ...]\n"
+	"-e [-nv] name [value ...]\n"
 	"    - set UEFI variable 'name' to 'value' ...'\n"
+	"      'nv' option makes the variable non-volatile\n"
 	"    - delete UEFI variable 'name' if 'value' not specified\n"
 #endif
 	"setenv [-f] name value ...\n"
diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
index e65b38dbf399..ae0d9c18ad43 100644
--- a/cmd/nvedit_efi.c
+++ b/cmd/nvedit_efi.c
@@ -346,6 +346,7 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	u16 *var_name16 = NULL, *p;
 	size_t len;
 	efi_guid_t guid;
+	u32 attributes;
 	efi_status_t ret;
 
 	if (argc == 1)
@@ -359,6 +360,16 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return CMD_RET_FAILURE;
 	}
 
+	attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
+		     EFI_VARIABLE_RUNTIME_ACCESS;
+	if (!strcmp(argv[1], "-nv")) {
+		attributes |= EFI_VARIABLE_NON_VOLATILE;
+		argc--;
+		argv++;
+		if (argc == 1)
+			return CMD_RET_SUCCESS;
+	}
+
 	var_name = argv[1];
 	if (argc == 2) {
 		/* delete */
@@ -385,9 +396,7 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	utf8_utf16_strncpy(&p, var_name, len + 1);
 
 	guid = efi_global_variable_guid;
-	ret = EFI_CALL(efi_set_variable(var_name16, &guid,
-					EFI_VARIABLE_BOOTSERVICE_ACCESS |
-					EFI_VARIABLE_RUNTIME_ACCESS,
+	ret = EFI_CALL(efi_set_variable(var_name16, &guid, attributes,
 					size, value));
 	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
 out:
-- 
2.20.1

[U-Boot] [PATCH v2 11/11] cmd: efidebug: make some boot variables non-volatile

[AKASHI Takahiro]

Boot####, BootOrder and BootNext should be non-volatile.

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

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 8890dd7268f1..ff3cad53f1b7 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -554,6 +554,7 @@ static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
 	}
 
 	ret = EFI_CALL(RT->set_variable(var_name16, &guid,
+					EFI_VARIABLE_NON_VOLATILE |
 					EFI_VARIABLE_BOOTSERVICE_ACCESS |
 					EFI_VARIABLE_RUNTIME_ACCESS,
 					size, data));
@@ -911,6 +912,7 @@ static int do_efi_boot_next(cmd_tbl_t *cmdtp, int flag,
 	guid = efi_global_variable_guid;
 	size = sizeof(u16);
 	ret = EFI_CALL(RT->set_variable(L"BootNext", &guid,
+					EFI_VARIABLE_NON_VOLATILE |
 					EFI_VARIABLE_BOOTSERVICE_ACCESS |
 					EFI_VARIABLE_RUNTIME_ACCESS,
 					size, &bootnext));
@@ -966,6 +968,7 @@ static int do_efi_boot_order(cmd_tbl_t *cmdtp, int flag,
 
 	guid = efi_global_variable_guid;
 	ret = EFI_CALL(RT->set_variable(L"BootOrder", &guid,
+					EFI_VARIABLE_NON_VOLATILE |
 					EFI_VARIABLE_BOOTSERVICE_ACCESS |
 					EFI_VARIABLE_RUNTIME_ACCESS,
 					size, bootorder));
-- 
2.20.1

[U-Boot] [PATCH v2 01/11] lib: charset: add u16_strcmp()

[Heinrich Schuchardt]

On 4/24/19 8:30 AM, AKASHI Takahiro wrote:
> u16 version of strcmp()
>
> AUTHER: Patrick Wildt <patrick@blueri.se>

%s/AUTHER/Author/

> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   include/charset.h |  5 +++++
>   lib/charset.c     | 10 ++++++++++
>   2 files changed, 15 insertions(+)
>
> diff --git a/include/charset.h b/include/charset.h
> index 65087f76d1fc..747a9b376c03 100644
> --- a/include/charset.h
> +++ b/include/charset.h
> @@ -166,6 +166,11 @@ s32 utf_to_lower(const s32 code);
>    */
>   s32 utf_to_upper(const s32 code);
>
> +/*
> + * u16_strcmp() - strcmp() for u16 strings
> + */
> +int u16_strcmp(const u16 *s1, const u16 *s2);
> +
>   /**
>    * u16_strlen - count non-zero words
>    *
> diff --git a/lib/charset.c b/lib/charset.c
> index 5e349ed5ee45..4a25ac0bdb9c 100644
> --- a/lib/charset.c
> +++ b/lib/charset.c
> @@ -335,6 +335,16 @@ s32 utf_to_upper(const s32 code)
>   	return ret;
>   }
>
> +int u16_strcmp(const u16 *s1, const u16 *s2)
> +{
> +	while (*s1 == *s2++)
> +		if (*s1++ == 0)
> +			return (0);
> +	--s2;

for (;*s1 == *s2; ++s1, ++s2)
	if (!s1)
		return 0;

does the same job without superfluous increment/decrement.

> +
> +	return (*(uint16_t *)s1 - *(uint16_t *)s2);

Why would you use both u16 and uint16_t in the same function?
You can do without any conversion here.

How about

#define u16_strcmp(s1, s2) u16_strncmp(s1, s2, SIZE_MAX)

like we do for the other string functions?

Best regards

Heinrich

> +}
> +
>   size_t u16_strlen(const u16 *in)
>   {
>   	size_t i;
>

[U-Boot] [PATCH v2 02/11] lib: charset: add u16_strncmp()

[Heinrich Schuchardt]

On 4/24/19 8:30 AM, AKASHI Takahiro wrote:
> u16_strncmp() works like u16_strcmp() but only at most n characters
> (in u16) are compared.
> This function will be used in a later patch.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   include/charset.h |  5 +++++
>   lib/charset.c     | 13 +++++++++++++
>   2 files changed, 18 insertions(+)
>
> diff --git a/include/charset.h b/include/charset.h
> index 747a9b376c03..49842a88bc8b 100644
> --- a/include/charset.h
> +++ b/include/charset.h
> @@ -171,6 +171,11 @@ s32 utf_to_upper(const s32 code);
>    */
>   int u16_strcmp(const u16 *s1, const u16 *s2);
>
> +/*
> + * u16_strncmp() - strncmp() for u16 strings

  * u16_strncmp() - compare two u16 string
  *
  * @s1:		first string to compare
  * @s2:		second string to compare
  * @n1:		maximum number of u16 to compare
  * Return:	0  if the first n u16 are the same in s1 and s2
  *		< 0 if the first different u16 in s1 is less than the
  *		corresponding u16 in s2
  *		> 0 if the first different u16 in s1 is greater than the
  *		corresponding u16 in s2

> + */
> +int u16_strncmp(const u16 *s1, const u16 *s2, size_t n);
> +
>   /**
>    * u16_strlen - count non-zero words
>    *
> diff --git a/lib/charset.c b/lib/charset.c
> index 4a25ac0bdb9c..85f08db68fe2 100644
> --- a/lib/charset.c
> +++ b/lib/charset.c
> @@ -345,6 +345,19 @@ int u16_strcmp(const u16 *s1, const u16 *s2)
>   	return (*(uint16_t *)s1 - *(uint16_t *)s2);
>   }
>
> +int u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
> +{
> +	while ((n-- > 0) && (*s1 == *s2++)) {
> +		if (*s1++ == 0)
> +			return 0;
> +		if (!n)
> +			return 0;
> +	}
> +	--s2;

For u16_strncmp() called with n = 0 I would prefer to see 0 as return
value instead of the result of an illegal memory access.

> +
> +	return (*(uint16_t *)s1 - *(uint16_t *)s2);

No need for a conversion here.

Let's avoid the superfluous increment/decrement, provide 0 for n = 0,
and make sure that the compiler calculates the difference only once per
loop:

int u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
{
         int ret = 0;

         for (; n; --n, ++s1, ++s2) {
                 ret = *s1 - *s2;
                 if (ret || !*s1)
                         break;
         }
         return ret;
}

I would like to see a unit test in test/unicode_ut.c.
The test should include the n = 0 case.

Best regards

Heinrich

> +}
> +
>   size_t u16_strlen(const u16 *in)
>   {
>   	size_t i;
>

[U-Boot] [PATCH v2 02/11] lib: charset: add u16_strncmp()

[Heinrich Schuchardt]

On 4/24/19 8:30 AM, AKASHI Takahiro wrote:
> u16_strncmp() works like u16_strcmp() but only at most n characters
> (in u16) are compared.
> This function will be used in a later patch.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

The only usage of u16_strncmp() is in patch 3.
u16_strcmp() is not used at all.

In patch 3 'memcmp(var_name16, L"BOOT", 8)' will do the job.

I am not sure if in other cases we wouldn't prefer to compare Unicode
codepoints instead of u16.

So I suggest to skip patches 1 and 2 and use memcmp() in patch 3.

Best regards

Heinrich

[U-Boot] [PATCH v2 03/11] cmd: efidebug: rework "boot dump" sub-command using GetNextVariableName()

[Heinrich Schuchardt]

On 4/24/19 8:30 AM, AKASHI Takahiro wrote:
> Efidebug command should be implemented using well-defined EFI interfaces,
> rather than using internal functions/data. This change will be needed in
> a later patch where UEFI variables are re-implemented.

I had trouble to get the point. In the next version I suggest to write:

Currently in do_efi_boot_dump() we directly read EFI variables from the
related environment variables. To accomodate alternative storage
backends we should switch to using the UEFI API instead.

>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   cmd/efidebug.c | 92 ++++++++++++++++++++++++++++++++++++--------------
>   1 file changed, 66 insertions(+), 26 deletions(-)
>
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index a40c4f4be286..8890dd7268f1 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -509,7 +509,7 @@ static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
>   	if (argc < 6 || argc > 7)
>   		return CMD_RET_USAGE;
>
> -	id = (int)simple_strtoul(argv[1], &endp, 16);
> +	id = simple_strtoul(argv[1], &endp, 16);

This change is correct but unrelated. Please, put it into a separate
patch. Or at least mention it in the commit message.

>   	if (*endp != '\0' || id > 0xffff)
>   		return CMD_RET_USAGE;
>
> @@ -595,7 +595,7 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag,
>
>   	guid = efi_global_variable_guid;
>   	for (i = 1; i < argc; i++, argv++) {
> -		id = (int)simple_strtoul(argv[1], &endp, 16);
> +		id = simple_strtoul(argv[1], &endp, 16);

Same here.

>   		if (*endp != '\0' || id > 0xffff)
>   			return CMD_RET_FAILURE;
>
> @@ -693,6 +693,27 @@ static void show_efi_boot_opt(int id)
>   	free(data);
>   }
>
> +static bool u16_isxdigit(u16 c)
> +{
> +	if (c & 0xff00)
> +		return false;
> +
> +	return isxdigit((u8)c);
> +}
> +
> +static int u16_tohex(u16 c)
> +{
> +	if (c >= '0' && c < '9')
> +		return c - '0';
> +	if (c >= 'A' && c < 'F')
> +		return c - 'A' + 10;
> +	if (c >= 'a' && c < 'f')
> +		return c - 'a' + 10;

Does the UEFI spec really allow lower case hexadecimal numbers here?
I only found an example with capital numbers. Would this imply that I
could have variables Boot00a0 and Boot00A0 side by side? Which one would
be selected by boot option 00a0?

Or should SetVariable() make a case insensitive search for variable names?

In EDK2 function FindVariableEx() in
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
uses CompareMem() to compare variable names.

Function BmCharToUint() is used to check the digits of boot options and
has this comment:

"It assumes the input Char is in the scope of L'0' ~ L'9'
and L'A' ~ L'F'"

So let's us assume that variable names are case sensitive and Boot
entries can only have capital hexadecimal digits.

So u16_isxdigit() is incorrect.

> +
> +	/* dummy */
> +	return -1;

As we do not check bounds here the function could be reduced to:

return c > '9' ? c - ('A' - 0xa) : c - '9';

But I would prefer one library function instead of the two static
functions which returns -1 for a non-digit and 0 - 15 for a digit.
And a unit test in test/unicoode_ut.c

> +}
> +
>   /**
>    * show_efi_boot_dump() - dump all UEFI load options
>    *
> @@ -709,38 +730,57 @@ static void show_efi_boot_opt(int id)
>   static int do_efi_boot_dump(cmd_tbl_t *cmdtp, int flag,
>   			    int argc, char * const argv[])
>   {
> -	char regex[256];
> -	char * const regexlist[] = {regex};
> -	char *variables = NULL, *boot, *value;
> -	int len;
> -	int id;
> +	u16 *var_name16, *p;
> +	efi_uintn_t buf_size, size;
> +	efi_guid_t guid;
> +	int id, i;
> +	efi_status_t ret;
>
>   	if (argc > 1)
>   		return CMD_RET_USAGE;
>
> -	snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_Boot[0-9A-F]+");
> -
> -	/* TODO: use GetNextVariableName? */
> -	len = hexport_r(&env_htab, '\n', H_MATCH_REGEX | H_MATCH_KEY,
> -			&variables, 0, 1, regexlist);
> +	buf_size = 128;
> +	var_name16 = malloc(buf_size);
> +	if (!var_name16)
> +		return CMD_RET_FAILURE;
>
> -	if (!len)
> -		return CMD_RET_SUCCESS;
> +	var_name16[0] = 0;
> +	for (;;) {
> +		size = buf_size;
> +		ret = EFI_CALL(efi_get_next_variable_name(&size, var_name16,
> +							  &guid));
> +		if (ret == EFI_NOT_FOUND)
> +			break;
> +		if (ret == EFI_BUFFER_TOO_SMALL) {
> +			buf_size = size;
> +			p = realloc(var_name16, buf_size);
> +			if (!p) {
> +				free(var_name16);
> +				return CMD_RET_FAILURE;
> +			}
> +			var_name16 = p;
> +			ret = EFI_CALL(efi_get_next_variable_name(&size,
> +								  var_name16,
> +								  &guid));
> +		}
> +		if (ret != EFI_SUCCESS) {
> +			free(var_name16);
> +			return CMD_RET_FAILURE;
> +		}
>
> -	if (len < 0)
> -		return CMD_RET_FAILURE;
> +		if (u16_strncmp(var_name16, L"Boot", 4) || var_name16[8] ||

We can use memcmp() here. This avoids introducing u16_strncmp().

> +		    !u16_isxdigit(var_name16[4]) ||
> +		    !u16_isxdigit(var_name16[5]) ||
> +		    !u16_isxdigit(var_name16[6]) ||
> +		    !u16_isxdigit(var_name16[7]))
> +			continue;
>
> -	boot = variables;
> -	while (*boot) {
> -		value = strstr(boot, "Boot") + 4;
> -		id = (int)simple_strtoul(value, NULL, 16);
> +		for (id = 0, i = 0; i < 4; i++)
> +			id = (id << 4) + u16_tohex(var_name16[4 + i]);

This all can be simplified if we unify u16_isxdigit() and u16_tohex().
Just one function returning -1 if not a hexadecimal and 0 - 15 otherwise.

>   		show_efi_boot_opt(id);
> -		boot = strchr(boot, '\n');
> -		if (!*boot)
> -			break;
> -		boot++;
>   	}
> -	free(variables);
> +
> +	free(var_name16);
>
>   	return CMD_RET_SUCCESS;
>   }
> @@ -914,7 +954,7 @@ static int do_efi_boot_order(cmd_tbl_t *cmdtp, int flag,
>   		return CMD_RET_FAILURE;
>
>   	for (i = 0; i < argc; i++) {
> -		id = (int)simple_strtoul(argv[i], &endp, 16);
> +		id = simple_strtoul(argv[i], &endp, 16);

This change is correct but unrelated. Please, put it into a separate
patch. Or at least mention it in the commit message.

Best regards

Heinrich

>   		if (*endp != '\0' || id > 0xffff) {
>   			printf("invalid value: %s\n", argv[i]);
>   			ret = CMD_RET_FAILURE;
>

[U-Boot] [PATCH v2 00/11] efi_loader: non-volatile variables support

[Heinrich Schuchardt]

On 4/24/19 8:30 AM, AKASHI Takahiro wrote:
> This patch set is an attempt to implement non-volatile attribute for
> UEFI variables. Under the current implementation,
> * SetVariable API doesn't recognize non-volatile attribute
> * While some variables are defined non-volatile in UEFI specification,
>    they are NOT marked as non-volatile in the code.
> * env_save() (or "env save" command) allows us to save all the variables
>    into persistent storage, but it may cause volatile UEFI variables,
>    along with irrelevant U-Boot variables, to be saved unconditionally.
>
> Those observation rationalizes that the implementation of UEFI variables
> should be revamped utilizing dedicated storage for them.
>
> This patch set is yet experimental and rough-edged(See known issues below),
> but shows how UEFI variables can be split from U-Boot environment.
> This enhancement will also be vital when we introduce UEFI secure boot
> where secure and tamper-resistant storage (with authentication) is
> required.
>
> Usage:
> To enable this feature, the following configs must be enabled:
>    CONFIG_ENV_IS_IN_FAT
>    CONFIG_ENV_FAT_INTERFACE
>    CONFIG_ENV_EFI_FAT_DEVICE_AND_PART
>    CONFIG_ENV_EFI_FAT_FILE
>
> You can also define a non-volatile variable from command interface:
> => setenv -e -nv FOO baa
>
> Known issues/restriction:
> * UEFI spec defines "globally defined variables" with specific
>    attributes, but with this patch, we don't check against the user-supplied
>    attribute for any variable.
> * Only FAT can be enabled for persistent storage for UEFI non-volatile
>    variables.
> * The whole area of storage will be saved at every update of one variable.
>    It can be optimized.
> * An error during saving may cause inconsistency between cache (hash table)
>    and the storage.
> * Cache is of fixed size and can be quite big for normal usage.

Hello Takahiro,

thanks a lot for looking into persisting non-volatile UEFI variables.

Before diving into the details of the patches let us discuss the overall
design.

My understanding is that we need a buffer that lives in
EFI_RUNTIME_SERVICES_DATA and holds all variables. It is this buffer
that the operating system will access when getting or setting variables.

If a non-volatile variable is set via SetVariable() we will call a
backend driver.

Our current backend is using U-Boot environment variables. It can only
be accessed at boottime. Currently there is no guarantee that the U-Boot
variables are saved before booting. So it does not support persisting
non-volatile variables reliably.

Your patch series supplies a new backend using a file for persisting
non-volatile variables. It can only be accessed at boottime. Essentially
this solution requires no restriction to FAT file systems. We could as
well use the EXT4 driver for reading or writing the file. This file
storage is completely independent of the U-Boot environment. So it shall
not involve any changes to files env/*. I think a documentation of the
file format provided in a README would be helpful.

In case of both backends we would return EFI_UNSUPPORTED when trying to
set a non-volatile variable at runtime. Setting a volatile variable at
runtime should be allowable as long as we have sufficient space in our
buffer.

We can remove these backends based on the EVT_SIGNAL_EXIT_BOOT_SERVICES
event.

A future backend may support persisting non-volatile variables at runtime.

I would prefer if we could first introduce patches that provide the
buffer for EFI variables and link it to the UEFI variable based backend
before adding the new backend as alternative.

Best regards

Heinrich


>
> Patch#1 to #4 are preparatory so that we won't rely on U-Boot environment,
> that is, env_get/set() helper functions.
> Patch#5 to #8 are core part of changes.
> Patch#9 to #11 are for modifying variable attributes.
>
> Changes in v2 (Apr 24, 2019)
> * rebased on efi-2019-07
> * revamp the implementation
>
> v1 (Nov 28, 2018)
> * initial
>
> AKASHI Takahiro (11):
>    lib: charset: add u16_strcmp()
>    lib: charset: add u16_strncmp()
>    cmd: efidebug: rework "boot dump" sub-command using
>      GetNextVariableName()
>    efi_loader: set OsIndicationsSupported at init
>    env: save UEFI non-volatile variables in dedicated storage
>    efi_loader: variable: support non-volatile attribute
>    efi_loader: variable: split UEFI variables from U-Boot environment
>    efi_loader: load saved non-volatile variables at init
>    efi_loader: bootmgr: handle BootNext as non-volatile
>    cmd: env: add -nv option for UEFI non-volatile variable
>    cmd: efidebug: make some boot variables non-volatile
>
>   cmd/bootefi.c                     |   4 -
>   cmd/efidebug.c                    |  95 +++++++++++-----
>   cmd/nvedit.c                      |   3 +-
>   cmd/nvedit_efi.c                  |  15 ++-
>   env/Kconfig                       |  34 ++++++
>   env/env.c                         |  98 ++++++++++++++++-
>   env/fat.c                         | 109 +++++++++++++++++++
>   include/asm-generic/global_data.h |   1 +
>   include/charset.h                 |  10 ++
>   include/environment.h             |  24 +++++
>   lib/charset.c                     |  23 ++++
>   lib/efi_loader/efi_bootmgr.c      |   3 +-
>   lib/efi_loader/efi_setup.c        |  13 +++
>   lib/efi_loader/efi_variable.c     | 174 ++++++++++++++++++++++++++++--
>   14 files changed, 560 insertions(+), 46 deletions(-)
>

[U-Boot] [PATCH v2 02/11] lib: charset: add u16_strncmp()

[AKASHI Takahiro]

On Wed, Apr 24, 2019 at 08:36:09PM +0200, Heinrich Schuchardt wrote:
> On 4/24/19 8:30 AM, AKASHI Takahiro wrote:
> >u16_strncmp() works like u16_strcmp() but only at most n characters
> >(in u16) are compared.
> >This function will be used in a later patch.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> The only usage of u16_strncmp() is in patch 3.
> u16_strcmp() is not used at all.

The fact is that u16_strcmp() is already used in my 'secure boot' patch
and then I moved on to non-volatile patch.

> In patch 3 'memcmp(var_name16, L"BOOT", 8)' will do the job.
> 
> I am not sure if in other cases we wouldn't prefer to compare Unicode
> codepoints instead of u16.

That is my concern, too :)

> So I suggest to skip patches 1 and 2 and use memcmp() in patch 3.

Okay, patch#1 will be planned to be included in 'secure boot' patch.

-Takahiro Akashi


> Best regards
> 
> Heinrich

[U-Boot] [PATCH v2 03/11] cmd: efidebug: rework "boot dump" sub-command using GetNextVariableName()

[AKASHI Takahiro]

On Wed, Apr 24, 2019 at 10:13:37PM +0200, Heinrich Schuchardt wrote:
> On 4/24/19 8:30 AM, AKASHI Takahiro wrote:
> >Efidebug command should be implemented using well-defined EFI interfaces,
> >rather than using internal functions/data. This change will be needed in
> >a later patch where UEFI variables are re-implemented.
> 
> I had trouble to get the point. In the next version I suggest to write:
> 
> Currently in do_efi_boot_dump() we directly read EFI variables from the
> related environment variables. To accomodate alternative storage
> backends we should switch to using the UEFI API instead.

Okay.

> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  cmd/efidebug.c | 92 ++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 66 insertions(+), 26 deletions(-)
> >
> >diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> >index a40c4f4be286..8890dd7268f1 100644
> >--- a/cmd/efidebug.c
> >+++ b/cmd/efidebug.c
> >@@ -509,7 +509,7 @@ static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
> >  	if (argc < 6 || argc > 7)
> >  		return CMD_RET_USAGE;
> >
> >-	id = (int)simple_strtoul(argv[1], &endp, 16);
> >+	id = simple_strtoul(argv[1], &endp, 16);
> 
> This change is correct but unrelated. Please, put it into a separate
> patch. Or at least mention it in the commit message.

Sure.

> >  	if (*endp != '\0' || id > 0xffff)
> >  		return CMD_RET_USAGE;
> >
> >@@ -595,7 +595,7 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag,
> >
> >  	guid = efi_global_variable_guid;
> >  	for (i = 1; i < argc; i++, argv++) {
> >-		id = (int)simple_strtoul(argv[1], &endp, 16);
> >+		id = simple_strtoul(argv[1], &endp, 16);
> 
> Same here.
> 
> >  		if (*endp != '\0' || id > 0xffff)
> >  			return CMD_RET_FAILURE;
> >
> >@@ -693,6 +693,27 @@ static void show_efi_boot_opt(int id)
> >  	free(data);
> >  }
> >
> >+static bool u16_isxdigit(u16 c)
> >+{
> >+	if (c & 0xff00)
> >+		return false;
> >+
> >+	return isxdigit((u8)c);
> >+}
> >+
> >+static int u16_tohex(u16 c)
> >+{
> >+	if (c >= '0' && c < '9')
> >+		return c - '0';
> >+	if (c >= 'A' && c < 'F')
> >+		return c - 'A' + 10;
> >+	if (c >= 'a' && c < 'f')
> >+		return c - 'a' + 10;
> 
> Does the UEFI spec really allow lower case hexadecimal numbers here?
> I only found an example with capital numbers. Would this imply that I
> could have variables Boot00a0 and Boot00A0 side by side? Which one would
> be selected by boot option 00a0?
> 
> Or should SetVariable() make a case insensitive search for variable names?

Good point. I found the description below in UEFI section 3.1.1:
        Each load option entry resides in a Boot####, Driver####,
        SysPrep####, OsRecovery####
        or PlatformRecovery#### variable where #### is replaced by
        a unique option number in
        printable hexadecimal representation using the digits 0-9,
        and the upper case versions of the
        characters A-F (0000-FFFF).

> In EDK2 function FindVariableEx() in
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> uses CompareMem() to compare variable names.
> 
> Function BmCharToUint() is used to check the digits of boot options and
> has this comment:
> 
> "It assumes the input Char is in the scope of L'0' ~ L'9'
> and L'A' ~ L'F'"
> 
> So let's us assume that variable names are case sensitive and Boot
> entries can only have capital hexadecimal digits.
> 
> So u16_isxdigit() is incorrect.
> 
> >+
> >+	/* dummy */
> >+	return -1;
> 
> As we do not check bounds here the function could be reduced to:
> 
> return c > '9' ? c - ('A' - 0xa) : c - '9';
> 
> But I would prefer one library function instead of the two static
> functions which returns -1 for a non-digit and 0 - 15 for a digit.
> And a unit test in test/unicoode_ut.c

Okay.

> >+}
> >+
> >  /**
> >   * show_efi_boot_dump() - dump all UEFI load options
> >   *
> >@@ -709,38 +730,57 @@ static void show_efi_boot_opt(int id)
> >  static int do_efi_boot_dump(cmd_tbl_t *cmdtp, int flag,
> >  			    int argc, char * const argv[])
> >  {
> >-	char regex[256];
> >-	char * const regexlist[] = {regex};
> >-	char *variables = NULL, *boot, *value;
> >-	int len;
> >-	int id;
> >+	u16 *var_name16, *p;
> >+	efi_uintn_t buf_size, size;
> >+	efi_guid_t guid;
> >+	int id, i;
> >+	efi_status_t ret;
> >
> >  	if (argc > 1)
> >  		return CMD_RET_USAGE;
> >
> >-	snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_Boot[0-9A-F]+");
> >-
> >-	/* TODO: use GetNextVariableName? */
> >-	len = hexport_r(&env_htab, '\n', H_MATCH_REGEX | H_MATCH_KEY,
> >-			&variables, 0, 1, regexlist);
> >+	buf_size = 128;
> >+	var_name16 = malloc(buf_size);
> >+	if (!var_name16)
> >+		return CMD_RET_FAILURE;
> >
> >-	if (!len)
> >-		return CMD_RET_SUCCESS;
> >+	var_name16[0] = 0;
> >+	for (;;) {
> >+		size = buf_size;
> >+		ret = EFI_CALL(efi_get_next_variable_name(&size, var_name16,
> >+							  &guid));
> >+		if (ret == EFI_NOT_FOUND)
> >+			break;
> >+		if (ret == EFI_BUFFER_TOO_SMALL) {
> >+			buf_size = size;
> >+			p = realloc(var_name16, buf_size);
> >+			if (!p) {
> >+				free(var_name16);
> >+				return CMD_RET_FAILURE;
> >+			}
> >+			var_name16 = p;
> >+			ret = EFI_CALL(efi_get_next_variable_name(&size,
> >+								  var_name16,
> >+								  &guid));
> >+		}
> >+		if (ret != EFI_SUCCESS) {
> >+			free(var_name16);
> >+			return CMD_RET_FAILURE;
> >+		}
> >
> >-	if (len < 0)
> >-		return CMD_RET_FAILURE;
> >+		if (u16_strncmp(var_name16, L"Boot", 4) || var_name16[8] ||
> 
> We can use memcmp() here. This avoids introducing u16_strncmp().

Okay.

> >+		    !u16_isxdigit(var_name16[4]) ||
> >+		    !u16_isxdigit(var_name16[5]) ||
> >+		    !u16_isxdigit(var_name16[6]) ||
> >+		    !u16_isxdigit(var_name16[7]))
> >+			continue;
> >
> >-	boot = variables;
> >-	while (*boot) {
> >-		value = strstr(boot, "Boot") + 4;
> >-		id = (int)simple_strtoul(value, NULL, 16);
> >+		for (id = 0, i = 0; i < 4; i++)
> >+			id = (id << 4) + u16_tohex(var_name16[4 + i]);
> 
> This all can be simplified if we unify u16_isxdigit() and u16_tohex().
> Just one function returning -1 if not a hexadecimal and 0 - 15 otherwise.

Will manage that.

> >  		show_efi_boot_opt(id);
> >-		boot = strchr(boot, '\n');
> >-		if (!*boot)
> >-			break;
> >-		boot++;
> >  	}
> >-	free(variables);
> >+
> >+	free(var_name16);
> >
> >  	return CMD_RET_SUCCESS;
> >  }
> >@@ -914,7 +954,7 @@ static int do_efi_boot_order(cmd_tbl_t *cmdtp, int flag,
> >  		return CMD_RET_FAILURE;
> >
> >  	for (i = 0; i < argc; i++) {
> >-		id = (int)simple_strtoul(argv[i], &endp, 16);
> >+		id = simple_strtoul(argv[i], &endp, 16);
> 
> This change is correct but unrelated. Please, put it into a separate
> patch. Or at least mention it in the commit message.

Okay

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >  		if (*endp != '\0' || id > 0xffff) {
> >  			printf("invalid value: %s\n", argv[i]);
> >  			ret = CMD_RET_FAILURE;
> >
> 

[U-Boot] [PATCH v2 01/11] lib: charset: add u16_strcmp()

[AKASHI Takahiro]

On Wed, Apr 24, 2019 at 06:24:44PM +0200, Heinrich Schuchardt wrote:
> On 4/24/19 8:30 AM, AKASHI Takahiro wrote:
> >u16 version of strcmp()
> >
> >AUTHER: Patrick Wildt <patrick@blueri.se>
> 
> %s/AUTHER/Author/

Okay

> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  include/charset.h |  5 +++++
> >  lib/charset.c     | 10 ++++++++++
> >  2 files changed, 15 insertions(+)
> >
> >diff --git a/include/charset.h b/include/charset.h
> >index 65087f76d1fc..747a9b376c03 100644
> >--- a/include/charset.h
> >+++ b/include/charset.h
> >@@ -166,6 +166,11 @@ s32 utf_to_lower(const s32 code);
> >   */
> >  s32 utf_to_upper(const s32 code);
> >
> >+/*
> >+ * u16_strcmp() - strcmp() for u16 strings
> >+ */
> >+int u16_strcmp(const u16 *s1, const u16 *s2);
> >+
> >  /**
> >   * u16_strlen - count non-zero words
> >   *
> >diff --git a/lib/charset.c b/lib/charset.c
> >index 5e349ed5ee45..4a25ac0bdb9c 100644
> >--- a/lib/charset.c
> >+++ b/lib/charset.c
> >@@ -335,6 +335,16 @@ s32 utf_to_upper(const s32 code)
> >  	return ret;
> >  }
> >
> >+int u16_strcmp(const u16 *s1, const u16 *s2)
> >+{
> >+	while (*s1 == *s2++)
> >+		if (*s1++ == 0)
> >+			return (0);
> >+	--s2;
> 
> for (;*s1 == *s2; ++s1, ++s2)
> 	if (!s1)
> 		return 0;
> 
> does the same job without superfluous increment/decrement.

Indeed :)

> >+
> >+	return (*(uint16_t *)s1 - *(uint16_t *)s2);
> 
> Why would you use both u16 and uint16_t in the same function?
> You can do without any conversion here.

Will fix.

> How about
> 
> #define u16_strcmp(s1, s2) u16_strncmp(s1, s2, SIZE_MAX)
> 
> like we do for the other string functions?

Sure

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >+}
> >+
> >  size_t u16_strlen(const u16 *in)
> >  {
> >  	size_t i;
> >
> 

[U-Boot] [PATCH v2 00/11] efi_loader: non-volatile variables support

[AKASHI Takahiro]

On Thu, Apr 25, 2019 at 12:12:39AM +0200, Heinrich Schuchardt wrote:
> On 4/24/19 8:30 AM, AKASHI Takahiro wrote:
> >This patch set is an attempt to implement non-volatile attribute for
> >UEFI variables. Under the current implementation,
> >* SetVariable API doesn't recognize non-volatile attribute
> >* While some variables are defined non-volatile in UEFI specification,
> >   they are NOT marked as non-volatile in the code.
> >* env_save() (or "env save" command) allows us to save all the variables
> >   into persistent storage, but it may cause volatile UEFI variables,
> >   along with irrelevant U-Boot variables, to be saved unconditionally.
> >
> >Those observation rationalizes that the implementation of UEFI variables
> >should be revamped utilizing dedicated storage for them.
> >
> >This patch set is yet experimental and rough-edged(See known issues below),
> >but shows how UEFI variables can be split from U-Boot environment.
> >This enhancement will also be vital when we introduce UEFI secure boot
> >where secure and tamper-resistant storage (with authentication) is
> >required.
> >
> >Usage:
> >To enable this feature, the following configs must be enabled:
> >   CONFIG_ENV_IS_IN_FAT
> >   CONFIG_ENV_FAT_INTERFACE
> >   CONFIG_ENV_EFI_FAT_DEVICE_AND_PART
> >   CONFIG_ENV_EFI_FAT_FILE
> >
> >You can also define a non-volatile variable from command interface:
> >=> setenv -e -nv FOO baa
> >
> >Known issues/restriction:
> >* UEFI spec defines "globally defined variables" with specific
> >   attributes, but with this patch, we don't check against the user-supplied
> >   attribute for any variable.
> >* Only FAT can be enabled for persistent storage for UEFI non-volatile
> >   variables.
> >* The whole area of storage will be saved at every update of one variable.
> >   It can be optimized.
> >* An error during saving may cause inconsistency between cache (hash table)
> >   and the storage.
> >* Cache is of fixed size and can be quite big for normal usage.
> 
> Hello Takahiro,
> 
> thanks a lot for looking into persisting non-volatile UEFI variables.
> 
> Before diving into the details of the patches let us discuss the overall
> design.

I like this kind of discussions as this patch set is more or less RFC.

> My understanding is that we need a buffer that lives in
> EFI_RUNTIME_SERVICES_DATA and holds all variables. It is this buffer
> that the operating system will access when getting or setting variables.
> 
> If a non-volatile variable is set via SetVariable() we will call a
> backend driver.

Get/SetVariable() is not mandatory in runtime services, and EBBR,
at least, says:
        If any EFI_RUNTIME_SERVICES functions are only available during
        boot services then firmware shall provide
        the global RuntimeServicesSupported variable to indicate which
        functions are available during runtime services.
        (in section 2.5), and
        Even when SetVariable() is not supported during runtime services,
        firmware should cache variable names and
        values in EfiRuntimeServicesData memory so that GetVariable() and
        GetNextVeriableName() can behave as specified.
        (in section 2.5.3)

# Variable update can also be done via 'capsule.'

> Our current backend is using U-Boot environment variables. It can only
> be accessed at boottime.

Right.

> Currently there is no guarantee that the U-Boot
> variables are saved before booting.

Right, but if 'saved before booting' were the only issue,
we could call env_save() in efi_start_image().
The problems are not there.

> So it does not support persisting
> non-volatile variables reliably.
> 
> Your patch series supplies a new backend using a file for persisting
> non-volatile variables. It can only be accessed at boottime. Essentially
> this solution requires no restriction to FAT file systems. We could as
> well use the EXT4 driver for reading or writing the file.

Right, we can also use flash, nand, emmc and others like U-Boot environment
as we still reply on helper functions for U-Boot environment.

> This file
> storage is completely independent of the U-Boot environment.

No. We don't have to care about format in persistent storage.
What is required is that we should provide Get/SetVariable APIs.

> So it shall
> not involve any changes to files env/*. I think a documentation of the
> file format provided in a README would be helpful.
> 
> In case of both backends we would return EFI_UNSUPPORTED when trying to
> set a non-volatile variable at runtime. Setting a volatile variable at
> runtime should be allowable as long as we have sufficient space in our
> buffer.

Well, there are several possibilities:
1. We don't support Get/SetVariable in runtime at all
2. GetVariable is available through 'cache' (U-Boot environment uses
   a on-memory hash tables to maintain variables.)
   Optionally, SetVariable can be achieved via capsule.
3. Get/SetVariable is implemented by an independent entity and gets
   avaiable even in runtime.
   This will be true if persistent storage is provided by secure world,
   in particular EL3 on arm. It will be quite likely for secure boot.

> We can remove these backends based on the EVT_SIGNAL_EXIT_BOOT_SERVICES
> event.
> 
> A future backend may support persisting non-volatile variables at runtime.
> 
> I would prefer if we could first introduce patches that provide the
> buffer for EFI variables and link it to the UEFI variable based backend
> before adding the new backend as alternative.

I'm not sure whether I understand your point fully, but
my current patch does that, doesn't it?
# We have to implement (2) above yet.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> 
> >
> >Patch#1 to #4 are preparatory so that we won't rely on U-Boot environment,
> >that is, env_get/set() helper functions.
> >Patch#5 to #8 are core part of changes.
> >Patch#9 to #11 are for modifying variable attributes.
> >
> >Changes in v2 (Apr 24, 2019)
> >* rebased on efi-2019-07
> >* revamp the implementation
> >
> >v1 (Nov 28, 2018)
> >* initial
> >
> >AKASHI Takahiro (11):
> >   lib: charset: add u16_strcmp()
> >   lib: charset: add u16_strncmp()
> >   cmd: efidebug: rework "boot dump" sub-command using
> >     GetNextVariableName()
> >   efi_loader: set OsIndicationsSupported at init
> >   env: save UEFI non-volatile variables in dedicated storage
> >   efi_loader: variable: support non-volatile attribute
> >   efi_loader: variable: split UEFI variables from U-Boot environment
> >   efi_loader: load saved non-volatile variables at init
> >   efi_loader: bootmgr: handle BootNext as non-volatile
> >   cmd: env: add -nv option for UEFI non-volatile variable
> >   cmd: efidebug: make some boot variables non-volatile
> >
> >  cmd/bootefi.c                     |   4 -
> >  cmd/efidebug.c                    |  95 +++++++++++-----
> >  cmd/nvedit.c                      |   3 +-
> >  cmd/nvedit_efi.c                  |  15 ++-
> >  env/Kconfig                       |  34 ++++++
> >  env/env.c                         |  98 ++++++++++++++++-
> >  env/fat.c                         | 109 +++++++++++++++++++
> >  include/asm-generic/global_data.h |   1 +
> >  include/charset.h                 |  10 ++
> >  include/environment.h             |  24 +++++
> >  lib/charset.c                     |  23 ++++
> >  lib/efi_loader/efi_bootmgr.c      |   3 +-
> >  lib/efi_loader/efi_setup.c        |  13 +++
> >  lib/efi_loader/efi_variable.c     | 174 ++++++++++++++++++++++++++++--
> >  14 files changed, 560 insertions(+), 46 deletions(-)
> >
> 

[U-Boot] [PATCH v2 05/11] env: save UEFI non-volatile variables in dedicated storage

[Heinrich Schuchardt]

On 4/24/19 8:30 AM, AKASHI Takahiro wrote:
> We need a variant of env_save()/env_load() to handle dedicated storage
> for UEFI variables.
> It is assumed that env_efi_load() will be called only ince at init

%s/ince/once/

> and that env_efi_save() will be called at every SetVariable.
>
> In this patch, new parameters will be expected to be configured:
>    CONFIG_ENV_EFI_FAT_DEVICE_AND_PART
>    CONFIG_ENV_EFI_FAT_FILE
> in case of CONFIG_ENV_IS_IN_FAT.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   env/Kconfig                       |  34 ++++++++++
>   env/env.c                         |  98 ++++++++++++++++++++++++++-
>   env/fat.c                         | 109 ++++++++++++++++++++++++++++++
>   include/asm-generic/global_data.h |   1 +
>   include/environment.h             |  24 +++++++
>   5 files changed, 265 insertions(+), 1 deletion(-)
>
> diff --git a/env/Kconfig b/env/Kconfig
> index 78300660c720..8f59e9347d4b 100644
> --- a/env/Kconfig
> +++ b/env/Kconfig
> @@ -438,6 +438,34 @@ config ENV_FAT_FILE
>   	  It's a string of the FAT file name. This file use to store the
>   	  environment.
>
> +config ENV_EFI_FAT_DEVICE_AND_PART

API wise there is no reason why we should prefer FAT over other
file-systemes.

> +	string "Device and partition for where to store the UEFI non-volatile variables in FAT"
> +	depends on ENV_IS_IN_FAT

It should be possible to store the U-Boot variable on a different medium
than the EFI variable.

> +	depends on EFI_LOADER
> +	help
> +	  Define this to a string to specify the partition of the device. It can
> +	  be as following:
> +
> +	    "D:P", "D:0", "D", "D:" or "D:auto" (D, P are integers. And P >= 1)
> +	       - "D:P": device D partition P. Error occurs if device D has no
> +	                partition table.
> +	       - "D:0": device D.
> +	       - "D" or "D:": device D partition 1 if device D has partition
> +	                      table, or the whole device D if has no partition
> +	                      table.
> +	       - "D:auto": first partition in device D with bootable flag set.
> +	                   If none, first valid partition in device D. If no
> +	                   partition table then means device D.
> +
> +config ENV_EFI_FAT_FILE
> +	string "Name of the FAT file to use for the UEFI non-volatile variables"
> +	depends on ENV_IS_IN_FAT
> +	depends on EFI_LOADER
> +	default "uboot_efi.env"
> +	help
> +	  It's a string of the FAT file name. This file use to store the
> +	  UEFI non-volatile variables.
> +
>   config ENV_EXT4_INTERFACE
>   	string "Name of the block device for the environment"
>   	depends on ENV_IS_IN_EXT4
> @@ -470,6 +498,12 @@ config ENV_EXT4_FILE
>   	  It's a string of the EXT4 file name. This file use to store the
>   	  environment (explicit path to the file)
>
> +config ENV_EFI_SIZE
> +	hex "UEFI Variables Environment Size"
> +	default 0x20000
> +	help
> +	  Size of the UEFI variables storage area
> +
>   if ARCH_ROCKCHIP || ARCH_SUNXI || ARCH_ZYNQ || ARCH_ZYNQMP || ARCH_VERSAL || ARC
>
>   config ENV_OFFSET
> diff --git a/env/env.c b/env/env.c
> index 4b417b90a291..d5af761ba57e 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -24,6 +24,10 @@ void env_fix_drivers(void)
>   			entry->load += gd->reloc_off;
>   		if (entry->save)
>   			entry->save += gd->reloc_off;
> +		if (entry->efi_load)
> +			entry->efi_load += gd->reloc_off;
> +		if (entry->efi_save)
> +			entry->efi_save += gd->reloc_off;
>   		if (entry->init)
>   			entry->init += gd->reloc_off;
>   	}
> @@ -125,7 +129,8 @@ __weak enum env_location env_get_location(enum env_operation op, int prio)
>   	if (prio >= ARRAY_SIZE(env_locations))
>   		return ENVL_UNKNOWN;
>
> -	gd->env_load_prio = prio;
> +	if (op != ENVOP_EFI)
> +		gd->env_load_prio = prio;
>
>   	return env_locations[prio];
>   }
> @@ -280,3 +285,94 @@ int env_init(void)
>
>   	return ret;
>   }
> +
> +#ifdef CONFIG_EFI_LOADER
> +extern struct hsearch_data efi_var_htab;
> +extern struct hsearch_data efi_nv_var_htab;
> +
> +/* TODO: experimental */
> +int env_efi_save(void)

This function should be __efi_runtime.

> +{
> +	struct env_driver *drv = NULL;
> +	int ret;
> +
> +	if (!efi_nv_var_htab.table)
> +		return 0;
> +
> +	if (gd->env_efi_prio == -1) {
> +		printf("Cannot save UEFI non-volatile variable\n");
> +		return -1;
> +	}
> +
> +	drv = _env_driver_lookup(env_get_location(ENVOP_EFI, gd->env_efi_prio));

Some drivers will support writing at runtime some will not.

I think at initialization you should decide on the one and only
environment driver. And the link to it should be shored in an
__efi_runtime variable.

> +	if (!drv) {
> +		printf("Cannot save UEFI non-volatile variable\n");
> +		return -1;
> +	}
> +
> +	ret = drv->efi_save();
> +	if (ret)
> +		printf("Saving UEFI non-volatile variable failed\n");
> +
> +	return ret;
> +}
> +
> +/* TODO: experimental */
> +/* This function should be called only once at init */
> +int env_efi_load(void)

This function needs to be __efi_runtime.

> +{
> +	struct env_driver *drv = NULL;
> +	int prio, ret;
> +	enum env_location loc;
> +
> +	gd->env_efi_prio = -1;
> +
> +	/* volatile variables */
> +	if (!efi_var_htab.table) {
> +		ret = himport_r(&efi_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
> +		if (!ret) {
> +			printf("Creating default UEFI variables failed\n");
> +			return -1;
> +		}
> +	}
> +
> +	/* non-loratile variables */
> +	if (efi_nv_var_htab.table)
> +		return 0;
> +
> +	for (prio = 0; prio < ARRAY_SIZE(env_locations); prio++) {
> +		loc = env_get_location(ENVOP_EFI, prio);
> +		drv = _env_driver_lookup(loc);
> +		if (!drv)
> +			continue;
> +
> +		if (drv->efi_load && drv->efi_save)
> +			break;
> +	}
> +	if (!drv || prio == ARRAY_SIZE(env_locations)) {
> +		printf("No driver for UEFI storage is available\n");
> +		return -1;
> +	}
> +
> +	gd->env_efi_prio = prio;
> +
> +	ret = drv->efi_load();
> +	if (ret) {
> +		printf("Loading UEFI non-volatile variables failed\n");
> +		return -1;
> +	}
> +
> +	/* FIXME: init needed here? */
> +	if (!efi_nv_var_htab.table) {
> +		ret = himport_r(&efi_nv_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
> +		if (!ret) {
> +			printf("Creating default UEFI non-volatile variables failed\n");
> +			return -1;
> +		}
> +
> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +#endif /* CONFIG_EFI_LOADER */
> diff --git a/env/fat.c b/env/fat.c
> index 7f74c64dfe7e..e2b050d4553d 100644
> --- a/env/fat.c
> +++ b/env/fat.c
> @@ -14,6 +14,7 @@
>   #include <malloc.h>
>   #include <memalign.h>
>   #include <search.h>
> +#include <efi_loader.h>
>   #include <errno.h>
>   #include <fat.h>
>   #include <mmc.h>
> @@ -128,6 +129,110 @@ err_env_relocate:
>   }
>   #endif /* LOADENV */
>
> +#ifdef CONFIG_EFI_LOADER
> +#if 1
> +extern int efi_variable_import(const char *buf, int check);
> +extern int efi_variable_export(env_t *env_out);
> +#endif
> +static int env_fat_efi_save(void)

Please, define a u-class for the different EFI variable stores.
Currently there should be two of them:

- u-boot variables
- EFI env file (on any block device supporting file writes)

The driver should provide an indication if it is supporting runtime access.

> +{
> +	env_t __aligned(ARCH_DMA_MINALIGN) env_new;
> +	struct blk_desc *dev_desc = NULL;
> +	disk_partition_t info;
> +	int dev, part;
> +	int err;
> +	loff_t size;
> +
> +	err = efi_variable_export(&env_new);
> +	if (err)
> +		return err;
> +
> +	part = blk_get_device_part_str(CONFIG_ENV_FAT_INTERFACE,
> +				       CONFIG_ENV_EFI_FAT_DEVICE_AND_PART,
> +				       &dev_desc, &info, 1);
> +	if (part < 0)
> +		return 1;
> +
> +	dev = dev_desc->devnum;
> +	if (fat_set_blk_dev(dev_desc, &info) != 0) {
> +		/*
> +		 * This printf is embedded in the messages from env_save that
> +		 * will calling it. The missing \n is intentional.
> +		 */
> +		printf("Unable to use %s %d:%d... ",
> +		       CONFIG_ENV_FAT_INTERFACE, dev, part);
> +		return 1;
> +	}
> +
> +	err = file_fat_write(CONFIG_ENV_EFI_FAT_FILE,
> +			     (void *)&env_new, 0, sizeof(env_t), &size);
> +	if (err < 0) {
> +		/*
> +		 * This printf is embedded in the messages from env_save that
> +		 * will calling it. The missing \n is intentional.
> +		 */
> +		printf("Unable to write \"%s\" from %s%d:%d... ",
> +		       CONFIG_ENV_EFI_FAT_FILE, CONFIG_ENV_FAT_INTERFACE,
> +		       dev, part);
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int env_fat_efi_load(void)
> +{
> +	ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_EFI_SIZE);
> +	struct blk_desc *dev_desc = NULL;
> +	disk_partition_t info;
> +	int dev, part;
> +	int err;
> +
> +#ifdef CONFIG_MMC
> +	if (!strcmp(CONFIG_ENV_FAT_INTERFACE, "mmc"))
> +		mmc_initialize(NULL);
> +#endif
> +
> +	part = blk_get_device_part_str(CONFIG_ENV_FAT_INTERFACE,
> +				       CONFIG_ENV_EFI_FAT_DEVICE_AND_PART,
> +				       &dev_desc, &info, 1);
> +	if (part < 0)
> +		goto err_env_relocate;
> +
> +	dev = dev_desc->devnum;
> +	if (fat_set_blk_dev(dev_desc, &info) != 0) {
> +		/*
> +		 * This printf is embedded in the messages from env_save that
> +		 * will calling it. The missing \n is intentional.
> +		 */
> +		printf("Unable to use %s %d:%d...\n",
> +		       CONFIG_ENV_FAT_INTERFACE, dev, part);
> +		goto err_env_relocate;
> +	}
> +
> +	err = file_fat_read(CONFIG_ENV_EFI_FAT_FILE, buf, CONFIG_ENV_EFI_SIZE);
> +	if (err <= 0 && (err != -ENOENT)) {
> +		/*
> +		 * This printf is embedded in the messages from env_save that
> +		 * will calling it. The missing \n is intentional.
> +		 */
> +		printf("Unable to read \"%s\" from %s %d:%d...\n",
> +		       CONFIG_ENV_EFI_FAT_FILE, CONFIG_ENV_FAT_INTERFACE,
> +		       dev, part);
> +		goto err_env_relocate;
> +	}
> +
> +	if (err > 0)
> +		return efi_variable_import(buf, 1);
> +	else
> +		return 0;
> +
> +err_env_relocate:
> +
> +	return -EIO;
> +}
> +#endif /* CONFIG_EFI_LOADER */
> +
>   U_BOOT_ENV_LOCATION(fat) = {
>   	.location	= ENVL_FAT,
>   	ENV_NAME("FAT")
> @@ -137,4 +242,8 @@ U_BOOT_ENV_LOCATION(fat) = {
>   #ifdef CMD_SAVEENV
>   	.save		= env_save_ptr(env_fat_save),
>   #endif
> +#ifdef CONFIG_EFI_LOADER
> +	.efi_load	= env_fat_efi_load,
> +	.efi_save	= env_fat_efi_save,
> +#endif
>   };
> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> index 78dcf40bff48..8c4d56c346f3 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -51,6 +51,7 @@ typedef struct global_data {
>   	unsigned long env_valid;	/* Environment valid? enum env_valid */
>   	unsigned long env_has_init;	/* Bitmask of boolean of struct env_location offsets */
>   	int env_load_prio;		/* Priority of the loaded environment */
> +	int env_efi_prio;		/* Priority of the UEFI variables */
>
>   	unsigned long ram_base;		/* Base address of RAM used by U-Boot */
>   	unsigned long ram_top;		/* Top address of RAM used by U-Boot */
> diff --git a/include/environment.h b/include/environment.h
> index cd966761416e..f62399863ef9 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -200,6 +200,7 @@ enum env_operation {
>   	ENVOP_INIT,	/* we want to call the init function */
>   	ENVOP_LOAD,	/* we want to call the load function */
>   	ENVOP_SAVE,	/* we want to call the save function */
> +	ENVOP_EFI,	/* we want to call the efi_load/save function */
>   };
>
>   struct env_driver {
> @@ -225,6 +226,24 @@ struct env_driver {

If you would want to reuse the env driver you could simply change load()
and save() to accept a filename as parameter.

Buf why mess with env_driver?

I think the EFI variable driver can simply use the functions provided in
include/fs.h: fs_set_blk_dev(), fs_write(), fs_read().

Best regards

Heinrich

>   	 */
>   	int (*save)(void);
>
> +	/**
> +	 * efi_load() - Load UEFI non-volatile variables from storage
> +	 *
> +	 * This method is required for UEFI non-volatile variables
> +	 *
> +	 * @return 0 if OK, -ve on error
> +	 */
> +	int (*efi_load)(void);
> +
> +	/**
> +	 * efi_save() - Save UEFI non-volatile variables to storage
> +	 *
> +	 * This method is required for UEFI non-volatile variables
> +	 *
> +	 * @return 0 if OK, -ve on error
> +	 */
> +	int (*efi_save)(void);
> +
>   	/**
>   	 * init() - Set up the initial pre-relocation environment
>   	 *
> @@ -312,6 +331,11 @@ void eth_parse_enetaddr(const char *addr, uint8_t *enetaddr);
>   int eth_env_get_enetaddr(const char *name, uint8_t *enetaddr);
>   int eth_env_set_enetaddr(const char *name, const uint8_t *enetaddr);
>
> +#ifdef CONFIG_EFI_LOADER
> +int env_efi_load(void);
> +int env_efi_save(void);
> +#endif
> +
>   #endif /* DO_DEPS_ONLY */
>
>   #endif /* _ENVIRONMENT_H_ */
>