Re: [PATCH] x86/Intel: don't log bogus frequency range on Core/Core2 processors

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Feb 04, 2022 at 02:56:43PM +0100, Jan Beulich wrote:
> Models 0F and 17 don't have PLATFORM_INFO documented. While it exists on
> at least model 0F, the information there doesn't match the scheme used
> on newer models (I'm observing a range of 700 ... 600 MHz reported on a
> Xeon E5345).

Maybe it would be best to limit ourselves to the models that have the
MSR documented in the SDM?

> 
> Sadly the Enhanced Intel Core instance of the table entry is not self-
> consistent: The numeric description of the low 3 bits doesn't match the
> subsequent more textual description in some of the cases; I'm using the
> former here.
> 
> Include the older Core model 0E as well as the two other Core2 models,
> none of which have respective MSR tables in the SDM.
> 
> Fixes: f6b6517cd5db ("x86: retrieve and log CPU frequency information")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> While the SDM table for the two models lists FSB_FREQ, I'm afraid its
> information is of little use here: If anything it could serve as a
> reference for the frequency determined by calibrate_APIC_clock().
> ---
> RFC: I may want to rebase over Roger's addition of intel-family.h, but
>      first of all I wanted to see whether going this route is deemed
>      acceptable at all.
> 
> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -435,6 +435,26 @@ static void intel_log_freq(const struct
>          if ( c->x86 == 6 )
>              switch ( c->x86_model )
>              {
> +                static const unsigned short core_factors[] =
> +                    { 26667, 13333, 20000, 16667, 33333, 10000, 40000 };
> +
> +            case 0x0e: /* Core */
> +            case 0x0f: case 0x16: case 0x17: case 0x1d: /* Core2 */
> +                /*
> +                 * PLATFORM_INFO, while not documented for these, appears to
> +                 * exist in at least some cases, but what it holds doesn't
> +                 * match the scheme used by newer CPUs.  At a guess, the min
> +                 * and max fields look to be reversed, while the scaling
> +                 * factor is encoded in FSB_FREQ.
> +                 */
> +                if ( min_ratio > max_ratio )
> +                    SWAP(min_ratio, max_ratio);
> +                if ( rdmsr_safe(MSR_FSB_FREQ, msrval) ||
> +                     (msrval &= 7) >= ARRAY_SIZE(core_factors) )
> +                    return;
> +                factor = core_factors[msrval];
> +                break;
> +
>              case 0x1a: case 0x1e: case 0x1f: case 0x2e: /* Nehalem */
>              case 0x25: case 0x2c: case 0x2f: /* Westmere */
>                  factor = 13333;

Seeing that the MSR is present on non documented models and has
unknown behavior we might want to further sanity check that min < max
before printing anything?

Thanks, Roger.