improve kvmclock difference on migration

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

     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);

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

+                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);
 }
 
+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    return s->mach_use_reliable_get_clock;
+}
+
+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[]) {

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

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

Sorry for not noticing the following issues on the previous reviews. I was only paying attention to the vmstate and machine code:

hw/i386/kvm/clock.c

Quoted 66 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    |  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)

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

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

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

Can you please use "diff -p" on your patches?

Quoted 4 lines

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

     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);

hw/i386/kvm/clock.c line 121

Quoted 10 lines

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

+        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()) {

Why not use src_use_reliable_get_clock here, too? (Maybe rename it to simply clock_is_reliable?)

You can change kvm_get_clock() to kvm_get_clock(KVMClockState*), and make it update both s->clock and s->clock_is_reliable. With this, s->clock and s->clock_is_reliable will be always in sync, and have a single code path for both local restore and migration:

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

Updating the table from my previous message, the result would be:

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

        | kvm_has_adj_clock_stable() | clock_is_reliable   |
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  | false      | (any)         | false     | false   |
pc-2.7  | true       | (any)         | true (**) | false   |

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

(*) More precisely: in the first kvmclock_vm_state_change() call
just after migration. It will get updated the next time s->clock is set.

(**) On purpose, so the code I suggest above would work for local
restore, too.

Quoted 81 lines

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

+                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);
 }
 
+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    return s->mach_use_reliable_get_clock;
+}
+
+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[]) {

hw/i386/kvm/clock.c line 219

Quoted 10 lines

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

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

I can't tell if s->clock_valid here is a bug, or a feature. In either case, we seem to be contradicting the comment at kvmclock_vm_state_change():

    /*
     * 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.
     */
    s->clock_valid = true;

Quoted 82 lines

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

+}
+
 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 89 lines · Eduardo Habkost

Sorry for not noticing the following issues on the previous reviews. I was only paying attention to the vmstate and machine code:

hw/i386/kvm/clock.c

Quoted 66 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    |  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)

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

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

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

Can you please use "diff -p" on your patches?

Quoted 14 lines

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

     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()) {

Quoted 2 lines

Why not use src_use_reliable_get_clock here, too? (Maybe rename it to simply clock_is_reliable?)

Because you end up mixing two mental ideas (one: whether the source has KVM_GET_CLOCK, second: whether the destination has KVM_GET_CLOCK into one variable). I find it more confusing than not. Maybe its just my limited brain but i find it
confusing.

Quoted 5 lines

You can change kvm_get_clock() to kvm_get_clock(KVMClockState*), and make it update both s->clock and s->clock_is_reliable. With this, s->clock and s->clock_is_reliable will be always in sync, and have a single code path for both local restore and migration:

There should not be a single path for migration/runtime cases:
on migration, we care whether the source used reliable KVM_GET_CLOCK.

On runtime, we care whether the destination uses reliable KVM_GET_CLOCK.

Quoted 128 lines

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

Updating the table from my previous message, the result would be:

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

        | kvm_has_adj_clock_stable() | clock_is_reliable   |
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  | false      | (any)         | false     | false   |
pc-2.7  | true       | (any)         | true (**) | false   |

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

(*) More precisely: in the first kvmclock_vm_state_change() call
just after migration. It will get updated the next time s->clock is set.

(**) On purpose, so the code I suggest above would work for local
restore, too.

Quoted 81 lines

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

+                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);
 }
 
+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    return s->mach_use_reliable_get_clock;
+}
+
+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[]) {

hw/i386/kvm/clock.c line 219

Quoted 10 lines

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

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

I can't tell if s->clock_valid here is a bug, or a feature. In either case, we seem to be contradicting the comment at kvmclock_vm_state_change():

Quoted 7 lines

    /*
     * 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.
     */
    s->clock_valid = true;

Sorry, what is the problem??

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

Why not use src_use_reliable_get_clock here, too? (Maybe rename it to simply clock_is_reliable?)

Because you end up mixing two mental ideas (one: whether the source has KVM_GET_CLOCK, second: whether the destination has KVM_GET_CLOCK into one variable). I find it more confusing than not. Maybe its just my limited brain but i find it
confusing.

I find it simpler and easier to reason about.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

You can change kvm_get_clock() to kvm_get_clock(KVMClockState*), and make it update both s->clock and s->clock_is_reliable. With this, s->clock and s->clock_is_reliable will be always in sync, and have a single code path for both local restore and migration:

Quoted 4 lines

Quoted 4 lines

There should not be a single path for migration/runtime cases:
on migration, we care whether the source used reliable KVM_GET_CLOCK.

On runtime, we care whether the destination uses reliable KVM_GET_CLOCK.

The conditions may be different, but both can be translated to a "should we trust the value in s->clock" flag, to simplify the code (just like Paolo's suggestion on his reply to v1 today).

Quoted 93 lines · Marcelo Tosatti

Quoted 91 lines · Eduardo Habkost

Sorry for not noticing the following issues on the previous reviews. I was only paying attention to the vmstate and machine code:

hw/i386/kvm/clock.c

Quoted 66 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    |  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)

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

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

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

Can you please use "diff -p" on your patches?

Quoted 4 lines

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

     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);

hw/i386/kvm/clock.c line 120

Quoted 10 lines

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

+        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()) {

Why not use src_use_reliable_get_clock here, too? (Maybe rename it to simply clock_is_reliable?)

Quoted 5 lines

Because you end up mixing two mental ideas (one: whether the source has KVM_GET_CLOCK, second: whether the destination has KVM_GET_CLOCK into one variable). I find it more confusing than not. Maybe its just my limited brain but i find it
confusing.

I find it simpler and easier to reason about.

Quoted 94 lines · Eduardo Habkost

Quoted 93 lines · Marcelo Tosatti

Quoted 91 lines · Eduardo Habkost

Sorry for not noticing the following issues on the previous reviews. I was only paying attention to the vmstate and machine code:

hw/i386/kvm/clock.c

Quoted 66 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    |  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)

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

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

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

Can you please use "diff -p" on your patches?

Quoted 4 lines

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

     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);

hw/i386/kvm/clock.c line 120

Quoted 10 lines

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

+        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()) {

Why not use src_use_reliable_get_clock here, too? (Maybe rename it to simply clock_is_reliable?)

Quoted 7 lines

Quoted 5 lines

Because you end up mixing two mental ideas (one: whether the source has KVM_GET_CLOCK, second: whether the destination has KVM_GET_CLOCK into one variable). I find it more confusing than not. Maybe its just my limited brain but i find it
confusing.

I find it simpler and easier to reason about.

I disagree, but i don't mind. Is this what you refer to:

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

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

+    bool clock_is_reliable;
 } KVMClockState;
 
 struct pvclock_vcpu_time_info {

@@ -112,25 +112,17 @@
         struct kvm_clock_data data = {};
         uint64_t pvclock_via_mem = 0;
 
-        /* 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 +6 -12

         /* We can't rely on the saved clock value, just discard it */

@@ -181,20 +173,14 @@
 {
     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;
+    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_src_use_reliable_get_clock(void *opaque)
+static bool kvmclock_clock_is_reliable_needed(void *opaque)
 {
     KVMClockState *s = opaque;
 

@@ -202,12 +188,12 @@
 }
 
 static const VMStateDescription kvmclock_reliable_get_clock = {
-    .name = "kvmclock/src_use_reliable_get_clock",
+    .name = "kvmclock/clock_is_reliable",
     .version_id = 1,
     .minimum_version_id = 1,
-    .needed = kvmclock_src_use_reliable_get_clock,
+    .needed = kvmclock_clock_is_reliable_needed,
     .fields = (VMStateField[]) {
-        VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
+        VMSTATE_BOOL(clock_is_reliable, KVMClockState),
         VMSTATE_END_OF_LIST()
     }
 };

Quoted 117 lines · Marcelo Tosatti

Quoted 94 lines · Eduardo Habkost

Quoted 93 lines · Marcelo Tosatti

Quoted 91 lines · Eduardo Habkost

Sorry for not noticing the following issues on the previous reviews. I was only paying attention to the vmstate and machine code:

hw/i386/kvm/clock.c

Quoted 66 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    |  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)

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

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

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

Can you please use "diff -p" on your patches?

Quoted 4 lines

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

     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);

hw/i386/kvm/clock.c line 120

Quoted 10 lines

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

+        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()) {

Why not use src_use_reliable_get_clock here, too? (Maybe rename it to simply clock_is_reliable?)

Quoted 7 lines

Quoted 5 lines

Because you end up mixing two mental ideas (one: whether the source has KVM_GET_CLOCK, second: whether the destination has KVM_GET_CLOCK into one variable). I find it more confusing than not. Maybe its just my limited brain but i find it
confusing.

I find it simpler and easier to reason about.

I disagree, but i don't mind. Is this what you refer to:

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

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

@@ -37,11 +37,11 @@
     uint64_t clock;
     bool clock_valid;
 
-    /* whether machine supports reliable KVM_GET_CLOCK */
+    /* whether machine type supports reliable get clock */
     bool mach_use_reliable_get_clock;
 

hw/i386/kvm/clock.c line 43

Quoted 3 lines

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

-    /* whether source host supported reliable KVM_GET_CLOCK */
-    bool src_use_reliable_get_clock;
+    /* whether clock is reliable */

I would make the comment more explicit about its meaning:
/* whether the 'clock' value was obtained in a host with
* reliable KVM_GET_CLOCK */

hw/i386/kvm/clock.c line 44

Quoted 1 line

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

+    bool clock_is_reliable;

} KVMClockState;

Quoted 22 lines

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

 struct pvclock_vcpu_time_info {

@@ -112,25 +112,17 @@
         struct kvm_clock_data data = {};
         uint64_t pvclock_via_mem = 0;
 
-        /* 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 115

Quoted 4 lines

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

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

A comment here would be welcome. Something like: "If the host where s->clock was read did not support reliable KVM_GET_CLOCK, read kvmclock value from memory".

hw/i386/kvm/clock.c line 124

Quoted 9 lines

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

+        if (!s->clock_is_reliable) {
+            pvclock_via_mem = kvmclock_current_nsec(s);
+        }
+
+        /* migration/savevm/init restore
+         * update clock_is_reliable to match local
+         * host capabilities.
+         */
+        if (s->clock_valid == false) {

Why the s->clock_valid check? It seems unnecessary.

(If this is an optimization to avoid extra ioctl() calls, you can cache it inside kvm_has_adjust_clock_stable()).

hw/i386/kvm/clock.c line 126

Quoted 2 lines

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

+            s->clock_is_reliable = kvm_has_adjust_clock_stable();
         }

This should go to kvm_get_clock(), so s->clock and s->clock_is_reliable are guaranteed to be always in sync.

Quoted 41 lines

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

         /* We can't rely on the saved clock value, just discard it */

@@ -181,20 +173,14 @@
 {
     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;
+    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_src_use_reliable_get_clock(void *opaque)
+static bool kvmclock_clock_is_reliable_needed(void *opaque)
 {
     KVMClockState *s = opaque;
 

@@ -202,12 +188,12 @@
 }
 
 static const VMStateDescription kvmclock_reliable_get_clock = {
-    .name = "kvmclock/src_use_reliable_get_clock",
+    .name = "kvmclock/clock_is_reliable",
     .version_id = 1,
     .minimum_version_id = 1,
-    .needed = kvmclock_src_use_reliable_get_clock,
+    .needed = kvmclock_clock_is_reliable_needed,
     .fields = (VMStateField[]) {
-        VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
+        VMSTATE_BOOL(clock_is_reliable, KVMClockState),
         VMSTATE_END_OF_LIST()
     }
 };

Signature · 1 line

Eduardo

Quoted 118 lines · Eduardo Habkost

Quoted 117 lines · Marcelo Tosatti

Quoted 94 lines · Eduardo Habkost

Quoted 93 lines · Marcelo Tosatti

Quoted 91 lines · Eduardo Habkost

Sorry for not noticing the following issues on the previous reviews. I was only paying attention to the vmstate and machine code:

hw/i386/kvm/clock.c

Quoted 66 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    |  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)

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

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

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

Can you please use "diff -p" on your patches?

Quoted 4 lines

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

     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);

hw/i386/kvm/clock.c line 120

Quoted 10 lines

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

+        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()) {

Why not use src_use_reliable_get_clock here, too? (Maybe rename it to simply clock_is_reliable?)

Quoted 7 lines

Quoted 5 lines

Because you end up mixing two mental ideas (one: whether the source has KVM_GET_CLOCK, second: whether the destination has KVM_GET_CLOCK into one variable). I find it more confusing than not. Maybe its just my limited brain but i find it
confusing.

I find it simpler and easier to reason about.

I disagree, but i don't mind. Is this what you refer to:

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

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

@@ -37,11 +37,11 @@
     uint64_t clock;
     bool clock_valid;
 
-    /* whether machine supports reliable KVM_GET_CLOCK */
+    /* whether machine type supports reliable get clock */
     bool mach_use_reliable_get_clock;
 

Quoted 7 lines

hw/i386/kvm/clock.c line 43

Quoted 3 lines

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

-    /* whether source host supported reliable KVM_GET_CLOCK */
-    bool src_use_reliable_get_clock;
+    /* whether clock is reliable */

I would make the comment more explicit about its meaning:
/* whether the 'clock' value was obtained in a host with
* reliable KVM_GET_CLOCK */

Done.

Quoted 25 lines

hw/i386/kvm/clock.c line 44

Quoted 1 line

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

+    bool clock_is_reliable;

} KVMClockState;

Quoted 22 lines

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

 struct pvclock_vcpu_time_info {

@@ -112,25 +112,17 @@
         struct kvm_clock_data data = {};
         uint64_t pvclock_via_mem = 0;
 
-        /* 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 115

Quoted 4 lines

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

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

A comment here would be welcome. Something like: "If the host where s->clock was read did not support reliable KVM_GET_CLOCK, read kvmclock value from memory".

Done.

Quoted 10 lines

Quoted 9 lines

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

+        if (!s->clock_is_reliable) {
+            pvclock_via_mem = kvmclock_current_nsec(s);
+        }
+
+        /* migration/savevm/init restore
+         * update clock_is_reliable to match local
+         * host capabilities.
+         */
+        if (s->clock_valid == false) {

Quoted 1 line

Why the s->clock_valid check? It seems unnecessary.

Because if s->clock_valid == false (from migration), kvm_has_adjust_clock_stable() from the local host is irrelevant.

If s->clock_valid == true, kvm_has_adjust_clock_stable() is what decides whether to use KVM_GET_CLOCK or not.

Quoted 2 lines

(If this is an optimization to avoid extra ioctl() calls, you can cache it inside kvm_has_adjust_clock_stable()).

Its not.

Quoted 5 lines

hw/i386/kvm/clock.c line 126

Quoted 2 lines

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

+            s->clock_is_reliable = kvm_has_adjust_clock_stable();
         }

This should go to kvm_get_clock(), so s->clock and s->clock_is_reliable are guaranteed to be always in sync.

No: the point to change clock_is_reliable is the following: whether its the first read of the clock (migration), or subsequent reads (normal machine operation).

Now kvm_get_clock() just reads the clock, so i don't see it as the right place to change the variable.

Agreed?

Quoted 45 lines

Quoted 42 lines

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

         /* We can't rely on the saved clock value, just discard it */

@@ -181,20 +173,14 @@
 {
     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;
+    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_src_use_reliable_get_clock(void *opaque)
+static bool kvmclock_clock_is_reliable_needed(void *opaque)
 {
     KVMClockState *s = opaque;
 

@@ -202,12 +188,12 @@
 }
 
 static const VMStateDescription kvmclock_reliable_get_clock = {
-    .name = "kvmclock/src_use_reliable_get_clock",
+    .name = "kvmclock/clock_is_reliable",
     .version_id = 1,
     .minimum_version_id = 1,
-    .needed = kvmclock_src_use_reliable_get_clock,
+    .needed = kvmclock_clock_is_reliable_needed,
     .fields = (VMStateField[]) {
-        VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
+        VMSTATE_BOOL(clock_is_reliable, KVMClockState),
         VMSTATE_END_OF_LIST()
     }
 };

Signature · 1 line

Eduardo

Quoted 175 lines · Marcelo Tosatti

Quoted 118 lines · Eduardo Habkost

Quoted 117 lines · Marcelo Tosatti

Quoted 94 lines · Eduardo Habkost

Quoted 93 lines · Marcelo Tosatti

Quoted 91 lines · Eduardo Habkost

Sorry for not noticing the following issues on the previous reviews. I was only paying attention to the vmstate and machine code:

hw/i386/kvm/clock.c

Quoted 66 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    |  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)

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

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

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

Can you please use "diff -p" on your patches?

Quoted 4 lines

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

     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);

