Re: xen 4.14.3 incorrect (~3x) cpu frequency reported

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.01.2022 16:43, Andrew Cooper wrote:
> On 10/01/2022 13:11, Jan Beulich wrote:
>> On 10.01.2022 13:37, Roger Pau Monné wrote:
>>> On Fri, Jan 07, 2022 at 12:39:04PM +0100, Jan Beulich wrote:
>>>> @@ -415,16 +416,35 @@ static int64_t __init init_hpet(struct p
>>>>  
>>>>      pts->frequency = hpet_rate;
>>>>  
>>>> +for(i = 0; i < 16; ++i) {//temp
>>>>      count = hpet_read32(HPET_COUNTER);
>>>>      start = rdtsc_ordered();
>>>>      target = count + CALIBRATE_VALUE(hpet_rate);
>>>>      if ( target < count )
>>>>          while ( hpet_read32(HPET_COUNTER) >= count )
>>>>              continue;
>>>> -    while ( hpet_read32(HPET_COUNTER) < target )
>>>> +    while ( (count = hpet_read32(HPET_COUNTER)) < target )
>>>>          continue;
>>>>  
>>>> -    return (rdtsc_ordered() - start) * CALIBRATE_FRAC;
>>>> +    expired = rdtsc_ordered() - start;
>>> There's also a window between the HPET read and the TSC read where an
>>> SMI/NMI could cause a wrong frequency detection.
>> Right, as said in an earlier reply I did notice this myself (actually
>> on the way home on Friday). As also said there, for now I can't see
>> any real (i.e. complete) solution to this and the similar instances
>> of the issue elsewhere.
> 
> Calibration loops like this cannot be made robust.  This is specifically
> why frequency information is being made available via architectural
> means, and is available via non-architectural means in other cases.
> 
> There's a whole bunch of situations (#STOPCLK, TERM and PSMI off the top
> of my head, but I bet HDC will screw with things too) which will mess
> with any calibration loop, even if you could disable SMIs.  While,
> mechanically, we can disable SMIs on AMD with CLGI, we absolutely
> shouldn't run a calibration loop like this with SMIs disabled.
> 
> 
> Much as I hate to suggest it, we should parse the frequency out of the
> long CPUID string, and compare it to the calibration time.  (This
> technique is mandated in the Intel BWG during very early setup.)

This, according to some initial checking, might work for Intel CPUs,
but apparently won't work for AMD ones (and I don't dare to think of
what might happen if we run under, say, qemu). Imo we'd need to deal
gracefully with the case that we can't parse the frequency out of
that string, with "gracefully" including that our calibration still
won't be too far off.

Also I wonder if we wouldn't better prefer CPUID leaf 0x15 / 0x16
data over parsing extended leaf.

> If it is different by a large margin, rerun the calibration, and if it
> is still different, complain loudly into the logs.  This will fix a
> one-off-spurious event, whereas if e.g. the system is thermally
> throttling due to a bad heat sync, there is nothing software can do to
> recover the system.

I was already considering to use reference data like that from CPUID
leaves 0x15 / 0x16, but I couldn't really settle on what "large
margin" would really want to be. Imo we should try to correct not-
just-as-large errors as well, if we can.

For the moment I continue to consider my plan (outlined in another
reply on this thread) superior to what you suggest, but I'll be
looking forward to further replies from you or others.

Jan