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

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

[Haozhong Zhang]

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

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

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

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

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

Haozhong Zhang (3):
  target-i386: 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

 hw/i386/pc.c          |  1 +
 hw/i386/pc_piix.c     |  1 +
 hw/i386/pc_q35.c      |  1 +
 include/hw/i386/pc.h  |  1 +
 include/sysemu/kvm.h  |  2 ++
 kvm-all.c             |  1 +
 target-arm/kvm.c      |  5 +++++
 target-i386/cpu.h     |  1 +
 target-i386/kvm.c     | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
 target-i386/machine.c | 21 ++++++++++++++++++++
 target-mips/kvm.c     |  5 +++++
 target-ppc/kvm.c      |  5 +++++
 target-s390x/kvm.c    |  5 +++++
 13 files changed, 103 insertions(+)

-- 
2.4.8

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

[Haozhong Zhang]

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

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

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

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

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

Haozhong Zhang (3):
  target-i386: 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

 hw/i386/pc.c          |  1 +
 hw/i386/pc_piix.c     |  1 +
 hw/i386/pc_q35.c      |  1 +
 include/hw/i386/pc.h  |  1 +
 include/sysemu/kvm.h  |  2 ++
 kvm-all.c             |  1 +
 target-arm/kvm.c      |  5 +++++
 target-i386/cpu.h     |  1 +
 target-i386/kvm.c     | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
 target-i386/machine.c | 21 ++++++++++++++++++++
 target-mips/kvm.c     |  5 +++++
 target-ppc/kvm.c      |  5 +++++
 target-s390x/kvm.c    |  5 +++++
 13 files changed, 103 insertions(+)

-- 
2.4.8

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

[Haozhong Zhang]

A new subsection 'vmstate_tsc_khz' is added to migrate vcpu's TSC
rate. For the backwards compatibility, this subsection is not migrated
on pc-*-2.4 and older machine types.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 hw/i386/pc.c          |  1 +
 hw/i386/pc_piix.c     |  1 +
 hw/i386/pc_q35.c      |  1 +
 include/hw/i386/pc.h  |  1 +
 target-i386/cpu.h     |  1 +
 target-i386/machine.c | 21 +++++++++++++++++++++
 6 files changed, 26 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0cb8afd..2f2fc93 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1952,6 +1952,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     pcmc->get_hotplug_handler = mc->get_hotplug_handler;
+    pcmc->save_tsc_khz = true;
     mc->get_hotplug_handler = pc_get_hotpug_handler;
     mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
     mc->default_boot_order = "cad";
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 393dcc4..fc71321 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -487,6 +487,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
     m->alias = NULL;
     m->is_default = 0;
     pcmc->broken_reserved_end = true;
+    pcmc->save_tsc_khz = false;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 2f8f396..858ed69 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
     pc_q35_2_5_machine_options(m);
     m->alias = NULL;
     pcmc->broken_reserved_end = true;
+    pcmc->save_tsc_khz = false;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 606dbc2..875d099 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -60,6 +60,7 @@ struct PCMachineClass {
 
     /*< public >*/
     bool broken_reserved_end;
+    bool save_tsc_khz;
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
 };
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 62f7879..4f2f4a3 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -970,6 +970,7 @@ typedef struct CPUX86State {
     uint32_t sipi_vector;
     bool tsc_valid;
     int64_t tsc_khz;
+    int64_t tsc_khz_saved;
     void *kvm_xsave_buf;
 
     uint64_t mcg_cap;
diff --git a/target-i386/machine.c b/target-i386/machine.c
index a18e16e..4d8157c 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -775,6 +775,26 @@ static const VMStateDescription vmstate_xss = {
     }
 };
 
+static bool tsc_khz_needed(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    CPUX86State *env = &cpu->env;
+    MachineClass *mc = MACHINE_GET_CLASS((qdev_get_machine()));
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(mc);
+    return env->tsc_khz_saved && pcmc->save_tsc_khz;
+}
+
+static const VMStateDescription vmstate_tsc_khz = {
+    .name = "cpu/tsc_khz",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = tsc_khz_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT64(env.tsc_khz_saved, X86CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 VMStateDescription vmstate_x86_cpu = {
     .name = "cpu",
     .version_id = 12,
@@ -895,6 +915,7 @@ VMStateDescription vmstate_x86_cpu = {
         &vmstate_msr_hyperv_runtime,
         &vmstate_avx512,
         &vmstate_xss,
+        &vmstate_tsc_khz,
         NULL
     }
 };
-- 
2.4.8

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

[Haozhong Zhang]

A new subsection 'vmstate_tsc_khz' is added to migrate vcpu's TSC
rate. For the backwards compatibility, this subsection is not migrated
on pc-*-2.4 and older machine types.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 hw/i386/pc.c          |  1 +
 hw/i386/pc_piix.c     |  1 +
 hw/i386/pc_q35.c      |  1 +
 include/hw/i386/pc.h  |  1 +
 target-i386/cpu.h     |  1 +
 target-i386/machine.c | 21 +++++++++++++++++++++
 6 files changed, 26 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0cb8afd..2f2fc93 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1952,6 +1952,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     pcmc->get_hotplug_handler = mc->get_hotplug_handler;
+    pcmc->save_tsc_khz = true;
     mc->get_hotplug_handler = pc_get_hotpug_handler;
     mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
     mc->default_boot_order = "cad";
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 393dcc4..fc71321 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -487,6 +487,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
     m->alias = NULL;
     m->is_default = 0;
     pcmc->broken_reserved_end = true;
+    pcmc->save_tsc_khz = false;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 2f8f396..858ed69 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
     pc_q35_2_5_machine_options(m);
     m->alias = NULL;
     pcmc->broken_reserved_end = true;
+    pcmc->save_tsc_khz = false;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 606dbc2..875d099 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -60,6 +60,7 @@ struct PCMachineClass {
 
     /*< public >*/
     bool broken_reserved_end;
+    bool save_tsc_khz;
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
 };
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 62f7879..4f2f4a3 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -970,6 +970,7 @@ typedef struct CPUX86State {
     uint32_t sipi_vector;
     bool tsc_valid;
     int64_t tsc_khz;
+    int64_t tsc_khz_saved;
     void *kvm_xsave_buf;
 
     uint64_t mcg_cap;
diff --git a/target-i386/machine.c b/target-i386/machine.c
index a18e16e..4d8157c 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -775,6 +775,26 @@ static const VMStateDescription vmstate_xss = {
     }
 };
 
+static bool tsc_khz_needed(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    CPUX86State *env = &cpu->env;
+    MachineClass *mc = MACHINE_GET_CLASS((qdev_get_machine()));
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(mc);
+    return env->tsc_khz_saved && pcmc->save_tsc_khz;
+}
+
+static const VMStateDescription vmstate_tsc_khz = {
+    .name = "cpu/tsc_khz",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = tsc_khz_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT64(env.tsc_khz_saved, X86CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 VMStateDescription vmstate_x86_cpu = {
     .name = "cpu",
     .version_id = 12,
@@ -895,6 +915,7 @@ VMStateDescription vmstate_x86_cpu = {
         &vmstate_msr_hyperv_runtime,
         &vmstate_avx512,
         &vmstate_xss,
+        &vmstate_tsc_khz,
         NULL
     }
 };
-- 
2.4.8

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

[Haozhong Zhang]

The value of the migrated vcpu's TSC rate is determined as below.
 1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
    user-specified value will be used.
 2. If neither a user-specified TSC rate nor a migrated TSC rate is
    present, we will use the TSC rate from KVM (returned by
    KVM_GET_TSC_KHZ).
 3. Otherwise, we will use the migrated TSC rate.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 include/sysemu/kvm.h |  2 ++
 kvm-all.c            |  1 +
 target-arm/kvm.c     |  5 +++++
 target-i386/kvm.c    | 33 +++++++++++++++++++++++++++++++++
 target-mips/kvm.c    |  5 +++++
 target-ppc/kvm.c     |  5 +++++
 target-s390x/kvm.c   |  5 +++++
 7 files changed, 56 insertions(+)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 461ef65..0ec8b98 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -328,6 +328,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
 
 int kvm_arch_msi_data_to_gsi(uint32_t data);
 
+int kvm_arch_setup_tsc_khz(CPUState *cpu);
+
 int kvm_set_irq(KVMState *s, int irq, int level);
 int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
 
diff --git a/kvm-all.c b/kvm-all.c
index c442838..1ecaf04 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1757,6 +1757,7 @@ static void do_kvm_cpu_synchronize_post_init(void *arg)
 {
     CPUState *cpu = arg;
 
+    kvm_arch_setup_tsc_khz(cpu);
     kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
     cpu->kvm_vcpu_dirty = false;
 }
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 79ef4c6..a724f6d 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -614,3 +614,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
 {
     return (data - 32) & 0xffff;
 }
+
+int kvm_arch_setup_tsc_khz(CPUState *cs)
+{
+    return 0;
+}
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 64046cb..aae5e58 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
 {
     abort();
 }
+
+int kvm_arch_setup_tsc_khz(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+    CPUX86State *env = &cpu->env;
+    int r;
+
+    /*
+     * Prepare vcpu's TSC rate to be migrated.
+     *
+     * - If the user specifies the TSC rate by cpu option 'tsc-freq',
+     *   we will use the user-specified value.
+     *
+     * - If there is neither user-specified TSC rate nor migrated TSC
+     *   rate, we will ask KVM for the TSC rate by calling
+     *   KVM_GET_TSC_KHZ.
+     *
+     * - Otherwise, if there is a migrated TSC rate, we will use the
+     *   migrated value.
+     */
+    if (env->tsc_khz) {
+        env->tsc_khz_saved = env->tsc_khz;
+    } else if (!env->tsc_khz_saved) {
+        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_saved = r;
+    }
+
+    return 0;
+}
diff --git a/target-mips/kvm.c b/target-mips/kvm.c
index 12d7db3..fb26d7e 100644
--- a/target-mips/kvm.c
+++ b/target-mips/kvm.c
@@ -687,3 +687,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
 {
     abort();
 }
+
+int kvm_arch_setup_tsc_khz(CPUState *cs)
+{
+    return 0;
+}
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index ac70f08..c429f0c 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2510,3 +2510,8 @@ int kvmppc_enable_hwrng(void)
 
     return kvmppc_enable_hcall(kvm_state, H_RANDOM);
 }
+
+int kvm_arch_setup_tsc_khz(CPUState *cs)
+{
+    return 0;
+}
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index c3be180..db5d436 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -2248,3 +2248,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
 {
     abort();
 }
+
+int kvm_arch_setup_tsc_khz(CPUState *cs)
+{
+    return 0;
+}
-- 
2.4.8

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

[Haozhong Zhang]

The value of the migrated vcpu's TSC rate is determined as below.
 1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
    user-specified value will be used.
 2. If neither a user-specified TSC rate nor a migrated TSC rate is
    present, we will use the TSC rate from KVM (returned by
    KVM_GET_TSC_KHZ).
 3. Otherwise, we will use the migrated TSC rate.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 include/sysemu/kvm.h |  2 ++
 kvm-all.c            |  1 +
 target-arm/kvm.c     |  5 +++++
 target-i386/kvm.c    | 33 +++++++++++++++++++++++++++++++++
 target-mips/kvm.c    |  5 +++++
 target-ppc/kvm.c     |  5 +++++
 target-s390x/kvm.c   |  5 +++++
 7 files changed, 56 insertions(+)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 461ef65..0ec8b98 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -328,6 +328,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
 
 int kvm_arch_msi_data_to_gsi(uint32_t data);
 
+int kvm_arch_setup_tsc_khz(CPUState *cpu);
+
 int kvm_set_irq(KVMState *s, int irq, int level);
 int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
 
diff --git a/kvm-all.c b/kvm-all.c
index c442838..1ecaf04 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1757,6 +1757,7 @@ static void do_kvm_cpu_synchronize_post_init(void *arg)
 {
     CPUState *cpu = arg;
 
+    kvm_arch_setup_tsc_khz(cpu);
     kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
     cpu->kvm_vcpu_dirty = false;
 }
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 79ef4c6..a724f6d 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -614,3 +614,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
 {
     return (data - 32) & 0xffff;
 }
+
+int kvm_arch_setup_tsc_khz(CPUState *cs)
+{
+    return 0;
+}
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 64046cb..aae5e58 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
 {
     abort();
 }
+
+int kvm_arch_setup_tsc_khz(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+    CPUX86State *env = &cpu->env;
+    int r;
+
+    /*
+     * Prepare vcpu's TSC rate to be migrated.
+     *
+     * - If the user specifies the TSC rate by cpu option 'tsc-freq',
+     *   we will use the user-specified value.
+     *
+     * - If there is neither user-specified TSC rate nor migrated TSC
+     *   rate, we will ask KVM for the TSC rate by calling
+     *   KVM_GET_TSC_KHZ.
+     *
+     * - Otherwise, if there is a migrated TSC rate, we will use the
+     *   migrated value.
+     */
+    if (env->tsc_khz) {
+        env->tsc_khz_saved = env->tsc_khz;
+    } else if (!env->tsc_khz_saved) {
+        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_saved = r;
+    }
+
+    return 0;
+}
diff --git a/target-mips/kvm.c b/target-mips/kvm.c
index 12d7db3..fb26d7e 100644
--- a/target-mips/kvm.c
+++ b/target-mips/kvm.c
@@ -687,3 +687,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
 {
     abort();
 }
+
+int kvm_arch_setup_tsc_khz(CPUState *cs)
+{
+    return 0;
+}
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index ac70f08..c429f0c 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2510,3 +2510,8 @@ int kvmppc_enable_hwrng(void)
 
     return kvmppc_enable_hcall(kvm_state, H_RANDOM);
 }
+
+int kvm_arch_setup_tsc_khz(CPUState *cs)
+{
+    return 0;
+}
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index c3be180..db5d436 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -2248,3 +2248,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
 {
     abort();
 }
+
+int kvm_arch_setup_tsc_khz(CPUState *cs)
+{
+    return 0;
+}
-- 
2.4.8

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

[Haozhong Zhang]

Set vcpu's TSC rate to the migrated value if the user does not specify a
TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If
KVM supports TSC scaling, guest programs will observe TSC increasing in
the migrated rate other than the host TSC rate.

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

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index aae5e58..2be70df 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs)
     int r;
 
     /*
+     * If a TSC rate is migrated and the user does not specify the
+     * vcpu's TSC rate on the destination, the migrated TSC rate will
+     * be used on the destination after the migration.
+     */
+    if (env->tsc_khz_saved && !env->tsc_khz) {
+        if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) {
+            r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved);
+            if (r < 0) {
+                fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
+            }
+        } else {
+            r = -1;
+            fprintf(stderr, "KVM doesn't support TSC scaling\n");
+        }
+        if (r < 0) {
+            fprintf(stderr, "Use host TSC frequency instead. "
+                    "Guest TSC may be inaccurate.\n");
+        }
+    }
+
+    /*
      * Prepare vcpu's TSC rate to be migrated.
      *
      * - If the user specifies the TSC rate by cpu option 'tsc-freq',
-- 
2.4.8

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

[Haozhong Zhang]

Set vcpu's TSC rate to the migrated value if the user does not specify a
TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If
KVM supports TSC scaling, guest programs will observe TSC increasing in
the migrated rate other than the host TSC rate.

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

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index aae5e58..2be70df 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs)
     int r;
 
     /*
+     * If a TSC rate is migrated and the user does not specify the
+     * vcpu's TSC rate on the destination, the migrated TSC rate will
+     * be used on the destination after the migration.
+     */
+    if (env->tsc_khz_saved && !env->tsc_khz) {
+        if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) {
+            r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved);
+            if (r < 0) {
+                fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
+            }
+        } else {
+            r = -1;
+            fprintf(stderr, "KVM doesn't support TSC scaling\n");
+        }
+        if (r < 0) {
+            fprintf(stderr, "Use host TSC frequency instead. "
+                    "Guest TSC may be inaccurate.\n");
+        }
+    }
+
+    /*
      * Prepare vcpu's TSC rate to be migrated.
      *
      * - If the user specifies the TSC rate by cpu option 'tsc-freq',
-- 
2.4.8

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

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 4503 bytes --]

On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> The value of the migrated vcpu's TSC rate is determined as below.
>  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
>     user-specified value will be used.
>  2. If neither a user-specified TSC rate nor a migrated TSC rate is
>     present, we will use the TSC rate from KVM (returned by
>     KVM_GET_TSC_KHZ).
>  3. Otherwise, we will use the migrated TSC rate.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  include/sysemu/kvm.h |  2 ++
>  kvm-all.c            |  1 +
>  target-arm/kvm.c     |  5 +++++
>  target-i386/kvm.c    | 33 +++++++++++++++++++++++++++++++++
>  target-mips/kvm.c    |  5 +++++
>  target-ppc/kvm.c     |  5 +++++
>  target-s390x/kvm.c   |  5 +++++
>  7 files changed, 56 insertions(+)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 461ef65..0ec8b98 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -328,6 +328,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
>  
>  int kvm_arch_msi_data_to_gsi(uint32_t data);
>  
> +int kvm_arch_setup_tsc_khz(CPUState *cpu);
> +
>  int kvm_set_irq(KVMState *s, int irq, int level);
>  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
>  
> diff --git a/kvm-all.c b/kvm-all.c
> index c442838..1ecaf04 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1757,6 +1757,7 @@ static void do_kvm_cpu_synchronize_post_init(void *arg)
>  {
>      CPUState *cpu = arg;
>  
> +    kvm_arch_setup_tsc_khz(cpu);

Sorry if this is a stupid question, but why aren't you doing this from
the i386 kvm_arch_put_registers when level == KVM_PUT_FULL_STATE, rather
than introducing x86 specifics to the generic KVM api?

Cheers
James

>      kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
>      cpu->kvm_vcpu_dirty = false;
>  }
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 79ef4c6..a724f6d 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -614,3 +614,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      return (data - 32) & 0xffff;
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +    return 0;
> +}
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 64046cb..aae5e58 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      abort();
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +    X86CPU *cpu = X86_CPU(cs);
> +    CPUX86State *env = &cpu->env;
> +    int r;
> +
> +    /*
> +     * Prepare vcpu's TSC rate to be migrated.
> +     *
> +     * - If the user specifies the TSC rate by cpu option 'tsc-freq',
> +     *   we will use the user-specified value.
> +     *
> +     * - If there is neither user-specified TSC rate nor migrated TSC
> +     *   rate, we will ask KVM for the TSC rate by calling
> +     *   KVM_GET_TSC_KHZ.
> +     *
> +     * - Otherwise, if there is a migrated TSC rate, we will use the
> +     *   migrated value.
> +     */
> +    if (env->tsc_khz) {
> +        env->tsc_khz_saved = env->tsc_khz;
> +    } else if (!env->tsc_khz_saved) {
> +        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_saved = r;
> +    }
> +
> +    return 0;
> +}
> diff --git a/target-mips/kvm.c b/target-mips/kvm.c
> index 12d7db3..fb26d7e 100644
> --- a/target-mips/kvm.c
> +++ b/target-mips/kvm.c
> @@ -687,3 +687,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      abort();
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +    return 0;
> +}
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index ac70f08..c429f0c 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -2510,3 +2510,8 @@ int kvmppc_enable_hwrng(void)
>  
>      return kvmppc_enable_hcall(kvm_state, H_RANDOM);
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +    return 0;
> +}
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index c3be180..db5d436 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -2248,3 +2248,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      abort();
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +    return 0;
> +}
> -- 
> 2.4.8
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 4503 bytes --]

