[Qemu-devel] Re: [PATCH 14/22] kvm: Fix race between timer signals and vcpu entry under !IOTHREAD

[Marcelo Tosatti]

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 <address@hidden>
> CC: Stefan Hajnoczi <address@hidden>
> ---
>  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