From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eduardo Habkost Subject: Re: [PATCH v3 2/3] target-i386: calculate vcpu's TSC rate to be migrated Date: Fri, 6 Nov 2015 13:12:22 -0200 Message-ID: <20151106151222.GS4180@thinpad.lan.raisama.net> References: <1446456403-29909-1-git-send-email-haozhong.zhang@intel.com> <1446456403-29909-3-git-send-email-haozhong.zhang@intel.com> <20151104214231.GM4180@thinpad.lan.raisama.net> <20151105013051.GA24388@hzzhang-OptiPlex-9020.sh.intel.com> <20151105160501.GN4180@thinpad.lan.raisama.net> <20151106023224.GB27580@hzzhang-OptiPlex-9020.sh.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii To: qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , "Michael S. Tsirkin" , Paolo Bonzini , Richard Henderson , Peter Maydell , Marcelo Tosatti , James Hogan , Aurelien Jarno , Leon Alrae , Alexander Graf , Christian Borntraeger , Cornelia Huck , kvm@vger.kernel.org, qemu-ppc@nongnu.org Return-path: Received: from mx1.redhat.com ([209.132.183.28]:37878 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751111AbbKFPM0 (ORCPT ); Fri, 6 Nov 2015 10:12:26 -0500 Content-Disposition: inline In-Reply-To: <20151106023224.GB27580@hzzhang-OptiPlex-9020.sh.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43292) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zuigq-0002uQ-5J for qemu-devel@nongnu.org; Fri, 06 Nov 2015 10:12:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zuigl-0002bm-4o for qemu-devel@nongnu.org; Fri, 06 Nov 2015 10:12:32 -0500 Date: Fri, 6 Nov 2015 13:12:22 -0200 From: Eduardo Habkost Message-ID: <20151106151222.GS4180@thinpad.lan.raisama.net> References: <1446456403-29909-1-git-send-email-haozhong.zhang@intel.com> <1446456403-29909-3-git-send-email-haozhong.zhang@intel.com> <20151104214231.GM4180@thinpad.lan.raisama.net> <20151105013051.GA24388@hzzhang-OptiPlex-9020.sh.intel.com> <20151105160501.GN4180@thinpad.lan.raisama.net> <20151106023224.GB27580@hzzhang-OptiPlex-9020.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151106023224.GB27580@hzzhang-OptiPlex-9020.sh.intel.com> Subject: Re: [Qemu-devel] [PATCH v3 2/3] target-i386: calculate vcpu's TSC rate to be migrated List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , "Michael S. Tsirkin" , Paolo Bonzini , Richard Henderson , Peter Maydell , Marcelo Tosatti , James Hogan , Aurelien Jarno , Leon Alrae , Alexander Graf , Christian Borntraeger , Cornelia Huck , kvm@vger.kernel.org, qemu-ppc@nongnu.org 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