From 6d759117d3fd28e38c49c56c9de206cc718d32fa Mon Sep 17 00:00:00 2001 From: Jeff Cody Date: Tue, 15 Jan 2013 10:47:24 -0500 Subject: block: fix null-pointer bug on error case in block commit 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 Signed-off-by: Jeff Cody Signed-off-by: Stefan Hajnoczi --- block/commit.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/block/commit.c b/block/commit.c index 61ebdba54f..553447efe7 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); } -- cgit v1.2.1 From 6bf3ee07ff55aa795010a8e071826f38e9a26112 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 16 Jan 2013 10:54:34 +0100 Subject: ide: Remove wrong assertion 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 Signed-off-by: Stefan Hajnoczi --- hw/ide/pci.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/ide/pci.c b/hw/ide/pci.c index e6226e3197..59fd53992a 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; -- cgit v1.2.1 From 2ea9b58f0bc62445b7ace2381b4c4db7d5597e19 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 16 Jan 2013 19:25:51 +0100 Subject: aio: Fix return value of aio_poll() 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 Signed-off-by: Stefan Hajnoczi --- 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 88d09e1cfb..fe4dbb4523 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 f5ea027f8c..38723bf1d3 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 0933f05878..8eda924599 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 e4ebef76b9..c1738706cd 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); -- cgit v1.2.1 From bcbbd234d42f1111e42b91376db61922d42e7e9e Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 16 Jan 2013 21:19:59 +0100 Subject: win32-aio: Fix vectored reads Copying data in the right direction really helps a lot! Cc: qemu-stable@nongnu.org Signed-off-by: Kevin Wolf Signed-off-by: Stefan Hajnoczi --- 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 03833700b4..e4b7b75d29 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); -- cgit v1.2.1 From e8bccad5ac6095b5af7946cd72d9aacb57f7c0a3 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 16 Jan 2013 21:20:00 +0100 Subject: win32-aio: Fix memory leak 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 Signed-off-by: Stefan Hajnoczi --- 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 e4b7b75d29..b9236ea74d 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); } -- cgit v1.2.1 From 3249dbe661ba6ef108ecde97c54b4a4104d719c3 Mon Sep 17 00:00:00 2001 From: Michael Tokarev Date: Thu, 17 Jan 2013 18:47:52 +0400 Subject: win32-aio: use iov utility functions instead of open-coding them 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 --- 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 b9236ea74d..5d0fbbfb7d 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 #include @@ -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 { -- cgit v1.2.1 From cd7fdfe59f4f965665dcd9868fe3764f5256d6aa Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 15 Jan 2013 17:19:38 +0100 Subject: dataplane: avoid reentrancy during virtio_blk_data_plane_stop() 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 --- 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 4b26faa6c2..3f2da22669 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; } -- cgit v1.2.1 From cf139388ad5b39228793f34eea99e0ea9a2924aa Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 17 Jan 2013 16:46:54 +0100 Subject: dataplane: support viostor virtio-pci status bit setting 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 --- 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 df57b35f1b..34913ee40e 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 -- cgit v1.2.1