On Mon, Nov 28, 2016 at 02:47:18PM +0100, Paolo Bonzini wrote:
>
>
> On 17/11/2016 13:16, Marcelo Tosatti wrote:
> > 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.
>
> I still cannot understand this case.
>
> If the source has masterclock _disabled_, shouldn't it read kvmclock
> from memory? But that's not what your patch does. To be clear, what
> I mean is:
>
> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> index 653b0b4..6f6e2dc 100644
> --- a/hw/i386/kvm/clock.c
> +++ b/hw/i386/kvm/clock.c
> @@ -97,6 +97,7 @@ static uint64_t kvm_get_clock(void)
> fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
> abort();
> }
> + s->src_use_reliable_get_clock = data.flags & KVM_CLOCK_TSC_STABLE;
I still don't understand the reasoning behind
kvm_has_adjust_clock_stable() vs (flags & KVM_CLOCK_TSC_STABLE),
but on either case, updating src_use_reliable_get_clock inside
kvm_get_clock() looks like the right thing to do.
> return data.clock;
> }
>
> @@ -110,34 +111,19 @@ static void kvmclock_vm_state_change(void *opaque, int
> running,
>
> if (running) {
> struct kvm_clock_data data = {};
> - uint64_t pvclock_via_mem = 0;
>
> - /* local (running VM) restore */
> - if (s->clock_valid) {
> - /*
> - * if host does not support reliable KVM_GET_CLOCK,
> - * read kvmclock value from memory
> - */
> - if (!kvm_has_adjust_clock_stable()) {
> - pvclock_via_mem = kvmclock_current_nsec(s);
> - }
> - /* migration/savevm/init restore */
> - } else {
> - /*
> - * use s->clock in case machine uses reliable
> - * get clock and source host supported
> - * reliable get clock
> - */
> - if (!s->src_use_reliable_get_clock) {
> - pvclock_via_mem = kvmclock_current_nsec(s);
> + /*
> + * if last KVM_GET_CLOCK did not retrieve a reliable value,
> + * we can't rely on the saved clock value. Just discard it and
> + * read kvmclock value from memory
> + */
> + if (!s->src_use_reliable_get_clock) {
> + uint64_t pvclock_via_mem = kvmclock_current_nsec(s);
> + if (pvclock_via_mem) {
> + s->clock = pvclock_via_mem;
> }
> }
This is equivalent to the logic I suggested on my reply to v3.
Much simpler.
>
> - /* We can't rely on the saved clock value, just discard it */
> - if (pvclock_via_mem) {
> - s->clock = pvclock_via_mem;
> - }
> -
> s->clock_valid = false;
>
> data.clock = s->clock;
> @@ -181,16 +168,6 @@ static void kvmclock_realize(DeviceState *dev, Error
> **errp)
> {
> KVMClockState *s = KVM_CLOCK(dev);
>
> - /*
> - * 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;
> - }
> -
> qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
> }
>
--
Eduardo
