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

[Marcelo Tosatti <mtosatti@redhat.com>]

On Mon, May 15, 2017 at 06:19:57PM +0200, Radim Krčmář wrote:
> 2017-05-13 00:46-0300, Marcelo Tosatti:
> > On Fri, May 12, 2017 at 07:37:12PM +0200, Radim Krčmář wrote:
> > > 2017-05-12 12:31-0300, Marcelo Tosatti:
> > > > Now with
> > > > 
> > > > +			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.
> > > > 
> > > > +			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?
> > > 
> > > 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.
> 
> > 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.
> 
> > 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.
> 
> > 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().
> 
> >> 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.
> 
> > But lets see if that is really necessary when the QEMU
> > PTP/CLOCK_MONOTONIC delta stuff is done (working on it).
> 
> Right.
> 
> > 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).
> 
> >                                                           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?
> 
> >> 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.

Actually you are right, your patch is fine because the 
length of time between kvm_get_time_and_clockread()
and get_kvmclock_ns(kvm) is compensated by

- grdtsc() + ka->master_cycle_now = 
- ( +grdtsc() - ka->master_cycle_now)

Which is the length of time between kvm_get_time_and_clockread() 
and get_kvmclock_ns(kvm).

Its much cleaner indeed. Can you please apply it? Thanks.