Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190422101526.28415-1-drake@endlessm.com> <20190423090849.GH11158@hirez.programming.kicks-ass.net>
In-Reply-To: <20190423090849.GH11158@hirez.programming.kicks-ass.net>
From:   Daniel Drake <drake@endlessm.com>
Date:   Wed, 24 Apr 2019 13:53:02 +0800
Message-ID: <CAD8Lp45C2EsEpxtpdNp6afPvqGOSU1yW4=AB2fi0Rhk3fxZ89Q@mail.gmail.com>
Subject: Re: [PATCH 1/2] x86/tsc: use CPUID.0x16 to calculate missing crystal frequency
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        hpa@zytor.com, x86@kernel.org,
        Linux Kernel <linux-kernel@vger.kernel.org>,
        Len Brown <len.brown@intel.com>,
        "Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
        Linux Upstreaming Team <linux@endlessm.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Apr 23, 2019 at 5:08 PM Peter Zijlstra <peterz@infradead.org> wrote:
> Well, SKX isn't exactly 'missing'; it would be very good if we can
> confirm the new code is still correct under the below mentioned
> conditions.
>
> commit b511203093489eb1829cb4de86e8214752205ac6
> Author: Len Brown <len.brown@intel.com>
> Date:   Fri Dec 22 00:27:55 2017 -0500
>
>     x86/tsc: Fix erroneous TSC rate on Skylake Xeon
>
>     The INTEL_FAM6_SKYLAKE_X hardcoded crystal_khz value of 25MHZ is
>     problematic:
>
>      - SKX workstations (with same model # as server variants) use a 24 MHz
>        crystal.  This results in a -4.0% time drift rate on SKX workstations.

Checking http://instlatx64.atw.hu/ there are 11 platforms listed as
00050654 (skylake X), doing the calculations manually:
18-Core Intel Xeon Gold 6154, 3000 MHz: 25000000
2x 16-Core Intel Xeon Gold 6130, 2100 MHz: 25000000
2x 18-Core Intel Xeon Gold 6154, 3000 MHz: 25000000
2x 28-Core Intel Xeon Platinum 8180, 2500 MHz: 25000000
2x HexaCore Intel Xeon Bronze 3106: 25000000
2x OctalCore Intel Xeon Silver 4108, 3000 MHz: 25000000
10-Core Xeon W-2155: 23913043
HexaCore Intel Core i7-7800X: 23972602
10-Core Intel Core i9-7900X, 4000 MHz: 23913043
18-Core Intel Core i9-7980XE: 24074074
28-Core Intel Xeon W-3175X: 25000000

So given that the results include 24MHz and 25MHz crystal clocks
calculated on different products then it looks like this variance is
correctly accounted for.

>      - SKX servers subject the crystal to an EMI reduction circuit that reduces its
>        actual frequency by (approximately) -0.25%.  This results in -1 second per
>        10 minute time drift as compared to network time.
>
>     This issue can also trigger a timer and power problem, on configurations
>     that use the LAPIC timer (versus the TSC deadline timer).  Clock ticks
>     scheduled with the LAPIC timer arrive a few usec before the time they are
>     expected (according to the slow TSC).  This causes Linux to poll-idle, when
>     it should be in an idle power saving state.  The idle and clock code do not
>     graciously recover from this error, sometimes resulting in significant
>     polling and measurable power impact.
>
>     Stop using native_calibrate_tsc() for INTEL_FAM6_SKYLAKE_X.
>     native_calibrate_tsc() will return 0, boot will run with tsc_khz = cpu_khz,
>     and the TSC refined calibration will update tsc_khz to correct for the
>     difference.

It sounds like I should add a condition:
    if (boot_cpu_data.x86_model != INTEL_FAM6_SKYLAKE_X)
        setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
so that refined TSC calibration will run on SKYLAKE_X.

That does raise another question though. Taking other platforms such
as KABYLAKE_MOBILE, the previous code hardcoded a precise 24MHz value,
but my new code calculates 23893805Hz. Is that small discrepancy small
enough to trigger the timer and power problem as described for
SKYLAKE_X above?

If so, maybe the logic should instead be:
    if (CPUID.0x15 provided Crystal Hz, i.e. we didn't have to calculate it)
        setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
This would enable TSC refinement on all variants of skylake and kabylake.

Daniel