linux-kernel - Re: [PATCH v2 3/3] x86/tsc_msr: Make MSR derived TSC frequency more accurate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <b6c1e57e-8001-19f1-7f37-7a5975e565d6@redhat.com>
Date:   Fri, 7 Feb 2020 09:31:56 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Andy Shevchenko <andy@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Vipul Kumar <vipulk0511@...il.com>,
        Vipul Kumar <vipul_kumar@...tor.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Srikanth Krishnakar <Srikanth_Krishnakar@...tor.com>,
        Cedric Hombourger <Cedric_Hombourger@...tor.com>,
        Len Brown <len.brown@...el.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Stable <stable@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] x86/tsc_msr: Make MSR derived TSC frequency more
 accurate

Hi All,

On 2/5/20 5:59 PM, Andy Shevchenko wrote:
> On Wed, Feb 5, 2020 at 5:34 PM Hans de Goede <hdegoede@...hat.com> wrote:
>>
>> The "Intel 64 and IA-32 Architectures Software Developer’s Manual
>> Volume 4: Model-Specific Registers" has the following table for the
>> values from freq_desc_byt:
>>
>>     000B: 083.3 MHz
>>     001B: 100.0 MHz
>>     010B: 133.3 MHz
>>     011B: 116.7 MHz
>>     100B: 080.0 MHz
>>
>> Notice how for e.g the 83.3 MHz value there are 3 significant digits,
>> which translates to an accuracy of a 1000 ppm, where as your typical
>> crystal oscillator is 20 - 100 ppm, so the accuracy of the frequency
>> format used in the Software Developer’s Manual is not really helpful.
>>
>> As far as we know Bay Trail SoCs use a 25 MHz crystal and Cherry Trail
>> uses a 19.2 MHz crystal, the crystal is the source clk for a root PLL
>> which outputs 1600 and 100 MHz. It is unclear if the root PLL outputs are
>> used directly by the CPU clock PLL or if there is another PLL in between.
>>
>> This does not matter though, we can model the chain of PLLs as a single
>> PLL with a quotient equal to the quotients of all PLLs in the chain
>> multiplied.
>>
>> So we can create a simplified model of the CPU clock setup using a
>> reference clock of 100 MHz plus a quotient which gets us as close to the
>> frequency from the SDM as possible.
>>
>> For the 83.3 MHz example from above this would give us 100 MHz * 5 / 6 =
>> 83 and 1/3 MHz, which matches exactly what has been measured on actual hw.
>>
>> This commit makes the tsc_msr.c code use a simplified PLL model with a
>> reference clock of 100 MHz for all Bay and Cherry Trail models.
>>
>> This has been tested on the following models:
>>
>>                CPU freq before:        CPU freq after this commit:
>> Intel N2840   2165.800 MHz            2166.667 MHz
>> Intel Z3736   1332.800 MHz            1333.333 MHz
>> Intel Z3775   1466.300 MHz            1466.667 MHz
>> Intel Z8350   1440.000 MHz            1440.000 MHz
>> Intel Z8750   1600.000 MHz            1600.000 MHz
>>
>> This fixes the time drifting by about 1 second per hour (20 - 30 seconds
>> per day) on (some) devices which rely on the tsc_msr.c code to determine
>> the TSC frequency.
> 
> Thanks for this effort!
> 
> ...
> 
>> +#define REFERENCE_KHZ 100000
> 
> Perhaps TSC_REFERENCE_KHZ ?

Ok, changed to TSC_REFERENCE_KHZ for v3

> 
> ...
> 
>> +       struct {
>> +               u32 multiplier;
>> +               u32 divider;
>> +       } pairs[MAX_NUM_FREQS];
> 
> Perhaps pairs -> muldiv ?

Ok, changed to muldiv for v3

> 
> ...
> 
>> +       .pairs = { {  5,  6 }, { 1,  1 }, { 4, 3 }, { 7, 6 }, { 4, 5 },
>> +                  { 14, 15 }, { 9, 10 }, { 8, 9 }, { 7, 8 } },
> 
> Maybe 4 per line or alike (8 per line) for better understanding which
> muldiv maps to which value?

Ok, changed for v3.

> 
> ...
> 
>> +       .pairs = { { 0, 0 }, { 1, 1 }, { 4, 3 } },
> 
> And maybe list all of them always? (I'm fine with either approach).

I prefer to just list the valid ones.

> 
> ...
> 
>> +/* 24 MHz crystal? : 24 * 13 / 4 = 78 MHz */
> 
> Perhaps Cc to LGM SoC developers team (they did it recently, so, they
> have to know).

Ok, I've added the following people to the Cc for v3 based on the Sob-s and Cc-s of:
0cc5359d8fd45bc("x86/cpu: Update init data for new Airmont CPU model"):

Cc: Rahul Tanwar <rahul.tanwar@...ux.intel.com>
Cc: Tony Luck <tony.luck@...el.com>
Cc: Gayatri Kammela <gayatri.kammela@...el.com>


> ...
> 
>> +       if (freq_desc->pairs[index].divider) {
> 
>> +               freq = DIV_ROUND_CLOSEST(REFERENCE_KHZ *
>> +                                           freq_desc->pairs[index].multiplier,
>> +                                        freq_desc->pairs[index].divider);
> 
> Maybe helper?
> 
>> +               /* Multiply by ratio before the divide for better accuracy */
>> +               res = DIV_ROUND_CLOSEST(REFERENCE_KHZ *
>> +                                           freq_desc->pairs[index].multiplier *
>> +                                           ratio,
>> +                                       freq_desc->pairs[index].divider);
> 
> ...which may be used here as well.

Nah, I would prefer to keep this as is. I'm never a fan of single line
helpers they just make it harder to see what the code is actually doing.

Regards,

Hans