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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Sean Christopherson <seanjc@google.com> writes:

> On Tue, Apr 19, 2022, Vitaly Kuznetsov wrote:
>> Sean Christopherson <seanjc@google.com> writes:
>> 
>> > +Vitaly
>> >
>> > On Thu, Apr 14, 2022, Anton Romanov wrote:
>> 
>> ...
>> 
>> >> @@ -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);
>> >
>> 
>> (apologies for the delayed reply)
>> 
>> No, I think Hyper-V's "Reenlightenment" feature overrides (re-defines?)
>> X86_FEATURE_CONSTANT_TSC. E.g. I've checked a VM on E5-2667 v4
>> (Broadwell) CPU with no TSC scaling. This VM has 'constant_tsc' and will
>> certainly get reenlightenment irq on migration.
>
> Ooh, so that a VM with a constant TSC be live migrated to another system with a
> constant, but different, TSC.  Does the below look correct as fixup for this patch?
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ab336f7c82e4..a944e4ba5532 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8708,10 +8708,18 @@ static void kvm_hyperv_tsc_notifier(void)
>         /* no guest entries from this point */
>         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;
> -       kvm_max_guest_tsc_khz = tsc_khz;
> +       /*
> +        * TSC frequency always matches when on Hyper-V.  Skip the updates if
> +        * the TSC is "officially" constant, in which case KVM doesn't use the
> +        * per-CPU and max variables.  Note, the notifier can still fire with
> +        * a constant TSC, e.g. if this VM (KVM is a Hyper-V guest) is migrated
> +        * to a system with a different TSC frequency.
> +        */
> +       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;
> +       }

Looks good for cpu_tsc_khz but I'm not particularly sure about
kvm_max_guest_tsc_khz.

AFAIU, kvm_max_guest_tsc_khz is normally only set when TSC scaling is
available (kvm_has_tsc_control) and Hyper-V wasn't exposing it as the
field was missing in Enlightened VMCS. The most recent version
(https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/datatypes/hv_vmx_enlightened_vmcs),
however, has 'TscMultiplier' so I guess it's possible now. (Note: eVMCS
version remains '1', just a few fields were added, interesting).

Another thing is why do we set kvm_max_guest_tsc_khz to 'tsc_khz' as
normally, this is the maximum possible guest-VM TSC frequency (see
kvm_arch_hardware_setup()). With reenlightenment, the VM can be migrated
to a host with different TSC frequency, this means the maximum possible
guest-VM TSC frequency may change. What do we do with L2 VM with
'unsupported' configurations after that?

TL;DR: I *think* we can drop kvm_max_guest_tsc_khz setting from
kvm_hyperv_tsc_notifier() for now as it a) doesn't seem to be needed for
non-tscscaling case and b) correct for the tsc-scaling case. I'll need
to investigate how recent Hyper-V versions work when CPU offers
TSC-scaling feature.

>
>         list_for_each_entry(kvm, &vm_list, vm_list) {
>                 __kvm_start_pvclock_update(kvm);
>

-- 
Vitaly