fix qemu-kvm sigsegv at exit

fix qemu-kvm sigsegv at exit

[Marcelo Tosatti]

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.

Make sure cpu is not stopped before proceeding to kvm_cpu_exec.

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..ab8f0e4 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -1868,7 +1868,8 @@ static int kvm_main_loop_cpu(CPUState *env)
         }
         if (run_cpu) {
             kvm_main_loop_wait(env, 0);
-            kvm_cpu_exec(env);
+            if (!is_cpu_stopped(env))
+                kvm_cpu_exec(env);
         } else {
             kvm_main_loop_wait(env, 1000);
         }

Re: fix qemu-kvm sigsegv at exit

[Gleb Natapov]

On Mon, Oct 26, 2009 at 04:46:02PM -0200, 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.
> 
> Make sure cpu is not stopped before proceeding to kvm_cpu_exec.
> 
> 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..ab8f0e4 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -1868,7 +1868,8 @@ static int kvm_main_loop_cpu(CPUState *env)
>          }
>          if (run_cpu) {
>              kvm_main_loop_wait(env, 0);
> -            kvm_cpu_exec(env);
> +            if (!is_cpu_stopped(env))
> +                kvm_cpu_exec(env);
I wonder if calling kvm_cpu_exec() after kvm_main_loop_wait() will fix
the problem?

>          } else {
>              kvm_main_loop_wait(env, 1000);
>          }
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: fix qemu-kvm sigsegv at exit

[Marcelo Tosatti]

On Mon, Oct 26, 2009 at 08:58:49PM +0200, Gleb Natapov wrote:
> On Mon, Oct 26, 2009 at 04:46:02PM -0200, 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.
> > 
> > Make sure cpu is not stopped before proceeding to kvm_cpu_exec.
> > 
> > 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..ab8f0e4 100644
> > --- a/qemu-kvm.c
> > +++ b/qemu-kvm.c
> > @@ -1868,7 +1868,8 @@ static int kvm_main_loop_cpu(CPUState *env)
> >          }
> >          if (run_cpu) {
> >              kvm_main_loop_wait(env, 0);
> > -            kvm_cpu_exec(env);
> > +            if (!is_cpu_stopped(env))
> > +                kvm_cpu_exec(env);
> I wonder if calling kvm_cpu_exec() after kvm_main_loop_wait() will fix
> the problem?

Yeah, that would also do it.

[PATCH v2] fix qemu-kvm sigsegv at exit

[Marcelo Tosatti]

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);
         }

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

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

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

[Marcelo Tosatti]

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

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

[Avi Kivity]

On 10/29/2009 12:38 AM, Marcelo Tosatti wrote:
>
>> 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).
>    

Fair enough, double bugfix; applied, thanks.

-- 
error compiling committee.c: too many arguments to function