| Message ID | 20200917111306.819263-1-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/i386: always create kvmclock device | expand |
Patchew URL: https://patchew.org/QEMU/20200917111306.819263-1-vkuznets@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: N/A. Internal error while reading log file The full log is available at http://patchew.org/logs/20200917111306.819263-1-vkuznets@redhat.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
no-reply@patchew.org writes: > Patchew URL: https://patchew.org/QEMU/20200917111306.819263-1-vkuznets@redhat.com/ > > > > Hi, > > This series seems to have some coding style problems. See output below for > more information: > > N/A. Internal error while reading log file error: copy-fd: write returned No space left on device fatal: failed to copy file to '/var/tmp/patchew-tester-tmp-h4pn40by/src/.git/objects/pack/pack-614a2d933aa3d31ccd9f7bddf2f28bf8fae5e07c.pack': No space left on device fatal: The remote end hung up unexpectedly does not look like something I can fix. No idea why 'no space left' is called a 'coding style problem' btw :-)
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.damhet@blade-group.com> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Vitaly Kuznetsov <vkuznets@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 ? > 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 > >
Antoine Damhet <antoine.damhet@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.damhet@blade-group.com> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@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 :-) > >> 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 Kuznetsov (vkuznets@redhat.com) wrote: > Antoine Damhet <antoine.damhet@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.damhet@blade-group.com> > >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@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. 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? 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 >
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > * Vitaly Kuznetsov (vkuznets@redhat.com) wrote: >> Antoine Damhet <antoine.damhet@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.damhet@blade-group.com> >> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@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 >>
On Thu, Sep 17, 2020 at 03:42:37PM +0100, Dr. David Alan Gilbert wrote: [...] > > > > > > 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. > > 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? I guess that if the migration succeed (and the VMState keeps it's initial value) the timestamp will be restored to 0 which is the current behavior. > > Dave [...]
* Vitaly Kuznetsov (vkuznets@redhat.com) wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > > > * Vitaly Kuznetsov (vkuznets@redhat.com) wrote: > >> Antoine Damhet <antoine.damhet@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.damhet@blade-group.com> > >> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@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 >
On 17/09/20 13:13, 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.damhet@blade-group.com> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Vitaly Kuznetsov <vkuznets@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()) { > 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); > Queued, thanks. Paolo
On 17/09/20 19:44, Dr. David Alan Gilbert wrote: >> 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) > Right -- unqueued it :) Paolo
On Thu, Sep 17, 2020 at 06:44:10PM +0100, Dr. David Alan Gilbert wrote: [...] > > >> > > > >> > 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) So, I tried the patch and: # patched -> patched Everything working as expected # patched -> unpatched Migration failure with: ``` Unknown savevm section or instance 'kvmclock' 0. Make sure that your current VM setup matches your saved VM setup, including any hotplugged devices load of migration failed: Invalid argument ``` # unpatched -> patched The guest hangs upon arrival, I don't know which value is restored but something is restored (and far enough from 0 to confuse Windows). > > > > 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 [...]
* Antoine Damhet (antoine.damhet@blade-group.com) wrote: > On Thu, Sep 17, 2020 at 06:44:10PM +0100, Dr. David Alan Gilbert wrote: > > [...] > > > > >> > > > > >> > 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) > > So, I tried the patch and: > > # patched -> patched > > Everything working as expected > > # patched -> unpatched > > Migration failure with: > > ``` > Unknown savevm section or instance 'kvmclock' 0. Make sure that your current VM setup matches your saved VM setup, including any hotplugged devices > load of migration failed: Invalid argument > ``` Right, that's what I expected and said we need to wire this fix to the machine type. Dave > > # unpatched -> patched > > The guest hangs upon arrival, I don't know which value is restored but > something is restored (and far enough from 0 to confuse Windows). > > > > > > > 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 > > [...] > > -- > Antoine 'xdbob' Damhet
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > * Antoine Damhet (antoine.damhet@blade-group.com) wrote: >> On Thu, Sep 17, 2020 at 06:44:10PM +0100, Dr. David Alan Gilbert wrote: >> >> [...] >> >> > > >> > >> > > >> > 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) >> >> So, I tried the patch and: >> >> # patched -> patched >> >> Everything working as expected >> >> # patched -> unpatched >> >> Migration failure with: >> >> ``` >> Unknown savevm section or instance 'kvmclock' 0. Make sure that your current VM setup matches your saved VM setup, including any hotplugged devices >> load of migration failed: Invalid argument >> ``` > > Right, that's what I expected and said we need to wire this fix to the > machine type. > v2 with the idea implemented is coming. As I'm not a regular contributor to machine types, please review thoroughly :-)
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()) { 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);
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.damhet@blade-group.com> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Vitaly Kuznetsov <vkuznets@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(-)