[PATCH v3 0/3] With this patch series we implement SPI Flash storage for EFI

[PATCH v3 0/3] With this patch series we implement SPI Flash storage for EFI

[Shantur Rathore]

variables.

Changes in v3:
- Fixed compiler warnings.

Changes in v2:
- Refactored efi_var_file to move common parts out as requested
- Changed ifdefs to use CONFIG_IS_DEFINED
- Fixed typos

Shantur Rathore (3):
  efi_var_file: refactor to move buffer functions
  efi_vars: Implement SPI Flash store
  defconfig: rockpro64: Enable SF EFI var store

 configs/rockpro64-rk3399_defconfig |   2 +
 include/efi_variable.h             |  26 +++++++
 lib/efi_loader/Kconfig             |  25 ++++++
 lib/efi_loader/Makefile            |   3 +-
 lib/efi_loader/efi_var_common.c    | 109 ++++++++++++++++++++++++++
 lib/efi_loader/efi_var_file.c      | 121 -----------------------------
 lib/efi_loader/efi_var_sf.c        |  91 ++++++++++++++++++++++
 lib/efi_loader/efi_variable.c      |  17 +++-
 8 files changed, 270 insertions(+), 124 deletions(-)
 create mode 100644 lib/efi_loader/efi_var_sf.c

-- 
2.40.1

[PATCH v3 1/3] efi_var_file: refactor to move buffer functions

[Shantur Rathore]

Currently efi_var_file.c has functions to store/read
EFI variables to/from memory buffer. These functions
can be used with other EFI variable stores so move
them out to efi_var_common.c

Signed-off-by: Shantur Rathore <i@shantur.com>
---

(no changes since v1)

 include/efi_variable.h          |   5 ++
 lib/efi_loader/Makefile         |   2 +-
 lib/efi_loader/efi_var_common.c | 109 ++++++++++++++++++++++++++++
 lib/efi_loader/efi_var_file.c   | 121 --------------------------------
 lib/efi_loader/efi_variable.c   |   8 ++-
 5 files changed, 122 insertions(+), 123 deletions(-)

diff --git a/include/efi_variable.h b/include/efi_variable.h
index 805e6c5f1e..bd0a31fc3e 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -161,6 +161,11 @@ efi_status_t efi_var_to_file(void);
 efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *lenp,
 					    u32 check_attr_mask);
 
+/* GUID used by Shim to store the MOK database */
+#define SHIM_LOCK_GUID \
+	EFI_GUID(0x605dab50, 0xe046, 0x4300, \
+		 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
+
 /**
  * efi_var_restore() - restore EFI variables from buffer
  *
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 8d31fc61c6..33b1910249 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -66,7 +66,7 @@ obj-y += efi_string.o
 obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
 obj-y += efi_var_common.o
 obj-y += efi_var_mem.o
-obj-y += efi_var_file.o
+obj-$(CONFIG_EFI_VARIABLE_FILE_STORE) += efi_var_file.o
 ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
 obj-y += efi_variable_tee.o
 else
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
index ad50bffd2b..7509c30b5a 100644
--- a/lib/efi_loader/efi_var_common.c
+++ b/lib/efi_loader/efi_var_common.c
@@ -10,6 +10,7 @@
 #include <efi_loader.h>
 #include <efi_variable.h>
 #include <stdlib.h>
+#include <u-boot/crc.h>
 
 enum efi_secure_mode {
 	EFI_MODE_SETUP,
@@ -40,6 +41,7 @@ static const struct efi_auth_var_name_type name_type[] = {
 
 static bool efi_secure_boot;
 static enum efi_secure_mode efi_secure_mode;
+static const efi_guid_t shim_lock_guid = SHIM_LOCK_GUID;
 
 /**
  * efi_efi_get_variable() - retrieve value of a UEFI variable
@@ -417,3 +419,110 @@ void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size)
 
 	return buf;
 }
+
+efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *lenp,
+					    u32 check_attr_mask)
+{
+	size_t len = EFI_VAR_BUF_SIZE;
+	struct efi_var_file *buf;
+	struct efi_var_entry *var, *old_var;
+	size_t old_var_name_length = 2;
+
+	*bufp = NULL; /* Avoid double free() */
+	buf = calloc(1, len);
+	if (!buf)
+		return EFI_OUT_OF_RESOURCES;
+	var = buf->var;
+	old_var = var;
+	for (;;) {
+		efi_uintn_t data_length, var_name_length;
+		u8 *data;
+		efi_status_t ret;
+
+		if ((uintptr_t)buf + len <=
+		    (uintptr_t)var->name + old_var_name_length)
+			return EFI_BUFFER_TOO_SMALL;
+
+		var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name;
+		memcpy(var->name, old_var->name, old_var_name_length);
+		guidcpy(&var->guid, &old_var->guid);
+		ret = efi_get_next_variable_name_int(
+				&var_name_length, var->name, &var->guid);
+		if (ret == EFI_NOT_FOUND)
+			break;
+		if (ret != EFI_SUCCESS) {
+			free(buf);
+			return ret;
+		}
+		old_var_name_length = var_name_length;
+		old_var = var;
+
+		data = (u8 *)var->name + old_var_name_length;
+		data_length = (uintptr_t)buf + len - (uintptr_t)data;
+		ret = efi_get_variable_int(var->name, &var->guid,
+					   &var->attr, &data_length, data,
+					   &var->time);
+		if (ret != EFI_SUCCESS) {
+			free(buf);
+			return ret;
+		}
+		if ((var->attr & check_attr_mask) == check_attr_mask) {
+			var->length = data_length;
+			var = (struct efi_var_entry *)ALIGN((uintptr_t)data + data_length, 8);
+		}
+	}
+
+	buf->reserved = 0;
+	buf->magic = EFI_VAR_FILE_MAGIC;
+	len = (uintptr_t)var - (uintptr_t)buf;
+	buf->crc32 = crc32(0, (u8 *)buf->var,
+			   len - sizeof(struct efi_var_file));
+	buf->length = len;
+	*bufp = buf;
+	*lenp = len;
+
+	return EFI_SUCCESS;
+}
+
+efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
+{
+	struct efi_var_entry *var, *last_var;
+	u16 *data;
+	efi_status_t ret;
+
+	if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
+	    buf->crc32 != crc32(0, (u8 *)buf->var,
+				buf->length - sizeof(struct efi_var_file))) {
+		log_err("Invalid EFI variables file\n");
+		return EFI_INVALID_PARAMETER;
+	}
+
+	last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
+	for (var = buf->var; var < last_var;
+	     var = (struct efi_var_entry *)
+		   ALIGN((uintptr_t)data + var->length, 8)) {
+
+		data = var->name + u16_strlen(var->name) + 1;
+
+		/*
+		 * Secure boot related and volatile variables shall only be
+		 * restored from U-Boot's preseed.
+		 */
+		if (!safe &&
+		    (efi_auth_var_get_type(var->name, &var->guid) !=
+		     EFI_AUTH_VAR_NONE ||
+		     !guidcmp(&var->guid, &shim_lock_guid) ||
+		     !(var->attr & EFI_VARIABLE_NON_VOLATILE)))
+			continue;
+		if (!var->length)
+			continue;
+		if (efi_var_mem_find(&var->guid, var->name, NULL))
+			continue;
+		ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
+				      var->length, data, 0, NULL,
+				      var->time);
+		if (ret != EFI_SUCCESS)
+			log_err("Failed to set EFI variable %ls\n", var->name);
+	}
+	return EFI_SUCCESS;
+}
diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
index d7dba05071..166501a355 100644
--- a/lib/efi_loader/efi_var_file.c
+++ b/lib/efi_loader/efi_var_file.c
@@ -15,17 +15,9 @@
 #include <mapmem.h>
 #include <efi_loader.h>
 #include <efi_variable.h>
