improve kvmclock difference on migration

Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which indicates that KVM_GET_CLOCK returns a value as seen by the guest at that moment.

For new machine types, use this value rather than reading from guest memory.

This reduces kvmclock difference on migration from 5s to 0.1s (when max_downtime == 5s).

Signed-off-by Marcelo Tosatti <[email protected]>

---
 hw/i386/kvm/clock.c    |  108 ++++++++++++++++++++++++++++++++++++++++++-------
 include/hw/i386/pc.h   |    5 ++
 target-i386/kvm.c      |    7 +++
 target-i386/kvm_i386.h |    1 
 4 files changed, 107 insertions(+), 14 deletions(-)

v2:
- improve variable names (Juan)
- consolidate code on kvm_get_clock function (Paolo)
- return mach_use_reliable_get_clock from needed function (Paolo)

Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================

hw/i386/kvm/clock.c +30 -2

@@ -36,6 +36,12 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -81,6 +87,19 @@
     return nsec + time.system_time;
 }
 
+static uint64_t kvm_get_clock(void)
+{
+    struct kvm_clock_data data;
+    int ret;
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+                abort();
+    }
+    return data.clock;
+}
+
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {

@@ -91,15 +110,37 @@
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);
+        uint64_t pvclock_via_mem = 0;
 
-        s->clock_valid = false;
+        /* local (running VM) restore */
+        if (s->clock_valid) {
+            /*
+             * if host does not support reliable KVM_GET_CLOCK,
+             * read kvmclock value from memory
+             */
+            if (!kvm_has_adjust_clock_stable()) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }
+        /* migration/savevm/init restore */

hw/i386/kvm/clock.c +10 -12

+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
-        if (time_at_migration) {
-            s->clock = time_at_migration;
+        /* We can't rely on the saved clock value, just discard it */
+        if (pvclock_via_mem) {
+            s->clock = pvclock_via_mem;
         }
 
+        s->clock_valid = false;
+
         data.clock = s->clock;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {

@@ -120,8 +161,6 @@
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
         if (s->clock_valid) {
             return;

@@ -129,13 +168,7 @@
 
         kvm_synchronize_all_tsc();
 
-        ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
-        if (ret < 0) {
-            fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
-            abort();
-        }
-        s->clock = data.clock;
-
+        s->clock = kvm_get_clock();
         /*
          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return

@@ -152,22 +185,69 @@
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;

hw/i386/kvm/clock.c +21 -0

+}
+
+static const VMStateDescription kvmclock_reliable_get_clock = {
+    .name = "kvmclock/src_use_reliable_get_clock",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = kvmclock_src_use_reliable_get_clock,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void kvmclock_pre_save(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    s->clock = kvm_get_clock();
+}
+
 static const VMStateDescription kvmclock_vmsd = {
     .name = "kvmclock",
     .version_id = 1,
     .minimum_version_id = 1,
+    .pre_save = kvmclock_pre_save,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(clock, KVMClockState),
         VMSTATE_END_OF_LIST()

hw/i386/kvm/clock.c +4 -0

+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void kvmclock_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = kvmclock_realize;
     dc->vmsd = &kvmclock_vmsd;
+    dc->props = kvmclock_properties;
 }
 
 static const TypeInfo kvmclock_info = {

Index: qemu-mig-advance-clock/include/hw/i386/pc.h
===================================================================

include/hw/i386/pc.h +5 -0

@@ -370,6 +370,11 @@
 #define PC_COMPAT_2_7 \
     HW_COMPAT_2_7 \
     {\
+        .driver   = "kvmclock",\
+        .property = "mach_use_reliable_get_clock",\
+        .value    = "off",\
+    },\
+    {\
         .driver   = TYPE_X86_CPU,\
         .property = "l3-cache",\
         .value    = "off",\

Index: qemu-mig-advance-clock/target-i386/kvm.c
===================================================================

target-i386/kvm.c +7 -0

@@ -117,6 +117,13 @@
     return kvm_check_extension(kvm_state, KVM_CAP_X86_SMM);
 }
 
+bool kvm_has_adjust_clock_stable(void)
+{
+    int ret = kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK);
+
+    return (ret == KVM_CLOCK_TSC_STABLE);
+}
+
 bool kvm_allows_irq0_override(void)
 {
     return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();

Index: qemu-mig-advance-clock/target-i386/kvm_i386.h
===================================================================

target-i386/kvm_i386.h +1 -0

@@ -17,6 +17,7 @@
 
 bool kvm_allows_irq0_override(void);
 bool kvm_has_smm(void);
+bool kvm_has_adjust_clock_stable(void);
 void kvm_synchronize_all_tsc(void);
 void kvm_arch_reset_vcpu(X86CPU *cs);
 void kvm_arch_do_init_vcpu(X86CPU *cs);

Quoted 79 lines · Marcelo Tosatti

Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which indicates that KVM_GET_CLOCK returns a value as seen by the guest at that moment.

For new machine types, use this value rather than reading from guest memory.

This reduces kvmclock difference on migration from 5s to 0.1s (when max_downtime == 5s).

Signed-off-by Marcelo Tosatti <[email protected]>

---
 hw/i386/kvm/clock.c    |  108 ++++++++++++++++++++++++++++++++++++++++++-------
 include/hw/i386/pc.h   |    5 ++
 target-i386/kvm.c      |    7 +++
 target-i386/kvm_i386.h |    1 
 4 files changed, 107 insertions(+), 14 deletions(-)

v2:
- improve variable names (Juan)
- consolidate code on kvm_get_clock function (Paolo)
- return mach_use_reliable_get_clock from needed function (Paolo)

Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================

hw/i386/kvm/clock.c +30 -2

@@ -36,6 +36,12 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -81,6 +87,19 @@
     return nsec + time.system_time;
 }
 
+static uint64_t kvm_get_clock(void)
+{
+    struct kvm_clock_data data;
+    int ret;
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+                abort();
+    }
+    return data.clock;
+}
+
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {

@@ -91,15 +110,37 @@
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);
+        uint64_t pvclock_via_mem = 0;
 
-        s->clock_valid = false;
+        /* local (running VM) restore */
+        if (s->clock_valid) {
+            /*
+             * if host does not support reliable KVM_GET_CLOCK,
+             * read kvmclock value from memory
+             */
+            if (!kvm_has_adjust_clock_stable()) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }
+        /* migration/savevm/init restore */

hw/i386/kvm/clock.c line 134

Quoted 10 lines

hw/i386/kvm/clock.c +10 -0

+        } else {
+            /*
+             * use s->clock in case machine uses reliable
+             * get clock and source host supported
+             * reliable get clock
+             */
+            if (!(s->mach_use_reliable_get_clock &&
+                  s->src_use_reliable_get_clock)) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }

The s->mach_use_reliable_get_clock check seems redundant. src_use_reliable_get_clock is set only if mach_use_reliable_get_clock is true.

Quoted 46 lines

hw/i386/kvm/clock.c +10 -12

+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
-        if (time_at_migration) {
-            s->clock = time_at_migration;
+        /* We can't rely on the saved clock value, just discard it */
+        if (pvclock_via_mem) {
+            s->clock = pvclock_via_mem;
         }
 
+        s->clock_valid = false;
+
         data.clock = s->clock;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {

@@ -120,8 +161,6 @@
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
         if (s->clock_valid) {
             return;

@@ -129,13 +168,7 @@
 
         kvm_synchronize_all_tsc();
 
-        ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
-        if (ret < 0) {
-            fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
-            abort();
-        }
-        s->clock = data.clock;
-
+        s->clock = kvm_get_clock();
         /*
          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return

@@ -152,22 +185,69 @@
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;

hw/i386/kvm/clock.c line 200

Quoted 10 lines

hw/i386/kvm/clock.c +10 -0

+
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,
+     * if host kernel does provide reliable KVM_GET_CLOCK,
+     * set src_use_reliable_get_clock=true so that destination
+     * avoids reading kvmclock from memory.
+     */
+    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
+        s->src_use_reliable_get_clock = true;
+    }

It feels fragile to change device state inside the .needed function. Better to initialize src_use_reliable_get_clock on kvmclock_realize()?

What exactly ensures src_use_reliable_get_clock is correctly initialized on the migration destination as well?

hw/i386/kvm/clock.c line 202

Quoted 2 lines

hw/i386/kvm/clock.c +2 -0

+
+    return s->mach_use_reliable_get_clock;

If if kvm_has_adjust_clock_stable() is false, isn't it simpler to simply skip the section?

Quoted 147 lines · Eduardo Habkost

Quoted 79 lines · Marcelo Tosatti

Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which indicates that KVM_GET_CLOCK returns a value as seen by the guest at that moment.

For new machine types, use this value rather than reading from guest memory.

This reduces kvmclock difference on migration from 5s to 0.1s (when max_downtime == 5s).

Signed-off-by Marcelo Tosatti <[email protected]>

---
 hw/i386/kvm/clock.c    |  108 ++++++++++++++++++++++++++++++++++++++++++-------
 include/hw/i386/pc.h   |    5 ++
 target-i386/kvm.c      |    7 +++
 target-i386/kvm_i386.h |    1 
 4 files changed, 107 insertions(+), 14 deletions(-)

v2:
- improve variable names (Juan)
- consolidate code on kvm_get_clock function (Paolo)
- return mach_use_reliable_get_clock from needed function (Paolo)

Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================

hw/i386/kvm/clock.c +30 -2

@@ -36,6 +36,12 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -81,6 +87,19 @@
     return nsec + time.system_time;
 }
 
+static uint64_t kvm_get_clock(void)
+{
+    struct kvm_clock_data data;
+    int ret;
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+                abort();
+    }
+    return data.clock;
+}
+
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {

@@ -91,15 +110,37 @@
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);
+        uint64_t pvclock_via_mem = 0;
 
-        s->clock_valid = false;
+        /* local (running VM) restore */
+        if (s->clock_valid) {
+            /*
+             * if host does not support reliable KVM_GET_CLOCK,
+             * read kvmclock value from memory
+             */
+            if (!kvm_has_adjust_clock_stable()) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }
+        /* migration/savevm/init restore */

hw/i386/kvm/clock.c line 134

Quoted 10 lines

hw/i386/kvm/clock.c +10 -0

+        } else {
+            /*
+             * use s->clock in case machine uses reliable
+             * get clock and source host supported
+             * reliable get clock
+             */
+            if (!(s->mach_use_reliable_get_clock &&
+                  s->src_use_reliable_get_clock)) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }

The s->mach_use_reliable_get_clock check seems redundant. src_use_reliable_get_clock is set only if mach_use_reliable_get_clock is true.

Quoted 52 lines

hw/i386/kvm/clock.c +16 -12

+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
-        if (time_at_migration) {
-            s->clock = time_at_migration;
+        /* We can't rely on the saved clock value, just discard it */
+        if (pvclock_via_mem) {
+            s->clock = pvclock_via_mem;
         }
 
+        s->clock_valid = false;
+
         data.clock = s->clock;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {

@@ -120,8 +161,6 @@
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
         if (s->clock_valid) {
             return;

@@ -129,13 +168,7 @@
 
         kvm_synchronize_all_tsc();
 
-        ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
-        if (ret < 0) {
-            fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
-            abort();
-        }
-        s->clock = data.clock;
-
+        s->clock = kvm_get_clock();
         /*
          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return

@@ -152,22 +185,69 @@
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,
+     * if host kernel does provide reliable KVM_GET_CLOCK,
+     * set src_use_reliable_get_clock=true so that destination
+     * avoids reading kvmclock from memory.

Quoted 8 lines

hw/i386/kvm/clock.c line 200

Quoted 4 lines

hw/i386/kvm/clock.c +4 -0

+     */
+    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
+        s->src_use_reliable_get_clock = true;
+    }

It feels fragile to change device state inside the .needed function. Better to initialize src_use_reliable_get_clock on kvmclock_realize()?

Quoted 8 lines

What exactly ensures src_use_reliable_get_clock is correctly initialized on the migration destination as well?

hw/i386/kvm/clock.c line 202

Quoted 2 lines

hw/i386/kvm/clock.c +2 -0

+
+    return s->mach_use_reliable_get_clock;

If if kvm_has_adjust_clock_stable() is false, isn't it simpler to simply skip the section?

This is what I asked for. :)

However, I was proposing a different way to initialize src_use_reliable_get_clock. I still have to understand exactly how Marcelo's algorithm works because (based on the kvmclock code) it's more trick than it seems.

Paolo

Quoted 11 lines

hw/i386/kvm/clock.c line 203

Quoted 9 lines

hw/i386/kvm/clock.c +8 -0

+     * avoids reading kvmclock from memory.
+     */
+    if (s->mach_use_reliable_get_clock &&
 kvm_has_adjust_clock_stable()) {
+        s->src_use_reliable_get_clock = true;
+    }
+
+    return s->src_use_reliable_get_clock;
+}

Here you can just return s->mach_use_reliable_get_clock.

mach_use_reliable_get_clock can be true but host might not support it.

Quoted 2 lines · Paolo Bonzini

Quoted 1 line · Eduardo Habkost

On Thu, Nov 17, 2016 at 11:24:13AM -0200, Marcelo Tosatti wrote:

Quoted 9 lines

Quoted 8 lines

What exactly ensures src_use_reliable_get_clock is correctly initialized on the migration destination as well?

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+    return s->mach_use_reliable_get_clock;

If if kvm_has_adjust_clock_stable() is false, isn't it simpler to simply skip the section?

Quoted 8 lines

This is what I asked for. :)

However, I was proposing a different way to initialize src_use_reliable_get_clock. I still have to understand exactly how Marcelo's algorithm works because (based on the kvmclock code) it's more trick than it seems.

Paolo

You asked me to return s->mach_use_reliable_get_clock:

Quoted 12 lines

Quoted 11 lines

hw/i386/kvm/clock.c line 203

Quoted 9 lines

hw/i386/kvm/clock.c +3 -0

+     * avoids reading kvmclock from memory.
+     */
+    if (s->mach_use_reliable_get_clock &&

kvm_has_adjust_clock_stable()) {

hw/i386/kvm/clock.c +5 -0

+        s->src_use_reliable_get_clock = true;
+    }
+
+    return s->src_use_reliable_get_clock;
+}

Here you can just return s->mach_use_reliable_get_clock.

Quoted 1 line

mach_use_reliable_get_clock can be true but host might not support it.

Yes, but the "needed" function is only required to avoid breaking pc-i440fx-2.7 and earlier. If you return true here, you can still migrate a "false" value for src_use_reliable_get_clock.

===================

I don't see why avoid the subsection, since new machine type is incompatible anyway. So Eduardo on your suggestion to skip sending the subsection, what is the advantage?

Quoted 85 lines · Marcelo Tosatti

Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which indicates that KVM_GET_CLOCK returns a value as seen by the guest at that moment.

For new machine types, use this value rather than reading from guest memory.

This reduces kvmclock difference on migration from 5s to 0.1s (when max_downtime == 5s).

Signed-off-by Marcelo Tosatti <[email protected]>

---
 hw/i386/kvm/clock.c    |  108 ++++++++++++++++++++++++++++++++++++++++++-------
 include/hw/i386/pc.h   |    5 ++
 target-i386/kvm.c      |    7 +++
 target-i386/kvm_i386.h |    1 
 4 files changed, 107 insertions(+), 14 deletions(-)

v2:
- improve variable names (Juan)
- consolidate code on kvm_get_clock function (Paolo)
- return mach_use_reliable_get_clock from needed function (Paolo)

Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================

hw/i386/kvm/clock.c +36 -2

@@ -36,6 +36,12 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -81,6 +87,19 @@
     return nsec + time.system_time;
 }
 
+static uint64_t kvm_get_clock(void)
+{
+    struct kvm_clock_data data;
+    int ret;
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+                abort();
+    }
+    return data.clock;
+}
+
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {

@@ -91,15 +110,37 @@
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);
+        uint64_t pvclock_via_mem = 0;
 
-        s->clock_valid = false;
+        /* local (running VM) restore */
+        if (s->clock_valid) {
+            /*
+             * if host does not support reliable KVM_GET_CLOCK,
+             * read kvmclock value from memory
+             */
+            if (!kvm_has_adjust_clock_stable()) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }
+        /* migration/savevm/init restore */
+        } else {
+            /*
+             * use s->clock in case machine uses reliable
+             * get clock and source host supported
+             * reliable get clock
+             */

hw/i386/kvm/clock.c line 134

Quoted 4 lines

hw/i386/kvm/clock.c +4 -0

+            if (!(s->mach_use_reliable_get_clock &&
+                  s->src_use_reliable_get_clock)) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }

The s->mach_use_reliable_get_clock check seems redundant. src_use_reliable_get_clock is set only if mach_use_reliable_get_clock is true.

Quoted 46 lines

hw/i386/kvm/clock.c +10 -12

