[Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Paolo Bonzini]

This series started as an attempt to always use the dataplane path
for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
was three-fold:

1) to add more coverage for dataplane

2) to remove virtio_add_queue_aio

3) to simplify the dataplane start/stop code

It achieves the first two objectives, and while it doesn't quite
achieve the third it does cleanup the generic ioeventfd code in
virtio-bus more than I expected.  In particular, it reduces the set
of callbacks that transports must implement, and it removes the ugly
case where ioeventfd is started with generic callbacks and then moved
to the dataplane callbacks.  It also enables some simplification of the
functions that deal with host notifiers, and detects some configuration
errors better.

I've tested it with virtio-blk, virtio-scsi and vhost-net.

Patch 1 is a bugfix that I found while testing the TCG+dataplane combo.

Patches 2 and 3 are simplifications that are too nice to leave
them for later in the series.

Patch 4 moves some of the ioeventfd code from virtio-bus.c to
virtio.c.  At this point the transition is a bit half-assed, but
this changes as soon as we remove the generic->dataplane
handler transition.

Patches 5 to 7 do exactly that, and then the spring cleaning
begins, lasting for the whole second half of the series.

Thanks,

Paolo

Paolo Bonzini (13):
  virtio: disable ioeventfd as early as possible
  virtio: move ioeventfd_disabled flag to VirtioBusState
  virtio: move ioeventfd_started flag to VirtioBusState
  virtio: add start_ioeventfd and stop_ioeventfd to VirtioDeviceClass
  virtio: introduce virtio_device_ioeventfd_enabled
  virtio-blk: always use dataplane path if ioeventfd is active
  virtio-scsi: always use dataplane path if ioeventfd is active
  Revert "virtio: Introduce virtio_add_queue_aio"
  virtio: remove set_handler argument from set_host_notifier_internal
  virtio: remove ioeventfd_disabled altogether
  virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd
  virtio: inline virtio_queue_set_host_notifier_fd_handler
  virtio: inline set_host_notifier_internal

 hw/block/dataplane/virtio-blk.c |  74 +++++++++++--------
 hw/block/dataplane/virtio-blk.h |   6 +-
 hw/block/virtio-blk.c           |  15 ++--
 hw/s390x/virtio-ccw.c           |  44 ++----------
 hw/s390x/virtio-ccw.h           |   2 -
 hw/scsi/virtio-scsi-dataplane.c |  56 +++++++++------
 hw/scsi/virtio-scsi.c           |  24 +++----
 hw/virtio/vhost.c               |   5 +-
 hw/virtio/virtio-bus.c          | 155 +++++++++++++++-------------------------
 hw/virtio/virtio-mmio.c         |  35 +--------
 hw/virtio/virtio-pci.c          |  40 ++---------
 hw/virtio/virtio-pci.h          |   2 -
 hw/virtio/virtio.c              | 139 ++++++++++++++++++++++-------------
 include/hw/virtio/virtio-bus.h  |  27 ++++---
 include/hw/virtio/virtio-scsi.h |   6 +-
 include/hw/virtio/virtio.h      |  11 +--
 16 files changed, 291 insertions(+), 350 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 01/13] virtio: disable ioeventfd as early as possible

[Paolo Bonzini]

Avoid "tricking" virtio-blk-dataplane into thinking that ioeventfd will be
available when it is not.  This bug has always been there, but it will break
TCG+ioeventfd=on once the dataplane code will be always used when ioeventfd=on.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/s390x/virtio-ccw.c  | 8 ++++----
 hw/virtio/virtio-pci.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index ee136ab..31304fe 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -709,6 +709,10 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
         sch->cssid, sch->ssid, sch->schid, sch->devno,
         ccw_dev->bus_id.valid ? "user-configured" : "auto-configured");
 
+    if (!kvm_eventfds_enabled()) {
+        dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
+    }
+
     if (k->realize) {
         k->realize(dev, &err);
     }
@@ -1311,10 +1315,6 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
         return;
     }
 
-    if (!kvm_eventfds_enabled()) {
-        dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
-    }
-
     sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus);
 
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 2d60a00..b93e2df 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1719,10 +1719,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
         pci_register_bar(&proxy->pci_dev, proxy->legacy_io_bar,
                          PCI_BASE_ADDRESS_SPACE_IO, &proxy->bar);
     }
-
-    if (!kvm_has_many_ioeventfds()) {
-        proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
-    }
 }
 
 static void virtio_pci_device_unplugged(DeviceState *d)
@@ -1751,6 +1747,10 @@ 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()) {
+        proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
+    }
+
     /*
      * virtio pci bar layout used by default.
      * subclasses can re-arrange things if needed.
-- 
2.7.4

Re: [Qemu-devel] [PATCH 01/13] virtio: disable ioeventfd as early as possible

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:29 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Avoid "tricking" virtio-blk-dataplane into thinking that ioeventfd will be
> available when it is not.  This bug has always been there, but it will break
> TCG+ioeventfd=on once the dataplane code will be always used when ioeventfd=on.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/s390x/virtio-ccw.c  | 8 ++++----
>  hw/virtio/virtio-pci.c | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)

I had simply copied the pci logic (as in other places :), but this
makes sense.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH 01/13] virtio: disable ioeventfd as early as possible

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

On Mon, Oct 10, 2016 at 01:53:29PM +0200, Paolo Bonzini wrote:
> Avoid "tricking" virtio-blk-dataplane into thinking that ioeventfd will be
> available when it is not.  This bug has always been there, but it will break
> TCG+ioeventfd=on once the dataplane code will be always used when ioeventfd=on.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/s390x/virtio-ccw.c  | 8 ++++----
>  hw/virtio/virtio-pci.c | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 02/13] virtio: move ioeventfd_disabled flag to VirtioBusState

[Paolo Bonzini]

This simplifies the code and removes the ioeventfd_set_disabled
callback.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/s390x/virtio-ccw.c          | 11 +----------
 hw/s390x/virtio-ccw.h          |  1 -
 hw/virtio/virtio-bus.c         |  4 ++--
 hw/virtio/virtio-mmio.c        | 13 +------------
 hw/virtio/virtio-pci.c         | 11 +----------
 hw/virtio/virtio-pci.h         |  1 -
 include/hw/virtio/virtio-bus.h |  8 ++++++--
 7 files changed, 11 insertions(+), 38 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 31304fe..672e6c4 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -82,15 +82,7 @@ static bool virtio_ccw_ioeventfd_disabled(DeviceState *d)
 {
     VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
 
-    return dev->ioeventfd_disabled ||
-        !(dev->flags & VIRTIO_CCW_FLAG_USE_IOEVENTFD);
-}
-
-static void virtio_ccw_ioeventfd_set_disabled(DeviceState *d, bool disabled)
-{
-    VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
-
-    dev->ioeventfd_disabled = disabled;
+    return !(dev->flags & VIRTIO_CCW_FLAG_USE_IOEVENTFD);
 }
 
 static int virtio_ccw_ioeventfd_assign(DeviceState *d, EventNotifier *notifier,
@@ -1619,7 +1611,6 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
     k->ioeventfd_started = virtio_ccw_ioeventfd_started;
     k->ioeventfd_set_started = virtio_ccw_ioeventfd_set_started;
     k->ioeventfd_disabled = virtio_ccw_ioeventfd_disabled;
-    k->ioeventfd_set_disabled = virtio_ccw_ioeventfd_set_disabled;
     k->ioeventfd_assign = virtio_ccw_ioeventfd_assign;
 }
 
diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index 565094e..327e75a 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -87,7 +87,6 @@ struct VirtioCcwDevice {
     uint32_t max_rev;
     VirtioBusState bus;
     bool ioeventfd_started;
-    bool ioeventfd_disabled;
     uint32_t flags;
     uint8_t thinint_isc;
     AdapterRoutes routes;
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 11f65bd..3607c29 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -195,7 +195,7 @@ void virtio_bus_start_ioeventfd(VirtioBusState *bus)
     if (!k->ioeventfd_started || k->ioeventfd_started(proxy)) {
         return;
     }
-    if (k->ioeventfd_disabled(proxy)) {
+    if (bus->ioeventfd_disabled || k->ioeventfd_disabled(proxy)) {
         return;
     }
     vdev = virtio_bus_get_device(bus);
@@ -257,7 +257,7 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
     if (!k->ioeventfd_started) {
         return -ENOSYS;
     }
-    k->ioeventfd_set_disabled(proxy, assign);
+    bus->ioeventfd_disabled = assign;
     if (assign) {
         /*
          * Stop using the generic ioeventfd, we are doing eventfd handling
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 13798b3..12a9e79 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -89,7 +89,6 @@ typedef struct {
     uint32_t guest_page_shift;
     /* virtio-bus */
     VirtioBusState bus;
-    bool ioeventfd_disabled;
     bool ioeventfd_started;
     bool format_transport_address;
 } VirtIOMMIOProxy;
@@ -111,16 +110,7 @@ static void virtio_mmio_ioeventfd_set_started(DeviceState *d, bool started,
 
 static bool virtio_mmio_ioeventfd_disabled(DeviceState *d)
 {
-    VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
-
-    return !kvm_eventfds_enabled() || proxy->ioeventfd_disabled;
-}
-
-static void virtio_mmio_ioeventfd_set_disabled(DeviceState *d, bool disabled)
-{
-    VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
-
-    proxy->ioeventfd_disabled = disabled;
+    return !kvm_eventfds_enabled();
 }
 
 static int virtio_mmio_ioeventfd_assign(DeviceState *d,
@@ -560,7 +550,6 @@ static void virtio_mmio_bus_class_init(ObjectClass *klass, void *data)
     k->ioeventfd_started = virtio_mmio_ioeventfd_started;
     k->ioeventfd_set_started = virtio_mmio_ioeventfd_set_started;
     k->ioeventfd_disabled = virtio_mmio_ioeventfd_disabled;
-    k->ioeventfd_set_disabled = virtio_mmio_ioeventfd_set_disabled;
     k->ioeventfd_assign = virtio_mmio_ioeventfd_assign;
     k->has_variable_vring_alignment = true;
     bus_class->max_dev = 1;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index b93e2df..580fe96 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -281,15 +281,7 @@ static bool virtio_pci_ioeventfd_disabled(DeviceState *d)
 {
     VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
 
-    return proxy->ioeventfd_disabled ||
-        !(proxy->flags & VIRTIO_PCI_FLAG_USE_IOEVENTFD);
-}
-
-static void virtio_pci_ioeventfd_set_disabled(DeviceState *d, bool disabled)
-{
-    VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
-
-    proxy->ioeventfd_disabled = disabled;
+    return !(proxy->flags & VIRTIO_PCI_FLAG_USE_IOEVENTFD);
 }
 
 #define QEMU_VIRTIO_PCI_QUEUE_MEM_MULT 0x1000
@@ -2542,7 +2534,6 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
     k->ioeventfd_started = virtio_pci_ioeventfd_started;
     k->ioeventfd_set_started = virtio_pci_ioeventfd_set_started;
     k->ioeventfd_disabled = virtio_pci_ioeventfd_disabled;
-    k->ioeventfd_set_disabled = virtio_pci_ioeventfd_set_disabled;
     k->ioeventfd_assign = virtio_pci_ioeventfd_assign;
 }
 
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 541cbdb..17ff195 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -158,7 +158,6 @@ struct VirtIOPCIProxy {
     uint32_t guest_features[2];
     VirtIOPCIQueue vqs[VIRTIO_QUEUE_MAX];
 
-    bool ioeventfd_disabled;
     bool ioeventfd_started;
     VirtIOIRQFD *vector_irqfd;
     int nvqs_with_notifiers;
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 2e4b67e..4aabec4 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -83,8 +83,6 @@ typedef struct VirtioBusClass {
     void (*ioeventfd_set_started)(DeviceState *d, bool started, bool err);
     /* Returns true if the ioeventfd has been disabled for the device. */
     bool (*ioeventfd_disabled)(DeviceState *d);
-    /* Sets the 'ioeventfd disabled' state for the device. */
-    void (*ioeventfd_set_disabled)(DeviceState *d, bool disabled);
     /*
      * Assigns/deassigns the ioeventfd backing for the transport on
      * the device for queue number n. Returns an error value on
@@ -102,6 +100,12 @@ typedef struct VirtioBusClass {
 
 struct VirtioBusState {
     BusState parent_obj;
+
+    /*
+     * Set if the default ioeventfd handlers are disabled by vhost
+     * or dataplane.
+     */
+    bool ioeventfd_disabled;
 };
 
 void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 02/13] virtio: move ioeventfd_disabled flag to VirtioBusState

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:30 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This simplifies the code and removes the ioeventfd_set_disabled
> callback.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/s390x/virtio-ccw.c          | 11 +----------
>  hw/s390x/virtio-ccw.h          |  1 -
>  hw/virtio/virtio-bus.c         |  4 ++--
>  hw/virtio/virtio-mmio.c        | 13 +------------
>  hw/virtio/virtio-pci.c         | 11 +----------
>  hw/virtio/virtio-pci.h         |  1 -
>  include/hw/virtio/virtio-bus.h |  8 ++++++--
>  7 files changed, 11 insertions(+), 38 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH 03/13] virtio: move ioeventfd_started flag to VirtioBusState

[Paolo Bonzini]

This simplifies the code and removes the ioeventfd_started
and ioeventfd_set_started callback.  The only difference is
in how virtio-ccw handles an error---it doesn't disable
ioeventfd forever anymore.  It was the only backend to do
so, and if desired this behavior should be implemented in
virtio-bus.c.

Instead of ioeventfd_started, the ioeventfd_assign callback now
determines whether the virtio bus supports host notifiers.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/dataplane/virtio-blk.c |  2 +-
 hw/s390x/virtio-ccw.c           | 21 ---------------------
 hw/s390x/virtio-ccw.h           |  1 -
 hw/scsi/virtio-scsi-dataplane.c |  2 +-
 hw/virtio/vhost.c               |  2 +-
 hw/virtio/virtio-bus.c          | 14 ++++++--------
 hw/virtio/virtio-mmio.c         | 18 ------------------
 hw/virtio/virtio-pci.c          | 17 -----------------
 hw/virtio/virtio-pci.h          |  1 -
 include/hw/virtio/virtio-bus.h  | 17 +++++++----------
 10 files changed, 16 insertions(+), 79 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 704a763..9b268f4 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -93,7 +93,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
     }
 
     /* Don't try if transport does not support notifiers. */
-    if (!k->set_guest_notifiers || !k->ioeventfd_started) {
+    if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
         error_setg(errp,
                    "device is incompatible with dataplane "
                    "(transport does not support notifiers)");
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 672e6c4..bf5670c 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -59,25 +59,6 @@ static void virtio_ccw_stop_ioeventfd(VirtioCcwDevice *dev)
     virtio_bus_stop_ioeventfd(&dev->bus);
 }
 