-#include <u-boot/crc.h>
 
 #define PART_STR_LEN 10
 
-/* GUID used by Shim to store the MOK database */
-#define SHIM_LOCK_GUID \
-	EFI_GUID(0x605dab50, 0xe046, 0x4300, \
-		 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
-
-static const efi_guid_t shim_lock_guid = SHIM_LOCK_GUID;
-
 /**
  * efi_set_blk_dev_to_system_partition() - select EFI system partition
  *
@@ -53,70 +45,6 @@ static efi_status_t __maybe_unused efi_set_blk_dev_to_system_partition(void)
 	return EFI_SUCCESS;
 }
 
-efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *lenp,
-					    u32 check_attr_mask)
-{
-	size_t len = EFI_VAR_BUF_SIZE;
-	struct efi_var_file *buf;
-	struct efi_var_entry *var, *old_var;
-	size_t old_var_name_length = 2;
-
-	*bufp = NULL; /* Avoid double free() */
-	buf = calloc(1, len);
-	if (!buf)
-		return EFI_OUT_OF_RESOURCES;
-	var = buf->var;
-	old_var = var;
-	for (;;) {
-		efi_uintn_t data_length, var_name_length;
-		u8 *data;
-		efi_status_t ret;
-
-		if ((uintptr_t)buf + len <=
-		    (uintptr_t)var->name + old_var_name_length)
-			return EFI_BUFFER_TOO_SMALL;
-
-		var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name;
-		memcpy(var->name, old_var->name, old_var_name_length);
-		guidcpy(&var->guid, &old_var->guid);
-		ret = efi_get_next_variable_name_int(
-				&var_name_length, var->name, &var->guid);
-		if (ret == EFI_NOT_FOUND)
-			break;
-		if (ret != EFI_SUCCESS) {
-			free(buf);
-			return ret;
-		}
-		old_var_name_length = var_name_length;
-		old_var = var;
-
-		data = (u8 *)var->name + old_var_name_length;
-		data_length = (uintptr_t)buf + len - (uintptr_t)data;
-		ret = efi_get_variable_int(var->name, &var->guid,
-					   &var->attr, &data_length, data,
-					   &var->time);
-		if (ret != EFI_SUCCESS) {
-			free(buf);
-			return ret;
-		}
-		if ((var->attr & check_attr_mask) == check_attr_mask) {
-			var->length = data_length;
-			var = (struct efi_var_entry *)ALIGN((uintptr_t)data + data_length, 8);
-		}
-	}
-
-	buf->reserved = 0;
-	buf->magic = EFI_VAR_FILE_MAGIC;
-	len = (uintptr_t)var - (uintptr_t)buf;
-	buf->crc32 = crc32(0, (u8 *)buf->var,
-			   len - sizeof(struct efi_var_file));
-	buf->length = len;
-	*bufp = buf;
-	*lenp = len;
-
-	return EFI_SUCCESS;
-}
-
 /**
  * efi_var_to_file() - save non-volatile variables as file
  *
@@ -126,7 +54,6 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
  */
 efi_status_t efi_var_to_file(void)
 {
-#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
 	efi_status_t ret;
 	struct efi_var_file *buf;
 	loff_t len;
@@ -150,52 +77,6 @@ error:
 		log_err("Failed to persist EFI variables\n");
 	free(buf);
 	return ret;
-#else
-	return EFI_SUCCESS;
-#endif
-}
-
-efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
-{
-	struct efi_var_entry *var, *last_var;
-	u16 *data;
-	efi_status_t ret;
-
-	if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
-	    buf->crc32 != crc32(0, (u8 *)buf->var,
-				buf->length - sizeof(struct efi_var_file))) {
-		log_err("Invalid EFI variables file\n");
-		return EFI_INVALID_PARAMETER;
-	}
-
-	last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
-	for (var = buf->var; var < last_var;
-	     var = (struct efi_var_entry *)
-		   ALIGN((uintptr_t)data + var->length, 8)) {
-
-		data = var->name + u16_strlen(var->name) + 1;
-
-		/*
-		 * Secure boot related and volatile variables shall only be
-		 * restored from U-Boot's preseed.
-		 */
-		if (!safe &&
-		    (efi_auth_var_get_type(var->name, &var->guid) !=
-		     EFI_AUTH_VAR_NONE ||
-		     !guidcmp(&var->guid, &shim_lock_guid) ||
-		     !(var->attr & EFI_VARIABLE_NON_VOLATILE)))
-			continue;
-		if (!var->length)
-			continue;
-		if (efi_var_mem_find(&var->guid, var->name, NULL))
-			continue;
-		ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
-				      var->length, data, 0, NULL,
-				      var->time);
-		if (ret != EFI_SUCCESS)
-			log_err("Failed to set EFI variable %ls\n", var->name);
-	}
-	return EFI_SUCCESS;
 }
 
 /**
@@ -214,7 +95,6 @@ efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
  */
 efi_status_t efi_var_from_file(void)
 {
-#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
 	struct efi_var_file *buf;
 	loff_t len;
 	efi_status_t ret;
@@ -239,6 +119,5 @@ efi_status_t efi_var_from_file(void)
 		log_err("Invalid EFI variables file\n");
 error:
 	free(buf);
-#endif
 	return EFI_SUCCESS;
 }
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index be95ed44e6..adc5ac6a80 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -357,8 +357,11 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
 	 * Write non-volatile EFI variables to file
 	 * TODO: check if a value change has occured to avoid superfluous writes
 	 */