hw/i386/kvm/clock.c line 120

Quoted 10 lines

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

+        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()) {

Why not use src_use_reliable_get_clock here, too? (Maybe rename it to simply clock_is_reliable?)

Quoted 7 lines

Quoted 5 lines

Because you end up mixing two mental ideas (one: whether the source has KVM_GET_CLOCK, second: whether the destination has KVM_GET_CLOCK into one variable). I find it more confusing than not. Maybe its just my limited brain but i find it
confusing.

I find it simpler and easier to reason about.

I disagree, but i don't mind. Is this what you refer to:

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

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

@@ -37,11 +37,11 @@
     uint64_t clock;
     bool clock_valid;
 
-    /* whether machine supports reliable KVM_GET_CLOCK */
+    /* whether machine type supports reliable get clock */
     bool mach_use_reliable_get_clock;
 

Quoted 7 lines

hw/i386/kvm/clock.c line 43

Quoted 3 lines

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

-    /* whether source host supported reliable KVM_GET_CLOCK */
-    bool src_use_reliable_get_clock;
+    /* whether clock is reliable */

I would make the comment more explicit about its meaning:
/* whether the 'clock' value was obtained in a host with
* reliable KVM_GET_CLOCK */

Done.

Quoted 25 lines

hw/i386/kvm/clock.c line 44

Quoted 1 line

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

+    bool clock_is_reliable;

} KVMClockState;

Quoted 22 lines

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

 struct pvclock_vcpu_time_info {

@@ -112,25 +112,17 @@
         struct kvm_clock_data data = {};
         uint64_t pvclock_via_mem = 0;
 
-        /* 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 115

Quoted 4 lines

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

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

A comment here would be welcome. Something like: "If the host where s->clock was read did not support reliable KVM_GET_CLOCK, read kvmclock value from memory".

Done.

Quoted 10 lines

Quoted 9 lines

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

+        if (!s->clock_is_reliable) {
+            pvclock_via_mem = kvmclock_current_nsec(s);
+        }
+
+        /* migration/savevm/init restore
+         * update clock_is_reliable to match local
+         * host capabilities.
+         */
+        if (s->clock_valid == false) {

Quoted 4 lines

Quoted 1 line

Why the s->clock_valid check? It seems unnecessary.

Because if s->clock_valid == false (from migration), kvm_has_adjust_clock_stable() from the local host is irrelevant.

That's true, and that's why updating it on kvm_get_clock() looks like a more logical place.

Quoted 16 lines

If s->clock_valid == true, kvm_has_adjust_clock_stable() is what decides whether to use KVM_GET_CLOCK or not.

Quoted 2 lines

(If this is an optimization to avoid extra ioctl() calls, you can cache it inside kvm_has_adjust_clock_stable()).

Its not.

Quoted 5 lines

hw/i386/kvm/clock.c line 126

Quoted 2 lines

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

+            s->clock_is_reliable = kvm_has_adjust_clock_stable();
         }

This should go to kvm_get_clock(), so s->clock and s->clock_is_reliable are guaranteed to be always in sync.

Quoted 8 lines

No: the point to change clock_is_reliable is the following: whether its the first read of the clock (migration), or subsequent reads (normal machine operation).

Now kvm_get_clock() just reads the clock, so i don't see it as the right place to change the variable.

Agreed?

I don't agree. clock_is_reliable is information about the current value in s->clock, I don't see why we need to change s->clock and s->clock_is_reliable at different places in the code.

Your code might work too, but I can't tell without reviewing very carefully the state transitions KVMClockState could go through. I don't see the point of making the state transitions in this code even more complex, if we can simply stick to a simple "s->clock and s->clock_is_reliable are always in sync" rule.

Quoted 46 lines

Quoted 46 lines

Quoted 43 lines

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

         /* We can't rely on the saved clock value, just discard it */

@@ -181,20 +173,14 @@
 {
     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;
+    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_src_use_reliable_get_clock(void *opaque)
+static bool kvmclock_clock_is_reliable_needed(void *opaque)
 {
     KVMClockState *s = opaque;
 

@@ -202,12 +188,12 @@
 }
 
 static const VMStateDescription kvmclock_reliable_get_clock = {
-    .name = "kvmclock/src_use_reliable_get_clock",
+    .name = "kvmclock/clock_is_reliable",
     .version_id = 1,
     .minimum_version_id = 1,
-    .needed = kvmclock_src_use_reliable_get_clock,
+    .needed = kvmclock_clock_is_reliable_needed,
     .fields = (VMStateField[]) {
-        VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
+        VMSTATE_BOOL(clock_is_reliable, KVMClockState),
         VMSTATE_END_OF_LIST()
     }
 };

Signature · 1 line

Eduardo

Signature · 1 line

Eduardo

Quoted 176 lines · Eduardo Habkost

Quoted 175 lines · Marcelo Tosatti

Quoted 118 lines · Eduardo Habkost

Quoted 117 lines · Marcelo Tosatti

Quoted 94 lines · Eduardo Habkost

Quoted 93 lines · Marcelo Tosatti

Quoted 91 lines · Eduardo Habkost

Sorry for not noticing the following issues on the previous reviews. I was only paying attention to the vmstate and machine code:

hw/i386/kvm/clock.c

Quoted 66 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    |  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)

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

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

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

Can you please use "diff -p" on your patches?

Quoted 4 lines

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

     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);

hw/i386/kvm/clock.c line 120

Quoted 10 lines

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

+        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()) {

Why not use src_use_reliable_get_clock here, too? (Maybe rename it to simply clock_is_reliable?)

Quoted 7 lines

Quoted 5 lines

Because you end up mixing two mental ideas (one: whether the source has KVM_GET_CLOCK, second: whether the destination has KVM_GET_CLOCK into one variable). I find it more confusing than not. Maybe its just my limited brain but i find it
confusing.

I find it simpler and easier to reason about.

I disagree, but i don't mind. Is this what you refer to:

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

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

@@ -37,11 +37,11 @@
     uint64_t clock;
     bool clock_valid;
 
-    /* whether machine supports reliable KVM_GET_CLOCK */
+    /* whether machine type supports reliable get clock */
     bool mach_use_reliable_get_clock;
 

Quoted 7 lines

hw/i386/kvm/clock.c line 43

Quoted 3 lines

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

-    /* whether source host supported reliable KVM_GET_CLOCK */
-    bool src_use_reliable_get_clock;
+    /* whether clock is reliable */

I would make the comment more explicit about its meaning:
/* whether the 'clock' value was obtained in a host with
* reliable KVM_GET_CLOCK */

Done.

Quoted 25 lines

hw/i386/kvm/clock.c line 44

Quoted 1 line

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

+    bool clock_is_reliable;

} KVMClockState;

Quoted 22 lines

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

 struct pvclock_vcpu_time_info {

@@ -112,25 +112,17 @@
         struct kvm_clock_data data = {};
         uint64_t pvclock_via_mem = 0;
 
-        /* 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 115

Quoted 4 lines

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

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

A comment here would be welcome. Something like: "If the host where s->clock was read did not support reliable KVM_GET_CLOCK, read kvmclock value from memory".

Done.

Quoted 10 lines

Quoted 9 lines

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

+        if (!s->clock_is_reliable) {
+            pvclock_via_mem = kvmclock_current_nsec(s);
+        }
+
+        /* migration/savevm/init restore
+         * update clock_is_reliable to match local
+         * host capabilities.
+         */
+        if (s->clock_valid == false) {

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Why the s->clock_valid check? It seems unnecessary.

Because if s->clock_valid == false (from migration), kvm_has_adjust_clock_stable() from the local host is irrelevant.

That's true, and that's why updating it on kvm_get_clock() looks like a more logical place.

But the current patch updates
s->clock_is_reliable at the "if (running)"
case of kvmclock_vm_state_change().

There, both s->clock_is_reliable and s->clock are
updated in sync:

        if (s->clock_valid == false) {
            s->clock_is_reliable = kvm_has_adjust_clock_stable();
        }

        /* We can't rely on the saved clock value, just discard it */
        if (pvclock_via_mem) {
            s->clock = pvclock_via_mem;
        }

However, there is no need to call kvm_get_clock() there.

kvm_get_clock() is called at the "} else {" case.

Are you suggesting to

* move s->clock_is_reliable assignment to kvm_get_clock()
* move s->clock assignment to kvm_get_clock()
* call kvm_get_clock() from the "if (running)" because the assignments are in that function now.

But you don't need KVM_GET_CLOCK to be called there.

Quoted 31 lines

Quoted 18 lines

If s->clock_valid == true, kvm_has_adjust_clock_stable() is what decides whether to use KVM_GET_CLOCK or not.

Quoted 2 lines

(If this is an optimization to avoid extra ioctl() calls, you can cache it inside kvm_has_adjust_clock_stable()).

Its not.

Quoted 5 lines

hw/i386/kvm/clock.c line 126

Quoted 2 lines

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

+            s->clock_is_reliable = kvm_has_adjust_clock_stable();
         }

This should go to kvm_get_clock(), so s->clock and s->clock_is_reliable are guaranteed to be always in sync.

Quoted 8 lines

No: the point to change clock_is_reliable is the following: whether its the first read of the clock (migration), or subsequent reads (normal machine operation).

Now kvm_get_clock() just reads the clock, so i don't see it as the right place to change the variable.

Agreed?

I don't agree. clock_is_reliable is information about the current value in s->clock, I don't see why we need to change s->clock and s->clock_is_reliable at different places in the code.

Quoted 5 lines

Your code might work too, but I can't tell without reviewing very carefully the state transitions KVMClockState could go through. I don't see the point of making the state transitions in this code even more complex, if we can simply stick to a simple "s->clock and s->clock_is_reliable are always in sync" rule.

It is updating from the same place:

        /* migration/savevm/init restore
         * update clock_is_reliable to match local
         * host capabilities.
         */
        if (s->clock_valid == false) {
            s->clock_is_reliable = kvm_has_adjust_clock_stable();
        }

        /* We can't rely on the saved clock value, just discard it */
        if (pvclock_via_mem) {
            s->clock = pvclock_via_mem;
        }

Quoted 51 lines

Quoted 48 lines

Quoted 48 lines

Quoted 45 lines

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

         /* We can't rely on the saved clock value, just discard it */

@@ -181,20 +173,14 @@
 {
     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;
+    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_src_use_reliable_get_clock(void *opaque)
+static bool kvmclock_clock_is_reliable_needed(void *opaque)
 {
     KVMClockState *s = opaque;
 

@@ -202,12 +188,12 @@
 }
 
 static const VMStateDescription kvmclock_reliable_get_clock = {
-    .name = "kvmclock/src_use_reliable_get_clock",
+    .name = "kvmclock/clock_is_reliable",
     .version_id = 1,
     .minimum_version_id = 1,
-    .needed = kvmclock_src_use_reliable_get_clock,
+    .needed = kvmclock_clock_is_reliable_needed,
     .fields = (VMStateField[]) {
-        VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
+        VMSTATE_BOOL(clock_is_reliable, KVMClockState),
         VMSTATE_END_OF_LIST()
     }
 };

Signature · 1 line

Eduardo

Signature · 1 line

Eduardo

Quoted 192 lines · Marcelo Tosatti

Quoted 176 lines · Eduardo Habkost

Quoted 175 lines · Marcelo Tosatti

Quoted 118 lines · Eduardo Habkost

Quoted 117 lines · Marcelo Tosatti

Quoted 94 lines · Eduardo Habkost

Quoted 93 lines · Marcelo Tosatti

Quoted 91 lines · Eduardo Habkost

Sorry for not noticing the following issues on the previous reviews. I was only paying attention to the vmstate and machine code:

hw/i386/kvm/clock.c

Quoted 66 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    |  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)

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

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

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

Can you please use "diff -p" on your patches?

Quoted 4 lines

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

     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);

hw/i386/kvm/clock.c line 120

Quoted 10 lines

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

+        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()) {

Why not use src_use_reliable_get_clock here, too? (Maybe rename it to simply clock_is_reliable?)

Quoted 7 lines

Quoted 5 lines

Because you end up mixing two mental ideas (one: whether the source has KVM_GET_CLOCK, second: whether the destination has KVM_GET_CLOCK into one variable). I find it more confusing than not. Maybe its just my limited brain but i find it
confusing.

I find it simpler and easier to reason about.

I disagree, but i don't mind. Is this what you refer to:

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

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

@@ -37,11 +37,11 @@
     uint64_t clock;
     bool clock_valid;
 
-    /* whether machine supports reliable KVM_GET_CLOCK */
+    /* whether machine type supports reliable get clock */
     bool mach_use_reliable_get_clock;
 

Quoted 7 lines

hw/i386/kvm/clock.c line 43

Quoted 3 lines

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

-    /* whether source host supported reliable KVM_GET_CLOCK */
-    bool src_use_reliable_get_clock;
+    /* whether clock is reliable */

I would make the comment more explicit about its meaning:
/* whether the 'clock' value was obtained in a host with
* reliable KVM_GET_CLOCK */

Done.

Quoted 25 lines

hw/i386/kvm/clock.c line 44

Quoted 1 line

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

+    bool clock_is_reliable;

} KVMClockState;

