Dear Takahiro,
In message 20190604065211.15907-1-takahiro.akashi@linaro.org you wrote:
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.
These are all valid arguments, and actually there are also a number of U-Boot variables that are 100% volatile (say, "filesize") so saving them to the persistent environment is just a waste of resources.
Those observation rationalizes that the implementation of UEFI variables should be revamped utilizing dedicated storage for them.
This patch set is yet experimental and rough-edged(See known issues below), but shows how UEFI variables can be split from U-Boot environment.
I dislike this patch, because it has a narrow view, is prety intrusive and not generally usable.
I suggest adding a "volatile" property to the already existing mechanism of environment variable flags, and excluding variables with that flag from "env export" and "env save" operations.
The advantages of this approach:
- only minimal code additions needed - generally useful, i. e. not only for UEFI but also for "normal" U-Boot environment variables (think of "filesize" etc.) - independent of environment storage, i. e. it is not limited to FAT and also works with "env export".
Known issues/restriction/TODO:
- UEFI spec defines "globally defined variables" with specific attributes, but with this patch, we don't check against the user-supplied attribute for any variable.
This could be handled using glags, too.
- Only FAT can be enabled for persistent storage for UEFI non-volatile variables.
If you want to save / export _only_ UEFI variables (but not the rest of the U-Boot environment), you could for example add a "UEFI" property to the variable flags, and extend the "save" and "export" commands to process only variables with a given property (in your case, UEFI). Again, this requires no special code, is storage-agnostic and fits well into the existing code base. The major benefit is that this is a useful extension not only for UEFI, but for other use cases as well.
- The whole area of storage will be saved at every update of one variable. It can be optimized.
If it is desired that variable changes get immediately written to the persistent storage, this could also be easily implemented using the variable flags - just add an "autosave" property and run "env save" whenever the value f such a variable changes.
- An error during saving may cause inconsistency between cache (hash table) and the storage.
It would also cause that the written copy is invalid (checksum), so the redundant copy of the environment will be used on next bot. This is the same for all errors when storing the persistent environment.
cmd/efidebug.c | 3 + cmd/nvedit.c | 3 +- cmd/nvedit_efi.c | 15 +- env/Kconfig | 39 ++++ env/env.c | 155 ++++++++++++- env/fat.c | 105 +++++++++ include/asm-generic/global_data.h | 3 + include/environment.h | 31 +++ lib/efi_loader/Kconfig | 10 + lib/efi_loader/efi_bootmgr.c | 3 +- lib/efi_loader/efi_setup.c | 6 + lib/efi_loader/efi_variable.c | 354 +++++++++++++++++++++++------- 12 files changed, 641 insertions(+), 86 deletions(-)
This patch is pretty intrusive and single-purpose.
Can you please think about my suggestion? I feel it would be much less intrusive and much more usefule especially for non-UEFI use cases. Also, it helps to avoid a few of your limitations.
Thanks.
Best regards,
Wolfgang Denk