[PATCH 0/3] Fix migration of linux guests using kvmclock

[PATCH 0/3] Fix migration of linux guests using kvmclock

[Glauber Costa]

This series uses the new ioctl I've just posted to the kernel.
It is pretty straightforward usage.

We first have to save and restore kvm msr registers, and later, which
is done in patch 2. It has nothing to do with our ioctls, but is required
for the whole thing to work.

Patch 1 is a dependency, that quintela plans on sending to qemu.git too,
but is provided here for people that might want to test it.

[PATCH 1/3] introduce VMSTATE_U64

[Glauber Costa]

This is a patch actually written by Juan, which, according to him,
he plans on posting to qemu.git. Problem is that linux defines
u64 in a way that is type-uncompatible with uint64_t.

I am including it here, because it is a dependency to my patch series
that follows.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/hw.h  |   13 +++++++++++++
 savevm.c |   23 +++++++++++++++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index 8c223f8..348bf1d 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -330,6 +330,10 @@ extern const VMStateInfo vmstate_info_uint16;
 extern const VMStateInfo vmstate_info_uint32;
 extern const VMStateInfo vmstate_info_uint64;
 
+#ifdef __linux__
+extern const VMStateInfo vmstate_info_u64;
+#endif
+
 extern const VMStateInfo vmstate_info_timer;
 extern const VMStateInfo vmstate_info_ptimer;
 extern const VMStateInfo vmstate_info_buffer;
@@ -538,6 +542,15 @@ extern const VMStateDescription vmstate_i2c_slave;
 #define VMSTATE_UINT64(_f, _s)                                        \
     VMSTATE_UINT64_V(_f, _s, 0)
 
+/* This is needed because on linux __u64 is unsigned long long
+   and on glibc uint64_t is unsigned long on 64 bits */
+#ifdef __linux__
+#define VMSTATE_U64_V(_f, _s, _v)                                     \
+    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_u64, __u64)
+#define VMSTATE_U64(_f, _s)                                           \
+    VMSTATE_U64_V(_f, _s, 0)
+#endif
+
 #define VMSTATE_UINT8_EQUAL(_f, _s)                                   \
     VMSTATE_SINGLE(_f, _s, 0, vmstate_info_uint8_equal, uint8_t)
 
diff --git a/savevm.c b/savevm.c
index a6f3556..1a68aa5 100644
--- a/savevm.c
+++ b/savevm.c
@@ -848,6 +848,29 @@ const VMStateInfo vmstate_info_uint64 = {
     .put  = put_uint64,
 };
 
+/* 64 bit linux kernel unsigned int */
+
+#ifdef __linux__
+static int get_u64(QEMUFile *f, void *pv, size_t size)
+{
+    __u64 *v = pv;
+    qemu_get_be64s(f, (uint64_t *)v);
+    return 0;
+}
+
+static void put_u64(QEMUFile *f, void *pv, size_t size)
+{
+    __u64 *v = pv;
+    qemu_put_be64s(f, (uint64_t *)v);
+}
+
+const VMStateInfo vmstate_info_u64 = {
+    .name = "__u64",
+    .get  = get_u64,
+    .put  = put_u64,
+};
+#endif /* __linux__ */
+
 /* 8 bit int. See that the received value is the same than the one
    in the field */
 
-- 
1.6.2.2

[PATCH 2/3] properly save kvm system time msr registers

[Glauber Costa]

Currently, the msrs involved in setting up pvclock are not saved over
migration and/or save/restore. This patch puts their value in special
fields in our CPUState, and deal with them using vmstate.

kvm also has to account for it, by including them in the msr list
for the ioctls.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu-kvm-x86.c        |   15 +++++++++++++--
 target-i386/cpu.h     |    4 +++-
 target-i386/machine.c |    3 +++
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 475151d..693f5b2 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -834,6 +834,12 @@ static int get_msr_entry(struct kvm_msr_entry *entry, CPUState *env)
         case MSR_VM_HSAVE_PA:
             env->vm_hsave     = entry->data;
             break;
+        case MSR_KVM_SYSTEM_TIME:
+            env->system_time_msr = entry->data;
+            break;
+        case MSR_KVM_WALL_CLOCK:
+            env->wall_clock_msr = entry->data;
+            break;
         default:
             printf("Warning unknown msr index 0x%x\n", entry->index);
             return 1;
@@ -842,9 +848,9 @@ static int get_msr_entry(struct kvm_msr_entry *entry, CPUState *env)
 }
 
 #ifdef TARGET_X86_64
-#define MSR_COUNT 10
+#define MSR_COUNT 12
 #else
-#define MSR_COUNT 6
+#define MSR_COUNT 8
 #endif
 
 static void set_v8086_seg(struct kvm_segment *lhs, const SegmentCache *rhs)
@@ -1002,6 +1008,8 @@ void kvm_arch_load_regs(CPUState *env)
         set_msr_entry(&msrs[n++], MSR_LSTAR  ,           env->lstar);
     }
 #endif
+    set_msr_entry(&msrs[n++], MSR_KVM_SYSTEM_TIME,  env->system_time_msr);
+    set_msr_entry(&msrs[n++], MSR_KVM_WALL_CLOCK,  env->wall_clock_msr);
 
     rc = kvm_set_msrs(env->kvm_cpu_state.vcpu_ctx, msrs, n);
     if (rc == -1)
@@ -1181,6 +1189,9 @@ void kvm_arch_save_regs(CPUState *env)
         msrs[n++].index = MSR_LSTAR;
     }
 #endif
+    msrs[n++].index = MSR_KVM_SYSTEM_TIME;
+    msrs[n++].index = MSR_KVM_WALL_CLOCK;
+
     rc = kvm_get_msrs(env->kvm_cpu_state.vcpu_ctx, msrs, n);
     if (rc == -1) {
         perror("kvm_get_msrs FAILED");
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 278d3e3..4605fd2 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -667,6 +667,8 @@ typedef struct CPUX86State {
     target_ulong fmask;
     target_ulong kernelgsbase;
 #endif
+    uint64_t system_time_msr;
+    uint64_t wall_clock_msr;
 
     uint64_t tsc;
 
@@ -886,7 +888,7 @@ uint64_t cpu_get_tsc(CPUX86State *env);
 #define cpu_signal_handler cpu_x86_signal_handler
 #define cpu_list x86_cpu_list
 
-#define CPU_SAVE_VERSION 11
+#define CPU_SAVE_VERSION 12
 
 /* MMU modes definitions */
 #define MMU_MODE0_SUFFIX _kernel
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 16d9c57..b364936 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -479,6 +479,9 @@ const VMStateDescription vmstate_cpu = {
         VMSTATE_UINT64_ARRAY_V(mce_banks, CPUState, MCE_BANKS_DEF *4, 10),
         /* rdtscp */
         VMSTATE_UINT64_V(tsc_aux, CPUState, 11),
+        /* KVM pvclock msr */
+        VMSTATE_UINT64_V(system_time_msr, CPUState, 12),
+        VMSTATE_UINT64_V(wall_clock_msr, CPUState, 12),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.6.2.2

[PATCH 3/3] get and set clock upon migration

[Glauber Costa]

Register a vmstate handler for kvmclock. The goal here is to pass
information about current time to migration, so we avoid going backwards
or jumping too much. We use our plain new ioctls for that.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu-kvm-x86.c |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 693f5b2..909c147 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -767,6 +767,37 @@ int kvm_qemu_destroy_memory_alias(uint64_t phys_start)
 	return kvm_destroy_memory_alias(kvm_context, phys_start);
 }
 
+#ifdef KVM_CAP_ADJUST_CLOCK
+static struct kvm_clock_data kvmclock_data;
+
+static void kvmclock_pre_save(void *opaque)
+{
+    struct kvm_clock_data *cl = opaque;
+
+    kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, cl);
+}
+
+static int kvmclock_post_load(void *opaque, int version_id)
+{
+    struct kvm_clock_data *cl = opaque;
+
+    return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, cl);
+}
+
+static const VMStateDescription vmstate_kvmclock= {
+    .name = "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),
+        VMSTATE_END_OF_LIST()
+    }
+};
+#endif
+
 int kvm_arch_qemu_create_context(void)
 {
     int i;
@@ -788,6 +819,10 @@ int kvm_arch_qemu_create_context(void)
             kvm_has_vm_hsave_pa = 1;
     }
 
