[PATCH v5 0/3] target-i386: save/restore vcpu's TSC rate during migration

[PATCH v5 0/3] target-i386: save/restore vcpu's TSC rate during migration

[Haozhong Zhang]

This patchset enables QEMU to save/restore vcpu's TSC rate during the
migration on machine types pc-*-2.5 or newer.

On the source machine:
 * If the vcpu's TSC rate is specified by the cpu option 'tsc-freq',
   then this user-specified TSC rate will be migrated.
 * Otherwise, the TSC rate returned by KVM_GET_TSC_KHZ will be
   migrated. For a fresh VM, this is the host TSC rate.

On the destination machine:
 * If the vcpu's TSC rate has been specified by the cpu option
   'tsc-freq' and is inconsistent with the migrated TSC rate, then
   the migration will be aborted.
 * Otherwise, QEMU will try to use the migrated TSC rate. If KVM on
   the destination supports TSC scaling, guest programs will observe a
   consistent TSC rate across the migration. If TSC scaling is not
   supported, the migration will not be aborted and QEMU will behave
   like before, i.e using the host TSC rate instead.

Changes in v5:
 * Move KVM_GET_TSC_KHZ call to kvm_arch_init_vcpu().
 * Remove an unnecessary warning message.
 * Unify TSC rate setting code in kvm_arch_init_vcpu() and
   kvm_arch_put_registers().

Changes in v4:
 * Make all code x86 specific.
 * Abort the migration if the user-specified TSC rate is inconsistent
   with the migrated TSC rate.
 * Move the sanity check to cpu_post_load().
 * All KVM_SET_TSC_KHZ and save/restore use env->tsc_khz.
 * Replace env->tsc_khz_saved with env->user_tsc_khz, and only use the
   latter for sanity check.

Changes in v3:
 * Change the cpu option 'save-tsc-freq' to an internal flag.
 * Remove the cpu option 'load-tsc-freq' and change the logic of
   loading the migrated TSC rate as above.
 * Move the setup of migrated TSC rate back to
   do_kvm_cpu_synchronize_post_init().

Changes in v2:
 * Add a pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' to
   control the migration of vcpu's TSC rate.
 * Move all logic of setting TSC rate to target-i386.
 * Remove the duplicated TSC setup in kvm_arch_init_vcpu().

Haozhong Zhang (3):
  target-i386: fallback vcpu's TSC rate to value returned by KVM
  target-i386: reorganize TSC rate setting code
  target-i386: add support to migrate vcpu's TSC rate

 hw/i386/pc.c          |  1 +
 hw/i386/pc_piix.c     |  1 +
 hw/i386/pc_q35.c      |  1 +
 include/hw/i386/pc.h  |  1 +
 target-i386/cpu.c     |  2 +-
 target-i386/cpu.h     |  1 +
 target-i386/kvm.c     | 59 +++++++++++++++++++++++++++++++++++++++++++++------
 target-i386/machine.c | 28 ++++++++++++++++++++++++
 8 files changed, 87 insertions(+), 7 deletions(-)

-- 
2.4.8

[Qemu-devel] [PATCH v5 0/3] target-i386: save/restore vcpu's TSC rate during migration

[Haozhong Zhang]

This patchset enables QEMU to save/restore vcpu's TSC rate during the
migration on machine types pc-*-2.5 or newer.

On the source machine:
 * If the vcpu's TSC rate is specified by the cpu option 'tsc-freq',
   then this user-specified TSC rate will be migrated.
 * Otherwise, the TSC rate returned by KVM_GET_TSC_KHZ will be
   migrated. For a fresh VM, this is the host TSC rate.

On the destination machine:
 * If the vcpu's TSC rate has been specified by the cpu option
   'tsc-freq' and is inconsistent with the migrated TSC rate, then
   the migration will be aborted.
 * Otherwise, QEMU will try to use the migrated TSC rate. If KVM on
   the destination supports TSC scaling, guest programs will observe a
   consistent TSC rate across the migration. If TSC scaling is not
   supported, the migration will not be aborted and QEMU will behave
   like before, i.e using the host TSC rate instead.

Changes in v5:
 * Move KVM_GET_TSC_KHZ call to kvm_arch_init_vcpu().
 * Remove an unnecessary warning message.
 * Unify TSC rate setting code in kvm_arch_init_vcpu() and
   kvm_arch_put_registers().

Changes in v4:
 * Make all code x86 specific.
 * Abort the migration if the user-specified TSC rate is inconsistent
   with the migrated TSC rate.
 * Move the sanity check to cpu_post_load().
 * All KVM_SET_TSC_KHZ and save/restore use env->tsc_khz.
 * Replace env->tsc_khz_saved with env->user_tsc_khz, and only use the
   latter for sanity check.

Changes in v3:
 * Change the cpu option 'save-tsc-freq' to an internal flag.
 * Remove the cpu option 'load-tsc-freq' and change the logic of
   loading the migrated TSC rate as above.
 * Move the setup of migrated TSC rate back to
   do_kvm_cpu_synchronize_post_init().

Changes in v2:
 * Add a pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' to
   control the migration of vcpu's TSC rate.
 * Move all logic of setting TSC rate to target-i386.
 * Remove the duplicated TSC setup in kvm_arch_init_vcpu().

Haozhong Zhang (3):
  target-i386: fallback vcpu's TSC rate to value returned by KVM
  target-i386: reorganize TSC rate setting code
  target-i386: add support to migrate vcpu's TSC rate

 hw/i386/pc.c          |  1 +
 hw/i386/pc_piix.c     |  1 +
 hw/i386/pc_q35.c      |  1 +
 include/hw/i386/pc.h  |  1 +
 target-i386/cpu.c     |  2 +-
 target-i386/cpu.h     |  1 +
 target-i386/kvm.c     | 59 +++++++++++++++++++++++++++++++++++++++++++++------
 target-i386/machine.c | 28 ++++++++++++++++++++++++
 8 files changed, 87 insertions(+), 7 deletions(-)

-- 
2.4.8

[PATCH v5 1/3] target-i386: fallback vcpu's TSC rate to value returned by KVM

[Haozhong Zhang]

If no user-specified TSC rate is present, we will try to set
env->tsc_khz to the value returned by KVM_GET_TSC_KHZ.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 target-i386/kvm.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 2a9953b..9e4d27f 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -832,6 +832,18 @@ int kvm_arch_init_vcpu(CPUState *cs)
         }
     }
 
+    /*
+     * If no user-specified TSC frequency is present, we will try to
+     * set env->tsc_khz to the value used by KVM.
+     */
+    if (!env->tsc_khz) {
+        r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
+            kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP;
+        if (r > 0) {
+            env->tsc_khz = r;
+        }
+    }
+
     if (has_xsave) {
         env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
     }
-- 
2.4.8

[Qemu-devel] [PATCH v5 1/3] target-i386: fallback vcpu's TSC rate to value returned by KVM

[Haozhong Zhang]

If no user-specified TSC rate is present, we will try to set
env->tsc_khz to the value returned by KVM_GET_TSC_KHZ.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 target-i386/kvm.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 2a9953b..9e4d27f 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -832,6 +832,18 @@ int kvm_arch_init_vcpu(CPUState *cs)
         }
     }
 
