From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization Date: Tue, 15 Dec 2009 14:33:27 +0200 Message-ID: <20091215123327.GD8031@redhat.com> References: <20091209174654.GA28056@amt.cnet> <4B20C058.5080307@redhat.com> <4B20C32B.7070005@redhat.com> <20091214203637.GA7120@amt.cnet> <20091215112037.GC8031@redhat.com> <20091215122415.GA9715@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Avi Kivity , Glauber de Oliveira Costa , kvm To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:37102 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760146AbZLOMd3 (ORCPT ); Tue, 15 Dec 2009 07:33:29 -0500 Received: from int-mx04.intmail.prod.int.phx2.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.17]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id nBFCXSVY018471 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 15 Dec 2009 07:33:29 -0500 Content-Disposition: inline In-Reply-To: <20091215122415.GA9715@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Dec 15, 2009 at 10:24:15AM -0200, Marcelo Tosatti wrote: > On Tue, Dec 15, 2009 at 01:20:38PM +0200, Gleb Natapov wrote: > > On Mon, Dec 14, 2009 at 06:36:37PM -0200, Marcelo Tosatti wrote: > > > > > > So that the vcpu state is initialized, from vcpu thread context, after > > > machine initialization is settled. > > > > > > This allows to revert apic_init's apic_reset call. apic_reset now > > > happens through system_reset, similarly to qemu upstream. > > > > > This patch essentially revers commit 898c51c3. This commit fixes two > > races. First race is like this: > > > > vcpu0 vcpu1 > > > > starts running > > loads lapic state into kernel > > sends event to vcpu1 > > starts running > > loads lapic state into kernel > > overwrites event from vcpu0 > > > > At the time 898c51c3 was committed the race was easily reproducible > > by starting VM with 16 cpus + seabios. Sometimes some vcpus lost INIT/SIPI > > events. Now I am not able to reproduce it even with this patch applied, > > so something else changed, but it doesn't make the race non existent or > > acceptable. > > Note qemu_kvm_load_lapic depends on env->created set (kvm_vcpu_inited), Uhh. What about doing this: diff --git a/qemu-kvm.c b/qemu-kvm.c index 44e8b75..fa6db8e 100644 --- a/qemu-kvm.c +++ b/qemu-kvm.c @@ -1920,12 +1920,12 @@ static void *ap_main_loop(void *_env) pthread_mutex_lock(&qemu_mutex); cpu_single_env = env; + current_env->created = 1; kvm_arch_init_vcpu(env); kvm_arch_load_regs(env); /* signal VCPU creation */ - current_env->created = 1; pthread_cond_signal(&qemu_vcpu_cond); /* and wait for machine initialization */ > so having init_vcpu+load_regs before signalling vcpu creation did not > fix this one (but yeah, thanks for the reminder on the races). > > > The second race is during machine start after migration. The race is > > between event loop and vcpu: > > > > event loop vcpu > > > > starts running > > gets RTC timer event > > sends interrupt to vcpu > > starts running > > loads lapic state into kernel > > overwrites interrupt from RTC > > > > > > In short vcpu state that can be influenced by sources outside vcpu thread > > itself should be uploaded into the kernel before signaling qemu_system_ready > > condition. > > > > -- > > Gleb. > > -- > > To unsubscribe from this list: send the line "unsubscribe kvm" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Gleb.