[U-Boot] [PATCH v3 0/7] efi_loader: non-volatile variables support

[Wolfgang Denk]

Dear Takahiro,

In message <20190604065211.15907-1-takahiro.akashi at 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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The Gates in my computer are AND, OR and NOT; they are not Bill.