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

[Marcelo Tosatti <mtosatti@redhat.com>]

On Mon, Nov 14, 2016 at 06:20:29PM +0100, Paolo Bonzini wrote:
> 
> 
> On 14/11/2016 18:13, Marcelo Tosatti wrote:
> > On Mon, Nov 14, 2016 at 05:43:33PM +0100, Paolo Bonzini wrote:
> >>
> >>
> >> On 14/11/2016 16:40, Marcelo Tosatti wrote:
> >>> static bool kvmclock_src_use_reliable_get_clock(void *opaque)
> >>> {
> >>>     KVMClockState *s = opaque;
> >>>
> >>>     /*
> >>>      * On machine types that support reliable KVM_GET_CLOCK,
> >>>      * if host kernel does provide reliable KVM_GET_CLOCK,
> >>>      * set src_use_reliable_get_clock=true so that destination
> >>>      * avoids reading kvmclock from memory.
> >>>      */
> >>>     if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable())
> >>>     {
> >>>         s->src_use_reliable_get_clock = true;
> >>>     }
> >>>
> >>>     return s->mach_use_reliable_get_clock;
> >>> }
> >>>
> >>>
> >>> Ah, OK, done.
> >>
> >> s->src_use_reliable_get_clock should not be set with
> >> KVM_CHECK_EXTENSION, but rather from the flags returned by KVM_GET_CLOCK.
> > 
> > Well, thats not right: What matters is the presence of get_kvmclock_ns 
> > which returns a value that the guest sees. 
> > 
> >                 get_kernel_monotonic_clock() + kvmclock_offset +
> >                 (rdtsc() - tsc_timestamp)
> > 
> > IOW what the guest sees. And you changed that in 
> > 
> > commit 108b249c453dd7132599ab6dc7e435a7036c193f
> > Author: Paolo Bonzini <pbonzini@redhat.com>
> > Date:   Thu Sep 1 14:21:03 2016 +0200
> > 
> >     KVM: x86: introduce get_kvmclock_ns
> > 
> > And the correct behaviour (once KVM_GET_CLOCK is fixed per 
> > previous message to return rdtsc - tsc_timestamp for the 
> > non masterclock case) depends on this commit above, 
> > not on masterclock.
> 
> This commit in turn only gets the correct behavior if 
> "vcpu->arch.hv_clock.flags & PVCLOCK_TSC_STABLE_BIT" (and it will be 
> changed soon to ka->use_masterclock).  KVM_CHECK_EXTENSION can still 
> return KVM_CLOCK_TSC_STABLE even if the masterclock is disabled, 
> because KVM_CHECK_EXTENSION only tells you which flags are known for
> this version of the KVM module.

What QEMU wants is to use KVM_GET_CLOCK at pre_save independently
of whether masterclock is enabled or not... it just depends
on KVM_GET_CLOCK being correct for the masterclock case
(108b249c453dd7132599ab6dc7e435a7036c193f).

So a "reliable KVM_GET_CLOCK" (that does not timebackward
when masterclock is enabled) is much simpler to userspace
than "whether masterclock is enabled or not".

If you have a reason why that should not be the case,
let me know.

> To see if the masterclock is enabled _now_, you need to check what
> KVM_GET_CLOCK sets in the flags.  From the KVM_CLOCK_TSC_STABLE patch:
> 
> 		user_ns.flags = kvm->arch.use_master_clock ? KVM_CLOCK_TSC_STABLE : 0;

Again, whether masterclock is enable is independent of 
being able to use KVM_GET_CLOCK at pre_save.