-static bool virtio_ccw_ioeventfd_started(DeviceState *d)
-{
-    VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
-
-    return dev->ioeventfd_started;
-}
-
-static void virtio_ccw_ioeventfd_set_started(DeviceState *d, bool started,
-                                             bool err)
-{
-    VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
-
-    dev->ioeventfd_started = started;
-    if (err) {
-        /* Disable ioeventfd for this device. */
-        dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
-    }
-}
-
 static bool virtio_ccw_ioeventfd_disabled(DeviceState *d)
 {
     VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
@@ -1608,8 +1589,6 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
     k->pre_plugged = virtio_ccw_pre_plugged;
     k->device_plugged = virtio_ccw_device_plugged;
     k->device_unplugged = virtio_ccw_device_unplugged;
-    k->ioeventfd_started = virtio_ccw_ioeventfd_started;
-    k->ioeventfd_set_started = virtio_ccw_ioeventfd_set_started;
     k->ioeventfd_disabled = virtio_ccw_ioeventfd_disabled;
     k->ioeventfd_assign = virtio_ccw_ioeventfd_assign;
 }
diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index 327e75a..77d10f1 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -86,7 +86,6 @@ struct VirtioCcwDevice {
     int revision;
     uint32_t max_rev;
     VirtioBusState bus;
-    bool ioeventfd_started;
     uint32_t flags;
     uint8_t thinint_isc;
     AdapterRoutes routes;
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index b173b94..f537b4e 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -31,7 +31,7 @@ void virtio_scsi_set_iothread(VirtIOSCSI *s, IOThread *iothread)
     s->ctx = iothread_get_aio_context(vs->conf.iothread);
 
     /* Don't try if transport does not support notifiers. */
-    if (!k->set_guest_notifiers || !k->ioeventfd_started) {
+    if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
         fprintf(stderr, "virtio-scsi: Failed to set iothread "
                    "(transport does not support notifiers)");
         exit(1);
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index bd051ab..501a5f4 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1190,7 +1190,7 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
     int i, r, e;
 
-    if (!k->ioeventfd_started) {
+    if (!k->ioeventfd_assign) {
         error_report("binding does not support host notifiers");
         r = -ENOSYS;
         goto fail;
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 3607c29..97cdb11 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -192,10 +192,10 @@ void virtio_bus_start_ioeventfd(VirtioBusState *bus)
     VirtIODevice *vdev;
     int n, r;
 
-    if (!k->ioeventfd_started || k->ioeventfd_started(proxy)) {
+    if (!k->ioeventfd_assign || k->ioeventfd_disabled(proxy)) {
         return;
     }
-    if (bus->ioeventfd_disabled || k->ioeventfd_disabled(proxy)) {
+    if (bus->ioeventfd_started || bus->ioeventfd_disabled) {
         return;
     }
     vdev = virtio_bus_get_device(bus);
@@ -208,7 +208,7 @@ void virtio_bus_start_ioeventfd(VirtioBusState *bus)
             goto assign_error;
         }
     }
-    k->ioeventfd_set_started(proxy, true, false);
+    bus->ioeventfd_started = true;
     return;
 
 assign_error:
@@ -220,18 +220,16 @@ assign_error:
         r = set_host_notifier_internal(proxy, bus, n, false, false);
         assert(r >= 0);
     }
-    k->ioeventfd_set_started(proxy, false, true);
     error_report("%s: failed. Fallback to userspace (slower).", __func__);
 }
 
 void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
 {
-    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
     DeviceState *proxy = DEVICE(BUS(bus)->parent);
     VirtIODevice *vdev;
     int n, r;
 
-    if (!k->ioeventfd_started || !k->ioeventfd_started(proxy)) {
+    if (!bus->ioeventfd_started) {
         return;
     }
     vdev = virtio_bus_get_device(bus);
@@ -242,7 +240,7 @@ void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
         r = set_host_notifier_internal(proxy, bus, n, false, false);
         assert(r >= 0);
     }
-    k->ioeventfd_set_started(proxy, false, false);
+    bus->ioeventfd_started = false;
 }
 
 /*
@@ -254,7 +252,7 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
     DeviceState *proxy = DEVICE(BUS(bus)->parent);
 
-    if (!k->ioeventfd_started) {
+    if (!k->ioeventfd_assign) {
         return -ENOSYS;
     }
     bus->ioeventfd_disabled = assign;
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 12a9e79..04a9655 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -89,25 +89,9 @@ typedef struct {
     uint32_t guest_page_shift;
     /* virtio-bus */
     VirtioBusState bus;
-    bool ioeventfd_started;
     bool format_transport_address;
 } VirtIOMMIOProxy;
 
-static bool virtio_mmio_ioeventfd_started(DeviceState *d)
-{
-    VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
-
-    return proxy->ioeventfd_started;
-}
-
-static void virtio_mmio_ioeventfd_set_started(DeviceState *d, bool started,
-                                              bool err)
-{
-    VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
-
-    proxy->ioeventfd_started = started;
-}
-
 static bool virtio_mmio_ioeventfd_disabled(DeviceState *d)
 {
     return !kvm_eventfds_enabled();
@@ -547,8 +531,6 @@ static void virtio_mmio_bus_class_init(ObjectClass *klass, void *data)
     k->save_config = virtio_mmio_save_config;
     k->load_config = virtio_mmio_load_config;
     k->set_guest_notifiers = virtio_mmio_set_guest_notifiers;
-    k->ioeventfd_started = virtio_mmio_ioeventfd_started;
-    k->ioeventfd_set_started = virtio_mmio_ioeventfd_set_started;
     k->ioeventfd_disabled = virtio_mmio_ioeventfd_disabled;
     k->ioeventfd_assign = virtio_mmio_ioeventfd_assign;
     k->has_variable_vring_alignment = true;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 580fe96..7752ed6 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -262,21 +262,6 @@ static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f)
     return 0;
 }
 
-static bool virtio_pci_ioeventfd_started(DeviceState *d)
-{
-    VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
-
-    return proxy->ioeventfd_started;
-}
-
-static void virtio_pci_ioeventfd_set_started(DeviceState *d, bool started,
-                                             bool err)
-{
-    VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
-
-    proxy->ioeventfd_started = started;
-}
-
 static bool virtio_pci_ioeventfd_disabled(DeviceState *d)
 {
     VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
@@ -2531,8 +2516,6 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
     k->device_plugged = virtio_pci_device_plugged;
     k->device_unplugged = virtio_pci_device_unplugged;
     k->query_nvectors = virtio_pci_query_nvectors;
-    k->ioeventfd_started = virtio_pci_ioeventfd_started;
-    k->ioeventfd_set_started = virtio_pci_ioeventfd_set_started;
     k->ioeventfd_disabled = virtio_pci_ioeventfd_disabled;
     k->ioeventfd_assign = virtio_pci_ioeventfd_assign;
 }
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 17ff195..7ef799a 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -158,7 +158,6 @@ struct VirtIOPCIProxy {
     uint32_t guest_features[2];
     VirtIOPCIQueue vqs[VIRTIO_QUEUE_MAX];
 
-    bool ioeventfd_started;
     VirtIOIRQFD *vector_irqfd;
     int nvqs_with_notifiers;
     VirtioBusState bus;
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 4aabec4..f74ef1e 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -70,17 +70,9 @@ typedef struct VirtioBusClass {
     void (*device_unplugged)(DeviceState *d);
     int (*query_nvectors)(DeviceState *d);
     /*
-     * ioeventfd handling: if the transport implements ioeventfd_started,
-     * it must implement the other ioeventfd callbacks as well
+     * ioeventfd handling: if the transport implements ioeventfd_assign,
+     * it must implement ioeventfd_disabled as well.
      */
-    /* Returns true if the ioeventfd has been started for the device. */
-    bool (*ioeventfd_started)(DeviceState *d);
-    /*
-     * Sets the 'ioeventfd started' state after the ioeventfd has been
-     * started/stopped for the device. err signifies whether an error
-     * had occurred.
-     */
-    void (*ioeventfd_set_started)(DeviceState *d, bool started, bool err);
     /* Returns true if the ioeventfd has been disabled for the device. */
     bool (*ioeventfd_disabled)(DeviceState *d);
     /*
@@ -106,6 +98,11 @@ struct VirtioBusState {
      * or dataplane.
      */
     bool ioeventfd_disabled;
+
+    /*
+     * Set if ioeventfd has been started.
+     */
+    bool ioeventfd_started;
 };
 
 void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 03/13] virtio: move ioeventfd_started flag to VirtioBusState

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:31 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This simplifies the code and removes the ioeventfd_started
> and ioeventfd_set_started callback.  The only difference is
> in how virtio-ccw handles an error---it doesn't disable
> ioeventfd forever anymore.  It was the only backend to do
> so, and if desired this behavior should be implemented in
> virtio-bus.c.

I guess this depends on whether we consider failures to be generally
transient or generally permanent. I decided to disable ioeventfd on
failure as I could not think of a transient failure; but in the end, it
does not really matter much, so I'm fine with this change.

> 
> Instead of ioeventfd_started, the ioeventfd_assign callback now
> determines whether the virtio bus supports host notifiers.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/block/dataplane/virtio-blk.c |  2 +-
>  hw/s390x/virtio-ccw.c           | 21 ---------------------
>  hw/s390x/virtio-ccw.h           |  1 -
>  hw/scsi/virtio-scsi-dataplane.c |  2 +-
>  hw/virtio/vhost.c               |  2 +-
>  hw/virtio/virtio-bus.c          | 14 ++++++--------
>  hw/virtio/virtio-mmio.c         | 18 ------------------
>  hw/virtio/virtio-pci.c          | 17 -----------------
>  hw/virtio/virtio-pci.h          |  1 -
>  include/hw/virtio/virtio-bus.h  | 17 +++++++----------
>  10 files changed, 16 insertions(+), 79 deletions(-)

Nice amount of code deletion :)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH 04/13] virtio: add start_ioeventfd and stop_ioeventfd to VirtioDeviceClass

[Paolo Bonzini]

Allow customization of the start and stop of ioeventfd.  This will
allow direct start of dataplane without passing through the default
ioeventfd handlers, which in turn allows using the dataplane logic
instead of virtio_add_queue_aio.  It will also enable some code
simplification, because the sole entry point to ioeventfd setup
will be virtio_bus_set_host_notifier.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/virtio-bus.c         | 55 +++++++++++-------------------------
 hw/virtio/virtio.c             | 64 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-bus.h |  7 ++++-
 include/hw/virtio/virtio.h     |  4 +++
 4 files changed, 90 insertions(+), 40 deletions(-)

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 97cdb11..1e09f78 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -153,8 +153,8 @@ void virtio_bus_set_vdev_config(VirtioBusState *bus, uint8_t *config)
  * assign: register/deregister ioeventfd with the kernel
  * set_handler: use the generic ioeventfd handler
  */
-static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
-                                      int n, bool assign, bool set_handler)
+int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
+                               int n, bool assign, bool set_handler)
 {
     VirtIODevice *vdev = virtio_bus_get_device(bus);
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
@@ -185,61 +185,38 @@ static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
     return r;
 }
 
-void virtio_bus_start_ioeventfd(VirtioBusState *bus)
+int virtio_bus_start_ioeventfd(VirtioBusState *bus)
 {
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
     DeviceState *proxy = DEVICE(BUS(bus)->parent);
-    VirtIODevice *vdev;
-    int n, r;
+    VirtIODevice *vdev = virtio_bus_get_device(bus);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
+    int r;
 
     if (!k->ioeventfd_assign || k->ioeventfd_disabled(proxy)) {
-        return;
+        return -ENOSYS;
     }
     if (bus->ioeventfd_started || bus->ioeventfd_disabled) {
-        return;
+        return 0;
     }
-    vdev = virtio_bus_get_device(bus);
-    for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
-        if (!virtio_queue_get_num(vdev, n)) {
-            continue;
-        }
-        r = set_host_notifier_internal(proxy, bus, n, true, true);
-        if (r < 0) {
-            goto assign_error;
-        }
+    r = vdc->start_ioeventfd(vdev);
+    if (r < 0) {
+        error_report("%s: failed. Fallback to userspace (slower).", __func__);
+        return r;
     }
     bus->ioeventfd_started = true;
-    return;
-
-assign_error:
-    while (--n >= 0) {
-        if (!virtio_queue_get_num(vdev, n)) {
-            continue;
-        }
-
-        r = set_host_notifier_internal(proxy, bus, n, false, false);
-        assert(r >= 0);
-    }
-    error_report("%s: failed. Fallback to userspace (slower).", __func__);
+    return 0;
 }
 
 void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
 {
-    DeviceState *proxy = DEVICE(BUS(bus)->parent);
-    VirtIODevice *vdev;
-    int n, r;
+    VirtIODevice *vdev = virtio_bus_get_device(bus);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
     if (!bus->ioeventfd_started) {
         return;
     }
-    vdev = virtio_bus_get_device(bus);
-    for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
-        if (!virtio_queue_get_num(vdev, n)) {
-            continue;
-        }
-        r = set_host_notifier_internal(proxy, bus, n, false, false);
-        assert(r >= 0);
-    }
+    vdc->stop_ioeventfd(vdev);
     bus->ioeventfd_started = false;
 }
 
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 18ce333..3f6005a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2120,15 +2120,79 @@ static Property virtio_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
+{
+    VirtioBusState *qbus = VIRTIO_BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    DeviceState *proxy = DEVICE(BUS(qbus)->parent);
+    int n, r, err;
+
+    for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
+        if (!virtio_queue_get_num(vdev, n)) {
+            continue;
+        }
+        r = set_host_notifier_internal(proxy, qbus, n, true, true);
+        if (r < 0) {
+            err = r;
+            goto assign_error;
+        }
+    }
+    return 0;
+
+assign_error:
+    while (--n >= 0) {
+        if (!virtio_queue_get_num(vdev, n)) {
+            continue;
+        }
+
+        r = set_host_notifier_internal(proxy, qbus, n, false, false);
+        assert(r >= 0);
+    }
+    return err;
+}
+
+int virtio_device_start_ioeventfd(VirtIODevice *vdev)
+{
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
+    VirtioBusState *vbus = VIRTIO_BUS(qbus);
+
+    return virtio_bus_start_ioeventfd(vbus);
+}
+
+static void virtio_device_stop_ioeventfd_impl(VirtIODevice *vdev)
+{
+    VirtioBusState *qbus = VIRTIO_BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    DeviceState *proxy = DEVICE(BUS(qbus)->parent);
+    int n, r;
+
+    for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
+        if (!virtio_queue_get_num(vdev, n)) {
+            continue;
+        }
+        r = set_host_notifier_internal(proxy, qbus, n, false, false);
+        assert(r >= 0);
+    }
+}
+
+void virtio_device_stop_ioeventfd(VirtIODevice *vdev)
+{
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
+    VirtioBusState *vbus = VIRTIO_BUS(qbus);
+
+    virtio_bus_stop_ioeventfd(vbus);
+}
+
 static void virtio_device_class_init(ObjectClass *klass, void *data)
 {
     /* Set the default value here. */
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = virtio_device_realize;
     dc->unrealize = virtio_device_unrealize;
     dc->bus_type = TYPE_VIRTIO_BUS;
     dc->props = virtio_properties;
+    vdc->start_ioeventfd = virtio_device_start_ioeventfd_impl;
+    vdc->stop_ioeventfd = virtio_device_stop_ioeventfd_impl;
 }
 
 static const TypeInfo virtio_device_info = {
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index f74ef1e..aa326e1 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -132,10 +132,15 @@ static inline VirtIODevice *virtio_bus_get_device(VirtioBusState *bus)
 }
 
 /* Start the ioeventfd. */
-void virtio_bus_start_ioeventfd(VirtioBusState *bus);
+int virtio_bus_start_ioeventfd(VirtioBusState *bus);
 /* Stop the ioeventfd. */
 void virtio_bus_stop_ioeventfd(VirtioBusState *bus);
 /* Switch from/to the generic ioeventfd handler */
 int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign);
 
+/* This is temporary.  It is only needed because virtio_bus_set_host_notifier
+ * sets ioeventfd_disabled but we will shortly get rid of it.  */
+int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
+                               int n, bool assign, bool set_handler);
+
 #endif /* VIRTIO_BUS_H */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 888c8de..343a781 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -125,6 +125,8 @@ typedef struct VirtioDeviceClass {
      * must mask in frontend instead.
      */
     void (*guest_notifier_mask)(VirtIODevice *vdev, int n, bool mask);
+    int (*start_ioeventfd)(VirtIODevice *vdev);
+    void (*stop_ioeventfd)(VirtIODevice *vdev);
     void (*save)(VirtIODevice *vdev, QEMUFile *f);
     int (*load)(VirtIODevice *vdev, QEMUFile *f, int version_id);
 } VirtioDeviceClass;
@@ -274,6 +276,8 @@ uint16_t virtio_get_queue_index(VirtQueue *vq);
 EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq);
 void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                 bool with_irqfd);
+int virtio_device_start_ioeventfd(VirtIODevice *vdev);
+void virtio_device_stop_ioeventfd(VirtIODevice *vdev);
 EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
 void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 04/13] virtio: add start_ioeventfd and stop_ioeventfd to VirtioDeviceClass

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:32 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Allow customization of the start and stop of ioeventfd.  This will
> allow direct start of dataplane without passing through the default
> ioeventfd handlers, which in turn allows using the dataplane logic
> instead of virtio_add_queue_aio.  It will also enable some code
> simplification, because the sole entry point to ioeventfd setup
> will be virtio_bus_set_host_notifier.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio/virtio-bus.c         | 55 +++++++++++-------------------------
>  hw/virtio/virtio.c             | 64 ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-bus.h |  7 ++++-
>  include/hw/virtio/virtio.h     |  4 +++
>  4 files changed, 90 insertions(+), 40 deletions(-)
> 

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 18ce333..3f6005a 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2120,15 +2120,79 @@ static Property virtio_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
> 
> +static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)

The _impl postfix looks a bit awkward.
virtio_device_do_start_ioeventfd?
virtio_device_start_ioeventd_internal? (I don't really like these
either...)

Anyway,

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH 05/13] virtio: introduce virtio_device_ioeventfd_enabled

[Paolo Bonzini]

This will be used to forbid iothread configuration when the
proxy does not allow using ioeventfd.  To simplify the implementation,
change the direction of the ioeventfd_disabled callback too.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        v1->v2: check k->ioeventfd_assign in virtio_bus_ioeventfd_enabled

 hw/s390x/virtio-ccw.c          |  6 +++---
 hw/virtio/virtio-bus.c         | 10 +++++++++-
 hw/virtio/virtio-mmio.c        |  6 +++---
 hw/virtio/virtio-pci.c         |  6 +++---
 hw/virtio/virtio.c             |  8 ++++++++
 include/hw/virtio/virtio-bus.h |  8 +++++---
 include/hw/virtio/virtio.h     |  1 +
 7 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index bf5670c..7d7f8f6 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -59,11 +59,11 @@ static void virtio_ccw_stop_ioeventfd(VirtioCcwDevice *dev)
     virtio_bus_stop_ioeventfd(&dev->bus);
 }
 
-static bool virtio_ccw_ioeventfd_disabled(DeviceState *d)
+static bool virtio_ccw_ioeventfd_enabled(DeviceState *d)
 {
     VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
 
-    return !(dev->flags & VIRTIO_CCW_FLAG_USE_IOEVENTFD);
+    return (dev->flags & VIRTIO_CCW_FLAG_USE_IOEVENTFD) != 0;
 }
 
 static int virtio_ccw_ioeventfd_assign(DeviceState *d, EventNotifier *notifier,
@@ -1589,7 +1589,7 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
     k->pre_plugged = virtio_ccw_pre_plugged;
     k->device_plugged = virtio_ccw_device_plugged;
     k->device_unplugged = virtio_ccw_device_unplugged;
-    k->ioeventfd_disabled = virtio_ccw_ioeventfd_disabled;
+    k->ioeventfd_enabled = virtio_ccw_ioeventfd_enabled;
     k->ioeventfd_assign = virtio_ccw_ioeventfd_assign;
 }
 
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 1e09f78..ac8b24b 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -193,7 +193,7 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus)
     VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
     int r;
 
-    if (!k->ioeventfd_assign || k->ioeventfd_disabled(proxy)) {
+    if (!k->ioeventfd_assign || !k->ioeventfd_enabled(proxy)) {
         return -ENOSYS;
     }
     if (bus->ioeventfd_started || bus->ioeventfd_disabled) {
@@ -220,6 +220,14 @@ void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
     bus->ioeventfd_started = false;
 }
 
+bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus)
+{
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
+    DeviceState *proxy = DEVICE(BUS(bus)->parent);
+
+    return k->ioeventfd_assign && k->ioeventfd_enabled(proxy);
+}
+
 /*
  * This function switches from/to the generic ioeventfd handler.
  * assign==false means 'use generic ioeventfd handler'.
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 04a9655..a30270f 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -92,9 +92,9 @@ typedef struct {
     bool format_transport_address;
 } VirtIOMMIOProxy;
 
-static bool virtio_mmio_ioeventfd_disabled(DeviceState *d)
+static bool virtio_mmio_ioeventfd_enabled(DeviceState *d)
 {
-    return !kvm_eventfds_enabled();
+    return kvm_eventfds_enabled();
 }
 
 static int virtio_mmio_ioeventfd_assign(DeviceState *d,
@@ -531,7 +531,7 @@ static void virtio_mmio_bus_class_init(ObjectClass *klass, void *data)
     k->save_config = virtio_mmio_save_config;
     k->load_config = virtio_mmio_load_config;
     k->set_guest_notifiers = virtio_mmio_set_guest_notifiers;
-    k->ioeventfd_disabled = virtio_mmio_ioeventfd_disabled;
+    k->ioeventfd_enabled = virtio_mmio_ioeventfd_enabled;
     k->ioeventfd_assign = virtio_mmio_ioeventfd_assign;
     k->has_variable_vring_alignment = true;
     bus_class->max_dev = 1;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 7752ed6..5a0b976 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -262,11 +262,11 @@ static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f)
     return 0;
 }
 
-static bool virtio_pci_ioeventfd_disabled(DeviceState *d)
+static bool virtio_pci_ioeventfd_enabled(DeviceState *d)
 {
     VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
 
-    return !(proxy->flags & VIRTIO_PCI_FLAG_USE_IOEVENTFD);
+    return (proxy->flags & VIRTIO_PCI_FLAG_USE_IOEVENTFD) != 0;
 }
 
 #define QEMU_VIRTIO_PCI_QUEUE_MEM_MULT 0x1000
@@ -2516,7 +2516,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
     k->device_plugged = virtio_pci_device_plugged;
     k->device_unplugged = virtio_pci_device_unplugged;
     k->query_nvectors = virtio_pci_query_nvectors;
-    k->ioeventfd_disabled = virtio_pci_ioeventfd_disabled;
+    k->ioeventfd_enabled = virtio_pci_ioeventfd_enabled;
     k->ioeventfd_assign = virtio_pci_ioeventfd_assign;
 }
 
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 3f6005a..d1141fb 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2195,6 +2195,14 @@ static void virtio_device_class_init(ObjectClass *klass, void *data)
     vdc->stop_ioeventfd = virtio_device_stop_ioeventfd_impl;
 }
 
+bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev)
+{
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
+    VirtioBusState *vbus = VIRTIO_BUS(qbus);
+
+    return virtio_bus_ioeventfd_enabled(vbus);
+}
+
 static const TypeInfo virtio_device_info = {
     .name = TYPE_VIRTIO_DEVICE,
     .parent = TYPE_DEVICE,
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index aa326e1..521fac7 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -71,10 +71,10 @@ typedef struct VirtioBusClass {
     int (*query_nvectors)(DeviceState *d);
     /*
      * ioeventfd handling: if the transport implements ioeventfd_assign,
-     * it must implement ioeventfd_disabled as well.
+     * it must implement ioeventfd_enabled as well.
      */
-    /* Returns true if the ioeventfd has been disabled for the device. */
-    bool (*ioeventfd_disabled)(DeviceState *d);
+    /* Returns true if the ioeventfd is enabled for the device. */
+    bool (*ioeventfd_enabled)(DeviceState *d);
     /*
      * Assigns/deassigns the ioeventfd backing for the transport on
      * the device for queue number n. Returns an error value on
@@ -131,6 +131,8 @@ static inline VirtIODevice *virtio_bus_get_device(VirtioBusState *bus)
     return (VirtIODevice *)qdev;
 }
 
+/* Return whether the proxy allows ioeventfd.  */
+bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus);
 /* Start the ioeventfd. */
 int virtio_bus_start_ioeventfd(VirtioBusState *bus);
 /* Stop the ioeventfd. */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 343a781..5a5ba72 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -278,6 +278,7 @@ void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                 bool with_irqfd);
 int virtio_device_start_ioeventfd(VirtIODevice *vdev);
 void virtio_device_stop_ioeventfd(VirtIODevice *vdev);
+bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev);
 EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
 void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 05/13] virtio: introduce virtio_device_ioeventfd_enabled

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:33 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This will be used to forbid iothread configuration when the
> proxy does not allow using ioeventfd.  To simplify the implementation,
> change the direction of the ioeventfd_disabled callback too.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>         v1->v2: check k->ioeventfd_assign in virtio_bus_ioeventfd_enabled
> 
>  hw/s390x/virtio-ccw.c          |  6 +++---
>  hw/virtio/virtio-bus.c         | 10 +++++++++-
>  hw/virtio/virtio-mmio.c        |  6 +++---
>  hw/virtio/virtio-pci.c         |  6 +++---
>  hw/virtio/virtio.c             |  8 ++++++++
>  include/hw/virtio/virtio-bus.h |  8 +++++---
>  include/hw/virtio/virtio.h     |  1 +
>  7 files changed, 32 insertions(+), 13 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH 06/13] virtio-blk: always use dataplane path if ioeventfd is active

[Paolo Bonzini]

Override start_ioeventfd and stop_ioeventfd to start/stop the
whole dataplane logic.  This has some positive side effects:

- no need anymore for virtio_add_queue_aio (i.e. a revert of
  commit 0ff841f6d138904d514efa1d885bcaf54583852d)

- no need anymore to switch from generic ioeventfd handlers to
  dataplane

It detects some errors better:

    $ qemu-system-x86_64 -object iothread,id=io \
          -drive id=null,file=null-aio://,if=none,format=raw \
          -device virtio-blk-pci,ioeventfd=off,iothread=io,drive=null
    qemu-system-x86_64: -device virtio-blk-pci,ioeventfd=off,iothread=io,drive=null:
    ioeventfd is required for iothread

while previously it would have started just fine.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        v1->v2: improvement in error message consistency
        loosen virtio_blk_set_status assertion [Christian]

 hw/block/dataplane/virtio-blk.c | 74 +++++++++++++++++++++++++----------------
 hw/block/dataplane/virtio-blk.h |  6 ++--
 hw/block/virtio-blk.c           | 15 ++++-----
 3 files changed, 55 insertions(+), 40 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 9b268f4..d96f45c 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -83,28 +83,33 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
                                   Error **errp)
 {
     VirtIOBlockDataPlane *s;
-    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
 
     *dataplane = NULL;
 
-    if (!conf->iothread) {
-        return;
-    }
-
     /* Don't try if transport does not support notifiers. */
-    if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
-        error_setg(errp,
-                   "device is incompatible with dataplane "
-                   "(transport does not support notifiers)");
-        return;
-    }
+    if (conf->iothread) {
+        if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
+            error_setg(errp,
+                       "device is incompatible with iothread "
+                       "(transport does not support notifiers)");
+            return;
+        }
+        if (!virtio_device_ioeventfd_enabled(vdev)) {
+            error_setg(errp, "ioeventfd is required for iothread");
+            return;
+        }
 
-    /* If dataplane is (re-)enabled while the guest is running there could be
-     * block jobs that can conflict.
-     */
-    if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
-        error_prepend(errp, "cannot start dataplane thread: ");
+        /* If dataplane is (re-)enabled while the guest is running there could
+         * be block jobs that can conflict.
+         */
+        if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
+            error_prepend(errp, "cannot start virtio-blk dataplane: ");
+            return;
+        }
+    }
+    if (!virtio_device_ioeventfd_enabled(vdev)) {
         return;
     }
 
@@ -112,9 +117,13 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
     s->vdev = vdev;
     s->conf = conf;
 
-    s->iothread = conf->iothread;
-    object_ref(OBJECT(s->iothread));
-    s->ctx = iothread_get_aio_context(s->iothread);
+    if (conf->iothread) {
+        s->iothread = conf->iothread;
+        object_ref(OBJECT(s->iothread));
+        s->ctx = iothread_get_aio_context(s->iothread);
+    } else {
+        s->ctx = qemu_get_aio_context();
+    }
     s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
     s->batch_notify_vqs = bitmap_new(conf->num_queues);
 
@@ -124,14 +133,18 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
 /* Context: QEMU global mutex held */
 void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
 {
+    VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
+
     if (!s) {
         return;
     }
 
-    virtio_blk_data_plane_stop(s);
+    assert(!vblk->dataplane_started);
     g_free(s->batch_notify_vqs);
     qemu_bh_delete(s->bh);
-    object_unref(OBJECT(s->iothread));
+    if (s->iothread) {
+        object_unref(OBJECT(s->iothread));
+    }
     g_free(s);
 }
 
@@ -147,17 +160,18 @@ static void virtio_blk_data_plane_handle_output(VirtIODevice *vdev,
 }
 
 /* Context: QEMU global mutex held */
-void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
+int virtio_blk_data_plane_start(VirtIODevice *vdev)
 {
-    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
+    VirtIOBlock *vblk = VIRTIO_BLK(vdev);
+    VirtIOBlockDataPlane *s = vblk->dataplane;
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vblk)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-    VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
     unsigned i;
     unsigned nvqs = s->conf->num_queues;
     int r;
 
     if (vblk->dataplane_started || s->starting) {
-        return;
+        return 0;
     }
 
     s->starting = true;
@@ -204,20 +218,22 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
                 virtio_blk_data_plane_handle_output);
     }
     aio_context_release(s->ctx);
-    return;
+    return 0;
 
   fail_guest_notifiers:
     vblk->dataplane_disabled = true;
     s->starting = false;
     vblk->dataplane_started = true;
+    return -ENOSYS;
 }
 
 /* Context: QEMU global mutex held */
-void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
+void virtio_blk_data_plane_stop(VirtIODevice *vdev)
 {
-    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
+    VirtIOBlock *vblk = VIRTIO_BLK(vdev);
+    VirtIOBlockDataPlane *s = vblk->dataplane;
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vblk));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-    VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
     unsigned i;
     unsigned nvqs = s->conf->num_queues;
 
