Series comparison

-[Qemu-devel] [PULL 0/7] Block patches
+[PULL for-6.0 0/6] Block patches
-The following changes since commit 464588675455afda2899e20a0b120e4075de50c7:
+The following changes since commit 6d40ce00c1166c317e298ad82ecf10e650c4f87d:
-  Merge remote-tracking branch 'remotes/sstabellini/tags/xen-20170627-tag' into staging (2017-06-29 11:45:01 +0100)
+  Update version for v6.0.0-rc1 release (2021-03-30 18:19:07 +0100)
-are available in the git repository at:
+are available in the Git repository at:
-  git://github.com/stefanha/qemu.git tags/block-pull-request
+  https://gitlab.com/stefanha/qemu.git tags/block-pull-request
-for you to fetch changes up to c324fd0a39cee43c13f6d1fb34f74fc5e2fb007b:
+for you to fetch changes up to b6489ac06695e257ea0a9841364577e247fdee30:
-  virtio-pci: use ioeventfd even when KVM is disabled (2017-06-30 11:03:45 +0100)
+  test-coroutine: Add rwlock downgrade test (2021-03-31 10:44:21 +0100)
 ----------------------------------------------------------------
 Pull request
 A fix for VDI image files and more generally for CoRwlock.
 ----------------------------------------------------------------
-----------------------------------------------------------------
+David Edmondson (4):
   block/vdi: When writing new bmap entry fails, don't leak the buffer
   block/vdi: Don't assume that blocks are larger than VdiHeader
   coroutine-lock: Store the coroutine in the CoWaitRecord only once
   test-coroutine: Add rwlock downgrade test
-Stefan Hajnoczi (7):
+Paolo Bonzini (2):
-  virtio-blk: trace vdev so devices can be distinguished
+  coroutine-lock: Reimplement CoRwlock to fix downgrade bug
-  libqos: fix typo in virtio.h QVirtQueue->used comment
+  test-coroutine: Add rwlock upgrade test
   libqos: add virtio used ring support
   tests: fix virtio-scsi-test ISR dependence
   tests: fix virtio-blk-test ISR dependence
   tests: fix virtio-net-test ISR dependence
   virtio-pci: use ioeventfd even when KVM is disabled
- tests/libqos/virtio.h    |  8 ++++++-
+ include/qemu/coroutine.h    |  17 ++--
- hw/block/virtio-blk.c    | 12 ++++++----
+ block/vdi.c                 |  11 ++-
- hw/virtio/virtio-pci.c   |  2 +-
+ tests/unit/test-coroutine.c | 161 +++++++++++++++++++++++++++++++++++
- tests/libqos/virtio.c    | 60 ++++++++++++++++++++++++++++++++++++++++++++++++
+ util/qemu-coroutine-lock.c  | 165 +++++++++++++++++++++++-------------
- tests/virtio-blk-test.c  | 27 ++++++++++++++--------
+files changed, 282 insertions(+), 72 deletions(-)
  tests/virtio-net-test.c  |  6 ++---
  tests/virtio-scsi-test.c |  2 +-
  hw/block/trace-events    | 10 ++++----
 files changed, 101 insertions(+), 26 deletions(-)
 --
-.9.4
+.30.2

-[Qemu-devel] [PULL 1/7] virtio-blk: trace vdev so devices can be distinguished
+Deleted patch
-It is hard to analyze trace logs with multiple virtio-blk devices
-because none of the trace events include the VirtIODevice *vdev.
-This patch adds vdev so it's clear which device a request is associated
-with.
-I considered using VirtIOBlock *s instead but VirtIODevice *vdev is more
-general and may be correlated with generic virtio trace events like
-virtio_set_status.
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
-Reviewed-by: Fam Zheng <famz@redhat.com>
-Message-id: 20170614092930.11234-1-stefanha@redhat.com
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
----
- hw/block/virtio-blk.c | 12 +++++++-----
- hw/block/trace-events | 10 +++++-----
-files changed, 12 insertions(+), 10 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 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
-     VirtIOBlock *s = req->dev;
-     VirtIODevice *vdev = VIRTIO_DEVICE(s);
--    trace_virtio_blk_req_complete(req, status);
-+    trace_virtio_blk_req_complete(vdev, req, status);
-     stb_p(&req->in->status, status);
-     virtqueue_push(req->vq, &req->elem, req->in_len);
-@@ -XXX,XX +XXX,XX @@ static void virtio_blk_rw_complete(void *opaque, int ret)
- {
-     VirtIOBlockReq *next = opaque;
-     VirtIOBlock *s = next->dev;
-+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
-     aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
-     while (next) {
-         VirtIOBlockReq *req = next;
-         next = req->mr_next;
--        trace_virtio_blk_rw_complete(req, ret);
-+        trace_virtio_blk_rw_complete(vdev, req, ret);
-         if (req->qiov.nalloc != -1) {
-             /* If nalloc is != 1 req->qiov is a local copy of the original
-@@ -XXX,XX +XXX,XX @@ static inline void submit_requests(BlockBackend *blk, MultiReqBuffer *mrb,
-             mrb->reqs[i - 1]->mr_next = mrb->reqs[i];
-         }
--        trace_virtio_blk_submit_multireq(mrb, start, num_reqs,
-+        trace_virtio_blk_submit_multireq(VIRTIO_DEVICE(mrb->reqs[start]->dev),
-+                                         mrb, start, num_reqs,
-                                          sector_num << BDRV_SECTOR_BITS,
-                                          qiov->size, is_write);
-         block_acct_merge_done(blk_get_stats(blk),
-@@ -XXX,XX +XXX,XX @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
-         if (is_write) {
-             qemu_iovec_init_external(&req->qiov, iov, out_num);
--            trace_virtio_blk_handle_write(req, req->sector_num,
-+            trace_virtio_blk_handle_write(vdev, req, req->sector_num,
-                                           req->qiov.size / BDRV_SECTOR_SIZE);
-         } else {
-             qemu_iovec_init_external(&req->qiov, in_iov, in_num);
--            trace_virtio_blk_handle_read(req, req->sector_num,
-+            trace_virtio_blk_handle_read(vdev, req, req->sector_num,
-                                          req->qiov.size / BDRV_SECTOR_SIZE);
-         }
-diff --git a/hw/block/trace-events b/hw/block/trace-events
-index XXXXXXX..XXXXXXX 100644
---- a/hw/block/trace-events
-+++ b/hw/block/trace-events
-@@ -XXX,XX +XXX,XX @@
- # See docs/tracing.txt for syntax documentation.
- # hw/block/virtio-blk.c
--virtio_blk_req_complete(void *req, int status) "req %p status %d"
--virtio_blk_rw_complete(void *req, int ret) "req %p ret %d"
--virtio_blk_handle_write(void *req, uint64_t sector, size_t nsectors) "req %p sector %"PRIu64" nsectors %zu"
--virtio_blk_handle_read(void *req, uint64_t sector, size_t nsectors) "req %p sector %"PRIu64" nsectors %zu"
--virtio_blk_submit_multireq(void *mrb, int start, int num_reqs, uint64_t offset, size_t size, bool is_write) "mrb %p start %d num_reqs %d offset %"PRIu64" size %zu is_write %d"
-+virtio_blk_req_complete(void *vdev, void *req, int status) "vdev %p req %p status %d"
-+virtio_blk_rw_complete(void *vdev, void *req, int ret) "vdev %p req %p ret %d"
-+virtio_blk_handle_write(void *vdev, void *req, uint64_t sector, size_t nsectors) "vdev %p req %p sector %"PRIu64" nsectors %zu"
-+virtio_blk_handle_read(void *vdev, void *req, uint64_t sector, size_t nsectors) "vdev %p req %p sector %"PRIu64" nsectors %zu"
-+virtio_blk_submit_multireq(void *vdev, void *mrb, int start, int num_reqs, uint64_t offset, size_t size, bool is_write) "vdev %p mrb %p start %d num_reqs %d offset %"PRIu64" size %zu is_write %d"
- # hw/block/hd-geometry.c
- hd_geometry_lchs_guess(void *blk, int cyls, int heads, int secs) "blk %p LCHS %d %d %d"
---
-.9.4

-[Qemu-devel] [PULL 7/7] virtio-pci: use ioeventfd even when KVM is disabled
+[PULL for-6.0 1/6] block/vdi: When writing new bmap entry fails, don't leak the buffer
-Old kvm.ko versions only supported a tiny number of ioeventfds so
+From: David Edmondson <david.edmondson@oracle.com>
 virtio-pci avoids ioeventfds when kvm_has_many_ioeventfds() returns 0.
-Do not check kvm_has_many_ioeventfds() when KVM is disabled since it
+If a new bitmap entry is allocated, requiring the entire block to be
-always returns 0.  Since commit 8c56c1a592b5092d91da8d8943c17777d6462a6f
+written, avoiding leaking the buffer allocated for the block should
-("memory: emulate ioeventfd") it has been possible to use ioeventfds in
+the write fail.
 qtest or TCG mode.
-This patch makes -device virtio-blk-pci,iothread=iothread0 work even
+Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
-when KVM is disabled.
+Signed-off-by: David Edmondson <david.edmondson@oracle.com>
+Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
-I have tested that virtio-blk-pci works under TCG both with and without
+Acked-by: Max Reitz <mreitz@redhat.com>
-iothread.
+Message-id: 20210325112941.365238-2-pbonzini@redhat.com
+Message-Id: <20210309144015.557477-2-david.edmondson@oracle.com>
-This patch fixes qemu-iotests 068, which was accidentally merged early
+Acked-by: Max Reitz <mreitz@redhat.com>
-despite the dependency on ioeventfd.
+Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
 Cc: Michael S. Tsirkin <mst@redhat.com>
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
 Reviewed-by: Fam Zheng <famz@redhat.com>
 Tested-by: Eric Blake <eblake@redhat.com>
 Tested-by: Kevin Wolf <kwolf@redhat.com>
 Message-id: 20170628184724.21378-7-stefanha@redhat.com
 Message-id: 20170615163813.7255-2-stefanha@redhat.com
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- hw/virtio/virtio-pci.c | 2 +-
+ block/vdi.c | 1 +
-file changed, 1 insertion(+), 1 deletion(-)
+file changed, 1 insertion(+)
-diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
+diff --git a/block/vdi.c b/block/vdi.c
 index XXXXXXX..XXXXXXX 100644
---- a/hw/virtio/virtio-pci.c
+--- a/block/vdi.c
-+++ b/hw/virtio/virtio-pci.c
++++ b/block/vdi.c
-@@ -XXX,XX +XXX,XX @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
+@@ -XXX,XX +XXX,XX @@ nonallocating_write:
-     bool pcie_port = pci_bus_is_express(pci_dev->bus) &&
-                      !pci_bus_is_root(pci_dev->bus);
+     logout("finished data write\n");
+     if (ret < 0) {
--    if (!kvm_has_many_ioeventfds()) {
++        g_free(block);
-+    if (kvm_enabled() && !kvm_has_many_ioeventfds()) {
+         return ret;
          proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
      }
 --
-.9.4
+.30.2

-[Qemu-devel] [PULL 6/7] tests: fix virtio-net-test ISR dependence
+[PULL for-6.0 2/6] block/vdi: Don't assume that blocks are larger than VdiHeader
-Use the new used ring APIs instead of assuming ISR being set means the
+From: David Edmondson <david.edmondson@oracle.com>
 request has completed.
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
+Given that the block size is read from the header of the VDI file, a
-Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
+wide variety of sizes might be seen. Rather than re-using a block
-Reviewed-by: Fam Zheng <famz@redhat.com>
+sized memory region when writing the VDI header, allocate an
-Tested-by: Eric Blake <eblake@redhat.com>
+appropriately sized buffer.
-Tested-by: Kevin Wolf <kwolf@redhat.com>
-Message-id: 20170628184724.21378-6-stefanha@redhat.com
+Signed-off-by: David Edmondson <david.edmondson@oracle.com>
 Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
 Acked-by: Max Reitz <mreitz@redhat.com>
 Message-id: 20210325112941.365238-3-pbonzini@redhat.com
 Message-Id: <20210309144015.557477-3-david.edmondson@oracle.com>
 Acked-by: Max Reitz <mreitz@redhat.com>
 Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- tests/virtio-net-test.c | 6 +++---
+ block/vdi.c | 10 ++++++----
-file changed, 3 insertions(+), 3 deletions(-)
+file changed, 6 insertions(+), 4 deletions(-)
-diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
+diff --git a/block/vdi.c b/block/vdi.c
 index XXXXXXX..XXXXXXX 100644
---- a/tests/virtio-net-test.c
+--- a/block/vdi.c
-+++ b/tests/virtio-net-test.c
++++ b/block/vdi.c
-@@ -XXX,XX +XXX,XX @@ static void rx_test(QVirtioDevice *dev,
+@@ -XXX,XX +XXX,XX @@ nonallocating_write:
-     ret = iov_send(socket, iov, 2, 0, sizeof(len) + sizeof(test));
-     g_assert_cmpint(ret, ==, sizeof(test) + sizeof(len));
+     if (block) {
+         /* One or more new blocks were allocated. */
--    qvirtio_wait_queue_isr(dev, vq, QVIRTIO_NET_TIMEOUT_US);
+-        VdiHeader *header = (VdiHeader *) block;
-+    qvirtio_wait_used_elem(dev, vq, free_head, QVIRTIO_NET_TIMEOUT_US);
++        VdiHeader *header;
-     memread(req_addr + VNET_HDR_SIZE, buffer, sizeof(test));
+         uint8_t *base;
-     g_assert_cmpstr(buffer, ==, "TEST");
+         uint64_t offset;
+         uint32_t n_sectors;
-@@ -XXX,XX +XXX,XX @@ static void tx_test(QVirtioDevice *dev,
-     free_head = qvirtqueue_add(vq, req_addr, 64, false, false);
++        g_free(block);
-     qvirtqueue_kick(dev, vq, free_head);
++        header = g_malloc(sizeof(*header));
++
--    qvirtio_wait_queue_isr(dev, vq, QVIRTIO_NET_TIMEOUT_US);
+         logout("now writing modified header\n");
-+    qvirtio_wait_used_elem(dev, vq, free_head, QVIRTIO_NET_TIMEOUT_US);
+         assert(VDI_IS_ALLOCATED(bmap_first));
-     guest_free(alloc, req_addr);
+         *header = s->header;
+         vdi_header_to_le(header);
-     ret = qemu_recv(socket, &len, sizeof(len), 0);
+-        ret = bdrv_pwrite(bs->file, 0, block, sizeof(VdiHeader));
-@@ -XXX,XX +XXX,XX @@ static void rx_stop_cont_test(QVirtioDevice *dev,
+-        g_free(block);
-     rsp = qmp("{ 'execute' : 'cont'}");
+-        block = NULL;
-     QDECREF(rsp);
++        ret = bdrv_pwrite(bs->file, 0, header, sizeof(*header));
++        g_free(header);
--    qvirtio_wait_queue_isr(dev, vq, QVIRTIO_NET_TIMEOUT_US);
-+    qvirtio_wait_used_elem(dev, vq, free_head, QVIRTIO_NET_TIMEOUT_US);
+         if (ret < 0) {
-     memread(req_addr + VNET_HDR_SIZE, buffer, sizeof(test));
+             return ret;
      g_assert_cmpstr(buffer, ==, "TEST");
 --
-.9.4
+.30.2

-[Qemu-devel] [PULL 5/7] tests: fix virtio-blk-test ISR dependence
+[PULL for-6.0 3/6] coroutine-lock: Store the coroutine in the CoWaitRecord only once
-Use the new used ring APIs instead of assuming ISR being set means the
+From: David Edmondson <david.edmondson@oracle.com>
 request has completed.
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
+When taking the slow path for mutex acquisition, set the coroutine
-Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
+value in the CoWaitRecord in push_waiter(), rather than both there and
-Reviewed-by: Fam Zheng <famz@redhat.com>
+in the caller.
-Tested-by: Eric Blake <eblake@redhat.com>
-Tested-by: Kevin Wolf <kwolf@redhat.com>
+Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
-Message-id: 20170628184724.21378-5-stefanha@redhat.com
+Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
 Signed-off-by: David Edmondson <david.edmondson@oracle.com>
 Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
 Message-id: 20210325112941.365238-4-pbonzini@redhat.com
 Message-Id: <20210309144015.557477-4-david.edmondson@oracle.com>
 Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- tests/virtio-blk-test.c | 27 +++++++++++++++++----------
+ util/qemu-coroutine-lock.c | 1 -
-file changed, 17 insertions(+), 10 deletions(-)
+file changed, 1 deletion(-)
-diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
+diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
 index XXXXXXX..XXXXXXX 100644
---- a/tests/virtio-blk-test.c
+--- a/util/qemu-coroutine-lock.c
-+++ b/tests/virtio-blk-test.c
++++ b/util/qemu-coroutine-lock.c
-@@ -XXX,XX +XXX,XX @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
+@@ -XXX,XX +XXX,XX @@ static void coroutine_fn qemu_co_mutex_lock_slowpath(AioContext *ctx,
+     unsigned old_handoff;
-     qvirtqueue_kick(dev, vq, free_head);
+     trace_qemu_co_mutex_lock_entry(mutex, self);
--    qvirtio_wait_queue_isr(dev, vq, QVIRTIO_BLK_TIMEOUT_US);
+-    w.co = self;
-+    qvirtio_wait_used_elem(dev, vq, free_head, QVIRTIO_BLK_TIMEOUT_US);
+     push_waiter(mutex, &w);
-     status = readb(req_addr + 528);
-     g_assert_cmpint(status, ==, 0);
+     /* This is the "Responsibility Hand-Off" protocol; a lock() picks from
@@ -XXX,XX +XXX,XX @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
      qvirtqueue_kick(dev, vq, free_head);
 -    qvirtio_wait_queue_isr(dev, vq, QVIRTIO_BLK_TIMEOUT_US);
 +    qvirtio_wait_used_elem(dev, vq, free_head, QVIRTIO_BLK_TIMEOUT_US);
      status = readb(req_addr + 528);
      g_assert_cmpint(status, ==, 0);
@@ -XXX,XX +XXX,XX @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
          qvirtqueue_add(vq, req_addr + 528, 1, true, false);
          qvirtqueue_kick(dev, vq, free_head);
 -        qvirtio_wait_queue_isr(dev, vq, QVIRTIO_BLK_TIMEOUT_US);
 +        qvirtio_wait_used_elem(dev, vq, free_head, QVIRTIO_BLK_TIMEOUT_US);
          status = readb(req_addr + 528);
          g_assert_cmpint(status, ==, 0);
@@ -XXX,XX +XXX,XX @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
          qvirtqueue_kick(dev, vq, free_head);
 -        qvirtio_wait_queue_isr(dev, vq, QVIRTIO_BLK_TIMEOUT_US);
 +        qvirtio_wait_used_elem(dev, vq, free_head, QVIRTIO_BLK_TIMEOUT_US);
          status = readb(req_addr + 528);
          g_assert_cmpint(status, ==, 0);
@@ -XXX,XX +XXX,XX @@ static void pci_indirect(void)
      free_head = qvirtqueue_add_indirect(&vqpci->vq, indirect);
      qvirtqueue_kick(&dev->vdev, &vqpci->vq, free_head);
 -    qvirtio_wait_queue_isr(&dev->vdev, &vqpci->vq,
 +    qvirtio_wait_used_elem(&dev->vdev, &vqpci->vq, free_head,
                             QVIRTIO_BLK_TIMEOUT_US);
      status = readb(req_addr + 528);
      g_assert_cmpint(status, ==, 0);
@@ -XXX,XX +XXX,XX @@ static void pci_indirect(void)
      free_head = qvirtqueue_add_indirect(&vqpci->vq, indirect);
      qvirtqueue_kick(&dev->vdev, &vqpci->vq, free_head);
 -    qvirtio_wait_queue_isr(&dev->vdev, &vqpci->vq,
 +    qvirtio_wait_used_elem(&dev->vdev, &vqpci->vq, free_head,
                             QVIRTIO_BLK_TIMEOUT_US);
      status = readb(req_addr + 528);
      g_assert_cmpint(status, ==, 0);
@@ -XXX,XX +XXX,XX @@ static void pci_msix(void)
      qvirtqueue_add(&vqpci->vq, req_addr + 528, 1, true, false);
      qvirtqueue_kick(&dev->vdev, &vqpci->vq, free_head);
 -    qvirtio_wait_queue_isr(&dev->vdev, &vqpci->vq,
 +    qvirtio_wait_used_elem(&dev->vdev, &vqpci->vq, free_head,
                             QVIRTIO_BLK_TIMEOUT_US);
      status = readb(req_addr + 528);
@@ -XXX,XX +XXX,XX @@ static void pci_msix(void)
      qvirtqueue_kick(&dev->vdev, &vqpci->vq, free_head);
 -    qvirtio_wait_queue_isr(&dev->vdev, &vqpci->vq,
 +    qvirtio_wait_used_elem(&dev->vdev, &vqpci->vq, free_head,
                             QVIRTIO_BLK_TIMEOUT_US);
      status = readb(req_addr + 528);
@@ -XXX,XX +XXX,XX @@ static void pci_idx(void)
      uint64_t capacity;
      uint32_t features;
      uint32_t free_head;
 +    uint32_t write_head;
 +    uint32_t desc_idx;
      uint8_t status;
      char *data;
@@ -XXX,XX +XXX,XX @@ static void pci_idx(void)
      qvirtqueue_add(&vqpci->vq, req_addr + 528, 1, true, false);
      qvirtqueue_kick(&dev->vdev, &vqpci->vq, free_head);
 -    qvirtio_wait_queue_isr(&dev->vdev, &vqpci->vq, QVIRTIO_BLK_TIMEOUT_US);
 +    qvirtio_wait_used_elem(&dev->vdev, &vqpci->vq, free_head,
 +                           QVIRTIO_BLK_TIMEOUT_US);
      /* Write request */
      req.type = VIRTIO_BLK_T_OUT;
@@ -XXX,XX +XXX,XX @@ static void pci_idx(void)
      qvirtqueue_add(&vqpci->vq, req_addr + 16, 512, false, true);
      qvirtqueue_add(&vqpci->vq, req_addr + 528, 1, true, false);
      qvirtqueue_kick(&dev->vdev, &vqpci->vq, free_head);
 +    write_head = free_head;
      /* No notification expected */
      status = qvirtio_wait_status_byte_no_isr(&dev->vdev,
@@ -XXX,XX +XXX,XX @@ static void pci_idx(void)
      qvirtqueue_kick(&dev->vdev, &vqpci->vq, free_head);
 -    qvirtio_wait_queue_isr(&dev->vdev, &vqpci->vq,
 +    /* We get just one notification for both requests */
 +    qvirtio_wait_used_elem(&dev->vdev, &vqpci->vq, write_head,
                             QVIRTIO_BLK_TIMEOUT_US);
 +    g_assert(qvirtqueue_get_buf(&vqpci->vq, &desc_idx));
 +    g_assert_cmpint(desc_idx, ==, free_head);
      status = readb(req_addr + 528);
      g_assert_cmpint(status, ==, 0);
 --
-.9.4
+.30.2

-[Qemu-devel] [PULL 4/7] tests: fix virtio-scsi-test ISR dependence
+[PULL for-6.0 4/6] coroutine-lock: Reimplement CoRwlock to fix downgrade bug
-Use the new used ring APIs instead of assuming ISR being set means the
+From: Paolo Bonzini <pbonzini@redhat.com>
-request has completed.
+An invariant of the current rwlock is that if multiple coroutines hold a
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
+reader lock, all must be runnable. The unlock implementation relies on
-Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
+this, choosing to wake a single coroutine when the final read lock
-Reviewed-by: Fam Zheng <famz@redhat.com>
+holder exits the critical section, assuming that it will wake a
-Tested-by: Eric Blake <eblake@redhat.com>
+coroutine attempting to acquire a write lock.
-Tested-by: Kevin Wolf <kwolf@redhat.com>
-Message-id: 20170628184724.21378-4-stefanha@redhat.com
+The downgrade implementation violates this assumption by creating a
 read lock owning coroutine that is exclusively runnable - any other
 coroutines that are waiting to acquire a read lock are *not* made
 runnable when the write lock holder converts its ownership to read
 only.
 More in general, the old implementation had lots of other fairness bugs.
 The root cause of the bugs was that CoQueue would wake up readers even
 if there were pending writers, and would wake up writers even if there
 were readers.  In that case, the coroutine would go back to sleep *at
 the end* of the CoQueue, losing its place at the head of the line.
 To fix this, keep the queue of waiters explicitly in the CoRwlock
 instead of using CoQueue, and store for each whether it is a
 potential reader or a writer.  This way, downgrade can look at the
 first queued coroutines and wake it only if it is a reader, causing
 all other readers in line to be released in turn.
 Reported-by: David Edmondson <david.edmondson@oracle.com>
 Reviewed-by: David Edmondson <david.edmondson@oracle.com>
 Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
 Message-id: 20210325112941.365238-5-pbonzini@redhat.com
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- tests/virtio-scsi-test.c | 2 +-
+ include/qemu/coroutine.h   |  17 ++--
-file changed, 1 insertion(+), 1 deletion(-)
+ util/qemu-coroutine-lock.c | 164 +++++++++++++++++++++++--------------
+files changed, 114 insertions(+), 67 deletions(-)
-diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
 diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
 index XXXXXXX..XXXXXXX 100644
---- a/tests/virtio-scsi-test.c
+--- a/include/qemu/coroutine.h
-+++ b/tests/virtio-scsi-test.c
++++ b/include/qemu/coroutine.h
-@@ -XXX,XX +XXX,XX @@ static uint8_t virtio_scsi_do_command(QVirtIOSCSI *vs, const uint8_t *cdb,
+@@ -XXX,XX +XXX,XX @@ bool qemu_co_enter_next_impl(CoQueue *queue, QemuLockable *lock);
  bool qemu_co_queue_empty(CoQueue *queue);
 +typedef struct CoRwTicket CoRwTicket;
  typedef struct CoRwlock {
 -    int pending_writer;
 -    int reader;
      CoMutex mutex;
 -    CoQueue queue;
 +
 +    /* Number of readers, or -1 if owned for writing.  */
 +    int owners;
 +
 +    /* Waiting coroutines.  */
 +    QSIMPLEQ_HEAD(, CoRwTicket) tickets;
  } CoRwlock;
  /**
@@ -XXX,XX +XXX,XX @@ void qemu_co_rwlock_rdlock(CoRwlock *lock);
  /**
   * Write Locks the CoRwlock from a reader.  This is a bit more efficient than
   * @qemu_co_rwlock_unlock followed by a separate @qemu_co_rwlock_wrlock.
 - * However, if the lock cannot be upgraded immediately, control is transferred
 - * to the caller of the current coroutine.  Also, @qemu_co_rwlock_upgrade
 - * only overrides CoRwlock fairness if there are no concurrent readers, so
 - * another writer might run while @qemu_co_rwlock_upgrade blocks.
 + * Note that if the lock cannot be upgraded immediately, control is transferred
 + * to the caller of the current coroutine; another writer might run while
 + * @qemu_co_rwlock_upgrade blocks.
   */
  void qemu_co_rwlock_upgrade(CoRwlock *lock);
 diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
 index XXXXXXX..XXXXXXX 100644
 --- a/util/qemu-coroutine-lock.c
 +++ b/util/qemu-coroutine-lock.c
@@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex)
      trace_qemu_co_mutex_unlock_return(mutex, self);
  }
 +struct CoRwTicket {
 +    bool read;
 +    Coroutine *co;
 +    QSIMPLEQ_ENTRY(CoRwTicket) next;
 +};
 +
  void qemu_co_rwlock_init(CoRwlock *lock)
  {
 -    memset(lock, 0, sizeof(*lock));
 -    qemu_co_queue_init(&lock->queue);
      qemu_co_mutex_init(&lock->mutex);
 +    lock->owners = 0;
 +    QSIMPLEQ_INIT(&lock->tickets);
 +}
 +
 +/* Releases the internal CoMutex.  */
 +static void qemu_co_rwlock_maybe_wake_one(CoRwlock *lock)
 +{
 +    CoRwTicket *tkt = QSIMPLEQ_FIRST(&lock->tickets);
 +    Coroutine *co = NULL;
 +
 +    /*
 +     * Setting lock->owners here prevents rdlock and wrlock from
 +     * sneaking in between unlock and wake.
 +     */
 +
 +    if (tkt) {
 +        if (tkt->read) {
 +            if (lock->owners >= 0) {
 +                lock->owners++;
 +                co = tkt->co;
 +            }
 +        } else {
 +            if (lock->owners == 0) {
 +                lock->owners = -1;
 +                co = tkt->co;
 +            }
 +        }
 +    }
 +
 +    if (co) {
 +        QSIMPLEQ_REMOVE_HEAD(&lock->tickets, next);
 +        qemu_co_mutex_unlock(&lock->mutex);
 +        aio_co_wake(co);
 +    } else {
 +        qemu_co_mutex_unlock(&lock->mutex);
 +    }
  }
  void qemu_co_rwlock_rdlock(CoRwlock *lock)
@@ -XXX,XX +XXX,XX @@ void qemu_co_rwlock_rdlock(CoRwlock *lock)
      qemu_co_mutex_lock(&lock->mutex);
      /* For fairness, wait if a writer is in line.  */
 -    while (lock->pending_writer) {
 -        qemu_co_queue_wait(&lock->queue, &lock->mutex);
 -    }
 -    lock->reader++;
 -    qemu_co_mutex_unlock(&lock->mutex);
 -
 -    /* The rest of the read-side critical section is run without the mutex.  */
 -    self->locks_held++;
 -}
 -
 -void qemu_co_rwlock_unlock(CoRwlock *lock)
 -{
 -    Coroutine *self = qemu_coroutine_self();
 -
 -    assert(qemu_in_coroutine());
 -    if (!lock->reader) {
 -        /* The critical section started in qemu_co_rwlock_wrlock.  */
 -        qemu_co_queue_restart_all(&lock->queue);
 +    if (lock->owners == 0 || (lock->owners > 0 && QSIMPLEQ_EMPTY(&lock->tickets))) {
 +        lock->owners++;
 +        qemu_co_mutex_unlock(&lock->mutex);
      } else {
 -        self->locks_held--;
 +        CoRwTicket my_ticket = { true, self };
 +        QSIMPLEQ_INSERT_TAIL(&lock->tickets, &my_ticket, next);
 +        qemu_co_mutex_unlock(&lock->mutex);
 +        qemu_coroutine_yield();
 +        assert(lock->owners >= 1);
 +
 +        /* Possibly wake another reader, which will wake the next in line.  */
          qemu_co_mutex_lock(&lock->mutex);
 -        lock->reader--;
 -        assert(lock->reader >= 0);
 -        /* Wakeup only one waiting writer */
 -        if (!lock->reader) {
 -            qemu_co_queue_next(&lock->queue);
 -        }
 +        qemu_co_rwlock_maybe_wake_one(lock);
      }
+-    qemu_co_mutex_unlock(&lock->mutex);
-     qvirtqueue_kick(vs->dev, vq, free_head);
++
--    qvirtio_wait_queue_isr(vs->dev, vq, QVIRTIO_SCSI_TIMEOUT_US);
++    self->locks_held++;
-+    qvirtio_wait_used_elem(vs->dev, vq, free_head, QVIRTIO_SCSI_TIMEOUT_US);
++}
++
-     response = readb(resp_addr +
++void qemu_co_rwlock_unlock(CoRwlock *lock)
-                      offsetof(struct virtio_scsi_cmd_resp, response));
++{
 +    Coroutine *self = qemu_coroutine_self();
 +
 +    assert(qemu_in_coroutine());
 +    self->locks_held--;
 +
 +    qemu_co_mutex_lock(&lock->mutex);
 +    if (lock->owners > 0) {
 +        lock->owners--;
 +    } else {
 +        assert(lock->owners == -1);
 +        lock->owners = 0;
 +    }
 +
 +    qemu_co_rwlock_maybe_wake_one(lock);
  }
  void qemu_co_rwlock_downgrade(CoRwlock *lock)
  {
 -    Coroutine *self = qemu_coroutine_self();
 +    qemu_co_mutex_lock(&lock->mutex);
 +    assert(lock->owners == -1);
 +    lock->owners = 1;
 -    /* lock->mutex critical section started in qemu_co_rwlock_wrlock or
 -     * qemu_co_rwlock_upgrade.
 -     */
 -    assert(lock->reader == 0);
 -    lock->reader++;
 -    qemu_co_mutex_unlock(&lock->mutex);
 -
 -    /* The rest of the read-side critical section is run without the mutex.  */
 -    self->locks_held++;
 +    /* Possibly wake another reader, which will wake the next in line.  */
 +    qemu_co_rwlock_maybe_wake_one(lock);
  }
  void qemu_co_rwlock_wrlock(CoRwlock *lock)
  {
 +    Coroutine *self = qemu_coroutine_self();
 +
      qemu_co_mutex_lock(&lock->mutex);
 -    lock->pending_writer++;
 -    while (lock->reader) {
 -        qemu_co_queue_wait(&lock->queue, &lock->mutex);
 +    if (lock->owners == 0) {
 +        lock->owners = -1;
 +        qemu_co_mutex_unlock(&lock->mutex);
 +    } else {
 +        CoRwTicket my_ticket = { false, qemu_coroutine_self() };
 +
 +        QSIMPLEQ_INSERT_TAIL(&lock->tickets, &my_ticket, next);
 +        qemu_co_mutex_unlock(&lock->mutex);
 +        qemu_coroutine_yield();
 +        assert(lock->owners == -1);
      }
 -    lock->pending_writer--;
 -    /* The rest of the write-side critical section is run with
 -     * the mutex taken, so that lock->reader remains zero.
 -     * There is no need to update self->locks_held.
 -     */
 +    self->locks_held++;
  }
  void qemu_co_rwlock_upgrade(CoRwlock *lock)
  {
 -    Coroutine *self = qemu_coroutine_self();
 -
      qemu_co_mutex_lock(&lock->mutex);
 -    assert(lock->reader > 0);
 -    lock->reader--;
 -    lock->pending_writer++;
 -    while (lock->reader) {
 -        qemu_co_queue_wait(&lock->queue, &lock->mutex);
 +    assert(lock->owners > 0);
 +    /* For fairness, wait if a writer is in line.  */
 +    if (lock->owners == 1 && QSIMPLEQ_EMPTY(&lock->tickets)) {
 +        lock->owners = -1;
 +        qemu_co_mutex_unlock(&lock->mutex);
 +    } else {
 +        CoRwTicket my_ticket = { false, qemu_coroutine_self() };
 +
 +        lock->owners--;
 +        QSIMPLEQ_INSERT_TAIL(&lock->tickets, &my_ticket, next);
 +        qemu_co_rwlock_maybe_wake_one(lock);
 +        qemu_coroutine_yield();
 +        assert(lock->owners == -1);
      }
 -    lock->pending_writer--;
 -
 -    /* The rest of the write-side critical section is run with
 -     * the mutex taken, similar to qemu_co_rwlock_wrlock.  Do
 -     * not account for the lock twice in self->locks_held.
 -     */
 -    self->locks_held--;
  }
 --
-.9.4
+.30.2

-[Qemu-devel] [PULL 2/7] libqos: fix typo in virtio.h QVirtQueue->used comment
+[PULL for-6.0 5/6] test-coroutine: Add rwlock upgrade test
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
+From: Paolo Bonzini <pbonzini@redhat.com>
-Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
-Reviewed-by: Fam Zheng <famz@redhat.com>
+Test that rwlock upgrade is fair, and that readers go back to sleep if
-Tested-by: Eric Blake <eblake@redhat.com>
+a writer is in line.
-Tested-by: Kevin Wolf <kwolf@redhat.com>
-Message-id: 20170628184724.21378-2-stefanha@redhat.com
+Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
 Message-id: 20210325112941.365238-6-pbonzini@redhat.com
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- tests/libqos/virtio.h | 2 +-
+ tests/unit/test-coroutine.c | 62 +++++++++++++++++++++++++++++++++++++
-file changed, 1 insertion(+), 1 deletion(-)
+file changed, 62 insertions(+)
-diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
+diff --git a/tests/unit/test-coroutine.c b/tests/unit/test-coroutine.c
 index XXXXXXX..XXXXXXX 100644
---- a/tests/libqos/virtio.h
+--- a/tests/unit/test-coroutine.c
-+++ b/tests/libqos/virtio.h
++++ b/tests/unit/test-coroutine.c
-@@ -XXX,XX +XXX,XX @@ typedef struct QVirtioDevice {
+@@ -XXX,XX +XXX,XX @@ static void test_co_mutex_lockable(void)
- typedef struct QVirtQueue {
+     g_assert(QEMU_MAKE_LOCKABLE(null_pointer) == NULL);
-     uint64_t desc; /* This points to an array of struct vring_desc */
+ }
-     uint64_t avail; /* This points to a struct vring_avail */
--    uint64_t used; /* This points to a struct vring_desc */
++static CoRwlock rwlock;
-+    uint64_t used; /* This points to a struct vring_used */
++
-     uint16_t index;
++/* Test that readers are properly sent back to the queue when upgrading,
-     uint32_t size;
++ * even if they are the sole readers.  The test scenario is as follows:
-     uint32_t free_head;
++ *
 + *
 + * | c1           | c2         |
 + * |--------------+------------+
 + * | rdlock       |            |
 + * | yield        |            |
 + * |              | wrlock     |
 + * |              | <queued>   |
 + * | upgrade      |            |
 + * | <queued>     | <dequeued> |
 + * |              | unlock     |
 + * | <dequeued>   |            |
 + * | unlock       |            |
 + */
 +
 +static void coroutine_fn rwlock_yield_upgrade(void *opaque)
 +{
 +    qemu_co_rwlock_rdlock(&rwlock);
 +    qemu_coroutine_yield();
 +
 +    qemu_co_rwlock_upgrade(&rwlock);
 +    qemu_co_rwlock_unlock(&rwlock);
 +
 +    *(bool *)opaque = true;
 +}
 +
 +static void coroutine_fn rwlock_wrlock_yield(void *opaque)
 +{
 +    qemu_co_rwlock_wrlock(&rwlock);
 +    qemu_coroutine_yield();
 +
 +    qemu_co_rwlock_unlock(&rwlock);
 +    *(bool *)opaque = true;
 +}
 +
 +static void test_co_rwlock_upgrade(void)
 +{
 +    bool c1_done = false;
 +    bool c2_done = false;
 +    Coroutine *c1, *c2;
 +
 +    qemu_co_rwlock_init(&rwlock);
 +    c1 = qemu_coroutine_create(rwlock_yield_upgrade, &c1_done);
 +    c2 = qemu_coroutine_create(rwlock_wrlock_yield, &c2_done);
 +
 +    qemu_coroutine_enter(c1);
 +    qemu_coroutine_enter(c2);
 +
 +    /* c1 now should go to sleep.  */
 +    qemu_coroutine_enter(c1);
 +    g_assert(!c1_done);
 +
 +    qemu_coroutine_enter(c2);
 +    g_assert(c1_done);
 +    g_assert(c2_done);
 +}
 +
  /*
   * Check that creation, enter, and return work
   */
@@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv)
      g_test_add_func("/basic/order", test_order);
      g_test_add_func("/locking/co-mutex", test_co_mutex);
      g_test_add_func("/locking/co-mutex/lockable", test_co_mutex_lockable);
 +    g_test_add_func("/locking/co-rwlock/upgrade", test_co_rwlock_upgrade);
      if (g_test_perf()) {
          g_test_add_func("/perf/lifecycle", perf_lifecycle);
          g_test_add_func("/perf/nesting", perf_nesting);
 --
-.9.4
+.30.2

-[Qemu-devel] [PULL 3/7] libqos: add virtio used ring support
+[PULL for-6.0 6/6] test-coroutine: Add rwlock downgrade test
-Existing tests do not touch the virtqueue used ring.  Instead they poll
+From: David Edmondson <david.edmondson@oracle.com>
 the virtqueue ISR register and peek into their request's device-specific
 status field.
-It turns out that the virtqueue ISR register can be set to 1 more than
+Test that downgrading an rwlock does not result in a failure to
-once for a single notification (see commit
+schedule coroutines queued on the rwlock.
 d768b5640946b7da55ce8335509df297e2c7cd "virtio: set ISR on dataplane
 notifications").  This causes problems for tests that assume a 1:1
 correspondence between the ISR being 1 and request completion.
-Peeking at device-specific status fields is also problematic if the
+The diagram associated with test_co_rwlock_downgrade() describes the
-device has no field that can be abused for EINPROGRESS polling
+intended behaviour, but what was observed previously corresponds to:
 semantics.  This is the case if all the field's values may be set by the
 device; there's no magic constant left for polling.
-It's time to process the used ring for completed requests, just like a
+| c1     | c2         | c3         | c4       |
-real virtio guest driver.  This patch adds the necessary APIs.
+|--------+------------+------------+----------|
 | rdlock |            |            |          |
 | yield  |            |            |          |
 |        | wrlock     |            |          |
 |        | <queued>   |            |          |
 |        |            | rdlock     |          |
 |        |            | <queued>   |          |
 |        |            |            | wrlock   |
 |        |            |            | <queued> |
 | unlock |            |            |          |
 | yield  |            |            |          |
 |        | <dequeued> |            |          |
 |        | downgrade  |            |          |
 |        | ...        |            |          |
 |        | unlock     |            |          |
 |        |            | <dequeued> |          |
 |        |            | <queued>   |          |
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
+This results in a failure...
-Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
-Reviewed-by: Fam Zheng <famz@redhat.com>
+ERROR:../tests/test-coroutine.c:369:test_co_rwlock_downgrade: assertion failed: (c3_done)
-Tested-by: Eric Blake <eblake@redhat.com>
+Bail out! ERROR:../tests/test-coroutine.c:369:test_co_rwlock_downgrade: assertion failed: (c3_done)
-Tested-by: Kevin Wolf <kwolf@redhat.com>
-Message-id: 20170628184724.21378-3-stefanha@redhat.com
+...as a result of the c3 coroutine failing to run to completion.
 Signed-off-by: David Edmondson <david.edmondson@oracle.com>
 Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
 Message-id: 20210325112941.365238-7-pbonzini@redhat.com
 Message-Id: <20210309144015.557477-5-david.edmondson@oracle.com>
 Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- tests/libqos/virtio.h |  6 ++++++
+ tests/unit/test-coroutine.c | 99 +++++++++++++++++++++++++++++++++++++
- tests/libqos/virtio.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++
+file changed, 99 insertions(+)
 files changed, 66 insertions(+)
-diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
+diff --git a/tests/unit/test-coroutine.c b/tests/unit/test-coroutine.c
 index XXXXXXX..XXXXXXX 100644
---- a/tests/libqos/virtio.h
+--- a/tests/unit/test-coroutine.c
-+++ b/tests/libqos/virtio.h
++++ b/tests/unit/test-coroutine.c
-@@ -XXX,XX +XXX,XX @@ typedef struct QVirtQueue {
+@@ -XXX,XX +XXX,XX @@ static void test_co_rwlock_upgrade(void)
-     uint32_t free_head;
+     g_assert(c2_done);
      uint32_t num_free;
      uint32_t align;
 +    uint16_t last_used_idx;
      bool indirect;
      bool event;
  } QVirtQueue;
@@ -XXX,XX +XXX,XX @@ uint8_t qvirtio_wait_status_byte_no_isr(QVirtioDevice *d,
                                          QVirtQueue *vq,
                                          uint64_t addr,
                                          gint64 timeout_us);
 +void qvirtio_wait_used_elem(QVirtioDevice *d,
 +                            QVirtQueue *vq,
 +                            uint32_t desc_idx,
 +                            gint64 timeout_us);
  void qvirtio_wait_config_isr(QVirtioDevice *d, gint64 timeout_us);
  QVirtQueue *qvirtqueue_setup(QVirtioDevice *d,
                               QGuestAllocator *alloc, uint16_t index);
@@ -XXX,XX +XXX,XX @@ uint32_t qvirtqueue_add(QVirtQueue *vq, uint64_t data, uint32_t len, bool write,
                                                                      bool next);
  uint32_t qvirtqueue_add_indirect(QVirtQueue *vq, QVRingIndirectDesc *indirect);
  void qvirtqueue_kick(QVirtioDevice *d, QVirtQueue *vq, uint32_t free_head);
 +bool qvirtqueue_get_buf(QVirtQueue *vq, uint32_t *desc_idx);
  void qvirtqueue_set_used_event(QVirtQueue *vq, uint16_t idx);
  #endif
 diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
 index XXXXXXX..XXXXXXX 100644
 --- a/tests/libqos/virtio.c
 +++ b/tests/libqos/virtio.c
@@ -XXX,XX +XXX,XX @@ uint8_t qvirtio_wait_status_byte_no_isr(QVirtioDevice *d,
      return val;
  }
-+/*
++static void coroutine_fn rwlock_rdlock_yield(void *opaque)
 + * qvirtio_wait_used_elem:
 + * @desc_idx: The next expected vq->desc[] index in the used ring
 + * @timeout_us: How many microseconds to wait before failing
 + *
 + * This function waits for the next completed request on the used ring.
 + */
 +void qvirtio_wait_used_elem(QVirtioDevice *d,
 +                            QVirtQueue *vq,
 +                            uint32_t desc_idx,
 +                            gint64 timeout_us)
 +{
-+    gint64 start_time = g_get_monotonic_time();
++    qemu_co_rwlock_rdlock(&rwlock);
 +    qemu_coroutine_yield();
 +
-+    for (;;) {
++    qemu_co_rwlock_unlock(&rwlock);
-+        uint32_t got_desc_idx;
++    qemu_coroutine_yield();
 +
-+        clock_step(100);
++    *(bool *)opaque = true;
 +
 +        if (d->bus->get_queue_isr_status(d, vq) &&
 +            qvirtqueue_get_buf(vq, &got_desc_idx)) {
 +            g_assert_cmpint(got_desc_idx, ==, desc_idx);
 +            return;
 +        }
 +
 +        g_assert(g_get_monotonic_time() - start_time <= timeout_us);
 +    }
 +}
 +
- void qvirtio_wait_config_isr(QVirtioDevice *d, gint64 timeout_us)
++static void coroutine_fn rwlock_wrlock_downgrade(void *opaque)
  {
      gint64 start_time = g_get_monotonic_time();
@@ -XXX,XX +XXX,XX @@ void qvirtqueue_kick(QVirtioDevice *d, QVirtQueue *vq, uint32_t free_head)
      }
  }
 +/*
 + * qvirtqueue_get_buf:
 + * @desc_idx: A pointer that is filled with the vq->desc[] index, may be NULL
 + *
 + * This function gets the next used element if there is one ready.
 + *
 + * Returns: true if an element was ready, false otherwise
 + */
 +bool qvirtqueue_get_buf(QVirtQueue *vq, uint32_t *desc_idx)
 +{
-+    uint16_t idx;
++    qemu_co_rwlock_wrlock(&rwlock);
 +
-+    idx = readw(vq->used + offsetof(struct vring_used, idx));
++    qemu_co_rwlock_downgrade(&rwlock);
-+    if (idx == vq->last_used_idx) {
++    qemu_co_rwlock_unlock(&rwlock);
-+        return false;
++    *(bool *)opaque = true;
 +    }
 +
 +    if (desc_idx) {
 +        uint64_t elem_addr;
 +
 +        elem_addr = vq->used +
 +                    offsetof(struct vring_used, ring) +
 +                    (vq->last_used_idx % vq->size) *
 +                    sizeof(struct vring_used_elem);
 +        *desc_idx = readl(elem_addr + offsetof(struct vring_used_elem, id));
 +    }
 +
 +    vq->last_used_idx++;
 +    return true;
 +}
 +
- void qvirtqueue_set_used_event(QVirtQueue *vq, uint16_t idx)
++static void coroutine_fn rwlock_rdlock(void *opaque)
- {
++{
-     g_assert(vq->event);
++    qemu_co_rwlock_rdlock(&rwlock);
 +
 +    qemu_co_rwlock_unlock(&rwlock);
 +    *(bool *)opaque = true;
 +}
 +
 +static void coroutine_fn rwlock_wrlock(void *opaque)
 +{
 +    qemu_co_rwlock_wrlock(&rwlock);
 +
 +    qemu_co_rwlock_unlock(&rwlock);
 +    *(bool *)opaque = true;
 +}
 +
 +/*
 + * Check that downgrading a reader-writer lock does not cause a hang.
 + *
 + * Four coroutines are used to produce a situation where there are
 + * both reader and writer hopefuls waiting to acquire an rwlock that
 + * is held by a reader.
 + *
 + * The correct sequence of operations we aim to provoke can be
 + * represented as:
 + *
 + * | c1     | c2         | c3         | c4         |
 + * |--------+------------+------------+------------|
 + * | rdlock |            |            |            |
 + * | yield  |            |            |            |
 + * |        | wrlock     |            |            |
 + * |        | <queued>   |            |            |
 + * |        |            | rdlock     |            |
 + * |        |            | <queued>   |            |
 + * |        |            |            | wrlock     |
 + * |        |            |            | <queued>   |
 + * | unlock |            |            |            |
 + * | yield  |            |            |            |
 + * |        | <dequeued> |            |            |
 + * |        | downgrade  |            |            |
 + * |        |            | <dequeued> |            |
 + * |        |            | unlock     |            |
 + * |        | ...        |            |            |
 + * |        | unlock     |            |            |
 + * |        |            |            | <dequeued> |
 + * |        |            |            | unlock     |
 + */
 +static void test_co_rwlock_downgrade(void)
 +{
 +    bool c1_done = false;
 +    bool c2_done = false;
 +    bool c3_done = false;
 +    bool c4_done = false;
 +    Coroutine *c1, *c2, *c3, *c4;
 +
 +    qemu_co_rwlock_init(&rwlock);
 +
 +    c1 = qemu_coroutine_create(rwlock_rdlock_yield, &c1_done);
 +    c2 = qemu_coroutine_create(rwlock_wrlock_downgrade, &c2_done);
 +    c3 = qemu_coroutine_create(rwlock_rdlock, &c3_done);
 +    c4 = qemu_coroutine_create(rwlock_wrlock, &c4_done);
 +
 +    qemu_coroutine_enter(c1);
 +    qemu_coroutine_enter(c2);
 +    qemu_coroutine_enter(c3);
 +    qemu_coroutine_enter(c4);
 +
 +    qemu_coroutine_enter(c1);
 +
 +    g_assert(c2_done);
 +    g_assert(c3_done);
 +    g_assert(c4_done);
 +
 +    qemu_coroutine_enter(c1);
 +
 +    g_assert(c1_done);
 +}
 +
  /*
   * Check that creation, enter, and return work
   */
@@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv)
      g_test_add_func("/locking/co-mutex", test_co_mutex);
      g_test_add_func("/locking/co-mutex/lockable", test_co_mutex_lockable);
      g_test_add_func("/locking/co-rwlock/upgrade", test_co_rwlock_upgrade);
 +    g_test_add_func("/locking/co-rwlock/downgrade", test_co_rwlock_downgrade);
      if (g_test_perf()) {
          g_test_add_func("/perf/lifecycle", perf_lifecycle);
          g_test_add_func("/perf/nesting", perf_nesting);
 --
-.9.4
+.30.2

The following changes since commit 464588675455afda2899e20a0b120e4075de50c7:

Merge remote-tracking branch 'remotes/sstabellini/tags/xen-20170627-tag' into staging (2017-06-29 11:45:01 +0100)

are available in the git repository at:

git://github.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to c324fd0a39cee43c13f6d1fb34f74fc5e2fb007b:

virtio-pci: use ioeventfd even when KVM is disabled (2017-06-30 11:03:45 +0100)

----------------------------------------------------------------

Stefan Hajnoczi (7):
  virtio-blk: trace vdev so devices can be distinguished
  libqos: fix typo in virtio.h QVirtQueue->used comment
  libqos: add virtio used ring support
  tests: fix virtio-scsi-test ISR dependence
  tests: fix virtio-blk-test ISR dependence
  tests: fix virtio-net-test ISR dependence
  virtio-pci: use ioeventfd even when KVM is disabled

-- 
2.9.4

It is hard to analyze trace logs with multiple virtio-blk devices
because none of the trace events include the VirtIODevice *vdev.

This patch adds vdev so it's clear which device a request is associated
with.

I considered using VirtIOBlock *s instead but VirtIODevice *vdev is more
general and may be correlated with generic virtio trace events like
virtio_set_status.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Message-id: 20170614092930.11234-1-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 12 +++++++-----
 hw/block/trace-events | 10 +++++-----
 2 files changed, 12 insertions(+), 10 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 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
     VirtIOBlock *s = req->dev;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
-    trace_virtio_blk_req_complete(req, status);
+    trace_virtio_blk_req_complete(vdev, req, status);
 
     stb_p(&req->in->status, status);
     virtqueue_push(req->vq, &req->elem, req->in_len);
@@ -XXX,XX +XXX,XX @@ static void virtio_blk_rw_complete(void *opaque, int ret)
 {
     VirtIOBlockReq *next = opaque;
     VirtIOBlock *s = next->dev;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
     aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
     while (next) {
         VirtIOBlockReq *req = next;
         next = req->mr_next;
-        trace_virtio_blk_rw_complete(req, ret);
+        trace_virtio_blk_rw_complete(vdev, req, ret);
 
         if (req->qiov.nalloc != -1) {
             /* If nalloc is != 1 req->qiov is a local copy of the original
@@ -XXX,XX +XXX,XX @@ static inline void submit_requests(BlockBackend *blk, MultiReqBuffer *mrb,
             mrb->reqs[i - 1]->mr_next = mrb->reqs[i];
         }
 
-        trace_virtio_blk_submit_multireq(mrb, start, num_reqs,
+        trace_virtio_blk_submit_multireq(VIRTIO_DEVICE(mrb->reqs[start]->dev),
+                                         mrb, start, num_reqs,
                                          sector_num << BDRV_SECTOR_BITS,
                                          qiov->size, is_write);
         block_acct_merge_done(blk_get_stats(blk),
@@ -XXX,XX +XXX,XX @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 
         if (is_write) {
             qemu_iovec_init_external(&req->qiov, iov, out_num);
-            trace_virtio_blk_handle_write(req, req->sector_num,
+            trace_virtio_blk_handle_write(vdev, req, req->sector_num,
                                           req->qiov.size / BDRV_SECTOR_SIZE);
         } else {
             qemu_iovec_init_external(&req->qiov, in_iov, in_num);
-            trace_virtio_blk_handle_read(req, req->sector_num,
+            trace_virtio_blk_handle_read(vdev, req, req->sector_num,
                                          req->qiov.size / BDRV_SECTOR_SIZE);
         }
 
diff --git a/hw/block/trace-events b/hw/block/trace-events
index XXXXXXX..XXXXXXX 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -XXX,XX +XXX,XX @@
 # See docs/tracing.txt for syntax documentation.
 
 # hw/block/virtio-blk.c
-virtio_blk_req_complete(void *req, int status) "req %p status %d"
-virtio_blk_rw_complete(void *req, int ret) "req %p ret %d"
-virtio_blk_handle_write(void *req, uint64_t sector, size_t nsectors) "req %p sector %"PRIu64" nsectors %zu"
-virtio_blk_handle_read(void *req, uint64_t sector, size_t nsectors) "req %p sector %"PRIu64" nsectors %zu"
-virtio_blk_submit_multireq(void *mrb, int start, int num_reqs, uint64_t offset, size_t size, bool is_write) "mrb %p start %d num_reqs %d offset %"PRIu64" size %zu is_write %d"
+virtio_blk_req_complete(void *vdev, void *req, int status) "vdev %p req %p status %d"
+virtio_blk_rw_complete(void *vdev, void *req, int ret) "vdev %p req %p ret %d"
+virtio_blk_handle_write(void *vdev, void *req, uint64_t sector, size_t nsectors) "vdev %p req %p sector %"PRIu64" nsectors %zu"
+virtio_blk_handle_read(void *vdev, void *req, uint64_t sector, size_t nsectors) "vdev %p req %p sector %"PRIu64" nsectors %zu"
+virtio_blk_submit_multireq(void *vdev, void *mrb, int start, int num_reqs, uint64_t offset, size_t size, bool is_write) "vdev %p mrb %p start %d num_reqs %d offset %"PRIu64" size %zu is_write %d"
 
 # hw/block/hd-geometry.c
 hd_geometry_lchs_guess(void *blk, int cyls, int heads, int secs) "blk %p LCHS %d %d %d"
-- 
2.9.4

Existing tests do not touch the virtqueue used ring.  Instead they poll
the virtqueue ISR register and peek into their request's device-specific
status field.

It turns out that the virtqueue ISR register can be set to 1 more than
once for a single notification (see commit
83d768b5640946b7da55ce8335509df297e2c7cd "virtio: set ISR on dataplane
notifications").  This causes problems for tests that assume a 1:1
correspondence between the ISR being 1 and request completion.

Peeking at device-specific status fields is also problematic if the
device has no field that can be abused for EINPROGRESS polling
semantics.  This is the case if all the field's values may be set by the
device; there's no magic constant left for polling.

It's time to process the used ring for completed requests, just like a
real virtio guest driver.  This patch adds the necessary APIs.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Tested-by: Eric Blake <eblake@redhat.com>
Tested-by: Kevin Wolf <kwolf@redhat.com>
Message-id: 20170628184724.21378-3-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqos/virtio.h |  6 ++++++
 tests/libqos/virtio.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
index XXXXXXX..XXXXXXX 100644
--- a/tests/libqos/virtio.h
+++ b/tests/libqos/virtio.h
@@ -XXX,XX +XXX,XX @@ typedef struct QVirtQueue {
     uint32_t free_head;
     uint32_t num_free;
     uint32_t align;
+    uint16_t last_used_idx;
     bool indirect;
     bool event;
 } QVirtQueue;
@@ -XXX,XX +XXX,XX @@ uint8_t qvirtio_wait_status_byte_no_isr(QVirtioDevice *d,
                                         QVirtQueue *vq,
                                         uint64_t addr,
                                         gint64 timeout_us);
+void qvirtio_wait_used_elem(QVirtioDevice *d,
+                            QVirtQueue *vq,
+                            uint32_t desc_idx,
+                            gint64 timeout_us);
 void qvirtio_wait_config_isr(QVirtioDevice *d, gint64 timeout_us);
 QVirtQueue *qvirtqueue_setup(QVirtioDevice *d,
                              QGuestAllocator *alloc, uint16_t index);
@@ -XXX,XX +XXX,XX @@ uint32_t qvirtqueue_add(QVirtQueue *vq, uint64_t data, uint32_t len, bool write,
                                                                     bool next);
 uint32_t qvirtqueue_add_indirect(QVirtQueue *vq, QVRingIndirectDesc *indirect);
 void qvirtqueue_kick(QVirtioDevice *d, QVirtQueue *vq, uint32_t free_head);
+bool qvirtqueue_get_buf(QVirtQueue *vq, uint32_t *desc_idx);
 
 void qvirtqueue_set_used_event(QVirtQueue *vq, uint16_t idx);
 #endif
diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/libqos/virtio.c
+++ b/tests/libqos/virtio.c
@@ -XXX,XX +XXX,XX @@ uint8_t qvirtio_wait_status_byte_no_isr(QVirtioDevice *d,
     return val;
 }
 
+/*
+ * qvirtio_wait_used_elem:
+ * @desc_idx: The next expected vq->desc[] index in the used ring
+ * @timeout_us: How many microseconds to wait before failing
+ *
+ * This function waits for the next completed request on the used ring.
+ */
+void qvirtio_wait_used_elem(QVirtioDevice *d,
+                            QVirtQueue *vq,
+                            uint32_t desc_idx,
+                            gint64 timeout_us)
+{
+    gint64 start_time = g_get_monotonic_time();
+
+    for (;;) {
+        uint32_t got_desc_idx;
+
+        clock_step(100);
+
+        if (d->bus->get_queue_isr_status(d, vq) &&
+            qvirtqueue_get_buf(vq, &got_desc_idx)) {
+            g_assert_cmpint(got_desc_idx, ==, desc_idx);
+            return;
+        }
+
+        g_assert(g_get_monotonic_time() - start_time <= timeout_us);
+    }
+}
+
 void qvirtio_wait_config_isr(QVirtioDevice *d, gint64 timeout_us)
 {
     gint64 start_time = g_get_monotonic_time();
@@ -XXX,XX +XXX,XX @@ void qvirtqueue_kick(QVirtioDevice *d, QVirtQueue *vq, uint32_t free_head)
     }
 }
 
+/*
+ * qvirtqueue_get_buf:
+ * @desc_idx: A pointer that is filled with the vq->desc[] index, may be NULL
+ *
+ * This function gets the next used element if there is one ready.
+ *
+ * Returns: true if an element was ready, false otherwise
+ */
+bool qvirtqueue_get_buf(QVirtQueue *vq, uint32_t *desc_idx)
+{
+    uint16_t idx;
+
+    idx = readw(vq->used + offsetof(struct vring_used, idx));
+    if (idx == vq->last_used_idx) {
+        return false;
+    }
+
+    if (desc_idx) {
+        uint64_t elem_addr;
+
+        elem_addr = vq->used +
+                    offsetof(struct vring_used, ring) +
+                    (vq->last_used_idx % vq->size) *
+                    sizeof(struct vring_used_elem);
+        *desc_idx = readl(elem_addr + offsetof(struct vring_used_elem, id));
+    }
+
+    vq->last_used_idx++;
+    return true;
+}
+
 void qvirtqueue_set_used_event(QVirtQueue *vq, uint16_t idx)
 {
     g_assert(vq->event);
-- 
2.9.4

Use the new used ring APIs instead of assuming ISR being set means the
request has completed.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Tested-by: Eric Blake <eblake@redhat.com>
Tested-by: Kevin Wolf <kwolf@redhat.com>
Message-id: 20170628184724.21378-4-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/virtio-scsi-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -XXX,XX +XXX,XX @@ static uint8_t virtio_scsi_do_command(QVirtIOSCSI *vs, const uint8_t *cdb,
     }
 
     qvirtqueue_kick(vs->dev, vq, free_head);
-    qvirtio_wait_queue_isr(vs->dev, vq, QVIRTIO_SCSI_TIMEOUT_US);
+    qvirtio_wait_used_elem(vs->dev, vq, free_head, QVIRTIO_SCSI_TIMEOUT_US);
 
     response = readb(resp_addr +
                      offsetof(struct virtio_scsi_cmd_resp, response));
-- 
2.9.4

Use the new used ring APIs instead of assuming ISR being set means the
request has completed.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Tested-by: Eric Blake <eblake@redhat.com>
Tested-by: Kevin Wolf <kwolf@redhat.com>
Message-id: 20170628184724.21378-5-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/virtio-blk-test.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -XXX,XX +XXX,XX @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
 
     qvirtqueue_kick(dev, vq, free_head);
 
-    qvirtio_wait_queue_isr(dev, vq, QVIRTIO_BLK_TIMEOUT_US);
+    qvirtio_wait_used_elem(dev, vq, free_head, QVIRTIO_BLK_TIMEOUT_US);
     status = readb(req_addr + 528);
     g_assert_cmpint(status, ==, 0);
 
@@ -XXX,XX +XXX,XX @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
 
     qvirtqueue_kick(dev, vq, free_head);
 
-    qvirtio_wait_queue_isr(dev, vq, QVIRTIO_BLK_TIMEOUT_US);
+    qvirtio_wait_used_elem(dev, vq, free_head, QVIRTIO_BLK_TIMEOUT_US);
     status = readb(req_addr + 528);
     g_assert_cmpint(status, ==, 0);
 
@@ -XXX,XX +XXX,XX @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
         qvirtqueue_add(vq, req_addr + 528, 1, true, false);
         qvirtqueue_kick(dev, vq, free_head);
 
-        qvirtio_wait_queue_isr(dev, vq, QVIRTIO_BLK_TIMEOUT_US);
+        qvirtio_wait_used_elem(dev, vq, free_head, QVIRTIO_BLK_TIMEOUT_US);
         status = readb(req_addr + 528);
         g_assert_cmpint(status, ==, 0);
 
@@ -XXX,XX +XXX,XX @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
 
         qvirtqueue_kick(dev, vq, free_head);
 
-        qvirtio_wait_queue_isr(dev, vq, QVIRTIO_BLK_TIMEOUT_US);
+        qvirtio_wait_used_elem(dev, vq, free_head, QVIRTIO_BLK_TIMEOUT_US);
         status = readb(req_addr + 528);
         g_assert_cmpint(status, ==, 0);
 
@@ -XXX,XX +XXX,XX @@ static void pci_indirect(void)
     free_head = qvirtqueue_add_indirect(&vqpci->vq, indirect);
     qvirtqueue_kick(&dev->vdev, &vqpci->vq, free_head);
 
-    qvirtio_wait_queue_isr(&dev->vdev, &vqpci->vq,
+    qvirtio_wait_used_elem(&dev->vdev, &vqpci->vq, free_head,
                            QVIRTIO_BLK_TIMEOUT_US);
     status = readb(req_addr + 528);
     g_assert_cmpint(status, ==, 0);
@@ -XXX,XX +XXX,XX @@ static void pci_indirect(void)
     free_head = qvirtqueue_add_indirect(&vqpci->vq, indirect);
     qvirtqueue_kick(&dev->vdev, &vqpci->vq, free_head);
 
-    qvirtio_wait_queue_isr(&dev->vdev, &vqpci->vq,
+    qvirtio_wait_used_elem(&dev->vdev, &vqpci->vq, free_head,
                            QVIRTIO_BLK_TIMEOUT_US);
     status = readb(req_addr + 528);
     g_assert_cmpint(status, ==, 0);
@@ -XXX,XX +XXX,XX @@ static void pci_msix(void)
     qvirtqueue_add(&vqpci->vq, req_addr + 528, 1, true, false);
     qvirtqueue_kick(&dev->vdev, &vqpci->vq, free_head);
 
-    qvirtio_wait_queue_isr(&dev->vdev, &vqpci->vq,
+    qvirtio_wait_used_elem(&dev->vdev, &vqpci->vq, free_head,
                            QVIRTIO_BLK_TIMEOUT_US);
 
     status = readb(req_addr + 528);
@@ -XXX,XX +XXX,XX @@ static void pci_msix(void)
     qvirtqueue_kick(&dev->vdev, &vqpci->vq, free_head);
 
 
-    qvirtio_wait_queue_isr(&dev->vdev, &vqpci->vq,
+    qvirtio_wait_used_elem(&dev->vdev, &vqpci->vq, free_head,
                            QVIRTIO_BLK_TIMEOUT_US);
 
     status = readb(req_addr + 528);
@@ -XXX,XX +XXX,XX @@ static void pci_idx(void)
     uint64_t capacity;
     uint32_t features;
     uint32_t free_head;
+    uint32_t write_head;
+    uint32_t desc_idx;
     uint8_t status;
     char *data;
 
@@ -XXX,XX +XXX,XX @@ static void pci_idx(void)
     qvirtqueue_add(&vqpci->vq, req_addr + 528, 1, true, false);
     qvirtqueue_kick(&dev->vdev, &vqpci->vq, free_head);
 
-    qvirtio_wait_queue_isr(&dev->vdev, &vqpci->vq, QVIRTIO_BLK_TIMEOUT_US);
+    qvirtio_wait_used_elem(&dev->vdev, &vqpci->vq, free_head,
+                           QVIRTIO_BLK_TIMEOUT_US);
 
     /* Write request */
     req.type = VIRTIO_BLK_T_OUT;
@@ -XXX,XX +XXX,XX @@ static void pci_idx(void)
     qvirtqueue_add(&vqpci->vq, req_addr + 16, 512, false, true);
     qvirtqueue_add(&vqpci->vq, req_addr + 528, 1, true, false);
     qvirtqueue_kick(&dev->vdev, &vqpci->vq, free_head);
+    write_head = free_head;
 
     /* No notification expected */
     status = qvirtio_wait_status_byte_no_isr(&dev->vdev,
@@ -XXX,XX +XXX,XX @@ static void pci_idx(void)
 
     qvirtqueue_kick(&dev->vdev, &vqpci->vq, free_head);
 
-    qvirtio_wait_queue_isr(&dev->vdev, &vqpci->vq,
+    /* We get just one notification for both requests */
+    qvirtio_wait_used_elem(&dev->vdev, &vqpci->vq, write_head,
                            QVIRTIO_BLK_TIMEOUT_US);
+    g_assert(qvirtqueue_get_buf(&vqpci->vq, &desc_idx));
+    g_assert_cmpint(desc_idx, ==, free_head);
 
     status = readb(req_addr + 528);
     g_assert_cmpint(status, ==, 0);
-- 
2.9.4

Use the new used ring APIs instead of assuming ISR being set means the
request has completed.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Tested-by: Eric Blake <eblake@redhat.com>
Tested-by: Kevin Wolf <kwolf@redhat.com>
Message-id: 20170628184724.21378-6-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/virtio-net-test.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -XXX,XX +XXX,XX @@ static void rx_test(QVirtioDevice *dev,
     ret = iov_send(socket, iov, 2, 0, sizeof(len) + sizeof(test));
     g_assert_cmpint(ret, ==, sizeof(test) + sizeof(len));
 
-    qvirtio_wait_queue_isr(dev, vq, QVIRTIO_NET_TIMEOUT_US);
+    qvirtio_wait_used_elem(dev, vq, free_head, QVIRTIO_NET_TIMEOUT_US);
     memread(req_addr + VNET_HDR_SIZE, buffer, sizeof(test));
     g_assert_cmpstr(buffer, ==, "TEST");
 
@@ -XXX,XX +XXX,XX @@ static void tx_test(QVirtioDevice *dev,
     free_head = qvirtqueue_add(vq, req_addr, 64, false, false);
     qvirtqueue_kick(dev, vq, free_head);
 
-    qvirtio_wait_queue_isr(dev, vq, QVIRTIO_NET_TIMEOUT_US);
+    qvirtio_wait_used_elem(dev, vq, free_head, QVIRTIO_NET_TIMEOUT_US);
     guest_free(alloc, req_addr);
 
     ret = qemu_recv(socket, &len, sizeof(len), 0);
@@ -XXX,XX +XXX,XX @@ static void rx_stop_cont_test(QVirtioDevice *dev,
     rsp = qmp("{ 'execute' : 'cont'}");
     QDECREF(rsp);
 
-    qvirtio_wait_queue_isr(dev, vq, QVIRTIO_NET_TIMEOUT_US);
+    qvirtio_wait_used_elem(dev, vq, free_head, QVIRTIO_NET_TIMEOUT_US);
     memread(req_addr + VNET_HDR_SIZE, buffer, sizeof(test));
     g_assert_cmpstr(buffer, ==, "TEST");
 
-- 
2.9.4

Old kvm.ko versions only supported a tiny number of ioeventfds so
virtio-pci avoids ioeventfds when kvm_has_many_ioeventfds() returns 0.

Do not check kvm_has_many_ioeventfds() when KVM is disabled since it
always returns 0.  Since commit 8c56c1a592b5092d91da8d8943c17777d6462a6f
("memory: emulate ioeventfd") it has been possible to use ioeventfds in
qtest or TCG mode.

This patch makes -device virtio-blk-pci,iothread=iothread0 work even
when KVM is disabled.

I have tested that virtio-blk-pci works under TCG both with and without
iothread.

This patch fixes qemu-iotests 068, which was accidentally merged early
despite the dependency on ioeventfd.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Tested-by: Eric Blake <eblake@redhat.com>
Tested-by: Kevin Wolf <kwolf@redhat.com>
Message-id: 20170628184724.21378-7-stefanha@redhat.com
Message-id: 20170615163813.7255-2-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio-pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -XXX,XX +XXX,XX @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
     bool pcie_port = pci_bus_is_express(pci_dev->bus) &&
                      !pci_bus_is_root(pci_dev->bus);
 
-    if (!kvm_has_many_ioeventfds()) {
+    if (kvm_enabled() && !kvm_has_many_ioeventfds()) {
         proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
     }
 
-- 
2.9.4

The following changes since commit 6d40ce00c1166c317e298ad82ecf10e650c4f87d:

Update version for v6.0.0-rc1 release (2021-03-30 18:19:07 +0100)

are available in the Git repository at:

https://gitlab.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to b6489ac06695e257ea0a9841364577e247fdee30:

test-coroutine: Add rwlock downgrade test (2021-03-31 10:44:21 +0100)

----------------------------------------------------------------
Pull request

A fix for VDI image files and more generally for CoRwlock.

----------------------------------------------------------------

David Edmondson (4):
  block/vdi: When writing new bmap entry fails, don't leak the buffer
  block/vdi: Don't assume that blocks are larger than VdiHeader
  coroutine-lock: Store the coroutine in the CoWaitRecord only once
  test-coroutine: Add rwlock downgrade test

Paolo Bonzini (2):
  coroutine-lock: Reimplement CoRwlock to fix downgrade bug
  test-coroutine: Add rwlock upgrade test

include/qemu/coroutine.h    |  17 ++--
 block/vdi.c                 |  11 ++-
 tests/unit/test-coroutine.c | 161 +++++++++++++++++++++++++++++++++++
 util/qemu-coroutine-lock.c  | 165 +++++++++++++++++++++++-------------
 4 files changed, 282 insertions(+), 72 deletions(-)

-- 
2.30.2

From: David Edmondson <david.edmondson@oracle.com>

If a new bitmap entry is allocated, requiring the entire block to be
written, avoiding leaking the buffer allocated for the block should
the write fail.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: David Edmondson <david.edmondson@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Max Reitz <mreitz@redhat.com>
Message-id: 20210325112941.365238-2-pbonzini@redhat.com
Message-Id: <20210309144015.557477-2-david.edmondson@oracle.com>
Acked-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/vdi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/vdi.c b/block/vdi.c
index XXXXXXX..XXXXXXX 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -XXX,XX +XXX,XX @@ nonallocating_write:
 
     logout("finished data write\n");
     if (ret < 0) {
+        g_free(block);
         return ret;
     }
 
-- 
2.30.2

From: David Edmondson <david.edmondson@oracle.com>

Given that the block size is read from the header of the VDI file, a
wide variety of sizes might be seen. Rather than re-using a block
sized memory region when writing the VDI header, allocate an
appropriately sized buffer.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Max Reitz <mreitz@redhat.com>
Message-id: 20210325112941.365238-3-pbonzini@redhat.com
Message-Id: <20210309144015.557477-3-david.edmondson@oracle.com>
Acked-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/vdi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index XXXXXXX..XXXXXXX 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -XXX,XX +XXX,XX @@ nonallocating_write:
 
     if (block) {
         /* One or more new blocks were allocated. */
-        VdiHeader *header = (VdiHeader *) block;
+        VdiHeader *header;
         uint8_t *base;
         uint64_t offset;
         uint32_t n_sectors;
 
+        g_free(block);
+        header = g_malloc(sizeof(*header));
+
         logout("now writing modified header\n");
         assert(VDI_IS_ALLOCATED(bmap_first));
         *header = s->header;
         vdi_header_to_le(header);
-        ret = bdrv_pwrite(bs->file, 0, block, sizeof(VdiHeader));
-        g_free(block);
-        block = NULL;
+        ret = bdrv_pwrite(bs->file, 0, header, sizeof(*header));
+        g_free(header);
 
         if (ret < 0) {
             return ret;
-- 
2.30.2

From: David Edmondson <david.edmondson@oracle.com>

When taking the slow path for mutex acquisition, set the coroutine
value in the CoWaitRecord in push_waiter(), rather than both there and
in the caller.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: David Edmondson <david.edmondson@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 20210325112941.365238-4-pbonzini@redhat.com
Message-Id: <20210309144015.557477-4-david.edmondson@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/qemu-coroutine-lock.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index XXXXXXX..XXXXXXX 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn qemu_co_mutex_lock_slowpath(AioContext *ctx,
     unsigned old_handoff;
 
     trace_qemu_co_mutex_lock_entry(mutex, self);
-    w.co = self;
     push_waiter(mutex, &w);
 
     /* This is the "Responsibility Hand-Off" protocol; a lock() picks from
-- 
2.30.2

From: Paolo Bonzini <pbonzini@redhat.com>

An invariant of the current rwlock is that if multiple coroutines hold a
reader lock, all must be runnable. The unlock implementation relies on
this, choosing to wake a single coroutine when the final read lock
holder exits the critical section, assuming that it will wake a
coroutine attempting to acquire a write lock.

The downgrade implementation violates this assumption by creating a
read lock owning coroutine that is exclusively runnable - any other
coroutines that are waiting to acquire a read lock are *not* made
runnable when the write lock holder converts its ownership to read
only.

More in general, the old implementation had lots of other fairness bugs.
The root cause of the bugs was that CoQueue would wake up readers even
if there were pending writers, and would wake up writers even if there
were readers.  In that case, the coroutine would go back to sleep *at
the end* of the CoQueue, losing its place at the head of the line.

To fix this, keep the queue of waiters explicitly in the CoRwlock
instead of using CoQueue, and store for each whether it is a
potential reader or a writer.  This way, downgrade can look at the
first queued coroutines and wake it only if it is a reader, causing
all other readers in line to be released in turn.

Reported-by: David Edmondson <david.edmondson@oracle.com>
Reviewed-by: David Edmondson <david.edmondson@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 20210325112941.365238-5-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/coroutine.h   |  17 ++--
 util/qemu-coroutine-lock.c | 164 +++++++++++++++++++++++--------------
 2 files changed, 114 insertions(+), 67 deletions(-)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index XXXXXXX..XXXXXXX 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -XXX,XX +XXX,XX @@ bool qemu_co_enter_next_impl(CoQueue *queue, QemuLockable *lock);
 bool qemu_co_queue_empty(CoQueue *queue);
 
 
+typedef struct CoRwTicket CoRwTicket;
 typedef struct CoRwlock {
-    int pending_writer;
-    int reader;
     CoMutex mutex;
-    CoQueue queue;
+
+    /* Number of readers, or -1 if owned for writing.  */
+    int owners;
+
+    /* Waiting coroutines.  */
+    QSIMPLEQ_HEAD(, CoRwTicket) tickets;
 } CoRwlock;
 
 /**
@@ -XXX,XX +XXX,XX @@ void qemu_co_rwlock_rdlock(CoRwlock *lock);
 /**
  * Write Locks the CoRwlock from a reader.  This is a bit more efficient than
  * @qemu_co_rwlock_unlock followed by a separate @qemu_co_rwlock_wrlock.
- * However, if the lock cannot be upgraded immediately, control is transferred
- * to the caller of the current coroutine.  Also, @qemu_co_rwlock_upgrade
- * only overrides CoRwlock fairness if there are no concurrent readers, so
- * another writer might run while @qemu_co_rwlock_upgrade blocks.
+ * Note that if the lock cannot be upgraded immediately, control is transferred
+ * to the caller of the current coroutine; another writer might run while
+ * @qemu_co_rwlock_upgrade blocks.
  */
 void qemu_co_rwlock_upgrade(CoRwlock *lock);
 
diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index XXXXXXX..XXXXXXX 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex)
     trace_qemu_co_mutex_unlock_return(mutex, self);
 }
 
+struct CoRwTicket {
+    bool read;
+    Coroutine *co;
+    QSIMPLEQ_ENTRY(CoRwTicket) next;
+};
+
 void qemu_co_rwlock_init(CoRwlock *lock)
 {
-    memset(lock, 0, sizeof(*lock));
-    qemu_co_queue_init(&lock->queue);
     qemu_co_mutex_init(&lock->mutex);
+    lock->owners = 0;
+    QSIMPLEQ_INIT(&lock->tickets);
+}
+
+/* Releases the internal CoMutex.  */
+static void qemu_co_rwlock_maybe_wake_one(CoRwlock *lock)
+{
+    CoRwTicket *tkt = QSIMPLEQ_FIRST(&lock->tickets);
+    Coroutine *co = NULL;
+
+    /*
+     * Setting lock->owners here prevents rdlock and wrlock from
+     * sneaking in between unlock and wake.
+     */
+
+    if (tkt) {
+        if (tkt->read) {
+            if (lock->owners >= 0) {
+                lock->owners++;
+                co = tkt->co;
+            }
+        } else {
+            if (lock->owners == 0) {
+                lock->owners = -1;
+                co = tkt->co;
+            }
+        }
+    }
+
+    if (co) {
+        QSIMPLEQ_REMOVE_HEAD(&lock->tickets, next);
+        qemu_co_mutex_unlock(&lock->mutex);
+        aio_co_wake(co);
+    } else {
+        qemu_co_mutex_unlock(&lock->mutex);
+    }
 }
 
 void qemu_co_rwlock_rdlock(CoRwlock *lock)
@@ -XXX,XX +XXX,XX @@ void qemu_co_rwlock_rdlock(CoRwlock *lock)
 
     qemu_co_mutex_lock(&lock->mutex);
     /* For fairness, wait if a writer is in line.  */
-    while (lock->pending_writer) {
-        qemu_co_queue_wait(&lock->queue, &lock->mutex);
-    }
-    lock->reader++;
-    qemu_co_mutex_unlock(&lock->mutex);
-
-    /* The rest of the read-side critical section is run without the mutex.  */
-    self->locks_held++;
-}
-
-void qemu_co_rwlock_unlock(CoRwlock *lock)
-{
-    Coroutine *self = qemu_coroutine_self();
-
-    assert(qemu_in_coroutine());
-    if (!lock->reader) {
-        /* The critical section started in qemu_co_rwlock_wrlock.  */
-        qemu_co_queue_restart_all(&lock->queue);
+    if (lock->owners == 0 || (lock->owners > 0 && QSIMPLEQ_EMPTY(&lock->tickets))) {
+        lock->owners++;
+        qemu_co_mutex_unlock(&lock->mutex);
     } else {
-        self->locks_held--;
+        CoRwTicket my_ticket = { true, self };
 
+        QSIMPLEQ_INSERT_TAIL(&lock->tickets, &my_ticket, next);
+        qemu_co_mutex_unlock(&lock->mutex);
+        qemu_coroutine_yield();
+        assert(lock->owners >= 1);
+
+        /* Possibly wake another reader, which will wake the next in line.  */
         qemu_co_mutex_lock(&lock->mutex);
-        lock->reader--;
-        assert(lock->reader >= 0);
-        /* Wakeup only one waiting writer */
-        if (!lock->reader) {
-            qemu_co_queue_next(&lock->queue);
-        }
+        qemu_co_rwlock_maybe_wake_one(lock);
     }
-    qemu_co_mutex_unlock(&lock->mutex);
+
+    self->locks_held++;
+}
+
+void qemu_co_rwlock_unlock(CoRwlock *lock)
+{
+    Coroutine *self = qemu_coroutine_self();
+
+    assert(qemu_in_coroutine());
+    self->locks_held--;
+
+    qemu_co_mutex_lock(&lock->mutex);
+    if (lock->owners > 0) {
+        lock->owners--;
+    } else {
+        assert(lock->owners == -1);
+        lock->owners = 0;
+    }
+
+    qemu_co_rwlock_maybe_wake_one(lock);
 }
 
 void qemu_co_rwlock_downgrade(CoRwlock *lock)
 {
-    Coroutine *self = qemu_coroutine_self();
+    qemu_co_mutex_lock(&lock->mutex);
+    assert(lock->owners == -1);
+    lock->owners = 1;
 
-    /* lock->mutex critical section started in qemu_co_rwlock_wrlock or
-     * qemu_co_rwlock_upgrade.
-     */
-    assert(lock->reader == 0);
-    lock->reader++;
-    qemu_co_mutex_unlock(&lock->mutex);
-
-    /* The rest of the read-side critical section is run without the mutex.  */
-    self->locks_held++;
+    /* Possibly wake another reader, which will wake the next in line.  */
+    qemu_co_rwlock_maybe_wake_one(lock);
 }
 
 void qemu_co_rwlock_wrlock(CoRwlock *lock)
 {
+    Coroutine *self = qemu_coroutine_self();
+
     qemu_co_mutex_lock(&lock->mutex);
-    lock->pending_writer++;
-    while (lock->reader) {
-        qemu_co_queue_wait(&lock->queue, &lock->mutex);
+    if (lock->owners == 0) {
+        lock->owners = -1;
+        qemu_co_mutex_unlock(&lock->mutex);
+    } else {
+        CoRwTicket my_ticket = { false, qemu_coroutine_self() };
+
+        QSIMPLEQ_INSERT_TAIL(&lock->tickets, &my_ticket, next);
+        qemu_co_mutex_unlock(&lock->mutex);
+        qemu_coroutine_yield();
+        assert(lock->owners == -1);
     }
-    lock->pending_writer--;
 
-    /* The rest of the write-side critical section is run with
-     * the mutex taken, so that lock->reader remains zero.
-     * There is no need to update self->locks_held.
-     */
+    self->locks_held++;
 }
 
 void qemu_co_rwlock_upgrade(CoRwlock *lock)
 {
-    Coroutine *self = qemu_coroutine_self();
-
     qemu_co_mutex_lock(&lock->mutex);
-    assert(lock->reader > 0);
-    lock->reader--;
-    lock->pending_writer++;
-    while (lock->reader) {
-        qemu_co_queue_wait(&lock->queue, &lock->mutex);
+    assert(lock->owners > 0);
+    /* For fairness, wait if a writer is in line.  */
+    if (lock->owners == 1 && QSIMPLEQ_EMPTY(&lock->tickets)) {
+        lock->owners = -1;
+        qemu_co_mutex_unlock(&lock->mutex);
+    } else {
+        CoRwTicket my_ticket = { false, qemu_coroutine_self() };
+
+        lock->owners--;
+        QSIMPLEQ_INSERT_TAIL(&lock->tickets, &my_ticket, next);
+        qemu_co_rwlock_maybe_wake_one(lock);
+        qemu_coroutine_yield();
+        assert(lock->owners == -1);
     }
-    lock->pending_writer--;
-
-    /* The rest of the write-side critical section is run with
-     * the mutex taken, similar to qemu_co_rwlock_wrlock.  Do
-     * not account for the lock twice in self->locks_held.
-     */
-    self->locks_held--;
 }
-- 
2.30.2

From: Paolo Bonzini <pbonzini@redhat.com>

Test that rwlock upgrade is fair, and that readers go back to sleep if
a writer is in line.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 20210325112941.365238-6-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/unit/test-coroutine.c | 62 +++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/tests/unit/test-coroutine.c b/tests/unit/test-coroutine.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/unit/test-coroutine.c
+++ b/tests/unit/test-coroutine.c
@@ -XXX,XX +XXX,XX @@ static void test_co_mutex_lockable(void)
     g_assert(QEMU_MAKE_LOCKABLE(null_pointer) == NULL);
 }
 
+static CoRwlock rwlock;
+
+/* Test that readers are properly sent back to the queue when upgrading,
+ * even if they are the sole readers.  The test scenario is as follows:
+ *
+ *
+ * | c1           | c2         |
+ * |--------------+------------+
+ * | rdlock       |            |
+ * | yield        |            |
+ * |              | wrlock     |
+ * |              | <queued>   |
+ * | upgrade      |            |
+ * | <queued>     | <dequeued> |
+ * |              | unlock     |
+ * | <dequeued>   |            |
+ * | unlock       |            |
+ */
+
+static void coroutine_fn rwlock_yield_upgrade(void *opaque)
+{
+    qemu_co_rwlock_rdlock(&rwlock);
+    qemu_coroutine_yield();
+
+    qemu_co_rwlock_upgrade(&rwlock);
+    qemu_co_rwlock_unlock(&rwlock);
+
+    *(bool *)opaque = true;
+}
+
+static void coroutine_fn rwlock_wrlock_yield(void *opaque)
+{
+    qemu_co_rwlock_wrlock(&rwlock);
+    qemu_coroutine_yield();
+
+    qemu_co_rwlock_unlock(&rwlock);
+    *(bool *)opaque = true;
+}
+
+static void test_co_rwlock_upgrade(void)
+{
+    bool c1_done = false;
+    bool c2_done = false;
+    Coroutine *c1, *c2;
+
+    qemu_co_rwlock_init(&rwlock);
+    c1 = qemu_coroutine_create(rwlock_yield_upgrade, &c1_done);
+    c2 = qemu_coroutine_create(rwlock_wrlock_yield, &c2_done);
+
+    qemu_coroutine_enter(c1);
+    qemu_coroutine_enter(c2);
+
+    /* c1 now should go to sleep.  */
+    qemu_coroutine_enter(c1);
+    g_assert(!c1_done);
+
+    qemu_coroutine_enter(c2);
+    g_assert(c1_done);
+    g_assert(c2_done);
+}
+
 /*
  * Check that creation, enter, and return work
  */
@@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv)
     g_test_add_func("/basic/order", test_order);
     g_test_add_func("/locking/co-mutex", test_co_mutex);
     g_test_add_func("/locking/co-mutex/lockable", test_co_mutex_lockable);
+    g_test_add_func("/locking/co-rwlock/upgrade", test_co_rwlock_upgrade);
     if (g_test_perf()) {
         g_test_add_func("/perf/lifecycle", perf_lifecycle);
         g_test_add_func("/perf/nesting", perf_nesting);
-- 
2.30.2

From: David Edmondson <david.edmondson@oracle.com>

Test that downgrading an rwlock does not result in a failure to
schedule coroutines queued on the rwlock.

The diagram associated with test_co_rwlock_downgrade() describes the
intended behaviour, but what was observed previously corresponds to:

| c1     | c2         | c3         | c4       |
|--------+------------+------------+----------|
| rdlock |            |            |          |
| yield  |            |            |          |
|        | wrlock     |            |          |
|        | <queued>   |            |          |
|        |            | rdlock     |          |
|        |            | <queued>   |          |
|        |            |            | wrlock   |
|        |            |            | <queued> |
| unlock |            |            |          |
| yield  |            |            |          |
|        | <dequeued> |            |          |
|        | downgrade  |            |          |
|        | ...        |            |          |
|        | unlock     |            |          |
|        |            | <dequeued> |          |
|        |            | <queued>   |          |

This results in a failure...

ERROR:../tests/test-coroutine.c:369:test_co_rwlock_downgrade: assertion failed: (c3_done)
Bail out! ERROR:../tests/test-coroutine.c:369:test_co_rwlock_downgrade: assertion failed: (c3_done)

...as a result of the c3 coroutine failing to run to completion.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 20210325112941.365238-7-pbonzini@redhat.com
Message-Id: <20210309144015.557477-5-david.edmondson@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/unit/test-coroutine.c | 99 +++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/tests/unit/test-coroutine.c b/tests/unit/test-coroutine.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/unit/test-coroutine.c
+++ b/tests/unit/test-coroutine.c
@@ -XXX,XX +XXX,XX @@ static void test_co_rwlock_upgrade(void)
     g_assert(c2_done);
 }
 
+static void coroutine_fn rwlock_rdlock_yield(void *opaque)
+{
+    qemu_co_rwlock_rdlock(&rwlock);
+    qemu_coroutine_yield();
+
+    qemu_co_rwlock_unlock(&rwlock);
+    qemu_coroutine_yield();
+
+    *(bool *)opaque = true;
+}
+
+static void coroutine_fn rwlock_wrlock_downgrade(void *opaque)
+{
+    qemu_co_rwlock_wrlock(&rwlock);
+
+    qemu_co_rwlock_downgrade(&rwlock);
+    qemu_co_rwlock_unlock(&rwlock);
+    *(bool *)opaque = true;
+}
+
+static void coroutine_fn rwlock_rdlock(void *opaque)
+{
+    qemu_co_rwlock_rdlock(&rwlock);
+
+    qemu_co_rwlock_unlock(&rwlock);
+    *(bool *)opaque = true;
+}
+
+static void coroutine_fn rwlock_wrlock(void *opaque)
+{
+    qemu_co_rwlock_wrlock(&rwlock);
+
+    qemu_co_rwlock_unlock(&rwlock);
+    *(bool *)opaque = true;
+}
+
+/*
+ * Check that downgrading a reader-writer lock does not cause a hang.
+ *
+ * Four coroutines are used to produce a situation where there are
+ * both reader and writer hopefuls waiting to acquire an rwlock that
+ * is held by a reader.
+ *
+ * The correct sequence of operations we aim to provoke can be
+ * represented as:
+ *
+ * | c1     | c2         | c3         | c4         |
+ * |--------+------------+------------+------------|
+ * | rdlock |            |            |            |
+ * | yield  |            |            |            |
+ * |        | wrlock     |            |            |
+ * |        | <queued>   |            |            |
+ * |        |            | rdlock     |            |
+ * |        |            | <queued>   |            |
+ * |        |            |            | wrlock     |
+ * |        |            |            | <queued>   |
+ * | unlock |            |            |            |
+ * | yield  |            |            |            |
+ * |        | <dequeued> |            |            |
+ * |        | downgrade  |            |            |
+ * |        |            | <dequeued> |            |
+ * |        |            | unlock     |            |
+ * |        | ...        |            |            |
+ * |        | unlock     |            |            |
+ * |        |            |            | <dequeued> |
+ * |        |            |            | unlock     |
+ */
+static void test_co_rwlock_downgrade(void)
+{
+    bool c1_done = false;
+    bool c2_done = false;
+    bool c3_done = false;
+    bool c4_done = false;
+    Coroutine *c1, *c2, *c3, *c4;
+
+    qemu_co_rwlock_init(&rwlock);
+
+    c1 = qemu_coroutine_create(rwlock_rdlock_yield, &c1_done);
+    c2 = qemu_coroutine_create(rwlock_wrlock_downgrade, &c2_done);
+    c3 = qemu_coroutine_create(rwlock_rdlock, &c3_done);
+    c4 = qemu_coroutine_create(rwlock_wrlock, &c4_done);
+
+    qemu_coroutine_enter(c1);
+    qemu_coroutine_enter(c2);
+    qemu_coroutine_enter(c3);
+    qemu_coroutine_enter(c4);
+
+    qemu_coroutine_enter(c1);
+
+    g_assert(c2_done);
+    g_assert(c3_done);
+    g_assert(c4_done);
+
+    qemu_coroutine_enter(c1);
+
+    g_assert(c1_done);
+}
+
 /*
  * Check that creation, enter, and return work
  */
@@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv)
     g_test_add_func("/locking/co-mutex", test_co_mutex);
     g_test_add_func("/locking/co-mutex/lockable", test_co_mutex_lockable);
     g_test_add_func("/locking/co-rwlock/upgrade", test_co_rwlock_upgrade);
+    g_test_add_func("/locking/co-rwlock/downgrade", test_co_rwlock_downgrade);
     if (g_test_perf()) {
         g_test_add_func("/perf/lifecycle", perf_lifecycle);
         g_test_add_func("/perf/nesting", perf_nesting);
-- 
2.30.2