From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=36288 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pi5rg-0000Ww-09 for qemu-devel@nongnu.org; Wed, 26 Jan 2011 08:57:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pi5rA-0002M0-0F for qemu-devel@nongnu.org; Wed, 26 Jan 2011 08:56:51 -0500 Received: from david.siemens.de ([192.35.17.14]:21155) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pi5r9-0002LC-Ma for qemu-devel@nongnu.org; Wed, 26 Jan 2011 08:56:19 -0500 Message-ID: <4D4027FE.8090808@siemens.com> Date: Wed, 26 Jan 2011 14:56:14 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <1296034784-6513-1-git-send-email-stefanha@linux.vnet.ibm.com> <4D401DA4.9020307@siemens.com> In-Reply-To: <4D401DA4.9020307@siemens.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH] kvm: Prevent dynticks race condition for !CONFIG_IOTHREAD List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Marcelo Tosatti , qemu-devel@nongnu.org, Avi Kivity On 2011-01-26 14:12, Jan Kiszka wrote: > On 2011-01-26 10:39, Stefan Hajnoczi wrote: >> The dynticks timer arranges for SIGALRM to be raised when the next >> pending timer expires. When building with !CONFIG_IOTHREAD, we need to >> check whether a request to exit the vcpu is pending before re-entering >> the guest. >> >> Unfortunately there is a race condition here because SIGALRM may be >> raised after we check for an exit request but before re-entering the >> guest. In that case the guest is re-entered without the dynticks timer >> being rearmed. >> >> This results in temporary loss of timers until some other event forces a >> vmexit. In the case of a CPU-bound guest it can cause softlockups. >> >> This patch blocks SIGALRM before checking for an exit request and uses >> KVM's sigmask support to atomically unblock it when entering the guest, >> thereby making the exit request check safe. >> >> Signed-off-by: Stefan Hajnoczi >> --- >> cpus.c | 17 ++++++++++++++++- >> kvm-all.c | 16 ++++++++++++++++ >> 2 files changed, 32 insertions(+), 1 deletions(-) >> >> Does not affect qemu-kvm.git. Still worth having in qemu.git so we don't get >> odd behavior when building without --enable-io-thread. >> >> diff --git a/cpus.c b/cpus.c >> index 0309189..59dbfab 100644 >> --- a/cpus.c >> +++ b/cpus.c >> @@ -262,14 +262,29 @@ void qemu_main_loop_start(void) >> { >> } >> >> +static void kvm_init_sigmask(CPUState *env) >> +{ >> + int r; >> + sigset_t set; >> + >> + pthread_sigmask(SIG_SETMASK, NULL, &set); >> + r = kvm_set_signal_mask(env, &set); >> + if (r) { >> + fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(r)); >> + exit(1); >> + } >> +} >> + >> void qemu_init_vcpu(void *_env) >> { >> CPUState *env = _env; >> >> env->nr_cores = smp_cores; >> env->nr_threads = smp_threads; >> - if (kvm_enabled()) >> + if (kvm_enabled()) { >> kvm_init_vcpu(env); >> + kvm_init_sigmask(env); >> + } >> return; >> } >> >> diff --git a/kvm-all.c b/kvm-all.c >> index 255b6fa..9cc2553 100644 >> --- a/kvm-all.c >> +++ b/kvm-all.c >> @@ -890,19 +890,31 @@ int kvm_cpu_exec(CPUState *env) >> { >> struct kvm_run *run = env->kvm_run; >> int ret; >> +#ifndef CONFIG_IOTHREAD >> + sigset_t set, old_set; >> + >> + sigemptyset(&set); >> + sigaddset(&set, SIGALRM); >> +#endif >> >> DPRINTF("kvm_cpu_exec()\n"); >> >> do { >> #ifndef CONFIG_IOTHREAD >> + pthread_sigmask(SIG_BLOCK, &set, &old_set); >> + >> if (env->exit_request) { >> DPRINTF("interrupt exit requested\n"); >> + pthread_sigmask(SIG_SETMASK, &old_set, NULL); >> ret = 0; >> break; >> } >> #endif >> >> if (kvm_arch_process_irqchip_events(env)) { >> +#ifndef CONFIG_IOTHREAD >> + pthread_sigmask(SIG_SETMASK, &old_set, NULL); >> +#endif >> ret = 0; >> break; >> } >> @@ -920,6 +932,10 @@ int kvm_cpu_exec(CPUState *env) >> cpu_single_env = env; >> kvm_arch_post_run(env, run); >> >> +#ifndef CONFIG_IOTHREAD >> + pthread_sigmask(SIG_SETMASK, &old_set, NULL); >> +#endif >> + >> if (ret == -EINTR || ret == -EAGAIN) { >> cpu_exit(env); >> DPRINTF("io window exit\n"); > > Good catch, but the code should be organized differently, probably with > the help of signalfd which we need for SIGBUS anyway. I'm currently > reworking signaling bits. I will pick this up and integrate a > corresponding solution in my queue. Will keep you posted! > Looks like this should also include SIGIO in case hpet is selected as host timer. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux