Re: [Qemu-devel] [PATCH 3/9] dataplane: remove EventPoll in favor of AioContext

2013-03-08 Thread Christian Borntraeger
On 04/03/13 10:15, Stefan Hajnoczi wrote:
 From: Paolo Bonzini pbonz...@redhat.com
 
 During the review of the dataplane code, the EventPoll API morphed itself
 (not concidentially) into something very very similar to an AioContext.
 Thus, it is trivial to convert virtio-blk-dataplane to use AioContext,
 and a first baby step towards letting dataplane talk directly to the
 QEMU block layer.
 
 The only interesting note is the value-copy of EventNotifiers.  At least
 in my opinion this is part of the EventNotifier API and is even portable
 to Windows.  Of course, in this case you should not close the notifier's
 underlying file descriptors or handle with event_notifier_cleanup.
 
 Signed-off-by: Paolo Bonzini pbonz...@redhat.com
 Signed-off-by: Stefan Hajnoczi stefa...@redhat.com

Hmm, this broke data plane on our internal notifier prototype code on virtio-ccw
(attached for reference)
[...]


 +/* Note that these EventNotifiers are assigned by value.  This is
 + * fine as long as you do not call event_notifier_cleanup on them
 + * (because you don't own the file descriptor or handle; you just
 + * use it).
 + */

And this might be the reason. Currently we only have eventfd to wire up 
guest_to_host notifiers. The host_to_guest notification is not done
via vectors/irqfd, instead we let our qemu transport listen to the irq
eventfd. Worked fine so far with vhost and dataplane without this patch.

Any ideas how to properly enable a transport that has full host notifiers
but only poor mans guest notifiers?

Christian

From 76ceaec73c44f71b2e703accb157c09fef94ccd1 Mon Sep 17 00:00:00 2001
From: Cornelia Huck cornelia.h...@de.ibm.com
Date: Tue, 19 Feb 2013 13:48:17 +0100
Subject: [PATCH 23/28] Re-add guest/host notifiers.

Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 hw/s390x/virtio-ccw.c | 90 +++
 hw/s390x/virtio-ccw.h |  1 +
 2 files changed, 91 insertions(+)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 63c851f..d4fa42a 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -95,6 +95,7 @@ static void virtio_ccw_start_ioeventfd(VirtioCcwDevice *dev)
 int n, r;
 
 if (!(dev-flags  VIRTIO_CCW_FLAG_USE_IOEVENTFD) ||
+dev-ioeventfd_disabled ||
 dev-ioeventfd_started) {
 return;
 }
@@ -793,6 +794,89 @@ static void virtio_ccw_vmstate_change(DeviceState *d, bool running)
 }
 }
 
+static bool virtio_ccw_query_guest_notifiers(DeviceState *d)
+{
+VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+
+return !!(dev-sch-curr_status.pmcw.flags  PMCW_FLAGS_MASK_ENA);
+}
+
+static int virtio_ccw_set_host_notifier(DeviceState *d, int n, bool assign)
+{
+VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+
+/* Stop using the generic ioeventfd, we are doing eventfd handling
+ * ourselves below */
+dev-ioeventfd_disabled = assign;
+if (assign) {
+virtio_ccw_stop_ioeventfd(dev);
+}
+return virtio_ccw_set_guest2host_notifier(dev, n, assign, false);
+}
+
+static int virtio_ccw_set_guest_notifier(VirtioCcwDevice *dev, int n,
+ bool assign, bool with_irqfd)
+{
+VirtQueue *vq = virtio_get_queue(dev-vdev, n);
+EventNotifier *notifier = virtio_queue_get_guest_notifier(vq);
+
+if (assign) {
+int r = event_notifier_init(notifier, 0);
+
+if (r  0) {
+return r;
+}
+virtio_queue_set_guest_notifier_fd_handler(vq, true, with_irqfd);
+/* We do not support irqfd for classic I/O interrupts, because the
+ * classic interrupts are intermixed with the subchannel status, that
+ * is queried with test subchannel. We want to use vhost, though.
+ * Lets make sure to have vhost running and wire up the irq fd to 
+ * land in qemu (and only the irq fd) in this code.
+ */
+if (dev-vdev-guest_notifier_mask) {
+dev-vdev-guest_notifier_mask(dev-vdev, n, false);
+}
+/* get lost events and re-inject */
+if (dev-vdev-guest_notifier_pending 
+dev-vdev-guest_notifier_pending(dev-vdev, n)) {
+event_notifier_set(notifier);
+}
+} else {
+if (dev-vdev-guest_notifier_mask) {
+dev-vdev-guest_notifier_mask(dev-vdev, n, true);
+}
+virtio_queue_set_guest_notifier_fd_handler(vq, false, with_irqfd);
+event_notifier_cleanup(notifier);
+}
+return 0;
+}
+
+static int virtio_ccw_set_guest_notifiers(DeviceState *d, int nvqs,
+  bool assigned)
+{
+VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+VirtIODevice *vdev = dev-vdev;
+int r, n;
+
+for (n = 0; n  nvqs; n++) {
+if (!virtio_queue_get_num(vdev, n)) {
+break;
+}
+/* false - true, as soon as irqfd works */
+r = virtio_ccw_set_guest_notifier(dev, n, assigned, false);
+if (r  

Re: [Qemu-devel] [PATCH 3/9] dataplane: remove EventPoll in favor of AioContext

2013-03-08 Thread Paolo Bonzini
Il 08/03/2013 09:37, Christian Borntraeger ha scritto:
 +if (assign) {
 +int r = event_notifier_init(notifier, 0);
 +
 +if (r  0) {
 +return r;
 +}
 +virtio_queue_set_guest_notifier_fd_handler(vq, true, with_irqfd);

Perhaps you can simply hard-code with_irqfd to false in this call to
virtio_queue_set_guest_notifier_fd_handler and the one below?  Then the
guest notifier will be emulated in userspace and processed via
vdev-binding-notify.

You will not need to overwrite the EventNotifier which is IMO a pretty
ufly violation of encapsulation. :)

Paolo

 +/* We do not support irqfd for classic I/O interrupts, because the
 + * classic interrupts are intermixed with the subchannel status, that
 + * is queried with test subchannel. We want to use vhost, though.
 + * Lets make sure to have vhost running and wire up the irq fd to 
 + * land in qemu (and only the irq fd) in this code.
 + */
 +if (dev-vdev-guest_notifier_mask) {
 +dev-vdev-guest_notifier_mask(dev-vdev, n, false);
 +}
 +/* get lost events and re-inject */
 +if (dev-vdev-guest_notifier_pending 
 +dev-vdev-guest_notifier_pending(dev-vdev, n)) {
 +event_notifier_set(notifier);
 +}
 +} else {
 +if (dev-vdev-guest_notifier_mask) {
 +dev-vdev-guest_notifier_mask(dev-vdev, n, true);
 +}
 +virtio_queue_set_guest_notifier_fd_handler(vq, false, with_irqfd);
 +event_notifier_cleanup(notifier);




Re: [Qemu-devel] [PATCH 3/9] dataplane: remove EventPoll in favor of AioContext

2013-03-08 Thread Cornelia Huck
On Fri, 08 Mar 2013 10:22:36 +0100
Paolo Bonzini pbonz...@redhat.com wrote:

 Il 08/03/2013 09:37, Christian Borntraeger ha scritto:
  +if (assign) {
  +int r = event_notifier_init(notifier, 0);
  +
  +if (r  0) {
  +return r;
  +}
  +virtio_queue_set_guest_notifier_fd_handler(vq, true, with_irqfd);
 
 Perhaps you can simply hard-code with_irqfd to false in this call to
 virtio_queue_set_guest_notifier_fd_handler and the one below?  Then the
 guest notifier will be emulated in userspace and processed via
 vdev-binding-notify.

Well, effectively with_irqfd is already hardcoded to false (as
virtio_ccw_set_guest_notifiers() always calls this function with
with_irqfd=false), so that doesn't seem to be the problem here.

 
 You will not need to overwrite the EventNotifier which is IMO a pretty
 ufly violation of encapsulation. :)
 
 Paolo
 
  +/* We do not support irqfd for classic I/O interrupts, because the
  + * classic interrupts are intermixed with the subchannel status, 
  that
  + * is queried with test subchannel. We want to use vhost, though.
  + * Lets make sure to have vhost running and wire up the irq fd to 
  + * land in qemu (and only the irq fd) in this code.
  + */
  +if (dev-vdev-guest_notifier_mask) {
  +dev-vdev-guest_notifier_mask(dev-vdev, n, false);
  +}
  +/* get lost events and re-inject */
  +if (dev-vdev-guest_notifier_pending 
  +dev-vdev-guest_notifier_pending(dev-vdev, n)) {
  +event_notifier_set(notifier);
  +}
  +} else {
  +if (dev-vdev-guest_notifier_mask) {
  +dev-vdev-guest_notifier_mask(dev-vdev, n, true);
  +}
  +virtio_queue_set_guest_notifier_fd_handler(vq, false, with_irqfd);
  +event_notifier_cleanup(notifier);
 




Re: [Qemu-devel] [PATCH 3/9] dataplane: remove EventPoll in favor of AioContext

2013-03-08 Thread Paolo Bonzini
Il 08/03/2013 13:44, Cornelia Huck ha scritto:
  Perhaps you can simply hard-code with_irqfd to false in this call to
  virtio_queue_set_guest_notifier_fd_handler and the one below?  Then the
  guest notifier will be emulated in userspace and processed via
  vdev-binding-notify.
 
 Well, effectively with_irqfd is already hardcoded to false (as
 virtio_ccw_set_guest_notifiers() always calls this function with
 with_irqfd=false), so that doesn't seem to be the problem here.

Actually, the guest-host notifier is not touched by either the old or
the new code.  The code I modified only worries about the host-guest
notifier.

How did you track the problem to the assignment by value of EventNotifiers?

BTW:

 You will not need to overwrite the EventNotifier which is IMO a pretty
 ufly violation of encapsulation. 

This was nonsense. :)

Paolo