[Qemu-devel] [PULL for-1.4 0/8] Block patches

[Qemu-devel] [PULL for-1.4 0/8] Block patches

[Stefan Hajnoczi]

Bug fixes plus a cleanup from mjt which logically follows the win32-aio fixes.

The following changes since commit 47f4dac3fde809e3da4e60d9eb699f1d4b378249:

  Merge remote-tracking branch 'kraxel/chardev.1' into staging (2013-01-16 15:20:05 -0600)

are available in the git repository at:


  git://github.com/stefanha/qemu.git block

for you to fetch changes up to cf139388ad5b39228793f34eea99e0ea9a2924aa:

  dataplane: support viostor virtio-pci status bit setting (2013-01-18 16:59:20 +0100)

----------------------------------------------------------------
Jeff Cody (1):
      block: fix null-pointer bug on error case in block commit

Kevin Wolf (4):
      ide: Remove wrong assertion
      aio: Fix return value of aio_poll()
      win32-aio: Fix vectored reads
      win32-aio: Fix memory leak

Michael Tokarev (1):
      win32-aio: use iov utility functions instead of open-coding them

Stefan Hajnoczi (2):
      dataplane: avoid reentrancy during virtio_blk_data_plane_stop()
      dataplane: support viostor virtio-pci status bit setting

 aio-posix.c               |  3 ++-
 aio-win32.c               |  3 ++-
 block/commit.c            |  7 +++----
 block/win32-aio.c         | 19 ++++---------------
 hw/dataplane/virtio-blk.c |  9 ++++++---
 hw/ide/pci.c              |  1 -
 hw/virtio-blk.c           |  3 ++-
 include/block/aio.h       |  6 ++----
 tests/test-aio.c          |  4 ++--
 9 files changed, 23 insertions(+), 32 deletions(-)

-- 
1.8.0.2

[Qemu-devel] [PATCH 1/8] block: fix null-pointer bug on error case in block commit

[Stefan Hajnoczi]

From: Jeff Cody <jcody@redhat.com>

This is a bug that was caught by a coverity run by Markus.  In
the error case when we errored out to exit_restore_open early in the
function, 'overlay_bs' was still NULL at that point, although it is
used to look up flags and perform a bdrv_reopen().

Move the overlay_bs lookup to where it is needed, and check for NULL
before restoring the flags.  Also get rid of the unneeded parameter
initialization.

Reported-By: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/commit.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 61ebdba..553447e 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -65,7 +65,7 @@ static void coroutine_fn commit_run(void *opaque)
     BlockDriverState *active = s->active;
     BlockDriverState *top = s->top;
     BlockDriverState *base = s->base;
-    BlockDriverState *overlay_bs = NULL;
+    BlockDriverState *overlay_bs;
     int64_t sector_num, end;
     int ret = 0;
     int n = 0;
@@ -92,8 +92,6 @@ static void coroutine_fn commit_run(void *opaque)
         }
     }
 
-    overlay_bs = bdrv_find_overlay(active, top);
-
     end = s->common.len >> BDRV_SECTOR_BITS;
     buf = qemu_blockalign(top, COMMIT_BUFFER_SIZE);
 
@@ -156,7 +154,8 @@ exit_restore_reopen:
     if (s->base_flags != bdrv_get_flags(base)) {
         bdrv_reopen(base, s->base_flags, NULL);
     }
-    if (s->orig_overlay_flags != bdrv_get_flags(overlay_bs)) {
+    overlay_bs = bdrv_find_overlay(active, top);
+    if (overlay_bs && s->orig_overlay_flags != bdrv_get_flags(overlay_bs)) {
         bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL);
     }
 
-- 
1.8.0.2

[Qemu-devel] [PATCH 2/8] ide: Remove wrong assertion

[Stefan Hajnoczi]

From: Kevin Wolf <kwolf@redhat.com>

The Bus Master IDE Active bit (BM_STATUS_DMAING) is not only set when
the request is still in flight, but also when it has completed and the
size of the physical memory regions in the PRDT was larger than the
transfer size.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/ide/pci.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index e6226e3..59fd539 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -311,7 +311,6 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
             if (bm->bus->dma->aiocb) {
                 bdrv_drain_all();
                 assert(bm->bus->dma->aiocb == NULL);
-                assert((bm->status & BM_STATUS_DMAING) == 0);
             }
         } else {
             bm->cur_addr = bm->addr;
-- 
1.8.0.2

[Qemu-devel] [PATCH 3/8] aio: Fix return value of aio_poll()

[Stefan Hajnoczi]

From: Kevin Wolf <kwolf@redhat.com>

aio_poll() must return true if any work is still pending, even if it
didn't make progress, so that bdrv_drain_all() doesn't stop waiting too
early. The possibility of stopping early occasionally lead to a failed
assertion in bdrv_drain_all(), when some in-flight request was missed
and the function didn't really drain all requests.

In order to make that change, the return value as specified in the
function comment must change for blocking = false; fortunately, the
return value of blocking = false callers is only used in test cases, so
this change shouldn't cause any trouble.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 aio-posix.c         | 3 ++-
 aio-win32.c         | 3 ++-
 include/block/aio.h | 6 ++----
 tests/test-aio.c    | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/aio-posix.c b/aio-posix.c
index 88d09e1..fe4dbb4 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -264,5 +264,6 @@ bool aio_poll(AioContext *ctx, bool blocking)
         }
     }
 
-    return progress;
+    assert(progress || busy);
+    return true;
 }
diff --git a/aio-win32.c b/aio-win32.c
index f5ea027..38723bf 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -214,5 +214,6 @@ bool aio_poll(AioContext *ctx, bool blocking)
         events[ret - WAIT_OBJECT_0] = events[--count];
     }
 
-    return progress;
+    assert(progress || busy);
+    return true;
 }
diff --git a/include/block/aio.h b/include/block/aio.h
index 0933f05..8eda924 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -173,16 +173,14 @@ bool aio_pending(AioContext *ctx);
  * aio as a result of executing I/O completion or bh callbacks.
  *
  * If there is no pending AIO operation or completion (bottom half),
- * return false.  If there are pending bottom halves, return true.
+ * return false.  If there are pending AIO operations of bottom halves,
+ * return true.
  *
  * If there are no pending bottom halves, but there are pending AIO
  * operations, it may not be possible to make any progress without
  * blocking.  If @blocking is true, this function will wait until one
  * or more AIO events have completed, to ensure something has moved
  * before returning.
- *
- * If @blocking is false, this function will also return false if the
- * function cannot make any progress without blocking.
  */
 bool aio_poll(AioContext *ctx, bool blocking);
 
diff --git a/tests/test-aio.c b/tests/test-aio.c
index e4ebef7..c173870 100644
--- a/tests/test-aio.c
+++ b/tests/test-aio.c
@@ -315,13 +315,13 @@ static void test_wait_event_notifier_noflush(void)
     event_notifier_set(&data.e);
     g_assert(aio_poll(ctx, false));
     g_assert_cmpint(data.n, ==, 1);
-    g_assert(!aio_poll(ctx, false));
+    g_assert(aio_poll(ctx, false));
     g_assert_cmpint(data.n, ==, 1);
 
     event_notifier_set(&data.e);
     g_assert(aio_poll(ctx, false));
     g_assert_cmpint(data.n, ==, 2);
-    g_assert(!aio_poll(ctx, false));
+    g_assert(aio_poll(ctx, false));
     g_assert_cmpint(data.n, ==, 2);
 
     event_notifier_set(&dummy.e);
-- 
1.8.0.2

[Qemu-devel] [PATCH 4/8] win32-aio: Fix vectored reads

[Stefan Hajnoczi]

From: Kevin Wolf <kwolf@redhat.com>

Copying data in the right direction really helps a lot!

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/win32-aio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/win32-aio.c b/block/win32-aio.c
index 0383370..e4b7b75 100644
--- a/block/win32-aio.c
+++ b/block/win32-aio.c
@@ -84,7 +84,7 @@ static void win32_aio_process_completion(QEMUWin32AIOState *s,
             int i;
 
             for (i = 0; i < qiov->niov; ++i) {
-                memcpy(p, qiov->iov[i].iov_base, qiov->iov[i].iov_len);
+                memcpy(qiov->iov[i].iov_base, p, qiov->iov[i].iov_len);
                 p += qiov->iov[i].iov_len;
             }
             qemu_vfree(waiocb->buf);
-- 
1.8.0.2

[Qemu-devel] [PATCH 5/8] win32-aio: Fix memory leak

[Stefan Hajnoczi]

From: Kevin Wolf <kwolf@redhat.com>

The buffer is allocated for both reads and writes, and obviously it
should be freed even if an error occurs.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/win32-aio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/win32-aio.c b/block/win32-aio.c
index e4b7b75..b9236ea 100644
--- a/block/win32-aio.c
+++ b/block/win32-aio.c
@@ -87,8 +87,8 @@ static void win32_aio_process_completion(QEMUWin32AIOState *s,
                 memcpy(qiov->iov[i].iov_base, p, qiov->iov[i].iov_len);
                 p += qiov->iov[i].iov_len;
             }
-            qemu_vfree(waiocb->buf);
         }
+        qemu_vfree(waiocb->buf);
     }
 
 
-- 
1.8.0.2

[Qemu-devel] [PATCH 6/8] win32-aio: use iov utility functions instead of open-coding them

[Stefan Hajnoczi]

From: Michael Tokarev <mjt@tls.msk.ru>

We have iov_from_buf() and iov_to_buf(), use them instead of
open-coding these in block/win32-aio.c

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/win32-aio.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/block/win32-aio.c b/block/win32-aio.c
index b9236ea..5d0fbbf 100644
--- a/block/win32-aio.c
+++ b/block/win32-aio.c
@@ -29,6 +29,7 @@
 #include "block/aio.h"
 #include "raw-aio.h"
 #include "qemu/event_notifier.h"
+#include "qemu/iov.h"
 #include <windows.h>
 #include <winioctl.h>
 
