kvm: qemu: stop/start cpus before/after devices

kvm: qemu: stop/start cpus before/after devices

[Marcelo Tosatti]

From: Yaniv Kamay <yaniv@qumranet.com>

Stop cpus before devices when stopping the VM, start cpus after devices
when starting VM.

Acked-by: Dor Laor <dlaor@redhat.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index 93af6ea..4164368 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -285,7 +285,7 @@ static int all_threads_paused(void)
     return 1;
 }
 
-static void pause_all_threads(void)
+void qemu_kvm_pause_all_threads(void)
 {
     CPUState *penv = first_cpu;
 
@@ -305,7 +305,7 @@ static void pause_all_threads(void)
 	qemu_cond_wait(&qemu_pause_cond);
 }
 
-static void resume_all_threads(void)
+void qemu_kvm_resume_all_threads(void)
 {
     CPUState *penv = first_cpu;
 
@@ -319,14 +319,6 @@ static void resume_all_threads(void)
     }
 }
 
-static void kvm_vm_state_change_handler(void *context, int running, int reason)
-{
-    if (running)
-	resume_all_threads();
-    else
-	pause_all_threads();
-}
-
 static void update_regs_for_sipi(CPUState *env)
 {
     kvm_arch_update_regs_for_sipi(env);
@@ -371,7 +363,7 @@ static void qemu_kvm_system_reset(void)
 {
     CPUState *penv = first_cpu;
 
-    pause_all_threads();
+    qemu_kvm_pause_all_threads();
 
     qemu_system_reset();
 
@@ -380,7 +372,7 @@ static void qemu_kvm_system_reset(void)
         penv = (CPUState *)penv->next_cpu;
     }
 
-    resume_all_threads();
+    qemu_kvm_resume_all_threads();
 }
 
 static int kvm_main_loop_cpu(CPUState *env)
@@ -466,7 +458,6 @@ int kvm_init_ap(void)
 #ifdef TARGET_I386
     kvm_tpr_opt_setup();
 #endif
-    qemu_add_vm_change_state_handler(kvm_vm_state_change_handler, NULL);
 
     signal(SIG_IPI, sig_ipi_handler);
     return 0;
@@ -610,7 +601,7 @@ int kvm_main_loop(void)
 #endif
     }
 
-    pause_all_threads();
+    qemu_kvm_pause_all_threads();
     pthread_mutex_unlock(&qemu_mutex);
 
     return 0;
diff --git a/qemu/qemu-kvm.h b/qemu/qemu-kvm.h
index c0549df..ca59af8 100644
--- a/qemu/qemu-kvm.h
+++ b/qemu/qemu-kvm.h
@@ -119,6 +119,9 @@ int qemu_kvm_register_coalesced_mmio(target_phys_addr_t addr,
 int qemu_kvm_unregister_coalesced_mmio(target_phys_addr_t addr,
 				       unsigned int size);
 
+void qemu_kvm_pause_all_threads(void);
+void qemu_kvm_resume_all_threads(void);
+
 int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size);
 int kvm_uncoalesce_mmio_region(target_phys_addr_t start, ram_addr_t size);
 
diff --git a/qemu/vl.c b/qemu/vl.c
index 7ae266e..c52d2d7 100644
--- a/qemu/vl.c
+++ b/qemu/vl.c
@@ -3596,6 +3596,8 @@ void vm_start(void)
         cpu_enable_ticks();
         vm_running = 1;
         vm_state_notify(1, 0);
+        if (kvm_enabled())
+            qemu_kvm_resume_all_threads();
         qemu_rearm_alarm_timer(alarm_timer);
     }
 }
@@ -3605,6 +3607,8 @@ void vm_stop(int reason)
     if (vm_running) {
         cpu_disable_ticks();
         vm_running = 0;
+        if (kvm_enabled())
+            qemu_kvm_pause_all_threads();
         vm_state_notify(0, reason);
     }
 }

Re: kvm: qemu: stop/start cpus before/after devices

[Avi Kivity]

Marcelo Tosatti wrote:
> From: Yaniv Kamay <yaniv@qumranet.com>
>
> Stop cpus before devices when stopping the VM, start cpus after devices
> when starting VM.
>
>   

Why is this needed?

-- 
error compiling committee.c: too many arguments to function

Re: kvm: qemu: stop/start cpus before/after devices

[Marcelo Tosatti]

On Wed, Mar 25, 2009 at 01:45:52PM +0200, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> From: Yaniv Kamay <yaniv@qumranet.com>
>>
>> Stop cpus before devices when stopping the VM, start cpus after devices
>> when starting VM.
>>
>>   
>
> Why is this needed?

A vcpu could access a stopped device otherwise. 

Re: kvm: qemu: stop/start cpus before/after devices

[Marcelo Tosatti]

On Wed, Mar 25, 2009 at 11:26:19AM -0300, Marcelo Tosatti wrote:
> On Wed, Mar 25, 2009 at 01:45:52PM +0200, Avi Kivity wrote:
> > Marcelo Tosatti wrote:
> >> From: Yaniv Kamay <yaniv@qumranet.com>
> >>
> >> Stop cpus before devices when stopping the VM, start cpus after devices
> >> when starting VM.
> >>
> >>   
> >
> > Why is this needed?
> 
> A vcpu could access a stopped device otherwise. 

Actually on vm_stop its safe because the order happens to be correct,
but on vm_start its the other way around (vcpus start first, and they
should be started last).

Re: kvm: qemu: stop/start cpus before/after devices

[Avi Kivity]

Marcelo Tosatti wrote:
> On Wed, Mar 25, 2009 at 11:26:19AM -0300, Marcelo Tosatti wrote:
>   
>> On Wed, Mar 25, 2009 at 01:45:52PM +0200, Avi Kivity wrote:
>>     
>>> Marcelo Tosatti wrote:
>>>       
>>>> From: Yaniv Kamay <yaniv@qumranet.com>
>>>>
>>>> Stop cpus before devices when stopping the VM, start cpus after devices
>>>> when starting VM.
>>>>
>>>>   
>>>>         
>>> Why is this needed?
>>>       
>> A vcpu could access a stopped device otherwise. 
>>     
>
> Actually on vm_stop its safe because the order happens to be correct,
> but on vm_start its the other way around (vcpus start first, and they
> should be started last).
>   

Right, applied the patch, thanks.

-- 
error compiling committee.c: too many arguments to function

Re: kvm: qemu: stop/start cpus before/after devices

[Avi Kivity]

Marcelo Tosatti wrote:
> On Wed, Mar 25, 2009 at 11:26:19AM -0300, Marcelo Tosatti wrote:
>   
>> On Wed, Mar 25, 2009 at 01:45:52PM +0200, Avi Kivity wrote:
>>     
>>> Marcelo Tosatti wrote:
>>>       
>>>> From: Yaniv Kamay <yaniv@qumranet.com>
>>>>
>>>> Stop cpus before devices when stopping the VM, start cpus after devices
>>>> when starting VM.
>>>>
>>>>   
>>>>         
>>> Why is this needed?
>>>       
>> A vcpu could access a stopped device otherwise. 
>>     
>
> Actually on vm_stop its safe because the order happens to be correct,
> but on vm_start its the other way around (vcpus start first, and they
> should be started last).
>   

But, we are holding qemu_mutex.  How can vcpus access the devices?


-- 
error compiling committee.c: too many arguments to function

Re: kvm: qemu: stop/start cpus before/after devices

[Marcelo Tosatti]

On Thu, Mar 26, 2009 at 12:03:30PM +0200, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> On Wed, Mar 25, 2009 at 11:26:19AM -0300, Marcelo Tosatti wrote:
>>   
>>> On Wed, Mar 25, 2009 at 01:45:52PM +0200, Avi Kivity wrote:
>>>     
>>>> Marcelo Tosatti wrote:
>>>>       
>>>>> From: Yaniv Kamay <yaniv@qumranet.com>
>>>>>
>>>>> Stop cpus before devices when stopping the VM, start cpus after devices
>>>>> when starting VM.
>>>>>
>>>>>           
>>>> Why is this needed?
>>>>       
>>> A vcpu could access a stopped device otherwise.     
>>
>> Actually on vm_stop its safe because the order happens to be correct,
>> but on vm_start its the other way around (vcpus start first, and they
>> should be started last).
>>   
>
> But, we are holding qemu_mutex.  How can vcpus access the devices?

You're right, they can't. But its not bad to make it explicit, instead
of relying on the locking behaviour?

Re: kvm: qemu: stop/start cpus before/after devices

[Avi Kivity]

Marcelo Tosatti wrote:
> You're right, they can't. But its not bad to make it explicit, instead
> of relying on the locking behaviour?
>   

You could say that we should depend on proper locking instead the order 
of things shutting down and starting up :)

It's not just a cpu that can access a device; another device could dma 
into it, so we need to quiesce the system first (including vcpus) and 
then pause all devices.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.