Re: [Qemu-devel] [PATCH v5 2/4] virtio-pci: Use ioeventfd for virtqueue notify

[Anthony Liguori <anthony@codemonkey.ws>]

On 01/25/2011 01:59 PM, Stefan Hajnoczi wrote:
> int kvm_cpu_exec(CPUState *env)
> {
>      struct kvm_run *run = env->kvm_run;
>      int ret;
>
>      DPRINTF("kvm_cpu_exec()\n");
>
>      do {
>
> This is broken because a signal handler could change env->exit_request
> after this check:
>
> #ifndef CONFIG_IOTHREAD
>          if (env->exit_request) {
>              DPRINTF("interrupt exit requested\n");
>              ret = 0;
>              break;
>          }
> #endif
>    

Yeah, this is classic signal/select race with ioctl(KVM_RUN) subbing in 
for select.  But this is supposed to be mitigated by the fact that we 
block SIG_IPI except for when we execute KVM_RUN which means that we can 
reliably send SIG_IPI.

Of course, that doesn't help for SIGALRM unless we send a SIG_IPI from 
the SIGALRM handler which we do with the I/O thread but not w/o it.  At 
any rate, post stable-0.14, I want to enable I/O thread by default so I 
don't know that we really need to fix this...

>          if (kvm_arch_process_irqchip_events(env)) {
>              ret = 0;
>              break;
>          }
>
>          if (env->kvm_vcpu_dirty) {
>              kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE);
>              env->kvm_vcpu_dirty = 0;
>          }
>
>          kvm_arch_pre_run(env, run);
>          cpu_single_env = NULL;
>          qemu_mutex_unlock_iothread();
>
> env->exit_request might be set but we still reenter, possibly without
> rearming the timer:
>          ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
>
>    
>>> I can think of two solutions:
>>> 1. Block SIGALRM during critical regions, not sure if the necessary
>>> atomic signal mask capabilities are there in KVM.  Haven't looked at
>>> TCG yet either.
>>> 2. Make a portion of the timer code signal-safe and rearm the timer
>>> from within the SIGLARM handler.
>>>
>>>        
>> Or, switch to timerfd and stop using a signal based alarm timer.
>>      
> Doesn't work for !CONFIG_IOTHREAD.
>    

Yeah, we need to get rid of !CONFIG_IOTHREAD.  We need to run select() 
in parallel with TCG/KVM and interrupt the VCPUs appropriately when 
select() returns.

Regards,

Anthony Liguori

> Stefan
>
>