Re: [PATCH v2] KVM: x86: fix KVM_SET_CLOCK relative to setting correct clock value

["Radim Krčmář" <rkrcmar@redhat.com>]

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)

and the important fix for kvm master clock is the move of
kvm_gen_update_masterclock() before we read the time.

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?

(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);
 		break;
 	}
 	case KVM_GET_CLOCK: {

Thanks.