+#ifdef KVM_CAP_ADJUST_CLOCK
+    if (kvm_check_extension(kvm_state, KVM_CAP_ADJUST_CLOCK))
+        vmstate_register(0, &vmstate_kvmclock, &kvmclock_data);
+#endif
     return 0;
 }
 
-- 
1.6.2.2

Re: [PATCH 0/3] Fix migration of linux guests using kvmclock

[Marcelo Tosatti]

On Fri, Oct 16, 2009 at 03:27:36PM -0400, Glauber Costa wrote:
> This series uses the new ioctl I've just posted to the kernel.
> It is pretty straightforward usage.
> 
> We first have to save and restore kvm msr registers, and later, which
> is done in patch 2. It has nothing to do with our ioctls, but is required
> for the whole thing to work.
> 
> Patch 1 is a dependency, that quintela plans on sending to qemu.git too,
> but is provided here for people that might want to test it.

Applied, thanks.

Re: [PATCH 1/3] introduce VMSTATE_U64

[Avi Kivity]

On 10/17/2009 04:27 AM, Glauber Costa wrote:
> This is a patch actually written by Juan, which, according to him,
> he plans on posting to qemu.git. Problem is that linux defines
> u64 in a way that is type-uncompatible with uint64_t.
>
> I am including it here, because it is a dependency to my patch series
> that follows.
>
>    

Why can't we store these values in qemu as uint64_ts?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 1/3] introduce VMSTATE_U64

[Glauber Costa]

On Tue, Oct 20, 2009 at 08:40:26AM +0900, Avi Kivity wrote:
> On 10/17/2009 04:27 AM, Glauber Costa wrote:
>> This is a patch actually written by Juan, which, according to him,
>> he plans on posting to qemu.git. Problem is that linux defines
>> u64 in a way that is type-uncompatible with uint64_t.
>>
>> I am including it here, because it is a dependency to my patch series
>> that follows.
>>
>>    
>
> Why can't we store these values in qemu as uint64_ts?
Because then we have to redefine the whole structure in qemu.

the proposal is to simply pick the structures directly from linux. I believe
it is much easier, and the versioning scheme in vmstate will help us get 
around any changes they might suffer in the future.

Re: [PATCH 1/3] introduce VMSTATE_U64

[Andrew Theurer]

> On Tue, Oct 20, 2009 at 08:40:26AM +0900, Avi Kivity wrote:
> > On 10/17/2009 04:27 AM, Glauber Costa wrote:
> >> This is a patch actually written by Juan, which, according to him,
> >> he plans on posting to qemu.git. Problem is that linux defines
> >> u64 in a way that is type-uncompatible with uint64_t.
> >>
> >> I am including it here, because it is a dependency to my patch series
> >> that follows.
> >>
> >>    
> >
> > Why can't we store these values in qemu as uint64_ts?
> Because then we have to redefine the whole structure in qemu.
> 
> the proposal is to simply pick the structures directly from linux. I believe
> it is much easier, and the versioning scheme in vmstate will help us get 
> around any changes they might suffer in the future.

I get build errors with this.  Is there something extra I need to do?  I am currently running a rhel54 2.6.18 kernel.

  CC    qdev-properties.o
savevm.c: In function ‘get_u64’:
savevm.c:856: error: ‘__u64’ undeclared (first use in this function)
savevm.c:856: error: (Each undeclared identifier is reported only once
savevm.c:856: error: for each function it appears in.)
savevm.c:856: error: ‘v’ undeclared (first use in this function)
savevm.c: In function ‘put_u64’:
savevm.c:863: error: ‘__u64’ undeclared (first use in this function)
savevm.c:863: error: ‘v’ undeclared (first use in this function)
make[1]: *** [savevm.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [build-all] Error 2


Thanks,

-Andrew