Re: [PATCH] qemu-kvm: fix guest resetting

[Jan Kiszka <jan.kiszka@web.de>]

Anthony Liguori wrote:
> Marcelo Tosatti wrote:
>> diff --git a/qemu/qemu-kvm-ia64.c b/qemu/qemu-kvm-ia64.c
>> index 4d0ddd7..d227d22 100644
>> --- a/qemu/qemu-kvm-ia64.c
>> +++ b/qemu/qemu-kvm-ia64.c
>> @@ -61,3 +61,7 @@ int kvm_arch_try_push_interrupts(void *opaque)
>>  void kvm_arch_update_regs_for_sipi(CPUState *env)
>>  {
>>  }
>> +
>> +void kvm_arch_cpu_reset(CPUState *env)
>> +{
>> +}
>> diff --git a/qemu/qemu-kvm-powerpc.c b/qemu/qemu-kvm-powerpc.c
>> index 14ed945..024b18c 100644
>> --- a/qemu/qemu-kvm-powerpc.c
>> +++ b/qemu/qemu-kvm-powerpc.c
>> @@ -213,3 +213,7 @@ int handle_powerpc_dcr_write(int vcpu, uint32_t dcrn, uint32_t data)
>>  
>>      return 0; /* XXX ignore failed DCR ops */
>>  }
>> +
>> +void kvm_arch_cpu_reset(CPUState *env)
>> +{
>> +}
>> diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
>> index 9a771ff..28eb5c2 100644
>> --- a/qemu/qemu-kvm-x86.c
>> +++ b/qemu/qemu-kvm-x86.c
>> @@ -689,3 +689,19 @@ int handle_tpr_access(void *opaque, int vcpu,
>>      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;
>> +	}
>> +    }
>> +}
>> diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
>> index 3cc6d8e..1e1f065 100644
>> --- a/qemu/qemu-kvm.c
>> +++ b/qemu/qemu-kvm.c
>> @@ -31,8 +31,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;
>> @@ -52,7 +50,6 @@ struct vcpu_info {
>>      int signalled;
>>      int stop;
>>      int stopped;
>> -    int reload_regs;
>>      int created;
>>  } vcpu_info[256];
>>  
>> @@ -242,21 +239,29 @@ static void pause_all_threads(void)
>>  {
>>      int i;
>>  
>> +    if (cpu_single_env) {
>> +        fprintf(stderr, "qemu-kvm: pause_all_threads from vcpu context\n");
>> +        exit(0);
>> +    }
>> +
>>      for (i = 0; i < smp_cpus; ++i) {
>>  	vcpu_info[i].stop = 1;
>>  	pthread_kill(vcpu_info[i].thread, SIG_IPI);
>>      }
>> -    while (!all_threads_paused()) {
>> -	CPUState *env = cpu_single_env;
>> +    while (!all_threads_paused())
>>  	pthread_cond_wait(&qemu_pause_cond, &qemu_mutex);
>> -	cpu_single_env = env;
>> -    }
>> +    cpu_single_env = NULL;
>>  }
>>   
> 
> Personally, I prefer it the old way.  All of the open-coded 
> cpu_single_env's are tough to understand and I believe error-prone.  I 
> think a strategy of explicitly preserving cpu_single_env whenever we 
> drop qemu_mutex is more robust.  Explicitly setting cpu_single_env = 
> NULL happens to work because this is only called from the io thread.  
> It's less clear to a casual reader why it's necessary.
> 
> In fact, I'd be much more inclined to see a wrapper around 
> pthread_cond_wait() so that we never explicitly had to set cpu_single_env.

You mean something like this?
(Hunks may have some offsets, the patch is from the middle of my queue.)

Marcelo, I'm fine with your approach. I even switched my
vm_stop-on-breakpoint patch to this pattern, and it seems to work
nicely. Will roll out an updated series later (but probably not today -
weather is too fine here 8)).

---
 qemu/qemu-kvm.c |   32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

Index: b/qemu/qemu-kvm.c
===================================================================
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -12,6 +12,7 @@ int kvm_allowed = 1;
 int kvm_irqchip = 1;
 int kvm_pit = 1;
 
+#include <assert.h>
 #include <string.h>
 #include "hw/hw.h"
 #include "sysemu.h"
@@ -65,6 +66,14 @@ static inline unsigned long kvm_get_thre
     return syscall(SYS_gettid);
 }
 
+static void qemu_cond_wait(pthread_cond_t *cond)
+{
+    CPUState *env = cpu_single_env;
+
+    pthread_cond_wait(cond, &qemu_mutex);
+    cpu_single_env = env;
+}
+
 CPUState *qemu_kvm_cpu_env(int index)
 {
     return vcpu_info[index].env;
@@ -247,21 +256,22 @@ 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);
     }
-    while (!all_threads_paused()) {
-	CPUState *env = cpu_single_env;
-	pthread_cond_wait(&qemu_pause_cond, &qemu_mutex);
-	cpu_single_env = env;
-    }
+    while (!all_threads_paused())
+	qemu_cond_wait(&qemu_pause_cond);
 }
 
 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;
@@ -380,7 +390,7 @@ static void *ap_main_loop(void *_env)
 
     /* and wait for machine initialization */
     while (!qemu_system_ready)
-	pthread_cond_wait(&qemu_system_cond, &qemu_mutex);
+	qemu_cond_wait(&qemu_system_cond);
     pthread_mutex_unlock(&qemu_mutex);
 
     kvm_main_loop_cpu(env);
@@ -392,7 +402,7 @@ void kvm_init_new_ap(int cpu, CPUState *
     pthread_create(&vcpu_info[cpu].thread, NULL, ap_main_loop, env);
 
     while (vcpu_info[cpu].created == 0)
-	pthread_cond_wait(&qemu_vcpu_cond, &qemu_mutex);
+	qemu_cond_wait(&qemu_vcpu_cond);
 }
 
 int kvm_init_ap(void)
@@ -901,8 +911,6 @@ void qemu_kvm_aio_wait_start(void)
 
 void qemu_kvm_aio_wait(void)
 {
-    CPUState *cpu_single = cpu_single_env;
-
     if (!cpu_single_env) {
 	if (io_thread_sigfd != -1) {
 	    fd_set rfds;
@@ -919,10 +927,8 @@ void qemu_kvm_aio_wait(void)
 		sigfd_handler((void *)(unsigned long)io_thread_sigfd);
 	}
 	qemu_aio_poll();
-    } else {
-        pthread_cond_wait(&qemu_aio_cond, &qemu_mutex);
-        cpu_single_env = cpu_single;
-    }
+    } else
+        qemu_cond_wait(&qemu_aio_cond);
 }
 
 void qemu_kvm_aio_wait_end(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