Pull request for efi-2021-10-rc4

Thu Sep 9 22:08:24 CEST 2021

Hi Tom,

On Thu, 9 Sept 2021 at 06:08, Tom Rini <trini at konsulko.com> wrote:
>
> On Thu, Sep 09, 2021 at 12:52:52PM +0200, Heinrich Schuchardt wrote:
> >
> >
> > On 9/9/21 10:56 AM, Simon Glass wrote:
> > > Hi Heinrich,
> > >
> > > On Sat, 4 Sept 2021 at 12:08, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > > >
> > > >
> > > >
> > > > Am 4. September 2021 19:39:49 MESZ schrieb Tom Rini <trini at konsulko.com>:
> > > > > On Sat, Sep 04, 2021 at 07:03:48PM +0200, Heinrich Schuchardt wrote:
> > > > > >
> > > > > >
> > > > > > Am 4. September 2021 16:37:22 MESZ schrieb Tom Rini <trini at konsulko.com>:
> > > > > > > On Sat, Sep 04, 2021 at 03:08:38PM +0200, Heinrich Schuchardt wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > Am 4. September 2021 15:01:11 MESZ schrieb Tom Rini <trini at konsulko.com>:
> > > > > > > > > On Sat, Sep 04, 2021 at 11:56:47AM +0200, Heinrich Schuchardt wrote:
> > > > > > > > >
> > > > > > > > > > Dear Tom,
> > > > > > > > > >
> > > > > > > > > > The following changes since commit 94509b79b13e69c209199af0757afbde8d2ebd6d:
> > > > > > > > > >
> > > > > > > > > >    btrfs: Use default subvolume as filesystem root (2021-09-01 10:11:24
> > > > > > > > > > -0400)
> > > > > > > > > >
> > > > > > > > > > are available in the Git repository at:
> > > > > > > > > >
> > > > > > > > > >    https://source.denx.de/u-boot/custodians/u-boot-efi.git
> > > > > > > > > > tags/efi-2021-10-rc4
> > > > > > > > > >
> > > > > > > > > > for you to fetch changes up to 1dfa494610c5469cc28cf1f8538abf4be6c00324:
> > > > > > > > > >
> > > > > > > > > >    efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check
> > > > > > > > > > (2021-09-04 09:15:09 +0200)
> > > > > > > > > >
> > > > > > > > > > ----------------------------------------------------------------
> > > > > > > > > > Pull request for efi-2021-10-rc4
> > > > > > > > > >
> > > > > > > > > > Documentation:
> > > > > > > > > >
> > > > > > > > > >      Remove invalid reference to configuration variable in UEFI doc
> > > > > > > > > >
> > > > > > > > > > UEFI:
> > > > > > > > > >
> > > > > > > > > >      Parameter checks for the EFI_TCG2_PROTOCOL
> > > > > > > > > >      Improve support of preseeding UEFI variables.
> > > > > > > > > >      Correct the calculation of the size of loaded images.
> > > > > > > > > >      Allow for UEFI images with zero VirtualSize
> > > > > > > > > >
> > > > > > > > > > ----------------------------------------------------------------
> > > > > > > > > > Heinrich Schuchardt (5):
> > > > > > > > > >        efi_loader: sections with zero VirtualSize
> > > > > > > > > >        efi_loader: rounding of image size
> > > > > > > > > >        efi_loader: don't load signature database from file
> > > > > > > > > >        efi_loader: efi_auth_var_type for AuditMode, DeployedMode
> > > > > > > > > >        efi_loader: correct determination of secure boot state
> > > > > > > > > >
> > > > > > > > > > Masahisa Kojima (3):
> > > > > > > > > >        efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api
> > > > > > > > > >        efi_loader: fix boot_service_capability_min calculation
> > > > > > > > > >        efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check
> > > > > > > > >
> > > > > > > > > And I don't see Simon's revert in here either.  And he asked you about
> > > > > > > > > that yesterday:
> > > > > > > > > https://lore.kernel.org/r/CAPnjgZ3eRdjF0jb9S-cJK6y+feuyRyWf0hNkf2triB4DR4UFBQ@mail.gmail.com/
> > > > > > > > >
> > > > > > > > > So at this point, are you asserting there is nothing to revert?
> > > > > > > >
> > > > > > > > Never. Simons "revert" is breaking functionality. The concept for suporting blobs in devicetrees supplied by a prior bootstage has not been defined yet.
> > > > > > >
> > > > > > > And to be clearer, reverting something that was introduced in one rc in
> > > > > > > a later rc isn't breaking functionality.  U-Boot releases (well, the
> > > > > > > non-rc ones for sure) are on a very regular schedule.  External projects
> > > > > > > may not depend on some feature introduced at -rcN unless they're willing
> > > > > > > to accept that some changes could happen before release.
> > > > > > >
> > > > > >
> > > > > > There is no value delivered by Simon's series. Neither does the image get smaller nor does it fix anything. If he wants to enforce a design, it must work for all use cases. But this requires some conceptual work.
> > > > >
> > > > > Yes, and what's the rush to not do the conceptual work?  If I recall
> > > > > part of the thread correctly, yes, Simon didn't get his objections in
> > > > > before the patches were merged, but it was early enough in the release
> > > > > cycle that taking a step back and reverting was a reasonable request.
> > > > > What he had said wouldn't have changed if he had gotten the email out a
> > > > > few days earlier.
> > > > >
> > > > > So yes, please merge Simon's revert, or post and merge new more minimal
> > > > > revert that brings things to the same functional end.  There are
> > > > > objections to this implementation, and thus far Simon has been
> > > > > responding all of the requests to better clarify all of the related code
> > > > > and concepts that have been asked of him, so that in the end an
> > > > > implementation that fulfills all of the technical requirements can be
> > > > > created, that hopefully leaves all parties satisfied.
> > > > >
> > > >
> > > >
> > > > There is nothing wrong with the current code.
> > >
> > > The current code is misconceived and I did go to great effort to
> > > explain that in the 'devicetree' series.
> > >
> > > >
> > > > It is Simon's concept of blobs in devicetrees that is borked in that it ignores QEMU and any board that gets the DT from a prior boot stage.
> > >
> > > That's because I was completely unaware of this strange back-door
> > > approach. It would have helped a lot if someone had bothered to create
> >
> > CONFIG_OF_PRIOR_STAGE is not a backdoor. And you are the DM maintainer.
> >
> > > some documentation for the design. Then I would have seen the problem
> > > immediately.
> > >
> > > Anyway, it is now covered by the above series. The use of devicetree
> > > in U-Boot is very clear, I think.
> >
> > I see neither a design by you nor a series that covers
> > CONFIG_OF_PRIOR_STAGE. I have suggested that if you really want to move
> > this blob to a devicetree you could apply an overlay tree including
> > U-Boot specific fields to the devicetree of the prior stage. Did you yet
> > respond to this?
>
> Given that I feel like "u-boot,dm-spl" and "u-boot,dm-pre-reloc" are
> unlikely to survive upstream review, I would like to hear what you think
> about applying overlays, at least in general, Simon.

