kvmclock: Ensure time in migration never goes backward

PATCH In review Alexander Graf 5 participants 19 messages May 16, 2014 – Jul 15, 2014 KVM

Share & tools

Alexander Graf

author

2014-05-16 15:15 UTC #0 lore raw

PATCH v2 1/1 kvmclock: Ensure time in migration never goes backward

When we migrate we ask the kernel about its current belief on what the guest time would be. However, I've seen cases where the kvmclock guest structure indicates a time more recent than the kvm returned time.

To make sure we never go backwards, calculate what the guest would have seen as time at the point of migration and use that value instead of the kernel returned one when it's more recent.

While the underlying bug is supposedly fixed on newer KVM versions, it doesn't hurt to base the view of the kvmclock after migration on the same foundation in host as well as guest.

Signed-off-by Alexander Graf <[email protected]>

---

v1 -> v2:

- always use guest structure when available

---
 hw/i386/kvm/clock.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

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

index 892aa02..6f4ed28a 100644
@@ -14,6 +14,7 @@
  */
 
 #include "qemu-common.h"
+#include "qemu/host-utils.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "hw/sysbus.h"

@@ -34,6 +35,47 @@ typedef struct KVMClockState {
     bool clock_valid;
 } KVMClockState;
 
+struct pvclock_vcpu_time_info {
+    uint32_t   version;
+    uint32_t   pad0;
+    uint64_t   tsc_timestamp;
+    uint64_t   system_time;
+    uint32_t   tsc_to_system_mul;
+    int8_t     tsc_shift;
+    uint8_t    flags;
+    uint8_t    pad[2];
+} __attribute__((__packed__)); /* 32 bytes */
+
+static uint64_t kvmclock_current_nsec(KVMClockState *s)
+{
+    CPUState *cpu = first_cpu;
+    CPUX86State *env = cpu->env_ptr;
+    hwaddr kvmclock_struct_pa = env->system_time_msr & ~1ULL;
+    uint64_t migration_tsc = env->tsc;
+    struct pvclock_vcpu_time_info time;
+    uint64_t delta;
+    uint64_t nsec_lo;
+    uint64_t nsec_hi;
+    uint64_t nsec;
+
+    if (!(env->system_time_msr & 1ULL)) {
+        /* KVM clock not active */
+        return 0;
+    }
+
+    cpu_physical_memory_read(kvmclock_struct_pa, &time, sizeof(time));
+
+    delta = migration_tsc - time.tsc_timestamp;
+    if (time.tsc_shift < 0) {
+        delta >>= -time.tsc_shift;
+    } else {
+        delta <<= time.tsc_shift;
+    }
+
+    mulu64(&nsec_lo, &nsec_hi, delta, time.tsc_to_system_mul);
+    nsec = (nsec_lo >> 32) | (nsec_hi << 32);
+    return nsec + time.system_time;
+}
 
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)

@@ -45,9 +87,15 @@ static void kvmclock_vm_state_change(void *opaque, int running,
 
     if (running) {
         struct kvm_clock_data data;
+        uint64_t time_at_migration = kvmclock_current_nsec(s);
 
         s->clock_valid = false;
 
+	/* We can't rely on the migrated clock value, just discard it */
+	if (time_at_migration) {
+	        s->clock = time_at_migration;
+	}
+
         data.clock = s->clock;
         data.flags = 0;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);

Signature · 1 line

1.7.12.4

Marcelo Tosatti

Reviewed-by on v2 1/1

2014-05-18 13:21 UTC #1 lore raw

Quoted 11 lines · Alexander Graf

When we migrate we ask the kernel about its current belief on what the guest time would be. However, I've seen cases where the kvmclock guest structure indicates a time more recent than the kvm returned time.
To make sure we never go backwards, calculate what the guest would have seen as time at the point of migration and use that value instead of the kernel returned one when it's more recent.
While the underlying bug is supposedly fixed on newer KVM versions, it doesn't hurt to base the view of the kvmclock after migration on the same foundation in host as well as guest.

Remove this last phrase from the changelog please, the underlying bug is not fixed on newer KVM versions.

Otherwise

Reviewed-by Marcelo Tosatti <[email protected]>

Paolo Bonzini

maintainer

2014-05-19 11:31 UTC #2 lore raw

Il 18/05/2014 15:20, Marcelo Tosatti ha scritto:

Quoted 1 line

Reviewed-by Marcelo Tosatti <[email protected]>

Thanks Marcelo, applying to uq/master.

Paolo

Alexander Graf

author

2014-05-21 10:03 UTC #3 lore raw

Quoted 4 lines · Paolo Bonzini

Il 18/05/2014 15:20, Marcelo Tosatti ha scritto:
Quoted 1 line
Reviewed-by Marcelo Tosatti <[email protected]>
Thanks Marcelo, applying to uq/master.

Same here, please also CC to stable :).

Alex

Marcin Gibuła

2014-06-02 20:32 UTC #4 lore raw

In reply to Alexander Graf

hw/i386/kvm/clock.c typedef struct KVMClockState { · line 68

Quoted 3 lines

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

+    cpu_physical_memory_read(kvmclock_struct_pa, &time, sizeof(time));
+
+    delta = migration_tsc - time.tsc_timestamp;

Hi,

when I was testing live storage migration with libvirt I found out that this patch can cause virtual machine to hang when completing mirror job.

This is (probably) because kvmclock_current_nsec() is called twice in a row and on second call time.tsc_timestamp is larger than migration_tsc. This causes delta to be huge and sets timer to invalid value.

The double call happens when switching from old to new disk (pivoting in libvirt's nomenclature).

Example values:

First call: migration_tsc: 12052203518652476, time_tsc: 12052203301565676, delta 108543400

Second call: migration_tsc: 12052203518652476, time_tsc: 12052204478600322, delta 9223372036374801885

Perhaps it is worth adding:

if (time.tsc_timestamp > migration_tsc) {
    return 0;
}

there? Untested though...

Signature · 1 line

mg

Marcelo Tosatti

2014-06-03 05:17 UTC #5 lore raw

Quoted 26 lines · Marcin Gibuła

Review
Quoted 3 lines
(quoted diff) +3 -0
+    cpu_physical_memory_read(kvmclock_struct_pa, &time, sizeof(time));
+
+    delta = migration_tsc - time.tsc_timestamp;
Hi,
when I was testing live storage migration with libvirt I found out that this patch can cause virtual machine to hang when completing mirror job.
This is (probably) because kvmclock_current_nsec() is called twice in a row and on second call time.tsc_timestamp is larger than migration_tsc. This causes delta to be huge and sets timer to invalid value.
The double call happens when switching from old to new disk (pivoting in libvirt's nomenclature).
Example values:
First call: migration_tsc: 12052203518652476, time_tsc:
12052203301565676, delta 108543400
Second call: migration_tsc: 12052203518652476, time_tsc:
12052204478600322, delta 9223372036374801885

Quoted 7 lines

Perhaps it is worth adding:
if (time.tsc_timestamp > migration_tsc) {
    return 0;
}
there? Untested though...

Hi Marcin,

Can you give this patch a try? Should read the guest TSC values after stopping the VM.

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

index 6f4ed28a..bef2504 100644
@@ -17,6 +17,7 @@
 #include "qemu/host-utils.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
+#include "sysemu/cpus.h"
 #include "hw/sysbus.h"
 #include "hw/kvm/clock.h"

@@ -65,6 +66,7 @@ static uint64_t kvmclock_current_nsec(KVMClockState *s)
 
     cpu_physical_memory_read(kvmclock_struct_pa, &time, sizeof(time));
 
+    assert(time.tsc_timestamp <= migration_tsc);
     delta = migration_tsc - time.tsc_timestamp;
     if (time.tsc_shift < 0) {
         delta >>= -time.tsc_shift;

@@ -123,6 +125,8 @@ static void kvmclock_vm_state_change(void *opaque, int running,
         if (s->clock_valid) {
             return;
         }
+
+        cpu_synchronize_all_states();
         ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
         if (ret < 0) {
             fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));

Marcin Gibuła

2014-06-03 09:12 UTC #6 lore raw

Quoted 2 lines

Can you give this patch a try? Should read the guest TSC values after stopping the VM.

Yes, this patch fixes that.

Thanks,

Signature · 1 line

mg

Paolo Bonzini

maintainer

2014-06-03 11:14 UTC #7 lore raw

In reply to Marcelo Tosatti

Il 03/06/2014 07:16, Marcelo Tosatti ha scritto:

Quoted 63 lines · Marcin Gibuła

Quoted 26 lines
Review
Quoted 3 lines
(quoted diff) +3 -0
+    cpu_physical_memory_read(kvmclock_struct_pa, &time, sizeof(time));
+
+    delta = migration_tsc - time.tsc_timestamp;
Hi,
when I was testing live storage migration with libvirt I found out that this patch can cause virtual machine to hang when completing mirror job.
This is (probably) because kvmclock_current_nsec() is called twice in a row and on second call time.tsc_timestamp is larger than migration_tsc. This causes delta to be huge and sets timer to invalid value.
The double call happens when switching from old to new disk (pivoting in libvirt's nomenclature).
Example values:
First call: migration_tsc: 12052203518652476, time_tsc:
12052203301565676, delta 108543400
Second call: migration_tsc: 12052203518652476, time_tsc:
12052204478600322, delta 9223372036374801885
Quoted 7 lines
Perhaps it is worth adding:
if (time.tsc_timestamp > migration_tsc) {
    return 0;
}
there? Untested though...
Hi Marcin,
Can you give this patch a try? Should read the guest TSC values after stopping the VM.
hw/i386/kvm/clock.c +2 -0
index 6f4ed28a..bef2504 100644
@@ -17,6 +17,7 @@
 #include "qemu/host-utils.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
+#include "sysemu/cpus.h"
 #include "hw/sysbus.h"
 #include "hw/kvm/clock.h"
 
@@ -65,6 +66,7 @@ static uint64_t kvmclock_current_nsec(KVMClockState *s)
 
     cpu_physical_memory_read(kvmclock_struct_pa, &time, sizeof(time));
 
+    assert(time.tsc_timestamp <= migration_tsc);
     delta = migration_tsc - time.tsc_timestamp;
     if (time.tsc_shift < 0) {
         delta >>= -time.tsc_shift;

hw/i386/kvm/clock.c static void kvmclock_vm_state_change(void *opaque, int running, · line 132

Quoted 8 lines

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

@@ -123,6 +125,8 @@ static void kvmclock_vm_state_change(void *opaque, int running,
         if (s->clock_valid) {
             return;
         }
+
+        cpu_synchronize_all_states();
         ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
         if (ret < 0) {
             fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));

Can you send it with a real commit message?

Paolo

Marcelo Tosatti

2014-06-03 16:35 UTC #8 lore raw

PATCH 1/1 kvmclock: Ensure proper env->tsc value for kvmclock_current_nsec calculation

Ensure proper env->tsc value for kvmclock_current_nsec calculation.

Reported-by Marcin Gibuła <[email protected]>

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

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

index 6f4ed28a..bef2504 100644
@@ -17,6 +17,7 @@
 #include "qemu/host-utils.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
+#include "sysemu/cpus.h"
 #include "hw/sysbus.h"
 #include "hw/kvm/clock.h"

@@ -65,6 +66,7 @@ static uint64_t kvmclock_current_nsec(KVMClockState *s)
 
     cpu_physical_memory_read(kvmclock_struct_pa, &time, sizeof(time));
 
+    assert(time.tsc_timestamp <= migration_tsc);
     delta = migration_tsc - time.tsc_timestamp;
     if (time.tsc_shift < 0) {
         delta >>= -time.tsc_shift;

@@ -123,6 +125,8 @@ static void kvmclock_vm_state_change(void *opaque, int running,
         if (s->clock_valid) {
             return;
         }
+
+        cpu_synchronize_all_states();
         ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
         if (ret < 0) {
             fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));

Paolo Bonzini

maintainer

2014-07-15 19:45 UTC #9 lore raw

Re: [Qemu-devel] [PATCH uq/master] kvmclock: Ensure proper env->tsc value for kvmclock_current_nsec calculation

Il 03/06/2014 18:34, Marcelo Tosatti ha scritto:

Quoted 29 lines

Ensure proper env->tsc value for kvmclock_current_nsec calculation.

Reported-by Marcin Gibuła <[email protected]>

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

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

index 6f4ed28a..bef2504 100644
@@ -17,6 +17,7 @@
 #include "qemu/host-utils.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
+#include "sysemu/cpus.h"
 #include "hw/sysbus.h"
 #include "hw/kvm/clock.h"

@@ -65,6 +66,7 @@ static uint64_t kvmclock_current_nsec(KVMClockState *s)
 
     cpu_physical_memory_read(kvmclock_struct_pa, &time, sizeof(time));
 
+    assert(time.tsc_timestamp <= migration_tsc);
     delta = migration_tsc - time.tsc_timestamp;
     if (time.tsc_shift < 0) {
         delta >>= -time.tsc_shift;

hw/i386/kvm/clock.c static void kvmclock_vm_state_change(void *opaque, int running, · line 132

Quoted 8 lines

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

@@ -123,6 +125,8 @@ static void kvmclock_vm_state_change(void *opaque, int running,
         if (s->clock_valid) {
             return;
         }
+
+        cpu_synchronize_all_states();
         ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
         if (ret < 0) {
             fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));

This causes a hang during migration, so I'll revert the patch from 2.1.
Sorry.

Paolo

Marcin Gibuła

2014-07-15 20:03 UTC #10 lore raw

Re: [Qemu-devel] [PATCH uq/master] kvmclock: Ensure proper env->tsc value for kvmclock_current_nsec calculation

Quoted 15 lines

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

@@ -65,6 +66,7 @@ static uint64_t kvmclock_current_nsec(KVMClockState *s)
 
     cpu_physical_memory_read(kvmclock_struct_pa, &time, sizeof(time));
 
+    assert(time.tsc_timestamp <= migration_tsc);
     delta = migration_tsc - time.tsc_timestamp;
     if (time.tsc_shift < 0) {
         delta >>= -time.tsc_shift;

@@ -123,6 +125,8 @@ static void kvmclock_vm_state_change(void *opaque,
 int running,
         if (s->clock_valid) {
             return;
         }
+
+        cpu_synchronize_all_states();

Quoted 8 lines

Quoted 6 lines
hw/i386/kvm/clock.c +0 -0
         ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
         if (ret < 0) {
fprintf(stderr, "KVM_GET_CLOCK failed: %s\n",
strerror(ret));
This causes a hang during migration, so I'll revert the patch from 2.1.

For me this patch series fixed all hangs I had with migration (at least with qemu 2.0).

Signature · 1 line

mg

Paolo Bonzini

maintainer

2014-07-15 20:15 UTC #11 lore raw

Re: [Qemu-devel] [PATCH uq/master] kvmclock: Ensure proper env->tsc value for kvmclock_current_nsec calculation

Il 15/07/2014 22:03, Marcin Gibuła ha scritto:

Quoted 4 lines

Quoted 1 line
This causes a hang during migration, so I'll revert the patch from 2.1.
For me this patch series fixed all hangs I had with migration (at least with qemu 2.0).

Unfortunately, someone else bisected their hangs exactly to this patch:

http://permalink.gmane.org/gmane.comp.emulators.qemu/286681

I could not reproduce that, but I couldn't reproduce yours either so I'm inclined to trust him and revert the patch.

Paolo

Marcelo Tosatti

2014-07-15 20:31 UTC #12 lore raw

Re: [Qemu-devel] [PATCH uq/master] kvmclock: Ensure proper env->tsc value for kvmclock_current_nsec calculation

Quoted 6 lines · Paolo Bonzini

Il 15/07/2014 22:03, Marcin Gibuła ha scritto:
Quoted 4 lines
Quoted 1 line
This causes a hang during migration, so I'll revert the patch from 2.1.
For me this patch series fixed all hangs I had with migration (at least with qemu 2.0).

Quoted 6 lines

Unfortunately, someone else bisected their hangs exactly to this patch:
http://permalink.gmane.org/gmane.comp.emulators.qemu/286681
I could not reproduce that, but I couldn't reproduce yours either so I'm inclined to trust him and revert the patch.

This patch (or some form of updating env->tsc, see changelog) is necessary.

I was just about to reproduce it.

Andrey, can you please provide the qemu command line ?

Can you attempt to start VM via command line and migrate via QEMU monitor, then gdb -p to get a backtrace for all threads ?

Marcelo Tosatti

2014-07-15 20:35 UTC #13 lore raw

Re: [Qemu-devel] [PATCH uq/master] kvmclock: Ensure proper env->tsc value for kvmclock_current_nsec calculation

Quoted 17 lines · Marcelo Tosatti

Quoted 6 lines · Paolo Bonzini
Il 15/07/2014 22:03, Marcin Gibuła ha scritto:
Quoted 4 lines
Quoted 1 line
This causes a hang during migration, so I'll revert the patch from 2.1.
For me this patch series fixed all hangs I had with migration (at least with qemu 2.0).
Quoted 6 lines
Unfortunately, someone else bisected their hangs exactly to this patch:
http://permalink.gmane.org/gmane.comp.emulators.qemu/286681
I could not reproduce that, but I couldn't reproduce yours either so I'm inclined to trust him and revert the patch.
This patch (or some form of updating env->tsc, see changelog) is necessary.

Quoted 8 lines

I was just about to reproduce it.
Andrey, can you please provide the qemu command line ?
Can you attempt to start VM via command line and migrate via QEMU monitor, then gdb -p to get a backtrace for all threads ?

The backtrace in the following message is for a different problem, correct?

http://www.mail-archive.com/[email protected]/msg246161.html

Paolo Bonzini

maintainer

2014-07-15 20:44 UTC #14 lore raw

Re: [Qemu-devel] [PATCH uq/master] kvmclock: Ensure proper env->tsc value for kvmclock_current_nsec calculation

Il 15/07/2014 22:34, Marcelo Tosatti ha scritto:

Quoted 7 lines

The backtrace in the following message is for a different problem, correct?
http://www.mail-archive.com/[email protected]/msg246161.html

Correct.

Paolo

Andrey Korolyov

2014-07-15 21:06 UTC #15 lore raw

Re: [Qemu-devel] [PATCH uq/master] kvmclock: Ensure proper env->tsc value for kvmclock_current_nsec calculation

Quoted 5 lines · Paolo Bonzini

Il 15/07/2014 22:34, Marcelo Tosatti ha scritto:
Quoted 3 lines
The backtrace in the following message is for a different problem, correct?

Quoted 8 lines

Quoted 4 lines
http://www.mail-archive.com/[email protected]/msg246161.html
Correct.
Paolo

Sorry for being unclear. VM is running well, the resulting lockup visibly affects only disk, I wrote this as a lockup, but it is not complete lockup, I am able to execute cached executables (and see, for example, soft lockup warnings for the disk). Unfortunately I had not prepared -dbg package, so if you need exact trace, let me know, I thought that it is necessary just for quick verification. I`ll be very glad to provide any other information if necessary. As I mentioned in original thread, the issue is not specific to the iothread code, though I had not tested with other bus than virtio. If any of you who are interested in solving such a decision (thing which broke stuff for me fixes stuff in the other place) in place, I may offer you a sandboxed environment with rbd-ready deployment. The argument set follows:

qemu-system-x86_64 -enable-kvm -name vm27842 -S -machine
pc-i440fx-2.1,accel=kvm,usb=off -m 256 -realtime mlock=off -smp
12,sockets=1,cores=12,threads=12 -numa node,nodeid=0,cpus=0,mem=256
-uuid 9a44bdae-5702-463b-aa1e-d8d85055f6af -nographic -no-user-config
-nodefaults -chardev
socket,id=charmonitor,path=/var/lib/libvirt/qemu/vm27842.monitor,server,nowait
-mon chardev=charmonitor,id=monitor,mode=control -rtc
base=utc,clock=vm,driftfix=slew -global
kvm-pit.lost_tick_policy=discard -no-shutdown -boot strict=on -device
piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -device
virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 -drive file=rbd:dev-rack2/vm27842-Kxs:id=qemukvm:key=XXXXXXXX:auth_supported=cephx\;none:mon_host=10.6.0.1\:6789\;10.6.0.4\:6789,if=none,id=drive-virtio-disk0,format=raw,cache=writeback -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x6,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
-netdev tap,fd=23,id=hostnet0,vhost=on,vhostfd=24 -device
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:10:03:40,bus=pci.0,addr=0x3
-netdev tap,fd=25,id=hostnet1,vhost=on,vhostfd=26 -device
virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:10:03:41,bus=pci.0,addr=0x4
-chardev pty,id=charserial0 -device
isa-serial,chardev=charserial0,id=serial0 -chardev
socket,id=charchannel0,path=/var/lib/libvirt/qemu/vm27842.sock,server,nowait
-device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.1

Paolo Bonzini

maintainer

2014-07-15 21:08 UTC #16 lore raw

Re: [Qemu-devel] [PATCH uq/master] kvmclock: Ensure proper env->tsc value for kvmclock_current_nsec calculation

Il 15/07/2014 23:05, Andrey Korolyov ha scritto:

Quoted 8 lines · Paolo Bonzini

Quoted 7 lines
Il 15/07/2014 22:34, Marcelo Tosatti ha scritto:
Quoted 4 lines
The backtrace in the following message is for a different problem, correct?
http://www.mail-archive.com/[email protected]/msg246161.html

Quoted 8 lines

Quoted 3 lines
Correct.
Paolo
Sorry for being unclear. VM is running well, the resulting lockup visibly affects only disk, I wrote this as a lockup, but it is not complete lockup, I am able to execute cached executables (and see, for example, soft lockup warnings for the disk).

Yeah, that was my understanding. Can you reproduce it without rbd?

Paolo

Paolo Bonzini

maintainer

2014-07-15 20:43 UTC #17 lore raw

Re: [Qemu-devel] [PATCH uq/master] kvmclock: Ensure proper env->tsc value for kvmclock_current_nsec calculation

In reply to Marcelo Tosatti

Il 15/07/2014 22:31, Marcelo Tosatti ha scritto:

Quoted 2 lines

This patch (or some form of updating env->tsc, see changelog) is necessary.

Yes, I was going to revert Alex's too.

Quoted 7 lines

I was just about to reproduce it.
Andrey, can you please provide the qemu command line ?
Can you attempt to start VM via command line and migrate via QEMU monitor, then gdb -p to get a backtrace for all threads ?

Andrey provided backtraces for this issue here:

http://article.gmane.org/gmane.comp.emulators.qemu/286679

The first backtrace attached is bt-noreset.txt, the second is bt-reset.txt but they are identical. The backtraces lack symbols for QEMU, but it looks like the VMs are running because the CPU threads all look like this:

Thread 4 (Thread 0x7f0054ff9700 (LWP 2915)):

#0  0x00007f00a04dbc37 in ioctl () at ../sysdeps/unix/syscall-template.S:82
#1  0x00007f00a59ea0a9 in ?? ()
#2  0x00007f00a59ea1e5 in ?? ()
#3  0x00007f00a59d59bc in ?? ()
#4  0x00007f00a07b5e9a in start_thread (arg=0x7f0054ff9700) at pthread_create.c:308
#5  0x00007f00a04e33dd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:112
#6  0x0000000000000000 in ?? ()

where the ioctl is likely KVM_RUN.

Paolo

Paolo Bonzini

maintainer

2014-07-15 19:44 UTC #18 lore raw

In reply to Alexander Graf

Il 16/05/2014 17:15, Alexander Graf ha scritto:

Quoted 90 lines

To make sure we never go backwards, calculate what the guest would have seen as time at the point of migration and use that value instead of the kernel returned one when it's more recent.

While the underlying bug is supposedly fixed on newer KVM versions, it doesn't hurt to base the view of the kvmclock after migration on the same foundation in host as well as guest.

Signed-off-by Alexander Graf <[email protected]>

---

v1 -> v2:

- always use guest structure when available

---
 hw/i386/kvm/clock.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

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

index 892aa02..6f4ed28a 100644
@@ -14,6 +14,7 @@
  */
 
 #include "qemu-common.h"
+#include "qemu/host-utils.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "hw/sysbus.h"

@@ -34,6 +35,47 @@ typedef struct KVMClockState {
     bool clock_valid;
 } KVMClockState;
 
+struct pvclock_vcpu_time_info {
+    uint32_t   version;
+    uint32_t   pad0;
+    uint64_t   tsc_timestamp;
+    uint64_t   system_time;
+    uint32_t   tsc_to_system_mul;
+    int8_t     tsc_shift;
+    uint8_t    flags;
+    uint8_t    pad[2];
+} __attribute__((__packed__)); /* 32 bytes */
+
+static uint64_t kvmclock_current_nsec(KVMClockState *s)
+{
+    CPUState *cpu = first_cpu;
+    CPUX86State *env = cpu->env_ptr;
+    hwaddr kvmclock_struct_pa = env->system_time_msr & ~1ULL;
+    uint64_t migration_tsc = env->tsc;
+    struct pvclock_vcpu_time_info time;
+    uint64_t delta;
+    uint64_t nsec_lo;
+    uint64_t nsec_hi;
+    uint64_t nsec;
+
+    if (!(env->system_time_msr & 1ULL)) {
+        /* KVM clock not active */
+        return 0;
+    }
+
+    cpu_physical_memory_read(kvmclock_struct_pa, &time, sizeof(time));
+
+    delta = migration_tsc - time.tsc_timestamp;
+    if (time.tsc_shift < 0) {
+        delta >>= -time.tsc_shift;
+    } else {
+        delta <<= time.tsc_shift;
+    }
+
+    mulu64(&nsec_lo, &nsec_hi, delta, time.tsc_to_system_mul);
+    nsec = (nsec_lo >> 32) | (nsec_hi << 32);
+    return nsec + time.system_time;
+}
 
 static void kvmclock_vm_state_change(void *opaque, int running,
                                      RunState state)

@@ -45,9 +87,15 @@ static void kvmclock_vm_state_change(void *opaque, int running,
 
     if (running) {
         struct kvm_clock_data data;
+        uint64_t time_at_migration = kvmclock_current_nsec(s);

hw/i386/kvm/clock.c static void kvmclock_vm_state_change(void *opaque, int running, · line 101

Quoted 10 lines

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

         s->clock_valid = false;
 
+	/* We can't rely on the migrated clock value, just discard it */
+	if (time_at_migration) {
+	        s->clock = time_at_migration;
+	}
+
         data.clock = s->clock;
         data.flags = 0;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);

I'm going to revert this patch for 2.1-rc3, since the dependent patch "kvmclock: Ensure proper env->tsc value for kvmclock_current_nsec calculation" causes a hang during migration.

Paolo