From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56920) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VocOe-0004CK-Uj for qemu-devel@nongnu.org; Thu, 05 Dec 2013 12:07:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VocOY-0006Ku-Ty for qemu-devel@nongnu.org; Thu, 05 Dec 2013 12:07:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:21155) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VocOY-0006Jr-M4 for qemu-devel@nongnu.org; Thu, 05 Dec 2013 12:07:22 -0500 Date: Thu, 5 Dec 2013 15:06:55 -0200 From: Marcelo Tosatti Message-ID: <20131205170655.GA20427@amt.cnet> References: <1386054500.25757.10.camel@nexus> <529D90A6.2080801@lab.ntt.co.jp> <52A0186A.2050207@lab.ntt.co.jp> <1386224104.3091.3.camel@nexus> <52A04732.4040105@redhat.com> <52A07C5A.9090105@lab.ntt.co.jp> <52A08541.6090702@redhat.com> <52A09EF4.5080800@lab.ntt.co.jp> <52A0A37A.7060204@redhat.com> <20131205164000.GA18678@amt.cnet> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20131205164000.GA18678@amt.cnet> Subject: Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Gleb Natapov , Will Auld , qemu-devel@nongnu.org, kvm@vger.kernel.org, Fernando Luis Vazquez Cao On Thu, Dec 05, 2013 at 02:40:00PM -0200, Marcelo Tosatti wrote: > On Thu, Dec 05, 2013 at 05:02:02PM +0100, Paolo Bonzini wrote: > > Il 05/12/2013 16:42, Fernando Luis Vazquez Cao ha scritto: > > > (2013/12/05 22:53), Paolo Bonzini wrote: > > >> Il 05/12/2013 14:15, Fernando Luis Vazquez Cao ha scritto: > > >>> /* > > >>> * KVM is yet unable to synchronize TSC values of multiple VCPUs on > > >>> * writeback. Until this is fixed, we only write the offset to SMP > > >>> * guests after migration, desynchronizing the VCPUs, but avoiding > > >>> * huge jump-backs that would occur without any writeback at all. > > >>> */ > > >>> - if (smp_cpus == 1 || env->tsc != 0) { > > >>> + if (smp_cpus == 1 || env->tsc != 0 || level == KVM_PUT_RESET_STATE) { > > >>> kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc); > > >>> } > > >> This is still a bit ugly, and desynchronizes the VCPUs on reset. > > > > > > I agree it is a bit ugly, but in my testing QEMU seemed to loop over all > > > the VCPUS fast enough for the kernel side kvm_write_tsc() to do a > > > reasonable job of matching the offsets (the Linux guest did not mark > > > the TSC unstable due to the TSCs being unsynchronized). Am I missing > > > something? > > > > No, probably not. > > > > > I understand the benefits of what you are proposing but, since it is > > > wider is scope and it would be more difficult to backport, I would > > > prefer to implement it as a follow-up patch, unless you think that > > > the current patch as a standalone fix does more harm than good. > > > > It does some harm in that it introduces a case where KVM_PUT_RESET_STATE > > restores something, but KVM_PUT_FULL_STATE doesn't. > > > > If it really usually works, there shouldn't be a need for this "if" > > statement at all. > > > > Marcelo, what do you think? > > > > Paolo > > Its OK to drop it, provided the following is tested on SMP guests: > > 1. initialization. > 2. reboot. > 3. migration. > > With both stable and unstable TSC hosts (use wrmsr tool to write TSC on > a given host CPU, to make it an unstable TSC host). (A=rdmsr ; sleep 1s; > wrmsr A). > > To make sure the code is not securing against a kvm_write_tsc > cornercase. The TSCs should start synchronized, and remain synchronized across reboot and migration for stable TSC host case. It is not necessary to test the unstable TSC host case.