On 17/11/2016 13:16, Marcelo Tosatti wrote:
> On Mon, Nov 14, 2016 at 04:15:18PM -0200, Marcelo Tosatti wrote:
>> 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 <pbonz...@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.
>
> Is this point OK ?
>
> Using
>
> break;
> + case KVM_CAP_ADJUST_CLOCK:
> + r = KVM_CLOCK_TSC_STABLE;
> + break;
>
> To infer whether KVM_GET_CLOCK is fixed for the monotonic case.
Yes, I still haven't digested why it is correct (I need to read again
what you wrote), but it is indeed correct to use KVM_CAP_ADJUST_CLOCK
this way.
Paolo
