Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38301 )
Change subject: timestamps: Fix syncing to CBMEM ......................................................................
timestamps: Fix syncing to CBMEM
For timestamps added before CBMEM coming online and call to timestamp_sync_cache_to_cbmem(), ts_table->base_time was subtracted twice.
Change-Id: Ia786c12c68c8921c0d09bc58a29fefdc72bf0c6d Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/lib/timestamp.c 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/38301/1
diff --git a/src/lib/timestamp.c b/src/lib/timestamp.c index d2206f6..3855f6d 100644 --- a/src/lib/timestamp.c +++ b/src/lib/timestamp.c @@ -128,7 +128,7 @@
tse = &ts_table->entries[ts_table->num_entries++]; tse->entry_id = id; - tse->entry_stamp = ts_time - ts_table->base_time; + tse->entry_stamp = ts_time;
if (ts_table->num_entries == ts_table->max_entries) printk(BIOS_ERR, "ERROR: Timestamp table full\n"); @@ -148,6 +148,7 @@ return; }
+ ts_time -= ts_table->base_time; timestamp_add_table_entry(ts_table, id, ts_time);
if (CONFIG(TIMESTAMPS_ON_CONSOLE))
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38301 )
Change subject: timestamps: Fix syncing to CBMEM ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38301/1/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/38301/1/src/lib/timestamp.c@223 PS1, Line 223: if (ts_cbmem_table->base_time == 0) Hmm.. this is nowadays always true and lot of the commentary above is obsolete?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38301 )
Change subject: timestamps: Fix syncing to CBMEM ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38301/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38301/1//COMMIT_MSG@11 PS1, Line 11: subtracted twice. Not very obvious, but the second time it's supposed to subtract 0 (`ts_cbmem_table->base_time` is not set yet).
https://review.coreboot.org/c/coreboot/+/38301/1/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/38301/1/src/lib/timestamp.c@155 PS1, Line 155: printk(BIOS_INFO, "Timestamp - %s: %llu\n", timestamp_name(id), ts_time); Also makes this output relative to the `base_time`.
https://review.coreboot.org/c/coreboot/+/38301/1/src/lib/timestamp.c@223 PS1, Line 223: if (ts_cbmem_table->base_time == 0)
Hmm.. […]
Well, the code looks odd anyway: Unconditionally migrate entries but conditionally migrate `base_time`? It's confusing.
Hello Julius Werner, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38301
to look at the new patch set (#2).
Change subject: timestamps: Fix syncing, logging and comments ......................................................................
timestamps: Fix syncing, logging and comments
For timestamps added before CBMEM coming online and call to timestamp_sync_cache_to_cbmem(), ts_table->base_time was subtracted twice. The second time though, the value of zero was subtracted.
Make the stamps logged on the console relative to base_time too, such that cbmem -1 and cbmem -c outputs will match.
Remove comments about postponing initialisation of timestamps to ramstage, that does not happen anymore.
Change-Id: Ia786c12c68c8921c0d09bc58a29fefdc72bf0c6d Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/lib/timestamp.c 1 file changed, 6 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/38301/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38301 )
Change subject: timestamps: Fix syncing, logging and comments ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38301/1/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/38301/1/src/lib/timestamp.c@155 PS1, Line 155: printk(BIOS_INFO, "Timestamp - %s: %llu\n", timestamp_name(id), ts_time);
Also makes this output relative to the `base_time`.
Done
https://review.coreboot.org/c/coreboot/+/38301/1/src/lib/timestamp.c@223 PS1, Line 223: if (ts_cbmem_table->base_time == 0)
Well, the code looks odd anyway: Unconditionally migrate entries but […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38301 )
Change subject: timestamps: Fix syncing, logging and comments ......................................................................
Patch Set 2:
Yes, that fixed the issues for me. Tested with QEMU i440fx.
``` coreboot-4.11-750-gcc524cc7c5 Thu Jan 9 13:28:54 UTC 2020 bootblock starting (log level: 7)... Timestamp - end of bootblock: 10429189 FMAP: Found "FLASH" version 1.1 at 0x0. FMAP: base = 0xfffc0000 size = 0x40000 #areas = 3 FMAP: area COREBOOT found @ 200 (261632 bytes) CBFS: 'COREBOOT Locator' located CBFS at [200:40000) CBFS: Locating 'fallback/romstage' CBFS: Found @ offset 80 size 475c Timestamp - starting to load romstage: 19598638 Timestamp - finished loading romstage: 20910354 BS: bootblock times (exec / console): total (unknown) / 6 ms
coreboot-4.11-750-gcc524cc7c5 Thu Jan 9 13:28:54 UTC 2020 romstage starting (log level: 7)... QEMU: firmware config interface detected Firmware config version id: 3 CBMEM: IMD: root @ 0x3ffff000 254 entries. IMD: root @ 0x3fffec00 62 entries. MTRR Range: Start=fffc0000 End=0 (Size 40000) FMAP: area COREBOOT found @ 200 (261632 bytes) CBFS: 'COREBOOT Locator' located CBFS at [200:40000) CBFS: Locating 'fallback/postcar' CBFS: Found @ offset 11080 size 4e34 Decompressing stage fallback/postcar @ 0x3ffd2fc0 (36080 bytes) Loading module at 0x3ffd3000 with entry 0x3ffd3000. filesize: 0x49d0 memsize: 0x8cb0 Processing 258 relocs. Offset value of 0x3dfd3000 Timestamp - end of romstage: 55101609 BS: romstage times (exec / console): total (unknown) / 9 ms
coreboot-4.11-750-gcc524cc7c5 Thu Jan 9 13:28:54 UTC 2020 postcar starting (log level: 7)... Timestamp - start of postcar: 67419529 Timestamp - end of postcar: 68799422 FMAP: area COREBOOT found @ 200 (261632 bytes) CBFS: 'COREBOOT Locator' located CBFS at [200:40000) CBFS: Locating 'fallback/ramstage' CBFS: Found @ offset 4840 size c1be Timestamp - starting to load ramstage: 76122068 Decompressing stage fallback/ramstage @ 0x3ffadfc0 (144936 bytes) Timestamp - starting LZMA decompress (ignore for x86): 80102766 Timestamp - finished LZMA decompress (ignore for x86): 96925699 Loading module at 0x3ffae000 with entry 0x3ffae000. filesize: 0x18798 memsize: 0x235e8 Processing 1619 relocs. Offset value of 0x3f1ae000 Timestamp - finished loading ramstage: 108208604 BS: postcar times (exec / console): total (unknown) / 12 ms
coreboot-4.11-750-gcc524cc7c5 Thu Jan 9 13:28:54 UTC 2020 ramstage starting (log level: 7)... Timestamp - start of ramstage: 118835232 Timestamp - device enumeration: 120369140 BS: BS_DEV_INIT_CHIPS run times (exec / console): 0 / 1 ms
[…] ```
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38301 )
Change subject: timestamps: Fix syncing, logging and comments ......................................................................
Patch Set 2: Code-Review+1
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38301 )
Change subject: timestamps: Fix syncing, logging and comments ......................................................................
Patch Set 2: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38301 )
Change subject: timestamps: Fix syncing, logging and comments ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38301/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38301/1//COMMIT_MSG@11 PS1, Line 11: subtracted twice.
Not very obvious, but the second time it's supposed to subtract 0 […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38301 )
Change subject: timestamps: Fix syncing, logging and comments ......................................................................
Patch Set 2: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38301 )
Change subject: timestamps: Fix syncing, logging and comments ......................................................................
timestamps: Fix syncing, logging and comments
For timestamps added before CBMEM coming online and call to timestamp_sync_cache_to_cbmem(), ts_table->base_time was subtracted twice. The second time though, the value of zero was subtracted.
Make the stamps logged on the console relative to base_time too, such that cbmem -1 and cbmem -c outputs will match.
Remove comments about postponing initialisation of timestamps to ramstage, that does not happen anymore.
Change-Id: Ia786c12c68c8921c0d09bc58a29fefdc72bf0c6d Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38301 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Martin Roth martinroth@google.com Reviewed-by: Nico Huber nico.h@gmx.de --- M src/lib/timestamp.c 1 file changed, 6 insertions(+), 9 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Nico Huber: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve
diff --git a/src/lib/timestamp.c b/src/lib/timestamp.c index d2206f6..9cbe308 100644 --- a/src/lib/timestamp.c +++ b/src/lib/timestamp.c @@ -128,7 +128,7 @@
tse = &ts_table->entries[ts_table->num_entries++]; tse->entry_id = id; - tse->entry_stamp = ts_time - ts_table->base_time; + tse->entry_stamp = ts_time;
if (ts_table->num_entries == ts_table->max_entries) printk(BIOS_ERR, "ERROR: Timestamp table full\n"); @@ -148,6 +148,7 @@ return; }
+ ts_time -= ts_table->base_time; timestamp_add_table_entry(ts_table, id, ts_time);
if (CONFIG(TIMESTAMPS_ON_CONSOLE)) @@ -206,22 +207,18 @@ * * If you try to initialize timestamps before ramstage but don't define * a TIMESTAMP region, all operations will fail (safely), and coreboot - * will behave as if timestamps only get initialized in ramstage. - * - * If timestamps only get initialized in ramstage, the base_time from - * timestamp_init() will get ignored and all timestamps will be 0-based. + * will behave as if timestamps collection was disabled. */
+ /* Inherit cache base_time. */ + ts_cbmem_table->base_time = ts_cache_table->base_time; + for (i = 0; i < ts_cache_table->num_entries; i++) { struct timestamp_entry *tse = &ts_cache_table->entries[i]; timestamp_add_table_entry(ts_cbmem_table, tse->entry_id, tse->entry_stamp); }
- /* Freshly added cbmem table has base_time 0. Inherit cache base_time */ - if (ts_cbmem_table->base_time == 0) - ts_cbmem_table->base_time = ts_cache_table->base_time; - /* Cache no longer required. */ ts_cache_table->num_entries = 0; }
-
Kyösti Mälkki (Code Review) -
Martin Roth (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)