Patchwork [v3,15/21] kvm: x86: Introduce kvmclock device to save/restore its state

login
register
mail settings
Submitter Jan Kiszka
Date Jan. 4, 2011, 8:32 a.m.
Message ID <defb249386b08f440163c10d5d815586ef1023ec.1294129949.git.jan.kiszka@web.de>
Download mbox | patch
Permalink /patch/77425/
State New
Headers show

Comments

Jan Kiszka - Jan. 4, 2011, 8:32 a.m.
From: Jan Kiszka <jan.kiszka@siemens.com>

If kvmclock is used, which implies the kernel supports it, register a
kvmclock device with the sysbus. Its main purpose is to save and restore
the kernel state on migration, but this will also allow to visualize it
one day.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
CC: Glauber Costa <glommer@redhat.com>
---
 target-i386/kvm.c |   92 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 91 insertions(+), 1 deletions(-)
Glauber Costa - Jan. 4, 2011, 11:08 a.m.
On Tue, 2011-01-04 at 09:32 +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> If kvmclock is used, which implies the kernel supports it, register a
> kvmclock device with the sysbus. Its main purpose is to save and restore
> the kernel state on migration, but this will also allow to visualize it
> one day.

I would prefer having no pre-save, and doing the ioctl in the state
change handler. But I won't nitpick about that, if the maintainers think
this is okay, all the rest of the patch looks fine as well.
Jan Kiszka - Jan. 4, 2011, 11:34 a.m.
Am 04.01.2011 12:08, Glauber Costa wrote:
> On Tue, 2011-01-04 at 09:32 +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> If kvmclock is used, which implies the kernel supports it, register a
>> kvmclock device with the sysbus. Its main purpose is to save and restore
>> the kernel state on migration, but this will also allow to visualize it
>> one day.
> 
> I would prefer having no pre-save, and doing the ioctl in the state
> change handler. But I won't nitpick about that, if the maintainers think
> this is okay, all the rest of the patch looks fine as well.

I did this for a reason: to be able to obtain the current clock state
even while the vm is running. It's cleaner IMHO.

Jan
Glauber Costa - Jan. 4, 2011, 11:43 a.m.
On Tue, 2011-01-04 at 12:34 +0100, Jan Kiszka wrote:
> Am 04.01.2011 12:08, Glauber Costa wrote:
> > On Tue, 2011-01-04 at 09:32 +0100, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> If kvmclock is used, which implies the kernel supports it, register a
> >> kvmclock device with the sysbus. Its main purpose is to save and restore
> >> the kernel state on migration, but this will also allow to visualize it
> >> one day.
> > 
> > I would prefer having no pre-save, and doing the ioctl in the state
> > change handler. But I won't nitpick about that, if the maintainers think
> > this is okay, all the rest of the patch looks fine as well.
> 
> I did this for a reason: to be able to obtain the current clock state
> even while the vm is running. It's cleaner IMHO.

but if we're on pre-save, we are certain that the vm is stopped at this
moment, no ?
Jan Kiszka - Jan. 4, 2011, 11:45 a.m.
Am 04.01.2011 12:43, Glauber Costa wrote:
> On Tue, 2011-01-04 at 12:34 +0100, Jan Kiszka wrote:
>> Am 04.01.2011 12:08, Glauber Costa wrote:
>>> On Tue, 2011-01-04 at 09:32 +0100, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> If kvmclock is used, which implies the kernel supports it, register a
>>>> kvmclock device with the sysbus. Its main purpose is to save and restore
>>>> the kernel state on migration, but this will also allow to visualize it
>>>> one day.
>>>
>>> I would prefer having no pre-save, and doing the ioctl in the state
>>> change handler. But I won't nitpick about that, if the maintainers think
>>> this is okay, all the rest of the patch looks fine as well.
>>
>> I did this for a reason: to be able to obtain the current clock state
>> even while the vm is running. It's cleaner IMHO.
> 
> but if we're on pre-save, we are certain that the vm is stopped at this
> moment, no ?

Maybe at the moment, but not for device state dumping or maybe (dunno)
for Kemari's continuous sync'ing. I simply want to avoid this implicit
dependency.

