[U-Boot] [PATCH v3 0/7] efi_loader: non-volatile variables support

[U-Boot] [PATCH v3 0/7] 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.

Note:
When StMM services is supported as a backing variables storage, 
efi_get_variable/efi_get_next_variable_name/efi_set_variable() will
be replaced with stub functions to communicate with secure world.

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/TODO:
* 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.
* I still see some Travis CI errors.

Patch#1 to #4 are core part of changes.
Patch#5 to #6 are to change some existing variables' attributes.
Patch#7 is to extend env command for non-volatile variables.

Changes in v3 (Jun 4, 2019)
* remove already-merged patches
* revamp the code again
* introduce CONFIG_EFI_VARIABLE_USE_ENV for this configuration.
  Once another backing storage, i.e. StMM services for secure boot,
  is supported, another option will be added.

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

v1 (Nov 28, 2018)
* initial

AKASHI Takahiro (7):
  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: make BootNext non-volatile
  cmd: efidebug: make some boot variables non-volatile
  cmd: env: add -nv option for UEFI non-volatile variable

 cmd/efidebug.c                    |   3 +
 cmd/nvedit.c                      |   3 +-
 cmd/nvedit_efi.c                  |  15 +-
 env/Kconfig                       |  39 ++++
 env/env.c                         | 155 ++++++++++++-
 env/fat.c                         | 105 +++++++++
 include/asm-generic/global_data.h |   3 +
 include/environment.h             |  31 +++
 lib/efi_loader/Kconfig            |  10 +
 lib/efi_loader/efi_bootmgr.c      |   3 +-
 lib/efi_loader/efi_setup.c        |   6 +
 lib/efi_loader/efi_variable.c     | 354 +++++++++++++++++++++++-------
 12 files changed, 641 insertions(+), 86 deletions(-)

-- 
2.21.0

[U-Boot] [PATCH v3 1/7] 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                       |  39 ++++++++
 env/env.c                         | 155 +++++++++++++++++++++++++++++-
 env/fat.c                         | 105 ++++++++++++++++++++
 include/asm-generic/global_data.h |   3 +
 include/environment.h             |  31 ++++++
 5 files changed, 332 insertions(+), 1 deletion(-)

diff --git a/env/Kconfig b/env/Kconfig
index 1e10c7a4c46b..a36b6ee48f10 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -399,6 +399,10 @@ config ENV_IS_IN_UBI
 	  the environment in.  This will enable redundant environments in UBI.
 	  It is assumed that both volumes are in the same MTD partition.
 
+config ENV_EFI
+	bool
+	default n
+
 config ENV_FAT_INTERFACE
 	string "Name of the block device for the environment"
 	depends on ENV_IS_IN_FAT
@@ -438,6 +442,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 ENV_EFI
+	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 ENV_EFI
+	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 +502,13 @@ 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"
+	depends on ENV_EFI
+	default 0x20000
+	help
+	  Size of the UEFI variables storage area
+
 if ARCH_ROCKCHIP || ARCH_SUNXI || ARCH_ZYNQ || ARCH_ZYNQMP || ARCH_VERSAL || ARC || ARCH_STM32MP
 
 config ENV_OFFSET
diff --git a/env/env.c b/env/env.c
index 4b417b90a291..202079f6d4c0 100644
--- a/env/env.c
+++ b/env/env.c
@@ -24,6 +24,12 @@ void env_fix_drivers(void)
 			entry->load += gd->reloc_off;
 		if (entry->save)
 			entry->save += gd->reloc_off;
+#ifdef CONFIG_ENV_EFI
+		if (entry->efi_load)
+			entry->efi_load += gd->reloc_off;
+		if (entry->efi_save)
+			entry->efi_save += gd->reloc_off;
+#endif
 		if (entry->init)
 			entry->init += gd->reloc_off;
 	}
@@ -125,7 +131,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 +287,149 @@ int env_init(void)
 
 	return ret;
 }
+
+#ifdef CONFIG_ENV_EFI
+struct hsearch_data efi_var_htab;
+struct hsearch_data efi_nv_var_htab;
+
+int env_efi_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 - ENV_HEADER_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 - ENV_HEADER_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;
+}
+
+int env_efi_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 - ENV_HEADER_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 - ENV_HEADER_SIZE);
+
+	return 0;
+}
+
+int env_efi_save(void)
+{
+#ifdef CONFIG_ENV_IS_NOWHERE
+	return 0;
+#else
+	struct env_driver *drv = NULL;
+	int ret;
+
+	if (!efi_nv_var_htab.table)
+		return 0;
+
+	if (gd->env_efi_prio == -1) {
+		pr_warn("No UEFI non-volatile variable storage\n");
+		return -1;
+	}
+
+	drv = _env_driver_lookup(env_get_location(ENVOP_EFI, gd->env_efi_prio));
+	if (!drv) {
+		pr_warn("No UEFI non-volatile variable storage\n");
+		return -1;
+	}
+
+	ret = drv->efi_save();
+	if (ret)
+		pr_err("Saving UEFI non-volatile variable failed\n");
+
+	return ret;
+#endif
+}
+
+/* This function should be called only once at init */
+int env_efi_load(void)
+{
+#ifndef CONFIG_ENV_IS_NOWHERE
+	struct env_driver *drv;
+	int prio;
+	enum env_location loc;
+#endif
+	int ret;
+
+	/* volatile variables */
+	if (!efi_var_htab.table) {
+		ret = himport_r(&efi_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
+		if (!ret) {
+			pr_err("Creating UEFI volatile variables failed\n");
+			return -1;
+		}
+	}
+
+#ifndef CONFIG_ENV_IS_NOWHERE
+	gd->env_efi_prio = -1;
+
+	/* non-volatile variables */
+	if (efi_nv_var_htab.table)
+		return 0;
+
+	for (drv = NULL, 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)) {
+		pr_warn("No UEFI non-volatile variable storage\n");
+		goto skip_load;
+	}
+
+	gd->env_efi_prio = prio;
+
+	ret = drv->efi_load();
+	if (ret) {
+		pr_err("Loading UEFI non-volatile variables failed\n");
+		return -1;
+	}
+skip_load:
+#endif /* CONFIG_ENV_IS_NOWHERE */
+
+	if (!efi_nv_var_htab.table) {
+		ret = himport_r(&efi_nv_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
+		if (!ret) {
+			pr_err("Creating UEFI non-volatile variables failed\n");
+			return -1;
+		}
+
+		return 0;
+	}
+
+	return 0;
+}
+#endif /* CONFIG_ENV_EFI */
diff --git a/env/fat.c b/env/fat.c
index 7f74c64dfe7e..7cd6dc51baea 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,106 @@ err_env_relocate:
 }
 #endif /* LOADENV */
 
+#ifdef CONFIG_ENV_EFI
+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 = env_efi_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 env_efi_import(buf, 1);
+	else
+		return 0;
+
+err_env_relocate:
+
+	return -EIO;
+}
+#endif /* CONFIG_ENV_EFI */
+
 U_BOOT_ENV_LOCATION(fat) = {
 	.location	= ENVL_FAT,
 	ENV_NAME("FAT")
@@ -137,4 +238,8 @@ U_BOOT_ENV_LOCATION(fat) = {
 #ifdef CMD_SAVEENV
 	.save		= env_save_ptr(env_fat_save),
 #endif
+#ifdef CONFIG_ENV_EFI
+	.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 02a3ed683821..5ed390cc31c7 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -52,6 +52,9 @@ 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 */
+#ifdef CONFIG_ENV_EFI
+	int env_efi_prio;		/* Priority of the UEFI variables */
+#endif
 
 	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..32a03014690f 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,26 @@ struct env_driver {
 	 */
 	int (*save)(void);
 
+#ifdef CONFIG_ENV_EFI
+	/**
+	 * 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);
+#endif
+
 	/**
 	 * init() - Set up the initial pre-relocation environment
 	 *
@@ -312,6 +333,16 @@ 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_ENV_EFI
+extern struct hsearch_data efi_var_htab;
+extern struct hsearch_data efi_nv_var_htab;
+
+int env_efi_import(const char *buf, int check);
+int env_efi_export(env_t *env_out);
+int env_efi_load(void);
+int env_efi_save(void);
+#endif
+
 #endif /* DO_DEPS_ONLY */
 
 #endif /* _ENVIRONMENT_H_ */
-- 
2.21.0

[U-Boot] [PATCH v3 2/7] 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 50bc10537f40..e56053194dae 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"))) {
@@ -468,7 +470,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;
@@ -480,12 +482,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.21.0

[U-Boot] [PATCH v3 3/7] 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/Kconfig        |  10 +
 lib/efi_loader/efi_variable.c | 342 ++++++++++++++++++++++++++--------
 2 files changed, 275 insertions(+), 77 deletions(-)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index cd5436c576b1..8bf4b1754d06 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -18,6 +18,16 @@ config EFI_LOADER
 
 if EFI_LOADER
 
+choice
+	prompt "Select variables storage"
+	default EFI_VARIABLE_USE_ENV
+
+config EFI_VARIABLE_USE_ENV
+	bool "Same device as U-Boot environment"
+	select ENV_EFI
+
+endchoice
+
 config EFI_GET_TIME
 	bool "GetTime() runtime service"
 	depends on DM_RTC
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index e56053194dae..d9887be938c2 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -48,6 +48,66 @@
  * 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.
+ *
+ *	struct hsearch_data efi_var_htab
+ *	struct hsearch_data efi_nv_var_htab
+ */
+
+static char *env_efi_get(const char *name, bool is_non_volatile)
+{
+	struct hsearch_data *htab;
+	ENTRY e, *ep;
+
+	/* WATCHDOG_RESET(); */
+
+	if (is_non_volatile)
+		htab = &efi_nv_var_htab;
+	else
+		htab = &efi_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,
+		       bool is_non_volatile)
+{
+	struct hsearch_data *htab;
+	ENTRY e, *ep;
+	int ret;
+
+	if (is_non_volatile)
+		htab = &efi_nv_var_htab;
+	else
+		htab = &efi_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;
+}
+
 #define PREFIX_LEN (strlen("efi_xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx_"))
 
 /**
@@ -147,24 +207,12 @@ static const char *parse_attr(const char *str, u32 *attrp)
 	return str;
 }
 
-/**
- * efi_efi_get_variable() - retrieve value of a UEFI variable
- *
- * This function implements the GetVariable runtime service.
- *
- * See the Unified Extensible Firmware Interface (UEFI) specification for
- * details.
- *
- * @variable_name:	name of the variable
- * @vendor:		vendor GUID
- * @attributes:		attributes of the variable
- * @data_size:		size of the buffer to which the variable value is copied
- * @data:		buffer to which the variable value is copied
- * Return:		status code
- */
-efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
-				     const efi_guid_t *vendor, u32 *attributes,
-				     efi_uintn_t *data_size, void *data)
+static
+efi_status_t EFIAPI efi_get_variable_common(u16 *variable_name,
+					    const efi_guid_t *vendor,
+					    u32 *attributes,
+					    efi_uintn_t *data_size, void *data,
+					    bool is_non_volatile)
 {
 	char *native_name;
 	efi_status_t ret;
@@ -172,22 +220,19 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
 	const char *val, *s;
 	u32 attr;
 
-	EFI_ENTRY("\"%ls\" %pUl %p %p %p", variable_name, vendor, attributes,
-		  data_size, data);
-
 	if (!variable_name || !vendor || !data_size)
 		return EFI_EXIT(EFI_INVALID_PARAMETER);
 
 	ret = efi_to_native(&native_name, variable_name, vendor);
 	if (ret)
-		return EFI_EXIT(ret);
+		return ret;
 
 	EFI_PRINT("get '%s'\n", native_name);
 
-	val = env_get(native_name);
+	val = env_efi_get(native_name, is_non_volatile);
 	free(native_name);
 	if (!val)
-		return EFI_EXIT(EFI_NOT_FOUND);
+		return EFI_NOT_FOUND;
 
 	val = parse_attr(val, &attr);
 
@@ -198,7 +243,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
 
 		/* number of hexadecimal digits must be even */
 		if (len & 1)
-			return EFI_EXIT(EFI_DEVICE_ERROR);
+			return EFI_DEVICE_ERROR;
 
 		/* two characters per byte: */
 		len /= 2;
@@ -210,10 +255,10 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
 		}
 
 		if (!data)
-			return EFI_EXIT(EFI_INVALID_PARAMETER);
+			return EFI_INVALID_PARAMETER;
 
 		if (hex2bin(data, s, len))
-			return EFI_EXIT(EFI_DEVICE_ERROR);
+			return EFI_DEVICE_ERROR;
 
 		EFI_PRINT("got value: \"%s\"\n", s);
 	} else if ((s = prefix(val, "(utf8)"))) {
@@ -227,7 +272,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
 		}
 
 		if (!data)
-			return EFI_EXIT(EFI_INVALID_PARAMETER);
+			return EFI_INVALID_PARAMETER;
 
 		memcpy(data, s, len);
 		((char *)data)[len] = '\0';
@@ -235,13 +280,67 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
 		EFI_PRINT("got value: \"%s\"\n", (char *)data);
 	} else {
 		EFI_PRINT("invalid value: '%s'\n", val);
-		return EFI_EXIT(EFI_DEVICE_ERROR);
+		return EFI_DEVICE_ERROR;
 	}
 
 out:
 	if (attributes)
 		*attributes = attr & EFI_VARIABLE_MASK;
 
+	return ret;
+}
+
+static
+efi_status_t EFIAPI efi_get_volatile_variable(u16 *variable_name,
+					      const efi_guid_t *vendor,
+					      u32 *attributes,
+					      efi_uintn_t *data_size,
+					      void *data)
+{
+	return efi_get_variable_common(variable_name, vendor, attributes,
+				       data_size, data, false);
+}
+
+efi_status_t EFIAPI efi_get_nonvolatile_variable(u16 *variable_name,
+						 const efi_guid_t *vendor,
+						 u32 *attributes,
+						 efi_uintn_t *data_size,
+						 void *data)
+{
+	return efi_get_variable_common(variable_name, vendor, attributes,
+				       data_size, data, true);
+}
+
+/**
+ * efi_efi_get_variable() - retrieve value of a UEFI variable
+ *
+ * This function implements the GetVariable runtime service.
+ *
+ * See the Unified Extensible Firmware Interface (UEFI) specification for
+ * details.
+ *
+ * @variable_name:	name of the variable
+ * @vendor:		vendor GUID
+ * @attributes:		attributes of the variable
+ * @data_size:		size of the buffer to which the variable value is copied
+ * @data:		buffer to which the variable value is copied
+ * Return:		status code
+ */
+efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
+				     const efi_guid_t *vendor, u32 *attributes,
+				     efi_uintn_t *data_size, void *data)
+{
+	efi_status_t ret;
+
+	EFI_ENTRY("\"%ls\" %pUl %p %p %p", variable_name, vendor, attributes,
+		  data_size, data);
+
+	ret = efi_get_volatile_variable(variable_name, vendor, attributes,
+					data_size, data);
+	if (ret == EFI_NOT_FOUND)
+		ret = efi_get_nonvolatile_variable(variable_name, vendor,
+						   attributes, data_size, data);
+
 	return EFI_EXIT(ret);
 }
 
@@ -331,7 +430,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};
@@ -387,10 +486,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);
 
@@ -403,77 +531,71 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
 	return EFI_EXIT(ret);
 }
 
-/**
- * efi_efi_set_variable() - set value of a UEFI variable
- *
- * This function implements the SetVariable runtime service.
- *
- * See the Unified Extensible Firmware Interface (UEFI) specification for
- * details.
- *
- * @variable_name:	name of the variable
- * @vendor:		vendor GUID
- * @attributes:		attributes of the variable
- * @data_size:		size of the buffer with the variable value
- * @data:		buffer with the variable value
- * Return:		status code
- */
-efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
-				     const efi_guid_t *vendor, u32 attributes,
-				     efi_uintn_t data_size, const void *data)
+static
+efi_status_t EFIAPI efi_set_variable_common(u16 *variable_name,
+					    const efi_guid_t *vendor,
+					    u32 attributes,
+					    efi_uintn_t data_size,
+					    const void *data,
+					    bool is_non_volatile)
 {
 	char *native_name = NULL, *val = NULL, *s;
-	efi_status_t ret = EFI_SUCCESS;
+	efi_uintn_t size;
 	u32 attr;
-
-	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
-		  data_size, data);
+	efi_status_t ret = EFI_SUCCESS;
 
 	/* TODO: implement APPEND_WRITE */
 	if (!variable_name || !vendor ||
 	    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
 		ret = EFI_INVALID_PARAMETER;
-		goto out;
+		goto err;
 	}
 
 	ret = efi_to_native(&native_name, variable_name, vendor);
 	if (ret)
-		goto out;
+		goto err;
 
 #define ACCESS_ATTR (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)
 
-	if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
-		/* delete the variable: */
-		env_set(native_name, NULL);
-		ret = EFI_SUCCESS;
-		goto out;
+	/* check if a variable exists */
+	size = 0;
+	ret = EFI_CALL(efi_get_variable(variable_name, vendor, &attr,
+					&size, NULL));
+	if (ret == EFI_BUFFER_TOO_SMALL) {
+		if ((is_non_volatile && !(attr & EFI_VARIABLE_NON_VOLATILE)) ||
+		    (!is_non_volatile && (attr & EFI_VARIABLE_NON_VOLATILE))) {
+			ret = EFI_INVALID_PARAMETER;
+			goto err;
+		}
 	}
 
-	val = env_get(native_name);
-	if (val) {
-		parse_attr(val, &attr);
-
-		/* We should not free val */
-		val = NULL;
-		if (attr & READ_ONLY) {
-			ret = EFI_WRITE_PROTECTED;
+	/* delete a variable */
+	if (data_size == 0 || !(attributes & ACCESS_ATTR)) {
+		if (size) {
+			if (attr & READ_ONLY) {
+				ret = EFI_WRITE_PROTECTED;
+				goto err;
+			}
 			goto out;
 		}
+		ret = EFI_SUCCESS;
+		goto err; /* not error, but nothing to do */
+	}
 
+	/* create/modify a variable */
+	if (size && attr != attributes) {
 		/*
 		 * attributes won't be changed
 		 * TODO: take care of APPEND_WRITE once supported
 		 */
-		if (attr != attributes) {
-			ret = EFI_INVALID_PARAMETER;
-			goto out;
-		}
+		ret = EFI_INVALID_PARAMETER;
+		goto err;
 	}
 
 	val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1);
 	if (!val) {
 		ret = EFI_OUT_OF_RESOURCES;
-		goto out;
+		goto err;
 	}
 
 	s = val;
@@ -487,7 +609,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
 		       EFI_VARIABLE_RUNTIME_ACCESS);
 	s += sprintf(s, "{");
 	while (attributes) {
-		u32 attr = 1 << (ffs(attributes) - 1);
+		attr = 1 << (ffs(attributes) - 1);
 
 		if (attr == EFI_VARIABLE_NON_VOLATILE)
 			s += sprintf(s, "nv");
@@ -509,12 +631,78 @@ 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))
+out:
+	ret = EFI_SUCCESS;
+	if (env_efi_set(native_name, val, is_non_volatile))
 		ret = EFI_DEVICE_ERROR;
 
-out:
+err:
 	free(native_name);
 	free(val);
 
+	return ret;
+}
+
+static
+efi_status_t EFIAPI efi_set_volatile_variable(u16 *variable_name,
+					      const efi_guid_t *vendor,
+					      u32 attributes,
+					      efi_uintn_t data_size,
+					      const void *data)
+{
+	return efi_set_variable_common(variable_name, vendor, attributes,
+				       data_size, data, false);
+}
+
+efi_status_t EFIAPI efi_set_nonvolatile_variable(u16 *variable_name,
+						 const efi_guid_t *vendor,
+						 u32 attributes,
+						 efi_uintn_t data_size,
+						 const void *data)
+{
+	efi_status_t ret;
+
+	ret = efi_set_variable_common(variable_name, vendor, attributes,
+				      data_size, data, true);
+	if (ret == EFI_SUCCESS)
+		/* FIXME: what if save failed? */
+		if (env_efi_save())
+			ret = EFI_DEVICE_ERROR;
+
+	return ret;
+}
+
+/**
+ * efi_efi_set_variable() - set value of a UEFI variable
+ *
+ * This function implements the SetVariable runtime service.
+ *
+ * See the Unified Extensible Firmware Interface (UEFI) specification for
+ * details.
+ *
+ * @variable_name:	name of the variable
+ * @vendor:		vendor GUID
+ * @attributes:		attributes of the variable
+ * @data_size:		size of the buffer with the variable value
+ * @data:		buffer with the variable value
+ * Return:		status code
+ */
+efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
+				     const efi_guid_t *vendor, u32 attributes,
+				     efi_uintn_t data_size, const void *data)
+{
+	efi_status_t ret;
+
+	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
+		  data_size, data);
+
+	if (attributes & EFI_VARIABLE_NON_VOLATILE)
+		ret = efi_set_nonvolatile_variable(variable_name, vendor,
+						   attributes,
+						   data_size, data);
+	else
+		ret = efi_set_volatile_variable(variable_name, vendor,
+						attributes, data_size, data);
+
 	return EFI_EXIT(ret);
 }
-- 
2.21.0

[U-Boot] [PATCH v3 4/7] 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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index 8691d686d29d..45d6aca051f3 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -8,6 +8,7 @@
 #include <common.h>
 #include <bootm.h>
 #include <efi_loader.h>
+#include <environment.h>
 
 #define OBJ_LIST_NOT_INITIALIZED 1
 
@@ -102,6 +103,11 @@ efi_status_t efi_init_obj_list(void)
 	/* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
 	switch_to_non_secure_mode();
 
+#ifdef CONFIG_EFI_VARIABLE_USE_ENV
+	/* Load non-volatile variables */
+	env_efi_load();
+#endif
+
 	/* Define supported languages */
 	ret = efi_init_platform_lang();
 	if (ret != EFI_SUCCESS)
