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

[Ilias Apalodimas]

On Mon, 27 Nov 2023 at 11:31, Shantur Rathore <i at shantur.com> wrote:
>
> Hi Ilias,
>
> On Mon, Nov 27, 2023 at 7:09 AM Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Hi Shantur
> >
> > Please don't send a v2 unless the v1 discussion has settled. It just
> > makes life harder. I'll ignore v2 for now and respond here.
> >
>
> Sure, I'm still learning the ways around here.

No worries -- we've all been there.

>
> > [...]
> >
> > >
> > > >
> > > > > +       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.
> >
> > Yes, they are, but there is a subtle difference here.
> > efi_variable.c and efi_variable_tee.c have their own version of
> > runtime service implementation. The functions that can be shared
> > across the two are in efi_var_common.c.
> > What your patch does is shoehorn an SPI backend storage to the
> > existing implementation for storing variables to a file. Depending on
> > what we decide for runtime calls that can end up being messy.
> >
> > efi_variable.c -> deals with variables on a file
> > efi_variable_tee.c -> deals with variables hidden behind the secure world
> >
> > I think it would be cleaner to add efi_variable_sf.c and move the
> > common functions you need from efi_variable.c -> efi_var_common.c
> > Heinrich, do you have a preference for that?
>
> I agree, the v1 patch was not implementing it cleanly.
> Based on Akashi's comments I made these changes for v2 ( v3 with some
> compiler warning fixes )
>
> - Moved common stuff out of efi_var_file.c to efi_var_common.c
> - Added efi_var_sf.c to handle writing vars to SPI Flash as
> efi_var_file.c does for File
>
> Happy to change things if needed.

Great, I'll have a look at v3 then

Thanks
/Ilias
>
> Kind regards,
> Shantur