On Thu, Sep 09, 2021 at 01:21:17PM +0200, Heinrich Schuchardt wrote: > > > On 9/9/21 10:56 AM, Simon Glass wrote: > > Hi Ilias, > > > > On Sat, 4 Sept 2021 at 14:09, Ilias Apalodimas > > <ilias.apalodi...@linaro.org> wrote: > > > > > > Hi Tom, > > > > > > On Sat, 4 Sept 2021 at 21:08, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > On Sat, Sep 04, 2021 at 08:02:49PM +0200, Heinrich Schuchardt wrote: > > > > > > > > > > > > > > > Am 4. September 2021 19:39:49 MESZ schrieb Tom Rini > > > > > <tr...@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 > > > > > > > <tr...@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 > > > > > > > > > <tr...@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+feuyrywf0hnkf2trib4dr4u...@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. > > > > > > > > > > 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. > > > > > > > > Then it should be pretty easy to get Simon to withdraw his objections, > > > > if there's such a fundamental "this is the only possible way, no > > > > changes" path forward. > > > > > > > > > Simon's patches have no functional end. So what do you mean by "same > > > > > functional end"? > > > > > > > > I mean the state of the EFI subsystem, prior to the code in question > > > > being merged, without breaking the other assorted EFI changes that have > > > > come in since then. > > > > > > > > -- > > > > Tom > > > I'll sum this up since there's many emails on the topic. > > > > > > The current changes move the public needed for capsule updates in > > > U-Boot's .rodata section. > > > When I sent this, I assumed u-boot was mapping .rodata as read only. > > > Since it doesn't the protection I was hoping for isn't there. So > > > security wise the two different proposals are on par. Arguably it's > > > easier to fix .rodata instead of copying the key from the dtb and > > > switching the pages to RO, but that's really minor. > > > > > > However keeping the key on the DTB has some of limitations, with the > > > most notable being that you *must* only use CONFIG_OF_SEPARATE for > > > your DTB, while there's four different ways available in the Kconfig > > > (and 3 are usable on production). I've repeated enough times, that I > > > don't mind changing the code and keeping the key on the DTB, as long > > > as the limitations are lifted. If that means reverting the patch now > > > and fixing it in the future, I am fine with that as well. > > > To be honest I don't understand why this has to be set in stone. Even > > > if we keep the current patchset and change it to the dtb in the > > > future, that will have zero effect on the users. Once they upgrade to > > > the newer, shinier version, their key will just be read from a > > > different location, but that's all hidden from the user. The only they > > > will have to change, is how they include that key on the final binary. > > > > This is a reasonable summary I think. > > > > The devicetree series I sent explains how to deal with the limitations > > here. Heinrich requested that I clarify this which I did. I fully > > expected that the revert would then be applied. But instead it seems > > there is not even agreement about the status quo (of use of devicetree > > in U-Boot). > > > > OF_SEPARATE is used by the vast majority of boards, including most > > qemu builds. I think the OF_BOARD thing should probably be deleted. > > The OF_EMBED thing should not be used in production. It is needed with > > the EFI app though and I recently sent a series to support updating > > the DT there. > > > > For OF_PRIOR_STAGE the prior state is responsible for supplying the > > DT, and needs to do so and meet U-Boot's requirements. I have clearly > > set that out in the devicetree patch. > > Yes, and this was wrong. You cannot impose U-Boot's requirements on > other projects.
This is true IF AND ONLY IF the requirements are not in a documented, reviewed and submitted binding. U-Boot is no more, but also no less, special in this regard. -- Tom
signature.asc
Description: PGP signature