[PATCH v2 0/9] Bug-fixes for a few boards

[PATCH v2 0/9] Bug-fixes for a few boards

[Simon Glass]

This series includes fixes to get some rockchip and nvidia boards
working again. It also drops the broken Beaglebone Black config and
provides a devicetree fix for coral (x86).

Changes in v2:
- Put the conditions under EFI_TCG2_PROTOCOL
- Consider MEASURED_BOOT too
- Remove the superfluous if() and drop the debug() as well
- Use 'phase' instead of 'stage'
- Add new patch to correct memory size in SPL
- Drop patch "regulator: rk8xx: Fix incorrect parameter"
- Rewrite boneblack patch to onstead drop the target and update docs

Simon Glass (9):
  nvidia: nyan-big: Disable debug UART
  tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL
  rockchip: veyron: Add logging for power init
  power: regulator: Handle autoset in regulators_enable_boot_on()
  fdt: Correct condition for bloblist existing
  spl: Allow ATF to work when dcache is disabled
  rockchip: Ensure memory size is available in RK3399 SPL
  rockchip: bob: kevin: Disable dcache in SPL
  Drop the special am335x_boneblack_vboot target

 board/google/veyron/veyron.c               | 30 +++----
 board/ti/am335x/MAINTAINERS                |  1 -
 boot/Kconfig                               |  4 +
 common/spl/spl_atf.c                       |  3 +-
 configs/am335x_boneblack_vboot_defconfig   | 94 ----------------------
 configs/am335x_evm_defconfig               |  3 +-
 configs/chromebook_bob_defconfig           |  1 +
 configs/chromebook_kevin_defconfig         |  1 +
 configs/nyan-big_defconfig                 |  1 -
 doc/usage/fit/beaglebone_vboot.rst         | 21 +++--
 drivers/power/regulator/regulator-uclass.c |  2 +-
 drivers/ram/rockchip/sdram_rk3399.c        | 49 ++++++-----
 lib/Kconfig                                |  4 -
 lib/fdtdec.c                               | 12 ++-
 14 files changed, 70 insertions(+), 156 deletions(-)
 delete mode 100644 configs/am335x_boneblack_vboot_defconfig

-- 
2.34.1

[PATCH v2 1/9] nvidia: nyan-big: Disable debug UART

[Simon Glass]

This cannot be enabled early in boot since some other init is needed.
At this point it is unclear exactly what init is needed, so disable
the debug UART to avoid a hang.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 configs/nyan-big_defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configs/nyan-big_defconfig b/configs/nyan-big_defconfig
index 1483d17d975..4dec710cf8d 100644
--- a/configs/nyan-big_defconfig
+++ b/configs/nyan-big_defconfig
@@ -17,7 +17,6 @@ CONFIG_TEGRA124=y
 CONFIG_TARGET_NYAN_BIG=y
 CONFIG_TEGRA_GPU=y
 CONFIG_SYS_LOAD_ADDR=0x82408000
-CONFIG_DEBUG_UART=y
 CONFIG_FIT=y
 CONFIG_FIT_BEST_MATCH=y
 CONFIG_BOOTSTAGE=y
-- 
2.34.1

[PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Simon Glass]

It does not make sense to enable all SHA algorithms unless they are
needed. It bloats the code and in this case, causes chromebook_link to
fail to build. That board does use the TPM, but not with measured boot,
nor EFI.

Since EFI_TCG2_PROTOCOL already selects these options, we just need to
add them to MEASURED_BOOT as well.

Note that the original commit combines refactoring and new features,
which makes it hard to see what is going on.

Fixes: 97707f12fda tpm: Support boot measurements
Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Put the conditions under EFI_TCG2_PROTOCOL
- Consider MEASURED_BOOT too

 boot/Kconfig | 4 ++++
 lib/Kconfig  | 4 ----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/boot/Kconfig b/boot/Kconfig
index 6f3096c15a6..b061891e109 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
 config MEASURED_BOOT
 	bool "Measure boot images and configuration when booting without EFI"
 	depends on HASH && TPM_V2
+	select SHA1
+	select SHA256
+	select SHA384
+	select SHA512
 	help
 	  This option enables measurement of the boot process when booting
 	  without UEFI . Measurement involves creating cryptographic hashes
diff --git a/lib/Kconfig b/lib/Kconfig
index 189e6eb31aa..568892fce44 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -438,10 +438,6 @@ config TPM
 	bool "Trusted Platform Module (TPM) Support"
 	depends on DM
 	imply DM_RNG
-	select SHA1
-	select SHA256
-	select SHA384
-	select SHA512
 	help
 	  This enables support for TPMs which can be used to provide security
 	  features for your board. The TPM can be connected via LPC or I2C
-- 
2.34.1

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Ilias Apalodimas]

Hi Simon,

On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
>
> It does not make sense to enable all SHA algorithms unless they are
> needed. It bloats the code and in this case, causes chromebook_link to
> fail to build. That board does use the TPM, but not with measured boot,
> nor EFI.
>
> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> add them to MEASURED_BOOT as well.
>
> Note that the original commit combines refactoring and new features,
> which makes it hard to see what is going on.
>
> Fixes: 97707f12fda tpm: Support boot measurements
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v2:
> - Put the conditions under EFI_TCG2_PROTOCOL
> - Consider MEASURED_BOOT too
>
>  boot/Kconfig | 4 ++++
>  lib/Kconfig  | 4 ----
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/boot/Kconfig b/boot/Kconfig
> index 6f3096c15a6..b061891e109 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
>  config MEASURED_BOOT
>         bool "Measure boot images and configuration when booting without EFI"
>         depends on HASH && TPM_V2
> +       select SHA1
> +       select SHA256
> +       select SHA384
> +       select SHA512
>         help
>           This option enables measurement of the boot process when booting
>           without UEFI . Measurement involves creating cryptographic hashes
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 189e6eb31aa..568892fce44 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -438,10 +438,6 @@ config TPM
>         bool "Trusted Platform Module (TPM) Support"
>         depends on DM
>         imply DM_RNG
> -       select SHA1
> -       select SHA256
> -       select SHA384
> -       select SHA512

I am not sure this is the right way to deal with your problem.
The TPM main functionality is to measure and extend PCRs, so shaXXXX
is really required. To make things even worse, you don't know the PCR
banks that are enabled beforehand. This is a runtime config of the
TPM.

 So this would make the TPM pretty useless. Can't you remove something
that doesn't break functionality?

Thanks
/Ilias
>         help
>           This enables support for TPMs which can be used to provide security
>           features for your board. The TPM can be connected via LPC or I2C
> --
> 2.34.1
>

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Heinrich Schuchardt]

On 6/14/24 08:03, Ilias Apalodimas wrote:
> Hi Simon,
>
> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
>>
>> It does not make sense to enable all SHA algorithms unless they are
>> needed. It bloats the code and in this case, causes chromebook_link to
>> fail to build. That board does use the TPM, but not with measured boot,
>> nor EFI.
>>
>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
>> add them to MEASURED_BOOT as well.
>>
>> Note that the original commit combines refactoring and new features,
>> which makes it hard to see what is going on.
>>
>> Fixes: 97707f12fda tpm: Support boot measurements
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v2:
>> - Put the conditions under EFI_TCG2_PROTOCOL
>> - Consider MEASURED_BOOT too
>>
>>   boot/Kconfig | 4 ++++
>>   lib/Kconfig  | 4 ----
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/boot/Kconfig b/boot/Kconfig
>> index 6f3096c15a6..b061891e109 100644
>> --- a/boot/Kconfig
>> +++ b/boot/Kconfig
>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
>>   config MEASURED_BOOT
>>          bool "Measure boot images and configuration when booting without EFI"
>>          depends on HASH && TPM_V2
>> +       select SHA1
>> +       select SHA256
>> +       select SHA384
>> +       select SHA512
>>          help
>>            This option enables measurement of the boot process when booting
>>            without UEFI . Measurement involves creating cryptographic hashes
>> diff --git a/lib/Kconfig b/lib/Kconfig
>> index 189e6eb31aa..568892fce44 100644
>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -438,10 +438,6 @@ config TPM
>>          bool "Trusted Platform Module (TPM) Support"
>>          depends on DM
>>          imply DM_RNG
>> -       select SHA1
>> -       select SHA256
>> -       select SHA384
>> -       select SHA512
>
> I am not sure this is the right way to deal with your problem.
> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> is really required. To make things even worse, you don't know the PCR
> banks that are enabled beforehand. This is a runtime config of the
> TPM.

If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
cannot extend PCRs. So it seems fine to let these two select the
complete set of hashing algorithms. As Simon pointed out for
EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.

Even if U-Boot does not support measured boot (EFI or non-EFI) we might
still be using the TPMs RNG.

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

>
>   So this would make the TPM pretty useless. Can't you remove something
> that doesn't break functionality?
>
> Thanks
> /Ilias
>>          help
>>            This enables support for TPMs which can be used to provide security
>>            features for your board. The TPM can be connected via LPC or I2C
>> --
>> 2.34.1
>>

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Ilias Apalodimas]

On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 6/14/24 08:03, Ilias Apalodimas wrote:
> > Hi Simon,
> >
> > On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> >>
> >> It does not make sense to enable all SHA algorithms unless they are
> >> needed. It bloats the code and in this case, causes chromebook_link to
> >> fail to build. That board does use the TPM, but not with measured boot,
> >> nor EFI.
> >>
> >> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> >> add them to MEASURED_BOOT as well.
> >>
> >> Note that the original commit combines refactoring and new features,
> >> which makes it hard to see what is going on.
> >>
> >> Fixes: 97707f12fda tpm: Support boot measurements
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >> ---
> >>
> >> Changes in v2:
> >> - Put the conditions under EFI_TCG2_PROTOCOL
> >> - Consider MEASURED_BOOT too
> >>
> >>   boot/Kconfig | 4 ++++
> >>   lib/Kconfig  | 4 ----
> >>   2 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/boot/Kconfig b/boot/Kconfig
> >> index 6f3096c15a6..b061891e109 100644
> >> --- a/boot/Kconfig
> >> +++ b/boot/Kconfig
> >> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
> >>   config MEASURED_BOOT
> >>          bool "Measure boot images and configuration when booting without EFI"
> >>          depends on HASH && TPM_V2
> >> +       select SHA1
> >> +       select SHA256
> >> +       select SHA384
> >> +       select SHA512
> >>          help
> >>            This option enables measurement of the boot process when booting
> >>            without UEFI . Measurement involves creating cryptographic hashes
> >> diff --git a/lib/Kconfig b/lib/Kconfig
> >> index 189e6eb31aa..568892fce44 100644
> >> --- a/lib/Kconfig
> >> +++ b/lib/Kconfig
> >> @@ -438,10 +438,6 @@ config TPM
> >>          bool "Trusted Platform Module (TPM) Support"
> >>          depends on DM
> >>          imply DM_RNG
> >> -       select SHA1
> >> -       select SHA256
> >> -       select SHA384
> >> -       select SHA512
> >
> > I am not sure this is the right way to deal with your problem.
> > The TPM main functionality is to measure and extend PCRs, so shaXXXX
> > is really required. To make things even worse, you don't know the PCR
> > banks that are enabled beforehand. This is a runtime config of the
> > TPM.
>
> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> cannot extend PCRs. So it seems fine to let these two select the
> complete set of hashing algorithms. As Simon pointed out for
> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.

It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
0xb0000000

Regards
/Ilias
>
> Even if U-Boot does not support measured boot (EFI or non-EFI) we might
> still be using the TPMs RNG.
>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>
> >
> >   So this would make the TPM pretty useless. Can't you remove something
> > that doesn't break functionality?
> >
> > Thanks
> > /Ilias
> >>          help
> >>            This enables support for TPMs which can be used to provide security
> >>            features for your board. The TPM can be connected via LPC or I2C
> >> --
> >> 2.34.1
> >>
>

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Heinrich Schuchardt]

On 14.06.24 09:01, Ilias Apalodimas wrote:
> On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 6/14/24 08:03, Ilias Apalodimas wrote:
>>> Hi Simon,
>>>
>>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
>>>>
>>>> It does not make sense to enable all SHA algorithms unless they are
>>>> needed. It bloats the code and in this case, causes chromebook_link to
>>>> fail to build. That board does use the TPM, but not with measured boot,
>>>> nor EFI.
>>>>
>>>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
>>>> add them to MEASURED_BOOT as well.
>>>>
>>>> Note that the original commit combines refactoring and new features,
>>>> which makes it hard to see what is going on.
>>>>
>>>> Fixes: 97707f12fda tpm: Support boot measurements
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Put the conditions under EFI_TCG2_PROTOCOL
>>>> - Consider MEASURED_BOOT too
>>>>
>>>>    boot/Kconfig | 4 ++++
>>>>    lib/Kconfig  | 4 ----
>>>>    2 files changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/boot/Kconfig b/boot/Kconfig
>>>> index 6f3096c15a6..b061891e109 100644
>>>> --- a/boot/Kconfig
>>>> +++ b/boot/Kconfig
>>>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
>>>>    config MEASURED_BOOT
>>>>           bool "Measure boot images and configuration when booting without EFI"
>>>>           depends on HASH && TPM_V2
>>>> +       select SHA1
>>>> +       select SHA256
>>>> +       select SHA384
>>>> +       select SHA512
>>>>           help
>>>>             This option enables measurement of the boot process when booting
>>>>             without UEFI . Measurement involves creating cryptographic hashes
>>>> diff --git a/lib/Kconfig b/lib/Kconfig
>>>> index 189e6eb31aa..568892fce44 100644
>>>> --- a/lib/Kconfig
>>>> +++ b/lib/Kconfig
>>>> @@ -438,10 +438,6 @@ config TPM
>>>>           bool "Trusted Platform Module (TPM) Support"
>>>>           depends on DM
>>>>           imply DM_RNG
>>>> -       select SHA1
>>>> -       select SHA256
>>>> -       select SHA384
>>>> -       select SHA512
>>>
>>> I am not sure this is the right way to deal with your problem.
>>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
>>> is really required. To make things even worse, you don't know the PCR
>>> banks that are enabled beforehand. This is a runtime config of the
>>> TPM.
>>
>> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
>> cannot extend PCRs. So it seems fine to let these two select the
>> complete set of hashing algorithms. As Simon pointed out for
>> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
>
> It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> 0xb0000000

So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.

TPM v1 only needs SHA-1.

In cmd/tpm-v2.c do_tpm2_pcr_extend() and do_tpm_pcr_read() assume
SHA256. Function tpm_pcr_extend() shows the same limitation. This bug
should be fixed. But as is CMD_TPM_V2 seems only to require CONFIG_SHA256.

Best regards

Heinrich

>
> Regards
> /Ilias
>>
>> Even if U-Boot does not support measured boot (EFI or non-EFI) we might
>> still be using the TPMs RNG.
>>
>> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>
>>>
>>>    So this would make the TPM pretty useless. Can't you remove something
>>> that doesn't break functionality?
>>>
>>> Thanks
>>> /Ilias
>>>>           help
>>>>             This enables support for TPMs which can be used to provide security
>>>>             features for your board. The TPM can be connected via LPC or I2C
>>>> --
>>>> 2.34.1
>>>>
>>

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Ilias Apalodimas]

Hi Heinrich,


On Fri, 14 Jun 2024 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 14.06.24 09:01, Ilias Apalodimas wrote:
> > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 6/14/24 08:03, Ilias Apalodimas wrote:
> >>> Hi Simon,
> >>>
> >>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> >>>>
> >>>> It does not make sense to enable all SHA algorithms unless they are
> >>>> needed. It bloats the code and in this case, causes chromebook_link to
> >>>> fail to build. That board does use the TPM, but not with measured boot,
> >>>> nor EFI.
> >>>>
> >>>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> >>>> add them to MEASURED_BOOT as well.
> >>>>
> >>>> Note that the original commit combines refactoring and new features,
> >>>> which makes it hard to see what is going on.
> >>>>
> >>>> Fixes: 97707f12fda tpm: Support boot measurements
> >>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> >>>> ---
> >>>>
> >>>> Changes in v2:
> >>>> - Put the conditions under EFI_TCG2_PROTOCOL
> >>>> - Consider MEASURED_BOOT too
> >>>>
> >>>>    boot/Kconfig | 4 ++++
> >>>>    lib/Kconfig  | 4 ----
> >>>>    2 files changed, 4 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/boot/Kconfig b/boot/Kconfig
> >>>> index 6f3096c15a6..b061891e109 100644
> >>>> --- a/boot/Kconfig
> >>>> +++ b/boot/Kconfig
> >>>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
> >>>>    config MEASURED_BOOT
> >>>>           bool "Measure boot images and configuration when booting without EFI"
> >>>>           depends on HASH && TPM_V2
> >>>> +       select SHA1
> >>>> +       select SHA256
> >>>> +       select SHA384
> >>>> +       select SHA512
> >>>>           help
> >>>>             This option enables measurement of the boot process when booting
> >>>>             without UEFI . Measurement involves creating cryptographic hashes
> >>>> diff --git a/lib/Kconfig b/lib/Kconfig
> >>>> index 189e6eb31aa..568892fce44 100644
> >>>> --- a/lib/Kconfig
> >>>> +++ b/lib/Kconfig
> >>>> @@ -438,10 +438,6 @@ config TPM
> >>>>           bool "Trusted Platform Module (TPM) Support"
> >>>>           depends on DM
> >>>>           imply DM_RNG
> >>>> -       select SHA1
> >>>> -       select SHA256
> >>>> -       select SHA384
> >>>> -       select SHA512
> >>>
> >>> I am not sure this is the right way to deal with your problem.
> >>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> >>> is really required. To make things even worse, you don't know the PCR
> >>> banks that are enabled beforehand. This is a runtime config of the
> >>> TPM.
> >>
> >> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> >> cannot extend PCRs. So it seems fine to let these two select the
> >> complete set of hashing algorithms. As Simon pointed out for
> >> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
> >
> > It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> > 0xb0000000
>
> So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.
>
> TPM v1 only needs SHA-1.

I still prefer to leave the TPM in a working state tbh.
>
> In cmd/tpm-v2.c do_tpm2_pcr_extend() and do_tpm_pcr_read() assume
> SHA256. Function tpm_pcr_extend() shows the same limitation. This bug
> should be fixed. But as is CMD_TPM_V2 seems only to require CONFIG_SHA256.
>
> Best regards
>
> Heinrich
>
> >
> > Regards
> > /Ilias
> >>
> >> Even if U-Boot does not support measured boot (EFI or non-EFI) we might
> >> still be using the TPMs RNG.
> >>
> >> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>
> >>>
> >>>    So this would make the TPM pretty useless. Can't you remove something
> >>> that doesn't break functionality?
> >>>
> >>> Thanks
> >>> /Ilias
> >>>>           help
> >>>>             This enables support for TPMs which can be used to provide security
> >>>>             features for your board. The TPM can be connected via LPC or I2C
> >>>> --
> >>>> 2.34.1
> >>>>
> >>
>

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Ilias Apalodimas]

Hi Heinrich

resending the reply, I accidentally sent half of the message...

On Fri, 14 Jun 2024 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 14.06.24 09:01, Ilias Apalodimas wrote:
> > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 6/14/24 08:03, Ilias Apalodimas wrote:
> >>> Hi Simon,
> >>>
> >>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> >>>>
> >>>> It does not make sense to enable all SHA algorithms unless they are
> >>>> needed. It bloats the code and in this case, causes chromebook_link to
> >>>> fail to build. That board does use the TPM, but not with measured boot,
> >>>> nor EFI.
> >>>>
> >>>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> >>>> add them to MEASURED_BOOT as well.
> >>>>
> >>>> Note that the original commit combines refactoring and new features,
> >>>> which makes it hard to see what is going on.
> >>>>
> >>>> Fixes: 97707f12fda tpm: Support boot measurements
> >>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> >>>> ---
> >>>>
> >>>> Changes in v2:
> >>>> - Put the conditions under EFI_TCG2_PROTOCOL
> >>>> - Consider MEASURED_BOOT too
> >>>>
> >>>>    boot/Kconfig | 4 ++++
> >>>>    lib/Kconfig  | 4 ----
> >>>>    2 files changed, 4 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/boot/Kconfig b/boot/Kconfig
> >>>> index 6f3096c15a6..b061891e109 100644
> >>>> --- a/boot/Kconfig
> >>>> +++ b/boot/Kconfig
> >>>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
> >>>>    config MEASURED_BOOT
> >>>>           bool "Measure boot images and configuration when booting without EFI"
> >>>>           depends on HASH && TPM_V2
> >>>> +       select SHA1
> >>>> +       select SHA256
> >>>> +       select SHA384
> >>>> +       select SHA512
> >>>>           help
> >>>>             This option enables measurement of the boot process when booting
> >>>>             without UEFI . Measurement involves creating cryptographic hashes
> >>>> diff --git a/lib/Kconfig b/lib/Kconfig
> >>>> index 189e6eb31aa..568892fce44 100644
> >>>> --- a/lib/Kconfig
> >>>> +++ b/lib/Kconfig
> >>>> @@ -438,10 +438,6 @@ config TPM
> >>>>           bool "Trusted Platform Module (TPM) Support"
> >>>>           depends on DM
> >>>>           imply DM_RNG
> >>>> -       select SHA1
> >>>> -       select SHA256
> >>>> -       select SHA384
> >>>> -       select SHA512
> >>>
> >>> I am not sure this is the right way to deal with your problem.
> >>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> >>> is really required. To make things even worse, you don't know the PCR
> >>> banks that are enabled beforehand. This is a runtime config of the
> >>> TPM.
> >>
> >> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> >> cannot extend PCRs. So it seems fine to let these two select the
> >> complete set of hashing algorithms. As Simon pointed out for
> >> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
> >
> > It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> > 0xb0000000
>
> So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.
>
> TPM v1 only needs SHA-1.

