Kangheui Won has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
soc/amd/picasso: pass verstage timestamps to x86
Remove stubs for psp_verstage and initialize timestamp table with data from psp_verstage on bootblock.
We need to convert numbers since timestamps on verstage uses microseconds granularity and x86 uses TSC.
BUG=b:154142138, b:159220781 BRANCH=zork TEST=boot to kernel, run 'cbmem -t' and check verstage timestamps are included in the result.
Change-Id: I5e89bb54f478153fb40ba51b5ab61fa20af3b99a Signed-off-by: Kangheui Won khwon@chromium.org --- M src/include/bootblock_common.h M src/lib/Makefile.inc M src/lib/bootblock.c M src/soc/amd/picasso/bootblock/bootblock.c M src/soc/amd/picasso/psp_verstage/Makefile.inc M src/soc/amd/picasso/psp_verstage/psp_verstage.c D src/soc/amd/picasso/psp_verstage/timestamp.c 7 files changed, 43 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45059/1
diff --git a/src/include/bootblock_common.h b/src/include/bootblock_common.h index 97ccf96..da627d2 100644 --- a/src/include/bootblock_common.h +++ b/src/include/bootblock_common.h @@ -29,6 +29,8 @@ asmlinkage void ap_bootblock_c_entry(void);
void bootblock_main_with_basetime(uint64_t base_timestamp); +void bootblock_main_with_timestamp(uint64_t base_timestamp, + struct timestamp_entry *timestamps, size_t num_timestamps);
/* This is the argument structure passed from decompressor to bootblock. */ struct bootblock_arg { diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index 4ce133a..5a960e9 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -68,10 +68,7 @@ verstage-y += memcmp.c verstage-y += string.c
-# TODO: Remove this when PSP bootblock timestamps are implemented. -ifeq ($(CONFIG_VBOOT_STARTS_BEFORE_BOOTBLOCK),) verstage-$(CONFIG_COLLECT_TIMESTAMPS) += timestamp.c -endif verstage-y += boot_device.c verstage-$(CONFIG_CONSOLE_CBMEM) += cbmem_console.c
diff --git a/src/lib/bootblock.c b/src/lib/bootblock.c index 1c433d0..3e2ef06 100644 --- a/src/lib/bootblock.c +++ b/src/lib/bootblock.c @@ -25,7 +25,7 @@ * entered from C code. This function assumes that the timer has already been * initialized, so it does not call init_timer(). */ -static void bootblock_main_with_timestamp(uint64_t base_timestamp, +void bootblock_main_with_timestamp(uint64_t base_timestamp, struct timestamp_entry *timestamps, size_t num_timestamps) { /* Initialize timestamps if we have TIMESTAMP region in memlayout.ld. */ diff --git a/src/soc/amd/picasso/bootblock/bootblock.c b/src/soc/amd/picasso/bootblock/bootblock.c index 4700027..2ba6be9 100644 --- a/src/soc/amd/picasso/bootblock/bootblock.c +++ b/src/soc/amd/picasso/bootblock/bootblock.c @@ -3,6 +3,7 @@ #include <stdint.h> #include <symbols.h> #include <amdblocks/reset.h> +#include <timestamp.h> #include <bootblock_common.h> #include <console/console.h> #include <cpu/x86/cache.h> @@ -119,7 +120,44 @@ write_resume_eip(); enable_pci_mmconf();
+#if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK) + struct transfer_info_struct *info = (struct transfer_info_struct *) + CONFIG_PSP_SHAREDMEM_BASE; + + if (info->magic_val == TRANSFER_MAGIC_VAL && + info->timestamp_offset != 0) { + int i; + uint32_t tick_freq_mhz = timestamp_tick_freq_mhz(); + struct timestamp_table *psp_ts_table = (struct timestamp_table *) + (CONFIG_PSP_SHAREDMEM_BASE + info->timestamp_offset); + + /* if we're unable to determine timestamp tick, leave it as raw number */ + if (tick_freq_mhz == 0) { + tick_freq_mhz = 1; + } + + for (i = 0; i < psp_ts_table->num_entries; i++) { + struct timestamp_entry *tse = &psp_ts_table->entries[i]; + /* + * Since bootblock_main_with_timestamp calls timestamp_add and + * timestamp_add subtracts base_time, we need to add base_time here. + * Also values are in us so multiply it by tick_freq_mhz + */ + uint64_t abs_stamp = psp_ts_table->base_time + tse->entry_stamp; + tse->entry_stamp = abs_stamp * tick_freq_mhz; + } + + base_timestamp = psp_ts_table->base_time * tick_freq_mhz; + bootblock_main_with_timestamp(base_timestamp, + psp_ts_table->entries, psp_ts_table->num_entries); + + } else { + bootblock_main_with_basetime(base_timestamp); + } +#else bootblock_main_with_basetime(base_timestamp); +#endif + }
void bootblock_soc_early_init(void) diff --git a/src/soc/amd/picasso/psp_verstage/Makefile.inc b/src/soc/amd/picasso/psp_verstage/Makefile.inc index 905613e..4f1642b 100644 --- a/src/soc/amd/picasso/psp_verstage/Makefile.inc +++ b/src/soc/amd/picasso/psp_verstage/Makefile.inc @@ -15,7 +15,6 @@ verstage-y += reset.c verstage-y += svc.c verstage-y += timer.c -verstage-y += timestamp.c verstage-y += vboot_crypto.c
verstage-y += $(top)/src/vendorcode/amd/fsp/picasso/bl_uapp/bl_uapp_startup.S diff --git a/src/soc/amd/picasso/psp_verstage/psp_verstage.c b/src/soc/amd/picasso/psp_verstage/psp_verstage.c index ef4884f..b429ef3 100644 --- a/src/soc/amd/picasso/psp_verstage/psp_verstage.c +++ b/src/soc/amd/picasso/psp_verstage/psp_verstage.c @@ -16,6 +16,7 @@ #include <arch/stages.h> #include <stdarg.h> #include <stdio.h> +#include <timestamp.h>
extern char _bss_start, _bss_end; static struct mem_region_device boot_dev = @@ -207,6 +208,7 @@ * Do not use printk() before console_init() * Do not use post_code() before verstage_mainboard_init() */ + timestamp_init(timestamp_get()); svc_write_postcode(POSTCODE_ENTERED_PSP_VERSTAGE); svc_debug_print("Entering verstage on PSP\n"); memset(&_bss_start, '\0', &_bss_end - &_bss_start); diff --git a/src/soc/amd/picasso/psp_verstage/timestamp.c b/src/soc/amd/picasso/psp_verstage/timestamp.c deleted file mode 100644 index b3b8f75..0000000 --- a/src/soc/amd/picasso/psp_verstage/timestamp.c +++ /dev/null @@ -1,17 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <timestamp.h> - -/* Stubs */ -void timestamp_add_now(enum timestamp_id id) -{ -} - -void timestamp_add(enum timestamp_id id, uint64_t ts) -{ -} - -uint64_t timestamp_get(void) -{ - return 0; -}
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45059/1/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/1/src/soc/amd/picasso/bootblo... PS1, Line 135: if (tick_freq_mhz == 0) { braces {} are not necessary for single statement blocks
Hello Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45059
to look at the new patch set (#2).
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
soc/amd/picasso: pass verstage timestamps to x86
Remove stubs for psp_verstage and initialize timestamp table with data from psp_verstage on bootblock.
We need to convert numbers since timestamps on verstage uses microseconds granularity and x86 uses TSC.
BUG=b:154142138, b:159220781 BRANCH=zork TEST=boot to kernel, run 'cbmem -t' and check verstage timestamps are included in the result.
Change-Id: I5e89bb54f478153fb40ba51b5ab61fa20af3b99a Signed-off-by: Kangheui Won khwon@chromium.org --- M src/include/bootblock_common.h M src/lib/Makefile.inc M src/lib/bootblock.c M src/soc/amd/picasso/bootblock/bootblock.c M src/soc/amd/picasso/psp_verstage/Makefile.inc M src/soc/amd/picasso/psp_verstage/psp_verstage.c D src/soc/amd/picasso/psp_verstage/timestamp.c 7 files changed, 42 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45059/2
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 2: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45059/2/src/lib/bootblock.c File src/lib/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/2/src/lib/bootblock.c@28 PS2, Line 28: void bootblock_main_with_timestamp(uint64_t base_timestamp, These bootblock changes should reside in their own patch.
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 123: #if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK) Why is this guarded by a macro and not a C if() ?
if (CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)) bootblock_main_with_timestamp(...); else bootblock_main_with_basetime(...);
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 125: CONFIG_PSP_SHAREDMEM_BASE; You should cast ints through uintptr_t for establishing pointer values.
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 130: uint32_t tick_freq_mhz = timestamp_tick_freq_mhz(); This is the tsc frequency since we aren't selecting COLLECT_TIMESTAMPS_NO_TSC. How are we aligning the base times across x86 and psp? I would expect us to normalize on a common clock.
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45059/2/src/lib/bootblock.c File src/lib/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/2/src/lib/bootblock.c@28 PS2, Line 28: void bootblock_main_with_timestamp(uint64_t base_timestamp,
These bootblock changes should reside in their own patch.
Okay, I'll split patches.
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 123: #if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)
Why is this guarded by a macro and not a C if() ? […]
No particular reason, I just followed what's done in bootblock_soc_init() below.
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 130: uint32_t tick_freq_mhz = timestamp_tick_freq_mhz();
This is the tsc frequency since we aren't selecting COLLECT_TIMESTAMPS_NO_TSC. […]
It's done in line 145-149. Since timestamps from psp is normalized as 1us scale we can just multiply this. However if timestamp_tick_freq_mhz() returns 0 (which means there was error getting it) we're leaving it as raw number.
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45059/2/src/lib/bootblock.c File src/lib/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/2/src/lib/bootblock.c@28 PS2, Line 28: void bootblock_main_with_timestamp(uint64_t base_timestamp,
Okay, I'll split patches.
Done
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 125: CONFIG_PSP_SHAREDMEM_BASE;
You should cast ints through uintptr_t for establishing pointer values.
Are you referring to line 132? Or did you mean "(struct transfer_info_struct *)(uintptr_t)CONFIG_PSP_SHAREDMEM_BASE"?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 123: #if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)
No particular reason, I just followed what's done in bootblock_soc_init() below.
That shouldn't be guarded by an #if either.
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 125: CONFIG_PSP_SHAREDMEM_BASE;
Are you referring to line 132? Or did you mean "(struct transfer_info_struct *)(uintptr_t)CONFIG_PSP […]
CONFIG_PSP_SHAREDMEM is a #define that is an int to the type system. ints should not be casted as a pointer for code compatibility. They should be cast through a pointer type that has the correct length. Your suggestion of using (uintptr_t) would be correct, but it might need to go through (void *) as well. i.e. (struct transfer_info_struct *)(void *)(uintptr_t)CONFIG_PSP_SHAREDMEM_BASE;
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 130: uint32_t tick_freq_mhz = timestamp_tick_freq_mhz();
It's done in line 145-149. […]
We should normalize to use 1us tick regardless of environment. Then there's no funny math required.
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 149: base_timestamp = psp_ts_table->base_time * tick_freq_mhz; None of this ensures the timestamp from x86 is monotonically increasing relative to psp. What are you doing to ensure a consistent timeline?
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45059/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45059/3//COMMIT_MSG@8 PS3, Line 8: : Remove stubs for psp_verstage and This was moved to the other patch.
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... PS3, Line 125: CONFIG_PSP_SHAREDMEM_BASE Would it be better to use the '_transfer_buffer' symbol here? It's based on the value in CONFIG_PSP_SHAREDMEM_BASE, so it's not a critical change, but might make more sense.
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... PS3, Line 132: CONFIG_PSP_SHAREDMEM_BASE Same idea here, but maybe use the symbol _psp_sharedmem_dram?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... PS3, Line 146: tse->entry_stamp = abs_stamp * tick_freq_mhz; I don't think this math is correct (despite my objection of doing this at all). You essentially have two time domains and trying to align the units. 2 multiplications won't scale that since abs_stamp is just in the time unit from psp (in this case 1 us but nothing guarantees that). The timestamp_table object has tick_freq_mhz in it. The values need to be scaled up or down based on timestamp_tick_freq_mhz() and the one in the transfer table. It seems you are assuming everything is 1us and not commenting on that assumption.
COLLECT_TIMESTAMPS_NO_TSC uses the monotonic timer as a timestamp source which by definition is in microseconds. Therefore, when that is used nothing is needed for scaling (though there's still an issue of monotonically increasing values which may need a relative offset).
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... PS3, Line 157: bootblock_main_with_basetime(base_timestamp); As I noted before line 154 and this should be collapsed as well as removing #if conditionals.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... PS3, Line 123: #if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK) Instead of letting the preprocessor do this, can this be done in C?
Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45059
to look at the new patch set (#4).
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
soc/amd/picasso: pass verstage timestamps to x86
Initialize timestamp table with data from psp_verstage on bootblock.
We need to convert numbers since timestamps on verstage uses microseconds granularity and x86 uses TSC.
BUG=b:154142138, b:159220781 BRANCH=zork TEST=boot to kernel, run 'cbmem -t' and check verstage timestamps are included in the result.
Change-Id: I5e89bb54f478153fb40ba51b5ab61fa20af3b99a Signed-off-by: Kangheui Won khwon@chromium.org --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 35 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45059/4
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45059/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45059/3//COMMIT_MSG@8 PS3, Line 8: : Remove stubs for psp_verstage and
This was moved to the other patch.
Done
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 123: #if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)
That shouldn't be guarded by an #if either.
Done
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... PS3, Line 123: #if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)
Instead of letting the preprocessor do this, can this be done in C?
Done
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... PS3, Line 157: bootblock_main_with_basetime(base_timestamp);
As I noted before line 154 and this should be collapsed as well as removing #if conditionals.
Done
Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45059
to look at the new patch set (#5).
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
soc/amd/picasso: pass verstage timestamps to x86
Initialize timestamp table with data from psp_verstage on bootblock.
We need to convert numbers since timestamps on verstage uses microseconds granularity and x86 uses TSC.
BUG=b:154142138, b:159220781 BRANCH=zork TEST=boot to kernel, run 'cbmem -t' and check verstage timestamps are included in the result.
Change-Id: I5e89bb54f478153fb40ba51b5ab61fa20af3b99a Signed-off-by: Kangheui Won khwon@chromium.org --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 35 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45059/5
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 5:
(6 comments)
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 125: CONFIG_PSP_SHAREDMEM_BASE;
CONFIG_PSP_SHAREDMEM is a #define that is an int to the type system. […]
Done
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 130: uint32_t tick_freq_mhz = timestamp_tick_freq_mhz();
We should normalize to use 1us tick regardless of environment. Then there's no funny math required.
I think this conversation continued on another comment on patchset 3, if it's different question please reopen this.
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 149: base_timestamp = psp_ts_table->base_time * tick_freq_mhz;
None of this ensures the timestamp from x86 is monotonically increasing relative to psp. […]
I specifically asked AMD about this and haven't got answer yet: b/167148121
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... PS3, Line 125: CONFIG_PSP_SHAREDMEM_BASE
Would it be better to use the '_transfer_buffer' symbol here? It's based on the value in CONFIG_PSP […]
Done
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... PS3, Line 132: CONFIG_PSP_SHAREDMEM_BASE
Same idea here, but maybe use the symbol _psp_sharedmem_dram?
Changed to _transfer_buffer. Is there any reason to use two different names?
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... PS3, Line 146: tse->entry_stamp = abs_stamp * tick_freq_mhz;
I don't think this math is correct (despite my objection of doing this at all). […]
If we use COLLECT_TIMESTAMPS_NO_TSC we need to supply monotonic timer instead of TSC. Are you suggesting that we should use PTSC for timestamps like what we did on stoneyridge?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... PS3, Line 146: tse->entry_stamp = abs_stamp * tick_freq_mhz;
If we use COLLECT_TIMESTAMPS_NO_TSC we need to supply monotonic timer instead of TSC. […]
That or use tsc as the implementation for monotonic timer. One still needs to figure out the offset issue (when the TSC actually started, if using TSC) and carry it forward through the rest of the stages so the timestamps make sense.
Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45059
to look at the new patch set (#6).
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
soc/amd/picasso: pass verstage timestamps to x86
Initialize timestamp table with data from psp_verstage on bootblock.
PSP will keep its own timer value when x86 is released on transfer_buffer. So we need certain offset to x86 timer but it's difficult to keep the offset across various stages in coreboot and depthcharge.
Adding value directly to TSC can sovle it, it will be kept across coreboot and depthcharge.
BUG=b:159220781, b:167148121 BRANCH=zork TEST=boot to kernel, run 'cbmem -t' and check verstage timestamps are included in the result.
Change-Id: I5e89bb54f478153fb40ba51b5ab61fa20af3b99a Signed-off-by: Kangheui Won khwon@chromium.org --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 50 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45059/6
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 149: base_timestamp = psp_ts_table->base_time * tick_freq_mhz;
I specifically asked AMD about this and haven't got answer yet: b/167148121
Fixed on latest change
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... PS3, Line 146: tse->entry_stamp = abs_stamp * tick_freq_mhz;
That or use tsc as the implementation for monotonic timer. […]
separated that into CB:46058
Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45059/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45059/6//COMMIT_MSG@16 PS6, Line 16: Adding value directly to TSC can sovle it, it will be kept across nit "solve"
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 124: struct transfer_info_struct *info = (struct transfer_info_struct *) this assumes I have a transfer buffer set up. What if I don't? What if vboot is running after boot block? Should this be after the if-config check?
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 6: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 157: * Since bootblock_main_with_timestamp calls What about adding a flag to bootblock_main_with_timestamp?
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 161: tse->entry_stamp = psp_ts_table->base_time + tse->entry_stamp; nit: +=
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 138: (uint64_t)info->timestamp This is implicitly assuming the time unit is 1us.
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 144: wrmsr(TSC_MSR, msr); Rewriting the TSC counter out of sequence with the other APs can cause issues. Did the kernel complain? Are the TSCs being resync'd somewhere else?
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 161: psp_ts_table->base_tim Assuming 1us unit.
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 6: Code-Review-1
(3 comments)
Need further work -
a) Ensure that out-of-sync TSC won't cause any side-effects in kernel. b) Combined with a), need additional verification on S3 resume path.
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 138: (uint64_t)info->timestamp
This is implicitly assuming the time unit is 1us.
I stated that in line 131.
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 144: wrmsr(TSC_MSR, msr);
Rewriting the TSC counter out of sequence with the other APs can cause issues. […]
Yes kernel complained about it - [ 0.000000] tsc: Fast TSC calibration using PIT [ 0.289103] TSC synchronization [CPU#0 -> CPU#1]: [ 0.289103] Measured 5514916724 cycles TSC warp between CPUs, turning off TSC clock. [ 0.289103] tsc: Marking TSC unstable due to check_tsc_sync_source failed
I need to investigate further if this has any side effects.
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 157: * Since bootblock_main_with_timestamp calls
What about adding a flag to bootblock_main_with_timestamp?
What flag do you mean?
Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi, Rob Barnes, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45059
to look at the new patch set (#7).
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
soc/amd/picasso: pass verstage timestamps to x86
Initialize timestamp table with data from psp_verstage on bootblock.
PSP keeps its own timestamp and pass it in transfer_buffer. However PSP timestamp and TSC may be out of sync so we can't just merge two tables without modification.
info->timestamp contains PSP's clock value (in us) when x86 processor released and base_timestamp contains TSC value when bootblock is started. The time between x86 release and bootblock entry should be very short so we can think those two happened at the same time and use them for sync.
In some cases there will be underflow in timestamp entries but cbmem utility can handle wrap-over in entries. Few timestamp values including 1st timestamp can be very large but we can still get the time spent on boot without any problem.
BUG=b:159220781, b:167148121, b:171422583 BRANCH=zork TEST=boot to kernel, run 'cbmem -t' and check verstage timestamps are included in the result.
Change-Id: I5e89bb54f478153fb40ba51b5ab61fa20af3b99a Signed-off-by: Kangheui Won khwon@chromium.org --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 50 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45059/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45059/7/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/7/src/soc/amd/picasso/bootblo... PS7, Line 151: * We ignore the time between x86 processor release and bootblock. line over 96 characters
https://review.coreboot.org/c/coreboot/+/45059/7/src/soc/amd/picasso/bootblo... PS7, Line 157: * We don't need to convert unit since both PSP and coreboot will use line over 96 characters
https://review.coreboot.org/c/coreboot/+/45059/7/src/soc/amd/picasso/bootblo... PS7, Line 161: tse->entry_stamp += psp_ts_table->base_time + base_timestamp - psp_last_ts; line over 96 characters
Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi, Rob Barnes, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45059
to look at the new patch set (#8).
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
soc/amd/picasso: pass verstage timestamps to x86
Initialize timestamp table with data from psp_verstage on bootblock.
PSP keeps its own timestamp and pass it in transfer_buffer. However PSP timestamp and TSC may be out of sync so we can't just merge two tables without modification.
info->timestamp contains PSP's clock value (in us) when x86 processor released and base_timestamp contains TSC value when bootblock is started. The time between x86 release and bootblock entry should be very short so we can think those two happened at the same time and use them for sync.
In some cases there will be underflow in timestamp entries but cbmem utility can handle wrap-over in entries. Few timestamp values including 1st timestamp can be very large but we can still get the time spent on boot without any problem.
BUG=b:159220781, b:167148121, b:171422583 BRANCH=zork TEST=boot to kernel, run 'cbmem -t' and check verstage timestamps are included in the result.
Change-Id: I5e89bb54f478153fb40ba51b5ab61fa20af3b99a Signed-off-by: Kangheui Won khwon@chromium.org --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 52 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45059/8
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 8:
(5 comments)
https://review.coreboot.org/c/coreboot/+/45059/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45059/6//COMMIT_MSG@16 PS6, Line 16: Adding value directly to TSC can sovle it, it will be kept across
nit "solve"
Done
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 124: struct transfer_info_struct *info = (struct transfer_info_struct *)
this assumes I have a transfer buffer set up. […]
_transfer_buffer is defined in src/soc/amd/picasso/memlayout_x86.ld and guarded with CONFIG(VBOOT). Do we also need to worry about when VBOOT is not used?
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 144: wrmsr(TSC_MSR, msr);
Yes kernel complained about it - […]
Latest patchset does not touch TSC value
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 161: tse->entry_stamp = psp_ts_table->base_time + tse->entry_stamp;
nit: +=
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 161: psp_ts_table->base_tim
Assuming 1us unit.
Stated in the comments.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45059/8/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/8/src/soc/amd/picasso/bootblo... PS8, Line 134: info->magic_val == TRANSFER_MAGIC_VAL) { Maybe break this out into a separate function and put the call to that function inside this if?
Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi, Rob Barnes, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45059
to look at the new patch set (#9).
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
soc/amd/picasso: pass verstage timestamps to x86
Initialize timestamp table with data from psp_verstage on bootblock.
PSP keeps its own timestamp and pass it in transfer_buffer. However PSP timestamp and TSC may be out of sync so we can't just merge two tables without modification.
info->timestamp contains PSP's clock value (in us) when x86 processor released and base_timestamp contains TSC value when bootblock is started. The time between x86 release and bootblock entry should be very short so we can think those two happened at the same time and use them for sync.
In some cases there will be underflow in timestamp entries but cbmem utility can handle wrap-over in entries. Few timestamp values including 1st timestamp can be very large but we can still get the time spent on boot without any problem.
BUG=b:159220781, b:167148121, b:171422583 BRANCH=zork TEST=boot to kernel, run 'cbmem -t' and check verstage timestamps are included in the result.
Change-Id: I5e89bb54f478153fb40ba51b5ab61fa20af3b99a Signed-off-by: Kangheui Won khwon@chromium.org --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 60 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45059/9
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45059/8/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/8/src/soc/amd/picasso/bootblo... PS8, Line 134: info->magic_val == TRANSFER_MAGIC_VAL) {
Maybe break this out into a separate function and put the call to that function inside this if?
Done
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 9: Code-Review+2
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 9:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/3/src/soc/amd/picasso/bootblo... PS3, Line 132: CONFIG_PSP_SHAREDMEM_BASE
Changed to _transfer_buffer. […]
It's not really a big deal, since one is based on the other, but it's probably a good idea to use the symbol from the linker table instead of the Kconfig option. Maybe this is just a personal preference.
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 124: struct transfer_info_struct *info = (struct transfer_info_struct *)
_transfer_buffer is defined in src/soc/amd/picasso/memlayout_x86.ld and guarded with CONFIG(VBOOT). […]
Yeah, I think we do need to worry about when vboot isn't used. Not for chromeos, but typically the chromeos platforms will also be ported to a non-chromeos firmware version. Check out https://mrchromebox.tech/ to see the ports.
Maybe put the struct and all the code inside the if into a separate function, then run the old bootblock main in this func if that one didn't set the timestamps? In there, you can just check if verstage is enabled and starts before bootblock and return immediately if it isn't.
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 138: (uint64_t)info->timestamp
I stated that in line 131.
Ack
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 157: * Since bootblock_main_with_timestamp calls
What flag do you mean?
Rob, are you saying to put the code to adjust all the timestamps based on the base time into bootblock_main_with_timestamp, then pass in a flag to do that?
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 157: * Since bootblock_main_with_timestamp calls
Rob, are you saying to put the code to adjust all the timestamps based on the base time into bootblo […]
Yes, as martin said. Maybe that's making bootblock_main_with_timestamp to complex.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... PS9, Line 149: if (info->timestamp_offset != 0) { If this condition fails bootblock_main_* will never be called and we'll return to assembly.
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... PS9, Line 174: base_timestamp += psp_ts_table->base_time - psp_last_ts; I'm having a hard time following the logic.
A: time when psp timer started ticking (unknown in absolute terms) B: psp_ts_table->base_time is the first timestamp taken on psp. C: psp_last_ts is when x86 was released
A through C are in psp time domain of 1us tick.
D: first timestamp x86 reads -> base_timestamp in this code and relative to C
So the base_time in the new timestamp tables needs to have a negative offset of psp_last_time (C) for the x86 derived timestamps for the math to work out.
Why wouldn't we just subtract psp_last_ts from base_timestamp such that it gets added in timestamp_add() for every subsequent entry? Is psp_last_ts absolute or relative to psp_ts_table->base_time?
The math may work out, but the current code doesn't read as straight forward. Regardless of relative vs absolute things would read clearer by subtracting psp_last_ts which is calculating C - A as an offset. i.e. either:
relative: base_timestamp -= psp_last_ts + psp_ts_table->base_time; absolute: base_timestamp -= psp_last_ts;
I think the same is true for the psp entries. They should be adjusted according to the calculated offset once we know the new base_time being passed into bootblock_main_with_timestamp(). i.e. Those should be adjusted by the same magnitude but in the opposite direction.
Once we calculated new base_timestamp that same value should be used to manipulate the psp entries:
tse->entry_stamp -= base_timestamp;
Again, I think the math may work out, but it's not super clear. And we're duplicating the math when it should be calculated once and utilized.
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... PS9, Line 175: bootblock_main_with_timestamp(base_timestamp, FWIW, this manipulation of timestamp table base time moves the basis around that's impossible to maintain/retrieve time to first timestamp in psp. We lose all that visibility.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... PS9, Line 174: base_timestamp += psp_ts_table->base_time - psp_last_ts;
I'm having a hard time following the logic. […]
I should also note that it appears the code is assuming 'A' always have a value of 0. Is that true on resets and suspend resume?
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 9: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... PS9, Line 149: if (info->timestamp_offset != 0) {
If this condition fails bootblock_main_* will never be called and we'll return to assembly.
Yeah, that's bad.
Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi, Rob Barnes, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45059
to look at the new patch set (#10).
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
soc/amd/picasso: pass verstage timestamps to x86
Initialize timestamp table with data from psp_verstage on bootblock.
PSP keeps its own timestamp and pass it in transfer_buffer. However PSP timestamp and TSC may be out of sync so we can't just merge two tables without modification.
info->timestamp contains PSP's clock value (in us) when x86 processor released and base_timestamp contains TSC value when bootblock is started. The time between x86 release and bootblock entry should be very short so we can think those two happened at the same time and use them for sync.
In some cases there will be underflow in timestamp entries but cbmem utility can handle wrap-over in entries. Few timestamp values including 1st timestamp can be very large but we can still get the time spent on boot without any problem.
BUG=b:159220781, b:167148121, b:171422583 BRANCH=zork TEST=boot to kernel, run 'cbmem -t' and check verstage timestamps are included in the result.
Change-Id: I5e89bb54f478153fb40ba51b5ab61fa20af3b99a Signed-off-by: Kangheui Won khwon@chromium.org --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 66 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45059/10
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/6/src/soc/amd/picasso/bootblo... PS6, Line 124: struct transfer_info_struct *info = (struct transfer_info_struct *)
Yeah, I think we do need to worry about when vboot isn't used. […]
Done
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... PS9, Line 149: if (info->timestamp_offset != 0) {
Yeah, that's bad.
Oops, my bad. Fixed in latest patchset.
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... PS9, Line 174: base_timestamp += psp_ts_table->base_time - psp_last_ts;
I should also note that it appears the code is assuming 'A' always have a value of 0. […]
psp_last_ts is absolute so it's completely my mistake to put base_time here.
And I think what you suggested actually simplifies the math here a lot, thanks for the advice. Applied in latest patchset.
About PSP's initial timer value - it's nowhere documented but I think it has value of 0 on reset/resume from my experience.
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... PS9, Line 175: bootblock_main_with_timestamp(base_timestamp,
FWIW, this manipulation of timestamp table base time moves the basis around that's impossible to mai […]
In latest patchset we're moving basis to starting time of PSP timer, not the time when timestamp_init() is called in the PSP. So we're losing basis in the PSP but I think it's okay since the first entry(5:start of verified boot) will now contain that basis after adjusting and the time between basis in the PSP and the first entry is very small. (<2ms from past measurements)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 10:
(5 comments)
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... PS10, Line 118: static int transfer_buffer_valid(struct transfer_info_struct *ptr) const struct transfer_info_struct *ptr
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... PS10, Line 129: #if CONFIG(VBOOT) Why did this #if condition come back?
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... PS10, Line 130: struct transfer_info_struct *info = (struct transfer_info_struct *) const
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... PS10, Line 133: if (transfer_buffer_valid(info)) { fwiw, you can save horizontal real estate by bailing early:
if (!transfer_buffer_valid(info) || info->timestamp == 0) return;
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... PS10, Line 158: tse->entry_stamp += psp_ts_table->base_time + base_timestamp; I think you should probably leave a note that base_timestamp is added here because timestamp_add() subtracts that value when adding new entries.
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... PS10, Line 129: #if CONFIG(VBOOT)
Why did this #if condition come back?
Because _transfer_buffer is guarded with CONFIG(VBOOT) in soc/amd/picasso/memlayout_x86.ld. I thought if CONFIG_VBOOT is not selected below statement will cause compilation error, am I missing something?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... PS10, Line 129: #if CONFIG(VBOOT)
Because _transfer_buffer is guarded with CONFIG(VBOOT) in soc/amd/picasso/memlayout_x86.ld. […]
I had thought we worked around needing the #if. There's a DECLARE_REGION(transfer_buffer) in include/symbols.h which should provide the declaration of the symbols. The CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK) guard below will not need to link this code because of dead code elimination. It should work without?
Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Patrick Georgi, Rob Barnes, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45059
to look at the new patch set (#11).
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
soc/amd/picasso: pass verstage timestamps to x86
Initialize timestamp table with data from psp_verstage on bootblock.
PSP keeps its own timestamp and pass it in transfer_buffer. However PSP timestamp and TSC may be out of sync so we can't just merge two tables without modification.
info->timestamp contains PSP's clock value (in us) when x86 processor released and base_timestamp contains TSC value when bootblock is started. The time between x86 release and bootblock entry should be very short so we can think those two happened at the same time and use them for sync.
In some cases there will be underflow in timestamp entries but cbmem utility can handle wrap-over in entries. Few timestamp values including 1st timestamp can be very large but we can still get the time spent on boot without any problem.
BUG=b:159220781, b:167148121, b:171422583 BRANCH=zork TEST=boot to kernel, run 'cbmem -t' and check verstage timestamps are included in the result.
Change-Id: I5e89bb54f478153fb40ba51b5ab61fa20af3b99a Signed-off-by: Kangheui Won khwon@chromium.org --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 63 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/45059/11
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 11:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... PS10, Line 118: static int transfer_buffer_valid(struct transfer_info_struct *ptr)
const struct transfer_info_struct *ptr
Done
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... PS10, Line 129: #if CONFIG(VBOOT)
I had thought we worked around needing the #if. […]
Ah, thanks. I wasn't aware about DECLARE_REGION in symbols.h.
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... PS10, Line 130: struct transfer_info_struct *info = (struct transfer_info_struct *)
const
Done
https://review.coreboot.org/c/coreboot/+/45059/10/src/soc/amd/picasso/bootbl... PS10, Line 158: tse->entry_stamp += psp_ts_table->base_time + base_timestamp;
I think you should probably leave a note that base_timestamp is added here because timestamp_add() s […]
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 11: Code-Review+2
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/9/src/soc/amd/picasso/bootblo... PS9, Line 174: base_timestamp += psp_ts_table->base_time - psp_last_ts;
psp_last_ts is absolute so it's completely my mistake to put base_time here. […]
Done
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
soc/amd/picasso: pass verstage timestamps to x86
Initialize timestamp table with data from psp_verstage on bootblock.
PSP keeps its own timestamp and pass it in transfer_buffer. However PSP timestamp and TSC may be out of sync so we can't just merge two tables without modification.
info->timestamp contains PSP's clock value (in us) when x86 processor released and base_timestamp contains TSC value when bootblock is started. The time between x86 release and bootblock entry should be very short so we can think those two happened at the same time and use them for sync.
In some cases there will be underflow in timestamp entries but cbmem utility can handle wrap-over in entries. Few timestamp values including 1st timestamp can be very large but we can still get the time spent on boot without any problem.
BUG=b:159220781, b:167148121, b:171422583 BRANCH=zork TEST=boot to kernel, run 'cbmem -t' and check verstage timestamps are included in the result.
Change-Id: I5e89bb54f478153fb40ba51b5ab61fa20af3b99a Signed-off-by: Kangheui Won khwon@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/45059 Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 63 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/soc/amd/picasso/bootblock/bootblock.c b/src/soc/amd/picasso/bootblock/bootblock.c index c715324..4bff042 100644 --- a/src/soc/amd/picasso/bootblock/bootblock.c +++ b/src/soc/amd/picasso/bootblock/bootblock.c @@ -3,6 +3,7 @@ #include <stdint.h> #include <symbols.h> #include <amdblocks/reset.h> +#include <timestamp.h> #include <bootblock_common.h> #include <console/console.h> #include <cpu/x86/cache.h> @@ -10,6 +11,7 @@ #include <cpu/amd/msr.h> #include <cpu/x86/mtrr.h> #include <cpu/amd/mtrr.h> +#include <cpu/x86/tsc.h> #include <pc80/mc146818rtc.h> #include <soc/psp_transfer.h> #include <soc/southbridge.h> @@ -108,12 +110,73 @@ wrmsr(S3_RESUME_EIP_MSR, s3_resume_entry); }
+static int transfer_buffer_valid(const struct transfer_info_struct *ptr) +{ + if (ptr->magic_val == TRANSFER_MAGIC_VAL) + return 1; + else + return 0; +} + +static void boot_with_psp_timestamp(uint64_t base_timestamp) +{ + const struct transfer_info_struct *info = (const struct transfer_info_struct *) + (void *)(uintptr_t)_transfer_buffer; + + if (!transfer_buffer_valid(info) || info->timestamp == 0) + return; + + /* + * info->timestamp is PSP's timestamp (in microseconds) + * when x86 processor is released. + */ + uint64_t psp_last_ts = info->timestamp; + + int i; + struct timestamp_table *psp_ts_table = + (struct timestamp_table *)(void *) + ((uintptr_t)_transfer_buffer + info->timestamp_offset); + /* new base_timestamp will be offset for all PSP timestamps. */ + base_timestamp -= psp_last_ts; + + for (i = 0; i < psp_ts_table->num_entries; i++) { + struct timestamp_entry *tse = &psp_ts_table->entries[i]; + /* + * We ignore the time between x86 processor release and bootblock. + * Since timestamp_add subtracts base_time, we first add old base_time + * to make it absolute then add base_timestamp again since + * it'll be a new base_time. + * + * We don't need to convert unit since both PSP and coreboot + * will use 1us granularity. + * + */ + tse->entry_stamp += psp_ts_table->base_time + base_timestamp; + } + + bootblock_main_with_timestamp(base_timestamp, psp_ts_table->entries, + psp_ts_table->num_entries); +} + asmlinkage void bootblock_c_entry(uint64_t base_timestamp) { set_caching(); write_resume_eip(); enable_pci_mmconf();
+ /* + * base_timestamp is raw tsc value. We need to divide by tsc_freq_mhz + * when we use micro-seconds granularity for Zork + */ + base_timestamp /= tsc_freq_mhz(); + + if (CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)) + boot_with_psp_timestamp(base_timestamp); + + /* + * if VBOOT_STARTS_BEFORE_BOOTBLOCK is not selected or + * previous step did nothing, proceed with normal bootblock main. + */ bootblock_main_with_basetime(base_timestamp); }
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Eric Peers (Code Review)
-
Kangheui Won (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)
-
Rob Barnes (Code Review)