diff --git a/hw/block/dataplane/virtio-blk.h b/hw/block/dataplane/virtio-blk.h
index b1f0b95..db3f47b 100644
--- a/hw/block/dataplane/virtio-blk.h
+++ b/hw/block/dataplane/virtio-blk.h
@@ -23,9 +23,9 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
                                   VirtIOBlockDataPlane **dataplane,
                                   Error **errp);
 void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s);
-void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s);
-void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s);
-void virtio_blk_data_plane_drain(VirtIOBlockDataPlane *s);
 void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s, VirtQueue *vq);
 
+int virtio_blk_data_plane_start(VirtIODevice *vdev);
+void virtio_blk_data_plane_stop(VirtIODevice *vdev);
+
 #endif /* HW_DATAPLANE_VIRTIO_BLK_H */
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 3a6112f..569a432 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -604,7 +604,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
         /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
          * dataplane here instead of waiting for .set_status().
          */
-        virtio_blk_data_plane_start(s->dataplane);
+        virtio_device_start_ioeventfd(vdev);
         if (!s->dataplane_disabled) {
             return;
         }
@@ -668,11 +668,9 @@ static void virtio_blk_reset(VirtIODevice *vdev)
         virtio_blk_free_request(req);
     }
 
-    if (s->dataplane) {
-        virtio_blk_data_plane_stop(s->dataplane);
-    }
     aio_context_release(ctx);
 
+    assert(!s->dataplane_started);
     blk_set_enable_write_cache(s->blk, s->original_wce);
 }
 
@@ -770,9 +768,8 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
 
-    if (s->dataplane && !(status & (VIRTIO_CONFIG_S_DRIVER |
-                                    VIRTIO_CONFIG_S_DRIVER_OK))) {
-        virtio_blk_data_plane_stop(s->dataplane);
+    if (!(status & (VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK))) {
+        assert(!s->dataplane_started);
     }
 
     if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
@@ -915,7 +912,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     s->sector_mask = (s->conf.conf.logical_block_size / BDRV_SECTOR_SIZE) - 1;
 
     for (i = 0; i < conf->num_queues; i++) {
-        virtio_add_queue_aio(vdev, 128, virtio_blk_handle_output);
+        virtio_add_queue(vdev, 128, virtio_blk_handle_output);
     }
     virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err);
     if (err != NULL) {
@@ -990,6 +987,8 @@ static void virtio_blk_class_init(ObjectClass *klass, void *data)
     vdc->reset = virtio_blk_reset;
     vdc->save = virtio_blk_save_device;
     vdc->load = virtio_blk_load_device;
+    vdc->start_ioeventfd = virtio_blk_data_plane_start;
+    vdc->stop_ioeventfd = virtio_blk_data_plane_stop;
 }
 
 static const TypeInfo virtio_blk_info = {
-- 
2.7.4

Re: [Qemu-devel] [PATCH 06/13] virtio-blk: always use dataplane path if ioeventfd is active

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:34 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Override start_ioeventfd and stop_ioeventfd to start/stop the
> whole dataplane logic.  This has some positive side effects:
> 
> - no need anymore for virtio_add_queue_aio (i.e. a revert of
>   commit 0ff841f6d138904d514efa1d885bcaf54583852d)
> 
> - no need anymore to switch from generic ioeventfd handlers to
>   dataplane
> 
> It detects some errors better:
> 
>     $ qemu-system-x86_64 -object iothread,id=io \
>           -drive id=null,file=null-aio://,if=none,format=raw \
>           -device virtio-blk-pci,ioeventfd=off,iothread=io,drive=null
>     qemu-system-x86_64: -device virtio-blk-pci,ioeventfd=off,iothread=io,drive=null:
>     ioeventfd is required for iothread
> 
> while previously it would have started just fine.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>         v1->v2: improvement in error message consistency
>         loosen virtio_blk_set_status assertion [Christian]
> 
>  hw/block/dataplane/virtio-blk.c | 74 +++++++++++++++++++++++++----------------
>  hw/block/dataplane/virtio-blk.h |  6 ++--
>  hw/block/virtio-blk.c           | 15 ++++-----
>  3 files changed, 55 insertions(+), 40 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 9b268f4..d96f45c 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -83,28 +83,33 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
>                                    Error **errp)
>  {
>      VirtIOBlockDataPlane *s;
> -    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));

Unrelated change?

>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> 
>      *dataplane = NULL;
> 
> -    if (!conf->iothread) {
> -        return;
> -    }
> -
>      /* Don't try if transport does not support notifiers. */

I'd move this comment together with the next block, otherwise this is
confusing.

> -    if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
> -        error_setg(errp,
> -                   "device is incompatible with dataplane "
> -                   "(transport does not support notifiers)");
> -        return;
> -    }
> +    if (conf->iothread) {
> +        if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
> +            error_setg(errp,
> +                       "device is incompatible with iothread "
> +                       "(transport does not support notifiers)");
> +            return;
> +        }
> +        if (!virtio_device_ioeventfd_enabled(vdev)) {
> +            error_setg(errp, "ioeventfd is required for iothread");
> +            return;
> +        }
> 
> -    /* If dataplane is (re-)enabled while the guest is running there could be
> -     * block jobs that can conflict.
> -     */
> -    if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
> -        error_prepend(errp, "cannot start dataplane thread: ");
> +        /* If dataplane is (re-)enabled while the guest is running there could
> +         * be block jobs that can conflict.
> +         */
> +        if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
> +            error_prepend(errp, "cannot start virtio-blk dataplane: ");
> +            return;
> +        }
> +    }
> +    if (!virtio_device_ioeventfd_enabled(vdev)) {
>          return;
>      }
> 

(...)

> @@ -124,14 +133,18 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
>  /* Context: QEMU global mutex held */
>  void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
>  {
> +    VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
> +
>      if (!s) {

You already dereferenced s above...

>          return;
>      }
> 
> -    virtio_blk_data_plane_stop(s);
> +    assert(!vblk->dataplane_started);
>      g_free(s->batch_notify_vqs);
>      qemu_bh_delete(s->bh);
> -    object_unref(OBJECT(s->iothread));
> +    if (s->iothread) {
> +        object_unref(OBJECT(s->iothread));
> +    }
>      g_free(s);
>  }
> 

In general, I think this looks good, but I'll have a look at the end
result as well.

Re: [Qemu-devel] [PATCH 06/13] virtio-blk: always use dataplane path if ioeventfd is active

[Cornelia Huck]

On Wed, 19 Oct 2016 12:48:27 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> In general, I think this looks good, but I'll have a look at the end
> result as well.

With my comments addressed:

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH 07/13] virtio-scsi: always use dataplane path if ioeventfd is active

[Paolo Bonzini]

Override start_ioeventfd and stop_ioeventfd to start/stop the
whole dataplane logic.  This has some positive side effects:

- no need anymore for virtio_add_queue_aio (i.e. a revert of
  commit 1c627137c10ee2dcf59e0383ade8a9abfa2d4355)

- no need anymore to switch from generic ioeventfd handlers to
  dataplane

It detects some errors better:

    $ qemu-system-x86_64 -object iothread,id=io \
          -device virtio-scsi-pci,ioeventfd=off,iothread=io
    qemu-system-x86_64: -device virtio-scsi-pci,ioeventfd=off,iothread=io:
    ioeventfd is required for iothread

while previously it would have started just fine.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/virtio-scsi-dataplane.c | 56 +++++++++++++++++++++++++----------------
 hw/scsi/virtio-scsi.c           | 24 ++++++++----------
 include/hw/virtio/virtio-scsi.h |  6 ++---
 3 files changed, 48 insertions(+), 38 deletions(-)

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index f537b4e..aa6be54 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -12,6 +12,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/virtio/virtio-scsi.h"
 #include "qemu/error-report.h"
 #include "sysemu/block-backend.h"
@@ -21,20 +22,30 @@
 #include "hw/virtio/virtio-access.h"
 
 /* Context: QEMU global mutex held */
-void virtio_scsi_set_iothread(VirtIOSCSI *s, IOThread *iothread)
+void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp)
 {
-    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
-    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
 
-    assert(!s->ctx);
-    s->ctx = iothread_get_aio_context(vs->conf.iothread);
-
-    /* Don't try if transport does not support notifiers. */
-    if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
-        fprintf(stderr, "virtio-scsi: Failed to set iothread "
-                   "(transport does not support notifiers)");
-        exit(1);
+    if (vs->conf.iothread) {
+        if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
+            error_setg(errp,
+                       "device is incompatible with iothread "
+                       "(transport does not support notifiers)");
+            return;
+        }
+        if (!virtio_device_ioeventfd_enabled(vdev)) {
+            error_setg(errp, "ioeventfd is required for iothread");
+            return;
+        }
+        s->ctx = iothread_get_aio_context(vs->conf.iothread);
+    } else {
+        if (!virtio_device_ioeventfd_enabled(vdev)) {
+            return;
+        }
+        s->ctx = qemu_get_aio_context();
     }
 }
 
@@ -105,19 +116,19 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s)
 }
 
 /* Context: QEMU global mutex held */
-void virtio_scsi_dataplane_start(VirtIOSCSI *s)
+int virtio_scsi_dataplane_start(VirtIODevice *vdev)
 {
     int i;
     int rc;
-    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
+    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
+    VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
     if (s->dataplane_started ||
         s->dataplane_starting ||
-        s->dataplane_fenced ||
-        s->ctx != iothread_get_aio_context(vs->conf.iothread)) {
-        return;
+        s->dataplane_fenced) {
+        return 0;
     }
 
     s->dataplane_starting = true;
@@ -152,7 +163,7 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
     s->dataplane_starting = false;
     s->dataplane_started = true;
     aio_context_release(s->ctx);
-    return;
+    return 0;
 
 fail_vrings:
     virtio_scsi_clear_aio(s);
@@ -165,14 +176,16 @@ fail_guest_notifiers:
     s->dataplane_fenced = true;
     s->dataplane_starting = false;
     s->dataplane_started = true;
+    return -ENOSYS;
 }
 
 /* Context: QEMU global mutex held */
-void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
+void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
 {
-    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
+    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
+    VirtIOSCSI *s = VIRTIO_SCSI(vdev);
     int i;
 
     if (!s->dataplane_started || s->dataplane_stopping) {
@@ -186,7 +199,6 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
         return;
     }
     s->dataplane_stopping = true;
-    assert(s->ctx == iothread_get_aio_context(vs->conf.iothread));
 
     aio_context_acquire(s->ctx);
 
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index e596b64..38d3068 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -431,7 +431,7 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     VirtIOSCSI *s = (VirtIOSCSI *)vdev;
 
     if (s->ctx) {
-        virtio_scsi_dataplane_start(s);
+        virtio_device_start_ioeventfd(vdev);
         if (!s->dataplane_fenced) {
             return;
         }
@@ -593,7 +593,7 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
     VirtIOSCSI *s = (VirtIOSCSI *)vdev;
 
     if (s->ctx) {
-        virtio_scsi_dataplane_start(s);
+        virtio_device_start_ioeventfd(vdev);
         if (!s->dataplane_fenced) {
             return;
         }
@@ -651,9 +651,7 @@ static void virtio_scsi_reset(VirtIODevice *vdev)
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
 
-    if (s->ctx) {
-        virtio_scsi_dataplane_stop(s);
-    }
+    assert(!s->dataplane_started);
     s->resetting++;
     qbus_reset_all(&s->bus.qbus);
     s->resetting--;
@@ -746,7 +744,7 @@ static void virtio_scsi_handle_event(VirtIODevice *vdev, VirtQueue *vq)
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
     if (s->ctx) {
-        virtio_scsi_dataplane_start(s);
+        virtio_device_start_ioeventfd(vdev);
         if (!s->dataplane_fenced) {
             return;
         }
@@ -845,14 +843,10 @@ void virtio_scsi_common_realize(DeviceState *dev, Error **errp,
     s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
     s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
 
-    s->ctrl_vq = virtio_add_queue_aio(vdev, VIRTIO_SCSI_VQ_SIZE, ctrl);
-    s->event_vq = virtio_add_queue_aio(vdev, VIRTIO_SCSI_VQ_SIZE, evt);
+    s->ctrl_vq = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, ctrl);
+    s->event_vq = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, evt);
     for (i = 0; i < s->conf.num_queues; i++) {
-        s->cmd_vqs[i] = virtio_add_queue_aio(vdev, VIRTIO_SCSI_VQ_SIZE, cmd);
-    }
-
-    if (s->conf.iothread) {
-        virtio_scsi_set_iothread(VIRTIO_SCSI(s), s->conf.iothread);
+        s->cmd_vqs[i] = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, cmd);
     }
 }
 
@@ -882,6 +876,8 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
             return;
         }
     }
+
+    virtio_scsi_dataplane_setup(s, errp);
 }
 
 static void virtio_scsi_instance_init(Object *obj)
@@ -946,6 +942,8 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data)
     vdc->set_config = virtio_scsi_set_config;
     vdc->get_features = virtio_scsi_get_features;
     vdc->reset = virtio_scsi_reset;
+    vdc->start_ioeventfd = virtio_scsi_dataplane_start;
+    vdc->stop_ioeventfd = virtio_scsi_dataplane_stop;
     hc->plug = virtio_scsi_hotplug;
     hc->unplug = virtio_scsi_hotunplug;
 }
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index a1e0cfb..9fbc7d7 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -134,9 +134,9 @@ void virtio_scsi_free_req(VirtIOSCSIReq *req);
 void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
                             uint32_t event, uint32_t reason);
 
-void virtio_scsi_set_iothread(VirtIOSCSI *s, IOThread *iothread);
-void virtio_scsi_dataplane_start(VirtIOSCSI *s);
-void virtio_scsi_dataplane_stop(VirtIOSCSI *s);
+void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp);
+int virtio_scsi_dataplane_start(VirtIODevice *s);
+void virtio_scsi_dataplane_stop(VirtIODevice *s);
 void virtio_scsi_dataplane_notify(VirtIODevice *vdev, VirtIOSCSIReq *req);
 
 #endif /* QEMU_VIRTIO_SCSI_H */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 07/13] virtio-scsi: always use dataplane path if ioeventfd is active

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:35 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Override start_ioeventfd and stop_ioeventfd to start/stop the
> whole dataplane logic.  This has some positive side effects:
> 
> - no need anymore for virtio_add_queue_aio (i.e. a revert of
>   commit 1c627137c10ee2dcf59e0383ade8a9abfa2d4355)
> 
> - no need anymore to switch from generic ioeventfd handlers to
>   dataplane
> 
> It detects some errors better:
> 
>     $ qemu-system-x86_64 -object iothread,id=io \
>           -device virtio-scsi-pci,ioeventfd=off,iothread=io
>     qemu-system-x86_64: -device virtio-scsi-pci,ioeventfd=off,iothread=io:
>     ioeventfd is required for iothread
> 
> while previously it would have started just fine.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi/virtio-scsi-dataplane.c | 56 +++++++++++++++++++++++++----------------
>  hw/scsi/virtio-scsi.c           | 24 ++++++++----------
>  include/hw/virtio/virtio-scsi.h |  6 ++---
>  3 files changed, 48 insertions(+), 38 deletions(-)

Looks good to me as well, but give me some more time before I give an
R-b.

Re: [Qemu-devel] [PATCH 07/13] virtio-scsi: always use dataplane path if ioeventfd is active

[Cornelia Huck]

On Wed, 19 Oct 2016 12:51:37 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Mon, 10 Oct 2016 13:53:35 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > Override start_ioeventfd and stop_ioeventfd to start/stop the
> > whole dataplane logic.  This has some positive side effects:
> > 
> > - no need anymore for virtio_add_queue_aio (i.e. a revert of
> >   commit 1c627137c10ee2dcf59e0383ade8a9abfa2d4355)
> > 
> > - no need anymore to switch from generic ioeventfd handlers to
> >   dataplane
> > 
> > It detects some errors better:
> > 
> >     $ qemu-system-x86_64 -object iothread,id=io \
> >           -device virtio-scsi-pci,ioeventfd=off,iothread=io
> >     qemu-system-x86_64: -device virtio-scsi-pci,ioeventfd=off,iothread=io:
> >     ioeventfd is required for iothread
> > 
> > while previously it would have started just fine.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  hw/scsi/virtio-scsi-dataplane.c | 56 +++++++++++++++++++++++++----------------
> >  hw/scsi/virtio-scsi.c           | 24 ++++++++----------
> >  include/hw/virtio/virtio-scsi.h |  6 ++---
> >  3 files changed, 48 insertions(+), 38 deletions(-)
> 
> Looks good to me as well, but give me some more time before I give an
> R-b.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH 08/13] Revert "virtio: Introduce virtio_add_queue_aio"

[Paolo Bonzini]

This reverts commit 872dd82c83745a603d2e07a03d34313eb6467ae4.
virtio_add_queue_aio is unused.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/virtio.c         | 38 ++++----------------------------------
 include/hw/virtio/virtio.h |  3 ---
 2 files changed, 4 insertions(+), 37 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index d1141fb..93eec83 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -97,7 +97,6 @@ struct VirtQueue
     uint16_t vector;
     VirtIOHandleOutput handle_output;
     VirtIOHandleOutput handle_aio_output;
-    bool use_aio;
     VirtIODevice *vdev;
     EventNotifier guest_notifier;
     EventNotifier host_notifier;
@@ -1264,9 +1263,8 @@ void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector)
     }
 }
 
-static VirtQueue *virtio_add_queue_internal(VirtIODevice *vdev, int queue_size,
-                                            VirtIOHandleOutput handle_output,
-                                            bool use_aio)
+VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
+                            VirtIOHandleOutput handle_output)
 {
     int i;
 
@@ -1283,28 +1281,10 @@ static VirtQueue *virtio_add_queue_internal(VirtIODevice *vdev, int queue_size,
     vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
     vdev->vq[i].handle_output = handle_output;
     vdev->vq[i].handle_aio_output = NULL;
-    vdev->vq[i].use_aio = use_aio;
 
     return &vdev->vq[i];
 }
 
-/* Add a virt queue and mark AIO.
- * An AIO queue will use the AioContext based event interface instead of the
- * default IOHandler and EventNotifier interface.
- */
-VirtQueue *virtio_add_queue_aio(VirtIODevice *vdev, int queue_size,
-                                VirtIOHandleOutput handle_output)
-{
-    return virtio_add_queue_internal(vdev, queue_size, handle_output, true);
-}
-
-/* Add a normal virt queue (on the contrary to the AIO version above. */
-VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
-                            VirtIOHandleOutput handle_output)
-{
-    return virtio_add_queue_internal(vdev, queue_size, handle_output, false);
-}
-
 void virtio_del_queue(VirtIODevice *vdev, int n)
 {
     if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
@@ -2024,21 +2004,11 @@ static void virtio_queue_host_notifier_read(EventNotifier *n)
 void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler)
 {
-    AioContext *ctx = qemu_get_aio_context();
     if (assign && set_handler) {
-        if (vq->use_aio) {
-            aio_set_event_notifier(ctx, &vq->host_notifier, true,
+        event_notifier_set_handler(&vq->host_notifier, true,
                                    virtio_queue_host_notifier_read);
-        } else {
-            event_notifier_set_handler(&vq->host_notifier, true,
-                                       virtio_queue_host_notifier_read);
-        }
     } else {
-        if (vq->use_aio) {
-            aio_set_event_notifier(ctx, &vq->host_notifier, true, NULL);
-        } else {
-            event_notifier_set_handler(&vq->host_notifier, true, NULL);
-        }
+        event_notifier_set_handler(&vq->host_notifier, true, NULL);
     }
     if (!assign) {
         /* Test and clear notifier before after disabling event,
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 5a5ba72..89586cb 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -148,9 +148,6 @@ typedef void (*VirtIOHandleOutput)(VirtIODevice *, VirtQueue *);
 VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
                             VirtIOHandleOutput handle_output);
 
-VirtQueue *virtio_add_queue_aio(VirtIODevice *vdev, int queue_size,
-                                VirtIOHandleOutput handle_output);
-
 void virtio_del_queue(VirtIODevice *vdev, int n);
 
 void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 08/13] Revert "virtio: Introduce virtio_add_queue_aio"

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:36 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This reverts commit 872dd82c83745a603d2e07a03d34313eb6467ae4.
> virtio_add_queue_aio is unused.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio/virtio.c         | 38 ++++----------------------------------
>  include/hw/virtio/virtio.h |  3 ---
>  2 files changed, 4 insertions(+), 37 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH 09/13] virtio: remove set_handler argument from set_host_notifier_internal

[Paolo Bonzini]

Make virtio_device_start_ioeventfd_impl use the same logic as
dataplane to set up the host notifier.  This removes the need
for the set_handler argument in set_host_notifier_internal.

This is a first step towards using virtio_bus_set_host_notifier
as the sole entry point to set up ioeventfds.  At least now
the functions have the same interface, but they still differ
in that virtio_bus_set_host_notifier sets ioeventfd_disabled.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/virtio-bus.c         | 12 +++---------
 hw/virtio/virtio.c             | 16 +++++++++++++---
 include/hw/virtio/virtio-bus.h |  2 +-
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index ac8b24b..046bcc9 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -147,14 +147,8 @@ void virtio_bus_set_vdev_config(VirtioBusState *bus, uint8_t *config)
     }
 }
 
-/*
- * This function handles both assigning the ioeventfd handler and
- * registering it with the kernel.
- * assign: register/deregister ioeventfd with the kernel
- * set_handler: use the generic ioeventfd handler
- */
 int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
-                               int n, bool assign, bool set_handler)
+                               int n, bool assign)
 {
     VirtIODevice *vdev = virtio_bus_get_device(bus);
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
@@ -169,7 +163,7 @@ int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
                          __func__, strerror(-r), r);
             return r;
         }
-        virtio_queue_set_host_notifier_fd_handler(vq, true, set_handler);
+        virtio_queue_set_host_notifier_fd_handler(vq, true, false);
         r = k->ioeventfd_assign(proxy, notifier, n, assign);
         if (r < 0) {
             error_report("%s: unable to assign ioeventfd: %d", __func__, r);
@@ -254,7 +248,7 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
          */
         virtio_bus_stop_ioeventfd(bus);
     }
-    return set_host_notifier_internal(proxy, bus, n, assign, false);
+    return set_host_notifier_internal(proxy, bus, n, assign);
 }
 
 static char *virtio_bus_get_dev_path(DeviceState *dev)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 93eec83..2817d42 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2100,11 +2100,21 @@ static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
         if (!virtio_queue_get_num(vdev, n)) {
             continue;
         }
-        r = set_host_notifier_internal(proxy, qbus, n, true, true);
+        r = set_host_notifier_internal(proxy, qbus, n, true);
         if (r < 0) {
             err = r;
             goto assign_error;
         }
+        virtio_queue_set_host_notifier_fd_handler(&vdev->vq[n], true, true);
+    }
+
+    for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
+        /* Kick right away to begin processing requests already in vring */
+        VirtQueue *vq = &vdev->vq[n];;
+        if (!vq->vring.num) {
+            continue;
+        }
+        event_notifier_set(&vq->host_notifier);
     }
     return 0;
 
@@ -2114,7 +2124,7 @@ assign_error:
             continue;
         }
 
-        r = set_host_notifier_internal(proxy, qbus, n, false, false);
+        r = set_host_notifier_internal(proxy, qbus, n, false);
         assert(r >= 0);
     }
     return err;
@@ -2138,7 +2148,7 @@ static void virtio_device_stop_ioeventfd_impl(VirtIODevice *vdev)
         if (!virtio_queue_get_num(vdev, n)) {
             continue;
         }
-        r = set_host_notifier_internal(proxy, qbus, n, false, false);
+        r = set_host_notifier_internal(proxy, qbus, n, false);
         assert(r >= 0);
     }
 }
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 521fac7..af6b5c4 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -143,6 +143,6 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign);
 /* This is temporary.  It is only needed because virtio_bus_set_host_notifier
  * sets ioeventfd_disabled but we will shortly get rid of it.  */
 int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
-                               int n, bool assign, bool set_handler);
+                               int n, bool assign);
 
 #endif /* VIRTIO_BUS_H */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 09/13] virtio: remove set_handler argument from set_host_notifier_internal

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:37 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Make virtio_device_start_ioeventfd_impl use the same logic as
> dataplane to set up the host notifier.  This removes the need
> for the set_handler argument in set_host_notifier_internal.
> 
> This is a first step towards using virtio_bus_set_host_notifier
> as the sole entry point to set up ioeventfds.  At least now
> the functions have the same interface, but they still differ
> in that virtio_bus_set_host_notifier sets ioeventfd_disabled.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio/virtio-bus.c         | 12 +++---------
>  hw/virtio/virtio.c             | 16 +++++++++++++---
>  include/hw/virtio/virtio-bus.h |  2 +-
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 93eec83..2817d42 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2100,11 +2100,21 @@ static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
>          if (!virtio_queue_get_num(vdev, n)) {
>              continue;
>          }
> -        r = set_host_notifier_internal(proxy, qbus, n, true, true);
> +        r = set_host_notifier_internal(proxy, qbus, n, true);
>          if (r < 0) {
>              err = r;
>              goto assign_error;
>          }
> +        virtio_queue_set_host_notifier_fd_handler(&vdev->vq[n], true, true);
> +    }
> +
> +    for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
> +        /* Kick right away to begin processing requests already in vring */
> +        VirtQueue *vq = &vdev->vq[n];;

Extra ';'

> +        if (!vq->vring.num) {
> +            continue;
> +        }
> +        event_notifier_set(&vq->host_notifier);
>      }
>      return 0;
> 

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH 10/13] virtio: remove ioeventfd_disabled altogether

[Paolo Bonzini]

Now that there is not anymore a switch from the generic ioeventfd handler
to the dataplane handler, virtio_bus_set_host_notifier(assign=true) is
always called with !bus->ioeventfd_started, hence virtio_bus_stop_ioeventfd
does nothing in this case.  Move the invocation to vhost.c, which is the
only place that needs it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/vhost.c              |  3 +++
 hw/virtio/virtio-bus.c         | 23 ++++++++---------------
 include/hw/virtio/virtio-bus.h |  6 ------
 3 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 501a5f4..131f164 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1196,6 +1196,7 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
         goto fail;
     }
 
+    virtio_device_stop_ioeventfd(vdev);
     for (i = 0; i < hdev->nvqs; ++i) {
         r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
                                          true);
@@ -1215,6 +1216,7 @@ fail_vq:
         }
         assert (e >= 0);
     }
+    virtio_device_start_ioeventfd(vdev);
 fail:
     return r;
 }
@@ -1237,6 +1239,7 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
         }
         assert (r >= 0);
     }
+    virtio_device_start_ioeventfd(vdev);
 }
 
 /* Test and clear event pending status.
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 046bcc9..440c742 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -190,7 +190,7 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus)
     if (!k->ioeventfd_assign || !k->ioeventfd_enabled(proxy)) {
         return -ENOSYS;
     }
-    if (bus->ioeventfd_started || bus->ioeventfd_disabled) {
+    if (bus->ioeventfd_started) {
         return 0;
     }
     r = vdc->start_ioeventfd(vdev);
@@ -223,8 +223,8 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus)
 }
 
 /*
- * This function switches from/to the generic ioeventfd handler.
- * assign==false means 'use generic ioeventfd handler'.
+ * This function switches ioeventfd on/off in the device.
+ * The caller must set or clear the handlers for the EventNotifier.
  */
 int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
 {
@@ -234,19 +234,12 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
     if (!k->ioeventfd_assign) {
         return -ENOSYS;
     }
-    bus->ioeventfd_disabled = assign;
     if (assign) {
-        /*
-         * Stop using the generic ioeventfd, we are doing eventfd handling
-         * ourselves below
-         *
-         * FIXME: We should just switch the handler and not deassign the
-         * ioeventfd.
-         * Otherwise, there's a window where we don't have an
-         * ioeventfd and we may end up with a notification where
-         * we don't expect one.
-         */
-        virtio_bus_stop_ioeventfd(bus);
+        assert(!bus->ioeventfd_started);
+    } else {
+        if (!bus->ioeventfd_started) {
+            return 0;
+        }
     }
     return set_host_notifier_internal(proxy, bus, n, assign);
 }
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index af6b5c4..cbdf745 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -94,12 +94,6 @@ struct VirtioBusState {
     BusState parent_obj;
 
     /*
-     * Set if the default ioeventfd handlers are disabled by vhost
-     * or dataplane.
-     */
-    bool ioeventfd_disabled;
-
-    /*
      * Set if ioeventfd has been started.
      */
     bool ioeventfd_started;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 10/13] virtio: remove ioeventfd_disabled altogether

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:38 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Now that there is not anymore a switch from the generic ioeventfd handler
> to the dataplane handler, virtio_bus_set_host_notifier(assign=true) is
> always called with !bus->ioeventfd_started, hence virtio_bus_stop_ioeventfd
> does nothing in this case.  Move the invocation to vhost.c, which is the
> only place that needs it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio/vhost.c              |  3 +++
>  hw/virtio/virtio-bus.c         | 23 ++++++++---------------
>  include/hw/virtio/virtio-bus.h |  6 ------
>  3 files changed, 11 insertions(+), 21 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH 11/13] virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd

[Paolo Bonzini]

ioeventfd_disabled was the only reason for the default
implementation of virtio_device_start_ioeventfd_impl not to use
virtio_bus_set_host_notifier.  This is now fixed, and the sole entry
point to set up ioeventfd can now be virtio_bus_set_host_notifier.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/virtio-bus.c         | 4 ++--
 hw/virtio/virtio.c             | 8 +++-----
 include/hw/virtio/virtio-bus.h | 5 -----
 3 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 440c742..d6feaaf 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -147,8 +147,8 @@ void virtio_bus_set_vdev_config(VirtioBusState *bus, uint8_t *config)
     }
 }
 
-int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
-                               int n, bool assign)
+static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
+                                      int n, bool assign)
 {
     VirtIODevice *vdev = virtio_bus_get_device(bus);
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 2817d42..9a10d86 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2093,14 +2093,13 @@ static Property virtio_properties[] = {
 static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
 {
     VirtioBusState *qbus = VIRTIO_BUS(qdev_get_parent_bus(DEVICE(vdev)));
-    DeviceState *proxy = DEVICE(BUS(qbus)->parent);
     int n, r, err;
 
     for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
         if (!virtio_queue_get_num(vdev, n)) {
             continue;
         }
-        r = set_host_notifier_internal(proxy, qbus, n, true);
+        r = virtio_bus_set_host_notifier(qbus, n, true);
         if (r < 0) {
             err = r;
             goto assign_error;
@@ -2124,7 +2123,7 @@ assign_error:
             continue;
         }
 
-        r = set_host_notifier_internal(proxy, qbus, n, false);
+        r = virtio_bus_set_host_notifier(qbus, n, false);
         assert(r >= 0);
     }
     return err;
@@ -2141,14 +2140,13 @@ int virtio_device_start_ioeventfd(VirtIODevice *vdev)
 static void virtio_device_stop_ioeventfd_impl(VirtIODevice *vdev)
 {
     VirtioBusState *qbus = VIRTIO_BUS(qdev_get_parent_bus(DEVICE(vdev)));
-    DeviceState *proxy = DEVICE(BUS(qbus)->parent);
     int n, r;
 
     for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
         if (!virtio_queue_get_num(vdev, n)) {
             continue;
         }
-        r = set_host_notifier_internal(proxy, qbus, n, false);
+        r = virtio_bus_set_host_notifier(qbus, n, false);
         assert(r >= 0);
     }
 }
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index cbdf745..fdf7fda 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -134,9 +134,4 @@ void virtio_bus_stop_ioeventfd(VirtioBusState *bus);
 /* Switch from/to the generic ioeventfd handler */
 int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign);
 
-/* This is temporary.  It is only needed because virtio_bus_set_host_notifier
- * sets ioeventfd_disabled but we will shortly get rid of it.  */
-int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
-                               int n, bool assign);
-
 #endif /* VIRTIO_BUS_H */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 11/13] virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:39 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> ioeventfd_disabled was the only reason for the default
> implementation of virtio_device_start_ioeventfd_impl not to use
> virtio_bus_set_host_notifier.  This is now fixed, and the sole entry
> point to set up ioeventfd can now be virtio_bus_set_host_notifier.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio/virtio-bus.c         | 4 ++--
>  hw/virtio/virtio.c             | 8 +++-----
>  include/hw/virtio/virtio-bus.h | 5 -----
>  3 files changed, 5 insertions(+), 12 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH 12/13] virtio: inline virtio_queue_set_host_notifier_fd_handler

[Paolo Bonzini]

Of the three possible parameter combinations for
virtio_queue_set_host_notifier_fd_handler:

- assign=true/set_handler=true is only called from
  virtio_device_start_ioeventfd

- assign=false/set_handler=false is called from
  set_host_notifier_internal but it only does something when
  reached from virtio_device_stop_ioeventfd_impl; otherwise
  there is no EventNotifier set on qemu_get_aio_context().

- assign=true/set_handler=false is called from
  set_host_notifier_internal, but it is not doing anything:
  with the new start_ioeventfd and stop_ioeventfd methods,
  there is never an EventNotifier set on qemu_get_aio_context()
  at this point.  This is enforced by the assertion in
  virtio_bus_set_host_notifier.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/virtio-bus.c     | 19 +++++++++++--------
 hw/virtio/virtio.c         | 27 +++++++++------------------
 include/hw/virtio/virtio.h |  3 +--
 3 files changed, 21 insertions(+), 28 deletions(-)

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index d6feaaf..96b7e76 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -163,19 +163,22 @@ static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
                          __func__, strerror(-r), r);
             return r;
         }
-        virtio_queue_set_host_notifier_fd_handler(vq, true, false);
-        r = k->ioeventfd_assign(proxy, notifier, n, assign);
+        r = k->ioeventfd_assign(proxy, notifier, n, true);
         if (r < 0) {
             error_report("%s: unable to assign ioeventfd: %d", __func__, r);
-            virtio_queue_set_host_notifier_fd_handler(vq, false, false);
-            event_notifier_cleanup(notifier);
-            return r;
+            goto cleanup_event_notifier;
         }
+        return 0;
     } else {
-        k->ioeventfd_assign(proxy, notifier, n, assign);
-        virtio_queue_set_host_notifier_fd_handler(vq, false, false);
-        event_notifier_cleanup(notifier);
+        k->ioeventfd_assign(proxy, notifier, n, false);
     }
+
+cleanup_event_notifier:
+    /* Test and clear notifier before after disabling event,
+     * in case poll callback didn't have time to run.
+     */
+    virtio_queue_host_notifier_read(notifier);
+    event_notifier_cleanup(notifier);
     return r;
 }
 
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 9a10d86..a771555 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1993,7 +1993,7 @@ void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
     }
 }
 
-static void virtio_queue_host_notifier_read(EventNotifier *n)
+void virtio_queue_host_notifier_read(EventNotifier *n)
 {
     VirtQueue *vq = container_of(n, VirtQueue, host_notifier);
     if (event_notifier_test_and_clear(n)) {
@@ -2001,22 +2001,6 @@ static void virtio_queue_host_notifier_read(EventNotifier *n)
     }
 }
 
-void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
-                                               bool set_handler)
-{
-    if (assign && set_handler) {
-        event_notifier_set_handler(&vq->host_notifier, true,
-                                   virtio_queue_host_notifier_read);
-    } else {
-        event_notifier_set_handler(&vq->host_notifier, true, NULL);
-    }
-    if (!assign) {
-        /* Test and clear notifier before after disabling event,
-         * in case poll callback didn't have time to run. */
-        virtio_queue_host_notifier_read(&vq->host_notifier);
-    }
-}
-
 EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq)
 {
     return &vq->host_notifier;
@@ -2096,6 +2080,7 @@ static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
     int n, r, err;
 
     for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
+        VirtQueue *vq = &vdev->vq[n];
         if (!virtio_queue_get_num(vdev, n)) {
             continue;
         }
@@ -2104,7 +2089,8 @@ static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
             err = r;
             goto assign_error;
         }
-        virtio_queue_set_host_notifier_fd_handler(&vdev->vq[n], true, true);
+        event_notifier_set_handler(&vq->host_notifier, true,
+                                   virtio_queue_host_notifier_read);
     }
 
     for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
@@ -2119,10 +2105,12 @@ static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
 
 assign_error:
     while (--n >= 0) {
+        VirtQueue *vq = &vdev->vq[n];
         if (!virtio_queue_get_num(vdev, n)) {
             continue;
         }
 
+        event_notifier_set_handler(&vq->host_notifier, true, NULL);
         r = virtio_bus_set_host_notifier(qbus, n, false);
         assert(r >= 0);
     }
@@ -2143,9 +2131,12 @@ static void virtio_device_stop_ioeventfd_impl(VirtIODevice *vdev)
     int n, r;
 
     for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
+        VirtQueue *vq = &vdev->vq[n];
+
         if (!virtio_queue_get_num(vdev, n)) {
             continue;
         }
+        event_notifier_set_handler(&vq->host_notifier, true, NULL);
         r = virtio_bus_set_host_notifier(qbus, n, false);
         assert(r >= 0);
     }
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 89586cb..a94b6cb 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -277,8 +277,7 @@ int virtio_device_start_ioeventfd(VirtIODevice *vdev);
 void virtio_device_stop_ioeventfd(VirtIODevice *vdev);
 bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev);
 EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
-void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
-                                               bool set_handler);
+void virtio_queue_host_notifier_read(EventNotifier *n);
 void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
                                                 void (*fn)(VirtIODevice *,
                                                            VirtQueue *));
-- 
2.7.4

Re: [Qemu-devel] [PATCH 12/13] virtio: inline virtio_queue_set_host_notifier_fd_handler

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:40 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Of the three possible parameter combinations for
> virtio_queue_set_host_notifier_fd_handler:
> 
> - assign=true/set_handler=true is only called from
>   virtio_device_start_ioeventfd
> 
> - assign=false/set_handler=false is called from
>   set_host_notifier_internal but it only does something when
>   reached from virtio_device_stop_ioeventfd_impl; otherwise
>   there is no EventNotifier set on qemu_get_aio_context().
> 
> - assign=true/set_handler=false is called from
>   set_host_notifier_internal, but it is not doing anything:
>   with the new start_ioeventfd and stop_ioeventfd methods,
>   there is never an EventNotifier set on qemu_get_aio_context()
>   at this point.  This is enforced by the assertion in
>   virtio_bus_set_host_notifier.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio/virtio-bus.c     | 19 +++++++++++--------
>  hw/virtio/virtio.c         | 27 +++++++++------------------
>  include/hw/virtio/virtio.h |  3 +--
>  3 files changed, 21 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index d6feaaf..96b7e76 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -163,19 +163,22 @@ static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
>                           __func__, strerror(-r), r);
>              return r;
>          }
> -        virtio_queue_set_host_notifier_fd_handler(vq, true, false);
> -        r = k->ioeventfd_assign(proxy, notifier, n, assign);
> +        r = k->ioeventfd_assign(proxy, notifier, n, true);
>          if (r < 0) {
>              error_report("%s: unable to assign ioeventfd: %d", __func__, r);
> -            virtio_queue_set_host_notifier_fd_handler(vq, false, false);
> -            event_notifier_cleanup(notifier);
> -            return r;
> +            goto cleanup_event_notifier;
>          }
> +        return 0;
>      } else {
> -        k->ioeventfd_assign(proxy, notifier, n, assign);
> -        virtio_queue_set_host_notifier_fd_handler(vq, false, false);
> -        event_notifier_cleanup(notifier);
> +        k->ioeventfd_assign(proxy, notifier, n, false);
>      }
> +
> +cleanup_event_notifier:
> +    /* Test and clear notifier before after disabling event,

Now is the time to get rid of "before after" :)

> +     * in case poll callback didn't have time to run.
> +     */
> +    virtio_queue_host_notifier_read(notifier);
> +    event_notifier_cleanup(notifier);
>      return r;
>  }

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH 13/13] virtio: inline set_host_notifier_internal

[Paolo Bonzini]

This is only called from virtio_bus_set_host_notifier.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        v1->v2: rebase after commit a8bba0a ("virtio: Tell the user what went
        wrong when event_notifier_init failed", 2016-09-09)

 hw/virtio/virtio-bus.c | 62 +++++++++++++++++++++-----------------------------
 1 file changed, 26 insertions(+), 36 deletions(-)

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 96b7e76..dc45b78 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -147,41 +147,6 @@ void virtio_bus_set_vdev_config(VirtioBusState *bus, uint8_t *config)
     }
 }
 
-static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
-                                      int n, bool assign)
-{
-    VirtIODevice *vdev = virtio_bus_get_device(bus);
-    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
-    VirtQueue *vq = virtio_get_queue(vdev, n);
-    EventNotifier *notifier = virtio_queue_get_host_notifier(vq);
-    int r = 0;
-
-    if (assign) {
-        r = event_notifier_init(notifier, 1);
-        if (r < 0) {
-            error_report("%s: unable to init event notifier: %s (%d)",
-                         __func__, strerror(-r), r);
-            return r;
-        }
-        r = k->ioeventfd_assign(proxy, notifier, n, true);
-        if (r < 0) {
-            error_report("%s: unable to assign ioeventfd: %d", __func__, r);
-            goto cleanup_event_notifier;
-        }
-        return 0;
-    } else {
-        k->ioeventfd_assign(proxy, notifier, n, false);
-    }
-
-cleanup_event_notifier:
-    /* Test and clear notifier before after disabling event,
-     * in case poll callback didn't have time to run.
-     */
-    virtio_queue_host_notifier_read(notifier);
-    event_notifier_cleanup(notifier);
-    return r;
-}
-
 int virtio_bus_start_ioeventfd(VirtioBusState *bus)
 {
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
@@ -231,20 +196,45 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus)
  */
 int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
 {
+    VirtIODevice *vdev = virtio_bus_get_device(bus);
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
     DeviceState *proxy = DEVICE(BUS(bus)->parent);
+    VirtQueue *vq = virtio_get_queue(vdev, n);
+    EventNotifier *notifier = virtio_queue_get_host_notifier(vq);
+    int r = 0;
 
     if (!k->ioeventfd_assign) {
         return -ENOSYS;
     }
+
     if (assign) {
         assert(!bus->ioeventfd_started);
+        r = event_notifier_init(notifier, 1);
+        if (r < 0) {
+            error_report("%s: unable to init event notifier: %s (%d)",
+                         __func__, strerror(-r), r);
+            return r;
+        }
+        r = k->ioeventfd_assign(proxy, notifier, n, true);
+        if (r < 0) {
+            error_report("%s: unable to assign ioeventfd: %d", __func__, r);
+            goto cleanup_event_notifier;
+        }
+        return 0;
     } else {
         if (!bus->ioeventfd_started) {
             return 0;
         }
+        k->ioeventfd_assign(proxy, notifier, n, false);
     }
-    return set_host_notifier_internal(proxy, bus, n, assign);
+
+cleanup_event_notifier:
+    /* Test and clear notifier before after disabling event,
+     * in case poll callback didn't have time to run.
+     */
+    virtio_queue_host_notifier_read(notifier);
+    event_notifier_cleanup(notifier);
+    return r;
 }
 
 static char *virtio_bus_get_dev_path(DeviceState *dev)
-- 
2.7.4

Re: [Qemu-devel] [PATCH 13/13] virtio: inline set_host_notifier_internal

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:41 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This is only called from virtio_bus_set_host_notifier.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>         v1->v2: rebase after commit a8bba0a ("virtio: Tell the user what went
>         wrong when event_notifier_init failed", 2016-09-09)
> 
>  hw/virtio/virtio-bus.c | 62 +++++++++++++++++++++-----------------------------
>  1 file changed, 26 insertions(+), 36 deletions(-)
> 

> +cleanup_event_notifier:
> +    /* Test and clear notifier before after disabling event,

I always stumble over that "before after" :)

> +     * in case poll callback didn't have time to run.
> +     */
> +    virtio_queue_host_notifier_read(notifier);
> +    event_notifier_cleanup(notifier);
> +    return r;
>  }
> 
>  static char *virtio_bus_get_dev_path(DeviceState *dev)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:28 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This series started as an attempt to always use the dataplane path
> for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
> was three-fold:
> 
> 1) to add more coverage for dataplane
> 
> 2) to remove virtio_add_queue_aio
> 
> 3) to simplify the dataplane start/stop code
> 
> It achieves the first two objectives, and while it doesn't quite
> achieve the third it does cleanup the generic ioeventfd code in
> virtio-bus more than I expected.  In particular, it reduces the set
> of callbacks that transports must implement, and it removes the ugly
> case where ioeventfd is started with generic callbacks and then moved
> to the dataplane callbacks.  It also enables some simplification of the
> functions that deal with host notifiers, and detects some configuration
> errors better.
> 
> I've tested it with virtio-blk, virtio-scsi and vhost-net.

Do you have a branch somewhere?

> 
> Patch 1 is a bugfix that I found while testing the TCG+dataplane combo.
> 
> Patches 2 and 3 are simplifications that are too nice to leave
> them for later in the series.
> 
> Patch 4 moves some of the ioeventfd code from virtio-bus.c to
> virtio.c.  At this point the transition is a bit half-assed, but
> this changes as soon as we remove the generic->dataplane
> handler transition.
> 
> Patches 5 to 7 do exactly that, and then the spring cleaning
> begins, lasting for the whole second half of the series.

