From 9d90b8730bb61c7870c9d3c700d64cfa1f0854cd Mon Sep 17 00:00:00 2001 From: Scott Soule Cheloha Date: Thu, 15 Sep 2022 19:30:52 +0000 Subject: tsc: configure LFENCE to serialize dispatch before testing TSC sync On AMD CPUs, LFENCE does not serialize instruction dispatch until MSR C001_1029[1] is properly configured. We do this in identifycpu(); see amd64/identcpu.c,v 1.103. The upshot is that the first TSC synchronization test is currently invalid on most AMD CPUs because the LFENCE in the test loop does not ensure that the AP loads the BP's latest TSC value before executing RDTSC. So the synchronization test is yielding false positives on AMD CPUs where the TSCs are actually synchronized. The simplest fix is to wait until after the secondary CPU runs identifycpu() in cpu_hatch() to test TSC synchronization. Moving the TSC sync test after CPU identification means that we can remove the CPUID() calls from tsc.c: the CPU feature flags are set in identifycpu() so we no longer need to test for IA32_TSC_ADJUST support by hand. While we are at it, we should also pass the correct cpu_info pointer to tsc_test_sync_bp(). It was unused before, so the bug was harmless, but we definitely need the BP's cpu_info pointer, not the AP's pointer. Unfortunately, this change does not fix the TSC sync problems we've been seeing on e.g. dv@'s and jmc@'s Ryzen 5 machines. Hopefully the problem on those machines is buggy firmware and not another architectural misunderstanding on my part. Prompted by robert@. Problem diagnosed by brynet@. With input from robert@, brynet@, and deraadt@. Tested by robert@, brynet@, dv@, phessler@, and jmc@. ok robert@ brynet@ sthen@ --- sys/arch/amd64/amd64/cpu.c | 40 +++++++++++++++++++++------------------- sys/arch/amd64/amd64/tsc.c | 28 +++++++++------------------- 2 files changed, 30 insertions(+), 38 deletions(-) (limited to 'sys') diff --git a/sys/arch/amd64/amd64/cpu.c b/sys/arch/amd64/amd64/cpu.c index 4d0d9062980..8ebd254786f 100644 --- a/sys/arch/amd64/amd64/cpu.c +++ b/sys/arch/amd64/amd64/cpu.c @@ -1,4 +1,4 @@ -/* $OpenBSD: cpu.c,v 1.158 2022/08/12 02:20:36 cheloha Exp $ */ +/* $OpenBSD: cpu.c,v 1.159 2022/09/15 19:30:51 cheloha Exp $ */ /* $NetBSD: cpu.c,v 1.1 2003/04/26 18:39:26 fvdl Exp $ */ /*- @@ -854,16 +854,6 @@ cpu_start_secondary(struct cpu_info *ci) printf("dropping into debugger; continue from here to resume boot\n"); db_enter(); #endif - } else { - /* - * Test if TSCs are synchronized. Invalidate cache to - * minimize possible cache effects. Disable interrupts to - * try to rule out external interference. - */ - s = intr_disable(); - wbinvd(); - tsc_test_sync_bp(ci); - intr_restore(s); } if ((ci->ci_flags & CPUF_IDENTIFIED) == 0) { @@ -878,6 +868,18 @@ cpu_start_secondary(struct cpu_info *ci) ci->ci_dev->dv_xname); } + if (ci->ci_flags & CPUF_IDENTIFIED) { + /* + * Test if TSCs are synchronized. Invalidate cache to + * minimize possible cache effects. Disable interrupts to + * try to rule out external interference. + */ + s = intr_disable(); + wbinvd(); + tsc_test_sync_bp(curcpu()); + intr_restore(s); + } + CPU_START_CLEANUP(ci); pmap_kremove(MP_TRAMPOLINE, PAGE_SIZE); @@ -905,7 +907,7 @@ cpu_boot_secondary(struct cpu_info *ci) /* Test if TSCs are synchronized again. */ s = intr_disable(); wbinvd(); - tsc_test_sync_bp(ci); + tsc_test_sync_bp(curcpu()); intr_restore(s); } } @@ -930,14 +932,7 @@ cpu_hatch(void *v) if (ci->ci_flags & CPUF_PRESENT) panic("%s: already running!?", ci->ci_dev->dv_xname); #endif - - /* - * Test if our TSC is synchronized for the first time. - * Note that interrupts are off at this point. - */ - wbinvd(); atomic_setbits_int(&ci->ci_flags, CPUF_PRESENT); - tsc_test_sync_ap(ci); lapic_enable(); lapic_startclock(); @@ -960,6 +955,13 @@ cpu_hatch(void *v) atomic_setbits_int(&ci->ci_flags, CPUF_IDENTIFIED); } + /* + * Test if our TSC is synchronized for the first time. + * Note that interrupts are off at this point. + */ + wbinvd(); + tsc_test_sync_ap(ci); + while ((ci->ci_flags & CPUF_GO) == 0) delay(10); #ifdef HIBERNATE diff --git a/sys/arch/amd64/amd64/tsc.c b/sys/arch/amd64/amd64/tsc.c index 276098f05bf..66cbde708e2 100644 --- a/sys/arch/amd64/amd64/tsc.c +++ b/sys/arch/amd64/amd64/tsc.c @@ -1,4 +1,4 @@ -/* $OpenBSD: tsc.c,v 1.26 2022/08/25 17:38:16 cheloha Exp $ */ +/* $OpenBSD: tsc.c,v 1.27 2022/09/15 19:30:51 cheloha Exp $ */ /* * Copyright (c) 2008 The NetBSD Foundation, Inc. * Copyright (c) 2016,2017 Reyk Floeter @@ -301,8 +301,8 @@ volatile u_int tsc_ingress_barrier; /* [a] Test start barrier */ volatile u_int tsc_test_rounds; /* [p] Remaining test rounds */ int tsc_is_synchronized = 1; /* [p] Have we ever failed the test? */ +void tsc_adjust_reset(struct cpu_info *, struct tsc_test_status *); void tsc_report_test_results(void); -void tsc_reset_adjust(struct tsc_test_status *); void tsc_test_ap(void); void tsc_test_bp(void); @@ -317,7 +317,7 @@ tsc_test_sync_bp(struct cpu_info *ci) return; #endif /* Reset IA32_TSC_ADJUST if it exists. */ - tsc_reset_adjust(&tsc_bp_status); + tsc_adjust_reset(ci, &tsc_bp_status); /* Reset the test cycle limit and round count. */ tsc_test_cycles = TSC_TEST_MSECS * tsc_frequency / 1000; @@ -384,7 +384,7 @@ tsc_test_sync_ap(struct cpu_info *ci) __func__, ci->ci_dev->dv_xname, tsc_ap_name); } - tsc_reset_adjust(&tsc_ap_status); + tsc_adjust_reset(ci, &tsc_ap_status); /* * The AP is only responsible for running the test and @@ -433,24 +433,14 @@ tsc_report_test_results(void) /* * Reset IA32_TSC_ADJUST if we have it. - * - * XXX We should rearrange cpu_hatch() so that the feature - * flags are already set before we get here. Check CPUID - * by hand until then. */ void -tsc_reset_adjust(struct tsc_test_status *tts) +tsc_adjust_reset(struct cpu_info *ci, struct tsc_test_status *tts) { - uint32_t eax, ebx, ecx, edx; - - CPUID(0, eax, ebx, ecx, edx); - if (eax >= 7) { - CPUID_LEAF(7, 0, eax, ebx, ecx, edx); - if (ISSET(ebx, SEFF0EBX_TSC_ADJUST)) { - tts->adj = rdmsr(MSR_TSC_ADJUST); - if (tts->adj != 0) - wrmsr(MSR_TSC_ADJUST, 0); - } + if (ISSET(ci->ci_feature_sefflags_ebx, SEFF0EBX_TSC_ADJUST)) { + tts->adj = rdmsr(MSR_TSC_ADJUST); + if (tts->adj != 0) + wrmsr(MSR_TSC_ADJUST, 0); } } -- cgit v1.2.3