From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Lfhtj-00021l-Mz for qemu-devel@nongnu.org; Fri, 06 Mar 2009 16:48:03 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Lfhti-00021K-P8 for qemu-devel@nongnu.org; Fri, 06 Mar 2009 16:48:02 -0500 Received: from [199.232.76.173] (port=54872 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Lfhti-00021H-IC for qemu-devel@nongnu.org; Fri, 06 Mar 2009 16:48:02 -0500 Received: from savannah.gnu.org ([199.232.41.3]:41713 helo=sv.gnu.org) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Lfhti-0001Na-6C for qemu-devel@nongnu.org; Fri, 06 Mar 2009 16:48:02 -0500 Received: from cvs.savannah.gnu.org ([199.232.41.69]) by sv.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Lfhth-0005iE-AA for qemu-devel@nongnu.org; Fri, 06 Mar 2009 21:48:01 +0000 Received: from aurel32 by cvs.savannah.gnu.org with local (Exim 4.69) (envelope-from ) id 1Lfhtg-0005iA-Rk for qemu-devel@nongnu.org; Fri, 06 Mar 2009 21:48:01 +0000 MIME-Version: 1.0 Errors-To: aurel32 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From: Aurelien Jarno Message-Id: Date: Fri, 06 Mar 2009 21:48:00 +0000 Subject: [Qemu-devel] [6728] Fix race condition on access to env->interrupt_request Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org 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 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 #include +#include #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; }