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.
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.
>
> 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 03:49:31 -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,20 +934,7 @@ 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);
>
> if ((ci->ci_flags & CPUF_IDENTIFIED) == 0) {
> /*
> @@ -960,6 +951,19 @@ 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();
> + cpu_ucode_apply(ci);
> + cpu_tsx_disable(ci);
>
> while ((ci->ci_flags & CPUF_GO) == 0)
> delay(10);
>
>
