[Qemu-devel] [6728] Fix race condition on access to env->interrupt_request

[Aurelien Jarno <aurelien@aurel32.net>]

Revision: 6728
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6728
Author:   aurel32
Date:     2009-03-06 21:48:00 +0000 (Fri, 06 Mar 2009)
Log Message:
-----------
Fix race condition on access to env->interrupt_request

env->interrupt_request is accessed as the bit level from both main code
and signal handler, making a race condition possible even on CISC CPU.
This causes freeze of QEMU under high load when running the dyntick
clock.

The patch below move the bit corresponding to CPU_INTERRUPT_EXIT in a
separate variable, declared as volatile sig_atomic_t, so it should be
work even on RISC CPU.

We may want to move the cpu_interrupt(env, CPU_INTERRUPT_EXIT) case in
its own function and get rid of CPU_INTERRUPT_EXIT. That can be done
later, I wanted to keep the patch short for easier review.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

Modified Paths:
--------------
    trunk/cpu-defs.h
    trunk/cpu-exec.c
    trunk/exec.c
    trunk/kvm-all.c

Modified: trunk/cpu-defs.h
===================================================================
--- trunk/cpu-defs.h	2009-03-06 20:27:40 UTC (rev 6727)
+++ trunk/cpu-defs.h	2009-03-06 21:48:00 UTC (rev 6728)
@@ -27,6 +27,7 @@
 #include "config.h"
 #include <setjmp.h>
 #include <inttypes.h>
+#include <signal.h>
 #include "osdep.h"
 #include "sys-queue.h"
 
@@ -170,6 +171,7 @@
                                      memory was accessed */             \
     uint32_t halted; /* Nonzero if the CPU is in suspend state */       \
     uint32_t interrupt_request;                                         \
+    volatile sig_atomic_t exit_request;                                 \
     /* The meaning of the MMU modes is defined in the target code. */   \
     CPUTLBEntry tlb_table[NB_MMU_MODES][CPU_TLB_SIZE];                  \
     target_phys_addr_t iotlb[NB_MMU_MODES][CPU_TLB_SIZE];               \

Modified: trunk/cpu-exec.c
===================================================================
--- trunk/cpu-exec.c	2009-03-06 20:27:40 UTC (rev 6727)
+++ trunk/cpu-exec.c	2009-03-06 21:48:00 UTC (rev 6728)
@@ -311,7 +311,7 @@
                 env->exception_index = -1;
             }
 #ifdef USE_KQEMU
-            if (kqemu_is_ok(env) && env->interrupt_request == 0) {
+            if (kqemu_is_ok(env) && env->interrupt_request == 0 && env->exit_request == 0) {
                 int ret;
                 env->eflags = env->eflags | helper_cc_compute_all(CC_OP) | (DF & DF_MASK);
                 ret = kqemu_cpu_exec(env);
@@ -326,7 +326,7 @@
                 } else if (ret == 2) {
                     /* softmmu execution needed */
                 } else {
-                    if (env->interrupt_request != 0) {
+                    if (env->interrupt_request != 0 || env->exit_request != 0) {
                         /* hardware interrupt will be executed just after */
                     } else {
                         /* otherwise, we restart */
@@ -525,12 +525,12 @@
                            the program flow was changed */
                         next_tb = 0;
                     }
-                    if (interrupt_request & CPU_INTERRUPT_EXIT) {
-                        env->interrupt_request &= ~CPU_INTERRUPT_EXIT;
-                        env->exception_index = EXCP_INTERRUPT;
-                        cpu_loop_exit();
-                    }
                 }
+                if (unlikely(env->exit_request)) {
+                    env->exit_request = 0;
+                    env->exception_index = EXCP_INTERRUPT;
+                    cpu_loop_exit();
+                }
 #ifdef DEBUG_EXEC
                 if (qemu_loglevel_mask(CPU_LOG_TB_CPU)) {
                     /* restore flags in standard format */
@@ -599,7 +599,7 @@
                    TB, but before it is linked into a potentially
                    infinite loop and becomes env->current_tb. Avoid
                    starting execution if there is a pending interrupt. */
-                if (unlikely (env->interrupt_request & CPU_INTERRUPT_EXIT))
+                if (unlikely (env->exit_request))
                     env->current_tb = NULL;
 
                 while (env->current_tb) {

Modified: trunk/exec.c
===================================================================
--- trunk/exec.c	2009-03-06 20:27:40 UTC (rev 6727)
+++ trunk/exec.c	2009-03-06 21:48:00 UTC (rev 6728)
@@ -1501,9 +1501,12 @@
 #endif
     int old_mask;
 
+    if (mask & CPU_INTERRUPT_EXIT) {
+        env->exit_request = 1;
+        mask &= ~CPU_INTERRUPT_EXIT;
+    }
+
     old_mask = env->interrupt_request;
-    /* FIXME: This is probably not threadsafe.  A different thread could
-       be in the middle of a read-modify-write operation.  */
     env->interrupt_request |= mask;
 #if defined(USE_NPTL)
     /* FIXME: TB unchaining isn't SMP safe.  For now just ignore the
@@ -1514,10 +1517,8 @@
     if (use_icount) {
         env->icount_decr.u16.high = 0xffff;
 #ifndef CONFIG_USER_ONLY
-        /* CPU_INTERRUPT_EXIT isn't a real interrupt.  It just means
-           an async event happened and we need to process it.  */
         if (!can_do_io(env)
-            && (mask & ~(old_mask | CPU_INTERRUPT_EXIT)) != 0) {
+            && (mask & ~old_mask) != 0) {
             cpu_abort(env, "Raised interrupt while not in I/O function");
         }
 #endif

Modified: trunk/kvm-all.c
===================================================================
--- trunk/kvm-all.c	2009-03-06 20:27:40 UTC (rev 6727)
+++ trunk/kvm-all.c	2009-03-06 21:48:00 UTC (rev 6728)
@@ -445,7 +445,7 @@
     do {
         kvm_arch_pre_run(env, run);
 
-        if ((env->interrupt_request & CPU_INTERRUPT_EXIT)) {
+        if (env->exit_request) {
             dprintf("interrupt exit requested\n");
             ret = 0;
             break;
@@ -512,8 +512,8 @@
         }
     } while (ret > 0);
 
-    if ((env->interrupt_request & CPU_INTERRUPT_EXIT)) {
-        env->interrupt_request &= ~CPU_INTERRUPT_EXIT;
+    if (env->exit_request) {
+        env->exit_request = 0;
         env->exception_index = EXCP_INTERRUPT;
     }