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

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Wed, May 11, 2022, Anton Romanov wrote:
> @@ -8646,9 +8663,10 @@ static void tsc_khz_changed(void *data)
>  	struct cpufreq_freqs *freq = data;
>  	unsigned long khz = 0;
>  
> +	WARN_ON_ONCE(boot_cpu_has(X86_FEATURE_CONSTANT_TSC));

Nit, please add a newline to isolate the WARN.

>  	if (data)
>  		khz = freq->new;
> -	else if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
> +	else
>  		khz = cpufreq_quick_get(raw_smp_processor_id());
>  	if (!khz)
>  		khz = tsc_khz;
> @@ -8669,8 +8687,10 @@ static void kvm_hyperv_tsc_notifier(void)
>  	hyperv_stop_tsc_emulation();
>  
>  	/* TSC frequency always matches when on Hyper-V */
> -	for_each_present_cpu(cpu)
> -		per_cpu(cpu_tsc_khz, cpu) = tsc_khz;
> +	if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) {
> +		for_each_present_cpu(cpu)
> +			per_cpu(cpu_tsc_khz, cpu) = tsc_khz;
> +	}
>  	kvm_max_guest_tsc_khz = tsc_khz;
>  
>  	list_for_each_entry(kvm, &vm_list, vm_list) {
> @@ -8783,7 +8803,8 @@ static struct notifier_block kvmclock_cpufreq_notifier_block = {
>  
>  static int kvmclock_cpu_online(unsigned int cpu)
>  {
> -	tsc_khz_changed(NULL);
> +	if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
> +		tsc_khz_changed(NULL);


Ah rats, I missed something in v3.  Rather than handle this in the notifier, KVM
can simply not register the notifier in the first place.  The CPUHP_AP_X86_KVM_CLK_ONLINE
hook exists purely to muck with cpu_tsc_khz.  And that makes the WARN_ON_ONCE in
tsc_khz_changed() much less rendunat (having a caller and its callee check the
same thing in quick succession felt silly).

I.e. instead of modifying kvmclock_cpu_online(), do:

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6567aec84223..e9efb8d00673 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8882,10 +8882,10 @@ static void kvm_timer_init(void)
                }
                cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block,
                                          CPUFREQ_TRANSITION_NOTIFIER);
-       }
 
-       cpuhp_setup_state(CPUHP_AP_X86_KVM_CLK_ONLINE, "x86/kvm/clk:online",
-                         kvmclock_cpu_online, kvmclock_cpu_down_prep);
+               cpuhp_setup_state(CPUHP_AP_X86_KVM_CLK_ONLINE, "x86/kvm/clk:online",
+                                 kvmclock_cpu_online, kvmclock_cpu_down_prep);
+       }
 }
 
 #ifdef CONFIG_X86_64
@@ -9038,10 +9038,11 @@ void kvm_arch_exit(void)
 #endif
        kvm_lapic_exit();
 
-       if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
+       if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) {
                cpufreq_unregister_notifier(&kvmclock_cpufreq_notifier_block,
                                            CPUFREQ_TRANSITION_NOTIFIER);
-       cpuhp_remove_state_nocalls(CPUHP_AP_X86_KVM_CLK_ONLINE);
+               cpuhp_remove_state_nocalls(CPUHP_AP_X86_KVM_CLK_ONLINE);
+       }
 #ifdef CONFIG_X86_64
        pvclock_gtod_unregister_notifier(&pvclock_gtod_notifier);
        irq_work_sync(&pvclock_irq_work);



>  	return 0;
>  }
>  
> -- 
> 2.36.0.550.gb090851708-goog
>