[PATCH v1 0/3] efi_vars: SPI Flash store

[PATCH v1 0/3] efi_vars: SPI Flash store

[Shantur Rathore]

With this patch series we implement SPI Flash storage for EFI
variables.


Shantur Rathore (3):
  efi: filestore: don't compile when config disabled
  efi_vars: Implement SPI Flash store
  defconfig: rockpro64: Enable SF EFI var store

 configs/rockpro64-rk3399_defconfig |  2 +
 include/efi_variable.h             | 46 ++++++++++++---
 lib/efi_loader/Kconfig             | 18 ++++++
 lib/efi_loader/Makefile            |  1 +
 lib/efi_loader/efi_var_file.c      | 13 +++--
 lib/efi_loader/efi_var_sf.c        | 91 ++++++++++++++++++++++++++++++
 lib/efi_loader/efi_variable.c      | 14 ++++-
 7 files changed, 169 insertions(+), 16 deletions(-)
 create mode 100644 lib/efi_loader/efi_var_sf.c

-- 
2.40.1

[PATCH v1 1/3] efi: filestore: don't compile when config disabled

[Shantur Rathore]

Compile out filestore functions when config isn't enabled.

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

 include/efi_variable.h        | 21 ++++++++++++---------
 lib/efi_loader/efi_var_file.c | 13 +++++++------
 lib/efi_loader/efi_variable.c | 10 +++++++++-
 3 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/include/efi_variable.h b/include/efi_variable.h
index 805e6c5f1e..ca7e19d514 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -136,15 +136,6 @@ struct efi_var_file {
 	struct efi_var_entry var[];
 };
 
-/**
- * efi_var_to_file() - save non-volatile variables as file
- *
- * File ubootefi.var is created on the EFI system partion.
- *
- * Return:	status code
- */
-efi_status_t efi_var_to_file(void);
-
 /**
  * efi_var_collect() - collect variables in buffer
  *
@@ -172,6 +163,16 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
  */
 efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
 
+#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
+/**
+ * efi_var_to_file() - save non-volatile variables as file
+ *
+ * File ubootefi.var is created on the EFI system parition.
+ *
+ * Return:	status code
+ */
+efi_status_t efi_var_to_file(void);
+
 /**
  * efi_var_from_file() - read variables from file
  *
@@ -185,6 +186,8 @@ efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
  */
 efi_status_t efi_var_from_file(void);
 
+#endif // CONFIG_EFI_VARIABLE_FILE_STORE
+
 /**
  * efi_var_mem_init() - set-up variable list
  *
diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
index 62e071bd83..7ceb7e3cf7 100644
--- a/lib/efi_loader/efi_var_file.c
+++ b/lib/efi_loader/efi_var_file.c
@@ -117,6 +117,8 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
 	return EFI_SUCCESS;
 }
 
+#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
+
 /**
  * efi_var_to_file() - save non-volatile variables as file
  *
@@ -126,7 +128,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,11 +151,10 @@ error:
 		log_err("Failed to persist EFI variables\n");
 	free(buf);
 	return ret;
-#else
-	return EFI_SUCCESS;
-#endif
 }
 
+#endif // CONFIG_EFI_VARIABLE_FILE_STORE
+
 efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
 {
 	struct efi_var_entry *var, *last_var;
@@ -198,6 +198,7 @@ efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
 	return EFI_SUCCESS;
 }
 
+#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
 /**
  * efi_var_from_file() - read variables from file
  *
@@ -211,7 +212,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;
@@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void)
 		log_err("Invalid EFI variables file\n");
 error:
 	free(buf);
-#endif
 	return EFI_SUCCESS;
 }
+
+#endif // CONFIG_EFI_VARIABLE_FILE_STORE
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index be95ed44e6..7fa444451d 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 (attributes & EFI_VARIABLE_NON_VOLATILE) {
+#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
 		efi_var_to_file();
+#endif
+	}
 
 	return EFI_SUCCESS;
 }
@@ -466,7 +469,12 @@ efi_status_t efi_init_variables(void)
 	if (ret != EFI_SUCCESS)
 		return ret;
 
+#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
 	ret = efi_var_from_file();
+#else
+	ret = EFI_SUCCESS;
+#endif
+
 	if (ret != EFI_SUCCESS)
 		return ret;
 	if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) {
-- 
2.40.1

[PATCH v1 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>
---

 include/efi_variable.h        | 25 ++++++++++
 lib/efi_loader/Kconfig        | 18 +++++++
 lib/efi_loader/Makefile       |  1 +
 lib/efi_loader/efi_var_sf.c   | 91 +++++++++++++++++++++++++++++++++++
 lib/efi_loader/efi_variable.c |  4 ++
 5 files changed, 139 insertions(+)
 create mode 100644 lib/efi_loader/efi_var_sf.c

diff --git a/include/efi_variable.h b/include/efi_variable.h
index ca7e19d514..766dd109f5 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -188,6 +188,31 @@ efi_status_t efi_var_from_file(void);
 
 #endif // CONFIG_EFI_VARIABLE_FILE_STORE
 
+#ifdef CONFIG_EFI_VARIABLE_SF_STORE
+
+/**
+ * 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);
+
+#endif // CONFIG_EFI_VARIABLE_SF_STORE
+
 /**
  * efi_var_mem_init() - set-up variable list
  *
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 4ccd26f94a..1fcc6fabb4 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,13 @@ config EFI_VARIABLE_NO_STORE
 
 endchoice
 
+config EFI_VARIABLE_SF_OFFSET
+	hex "EFI variables in SPI flash offset"
+	depends on EFI_VARIABLE_SF_STORE
+	default 0x7D0000
+	help
+	  Offset from the start of the SPI Flash where EFI variables will be stored.
+
 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 8d31fc61c6..b9b715b1ff 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-y += 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..e604a2225c
--- /dev/null
+++ b/lib/efi_loader/efi_var_sf.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * File interface for UEFI variables
+ *
+ * Copyright (c) 2023, Shantur Rtahore
+ */
+
+#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 SF size");
+		ret = EFI_BAD_BUFFER_SIZE;
+		goto error;
+	}
+
+	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, 0, &sfdev);
+	if (ret)
+		goto error;
+
+	ret = spi_flash_erase_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, EFI_VAR_BUF_SIZE);
+	debug("%s - Erased SPI Flash offset %lx\n", __func__, CONFIG_EFI_VARIABLE_SF_OFFSET);
+	if (ret)
+		goto error;
+
+	ret = spi_flash_write_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, len, buf);
+	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("Out of memory\n");
+		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);
+
+	debug("%s - read buffer buf->length: %x\n", __func__, buf->length);
+
+	if (ret || buf->length < sizeof(struct efi_var_file)) {
+		log_err("Failed to load EFI variables\n");
+		goto error;
+	}
+
+	ret = efi_var_restore(buf, false);
+	if (ret != EFI_SUCCESS)
+		log_err("Invalid EFI variables file\n");
+
+	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 7fa444451d..0f85962f05 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -360,6 +360,8 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
 	if (attributes & EFI_VARIABLE_NON_VOLATILE) {
 #ifdef CONFIG_EFI_VARIABLE_FILE_STORE
 		efi_var_to_file();
+#elif CONFIG_EFI_VARIABLE_SF_STORE
+		efi_var_to_sf();
 #endif
 	}
 
@@ -471,6 +473,8 @@ efi_status_t efi_init_variables(void)
 
 #ifdef CONFIG_EFI_VARIABLE_FILE_STORE
 	ret = efi_var_from_file();
+#elif CONFIG_EFI_VARIABLE_SF_STORE
+	ret = efi_var_from_sf();
 #else
 	ret = EFI_SUCCESS;
 #endif
-- 
2.40.1

[PATCH v1 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 affb6137e0..b9dededc9a 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 v1 1/3] efi: filestore: don't compile when config disabled

[AKASHI Takahiro]

On Tue, Nov 21, 2023 at 11:57:11PM +0000, Shantur Rathore wrote:
> Compile out filestore functions when config isn't enabled.
> 
> Signed-off-by: Shantur Rathore <i@shantur.com>
> ---
> 
>  include/efi_variable.h        | 21 ++++++++++++---------
>  lib/efi_loader/efi_var_file.c | 13 +++++++------
>  lib/efi_loader/efi_variable.c | 10 +++++++++-
>  3 files changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/include/efi_variable.h b/include/efi_variable.h
> index 805e6c5f1e..ca7e19d514 100644
> --- a/include/efi_variable.h
> +++ b/include/efi_variable.h
> @@ -136,15 +136,6 @@ struct efi_var_file {
>  	struct efi_var_entry var[];
>  };
>  
> -/**
> - * efi_var_to_file() - save non-volatile variables as file
> - *
> - * File ubootefi.var is created on the EFI system partion.
> - *
> - * Return:	status code
> - */
> -efi_status_t efi_var_to_file(void);
> -
>  /**
>   * efi_var_collect() - collect variables in buffer
>   *
> @@ -172,6 +163,16 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
>   */
>  efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
>  
> +#ifdef CONFIG_EFI_VARIABLE_FILE_STORE

I don't think we need this guard because any function declaration
in a header is harmless even if that function implementation is opted out.

> +/**
> + * efi_var_to_file() - save non-volatile variables as file
> + *
> + * File ubootefi.var is created on the EFI system parition.
> + *
> + * Return:	status code
> + */
> +efi_status_t efi_var_to_file(void);
> +
>  /**
>   * efi_var_from_file() - read variables from file
>   *
> @@ -185,6 +186,8 @@ efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
>   */
>  efi_status_t efi_var_from_file(void);
>  
> +#endif // CONFIG_EFI_VARIABLE_FILE_STORE
> +
>  /**
>   * efi_var_mem_init() - set-up variable list
>   *
> diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
> index 62e071bd83..7ceb7e3cf7 100644
> --- a/lib/efi_loader/efi_var_file.c
> +++ b/lib/efi_loader/efi_var_file.c
> @@ -117,6 +117,8 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
>  	return EFI_SUCCESS;
>  }
>  
> +#ifdef CONFIG_EFI_VARIABLE_FILE_STORE

Can you refactor the code further to purge all "#ifdef" stuffs from this file?

I mean:
1) move efi_var_collect/restor() to a always-compiled file, say,
   efi_var_common.c/efi_variable.c
2) modify call sites of the other functions, for example, as follows:
     if (CONFIG_IS_ENABLED(EFI_VARIABLE_STORE))
         efi_var_to_file();
   # I'm not sure why the return code is ignored everywhere.
3) modify Makefile to compile efi_var_file.c only if EFI_VARIABLE_FILE_STORE
   is enabled
4) remove "#ifdef CONFIG_EFI_VARIABLE_FILE_STORE" from efi_var_file.c

-Takahiro Akashi

> +
>  /**
>   * efi_var_to_file() - save non-volatile variables as file
>   *
> @@ -126,7 +128,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,11 +151,10 @@ error:
>  		log_err("Failed to persist EFI variables\n");
>  	free(buf);
>  	return ret;
> -#else
> -	return EFI_SUCCESS;
> -#endif
>  }
>  
> +#endif // CONFIG_EFI_VARIABLE_FILE_STORE
> +
>  efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
>  {
>  	struct efi_var_entry *var, *last_var;
> @@ -198,6 +198,7 @@ efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
>  	return EFI_SUCCESS;
>  }
>  
> +#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
>  /**
>   * efi_var_from_file() - read variables from file
>   *
> @@ -211,7 +212,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;
> @@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void)
>  		log_err("Invalid EFI variables file\n");
>  error:
>  	free(buf);
> -#endif
>  	return EFI_SUCCESS;
>  }
> +
> +#endif // CONFIG_EFI_VARIABLE_FILE_STORE
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index be95ed44e6..7fa444451d 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 (attributes & EFI_VARIABLE_NON_VOLATILE) {
> +#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
>  		efi_var_to_file();
> +#endif
> +	}
>  
>  	return EFI_SUCCESS;
>  }
> @@ -466,7 +469,12 @@ efi_status_t efi_init_variables(void)
>  	if (ret != EFI_SUCCESS)
>  		return ret;
>  
> +#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
>  	ret = efi_var_from_file();
> +#else
> +	ret = EFI_SUCCESS;
> +#endif
> +
>  	if (ret != EFI_SUCCESS)
>  		return ret;
>  	if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) {
> -- 
> 2.40.1
> 

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

[AKASHI Takahiro]

On Tue, Nov 21, 2023 at 11:57:12PM +0000, Shantur Rathore wrote:
> 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>
> ---
> 
>  include/efi_variable.h        | 25 ++++++++++
>  lib/efi_loader/Kconfig        | 18 +++++++
>  lib/efi_loader/Makefile       |  1 +
>  lib/efi_loader/efi_var_sf.c   | 91 +++++++++++++++++++++++++++++++++++
>  lib/efi_loader/efi_variable.c |  4 ++
>  5 files changed, 139 insertions(+)
>  create mode 100644 lib/efi_loader/efi_var_sf.c
> 
> diff --git a/include/efi_variable.h b/include/efi_variable.h
> index ca7e19d514..766dd109f5 100644
> --- a/include/efi_variable.h
> +++ b/include/efi_variable.h
> @@ -188,6 +188,31 @@ efi_status_t efi_var_from_file(void);
>  
>  #endif // CONFIG_EFI_VARIABLE_FILE_STORE
>  
> +#ifdef CONFIG_EFI_VARIABLE_SF_STORE

Not needed as I said.

> +
> +/**
> + * 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);
> +
> +#endif // CONFIG_EFI_VARIABLE_SF_STORE
> +
>  /**
>   * efi_var_mem_init() - set-up variable list
>   *
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 4ccd26f94a..1fcc6fabb4 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,13 @@ config EFI_VARIABLE_NO_STORE
>  
>  endchoice
>  
> +config EFI_VARIABLE_SF_OFFSET
> +	hex "EFI variables in SPI flash offset"
> +	depends on EFI_VARIABLE_SF_STORE
> +	default 0x7D0000

The default value is definitely board-specific.
Please add "if TARGET_ROCKPRO64_RK3399(?)".

> +	help
> +	  Offset from the start of the SPI Flash where EFI variables will be stored.
> +
>  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 8d31fc61c6..b9b715b1ff 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-y += 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..e604a2225c
> --- /dev/null
> +++ b/lib/efi_loader/efi_var_sf.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * File interface for UEFI variables

For Spi Flash?

> + *
> + * Copyright (c) 2023, Shantur Rtahore
> + */
> +
> +#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 SF size");
> +		ret = EFI_BAD_BUFFER_SIZE;
> +		goto error;
> +	}
> +
> +	debug("%s - Got buffer to write buf->len : %d\n", __func__, buf->length);

log_debug()?

> +
> +	if (ret != EFI_SUCCESS)
> +		goto error;
> +
> +	ret = uclass_get_device(UCLASS_SPI_FLASH, 0, &sfdev);
> +	if (ret)
> +		goto error;
> +
> +	ret = spi_flash_erase_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, EFI_VAR_BUF_SIZE);
> +	debug("%s - Erased SPI Flash offset %lx\n", __func__, CONFIG_EFI_VARIABLE_SF_OFFSET);
> +	if (ret)
> +		goto error;
> +
> +	ret = spi_flash_write_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, len, buf);
> +	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("Out of memory\n");

I hope that we see a more specific diag message here.

> +		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);
> +
> +	debug("%s - read buffer buf->length: %x\n", __func__, buf->length);
> +
> +	if (ret || buf->length < sizeof(struct efi_var_file)) {
> +		log_err("Failed to load EFI variables\n");

I hope that we see a more specific diag message here.

> +		goto error;
> +	}
> +
> +	ret = efi_var_restore(buf, false);
> +	if (ret != EFI_SUCCESS)
> +		log_err("Invalid EFI variables file\n");
> +
> +	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 7fa444451d..0f85962f05 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -360,6 +360,8 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
>  	if (attributes & EFI_VARIABLE_NON_VOLATILE) {
>  #ifdef CONFIG_EFI_VARIABLE_FILE_STORE
>  		efi_var_to_file();
> +#elif CONFIG_EFI_VARIABLE_SF_STORE
> +		efi_var_to_sf();

if (CONFIG_IS_ENABLED(EFI_VARIABLE_SF_STORE))
    efi_var_to_sf();

-Takahiro Akashi

>  #endif
>  	}
>  
> @@ -471,6 +473,8 @@ efi_status_t efi_init_variables(void)
>  
>  #ifdef CONFIG_EFI_VARIABLE_FILE_STORE
>  	ret = efi_var_from_file();
> +#elif CONFIG_EFI_VARIABLE_SF_STORE
> +	ret = efi_var_from_sf();
>  #else
>  	ret = EFI_SUCCESS;
>  #endif
> -- 
> 2.40.1
> 

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