+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
-        if (time_at_migration) {
-            s->clock = time_at_migration;
+        /* We can't rely on the saved clock value, just discard it */
+        if (pvclock_via_mem) {
+            s->clock = pvclock_via_mem;
         }
 
+        s->clock_valid = false;
+
         data.clock = s->clock;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {

@@ -120,8 +161,6 @@
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
         if (s->clock_valid) {
             return;

@@ -129,13 +168,7 @@
 
         kvm_synchronize_all_tsc();
 
-        ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
-        if (ret < 0) {
-            fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
-            abort();
-        }
-        s->clock = data.clock;
-
+        s->clock = kvm_get_clock();
         /*
          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return

@@ -152,22 +185,69 @@
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;

hw/i386/kvm/clock.c line 200

Quoted 10 lines

hw/i386/kvm/clock.c +10 -0

+
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,
+     * if host kernel does provide reliable KVM_GET_CLOCK,
+     * set src_use_reliable_get_clock=true so that destination
+     * avoids reading kvmclock from memory.
+     */
+    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
+        s->src_use_reliable_get_clock = true;
+    }

It feels fragile to change device state inside the .needed function. Better to initialize src_use_reliable_get_clock on kvmclock_realize()?

What exactly ensures src_use_reliable_get_clock is correctly initialized on the migration destination as well?

hw/i386/kvm/clock.c line 202

Quoted 3 lines

hw/i386/kvm/clock.c +2 -0

+
+    return s->mach_use_reliable_get_clock;

If if kvm_has_adjust_clock_stable() is false, isn't it simpler to simply skip the section?

It looks like mach_use_reliable_get_clock and src_use_reliable_get_clock could become a single field:

* use_reliable_get_clock ("x-use-reliable-get-clock" property) set to true by default (set on DEFINE_PROP_BOOL parameter)
* "x-use-reliable-get-clock" set to false by default on older machine-types
* use_reliable_get_clock forced to false on kvmclock_realize() if !kvm_has_adjust_clock_stable()
* kvmclock_reliable_get_clock.needed return s->use_reliable_get_clock

Quoted 29 lines

hw/i386/kvm/clock.c +21 -0

+}
+
+static const VMStateDescription kvmclock_reliable_get_clock = {
+    .name = "kvmclock/src_use_reliable_get_clock",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = kvmclock_src_use_reliable_get_clock,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void kvmclock_pre_save(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    s->clock = kvm_get_clock();
+}
+
 static const VMStateDescription kvmclock_vmsd = {
     .name = "kvmclock",
     .version_id = 1,
     .minimum_version_id = 1,
+    .pre_save = kvmclock_pre_save,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(clock, KVMClockState),
         VMSTATE_END_OF_LIST()

hw/i386/kvm/clock.c line 240

Quoted 10 lines

hw/i386/kvm/clock.c +7 -0

+    },
+    .subsections = (const VMStateDescription * []) {
+        &kvmclock_reliable_get_clock,
+        NULL
     }
 };
 
+static Property kvmclock_properties[] = {
+    DEFINE_PROP_BOOL("mach_use_reliable_get_clock", KVMClockState,
+                      mach_use_reliable_get_clock, true),

QOM property coding style is to use "-" instead of "_". Also, please prefix it with "x-" to indicate it is not part of QEMU stable external interface.

Quoted 53 lines

hw/i386/kvm/clock.c +4 -0

+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void kvmclock_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = kvmclock_realize;
     dc->vmsd = &kvmclock_vmsd;
+    dc->props = kvmclock_properties;
 }
 
 static const TypeInfo kvmclock_info = {

Index: qemu-mig-advance-clock/include/hw/i386/pc.h
===================================================================

include/hw/i386/pc.h +5 -0

@@ -370,6 +370,11 @@
 #define PC_COMPAT_2_7 \
     HW_COMPAT_2_7 \
     {\
+        .driver   = "kvmclock",\
+        .property = "mach_use_reliable_get_clock",\
+        .value    = "off",\
+    },\
+    {\
         .driver   = TYPE_X86_CPU,\
         .property = "l3-cache",\
         .value    = "off",\

Index: qemu-mig-advance-clock/target-i386/kvm.c
===================================================================

target-i386/kvm.c +7 -0

@@ -117,6 +117,13 @@
     return kvm_check_extension(kvm_state, KVM_CAP_X86_SMM);
 }
 
+bool kvm_has_adjust_clock_stable(void)
+{
+    int ret = kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK);
+
+    return (ret == KVM_CLOCK_TSC_STABLE);
+}
+
 bool kvm_allows_irq0_override(void)
 {
     return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();

Index: qemu-mig-advance-clock/target-i386/kvm_i386.h
===================================================================

target-i386/kvm_i386.h +0 -0

@@ -17,6 +17,7 @@
 

Quoted 8 lines

target-i386/kvm_i386.h +1 -0

 bool kvm_allows_irq0_override(void);
 bool kvm_has_smm(void);
+bool kvm_has_adjust_clock_stable(void);
 void kvm_synchronize_all_tsc(void);
 void kvm_arch_reset_vcpu(X86CPU *cs);
 void kvm_arch_do_init_vcpu(X86CPU *cs);

Signature · 1 line

Eduardo

Quoted 86 lines · Eduardo Habkost

Quoted 85 lines · Marcelo Tosatti

Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which indicates that KVM_GET_CLOCK returns a value as seen by the guest at that moment.

For new machine types, use this value rather than reading from guest memory.

This reduces kvmclock difference on migration from 5s to 0.1s (when max_downtime == 5s).

Signed-off-by Marcelo Tosatti <[email protected]>

---
 hw/i386/kvm/clock.c    |  108 ++++++++++++++++++++++++++++++++++++++++++-------
 include/hw/i386/pc.h   |    5 ++
 target-i386/kvm.c      |    7 +++
 target-i386/kvm_i386.h |    1 
 4 files changed, 107 insertions(+), 14 deletions(-)

v2:
- improve variable names (Juan)
- consolidate code on kvm_get_clock function (Paolo)
- return mach_use_reliable_get_clock from needed function (Paolo)

Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================

hw/i386/kvm/clock.c +36 -2

@@ -36,6 +36,12 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -81,6 +87,19 @@
     return nsec + time.system_time;
 }
 
+static uint64_t kvm_get_clock(void)
+{
+    struct kvm_clock_data data;
+    int ret;
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+                abort();
+    }
+    return data.clock;
+}
+
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {

@@ -91,15 +110,37 @@
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);
+        uint64_t pvclock_via_mem = 0;
 
-        s->clock_valid = false;
+        /* local (running VM) restore */
+        if (s->clock_valid) {
+            /*
+             * if host does not support reliable KVM_GET_CLOCK,
+             * read kvmclock value from memory
+             */
+            if (!kvm_has_adjust_clock_stable()) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }
+        /* migration/savevm/init restore */
+        } else {
+            /*
+             * use s->clock in case machine uses reliable
+             * get clock and source host supported
+             * reliable get clock
+             */

Quoted 8 lines

hw/i386/kvm/clock.c line 123

Quoted 4 lines

hw/i386/kvm/clock.c +4 -0

+            if (!(s->mach_use_reliable_get_clock &&
+                  s->src_use_reliable_get_clock)) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }

The s->mach_use_reliable_get_clock check seems redundant. src_use_reliable_get_clock is set only if mach_use_reliable_get_clock is true.

Done.

Quoted 61 lines

Quoted 46 lines

hw/i386/kvm/clock.c +10 -12

