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

[PATCH v2 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. When cooperating with KVM which supports TSC scaling, guest
programs can observe a consistent guest TSC rate even though they are
migrated among machines with different host TSC rates.

A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
control the migration of vcpu's TSC rate.
 * By default, the migration of vcpu's TSC rate is enabled only on
   pc-*-2.5 and newer machine types. If the cpu option 'save-tsc-freq'
   is present, the vcpu's TSC rate will be migrated from older machine
   types as well.
 * Another cpu option 'load-tsc-freq' controls whether the migrated
   vcpu's TSC rate is used. By default, QEMU will not use the migrated
   TSC rate if this option is not present. Otherwise, QEMU will use
   the migrated TSC rate and override the TSC rate given by the cpu
   option 'tsc-freq'.

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: add a subsection for migrating vcpu's TSC rate
  target-i386: calculate vcpu's TSC rate to be migrated
  target-i386: load the migrated vcpu's TSC rate

 include/hw/i386/pc.h  |  5 +++++
 target-i386/cpu.c     |  2 ++
 target-i386/cpu.h     |  3 +++
 target-i386/kvm.c     | 61 +++++++++++++++++++++++++++++++++++++++++++--------
 target-i386/machine.c | 19 ++++++++++++++++
 5 files changed, 81 insertions(+), 9 deletions(-)

-- 
2.4.8

[Qemu-devel] [PATCH v2 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. When cooperating with KVM which supports TSC scaling, guest
programs can observe a consistent guest TSC rate even though they are
migrated among machines with different host TSC rates.

A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
control the migration of vcpu's TSC rate.
 * By default, the migration of vcpu's TSC rate is enabled only on
   pc-*-2.5 and newer machine types. If the cpu option 'save-tsc-freq'
   is present, the vcpu's TSC rate will be migrated from older machine
   types as well.
 * Another cpu option 'load-tsc-freq' controls whether the migrated
   vcpu's TSC rate is used. By default, QEMU will not use the migrated
   TSC rate if this option is not present. Otherwise, QEMU will use
   the migrated TSC rate and override the TSC rate given by the cpu
   option 'tsc-freq'.

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: add a subsection for migrating vcpu's TSC rate
  target-i386: calculate vcpu's TSC rate to be migrated
  target-i386: load the migrated vcpu's TSC rate

 include/hw/i386/pc.h  |  5 +++++
 target-i386/cpu.c     |  2 ++
 target-i386/cpu.h     |  3 +++
 target-i386/kvm.c     | 61 +++++++++++++++++++++++++++++++++++++++++++--------
 target-i386/machine.c | 19 ++++++++++++++++
 5 files changed, 81 insertions(+), 9 deletions(-)

-- 
2.4.8

[PATCH v2 1/3] target-i386: add a subsection for migrating vcpu's TSC rate

[Haozhong Zhang]

The newly added subsection 'vmstate_tsc_khz' is used by following
patches to migrate vcpu's TSC rate. For the back migration
compatibility, this subsection is not migrated on pc-*-2.4 and older
machine types by default. If users do want to migrate this subsection on
older machine types, they can enable it by giving a new cpu option
'save-tsc-freq'.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 include/hw/i386/pc.h  |  5 +++++
 target-i386/cpu.c     |  1 +
 target-i386/cpu.h     |  2 ++
 target-i386/machine.c | 19 +++++++++++++++++++
 4 files changed, 27 insertions(+)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 0503485..7fde50f 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -300,6 +300,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 #define PC_COMPAT_2_4 \
         HW_COMPAT_2_4 \
         {\
+            .driver   = TYPE_X86_CPU,\
+            .property = "save-tsc-freq",\
+            .value    = "off",\
+        },\
+        {\
             .driver   = "Haswell-" TYPE_X86_CPU,\
             .property = "abm",\
             .value    = "off",\
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 05d7f26..b6bb457 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3143,6 +3143,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, false),
     DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
     DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
+    DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, env.save_tsc_khz, true),
     DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
     DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
     DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 54d9d50..ba1a289 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -966,6 +966,8 @@ typedef struct CPUX86State {
     uint32_t sipi_vector;
     bool tsc_valid;
     int64_t tsc_khz;
+    int64_t tsc_khz_incoming;
+    bool save_tsc_khz;
     void *kvm_xsave_buf;
 
     uint64_t mcg_cap;
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 9fa0563..7d68d63 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -752,6 +752,24 @@ static const VMStateDescription vmstate_xss = {
     }
 };
 
+static bool tsc_khz_needed(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    CPUX86State *env = &cpu->env;
+    return env->tsc_khz_incoming && env->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_incoming, X86CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 VMStateDescription vmstate_x86_cpu = {
     .name = "cpu",
     .version_id = 12,
@@ -871,6 +889,7 @@ VMStateDescription vmstate_x86_cpu = {
         &vmstate_msr_hyperv_crash,
         &vmstate_avx512,
         &vmstate_xss,
+        &vmstate_tsc_khz,
         NULL
     }
 };
-- 
2.4.8

[Qemu-devel] [PATCH v2 1/3] target-i386: add a subsection for migrating vcpu's TSC rate

[Haozhong Zhang]

The newly added subsection 'vmstate_tsc_khz' is used by following
patches to migrate vcpu's TSC rate. For the back migration
compatibility, this subsection is not migrated on pc-*-2.4 and older
machine types by default. If users do want to migrate this subsection on
older machine types, they can enable it by giving a new cpu option
'save-tsc-freq'.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 include/hw/i386/pc.h  |  5 +++++
 target-i386/cpu.c     |  1 +
 target-i386/cpu.h     |  2 ++
 target-i386/machine.c | 19 +++++++++++++++++++
 4 files changed, 27 insertions(+)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 0503485..7fde50f 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -300,6 +300,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 #define PC_COMPAT_2_4 \
         HW_COMPAT_2_4 \
         {\
+            .driver   = TYPE_X86_CPU,\
+            .property = "save-tsc-freq",\
+            .value    = "off",\
+        },\
+        {\
             .driver   = "Haswell-" TYPE_X86_CPU,\
             .property = "abm",\
             .value    = "off",\
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 05d7f26..b6bb457 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3143,6 +3143,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, false),
     DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
     DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
+    DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, env.save_tsc_khz, true),
     DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
     DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
     DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 54d9d50..ba1a289 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -966,6 +966,8 @@ typedef struct CPUX86State {
     uint32_t sipi_vector;
     bool tsc_valid;
     int64_t tsc_khz;
+    int64_t tsc_khz_incoming;
+    bool save_tsc_khz;
     void *kvm_xsave_buf;
 
     uint64_t mcg_cap;
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 9fa0563..7d68d63 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -752,6 +752,24 @@ static const VMStateDescription vmstate_xss = {
     }
 };
 
+static bool tsc_khz_needed(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    CPUX86State *env = &cpu->env;
+    return env->tsc_khz_incoming && env->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_incoming, X86CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 VMStateDescription vmstate_x86_cpu = {
     .name = "cpu",
     .version_id = 12,
@@ -871,6 +889,7 @@ VMStateDescription vmstate_x86_cpu = {
         &vmstate_msr_hyperv_crash,
         &vmstate_avx512,
         &vmstate_xss,
+        &vmstate_tsc_khz,
         NULL
     }
 };
-- 
2.4.8

[PATCH v2 2/3] target-i386: calculate vcpu's TSC rate to be migrated

[Haozhong Zhang]

If vcpu's TSC rate is not specified by the cpu option 'tsc-freq', we
will use the value returned by KVM_GET_TSC_KHZ; otherwise, we use the
user-specified value.

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

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 80d1a7e..698524a 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -2213,6 +2213,35 @@ static int kvm_get_debugregs(X86CPU *cpu)
     return 0;
 }
 
+static int kvm_setup_tsc_khz(X86CPU *cpu, int level)
+{
+    CPUState *cs = CPU(cpu);
+    CPUX86State *env = &cpu->env;
+    int r;
+
+    if (level < KVM_PUT_FULL_STATE)
+        return 0;
+
+    /*
+     * Prepare the vcpu's TSC rate (ie. env->tsc_khz_incoming) to be migrated.
+     * 1. If no user-specified value is provided, we will use the value from
+     *    KVM;
+     * 2. Otherwise, we just use the user-specified value.
+     */
+    if (!env->tsc_khz) {
+        r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
+        if (r < 0) {
+            fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
+            return r;
+        }
+        env->tsc_khz_incoming = r;
+    } else {
+        env->tsc_khz_incoming = env->tsc_khz;
+    }
+
+    return 0;
+}
+
 int kvm_arch_put_registers(CPUState *cpu, int level)
 {
     X86CPU *x86_cpu = X86_CPU(cpu);
@@ -2248,6 +2277,10 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
     if (ret < 0) {
         return ret;
     }
+    ret = kvm_setup_tsc_khz(x86_cpu, level);
+    if (ret < 0) {
+        return ret;
+    }
     ret = kvm_put_msrs(x86_cpu, level);
     if (ret < 0) {
         return ret;
-- 
2.4.8

[Qemu-devel] [PATCH v2 2/3] target-i386: calculate vcpu's TSC rate to be migrated

[Haozhong Zhang]

If vcpu's TSC rate is not specified by the cpu option 'tsc-freq', we
will use the value returned by KVM_GET_TSC_KHZ; otherwise, we use the
user-specified value.

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

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 80d1a7e..698524a 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -2213,6 +2213,35 @@ static int kvm_get_debugregs(X86CPU *cpu)
     return 0;
 }
 
+static int kvm_setup_tsc_khz(X86CPU *cpu, int level)
+{
+    CPUState *cs = CPU(cpu);
+    CPUX86State *env = &cpu->env;
+    int r;
+
+    if (level < KVM_PUT_FULL_STATE)
+        return 0;
+
+    /*
+     * Prepare the vcpu's TSC rate (ie. env->tsc_khz_incoming) to be migrated.
+     * 1. If no user-specified value is provided, we will use the value from
+     *    KVM;
+     * 2. Otherwise, we just use the user-specified value.
+     */
+    if (!env->tsc_khz) {
+        r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
+        if (r < 0) {
+            fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
+            return r;
+        }
+        env->tsc_khz_incoming = r;
+    } else {
+        env->tsc_khz_incoming = env->tsc_khz;
+    }
+
+    return 0;
+}
+
 int kvm_arch_put_registers(CPUState *cpu, int level)
 {
     X86CPU *x86_cpu = X86_CPU(cpu);
@@ -2248,6 +2277,10 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
     if (ret < 0) {
         return ret;
     }
+    ret = kvm_setup_tsc_khz(x86_cpu, level);
+    if (ret < 0) {
+        return ret;
+    }
     ret = kvm_put_msrs(x86_cpu, level);
     if (ret < 0) {
         return ret;
-- 
2.4.8

[PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate

[Haozhong Zhang]

Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
scaling, guest programs will observe TSC increasing in the migrated rate
other than the host TSC rate.

The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
present, then the loading will be enabled and the migrated vcpu's TSC
rate will override the value specified by the cpu option
'tsc-freq'. Otherwise, the loading will be disabled.

The setting of vcpu's TSC rate in this patch duplicates the code in
kvm_arch_init_vcpu(), so we remove the latter one.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 target-i386/cpu.c |  1 +
 target-i386/cpu.h |  1 +
 target-i386/kvm.c | 28 +++++++++++++++++++---------
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b6bb457..763ba4b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3144,6 +3144,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
     DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
     DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, env.save_tsc_khz, true),
+    DEFINE_PROP_BOOL("load-tsc-freq", X86CPU, env.load_tsc_khz, false),
     DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
     DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
     DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index ba1a289..353f5fb 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -968,6 +968,7 @@ typedef struct CPUX86State {
     int64_t tsc_khz;
     int64_t tsc_khz_incoming;
     bool save_tsc_khz;
+    bool load_tsc_khz;
     void *kvm_xsave_buf;
 
     uint64_t mcg_cap;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 698524a..34616f5 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -743,15 +743,6 @@ 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_has_xsave()) {
         env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
     }
@@ -2223,6 +2214,25 @@ static int kvm_setup_tsc_khz(X86CPU *cpu, int level)
         return 0;
 
     /*
+     * If the cpu option 'load-tsc-freq' is present, the vcpu's TSC rate in the
+     * migrated state will be used and the overrides the user-specified vcpu's
+     * TSC rate (if any).
+     */
+    if (runstate_check(RUN_STATE_INMIGRATE) &&
+        env->load_tsc_khz && env->tsc_khz_incoming) {
+        env->tsc_khz = env->tsc_khz_incoming;
+    }
+
+    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;
+        }
+    }
+
+    /*
      * Prepare the vcpu's TSC rate (ie. env->tsc_khz_incoming) to be migrated.
      * 1. If no user-specified value is provided, we will use the value from
      *    KVM;
-- 
2.4.8

[Qemu-devel] [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate

[Haozhong Zhang]

Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
scaling, guest programs will observe TSC increasing in the migrated rate
other than the host TSC rate.

The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
present, then the loading will be enabled and the migrated vcpu's TSC
rate will override the value specified by the cpu option
'tsc-freq'. Otherwise, the loading will be disabled.

The setting of vcpu's TSC rate in this patch duplicates the code in
kvm_arch_init_vcpu(), so we remove the latter one.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 target-i386/cpu.c |  1 +
 target-i386/cpu.h |  1 +
 target-i386/kvm.c | 28 +++++++++++++++++++---------
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b6bb457..763ba4b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3144,6 +3144,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
     DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
     DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, env.save_tsc_khz, true),
+    DEFINE_PROP_BOOL("load-tsc-freq", X86CPU, env.load_tsc_khz, false),
     DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
     DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
     DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index ba1a289..353f5fb 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -968,6 +968,7 @@ typedef struct CPUX86State {
     int64_t tsc_khz;
     int64_t tsc_khz_incoming;
     bool save_tsc_khz;
+    bool load_tsc_khz;
     void *kvm_xsave_buf;
 
     uint64_t mcg_cap;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 698524a..34616f5 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -743,15 +743,6 @@ 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_has_xsave()) {
         env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
     }
@@ -2223,6 +2214,25 @@ static int kvm_setup_tsc_khz(X86CPU *cpu, int level)
         return 0;
 
     /*
+     * If the cpu option 'load-tsc-freq' is present, the vcpu's TSC rate in the
+     * migrated state will be used and the overrides the user-specified vcpu's
+     * TSC rate (if any).
+     */
+    if (runstate_check(RUN_STATE_INMIGRATE) &&
+        env->load_tsc_khz && env->tsc_khz_incoming) {
+        env->tsc_khz = env->tsc_khz_incoming;
+    }
+
+    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;
+        }
+    }
+
+    /*
      * Prepare the vcpu's TSC rate (ie. env->tsc_khz_incoming) to be migrated.
      * 1. If no user-specified value is provided, we will use the value from
      *    KVM;
-- 
2.4.8

Re: [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate

[Eduardo Habkost]

On Tue, Oct 20, 2015 at 03:22:54PM +0800, Haozhong Zhang wrote:
> Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
> scaling, guest programs will observe TSC increasing in the migrated rate
> other than the host TSC rate.
> 
> The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
> present, then the loading will be enabled and the migrated vcpu's TSC
> rate will override the value specified by the cpu option
> 'tsc-freq'. Otherwise, the loading will be disabled.

Why do we need an option? Why can't we enable loading unconditionally?

> 
> The setting of vcpu's TSC rate in this patch duplicates the code in
> kvm_arch_init_vcpu(), so we remove the latter one.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  target-i386/cpu.c |  1 +
>  target-i386/cpu.h |  1 +
>  target-i386/kvm.c | 28 +++++++++++++++++++---------
>  3 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index b6bb457..763ba4b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -3144,6 +3144,7 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
>      DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
>      DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, env.save_tsc_khz, true),
> +    DEFINE_PROP_BOOL("load-tsc-freq", X86CPU, env.load_tsc_khz, false),
>      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
>      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
>      DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index ba1a289..353f5fb 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -968,6 +968,7 @@ typedef struct CPUX86State {
>      int64_t tsc_khz;
>      int64_t tsc_khz_incoming;
>      bool save_tsc_khz;
> +    bool load_tsc_khz;
>      void *kvm_xsave_buf;
>  
>      uint64_t mcg_cap;
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 698524a..34616f5 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -743,15 +743,6 @@ 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_has_xsave()) {
>          env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
>      }
> @@ -2223,6 +2214,25 @@ static int kvm_setup_tsc_khz(X86CPU *cpu, int level)
>          return 0;
>  
>      /*
> +     * If the cpu option 'load-tsc-freq' is present, the vcpu's TSC rate in the
> +     * migrated state will be used and the overrides the user-specified vcpu's
> +     * TSC rate (if any).
> +     */
> +    if (runstate_check(RUN_STATE_INMIGRATE) &&
> +        env->load_tsc_khz && env->tsc_khz_incoming) {
> +        env->tsc_khz = env->tsc_khz_incoming;
> +    }

Please don't make the results of the function depend on global QEMU
runstate, as it makes it harder to reason about it, and easy to
introduce subtle bugs if we change initialization order. Can't we just
ensure tsc_khz gets set to the right value before the function is
called, inside the code that loads migration data?

> +
> +    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;
> +        }
> +    }

So, the final result here does not depend on the configuration, but also
on host capabilities. That means nobody can possibly know if the
tsc-freq option really works, until they enable it, run the VM, and
check the results from inside the VM. Not a good idea.

(This doesn't apply just to the new code, the existing code is already
broken this way.)

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate

[Eduardo Habkost]

On Tue, Oct 20, 2015 at 03:22:54PM +0800, Haozhong Zhang wrote:
> Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
> scaling, guest programs will observe TSC increasing in the migrated rate
> other than the host TSC rate.
> 
> The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
> present, then the loading will be enabled and the migrated vcpu's TSC
> rate will override the value specified by the cpu option
> 'tsc-freq'. Otherwise, the loading will be disabled.

Why do we need an option? Why can't we enable loading unconditionally?

> 
> The setting of vcpu's TSC rate in this patch duplicates the code in
> kvm_arch_init_vcpu(), so we remove the latter one.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  target-i386/cpu.c |  1 +
>  target-i386/cpu.h |  1 +
>  target-i386/kvm.c | 28 +++++++++++++++++++---------
>  3 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index b6bb457..763ba4b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -3144,6 +3144,7 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
>      DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
>      DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, env.save_tsc_khz, true),
> +    DEFINE_PROP_BOOL("load-tsc-freq", X86CPU, env.load_tsc_khz, false),
>      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
>      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
>      DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index ba1a289..353f5fb 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -968,6 +968,7 @@ typedef struct CPUX86State {
>      int64_t tsc_khz;
>      int64_t tsc_khz_incoming;
>      bool save_tsc_khz;
> +    bool load_tsc_khz;
>      void *kvm_xsave_buf;
>  
>      uint64_t mcg_cap;
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 698524a..34616f5 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -743,15 +743,6 @@ 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_has_xsave()) {
>          env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
>      }
> @@ -2223,6 +2214,25 @@ static int kvm_setup_tsc_khz(X86CPU *cpu, int level)
>          return 0;
>  
>      /*
> +     * If the cpu option 'load-tsc-freq' is present, the vcpu's TSC rate in the
> +     * migrated state will be used and the overrides the user-specified vcpu's
> +     * TSC rate (if any).
> +     */
> +    if (runstate_check(RUN_STATE_INMIGRATE) &&
> +        env->load_tsc_khz && env->tsc_khz_incoming) {
> +        env->tsc_khz = env->tsc_khz_incoming;
> +    }

Please don't make the results of the function depend on global QEMU
runstate, as it makes it harder to reason about it, and easy to
introduce subtle bugs if we change initialization order. Can't we just
ensure tsc_khz gets set to the right value before the function is
called, inside the code that loads migration data?

> +
> +    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;
> +        }
> +    }

So, the final result here does not depend on the configuration, but also
on host capabilities. That means nobody can possibly know if the
tsc-freq option really works, until they enable it, run the VM, and
check the results from inside the VM. Not a good idea.

(This doesn't apply just to the new code, the existing code is already
broken this way.)

-- 
Eduardo

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

[Eduardo Habkost]

On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> This patchset enables QEMU to save/restore vcpu's TSC rate during the
> migration. When cooperating with KVM which supports TSC scaling, guest
> programs can observe a consistent guest TSC rate even though they are
> migrated among machines with different host TSC rates.
> 
> A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> control the migration of vcpu's TSC rate.

The requirements and goals aren't clear to me. I see two possible use
cases, here:

1) Best effort to keep TSC frequency constant if possible (but not
   aborting migration if not possible). This would be an interesting
   default, but a bit unpredictable.
2) Strictly ensuring TSC frequency stays constant on migration (and
   aborting migration if not possible). This would be an useful feature,
   but can't be enabled by default unless both hosts have the same TSC
   frequency or support TSC scaling.

Which one(s) you are trying to implement?

In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
the requirements and goals are not clear.

Once we know what exactly is the goal, we could enable the new mode with
a single option, instead of raw options to control migration stream
loading/saving.


>  * By default, the migration of vcpu's TSC rate is enabled only on
>    pc-*-2.5 and newer machine types. If the cpu option 'save-tsc-freq'
>    is present, the vcpu's TSC rate will be migrated from older machine
>    types as well.
>  * Another cpu option 'load-tsc-freq' controls whether the migrated
>    vcpu's TSC rate is used. By default, QEMU will not use the migrated
>    TSC rate if this option is not present. Otherwise, QEMU will use
>    the migrated TSC rate and override the TSC rate given by the cpu
>    option 'tsc-freq'.
> 
> 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: add a subsection for migrating vcpu's TSC rate
>   target-i386: calculate vcpu's TSC rate to be migrated
>   target-i386: load the migrated vcpu's TSC rate
> 
>  include/hw/i386/pc.h  |  5 +++++
>  target-i386/cpu.c     |  2 ++
>  target-i386/cpu.h     |  3 +++
>  target-i386/kvm.c     | 61 +++++++++++++++++++++++++++++++++++++++++++--------
>  target-i386/machine.c | 19 ++++++++++++++++
>  5 files changed, 81 insertions(+), 9 deletions(-)
> 
> -- 
> 2.4.8
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> This patchset enables QEMU to save/restore vcpu's TSC rate during the
> migration. When cooperating with KVM which supports TSC scaling, guest
> programs can observe a consistent guest TSC rate even though they are
> migrated among machines with different host TSC rates.
> 
> A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> control the migration of vcpu's TSC rate.

The requirements and goals aren't clear to me. I see two possible use
cases, here:

1) Best effort to keep TSC frequency constant if possible (but not
   aborting migration if not possible). This would be an interesting
   default, but a bit unpredictable.
2) Strictly ensuring TSC frequency stays constant on migration (and
   aborting migration if not possible). This would be an useful feature,
   but can't be enabled by default unless both hosts have the same TSC
   frequency or support TSC scaling.

Which one(s) you are trying to implement?

In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
the requirements and goals are not clear.

Once we know what exactly is the goal, we could enable the new mode with
a single option, instead of raw options to control migration stream
loading/saving.


>  * By default, the migration of vcpu's TSC rate is enabled only on
>    pc-*-2.5 and newer machine types. If the cpu option 'save-tsc-freq'
>    is present, the vcpu's TSC rate will be migrated from older machine
>    types as well.
>  * Another cpu option 'load-tsc-freq' controls whether the migrated
>    vcpu's TSC rate is used. By default, QEMU will not use the migrated
>    TSC rate if this option is not present. Otherwise, QEMU will use
>    the migrated TSC rate and override the TSC rate given by the cpu
>    option 'tsc-freq'.
> 
> 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: add a subsection for migrating vcpu's TSC rate
>   target-i386: calculate vcpu's TSC rate to be migrated
>   target-i386: load the migrated vcpu's TSC rate
> 
>  include/hw/i386/pc.h  |  5 +++++
>  target-i386/cpu.c     |  2 ++
>  target-i386/cpu.h     |  3 +++
>  target-i386/kvm.c     | 61 +++++++++++++++++++++++++++++++++++++++++++--------
>  target-i386/machine.c | 19 ++++++++++++++++
>  5 files changed, 81 insertions(+), 9 deletions(-)
> 
> -- 
> 2.4.8
> 

-- 
Eduardo

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

[Haozhong Zhang]

On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > migration. When cooperating with KVM which supports TSC scaling, guest
> > programs can observe a consistent guest TSC rate even though they are
> > migrated among machines with different host TSC rates.
> > 
> > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > control the migration of vcpu's TSC rate.
> 
> The requirements and goals aren't clear to me. I see two possible use
> cases, here:
> 
> 1) Best effort to keep TSC frequency constant if possible (but not
>    aborting migration if not possible). This would be an interesting
>    default, but a bit unpredictable.
> 2) Strictly ensuring TSC frequency stays constant on migration (and
>    aborting migration if not possible). This would be an useful feature,
>    but can't be enabled by default unless both hosts have the same TSC
>    frequency or support TSC scaling.
> 
> Which one(s) you are trying to implement?
>

The former. I agree that it's unpredictable if setting vcpu's TSC
frequency to the migrated value is enabled by default (but not in this
patchset). The cpu option 'load-tsc-freq' is introduced to allow users
to enable this behavior if they do know the underlying KVM and CPU
support TSC scaling. In this way, I think the behavior is predictable
as users do know what they are doing.

> In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> the requirements and goals are not clear.
>

If KVM_CAP_TSC_CONTROL is unavailable, QEMU and KVM will use the host
TSC frequency as vcpu's TSC frequency.

If KVM_CAP_TSC_CONTROL is available and KVM_SET_TSC_KHZ fails, the
setting of TSC frequency will fail and abort either the VM creation
(this is the case for cpu option 'tsc-freq') or the migration.

> Once we know what exactly is the goal, we could enable the new mode with
> a single option, instead of raw options to control migration stream
> loading/saving.
>

Saving vcpu's TSC frequency does not depend on TSC scaling but the
loading does. And that is why I introduce two cpu options to control
them separately.

Haozhong

> 
> >  * By default, the migration of vcpu's TSC rate is enabled only on
> >    pc-*-2.5 and newer machine types. If the cpu option 'save-tsc-freq'
> >    is present, the vcpu's TSC rate will be migrated from older machine
> >    types as well.
> >  * Another cpu option 'load-tsc-freq' controls whether the migrated
> >    vcpu's TSC rate is used. By default, QEMU will not use the migrated
> >    TSC rate if this option is not present. Otherwise, QEMU will use
> >    the migrated TSC rate and override the TSC rate given by the cpu
> >    option 'tsc-freq'.
> > 
> > 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: add a subsection for migrating vcpu's TSC rate
> >   target-i386: calculate vcpu's TSC rate to be migrated
> >   target-i386: load the migrated vcpu's TSC rate
> > 
> >  include/hw/i386/pc.h  |  5 +++++
> >  target-i386/cpu.c     |  2 ++
> >  target-i386/cpu.h     |  3 +++
> >  target-i386/kvm.c     | 61 +++++++++++++++++++++++++++++++++++++++++++--------
> >  target-i386/machine.c | 19 ++++++++++++++++
> >  5 files changed, 81 insertions(+), 9 deletions(-)
> > 
> > -- 
> > 2.4.8
> > 
> 
> -- 
> Eduardo

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

[Haozhong Zhang]

On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > migration. When cooperating with KVM which supports TSC scaling, guest
> > programs can observe a consistent guest TSC rate even though they are
> > migrated among machines with different host TSC rates.
> > 
> > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > control the migration of vcpu's TSC rate.
> 
> The requirements and goals aren't clear to me. I see two possible use
> cases, here:
> 
> 1) Best effort to keep TSC frequency constant if possible (but not
>    aborting migration if not possible). This would be an interesting
>    default, but a bit unpredictable.
> 2) Strictly ensuring TSC frequency stays constant on migration (and
>    aborting migration if not possible). This would be an useful feature,
>    but can't be enabled by default unless both hosts have the same TSC
>    frequency or support TSC scaling.
> 
> Which one(s) you are trying to implement?
>

The former. I agree that it's unpredictable if setting vcpu's TSC
frequency to the migrated value is enabled by default (but not in this
patchset). The cpu option 'load-tsc-freq' is introduced to allow users
to enable this behavior if they do know the underlying KVM and CPU
support TSC scaling. In this way, I think the behavior is predictable
as users do know what they are doing.

> In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> the requirements and goals are not clear.
>

If KVM_CAP_TSC_CONTROL is unavailable, QEMU and KVM will use the host
TSC frequency as vcpu's TSC frequency.

