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

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

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

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

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 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(-)

Index: b/qemu/qemu-kvm-ia64.c
===================================================================
--- a/qemu/qemu-kvm-ia64.c
+++ 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)
+{
+}
Index: b/qemu/qemu-kvm-powerpc.c
===================================================================
--- a/qemu/qemu-kvm-powerpc.c
+++ 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)
+{
+}
Index: b/qemu/qemu-kvm-x86.c
===================================================================
--- a/qemu/qemu-kvm-x86.c
+++ 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;
+	}
+    }
+}
Index: b/qemu/qemu-kvm.c
===================================================================
--- a/qemu/qemu-kvm.c
+++ 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;
 }
Index: b/qemu/qemu-kvm.h
===================================================================
--- a/qemu/qemu-kvm.h
+++ 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);
 
Index: b/qemu/vl.c
===================================================================
--- a/qemu/vl.c
+++ 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

Re: [PATCH 2/2] qemu-kvm: Fix guest resetting v2

[Avi Kivity]

Jan Kiszka wrote:
> 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.

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

Re: [PATCH 2/2] qemu-kvm: Fix guest resetting v2

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 861 bytes --]

Avi Kivity wrote:
> Jan Kiszka wrote:
>> 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.

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

[-- Attachment #2: Type: text/plain, Size: 320 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 158 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH 2/2] qemu-kvm: Fix guest resetting v2

[Avi Kivity]

Jan Kiszka wrote:
> Avi Kivity wrote:
>   
>> Jan Kiszka wrote:
>>     
>>> 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.
>>     
>
> 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.

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