From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH v3 07/10] qemu-kvm: Cleanup/fix TSC and PV clock writeback Date: Thu, 25 Feb 2010 00:58:14 -0300 Message-ID: <20100225035814.GA470@amt.cnet> References: <20100224231708.GB16246@amt.cnet> <4B85BA33.5080008@web.de> <20100224234935.GA17862@amt.cnet> <4B85BD22.6050209@web.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Avi Kivity , kvm@vger.kernel.org, qemu-devel@nongnu.org, Gleb Natapov , Zachary Amsden To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:14333 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932262Ab0BYEkf (ORCPT ); Wed, 24 Feb 2010 23:40:35 -0500 Content-Disposition: inline In-Reply-To: <4B85BD22.6050209@web.de> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Feb 25, 2010 at 12:58:26AM +0100, Jan Kiszka wrote: > Marcelo Tosatti wrote: > > On Thu, Feb 25, 2010 at 12:45:55AM +0100, Jan Kiszka wrote: > >> Marcelo Tosatti wrote: > >>> On Wed, Feb 24, 2010 at 03:17:55PM +0100, Jan Kiszka wrote: > >>>> Drop kvm_load_tsc in favor of level-dependent writeback in > >>>> kvm_arch_load_regs. KVM's PV clock MSRs fall in the same category and > >>>> should therefore only be written back on full sync. > >>>> > >>>> Signed-off-by: Jan Kiszka > >>>> --- > >>>> qemu-kvm-x86.c | 19 +++++-------------- > >>>> qemu-kvm.h | 4 ---- > >>>> target-i386/machine.c | 5 ----- > >>>> 3 files changed, 5 insertions(+), 23 deletions(-) > >>>> > >>>> diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c > >>>> index 840c1c9..84fd7fa 100644 > >>>> --- a/qemu-kvm-x86.c > >>>> +++ b/qemu-kvm-x86.c > >>>> @@ -965,8 +965,11 @@ void kvm_arch_load_regs(CPUState *env, int level) > >>>> set_msr_entry(&msrs[n++], MSR_LSTAR , env->lstar); > >>>> } > >>>> #endif > >>>> - set_msr_entry(&msrs[n++], MSR_KVM_SYSTEM_TIME, env->system_time_msr); > >>>> - set_msr_entry(&msrs[n++], MSR_KVM_WALL_CLOCK, env->wall_clock_msr); > >>>> + if (level == KVM_PUT_FULL_STATE) { > >>>> + set_msr_entry(&msrs[n++], MSR_IA32_TSC, env->tsc); > >>>> + set_msr_entry(&msrs[n++], MSR_KVM_SYSTEM_TIME, env->system_time_msr); > >>>> + set_msr_entry(&msrs[n++], MSR_KVM_WALL_CLOCK, env->wall_clock_msr); > >>>> + } > >>> As things stand today, the TSC should only be written on migration. See > >>> 53f658b3c33616a4997ee254311b335e59063289 in the kernel. > >> Migration and power-up - that's what this patch ensures (=> > >> KVM_PUT_FULL_STATE). Or where do you see any problem? > >> > >> Jan > >> > > > > The problem is it should not write on power up (the kernel attempts > > to synchronize the TSCs in that case, see the commit). > > > > OK, need to read this more carefully. > > I do not yet understand the difference from user space POV: it tries to > transfer the identical TSC values to all currently stopped VCPU threads. guest tsc = host tsc + offset So at the time you set_msr(TSC), the guest visible TSC starts ticking. For SMP guests, this does not happen exactly at the same time for all vcpus. > That should not be different if we are booting a fresh VM or loading a > complete state of a migrated image. If it does, it looks like a KVM > kernel deficit on first glance. Yes it is a deficit. After migration TSCs of SMP guests go out of sync. Zachary is working on that. > > Jan >