[RFC] Do clock adjustments over migration

[RFC] Do clock adjustments over migration

[Glauber Costa]

Hey,

This patch is a proposal only. Among other things, it relies on a patch
Juan is yet to send, and I also would want to give it a bit more testing.
It shows my indented use of the new ioctl interface I've been proposing.

First of all, we have to save the kvmclock msrs. This is per-cpu, and we
were failing to do it so far.

The ioctls are issued in pre-save and post-load sections of a new vmstate
handler. I am not doing it in the cpu vmstate handler, because this has to
be done once per VM, not cpu. What I basically do is to grab the time
from GET ioctl, pass on through migration, and then do a SET on the other
side. Should be straighforward.

Please let me hear your thoughts. And don't get me started with this
"you can't hear thoughts" thing!

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 kvm/include/linux/kvm.h |    9 +++++++++
 qemu-kvm-x86.c          |    6 ++++++
 qemu-kvm.c              |   29 +++++++++++++++++++++++++++++
 target-i386/cpu.h       |    3 ++-
 target-i386/machine.c   |    2 ++
 5 files changed, 48 insertions(+), 1 deletions(-)

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index fffcfd8..75e2ffd 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -834,6 +834,9 @@ 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;
         default:
             printf("Warning unknown msr index 0x%x\n", entry->index);
             return 1;
@@ -1001,6 +1004,7 @@ 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);
 
     rc = kvm_set_msrs(env->kvm_cpu_state.vcpu_ctx, msrs, n);
     if (rc == -1)
@@ -1179,6 +1183,8 @@ void kvm_arch_save_regs(CPUState *env)
         msrs[n++].index = MSR_LSTAR;
     }
 #endif
+    msrs[n++].index = MSR_KVM_SYSTEM_TIME;
+
     rc = kvm_get_msrs(env->kvm_cpu_state.vcpu_ctx, msrs, n);
     if (rc == -1) {
         perror("kvm_get_msrs FAILED");
diff --git a/qemu-kvm.c b/qemu-kvm.c
index 87ece3d..75e9575 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -347,6 +347,34 @@ int kvm_dirty_pages_log_reset(kvm_context_t kvm)
 
 
 static int kvm_create_context(void);
+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 = "timer",
+    .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()
+    }
+};
 
 int kvm_init(int smp_cpus)
 {
@@ -406,6 +434,7 @@ int kvm_init(int smp_cpus)
             set_gsi(kvm_context, i);
     }
 
+    vmstate_register(0, &vmstate_kvmclock, &kvmclock_data);
     pthread_mutex_lock(&qemu_mutex);
     return kvm_create_context();
 
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 278d3e3..8022772 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -667,6 +667,7 @@ typedef struct CPUX86State {
     target_ulong fmask;
     target_ulong kernelgsbase;
 #endif
+    uint64_t system_time_msr;
 
     uint64_t tsc;
 
@@ -886,7 +887,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..d5267be 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -479,6 +479,8 @@ 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_END_OF_LIST()
     }
 };
-- 
1.6.2.2

Re: [RFC] Do clock adjustments over migration

[Marcelo Tosatti]

On Thu, Oct 15, 2009 at 01:15:25PM -0400, Glauber Costa wrote:
> Hey,
> 
> This patch is a proposal only. Among other things, it relies on a patch
> Juan is yet to send, and I also would want to give it a bit more testing.
> It shows my indented use of the new ioctl interface I've been proposing.
> 
> First of all, we have to save the kvmclock msrs. This is per-cpu, and we
> were failing to do it so far.
> 
> The ioctls are issued in pre-save and post-load sections of a new vmstate
> handler. I am not doing it in the cpu vmstate handler, because this has to
> be done once per VM, not cpu. What I basically do is to grab the time
> from GET ioctl, pass on through migration, and then do a SET on the other
> side. Should be straighforward.
> 
> Please let me hear your thoughts. And don't get me started with this
> "you can't hear thoughts" thing!
> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  kvm/include/linux/kvm.h |    9 +++++++++
>  qemu-kvm-x86.c          |    6 ++++++
>  qemu-kvm.c              |   29 +++++++++++++++++++++++++++++
>  target-i386/cpu.h       |    3 ++-
>  target-i386/machine.c   |    2 ++
>  5 files changed, 48 insertions(+), 1 deletions(-)
> 
> diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
> index fffcfd8..75e2ffd 100644
> --- a/qemu-kvm-x86.c
> +++ b/qemu-kvm-x86.c
> @@ -834,6 +834,9 @@ 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;
>          default:
>              printf("Warning unknown msr index 0x%x\n", entry->index);
>              return 1;
> @@ -1001,6 +1004,7 @@ 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);
>  
>      rc = kvm_set_msrs(env->kvm_cpu_state.vcpu_ctx, msrs, n);
>      if (rc == -1)
> @@ -1179,6 +1183,8 @@ void kvm_arch_save_regs(CPUState *env)
>          msrs[n++].index = MSR_LSTAR;
>      }
>  #endif
> +    msrs[n++].index = MSR_KVM_SYSTEM_TIME;
> +

fix MSR_COUNT for kvm_arch_save_regs()

Otherwise looks good.

Re: [RFC] Do clock adjustments over migration

[Glauber Costa]

On Thu, Oct 15, 2009 at 05:08:16PM -0300, Marcelo Tosatti wrote:
> On Thu, Oct 15, 2009 at 01:15:25PM -0400, Glauber Costa wrote:
> > Hey,
> > 
> > This patch is a proposal only. Among other things, it relies on a patch
> > Juan is yet to send, and I also would want to give it a bit more testing.
> > It shows my indented use of the new ioctl interface I've been proposing.
> > 
> > First of all, we have to save the kvmclock msrs. This is per-cpu, and we
> > were failing to do it so far.
> > 
> > The ioctls are issued in pre-save and post-load sections of a new vmstate
> > handler. I am not doing it in the cpu vmstate handler, because this has to
> > be done once per VM, not cpu. What I basically do is to grab the time
> > from GET ioctl, pass on through migration, and then do a SET on the other
> > side. Should be straighforward.
> > 
> > Please let me hear your thoughts. And don't get me started with this
> > "you can't hear thoughts" thing!
> > 
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > ---
> >  kvm/include/linux/kvm.h |    9 +++++++++
> >  qemu-kvm-x86.c          |    6 ++++++
> >  qemu-kvm.c              |   29 +++++++++++++++++++++++++++++
> >  target-i386/cpu.h       |    3 ++-
> >  target-i386/machine.c   |    2 ++
> >  5 files changed, 48 insertions(+), 1 deletions(-)
> > 
> > diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
> > index fffcfd8..75e2ffd 100644
> > --- a/qemu-kvm-x86.c
> > +++ b/qemu-kvm-x86.c
> > @@ -834,6 +834,9 @@ 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;
> >          default:
> >              printf("Warning unknown msr index 0x%x\n", entry->index);
> >              return 1;
> > @@ -1001,6 +1004,7 @@ 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);
> >  
> >      rc = kvm_set_msrs(env->kvm_cpu_state.vcpu_ctx, msrs, n);
> >      if (rc == -1)
> > @@ -1179,6 +1183,8 @@ void kvm_arch_save_regs(CPUState *env)
> >          msrs[n++].index = MSR_LSTAR;
> >      }
> >  #endif
> > +    msrs[n++].index = MSR_KVM_SYSTEM_TIME;
> > +
> 
> fix MSR_COUNT for kvm_arch_save_regs()
yeah, thanks

I'll also wait until tomorrow to see if people have more comments on it.

> 
> Otherwise looks good.
Great!

Re: [RFC] Do clock adjustments over migration

[Glauber Costa]

On Thu, Oct 15, 2009 at 05:08:16PM -0300, Marcelo Tosatti wrote:
> On Thu, Oct 15, 2009 at 01:15:25PM -0400, Glauber Costa wrote:
> > Hey,
> > 
> > This patch is a proposal only. Among other things, it relies on a patch
> > Juan is yet to send, and I also would want to give it a bit more testing.
> > It shows my indented use of the new ioctl interface I've been proposing.
> > 
> > First of all, we have to save the kvmclock msrs. This is per-cpu, and we
> > were failing to do it so far.
> > 
> > The ioctls are issued in pre-save and post-load sections of a new vmstate
> > handler. I am not doing it in the cpu vmstate handler, because this has to
> > be done once per VM, not cpu. What I basically do is to grab the time
> > from GET ioctl, pass on through migration, and then do a SET on the other
> > side. Should be straighforward.
> > 
> > Please let me hear your thoughts. And don't get me started with this
> > "you can't hear thoughts" thing!
> > 
> 
> Otherwise looks good.
> 
Also, note that, in vmstate table for kvmclock, I am using U64, instead of 
UINT64. This is unexistant on current qemu, I had to patch it.

Juan said he's planning on sending it to qemu-devel today or tomorrow.