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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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
>