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

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

2017-05-11 12:39-0300, Marcelo Tosatti:
> On Wed, May 10, 2017 at 08:04:31PM +0200, Radim Krčmář wrote:
> > 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.
> > 
> > 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?

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 464da936c53d..8db1d09e59d7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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;
 	}
 	case KVM_GET_CLOCK: {

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

That doesn't explain the need for kvm_gen_update_masterclock().
kvm_gen_update_masterclock() moves the master clock base to the current
kernel boot clock; the non-master clock case uses the same kernel boot
clock to return the current kvmclock -- the computed offset is the same
in both cases.

Updating the master clock simplifies the case by resetting possible
divergence (due to different frequencies) between master clock and
kernel boot clock, but I'd still like to know (and document) if that is
the reason.

In what test does updating the master clock make a difference?

Thanks.