Re: Re: [PATCH v2 2/2] KVM: x86: use x86_get_freq to get freq for kvmclock

Maxim Levitsky
2021-12-02T07:19:39Z

On Thu, 2021-12-02 at 13:26 +0800, zhenwei pi wrote:
> On 12/2/21 10:48 AM, Thomas Gleixner wrote:
> > On Wed, Dec 01 2021 at 10:46, zhenwei pi wrote:
> > > If the host side supports APERF&MPERF feature, the guest side may get
> > > mismatched frequency.
> > >
> > > KVM uses x86_get_cpufreq_khz() to get the same frequency for guest side.
> > >
> > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> > > ---
> > > arch/x86/kvm/x86.c | 4 +---
> > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 5a403d92833f..125ed3c8b21a 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -8305,10 +8305,8 @@ static void tsc_khz_changed(void *data)
> > >
> > > if (data)
> > > khz = freq->new;
> > > - else if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
> > > - khz = cpufreq_quick_get(raw_smp_processor_id());
> > > if (!khz)
> > > - khz = tsc_khz;
> > > + khz = x86_get_cpufreq_khz(raw_smp_processor_id());
> >
> > my brain compiler tells me that this is broken.
> > Without this patch:
> 1, boot_cpu_has(X86_FEATURE_CONSTANT_TSC) is true:
> no kvmclock_cpufreq_notifier, and khz = tsc_khz;
>
> 2, boot_cpu_has(X86_FEATURE_CONSTANT_TSC) is false:
> during installing kmod, try cpufreq_quick_get(), or use tsc_khz;
> and get changed by kvmclock_cpufreq_notifier.
>
> With this patch:
> 1, boot_cpu_has(X86_FEATURE_CONSTANT_TSC) is true:
> no kvmclock_cpufreq_notifier, try aperf/mperf, or try
> cpufreq_quick_get(), or use cpu_khz
>
> 2, boot_cpu_has(X86_FEATURE_CONSTANT_TSC) is false:
> during installing kmod, try aperf/mperf, or try cpufreq_quick_get(), or
> use cpu_khz;
> and get changed by kvmclock_cpufreq_notifier.
>
> I tested on Skylake&Icelake CPU, and got different CPU frequency from
> host & guest, the main purpose of this patch is to get the same frequency.
>

Note that on my Zen2 machine (3970X), aperf/mperf returns current cpu freqency,
as now see in /proc/cpuinfo, while TSC is always running with base CPU clock frequency (3.7 GHZ)
(that is max frequency that CPU is guranteed to run with, anything above is boost 'bonus')

[mlevitsk@starship ~/Kernel/br-vm-64/src]$cat /proc/cpuinfo | grep "cpu MHz"
cpu MHz : 3685.333
cpu MHz : 2200.000
cpu MHz : 2200.000
cpu MHz : 2200.000
cpu MHz : 2200.000
cpu MHz : 2200.000
cpu MHz : 2200.000
cpu MHz : 2200.000
cpu MHz : 2200.000
cpu MHz : 2200.000
cpu MHz : 2200.000
cpu MHz : 2761.946
cpu MHz : 2200.000
cpu MHz : 2200.000
cpu MHz : 2200.000
...

[mlevitsk@starship ~/Kernel/master/src]$dmesg | grep tsc
[ 0.000000] tsc: Fast TSC calibration using PIT
[ 0.000000] tsc: Detected 3700.230 MHz processor
...

Before I forget about it I do want to point out few things
that are not 100% related to this thread but do related to TSC:

1. It sucks that on AMD, the TSC frequency is calibrated from other
clocksources like PIT/HPET, since the result is not exact and varies
from boot to boot. I do wonder if they have something like that
APERF/MPERF thing which sadly is not what I was looking for.

2. In the guest on AMD, we mark the TSC as unsynchronized always due to the code
in unsynchronized_tsc, unless invariant tsc is used in guest cpuid,
which is IMHO not fair to AMD as we don't do this for Intel cpus.
(look at unsynchronized_tsc function)

3. I wish the kernel would export the tsc frequency it found to userspace
somewhere in /sys or /proc, as this would be very useful for userspace applications.
Currently it can only be found in dmesg if I am not mistaken..
I don't mind if such frequency would only be exported if the TSC is stable,
always running, not affected by CPUfreq, etc.

Best regards,
Maxim Levitsky