Re: [PATCH kvm-unit-tests] KVM: x86: add hyperv clock test case

[Marcelo Tosatti <mtosatti@redhat.com>]

On Wed, May 25, 2016 at 09:33:07PM +0300, Roman Kagan wrote:
> On Tue, Apr 26, 2016 at 01:34:56PM +0300, Roman Kagan wrote:
> > On Mon, Apr 25, 2016 at 11:47:23AM +0300, Roman Kagan wrote:
> > > On Fri, Apr 22, 2016 at 08:08:47PM +0200, Paolo Bonzini wrote:
> > > > On 22/04/2016 15:32, Roman Kagan wrote:
> > > > > The first value is derived from the kvm_clock's tsc_to_system_mul and
> > > > > tsc_shift, and matches hosts's vcpu->hw_tsc_khz.  The second is
> > > > > calibrated using emulated HPET.  The difference is those +14 ppm.
> > > > > 
> > > > > This is on i7-2600, invariant TSC present, TSC scaling not present.
> > > > > 
> > > > > I'll dig further but I'd appreciate any comment on whether it was within
> > > > > tolerance or not.
> > > > 
> > > > The solution to the bug is to change the Hyper-V reference time MSR to
> > > > use the same formula as the Hyper-V TSC-based clock.  Likewise,
> > > > KVM_GET_CLOCK and KVM_SET_CLOCK should not use ktime_get_ns().
> > > 
> > > Umm, I'm not sure it's a good idea...
> > > 
> > > E.g. virtualized HPET sits in userspace and thus uses
> > > clock_gettime(CLOCK_MONOTONIC), so the drift will remain.
> > > 
> > > AFAICT the root cause is the following: KVM master clock uses the same
> > > multiplier/shift as the vsyscall time in host userspace.  However, the
> > > offsets in vsyscall_gtod_data get updated all the time with corrections
> > > from NTP and so on.  Therefore even if the TSC rate is somewhat
> > > miscalibrated, the error is kept small in vsyscall time functions.  OTOH
> > > the offsets in KVM clock are basically never updated, so the error keeps
> > > linearly growing over time.
> > 
> > This seems to be due to a typo:

Its not a typo, the code only updated the notifier on 
VCLOCK_TSC -> !VCLOCK_TSC transition.

> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -5819,7 +5819,7 @@ static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
> >         /* disable master clock if host does not trust, or does not
> >          * use, TSC clocksource
> >          */
> > -       if (gtod->clock.vclock_mode != VCLOCK_TSC &&
> > +       if (gtod->clock.vclock_mode == VCLOCK_TSC &&
> >             atomic_read(&kvm_guest_has_master_clock) != 0)
> >                 queue_work(system_long_wq, &pvclock_gtod_work);
> > 
> > 
> > as a result, the global pvclock_gtod_data was kept up to date, but the
> > requests to update per-vm copies were never issued.
> > 
> > With the patch I'm now seeing different test failures which I'm looking
> > into.

The queue_work is not enough: it opens a window where guest clock
(via shared memory) and CLOCK_GETTIME can go out of sync.

> > 
> > Meanwhile I'm wondering if this scheme is not too costly: on my machine
> > pvclock_gtod_notify() is called at kHz rate, and the work it schedules
> > does
> > 
> > static void pvclock_gtod_update_fn(struct work_struct *work)
> > {
> > [...]
> >         spin_lock(&kvm_lock);
> >         list_for_each_entry(kvm, &vm_list, vm_list)
> >                 kvm_for_each_vcpu(i, vcpu, kvm)
> >                         kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
> >         atomic_set(&kvm_guest_has_master_clock, 0);
> >         spin_unlock(&kvm_lock);
> > }
> > 
> > KVM_REQ_MASTERCLOCK_UPDATE makes all VCPUs synchronize:
> > 
> > static void kvm_gen_update_masterclock(struct kvm *kvm)
> > {
> > [...]
> >         spin_lock(&ka->pvclock_gtod_sync_lock);
> >         kvm_make_mclock_inprogress_request(kvm);
> >         /* no guest entries from this point */
> >         pvclock_update_vm_gtod_copy(kvm);
> > 
> >         kvm_for_each_vcpu(i, vcpu, kvm)
> >                 kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> > 
> >         /* guest entries allowed */
> >         kvm_for_each_vcpu(i, vcpu, kvm)
> >                 clear_bit(KVM_REQ_MCLOCK_INPROGRESS, &vcpu->requests);
> > 
> >         spin_unlock(&ka->pvclock_gtod_sync_lock);
> > [...]
> > }
> > 
> > so on a host with many VMs it may become an issue.
> 
> Ping
> 
> Roman.

1) Can call notifier only when frequency changes.
2) Can calculate how much drift between clocks and do not allow guest
entry.

Will post a patch soon, one or two weeks max (again, independent of your patchset).