[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/Intel: don't log bogus frequency range on Core/Core2 processors
On 08.02.2022 15:20, Roger Pau Monné wrote: > 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: >>>> --- 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. Oh, I see. Yes, I did consider this, but decided against because it would hide cases where we're not in line with reality. I might not have spotted the issue here if we would have had such a check in place already (maybe the too low number would have caught my attention, but the <high> ... <low> range logged was far more obviously wrong). (In any event, if such a change was to be made, I think it should be a separate patch.) Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |