improve kvmclock difference on migration

hw/i386/kvm/clock.c +3 -1

+            }
+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
+        /* We can't rely on the saved clock value, just discard it */
         if (time_at_migration) {
             s->clock = time_at_migration;

hw/i386/kvm/clock.c +6 -0

+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,

hw/i386/kvm/clock.c line 109

Quoted 8 lines · Marcelo Tosatti

hw/i386/kvm/clock.c +8 -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()) {
+                time_at_migration = kvmclock_current_nsec(s);

Just assign to s->clock here...

hw/i386/kvm/clock.c line 120

Quoted 11 lines

hw/i386/kvm/clock.c +11 -0

+            }
+        /* migration/savevm/init restore */
+        } else {
+            /*
+             * use s->clock in case machine uses reliable
+             * get clock and host where vm was executing
+             * supported reliable get clock
+             */
+            if (!s->mach_use_reliable_get_clock ||
+                !s->src_use_reliable_get_clock) {
+                time_at_migration = kvmclock_current_nsec(s);

... and here, so that time_at_migration is not needed anymore.

Also here it's enough to look at s->src_user_reliable_get_clock, because if s->mach_use_reliable_get_clock is false, s->src_use_reliable_get_clock will be false as well.

Quoted 7 lines

hw/i386/kvm/clock.c +3 -1

+            }
+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
+        /* We can't rely on the saved clock value, just discard it */
         if (time_at_migration) {
             s->clock = time_at_migration;

[...]

Quoted 11 lines

hw/i386/kvm/clock.c +10 -0

+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.
+     */

hw/i386/kvm/clock.c line 198

Quoted 6 lines

hw/i386/kvm/clock.c +6 -0

+    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
+        s->src_use_reliable_get_clock = true;
+    }
+
+    return s->src_use_reliable_get_clock;
+}

Here you can just return s->mach_use_reliable_get_clock.

To set
s->src_use_reliable_get_clock, after issuing KVM_GET_CLOCK you can look at the KVM_CLOCK_TSC_STABLE bit in the kvm_clock struct's flags.

You don't actually need kvm_has_adjust_clock_stable(), but please place the KVM_GET_CLOCK code for kvmclock_pre_save and kvmclock_vm_state_change in a common function.

Also, just another small nit: please make your scripts use the "-p" option on diff. :)

Thanks,

Paolo

Quoted 12 lines · Paolo Bonzini

hw/i386/kvm/clock.c line 109

Quoted 8 lines · Marcelo Tosatti

hw/i386/kvm/clock.c +8 -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()) {
+                time_at_migration = kvmclock_current_nsec(s);

Quoted 1 line

Just assign to s->clock here...

If kvmclock is not enabled, you want to use s->clock, rather than 0.

Quoted 12 lines

Quoted 11 lines

hw/i386/kvm/clock.c +11 -0

+            }
+        /* migration/savevm/init restore */
+        } else {
+            /*
+             * use s->clock in case machine uses reliable
+             * get clock and host where vm was executing
+             * supported reliable get clock
+             */
+            if (!s->mach_use_reliable_get_clock ||
+                !s->src_use_reliable_get_clock) {
+                time_at_migration = kvmclock_current_nsec(s);

Quoted 1 line

... and here, so that time_at_migration is not needed anymore.

Same as above.

Quoted 3 lines

Also here it's enough to look at s->src_user_reliable_get_clock, because if s->mach_use_reliable_get_clock is false, s->src_use_reliable_get_clock will be false as well.

Yes, but i like the code annotation.

Quoted 21 lines

Quoted 7 lines

hw/i386/kvm/clock.c +3 -1

+            }
+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
+        /* We can't rely on the saved clock value, just discard it */
         if (time_at_migration) {
             s->clock = time_at_migration;

[...]

Quoted 11 lines

hw/i386/kvm/clock.c +10 -0

+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.
+     */

Quoted 8 lines

hw/i386/kvm/clock.c line 198

Quoted 6 lines

hw/i386/kvm/clock.c +6 -0

+    if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
+        s->src_use_reliable_get_clock = true;
+    }
+
+    return s->src_use_reliable_get_clock;
+}

Here you can just return s->mach_use_reliable_get_clock.

mach_use_reliable_get_clock can be true but host might not support it.

Quoted 3 lines

To set
s->src_use_reliable_get_clock, after issuing KVM_GET_CLOCK you can look at the KVM_CLOCK_TSC_STABLE bit in the kvm_clock struct's flags.

KVM_CLOCK_TSC_STABLE bit in the kvmclock structure != KVM_GET_CLOCK returns reliable value, right?

Quoted 30 lines · Marcelo Tosatti

Quoted 12 lines · Paolo Bonzini

hw/i386/kvm/clock.c line 109

Quoted 8 lines · Marcelo Tosatti

hw/i386/kvm/clock.c +8 -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()) {
+                time_at_migration = kvmclock_current_nsec(s);

Quoted 1 line

Just assign to s->clock here...

If kvmclock is not enabled, you want to use s->clock, rather than 0.

Quoted 12 lines

Quoted 11 lines

hw/i386/kvm/clock.c +11 -0

+            }
+        /* migration/savevm/init restore */
+        } else {
+            /*
+             * use s->clock in case machine uses reliable
+             * get clock and host where vm was executing
+             * supported reliable get clock
+             */
+            if (!s->mach_use_reliable_get_clock ||
+                !s->src_use_reliable_get_clock) {
+                time_at_migration = kvmclock_current_nsec(s);

Quoted 3 lines

Quoted 1 line

... and here, so that time_at_migration is not needed anymore.

Same as above.

You're right.

Quoted 5 lines

Quoted 3 lines

Also here it's enough to look at s->src_user_reliable_get_clock, because if s->mach_use_reliable_get_clock is false, s->src_use_reliable_get_clock will be false as well.

Yes, but i like the code annotation.

Ah, I think we're looking at it differently.

I'm thinking "mach_use_reliable_get_clock is just for migration,

src_use_reliable_get_clock is the state".

Perhaps you're thinking of
enabling/disabling the whole new code for old machines?

What is the
advantage?

Quoted 31 lines

Quoted 30 lines

Quoted 8 lines

hw/i386/kvm/clock.c +3 -1

+            }
+        }
 
-        /* We can't rely on the migrated clock value, just discard it */
+        /* We can't rely on the saved clock value, just discard it */
         if (time_at_migration) {
             s->clock = time_at_migration;

[...]

Quoted 7 lines

hw/i386/kvm/clock.c +6 -0

+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,

hw/i386/kvm/clock.c line 198

Quoted 10 lines

hw/i386/kvm/clock.c +10 -0

+     * 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->src_use_reliable_get_clock;
+}

Here you can just return s->mach_use_reliable_get_clock.

Quoted 1 line

mach_use_reliable_get_clock can be true but host might not support it.

Yes, but the "needed" function is only required to avoid breaking pc-i440fx-2.7 and earlier.

If you return true here, you can still
migrate a "false" value for src_use_reliable_get_clock.

Quoted 6 lines

Quoted 3 lines

To set
s->src_use_reliable_get_clock, after issuing KVM_GET_CLOCK you can look at the KVM_CLOCK_TSC_STABLE bit in the kvm_clock struct's flags.

KVM_CLOCK_TSC_STABLE bit in the kvmclock structure != KVM_GET_CLOCK returns reliable value, right?

It is the same as "is using masterclock", which is actually a stricter condition than the KVM_CHECK_EXTENSION return value. The right check to use is whether masterclock is in use,

and then the idea is to treat clock,src_use_reliable_get_clock as one tuple that is updated atomically.

Paolo

Well, i didnt want to mix the meaning of the variables:

hw/i386/kvm/clock.c +5 -0

+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;

See the comments on top (later if you look at the variable, then have to think: well it has one name, but its disabled by that other path as well, so its more than its name,etc...).

Quoted 1 line

I'm thinking "mach_use_reliable_get_clock is just for migration,

Thats whether the machine supports it. New machines have it enabled, olders don't.

Quoted 1 line

src_use_reliable_get_clock is the state".

Thats whether the migration source supported it.

Quoted 19 lines

Quoted 19 lines

Quoted 18 lines

Quoted 6 lines

hw/i386/kvm/clock.c +6 -0

+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,

hw/i386/kvm/clock.c line 198

Quoted 10 lines

hw/i386/kvm/clock.c +10 -0

+     * 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->src_use_reliable_get_clock;
+}

Here you can just return s->mach_use_reliable_get_clock.

Quoted 4 lines

Quoted 1 line

mach_use_reliable_get_clock can be true but host might not support it.

Yes, but the "needed" function is only required to avoid breaking pc-i440fx-2.7 and earlier.

"needed" is required so that the migration between:

SRC             DEST                BEHAVIOUR
~support        supports            on migration read from guest,

on stop/cont use kvm_get_clock/kvm_set_clock

Destination does not use KVM_GET_CLOCK value (which is broken and should not be used).

Quoted 2 lines

If you return true here, you can still
migrate a "false" value for src_use_reliable_get_clock.

But the source only uses a reliable KVM_GET_CLOCK if both conditions are true.

And the subsection is only needed if the source uses a reliable KVM_GET_CLOCK.

Quoted 3 lines

It is the same as "is using masterclock", which is actually a stricter condition than the KVM_CHECK_EXTENSION return value. The right check to use is whether masterclock is in use,

Actually its "has a reliable KVM_GET_CLOCK" (which returns get_kernel_clock() + (rdtsc() - tsc_timestamp),

"broken KVM_GET_CLOCK" = get_kernel_clock()

Quoted 13 lines · Marcelo Tosatti

Well, i didnt want to mix the meaning of the variables:

(quoted diff) +5 -0

+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;

See the comments on top (later if you look at the variable, then have to think: well it has one name, but its disabled by that other path as well, so its more than its name,etc...).

Quoted 4 lines

Quoted 1 line

I'm thinking "mach_use_reliable_get_clock is just for migration,

Thats whether the machine supports it. New machines have it enabled, olders don't.

Yes.

Quoted 3 lines

Quoted 1 line

src_use_reliable_get_clock is the state".

Thats whether the migration source supported it.

But it's not used only for migration. It's used on every vmstate change (running->stop and stop->running, isn't it?

I think that, apart from
the migration case, it's better to use s->clock if kvmclock is stable, even on older machine types.

Quoted 31 lines

Quoted 19 lines

Quoted 19 lines

Quoted 18 lines

Quoted 6 lines

(quoted diff) +6 -0

+static bool kvmclock_src_use_reliable_get_clock(void *opaque)
+{
+    KVMClockState *s = opaque;
+
+    /*
+     * On machine types that support reliable KVM_GET_CLOCK,

Review

Quoted 10 lines

(quoted diff) +10 -0

+     * 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->src_use_reliable_get_clock;
+}

Here you can just return s->mach_use_reliable_get_clock.

Quoted 4 lines

Quoted 1 line

mach_use_reliable_get_clock can be true but host might not support it.

Yes, but the "needed" function is only required to avoid breaking pc-i440fx-2.7 and earlier.

"needed" is required so that the migration between:

SRC             DEST                BEHAVIOUR
~support        supports            on migration read from guest,

on stop/cont use kvm_get_clock/kvm_set_clock

Quoted 2 lines

Destination does not use KVM_GET_CLOCK value (which is broken and should not be used).

If needed returns false, the destination will see
src_use_reliable_get_clock = false anyway.

If needed returns true, the destination can still see src_use_reliable_get_clock = false if that's the value.

needed src_use_reliable_get_clock effect
false false use kvmclock_current_nsec
false true use kvmclock_current_nsec
true false use kvmclock_current_nsec
true true use s->clock

So the idea is:

- set s->clock and s->reliable_get_clock on every KVM_GET_CLOCK

- on migration source, use subsections and s->mach_use_reliable_get_clock to avoid breaking migration on pre-2.8 machine types

- on migration destination, use .pre_load so that s->reliable_get_clock is initialized to false on older machine types

Quoted 4 lines

Quoted 3 lines

If you return true here, you can still
migrate a "false" value for src_use_reliable_get_clock.

Quoted 5 lines

But the source only uses a reliable KVM_GET_CLOCK if both conditions are true.

And the subsection is only needed if the source uses a reliable KVM_GET_CLOCK.

Yes, but the point of the subsection is just to avoid breaking migration format.

Quoted 8 lines

Quoted 3 lines

It is the same as "is using masterclock", which is actually a stricter condition than the KVM_CHECK_EXTENSION return value. The right check to use is whether masterclock is in use,

Actually its "has a reliable KVM_GET_CLOCK" (which returns get_kernel_clock() + (rdtsc() - tsc_timestamp),

"broken KVM_GET_CLOCK" = get_kernel_clock()

Yes. And these end up being the same.

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.

So s->src_use_reliable_get_clock is only used to indicate to the destination that: "you can use KVM_GET_CLOCK value, its safe".

Quoted 15 lines · Marcelo Tosatti

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;
    }

Quoted 5 lines

    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.

Quoted 3 lines

So s->src_use_reliable_get_clock is only used to indicate to the destination that: "you can use KVM_GET_CLOCK value, its safe".

Yes, we agree. I was listing all the points, not just those where we disagree. Actually I'm not sure where we disagree, except on using flags from KVM_CHECK_EXTENSION vs. flags from KVM_GET_CLOCK...

Paolo

Quoted 18 lines · Paolo Bonzini

Quoted 15 lines · Marcelo Tosatti

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;
    }

Quoted 8 lines

Quoted 5 lines

    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 <[email protected]>
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.

Quoted 9 lines

Quoted 3 lines

So s->src_use_reliable_get_clock is only used to indicate to the destination that: "you can use KVM_GET_CLOCK value, its safe".

Yes, we agree. I was listing all the points, not just those where we disagree. Actually I'm not sure where we disagree, except on using flags from KVM_CHECK_EXTENSION vs. flags from KVM_GET_CLOCK...

Paolo

Quoted 40 lines · Marcelo Tosatti

Quoted 18 lines · Paolo Bonzini

Quoted 15 lines · Marcelo Tosatti

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;
    }

Quoted 8 lines

Quoted 5 lines

    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 <[email protected]>
Date:   Thu Sep 1 14:21:03 2016 +0200

Quoted 6 lines

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.

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;

Quoted 50 lines · Paolo Bonzini

Quoted 40 lines · Marcelo Tosatti

Quoted 18 lines · Paolo Bonzini

Quoted 15 lines · Marcelo Tosatti

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;
    }

Quoted 8 lines

Quoted 5 lines

    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 <[email protected]>
Date:   Thu Sep 1 14:21:03 2016 +0200

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 70 lines · Marcelo Tosatti

Quoted 50 lines · Paolo Bonzini

Quoted 40 lines · Marcelo Tosatti

Quoted 18 lines · Paolo Bonzini

Quoted 15 lines · Marcelo Tosatti

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;
    }

Quoted 8 lines

Quoted 5 lines

    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 <[email protected]>
Date:   Thu Sep 1 14:21:03 2016 +0200

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 4 lines

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;

(quoted diff) +3 -0

+   case KVM_CAP_ADJUST_CLOCK:
+               r = KVM_CLOCK_TSC_STABLE;
+               break;

To infer whether KVM_GET_CLOCK is fixed for the monotonic case.

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.

Quoted 11 lines · Marcelo Tosatti

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:

hw/i386/kvm/clock.c +0 -0

index 653b0b4..6f6e2dc 100644

hw/i386/kvm/clock.c +1 -19

     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);
+        /*

hw/i386/kvm/clock.c +0 -15

-        /* 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);
 }
 

Quoted 1 line

+ 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.

Quoted 9 lines · Marcelo Tosatti

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".

Quoted 2 lines

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:

hw/i386/kvm/clock.c +10 -34

index 653b0b4..6f6e2dc 100644
@@ -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;
     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;
             }
         }
 
-        /* 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);
 }
 

If the source masterclock is disabled, then the guest does not enable the optimization to not use a global variable to guarantee monotonicity. Therefore there will be no time backwards events (the timer backwards events crashed guests, and are the reason for reading from guest memory).

So if there are no flaws in the reasoning above, no, there is no need to read from memory if masterclock is disabled.

Can you state the reasons why you think it should be enabled?

hw/i386/kvm/clock.c line 109

Quoted 8 lines · Marcelo Tosatti

hw/i386/kvm/clock.c +8 -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()) {
+                time_at_migration = kvmclock_current_nsec(s);

Just assign to s->clock here...

Also, just another small nit: please make your scripts use the "-p" option on diff. :)

Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which indicates that KVM_GET_CLOCK returns a value as seen by the guest at that moment.

For new machine types, use this value rather than reading from guest memory.

This reduces kvmclock difference on migration from 5s to 0.1s (when max_downtime == 5s).

Signed-off-by Marcelo Tosatti <[email protected]>

hw/i386/kvm/clock.c +6 -1

-        s->clock_valid = false;
+        /* local (running VM) restore */
+        if (s->clock_valid) {
+            /*
+             * if host does not support reliable KVM_GET_CLOCK,
+             * read kvmclock value from memory
+             */

Quoted 11 lines · Marcelo Tosatti

Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which indicates that KVM_GET_CLOCK returns a value as seen by the guest at that moment.

For new machine types, use this value rather than reading from guest memory.

This reduces kvmclock difference on migration from 5s to 0.1s (when max_downtime == 5s).

Signed-off-by Marcelo Tosatti <[email protected]>

Acked-by Juan Quintela <[email protected]>

But, if you have to respin it ....

hw/i386/kvm/clock.c line 44

Quoted 5 lines

hw/i386/kvm/clock.c +5 -0

+    /* whether machine supports reliable KVM_GET_CLOCK */
+    bool mach_use_reliable_get_clock;
+
+    /* whether source host supported reliable KVM_GET_CLOCK */
+    bool src_use_reliable_get_clock;

This two names are really long, but I don't have better suggesitons :-()

hw/i386/kvm/clock.c line 100

Quoted 4 lines

     if (running) {
         struct kvm_clock_data data = {};

hw/i386/kvm/clock.c +1 -1

-        uint64_t time_at_migration = kvmclock_current_nsec(s);
+        uint64_t time_at_migration = 0;

This was not "time_at_migration", it was not already before, but just now it looks really weird. (as it was already faulty, this is why it is only a suggestion.)

Quoted 8 lines

hw/i386/kvm/clock.c +6 -1

-        s->clock_valid = false;
+        /* local (running VM) restore */
+        if (s->clock_valid) {
+            /*
+             * if host does not support reliable KVM_GET_CLOCK,
+             * read kvmclock value from memory
+             */

hw/i386/kvm/clock.c line 117

Quoted 10 lines

hw/i386/kvm/clock.c +10 -0

+            if (!kvm_has_adjust_clock_stable()) {
+                time_at_migration = kvmclock_current_nsec(s);
+            }
+        /* migration/savevm/init restore */
+        } else {
+            /*
+             * use s->clock in case machine uses reliable
+             * get clock and host where vm was executing
+             * supported reliable get clock
+             */

This comment is just weird. Simplifying

    /* If A and B do C */
    if (!A and || !B) {
       then D();
    }

Doing the opposite comment?

Migration code looks rigth.

Once said that, I continue hating clocks.