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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
albeit I'd much prefer if the formatting adjustments were dropped.

Jan