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@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+feuyRyWf0hNkf2triB4DR4UFB...

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.

See the discussion on the ML.

Yes, I have been following the discussion, which is why I'm wondering why there's still not been a revert in your tree, to bring things back to the state of the previous release, until everyone is in something closer to agreement about how it should be handled moving forward.

Am 4. September 2021 16:37:22 MESZ schrieb Tom Rini trini@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@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+feuyRyWf0hNkf2triB4DR4UFB...

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.

We already have other blobs that are not in decicetrees and Simon never complained. So why picking on this blob?

Best regards

Heinrich

Am 4. September 2021 19:39:49 MESZ schrieb Tom Rini trini@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@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@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+feuyRyWf0hNkf2triB4DR4UFB...

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.

Simon's patches have no functional end. So what do you mean by "same functional end"?

Best regards

Heinrich

Hi Tom,

On Sat, 4 Sept 2021 at 21:08, Tom Rini trini@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 trini@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@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@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+feuyRyWf0hNkf2triB4DR4UFB... > >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.

Regards /Ilias

Hi Ilias,

On Sat, 4 Sept 2021 at 14:09, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

Hi Tom,

On Sat, 4 Sept 2021 at 21:08, Tom Rini trini@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 trini@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@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@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+feuyRyWf0hNkf2triB4DR4UFB... > > > >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.

https://patchwork.ozlabs.org/project/uboot/list/?series=260032&state=*

So what has happened here is a short-cut and not the correct approach. It needs to be reverted before the release, since otherwise people will rely on it and we will be stuck with two ways to do signatures.

Regards, Simon

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.apalodimas@linaro.org wrote:

...

Hi Tom,

On Sat, 4 Sept 2021 at 21:08, Tom Rini trini@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 trini@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@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@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+feuyRyWf0hNkf2triB4DR4UFB... > > > > > > > > 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.

On 9/9/21 10:56 AM, Simon Glass wrote:

Hi Heinrich,

On Sat, 4 Sept 2021 at 12:08, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

Am 4. September 2021 19:39:49 MESZ schrieb Tom Rini trini@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@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@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+feuyRyWf0hNkf2triB4DR4UFB... > > 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?

...

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.

Heinrich

Regards, Simon

Hi Tom,

On Thu, 9 Sept 2021 at 06:08, Tom Rini trini@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@gmx.de wrote:

...

Am 4. September 2021 19:39:49 MESZ schrieb Tom Rini trini@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@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@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+feuyRyWf0hNkf2triB4DR4UFB... > > > > > > 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@...

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.

No.

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

From: Simon Glass sjg@chromium.org Date: Thu, 9 Sep 2021 14:08:24 -0600

Hi Simon,

Hi Tom,

On Thu, 9 Sept 2021 at 06:08, Tom Rini trini@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@gmx.de wrote:

...

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.

Well, the Linux DT maintainers tend to be pretty thorough in their review and will resist crappy ideas ;). I think there is merit in the way we currently do things by augmenting the standard Linux device tree with these properties. At least on platforms where U-Boot is the first bit of firmware that runs (apart from ROM code). But I suspect there would be some resistence if we proposed to add these properties directly to the upstream Linux DTs instead.

On the other hand I don't see why maintainers of firmware that runs before U-Boot that provides a DT to U-Boot through a CONFIG_OF_PRIOR_STAGE mechanism would never add "u-boot," properties to their device trees if they use U-Boot as a 2nd stage loader. I'm sure the device trees providied by m1n1 for the Apple M1 could contain additional nodes and "u-boot," properties if necessary.

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.

I still think that U-Boot TPL/SPL isn't an issue for platforms that use CONFIG_OF_PRIOR_STAGE. The QEMU example that was given is a bit artificial in that it is a configuration specifically designed for testing U-Boot SPL. Folks using a QEMU-based virtualization solution don't care about that configuration and will only use U-Boot proper.

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

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.

But U-Boot must not make unreasonable demands.

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

I diasagree that's a misconception. I've already explained why it isn't practical for U-Boot to have hardwired device trees for things like QEMU, Raspberry Pi or Apple M1. I also don't see what specific U-Boot cofiguration is needed for those platforms. Currently U-Boot functions just fine with on these platforms without having any "u-boot," properties in the DT they provide to U-Boot. Granted, I'm not trying to do any sort of verified/secure boot on those platforms. But any meaningful verified/secure boot implementation needs a high degree of agreement between the integrated firmware components anyway.

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

No.

No.

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.

I really think this needs to be judged on a case-by-case basis. Using this as a leading principle is fine, but ruling out binary data linked into the u-boot binary itself out of principle if embedding that data into the device tree isn't practical seems counterproductive to me.

Cheers,

Mark

Hi

I think it may be good to step back and talk about the why's rather the how's. (some elements were discussed in DeviceTree Evolution technical report https://docs.google.com/document/d/1CLkhLRaz_zcCq44DLGmPZQFPbYHOC6nzPowaL0XmRk0/edit?usp=sharing )

We are witnessing changes in the embedded products value chain when it is not controlled in an integrated vertical market (such as as Android for phones or Chromebooks). End-product builders (car makers, robot makers...) are willing to avoid paying over and over for the same things. One identified solution for that is to implement clear boundaries of responsibility between different supply chain providers.

This led to the creation of Arm SystemReady: a contract between firmware and OS. SystemReady defines UEFI *interface* (not to be confused with EDK2) as the functional aspects and either ACPI or DT for the hardware description. At present three non-secure open source firmware projects are targeted for SysteReady implementation: EDK2, U-Boot and LinuxBoot. SystemReady states that the hardware description is a "given" by the platform: the OS does *not* come with the one it wants to use. I read adoption by RISC V community of the EBBR recipe of SystemReady-IR to signal we are addressing an important and concrete market need.

To identify the consequences on the firmware world, let's drill down into what it means for 4 identical boards that differ from the amount of soldiered memory.

The current practice is often to maintain TFA, OP-TEE, U-Boot and even Linux with that information. This parallel maintenance can be as simple as a single line of code (a #define in TFA a single) or a whole structure of the memory map and assumptions crippled throughout the U-Boot code. The good things is that it avoids need for communication between firmware elements. But if we assume that the tier1 integrator is dealing with U-Boot and the board vendor provides TF-A and OP-TEE, it makes Tier 1 integrator life complicated to also deal with the memory size aspect. Another example of the problems of parallel maintenance: TrustZone is often used by the SoC/board vendor to stick platform code. But car vendors want to stick in DRM, digital wallets, insurance company, rental company trusted applications. In that case, the product maker should be able to easily control TrustZone parameters. That would be greatly facilitated by having to change only one parameter in one project. Looking at a particular case, it has been awfully difficult to update just the secure memory size: single line of codes in TFA, U-Boot code and structures had to be changed to accommodate a secure memory size change.

So if you stop thinking one single entity controls all aspects of firmware (secure, non-secure, and beyond) as well as operating systems, there are a number of things to do. And the majority of Linaro patches are delivering on making this happen.

I don't say that all projects should follow that. I say that there must be a scheme to properly address SystemReady defined DT lifecycle because it is addressing a major market demand (yes not universal but major).

Other considerations: - There may be co-existing alternate schemes such as VBE or Android Verified Boot in U-Boot. SystemReady is not the one that has to be used all the time. It serves a big market but not all markets. - DeviceTree abstract language can be used anywhere as a similar technology as protobufs: to represent complex information. For example I see great benefits in using this in blob lists But the lifecycle and content of the one DTB that will reach the OS must be handled as part of the firmware to OS ABI. - The DTB lifecycle will be the same in U-Boot and LinuxBoot so that ultimately product vendors may simply choose which non-secure firmware they want. It means that information passing between firmware components (call it HOBs or blob lists) should be standardized across non-secure firmwares. - U-Boot private configuration have no room in such an OS DTB (the one handover by firmware to OS). If U-Boot wants to use DTB format to store some variables that are stored in the U-Boot environment file today: perfect, but that's a separate object from OS DTB. Should U-Boot want to inject some elements through fixups in the OS DTB, fine providing that there is a standardized binding for that. - There may be questions on some elements. For instance inventory information such as part numbers, board name... Some are defined in DT, some are also defined in SMBIOS, some are defined only in SMBIOS. Based on SystemReady compliance and distro tests field feedback, it sounds like we will need to revisit this and be clearer and more exhaustive in how we deal with those parameters. Regardless of that, one need to think of who is providing the information (say the serial number) so that signing firmware is not making the process stiff and complicated (simple for an integrated vertical market, complex in multiparty value chain).

On Thu, 9 Sept 2021 at 10:57, Simon Glass sjg@chromium.org wrote:

Hi Heinrich,

On Sat, 4 Sept 2021 at 12:08, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

Am 4. September 2021 19:39:49 MESZ schrieb Tom Rini <trini@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@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@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+feuyRyWf0hNkf2triB4DR4UFB...

...

...

...

...

...

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

...

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.

Regards, Simon

Hi Simon,

On Thu, 9 Sept 2021 at 22:08, Simon Glass sjg@chromium.org wrote:

Hi François,

On Thu, 9 Sept 2021 at 06:17, François Ozog francois.ozog@linaro.org wrote:

...

Hi

I think it may be good to step back and talk about the why's rather the

how's. (some elements were discussed in DeviceTree Evolution technical report)

...

We are witnessing changes in the embedded products value chain when it

is not controlled in an integrated vertical market (such as as Android for phones or Chromebooks). End-product builders (car makers, robot makers...) are willing to avoid paying over and over for the same things. One identified solution for that is to implement clear boundaries of responsibility between different supply chain providers.

...

This led to the creation of Arm SystemReady: a contract between

firmware and OS. SystemReady defines UEFI *interface* (not to be confused with EDK2) as the functional aspects and either ACPI or DT for the hardware description. At present three non-secure open source firmware projects are targeted for SysteReady implementation: EDK2, U-Boot and LinuxBoot. SystemReady states that the hardware description is a "given" by the platform: the OS does *not* come with the one it wants to use. I read adoption by RISC V community of the EBBR recipe of SystemReady-IR to signal we are addressing an important and concrete market need.

...

To identify the consequences on the firmware world, let's drill down

into what it means for 4 identical boards that differ from the amount of soldiered memory.

...

The current practice is often to maintain TFA, OP-TEE, U-Boot and even

Linux with that information. This parallel maintenance can be as simple as a single line of code (a #define in TFA a single) or a whole structure of the memory map and assumptions crippled throughout the U-Boot code. The good things is that it avoids need for communication between firmware elements. But if we assume that the tier1 integrator is dealing with U-Boot and the board vendor provides TF-A and OP-TEE, it makes Tier 1 integrator life complicated to also deal with the memory size aspect. Another example of the problems of parallel maintenance: TrustZone is often used by the SoC/board vendor to stick platform code. But car vendors want to stick in DRM, digital wallets, insurance company, rental company trusted applications. In that case, the product maker should be able to easily control TrustZone parameters. That would be greatly facilitated by having to change only one parameter in one project. Looking at a particular case, it has been awfully difficult to update just the secure memory size: single line of codes in TFA, U-Boot code and structures had to be changed to accommodate a secure memory size change.

...

So if you stop thinking one single entity controls all aspects of

firmware (secure, non-secure, and beyond) as well as operating systems, there are a number of things to do. And the majority of Linaro patches are delivering on making this happen.

...

I don't say that all projects should follow that. I say that there must

be a scheme to properly address SystemReady defined DT lifecycle because it is addressing a major market demand (yes not universal but major).

...

Other considerations:

• There may be co-existing alternate schemes such as VBE or Android

Verified Boot in U-Boot. SystemReady is not the one that has to be used all the time. It serves a big market but not all markets.

...

• DeviceTree abstract language can be used anywhere as a similar

technology as protobufs: to represent complex information. For example I see great benefits in using this in blob lists But the lifecycle and content of the one DTB that will reach the OS must be handled as part of the firmware to OS ABI.

...

• The DTB lifecycle will be the same in U-Boot and LinuxBoot so that

ultimately product vendors may simply choose which non-secure firmware they want. It means that information passing between firmware components (call it HOBs or blob lists) should be standardized across non-secure firmwares.

To this point I think I agree, so far as I understand it.

(please don't use protobuf in firmware...it requires code generation)

...

• U-Boot private configuration have no room in such an OS DTB (the one

handover by firmware to OS). If U-Boot wants to use DTB format to store some variables that are stored in the U-Boot environment file today: perfect, but that's a separate object from OS DTB. Should U-Boot want to inject some elements through fixups in the OS DTB, fine providing that there is a standardized binding for that. - There may be questions on some elements. For instance inventory information such as part numbers, board name... Some are defined in DT, some are also defined in SMBIOS, some are defined only in SMBIOS. Based on SystemReady compliance and distro tests field feedback, it sounds like we will need to revisit this and be clearer and more exhaustive in how we deal with those parameters. Regardless of that, one need to think of who is providing the information (say the serial number) so that signing firmware is not making the process stiff and complicated (simple for an integrated vertical market, complex in multiparty value chain).

Here you are, with respect and in my opinion, going off into the weeds.

U-Boot *absolutely requires* some properties and nodes in order to function correctly. In some special cases, this can be avoided, but in general, it is required. Examples are the u-boot,dm- tags and the binman image definition, The current 'config' node is there also.

Without them you cannot make use of the available U-Boot features. I now see that this point is the root of much of the confusion and misunderstanding of the past month or so. We must resolve it and put it to bed.

I think I understand your point and still view things differently. The DT passed to the OS shall essentially be a downstream information passing construct (from TF-A to U-Boot, from U-Boot to TFA). Should U-Boot need information from Linux (what you describe above) then it is about downstream to upstream and that is configuration information. The configuration information storage is irrelevant to the OS. So I would place that in either U-Boot environment or U-Boot private DT. In EDK2 or LinuxBoot, the information you refer to is kept separate from ACPI table or DT.

(Honestly I cannot fathom why we would want to use SMBIOS in this day and age :-) but that does not relate to the point at issue here)

(Well, I admit I am not a SMBIOS fan either....if we could forget entirely that would be great and I would prefer the OS guys to introduce proper abstraction to get the information rather than just being a passive passthrough for SMBIOS related information).

Regards, Simon

...

On Thu, 9 Sept 2021 at 10:57, Simon Glass sjg@chromium.org wrote:

...

Hi Heinrich,

On Sat, 4 Sept 2021 at 12:08, Heinrich Schuchardt xypron.glpk@gmx.de

wrote:

...

...

...

Am 4. September 2021 19:39:49 MESZ schrieb Tom Rini <

trini@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@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@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+feuyRyWf0hNkf2triB4DR4UFB...

...

...

...

...

...

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

...

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.

Regards, Simon

-- François-Frédéric Ozog | Director Business Development T: +33.67221.6485 francois.ozog@linaro.org | Skype: ffozog