| Message ID | 20090324171652.GA3655@amt.cnet (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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?
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. -- 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
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). -- 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
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.
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?
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? -- 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
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.
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); } }