On Wed, May 10, 2017 at 08:04:31PM +0200, Radim Krčmář wrote:
> 2017-05-03 10:43-0300, Marcelo Tosatti:
> > In the masterclock enabled case, kvmclock_offset must be adjusted so
> > that user_ns.clock = master_kernel_ns + kvmclock_offset (that is, the
> > value set from KVM_SET_CLOCK is the one visible at system_timestamp).
>
> IIUC, we want to achieve
>
> user_ns.clock == get_kvmclock_ns(kvm)
Yes, or equivalently we want
user_ns.clock = master_kernel_ns + kvmclock_offset,
when
guest_rdtsc() == ka->master_cycle_now.
> and the important fix for kvm master clock is the move of
> kvm_gen_update_masterclock() before we read the time.
Yes.
> The rest is just a minor optimization that also ignores time since
> master_kernel_ns() and therefore pins user_ns.clock to a slightly
> earlier time.
>
> But all attention was given to the "minor optimization" -- have I missed
> something about the direct use of ka->master_kernel_ns?
I haven't attempted to optimize anything. Not sure what you mean.
>
> (If not, I'd rather have just the one-line move.)
>
> ---
> Detailed reasoning.
>
> kvm_gen_update_masterclock() shifts the master clock (changes its
> percieved frequency, because it is reset from kernel boot clock again,
> even though they are diverging), so we must not compute the
> kvmclock_offset with an old master clock and apply it to a shifted one.
>
> Using offset from ka->master_kernel_ns or get_kvmclock_ns()
> ("ka->master_kernel_ns + small delta") doesn't make much difference
> then, because the user_ns is already an indeterminable point in the
> past. (We just assume that user_ns.clock is now, whenever that is.)
>
> ---
> And a possible improvement.
>
> Using kvm_gen_update_masterclock() seems superfluous. There are three
> major cases depending on state of the master clock:
>
> enabled)
> We can just match the kvmclock_offset so the following holds
> user_ns.clock == get_kvmclock_ns(kvm)
> No need to update the master clock.
>
> disabled)
> The kvmclock_offset is set from the kernel boot clock and that is
> already correct.
>
> disabled, but call to kvm_gen_update_masterclock() would enable it)
> The master clock will be updates with the kernel boot clock when a
> VCPU runs. This means that master clock and kernel boot clock will
> begin diverging at a later point, but the initial offset is the same.
> Userspace can only tell the difference by calling KVM_GET_CLOCK
> afterwards and seeing the stable bit unset.
> Guest is mostly unaffected -- the inaccuracy from calling
> KVM_SET_CLOCK is much bigger than accumulation of a slightly
> different frequency will in few jiffies.
>
> Do you see a reason to use kvm_gen_update_masterclock()?
>
> I think that the code could be just:
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 464da936c53d..f024216a858d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4177,7 +4177,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
> r = 0;
> now_ns = get_kvmclock_ns(kvm);
> kvm->arch.kvmclock_offset += user_ns.clock - now_ns;
> - kvm_gen_update_masterclock(kvm);
> + kvm_make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE);
No, this is wrong. kvm_gen_update_masterclock() must happen before
the assignment of kvm->arch.kvmclock_offset and get_kvmclock_ns().
(so that kvm_gen_update_masterclock sets ka->use_master_clock to true
and get_kvmclock_ns() enters this case:
hv_clock.tsc_timestamp = ka->master_cycle_now;
hv_clock.system_time = ka->master_kernel_ns +
ka->kvmclock_offset;
spin_unlock(&ka->pvclock_gtod_sync_lock);
kvm_get_time_scale(NSEC_PER_SEC, __this_cpu_read(cpu_tsc_khz) *
1000LL,
&hv_clock.tsc_shift,
&hv_clock.tsc_to_system_mul);
return __pvclock_read_cycles(&hv_clock, rdtsc());
Makes sense?
