Series comparison

 [Qemu-devel] [PULL 0/1] Block patches
-The following changes since commit 0b5e750bea635b167eb03d86c3d9a09bbd43bc06:
+The following changes since commit cc6613e244e86c66f83467eab5284825d7057cea:
-  Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2019-02-12 10:53:37 +0000)
+  Merge remote-tracking branch 'remotes/stefanha/tags/tracing-pull-request' into staging (2019-09-03 11:06:09 +0100)
 are available in the Git repository at:
-  git://github.com/stefanha/qemu.git tags/block-pull-request
+  https://github.com/stefanha/qemu.git tags/block-pull-request
-for you to fetch changes up to 42824b4d16da56a50ff4027f6cd22378e0e2666e:
+for you to fetch changes up to ebb6ff25cd888a52a64a9adc3692541c6d1d9a42:
-  virtio-blk: set correct config size for the host driver (2019-02-13 16:18:17 +0800)
+  virtio-blk: Cancel the pending BH when the dataplane is reset (2019-09-03 16:11:18 +0100)
 ----------------------------------------------------------------
 Pull request
-Fix a virtio-blk migration regression.
 ----------------------------------------------------------------
-Changpeng Liu (1):
+Philippe Mathieu-Daudé (1):
-  virtio-blk: set correct config size for the host driver
+  virtio-blk: Cancel the pending BH when the dataplane is reset
- hw/block/virtio-blk.c | 13 +++++++++----
+ hw/block/dataplane/virtio-blk.c | 3 +++
-file changed, 9 insertions(+), 4 deletions(-)
+file changed, 3 insertions(+)
 --
-.20.1
+.21.0

-[Qemu-devel] [PULL 1/1] virtio-blk: set correct config size for the host driver
+[Qemu-devel] [PULL 1/1] virtio-blk: Cancel the pending BH when the dataplane is reset
-From: Changpeng Liu <changpeng.liu@intel.com>
+From: Philippe Mathieu-Daudé <philmd@redhat.com>
-Commit caa1ee43 "vhost-user-blk: add discard/write zeroes features
+When 'system_reset' is called, the main loop clear the memory
-support" added fields to struct virtio_blk_config. This changes
+region cache before the BH has a chance to execute. Later when
-the size of the config space and breaks migration from QEMU 3.1
+the deferred function is called, some assumptions that were
-and older:
+made when scheduling them are no longer true when they actually
 execute.
-qemu-system-ppc64: get_pci_config_device: Bad config data: i=0x10 read: 41 device: 1 cmask: ff wmask: 80 w1cmask:0
+This is what happens using a virtio-blk device (fresh RHEL7.8 install):
 qemu-system-ppc64: Failed to load PCIDevice:config
 qemu-system-ppc64: Failed to load virtio-blk:virtio
 qemu-system-ppc64: error while loading state for instance 0x0 of device 'pci@800000020000000:01.0/virtio-blk'
 qemu-system-ppc64: load of migration failed: Invalid argument
