* [Qemu-devel] [PATCH 01/13] virtio: disable ioeventfd as early as possible
2016-10-21 20:48 [Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop Paolo Bonzini
@ 2016-10-21 20:48 ` Paolo Bonzini
2016-10-21 20:48 ` [Qemu-devel] [PATCH 02/13] virtio: move ioeventfd_disabled flag to VirtioBusState Paolo Bonzini
` (12 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-10-21 20:48 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, stefanha, famz
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.
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
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 06831de..29896d8 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_idx,
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.
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Qemu-devel] [PATCH 02/13] virtio: move ioeventfd_disabled flag to VirtioBusState
2016-10-21 20:48 [Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop Paolo Bonzini
2016-10-21 20:48 ` [Qemu-devel] [PATCH 01/13] virtio: disable ioeventfd as early as possible Paolo Bonzini
@ 2016-10-21 20:48 ` Paolo Bonzini
2016-10-21 20:48 ` [Qemu-devel] [PATCH 03/13] virtio: move ioeventfd_started " Paolo Bonzini
` (11 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-10-21 20:48 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, stefanha, famz
This simplifies the code and removes the ioeventfd_set_disabled
callback.
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.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 29896d8..a8cf1e5 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 b4edea6..b13d28d 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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Qemu-devel] [PATCH 03/13] virtio: move ioeventfd_started flag to VirtioBusState
2016-10-21 20:48 [Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop Paolo Bonzini
2016-10-21 20:48 ` [Qemu-devel] [PATCH 01/13] virtio: disable ioeventfd as early as possible Paolo Bonzini
2016-10-21 20:48 ` [Qemu-devel] [PATCH 02/13] virtio: move ioeventfd_disabled flag to VirtioBusState Paolo Bonzini
@ 2016-10-21 20:48 ` Paolo Bonzini
2016-10-21 20:48 ` [Qemu-devel] [PATCH 04/13] virtio: add start_ioeventfd and stop_ioeventfd to VirtioDeviceClass Paolo Bonzini
` (10 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-10-21 20:48 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, stefanha, famz
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>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.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 a8cf1e5..f00fc25 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 b13d28d..4d206c8 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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Qemu-devel] [PATCH 04/13] virtio: add start_ioeventfd and stop_ioeventfd to VirtioDeviceClass
2016-10-21 20:48 [Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop Paolo Bonzini
` (2 preceding siblings ...)
2016-10-21 20:48 ` [Qemu-devel] [PATCH 03/13] virtio: move ioeventfd_started " Paolo Bonzini
@ 2016-10-21 20:48 ` Paolo Bonzini
2016-10-21 20:48 ` [Qemu-devel] [PATCH 05/13] virtio: introduce virtio_device_ioeventfd_enabled Paolo Bonzini
` (9 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-10-21 20:48 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, stefanha, famz
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>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v2->v3: avoid virtio-mmio failures [Cornelia]
hw/virtio/virtio-bus.c | 54 +++++++++++------------------------
hw/virtio/virtio.c | 64 ++++++++++++++++++++++++++++++++++++++++++
include/hw/virtio/virtio-bus.h | 7 ++++-
include/hw/virtio/virtio.h | 4 +++
4 files changed, 91 insertions(+), 38 deletions(-)
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 97cdb11..a8d2bd8 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,41 @@ 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;
+ VirtioDeviceClass *vdc;
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 = VIRTIO_DEVICE_GET_CLASS(vdev);
+ vdc->stop_ioeventfd(vdev);
bus->ioeventfd_started = false;
}
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index d48d1a9..7740975 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2158,15 +2158,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 b913aac..0e70951 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;
@@ -265,6 +267,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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Qemu-devel] [PATCH 05/13] virtio: introduce virtio_device_ioeventfd_enabled
2016-10-21 20:48 [Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop Paolo Bonzini
` (3 preceding siblings ...)
2016-10-21 20:48 ` [Qemu-devel] [PATCH 04/13] virtio: add start_ioeventfd and stop_ioeventfd to VirtioDeviceClass Paolo Bonzini
@ 2016-10-21 20:48 ` Paolo Bonzini
2016-10-21 20:48 ` [Qemu-devel] [PATCH 06/13] virtio-blk: always use dataplane path if ioeventfd is active Paolo Bonzini
` (8 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-10-21 20:48 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, stefanha, famz
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>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v2->v3: fix NULL-check-after-dereference [Cornelia]
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 a8d2bd8..3ffec66 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) {
@@ -223,6 +223,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 f00fc25..62001b4 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 7740975..c0c7856 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2233,6 +2233,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 0e70951..5d43f7e 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -269,6 +269,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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Qemu-devel] [PATCH 06/13] virtio-blk: always use dataplane path if ioeventfd is active
2016-10-21 20:48 [Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop Paolo Bonzini
` (4 preceding siblings ...)
2016-10-21 20:48 ` [Qemu-devel] [PATCH 05/13] virtio: introduce virtio_device_ioeventfd_enabled Paolo Bonzini
@ 2016-10-21 20:48 ` Paolo Bonzini
2016-10-21 20:48 ` [Qemu-devel] [PATCH 07/13] virtio-scsi: " Paolo Bonzini
` (7 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-10-21 20:48 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, stefanha, famz
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.
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/block/dataplane/virtio-blk.c | 73 +++++++++++++++++++++++++----------------
hw/block/dataplane/virtio-blk.h | 6 ++--
hw/block/virtio-blk.c | 15 ++++-----
3 files changed, 55 insertions(+), 39 deletions(-)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 9b268f4..90ef557 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -88,23 +88,28 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
*dataplane = NULL;
- if (!conf->iothread) {
- 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;
+ }
- /* 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 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 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: ");
+ /* Don't try if transport does not support notifiers. */
+ 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,19 @@ 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;
+
if (!s) {
return;
}
- virtio_blk_data_plane_stop(s);
+ vblk = VIRTIO_BLK(s->vdev);
+ 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 +161,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 +219,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 37fe72b..0c5fd27 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -611,7 +611,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;
}
@@ -687,11 +687,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);
}
@@ -789,9 +787,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)) {
@@ -919,7 +916,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) {
@@ -1002,6 +999,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 = {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Qemu-devel] [PATCH 07/13] virtio-scsi: always use dataplane path if ioeventfd is active
2016-10-21 20:48 [Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop Paolo Bonzini
` (5 preceding siblings ...)
2016-10-21 20:48 ` [Qemu-devel] [PATCH 06/13] virtio-blk: always use dataplane path if ioeventfd is active Paolo Bonzini
@ 2016-10-21 20:48 ` Paolo Bonzini
2016-11-11 4:09 ` Alex Williamson
2016-10-21 20:48 ` [Qemu-devel] [PATCH 08/13] Revert "virtio: Introduce virtio_add_queue_aio" Paolo Bonzini
` (6 subsequent siblings)
13 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2016-10-21 20:48 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, stefanha, famz
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.
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
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 4762f05..3e5ae6a 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -434,7 +434,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;
}
@@ -610,7 +610,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;
}
@@ -669,9 +669,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--;
@@ -749,7 +747,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;
}
@@ -848,14 +846,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);
}
}
@@ -885,6 +879,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)
@@ -957,6 +953,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 */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH 07/13] virtio-scsi: always use dataplane path if ioeventfd is active
2016-10-21 20:48 ` [Qemu-devel] [PATCH 07/13] virtio-scsi: " Paolo Bonzini
@ 2016-11-11 4:09 ` Alex Williamson
2016-11-11 20:24 ` Paolo Bonzini
0 siblings, 1 reply; 27+ messages in thread
From: Alex Williamson @ 2016-11-11 4:09 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, cornelia.huck, famz, stefanha, mst
On Fri, 21 Oct 2016 22:48:10 +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.
>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 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 like this added a more subtle regression with MST's previous pull
request, I have an OVMF/440FX/virtio-scsi/virtio-net/win8.1 VM, with
this patch it fails to shutdown. QEMU just spins at 400% load.
(ps from libvirt launched vm, linebreaks added)
/usr/local/bin/qemu-system-x86_64 -name guest=Steam,debug-threads=on -S \
-object secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-54-Steam/master-key.aes \
-machine pc-i440fx-2.8,accel=kvm,usb=off,vmport=off \
-cpu host,hv_time,hv_relaxed,hv_vapic,hv_spinlocks=0x1fff,hv_vendor_id=KeenlyKVM,kvm=off \
-drive file=/usr/share/edk2.git/ovmf-x64/OVMF_CODE-pure-efi.fd,if=pflash,format=raw,unit=0,readonly=on \
-drive file=/var/lib/libvirt/qemu/nvram/Steam_VARS.fd,if=pflash,format=raw,unit=1 \
-m 4096 -mem-prealloc -mem-path /dev/hugepages/libvirt/qemu -realtime mlock=off \
-smp 4,sockets=1,cores=4,threads=1 -uuid 79f39860-d659-426b-89e8-90cb46ee57c6 \
-display none -no-user-config -nodefaults \
-chardev socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-54-Steam/monitor.sock,server,nowait \
-mon chardev=charmonitor,id=monitor,mode=control -rtc base=localtime,driftfix=slew \
-global kvm-pit.lost_tick_policy=discard -no-hpet -no-shutdown -boot menu=on,strict=on \
-device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x8.0x7 \
-device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x8 \
-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x8.0x1 \
-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x8.0x2 \
-device virtio-scsi-pci,id=scsi0,num_queues=4,bus=pci.0,addr=0x5 \
-drive file=/mnt/store/vm/Steam.img,format=raw,if=none,id=drive-scsi0-0-0-0,cache=none \
-device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=2 \
-netdev tap,fd=28,id=hostnet0,vhost=on,vhostfd=30 \
-device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:60:ef:ac,bus=pci.0,addr=0x3 \
-device vfio-pci,host=01:00.0,id=hostdev0,bus=pci.0,addr=0x2 \
-device vfio-pci,host=01:00.1,id=hostdev1,bus=pci.0,addr=0x6,rombar=0 \
-device vfio-pci,host=02:00.0,id=hostdev2,bus=pci.0,addr=0x7 -snapshot -msg timestamp=on
Fails:
ad07cd6 virtio-scsi: always use dataplane path if ioeventfd is active
Works (but I'm not using virtio-blk):
9ffe337 virtio-blk: always use dataplane path if ioeventfd is active
Thanks,
Alex
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 07/13] virtio-scsi: always use dataplane path if ioeventfd is active
2016-11-11 4:09 ` Alex Williamson
@ 2016-11-11 20:24 ` Paolo Bonzini
2016-11-11 21:03 ` Alex Williamson
0 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2016-11-11 20:24 UTC (permalink / raw)
To: Alex Williamson; +Cc: cornelia.huck, famz, qemu-devel, stefanha, mst
On 11/11/2016 05:09, Alex Williamson wrote:
> On Fri, 21 Oct 2016 22:48:10 +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.
>>
>> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> 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 like this added a more subtle regression with MST's previous pull
> request, I have an OVMF/440FX/virtio-scsi/virtio-net/win8.1 VM, with
> this patch it fails to shutdown. QEMU just spins at 400% load.
I cannot reproduce this one. :( You said you get it even with vhost,
what if you remove VFIO and add a regular VGA?
If you can post a backtrace of all threads at the time of the hang, from
origin/master (so without vhost, and not at ad07cd6) that could help.
Paolo
> (ps from libvirt launched vm, linebreaks added)
>
> /usr/local/bin/qemu-system-x86_64 -name guest=Steam,debug-threads=on -S \
> -object secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-54-Steam/master-key.aes \
> -machine pc-i440fx-2.8,accel=kvm,usb=off,vmport=off \
> -cpu host,hv_time,hv_relaxed,hv_vapic,hv_spinlocks=0x1fff,hv_vendor_id=KeenlyKVM,kvm=off \
> -drive file=/usr/share/edk2.git/ovmf-x64/OVMF_CODE-pure-efi.fd,if=pflash,format=raw,unit=0,readonly=on \
> -drive file=/var/lib/libvirt/qemu/nvram/Steam_VARS.fd,if=pflash,format=raw,unit=1 \
> -m 4096 -mem-prealloc -mem-path /dev/hugepages/libvirt/qemu -realtime mlock=off \
> -smp 4,sockets=1,cores=4,threads=1 -uuid 79f39860-d659-426b-89e8-90cb46ee57c6 \
> -display none -no-user-config -nodefaults \
> -chardev socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-54-Steam/monitor.sock,server,nowait \
> -mon chardev=charmonitor,id=monitor,mode=control -rtc base=localtime,driftfix=slew \
> -global kvm-pit.lost_tick_policy=discard -no-hpet -no-shutdown -boot menu=on,strict=on \
> -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x8.0x7 \
> -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x8 \
> -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x8.0x1 \
> -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x8.0x2 \
> -device virtio-scsi-pci,id=scsi0,num_queues=4,bus=pci.0,addr=0x5 \
> -drive file=/mnt/store/vm/Steam.img,format=raw,if=none,id=drive-scsi0-0-0-0,cache=none \
> -device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=2 \
> -netdev tap,fd=28,id=hostnet0,vhost=on,vhostfd=30 \
> -device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:60:ef:ac,bus=pci.0,addr=0x3 \
> -device vfio-pci,host=01:00.0,id=hostdev0,bus=pci.0,addr=0x2 \
> -device vfio-pci,host=01:00.1,id=hostdev1,bus=pci.0,addr=0x6,rombar=0 \
> -device vfio-pci,host=02:00.0,id=hostdev2,bus=pci.0,addr=0x7 -snapshot -msg timestamp=on
>
> Fails:
> ad07cd6 virtio-scsi: always use dataplane path if ioeventfd is active
>
> Works (but I'm not using virtio-blk):
> 9ffe337 virtio-blk: always use dataplane path if ioeventfd is active
>
> Thanks,
> Alex
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 07/13] virtio-scsi: always use dataplane path if ioeventfd is active
2016-11-11 20:24 ` Paolo Bonzini
@ 2016-11-11 21:03 ` Alex Williamson
2016-11-14 13:41 ` Paolo Bonzini
0 siblings, 1 reply; 27+ messages in thread
From: Alex Williamson @ 2016-11-11 21:03 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: cornelia.huck, famz, qemu-devel, stefanha, mst
On Fri, 11 Nov 2016 21:24:33 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 11/11/2016 05:09, Alex Williamson wrote:
> > On Fri, 21 Oct 2016 22:48:10 +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.
> >>
> >> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> >> 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 like this added a more subtle regression with MST's previous pull
> > request, I have an OVMF/440FX/virtio-scsi/virtio-net/win8.1 VM, with
> > this patch it fails to shutdown. QEMU just spins at 400% load.
>
> I cannot reproduce this one. :( You said you get it even with vhost,
> what if you remove VFIO and add a regular VGA?
>
> If you can post a backtrace of all threads at the time of the hang, from
> origin/master (so without vhost, and not at ad07cd6) that could help.
Yes, it occurs with all of the vfio devices removed using VNC/Cirrus.
Backtrace on all threads running 6bbcb76:
Thread 7 (Thread 0x7f5734dff700 (LWP 13136)):
#0 0x00007f5748adcbd0 in pthread_cond_wait@@GLIBC_2.3.2 () at ../sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:185
#1 0x0000565105c1b5a1 in qemu_cond_wait (cond=0x565108c8f120, mutex=0x565108c8f150) at util/qemu-thread-posix.c:137
#2 0x0000565105b2219b in vnc_worker_thread_loop (queue=0x565108c8f120) at ui/vnc-jobs.c:228
#3 0x0000565105b226d1 in vnc_worker_thread (arg=0x565108c8f120) at ui/vnc-jobs.c:335
#4 0x00007f5748ad75ca in start_thread (arg=0x7f5734dff700) at pthread_create.c:333
#5 0x00007f57488110ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
Thread 6 (Thread 0x7f5736bfe700 (LWP 13133)):
#0 0x00007f5748806ce7 in ioctl () at ../sysdeps/unix/syscall-template.S:84
#1 0x000056510579734e in kvm_vcpu_ioctl (cpu=0x565107b21b70, type=44672) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:2079
#2 0x0000565105796cd5 in kvm_cpu_exec (cpu=0x565107b21b70) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1929
#3 0x000056510577dc58 in qemu_kvm_cpu_thread_fn (arg=0x565107b21b70) at /net/gimli/home/alwillia/Work/qemu.git/cpus.c:998
#4 0x00007f5748ad75ca in start_thread (arg=0x7f5736bfe700) at pthread_create.c:333
#5 0x00007f57488110ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
Thread 5 (Thread 0x7f57373ff700 (LWP 13132)):
#0 0x00007f5748806ce7 in ioctl () at ../sysdeps/unix/syscall-template.S:84
#1 0x000056510579734e in kvm_vcpu_ioctl (cpu=0x565107b02350, type=44672) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:2079
#2 0x0000565105796cd5 in kvm_cpu_exec (cpu=0x565107b02350) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1929
#3 0x000056510577dc58 in qemu_kvm_cpu_thread_fn (arg=0x565107b02350) at /net/gimli/home/alwillia/Work/qemu.git/cpus.c:998
#4 0x00007f5748ad75ca in start_thread (arg=0x7f57373ff700) at pthread_create.c:333
#5 0x00007f57488110ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
Thread 4 (Thread 0x7f5737c00700 (LWP 13131)):
#0 0x00007f5748806ce7 in ioctl () at ../sysdeps/unix/syscall-template.S:84
#1 0x000056510579734e in kvm_vcpu_ioctl (cpu=0x565107ae2b20, type=44672) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:2079
#2 0x0000565105796cd5 in kvm_cpu_exec (cpu=0x565107ae2b20) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1929
#3 0x000056510577dc58 in qemu_kvm_cpu_thread_fn (arg=0x565107ae2b20) at /net/gimli/home/alwillia/Work/qemu.git/cpus.c:998
#4 0x00007f5748ad75ca in start_thread (arg=0x7f5737c00700) at pthread_create.c:333
#5 0x00007f57488110ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
Thread 3 (Thread 0x7f5738401700 (LWP 13130)):
#0 0x00007f5748806ce7 in ioctl () at ../sysdeps/unix/syscall-template.S:84
#1 0x000056510579734e in kvm_vcpu_ioctl (cpu=0x565107a85270, type=44672) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:2079
#2 0x0000565105796cd5 in kvm_cpu_exec (cpu=0x565107a85270) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1929
#3 0x000056510577dc58 in qemu_kvm_cpu_thread_fn (arg=0x565107a85270) at /net/gimli/home/alwillia/Work/qemu.git/cpus.c:998
#4 0x00007f5748ad75ca in start_thread (arg=0x7f5738401700) at pthread_create.c:333
#5 0x00007f57488110ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
Thread 2 (Thread 0x7f573b221700 (LWP 13122)):
#0 0x00007f574880b239 in syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
#1 0x0000565105c1b92f in futex_wait (ev=0x5651066c5624 <rcu_call_ready_event>, val=4294967295) at util/qemu-thread-posix.c:306
#2 0x0000565105c1ba32 in qemu_event_wait (ev=0x5651066c5624 <rcu_call_ready_event>) at util/qemu-thread-posix.c:422
#3 0x0000565105c31f30 in call_rcu_thread (opaque=0x0) at util/rcu.c:249
#4 0x00007f5748ad75ca in start_thread (arg=0x7f573b221700) at pthread_create.c:333
#5 0x00007f57488110ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
Thread 1 (Thread 0x7f57638c4f80 (LWP 13114)):
#0 0x00007f5748805631 in __GI_ppoll (fds=0x565108f6e6b0, nfds=13, timeout=<optimized out>, sigmask=0x0) at ../sysdeps/unix/sysv/linux/ppoll.c:50
#1 0x0000565105b3b20b in qemu_poll_ns (fds=0x565108f6e6b0, nfds=13, timeout=2999772164) at qemu-timer.c:325
#2 0x0000565105b3a630 in os_host_main_loop_wait (timeout=2999772164) at main-loop.c:254
#3 0x0000565105b3a6f3 in main_loop_wait (nonblocking=0) at main-loop.c:508
#4 0x00005651058c9d80 in main_loop () at vl.c:1966
#5 0x00005651058d169e in main (argc=64, argv=0x7fff56193428, envp=0x7fff56193630) at vl.c:4684
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 07/13] virtio-scsi: always use dataplane path if ioeventfd is active
2016-11-11 21:03 ` Alex Williamson
@ 2016-11-14 13:41 ` Paolo Bonzini
2016-11-14 17:09 ` Alex Williamson
0 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2016-11-14 13:41 UTC (permalink / raw)
To: Alex Williamson; +Cc: cornelia.huck, famz, qemu-devel, stefanha, mst
On 11/11/2016 22:03, Alex Williamson wrote:
> On Fri, 11 Nov 2016 21:24:33 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>> If you can post a backtrace of all threads at the time of the hang, from
>> origin/master (so without vhost, and not at ad07cd6) that could help.
>
> Yes, it occurs with all of the vfio devices removed using VNC/Cirrus.
I cannot reproduce it anyway. :(
As you said on IRC it's a pretty standard "event loop doing nothing"
backtrace, so it seems that an eventfd write was lost.
Since I was lucky with the vhost patch, perhaps this can help:
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index f2ea29d..22d6cd5 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -202,13 +202,15 @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
aio_context_acquire(s->ctx);
virtio_scsi_clear_aio(s);
- aio_context_release(s->ctx);
-
- blk_drain_all(); /* ensure there are no in-flight requests */
for (i = 0; i < vs->conf.num_queues + 2; i++) {
+ VirtQueue *vq = virtio_get_queue(vdev, i);
virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+ virtio_queue_host_notifier_aio_read(virtio_queue_get_guest_notifier(vq));
}
+ aio_context_release(s->ctx);
+
+ blk_drain_all(); /* ensure there are no in-flight requests */
/* Clean up guest notifier (irq) */
k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 89b0b80..9c894d7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2018,7 +2018,7 @@ EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq)
return &vq->guest_notifier;
}
-static void virtio_queue_host_notifier_aio_read(EventNotifier *n)
+void virtio_queue_host_notifier_aio_read(EventNotifier *n)
{
VirtQueue *vq = container_of(n, VirtQueue, host_notifier);
if (event_notifier_test_and_clear(n)) {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 35ede30..d3dfc69 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -274,6 +274,7 @@ int virtio_device_grab_ioeventfd(VirtIODevice *vdev);
void virtio_device_release_ioeventfd(VirtIODevice *vdev);
bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev);
EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
+void virtio_queue_host_notifier_aio_read(EventNotifier *n);
void virtio_queue_host_notifier_read(EventNotifier *n);
void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
void (*fn)(VirtIODevice *,
And if it doesn't work here is some printf debugging. It's pretty verbose but
the interesting part starts pretty much where you issue the virsh shutdown or
system_powerdown command:
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index f2ea29d..ec0f750 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -108,11 +108,13 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s)
VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
int i;
+ printf("before clear\n");
virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, NULL);
virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, NULL);
for (i = 0; i < vs->conf.num_queues; i++) {
virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, NULL);
}
+ printf("after clear\n");
}
/* Context: QEMU global mutex held */
@@ -202,15 +204,20 @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
aio_context_acquire(s->ctx);
virtio_scsi_clear_aio(s);
- aio_context_release(s->ctx);
-
- blk_drain_all(); /* ensure there are no in-flight requests */
for (i = 0; i < vs->conf.num_queues + 2; i++) {
+ VirtQueue *vq = virtio_get_queue(vdev, i);
virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+ virtio_queue_host_notifier_aio_read(virtio_queue_get_guest_notifier(vq));
}
+ aio_context_release(s->ctx);
+
+ printf("before drain\n");
+ blk_drain_all(); /* ensure there are no in-flight requests */
+ printf("after drain\n");
/* Clean up guest notifier (irq) */
+ printf("end of virtio_scsi_dataplane_stop\n");
k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
s->dataplane_stopping = false;
s->dataplane_started = false;
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 3e5ae6a..e8b83d4 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -75,6 +75,7 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
}
if (req->sreq) {
+ printf("finish %x\n", req->sreq->tag);
req->sreq->hba_private = NULL;
scsi_req_unref(req->sreq);
}
@@ -549,6 +549,7 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
return -ENOENT;
}
virtio_scsi_ctx_check(s, d);
+ printf("prepare %lx %x\n", req->req.cmd.tag, req->req.cmd.cdb[0]);
req->sreq = scsi_req_new(d, req->req.cmd.tag,
virtio_scsi_get_lun(req->req.cmd.lun),
req->req.cmd.cdb, req);
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 62001b4..c75dec3 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -336,11 +336,13 @@ static int virtio_pci_ioeventfd_assign(DeviceState *d, EventNotifier *notifier,
static void virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy)
{
+ printf("start ioeventfd %s\n", object_class_get_name(object_get_class(OBJECT(proxy))));
virtio_bus_start_ioeventfd(&proxy->bus);
}
static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy)
{
+ printf("stop ioeventfd %s\n", object_class_get_name(object_get_class(OBJECT(proxy))));
virtio_bus_stop_ioeventfd(&proxy->bus);
}
@@ -376,6 +378,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
}
break;
case VIRTIO_PCI_STATUS:
+ printf("set status %s %x\n", object_class_get_name(object_get_class(OBJECT(proxy))), val & 0xFF);
if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
virtio_pci_stop_ioeventfd(proxy);
}
@@ -1274,6 +1277,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
vdev->config_vector = val;
break;
case VIRTIO_PCI_COMMON_STATUS:
+ printf("set status %s %x\n", object_class_get_name(object_get_class(OBJECT(proxy))), (uint8_t)val);
if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
virtio_pci_stop_ioeventfd(proxy);
}
> Backtrace on all threads running 6bbcb76:
>
> Thread 7 (Thread 0x7f5734dff700 (LWP 13136)):
> #0 0x00007f5748adcbd0 in pthread_cond_wait@@GLIBC_2.3.2 () at ../sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:185
> #1 0x0000565105c1b5a1 in qemu_cond_wait (cond=0x565108c8f120, mutex=0x565108c8f150) at util/qemu-thread-posix.c:137
> #2 0x0000565105b2219b in vnc_worker_thread_loop (queue=0x565108c8f120) at ui/vnc-jobs.c:228
> #3 0x0000565105b226d1 in vnc_worker_thread (arg=0x565108c8f120) at ui/vnc-jobs.c:335
> #4 0x00007f5748ad75ca in start_thread (arg=0x7f5734dff700) at pthread_create.c:333
> #5 0x00007f57488110ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
>
> Thread 6 (Thread 0x7f5736bfe700 (LWP 13133)):
> #0 0x00007f5748806ce7 in ioctl () at ../sysdeps/unix/syscall-template.S:84
> #1 0x000056510579734e in kvm_vcpu_ioctl (cpu=0x565107b21b70, type=44672) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:2079
> #2 0x0000565105796cd5 in kvm_cpu_exec (cpu=0x565107b21b70) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1929
> #3 0x000056510577dc58 in qemu_kvm_cpu_thread_fn (arg=0x565107b21b70) at /net/gimli/home/alwillia/Work/qemu.git/cpus.c:998
> #4 0x00007f5748ad75ca in start_thread (arg=0x7f5736bfe700) at pthread_create.c:333
> #5 0x00007f57488110ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
>
> Thread 5 (Thread 0x7f57373ff700 (LWP 13132)):
> #0 0x00007f5748806ce7 in ioctl () at ../sysdeps/unix/syscall-template.S:84
> #1 0x000056510579734e in kvm_vcpu_ioctl (cpu=0x565107b02350, type=44672) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:2079
> #2 0x0000565105796cd5 in kvm_cpu_exec (cpu=0x565107b02350) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1929
> #3 0x000056510577dc58 in qemu_kvm_cpu_thread_fn (arg=0x565107b02350) at /net/gimli/home/alwillia/Work/qemu.git/cpus.c:998
> #4 0x00007f5748ad75ca in start_thread (arg=0x7f57373ff700) at pthread_create.c:333
> #5 0x00007f57488110ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
>
> Thread 4 (Thread 0x7f5737c00700 (LWP 13131)):
> #0 0x00007f5748806ce7 in ioctl () at ../sysdeps/unix/syscall-template.S:84
> #1 0x000056510579734e in kvm_vcpu_ioctl (cpu=0x565107ae2b20, type=44672) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:2079
> #2 0x0000565105796cd5 in kvm_cpu_exec (cpu=0x565107ae2b20) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1929
> #3 0x000056510577dc58 in qemu_kvm_cpu_thread_fn (arg=0x565107ae2b20) at /net/gimli/home/alwillia/Work/qemu.git/cpus.c:998
> #4 0x00007f5748ad75ca in start_thread (arg=0x7f5737c00700) at pthread_create.c:333
> #5 0x00007f57488110ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
>
> Thread 3 (Thread 0x7f5738401700 (LWP 13130)):
> #0 0x00007f5748806ce7 in ioctl () at ../sysdeps/unix/syscall-template.S:84
> #1 0x000056510579734e in kvm_vcpu_ioctl (cpu=0x565107a85270, type=44672) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:2079
> #2 0x0000565105796cd5 in kvm_cpu_exec (cpu=0x565107a85270) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1929
> #3 0x000056510577dc58 in qemu_kvm_cpu_thread_fn (arg=0x565107a85270) at /net/gimli/home/alwillia/Work/qemu.git/cpus.c:998
> #4 0x00007f5748ad75ca in start_thread (arg=0x7f5738401700) at pthread_create.c:333
> #5 0x00007f57488110ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
>
> Thread 2 (Thread 0x7f573b221700 (LWP 13122)):
> #0 0x00007f574880b239 in syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
> #1 0x0000565105c1b92f in futex_wait (ev=0x5651066c5624 <rcu_call_ready_event>, val=4294967295) at util/qemu-thread-posix.c:306
> #2 0x0000565105c1ba32 in qemu_event_wait (ev=0x5651066c5624 <rcu_call_ready_event>) at util/qemu-thread-posix.c:422
> #3 0x0000565105c31f30 in call_rcu_thread (opaque=0x0) at util/rcu.c:249
> #4 0x00007f5748ad75ca in start_thread (arg=0x7f573b221700) at pthread_create.c:333
> #5 0x00007f57488110ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
>
> Thread 1 (Thread 0x7f57638c4f80 (LWP 13114)):
> #0 0x00007f5748805631 in __GI_ppoll (fds=0x565108f6e6b0, nfds=13, timeout=<optimized out>, sigmask=0x0) at ../sysdeps/unix/sysv/linux/ppoll.c:50
> #1 0x0000565105b3b20b in qemu_poll_ns (fds=0x565108f6e6b0, nfds=13, timeout=2999772164) at qemu-timer.c:325
> #2 0x0000565105b3a630 in os_host_main_loop_wait (timeout=2999772164) at main-loop.c:254
> #3 0x0000565105b3a6f3 in main_loop_wait (nonblocking=0) at main-loop.c:508
> #4 0x00005651058c9d80 in main_loop () at vl.c:1966
> #5 0x00005651058d169e in main (argc=64, argv=0x7fff56193428, envp=0x7fff56193630) at vl.c:4684
>
>
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH 07/13] virtio-scsi: always use dataplane path if ioeventfd is active
2016-11-14 13:41 ` Paolo Bonzini
@ 2016-11-14 17:09 ` Alex Williamson
2016-11-14 18:10 ` Paolo Bonzini
0 siblings, 1 reply; 27+ messages in thread
From: Alex Williamson @ 2016-11-14 17:09 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: cornelia.huck, famz, qemu-devel, stefanha, mst
[-- Attachment #1: Type: text/plain, Size: 14103 bytes --]
On Mon, 14 Nov 2016 14:41:42 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 11/11/2016 22:03, Alex Williamson wrote:
> > On Fri, 11 Nov 2016 21:24:33 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> If you can post a backtrace of all threads at the time of the hang, from
> >> origin/master (so without vhost, and not at ad07cd6) that could help.
> >
> > Yes, it occurs with all of the vfio devices removed using VNC/Cirrus.
>
> I cannot reproduce it anyway. :(
>
> As you said on IRC it's a pretty standard "event loop doing nothing"
> backtrace, so it seems that an eventfd write was lost.
>
> Since I was lucky with the vhost patch, perhaps this can help:
>
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index f2ea29d..22d6cd5 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -202,13 +202,15 @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
>
> aio_context_acquire(s->ctx);
> virtio_scsi_clear_aio(s);
> - aio_context_release(s->ctx);
> -
> - blk_drain_all(); /* ensure there are no in-flight requests */
>
> for (i = 0; i < vs->conf.num_queues + 2; i++) {
> + VirtQueue *vq = virtio_get_queue(vdev, i);
> virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
> + virtio_queue_host_notifier_aio_read(virtio_queue_get_guest_notifier(vq));
> }
> + aio_context_release(s->ctx);
> +
> + blk_drain_all(); /* ensure there are no in-flight requests */
>
> /* Clean up guest notifier (irq) */
> k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 89b0b80..9c894d7 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2018,7 +2018,7 @@ EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq)
> return &vq->guest_notifier;
> }
>
> -static void virtio_queue_host_notifier_aio_read(EventNotifier *n)
> +void virtio_queue_host_notifier_aio_read(EventNotifier *n)
> {
> VirtQueue *vq = container_of(n, VirtQueue, host_notifier);
> if (event_notifier_test_and_clear(n)) {
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 35ede30..d3dfc69 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -274,6 +274,7 @@ int virtio_device_grab_ioeventfd(VirtIODevice *vdev);
> void virtio_device_release_ioeventfd(VirtIODevice *vdev);
> bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev);
> EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
> +void virtio_queue_host_notifier_aio_read(EventNotifier *n);
> void virtio_queue_host_notifier_read(EventNotifier *n);
> void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
> void (*fn)(VirtIODevice *,
>
Hmm, fixed yet not fixed. I get a nice shutdown and it even eliminates
a cpu spike shown in virt-manager at the end of shutdown that was
typical previously, but then I noticed dmesg showing me segfaults, so I
hooked up gdb and:
Thread 3 "CPU 0/KVM" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fb4f73ba700 (LWP 2713)]
0x00005593dacc2800 in virtio_queue_notify_aio_vq (vq=0x5593dd4a7378) at /net/gimli/home/alwillia/Work/qemu.git/hw/virtio/virtio.c:1242
1242 trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
(gdb) bt
#0 0x00005593dacc2800 in virtio_queue_notify_aio_vq (vq=0x5593dd4a7378) at /net/gimli/home/alwillia/Work/qemu.git/hw/virtio/virtio.c:1242
#1 0x00005593dacc4a4e in virtio_queue_host_notifier_aio_read (n=0x5593dd4a73d8) at /net/gimli/home/alwillia/Work/qemu.git/hw/virtio/virtio.c:2025
#2 0x00005593daca4997 in virtio_scsi_dataplane_stop (vdev=0x5593dc0cc0f0) at /net/gimli/home/alwillia/Work/qemu.git/hw/scsi/virtio-scsi-dataplane.c:209
#3 0x00005593daf6a4b7 in virtio_bus_stop_ioeventfd (bus=0x5593dc0cc078) at hw/virtio/virtio-bus.c:219
#4 0x00005593daf64279 in virtio_pci_stop_ioeventfd (proxy=0x5593dc0c3ce0) at hw/virtio/virtio-pci.c:344
#5 0x00005593daf643d5 in virtio_ioport_write (opaque=0x5593dc0c3ce0, addr=18, val=0) at hw/virtio/virtio-pci.c:380
#6 0x00005593daf6484d in virtio_pci_config_write (opaque=0x5593dc0c3ce0, addr=18, val=0, size=1) at hw/virtio/virtio-pci.c:508
#7 0x00005593dac592fd in memory_region_write_accessor (mr=0x5593dc0c45d0, addr=18, value=0x7fb4f73b74b8, size=1, shift=0, mask=255, attrs=...)
at /net/gimli/home/alwillia/Work/qemu.git/memory.c:526
#8 0x00005593dac59515 in access_with_adjusted_size (addr=18, value=0x7fb4f73b74b8, size=1, access_size_min=1, access_size_max=4, access=
0x5593dac59213 <memory_region_write_accessor>, mr=0x5593dc0c45d0, attrs=...) at /net/gimli/home/alwillia/Work/qemu.git/memory.c:592
#9 0x00005593dac5bc55 in memory_region_dispatch_write (mr=0x5593dc0c45d0, addr=18, data=0, size=1, attrs=...) at /net/gimli/home/alwillia/Work/qemu.git/memory.c:1323
#10 0x00005593dac07583 in address_space_write_continue (as=0x5593db727de0 <address_space_io>, addr=49298, attrs=..., buf=0x7fb520354000 "", len=1, addr1=18, l=1, mr=0x5593dc0c45d0)
at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2621
#11 0x00005593dac076cb in address_space_write (as=0x5593db727de0 <address_space_io>, addr=49298, attrs=..., buf=0x7fb520354000 "", len=1)
at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2666
#12 0x00005593dac07a57 in address_space_rw (as=0x5593db727de0 <address_space_io>, addr=49298, attrs=..., buf=0x7fb520354000 "", len=1, is_write=true)
at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2768
#13 0x00005593dac558d7 in kvm_handle_io (port=49298, attrs=..., data=0x7fb520354000, direction=1, size=1, count=1) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1800
#14 0x00005593dac55ddd in kvm_cpu_exec (cpu=0x5593dc0a6490) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1958
#15 0x00005593dac3cc58 in qemu_kvm_cpu_thread_fn (arg=0x5593dc0a6490) at /net/gimli/home/alwillia/Work/qemu.git/cpus.c:998
#16 0x00007fb5054715ca in start_thread (arg=0x7fb4f73ba700) at pthread_create.c:333
#17 0x00007fb5051ab0ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
> And if it doesn't work here is some printf debugging. It's pretty verbose but
> the interesting part starts pretty much where you issue the virsh shutdown or
> system_powerdown command:
>
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index f2ea29d..ec0f750 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -108,11 +108,13 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s)
> VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
> int i;
>
> + printf("before clear\n");
> virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, NULL);
> virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, NULL);
> for (i = 0; i < vs->conf.num_queues; i++) {
> virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, NULL);
> }
> + printf("after clear\n");
> }
>
> /* Context: QEMU global mutex held */
> @@ -202,15 +204,20 @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
>
> aio_context_acquire(s->ctx);
> virtio_scsi_clear_aio(s);
> - aio_context_release(s->ctx);
> -
> - blk_drain_all(); /* ensure there are no in-flight requests */
>
> for (i = 0; i < vs->conf.num_queues + 2; i++) {
> + VirtQueue *vq = virtio_get_queue(vdev, i);
> virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
> + virtio_queue_host_notifier_aio_read(virtio_queue_get_guest_notifier(vq));
> }
> + aio_context_release(s->ctx);
> +
> + printf("before drain\n");
> + blk_drain_all(); /* ensure there are no in-flight requests */
> + printf("after drain\n");
>
> /* Clean up guest notifier (irq) */
> + printf("end of virtio_scsi_dataplane_stop\n");
> k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
> s->dataplane_stopping = false;
> s->dataplane_started = false;
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 3e5ae6a..e8b83d4 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -75,6 +75,7 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
> }
>
> if (req->sreq) {
> + printf("finish %x\n", req->sreq->tag);
> req->sreq->hba_private = NULL;
> scsi_req_unref(req->sreq);
> }
> @@ -549,6 +549,7 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
> return -ENOENT;
> }
> virtio_scsi_ctx_check(s, d);
> + printf("prepare %lx %x\n", req->req.cmd.tag, req->req.cmd.cdb[0]);
> req->sreq = scsi_req_new(d, req->req.cmd.tag,
> virtio_scsi_get_lun(req->req.cmd.lun),
> req->req.cmd.cdb, req);
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 62001b4..c75dec3 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -336,11 +336,13 @@ static int virtio_pci_ioeventfd_assign(DeviceState *d, EventNotifier *notifier,
>
> static void virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy)
> {
> + printf("start ioeventfd %s\n", object_class_get_name(object_get_class(OBJECT(proxy))));
> virtio_bus_start_ioeventfd(&proxy->bus);
> }
>
> static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy)
> {
> + printf("stop ioeventfd %s\n", object_class_get_name(object_get_class(OBJECT(proxy))));
> virtio_bus_stop_ioeventfd(&proxy->bus);
> }
>
> @@ -376,6 +378,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> }
> break;
> case VIRTIO_PCI_STATUS:
> + printf("set status %s %x\n", object_class_get_name(object_get_class(OBJECT(proxy))), val & 0xFF);
> if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> virtio_pci_stop_ioeventfd(proxy);
> }
> @@ -1274,6 +1277,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
> vdev->config_vector = val;
> break;
> case VIRTIO_PCI_COMMON_STATUS:
> + printf("set status %s %x\n", object_class_get_name(object_get_class(OBJECT(proxy))), (uint8_t)val);
> if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> virtio_pci_stop_ioeventfd(proxy);
> }
>
This required making virtio_queue_host_notifier_aio_read() non-static
and adding the forward declaration, stolen from the first patch. The
attached log starts at the point where there guest is idle and I issue
a virsh shutdown. This also results in a segfault nearly identical to
above:
Thread 4 "CPU 1/KVM" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7f7194901700 (LWP 3804)]
0x000056358070788a in virtio_queue_notify_aio_vq (vq=0x56358310d378) at /net/gimli/home/alwillia/Work/qemu.git/hw/virtio/virtio.c:1242
1242 trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
(gdb) bt
#0 0x000056358070788a in virtio_queue_notify_aio_vq (vq=0x56358310d378) at /net/gimli/home/alwillia/Work/qemu.git/hw/virtio/virtio.c:1242
#1 0x0000563580709ad8 in virtio_queue_host_notifier_aio_read (n=0x56358310d3d8) at /net/gimli/home/alwillia/Work/qemu.git/hw/virtio/virtio.c:2025
#2 0x00005635806e99fd in virtio_scsi_dataplane_stop (vdev=0x563581d320f0) at /net/gimli/home/alwillia/Work/qemu.git/hw/scsi/virtio-scsi-dataplane.c:211
#3 0x00005635809af5fe in virtio_bus_stop_ioeventfd (bus=0x563581d32078) at hw/virtio/virtio-bus.c:219
#4 0x00005635809a9353 in virtio_pci_stop_ioeventfd (proxy=0x563581d29ce0) at hw/virtio/virtio-pci.c:346
#5 0x00005635809a94e0 in virtio_ioport_write (opaque=0x563581d29ce0, addr=18, val=0) at hw/virtio/virtio-pci.c:383
#6 0x00005635809a995d in virtio_pci_config_write (opaque=0x563581d29ce0, addr=18, val=0, size=1) at hw/virtio/virtio-pci.c:511
#7 0x000056358069e2fd in memory_region_write_accessor (mr=0x563581d2a5d0, addr=18, value=0x7f71948fe4b8, size=1, shift=0, mask=255, attrs=...)
at /net/gimli/home/alwillia/Work/qemu.git/memory.c:526
#8 0x000056358069e515 in access_with_adjusted_size (addr=18, value=0x7f71948fe4b8, size=1, access_size_min=1, access_size_max=4, access=
0x56358069e213 <memory_region_write_accessor>, mr=0x563581d2a5d0, attrs=...) at /net/gimli/home/alwillia/Work/qemu.git/memory.c:592
#9 0x00005635806a0c55 in memory_region_dispatch_write (mr=0x563581d2a5d0, addr=18, data=0, size=1, attrs=...) at /net/gimli/home/alwillia/Work/qemu.git/memory.c:1323
#10 0x000056358064c583 in address_space_write_continue (as=0x56358116cde0 <address_space_io>, addr=49298, attrs=..., buf=0x7f71be099000 "", len=1, addr1=18, l=1, mr=0x563581d2a5d0)
at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2621
#11 0x000056358064c6cb in address_space_write (as=0x56358116cde0 <address_space_io>, addr=49298, attrs=..., buf=0x7f71be099000 "", len=1)
at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2666
#12 0x000056358064ca57 in address_space_rw (as=0x56358116cde0 <address_space_io>, addr=49298, attrs=..., buf=0x7f71be099000 "", len=1, is_write=true)
at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2768
#13 0x000056358069a8d7 in kvm_handle_io (port=49298, attrs=..., data=0x7f71be099000, direction=1, size=1, count=1) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1800
#14 0x000056358069addd in kvm_cpu_exec (cpu=0x563581d6d030) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1958
#15 0x0000563580681c58 in qemu_kvm_cpu_thread_fn (arg=0x563581d6d030) at /net/gimli/home/alwillia/Work/qemu.git/cpus.c:998
#16 0x00007f71a31b95ca in start_thread (arg=0x7f7194901700) at pthread_create.c:333
#17 0x00007f71a2ef30ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
If you care to match line numbers, my tree is based on
6bbcb76301a72dc80c8d29af13d40bb9a759c9c6, it includes you patch:
virtio: introduce grab/release_ioeventfd to fix vhost
Plus your first fix removing the assert and return 0 case from
virtio_bus_set_host_notifier(). Thanks,
Alex
[-- Attachment #2: shutdown.log.bz2 --]
[-- Type: application/x-bzip, Size: 8158 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH 07/13] virtio-scsi: always use dataplane path if ioeventfd is active
2016-11-14 17:09 ` Alex Williamson
@ 2016-11-14 18:10 ` Paolo Bonzini
2016-11-14 18:49 ` Alex Williamson
0 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2016-11-14 18:10 UTC (permalink / raw)
To: Alex Williamson; +Cc: cornelia.huck, famz, qemu-devel, stefanha, mst
On 14/11/2016 18:09, Alex Williamson wrote:
> Hmm, fixed yet not fixed. I get a nice shutdown and it even eliminates
> a cpu spike shown in virt-manager at the end of shutdown that was
> typical previously, but then I noticed dmesg showing me segfaults, so I
> hooked up gdb and:
Well, that I don't get the segfault already says something... Though
it's not even clear from the backtrace what is causing the segfault.
Let's try the same printf without the change.
Paolo
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index f2ea29d..ec0f750 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -108,11 +108,13 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s)
VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
int i;
+ printf("before clear\n");
virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, NULL);
virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, NULL);
for (i = 0; i < vs->conf.num_queues; i++) {
virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, NULL);
}
+ printf("after clear\n");
}
/* Context: QEMU global mutex held */
@@ -202,15 +204,18 @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
aio_context_acquire(s->ctx);
virtio_scsi_clear_aio(s);
aio_context_release(s->ctx);
+ printf("before drain\n");
blk_drain_all(); /* ensure there are no in-flight requests */
+ printf("after drain\n");
for (i = 0; i < vs->conf.num_queues + 2; i++) {
virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
}
/* Clean up guest notifier (irq) */
+ printf("end of virtio_scsi_dataplane_stop\n");
k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
s->dataplane_stopping = false;
s->dataplane_started = false;
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 3e5ae6a..dabcb54 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -75,6 +75,7 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
}
if (req->sreq) {
+ printf("finish %x\n", req->sreq->tag);
req->sreq->hba_private = NULL;
scsi_req_unref(req->sreq);
}
@@ -549,6 +550,7 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
return -ENOENT;
}
virtio_scsi_ctx_check(s, d);
+ printf("prepare %lx %x\n", req->req.cmd.tag, req->req.cmd.cdb[0]);
req->sreq = scsi_req_new(d, req->req.cmd.tag,
virtio_scsi_get_lun(req->req.cmd.lun),
req->req.cmd.cdb, req);
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 62001b4..c75dec3 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -336,11 +336,13 @@ static int virtio_pci_ioeventfd_assign(DeviceState *d, EventNotifier *notifier,
static void virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy)
{
+ printf("start ioeventfd %s\n", object_class_get_name(object_get_class(OBJECT(proxy))));
virtio_bus_start_ioeventfd(&proxy->bus);
}
static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy)
{
+ printf("stop ioeventfd %s\n", object_class_get_name(object_get_class(OBJECT(proxy))));
virtio_bus_stop_ioeventfd(&proxy->bus);
}
@@ -376,6 +378,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
}
break;
case VIRTIO_PCI_STATUS:
+ printf("set status %s %x\n", object_class_get_name(object_get_class(OBJECT(proxy))), val & 0xFF);
if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
virtio_pci_stop_ioeventfd(proxy);
}
@@ -1274,6 +1277,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
vdev->config_vector = val;
break;
case VIRTIO_PCI_COMMON_STATUS:
+ printf("set status %s %x\n", object_class_get_name(object_get_class(OBJECT(proxy))), (uint8_t)val);
if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
virtio_pci_stop_ioeventfd(proxy);
}
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 89b0b80..9c894d7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2018,7 +2018,7 @@ EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq)
return &vq->guest_notifier;
}
-static void virtio_queue_host_notifier_aio_read(EventNotifier *n)
+void virtio_queue_host_notifier_aio_read(EventNotifier *n)
{
VirtQueue *vq = container_of(n, VirtQueue, host_notifier);
if (event_notifier_test_and_clear(n)) {
@@ -2046,6 +2046,9 @@ void virtio_queue_host_notifier_read(EventNotifier *n)
{
VirtQueue *vq = container_of(n, VirtQueue, host_notifier);
if (event_notifier_test_and_clear(n)) {
+ VirtIODevice *vdev = vq->vdev;
+
+ printf("virtio_queue_host_notifier_read %ld\n", vq - vdev->vq);
virtio_queue_notify_vq(vq);
}
}
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 35ede30..d3dfc69 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -274,6 +274,7 @@ int virtio_device_grab_ioeventfd(VirtIODevice *vdev);
void virtio_device_release_ioeventfd(VirtIODevice *vdev);
bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev);
EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
+void virtio_queue_host_notifier_aio_read(EventNotifier *n);
void virtio_queue_host_notifier_read(EventNotifier *n);
void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
void (*fn)(VirtIODevice *,
> Thread 3 "CPU 0/KVM" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7fb4f73ba700 (LWP 2713)]
> 0x00005593dacc2800 in virtio_queue_notify_aio_vq (vq=0x5593dd4a7378) at /net/gimli/home/alwillia/Work/qemu.git/hw/virtio/virtio.c:1242
> 1242 trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
> (gdb) bt
> #0 0x00005593dacc2800 in virtio_queue_notify_aio_vq (vq=0x5593dd4a7378) at /net/gimli/home/alwillia/Work/qemu.git/hw/virtio/virtio.c:1242
> #1 0x00005593dacc4a4e in virtio_queue_host_notifier_aio_read (n=0x5593dd4a73d8) at /net/gimli/home/alwillia/Work/qemu.git/hw/virtio/virtio.c:2025
> #2 0x00005593daca4997 in virtio_scsi_dataplane_stop (vdev=0x5593dc0cc0f0) at /net/gimli/home/alwillia/Work/qemu.git/hw/scsi/virtio-scsi-dataplane.c:209
> #3 0x00005593daf6a4b7 in virtio_bus_stop_ioeventfd (bus=0x5593dc0cc078) at hw/virtio/virtio-bus.c:219
> #4 0x00005593daf64279 in virtio_pci_stop_ioeventfd (proxy=0x5593dc0c3ce0) at hw/virtio/virtio-pci.c:344
> #5 0x00005593daf643d5 in virtio_ioport_write (opaque=0x5593dc0c3ce0, addr=18, val=0) at hw/virtio/virtio-pci.c:380
> #6 0x00005593daf6484d in virtio_pci_config_write (opaque=0x5593dc0c3ce0, addr=18, val=0, size=1) at hw/virtio/virtio-pci.c:508
> #7 0x00005593dac592fd in memory_region_write_accessor (mr=0x5593dc0c45d0, addr=18, value=0x7fb4f73b74b8, size=1, shift=0, mask=255, attrs=...)
> at /net/gimli/home/alwillia/Work/qemu.git/memory.c:526
> #8 0x00005593dac59515 in access_with_adjusted_size (addr=18, value=0x7fb4f73b74b8, size=1, access_size_min=1, access_size_max=4, access=
> 0x5593dac59213 <memory_region_write_accessor>, mr=0x5593dc0c45d0, attrs=...) at /net/gimli/home/alwillia/Work/qemu.git/memory.c:592
> #9 0x00005593dac5bc55 in memory_region_dispatch_write (mr=0x5593dc0c45d0, addr=18, data=0, size=1, attrs=...) at /net/gimli/home/alwillia/Work/qemu.git/memory.c:1323
> #10 0x00005593dac07583 in address_space_write_continue (as=0x5593db727de0 <address_space_io>, addr=49298, attrs=..., buf=0x7fb520354000 "", len=1, addr1=18, l=1, mr=0x5593dc0c45d0)
> at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2621
> #11 0x00005593dac076cb in address_space_write (as=0x5593db727de0 <address_space_io>, addr=49298, attrs=..., buf=0x7fb520354000 "", len=1)
> at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2666
> #12 0x00005593dac07a57 in address_space_rw (as=0x5593db727de0 <address_space_io>, addr=49298, attrs=..., buf=0x7fb520354000 "", len=1, is_write=true)
> at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2768
> #13 0x00005593dac558d7 in kvm_handle_io (port=49298, attrs=..., data=0x7fb520354000, direction=1, size=1, count=1) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1800
> #14 0x00005593dac55ddd in kvm_cpu_exec (cpu=0x5593dc0a6490) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1958
> #15 0x00005593dac3cc58 in qemu_kvm_cpu_thread_fn (arg=0x5593dc0a6490) at /net/gimli/home/alwillia/Work/qemu.git/cpus.c:998
> #16 0x00007fb5054715ca in start_thread (arg=0x7fb4f73ba700) at pthread_create.c:333
> #17 0x00007fb5051ab0ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
>
>> And if it doesn't work here is some printf debugging. It's pretty verbose but
>> the interesting part starts pretty much where you issue the virsh shutdown or
>> system_powerdown command:
>>
>> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
>> index f2ea29d..ec0f750 100644
>> --- a/hw/scsi/virtio-scsi-dataplane.c
>> +++ b/hw/scsi/virtio-scsi-dataplane.c
>> @@ -108,11 +108,13 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s)
>> VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
>> int i;
>>
>> + printf("before clear\n");
>> virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, NULL);
>> virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, NULL);
>> for (i = 0; i < vs->conf.num_queues; i++) {
>> virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, NULL);
>> }
>> + printf("after clear\n");
>> }
>>
>> /* Context: QEMU global mutex held */
>> @@ -202,15 +204,20 @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
>>
>> aio_context_acquire(s->ctx);
>> virtio_scsi_clear_aio(s);
>> - aio_context_release(s->ctx);
>> -
>> - blk_drain_all(); /* ensure there are no in-flight requests */
>>
>> for (i = 0; i < vs->conf.num_queues + 2; i++) {
>> + VirtQueue *vq = virtio_get_queue(vdev, i);
>> virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
>> + virtio_queue_host_notifier_aio_read(virtio_queue_get_guest_notifier(vq));
>> }
>> + aio_context_release(s->ctx);
>> +
>> + printf("before drain\n");
>> + blk_drain_all(); /* ensure there are no in-flight requests */
>> + printf("after drain\n");
>>
>> /* Clean up guest notifier (irq) */
>> + printf("end of virtio_scsi_dataplane_stop\n");
>> k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
>> s->dataplane_stopping = false;
>> s->dataplane_started = false;
>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>> index 3e5ae6a..e8b83d4 100644
>> --- a/hw/scsi/virtio-scsi.c
>> +++ b/hw/scsi/virtio-scsi.c
>> @@ -75,6 +75,7 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
>> }
>>
>> if (req->sreq) {
>> + printf("finish %x\n", req->sreq->tag);
>> req->sreq->hba_private = NULL;
>> scsi_req_unref(req->sreq);
>> }
>> @@ -549,6 +549,7 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
>> return -ENOENT;
>> }
>> virtio_scsi_ctx_check(s, d);
>> + printf("prepare %lx %x\n", req->req.cmd.tag, req->req.cmd.cdb[0]);
>> req->sreq = scsi_req_new(d, req->req.cmd.tag,
>> virtio_scsi_get_lun(req->req.cmd.lun),
>> req->req.cmd.cdb, req);
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 62001b4..c75dec3 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -336,11 +336,13 @@ static int virtio_pci_ioeventfd_assign(DeviceState *d, EventNotifier *notifier,
>>
>> static void virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy)
>> {
>> + printf("start ioeventfd %s\n", object_class_get_name(object_get_class(OBJECT(proxy))));
>> virtio_bus_start_ioeventfd(&proxy->bus);
>> }
>>
>> static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy)
>> {
>> + printf("stop ioeventfd %s\n", object_class_get_name(object_get_class(OBJECT(proxy))));
>> virtio_bus_stop_ioeventfd(&proxy->bus);
>> }
>>
>> @@ -376,6 +378,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>> }
>> break;
>> case VIRTIO_PCI_STATUS:
>> + printf("set status %s %x\n", object_class_get_name(object_get_class(OBJECT(proxy))), val & 0xFF);
>> if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
>> virtio_pci_stop_ioeventfd(proxy);
>> }
>> @@ -1274,6 +1277,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
>> vdev->config_vector = val;
>> break;
>> case VIRTIO_PCI_COMMON_STATUS:
>> + printf("set status %s %x\n", object_class_get_name(object_get_class(OBJECT(proxy))), (uint8_t)val);
>> if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
>> virtio_pci_stop_ioeventfd(proxy);
>> }
>>
>
> This required making virtio_queue_host_notifier_aio_read() non-static
> and adding the forward declaration, stolen from the first patch. The
> attached log starts at the point where there guest is idle and I issue
> a virsh shutdown. This also results in a segfault nearly identical to
> above:
>
> Thread 4 "CPU 1/KVM" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7f7194901700 (LWP 3804)]
> 0x000056358070788a in virtio_queue_notify_aio_vq (vq=0x56358310d378) at /net/gimli/home/alwillia/Work/qemu.git/hw/virtio/virtio.c:1242
> 1242 trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
> (gdb) bt
> #0 0x000056358070788a in virtio_queue_notify_aio_vq (vq=0x56358310d378) at /net/gimli/home/alwillia/Work/qemu.git/hw/virtio/virtio.c:1242
> #1 0x0000563580709ad8 in virtio_queue_host_notifier_aio_read (n=0x56358310d3d8) at /net/gimli/home/alwillia/Work/qemu.git/hw/virtio/virtio.c:2025
> #2 0x00005635806e99fd in virtio_scsi_dataplane_stop (vdev=0x563581d320f0) at /net/gimli/home/alwillia/Work/qemu.git/hw/scsi/virtio-scsi-dataplane.c:211
> #3 0x00005635809af5fe in virtio_bus_stop_ioeventfd (bus=0x563581d32078) at hw/virtio/virtio-bus.c:219
> #4 0x00005635809a9353 in virtio_pci_stop_ioeventfd (proxy=0x563581d29ce0) at hw/virtio/virtio-pci.c:346
> #5 0x00005635809a94e0 in virtio_ioport_write (opaque=0x563581d29ce0, addr=18, val=0) at hw/virtio/virtio-pci.c:383
> #6 0x00005635809a995d in virtio_pci_config_write (opaque=0x563581d29ce0, addr=18, val=0, size=1) at hw/virtio/virtio-pci.c:511
> #7 0x000056358069e2fd in memory_region_write_accessor (mr=0x563581d2a5d0, addr=18, value=0x7f71948fe4b8, size=1, shift=0, mask=255, attrs=...)
> at /net/gimli/home/alwillia/Work/qemu.git/memory.c:526
> #8 0x000056358069e515 in access_with_adjusted_size (addr=18, value=0x7f71948fe4b8, size=1, access_size_min=1, access_size_max=4, access=
> 0x56358069e213 <memory_region_write_accessor>, mr=0x563581d2a5d0, attrs=...) at /net/gimli/home/alwillia/Work/qemu.git/memory.c:592
> #9 0x00005635806a0c55 in memory_region_dispatch_write (mr=0x563581d2a5d0, addr=18, data=0, size=1, attrs=...) at /net/gimli/home/alwillia/Work/qemu.git/memory.c:1323
> #10 0x000056358064c583 in address_space_write_continue (as=0x56358116cde0 <address_space_io>, addr=49298, attrs=..., buf=0x7f71be099000 "", len=1, addr1=18, l=1, mr=0x563581d2a5d0)
> at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2621
> #11 0x000056358064c6cb in address_space_write (as=0x56358116cde0 <address_space_io>, addr=49298, attrs=..., buf=0x7f71be099000 "", len=1)
> at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2666
> #12 0x000056358064ca57 in address_space_rw (as=0x56358116cde0 <address_space_io>, addr=49298, attrs=..., buf=0x7f71be099000 "", len=1, is_write=true)
> at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2768
> #13 0x000056358069a8d7 in kvm_handle_io (port=49298, attrs=..., data=0x7f71be099000, direction=1, size=1, count=1) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1800
> #14 0x000056358069addd in kvm_cpu_exec (cpu=0x563581d6d030) at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1958
> #15 0x0000563580681c58 in qemu_kvm_cpu_thread_fn (arg=0x563581d6d030) at /net/gimli/home/alwillia/Work/qemu.git/cpus.c:998
> #16 0x00007f71a31b95ca in start_thread (arg=0x7f7194901700) at pthread_create.c:333
> #17 0x00007f71a2ef30ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
>
> If you care to match line numbers, my tree is based on
> 6bbcb76301a72dc80c8d29af13d40bb9a759c9c6, it includes you patch:
>
> virtio: introduce grab/release_ioeventfd to fix vhost
>
> Plus your first fix removing the assert and return 0 case from
> virtio_bus_set_host_notifier(). Thanks,
>
> Alex
>
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH 07/13] virtio-scsi: always use dataplane path if ioeventfd is active
2016-11-14 18:10 ` Paolo Bonzini
@ 2016-11-14 18:49 ` Alex Williamson
2016-11-14 19:33 ` Paolo Bonzini
0 siblings, 1 reply; 27+ messages in thread
From: Alex Williamson @ 2016-11-14 18:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: cornelia.huck, famz, qemu-devel, stefanha, mst
[-- Attachment #1: Type: text/plain, Size: 686 bytes --]
On Mon, 14 Nov 2016 19:10:54 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 14/11/2016 18:09, Alex Williamson wrote:
> > Hmm, fixed yet not fixed. I get a nice shutdown and it even eliminates
> > a cpu spike shown in virt-manager at the end of shutdown that was
> > typical previously, but then I noticed dmesg showing me segfaults, so I
> > hooked up gdb and:
>
> Well, that I don't get the segfault already says something... Though
> it's not even clear from the backtrace what is causing the segfault.
> Let's try the same printf without the change.
Log starting from virsh shutdown. The final line occurs just as the VM
starts spinning on all vcpus. Thanks,
Alex
[-- Attachment #2: shutdown.log.bz2 --]
[-- Type: application/x-bzip, Size: 6221 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 07/13] virtio-scsi: always use dataplane path if ioeventfd is active
2016-11-14 18:49 ` Alex Williamson
@ 2016-11-14 19:33 ` Paolo Bonzini
2016-11-14 19:48 ` Alex Williamson
0 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2016-11-14 19:33 UTC (permalink / raw)
To: Alex Williamson; +Cc: cornelia.huck, famz, qemu-devel, stefanha, mst
On 14/11/2016 19:49, Alex Williamson wrote:
> On Mon, 14 Nov 2016 19:10:54 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> On 14/11/2016 18:09, Alex Williamson wrote:
>>> Hmm, fixed yet not fixed. I get a nice shutdown and it even eliminates
>>> a cpu spike shown in virt-manager at the end of shutdown that was
>>> typical previously, but then I noticed dmesg showing me segfaults, so I
>>> hooked up gdb and:
>>
>> Well, that I don't get the segfault already says something... Though
>> it's not even clear from the backtrace what is causing the segfault.
>> Let's try the same printf without the change.
>
> Log starting from virsh shutdown. The final line occurs just as the VM
> starts spinning on all vcpus. Thanks,
Ok, this could be a start. Just for information what is your virtio-win
version?
Paolo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 07/13] virtio-scsi: always use dataplane path if ioeventfd is active
2016-11-14 19:33 ` Paolo Bonzini
@ 2016-11-14 19:48 ` Alex Williamson
0 siblings, 0 replies; 27+ messages in thread
From: Alex Williamson @ 2016-11-14 19:48 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: cornelia.huck, famz, qemu-devel, stefanha, mst
On Mon, 14 Nov 2016 20:33:32 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 14/11/2016 19:49, Alex Williamson wrote:
> > On Mon, 14 Nov 2016 19:10:54 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> >> On 14/11/2016 18:09, Alex Williamson wrote:
> >>> Hmm, fixed yet not fixed. I get a nice shutdown and it even eliminates
> >>> a cpu spike shown in virt-manager at the end of shutdown that was
> >>> typical previously, but then I noticed dmesg showing me segfaults, so I
> >>> hooked up gdb and:
> >>
> >> Well, that I don't get the segfault already says something... Though
> >> it's not even clear from the backtrace what is causing the segfault.
> >> Let's try the same printf without the change.
> >
> > Log starting from virsh shutdown. The final line occurs just as the VM
> > starts spinning on all vcpus. Thanks,
>
> Ok, this could be a start. Just for information what is your virtio-win
> version?
Device manager shows:
Driver Date: 9/22/2015
Driver Version: 62.72.104.11000
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 08/13] Revert "virtio: Introduce virtio_add_queue_aio"
2016-10-21 20:48 [Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop Paolo Bonzini
` (6 preceding siblings ...)
2016-10-21 20:48 ` [Qemu-devel] [PATCH 07/13] virtio-scsi: " Paolo Bonzini
@ 2016-10-21 20:48 ` Paolo Bonzini
2016-10-21 20:48 ` [Qemu-devel] [PATCH 09/13] virtio: remove set_handler argument from set_host_notifier_internal Paolo Bonzini
` (5 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-10-21 20:48 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, stefanha, famz
This reverts commit 872dd82c83745a603d2e07a03d34313eb6467ae4.
virtio_add_queue_aio is unused.
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.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 c0c7856..1972b23 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;
@@ -1287,9 +1286,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;
@@ -1306,28 +1304,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) {
@@ -2062,21 +2042,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 5d43f7e..0e14c2c 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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Qemu-devel] [PATCH 09/13] virtio: remove set_handler argument from set_host_notifier_internal
2016-10-21 20:48 [Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop Paolo Bonzini
` (7 preceding siblings ...)
2016-10-21 20:48 ` [Qemu-devel] [PATCH 08/13] Revert "virtio: Introduce virtio_add_queue_aio" Paolo Bonzini
@ 2016-10-21 20:48 ` Paolo Bonzini
2016-10-24 9:04 ` Cornelia Huck
2016-10-21 20:48 ` [Qemu-devel] [PATCH 10/13] virtio: remove ioeventfd_disabled altogether Paolo Bonzini
` (4 subsequent siblings)
13 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2016-10-21 20:48 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, stefanha, famz
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>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.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 3ffec66..a619445 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);
@@ -257,7 +251,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 1972b23..0e0bb46 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2138,11 +2138,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;
@@ -2152,7 +2162,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;
@@ -2176,7 +2186,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 */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH 09/13] virtio: remove set_handler argument from set_host_notifier_internal
2016-10-21 20:48 ` [Qemu-devel] [PATCH 09/13] virtio: remove set_handler argument from set_host_notifier_internal Paolo Bonzini
@ 2016-10-24 9:04 ` Cornelia Huck
0 siblings, 0 replies; 27+ messages in thread
From: Cornelia Huck @ 2016-10-24 9:04 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, mst, stefanha, famz
On Fri, 21 Oct 2016 22:48:12 +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>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.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 1972b23..0e0bb46 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2138,11 +2138,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];;
There's still an extra semicolon here.
> + if (!vq->vring.num) {
> + continue;
> + }
> + event_notifier_set(&vq->host_notifier);
> }
> return 0;
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 10/13] virtio: remove ioeventfd_disabled altogether
2016-10-21 20:48 [Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop Paolo Bonzini
` (8 preceding siblings ...)
2016-10-21 20:48 ` [Qemu-devel] [PATCH 09/13] virtio: remove set_handler argument from set_host_notifier_internal Paolo Bonzini
@ 2016-10-21 20:48 ` Paolo Bonzini
2016-10-21 20:48 ` [Qemu-devel] [PATCH 11/13] virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd Paolo Bonzini
` (3 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-10-21 20:48 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, stefanha, famz
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Qemu-devel] [PATCH 11/13] virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd
2016-10-21 20:48 [Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop Paolo Bonzini
` (9 preceding siblings ...)
2016-10-21 20:48 ` [Qemu-devel] [PATCH 10/13] virtio: remove ioeventfd_disabled altogether Paolo Bonzini
@ 2016-10-21 20:48 ` Paolo Bonzini
2016-10-21 20:48 ` [Qemu-devel] [PATCH 12/13] virtio: inline virtio_queue_set_host_notifier_fd_handler Paolo Bonzini
` (2 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-10-21 20:48 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, stefanha, famz
ioeventfd_disabled was the only reason for the default
implementation of virtio_device_start_ioeventfd not to use
virtio_bus_set_host_notifier. This is now fixed, and the sole entry
point to set up ioeventfd can be virtio_bus_set_host_notifier.
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.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 b0e4544..fd105b8 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 0e0bb46..8b74600 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2131,14 +2131,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;
@@ -2162,7 +2161,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;
@@ -2179,14 +2178,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 */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Qemu-devel] [PATCH 12/13] virtio: inline virtio_queue_set_host_notifier_fd_handler
2016-10-21 20:48 [Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop Paolo Bonzini
` (10 preceding siblings ...)
2016-10-21 20:48 ` [Qemu-devel] [PATCH 11/13] virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd Paolo Bonzini
@ 2016-10-21 20:48 ` Paolo Bonzini
2016-10-21 20:48 ` [Qemu-devel] [PATCH 13/13] virtio: inline set_host_notifier_internal Paolo Bonzini
2016-10-24 13:08 ` [Qemu-devel] [14/13] fixup! virtio: remove set_handler argument from set_host_notifier_internal Paolo Bonzini
13 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-10-21 20:48 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, stefanha, famz
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.
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v2->v3: fix "before after" in comment [Cornelia]
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 fd105b8..670746c 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 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 8b74600..5e5d09a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2031,7 +2031,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)) {
@@ -2039,22 +2039,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;
@@ -2134,6 +2118,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;
}
@@ -2142,7 +2127,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++) {
@@ -2157,10 +2143,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);
}
@@ -2181,9 +2169,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 0e14c2c..f9d9407 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -268,8 +268,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 *));
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Qemu-devel] [PATCH 13/13] virtio: inline set_host_notifier_internal
2016-10-21 20:48 [Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop Paolo Bonzini
` (11 preceding siblings ...)
2016-10-21 20:48 ` [Qemu-devel] [PATCH 12/13] virtio: inline virtio_queue_set_host_notifier_fd_handler Paolo Bonzini
@ 2016-10-21 20:48 ` Paolo Bonzini
2016-10-24 13:08 ` [Qemu-devel] [14/13] fixup! virtio: remove set_handler argument from set_host_notifier_internal Paolo Bonzini
13 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-10-21 20:48 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, stefanha, famz
This is only called from virtio_bus_set_host_notifier.
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
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 670746c..bf61f66 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 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);
@@ -234,20 +199,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 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)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Qemu-devel] [14/13] fixup! virtio: remove set_handler argument from set_host_notifier_internal
2016-10-21 20:48 [Qemu-devel] [PATCH v3 00/13] virtio: cleanup ioeventfd start/stop Paolo Bonzini
` (12 preceding siblings ...)
2016-10-21 20:48 ` [Qemu-devel] [PATCH 13/13] virtio: inline set_host_notifier_internal Paolo Bonzini
@ 2016-10-24 13:08 ` Paolo Bonzini
13 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-10-24 13:08 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, stefanha, famz
---
hw/virtio/virtio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 0e0bb46..1e2136d 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2148,7 +2148,7 @@ static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
/* Kick right away to begin processing requests already in vring */
- VirtQueue *vq = &vdev->vq[n];;
+ VirtQueue *vq = &vdev->vq[n];
if (!vq->vring.num) {
continue;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread