From: Longpeng <longpeng2@huawei.com>

We found an issue when repeat reboot in guest during migration, it cause the
migration thread never be waken up again.

<main loop>                        |<migration_thread>
                                   |
LOCK BQL                           |
...                                |
main_loop_should_exit              |
 pause_all_vcpus                   |
  1. set all cpus ->stop=true      |
     and then kick                 |
  2. return if all cpus is paused  |
     (by '->stopped == true'), else|
  3. qemu_cond_wait [BQL UNLOCK]   |
                                   |LOCK BQL
                                   |...
                                   |do_vm_stop
                                   | pause_all_vcpus
                                   |  (A)set all cpus ->stop=true
                                   |     and then kick
                                   |  (B)return if all cpus is paused
                                   |     (by '->stopped == true'), else
                                   |  (C)qemu_cond_wait [BQL UNLOCK]
  4. be waken up and LOCK BQL      |  (D)be waken up BUT wait for  BQL
  5. goto 2.                       |
 (BQL is still LOCKed)             |
 resume_all_vcpus                  |
  1. set all cpus ->stop=false     |
     and ->stopped=false           |
...                                |
BQL UNLOCK                         |  (E)LOCK BQL
                                   |  (F)goto B. [but stopped is false now!]
                                   |Finally, sleep at step 3 forever.

As suggested by Paolo, resume_all_vcpus should notice this race, so we need
to move the change of runstate before pause_all_vcpus in do_vm_stop() and
ignore the resume request if runstate is not running.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Dr . David Alan Gilbert <dgilbert@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 cpus.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/cpus.c b/cpus.c
index b4f8b84..ef441bd 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1026,9 +1026,9 @@ static int do_vm_stop(RunState state, bool send_stop)
     int ret = 0;
 
     if (runstate_is_running()) {
+        runstate_set(state);
         cpu_disable_ticks();
         pause_all_vcpus();
-        runstate_set(state);
         vm_state_notify(0, state);
         if (send_stop) {
             qapi_event_send_stop();
@@ -1899,6 +1899,10 @@ void resume_all_vcpus(void)
 {
     CPUState *cpu;
 
+    if (!runstate_is_running()) {
+        return;
+    }
+
     qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
     CPU_FOREACH(cpu) {
         cpu_resume(cpu);
-- 
1.8.3.1

On 16/03/20 09:37, Longpeng(Mike) wrote:
> From: Longpeng <longpeng2@huawei.com>
> 
> We found an issue when repeat reboot in guest during migration, it cause the
> migration thread never be waken up again.
> 
> <main loop>                        |<migration_thread>
>                                    |
> LOCK BQL                           |
> ...                                |
> main_loop_should_exit              |
>  pause_all_vcpus                   |
>   1. set all cpus ->stop=true      |
>      and then kick                 |
>   2. return if all cpus is paused  |
>      (by '->stopped == true'), else|
>   3. qemu_cond_wait [BQL UNLOCK]   |
>                                    |LOCK BQL
>                                    |...
>                                    |do_vm_stop
>                                    | pause_all_vcpus
>                                    |  (A)set all cpus ->stop=true
>                                    |     and then kick
>                                    |  (B)return if all cpus is paused
>                                    |     (by '->stopped == true'), else
>                                    |  (C)qemu_cond_wait [BQL UNLOCK]
>   4. be waken up and LOCK BQL      |  (D)be waken up BUT wait for  BQL
>   5. goto 2.                       |
>  (BQL is still LOCKed)             |
>  resume_all_vcpus                  |
>   1. set all cpus ->stop=false     |
>      and ->stopped=false           |
> ...                                |
> BQL UNLOCK                         |  (E)LOCK BQL
>                                    |  (F)goto B. [but stopped is false now!]
>                                    |Finally, sleep at step 3 forever.
> 
> As suggested by Paolo, resume_all_vcpus should notice this race, so we need
> to move the change of runstate before pause_all_vcpus in do_vm_stop() and
> ignore the resume request if runstate is not running.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Dr . David Alan Gilbert <dgilbert@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Longpeng <longpeng2@huawei.com>

Queued, thanks!

Paolo

> ---
>  cpus.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/cpus.c b/cpus.c
> index b4f8b84..ef441bd 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1026,9 +1026,9 @@ static int do_vm_stop(RunState state, bool send_stop)
>      int ret = 0;
>  
>      if (runstate_is_running()) {
> +        runstate_set(state);
>          cpu_disable_ticks();
>          pause_all_vcpus();
> -        runstate_set(state);
>          vm_state_notify(0, state);
>          if (send_stop) {
>              qapi_event_send_stop();
> @@ -1899,6 +1899,10 @@ void resume_all_vcpus(void)
>  {
>      CPUState *cpu;
>  
> +    if (!runstate_is_running()) {
> +        return;
> +    }
> +
>      qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
>      CPU_FOREACH(cpu) {
>          cpu_resume(cpu);
>