Change in coreboot[master]: lib/bootblock.c: Work around invalid timestamp argument

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47699 )

Change subject: lib/bootblock.c: Work around invalid timestamp argument
......................................................................

lib/bootblock.c: Work around invalid timestamp argument

FSP-T does not respect its own spec and uses registers where coreboot
has its initial timestamp stored.

Change-Id: I8515195f50fef5f5e26a542bbef7501ba4c9f768
Signed-off-by: Arthur Heymans 
---
M src/lib/bootblock.c
1 file changed, 9 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/47699/1

diff --git a/src/lib/bootblock.c b/src/lib/bootblock.c
index 1509c8c..e894e23 100644
--- a/src/lib/bootblock.c
+++ b/src/lib/bootblock.c
@@ -26,11 +26,16 @@
 void bootblock_main_with_timestamp(uint64_t base_timestamp,
    struct timestamp_entry *timestamps, size_t num_timestamps)
 {
+	const uint64_t bootblock_main_timestamp = timestamp_get();
    /* Initialize timestamps if we have TIMESTAMP region in memlayout.ld. */
    if (CONFIG(COLLECT_TIMESTAMPS) &&
        REGION_SIZE(timestamp) > 0) {
    	int i;
-		timestamp_init(base_timestamp);
+		uint64_t initial_timestamp = base_timestamp;
+		/* Check function argument sanity */
+		if (bootblock_main_timestamp < base_timestamp)
+			initial_timestamp = bootblock_main_timestamp;
+		timestamp_init(initial_timestamp);
    	for (i = 0; i < num_timestamps; i++)
    		timestamp_add(timestamps[i].entry_id,
    			      timestamps[i].entry_stamp);
@@ -52,6 +57,9 @@
    	exception_init();
    }

+	if (bootblock_main_timestamp < base_timestamp)
+		printk(BIOS_NOTICE, "%s: invalid base_timestamp argument\n", __func__);
+
    bootblock_soc_init();
    bootblock_mainboard_init();

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47699 )

Change subject: lib/bootblock.c: Work around invalid timestamp argument
......................................................................

Patch Set 1: Code-Review-1

This sounds like a fix in generic code to a very platform-specific problem, so generally not the right thing to do. Can you explain in more detail what the problem is? There should be some way to solve this without changing the code for all other architectures (e.g. if the platform doesn't have a valid initial timestamp, it should just be entering through main() instead of bootblock_main_with_basetime()).

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47699 )

Change subject: lib/bootblock.c: Work around invalid timestamp argument
......................................................................

Patch Set 1: Code-Review-2
> Patch Set 1: Code-Review-1
>
> This sounds like a fix in generic code to a very platform-specific problem, so generally not the right thing to do. Can you explain in more detail what the problem is? There should be some way to solve this without changing the code for all other architectures (e.g. if the platform doesn't have a valid initial timestamp, it should just be entering through main() instead of bootblock_main_with_basetime()).

Agreed. This issue seems to happen on soc/intel/xeon_sp/cpx where FSP-T violates its own spec. FSP-T should not trash the content of some registers. Coreboot stores the initial timestamp in those. So what happens is that the initial timestamp is later than some following timestamps.

The proper place for this fix seems to be soc/intel/xeon_sp/bootblock.c

[Arthur Heymans (Code Review)]

Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/47699 )

Change subject: lib/bootblock.c: Work around invalid timestamp argument
......................................................................

Abandoned

Superseded in CB:47738