-	if (attributes & EFI_VARIABLE_NON_VOLATILE)
+#if CONFIG_IS_ENABLED(EFI_VARIABLE_FILE_STORE)
+	if (attributes & EFI_VARIABLE_NON_VOLATILE) {
 		efi_var_to_file();
+	}
+#endif
 
 	return EFI_SUCCESS;
 }
@@ -466,9 +469,12 @@ efi_status_t efi_init_variables(void)
 	if (ret != EFI_SUCCESS)
 		return ret;
 
+#if CONFIG_IS_ENABLED(EFI_VARIABLE_FILE_STORE)
 	ret = efi_var_from_file();
 	if (ret != EFI_SUCCESS)
 		return ret;
+#endif
+
 	if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) {
 		ret = efi_var_restore((struct efi_var_file *)
 				      __efi_var_file_begin, true);
-- 
2.40.1

[PATCH v3 2/3] efi_vars: Implement SPI Flash store

[Shantur Rathore]

Currently U-boot uses ESP as storage for EFI variables.
Devices with SPI Flash are used for storing environment with this
commit we allow EFI variables to be stored on SPI Flash.

Signed-off-by: Shantur Rathore <i@shantur.com>
---

(no changes since v1)

 include/efi_variable.h        | 21 ++++++++
 lib/efi_loader/Kconfig        | 25 ++++++++++
 lib/efi_loader/Makefile       |  1 +
 lib/efi_loader/efi_var_sf.c   | 91 +++++++++++++++++++++++++++++++++++
 lib/efi_loader/efi_variable.c | 15 ++++--
 5 files changed, 149 insertions(+), 4 deletions(-)
 create mode 100644 lib/efi_loader/efi_var_sf.c

diff --git a/include/efi_variable.h b/include/efi_variable.h
index bd0a31fc3e..f90bbb0548 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -190,6 +190,27 @@ efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
  */
 efi_status_t efi_var_from_file(void);
 
+/**
+ * efi_var_from_sf() - read variables from SPI Flash
+ *
+ * EFI variable buffer is read from SPI Flash at offset defined with
+ * CONFIG_EFI_VARIABLE_SF_OFFSET of length CONFIG_EFI_VAR_BUF_SIZE
+ *
+ *
+ * Return:	status code
+ */
+efi_status_t efi_var_from_sf(void);
+
+/**
+ * efi_var_to_sf() - save non-volatile variables to SPI Flash
+ *
+ * EFI variable buffer is saved to SPI Flash at offset defined with
+ * CONFIG_EFI_VARIABLE_SF_OFFSET of length CONFIG_EFI_VAR_BUF_SIZE.
+ *
+ * Return:	status code
+ */
+efi_status_t efi_var_to_sf(void);
+
 /**
  * efi_var_mem_init() - set-up variable list
  *
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 2e3935467c..d3200cb3e3 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -54,6 +54,17 @@ config EFI_VARIABLE_FILE_STORE
 	  Select this option if you want non-volatile UEFI variables to be
 	  stored as file /ubootefi.var on the EFI system partition.
 
+config EFI_VARIABLE_SF_STORE
+	bool "Store non-volatile UEFI variables in SPI Flash"
+	depends on SPI_FLASH
+	help
+	  Select this option if you want non-volatile UEFI variables to be
+	  stored in SPI Flash.
+	  Define CONFIG_EFI_VARIABLE_SF_OFFSET as offset in SPI Flash to use as
+	  the storage for variables. CONFIG_EFI_VAR_BUF_SIZE defines the space
+	  needed.
+
+
 config EFI_MM_COMM_TEE
 	bool "UEFI variables storage service via the trusted world"
 	depends on OPTEE
@@ -108,6 +119,20 @@ config EFI_VARIABLE_NO_STORE
 
 endchoice
 
+config EFI_VARIABLE_SF_OFFSET
+	hex "EFI variables in SPI flash offset"
+	depends on EFI_VARIABLE_SF_STORE
+	help
+	  Offset from the start of the SPI Flash where EFI variables will be stored.
+	  This should be aligned to the sector size of SPI Flash.
+
+config EFI_VARIABLE_SF_DEVICE_INDEX
+	int "Device Index for target SPI Flash"
+	default 0
+	help
+	  The index of SPI Flash device used for storing EFI variables. This would be
+	  needed if there are more than 1 SPI Flash devices available to use.
+
 config EFI_VARIABLES_PRESEED
 	bool "Initial values for UEFI variables"
 	depends on !EFI_MM_COMM_TEE
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 33b1910249..823e0a759f 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
 obj-y += efi_var_common.o
 obj-y += efi_var_mem.o
 obj-$(CONFIG_EFI_VARIABLE_FILE_STORE) += efi_var_file.o
+obj-$(CONFIG_EFI_VARIABLE_SF_STORE) += efi_var_sf.o
 ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
 obj-y += efi_variable_tee.o
 else
diff --git a/lib/efi_loader/efi_var_sf.c b/lib/efi_loader/efi_var_sf.c
new file mode 100644
index 0000000000..79ab99640c
--- /dev/null
+++ b/lib/efi_loader/efi_var_sf.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * SPI Flash interface for UEFI variables
+ *
+ * Copyright (c) 2023, Shantur Rathore
+ */
+
+#define LOG_CATEGORY LOGC_EFI
+
+#include <efi_loader.h>
+#include <efi_variable.h>
+#include <spi_flash.h>
+#include <dm.h>
+
+efi_status_t efi_var_to_sf(void)
+{
+	efi_status_t ret;
+	struct efi_var_file *buf;
+	loff_t len;
+	struct udevice *sfdev;
+
+	ret = efi_var_collect(&buf, &len, EFI_VARIABLE_NON_VOLATILE);
+	if (len > EFI_VAR_BUF_SIZE) {
+		log_err("EFI var buffer length more than target SPI Flash size");
+		ret = EFI_OUT_OF_RESOURCES;
+		goto error;
+	}
+
+	log_debug("%s - Got buffer to write buf->len : %d\n", __func__, buf->length);
+
+	if (ret != EFI_SUCCESS)
+		goto error;
+
+	ret = uclass_get_device(UCLASS_SPI_FLASH, CONFIG_EFI_VARIABLE_SF_DEVICE_INDEX, &sfdev);
+	if (ret)
+		goto error;
+
+	ret = spi_flash_erase_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, EFI_VAR_BUF_SIZE);
+	log_debug("%s - Erased SPI Flash offset %x\n", __func__, CONFIG_EFI_VARIABLE_SF_OFFSET);
+	if (ret)
+		goto error;
+
+	ret = spi_flash_write_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, len, buf);
+	log_debug("%s - Wrote buffer to SPI Flash : %ld\n", __func__, ret);
+
+	if (ret)
+		goto error;
+
+	ret = EFI_SUCCESS;
+error:
+	if (ret)
+		log_err("Failed to persist EFI variables in SF\n");
+	free(buf);
+	return ret;
+}
+
+efi_status_t efi_var_from_sf(void)
+{
+	struct efi_var_file *buf;
+	efi_status_t ret;
+	struct udevice *sfdev;
+
+	buf = calloc(1, EFI_VAR_BUF_SIZE);
+	if (!buf) {
+		log_err("%s - Unable to allocate buffer\n", __func__);
+		return EFI_OUT_OF_RESOURCES;
+	}
+
+	ret = uclass_get_device(UCLASS_SPI_FLASH, 0, &sfdev);
+	if (ret)
+		goto error;
+
+	ret = spi_flash_read_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET,
+		EFI_VAR_BUF_SIZE, buf);
+
+	log_debug("%s - read buffer buf->length: %x\n", __func__, buf->length);
+
+	if (ret || buf->length < sizeof(struct efi_var_file)) {
+		log_err("%s - buffer read from SPI Flash isn't valid\n", __func__);
+		goto error;
+	}
+
+	ret = efi_var_restore(buf, false);
+	if (ret != EFI_SUCCESS)
+		log_err("%s - Unable to restore EFI variables from buffer\n", __func__);
+
+	ret = EFI_SUCCESS;
+error:
+	free(buf);
+	return ret;
+}
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index adc5ac6a80..f73fb40061 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -354,14 +354,16 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
 		ret = EFI_SUCCESS;
 
 	/*
-	 * Write non-volatile EFI variables to file
+	 * Write non-volatile EFI variables to file or SPI Flash
 	 * TODO: check if a value change has occured to avoid superfluous writes
 	 */