I'll continue to look at the patches from 4 on tomorrow.

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Paolo Bonzini]

----- Original Message -----
> From: "Cornelia Huck" <cornelia.huck@de.ibm.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, stefanha@redhat.com, borntraeger@de.ibm.com, famz@redhat.com, mst@redhat.com
> Sent: Tuesday, October 18, 2016 7:24:45 PM
> Subject: Re: [PATCH 00/12] virtio: cleanup ioeventfd start/stop
> 
> On Mon, 10 Oct 2016 13:53:28 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > This series started as an attempt to always use the dataplane path
> > for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
> > was three-fold:
> > 
> > 1) to add more coverage for dataplane
> > 
> > 2) to remove virtio_add_queue_aio
> > 
> > 3) to simplify the dataplane start/stop code
> > 
> > It achieves the first two objectives, and while it doesn't quite
> > achieve the third it does cleanup the generic ioeventfd code in
> > virtio-bus more than I expected.  In particular, it reduces the set
> > of callbacks that transports must implement, and it removes the ugly
> > case where ioeventfd is started with generic callbacks and then moved
> > to the dataplane callbacks.  It also enables some simplification of the
> > functions that deal with host notifiers, and detects some configuration
> > errors better.
> > 
> > I've tested it with virtio-blk, virtio-scsi and vhost-net.
> 
> Do you have a branch somewhere?

ioeventfd-virtio at git://github.com/bonzini/qemu.git

Paolo

> > 
> > Patch 1 is a bugfix that I found while testing the TCG+dataplane combo.
> > 
> > Patches 2 and 3 are simplifications that are too nice to leave
> > them for later in the series.
> > 
> > Patch 4 moves some of the ioeventfd code from virtio-bus.c to
> > virtio.c.  At this point the transition is a bit half-assed, but
> > this changes as soon as we remove the generic->dataplane
> > handler transition.
> > 
> > Patches 5 to 7 do exactly that, and then the spring cleaning
> > begins, lasting for the whole second half of the series.
> 
> I'll continue to look at the patches from 4 on tomorrow.
> 
> 

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:28 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This series started as an attempt to always use the dataplane path
> for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
> was three-fold:
> 
> 1) to add more coverage for dataplane
> 
> 2) to remove virtio_add_queue_aio
> 
> 3) to simplify the dataplane start/stop code
> 
> It achieves the first two objectives, and while it doesn't quite
> achieve the third it does cleanup the generic ioeventfd code in
> virtio-bus more than I expected.  In particular, it reduces the set
> of callbacks that transports must implement, and it removes the ugly
> case where ioeventfd is started with generic callbacks and then moved
> to the dataplane callbacks.  It also enables some simplification of the
> functions that deal with host notifiers, and detects some configuration
> errors better.
> 
> I've tested it with virtio-blk, virtio-scsi and vhost-net.

Hm. 'make check' on a s390 host faults in check-qtest-aarch64 (your
branch rebased to current master; master itself is fine). I'll see if I
can find out more (probably later today).

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Cornelia Huck]

On Wed, 19 Oct 2016 14:17:59 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Mon, 10 Oct 2016 13:53:28 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > This series started as an attempt to always use the dataplane path
> > for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
> > was three-fold:
> > 
> > 1) to add more coverage for dataplane
> > 
> > 2) to remove virtio_add_queue_aio
> > 
> > 3) to simplify the dataplane start/stop code
> > 
> > It achieves the first two objectives, and while it doesn't quite
> > achieve the third it does cleanup the generic ioeventfd code in
> > virtio-bus more than I expected.  In particular, it reduces the set
> > of callbacks that transports must implement, and it removes the ugly
> > case where ioeventfd is started with generic callbacks and then moved
> > to the dataplane callbacks.  It also enables some simplification of the
> > functions that deal with host notifiers, and detects some configuration
> > errors better.
> > 
> > I've tested it with virtio-blk, virtio-scsi and vhost-net.
> 
> Hm. 'make check' on a s390 host faults in check-qtest-aarch64 (your
> branch rebased to current master; master itself is fine). I'll see if I
> can find out more (probably later today).

I've bisected this to patch 4 ("virtio: add start_ioeventfd and
stop_ioeventfd to VirtioDeviceClass").

More details of the failure:
- host is s390x with Fedora 23
- # Configured with: '../configure' '--target-list=s390x-softmmu s390x-linux-user aarch64-softmmu' '--enable-kvm' '--enable-vhost-net' '--enable-linux-aio'
- dmesg has:
[ 1774.006703] User process fault: interruption code 0010 ilc:3 in qemu-system-aarch64[10000000+7b9000]
[ 1774.006720] Failing address: 0000000000000000 TEID: 0000000000000800
[ 1774.006723] Fault in primary space mode while using user ASCE.
[ 1774.006731] AS:00000000373ac1c7 R3:0000000048148007 S:0000000000000020 
[ 1774.006739] CPU: 1 PID: 24183 Comm: qemu-system-aar Not tainted 4.8.0-20161019.0.eb97ed0.8b99dbe.fc23.s390xdefault #1
[ 1774.006742] Hardware name: IBM              2827 H43              738              (LPAR)
[ 1774.006745] task: 000000004ccb8008 task.stack: 00000000355e0000
[ 1774.006748] User PSW : 0705000180000000 0000000010332380
[ 1774.006753]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:1 AS:0 CC:0 PM:0 RI:0 EA:3
[ 1774.006757] User GPRS: 0000000000000003 0000000000000000 0000000000000000 0000000000000001
[ 1774.006761]            00000000102e1740 0000000000000178 000000001069b2ac 00000000101f8278
[ 1774.006764]            0000000000000000 0000000047471ee0 0000000000000000 00000000101f6600
[ 1774.006766]            0000000000000000 000000004795f788 00000000102e14a0 000003ffe307e988
[ 1774.006774] User Code: 000000001033237a: 07fe		bcr	15,%r14
                          000000001033237c: 0707		bcr	0,%r7
                         #000000001033237e: 0707		bcr	0,%r7
                         >0000000010332380: e32020000004	lg	%r2,0(%r2)
                          0000000010332386: 07fe		bcr	15,%r14
                          0000000010332388: e31020000004	lg	%r1,0(%r2)
                          000000001033238e: e32010500090	llgc	%r2,80(%r1)
                          0000000010332394: 07fe		bcr	15,%r14
[ 1774.006812] Last Breaking-Event-Address:
[ 1774.006816]  [<00000000102e149a>] 0x102e149a

(once for each failure)
- failures are in qom-test for aarch64:
TEST: tests/qom-test... (pid=23997)
  /aarch64/qom/integratorcp:                                           OK
  /aarch64/qom/nuri:                                                   OK
  /aarch64/qom/verdex:                                                 OK
  /aarch64/qom/ast2500-evb:                                            OK
  /aarch64/qom/smdkc210:                                               OK
  /aarch64/qom/collie:                                                 OK
  /aarch64/qom/imx25-pdk:                                              OK
  /aarch64/qom/none:                                                   OK
  /aarch64/qom/spitz:                                                  OK
  /aarch64/qom/realview-pbx-a9:                                        OK
  /aarch64/qom/realview-eb:                                            OK
  /aarch64/qom/versatilepb:                                            OK
  /aarch64/qom/realview-pb-a8:                                         OK
  /aarch64/qom/musicpal:                                               OK
  /aarch64/qom/z2:                                                     OK
  /aarch64/qom/akita:                                                  OK
  /aarch64/qom/virt-2.7:                                               Broken pipe
FAIL
GTester: last random seed: R02Saec62eb6f9ebd3e5bfcbf42d0aaf165a
(pid=24053)
  /aarch64/qom/kzm:                                                    OK
  /aarch64/qom/virt-2.8:                                               Broken pipe
FAIL
GTester: last random seed: R02S3472a1653451d1812262f7d72624492e
(pid=24063)
  /aarch64/qom/realview-eb-mpcore:                                     OK
  /aarch64/qom/sx1:                                                    OK
  /aarch64/qom/sx1-v1:                                                 OK
  /aarch64/qom/virt-2.6:                                               Broken pipe
FAIL
GTester: last random seed: R02Se4b753c5be66c0ef7870bebcca8771f8
(pid=24098)
  /aarch64/qom/cubieboard:                                             OK
  /aarch64/qom/highbank:                                               OK
  /aarch64/qom/raspi2:                                                 OK
  /aarch64/qom/netduino2:                                              OK
  /aarch64/qom/terrier:                                                OK
  /aarch64/qom/n810:                                                   OK
  /aarch64/qom/mainstone:                                              OK
  /aarch64/qom/palmetto-bmc:                                           OK
  /aarch64/qom/sabrelite:                                              OK
  /aarch64/qom/midway:                                                 OK
  /aarch64/qom/cheetah:                                                OK
  /aarch64/qom/tosa:                                                   OK
  /aarch64/qom/borzoi:                                                 OK
  /aarch64/qom/versatileab:                                            OK
  /aarch64/qom/lm3s6965evb:                                            OK
  /aarch64/qom/n800:                                                   OK
  /aarch64/qom/connex:                                                 OK
  /aarch64/qom/xilinx-zynq-a9:                                         OK
  /aarch64/qom/xlnx-ep108:                                             OK
  /aarch64/qom/vexpress-a9:                                            Broken pipe
FAIL
GTester: last random seed: R02Sdf4aceaaef3ceb060fd5996ecfd05bbb
(pid=24180)
  /aarch64/qom/vexpress-a15:                                           Broken pipe
FAIL
GTester: last random seed: R02Sdf4de27065ea3baf0b2acc109af636b8
(pid=24187)
  /aarch64/qom/xlnx-zcu102:                                            OK
  /aarch64/qom/canon-a1100:                                            OK
  /aarch64/qom/lm3s811evb:                                             OK
FAIL: tests/qom-test

Do these boards maybe have something interesting in common?

No further time to look into this today, sorry.

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Paolo Bonzini]

On 19/10/2016 17:38, Cornelia Huck wrote:
> - failures are in qom-test for aarch64:
> TEST: tests/qom-test... (pid=23997)
>   /aarch64/qom/integratorcp:                                           OK
>   /aarch64/qom/nuri:                                                   OK
>   /aarch64/qom/verdex:                                                 OK
>   /aarch64/qom/ast2500-evb:                                            OK
>   /aarch64/qom/smdkc210:                                               OK
>   /aarch64/qom/collie:                                                 OK
>   /aarch64/qom/imx25-pdk:                                              OK
>   /aarch64/qom/none:                                                   OK
>   /aarch64/qom/spitz:                                                  OK
>   /aarch64/qom/realview-pbx-a9:                                        OK
>   /aarch64/qom/realview-eb:                                            OK
>   /aarch64/qom/versatilepb:                                            OK
>   /aarch64/qom/realview-pb-a8:                                         OK
>   /aarch64/qom/musicpal:                                               OK
>   /aarch64/qom/z2:                                                     OK
>   /aarch64/qom/akita:                                                  OK
>   /aarch64/qom/virt-2.7:                                               Broken pipe
> FAIL
> GTester: last random seed: R02Saec62eb6f9ebd3e5bfcbf42d0aaf165a
> (pid=24053)
>   /aarch64/qom/kzm:                                                    OK
>   /aarch64/qom/virt-2.8:                                               Broken pipe
> FAIL
> GTester: last random seed: R02S3472a1653451d1812262f7d72624492e
> (pid=24063)
>   /aarch64/qom/realview-eb-mpcore:                                     OK
>   /aarch64/qom/sx1:                                                    OK
>   /aarch64/qom/sx1-v1:                                                 OK
>   /aarch64/qom/virt-2.6:                                               Broken pipe
> FAIL
> GTester: last random seed: R02Se4b753c5be66c0ef7870bebcca8771f8
> (pid=24098)
>   /aarch64/qom/cubieboard:                                             OK
>   /aarch64/qom/highbank:                                               OK
>   /aarch64/qom/raspi2:                                                 OK
>   /aarch64/qom/netduino2:                                              OK
>   /aarch64/qom/terrier:                                                OK
>   /aarch64/qom/n810:                                                   OK
>   /aarch64/qom/mainstone:                                              OK
>   /aarch64/qom/palmetto-bmc:                                           OK
>   /aarch64/qom/sabrelite:                                              OK
>   /aarch64/qom/midway:                                                 OK
>   /aarch64/qom/cheetah:                                                OK
>   /aarch64/qom/tosa:                                                   OK
>   /aarch64/qom/borzoi:                                                 OK
>   /aarch64/qom/versatileab:                                            OK
>   /aarch64/qom/lm3s6965evb:                                            OK
>   /aarch64/qom/n800:                                                   OK
>   /aarch64/qom/connex:                                                 OK
>   /aarch64/qom/xilinx-zynq-a9:                                         OK
>   /aarch64/qom/xlnx-ep108:                                             OK
>   /aarch64/qom/vexpress-a9:                                            Broken pipe
> FAIL
> GTester: last random seed: R02Sdf4aceaaef3ceb060fd5996ecfd05bbb
> (pid=24180)
>   /aarch64/qom/vexpress-a15:                                           Broken pipe
> FAIL
> GTester: last random seed: R02Sdf4de27065ea3baf0b2acc109af636b8
> (pid=24187)
>   /aarch64/qom/xlnx-zcu102:                                            OK
>   /aarch64/qom/canon-a1100:                                            OK
>   /aarch64/qom/lm3s811evb:                                             OK
> FAIL: tests/qom-test
> 
> Do these boards maybe have something interesting in common?
> 
> No further time to look into this today, sorry.

They use virtio-mmio, probably.  I'll look at it tomorrow.

The s390 spew is an abort(), isn't it?
Paolo

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Cornelia Huck]

