Re: [PATCH] x86, tsc: Limit CPU frequency calibration on AMD

[john stultz <johnstul@us.ibm.com>]

On Thu, 2010-08-19 at 22:29 +0200, Borislav Petkov wrote:
> From: john stultz <johnstul@us.ibm.com>
> Date: Thu, Aug 19, 2010 at 02:47:35PM -0400
> 
> Hi John,
> 
> > On Wed, Aug 18, 2010 at 9:16 AM, Borislav Petkov <bp@amd64.org> wrote:
> > > 6b37f5a20c0e5c334c010a587058354215433e92 introduced the CPU frequency
> > > calibration code for AMD CPUs whose TSCs didn't increment with the
> > > core's P0 frequency. From F10h, revB onward, the TSC increment rate is
> > > denoted by MSRC001_0015[24] and when this bit is set (which is normally
> > > done by the BIOS,) the TSC increments with the P0 frequency so the
> > > calibration is not needed and booting can be a couple of mcecs faster on
> > > those machines.
> > 
> > Very cool. This was brought up on an earlier thread and is really nice
> > because it also avoids the 50+ppm variability easily seen in the
> > calibrate results boot to boot. That variance causes difficulty
> > keeping close NTP sync across reboots, as the persistent drift value
> > is invalid after a reboot.
> > 
> > I recently proposed a timer based solution that doesn't block bootup,
> > and allows for very accurate calibration.  This might help fill the
> > gaps on legacy systems that do not provide TSC freq information. I'd
> > be interested if you had any comments.
> > 
> > https://kerneltrap.org/mailarchive/linux-kernel/2010/7/28/4598868
> > 
> > Notes on the code below.
> > 
> > > Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> > > ---
> > >  arch/x86/kernel/tsc.c |   14 ++++++++++++--
> > >  1 files changed, 12 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > > index ce8e502..41b2b8b 100644
> > > --- a/arch/x86/kernel/tsc.c
> > > +++ b/arch/x86/kernel/tsc.c
> > > @@ -927,8 +927,18 @@ void __init tsc_init(void)
> > >        }
> > >
> > >        if (cpu_has(&boot_cpu_data, X86_FEATURE_CONSTANT_TSC) &&
> > > -                       (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
> > > -               cpu_khz = calibrate_cpu();
> > > +                       (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)) {
> > > +
> > > +               if (boot_cpu_data.x86 > 0x10 ||
> > > +                   (boot_cpu_data.x86 == 0x10 &&
> > > +                    boot_cpu_data.x86_model >= 0x2)) {
> > > +                       u64 val;
> > > +
> > > +                       rdmsrl(MSR_K7_HWCR, val);
> > > +                       if (!(val & BIT(24)))
> > > +                               cpu_khz = calibrate_cpu();
> > > +               }
> > > +       }
> > 
> > Am I just missing the point in the code where you actually use the msr
> > value to assign cpu_khz? Or is the idea that the default tsc
> > calibration already is correct, and we don't need to further refine
> > it?
> 
> Yes.
> 
> Actually Alok brought the code to my attention originally, and what
> puzzled me was the fact that we do calibrate_cpu() on all F10h and later
> AMD machines needlessly (well, almost all, maybe 99%). This is because,
> on F10h, revB machines and later we support "TSC increments with P0
> frequency" so what native_calibrate_tsc() comes up with in terms of
> tsc_khz should be the same as cpu_khz.
> 
> So the MSR bit check above is to see whether the TSC increments with P0
> frequency and call calibrate_cpu only if _not_.

Ah. I see, sorry for misreading it.


> As a result, this patch basically drops the additional cpu_khz
> calibration when it isn't needed. And as such it doesn't have a whole
> lot to do with the actual TSC calibration - this is up to you guys :).
> 
> The original reason for the calibrate_cpu() is that there's the
> possibility for the TSC to count with the northbridge clockrate and
> there we need to recalibrate obviously.
> 
> Hope that makes it more clear.
> 
> > And yea, the calibrate_cpu function needs to be renamed.
> 
> Done, the whole code is moved to cpu/amd.c anyway. I'm currently testing
> the new version and will be sending out maybe tomorrow or so.

Great!

thanks
-john