Re: [PATCH] KVM: x86: Use current rather than snapshotted TSC frequency if it is constant

[Sean Christopherson <seanjc@google.com>]

+Vitaly

On Thu, Apr 14, 2022, Anton Romanov wrote:
> Don't snapshot tsc_khz into per-cpu cpu_tsc_khz if the
> host TSC is constant, in which case the actual TSC frequency will never
> change and thus capturing TSC during initialization is
> unnecessary, KVM can simply use tsc_khz.
> This value is snapshotted from
> kvm_timer_init->kvmclock_cpu_online->tsc_khz_changed(NULL)

Nit, please wrap changelogs at ~75 chars.  It's not a hard rule, e.g. if running
over or cutting early improves readability, then by all means.  But wrapping
somewhat randomly makes reading the changelog unnecessarily difficult.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 547ba00ef64f..4ae9a03f549d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2907,6 +2907,19 @@ static void kvm_update_masterclock(struct kvm *kvm)
>  	kvm_end_pvclock_update(kvm);
>  }
>  
> +/*
> + * If kvm is built into kernel it is possible that tsc_khz saved into
> + * per-cpu cpu_tsc_khz was yet unrefined value. If CPU provides CONSTANT_TSC it
> + * doesn't make sense to snapshot it anyway so just return tsc_khz
> + */
> +static unsigned long get_cpu_tsc_khz(void)
> +{
> +	if (static_cpu_has(X86_FEATURE_CONSTANT_TSC))
> +		return tsc_khz;
> +	else
> +		return __this_cpu_read(cpu_tsc_khz);
> +}
> +
>  /* Called within read_seqcount_begin/retry for kvm->pvclock_sc.  */
>  static void __get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
>  {
> @@ -2917,7 +2930,7 @@ static void __get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
>  	get_cpu();
>  
>  	data->flags = 0;
> -	if (ka->use_master_clock && __this_cpu_read(cpu_tsc_khz)) {
> +	if (ka->use_master_clock && get_cpu_tsc_khz()) {

It might make sense to open code this to make it more obvious why the "else" path
exists.  That'd also eliminate a condition branch on CPUs with a constant TSC,
though I don't know if we care that much about the performance here.

	if (ka->use_master_clock &&
	    (static_cpu_has(X86_FEATURE_CONSTANT_TSC) || __this_cpu_read(cpu_tsc_khz)))

And/or add a comment about cpu_tsc_khz being zero when the CPU is being offlined?

>  #ifdef CONFIG_X86_64
>  		struct timespec64 ts;
>  

...

> @@ -8646,9 +8659,12 @@ static void tsc_khz_changed(void *data)
>  	struct cpufreq_freqs *freq = data;
>  	unsigned long khz = 0;
>  
> +	if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
> +		return;

Vitaly,

The Hyper-V guest code also sets cpu_tsc_khz, should we WARN if that notifier is
invoked and Hyper-V told us there's a constant TSC?

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ab336f7c82e4..ca8e20f5ffc0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8701,6 +8701,8 @@ static void kvm_hyperv_tsc_notifier(void)
        struct kvm *kvm;
        int cpu;

+       WARN_ON_ONCE(boot_cpu_has(X86_FEATURE_CONSTANT_TSC));
+
        mutex_lock(&kvm_lock);
        list_for_each_entry(kvm, &vm_list, vm_list)
                kvm_make_mclock_inprogress_request(kvm);