I would be pretty disappointed if vendor,propname could not survive
upstream review, given that it is in the DT spec explicitly, and that
linux, is used in Linux.

To answer your question, I think it is a terrible idea and would lead
to much pain and misery and eventually the failure of U-Boot to
function as a useful and efficient bootloader . It moves something
that I think can be easily accomplished (from a technical POV anyway)
at built-time into the run-time domain. Leaving aside that devicetree
overlays are arguably not supported/implemented so far as the DT spec
is concerned, it adds overhead to SPL and U-Boot (particularly
pre-reloc) that is likely to make the whole thing infeasible.

Also, somewhat off-topic, this is the first I have ever heard of the
idea of U-Boot needing to put its properties in a separate place. I
tried to cover this in 'Why not have two devicetrees?' here:

https://patchwork.ozlabs.org/project/uboot/patch/20210828164630.81050-4-sjg@chromium.org/

How hard do we really want to make this? If a DT is provided to
U-Boot, it needs to be suitable for U-Boot.

The whole idea is driven from a misconception that U-Boot is just
borrowing a devicetree from Linux or qemu or TF-A.

So to the extent that this implies that U-Boot cannot have its own
properties and nodes in the devicetree;

No.

U-Boot uses the devicetree for its configuration and it must be
supplied based on U-Boot's requirements. I will try to send another
version of the devicetree doc series.

>
> > > > Simon's patches have no functional end. So what do you mean by "same functional end"?
> > >
> > > So, please, again, will someone apply the revert before the release
> > > and people start relying on it.
> >
> > Sure we want capsule updates. My understanding is that you want to
> > modify the current implementation. That is fine. It has no effect on the
> > user where a blob is stored unless you remove it as your series does. So
> > I cannot understand your urgency. Instead collaborate with Ilias and me
> > on the possible design change.
>
> I believe Simon's urgency comes from the idea that once we put this
> method in a release we'll be stuck with using it or supporting it
> forever.  That's certainly a general concern of mine.  We can make
> changes between -rc1 and release and if something else decides to depend
> on an ABI we changed in the middle there, that was a bad idea on their
> part.  But then it's on us once it's in a full release.

That's exactly it. I am particularly concerned since there does not
seem to even be agreement (from Heinrich at least) in how U-Boot uses
device tree *today*, some of which is apparently a bug to be fixed :-)

Regards,
Simon