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

[Shantur Rathore]

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.

> [...]
>
> >
> > >
> > > > +       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.

Kind regards,
Shantur