-#if CONFIG_IS_ENABLED(EFI_VARIABLE_FILE_STORE)
 	if (attributes & EFI_VARIABLE_NON_VOLATILE) {
+#if CONFIG_IS_ENABLED(EFI_VARIABLE_FILE_STORE)
 		efi_var_to_file();
-	}
+#elif CONFIG_IS_ENABLED(EFI_VARIABLE_SF_STORE)
+		efi_var_to_sf();
 #endif
+	}
 
 	return EFI_SUCCESS;
 }
@@ -471,9 +473,14 @@ efi_status_t efi_init_variables(void)
 
 #if CONFIG_IS_ENABLED(EFI_VARIABLE_FILE_STORE)
 	ret = efi_var_from_file();
+#elif CONFIG_IS_ENABLED(EFI_VARIABLE_SF_STORE)
+	ret = efi_var_from_sf();
+#else
+	ret = EFI_SUCCESS;
+#endif
+
 	if (ret != EFI_SUCCESS)
 		return ret;
-#endif
 
 	if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) {
 		ret = efi_var_restore((struct efi_var_file *)
-- 
2.40.1

[PATCH v3 3/3] defconfig: rockpro64: Enable SF EFI var store

[Shantur Rathore]

RockPro64 uses SPI Flash for storing env, also use it store
EFI variables.

Signed-off-by: Shantur Rathore <i@shantur.com>
---

 configs/rockpro64-rk3399_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
index 4cd6b76665..f550f2ae43 100644
--- a/configs/rockpro64-rk3399_defconfig
+++ b/configs/rockpro64-rk3399_defconfig
@@ -8,6 +8,8 @@ CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
 CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x300000
 CONFIG_ENV_SIZE=0x8000
 CONFIG_ENV_OFFSET=0x3F8000
+CONFIG_EFI_VARIABLE_SF_STORE=y
+CONFIG_EFI_VARIABLE_SF_OFFSET=0x7D0000
 CONFIG_DEFAULT_DEVICE_TREE="rk3399-rockpro64"
 CONFIG_DM_RESET=y
 CONFIG_ROCKCHIP_RK3399=y
-- 
2.40.1

Re: [PATCH v3 2/3] efi_vars: Implement SPI Flash store

[Ilias Apalodimas]

Hi Shantur

I have a few remarks on the architecture.
Up to now, we are supporting
1. Variables on a file
2. Variables on an RPMB

The reason those two are in different files is that we generally
expect to use different bootime services and few differences in
efi_variables_boot_exit_notify() and efi_init_variables().
Whatever is common, for example the runtime functions are common
across those two since they both implement variable runtime service
via the memory backend, goes into efi_var_common.c, the memory backend
operations should go in efi_var_mem.c.

Since the SPI and file storage are similar -- and probably any storage
medium controlled by the non-secure world, I am fine treating treat
efi_variable.c as the variable management for the non-secure world and
see if that needs changing in the future.

As Heinrich pointed out the functions you are moving are better off
moved into efi_var_mem.c.

[...]

> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index adc5ac6a80..f73fb40061 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -354,14 +354,16 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
>                 ret = EFI_SUCCESS;
>
>         /*
> -        * Write non-volatile EFI variables to file
> +        * Write non-volatile EFI variables to file or SPI Flash
>          * TODO: check if a value change has occured to avoid superfluous writes
>          */
> -#if CONFIG_IS_ENABLED(EFI_VARIABLE_FILE_STORE)
>         if (attributes & EFI_VARIABLE_NON_VOLATILE) {
> +#if CONFIG_IS_ENABLED(EFI_VARIABLE_FILE_STORE)
>                 efi_var_to_file();
> -       }
> +#elif CONFIG_IS_ENABLED(EFI_VARIABLE_SF_STORE)
> +               efi_var_to_sf();
>  #endif
> +       }
>
>         return EFI_SUCCESS;
>  }
> @@ -471,9 +473,14 @@ efi_status_t efi_init_variables(void)
>
>  #if CONFIG_IS_ENABLED(EFI_VARIABLE_FILE_STORE)
>         ret = efi_var_from_file();
> +#elif CONFIG_IS_ENABLED(EFI_VARIABLE_SF_STORE)
> +       ret = efi_var_from_sf();
> +#else
> +       ret = EFI_SUCCESS;
> +#endif

