[PATCH 0/2] Fix savevm odness related to kvmclock

[PATCH 0/2] Fix savevm odness related to kvmclock

[Glauber Costa]

Some users told me that savevm path is behaving oddly wrt kvmclock.
The first oddness is that a guarantee we never made (AFAIK) is being broken:
two consecutive "savevm" operations, with the machine stopped in between
produces different results, due to the call to KVM_GET_CLOCK ioctl.
I believe the assumption that if the vm does not run, its saveable
state won't change is fairly reasonable. Maybe we should formally
guarantee that?

The second patch deals with the fact that this happens even if
kvmclock is disabled in cpuid: its savevm section is registered
nevertheless. Here, I try to register it only if it's enabled at
machine start.

Thanks

Glauber Costa (2):
  make kvmclock value idempotent for stopped machine
  Do not register kvmclock savevm section if kvmclock is disabled.

 cpus.c            |    7 +++++++
 qemu-kvm-x86.c    |   25 +++++++++++++++----------
 qemu-kvm.h        |    4 ++++
 target-i386/kvm.c |    7 +++++++
 4 files changed, 33 insertions(+), 10 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH 0/2] Fix savevm odness related to kvmclock

[Glauber Costa]

Some users told me that savevm path is behaving oddly wrt kvmclock.
The first oddness is that a guarantee we never made (AFAIK) is being broken:
two consecutive "savevm" operations, with the machine stopped in between
produces different results, due to the call to KVM_GET_CLOCK ioctl.
I believe the assumption that if the vm does not run, its saveable
state won't change is fairly reasonable. Maybe we should formally
guarantee that?

The second patch deals with the fact that this happens even if
kvmclock is disabled in cpuid: its savevm section is registered
nevertheless. Here, I try to register it only if it's enabled at
machine start.

Thanks

Glauber Costa (2):
  make kvmclock value idempotent for stopped machine
  Do not register kvmclock savevm section if kvmclock is disabled.

 cpus.c            |    7 +++++++
 qemu-kvm-x86.c    |   25 +++++++++++++++----------
 qemu-kvm.h        |    4 ++++
 target-i386/kvm.c |    7 +++++++
 4 files changed, 33 insertions(+), 10 deletions(-)

-- 
1.7.2.3

[PATCH 1/2] make kvmclock value idempotent for stopped machine

[Glauber Costa]

Although we never made such commitment clear (well, to the best
of my knowledge), some people expect that two savevm issued in sequence
in a stopped machine will yield the same results. This is not a crazy
requirement, since we don't expect a stopped machine to be updating its state,
for any device.

With kvmclock, this is not the case, since the .pre_save hook will issue an
ioctl to the host to acquire a timestamp, which is always changing.

This patch moves the value acquisition to vm_stop. This should mean our
get clock ioctl is issued more times, but this should be fine since vm_stop
is not a hot path.

When we do migrate, we'll transfer that value along.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 cpus.c         |    4 ++++
 qemu-kvm-x86.c |    7 ++-----
 qemu-kvm.h     |    2 ++
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/cpus.c b/cpus.c
index a55c330..879a03a 100644
--- a/cpus.c
+++ b/cpus.c
@@ -112,6 +112,10 @@ static void do_vm_stop(int reason)
         pause_all_vcpus();
         vm_state_notify(0, reason);
         monitor_protocol_event(QEVENT_STOP, NULL);
+        if (kvm_enabled()) {
+            kvmclock_update_clock();
+        }
+
     }
 }
 
diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 20b7d6d..d099d3d 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -500,11 +500,9 @@ static int kvm_enable_tpr_access_reporting(CPUState *env)
 #ifdef KVM_CAP_ADJUST_CLOCK
 static struct kvm_clock_data kvmclock_data;
 
-static void kvmclock_pre_save(void *opaque)
+void kvmclock_update_clock(void)
 {
-    struct kvm_clock_data *cl = opaque;
-
-    kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, cl);
+    kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &kvmclock_data);
 }
 
 static int kvmclock_post_load(void *opaque, int version_id)
@@ -519,7 +517,6 @@ static const VMStateDescription vmstate_kvmclock= {
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
-    .pre_save = kvmclock_pre_save,
     .post_load = kvmclock_post_load,
     .fields      = (VMStateField []) {
         VMSTATE_U64(clock, struct kvm_clock_data),
diff --git a/qemu-kvm.h b/qemu-kvm.h
index 0f3fb50..b0b7ab3 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -752,6 +752,8 @@ int handle_tpr_access(void *opaque, CPUState *env, uint64_t rip,
 #define qemu_kvm_cpu_stop(env) do {} while(0)
 #endif
 
+void kvmclock_update_clock(void);
+
 #ifdef CONFIG_KVM
 
 typedef struct KVMSlot {
-- 
1.7.2.3

[Qemu-devel] [PATCH 1/2] make kvmclock value idempotent for stopped machine

[Glauber Costa]

Although we never made such commitment clear (well, to the best
of my knowledge), some people expect that two savevm issued in sequence
in a stopped machine will yield the same results. This is not a crazy
requirement, since we don't expect a stopped machine to be updating its state,
for any device.

With kvmclock, this is not the case, since the .pre_save hook will issue an
ioctl to the host to acquire a timestamp, which is always changing.

This patch moves the value acquisition to vm_stop. This should mean our
get clock ioctl is issued more times, but this should be fine since vm_stop
is not a hot path.

When we do migrate, we'll transfer that value along.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 cpus.c         |    4 ++++
 qemu-kvm-x86.c |    7 ++-----
 qemu-kvm.h     |    2 ++
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/cpus.c b/cpus.c
index a55c330..879a03a 100644
--- a/cpus.c
+++ b/cpus.c
@@ -112,6 +112,10 @@ static void do_vm_stop(int reason)
         pause_all_vcpus();
         vm_state_notify(0, reason);
         monitor_protocol_event(QEVENT_STOP, NULL);
+        if (kvm_enabled()) {
+            kvmclock_update_clock();
+        }
+
     }
 }
 
diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 20b7d6d..d099d3d 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -500,11 +500,9 @@ static int kvm_enable_tpr_access_reporting(CPUState *env)
 #ifdef KVM_CAP_ADJUST_CLOCK
 static struct kvm_clock_data kvmclock_data;
 
-static void kvmclock_pre_save(void *opaque)
+void kvmclock_update_clock(void)
 {
-    struct kvm_clock_data *cl = opaque;
-
-    kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, cl);
+    kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &kvmclock_data);
 }
 
 static int kvmclock_post_load(void *opaque, int version_id)
@@ -519,7 +517,6 @@ static const VMStateDescription vmstate_kvmclock= {
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
-    .pre_save = kvmclock_pre_save,
     .post_load = kvmclock_post_load,
     .fields      = (VMStateField []) {
         VMSTATE_U64(clock, struct kvm_clock_data),
diff --git a/qemu-kvm.h b/qemu-kvm.h
index 0f3fb50..b0b7ab3 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -752,6 +752,8 @@ int handle_tpr_access(void *opaque, CPUState *env, uint64_t rip,
 #define qemu_kvm_cpu_stop(env) do {} while(0)
 #endif
 
+void kvmclock_update_clock(void);
+
 #ifdef CONFIG_KVM
 
 typedef struct KVMSlot {
-- 
1.7.2.3

[PATCH 2/2] Do not register kvmclock savevm section if kvmclock is disabled.

[Glauber Costa]

Usually nobody usually thinks about that scenario (me included and specially),
but kvmclock can be actually disabled in the host.

It happens in two scenarios:
 1. host too old.
 2. we passed -kvmclock to our -cpu parameter.

In both cases, we should not register kvmclock savevm section. This patch
achives that by registering this section only if kvmclock is actually
currently enabled in cpuid.

The only caveat is that we have to register the savevm section a little bit
later, since we won't know the final kvmclock state before cpuid gets parsed.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 cpus.c            |    3 +++
 qemu-kvm-x86.c    |   20 ++++++++++++++------
 qemu-kvm.h        |    2 ++
 target-i386/kvm.c |    7 +++++++
 4 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/cpus.c b/cpus.c
index 879a03a..eef716c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -97,6 +97,9 @@ void cpu_synchronize_all_post_init(void)
     for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) {
         cpu_synchronize_post_init(cpu);
     }
+    if (kvm_enabled()) {
+        kvmclock_register_savevm();
+    }
 }
 
 int cpu_is_stopped(CPUState *env)
diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index d099d3d..668c8cf 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -502,6 +502,9 @@ static struct kvm_clock_data kvmclock_data;
 
 void kvmclock_update_clock(void)
 {
+    if (!kvmclock_enabled)
+        return;
+
     kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &kvmclock_data);
 }
 
@@ -525,6 +528,17 @@ static const VMStateDescription vmstate_kvmclock= {
 };
 #endif
 
+/* This has to happen after vcpu setup*/
+void kvmclock_register_savevm(void)
+{
+#ifdef KVM_CAP_ADJUST_CLOCK
+    if (kvmclock_enabled && kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK)) {
+        printf("registering kvmclock savevm section\n");
+        vmstate_register(NULL, 0, &vmstate_kvmclock, &kvmclock_data);
+    }
+#endif
+}
+
 int kvm_arch_qemu_create_context(void)
 {
     int r;
@@ -542,12 +556,6 @@ int kvm_arch_qemu_create_context(void)
         return -1;
     }
 
