From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH 14/22] kvm: Fix race between timer signals and vcpu entry under !IOTHREAD Date: Tue, 1 Feb 2011 10:47:07 -0200 Message-ID: <20110201124707.GA12061@amt.cnet> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Avi Kivity , kvm@vger.kernel.org, qemu-devel@nongnu.org, Stefan Hajnoczi To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:29931 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753113Ab1BANPH (ORCPT ); Tue, 1 Feb 2011 08:15:07 -0500 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Jan 27, 2011 at 02:09:58PM +0100, Jan Kiszka wrote: > Found by Stefan Hajnoczi: There is a race in kvm_cpu_exec between > checking for exit_request on vcpu entry and timer signals arriving > before KVM starts to catch them. Plug it by blocking both timer related > signals also on !CONFIG_IOTHREAD and process those via signalfd. > > Signed-off-by: Jan Kiszka > CC: Stefan Hajnoczi > --- > cpus.c | 18 ++++++++++++++++++ > 1 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/cpus.c b/cpus.c > index fc3f222..29b1070 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -254,6 +254,10 @@ static void qemu_kvm_init_cpu_signals(CPUState *env) > pthread_sigmask(SIG_BLOCK, NULL, &set); > sigdelset(&set, SIG_IPI); > sigdelset(&set, SIGBUS); > +#ifndef CONFIG_IOTHREAD > + sigdelset(&set, SIGIO); > + sigdelset(&set, SIGALRM); > +#endif I'd prefer separate qemu_kvm_init_cpu_signals in the !IOTHREAD section. > r = kvm_set_signal_mask(env, &set); > if (r) { > fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(-r)); > @@ -351,6 +355,12 @@ static void qemu_kvm_eat_signals(CPUState *env) > exit(1); > } > } while (sigismember(&chkset, SIG_IPI) || sigismember(&chkset, SIGBUS)); > + > +#ifndef CONFIG_IOTHREAD > + if (sigismember(&chkset, SIGIO) || sigismember(&chkset, SIGALRM)) { > + qemu_notify_event(); > + } > +#endif Why is this necessary? You should break out of cpu_exec_all if there's a pending alarm (see qemu_alarm_pending()). > } > > #else /* _WIN32 */ > @@ -398,6 +408,14 @@ int qemu_init_main_loop(void) > int ret; > > sigemptyset(&blocked_signals); > + if (kvm_enabled()) { > + /* > + * We need to process timer signals synchronously to avoid a race > + * between exit_request check and KVM vcpu entry. > + */ > + sigaddset(&blocked_signals, SIGIO); > + sigaddset(&blocked_signals, SIGALRM); > + } A block_io_signals() function for !IOTHREAD would be nicer. > > ret = qemu_signalfd_init(blocked_signals); > if (ret) { > -- > 1.7.1 From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=55655 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PkG4Z-0007N4-9f for qemu-devel@nongnu.org; Tue, 01 Feb 2011 08:15:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PkG4X-0006UP-NR for qemu-devel@nongnu.org; Tue, 01 Feb 2011 08:15:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41089) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PkG4X-0006UI-Dv for qemu-devel@nongnu.org; Tue, 01 Feb 2011 08:15:05 -0500 Date: Tue, 1 Feb 2011 10:47:07 -0200 From: Marcelo Tosatti Message-ID: <20110201124707.GA12061@amt.cnet> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: [Qemu-devel] Re: [PATCH 14/22] kvm: Fix race between timer signals and vcpu entry under !IOTHREAD List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Stefan Hajnoczi , Avi Kivity , kvm@vger.kernel.org, qemu-devel@nongnu.org On Thu, Jan 27, 2011 at 02:09:58PM +0100, Jan Kiszka wrote: > Found by Stefan Hajnoczi: There is a race in kvm_cpu_exec between > checking for exit_request on vcpu entry and timer signals arriving > before KVM starts to catch them. Plug it by blocking both timer related > signals also on !CONFIG_IOTHREAD and process those via signalfd. > > Signed-off-by: Jan Kiszka > CC: Stefan Hajnoczi > --- > cpus.c | 18 ++++++++++++++++++ > 1 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/cpus.c b/cpus.c > index fc3f222..29b1070 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -254,6 +254,10 @@ static void qemu_kvm_init_cpu_signals(CPUState *env) > pthread_sigmask(SIG_BLOCK, NULL, &set); > sigdelset(&set, SIG_IPI); > sigdelset(&set, SIGBUS); > +#ifndef CONFIG_IOTHREAD > + sigdelset(&set, SIGIO); > + sigdelset(&set, SIGALRM); > +#endif I'd prefer separate qemu_kvm_init_cpu_signals in the !IOTHREAD section. > r = kvm_set_signal_mask(env, &set); > if (r) { > fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(-r)); > @@ -351,6 +355,12 @@ static void qemu_kvm_eat_signals(CPUState *env) > exit(1); > } > } while (sigismember(&chkset, SIG_IPI) || sigismember(&chkset, SIGBUS)); > + > +#ifndef CONFIG_IOTHREAD > + if (sigismember(&chkset, SIGIO) || sigismember(&chkset, SIGALRM)) { > + qemu_notify_event(); > + } > +#endif Why is this necessary? You should break out of cpu_exec_all if there's a pending alarm (see qemu_alarm_pending()). > } > > #else /* _WIN32 */ > @@ -398,6 +408,14 @@ int qemu_init_main_loop(void) > int ret; > > sigemptyset(&blocked_signals); > + if (kvm_enabled()) { > + /* > + * We need to process timer signals synchronously to avoid a race > + * between exit_request check and KVM vcpu entry. > + */ > + sigaddset(&blocked_signals, SIGIO); > + sigaddset(&blocked_signals, SIGALRM); > + } A block_io_signals() function for !IOTHREAD would be nicer. > > ret = qemu_signalfd_init(blocked_signals); > if (ret) { > -- > 1.7.1