-- 
2.21.0

[U-Boot] [PATCH v3 5/7] efi_loader: bootmgr: make BootNext 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 43791422c819..b2102c5b5af2 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -210,7 +210,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.21.0

[U-Boot] [PATCH v3 6/7] 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 c4ac9dd634e2..e65722625455 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -558,6 +558,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));
@@ -909,6 +910,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));
@@ -964,6 +966,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.21.0

[U-Boot] [PATCH v3 7/7] 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 ff8eaa1aad2d..60a8ac84c811 100644
--- a/cmd/nvedit_efi.c
+++ b/cmd/nvedit_efi.c
@@ -349,6 +349,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)
@@ -362,6 +363,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 */
@@ -391,9 +402,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));
 	if (ret == EFI_SUCCESS) {
 		ret = CMD_RET_SUCCESS;
-- 
2.21.0

[U-Boot] [PATCH v3 1/7] env: save UEFI non-volatile variables in dedicated storage

[Heinrich Schuchardt]

On 6/4/19 8:52 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
> 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                       |  39 ++++++++
>   env/env.c                         | 155 +++++++++++++++++++++++++++++-
>   env/fat.c                         | 105 ++++++++++++++++++++
>   include/asm-generic/global_data.h |   3 +
>   include/environment.h             |  31 ++++++
>   5 files changed, 332 insertions(+), 1 deletion(-)
>
> diff --git a/env/Kconfig b/env/Kconfig
> index 1e10c7a4c46b..a36b6ee48f10 100644
> --- a/env/Kconfig
> +++ b/env/Kconfig
> @@ -399,6 +399,10 @@ config ENV_IS_IN_UBI
>   	  the environment in.  This will enable redundant environments in UBI.
>   	  It is assumed that both volumes are in the same MTD partition.
>

Our store will be completely unrelated to U-Boot environment variables.
So why should we put anything into this Kconfig?

> +config ENV_EFI
> +	bool
> +	default n
> +
>   config ENV_FAT_INTERFACE
>   	string "Name of the block device for the environment"
>   	depends on ENV_IS_IN_FAT
> @@ -438,6 +442,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 ENV_EFI
> +	help
> +	  Define this to a string to specify the partition of the device. It can
> +	  be as following:

The following information is not specific to this variable. So we can
cut it short:

"String defining the device number and the partition number in the
format <device number>:<partition number>, e.g. 0:1."


Where do you specify on which subsystem (mmc, scsi, sata, nvme) the file
is stored?

I would prefer nopt to have a restriction to FAT. If the EXT4 driver is
enabled writing and reading to an ext4 valume should work as well. When
calling fs_set_blk_dev() you will not specify any file system.

> +
> +	    "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"

%s/UEFI non-volatile variables/non-volatile UEFI variables/

No need for the file system being FAT.

> +	depends on ENV_IS_IN_FAT
> +	depends on ENV_EFI
> +	default "uboot_efi.env"
> +	help
> +	  It's a string of the FAT file name. This file use to store the

It is obvious that afile name is a string. But why restrict to a file
name? This could also be a path to a non-root file:

"Path of the file used to store non-volatile UEFI variables."

But can't we simply have a single variable, where we put all parts of
the definition, e.g.

mmc 0:1 /EFI/nv-var.store

> +	  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 +502,13 @@ 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"
> +	depends on ENV_EFI
> +	default 0x20000

If we are writing to a file, the available space on the partition and in
RAM is the limit. I see no need for this variable in this case.

> +	help
> +	  Size of the UEFI variables storage area
> +
>   if ARCH_ROCKCHIP || ARCH_SUNXI || ARCH_ZYNQ || ARCH_ZYNQMP || ARCH_VERSAL || ARC || ARCH_STM32MP
>
>   config ENV_OFFSET
> diff --git a/env/env.c b/env/env.c
> index 4b417b90a291..202079f6d4c0 100644
> --- a/env/env.c
> +++ b/env/env.c

We are not creating a driver for U-Boot environment variables. So I
would keep away from the code in this directory. You can put the load
and save function into lib/efi_loader/ and use the normal file-system
functions there to read and write the file.

Essentially I expect we will end up with a class of drivers which we can
use as backends for nv variables, e.g.

* file storage
* OP-TEE module
* U-Boot variables
* none

Best regards

Heinrich

> @@ -24,6 +24,12 @@ void env_fix_drivers(void)
>   			entry->load += gd->reloc_off;
>   		if (entry->save)
>   			entry->save += gd->reloc_off;
> +#ifdef CONFIG_ENV_EFI
> +		if (entry->efi_load)
> +			entry->efi_load += gd->reloc_off;
> +		if (entry->efi_save)
> +			entry->efi_save += gd->reloc_off;
> +#endif
>   		if (entry->init)
>   			entry->init += gd->reloc_off;
>   	}
> @@ -125,7 +131,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 +287,149 @@ int env_init(void)
>
>   	return ret;
>   }
> +
> +#ifdef CONFIG_ENV_EFI
> +struct hsearch_data efi_var_htab;
> +struct hsearch_data efi_nv_var_htab;
> +
> +int env_efi_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 - ENV_HEADER_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 - ENV_HEADER_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;
> +}
> +
> +int env_efi_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 - ENV_HEADER_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 - ENV_HEADER_SIZE);
> +
> +	return 0;
> +}
> +
> +int env_efi_save(void)
> +{
> +#ifdef CONFIG_ENV_IS_NOWHERE
> +	return 0;
> +#else
> +	struct env_driver *drv = NULL;
> +	int ret;
> +
> +	if (!efi_nv_var_htab.table)
> +		return 0;
> +
> +	if (gd->env_efi_prio == -1) {
> +		pr_warn("No UEFI non-volatile variable storage\n");
> +		return -1;
> +	}
> +
> +	drv = _env_driver_lookup(env_get_location(ENVOP_EFI, gd->env_efi_prio));
> +	if (!drv) {
> +		pr_warn("No UEFI non-volatile variable storage\n");
> +		return -1;
> +	}
> +
> +	ret = drv->efi_save();
> +	if (ret)
> +		pr_err("Saving UEFI non-volatile variable failed\n");
> +
> +	return ret;
> +#endif
> +}
> +
> +/* This function should be called only once at init */
> +int env_efi_load(void)
> +{
> +#ifndef CONFIG_ENV_IS_NOWHERE
> +	struct env_driver *drv;
> +	int prio;
> +	enum env_location loc;
> +#endif
> +	int ret;
> +
> +	/* volatile variables */
> +	if (!efi_var_htab.table) {
> +		ret = himport_r(&efi_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
> +		if (!ret) {
> +			pr_err("Creating UEFI volatile variables failed\n");
> +			return -1;
> +		}
> +	}
> +
> +#ifndef CONFIG_ENV_IS_NOWHERE
> +	gd->env_efi_prio = -1;
> +
> +	/* non-volatile variables */
> +	if (efi_nv_var_htab.table)
> +		return 0;
> +
> +	for (drv = NULL, 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)) {
> +		pr_warn("No UEFI non-volatile variable storage\n");
> +		goto skip_load;
> +	}
> +
> +	gd->env_efi_prio = prio;
> +
> +	ret = drv->efi_load();
> +	if (ret) {
> +		pr_err("Loading UEFI non-volatile variables failed\n");
> +		return -1;
> +	}
> +skip_load:
> +#endif /* CONFIG_ENV_IS_NOWHERE */
> +
> +	if (!efi_nv_var_htab.table) {
> +		ret = himport_r(&efi_nv_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
> +		if (!ret) {
> +			pr_err("Creating UEFI non-volatile variables failed\n");
> +			return -1;
> +		}
> +
> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +#endif /* CONFIG_ENV_EFI */
> diff --git a/env/fat.c b/env/fat.c
> index 7f74c64dfe7e..7cd6dc51baea 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,106 @@ err_env_relocate:
>   }
>   #endif /* LOADENV */
>
> +#ifdef CONFIG_ENV_EFI
> +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 = env_efi_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 env_efi_import(buf, 1);
> +	else
> +		return 0;
> +
> +err_env_relocate:
> +
> +	return -EIO;
> +}
> +#endif /* CONFIG_ENV_EFI */
> +
>   U_BOOT_ENV_LOCATION(fat) = {
>   	.location	= ENVL_FAT,
>   	ENV_NAME("FAT")
> @@ -137,4 +238,8 @@ U_BOOT_ENV_LOCATION(fat) = {
>   #ifdef CMD_SAVEENV
>   	.save		= env_save_ptr(env_fat_save),
>   #endif
> +#ifdef CONFIG_ENV_EFI
> +	.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 02a3ed683821..5ed390cc31c7 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -52,6 +52,9 @@ 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 */
> +#ifdef CONFIG_ENV_EFI
> +	int env_efi_prio;		/* Priority of the UEFI variables */
> +#endif
>
>   	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..32a03014690f 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,26 @@ struct env_driver {
>   	 */
>   	int (*save)(void);
>
> +#ifdef CONFIG_ENV_EFI
> +	/**
> +	 * 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);
> +#endif
> +
>   	/**
>   	 * init() - Set up the initial pre-relocation environment
>   	 *
> @@ -312,6 +333,16 @@ 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_ENV_EFI
> +extern struct hsearch_data efi_var_htab;
> +extern struct hsearch_data efi_nv_var_htab;
> +
> +int env_efi_import(const char *buf, int check);
> +int env_efi_export(env_t *env_out);
> +int env_efi_load(void);
> +int env_efi_save(void);
> +#endif
> +
>   #endif /* DO_DEPS_ONLY */
>
>   #endif /* _ENVIRONMENT_H_ */
>

[U-Boot] [PATCH v3 2/7] efi_loader: variable: support non-volatile attribute

[Heinrich Schuchardt]

On 6/4/19 8:52 AM, AKASHI Takahiro wrote:
> 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>

Reviewed-by: Heinrich Schuchardt

I will cherry-pick this patch. It is already showing relevant output:

Warning: e1000#0 using MAC address from ROM
OsIndicationsSupported: BS|RT, DataSize = 0x8
     00000000: 00 00 00 00 00 00 00 00                          ........
PlatformLang: NV|BS|RT, DataSize = 0x6
     00000000: 65 6e 2d 55 53 00                                en-US.
PlatformLangCodes: BS|RT, DataSize = 0x6
     00000000: 65 6e 2d 55 53 00                                en-US.

> ---
>   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 50bc10537f40..e56053194dae 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"))) {
> @@ -468,7 +470,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;
> @@ -480,12 +482,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");
>

[U-Boot] [PATCH v3 3/7] efi_loader: variable: split UEFI variables from U-Boot environment

[Heinrich Schuchardt]

On 6/4/19 8:52 AM, AKASHI Takahiro wrote:
> 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/Kconfig        |  10 +
>   lib/efi_loader/efi_variable.c | 342 ++++++++++++++++++++++++++--------
>   2 files changed, 275 insertions(+), 77 deletions(-)
>
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index cd5436c576b1..8bf4b1754d06 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -18,6 +18,16 @@ config EFI_LOADER
>
>   if EFI_LOADER
>
> +choice
> +	prompt "Select variables storage"
> +	default EFI_VARIABLE_USE_ENV
> +
> +config EFI_VARIABLE_USE_ENV
> +	bool "Same device as U-Boot environment"
> +	select ENV_EFI
> +
> +endchoice
> +
>   config EFI_GET_TIME
>   	bool "GetTime() runtime service"
>   	depends on DM_RTC
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index e56053194dae..d9887be938c2 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -48,6 +48,66 @@
>    * 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.
> + *
> + *	struct hsearch_data efi_var_htab
> + *	struct hsearch_data efi_nv_var_htab
> + */
> +
> +static char *env_efi_get(const char *name, bool is_non_volatile)
> +{
> +	struct hsearch_data *htab;
> +	ENTRY e, *ep;
> +
> +	/* WATCHDOG_RESET(); */
> +
> +	if (is_non_volatile)
> +		htab = &efi_nv_var_htab;
> +	else
> +		htab = &efi_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,
> +		       bool is_non_volatile)
> +{
> +	struct hsearch_data *htab;
> +	ENTRY e, *ep;
> +	int ret;
> +
> +	if (is_non_volatile)
> +		htab = &efi_nv_var_htab;
> +	else
> +		htab = &efi_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;
> +}
> +
>   #define PREFIX_LEN (strlen("efi_xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx_"))
>
>   /**
> @@ -147,24 +207,12 @@ static const char *parse_attr(const char *str, u32 *attrp)
>   	return str;
>   }
>
> -/**
> - * efi_efi_get_variable() - retrieve value of a UEFI variable
> - *
> - * This function implements the GetVariable runtime service.
> - *
> - * See the Unified Extensible Firmware Interface (UEFI) specification for
> - * details.
> - *
> - * @variable_name:	name of the variable
> - * @vendor:		vendor GUID
> - * @attributes:		attributes of the variable
> - * @data_size:		size of the buffer to which the variable value is copied
> - * @data:		buffer to which the variable value is copied
> - * Return:		status code
> - */
> -efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> -				     const efi_guid_t *vendor, u32 *attributes,
> -				     efi_uintn_t *data_size, void *data)
> +static
> +efi_status_t EFIAPI efi_get_variable_common(u16 *variable_name,
> +					    const efi_guid_t *vendor,
> +					    u32 *attributes,
> +					    efi_uintn_t *data_size, void *data,
> +					    bool is_non_volatile)
>   {
>   	char *native_name;
>   	efi_status_t ret;
> @@ -172,22 +220,19 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
>   	const char *val, *s;
>   	u32 attr;
>
> -	EFI_ENTRY("\"%ls\" %pUl %p %p %p", variable_name, vendor, attributes,
> -		  data_size, data);
> -
>   	if (!variable_name || !vendor || !data_size)
>   		return EFI_EXIT(EFI_INVALID_PARAMETER);
>
>   	ret = efi_to_native(&native_name, variable_name, vendor);
>   	if (ret)
> -		return EFI_EXIT(ret);
> +		return ret;
>
>   	EFI_PRINT("get '%s'\n", native_name);
>
> -	val = env_get(native_name);
> +	val = env_efi_get(native_name, is_non_volatile);
>   	free(native_name);
>   	if (!val)
> -		return EFI_EXIT(EFI_NOT_FOUND);
> +		return EFI_NOT_FOUND;
>
>   	val = parse_attr(val, &attr);
>
> @@ -198,7 +243,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
>
>   		/* number of hexadecimal digits must be even */
>   		if (len & 1)
> -			return EFI_EXIT(EFI_DEVICE_ERROR);
> +			return EFI_DEVICE_ERROR;
>
>   		/* two characters per byte: */
>   		len /= 2;
> @@ -210,10 +255,10 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
>   		}
>
>   		if (!data)
> -			return EFI_EXIT(EFI_INVALID_PARAMETER);
> +			return EFI_INVALID_PARAMETER;
>
>   		if (hex2bin(data, s, len))
> -			return EFI_EXIT(EFI_DEVICE_ERROR);
> +			return EFI_DEVICE_ERROR;
>
>   		EFI_PRINT("got value: \"%s\"\n", s);
>   	} else if ((s = prefix(val, "(utf8)"))) {
> @@ -227,7 +272,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
>   		}
>
>   		if (!data)
> -			return EFI_EXIT(EFI_INVALID_PARAMETER);
> +			return EFI_INVALID_PARAMETER;
>
>   		memcpy(data, s, len);
>   		((char *)data)[len] = '\0';
> @@ -235,13 +280,67 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
>   		EFI_PRINT("got value: \"%s\"\n", (char *)data);
>   	} else {
>   		EFI_PRINT("invalid value: '%s'\n", val);
> -		return EFI_EXIT(EFI_DEVICE_ERROR);
> +		return EFI_DEVICE_ERROR;
>   	}
>
>   out:
>   	if (attributes)
>   		*attributes = attr & EFI_VARIABLE_MASK;
>
> +	return ret;
> +}
> +
> +static
> +efi_status_t EFIAPI efi_get_volatile_variable(u16 *variable_name,
> +					      const efi_guid_t *vendor,
> +					      u32 *attributes,
> +					      efi_uintn_t *data_size,
> +					      void *data)
> +{
> +	return efi_get_variable_common(variable_name, vendor, attributes,
> +				       data_size, data, false);
> +}
> +
> +efi_status_t EFIAPI efi_get_nonvolatile_variable(u16 *variable_name,
> +						 const efi_guid_t *vendor,
> +						 u32 *attributes,
> +						 efi_uintn_t *data_size,
> +						 void *data)
> +{
> +	return efi_get_variable_common(variable_name, vendor, attributes,
> +				       data_size, data, true);
> +}
> +
> +/**
> + * efi_efi_get_variable() - retrieve value of a UEFI variable
> + *
> + * This function implements the GetVariable runtime service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * @variable_name:	name of the variable
> + * @vendor:		vendor GUID
> + * @attributes:		attributes of the variable
> + * @data_size:		size of the buffer to which the variable value is copied
> + * @data:		buffer to which the variable value is copied
> + * Return:		status code
> + */
> +efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> +				     const efi_guid_t *vendor, u32 *attributes,
> +				     efi_uintn_t *data_size, void *data)
> +{
> +	efi_status_t ret;
> +
> +	EFI_ENTRY("\"%ls\" %pUl %p %p %p", variable_name, vendor, attributes,
> +		  data_size, data);
> +
> +	ret = efi_get_volatile_variable(variable_name, vendor, attributes,
> +					data_size, data);
> +	if (ret == EFI_NOT_FOUND)
> +		ret = efi_get_nonvolatile_variable(variable_name, vendor,
> +						   attributes, data_size, data);
> +
>   	return EFI_EXIT(ret);
>   }
>
> @@ -331,7 +430,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};
> @@ -387,10 +486,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);
>
> @@ -403,77 +531,71 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
>   	return EFI_EXIT(ret);
>   }
>
> -/**
> - * efi_efi_set_variable() - set value of a UEFI variable
> - *
> - * This function implements the SetVariable runtime service.
> - *
> - * See the Unified Extensible Firmware Interface (UEFI) specification for
> - * details.
> - *
> - * @variable_name:	name of the variable
> - * @vendor:		vendor GUID
> - * @attributes:		attributes of the variable
> - * @data_size:		size of the buffer with the variable value
> - * @data:		buffer with the variable value
> - * Return:		status code
> - */
> -efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> -				     const efi_guid_t *vendor, u32 attributes,
> -				     efi_uintn_t data_size, const void *data)
> +static
> +efi_status_t EFIAPI efi_set_variable_common(u16 *variable_name,
> +					    const efi_guid_t *vendor,
> +					    u32 attributes,
> +					    efi_uintn_t data_size,
> +					    const void *data,
> +					    bool is_non_volatile)
>   {
>   	char *native_name = NULL, *val = NULL, *s;
> -	efi_status_t ret = EFI_SUCCESS;
> +	efi_uintn_t size;
>   	u32 attr;
> -
> -	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
> -		  data_size, data);
> +	efi_status_t ret = EFI_SUCCESS;
>
>   	/* TODO: implement APPEND_WRITE */
>   	if (!variable_name || !vendor ||
>   	    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
>   		ret = EFI_INVALID_PARAMETER;
> -		goto out;
> +		goto err;
>   	}
>
>   	ret = efi_to_native(&native_name, variable_name, vendor);
>   	if (ret)
> -		goto out;
> +		goto err;
>
>   #define ACCESS_ATTR (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)
>
> -	if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
> -		/* delete the variable: */
> -		env_set(native_name, NULL);
> -		ret = EFI_SUCCESS;
> -		goto out;
> +	/* check if a variable exists */
> +	size = 0;
> +	ret = EFI_CALL(efi_get_variable(variable_name, vendor, &attr,
> +					&size, NULL));
> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		if ((is_non_volatile && !(attr & EFI_VARIABLE_NON_VOLATILE)) ||
> +		    (!is_non_volatile && (attr & EFI_VARIABLE_NON_VOLATILE))) {
> +			ret = EFI_INVALID_PARAMETER;
> +			goto err;
> +		}
>   	}
>
> -	val = env_get(native_name);
> -	if (val) {
> -		parse_attr(val, &attr);
> -
> -		/* We should not free val */
> -		val = NULL;
> -		if (attr & READ_ONLY) {
> -			ret = EFI_WRITE_PROTECTED;
> +	/* delete a variable */
> +	if (data_size == 0 || !(attributes & ACCESS_ATTR)) {
> +		if (size) {
> +			if (attr & READ_ONLY) {
> +				ret = EFI_WRITE_PROTECTED;
> +				goto err;
> +			}
>   			goto out;
>   		}
> +		ret = EFI_SUCCESS;
> +		goto err; /* not error, but nothing to do */
> +	}
>
> +	/* create/modify a variable */
> +	if (size && attr != attributes) {
>   		/*
>   		 * attributes won't be changed
>   		 * TODO: take care of APPEND_WRITE once supported
>   		 */
> -		if (attr != attributes) {
> -			ret = EFI_INVALID_PARAMETER;
> -			goto out;
> -		}
> +		ret = EFI_INVALID_PARAMETER;
> +		goto err;
>   	}
>
>   	val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1);
>   	if (!val) {
>   		ret = EFI_OUT_OF_RESOURCES;
> -		goto out;
> +		goto err;
>   	}
>
>   	s = val;
> @@ -487,7 +609,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
>   		       EFI_VARIABLE_RUNTIME_ACCESS);
>   	s += sprintf(s, "{");
>   	while (attributes) {
> -		u32 attr = 1 << (ffs(attributes) - 1);
> +		attr = 1 << (ffs(attributes) - 1);
>
>   		if (attr == EFI_VARIABLE_NON_VOLATILE)
>   			s += sprintf(s, "nv");
> @@ -509,12 +631,78 @@ 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))
> +out:
> +	ret = EFI_SUCCESS;
> +	if (env_efi_set(native_name, val, is_non_volatile))
>   		ret = EFI_DEVICE_ERROR;
>
> -out:
> +err:
>   	free(native_name);
>   	free(val);
>
> +	return ret;
> +}
> +
> +static
> +efi_status_t EFIAPI efi_set_volatile_variable(u16 *variable_name,

Once you have implemented this we can make the variable service
available at runtime. So I suggest to define everything here as __runtime.

Why do you any of these three functions (efi_set_volatile_variable(),
efi_set_nonvolatile_variable(), efi_set_nonvolatile_variable()). Just
use an if based on attributes in efi_set_variable() to call
env_efi_save, when a non-volatile function is changed.

What is the advantage of having two separate RAM stores? Can't the save
function sort out what to save and what not to save according to the NV
flag?

> +					      const efi_guid_t *vendor,
> +					      u32 attributes,
> +					      efi_uintn_t data_size,
> +					      const void *data)
> +{
> +	return efi_set_variable_common(variable_name, vendor, attributes,
> +				       data_size, data, false);
> +}
> +
> +efi_status_t EFIAPI efi_set_nonvolatile_variable(u16 *variable_name,
> +						 const efi_guid_t *vendor,
> +						 u32 attributes,
> +						 efi_uintn_t data_size,
> +						 const void *data)
> +{
> +	efi_status_t ret;
> +
> +	ret = efi_set_variable_common(variable_name, vendor, attributes,
> +				      data_size, data, true);
> +	if (ret == EFI_SUCCESS)
> +		/* FIXME: what if save failed? */
> +		if (env_efi_save())

Somewhere we need the ability to switch between different backends. Will
that be in env_efi_save()?

> +			ret = EFI_DEVICE_ERROR;
> +
> +	return ret;
> +}
> +
> +/**
> + * efi_efi_set_variable() - set value of a UEFI variable
> + *
> + * This function implements the SetVariable runtime service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * @variable_name:	name of the variable
> + * @vendor:		vendor GUID
> + * @attributes:		attributes of the variable
> + * @data_size:		size of the buffer with the variable value
> + * @data:		buffer with the variable value
> + * Return:		status code
> + */
> +efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> +				     const efi_guid_t *vendor, u32 attributes,
> +				     efi_uintn_t data_size, const void *data)
> +{
> +	efi_status_t ret;
> +
> +	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
> +		  data_size, data);
> +
> +	if (attributes & EFI_VARIABLE_NON_VOLATILE)
> +		ret = efi_set_nonvolatile_variable(variable_name, vendor,
> +						   attributes,
> +						   data_size, data);
> +	else
> +		ret = efi_set_volatile_variable(variable_name, vendor,
> +						attributes, data_size, data);
> +
>   	return EFI_EXIT(ret);
>   }
>

[U-Boot] [PATCH v3 4/7] efi_loader: load saved non-volatile variables at init

[Heinrich Schuchardt]

On 6/4/19 8:52 AM, AKASHI Takahiro wrote:
> 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 | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index 8691d686d29d..45d6aca051f3 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -8,6 +8,7 @@
>   #include <common.h>
>   #include <bootm.h>
>   #include <efi_loader.h>
> +#include <environment.h>
>
>   #define OBJ_LIST_NOT_INITIALIZED 1
>
> @@ -102,6 +103,11 @@ efi_status_t efi_init_obj_list(void)
>   	/* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
>   	switch_to_non_secure_mode();
>
> +#ifdef CONFIG_EFI_VARIABLE_USE_ENV

No clue what ENV refers to here as we are not talking about U-Boot
environment variables anymore. How about CONFIG_EFI_PERSISTENT_VARIABLES.


> +	/* Load non-volatile variables */
> +	env_efi_load();

Can't we make env_efi_load() a __weak function which does nothing. If we
have a backend, that backend replaces the weak function. That way we
restrict the config variables to the Makefile.

Regards

Heinrich

> +#endif
> +
>   	/* Define supported languages */
>   	ret = efi_init_platform_lang();
>   	if (ret != EFI_SUCCESS)
>

[U-Boot] [PATCH v3 6/7] cmd: efidebug: make some boot variables non-volatile

[Heinrich Schuchardt]

On 6/4/19 8:52 AM, AKASHI Takahiro wrote:
> Boot####, BootOrder and BootNext should be non-volatile.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Reviewed-by: Heinrich Schuchardt

> ---
>   cmd/efidebug.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index c4ac9dd634e2..e65722625455 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -558,6 +558,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));
> @@ -909,6 +910,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));
> @@ -964,6 +966,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));
>

[U-Boot] [PATCH v3 5/7] efi_loader: bootmgr: make BootNext non-volatile

[Heinrich Schuchardt]

On 6/4/19 8:52 AM, AKASHI Takahiro wrote:
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

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

> ---
>   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 43791422c819..b2102c5b5af2 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -210,7 +210,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) {
>

[U-Boot] [PATCH v3 7/7] cmd: env: add -nv option for UEFI non-volatile variable

[Heinrich Schuchardt]

On 6/4/19 8:52 AM, AKASHI Takahiro wrote:
> 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>

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

[U-Boot] [PATCH v3 1/7] env: save UEFI non-volatile variables in dedicated storage

[AKASHI Takahiro]

On Tue, Jun 04, 2019 at 11:09:56PM +0200, Heinrich Schuchardt wrote:
> On 6/4/19 8:52 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
> >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                       |  39 ++++++++
> >  env/env.c                         | 155 +++++++++++++++++++++++++++++-
> >  env/fat.c                         | 105 ++++++++++++++++++++
> >  include/asm-generic/global_data.h |   3 +
> >  include/environment.h             |  31 ++++++
> >  5 files changed, 332 insertions(+), 1 deletion(-)
> >
> >diff --git a/env/Kconfig b/env/Kconfig
> >index 1e10c7a4c46b..a36b6ee48f10 100644
> >--- a/env/Kconfig
> >+++ b/env/Kconfig
> >@@ -399,6 +399,10 @@ config ENV_IS_IN_UBI
> >  	  the environment in.  This will enable redundant environments in UBI.
> >  	  It is assumed that both volumes are in the same MTD partition.
> >
> 
> Our store will be completely unrelated to U-Boot environment variables.
> So why should we put anything into this Kconfig?

This is a discussion.
Some of my code still mimics the logic of U-Boot environment,
for instance, backing driver lookup.
I think that U-Boot maintainers may want U-Boot and UEFI code
to stay as close as possible.

> >+config ENV_EFI
> >+	bool
> >+	default n
> >+
> >  config ENV_FAT_INTERFACE
> >  	string "Name of the block device for the environment"
> >  	depends on ENV_IS_IN_FAT
> >@@ -438,6 +442,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 ENV_EFI
> >+	help
> >+	  Define this to a string to specify the partition of the device. It can
> >+	  be as following:
> 
> The following information is not specific to this variable. So we can
> cut it short:
> 
> "String defining the device number and the partition number in the
> format <device number>:<partition number>, e.g. 0:1."

The text is just a copy of the one as for CONFIG_ENV_FAT_DEVICE_AND_PART.
This is another reason that I put the code under "env."

After your comment, the text can be modified as it refers to
CONFIG_ENV_FAT_DEVICE_AND_PART for details.

> 
> Where do you specify on which subsystem (mmc, scsi, sata, nvme) the file
> is stored?

Haven't you read the cover letter? Please read it first.
I always try to put bunch of information regarding my patch there.

> I would prefer nopt to have a restriction to FAT. If the EXT4 driver is
> enabled writing and reading to an ext4 valume should work as well. When
> calling fs_set_blk_dev() you will not specify any file system.

I remember that I have answered this comment before:
This is just an example for backing storage.
It is a matter of time to support other devices as U-Boot environment does.
This is another reason that I put the code under "env."

> >+
> >+	    "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"
> 
> %s/UEFI non-volatile variables/non-volatile UEFI variables/
> 
> No need for the file system being FAT.

ditto.

> >+	depends on ENV_IS_IN_FAT
> >+	depends on ENV_EFI
> >+	default "uboot_efi.env"
> >+	help
> >+	  It's a string of the FAT file name. This file use to store the
> 
> It is obvious that afile name is a string. But why restrict to a file
> name? This could also be a path to a non-root file:

Again, this is a copy from CONFIG_ENV_FAT_FILE.

> "Path of the file used to store non-volatile UEFI variables."
> 
> But can't we simply have a single variable, where we put all parts of
> the definition, e.g.
> 
> mmc 0:1 /EFI/nv-var.store

Again, my code follows U-Boot's configuration style.

> >+	  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 +502,13 @@ 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"
> >+	depends on ENV_EFI
> >+	default 0x20000
> 
> If we are writing to a file, the available space on the partition and in
> RAM is the limit. I see no need for this variable in this case.

The configuration is not properly used in my code yet.
Anyway, it is not the size of written file, but the size of buffer
in my intent.

> >+	help
> >+	  Size of the UEFI variables storage area
> >+
> >  if ARCH_ROCKCHIP || ARCH_SUNXI || ARCH_ZYNQ || ARCH_ZYNQMP || ARCH_VERSAL || ARC || ARCH_STM32MP
> >
> >  config ENV_OFFSET
> >diff --git a/env/env.c b/env/env.c
> >index 4b417b90a291..202079f6d4c0 100644
> >--- a/env/env.c
> >+++ b/env/env.c
> 
> We are not creating a driver for U-Boot environment variables. So I
> would keep away from the code in this directory. You can put the load
> and save function into lib/efi_loader/ and use the normal file-system
> functions there to read and write the file.

ditto.

> Essentially I expect we will end up with a class of drivers which we can
> use as backends for nv variables, e.g.

I heavily disagree.
We have no needs to implement UEFI features only with UEFI interfaces.

-Takahiro Akashi

> * file storage
> * OP-TEE module
> * U-Boot variables
> * none
> 
> Best regards
> 
> Heinrich
> 
> >@@ -24,6 +24,12 @@ void env_fix_drivers(void)
> >  			entry->load += gd->reloc_off;
> >  		if (entry->save)
> >  			entry->save += gd->reloc_off;
> >+#ifdef CONFIG_ENV_EFI
> >+		if (entry->efi_load)
> >+			entry->efi_load += gd->reloc_off;
> >+		if (entry->efi_save)
> >+			entry->efi_save += gd->reloc_off;
> >+#endif
> >  		if (entry->init)
> >  			entry->init += gd->reloc_off;
> >  	}
> >@@ -125,7 +131,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 +287,149 @@ int env_init(void)
> >
> >  	return ret;
> >  }
> >+
> >+#ifdef CONFIG_ENV_EFI
> >+struct hsearch_data efi_var_htab;
> >+struct hsearch_data efi_nv_var_htab;
> >+
> >+int env_efi_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 - ENV_HEADER_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 - ENV_HEADER_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;
> >+}
> >+
> >+int env_efi_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 - ENV_HEADER_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 - ENV_HEADER_SIZE);
> >+
> >+	return 0;
> >+}
> >+
> >+int env_efi_save(void)
> >+{
> >+#ifdef CONFIG_ENV_IS_NOWHERE
> >+	return 0;
> >+#else
> >+	struct env_driver *drv = NULL;
> >+	int ret;
> >+
> >+	if (!efi_nv_var_htab.table)
> >+		return 0;
> >+
> >+	if (gd->env_efi_prio == -1) {
> >+		pr_warn("No UEFI non-volatile variable storage\n");
> >+		return -1;
> >+	}
> >+
> >+	drv = _env_driver_lookup(env_get_location(ENVOP_EFI, gd->env_efi_prio));
> >+	if (!drv) {
> >+		pr_warn("No UEFI non-volatile variable storage\n");
> >+		return -1;
> >+	}
> >+
> >+	ret = drv->efi_save();
> >+	if (ret)
> >+		pr_err("Saving UEFI non-volatile variable failed\n");
> >+
> >+	return ret;
> >+#endif
> >+}
> >+
> >+/* This function should be called only once at init */
> >+int env_efi_load(void)
> >+{
> >+#ifndef CONFIG_ENV_IS_NOWHERE
> >+	struct env_driver *drv;
> >+	int prio;
> >+	enum env_location loc;
> >+#endif
> >+	int ret;
> >+
> >+	/* volatile variables */
> >+	if (!efi_var_htab.table) {
> >+		ret = himport_r(&efi_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
> >+		if (!ret) {
> >+			pr_err("Creating UEFI volatile variables failed\n");
> >+			return -1;
> >+		}
> >+	}
> >+
> >+#ifndef CONFIG_ENV_IS_NOWHERE
> >+	gd->env_efi_prio = -1;
> >+
> >+	/* non-volatile variables */
> >+	if (efi_nv_var_htab.table)
> >+		return 0;
> >+
> >+	for (drv = NULL, 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)) {
> >+		pr_warn("No UEFI non-volatile variable storage\n");
> >+		goto skip_load;
> >+	}
> >+
> >+	gd->env_efi_prio = prio;
> >+
> >+	ret = drv->efi_load();
> >+	if (ret) {
> >+		pr_err("Loading UEFI non-volatile variables failed\n");
> >+		return -1;
> >+	}
> >+skip_load:
> >+#endif /* CONFIG_ENV_IS_NOWHERE */
> >+
> >+	if (!efi_nv_var_htab.table) {
> >+		ret = himport_r(&efi_nv_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
> >+		if (!ret) {
> >+			pr_err("Creating UEFI non-volatile variables failed\n");
> >+			return -1;
> >+		}
> >+
> >+		return 0;
> >+	}
> >+
> >+	return 0;
> >+}
> >+#endif /* CONFIG_ENV_EFI */
> >diff --git a/env/fat.c b/env/fat.c
> >index 7f74c64dfe7e..7cd6dc51baea 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,106 @@ err_env_relocate:
> >  }
> >  #endif /* LOADENV */
> >
> >+#ifdef CONFIG_ENV_EFI
> >+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 = env_efi_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 env_efi_import(buf, 1);
> >+	else
> >+		return 0;
> >+
> >+err_env_relocate:
> >+
> >+	return -EIO;
> >+}
> >+#endif /* CONFIG_ENV_EFI */
> >+
> >  U_BOOT_ENV_LOCATION(fat) = {
> >  	.location	= ENVL_FAT,
> >  	ENV_NAME("FAT")
> >@@ -137,4 +238,8 @@ U_BOOT_ENV_LOCATION(fat) = {
> >  #ifdef CMD_SAVEENV
> >  	.save		= env_save_ptr(env_fat_save),
> >  #endif
> >+#ifdef CONFIG_ENV_EFI
> >+	.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 02a3ed683821..5ed390cc31c7 100644
> >--- a/include/asm-generic/global_data.h
> >+++ b/include/asm-generic/global_data.h
> >@@ -52,6 +52,9 @@ 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 */
> >+#ifdef CONFIG_ENV_EFI
> >+	int env_efi_prio;		/* Priority of the UEFI variables */
> >+#endif
> >
> >  	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..32a03014690f 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,26 @@ struct env_driver {
> >  	 */
> >  	int (*save)(void);
> >
> >+#ifdef CONFIG_ENV_EFI
> >+	/**
> >+	 * 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);
> >+#endif
> >+
> >  	/**
> >  	 * init() - Set up the initial pre-relocation environment
> >  	 *
> >@@ -312,6 +333,16 @@ 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_ENV_EFI
> >+extern struct hsearch_data efi_var_htab;
> >+extern struct hsearch_data efi_nv_var_htab;
> >+
> >+int env_efi_import(const char *buf, int check);
> >+int env_efi_export(env_t *env_out);
> >+int env_efi_load(void);
> >+int env_efi_save(void);
> >+#endif
> >+
> >  #endif /* DO_DEPS_ONLY */
> >
> >  #endif /* _ENVIRONMENT_H_ */
> >
> 

[U-Boot] [PATCH v3 3/7] efi_loader: variable: split UEFI variables from U-Boot environment

[AKASHI Takahiro]

On Tue, Jun 04, 2019 at 11:31:24PM +0200, Heinrich Schuchardt wrote:
> On 6/4/19 8:52 AM, AKASHI Takahiro wrote:
> >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/Kconfig        |  10 +
> >  lib/efi_loader/efi_variable.c | 342 ++++++++++++++++++++++++++--------
> >  2 files changed, 275 insertions(+), 77 deletions(-)
> >
> >diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> >index cd5436c576b1..8bf4b1754d06 100644
> >--- a/lib/efi_loader/Kconfig
> >+++ b/lib/efi_loader/Kconfig
> >@@ -18,6 +18,16 @@ config EFI_LOADER
> >
> >  if EFI_LOADER
> >
> >+choice
> >+	prompt "Select variables storage"
> >+	default EFI_VARIABLE_USE_ENV
> >+
> >+config EFI_VARIABLE_USE_ENV
> >+	bool "Same device as U-Boot environment"
> >+	select ENV_EFI
> >+
> >+endchoice
> >+
> >  config EFI_GET_TIME
> >  	bool "GetTime() runtime service"
> >  	depends on DM_RTC
> >diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> >index e56053194dae..d9887be938c2 100644
> >--- a/lib/efi_loader/efi_variable.c
> >+++ b/lib/efi_loader/efi_variable.c
> >@@ -48,6 +48,66 @@
> >   * 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.
> >+ *
> >+ *	struct hsearch_data efi_var_htab
> >+ *	struct hsearch_data efi_nv_var_htab
> >+ */
> >+
> >+static char *env_efi_get(const char *name, bool is_non_volatile)
> >+{
> >+	struct hsearch_data *htab;
> >+	ENTRY e, *ep;
> >+
> >+	/* WATCHDOG_RESET(); */
> >+
> >+	if (is_non_volatile)
> >+		htab = &efi_nv_var_htab;
> >+	else
> >+		htab = &efi_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,
> >+		       bool is_non_volatile)
> >+{
> >+	struct hsearch_data *htab;
> >+	ENTRY e, *ep;
> >+	int ret;
> >+
> >+	if (is_non_volatile)
> >+		htab = &efi_nv_var_htab;
> >+	else
> >+		htab = &efi_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;
> >+}
> >+
> >  #define PREFIX_LEN (strlen("efi_xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx_"))
> >
> >  /**
> >@@ -147,24 +207,12 @@ static const char *parse_attr(const char *str, u32 *attrp)
> >  	return str;
> >  }
> >
> >-/**
> >- * efi_efi_get_variable() - retrieve value of a UEFI variable
> >- *
> >- * This function implements the GetVariable runtime service.
> >- *
> >- * See the Unified Extensible Firmware Interface (UEFI) specification for
> >- * details.
> >- *
> >- * @variable_name:	name of the variable
> >- * @vendor:		vendor GUID
> >- * @attributes:		attributes of the variable
> >- * @data_size:		size of the buffer to which the variable value is copied
> >- * @data:		buffer to which the variable value is copied
> >- * Return:		status code
> >- */
> >-efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >-				     const efi_guid_t *vendor, u32 *attributes,
> >-				     efi_uintn_t *data_size, void *data)
> >+static
> >+efi_status_t EFIAPI efi_get_variable_common(u16 *variable_name,
> >+					    const efi_guid_t *vendor,
> >+					    u32 *attributes,
> >+					    efi_uintn_t *data_size, void *data,
> >+					    bool is_non_volatile)
> >  {
> >  	char *native_name;
> >  	efi_status_t ret;
> >@@ -172,22 +220,19 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >  	const char *val, *s;
> >  	u32 attr;
> >
> >-	EFI_ENTRY("\"%ls\" %pUl %p %p %p", variable_name, vendor, attributes,
> >-		  data_size, data);
> >-
> >  	if (!variable_name || !vendor || !data_size)
> >  		return EFI_EXIT(EFI_INVALID_PARAMETER);
> >
> >  	ret = efi_to_native(&native_name, variable_name, vendor);
> >  	if (ret)
> >-		return EFI_EXIT(ret);
> >+		return ret;
> >
> >  	EFI_PRINT("get '%s'\n", native_name);
> >
> >-	val = env_get(native_name);
> >+	val = env_efi_get(native_name, is_non_volatile);
> >  	free(native_name);
> >  	if (!val)
> >-		return EFI_EXIT(EFI_NOT_FOUND);
> >+		return EFI_NOT_FOUND;
> >
> >  	val = parse_attr(val, &attr);
> >
> >@@ -198,7 +243,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >
> >  		/* number of hexadecimal digits must be even */
> >  		if (len & 1)
> >-			return EFI_EXIT(EFI_DEVICE_ERROR);
> >+			return EFI_DEVICE_ERROR;
> >
> >  		/* two characters per byte: */
> >  		len /= 2;
> >@@ -210,10 +255,10 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >  		}
> >
> >  		if (!data)
> >-			return EFI_EXIT(EFI_INVALID_PARAMETER);
> >+			return EFI_INVALID_PARAMETER;
> >
> >  		if (hex2bin(data, s, len))
> >-			return EFI_EXIT(EFI_DEVICE_ERROR);
> >+			return EFI_DEVICE_ERROR;
> >
> >  		EFI_PRINT("got value: \"%s\"\n", s);
> >  	} else if ((s = prefix(val, "(utf8)"))) {
> >@@ -227,7 +272,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >  		}
> >
> >  		if (!data)
> >-			return EFI_EXIT(EFI_INVALID_PARAMETER);
> >+			return EFI_INVALID_PARAMETER;
> >
> >  		memcpy(data, s, len);
> >  		((char *)data)[len] = '\0';
> >@@ -235,13 +280,67 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >  		EFI_PRINT("got value: \"%s\"\n", (char *)data);
> >  	} else {
> >  		EFI_PRINT("invalid value: '%s'\n", val);
> >-		return EFI_EXIT(EFI_DEVICE_ERROR);
> >+		return EFI_DEVICE_ERROR;
> >  	}
> >
> >  out:
> >  	if (attributes)
> >  		*attributes = attr & EFI_VARIABLE_MASK;
> >
> >+	return ret;
> >+}
> >+
> >+static
> >+efi_status_t EFIAPI efi_get_volatile_variable(u16 *variable_name,
> >+					      const efi_guid_t *vendor,
> >+					      u32 *attributes,
> >+					      efi_uintn_t *data_size,
> >+					      void *data)
> >+{
> >+	return efi_get_variable_common(variable_name, vendor, attributes,
> >+				       data_size, data, false);
> >+}
> >+
> >+efi_status_t EFIAPI efi_get_nonvolatile_variable(u16 *variable_name,
> >+						 const efi_guid_t *vendor,
> >+						 u32 *attributes,
> >+						 efi_uintn_t *data_size,
> >+						 void *data)
> >+{
> >+	return efi_get_variable_common(variable_name, vendor, attributes,
> >+				       data_size, data, true);
> >+}
> >+
> >+/**
> >+ * efi_efi_get_variable() - retrieve value of a UEFI variable
> >+ *
> >+ * This function implements the GetVariable runtime service.
> >+ *
> >+ * See the Unified Extensible Firmware Interface (UEFI) specification for
> >+ * details.
> >+ *
> >+ * @variable_name:	name of the variable
> >+ * @vendor:		vendor GUID
> >+ * @attributes:		attributes of the variable
> >+ * @data_size:		size of the buffer to which the variable value is copied
> >+ * @data:		buffer to which the variable value is copied
> >+ * Return:		status code
> >+ */
> >+efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >+				     const efi_guid_t *vendor, u32 *attributes,
> >+				     efi_uintn_t *data_size, void *data)
> >+{
> >+	efi_status_t ret;
> >+
> >+	EFI_ENTRY("\"%ls\" %pUl %p %p %p", variable_name, vendor, attributes,
> >+		  data_size, data);
> >+
> >+	ret = efi_get_volatile_variable(variable_name, vendor, attributes,
> >+					data_size, data);
> >+	if (ret == EFI_NOT_FOUND)
> >+		ret = efi_get_nonvolatile_variable(variable_name, vendor,
> >+						   attributes, data_size, data);
> >+
> >  	return EFI_EXIT(ret);
> >  }
> >
> >@@ -331,7 +430,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};
> >@@ -387,10 +486,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);
> >
> >@@ -403,77 +531,71 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
> >  	return EFI_EXIT(ret);
> >  }
> >
> >-/**
> >- * efi_efi_set_variable() - set value of a UEFI variable
> >- *
> >- * This function implements the SetVariable runtime service.
> >- *
> >- * See the Unified Extensible Firmware Interface (UEFI) specification for
> >- * details.
> >- *
> >- * @variable_name:	name of the variable
> >- * @vendor:		vendor GUID
> >- * @attributes:		attributes of the variable
> >- * @data_size:		size of the buffer with the variable value
> >- * @data:		buffer with the variable value
> >- * Return:		status code
> >- */
> >-efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >-				     const efi_guid_t *vendor, u32 attributes,
> >-				     efi_uintn_t data_size, const void *data)
> >+static
> >+efi_status_t EFIAPI efi_set_variable_common(u16 *variable_name,
> >+					    const efi_guid_t *vendor,
> >+					    u32 attributes,
> >+					    efi_uintn_t data_size,
> >+					    const void *data,
> >+					    bool is_non_volatile)
> >  {
> >  	char *native_name = NULL, *val = NULL, *s;
> >-	efi_status_t ret = EFI_SUCCESS;
> >+	efi_uintn_t size;
> >  	u32 attr;
> >-
> >-	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
> >-		  data_size, data);
> >+	efi_status_t ret = EFI_SUCCESS;
> >
> >  	/* TODO: implement APPEND_WRITE */
> >  	if (!variable_name || !vendor ||
> >  	    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
> >  		ret = EFI_INVALID_PARAMETER;
> >-		goto out;
> >+		goto err;
> >  	}
> >
> >  	ret = efi_to_native(&native_name, variable_name, vendor);
> >  	if (ret)
> >-		goto out;
> >+		goto err;
> >
> >  #define ACCESS_ATTR (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)
> >
> >-	if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
> >-		/* delete the variable: */
> >-		env_set(native_name, NULL);
> >-		ret = EFI_SUCCESS;
> >-		goto out;
> >+	/* check if a variable exists */
> >+	size = 0;
> >+	ret = EFI_CALL(efi_get_variable(variable_name, vendor, &attr,
> >+					&size, NULL));
> >+	if (ret == EFI_BUFFER_TOO_SMALL) {
> >+		if ((is_non_volatile && !(attr & EFI_VARIABLE_NON_VOLATILE)) ||
> >+		    (!is_non_volatile && (attr & EFI_VARIABLE_NON_VOLATILE))) {
> >+			ret = EFI_INVALID_PARAMETER;
> >+			goto err;
> >+		}
> >  	}
> >
> >-	val = env_get(native_name);
> >-	if (val) {
> >-		parse_attr(val, &attr);
> >-
> >-		/* We should not free val */
> >-		val = NULL;
> >-		if (attr & READ_ONLY) {
> >-			ret = EFI_WRITE_PROTECTED;
> >+	/* delete a variable */
> >+	if (data_size == 0 || !(attributes & ACCESS_ATTR)) {
> >+		if (size) {
> >+			if (attr & READ_ONLY) {
> >+				ret = EFI_WRITE_PROTECTED;
> >+				goto err;
> >+			}
> >  			goto out;
> >  		}
> >+		ret = EFI_SUCCESS;
> >+		goto err; /* not error, but nothing to do */
> >+	}
> >
> >+	/* create/modify a variable */
> >+	if (size && attr != attributes) {
> >  		/*
> >  		 * attributes won't be changed
> >  		 * TODO: take care of APPEND_WRITE once supported
> >  		 */
> >-		if (attr != attributes) {
> >-			ret = EFI_INVALID_PARAMETER;
> >-			goto out;
> >-		}
> >+		ret = EFI_INVALID_PARAMETER;
> >+		goto err;
> >  	}
> >
> >  	val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1);
> >  	if (!val) {
> >  		ret = EFI_OUT_OF_RESOURCES;
> >-		goto out;
> >+		goto err;
> >  	}
> >
> >  	s = val;
> >@@ -487,7 +609,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >  		       EFI_VARIABLE_RUNTIME_ACCESS);
> >  	s += sprintf(s, "{");
> >  	while (attributes) {
> >-		u32 attr = 1 << (ffs(attributes) - 1);
> >+		attr = 1 << (ffs(attributes) - 1);
> >
> >  		if (attr == EFI_VARIABLE_NON_VOLATILE)
> >  			s += sprintf(s, "nv");
> >@@ -509,12 +631,78 @@ 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))
> >+out:
> >+	ret = EFI_SUCCESS;
> >+	if (env_efi_set(native_name, val, is_non_volatile))
> >  		ret = EFI_DEVICE_ERROR;
> >
> >-out:
> >+err:
> >  	free(native_name);
> >  	free(val);
> >
> >+	return ret;
> >+}
> >+
> >+static
> >+efi_status_t EFIAPI efi_set_volatile_variable(u16 *variable_name,
> 
> Once you have implemented this we can make the variable service
> available at runtime. So I suggest to define everything here as __runtime.

Good question! I intended so when I started this work, but
we can't do that partly because bunch of code from U-Boot, which is not
part of RUNTIME_CODE, will be exercised and partly because all the entries
in a hash table are allocated by *malloc*, which are again not part of
RUNTIME_DATA.
It is quite painful to modify the code and data so that they are accessible
at UEFI runtime.

Instead, I implemented "caching" features for runtime access.
I'm going to post the patch set (as RFC) later today.

> Why do you any of these three functions (efi_set_volatile_variable(),
> efi_set_nonvolatile_variable(), efi_set_nonvolatile_variable()). Just
> use an if based on attributes in efi_set_variable() to call
> env_efi_save, when a non-volatile function is changed.

I don't get you point. Please clarify your comment.

> What is the advantage of having two separate RAM stores? Can't the save
> function sort out what to save and what not to save according to the NV
> flag?

Technically, we can do that, but it is nothing but inefficient.
Just FYI, EDK2 also maintains separate buffers.

> >+					      const efi_guid_t *vendor,
> >+					      u32 attributes,
> >+					      efi_uintn_t data_size,
> >+					      const void *data)
> >+{
> >+	return efi_set_variable_common(variable_name, vendor, attributes,
> >+				       data_size, data, false);
> >+}
> >+
> >+efi_status_t EFIAPI efi_set_nonvolatile_variable(u16 *variable_name,
> >+						 const efi_guid_t *vendor,
> >+						 u32 attributes,
> >+						 efi_uintn_t data_size,
> >+						 const void *data)
> >+{
> >+	efi_status_t ret;
> >+
> >+	ret = efi_set_variable_common(variable_name, vendor, attributes,
> >+				      data_size, data, true);
> >+	if (ret == EFI_SUCCESS)
> >+		/* FIXME: what if save failed? */
> >+		if (env_efi_save())
> 
> Somewhere we need the ability to switch between different backends. Will
> that be in env_efi_save()?

That is why I put the related code in "env."

Thanks,
-Takahiro Akashi

> >+			ret = EFI_DEVICE_ERROR;
> >+
> >+	return ret;
> >+}
> >+
> >+/**
> >+ * efi_efi_set_variable() - set value of a UEFI variable
> >+ *
> >+ * This function implements the SetVariable runtime service.
> >+ *
> >+ * See the Unified Extensible Firmware Interface (UEFI) specification for
> >+ * details.
> >+ *
> >+ * @variable_name:	name of the variable
> >+ * @vendor:		vendor GUID
> >+ * @attributes:		attributes of the variable
> >+ * @data_size:		size of the buffer with the variable value
> >+ * @data:		buffer with the variable value
> >+ * Return:		status code
> >+ */
> >+efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >+				     const efi_guid_t *vendor, u32 attributes,
> >+				     efi_uintn_t data_size, const void *data)
> >+{
> >+	efi_status_t ret;
> >+
> >+	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
> >+		  data_size, data);
> >+
> >+	if (attributes & EFI_VARIABLE_NON_VOLATILE)
> >+		ret = efi_set_nonvolatile_variable(variable_name, vendor,
> >+						   attributes,
> >+						   data_size, data);
> >+	else
> >+		ret = efi_set_volatile_variable(variable_name, vendor,
> >+						attributes, data_size, data);
> >+
> >  	return EFI_EXIT(ret);
> >  }
> >
> 

[U-Boot] [PATCH v3 4/7] efi_loader: load saved non-volatile variables at init

[AKASHI Takahiro]

On Tue, Jun 04, 2019 at 11:38:27PM +0200, Heinrich Schuchardt wrote:
> On 6/4/19 8:52 AM, AKASHI Takahiro wrote:
> >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 | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> >diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> >index 8691d686d29d..45d6aca051f3 100644
> >--- a/lib/efi_loader/efi_setup.c
> >+++ b/lib/efi_loader/efi_setup.c
> >@@ -8,6 +8,7 @@
> >  #include <common.h>
> >  #include <bootm.h>
> >  #include <efi_loader.h>
> >+#include <environment.h>
> >
> >  #define OBJ_LIST_NOT_INITIALIZED 1
> >
> >@@ -102,6 +103,11 @@ efi_status_t efi_init_obj_list(void)
> >  	/* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
> >  	switch_to_non_secure_mode();
> >
> >+#ifdef CONFIG_EFI_VARIABLE_USE_ENV
> 
> No clue what ENV refers to here as we are not talking about U-Boot
> environment variables anymore. How about CONFIG_EFI_PERSISTENT_VARIABLES.

It will be trivial once you take a look at "menuconfig."

> 
> >+	/* Load non-volatile variables */
> >+	env_efi_load();
> 
> Can't we make env_efi_load() a __weak function which does nothing. If we
> have a backend, that backend replaces the weak function. That way we
> restrict the config variables to the Makefile.

This is a discussion.
There can be different approaches here, so
I would like to deter to a developer who will implement a next
backing storage (other than U-Boot env), which is likely to be
Standalone MM services for secure boot.
Unfortunately I'm not responsible for that(StMM).

I hope that some Linaro engineers may have comments here.

Thanks,
-Takahiro Akashi

> Regards
> 
> Heinrich
> 
> >+#endif
> >+
> >  	/* Define supported languages */
> >  	ret = efi_init_platform_lang();
> >  	if (ret != EFI_SUCCESS)
> >
> 

[U-Boot] [PATCH v3 5/7] efi_loader: bootmgr: make BootNext non-volatile

[Ilias Apalodimas]

On Tue, Jun 04, 2019 at 03:52:09PM +0900, AKASHI Takahiro wrote:
> 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 43791422c819..b2102c5b5af2 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -210,7 +210,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) {

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[U-Boot] [PATCH v3 6/7] cmd: efidebug: make some boot variables non-volatile

[Ilias Apalodimas]

On Tue, Jun 04, 2019 at 03:52:10PM +0900, AKASHI Takahiro wrote:
> Boot####, BootOrder and BootNext should be non-volatile.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Reviewed-by: Heinrich Schuchardt
> ---
>  cmd/efidebug.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index c4ac9dd634e2..e65722625455 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -558,6 +558,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));
> @@ -909,6 +910,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));
> @@ -964,6 +966,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));

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[U-Boot] [PATCH v3 1/7] env: save UEFI non-volatile variables in dedicated storage

