Pull request for efi-2021-10-rc4

Pull request for efi-2021-10-rc4

[Heinrich Schuchardt]

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

  include/efi_tcg2.h                |  4 +++-
  include/efi_variable.h            |  6 +++++-
  lib/efi_loader/efi_image_loader.c | 35 +++++++++++++++++++++++++------
  lib/efi_loader/efi_tcg2.c         | 21 ++++++++++++++++++-
  lib/efi_loader/efi_var_common.c   | 43
++++++++++++++++++++++++++++++---------
  lib/efi_loader/efi_var_file.c     | 41
++++++++++++++++++++++---------------
  lib/efi_loader/efi_variable.c     |  6 +++---
  7 files changed, 118 insertions(+), 38 deletions(-)

Re: Pull request for efi-2021-10-rc4

[Heinrich Schuchardt]

Hello Tom,

the doc patch was missing I have updated tag:

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 538c0f2d3798261161a28a05e445d0c85af56276:

   efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check
(2021-09-04 12:03:57 +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

Michal Simek (1):
       doc: Remove information about CAPSULE_FMP_HEADER

  doc/develop/uefi/uefi.rst         |  1 -
  include/efi_tcg2.h                |  4 +++-
  include/efi_variable.h            |  6 +++++-
  lib/efi_loader/efi_image_loader.c | 35 +++++++++++++++++++++++++------
  lib/efi_loader/efi_tcg2.c         | 21 ++++++++++++++++++-
  lib/efi_loader/efi_var_common.c   | 43
++++++++++++++++++++++++++++++---------
  lib/efi_loader/efi_var_file.c     | 41
++++++++++++++++++++++---------------
  lib/efi_loader/efi_variable.c     |  6 +++---
  8 files changed, 118 insertions(+), 39 deletions(-)

Re: Pull request for efi-2021-10-rc4

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]

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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Pull request for efi-2021-10-rc4

[Heinrich Schuchardt]

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+feuyRyWf0hNkf2triB4DR4UFBQ@mail.gmail.com/
>
>So at this point, are you asserting there is nothing to revert?
>
>-- 
>Tom

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.

Best regards

Heinrich

Re: Pull request for efi-2021-10-rc4

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]

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

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Pull request for efi-2021-10-rc4

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2728 bytes --]

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

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Pull request for efi-2021-10-rc4

[Heinrich Schuchardt]

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

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

Best regards

Heinrich 

Re: Pull request for efi-2021-10-rc4

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4145 bytes --]

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

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Pull request for efi-2021-10-rc4

[Heinrich Schuchardt]

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

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 

Re: Pull request for efi-2021-10-rc4

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5183 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Pull request for efi-2021-10-rc4

[Ilias Apalodimas]

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

Re: Pull request for efi-2021-10-rc4

[AKASHI Takahiro]

On Sat, Sep 04, 2021 at 11:08:28PM +0300, Ilias Apalodimas 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+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.
> > >
> > > 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.

But it won't prevent the other systems from using their device trees.

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

Different people have different requirements/assumptions on
different systems. It's also true for firmware update.
Some might like Ilias' approach, others are satisfied with device tree.

I think that what we need to do is, instead of having a single solution,
to allow users to decide which method, or even their own one,
they want to use on their systems. Right?

> 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

Surely it is system admin/integrators, not users, who care here.

-Takahiro Akashi

> 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

Re: Pull request for efi-2021-10-rc4

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

On Sat, Sep 04, 2021 at 12:10:07PM +0200, Heinrich Schuchardt wrote:

> Hello Tom,
> 
> the doc patch was missing I have updated tag:
> 
> 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 538c0f2d3798261161a28a05e445d0c85af56276:
> 
>   efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check
> (2021-09-04 12:03:57 +0200)
> 

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Pull request for efi-2021-10-rc4

[Simon Glass]

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

Re: Pull request for efi-2021-10-rc4

[Simon Glass]

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

Re: Pull request for efi-2021-10-rc4

[Heinrich Schuchardt]

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+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?

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

Re: Pull request for efi-2021-10-rc4

[Heinrich Schuchardt]

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

Instead I suggested to use an overlay tree to add our stuff on the
devicetree from the prior stage once U-Boot is invoked.

Best regards

Heinrich

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

Re: Pull request for efi-2021-10-rc4

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 9287 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Pull request for efi-2021-10-rc4

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 7558 bytes --]

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

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

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Pull request for efi-2021-10-rc4

[François Ozog]

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

Re: Pull request for efi-2021-10-rc4

[Simon Glass]

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

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

Re: Pull request for efi-2021-10-rc4

[Simon Glass]

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.

(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)

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

Re: Pull request for efi-2021-10-rc4

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 10079 bytes --]

On Thu, Sep 09, 2021 at 02:08:24PM -0600, Simon Glass wrote:
> 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+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.

Perhaps for u-boot,dm-pre-reloc and dm-spl we can figure out something
slightly clever?  I really had hoped that by now we might know enough
about what really is or isn't needed that early that we could
rule-of-thumb our way out of it or something.

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

If we need properties to exist in the device tree that we're provided
then it needs to be in the upstream device tree and bindings.  Is
u-boot,dm-spl valid and in a registered namespace?  Yes.  But if we
expect to be able to play nicely with the whole of the device tree
ecosystem then we need to document what we're doing, in the upstream
binding documentation so that validators can validate it.  If we expect
to be given and then pass on our device tree there's not another option
here.  Otherwise then yes, we are just borrowing a device tree from
someplace else and making it one-off.

That this hasn't come up before is at least partly because it's only
getting closer to the point where it seems like just maybe the OS has
sufficiently complete bindings that the thought of updating the device
tree infrequently is becoming feasible.

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

Maybe we'll need to have a higher level re-think then too, at least
looking forward, about what can and can't live where.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Pull request for efi-2021-10-rc4

[Mark Kettenis]

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

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

Re: Pull request for efi-2021-10-rc4

[François Ozog]

On Thu, 9 Sept 2021 at 23:22, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:

> > 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@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.
>
> 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.
>
+1
Let's also take the example of PSCI interface. This may be offered by
SCP firmware sitting on an external micro-controller or by OP-TEE
Trusted Application or intercepted by Qemu.
Let's assume the piece of code offering the PSCI interface change and
offer a new version of API.
Current situation in most boards: you have to sync up all projects (TFA,
OP-TEE, U-Boot, Linux) with that new information. The forward and backward
compatibilities are a problem.
Desired situation: a single authoritative entity (TFA? U-Boot?) is using a
platform specific way to sense the offered API and conduit and injects it
in the DT.

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


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

Re: Pull request for efi-2021-10-rc4

[François Ozog]

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


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