Instead of those ifdefs can we do something different?
Define a structure with two function callbacks for read/write(). Add
the same function in both efi_var_file.c and efi_var_sp.c which fills
in those callbacks and have an efi_init_variable() call to that
function (obviously the SPI and file will be mutually exclusive)

Thanks
/Ilias

> +
>         if (ret != EFI_SUCCESS)
>                 return ret;
> -#endif
>
>         if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) {
>                 ret = efi_var_restore((struct efi_var_file *)
> --
> 2.40.1
>

Re: [PATCH v3 3/3] defconfig: rockpro64: Enable SF EFI var store

[Simon Glass]

Hi Shantur,

On Sun, 26 Nov 2023 at 15:09, Shantur Rathore <i@shantur.com> wrote:
>
> RockPro64 uses SPI Flash for storing env, also use it store
> EFI variables.
>
> Signed-off-by: Shantur Rathore <i@shantur.com>
> ---
>
>  configs/rockpro64-rk3399_defconfig | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
> index 4cd6b76665..f550f2ae43 100644
> --- a/configs/rockpro64-rk3399_defconfig
> +++ b/configs/rockpro64-rk3399_defconfig
> @@ -8,6 +8,8 @@ CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>  CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x300000
>  CONFIG_ENV_SIZE=0x8000
>  CONFIG_ENV_OFFSET=0x3F8000
> +CONFIG_EFI_VARIABLE_SF_STORE=y
> +CONFIG_EFI_VARIABLE_SF_OFFSET=0x7D0000

Can we use this offset in binman when creating the SPI-flash image?

>  CONFIG_DEFAULT_DEVICE_TREE="rk3399-rockpro64"
>  CONFIG_DM_RESET=y
>  CONFIG_ROCKCHIP_RK3399=y
> --
> 2.40.1
>

Regards,
Simon

Re: [PATCH v3 2/3] efi_vars: Implement SPI Flash store

[Shantur Rathore]

Hi Ilias,

On Thu, Nov 30, 2023 at 10:10 PM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Shantur
>
> I have a few remarks on the architecture.
> Up to now, we are supporting
> 1. Variables on a file
> 2. Variables on an RPMB
>
> The reason those two are in different files is that we generally
> expect to use different bootime services and few differences in
> efi_variables_boot_exit_notify() and efi_init_variables().
> Whatever is common, for example the runtime functions are common
> across those two since they both implement variable runtime service
> via the memory backend, goes into efi_var_common.c, the memory backend
> operations should go in efi_var_mem.c.
>
> Since the SPI and file storage are similar -- and probably any storage
> medium controlled by the non-secure world, I am fine treating treat
> efi_variable.c as the variable management for the non-secure world and
> see if that needs changing in the future.
>
> As Heinrich pointed out the functions you are moving are better off
> moved into efi_var_mem.c.
>
> [...]

Happy to move them efi_var_mem.c
I will do that as part of v4

>
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index adc5ac6a80..f73fb40061 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -354,14 +354,16 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
> >                 ret = EFI_SUCCESS;
> >
> >         /*
> > -        * Write non-volatile EFI variables to file
> > +        * Write non-volatile EFI variables to file or SPI Flash
> >          * TODO: check if a value change has occured to avoid superfluous writes
> >          */
> > -#if CONFIG_IS_ENABLED(EFI_VARIABLE_FILE_STORE)
> >         if (attributes & EFI_VARIABLE_NON_VOLATILE) {
> > +#if CONFIG_IS_ENABLED(EFI_VARIABLE_FILE_STORE)
> >                 efi_var_to_file();
> > -       }
> > +#elif CONFIG_IS_ENABLED(EFI_VARIABLE_SF_STORE)
> > +               efi_var_to_sf();
> >  #endif
> > +       }
> >
> >         return EFI_SUCCESS;
> >  }
> > @@ -471,9 +473,14 @@ efi_status_t efi_init_variables(void)
> >
> >  #if CONFIG_IS_ENABLED(EFI_VARIABLE_FILE_STORE)
> >         ret = efi_var_from_file();
> > +#elif CONFIG_IS_ENABLED(EFI_VARIABLE_SF_STORE)
> > +       ret = efi_var_from_sf();
> > +#else
> > +       ret = EFI_SUCCESS;
> > +#endif
>
> Instead of those ifdefs can we do something different?
> Define a structure with two function callbacks for read/write(). Add
> the same function in both efi_var_file.c and efi_var_sp.c which fills
> in those callbacks and have an efi_init_variable() call to that
> function (obviously the SPI and file will be mutually exclusive)
>

