[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 at 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