If KVM_CAP_TSC_CONTROL is available and KVM_SET_TSC_KHZ fails, the
setting of TSC frequency will fail and abort either the VM creation
(this is the case for cpu option 'tsc-freq') or the migration.

> Once we know what exactly is the goal, we could enable the new mode with
> a single option, instead of raw options to control migration stream
> loading/saving.
>

Saving vcpu's TSC frequency does not depend on TSC scaling but the
loading does. And that is why I introduce two cpu options to control
them separately.

Haozhong

> 
> >  * By default, the migration of vcpu's TSC rate is enabled only on
> >    pc-*-2.5 and newer machine types. If the cpu option 'save-tsc-freq'
> >    is present, the vcpu's TSC rate will be migrated from older machine
> >    types as well.
> >  * Another cpu option 'load-tsc-freq' controls whether the migrated
> >    vcpu's TSC rate is used. By default, QEMU will not use the migrated
> >    TSC rate if this option is not present. Otherwise, QEMU will use
> >    the migrated TSC rate and override the TSC rate given by the cpu
> >    option 'tsc-freq'.
> > 
> > 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: add a subsection for migrating vcpu's TSC rate
> >   target-i386: calculate vcpu's TSC rate to be migrated
> >   target-i386: load the migrated vcpu's TSC rate
> > 
> >  include/hw/i386/pc.h  |  5 +++++
> >  target-i386/cpu.c     |  2 ++
> >  target-i386/cpu.h     |  3 +++
> >  target-i386/kvm.c     | 61 +++++++++++++++++++++++++++++++++++++++++++--------
> >  target-i386/machine.c | 19 ++++++++++++++++
> >  5 files changed, 81 insertions(+), 9 deletions(-)
> > 
> > -- 
> > 2.4.8
> > 
> 
> -- 
> Eduardo

Re: [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate

[Haozhong Zhang]

On Thu, Oct 22, 2015 at 04:11:37PM -0200, Eduardo Habkost wrote:
> On Tue, Oct 20, 2015 at 03:22:54PM +0800, Haozhong Zhang wrote:
> > Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
> > scaling, guest programs will observe TSC increasing in the migrated rate
> > other than the host TSC rate.
> > 
> > The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
> > present, then the loading will be enabled and the migrated vcpu's TSC
> > rate will override the value specified by the cpu option
> > 'tsc-freq'. Otherwise, the loading will be disabled.
> 
> Why do we need an option? Why can't we enable loading unconditionally?
>
> > 
> > The setting of vcpu's TSC rate in this patch duplicates the code in
> > kvm_arch_init_vcpu(), so we remove the latter one.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  target-i386/cpu.c |  1 +
> >  target-i386/cpu.h |  1 +
> >  target-i386/kvm.c | 28 +++++++++++++++++++---------
> >  3 files changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index b6bb457..763ba4b 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -3144,6 +3144,7 @@ static Property x86_cpu_properties[] = {
> >      DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
> >      DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
> >      DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, env.save_tsc_khz, true),
> > +    DEFINE_PROP_BOOL("load-tsc-freq", X86CPU, env.load_tsc_khz, false),
> >      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
> >      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
> >      DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index ba1a289..353f5fb 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -968,6 +968,7 @@ typedef struct CPUX86State {
> >      int64_t tsc_khz;
> >      int64_t tsc_khz_incoming;
> >      bool save_tsc_khz;
> > +    bool load_tsc_khz;
> >      void *kvm_xsave_buf;
> >  
> >      uint64_t mcg_cap;
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 698524a..34616f5 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -743,15 +743,6 @@ 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_has_xsave()) {
> >          env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
> >      }
> > @@ -2223,6 +2214,25 @@ static int kvm_setup_tsc_khz(X86CPU *cpu, int level)
> >          return 0;
> >  
> >      /*
> > +     * If the cpu option 'load-tsc-freq' is present, the vcpu's TSC rate in the
> > +     * migrated state will be used and the overrides the user-specified vcpu's
> > +     * TSC rate (if any).
> > +     */
> > +    if (runstate_check(RUN_STATE_INMIGRATE) &&
> > +        env->load_tsc_khz && env->tsc_khz_incoming) {
> > +        env->tsc_khz = env->tsc_khz_incoming;
> > +    }
> 
> Please don't make the results of the function depend on global QEMU
> runstate, as it makes it harder to reason about it, and easy to
> introduce subtle bugs if we change initialization order. Can't we just
> ensure tsc_khz gets set to the right value before the function is
> called, inside the code that loads migration data?
>

I can make kvm_setup_tsc_khz() called in
do_kvm_cpu_synchronize_post_init() and also make empty stubs for other
targets.

> > +
> > +    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;
> > +        }
> > +    }
> 
> So, the final result here does not depend on the configuration, but also
> on host capabilities. That means nobody can possibly know if the
> tsc-freq option really works, until they enable it, run the VM, and
> check the results from inside the VM. Not a good idea.
> 
> (This doesn't apply just to the new code, the existing code is already
> broken this way.)
>

Really should abort QEMU here for both tsc-freq and migration.

> -- 
> Eduardo

Re: [Qemu-devel] [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate

[Haozhong Zhang]

On Thu, Oct 22, 2015 at 04:11:37PM -0200, Eduardo Habkost wrote:
> On Tue, Oct 20, 2015 at 03:22:54PM +0800, Haozhong Zhang wrote:
> > Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
> > scaling, guest programs will observe TSC increasing in the migrated rate
> > other than the host TSC rate.
> > 
> > The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
> > present, then the loading will be enabled and the migrated vcpu's TSC
> > rate will override the value specified by the cpu option
> > 'tsc-freq'. Otherwise, the loading will be disabled.
> 
> Why do we need an option? Why can't we enable loading unconditionally?
>
> > 
> > The setting of vcpu's TSC rate in this patch duplicates the code in
> > kvm_arch_init_vcpu(), so we remove the latter one.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  target-i386/cpu.c |  1 +
> >  target-i386/cpu.h |  1 +
> >  target-i386/kvm.c | 28 +++++++++++++++++++---------
> >  3 files changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index b6bb457..763ba4b 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -3144,6 +3144,7 @@ static Property x86_cpu_properties[] = {
> >      DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
> >      DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
> >      DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, env.save_tsc_khz, true),
> > +    DEFINE_PROP_BOOL("load-tsc-freq", X86CPU, env.load_tsc_khz, false),
> >      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
> >      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
> >      DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index ba1a289..353f5fb 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -968,6 +968,7 @@ typedef struct CPUX86State {
> >      int64_t tsc_khz;
> >      int64_t tsc_khz_incoming;
> >      bool save_tsc_khz;
> > +    bool load_tsc_khz;
> >      void *kvm_xsave_buf;
> >  
> >      uint64_t mcg_cap;
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 698524a..34616f5 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -743,15 +743,6 @@ 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_has_xsave()) {
> >          env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
> >      }
> > @@ -2223,6 +2214,25 @@ static int kvm_setup_tsc_khz(X86CPU *cpu, int level)
> >          return 0;
> >  
> >      /*
> > +     * If the cpu option 'load-tsc-freq' is present, the vcpu's TSC rate in the
> > +     * migrated state will be used and the overrides the user-specified vcpu's
> > +     * TSC rate (if any).
> > +     */
> > +    if (runstate_check(RUN_STATE_INMIGRATE) &&
> > +        env->load_tsc_khz && env->tsc_khz_incoming) {
> > +        env->tsc_khz = env->tsc_khz_incoming;
> > +    }
> 
> Please don't make the results of the function depend on global QEMU
> runstate, as it makes it harder to reason about it, and easy to
> introduce subtle bugs if we change initialization order. Can't we just
> ensure tsc_khz gets set to the right value before the function is
> called, inside the code that loads migration data?
>

I can make kvm_setup_tsc_khz() called in
do_kvm_cpu_synchronize_post_init() and also make empty stubs for other
targets.

> > +
> > +    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;
> > +        }
> > +    }
> 
> So, the final result here does not depend on the configuration, but also
> on host capabilities. That means nobody can possibly know if the
> tsc-freq option really works, until they enable it, run the VM, and
> check the results from inside the VM. Not a good idea.
> 
> (This doesn't apply just to the new code, the existing code is already
> broken this way.)
>

Really should abort QEMU here for both tsc-freq and migration.

> -- 
> Eduardo

Re: [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate

[Haozhong Zhang]

On Thu, Oct 22, 2015 at 04:11:37PM -0200, Eduardo Habkost wrote:
> On Tue, Oct 20, 2015 at 03:22:54PM +0800, Haozhong Zhang wrote:
> > Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
> > scaling, guest programs will observe TSC increasing in the migrated rate
> > other than the host TSC rate.
> > 
> > The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
> > present, then the loading will be enabled and the migrated vcpu's TSC
> > rate will override the value specified by the cpu option
> > 'tsc-freq'. Otherwise, the loading will be disabled.
> 
> Why do we need an option? Why can't we enable loading unconditionally?
>

If TSC scaling is not supported by KVM and CPU, unconditionally
enabling this loading will not take effect which would be different
from users' expectation. 'load-tsc-freq' is introduced to allow users
to enable the loading of migrated TSC frequency if they do know the
underlying KVM and CPU have TSC scaling support.

> > 
> > The setting of vcpu's TSC rate in this patch duplicates the code in
> > kvm_arch_init_vcpu(), so we remove the latter one.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  target-i386/cpu.c |  1 +
> >  target-i386/cpu.h |  1 +
> >  target-i386/kvm.c | 28 +++++++++++++++++++---------
> >  3 files changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index b6bb457..763ba4b 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -3144,6 +3144,7 @@ static Property x86_cpu_properties[] = {
> >      DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
> >      DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
> >      DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, env.save_tsc_khz, true),
> > +    DEFINE_PROP_BOOL("load-tsc-freq", X86CPU, env.load_tsc_khz, false),
> >      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
> >      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
> >      DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index ba1a289..353f5fb 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -968,6 +968,7 @@ typedef struct CPUX86State {
> >      int64_t tsc_khz;
> >      int64_t tsc_khz_incoming;
> >      bool save_tsc_khz;
> > +    bool load_tsc_khz;
> >      void *kvm_xsave_buf;
> >  
> >      uint64_t mcg_cap;
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 698524a..34616f5 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -743,15 +743,6 @@ 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_has_xsave()) {
> >          env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
> >      }
> > @@ -2223,6 +2214,25 @@ static int kvm_setup_tsc_khz(X86CPU *cpu, int level)
> >          return 0;
> >  
> >      /*
> > +     * If the cpu option 'load-tsc-freq' is present, the vcpu's TSC rate in the
> > +     * migrated state will be used and the overrides the user-specified vcpu's
> > +     * TSC rate (if any).
> > +     */
> > +    if (runstate_check(RUN_STATE_INMIGRATE) &&
> > +        env->load_tsc_khz && env->tsc_khz_incoming) {
> > +        env->tsc_khz = env->tsc_khz_incoming;
> > +    }
> 
> Please don't make the results of the function depend on global QEMU
> runstate, as it makes it harder to reason about it, and easy to
> introduce subtle bugs if we change initialization order. Can't we just
> ensure tsc_khz gets set to the right value before the function is
> called, inside the code that loads migration data?
> 
> > +
> > +    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;
> > +        }
> > +    }
> 
> So, the final result here does not depend on the configuration, but also
> on host capabilities. That means nobody can possibly know if the
> tsc-freq option really works, until they enable it, run the VM, and
> check the results from inside the VM. Not a good idea.
> 
> (This doesn't apply just to the new code, the existing code is already
> broken this way.)
> 
> -- 
> Eduardo

Re: [Qemu-devel] [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate

[Haozhong Zhang]

On Thu, Oct 22, 2015 at 04:11:37PM -0200, Eduardo Habkost wrote:
> On Tue, Oct 20, 2015 at 03:22:54PM +0800, Haozhong Zhang wrote:
> > Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
> > scaling, guest programs will observe TSC increasing in the migrated rate
> > other than the host TSC rate.
> > 
> > The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
> > present, then the loading will be enabled and the migrated vcpu's TSC
> > rate will override the value specified by the cpu option
> > 'tsc-freq'. Otherwise, the loading will be disabled.
> 
> Why do we need an option? Why can't we enable loading unconditionally?
>

If TSC scaling is not supported by KVM and CPU, unconditionally
enabling this loading will not take effect which would be different
from users' expectation. 'load-tsc-freq' is introduced to allow users
to enable the loading of migrated TSC frequency if they do know the
underlying KVM and CPU have TSC scaling support.

> > 
> > The setting of vcpu's TSC rate in this patch duplicates the code in
> > kvm_arch_init_vcpu(), so we remove the latter one.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  target-i386/cpu.c |  1 +
> >  target-i386/cpu.h |  1 +
> >  target-i386/kvm.c | 28 +++++++++++++++++++---------
> >  3 files changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index b6bb457..763ba4b 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -3144,6 +3144,7 @@ static Property x86_cpu_properties[] = {
> >      DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
> >      DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
> >      DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, env.save_tsc_khz, true),
> > +    DEFINE_PROP_BOOL("load-tsc-freq", X86CPU, env.load_tsc_khz, false),
> >      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
> >      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
> >      DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index ba1a289..353f5fb 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -968,6 +968,7 @@ typedef struct CPUX86State {
> >      int64_t tsc_khz;
> >      int64_t tsc_khz_incoming;
> >      bool save_tsc_khz;
> > +    bool load_tsc_khz;
> >      void *kvm_xsave_buf;
> >  
> >      uint64_t mcg_cap;
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 698524a..34616f5 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -743,15 +743,6 @@ 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_has_xsave()) {
> >          env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
> >      }
> > @@ -2223,6 +2214,25 @@ static int kvm_setup_tsc_khz(X86CPU *cpu, int level)
> >          return 0;
> >  
> >      /*
> > +     * If the cpu option 'load-tsc-freq' is present, the vcpu's TSC rate in the
> > +     * migrated state will be used and the overrides the user-specified vcpu's
> > +     * TSC rate (if any).
> > +     */
> > +    if (runstate_check(RUN_STATE_INMIGRATE) &&
> > +        env->load_tsc_khz && env->tsc_khz_incoming) {
> > +        env->tsc_khz = env->tsc_khz_incoming;
> > +    }
> 
> Please don't make the results of the function depend on global QEMU
> runstate, as it makes it harder to reason about it, and easy to
> introduce subtle bugs if we change initialization order. Can't we just
> ensure tsc_khz gets set to the right value before the function is
> called, inside the code that loads migration data?
> 
> > +
> > +    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;
> > +        }
> > +    }
> 
> So, the final result here does not depend on the configuration, but also
> on host capabilities. That means nobody can possibly know if the
> tsc-freq option really works, until they enable it, run the VM, and
> check the results from inside the VM. Not a good idea.
> 
> (This doesn't apply just to the new code, the existing code is already
> broken this way.)
> 
> -- 
> Eduardo

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

[Marcelo Tosatti]

On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > migration. When cooperating with KVM which supports TSC scaling, guest
> > programs can observe a consistent guest TSC rate even though they are
> > migrated among machines with different host TSC rates.
> > 
> > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > control the migration of vcpu's TSC rate.
> 
> The requirements and goals aren't clear to me. I see two possible use
> cases, here:
> 
> 1) Best effort to keep TSC frequency constant if possible (but not
>    aborting migration if not possible). This would be an interesting
>    default, but a bit unpredictable.
> 2) Strictly ensuring TSC frequency stays constant on migration (and
>    aborting migration if not possible). This would be an useful feature,
>    but can't be enabled by default unless both hosts have the same TSC
>    frequency or support TSC scaling.

Only destination needs to support TSC scaling, to match the frequency
of the incoming host.

The KVM code for this feature has submitted or integrated? 

> Which one(s) you are trying to implement?
> 
> In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> the requirements and goals are not clear.
> 
> Once we know what exactly is the goal, we could enable the new mode with
> a single option, instead of raw options to control migration stream
> loading/saving.

Windows and Linux guests have paravirt clocks and/or options to
disable direct TSC usage for timekeeping purposes. So disabling
migration seems overkill.

> 
> 
> >  * By default, the migration of vcpu's TSC rate is enabled only on
> >    pc-*-2.5 and newer machine types. If the cpu option 'save-tsc-freq'
> >    is present, the vcpu's TSC rate will be migrated from older machine
> >    types as well.
> >  * Another cpu option 'load-tsc-freq' controls whether the migrated
> >    vcpu's TSC rate is used. By default, QEMU will not use the migrated
> >    TSC rate if this option is not present. Otherwise, QEMU will use
> >    the migrated TSC rate and override the TSC rate given by the cpu
> >    option 'tsc-freq'.
> > 
> > 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: add a subsection for migrating vcpu's TSC rate
> >   target-i386: calculate vcpu's TSC rate to be migrated
> >   target-i386: load the migrated vcpu's TSC rate
> > 
> >  include/hw/i386/pc.h  |  5 +++++
> >  target-i386/cpu.c     |  2 ++
> >  target-i386/cpu.h     |  3 +++
> >  target-i386/kvm.c     | 61 +++++++++++++++++++++++++++++++++++++++++++--------
> >  target-i386/machine.c | 19 ++++++++++++++++
> >  5 files changed, 81 insertions(+), 9 deletions(-)
> > 
> > -- 
> > 2.4.8
> > 
> 
> -- 
> Eduardo

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

[Marcelo Tosatti]

On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > migration. When cooperating with KVM which supports TSC scaling, guest
> > programs can observe a consistent guest TSC rate even though they are
> > migrated among machines with different host TSC rates.
> > 
> > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > control the migration of vcpu's TSC rate.
> 
> The requirements and goals aren't clear to me. I see two possible use
> cases, here:
> 
> 1) Best effort to keep TSC frequency constant if possible (but not
>    aborting migration if not possible). This would be an interesting
>    default, but a bit unpredictable.
> 2) Strictly ensuring TSC frequency stays constant on migration (and
>    aborting migration if not possible). This would be an useful feature,
>    but can't be enabled by default unless both hosts have the same TSC
>    frequency or support TSC scaling.

Only destination needs to support TSC scaling, to match the frequency
of the incoming host.

The KVM code for this feature has submitted or integrated? 

> Which one(s) you are trying to implement?
> 
> In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> the requirements and goals are not clear.
> 
> Once we know what exactly is the goal, we could enable the new mode with
> a single option, instead of raw options to control migration stream
> loading/saving.

Windows and Linux guests have paravirt clocks and/or options to
disable direct TSC usage for timekeeping purposes. So disabling
migration seems overkill.

> 
> 
> >  * By default, the migration of vcpu's TSC rate is enabled only on
> >    pc-*-2.5 and newer machine types. If the cpu option 'save-tsc-freq'
> >    is present, the vcpu's TSC rate will be migrated from older machine
> >    types as well.
> >  * Another cpu option 'load-tsc-freq' controls whether the migrated
> >    vcpu's TSC rate is used. By default, QEMU will not use the migrated
> >    TSC rate if this option is not present. Otherwise, QEMU will use
> >    the migrated TSC rate and override the TSC rate given by the cpu
> >    option 'tsc-freq'.
> > 
> > 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: add a subsection for migrating vcpu's TSC rate
> >   target-i386: calculate vcpu's TSC rate to be migrated
> >   target-i386: load the migrated vcpu's TSC rate
> > 
> >  include/hw/i386/pc.h  |  5 +++++
> >  target-i386/cpu.c     |  2 ++
> >  target-i386/cpu.h     |  3 +++
> >  target-i386/kvm.c     | 61 +++++++++++++++++++++++++++++++++++++++++++--------
> >  target-i386/machine.c | 19 ++++++++++++++++
> >  5 files changed, 81 insertions(+), 9 deletions(-)
> > 
> > -- 
> > 2.4.8
> > 
> 
> -- 
> Eduardo

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

[Eduardo Habkost]

On Fri, Oct 23, 2015 at 08:35:20AM -0200, Marcelo Tosatti wrote:
> On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> > On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > > migration. When cooperating with KVM which supports TSC scaling, guest
> > > programs can observe a consistent guest TSC rate even though they are
> > > migrated among machines with different host TSC rates.
> > > 
> > > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > > control the migration of vcpu's TSC rate.
> > 
> > The requirements and goals aren't clear to me. I see two possible use
> > cases, here:
> > 
> > 1) Best effort to keep TSC frequency constant if possible (but not
> >    aborting migration if not possible). This would be an interesting
> >    default, but a bit unpredictable.
> > 2) Strictly ensuring TSC frequency stays constant on migration (and
> >    aborting migration if not possible). This would be an useful feature,
> >    but can't be enabled by default unless both hosts have the same TSC
> >    frequency or support TSC scaling.
> 
> Only destination needs to support TSC scaling, to match the frequency
> of the incoming host.

True.

> 
> The KVM code for this feature has submitted or integrated? 
> 
> > Which one(s) you are trying to implement?
> > 
> > In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> > KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> > the requirements and goals are not clear.
> > 
> > Once we know what exactly is the goal, we could enable the new mode with
> > a single option, instead of raw options to control migration stream
> > loading/saving.
> 
> Windows and Linux guests have paravirt clocks and/or options to
> disable direct TSC usage for timekeeping purposes. So disabling
> migration seems overkill.

I assume that users who set TSC frequency explicitly in the VM config
care about it (otherwise they wouldn't be setting it explicitly) and
don't want it to change after migration.

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Oct 23, 2015 at 08:35:20AM -0200, Marcelo Tosatti wrote:
> On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> > On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > > migration. When cooperating with KVM which supports TSC scaling, guest
> > > programs can observe a consistent guest TSC rate even though they are
> > > migrated among machines with different host TSC rates.
> > > 
> > > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > > control the migration of vcpu's TSC rate.
> > 
> > The requirements and goals aren't clear to me. I see two possible use
> > cases, here:
> > 
> > 1) Best effort to keep TSC frequency constant if possible (but not
> >    aborting migration if not possible). This would be an interesting
> >    default, but a bit unpredictable.
> > 2) Strictly ensuring TSC frequency stays constant on migration (and
> >    aborting migration if not possible). This would be an useful feature,
> >    but can't be enabled by default unless both hosts have the same TSC
> >    frequency or support TSC scaling.
> 
> Only destination needs to support TSC scaling, to match the frequency
> of the incoming host.

True.

> 
> The KVM code for this feature has submitted or integrated? 
> 
> > Which one(s) you are trying to implement?
> > 
> > In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> > KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> > the requirements and goals are not clear.
> > 
> > Once we know what exactly is the goal, we could enable the new mode with
> > a single option, instead of raw options to control migration stream
> > loading/saving.
> 
> Windows and Linux guests have paravirt clocks and/or options to
> disable direct TSC usage for timekeeping purposes. So disabling
> migration seems overkill.

I assume that users who set TSC frequency explicitly in the VM config
care about it (otherwise they wouldn't be setting it explicitly) and
don't want it to change after migration.

-- 
Eduardo

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

[Haozhong Zhang]

On Fri, Oct 23, 2015 at 08:35:20AM -0200, Marcelo Tosatti wrote:
> On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> > On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > > migration. When cooperating with KVM which supports TSC scaling, guest
> > > programs can observe a consistent guest TSC rate even though they are
> > > migrated among machines with different host TSC rates.
> > > 
> > > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > > control the migration of vcpu's TSC rate.
> > 
> > The requirements and goals aren't clear to me. I see two possible use
> > cases, here:
> > 
> > 1) Best effort to keep TSC frequency constant if possible (but not
> >    aborting migration if not possible). This would be an interesting
> >    default, but a bit unpredictable.
> > 2) Strictly ensuring TSC frequency stays constant on migration (and
> >    aborting migration if not possible). This would be an useful feature,
> >    but can't be enabled by default unless both hosts have the same TSC
> >    frequency or support TSC scaling.
> 
> Only destination needs to support TSC scaling, to match the frequency
> of the incoming host.
>

Yes.

> The KVM code for this feature has submitted or integrated?

submitted and can be found at http://www.spinics.net/lists/kvm/msg122431.html

> 
> > Which one(s) you are trying to implement?
> > 
> > In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> > KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> > the requirements and goals are not clear.
> > 
> > Once we know what exactly is the goal, we could enable the new mode with
> > a single option, instead of raw options to control migration stream
> > loading/saving.
> 
> Windows and Linux guests have paravirt clocks and/or options to
> disable direct TSC usage for timekeeping purposes. So disabling
> migration seems overkill.
>

For KVM clock, guest users still need to know the host TSC (possibly
adjusted by scaling and offset) to know how long has passed since the
time provided by the PV clock. The KVM patch has adjusted KVM clock
for VMX TSC scaling so that it can be safely used across migration.

Haozhong

> > 
> > 
> > >  * By default, the migration of vcpu's TSC rate is enabled only on
> > >    pc-*-2.5 and newer machine types. If the cpu option 'save-tsc-freq'
> > >    is present, the vcpu's TSC rate will be migrated from older machine
> > >    types as well.
> > >  * Another cpu option 'load-tsc-freq' controls whether the migrated
> > >    vcpu's TSC rate is used. By default, QEMU will not use the migrated
> > >    TSC rate if this option is not present. Otherwise, QEMU will use
> > >    the migrated TSC rate and override the TSC rate given by the cpu
> > >    option 'tsc-freq'.
> > > 
> > > 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: add a subsection for migrating vcpu's TSC rate
> > >   target-i386: calculate vcpu's TSC rate to be migrated
> > >   target-i386: load the migrated vcpu's TSC rate
> > > 
> > >  include/hw/i386/pc.h  |  5 +++++
> > >  target-i386/cpu.c     |  2 ++
> > >  target-i386/cpu.h     |  3 +++
> > >  target-i386/kvm.c     | 61 +++++++++++++++++++++++++++++++++++++++++++--------
> > >  target-i386/machine.c | 19 ++++++++++++++++
> > >  5 files changed, 81 insertions(+), 9 deletions(-)
> > > 
> > > -- 
> > > 2.4.8
> > > 
> > 
> > -- 
> > Eduardo
> 

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

[Eduardo Habkost]

On Fri, Oct 23, 2015 at 10:27:27AM +0800, Haozhong Zhang wrote:
> On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> > On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > > migration. When cooperating with KVM which supports TSC scaling, guest
> > > programs can observe a consistent guest TSC rate even though they are
> > > migrated among machines with different host TSC rates.
> > > 
> > > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > > control the migration of vcpu's TSC rate.
> > 
> > The requirements and goals aren't clear to me. I see two possible use
> > cases, here:
> > 
> > 1) Best effort to keep TSC frequency constant if possible (but not
> >    aborting migration if not possible). This would be an interesting
> >    default, but a bit unpredictable.
> > 2) Strictly ensuring TSC frequency stays constant on migration (and
> >    aborting migration if not possible). This would be an useful feature,
> >    but can't be enabled by default unless both hosts have the same TSC
> >    frequency or support TSC scaling.
> > 
> > Which one(s) you are trying to implement?
> >
> 
> The former. I agree that it's unpredictable if setting vcpu's TSC
> frequency to the migrated value is enabled by default (but not in this
> patchset). The cpu option 'load-tsc-freq' is introduced to allow users
> to enable this behavior if they do know the underlying KVM and CPU
> support TSC scaling. In this way, I think the behavior is predictable
> as users do know what they are doing.