+    /*
+     * If no user-specified TSC frequency is present, we will try to
+     * set env->tsc_khz to the value used by KVM.
+     */
+    if (!env->tsc_khz) {
+        r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
+            kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP;
+        if (r > 0) {
+            env->tsc_khz = r;
+        }
+    }
+
     if (has_xsave) {
         env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
     }
-- 
2.4.8

[PATCH v5 2/3] target-i386: reorganize TSC rate setting code

[Haozhong Zhang]

Following two changes are made to the TSC rate setting code in
kvm_arch_init_vcpu():
 * The code is moved to a new function kvm_arch_set_tsc_khz().
 * If setting user-specified TSC rate fails and the host TSC rate is
   inconsistent with the user-specified one, print a warning message.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 target-i386/kvm.c | 45 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 9e4d27f..6a1acb4 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -524,6 +524,41 @@ static bool hyperv_enabled(X86CPU *cpu)
             cpu->hyperv_runtime);
 }
 
+/**
+ * Ask KVM to set vcpu's TSC rate to X86_CPU(cs)->env.tsc_khz.
+ *
+ * Returns: 0        if successful;
+ *          -ENOTSUP if KVM_CAP_TSC_CONTROL is unavailable;
+ *          -EIO     if KVM_SET_TSC_KHZ fails.
+ */
+static int kvm_arch_set_tsc_khz(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+    CPUX86State *env = &cpu->env;
+    int has_tsc_control, r = 0;
+
+    if (!env->tsc_khz) {
+        return 0;
+    }
+
+    has_tsc_control = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
+    if (has_tsc_control) {
+        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
+    }
+
+    if (!has_tsc_control || r < 0) {
+        r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
+            kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP;
+        if (r <= 0 || r != env->tsc_khz) {
+            error_report("warning: TSC frequency mismatch between "
+                         "VM and host, and TSC scaling unavailable");
+            return has_tsc_control ? -EIO : -ENOTSUP;
+        }
+    }
+
+    return 0;
+}
+
 static Error *invtsc_mig_blocker;
 
 #define KVM_MAX_CPUID_ENTRIES  100
@@ -823,13 +858,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
         return r;
     }
 
-    r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
-    if (r && env->tsc_khz) {
-        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
-        if (r < 0) {
-            fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
-            return r;
-        }
+    if (kvm_arch_set_tsc_khz(cs) == -EIO) {
+        fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
+        return -EIO;
     }
 
     /*
-- 
2.4.8

[Qemu-devel] [PATCH v5 2/3] target-i386: reorganize TSC rate setting code

[Haozhong Zhang]

Following two changes are made to the TSC rate setting code in
kvm_arch_init_vcpu():
 * The code is moved to a new function kvm_arch_set_tsc_khz().
 * If setting user-specified TSC rate fails and the host TSC rate is
   inconsistent with the user-specified one, print a warning message.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 target-i386/kvm.c | 45 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 9e4d27f..6a1acb4 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -524,6 +524,41 @@ static bool hyperv_enabled(X86CPU *cpu)
             cpu->hyperv_runtime);
 }
 
+/**
+ * Ask KVM to set vcpu's TSC rate to X86_CPU(cs)->env.tsc_khz.
+ *
+ * Returns: 0        if successful;
+ *          -ENOTSUP if KVM_CAP_TSC_CONTROL is unavailable;
+ *          -EIO     if KVM_SET_TSC_KHZ fails.
+ */
+static int kvm_arch_set_tsc_khz(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+    CPUX86State *env = &cpu->env;
+    int has_tsc_control, r = 0;
+
+    if (!env->tsc_khz) {
+        return 0;
+    }
+
+    has_tsc_control = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
+    if (has_tsc_control) {
+        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
+    }
+
+    if (!has_tsc_control || r < 0) {
+        r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
+            kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP;
+        if (r <= 0 || r != env->tsc_khz) {
+            error_report("warning: TSC frequency mismatch between "
+                         "VM and host, and TSC scaling unavailable");
+            return has_tsc_control ? -EIO : -ENOTSUP;
+        }
+    }
+
+    return 0;
+}
+
 static Error *invtsc_mig_blocker;
 
 #define KVM_MAX_CPUID_ENTRIES  100
@@ -823,13 +858,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
         return r;
     }
 
-    r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
-    if (r && env->tsc_khz) {
-        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
-        if (r < 0) {
-            fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
-            return r;
-        }
+    if (kvm_arch_set_tsc_khz(cs) == -EIO) {
+        fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
+        return -EIO;
     }
 
     /*
-- 
2.4.8

[PATCH v5 3/3] target-i386: add support to migrate vcpu's TSC rate

[Haozhong Zhang]

This patch enables migrating vcpu's TSC rate. If KVM on the destination
machine supports TSC scaling, guest programs will observe a consistent
TSC rate across the migration.

If TSC scaling is not supported on the destination machine, the
migration will not be aborted and QEMU on the destination will not set
vcpu's TSC rate to the migrated value.

If vcpu's TSC rate specified by CPU option 'tsc-freq' on the destination
machine is inconsistent with the migrated TSC rate, the migration will
be aborted.

For backwards compatibility, the migration of vcpu's TSC rate is
disabled on pc-*-2.4 and older machine types.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 hw/i386/pc.c          |  1 +
 hw/i386/pc_piix.c     |  1 +
 hw/i386/pc_q35.c      |  1 +
 include/hw/i386/pc.h  |  1 +
 target-i386/cpu.c     |  2 +-
 target-i386/cpu.h     |  1 +
 target-i386/kvm.c     |  4 ++++
 target-i386/machine.c | 28 ++++++++++++++++++++++++++++
 8 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0cb8afd..2f2fc93 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1952,6 +1952,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     pcmc->get_hotplug_handler = mc->get_hotplug_handler;
+    pcmc->save_tsc_khz = true;
     mc->get_hotplug_handler = pc_get_hotpug_handler;
     mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
     mc->default_boot_order = "cad";
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 07d0baa..7c5b0d2 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -489,6 +489,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
     m->alias = NULL;
     m->is_default = 0;
     pcmc->broken_reserved_end = true;
+    pcmc->save_tsc_khz = false;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0fdae09..fd8efe3 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -387,6 +387,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
     m->hw_version = "2.4.0";
     m->alias = NULL;
     pcmc->broken_reserved_end = true;
+    pcmc->save_tsc_khz = false;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 4bbc0ff..fea0f28 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -60,6 +60,7 @@ struct PCMachineClass {
 
     /*< public >*/
     bool broken_reserved_end;
+    bool save_tsc_khz;
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
 };
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e5f1c5b..98c6a4c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1724,7 +1724,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
         return;
     }
 
-    cpu->env.tsc_khz = value / 1000;
+    cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
 }
 
 static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaque,
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index fc4a605..ffe0bce 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -973,6 +973,7 @@ typedef struct CPUX86State {
     uint32_t sipi_vector;
     bool tsc_valid;
     int64_t tsc_khz;
+    int64_t user_tsc_khz; /* for sanity check only */
     void *kvm_xsave_buf;
 
     uint64_t mcg_cap;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 6a1acb4..6856899 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -2384,6 +2384,10 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
         }
     }
 
