From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=52468 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PkGdj-0007KJ-SW for qemu-devel@nongnu.org; Tue, 01 Feb 2011 08:52:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PkGdi-0006kW-FZ for qemu-devel@nongnu.org; Tue, 01 Feb 2011 08:51:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:22674) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PkGdi-0006kR-7T for qemu-devel@nongnu.org; Tue, 01 Feb 2011 08:51:26 -0500 Date: Tue, 1 Feb 2011 11:48:21 -0200 From: Marcelo Tosatti Message-ID: <20110201134821.GA12848@amt.cnet> References: <20110201124707.GA12061@amt.cnet> <4D480B76.90509@siemens.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4D480B76.90509@siemens.com> 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 Tue, Feb 01, 2011 at 02:32:38PM +0100, Jan Kiszka wrote: > On 2011-02-01 13:47, Marcelo Tosatti wrote: > > 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. > > You mean to duplicate qemu_kvm_init_cpu_signals for both configurations? Yes, so to avoid #ifdefs spread. > >> + > >> +#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()). > > qemu_alarm_pending() is not true until the signal is actually taken. The > alarm handler sets the required flags. Right. What i mean is you need to execute the signal handler inside cpu_exec_all loop (so that alarm pending is set). So, if there is a SIGALRM pending, qemu_run_timers has highest priority, not vcpu execution. > >> > >> #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. > > Well, we aren't blocking all I/O signals, so I decided against causing > confusion to people that try to compare the result against real > block_io_signals. If you mean just pushing those lines that set up > blocked_signals into a separate function, then I need to find a good > name for it. Yes, separate function, similar to CONFIG_IOTHREAD case (feel free to rename function).