From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=43487 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pi5Ae-0006sn-CW for qemu-devel@nongnu.org; Wed, 26 Jan 2011 08:12:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pi5Ad-0000gN-5T for qemu-devel@nongnu.org; Wed, 26 Jan 2011 08:12:24 -0500 Received: from thoth.sbs.de ([192.35.17.2]:21320) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pi5Ac-0000fV-SV for qemu-devel@nongnu.org; Wed, 26 Jan 2011 08:12:23 -0500 Message-ID: <4D401DA4.9020307@siemens.com> Date: Wed, 26 Jan 2011 14:12:04 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <1296034784-6513-1-git-send-email-stefanha@linux.vnet.ibm.com> In-Reply-To: <1296034784-6513-1-git-send-email-stefanha@linux.vnet.ibm.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 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! Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux