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 <[email protected]>
> 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
