Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization

[Gleb Natapov <gleb@redhat.com>]

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.