|Summary:||[PATCH]: Add support for CPUID 0x16 in tsc_freq_cpuid() (TSC clock)|
|Product:||Base System||Reporter:||Neel Chauhan <neel>|
|Component:||kern||Assignee:||freebsd-bugs mailing list <bugs>|
|Severity:||Affects Some People||CC:||emaste, kib, neel|
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 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 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 9 commit-hook 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 <firstname.lastname@example.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!
Comment 11 Ed Maste 2019-12-10 21:19:15 UTC
Merged to stable/12 in r353006