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@

diff --git a/sys/arch/amd64/amd64/cpu.c b/sys/arch/amd64/amd64/cpu.c
index 4d0d906..8ebd254 100644 (file)
--- 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 276098f..66cbde7 100644 (file)
--- 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 <reyk@openbsd.org>
@@ -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);
        }
 }