[Ilias Apalodimas]

Hi Shahtur

On Wed, 22 Nov 2023 at 01:58, Shantur Rathore <i@shantur.com> wrote:
>
> 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>
> ---
>
>  include/efi_variable.h        | 25 ++++++++++
>  lib/efi_loader/Kconfig        | 18 +++++++
>  lib/efi_loader/Makefile       |  1 +
>  lib/efi_loader/efi_var_sf.c   | 91 +++++++++++++++++++++++++++++++++++
>  lib/efi_loader/efi_variable.c |  4 ++
>  5 files changed, 139 insertions(+)
>  create mode 100644 lib/efi_loader/efi_var_sf.c
>
> diff --git a/include/efi_variable.h b/include/efi_variable.h
> index ca7e19d514..766dd109f5 100644
> --- a/include/efi_variable.h
> +++ b/include/efi_variable.h
> @@ -188,6 +188,31 @@ efi_status_t efi_var_from_file(void);
>
>  #endif // CONFIG_EFI_VARIABLE_FILE_STORE
>
> +#ifdef CONFIG_EFI_VARIABLE_SF_STORE
> +
> +/**
> + * 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);
> +
> +#endif // CONFIG_EFI_VARIABLE_SF_STORE
> +
>  /**
>   * efi_var_mem_init() - set-up variable list
>   *
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 4ccd26f94a..1fcc6fabb4 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,13 @@ config EFI_VARIABLE_NO_STORE
>
>  endchoice
>
> +config EFI_VARIABLE_SF_OFFSET
> +       hex "EFI variables in SPI flash offset"
> +       depends on EFI_VARIABLE_SF_STORE
> +       default 0x7D0000
> +       help
> +         Offset from the start of the SPI Flash where EFI variables will be stored.
> +
>  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 8d31fc61c6..b9b715b1ff 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-y += 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..e604a2225c
> --- /dev/null
> +++ b/lib/efi_loader/efi_var_sf.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * File interface for UEFI variables
> + *
> + * Copyright (c) 2023, Shantur Rtahore

Name is misspelled

> + */
> +
> +#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 SF size");
> +               ret = EFI_BAD_BUFFER_SIZE;

The UEFI spec doesn't define this return code for SetVariable.
Instead, EFI_OUT_OF_RESOURCES is used when the available storage isn't
enough to hold the variable and its data.

> +               goto error;
> +       }
> +
> +       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, 0, &sfdev);

This is a bit weird. Do we always want to use the first available SPI
on the system to store the variables?
I don't remember if the driver model has a mechanism to define one of
the SPI flashes, but if it doesn't perhaps we should use a Kconfig for
it?

> +       if (ret)
> +               goto error;
> +
> +       ret = spi_flash_erase_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, EFI_VAR_BUF_SIZE);
> +       debug("%s - Erased SPI Flash offset %lx\n", __func__, CONFIG_EFI_VARIABLE_SF_OFFSET);
> +       if (ret)
> +               goto error;
> +
> +       ret = spi_flash_write_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, len, buf);
> +       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("Out of memory\n");
> +               return EFI_OUT_OF_RESOURCES;
> +       }

You don't want normal memory for the variables.  Instead, you want EFI
memory which is marked for EFI runtime services data. U-Boot already
has code to support Get/GetNextvariableRT, you just need to use
efi_var_mem_init().
If you need hints have a look at lib/efi_loader/efi_variable_tee.c
which implements variables stored in an RPMB, but still uses the
memory backend for runtime services.

