[PATCH] x86/tsc: Fix diagnostics for TSC frequency

[PATCH] x86/tsc: Fix diagnostics for TSC frequency

[Andrew Cooper]

A Gemini Lake platform prints:

  (XEN) CPU0: TSC: 19200000MHz * 279 / 3 = 1785600000MHz
  (XEN) CPU0: 800..1800 MHz

during boot.  The units on the first line are Hz, not MHz, so correct that and
add a space for clarity.

Also, for the min/max line, use three dots instead of two and add more spaces
so that the line can't be mistaken for being a double decimal point typo.

Boot now reads:

  (XEN) CPU0: TSC: 19200000 Hz * 279 / 3 = 1785600000 Hz
  (XEN) CPU0: 800 ... 1800 MHz

Extend these changes to the other TSC diagnostics.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/amd.c   |  4 ++--
 xen/arch/x86/cpu/intel.c | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 0cc6853c42..8bc51bec10 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -624,10 +624,10 @@ void amd_log_freq(const struct cpuinfo_x86 *c)
 	if (idx && idx < h &&
 	    !rdmsr_safe(0xC0010064 + idx, val) && (val >> 63) &&
 	    !rdmsr_safe(0xC0010064, hi) && (hi >> 63))
-		printk("CPU%u: %lu (%lu..%lu) MHz\n",
+		printk("CPU%u: %lu (%lu ... %lu) MHz\n",
 		       smp_processor_id(), FREQ(val), FREQ(lo), FREQ(hi));
 	else if (h && !rdmsr_safe(0xC0010064, hi) && (hi >> 63))
-		printk("CPU%u: %lu..%lu MHz\n",
+		printk("CPU%u: %lu ... %lu MHz\n",
 		       smp_processor_id(), FREQ(lo), FREQ(hi));
 	else
 		printk("CPU%u: %lu MHz\n", smp_processor_id(), FREQ(lo));
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index 69e99bb358..ed70b43942 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -396,14 +396,14 @@ static void intel_log_freq(const struct cpuinfo_x86 *c)
 
             val *= ebx;
             do_div(val, eax);
-            printk("CPU%u: TSC: %uMHz * %u / %u = %LuMHz\n",
+            printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n",
                    smp_processor_id(), ecx, ebx, eax, val);
         }
         else if ( ecx | eax | ebx )
         {
             printk("CPU%u: TSC:", smp_processor_id());
             if ( ecx )
-                printk(" core: %uMHz", ecx);
+                printk(" core: %u MHz", ecx);
             if ( ebx && eax )
                 printk(" ratio: %u / %u", ebx, eax);
             printk("\n");
@@ -417,11 +417,11 @@ static void intel_log_freq(const struct cpuinfo_x86 *c)
         {
             printk("CPU%u:", smp_processor_id());
             if ( ecx )
-                printk(" bus: %uMHz", ecx);
+                printk(" bus: %u MHz", ecx);
             if ( eax )
-                printk(" base: %uMHz", eax);
+                printk(" base: %u MHz", eax);
             if ( ebx )
-                printk(" max: %uMHz", ebx);
+                printk(" max: %u MHz", ebx);
             printk("\n");
         }
     }
@@ -446,7 +446,7 @@ static void intel_log_freq(const struct cpuinfo_x86 *c)
 
         printk("CPU%u: ", smp_processor_id());
         if ( min_ratio )
-            printk("%u..", (factor * min_ratio + 50) / 100);
+            printk("%u ... ", (factor * min_ratio + 50) / 100);
         printk("%u MHz\n", (factor * max_ratio + 50) / 100);
     }
 }
-- 
2.11.0

Re: [PATCH] x86/tsc: Fix diagnostics for TSC frequency

[Jan Beulich]

On 05.08.2020 16:18, Andrew Cooper wrote:
> A Gemini Lake platform prints:
> 
>   (XEN) CPU0: TSC: 19200000MHz * 279 / 3 = 1785600000MHz
>   (XEN) CPU0: 800..1800 MHz
> 
> during boot.  The units on the first line are Hz, not MHz, so correct that and
> add a space for clarity.
> 
> Also, for the min/max line, use three dots instead of two and add more spaces
> so that the line can't be mistaken for being a double decimal point typo.
> 
> Boot now reads:
> 
>   (XEN) CPU0: TSC: 19200000 Hz * 279 / 3 = 1785600000 Hz
>   (XEN) CPU0: 800 ... 1800 MHz
> 
> Extend these changes to the other TSC diagnostics.

I'm happy to see the unit mistake fixed, but the choice of
formatting was pretty deliberate when the code was introduced:
As dense as possible without making things unreadable or
ambiguous. (Considering "a double decimal point typo" looks
like a joke to me, really.)

> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -396,14 +396,14 @@ static void intel_log_freq(const struct cpuinfo_x86 *c)
>  
>              val *= ebx;
>              do_div(val, eax);
> -            printk("CPU%u: TSC: %uMHz * %u / %u = %LuMHz\n",
> +            printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n",
>                     smp_processor_id(), ecx, ebx, eax, val);

For this one I wonder whether ecx wouldn't better be scaled down to
kHz, and val down to MHz.

>          }
>          else if ( ecx | eax | ebx )
>          {
>              printk("CPU%u: TSC:", smp_processor_id());
>              if ( ecx )
> -                printk(" core: %uMHz", ecx);
> +                printk(" core: %u MHz", ecx);

This one now clearly wants to say Hz too, or (as above) scaling
down to kHz. With at least this last issue addressed
Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit I'd much prefer if the formatting adjustments were dropped.

Jan

Re: [PATCH] x86/tsc: Fix diagnostics for TSC frequency

[Andrew Cooper]

On 05/08/2020 15:54, Jan Beulich wrote:
> On 05.08.2020 16:18, Andrew Cooper wrote:
>> A Gemini Lake platform prints:
>>
>>   (XEN) CPU0: TSC: 19200000MHz * 279 / 3 = 1785600000MHz
>>   (XEN) CPU0: 800..1800 MHz
>>
>> during boot.  The units on the first line are Hz, not MHz, so correct that and
>> add a space for clarity.
>>
>> Also, for the min/max line, use three dots instead of two and add more spaces
>> so that the line can't be mistaken for being a double decimal point typo.
>>
>> Boot now reads:
>>
>>   (XEN) CPU0: TSC: 19200000 Hz * 279 / 3 = 1785600000 Hz
>>   (XEN) CPU0: 800 ... 1800 MHz
>>
>> Extend these changes to the other TSC diagnostics.
> I'm happy to see the unit mistake fixed, but the choice of
> formatting was pretty deliberate when the code was introduced:
> As dense as possible without making things unreadable or
> ambiguous. (Considering "a double decimal point typo" looks
> like a joke to me, really.)

I literally thought it was a typo until I read the code.  So no - I'm
very much not joking.

Decimal points are extremely commonly seen with frequencies, and nothing
else in the log line gives any hint that it is range.

Despite being deliberate, it is overly dense and ambiguous as a consequence.

>> --- a/xen/arch/x86/cpu/intel.c
>> +++ b/xen/arch/x86/cpu/intel.c
>> @@ -396,14 +396,14 @@ static void intel_log_freq(const struct cpuinfo_x86 *c)
>>  
>>              val *= ebx;
>>              do_div(val, eax);
>> -            printk("CPU%u: TSC: %uMHz * %u / %u = %LuMHz\n",
>> +            printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n",
>>                     smp_processor_id(), ecx, ebx, eax, val);
> For this one I wonder whether ecx wouldn't better be scaled down to
> kHz, and val down to MHz.

That depends on whether we will lose precision in the process.

In principle we can, given ecx's unit of Hz, so I'd be tempted to leave
it as is.

>
>>          }
>>          else if ( ecx | eax | ebx )
>>          {
>>              printk("CPU%u: TSC:", smp_processor_id());
>>              if ( ecx )
>> -                printk(" core: %uMHz", ecx);
>> +                printk(" core: %u MHz", ecx);
> This one now clearly wants to say Hz too, or (as above) scaling
> down to kHz.

Oops.  Will fix.

> With at least this last issue addressed
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks,

~Andrew

Re: [PATCH] x86/tsc: Fix diagnostics for TSC frequency

[Jan Beulich]

On 05.08.2020 18:25, Andrew Cooper wrote:
> On 05/08/2020 15:54, Jan Beulich wrote:
>> On 05.08.2020 16:18, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpu/intel.c
>>> +++ b/xen/arch/x86/cpu/intel.c
>>> @@ -396,14 +396,14 @@ static void intel_log_freq(const struct cpuinfo_x86 *c)
>>>  
>>>              val *= ebx;
>>>              do_div(val, eax);
>>> -            printk("CPU%u: TSC: %uMHz * %u / %u = %LuMHz\n",
>>> +            printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n",
>>>                     smp_processor_id(), ecx, ebx, eax, val);
>> For this one I wonder whether ecx wouldn't better be scaled down to
>> kHz, and val down to MHz.
> 
> That depends on whether we will lose precision in the process.

I don't think losing the last three digits for the base clock and
the last six ones of the calculated value would do any harm at all.
All it would do (imo) is to make the numbers better readable (due
less counting, and hence less possible counting mistakes).

> In principle we can, given ecx's unit of Hz, so I'd be tempted to leave
> it as is.

Well, okay.

Jan