Sure, will add to v4 too.

Thanks,
Shantur

Re: [PATCH v3 3/3] defconfig: rockpro64: Enable SF EFI var store

[Shantur Rathore]

Hi Simon,

On Fri, Dec 1, 2023 at 6:44 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Shantur,
>
> On Sun, 26 Nov 2023 at 15:09, Shantur Rathore <i@shantur.com> wrote:
> >
> > RockPro64 uses SPI Flash for storing env, also use it store
> > EFI variables.
> >
> > Signed-off-by: Shantur Rathore <i@shantur.com>
> > ---
> >
> >  configs/rockpro64-rk3399_defconfig | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
> > index 4cd6b76665..f550f2ae43 100644
> > --- a/configs/rockpro64-rk3399_defconfig
> > +++ b/configs/rockpro64-rk3399_defconfig
> > @@ -8,6 +8,8 @@ CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
> >  CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x300000
> >  CONFIG_ENV_SIZE=0x8000
> >  CONFIG_ENV_OFFSET=0x3F8000
> > +CONFIG_EFI_VARIABLE_SF_STORE=y
> > +CONFIG_EFI_VARIABLE_SF_OFFSET=0x7D0000
>
> Can we use this offset in binman when creating the SPI-flash image?

Unless I missed something, binman is used when something is bundled
with a u-boot image.
This is similar to ENV_OFFSET letting u-boot know where it can write
the EFI variables.
I didn't see ENV_OFFSET referred in rk3399-u-boot.dtsi or
rk3399-rockpro64-u-boot.dtsi

Please correct me if I am wrong.

Kind regards,
Shantur

Re: [PATCH v3 3/3] defconfig: rockpro64: Enable SF EFI var store

[Simon Glass]

Hi Shantur,

