On Tue, Mar 29, 2022 at 03:26:49PM +1100, Jonathan Gray wrote:
> On Mon, Mar 28, 2022 at 10:52:09PM -0500, Scott Cheloha wrote:
> > I want to use the IA32_TSC_ADJUST MSR where available when testing TSC
> > synchronization. We note if it's available during CPU identification.
> >
> > Can we do CPU identification earlier in cpu_hatch() and
> > cpu_start_secondary(), before we do the TSC sync testing?
> >
> > This can wait until after release. I'm just trying to suss out
> > whether there is an order dependency I'm not seeing. My laptop
> > appears to boot and resume no differently with this patch.
> >
> > Thoughts?
>
> The rest aside, moving the cpu_ucode_apply() call to after the
> identifycpu() call is wrong as microcode can add cpuid bits.
> I would keep cpu_tsx_disable() before it as well.
Okay, moved them up.
> I'm sure I've had problems trying to change the sequencing
> of lapic, tsc freq and identify in the past. It caused problems
> only on certain machines.
Uh, identifycpu() calls tsc_identify() calls tsc_freq_cpuid(), which
quietly sets lapic_per_second (a variable in amd64/lapic.c) based on
the core crystal frequency reported via CPUID.
If lapic_per_second is non-zero when lapic_calibrate_timer() is
called, the BP skips manual calibration of the LAPIC counter with
the i8254.
It might be less surprising if lapic_calibrate_timer() grabbed the
core crystal frequency via a function in amd64/tsc.c. Then the order
of TSC-sync-test and LAPIC setup wouldn't matter. We disable
interrupts when doing TSC sync anyway, so even if the LAPIC were
running we wouldn't see the interrupts.
...
But maybe you're talking about some other issue :)
Index: cpu.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/cpu.c,v
retrieving revision 1.155
diff -u -p -r1.155 cpu.c
--- cpu.c 21 Feb 2022 11:03:39 -0000 1.155
+++ cpu.c 29 Mar 2022 15:19:54 -0000
@@ -842,20 +842,7 @@ cpu_start_secondary(struct cpu_info *ci)
printf("dropping into debugger; continue from here to resume
boot\n");
db_enter();
#endif
- } else {
- /*
- * Synchronize time stamp counters. Invalidate cache and
- * synchronize twice (in tsc_sync_bp) to minimize possible
- * cache effects. Disable interrupts to try and rule out any
- * external interference.
- */
- s = intr_disable();
- wbinvd();
- tsc_sync_bp(ci);
- intr_restore(s);
-#ifdef TSC_DEBUG
- printf("TSC skew=%lld\n", (long long)ci->ci_tsc_skew);
-#endif
+ goto cleanup;
}
if ((ci->ci_flags & CPUF_IDENTIFIED) == 0) {
@@ -865,11 +852,28 @@ cpu_start_secondary(struct cpu_info *ci)
for (i = 2000000; (ci->ci_flags & CPUF_IDENTIFY) && i > 0; i--)
delay(10);
- if (ci->ci_flags & CPUF_IDENTIFY)
+ if (ci->ci_flags & CPUF_IDENTIFY) {
printf("%s: failed to identify\n",
ci->ci_dev->dv_xname);
+ goto cleanup;
+ }
}
+ /*
+ * Synchronize time stamp counters. Invalidate cache and
+ * synchronize twice (in tsc_sync_bp) to minimize possible
+ * cache effects. Disable interrupts to try and rule out any
+ * external interference.
+ */
+ s = intr_disable();
+ wbinvd();
+ tsc_sync_bp(ci);
+ intr_restore(s);
+#ifdef TSC_DEBUG
+ printf("TSC skew=%lld\n", (long long)ci->ci_tsc_skew);
+#endif
+
+cleanup:
CPU_START_CLEANUP(ci);
pmap_kremove(MP_TRAMPOLINE, PAGE_SIZE);
@@ -930,18 +934,8 @@ cpu_hatch(void *v)
if (ci->ci_flags & CPUF_PRESENT)
panic("%s: already running!?", ci->ci_dev->dv_xname);
#endif
-
- /*
- * Synchronize the TSC for the first time. Note that interrupts are
- * off at this point.
- */
- wbinvd();
ci->ci_flags |= CPUF_PRESENT;
- ci->ci_tsc_skew = 0; /* reset on resume */
- tsc_sync_ap(ci);
- lapic_enable();
- lapic_startclock();
cpu_ucode_apply(ci);
cpu_tsx_disable(ci);
@@ -960,6 +954,17 @@ cpu_hatch(void *v)
/* Prevent identifycpu() from running again */
atomic_setbits_int(&ci->ci_flags, CPUF_IDENTIFIED);
}
+
+ /*
+ * Synchronize the TSC for the first time. Note that interrupts are
+ * off at this point.
+ */
+ wbinvd();
+ ci->ci_tsc_skew = 0; /* reset on resume */
+ tsc_sync_ap(ci);
+
+ lapic_enable();
+ lapic_startclock();
while ((ci->ci_flags & CPUF_GO) == 0)
delay(10);
