"Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
> * Vitaly Kuznetsov (vkuzn...@redhat.com) wrote:
>> Antoine Damhet <antoine.dam...@blade-group.com> writes:
>>
>> > On Thu, Sep 17, 2020 at 01:13:06PM +0200, Vitaly Kuznetsov wrote:
>> >> QEMU's kvmclock device is only created when KVM PV feature bits for
>> >> kvmclock (KVM_FEATURE_CLOCKSOURCE/KVM_FEATURE_CLOCKSOURCE2) are
>> >> exposed to the guest. With 'kvm=off' cpu flag the device is not
>> >> created and we don't call KVM_GET_CLOCK/KVM_SET_CLOCK upon migration.
>> >> It was reported that without these call at least Hyper-V TSC page
>> >> clocksouce (which can be enabled independently) gets broken after
>> >> migration.
>> >>
>> >> Switch to creating kvmclock QEMU device unconditionally, it seems
>> >> to always make sense to call KVM_GET_CLOCK/KVM_SET_CLOCK on migration.
>> >> Use KVM_CAP_ADJUST_CLOCK check instead of CPUID feature bits.
>> >>
>> >> Reported-by: Antoine Damhet <antoine.dam...@blade-group.com>
>> >> Suggested-by: Paolo Bonzini <pbonz...@redhat.com>
>> >> Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
>> >> ---
>> >> hw/i386/kvm/clock.c | 6 +-----
>> >> target/i386/kvm.c | 5 +++++
>> >> target/i386/kvm_i386.h | 1 +
>> >> 3 files changed, 7 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
>> >> index 64283358f91d..526c9ea5172b 100644
>> >> --- a/hw/i386/kvm/clock.c
>> >> +++ b/hw/i386/kvm/clock.c
>> >> @@ -330,11 +330,7 @@ static const TypeInfo kvmclock_info = {
>> >> /* Note: Must be called after VCPU initialization. */
>> >> void kvmclock_create(void)
>> >> {
>> >> - X86CPU *cpu = X86_CPU(first_cpu);
>> >> -
>> >> - if (kvm_enabled() &&
>> >> - cpu->env.features[FEAT_KVM] & ((1ULL << KVM_FEATURE_CLOCKSOURCE)
>> >> |
>> >> - (1ULL <<
>> >> KVM_FEATURE_CLOCKSOURCE2))) {
>> >> + if (kvm_enabled() && kvm_has_adjust_clock()) {
>> >
>> > Shouldn't the old check used when machine type <= 5.1 in order to avoid
>> > migration incompatibility ?
>>
>> Hm, when the check fails we just don't create the device and no error is
>> reported, so even if we have kvmclock data in the migration stream but
>> fail to create it migration will still succeed, right? (not a migration
>> expert here :-)
>
> When the migration stream is parsed, it'll try and find a "kvmclock"
> device to pass the data it's reading to; if one doesn't exist it'll
> fail.
This may happen with an older machine type when the destination is
running an unfixed QEMU and the source has the fix, right? The solution
would be to introduce a flag for older machine types (or for new ones)
like 'kvmclock_always'.
>
> The other question is in the incoming direction from an older VM;
> you'll have a kvm clock created here, but you won't load the kvm clock
> state from the migration stream - what is this clock going to do?
This is not really a problem I believe: the clock was absent on the
source and things somehow worked for the guest so even if we don't
initialize kvmclock properly on the destination nothing bad is expected.
>
> Dave
>
>> >
>> >> sysbus_create_simple(TYPE_KVM_CLOCK, -1, NULL);
>> >> }
>> >> }
>> >> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> >> index 4a8b3a41c1bc..20b31b65307b 100644
>> >> --- a/target/i386/kvm.c
>> >> +++ b/target/i386/kvm.c
>> >> @@ -143,6 +143,11 @@ bool kvm_has_adjust_clock_stable(void)
>> >> return (ret == KVM_CLOCK_TSC_STABLE);
>> >> }
>> >>
>> >> +bool kvm_has_adjust_clock(void)
>> >> +{
>> >> + return kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK);
>> >> +}
>> >> +
>> >> bool kvm_has_exception_payload(void)
>> >> {
>> >> return has_exception_payload;
>> >> diff --git a/target/i386/kvm_i386.h b/target/i386/kvm_i386.h
>> >> index 064b8798a26c..0fce4e51d2d6 100644
>> >> --- a/target/i386/kvm_i386.h
>> >> +++ b/target/i386/kvm_i386.h
>> >> @@ -34,6 +34,7 @@
>> >>
>> >> bool kvm_allows_irq0_override(void);
>> >> bool kvm_has_smm(void);
>> >> +bool kvm_has_adjust_clock(void);
>> >> bool kvm_has_adjust_clock_stable(void);
>> >> bool kvm_has_exception_payload(void);
>> >> void kvm_synchronize_all_tsc(void);
>> >> --
>> >> 2.25.4
>> >>
>> >>
>>
>> --
>> Vitaly
>>
--
Vitaly
