[PATCH 1/1] kvm: qemu: Add VIRTIO_F_NOTIFY_ON_EMPTY

[PATCH 1/1] kvm: qemu: Add VIRTIO_F_NOTIFY_ON_EMPTY

[Mark McLoughlin]

Set the VIRTIO_F_NOTIFY_ON_EMPTY feature bit so the
guest can rely on us notifying them when the queue
is empty.

Also, only notify when the available queue is empty
*and* when we've finished with all the buffers we
had detached. Right now, when the queue is empty,
we notify the guest for every used buffer.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 qemu/hw/virtio.c |    6 +++++-
 qemu/hw/virtio.h |    5 +++++
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
index 2ea1ebe..3119ea9 100644
--- a/qemu/hw/virtio.c
+++ b/qemu/hw/virtio.c
@@ -138,6 +138,7 @@ void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
     /* Make sure buffer is written before we update index. */
     wmb();
     vq->vring.used->idx++;
+    vq->inuse--;
 }
 
 int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
@@ -187,6 +188,8 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
 
     elem->index = head;
 
+    vq->inuse++;
+
     return elem->in_num + elem->out_num;
 }
 
@@ -274,6 +277,7 @@ static uint32_t virtio_ioport_read(void *opaque, uint32_t addr)
     switch (addr) {
     case VIRTIO_PCI_HOST_FEATURES:
 	ret = vdev->get_features(vdev);
+	ret |= (1 << VIRTIO_F_NOTIFY_ON_EMPTY);
 	break;
     case VIRTIO_PCI_GUEST_FEATURES:
 	ret = vdev->features;
@@ -430,7 +434,7 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
 void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
 {
     /* Always notify when queue is empty */
-    if (vq->vring.avail->idx != vq->last_avail_idx &&
+    if ((vq->inuse || vq->vring.avail->idx != vq->last_avail_idx)) &&
 	(vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT))
 	return;
 
diff --git a/qemu/hw/virtio.h b/qemu/hw/virtio.h
index 61f5038..1adaed3 100644
--- a/qemu/hw/virtio.h
+++ b/qemu/hw/virtio.h
@@ -30,6 +30,10 @@
 /* We've given up on this device. */
 #define VIRTIO_CONFIG_S_FAILED		0x80
 
+/* We notify when the ring is completely used, even if the guest is supressing
+ * callbacks */
+#define VIRTIO_F_NOTIFY_ON_EMPTY        24
+
 /* from Linux's linux/virtio_ring.h */
 
 /* This marks a buffer as continuing via the next field. */
@@ -86,6 +90,7 @@ struct VirtQueue
     VRing vring;
     uint32_t pfn;
     uint16_t last_avail_idx;
+    int inuse;
     void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq);
 };
 
-- 
1.5.4.1

Re: [PATCH 1/1] kvm: qemu: Add VIRTIO_F_NOTIFY_ON_EMPTY

[Avi Kivity]

Mark McLoughlin wrote:
> Set the VIRTIO_F_NOTIFY_ON_EMPTY feature bit so the
> guest can rely on us notifying them when the queue
> is empty.
>
> Also, only notify when the available queue is empty
> *and* when we've finished with all the buffers we
> had detached. Right now, when the queue is empty,
> we notify the guest for every used buffer.
>   

This is a bit inflexible.  I would prefer a mechanism where the guest 
tells us at what queue index it would like the notification, like Xen.  
If it wants it on queue empty, it just sets the index appropriately.

Or is the usage in the drivers already upstream and I have no choice 
about applying this?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 1/1] kvm: qemu: Add VIRTIO_F_NOTIFY_ON_EMPTY

[Mark McLoughlin]

On Wed, 2008-06-18 at 06:31 -0700, Avi Kivity wrote:
> Mark McLoughlin wrote:
> > Set the VIRTIO_F_NOTIFY_ON_EMPTY feature bit so the
> > guest can rely on us notifying them when the queue
> > is empty.
> >
> > Also, only notify when the available queue is empty
> > *and* when we've finished with all the buffers we
> > had detached. Right now, when the queue is empty,
> > we notify the guest for every used buffer.
> >   
> 
> This is a bit inflexible.  I would prefer a mechanism where the guest 
> tells us at what queue index it would like the notification, like Xen.  
> If it wants it on queue empty, it just sets the index appropriately.
> 
> Or is the usage in the drivers already upstream and I have no choice 
> about applying this?

Something like that :-)

commits:

  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=b4f68be6c5d507afdcd74f5be3df0b1209cda503
  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=363f15149cfba67d29f1e6a6103dda079f27f3fa
  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=20887611523e749d99cc7d64ff6c97d27529fbae

Cheers,
Mark.