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

[Paolo Bonzini]

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 <address@hidden>
> 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.

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;

Thanks,

Paolo

>>> So s->src_use_reliable_get_clock is only used to indicate 
>>> to the destination that: "you can use KVM_GET_CLOCK value, 
>>> its safe".
>>
>> Yes, we agree.  I was listing all the points, not just those where we
>> disagree.  Actually I'm not sure where we disagree, except on using
>> flags from KVM_CHECK_EXTENSION vs. flags from KVM_GET_CLOCK...
>>
>> Paolo