On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> The value of the migrated vcpu's TSC rate is determined as below.
>  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
>     user-specified value will be used.
>  2. If neither a user-specified TSC rate nor a migrated TSC rate is
>     present, we will use the TSC rate from KVM (returned by
>     KVM_GET_TSC_KHZ).
>  3. Otherwise, we will use the migrated TSC rate.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  include/sysemu/kvm.h |  2 ++
>  kvm-all.c            |  1 +
>  target-arm/kvm.c     |  5 +++++
>  target-i386/kvm.c    | 33 +++++++++++++++++++++++++++++++++
>  target-mips/kvm.c    |  5 +++++
>  target-ppc/kvm.c     |  5 +++++
>  target-s390x/kvm.c   |  5 +++++
>  7 files changed, 56 insertions(+)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 461ef65..0ec8b98 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -328,6 +328,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
>  
>  int kvm_arch_msi_data_to_gsi(uint32_t data);
>  
> +int kvm_arch_setup_tsc_khz(CPUState *cpu);
> +
>  int kvm_set_irq(KVMState *s, int irq, int level);
>  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
>  
> diff --git a/kvm-all.c b/kvm-all.c
> index c442838..1ecaf04 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1757,6 +1757,7 @@ static void do_kvm_cpu_synchronize_post_init(void *arg)
>  {
>      CPUState *cpu = arg;
>  
> +    kvm_arch_setup_tsc_khz(cpu);

Sorry if this is a stupid question, but why aren't you doing this from
the i386 kvm_arch_put_registers when level == KVM_PUT_FULL_STATE, rather
than introducing x86 specifics to the generic KVM api?

Cheers
James

>      kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
>      cpu->kvm_vcpu_dirty = false;
>  }
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 79ef4c6..a724f6d 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -614,3 +614,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      return (data - 32) & 0xffff;
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +    return 0;
> +}
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 64046cb..aae5e58 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      abort();
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +    X86CPU *cpu = X86_CPU(cs);
> +    CPUX86State *env = &cpu->env;
> +    int r;
> +
> +    /*
> +     * Prepare vcpu's TSC rate to be migrated.
> +     *
> +     * - If the user specifies the TSC rate by cpu option 'tsc-freq',
> +     *   we will use the user-specified value.
> +     *
> +     * - If there is neither user-specified TSC rate nor migrated TSC
> +     *   rate, we will ask KVM for the TSC rate by calling
> +     *   KVM_GET_TSC_KHZ.
> +     *
> +     * - Otherwise, if there is a migrated TSC rate, we will use the
> +     *   migrated value.
> +     */
> +    if (env->tsc_khz) {
> +        env->tsc_khz_saved = env->tsc_khz;
> +    } else if (!env->tsc_khz_saved) {
> +        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_saved = r;
> +    }
> +
> +    return 0;
> +}
> diff --git a/target-mips/kvm.c b/target-mips/kvm.c
> index 12d7db3..fb26d7e 100644
> --- a/target-mips/kvm.c
> +++ b/target-mips/kvm.c
> @@ -687,3 +687,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      abort();
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +    return 0;
> +}
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index ac70f08..c429f0c 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -2510,3 +2510,8 @@ int kvmppc_enable_hwrng(void)
>  
>      return kvmppc_enable_hcall(kvm_state, H_RANDOM);
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +    return 0;
> +}
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index c3be180..db5d436 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -2248,3 +2248,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      abort();
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +    return 0;
> +}
> -- 
> 2.4.8
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Haozhong Zhang]

On Mon, Nov 02, 2015 at 09:40:18AM +0000, James Hogan wrote:
> On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> > The value of the migrated vcpu's TSC rate is determined as below.
> >  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
> >     user-specified value will be used.
> >  2. If neither a user-specified TSC rate nor a migrated TSC rate is
> >     present, we will use the TSC rate from KVM (returned by
> >     KVM_GET_TSC_KHZ).
> >  3. Otherwise, we will use the migrated TSC rate.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  include/sysemu/kvm.h |  2 ++
> >  kvm-all.c            |  1 +
> >  target-arm/kvm.c     |  5 +++++
> >  target-i386/kvm.c    | 33 +++++++++++++++++++++++++++++++++
> >  target-mips/kvm.c    |  5 +++++
> >  target-ppc/kvm.c     |  5 +++++
> >  target-s390x/kvm.c   |  5 +++++
> >  7 files changed, 56 insertions(+)
> > 
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index 461ef65..0ec8b98 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -328,6 +328,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
> >  
> >  int kvm_arch_msi_data_to_gsi(uint32_t data);
> >  
> > +int kvm_arch_setup_tsc_khz(CPUState *cpu);
> > +
> >  int kvm_set_irq(KVMState *s, int irq, int level);
> >  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
> >  
> > diff --git a/kvm-all.c b/kvm-all.c
> > index c442838..1ecaf04 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -1757,6 +1757,7 @@ static void do_kvm_cpu_synchronize_post_init(void *arg)
> >  {
> >      CPUState *cpu = arg;
> >  
> > +    kvm_arch_setup_tsc_khz(cpu);
> 
> Sorry if this is a stupid question, but why aren't you doing this from
> the i386 kvm_arch_put_registers when level == KVM_PUT_FULL_STATE, rather
> than introducing x86 specifics to the generic KVM api?
> 
> Cheers
> James
>

Yes, I could call kvm_arch_setup_tsc_khz() in kvm_arch_put_registers()
of target-i386 when level == KVM_PUT_FULL_STATE, so that I will not need
to make kvm_arch_setup_tsc_khz() a generic KVM API (which looks weird
for targets other than i386).

Thanks, James!

Haozhong

> >      kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
> >      cpu->kvm_vcpu_dirty = false;
> >  }
> > diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> > index 79ef4c6..a724f6d 100644
> > --- a/target-arm/kvm.c
> > +++ b/target-arm/kvm.c
> > @@ -614,3 +614,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> >  {
> >      return (data - 32) & 0xffff;
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +    return 0;
> > +}
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 64046cb..aae5e58 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> >  {
> >      abort();
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +    X86CPU *cpu = X86_CPU(cs);
> > +    CPUX86State *env = &cpu->env;
> > +    int r;
> > +
> > +    /*
> > +     * Prepare vcpu's TSC rate to be migrated.
> > +     *
> > +     * - If the user specifies the TSC rate by cpu option 'tsc-freq',
> > +     *   we will use the user-specified value.
> > +     *
> > +     * - If there is neither user-specified TSC rate nor migrated TSC
> > +     *   rate, we will ask KVM for the TSC rate by calling
> > +     *   KVM_GET_TSC_KHZ.
> > +     *
> > +     * - Otherwise, if there is a migrated TSC rate, we will use the
> > +     *   migrated value.
> > +     */
> > +    if (env->tsc_khz) {
> > +        env->tsc_khz_saved = env->tsc_khz;
> > +    } else if (!env->tsc_khz_saved) {
> > +        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_saved = r;
> > +    }
> > +
> > +    return 0;
> > +}
> > diff --git a/target-mips/kvm.c b/target-mips/kvm.c
> > index 12d7db3..fb26d7e 100644
> > --- a/target-mips/kvm.c
> > +++ b/target-mips/kvm.c
> > @@ -687,3 +687,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> >  {
> >      abort();
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +    return 0;
> > +}
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index ac70f08..c429f0c 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -2510,3 +2510,8 @@ int kvmppc_enable_hwrng(void)
> >  
> >      return kvmppc_enable_hcall(kvm_state, H_RANDOM);
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +    return 0;
> > +}
> > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> > index c3be180..db5d436 100644
> > --- a/target-s390x/kvm.c
> > +++ b/target-s390x/kvm.c
> > @@ -2248,3 +2248,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> >  {
> >      abort();
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +    return 0;
> > +}
> > -- 
> > 2.4.8
> > 

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

[Haozhong Zhang]

On Mon, Nov 02, 2015 at 09:40:18AM +0000, James Hogan wrote:
> On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> > The value of the migrated vcpu's TSC rate is determined as below.
> >  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
> >     user-specified value will be used.
> >  2. If neither a user-specified TSC rate nor a migrated TSC rate is
> >     present, we will use the TSC rate from KVM (returned by
> >     KVM_GET_TSC_KHZ).
> >  3. Otherwise, we will use the migrated TSC rate.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  include/sysemu/kvm.h |  2 ++
> >  kvm-all.c            |  1 +
> >  target-arm/kvm.c     |  5 +++++
> >  target-i386/kvm.c    | 33 +++++++++++++++++++++++++++++++++
> >  target-mips/kvm.c    |  5 +++++
> >  target-ppc/kvm.c     |  5 +++++
> >  target-s390x/kvm.c   |  5 +++++
> >  7 files changed, 56 insertions(+)
> > 
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index 461ef65..0ec8b98 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -328,6 +328,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
> >  
> >  int kvm_arch_msi_data_to_gsi(uint32_t data);
> >  
> > +int kvm_arch_setup_tsc_khz(CPUState *cpu);
> > +
> >  int kvm_set_irq(KVMState *s, int irq, int level);
> >  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
> >  
> > diff --git a/kvm-all.c b/kvm-all.c
> > index c442838..1ecaf04 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -1757,6 +1757,7 @@ static void do_kvm_cpu_synchronize_post_init(void *arg)
> >  {
> >      CPUState *cpu = arg;
> >  
> > +    kvm_arch_setup_tsc_khz(cpu);
> 
> Sorry if this is a stupid question, but why aren't you doing this from
> the i386 kvm_arch_put_registers when level == KVM_PUT_FULL_STATE, rather
> than introducing x86 specifics to the generic KVM api?
> 
> Cheers
> James
>

Yes, I could call kvm_arch_setup_tsc_khz() in kvm_arch_put_registers()
of target-i386 when level == KVM_PUT_FULL_STATE, so that I will not need
to make kvm_arch_setup_tsc_khz() a generic KVM API (which looks weird
for targets other than i386).

Thanks, James!

Haozhong

> >      kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
> >      cpu->kvm_vcpu_dirty = false;
> >  }
> > diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> > index 79ef4c6..a724f6d 100644
> > --- a/target-arm/kvm.c
> > +++ b/target-arm/kvm.c
> > @@ -614,3 +614,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> >  {
> >      return (data - 32) & 0xffff;
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +    return 0;
> > +}
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 64046cb..aae5e58 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> >  {
> >      abort();
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +    X86CPU *cpu = X86_CPU(cs);
> > +    CPUX86State *env = &cpu->env;
> > +    int r;
> > +
> > +    /*
> > +     * Prepare vcpu's TSC rate to be migrated.
> > +     *
> > +     * - If the user specifies the TSC rate by cpu option 'tsc-freq',
> > +     *   we will use the user-specified value.
> > +     *
> > +     * - If there is neither user-specified TSC rate nor migrated TSC
> > +     *   rate, we will ask KVM for the TSC rate by calling
> > +     *   KVM_GET_TSC_KHZ.
> > +     *
> > +     * - Otherwise, if there is a migrated TSC rate, we will use the
> > +     *   migrated value.
> > +     */
> > +    if (env->tsc_khz) {
> > +        env->tsc_khz_saved = env->tsc_khz;
> > +    } else if (!env->tsc_khz_saved) {
> > +        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_saved = r;
> > +    }
> > +
> > +    return 0;
> > +}
> > diff --git a/target-mips/kvm.c b/target-mips/kvm.c
> > index 12d7db3..fb26d7e 100644
> > --- a/target-mips/kvm.c
> > +++ b/target-mips/kvm.c
> > @@ -687,3 +687,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> >  {
> >      abort();
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +    return 0;
> > +}
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index ac70f08..c429f0c 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -2510,3 +2510,8 @@ int kvmppc_enable_hwrng(void)
> >  
> >      return kvmppc_enable_hcall(kvm_state, H_RANDOM);
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +    return 0;
> > +}
> > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> > index c3be180..db5d436 100644
> > --- a/target-s390x/kvm.c
> > +++ b/target-s390x/kvm.c
> > @@ -2248,3 +2248,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> >  {
> >      abort();
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +    return 0;
> > +}
> > -- 
> > 2.4.8
> > 

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

[Eduardo Habkost]

On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> The value of the migrated vcpu's TSC rate is determined as below.
>  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
>     user-specified value will be used.
>  2. If neither a user-specified TSC rate nor a migrated TSC rate is
>     present, we will use the TSC rate from KVM (returned by
>     KVM_GET_TSC_KHZ).
>  3. Otherwise, we will use the migrated TSC rate.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
[...]
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 64046cb..aae5e58 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      abort();
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +    X86CPU *cpu = X86_CPU(cs);
> +    CPUX86State *env = &cpu->env;
> +    int r;
> +
> +    /*
> +     * Prepare vcpu's TSC rate to be migrated.
> +     *
> +     * - If the user specifies the TSC rate by cpu option 'tsc-freq',
> +     *   we will use the user-specified value.
> +     *
> +     * - If there is neither user-specified TSC rate nor migrated TSC
> +     *   rate, we will ask KVM for the TSC rate by calling
> +     *   KVM_GET_TSC_KHZ.
> +     *
> +     * - Otherwise, if there is a migrated TSC rate, we will use the
> +     *   migrated value.
> +     */
> +    if (env->tsc_khz) {
> +        env->tsc_khz_saved = env->tsc_khz;
> +    } else if (!env->tsc_khz_saved) {
> +        r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> +        if (r < 0) {
> +            fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
> +            return r;
> +        }

The lack of KVM_CAP_GET_TSC_KHZ should make QEMU abort, unless the user
is explicitly requesting a more strict mode where the TSC frequency will
be guaranteed to never change.

> +        env->tsc_khz_saved = r;
> +    }

Why do you need a separate tsc_khz_saved field, and don't simply use
tsc_khz? It would have the additional feature of letting QMP clients
query the current TSC rate by asking for the tsc-freq property on CPU
objects.


> +
> +    return 0;
> +}

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> The value of the migrated vcpu's TSC rate is determined as below.
>  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
>     user-specified value will be used.
>  2. If neither a user-specified TSC rate nor a migrated TSC rate is
>     present, we will use the TSC rate from KVM (returned by
>     KVM_GET_TSC_KHZ).
>  3. Otherwise, we will use the migrated TSC rate.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
[...]
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 64046cb..aae5e58 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      abort();
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +    X86CPU *cpu = X86_CPU(cs);
> +    CPUX86State *env = &cpu->env;
> +    int r;
> +
> +    /*
> +     * Prepare vcpu's TSC rate to be migrated.
> +     *
> +     * - If the user specifies the TSC rate by cpu option 'tsc-freq',
> +     *   we will use the user-specified value.
> +     *
> +     * - If there is neither user-specified TSC rate nor migrated TSC
> +     *   rate, we will ask KVM for the TSC rate by calling
> +     *   KVM_GET_TSC_KHZ.
> +     *
> +     * - Otherwise, if there is a migrated TSC rate, we will use the
> +     *   migrated value.
> +     */
> +    if (env->tsc_khz) {
> +        env->tsc_khz_saved = env->tsc_khz;
> +    } else if (!env->tsc_khz_saved) {
> +        r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> +        if (r < 0) {
> +            fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
> +            return r;
> +        }

The lack of KVM_CAP_GET_TSC_KHZ should make QEMU abort, unless the user
is explicitly requesting a more strict mode where the TSC frequency will
be guaranteed to never change.

> +        env->tsc_khz_saved = r;
> +    }

Why do you need a separate tsc_khz_saved field, and don't simply use
tsc_khz? It would have the additional feature of letting QMP clients
query the current TSC rate by asking for the tsc-freq property on CPU
objects.


> +
> +    return 0;
> +}

-- 
Eduardo

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

[Haozhong Zhang]

On 11/04/15 19:42, Eduardo Habkost wrote:
> On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> > The value of the migrated vcpu's TSC rate is determined as below.
> >  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
> >     user-specified value will be used.
> >  2. If neither a user-specified TSC rate nor a migrated TSC rate is
> >     present, we will use the TSC rate from KVM (returned by
> >     KVM_GET_TSC_KHZ).
> >  3. Otherwise, we will use the migrated TSC rate.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> [...]
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 64046cb..aae5e58 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> >  {
> >      abort();
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +    X86CPU *cpu = X86_CPU(cs);
> > +    CPUX86State *env = &cpu->env;
> > +    int r;
> > +
> > +    /*
> > +     * Prepare vcpu's TSC rate to be migrated.
> > +     *
> > +     * - If the user specifies the TSC rate by cpu option 'tsc-freq',
> > +     *   we will use the user-specified value.
> > +     *
> > +     * - If there is neither user-specified TSC rate nor migrated TSC
> > +     *   rate, we will ask KVM for the TSC rate by calling
> > +     *   KVM_GET_TSC_KHZ.
> > +     *
> > +     * - Otherwise, if there is a migrated TSC rate, we will use the
> > +     *   migrated value.
> > +     */
> > +    if (env->tsc_khz) {
> > +        env->tsc_khz_saved = env->tsc_khz;
> > +    } else if (!env->tsc_khz_saved) {
> > +        r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > +        if (r < 0) {
> > +            fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
> > +            return r;
> > +        }
> 
> The lack of KVM_CAP_GET_TSC_KHZ should make QEMU abort, unless the user
> is explicitly requesting a more strict mode where the TSC frequency will
> be guaranteed to never change.
>

I agree KVM_CAP_GET_TSC_KHZ should be checked before KVM_GET_TSC_KHZ,
but I don't think the lack of it should abort QEMU. This piece of code
on the source machine is just to get the TSC frequency to be
migrated. If it fails, it will leave env->tsc_khz_saved be 0. And
according to tsc_khz_needed() in patch 1, if env->tsc_khz_saved == 0,
no TSC frequency will be migrated. So the lack of KVM_CAP_GET_TSC_KHZ
only hurts the migration and does not need to abort QEMU on the source
machine.

> > +        env->tsc_khz_saved = r;
> > +    }
> 
> Why do you need a separate tsc_khz_saved field, and don't simply use
> tsc_khz? It would have the additional feature of letting QMP clients
> query the current TSC rate by asking for the tsc-freq property on CPU
> objects.
>

It's to avoid overriding env->tsc_khz on the destination in the
migration. I can change this line to
             env->tsc_khz = env->tsc_khz_saved = r;

For the additional QMP feature, will the value of tsc-freq property be
env->tsc_khz? If yes, I guess the above change would be fine?

Haozhong

> 
> > +
> > +    return 0;
> > +}
> 
> -- 
> 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 v3 2/3] target-i386: calculate vcpu's TSC rate to be migrated

[Haozhong Zhang]