I'm confused. If load-tsc-freq doesn't abort when TSC scaling isn't
available (use case #1), why isn't it enabled by default? On the other
hand, if you expect the user to enable it only if the host supports TSC
scaling, why doesn't it abort if TSC scaling isn't available?

I mean, we can implement both use cases above this way:

1) If the user didn't ask for anything explicitly:
  * If the tsc-freq value is available in the migration stream, try to
    set it (but don't abort if it can't be set). (use case #1 above)
    * Rationale: it won't hurt to try to make the VM behave nicely if
      possible, without blocking migration if TSC scaling isn't
      available.
2) If the user asked for the TSC frequency to be enforced, set it and
  abort if it couldn't be set (use case #2 above). This could apply to
  both cases:
  2.1) If tsc-freq is explicitly set in the command-line.
    * Rationale: if the user asked for a specific frequency, we
      should do what was requested and not ignore errors silently.
  2.2) If tsc-freq is available in the migration stream, and the
    user asked explicitly for it to be enforced.
    * Rationale: the user is telling us that the incoming tsc-freq
      is important, so we shouldn't ignore it silently.
    * Open question: how should we name the new option?
      "load-tsc-freq" would be misleading because it won't be just about
      _loading_ tsc-freq (we would be loading it on use case #1, too),
      but about making sure it is enforced. "strict-tsc-freq"?
      "enforce-tsc-freq"?

We don't need to implement both #1 and #2 at the same time. But if you
just want to implement #1 first, I don't see the need for the
"load-tsc-freq" option.

On the migration source, we need another option or internal machine flag
for #1. I am not sure it should be an user-visible option. If
user-visible, I don't know how to name it. "save-tsc-freq" describes it
correctly, but it doesn't make its purpose very clear. Any suggestions?
It can also be implemented first as an internal machine class flag (set
in pc >= 2.5 only), and possibly become a user-visible option later.

> 
> > In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> > KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> > the requirements and goals are not clear.
> >
> 
> If KVM_CAP_TSC_CONTROL is unavailable, QEMU and KVM will use the host
> TSC frequency as vcpu's TSC frequency.
> 
> If KVM_CAP_TSC_CONTROL is available and KVM_SET_TSC_KHZ fails, the
> setting of TSC frequency will fail and abort either the VM creation
> (this is the case for cpu option 'tsc-freq') or the migration.

I don't see why the lack of KVM_CAP_TSC_CONTROL and failure of
KVM_SET_TSC_KHZ should be treated differently. In both cases it means we
the TSC frequency can't be set.

I mean: if KVM_SET_TSC_KHZ is important enough for the user to make QEMU
abort, it should abort if KVM_CAP_TSC_CONTROL isn't even available. On
the other hand, if the user doesn't care about the lack of
KVM_CAP_TSC_CONTROL (meaning it isn't possible to call KVM_SET_TSC_KHZ
at all), I don't see why they would care if KVM_SET_TSC_KHZ failed.


> 
> > Once we know what exactly is the goal, we could enable the new mode with
> > a single option, instead of raw options to control migration stream
> > loading/saving.
> >
> 
> Saving vcpu's TSC frequency does not depend on TSC scaling but the
> loading does. And that is why I introduce two cpu options to control
> them separately.

I understand we probably need internal flags on the source and
destination side to control saving/loading/setting the tsc-freq. But I
am still trying to clarify what are the configuration semantics we need,
exactly, to properly evaluate both the new configuration interface and
its implementation.

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Oct 23, 2015 at 10:27:27AM +0800, Haozhong Zhang wrote:
> On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> > On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > > migration. When cooperating with KVM which supports TSC scaling, guest
> > > programs can observe a consistent guest TSC rate even though they are
> > > migrated among machines with different host TSC rates.
> > > 
> > > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > > control the migration of vcpu's TSC rate.
> > 
> > The requirements and goals aren't clear to me. I see two possible use
> > cases, here:
> > 
> > 1) Best effort to keep TSC frequency constant if possible (but not
> >    aborting migration if not possible). This would be an interesting
> >    default, but a bit unpredictable.
> > 2) Strictly ensuring TSC frequency stays constant on migration (and
> >    aborting migration if not possible). This would be an useful feature,
> >    but can't be enabled by default unless both hosts have the same TSC
> >    frequency or support TSC scaling.
> > 
> > Which one(s) you are trying to implement?
> >
> 
> The former. I agree that it's unpredictable if setting vcpu's TSC
> frequency to the migrated value is enabled by default (but not in this
> patchset). The cpu option 'load-tsc-freq' is introduced to allow users
> to enable this behavior if they do know the underlying KVM and CPU
> support TSC scaling. In this way, I think the behavior is predictable
> as users do know what they are doing.

I'm confused. If load-tsc-freq doesn't abort when TSC scaling isn't
available (use case #1), why isn't it enabled by default? On the other
hand, if you expect the user to enable it only if the host supports TSC
scaling, why doesn't it abort if TSC scaling isn't available?

I mean, we can implement both use cases above this way:

1) If the user didn't ask for anything explicitly:
  * If the tsc-freq value is available in the migration stream, try to
    set it (but don't abort if it can't be set). (use case #1 above)
    * Rationale: it won't hurt to try to make the VM behave nicely if
      possible, without blocking migration if TSC scaling isn't
      available.
2) If the user asked for the TSC frequency to be enforced, set it and
  abort if it couldn't be set (use case #2 above). This could apply to
  both cases:
  2.1) If tsc-freq is explicitly set in the command-line.
    * Rationale: if the user asked for a specific frequency, we
      should do what was requested and not ignore errors silently.
  2.2) If tsc-freq is available in the migration stream, and the
    user asked explicitly for it to be enforced.
    * Rationale: the user is telling us that the incoming tsc-freq
      is important, so we shouldn't ignore it silently.
    * Open question: how should we name the new option?
      "load-tsc-freq" would be misleading because it won't be just about
      _loading_ tsc-freq (we would be loading it on use case #1, too),
      but about making sure it is enforced. "strict-tsc-freq"?
      "enforce-tsc-freq"?

We don't need to implement both #1 and #2 at the same time. But if you
just want to implement #1 first, I don't see the need for the
"load-tsc-freq" option.

On the migration source, we need another option or internal machine flag
for #1. I am not sure it should be an user-visible option. If
user-visible, I don't know how to name it. "save-tsc-freq" describes it
correctly, but it doesn't make its purpose very clear. Any suggestions?
It can also be implemented first as an internal machine class flag (set
in pc >= 2.5 only), and possibly become a user-visible option later.

> 
> > In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> > KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> > the requirements and goals are not clear.
> >
> 
> If KVM_CAP_TSC_CONTROL is unavailable, QEMU and KVM will use the host
> TSC frequency as vcpu's TSC frequency.
> 
> If KVM_CAP_TSC_CONTROL is available and KVM_SET_TSC_KHZ fails, the
> setting of TSC frequency will fail and abort either the VM creation
> (this is the case for cpu option 'tsc-freq') or the migration.

I don't see why the lack of KVM_CAP_TSC_CONTROL and failure of
KVM_SET_TSC_KHZ should be treated differently. In both cases it means we
the TSC frequency can't be set.

I mean: if KVM_SET_TSC_KHZ is important enough for the user to make QEMU
abort, it should abort if KVM_CAP_TSC_CONTROL isn't even available. On
the other hand, if the user doesn't care about the lack of
KVM_CAP_TSC_CONTROL (meaning it isn't possible to call KVM_SET_TSC_KHZ
at all), I don't see why they would care if KVM_SET_TSC_KHZ failed.


> 
> > Once we know what exactly is the goal, we could enable the new mode with
> > a single option, instead of raw options to control migration stream
> > loading/saving.
> >
> 
> Saving vcpu's TSC frequency does not depend on TSC scaling but the
> loading does. And that is why I introduce two cpu options to control
> them separately.

I understand we probably need internal flags on the source and
destination side to control saving/loading/setting the tsc-freq. But I
am still trying to clarify what are the configuration semantics we need,
exactly, to properly evaluate both the new configuration interface and
its implementation.

-- 
Eduardo

Re: [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate

[Eduardo Habkost]

On Fri, Oct 23, 2015 at 11:14:48AM +0800, Haozhong Zhang wrote:
> On Thu, Oct 22, 2015 at 04:11:37PM -0200, Eduardo Habkost wrote:
> > On Tue, Oct 20, 2015 at 03:22:54PM +0800, Haozhong Zhang wrote:
> > > Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
> > > scaling, guest programs will observe TSC increasing in the migrated rate
> > > other than the host TSC rate.
> > > 
> > > The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
> > > present, then the loading will be enabled and the migrated vcpu's TSC
> > > rate will override the value specified by the cpu option
> > > 'tsc-freq'. Otherwise, the loading will be disabled.
> > 
> > Why do we need an option? Why can't we enable loading unconditionally?
> >
> 
> If TSC scaling is not supported by KVM and CPU, unconditionally
> enabling this loading will not take effect which would be different
> from users' expectation. 'load-tsc-freq' is introduced to allow users
> to enable the loading of migrated TSC frequency if they do know the
> underlying KVM and CPU have TSC scaling support.
> 

I don't get your argument about user expectations. We can't read the
user's mind, but let's enumerate all possible scenarios:

* Host has TSC scaling, user expect TSC frequency to be set:
  * We set it. The user is happy.
* Host has TSC scaling, user doesn't expect TSC frequency to be
  set:
  * We still set it. VM behaves better, guest doesn't see changing TSC
    frequency. User didn't expect it but won't be unhappy.
* No TSC scaling, user expect TSC frequency to be set:
  * We won't set it, user will be unhappy. But I believe we all agree
    we shouldn't make QEMU abort migration by default on all hosts that
    don't support TSC scaling.
* No TSC scaling, user doesn't expect TSC frequency to be set:
  * We don't set it. User is happy.

Could you clarify on which items you disagree above, exactly?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate

[Eduardo Habkost]

On Fri, Oct 23, 2015 at 11:14:48AM +0800, Haozhong Zhang wrote:
> On Thu, Oct 22, 2015 at 04:11:37PM -0200, Eduardo Habkost wrote:
> > On Tue, Oct 20, 2015 at 03:22:54PM +0800, Haozhong Zhang wrote:
> > > Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
> > > scaling, guest programs will observe TSC increasing in the migrated rate
> > > other than the host TSC rate.
> > > 
> > > The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
> > > present, then the loading will be enabled and the migrated vcpu's TSC
> > > rate will override the value specified by the cpu option
> > > 'tsc-freq'. Otherwise, the loading will be disabled.
> > 
> > Why do we need an option? Why can't we enable loading unconditionally?
> >
> 
> If TSC scaling is not supported by KVM and CPU, unconditionally
> enabling this loading will not take effect which would be different
> from users' expectation. 'load-tsc-freq' is introduced to allow users
> to enable the loading of migrated TSC frequency if they do know the
> underlying KVM and CPU have TSC scaling support.
> 

I don't get your argument about user expectations. We can't read the
user's mind, but let's enumerate all possible scenarios:

* Host has TSC scaling, user expect TSC frequency to be set:
  * We set it. The user is happy.
* Host has TSC scaling, user doesn't expect TSC frequency to be
  set:
  * We still set it. VM behaves better, guest doesn't see changing TSC
    frequency. User didn't expect it but won't be unhappy.
* No TSC scaling, user expect TSC frequency to be set:
  * We won't set it, user will be unhappy. But I believe we all agree
    we shouldn't make QEMU abort migration by default on all hosts that
    don't support TSC scaling.
* No TSC scaling, user doesn't expect TSC frequency to be set:
  * We don't set it. User is happy.

Could you clarify on which items you disagree above, exactly?

-- 
Eduardo

Re: [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate

[haozhong.zhang]

On Fri, Oct 23, 2015 at 12:58:02PM -0200, Eduardo Habkost wrote:
> On Fri, Oct 23, 2015 at 11:14:48AM +0800, Haozhong Zhang wrote:
> > On Thu, Oct 22, 2015 at 04:11:37PM -0200, Eduardo Habkost wrote:
> > > On Tue, Oct 20, 2015 at 03:22:54PM +0800, Haozhong Zhang wrote:
> > > > Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
> > > > scaling, guest programs will observe TSC increasing in the migrated rate
> > > > other than the host TSC rate.
> > > > 
> > > > The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
> > > > present, then the loading will be enabled and the migrated vcpu's TSC
> > > > rate will override the value specified by the cpu option
> > > > 'tsc-freq'. Otherwise, the loading will be disabled.
> > > 
> > > Why do we need an option? Why can't we enable loading unconditionally?
> > >
> > 
> > If TSC scaling is not supported by KVM and CPU, unconditionally
> > enabling this loading will not take effect which would be different
> > from users' expectation. 'load-tsc-freq' is introduced to allow users
> > to enable the loading of migrated TSC frequency if they do know the
> > underlying KVM and CPU have TSC scaling support.
> > 
>

Sorry for the confusion, I changed my mind. The semantics of
'load-tsc-freq' is really confusing and we should not need it at all.

Now, what I want to implement is to migrate the TSC frequency as much
as possible. If it could not, QEMU does not abort the migration.

> I don't get your argument about user expectations. We can't read the
> user's mind, but let's enumerate all possible scenarios:
>
> * Host has TSC scaling, user expect TSC frequency to be set:
>   * We set it. The user is happy.
Agree.

> * Host has TSC scaling, user doesn't expect TSC frequency to be
>   set:
>   * We still set it. VM behaves better, guest doesn't see changing TSC
>     frequency. User didn't expect it but won't be unhappy.
Agree.

> * No TSC scaling, user expect TSC frequency to be set:
>   * We won't set it, user will be unhappy. But I believe we all agree
>     we shouldn't make QEMU abort migration by default on all hosts that
>     don't support TSC scaling.
Agree and display warning messages.

> * No TSC scaling, user doesn't expect TSC frequency to be set:
>   * We don't set it. User is happy.
Agree. This is the current QEMU's behavior, so it's still acceptable.

Thanks,
Haozhong

> 
> Could you clarify on which items you disagree above, exactly?
> 
> -- 
> Eduardo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Qemu-devel] [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate

[haozhong.zhang]

On Fri, Oct 23, 2015 at 12:58:02PM -0200, Eduardo Habkost wrote:
> On Fri, Oct 23, 2015 at 11:14:48AM +0800, Haozhong Zhang wrote:
> > On Thu, Oct 22, 2015 at 04:11:37PM -0200, Eduardo Habkost wrote:
> > > On Tue, Oct 20, 2015 at 03:22:54PM +0800, Haozhong Zhang wrote:
> > > > Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
> > > > scaling, guest programs will observe TSC increasing in the migrated rate
> > > > other than the host TSC rate.
> > > > 
> > > > The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
> > > > present, then the loading will be enabled and the migrated vcpu's TSC
> > > > rate will override the value specified by the cpu option
> > > > 'tsc-freq'. Otherwise, the loading will be disabled.
> > > 
> > > Why do we need an option? Why can't we enable loading unconditionally?
> > >
> > 
> > If TSC scaling is not supported by KVM and CPU, unconditionally
> > enabling this loading will not take effect which would be different
> > from users' expectation. 'load-tsc-freq' is introduced to allow users
> > to enable the loading of migrated TSC frequency if they do know the
> > underlying KVM and CPU have TSC scaling support.
> > 
>

Sorry for the confusion, I changed my mind. The semantics of
'load-tsc-freq' is really confusing and we should not need it at all.

Now, what I want to implement is to migrate the TSC frequency as much
as possible. If it could not, QEMU does not abort the migration.

> I don't get your argument about user expectations. We can't read the
> user's mind, but let's enumerate all possible scenarios:
>
> * Host has TSC scaling, user expect TSC frequency to be set:
>   * We set it. The user is happy.
Agree.

> * Host has TSC scaling, user doesn't expect TSC frequency to be
>   set:
>   * We still set it. VM behaves better, guest doesn't see changing TSC
>     frequency. User didn't expect it but won't be unhappy.
Agree.

> * No TSC scaling, user expect TSC frequency to be set:
>   * We won't set it, user will be unhappy. But I believe we all agree
>     we shouldn't make QEMU abort migration by default on all hosts that
>     don't support TSC scaling.
Agree and display warning messages.

> * No TSC scaling, user doesn't expect TSC frequency to be set:
>   * We don't set it. User is happy.
Agree. This is the current QEMU's behavior, so it's still acceptable.

Thanks,
Haozhong

> 
> Could you clarify on which items you disagree above, exactly?
> 
> -- 
> Eduardo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[haozhong.zhang]

On Fri, Oct 23, 2015 at 12:45:13PM -0200, Eduardo Habkost wrote:
> On Fri, Oct 23, 2015 at 10:27:27AM +0800, Haozhong Zhang wrote:
> > On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> > > On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > > > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > > > migration. When cooperating with KVM which supports TSC scaling, guest
> > > > programs can observe a consistent guest TSC rate even though they are
> > > > migrated among machines with different host TSC rates.
> > > > 
> > > > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > > > control the migration of vcpu's TSC rate.
> > > 
> > > The requirements and goals aren't clear to me. I see two possible use
> > > cases, here:
> > > 
> > > 1) Best effort to keep TSC frequency constant if possible (but not
> > >    aborting migration if not possible). This would be an interesting
> > >    default, but a bit unpredictable.
> > > 2) Strictly ensuring TSC frequency stays constant on migration (and
> > >    aborting migration if not possible). This would be an useful feature,
> > >    but can't be enabled by default unless both hosts have the same TSC
> > >    frequency or support TSC scaling.
> > > 
> > > Which one(s) you are trying to implement?
> > >
> > 
> > The former. I agree that it's unpredictable if setting vcpu's TSC
> > frequency to the migrated value is enabled by default (but not in this
> > patchset). The cpu option 'load-tsc-freq' is introduced to allow users
> > to enable this behavior if they do know the underlying KVM and CPU
> > support TSC scaling. In this way, I think the behavior is predictable
> > as users do know what they are doing.
> 
> I'm confused. If load-tsc-freq doesn't abort when TSC scaling isn't
> available (use case #1), why isn't it enabled by default? On the other
> hand, if you expect the user to enable it only if the host supports TSC
> scaling, why doesn't it abort if TSC scaling isn't available?
>

Sorry for the confusion. For user case #1, load-tsc-freq is really not
needed and the migrated TSC frequency should be set if possible
(ie. if TSC scaling is supported and KVM_SET_TSC_KHZ succeeds). If
setting TSC frequency fails, the migration will not be aborted.

> I mean, we can implement both use cases above this way:
> 
> 1) If the user didn't ask for anything explicitly:
>   * If the tsc-freq value is available in the migration stream, try to
>     set it (but don't abort if it can't be set). (use case #1 above)
>     * Rationale: it won't hurt to try to make the VM behave nicely if
>       possible, without blocking migration if TSC scaling isn't
>       available.
> 2) If the user asked for the TSC frequency to be enforced, set it and
>   abort if it couldn't be set (use case #2 above). This could apply to
>   both cases:
>   2.1) If tsc-freq is explicitly set in the command-line.
>     * Rationale: if the user asked for a specific frequency, we
>       should do what was requested and not ignore errors silently.
>   2.2) If tsc-freq is available in the migration stream, and the
>     user asked explicitly for it to be enforced.
>     * Rationale: the user is telling us that the incoming tsc-freq
>       is important, so we shouldn't ignore it silently.
>     * Open question: how should we name the new option?
>       "load-tsc-freq" would be misleading because it won't be just about
>       _loading_ tsc-freq (we would be loading it on use case #1, too),
>       but about making sure it is enforced. "strict-tsc-freq"?
>       "enforce-tsc-freq"?
> 
> We don't need to implement both #1 and #2 at the same time. But if you
> just want to implement #1 first, I don't see the need for the
> "load-tsc-freq" option.
> 
> On the migration source, we need another option or internal machine flag
> for #1. I am not sure it should be an user-visible option. If
> user-visible, I don't know how to name it. "save-tsc-freq" describes it
> correctly, but it doesn't make its purpose very clear. Any suggestions?
> It can also be implemented first as an internal machine class flag (set
> in pc >= 2.5 only), and possibly become a user-visible option later.
>

Because the way I implements 'save-tsc-freq' in patch 1, it's exposed
to users. I'm not familiar the way to make a feature only available
for newer machine types. Could you provide some suggestions to hide
'save-tsc-freq' from users?

For the name, if we make the option internal only, could we still use
'save-tsc-freq' as it does mean saving the TSC frequency.

> > 
> > > In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> > > KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> > > the requirements and goals are not clear.
> > >
> > 
> > If KVM_CAP_TSC_CONTROL is unavailable, QEMU and KVM will use the host
> > TSC frequency as vcpu's TSC frequency.
> > 
> > If KVM_CAP_TSC_CONTROL is available and KVM_SET_TSC_KHZ fails, the
> > setting of TSC frequency will fail and abort either the VM creation
> > (this is the case for cpu option 'tsc-freq') or the migration.
> 
> I don't see why the lack of KVM_CAP_TSC_CONTROL and failure of
> KVM_SET_TSC_KHZ should be treated differently. In both cases it means we
> the TSC frequency can't be set.
> 
> I mean: if KVM_SET_TSC_KHZ is important enough for the user to make QEMU
> abort, it should abort if KVM_CAP_TSC_CONTROL isn't even available. On
> the other hand, if the user doesn't care about the lack of
> KVM_CAP_TSC_CONTROL (meaning it isn't possible to call KVM_SET_TSC_KHZ
> at all), I don't see why they would care if KVM_SET_TSC_KHZ failed.
>

Yes, we should treat them equally. If either of KVM_CAP_TSC_CONTROL or
KVM_SET_TSC_KHZ fails, we should display a warning message (but not
fail the migration).

>
> > 
> > > Once we know what exactly is the goal, we could enable the new mode with
> > > a single option, instead of raw options to control migration stream
> > > loading/saving.
> > >
> > 
> > Saving vcpu's TSC frequency does not depend on TSC scaling but the
> > loading does. And that is why I introduce two cpu options to control
> > them separately.
> 
> I understand we probably need internal flags on the source and
> destination side to control saving/loading/setting the tsc-freq. But I
> am still trying to clarify what are the configuration semantics we need,
> exactly, to properly evaluate both the new configuration interface and
> its implementation.
>

As my above replies, we only need an internal flag 'save-tsc-freq' to
indicate whether it needs to save the TSC frequency to the migration
stream. While on the migration destination side, we set to the
migrated TSC frequency only if both TSC scaling is supported and
KVM_SET_TSC_KHZ succeeds; in other cases, we do not set TSC frequency
and not abort the migration.

Haozhong

> -- 
> Eduardo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[haozhong.zhang]

On Fri, Oct 23, 2015 at 12:45:13PM -0200, Eduardo Habkost wrote:
> On Fri, Oct 23, 2015 at 10:27:27AM +0800, Haozhong Zhang wrote:
> > On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> > > On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > > > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > > > migration. When cooperating with KVM which supports TSC scaling, guest
> > > > programs can observe a consistent guest TSC rate even though they are
> > > > migrated among machines with different host TSC rates.
> > > > 
> > > > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > > > control the migration of vcpu's TSC rate.
> > > 
> > > The requirements and goals aren't clear to me. I see two possible use
> > > cases, here:
> > > 
> > > 1) Best effort to keep TSC frequency constant if possible (but not
> > >    aborting migration if not possible). This would be an interesting
> > >    default, but a bit unpredictable.
> > > 2) Strictly ensuring TSC frequency stays constant on migration (and
> > >    aborting migration if not possible). This would be an useful feature,
> > >    but can't be enabled by default unless both hosts have the same TSC
> > >    frequency or support TSC scaling.
> > > 
> > > Which one(s) you are trying to implement?
> > >
> > 
> > The former. I agree that it's unpredictable if setting vcpu's TSC
> > frequency to the migrated value is enabled by default (but not in this
> > patchset). The cpu option 'load-tsc-freq' is introduced to allow users
> > to enable this behavior if they do know the underlying KVM and CPU
> > support TSC scaling. In this way, I think the behavior is predictable
> > as users do know what they are doing.
> 
> I'm confused. If load-tsc-freq doesn't abort when TSC scaling isn't
> available (use case #1), why isn't it enabled by default? On the other
> hand, if you expect the user to enable it only if the host supports TSC
> scaling, why doesn't it abort if TSC scaling isn't available?
>

Sorry for the confusion. For user case #1, load-tsc-freq is really not
needed and the migrated TSC frequency should be set if possible
(ie. if TSC scaling is supported and KVM_SET_TSC_KHZ succeeds). If
setting TSC frequency fails, the migration will not be aborted.

> I mean, we can implement both use cases above this way:
> 
> 1) If the user didn't ask for anything explicitly:
>   * If the tsc-freq value is available in the migration stream, try to
>     set it (but don't abort if it can't be set). (use case #1 above)
>     * Rationale: it won't hurt to try to make the VM behave nicely if
>       possible, without blocking migration if TSC scaling isn't
>       available.
> 2) If the user asked for the TSC frequency to be enforced, set it and
>   abort if it couldn't be set (use case #2 above). This could apply to
>   both cases:
>   2.1) If tsc-freq is explicitly set in the command-line.
>     * Rationale: if the user asked for a specific frequency, we
>       should do what was requested and not ignore errors silently.
>   2.2) If tsc-freq is available in the migration stream, and the
>     user asked explicitly for it to be enforced.
>     * Rationale: the user is telling us that the incoming tsc-freq
>       is important, so we shouldn't ignore it silently.
>     * Open question: how should we name the new option?
>       "load-tsc-freq" would be misleading because it won't be just about
>       _loading_ tsc-freq (we would be loading it on use case #1, too),
>       but about making sure it is enforced. "strict-tsc-freq"?
>       "enforce-tsc-freq"?
> 
> We don't need to implement both #1 and #2 at the same time. But if you
> just want to implement #1 first, I don't see the need for the
> "load-tsc-freq" option.
> 
> On the migration source, we need another option or internal machine flag
> for #1. I am not sure it should be an user-visible option. If
> user-visible, I don't know how to name it. "save-tsc-freq" describes it
> correctly, but it doesn't make its purpose very clear. Any suggestions?
> It can also be implemented first as an internal machine class flag (set
> in pc >= 2.5 only), and possibly become a user-visible option later.
>

Because the way I implements 'save-tsc-freq' in patch 1, it's exposed
to users. I'm not familiar the way to make a feature only available
for newer machine types. Could you provide some suggestions to hide
'save-tsc-freq' from users?

For the name, if we make the option internal only, could we still use
'save-tsc-freq' as it does mean saving the TSC frequency.

> > 
> > > In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> > > KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> > > the requirements and goals are not clear.
> > >
> > 
> > If KVM_CAP_TSC_CONTROL is unavailable, QEMU and KVM will use the host
> > TSC frequency as vcpu's TSC frequency.
> > 
> > If KVM_CAP_TSC_CONTROL is available and KVM_SET_TSC_KHZ fails, the
> > setting of TSC frequency will fail and abort either the VM creation
> > (this is the case for cpu option 'tsc-freq') or the migration.
> 
> I don't see why the lack of KVM_CAP_TSC_CONTROL and failure of
> KVM_SET_TSC_KHZ should be treated differently. In both cases it means we
> the TSC frequency can't be set.
> 
> I mean: if KVM_SET_TSC_KHZ is important enough for the user to make QEMU
> abort, it should abort if KVM_CAP_TSC_CONTROL isn't even available. On
> the other hand, if the user doesn't care about the lack of
> KVM_CAP_TSC_CONTROL (meaning it isn't possible to call KVM_SET_TSC_KHZ
> at all), I don't see why they would care if KVM_SET_TSC_KHZ failed.
>

Yes, we should treat them equally. If either of KVM_CAP_TSC_CONTROL or
KVM_SET_TSC_KHZ fails, we should display a warning message (but not
fail the migration).

>
> > 
> > > Once we know what exactly is the goal, we could enable the new mode with
> > > a single option, instead of raw options to control migration stream
> > > loading/saving.
> > >
> > 
> > Saving vcpu's TSC frequency does not depend on TSC scaling but the
> > loading does. And that is why I introduce two cpu options to control
> > them separately.
> 
> I understand we probably need internal flags on the source and
> destination side to control saving/loading/setting the tsc-freq. But I
> am still trying to clarify what are the configuration semantics we need,
> exactly, to properly evaluate both the new configuration interface and
> its implementation.
>

As my above replies, we only need an internal flag 'save-tsc-freq' to
indicate whether it needs to save the TSC frequency to the migration
stream. While on the migration destination side, we set to the
migrated TSC frequency only if both TSC scaling is supported and
KVM_SET_TSC_KHZ succeeds; in other cases, we do not set TSC frequency
and not abort the migration.

Haozhong

> -- 
> Eduardo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eduardo Habkost]

On Mon, Oct 26, 2015 at 10:09:13AM +0800, haozhong.zhang@intel.com wrote:
> On Fri, Oct 23, 2015 at 12:45:13PM -0200, Eduardo Habkost wrote:
> > On Fri, Oct 23, 2015 at 10:27:27AM +0800, Haozhong Zhang wrote:
> > > On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> > > > On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > > > > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > > > > migration. When cooperating with KVM which supports TSC scaling, guest
> > > > > programs can observe a consistent guest TSC rate even though they are
> > > > > migrated among machines with different host TSC rates.
> > > > > 
> > > > > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > > > > control the migration of vcpu's TSC rate.
> > > > 
> > > > The requirements and goals aren't clear to me. I see two possible use
> > > > cases, here:
> > > > 
> > > > 1) Best effort to keep TSC frequency constant if possible (but not
> > > >    aborting migration if not possible). This would be an interesting
> > > >    default, but a bit unpredictable.
> > > > 2) Strictly ensuring TSC frequency stays constant on migration (and
> > > >    aborting migration if not possible). This would be an useful feature,
> > > >    but can't be enabled by default unless both hosts have the same TSC
> > > >    frequency or support TSC scaling.
> > > > 
> > > > Which one(s) you are trying to implement?
> > > >
> > > 
> > > The former. I agree that it's unpredictable if setting vcpu's TSC
> > > frequency to the migrated value is enabled by default (but not in this
> > > patchset). The cpu option 'load-tsc-freq' is introduced to allow users
> > > to enable this behavior if they do know the underlying KVM and CPU
> > > support TSC scaling. In this way, I think the behavior is predictable
> > > as users do know what they are doing.
> > 
> > I'm confused. If load-tsc-freq doesn't abort when TSC scaling isn't
> > available (use case #1), why isn't it enabled by default? On the other
> > hand, if you expect the user to enable it only if the host supports TSC
> > scaling, why doesn't it abort if TSC scaling isn't available?
> >
> 
> Sorry for the confusion. For user case #1, load-tsc-freq is really not
> needed and the migrated TSC frequency should be set if possible
> (ie. if TSC scaling is supported and KVM_SET_TSC_KHZ succeeds). If
> setting TSC frequency fails, the migration will not be aborted.

Agreed.

> 
> > I mean, we can implement both use cases above this way:
> > 
> > 1) If the user didn't ask for anything explicitly:
> >   * If the tsc-freq value is available in the migration stream, try to
> >     set it (but don't abort if it can't be set). (use case #1 above)
> >     * Rationale: it won't hurt to try to make the VM behave nicely if
> >       possible, without blocking migration if TSC scaling isn't
> >       available.
> > 2) If the user asked for the TSC frequency to be enforced, set it and
> >   abort if it couldn't be set (use case #2 above). This could apply to
> >   both cases:
> >   2.1) If tsc-freq is explicitly set in the command-line.
> >     * Rationale: if the user asked for a specific frequency, we
> >       should do what was requested and not ignore errors silently.
> >   2.2) If tsc-freq is available in the migration stream, and the
> >     user asked explicitly for it to be enforced.
> >     * Rationale: the user is telling us that the incoming tsc-freq
> >       is important, so we shouldn't ignore it silently.
> >     * Open question: how should we name the new option?
> >       "load-tsc-freq" would be misleading because it won't be just about
> >       _loading_ tsc-freq (we would be loading it on use case #1, too),
> >       but about making sure it is enforced. "strict-tsc-freq"?
> >       "enforce-tsc-freq"?
> > 
> > We don't need to implement both #1 and #2 at the same time. But if you
> > just want to implement #1 first, I don't see the need for the
> > "load-tsc-freq" option.
> > 
> > On the migration source, we need another option or internal machine flag
> > for #1. I am not sure it should be an user-visible option. If
> > user-visible, I don't know how to name it. "save-tsc-freq" describes it
> > correctly, but it doesn't make its purpose very clear. Any suggestions?
> > It can also be implemented first as an internal machine class flag (set
> > in pc >= 2.5 only), and possibly become a user-visible option later.
> >
> 
> Because the way I implements 'save-tsc-freq' in patch 1, it's exposed
> to users. I'm not familiar the way to make a feature only available
> for newer machine types. Could you provide some suggestions to hide
> 'save-tsc-freq' from users?

You can make it an internal flag if you make it a PCMachineClass field,
set it to true on pc_machine_class_init(), and set it to false on
pc_*_2_4_machine_options().

I am unsure about the user-visible option. I am inclined towards making
it internal first because once we make it user-visible there's no
turning back.

> 
> For the name, if we make the option internal only, could we still use
> 'save-tsc-freq' as it does mean saving the TSC frequency.

save_tsc_freq sounds perfect for an internal flag, yes.

> 
> > > 
> > > > In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> > > > KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> > > > the requirements and goals are not clear.
> > > >
> > > 
> > > If KVM_CAP_TSC_CONTROL is unavailable, QEMU and KVM will use the host
> > > TSC frequency as vcpu's TSC frequency.
> > > 
> > > If KVM_CAP_TSC_CONTROL is available and KVM_SET_TSC_KHZ fails, the
> > > setting of TSC frequency will fail and abort either the VM creation
> > > (this is the case for cpu option 'tsc-freq') or the migration.
> > 
> > I don't see why the lack of KVM_CAP_TSC_CONTROL and failure of
> > KVM_SET_TSC_KHZ should be treated differently. In both cases it means we
> > the TSC frequency can't be set.
> > 
> > I mean: if KVM_SET_TSC_KHZ is important enough for the user to make QEMU
> > abort, it should abort if KVM_CAP_TSC_CONTROL isn't even available. On
> > the other hand, if the user doesn't care about the lack of
> > KVM_CAP_TSC_CONTROL (meaning it isn't possible to call KVM_SET_TSC_KHZ
> > at all), I don't see why they would care if KVM_SET_TSC_KHZ failed.
> >
> 
> Yes, we should treat them equally. If either of KVM_CAP_TSC_CONTROL or
> KVM_SET_TSC_KHZ fails, we should display a warning message (but not
> fail the migration).

Agreed. I mean, except when tsc-freq is explicitly set in the
command-line, in which case I believe we should abort (but we can do
that later, because that would be a change from the current behavior).

> 
> >
> > > 
> > > > Once we know what exactly is the goal, we could enable the new mode with
> > > > a single option, instead of raw options to control migration stream
> > > > loading/saving.
> > > >
> > > 
> > > Saving vcpu's TSC frequency does not depend on TSC scaling but the
> > > loading does. And that is why I introduce two cpu options to control
> > > them separately.
> > 
> > I understand we probably need internal flags on the source and
> > destination side to control saving/loading/setting the tsc-freq. But I
> > am still trying to clarify what are the configuration semantics we need,
> > exactly, to properly evaluate both the new configuration interface and
> > its implementation.
> >
> 
> As my above replies, we only need an internal flag 'save-tsc-freq' to
> indicate whether it needs to save the TSC frequency to the migration
> stream. While on the migration destination side, we set to the
> migrated TSC frequency only if both TSC scaling is supported and
> KVM_SET_TSC_KHZ succeeds; in other cases, we do not set TSC frequency
> and not abort the migration.

Agreed.

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Oct 26, 2015 at 10:09:13AM +0800, haozhong.zhang@intel.com wrote:
> On Fri, Oct 23, 2015 at 12:45:13PM -0200, Eduardo Habkost wrote:
> > On Fri, Oct 23, 2015 at 10:27:27AM +0800, Haozhong Zhang wrote:
> > > On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> > > > On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > > > > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > > > > migration. When cooperating with KVM which supports TSC scaling, guest
> > > > > programs can observe a consistent guest TSC rate even though they are
> > > > > migrated among machines with different host TSC rates.
> > > > > 
> > > > > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > > > > control the migration of vcpu's TSC rate.
> > > > 
> > > > The requirements and goals aren't clear to me. I see two possible use
> > > > cases, here:
> > > > 
> > > > 1) Best effort to keep TSC frequency constant if possible (but not
> > > >    aborting migration if not possible). This would be an interesting
> > > >    default, but a bit unpredictable.
> > > > 2) Strictly ensuring TSC frequency stays constant on migration (and
> > > >    aborting migration if not possible). This would be an useful feature,
> > > >    but can't be enabled by default unless both hosts have the same TSC
> > > >    frequency or support TSC scaling.
> > > > 
> > > > Which one(s) you are trying to implement?
> > > >
> > > 
> > > The former. I agree that it's unpredictable if setting vcpu's TSC
> > > frequency to the migrated value is enabled by default (but not in this
> > > patchset). The cpu option 'load-tsc-freq' is introduced to allow users
> > > to enable this behavior if they do know the underlying KVM and CPU
> > > support TSC scaling. In this way, I think the behavior is predictable
> > > as users do know what they are doing.
> > 
> > I'm confused. If load-tsc-freq doesn't abort when TSC scaling isn't
> > available (use case #1), why isn't it enabled by default? On the other
> > hand, if you expect the user to enable it only if the host supports TSC
> > scaling, why doesn't it abort if TSC scaling isn't available?
> >
> 
> Sorry for the confusion. For user case #1, load-tsc-freq is really not
> needed and the migrated TSC frequency should be set if possible
> (ie. if TSC scaling is supported and KVM_SET_TSC_KHZ succeeds). If
> setting TSC frequency fails, the migration will not be aborted.

Agreed.

> 
> > I mean, we can implement both use cases above this way:
> > 
> > 1) If the user didn't ask for anything explicitly:
> >   * If the tsc-freq value is available in the migration stream, try to
> >     set it (but don't abort if it can't be set). (use case #1 above)
> >     * Rationale: it won't hurt to try to make the VM behave nicely if
> >       possible, without blocking migration if TSC scaling isn't
> >       available.
> > 2) If the user asked for the TSC frequency to be enforced, set it and
> >   abort if it couldn't be set (use case #2 above). This could apply to
> >   both cases:
> >   2.1) If tsc-freq is explicitly set in the command-line.
> >     * Rationale: if the user asked for a specific frequency, we
> >       should do what was requested and not ignore errors silently.
> >   2.2) If tsc-freq is available in the migration stream, and the
> >     user asked explicitly for it to be enforced.
> >     * Rationale: the user is telling us that the incoming tsc-freq
> >       is important, so we shouldn't ignore it silently.
> >     * Open question: how should we name the new option?
> >       "load-tsc-freq" would be misleading because it won't be just about
> >       _loading_ tsc-freq (we would be loading it on use case #1, too),
> >       but about making sure it is enforced. "strict-tsc-freq"?
> >       "enforce-tsc-freq"?
> > 
> > We don't need to implement both #1 and #2 at the same time. But if you
> > just want to implement #1 first, I don't see the need for the
> > "load-tsc-freq" option.
> > 
> > On the migration source, we need another option or internal machine flag
> > for #1. I am not sure it should be an user-visible option. If
> > user-visible, I don't know how to name it. "save-tsc-freq" describes it
> > correctly, but it doesn't make its purpose very clear. Any suggestions?
> > It can also be implemented first as an internal machine class flag (set
> > in pc >= 2.5 only), and possibly become a user-visible option later.
> >
> 
> Because the way I implements 'save-tsc-freq' in patch 1, it's exposed
> to users. I'm not familiar the way to make a feature only available
> for newer machine types. Could you provide some suggestions to hide
> 'save-tsc-freq' from users?

You can make it an internal flag if you make it a PCMachineClass field,
set it to true on pc_machine_class_init(), and set it to false on
pc_*_2_4_machine_options().

I am unsure about the user-visible option. I am inclined towards making
it internal first because once we make it user-visible there's no
turning back.

> 
> For the name, if we make the option internal only, could we still use
> 'save-tsc-freq' as it does mean saving the TSC frequency.

save_tsc_freq sounds perfect for an internal flag, yes.

> 
> > > 
> > > > In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> > > > KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> > > > the requirements and goals are not clear.
> > > >
> > > 
> > > If KVM_CAP_TSC_CONTROL is unavailable, QEMU and KVM will use the host
> > > TSC frequency as vcpu's TSC frequency.
> > > 
> > > If KVM_CAP_TSC_CONTROL is available and KVM_SET_TSC_KHZ fails, the
> > > setting of TSC frequency will fail and abort either the VM creation
> > > (this is the case for cpu option 'tsc-freq') or the migration.
> > 
> > I don't see why the lack of KVM_CAP_TSC_CONTROL and failure of
> > KVM_SET_TSC_KHZ should be treated differently. In both cases it means we
> > the TSC frequency can't be set.
> > 
> > I mean: if KVM_SET_TSC_KHZ is important enough for the user to make QEMU
> > abort, it should abort if KVM_CAP_TSC_CONTROL isn't even available. On
> > the other hand, if the user doesn't care about the lack of
> > KVM_CAP_TSC_CONTROL (meaning it isn't possible to call KVM_SET_TSC_KHZ
> > at all), I don't see why they would care if KVM_SET_TSC_KHZ failed.
> >
> 
> Yes, we should treat them equally. If either of KVM_CAP_TSC_CONTROL or
> KVM_SET_TSC_KHZ fails, we should display a warning message (but not
> fail the migration).

Agreed. I mean, except when tsc-freq is explicitly set in the
command-line, in which case I believe we should abort (but we can do
that later, because that would be a change from the current behavior).

> 
> >
> > > 
> > > > Once we know what exactly is the goal, we could enable the new mode with
> > > > a single option, instead of raw options to control migration stream
> > > > loading/saving.
> > > >
> > > 
> > > Saving vcpu's TSC frequency does not depend on TSC scaling but the
> > > loading does. And that is why I introduce two cpu options to control
> > > them separately.
> > 
> > I understand we probably need internal flags on the source and
> > destination side to control saving/loading/setting the tsc-freq. But I
> > am still trying to clarify what are the configuration semantics we need,
> > exactly, to properly evaluate both the new configuration interface and
> > its implementation.
> >
> 
> As my above replies, we only need an internal flag 'save-tsc-freq' to
> indicate whether it needs to save the TSC frequency to the migration
> stream. While on the migration destination side, we set to the
> migrated TSC frequency only if both TSC scaling is supported and
> KVM_SET_TSC_KHZ succeeds; in other cases, we do not set TSC frequency
> and not abort the migration.

Agreed.

-- 
Eduardo

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

[haozhong.zhang]

On Mon, Oct 26, 2015 at 04:41:22PM -0200, Eduardo Habkost wrote:
> On Mon, Oct 26, 2015 at 10:09:13AM +0800, haozhong.zhang@intel.com wrote:
> > On Fri, Oct 23, 2015 at 12:45:13PM -0200, Eduardo Habkost wrote:
> > > On Fri, Oct 23, 2015 at 10:27:27AM +0800, Haozhong Zhang wrote:
> > > > On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> > > > > On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > > > > > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > > > > > migration. When cooperating with KVM which supports TSC scaling, guest
> > > > > > programs can observe a consistent guest TSC rate even though they are
> > > > > > migrated among machines with different host TSC rates.
> > > > > > 
> > > > > > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > > > > > control the migration of vcpu's TSC rate.
> > > > > 
> > > > > The requirements and goals aren't clear to me. I see two possible use
> > > > > cases, here:
> > > > > 
> > > > > 1) Best effort to keep TSC frequency constant if possible (but not
> > > > >    aborting migration if not possible). This would be an interesting
> > > > >    default, but a bit unpredictable.
> > > > > 2) Strictly ensuring TSC frequency stays constant on migration (and
> > > > >    aborting migration if not possible). This would be an useful feature,
> > > > >    but can't be enabled by default unless both hosts have the same TSC
> > > > >    frequency or support TSC scaling.
> > > > > 
> > > > > Which one(s) you are trying to implement?
> > > > >
> > > > 
> > > > The former. I agree that it's unpredictable if setting vcpu's TSC
> > > > frequency to the migrated value is enabled by default (but not in this
> > > > patchset). The cpu option 'load-tsc-freq' is introduced to allow users
> > > > to enable this behavior if they do know the underlying KVM and CPU
> > > > support TSC scaling. In this way, I think the behavior is predictable
> > > > as users do know what they are doing.
> > > 
> > > I'm confused. If load-tsc-freq doesn't abort when TSC scaling isn't
> > > available (use case #1), why isn't it enabled by default? On the other
> > > hand, if you expect the user to enable it only if the host supports TSC
> > > scaling, why doesn't it abort if TSC scaling isn't available?
> > >
> > 
> > Sorry for the confusion. For user case #1, load-tsc-freq is really not
> > needed and the migrated TSC frequency should be set if possible
> > (ie. if TSC scaling is supported and KVM_SET_TSC_KHZ succeeds). If
> > setting TSC frequency fails, the migration will not be aborted.
> 
> Agreed.
> 
> > 
> > > I mean, we can implement both use cases above this way:
> > > 
> > > 1) If the user didn't ask for anything explicitly:
> > >   * If the tsc-freq value is available in the migration stream, try to
> > >     set it (but don't abort if it can't be set). (use case #1 above)
> > >     * Rationale: it won't hurt to try to make the VM behave nicely if
> > >       possible, without blocking migration if TSC scaling isn't
> > >       available.
> > > 2) If the user asked for the TSC frequency to be enforced, set it and
> > >   abort if it couldn't be set (use case #2 above). This could apply to
> > >   both cases:
> > >   2.1) If tsc-freq is explicitly set in the command-line.
> > >     * Rationale: if the user asked for a specific frequency, we
> > >       should do what was requested and not ignore errors silently.
> > >   2.2) If tsc-freq is available in the migration stream, and the
> > >     user asked explicitly for it to be enforced.
> > >     * Rationale: the user is telling us that the incoming tsc-freq
> > >       is important, so we shouldn't ignore it silently.
> > >     * Open question: how should we name the new option?
> > >       "load-tsc-freq" would be misleading because it won't be just about
> > >       _loading_ tsc-freq (we would be loading it on use case #1, too),
> > >       but about making sure it is enforced. "strict-tsc-freq"?
> > >       "enforce-tsc-freq"?
> > > 
> > > We don't need to implement both #1 and #2 at the same time. But if you
> > > just want to implement #1 first, I don't see the need for the
> > > "load-tsc-freq" option.
> > > 
> > > On the migration source, we need another option or internal machine flag
> > > for #1. I am not sure it should be an user-visible option. If
> > > user-visible, I don't know how to name it. "save-tsc-freq" describes it
> > > correctly, but it doesn't make its purpose very clear. Any suggestions?
> > > It can also be implemented first as an internal machine class flag (set
> > > in pc >= 2.5 only), and possibly become a user-visible option later.
> > >
> > 
> > Because the way I implements 'save-tsc-freq' in patch 1, it's exposed
> > to users. I'm not familiar the way to make a feature only available
> > for newer machine types. Could you provide some suggestions to hide
> > 'save-tsc-freq' from users?
> 
> You can make it an internal flag if you make it a PCMachineClass field,
> set it to true on pc_machine_class_init(), and set it to false on
> pc_*_2_4_machine_options().
>

Thank you!

> I am unsure about the user-visible option. I am inclined towards making
> it internal first because once we make it user-visible there's no
> turning back.
>

I'll make it just an internal flag, so we don't need to worry about
the user-visible problem right now.

> > 
> > For the name, if we make the option internal only, could we still use
> > 'save-tsc-freq' as it does mean saving the TSC frequency.
> 
> save_tsc_freq sounds perfect for an internal flag, yes.
> 
> > 
> > > > 
> > > > > In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> > > > > KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> > > > > the requirements and goals are not clear.
> > > > >
> > > > 
> > > > If KVM_CAP_TSC_CONTROL is unavailable, QEMU and KVM will use the host
> > > > TSC frequency as vcpu's TSC frequency.
> > > > 
> > > > If KVM_CAP_TSC_CONTROL is available and KVM_SET_TSC_KHZ fails, the
> > > > setting of TSC frequency will fail and abort either the VM creation
> > > > (this is the case for cpu option 'tsc-freq') or the migration.
> > > 
> > > I don't see why the lack of KVM_CAP_TSC_CONTROL and failure of
> > > KVM_SET_TSC_KHZ should be treated differently. In both cases it means we
> > > the TSC frequency can't be set.
> > > 
> > > I mean: if KVM_SET_TSC_KHZ is important enough for the user to make QEMU
> > > abort, it should abort if KVM_CAP_TSC_CONTROL isn't even available. On
> > > the other hand, if the user doesn't care about the lack of
> > > KVM_CAP_TSC_CONTROL (meaning it isn't possible to call KVM_SET_TSC_KHZ
> > > at all), I don't see why they would care if KVM_SET_TSC_KHZ failed.
> > >
> > 
> > Yes, we should treat them equally. If either of KVM_CAP_TSC_CONTROL or
> > KVM_SET_TSC_KHZ fails, we should display a warning message (but not
> > fail the migration).
> 
> Agreed. I mean, except when tsc-freq is explicitly set in the
> command-line, in which case I believe we should abort (but we can do
> that later, because that would be a change from the current behavior).
>

I think we can make the change later. For now, we just display the
warning message.

Haozhong

> > 
> > >
> > > > 
> > > > > Once we know what exactly is the goal, we could enable the new mode with
> > > > > a single option, instead of raw options to control migration stream
> > > > > loading/saving.
> > > > >
> > > > 
> > > > Saving vcpu's TSC frequency does not depend on TSC scaling but the
> > > > loading does. And that is why I introduce two cpu options to control
> > > > them separately.
> > > 
> > > I understand we probably need internal flags on the source and
> > > destination side to control saving/loading/setting the tsc-freq. But I
> > > am still trying to clarify what are the configuration semantics we need,
> > > exactly, to properly evaluate both the new configuration interface and
> > > its implementation.
> > >
> > 
> > As my above replies, we only need an internal flag 'save-tsc-freq' to
> > indicate whether it needs to save the TSC frequency to the migration
> > stream. While on the migration destination side, we set to the
> > migrated TSC frequency only if both TSC scaling is supported and
> > KVM_SET_TSC_KHZ succeeds; in other cases, we do not set TSC frequency
> > and not abort the migration.
> 
> Agreed.
> 
> -- 
> Eduardo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[haozhong.zhang]

On Mon, Oct 26, 2015 at 04:41:22PM -0200, Eduardo Habkost wrote:
> On Mon, Oct 26, 2015 at 10:09:13AM +0800, haozhong.zhang@intel.com wrote:
> > On Fri, Oct 23, 2015 at 12:45:13PM -0200, Eduardo Habkost wrote:
> > > On Fri, Oct 23, 2015 at 10:27:27AM +0800, Haozhong Zhang wrote:
> > > > On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> > > > > On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > > > > > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > > > > > migration. When cooperating with KVM which supports TSC scaling, guest
> > > > > > programs can observe a consistent guest TSC rate even though they are
> > > > > > migrated among machines with different host TSC rates.
> > > > > > 
> > > > > > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > > > > > control the migration of vcpu's TSC rate.
> > > > > 
> > > > > The requirements and goals aren't clear to me. I see two possible use
> > > > > cases, here:
> > > > > 
> > > > > 1) Best effort to keep TSC frequency constant if possible (but not
> > > > >    aborting migration if not possible). This would be an interesting
> > > > >    default, but a bit unpredictable.
> > > > > 2) Strictly ensuring TSC frequency stays constant on migration (and
> > > > >    aborting migration if not possible). This would be an useful feature,
> > > > >    but can't be enabled by default unless both hosts have the same TSC
> > > > >    frequency or support TSC scaling.
> > > > > 
> > > > > Which one(s) you are trying to implement?
> > > > >
> > > > 
> > > > The former. I agree that it's unpredictable if setting vcpu's TSC
> > > > frequency to the migrated value is enabled by default (but not in this
> > > > patchset). The cpu option 'load-tsc-freq' is introduced to allow users
> > > > to enable this behavior if they do know the underlying KVM and CPU
> > > > support TSC scaling. In this way, I think the behavior is predictable
> > > > as users do know what they are doing.
> > > 
> > > I'm confused. If load-tsc-freq doesn't abort when TSC scaling isn't
> > > available (use case #1), why isn't it enabled by default? On the other
> > > hand, if you expect the user to enable it only if the host supports TSC
> > > scaling, why doesn't it abort if TSC scaling isn't available?
> > >
> > 
> > Sorry for the confusion. For user case #1, load-tsc-freq is really not
> > needed and the migrated TSC frequency should be set if possible
> > (ie. if TSC scaling is supported and KVM_SET_TSC_KHZ succeeds). If
> > setting TSC frequency fails, the migration will not be aborted.
> 
> Agreed.
> 
> > 
> > > I mean, we can implement both use cases above this way:
> > > 
> > > 1) If the user didn't ask for anything explicitly:
> > >   * If the tsc-freq value is available in the migration stream, try to
> > >     set it (but don't abort if it can't be set). (use case #1 above)
> > >     * Rationale: it won't hurt to try to make the VM behave nicely if
> > >       possible, without blocking migration if TSC scaling isn't
> > >       available.
> > > 2) If the user asked for the TSC frequency to be enforced, set it and
> > >   abort if it couldn't be set (use case #2 above). This could apply to
> > >   both cases:
> > >   2.1) If tsc-freq is explicitly set in the command-line.
> > >     * Rationale: if the user asked for a specific frequency, we
> > >       should do what was requested and not ignore errors silently.
> > >   2.2) If tsc-freq is available in the migration stream, and the
> > >     user asked explicitly for it to be enforced.
> > >     * Rationale: the user is telling us that the incoming tsc-freq
> > >       is important, so we shouldn't ignore it silently.
> > >     * Open question: how should we name the new option?
> > >       "load-tsc-freq" would be misleading because it won't be just about
> > >       _loading_ tsc-freq (we would be loading it on use case #1, too),
> > >       but about making sure it is enforced. "strict-tsc-freq"?
> > >       "enforce-tsc-freq"?
> > > 
> > > We don't need to implement both #1 and #2 at the same time. But if you
> > > just want to implement #1 first, I don't see the need for the
> > > "load-tsc-freq" option.
> > > 
> > > On the migration source, we need another option or internal machine flag
> > > for #1. I am not sure it should be an user-visible option. If
> > > user-visible, I don't know how to name it. "save-tsc-freq" describes it
> > > correctly, but it doesn't make its purpose very clear. Any suggestions?
> > > It can also be implemented first as an internal machine class flag (set
> > > in pc >= 2.5 only), and possibly become a user-visible option later.
> > >
> > 
> > Because the way I implements 'save-tsc-freq' in patch 1, it's exposed
> > to users. I'm not familiar the way to make a feature only available
> > for newer machine types. Could you provide some suggestions to hide
> > 'save-tsc-freq' from users?
> 
> You can make it an internal flag if you make it a PCMachineClass field,
> set it to true on pc_machine_class_init(), and set it to false on
> pc_*_2_4_machine_options().
>

Thank you!

> I am unsure about the user-visible option. I am inclined towards making
> it internal first because once we make it user-visible there's no
> turning back.
>

I'll make it just an internal flag, so we don't need to worry about
the user-visible problem right now.

> > 
> > For the name, if we make the option internal only, could we still use
> > 'save-tsc-freq' as it does mean saving the TSC frequency.
> 
> save_tsc_freq sounds perfect for an internal flag, yes.
> 
> > 
> > > > 
> > > > > In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> > > > > KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> > > > > the requirements and goals are not clear.
> > > > >
> > > > 
> > > > If KVM_CAP_TSC_CONTROL is unavailable, QEMU and KVM will use the host
> > > > TSC frequency as vcpu's TSC frequency.
> > > > 
> > > > If KVM_CAP_TSC_CONTROL is available and KVM_SET_TSC_KHZ fails, the
> > > > setting of TSC frequency will fail and abort either the VM creation
> > > > (this is the case for cpu option 'tsc-freq') or the migration.
> > > 
> > > I don't see why the lack of KVM_CAP_TSC_CONTROL and failure of
> > > KVM_SET_TSC_KHZ should be treated differently. In both cases it means we
> > > the TSC frequency can't be set.
> > > 
> > > I mean: if KVM_SET_TSC_KHZ is important enough for the user to make QEMU
> > > abort, it should abort if KVM_CAP_TSC_CONTROL isn't even available. On
> > > the other hand, if the user doesn't care about the lack of
> > > KVM_CAP_TSC_CONTROL (meaning it isn't possible to call KVM_SET_TSC_KHZ
> > > at all), I don't see why they would care if KVM_SET_TSC_KHZ failed.
> > >
> > 
> > Yes, we should treat them equally. If either of KVM_CAP_TSC_CONTROL or
> > KVM_SET_TSC_KHZ fails, we should display a warning message (but not
> > fail the migration).
> 
> Agreed. I mean, except when tsc-freq is explicitly set in the
> command-line, in which case I believe we should abort (but we can do
> that later, because that would be a change from the current behavior).
>

I think we can make the change later. For now, we just display the
warning message.

Haozhong

> > 
> > >
> > > > 
> > > > > Once we know what exactly is the goal, we could enable the new mode with
> > > > > a single option, instead of raw options to control migration stream
> > > > > loading/saving.
> > > > >
> > > > 
> > > > Saving vcpu's TSC frequency does not depend on TSC scaling but the
> > > > loading does. And that is why I introduce two cpu options to control
> > > > them separately.
> > > 
> > > I understand we probably need internal flags on the source and
> > > destination side to control saving/loading/setting the tsc-freq. But I
> > > am still trying to clarify what are the configuration semantics we need,
> > > exactly, to properly evaluate both the new configuration interface and
> > > its implementation.
> > >
> > 
> > As my above replies, we only need an internal flag 'save-tsc-freq' to
> > indicate whether it needs to save the TSC frequency to the migration
> > stream. While on the migration destination side, we set to the
> > migrated TSC frequency only if both TSC scaling is supported and
> > KVM_SET_TSC_KHZ succeeds; in other cases, we do not set TSC frequency
> > and not abort the migration.
> 
> Agreed.
> 
> -- 
> Eduardo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html