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

[Jan Beulich <jbeulich@xxxxxxxx>]

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