[PATCH] target/i386: always create kvmclock device

[PATCH] target/i386: always create kvmclock device

[Vitaly Kuznetsov]

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);
-- 
2.25.4

Re: [PATCH] target/i386: always create kvmclock device

[no-reply]

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

Re: [PATCH] target/i386: always create kvmclock device

[Vitaly Kuznetsov]

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 :-)

-- 
Vitaly

Re: [PATCH] target/i386: always create kvmclock device

[Antoine Damhet]

[-- Attachment #1: Type: text/plain, Size: 2814 bytes --]

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 'xdbob' Damhet

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] target/i386: always create kvmclock device

[Vitaly Kuznetsov]

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

Re: [PATCH] target/i386: always create kvmclock device

[Dr. David Alan Gilbert]

* 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 / Manchester, UK

Re: [PATCH] target/i386: always create kvmclock device

[Vitaly Kuznetsov]

"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
>> 

-- 
Vitaly

Re: [PATCH] target/i386: always create kvmclock device

[Dr. David Alan Gilbert]

* 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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] target/i386: always create kvmclock device

[Paolo Bonzini]

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

Re: [PATCH] target/i386: always create kvmclock device

[Antoine Damhet]

[-- Attachment #1: Type: text/plain, Size: 2077 bytes --]

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 'xdbob' Damhet

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] target/i386: always create kvmclock device

[Dr. David Alan Gilbert]

* 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 / Manchester, UK

Re: [PATCH] target/i386: always create kvmclock device

[Vitaly Kuznetsov]

"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 :-)

-- 
Vitaly

Re: [PATCH] target/i386: always create kvmclock device

[Antoine Damhet]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

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


[...]

-- 
Antoine 'xdbob' Damhet

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] target/i386: always create kvmclock device

[Paolo Bonzini]

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