From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54573) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VocGO-0005ym-Lm for qemu-devel@nongnu.org; Thu, 05 Dec 2013 11:59:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VocGH-0003fi-TU for qemu-devel@nongnu.org; Thu, 05 Dec 2013 11:58:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46756) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VocGH-0003fc-IR for qemu-devel@nongnu.org; Thu, 05 Dec 2013 11:58:49 -0500 Date: Thu, 5 Dec 2013 14:40:00 -0200 From: Marcelo Tosatti Message-ID: <20131205164000.GA18678@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <52A0A37A.7060204@redhat.com> 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 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.