> +
> +       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);
> +
> +       debug("%s - read buffer buf->length: %x\n", __func__, buf->length);
> +
> +       if (ret || buf->length < sizeof(struct efi_var_file)) {
> +               log_err("Failed to load EFI variables\n");
> +               goto error;
> +       }
> +
> +       ret = efi_var_restore(buf, false);
> +       if (ret != EFI_SUCCESS)
> +               log_err("Invalid EFI variables file\n");
> +
> +       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 7fa444451d..0f85962f05 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -360,6 +360,8 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
>         if (attributes & EFI_VARIABLE_NON_VOLATILE) {
>  #ifdef CONFIG_EFI_VARIABLE_FILE_STORE
>                 efi_var_to_file();
> +#elif CONFIG_EFI_VARIABLE_SF_STORE
> +               efi_var_to_sf();
>  #endif
>         }
>
> @@ -471,6 +473,8 @@ efi_status_t efi_init_variables(void)
>
>  #ifdef CONFIG_EFI_VARIABLE_FILE_STORE
>         ret = efi_var_from_file();
> +#elif CONFIG_EFI_VARIABLE_SF_STORE
> +       ret = efi_var_from_sf();
>  #else
>         ret = EFI_SUCCESS;
>  #endif
> --
> 2.40.1
>

Thanks
/Ilias

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

[Shantur Rathore]

Hi Ilias and Akashi,

On Wed, Nov 22, 2023 at 6:58 AM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Shahtur
>
> On Wed, 22 Nov 2023 at 01:58, Shantur Rathore <i@shantur.com> wrote:
> >
> > 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>
> > ---
> >
> >  include/efi_variable.h        | 25 ++++++++++
> >  lib/efi_loader/Kconfig        | 18 +++++++
> >  lib/efi_loader/Makefile       |  1 +
> >  lib/efi_loader/efi_var_sf.c   | 91 +++++++++++++++++++++++++++++++++++
> >  lib/efi_loader/efi_variable.c |  4 ++
> >  5 files changed, 139 insertions(+)
> >  create mode 100644 lib/efi_loader/efi_var_sf.c
> >
> > diff --git a/include/efi_variable.h b/include/efi_variable.h
> > index ca7e19d514..766dd109f5 100644
> > --- a/include/efi_variable.h
> > +++ b/include/efi_variable.h
> > @@ -188,6 +188,31 @@ efi_status_t efi_var_from_file(void);
> >
> >  #endif // CONFIG_EFI_VARIABLE_FILE_STORE
> >
> > +#ifdef CONFIG_EFI_VARIABLE_SF_STORE
> > +
> > +/**
> > + * 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);
> > +
> > +#endif // CONFIG_EFI_VARIABLE_SF_STORE
> > +
> >  /**
> >   * efi_var_mem_init() - set-up variable list
> >   *
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 4ccd26f94a..1fcc6fabb4 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,13 @@ config EFI_VARIABLE_NO_STORE
> >
> >  endchoice
> >
> > +config EFI_VARIABLE_SF_OFFSET
> > +       hex "EFI variables in SPI flash offset"
> > +       depends on EFI_VARIABLE_SF_STORE
> > +       default 0x7D0000
> > +       help
> > +         Offset from the start of the SPI Flash where EFI variables will be stored.
> > +
> >  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 8d31fc61c6..b9b715b1ff 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-y += 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..e604a2225c
> > --- /dev/null
> > +++ b/lib/efi_loader/efi_var_sf.c
> > @@ -0,0 +1,91 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * File interface for UEFI variables
> > + *
> > + * Copyright (c) 2023, Shantur Rtahore
>
> Name is misspelled
>

Fixed, thanks. I shouldn't be sending patches 2 minutes before midnight

> > + */
> > +
> > +#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 SF size");
> > +               ret = EFI_BAD_BUFFER_SIZE;
>
> The UEFI spec doesn't define this return code for SetVariable.
> Instead, EFI_OUT_OF_RESOURCES is used when the available storage isn't
> enough to hold the variable and its data.
>
> > +               goto error;
> > +       }
> > +
> > +       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, 0, &sfdev);
>
> This is a bit weird. Do we always want to use the first available SPI
> on the system to store the variables?
> I don't remember if the driver model has a mechanism to define one of
> the SPI flashes, but if it doesn't perhaps we should use a Kconfig for
> it?

Taken care of in V2. Thanks.

>
> > +       if (ret)
> > +               goto error;
> > +
> > +       ret = spi_flash_erase_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, EFI_VAR_BUF_SIZE);
> > +       debug("%s - Erased SPI Flash offset %lx\n", __func__, CONFIG_EFI_VARIABLE_SF_OFFSET);
> > +       if (ret)
> > +               goto error;
> > +
> > +       ret = spi_flash_write_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, len, buf);
> > +       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("Out of memory\n");
> > +               return EFI_OUT_OF_RESOURCES;
> > +       }
>
> You don't want normal memory for the variables.  Instead, you want EFI
> memory which is marked for EFI runtime services data. U-Boot already
> has code to support Get/GetNextvariableRT, you just need to use
> efi_var_mem_init().
> If you need hints have a look at lib/efi_loader/efi_variable_tee.c
> which implements variables stored in an RPMB, but still uses the
> memory backend for runtime services.
>

I believe EFI Runtime stuff is handled by efi_variable.c functions.
This code is run while we are still in U-boot to initialise the memory backed.
As of now, we don't have the SetVariableRT so I guess this won't be a problem.
I took the code from efi_var_file.c and just replaced file storage
with SPI Flash.

I will have to look deeper once I get to implement SetVariableRT which will
be a separate patch series once I get this in.

I have submitted V2 of the patch, can you kindly review them.

Kind regards,
Shantur

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

[Ilias Apalodimas]

Hi Shantur

Please don't send a v2 unless the v1 discussion has settled. It just
makes life harder. I'll ignore v2 for now and respond here.

[...]

>
> >
> > > +       if (ret)
> > > +               goto error;
> > > +
> > > +       ret = spi_flash_erase_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, EFI_VAR_BUF_SIZE);
> > > +       debug("%s - Erased SPI Flash offset %lx\n", __func__, CONFIG_EFI_VARIABLE_SF_OFFSET);
> > > +       if (ret)
> > > +               goto error;
> > > +
> > > +       ret = spi_flash_write_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, len, buf);
> > > +       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("Out of memory\n");
> > > +               return EFI_OUT_OF_RESOURCES;
> > > +       }
> >
> > You don't want normal memory for the variables.  Instead, you want EFI
> > memory which is marked for EFI runtime services data. U-Boot already
> > has code to support Get/GetNextvariableRT, you just need to use
> > efi_var_mem_init().
> > If you need hints have a look at lib/efi_loader/efi_variable_tee.c
> > which implements variables stored in an RPMB, but still uses the
> > memory backend for runtime services.
> >
>
> I believe EFI Runtime stuff is handled by efi_variable.c functions.

Yes, they are, but there is a subtle difference here.
efi_variable.c and efi_variable_tee.c have their own version of
runtime service implementation. The functions that can be shared
across the two are in efi_var_common.c.
What your patch does is shoehorn an SPI backend storage to the
existing implementation for storing variables to a file. Depending on
what we decide for runtime calls that can end up being messy.

efi_variable.c -> deals with variables on a file
efi_variable_tee.c -> deals with variables hidden behind the secure world

I think it would be cleaner to add efi_variable_sf.c and move the
common functions you need from efi_variable.c -> efi_var_common.c
Heinrich, do you have a preference for that?

Thanks
/Ilias
> This code is run while we are still in U-boot to initialise the memory backed.
> As of now, we don't have the SetVariableRT so I guess this won't be a problem.
> I took the code from efi_var_file.c and just replaced file storage
> with SPI Flash.
>
> I will have to look deeper once I get to implement SetVariableRT which will
> be a separate patch series once I get this in.
>
> I have submitted V2 of the patch, can you kindly review them.
>
> Kind regards,
> Shantur

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

[Shantur Rathore]

Hi Ilias,

On Mon, Nov 27, 2023 at 7:09 AM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Shantur
>
> Please don't send a v2 unless the v1 discussion has settled. It just
> makes life harder. I'll ignore v2 for now and respond here.
>

Sure, I'm still learning the ways around here.

> [...]
>
> >
> > >
> > > > +       if (ret)
> > > > +               goto error;
> > > > +
> > > > +       ret = spi_flash_erase_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, EFI_VAR_BUF_SIZE);
> > > > +       debug("%s - Erased SPI Flash offset %lx\n", __func__, CONFIG_EFI_VARIABLE_SF_OFFSET);
> > > > +       if (ret)
> > > > +               goto error;
> > > > +
> > > > +       ret = spi_flash_write_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, len, buf);
> > > > +       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("Out of memory\n");
> > > > +               return EFI_OUT_OF_RESOURCES;
> > > > +       }
> > >
> > > You don't want normal memory for the variables.  Instead, you want EFI
> > > memory which is marked for EFI runtime services data. U-Boot already
> > > has code to support Get/GetNextvariableRT, you just need to use
> > > efi_var_mem_init().
> > > If you need hints have a look at lib/efi_loader/efi_variable_tee.c
> > > which implements variables stored in an RPMB, but still uses the
> > > memory backend for runtime services.
> > >
> >
> > I believe EFI Runtime stuff is handled by efi_variable.c functions.
>
> Yes, they are, but there is a subtle difference here.
> efi_variable.c and efi_variable_tee.c have their own version of
> runtime service implementation. The functions that can be shared
> across the two are in efi_var_common.c.
> What your patch does is shoehorn an SPI backend storage to the
> existing implementation for storing variables to a file. Depending on
> what we decide for runtime calls that can end up being messy.
>
> efi_variable.c -> deals with variables on a file
> efi_variable_tee.c -> deals with variables hidden behind the secure world
>
> I think it would be cleaner to add efi_variable_sf.c and move the
> common functions you need from efi_variable.c -> efi_var_common.c
> Heinrich, do you have a preference for that?

I agree, the v1 patch was not implementing it cleanly.
Based on Akashi's comments I made these changes for v2 ( v3 with some
compiler warning fixes )

- Moved common stuff out of efi_var_file.c to efi_var_common.c
- Added efi_var_sf.c to handle writing vars to SPI Flash as
efi_var_file.c does for File

Happy to change things if needed.

Kind regards,
Shantur

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

[Ilias Apalodimas]

On Mon, 27 Nov 2023 at 11:31, Shantur Rathore <i@shantur.com> wrote:
>
> Hi Ilias,
>
> On Mon, Nov 27, 2023 at 7:09 AM Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Shantur
> >
> > Please don't send a v2 unless the v1 discussion has settled. It just
> > makes life harder. I'll ignore v2 for now and respond here.
> >
>
> Sure, I'm still learning the ways around here.

No worries -- we've all been there.

>
> > [...]
> >
> > >
> > > >
> > > > > +       if (ret)
> > > > > +               goto error;
> > > > > +
> > > > > +       ret = spi_flash_erase_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, EFI_VAR_BUF_SIZE);
> > > > > +       debug("%s - Erased SPI Flash offset %lx\n", __func__, CONFIG_EFI_VARIABLE_SF_OFFSET);
> > > > > +       if (ret)
> > > > > +               goto error;
> > > > > +
> > > > > +       ret = spi_flash_write_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, len, buf);
> > > > > +       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("Out of memory\n");
> > > > > +               return EFI_OUT_OF_RESOURCES;
> > > > > +       }
> > > >
> > > > You don't want normal memory for the variables.  Instead, you want EFI
> > > > memory which is marked for EFI runtime services data. U-Boot already
> > > > has code to support Get/GetNextvariableRT, you just need to use
> > > > efi_var_mem_init().
> > > > If you need hints have a look at lib/efi_loader/efi_variable_tee.c
> > > > which implements variables stored in an RPMB, but still uses the
> > > > memory backend for runtime services.
> > > >
> > >
> > > I believe EFI Runtime stuff is handled by efi_variable.c functions.
> >
> > Yes, they are, but there is a subtle difference here.
> > efi_variable.c and efi_variable_tee.c have their own version of
> > runtime service implementation. The functions that can be shared
> > across the two are in efi_var_common.c.
> > What your patch does is shoehorn an SPI backend storage to the
> > existing implementation for storing variables to a file. Depending on
> > what we decide for runtime calls that can end up being messy.
> >
> > efi_variable.c -> deals with variables on a file
> > efi_variable_tee.c -> deals with variables hidden behind the secure world
> >
> > I think it would be cleaner to add efi_variable_sf.c and move the
> > common functions you need from efi_variable.c -> efi_var_common.c
> > Heinrich, do you have a preference for that?
>
> I agree, the v1 patch was not implementing it cleanly.
> Based on Akashi's comments I made these changes for v2 ( v3 with some
> compiler warning fixes )
>
> - Moved common stuff out of efi_var_file.c to efi_var_common.c
> - Added efi_var_sf.c to handle writing vars to SPI Flash as
> efi_var_file.c does for File
>
> Happy to change things if needed.

Great, I'll have a look at v3 then

Thanks
/Ilias
>
> Kind regards,
> Shantur