Bug 240475 - [PATCH]: Add support for CPUID 0x16 in tsc_freq_cpuid() (TSC clock)
Summary: [PATCH]: Add support for CPUID 0x16 in tsc_freq_cpuid() (TSC clock)
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-bugs mailing list
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2019-09-10 15:01 UTC by Neel Chauhan
Modified: 2019-09-25 13:53 UTC (History)
2 users (show)

See Also:


Attachments
Patch (Revision 1) (614 bytes, patch)
2019-09-10 15:01 UTC, Neel Chauhan
no flags Details | Diff
dmesg log on the Spectre x360 without any patches (LiveUSB) (71.35 KB, text/plain)
2019-09-12 17:42 UTC, Neel Chauhan
no flags Details
dmesg log on the Spectre x360 with the Bug 240475 patch (Main System) (91.06 KB, text/plain)
2019-09-12 17:43 UTC, Neel Chauhan
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Neel Chauhan 2019-09-10 15:01:56 UTC
Created attachment 207346 [details]
Patch (Revision 1)

Keep in mind that I am new to the kernel.

Description:

If frequency for CPUID 0x15 is zero, fall back to CPUID 0x16 for the TSC clock if available. This is needed on some Intel SOCs like Skylake and Kaby Lake whcih report 0 for the crystal clock..

Inspired by Linux commit 604dc9170f2435d27da5039a3efd757dceadc684.

Tested on the HP Spectre x360 13-ap0043dx running 13-CURRENT with an accurate clock using this patch (without the patch, the ticks are slower).
Comment 1 Neel Chauhan 2019-09-10 18:33:48 UTC
Fixed description:

If frequency for CPUID 0x15 is zero, fall back to CPUID 0x16 for the TSC clock if available. This is needed on some Intel SOCs like Skylake and Kaby Lake which report 0 for the crystal clock.
Comment 2 Konstantin Belousov freebsd_committer 2019-09-12 15:30:44 UTC
(In reply to Neel Chauhan from comment #1)
It is wrong to use cpuid leaf 0x16 for frequency base, this is explicitly stated in Intel SDM.  Also, I am even more curious about your machine config.

Please post verbose dmesg, as requested.
Comment 3 Neel Chauhan 2019-09-12 17:41:59 UTC
This patch is based upon an existing Linux commit: https://github.com/torvalds/linux/commit/604dc9170f2435d27da5039a3efd757dceadc684

In the commit message:

    Skylake, Kabylake and all variants of those two chipsets report a
    crystal frequency of zero, however we can calculate the crystal clock
    speed by condidering data from CPUID.0x16.

    This method correctly distinguishes between the two crystal clock
    frequencies present on different Skylake X variants that caused
    headaches before.

    As the calculations do not quite match the previously-hardcoded values
    in some cases (e.g. 23913043Hz instead of 24MHz), TSC refinement is
    enabled on all platforms where we had to calculate the crystal
    frequency in this way.

This is also true on my system.

I will post two verbose dmesgs: one without any patches, and one with this patch (which also has drm-kmod and Vladimir Kondratyev's iichid loaded).
Comment 4 Neel Chauhan 2019-09-12 17:42:42 UTC
Created attachment 207426 [details]
dmesg log on the Spectre x360 without any patches (LiveUSB)
Comment 5 Neel Chauhan 2019-09-12 17:43:15 UTC
Created attachment 207427 [details]
dmesg log on the Spectre x360 with the Bug 240475 patch (Main System)
Comment 6 Konstantin Belousov freebsd_committer 2019-09-12 19:43:17 UTC
Your patch is still not equivalent to what Linux did.  Note that they still apply the nom/denom coefficient reported from leaf 0x15, but take adjusted mhz from 0x16, instead of zero hz from 0x15.  In other words, they do not hardcode 100000.
Comment 7 Neel Chauhan 2019-09-12 23:38:53 UTC
It's true that Linux does this (lines 660-661):

    crystal_khz = eax_base_mhz * 1000 *
        eax_denominator / ebx_numerator;

But in the same function, Linux returns this (line 674):

    return crystal_khz * ebx_numerator / eax_denominator;

So the numerator and denominator in crystal_khz effectively cancels out on CPUID 0x16.

From what I understand, Linux expects the value in kilohertz, so a multiplier of 1000 works on Linux. I believe FreeBSD expects hertz, so a multiplier of 1000000 is needed.

For unit conversion of megahertz to hertz, 1000 Hz = 1 KHz and 1000 KHz = 1 MHz, so 1 MHz = 1000 KHz * 1000 Hz.

Also, using the numerator and denominator wouldn't work (I've tried that, I got a kernel panic).
Comment 8 Konstantin Belousov freebsd_committer 2019-09-24 10:23:39 UTC
https://reviews.freebsd.org/D21777
Comment 9 commit-hook freebsd_committer 2019-09-25 13:37:19 UTC
A commit references this bug:

Author: kib
Date: Wed Sep 25 13:36:56 UTC 2019
New revision: 352684
URL: https://svnweb.freebsd.org/changeset/base/352684

Log:
  x86: Fall back to leaf 0x16 if TSC frequency is obtained by CPUID and
  leaf 0x15 is not functional.

  This should improve automatic TSC frequency determination on
  Skylake/Kabylake/... families, where 0x15 exists but does not provide
  all necessary information.  SDM contains relatively strong wording
  against such uses of 0x16, but Intel does not give us any other way to
  obtain the frequency. Linux did the same in the commit
  604dc9170f2435d27da5039a3efd757dceadc684.

  Based on submission by:	Neel Chauhan <neel@neelc.org>
  PR:	240475
  Reviewed by:	markj
  Sponsored by:	The FreeBSD Foundation
  MFC after:	1 week
  Differential revision:	https://reviews.freebsd.org/D21777

Changes:
  head/sys/x86/x86/tsc.c
Comment 10 Neel Chauhan 2019-09-25 13:53:35 UTC
Thanks for merging this into CURRENT!