diff mbox

[v2,5/6] Vring: use hostmem's RAM safe api

Message ID 1367549122-2948-6-git-send-email-qemulist@gmail.com
State New
Headers show

Commit Message

pingfan liu May 3, 2013, 2:45 a.m. UTC 
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Before mm-ops done, we should gurantee the validaion of regions which is
used by Vring self and the chunck pointed by vring desc. We acheive
this goal by inc refcnt of RAM-Device. When finished, we dec this cnt
through the interface of MemoryRegion.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/block/dataplane/virtio-blk.c     |    3 +-
 hw/virtio/dataplane/vring.c         |   95 +++++++++++++++++++++++++++-------
 include/hw/virtio/dataplane/vring.h |    4 +-
 3 files changed, 80 insertions(+), 22 deletions(-)

Comments

Stefan Hajnoczi May 3, 2013, 10:02 a.m. UTC | #1 
On Fri, May 03, 2013 at 10:45:21AM +0800, Liu Ping Fan wrote:
> @@ -109,11 +110,14 @@ bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
>  static int get_indirect(Vring *vring,
>                          struct iovec iov[], struct iovec *iov_end,
>                          unsigned int *out_num, unsigned int *in_num,
> -                        struct vring_desc *indirect)
> +                        struct vring_desc *indirect,
> +                        MemoryRegion ***mrs)
>  {
>      struct vring_desc desc;
>      unsigned int i = 0, count, found = 0;
> -
> +    MemoryRegion **cur = *mrs;
> +    int ret = 0;
> +    MemoryRegion *tmp;
>      /* Sanity check */
>      if (unlikely(indirect->len % sizeof(desc))) {
>          error_report("Invalid length in indirect descriptor: "
> @@ -132,19 +136,23 @@ static int get_indirect(Vring *vring,
>          return -EFAULT;
>      }
>  
> +    *mrs[0] = NULL;

The goto fail case is broken when we fail with cur > *mrs before calling
hostmem_lookup(..., cur, ...) since *cur will be undefined.

>      do {
>          struct vring_desc *desc_ptr;
>  
>          /* Translate indirect descriptor */
>          desc_ptr = hostmem_lookup(indirect->addr + found * sizeof(desc),
> -                                  sizeof(desc), NULL, false);
> +                                  sizeof(desc),
> +                                  &tmp,

Please use a more descriptive name, like desc_mr.  This function is
fairly involved so the variable names should be descriptive.

> +                                  false);
>          if (!desc_ptr) {
>              error_report("Failed to map indirect descriptor "
>                           "addr %#" PRIx64 " len %zu",
>                           (uint64_t)indirect->addr + found * sizeof(desc),
>                           sizeof(desc));
>              vring->broken = true;
> -            return -EFAULT;
> +            ret = -EFAULT;
> +            goto fail;
>          }
>          desc = *desc_ptr;
>  
> @@ -155,31 +163,40 @@ static int get_indirect(Vring *vring,
>              error_report("Loop detected: last one at %u "
>                           "indirect size %u", i, count);
>              vring->broken = true;
> -            return -EFAULT;
> +            memory_region_unref(tmp);
> +            ret = -EFAULT;
> +            goto fail;
>          }

No need to hold onto tmp and handle all these error cases.  After the
barrier() desc_ptr is no longer used because we've loaded the descriptor
into a local struct.  Please unref tmp right after barrier().

> @@ -209,15 +240,20 @@ static int get_indirect(Vring *vring,
>   * never a valid descriptor number) if none was found.  A negative code is
>   * returned on error.
>   *
> + * @mrs should be the same cnt as iov[]
> + *
>   * Stolen from linux/drivers/vhost/vhost.c.
>   */
>  int vring_pop(VirtIODevice *vdev, Vring *vring,
>                struct iovec iov[], struct iovec *iov_end,
> -              unsigned int *out_num, unsigned int *in_num)
> +              unsigned int *out_num, unsigned int *in_num,
> +              MemoryRegion **mrs)
>  {
>      struct vring_desc desc;
>      unsigned int i, head, found = 0, num = vring->vr.num;
>      uint16_t avail_idx, last_avail_idx;
> +    MemoryRegion **indirect, **cur = mrs;
> +    int ret = 0;
>  
>      /* If there was a fatal error then refuse operation */
>      if (vring->broken) {
> @@ -263,17 +299,20 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
>      *out_num = *in_num = 0;
>  
>      i = head;
> +    mrs[0] = NULL;

Same goto fail issue here when cur > *mrs before
hostmem_lookup(..., cur, ...) has been called.

>      do {
>          if (unlikely(i >= num)) {
>              error_report("Desc index is %u > %u, head = %u", i, num, head);
>              vring->broken = true;
> -            return -EFAULT;
> +            ret = -EFAULT;
> +            goto fail;
>          }
>          if (unlikely(++found > num)) {
>              error_report("Loop detected: last one at %u vq size %u head %u",
>                           i, num, head);
>              vring->broken = true;
> -            return -EFAULT;
> +            ret = -EFAULT;
> +            goto fail;
>          }
>          desc = vring->vr.desc[i];
>  
> @@ -281,10 +320,13 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
>          barrier();
>  
>          if (desc.flags & VRING_DESC_F_INDIRECT) {
> -            int ret = get_indirect(vring, iov, iov_end, out_num, in_num, &desc);
> +            indirect = cur;
> +            int ret = get_indirect(vring, iov, iov_end, out_num, in_num, &desc,
> +                        &indirect);
>              if (ret < 0) {
> -                return ret;
> +               goto fail;

Indentation.
pingfan liu May 6, 2013, 1:45 a.m. UTC | #2 
On Fri, May 3, 2013 at 6:02 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Fri, May 03, 2013 at 10:45:21AM +0800, Liu Ping Fan wrote:
>> @@ -109,11 +110,14 @@ bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
>>  static int get_indirect(Vring *vring,
>>                          struct iovec iov[], struct iovec *iov_end,
>>                          unsigned int *out_num, unsigned int *in_num,
>> -                        struct vring_desc *indirect)
>> +                        struct vring_desc *indirect,
>> +                        MemoryRegion ***mrs)
>>  {
>>      struct vring_desc desc;
>>      unsigned int i = 0, count, found = 0;
>> -
>> +    MemoryRegion **cur = *mrs;
>> +    int ret = 0;
>> +    MemoryRegion *tmp;
>>      /* Sanity check */
>>      if (unlikely(indirect->len % sizeof(desc))) {
>>          error_report("Invalid length in indirect descriptor: "
>> @@ -132,19 +136,23 @@ static int get_indirect(Vring *vring,
>>          return -EFAULT;
>>      }
>>
>> +    *mrs[0] = NULL;
>
> The goto fail case is broken when we fail with cur > *mrs before calling
> hostmem_lookup(..., cur, ...) since *cur will be undefined.
>
Will fix,
>>      do {
>>          struct vring_desc *desc_ptr;
>>
>>          /* Translate indirect descriptor */
>>          desc_ptr = hostmem_lookup(indirect->addr + found * sizeof(desc),
>> -                                  sizeof(desc), NULL, false);
>> +                                  sizeof(desc),
>> +                                  &tmp,
>
> Please use a more descriptive name, like desc_mr.  This function is
> fairly involved so the variable names should be descriptive.
>
Ok,
>> +                                  false);
>>          if (!desc_ptr) {
>>              error_report("Failed to map indirect descriptor "
>>                           "addr %#" PRIx64 " len %zu",
>>                           (uint64_t)indirect->addr + found * sizeof(desc),
>>                           sizeof(desc));
>>              vring->broken = true;
>> -            return -EFAULT;
>> +            ret = -EFAULT;
>> +            goto fail;
>>          }
>>          desc = *desc_ptr;
>>
>> @@ -155,31 +163,40 @@ static int get_indirect(Vring *vring,
>>              error_report("Loop detected: last one at %u "
>>                           "indirect size %u", i, count);
>>              vring->broken = true;
>> -            return -EFAULT;
>> +            memory_region_unref(tmp);
>> +            ret = -EFAULT;
>> +            goto fail;
>>          }
>
> No need to hold onto tmp and handle all these error cases.  After the
> barrier() desc_ptr is no longer used because we've loaded the descriptor
> into a local struct.  Please unref tmp right after barrier().
>
Ok,
>> @@ -209,15 +240,20 @@ static int get_indirect(Vring *vring,
>>   * never a valid descriptor number) if none was found.  A negative code is
>>   * returned on error.
>>   *
>> + * @mrs should be the same cnt as iov[]
>> + *
>>   * Stolen from linux/drivers/vhost/vhost.c.
>>   */
>>  int vring_pop(VirtIODevice *vdev, Vring *vring,
>>                struct iovec iov[], struct iovec *iov_end,
>> -              unsigned int *out_num, unsigned int *in_num)
>> +              unsigned int *out_num, unsigned int *in_num,
>> +              MemoryRegion **mrs)
>>  {
>>      struct vring_desc desc;
>>      unsigned int i, head, found = 0, num = vring->vr.num;
>>      uint16_t avail_idx, last_avail_idx;
>> +    MemoryRegion **indirect, **cur = mrs;
>> +    int ret = 0;
>>
>>      /* If there was a fatal error then refuse operation */
>>      if (vring->broken) {
>> @@ -263,17 +299,20 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
>>      *out_num = *in_num = 0;
>>
>>      i = head;
>> +    mrs[0] = NULL;
>
> Same goto fail issue here when cur > *mrs before
> hostmem_lookup(..., cur, ...) has been called.
>
Will fix
>>      do {
>>          if (unlikely(i >= num)) {
>>              error_report("Desc index is %u > %u, head = %u", i, num, head);
>>              vring->broken = true;
>> -            return -EFAULT;
>> +            ret = -EFAULT;
>> +            goto fail;
>>          }
>>          if (unlikely(++found > num)) {
>>              error_report("Loop detected: last one at %u vq size %u head %u",
>>                           i, num, head);
>>              vring->broken = true;
>> -            return -EFAULT;
>> +            ret = -EFAULT;
>> +            goto fail;
>>          }
>>          desc = vring->vr.desc[i];
>>
>> @@ -281,10 +320,13 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
>>          barrier();
>>
>>          if (desc.flags & VRING_DESC_F_INDIRECT) {
>> -            int ret = get_indirect(vring, iov, iov_end, out_num, in_num, &desc);
>> +            indirect = cur;
>> +            int ret = get_indirect(vring, iov, iov_end, out_num, in_num, &desc,
>> +                        &indirect);
>>              if (ret < 0) {
>> -                return ret;
>> +               goto fail;
>
> Indentation.
Will fix.

Thanks,
Pingfan
diff mbox

Patch

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 0356665..3bb57d1 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -305,7 +305,8 @@  static void handle_notify(EventNotifier *e)
         vring_disable_notification(s->vdev, &s->vring);
 
         for (;;) {
-            head = vring_pop(s->vdev, &s->vring, iov, end, &out_num, &in_num);
+            head = vring_pop(s->vdev, &s->vring, iov, end, &out_num,
+                        &in_num, NULL);
             if (head < 0) {
                 break; /* no more requests */
             }
diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index e3c3afb..440486d 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -27,7 +27,7 @@  bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
 
     vring->broken = false;
 
-    vring_ptr = hostmem_lookup(vring_addr, vring_size, NULL, true);
+    vring_ptr = hostmem_lookup(vring_addr, vring_size, &vring->vring_mr, true);
     if (!vring_ptr) {
         error_report("Failed to map vring "
                      "addr %#" HWADDR_PRIx " size %" HWADDR_PRIu,
@@ -50,6 +50,7 @@  bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
 
 void vring_teardown(Vring *vring)
 {
+    memory_region_unref(vring->vring_mr);
 }
 
 /* Disable guest->host notifies */
@@ -109,11 +110,14 @@  bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
 static int get_indirect(Vring *vring,
                         struct iovec iov[], struct iovec *iov_end,
                         unsigned int *out_num, unsigned int *in_num,
-                        struct vring_desc *indirect)
+                        struct vring_desc *indirect,
+                        MemoryRegion ***mrs)
 {
     struct vring_desc desc;
     unsigned int i = 0, count, found = 0;
-
+    MemoryRegion **cur = *mrs;
+    int ret = 0;
+    MemoryRegion *tmp;
     /* Sanity check */
     if (unlikely(indirect->len % sizeof(desc))) {
         error_report("Invalid length in indirect descriptor: "
@@ -132,19 +136,23 @@  static int get_indirect(Vring *vring,
         return -EFAULT;
     }
 
+    *mrs[0] = NULL;
     do {
         struct vring_desc *desc_ptr;
 
         /* Translate indirect descriptor */
         desc_ptr = hostmem_lookup(indirect->addr + found * sizeof(desc),
-                                  sizeof(desc), NULL, false);
+                                  sizeof(desc),
+                                  &tmp,
+                                  false);
         if (!desc_ptr) {
             error_report("Failed to map indirect descriptor "
                          "addr %#" PRIx64 " len %zu",
                          (uint64_t)indirect->addr + found * sizeof(desc),
                          sizeof(desc));
             vring->broken = true;
-            return -EFAULT;
+            ret = -EFAULT;
+            goto fail;
         }
         desc = *desc_ptr;
 
@@ -155,31 +163,40 @@  static int get_indirect(Vring *vring,
             error_report("Loop detected: last one at %u "
                          "indirect size %u", i, count);
             vring->broken = true;
-            return -EFAULT;
+            memory_region_unref(tmp);
+            ret = -EFAULT;
+            goto fail;
         }
 
         if (unlikely(desc.flags & VRING_DESC_F_INDIRECT)) {
             error_report("Nested indirect descriptor");
             vring->broken = true;
-            return -EFAULT;
+            memory_region_unref(tmp);
+            ret = -EFAULT;
+            goto fail;
         }
 
         /* Stop for now if there are not enough iovecs available. */
         if (iov >= iov_end) {
-            return -ENOBUFS;
+            memory_region_unref(tmp);
+            ret = -ENOBUFS;
+            goto fail;
         }
 
-        iov->iov_base = hostmem_lookup(desc.addr, desc.len, NULL,
+        iov->iov_base = hostmem_lookup(desc.addr, desc.len, cur,
                                        desc.flags & VRING_DESC_F_WRITE);
         if (!iov->iov_base) {
             error_report("Failed to map indirect descriptor"
                          "addr %#" PRIx64 " len %u",
                          (uint64_t)desc.addr, desc.len);
             vring->broken = true;
-            return -EFAULT;
+            memory_region_unref(tmp);
+            ret = -EFAULT;
+            goto fail;
         }
         iov->iov_len = desc.len;
         iov++;
+        cur++;
 
         /* If this is an input descriptor, increment that count. */
         if (desc.flags & VRING_DESC_F_WRITE) {
@@ -191,13 +208,27 @@  static int get_indirect(Vring *vring,
                 error_report("Indirect descriptor "
                              "has out after in: idx %u", i);
                 vring->broken = true;
-                return -EFAULT;
+                memory_region_unref(tmp);
+                ret = -EFAULT;
+                goto fail;
             }
             *out_num += 1;
         }
         i = desc.next;
+        memory_region_unref(tmp);
     } while (desc.flags & VRING_DESC_F_NEXT);
+    *mrs = cur;
     return 0;
+
+fail:
+    for (; cur > *mrs; cur--) {
+        memory_region_unref(*cur);
+    }
+    if (*cur) {
+        memory_region_unref(*cur);
+    }
+
+    return ret;
 }
 
 /* This looks in the virtqueue and for the first available buffer, and converts
@@ -209,15 +240,20 @@  static int get_indirect(Vring *vring,
  * never a valid descriptor number) if none was found.  A negative code is
  * returned on error.
  *
+ * @mrs should be the same cnt as iov[]
+ *
  * Stolen from linux/drivers/vhost/vhost.c.
  */
 int vring_pop(VirtIODevice *vdev, Vring *vring,
               struct iovec iov[], struct iovec *iov_end,
-              unsigned int *out_num, unsigned int *in_num)
+              unsigned int *out_num, unsigned int *in_num,
+              MemoryRegion **mrs)
 {
     struct vring_desc desc;
     unsigned int i, head, found = 0, num = vring->vr.num;
     uint16_t avail_idx, last_avail_idx;
+    MemoryRegion **indirect, **cur = mrs;
+    int ret = 0;
 
     /* If there was a fatal error then refuse operation */
     if (vring->broken) {
@@ -263,17 +299,20 @@  int vring_pop(VirtIODevice *vdev, Vring *vring,
     *out_num = *in_num = 0;
 
     i = head;
+    mrs[0] = NULL;
     do {
         if (unlikely(i >= num)) {
             error_report("Desc index is %u > %u, head = %u", i, num, head);
             vring->broken = true;
-            return -EFAULT;
+            ret = -EFAULT;
+            goto fail;
         }
         if (unlikely(++found > num)) {
             error_report("Loop detected: last one at %u vq size %u head %u",
                          i, num, head);
             vring->broken = true;
-            return -EFAULT;
+            ret = -EFAULT;
+            goto fail;
         }
         desc = vring->vr.desc[i];
 
@@ -281,10 +320,13 @@  int vring_pop(VirtIODevice *vdev, Vring *vring,
         barrier();
 
         if (desc.flags & VRING_DESC_F_INDIRECT) {
-            int ret = get_indirect(vring, iov, iov_end, out_num, in_num, &desc);
+            indirect = cur;
+            int ret = get_indirect(vring, iov, iov_end, out_num, in_num, &desc,
+                        &indirect);
             if (ret < 0) {
-                return ret;
+               goto fail;
             }
+            cur = indirect;
             continue;
         }
 
@@ -293,20 +335,23 @@  int vring_pop(VirtIODevice *vdev, Vring *vring,
          * with the current set.
          */
         if (iov >= iov_end) {
-            return -ENOBUFS;
+            ret = -ENOBUFS;
+            goto fail;
         }
 
         /* TODO handle non-contiguous memory across region boundaries */
-        iov->iov_base = hostmem_lookup(desc.addr, desc.len, NULL,
+        iov->iov_base = hostmem_lookup(desc.addr, desc.len, cur,
                                        desc.flags & VRING_DESC_F_WRITE);
         if (!iov->iov_base) {
             error_report("Failed to map vring desc addr %#" PRIx64 " len %u",
                          (uint64_t)desc.addr, desc.len);
             vring->broken = true;
-            return -EFAULT;
+            ret = -EFAULT;
+            goto fail;
         }
         iov->iov_len  = desc.len;
         iov++;
+        cur++;
 
         if (desc.flags & VRING_DESC_F_WRITE) {
             /* If this is an input descriptor,
@@ -318,7 +363,8 @@  int vring_pop(VirtIODevice *vdev, Vring *vring,
             if (unlikely(*in_num)) {
                 error_report("Descriptor has out after in: idx %d", i);
                 vring->broken = true;
-                return -EFAULT;
+                ret = -EFAULT;
+                goto fail;
             }
             *out_num += 1;
         }
@@ -328,6 +374,15 @@  int vring_pop(VirtIODevice *vdev, Vring *vring,
     /* On success, increment avail index. */
     vring->last_avail_idx++;
     return head;
+
+fail:
+    for (; cur > mrs; cur--) {
+        memory_region_unref(*cur);
+    }
+    if (*cur) {
+        memory_region_unref(*cur);
+    }
+    return ret;
 }
 
 /* After we've used one of their buffers, we tell them about it.
diff --git a/include/hw/virtio/dataplane/vring.h b/include/hw/virtio/dataplane/vring.h
index 56acffb..1aee7cf 100644
--- a/include/hw/virtio/dataplane/vring.h
+++ b/include/hw/virtio/dataplane/vring.h
@@ -23,6 +23,7 @@ 
 #include "hw/virtio/virtio.h"
 
 typedef struct {
+    MemoryRegion *vring_mr;   /* RAM's memoryRegion on which this vring sits */
     struct vring vr;                /* virtqueue vring mapped to host memory */
     uint16_t last_avail_idx;        /* last processed avail ring index */
     uint16_t last_used_idx;         /* last processed used ring index */
@@ -55,7 +56,8 @@  bool vring_enable_notification(VirtIODevice *vdev, Vring *vring);
 bool vring_should_notify(VirtIODevice *vdev, Vring *vring);
 int vring_pop(VirtIODevice *vdev, Vring *vring,
               struct iovec iov[], struct iovec *iov_end,
-              unsigned int *out_num, unsigned int *in_num);
+              unsigned int *out_num, unsigned int *in_num,
+              MemoryRegion **mrs);
 void vring_push(Vring *vring, unsigned int head, int len);
 
 #endif /* VRING_H */