On 11/04/15 19:42, Eduardo Habkost wrote:
> On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> > The value of the migrated vcpu's TSC rate is determined as below.
> >  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
> >     user-specified value will be used.
> >  2. If neither a user-specified TSC rate nor a migrated TSC rate is
> >     present, we will use the TSC rate from KVM (returned by
> >     KVM_GET_TSC_KHZ).
> >  3. Otherwise, we will use the migrated TSC rate.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> [...]
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 64046cb..aae5e58 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> >  {
> >      abort();
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +    X86CPU *cpu = X86_CPU(cs);
> > +    CPUX86State *env = &cpu->env;
> > +    int r;
> > +
> > +    /*
> > +     * Prepare vcpu's TSC rate to be migrated.
> > +     *
> > +     * - If the user specifies the TSC rate by cpu option 'tsc-freq',
> > +     *   we will use the user-specified value.
> > +     *
> > +     * - If there is neither user-specified TSC rate nor migrated TSC
> > +     *   rate, we will ask KVM for the TSC rate by calling
> > +     *   KVM_GET_TSC_KHZ.
> > +     *
> > +     * - Otherwise, if there is a migrated TSC rate, we will use the
> > +     *   migrated value.
> > +     */
> > +    if (env->tsc_khz) {
> > +        env->tsc_khz_saved = env->tsc_khz;
> > +    } else if (!env->tsc_khz_saved) {
> > +        r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > +        if (r < 0) {
> > +            fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
> > +            return r;
> > +        }
> 
> The lack of KVM_CAP_GET_TSC_KHZ should make QEMU abort, unless the user
> is explicitly requesting a more strict mode where the TSC frequency will
> be guaranteed to never change.
>

I agree KVM_CAP_GET_TSC_KHZ should be checked before KVM_GET_TSC_KHZ,
but I don't think the lack of it should abort QEMU. This piece of code
on the source machine is just to get the TSC frequency to be
migrated. If it fails, it will leave env->tsc_khz_saved be 0. And
according to tsc_khz_needed() in patch 1, if env->tsc_khz_saved == 0,
no TSC frequency will be migrated. So the lack of KVM_CAP_GET_TSC_KHZ
only hurts the migration and does not need to abort QEMU on the source
machine.

> > +        env->tsc_khz_saved = r;
> > +    }
> 
> Why do you need a separate tsc_khz_saved field, and don't simply use
> tsc_khz? It would have the additional feature of letting QMP clients
> query the current TSC rate by asking for the tsc-freq property on CPU
> objects.
>

It's to avoid overriding env->tsc_khz on the destination in the
migration. I can change this line to
             env->tsc_khz = env->tsc_khz_saved = r;

For the additional QMP feature, will the value of tsc-freq property be
env->tsc_khz? If yes, I guess the above change would be fine?

Haozhong

> 
> > +
> > +    return 0;
> > +}
> 
> -- 
> 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 v3 2/3] target-i386: calculate vcpu's TSC rate to be migrated

[Christian Borntraeger]

Am 02.11.2015 um 10:40 schrieb James Hogan:
> On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
>> The value of the migrated vcpu's TSC rate is determined as below.
>>  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
>>     user-specified value will be used.
>>  2. If neither a user-specified TSC rate nor a migrated TSC rate is
>>     present, we will use the TSC rate from KVM (returned by
>>     KVM_GET_TSC_KHZ).
>>  3. Otherwise, we will use the migrated TSC rate.
>>
>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>> ---
>>  include/sysemu/kvm.h |  2 ++
>>  kvm-all.c            |  1 +
>>  target-arm/kvm.c     |  5 +++++
>>  target-i386/kvm.c    | 33 +++++++++++++++++++++++++++++++++
>>  target-mips/kvm.c    |  5 +++++
>>  target-ppc/kvm.c     |  5 +++++
>>  target-s390x/kvm.c   |  5 +++++
>>  7 files changed, 56 insertions(+)
>>
>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>> index 461ef65..0ec8b98 100644
>> --- a/include/sysemu/kvm.h
>> +++ b/include/sysemu/kvm.h
>> @@ -328,6 +328,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
>>  
>>  int kvm_arch_msi_data_to_gsi(uint32_t data);
>>  
>> +int kvm_arch_setup_tsc_khz(CPUState *cpu);
>> +
>>  int kvm_set_irq(KVMState *s, int irq, int level);
>>  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
>>  
>> diff --git a/kvm-all.c b/kvm-all.c
>> index c442838..1ecaf04 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -1757,6 +1757,7 @@ static void do_kvm_cpu_synchronize_post_init(void *arg)
>>  {
>>      CPUState *cpu = arg;
>>  
>> +    kvm_arch_setup_tsc_khz(cpu);
> 
> Sorry if this is a stupid question, but why aren't you doing this from
> the i386 kvm_arch_put_registers when level == KVM_PUT_FULL_STATE, rather
> than introducing x86 specifics to the generic KVM api?
> 
> Cheers
> James

I agree. We should try to keep this in x86 code.

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

[Christian Borntraeger]

Am 02.11.2015 um 10:40 schrieb James Hogan:
> On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
>> The value of the migrated vcpu's TSC rate is determined as below.
>>  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
>>     user-specified value will be used.
>>  2. If neither a user-specified TSC rate nor a migrated TSC rate is
>>     present, we will use the TSC rate from KVM (returned by
>>     KVM_GET_TSC_KHZ).
>>  3. Otherwise, we will use the migrated TSC rate.
>>
>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>> ---
>>  include/sysemu/kvm.h |  2 ++
>>  kvm-all.c            |  1 +
>>  target-arm/kvm.c     |  5 +++++
>>  target-i386/kvm.c    | 33 +++++++++++++++++++++++++++++++++
>>  target-mips/kvm.c    |  5 +++++
>>  target-ppc/kvm.c     |  5 +++++
>>  target-s390x/kvm.c   |  5 +++++
>>  7 files changed, 56 insertions(+)
>>
>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>> index 461ef65..0ec8b98 100644
>> --- a/include/sysemu/kvm.h
>> +++ b/include/sysemu/kvm.h
>> @@ -328,6 +328,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
>>  
>>  int kvm_arch_msi_data_to_gsi(uint32_t data);
>>  
>> +int kvm_arch_setup_tsc_khz(CPUState *cpu);
>> +
>>  int kvm_set_irq(KVMState *s, int irq, int level);
>>  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
>>  
>> diff --git a/kvm-all.c b/kvm-all.c
>> index c442838..1ecaf04 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -1757,6 +1757,7 @@ static void do_kvm_cpu_synchronize_post_init(void *arg)
>>  {
>>      CPUState *cpu = arg;
>>  
>> +    kvm_arch_setup_tsc_khz(cpu);
> 
> Sorry if this is a stupid question, but why aren't you doing this from
> the i386 kvm_arch_put_registers when level == KVM_PUT_FULL_STATE, rather
> than introducing x86 specifics to the generic KVM api?
> 
> Cheers
> James

I agree. We should try to keep this in x86 code.

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

[Haozhong Zhang]

On 11/05/15 09:05, Christian Borntraeger wrote:
> Am 02.11.2015 um 10:40 schrieb James Hogan:
> > On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> >> The value of the migrated vcpu's TSC rate is determined as below.
> >>  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
> >>     user-specified value will be used.
> >>  2. If neither a user-specified TSC rate nor a migrated TSC rate is
> >>     present, we will use the TSC rate from KVM (returned by
> >>     KVM_GET_TSC_KHZ).
> >>  3. Otherwise, we will use the migrated TSC rate.
> >>
> >> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> >> ---
> >>  include/sysemu/kvm.h |  2 ++
> >>  kvm-all.c            |  1 +
> >>  target-arm/kvm.c     |  5 +++++
> >>  target-i386/kvm.c    | 33 +++++++++++++++++++++++++++++++++
> >>  target-mips/kvm.c    |  5 +++++
> >>  target-ppc/kvm.c     |  5 +++++
> >>  target-s390x/kvm.c   |  5 +++++
> >>  7 files changed, 56 insertions(+)
> >>
> >> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> >> index 461ef65..0ec8b98 100644
> >> --- a/include/sysemu/kvm.h
> >> +++ b/include/sysemu/kvm.h
> >> @@ -328,6 +328,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
> >>  
> >>  int kvm_arch_msi_data_to_gsi(uint32_t data);
> >>  
> >> +int kvm_arch_setup_tsc_khz(CPUState *cpu);
> >> +
> >>  int kvm_set_irq(KVMState *s, int irq, int level);
> >>  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
> >>  
> >> diff --git a/kvm-all.c b/kvm-all.c
> >> index c442838..1ecaf04 100644
> >> --- a/kvm-all.c
> >> +++ b/kvm-all.c
> >> @@ -1757,6 +1757,7 @@ static void do_kvm_cpu_synchronize_post_init(void *arg)
> >>  {
> >>      CPUState *cpu = arg;
> >>  
> >> +    kvm_arch_setup_tsc_khz(cpu);
> > 
> > Sorry if this is a stupid question, but why aren't you doing this from
> > the i386 kvm_arch_put_registers when level == KVM_PUT_FULL_STATE, rather
> > than introducing x86 specifics to the generic KVM api?
> > 
> > Cheers
> > James
> 
> I agree. We should try to keep this in x86 code.
> 
> 

As in another reply, I'm going to move the above line to
kvm_arch_put_registers() of target-i386 so that it will not pollute
other targets.

Haozhong

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

[Haozhong Zhang]

On 11/05/15 09:05, Christian Borntraeger wrote:
> Am 02.11.2015 um 10:40 schrieb James Hogan:
> > On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> >> The value of the migrated vcpu's TSC rate is determined as below.
> >>  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
> >>     user-specified value will be used.
> >>  2. If neither a user-specified TSC rate nor a migrated TSC rate is
> >>     present, we will use the TSC rate from KVM (returned by
> >>     KVM_GET_TSC_KHZ).
> >>  3. Otherwise, we will use the migrated TSC rate.
> >>
> >> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> >> ---
> >>  include/sysemu/kvm.h |  2 ++
> >>  kvm-all.c            |  1 +
> >>  target-arm/kvm.c     |  5 +++++
> >>  target-i386/kvm.c    | 33 +++++++++++++++++++++++++++++++++
> >>  target-mips/kvm.c    |  5 +++++
> >>  target-ppc/kvm.c     |  5 +++++
> >>  target-s390x/kvm.c   |  5 +++++
> >>  7 files changed, 56 insertions(+)
> >>
> >> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> >> index 461ef65..0ec8b98 100644
> >> --- a/include/sysemu/kvm.h
> >> +++ b/include/sysemu/kvm.h
> >> @@ -328,6 +328,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
> >>  
> >>  int kvm_arch_msi_data_to_gsi(uint32_t data);
> >>  
> >> +int kvm_arch_setup_tsc_khz(CPUState *cpu);
> >> +
> >>  int kvm_set_irq(KVMState *s, int irq, int level);
> >>  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
> >>  
> >> diff --git a/kvm-all.c b/kvm-all.c
> >> index c442838..1ecaf04 100644
> >> --- a/kvm-all.c
> >> +++ b/kvm-all.c
> >> @@ -1757,6 +1757,7 @@ static void do_kvm_cpu_synchronize_post_init(void *arg)
> >>  {
> >>      CPUState *cpu = arg;
> >>  
> >> +    kvm_arch_setup_tsc_khz(cpu);
> > 
> > Sorry if this is a stupid question, but why aren't you doing this from
> > the i386 kvm_arch_put_registers when level == KVM_PUT_FULL_STATE, rather
> > than introducing x86 specifics to the generic KVM api?
> > 
> > Cheers
> > James
> 
> I agree. We should try to keep this in x86 code.
> 
> 

As in another reply, I'm going to move the above line to
kvm_arch_put_registers() of target-i386 so that it will not pollute
other targets.

Haozhong

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

[Eduardo Habkost]

On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> On 11/04/15 19:42, Eduardo Habkost wrote:
> > On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> > > The value of the migrated vcpu's TSC rate is determined as below.
> > >  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
> > >     user-specified value will be used.
> > >  2. If neither a user-specified TSC rate nor a migrated TSC rate is
> > >     present, we will use the TSC rate from KVM (returned by
> > >     KVM_GET_TSC_KHZ).
> > >  3. Otherwise, we will use the migrated TSC rate.
> > > 
> > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > [...]
> > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > > index 64046cb..aae5e58 100644
> > > --- a/target-i386/kvm.c
> > > +++ b/target-i386/kvm.c
> > > @@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> > >  {
> > >      abort();
> > >  }
> > > +
> > > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > > +{
> > > +    X86CPU *cpu = X86_CPU(cs);
> > > +    CPUX86State *env = &cpu->env;
> > > +    int r;
> > > +
> > > +    /*
> > > +     * Prepare vcpu's TSC rate to be migrated.
> > > +     *
> > > +     * - If the user specifies the TSC rate by cpu option 'tsc-freq',
> > > +     *   we will use the user-specified value.
> > > +     *
> > > +     * - If there is neither user-specified TSC rate nor migrated TSC
> > > +     *   rate, we will ask KVM for the TSC rate by calling
> > > +     *   KVM_GET_TSC_KHZ.
> > > +     *
> > > +     * - Otherwise, if there is a migrated TSC rate, we will use the
> > > +     *   migrated value.
> > > +     */
> > > +    if (env->tsc_khz) {
> > > +        env->tsc_khz_saved = env->tsc_khz;
> > > +    } else if (!env->tsc_khz_saved) {
> > > +        r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > +        if (r < 0) {
> > > +            fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
> > > +            return r;
> > > +        }
> > 
> > The lack of KVM_CAP_GET_TSC_KHZ should make QEMU abort, unless the user
> > is explicitly requesting a more strict mode where the TSC frequency will
> > be guaranteed to never change.
> >
> 
> I agree KVM_CAP_GET_TSC_KHZ should be checked before KVM_GET_TSC_KHZ,
> but I don't think the lack of it should abort QEMU.


Oops, I meant to write: "the lack of KVM_CAP_GET_TSC_KHZ should not
abort QEMU".

> This piece of code
> on the source machine is just to get the TSC frequency to be
> migrated. If it fails, it will leave env->tsc_khz_saved be 0. And
> according to tsc_khz_needed() in patch 1, if env->tsc_khz_saved == 0,
> no TSC frequency will be migrated. So the lack of KVM_CAP_GET_TSC_KHZ
> only hurts the migration and does not need to abort QEMU on the source
> machine.

The lack of KVM_CAP_GET_TSC_KHZ shouldn't prevent migration either. but
it looks your code is not doing that: errors from
kvm_arch_setup_tsc_khz() are being ignored by
do_kvm_cpu_synchronize_post_init(), sorry for the noise.

> 
> > > +        env->tsc_khz_saved = r;
> > > +    }
> > 
> > Why do you need a separate tsc_khz_saved field, and don't simply use
> > tsc_khz? It would have the additional feature of letting QMP clients
> > query the current TSC rate by asking for the tsc-freq property on CPU
> > objects.
> >
> 
> It's to avoid overriding env->tsc_khz on the destination in the
> migration. I can change this line to
>              env->tsc_khz = env->tsc_khz_saved = r;

You are already avoiding overriding env->tsc_khz, because you use
KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
code, if you could just do this:

    if (!env->tsc_khz) {
        env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
    }


> 
> For the additional QMP feature, will the value of tsc-freq property be
> env->tsc_khz? If yes, I guess the above change would be fine?

It may work, but I still don't see the point of duplicating the field
and duplicating the existing SET_TSC_KHZ code.

-- 
Eduardo

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

[Eduardo Habkost]

On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> On 11/04/15 19:42, Eduardo Habkost wrote:
> > On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> > > The value of the migrated vcpu's TSC rate is determined as below.
> > >  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
> > >     user-specified value will be used.
> > >  2. If neither a user-specified TSC rate nor a migrated TSC rate is
> > >     present, we will use the TSC rate from KVM (returned by
> > >     KVM_GET_TSC_KHZ).
> > >  3. Otherwise, we will use the migrated TSC rate.
> > > 
> > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > [...]
> > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > > index 64046cb..aae5e58 100644
> > > --- a/target-i386/kvm.c
> > > +++ b/target-i386/kvm.c
> > > @@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> > >  {
> > >      abort();
> > >  }
> > > +
> > > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > > +{
> > > +    X86CPU *cpu = X86_CPU(cs);
> > > +    CPUX86State *env = &cpu->env;
> > > +    int r;
> > > +
> > > +    /*
> > > +     * Prepare vcpu's TSC rate to be migrated.
> > > +     *
> > > +     * - If the user specifies the TSC rate by cpu option 'tsc-freq',
> > > +     *   we will use the user-specified value.
> > > +     *
> > > +     * - If there is neither user-specified TSC rate nor migrated TSC
> > > +     *   rate, we will ask KVM for the TSC rate by calling
> > > +     *   KVM_GET_TSC_KHZ.
> > > +     *
> > > +     * - Otherwise, if there is a migrated TSC rate, we will use the
> > > +     *   migrated value.
> > > +     */
> > > +    if (env->tsc_khz) {
> > > +        env->tsc_khz_saved = env->tsc_khz;
> > > +    } else if (!env->tsc_khz_saved) {
> > > +        r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > +        if (r < 0) {
> > > +            fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
> > > +            return r;
> > > +        }
> > 
> > The lack of KVM_CAP_GET_TSC_KHZ should make QEMU abort, unless the user
> > is explicitly requesting a more strict mode where the TSC frequency will
> > be guaranteed to never change.
> >
> 
> I agree KVM_CAP_GET_TSC_KHZ should be checked before KVM_GET_TSC_KHZ,
> but I don't think the lack of it should abort QEMU.


Oops, I meant to write: "the lack of KVM_CAP_GET_TSC_KHZ should not
abort QEMU".

> This piece of code
> on the source machine is just to get the TSC frequency to be
> migrated. If it fails, it will leave env->tsc_khz_saved be 0. And
> according to tsc_khz_needed() in patch 1, if env->tsc_khz_saved == 0,
> no TSC frequency will be migrated. So the lack of KVM_CAP_GET_TSC_KHZ
> only hurts the migration and does not need to abort QEMU on the source
> machine.

The lack of KVM_CAP_GET_TSC_KHZ shouldn't prevent migration either. but
it looks your code is not doing that: errors from
kvm_arch_setup_tsc_khz() are being ignored by
do_kvm_cpu_synchronize_post_init(), sorry for the noise.

> 
> > > +        env->tsc_khz_saved = r;
> > > +    }
> > 
> > Why do you need a separate tsc_khz_saved field, and don't simply use
> > tsc_khz? It would have the additional feature of letting QMP clients
> > query the current TSC rate by asking for the tsc-freq property on CPU
> > objects.
> >
> 
> It's to avoid overriding env->tsc_khz on the destination in the
> migration. I can change this line to
>              env->tsc_khz = env->tsc_khz_saved = r;

You are already avoiding overriding env->tsc_khz, because you use
KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
code, if you could just do this:

    if (!env->tsc_khz) {
        env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
    }


> 
> For the additional QMP feature, will the value of tsc-freq property be
> env->tsc_khz? If yes, I guess the above change would be fine?

It may work, but I still don't see the point of duplicating the field
and duplicating the existing SET_TSC_KHZ code.

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Nov 02, 2015 at 05:26:43PM +0800, Haozhong Zhang wrote:
> Set vcpu's TSC rate to the migrated value if the user does not specify a
> TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If
> KVM supports TSC scaling, guest programs will observe TSC increasing in
> the migrated rate other than the host TSC rate.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  target-i386/kvm.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index aae5e58..2be70df 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs)
>      int r;
>  
>      /*
> +     * If a TSC rate is migrated and the user does not specify the
> +     * vcpu's TSC rate on the destination, the migrated TSC rate will
> +     * be used on the destination after the migration.
> +     */
> +    if (env->tsc_khz_saved && !env->tsc_khz) {
> +        if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) {
> +            r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved);

Why are you duplicating the existing KVM_SET_TSC_KHZ code in
kvm_arch_init_vcpu()?

> +            if (r < 0) {
> +                fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");

If you want to report errors, please use error_report().

(But I don't think we want to print those warnings. See below.)

> +            }
> +        } else {
> +            r = -1;
> +            fprintf(stderr, "KVM doesn't support TSC scaling\n");
> +        }
> +        if (r < 0) {
> +            fprintf(stderr, "Use host TSC frequency instead. "

Did you mean "Using host TSC frequency instead."?

> +                    "Guest TSC may be inaccurate.\n");
> +        }
> +    }

This will make QEMU print a warning every single time when migrating to
hosts that don't support TSC scaling, even if the source and destination
hosts already have the same TSC frequency. That means most users will
see a bogus warning, in today's hardware.

Maybe it will be acceptable to print a warning if (and only if) we know
that the host TSC is different from the original TSC frequency.

Considering that we already have code to handle tsc_khz that prints an
error, you don't need to duplicate it. You could handle both
user-provided and migration tsc_khz cases with the same code. With
something like this:

    if (env->tsc_khz) { /* may be set by the user, or loaded from incoming migration */
        r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
            kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) :
            -ENOTSUP;
        if (r < 0) {
            int64_t cur_freq = kvm_check_extension(KVM_CAP_GET_TSC_KHZ)) ?
                               kvm_vcpu_ioctl(KVM_GET_TSC_KHZ) :
                               0;
            /* If we know the host frequency, print a warning every time
             * there's a mismatch.
             * If we don't know the host frequency, print a warning only
             * if the user asked for a specific TSC frequency.
             */
            if ((cur_freq <= 0 && env->tsc_freq_requested_by_user) ||
                (cur_freq > 0 && cur_freq != env->tsc_khz)) {
                error_report("warning: TSC frequency mismatch between VM and host, and TSC scaling unavailable");
                if (env->tsc_freq_set_by_user) {
                    return r;
                }
            }
        }
    }

You will just need a new tsc_freq_requested_by_user field to track if
the TSC frequency was explicitly requested by the user.

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Nov 02, 2015 at 05:26:43PM +0800, Haozhong Zhang wrote:
> Set vcpu's TSC rate to the migrated value if the user does not specify a
> TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If
> KVM supports TSC scaling, guest programs will observe TSC increasing in
> the migrated rate other than the host TSC rate.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  target-i386/kvm.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index aae5e58..2be70df 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs)
>      int r;
>  
>      /*
> +     * If a TSC rate is migrated and the user does not specify the
> +     * vcpu's TSC rate on the destination, the migrated TSC rate will
> +     * be used on the destination after the migration.
> +     */
> +    if (env->tsc_khz_saved && !env->tsc_khz) {
> +        if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) {
> +            r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved);

Why are you duplicating the existing KVM_SET_TSC_KHZ code in
kvm_arch_init_vcpu()?

> +            if (r < 0) {
> +                fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");

If you want to report errors, please use error_report().

(But I don't think we want to print those warnings. See below.)

> +            }
> +        } else {
> +            r = -1;
> +            fprintf(stderr, "KVM doesn't support TSC scaling\n");
> +        }
> +        if (r < 0) {
> +            fprintf(stderr, "Use host TSC frequency instead. "

Did you mean "Using host TSC frequency instead."?

> +                    "Guest TSC may be inaccurate.\n");
> +        }
> +    }

This will make QEMU print a warning every single time when migrating to
hosts that don't support TSC scaling, even if the source and destination
hosts already have the same TSC frequency. That means most users will
see a bogus warning, in today's hardware.

Maybe it will be acceptable to print a warning if (and only if) we know
that the host TSC is different from the original TSC frequency.

Considering that we already have code to handle tsc_khz that prints an
error, you don't need to duplicate it. You could handle both
user-provided and migration tsc_khz cases with the same code. With
something like this:

    if (env->tsc_khz) { /* may be set by the user, or loaded from incoming migration */
        r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
            kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) :
            -ENOTSUP;
        if (r < 0) {
            int64_t cur_freq = kvm_check_extension(KVM_CAP_GET_TSC_KHZ)) ?
                               kvm_vcpu_ioctl(KVM_GET_TSC_KHZ) :
                               0;
            /* If we know the host frequency, print a warning every time
             * there's a mismatch.
             * If we don't know the host frequency, print a warning only
             * if the user asked for a specific TSC frequency.
             */
            if ((cur_freq <= 0 && env->tsc_freq_requested_by_user) ||
                (cur_freq > 0 && cur_freq != env->tsc_khz)) {
                error_report("warning: TSC frequency mismatch between VM and host, and TSC scaling unavailable");
                if (env->tsc_freq_set_by_user) {
                    return r;
                }
            }
        }
    }

You will just need a new tsc_freq_requested_by_user field to track if
the TSC frequency was explicitly requested by the user.

-- 
Eduardo

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

[haozhong.zhang]

On 11/05/15 14:05, Eduardo Habkost wrote:
> On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> > > > The value of the migrated vcpu's TSC rate is determined as below.
> > > >  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
> > > >     user-specified value will be used.
> > > >  2. If neither a user-specified TSC rate nor a migrated TSC rate is
> > > >     present, we will use the TSC rate from KVM (returned by
> > > >     KVM_GET_TSC_KHZ).
> > > >  3. Otherwise, we will use the migrated TSC rate.
> > > > 
> > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > [...]
> > > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > > > index 64046cb..aae5e58 100644
> > > > --- a/target-i386/kvm.c
> > > > +++ b/target-i386/kvm.c
> > > > @@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> > > >  {
> > > >      abort();
> > > >  }
> > > > +
> > > > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > > > +{
> > > > +    X86CPU *cpu = X86_CPU(cs);
> > > > +    CPUX86State *env = &cpu->env;
> > > > +    int r;
> > > > +
> > > > +    /*
> > > > +     * Prepare vcpu's TSC rate to be migrated.
> > > > +     *
> > > > +     * - If the user specifies the TSC rate by cpu option 'tsc-freq',
> > > > +     *   we will use the user-specified value.
> > > > +     *
> > > > +     * - If there is neither user-specified TSC rate nor migrated TSC
> > > > +     *   rate, we will ask KVM for the TSC rate by calling
> > > > +     *   KVM_GET_TSC_KHZ.
> > > > +     *
> > > > +     * - Otherwise, if there is a migrated TSC rate, we will use the
> > > > +     *   migrated value.
> > > > +     */
> > > > +    if (env->tsc_khz) {
> > > > +        env->tsc_khz_saved = env->tsc_khz;
> > > > +    } else if (!env->tsc_khz_saved) {
> > > > +        r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > +        if (r < 0) {
> > > > +            fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
> > > > +            return r;
> > > > +        }
> > > 
> > > The lack of KVM_CAP_GET_TSC_KHZ should make QEMU abort, unless the user
> > > is explicitly requesting a more strict mode where the TSC frequency will
> > > be guaranteed to never change.
> > >
> > 
> > I agree KVM_CAP_GET_TSC_KHZ should be checked before KVM_GET_TSC_KHZ,
> > but I don't think the lack of it should abort QEMU.
> 
> 
> Oops, I meant to write: "the lack of KVM_CAP_GET_TSC_KHZ should not
> abort QEMU".
> 
> > This piece of code
> > on the source machine is just to get the TSC frequency to be
> > migrated. If it fails, it will leave env->tsc_khz_saved be 0. And
> > according to tsc_khz_needed() in patch 1, if env->tsc_khz_saved == 0,
> > no TSC frequency will be migrated. So the lack of KVM_CAP_GET_TSC_KHZ
> > only hurts the migration and does not need to abort QEMU on the source
> > machine.
> 
> The lack of KVM_CAP_GET_TSC_KHZ shouldn't prevent migration either. but
> it looks your code is not doing that: errors from
> kvm_arch_setup_tsc_khz() are being ignored by
> do_kvm_cpu_synchronize_post_init(), sorry for the noise.
>
> > 
> > > > +        env->tsc_khz_saved = r;
> > > > +    }
> > > 
> > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > tsc_khz? It would have the additional feature of letting QMP clients
> > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > objects.
> > >
> > 
> > It's to avoid overriding env->tsc_khz on the destination in the
> > migration. I can change this line to
> >              env->tsc_khz = env->tsc_khz_saved = r;
> 
> You are already avoiding overriding env->tsc_khz, because you use
> KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> code, if you could just do this:
> 
>     if (!env->tsc_khz) {
>         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
>     }
>

Consider an example that we migrate a VM from machine A to machine B
and then to machine C, and QEMU on machine B is launched with the cpu
option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
beginning):
 1) In the migration from B to C, the user-specified TSC frequency by
    'tsc-freq' on B is expected to be migrated to C. That is, the
    value of env->tsc_khz on B is migrated.
 2) If TSC frequency is migrated through env->tsc_khz, then
    env->tsc_khz on B will be overrode in the migration from A to B
    before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
    different than the user-specified TSC frequency on B, the
    expectation in 1) will not be satisfied anymore.

So, I introduce the field tsc_khz_saved to migrate TSC frequency and
(in patch 3) let the destination decide if this migrated one will be
used.

And, adding a flag like tsc_freq_requested_by_user in your comments on
patch 3 does not solve the problem.

> 
> > 
> > For the additional QMP feature, will the value of tsc-freq property be
> > env->tsc_khz? If yes, I guess the above change would be fine?
> 
> It may work, but I still don't see the point of duplicating the field
> and duplicating the existing SET_TSC_KHZ code.
>

The reason to duplicate tsc_khz is explained above. The reason to
duplicate of SET_TSC_KHZ code will be in the reply to your comments on
patch 3.

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 v3 2/3] target-i386: calculate vcpu's TSC rate to be migrated

[haozhong.zhang]

On 11/05/15 14:05, Eduardo Habkost wrote:
> On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> > > > The value of the migrated vcpu's TSC rate is determined as below.
> > > >  1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
> > > >     user-specified value will be used.
> > > >  2. If neither a user-specified TSC rate nor a migrated TSC rate is
> > > >     present, we will use the TSC rate from KVM (returned by
> > > >     KVM_GET_TSC_KHZ).
> > > >  3. Otherwise, we will use the migrated TSC rate.
> > > > 
> > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > [...]
> > > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > > > index 64046cb..aae5e58 100644
> > > > --- a/target-i386/kvm.c
> > > > +++ b/target-i386/kvm.c
> > > > @@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> > > >  {
> > > >      abort();
> > > >  }
> > > > +
> > > > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > > > +{
> > > > +    X86CPU *cpu = X86_CPU(cs);
> > > > +    CPUX86State *env = &cpu->env;
> > > > +    int r;
> > > > +
> > > > +    /*
> > > > +     * Prepare vcpu's TSC rate to be migrated.
> > > > +     *
> > > > +     * - If the user specifies the TSC rate by cpu option 'tsc-freq',
> > > > +     *   we will use the user-specified value.
> > > > +     *
> > > > +     * - If there is neither user-specified TSC rate nor migrated TSC
> > > > +     *   rate, we will ask KVM for the TSC rate by calling
> > > > +     *   KVM_GET_TSC_KHZ.
> > > > +     *
> > > > +     * - Otherwise, if there is a migrated TSC rate, we will use the
> > > > +     *   migrated value.
> > > > +     */
> > > > +    if (env->tsc_khz) {
> > > > +        env->tsc_khz_saved = env->tsc_khz;
> > > > +    } else if (!env->tsc_khz_saved) {
> > > > +        r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > +        if (r < 0) {
> > > > +            fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
> > > > +            return r;
> > > > +        }
> > > 
> > > The lack of KVM_CAP_GET_TSC_KHZ should make QEMU abort, unless the user
> > > is explicitly requesting a more strict mode where the TSC frequency will
> > > be guaranteed to never change.
> > >
> > 
> > I agree KVM_CAP_GET_TSC_KHZ should be checked before KVM_GET_TSC_KHZ,
> > but I don't think the lack of it should abort QEMU.
> 
> 
> Oops, I meant to write: "the lack of KVM_CAP_GET_TSC_KHZ should not
> abort QEMU".
> 
> > This piece of code
> > on the source machine is just to get the TSC frequency to be
> > migrated. If it fails, it will leave env->tsc_khz_saved be 0. And
> > according to tsc_khz_needed() in patch 1, if env->tsc_khz_saved == 0,
> > no TSC frequency will be migrated. So the lack of KVM_CAP_GET_TSC_KHZ
> > only hurts the migration and does not need to abort QEMU on the source
> > machine.
> 
> The lack of KVM_CAP_GET_TSC_KHZ shouldn't prevent migration either. but
> it looks your code is not doing that: errors from
> kvm_arch_setup_tsc_khz() are being ignored by
> do_kvm_cpu_synchronize_post_init(), sorry for the noise.
>
> > 
> > > > +        env->tsc_khz_saved = r;
> > > > +    }
> > > 
> > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > tsc_khz? It would have the additional feature of letting QMP clients
> > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > objects.
> > >
> > 
> > It's to avoid overriding env->tsc_khz on the destination in the
> > migration. I can change this line to
> >              env->tsc_khz = env->tsc_khz_saved = r;
> 
> You are already avoiding overriding env->tsc_khz, because you use
> KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> code, if you could just do this:
> 
>     if (!env->tsc_khz) {
>         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
>     }
>

Consider an example that we migrate a VM from machine A to machine B
and then to machine C, and QEMU on machine B is launched with the cpu
option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
beginning):
 1) In the migration from B to C, the user-specified TSC frequency by
    'tsc-freq' on B is expected to be migrated to C. That is, the
    value of env->tsc_khz on B is migrated.
 2) If TSC frequency is migrated through env->tsc_khz, then
    env->tsc_khz on B will be overrode in the migration from A to B
    before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
    different than the user-specified TSC frequency on B, the
    expectation in 1) will not be satisfied anymore.

So, I introduce the field tsc_khz_saved to migrate TSC frequency and
(in patch 3) let the destination decide if this migrated one will be
used.

And, adding a flag like tsc_freq_requested_by_user in your comments on
patch 3 does not solve the problem.

> 
> > 
> > For the additional QMP feature, will the value of tsc-freq property be
> > env->tsc_khz? If yes, I guess the above change would be fine?
> 
> It may work, but I still don't see the point of duplicating the field
> and duplicating the existing SET_TSC_KHZ code.
>

The reason to duplicate tsc_khz is explained above. The reason to
duplicate of SET_TSC_KHZ code will be in the reply to your comments on
patch 3.

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 v3 3/3] target-i386: load the migrated vcpu's TSC rate

[Haozhong Zhang]

On 11/05/15 14:10, Eduardo Habkost wrote:
> On Mon, Nov 02, 2015 at 05:26:43PM +0800, Haozhong Zhang wrote:
> > Set vcpu's TSC rate to the migrated value if the user does not specify a
> > TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If
> > KVM supports TSC scaling, guest programs will observe TSC increasing in
> > the migrated rate other than the host TSC rate.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  target-i386/kvm.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index aae5e58..2be70df 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs)
> >      int r;
> >  
> >      /*
> > +     * If a TSC rate is migrated and the user does not specify the
> > +     * vcpu's TSC rate on the destination, the migrated TSC rate will
> > +     * be used on the destination after the migration.
> > +     */
> > +    if (env->tsc_khz_saved && !env->tsc_khz) {
> > +        if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) {
> > +            r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved);
> 
> Why are you duplicating the existing KVM_SET_TSC_KHZ code in
> kvm_arch_init_vcpu()?
>

Because they are called in different cases and their behaviors on
failure are different:
 1) KVM_SET_TSC_KHZ in kvm_arch_init_vcpu() is called only when a VM
    is created and a user-specified TSC frequency is given. If it
    fails, QEMU will abort.
 2) KVM_SET_TSC_KHZ in kvm_arch_setup_tsc_khz() is called on the
    destination only when TSC frequency is migrated and no
    user-specified TSC frequency is given. If it fails, QEMU as well
    as the migration will not be aborted.

However, after reading your comment at the end, they really could be
merged.

> > +            if (r < 0) {
> > +                fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> 
> If you want to report errors, please use error_report().
> 
> (But I don't think we want to print those warnings. See below.)
> 
> > +            }
> > +        } else {
> > +            r = -1;
> > +            fprintf(stderr, "KVM doesn't support TSC scaling\n");
> > +        }
> > +        if (r < 0) {
> > +            fprintf(stderr, "Use host TSC frequency instead. "
> 
> Did you mean "Using host TSC frequency instead."?
>

Yes.

> > +                    "Guest TSC may be inaccurate.\n");
> > +        }
> > +    }
> 
> This will make QEMU print a warning every single time when migrating to
> hosts that don't support TSC scaling, even if the source and destination
> hosts already have the same TSC frequency. That means most users will
> see a bogus warning, in today's hardware.
> 
> Maybe it will be acceptable to print a warning if (and only if) we know
> that the host TSC is different from the original TSC frequency.
>

Agree, I should add such a check to avoid bogus warnings.

> Considering that we already have code to handle tsc_khz that prints an
> error, you don't need to duplicate it. You could handle both
> user-provided and migration tsc_khz cases with the same code. With
> something like this:
>

Mostly, but as tsc_khz_saved in patch 2 is really needed, I'll make
some minor changes.

-     if (env->tsc_khz) { /* may be set by the user, or loaded from incoming migration */
+     if (env->tsc_khz || env->tsc_khz_saved) { /* may be set by the user, or loaded from incoming migration */
+         int64_t tgt_tsc_khz = env->tsc_khz ? : env->tsc_khz_saved;
>         r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
-             kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) :
+             kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, tgt_tsc_khz) :
>             -ENOTSUP;
>         if (r < 0) {
>             int64_t cur_freq = kvm_check_extension(KVM_CAP_GET_TSC_KHZ)) ?
>                                kvm_vcpu_ioctl(KVM_GET_TSC_KHZ) :
>                                0;
>             /* If we know the host frequency, print a warning every time
>              * there's a mismatch.
>              * If we don't know the host frequency, print a warning only
>              * if the user asked for a specific TSC frequency.
>              */
-             if ((cur_freq <= 0 && env->tsc_freq_requested_by_user) ||
+             if ((cur_freq <= 0 && env->tsc_khz) ||
-                 (cur_freq > 0 && cur_freq != env->tsc_khz)) {
+                 (cur_freq > 0 && cur_freq != tgt_tsc_khz)) {
>                 error_report("warning: TSC frequency mismatch between VM and host, and TSC scaling unavailable");
-                 if (env->tsc_freq_set_by_user) {
+                 if (env->tsc_khz) {
>                     return r;
>                 }
>             }
>         }
>     }
>

Haozhong

> You will just need a new tsc_freq_requested_by_user field to track if
> the TSC frequency was explicitly requested by the user.
> 
> -- 
> Eduardo

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

[Haozhong Zhang]

On 11/05/15 14:10, Eduardo Habkost wrote:
> On Mon, Nov 02, 2015 at 05:26:43PM +0800, Haozhong Zhang wrote:
> > Set vcpu's TSC rate to the migrated value if the user does not specify a
> > TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If
> > KVM supports TSC scaling, guest programs will observe TSC increasing in
> > the migrated rate other than the host TSC rate.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  target-i386/kvm.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index aae5e58..2be70df 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs)
> >      int r;
> >  
> >      /*
> > +     * If a TSC rate is migrated and the user does not specify the
> > +     * vcpu's TSC rate on the destination, the migrated TSC rate will
> > +     * be used on the destination after the migration.
> > +     */
> > +    if (env->tsc_khz_saved && !env->tsc_khz) {
> > +        if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) {
> > +            r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved);
> 
> Why are you duplicating the existing KVM_SET_TSC_KHZ code in
> kvm_arch_init_vcpu()?
>

Because they are called in different cases and their behaviors on
failure are different:
 1) KVM_SET_TSC_KHZ in kvm_arch_init_vcpu() is called only when a VM
    is created and a user-specified TSC frequency is given. If it
    fails, QEMU will abort.
 2) KVM_SET_TSC_KHZ in kvm_arch_setup_tsc_khz() is called on the
    destination only when TSC frequency is migrated and no
    user-specified TSC frequency is given. If it fails, QEMU as well
    as the migration will not be aborted.

However, after reading your comment at the end, they really could be
merged.

> > +            if (r < 0) {
> > +                fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> 
> If you want to report errors, please use error_report().
> 
> (But I don't think we want to print those warnings. See below.)
> 
> > +            }
> > +        } else {
> > +            r = -1;
> > +            fprintf(stderr, "KVM doesn't support TSC scaling\n");
> > +        }
> > +        if (r < 0) {
> > +            fprintf(stderr, "Use host TSC frequency instead. "
> 
> Did you mean "Using host TSC frequency instead."?
>

Yes.

> > +                    "Guest TSC may be inaccurate.\n");
> > +        }
> > +    }
> 
> This will make QEMU print a warning every single time when migrating to
> hosts that don't support TSC scaling, even if the source and destination
> hosts already have the same TSC frequency. That means most users will
> see a bogus warning, in today's hardware.
> 
> Maybe it will be acceptable to print a warning if (and only if) we know
> that the host TSC is different from the original TSC frequency.
>

Agree, I should add such a check to avoid bogus warnings.

> Considering that we already have code to handle tsc_khz that prints an
> error, you don't need to duplicate it. You could handle both
> user-provided and migration tsc_khz cases with the same code. With
> something like this:
>

Mostly, but as tsc_khz_saved in patch 2 is really needed, I'll make
some minor changes.

-     if (env->tsc_khz) { /* may be set by the user, or loaded from incoming migration */
+     if (env->tsc_khz || env->tsc_khz_saved) { /* may be set by the user, or loaded from incoming migration */
+         int64_t tgt_tsc_khz = env->tsc_khz ? : env->tsc_khz_saved;
>         r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
-             kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) :
+             kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, tgt_tsc_khz) :
>             -ENOTSUP;
>         if (r < 0) {
>             int64_t cur_freq = kvm_check_extension(KVM_CAP_GET_TSC_KHZ)) ?
>                                kvm_vcpu_ioctl(KVM_GET_TSC_KHZ) :
>                                0;
>             /* If we know the host frequency, print a warning every time
>              * there's a mismatch.
>              * If we don't know the host frequency, print a warning only
>              * if the user asked for a specific TSC frequency.
>              */
-             if ((cur_freq <= 0 && env->tsc_freq_requested_by_user) ||
+             if ((cur_freq <= 0 && env->tsc_khz) ||
-                 (cur_freq > 0 && cur_freq != env->tsc_khz)) {
+                 (cur_freq > 0 && cur_freq != tgt_tsc_khz)) {
>                 error_report("warning: TSC frequency mismatch between VM and host, and TSC scaling unavailable");
-                 if (env->tsc_freq_set_by_user) {
+                 if (env->tsc_khz) {
>                     return r;
>                 }
>             }
>         }
>     }
>

Haozhong

> You will just need a new tsc_freq_requested_by_user field to track if
> the TSC frequency was explicitly requested by the user.
> 
> -- 
> Eduardo

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

[Eduardo Habkost]

On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> On 11/05/15 14:05, Eduardo Habkost wrote:
> > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > On 11/04/15 19:42, Eduardo Habkost wrote:
[...]
> > > > > +        env->tsc_khz_saved = r;
> > > > > +    }
> > > > 
> > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > objects.
> > > >
> > > 
> > > It's to avoid overriding env->tsc_khz on the destination in the
> > > migration. I can change this line to
> > >              env->tsc_khz = env->tsc_khz_saved = r;
> > 
> > You are already avoiding overriding env->tsc_khz, because you use
> > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > code, if you could just do this:
> > 
> >     if (!env->tsc_khz) {
> >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> >     }
> >
> 
> Consider an example that we migrate a VM from machine A to machine B
> and then to machine C, and QEMU on machine B is launched with the cpu
> option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> beginning):
>  1) In the migration from B to C, the user-specified TSC frequency by
>     'tsc-freq' on B is expected to be migrated to C. That is, the
>     value of env->tsc_khz on B is migrated.
>  2) If TSC frequency is migrated through env->tsc_khz, then
>     env->tsc_khz on B will be overrode in the migration from A to B
>     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
>     different than the user-specified TSC frequency on B, the
>     expectation in 1) will not be satisfied anymore.

Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
This is not different from changing the CPU model and adding or removing
CPU flags when migrating, which is also incorrect. The command-line
parameters defining the VM must be the same when you migrate.

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> On 11/05/15 14:05, Eduardo Habkost wrote:
> > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > On 11/04/15 19:42, Eduardo Habkost wrote:
[...]
> > > > > +        env->tsc_khz_saved = r;
> > > > > +    }
> > > > 
> > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > objects.
> > > >
> > > 
> > > It's to avoid overriding env->tsc_khz on the destination in the
> > > migration. I can change this line to
> > >              env->tsc_khz = env->tsc_khz_saved = r;
> > 
> > You are already avoiding overriding env->tsc_khz, because you use
> > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > code, if you could just do this:
> > 
> >     if (!env->tsc_khz) {
> >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> >     }
> >
> 
> Consider an example that we migrate a VM from machine A to machine B
> and then to machine C, and QEMU on machine B is launched with the cpu
> option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> beginning):
>  1) In the migration from B to C, the user-specified TSC frequency by
>     'tsc-freq' on B is expected to be migrated to C. That is, the
>     value of env->tsc_khz on B is migrated.
>  2) If TSC frequency is migrated through env->tsc_khz, then
>     env->tsc_khz on B will be overrode in the migration from A to B
>     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
>     different than the user-specified TSC frequency on B, the
>     expectation in 1) will not be satisfied anymore.

Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
This is not different from changing the CPU model and adding or removing
CPU flags when migrating, which is also incorrect. The command-line
parameters defining the VM must be the same when you migrate.

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Nov 06, 2015 at 10:32:44AM +0800, Haozhong Zhang wrote:
> On 11/05/15 14:10, Eduardo Habkost wrote:
> > On Mon, Nov 02, 2015 at 05:26:43PM +0800, Haozhong Zhang wrote:
> > > Set vcpu's TSC rate to the migrated value if the user does not specify a
> > > TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If
> > > KVM supports TSC scaling, guest programs will observe TSC increasing in
> > > the migrated rate other than the host TSC rate.
> > > 
> > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > ---
> > >  target-i386/kvm.c | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > > index aae5e58..2be70df 100644
> > > --- a/target-i386/kvm.c
> > > +++ b/target-i386/kvm.c
> > > @@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs)
> > >      int r;
> > >  
> > >      /*
> > > +     * If a TSC rate is migrated and the user does not specify the
> > > +     * vcpu's TSC rate on the destination, the migrated TSC rate will
> > > +     * be used on the destination after the migration.
> > > +     */
> > > +    if (env->tsc_khz_saved && !env->tsc_khz) {
> > > +        if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) {
> > > +            r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved);
> > 
> > Why are you duplicating the existing KVM_SET_TSC_KHZ code in
> > kvm_arch_init_vcpu()?
> >
> 
> Because they are called in different cases and their behaviors on
> failure are different:
>  1) KVM_SET_TSC_KHZ in kvm_arch_init_vcpu() is called only when a VM
>     is created and a user-specified TSC frequency is given. If it
>     fails, QEMU will abort.
>  2) KVM_SET_TSC_KHZ in kvm_arch_setup_tsc_khz() is called on the
>     destination only when TSC frequency is migrated and no
>     user-specified TSC frequency is given. If it fails, QEMU as well
>     as the migration will not be aborted.
> 
> However, after reading your comment at the end, they really could be
> merged.

Agreed that the expected behavior ou failure is different. But it looks
like we are now on the same page about not duplicating code, with the
code you suggested below. :)

> 
> > > +            if (r < 0) {
> > > +                fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> > 
> > If you want to report errors, please use error_report().
> > 
> > (But I don't think we want to print those warnings. See below.)
> > 
> > > +            }
> > > +        } else {
> > > +            r = -1;
> > > +            fprintf(stderr, "KVM doesn't support TSC scaling\n");
> > > +        }
> > > +        if (r < 0) {
> > > +            fprintf(stderr, "Use host TSC frequency instead. "
> > 
> > Did you mean "Using host TSC frequency instead."?
> >
> 
> Yes.
> 
> > > +                    "Guest TSC may be inaccurate.\n");
> > > +        }
> > > +    }
> > 
> > This will make QEMU print a warning every single time when migrating to
> > hosts that don't support TSC scaling, even if the source and destination
> > hosts already have the same TSC frequency. That means most users will
> > see a bogus warning, in today's hardware.
> > 
> > Maybe it will be acceptable to print a warning if (and only if) we know
> > that the host TSC is different from the original TSC frequency.
> >
> 
> Agree, I should add such a check to avoid bogus warnings.
> 
> > Considering that we already have code to handle tsc_khz that prints an
> > error, you don't need to duplicate it. You could handle both
> > user-provided and migration tsc_khz cases with the same code. With
> > something like this:
> >
> 
> Mostly, but as tsc_khz_saved in patch 2 is really needed, I'll make
> some minor changes.
> 
> -     if (env->tsc_khz) { /* may be set by the user, or loaded from incoming migration */
> +     if (env->tsc_khz || env->tsc_khz_saved) { /* may be set by the user, or loaded from incoming migration */
> +         int64_t tgt_tsc_khz = env->tsc_khz ? : env->tsc_khz_saved;
> >         r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
> -             kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) :
> +             kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, tgt_tsc_khz) :
> >             -ENOTSUP;
> >         if (r < 0) {
> >             int64_t cur_freq = kvm_check_extension(KVM_CAP_GET_TSC_KHZ)) ?
> >                                kvm_vcpu_ioctl(KVM_GET_TSC_KHZ) :
> >                                0;
> >             /* If we know the host frequency, print a warning every time
> >              * there's a mismatch.
> >              * If we don't know the host frequency, print a warning only
> >              * if the user asked for a specific TSC frequency.
> >              */
> -             if ((cur_freq <= 0 && env->tsc_freq_requested_by_user) ||
> +             if ((cur_freq <= 0 && env->tsc_khz) ||
> -                 (cur_freq > 0 && cur_freq != env->tsc_khz)) {
> +                 (cur_freq > 0 && cur_freq != tgt_tsc_khz)) {
> >                 error_report("warning: TSC frequency mismatch between VM and host, and TSC scaling unavailable");
> -                 if (env->tsc_freq_set_by_user) {
> +                 if (env->tsc_khz) {
> >                     return r;
> >                 }
> >             }
> >         }
> >     }
> >

If your assumption that tsc_khz_saved is necessary is correct, the
changes above look good. But I am still not sure it is really needed (we
can continue the discussoin in the other thread).

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Nov 06, 2015 at 10:32:44AM +0800, Haozhong Zhang wrote:
> On 11/05/15 14:10, Eduardo Habkost wrote:
> > On Mon, Nov 02, 2015 at 05:26:43PM +0800, Haozhong Zhang wrote:
> > > Set vcpu's TSC rate to the migrated value if the user does not specify a
> > > TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If
> > > KVM supports TSC scaling, guest programs will observe TSC increasing in
> > > the migrated rate other than the host TSC rate.
> > > 
> > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > ---
> > >  target-i386/kvm.c | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > > index aae5e58..2be70df 100644
> > > --- a/target-i386/kvm.c
> > > +++ b/target-i386/kvm.c
> > > @@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs)
> > >      int r;
> > >  
> > >      /*
> > > +     * If a TSC rate is migrated and the user does not specify the
> > > +     * vcpu's TSC rate on the destination, the migrated TSC rate will
> > > +     * be used on the destination after the migration.
> > > +     */
> > > +    if (env->tsc_khz_saved && !env->tsc_khz) {
> > > +        if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) {
> > > +            r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved);
> > 
> > Why are you duplicating the existing KVM_SET_TSC_KHZ code in
> > kvm_arch_init_vcpu()?
> >
> 
> Because they are called in different cases and their behaviors on
> failure are different:
>  1) KVM_SET_TSC_KHZ in kvm_arch_init_vcpu() is called only when a VM
>     is created and a user-specified TSC frequency is given. If it
>     fails, QEMU will abort.
>  2) KVM_SET_TSC_KHZ in kvm_arch_setup_tsc_khz() is called on the
>     destination only when TSC frequency is migrated and no
>     user-specified TSC frequency is given. If it fails, QEMU as well
>     as the migration will not be aborted.
> 
> However, after reading your comment at the end, they really could be
> merged.

Agreed that the expected behavior ou failure is different. But it looks
like we are now on the same page about not duplicating code, with the
code you suggested below. :)

> 
> > > +            if (r < 0) {
> > > +                fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> > 
> > If you want to report errors, please use error_report().
> > 
> > (But I don't think we want to print those warnings. See below.)
> > 
> > > +            }
> > > +        } else {
> > > +            r = -1;
> > > +            fprintf(stderr, "KVM doesn't support TSC scaling\n");
> > > +        }
> > > +        if (r < 0) {
> > > +            fprintf(stderr, "Use host TSC frequency instead. "
> > 
> > Did you mean "Using host TSC frequency instead."?
> >
> 
> Yes.
> 
> > > +                    "Guest TSC may be inaccurate.\n");
> > > +        }
> > > +    }
> > 
> > This will make QEMU print a warning every single time when migrating to
> > hosts that don't support TSC scaling, even if the source and destination
> > hosts already have the same TSC frequency. That means most users will
> > see a bogus warning, in today's hardware.
> > 
> > Maybe it will be acceptable to print a warning if (and only if) we know
> > that the host TSC is different from the original TSC frequency.
> >
> 
> Agree, I should add such a check to avoid bogus warnings.
> 
> > Considering that we already have code to handle tsc_khz that prints an
> > error, you don't need to duplicate it. You could handle both
> > user-provided and migration tsc_khz cases with the same code. With
> > something like this:
> >
> 
> Mostly, but as tsc_khz_saved in patch 2 is really needed, I'll make
> some minor changes.
> 
> -     if (env->tsc_khz) { /* may be set by the user, or loaded from incoming migration */
> +     if (env->tsc_khz || env->tsc_khz_saved) { /* may be set by the user, or loaded from incoming migration */
> +         int64_t tgt_tsc_khz = env->tsc_khz ? : env->tsc_khz_saved;
> >         r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
> -             kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) :
> +             kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, tgt_tsc_khz) :
> >             -ENOTSUP;
> >         if (r < 0) {
> >             int64_t cur_freq = kvm_check_extension(KVM_CAP_GET_TSC_KHZ)) ?
> >                                kvm_vcpu_ioctl(KVM_GET_TSC_KHZ) :
> >                                0;
> >             /* If we know the host frequency, print a warning every time
> >              * there's a mismatch.
> >              * If we don't know the host frequency, print a warning only
> >              * if the user asked for a specific TSC frequency.
> >              */
> -             if ((cur_freq <= 0 && env->tsc_freq_requested_by_user) ||
> +             if ((cur_freq <= 0 && env->tsc_khz) ||
> -                 (cur_freq > 0 && cur_freq != env->tsc_khz)) {
> +                 (cur_freq > 0 && cur_freq != tgt_tsc_khz)) {
> >                 error_report("warning: TSC frequency mismatch between VM and host, and TSC scaling unavailable");
> -                 if (env->tsc_freq_set_by_user) {
> +                 if (env->tsc_khz) {
> >                     return r;
> >                 }
> >             }
> >         }
> >     }
> >

If your assumption that tsc_khz_saved is necessary is correct, the
changes above look good. But I am still not sure it is really needed (we
can continue the discussoin in the other thread).

-- 
Eduardo

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

[haozhong.zhang]

On 11/06/15 13:12, Eduardo Habkost wrote:
> On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> [...]
> > > > > > +        env->tsc_khz_saved = r;
> > > > > > +    }
> > > > > 
> > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > objects.
> > > > >
> > > > 
> > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > migration. I can change this line to
> > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > 
> > > You are already avoiding overriding env->tsc_khz, because you use
> > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > code, if you could just do this:
> > > 
> > >     if (!env->tsc_khz) {
> > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > >     }
> > >
> > 
> > Consider an example that we migrate a VM from machine A to machine B
> > and then to machine C, and QEMU on machine B is launched with the cpu
> > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > beginning):
> >  1) In the migration from B to C, the user-specified TSC frequency by
> >     'tsc-freq' on B is expected to be migrated to C. That is, the
> >     value of env->tsc_khz on B is migrated.
> >  2) If TSC frequency is migrated through env->tsc_khz, then
> >     env->tsc_khz on B will be overrode in the migration from A to B
> >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> >     different than the user-specified TSC frequency on B, the
> >     expectation in 1) will not be satisfied anymore.
> 
> Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> This is not different from changing the CPU model and adding or removing
> CPU flags when migrating, which is also incorrect. The command-line
> parameters defining the VM must be the same when you migrate.
>

Good to know it's an invalid usage. Then the question is what QEMU is
expected to do for this invalid usage?

 1) Abort the migration? But I find that the current QEMU does not
    abort the migration between different CPU models (e.g. Nehalem and
    Haswell).

 2) Or do not abort the migration and ignore tsc-freq option? If so,
    tsc_khz_saved will be not needed.

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 v3 2/3] target-i386: calculate vcpu's TSC rate to be migrated

[haozhong.zhang]

On 11/06/15 13:12, Eduardo Habkost wrote:
> On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> [...]
> > > > > > +        env->tsc_khz_saved = r;
> > > > > > +    }
> > > > > 
> > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > objects.
> > > > >
> > > > 
> > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > migration. I can change this line to
> > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > 
> > > You are already avoiding overriding env->tsc_khz, because you use
> > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > code, if you could just do this:
> > > 
> > >     if (!env->tsc_khz) {
> > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > >     }
> > >
> > 
> > Consider an example that we migrate a VM from machine A to machine B
> > and then to machine C, and QEMU on machine B is launched with the cpu
> > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > beginning):
> >  1) In the migration from B to C, the user-specified TSC frequency by
> >     'tsc-freq' on B is expected to be migrated to C. That is, the
> >     value of env->tsc_khz on B is migrated.
> >  2) If TSC frequency is migrated through env->tsc_khz, then
> >     env->tsc_khz on B will be overrode in the migration from A to B
> >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> >     different than the user-specified TSC frequency on B, the
> >     expectation in 1) will not be satisfied anymore.
> 
> Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> This is not different from changing the CPU model and adding or removing
> CPU flags when migrating, which is also incorrect. The command-line
> parameters defining the VM must be the same when you migrate.
>

Good to know it's an invalid usage. Then the question is what QEMU is
expected to do for this invalid usage?

 1) Abort the migration? But I find that the current QEMU does not
    abort the migration between different CPU models (e.g. Nehalem and
    Haswell).

 2) Or do not abort the migration and ignore tsc-freq option? If so,
    tsc_khz_saved will be not needed.

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 v3 2/3] target-i386: calculate vcpu's TSC rate to be migrated

[Eduardo Habkost]

On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> On 11/06/15 13:12, Eduardo Habkost wrote:
> > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > [...]
> > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > +    }
> > > > > > 
> > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > objects.
> > > > > >
> > > > > 
> > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > migration. I can change this line to
> > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > 
> > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > code, if you could just do this:
> > > > 
> > > >     if (!env->tsc_khz) {
> > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > >     }
> > > >
> > > 
> > > Consider an example that we migrate a VM from machine A to machine B
> > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > beginning):
> > >  1) In the migration from B to C, the user-specified TSC frequency by
> > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > >     value of env->tsc_khz on B is migrated.
> > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > >     env->tsc_khz on B will be overrode in the migration from A to B
> > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > >     different than the user-specified TSC frequency on B, the
> > >     expectation in 1) will not be satisfied anymore.
> > 
> > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > This is not different from changing the CPU model and adding or removing
> > CPU flags when migrating, which is also incorrect. The command-line
> > parameters defining the VM must be the same when you migrate.
> >
> 
> Good to know it's an invalid usage. Then the question is what QEMU is
> expected to do for this invalid usage?
> 
>  1) Abort the migration? But I find that the current QEMU does not
>     abort the migration between different CPU models (e.g. Nehalem and
>     Haswell).
> 
>  2) Or do not abort the migration and ignore tsc-freq option? If so,
>     tsc_khz_saved will be not needed.

My first choice is to abort migration. If we decide to abort today and
find it to cause problems, we can easily fix it. If we decide to
continue without aborting, it is difficult to change that behavior
without breaking existing setups.

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> On 11/06/15 13:12, Eduardo Habkost wrote:
> > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > [...]
> > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > +    }
> > > > > > 
> > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > objects.
> > > > > >
> > > > > 
> > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > migration. I can change this line to
> > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > 
> > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > code, if you could just do this:
> > > > 
> > > >     if (!env->tsc_khz) {
> > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > >     }
> > > >
> > > 
> > > Consider an example that we migrate a VM from machine A to machine B
> > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > beginning):
> > >  1) In the migration from B to C, the user-specified TSC frequency by
> > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > >     value of env->tsc_khz on B is migrated.
> > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > >     env->tsc_khz on B will be overrode in the migration from A to B
> > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > >     different than the user-specified TSC frequency on B, the
> > >     expectation in 1) will not be satisfied anymore.
> > 
> > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > This is not different from changing the CPU model and adding or removing
> > CPU flags when migrating, which is also incorrect. The command-line
> > parameters defining the VM must be the same when you migrate.
> >
> 
> Good to know it's an invalid usage. Then the question is what QEMU is
> expected to do for this invalid usage?
> 
>  1) Abort the migration? But I find that the current QEMU does not
>     abort the migration between different CPU models (e.g. Nehalem and
>     Haswell).
> 
>  2) Or do not abort the migration and ignore tsc-freq option? If so,
>     tsc_khz_saved will be not needed.

My first choice is to abort migration. If we decide to abort today and
find it to cause problems, we can easily fix it. If we decide to
continue without aborting, it is difficult to change that behavior
without breaking existing setups.

-- 
Eduardo

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

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > [...]
> > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > +    }
> > > > > > > 
> > > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > > objects.
> > > > > > >
> > > > > > 
> > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > migration. I can change this line to
> > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > 
> > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > > code, if you could just do this:
> > > > > 
> > > > >     if (!env->tsc_khz) {
> > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > >     }
> > > > >
> > > > 
> > > > Consider an example that we migrate a VM from machine A to machine B
> > > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > beginning):
> > > >  1) In the migration from B to C, the user-specified TSC frequency by
> > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > >     value of env->tsc_khz on B is migrated.
> > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > >     different than the user-specified TSC frequency on B, the
> > > >     expectation in 1) will not be satisfied anymore.
> > > 
> > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > This is not different from changing the CPU model and adding or removing
> > > CPU flags when migrating, which is also incorrect. The command-line
> > > parameters defining the VM must be the same when you migrate.
> > >
> > 
> > Good to know it's an invalid usage. Then the question is what QEMU is
> > expected to do for this invalid usage?
> > 
> >  1) Abort the migration? But I find that the current QEMU does not
> >     abort the migration between different CPU models (e.g. Nehalem and
> >     Haswell).
> > 
> >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> >     tsc_khz_saved will be not needed.
> 
> My first choice is to abort migration. If we decide to abort today and
> find it to cause problems, we can easily fix it. If we decide to
> continue without aborting, it is difficult to change that behavior
> without breaking existing setups.

Yes, if it's a bad config please abort the migration and put a clear
message in the log so we can tell easily.

Dave

> 
> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > [...]
> > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > +    }
> > > > > > > 
> > > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > > objects.
> > > > > > >
> > > > > > 
> > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > migration. I can change this line to
> > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > 
> > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > > code, if you could just do this:
> > > > > 
> > > > >     if (!env->tsc_khz) {
> > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > >     }
> > > > >
> > > > 
> > > > Consider an example that we migrate a VM from machine A to machine B
> > > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > beginning):
> > > >  1) In the migration from B to C, the user-specified TSC frequency by
> > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > >     value of env->tsc_khz on B is migrated.
> > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > >     different than the user-specified TSC frequency on B, the
> > > >     expectation in 1) will not be satisfied anymore.
> > > 
> > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > This is not different from changing the CPU model and adding or removing
> > > CPU flags when migrating, which is also incorrect. The command-line
> > > parameters defining the VM must be the same when you migrate.
> > >
> > 
> > Good to know it's an invalid usage. Then the question is what QEMU is
> > expected to do for this invalid usage?
> > 
> >  1) Abort the migration? But I find that the current QEMU does not
> >     abort the migration between different CPU models (e.g. Nehalem and
> >     Haswell).
> > 
> >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> >     tsc_khz_saved will be not needed.
> 
> My first choice is to abort migration. If we decide to abort today and
> find it to cause problems, we can easily fix it. If we decide to
> continue without aborting, it is difficult to change that behavior
> without breaking existing setups.

Yes, if it's a bad config please abort the migration and put a clear
message in the log so we can tell easily.

Dave

> 
> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Haozhong Zhang]

On 11/09/15 14:01, Eduardo Habkost wrote:
> On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > [...]
> > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > +    }
> > > > > > > 
> > > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > > objects.
> > > > > > >
> > > > > > 
> > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > migration. I can change this line to
> > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > 
> > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > > code, if you could just do this:
> > > > > 
> > > > >     if (!env->tsc_khz) {
> > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > >     }
> > > > >
> > > > 
> > > > Consider an example that we migrate a VM from machine A to machine B
> > > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > beginning):
> > > >  1) In the migration from B to C, the user-specified TSC frequency by
> > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > >     value of env->tsc_khz on B is migrated.
> > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > >     different than the user-specified TSC frequency on B, the
> > > >     expectation in 1) will not be satisfied anymore.
> > > 
> > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > This is not different from changing the CPU model and adding or removing
> > > CPU flags when migrating, which is also incorrect. The command-line
> > > parameters defining the VM must be the same when you migrate.
> > >
> > 
> > Good to know it's an invalid usage. Then the question is what QEMU is
> > expected to do for this invalid usage?
> > 
> >  1) Abort the migration? But I find that the current QEMU does not
> >     abort the migration between different CPU models (e.g. Nehalem and
> >     Haswell).
> > 
> >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> >     tsc_khz_saved will be not needed.
> 
> My first choice is to abort migration. If we decide to abort today and
> find it to cause problems, we can easily fix it. If we decide to
> continue without aborting, it is difficult to change that behavior
> without breaking existing setups.
> 
> -- 
> Eduardo

Agree, but I would like to relax the abort condition to "abort the
migration only if QEMU on the destination uses a different TSC
frequency than the migrated one" so that the following usages would be
still valid:
 1) Only QEMU on the destination has 'tsc-freq' option, but it' set to
    the same value of the migrated one.
 2) Only QEMU on the source has 'tsc-freq' option.
 3) QEMU on both sides have 'tsc-freq' option, but they are set to the
    same value.
In all above usages, TSC frequency on the destination is the same as
both the value on the source and the value explicitly expected by
users on the destination (by 'tsc-freq' on the destination).

And I still need tsc_khz_saved to tell on the destination whether
 1) both tsc-freq option and migrated TSC frequency are present, and
 2) above two values are the same.
Even though we restrictively requires QEMU on both sides use the same
CPU options, tsc_khz_saved is still needed because of 1).

Haozhong

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

[Haozhong Zhang]

On 11/09/15 14:01, Eduardo Habkost wrote:
> On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > [...]
> > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > +    }
> > > > > > > 
> > > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > > objects.
> > > > > > >
> > > > > > 
> > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > migration. I can change this line to
> > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > 
> > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > > code, if you could just do this:
> > > > > 
> > > > >     if (!env->tsc_khz) {
> > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > >     }
> > > > >
> > > > 
> > > > Consider an example that we migrate a VM from machine A to machine B
> > > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > beginning):
> > > >  1) In the migration from B to C, the user-specified TSC frequency by
> > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > >     value of env->tsc_khz on B is migrated.
> > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > >     different than the user-specified TSC frequency on B, the
> > > >     expectation in 1) will not be satisfied anymore.
> > > 
> > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > This is not different from changing the CPU model and adding or removing
> > > CPU flags when migrating, which is also incorrect. The command-line
> > > parameters defining the VM must be the same when you migrate.
> > >
> > 
> > Good to know it's an invalid usage. Then the question is what QEMU is
> > expected to do for this invalid usage?
> > 
> >  1) Abort the migration? But I find that the current QEMU does not
> >     abort the migration between different CPU models (e.g. Nehalem and
> >     Haswell).
> > 
> >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> >     tsc_khz_saved will be not needed.
> 
> My first choice is to abort migration. If we decide to abort today and
> find it to cause problems, we can easily fix it. If we decide to
> continue without aborting, it is difficult to change that behavior
> without breaking existing setups.
> 
> -- 
> Eduardo

Agree, but I would like to relax the abort condition to "abort the
migration only if QEMU on the destination uses a different TSC
frequency than the migrated one" so that the following usages would be
still valid:
 1) Only QEMU on the destination has 'tsc-freq' option, but it' set to
    the same value of the migrated one.
 2) Only QEMU on the source has 'tsc-freq' option.
 3) QEMU on both sides have 'tsc-freq' option, but they are set to the
    same value.
In all above usages, TSC frequency on the destination is the same as
both the value on the source and the value explicitly expected by
users on the destination (by 'tsc-freq' on the destination).

And I still need tsc_khz_saved to tell on the destination whether
 1) both tsc-freq option and migrated TSC frequency are present, and
 2) above two values are the same.
Even though we restrictively requires QEMU on both sides use the same
CPU options, tsc_khz_saved is still needed because of 1).

Haozhong

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

[Haozhong Zhang]

On 11/09/15 14:01, Eduardo Habkost wrote:
> On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > [...]
> > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > +    }
> > > > > > > 
> > > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > > objects.
> > > > > > >
> > > > > > 
> > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > migration. I can change this line to
> > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > 
> > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > > code, if you could just do this:
> > > > > 
> > > > >     if (!env->tsc_khz) {
> > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > >     }
> > > > >
> > > > 
> > > > Consider an example that we migrate a VM from machine A to machine B
> > > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > beginning):
> > > >  1) In the migration from B to C, the user-specified TSC frequency by
> > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > >     value of env->tsc_khz on B is migrated.
> > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > >     different than the user-specified TSC frequency on B, the
> > > >     expectation in 1) will not be satisfied anymore.
> > > 
> > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > This is not different from changing the CPU model and adding or removing
> > > CPU flags when migrating, which is also incorrect. The command-line
> > > parameters defining the VM must be the same when you migrate.
> > >
> > 
> > Good to know it's an invalid usage. Then the question is what QEMU is
> > expected to do for this invalid usage?
> > 
> >  1) Abort the migration? But I find that the current QEMU does not
> >     abort the migration between different CPU models (e.g. Nehalem and
> >     Haswell).
> > 
> >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> >     tsc_khz_saved will be not needed.
> 
> My first choice is to abort migration. If we decide to abort today and
> find it to cause problems, we can easily fix it. If we decide to
> continue without aborting, it is difficult to change that behavior
> without breaking existing setups.
>

Two additional questions:

 1) Existing QEMU allows 'tsc-freq' on the destination in the
    migration. If we decided to abort when both 'tsc-freq' and
    migrated TSC were present on the destination, it would break some
    existing usages. Considering backward compatibility, would above
    choice 2) be better?

 2) If we do decide to abort, could I use abort()? Or are there other
    clean approaches to abort?

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 v3 2/3] target-i386: calculate vcpu's TSC rate to be migrated

[Haozhong Zhang]

On 11/09/15 14:01, Eduardo Habkost wrote:
> On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > [...]
> > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > +    }
> > > > > > > 
> > > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > > objects.
> > > > > > >
> > > > > > 
> > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > migration. I can change this line to
> > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > 
> > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > > code, if you could just do this:
> > > > > 
> > > > >     if (!env->tsc_khz) {
> > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > >     }
> > > > >
> > > > 
> > > > Consider an example that we migrate a VM from machine A to machine B
> > > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > beginning):
> > > >  1) In the migration from B to C, the user-specified TSC frequency by
> > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > >     value of env->tsc_khz on B is migrated.
> > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > >     different than the user-specified TSC frequency on B, the
> > > >     expectation in 1) will not be satisfied anymore.
> > > 
> > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > This is not different from changing the CPU model and adding or removing
> > > CPU flags when migrating, which is also incorrect. The command-line
> > > parameters defining the VM must be the same when you migrate.
> > >
> > 
> > Good to know it's an invalid usage. Then the question is what QEMU is
> > expected to do for this invalid usage?
> > 
> >  1) Abort the migration? But I find that the current QEMU does not
> >     abort the migration between different CPU models (e.g. Nehalem and
> >     Haswell).
> > 
> >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> >     tsc_khz_saved will be not needed.
> 
> My first choice is to abort migration. If we decide to abort today and
> find it to cause problems, we can easily fix it. If we decide to
> continue without aborting, it is difficult to change that behavior
> without breaking existing setups.
>

Two additional questions:

 1) Existing QEMU allows 'tsc-freq' on the destination in the
    migration. If we decided to abort when both 'tsc-freq' and
    migrated TSC were present on the destination, it would break some
    existing usages. Considering backward compatibility, would above
    choice 2) be better?

 2) If we do decide to abort, could I use abort()? Or are there other
    clean approaches to abort?

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 v3 1/3] target-i386: add a subsection for migrating vcpu's TSC rate

[Eduardo Habkost]

On Mon, Nov 02, 2015 at 05:26:41PM +0800, Haozhong Zhang wrote:
> A new subsection 'vmstate_tsc_khz' is added to migrate vcpu's TSC
> rate. For the backwards compatibility, this subsection is not migrated
> on pc-*-2.4 and older machine types.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  hw/i386/pc.c          |  1 +
>  hw/i386/pc_piix.c     |  1 +
>  hw/i386/pc_q35.c      |  1 +
>  include/hw/i386/pc.h  |  1 +
>  target-i386/cpu.h     |  1 +
>  target-i386/machine.c | 21 +++++++++++++++++++++
>  6 files changed, 26 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 0cb8afd..2f2fc93 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1952,6 +1952,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>  
>      pcmc->get_hotplug_handler = mc->get_hotplug_handler;
> +    pcmc->save_tsc_khz = true;
>      mc->get_hotplug_handler = pc_get_hotpug_handler;
>      mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
>      mc->default_boot_order = "cad";
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 393dcc4..fc71321 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -487,6 +487,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
>      m->alias = NULL;
>      m->is_default = 0;
>      pcmc->broken_reserved_end = true;
> +    pcmc->save_tsc_khz = false;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>  }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 2f8f396..858ed69 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>      pc_q35_2_5_machine_options(m);
>      m->alias = NULL;
>      pcmc->broken_reserved_end = true;
> +    pcmc->save_tsc_khz = false;

I had suggested the PCMachineClass field, but now I've been thinking:
all other fields related to tsc_khz are in X86CPU, so I believe this
belongs to X86CPU too. It could be a simple X86CPU property set by
PC_COMPAT_2_4.

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Nov 02, 2015 at 05:26:41PM +0800, Haozhong Zhang wrote:
> A new subsection 'vmstate_tsc_khz' is added to migrate vcpu's TSC
> rate. For the backwards compatibility, this subsection is not migrated
> on pc-*-2.4 and older machine types.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  hw/i386/pc.c          |  1 +
>  hw/i386/pc_piix.c     |  1 +
>  hw/i386/pc_q35.c      |  1 +
>  include/hw/i386/pc.h  |  1 +
>  target-i386/cpu.h     |  1 +
>  target-i386/machine.c | 21 +++++++++++++++++++++
>  6 files changed, 26 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 0cb8afd..2f2fc93 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1952,6 +1952,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>  
>      pcmc->get_hotplug_handler = mc->get_hotplug_handler;
> +    pcmc->save_tsc_khz = true;
>      mc->get_hotplug_handler = pc_get_hotpug_handler;
>      mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
>      mc->default_boot_order = "cad";
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 393dcc4..fc71321 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -487,6 +487,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
>      m->alias = NULL;
>      m->is_default = 0;
>      pcmc->broken_reserved_end = true;
> +    pcmc->save_tsc_khz = false;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>  }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 2f8f396..858ed69 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>      pc_q35_2_5_machine_options(m);
>      m->alias = NULL;
>      pcmc->broken_reserved_end = true;
> +    pcmc->save_tsc_khz = false;

I had suggested the PCMachineClass field, but now I've been thinking:
all other fields related to tsc_khz are in X86CPU, so I believe this
belongs to X86CPU too. It could be a simple X86CPU property set by
PC_COMPAT_2_4.

-- 
Eduardo

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

[Haozhong Zhang]

On 11/11/15 12:16, Eduardo Habkost wrote:
> On Mon, Nov 02, 2015 at 05:26:41PM +0800, Haozhong Zhang wrote:
> > A new subsection 'vmstate_tsc_khz' is added to migrate vcpu's TSC
> > rate. For the backwards compatibility, this subsection is not migrated
> > on pc-*-2.4 and older machine types.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  hw/i386/pc.c          |  1 +
> >  hw/i386/pc_piix.c     |  1 +
> >  hw/i386/pc_q35.c      |  1 +
> >  include/hw/i386/pc.h  |  1 +
> >  target-i386/cpu.h     |  1 +
> >  target-i386/machine.c | 21 +++++++++++++++++++++
> >  6 files changed, 26 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 0cb8afd..2f2fc93 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1952,6 +1952,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> >      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> >  
> >      pcmc->get_hotplug_handler = mc->get_hotplug_handler;
> > +    pcmc->save_tsc_khz = true;
> >      mc->get_hotplug_handler = pc_get_hotpug_handler;
> >      mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
> >      mc->default_boot_order = "cad";
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 393dcc4..fc71321 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -487,6 +487,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
> >      m->alias = NULL;
> >      m->is_default = 0;
> >      pcmc->broken_reserved_end = true;
> > +    pcmc->save_tsc_khz = false;
> >      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> >  }
> >  
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 2f8f396..858ed69 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> >      pc_q35_2_5_machine_options(m);
> >      m->alias = NULL;
> >      pcmc->broken_reserved_end = true;
> > +    pcmc->save_tsc_khz = false;
> 
> I had suggested the PCMachineClass field, but now I've been thinking:
> all other fields related to tsc_khz are in X86CPU, so I believe this
> belongs to X86CPU too. It could be a simple X86CPU property set by
> PC_COMPAT_2_4.
>

Reasonable, will update in the next version.

Thanks,
Haozhong

> -- 
> Eduardo

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

[Haozhong Zhang]

On 11/11/15 12:16, Eduardo Habkost wrote:
> On Mon, Nov 02, 2015 at 05:26:41PM +0800, Haozhong Zhang wrote:
> > A new subsection 'vmstate_tsc_khz' is added to migrate vcpu's TSC
> > rate. For the backwards compatibility, this subsection is not migrated
> > on pc-*-2.4 and older machine types.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  hw/i386/pc.c          |  1 +
> >  hw/i386/pc_piix.c     |  1 +
> >  hw/i386/pc_q35.c      |  1 +
> >  include/hw/i386/pc.h  |  1 +
> >  target-i386/cpu.h     |  1 +
> >  target-i386/machine.c | 21 +++++++++++++++++++++
> >  6 files changed, 26 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 0cb8afd..2f2fc93 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1952,6 +1952,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> >      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> >  
> >      pcmc->get_hotplug_handler = mc->get_hotplug_handler;
> > +    pcmc->save_tsc_khz = true;
> >      mc->get_hotplug_handler = pc_get_hotpug_handler;
> >      mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
> >      mc->default_boot_order = "cad";
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 393dcc4..fc71321 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -487,6 +487,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
> >      m->alias = NULL;
> >      m->is_default = 0;
> >      pcmc->broken_reserved_end = true;
> > +    pcmc->save_tsc_khz = false;
> >      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> >  }
> >  
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 2f8f396..858ed69 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> >      pc_q35_2_5_machine_options(m);
> >      m->alias = NULL;
> >      pcmc->broken_reserved_end = true;
> > +    pcmc->save_tsc_khz = false;
> 
> I had suggested the PCMachineClass field, but now I've been thinking:
> all other fields related to tsc_khz are in X86CPU, so I believe this
> belongs to X86CPU too. It could be a simple X86CPU property set by
> PC_COMPAT_2_4.
>

Reasonable, will update in the next version.

Thanks,
Haozhong

> -- 
> Eduardo

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

[Eduardo Habkost]

On Tue, Nov 10, 2015 at 09:08:58AM +0800, Haozhong Zhang wrote:
> On 11/09/15 14:01, Eduardo Habkost wrote:
> > On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> > > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > > [...]
> > > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > > +    }
> > > > > > > > 
> > > > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > > > objects.
> > > > > > > >
> > > > > > > 
> > > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > > migration. I can change this line to
> > > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > > 
> > > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > > > code, if you could just do this:
> > > > > > 
> > > > > >     if (!env->tsc_khz) {
> > > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > > >     }
> > > > > >
> > > > > 
> > > > > Consider an example that we migrate a VM from machine A to machine B
> > > > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > > beginning):
> > > > >  1) In the migration from B to C, the user-specified TSC frequency by
> > > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > > >     value of env->tsc_khz on B is migrated.
> > > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > > >     different than the user-specified TSC frequency on B, the
> > > > >     expectation in 1) will not be satisfied anymore.
> > > > 
> > > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > > This is not different from changing the CPU model and adding or removing
> > > > CPU flags when migrating, which is also incorrect. The command-line
> > > > parameters defining the VM must be the same when you migrate.
> > > >
> > > 
> > > Good to know it's an invalid usage. Then the question is what QEMU is
> > > expected to do for this invalid usage?
> > > 
> > >  1) Abort the migration? But I find that the current QEMU does not
> > >     abort the migration between different CPU models (e.g. Nehalem and
> > >     Haswell).
> > > 
> > >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> > >     tsc_khz_saved will be not needed.
> > 
> > My first choice is to abort migration. If we decide to abort today and
> > find it to cause problems, we can easily fix it. If we decide to
> > continue without aborting, it is difficult to change that behavior
> > without breaking existing setups.
> 
> Agree, but I would like to relax the abort condition to "abort the
> migration only if QEMU on the destination uses a different TSC
> frequency than the migrated one" so that the following usages would be
> still valid:
>  1) Only QEMU on the destination has 'tsc-freq' option, but it' set to
>     the same value of the migrated one.
>  2) Only QEMU on the source has 'tsc-freq' option.
>  3) QEMU on both sides have 'tsc-freq' option, but they are set to the
>     same value.
> In all above usages, TSC frequency on the destination is the same as
> both the value on the source and the value explicitly expected by
> users on the destination (by 'tsc-freq' on the destination).

Yes, that's probably all we can do because we don't really know
the command-line on the source. All we know is that the user is
asking for a TSC frequency that doesn't match what we see in the
migration stream.

> 
> And I still need tsc_khz_saved to tell on the destination whether
>  1) both tsc-freq option and migrated TSC frequency are present, and
>  2) above two values are the same.
> Even though we restrictively requires QEMU on both sides use the same
> CPU options, tsc_khz_saved is still needed because of 1).
> 

So, it looks like we need an extra field because of two things:
the migration sanity check, and SET_TSC_KHZ error handling. But
what bothers me is that we have two redundant fields that could
contradict each other, and it is not clear which one we should
use on which case. Sometimes only tsc_khz is valid, sometimes
only tsc_khz is valid.

What if we set/use tsc_khz on all code (no exceptions), but add a
new field just to indicate if tsc-freq was set by the user?
Something like:

* SET_TSC_KHZ code uses tsc_khz only;
* "tsc-freq" property getter returns tsc_khz;
* VMState saves/loads tsc_khz only;
* Initialization code set tsc_khz = ioctl(KVM_GET_TSC_KHZ) in
  case tsc_khz is not set.

And a new user_tsc_khz field would be added and used only for
error checking:
* "tsc-freq" property setter changes both tsc_khz and
  user_tsc_khz;
* Sanity check code post migration-load ensures that
  (!user_tsc_khz || tsc_khz == user_tsc_khz);
* SET_TSC_KHZ error handling returns error if user_tsc_khz
  is set.

-- 
Eduardo

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

[Eduardo Habkost]

On Tue, Nov 10, 2015 at 09:08:58AM +0800, Haozhong Zhang wrote:
> On 11/09/15 14:01, Eduardo Habkost wrote:
> > On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> > > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > > [...]
> > > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > > +    }
> > > > > > > > 
> > > > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > > > objects.
> > > > > > > >
> > > > > > > 
> > > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > > migration. I can change this line to
> > > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > > 
> > > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > > > code, if you could just do this:
> > > > > > 
> > > > > >     if (!env->tsc_khz) {
> > > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > > >     }
> > > > > >
> > > > > 
> > > > > Consider an example that we migrate a VM from machine A to machine B
> > > > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > > beginning):
> > > > >  1) In the migration from B to C, the user-specified TSC frequency by
> > > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > > >     value of env->tsc_khz on B is migrated.
> > > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > > >     different than the user-specified TSC frequency on B, the
> > > > >     expectation in 1) will not be satisfied anymore.
> > > > 
> > > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > > This is not different from changing the CPU model and adding or removing
> > > > CPU flags when migrating, which is also incorrect. The command-line
> > > > parameters defining the VM must be the same when you migrate.
> > > >
> > > 
> > > Good to know it's an invalid usage. Then the question is what QEMU is
> > > expected to do for this invalid usage?
> > > 
> > >  1) Abort the migration? But I find that the current QEMU does not
> > >     abort the migration between different CPU models (e.g. Nehalem and
> > >     Haswell).
> > > 
> > >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> > >     tsc_khz_saved will be not needed.
> > 
> > My first choice is to abort migration. If we decide to abort today and
> > find it to cause problems, we can easily fix it. If we decide to
> > continue without aborting, it is difficult to change that behavior
> > without breaking existing setups.
> 
> Agree, but I would like to relax the abort condition to "abort the
> migration only if QEMU on the destination uses a different TSC
> frequency than the migrated one" so that the following usages would be
> still valid:
>  1) Only QEMU on the destination has 'tsc-freq' option, but it' set to
>     the same value of the migrated one.
>  2) Only QEMU on the source has 'tsc-freq' option.
>  3) QEMU on both sides have 'tsc-freq' option, but they are set to the
>     same value.
> In all above usages, TSC frequency on the destination is the same as
> both the value on the source and the value explicitly expected by
> users on the destination (by 'tsc-freq' on the destination).

Yes, that's probably all we can do because we don't really know
the command-line on the source. All we know is that the user is
asking for a TSC frequency that doesn't match what we see in the
migration stream.

> 
> And I still need tsc_khz_saved to tell on the destination whether
>  1) both tsc-freq option and migrated TSC frequency are present, and
>  2) above two values are the same.
> Even though we restrictively requires QEMU on both sides use the same
> CPU options, tsc_khz_saved is still needed because of 1).
> 

So, it looks like we need an extra field because of two things:
the migration sanity check, and SET_TSC_KHZ error handling. But
what bothers me is that we have two redundant fields that could
contradict each other, and it is not clear which one we should
use on which case. Sometimes only tsc_khz is valid, sometimes
only tsc_khz is valid.

What if we set/use tsc_khz on all code (no exceptions), but add a
new field just to indicate if tsc-freq was set by the user?
Something like:

* SET_TSC_KHZ code uses tsc_khz only;
* "tsc-freq" property getter returns tsc_khz;
* VMState saves/loads tsc_khz only;
* Initialization code set tsc_khz = ioctl(KVM_GET_TSC_KHZ) in
  case tsc_khz is not set.

And a new user_tsc_khz field would be added and used only for
error checking:
* "tsc-freq" property setter changes both tsc_khz and
  user_tsc_khz;
* Sanity check code post migration-load ensures that
  (!user_tsc_khz || tsc_khz == user_tsc_khz);
* SET_TSC_KHZ error handling returns error if user_tsc_khz
  is set.

-- 
Eduardo

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

[Eduardo Habkost]

On Wed, Nov 11, 2015 at 12:57:44AM +0800, Haozhong Zhang wrote:
> On 11/09/15 14:01, Eduardo Habkost wrote:
> > On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> > > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > > [...]
> > > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > > +    }
> > > > > > > > 
> > > > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > > > objects.
> > > > > > > >
> > > > > > > 
> > > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > > migration. I can change this line to
> > > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > > 
> > > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > > > code, if you could just do this:
> > > > > > 
> > > > > >     if (!env->tsc_khz) {
> > > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > > >     }
> > > > > >
> > > > > 
> > > > > Consider an example that we migrate a VM from machine A to machine B
> > > > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > > beginning):
> > > > >  1) In the migration from B to C, the user-specified TSC frequency by
> > > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > > >     value of env->tsc_khz on B is migrated.
> > > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > > >     different than the user-specified TSC frequency on B, the
> > > > >     expectation in 1) will not be satisfied anymore.
> > > > 
> > > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > > This is not different from changing the CPU model and adding or removing
> > > > CPU flags when migrating, which is also incorrect. The command-line
> > > > parameters defining the VM must be the same when you migrate.
> > > >
> > > 
> > > Good to know it's an invalid usage. Then the question is what QEMU is
> > > expected to do for this invalid usage?
> > > 
> > >  1) Abort the migration? But I find that the current QEMU does not
> > >     abort the migration between different CPU models (e.g. Nehalem and
> > >     Haswell).
> > > 
> > >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> > >     tsc_khz_saved will be not needed.
> > 
> > My first choice is to abort migration. If we decide to abort today and
> > find it to cause problems, we can easily fix it. If we decide to
> > continue without aborting, it is difficult to change that behavior
> > without breaking existing setups.
> >
> 
> Two additional questions:
> 
>  1) Existing QEMU allows 'tsc-freq' on the destination in the
>     migration. If we decided to abort when both 'tsc-freq' and
>     migrated TSC were present on the destination, it would break some
>     existing usages. Considering backward compatibility, would above
>     choice 2) be better?

We shouldn't abort simply because the section is present and tsc-freq is
set (because we will always send the section in the newer
machine-types). We should abort only when we know that the command-line
contradicts what we see in the migration stream.

> 
>  2) If we do decide to abort, could I use abort()? Or are there other
>     clean approaches to abort?

You don't need to abort QEMU. You just need to tell the migration code
that migration can't continue. The exact way to do it depends on where
you are hooking the sanity check code. If you use a
VMStateDescription.post_load hook, you can use error_report() and return
a negative errno value. CCing Quintela in case he has suggestions.

-- 
Eduardo

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

[Eduardo Habkost]

On Wed, Nov 11, 2015 at 12:57:44AM +0800, Haozhong Zhang wrote:
> On 11/09/15 14:01, Eduardo Habkost wrote:
> > On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> > > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > > [...]
> > > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > > +    }
> > > > > > > > 
> > > > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > > > objects.
> > > > > > > >
> > > > > > > 
> > > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > > migration. I can change this line to
> > > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > > 
> > > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > > > code, if you could just do this:
> > > > > > 
> > > > > >     if (!env->tsc_khz) {
> > > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > > >     }
> > > > > >
> > > > > 
> > > > > Consider an example that we migrate a VM from machine A to machine B
> > > > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > > beginning):
> > > > >  1) In the migration from B to C, the user-specified TSC frequency by
> > > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > > >     value of env->tsc_khz on B is migrated.
> > > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > > >     different than the user-specified TSC frequency on B, the
> > > > >     expectation in 1) will not be satisfied anymore.
> > > > 
> > > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > > This is not different from changing the CPU model and adding or removing
> > > > CPU flags when migrating, which is also incorrect. The command-line
> > > > parameters defining the VM must be the same when you migrate.
> > > >
> > > 
> > > Good to know it's an invalid usage. Then the question is what QEMU is
> > > expected to do for this invalid usage?
> > > 
> > >  1) Abort the migration? But I find that the current QEMU does not
> > >     abort the migration between different CPU models (e.g. Nehalem and
> > >     Haswell).
> > > 
> > >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> > >     tsc_khz_saved will be not needed.
> > 
> > My first choice is to abort migration. If we decide to abort today and
> > find it to cause problems, we can easily fix it. If we decide to
> > continue without aborting, it is difficult to change that behavior
> > without breaking existing setups.
> >
> 
> Two additional questions:
> 
>  1) Existing QEMU allows 'tsc-freq' on the destination in the
>     migration. If we decided to abort when both 'tsc-freq' and
>     migrated TSC were present on the destination, it would break some
>     existing usages. Considering backward compatibility, would above
>     choice 2) be better?

We shouldn't abort simply because the section is present and tsc-freq is
set (because we will always send the section in the newer
machine-types). We should abort only when we know that the command-line
contradicts what we see in the migration stream.

> 
>  2) If we do decide to abort, could I use abort()? Or are there other
>     clean approaches to abort?

You don't need to abort QEMU. You just need to tell the migration code
that migration can't continue. The exact way to do it depends on where
you are hooking the sanity check code. If you use a
VMStateDescription.post_load hook, you can use error_report() and return
a negative errno value. CCing Quintela in case he has suggestions.

-- 
Eduardo

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

[Haozhong Zhang]

On 11/11/15 13:23, Eduardo Habkost wrote:
> On Wed, Nov 11, 2015 at 12:57:44AM +0800, Haozhong Zhang wrote:
> > On 11/09/15 14:01, Eduardo Habkost wrote:
> > > On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> > > > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > > > [...]
> > > > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > > > +    }
> > > > > > > > > 
> > > > > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > > > > objects.
> > > > > > > > >
> > > > > > > > 
> > > > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > > > migration. I can change this line to
> > > > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > > > 
> > > > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > > > > code, if you could just do this:
> > > > > > > 
> > > > > > >     if (!env->tsc_khz) {
> > > > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > > > >     }
> > > > > > >
> > > > > > 
> > > > > > Consider an example that we migrate a VM from machine A to machine B
> > > > > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > > > beginning):
> > > > > >  1) In the migration from B to C, the user-specified TSC frequency by
> > > > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > > > >     value of env->tsc_khz on B is migrated.
> > > > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > > > >     different than the user-specified TSC frequency on B, the
> > > > > >     expectation in 1) will not be satisfied anymore.
> > > > > 
> > > > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > > > This is not different from changing the CPU model and adding or removing
> > > > > CPU flags when migrating, which is also incorrect. The command-line
> > > > > parameters defining the VM must be the same when you migrate.
> > > > >
> > > > 
> > > > Good to know it's an invalid usage. Then the question is what QEMU is
> > > > expected to do for this invalid usage?
> > > > 
> > > >  1) Abort the migration? But I find that the current QEMU does not
> > > >     abort the migration between different CPU models (e.g. Nehalem and
> > > >     Haswell).
> > > > 
> > > >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> > > >     tsc_khz_saved will be not needed.
> > > 
> > > My first choice is to abort migration. If we decide to abort today and
> > > find it to cause problems, we can easily fix it. If we decide to
> > > continue without aborting, it is difficult to change that behavior
> > > without breaking existing setups.
> > >
> > 
> > Two additional questions:
> > 
> >  1) Existing QEMU allows 'tsc-freq' on the destination in the
> >     migration. If we decided to abort when both 'tsc-freq' and
> >     migrated TSC were present on the destination, it would break some
> >     existing usages. Considering backward compatibility, would above
> >     choice 2) be better?
> 
> We shouldn't abort simply because the section is present and tsc-freq is
> set (because we will always send the section in the newer
> machine-types). We should abort only when we know that the command-line
> contradicts what we see in the migration stream.
>

Got it, only abort when there are contradicts.

> > 
> >  2) If we do decide to abort, could I use abort()? Or are there other
> >     clean approaches to abort?
> 
> You don't need to abort QEMU. You just need to tell the migration code
> that migration can't continue. The exact way to do it depends on where
> you are hooking the sanity check code. If you use a
> VMStateDescription.post_load hook, you can use error_report() and return
> a negative errno value. CCing Quintela in case he has suggestions.
>

Yes, I really should put the sanity check in cpu_post_load().

Thanks,
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 v3 2/3] target-i386: calculate vcpu's TSC rate to be migrated

[Haozhong Zhang]

On 11/11/15 13:23, Eduardo Habkost wrote:
> On Wed, Nov 11, 2015 at 12:57:44AM +0800, Haozhong Zhang wrote:
> > On 11/09/15 14:01, Eduardo Habkost wrote:
> > > On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> > > > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > > > [...]
> > > > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > > > +    }
> > > > > > > > > 
> > > > > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > > > > objects.
> > > > > > > > >
> > > > > > > > 
> > > > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > > > migration. I can change this line to
> > > > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > > > 
> > > > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > > > > code, if you could just do this:
> > > > > > > 
> > > > > > >     if (!env->tsc_khz) {
> > > > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > > > >     }
> > > > > > >
> > > > > > 
> > > > > > Consider an example that we migrate a VM from machine A to machine B
> > > > > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > > > beginning):
> > > > > >  1) In the migration from B to C, the user-specified TSC frequency by
> > > > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > > > >     value of env->tsc_khz on B is migrated.
> > > > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > > > >     different than the user-specified TSC frequency on B, the
> > > > > >     expectation in 1) will not be satisfied anymore.
> > > > > 
> > > > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > > > This is not different from changing the CPU model and adding or removing
> > > > > CPU flags when migrating, which is also incorrect. The command-line
> > > > > parameters defining the VM must be the same when you migrate.
> > > > >
> > > > 
> > > > Good to know it's an invalid usage. Then the question is what QEMU is
> > > > expected to do for this invalid usage?
> > > > 
> > > >  1) Abort the migration? But I find that the current QEMU does not
> > > >     abort the migration between different CPU models (e.g. Nehalem and
> > > >     Haswell).
> > > > 
> > > >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> > > >     tsc_khz_saved will be not needed.
> > > 
> > > My first choice is to abort migration. If we decide to abort today and
> > > find it to cause problems, we can easily fix it. If we decide to
> > > continue without aborting, it is difficult to change that behavior
> > > without breaking existing setups.
> > >
> > 
> > Two additional questions:
> > 
> >  1) Existing QEMU allows 'tsc-freq' on the destination in the
> >     migration. If we decided to abort when both 'tsc-freq' and
> >     migrated TSC were present on the destination, it would break some
> >     existing usages. Considering backward compatibility, would above
> >     choice 2) be better?
> 
> We shouldn't abort simply because the section is present and tsc-freq is
> set (because we will always send the section in the newer
> machine-types). We should abort only when we know that the command-line
> contradicts what we see in the migration stream.
>

Got it, only abort when there are contradicts.

> > 
> >  2) If we do decide to abort, could I use abort()? Or are there other
> >     clean approaches to abort?
> 
> You don't need to abort QEMU. You just need to tell the migration code
> that migration can't continue. The exact way to do it depends on where
> you are hooking the sanity check code. If you use a
> VMStateDescription.post_load hook, you can use error_report() and return
> a negative errno value. CCing Quintela in case he has suggestions.
>

Yes, I really should put the sanity check in cpu_post_load().

Thanks,
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 v3 2/3] target-i386: calculate vcpu's TSC rate to be migrated

[Haozhong Zhang]

On 11/11/15 12:54, Eduardo Habkost wrote:
> On Tue, Nov 10, 2015 at 09:08:58AM +0800, Haozhong Zhang wrote:
> > On 11/09/15 14:01, Eduardo Habkost wrote:
> > > On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> > > > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > > > [...]
> > > > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > > > +    }
> > > > > > > > > 
> > > > > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > > > > objects.
> > > > > > > > >
> > > > > > > > 
> > > > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > > > migration. I can change this line to
> > > > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > > > 
> > > > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > > > > code, if you could just do this:
> > > > > > > 
> > > > > > >     if (!env->tsc_khz) {
> > > > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > > > >     }
> > > > > > >
> > > > > > 
> > > > > > Consider an example that we migrate a VM from machine A to machine B
> > > > > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > > > beginning):
> > > > > >  1) In the migration from B to C, the user-specified TSC frequency by
> > > > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > > > >     value of env->tsc_khz on B is migrated.
> > > > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > > > >     different than the user-specified TSC frequency on B, the
> > > > > >     expectation in 1) will not be satisfied anymore.
> > > > > 
> > > > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > > > This is not different from changing the CPU model and adding or removing
> > > > > CPU flags when migrating, which is also incorrect. The command-line
> > > > > parameters defining the VM must be the same when you migrate.
> > > > >
> > > > 
> > > > Good to know it's an invalid usage. Then the question is what QEMU is
> > > > expected to do for this invalid usage?
> > > > 
> > > >  1) Abort the migration? But I find that the current QEMU does not
> > > >     abort the migration between different CPU models (e.g. Nehalem and
> > > >     Haswell).
> > > > 
> > > >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> > > >     tsc_khz_saved will be not needed.
> > > 
> > > My first choice is to abort migration. If we decide to abort today and
> > > find it to cause problems, we can easily fix it. If we decide to
> > > continue without aborting, it is difficult to change that behavior
> > > without breaking existing setups.
> > 
> > Agree, but I would like to relax the abort condition to "abort the
> > migration only if QEMU on the destination uses a different TSC
> > frequency than the migrated one" so that the following usages would be
> > still valid:
> >  1) Only QEMU on the destination has 'tsc-freq' option, but it' set to
> >     the same value of the migrated one.
> >  2) Only QEMU on the source has 'tsc-freq' option.
> >  3) QEMU on both sides have 'tsc-freq' option, but they are set to the
> >     same value.
> > In all above usages, TSC frequency on the destination is the same as
> > both the value on the source and the value explicitly expected by
> > users on the destination (by 'tsc-freq' on the destination).
> 
> Yes, that's probably all we can do because we don't really know
> the command-line on the source. All we know is that the user is
> asking for a TSC frequency that doesn't match what we see in the
> migration stream.
> 
> > 
> > And I still need tsc_khz_saved to tell on the destination whether
> >  1) both tsc-freq option and migrated TSC frequency are present, and
> >  2) above two values are the same.
> > Even though we restrictively requires QEMU on both sides use the same
> > CPU options, tsc_khz_saved is still needed because of 1).
> > 
> 
> So, it looks like we need an extra field because of two things:
> the migration sanity check, and SET_TSC_KHZ error handling. But
> what bothers me is that we have two redundant fields that could
> contradict each other, and it is not clear which one we should
> use on which case. Sometimes only tsc_khz is valid, sometimes
> only tsc_khz is valid.
> 
> What if we set/use tsc_khz on all code (no exceptions), but add a
> new field just to indicate if tsc-freq was set by the user?
> Something like:
> 
> * SET_TSC_KHZ code uses tsc_khz only;
> * "tsc-freq" property getter returns tsc_khz;
> * VMState saves/loads tsc_khz only;
> * Initialization code set tsc_khz = ioctl(KVM_GET_TSC_KHZ) in
>   case tsc_khz is not set.
> 
> And a new user_tsc_khz field would be added and used only for
> error checking:
> * "tsc-freq" property setter changes both tsc_khz and
>   user_tsc_khz;
> * Sanity check code post migration-load ensures that
>   (!user_tsc_khz || tsc_khz == user_tsc_khz);
> * SET_TSC_KHZ error handling returns error if user_tsc_khz
>   is set.
>

Your suggestion looks more clear than my current implementation. I'll
turn to it in the next version.

Thanks,
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 v3 2/3] target-i386: calculate vcpu's TSC rate to be migrated

[Haozhong Zhang]

On 11/11/15 12:54, Eduardo Habkost wrote:
> On Tue, Nov 10, 2015 at 09:08:58AM +0800, Haozhong Zhang wrote:
> > On 11/09/15 14:01, Eduardo Habkost wrote:
> > > On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zhang@intel.com wrote:
> > > > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zhang@intel.com wrote:
> > > > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > > > [...]
> > > > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > > > +    }
> > > > > > > > > 
> > > > > > > > > Why do you need a separate tsc_khz_saved field, and don't simply use
> > > > > > > > > tsc_khz? It would have the additional feature of letting QMP clients
> > > > > > > > > query the current TSC rate by asking for the tsc-freq property on CPU
> > > > > > > > > objects.
> > > > > > > > >
> > > > > > > > 
> > > > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > > > migration. I can change this line to
> > > > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > > > 
> > > > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
> > > > > > > you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
> > > > > > > code, if you could just do this:
> > > > > > > 
> > > > > > >     if (!env->tsc_khz) {
> > > > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > > > >     }
> > > > > > >
> > > > > > 
> > > > > > Consider an example that we migrate a VM from machine A to machine B
> > > > > > and then to machine C, and QEMU on machine B is launched with the cpu
> > > > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > > > beginning):
> > > > > >  1) In the migration from B to C, the user-specified TSC frequency by
> > > > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > > > >     value of env->tsc_khz on B is migrated.
> > > > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > > > >     different than the user-specified TSC frequency on B, the
> > > > > >     expectation in 1) will not be satisfied anymore.
> > > > > 
> > > > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > > > This is not different from changing the CPU model and adding or removing
> > > > > CPU flags when migrating, which is also incorrect. The command-line
> > > > > parameters defining the VM must be the same when you migrate.
> > > > >
> > > > 
> > > > Good to know it's an invalid usage. Then the question is what QEMU is
> > > > expected to do for this invalid usage?
> > > > 
> > > >  1) Abort the migration? But I find that the current QEMU does not
> > > >     abort the migration between different CPU models (e.g. Nehalem and
> > > >     Haswell).
> > > > 
> > > >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> > > >     tsc_khz_saved will be not needed.
> > > 
> > > My first choice is to abort migration. If we decide to abort today and
> > > find it to cause problems, we can easily fix it. If we decide to
> > > continue without aborting, it is difficult to change that behavior
> > > without breaking existing setups.
> > 
> > Agree, but I would like to relax the abort condition to "abort the
> > migration only if QEMU on the destination uses a different TSC
> > frequency than the migrated one" so that the following usages would be
> > still valid:
> >  1) Only QEMU on the destination has 'tsc-freq' option, but it' set to
> >     the same value of the migrated one.
> >  2) Only QEMU on the source has 'tsc-freq' option.
> >  3) QEMU on both sides have 'tsc-freq' option, but they are set to the
> >     same value.
> > In all above usages, TSC frequency on the destination is the same as
> > both the value on the source and the value explicitly expected by
> > users on the destination (by 'tsc-freq' on the destination).
> 
> Yes, that's probably all we can do because we don't really know
> the command-line on the source. All we know is that the user is
> asking for a TSC frequency that doesn't match what we see in the
> migration stream.
> 
> > 
> > And I still need tsc_khz_saved to tell on the destination whether
> >  1) both tsc-freq option and migrated TSC frequency are present, and
> >  2) above two values are the same.
> > Even though we restrictively requires QEMU on both sides use the same
> > CPU options, tsc_khz_saved is still needed because of 1).
> > 
> 
> So, it looks like we need an extra field because of two things:
> the migration sanity check, and SET_TSC_KHZ error handling. But
> what bothers me is that we have two redundant fields that could
> contradict each other, and it is not clear which one we should
> use on which case. Sometimes only tsc_khz is valid, sometimes
> only tsc_khz is valid.
> 
> What if we set/use tsc_khz on all code (no exceptions), but add a
> new field just to indicate if tsc-freq was set by the user?
> Something like:
> 
> * SET_TSC_KHZ code uses tsc_khz only;
> * "tsc-freq" property getter returns tsc_khz;
> * VMState saves/loads tsc_khz only;
> * Initialization code set tsc_khz = ioctl(KVM_GET_TSC_KHZ) in
>   case tsc_khz is not set.
> 
> And a new user_tsc_khz field would be added and used only for
> error checking:
> * "tsc-freq" property setter changes both tsc_khz and
>   user_tsc_khz;
> * Sanity check code post migration-load ensures that
>   (!user_tsc_khz || tsc_khz == user_tsc_khz);
> * SET_TSC_KHZ error handling returns error if user_tsc_khz
>   is set.
>

Your suggestion looks more clear than my current implementation. I'll
turn to it in the next version.

Thanks,
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 v3 1/3] target-i386: add a subsection for migrating vcpu's TSC rate

[Haozhong Zhang]

On 11/11/15 22:27, Haozhong Zhang wrote:
> On 11/11/15 12:16, Eduardo Habkost wrote:
[...]
> > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > index 2f8f396..858ed69 100644
> > > --- a/hw/i386/pc_q35.c
> > > +++ b/hw/i386/pc_q35.c
> > > @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> > >      pc_q35_2_5_machine_options(m);
> > >      m->alias = NULL;
> > >      pcmc->broken_reserved_end = true;
> > > +    pcmc->save_tsc_khz = false;
> > 
> > I had suggested the PCMachineClass field, but now I've been thinking:
> > all other fields related to tsc_khz are in X86CPU, so I believe this
> > belongs to X86CPU too. It could be a simple X86CPU property set by
> > PC_COMPAT_2_4.
> >
> 
> Reasonable, will update in the next version.

Or maybe no ...

I think there is still a problem to set a X86CPU property in
PC_COMPAT_2_4:

if I create a property for save_tsc_khz by adding
  DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, save_tsc_khz, true)
in x86_cpu_properties and add
  {
      .driver   = TYPE_X86_CPU,
      .property = "save-tsc-freq",
      .value    = "off",
  }
in PC_COMPAT_2_4, then "save-tsc-freq" will also become a
user-visible cpu option. But we agreed on keeping it as an
internal flag in the previous discussion.

Any other ways to set a property in PC_COMPAT_* while keeping that
property internal?

Haozhong

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

[Haozhong Zhang]

On 11/11/15 22:27, Haozhong Zhang wrote:
> On 11/11/15 12:16, Eduardo Habkost wrote:
[...]
> > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > index 2f8f396..858ed69 100644
> > > --- a/hw/i386/pc_q35.c
> > > +++ b/hw/i386/pc_q35.c
> > > @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> > >      pc_q35_2_5_machine_options(m);
> > >      m->alias = NULL;
> > >      pcmc->broken_reserved_end = true;
> > > +    pcmc->save_tsc_khz = false;
> > 
> > I had suggested the PCMachineClass field, but now I've been thinking:
> > all other fields related to tsc_khz are in X86CPU, so I believe this
> > belongs to X86CPU too. It could be a simple X86CPU property set by
> > PC_COMPAT_2_4.
> >
> 
> Reasonable, will update in the next version.

Or maybe no ...

I think there is still a problem to set a X86CPU property in
PC_COMPAT_2_4:

if I create a property for save_tsc_khz by adding
  DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, save_tsc_khz, true)
in x86_cpu_properties and add
  {
      .driver   = TYPE_X86_CPU,
      .property = "save-tsc-freq",
      .value    = "off",
  }
in PC_COMPAT_2_4, then "save-tsc-freq" will also become a
user-visible cpu option. But we agreed on keeping it as an
internal flag in the previous discussion.

Any other ways to set a property in PC_COMPAT_* while keeping that
property internal?

Haozhong

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

[Eduardo Habkost]

On Fri, Nov 13, 2015 at 10:23:54AM +0800, Haozhong Zhang wrote:
> On 11/11/15 22:27, Haozhong Zhang wrote:
> > On 11/11/15 12:16, Eduardo Habkost wrote:
> [...]
> > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > > index 2f8f396..858ed69 100644
> > > > --- a/hw/i386/pc_q35.c
> > > > +++ b/hw/i386/pc_q35.c
> > > > @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> > > >      pc_q35_2_5_machine_options(m);
> > > >      m->alias = NULL;
> > > >      pcmc->broken_reserved_end = true;
> > > > +    pcmc->save_tsc_khz = false;
> > > 
> > > I had suggested the PCMachineClass field, but now I've been thinking:
> > > all other fields related to tsc_khz are in X86CPU, so I believe this
> > > belongs to X86CPU too. It could be a simple X86CPU property set by
> > > PC_COMPAT_2_4.
> > >
> > 
> > Reasonable, will update in the next version.
> 
> Or maybe no ...
> 
> I think there is still a problem to set a X86CPU property in
> PC_COMPAT_2_4:
> 
> if I create a property for save_tsc_khz by adding
>   DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, save_tsc_khz, true)
> in x86_cpu_properties and add
>   {
>       .driver   = TYPE_X86_CPU,
>       .property = "save-tsc-freq",
>       .value    = "off",
>   }
> in PC_COMPAT_2_4, then "save-tsc-freq" will also become a
> user-visible cpu option. But we agreed on keeping it as an
> internal flag in the previous discussion.
> 
> Any other ways to set a property in PC_COMPAT_* while keeping that
> property internal?

I don't think making it internal is a requirement. It just make
things simpler because it allowed us to postpone decisions about
the user-visible parts.

...which seems to be a good reason to keep it on PCMachineClass
by now, if you prefer it that way. The subsection code is already
on machine.c and not on cpu.c, anyway.

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Nov 13, 2015 at 10:23:54AM +0800, Haozhong Zhang wrote:
> On 11/11/15 22:27, Haozhong Zhang wrote:
> > On 11/11/15 12:16, Eduardo Habkost wrote:
> [...]
> > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > > index 2f8f396..858ed69 100644
> > > > --- a/hw/i386/pc_q35.c
> > > > +++ b/hw/i386/pc_q35.c
> > > > @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> > > >      pc_q35_2_5_machine_options(m);
> > > >      m->alias = NULL;
> > > >      pcmc->broken_reserved_end = true;
> > > > +    pcmc->save_tsc_khz = false;
> > > 
> > > I had suggested the PCMachineClass field, but now I've been thinking:
> > > all other fields related to tsc_khz are in X86CPU, so I believe this
> > > belongs to X86CPU too. It could be a simple X86CPU property set by
> > > PC_COMPAT_2_4.
> > >
> > 
> > Reasonable, will update in the next version.
> 
> Or maybe no ...
> 
> I think there is still a problem to set a X86CPU property in
> PC_COMPAT_2_4:
> 
> if I create a property for save_tsc_khz by adding
>   DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, save_tsc_khz, true)
> in x86_cpu_properties and add
>   {
>       .driver   = TYPE_X86_CPU,
>       .property = "save-tsc-freq",
>       .value    = "off",
>   }
> in PC_COMPAT_2_4, then "save-tsc-freq" will also become a
> user-visible cpu option. But we agreed on keeping it as an
> internal flag in the previous discussion.
> 
> Any other ways to set a property in PC_COMPAT_* while keeping that
> property internal?

I don't think making it internal is a requirement. It just make
things simpler because it allowed us to postpone decisions about
the user-visible parts.

...which seems to be a good reason to keep it on PCMachineClass
by now, if you prefer it that way. The subsection code is already
on machine.c and not on cpu.c, anyway.

-- 
Eduardo

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

[Haozhong Zhang]

On 11/13/15 13:21, Eduardo Habkost wrote:
> On Fri, Nov 13, 2015 at 10:23:54AM +0800, Haozhong Zhang wrote:
> > On 11/11/15 22:27, Haozhong Zhang wrote:
> > > On 11/11/15 12:16, Eduardo Habkost wrote:
> > [...]
> > > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > > > index 2f8f396..858ed69 100644
> > > > > --- a/hw/i386/pc_q35.c
> > > > > +++ b/hw/i386/pc_q35.c
> > > > > @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> > > > >      pc_q35_2_5_machine_options(m);
> > > > >      m->alias = NULL;
> > > > >      pcmc->broken_reserved_end = true;
> > > > > +    pcmc->save_tsc_khz = false;
> > > > 
> > > > I had suggested the PCMachineClass field, but now I've been thinking:
> > > > all other fields related to tsc_khz are in X86CPU, so I believe this
> > > > belongs to X86CPU too. It could be a simple X86CPU property set by
> > > > PC_COMPAT_2_4.
> > > >
> > > 
> > > Reasonable, will update in the next version.
> > 
> > Or maybe no ...
> > 
> > I think there is still a problem to set a X86CPU property in
> > PC_COMPAT_2_4:
> > 
> > if I create a property for save_tsc_khz by adding
> >   DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, save_tsc_khz, true)
> > in x86_cpu_properties and add
> >   {
> >       .driver   = TYPE_X86_CPU,
> >       .property = "save-tsc-freq",
> >       .value    = "off",
> >   }
> > in PC_COMPAT_2_4, then "save-tsc-freq" will also become a
> > user-visible cpu option. But we agreed on keeping it as an
> > internal flag in the previous discussion.
> > 
> > Any other ways to set a property in PC_COMPAT_* while keeping that
> > property internal?
> 
> I don't think making it internal is a requirement. It just make
> things simpler because it allowed us to postpone decisions about
> the user-visible parts.
> 
> ...which seems to be a good reason to keep it on PCMachineClass
> by now, if you prefer it that way. The subsection code is already
> on machine.c and not on cpu.c, anyway.
>

Thanks, I'll keep it in PCMachineClass in the next version.

Haozhong

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

[Haozhong Zhang]

On 11/13/15 13:21, Eduardo Habkost wrote:
> On Fri, Nov 13, 2015 at 10:23:54AM +0800, Haozhong Zhang wrote:
> > On 11/11/15 22:27, Haozhong Zhang wrote:
> > > On 11/11/15 12:16, Eduardo Habkost wrote:
> > [...]
> > > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > > > index 2f8f396..858ed69 100644
> > > > > --- a/hw/i386/pc_q35.c
> > > > > +++ b/hw/i386/pc_q35.c
> > > > > @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> > > > >      pc_q35_2_5_machine_options(m);
> > > > >      m->alias = NULL;
> > > > >      pcmc->broken_reserved_end = true;
> > > > > +    pcmc->save_tsc_khz = false;
> > > > 
> > > > I had suggested the PCMachineClass field, but now I've been thinking:
> > > > all other fields related to tsc_khz are in X86CPU, so I believe this
> > > > belongs to X86CPU too. It could be a simple X86CPU property set by
> > > > PC_COMPAT_2_4.
> > > >
> > > 
> > > Reasonable, will update in the next version.
> > 
> > Or maybe no ...
> > 
> > I think there is still a problem to set a X86CPU property in
> > PC_COMPAT_2_4:
> > 
> > if I create a property for save_tsc_khz by adding
> >   DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, save_tsc_khz, true)
> > in x86_cpu_properties and add
> >   {
> >       .driver   = TYPE_X86_CPU,
> >       .property = "save-tsc-freq",
> >       .value    = "off",
> >   }
> > in PC_COMPAT_2_4, then "save-tsc-freq" will also become a
> > user-visible cpu option. But we agreed on keeping it as an
> > internal flag in the previous discussion.
> > 
> > Any other ways to set a property in PC_COMPAT_* while keeping that
> > property internal?
> 
> I don't think making it internal is a requirement. It just make
> things simpler because it allowed us to postpone decisions about
> the user-visible parts.
> 
> ...which seems to be a good reason to keep it on PCMachineClass
> by now, if you prefer it that way. The subsection code is already
> on machine.c and not on cpu.c, anyway.
>

Thanks, I'll keep it in PCMachineClass in the next version.

Haozhong