+    if (level == KVM_PUT_FULL_STATE) {
+        kvm_arch_set_tsc_khz(cpu);
+    }
+
     ret = kvm_getput_regs(x86_cpu, 1);
     if (ret < 0) {
         return ret;
diff --git a/target-i386/machine.c b/target-i386/machine.c
index a18e16e..3c5d24b 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -331,6 +331,13 @@ static int cpu_post_load(void *opaque, int version_id)
     CPUX86State *env = &cpu->env;
     int i;
 
+    if (env->tsc_khz && env->user_tsc_khz &&
+        env->tsc_khz != env->user_tsc_khz) {
+        fprintf(stderr, "Mismatch between user-specified TSC frequency and "
+                "migrated TSC frequency\n");
+        return -1;
+    }
+
     /*
      * Real mode guest segments register DPL should be zero.
      * Older KVM version were setting it wrongly.
@@ -775,6 +782,26 @@ static const VMStateDescription vmstate_xss = {
     }
 };
 
+static bool tsc_khz_needed(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    CPUX86State *env = &cpu->env;
+    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(mc);
+    return env->tsc_khz && pcmc->save_tsc_khz;
+}
+
+static const VMStateDescription vmstate_tsc_khz = {
+    .name = "cpu/tsc_khz",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = tsc_khz_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT64(env.tsc_khz, X86CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 VMStateDescription vmstate_x86_cpu = {
     .name = "cpu",
     .version_id = 12,
@@ -895,6 +922,7 @@ VMStateDescription vmstate_x86_cpu = {
         &vmstate_msr_hyperv_runtime,
         &vmstate_avx512,
         &vmstate_xss,
+        &vmstate_tsc_khz,
         NULL
     }
 };
-- 
2.4.8

[Qemu-devel] [PATCH v5 3/3] target-i386: add support to migrate vcpu's TSC rate

[Haozhong Zhang]

This patch enables migrating vcpu's TSC rate. If KVM on the destination
machine supports TSC scaling, guest programs will observe a consistent
TSC rate across the migration.

If TSC scaling is not supported on the destination machine, the
migration will not be aborted and QEMU on the destination will not set
vcpu's TSC rate to the migrated value.

If vcpu's TSC rate specified by CPU option 'tsc-freq' on the destination
machine is inconsistent with the migrated TSC rate, the migration will
be aborted.

For backwards compatibility, the migration of vcpu's TSC rate is
disabled on pc-*-2.4 and older machine types.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 hw/i386/pc.c          |  1 +
 hw/i386/pc_piix.c     |  1 +
 hw/i386/pc_q35.c      |  1 +
 include/hw/i386/pc.h  |  1 +
 target-i386/cpu.c     |  2 +-
 target-i386/cpu.h     |  1 +
 target-i386/kvm.c     |  4 ++++
 target-i386/machine.c | 28 ++++++++++++++++++++++++++++
 8 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0cb8afd..2f2fc93 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1952,6 +1952,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     pcmc->get_hotplug_handler = mc->get_hotplug_handler;
+    pcmc->save_tsc_khz = true;
     mc->get_hotplug_handler = pc_get_hotpug_handler;
     mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
     mc->default_boot_order = "cad";
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 07d0baa..7c5b0d2 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -489,6 +489,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
     m->alias = NULL;
     m->is_default = 0;
     pcmc->broken_reserved_end = true;
+    pcmc->save_tsc_khz = false;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0fdae09..fd8efe3 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -387,6 +387,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
     m->hw_version = "2.4.0";
     m->alias = NULL;
     pcmc->broken_reserved_end = true;
+    pcmc->save_tsc_khz = false;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 4bbc0ff..fea0f28 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -60,6 +60,7 @@ struct PCMachineClass {
 
     /*< public >*/
     bool broken_reserved_end;
+    bool save_tsc_khz;
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
 };
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e5f1c5b..98c6a4c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1724,7 +1724,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
         return;
     }
 
-    cpu->env.tsc_khz = value / 1000;
+    cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
 }
 
 static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaque,
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index fc4a605..ffe0bce 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -973,6 +973,7 @@ typedef struct CPUX86State {
     uint32_t sipi_vector;
     bool tsc_valid;
     int64_t tsc_khz;
+    int64_t user_tsc_khz; /* for sanity check only */
     void *kvm_xsave_buf;
 
     uint64_t mcg_cap;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 6a1acb4..6856899 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -2384,6 +2384,10 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
         }
     }
 
+    if (level == KVM_PUT_FULL_STATE) {
+        kvm_arch_set_tsc_khz(cpu);
+    }
+
     ret = kvm_getput_regs(x86_cpu, 1);
     if (ret < 0) {
         return ret;
diff --git a/target-i386/machine.c b/target-i386/machine.c
index a18e16e..3c5d24b 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -331,6 +331,13 @@ static int cpu_post_load(void *opaque, int version_id)
     CPUX86State *env = &cpu->env;
     int i;
 
+    if (env->tsc_khz && env->user_tsc_khz &&
+        env->tsc_khz != env->user_tsc_khz) {
+        fprintf(stderr, "Mismatch between user-specified TSC frequency and "
+                "migrated TSC frequency\n");
+        return -1;
+    }
+
     /*
      * Real mode guest segments register DPL should be zero.
      * Older KVM version were setting it wrongly.
@@ -775,6 +782,26 @@ static const VMStateDescription vmstate_xss = {
     }
 };
 
+static bool tsc_khz_needed(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    CPUX86State *env = &cpu->env;
+    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(mc);
+    return env->tsc_khz && pcmc->save_tsc_khz;
+}
+
+static const VMStateDescription vmstate_tsc_khz = {
+    .name = "cpu/tsc_khz",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = tsc_khz_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT64(env.tsc_khz, X86CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 VMStateDescription vmstate_x86_cpu = {
     .name = "cpu",
     .version_id = 12,
@@ -895,6 +922,7 @@ VMStateDescription vmstate_x86_cpu = {
         &vmstate_msr_hyperv_runtime,
         &vmstate_avx512,
         &vmstate_xss,
+        &vmstate_tsc_khz,
         NULL
     }
 };
-- 
2.4.8

Re: [PATCH v5 1/3] target-i386: fallback vcpu's TSC rate to value returned by KVM

[Eduardo Habkost]

On Tue, Nov 17, 2015 at 01:20:37PM +0800, Haozhong Zhang wrote:
> If no user-specified TSC rate is present, we will try to set
> env->tsc_khz to the value returned by KVM_GET_TSC_KHZ.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  target-i386/kvm.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 2a9953b..9e4d27f 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -832,6 +832,18 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          }
>      }
>  
> +    /*
> +     * If no user-specified TSC frequency is present, we will try to
> +     * set env->tsc_khz to the value used by KVM.
> +     */

If you send a new version of this series, please to describe
"why", not "what". We can see in the code that we are setting
env->tsc to the value used by KVM, but the comment need to
explain why.

> +    if (!env->tsc_khz) {
> +        r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
> +            kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP;
> +        if (r > 0) {
> +            env->tsc_khz = r;
> +        }
> +    }
> +
>      if (has_xsave) {
>          env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
>      }
> -- 
> 2.4.8
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5 1/3] target-i386: fallback vcpu's TSC rate to value returned by KVM

[Eduardo Habkost]

On Tue, Nov 17, 2015 at 01:20:37PM +0800, Haozhong Zhang wrote:
> If no user-specified TSC rate is present, we will try to set
> env->tsc_khz to the value returned by KVM_GET_TSC_KHZ.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  target-i386/kvm.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 2a9953b..9e4d27f 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -832,6 +832,18 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          }
>      }
>  
> +    /*
> +     * If no user-specified TSC frequency is present, we will try to
> +     * set env->tsc_khz to the value used by KVM.
> +     */

If you send a new version of this series, please to describe
"why", not "what". We can see in the code that we are setting
env->tsc to the value used by KVM, but the comment need to
explain why.

> +    if (!env->tsc_khz) {
> +        r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
> +            kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP;
> +        if (r > 0) {
> +            env->tsc_khz = r;
> +        }
> +    }
> +
>      if (has_xsave) {
>          env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
>      }
> -- 
> 2.4.8
> 

-- 
Eduardo

Re: [PATCH v5 2/3] target-i386: reorganize TSC rate setting code

[Eduardo Habkost]

On Tue, Nov 17, 2015 at 01:20:38PM +0800, Haozhong Zhang wrote:
> Following two changes are made to the TSC rate setting code in
> kvm_arch_init_vcpu():
>  * The code is moved to a new function kvm_arch_set_tsc_khz().
>  * If setting user-specified TSC rate fails and the host TSC rate is
>    inconsistent with the user-specified one, print a warning message.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

This matches what I was expecting, and now I see that we don't
even need the user_tsc_khz field.

> ---
>  target-i386/kvm.c | 45 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 9e4d27f..6a1acb4 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -524,6 +524,41 @@ static bool hyperv_enabled(X86CPU *cpu)
>              cpu->hyperv_runtime);
>  }
>  
> +/**
> + * Ask KVM to set vcpu's TSC rate to X86_CPU(cs)->env.tsc_khz.
> + *
> + * Returns: 0        if successful;
> + *          -ENOTSUP if KVM_CAP_TSC_CONTROL is unavailable;
> + *          -EIO     if KVM_SET_TSC_KHZ fails.

If KVM_SET_TSC_KHZ fails, the error code will be useful to
understand what went wrong. It's better to return the error code
returned by KVM instead of -EIO.

> + */
> +static int kvm_arch_set_tsc_khz(CPUState *cs)
> +{
> +    X86CPU *cpu = X86_CPU(cs);
> +    CPUX86State *env = &cpu->env;
> +    int has_tsc_control, r = 0;
> +
> +    if (!env->tsc_khz) {
> +        return 0;
> +    }
> +
> +    has_tsc_control = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> +    if (has_tsc_control) {
> +        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> +    }
> +
> +    if (!has_tsc_control || r < 0) {
> +        r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
> +            kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP;
> +        if (r <= 0 || r != env->tsc_khz) {
> +            error_report("warning: TSC frequency mismatch between "
> +                         "VM and host, and TSC scaling unavailable");
> +            return has_tsc_control ? -EIO : -ENOTSUP;
> +        }
> +    }

What about:

    r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
        kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) :
        -ENOTSUP;
    if (r < 0) {
        /* If KVM_SET_TSC_KHZ fails, it is an error only if the
         * current TSC frequency doesn't match the one we want.
         */
        int cur_freq = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
                       kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) :
                       -ENOTSUP;
       if (cur_freq <= 0 || cur_freq != env->tsc_khz) {
           error_report("warning: TSC frequency mismatch between "
                        "VM and host, and TSC scaling unavailable");
           return r;
       }
    }

    return 0;

> +
> +    return 0;
> +}
> +
>  static Error *invtsc_mig_blocker;
>  
>  #define KVM_MAX_CPUID_ENTRIES  100
> @@ -823,13 +858,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          return r;
>      }
>  
> -    r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> -    if (r && env->tsc_khz) {
> -        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> -        if (r < 0) {
> -            fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> -            return r;
> -        }
> +    if (kvm_arch_set_tsc_khz(cs) == -EIO) {
> +        fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");

Now kvm_arch_set_tsc_khz() prints an error message, we can remove
this one.

> +        return -EIO;

To keep the previous behavior without losing the error code
returned by KVM, this could be written as:

    r = kvm_arch_set_tsc_khz(cs);
    if (r < 0 && r != -ENOTSUP) {
        return r;
    }

But I belive the previous behavior was wrong. If tsc-freq was
explicitly set by the user, we should abort if
KVM_CAP_TSC_CONTROL is unavailable.

So I suggest we simply do this:

    r = kvm_arch_set_tsc_khz(cs);
    if (r < 0) {
        return r;
    }

(In case you implement this behavior change in the same patch,
please mention that in the commit message.)

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5 2/3] target-i386: reorganize TSC rate setting code

[Eduardo Habkost]

On Tue, Nov 17, 2015 at 01:20:38PM +0800, Haozhong Zhang wrote:
> Following two changes are made to the TSC rate setting code in
> kvm_arch_init_vcpu():
>  * The code is moved to a new function kvm_arch_set_tsc_khz().
>  * If setting user-specified TSC rate fails and the host TSC rate is
>    inconsistent with the user-specified one, print a warning message.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

This matches what I was expecting, and now I see that we don't
even need the user_tsc_khz field.

> ---
>  target-i386/kvm.c | 45 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 9e4d27f..6a1acb4 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -524,6 +524,41 @@ static bool hyperv_enabled(X86CPU *cpu)
>              cpu->hyperv_runtime);
>  }
>  
> +/**
> + * Ask KVM to set vcpu's TSC rate to X86_CPU(cs)->env.tsc_khz.
> + *
> + * Returns: 0        if successful;
> + *          -ENOTSUP if KVM_CAP_TSC_CONTROL is unavailable;
> + *          -EIO     if KVM_SET_TSC_KHZ fails.

If KVM_SET_TSC_KHZ fails, the error code will be useful to
understand what went wrong. It's better to return the error code
returned by KVM instead of -EIO.

> + */
> +static int kvm_arch_set_tsc_khz(CPUState *cs)
> +{
> +    X86CPU *cpu = X86_CPU(cs);
> +    CPUX86State *env = &cpu->env;
> +    int has_tsc_control, r = 0;
> +
> +    if (!env->tsc_khz) {
> +        return 0;
> +    }
> +
> +    has_tsc_control = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> +    if (has_tsc_control) {
> +        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> +    }
> +
> +    if (!has_tsc_control || r < 0) {
> +        r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
> +            kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP;
> +        if (r <= 0 || r != env->tsc_khz) {
> +            error_report("warning: TSC frequency mismatch between "
> +                         "VM and host, and TSC scaling unavailable");
> +            return has_tsc_control ? -EIO : -ENOTSUP;
> +        }
> +    }

What about:

    r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
        kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) :
        -ENOTSUP;
    if (r < 0) {
        /* If KVM_SET_TSC_KHZ fails, it is an error only if the
         * current TSC frequency doesn't match the one we want.
         */
        int cur_freq = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
                       kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) :
                       -ENOTSUP;
       if (cur_freq <= 0 || cur_freq != env->tsc_khz) {
           error_report("warning: TSC frequency mismatch between "
                        "VM and host, and TSC scaling unavailable");
           return r;
       }
    }

    return 0;

> +
> +    return 0;
> +}
> +
>  static Error *invtsc_mig_blocker;
>  
>  #define KVM_MAX_CPUID_ENTRIES  100
> @@ -823,13 +858,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          return r;
>      }
>  
> -    r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> -    if (r && env->tsc_khz) {
> -        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> -        if (r < 0) {
> -            fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> -            return r;
> -        }
> +    if (kvm_arch_set_tsc_khz(cs) == -EIO) {
> +        fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");

Now kvm_arch_set_tsc_khz() prints an error message, we can remove
this one.

> +        return -EIO;

To keep the previous behavior without losing the error code
returned by KVM, this could be written as:

    r = kvm_arch_set_tsc_khz(cs);
    if (r < 0 && r != -ENOTSUP) {
        return r;
    }

But I belive the previous behavior was wrong. If tsc-freq was
explicitly set by the user, we should abort if
KVM_CAP_TSC_CONTROL is unavailable.

So I suggest we simply do this:

    r = kvm_arch_set_tsc_khz(cs);
    if (r < 0) {
        return r;
    }

(In case you implement this behavior change in the same patch,
please mention that in the commit message.)

-- 
Eduardo

Re: [PATCH v5 3/3] target-i386: add support to migrate vcpu's TSC rate

[Eduardo Habkost]

Hi,

On Tue, Nov 17, 2015 at 01:20:39PM +0800, Haozhong Zhang wrote:
> This patch enables migrating vcpu's TSC rate. If KVM on the destination
> machine supports TSC scaling, guest programs will observe a consistent
> TSC rate across the migration.
> 
> If TSC scaling is not supported on the destination machine, the
> migration will not be aborted and QEMU on the destination will not set
> vcpu's TSC rate to the migrated value.
> 
> If vcpu's TSC rate specified by CPU option 'tsc-freq' on the destination
> machine is inconsistent with the migrated TSC rate, the migration will
> be aborted.
> 
> For backwards compatibility, the migration of vcpu's TSC rate is
> disabled on pc-*-2.4 and older machine types.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Now the logic in this patch (and the rest of the series) looks
good to me. All my suggestions are only related to code comments
and error handling:

[...]
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 6a1acb4..6856899 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -2384,6 +2384,10 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
>          }
>      }
>  
> +    if (level == KVM_PUT_FULL_STATE) {
> +        kvm_arch_set_tsc_khz(cpu);

Please add a comment here indicating that errors are being
ignored, and explaining why.

> +    }
> +
>      ret = kvm_getput_regs(x86_cpu, 1);
>      if (ret < 0) {
>          return ret;
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index a18e16e..3c5d24b 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -331,6 +331,13 @@ static int cpu_post_load(void *opaque, int version_id)
>      CPUX86State *env = &cpu->env;
>      int i;
>  
> +    if (env->tsc_khz && env->user_tsc_khz &&
> +        env->tsc_khz != env->user_tsc_khz) {
> +        fprintf(stderr, "Mismatch between user-specified TSC frequency and "
> +                "migrated TSC frequency\n");

Please use error_report() instead of fprintf().

> +        return -1;

Please return a valid -errno value. Other post_load functions
that implement sanity checks use -EINVAL (e.g.
global_state_post_load(), configuration_post_load()), so that's
probably what we should do here.

> +    }
> +
>      /*
>       * Real mode guest segments register DPL should be zero.
>       * Older KVM version were setting it wrongly.
> @@ -775,6 +782,26 @@ static const VMStateDescription vmstate_xss = {
>      }
>  };
>  
> +static bool tsc_khz_needed(void *opaque)
> +{
> +    X86CPU *cpu = opaque;
> +    CPUX86State *env = &cpu->env;
> +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(mc);
> +    return env->tsc_khz && pcmc->save_tsc_khz;
> +}
> +
> +static const VMStateDescription vmstate_tsc_khz = {
> +    .name = "cpu/tsc_khz",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = tsc_khz_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT64(env.tsc_khz, X86CPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  VMStateDescription vmstate_x86_cpu = {
>      .name = "cpu",
>      .version_id = 12,
> @@ -895,6 +922,7 @@ VMStateDescription vmstate_x86_cpu = {
>          &vmstate_msr_hyperv_runtime,
>          &vmstate_avx512,
>          &vmstate_xss,
> +        &vmstate_tsc_khz,
>          NULL
>      }
>  };
> -- 
> 2.4.8
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5 3/3] target-i386: add support to migrate vcpu's TSC rate

[Eduardo Habkost]

Hi,

On Tue, Nov 17, 2015 at 01:20:39PM +0800, Haozhong Zhang wrote:
> This patch enables migrating vcpu's TSC rate. If KVM on the destination
> machine supports TSC scaling, guest programs will observe a consistent
> TSC rate across the migration.
> 
> If TSC scaling is not supported on the destination machine, the
> migration will not be aborted and QEMU on the destination will not set
> vcpu's TSC rate to the migrated value.
> 
> If vcpu's TSC rate specified by CPU option 'tsc-freq' on the destination
> machine is inconsistent with the migrated TSC rate, the migration will
> be aborted.
> 
> For backwards compatibility, the migration of vcpu's TSC rate is
> disabled on pc-*-2.4 and older machine types.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Now the logic in this patch (and the rest of the series) looks
good to me. All my suggestions are only related to code comments
and error handling:

[...]
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 6a1acb4..6856899 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -2384,6 +2384,10 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
>          }
>      }
>  
> +    if (level == KVM_PUT_FULL_STATE) {
> +        kvm_arch_set_tsc_khz(cpu);

Please add a comment here indicating that errors are being
ignored, and explaining why.

> +    }
> +
>      ret = kvm_getput_regs(x86_cpu, 1);
>      if (ret < 0) {
>          return ret;
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index a18e16e..3c5d24b 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -331,6 +331,13 @@ static int cpu_post_load(void *opaque, int version_id)
>      CPUX86State *env = &cpu->env;
>      int i;
>  
> +    if (env->tsc_khz && env->user_tsc_khz &&
> +        env->tsc_khz != env->user_tsc_khz) {
> +        fprintf(stderr, "Mismatch between user-specified TSC frequency and "
> +                "migrated TSC frequency\n");

Please use error_report() instead of fprintf().

> +        return -1;

Please return a valid -errno value. Other post_load functions
that implement sanity checks use -EINVAL (e.g.
global_state_post_load(), configuration_post_load()), so that's
probably what we should do here.

> +    }
> +
>      /*
>       * Real mode guest segments register DPL should be zero.
>       * Older KVM version were setting it wrongly.
> @@ -775,6 +782,26 @@ static const VMStateDescription vmstate_xss = {
>      }
>  };
>  
> +static bool tsc_khz_needed(void *opaque)
> +{
> +    X86CPU *cpu = opaque;
> +    CPUX86State *env = &cpu->env;
> +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(mc);
> +    return env->tsc_khz && pcmc->save_tsc_khz;
> +}
> +
> +static const VMStateDescription vmstate_tsc_khz = {
> +    .name = "cpu/tsc_khz",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = tsc_khz_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT64(env.tsc_khz, X86CPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  VMStateDescription vmstate_x86_cpu = {
>      .name = "cpu",
>      .version_id = 12,
> @@ -895,6 +922,7 @@ VMStateDescription vmstate_x86_cpu = {
>          &vmstate_msr_hyperv_runtime,
>          &vmstate_avx512,
>          &vmstate_xss,
> +        &vmstate_tsc_khz,
>          NULL
>      }
>  };
> -- 
> 2.4.8
> 

-- 
Eduardo

Re: [PATCH v5 1/3] target-i386: fallback vcpu's TSC rate to value returned by KVM

[Haozhong Zhang]

On 11/17/15 11:14, Eduardo Habkost wrote:
> On Tue, Nov 17, 2015 at 01:20:37PM +0800, Haozhong Zhang wrote:
> > If no user-specified TSC rate is present, we will try to set
> > env->tsc_khz to the value returned by KVM_GET_TSC_KHZ.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  target-i386/kvm.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 2a9953b..9e4d27f 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -832,6 +832,18 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >          }
> >      }
> >  
> > +    /*
> > +     * If no user-specified TSC frequency is present, we will try to
> > +     * set env->tsc_khz to the value used by KVM.
> > +     */
> 
> If you send a new version of this series, please to describe
> "why", not "what". We can see in the code that we are setting
> env->tsc to the value used by KVM, but the comment need to
> explain why.
>

I'll update comments in the next version.

Haozhong

> > +    if (!env->tsc_khz) {
> > +        r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
> > +            kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP;
> > +        if (r > 0) {
> > +            env->tsc_khz = r;
> > +        }
> > +    }
> > +
> >      if (has_xsave) {
> >          env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
> >      }
> > -- 
> > 2.4.8
> > 
> 
> -- 
> Eduardo

Re: [Qemu-devel] [PATCH v5 1/3] target-i386: fallback vcpu's TSC rate to value returned by KVM

[Haozhong Zhang]

On 11/17/15 11:14, Eduardo Habkost wrote:
> On Tue, Nov 17, 2015 at 01:20:37PM +0800, Haozhong Zhang wrote:
> > If no user-specified TSC rate is present, we will try to set
> > env->tsc_khz to the value returned by KVM_GET_TSC_KHZ.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  target-i386/kvm.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 2a9953b..9e4d27f 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -832,6 +832,18 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >          }
> >      }
> >  
> > +    /*
> > +     * If no user-specified TSC frequency is present, we will try to
> > +     * set env->tsc_khz to the value used by KVM.
> > +     */
> 
> If you send a new version of this series, please to describe
> "why", not "what". We can see in the code that we are setting
> env->tsc to the value used by KVM, but the comment need to
> explain why.
>

I'll update comments in the next version.

Haozhong

> > +    if (!env->tsc_khz) {
> > +        r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
> > +            kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP;
> > +        if (r > 0) {
> > +            env->tsc_khz = r;
> > +        }
> > +    }
> > +
> >      if (has_xsave) {
> >          env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
> >      }
> > -- 
> > 2.4.8
> > 
> 
> -- 
> Eduardo

Re: [PATCH v5 2/3] target-i386: reorganize TSC rate setting code

[Haozhong Zhang]

On 11/17/15 11:32, Eduardo Habkost wrote:
> On Tue, Nov 17, 2015 at 01:20:38PM +0800, Haozhong Zhang wrote:
> > Following two changes are made to the TSC rate setting code in
> > kvm_arch_init_vcpu():
> >  * The code is moved to a new function kvm_arch_set_tsc_khz().
> >  * If setting user-specified TSC rate fails and the host TSC rate is
> >    inconsistent with the user-specified one, print a warning message.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> 
> This matches what I was expecting, and now I see that we don't
> even need the user_tsc_khz field.
>

I guess you mean the user_tsc_khz field is not needed when setting TSC
rate. It's still needed in patch 3 to check if the migrated TSC rate
is consistent with the user-specified TSC rate (and of course it's not
in kvm_arch_set_tsc_khz()).

> > ---
> >  target-i386/kvm.c | 45 ++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 38 insertions(+), 7 deletions(-)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 9e4d27f..6a1acb4 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -524,6 +524,41 @@ static bool hyperv_enabled(X86CPU *cpu)
> >              cpu->hyperv_runtime);
> >  }
> >  
> > +/**
> > + * Ask KVM to set vcpu's TSC rate to X86_CPU(cs)->env.tsc_khz.
> > + *
> > + * Returns: 0        if successful;
> > + *          -ENOTSUP if KVM_CAP_TSC_CONTROL is unavailable;
> > + *          -EIO     if KVM_SET_TSC_KHZ fails.
> 
> If KVM_SET_TSC_KHZ fails, the error code will be useful to
> understand what went wrong. It's better to return the error code
> returned by KVM instead of -EIO.
>

Yes, I'll change in the next version.

> > + */
> > +static int kvm_arch_set_tsc_khz(CPUState *cs)
> > +{
> > +    X86CPU *cpu = X86_CPU(cs);
> > +    CPUX86State *env = &cpu->env;
> > +    int has_tsc_control, r = 0;
> > +
> > +    if (!env->tsc_khz) {
> > +        return 0;
> > +    }
> > +
> > +    has_tsc_control = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> > +    if (has_tsc_control) {
> > +        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> > +    }
> > +
> > +    if (!has_tsc_control || r < 0) {
> > +        r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
> > +            kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP;
> > +        if (r <= 0 || r != env->tsc_khz) {
> > +            error_report("warning: TSC frequency mismatch between "
> > +                         "VM and host, and TSC scaling unavailable");
> > +            return has_tsc_control ? -EIO : -ENOTSUP;
> > +        }
> > +    }
> 
> What about:
> 
>     r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
>         kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) :
>         -ENOTSUP;
>     if (r < 0) {
>         /* If KVM_SET_TSC_KHZ fails, it is an error only if the
>          * current TSC frequency doesn't match the one we want.
>          */
>         int cur_freq = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
>                        kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) :
>                        -ENOTSUP;
>        if (cur_freq <= 0 || cur_freq != env->tsc_khz) {
>            error_report("warning: TSC frequency mismatch between "
>                         "VM and host, and TSC scaling unavailable");
>            return r;
>        }
>     }
> 
>     return 0;
>

Yes, your suggestion is better.

> > +
> > +    return 0;
> > +}
> > +
> >  static Error *invtsc_mig_blocker;
> >  
> >  #define KVM_MAX_CPUID_ENTRIES  100
> > @@ -823,13 +858,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >          return r;
> >      }
> >  
> > -    r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> > -    if (r && env->tsc_khz) {
> > -        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> > -        if (r < 0) {
> > -            fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> > -            return r;
> > -        }
> > +    if (kvm_arch_set_tsc_khz(cs) == -EIO) {
> > +        fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> 
> Now kvm_arch_set_tsc_khz() prints an error message, we can remove
> this one.

will remove in the next version.

> 
> > +        return -EIO;
> 
> To keep the previous behavior without losing the error code
> returned by KVM, this could be written as:
> 
>     r = kvm_arch_set_tsc_khz(cs);
>     if (r < 0 && r != -ENOTSUP) {
>         return r;
>     }
> 
> But I belive the previous behavior was wrong. If tsc-freq was
> explicitly set by the user, we should abort if
> KVM_CAP_TSC_CONTROL is unavailable.
> 
> So I suggest we simply do this:
> 
>     r = kvm_arch_set_tsc_khz(cs);
>     if (r < 0) {
>         return r;
>     }
>

Yes, I also prefer this one. I'll change and update the commit message
in the next version.

Thanks,
Haozhong

> (In case you implement this behavior change in the same patch,
> please mention that in the commit message.)
> 
> -- 
> Eduardo

Re: [Qemu-devel] [PATCH v5 2/3] target-i386: reorganize TSC rate setting code

[Haozhong Zhang]

On 11/17/15 11:32, Eduardo Habkost wrote:
> On Tue, Nov 17, 2015 at 01:20:38PM +0800, Haozhong Zhang wrote:
> > Following two changes are made to the TSC rate setting code in
> > kvm_arch_init_vcpu():
> >  * The code is moved to a new function kvm_arch_set_tsc_khz().
> >  * If setting user-specified TSC rate fails and the host TSC rate is
> >    inconsistent with the user-specified one, print a warning message.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> 
> This matches what I was expecting, and now I see that we don't
> even need the user_tsc_khz field.
>

I guess you mean the user_tsc_khz field is not needed when setting TSC
rate. It's still needed in patch 3 to check if the migrated TSC rate
is consistent with the user-specified TSC rate (and of course it's not
in kvm_arch_set_tsc_khz()).

> > ---
> >  target-i386/kvm.c | 45 ++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 38 insertions(+), 7 deletions(-)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 9e4d27f..6a1acb4 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -524,6 +524,41 @@ static bool hyperv_enabled(X86CPU *cpu)
> >              cpu->hyperv_runtime);
> >  }
> >  
> > +/**
> > + * Ask KVM to set vcpu's TSC rate to X86_CPU(cs)->env.tsc_khz.
> > + *
> > + * Returns: 0        if successful;
> > + *          -ENOTSUP if KVM_CAP_TSC_CONTROL is unavailable;
> > + *          -EIO     if KVM_SET_TSC_KHZ fails.
> 
> If KVM_SET_TSC_KHZ fails, the error code will be useful to
> understand what went wrong. It's better to return the error code
> returned by KVM instead of -EIO.
>

Yes, I'll change in the next version.

> > + */
> > +static int kvm_arch_set_tsc_khz(CPUState *cs)
> > +{
> > +    X86CPU *cpu = X86_CPU(cs);
> > +    CPUX86State *env = &cpu->env;
> > +    int has_tsc_control, r = 0;
> > +
> > +    if (!env->tsc_khz) {
> > +        return 0;
> > +    }
> > +
> > +    has_tsc_control = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> > +    if (has_tsc_control) {
> > +        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> > +    }
> > +
> > +    if (!has_tsc_control || r < 0) {
> > +        r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
> > +            kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP;
> > +        if (r <= 0 || r != env->tsc_khz) {
> > +            error_report("warning: TSC frequency mismatch between "
> > +                         "VM and host, and TSC scaling unavailable");
> > +            return has_tsc_control ? -EIO : -ENOTSUP;
> > +        }
> > +    }
> 
> What about:
> 
>     r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
>         kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) :
>         -ENOTSUP;
>     if (r < 0) {
>         /* If KVM_SET_TSC_KHZ fails, it is an error only if the
>          * current TSC frequency doesn't match the one we want.
>          */
>         int cur_freq = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
>                        kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) :
>                        -ENOTSUP;
>        if (cur_freq <= 0 || cur_freq != env->tsc_khz) {
>            error_report("warning: TSC frequency mismatch between "
>                         "VM and host, and TSC scaling unavailable");
>            return r;
>        }
>     }
> 
>     return 0;
>

Yes, your suggestion is better.

> > +
> > +    return 0;
> > +}
> > +
> >  static Error *invtsc_mig_blocker;
> >  
> >  #define KVM_MAX_CPUID_ENTRIES  100
> > @@ -823,13 +858,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >          return r;
> >      }
> >  
> > -    r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> > -    if (r && env->tsc_khz) {
> > -        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> > -        if (r < 0) {
> > -            fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> > -            return r;
> > -        }
> > +    if (kvm_arch_set_tsc_khz(cs) == -EIO) {
> > +        fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> 
> Now kvm_arch_set_tsc_khz() prints an error message, we can remove
> this one.

will remove in the next version.

> 
> > +        return -EIO;
> 
> To keep the previous behavior without losing the error code
> returned by KVM, this could be written as:
> 
>     r = kvm_arch_set_tsc_khz(cs);
>     if (r < 0 && r != -ENOTSUP) {
>         return r;
>     }
> 
> But I belive the previous behavior was wrong. If tsc-freq was
> explicitly set by the user, we should abort if
> KVM_CAP_TSC_CONTROL is unavailable.
> 
> So I suggest we simply do this:
> 
>     r = kvm_arch_set_tsc_khz(cs);
>     if (r < 0) {
>         return r;
>     }
>

Yes, I also prefer this one. I'll change and update the commit message
in the next version.

Thanks,
Haozhong

> (In case you implement this behavior change in the same patch,
> please mention that in the commit message.)
> 
> -- 
> Eduardo

Re: [PATCH v5 3/3] target-i386: add support to migrate vcpu's TSC rate

[Haozhong Zhang]

On 11/17/15 11:40, Eduardo Habkost wrote:
> Hi,
> 
> On Tue, Nov 17, 2015 at 01:20:39PM +0800, Haozhong Zhang wrote:
> > This patch enables migrating vcpu's TSC rate. If KVM on the destination
> > machine supports TSC scaling, guest programs will observe a consistent
> > TSC rate across the migration.
> > 
> > If TSC scaling is not supported on the destination machine, the
> > migration will not be aborted and QEMU on the destination will not set
> > vcpu's TSC rate to the migrated value.
> > 
> > If vcpu's TSC rate specified by CPU option 'tsc-freq' on the destination
> > machine is inconsistent with the migrated TSC rate, the migration will
> > be aborted.
> > 
> > For backwards compatibility, the migration of vcpu's TSC rate is
> > disabled on pc-*-2.4 and older machine types.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> 
> Now the logic in this patch (and the rest of the series) looks
> good to me. All my suggestions are only related to code comments
> and error handling:
> 
> [...]
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 6a1acb4..6856899 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -2384,6 +2384,10 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
> >          }
> >      }
> >  
> > +    if (level == KVM_PUT_FULL_STATE) {
> > +        kvm_arch_set_tsc_khz(cpu);
> 
> Please add a comment here indicating that errors are being
> ignored, and explaining why.
>

will add

> > +    }
> > +
> >      ret = kvm_getput_regs(x86_cpu, 1);
> >      if (ret < 0) {
> >          return ret;
> > diff --git a/target-i386/machine.c b/target-i386/machine.c
> > index a18e16e..3c5d24b 100644
> > --- a/target-i386/machine.c
> > +++ b/target-i386/machine.c
> > @@ -331,6 +331,13 @@ static int cpu_post_load(void *opaque, int version_id)
> >      CPUX86State *env = &cpu->env;
> >      int i;
> >  
> > +    if (env->tsc_khz && env->user_tsc_khz &&
> > +        env->tsc_khz != env->user_tsc_khz) {
> > +        fprintf(stderr, "Mismatch between user-specified TSC frequency and "
> > +                "migrated TSC frequency\n");
> 
> Please use error_report() instead of fprintf().
>

will change

> > +        return -1;
> 
> Please return a valid -errno value. Other post_load functions
> that implement sanity checks use -EINVAL (e.g.
> global_state_post_load(), configuration_post_load()), so that's
> probably what we should do here.
>

will change

Thanks,
Haozhong

> > +    }
> > +
> >      /*
> >       * Real mode guest segments register DPL should be zero.
> >       * Older KVM version were setting it wrongly.
> > @@ -775,6 +782,26 @@ static const VMStateDescription vmstate_xss = {
> >      }
> >  };
> >  
> > +static bool tsc_khz_needed(void *opaque)
> > +{
> > +    X86CPU *cpu = opaque;
> > +    CPUX86State *env = &cpu->env;
> > +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> > +    PCMachineClass *pcmc = PC_MACHINE_CLASS(mc);
> > +    return env->tsc_khz && pcmc->save_tsc_khz;
> > +}
> > +
> > +static const VMStateDescription vmstate_tsc_khz = {
> > +    .name = "cpu/tsc_khz",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = tsc_khz_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_INT64(env.tsc_khz, X86CPU),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  VMStateDescription vmstate_x86_cpu = {
> >      .name = "cpu",
> >      .version_id = 12,
> > @@ -895,6 +922,7 @@ VMStateDescription vmstate_x86_cpu = {
> >          &vmstate_msr_hyperv_runtime,
> >          &vmstate_avx512,
> >          &vmstate_xss,
> > +        &vmstate_tsc_khz,
> >          NULL
> >      }
> >  };
> > -- 
> > 2.4.8
> > 
> 
> -- 
> Eduardo

Re: [Qemu-devel] [PATCH v5 3/3] target-i386: add support to migrate vcpu's TSC rate

[Haozhong Zhang]

On 11/17/15 11:40, Eduardo Habkost wrote:
> Hi,
> 
> On Tue, Nov 17, 2015 at 01:20:39PM +0800, Haozhong Zhang wrote:
> > This patch enables migrating vcpu's TSC rate. If KVM on the destination
> > machine supports TSC scaling, guest programs will observe a consistent
> > TSC rate across the migration.
> > 
> > If TSC scaling is not supported on the destination machine, the
> > migration will not be aborted and QEMU on the destination will not set
> > vcpu's TSC rate to the migrated value.
> > 
> > If vcpu's TSC rate specified by CPU option 'tsc-freq' on the destination
> > machine is inconsistent with the migrated TSC rate, the migration will
> > be aborted.
> > 
> > For backwards compatibility, the migration of vcpu's TSC rate is
> > disabled on pc-*-2.4 and older machine types.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> 
> Now the logic in this patch (and the rest of the series) looks
> good to me. All my suggestions are only related to code comments
> and error handling:
> 
> [...]
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 6a1acb4..6856899 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -2384,6 +2384,10 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
> >          }
> >      }
> >  
> > +    if (level == KVM_PUT_FULL_STATE) {
> > +        kvm_arch_set_tsc_khz(cpu);
> 
> Please add a comment here indicating that errors are being
> ignored, and explaining why.
>

will add

> > +    }
> > +
> >      ret = kvm_getput_regs(x86_cpu, 1);
> >      if (ret < 0) {
> >          return ret;
> > diff --git a/target-i386/machine.c b/target-i386/machine.c
> > index a18e16e..3c5d24b 100644
> > --- a/target-i386/machine.c
> > +++ b/target-i386/machine.c
> > @@ -331,6 +331,13 @@ static int cpu_post_load(void *opaque, int version_id)
> >      CPUX86State *env = &cpu->env;
> >      int i;
> >  
> > +    if (env->tsc_khz && env->user_tsc_khz &&
> > +        env->tsc_khz != env->user_tsc_khz) {
> > +        fprintf(stderr, "Mismatch between user-specified TSC frequency and "
> > +                "migrated TSC frequency\n");
> 
> Please use error_report() instead of fprintf().
>

will change

> > +        return -1;
> 
> Please return a valid -errno value. Other post_load functions
> that implement sanity checks use -EINVAL (e.g.
> global_state_post_load(), configuration_post_load()), so that's
> probably what we should do here.
>

will change

Thanks,
Haozhong

> > +    }
> > +
> >      /*
> >       * Real mode guest segments register DPL should be zero.
> >       * Older KVM version were setting it wrongly.
> > @@ -775,6 +782,26 @@ static const VMStateDescription vmstate_xss = {
> >      }
> >  };
> >  
> > +static bool tsc_khz_needed(void *opaque)
> > +{
> > +    X86CPU *cpu = opaque;
> > +    CPUX86State *env = &cpu->env;
> > +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> > +    PCMachineClass *pcmc = PC_MACHINE_CLASS(mc);
> > +    return env->tsc_khz && pcmc->save_tsc_khz;
> > +}
> > +
> > +static const VMStateDescription vmstate_tsc_khz = {
> > +    .name = "cpu/tsc_khz",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = tsc_khz_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_INT64(env.tsc_khz, X86CPU),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  VMStateDescription vmstate_x86_cpu = {
> >      .name = "cpu",
> >      .version_id = 12,
> > @@ -895,6 +922,7 @@ VMStateDescription vmstate_x86_cpu = {
> >          &vmstate_msr_hyperv_runtime,
> >          &vmstate_avx512,
> >          &vmstate_xss,
> > +        &vmstate_tsc_khz,
> >          NULL
> >      }
> >  };
> > -- 
> > 2.4.8
> > 
> 
> -- 
> Eduardo

Re: [PATCH v5 2/3] target-i386: reorganize TSC rate setting code

[Eduardo Habkost]

On Tue, Nov 17, 2015 at 10:07:53PM +0800, Haozhong Zhang wrote:
> On 11/17/15 11:32, Eduardo Habkost wrote:
> > On Tue, Nov 17, 2015 at 01:20:38PM +0800, Haozhong Zhang wrote:
> > > Following two changes are made to the TSC rate setting code in
> > > kvm_arch_init_vcpu():
> > >  * The code is moved to a new function kvm_arch_set_tsc_khz().
> > >  * If setting user-specified TSC rate fails and the host TSC rate is
> > >    inconsistent with the user-specified one, print a warning message.
> > > 
> > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > 
> > This matches what I was expecting, and now I see that we don't
> > even need the user_tsc_khz field.
> >
> 
> I guess you mean the user_tsc_khz field is not needed when setting TSC
> rate. It's still needed in patch 3 to check if the migrated TSC rate
> is consistent with the user-specified TSC rate (and of course it's not
> in kvm_arch_set_tsc_khz()).

Yes, I was looking only at the error-checking logic in
kvm_arch_init_vcpu(). Then I noticed user_tsc_khz was added for
the migration sanity check (which makes sense).

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5 2/3] target-i386: reorganize TSC rate setting code

[Eduardo Habkost]

On Tue, Nov 17, 2015 at 10:07:53PM +0800, Haozhong Zhang wrote:
> On 11/17/15 11:32, Eduardo Habkost wrote:
> > On Tue, Nov 17, 2015 at 01:20:38PM +0800, Haozhong Zhang wrote:
> > > Following two changes are made to the TSC rate setting code in
> > > kvm_arch_init_vcpu():
> > >  * The code is moved to a new function kvm_arch_set_tsc_khz().
> > >  * If setting user-specified TSC rate fails and the host TSC rate is
> > >    inconsistent with the user-specified one, print a warning message.
> > > 
> > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > 
> > This matches what I was expecting, and now I see that we don't
> > even need the user_tsc_khz field.
> >
> 
> I guess you mean the user_tsc_khz field is not needed when setting TSC
> rate. It's still needed in patch 3 to check if the migrated TSC rate
> is consistent with the user-specified TSC rate (and of course it's not
> in kvm_arch_set_tsc_khz()).

Yes, I was looking only at the error-checking logic in
kvm_arch_init_vcpu(). Then I noticed user_tsc_khz was added for
the migration sanity check (which makes sense).

-- 
Eduardo