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.