[PATCH v9] efi_vars: Implement SPI Flash store

[Heinrich Schuchardt]

On 3/11/26 16:08, Heinrich Schuchardt wrote:
> On 2/16/26 14:17, Michal Simek wrote:
>> From: Shantur Rathore <i at shantur.com>
>>
>> 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 at shantur.com>
>> Signed-off-by: Michal Simek <michal.simek at amd.com>
>> Tested-by: Neil Armstrong <neil.armstrong at linaro.org> # on AML-S905D3-CC
>> Acked-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
>>
>> ---
>>
>> Changes in v9:
>> - Remove EFI_RT_VOLATILE_STORE enabling
>> - Cleanup commit message
>>
>> Changes in v8:
>> - Add missing EFI_VARIABLE_SF_STORE dependency to 
>> EFI_VARIABLE_SF_DEVICE_INDEX
>>
>> Changes in v7:
>> - sed -i 's/efi_var_from/efi_var_from_storage/g'
>>
>> Changes in v6:
>> - sed -i 's/efi_var_read/efi_var_from/g'
>> - sed -i 's/efi_var_write/efi_var_to_storage/g'
>>
>> Changes in v4:
>> - Extend Kconfig description
>> - Extend commit message and describe efivar missing part
>> - use unify methods for reading/writing variable
>>
>> 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
>>
>>   lib/efi_loader/Kconfig      | 33 +++++++++++++
>>   lib/efi_loader/Makefile     |  1 +
>>   lib/efi_loader/efi_var_sf.c | 92 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 126 insertions(+)
>>   create mode 100644 lib/efi_loader/efi_var_sf.c
>>
>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>> index 579eed658801..315dd4f3e794 100644
>> --- a/lib/efi_loader/Kconfig
>> +++ b/lib/efi_loader/Kconfig
>> @@ -124,6 +124,24 @@ 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.
>> +
>> +      Note that SPI Flash devices have a limited number of program/erase
>> +      cycles. Frequent updates to UEFI variables may cause excessive 
>> wear
>> +      and can permanently damage the flash device, particularly on 
>> SPI NAND
>> +      or low-end SPI NOR parts without wear leveling. This option 
>> should be
>> +      used with care on such systems, and is not recommended for 
>> platforms
>> +      where UEFI variables are updated frequently.
>> +
>>   config EFI_MM_COMM_TEE
>>       bool "UEFI variables storage service via the trusted world"
>>       depends on OPTEE
>> @@ -194,6 +212,21 @@ config FFA_SHARED_MM_BUF_ADDR
>>         the MM SP in secure world.
>>         It is assumed that the MM SP knows the address of the shared 
>> MM communication buffer.
>> +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"
>> +    depends on EFI_VARIABLE_SF_STORE
>> +    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 !COMPILE_TEST
>> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
>> index ca1775eb03be..d73ad43951b1 100644
>> --- a/lib/efi_loader/Makefile
>> +++ b/lib/efi_loader/Makefile
>> @@ -54,6 +54,7 @@ obj-y += efi_variable_tee.o
>>   else
>>   obj-y += efi_variable.o
>>   obj-$(CONFIG_EFI_VARIABLE_FILE_STORE) += efi_var_file.o
>> +obj-$(CONFIG_EFI_VARIABLE_SF_STORE) += efi_var_sf.o
>>   obj-$(CONFIG_EFI_VARIABLES_PRESEED) += efi_var_seed.o
>>   endif
>>   obj-y += efi_watchdog.o
>> diff --git a/lib/efi_loader/efi_var_sf.c b/lib/efi_loader/efi_var_sf.c
>> new file mode 100644
>> index 000000000000..61d68f7c5c94
>> --- /dev/null
>> +++ b/lib/efi_loader/efi_var_sf.c
>> @@ -0,0 +1,92 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * SPI Flash interface for UEFI variables
>> + *
>> + * Copyright (c) 2023, Shantur Rathore
>> + * Copyright (C) 2026, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#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_storage(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);
> 
> Hello Michal,
> 
> Thank you for driving this.
> 
> The log system is configurable to print file, line, function name if 
> wanted. We should not duplicate this here. Please, remove __func__ 
> throughout the patch.
> 
>> +
>> +    if (ret != EFI_SUCCESS)
>> +        goto error;
>> +
>> +    ret = uclass_get_device(UCLASS_SPI_FLASH, 
>> CONFIG_EFI_VARIABLE_SF_DEVICE_INDEX, &sfdev);
>> +    if (ret)
> 
> This is not an EFI return code. EFI_DEVICE_ERROR should be used.
> 
>> +        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);
> 
> A debug output if erasing failed would be more interesting.
> 
>> +    if (ret)
> 
> This is not an EFI return code. EFI_DEVICE_ERROR should be used.
> 
>> +        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);
> 
> The uclass_get_device() return code should be saved in an int variable 
> to avoid confusion.
> 
>> +
>> +    if (ret)
> 
> This is not an EFI return code. EFI_DEVICE_ERROR should be used.
> 
>> +        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_storage(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__);
> 
> This function may be called from an EFI GUI application. We should not 
> output any strings to the console from API functions. It is task of the 
> caller to take appropriate actions. E.g. show a pop-up if a failure 
> occurs. log_debug() is ok.

I got this wrong. This function is called when initializing the EFI 
sub-system. So writing errors to the console is ok. It is 
efi_var_to_storage() which is invoked by EFI applications and where we 
should avoid console output.

Best regards

Heinrich

> 
>> +        return EFI_OUT_OF_RESOURCES;
>> +    }
>> +
>> +    ret = uclass_get_device(UCLASS_SPI_FLASH, 0, &sfdev);
> 
> efi_var_restore() returns a signed int and not an unsigned efi_status_t. 
> Please, define a separate variable.
> 
>> +    if (ret)
> 
> Please, return EFI_DEVICE_ERROR.
> 
>> +        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 spi_flash_read_dm() failed, you will just print some random value for 
> buf->length. Please, print the return value instead.
> 
>> +
>> +    if (ret || buf->length < sizeof(struct efi_var_file)) {
>> +        log_err("%s - buffer read from SPI Flash isn't valid\n", 
>> __func__);
> 
> If spi_flash_read_dm() fails we should return EFI_DEVICE_ERROR.
> 
>> +        goto error;
> 
> After erasing the SPI flash buf->length will be zero here. This should 
> not stop us from setting EFI variables.
> 
> It is the task of efi_var_restore to check the validity of the buffer. 
> So the length check here should be removed.
> 
> The missing check in efi_var_restore() is
> 
> buf->length <= EFI_VAR_BUF_SIZE
> 
> to avoid a buffer overrun when calculating the CRC32.
> 
>> +    }
>> +
>> +    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;
> 
> We should not return a non-EFI return code here if uclass_get_device() 
> or spi_flash_read_dm() failed.
> 
>> +}
> 
> I configured starfive_visionfive2_defconfig with
> 
> CONFIG_EFI_VARIABLE_SF_STORE=y
> CONFIG_EFI_VARIABLE_SF_OFFSET=0xc0000
> CONFIG_EFI_VARIABLE_SF_DEVICE_INDEX=0
> CONFIG_EFI_VAR_BUF_SIZE=131072
> 
> and it works.
> 
> Best regards
> 
> Heinrich