Skip to contentSkip to conversation

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

PATCH Marcelo Tosatti 2 participants 8 messages Mar 24, 2009 – Mar 26, 2009 KVM

lore
Share & tools
lore
#0 lore raw

From: Yaniv Kamay <[email protected]>

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

qemu/qemu-kvm.c +5 -14
index 93af6ea..4164368 100644
@@ -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;
qemu/qemu-kvm.h +3 -0
index c0549df..ca59af8 100644
@@ -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);
 
qemu/vl.c +4 -0
index 7ae266e..c52d2d7 100644
@@ -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);
     }
 }
#1 lore raw
Quoted 6 lines · Marcelo Tosatti

From: Yaniv Kamay <[email protected]>

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

Why is this needed?

Signature · 1 line
error compiling committee.c: too many arguments to function
#2 lore raw
Quoted 9 lines · Avi Kivity
Quoted 6 lines · Marcelo Tosatti

From: Yaniv Kamay <[email protected]>

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.

#3 lore raw
Quoted 4 lines · Marcelo Tosatti
Quoted 3 lines · Avi Kivity
Quoted 2 lines · Marcelo Tosatti

From: Yaniv Kamay <[email protected]>

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

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).

#4 lore raw
Quoted 13 lines · Marcelo Tosatti
Quoted 11 lines · Marcelo Tosatti
Quoted 9 lines · Avi Kivity
Quoted 7 lines · Marcelo Tosatti

From: Yaniv Kamay <[email protected]>

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

Quoted 9 lines
Quoted 4 lines
Quoted 2 lines

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.

Signature · 1 line
error compiling committee.c: too many arguments to function
#5 lore raw
In reply to Marcelo Tosatti
Quoted 13 lines · Marcelo Tosatti
Quoted 11 lines · Marcelo Tosatti
Quoted 9 lines · Avi Kivity
Quoted 7 lines · Marcelo Tosatti

From: Yaniv Kamay <[email protected]>

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

Quoted 9 lines
Quoted 4 lines
Quoted 2 lines

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?

Signature · 1 line
error compiling committee.c: too many arguments to function
#6 lore raw
Quoted 15 lines · Avi Kivity
Quoted 14 lines · Marcelo Tosatti
Quoted 12 lines · Marcelo Tosatti
Quoted 10 lines · Avi Kivity
Quoted 6 lines · Marcelo Tosatti

From: Yaniv Kamay <[email protected]>

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

Why is this needed?

Quoted 8 lines
Quoted 6 lines
Quoted 1 line

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?

#7 lore raw
Quoted 3 lines · Marcelo Tosatti

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.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.