[PATCH v2] kvmclock: Ensure time in migration never goes backward

[PATCH v2] kvmclock: Ensure time in migration never goes backward

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

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - always use guest structure when available
---
 hw/i386/kvm/clock.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 892aa02..6f4ed28a 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -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);
-- 
1.7.12.4

[Qemu-devel] [PATCH v2] kvmclock: Ensure time in migration never goes backward

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

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - always use guest structure when available
---
 hw/i386/kvm/clock.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 892aa02..6f4ed28a 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -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);
-- 
1.7.12.4

Re: [PATCH v2] kvmclock: Ensure time in migration never goes backward

[Marcelo Tosatti]

On Fri, May 16, 2014 at 05:15:21PM +0200, Alexander Graf wrote:
> 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 <mtosatti@redhat.com>

Re: [Qemu-devel] [PATCH v2] kvmclock: Ensure time in migration never goes backward

[Marcelo Tosatti]

On Fri, May 16, 2014 at 05:15:21PM +0200, Alexander Graf wrote:
> 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 <mtosatti@redhat.com>

Re: [PATCH v2] kvmclock: Ensure time in migration never goes backward

[Paolo Bonzini]

Il 18/05/2014 15:20, Marcelo Tosatti ha scritto:
> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>

Thanks Marcelo, applying to uq/master.

Paolo

Re: [Qemu-devel] [PATCH v2] kvmclock: Ensure time in migration never goes backward

[Paolo Bonzini]

Il 18/05/2014 15:20, Marcelo Tosatti ha scritto:
> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>

Thanks Marcelo, applying to uq/master.

Paolo

Re: [PATCH v2] kvmclock: Ensure time in migration never goes backward

[Alexander Graf]

On 19.05.14 13:31, Paolo Bonzini wrote:
> Il 18/05/2014 15:20, Marcelo Tosatti ha scritto:
>> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> Thanks Marcelo, applying to uq/master.

Same here, please also CC to stable :).


Alex

Re: [Qemu-devel] [PATCH v2] kvmclock: Ensure time in migration never goes backward

[Alexander Graf]

On 19.05.14 13:31, Paolo Bonzini wrote:
> Il 18/05/2014 15:20, Marcelo Tosatti ha scritto:
>> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> Thanks Marcelo, applying to uq/master.

Same here, please also CC to stable :).


Alex

Re: [Qemu-devel] [PATCH v2] kvmclock: Ensure time in migration never goes backward

[Marcin Gibuła]

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

-- 
mg

Re: [Qemu-devel] [PATCH v2] kvmclock: Ensure time in migration never goes backward

[Marcelo Tosatti]

On Mon, Jun 02, 2014 at 10:31:44PM +0200, Marcin Gibuła wrote:
> >+    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...

Hi Marcin,

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


diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 6f4ed28a..bef2504 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -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));

Re: [Qemu-devel] [PATCH v2] kvmclock: Ensure time in migration never goes backward

[Marcin Gibuła]

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

Yes, this patch fixes that.

Thanks,

-- 
mg

Re: [Qemu-devel] [PATCH v2] kvmclock: Ensure time in migration never goes backward

[Paolo Bonzini]

Il 03/06/2014 07:16, Marcelo Tosatti ha scritto:
> On Mon, Jun 02, 2014 at 10:31:44PM +0200, Marcin Gibuła wrote:
>>> +    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...
>
> Hi Marcin,
>
> Can you give this patch a try?  Should read the guest TSC values after
> stopping the VM.
>
>
> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> index 6f4ed28a..bef2504 100644
> --- a/hw/i386/kvm/clock.c
> +++ b/hw/i386/kvm/clock.c
> @@ -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));
>

Can you send it with a real commit message?

Paolo

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

[Marcelo Tosatti]

Ensure proper env->tsc value for kvmclock_current_nsec calculation.

Reported-by: Marcin Gibuła <m.gibula@beyond.pl>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 6f4ed28a..bef2504 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -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));

Re: [PATCH v2] kvmclock: Ensure time in migration never goes backward

[Paolo Bonzini]

Il 16/05/2014 17:15, Alexander Graf ha scritto:
> 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 <agraf@suse.de>
>
> ---
>
> v1 -> v2:
>
>   - always use guest structure when available
> ---
>  hw/i386/kvm/clock.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> index 892aa02..6f4ed28a 100644
> --- a/hw/i386/kvm/clock.c
> +++ b/hw/i386/kvm/clock.c
> @@ -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);
>

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

Re: [Qemu-devel] [PATCH v2] kvmclock: Ensure time in migration never goes backward

[Paolo Bonzini]

Il 16/05/2014 17:15, Alexander Graf ha scritto:
> 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 <agraf@suse.de>
>
> ---
>
> v1 -> v2:
>
>   - always use guest structure when available
> ---
>  hw/i386/kvm/clock.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> index 892aa02..6f4ed28a 100644
> --- a/hw/i386/kvm/clock.c
> +++ b/hw/i386/kvm/clock.c
> @@ -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);
>

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

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

[Paolo Bonzini]

Il 03/06/2014 18:34, Marcelo Tosatti ha scritto:
>
> Ensure proper env->tsc value for kvmclock_current_nsec calculation.
>
> Reported-by: Marcin Gibuła <m.gibula@beyond.pl>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> index 6f4ed28a..bef2504 100644
> --- a/hw/i386/kvm/clock.c
> +++ b/hw/i386/kvm/clock.c
> @@ -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));
>
>

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

Paolo

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

[Marcin Gibuła]

>> @@ -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));
>>
>>
>
> 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).

-- 
mg

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

[Paolo Bonzini]

Il 15/07/2014 22:03, Marcin Gibuła ha scritto:
>> 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

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

[Marcelo Tosatti]

On Tue, Jul 15, 2014 at 10:15:14PM +0200, Paolo Bonzini wrote:
> Il 15/07/2014 22:03, Marcin Gibuła ha scritto:
> >>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.

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 ?

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

[Marcelo Tosatti]

On Tue, Jul 15, 2014 at 05:31:22PM -0300, Marcelo Tosatti wrote:
> On Tue, Jul 15, 2014 at 10:15:14PM +0200, Paolo Bonzini wrote:
> > Il 15/07/2014 22:03, Marcin Gibuła ha scritto:
> > >>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.
> 
> 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 ?
> 

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

http://www.mail-archive.com/qemu-devel@nongnu.org/msg246161.html

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

[Paolo Bonzini]

Il 15/07/2014 22:31, Marcelo Tosatti ha scritto:
> This patch (or some form of updating env->tsc, see changelog) 
> is necessary.

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

> 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

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

[Paolo Bonzini]

Il 15/07/2014 22:34, Marcelo Tosatti ha scritto:
> The backtrace in the following message is for a different problem,
> correct?
>
> http://www.mail-archive.com/qemu-devel@nongnu.org/msg246161.html
>
>
>

Correct.

Paolo

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

[Andrey Korolyov]

On Wed, Jul 16, 2014 at 12:43 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 15/07/2014 22:34, Marcelo Tosatti ha scritto:
>
>> The backtrace in the following message is for a different problem,
>> correct?
>>
>> http://www.mail-archive.com/qemu-devel@nongnu.org/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

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

[Paolo Bonzini]

Il 15/07/2014 23:05, Andrey Korolyov ha scritto:
> On Wed, Jul 16, 2014 at 12:43 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 15/07/2014 22:34, Marcelo Tosatti ha scritto:
>>
>>> The backtrace in the following message is for a different problem,
>>> correct?
>>>
>>> http://www.mail-archive.com/qemu-devel@nongnu.org/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).

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

Paolo