On Wed, 19 Oct 2016 22:44:18 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 19/10/2016 17:38, Cornelia Huck wrote:
> > - failures are in qom-test for aarch64:
> > TEST: tests/qom-test... (pid=23997)
> >   /aarch64/qom/integratorcp:                                           OK
> >   /aarch64/qom/nuri:                                                   OK
> >   /aarch64/qom/verdex:                                                 OK
> >   /aarch64/qom/ast2500-evb:                                            OK
> >   /aarch64/qom/smdkc210:                                               OK
> >   /aarch64/qom/collie:                                                 OK
> >   /aarch64/qom/imx25-pdk:                                              OK
> >   /aarch64/qom/none:                                                   OK
> >   /aarch64/qom/spitz:                                                  OK
> >   /aarch64/qom/realview-pbx-a9:                                        OK
> >   /aarch64/qom/realview-eb:                                            OK
> >   /aarch64/qom/versatilepb:                                            OK
> >   /aarch64/qom/realview-pb-a8:                                         OK
> >   /aarch64/qom/musicpal:                                               OK
> >   /aarch64/qom/z2:                                                     OK
> >   /aarch64/qom/akita:                                                  OK
> >   /aarch64/qom/virt-2.7:                                               Broken pipe
> > FAIL
> > GTester: last random seed: R02Saec62eb6f9ebd3e5bfcbf42d0aaf165a
> > (pid=24053)
> >   /aarch64/qom/kzm:                                                    OK
> >   /aarch64/qom/virt-2.8:                                               Broken pipe
> > FAIL
> > GTester: last random seed: R02S3472a1653451d1812262f7d72624492e
> > (pid=24063)
> >   /aarch64/qom/realview-eb-mpcore:                                     OK
> >   /aarch64/qom/sx1:                                                    OK
> >   /aarch64/qom/sx1-v1:                                                 OK
> >   /aarch64/qom/virt-2.6:                                               Broken pipe
> > FAIL
> > GTester: last random seed: R02Se4b753c5be66c0ef7870bebcca8771f8
> > (pid=24098)
> >   /aarch64/qom/cubieboard:                                             OK
> >   /aarch64/qom/highbank:                                               OK
> >   /aarch64/qom/raspi2:                                                 OK
> >   /aarch64/qom/netduino2:                                              OK
> >   /aarch64/qom/terrier:                                                OK
> >   /aarch64/qom/n810:                                                   OK
> >   /aarch64/qom/mainstone:                                              OK
> >   /aarch64/qom/palmetto-bmc:                                           OK
> >   /aarch64/qom/sabrelite:                                              OK
> >   /aarch64/qom/midway:                                                 OK
> >   /aarch64/qom/cheetah:                                                OK
> >   /aarch64/qom/tosa:                                                   OK
> >   /aarch64/qom/borzoi:                                                 OK
> >   /aarch64/qom/versatileab:                                            OK
> >   /aarch64/qom/lm3s6965evb:                                            OK
> >   /aarch64/qom/n800:                                                   OK
> >   /aarch64/qom/connex:                                                 OK
> >   /aarch64/qom/xilinx-zynq-a9:                                         OK
> >   /aarch64/qom/xlnx-ep108:                                             OK
> >   /aarch64/qom/vexpress-a9:                                            Broken pipe
> > FAIL
> > GTester: last random seed: R02Sdf4aceaaef3ceb060fd5996ecfd05bbb
> > (pid=24180)
> >   /aarch64/qom/vexpress-a15:                                           Broken pipe
> > FAIL
> > GTester: last random seed: R02Sdf4de27065ea3baf0b2acc109af636b8
> > (pid=24187)
> >   /aarch64/qom/xlnx-zcu102:                                            OK
> >   /aarch64/qom/canon-a1100:                                            OK
> >   /aarch64/qom/lm3s811evb:                                             OK
> > FAIL: tests/qom-test
> > 
> > Do these boards maybe have something interesting in common?
> > 
> > No further time to look into this today, sorry.
> 
> They use virtio-mmio, probably.  I'll look at it tomorrow.
> 
> The s390 spew is an abort(), isn't it?
> Paolo
> 

virtio-mmio looks like the best candidate (maybe the eventfd is not
stopped properly and writes happen where they are not supposed to
anymore?) But staring at the patch didn't make anything jump out at
me...

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Paolo Bonzini]

On 20/10/2016 11:03, Cornelia Huck wrote:
> On Wed, 19 Oct 2016 22:44:18 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> On 19/10/2016 17:38, Cornelia Huck wrote:
>>> - failures are in qom-test for aarch64:
>>> TEST: tests/qom-test... (pid=23997)
>>>   /aarch64/qom/integratorcp:                                           OK
>>>   /aarch64/qom/nuri:                                                   OK
>>>   /aarch64/qom/verdex:                                                 OK
>>>   /aarch64/qom/ast2500-evb:                                            OK
>>>   /aarch64/qom/smdkc210:                                               OK
>>>   /aarch64/qom/collie:                                                 OK
>>>   /aarch64/qom/imx25-pdk:                                              OK
>>>   /aarch64/qom/none:                                                   OK
>>>   /aarch64/qom/spitz:                                                  OK
>>>   /aarch64/qom/realview-pbx-a9:                                        OK
>>>   /aarch64/qom/realview-eb:                                            OK
>>>   /aarch64/qom/versatilepb:                                            OK
>>>   /aarch64/qom/realview-pb-a8:                                         OK
>>>   /aarch64/qom/musicpal:                                               OK
>>>   /aarch64/qom/z2:                                                     OK
>>>   /aarch64/qom/akita:                                                  OK
>>>   /aarch64/qom/virt-2.7:                                               Broken pipe
>>> FAIL
>>> GTester: last random seed: R02Saec62eb6f9ebd3e5bfcbf42d0aaf165a
>>> (pid=24053)
>>>   /aarch64/qom/kzm:                                                    OK
>>>   /aarch64/qom/virt-2.8:                                               Broken pipe
>>> FAIL
>>> GTester: last random seed: R02S3472a1653451d1812262f7d72624492e
>>> (pid=24063)
>>>   /aarch64/qom/realview-eb-mpcore:                                     OK
>>>   /aarch64/qom/sx1:                                                    OK
>>>   /aarch64/qom/sx1-v1:                                                 OK
>>>   /aarch64/qom/virt-2.6:                                               Broken pipe
>>> FAIL
>>> GTester: last random seed: R02Se4b753c5be66c0ef7870bebcca8771f8
>>> (pid=24098)
>>>   /aarch64/qom/cubieboard:                                             OK
>>>   /aarch64/qom/highbank:                                               OK
>>>   /aarch64/qom/raspi2:                                                 OK
>>>   /aarch64/qom/netduino2:                                              OK
>>>   /aarch64/qom/terrier:                                                OK
>>>   /aarch64/qom/n810:                                                   OK
>>>   /aarch64/qom/mainstone:                                              OK
>>>   /aarch64/qom/palmetto-bmc:                                           OK
>>>   /aarch64/qom/sabrelite:                                              OK
>>>   /aarch64/qom/midway:                                                 OK
>>>   /aarch64/qom/cheetah:                                                OK
>>>   /aarch64/qom/tosa:                                                   OK
>>>   /aarch64/qom/borzoi:                                                 OK
>>>   /aarch64/qom/versatileab:                                            OK
>>>   /aarch64/qom/lm3s6965evb:                                            OK
>>>   /aarch64/qom/n800:                                                   OK
>>>   /aarch64/qom/connex:                                                 OK
>>>   /aarch64/qom/xilinx-zynq-a9:                                         OK
>>>   /aarch64/qom/xlnx-ep108:                                             OK
>>>   /aarch64/qom/vexpress-a9:                                            Broken pipe
>>> FAIL
>>> GTester: last random seed: R02Sdf4aceaaef3ceb060fd5996ecfd05bbb
>>> (pid=24180)
>>>   /aarch64/qom/vexpress-a15:                                           Broken pipe
>>> FAIL
>>> GTester: last random seed: R02Sdf4de27065ea3baf0b2acc109af636b8
>>> (pid=24187)
>>>   /aarch64/qom/xlnx-zcu102:                                            OK
>>>   /aarch64/qom/canon-a1100:                                            OK
>>>   /aarch64/qom/lm3s811evb:                                             OK
>>> FAIL: tests/qom-test
>>>
>>> Do these boards maybe have something interesting in common?
>>>
>>> No further time to look into this today, sorry.
>>
>> They use virtio-mmio, probably.  I'll look at it tomorrow.
>>
>> The s390 spew is an abort(), isn't it?
>> Paolo
>>
> 
> virtio-mmio looks like the best candidate (maybe the eventfd is not
> stopped properly and writes happen where they are not supposed to
> anymore?) But staring at the patch didn't make anything jump out at
> me...

No, it's because virtio-mmio can be created without a device 
underneath.  virtio_bus_start_ioeventfd in that case is wrong, but 
virtio_bus_stop_ioeventfd should be a no-op.  The fix is trivial:

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 0479704..bf61f66 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -172,12 +172,15 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus)
 
 void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
 {
-    VirtIODevice *vdev = virtio_bus_get_device(bus);
-    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
+    VirtIODevice *vdev;
+    VirtioDeviceClass *vdc;
 
     if (!bus->ioeventfd_started) {
         return;
     }
+
+    vdev = virtio_bus_get_device(bus);
+    vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
     vdc->stop_ioeventfd(vdev);
     bus->ioeventfd_started = false;
 }


I'll post v3 tomorrow.

Paolo

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Cornelia Huck]

On Thu, 20 Oct 2016 18:53:31 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> No, it's because virtio-mmio can be created without a device 
> underneath.  virtio_bus_start_ioeventfd in that case is wrong, but 
> virtio_bus_stop_ioeventfd should be a no-op.  The fix is trivial:

I tend to forget this virtio-mmio speciality...

> 
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 0479704..bf61f66 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -172,12 +172,15 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus)
> 
>  void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
>  {
> -    VirtIODevice *vdev = virtio_bus_get_device(bus);
> -    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> +    VirtIODevice *vdev;
> +    VirtioDeviceClass *vdc;
> 
>      if (!bus->ioeventfd_started) {
>          return;
>      }
> +
> +    vdev = virtio_bus_get_device(bus);
> +    vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
>      vdc->stop_ioeventfd(vdev);
>      bus->ioeventfd_started = false;
>  }

Looks sane, and make check passes with this applied.

[Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop

[Paolo Bonzini]

This series started as an attempt to always use the dataplane path
for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
was three-fold:

1) to add more coverage for dataplane

2) to remove virtio_add_queue_aio

3) to simplify the dataplane start/stop code

It achieves the first two objectives, and while it doesn't quite
achieve the third it does cleanup the generic ioeventfd code in
virtio-bus more than I expected.  In particular, it reduces the set
of callbacks that transports must implement, and it removes the ugly
case where ioeventfd is started with generic callbacks and then moved
to the dataplane callbacks.  It also enables some simplification of the
functions that deal with host notifiers, and detects some configuration
errors better.

I've tested it with virtio-blk, virtio-scsi and vhost-net.

Patch 1 is a bugfix that I found while testing the TCG+dataplane combo.

Patches 2 and 3 are simplifications that are too nice to leave
them for later in the series.

Patch 4 moves some of the ioeventfd code from virtio-bus.c to
virtio.c.  At this point the transition is a bit half-assed, but
this changes as soon as we remove the generic->dataplane
handler transition.

Patches 5 to 7 do exactly that, and then the spring cleaning
begins, lasting for the whole second half of the series.

Thanks,

Paolo

        v2->v3: fix patch 4 to avoid virtio-mmio failures [Cornelia]
                fix patch 5 NULL-check-after-dereference [Cornelia]
                fix "before after" in comment [Cornelia]

Paolo Bonzini (13):
  virtio: disable ioeventfd as early as possible
  virtio: move ioeventfd_disabled flag to VirtioBusState
  virtio: move ioeventfd_started flag to VirtioBusState
  virtio: add start_ioeventfd and stop_ioeventfd to VirtioDeviceClass
  virtio: introduce virtio_device_ioeventfd_enabled
  virtio-blk: always use dataplane path if ioeventfd is active
  virtio-scsi: always use dataplane path if ioeventfd is active
  Revert "virtio: Introduce virtio_add_queue_aio"
  virtio: remove set_handler argument from set_host_notifier_internal
  virtio: remove ioeventfd_disabled altogether
  virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd
  virtio: inline virtio_queue_set_host_notifier_fd_handler
  virtio: inline set_host_notifier_internal

 hw/block/dataplane/virtio-blk.c |  73 +++++++++++--------
 hw/block/dataplane/virtio-blk.h |   6 +-
 hw/block/virtio-blk.c           |  15 ++--
 hw/s390x/virtio-ccw.c           |  44 ++----------
 hw/s390x/virtio-ccw.h           |   2 -
 hw/scsi/virtio-scsi-dataplane.c |  56 +++++++++------
 hw/scsi/virtio-scsi.c           |  24 +++----
 hw/virtio/vhost.c               |   5 +-
 hw/virtio/virtio-bus.c          | 154 ++++++++++++++++------------------------
 hw/virtio/virtio-mmio.c         |  35 +--------
 hw/virtio/virtio-pci.c          |  40 ++---------
 hw/virtio/virtio-pci.h          |   2 -
 hw/virtio/virtio.c              | 139 +++++++++++++++++++++++-------------
 include/hw/virtio/virtio-bus.h  |  27 ++++---
 include/hw/virtio/virtio-scsi.h |   6 +-
 include/hw/virtio/virtio.h      |  11 +--
 16 files changed, 292 insertions(+), 347 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 10/13] virtio: remove ioeventfd_disabled altogether

[Paolo Bonzini]

Now that there is not anymore a switch from the generic ioeventfd handler
to the dataplane handler, virtio_bus_set_host_notifier(assign=true) is
always called with !bus->ioeventfd_started, hence virtio_bus_stop_ioeventfd
does nothing in this case.  Move the invocation to vhost.c, which is the
only place that needs it.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/vhost.c              |  3 +++
 hw/virtio/virtio-bus.c         | 23 ++++++++---------------
 include/hw/virtio/virtio-bus.h |  6 ------
 3 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 501a5f4..131f164 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1196,6 +1196,7 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
         goto fail;
     }
 
+    virtio_device_stop_ioeventfd(vdev);
     for (i = 0; i < hdev->nvqs; ++i) {
         r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
                                          true);
@@ -1215,6 +1216,7 @@ fail_vq:
         }
         assert (e >= 0);
     }
+    virtio_device_start_ioeventfd(vdev);
 fail:
     return r;
 }
@@ -1237,6 +1239,7 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
         }
         assert (r >= 0);
     }
+    virtio_device_start_ioeventfd(vdev);
 }
 
 /* Test and clear event pending status.
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index a619445..b0e4544 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -190,7 +190,7 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus)
     if (!k->ioeventfd_assign || !k->ioeventfd_enabled(proxy)) {
         return -ENOSYS;
     }
-    if (bus->ioeventfd_started || bus->ioeventfd_disabled) {
+    if (bus->ioeventfd_started) {
         return 0;
     }
     r = vdc->start_ioeventfd(vdev);
@@ -226,8 +226,8 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus)
 }
 
 /*
- * This function switches from/to the generic ioeventfd handler.
- * assign==false means 'use generic ioeventfd handler'.
+ * This function switches ioeventfd on/off in the device.
+ * The caller must set or clear the handlers for the EventNotifier.
  */
 int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
 {
@@ -237,19 +237,12 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
     if (!k->ioeventfd_assign) {
         return -ENOSYS;
     }
-    bus->ioeventfd_disabled = assign;
     if (assign) {
-        /*
-         * Stop using the generic ioeventfd, we are doing eventfd handling
-         * ourselves below
-         *
-         * FIXME: We should just switch the handler and not deassign the
-         * ioeventfd.
-         * Otherwise, there's a window where we don't have an
-         * ioeventfd and we may end up with a notification where
-         * we don't expect one.
-         */
-        virtio_bus_stop_ioeventfd(bus);
+        assert(!bus->ioeventfd_started);
+    } else {
+        if (!bus->ioeventfd_started) {
+            return 0;
+        }
     }
     return set_host_notifier_internal(proxy, bus, n, assign);
 }
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index af6b5c4..cbdf745 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -94,12 +94,6 @@ struct VirtioBusState {
     BusState parent_obj;
 
     /*
-     * Set if the default ioeventfd handlers are disabled by vhost
-     * or dataplane.
-     */
-    bool ioeventfd_disabled;
-
-    /*
      * Set if ioeventfd has been started.
      */
     bool ioeventfd_started;
-- 
1.8.3.1