From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46893) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwXFo-0006Cv-BB for qemu-devel@nongnu.org; Wed, 11 Nov 2015 10:24:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZwXFN-0003gZ-6G for qemu-devel@nongnu.org; Wed, 11 Nov 2015 10:24:08 -0500 Date: Wed, 11 Nov 2015 13:23:37 -0200 From: Eduardo Habkost Message-ID: <20151111152337.GI4180@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> <20151106151222.GS4180@thinpad.lan.raisama.net> <20151109003355.GC27132@hzzhang-OptiPlex-9020.sh.intel.com> <20151109160123.GB4180@thinpad.lan.raisama.net> <20151110165744.GA1530@hzzhang-OptiPlex-9020.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151110165744.GA1530@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, Juan Quintela 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