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

[Wolfgang Denk]

Dear AKASHI Takahiro,

In message <20190719073606.GP21948 at linaro.org> you wrote:
>
> > 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

Yes, you are right.  I missed to review these patches in time.
It would have been much niver if these flags had simple, speaking
names like

	ENV_FLAG_READONLY
	ENV_FLAG_WRITEONCE
	ENV_FLAG_CHANGEDEFAULT

> long?
> What is the upper limit that you think?

> > Please just define
> > 
> > 	ENV_FLAGS_VOLATILE
> > and
> > 	ENV_FLAGS_AUTOSAVE
>
> Disagree.

Please explain why?

This is all that's needed. There is no more magic behind.

It makes no sense to invent ariticial "groups" like "access" or
"storage" when it just adds tons of new functions and many, many
lines of code.

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

I have read your code.  We don't want to have such aton of
duplicated funtions.  use the existing ones, and add context
handling.

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

As is, flags are stored in the ".flags" variable.  The reason is
that there was no backward compatible way to add such a property.

> I didn't find what are the definition of "illegal" characters.

The only characters which cannot be used in a variable name is '='
(which is the separator character in external storrage format, and
'\0', which is the terminator.

You cannot encode wadditional information in the external storage,
except by defining new  special variables - here, we use ".flags".

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

Yes, I have read this.  I find the implentation not elegant.  We
should keep such details out of the handler code to the extent
possible.  The data / parameters should prepared in a central place,
and just passed to the drivers.  It makes no sense to add "context"
knowhow to the drivers - these should just do I/O and not know about
such details.

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

No.  We should just change the existing interfaces to support
contexts, that's all.  not defining new ones (with longer names) and
then switching.

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

ENV_IS_NOWHERE means that there is no storage defined for the U-Boot
environment.  When implementing contexts properly, this becomes a
property of the U-Boot context only, while other contexts can have
other storage devices defined.

Yes, this needs fixing in a few places which assume that ther eis
only a single context - so far.

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

No we did not.  And there is absolutely no complicated
implementation needed.  All ne need is two new flags, and code to
test these flags in env_set() (for ENV_FLAGS_AUTOSAVE) and in
env_save() (for ENV_FLAGS_VOLATILE).

> > >     -> In this sense, NON_VOLATILE[or _AUTOSAVE] should be attributed
> > >        to a context itself.
> > 
> > No.  This is NOT what we need.
>
> Why?

Because it is a property of individual variables.  We do not have a
concept for context-wide flags, andif you want to define one, it
must not mess with the existing implementation of variable-specific
flags.

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

"Natural result"?  I don't think so.  It's a problem of your
implementation, and must be fixed, as it introduces incompatible
changes in behaviour.  I won't accept that.

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

No.  You may, for example, want to permanently set std* to
alternative devices, say to netconsole.  this only works if the
variables are saved, as is the case now.

Again, this must not be changed as it would break
backward-compatibility.

[Also I cannot see a problem with these specific variables.  Why do
you see such errors?]

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

See above.  The definition of the external storage format does not
allow any such additions.  Yes, this is very unfortunate, but that
code was written 19 years ago and such a need was just not foreseen.
As is, we have the ".flags" variable. We can extend this, and
eventually define similar things like ".context[U-Boot]",
".context[UEFI}" etc...

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

Just to make sure I undernd your intentions / code...

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

Well, I guess we can get rid of this in the context of this patch
series.

> And again, there is no *default* attribute for UEFI variables.

You see, a variable can be _either_ volatile or _not_.  So a single
flasg is all that's needed.  U-Boot has non-volatile as default, so
it would make sense to adapt this notation for the UEFI
implementation in U-Boot as well.  Then all contexts will just mark
volatile vars by setting the corresponding flag.  This does not
change behaviour for UEFI.  Yes, it changes the interface, but that
is probably easy, as you say there is no default.

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
There is wisdom in turning as often as possible from the familiar  to
the  unfamiliar: it keeps the mind nimble, it kills prejudice, and it
fosters humor.                                     - George Santayana