Hi Ilias,
On Thu, Nov 30, 2023 at 10:10 PM Ilias Apalodimas
<ilias.apalodi...@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