On Sat, 2 Dec 2023 at 16:12, Shantur Rathore <i@shantur.com> wrote:
>
> Hi Simon,
>
> On Fri, Dec 1, 2023 at 6:44 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Shantur,
> >
> > On Sun, 26 Nov 2023 at 15:09, Shantur Rathore <i@shantur.com> wrote:
> > >
> > > RockPro64 uses SPI Flash for storing env, also use it store
> > > EFI variables.
> > >
> > > Signed-off-by: Shantur Rathore <i@shantur.com>
> > > ---
> > >
> > >  configs/rockpro64-rk3399_defconfig | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
> > > index 4cd6b76665..f550f2ae43 100644
> > > --- a/configs/rockpro64-rk3399_defconfig
> > > +++ b/configs/rockpro64-rk3399_defconfig
> > > @@ -8,6 +8,8 @@ CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
> > >  CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x300000
> > >  CONFIG_ENV_SIZE=0x8000
> > >  CONFIG_ENV_OFFSET=0x3F8000
> > > +CONFIG_EFI_VARIABLE_SF_STORE=y
> > > +CONFIG_EFI_VARIABLE_SF_OFFSET=0x7D0000
> >
> > Can we use this offset in binman when creating the SPI-flash image?
>
> Unless I missed something, binman is used when something is bundled
> with a u-boot image.
> This is similar to ENV_OFFSET letting u-boot know where it can write
> the EFI variables.
> I didn't see ENV_OFFSET referred in rk3399-u-boot.dtsi or
> rk3399-rockpro64-u-boot.dtsi
>
> Please correct me if I am wrong.

Well, this is what I see in arch/arm/dts/rk3399-u-boot.dtsi :

#if defined(CONFIG_ROCKCHIP_SPI_IMAGE) && defined(CONFIG_HAS_ROM)
&binman {
   multiple-images;
   rom {
      filename = "u-boot.rom";
      size = <0x400000>;
      pad-byte = <0xff>;

      mkimage {
         args = "-n rk3399 -T rkspi";
         u-boot-spl {
         };
      };
      u-boot-img {
         offset = <0x40000>;
      };
      u-boot {
         offset = <0x300000>;
      };
      fdtmap {
      };
   };
};
#endif /* CONFIG_ROCKCHIP_SPI_IMAGE && CONFIG_HAS_ROM */

So it looks like 0x3f8000 fits in there somewhere.

In fact, I wonder if we should add something like:

u-boot-env {
   offset = <CONFIG_ENV_OFFSET>;
};

so the environment gets filled into the SPI flash when we write the image?

I suppose we could also add the EFI variable store, although that
would require adding a new entry type to binman.

Regards,
Simon

Re: [PATCH v3 3/3] defconfig: rockpro64: Enable SF EFI var store

[Shantur Rathore]

Hi Simon,

On Sun, Dec 3, 2023 at 5:44 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Shantur,
>
> On Sat, 2 Dec 2023 at 16:12, Shantur Rathore <i@shantur.com> wrote:
> >
> > Hi Simon,
> >
> > On Fri, Dec 1, 2023 at 6:44 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Shantur,
> > >
> > > On Sun, 26 Nov 2023 at 15:09, Shantur Rathore <i@shantur.com> wrote:
> > > >
> > > > RockPro64 uses SPI Flash for storing env, also use it store
> > > > EFI variables.
> > > >
> > > > Signed-off-by: Shantur Rathore <i@shantur.com>
> > > > ---
> > > >
> > > >  configs/rockpro64-rk3399_defconfig | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
> > > > index 4cd6b76665..f550f2ae43 100644
> > > > --- a/configs/rockpro64-rk3399_defconfig
> > > > +++ b/configs/rockpro64-rk3399_defconfig
> > > > @@ -8,6 +8,8 @@ CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
> > > >  CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x300000
> > > >  CONFIG_ENV_SIZE=0x8000
> > > >  CONFIG_ENV_OFFSET=0x3F8000
> > > > +CONFIG_EFI_VARIABLE_SF_STORE=y
> > > > +CONFIG_EFI_VARIABLE_SF_OFFSET=0x7D0000
> > >
> > > Can we use this offset in binman when creating the SPI-flash image?
> >
> > Unless I missed something, binman is used when something is bundled
> > with a u-boot image.
> > This is similar to ENV_OFFSET letting u-boot know where it can write
> > the EFI variables.
> > I didn't see ENV_OFFSET referred in rk3399-u-boot.dtsi or
> > rk3399-rockpro64-u-boot.dtsi
> >
> > Please correct me if I am wrong.
>
> Well, this is what I see in arch/arm/dts/rk3399-u-boot.dtsi :
>
> #if defined(CONFIG_ROCKCHIP_SPI_IMAGE) && defined(CONFIG_HAS_ROM)
> &binman {
>    multiple-images;
>    rom {
>       filename = "u-boot.rom";
>       size = <0x400000>;
>       pad-byte = <0xff>;
>
>       mkimage {
>          args = "-n rk3399 -T rkspi";
>          u-boot-spl {
>          };
>       };
>       u-boot-img {
>          offset = <0x40000>;
>       };
>       u-boot {
>          offset = <0x300000>;
>       };
>       fdtmap {
>       };
>    };
> };
> #endif /* CONFIG_ROCKCHIP_SPI_IMAGE && CONFIG_HAS_ROM */
>
> So it looks like 0x3f8000 fits in there somewhere.
>

Thanks for clarifying. Now I see how ENV space is declared as used by u-boot.


> In fact, I wonder if we should add something like:
>
> u-boot-env {
>    offset = <CONFIG_ENV_OFFSET>;
> };
>
> so the environment gets filled into the SPI flash when we write the image?
>

