On Mon, Nov 14, 2016 at 04:15:18PM -0200, Marcelo Tosatti wrote:
> On Mon, Nov 14, 2016 at 06:20:29PM +0100, Paolo Bonzini wrote:
> >
> >
> > On 14/11/2016 18:13, Marcelo Tosatti wrote:
> > > On Mon, Nov 14, 2016 at 05:43:33PM +0100, Paolo Bonzini wrote:
> > >>
> > >>
> > >> On 14/11/2016 16:40, Marcelo Tosatti wrote:
> > >>> static bool kvmclock_src_use_reliable_get_clock(void *opaque)
> > >>> {
> > >>> KVMClockState *s = opaque;
> > >>>
> > >>> /*
> > >>> * On machine types that support reliable KVM_GET_CLOCK,
> > >>> * if host kernel does provide reliable KVM_GET_CLOCK,
> > >>> * set src_use_reliable_get_clock=true so that destination
> > >>> * avoids reading kvmclock from memory.
> > >>> */
> > >>> if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable())
> > >>> {
> > >>> s->src_use_reliable_get_clock = true;
> > >>> }
> > >>>
> > >>> return s->mach_use_reliable_get_clock;
> > >>> }
> > >>>
> > >>>
> > >>> Ah, OK, done.
> > >>
> > >> s->src_use_reliable_get_clock should not be set with
> > >> KVM_CHECK_EXTENSION, but rather from the flags returned by KVM_GET_CLOCK.
> > >
> > > Well, thats not right: What matters is the presence of get_kvmclock_ns
> > > which returns a value that the guest sees.
> > >
> > > get_kernel_monotonic_clock() + kvmclock_offset +
> > > (rdtsc() - tsc_timestamp)
> > >
> > > IOW what the guest sees. And you changed that in
> > >
> > > commit 108b249c453dd7132599ab6dc7e435a7036c193f
> > > Author: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > > Date: Thu Sep 1 14:21:03 2016 +0200
> > >
> > > KVM: x86: introduce get_kvmclock_ns
> > >
> > > And the correct behaviour (once KVM_GET_CLOCK is fixed per
> > > previous message to return rdtsc - tsc_timestamp for the
> > > non masterclock case) depends on this commit above,
> > > not on masterclock.
> >
> > This commit in turn only gets the correct behavior if
> > "vcpu->arch.hv_clock.flags & PVCLOCK_TSC_STABLE_BIT" (and it will be
> > changed soon to ka->use_masterclock). KVM_CHECK_EXTENSION can still
> > return KVM_CLOCK_TSC_STABLE even if the masterclock is disabled,
> > because KVM_CHECK_EXTENSION only tells you which flags are known for
> > this version of the KVM module.
>
> What QEMU wants is to use KVM_GET_CLOCK at pre_save independently
> of whether masterclock is enabled or not... it just depends
> on KVM_GET_CLOCK being correct for the masterclock case
> (108b249c453dd7132599ab6dc7e435a7036c193f).
>
> So a "reliable KVM_GET_CLOCK" (that does not timebackward
> when masterclock is enabled) is much simpler to userspace
> than "whether masterclock is enabled or not".
>
> If you have a reason why that should not be the case,
> let me know.
>
> > To see if the masterclock is enabled _now_, you need to check what
> > KVM_GET_CLOCK sets in the flags. From the KVM_CLOCK_TSC_STABLE patch:
> >
> > user_ns.flags = kvm->arch.use_master_clock ? KVM_CLOCK_TSC_STABLE : 0;
>
> Again, whether masterclock is enable is independent of
> being able to use KVM_GET_CLOCK at pre_save.
Is this point OK ?
Using
break;
+ case KVM_CAP_ADJUST_CLOCK:
+ r = KVM_CLOCK_TSC_STABLE;
+ break;
To infer whether KVM_GET_CLOCK is fixed for the monotonic case.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
