Qemu aborted in ide_restart_bh after migration

Commit Message

Wangjing (Hogan, Cloud Infrastructure Service Product Dept.) Feb. 22, 2018, 8:23 a.m. UTC

This issue occurred at a very low probability, If we inject delays in address_space_write_continue like this, the issue occurred inevitably：
#######################################################################################
#######################################################################################
Reproduce steps:
1) use 'virsh create vm.xml' to start a VM, and the VM configured empty IDE CD-ROM.
2) when qemu process print "start sleep", execute "virsh reset vm; virsh suspend vm" command.
3) use "virsh save vm vm.state" to save VM's state.
4) use "virsh restore vm.state" to restore VM's state.
5) execute "virsh resume vm", and then qemu process aborted at ide_restart_bh.

I think we should set bm->dma_cb to NULL in bmdma_reset function to avoid this issues.

>> Empty IDE CD-ROM configured on the VM:

>>     <disk type='file' device='cdrom'>

>>       <driver name='qemu' type='raw' cache='none' io='threads'/>

>>       <target dev='hdb' bus='ide'/>

>>       <readonly/>

>>       <address type='drive' controller='0' bus='0' target='0' unit='1'/>

>>     </disk>

>> Make migration for this VM, then qemu aborted in ide_restart_bh. 

>> IDEState expect end_transfer_func equal to ide_atapi_cmd, but it refer to ide_dummy_transfer_stop.

>> I have no idea about this, can anyone help me?

>> 

>

>Do you have an easy way to reproduce this? 2.8.1 is a bit old at this point, but I don't think we've changed anything in the IDE emulator substantively since then, so I'm curious to see if I can get this to reproduce.

>

>I'm surprised an empty CD-ROM would cause this, though, since it shouldn't really have any commands in-flight that might get us to a weird edge case, so I want to take a close look at this.

>

>Denis Lunev noted some issues with migration that I couldn't solve at the time either. A reproducer would be fantastic.

>

>> qemu version is 2.8.1

>> (gdb) bt

>> #0  0x00007fcff7c4b157 in raise () from /usr/lib64/libc.so.6

>> #1  0x00007fcff7c4c848 in abort () from /usr/lib64/libc.so.6

>> #2  0x00007fcff7c441c6 in __assert_fail_base () from 

>> /usr/lib64/libc.so.6

>> #3  0x00007fcff7c44272 in __assert_fail () from /usr/lib64/libc.so.6

>> #4  0x00000000006207ab in ide_restart_bh (opaque=0x38b3430) at 

>> ....

>> (gdb)

>> 

>

>--

>—js

>

Patch

diff --git a/exec.c b/exec.c
index e8d7b33..b9779e0 100644
--- a/exec.c
+++ b/exec.c
@@ -3056,6 +3056,11 @@  static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,

         if (release_lock) {
             qemu_mutex_unlock_iothread();
+            if (mr->name && !strcmp(mr->name, "ide") && atomic_xchg_my_flag(0)) {
+                printf("start sleep");
+                usleep(5 * 1000 * 1000);
+                printf("end sleep");
+            }
             release_lock = false;
         }


w00185384@HGHY1w001853841 /cygdrive/f/GitHome/opensource/qemu
$ git diff
diff --git a/exec.c b/exec.c
index e8d7b33..b9779e0 100644
--- a/exec.c
+++ b/exec.c
@@ -3056,6 +3056,11 @@  static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,

         if (release_lock) {
             qemu_mutex_unlock_iothread();
+            if (mr->name && !strcmp(mr->name, "ide") && atomic_xchg_my_flag(0)) {
+                printf("start sleep");
+                usleep(5 * 1000 * 1000);
+                printf("end sleep");
+            }
             release_lock = false;
         }

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 1ab0a89..a1807c8 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -51,6 +51,11 @@  static void bmdma_start_dma(IDEDMA *dma, IDEState *s,
     if (bm->status & BM_STATUS_DMAING) {
         bm->dma_cb(bmdma_active_if(bm), 0);
     }
+
+    if (!(bm->status & BM_STATUS_DMAING) && bm->dma_cb &&
+        IDE_DMA_ATAPI == bmdma_active_if(bm)->dma_cmd) {
+        atomic_xchg_my_flag(1);
+    }
 }

 /**
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index e1c8ae1..095d6a4 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -47,4 +47,6 @@  void info_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 const char *error_get_progname(void);
 extern bool enable_timestamp_msg;

+int atomic_xchg_my_flag(int flag);
+
 #endif
diff --git a/util/qemu-error.c b/util/qemu-error.c
index a25d3b9..969bdeb 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -310,3 +310,9 @@  void info_report(const char *fmt, ...)
     vreport(REPORT_TYPE_INFO, fmt, ap);
     va_end(ap);
 }
+
+int atomic_xchg_my_flag(int flag)
+{
+    static int my_flag = 0;
+    return atomic_xchg(&my_flag, flag);
+}