[Qemu-devel] Re: [PATCH] kvm: Prevent dynticks race condition for !CONFIG_IOTHREAD

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2011-01-26 14:12, Jan Kiszka wrote:
> On 2011-01-26 10:39, Stefan Hajnoczi wrote:
>> The dynticks timer arranges for SIGALRM to be raised when the next
>> pending timer expires.  When building with !CONFIG_IOTHREAD, we need to
>> check whether a request to exit the vcpu is pending before re-entering
>> the guest.
>>
>> Unfortunately there is a race condition here because SIGALRM may be
>> raised after we check for an exit request but before re-entering the
>> guest.  In that case the guest is re-entered without the dynticks timer
>> being rearmed.
>>
>> This results in temporary loss of timers until some other event forces a
>> vmexit.  In the case of a CPU-bound guest it can cause softlockups.
>>
>> This patch blocks SIGALRM before checking for an exit request and uses
>> KVM's sigmask support to atomically unblock it when entering the guest,
>> thereby making the exit request check safe.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>> ---
>>  cpus.c    |   17 ++++++++++++++++-
>>  kvm-all.c |   16 ++++++++++++++++
>>  2 files changed, 32 insertions(+), 1 deletions(-)
>>
>> Does not affect qemu-kvm.git.  Still worth having in qemu.git so we don't get
>> odd behavior when building without --enable-io-thread.
>>
>> diff --git a/cpus.c b/cpus.c
>> index 0309189..59dbfab 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -262,14 +262,29 @@ void qemu_main_loop_start(void)
>>  {
>>  }
>>  
>> +static void kvm_init_sigmask(CPUState *env)
>> +{
>> +    int r;
>> +    sigset_t set;
>> +
>> +    pthread_sigmask(SIG_SETMASK, NULL, &set);
>> +    r = kvm_set_signal_mask(env, &set);
>> +    if (r) {
>> +        fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(r));
>> +        exit(1);
>> +    }
>> +}
>> +
>>  void qemu_init_vcpu(void *_env)
>>  {
>>      CPUState *env = _env;
>>  
>>      env->nr_cores = smp_cores;
>>      env->nr_threads = smp_threads;
>> -    if (kvm_enabled())
>> +    if (kvm_enabled()) {
>>          kvm_init_vcpu(env);
>> +        kvm_init_sigmask(env);
>> +    }
>>      return;
>>  }
>>  
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 255b6fa..9cc2553 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -890,19 +890,31 @@ int kvm_cpu_exec(CPUState *env)
>>  {
>>      struct kvm_run *run = env->kvm_run;
>>      int ret;
>> +#ifndef CONFIG_IOTHREAD
>> +    sigset_t set, old_set;
>> +
>> +    sigemptyset(&set);
>> +    sigaddset(&set, SIGALRM);
>> +#endif
>>  
>>      DPRINTF("kvm_cpu_exec()\n");
>>  
>>      do {
>>  #ifndef CONFIG_IOTHREAD
>> +        pthread_sigmask(SIG_BLOCK, &set, &old_set);
>> +
>>          if (env->exit_request) {
>>              DPRINTF("interrupt exit requested\n");
>> +            pthread_sigmask(SIG_SETMASK, &old_set, NULL);
>>              ret = 0;
>>              break;
>>          }
>>  #endif
>>  
>>          if (kvm_arch_process_irqchip_events(env)) {
>> +#ifndef CONFIG_IOTHREAD
>> +            pthread_sigmask(SIG_SETMASK, &old_set, NULL);
>> +#endif
>>              ret = 0;
>>              break;
>>          }
>> @@ -920,6 +932,10 @@ int kvm_cpu_exec(CPUState *env)
>>          cpu_single_env = env;
>>          kvm_arch_post_run(env, run);
>>  
>> +#ifndef CONFIG_IOTHREAD
>> +        pthread_sigmask(SIG_SETMASK, &old_set, NULL);
>> +#endif
>> +
>>          if (ret == -EINTR || ret == -EAGAIN) {
>>              cpu_exit(env);
>>              DPRINTF("io window exit\n");
> 
> Good catch, but the code should be organized differently, probably with
> the help of signalfd which we need for SIGBUS anyway. I'm currently
> reworking signaling bits. I will pick this up and integrate a
> corresponding solution in my queue. Will keep you posted!
> 

Looks like this should also include SIGIO in case hpet is selected as
host timer.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux