[U-Boot] [PATCH v5 00/19] efi_loader: non-volatile variables support

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

Wolfgang,

I would like to focus on the critical and most arguable point in this email.

On Tue, Oct 29, 2019 at 02:28:19PM +0100, Wolfgang Denk wrote:
> Dear Takahiro,
> 
> In message <20191028011435.GP10448@linaro.org> you wrote:
> >
> > > Can you please at least comment on the size impact?  How much does
> > > the code size grow on everage, especially for SPL?
> >
> > Well, from the next version as I will make a drastic change as far as
> > I follow your suggestion below.
> 
> OK.
> 
> > Okay, I see but I hope that you should have said so much earlier.
> > My patch (v5) has been stranded almost two months without any comments.
> 
> I have said so uin lcear and unmistakable words when discussion was
> to add me as maintainer for environment code.  I cannot commit to
> such a position as I don't have enough time available.
> 
> > > > > > > > * Non-volatile feature is not implemented in a general form and must be
> > > > > > > >   implemented by users in their sub-systems.
> > > > > 
> > > > > I don't understand what this means, or why such a decision was made.
> > > > > Which sub-systems do you have in mind here?
> > > >
> > > > UEFI sub-system/library.
> > > 
> > > What needs to be done to have this - say - for U-Boot context?
> > >
> > > > > What prevented you from implementing a solution to works for all of
> > > > > us?
> > > 
> > > ?
> >
> > At least for me or UEFI, nothing more, other than a *context*, is needed
> > to meet the requirement.
> > If some one really want to have such(?) a feature, he or she can on top
> > of my patch.
> > I believe that my attitude here is fair enough.
> 
> I would appreciate if you could actually answer my questions.  there
> are two of them:
> 
> You write: "Non-volatile feature is not implemented in a general
> form and must be implemented by users in their sub-systems."
> 
> Q1: Why did you not implement this feature in a generic way, so it
>     is automatically available to all sub-systems that support
>     contexts?
> 
> Q2: What exactly needs to be done, which code needs to be writtenm,
>     to implement this feature - say - for U-Boot context?
> 
> 
> > So do you always admit the following coding?
> > ===8<===
> >   (in some UEFI function)
> >   ...
> >   old_ctx = env_set_context(ctx_uefi);
> >   env_set(var, value);
> >   env_set_context(old_ctx);
> >   ...
> > ===>8===
> 
> This is even worse, as instead of adding a single argument to a
> function call you are adding two full fucntion calls.
> 
> But why would that be needed?
> 
> U-Boot is strictly single tasking.  You always know what the current
> context is, and nobody will change it behind your back.
> 
> And UEFI functions are not being called in random places, they are
> limited to a small set of UEFI commands, right?  So why do you not
> just enter UEFI context when you start executing UEFI code, and
> restore the rpevious state when returning to non-UEFI U-Boot?

Can you elaborate What you mean by "entering UEFI context"?

What I'm thinking of here is that we would add one more member field, which
is a pointer to my "env_context," in GD and change it in env_set_context().
This can be defined even as an inline function.
(My other patches, at least patch#1 to #6, can be used almost as they are.)

I don't see why you deny such a simple solution with lots of flexibility.

-Takahiro Akashi

> Or am I missing something?
> 
> > > > > You don't seriously expect to have patches which cause
> > > > > "incomprehensible memory corruption" to be included into mainline?
> > > >
> > > > It will be just a matter of time for debugging.
> > > 
> > > It might be difficult to find willing testers under such
> > > circumstances.  I would not want to run code with serious known
> > > bugs.
> >
> > I believe that the issue is independent from the basic approach
> > that we are now discussing.
> 
> The reason of this problem must be understood and the bug fixed
> before it can go into mainline.
> 
> > > I think this is a new feature that needs to be tested.  You focus on
> > > UEFI + FAT and I don't know if UEFI + non-FAT makes sense, but at
> > > least (1) non-UEFI + FAT and (2) non-UEFI + non-FAT are
> > > configurations which must be tested - absolute minimum is at least
> > > one example implementation. My suggestion would be to use something
> > > that is not file system based, but using plain storage, say a board
> > > which has the environment in SPI NOR flash.
> >
> > What do you mean by "non-UEFI"? Disabling UEFI, or testing U-Boot environment
> > variables on no-FAT device?
> > If the latter, I have tested it with flash (on qemu).
> 
> We need to test if the new context code works (and does not cause
> problems) on systems where UEFI is not enabled.
> 
> 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
> Don't you know anything? I should have thought anyone knows that  who
> knows anything about anything...      - Terry Pratchett, _Soul Music_