I still prefer to imply all algos.

>
> In cmd/tpm-v2.c do_tpm2_pcr_extend() and do_tpm_pcr_read() assume
> SHA256. Function tpm_pcr_extend() shows the same limitation. This bug
> should be fixed. But as is CMD_TPM_V2 seems only to require CONFIG_SHA256.

Isn't [0] fixing this?

[0] https://source.denx.de/u-boot/u-boot/-/commit/89aa8463cdf3919ca4f04fc24ec8b154ff56d97e
Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
> >
> > Regards
> > /Ilias
> >>
> >> Even if U-Boot does not support measured boot (EFI or non-EFI) we might
> >> still be using the TPMs RNG.
> >>
> >> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>
> >>>
> >>>    So this would make the TPM pretty useless. Can't you remove something
> >>> that doesn't break functionality?
> >>>
> >>> Thanks
> >>> /Ilias
> >>>>           help
> >>>>             This enables support for TPMs which can be used to provide security
> >>>>             features for your board. The TPM can be connected via LPC or I2C
> >>>> --
> >>>> 2.34.1
> >>>>
> >>
>

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Simon Glass]

Hi,

On Sat, 15 Jun 2024 at 01:03, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Heinrich
>
> resending the reply, I accidentally sent half of the message...
>
> On Fri, 14 Jun 2024 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 14.06.24 09:01, Ilias Apalodimas wrote:
> > > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >>
> > >> On 6/14/24 08:03, Ilias Apalodimas wrote:
> > >>> Hi Simon,
> > >>>
> > >>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> > >>>>
> > >>>> It does not make sense to enable all SHA algorithms unless they are
> > >>>> needed. It bloats the code and in this case, causes chromebook_link to
> > >>>> fail to build. That board does use the TPM, but not with measured boot,
> > >>>> nor EFI.
> > >>>>
> > >>>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> > >>>> add them to MEASURED_BOOT as well.
> > >>>>
> > >>>> Note that the original commit combines refactoring and new features,
> > >>>> which makes it hard to see what is going on.
> > >>>>
> > >>>> Fixes: 97707f12fda tpm: Support boot measurements
> > >>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> > >>>> ---
> > >>>>
> > >>>> Changes in v2:
> > >>>> - Put the conditions under EFI_TCG2_PROTOCOL
> > >>>> - Consider MEASURED_BOOT too
> > >>>>
> > >>>>    boot/Kconfig | 4 ++++
> > >>>>    lib/Kconfig  | 4 ----
> > >>>>    2 files changed, 4 insertions(+), 4 deletions(-)
> > >>>>
> > >>>> diff --git a/boot/Kconfig b/boot/Kconfig
> > >>>> index 6f3096c15a6..b061891e109 100644
> > >>>> --- a/boot/Kconfig
> > >>>> +++ b/boot/Kconfig
> > >>>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
> > >>>>    config MEASURED_BOOT
> > >>>>           bool "Measure boot images and configuration when booting without EFI"
> > >>>>           depends on HASH && TPM_V2
> > >>>> +       select SHA1
> > >>>> +       select SHA256
> > >>>> +       select SHA384
> > >>>> +       select SHA512
> > >>>>           help
> > >>>>             This option enables measurement of the boot process when booting
> > >>>>             without UEFI . Measurement involves creating cryptographic hashes
> > >>>> diff --git a/lib/Kconfig b/lib/Kconfig
> > >>>> index 189e6eb31aa..568892fce44 100644
> > >>>> --- a/lib/Kconfig
> > >>>> +++ b/lib/Kconfig
> > >>>> @@ -438,10 +438,6 @@ config TPM
> > >>>>           bool "Trusted Platform Module (TPM) Support"
> > >>>>           depends on DM
> > >>>>           imply DM_RNG
> > >>>> -       select SHA1
> > >>>> -       select SHA256
> > >>>> -       select SHA384
> > >>>> -       select SHA512
> > >>>
> > >>> I am not sure this is the right way to deal with your problem.
> > >>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> > >>> is really required. To make things even worse, you don't know the PCR
> > >>> banks that are enabled beforehand. This is a runtime config of the
> > >>> TPM.
> > >>
> > >> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> > >> cannot extend PCRs. So it seems fine to let these two select the
> > >> complete set of hashing algorithms. As Simon pointed out for
> > >> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
> > >
> > > It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> > > 0xb0000000

That's pretty normal for U-Boot though, since we want to avoid lots of
growth for things people might want control over. We can enable or
disable the SHA for the board, if this functionality is used outside
of measured boot and tcg2, but someone is enabling the tpm command.

> >
> > So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.
> >
> > TPM v1 only needs SHA-1.
>
> I still prefer to imply all algos.

'imply' would be OK in this case as I can disable it for that board. I
don't think it is in the spirit of U-Boot though.

isn't someone checking the growth in U-Boot? Or do so few boards have
TPMs that it didn't register? The size growth was 3.2KB on
chromebook_link.

>
> >
> > In cmd/tpm-v2.c do_tpm2_pcr_extend() and do_tpm_pcr_read() assume
> > SHA256. Function tpm_pcr_extend() shows the same limitation. This bug
> > should be fixed. But as is CMD_TPM_V2 seems only to require CONFIG_SHA256.
>
> Isn't [0] fixing this?
>
> [0] https://source.denx.de/u-boot/u-boot/-/commit/89aa8463cdf3919ca4f04fc24ec8b154ff56d97e
> Thanks
> /Ilias
> >
> > Best regards
> >
> > Heinrich
> >
> > >
> > > Regards
> > > /Ilias
> > >>
> > >> Even if U-Boot does not support measured boot (EFI or non-EFI) we might
> > >> still be using the TPMs RNG.
> > >>
> > >> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > >>
> > >>>
> > >>>    So this would make the TPM pretty useless. Can't you remove something
> > >>> that doesn't break functionality?
> > >>>
> > >>> Thanks
> > >>> /Ilias
> > >>>>           help
> > >>>>             This enables support for TPMs which can be used to provide security
> > >>>>             features for your board. The TPM can be connected via LPC or I2C
> > >>>> --
> > >>>> 2.34.1
> > >>>>
> > >>
> >

Regards,
Simon

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Tom Rini]

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

On Mon, Jun 17, 2024 at 07:53:22AM -0600, Simon Glass wrote:
> Hi,
> 
> On Sat, 15 Jun 2024 at 01:03, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Heinrich
> >
> > resending the reply, I accidentally sent half of the message...
> >
> > On Fri, 14 Jun 2024 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > > On 14.06.24 09:01, Ilias Apalodimas wrote:
> > > > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >>
> > > >> On 6/14/24 08:03, Ilias Apalodimas wrote:
> > > >>> Hi Simon,
> > > >>>
> > > >>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> > > >>>>
> > > >>>> It does not make sense to enable all SHA algorithms unless they are
> > > >>>> needed. It bloats the code and in this case, causes chromebook_link to
> > > >>>> fail to build. That board does use the TPM, but not with measured boot,
> > > >>>> nor EFI.
> > > >>>>
> > > >>>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> > > >>>> add them to MEASURED_BOOT as well.
> > > >>>>
> > > >>>> Note that the original commit combines refactoring and new features,
> > > >>>> which makes it hard to see what is going on.
> > > >>>>
> > > >>>> Fixes: 97707f12fda tpm: Support boot measurements
> > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> > > >>>> ---
> > > >>>>
> > > >>>> Changes in v2:
> > > >>>> - Put the conditions under EFI_TCG2_PROTOCOL
> > > >>>> - Consider MEASURED_BOOT too
> > > >>>>
> > > >>>>    boot/Kconfig | 4 ++++
> > > >>>>    lib/Kconfig  | 4 ----
> > > >>>>    2 files changed, 4 insertions(+), 4 deletions(-)
> > > >>>>
> > > >>>> diff --git a/boot/Kconfig b/boot/Kconfig
> > > >>>> index 6f3096c15a6..b061891e109 100644
> > > >>>> --- a/boot/Kconfig
> > > >>>> +++ b/boot/Kconfig
> > > >>>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
> > > >>>>    config MEASURED_BOOT
> > > >>>>           bool "Measure boot images and configuration when booting without EFI"
> > > >>>>           depends on HASH && TPM_V2
> > > >>>> +       select SHA1
> > > >>>> +       select SHA256
> > > >>>> +       select SHA384
> > > >>>> +       select SHA512
> > > >>>>           help
> > > >>>>             This option enables measurement of the boot process when booting
> > > >>>>             without UEFI . Measurement involves creating cryptographic hashes
> > > >>>> diff --git a/lib/Kconfig b/lib/Kconfig
> > > >>>> index 189e6eb31aa..568892fce44 100644
> > > >>>> --- a/lib/Kconfig
> > > >>>> +++ b/lib/Kconfig
> > > >>>> @@ -438,10 +438,6 @@ config TPM
> > > >>>>           bool "Trusted Platform Module (TPM) Support"
> > > >>>>           depends on DM
> > > >>>>           imply DM_RNG
> > > >>>> -       select SHA1
> > > >>>> -       select SHA256
> > > >>>> -       select SHA384
> > > >>>> -       select SHA512
> > > >>>
> > > >>> I am not sure this is the right way to deal with your problem.
> > > >>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> > > >>> is really required. To make things even worse, you don't know the PCR
> > > >>> banks that are enabled beforehand. This is a runtime config of the
> > > >>> TPM.
> > > >>
> > > >> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> > > >> cannot extend PCRs. So it seems fine to let these two select the
> > > >> complete set of hashing algorithms. As Simon pointed out for
> > > >> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
> > > >
> > > > It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> > > > 0xb0000000
> 
> That's pretty normal for U-Boot though, since we want to avoid lots of
> growth for things people might want control over. We can enable or
> disable the SHA for the board, if this functionality is used outside
> of measured boot and tcg2, but someone is enabling the tpm command.
> 
> > >
> > > So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.
> > >
> > > TPM v1 only needs SHA-1.
> >
> > I still prefer to imply all algos.
> 
> 'imply' would be OK in this case as I can disable it for that board. I
> don't think it is in the spirit of U-Boot though.
> 
> isn't someone checking the growth in U-Boot? Or do so few boards have
> TPMs that it didn't register? The size growth was 3.2KB on
> chromebook_link.

As always, yes, nearly every PR (I don't check the ones that touch just
a single board for example) gets a world build before/after. In this
case I likely assumed that it was acceptable growth for enabling
features. It sounds like some of the chromebook boards need to be
setting the features to cause link failure if a size is exceeded?

-- 
Tom

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

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Simon Glass]

Hi Tom,

On Mon, 17 Jun 2024 at 11:16, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Jun 17, 2024 at 07:53:22AM -0600, Simon Glass wrote:
> > Hi,
> >
> > On Sat, 15 Jun 2024 at 01:03, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Heinrich
> > >
> > > resending the reply, I accidentally sent half of the message...
> > >
> > > On Fri, 14 Jun 2024 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >
> > > > On 14.06.24 09:01, Ilias Apalodimas wrote:
> > > > > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > >>
> > > > >> On 6/14/24 08:03, Ilias Apalodimas wrote:
> > > > >>> Hi Simon,
> > > > >>>
> > > > >>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> > > > >>>>
> > > > >>>> It does not make sense to enable all SHA algorithms unless they are
> > > > >>>> needed. It bloats the code and in this case, causes chromebook_link to
> > > > >>>> fail to build. That board does use the TPM, but not with measured boot,
> > > > >>>> nor EFI.
> > > > >>>>
> > > > >>>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> > > > >>>> add them to MEASURED_BOOT as well.
> > > > >>>>
> > > > >>>> Note that the original commit combines refactoring and new features,
> > > > >>>> which makes it hard to see what is going on.
> > > > >>>>
> > > > >>>> Fixes: 97707f12fda tpm: Support boot measurements
> > > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > >>>> ---
> > > > >>>>
> > > > >>>> Changes in v2:
> > > > >>>> - Put the conditions under EFI_TCG2_PROTOCOL
> > > > >>>> - Consider MEASURED_BOOT too
> > > > >>>>
> > > > >>>>    boot/Kconfig | 4 ++++
> > > > >>>>    lib/Kconfig  | 4 ----
> > > > >>>>    2 files changed, 4 insertions(+), 4 deletions(-)
> > > > >>>>
> > > > >>>> diff --git a/boot/Kconfig b/boot/Kconfig
> > > > >>>> index 6f3096c15a6..b061891e109 100644
> > > > >>>> --- a/boot/Kconfig
> > > > >>>> +++ b/boot/Kconfig
> > > > >>>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
> > > > >>>>    config MEASURED_BOOT
> > > > >>>>           bool "Measure boot images and configuration when booting without EFI"
> > > > >>>>           depends on HASH && TPM_V2
> > > > >>>> +       select SHA1
> > > > >>>> +       select SHA256
> > > > >>>> +       select SHA384
> > > > >>>> +       select SHA512
> > > > >>>>           help
> > > > >>>>             This option enables measurement of the boot process when booting
> > > > >>>>             without UEFI . Measurement involves creating cryptographic hashes
> > > > >>>> diff --git a/lib/Kconfig b/lib/Kconfig
> > > > >>>> index 189e6eb31aa..568892fce44 100644
> > > > >>>> --- a/lib/Kconfig
> > > > >>>> +++ b/lib/Kconfig
> > > > >>>> @@ -438,10 +438,6 @@ config TPM
> > > > >>>>           bool "Trusted Platform Module (TPM) Support"
> > > > >>>>           depends on DM
> > > > >>>>           imply DM_RNG
> > > > >>>> -       select SHA1
> > > > >>>> -       select SHA256
> > > > >>>> -       select SHA384
> > > > >>>> -       select SHA512
> > > > >>>
> > > > >>> I am not sure this is the right way to deal with your problem.
> > > > >>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> > > > >>> is really required. To make things even worse, you don't know the PCR
> > > > >>> banks that are enabled beforehand. This is a runtime config of the
> > > > >>> TPM.
> > > > >>
> > > > >> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> > > > >> cannot extend PCRs. So it seems fine to let these two select the
> > > > >> complete set of hashing algorithms. As Simon pointed out for
> > > > >> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
> > > > >
> > > > > It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> > > > > 0xb0000000
> >
> > That's pretty normal for U-Boot though, since we want to avoid lots of
> > growth for things people might want control over. We can enable or
> > disable the SHA for the board, if this functionality is used outside
> > of measured boot and tcg2, but someone is enabling the tpm command.
> >
> > > >
> > > > So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.
> > > >
> > > > TPM v1 only needs SHA-1.
> > >
> > > I still prefer to imply all algos.
> >
> > 'imply' would be OK in this case as I can disable it for that board. I
> > don't think it is in the spirit of U-Boot though.
> >
> > isn't someone checking the growth in U-Boot? Or do so few boards have
> > TPMs that it didn't register? The size growth was 3.2KB on
> > chromebook_link.
>
> As always, yes, nearly every PR (I don't check the ones that touch just
> a single board for example) gets a world build before/after. In this
> case I likely assumed that it was acceptable growth for enabling
> features. It sounds like some of the chromebook boards need to be
> setting the features to cause link failure if a size is exceeded?

The problem is that some Intel platforms have binary blobs, so the
size isn't known unless you have a real blob.

Regards,
Simon


>
> --
> Tom

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Tom Rini]

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

On Tue, Jun 18, 2024 at 06:43:51AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 17 Jun 2024 at 11:16, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Jun 17, 2024 at 07:53:22AM -0600, Simon Glass wrote:
> > > Hi,
> > >
> > > On Sat, 15 Jun 2024 at 01:03, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > Hi Heinrich
> > > >
> > > > resending the reply, I accidentally sent half of the message...
> > > >
> > > > On Fri, 14 Jun 2024 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > >
> > > > > On 14.06.24 09:01, Ilias Apalodimas wrote:
> > > > > > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > >>
> > > > > >> On 6/14/24 08:03, Ilias Apalodimas wrote:
> > > > > >>> Hi Simon,
> > > > > >>>
> > > > > >>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> > > > > >>>>
> > > > > >>>> It does not make sense to enable all SHA algorithms unless they are
> > > > > >>>> needed. It bloats the code and in this case, causes chromebook_link to
> > > > > >>>> fail to build. That board does use the TPM, but not with measured boot,
> > > > > >>>> nor EFI.
> > > > > >>>>
> > > > > >>>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> > > > > >>>> add them to MEASURED_BOOT as well.
> > > > > >>>>
> > > > > >>>> Note that the original commit combines refactoring and new features,
> > > > > >>>> which makes it hard to see what is going on.
> > > > > >>>>
> > > > > >>>> Fixes: 97707f12fda tpm: Support boot measurements
> > > > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > >>>> ---
> > > > > >>>>
> > > > > >>>> Changes in v2:
> > > > > >>>> - Put the conditions under EFI_TCG2_PROTOCOL
> > > > > >>>> - Consider MEASURED_BOOT too
> > > > > >>>>
> > > > > >>>>    boot/Kconfig | 4 ++++
> > > > > >>>>    lib/Kconfig  | 4 ----
> > > > > >>>>    2 files changed, 4 insertions(+), 4 deletions(-)
> > > > > >>>>
> > > > > >>>> diff --git a/boot/Kconfig b/boot/Kconfig
> > > > > >>>> index 6f3096c15a6..b061891e109 100644
> > > > > >>>> --- a/boot/Kconfig
> > > > > >>>> +++ b/boot/Kconfig
> > > > > >>>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
> > > > > >>>>    config MEASURED_BOOT
> > > > > >>>>           bool "Measure boot images and configuration when booting without EFI"
> > > > > >>>>           depends on HASH && TPM_V2
> > > > > >>>> +       select SHA1
> > > > > >>>> +       select SHA256
> > > > > >>>> +       select SHA384
> > > > > >>>> +       select SHA512
> > > > > >>>>           help
> > > > > >>>>             This option enables measurement of the boot process when booting
> > > > > >>>>             without UEFI . Measurement involves creating cryptographic hashes
> > > > > >>>> diff --git a/lib/Kconfig b/lib/Kconfig
> > > > > >>>> index 189e6eb31aa..568892fce44 100644
> > > > > >>>> --- a/lib/Kconfig
> > > > > >>>> +++ b/lib/Kconfig
> > > > > >>>> @@ -438,10 +438,6 @@ config TPM
> > > > > >>>>           bool "Trusted Platform Module (TPM) Support"
> > > > > >>>>           depends on DM
> > > > > >>>>           imply DM_RNG
> > > > > >>>> -       select SHA1
> > > > > >>>> -       select SHA256
> > > > > >>>> -       select SHA384
> > > > > >>>> -       select SHA512
> > > > > >>>
> > > > > >>> I am not sure this is the right way to deal with your problem.
> > > > > >>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> > > > > >>> is really required. To make things even worse, you don't know the PCR
> > > > > >>> banks that are enabled beforehand. This is a runtime config of the
> > > > > >>> TPM.
> > > > > >>
> > > > > >> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> > > > > >> cannot extend PCRs. So it seems fine to let these two select the
> > > > > >> complete set of hashing algorithms. As Simon pointed out for
> > > > > >> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
> > > > > >
> > > > > > It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> > > > > > 0xb0000000
> > >
> > > That's pretty normal for U-Boot though, since we want to avoid lots of
> > > growth for things people might want control over. We can enable or
> > > disable the SHA for the board, if this functionality is used outside
> > > of measured boot and tcg2, but someone is enabling the tpm command.
> > >
> > > > >
> > > > > So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.
> > > > >
> > > > > TPM v1 only needs SHA-1.
> > > >
> > > > I still prefer to imply all algos.
> > >
> > > 'imply' would be OK in this case as I can disable it for that board. I
> > > don't think it is in the spirit of U-Boot though.
> > >
> > > isn't someone checking the growth in U-Boot? Or do so few boards have
> > > TPMs that it didn't register? The size growth was 3.2KB on
> > > chromebook_link.
> >
> > As always, yes, nearly every PR (I don't check the ones that touch just
> > a single board for example) gets a world build before/after. In this
> > case I likely assumed that it was acceptable growth for enabling
> > features. It sounds like some of the chromebook boards need to be
> > setting the features to cause link failure if a size is exceeded?
> 
> The problem is that some Intel platforms have binary blobs, so the
> size isn't known unless you have a real blob.