-#ifdef KVM_CAP_ADJUST_CLOCK
-    if (kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK)) {
-        vmstate_register(NULL, 0, &vmstate_kvmclock, &kvmclock_data);
-    }
-#endif
-
     r = kvm_set_boot_cpu_id(0);
     if (r < 0 && r != -ENOSYS) {
         return r;
diff --git a/qemu-kvm.h b/qemu-kvm.h
index b0b7ab3..f51a2d6 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -753,6 +753,8 @@ int handle_tpr_access(void *opaque, CPUState *env, uint64_t rip,
 #endif
 
 void kvmclock_update_clock(void);
+extern int kvmclock_enabled;
+void kvmclock_register_savevm(void);
 
 #ifdef CONFIG_KVM
 
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 95e5d02..5443765 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -293,6 +293,7 @@ void kvm_inject_x86_mce(CPUState *cenv, int bank, uint64_t status,
 }
 
 static int _kvm_arch_init_vcpu(CPUState *env);
+int kvmclock_enabled = 1;
 
 int kvm_arch_init_vcpu(CPUState *env)
 {
@@ -350,6 +351,12 @@ int kvm_arch_init_vcpu(CPUState *env)
     memset(c, 0, sizeof(*c));
     c->function = KVM_CPUID_FEATURES;
     c->eax = env->cpuid_kvm_features & get_para_features(env);
+
+    if (!(c->eax & (1 << KVM_FEATURE_CLOCKSOURCE))) {
+        /* In theory cpuid is per-cpu, and this is a global variable,
+         * but we don't expect kvmclock enabled in some cpus only */
+        kvmclock_enabled = 0;
+    }
 #endif
 
     cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused);
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/2] Do not register kvmclock savevm section if kvmclock is disabled.

[Glauber Costa]

Usually nobody usually thinks about that scenario (me included and specially),
but kvmclock can be actually disabled in the host.

It happens in two scenarios:
 1. host too old.
 2. we passed -kvmclock to our -cpu parameter.

In both cases, we should not register kvmclock savevm section. This patch
achives that by registering this section only if kvmclock is actually
currently enabled in cpuid.

The only caveat is that we have to register the savevm section a little bit
later, since we won't know the final kvmclock state before cpuid gets parsed.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 cpus.c            |    3 +++
 qemu-kvm-x86.c    |   20 ++++++++++++++------
 qemu-kvm.h        |    2 ++
 target-i386/kvm.c |    7 +++++++
 4 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/cpus.c b/cpus.c
index 879a03a..eef716c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -97,6 +97,9 @@ void cpu_synchronize_all_post_init(void)
     for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) {
         cpu_synchronize_post_init(cpu);
     }
+    if (kvm_enabled()) {
+        kvmclock_register_savevm();
+    }
 }
 
 int cpu_is_stopped(CPUState *env)
diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index d099d3d..668c8cf 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -502,6 +502,9 @@ static struct kvm_clock_data kvmclock_data;
 
 void kvmclock_update_clock(void)
 {
+    if (!kvmclock_enabled)
+        return;
+
     kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &kvmclock_data);
 }
 
@@ -525,6 +528,17 @@ static const VMStateDescription vmstate_kvmclock= {
 };
 #endif
 
+/* This has to happen after vcpu setup*/
+void kvmclock_register_savevm(void)
+{
+#ifdef KVM_CAP_ADJUST_CLOCK
+    if (kvmclock_enabled && kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK)) {
+        printf("registering kvmclock savevm section\n");
+        vmstate_register(NULL, 0, &vmstate_kvmclock, &kvmclock_data);
+    }
+#endif
+}
+
 int kvm_arch_qemu_create_context(void)
 {
     int r;
@@ -542,12 +556,6 @@ int kvm_arch_qemu_create_context(void)
         return -1;
     }
 
-#ifdef KVM_CAP_ADJUST_CLOCK
-    if (kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK)) {
-        vmstate_register(NULL, 0, &vmstate_kvmclock, &kvmclock_data);
-    }
-#endif
-
     r = kvm_set_boot_cpu_id(0);
     if (r < 0 && r != -ENOSYS) {
         return r;
diff --git a/qemu-kvm.h b/qemu-kvm.h
index b0b7ab3..f51a2d6 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -753,6 +753,8 @@ int handle_tpr_access(void *opaque, CPUState *env, uint64_t rip,
 #endif
 
 void kvmclock_update_clock(void);
+extern int kvmclock_enabled;
+void kvmclock_register_savevm(void);
 
 #ifdef CONFIG_KVM
 
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 95e5d02..5443765 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -293,6 +293,7 @@ void kvm_inject_x86_mce(CPUState *cenv, int bank, uint64_t status,
 }
 
 static int _kvm_arch_init_vcpu(CPUState *env);
+int kvmclock_enabled = 1;
 
 int kvm_arch_init_vcpu(CPUState *env)
 {
@@ -350,6 +351,12 @@ int kvm_arch_init_vcpu(CPUState *env)
     memset(c, 0, sizeof(*c));
     c->function = KVM_CPUID_FEATURES;
     c->eax = env->cpuid_kvm_features & get_para_features(env);
+
+    if (!(c->eax & (1 << KVM_FEATURE_CLOCKSOURCE))) {
+        /* In theory cpuid is per-cpu, and this is a global variable,
+         * but we don't expect kvmclock enabled in some cpus only */
+        kvmclock_enabled = 0;
+    }
 #endif
 
     cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused);
-- 
1.7.2.3

Re: [PATCH 2/2] Do not register kvmclock savevm section if kvmclock is disabled.

[Paolo Bonzini]

On 12/03/2010 11:49 AM, Glauber Costa wrote:
> +/* This has to happen after vcpu setup*/
> +void kvmclock_register_savevm(void)
> +{
> +#ifdef KVM_CAP_ADJUST_CLOCK
> +    if (kvmclock_enabled&&  kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK)) {
> +        printf("registering kvmclock savevm section\n");
> +        vmstate_register(NULL, 0,&vmstate_kvmclock,&kvmclock_data);
> +    }
> +#endif
> +}
> +

You should use a state change notifier for patch 1.  Even better, if you 
put this patch first adding the state change notifier will be very clean.

Paolo

[Qemu-devel] Re: [PATCH 2/2] Do not register kvmclock savevm section if kvmclock is disabled.

[Paolo Bonzini]

On 12/03/2010 11:49 AM, Glauber Costa wrote:
> +/* This has to happen after vcpu setup*/
> +void kvmclock_register_savevm(void)
> +{
> +#ifdef KVM_CAP_ADJUST_CLOCK
> +    if (kvmclock_enabled&&  kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK)) {
> +        printf("registering kvmclock savevm section\n");
> +        vmstate_register(NULL, 0,&vmstate_kvmclock,&kvmclock_data);
> +    }
> +#endif
> +}
> +

You should use a state change notifier for patch 1.  Even better, if you 
put this patch first adding the state change notifier will be very clean.

Paolo

Re: [PATCH 2/2] Do not register kvmclock savevm section if kvmclock is disabled.

[Paolo Bonzini]

On 12/03/2010 11:49 AM, Glauber Costa wrote:
> +/* This has to happen after vcpu setup*/
> +void kvmclock_register_savevm(void)
> +{
> +#ifdef KVM_CAP_ADJUST_CLOCK
> +    if (kvmclock_enabled&&  kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK)) {
> +        printf("registering kvmclock savevm section\n");
> +        vmstate_register(NULL, 0,&vmstate_kvmclock,&kvmclock_data);
> +    }
> +#endif
> +}
> +

You should use a state change notifier for patch 1.  Even better, if you 
put this patch first adding the state change notifier will be very clean.

Paolo

[Qemu-devel] Re: [PATCH 2/2] Do not register kvmclock savevm section if kvmclock is disabled.

[Paolo Bonzini]

On 12/03/2010 11:49 AM, Glauber Costa wrote:
> +/* This has to happen after vcpu setup*/
> +void kvmclock_register_savevm(void)
> +{
> +#ifdef KVM_CAP_ADJUST_CLOCK
> +    if (kvmclock_enabled&&  kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK)) {
> +        printf("registering kvmclock savevm section\n");
> +        vmstate_register(NULL, 0,&vmstate_kvmclock,&kvmclock_data);
> +    }
> +#endif
> +}
> +

You should use a state change notifier for patch 1.  Even better, if you 
put this patch first adding the state change notifier will be very clean.

Paolo