Skip to contentSkip to conversation

qemu-kvm: Fix guest resetting v2

PATCH Applied Jan Kiszka 2 participants 4 messages May 12, 2008 KVM

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

PATCH 2/2 qemu-kvm: Fix guest resetting v2

Resetting guests used to be racy, deadlock-prone, or simply broken (for SMP). This patch fixes the issues, following Marcelo's suggestion to consolidate the reset activity in the I/O thread. All vcpus are cleanly stopped before the emulated hardware is reset, and kvm_arch_cpu_reset is introduced and invoked to ensure that non-boot cpus are put into the right state on x86. Note that other arch may need to look into this service as well to get SMP reset right.

Moreover, a safety check is added to pause/resume_all_threads to ensure that they are only invoked in I/O thread context.

---
 qemu/qemu-kvm-ia64.c    |    4 ++++
 qemu/qemu-kvm-powerpc.c |    4 ++++
 qemu/qemu-kvm-x86.c     |   16 ++++++++++++++++
 qemu/qemu-kvm.c         |   34 +++++++++++++++++-----------------
 qemu/qemu-kvm.h         |    1 +
 qemu/vl.c               |    6 +-----
 6 files changed, 43 insertions(+), 22 deletions(-)
qemu/qemu-kvm-ia64.c +4 -0
Index: b/qemu/qemu-kvm-ia64.c
===================================================================
@@ -61,3 +61,7 @@ int kvm_arch_try_push_interrupts(void *o
 void kvm_arch_update_regs_for_sipi(CPUState *env)
 {
 }
+
+void kvm_arch_cpu_reset(CPUState *env)
+{
+}
qemu/qemu-kvm-powerpc.c +4 -0
Index: b/qemu/qemu-kvm-powerpc.c
===================================================================
@@ -213,3 +213,7 @@ int handle_powerpc_dcr_write(int vcpu, u
 
     return 0; /* XXX ignore failed DCR ops */
 }
+
+void kvm_arch_cpu_reset(CPUState *env)
+{
+}
qemu/qemu-kvm-x86.c +16 -0
Index: b/qemu/qemu-kvm-x86.c
===================================================================
@@ -671,3 +671,19 @@ int handle_tpr_access(void *opaque, int 
     kvm_tpr_access_report(cpu_single_env, rip, is_write);
     return 0;
 }
+
+void kvm_arch_cpu_reset(CPUState *env)
+{
+    struct kvm_mp_state mp_state = { .mp_state = KVM_MP_STATE_UNINITIALIZED };
+
+    kvm_arch_load_regs(env);
+    if (env->cpu_index != 0) {
+	if (kvm_irqchip_in_kernel(kvm_context))
+	    kvm_set_mpstate(kvm_context, env->cpu_index, &mp_state);
+	else {
+	    env->interrupt_request &= ~CPU_INTERRUPT_HARD;
+	    env->hflags |= HF_HALTED_MASK;
+	    env->exception_index = EXCP_HLT;
+	}
+    }
+}
qemu/qemu-kvm.c +17 -17
Index: b/qemu/qemu-kvm.c
===================================================================
@@ -32,8 +32,6 @@ kvm_context_t kvm_context;
 
 extern int smp_cpus;
 
-static int qemu_kvm_reset_requested;
-
 pthread_mutex_t qemu_mutex = PTHREAD_MUTEX_INITIALIZER;
 pthread_cond_t qemu_aio_cond = PTHREAD_COND_INITIALIZER;
 pthread_cond_t qemu_vcpu_cond = PTHREAD_COND_INITIALIZER;
@@ -53,7 +51,6 @@ struct vcpu_info {
     int signalled;
     int stop;
     int stopped;
-    int reload_regs;
     int created;
 } vcpu_info[256];
 
@@ -251,6 +248,8 @@ static void pause_all_threads(void)
 {
     int i;
 
+    assert(!cpu_single_env);
+
     for (i = 0; i < smp_cpus; ++i) {
 	vcpu_info[i].stop = 1;
 	pthread_kill(vcpu_info[i].thread, SIG_IPI);
@@ -263,6 +262,8 @@ static void resume_all_threads(void)
 {
     int i;
 
+    assert(!cpu_single_env);
+
     for (i = 0; i < smp_cpus; ++i) {
 	vcpu_info[i].stop = 0;
 	vcpu_info[i].stopped = 0;
@@ -307,15 +308,18 @@ static void setup_kernel_sigmask(CPUStat
     kvm_set_signal_mask(kvm_context, env->cpu_index, &set);
 }
 
-void qemu_kvm_system_reset_request(void)
+void qemu_kvm_system_reset(void)
 {
     int i;
 
-    for (i = 0; i < smp_cpus; ++i) {
-	vcpu_info[i].reload_regs = 1;
-	pthread_kill(vcpu_info[i].thread, SIG_IPI);
-    }
+    pause_all_threads();
+
     qemu_system_reset();
+
+    for (i = 0; i < smp_cpus; ++i)
+	kvm_arch_cpu_reset(vcpu_info[i].env);
+
+    resume_all_threads();
 }
 
 static int kvm_main_loop_cpu(CPUState *env)
@@ -348,11 +352,6 @@ static int kvm_main_loop_cpu(CPUState *e
 	    kvm_cpu_exec(env);
 	env->interrupt_request &= ~CPU_INTERRUPT_EXIT;
 	kvm_main_loop_wait(env, 0);
-        if (info->reload_regs) {
-	    info->reload_regs = 0;
-	    if (env->cpu_index == 0) /* ap needs to be placed in INIT */
-		kvm_arch_load_regs(env);
-	}
     }
     pthread_mutex_unlock(&qemu_mutex);
     return 0;
@@ -535,10 +534,8 @@ int kvm_main_loop(void)
             break;
         else if (qemu_powerdown_requested())
             qemu_system_powerdown();
-        else if (qemu_reset_requested()) {
-            pthread_kill(vcpu_info[0].thread, SIG_IPI);
-            qemu_kvm_reset_requested = 1;
-        }
+        else if (qemu_reset_requested())
+	    qemu_kvm_system_reset();
     }
 
     pause_all_threads();
@@ -647,6 +644,9 @@ static int kvm_halt(void *opaque, int vc
 
 static int kvm_shutdown(void *opaque, int vcpu)
 {
+    /* stop the current vcpu from going back to guest mode */
+    vcpu_info[cpu_single_env->cpu_index].stopped = 1;
+
     qemu_system_reset_request();
     return 1;
 }
qemu/qemu-kvm.h +1 -0
Index: b/qemu/qemu-kvm.h
===================================================================
@@ -59,6 +59,7 @@ void kvm_arch_post_kvm_run(void *opaque,
 int kvm_arch_has_work(CPUState *env);
 int kvm_arch_try_push_interrupts(void *opaque);
 void kvm_arch_update_regs_for_sipi(CPUState *env);
+void kvm_arch_cpu_reset(CPUState *env);
 
 CPUState *qemu_kvm_cpu_env(int index);
 
qemu/vl.c +1 -5
Index: b/qemu/vl.c
===================================================================
@@ -7302,11 +7302,7 @@ void qemu_system_reset_request(void)
     }
     if (cpu_single_env)
         cpu_interrupt(cpu_single_env, CPU_INTERRUPT_EXIT);
-#ifdef USE_KVM
-    if (kvm_allowed)
-        if (!no_reboot)
-            qemu_kvm_system_reset_request();
-#endif
+    main_loop_break();
 }
 
 void qemu_system_shutdown_request(void)

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

#1 lore raw
Quoted 9 lines · Jan Kiszka

Resetting guests used to be racy, deadlock-prone, or simply broken (for SMP). This patch fixes the issues, following Marcelo's suggestion to consolidate the reset activity in the I/O thread. All vcpus are cleanly stopped before the emulated hardware is reset, and kvm_arch_cpu_reset is introduced and invoked to ensure that non-boot cpus are put into the right state on x86. Note that other arch may need to look into this service as well to get SMP reset right.

hmm. This means that reset is executed asynchronously of the calling cpu.

Well, in a real machine a keyboard controller reset is asynchronous as well, so I guess that's not an issue.

Signature · 8 lines
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
author
#2 lore raw
Quoted 11 lines · Avi Kivity
Quoted 9 lines · Jan Kiszka

Resetting guests used to be racy, deadlock-prone, or simply broken (for SMP). This patch fixes the issues, following Marcelo's suggestion to consolidate the reset activity in the I/O thread. All vcpus are cleanly stopped before the emulated hardware is reset, and kvm_arch_cpu_reset is introduced and invoked to ensure that non-boot cpus are put into the right state on x86. Note that other arch may need to look into this service as well to get SMP reset right.

Quoted 1 line

hmm. This means that reset is executed asynchronously of the calling cpu.

Nope, the calling cpu is put in stopped state after informing the I/O thread about the reset request. We just cannot control when the request is handled /wrt to other cpus, but that's not different from real life I guess.

Jan

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

#3 lore raw
Quoted 14 lines · Jan Kiszka
Quoted 12 lines · Avi Kivity
Quoted 10 lines · Jan Kiszka

Resetting guests used to be racy, deadlock-prone, or simply broken (for SMP). This patch fixes the issues, following Marcelo's suggestion to consolidate the reset activity in the I/O thread. All vcpus are cleanly stopped before the emulated hardware is reset, and kvm_arch_cpu_reset is introduced and invoked to ensure that non-boot cpus are put into the right state on x86. Note that other arch may need to look into this service as well to get SMP reset right.

Quoted 8 lines
Quoted 2 lines

hmm. This means that reset is executed asynchronously of the calling cpu.

Nope, the calling cpu is put in stopped state after informing the I/O thread about the reset request. We just cannot control when the request is handled /wrt to other cpus, but that's not different from real life I guess.

That's true for triple-faults, but not for ordinary keyboard controller resets. All you do there is call main_loop_break().

But I think that's fine; on a real machine the keyboard controller reset is processed by a separate processor (the keyboard controller) which has nonzero latency.

Signature · 8 lines
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone