Patchwork [1/4] virtio-rng: use virtqueue_get_avail_bytes, fix migration

login
register
mail settings
Submitter Amit Shah
Date Nov. 20, 2012, 2:26 p.m.
Message ID <d2c338095e33c3fa35f62f0c2fa985b4f77962c8.1353421334.git.amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/200352/
State New
Headers show

Comments

Amit Shah - Nov. 20, 2012, 2:26 p.m.
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, as well
as makes the migration endian-safe.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-rng.c | 76 +++++++++------------------------------------------------
 1 file changed, 12 insertions(+), 64 deletions(-)
Anthony Liguori - Nov. 21, 2012, 1:44 a.m.
Amit Shah <amit.shah@redhat.com> writes:

> 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, as well
> as makes the migration endian-safe.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

To be clear, migration is already endian safe but this is definitely a
very nice cleanup.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  hw/virtio-rng.c | 76 +++++++++------------------------------------------------
>  1 file changed, 12 insertions(+), 64 deletions(-)
>
> diff --git a/hw/virtio-rng.c b/hw/virtio-rng.c
> index 3ca96c8..6c49bb2 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;
>  
>      /* We purposefully don't migrate this state.  The quota will reset on the
> @@ -48,17 +44,12 @@ 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;
>  }
>  
>  static void virtio_rng_process(VirtIORNG *vrng);
> @@ -67,6 +58,7 @@ static void virtio_rng_process(VirtIORNG *vrng);
>  static void chr_read(void *opaque, const void *buf, size_t size)
>  {
>      VirtIORNG *vrng = opaque;
> +    VirtQueueElement elem;
>      size_t len;
>      int offset;
>  
> @@ -78,15 +70,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);
>  
> @@ -100,21 +91,19 @@ static void chr_read(void *opaque, const void *buf, size_t size)
>  
>  static void virtio_rng_process(VirtIORNG *vrng)
>  {
> -    ssize_t size;
> +    size_t size;
>  
>      if (!is_guest_ready(vrng)) {
>          return;
>      }
>  
> -    size = pop_an_elem(vrng);
> +    size = get_request_size(vrng->vq);
>      size = MIN(vrng->quota_remaining, size);
> -
> -    if (size > 0) {
> +    if (size) {
>          rng_backend_request_entropy(vrng->rng, size, chr_read, vrng);
>      }
>  }
>  
> -
>  static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
>  {
>      VirtIORNG *vrng = DO_UPCAST(VirtIORNG, vdev, vdev);
> @@ -131,23 +120,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)
> @@ -159,30 +131,6 @@ static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id)
>      }
>      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);
> -    }
> -
>      /* We may have an element ready but couldn't process it due to a quota
>         limit.  Make sure to try again after live migration when the quota may
>         have been reset.
> @@ -232,7 +180,7 @@ VirtIODevice *virtio_rng_init(DeviceState *dev, VirtIORNGConf *conf)
>  
>      vrng->qdev = dev;
>      vrng->conf = conf;
> -    vrng->popped = false;
> +
>      vrng->quota_remaining = vrng->conf->max_bytes;
>  
>      g_assert_cmpint(vrng->conf->max_bytes, <=, INT64_MAX);
> -- 
> 1.8.0
Amit Shah - Nov. 21, 2012, 5:53 a.m.
On (Tue) 20 Nov 2012 [19:44:55], Anthony Liguori wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > 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, as well
> > as makes the migration endian-safe.
> >
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> 
> To be clear, migration is already endian safe but this is definitely a
> very nice cleanup.

The in_addr and out_addr fields have pointers -- so endian- plus
word-length safety isn't guaranteed if migrating across architectures.

		Amit

Patch

diff --git a/hw/virtio-rng.c b/hw/virtio-rng.c
index 3ca96c8..6c49bb2 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;
 
     /* We purposefully don't migrate this state.  The quota will reset on the
@@ -48,17 +44,12 @@  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;
 }
 
 static void virtio_rng_process(VirtIORNG *vrng);
@@ -67,6 +58,7 @@  static void virtio_rng_process(VirtIORNG *vrng);
 static void chr_read(void *opaque, const void *buf, size_t size)
 {
     VirtIORNG *vrng = opaque;
+    VirtQueueElement elem;
     size_t len;
     int offset;
 
@@ -78,15 +70,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);
 
@@ -100,21 +91,19 @@  static void chr_read(void *opaque, const void *buf, size_t size)
 
 static void virtio_rng_process(VirtIORNG *vrng)
 {
-    ssize_t size;
+    size_t size;
 
     if (!is_guest_ready(vrng)) {
         return;
     }
 
-    size = pop_an_elem(vrng);
+    size = get_request_size(vrng->vq);
     size = MIN(vrng->quota_remaining, size);
-
-    if (size > 0) {
+    if (size) {
         rng_backend_request_entropy(vrng->rng, size, chr_read, vrng);
     }
 }
 
-
 static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIORNG *vrng = DO_UPCAST(VirtIORNG, vdev, vdev);
@@ -131,23 +120,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)
@@ -159,30 +131,6 @@  static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id)
     }
     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);
-    }
-
     /* We may have an element ready but couldn't process it due to a quota
        limit.  Make sure to try again after live migration when the quota may
        have been reset.
@@ -232,7 +180,7 @@  VirtIODevice *virtio_rng_init(DeviceState *dev, VirtIORNGConf *conf)
 
     vrng->qdev = dev;
     vrng->conf = conf;
-    vrng->popped = false;
+
     vrng->quota_remaining = vrng->conf->max_bytes;
 
     g_assert_cmpint(vrng->conf->max_bytes, <=, INT64_MAX);