Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45390 )
Change subject: nb/intel/x4x: Relocate read to TPM base address ......................................................................
nb/intel/x4x: Relocate read to TPM base address
Other northbridges do it at the start of raminit. Also, since the TPM access register is 8 bits wide, use 8-bit ops instead of 32-bit ops.
This register works the same for all TXT-enabled northbridges: If the TPM access register is valid, and the establishment bit (bit 0) is set, then a DRTM has not been established on the platform (or the TPM is not present), and the memory will be unlocked. If bit 0 is clear, then the memory may remain locked depending on whether it could contain secrets.
Change-Id: Ic36a2810a861758ce733fe80c4e555439e2ffb7b Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/x4x/bootblock.c M src/northbridge/intel/x4x/raminit.c 2 files changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/45390/1
diff --git a/src/northbridge/intel/x4x/bootblock.c b/src/northbridge/intel/x4x/bootblock.c index baa4ae3..e83a2b8 100644 --- a/src/northbridge/intel/x4x/bootblock.c +++ b/src/northbridge/intel/x4x/bootblock.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
#include <arch/bootblock.h> -#include <arch/mmio.h> #include <device/pci_ops.h>
#include "x4x.h" @@ -9,9 +8,6 @@
void bootblock_early_northbridge_init(void) { - /* Disable LaGrande Technology (LT) */ - read32((void *)TPM_BASE_ADDRESS); - const uint32_t reg32 = CONFIG_MMCONF_BASE_ADDRESS | 16 | 1; pci_io_write_config32(HOST_BRIDGE, D0F0_PCIEXBAR_LO, reg32); } diff --git a/src/northbridge/intel/x4x/raminit.c b/src/northbridge/intel/x4x/raminit.c index a62771d..6536fbf 100644 --- a/src/northbridge/intel/x4x/raminit.c +++ b/src/northbridge/intel/x4x/raminit.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
+#include <arch/mmio.h> #include <device/pci_ops.h> #include <device/smbus_host.h> #include <cbmem.h> @@ -616,6 +617,9 @@ timestamp_add_now(TS_BEFORE_INITRAM); printk(BIOS_DEBUG, "Setting up RAM controller.\n");
+ /* Disable LaGrande Technology (LT) */ + read8((void *)TPM_BASE_ADDRESS); + pci_write_config8(HOST_BRIDGE, 0xdf, 0xff);
memset(&s, 0, sizeof(struct sysinfo));
Hello build bot (Jenkins), Damien Zammit, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45390
to look at the new patch set (#3).
Change subject: nb/intel/x4x: Relocate read to TPM base address ......................................................................
nb/intel/x4x: Relocate read to TPM base address
Other northbridges do it at the start of raminit. Also, since the TPM access register is 8 bits wide, use 8-bit ops instead of 32-bit ops.
This register works the same for all TXT-enabled northbridges: If the TPM access register is valid, and the establishment bit (bit 0) is set, then a DRTM has not been established on the platform (or the TPM is not present), and the memory will be unlocked. If bit 0 is clear, then the memory may remain locked depending on whether it could contain secrets.
Change-Id: Ic36a2810a861758ce733fe80c4e555439e2ffb7b Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/x4x/bootblock.c M src/northbridge/intel/x4x/raminit.c 2 files changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/45390/3
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45390 )
Change subject: nb/intel/x4x: Relocate read to TPM base address ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45390/4/src/northbridge/intel/x4x/r... File src/northbridge/intel/x4x/raminit.c:
https://review.coreboot.org/c/coreboot/+/45390/4/src/northbridge/intel/x4x/r... PS4, Line 621: read8((void *)TPM_BASE_ADDRESS); you don't use the information gathered, so this is as useless as before.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45390 )
Change subject: nb/intel/x4x: Relocate read to TPM base address ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45390/4/src/northbridge/intel/x4x/r... File src/northbridge/intel/x4x/raminit.c:
https://review.coreboot.org/c/coreboot/+/45390/4/src/northbridge/intel/x4x/r... PS4, Line 621: read8((void *)TPM_BASE_ADDRESS);
you don't use the information gathered, so this is as useless as before.
On Haswell with a TXT-capable CPU/PCH and no TPM, if I don't perform any reads to the TPM access register, raminit will fail. A single read is enough to unblock the memory. (Yes, an extra MSR 0x2e6 write is needed, but that would fail if I didn't read the TPM access register beforehand.
I recall reading somewhere that the MCH will always unblock the memory if a read to the access register returns a one in bit 0 (TPM establishment not asserted). Sure, that isn't being checked here, but without a TPM this always returns 0xff
Hello build bot (Jenkins), Damien Zammit, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45390
to look at the new patch set (#5).
Change subject: nb/intel/x4x: Uniformize reading the TPM access register ......................................................................
nb/intel/x4x: Uniformize reading the TPM access register
Other northbridges do it at the start of raminit. Also, since the TPM access register is 8 bits wide, use 8-bit ops instead of 32-bit ops. Like other northbridges do, poll for the valid bit to be set as well.
This register works the same for all TXT-enabled northbridges: If the TPM access register is valid, and the establishment bit (bit 0) is set, then a DRTM has not been established on the platform (or the TPM is not present), and the memory will be unlocked. If bit 0 is clear, then the memory may remain locked depending on whether it could contain secrets. If no TPM is present, the access register contains all ones. Therefore, the newly-added loop would only be executed once in most scenarios.
Change-Id: Ic36a2810a861758ce733fe80c4e555439e2ffb7b Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/x4x/bootblock.c M src/northbridge/intel/x4x/raminit.c 2 files changed, 5 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/45390/5
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45390 )
Change subject: nb/intel/x4x: Uniformize reading the TPM access register ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45390/4/src/northbridge/intel/x4x/r... File src/northbridge/intel/x4x/raminit.c:
https://review.coreboot.org/c/coreboot/+/45390/4/src/northbridge/intel/x4x/r... PS4, Line 621: read8((void *)TPM_BASE_ADDRESS);
On Haswell with a TXT-capable CPU/PCH and no TPM, if I don't perform any reads to the TPM access reg […]
I put this in a loop for consistency with the other platforms. We probably want to have some function in TXT code to check this register anyway.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45390 )
Change subject: nb/intel/x4x: Uniformize reading the TPM access register ......................................................................
Patch Set 7: Code-Review+1
(1 comment)
Curious, do you have a platform to test on?
https://review.coreboot.org/c/coreboot/+/45390/4/src/northbridge/intel/x4x/r... File src/northbridge/intel/x4x/raminit.c:
https://review.coreboot.org/c/coreboot/+/45390/4/src/northbridge/intel/x4x/r... PS4, Line 621: read8((void *)TPM_BASE_ADDRESS);
you don't use the information gathered, so this is as useless as before.
Reads can have side effects.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45390 )
Change subject: nb/intel/x4x: Uniformize reading the TPM access register ......................................................................
Patch Set 7:
Patch Set 7: Code-Review+1
(1 comment)
Curious, do you have a platform to test on?
I can test on some x4x board, as I have several. The biggest question is... But where are they?
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45390?usp=email )
Change subject: nb/intel/x4x: Uniformize reading the TPM access register ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Angel Pons (Code Review) -
Martin L Roth (Code Review)
-
Patrick Rudolph (Code Review)
-
Tim Wawrzynczak (Code Review)