-Since virtio-blk doesn't support the "discard" and "write zeroes"
+ $ (sleep 12.3; echo system_reset; sleep 12.3; echo system_reset; sleep 1; echo q) \
-features, it shouldn't even expose the associated fields in the
+   | qemu-system-x86_64 -m 4G -smp 8 -boot menu=on \
-config space actually. Just include all fields up to num_queues to
+     -device virtio-blk-pci,id=image1,drive=drive_image1 \
-match QEMU 3.1 and older.
+     -drive file=/var/lib/libvirt/images/rhel78.qcow2,if=none,id=drive_image1,format=qcow2,cache=none \
      -device virtio-net-pci,netdev=net0,id=nic0,mac=52:54:00:c4:e7:84 \
      -netdev tap,id=net0,script=/bin/true,downscript=/bin/true,vhost=on \
      -monitor stdio -serial null -nographic
   (qemu) system_reset
   (qemu) system_reset
   (qemu) qemu-system-x86_64: hw/virtio/virtio.c:225: vring_get_region_caches: Assertion `caches != NULL' failed.
   Aborted
-Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
+  (gdb) bt
-Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
+  Thread 1 (Thread 0x7f109c17b680 (LWP 10939)):
-Message-id: 1550022537-27565-1-git-send-email-changpeng.liu@intel.com
+  #0  0x00005604083296d1 in vring_get_region_caches (vq=0x56040a24bdd0) at hw/virtio/virtio.c:227
-Message-Id: <1550022537-27565-1-git-send-email-changpeng.liu@intel.com>
+  #1  0x000056040832972b in vring_avail_flags (vq=0x56040a24bdd0) at hw/virtio/virtio.c:235
   #2  0x000056040832d13d in virtio_should_notify (vdev=0x56040a240630, vq=0x56040a24bdd0) at hw/virtio/virtio.c:1648
   #3  0x000056040832d1f8 in virtio_notify_irqfd (vdev=0x56040a240630, vq=0x56040a24bdd0) at hw/virtio/virtio.c:1662
   #4  0x00005604082d213d in notify_guest_bh (opaque=0x56040a243ec0) at hw/block/dataplane/virtio-blk.c:75
   #5  0x000056040883dc35 in aio_bh_call (bh=0x56040a243f10) at util/async.c:90
   #6  0x000056040883dccd in aio_bh_poll (ctx=0x560409161980) at util/async.c:118
   #7  0x0000560408842af7 in aio_dispatch (ctx=0x560409161980) at util/aio-posix.c:460
   #8  0x000056040883e068 in aio_ctx_dispatch (source=0x560409161980, callback=0x0, user_data=0x0) at util/async.c:261
   #9  0x00007f10a8fca06d in g_main_context_dispatch () at /lib64/libglib-2.0.so.0
   #10 0x0000560408841445 in glib_pollfds_poll () at util/main-loop.c:215
   #11 0x00005604088414bf in os_host_main_loop_wait (timeout=0) at util/main-loop.c:238
   #12 0x00005604088415c4 in main_loop_wait (nonblocking=0) at util/main-loop.c:514
   #13 0x0000560408416b1e in main_loop () at vl.c:1923
   #14 0x000056040841e0e8 in main (argc=20, argv=0x7ffc2c3f9c58, envp=0x7ffc2c3f9d00) at vl.c:4578
 Fix this by cancelling the BH when the virtio dataplane is stopped.
 [This is version of the patch was modified as discussed with Philippe on
 the mailing list thread.
 --Stefan]
 Reported-by: Yihuang Yu <yihyu@redhat.com>
 Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
 Fixes: https://bugs.launchpad.net/qemu/+bug/1839428
 Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
 Message-Id: <20190816171503.24761-1-philmd@redhat.com>
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- hw/block/virtio-blk.c | 13 +++++++++----
+ hw/block/dataplane/virtio-blk.c | 3 +++
-file changed, 9 insertions(+), 4 deletions(-)
+file changed, 3 insertions(+)
-diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
+diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
 index XXXXXXX..XXXXXXX 100644
---- a/hw/block/virtio-blk.c
+--- a/hw/block/dataplane/virtio-blk.c
-+++ b/hw/block/virtio-blk.c
++++ b/hw/block/dataplane/virtio-blk.c
-@@ -XXX,XX +XXX,XX @@
+@@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
- #include "hw/virtio/virtio-bus.h"
+         virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
- #include "hw/virtio/virtio-access.h"
+     }
-+/* We don't support discard yet, hide associated config fields. */
++    qemu_bh_cancel(s->bh);
-+#define VIRTIO_BLK_CFG_SIZE offsetof(struct virtio_blk_config, \
++    notify_guest_bh(s); /* final chance to notify guest */
 +                                     max_discard_sectors)
 +
- static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
+     /* Clean up guest notifier (irq) */
-                                     VirtIOBlockReq *req)
+     k->set_guest_notifiers(qbus->parent, nvqs, false);
- {
@@ -XXX,XX +XXX,XX @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
      blkcfg.alignment_offset = 0;
      blkcfg.wce = blk_enable_write_cache(s->blk);
      virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues);
 -    memcpy(config, &blkcfg, sizeof(struct virtio_blk_config));
 +    memcpy(config, &blkcfg, VIRTIO_BLK_CFG_SIZE);
 +    QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE > sizeof(blkcfg));
  }
  static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
@@ -XXX,XX +XXX,XX @@ static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
      VirtIOBlock *s = VIRTIO_BLK(vdev);
      struct virtio_blk_config blkcfg;
 -    memcpy(&blkcfg, config, sizeof(blkcfg));
 +    memcpy(&blkcfg, config, VIRTIO_BLK_CFG_SIZE);
 +    QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE > sizeof(blkcfg));
      aio_context_acquire(blk_get_aio_context(s->blk));
      blk_set_enable_write_cache(s->blk, blkcfg.wce != 0);
@@ -XXX,XX +XXX,XX @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
          return;
      }
 -    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
 -                sizeof(struct virtio_blk_config));
 +    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, VIRTIO_BLK_CFG_SIZE);
      s->blk = conf->conf.blk;
      s->rq = NULL;
 --
-.20.1
+.21.0

From: Changpeng Liu <changpeng.liu@intel.com>

Commit caa1ee43 "vhost-user-blk: add discard/write zeroes features
support" added fields to struct virtio_blk_config. This changes
the size of the config space and breaks migration from QEMU 3.1
and older:

qemu-system-ppc64: get_pci_config_device: Bad config data: i=0x10 read: 41 device: 1 cmask: ff wmask: 80 w1cmask:0
qemu-system-ppc64: Failed to load PCIDevice:config
qemu-system-ppc64: Failed to load virtio-blk:virtio
qemu-system-ppc64: error while loading state for instance 0x0 of device 'pci@800000020000000:01.0/virtio-blk'
qemu-system-ppc64: load of migration failed: Invalid argument

Since virtio-blk doesn't support the "discard" and "write zeroes"
features, it shouldn't even expose the associated fields in the
config space actually. Just include all fields up to num_queues to
match QEMU 3.1 and older.

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-id: 1550022537-27565-1-git-send-email-changpeng.liu@intel.com
Message-Id: <1550022537-27565-1-git-send-email-changpeng.liu@intel.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -XXX,XX +XXX,XX @@
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
 
+/* We don't support discard yet, hide associated config fields. */
+#define VIRTIO_BLK_CFG_SIZE offsetof(struct virtio_blk_config, \
+                                     max_discard_sectors)
+
 static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
                                     VirtIOBlockReq *req)
 {
@@ -XXX,XX +XXX,XX @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
     blkcfg.alignment_offset = 0;
     blkcfg.wce = blk_enable_write_cache(s->blk);
     virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues);
-    memcpy(config, &blkcfg, sizeof(struct virtio_blk_config));
+    memcpy(config, &blkcfg, VIRTIO_BLK_CFG_SIZE);
+    QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE > sizeof(blkcfg));
 }
 
 static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
@@ -XXX,XX +XXX,XX @@ static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
     VirtIOBlock *s = VIRTIO_BLK(vdev);
     struct virtio_blk_config blkcfg;
 
-    memcpy(&blkcfg, config, sizeof(blkcfg));
+    memcpy(&blkcfg, config, VIRTIO_BLK_CFG_SIZE);
+    QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE > sizeof(blkcfg));
 
     aio_context_acquire(blk_get_aio_context(s->blk));
     blk_set_enable_write_cache(s->blk, blkcfg.wce != 0);
@@ -XXX,XX +XXX,XX @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
-                sizeof(struct virtio_blk_config));
+    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, VIRTIO_BLK_CFG_SIZE);
 
     s->blk = conf->conf.blk;
     s->rq = NULL;
-- 
2.20.1

From: Philippe Mathieu-Daudé <philmd@redhat.com>

When 'system_reset' is called, the main loop clear the memory
region cache before the BH has a chance to execute. Later when
the deferred function is called, some assumptions that were
made when scheduling them are no longer true when they actually
execute.

This is what happens using a virtio-blk device (fresh RHEL7.8 install):

$ (sleep 12.3; echo system_reset; sleep 12.3; echo system_reset; sleep 1; echo q) \
   | qemu-system-x86_64 -m 4G -smp 8 -boot menu=on \
     -device virtio-blk-pci,id=image1,drive=drive_image1 \
     -drive file=/var/lib/libvirt/images/rhel78.qcow2,if=none,id=drive_image1,format=qcow2,cache=none \
     -device virtio-net-pci,netdev=net0,id=nic0,mac=52:54:00:c4:e7:84 \
     -netdev tap,id=net0,script=/bin/true,downscript=/bin/true,vhost=on \
     -monitor stdio -serial null -nographic
  (qemu) system_reset
  (qemu) system_reset
  (qemu) qemu-system-x86_64: hw/virtio/virtio.c:225: vring_get_region_caches: Assertion `caches != NULL' failed.
  Aborted

(gdb) bt
  Thread 1 (Thread 0x7f109c17b680 (LWP 10939)):
  #0  0x00005604083296d1 in vring_get_region_caches (vq=0x56040a24bdd0) at hw/virtio/virtio.c:227
  #1  0x000056040832972b in vring_avail_flags (vq=0x56040a24bdd0) at hw/virtio/virtio.c:235
  #2  0x000056040832d13d in virtio_should_notify (vdev=0x56040a240630, vq=0x56040a24bdd0) at hw/virtio/virtio.c:1648
  #3  0x000056040832d1f8 in virtio_notify_irqfd (vdev=0x56040a240630, vq=0x56040a24bdd0) at hw/virtio/virtio.c:1662
  #4  0x00005604082d213d in notify_guest_bh (opaque=0x56040a243ec0) at hw/block/dataplane/virtio-blk.c:75
  #5  0x000056040883dc35 in aio_bh_call (bh=0x56040a243f10) at util/async.c:90
  #6  0x000056040883dccd in aio_bh_poll (ctx=0x560409161980) at util/async.c:118
  #7  0x0000560408842af7 in aio_dispatch (ctx=0x560409161980) at util/aio-posix.c:460
  #8  0x000056040883e068 in aio_ctx_dispatch (source=0x560409161980, callback=0x0, user_data=0x0) at util/async.c:261
  #9  0x00007f10a8fca06d in g_main_context_dispatch () at /lib64/libglib-2.0.so.0
  #10 0x0000560408841445 in glib_pollfds_poll () at util/main-loop.c:215
  #11 0x00005604088414bf in os_host_main_loop_wait (timeout=0) at util/main-loop.c:238
  #12 0x00005604088415c4 in main_loop_wait (nonblocking=0) at util/main-loop.c:514
  #13 0x0000560408416b1e in main_loop () at vl.c:1923
  #14 0x000056040841e0e8 in main (argc=20, argv=0x7ffc2c3f9c58, envp=0x7ffc2c3f9d00) at vl.c:4578

Fix this by cancelling the BH when the virtio dataplane is stopped.

[This is version of the patch was modified as discussed with Philippe on
the mailing list thread.
--Stefan]

Reported-by: Yihuang Yu <yihyu@redhat.com>
Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Fixes: https://bugs.launchpad.net/qemu/+bug/1839428
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20190816171503.24761-1-philmd@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
         virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
     }
 
+    qemu_bh_cancel(s->bh);
+    notify_guest_bh(s); /* final chance to notify guest */
+
     /* Clean up guest notifier (irq) */
     k->set_guest_notifiers(qbus->parent, nvqs, false);
 
-- 
2.21.0