Alright, but can't we put in some limit based on what the current blobs
are, or look at the last few blob releases and see if they change much
in size and put something in? Other platforms have blobs and size
limits...

-- 
Tom

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

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Simon Glass]

Hi Tom,

On Tue, 18 Jun 2024 at 08:15, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Jun 18, 2024 at 06:43:51AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 17 Jun 2024 at 11:16, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Mon, Jun 17, 2024 at 07:53:22AM -0600, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Sat, 15 Jun 2024 at 01:03, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > Hi Heinrich
> > > > >
> > > > > resending the reply, I accidentally sent half of the message...
> > > > >
> > > > > On Fri, 14 Jun 2024 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > >
> > > > > > On 14.06.24 09:01, Ilias Apalodimas wrote:
> > > > > > > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > >>
> > > > > > >> On 6/14/24 08:03, Ilias Apalodimas wrote:
> > > > > > >>> Hi Simon,
> > > > > > >>>
> > > > > > >>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> > > > > > >>>>
> > > > > > >>>> It does not make sense to enable all SHA algorithms unless they are
> > > > > > >>>> needed. It bloats the code and in this case, causes chromebook_link to
> > > > > > >>>> fail to build. That board does use the TPM, but not with measured boot,
> > > > > > >>>> nor EFI.
> > > > > > >>>>
> > > > > > >>>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> > > > > > >>>> add them to MEASURED_BOOT as well.
> > > > > > >>>>
> > > > > > >>>> Note that the original commit combines refactoring and new features,
> > > > > > >>>> which makes it hard to see what is going on.
> > > > > > >>>>
> > > > > > >>>> Fixes: 97707f12fda tpm: Support boot measurements
> > > > > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > >>>> ---
> > > > > > >>>>
> > > > > > >>>> Changes in v2:
> > > > > > >>>> - Put the conditions under EFI_TCG2_PROTOCOL
> > > > > > >>>> - Consider MEASURED_BOOT too
> > > > > > >>>>
> > > > > > >>>>    boot/Kconfig | 4 ++++
> > > > > > >>>>    lib/Kconfig  | 4 ----
> > > > > > >>>>    2 files changed, 4 insertions(+), 4 deletions(-)
> > > > > > >>>>
> > > > > > >>>> diff --git a/boot/Kconfig b/boot/Kconfig
> > > > > > >>>> index 6f3096c15a6..b061891e109 100644
> > > > > > >>>> --- a/boot/Kconfig
> > > > > > >>>> +++ b/boot/Kconfig
> > > > > > >>>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
> > > > > > >>>>    config MEASURED_BOOT
> > > > > > >>>>           bool "Measure boot images and configuration when booting without EFI"
> > > > > > >>>>           depends on HASH && TPM_V2
> > > > > > >>>> +       select SHA1
> > > > > > >>>> +       select SHA256
> > > > > > >>>> +       select SHA384
> > > > > > >>>> +       select SHA512
> > > > > > >>>>           help
> > > > > > >>>>             This option enables measurement of the boot process when booting
> > > > > > >>>>             without UEFI . Measurement involves creating cryptographic hashes
> > > > > > >>>> diff --git a/lib/Kconfig b/lib/Kconfig
> > > > > > >>>> index 189e6eb31aa..568892fce44 100644
> > > > > > >>>> --- a/lib/Kconfig
> > > > > > >>>> +++ b/lib/Kconfig
> > > > > > >>>> @@ -438,10 +438,6 @@ config TPM
> > > > > > >>>>           bool "Trusted Platform Module (TPM) Support"
> > > > > > >>>>           depends on DM
> > > > > > >>>>           imply DM_RNG
> > > > > > >>>> -       select SHA1
> > > > > > >>>> -       select SHA256
> > > > > > >>>> -       select SHA384
> > > > > > >>>> -       select SHA512
> > > > > > >>>
> > > > > > >>> I am not sure this is the right way to deal with your problem.
> > > > > > >>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> > > > > > >>> is really required. To make things even worse, you don't know the PCR
> > > > > > >>> banks that are enabled beforehand. This is a runtime config of the
> > > > > > >>> TPM.
> > > > > > >>
> > > > > > >> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> > > > > > >> cannot extend PCRs. So it seems fine to let these two select the
> > > > > > >> complete set of hashing algorithms. As Simon pointed out for
> > > > > > >> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
> > > > > > >
> > > > > > > It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> > > > > > > 0xb0000000
> > > >
> > > > That's pretty normal for U-Boot though, since we want to avoid lots of
> > > > growth for things people might want control over. We can enable or
> > > > disable the SHA for the board, if this functionality is used outside
> > > > of measured boot and tcg2, but someone is enabling the tpm command.
> > > >
> > > > > >
> > > > > > So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.
> > > > > >
> > > > > > TPM v1 only needs SHA-1.
> > > > >
> > > > > I still prefer to imply all algos.
> > > >
> > > > 'imply' would be OK in this case as I can disable it for that board. I
> > > > don't think it is in the spirit of U-Boot though.
> > > >
> > > > isn't someone checking the growth in U-Boot? Or do so few boards have
> > > > TPMs that it didn't register? The size growth was 3.2KB on
> > > > chromebook_link.
> > >
> > > As always, yes, nearly every PR (I don't check the ones that touch just
> > > a single board for example) gets a world build before/after. In this
> > > case I likely assumed that it was acceptable growth for enabling
> > > features. It sounds like some of the chromebook boards need to be
> > > setting the features to cause link failure if a size is exceeded?
> >
> > The problem is that some Intel platforms have binary blobs, so the
> > size isn't known unless you have a real blob.
>
> Alright, but can't we put in some limit based on what the current blobs
> are, or look at the last few blob releases and see if they change much
> in size and put something in? Other platforms have blobs and size
> limits...

Yes we can duplicate the entry sizes from binman into Kconfig options.
But I am not a fan of that...two variables controlling the same thing
and both can break, with nothing to connect them other than the poor
user.

I wonder if some magic could solve this, inferring limits from the
binman image. But that is getting into yet another domain...

Regards,
Simon

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Tom Rini]

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

On Tue, Jun 18, 2024 at 09:03:37PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 18 Jun 2024 at 08:15, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Jun 18, 2024 at 06:43:51AM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 17 Jun 2024 at 11:16, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Mon, Jun 17, 2024 at 07:53:22AM -0600, Simon Glass wrote:
> > > > > Hi,
> > > > >
> > > > > On Sat, 15 Jun 2024 at 01:03, Ilias Apalodimas
> > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > >
> > > > > > Hi Heinrich
> > > > > >
> > > > > > resending the reply, I accidentally sent half of the message...
> > > > > >
> > > > > > On Fri, 14 Jun 2024 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > >
> > > > > > > On 14.06.24 09:01, Ilias Apalodimas wrote:
> > > > > > > > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > >>
> > > > > > > >> On 6/14/24 08:03, Ilias Apalodimas wrote:
> > > > > > > >>> Hi Simon,
> > > > > > > >>>
> > > > > > > >>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >>>>
> > > > > > > >>>> It does not make sense to enable all SHA algorithms unless they are
> > > > > > > >>>> needed. It bloats the code and in this case, causes chromebook_link to
> > > > > > > >>>> fail to build. That board does use the TPM, but not with measured boot,
> > > > > > > >>>> nor EFI.
> > > > > > > >>>>
> > > > > > > >>>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> > > > > > > >>>> add them to MEASURED_BOOT as well.
> > > > > > > >>>>
> > > > > > > >>>> Note that the original commit combines refactoring and new features,
> > > > > > > >>>> which makes it hard to see what is going on.
> > > > > > > >>>>
> > > > > > > >>>> Fixes: 97707f12fda tpm: Support boot measurements
> > > > > > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > >>>> ---
> > > > > > > >>>>
> > > > > > > >>>> Changes in v2:
> > > > > > > >>>> - Put the conditions under EFI_TCG2_PROTOCOL
> > > > > > > >>>> - Consider MEASURED_BOOT too
> > > > > > > >>>>
> > > > > > > >>>>    boot/Kconfig | 4 ++++
> > > > > > > >>>>    lib/Kconfig  | 4 ----
> > > > > > > >>>>    2 files changed, 4 insertions(+), 4 deletions(-)
> > > > > > > >>>>
> > > > > > > >>>> diff --git a/boot/Kconfig b/boot/Kconfig
> > > > > > > >>>> index 6f3096c15a6..b061891e109 100644
> > > > > > > >>>> --- a/boot/Kconfig
> > > > > > > >>>> +++ b/boot/Kconfig
> > > > > > > >>>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
> > > > > > > >>>>    config MEASURED_BOOT
> > > > > > > >>>>           bool "Measure boot images and configuration when booting without EFI"
> > > > > > > >>>>           depends on HASH && TPM_V2
> > > > > > > >>>> +       select SHA1
> > > > > > > >>>> +       select SHA256
> > > > > > > >>>> +       select SHA384
> > > > > > > >>>> +       select SHA512
> > > > > > > >>>>           help
> > > > > > > >>>>             This option enables measurement of the boot process when booting
> > > > > > > >>>>             without UEFI . Measurement involves creating cryptographic hashes
> > > > > > > >>>> diff --git a/lib/Kconfig b/lib/Kconfig
> > > > > > > >>>> index 189e6eb31aa..568892fce44 100644
> > > > > > > >>>> --- a/lib/Kconfig
> > > > > > > >>>> +++ b/lib/Kconfig
> > > > > > > >>>> @@ -438,10 +438,6 @@ config TPM
> > > > > > > >>>>           bool "Trusted Platform Module (TPM) Support"
> > > > > > > >>>>           depends on DM
> > > > > > > >>>>           imply DM_RNG
> > > > > > > >>>> -       select SHA1
> > > > > > > >>>> -       select SHA256
> > > > > > > >>>> -       select SHA384
> > > > > > > >>>> -       select SHA512
> > > > > > > >>>
> > > > > > > >>> I am not sure this is the right way to deal with your problem.
> > > > > > > >>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> > > > > > > >>> is really required. To make things even worse, you don't know the PCR
> > > > > > > >>> banks that are enabled beforehand. This is a runtime config of the
> > > > > > > >>> TPM.
> > > > > > > >>
> > > > > > > >> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> > > > > > > >> cannot extend PCRs. So it seems fine to let these two select the
> > > > > > > >> complete set of hashing algorithms. As Simon pointed out for
> > > > > > > >> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
> > > > > > > >
> > > > > > > > It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> > > > > > > > 0xb0000000
> > > > >
> > > > > That's pretty normal for U-Boot though, since we want to avoid lots of
> > > > > growth for things people might want control over. We can enable or
> > > > > disable the SHA for the board, if this functionality is used outside
> > > > > of measured boot and tcg2, but someone is enabling the tpm command.
> > > > >
> > > > > > >
> > > > > > > So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.
> > > > > > >
> > > > > > > TPM v1 only needs SHA-1.
> > > > > >
> > > > > > I still prefer to imply all algos.
> > > > >
> > > > > 'imply' would be OK in this case as I can disable it for that board. I
> > > > > don't think it is in the spirit of U-Boot though.
> > > > >
> > > > > isn't someone checking the growth in U-Boot? Or do so few boards have
> > > > > TPMs that it didn't register? The size growth was 3.2KB on
> > > > > chromebook_link.
> > > >
> > > > As always, yes, nearly every PR (I don't check the ones that touch just
> > > > a single board for example) gets a world build before/after. In this
> > > > case I likely assumed that it was acceptable growth for enabling
> > > > features. It sounds like some of the chromebook boards need to be
> > > > setting the features to cause link failure if a size is exceeded?
> > >
> > > The problem is that some Intel platforms have binary blobs, so the
> > > size isn't known unless you have a real blob.
> >
> > Alright, but can't we put in some limit based on what the current blobs
> > are, or look at the last few blob releases and see if they change much
> > in size and put something in? Other platforms have blobs and size
> > limits...
> 
> Yes we can duplicate the entry sizes from binman into Kconfig options.
> But I am not a fan of that...two variables controlling the same thing
> and both can break, with nothing to connect them other than the poor
> user.
> 
> I wonder if some magic could solve this, inferring limits from the
> binman image. But that is getting into yet another domain...

I'm sorry, I just don't see what makes this case different from all of
the other cases where we have to include blobs. We know there's X amount
of flash available, blobs tend to take up Y and so we set
BOARD_SIZE_LIMIT to Z where there's some room for growth in U-Boot but
it takes in to account whatever limits the blobs place on us.

-- 
Tom

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

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Simon Glass]

Hi Tom,

On Wed, 19 Jun 2024 at 09:32, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Jun 18, 2024 at 09:03:37PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 18 Jun 2024 at 08:15, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Jun 18, 2024 at 06:43:51AM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Mon, 17 Jun 2024 at 11:16, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Mon, Jun 17, 2024 at 07:53:22AM -0600, Simon Glass wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Sat, 15 Jun 2024 at 01:03, Ilias Apalodimas
> > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > >
> > > > > > > Hi Heinrich
> > > > > > >
> > > > > > > resending the reply, I accidentally sent half of the message...
> > > > > > >
> > > > > > > On Fri, 14 Jun 2024 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > >
> > > > > > > > On 14.06.24 09:01, Ilias Apalodimas wrote:
> > > > > > > > > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > >>
> > > > > > > > >> On 6/14/24 08:03, Ilias Apalodimas wrote:
> > > > > > > > >>> Hi Simon,
> > > > > > > > >>>
> > > > > > > > >>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > >>>>
> > > > > > > > >>>> It does not make sense to enable all SHA algorithms unless they are
> > > > > > > > >>>> needed. It bloats the code and in this case, causes chromebook_link to
> > > > > > > > >>>> fail to build. That board does use the TPM, but not with measured boot,
> > > > > > > > >>>> nor EFI.
> > > > > > > > >>>>
> > > > > > > > >>>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> > > > > > > > >>>> add them to MEASURED_BOOT as well.
> > > > > > > > >>>>
> > > > > > > > >>>> Note that the original commit combines refactoring and new features,
> > > > > > > > >>>> which makes it hard to see what is going on.
> > > > > > > > >>>>
> > > > > > > > >>>> Fixes: 97707f12fda tpm: Support boot measurements
> > > > > > > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > >>>> ---
> > > > > > > > >>>>
> > > > > > > > >>>> Changes in v2:
> > > > > > > > >>>> - Put the conditions under EFI_TCG2_PROTOCOL
> > > > > > > > >>>> - Consider MEASURED_BOOT too
> > > > > > > > >>>>
> > > > > > > > >>>>    boot/Kconfig | 4 ++++
> > > > > > > > >>>>    lib/Kconfig  | 4 ----
> > > > > > > > >>>>    2 files changed, 4 insertions(+), 4 deletions(-)
> > > > > > > > >>>>
> > > > > > > > >>>> diff --git a/boot/Kconfig b/boot/Kconfig
> > > > > > > > >>>> index 6f3096c15a6..b061891e109 100644
> > > > > > > > >>>> --- a/boot/Kconfig
> > > > > > > > >>>> +++ b/boot/Kconfig
> > > > > > > > >>>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
> > > > > > > > >>>>    config MEASURED_BOOT
> > > > > > > > >>>>           bool "Measure boot images and configuration when booting without EFI"
> > > > > > > > >>>>           depends on HASH && TPM_V2
> > > > > > > > >>>> +       select SHA1
> > > > > > > > >>>> +       select SHA256
> > > > > > > > >>>> +       select SHA384
> > > > > > > > >>>> +       select SHA512
> > > > > > > > >>>>           help
> > > > > > > > >>>>             This option enables measurement of the boot process when booting
> > > > > > > > >>>>             without UEFI . Measurement involves creating cryptographic hashes
> > > > > > > > >>>> diff --git a/lib/Kconfig b/lib/Kconfig
> > > > > > > > >>>> index 189e6eb31aa..568892fce44 100644
> > > > > > > > >>>> --- a/lib/Kconfig
> > > > > > > > >>>> +++ b/lib/Kconfig
> > > > > > > > >>>> @@ -438,10 +438,6 @@ config TPM
> > > > > > > > >>>>           bool "Trusted Platform Module (TPM) Support"
> > > > > > > > >>>>           depends on DM
> > > > > > > > >>>>           imply DM_RNG
> > > > > > > > >>>> -       select SHA1
> > > > > > > > >>>> -       select SHA256
> > > > > > > > >>>> -       select SHA384
> > > > > > > > >>>> -       select SHA512
> > > > > > > > >>>
> > > > > > > > >>> I am not sure this is the right way to deal with your problem.
> > > > > > > > >>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> > > > > > > > >>> is really required. To make things even worse, you don't know the PCR
> > > > > > > > >>> banks that are enabled beforehand. This is a runtime config of the
> > > > > > > > >>> TPM.
> > > > > > > > >>
> > > > > > > > >> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> > > > > > > > >> cannot extend PCRs. So it seems fine to let these two select the
> > > > > > > > >> complete set of hashing algorithms. As Simon pointed out for
> > > > > > > > >> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
> > > > > > > > >
> > > > > > > > > It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> > > > > > > > > 0xb0000000
> > > > > >
> > > > > > That's pretty normal for U-Boot though, since we want to avoid lots of
> > > > > > growth for things people might want control over. We can enable or
> > > > > > disable the SHA for the board, if this functionality is used outside
> > > > > > of measured boot and tcg2, but someone is enabling the tpm command.
> > > > > >
> > > > > > > >
> > > > > > > > So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.
> > > > > > > >
> > > > > > > > TPM v1 only needs SHA-1.
> > > > > > >
> > > > > > > I still prefer to imply all algos.
> > > > > >
> > > > > > 'imply' would be OK in this case as I can disable it for that board. I
> > > > > > don't think it is in the spirit of U-Boot though.
> > > > > >
> > > > > > isn't someone checking the growth in U-Boot? Or do so few boards have
> > > > > > TPMs that it didn't register? The size growth was 3.2KB on
> > > > > > chromebook_link.
> > > > >
> > > > > As always, yes, nearly every PR (I don't check the ones that touch just
> > > > > a single board for example) gets a world build before/after. In this
> > > > > case I likely assumed that it was acceptable growth for enabling
> > > > > features. It sounds like some of the chromebook boards need to be
> > > > > setting the features to cause link failure if a size is exceeded?
> > > >
> > > > The problem is that some Intel platforms have binary blobs, so the
> > > > size isn't known unless you have a real blob.
> > >
> > > Alright, but can't we put in some limit based on what the current blobs
> > > are, or look at the last few blob releases and see if they change much
> > > in size and put something in? Other platforms have blobs and size
> > > limits...
> >
> > Yes we can duplicate the entry sizes from binman into Kconfig options.
> > But I am not a fan of that...two variables controlling the same thing
> > and both can break, with nothing to connect them other than the poor
> > user.
> >
> > I wonder if some magic could solve this, inferring limits from the
> > binman image. But that is getting into yet another domain...
>
> I'm sorry, I just don't see what makes this case different from all of
> the other cases where we have to include blobs. We know there's X amount
> of flash available, blobs tend to take up Y and so we set
> BOARD_SIZE_LIMIT to Z where there's some room for growth in U-Boot but
> it takes in to account whatever limits the blobs place on us.

The difference is that now we are using Binman, we have the limit in
the image description, so adding it to Kconfig a) involves
calculations and perhaps guesswork and b) results in two limits stored
in different places which may conflict.

I thought of a way to handle this in Binman, so will send that in the
next version.

Regards,
Simon

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Tom Rini]

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

