linux-kernel - Re: [PATCH v3 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: <36b1e1bb-cbb0-88c0-dab2-aa5e14233e63@redhat.com>
Date:   Sun, 23 Feb 2020 14:55:03 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Andy Shevchenko <andy@...radead.org>,
        Ingo Molnar <mingo@...hat.com>
Cc:     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>,
        Rahul Tanwar <rahul.tanwar@...ux.intel.com>,
        Tony Luck <tony.luck@...el.com>,
        Gayatri Kammela <gayatri.kammela@...el.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v3 3/3] x86/tsc_msr: Make MSR derived TSC frequency more
 accurate

Hi,

On 2/8/20 1:05 AM, Thomas Gleixner wrote:
> Hans,
> 
> Hans de Goede <hdegoede@...hat.com> writes:
>> @@ -120,11 +180,23 @@ unsigned long cpu_khz_from_msr(void)
>>   	rdmsr(MSR_FSB_FREQ, lo, hi);
>>   	index = lo & freq_desc->mask;
>>   
>> -	/* Map CPU reference clock freq ID(0-7) to CPU reference clock freq(KHz) */
>> -	freq = freq_desc->freqs[index];
>> -
>> -	/* TSC frequency = maximum resolved freq * maximum resolved bus ratio */
>> -	res = freq * ratio;
>> +	/*
>> +	 * Note this also catches cases where the index points to an unpopulated
>> +	 * part of muldiv, in that case the else will set freq and res to 0.
>> +	 */
>> +	if (freq_desc->muldiv[index].divider) {
>> +		freq = DIV_ROUND_CLOSEST(TSC_REFERENCE_KHZ *
>> +					   freq_desc->muldiv[index].multiplier,
>> +					 freq_desc->muldiv[index].divider);
>> +		/* Multiply by ratio before the divide for better accuracy */
>> +		res = DIV_ROUND_CLOSEST(TSC_REFERENCE_KHZ *
>> +					   freq_desc->muldiv[index].multiplier *
>> +					   ratio,
>> +					freq_desc->muldiv[index].divider);
> 
> What about:
> 
>          struct muldiv *md = &freq_desc->muldiv[index];
> 
>          if (md->divider) {
> 		tscref = TSC_REFERENCE_KHZ * md->multiplier;
>          	freq = DIV_ROUND_CLOSEST(tscref, md->divider);
> 		/*
>                   * Multiplying by ratio before the division has better
>                   * accuracy than just calculating freq * ratio
>                   */
>                  res = DIV_ROUND_CLOSEST(tscref * ratio, md->divider);
> 
> Hmm?

That indeed looks nicer, I've prepared (and tested) a v4 with the
suggested change, I'll send out v4 right after this email.

Regards,

Hans