[U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Heinrich,

On Wed, Jul 17, 2019 at 09:05:55PM +0200, Heinrich Schuchardt wrote:
> On 7/17/19 10:25 AM, AKASHI Takahiro wrote:
> ># In version 4 of this patch set, the implementation is changed to meet
> ># Wolfgang's requirements.
> ># I believe that this is NOT intrusive, and that my approach here is NOT
> ># selfish at all. If Wolfgang doesn't accept this approach, however,
> ># I would like to go for "Plan B" for UEFI variables implementation.
> >
> >This patch set is an attempt to implement non-volatile attribute for
> >UEFI variables. Under the current implementation,
> >* SetVariable API doesn't recognize non-volatile attribute
> >* While some variables are defined non-volatile in UEFI specification,
> >   they are NOT marked as non-volatile in the code.
> >* env_save() (or "env save" command) allows us to save all the variables
> >   into persistent storage, but it may cause volatile UEFI variables,
> >   along with irrelevant U-Boot variables, to be saved unconditionally.
> >
> >Those observation rationalizes that the implementation of UEFI variables
> >should be revamped utilizing dedicated storage for them.
> >
> >Basic ideas:
> >* Each variable may be labelled with a "context." Variables with
> >   different contexts will be loaded/saved in different storages.
> >   Currently, we define two contexts:
> >
> >     ENVCTX_UBOOT: U-Boot environment variables
> >     ENVCTX_UEFI: UEFI variables
> >
> >* Each variable may have a third attribute, "variable storage."
> >   There are three new flags defined:
> >   ('flags' are a bit-wise representation of attributes.)
> >
> >     ENV_FLAGS_VARSTORAGE_VOLATILE: A variable only exists during runtime.
> >     ENV_FLAGS_VARSTORAGE_NON_VOLATILE: A variable is persistent, but explicit
> >         "load" and "save" command is required.
> >     ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE: A variable is persistent,
> >         any change will be written back to storage immediately.
> >
> >* Following those extensions, new interfaces are introduced:
> >   for hashtable,
> >     hsearch_r -> hsearch_ext
> >     hmatch_r  -> hmatch_ext
> >     hdelete_r -> hdelete_ext
> >     himport_r -> himport_ext
> >     hexport_r -> hexport_ext
> >   for env,
> >     env_save   -> env_save_ext
> >     env_load   -> env_load_ext
> >     env_import -> env_import_ext
> >     env_export -> env_export_ext
> >   They will take one or two additional arguments, context and flags,
> >   comparing with the existing corresponding interfaces.
> >
> >* Even with those changes, existing interfaces will maintain
> >   its semantics, as if all the U-Boot environment variables were with
> >   UEFI_UBOOT context and VARSTORAGE_NON_VOLATILE attribute.
> >   (The only exception is "env_load()." See below.)
> >
> >* There is one thing visible to users; *Exported* variables now have
> >   attribute information, which is appended to variable's name
> >   in a format of ":xxx"
> >   For example,
> >     => printenv
> >        arch:san=arm
> >        baudrate:san=115200
> >        board:san=qemu-arm
> >        ...
> >   This information is necessary to preserve attributes across reboot
> >   (save/load).
> >
> >* Each environment storage driver must be modified so as to be aware
> >   of contexts. (See flash and fat in my patch set.)
> >
> >Known issues/restriction/TODO:
> >* Existing interfaces may be marked obsolute or deleted in the future.
> >
> >* Currently, only flash and fat drivers are modified to support contexts.
> >* Currently, only fat driver is modified to support UEFI context.
> >
> >     -> Applying changes to the other drivers is straightforward.
> >
> >* ENV_IS_NOWHERE doesn't work if we want to disable U-Boot environment,
> >   but still want to enable UEFI (hence UEFI variables).
> >
> >     -> One solution would be to add *intermediate* configurations
> >        to set ENV_IS_NOWHERE properly in such a case.
> >
> >* Any context won't have VARSTORAGE_NON_VOLATILE and
> >   VARSTORAGE_NON_VOLATILE_AUTOSAVE entries at the same time. This is
> >   because it will make "save" operation ugly and unnecessarily complicated
> >   as we discussed in ML.
> >
> >     -> In this sense, NON_VOLATILE[or _AUTOSAVE] should be attributed
> >        to a context itself.
> >
> >* env_load() may lose some of compatibility; It now always imports variables
> >   *without* clearing existing entries (i.e. H_NOCLEAR specified) in order
> >   to support multiple contexts simultaneously.
> >
> >     -> I have no other good idea to solve this issue.
> >
> >* Unfortunately, some of existing U-Boot variables seem to be
> >   "volatile" in nature. For example, we will see errors at boot time:
> >	...
> >	Loading Environment from Flash... OK
> >	## Error inserting "fdtcontroladdr" variable, errno=22
> >	In:    pl011 at 9000000
> >	Out:   pl011 at 9000000
> >	Err:   pl011 at 9000000
> >	## Error inserting "stdin" variable, errno=22
> >	## Error inserting "stdout" variable, errno=22
> >	## Error inserting "stderr" variable, errno=22
> >
> >     -> We will have to changes their attributes one-by-one.
> >	(Those can also be set through ENV_FLAGS_LIST_STATIC in env_flags.h).
> >
> >* As described above, attribute information is visible to users.
> >   I'm afraid that it might lead to any incompatibility somewhere.
> >   (I don't notice any issues though.)
> >
> >* The whole area of storage will be saved at *every* update of
> >   one UEFI variable. It should be optimized if possible.
> >
> >* An error during "save" operation may cause inconsistency between cache
> >   (hash table) and the storage.
> >
> >     -> This is not UEFI specific though.
> >
> >* Patch#15 may be unnecessary.
> >
> >* Add tests if necessary.
> >
> >Note:
> >If we need "secure storage" for UEFI variables, efi_get_variable/
> >efi_get_next_variable_name/efi_set_variable() should be completely replaced
> >with stub functions to communicate with secure world.
> >This patch won't affect such an implementation.
> >
> >Usage:
> >To enable this feature, the following configs must be enabled:
> >   CONFIG_ENV_IS_IN_FAT
> >   CONFIG_ENV_FAT_INTERFACE
> >   CONFIG_ENV_EFI_FAT_DEVICE_AND_PART
> >   CONFIG_ENV_EFI_FAT_FILE
> >
> >You can also define a non-volatile variable from command interface:
> >=> setenv -e -nv FOO baa
> >
> >Patch#1 to #5 are to add 'context' support to env interfaces.
> >Patch#6 to #13 are to add 'variable storage' attribute to env interfaces.
> >Patch#14 to #16 are to modify UEFI variable implementation to utilize
> >   new env interfaces.
> >
> >Changes in v4 (July 17, 2019)
> >* remove already-merged patches
> >* revamp after Wolfgang's suggestion
> >
> >Changes in v3 (June 4, 2019)
> >* remove already-merged patches
> >* revamp the code again
> >* introduce CONFIG_EFI_VARIABLE_USE_ENV for this configuration.
> >   Once another backing storage, i.e. StMM services for secure boot,
> >   is supported, another option will be added.
> >
> >Changes in v2 (Apr 24, 2019)
> >* rebased on efi-2019-07
> >* revamp the implementation
> >
> >v1 (Nov 28, 2018)
> >* initial
> 
> Thanks for all the effort you put into getting this implemented.
> 
> Unfortunately the code does not compile after applying patches 1-15 for
> sandbox_defconfig:
> 
> In file included from include/asm-generic/global_data.h:23,
>                  from ./arch/sandbox/include/asm/global_data.h:18,
>                  from include/dm/of.h:11,
>                  from include/dm/ofnode.h:12,
>                  from include/dm/device.h:13,
>                  from include/dm.h:9,
>                  from drivers/net/mdio_sandbox.c:7:
> include/environment.h:158:19: error: ‘CONFIG_ENV_SIZE’ undeclared here
> (not in a function); did you mean ‘CONFIG_OF_LIVE’?
>  #define ENV_SIZE (CONFIG_ENV_SIZE - ENV_HEADER_SIZE)
>                    ^~~~~~~~~~~~~~~
> include/environment.h:162:21: note: in expansion of macro ‘ENV_SIZE’
>   unsigned char data[ENV_SIZE]; /* Environment data  */
>                      ^~~~~~~~
>   CC      drivers/power/domain/sandbox-power-domain.o
> 
> Here is another error:
> 
>   CC      cmd/nvedit.o
> cmd/nvedit.c: In function ‘do_env_print_ext’:
> cmd/nvedit.c:129:13: error: ‘ENVCTX_UEFI’ undeclared (first use in this
> function); did you mean ‘ENVCTX_UBOOT’?
>   if (ctx == ENVCTX_UEFI)
>              ^~~~~~~~~~~
>              ENVCTX_UBOOT
> cmd/nvedit.c:129:13: note: each undeclared identifier is reported only
> once for each function it appears in
> 
> Please, run Travis CI before resubmitting.

I will fix, but please note that this patch set is more or less RFC,
and that the primary aim is to see if Wolfgang agrees with basic ideas
and the approach here.

> Patch 16 is not based on origin/master and cannot be applied.

Currently, the patch is based on v2019.07.

> I found no adjustments to test/env/*. How will your changes be tested?

Future work, but "ut env" and "bootefi selftest" have been passed in
relevant test cases.
Please note, again, that even without my patch, the current "ut env" is
far from perfect in terms of coverage of available features.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >
> >AKASHI Takahiro (16):
> >   hashtable: extend interfaces to handle entries with context
> >   env: extend interfaces to import/export U-Boot environment per context
> >   env: extend interfaces to label variable with context
> >   env: flash: add U-Boot environment context support
> >   env: fat: add U-Boot environment context support
> >   env: add variable storage attribute support
> >   env: add/expose attribute helper functions for hashtable
> >   hashtable: import/export entries with flags
> >   hashtable: extend hdelete_ext for autosave
> >   env: save non-volatile variables only
> >   env: save a context immediately if 'autosave' variable is changed
> >   env: extend interfaces to get/set attributes
> >   cmd: env: show variable storage attribute in "env flags" command
> >   env: fat: support UEFI context
> >   env,efi_loader: define flags for well-known global UEFI variables
> >   efi_loader: variable: rework with new extended env interfaces
> >
> >  cmd/nvedit.c                      | 186 ++++++++++++++++++++++--------
> >  env/Kconfig                       |  42 ++++++-
> >  env/common.c                      |  82 ++++++++++---
> >  env/env.c                         |  92 +++++++++++----
> >  env/fat.c                         | 112 +++++++++++++++---
> >  env/flags.c                       | 149 +++++++++++++++++++++++-
> >  env/flash.c                       |  28 +++--
> >  include/asm-generic/global_data.h |   7 +-
> >  include/efi_loader.h              |   1 +
> >  include/env_default.h             |   1 +
> >  include/env_flags.h               |  63 +++++++++-
> >  include/environment.h             |  89 ++++++++++++--
> >  include/exports.h                 |   4 +
> >  include/search.h                  |  16 +++
> >  lib/efi_loader/Kconfig            |  12 ++
> >  lib/efi_loader/Makefile           |   2 +-
> >  lib/efi_loader/efi_setup.c        |   5 +
> >  lib/efi_loader/efi_variable.c     |  50 ++++++--
> >  lib/hashtable.c                   | 143 ++++++++++++++++++++---
> >  19 files changed, 925 insertions(+), 159 deletions(-)
> >
>