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

[Jan Kiszka]

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 <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.

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