KVM: x86: fix KVM_SET_CLOCK relative to setting correct clock value

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).

This way the guest clock:

1. Starts counting when KVM_SET_CLOCK executes.
2. With the value provided by userspace.

arch/x86/kvm/x86.c +2 -2

-		now_ns = get_kvmclock_ns(kvm);
-		kvm->arch.kvmclock_offset += user_ns.clock - now_ns;
+
 		kvm_gen_update_masterclock(kvm);
+		if (ka->use_master_clock) {

Quoted 4 lines · 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).

Quoted 4 lines

This way the guest clock:

1. Starts counting when KVM_SET_CLOCK executes.
2. With the value provided by userspace.

So this fixes rounding errors?

Quoted 5 lines

arch/x86/kvm/x86.c +2 -2

-		now_ns = get_kvmclock_ns(kvm);
-		kvm->arch.kvmclock_offset += user_ns.clock - now_ns;
+
 		kvm_gen_update_masterclock(kvm);
+		if (ka->use_master_clock) {

arch/x86/kvm/x86.c line 4200

Quoted 10 lines

arch/x86/kvm/x86.c +10 -0

+			/*
+			 * 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).
+			 */
+			kvm->arch.kvmclock_offset = user_ns.clock -
+							ka->master_kernel_ns;
+

This needs to hold ka->pvclock_gtod_sync_lock, I think.

arch/x86/kvm/x86.c line 4202

Quoted 2 lines

arch/x86/kvm/x86.c +2 -0

+			kvm_for_each_vcpu(i, vcpu, kvm)
+				kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);

kvm_gen_update_masterclock already does that, why did you move that to before the assignment?

Quoted 10 lines

Quoted 8 lines

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).

This way the guest clock:

1. Starts counting when KVM_SET_CLOCK executes.
2. With the value provided by userspace.

So this fixes rounding errors?

No. It just does the correct

"user_ns.clock = master_kernel_ns + kvmclock_offset = return value of get_kvmclock_ns(kvm) at moment of KVM_SET_CLOCK"

(that is, when guest_rdtsc() == tsc_timestamp, so guest_rdtsc() - tsc_timestamp = 0, you want get_kvmclock_ns() to return user_ns.clock).

Quoted 4 lines

arch/x86/kvm/x86.c line 4202

Quoted 2 lines

arch/x86/kvm/x86.c +2 -0

+			kvm_for_each_vcpu(i, vcpu, kvm)
+				kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);

kvm_gen_update_masterclock already does that, why did you move that to before the assignment?

Its after the assignment of kvmclock_offset because KVM_REQ_CLOCK_UPDATES processing by vcpus make use of kvmclock_offset.

So if you change that value, you have to request the vcpus to recalculate their kvmclock areas using the new kvmclock_offset.

Quoted 13 lines · Marcelo Tosatti

Quoted 10 lines

Quoted 8 lines

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).

This way the guest clock:

1. Starts counting when KVM_SET_CLOCK executes.
2. With the value provided by userspace.

So this fixes rounding errors?

No. It just does the correct

Quoted 5 lines

"user_ns.clock = master_kernel_ns + kvmclock_offset = return value of get_kvmclock_ns(kvm) at moment of KVM_SET_CLOCK"

(that is, when guest_rdtsc() == tsc_timestamp, so guest_rdtsc() - tsc_timestamp = 0, you want get_kvmclock_ns() to return user_ns.clock).

Yep, and it currently doesn't match because the current code introduces rounding errors?

Quoted 6 lines

Quoted 4 lines

arch/x86/kvm/x86.c line 4202

Quoted 2 lines

arch/x86/kvm/x86.c +2 -0

+			kvm_for_each_vcpu(i, vcpu, kvm)
+				kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);

kvm_gen_update_masterclock already does that, why did you move that to before the assignment?

Its after the assignment of kvmclock_offset because KVM_REQ_CLOCK_UPDATES processing by vcpus make use of kvmclock_offset.

Sure, but why did you move kvm_gen_update_masterclock before?

If you left kvm_gen_update_masterclock after the update of kvm->arch.kvmclock_offset, the duplicate KVM_REQ_CLOCK_UPDATE would not be necessary.

Paolo

Quoted 2 lines

So if you change that value, you have to request the vcpus to recalculate their kvmclock areas using the new kvmclock_offset.

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).

2017-05-03 10:43-0300, Marcelo Tosatti:

Quoted 3 lines

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:

arch/x86/kvm/x86.c +0 -0

index 464da936c53d..f024216a858d 100644

Quoted 3 lines

2017-05-03 10:43-0300, Marcelo Tosatti:
and the important fix for kvm master clock is the move of kvm_gen_update_masterclock() before we read the time.

Quoted 6 lines

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.

Quoted 19 lines

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:

arch/x86/kvm/x86.c +0 -0

index 464da936c53d..f024216a858d 100644

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp, · line 4180

Quoted 5 lines

arch/x86/kvm/x86.c +1 -1

@@ -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?

arch/x86/kvm/x86.c +0 -0

index 464da936c53d..8db1d09e59d7 100644
@@ -4175,9 +4175,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;

2017-05-11 12:39-0300, Marcelo Tosatti:

Quoted 9 lines · Radim Krčmář

Quoted 8 lines

2017-05-03 10:43-0300, Marcelo Tosatti:
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.

Quoted 4 lines

Quoted 2 lines

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.

I mean, why doesn't the patch look like this?

arch/x86/kvm/x86.c +0 -0

index 464da936c53d..8db1d09e59d7 100644

Quoted 10 lines

2017-05-11 12:39-0300, Marcelo Tosatti:

Quoted 9 lines · Radim Krčmář

Quoted 8 lines

2017-05-03 10:43-0300, Marcelo Tosatti:
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.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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.

I mean, why doesn't the patch look like this?

d

Quoted 5 lines

arch/x86/kvm/x86.c +0 -0

index 464da936c53d..8db1d09e59d7 100644

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp, · line 4182

Quoted 9 lines

arch/x86/kvm/x86.c +2 -1

@@ -4175,9 +4175,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 
 		r = 0;
+		kvm_gen_update_masterclock(kvm);
 		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;

        now_ns = ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)
        kvmclock_offset += user_ns.clock - (ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)
        kvmclock_offset = kvmclock_offset_prev + user_ns.clock - (ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)

In case of VM was just initialized before migration, kvmclock_offset_prev is -ktime_get_boot_ns()

        kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - (ka->master_kernel_ns -ktime_get_boot_ns() + grdtsc() - ka->master_cycle_now))

But master_kernel_ns = ktime_get_boot_ns() + delta-between-vm-init-and-KVM_SET_CLOCK (AKA delta)

                    (the same one from VM init) 

        kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - (ktime_get_boot_ns() + delta + -ktime_get_boot_ns() + grdtsc() - ka->master_cycle_now))

        kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - delta - grdtsc() + ka->master_cycle_now

But we don't want grdtsc() - ka->master_cycle_now in there.

Note: grdtsc() == guest read tsc.

Now with

(quoted diff) +2 -0

+			kvm->arch.kvmclock_offset = user_ns.clock -
+							ka->master_kernel_ns;

What happens is that guest clock starts counting, via kernel timekeeper, at the moment kvm_get_time_and_clockread() runs. If you add grdtsc() - ka->master_cycle_now in there, you are mindfully counting clock twice (first: kernel timekeeper, second: the TSC between the (grdtsc() - ka->master_cycle_now) in question.

(quoted diff) +1 -0

+			kvm->arch.kvmclock_offset =  -ktime_get_boot_ns() +user_ns.clock -delta

Note that (grdtsc() - ka->master_cycle_now) is susceptible to scheduling etc.

Makes sense?

2017-05-12 12:31-0300, Marcelo Tosatti:

Quoted 65 lines · Radim Krčmář

Quoted 10 lines

2017-05-11 12:39-0300, Marcelo Tosatti:

Quoted 9 lines · Radim Krčmář

Quoted 8 lines

2017-05-03 10:43-0300, Marcelo Tosatti:
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.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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.

I mean, why doesn't the patch look like this?

d

Quoted 5 lines

arch/x86/kvm/x86.c +0 -0

index 464da936c53d..8db1d09e59d7 100644

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp, · line 4182

Quoted 9 lines

arch/x86/kvm/x86.c +2 -1

@@ -4175,9 +4175,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 
 		r = 0;
+		kvm_gen_update_masterclock(kvm);
 		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;

        now_ns = ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)
        kvmclock_offset += user_ns.clock - (ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)
        kvmclock_offset = kvmclock_offset_prev + user_ns.clock - (ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)

In case of VM was just initialized before migration, kvmclock_offset_prev is -ktime_get_boot_ns()

        kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - (ka->master_kernel_ns -ktime_get_boot_ns() + grdtsc() - ka->master_cycle_now))

But master_kernel_ns = ktime_get_boot_ns() + delta-between-vm-init-and-KVM_SET_CLOCK (AKA delta)

                    (the same one from VM init) 

        kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - (ktime_get_boot_ns() + delta + -ktime_get_boot_ns() + grdtsc() - ka->master_cycle_now))

        kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - delta - grdtsc() + ka->master_cycle_now

But we don't want grdtsc() - ka->master_cycle_now in there.

Note: grdtsc() == guest read tsc.

Now with

(quoted diff) +2 -0

+			kvm->arch.kvmclock_offset = user_ns.clock -
+							ka->master_kernel_ns;

What happens is that guest clock starts counting, via kernel timekeeper, at the moment kvm_get_time_and_clockread() runs. If you add grdtsc() - ka->master_cycle_now in there, you are mindfully counting clock twice (first: kernel timekeeper, second: the TSC between the (grdtsc() - ka->master_cycle_now) in question.

(quoted diff) +1 -0

+			kvm->arch.kvmclock_offset =  -ktime_get_boot_ns() +user_ns.clock -delta

Quoted 4 lines

Note that (grdtsc() - ka->master_cycle_now) is susceptible to scheduling etc.

Makes sense?

Yes. The simpler code starts the kvmclock a bit later, but both are correct -- anything within KVM_SET_CLOCK runtime is.

If we care about accuracy, then we should let userspace provide a (kernel timestamp, kvm timestamp) pair, so the value of kvmclock can really be controlled.

Adding ugly optimizations to work around shortcomings of the API is going the wrong way ...

Quoted 17 lines

2017-05-12 12:31-0300, Marcelo Tosatti:

Quoted 16 lines

Now with

(quoted diff) +2 -0

+			kvm->arch.kvmclock_offset = user_ns.clock -
+							ka->master_kernel_ns;

What happens is that guest clock starts counting, via kernel timekeeper, at the moment kvm_get_time_and_clockread() runs. If you add grdtsc() - ka->master_cycle_now in there, you are mindfully counting clock twice (first: kernel timekeeper, second: the TSC between the (grdtsc() - ka->master_cycle_now) in question.

(quoted diff) +1 -0

+			kvm->arch.kvmclock_offset =  -ktime_get_boot_ns() +user_ns.clock -delta

Note that (grdtsc() - ka->master_cycle_now) is susceptible to scheduling etc.

Quoted 4 lines

Quoted 1 line

Makes sense?

Yes. The simpler code starts the kvmclock a bit later, but both are correct -- anything within KVM_SET_CLOCK runtime is.

No the simpler code is not correct. You count time with two clocks for a small period of time.

KVM_SET_CLOCK means: set the guest clock to the passed value and start counting it from there.

With the simple fix, KVM_SET_CLOCK does: set the guest clock to the passed value, but also add the delta between kvm_get_time_and_clockread() and get_kvmclock_ns().

Which is variable, due to scheduling.

So it is just wrong.

Quoted 3 lines

If we care about accuracy, then we should let userspace provide a (kernel timestamp, kvm timestamp) pair, so the value of kvmclock can really be controlled.

I suppose something else has to be done: the control of the clock, from whatever userspace is using to measure passage of time, to TSC, has to be done in kernel.

But lets see if that is really necessary when the QEMU PTP/CLOCK_MONOTONIC delta stuff is done (working on it).

In the meantime, do you have anything against this patch? I depend on it for the work above.

I depend on
it for the work above.

Quoted 2 lines

Adding ugly optimizations to work around shortcomings of the API is going the wrong way ...

What optimization you refer to?

2017-05-13 00:46-0300, Marcelo Tosatti:

Quoted 18 lines · Radim Krčmář

Quoted 17 lines

2017-05-12 12:31-0300, Marcelo Tosatti:

Quoted 16 lines

Now with

(quoted diff) +2 -0

+			kvm->arch.kvmclock_offset = user_ns.clock -
+							ka->master_kernel_ns;

What happens is that guest clock starts counting, via kernel timekeeper, at the moment kvm_get_time_and_clockread() runs. If you add grdtsc() - ka->master_cycle_now in there, you are mindfully counting clock twice (first: kernel timekeeper, second: the TSC between the (grdtsc() - ka->master_cycle_now) in question.

(quoted diff) +1 -0

+			kvm->arch.kvmclock_offset =  -ktime_get_boot_ns() +user_ns.clock -delta

Note that (grdtsc() - ka->master_cycle_now) is susceptible to scheduling etc.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Makes sense?

Yes. The simpler code starts the kvmclock a bit later, but both are correct -- anything within KVM_SET_CLOCK runtime is.

No the simpler code is not correct. You count time with two clocks for a small period of time.

A clock that counts kernel-nanoseconds is instantly replaced by a clock that counts masterclock-nanoseconds, not incorrect by itself.

The simpler code will get the same kvmclock_offset as your code where kvm_get_time_and_clockread() is called a bit later.
The distribution of resulting kvm_offsets will differ, but they must both be correct or both incorrect, because they are already off-mark.

Quoted 2 lines

KVM_SET_CLOCK means: set the guest clock to the passed value and start counting it from there.

Which is exactly what both versions do.

Quoted 5 lines

With the simple fix, KVM_SET_CLOCK does: set the guest clock to the passed value, but also add the delta between kvm_get_time_and_clockread() and get_kvmclock_ns().

Which is variable, due to scheduling.

Yes.

Quoted 1 line

So it is just wrong.

It makes the matter slightly worse by adding some execution time, but the whole interface is "just wrong" even without that: we already have the variability of the time between userspace's decision on user_ns.clock value and kvm_get_time_and_clockread().

Quoted 7 lines

Quoted 3 lines

If we care about accuracy, then we should let userspace provide a (kernel timestamp, kvm timestamp) pair, so the value of kvmclock can really be controlled.

I suppose something else has to be done: the control of the clock, from whatever userspace is using to measure passage of time, to TSC, has to be done in kernel.

We agree, just worded it differently.

Quoted 2 lines

But lets see if that is really necessary when the QEMU PTP/CLOCK_MONOTONIC delta stuff is done (working on it).

Right.

Quoted 2 lines

In the meantime, do you have anything against this patch? I depend on it for the work above.

The reasoning provided with the patch does not explain why

* kvmclock_offset must be adjusted so that
* user_ns.clock = master_kernel_ns + kvmclock_offset

Please explain why it "must". I assert that it does not have to be.

If we agree that it is not necessary, then it is an optimization and I'd like numbers to show that we are getting something that balances the obfuscation; and why do we want it if we don't care about the better solution (discussed above).

Quoted 2 lines

I depend on
it for the work above.

Describing how other code couldn't work without this is great reason, but again, please be specific -- what difference it make?

Quoted 4 lines

Quoted 2 lines

Adding ugly optimizations to work around shortcomings of the API is going the wrong way ...

What optimization you refer to?

I refer to everything on top of the second hunk I posted.

Thanks.