On Thu, Jun 20, 2024 at 05:05:29PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 19 Jun 2024 at 09:32, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Jun 18, 2024 at 09:03:37PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Tue, 18 Jun 2024 at 08:15, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Jun 18, 2024 at 06:43:51AM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Mon, 17 Jun 2024 at 11:16, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Mon, Jun 17, 2024 at 07:53:22AM -0600, Simon Glass wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Sat, 15 Jun 2024 at 01:03, Ilias Apalodimas
> > > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > > >
> > > > > > > > Hi Heinrich
> > > > > > > >
> > > > > > > > resending the reply, I accidentally sent half of the message...
> > > > > > > >
> > > > > > > > On Fri, 14 Jun 2024 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > >
> > > > > > > > > On 14.06.24 09:01, Ilias Apalodimas wrote:
> > > > > > > > > > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > > >>
> > > > > > > > > >> On 6/14/24 08:03, Ilias Apalodimas wrote:
> > > > > > > > > >>> Hi Simon,
> > > > > > > > > >>>
> > > > > > > > > >>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > >>>>
> > > > > > > > > >>>> It does not make sense to enable all SHA algorithms unless they are
> > > > > > > > > >>>> needed. It bloats the code and in this case, causes chromebook_link to
> > > > > > > > > >>>> fail to build. That board does use the TPM, but not with measured boot,
> > > > > > > > > >>>> nor EFI.
> > > > > > > > > >>>>
> > > > > > > > > >>>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> > > > > > > > > >>>> add them to MEASURED_BOOT as well.
> > > > > > > > > >>>>
> > > > > > > > > >>>> Note that the original commit combines refactoring and new features,
> > > > > > > > > >>>> which makes it hard to see what is going on.
> > > > > > > > > >>>>
> > > > > > > > > >>>> Fixes: 97707f12fda tpm: Support boot measurements
> > > > > > > > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > >>>> ---
> > > > > > > > > >>>>
> > > > > > > > > >>>> Changes in v2:
> > > > > > > > > >>>> - Put the conditions under EFI_TCG2_PROTOCOL
> > > > > > > > > >>>> - Consider MEASURED_BOOT too
> > > > > > > > > >>>>
> > > > > > > > > >>>>    boot/Kconfig | 4 ++++
> > > > > > > > > >>>>    lib/Kconfig  | 4 ----
> > > > > > > > > >>>>    2 files changed, 4 insertions(+), 4 deletions(-)
> > > > > > > > > >>>>
> > > > > > > > > >>>> diff --git a/boot/Kconfig b/boot/Kconfig
> > > > > > > > > >>>> index 6f3096c15a6..b061891e109 100644
> > > > > > > > > >>>> --- a/boot/Kconfig
> > > > > > > > > >>>> +++ b/boot/Kconfig
> > > > > > > > > >>>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
> > > > > > > > > >>>>    config MEASURED_BOOT
> > > > > > > > > >>>>           bool "Measure boot images and configuration when booting without EFI"
> > > > > > > > > >>>>           depends on HASH && TPM_V2
> > > > > > > > > >>>> +       select SHA1
> > > > > > > > > >>>> +       select SHA256
> > > > > > > > > >>>> +       select SHA384
> > > > > > > > > >>>> +       select SHA512
> > > > > > > > > >>>>           help
> > > > > > > > > >>>>             This option enables measurement of the boot process when booting
> > > > > > > > > >>>>             without UEFI . Measurement involves creating cryptographic hashes
> > > > > > > > > >>>> diff --git a/lib/Kconfig b/lib/Kconfig
> > > > > > > > > >>>> index 189e6eb31aa..568892fce44 100644
> > > > > > > > > >>>> --- a/lib/Kconfig
> > > > > > > > > >>>> +++ b/lib/Kconfig
> > > > > > > > > >>>> @@ -438,10 +438,6 @@ config TPM
> > > > > > > > > >>>>           bool "Trusted Platform Module (TPM) Support"
> > > > > > > > > >>>>           depends on DM
> > > > > > > > > >>>>           imply DM_RNG
> > > > > > > > > >>>> -       select SHA1
> > > > > > > > > >>>> -       select SHA256
> > > > > > > > > >>>> -       select SHA384
> > > > > > > > > >>>> -       select SHA512
> > > > > > > > > >>>
> > > > > > > > > >>> I am not sure this is the right way to deal with your problem.
> > > > > > > > > >>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> > > > > > > > > >>> is really required. To make things even worse, you don't know the PCR
> > > > > > > > > >>> banks that are enabled beforehand. This is a runtime config of the
> > > > > > > > > >>> TPM.
> > > > > > > > > >>
> > > > > > > > > >> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> > > > > > > > > >> cannot extend PCRs. So it seems fine to let these two select the
> > > > > > > > > >> complete set of hashing algorithms. As Simon pointed out for
> > > > > > > > > >> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
> > > > > > > > > >
> > > > > > > > > > It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> > > > > > > > > > 0xb0000000
> > > > > > >
> > > > > > > That's pretty normal for U-Boot though, since we want to avoid lots of
> > > > > > > growth for things people might want control over. We can enable or
> > > > > > > disable the SHA for the board, if this functionality is used outside
> > > > > > > of measured boot and tcg2, but someone is enabling the tpm command.
> > > > > > >
> > > > > > > > >
> > > > > > > > > So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.
> > > > > > > > >
> > > > > > > > > TPM v1 only needs SHA-1.
> > > > > > > >
> > > > > > > > I still prefer to imply all algos.
> > > > > > >
> > > > > > > 'imply' would be OK in this case as I can disable it for that board. I
> > > > > > > don't think it is in the spirit of U-Boot though.
> > > > > > >
> > > > > > > isn't someone checking the growth in U-Boot? Or do so few boards have
> > > > > > > TPMs that it didn't register? The size growth was 3.2KB on
> > > > > > > chromebook_link.
> > > > > >
> > > > > > As always, yes, nearly every PR (I don't check the ones that touch just
> > > > > > a single board for example) gets a world build before/after. In this
> > > > > > case I likely assumed that it was acceptable growth for enabling
> > > > > > features. It sounds like some of the chromebook boards need to be
> > > > > > setting the features to cause link failure if a size is exceeded?
> > > > >
> > > > > The problem is that some Intel platforms have binary blobs, so the
> > > > > size isn't known unless you have a real blob.
> > > >
> > > > Alright, but can't we put in some limit based on what the current blobs
> > > > are, or look at the last few blob releases and see if they change much
> > > > in size and put something in? Other platforms have blobs and size
> > > > limits...
> > >
> > > Yes we can duplicate the entry sizes from binman into Kconfig options.
> > > But I am not a fan of that...two variables controlling the same thing
> > > and both can break, with nothing to connect them other than the poor
> > > user.
> > >
> > > I wonder if some magic could solve this, inferring limits from the
> > > binman image. But that is getting into yet another domain...
> >
> > I'm sorry, I just don't see what makes this case different from all of
> > the other cases where we have to include blobs. We know there's X amount
> > of flash available, blobs tend to take up Y and so we set
> > BOARD_SIZE_LIMIT to Z where there's some room for growth in U-Boot but
> > it takes in to account whatever limits the blobs place on us.
> 
> The difference is that now we are using Binman, we have the limit in
> the image description, so adding it to Kconfig a) involves
> calculations and perhaps guesswork and b) results in two limits stored
> in different places which may conflict.
> 
> I thought of a way to handle this in Binman, so will send that in the
> next version.

I'm still confused, sorry. This sounds like a whole lot more work than
setting BOARD_SIZE_LIMIT reasonably so that we fail to link and CI
errors out, before we get to buildman and needing to make this case
still fail but with fake blobs.

-- 
Tom

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

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Simon Glass]

Hi Tom,

On Thu, 20 Jun 2024 at 17:19, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Jun 20, 2024 at 05:05:29PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 19 Jun 2024 at 09:32, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Jun 18, 2024 at 09:03:37PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 18 Jun 2024 at 08:15, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Tue, Jun 18, 2024 at 06:43:51AM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Mon, 17 Jun 2024 at 11:16, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Mon, Jun 17, 2024 at 07:53:22AM -0600, Simon Glass wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Sat, 15 Jun 2024 at 01:03, Ilias Apalodimas
> > > > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Heinrich
> > > > > > > > >
> > > > > > > > > resending the reply, I accidentally sent half of the message...
> > > > > > > > >
> > > > > > > > > On Fri, 14 Jun 2024 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > > >
> > > > > > > > > > On 14.06.24 09:01, Ilias Apalodimas wrote:
> > > > > > > > > > > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > > > >>
> > > > > > > > > > >> On 6/14/24 08:03, Ilias Apalodimas wrote:
> > > > > > > > > > >>> Hi Simon,
> > > > > > > > > > >>>
> > > > > > > > > > >>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > >>>>
> > > > > > > > > > >>>> It does not make sense to enable all SHA algorithms unless they are
> > > > > > > > > > >>>> needed. It bloats the code and in this case, causes chromebook_link to
> > > > > > > > > > >>>> fail to build. That board does use the TPM, but not with measured boot,
> > > > > > > > > > >>>> nor EFI.
> > > > > > > > > > >>>>
> > > > > > > > > > >>>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> > > > > > > > > > >>>> add them to MEASURED_BOOT as well.
> > > > > > > > > > >>>>
> > > > > > > > > > >>>> Note that the original commit combines refactoring and new features,
> > > > > > > > > > >>>> which makes it hard to see what is going on.
> > > > > > > > > > >>>>
> > > > > > > > > > >>>> Fixes: 97707f12fda tpm: Support boot measurements
> > > > > > > > > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > >>>> ---
> > > > > > > > > > >>>>
> > > > > > > > > > >>>> Changes in v2:
> > > > > > > > > > >>>> - Put the conditions under EFI_TCG2_PROTOCOL
> > > > > > > > > > >>>> - Consider MEASURED_BOOT too
> > > > > > > > > > >>>>
> > > > > > > > > > >>>>    boot/Kconfig | 4 ++++
> > > > > > > > > > >>>>    lib/Kconfig  | 4 ----
> > > > > > > > > > >>>>    2 files changed, 4 insertions(+), 4 deletions(-)
> > > > > > > > > > >>>>
> > > > > > > > > > >>>> diff --git a/boot/Kconfig b/boot/Kconfig
> > > > > > > > > > >>>> index 6f3096c15a6..b061891e109 100644
> > > > > > > > > > >>>> --- a/boot/Kconfig
> > > > > > > > > > >>>> +++ b/boot/Kconfig
> > > > > > > > > > >>>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
> > > > > > > > > > >>>>    config MEASURED_BOOT
> > > > > > > > > > >>>>           bool "Measure boot images and configuration when booting without EFI"
> > > > > > > > > > >>>>           depends on HASH && TPM_V2
> > > > > > > > > > >>>> +       select SHA1
> > > > > > > > > > >>>> +       select SHA256
> > > > > > > > > > >>>> +       select SHA384
> > > > > > > > > > >>>> +       select SHA512
> > > > > > > > > > >>>>           help
> > > > > > > > > > >>>>             This option enables measurement of the boot process when booting
> > > > > > > > > > >>>>             without UEFI . Measurement involves creating cryptographic hashes
> > > > > > > > > > >>>> diff --git a/lib/Kconfig b/lib/Kconfig
> > > > > > > > > > >>>> index 189e6eb31aa..568892fce44 100644
> > > > > > > > > > >>>> --- a/lib/Kconfig
> > > > > > > > > > >>>> +++ b/lib/Kconfig
> > > > > > > > > > >>>> @@ -438,10 +438,6 @@ config TPM
> > > > > > > > > > >>>>           bool "Trusted Platform Module (TPM) Support"
> > > > > > > > > > >>>>           depends on DM
> > > > > > > > > > >>>>           imply DM_RNG
> > > > > > > > > > >>>> -       select SHA1
> > > > > > > > > > >>>> -       select SHA256
> > > > > > > > > > >>>> -       select SHA384
> > > > > > > > > > >>>> -       select SHA512
> > > > > > > > > > >>>
> > > > > > > > > > >>> I am not sure this is the right way to deal with your problem.
> > > > > > > > > > >>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> > > > > > > > > > >>> is really required. To make things even worse, you don't know the PCR
> > > > > > > > > > >>> banks that are enabled beforehand. This is a runtime config of the
> > > > > > > > > > >>> TPM.
> > > > > > > > > > >>
> > > > > > > > > > >> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> > > > > > > > > > >> cannot extend PCRs. So it seems fine to let these two select the
> > > > > > > > > > >> complete set of hashing algorithms. As Simon pointed out for
> > > > > > > > > > >> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
> > > > > > > > > > >
> > > > > > > > > > > It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> > > > > > > > > > > 0xb0000000
> > > > > > > >
> > > > > > > > That's pretty normal for U-Boot though, since we want to avoid lots of
> > > > > > > > growth for things people might want control over. We can enable or
> > > > > > > > disable the SHA for the board, if this functionality is used outside
> > > > > > > > of measured boot and tcg2, but someone is enabling the tpm command.
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.
> > > > > > > > > >
> > > > > > > > > > TPM v1 only needs SHA-1.
> > > > > > > > >
> > > > > > > > > I still prefer to imply all algos.
> > > > > > > >
> > > > > > > > 'imply' would be OK in this case as I can disable it for that board. I
> > > > > > > > don't think it is in the spirit of U-Boot though.
> > > > > > > >
> > > > > > > > isn't someone checking the growth in U-Boot? Or do so few boards have
> > > > > > > > TPMs that it didn't register? The size growth was 3.2KB on
> > > > > > > > chromebook_link.
> > > > > > >
> > > > > > > As always, yes, nearly every PR (I don't check the ones that touch just
> > > > > > > a single board for example) gets a world build before/after. In this
> > > > > > > case I likely assumed that it was acceptable growth for enabling
> > > > > > > features. It sounds like some of the chromebook boards need to be
> > > > > > > setting the features to cause link failure if a size is exceeded?
> > > > > >
> > > > > > The problem is that some Intel platforms have binary blobs, so the
> > > > > > size isn't known unless you have a real blob.
> > > > >
> > > > > Alright, but can't we put in some limit based on what the current blobs
> > > > > are, or look at the last few blob releases and see if they change much
> > > > > in size and put something in? Other platforms have blobs and size
> > > > > limits...
> > > >
> > > > Yes we can duplicate the entry sizes from binman into Kconfig options.
> > > > But I am not a fan of that...two variables controlling the same thing
> > > > and both can break, with nothing to connect them other than the poor
> > > > user.
> > > >
> > > > I wonder if some magic could solve this, inferring limits from the
> > > > binman image. But that is getting into yet another domain...
> > >
> > > I'm sorry, I just don't see what makes this case different from all of
> > > the other cases where we have to include blobs. We know there's X amount
> > > of flash available, blobs tend to take up Y and so we set
> > > BOARD_SIZE_LIMIT to Z where there's some room for growth in U-Boot but
> > > it takes in to account whatever limits the blobs place on us.
> >
> > The difference is that now we are using Binman, we have the limit in
> > the image description, so adding it to Kconfig a) involves
> > calculations and perhaps guesswork and b) results in two limits stored
> > in different places which may conflict.
> >
> > I thought of a way to handle this in Binman, so will send that in the
> > next version.
>
> I'm still confused, sorry. This sounds like a whole lot more work than
> setting BOARD_SIZE_LIMIT reasonably so that we fail to link and CI
> errors out, before we get to buildman and needing to make this case
> still fail but with fake blobs.

It is simpler for me, since I don't need to reverse-engineer the space
requirement for each board. With the patch I sent, I can just add some
reasonable numbers to each entry and it will do the right thing.

Regards,
Simon

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Tom Rini]

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

On Fri, Jun 21, 2024 at 08:57:51AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 20 Jun 2024 at 17:19, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Jun 20, 2024 at 05:05:29PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 19 Jun 2024 at 09:32, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Jun 18, 2024 at 09:03:37PM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Tue, 18 Jun 2024 at 08:15, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Tue, Jun 18, 2024 at 06:43:51AM -0600, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Mon, 17 Jun 2024 at 11:16, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Mon, Jun 17, 2024 at 07:53:22AM -0600, Simon Glass wrote:
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > On Sat, 15 Jun 2024 at 01:03, Ilias Apalodimas
> > > > > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Heinrich
> > > > > > > > > >
> > > > > > > > > > resending the reply, I accidentally sent half of the message...
> > > > > > > > > >
> > > > > > > > > > On Fri, 14 Jun 2024 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On 14.06.24 09:01, Ilias Apalodimas wrote:
> > > > > > > > > > > > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > > > > >>
> > > > > > > > > > > >> On 6/14/24 08:03, Ilias Apalodimas wrote:
> > > > > > > > > > > >>> Hi Simon,
> > > > > > > > > > > >>>
> > > > > > > > > > > >>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > >>>>
> > > > > > > > > > > >>>> It does not make sense to enable all SHA algorithms unless they are
> > > > > > > > > > > >>>> needed. It bloats the code and in this case, causes chromebook_link to
> > > > > > > > > > > >>>> fail to build. That board does use the TPM, but not with measured boot,
> > > > > > > > > > > >>>> nor EFI.
> > > > > > > > > > > >>>>
> > > > > > > > > > > >>>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> > > > > > > > > > > >>>> add them to MEASURED_BOOT as well.
> > > > > > > > > > > >>>>
> > > > > > > > > > > >>>> Note that the original commit combines refactoring and new features,
> > > > > > > > > > > >>>> which makes it hard to see what is going on.
> > > > > > > > > > > >>>>
> > > > > > > > > > > >>>> Fixes: 97707f12fda tpm: Support boot measurements
> > > > > > > > > > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > > >>>> ---
> > > > > > > > > > > >>>>
> > > > > > > > > > > >>>> Changes in v2:
> > > > > > > > > > > >>>> - Put the conditions under EFI_TCG2_PROTOCOL
> > > > > > > > > > > >>>> - Consider MEASURED_BOOT too
> > > > > > > > > > > >>>>
> > > > > > > > > > > >>>>    boot/Kconfig | 4 ++++
> > > > > > > > > > > >>>>    lib/Kconfig  | 4 ----
> > > > > > > > > > > >>>>    2 files changed, 4 insertions(+), 4 deletions(-)
> > > > > > > > > > > >>>>
> > > > > > > > > > > >>>> diff --git a/boot/Kconfig b/boot/Kconfig
> > > > > > > > > > > >>>> index 6f3096c15a6..b061891e109 100644
> > > > > > > > > > > >>>> --- a/boot/Kconfig
> > > > > > > > > > > >>>> +++ b/boot/Kconfig
> > > > > > > > > > > >>>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
> > > > > > > > > > > >>>>    config MEASURED_BOOT
> > > > > > > > > > > >>>>           bool "Measure boot images and configuration when booting without EFI"
> > > > > > > > > > > >>>>           depends on HASH && TPM_V2
> > > > > > > > > > > >>>> +       select SHA1
> > > > > > > > > > > >>>> +       select SHA256
> > > > > > > > > > > >>>> +       select SHA384
> > > > > > > > > > > >>>> +       select SHA512
> > > > > > > > > > > >>>>           help
> > > > > > > > > > > >>>>             This option enables measurement of the boot process when booting
> > > > > > > > > > > >>>>             without UEFI . Measurement involves creating cryptographic hashes
> > > > > > > > > > > >>>> diff --git a/lib/Kconfig b/lib/Kconfig
> > > > > > > > > > > >>>> index 189e6eb31aa..568892fce44 100644
> > > > > > > > > > > >>>> --- a/lib/Kconfig
> > > > > > > > > > > >>>> +++ b/lib/Kconfig
> > > > > > > > > > > >>>> @@ -438,10 +438,6 @@ config TPM
> > > > > > > > > > > >>>>           bool "Trusted Platform Module (TPM) Support"
> > > > > > > > > > > >>>>           depends on DM
> > > > > > > > > > > >>>>           imply DM_RNG
> > > > > > > > > > > >>>> -       select SHA1
> > > > > > > > > > > >>>> -       select SHA256
> > > > > > > > > > > >>>> -       select SHA384
> > > > > > > > > > > >>>> -       select SHA512
> > > > > > > > > > > >>>
> > > > > > > > > > > >>> I am not sure this is the right way to deal with your problem.
> > > > > > > > > > > >>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> > > > > > > > > > > >>> is really required. To make things even worse, you don't know the PCR
> > > > > > > > > > > >>> banks that are enabled beforehand. This is a runtime config of the
> > > > > > > > > > > >>> TPM.
> > > > > > > > > > > >>
> > > > > > > > > > > >> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> > > > > > > > > > > >> cannot extend PCRs. So it seems fine to let these two select the
> > > > > > > > > > > >> complete set of hashing algorithms. As Simon pointed out for
> > > > > > > > > > > >> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
> > > > > > > > > > > >
> > > > > > > > > > > > It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> > > > > > > > > > > > 0xb0000000
> > > > > > > > >
> > > > > > > > > That's pretty normal for U-Boot though, since we want to avoid lots of
> > > > > > > > > growth for things people might want control over. We can enable or
> > > > > > > > > disable the SHA for the board, if this functionality is used outside
> > > > > > > > > of measured boot and tcg2, but someone is enabling the tpm command.
> > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.
> > > > > > > > > > >
> > > > > > > > > > > TPM v1 only needs SHA-1.
> > > > > > > > > >
> > > > > > > > > > I still prefer to imply all algos.
> > > > > > > > >
> > > > > > > > > 'imply' would be OK in this case as I can disable it for that board. I
> > > > > > > > > don't think it is in the spirit of U-Boot though.
> > > > > > > > >
> > > > > > > > > isn't someone checking the growth in U-Boot? Or do so few boards have
> > > > > > > > > TPMs that it didn't register? The size growth was 3.2KB on
> > > > > > > > > chromebook_link.
> > > > > > > >
> > > > > > > > As always, yes, nearly every PR (I don't check the ones that touch just
> > > > > > > > a single board for example) gets a world build before/after. In this
> > > > > > > > case I likely assumed that it was acceptable growth for enabling
> > > > > > > > features. It sounds like some of the chromebook boards need to be
> > > > > > > > setting the features to cause link failure if a size is exceeded?
> > > > > > >
> > > > > > > The problem is that some Intel platforms have binary blobs, so the
> > > > > > > size isn't known unless you have a real blob.
> > > > > >
> > > > > > Alright, but can't we put in some limit based on what the current blobs
> > > > > > are, or look at the last few blob releases and see if they change much
> > > > > > in size and put something in? Other platforms have blobs and size
> > > > > > limits...
> > > > >
> > > > > Yes we can duplicate the entry sizes from binman into Kconfig options.
> > > > > But I am not a fan of that...two variables controlling the same thing
> > > > > and both can break, with nothing to connect them other than the poor
> > > > > user.
> > > > >
> > > > > I wonder if some magic could solve this, inferring limits from the
> > > > > binman image. But that is getting into yet another domain...
> > > >
> > > > I'm sorry, I just don't see what makes this case different from all of
> > > > the other cases where we have to include blobs. We know there's X amount
> > > > of flash available, blobs tend to take up Y and so we set
> > > > BOARD_SIZE_LIMIT to Z where there's some room for growth in U-Boot but
> > > > it takes in to account whatever limits the blobs place on us.
> > >
> > > The difference is that now we are using Binman, we have the limit in
> > > the image description, so adding it to Kconfig a) involves
> > > calculations and perhaps guesswork and b) results in two limits stored
> > > in different places which may conflict.
> > >
> > > I thought of a way to handle this in Binman, so will send that in the
> > > next version.
> >
> > I'm still confused, sorry. This sounds like a whole lot more work than
> > setting BOARD_SIZE_LIMIT reasonably so that we fail to link and CI
> > errors out, before we get to buildman and needing to make this case
> > still fail but with fake blobs.
> 
> It is simpler for me, since I don't need to reverse-engineer the space
> requirement for each board. With the patch I sent, I can just add some
> reasonable numbers to each entry and it will do the right thing.

