From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 14/22] kvm: Fix race between timer signals and vcpu entry under !IOTHREAD Date: Tue, 01 Feb 2011 14:32:38 +0100 Message-ID: <4D480B76.90509@siemens.com> References: <20110201124707.GA12061@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Avi Kivity , "kvm@vger.kernel.org" , "qemu-devel@nongnu.org" , Stefan Hajnoczi To: Marcelo Tosatti Return-path: Received: from thoth.sbs.de ([192.35.17.2]:16989 "EHLO thoth.sbs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751307Ab1BANc5 (ORCPT ); Tue, 1 Feb 2011 08:32:57 -0500 In-Reply-To: <20110201124707.GA12061@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: 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? > >> 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()). qemu_alarm_pending() is not true until the signal is actually taken. The alarm handler sets the required flags. > >> } >> >> #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. > >> >> ret = qemu_signalfd_init(blocked_signals); >> if (ret) { >> -- >> 1.7.1 Thanks, Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=48045 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PkGLf-0000pa-P7 for qemu-devel@nongnu.org; Tue, 01 Feb 2011 08:32:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PkGLe-000282-CO for qemu-devel@nongnu.org; Tue, 01 Feb 2011 08:32:47 -0500 Received: from thoth.sbs.de ([192.35.17.2]:28430) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PkGLe-000270-2K for qemu-devel@nongnu.org; Tue, 01 Feb 2011 08:32:46 -0500 Message-ID: <4D480B76.90509@siemens.com> Date: Tue, 01 Feb 2011 14:32:38 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <20110201124707.GA12061@amt.cnet> In-Reply-To: <20110201124707.GA12061@amt.cnet> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: Marcelo Tosatti Cc: Stefan Hajnoczi , Avi Kivity , "kvm@vger.kernel.org" , "qemu-devel@nongnu.org" 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? > >> 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()). qemu_alarm_pending() is not true until the signal is actually taken. The alarm handler sets the required flags. > >> } >> >> #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. > >> >> ret = qemu_signalfd_init(blocked_signals); >> if (ret) { >> -- >> 1.7.1 Thanks, Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux