| Message ID | 1446456403-29909-3-git-send-email-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 > > -- 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
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; > +}
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 -- 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
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. -- 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
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 -- 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
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.
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 -- 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
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.
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 -- 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
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 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 -- 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
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 -- 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
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 -- 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
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.
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.
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 -- 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
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 -- 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
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; +}
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(+)