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@9000000 Out: pl011@9000000 Err: pl011@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(-)