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

[AKASHI Takahiro]

Wolfgang,

On Fri, Oct 25, 2019 at 03:25:23PM +0200, Wolfgang Denk wrote:
> Dear Takahiro,
> 
> In message <20191025075645.GJ10448 at linaro.org> you wrote:
> >
> > I won't and cannot make replies on every comment that you gave me
> > below because we are very different at some basic points and
> > other comments are just details.
> 
> 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.

> 
> > So first I wanted to know if you agree to my *basic* approach or not,
> > not details, in order to go further, but still don't see
> > yes or no below.
> 
> To be honest: when I saw your monster patch series which basically
> touches every piece of code in U-Boot, I felt a strong temptation to
> just send a NAK and be done with it.  But I know this would not be
> fair.  But to be able to say yes I would need days to review the
> code and probably run some tests myself, and I don;t have that time.

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.

> So I can neither say yes or no, sorry.
> 
> > > My biggest concern is that such a highly invasive change cannot be
> > > simply rubberstamped in a code review - I think this also needs
> > > runtime testing on at least a significant number of the affected
> > > boards.  We should try to get help from at least some board
> > > maintainers - maybe you should ask for help for such testing n the
> > > board maintainers mailing list?
> 
> This is a point which is important to me.  We need at least a few
> "Tested-by" credits...
> 
> > > > > > * 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.

> 
> > > > > > Known issues/restriction/TODO:
> > > > > > * The current form of patchset is not 'bisect'able.
> > > > > >   Not to break 'bisect,' all the patches in this patch set must be
> > > > > >   put into a single commit when merging.
> > > > > >   (This can be mitigated by modifying/splitting Patch#18/#19 though.)
> > > 
> > > OK, so you are aware of this problem.
> > > 
> > > I must admit that I really hate this. If you could avoid all the API
> > > changes, this would solve this problem, wouldn't it?
> >
> > "Avoid all the API changes" is an approach that I took in all my
> > previous versions, but you *denied* it.
> >
> > That is: I proposed an approach in which the existing interfaces,
> > env_get/set(), were maintained for existing users/sub-systems.
> > Only new users who want to enjoy merits from new "context" feature may
> > use new *extended* interfaces, env_[get|set]_ext(), in my case UEFI.
> > As you *denied* it, this version (v5) is an inevitable result.
> >
> > Don't take me wrong, but I think that you made inconsistent comments.
> 
> I think you misunderstand. If we just need the same pointer in all
> functions dealing with the environment, there are at least two ways
> to implement this: we can add it as an argument to each and every
> function call; this will blow up code size and also impact execution
> speed.  Or we can add it to some (private or public) data structure
> that is visible everywhere.  In the simplest case we could add such
> a pointer to the global data (GD) structure as I suggested in my
> previous mail.  this would allow you to use basically the same code
> as now, but without needing to change all the argument lists.  In
> the result, you could drop your modifications of all common and
> board specififc files.  The code changes would be concentrated on
> the environment code, and it should be anle to submit bisectable
> patches again.
> 
> Yes, global variables have disadvantages, too, but does it not make
> sense here?  I think we will have only one active environment
> context at any time in U-Boot, so this seems to be at least a lesser
> evil than the zillion of changes of all call arguments.

I have thought of an idea as you mentioned above before but immediately
gave it up as it seems to be an ugly implementation and I simply dislike it.
But if you and Tom (or whoever can say ACK to my patch) think that it is
the *only* solution that you can accept, I will have to change my mind.

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

> 
> > > > > > * Unfortunately, this code fails to read U-Boot environment from flash
> > > > > >   at boot time due to incomprehensible memory corruption.
> > > > > >   See murky workaround, which is marked as FIXME, in env/flash.c.
> > > 
> > > Argh.  This is a killing point, isn't it?
> > > 
> > > 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.

> > > > > > * An error during "save" operation may cause inconsistency between
> > > > > >   cache (hash table) and the storage.
> > > > > >     -> This is not UEFI specific though.
> > > 
> > > Is this a new problem, or do you just mention this here for
> > > completeness?  We always had this issue, didn't we?
> >
> > As I said, "this is not UEFI specific."
> 
> This does not answer my question.  Are you just refering to the
> general problem that a write to the persistent storage area might
> fail, which has ever been present and which is inherently
> unfixxable, or does your new code add any additional problems of
> this kind?

A general problem in your term.

> > > How much testing can be done on boards that don't use FAT to store
> > > the environment?
> >
> > As I said, 
> > > > > >   In this version, only FAT file system and flash devices are supported,
> > > > > >   but it is quite straightforward to modify other drivers.
> >
> > If you don't think my patch is not qualified for a "PATCH" for some reason,
> > I will sent one as "RFC" from the next version. I don't care.
> 
> 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).

Thanks,
-Takahiro Akashi

> 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
> Our OS who art in CPU, UNIX be thy name.
> Thy programs run, thy syscalls done,
> In kernel as it is in user!