Do we want to reset the environment to default on every u-boot update?
I believe this would overwrite the environment space isn't it.

> I suppose we could also add the EFI variable store, although that
> would require adding a new entry type to binman.
>

I am not sure about this but surely EFI vars can be moved to a location
just after the u-boot environment and increase the u-boot image size.

Would you still need an entry in dts?

Re: [PATCH v3 3/3] defconfig: rockpro64: Enable SF EFI var store

[Dragan Simic]

On 2023-12-03 22:49, Shantur Rathore wrote:
> On Sun, Dec 3, 2023 at 5:44 PM Simon Glass <sjg@chromium.org> wrote:
>> In fact, I wonder if we should add something like:
>> 
>> u-boot-env {
>>    offset = <CONFIG_ENV_OFFSET>;
>> };
>> 
>> so the environment gets filled into the SPI flash when we write the 
>> image?
> 
> Do we want to reset the environment to default on every u-boot update?
> I believe this would overwrite the environment space isn't it.

Obviously, resetting the U-Boot environment on updates would be a really 
bad idea, which may even render some boards unbootable.

>> I suppose we could also add the EFI variable store, although that
>> would require adding a new entry type to binman.
> 
> I am not sure about this but surely EFI vars can be moved to a location
> just after the u-boot environment and increase the u-boot image size.
> 
> Would you still need an entry in dts?

Re: [PATCH v3 3/3] defconfig: rockpro64: Enable SF EFI var store

[Simon Glass]

Hi Shantur,

On Sun, 3 Dec 2023 at 14:49, Shantur Rathore <i@shantur.com> wrote:
>
> Hi Simon,
>
> On Sun, Dec 3, 2023 at 5:44 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Shantur,
> >
> > On Sat, 2 Dec 2023 at 16:12, Shantur Rathore <i@shantur.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Fri, Dec 1, 2023 at 6:44 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Shantur,
> > > >
> > > > On Sun, 26 Nov 2023 at 15:09, Shantur Rathore <i@shantur.com> wrote:
> > > > >
> > > > > RockPro64 uses SPI Flash for storing env, also use it store
> > > > > EFI variables.
> > > > >
> > > > > Signed-off-by: Shantur Rathore <i@shantur.com>
> > > > > ---
> > > > >
> > > > >  configs/rockpro64-rk3399_defconfig | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
> > > > > index 4cd6b76665..f550f2ae43 100644
> > > > > --- a/configs/rockpro64-rk3399_defconfig
> > > > > +++ b/configs/rockpro64-rk3399_defconfig
> > > > > @@ -8,6 +8,8 @@ CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
> > > > >  CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x300000
> > > > >  CONFIG_ENV_SIZE=0x8000
> > > > >  CONFIG_ENV_OFFSET=0x3F8000
> > > > > +CONFIG_EFI_VARIABLE_SF_STORE=y
> > > > > +CONFIG_EFI_VARIABLE_SF_OFFSET=0x7D0000
> > > >
> > > > Can we use this offset in binman when creating the SPI-flash image?
> > >
> > > Unless I missed something, binman is used when something is bundled
> > > with a u-boot image.
> > > This is similar to ENV_OFFSET letting u-boot know where it can write
> > > the EFI variables.
> > > I didn't see ENV_OFFSET referred in rk3399-u-boot.dtsi or
> > > rk3399-rockpro64-u-boot.dtsi
> > >
> > > Please correct me if I am wrong.
> >
> > Well, this is what I see in arch/arm/dts/rk3399-u-boot.dtsi :
> >
> > #if defined(CONFIG_ROCKCHIP_SPI_IMAGE) && defined(CONFIG_HAS_ROM)
> > &binman {
> >    multiple-images;
> >    rom {
> >       filename = "u-boot.rom";
> >       size = <0x400000>;
> >       pad-byte = <0xff>;
> >
> >       mkimage {
> >          args = "-n rk3399 -T rkspi";
> >          u-boot-spl {
> >          };
> >       };
> >       u-boot-img {
> >          offset = <0x40000>;
> >       };
> >       u-boot {
> >          offset = <0x300000>;
> >       };
> >       fdtmap {
> >       };
> >    };
> > };
> > #endif /* CONFIG_ROCKCHIP_SPI_IMAGE && CONFIG_HAS_ROM */
> >
> > So it looks like 0x3f8000 fits in there somewhere.
> >
>
> Thanks for clarifying. Now I see how ENV space is declared as used by u-boot.
>
>
> > In fact, I wonder if we should add something like:
> >
> > u-boot-env {
> >    offset = <CONFIG_ENV_OFFSET>;
> > };
> >
> > so the environment gets filled into the SPI flash when we write the image?
> >
>
> Do we want to reset the environment to default on every u-boot update?
> I believe this would overwrite the environment space isn't it.

It depends on how the update is done. If the image written is smaller
than the full size of the SPI flash and the environment is at the end
of it, then it would be preserved.

But in fact the u-boot.rom file is 0x400000 bytes (4MB) so I don't see
how that would work. So we might as well add the environment in?

>
> > I suppose we could also add the EFI variable store, although that
> > would require adding a new entry type to binman.
> >
>
> I am not sure about this but surely EFI vars can be moved to a location
> just after the u-boot environment and increase the u-boot image size.

Yes.

>
> Would you still need an entry in dts?

My point was really just that the binman description should contain
the full layout of the flash. Otherwise someone might adjust something
such that the environment overlays U-Boot code, etc.

Regards,
Simon