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

[Wolfgang Denk <wd@denx.de>]

Dear Takahiro,

In message <20190717082525.891-1-takahiro.akashi@linaro.org> you wrote:
> # In version 4 of this patch set, the implementation is changed to meet
> # Wolfgang's requirements.

Thanks.

> * 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.

Urgh... ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE is way too long,
and unnecessary complicated.  And we don't need 3 new flags, as ENV_FLAGS_VARSTORAGE_NON_VOLATILE
is the default anyway.

Please just define

	ENV_FLAGS_VOLATILE
and
	ENV_FLAGS_AUTOSAVE

> * 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

Why do we need all these _ext names?  Please don't.  Just give the
newly added functions new names.

> * 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).

NAK. ':' is a legal character in a variable name, so you cannot use
it here for new purposes. For example:

	=> setenv baudrate:san foo
	=> printenv baudrate:san
	baudrate:san=foo

> * Each environment storage driver must be modified so as to be aware
>   of contexts. (See flash and fat in my patch set.)

I dislike this.  Can we not do all the different handling in a
central place, and basically leave the storage handlers unchanged?
They do not need to know about contexts; they just do the I/O.

> Known issues/restriction/TODO:
> * Existing interfaces may be marked obsolute or deleted in the future.

Which "existing interfaces" are you talking about?

> * 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.

Please explain what you have in ind here.  What I'm guessing from
this short description sounds not so good to me, but you may have
better ideas...

> * 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.

NAK.  Both the VOLATILE and the AUTOSAVE properties are interesting
features, and should be available to normale U-Boot environment
variables.  So the U-Boot context must support all variantes of
"normal" (persistent, non-autosave), volatile and autosave variables
at the same time.  If you don't need this for UEFI, ok.  But we want
to have this for U-Boot.

>     -> In this sense, NON_VOLATILE[or _AUTOSAVE] should be attributed
>        to a context itself.

No.  This is NOT what we need.

> * 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.

NAK. This must ne fixed.

> * 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).

I think your approach to fix this is wrong.  The problem should not
happen in the first place.  And no,  stdin/stdout/stderr are _not_
volatile.

> * 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.)

Correct, the current code is incompatible.  This must be solved
differently.  Colon is a legal character in a variable name.

> * The whole area of storage will be saved at *every* update of
>   one UEFI variable. It should be optimized if possible.

This is only true for UEFI storage, right?

> You can also define a non-volatile variable from command interface:
> => setenv -e -nv FOO baa

This makes no sense.  Being non-volatile is the default.  This must
remain as is.  Only "volatile" and "autosave" are new attributes.

More comments with the patches...

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
It usually takes more than three weeks to prepare  a  good  impromptu
speech.                                                  - Mark Twain