[Ilias Apalodimas]

Hi Akashi-san

Thanks for doing this!

[...]
> +	return 0;
> +}
> +
> +int env_efi_save(void)
> +{
> +#ifdef CONFIG_ENV_IS_NOWHERE
One of the 'features' we discussed is the ability to have CONFIG_ENV_IS_NOWHERE
set (not allowing users to change the U-Boot ENV) and still be able to store 
UEFI variables. Doesn't this ifdef prevent that from happening?
> +	return 0;
> +#else
> +	struct env_driver *drv = NULL;
> +	int ret;
> +
> +	if (!efi_nv_var_htab.table)
> +		return 0;
> +
> +	if (gd->env_efi_prio == -1) {
> +		pr_warn("No UEFI non-volatile variable storage\n");
> +		return -1;
> +	}
> +
> +	drv = _env_driver_lookup(env_get_location(ENVOP_EFI, gd->env_efi_prio));
> +	if (!drv) {
> +		pr_warn("No UEFI non-volatile variable storage\n");
> +		return -1;
> +	}
> +
> +	ret = drv->efi_save();
> +	if (ret)
> +		pr_err("Saving UEFI non-volatile variable failed\n");
> +
> +	return ret;
> +#endif
> +}
> +
> +/* This function should be called only once at init */
> +int env_efi_load(void)
> +{
> +#ifndef CONFIG_ENV_IS_NOWHERE
ditto
> +	struct env_driver *drv;
> +	int prio;
> +	enum env_location loc;
> +#endif
> +	int ret;
> +
> +	/* volatile variables */
> +	if (!efi_var_htab.table) {
> +		ret = himport_r(&efi_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
> +		if (!ret) {
> +			pr_err("Creating UEFI volatile variables failed\n");
> +			return -1;
> +		}
> +	}
> +
> +#ifndef CONFIG_ENV_IS_NOWHERE
ditto
> +	gd->env_efi_prio = -1;
> +
> +	/* non-volatile variables */
> +	if (efi_nv_var_htab.table)
> +		return 0;
> +
> +	for (drv = NULL, 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)) {
> +		pr_warn("No UEFI non-volatile variable storage\n");
> +		goto skip_load;
> +	}
> +
> +	gd->env_efi_prio = prio;
> +
> +	ret = drv->efi_load();
> +	if (ret) {
> +		pr_err("Loading UEFI non-volatile variables failed\n");
> +		return -1;
> +	}
> +skip_load:
> +#endif /* CONFIG_ENV_IS_NOWHERE */
[...]

Thanks
/Ilias

[U-Boot] [PATCH v3 1/7] env: save UEFI non-volatile variables in dedicated storage

[AKASHI Takahiro]

Ilias,

On Tue, Jun 11, 2019 at 01:59:52PM +0300, Ilias Apalodimas wrote:
> Hi Akashi-san
> 
> Thanks for doing this!
> 
> [...]
> > +	return 0;
> > +}
> > +
> > +int env_efi_save(void)
> > +{
> > +#ifdef CONFIG_ENV_IS_NOWHERE
> One of the 'features' we discussed is the ability to have CONFIG_ENV_IS_NOWHERE
> set (not allowing users to change the U-Boot ENV) and still be able to store 
> UEFI variables. Doesn't this ifdef prevent that from happening?

Yeah, I don't forget this requirement, but it is not yet agreed that
"driver" framework be shared between U-Boot environment and
UEFI variables. As you can see, env/fat.c has some duplicated code
in my current implementation and more will be added for other devices.

So I will fix this issue depending on the discussion.

-Takahiro Akashi

> > +	return 0;
> > +#else
> > +	struct env_driver *drv = NULL;
> > +	int ret;
> > +
> > +	if (!efi_nv_var_htab.table)
> > +		return 0;
> > +
> > +	if (gd->env_efi_prio == -1) {
> > +		pr_warn("No UEFI non-volatile variable storage\n");
> > +		return -1;
> > +	}
> > +
> > +	drv = _env_driver_lookup(env_get_location(ENVOP_EFI, gd->env_efi_prio));
> > +	if (!drv) {
> > +		pr_warn("No UEFI non-volatile variable storage\n");
> > +		return -1;
> > +	}
> > +
> > +	ret = drv->efi_save();
> > +	if (ret)
> > +		pr_err("Saving UEFI non-volatile variable failed\n");
> > +
> > +	return ret;
> > +#endif
> > +}
> > +
> > +/* This function should be called only once at init */
> > +int env_efi_load(void)
> > +{
> > +#ifndef CONFIG_ENV_IS_NOWHERE
> ditto
> > +	struct env_driver *drv;
> > +	int prio;
> > +	enum env_location loc;
> > +#endif
> > +	int ret;
> > +
> > +	/* volatile variables */
> > +	if (!efi_var_htab.table) {
> > +		ret = himport_r(&efi_var_htab, NULL, 0, '\0', 0, 0, 0, NULL);
> > +		if (!ret) {
> > +			pr_err("Creating UEFI volatile variables failed\n");
> > +			return -1;
> > +		}
> > +	}
> > +
> > +#ifndef CONFIG_ENV_IS_NOWHERE
> ditto
> > +	gd->env_efi_prio = -1;
> > +
> > +	/* non-volatile variables */
> > +	if (efi_nv_var_htab.table)
> > +		return 0;
> > +
> > +	for (drv = NULL, 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)) {
> > +		pr_warn("No UEFI non-volatile variable storage\n");
> > +		goto skip_load;
> > +	}
> > +
> > +	gd->env_efi_prio = prio;
> > +
> > +	ret = drv->efi_load();
> > +	if (ret) {
> > +		pr_err("Loading UEFI non-volatile variables failed\n");
> > +		return -1;
> > +	}
> > +skip_load:
> > +#endif /* CONFIG_ENV_IS_NOWHERE */
> [...]
> 
> Thanks
> /Ilias

[U-Boot] [PATCH v3 0/7] efi_loader: non-volatile variables support

[Wolfgang Denk]

Dear Takahiro,

In message <20190604065211.15907-1-takahiro.akashi@linaro.org> you 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.

These are all valid arguments, and actually there are also a number
of U-Boot variables that are 100% volatile (say, "filesize") so
saving them to the persistent environment is just a waste of
resources.

> 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.

I dislike this patch, because it has a narrow view, is prety
intrusive and not generally usable.


I suggest adding a "volatile" property to the already existing
mechanism of environment variable flags, and excluding variables
with that flag from "env export" and "env save" operations.

The advantages of this approach:

- only minimal code additions needed
- generally useful, i. e. not only for UEFI but also for "normal"
  U-Boot environment variables (think of "filesize" etc.)
- independent of environment storage, i. e. it is not limited to FAT
  and also works with "env export".

> Known issues/restriction/TODO:
> * 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.

This could be handled using glags, too.

> * Only FAT can be enabled for persistent storage for UEFI non-volatile
>   variables.

If you want to save / export _only_ UEFI variables (but not the rest
of the U-Boot environment), you could for example add a "UEFI"
property to the variable flags, and extend the "save" and "export"
commands to process only variables with a given property (in your
case, UEFI).  Again, this requires no special code, is
storage-agnostic and fits well into the existing code base.  The
major benefit is that this is a useful extension not only for UEFI,
but for other use cases as well.

> * The whole area of storage will be saved at every update of one variable.
>   It can be optimized.

If it is desired that variable changes get immediately written to
the persistent storage, this could also be easily implemented using
the variable flags - just add an "autosave" property and run
"env save" whenever the value f such a variable changes.

> * An error during saving may cause inconsistency between cache (hash table)
>   and the storage.

It would also cause that the written copy is invalid (checksum), so
the redundant copy of the environment will be used on next bot.
This is the same for all errors when storing the persistent
environment.

>  cmd/efidebug.c                    |   3 +
>  cmd/nvedit.c                      |   3 +-
>  cmd/nvedit_efi.c                  |  15 +-
>  env/Kconfig                       |  39 ++++
>  env/env.c                         | 155 ++++++++++++-
>  env/fat.c                         | 105 +++++++++
>  include/asm-generic/global_data.h |   3 +
>  include/environment.h             |  31 +++
>  lib/efi_loader/Kconfig            |  10 +
>  lib/efi_loader/efi_bootmgr.c      |   3 +-
>  lib/efi_loader/efi_setup.c        |   6 +
>  lib/efi_loader/efi_variable.c     | 354 +++++++++++++++++++++++-------
>  12 files changed, 641 insertions(+), 86 deletions(-)

This patch is pretty intrusive and single-purpose.

Can you please think about my suggestion?  I feel it would be much
less intrusive and much more usefule especially for non-UEFI use
cases.  Also, it helps to avoid a few of your limitations.


Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The Gates in my computer are AND, OR and NOT; they are not Bill.