Re: [PATCH v2] fix qemu-kvm sigsegv at exit

[Marcelo Tosatti <mtosatti@redhat.com>]

On Wed, Oct 28, 2009 at 11:42:56AM +0200, Avi Kivity wrote:
> On 10/27/2009 05:33 PM, Marcelo Tosatti wrote:
>> Michael reported a qemu-kvm SIGSEGV at shutdown:
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> [Switching to Thread 0x411d0940 (LWP 14446)]
>> 0x000000000040afb4 in qemu_mod_timer (ts=0x19f0fd0,
>> expire_time=62275467335)
>>      at /home/mst/scm/qemu-kvm/vl.c:1009
>> 1009            if ((alarm_timer->flags&  ALARM_FLAG_EXPIRED) == 0)
>> {
>> (gdb) l
>> 1004        ts->next = *pt;
>> 1005        *pt = ts;
>> 1006
>> 1007        /* Rearm if necessary  */
>> 1008        if (pt ==&active_timers[ts->clock->type]) {
>> 1009            if ((alarm_timer->flags&  ALARM_FLAG_EXPIRED) == 0)
>> {
>> 1010          	qemu_rearm_alarm_timer(alarm_timer);
>> 1011            }
>> 1012            /* Interrupt execution to force deadline
>> recalculation.  */
>> 1013            if (use_icount)
>> (gdb) p alarm_timer
>> $1 = (struct qemu_alarm_timer *) 0x0
>>
>> Problem is kvm_main_loop_wait(env, 0) can process the stop request
>> (signalling iothread that vcpu is stopped, so its OK to exit) and
>> continue to kvm_cpu_exec.
>>
>> Reorder kvm_main_loop_wait and kvm_cpu_exec, as suggested by Gleb.
>>
>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>> Reported-by: "Michael S. Tsirkin"<mst@redhat.com>
>>
>> diff --git a/qemu-kvm.c b/qemu-kvm.c
>> index 4c13628..809fd65 100644
>> --- a/qemu-kvm.c
>> +++ b/qemu-kvm.c
>> @@ -1867,8 +1867,8 @@ static int kvm_main_loop_cpu(CPUState *env)
>>               run_cpu = !env->halted;
>>           }
>>           if (run_cpu) {
>> -            kvm_main_loop_wait(env, 0);
>>               kvm_cpu_exec(env);
>> +            kvm_main_loop_wait(env, 0);
>>           } else {
>>               kvm_main_loop_wait(env, 1000);
>>           }
>>    
>
> This will miss an event at the very beginning of the loop (powerdown  
> requested before we had a chance to spin up?).  I think it should be  
> fine since there will be a pending signal which will break us out of  
> kvm_cpu_exec() before it has a chance to do anything (i.e. spin in the  
> guest).
>
> So, I think it's fine, but please double check the above.

That can happen but only with the unpatched version, where
kvm_main_loop_wait eats the signal, sets env->stopped = 1, 
and proceeds to kvm_cpu_exec.
With the patch, this is not the case.

As before, the signal sender will send an IPI with smp_send_reschedule
(and vcpu_enter_guest checks sigpending after disabling interrupts).