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

[Ilias Apalodimas]

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.

[...]

>
> >
> > > +       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?

Thanks
/Ilias
> 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