Yes, I very much do not like guessing about 3 numbers instead of
guessing about 1 number and using the standard mechanism we already
have. Please use BOARD_SIZE_LIMIT as this is the standard mechanism to
enforce size limits on U-Boot itself.

-- 
Tom

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

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Simon Glass]

Hi Tom,

On Fri, 21 Jun 2024 at 10:05, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Jun 21, 2024 at 08:57:51AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 20 Jun 2024 at 17:19, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Jun 20, 2024 at 05:05:29PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Wed, 19 Jun 2024 at 09:32, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Tue, Jun 18, 2024 at 09:03:37PM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Tue, 18 Jun 2024 at 08:15, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Tue, Jun 18, 2024 at 06:43:51AM -0600, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Mon, 17 Jun 2024 at 11:16, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Jun 17, 2024 at 07:53:22AM -0600, Simon Glass wrote:
> > > > > > > > > > Hi,
> > > > > > > > > >
> > > > > > > > > > On Sat, 15 Jun 2024 at 01:03, Ilias Apalodimas
> > > > > > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Heinrich
> > > > > > > > > > >
> > > > > > > > > > > resending the reply, I accidentally sent half of the message...
> > > > > > > > > > >
> > > > > > > > > > > On Fri, 14 Jun 2024 at 12:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > On 14.06.24 09:01, Ilias Apalodimas wrote:
> > > > > > > > > > > > > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > > > > > >>
> > > > > > > > > > > > >> On 6/14/24 08:03, Ilias Apalodimas wrote:
> > > > > > > > > > > > >>> Hi Simon,
> > > > > > > > > > > > >>>
> > > > > > > > > > > > >>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > > >>>>
> > > > > > > > > > > > >>>> It does not make sense to enable all SHA algorithms unless they are
> > > > > > > > > > > > >>>> needed. It bloats the code and in this case, causes chromebook_link to
> > > > > > > > > > > > >>>> fail to build. That board does use the TPM, but not with measured boot,
> > > > > > > > > > > > >>>> nor EFI.
> > > > > > > > > > > > >>>>
> > > > > > > > > > > > >>>> Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> > > > > > > > > > > > >>>> add them to MEASURED_BOOT as well.
> > > > > > > > > > > > >>>>
> > > > > > > > > > > > >>>> Note that the original commit combines refactoring and new features,
> > > > > > > > > > > > >>>> which makes it hard to see what is going on.
> > > > > > > > > > > > >>>>
> > > > > > > > > > > > >>>> Fixes: 97707f12fda tpm: Support boot measurements
> > > > > > > > > > > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > > > >>>> ---
> > > > > > > > > > > > >>>>
> > > > > > > > > > > > >>>> Changes in v2:
> > > > > > > > > > > > >>>> - Put the conditions under EFI_TCG2_PROTOCOL
> > > > > > > > > > > > >>>> - Consider MEASURED_BOOT too
> > > > > > > > > > > > >>>>
> > > > > > > > > > > > >>>>    boot/Kconfig | 4 ++++
> > > > > > > > > > > > >>>>    lib/Kconfig  | 4 ----
> > > > > > > > > > > > >>>>    2 files changed, 4 insertions(+), 4 deletions(-)
> > > > > > > > > > > > >>>>
> > > > > > > > > > > > >>>> diff --git a/boot/Kconfig b/boot/Kconfig
> > > > > > > > > > > > >>>> index 6f3096c15a6..b061891e109 100644
> > > > > > > > > > > > >>>> --- a/boot/Kconfig
> > > > > > > > > > > > >>>> +++ b/boot/Kconfig
> > > > > > > > > > > > >>>> @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT
> > > > > > > > > > > > >>>>    config MEASURED_BOOT
> > > > > > > > > > > > >>>>           bool "Measure boot images and configuration when booting without EFI"
> > > > > > > > > > > > >>>>           depends on HASH && TPM_V2
> > > > > > > > > > > > >>>> +       select SHA1
> > > > > > > > > > > > >>>> +       select SHA256
> > > > > > > > > > > > >>>> +       select SHA384
> > > > > > > > > > > > >>>> +       select SHA512
> > > > > > > > > > > > >>>>           help
> > > > > > > > > > > > >>>>             This option enables measurement of the boot process when booting
> > > > > > > > > > > > >>>>             without UEFI . Measurement involves creating cryptographic hashes
> > > > > > > > > > > > >>>> diff --git a/lib/Kconfig b/lib/Kconfig
> > > > > > > > > > > > >>>> index 189e6eb31aa..568892fce44 100644
> > > > > > > > > > > > >>>> --- a/lib/Kconfig
> > > > > > > > > > > > >>>> +++ b/lib/Kconfig
> > > > > > > > > > > > >>>> @@ -438,10 +438,6 @@ config TPM
> > > > > > > > > > > > >>>>           bool "Trusted Platform Module (TPM) Support"
> > > > > > > > > > > > >>>>           depends on DM
> > > > > > > > > > > > >>>>           imply DM_RNG
> > > > > > > > > > > > >>>> -       select SHA1
> > > > > > > > > > > > >>>> -       select SHA256
> > > > > > > > > > > > >>>> -       select SHA384
> > > > > > > > > > > > >>>> -       select SHA512
> > > > > > > > > > > > >>>
> > > > > > > > > > > > >>> I am not sure this is the right way to deal with your problem.
> > > > > > > > > > > > >>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> > > > > > > > > > > > >>> is really required. To make things even worse, you don't know the PCR
> > > > > > > > > > > > >>> banks that are enabled beforehand. This is a runtime config of the
> > > > > > > > > > > > >>> TPM.
> > > > > > > > > > > > >>
> > > > > > > > > > > > >> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> > > > > > > > > > > > >> cannot extend PCRs. So it seems fine to let these two select the
> > > > > > > > > > > > >> complete set of hashing algorithms. As Simon pointed out for
> > > > > > > > > > > > >> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
> > > > > > > > > > > > >
> > > > > > > > > > > > > It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> > > > > > > > > > > > > 0xb0000000
> > > > > > > > > >
> > > > > > > > > > That's pretty normal for U-Boot though, since we want to avoid lots of
> > > > > > > > > > growth for things people might want control over. We can enable or
> > > > > > > > > > disable the SHA for the board, if this functionality is used outside
> > > > > > > > > > of measured boot and tcg2, but someone is enabling the tpm command.
> > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.
> > > > > > > > > > > >
> > > > > > > > > > > > TPM v1 only needs SHA-1.
> > > > > > > > > > >
> > > > > > > > > > > I still prefer to imply all algos.
> > > > > > > > > >
> > > > > > > > > > 'imply' would be OK in this case as I can disable it for that board. I
> > > > > > > > > > don't think it is in the spirit of U-Boot though.
> > > > > > > > > >
> > > > > > > > > > isn't someone checking the growth in U-Boot? Or do so few boards have
> > > > > > > > > > TPMs that it didn't register? The size growth was 3.2KB on
> > > > > > > > > > chromebook_link.
> > > > > > > > >
> > > > > > > > > As always, yes, nearly every PR (I don't check the ones that touch just
> > > > > > > > > a single board for example) gets a world build before/after. In this
> > > > > > > > > case I likely assumed that it was acceptable growth for enabling
> > > > > > > > > features. It sounds like some of the chromebook boards need to be
> > > > > > > > > setting the features to cause link failure if a size is exceeded?
> > > > > > > >
> > > > > > > > The problem is that some Intel platforms have binary blobs, so the
> > > > > > > > size isn't known unless you have a real blob.
> > > > > > >
> > > > > > > Alright, but can't we put in some limit based on what the current blobs
> > > > > > > are, or look at the last few blob releases and see if they change much
> > > > > > > in size and put something in? Other platforms have blobs and size
> > > > > > > limits...
> > > > > >
> > > > > > Yes we can duplicate the entry sizes from binman into Kconfig options.
> > > > > > But I am not a fan of that...two variables controlling the same thing
> > > > > > and both can break, with nothing to connect them other than the poor
> > > > > > user.
> > > > > >
> > > > > > I wonder if some magic could solve this, inferring limits from the
> > > > > > binman image. But that is getting into yet another domain...
> > > > >
> > > > > I'm sorry, I just don't see what makes this case different from all of
> > > > > the other cases where we have to include blobs. We know there's X amount
> > > > > of flash available, blobs tend to take up Y and so we set
> > > > > BOARD_SIZE_LIMIT to Z where there's some room for growth in U-Boot but
> > > > > it takes in to account whatever limits the blobs place on us.
> > > >
> > > > The difference is that now we are using Binman, we have the limit in
> > > > the image description, so adding it to Kconfig a) involves
> > > > calculations and perhaps guesswork and b) results in two limits stored
> > > > in different places which may conflict.
> > > >
> > > > I thought of a way to handle this in Binman, so will send that in the
> > > > next version.
> > >
> > > I'm still confused, sorry. This sounds like a whole lot more work than
> > > setting BOARD_SIZE_LIMIT reasonably so that we fail to link and CI
> > > errors out, before we get to buildman and needing to make this case
> > > still fail but with fake blobs.
> >
> > It is simpler for me, since I don't need to reverse-engineer the space
> > requirement for each board. With the patch I sent, I can just add some
> > reasonable numbers to each entry and it will do the right thing.
>
> Yes, I very much do not like guessing about 3 numbers instead of
> guessing about 1 number and using the standard mechanism we already
> have. Please use BOARD_SIZE_LIMIT as this is the standard mechanism to
> enforce size limits on U-Boot itself.

If it were that easy I would have sent a patch :-)

Here is the map for this board:

ImagePos    Offset      Size  Name
00000000  00000000  00800000  rom
ff800000   ff800000  00001000  intel-descriptor
ff801000   ff801000  001ff000  intel-me
ffef0000   ffef0000  000999f0  u-boot-with-ucode-ptr
fff899f0   fff899f0  00005554  u-boot-dtb-with-ucode
fff8ef50   fff8ef50  00000000  u-boot-ucode
fff8ef50   fff8ef50  00000571  fdtmap
fff90000   fff90000  00010000  intel-vga
fffa0000   fffa0000  0002fc94  intel-mrc
fffcfc94   fffcfc94  00000000  private-files
fffff800   fffff800  00000070  x86-start16
fffffff0   fffffff0  00000005  x86-reset16
fffffff8   fffffff8  00000008  image-header

What limit should I set on what?

- the U-Boot is the thing you are wanting to limit
- the dtb has microcode added
- the ucode is empty in this case
- the fdtmap is variable in size

So this all seems a bit backwards. The actual limit is that
(u-boot-with-ucode-ptr + u-boot-dtb-with-ucode + u-boot-ucode +
fdtmap) fits in the space available. Note that some boards don't have
intel-vga or intel-mrc.

With the other patch I sent I can have a sensible limit for all x86 boards.

Regards,
Simon

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Tom Rini]

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

On Fri, Jun 21, 2024 at 11:55:42AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 21 Jun 2024 at 10:05, Tom Rini <trini@konsulko.com> wrote:
[snip]
> > Yes, I very much do not like guessing about 3 numbers instead of
> > guessing about 1 number and using the standard mechanism we already
> > have. Please use BOARD_SIZE_LIMIT as this is the standard mechanism to
> > enforce size limits on U-Boot itself.
> 
> If it were that easy I would have sent a patch :-)
> 
> Here is the map for this board:
> 
> ImagePos    Offset      Size  Name
> 00000000  00000000  00800000  rom
> ff800000   ff800000  00001000  intel-descriptor
> ff801000   ff801000  001ff000  intel-me
> ffef0000   ffef0000  000999f0  u-boot-with-ucode-ptr
> fff899f0   fff899f0  00005554  u-boot-dtb-with-ucode
> fff8ef50   fff8ef50  00000000  u-boot-ucode
> fff8ef50   fff8ef50  00000571  fdtmap
> fff90000   fff90000  00010000  intel-vga
> fffa0000   fffa0000  0002fc94  intel-mrc
> fffcfc94   fffcfc94  00000000  private-files
> fffff800   fffff800  00000070  x86-start16
> fffffff0   fffffff0  00000005  x86-reset16
> fffffff8   fffffff8  00000008  image-header
> 
> What limit should I set on what?

Is this a trick question?
$ printf %d\\n $(( 0xfff90000 - 0xffef0000))
655360

Of course since we're less than that today, you can reduce it by
whatever other magic numbers I'm not seeing but are part of your assumed
sizes.

> - the U-Boot is the thing you are wanting to limit
> - the dtb has microcode added
> - the ucode is empty in this case
> - the fdtmap is variable in size
> 
> So this all seems a bit backwards. The actual limit is that
> (u-boot-with-ucode-ptr + u-boot-dtb-with-ucode + u-boot-ucode +
> fdtmap) fits in the space available. Note that some boards don't have
> intel-vga or intel-mrc.
> 
> With the other patch I sent I can have a sensible limit for all x86 boards.

And you can set the same sensible limit with the existing mechanism with
the bonus of it not making x86 different from the rest?

-- 
Tom

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

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Simon Glass]

Hi Tom,

On Fri, 21 Jun 2024 at 13:19, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Jun 21, 2024 at 11:55:42AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 21 Jun 2024 at 10:05, Tom Rini <trini@konsulko.com> wrote:
> [snip]
> > > Yes, I very much do not like guessing about 3 numbers instead of
> > > guessing about 1 number and using the standard mechanism we already
> > > have. Please use BOARD_SIZE_LIMIT as this is the standard mechanism to
> > > enforce size limits on U-Boot itself.
> >
> > If it were that easy I would have sent a patch :-)
> >
> > Here is the map for this board:
> >
> > ImagePos    Offset      Size  Name
> > 00000000  00000000  00800000  rom
> > ff800000   ff800000  00001000  intel-descriptor
> > ff801000   ff801000  001ff000  intel-me
> > ffef0000   ffef0000  000999f0  u-boot-with-ucode-ptr
> > fff899f0   fff899f0  00005554  u-boot-dtb-with-ucode
> > fff8ef50   fff8ef50  00000000  u-boot-ucode
> > fff8ef50   fff8ef50  00000571  fdtmap
> > fff90000   fff90000  00010000  intel-vga
> > fffa0000   fffa0000  0002fc94  intel-mrc
> > fffcfc94   fffcfc94  00000000  private-files
> > fffff800   fffff800  00000070  x86-start16
> > fffffff0   fffffff0  00000005  x86-reset16
> > fffffff8   fffffff8  00000008  image-header
> >
> > What limit should I set on what?
>
> Is this a trick question?
> $ printf %d\\n $(( 0xfff90000 - 0xffef0000))
> 655360
>
> Of course since we're less than that today, you can reduce it by
> whatever other magic numbers I'm not seeing but are part of your assumed
> sizes.

That limit is on u-boot-nodtb.bin. Even with a size (for that file) of
634816 it doesn't fit. I need to calculate a size based on the size of
the dtb and the microcode...which of course can change.

>
> > - the U-Boot is the thing you are wanting to limit
> > - the dtb has microcode added
> > - the ucode is empty in this case
> > - the fdtmap is variable in size
> >
> > So this all seems a bit backwards. The actual limit is that
> > (u-boot-with-ucode-ptr + u-boot-dtb-with-ucode + u-boot-ucode +
> > fdtmap) fits in the space available. Note that some boards don't have
> > intel-vga or intel-mrc.
> >
> > With the other patch I sent I can have a sensible limit for all x86 boards.

Did you miss the comments above?

>
> And you can set the same sensible limit with the existing mechanism with
> the bonus of it not making x86 different from the rest?

I understand that it is possible to set a limit for u-boot-nodtb.bin
but that is not accurate nor sufficient in the presence of blobs. The
solution belongs in Binman.

Regards,
Simon

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Tom Rini]

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

On Fri, Jun 21, 2024 at 01:38:07PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 21 Jun 2024 at 13:19, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Jun 21, 2024 at 11:55:42AM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 21 Jun 2024 at 10:05, Tom Rini <trini@konsulko.com> wrote:
> > [snip]
> > > > Yes, I very much do not like guessing about 3 numbers instead of
> > > > guessing about 1 number and using the standard mechanism we already
> > > > have. Please use BOARD_SIZE_LIMIT as this is the standard mechanism to
> > > > enforce size limits on U-Boot itself.
> > >
> > > If it were that easy I would have sent a patch :-)
> > >
> > > Here is the map for this board:
> > >
> > > ImagePos    Offset      Size  Name
> > > 00000000  00000000  00800000  rom
> > > ff800000   ff800000  00001000  intel-descriptor
> > > ff801000   ff801000  001ff000  intel-me
> > > ffef0000   ffef0000  000999f0  u-boot-with-ucode-ptr
> > > fff899f0   fff899f0  00005554  u-boot-dtb-with-ucode
> > > fff8ef50   fff8ef50  00000000  u-boot-ucode
> > > fff8ef50   fff8ef50  00000571  fdtmap
> > > fff90000   fff90000  00010000  intel-vga
> > > fffa0000   fffa0000  0002fc94  intel-mrc
> > > fffcfc94   fffcfc94  00000000  private-files
> > > fffff800   fffff800  00000070  x86-start16
> > > fffffff0   fffffff0  00000005  x86-reset16
> > > fffffff8   fffffff8  00000008  image-header
> > >
> > > What limit should I set on what?
> >
> > Is this a trick question?
> > $ printf %d\\n $(( 0xfff90000 - 0xffef0000))
> > 655360
> >
> > Of course since we're less than that today, you can reduce it by
> > whatever other magic numbers I'm not seeing but are part of your assumed
> > sizes.
> 
> That limit is on u-boot-nodtb.bin. Even with a size (for that file) of
> 634816 it doesn't fit. I need to calculate a size based on the size of
> the dtb and the microcode...which of course can change.

Yes, and you're able to assume some size for them, which is what you
put in the dts file?

> > > - the U-Boot is the thing you are wanting to limit
> > > - the dtb has microcode added
> > > - the ucode is empty in this case
> > > - the fdtmap is variable in size
> > >
> > > So this all seems a bit backwards. The actual limit is that
> > > (u-boot-with-ucode-ptr + u-boot-dtb-with-ucode + u-boot-ucode +
> > > fdtmap) fits in the space available. Note that some boards don't have
> > > intel-vga or intel-mrc.
> > >
> > > With the other patch I sent I can have a sensible limit for all x86 boards.
> 
> Did you miss the comments above?

No, I saw them. They're similar constraints to other systems.
> 
> >
> > And you can set the same sensible limit with the existing mechanism with
> > the bonus of it not making x86 different from the rest?
> 
> I understand that it is possible to set a limit for u-boot-nodtb.bin
> but that is not accurate nor sufficient in the presence of blobs. The
> solution belongs in Binman.

Your series puts reasonable estimates on the size of the blobs and then
will give a failure such as:
binman: Node '/binman/rom/intel-vga': Offset 0xfff90000 (4294508544)
overlaps with previous entry '/binman/rom/fdtmap' ending at 0xfff902e1
(4294509281)
Which is not as nice as (I just threw in a limit):
u-boot-nodtb.bin exceeds file size limit:
  limit:  0x927c0 bytes
  actual: 0x9a810 bytes
  excess: 0x8050 bytes
make[1]: *** [/home/trini/work/u-boot/u-boot/Makefile:1359: u-boot-nodtb.bin] Error 1

And tells us how much we need to get back size wise. Aside from when
using the actual blobs (and in which case a real error will be shown
when trying to use them), it's always about making an estimate on the
part of the system that we control.

-- 
Tom

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

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Simon Glass]

Hi Tom,

On Fri, 21 Jun 2024 at 16:12, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Jun 21, 2024 at 01:38:07PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 21 Jun 2024 at 13:19, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Jun 21, 2024 at 11:55:42AM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Fri, 21 Jun 2024 at 10:05, Tom Rini <trini@konsulko.com> wrote:
> > > [snip]
> > > > > Yes, I very much do not like guessing about 3 numbers instead of
> > > > > guessing about 1 number and using the standard mechanism we already
> > > > > have. Please use BOARD_SIZE_LIMIT as this is the standard mechanism to
> > > > > enforce size limits on U-Boot itself.
> > > >
> > > > If it were that easy I would have sent a patch :-)
> > > >
> > > > Here is the map for this board:
> > > >
> > > > ImagePos    Offset      Size  Name
> > > > 00000000  00000000  00800000  rom
> > > > ff800000   ff800000  00001000  intel-descriptor
> > > > ff801000   ff801000  001ff000  intel-me
> > > > ffef0000   ffef0000  000999f0  u-boot-with-ucode-ptr
> > > > fff899f0   fff899f0  00005554  u-boot-dtb-with-ucode
> > > > fff8ef50   fff8ef50  00000000  u-boot-ucode
> > > > fff8ef50   fff8ef50  00000571  fdtmap
> > > > fff90000   fff90000  00010000  intel-vga
> > > > fffa0000   fffa0000  0002fc94  intel-mrc
> > > > fffcfc94   fffcfc94  00000000  private-files
> > > > fffff800   fffff800  00000070  x86-start16
> > > > fffffff0   fffffff0  00000005  x86-reset16
> > > > fffffff8   fffffff8  00000008  image-header
> > > >
> > > > What limit should I set on what?
> > >
> > > Is this a trick question?
> > > $ printf %d\\n $(( 0xfff90000 - 0xffef0000))
> > > 655360
> > >
> > > Of course since we're less than that today, you can reduce it by
> > > whatever other magic numbers I'm not seeing but are part of your assumed
> > > sizes.
> >
> > That limit is on u-boot-nodtb.bin. Even with a size (for that file) of
> > 634816 it doesn't fit. I need to calculate a size based on the size of
> > the dtb and the microcode...which of course can change.
>
> Yes, and you're able to assume some size for them, which is what you
> put in the dts file?

I just put in a limit for the blobs, whose sizes are known.

>
> > > > - the U-Boot is the thing you are wanting to limit
> > > > - the dtb has microcode added
> > > > - the ucode is empty in this case
> > > > - the fdtmap is variable in size
> > > >
> > > > So this all seems a bit backwards. The actual limit is that
> > > > (u-boot-with-ucode-ptr + u-boot-dtb-with-ucode + u-boot-ucode +
> > > > fdtmap) fits in the space available. Note that some boards don't have
> > > > intel-vga or intel-mrc.
> > > >
> > > > With the other patch I sent I can have a sensible limit for all x86 boards.
> >
> > Did you miss the comments above?
>
> No, I saw them. They're similar constraints to other systems.
> >
> > >
> > > And you can set the same sensible limit with the existing mechanism with
> > > the bonus of it not making x86 different from the rest?
> >
> > I understand that it is possible to set a limit for u-boot-nodtb.bin
> > but that is not accurate nor sufficient in the presence of blobs. The
> > solution belongs in Binman.
>
> Your series puts reasonable estimates on the size of the blobs and then
> will give a failure such as:
> binman: Node '/binman/rom/intel-vga': Offset 0xfff90000 (4294508544)
> overlaps with previous entry '/binman/rom/fdtmap' ending at 0xfff902e1
> (4294509281)
> Which is not as nice as (I just threw in a limit):
> u-boot-nodtb.bin exceeds file size limit:
>   limit:  0x927c0 bytes
>   actual: 0x9a810 bytes
>   excess: 0x8050 bytes
> make[1]: *** [/home/trini/work/u-boot/u-boot/Makefile:1359: u-boot-nodtb.bin] Error 1
>
> And tells us how much we need to get back size wise. Aside from when
> using the actual blobs (and in which case a real error will be shown
> when trying to use them), it's always about making an estimate on the
> part of the system that we control.

Thanks for digging into this.

I wasn't aware that the limit was just an estimate. Since it produces
a build error it seems to be enforced as a hard requirement. But I can
set a limit and we'll see how things go.

I still like the binman block-size thing though. The error message
binman provides actually tells you what to do (reduce the space of the
things that are overflowing into intel-vga). So I'd like to add that
too.

Regards,
Simon

Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Tom Rini]

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

On Sun, Jun 23, 2024 at 03:52:12PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 21 Jun 2024 at 16:12, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Jun 21, 2024 at 01:38:07PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 21 Jun 2024 at 13:19, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Fri, Jun 21, 2024 at 11:55:42AM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Fri, 21 Jun 2024 at 10:05, Tom Rini <trini@konsulko.com> wrote:
> > > > [snip]
> > > > > > Yes, I very much do not like guessing about 3 numbers instead of
> > > > > > guessing about 1 number and using the standard mechanism we already
> > > > > > have. Please use BOARD_SIZE_LIMIT as this is the standard mechanism to
> > > > > > enforce size limits on U-Boot itself.
> > > > >
> > > > > If it were that easy I would have sent a patch :-)
> > > > >
> > > > > Here is the map for this board:
> > > > >
> > > > > ImagePos    Offset      Size  Name
> > > > > 00000000  00000000  00800000  rom
> > > > > ff800000   ff800000  00001000  intel-descriptor
> > > > > ff801000   ff801000  001ff000  intel-me
> > > > > ffef0000   ffef0000  000999f0  u-boot-with-ucode-ptr
> > > > > fff899f0   fff899f0  00005554  u-boot-dtb-with-ucode
> > > > > fff8ef50   fff8ef50  00000000  u-boot-ucode
> > > > > fff8ef50   fff8ef50  00000571  fdtmap
> > > > > fff90000   fff90000  00010000  intel-vga
> > > > > fffa0000   fffa0000  0002fc94  intel-mrc
> > > > > fffcfc94   fffcfc94  00000000  private-files
> > > > > fffff800   fffff800  00000070  x86-start16
> > > > > fffffff0   fffffff0  00000005  x86-reset16
> > > > > fffffff8   fffffff8  00000008  image-header
> > > > >
> > > > > What limit should I set on what?
> > > >
> > > > Is this a trick question?
> > > > $ printf %d\\n $(( 0xfff90000 - 0xffef0000))
> > > > 655360
> > > >
> > > > Of course since we're less than that today, you can reduce it by
> > > > whatever other magic numbers I'm not seeing but are part of your assumed
> > > > sizes.
> > >
> > > That limit is on u-boot-nodtb.bin. Even with a size (for that file) of
> > > 634816 it doesn't fit. I need to calculate a size based on the size of
> > > the dtb and the microcode...which of course can change.
> >
> > Yes, and you're able to assume some size for them, which is what you
> > put in the dts file?
> 
> I just put in a limit for the blobs, whose sizes are known.
> 
> >
> > > > > - the U-Boot is the thing you are wanting to limit
> > > > > - the dtb has microcode added
> > > > > - the ucode is empty in this case
> > > > > - the fdtmap is variable in size
> > > > >
> > > > > So this all seems a bit backwards. The actual limit is that
> > > > > (u-boot-with-ucode-ptr + u-boot-dtb-with-ucode + u-boot-ucode +
> > > > > fdtmap) fits in the space available. Note that some boards don't have
> > > > > intel-vga or intel-mrc.
> > > > >
> > > > > With the other patch I sent I can have a sensible limit for all x86 boards.
> > >
> > > Did you miss the comments above?
> >
> > No, I saw them. They're similar constraints to other systems.
> > >
> > > >
> > > > And you can set the same sensible limit with the existing mechanism with
> > > > the bonus of it not making x86 different from the rest?
> > >
> > > I understand that it is possible to set a limit for u-boot-nodtb.bin
> > > but that is not accurate nor sufficient in the presence of blobs. The
> > > solution belongs in Binman.
> >
> > Your series puts reasonable estimates on the size of the blobs and then
> > will give a failure such as:
> > binman: Node '/binman/rom/intel-vga': Offset 0xfff90000 (4294508544)
> > overlaps with previous entry '/binman/rom/fdtmap' ending at 0xfff902e1
> > (4294509281)
> > Which is not as nice as (I just threw in a limit):
> > u-boot-nodtb.bin exceeds file size limit:
> >   limit:  0x927c0 bytes
> >   actual: 0x9a810 bytes
> >   excess: 0x8050 bytes
> > make[1]: *** [/home/trini/work/u-boot/u-boot/Makefile:1359: u-boot-nodtb.bin] Error 1
> >
> > And tells us how much we need to get back size wise. Aside from when
> > using the actual blobs (and in which case a real error will be shown
> > when trying to use them), it's always about making an estimate on the
> > part of the system that we control.
> 
> Thanks for digging into this.
> 
> I wasn't aware that the limit was just an estimate. Since it produces
> a build error it seems to be enforced as a hard requirement. But I can
> set a limit and we'll see how things go.
> 
> I still like the binman block-size thing though. The error message
> binman provides actually tells you what to do (reduce the space of the
> things that are overflowing into intel-vga). So I'd like to add that
> too.

I think this is a reasonable compromise, thank you.

-- 
Tom

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

[PATCH v2 3/9] rockchip: veyron: Add logging for power init

[Simon Glass]

Add better logging for power init so that CONFIG_LOG_ERROR_RETURN can
be enabled.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Remove the superfluous if() and drop the debug() as well

 board/google/veyron/veyron.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/board/google/veyron/veyron.c b/board/google/veyron/veyron.c
index 32dbcdc4d10..6d4c9debdee 100644
--- a/board/google/veyron/veyron.c
+++ b/board/google/veyron/veyron.c
@@ -29,44 +29,38 @@ static int veyron_init(void)
 	int ret;
 
 	ret = regulator_get_by_platname("vdd_arm", &dev);
-	if (ret) {
-		debug("Cannot set regulator name\n");
-		return ret;
-	}
+	if (ret)
+		return log_msg_ret("vdd", ret);
 
 	/* Slowly raise to max CPU voltage to prevent overshoot */
 	ret = regulator_set_value(dev, 1200000);
 	if (ret)
-		return ret;
+		return log_msg_ret("s12", ret);
 	udelay(175); /* Must wait for voltage to stabilize, 2mV/us */
 	ret = regulator_set_value(dev, 1400000);
 	if (ret)
-		return ret;
+		return log_msg_ret("s14", ret);
 	udelay(100); /* Must wait for voltage to stabilize, 2mV/us */
 
 	ret = rockchip_get_clk(&clk.dev);
 	if (ret)
-		return ret;
+		return log_msg_ret("clk", ret);
 	clk.id = PLL_APLL;
 	ret = clk_set_rate(&clk, 1800000000);
 	if (IS_ERR_VALUE(ret))
-		return ret;
+		return log_msg_ret("s18", ret);
 
 	ret = regulator_get_by_platname("vcc33_sd", &dev);
-	if (ret) {
-		debug("Cannot get regulator name\n");
-		return ret;
-	}
+	if (ret)
+		return log_msg_ret("vcc", ret);
 
 	ret = regulator_set_value(dev, 3300000);
 	if (ret)
-		return ret;
+		return log_msg_ret("s33", ret);
 
 	ret = regulators_enable_boot_on(false);
-	if (ret) {
-		debug("%s: Cannot enable boot on regulators\n", __func__);
-		return ret;
-	}
+	if (ret)
+		return log_msg_ret("boo", ret);
 
 	return 0;
 }
