* Vitaly Kuznetsov (vkuzn...@redhat.com) wrote: > "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?
Yes I think so. > The solution > would be to introduce a flag for older machine types (or for new ones) > like 'kvmclock_always'. Yep sounds the normal answer. (You might want to try it first to trigger the bug) > > 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. OK. Dave > > > > 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 > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK