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

Note: pre_save contradicts the following comment
/*
* If the VM is stopped, declare the clock state valid to
* avoid re-reading it on next vmsave (which would return
* a different value). Will be reset when the VM is continued. */
But the comment is bogus: vm_state_change is never called twice in a row with running=0 or running=1.

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

---
 hw/i386/kvm/clock.c    |  107
 ++++++++++++++++++++++++++++++++++++++++++-------
 include/hw/i386/pc.h   |    5 ++
 target-i386/kvm.c      |    7 +++
 target-i386/kvm_i386.h |    1
 4 files changed, 106 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) v3:
- simplify check for src_use_reliable_get_clock (Eduardo)
- move src_use_reliable_get_clock initialization to realize (Eduardo)

v4:
- have kvm_get_clock and clock_is_reliable assignments synchronized (Eduardo)
- add comment to the reasoning of kvmclock_pre_save (Eduardo)

Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================
--- qemu-mig-advance-clock.orig/hw/i386/kvm/clock.c 2016-11-17
15:07:11.220632761 -0200
+++ qemu-mig-advance-clock/hw/i386/kvm/clock.c 2016-12-10 13:57:58.115983966
-0200

hw/i386/kvm/clock.c +33 -9

@@ -36,6 +36,13 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine type supports reliable get clock */
+    bool mach_use_reliable_get_clock;
+
+    /* whether the 'clock' value was obtained in a host with
+     * reliable KVM_GET_CLOCK */
+    bool clock_is_reliable;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -81,6 +88,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 +111,23 @@
 
     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;
+        /*
+         * If the host where s->clock was read did not support reliable
+         * KVM_GET_CLOCK, read kvmclock value from memory.
+         */
+        if (!s->clock_is_reliable) {
+            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 +148,6 @@
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
         if (s->clock_valid) {
             return;

@@ -129,13 +155,11 @@
 
         kvm_synchronize_all_tsc();
 
-        ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
-        if (ret < 0) {

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

         /*
          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return

@@ -149,25 +173,80 @@
 {
     KVMClockState *s = KVM_CLOCK(dev);
 
+    if (kvm_has_adjust_clock_stable()) {
+        s->clock_is_reliable = true;
+    }
+
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_clock_is_reliable_needed(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    return s->mach_use_reliable_get_clock;
+}
+
+static const VMStateDescription kvmclock_reliable_get_clock = {
+    .name = "kvmclock/clock_is_reliable",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = kvmclock_clock_is_reliable_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(clock_is_reliable, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+/*
+ * When migrating, read the clock just before migration,
+ * so that the guest clock counts during the events
+ * between:
+ *
+ *  * vm_stop()
+ *  *
+ *  * pre_save()
+ *
+ *  This reduces kvmclock difference on migration from 5s
+ *  to 0.1s (when max_downtime == 5s), because sending the
+ *  final pages of memory (which happens between vm_stop()
+ *  and pre_save()) takes max_downtime.
+ */
+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
===================================================================
--- qemu-mig-advance-clock.orig/include/hw/i386/pc.h 2016-11-17
15:07:11.220632761 -0200
+++ qemu-mig-advance-clock/include/hw/i386/pc.h 2016-11-17 15:08:58.096791416
-0200

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
===================================================================
--- qemu-mig-advance-clock.orig/target-i386/kvm.c 2016-11-17
15:07:11.221632762 -0200
+++ qemu-mig-advance-clock/target-i386/kvm.c 2016-11-17 15:07:15.867639659
-0200

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
===================================================================
--- qemu-mig-advance-clock.orig/target-i386/kvm_i386.h 2016-11-17
15:07:11.222632764 -0200
+++ qemu-mig-advance-clock/target-i386/kvm_i386.h 2016-11-17
15:07:15.867639659 -0200

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

Hello Marcelo,

Quoted 124 lines

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

Note: pre_save contradicts the following comment
/*
* If the VM is stopped, declare the clock state valid to
* avoid re-reading it on next vmsave (which would return
* a different value). Will be reset when the VM is continued. */
But the comment is bogus: vm_state_change is never called twice in a row with running=0 or running=1.

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

---
 hw/i386/kvm/clock.c    |  107
 ++++++++++++++++++++++++++++++++++++++++++-------
 include/hw/i386/pc.h   |    5 ++
 target-i386/kvm.c      |    7 +++
 target-i386/kvm_i386.h |    1
 4 files changed, 106 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) v3:
- simplify check for src_use_reliable_get_clock (Eduardo)
- move src_use_reliable_get_clock initialization to realize (Eduardo)

v4:
- have kvm_get_clock and clock_is_reliable assignments synchronized (Eduardo)
- add comment to the reasoning of kvmclock_pre_save (Eduardo)

Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================
--- qemu-mig-advance-clock.orig/hw/i386/kvm/clock.c 2016-11-17
15:07:11.220632761 -0200
+++ qemu-mig-advance-clock/hw/i386/kvm/clock.c 2016-12-10 13:57:58.115983966
-0200

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

@@ -36,6 +36,13 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine type supports reliable get clock */
+    bool mach_use_reliable_get_clock;
+
+    /* whether the 'clock' value was obtained in a host with
+     * reliable KVM_GET_CLOCK */
+    bool clock_is_reliable;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -81,6 +88,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 +111,23 @@
 
     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;
+        /*
+         * If the host where s->clock was read did not support reliable
+         * KVM_GET_CLOCK, read kvmclock value from memory.
+         */
+        if (!s->clock_is_reliable) {
+            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 +148,6 @@
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
         if (s->clock_valid) {
             return;

@@ -129,13 +155,11 @@
 
         kvm_synchronize_all_tsc();
 

hw/i386/kvm/clock.c line 162

Quoted 12 lines

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

-        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();
+        /* any code that sets s->clock needs to ensure clock_is_reliable
+         * is correctly set.
+         */
+        s->clock_is_reliable = kvm_has_adjust_clock_stable();

Is it required to call 'kvm_has_adjust_clock_stable' here. Is there any chance of getting different value then in 'realize'?

Quoted 139 lines

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

          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return

@@ -149,25 +173,80 @@
 {
     KVMClockState *s = KVM_CLOCK(dev);
 
+    if (kvm_has_adjust_clock_stable()) {
+        s->clock_is_reliable = true;
+    }
+
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_clock_is_reliable_needed(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    return s->mach_use_reliable_get_clock;
+}
+
+static const VMStateDescription kvmclock_reliable_get_clock = {
+    .name = "kvmclock/clock_is_reliable",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = kvmclock_clock_is_reliable_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(clock_is_reliable, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+/*
+ * When migrating, read the clock just before migration,
+ * so that the guest clock counts during the events
+ * between:
+ *
+ *  * vm_stop()
+ *  *
+ *  * pre_save()
+ *
+ *  This reduces kvmclock difference on migration from 5s
+ *  to 0.1s (when max_downtime == 5s), because sending the
+ *  final pages of memory (which happens between vm_stop()
+ *  and pre_save()) takes max_downtime.
+ */
+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
===================================================================
--- qemu-mig-advance-clock.orig/include/hw/i386/pc.h 2016-11-17
15:07:11.220632761 -0200
+++ qemu-mig-advance-clock/include/hw/i386/pc.h 2016-11-17 15:08:58.096791416
-0200

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
===================================================================
--- qemu-mig-advance-clock.orig/target-i386/kvm.c 2016-11-17
15:07:11.221632762 -0200
+++ qemu-mig-advance-clock/target-i386/kvm.c 2016-11-17 15:07:15.867639659
-0200

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
===================================================================
--- qemu-mig-advance-clock.orig/target-i386/kvm_i386.h 2016-11-17
15:07:11.222632764 -0200
+++ qemu-mig-advance-clock/target-i386/kvm_i386.h 2016-11-17
15:07:15.867639659 -0200

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 2 lines

Hello Marcelo,

Hi Pankaj,

Quoted 135 lines

Quoted 135 lines

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

Note: pre_save contradicts the following comment
/*
* If the VM is stopped, declare the clock state valid to
* avoid re-reading it on next vmsave (which would return
* a different value). Will be reset when the VM is continued. */
But the comment is bogus: vm_state_change is never called twice in a row with running=0 or running=1.

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

---
 hw/i386/kvm/clock.c    |  107
 ++++++++++++++++++++++++++++++++++++++++++-------
 include/hw/i386/pc.h   |    5 ++
 target-i386/kvm.c      |    7 +++
 target-i386/kvm_i386.h |    1
 4 files changed, 106 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) v3:
- simplify check for src_use_reliable_get_clock (Eduardo)
- move src_use_reliable_get_clock initialization to realize (Eduardo)

v4:
- have kvm_get_clock and clock_is_reliable assignments synchronized (Eduardo)
- add comment to the reasoning of kvmclock_pre_save (Eduardo)

Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================
--- qemu-mig-advance-clock.orig/hw/i386/kvm/clock.c 2016-11-17
15:07:11.220632761 -0200
+++ qemu-mig-advance-clock/hw/i386/kvm/clock.c 2016-12-10
13:57:58.115983966
-0200

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

@@ -36,6 +36,13 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine type supports reliable get clock */
+    bool mach_use_reliable_get_clock;
+
+    /* whether the 'clock' value was obtained in a host with
+     * reliable KVM_GET_CLOCK */
+    bool clock_is_reliable;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -81,6 +88,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 +111,23 @@
 
     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;
+        /*
+         * If the host where s->clock was read did not support reliable
+         * KVM_GET_CLOCK, read kvmclock value from memory.
+         */
+        if (!s->clock_is_reliable) {
+            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 +148,6 @@
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
         if (s->clock_valid) {
             return;

@@ -129,13 +155,11 @@
 
         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();

Quoted 8 lines

hw/i386/kvm/clock.c line 162

Quoted 4 lines

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

+        /* any code that sets s->clock needs to ensure clock_is_reliable
+         * is correctly set.
+         */
+        s->clock_is_reliable = kvm_has_adjust_clock_stable();

Is it required to call 'kvm_has_adjust_clock_stable' here. Is there any chance
of getting different value then in 'realize'?

Yes it can happen: migration overwrites s->clock_is_reliable value (but you can't see that from grepping).

Quoted 142 lines

Quoted 142 lines

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

          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return

@@ -149,25 +173,80 @@
 {
     KVMClockState *s = KVM_CLOCK(dev);
 
+    if (kvm_has_adjust_clock_stable()) {
+        s->clock_is_reliable = true;
+    }
+
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_clock_is_reliable_needed(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    return s->mach_use_reliable_get_clock;
+}
+
+static const VMStateDescription kvmclock_reliable_get_clock = {
+    .name = "kvmclock/clock_is_reliable",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = kvmclock_clock_is_reliable_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(clock_is_reliable, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+/*
+ * When migrating, read the clock just before migration,
+ * so that the guest clock counts during the events
+ * between:
+ *
+ *  * vm_stop()
+ *  *
+ *  * pre_save()
+ *
+ *  This reduces kvmclock difference on migration from 5s
+ *  to 0.1s (when max_downtime == 5s), because sending the
+ *  final pages of memory (which happens between vm_stop()
+ *  and pre_save()) takes max_downtime.
+ */
+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
===================================================================
--- qemu-mig-advance-clock.orig/include/hw/i386/pc.h 2016-11-17
15:07:11.220632761 -0200
+++ qemu-mig-advance-clock/include/hw/i386/pc.h 2016-11-17
15:08:58.096791416
-0200

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
===================================================================
--- qemu-mig-advance-clock.orig/target-i386/kvm.c 2016-11-17
15:07:11.221632762 -0200
+++ qemu-mig-advance-clock/target-i386/kvm.c 2016-11-17
15:07:15.867639659
-0200

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
===================================================================
--- qemu-mig-advance-clock.orig/target-i386/kvm_i386.h 2016-11-17
15:07:11.222632764 -0200
+++ qemu-mig-advance-clock/target-i386/kvm_i386.h 2016-11-17
15:07:15.867639659 -0200

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 157 lines · Pankaj Gupta

Quoted 2 lines

Hello Marcelo,

Hi Pankaj,

Quoted 147 lines

Quoted 140 lines

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

Note: pre_save contradicts the following comment
/*
* If the VM is stopped, declare the clock state valid to
* avoid re-reading it on next vmsave (which would return
* a different value). Will be reset when the VM is continued. */
But the comment is bogus: vm_state_change is never called twice in a row
with running=0 or running=1.

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

---
 hw/i386/kvm/clock.c    |  107
 ++++++++++++++++++++++++++++++++++++++++++-------
 include/hw/i386/pc.h   |    5 ++
 target-i386/kvm.c      |    7 +++
 target-i386/kvm_i386.h |    1
 4 files changed, 106 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) v3:
- simplify check for src_use_reliable_get_clock (Eduardo)
- move src_use_reliable_get_clock initialization to realize (Eduardo)

v4:
- have kvm_get_clock and clock_is_reliable assignments synchronized (Eduardo)
- add comment to the reasoning of kvmclock_pre_save (Eduardo)

Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
===================================================================
--- qemu-mig-advance-clock.orig/hw/i386/kvm/clock.c 2016-11-17
15:07:11.220632761 -0200
+++ qemu-mig-advance-clock/hw/i386/kvm/clock.c 2016-12-10
13:57:58.115983966
-0200

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

@@ -36,6 +36,13 @@
 
     uint64_t clock;
     bool clock_valid;
+
+    /* whether machine type supports reliable get clock */
+    bool mach_use_reliable_get_clock;
+
+    /* whether the 'clock' value was obtained in a host with
+     * reliable KVM_GET_CLOCK */
+    bool clock_is_reliable;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -81,6 +88,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 +111,23 @@
 
     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;
+        /*
+         * If the host where s->clock was read did not support

reliable

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

+         * KVM_GET_CLOCK, read kvmclock value from memory.
+         */
+        if (!s->clock_is_reliable) {
+            pvclock_via_mem = kvmclock_current_nsec(s);
+        }
 
-        /* We can't rely on the migrated clock value, just discard it

*/

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

-        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 +148,6 @@
             }
         }
     } else {
-        struct kvm_clock_data data;
-        int ret;
 
         if (s->clock_valid) {
             return;

@@ -129,13 +155,11 @@
 
         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();
+        /* any code that sets s->clock needs to ensure

clock_is_reliable

hw/i386/kvm/clock.c line 162

Quoted 3 lines

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

+         * is correctly set.
+         */
+        s->clock_is_reliable = kvm_has_adjust_clock_stable();

Is it required to call 'kvm_has_adjust_clock_stable' here. Is there any chance
of getting different value then in 'realize'?

Quoted 2 lines

Yes it can happen: migration overwrites s->clock_is_reliable value (but you can't see that from grepping).

o.k.

Quoted 132 lines

Quoted 132 lines

Quoted 132 lines

* If the VM is stopped, declare the clock state valid to
* avoid re-reading it on next vmsave (which would return

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

@@ -149,25 +173,80 @@
 {
     KVMClockState *s = KVM_CLOCK(dev);
 
+    if (kvm_has_adjust_clock_stable()) {
+        s->clock_is_reliable = true;
+    }
+
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_clock_is_reliable_needed(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    return s->mach_use_reliable_get_clock;
+}
+
+static const VMStateDescription kvmclock_reliable_get_clock = {
+    .name = "kvmclock/clock_is_reliable",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = kvmclock_clock_is_reliable_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(clock_is_reliable, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+/*
+ * When migrating, read the clock just before migration,
+ * so that the guest clock counts during the events
+ * between:
+ *
+ *  * vm_stop()
+ *  *
+ *  * pre_save()
+ *
+ *  This reduces kvmclock difference on migration from 5s
+ *  to 0.1s (when max_downtime == 5s), because sending the
+ *  final pages of memory (which happens between vm_stop()
+ *  and pre_save()) takes max_downtime.
+ */
+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
===================================================================
--- qemu-mig-advance-clock.orig/include/hw/i386/pc.h 2016-11-17
15:07:11.220632761 -0200
+++ qemu-mig-advance-clock/include/hw/i386/pc.h 2016-11-17
15:08:58.096791416
-0200

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
===================================================================
--- qemu-mig-advance-clock.orig/target-i386/kvm.c 2016-11-17
15:07:11.221632762 -0200
+++ qemu-mig-advance-clock/target-i386/kvm.c 2016-11-17
15:07:15.867639659
-0200

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
===================================================================
--- qemu-mig-advance-clock.orig/target-i386/kvm_i386.h 2016-11-17
15:07:11.222632764 -0200
+++ qemu-mig-advance-clock/target-i386/kvm_i386.h 2016-11-17
15:07:15.867639659 -0200

(quoted diff) +0 -0

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

Quoted 12 lines

Quoted 10 lines

target-i386/kvm_i386.h line 23

Quoted 10 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);

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);

}

On Sat, Dec 10, 2016 at 03:21:50PM -0200, Marcelo Tosatti wrote:
[...]

Quoted 4 lines

static void kvmclock_realize(DeviceState *dev, Error **errp)

 {
     KVMClockState *s = KVM_CLOCK(dev);

hw/i386/kvm/clock.c line 179

Quoted 4 lines

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

+    if (kvm_has_adjust_clock_stable()) {
+        s->clock_is_reliable = true;
+    }
+

This seems unnecessary, as kvmclock_vm_state_change() makes sure it is set at the same time as s->clock. Should we just remove it?

Quoted 11 lines · Eduardo Habkost

On Sat, Dec 10, 2016 at 03:21:50PM -0200, Marcelo Tosatti wrote:
[...]

Quoted 8 lines

static void kvmclock_realize(DeviceState *dev, Error **errp)

 {
     KVMClockState *s = KVM_CLOCK(dev);

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

+    if (kvm_has_adjust_clock_stable()) {
+        s->clock_is_reliable = true;
+    }
+

Quoted 2 lines

This seems unnecessary, as kvmclock_vm_state_change() makes sure it is set at the same time as s->clock. Should we just remove it?

There is this initialization that goes from ~running -> running which assumes its initialized:

static void kvmclock_vm_state_change(void *opaque, int running,
RunState state)

{
    KVMClockState *s = opaque;
    CPUState *cpu;
    int cap_clock_ctrl = kvm_check_extension(kvm_state,
KVM_CAP_KVMCLOCK_CTRL);
    int ret;

    if (running) {
        struct kvm_clock_data data = {};
        uint64_t pvclock_via_mem = 0;

        /*
         * If the host where s->clock was read did not support reliable
         * KVM_GET_CLOCK, read kvmclock value from memory.
         */
        if (!s->clock_is_reliable) {
            pvclock_via_mem = kvmclock_current_nsec(s);
        }

I'd like to make a few cleanups and add more documentation:

hw/i386/kvm/clock.c +52 -19

index eacc9dc..f767ea9 100644
@@ -37,7 +37,7 @@ typedef struct KVMClockState {
     uint64_t clock;
     bool clock_valid;
 
-    /* whether machine type supports reliable get clock */
+    /* whether machine type supports reliable KVM_GET_CLOCK */
     bool mach_use_reliable_get_clock;
 
     /* whether the 'clock' value was obtained in a host with

@@ -88,7 +88,7 @@ static uint64_t kvmclock_current_nsec(KVMClockState *s)
     return nsec + time.system_time;
 }
 
-static uint64_t kvm_get_clock(void)
+static void kvm_update_clock(void)
 {
     struct kvm_clock_data data;
     int ret;

@@ -98,7 +98,48 @@ static uint64_t kvm_get_clock(void)
         fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
                 abort();
     }
-    return data.clock;
+    s->clock = data.clock;
+
+    /* If kvm_has_adjust_clock_stable() is false, KVM_GET_CLOCK returns
+     * essentially CLOCK_MONOTONIC plus a guest-specific adjustment.  This
+     * can drift from the TSC-based value that is computed by the guest,
+     * so we need to go through kvmclock_current_nsec().  If
+     * kvm_has_adjust_clock_stable() is true, and the flags contain
+     * KVM_CLOCK_TSC_STABLE, then KVM_GET_CLOCK returns a TSC-based value
+     * and kvmclock_current_nsec() is not necessary.
+     *
+     * Here, however, we need not check KVM_CLOCK_TSC_STABLE.  This is because:
+     *
+     * - if the host has disabled the kvmclock master clock, the guest already
+     *   has protection against time going backwards.  This "safety net" is only
+     *   absent when kvmclock is stable;
+     *
+     * - therefore, we can replace a check like
+     *
+     *       if last KVM_GET_CLOCK was not reliable
+     *           read from memory
+     *
+     *   with
+     *
+     *       if last KVM_GET_CLOCK was not reliable && masterclock is enabled
+     *           read from memory
+     *
+     * However:
+     *
+     * - if kvm_has_adjust_clock_stable() returns false, the left side is
+     *   always true (KVM_GET_CLOCK is never reliable), and the right side is
+     *   unknown (because we don't have data.flags).  We must assume it's true
+     *   and read from memory.
+     *
+     * - if kvm_has_adjust_clock_stable() returns true, the result of the &&
+     *   is always false (masterclock is enabled iff KVM_GET_CLOCK is reliable)
+     *
+     * So we can just use this instead:
+     *
+     *       if !kvm_has_adjust_clock_stable() then
+     *           read from memory
+     */
+    s->clock_is_reliable = kvm_has_adjust_clock_stable();
 }
 
 static void kvmclock_vm_state_change(void *opaque, int running,

@@ -111,19 +153,17 @@ static void kvmclock_vm_state_change(void *opaque, int running,
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t pvclock_via_mem = 0;
 
         /*
          * If the host where s->clock was read did not support reliable
          * KVM_GET_CLOCK, read kvmclock value from memory.
          */
         if (!s->clock_is_reliable) {
-            pvclock_via_mem = kvmclock_current_nsec(s);
-        }
-
-        /* We can't rely on the saved clock value, just discard it */
-        if (pvclock_via_mem) {
-            s->clock = pvclock_via_mem;
+            uint64_t pvclock_via_mem = kvmclock_current_nsec(s);
+            /* 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;

@@ -155,11 +195,7 @@ static void kvmclock_vm_state_change(void *opaque, int running,
 
         kvm_synchronize_all_tsc();
 
-        s->clock = kvm_get_clock();
-        /* any code that sets s->clock needs to ensure clock_is_reliable
-         * is correctly set.
-         */
-        s->clock_is_reliable = kvm_has_adjust_clock_stable();
+        kvm_update_clock();
         /*
          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return

@@ -173,9 +209,7 @@ static void kvmclock_realize(DeviceState *dev, Error **errp)
 {
     KVMClockState *s = KVM_CLOCK(dev);
 
-    if (kvm_has_adjust_clock_stable()) {
-        s->clock_is_reliable = true;
-    }
+    kvm_update_clock();
 
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }

@@ -216,7 +250,7 @@ static void kvmclock_pre_save(void *opaque)
 {
     KVMClockState *s = opaque;
 
-    s->clock = kvm_get_clock();
+    kvm_update_clock();
 }
 
 static const VMStateDescription kvmclock_vmsd = {

Quoted 2 lines · Paolo Bonzini

I'd like to make a few cleanups and add more documentation:

Looks good to me.

Reviewed-by Eduardo Habkost <[email protected]>