Jan
Glauber Costa - Jan. 4, 2011, 11:48 a.m.
On Tue, 2011-01-04 at 12:45 +0100, Jan Kiszka wrote:
> Am 04.01.2011 12:43, Glauber Costa wrote:
> > On Tue, 2011-01-04 at 12:34 +0100, Jan Kiszka wrote:
> >> Am 04.01.2011 12:08, Glauber Costa wrote:
> >>> On Tue, 2011-01-04 at 09:32 +0100, Jan Kiszka wrote:
> >>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>
> >>>> If kvmclock is used, which implies the kernel supports it, register a
> >>>> kvmclock device with the sysbus. Its main purpose is to save and restore
> >>>> the kernel state on migration, but this will also allow to visualize it
> >>>> one day.
> >>>
> >>> I would prefer having no pre-save, and doing the ioctl in the state
> >>> change handler. But I won't nitpick about that, if the maintainers think
> >>> this is okay, all the rest of the patch looks fine as well.
> >>
> >> I did this for a reason: to be able to obtain the current clock state
> >> even while the vm is running. It's cleaner IMHO.
> > 
> > but if we're on pre-save, we are certain that the vm is stopped at this
> > moment, no ?
> 
> Maybe at the moment, but not for device state dumping or maybe (dunno)
> for Kemari's continuous sync'ing. I simply want to avoid this implicit
> dependency.

that's fine.

Patch

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 69b8234..47cb22b 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -29,6 +29,7 @@ 
 #include "hw/apic.h"
 #include "ioport.h"
 #include "kvm_x86.h"
+#include "hw/sysbus.h"
 
 #ifdef CONFIG_KVM_PARA
 #include <linux/kvm_para.h>
@@ -309,6 +310,85 @@  void kvm_inject_x86_mce(CPUState *cenv, int bank, uint64_t status,
 #endif
 }
 
+#if defined(CONFIG_KVM_PARA) && defined(KVM_CAP_ADJUST_CLOCK)
+typedef struct KVMClockState {
+    SysBusDevice busdev;
+    uint64_t clock;
+    bool clock_valid;
+} KVMClockState;
+
+static void kvmclock_pre_save(void *opaque)
+{
+    KVMClockState *s = opaque;
+    struct kvm_clock_data data;
+    int ret;
+
+    if (s->clock_valid) {
+        return;
+    }
+    ret = kvm_vm_ioctl(KVM_GET_CLOCK, &data);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+        data.clock = 0;
+    }
+    s->clock = data.clock;
+    /*
+     * If the VM is stopped, declare the clock state valid to avoid re-reading
+     * it on next vmsave (which would return a different value). Will be reset
+     * when the VM is continued.
+     */
+    s->clock_valid = !vm_running;
+}
+
+static int kvmclock_post_load(void *opaque, int version_id)
+{
+    KVMClockState *s = opaque;
+    struct kvm_clock_data data;
+
+    data.clock = s->clock;
+    data.flags = 0;
+    return kvm_vm_ioctl(KVM_SET_CLOCK, &data);
+}
+
+static void kvmclock_vm_state_change(void *opaque, int running, int reason)
+{
+    KVMClockState *s = opaque;
+
+    if (running) {
+        s->clock_valid = false;
+    }
+}
+
+static int kvmclock_init(SysBusDevice *dev)
+{
+    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
+
+    qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
+    return 0;
+}
+
+static const VMStateDescription kvmclock_vmsd= {
+    .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_UINT64(clock, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static SysBusDeviceInfo kvmclock_info = {
+    .qdev.name = "kvmclock",
+    .qdev.size = sizeof(KVMClockState),
+    .qdev.vmsd = &kvmclock_vmsd,
+    .qdev.no_user = 1,
+    .init = kvmclock_init,
+};
+#endif /* CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK */
+
 int kvm_arch_init_vcpu(CPUState *env)
 {
     struct {
@@ -335,7 +415,6 @@  int kvm_arch_init_vcpu(CPUState *env)
     env->cpuid_svm_features  &= kvm_x86_get_supported_cpuid(0x8000000A,
                                                             0, R_EDX);
 
-
     cpuid_i = 0;
 
 #ifdef CONFIG_KVM_PARA
@@ -442,6 +521,13 @@  int kvm_arch_init_vcpu(CPUState *env)
     }
 #endif
 
+#if defined(CONFIG_KVM_PARA) && defined(KVM_CAP_ADJUST_CLOCK)
+    if (cpu_is_bsp(env) &&
+        (env->cpuid_kvm_features & (1ULL << KVM_FEATURE_CLOCKSOURCE))) {
+        sysbus_create_simple("kvmclock", -1, NULL);
+    }
+#endif
+
     return kvm_vcpu_ioctl(env, KVM_SET_CPUID2, &cpuid_data);
 }
 
@@ -531,6 +617,10 @@  int kvm_arch_init(int smp_cpus)
     int ret;
     struct utsname utsname;
 
+#if defined(CONFIG_KVM_PARA) && defined(KVM_CAP_ADJUST_CLOCK)
+    sysbus_register_withprop(&kvmclock_info);
+#endif
+
     ret = kvm_get_supported_msrs();
     if (ret < 0) {
         return ret;