Re: [Qemu-devel] [qemu patch V4 2/2] kvmclock: reduce kvmclock difference on migration

[Paolo Bonzini <pbonzini@redhat.com>]

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

diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index eacc9dc..f767ea9 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -37,7 +37,7 @@ typedef struct KVMClockState {
     uint64_t clock;
     bool clock_valid;
 
-    /* whether machine type supports reliable get clock */
+    /* whether machine type supports reliable KVM_GET_CLOCK */
     bool mach_use_reliable_get_clock;
 
     /* whether the 'clock' value was obtained in a host with
@@ -88,7 +88,7 @@ static uint64_t kvmclock_current_nsec(KVMClockState *s)
     return nsec + time.system_time;
 }
 
-static uint64_t kvm_get_clock(void)
+static void kvm_update_clock(void)
 {
     struct kvm_clock_data data;
     int ret;
@@ -98,7 +98,48 @@ static uint64_t kvm_get_clock(void)
         fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
                 abort();
     }
-    return data.clock;
+    s->clock = data.clock;
+
+    /* If kvm_has_adjust_clock_stable() is false, KVM_GET_CLOCK returns
+     * essentially CLOCK_MONOTONIC plus a guest-specific adjustment.  This
+     * can drift from the TSC-based value that is computed by the guest,
+     * so we need to go through kvmclock_current_nsec().  If
+     * kvm_has_adjust_clock_stable() is true, and the flags contain
+     * KVM_CLOCK_TSC_STABLE, then KVM_GET_CLOCK returns a TSC-based value
+     * and kvmclock_current_nsec() is not necessary.
+     *
+     * Here, however, we need not check KVM_CLOCK_TSC_STABLE.  This is because:
+     *
+     * - if the host has disabled the kvmclock master clock, the guest already
+     *   has protection against time going backwards.  This "safety net" is only
+     *   absent when kvmclock is stable;
+     *
+     * - therefore, we can replace a check like
+     *
+     *       if last KVM_GET_CLOCK was not reliable
+     *           read from memory
+     *
+     *   with
+     *
+     *       if last KVM_GET_CLOCK was not reliable && masterclock is enabled
+     *           read from memory
+     *
+     * However:
+     *
+     * - if kvm_has_adjust_clock_stable() returns false, the left side is
+     *   always true (KVM_GET_CLOCK is never reliable), and the right side is
+     *   unknown (because we don't have data.flags).  We must assume it's true
+     *   and read from memory.
+     *
+     * - if kvm_has_adjust_clock_stable() returns true, the result of the &&
+     *   is always false (masterclock is enabled iff KVM_GET_CLOCK is reliable)
+     *
+     * So we can just use this instead:
+     *
+     *       if !kvm_has_adjust_clock_stable() then
+     *           read from memory
+     */
+    s->clock_is_reliable = kvm_has_adjust_clock_stable();
 }
 
 static void kvmclock_vm_state_change(void *opaque, int running,
@@ -111,19 +153,17 @@ static void kvmclock_vm_state_change(void *opaque, int running,
 
     if (running) {
         struct kvm_clock_data data = {};
-        uint64_t pvclock_via_mem = 0;
 
         /*
          * If the host where s->clock was read did not support reliable
          * KVM_GET_CLOCK, read kvmclock value from memory.
          */
         if (!s->clock_is_reliable) {
-            pvclock_via_mem = kvmclock_current_nsec(s);
-        }
-
-        /* We can't rely on the saved clock value, just discard it */
-        if (pvclock_via_mem) {
-            s->clock = pvclock_via_mem;
+            uint64_t pvclock_via_mem = kvmclock_current_nsec(s);
+            /* We can't rely on the saved clock value, just discard it */
+            if (pvclock_via_mem) {
+                s->clock = pvclock_via_mem;
+            }
         }
 
         s->clock_valid = false;
@@ -155,11 +195,7 @@ static void kvmclock_vm_state_change(void *opaque, int running,
 
         kvm_synchronize_all_tsc();
 
-        s->clock = kvm_get_clock();
-        /* any code that sets s->clock needs to ensure clock_is_reliable
-         * is correctly set.
-         */
-        s->clock_is_reliable = kvm_has_adjust_clock_stable();
+        kvm_update_clock();
         /*
          * If the VM is stopped, declare the clock state valid to
          * avoid re-reading it on next vmsave (which would return
@@ -173,9 +209,7 @@ static void kvmclock_realize(DeviceState *dev, Error **errp)
 {
     KVMClockState *s = KVM_CLOCK(dev);
 
-    if (kvm_has_adjust_clock_stable()) {
-        s->clock_is_reliable = true;
-    }
+    kvm_update_clock();
 
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
 }
@@ -216,7 +250,7 @@ static void kvmclock_pre_save(void *opaque)
 {
     KVMClockState *s = opaque;
 
-    s->clock = kvm_get_clock();
+    kvm_update_clock();
 }
 
 static const VMStateDescription kvmclock_vmsd = {