@@ -81,7 +75,7 @@ int board_early_init_r(void)
 	if (!fdt_node_check_compatible(gd->fdt_blob, 0, "google,veyron")) {
 		ret = veyron_init();
 		if (ret)
-			return ret;
+			return log_msg_ret("vey", ret);
 	}
 #endif
 	/*
-- 
2.34.1

Re: [PATCH v2 3/9] rockchip: veyron: Add logging for power init

[Quentin Schulz]

Hi Simon,

On 6/10/24 4:59 PM, Simon Glass wrote:
> Add better logging for power init so that CONFIG_LOG_ERROR_RETURN can
> be enabled.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

[PATCH v2 4/9] power: regulator: Handle autoset in regulators_enable_boot_on()

[Simon Glass]

With a recent change, regulators_enable_boot_on() returns an error if a
regulator is already set. Check for and handle this situation.

Fixes: d99fb64a98a power: regulator: Only run autoset once for each regulator
Reviewed-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 drivers/power/regulator/regulator-uclass.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
index 77d101f262e..d9e1fb68295 100644
--- a/drivers/power/regulator/regulator-uclass.c
+++ b/drivers/power/regulator/regulator-uclass.c
@@ -518,7 +518,7 @@ int regulators_enable_boot_on(bool verbose)
 	     dev;
 	     uclass_next_device(&dev)) {
 		ret = regulator_autoset(dev);
-		if (ret == -EMEDIUMTYPE) {
+		if (ret == -EMEDIUMTYPE || ret == -EALREADY) {
 			ret = 0;
 			continue;
 		}
-- 
2.34.1

[PATCH v2 5/9] fdt: Correct condition for bloblist existing

[Simon Glass]

On some boards, the bloblist is created in SPL once SDRAM is ready. It
cannot be accessed until that point, so is not available early in SPL.

Add a condition to avoid a hang in this case.

This fixes a hang in chromebook_coral

Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist")

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Use 'phase' instead of 'stage'

 lib/fdtdec.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index b2c59ab3818..79eaa56ea39 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1669,8 +1669,16 @@ int fdtdec_setup(void)
 {
 	int ret = -ENOENT;
 
-	/* If allowing a bloblist, check that first */
-	if (CONFIG_IS_ENABLED(BLOBLIST)) {
+	/*
+	 * If allowing a bloblist, check that first. This would be better
+	 * handled with an OF_BLOBLIST Kconfig, but that caused far too much
+	 * argument, so add a hack here, used e.g. by chromebook_coral
+	 * The necessary test is whether the previous phase passed a bloblist,
+	 * not whether this phase creates one.
+	 */
+	if (CONFIG_IS_ENABLED(OF_BLOBLIST) &&
+	    (spl_prev_phase() != PHASE_TPL ||
+	     !IS_ENABLED(CONFIG_TPL_BLOBLIST))) {
 		ret = bloblist_maybe_init();
 		if (!ret) {
 			gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
-- 
2.34.1

[PATCH v2 6/9] spl: Allow ATF to work when dcache is disabled

[Simon Glass]

The dcache may not be enabled in SPL. Add a check to avoid trying to
use an undefined function.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Tom Rini <trini@konsulko.com>
---

(no changes since v1)

 common/spl/spl_atf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/common/spl/spl_atf.c b/common/spl/spl_atf.c
index 3bdd013a35f..9afe6456bc4 100644
--- a/common/spl/spl_atf.c
+++ b/common/spl/spl_atf.c
@@ -204,7 +204,8 @@ static void __noreturn bl31_entry(uintptr_t bl31_entry, uintptr_t bl32_entry,
 						       fdt_addr);
 
 	raw_write_daif(SPSR_EXCEPTION_MASK);
-	dcache_disable();
+	if (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF))
+		dcache_disable();
 
 	atf_entry(bl31_params, (void *)fdt_addr);
 }
-- 
2.34.1

[PATCH v2 7/9] rockchip: Ensure memory size is available in RK3399 SPL

[Simon Glass]

At present gd->ram_size is 0 in SPL, meaning that it is not possible to
enable the cache. Correct this by always populating the RAM size
correctly.

Part of the confusion here comes from the large blocks of code which
are #ifdefed out. Add a function phase_sdram_init() which returns
whether SDRAM init should happen in the current phase, using that as
needed to control the code flow.

This increases code size by about 500 bytes in SPL when the cache is on,
since it must call the rather large rockchip_sdram_size() function.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Add new patch to correct memory size in SPL

 drivers/ram/rockchip/sdram_rk3399.c | 49 ++++++++++++++++-------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
index 02cc4a38cf0..2f37dd712e7 100644
--- a/drivers/ram/rockchip/sdram_rk3399.c
+++ b/drivers/ram/rockchip/sdram_rk3399.c
@@ -13,6 +13,7 @@
 #include <log.h>
 #include <ram.h>
 #include <regmap.h>
+#include <spl.h>
 #include <syscon.h>
 #include <asm/arch-rockchip/clock.h>
 #include <asm/arch-rockchip/cru.h>
@@ -63,8 +64,6 @@ struct chan_info {
 };
 
 struct dram_info {
-#if defined(CONFIG_TPL_BUILD) || \
-	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
 	u32 pwrup_srefresh_exit[2];
 	struct chan_info chan[2];
 	struct clk ddr_clk;
@@ -75,7 +74,6 @@ struct dram_info {
 	struct rk3399_pmusgrf_regs *pmusgrf;
 	struct rk3399_ddr_cic_regs *cic;
 	const struct sdram_rk3399_ops *ops;
-#endif
 	struct ram_info info;
 	struct rk3399_pmugrf_regs *pmugrf;
 };
@@ -92,9 +90,6 @@ struct sdram_rk3399_ops {
 					struct rk3399_sdram_params *params);
 };
 
-#if defined(CONFIG_TPL_BUILD) || \
-	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
-
 struct rockchip_dmc_plat {
 #if CONFIG_IS_ENABLED(OF_PLATDATA)
 	struct dtd_rockchip_rk3399_dmc dtplat;
@@ -191,6 +186,17 @@ struct io_setting {
 	},
 };
 
+/**
+ * phase_sdram_init() - Check if this is the phase where SDRAM init happens
+ *
+ * Returns: true to do SDRAM init in this phase, false to not
+ */
+static bool phase_sdram_init(void)
+{
+	return spl_phase() == PHASE_TPL ||
+	    (!IS_ENABLED(CONFIG_TPL) && !spl_in_proper());
+}
+
 static struct io_setting *
 lpddr4_get_io_settings(const struct rk3399_sdram_params *params, u32 mr5)
 {
@@ -3024,7 +3030,7 @@ static int rk3399_dmc_of_to_plat(struct udevice *dev)
 	struct rockchip_dmc_plat *plat = dev_get_plat(dev);
 	int ret;
 
-	if (!CONFIG_IS_ENABLED(OF_REAL))
+	if (!CONFIG_IS_ENABLED(OF_REAL) || !phase_sdram_init())
 		return 0;
 
 	ret = dev_read_u32_array(dev, "rockchip,sdram-params",
@@ -3138,23 +3144,25 @@ static int rk3399_dmc_init(struct udevice *dev)
 
 	return 0;
 }
-#endif
 
 static int rk3399_dmc_probe(struct udevice *dev)
 {
-#if defined(CONFIG_TPL_BUILD) || \
-	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
-	if (rk3399_dmc_init(dev))
-		return 0;
-#else
 	struct dram_info *priv = dev_get_priv(dev);
 
-	priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
-	debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
-	priv->info.base = CFG_SYS_SDRAM_BASE;
-	priv->info.size =
-		rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
-#endif
+	if (phase_sdram_init()) {
+		if (rk3399_dmc_init(dev))
+			return 0;
+	} else {
+		priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
+		debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
+	}
+
+	if (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) {
+		priv->info.base = CFG_SYS_SDRAM_BASE;
+		priv->info.size =
+			rockchip_sdram_size((ulong)&priv->pmugrf->os_reg2);
+	}
+
 	return 0;
 }
 
@@ -3181,10 +3189,7 @@ U_BOOT_DRIVER(dmc_rk3399) = {
 	.id = UCLASS_RAM,
 	.of_match = rk3399_dmc_ids,
 	.ops = &rk3399_dmc_ops,
-#if defined(CONFIG_TPL_BUILD) || \
-	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
 	.of_to_plat = rk3399_dmc_of_to_plat,
-#endif
 	.probe = rk3399_dmc_probe,
 	.priv_auto	= sizeof(struct dram_info),
 #if defined(CONFIG_TPL_BUILD) || \
-- 
2.34.1

Re: [PATCH v2 7/9] rockchip: Ensure memory size is available in RK3399 SPL

[Quentin Schulz]

Hi Simon,

On 6/10/24 4:59 PM, Simon Glass wrote:
> At present gd->ram_size is 0 in SPL, meaning that it is not possible to
> enable the cache. Correct this by always populating the RAM size
> correctly.
> 
> Part of the confusion here comes from the large blocks of code which
> are #ifdefed out. Add a function phase_sdram_init() which returns
> whether SDRAM init should happen in the current phase, using that as
> needed to control the code flow.
> 
> This increases code size by about 500 bytes in SPL when the cache is on,
> since it must call the rather large rockchip_sdram_size() function.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Add new patch to correct memory size in SPL
> 
>   drivers/ram/rockchip/sdram_rk3399.c | 49 ++++++++++++++++-------------
>   1 file changed, 27 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
> index 02cc4a38cf0..2f37dd712e7 100644
> --- a/drivers/ram/rockchip/sdram_rk3399.c
> +++ b/drivers/ram/rockchip/sdram_rk3399.c
> @@ -13,6 +13,7 @@
>   #include <log.h>
>   #include <ram.h>
>   #include <regmap.h>
> +#include <spl.h>
>   #include <syscon.h>
>   #include <asm/arch-rockchip/clock.h>
>   #include <asm/arch-rockchip/cru.h>
> @@ -63,8 +64,6 @@ struct chan_info {
>   };
>   
>   struct dram_info {
> -#if defined(CONFIG_TPL_BUILD) || \
> -	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
>   	u32 pwrup_srefresh_exit[2];
>   	struct chan_info chan[2];
>   	struct clk ddr_clk;
> @@ -75,7 +74,6 @@ struct dram_info {
>   	struct rk3399_pmusgrf_regs *pmusgrf;
>   	struct rk3399_ddr_cic_regs *cic;
>   	const struct sdram_rk3399_ops *ops;
> -#endif
>   	struct ram_info info;
>   	struct rk3399_pmugrf_regs *pmugrf;
>   };
> @@ -92,9 +90,6 @@ struct sdram_rk3399_ops {
>   					struct rk3399_sdram_params *params);
>   };
>   
> -#if defined(CONFIG_TPL_BUILD) || \
> -	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
> -
>   struct rockchip_dmc_plat {
>   #if CONFIG_IS_ENABLED(OF_PLATDATA)
>   	struct dtd_rockchip_rk3399_dmc dtplat;
> @@ -191,6 +186,17 @@ struct io_setting {
>   	},
>   };
>   
> +/**
> + * phase_sdram_init() - Check if this is the phase where SDRAM init happens
> + *
> + * Returns: true to do SDRAM init in this phase, false to not
> + */
> +static bool phase_sdram_init(void)
> +{
> +	return spl_phase() == PHASE_TPL ||
> +	    (!IS_ENABLED(CONFIG_TPL) && !spl_in_proper());
> +}
> +
>   static struct io_setting *
>   lpddr4_get_io_settings(const struct rk3399_sdram_params *params, u32 mr5)
>   {
> @@ -3024,7 +3030,7 @@ static int rk3399_dmc_of_to_plat(struct udevice *dev)
>   	struct rockchip_dmc_plat *plat = dev_get_plat(dev);
>   	int ret;
>   
> -	if (!CONFIG_IS_ENABLED(OF_REAL))
> +	if (!CONFIG_IS_ENABLED(OF_REAL) || !phase_sdram_init())
>   		return 0;
>   
>   	ret = dev_read_u32_array(dev, "rockchip,sdram-params",
> @@ -3138,23 +3144,25 @@ static int rk3399_dmc_init(struct udevice *dev)
>   
>   	return 0;
>   }
> -#endif
>   
>   static int rk3399_dmc_probe(struct udevice *dev)
>   {
> -#if defined(CONFIG_TPL_BUILD) || \
> -	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
> -	if (rk3399_dmc_init(dev))
> -		return 0;
> -#else
>   	struct dram_info *priv = dev_get_priv(dev);
>   
> -	priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
> -	debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
> -	priv->info.base = CFG_SYS_SDRAM_BASE;
> -	priv->info.size =
> -		rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
> -#endif
> +	if (phase_sdram_init()) {
> +		if (rk3399_dmc_init(dev))
> +			return 0;
> +	} else {
> +		priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
> +		debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
> +	}
> +
> +	if (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) {
> +		priv->info.base = CFG_SYS_SDRAM_BASE;
> +		priv->info.size =
> +			rockchip_sdram_size((ulong)&priv->pmugrf->os_reg2);
> +	}
> +

Isn't the whole change summarized to making sure that priv->info.base 
and priv->info.size are set when DCACHE is enabled AND we're in the 
first stage BL (TPL or SPL if no TPL)?

i.e., shouldn't the following code be enough:

"""
static int rk3399_dmc_probe(struct udevice *dev)
{
#if defined(CONFIG_TPL_BUILD) || \
	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
	if (rk3399_dmc_init(dev))
		return 0;
#else
	struct dram_info *priv = dev_get_priv(dev);

	priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
	debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
#endif
	priv->info.base = CFG_SYS_SDRAM_BASE;
	priv->info.size =
		rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);

	return 0;
}
"""
?

Then what's after the endif could be guarded by if 
(!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) { if we need to but it's not clear 
to me why that is needed?

Basically, I'm not sure the migration from ifdefs to the 
phase_sdram_init() function is necessary. I'm not against it, but it 
makes the whole thing much harder to read and hides the actual changes.

Additionally, why was the cast to phys_addr_t changed to a ulong? The 
function actually expects a phys_addr_t.

Finally, can you please explain why gd->ram_size being 0 is an issue for 
the caches, where is this checked? I'm not too familiar with the caches 
in general :)

Cheers,
Quentin

Re: [PATCH v2 7/9] rockchip: Ensure memory size is available in RK3399 SPL

[Jonas Karlman]

Hi Simon and Quentin,

On 2024-06-11 13:27, Quentin Schulz wrote:
> Hi Simon,
> 
> On 6/10/24 4:59 PM, Simon Glass wrote:
>> At present gd->ram_size is 0 in SPL, meaning that it is not possible to
>> enable the cache. Correct this by always populating the RAM size
>> correctly.
>>
>> Part of the confusion here comes from the large blocks of code which
>> are #ifdefed out. Add a function phase_sdram_init() which returns
>> whether SDRAM init should happen in the current phase, using that as
>> needed to control the code flow.
>>
>> This increases code size by about 500 bytes in SPL when the cache is on,
>> since it must call the rather large rockchip_sdram_size() function.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v2:
>> - Add new patch to correct memory size in SPL
>>
>>   drivers/ram/rockchip/sdram_rk3399.c | 49 ++++++++++++++++-------------
>>   1 file changed, 27 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
>> index 02cc4a38cf0..2f37dd712e7 100644
>> --- a/drivers/ram/rockchip/sdram_rk3399.c
>> +++ b/drivers/ram/rockchip/sdram_rk3399.c
>> @@ -13,6 +13,7 @@
>>   #include <log.h>
>>   #include <ram.h>
>>   #include <regmap.h>
>> +#include <spl.h>
>>   #include <syscon.h>
>>   #include <asm/arch-rockchip/clock.h>
>>   #include <asm/arch-rockchip/cru.h>
>> @@ -63,8 +64,6 @@ struct chan_info {
>>   };
>>   
>>   struct dram_info {
>> -#if defined(CONFIG_TPL_BUILD) || \
>> -	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
>>   	u32 pwrup_srefresh_exit[2];
>>   	struct chan_info chan[2];
>>   	struct clk ddr_clk;
>> @@ -75,7 +74,6 @@ struct dram_info {
>>   	struct rk3399_pmusgrf_regs *pmusgrf;
>>   	struct rk3399_ddr_cic_regs *cic;
>>   	const struct sdram_rk3399_ops *ops;
>> -#endif
>>   	struct ram_info info;
>>   	struct rk3399_pmugrf_regs *pmugrf;
>>   };
>> @@ -92,9 +90,6 @@ struct sdram_rk3399_ops {
>>   					struct rk3399_sdram_params *params);
>>   };
>>   
>> -#if defined(CONFIG_TPL_BUILD) || \
>> -	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
>> -
>>   struct rockchip_dmc_plat {
>>   #if CONFIG_IS_ENABLED(OF_PLATDATA)
>>   	struct dtd_rockchip_rk3399_dmc dtplat;
>> @@ -191,6 +186,17 @@ struct io_setting {
>>   	},
>>   };
>>   
>> +/**
>> + * phase_sdram_init() - Check if this is the phase where SDRAM init happens
>> + *
>> + * Returns: true to do SDRAM init in this phase, false to not
>> + */
>> +static bool phase_sdram_init(void)
>> +{
>> +	return spl_phase() == PHASE_TPL ||
>> +	    (!IS_ENABLED(CONFIG_TPL) && !spl_in_proper());
>> +}
>> +
>>   static struct io_setting *
>>   lpddr4_get_io_settings(const struct rk3399_sdram_params *params, u32 mr5)
>>   {
>> @@ -3024,7 +3030,7 @@ static int rk3399_dmc_of_to_plat(struct udevice *dev)
>>   	struct rockchip_dmc_plat *plat = dev_get_plat(dev);
>>   	int ret;
>>   
>> -	if (!CONFIG_IS_ENABLED(OF_REAL))
>> +	if (!CONFIG_IS_ENABLED(OF_REAL) || !phase_sdram_init())
>>   		return 0;
>>   
>>   	ret = dev_read_u32_array(dev, "rockchip,sdram-params",
>> @@ -3138,23 +3144,25 @@ static int rk3399_dmc_init(struct udevice *dev)
>>   
>>   	return 0;
>>   }
>> -#endif
>>   
>>   static int rk3399_dmc_probe(struct udevice *dev)
>>   {
>> -#if defined(CONFIG_TPL_BUILD) || \
>> -	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
>> -	if (rk3399_dmc_init(dev))
>> -		return 0;
>> -#else
>>   	struct dram_info *priv = dev_get_priv(dev);
>>   
>> -	priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
>> -	debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
>> -	priv->info.base = CFG_SYS_SDRAM_BASE;
>> -	priv->info.size =
>> -		rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
>> -#endif
>> +	if (phase_sdram_init()) {
>> +		if (rk3399_dmc_init(dev))
>> +			return 0;
>> +	} else {
>> +		priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
>> +		debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
>> +	}
>> +
>> +	if (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) {
>> +		priv->info.base = CFG_SYS_SDRAM_BASE;
>> +		priv->info.size =
>> +			rockchip_sdram_size((ulong)&priv->pmugrf->os_reg2);
>> +	}
>> +
> 
> Isn't the whole change summarized to making sure that priv->info.base 
> and priv->info.size are set when DCACHE is enabled AND we're in the 
> first stage BL (TPL or SPL if no TPL)?
> 
> i.e., shouldn't the following code be enough:
> 
> """
> static int rk3399_dmc_probe(struct udevice *dev)
> {
> #if defined(CONFIG_TPL_BUILD) || \
> 	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
> 	if (rk3399_dmc_init(dev))
> 		return 0;
> #else
> 	struct dram_info *priv = dev_get_priv(dev);
> 
> 	priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
> 	debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
> #endif
> 	priv->info.base = CFG_SYS_SDRAM_BASE;
> 	priv->info.size =
> 		rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
> 
> 	return 0;
> }
> """
> ?
> 
> Then what's after the endif could be guarded by if 
> (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) { if we need to but it's not clear 
> to me why that is needed?

Agree, this look strange to me too and your diff much cleaner :-)

> 
> Basically, I'm not sure the migration from ifdefs to the 
> phase_sdram_init() function is necessary. I'm not against it, but it 
> makes the whole thing much harder to read and hides the actual changes.
> 
> Additionally, why was the cast to phys_addr_t changed to a ulong? The 
> function actually expects a phys_addr_t.
> 
> Finally, can you please explain why gd->ram_size being 0 is an issue for 
> the caches, where is this checked? I'm not too familiar with the caches 
> in general :)

My best guess is that enabling of caches in SPL cause issue for
bob/kevin because they only use SPL not TPL+SPL like (if I am not
mistaken) all other Rockchip arm64 targets.

Using SPL-only was not something I tested when caches was enabled in SPL.

Maybe bob/kevin can be changed to also use TPL+SPL similar to all other
RK3399 boards?

How U-Boot works on these chromebooks is still a mystery to me. If I
understand correctly SPL is only involved for bare metal boot, if this
is the case then using TPL + back-to-brom to load SPL should probably
work fine?, and would align all RK3399 boards to work in a similar way.

Regards,
Jonas

> 
> Cheers,
> Quentin

Re: [PATCH v2 7/9] rockchip: Ensure memory size is available in RK3399 SPL

[Quentin Schulz]

Hi Jonas,

On 6/11/24 3:43 PM, Jonas Karlman wrote:
> Hi Simon and Quentin,
> 
> On 2024-06-11 13:27, Quentin Schulz wrote:
>> Hi Simon,
>>
>> On 6/10/24 4:59 PM, Simon Glass wrote:
>>> At present gd->ram_size is 0 in SPL, meaning that it is not possible to
>>> enable the cache. Correct this by always populating the RAM size
>>> correctly.
>>>
>>> Part of the confusion here comes from the large blocks of code which
>>> are #ifdefed out. Add a function phase_sdram_init() which returns
>>> whether SDRAM init should happen in the current phase, using that as
>>> needed to control the code flow.
>>>
>>> This increases code size by about 500 bytes in SPL when the cache is on,
>>> since it must call the rather large rockchip_sdram_size() function.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> Changes in v2:
>>> - Add new patch to correct memory size in SPL
>>>
>>>    drivers/ram/rockchip/sdram_rk3399.c | 49 ++++++++++++++++-------------
>>>    1 file changed, 27 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
>>> index 02cc4a38cf0..2f37dd712e7 100644
>>> --- a/drivers/ram/rockchip/sdram_rk3399.c
>>> +++ b/drivers/ram/rockchip/sdram_rk3399.c
>>> @@ -13,6 +13,7 @@
>>>    #include <log.h>
>>>    #include <ram.h>
>>>    #include <regmap.h>
>>> +#include <spl.h>
>>>    #include <syscon.h>
>>>    #include <asm/arch-rockchip/clock.h>
>>>    #include <asm/arch-rockchip/cru.h>
>>> @@ -63,8 +64,6 @@ struct chan_info {
>>>    };
>>>    
>>>    struct dram_info {
>>> -#if defined(CONFIG_TPL_BUILD) || \
>>> -	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
>>>    	u32 pwrup_srefresh_exit[2];
>>>    	struct chan_info chan[2];
>>>    	struct clk ddr_clk;
>>> @@ -75,7 +74,6 @@ struct dram_info {
>>>    	struct rk3399_pmusgrf_regs *pmusgrf;
>>>    	struct rk3399_ddr_cic_regs *cic;
>>>    	const struct sdram_rk3399_ops *ops;
>>> -#endif
>>>    	struct ram_info info;
>>>    	struct rk3399_pmugrf_regs *pmugrf;
>>>    };
>>> @@ -92,9 +90,6 @@ struct sdram_rk3399_ops {
>>>    					struct rk3399_sdram_params *params);
>>>    };
>>>    
>>> -#if defined(CONFIG_TPL_BUILD) || \
>>> -	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
>>> -
>>>    struct rockchip_dmc_plat {
>>>    #if CONFIG_IS_ENABLED(OF_PLATDATA)
>>>    	struct dtd_rockchip_rk3399_dmc dtplat;
>>> @@ -191,6 +186,17 @@ struct io_setting {
>>>    	},
>>>    };
>>>    
>>> +/**
>>> + * phase_sdram_init() - Check if this is the phase where SDRAM init happens
>>> + *
>>> + * Returns: true to do SDRAM init in this phase, false to not
>>> + */
>>> +static bool phase_sdram_init(void)
>>> +{
>>> +	return spl_phase() == PHASE_TPL ||
>>> +	    (!IS_ENABLED(CONFIG_TPL) && !spl_in_proper());
>>> +}
>>> +
>>>    static struct io_setting *
>>>    lpddr4_get_io_settings(const struct rk3399_sdram_params *params, u32 mr5)
>>>    {
>>> @@ -3024,7 +3030,7 @@ static int rk3399_dmc_of_to_plat(struct udevice *dev)
>>>    	struct rockchip_dmc_plat *plat = dev_get_plat(dev);
>>>    	int ret;
>>>    
>>> -	if (!CONFIG_IS_ENABLED(OF_REAL))
>>> +	if (!CONFIG_IS_ENABLED(OF_REAL) || !phase_sdram_init())
>>>    		return 0;
>>>    
>>>    	ret = dev_read_u32_array(dev, "rockchip,sdram-params",
>>> @@ -3138,23 +3144,25 @@ static int rk3399_dmc_init(struct udevice *dev)
>>>    
>>>    	return 0;
>>>    }
>>> -#endif
>>>    
>>>    static int rk3399_dmc_probe(struct udevice *dev)
>>>    {
>>> -#if defined(CONFIG_TPL_BUILD) || \
>>> -	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
>>> -	if (rk3399_dmc_init(dev))
>>> -		return 0;
>>> -#else
>>>    	struct dram_info *priv = dev_get_priv(dev);
>>>    
>>> -	priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
>>> -	debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
>>> -	priv->info.base = CFG_SYS_SDRAM_BASE;
>>> -	priv->info.size =
>>> -		rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
>>> -#endif
>>> +	if (phase_sdram_init()) {
>>> +		if (rk3399_dmc_init(dev))
>>> +			return 0;
>>> +	} else {
>>> +		priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
>>> +		debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
>>> +	}
>>> +
>>> +	if (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) {
>>> +		priv->info.base = CFG_SYS_SDRAM_BASE;
>>> +		priv->info.size =
>>> +			rockchip_sdram_size((ulong)&priv->pmugrf->os_reg2);
>>> +	}
>>> +
>>
>> Isn't the whole change summarized to making sure that priv->info.base
>> and priv->info.size are set when DCACHE is enabled AND we're in the
>> first stage BL (TPL or SPL if no TPL)?
>>
>> i.e., shouldn't the following code be enough:
>>
>> """
>> static int rk3399_dmc_probe(struct udevice *dev)
>> {
>> #if defined(CONFIG_TPL_BUILD) || \
>> 	(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
>> 	if (rk3399_dmc_init(dev))
>> 		return 0;
>> #else
>> 	struct dram_info *priv = dev_get_priv(dev);
>>
>> 	priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
>> 	debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
>> #endif
>> 	priv->info.base = CFG_SYS_SDRAM_BASE;
>> 	priv->info.size =
>> 		rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
>>
>> 	return 0;
>> }
>> """
>> ?
>>
>> Then what's after the endif could be guarded by if
>> (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) { if we need to but it's not clear
>> to me why that is needed?
> 
> Agree, this look strange to me too and your diff much cleaner :-)
> 
>>
>> Basically, I'm not sure the migration from ifdefs to the
>> phase_sdram_init() function is necessary. I'm not against it, but it
>> makes the whole thing much harder to read and hides the actual changes.
>>
>> Additionally, why was the cast to phys_addr_t changed to a ulong? The
>> function actually expects a phys_addr_t.
>>
>> Finally, can you please explain why gd->ram_size being 0 is an issue for
>> the caches, where is this checked? I'm not too familiar with the caches
>> in general :)
> 
> My best guess is that enabling of caches in SPL cause issue for
> bob/kevin because they only use SPL not TPL+SPL like (if I am not
> mistaken) all other Rockchip arm64 targets.
> 
> Using SPL-only was not something I tested when caches was enabled in SPL.
> 
> Maybe bob/kevin can be changed to also use TPL+SPL similar to all other
> RK3399 boards?
> 
> How U-Boot works on these chromebooks is still a mystery to me. If I
> understand correctly SPL is only involved for bare metal boot, if this
> is the case then using TPL + back-to-brom to load SPL should probably
> work fine?, and would align all RK3399 boards to work in a similar way.
> 

Once I repair my Gru Bob, I'll be able to finally migrate it to Linux 
but broken power and volume rocker buttons make it impossible to disable 
some of the EC security stuff :)

Anyway, my understanding is that coreboot is started by the BootROM, 
then it loads U-Boot as its payload (I assume SPL???) which allows to 
boot UEFI systems (coreboot doesn't).

I assume no TPL because we don't want to back-to-brom as coreboot's 
payload wouldn't be loaded by the BootROM so it wouldn't know what to do 
with it.

I'll let Simon answer and will religiously listen :)

Cheers,
Quentin

Re: [PATCH v2 7/9] rockchip: Ensure memory size is available in RK3399 SPL

[Simon Glass]

Hi Quentin,

On Tue, 11 Jun 2024 at 05:27, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Simon,
>
> On 6/10/24 4:59 PM, Simon Glass wrote:
> > At present gd->ram_size is 0 in SPL, meaning that it is not possible to
> > enable the cache. Correct this by always populating the RAM size
> > correctly.
> >
> > Part of the confusion here comes from the large blocks of code which
> > are #ifdefed out. Add a function phase_sdram_init() which returns
> > whether SDRAM init should happen in the current phase, using that as
> > needed to control the code flow.
> >
> > This increases code size by about 500 bytes in SPL when the cache is on,
> > since it must call the rather large rockchip_sdram_size() function.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Add new patch to correct memory size in SPL
> >
> >   drivers/ram/rockchip/sdram_rk3399.c | 49 ++++++++++++++++-------------
> >   1 file changed, 27 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
> > index 02cc4a38cf0..2f37dd712e7 100644
> > --- a/drivers/ram/rockchip/sdram_rk3399.c
> > +++ b/drivers/ram/rockchip/sdram_rk3399.c
> > @@ -13,6 +13,7 @@
> >   #include <log.h>
> >   #include <ram.h>
> >   #include <regmap.h>
> > +#include <spl.h>
> >   #include <syscon.h>
> >   #include <asm/arch-rockchip/clock.h>
> >   #include <asm/arch-rockchip/cru.h>
> > @@ -63,8 +64,6 @@ struct chan_info {
> >   };
> >
> >   struct dram_info {
> > -#if defined(CONFIG_TPL_BUILD) || \
> > -     (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
> >       u32 pwrup_srefresh_exit[2];
> >       struct chan_info chan[2];
> >       struct clk ddr_clk;
> > @@ -75,7 +74,6 @@ struct dram_info {
> >       struct rk3399_pmusgrf_regs *pmusgrf;
> >       struct rk3399_ddr_cic_regs *cic;
> >       const struct sdram_rk3399_ops *ops;
> > -#endif
> >       struct ram_info info;
> >       struct rk3399_pmugrf_regs *pmugrf;
> >   };
> > @@ -92,9 +90,6 @@ struct sdram_rk3399_ops {
> >                                       struct rk3399_sdram_params *params);
> >   };
> >
> > -#if defined(CONFIG_TPL_BUILD) || \
> > -     (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
> > -
> >   struct rockchip_dmc_plat {
> >   #if CONFIG_IS_ENABLED(OF_PLATDATA)
> >       struct dtd_rockchip_rk3399_dmc dtplat;
> > @@ -191,6 +186,17 @@ struct io_setting {
> >       },
> >   };
> >
> > +/**
> > + * phase_sdram_init() - Check if this is the phase where SDRAM init happens
> > + *
> > + * Returns: true to do SDRAM init in this phase, false to not
> > + */
> > +static bool phase_sdram_init(void)
> > +{
> > +     return spl_phase() == PHASE_TPL ||
> > +         (!IS_ENABLED(CONFIG_TPL) && !spl_in_proper());
> > +}
> > +
> >   static struct io_setting *
> >   lpddr4_get_io_settings(const struct rk3399_sdram_params *params, u32 mr5)
> >   {
> > @@ -3024,7 +3030,7 @@ static int rk3399_dmc_of_to_plat(struct udevice *dev)
> >       struct rockchip_dmc_plat *plat = dev_get_plat(dev);
> >       int ret;
> >
> > -     if (!CONFIG_IS_ENABLED(OF_REAL))
> > +     if (!CONFIG_IS_ENABLED(OF_REAL) || !phase_sdram_init())
> >               return 0;
> >
> >       ret = dev_read_u32_array(dev, "rockchip,sdram-params",
> > @@ -3138,23 +3144,25 @@ static int rk3399_dmc_init(struct udevice *dev)
> >
> >       return 0;
> >   }
> > -#endif
> >
> >   static int rk3399_dmc_probe(struct udevice *dev)
> >   {
> > -#if defined(CONFIG_TPL_BUILD) || \
> > -     (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
> > -     if (rk3399_dmc_init(dev))
> > -             return 0;
> > -#else
> >       struct dram_info *priv = dev_get_priv(dev);
> >
> > -     priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
> > -     debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
> > -     priv->info.base = CFG_SYS_SDRAM_BASE;
> > -     priv->info.size =
> > -             rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
> > -#endif
> > +     if (phase_sdram_init()) {
> > +             if (rk3399_dmc_init(dev))
> > +                     return 0;
> > +     } else {
> > +             priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
> > +             debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
> > +     }
> > +
> > +     if (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) {
> > +             priv->info.base = CFG_SYS_SDRAM_BASE;
> > +             priv->info.size =
> > +                     rockchip_sdram_size((ulong)&priv->pmugrf->os_reg2);
> > +     }
> > +
>
> Isn't the whole change summarized to making sure that priv->info.base
> and priv->info.size are set when DCACHE is enabled AND we're in the
> first stage BL (TPL or SPL if no TPL)?
>
> i.e., shouldn't the following code be enough:
>
> """
> static int rk3399_dmc_probe(struct udevice *dev)
> {
> #if defined(CONFIG_TPL_BUILD) || \
>         (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
>         if (rk3399_dmc_init(dev))
>                 return 0;
> #else
>         struct dram_info *priv = dev_get_priv(dev);
>
>         priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
>         debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
> #endif
>         priv->info.base = CFG_SYS_SDRAM_BASE;
>         priv->info.size =
>                 rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
>
>         return 0;
> }
> """
> ?

Yes, that's the nub of it. Of course, it doesn't actually fix the
problem. I presume the SRAM appears in the page tables?

>
> Then what's after the endif could be guarded by if
> (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) { if we need to but it's not clear
> to me why that is needed?

Just to avoid increasing code size when the cache is off.

>
> Basically, I'm not sure the migration from ifdefs to the
> phase_sdram_init() function is necessary. I'm not against it, but it
> makes the whole thing much harder to read and hides the actual changes.

Yes well it would have been a lot better if I had put it in a separate
patch, which I forgot to do.

>
> Additionally, why was the cast to phys_addr_t changed to a ulong? The
> function actually expects a phys_addr_t.

Hmm that is something that we brought in for 32-bit machines. For
64-bit I don't really see why we are using it. It makes printf() hard.

But anyway, I should have left it alone :-)

>
> Finally, can you please explain why gd->ram_size being 0 is an issue for
> the caches, where is this checked? I'm not too familiar with the caches
> in general :)

It puts the MMU table just below the top of RAM which is currently
base + size (0 + 0) = somewhere like (4GB - 64KB).

Regards,
Simon

[PATCH v2 8/9] rockchip: bob: kevin: Disable dcache in SPL

[Simon Glass]

This causes a hang, so disable it. Unfortunately the RAM-size fix does
not resolve the problem and I am unsure what is wrong. As soon as the
cache is enabled the board appears to hang.

Fixes: 6d8cdfd1536 ("rockchip: spl: Enable caches to speed up checksum validation")

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 configs/chromebook_bob_defconfig   | 1 +
 configs/chromebook_kevin_defconfig | 1 +
 2 files changed, 2 insertions(+)

diff --git a/configs/chromebook_bob_defconfig b/configs/chromebook_bob_defconfig
index acfe3934104..b2ecfa6050c 100644
--- a/configs/chromebook_bob_defconfig
+++ b/configs/chromebook_bob_defconfig
@@ -1,5 +1,6 @@
 CONFIG_ARM=y
 CONFIG_SKIP_LOWLEVEL_INIT=y
+CONFIG_SPL_SYS_DCACHE_OFF=y
 CONFIG_COUNTER_FREQUENCY=24000000
 CONFIG_ARCH_ROCKCHIP=y
 CONFIG_TEXT_BASE=0x00200000
diff --git a/configs/chromebook_kevin_defconfig b/configs/chromebook_kevin_defconfig
index 95fdb418d82..da748e4f022 100644
--- a/configs/chromebook_kevin_defconfig
+++ b/configs/chromebook_kevin_defconfig
@@ -2,6 +2,7 @@ CONFIG_ARM=y
 CONFIG_SKIP_LOWLEVEL_INIT=y
 CONFIG_COUNTER_FREQUENCY=24000000
 CONFIG_ARCH_ROCKCHIP=y
+CONFIG_SPL_SYS_DCACHE_OFF=y
 CONFIG_TEXT_BASE=0x00200000
 CONFIG_SPL_GPIO=y
 CONFIG_NR_DRAM_BANKS=1
-- 
2.34.1

[PATCH v2 9/9] Drop the special am335x_boneblack_vboot target

[Simon Glass]

Now that am335x_evm boots OK on the Beaglebone black, drop the latter
and update the docs to cover the change.

Also add a few updates about 'make fit' and drop the note about the
security review, as U-Boot's verified boot has had quite extensive
review now.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Drop patch "regulator: rk8xx: Fix incorrect parameter"
- Rewrite boneblack patch to onstead drop the target and update docs

 board/ti/am335x/MAINTAINERS              |  1 -
 configs/am335x_boneblack_vboot_defconfig | 94 ------------------------
 configs/am335x_evm_defconfig             |  3 +-
 doc/usage/fit/beaglebone_vboot.rst       | 21 +++---
 4 files changed, 12 insertions(+), 107 deletions(-)
 delete mode 100644 configs/am335x_boneblack_vboot_defconfig

diff --git a/board/ti/am335x/MAINTAINERS b/board/ti/am335x/MAINTAINERS
index 219c8715bf1..ed8800a2663 100644
--- a/board/ti/am335x/MAINTAINERS
+++ b/board/ti/am335x/MAINTAINERS
@@ -3,6 +3,5 @@ M:	Tom Rini <trini@konsulko.com>
 S:	Maintained
 F:	board/ti/am335x/
 F:	include/configs/am335x_evm.h
-F:	configs/am335x_boneblack_vboot_defconfig
 F:	configs/am335x_evm_defconfig
 F:	configs/am335x_evm_spiboot_defconfig
diff --git a/configs/am335x_boneblack_vboot_defconfig b/configs/am335x_boneblack_vboot_defconfig
deleted file mode 100644
index d473a1a793b..00000000000
--- a/configs/am335x_boneblack_vboot_defconfig
+++ /dev/null
@@ -1,94 +0,0 @@
-CONFIG_ARM=y
-CONFIG_ARCH_CPU_INIT=y
-# CONFIG_SPL_USE_ARCH_MEMCPY is not set
-# CONFIG_SPL_USE_ARCH_MEMSET is not set
-CONFIG_ARCH_OMAP2PLUS=y
-CONFIG_TI_COMMON_CMD_OPTIONS=y
-CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
-CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x4030ff00
-CONFIG_SF_DEFAULT_SPEED=24000000
-CONFIG_DEFAULT_DEVICE_TREE="am335x-boneblack"
-CONFIG_AM33XX=y
-CONFIG_CLOCK_SYNTHESIZER=y
-CONFIG_SPL=y
-CONFIG_ENV_OFFSET_REDUND=0x280000
-CONFIG_TIMESTAMP=y
-CONFIG_FIT_SIGNATURE=y
-CONFIG_FIT_VERBOSE=y
-CONFIG_SYS_BOOTM_LEN=0x1000000
-CONFIG_DISTRO_DEFAULTS=y
-CONFIG_AUTOBOOT_KEYED=y
-CONFIG_AUTOBOOT_PROMPT="Press SPACE to abort autoboot in %d seconds\n"
-CONFIG_AUTOBOOT_DELAY_STR="d"
-CONFIG_AUTOBOOT_STOP_STR=" "
-CONFIG_BOOTCOMMAND="run findfdt; run init_console; run finduuid; run distro_bootcmd"
-CONFIG_SYS_CONSOLE_INFO_QUIET=y
-CONFIG_ARCH_MISC_INIT=y
-CONFIG_SPL_SYS_MALLOC=y
-CONFIG_SPL_SYS_MALLOC_SIZE=0x800000
-CONFIG_SPL_MUSB_NEW=y
-# CONFIG_SPL_NAND_SUPPORT is not set
-CONFIG_SPL_NET=y
-CONFIG_SPL_NET_VCI_STRING="AM33xx U-Boot SPL"
-CONFIG_SPL_OS_BOOT=y
-CONFIG_SPL_FALCON_BOOT_MMCSD=y
-CONFIG_SYS_MMCSD_RAW_MODE_KERNEL_SECTOR=0x1700
-CONFIG_SYS_MMCSD_RAW_MODE_ARGS_SECTOR=0x1500
-CONFIG_SYS_MMCSD_RAW_MODE_ARGS_SECTORS=0x200
-CONFIG_CMD_SPL=y
-CONFIG_SYS_I2C_EEPROM_ADDR_LEN=2
-# CONFIG_CMD_SETEXPR is not set
-CONFIG_BOOTP_DNS2=y
-CONFIG_OF_CONTROL=y
-CONFIG_SPL_OF_CONTROL=y
-CONFIG_ENV_OVERWRITE=y
-CONFIG_ENV_IS_IN_MMC=y
-CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
-CONFIG_SYS_RELOC_GD_ENV_ADDR=y
-CONFIG_SYS_MMC_ENV_DEV=1
-CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG=y
-CONFIG_VERSION_VARIABLE=y
-CONFIG_NET_RETRY_COUNT=10
-CONFIG_BOOTP_SEND_HOSTNAME=y
-# CONFIG_SPL_BLK is not set
-CONFIG_BOOTCOUNT_LIMIT=y
-CONFIG_SYS_BOOTCOUNT_BE=y
-CONFIG_DFU_MMC=y
-CONFIG_DFU_RAM=y
-CONFIG_USB_FUNCTION_FASTBOOT=y
-CONFIG_DM_I2C=y
-CONFIG_MISC=y
-CONFIG_SYS_I2C_EEPROM_ADDR=0x50
-# CONFIG_SPL_DM_MMC is not set
-CONFIG_MMC_OMAP_HS=y
-CONFIG_MTD=y
-CONFIG_DM_SPI_FLASH=y
-CONFIG_SPI_FLASH_WINBOND=y
-CONFIG_PHY_ATHEROS=y
-CONFIG_PHY_SMSC=y
-CONFIG_PHY_GIGE=y
-CONFIG_MII=y
-CONFIG_DRIVER_TI_CPSW=y
-CONFIG_DM_PMIC=y
-# CONFIG_SPL_DM_PMIC is not set
-CONFIG_PMIC_TPS65217=y
-CONFIG_SPL_POWER_TPS65910=y
-CONFIG_SPI=y
-CONFIG_DM_SPI=y
-CONFIG_OMAP3_SPI=y
-CONFIG_TIMER=y
-CONFIG_OMAP_TIMER=y
-CONFIG_USB=y
-CONFIG_DM_USB_GADGET=y
-CONFIG_SPL_DM_USB_GADGET=y
-CONFIG_USB_MUSB_HOST=y
-CONFIG_USB_MUSB_GADGET=y
-CONFIG_USB_MUSB_TI=y
-CONFIG_USB_GADGET=y
-CONFIG_SPL_USB_GADGET=y
-CONFIG_USB_GADGET_MANUFACTURER="Texas Instruments"
-CONFIG_USB_GADGET_VENDOR_NUM=0x0451
-CONFIG_USB_GADGET_PRODUCT_NUM=0xd022
-CONFIG_USB_ETHER=y
-CONFIG_SPL_USB_ETHER=y
-CONFIG_LZO=y
diff --git a/configs/am335x_evm_defconfig b/configs/am335x_evm_defconfig
index d243cb16e72..cabc181460a 100644
--- a/configs/am335x_evm_defconfig
+++ b/configs/am335x_evm_defconfig
@@ -13,6 +13,8 @@ CONFIG_AM335X_USB0_PERIPHERAL=y
 CONFIG_AM335X_USB1=y
 CONFIG_SPL=y
 CONFIG_TIMESTAMP=y
+CONFIG_FIT_SIGNATURE=y
+CONFIG_FIT_VERBOSE=y
 CONFIG_SPL_LOAD_FIT=y
 CONFIG_SYS_BOOTM_LEN=0x1000000
 CONFIG_DISTRO_DEFAULTS=y
@@ -119,5 +121,4 @@ CONFIG_SPL_USB_ETHER=y
 CONFIG_WDT=y
 # CONFIG_SPL_WDT is not set
 CONFIG_DYNAMIC_CRC_TABLE=y
-CONFIG_RSA=y
 CONFIG_LZO=y
diff --git a/doc/usage/fit/beaglebone_vboot.rst b/doc/usage/fit/beaglebone_vboot.rst
index cd6bb141910..1360c71803c 100644
--- a/doc/usage/fit/beaglebone_vboot.rst
+++ b/doc/usage/fit/beaglebone_vboot.rst
@@ -67,18 +67,20 @@ a. Set up the environment variable to point to your toolchain. You will need
 
        export CROSS_COMPILE=arm-linux-gnueabi-
 
-b. Configure and build U-Boot with verified boot enabled::
+b. Configure and build U-Boot with verified boot enabled. Note that we use the
+am335x_evm target since it covers all boards based on the AM335x evaluation
+board::
 
     export UBOOT=/path/to/u-boot
     cd $UBOOT
     # You can add -j10 if you have 10 CPUs to make it faster
-    make O=b/am335x_boneblack_vboot am335x_boneblack_vboot_config all
-    export UOUT=$UBOOT/b/am335x_boneblack_vboot
+    make O=b/am335x_evm am335x_evm_config all
+    export UOUT=$UBOOT/b/am335x_evm
 
 c. You will now have a U-Boot image::
 
-    file b/am335x_boneblack_vboot/u-boot-dtb.img
-    b/am335x_boneblack_vboot/u-boot-dtb.img: u-boot legacy uImage,
+    file b/am335x_evm/u-boot-dtb.img
+    b/am335x_evm/u-boot-dtb.img: u-boot legacy uImage,
       U-Boot 2014.07-rc2-00065-g2f69f8, Firmware/ARM, Firmware Image
       (Not compressed), 395375 bytes, Sat May 31 16:19:04 2014,
       Load Address: 0x80800000, Entry Point: 0x00000000,
@@ -466,7 +468,7 @@ the private key that you signed with so that it can verify any kernels that
 you sign::
 
     cd $UBOOT
-    make O=b/am335x_boneblack_vboot EXT_DTB=${WORK}/am335x-boneblack-pubkey.dtb
+    make O=b/am335x_evm EXT_DTB=${WORK}/am335x-boneblack-pubkey.dtb
 
 Here we are overriding the normal device tree file with our one, which
 contains the public key.
@@ -597,14 +599,11 @@ Further Improvements
 
 Several of the steps here can be easily automated. In particular it would be
 capital if signing and packaging a kernel were easy, perhaps a simple make
-target in the kernel.
+target in the kernel. A stating point for this is the 'make image.fit' target
+for ARM64 in Linux from v6.9 onwards.
 
 Some mention of how to use multiple .dtb files in a FIT might be useful.
 
-U-Boot's verified boot mechanism has not had a robust and independent security
-review. Such a review should look at the implementation and its resistance to
-attacks.
-
 Perhaps the verified boot feature could be integrated into the Amstrom
 distribution.
 
-- 
2.34.1

Re: [PATCH v2 9/9] Drop the special am335x_boneblack_vboot target

[Tom Rini]

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

On Mon, Jun 10, 2024 at 08:59:20AM -0600, Simon Glass wrote:

> Now that am335x_evm boots OK on the Beaglebone black, drop the latter
> and update the docs to cover the change.
> 
> Also add a few updates about 'make fit' and drop the note about the
> security review, as U-Boot's verified boot has had quite extensive
> review now.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

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