+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
-        if (time_at_migration) {
-            s->clock = time_at_migration;
+        /* We can't rely on the saved clock value, just discard it */
+        if (pvclock_via_mem) {
+            s->clock = pvclock_via_mem;
         }
 
+        s->clock_valid = false;
+
         data.clock = s->clock;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {

@@ -120,8 +161,6 @@
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
         if (s->clock_valid) {
             return;

@@ -129,13 +168,7 @@
 
         kvm_synchronize_all_tsc();
 
-        ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
-        if (ret < 0) {
-            fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
-            abort();
-        }
-        s->clock = data.clock;
-
+        s->clock = kvm_get_clock();
         /*
          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return

@@ -152,22 +185,69 @@
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;

hw/i386/kvm/clock.c line 200

Quoted 10 lines

hw/i386/kvm/clock.c +10 -0

+
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,
+     * if host kernel does provide reliable KVM_GET_CLOCK,
+     * set src_use_reliable_get_clock=true so that destination
+     * avoids reading kvmclock from memory.
+     */
+    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
+        s->src_use_reliable_get_clock = true;
+    }

It feels fragile to change device state inside the .needed function. Better to initialize src_use_reliable_get_clock on kvmclock_realize()?

Quoted 2 lines

What exactly ensures src_use_reliable_get_clock is correctly initialized on the migration destination as well?

Its initialized to false (because its part of device state).

Quoted 11 lines

hw/i386/kvm/clock.c line 202

Quoted 4 lines

hw/i386/kvm/clock.c +2 -0

+
+    return s->mach_use_reliable_get_clock;

If if kvm_has_adjust_clock_stable() is false, isn't it simpler to simply skip the section?

It looks like mach_use_reliable_get_clock and src_use_reliable_get_clock could become a single field:

Quoted 7 lines

* use_reliable_get_clock ("x-use-reliable-get-clock" property) set to true by default (set on DEFINE_PROP_BOOL parameter)
* "x-use-reliable-get-clock" set to false by default on older machine-types
* use_reliable_get_clock forced to false on kvmclock_realize() if !kvm_has_adjust_clock_stable()
* kvmclock_reliable_get_clock.needed return s->use_reliable_get_clock

No because use_reliable_get_clock can be false because the destination host does not support kvm_has_adjust_clock_stable(), but the source host supports it (so we want to use the value in the first incoming migration).

So the variable indicates whether the source host supported migration, not whether the destination supports it.

How about the following:

Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================

hw/i386/kvm/clock.c +55 -14

@@ -36,6 +36,12 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -81,6 +87,19 @@
     return nsec + time.system_time;
 }
 
+static uint64_t kvm_get_clock(void)
+{
+    struct kvm_clock_data data;
+    int ret;
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+                abort();
+    }
+    return data.clock;
+}
+
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {

@@ -91,15 +110,36 @@
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);
+        uint64_t pvclock_via_mem = 0;
 
-        s->clock_valid = false;
+        /* local (running VM) restore */
+        if (s->clock_valid) {
+            /*
+             * if host does not support reliable KVM_GET_CLOCK,
+             * read kvmclock value from memory
+             */
+            if (!kvm_has_adjust_clock_stable()) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }
+        /* migration/savevm/init restore */
+        } else {
+            /*
+             * use s->clock in case machine uses reliable
+             * get clock and source host supported
+             * reliable get clock
+             */
+            if (!s->src_use_reliable_get_clock) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }
+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
-        if (time_at_migration) {
-            s->clock = time_at_migration;
+        /* We can't rely on the saved clock value, just discard it */
+        if (pvclock_via_mem) {
+            s->clock = pvclock_via_mem;
         }
 
+        s->clock_valid = false;
+
         data.clock = s->clock;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {

@@ -120,8 +160,6 @@
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
         if (s->clock_valid) {
             return;

@@ -129,13 +167,7 @@
 
         kvm_synchronize_all_tsc();
 
-        ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
-        if (ret < 0) {
-            fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
-            abort();
-        }
-        s->clock = data.clock;
-
+        s->clock = kvm_get_clock();
         /*
          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return

@@ -149,25 +181,72 @@
 {
     KVMClockState *s = KVM_CLOCK(dev);
 
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,
+     * if host kernel does provide reliable KVM_GET_CLOCK,
+     * set src_use_reliable_get_clock=true so that destination
+     * avoids reading kvmclock from memory.
+     */
+    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
+        s->src_use_reliable_get_clock = true;
+    }

hw/i386/kvm/clock.c +31 -0

+
+static const VMStateDescription kvmclock_reliable_get_clock = {
+    .name = "kvmclock/src_use_reliable_get_clock",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = kvmclock_src_use_reliable_get_clock,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void kvmclock_pre_save(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    s->clock = kvm_get_clock();
+}
+
 static const VMStateDescription kvmclock_vmsd = {
     .name = "kvmclock",
     .version_id = 1,
     .minimum_version_id = 1,
+    .pre_save = kvmclock_pre_save,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(clock, KVMClockState),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+        &kvmclock_reliable_get_clock,
+        NULL
     }
 };
 
+static Property kvmclock_properties[] = {
+    DEFINE_PROP_BOOL("x-mach-use-reliable-get-clock", KVMClockState,
+                      mach_use_reliable_get_clock, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void kvmclock_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = kvmclock_realize;
     dc->vmsd = &kvmclock_vmsd;
+    dc->props = kvmclock_properties;
 }
 
 static const TypeInfo kvmclock_info = {

Index: qemu-mig-advance-clock/include/hw/i386/pc.h
===================================================================

include/hw/i386/pc.h +5 -0

@@ -370,6 +370,11 @@
 #define PC_COMPAT_2_7 \
     HW_COMPAT_2_7 \
     {\
+        .driver   = "kvmclock",\
+        .property = "x-mach-use-reliable-get-clock",\
+        .value    = "off",\
+    },\
+    {\
         .driver   = TYPE_X86_CPU,\
         .property = "l3-cache",\
         .value    = "off",\

Index: qemu-mig-advance-clock/target-i386/kvm.c
===================================================================

target-i386/kvm.c +7 -0

@@ -117,6 +117,13 @@
     return kvm_check_extension(kvm_state, KVM_CAP_X86_SMM);
 }
 
+bool kvm_has_adjust_clock_stable(void)
+{
+    int ret = kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK);
+
+    return (ret == KVM_CLOCK_TSC_STABLE);
+}
+
 bool kvm_allows_irq0_override(void)
 {
     return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();

Index: qemu-mig-advance-clock/target-i386/kvm_i386.h
===================================================================

target-i386/kvm_i386.h +1 -0

@@ -17,6 +17,7 @@
 
 bool kvm_allows_irq0_override(void);
 bool kvm_has_smm(void);
+bool kvm_has_adjust_clock_stable(void);
 void kvm_synchronize_all_tsc(void);
 void kvm_arch_reset_vcpu(X86CPU *cs);
 void kvm_arch_do_init_vcpu(X86CPU *cs);

Quoted 159 lines · Marcelo Tosatti

Quoted 86 lines · Eduardo Habkost

Quoted 85 lines · Marcelo Tosatti

Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which indicates that KVM_GET_CLOCK returns a value as seen by the guest at that moment.

For new machine types, use this value rather than reading from guest memory.

This reduces kvmclock difference on migration from 5s to 0.1s (when max_downtime == 5s).

Signed-off-by Marcelo Tosatti <[email protected]>

---
 hw/i386/kvm/clock.c    |  108 ++++++++++++++++++++++++++++++++++++++++++-------
 include/hw/i386/pc.h   |    5 ++
 target-i386/kvm.c      |    7 +++
 target-i386/kvm_i386.h |    1 
 4 files changed, 107 insertions(+), 14 deletions(-)

v2:
- improve variable names (Juan)
- consolidate code on kvm_get_clock function (Paolo)
- return mach_use_reliable_get_clock from needed function (Paolo)

Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================

hw/i386/kvm/clock.c +36 -2

@@ -36,6 +36,12 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -81,6 +87,19 @@
     return nsec + time.system_time;
 }
 
+static uint64_t kvm_get_clock(void)
+{
+    struct kvm_clock_data data;
+    int ret;
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+                abort();
+    }
+    return data.clock;
+}
+
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {

@@ -91,15 +110,37 @@
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);
+        uint64_t pvclock_via_mem = 0;
 
-        s->clock_valid = false;
+        /* local (running VM) restore */
+        if (s->clock_valid) {
+            /*
+             * if host does not support reliable KVM_GET_CLOCK,
+             * read kvmclock value from memory
+             */
+            if (!kvm_has_adjust_clock_stable()) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }
+        /* migration/savevm/init restore */
+        } else {
+            /*
+             * use s->clock in case machine uses reliable
+             * get clock and source host supported
+             * reliable get clock
+             */

Quoted 8 lines

hw/i386/kvm/clock.c line 123

Quoted 4 lines

hw/i386/kvm/clock.c +4 -0

+            if (!(s->mach_use_reliable_get_clock &&
+                  s->src_use_reliable_get_clock)) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }

The s->mach_use_reliable_get_clock check seems redundant. src_use_reliable_get_clock is set only if mach_use_reliable_get_clock is true.

Done.

Quoted 61 lines

Quoted 46 lines

hw/i386/kvm/clock.c +10 -12

+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
-        if (time_at_migration) {
-            s->clock = time_at_migration;
+        /* We can't rely on the saved clock value, just discard it */
+        if (pvclock_via_mem) {
+            s->clock = pvclock_via_mem;
         }
 
+        s->clock_valid = false;
+
         data.clock = s->clock;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {

@@ -120,8 +161,6 @@
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
         if (s->clock_valid) {
             return;

@@ -129,13 +168,7 @@
 
         kvm_synchronize_all_tsc();
 
-        ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
-        if (ret < 0) {
-            fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
-            abort();
-        }
-        s->clock = data.clock;
-
+        s->clock = kvm_get_clock();
         /*
          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return

@@ -152,22 +185,69 @@
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;

hw/i386/kvm/clock.c line 200

Quoted 10 lines

hw/i386/kvm/clock.c +10 -0

+
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,
+     * if host kernel does provide reliable KVM_GET_CLOCK,
+     * set src_use_reliable_get_clock=true so that destination
+     * avoids reading kvmclock from memory.
+     */
+    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
+        s->src_use_reliable_get_clock = true;
+    }

It feels fragile to change device state inside the .needed function. Better to initialize src_use_reliable_get_clock on kvmclock_realize()?

Quoted 4 lines

Quoted 2 lines

What exactly ensures src_use_reliable_get_clock is correctly initialized on the migration destination as well?

Its initialized to false (because its part of device state).

Do you mean it is always going to be false on the destination host? What if .needed is called by other code? .needed shouldn't have any side-effects.

Also, the difference between the variables on the destination and the source is very confusing, see other comments below:

Quoted 18 lines

Quoted 10 lines

hw/i386/kvm/clock.c line 202

Quoted 3 lines

hw/i386/kvm/clock.c +2 -0

+
+    return s->mach_use_reliable_get_clock;

If if kvm_has_adjust_clock_stable() is false, isn't it simpler to simply skip the section?

It looks like mach_use_reliable_get_clock and src_use_reliable_get_clock could become a single field:

Quoted 7 lines

* use_reliable_get_clock ("x-use-reliable-get-clock" property) set to true by default (set on DEFINE_PROP_BOOL parameter)
* "x-use-reliable-get-clock" set to false by default on older machine-types
* use_reliable_get_clock forced to false on kvmclock_realize() if !kvm_has_adjust_clock_stable()
* kvmclock_reliable_get_clock.needed return s->use_reliable_get_clock

Quoted 5 lines

No because use_reliable_get_clock can be false because the destination host does not support kvm_has_adjust_clock_stable(), but the source host supports it (so we want to use the value in the first incoming migration).

Are you talking about the value of the fields on the source host, or on the destination host? In this case, you mean that use_reliable_get_clock should be false on the destination, right?

Quoted 2 lines

So the variable indicates whether the source host supported migration, not whether the destination supports it.

Good, that's the whole point of the field, isn't it? I will try to summarize all cases below. Please correct me if something is incorrect.

--------+------------+---------------+-----------+------------+

        | kvm_has_adj_clock_stable() | use_reliable_get_clock |
machine | src        | dst           | src       | dst        |
--------+------------+---------------+-----------+------------+
pc-2.8  | false      | false         | false     | false      |
pc-2.8  | false      | true          | false     | false (*)  |
pc-2.8  | true       | false         | true      | true (**)  |
pc-2.8  | true       | true          | true      | true       |

--------+------------+---------------+-----------+------------+
pc-2.7 | (any) | (any) | false | false |
--------+------------+---------------+-----------+------------+

(*) I guess this is where things would break if we skip the
subsection (and use a single field). If the subsection is skipped, use_reliable_get_clock would be set to true on the destination. (See my comment below on the new patch you sent)

(**) Destination can still do the right thing here, right?

Quoted 1 line

[...]

Quoted 123 lines

How about the following:

Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================

hw/i386/kvm/clock.c +56 -14

@@ -36,6 +36,12 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -81,6 +87,19 @@
     return nsec + time.system_time;
 }
 
+static uint64_t kvm_get_clock(void)
+{
+    struct kvm_clock_data data;
+    int ret;
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+                abort();
+    }
+    return data.clock;
+}
+
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {

@@ -91,15 +110,36 @@
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);
+        uint64_t pvclock_via_mem = 0;
 
-        s->clock_valid = false;
+        /* local (running VM) restore */
+        if (s->clock_valid) {
+            /*
+             * if host does not support reliable KVM_GET_CLOCK,
+             * read kvmclock value from memory
+             */
+            if (!kvm_has_adjust_clock_stable()) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }
+        /* migration/savevm/init restore */
+        } else {
+            /*
+             * use s->clock in case machine uses reliable
+             * get clock and source host supported
+             * reliable get clock
+             */
+            if (!s->src_use_reliable_get_clock) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }
+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
-        if (time_at_migration) {
-            s->clock = time_at_migration;
+        /* We can't rely on the saved clock value, just discard it */
+        if (pvclock_via_mem) {
+            s->clock = pvclock_via_mem;
         }
 
+        s->clock_valid = false;
+
         data.clock = s->clock;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {

@@ -120,8 +160,6 @@
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
         if (s->clock_valid) {
             return;

@@ -129,13 +167,7 @@
 
         kvm_synchronize_all_tsc();
 
-        ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
-        if (ret < 0) {
-            fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
-            abort();
-        }
-        s->clock = data.clock;
-
+        s->clock = kvm_get_clock();
         /*
          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return

@@ -149,25 +181,72 @@
 {
     KVMClockState *s = KVM_CLOCK(dev);
 
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,
+     * if host kernel does provide reliable KVM_GET_CLOCK,
+     * set src_use_reliable_get_clock=true so that destination
+     * avoids reading kvmclock from memory.
+     */
+    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
+        s->src_use_reliable_get_clock = true;
+    }
+
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 

hw/i386/kvm/clock.c line 202

Quoted 6 lines

hw/i386/kvm/clock.c +6 -0

+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    return s->mach_use_reliable_get_clock;
+}

I didn't review kvmclock_vm_state_change() and kvm_has_adjust_clock_stable(), but the rules to migrate src_use_reliable_get_clock look good now, and seem to match the table above.

Quoted 97 lines

hw/i386/kvm/clock.c +31 -0

+
+static const VMStateDescription kvmclock_reliable_get_clock = {
+    .name = "kvmclock/src_use_reliable_get_clock",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = kvmclock_src_use_reliable_get_clock,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void kvmclock_pre_save(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    s->clock = kvm_get_clock();
+}
+
 static const VMStateDescription kvmclock_vmsd = {
     .name = "kvmclock",
     .version_id = 1,
     .minimum_version_id = 1,
+    .pre_save = kvmclock_pre_save,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(clock, KVMClockState),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+        &kvmclock_reliable_get_clock,
+        NULL
     }
 };
 
+static Property kvmclock_properties[] = {
+    DEFINE_PROP_BOOL("x-mach-use-reliable-get-clock", KVMClockState,
+                      mach_use_reliable_get_clock, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void kvmclock_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = kvmclock_realize;
     dc->vmsd = &kvmclock_vmsd;
+    dc->props = kvmclock_properties;
 }
 
 static const TypeInfo kvmclock_info = {

Index: qemu-mig-advance-clock/include/hw/i386/pc.h
===================================================================

include/hw/i386/pc.h +5 -0

@@ -370,6 +370,11 @@
 #define PC_COMPAT_2_7 \
     HW_COMPAT_2_7 \
     {\
+        .driver   = "kvmclock",\
+        .property = "x-mach-use-reliable-get-clock",\
+        .value    = "off",\
+    },\
+    {\
         .driver   = TYPE_X86_CPU,\
         .property = "l3-cache",\
         .value    = "off",\

Index: qemu-mig-advance-clock/target-i386/kvm.c
===================================================================

target-i386/kvm.c +7 -0

@@ -117,6 +117,13 @@
     return kvm_check_extension(kvm_state, KVM_CAP_X86_SMM);
 }
 
+bool kvm_has_adjust_clock_stable(void)
+{
+    int ret = kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK);
+
+    return (ret == KVM_CLOCK_TSC_STABLE);
+}
+
 bool kvm_allows_irq0_override(void)
 {
     return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();

Index: qemu-mig-advance-clock/target-i386/kvm_i386.h
===================================================================

target-i386/kvm_i386.h +1 -0

@@ -17,6 +17,7 @@
 
 bool kvm_allows_irq0_override(void);
 bool kvm_has_smm(void);
+bool kvm_has_adjust_clock_stable(void);
 void kvm_synchronize_all_tsc(void);
 void kvm_arch_reset_vcpu(X86CPU *cs);
 void kvm_arch_do_init_vcpu(X86CPU *cs);

Signature · 1 line

Eduardo

Quoted 163 lines · Eduardo Habkost

Quoted 162 lines · Marcelo Tosatti

Quoted 86 lines · Eduardo Habkost

Quoted 85 lines · Marcelo Tosatti

Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which indicates that KVM_GET_CLOCK returns a value as seen by the guest at that moment.

For new machine types, use this value rather than reading from guest memory.

This reduces kvmclock difference on migration from 5s to 0.1s (when max_downtime == 5s).

Signed-off-by Marcelo Tosatti <[email protected]>

---
 hw/i386/kvm/clock.c    |  108 ++++++++++++++++++++++++++++++++++++++++++-------
 include/hw/i386/pc.h   |    5 ++
 target-i386/kvm.c      |    7 +++
 target-i386/kvm_i386.h |    1 
 4 files changed, 107 insertions(+), 14 deletions(-)

v2:
- improve variable names (Juan)
- consolidate code on kvm_get_clock function (Paolo)
- return mach_use_reliable_get_clock from needed function (Paolo)

Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================

hw/i386/kvm/clock.c +36 -2

@@ -36,6 +36,12 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -81,6 +87,19 @@
     return nsec + time.system_time;
 }
 
+static uint64_t kvm_get_clock(void)
+{
+    struct kvm_clock_data data;
+    int ret;
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+                abort();
+    }
+    return data.clock;
+}
+
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)
 {

@@ -91,15 +110,37 @@
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);
+        uint64_t pvclock_via_mem = 0;
 
-        s->clock_valid = false;
+        /* local (running VM) restore */
+        if (s->clock_valid) {
+            /*
+             * if host does not support reliable KVM_GET_CLOCK,
+             * read kvmclock value from memory
+             */
+            if (!kvm_has_adjust_clock_stable()) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }
+        /* migration/savevm/init restore */
+        } else {
+            /*
+             * use s->clock in case machine uses reliable
+             * get clock and source host supported
+             * reliable get clock
+             */

Quoted 8 lines

hw/i386/kvm/clock.c line 123

Quoted 4 lines

hw/i386/kvm/clock.c +4 -0

+            if (!(s->mach_use_reliable_get_clock &&
+                  s->src_use_reliable_get_clock)) {
+                pvclock_via_mem = kvmclock_current_nsec(s);
+            }

The s->mach_use_reliable_get_clock check seems redundant. src_use_reliable_get_clock is set only if mach_use_reliable_get_clock is true.

Done.

Quoted 63 lines

Quoted 46 lines

hw/i386/kvm/clock.c +10 -12

+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
-        if (time_at_migration) {
-            s->clock = time_at_migration;
+        /* We can't rely on the saved clock value, just discard it */
+        if (pvclock_via_mem) {
+            s->clock = pvclock_via_mem;
         }
 
+        s->clock_valid = false;
+
         data.clock = s->clock;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {

@@ -120,8 +161,6 @@
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
         if (s->clock_valid) {
             return;

@@ -129,13 +168,7 @@
 
         kvm_synchronize_all_tsc();
 
-        ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
-        if (ret < 0) {
-            fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
-            abort();
-        }
-        s->clock = data.clock;
-
+        s->clock = kvm_get_clock();
         /*
          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return

@@ -152,22 +185,69 @@
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;

hw/i386/kvm/clock.c line 200

Quoted 10 lines

hw/i386/kvm/clock.c +10 -0

+
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,
+     * if host kernel does provide reliable KVM_GET_CLOCK,
+     * set src_use_reliable_get_clock=true so that destination
+     * avoids reading kvmclock from memory.
+     */
+    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
+        s->src_use_reliable_get_clock = true;
+    }

It feels fragile to change device state inside the .needed function. Better to initialize src_use_reliable_get_clock on kvmclock_realize()?

What exactly ensures src_use_reliable_get_clock is correctly initialized on the migration destination as well?

Quoted 5 lines

Quoted 1 line

Its initialized to false (because its part of device state).

Do you mean it is always going to be false on the destination host? What if .needed is called by other code? .needed shouldn't have any side-effects.

I don't think needed is called by any other code.

Quoted 3 lines

Also, the difference between the variables on the destination and the source is very confusing, see other comments below:

Yes it is: its because you don't know when is what. But if you consider separate events:

initialization, incoming migration, vmstop, vmcontinue.

And the values of the variables for each case, then it becomes clear.

If anyone has a suggestion to improve clarify, its welcome.

Quoted 25 lines

Quoted 19 lines

Quoted 11 lines

hw/i386/kvm/clock.c line 202

Quoted 4 lines

hw/i386/kvm/clock.c +2 -0

+
+    return s->mach_use_reliable_get_clock;

If if kvm_has_adjust_clock_stable() is false, isn't it simpler to simply skip the section?

It looks like mach_use_reliable_get_clock and src_use_reliable_get_clock could become a single field:

Quoted 7 lines

* use_reliable_get_clock ("x-use-reliable-get-clock" property) set to true by default (set on DEFINE_PROP_BOOL parameter)
* "x-use-reliable-get-clock" set to false by default on older machine-types
* use_reliable_get_clock forced to false on kvmclock_realize() if !kvm_has_adjust_clock_stable()
* kvmclock_reliable_get_clock.needed return s->use_reliable_get_clock

Quoted 5 lines

No because use_reliable_get_clock can be false because the destination host does not support kvm_has_adjust_clock_stable(), but the source host supports it (so we want to use the value in the first incoming migration).

Quoted 3 lines

Are you talking about the value of the fields on the source host, or on the destination host? In this case, you mean that use_reliable_get_clock should be false on the destination, right?

Yes, destination.

Quoted 6 lines

Quoted 2 lines

So the variable indicates whether the source host supported migration, not whether the destination supports it.

Good, that's the whole point of the field, isn't it? I will try to summarize all cases below. Please correct me if something is incorrect.

Yes. So in this case, the cleanup/simplification to use a single variable is not possible (or maybe it is, but it just complicates things).

Quoted 8 lines

--------+------------+---------------+-----------+------------+

        | kvm_has_adj_clock_stable() | use_reliable_get_clock |
machine | src        | dst           | src       | dst        |
--------+------------+---------------+-----------+------------+
pc-2.8  | false      | false         | false     | false      |
pc-2.8  | false      | true          | false     | false (*)  |
pc-2.8  | true       | false         | true      | true (**)  |
pc-2.8  | true       | true          | true      | true       |

Quoted 8 lines

--------+------------+---------------+-----------+------------+
pc-2.7 | (any) | (any) | false | false |
--------+------------+---------------+-----------+------------+

(*) I guess this is where things would break if we skip the
subsection (and use a single field). If the subsection is skipped, use_reliable_get_clock would be set to true on the destination. (See my comment below on the new patch you sent)

Well, perhaps you can massage things so that it works without sending subsection (that is):

* Send subsection only in case value is true.
* Do not send subsection in case value is false (then treat non presence of subsection and mach_reliable_clock=true to mean source did not support kvm_has_reliable_clock).

Once you agree on "send a subsection for both cases", then you handle it.

The code is there, its simple, readable, so...

(what you can't do is to unify src_use_get_reliable_clock (remote information), and local information). Because you need to know both things to be able to handle the 4 cases properly on incoming migration.

[...]