[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 at linaro.org> wrote:
>
> Hi Shahtur
>
> On Wed, 22 Nov 2023 at 01:58, Shantur Rathore <i at 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 at 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