Quoted 22 lines

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

 struct pvclock_vcpu_time_info {

@@ -112,25 +112,17 @@
         struct kvm_clock_data data = {};
         uint64_t pvclock_via_mem = 0;
 
-        /* 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 115

Quoted 4 lines

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

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

A comment here would be welcome. Something like: "If the host where s->clock was read did not support reliable KVM_GET_CLOCK, read kvmclock value from memory".

Done.

Quoted 10 lines

Quoted 9 lines

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

+        if (!s->clock_is_reliable) {
+            pvclock_via_mem = kvmclock_current_nsec(s);
+        }
+
+        /* migration/savevm/init restore
+         * update clock_is_reliable to match local
+         * host capabilities.
+         */
+        if (s->clock_valid == false) {

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Why the s->clock_valid check? It seems unnecessary.

Because if s->clock_valid == false (from migration), kvm_has_adjust_clock_stable() from the local host is irrelevant.

That's true, and that's why updating it on kvm_get_clock() looks like a more logical place.

But the current patch updates
s->clock_is_reliable at the "if (running)"
case of kvmclock_vm_state_change().

There, both s->clock_is_reliable and s->clock are
updated in sync:

Quoted 9 lines

        if (s->clock_valid == false) {
            s->clock_is_reliable = kvm_has_adjust_clock_stable();
        }

        /* We can't rely on the saved clock value, just discard it */
        if (pvclock_via_mem) {
            s->clock = pvclock_via_mem;
        }

Here s->clock is updated only to be used to set data.clock in the next line (and never used again). I'm talking about every other s->clock assignment in the code.

Quoted 1 line

However, there is no need to call kvm_get_clock() there.

I'm talking about the existing kvm_get_clock() calls.

Quoted 9 lines

kvm_get_clock() is called at the "} else {" case.

Are you suggesting to

* move s->clock_is_reliable assignment to kvm_get_clock()
* move s->clock assignment to kvm_get_clock()
* call kvm_get_clock() from the "if (running)" because the assignments are in that function now.

No.

Quoted 2 lines

But you don't need KVM_GET_CLOCK to be called there.

This is what I'm suggesting:

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

[ehabkost: simplified migration logic]

Signed-off-by Eduardo Habkost <[email protected]>

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

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

index 0f75dd3..df07ec7 100644
@@ -35,6 +35,13 @@ typedef struct KVMClockState {
     /*< public >*/
 
     uint64_t clock;
+    /* whether the value in 'clock' is reliable (i.e. was set by a host
+     * with reliable KVM_GET_CLOCK) */
+    bool clock_is_reliable;
+    /* Enable migration of clock_is_reliable. Some machine-types
+     * will disable it for compatibility with older QEMU versions.
+     */
+    bool migrate_reliable_flag;
     bool clock_valid;
 } KVMClockState;
 

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

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

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

@@ -91,16 +112,18 @@ static void kvmclock_vm_state_change(void *opaque, int running,
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);
 
         s->clock_valid = false;
 
-        /* We can't rely on the migrated clock value, just discard it */
-        if (time_at_migration) {
-            s->clock = time_at_migration;
+        /* if host that set s->clock did not support reliable KVM_GET_CLOCK,
+         * read kvmclock value from memory
+         */
+        if (!s->clock_is_reliable) {
+            data.clock = kvmclock_current_nsec(s);
+        } else {
+            data.clock = s->clock;
         }
 
-        data.clock = s->clock;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {
             fprintf(stderr, "KVM_SET_CLOCK failed: %s\n", strerror(ret));

@@ -120,22 +143,23 @@ static void kvmclock_vm_state_change(void *opaque, int running,
 

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

         if (s->clock_valid) {
             return;
         }
 
         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));

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

         /*
          * 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 +43 -0

@@ -152,22 +176,65 @@ static void kvmclock_realize(DeviceState *dev, Error **errp)
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
 
+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    return s->migrate_reliable_flag;
+}
+
+static const VMStateDescription kvmclock_reliable_get_clock = {
+    .name = "kvmclock/clock_is_reliable",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = kvmclock_src_use_reliable_get_clock,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(clock_is_reliable, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void kvmclock_pre_save(void *opaque)
+{
+    KVMClockState *s = opaque;
+    /*TODO: add comment explaining why this is here:
+     * "re-read clock value here because [...]"" */
+
+    /*XXX: is it correct to call kvm_get_clock() here
+     * if s->clock_is_valid is true? (meaning the kvmclock was
+     * already stopped)
+     */
+    kvm_get_clock(s);
+}
+
 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-migrate-reliable-flag", KVMClockState,
+                      migrate_reliable_flag, 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 = {

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

index 4b74130..fb80d69 100644
@@ -372,6 +372,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 #define PC_COMPAT_2_7 \
     HW_COMPAT_2_7 \
     {\
+        .driver   = "kvmclock",\
+        .property = "x-migrate-reliable-flag",\
+        .value    = "off",\
+    },\
+    {\
         .driver   = TYPE_X86_CPU,\
         .property = "l3-cache",\
         .value    = "off",\

target-i386/kvm.c +7 -0

index f62264a..10a9cd8 100644
@@ -117,6 +117,13 @@ bool kvm_has_smm(void)
     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();

target-i386/kvm_i386.h +1 -0

index 7607929..bfce427 100644
@@ -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

2.7.4

Quoted 39 lines

Quoted 31 lines

Quoted 18 lines

If s->clock_valid == true, kvm_has_adjust_clock_stable() is what decides whether to use KVM_GET_CLOCK or not.

Quoted 2 lines

(If this is an optimization to avoid extra ioctl() calls, you can cache it inside kvm_has_adjust_clock_stable()).

Its not.

Quoted 5 lines

hw/i386/kvm/clock.c line 126

Quoted 2 lines

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

+            s->clock_is_reliable = kvm_has_adjust_clock_stable();
         }

This should go to kvm_get_clock(), so s->clock and s->clock_is_reliable are guaranteed to be always in sync.

Quoted 8 lines

No: the point to change clock_is_reliable is the following: whether its the first read of the clock (migration), or subsequent reads (normal machine operation).

Now kvm_get_clock() just reads the clock, so i don't see it as the right place to change the variable.

Agreed?

I don't agree. clock_is_reliable is information about the current value in s->clock, I don't see why we need to change s->clock and s->clock_is_reliable at different places in the code.

Quoted 5 lines

Your code might work too, but I can't tell without reviewing very carefully the state transitions KVMClockState could go through. I don't see the point of making the state transitions in this code even more complex, if we can simply stick to a simple "s->clock and s->clock_is_reliable are always in sync" rule.

It is updating from the same place:

Quoted 12 lines

        /* migration/savevm/init restore
         * update clock_is_reliable to match local
         * host capabilities.
         */
        if (s->clock_valid == false) {
            s->clock_is_reliable = kvm_has_adjust_clock_stable();
        }

        /* We can't rely on the saved clock value, just discard it */
        if (pvclock_via_mem) {
            s->clock = pvclock_via_mem;
        }

I'm talking about the other s->clock assignments. This one doesn't even seem to be necessary.

Quoted 49 lines

Quoted 49 lines

Quoted 48 lines

Quoted 48 lines

Quoted 45 lines

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

         /* We can't rely on the saved clock value, just discard it */

@@ -181,20 +173,14 @@
 {
     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;
+    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_src_use_reliable_get_clock(void *opaque)
+static bool kvmclock_clock_is_reliable_needed(void *opaque)
 {
     KVMClockState *s = opaque;
 

@@ -202,12 +188,12 @@
 }
 
 static const VMStateDescription kvmclock_reliable_get_clock = {
-    .name = "kvmclock/src_use_reliable_get_clock",
+    .name = "kvmclock/clock_is_reliable",
     .version_id = 1,
     .minimum_version_id = 1,
-    .needed = kvmclock_src_use_reliable_get_clock,
+    .needed = kvmclock_clock_is_reliable_needed,
     .fields = (VMStateField[]) {
-        VMSTATE_BOOL(src_use_reliable_get_clock, KVMClockState),
+        VMSTATE_BOOL(clock_is_reliable, KVMClockState),
         VMSTATE_END_OF_LIST()
     }
 };

Signature · 1 line

Eduardo

Quoted 2 lines

Quoted 2 lines

Signature · 1 line

Eduardo

Signature · 1 line

Eduardo

Quoted 270 lines · Eduardo Habkost

Quoted 192 lines · Marcelo Tosatti

Quoted 176 lines · Eduardo Habkost

Quoted 175 lines · Marcelo Tosatti

Quoted 118 lines · Eduardo Habkost

Quoted 117 lines · Marcelo Tosatti

Quoted 94 lines · Eduardo Habkost

Quoted 93 lines · Marcelo Tosatti

Quoted 91 lines · Eduardo Habkost

Sorry for not noticing the following issues on the previous reviews. I was only paying attention to the vmstate and machine code:

hw/i386/kvm/clock.c

Quoted 66 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    |  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)

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

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

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

Can you please use "diff -p" on your patches?

Quoted 4 lines

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

     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t time_at_migration = kvmclock_current_nsec(s);

hw/i386/kvm/clock.c line 120

Quoted 10 lines

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

+        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()) {

Why not use src_use_reliable_get_clock here, too? (Maybe rename it to simply clock_is_reliable?)

Quoted 7 lines

Quoted 5 lines

Because you end up mixing two mental ideas (one: whether the source has KVM_GET_CLOCK, second: whether the destination has KVM_GET_CLOCK into one variable). I find it more confusing than not. Maybe its just my limited brain but i find it
confusing.

I find it simpler and easier to reason about.

I disagree, but i don't mind. Is this what you refer to:

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

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

@@ -37,11 +37,11 @@
     uint64_t clock;
     bool clock_valid;
 
-    /* whether machine supports reliable KVM_GET_CLOCK */
+    /* whether machine type supports reliable get clock */
     bool mach_use_reliable_get_clock;
 

Quoted 7 lines

hw/i386/kvm/clock.c line 43

Quoted 3 lines

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

-    /* whether source host supported reliable KVM_GET_CLOCK */
-    bool src_use_reliable_get_clock;
+    /* whether clock is reliable */

I would make the comment more explicit about its meaning:
/* whether the 'clock' value was obtained in a host with
* reliable KVM_GET_CLOCK */

Done.

Quoted 25 lines

hw/i386/kvm/clock.c line 44

Quoted 1 line

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

+    bool clock_is_reliable;

} KVMClockState;

Quoted 22 lines

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

 struct pvclock_vcpu_time_info {

@@ -112,25 +112,17 @@
         struct kvm_clock_data data = {};
         uint64_t pvclock_via_mem = 0;
 
-        /* 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 115

Quoted 4 lines

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

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

A comment here would be welcome. Something like: "If the host where s->clock was read did not support reliable KVM_GET_CLOCK, read kvmclock value from memory".

Done.

Quoted 10 lines

Quoted 9 lines

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

+        if (!s->clock_is_reliable) {
+            pvclock_via_mem = kvmclock_current_nsec(s);
+        }
+
+        /* migration/savevm/init restore
+         * update clock_is_reliable to match local
+         * host capabilities.
+         */
+        if (s->clock_valid == false) {

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Why the s->clock_valid check? It seems unnecessary.

Because if s->clock_valid == false (from migration), kvm_has_adjust_clock_stable() from the local host is irrelevant.

That's true, and that's why updating it on kvm_get_clock() looks like a more logical place.

But the current patch updates
s->clock_is_reliable at the "if (running)"
case of kvmclock_vm_state_change().

There, both s->clock_is_reliable and s->clock are
updated in sync:

Quoted 9 lines

        if (s->clock_valid == false) {
            s->clock_is_reliable = kvm_has_adjust_clock_stable();
        }

        /* We can't rely on the saved clock value, just discard it */
        if (pvclock_via_mem) {
            s->clock = pvclock_via_mem;
        }

Here s->clock is updated only to be used to set data.clock in the next line (and never used again). I'm talking about every other s->clock assignment in the code.

Quoted 1 line

However, there is no need to call kvm_get_clock() there.

I'm talking about the existing kvm_get_clock() calls.

Quoted 9 lines

kvm_get_clock() is called at the "} else {" case.

Are you suggesting to

* move s->clock_is_reliable assignment to kvm_get_clock()
* move s->clock assignment to kvm_get_clock()
* call kvm_get_clock() from the "if (running)" because the assignments are in that function now.

No.

Quoted 2 lines

But you don't need KVM_GET_CLOCK to be called there.

This is what I'm suggesting:

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

[ehabkost: simplified migration logic]

Signed-off-by Eduardo Habkost <[email protected]>

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

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

index 0f75dd3..df07ec7 100644
@@ -35,6 +35,13 @@ typedef struct KVMClockState {
     /*< public >*/
 
     uint64_t clock;
+    /* whether the value in 'clock' is reliable (i.e. was set by a host
+     * with reliable KVM_GET_CLOCK) */
+    bool clock_is_reliable;
+    /* Enable migration of clock_is_reliable. Some machine-types
+     * will disable it for compatibility with older QEMU versions.
+     */
+    bool migrate_reliable_flag;
     bool clock_valid;
 } KVMClockState;
 

@@ -81,6 +88,20 @@ static uint64_t kvmclock_current_nsec(KVMClockState *s)
     return nsec + time.system_time;
 }
 
+static void kvm_get_clock(KVMClockState *s)
+{
+    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();
+    }