From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [PATCH v2] fix qemu-kvm sigsegv at exit
Date: Wed, 28 Oct 2009 20:38:45 -0200
Message-ID: <20091028223845.GA18038@amt.cnet>
References: <20091026184602.GB6016@amt.cnet> <20091026185849.GA5930@redhat.com> <20091027153346.GA9730@amt.cnet> <4AE81220.2000102@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Gleb Natapov <gleb@redhat.com>, kvm <kvm@vger.kernel.org>
To: Avi Kivity <avi@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:17393 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751449AbZJ1WkM (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 28 Oct 2009 18:40:12 -0400
Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])
	by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n9SMeGVu006768
	for <kvm@vger.kernel.org>; Wed, 28 Oct 2009 18:40:16 -0400
Content-Disposition: inline
In-Reply-To: <4AE81220.2000102@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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