@@ -80,13 +81,7 @@ static void win32_aio_process_completion(QEMUWin32AIOState *s,
     if (!waiocb->is_linear) {
         if (ret == 0 && waiocb->is_read) {
             QEMUIOVector *qiov = waiocb->qiov;
-            char *p = waiocb->buf;
-            int i;
-
-            for (i = 0; i < qiov->niov; ++i) {
-                memcpy(qiov->iov[i].iov_base, p, qiov->iov[i].iov_len);
-                p += qiov->iov[i].iov_len;
-            }
+            iov_from_buf(qiov->iov, qiov->niov, 0, waiocb->buf, qiov->size);
         }
         qemu_vfree(waiocb->buf);
     }
@@ -153,13 +148,7 @@ BlockDriverAIOCB *win32_aio_submit(BlockDriverState *bs,
     if (qiov->niov > 1) {
         waiocb->buf = qemu_blockalign(bs, qiov->size);
         if (type & QEMU_AIO_WRITE) {
-            char *p = waiocb->buf;
-            int i;
-
-            for (i = 0; i < qiov->niov; ++i) {
-                memcpy(p, qiov->iov[i].iov_base, qiov->iov[i].iov_len);
-                p += qiov->iov[i].iov_len;
-            }
+            iov_to_buf(qiov->iov, qiov->niov, 0, waiocb->buf, qiov->size);
         }
         waiocb->is_linear = false;
     } else {
-- 
1.8.0.2

[Qemu-devel] [PATCH 7/8] dataplane: avoid reentrancy during virtio_blk_data_plane_stop()

[Stefan Hajnoczi]

When dataplane is stopping, the s->vdev->binding->set_host_notifier(...,
false) call can invoke the virtqueue handler if an ioeventfd
notification is pending.  This causes hw/virtio-blk.c to invoke
virtio_blk_data_plane_start() before virtio_blk_data_plane_stop()
returns!

The result is that we try to restart dataplane while trying to stop it
and the following assertion is raised:

  msix_set_mask_notifier: Assertion `!dev->msix_mask_notifier' failed.

Although the code was intended to prevent this scenario, the s->started
boolean isn't enough.  Add s->stopping so that we can postpone clearing
s->started until we've completely stopped dataplane.

This way, virtqueue handler calls during virtio_blk_data_plane_stop()
are ignored.  When dataplane is legitimately started again later we
already self-kick ourselves to resume processing.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/dataplane/virtio-blk.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/dataplane/virtio-blk.c b/hw/dataplane/virtio-blk.c
index 4b26faa..3f2da22 100644
--- a/hw/dataplane/virtio-blk.c
+++ b/hw/dataplane/virtio-blk.c
@@ -40,6 +40,7 @@ typedef struct {
 
 struct VirtIOBlockDataPlane {
     bool started;
+    bool stopping;
     QEMUBH *start_bh;
     QemuThread thread;
 
@@ -357,7 +358,7 @@ static void *data_plane_thread(void *opaque)
 
     do {
         event_poll(&s->event_poll);
-    } while (s->started || s->num_reqs > 0);
+    } while (!s->stopping || s->num_reqs > 0);
     return NULL;
 }
 
@@ -486,10 +487,10 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 
 void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
 {
-    if (!s->started) {
+    if (!s->started || s->stopping) {
         return;
     }
-    s->started = false;
+    s->stopping = true;
     trace_virtio_blk_data_plane_stop(s);
 
     /* Stop thread or cancel pending thread creation BH */
@@ -511,4 +512,6 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     s->vdev->binding->set_guest_notifiers(s->vdev->binding_opaque, 1, false);
 
     vring_teardown(&s->vring);
+    s->started = false;
+    s->stopping = false;
 }
-- 
1.8.0.2

[Qemu-devel] [PATCH 8/8] dataplane: support viostor virtio-pci status bit setting

[Stefan Hajnoczi]

The viostor virtio-blk driver for Windows does not use the
VIRTIO_CONFIG_S_DRIVER bit.  It only sets the VIRTIO_CONFIG_S_DRIVER_OK
bit.

The viostor driver refreshes the virtio-pci status byte sometimes while
the guest is running.  We misinterpret 0x4 (VIRTIO_CONFIG_S_DRIVER_OK)
as an indication that virtio-blk-data-plane should be stopped since 0x2
(VIRTIO_CONFIG_S_DRIVER) is missing.  The result is that the device
becomes unresponsive.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio-blk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index df57b35..34913ee 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -571,7 +571,8 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
     uint32_t features;
 
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
-    if (s->dataplane && !(status & VIRTIO_CONFIG_S_DRIVER)) {
+    if (s->dataplane && !(status & (VIRTIO_CONFIG_S_DRIVER |
+                                    VIRTIO_CONFIG_S_DRIVER_OK))) {
         virtio_blk_data_plane_stop(s->dataplane);
     }
 #endif
-- 
1.8.0.2

Re: [Qemu-devel] [PULL for-1.4 0/8] Block patches

[Anthony Liguori]

Pulled.  Thanks.

Regards,

Anthony Liguori