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

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

On Tue, Feb 08, 2022 at 11:51:03AM +0100, Jan Beulich wrote:
> On 08.02.2022 09:54, Roger Pau Monné wrote:
> > 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?
> 
> Well, yes, that's what I wasn't sure about: The information is used only
> for logging, so it's not the end of the world if we display something
> strange. We'd want to address such anomalies (like the one I did observe
> here) of course. But I wonder whether being entirely silent is really
> better.

OK, let's add the quirk for Core/Core2 then.

> >> --- 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?
> 
> But I'm already swapping the two in the opposite case?

You are only doing the swapping for Core/Core2.

What I mean is that given the possible availability of
MSR_INTEL_PLATFORM_INFO on undocumented platforms and the different
semantics we should unconditionally check that the frequencies we are
going to print are sane, and one easy check would be that min < max
before printing.

Thanks, Roger.