Sean Christopherson <seanjc@xxxxxxxxxx> writes:
> On Tue, Apr 19, 2022, Vitaly Kuznetsov wrote:
>> Sean Christopherson <seanjc@xxxxxxxxxx> 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
