Mathew King has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36664 )
Change subject: x86/tsc: Only call tsc_freq_mhz once to init clock ......................................................................
x86/tsc: Only call tsc_freq_mhz once to init clock
This change partially reverts https://review.coreboot.org/c/coreboot/+/33928 by only calling tsc_freq_mhz on timer init and storing the result.
The change caused an error on Drallion where coreboot could not communicate with the TPM on a cold boot causing the system to not boot.
BUG=b:144002424 TEST=Repeatedly cold boot drallion without TPM errors
Change-Id: I8e9edafa3007568e8d27e3c19c9fd6fa7637786c Signed-off-by: Mathew King mathewk@chromium.org --- M src/cpu/x86/tsc/delay_tsc.c 1 file changed, 12 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36664/1
diff --git a/src/cpu/x86/tsc/delay_tsc.c b/src/cpu/x86/tsc/delay_tsc.c index 7aa887a..dcec4fb 100644 --- a/src/cpu/x86/tsc/delay_tsc.c +++ b/src/cpu/x86/tsc/delay_tsc.c @@ -18,9 +18,18 @@ #include <delay.h> #include <thread.h>
+static unsigned long clocks_per_usec CAR_GLOBAL; + void init_timer(void) { - (void)tsc_freq_mhz(); + if (!car_get_var(clocks_per_usec)) + car_set_var(clocks_per_usec, tsc_freq_mhz()); +} + +static inline unsigned long get_clocks_per_usec(void) +{ + init_timer(); + return car_get_var(clocks_per_usec); }
void udelay(unsigned int us) @@ -34,7 +43,7 @@
start = rdtscll(); clocks = us; - clocks *= tsc_freq_mhz(); + clocks *= get_clocks_per_usec(); current = rdtscll(); while ((current - start) < clocks) { cpu_relax(); @@ -72,7 +81,7 @@
current_tick = rdtscll(); ticks_elapsed = current_tick - mono_counter->last_value; - ticks_per_usec = tsc_freq_mhz(); + ticks_per_usec = get_clocks_per_usec();
/* Update current time and tick values only if a full tick occurred. */ if (ticks_elapsed >= ticks_per_usec) {
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36664 )
Change subject: x86/tsc: Only call tsc_freq_mhz once to init clock ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36664/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36664/1//COMMIT_MSG@14 PS1, Line 14: communicate with the TPM on a cold boot causing the system to not boot. Could you please elaborate on why this was the case? I'm not understanding the full reason.
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36664 )
Change subject: x86/tsc: Only call tsc_freq_mhz once to init clock ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36664/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36664/1//COMMIT_MSG@14 PS1, Line 14: communicate with the TPM on a cold boot causing the system to not boot.
Could you please elaborate on why this was the case? I'm not understanding the full reason.
I was able verify the culprit cl and the only change in logic was not storing the result of tsc_freq_mhz. I looked at soc/intel/common/block/timer/timer.c and I did not see anything obvious to why this change broke us, but I verified by both rolling back the cl and this cl that we are no longer broken.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36664 )
Change subject: x86/tsc: Only call tsc_freq_mhz once to init clock ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36664/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36664/1//COMMIT_MSG@14 PS1, Line 14: communicate with the TPM on a cold boot causing the system to not boot.
I was able verify the culprit cl and the only change in logic was not storing the result of tsc_freq […]
I think we should assume there are callers outside this tsc/delay_tsc.c that would benefit of having tsc_freq_mhz() result cached in .bss. I kind of like cpu/intel/common/fsb.c:get_fsb_tsc() implementation, but that's probably because I wrote it. I would rather see all soc/intel/x/tsc_freq_mhz.c combined the same way.
At the moment I can think of two failure modes: Source having some invalid register clobbers in cpuid() inline assembly as those are called from intel/common/block/timer/timer.c, or cpuid synchronizing instruction side-effects.
Last, IMHO it should be forbidden for tsc_freq_mhz() to ever return 0.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36664 )
Change subject: x86/tsc: Only call tsc_freq_mhz once to init clock ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36664/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36664/1//COMMIT_MSG@14 PS1, Line 14: communicate with the TPM on a cold boot causing the system to not boot.
I think we should assume there are callers outside this tsc/delay_tsc. […]
I'm not disputing that the bisection yielded this result. I wanted to understand the reason for the issue.
I'm not sold on the clobber registers, but we could, for some reason, be returning 0 from get_freq_from_cpuid16h() intermittently. It would be good to prove out that theory to understand root of the real problem.
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36664 )
Change subject: x86/tsc: Only call tsc_freq_mhz once to init clock ......................................................................
Patch Set 1:
(1 comment)
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36664/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36664/1//COMMIT_MSG@14 PS1, Line 14: communicate with the TPM on a cold boot causing the system to not boot.
I'm not disputing that the bisection yielded this result. […]
I will see if I can identify the root cause.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36664 )
Change subject: x86/tsc: Only call tsc_freq_mhz once to init clock ......................................................................
Patch Set 1:
tsc_freq_mhz() function can only return 0 in below cases
1. CPUID 15h not supported 2. CPUID 16h not supported 3. CPUID 15h is returning 0 in core_crystal_nominal_freq_khz
if that would have the case then why its only happening 2nd cold boot and not always ?
Mathew King has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36664 )
Change subject: x86/tsc: Only call tsc_freq_mhz once to init clock ......................................................................
Abandoned
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36664 )
Change subject: x86/tsc: Only call tsc_freq_mhz once to init clock ......................................................................
Patch Set 1:
I need to see the arguments why this was abandoned. Or in other words, where was the fix? Are we hitting some CPU errata or undisclosed silicon bug?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36664 )
Change subject: x86/tsc: Only call tsc_freq_mhz once to init clock ......................................................................
Patch Set 1:
Patch Set 1:
I need to see the arguments why this was abandoned. Or in other words, where was the fix? Are we hitting some CPU errata or undisclosed silicon bug?
The failure was occurring after FSP-S requested a reset. Inside of FSP it was applying power management configuration. Prior to the mainboard policy being deployed the reset would happen. On that subsequent boot we were missing h1 interrupts because of the of power management policy. I cannot claim that I understand why this CL would help in such scenarios, though.
https://review.coreboot.org/c/coreboot/+/37319 was the solution people came up with for ensuring the mainboard power management policy was provisioned.
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)
-
Mathew King (Code Review)
-
Subrata Banik (Code Review)