Re: Re: [PATCH 2/2] x86: use KNOWN_FREQ and RELIABLE TSC flags on certain processors/SoCs

[Thomas Gleixner]

On Thu, 10 Nov 2016, Bin Gao wrote:
> > > @@ -702,6 +702,15 @@ unsigned long native_calibrate_tsc(void)
> > >  		}
> > >  	}
> > >  
> > > +	setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
> > 
> > I can understand the one below, but this one changes existing behaviour w/o explaining why this is correct and desired. If at all then this wants to be a seperate patch and not just mingled in your goldmont update.
> 
> native_calibrate_tsc() implements determining TSC frequency via CPUID.
> The purpose to add X86_FEATURE_TSC_KNOWN_FREQ flag is exactly for this case:
> TSC frequency determined via CPUID or MSR are always correct and the whole
> calibration should be skipped.

Did you actually verify that this is correct and does not introduce NTP
issues compared to the long term calibration on such platforms?

We've been burnt before and myself and others wasted enough time already
debugging that crap.

> I will create a seperate patch for this to ensure it's not confusing with
> the MSR related change below.

Yes please.
 
> > > @@ -100,5 +100,9 @@ unsigned long cpu_khz_from_msr(void)  #ifdef 
> > > CONFIG_X86_LOCAL_APIC
> > >  	lapic_timer_frequency = (freq * 1000) / HZ;  #endif
> > > +
> > > +	setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
> > > +	setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
> > 
> > Why is this automatically reliable and of known frequency?
> 

> As I said above, TSC frequency determined by CPUID or MSR is always considered
> "known" because it is reported by HW.
> Regarding the reliable, unfortunately however, there is no a HW way to report
> it. We were told by silicon design team it's "reliable".

Please add a comment which explains this in great length.

Thanks,

	tglx