Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 6 Aug 2021 16:20:08 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Suleiman Souhlal <suleiman@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, ssouhlal@freebsd.org,
        hikalium@chromium.org, senozhatsky@chromium.org,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kvm,x86: Use the refined tsc rate for the guest tsc.
Message-ID: <YQ1hOJNMnD6gCRjD@google.com>
References: <20210803075914.3070477-1-suleiman@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210803075914.3070477-1-suleiman@google.com>
Precedence: bulk

"KVM: x86:" is the preferred scope.  And for whatever reason, punctuation at the
end of the shortlog is almost always omitted.

On Tue, Aug 03, 2021, Suleiman Souhlal wrote:
> Prior to this change,

In the changelog proper, please state what change is being made before launching
into the effects of the change.  Oftentimes that does mean restating the shortlog,
but it's very helpful to reviewers/readers to provide a single cohesive/coherent
explanation.

> the initial tsc rate used by kvm would be the unrefined rate, instead of the
> refined rate that is derived later at boot and used for timekeeping. This can
> cause time to advance at different rates between the host and the guest.

This needs a lot more explanation, with a clear statement of the direct effects
of the change and with explicit references to variables and probably functions.
Normally I prefer abstraction over explicit code references, but in this case
there are too many ambiguous terms to understand the intended change without a
lot of staring.  E.g. at first blush, it's not obvious that "boot" refers to the
host kernel boot, especially for those of us that load KVM as a module.

And the statement "the initial tsc rate used by kvm would be the unrefined rate"
will not always hold true, e.g. when KVM is loaded long after boot.  That also
confused me.

IIUC, this "fixes" a race where KVM is initialized before the second call to
tsc_refine_calibration_work() completes.  Fixes in quotes because it doesn't
actually fix the race, it just papers over the problem to get the desired behavior.
If the race can't be truly fixed, the changelog should explain why it can't be
fixed, otherwise fudging our way around the race is not justifiable.

Ideally, we would find a way to fix the race, e.g. by ensuring KVM can't load or
by stalling KVM initialization until refinement completes (or fails).  tsc_khz is
consumed by KVM in multiple paths, and initializing KVM before tsc_khz calibration
is fully refined means some part of KVM will use the wrong tsc_khz, e.g. the VMX
preemption timer.  Due to sanity checks in tsc_refine_calibration_work(), the delta
won't be more than 1%, but it's still far from ideal.

> Signed-off-by: Suleiman Souhlal <suleiman@google.com>
> ---
>  arch/x86/kvm/x86.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4116567f3d44..1e59bb326c10 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2199,6 +2199,7 @@ static atomic_t kvm_guest_has_master_clock = ATOMIC_INIT(0);
>  #endif
>  
>  static DEFINE_PER_CPU(unsigned long, cpu_tsc_khz);
> +static DEFINE_PER_CPU(bool, cpu_tsc_khz_changed);
>  static unsigned long max_tsc_khz;
>  
>  static u32 adjust_tsc_khz(u32 khz, s32 ppm)
> @@ -2906,6 +2907,14 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
>  		kvm_make_request(KVM_REQ_CLOCK_UPDATE, v);
>  		return 1;
>  	}
> +	/*
> +	 * Use the refined tsc_khz instead of the tsc_khz at boot (which was
> +	 * not refined yet when we got it),

As above, this does not hold true in all cases.  And for anyone that isn't familiar
with tsc_refine_calibration_work(), the "refined tsc_khz instead of the tsc_khz"
blurb is borderline nonsensical.

> +	 * If the frequency does change, it does not get refined any further,
> +	 * so it is safe to use the one gotten from the notifiers.
> +	 */
> +	if (!__this_cpu_read(cpu_tsc_khz_changed))
> +		tgt_tsc_khz = tsc_khz;
>  	if (!use_master_clock) {
>  		host_tsc = rdtsc();
>  		kernel_ns = get_kvmclock_base_ns();
> @@ -8086,6 +8095,8 @@ static void tsc_khz_changed(void *data)
>  		khz = freq->new;
>  	else if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
>  		khz = cpufreq_quick_get(raw_smp_processor_id());
> +	if (khz)
> +		__this_cpu_write(cpu_tsc_khz_changed, true);

On a system with a constant TSC and KVM loaded after boot, cpu_tsc_khz_changed
will never be true.  Ditto for the case where refinement fails.  That may not be
a functional issue, but it's confusing.

This also fails to gate other readers of kvm_guest_time_update.  In particular,
KVM_GET_CLOCK -> get_kvmclock_ns() will use the "wrong" frequency when userspace
is retrieving guest TSC.

>  	if (!khz)
>  		khz = tsc_khz;
>  	__this_cpu_write(cpu_tsc_khz, khz);
> -- 
> 2.32.0.554.ge1b32706d8-goog
>