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

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

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

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/89aa8463cdf3919ca4f04fc24ec8b1... 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

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

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

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

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

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

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

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