Re: amd64: do CPU identification before TSC sync test
> On May 11, 2022, at 2:51 AM, Yuichiro NAITO wrote: > > On 5/11/22 14:34, Yuichiro NAITO wrote: >> After applying your patch, cpu1 is not identified on my current kernel. >> Dmesg shows as follows. I'll see it further more. > > I found that LAPIC is necessary for `delay` function that is used in > `idendifycpu` and > waiting loop for CPUF_IDENTIFY flag is set. Both lapic_delay() and tsc_delay() are plenty fast. i8254_delay() might cause issues on a VM. I need to think a bit, but your rearrangement of the patch is a step in the right direction. I will reply later with something better. Thank you for testing!
Re: amd64: do CPU identification before TSC sync test
try next snap clematis wrote: > Hi, > I can't see any recent commits but has any of those changes been added to > recent snapshots? I was running one from 2nd of May and wasn't hitting > those cpu failed to identify errors but I do with the one from the 10th > of May. > > > cpu1 at mainbus0: apid 1 (application processor) > cpu1: failed to identify > > Full dmesg: https://dmesgd.nycbug.org/index.cgi?do=view&id=6579 > > Thanks, > -- > clematis (0xA2C87EDB507B4C53) >
Re: amd64: do CPU identification before TSC sync test
On 5/11/22 14:34, Yuichiro NAITO wrote:
After applying your patch, cpu1 is not identified on my current kernel.
Dmesg shows as follows. I'll see it further more.
I found that LAPIC is necessary for `delay` function that is used in
`idendifycpu` and
waiting loop for CPUF_IDENTIFY flag is set.
I partly reverted calling `lapic_enable` and `lapic_startclock` and it works
for me.
Please see my following patch.
diff --git a/sys/arch/amd64/amd64/cpu.c b/sys/arch/amd64/amd64/cpu.c
index 4778347a12e..83c0297e7f1 100644
--- a/sys/arch/amd64/amd64/cpu.c
+++ b/sys/arch/amd64/amd64/cpu.c
@@ -844,50 +844,54 @@ cpu_start_secondary(struct cpu_info *ci)
* wait for it to become ready
*/
for (i = 10; (!(ci->ci_flags & CPUF_PRESENT)) && i>0;i--) {
delay(10);
}
if (! (ci->ci_flags & CPUF_PRESENT)) {
printf("%s: failed to become ready\n", ci->ci_dev->dv_xname);
#if defined(MPDEBUG) && defined(DDB)
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) {
atomic_setbits_int(&ci->ci_flags, CPUF_IDENTIFY);
/* wait for it to identify */
for (i = 200; (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);
pmap_kremove(MP_TRAMP_DATA, PAGE_SIZE);
}
void
cpu_boot_secondary(struct cpu_info *ci)
{
int i;
int64_t drift;
@@ -932,53 +936,53 @@ void
cpu_hatch(void *v)
{
struct cpu_info *ci = (struct cpu_info *)v;
int s;
cpu_init_msrs(ci);
#ifdef DEBUG
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) {
/*
* We need to wait until we can identify, otherwise dmesg
* output will be messy.
*/
while ((ci->ci_flags & CPUF_IDENTIFY) == 0)
delay(10);
identifycpu(ci);
/* Signal we're done */
atomic_clearbits_int(&ci->ci_flags, CPUF_IDENTIFY);
/* 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);
+
while ((ci->ci_flags & CPUF_GO) == 0)
delay(10);
#ifdef HIBERNATE
if ((ci->ci_flags & CPUF_PARK) != 0) {
atomic_clearbits_int(&ci->ci_flags, CPUF_PARK);
hibernate_drop_to_real_mode();
}
#endif /* HIBERNATE */
#ifdef DEBUG
if (ci->ci_flags & CPUF_RUNNING)
--
Yuichiro NAITO (naito.yuich...@gmail.com)
Re: amd64: do CPU identification before TSC sync test
Hi, Scott.
After applying your patch, cpu1 is not identified on my current kernel.
Dmesg shows as follows. I'll see it further more.
OpenBSD 7.1-current (GENERIC.MP) #3: Wed May 11 12:27:53 JST 2022
yuich...@yuichiro-obsd.soum.co.jp:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 2129526784 (2030MB)
avail mem = 2047676416 (1952MB)
random: good seed from bootblocks
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.7 @ 0xe0010 (242 entries)
bios0: vendor Phoenix Technologies LTD version "6.00" date 12/12/2018
bios0: VMware, Inc. VMware Virtual Platform
acpi0 at bios0: ACPI 4.0
acpi0: sleep states S0 S1 S4 S5
acpi0: tables DSDT FACP BOOT APIC MCFG SRAT HPET WAET
acpi0: wakeup devices PCI0(S3) USB_(S1) P2P0(S3) S1F0(S3) S2F0(S3) S8F0(S3)
S16F(S3) S17F(S3) S18F(S3) S22F(S3) S23F(S3) S24F(S3) S25F(S3) PE40(S3)
S1F0(S3) PE50(S3) [...]
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: AMD Ryzen 7 5700G with Radeon Graphics, 3792.28 MHz, 19-50-00
cpu0:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,SSE3,PCLMUL,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,HV,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,RDSEED,ADX,SMAP,CLFLUSHOPT,CLWB,SHA,PKU,IBPB,XSAVEOPT,XSAVEC,XSAVES
cpu0: 32KB 64b/line 8-way I-cache, 32KB 64b/line 8-way D-cache, 512KB 64b/line
8-way L2 cache
cpu0: ITLB 64 4KB entries fully associative, 64 4MB entries fully associative
cpu0: DTLB 64 4KB entries fully associative, 64 4MB entries fully associative
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
cpu0: apic clock running at 65MHz
cpu1 at mainbus0: apid 2 (application processor)
cpu1: failed to identify
On 5/11/22 11:04, Scott Cheloha wrote:
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 - 1.156
+++ cpu.c 11 May 2022 02:00:37 -
@@ -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 = 200; (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
+* c
Re: amd64: do CPU identification before TSC sync test
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 - 1.156
+++ cpu.c 11 May 2022 02:00:37 -
@@ -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 = 200; (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);
Re: amd64: do CPU identification before TSC sync test
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 - 1.155
+++ cpu.c 29 Mar 2022 15:19:54 -
@@ -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 = 200; (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);
Re: amd64: do CPU identification before TSC sync test
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 - 1.155
> +++ cpu.c 29 Mar 2022 03:49:31 -
> @@ -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 = 200; (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);
>
>
amd64: do CPU identification before TSC sync test
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?
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 - 1.155
+++ cpu.c 29 Mar 2022 03:49:31 -
@@ -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 = 200; (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);
