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

Commit Message

Radim Krčmář May 10, 2017, 6:04 p.m. UTC

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:


Thanks.

Comments

Marcelo Tosatti May 11, 2017, 3:39 p.m. UTC | #1

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?

Patch

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: {