[Qemu-devel] Use per-cpu reset handlers.

[Glauber Costa <glommer@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 583 bytes --]

The proposal in this patch is to add a system_reset caller that only
resets state related to the cpu. This will guarantee that does functions
are called from the cpu-threads, not the I/O thread.

In principle, it might seem close to the remote execution mechanism, but:
 * It does not involve any extra signalling, so it should be faster.
 * The cpu is guaranteed to be stopped, so it is much less racy.
 * What runs where becomes more explicit.
 * This is much, much less racy

The previous implementation was giving me races on reset. This one makes
it work flawlesly w.r.t reset.

[-- Attachment #2: 0001-Use-per-cpu-reset-handlers.patch --]
[-- Type: application/octet-stream, Size: 7664 bytes --]

From ccb2fd51aa8b181c7c9ac5266d439e173f360295 Mon Sep 17 00:00:00 2001
From: Glauber Costa <glommer@redhat.com>
Date: Fri, 27 Nov 2009 14:34:38 -0200
Subject: [PATCH] Use per-cpu reset handlers.

The proposal in this patch is to add a system_reset caller that only
resets state related to the cpu. This will guarantee that does functions
are called from the cpu-threads, not the I/O thread.

In principle, it might seem close to the remote execution mechanism, but:
 * It does not involve any extra signalling, so it should be faster.
 * The cpu is guaranteed to be stopped, so it is much less racy.
 * What runs where becomes more explicit.
 * This is much, much less racy

The previous implementation was giving me races on reset. This one makes
it work flawlesly w.r.t reset.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 cpu-defs.h    |    2 ++
 exec-all.h    |   12 ++++++++++++
 exec.c        |   32 ++++++++++++++++++++++++++++++++
 hw/apic-kvm.c |   18 +++++++++---------
 kvm-all.c     |    7 +++----
 vl.c          |   10 ++++++++++
 6 files changed, 68 insertions(+), 13 deletions(-)

diff --git a/cpu-defs.h b/cpu-defs.h
index 18792fc..37fd11c 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -185,6 +185,8 @@ typedef struct QemuWorkItem {
     CPUWatchpoint *watchpoint_hit;                                      \
                                                                         \
     QemuWorkItem queued_work;                                           \
+    int reset_requested;                                                \
+    QTAILQ_HEAD(reset_head, CPUResetEntry) reset_handlers;              \
     uint64_t queued_local, queued_total;                                \
     struct QemuMutex queue_lock;                                        \
                                                                         \
diff --git a/exec-all.h b/exec-all.h
index dd134a9..f6767ab 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -78,6 +78,18 @@ void cpu_io_recompile(CPUState *env, void *retaddr);
 TranslationBlock *tb_gen_code(CPUState *env, 
                               target_ulong pc, target_ulong cs_base, int flags,
                               int cflags);
+
+typedef void CPUResetHandler(CPUState *env);
+typedef struct CPUResetEntry {
+    QTAILQ_ENTRY(CPUResetEntry) entry;
+    CPUResetHandler *func;
+    void *opaque;
+} CPUResetEntry;
+
+void qemu_register_reset_cpu(CPUState *env, CPUResetHandler *func);
+void qemu_unregister_reset_cpu(CPUState *env, CPUResetHandler *func);
+
+void qemu_cpu_reset(CPUState *env);
 void cpu_exec_init(CPUState *env);
 void QEMU_NORETURN cpu_loop_exit(void);
 int page_unprotect(target_ulong address, unsigned long pc, void *puc);
diff --git a/exec.c b/exec.c
index 076d26b..6be3332 100644
--- a/exec.c
+++ b/exec.c
@@ -588,6 +588,7 @@ void cpu_exec_init(CPUState *env)
     env->numa_node = 0;
     QTAILQ_INIT(&env->breakpoints);
     QTAILQ_INIT(&env->watchpoints);
+    QTAILQ_INIT(&env->reset_handlers);
     *penv = env;
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
@@ -599,6 +600,37 @@ void cpu_exec_init(CPUState *env)
 #endif
 }
 
+void qemu_register_reset_cpu(CPUState *env, CPUResetHandler *func)
+{
+    CPUResetEntry *re = qemu_mallocz(sizeof(CPUResetEntry));
+    
+    re->func = func;
+    re->opaque = env;
+    QTAILQ_INSERT_TAIL(&env->reset_handlers, re, entry);
+}
+
+void qemu_unregister_reset_cpu(CPUState *env, CPUResetHandler *func)
+{
+    CPUResetEntry *re;
+    
+    QTAILQ_FOREACH(re, &env->reset_handlers, entry) {
+        if (re->func == func && re->opaque == env) {
+            QTAILQ_REMOVE(&env->reset_handlers, re, entry);
+            qemu_free(re);
+            return;
+        }
+    }
+}
+
+void qemu_cpu_reset(CPUState *env)
+{   
+    CPUResetEntry *re, *nre;
+    /* reset all devices */                                                          
+    QTAILQ_FOREACH_SAFE(re, &env->reset_handlers, entry, nre) {                           
+        re->func(re->opaque);
+    }                                                                                
+} 
+
 static inline void invalidate_page_bitmap(PageDesc *p)
 {
     if (p->code_bitmap) {
diff --git a/hw/apic-kvm.c b/hw/apic-kvm.c
index dc61386..abe3ee0 100644
--- a/hw/apic-kvm.c
+++ b/hw/apic-kvm.c
@@ -89,20 +89,20 @@ static const VMStateDescription vmstate_apic = {
     .post_load = apic_post_load,
 };
 
-static void kvm_apic_reset(void *opaque)
+static void kvm_apic_reset(CPUState *env)
 {
-    APICState *s = opaque;
+    APICState *s = env->apic_state;
     int bsp;
     int i;
 
-    cpu_synchronize_state(s->cpu_env);
+    cpu_synchronize_state(env);
 
-    bsp = cpu_is_bsp(s->cpu_env);
+    bsp = cpu_is_bsp(env);
 
     s->dev.apicbase = 0xfee00000 |
         (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
 
-    cpu_reset(s->cpu_env);
+    cpu_reset(env);
 
     s->dev.tpr = 0;
     s->dev.spurious_vec = 0xff;
@@ -123,13 +123,13 @@ static void kvm_apic_reset(void *opaque)
     s->dev.wait_for_sipi = 1;
 
 
-    s->cpu_env->mp_state
+    env->mp_state
             = bsp ? KVM_MP_STATE_RUNNABLE : KVM_MP_STATE_UNINITIALIZED;
 
     /* We have to tell the kernel about mp_state, but also save sregs, since
      * apic base was just updated
      */
-    kvm_arch_put_registers(s->cpu_env);
+    kvm_arch_put_registers(env);
 
     if (bsp) {
         /*
@@ -139,7 +139,7 @@ static void kvm_apic_reset(void *opaque)
          */
         s->dev.lvt[APIC_LVT_LINT0] = 0x700;
     }
-    kvm_set_lapic(s->cpu_env, &s->kvm_lapic_state);
+    kvm_set_lapic(env, &s->kvm_lapic_state);
 }
 
 int kvm_apic_init(CPUState *env)
@@ -153,7 +153,7 @@ int kvm_apic_init(CPUState *env)
     msix_supported = 1;
 
     vmstate_register(s->dev.id, &vmstate_apic, s);
-    qemu_register_reset(kvm_apic_reset, s);
+    qemu_register_reset_cpu(env, kvm_apic_reset);
 
     return 0;
 }
diff --git a/kvm-all.c b/kvm-all.c
index b73fbaa..7e97100 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -21,6 +21,7 @@
 #include <linux/kvm.h>
 
 #include "qemu-common.h"
+#include "exec-all.h"
 #include "sysemu.h"
 #include "hw/hw.h"
 #include "gdbstub.h"
@@ -148,10 +149,8 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
     return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
 }
 
-static void kvm_reset_vcpu(void *opaque)
+static void kvm_reset_vcpu(CPUState *env)
 {
-    CPUState *env = opaque;
-
     kvm_arch_reset_vcpu(env);
     if (kvm_arch_put_registers(env)) {
         fprintf(stderr, "Fatal: kvm vcpu reset failed\n");
@@ -203,7 +202,7 @@ int kvm_init_vcpu(CPUState *env)
 
     ret = kvm_arch_init_vcpu(env);
     if (ret == 0) {
-        qemu_register_reset(kvm_reset_vcpu, env);
+        qemu_register_reset_cpu(env, kvm_reset_vcpu);
     }
 err:
     return ret;
diff --git a/vl.c b/vl.c
index 666bf00..696f196 100644
--- a/vl.c
+++ b/vl.c
@@ -3234,11 +3234,17 @@ void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
 void qemu_system_reset(void)
 {
     QEMUResetEntry *re, *nre;
+    CPUState *penv = first_cpu;
 
     /* reset all devices */
     QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
         re->func(re->opaque);
     }
+
+    while (penv) {
+        penv->reset_requested = 1;
+        penv = penv->next_cpu;
+    }
 }
 
 void qemu_system_reset_request(void)
@@ -3530,6 +3536,10 @@ static void *kvm_cpu_thread_fn(void *arg)
     }
 
     while (1) {
+        if (env->reset_requested) {
+            qemu_cpu_reset(env);
+            env->reset_requested = 0;
+        }
         if (cpu_can_run(env))
             qemu_cpu_exec(env);
         qemu_wait_io_event(env);
-- 
1.6.5.2