[Qemu-devel] [PATCH 0/3] vhost: fix vring layout

[Qemu-devel] [PATCH 0/3] vhost: fix vring layout

[Greg Kurz]

The vhost code currently assumes vring are mapped in memory following the
legacy virtio layout. This may cause QEMU to fail with virtio 1 devices
if the used ring is mapped below the descriptor table.

---

Greg Kurz (3):
      vhost: adapt vhost_verify_ring_mappings() to virtio 1 ring layout
      vhost: drop legacy vring layout bits
      virtio: drop virtio_queue_get_ring_{size,addr}()


 hw/virtio/vhost.c          |   92 +++++++++++++++++++++++++++++---------------
 hw/virtio/virtio.c         |   11 -----
 include/hw/virtio/vhost.h  |    7 ++-
 include/hw/virtio/virtio.h |    2 -
 4 files changed, 64 insertions(+), 48 deletions(-)

--
Greg

[Qemu-devel] [PATCH 1/3] vhost: adapt vhost_verify_ring_mappings() to virtio 1 ring layout

[Greg Kurz]

With virtio 1, the vring layout is split in 3 separate regions of
contiguous memory for the descriptor table, the available ring and the
used ring, as opposed with legacy virtio which uses a single region.

In case of memory re-mapping, the code ensures it doesn't affect the
vring mapping. This is done in vhost_verify_ring_mappings() which assumes
the device is legacy.

This patch changes vhost_verify_ring_mappings() to check the mappings of
each part of the vring separately.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/virtio/vhost.c         |   79 ++++++++++++++++++++++++++++++++++-----------
 include/hw/virtio/vhost.h |    4 ++
 2 files changed, 64 insertions(+), 19 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 131f1643b2a4..10133f12ebba 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -421,32 +421,73 @@ static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
     dev->log_size = size;
 }
 
+
+static int vhost_verify_ring_part_mapping(void *part,
+                                          uint64_t part_addr,
+                                          uint64_t part_size,
+                                          uint64_t start_addr,
+                                          uint64_t size)
+{
+    hwaddr l;
+    void *p;
+    int r = 0;
+
+    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
+        return 0;
+    }
+    l = part_size;
+    p = cpu_physical_memory_map(part_addr, &l, 1);
+    if (!p || l != part_size) {
+        r = -ENOMEM;
+    }
+    if (p != part) {
+        r = -EBUSY;
+    }
+    cpu_physical_memory_unmap(p, l, 0, 0);
+    return r;
+}
+
 static int vhost_verify_ring_mappings(struct vhost_dev *dev,
                                       uint64_t start_addr,
                                       uint64_t size)
 {
-    int i;
+    int i, j;
     int r = 0;
+    const char *part_name[] = {
+        "descriptor table",
+        "available ring",
+        "used ring"
+    };
 
-    for (i = 0; !r && i < dev->nvqs; ++i) {
+    for (i = 0; i < dev->nvqs; ++i) {
         struct vhost_virtqueue *vq = dev->vqs + i;
-        hwaddr l;
-        void *p;
 
-        if (!ranges_overlap(start_addr, size, vq->ring_phys, vq->ring_size)) {
-            continue;
+        j = 0;
+        r = vhost_verify_ring_part_mapping(vq->desc, vq->desc_phys,
+                                           vq->desc_size, start_addr, size);
+        if (!r) {
+            break;
         }
-        l = vq->ring_size;
-        p = cpu_physical_memory_map(vq->ring_phys, &l, 1);
-        if (!p || l != vq->ring_size) {
-            error_report("Unable to map ring buffer for ring %d", i);
-            r = -ENOMEM;
+
+        j++;
+        r = vhost_verify_ring_part_mapping(vq->avail, vq->avail_phys,
+                                           vq->avail_size, start_addr, size);
+        if (!r) {
+            break;
         }
-        if (p != vq->ring) {
-            error_report("Ring buffer relocated for ring %d", i);
-            r = -EBUSY;
+
+        j++;
+        r = vhost_verify_ring_part_mapping(vq->used, vq->used_phys,
+                                           vq->used_size, start_addr, size);
+        if (!r) {
+            break;
         }
-        cpu_physical_memory_unmap(p, l, 0, 0);
+    }
+
+    if (r == -ENOMEM) {
+        error_report("Unable to map %s for ring %d", part_name[j], i);
+    } else if (r == -EBUSY) {
+        error_report("%s relocated for ring %d", part_name[j], i);
     }
     return r;
 }
@@ -860,15 +901,15 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
         }
     }
 
-    s = l = virtio_queue_get_desc_size(vdev, idx);
-    a = virtio_queue_get_desc_addr(vdev, idx);
+    vq->desc_size = s = l = virtio_queue_get_desc_size(vdev, idx);
+    vq->desc_phys = a = virtio_queue_get_desc_addr(vdev, idx);
     vq->desc = cpu_physical_memory_map(a, &l, 0);
     if (!vq->desc || l != s) {
         r = -ENOMEM;
         goto fail_alloc_desc;
     }
-    s = l = virtio_queue_get_avail_size(vdev, idx);
-    a = virtio_queue_get_avail_addr(vdev, idx);
+    vq->avail_size = s = l = virtio_queue_get_avail_size(vdev, idx);
+    vq->avail_phys = a = virtio_queue_get_avail_addr(vdev, idx);
     vq->avail = cpu_physical_memory_map(a, &l, 0);
     if (!vq->avail || l != s) {
         r = -ENOMEM;
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index e433089ea97f..56b567f1997f 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -14,6 +14,10 @@ struct vhost_virtqueue {
     void *avail;
     void *used;
     int num;
+    unsigned long long desc_phys;
+    unsigned desc_size;
+    unsigned long long avail_phys;
+    unsigned avail_size;
     unsigned long long used_phys;
     unsigned used_size;
     void *ring;

[Qemu-devel] [PATCH 2/3] vhost: drop legacy vring layout bits

[Greg Kurz]

The legacy vring layout is not used anymore. This patch simply removes it.

This also fixes a bug with virtio 1 devices when the vring descriptor table
is mapped at a higher address than the used vring because the following
function may return an insanely great value:

hwaddr virtio_queue_get_ring_size(VirtIODevice *vdev, int n)
{
    return vdev->vq[n].vring.used - vdev->vq[n].vring.desc +
           virtio_queue_get_used_size(vdev, n);
}

and the mapping fails.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/virtio/vhost.c         |   13 -------------
 include/hw/virtio/vhost.h |    3 ---
 2 files changed, 16 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 10133f12ebba..9fa81c9b65cb 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -923,14 +923,6 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
         goto fail_alloc_used;
     }
 
-    vq->ring_size = s = l = virtio_queue_get_ring_size(vdev, idx);
-    vq->ring_phys = a = virtio_queue_get_ring_addr(vdev, idx);
-    vq->ring = cpu_physical_memory_map(a, &l, 1);
-    if (!vq->ring || l != s) {
-        r = -ENOMEM;
-        goto fail_alloc_ring;
-    }
-
     r = vhost_virtqueue_set_addr(dev, vq, vhost_vq_index, dev->log_enabled);
     if (r < 0) {
         r = -errno;
@@ -971,9 +963,6 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
 fail_vector:
 fail_kick:
 fail_alloc:
-    cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev, idx),
-                              0, 0);
-fail_alloc_ring:
     cpu_physical_memory_unmap(vq->used, virtio_queue_get_used_size(vdev, idx),
                               0, 0);
 fail_alloc_used:
@@ -1014,8 +1003,6 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
                                                 vhost_vq_index);
     }
 
-    cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev, idx),
-                              0, virtio_queue_get_ring_size(vdev, idx));
     cpu_physical_memory_unmap(vq->used, virtio_queue_get_used_size(vdev, idx),
                               1, virtio_queue_get_used_size(vdev, idx));
     cpu_physical_memory_unmap(vq->avail, virtio_queue_get_avail_size(vdev, idx),
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 56b567f1997f..1fe5aadef5ce 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -20,9 +20,6 @@ struct vhost_virtqueue {
     unsigned avail_size;
     unsigned long long used_phys;
     unsigned used_size;
-    void *ring;
-    unsigned long long ring_phys;
-    unsigned ring_size;
     EventNotifier masked_notifier;
 };
 

[Qemu-devel] [PATCH 3/3] virtio: drop virtio_queue_get_ring_{size, addr}()

[Greg Kurz]

These are not used anymore.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/virtio/virtio.c         |   11 -----------
 include/hw/virtio/virtio.h |    2 --
 2 files changed, 13 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index bcbcfe063c19..8f3e07effcb0 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1935,11 +1935,6 @@ hwaddr virtio_queue_get_used_addr(VirtIODevice *vdev, int n)
     return vdev->vq[n].vring.used;
 }
 
-hwaddr virtio_queue_get_ring_addr(VirtIODevice *vdev, int n)
-{
-    return vdev->vq[n].vring.desc;
-}
-
 hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
 {
     return sizeof(VRingDesc) * vdev->vq[n].vring.num;
@@ -1957,12 +1952,6 @@ hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)
         sizeof(VRingUsedElem) * vdev->vq[n].vring.num;
 }
 
-hwaddr virtio_queue_get_ring_size(VirtIODevice *vdev, int n)
-{
-    return vdev->vq[n].vring.used - vdev->vq[n].vring.desc +
-	    virtio_queue_get_used_size(vdev, n);
-}
-
 uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n)
 {
     return vdev->vq[n].last_avail_idx;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index ac65d6a5941d..87c0c70cb407 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -255,11 +255,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
 hwaddr virtio_queue_get_avail_addr(VirtIODevice *vdev, int n);
 hwaddr virtio_queue_get_used_addr(VirtIODevice *vdev, int n);
-hwaddr virtio_queue_get_ring_addr(VirtIODevice *vdev, int n);
 hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n);
 hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n);
 hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n);
-hwaddr virtio_queue_get_ring_size(VirtIODevice *vdev, int n);
 uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n);
 void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx);
 void virtio_queue_invalidate_signalled_used(VirtIODevice *vdev, int n);

Re: [Qemu-devel] [PATCH 1/3] vhost: adapt vhost_verify_ring_mappings() to virtio 1 ring layout

[Cornelia Huck]

On Fri, 04 Nov 2016 09:39:15 +0100
Greg Kurz <groug@kaod.org> wrote:

> With virtio 1, the vring layout is split in 3 separate regions of
> contiguous memory for the descriptor table, the available ring and the
> used ring, as opposed with legacy virtio which uses a single region.
> 
> In case of memory re-mapping, the code ensures it doesn't affect the
> vring mapping. This is done in vhost_verify_ring_mappings() which assumes
> the device is legacy.
> 
> This patch changes vhost_verify_ring_mappings() to check the mappings of
> each part of the vring separately.

Add "This will work for legacy mappings as well." ?

> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/virtio/vhost.c         |   79 ++++++++++++++++++++++++++++++++++-----------
>  include/hw/virtio/vhost.h |    4 ++
>  2 files changed, 64 insertions(+), 19 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH 2/3] vhost: drop legacy vring layout bits

[Cornelia Huck]

On Fri, 04 Nov 2016 09:39:22 +0100
Greg Kurz <groug@kaod.org> wrote:

> The legacy vring layout is not used anymore. 

"as we use separate mappings even for legacy devices." ?

Otherwise, this may confuse the casual reader into thinking legacy is
not supported anymore.

> This patch simply removes it.
> 
> This also fixes a bug with virtio 1 devices when the vring descriptor table
> is mapped at a higher address than the used vring because the following
> function may return an insanely great value:
> 
> hwaddr virtio_queue_get_ring_size(VirtIODevice *vdev, int n)
> {
>     return vdev->vq[n].vring.used - vdev->vq[n].vring.desc +
>            virtio_queue_get_used_size(vdev, n);
> }
> 
> and the mapping fails.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/virtio/vhost.c         |   13 -------------
>  include/hw/virtio/vhost.h |    3 ---
>  2 files changed, 16 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH 3/3] virtio: drop virtio_queue_get_ring_{size, addr}()

[Cornelia Huck]

On Fri, 04 Nov 2016 09:39:29 +0100
Greg Kurz <groug@kaod.org> wrote:

> These are not used anymore.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/virtio/virtio.c         |   11 -----------
>  include/hw/virtio/virtio.h |    2 --
>  2 files changed, 13 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH 1/3] vhost: adapt vhost_verify_ring_mappings() to virtio 1 ring layout

[Greg Kurz]

On Fri, 4 Nov 2016 10:38:32 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Fri, 04 Nov 2016 09:39:15 +0100
> Greg Kurz <groug@kaod.org> wrote:
> 
> > With virtio 1, the vring layout is split in 3 separate regions of
> > contiguous memory for the descriptor table, the available ring and the
> > used ring, as opposed with legacy virtio which uses a single region.
> > 
> > In case of memory re-mapping, the code ensures it doesn't affect the
> > vring mapping. This is done in vhost_verify_ring_mappings() which assumes
> > the device is legacy.
> > 
> > This patch changes vhost_verify_ring_mappings() to check the mappings of
> > each part of the vring separately.  
> 
> Add "This will work for legacy mappings as well." ?
> 

Sure!

> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/virtio/vhost.c         |   79 ++++++++++++++++++++++++++++++++++-----------
> >  include/hw/virtio/vhost.h |    4 ++
> >  2 files changed, 64 insertions(+), 19 deletions(-)  
> 
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 

Re: [Qemu-devel] [PATCH 2/3] vhost: drop legacy vring layout bits

[Greg Kurz]

On Fri, 4 Nov 2016 10:40:44 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Fri, 04 Nov 2016 09:39:22 +0100
> Greg Kurz <groug@kaod.org> wrote:
> 
> > The legacy vring layout is not used anymore.   
> 
> "as we use separate mappings even for legacy devices." ?
> 
> Otherwise, this may confuse the casual reader into thinking legacy is
> not supported anymore.
> 

Yeah you're right. I'll add this too.

> > This patch simply removes it.
> > 
> > This also fixes a bug with virtio 1 devices when the vring descriptor table
> > is mapped at a higher address than the used vring because the following
> > function may return an insanely great value:
> > 
> > hwaddr virtio_queue_get_ring_size(VirtIODevice *vdev, int n)
> > {
> >     return vdev->vq[n].vring.used - vdev->vq[n].vring.desc +
> >            virtio_queue_get_used_size(vdev, n);
> > }
> > 
> > and the mapping fails.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/virtio/vhost.c         |   13 -------------
> >  include/hw/virtio/vhost.h |    3 ---
> >  2 files changed, 16 deletions(-)  
> 
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>