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

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

Hi Wolfgang,

On Fri, Oct 25, 2019 at 09:06:44AM +0200, Wolfgang Denk wrote:
> Dear Takahiro,
> 
> In message <20191023065332.GE10448@linaro.org> you wrote:
> >
> > This is my second ping.
> > Could you please take time to review this patch?
> 
> Sorry, I'm afraid I will not find the time to review any such
> monster patch series any time soon.  I hope Joe (added to Cc:)
> has more resources available.
> 
> Only a few comments below...

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.

> > > > # In version 5 of this patch set, the implementation is changed again.
> > > > #
> > > > # I believe that this is NOT intrusive, and that my approach here is NOT
> > > > # selfish at all. If Wolfgang doesn't accept this approach, however,
> 
> 371 files changed, 3690 insertions(+), 2337 deletions(-)
> 
> I don't know what your scales are, but for me such a patch is
> extremely invasive. It affects a zillion of files in common code
> plus a ton of board specific files.
> 
> I did not find any information about the size impact or if the
> modified code continues to build for all boards - I remember we have
> a number of board with tight resources here and there.
> 
> You should at least provide some information how much bigger the new
> code gets.
> 
> From a quick glance I think the patches are not cleanly separated -
> you cannot change interfaces for the implementation in one step and
> for the callers in another, as this breaks bisectability.

As you noticed below, I'm aware of that.
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.

> 
> 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?
> 
> 
> Please do not misunderstand me - I am not trying to block any of
> this - I understand and appreciate the huge amount of efforts you
> have put into this.  But I feel this needs not only careful review,
> but also actual testing on as many of the effected boards as
> possible, and I simply don't have time for that.

Okay.

> 
> > > > * To access (get or set) a variable, associated context must be presented.
> > > >   So, almost of all existing env interfaces are changed to accept one
> > > >   extra argument, ctx.
> > > >   (I believe that this is Wolfgang's *requirement*.)
> 
> I wonder if we really need to change all interfaces.  I fear the
> size impact might bite us.  I only had a glimpse at the actual code,
> but it seemed to me as if we were just pssing the same information
> around everywhere.  Could we not use GD nstead, for example?
> 
> > > > * 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 prevented you from implementing a solution to works for all of
> us?
> 
> > > >   In version 4, U-Boot environment's attributes are extended to support
> > > >   non-volatile (or auto-save capability), but Wolfgang rejected
> > > >   my approach.
> > > >   As modifying attributes for this purpose would cause bunch of
> > > >   incompatibility issues (as far as I said in my cover letter and
> > > >   the discussions in ML), I would prefer a much simple approach.
> 
> I think we still have a different opinion here, but I'm lacking time
> for a thorough readding of the new code, so I hold back.  I hope
> that Joe can have a closer look...

You don't have to worry about my previous versions.
Just focus on the current v5.

> 
> > > > * Each backing storage driver must be converted to be aligned with
> > > >   new env interfaces to handle multiple contexts in parallel and
> > > >   provide context-specific Kconfig configurations for driver parameters.
> > > > 
> > > >   In this version, only FAT file system and flash devices are supported,
> > > >   but it is quite straightforward to modify other drivers.
> 
> If I see this correctly, there is a fundamental change in the
> implementation before: Up to now, the environment seize on external
> storage has been a compile time constant (CONFIG_ENV_SIZE).
> 
> Now this value gets computed, and I'm not even sure if this is a
> contant at run time.

Yes, it's constant for "U-Boot environment" (== U-Boot variables) context.
But for other sus-systems, in my case UEFI, the total size of storage
for (UEFI) variables may be different, but still constant.

> This scares me.  Does this not break compatibility?  How do you
> upgrade a system from an older version of U-Boot to one with your
> patches?
> 
> > > > 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.

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

> > > > * 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 variables, right?

Yes.

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

> > > > * I cannot test all the platforms affected by this patchset.
> 
> Sure, so please seek help from the board maintainers.
> 
> And please provide size statistics.
> 
> > > > To enable this feature for example with FAT file system, the following
> > > > configs must be enabled:
> > > >   CONFIG_ENV_IS_IN_FAT
> > > >   CONFIG_ENV_FAT_INTERFACE
> > > >   CONFIG_ENV_EFI_FAT_DEVICE_AND_PART
> > > >   CONFIG_ENV_EFI_FAT_FILE
> 
> 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.

Thanks,
-Takahiro Akashi

> 
> Sorry that I can't be of any better help here...
> 
> 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
> 'What shall we do?' said Twoflower.  'Panic?'  said  Rincewind  hope-
> fully. He always held that panic was the best means of survival; back
> in  the  olden days, his theory went, people faced with hungry sabre-
> toothed tigers could be divided very simply in those who panicked and
> those who stood there saying 'What a magnificent  brute!'  or  'Here,
> pussy.'                      - Terry Pratchett, _The Light Fantastic_