On Tue, Feb 08, 2022 at 03:28:23PM +0100, Jan Beulich wrote:
> 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.)
My suggestion was to avoid printing both (max and min) if min > max,
as there's obviously something wrong there. Maybe we could print
unconditionally for debug builds, or print an error message otherwise
to note that PLATFORM_INFO is present but the values calculated don't
make sense?
In any case, this is just for informational purposes, so I don't
really want to delay you anymore with this. If you think both options
above are not worth it, feel free to take my Ack for this one:
Acked-by: Roger Pau Monné <[email protected]>
Thanks, Roger.
