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

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

On Fri, Jul 19, 2019 at 08:50:18AM +0200, Wolfgang Denk wrote:
> 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.

But this rule *does* conform with other flags' naming rule.
Aren't other name, like
        env_flags_varaccess_changedefault in env_flags_varaccess, or
        ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR
long?
What is the upper limit that you think?

> And we don't need 3 new flags, as ENV_FLAGS_VARSTORAGE_NON_VOLATILE
> is the default anyway.

"default" may be different on different contexts.
It cannot be the reason.

> Please just define
> 
> 	ENV_FLAGS_VOLATILE
> and
> 	ENV_FLAGS_AUTOSAVE

Disagree.

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

You seem to misunderstand my patch. Please read my code first.
I have not renamed the existing interfaces, which still remain
for compatibility, and added new functions with "extended" 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:

What is your recommendation?
I didn't find what are the definition of "illegal" characters.

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

How? I don't get your point.
Did you read patch (#4 and) #5 and #14?
Context-specific code are quite isolated and yet we need to modify
drivers a bit.

> > Known issues/restriction/TODO:
> > * Existing interfaces may be marked obsolute or deleted in the future.
> 
> Which "existing interfaces" are you talking about?

h*_r(), env_import/export(), env_save/load() and etc.
Once all the occurrences in the repository have been udpated
using new interfaces, we will be able to delete them, won't you?

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

For example,
if we want to use FAT for UEFI variables, but don't want to enable
U-Boot envrionment for some reason (don't ask me why now),
we cannot simply enable ENV_IS_NOWHERE, then U-Boot envrionment
won't work as expected in some places.

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

NAK. Do you remember our discussion in the past?
Allowing such a feature may end up with ugly and complicated
implementation. I believe that we agreed on this point before.
> 
> >     -> In this sense, NON_VOLATILE[or _AUTOSAVE] should be attributed
> >        to a context itself.
> 
> No.  This is NOT what we need.

Why?

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

NAK.
This is a natural result of supporting multiple contexts
in U-Boot environment (plus UEFI variables in one hash table.)

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

I can't understand why we need to save stdin/stdout/stderr as
non-volatile variables. It should be initialized at every boot.

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

How?
You denied, in the past discussions in ML, that key or value be
'structured' as an opaque object.

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

Yes, so?

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

'-nv' option is already merged in the upstream, and
it is only valid for UEFI variables.
And again, there is no *default* attribute for UEFI variables.

Thanks you for your comments,
-Takahiro Akashi


> 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