[Qemu-devel] [PATCH 0/3] Fixes for virtio-rng

[Qemu-devel] [PATCH 0/3] Fixes for virtio-rng

[Amit Shah]

Hey Anthony,

Here are the fixes for the virtio-rng code -- ideally these should be
folded in the series before you merge, but if the original series is
ready to apply, these can go in as follow-on patches.


Amit Shah (3):
  virtio-rng: use virtqueue_get_avail_bytes, fix migration
  virtio-rng: remove extra request for entropy
  virtio-rng: fix typos, comments

 hw/virtio-rng.c    |   80 ++++++----------------------------------------------
 include/qemu/rng.h |    6 ++--
 2 files changed, 12 insertions(+), 74 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH 1/3] virtio-rng: use virtqueue_get_avail_bytes, fix migration

[Amit Shah]

Popping an elem from the vq just to find out its length causes problems
with save/load later on.  Use the new virtqueue_get_avail_bytes()
function instead, saves us the complexity in the migration code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-rng.c |   69 +++++++-----------------------------------------------
 1 files changed, 9 insertions(+), 60 deletions(-)

diff --git a/hw/virtio-rng.c b/hw/virtio-rng.c
index b7fb5e9..42ac30b 100644
--- a/hw/virtio-rng.c
+++ b/hw/virtio-rng.c
@@ -22,14 +22,10 @@ typedef struct VirtIORNG {
 
     /* Only one vq - guest puts buffer(s) on it when it needs entropy */
     VirtQueue *vq;
-    VirtQueueElement elem;
 
     /* Config data for the device -- currently only chardev */
     VirtIORNGConf *conf;
 
-    /* Whether we've popped a vq element into 'elem' above */
-    bool popped;
-
     RngBackend *rng;
 } VirtIORNG;
 
@@ -42,23 +38,19 @@ static bool is_guest_ready(VirtIORNG *vrng)
     return false;
 }
 
-static size_t pop_an_elem(VirtIORNG *vrng)
+static size_t get_request_size(VirtQueue *vq)
 {
-    size_t size;
+    unsigned int in, out;
 
-    if (!vrng->popped && !virtqueue_pop(vrng->vq, &vrng->elem)) {
-        return 0;
-    }
-    vrng->popped = true;
-
-    size = iov_size(vrng->elem.in_sg, vrng->elem.in_num);
-    return size;
+    virtqueue_get_avail_bytes(vq, &in, &out);
+    return in;
 }
 
 /* Send data from a char device over to the guest */
 static void chr_read(void *opaque, const void *buf, size_t size)
 {
     VirtIORNG *vrng = opaque;
+    VirtQueueElement elem;
     size_t len;
     int offset;
 
@@ -68,15 +60,14 @@ static void chr_read(void *opaque, const void *buf, size_t size)
 
     offset = 0;
     while (offset < size) {
-        if (!pop_an_elem(vrng)) {
+        if (!virtqueue_pop(vrng->vq, &elem)) {
             break;
         }
-        len = iov_from_buf(vrng->elem.in_sg, vrng->elem.in_num,
+        len = iov_from_buf(elem.in_sg, elem.in_num,
                            0, buf + offset, size - offset);
         offset += len;
 
-        virtqueue_push(vrng->vq, &vrng->elem, len);
-        vrng->popped = false;
+        virtqueue_push(vrng->vq, &elem, len);
     }
     virtio_notify(&vrng->vdev, vrng->vq);
 
@@ -96,7 +87,7 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
     VirtIORNG *vrng = DO_UPCAST(VirtIORNG, vdev, vdev);
     size_t size;
 
-    size = pop_an_elem(vrng);
+    size = get_request_size(vq);
     if (size) {
         rng_backend_request_entropy(vrng->rng, size, chr_read, vrng);
     }
@@ -112,23 +103,6 @@ static void virtio_rng_save(QEMUFile *f, void *opaque)
     VirtIORNG *vrng = opaque;
 
     virtio_save(&vrng->vdev, f);
-
-    qemu_put_byte(f, vrng->popped);
-    if (vrng->popped) {
-        int i;
-
-        qemu_put_be32(f, vrng->elem.index);
-
-        qemu_put_be32(f, vrng->elem.in_num);
-        for (i = 0; i < vrng->elem.in_num; i++) {
-            qemu_put_be64(f, vrng->elem.in_addr[i]);
-        }
-
-        qemu_put_be32(f, vrng->elem.out_num);
-        for (i = 0; i < vrng->elem.out_num; i++) {
-            qemu_put_be64(f, vrng->elem.out_addr[i]);
-        }
-    }
 }
 
 static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id)
@@ -139,30 +113,6 @@ static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id)
         return -EINVAL;
     }
     virtio_load(&vrng->vdev, f);
-
-    vrng->popped = qemu_get_byte(f);
-    if (vrng->popped) {
-        int i;
-
-        vrng->elem.index = qemu_get_be32(f);
-
-        vrng->elem.in_num = qemu_get_be32(f);
-        g_assert(vrng->elem.in_num < VIRTQUEUE_MAX_SIZE);
-        for (i = 0; i < vrng->elem.in_num; i++) {
-            vrng->elem.in_addr[i] = qemu_get_be64(f);
-        }
-
-        vrng->elem.out_num = qemu_get_be32(f);
-        g_assert(vrng->elem.out_num < VIRTQUEUE_MAX_SIZE);
-        for (i = 0; i < vrng->elem.out_num; i++) {
-            vrng->elem.out_addr[i] = qemu_get_be64(f);
-        }
-
-        virtqueue_map_sg(vrng->elem.in_sg, vrng->elem.in_addr,
-                         vrng->elem.in_num, 1);
-        virtqueue_map_sg(vrng->elem.out_sg, vrng->elem.out_addr,
-                         vrng->elem.out_num, 0);
-    }
     return 0;
 }
 
@@ -195,7 +145,6 @@ VirtIODevice *virtio_rng_init(DeviceState *dev, VirtIORNGConf *conf)
 
     vrng->qdev = dev;
     vrng->conf = conf;
-    vrng->popped = false;
     register_savevm(dev, "virtio-rng", -1, 1, virtio_rng_save,
                     virtio_rng_load, vrng);
 
-- 
1.7.7.6

[Qemu-devel] [PATCH 2/3] virtio-rng: remove extra request for entropy

[Amit Shah]

If we just got a few bytes from the backends, but the guest is ready to
accept more, don't poke the backends once more for more bytes; it
already knows (from the request in handle_input()).

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-rng.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/hw/virtio-rng.c b/hw/virtio-rng.c
index 42ac30b..bb19cfa 100644
--- a/hw/virtio-rng.c
+++ b/hw/virtio-rng.c
@@ -70,16 +70,6 @@ static void chr_read(void *opaque, const void *buf, size_t size)
         virtqueue_push(vrng->vq, &elem, len);
     }
     virtio_notify(&vrng->vdev, vrng->vq);
-
-    /*
-     * Lastly, if we had multiple elems queued by the guest, and we
-     * didn't have enough data to fill them all, indicate we want more
-     * data.
-     */
-    len = pop_an_elem(vrng);
-    if (len) {
-        rng_backend_request_entropy(vrng->rng, size, chr_read, vrng);
-    }
 }
 
 static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
-- 
1.7.7.6

[Qemu-devel] [PATCH 3/3] virtio-rng: fix typos, comments

[Amit Shah]

Fix typos and update comments to match current implementation.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-rng.c    |    1 -
 include/qemu/rng.h |    6 +++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/virtio-rng.c b/hw/virtio-rng.c
index bb19cfa..290b2b6 100644
--- a/hw/virtio-rng.c
+++ b/hw/virtio-rng.c
@@ -23,7 +23,6 @@ typedef struct VirtIORNG {
     /* Only one vq - guest puts buffer(s) on it when it needs entropy */
     VirtQueue *vq;
 
-    /* Config data for the device -- currently only chardev */
     VirtIORNGConf *conf;
 
     RngBackend *rng;
diff --git a/include/qemu/rng.h b/include/qemu/rng.h
index 7e9d672..9836463 100644
--- a/include/qemu/rng.h
+++ b/include/qemu/rng.h
@@ -61,10 +61,10 @@ struct RngBackend
  * This function is used by the front-end to request entropy from an entropy
  * source.  This function can be called multiple times before @receive_entropy
  * is invoked with different values of @receive_entropy and @opaque.  The
- * backend will queue each request and handle appropriate.
+ * backend will queue each request and handle appropriately.
  *
  * The backend does not need to pass the full amount of data to @receive_entropy
- * but will pass at a value greater than 0.
+ * but will pass a a value greater than 0.
  */
 void rng_backend_request_entropy(RngBackend *s, size_t size,
                                  EntropyReceiveFunc *receive_entropy,
@@ -87,7 +87,7 @@ void rng_backend_cancel_requests(RngBackend *s);
  *
  * This function will open the backend if it is not already open.  Calling this
  * function on an already opened backend will not result in an error.
- */ 
+ */
 void rng_backend_open(RngBackend *s, Error **errp);
 
 #endif
-- 
1.7.7.6