On Tue, Mar 29, 2022 at 10:24:03AM -0500, Scott Cheloha wrote:
> 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.
>
> [...]
6 week bump + rebase.
Once again, I want to do CPU identification before the TSC sync test
so we can check for and use the IA32_TSC_ADJUST MSR during the sync
test.
Does anyone understand amd64 CPU startup well enough to say whether
this rearrangement is going to break something?
Is this ok?
Index: cpu.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/cpu.c,v
retrieving revision 1.156
diff -u -p -r1.156 cpu.c
--- cpu.c 26 Apr 2022 08:35:30 -0000 1.156
+++ cpu.c 11 May 2022 02:00:37 -0000
@@ -852,20 +852,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) {
@@ -875,11 +862,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);
@@ -940,18 +944,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);
@@ -970,6 +964,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);
