On Sun, Dec 27, 2020 at 04:34:32AM +0000, James Cook wrote:
> On Thu, Dec 24, 2020 at 03:34:33PM +1100, Jonathan Gray wrote:
> > On Thu, Dec 24, 2020 at 12:42:23AM +0000, James Cook wrote:
> > > On Wed, Dec 23, 2020 at 08:43:10PM +0000, James Cook wrote:
> > > > On Wed, Dec 23, 2020 at 11:47:05PM +1100, Jonathan Gray wrote:
> > > > > On Wed, Dec 23, 2020 at 12:31:10PM +1100, Jonathan Gray wrote:
> > > > > > On Tue, Dec 22, 2020 at 06:30:48PM +0000, James Cook wrote:
> > > > > > > > + case 0xa6: /* Coffeelake mobile */
> > > > > > >
> > > > > > > The laptop's CPU is an i7-10710U, which I think is in the Comet
> > > > > > > Lake
> > > > > > > series, not Coffee Lake.
> > > > > >
> > > > > > Yes 0xa6 is comet lake.
> > > > > >
> > > > > > But we should really do what FreeBSD and Linux do and fallback to
> > > > > > cpuid 0x16 as Intel keeps creating new skylake variants.
> > > > > >
> > > > > > The frequency from cpuid 0x15 is Hz, from 0x16 it is MHz.
> > > > > >
> > > > > > Untested as I don't have any >= skylake machines.
> > > > > > If you can add a printf to check the value is sane that would
> > > > > > be helpful.
> > > > >
> > > > > As noticed by tb@ the last diff wasn't quite right:
> > > > >
> > > > > Index: sys/arch/amd64/amd64/tsc.c
> > > > > ===================================================================
> > > > > RCS file: /cvs/src/sys/arch/amd64/amd64/tsc.c,v
> > > > > retrieving revision 1.21
> > > > > diff -u -p -r1.21 tsc.c
> > > > > --- sys/arch/amd64/amd64/tsc.c 6 Sep 2020 20:50:00 -0000
> > > > > 1.21
> > > > > +++ sys/arch/amd64/amd64/tsc.c 23 Dec 2020 12:25:32 -0000
> > > > > @@ -66,14 +66,16 @@ tsc_freq_cpuid(struct cpu_info *ci)
> > > > > eax = ebx = khz = dummy = 0;
> > > > > CPUID(0x15, eax, ebx, khz, dummy);
> > > > > khz /= 1000;
> > > > > - if (khz == 0) {
> > > > > + /*
> > > > > + * Fallback to 'Processor Base Frequency' from cpuid
> > > > > 0x16 when
> > > > > + * 'nominal frequency of the core crystal clock' from
> > > > > cpuid 0x15
> > > > > + * is 0 on >= Skylake
> > > > > + */
> > > > > + if (khz == 0 && cpuid_level >= 0x16) {
> > > > > + CPUID(0x16, khz, dummy, dummy, dummy);
> > > > > + khz = khz * 1000 * eax / ebx;
> > > > > + } else if (khz == 0) {
> > > > > switch (ci->ci_model) {
> > > > > - case 0x4e: /* Skylake mobile */
> > > > > - case 0x5e: /* Skylake desktop */
> > > > > - case 0x8e: /* Kabylake mobile */
> > > > > - case 0x9e: /* Kabylake desktop */
> > > > > - khz = 24000; /* 24.0 MHz */
> > > > > - break;
> > > > > case 0x5f: /* Atom Denverton */
> > > > > khz = 25000; /* 25.0 MHz */
> > > > > break;
> > > >
> > > > The patch works (I tested bsd.rd; sleep and date both behave right).
> > > >
> > > > Based on added printfs, it ends up with a khz of 23880, computed as
> > > > 1600 * 1000 * 2 / 134.
> > >
> > > I noticed something strange about the hw.setperf and hw.cpuspeed
> > > sysctls. I don't know if they're related to the original bug or its
> > > fix.
> > >
> > > My hw.cpuspeed sysctl starts out at 16264, which seems way too high.
> > > This page
> > >
> > >
> > > https://ark.intel.com/content/www/us/en/ark/products/196448/intel-core-i7-10710u-processor-12m-cache-up-to-4-70-ghz.html
> > >
> > > claims a "Max Turbo Frequency" of 4.70 GHz.
> > >
> > > hw.setperf seems to start out at 1320, as indicated by sysctl output
> > > when I change it. Of course, if I lower it, I can't bring it back to
> > > 1320. If I set it to 100, hw.cpuspeed is a more plausible 1601.
> >
> > The max + 1 state is the turbo state.
> >
> > As for why cpuspeed is so high, have a look at
> > sys/arch/amd64/amd64/identcpu.c cpu_freq() can you confirm if
> > cpu_freq_ctr() returns a non zero value? If so there is something wrong
> > with the performance counter method of getting cpuspeed.
>
> I added a printf (below) in cpu_freq; cpu_freq_ctr is indeed returning
> non-zero (so my second printf is not reached). It gets called a few
> times; the first time it's 16263595810 and the other times it's
> approximately 3890938820 (it varies a bit).
>
> I didn't understand your comment about the turbo state and max+1... I'm
> seeing an initial value of 1320 for hw.setperf and I thought max for it
> is 100.
from your earlier bsd.mp dmesg:
cpu0: Enhanced SpeedStep 16268 MHz: speeds: 1601, 1600, 1500, 1400, 1300, 1200,
1100, 1000, 900, 800, 700, 600, 500, 400 MHz
1601 is variable/turbo speed the others are fixed. When running in
turbo mode getting the current frequency involves msrs which
hw.cpuspeed doesn't do. The initial 'x MHz:' is from cpuspeed not from
the acpi table.
On modern machines these values are from acpicpu(4)/acpi _PSS.
Can you show the output of running with the following diff to dump the
performance counter control values?
MSR_PERF_FIXED_CTR_CTRL 0x38d
MSR_PERF_GLOBAL_CTRL 0x38f
on a broadwell laptop this shows
cpu0 cpu_freq_ctr cpuid 0x0a eax 0x7300403 ebx 0x0 ecx 0x0 edx 0x603
cpu0 cpu_freq_ctr perf ver 3 gp ctrs 4 fixed 3
cpu0 cpu_freq_ctr MSR_PERF_FIXED_CTR_CTRL 0x0
cpu0 cpu_freq_ctr MSR_PERF_GLOBAL_CTRL 0xf
cpu1 cpu_freq_ctr cpuid 0x0a eax 0x7300403 ebx 0x0 ecx 0x0 edx 0x603
cpu1 cpu_freq_ctr perf ver 3 gp ctrs 4 fixed 3
cpu1 cpu_freq_ctr MSR_PERF_FIXED_CTR_CTRL 0x0
cpu1 cpu_freq_ctr MSR_PERF_GLOBAL_CTRL 0xf
cpu2 cpu_freq_ctr cpuid 0x0a eax 0x7300403 ebx 0x0 ecx 0x0 edx 0x603
cpu2 cpu_freq_ctr perf ver 3 gp ctrs 4 fixed 3
cpu2 cpu_freq_ctr MSR_PERF_FIXED_CTR_CTRL 0x0
cpu2 cpu_freq_ctr MSR_PERF_GLOBAL_CTRL 0xf
cpu3 cpu_freq_ctr cpuid 0x0a eax 0x7300403 ebx 0x0 ecx 0x0 edx 0x603
cpu3 cpu_freq_ctr perf ver 3 gp ctrs 4 fixed 3
cpu3 cpu_freq_ctr MSR_PERF_FIXED_CTR_CTRL 0x0
cpu3 cpu_freq_ctr MSR_PERF_GLOBAL_CTRL 0xf
Index: sys/arch/amd64/amd64/identcpu.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/identcpu.c,v
retrieving revision 1.117
diff -u -p -r1.117 identcpu.c
--- sys/arch/amd64/amd64/identcpu.c 13 Sep 2020 05:57:28 -0000 1.117
+++ sys/arch/amd64/amd64/identcpu.c 27 Dec 2020 06:26:13 -0000
@@ -415,6 +415,7 @@ uint64_t
cpu_freq_ctr(struct cpu_info *ci)
{
uint64_t count, last_count, msr;
+ uint32_t eax, ebx, ecx, edx;
if ((ci->ci_flags & CPUF_CONST_TSC) == 0 ||
(cpu_perf_eax & CPUIDEAX_VERID) <= 1 ||
@@ -422,15 +423,28 @@ cpu_freq_ctr(struct cpu_info *ci)
return (0);
msr = rdmsr(MSR_PERF_FIXED_CTR_CTRL);
- if (msr & MSR_PERF_FIXED_CTR_FC(1, MSR_PERF_FIXED_CTR_FC_MASK)) {
+ if (msr & MSR_PERF_FIXED_CTR_FC(1, MSR_PERF_FIXED_CTR_FC_1)) {
/* some hypervisor is dicking us around */
return (0);
}
+ CPUID(0x0a, eax, ebx, ecx, edx);
+ printf("%s %s cpuid 0x0a eax 0x%x ebx 0x%x ecx 0x%x edx 0x%x\n",
+ ci->ci_dev->dv_xname, __func__, eax, ebx, ecx, edx);
+ printf("%s %s perf ver %d gp ctrs %d fixed %d\n",
+ ci->ci_dev->dv_xname, __func__, eax & 0xff, (eax >> 8) & 0xff,
+ edx & 0x1f);
+
+ printf("%s %s MSR_PERF_FIXED_CTR_CTRL 0x%llx\n", ci->ci_dev->dv_xname,
+ __func__, msr);
+
msr |= MSR_PERF_FIXED_CTR_FC(1, MSR_PERF_FIXED_CTR_FC_1);
wrmsr(MSR_PERF_FIXED_CTR_CTRL, msr);
- msr = rdmsr(MSR_PERF_GLOBAL_CTRL) | MSR_PERF_GLOBAL_CTR1_EN;
+ msr = rdmsr(MSR_PERF_GLOBAL_CTRL);
+ printf("%s %s MSR_PERF_GLOBAL_CTRL 0x%llx\n", ci->ci_dev->dv_xname,
+ __func__, msr);
+ msr |= MSR_PERF_GLOBAL_CTR1_EN;
wrmsr(MSR_PERF_GLOBAL_CTRL, msr);
last_count = rdmsr(MSR_PERF_FIXED_CTR1);
