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

[Eduardo Habkost <ehabkost@redhat.com>]

On Fri, Dec 16, 2016 at 11:03:33AM +0100, Paolo Bonzini wrote:
> I'd like to make a few cleanups and add more documentation:
> 

Looks good to me.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> 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 = {

-- 
Eduardo