Change in coreboot[master]: soc/intel/xeon_sp: Work around FSP-T not respecting its own API
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47738 )
Change subject: soc/intel/xeon_sp: Work around FSP-T not respecting its own API ......................................................................
soc/intel/xeon_sp: Work around FSP-T not respecting its own API
The CPX FSP-T does not respect the FSP2.x spec and uses registers where coreboot has its initial timestamp stored.
Change-Id: I4ba15decec22cd473e63149ec399d82c5e3fd214 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/xeon_sp/bootblock.c 1 file changed, 14 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/47738/1
diff --git a/src/soc/intel/xeon_sp/bootblock.c b/src/soc/intel/xeon_sp/bootblock.c index 5adda44..5524c66 100644 --- a/src/soc/intel/xeon_sp/bootblock.c +++ b/src/soc/intel/xeon_sp/bootblock.c @@ -33,11 +33,20 @@ .UpdTerminator = 0x55AA, };
+static uint64_t assembly_timestamp; +static uint64_t bootblock_timestamp; + asmlinkage void bootblock_c_entry(uint64_t base_timestamp) { + /* + * FSP-T does not respect its own API and trashes registers + * coreboot uses to store its initial timestamp. + */ + assembly_timestamp = base_timestamp; + bootblock_timestamp = timestamp_get(); fast_spi_cache_bios_region();
- bootblock_main_with_basetime(base_timestamp); + bootblock_main_with_basetime(MIN(assembly_timestamp, bootblock_timestamp)); }
void bootblock_soc_early_init(void) @@ -55,6 +64,10 @@ { if (CONFIG(BOOTBLOCK_CONSOLE)) printk(BIOS_DEBUG, "FSP TempRamInit successful...\n"); + + if (assembly_timestamp < bootblock_timestamp) + printk(BIOS_WARNING, "Invalid initial timestamp detected\n"); + if (CONFIG(FSP_CAR)) report_fspt_output(); bootblock_pch_init();
Hello Marc Jones, Jonathan Zhang, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47738
to look at the new patch set (#2).
Change subject: soc/intel/xeon_sp: Work around FSP-T not respecting its own API ......................................................................
soc/intel/xeon_sp: Work around FSP-T not respecting its own API
The CPX FSP-T does not respect the FSP2.x spec and uses registers where coreboot has its initial timestamp stored.
Change-Id: I4ba15decec22cd473e63149ec399d82c5e3fd214 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/xeon_sp/bootblock.c 1 file changed, 14 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/47738/2
Hello build bot (Jenkins), Marc Jones, Jonathan Zhang, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47738
to look at the new patch set (#3).
Change subject: soc/intel/xeon_sp: Work around FSP-T not respecting its own API ......................................................................
soc/intel/xeon_sp: Work around FSP-T not respecting its own API
The CPX FSP-T does not respect the FSP2.x spec and uses registers where coreboot has its initial timestamp stored.
If the initial timestamp is later than some other timestamps this messes up the timestamps 'cbmem -t' reports as it thinks they are a result from a timestamp overflow (reporting that it took 100k years to boot).
TEST: The ocp/deltalake boots within the span of a lifetime.
Change-Id: I4ba15decec22cd473e63149ec399d82c5e3fd214 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/xeon_sp/bootblock.c 1 file changed, 14 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/47738/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47738 )
Change subject: soc/intel/xeon_sp: Work around FSP-T not respecting its own API ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/47738/3/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock.c:
https://review.coreboot.org/c/coreboot/+/47738/3/src/soc/intel/xeon_sp/bootb... PS3, Line 69: printk(BIOS_WARNING, "Invalid initial timestamp detected\n"); nit: Maybe guard this with BOOTBLOCK_CONSOLE as well?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47738 )
Change subject: soc/intel/xeon_sp: Work around FSP-T not respecting its own API ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/47738/3/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock.c:
https://review.coreboot.org/c/coreboot/+/47738/3/src/soc/intel/xeon_sp/bootb... PS3, Line 69: printk(BIOS_WARNING, "Invalid initial timestamp detected\n");
nit: Maybe guard this with BOOTBLOCK_CONSOLE as well?
Saw follow-up.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47738 )
Change subject: soc/intel/xeon_sp: Work around FSP-T not respecting its own API ......................................................................
soc/intel/xeon_sp: Work around FSP-T not respecting its own API
The CPX FSP-T does not respect the FSP2.x spec and uses registers where coreboot has its initial timestamp stored.
If the initial timestamp is later than some other timestamps this messes up the timestamps 'cbmem -t' reports as it thinks they are a result from a timestamp overflow (reporting that it took 100k years to boot).
TEST: The ocp/deltalake boots within the span of a lifetime.
Change-Id: I4ba15decec22cd473e63149ec399d82c5e3fd214 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/47738 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/xeon_sp/bootblock.c 1 file changed, 14 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/soc/intel/xeon_sp/bootblock.c b/src/soc/intel/xeon_sp/bootblock.c index 5adda44..baf5ab5 100644 --- a/src/soc/intel/xeon_sp/bootblock.c +++ b/src/soc/intel/xeon_sp/bootblock.c @@ -33,11 +33,20 @@ .UpdTerminator = 0x55AA, };
+static uint64_t assembly_timestamp; +static uint64_t bootblock_timestamp; + asmlinkage void bootblock_c_entry(uint64_t base_timestamp) { + /* + * FSP-T does not respect its own API and trashes registers + * coreboot uses to store its initial timestamp. + */ + assembly_timestamp = base_timestamp; + bootblock_timestamp = timestamp_get(); fast_spi_cache_bios_region();
- bootblock_main_with_basetime(base_timestamp); + bootblock_main_with_basetime(MIN(assembly_timestamp, bootblock_timestamp)); }
void bootblock_soc_early_init(void) @@ -55,6 +64,10 @@ { if (CONFIG(BOOTBLOCK_CONSOLE)) printk(BIOS_DEBUG, "FSP TempRamInit successful...\n"); + + if (assembly_timestamp > bootblock_timestamp) + printk(BIOS_WARNING, "Invalid initial timestamp detected\n"); + if (CONFIG(FSP_CAR)) report_fspt_output(); bootblock_pch_init();
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Patrick Georgi (Code Review)