dataplane: simplify indirect descriptor read

bring_map currently fails if one of the entries it's mapping is contigious in GPA but not HVA address space. Introduce a mapped_len parameter so it can handle this, returning the actual mapped length.

This will still fail if there's no space left in the sg, but luckily max queue size in use is currently 256, while max sg size is 1024, so we should be OK even is all entries happen to cross a single DIMM boundary.

Won't work well with very small DIMM sizes, unfortunately:
e.g. this will fail with 4K DIMMs where a single request might span a large number of DIMMs.

Let's hope these are uncommon - at least we are not breaking things.

Reported-by Stefan Hajnoczi <[email protected]>

Reported-by Igor Mammedov <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Warning: compile-tested only.

 hw/virtio/dataplane/vring.c | 68

++++++++++++++++++++++++++++++--------------- 1 file changed, 46 insertions(+), 22 deletions(-)

hw/virtio/dataplane/vring.c +0 -0

index 0b92fcf..9ae9424 100644
@@ -25,15 +25,30 @@
 
 /* vring_map can be coupled with vring_unmap or (if you still have

the
* value returned in *mr) memory_region_unref.
+ * Returns NULL on failure.
+ * Callers that can handle a partial mapping must supply mapped_len pointer to
+ * get the actual length mapped.
+ * Passing mapped_len == NULL requires either a full mapping or a failure. */

hw/virtio/dataplane/vring.c +2 -1

-static void *vring_map(MemoryRegion **mr, hwaddr phys, hwaddr len,
+static void *vring_map(MemoryRegion **mr, hwaddr phys,
+                       hwaddr len, hwaddr *mapped_len,
                        bool is_write)
 {
     MemoryRegionSection section =

memory_region_find(get_system_memory(), phys, len);

hw/virtio/dataplane/vring.c +7 -1

+    uint64_t size;
 
-    if (!section.mr || int128_get64(section.size) < len) {
+    if (!section.mr) {
         goto out;
     }
+
+    size = int128_get64(section.size);
+    assert(size);
+
+    /* Passing mapped_len == NULL requires either a full mapping or

a failure. */

hw/virtio/dataplane/vring.c +9 -1

+    if (!mapped_len && size < len) {
+        goto out;
+    }
+
     if (is_write && section.readonly) {
         goto out;
     }

@@ -46,6 +61,10 @@ static void *vring_map(MemoryRegion **mr, hwaddr
 phys, hwaddr len, goto out;
     }
 
+    if (mapped_len) {
+        *mapped_len = MIN(size, len);
+    }
+
     *mr = section.mr;
     return memory_region_get_ram_ptr(section.mr) +
 section.offset_within_region; 

@@ -78,7 +97,7 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev,
 int n) addr = virtio_queue_get_desc_addr(vdev, n);
     size = virtio_queue_get_desc_size(vdev, n);
     /* Map the descriptor area as read only */
-    ptr = vring_map(&vring->mr_desc, addr, size, false);
+    ptr = vring_map(&vring->mr_desc, addr, size, NULL, false);
     if (!ptr) {
         error_report("Failed to map 0x%" HWADDR_PRIx " byte for
 vring desc " "at 0x%" HWADDR_PRIx,

@@ -92,7 +111,7 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev,

int n) /* Add the size of the used_event_idx */

     size += sizeof(uint16_t);
     /* Map the driver area as read only */

hw/virtio/dataplane/vring.c +1 -1

-    ptr = vring_map(&vring->mr_avail, addr, size, false);
+    ptr = vring_map(&vring->mr_avail, addr, size, NULL, false);
     if (!ptr) {
         error_report("Failed to map 0x%" HWADDR_PRIx " byte for

vring avail " "at 0x%" HWADDR_PRIx,

hw/virtio/dataplane/vring.c +17 -13

@@ -106,7 +125,7 @@ bool vring_setup(Vring *vring, VirtIODevice
 *vdev, int n) /* Add the size of the avail_event_idx */
     size += sizeof(uint16_t);
     /* Map the device area as read-write */
-    ptr = vring_map(&vring->mr_used, addr, size, true);
+    ptr = vring_map(&vring->mr_used, addr, size, NULL, true);
     if (!ptr) {
         error_report("Failed to map 0x%" HWADDR_PRIx " byte for
 vring used " "at 0x%" HWADDR_PRIx,

@@ -206,6 +225,7 @@ static int get_desc(Vring *vring,
 VirtQueueElement *elem, struct iovec *iov;
     hwaddr *addr;
     MemoryRegion *mr;
+    hwaddr len;
 
     if (desc->flags & VRING_DESC_F_WRITE) {
         num = &elem->in_num;

@@ -224,26 +244,30 @@ static int get_desc(Vring *vring,
 VirtQueueElement *elem, }
     }
 
-    /* Stop for now if there are not enough iovecs available. */
-    if (*num >= VIRTQUEUE_MAX_SIZE) {
-        error_report("Invalid SG num: %u", *num);
-        return -EFAULT;
-    }
+    while (desc->len) {
+        /* Stop for now if there are not enough iovecs available. */
+        if (*num >= VIRTQUEUE_MAX_SIZE) {
+            error_report("Invalid SG num: %u", *num);
+            return -EFAULT;
+        }
 
-    /* TODO handle non-contiguous memory across region boundaries */
-    iov->iov_base = vring_map(&mr, desc->addr, desc->len,
-                              desc->flags & VRING_DESC_F_WRITE);
-    if (!iov->iov_base) {
-        error_report("Failed to map descriptor addr %#" PRIx64 " len
 %u",
-                     (uint64_t)desc->addr, desc->len);
-        return -EFAULT;
+        iov->iov_base = vring_map(&mr, desc->addr, desc->len, &len,
+                                  desc->flags & VRING_DESC_F_WRITE);
+        if (!iov->iov_base) {
+            error_report("Failed to map descriptor addr %#" PRIx64 "
 len %u",
+                         (uint64_t)desc->addr, desc->len);
+            return -EFAULT;
+        }
+
+        /* The MemoryRegion is looked up again and unref'ed later,

leave the

hw/virtio/dataplane/vring.c +6 -1

+         * ref in place.  */
+        iov->iov_len = len;
+        *addr = desc->addr;
+        desc->len -= len;
+        desc->addr += len;
+        *num += 1;
     }
 
-    /* The MemoryRegion is looked up again and unref'ed later, leave

the

hw/virtio/dataplane/vring.c +0 -4

-     * ref in place.  */
-    iov->iov_len = desc->len;
-    *addr = desc->addr;
-    *num += 1;
     return 0;
 }

Use address_space_read to make sure we handle the case of an indirect descriptor crossing DIMM boundary correctly.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Warning: compile-tested only.

 hw/virtio/dataplane/vring.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

hw/virtio/dataplane/vring.c +15 -0

index 68f1994..0b92fcf 100644
@@ -257,6 +257,21 @@ static void copy_in_vring_desc(VirtIODevice
 *vdev, host->next = virtio_lduw_p(vdev, &guest->next);
 }
 
+static bool read_vring_desc(VirtIODevice *vdev,
+                            hwaddr guest,
+                            struct vring_desc *host)
+{
+    if (address_space_read(&address_space_memory, guest,
 MEMTXATTRS_UNSPECIFIED,
+                           (uint8_t *)host, sizeof *host)) {
+        return false;
+    }
+    host->addr = virtio_tswap64(vdev, host->addr);
+    host->len = virtio_tswap32(vdev, host->len);
+    host->flags = virtio_tswap16(vdev, host->flags);
+    host->next = virtio_tswap16(vdev, host->next);
+    return true;
+}
+
 /* This is stolen from linux/drivers/vhost/vhost.c. */
 static int get_indirect(VirtIODevice *vdev, Vring *vring,

VirtQueueElement *elem, struct vring_desc
*indirect) @@ -284,23 +299,16 @@ static int get_indirect(VirtIODevice *vdev, Vring *vring, }

     do {

hw/virtio/dataplane/vring.c +1 -3

-                             sizeof(desc), false);
-        if (!desc_ptr) {
-            error_report("Failed to map indirect descriptor "
+        if (!read_vring_desc(vdev, indirect->addr + found *

sizeof(desc),

hw/virtio/dataplane/vring.c +2 -0

+                             &desc)) {
+            error_report("Failed to read indirect descriptor "
                          "addr %#" PRIx64 " len %zu",
                          (uint64_t)indirect->addr + found *

sizeof(desc), sizeof(desc));
             vring->broken = true;
             return -EFAULT;
         }

hw/virtio/dataplane/vring.c +0 -2

-        copy_in_vring_desc(vdev, desc_ptr, &desc);
-        memory_region_unref(mr);
 
         /* Ensure descriptor has been loaded before accessing fields

*/ barrier(); /* read_barrier_depends(); */

hw/virtio/dataplane/vring.c static int get_indirect(VirtIODevice *vdev, Vring *vring, · line 292

Quoted 6 lines · On Wed, 28 Oct 2015 17:48:02 +0200 "Michael S. Tsirkin" <[email protected]> wrote:

hw/virtio/dataplane/vring.c +0 -5

-        struct vring_desc *desc_ptr;
-        MemoryRegion *mr;
-
         /* Translate indirect descriptor */
-        desc_ptr = vring_map(&mr,
-                             indirect->addr + found * sizeof(desc),

Is it correct to use 'found' as iterator here vs using desc.next as is done when translating direct descriptors?

Use address_space_read to make sure we handle the case of an indirect descriptor crossing DIMM boundary correctly.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Warning: compile-tested only.

 hw/virtio/dataplane/vring.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

hw/virtio/dataplane/vring.c +15 -0

index 68f1994..0b92fcf 100644
@@ -257,6 +257,21 @@ static void copy_in_vring_desc(VirtIODevice
 *vdev, host->next = virtio_lduw_p(vdev, &guest->next);
 }
 
+static bool read_vring_desc(VirtIODevice *vdev,
+                            hwaddr guest,
+                            struct vring_desc *host)
+{
+    if (address_space_read(&address_space_memory, guest,
 MEMTXATTRS_UNSPECIFIED,
+                           (uint8_t *)host, sizeof *host)) {
+        return false;
+    }
+    host->addr = virtio_tswap64(vdev, host->addr);
+    host->len = virtio_tswap32(vdev, host->len);
+    host->flags = virtio_tswap16(vdev, host->flags);
+    host->next = virtio_tswap16(vdev, host->next);
+    return true;
+}
+
 /* This is stolen from linux/drivers/vhost/vhost.c. */
 static int get_indirect(VirtIODevice *vdev, Vring *vring,

VirtQueueElement *elem, struct vring_desc
*indirect) @@ -284,23 +299,16 @@ static int get_indirect(VirtIODevice *vdev, Vring *vring, }

     do {

hw/virtio/dataplane/vring.c +1 -8

-        struct vring_desc *desc_ptr;
-        MemoryRegion *mr;
-
         /* Translate indirect descriptor */
-        desc_ptr = vring_map(&mr,
-                             indirect->addr + found * sizeof(desc),
-                             sizeof(desc), false);
-        if (!desc_ptr) {
-            error_report("Failed to map indirect descriptor "
+        if (!read_vring_desc(vdev, indirect->addr + found *

sizeof(desc),

hw/virtio/dataplane/vring.c +2 -0

+                             &desc)) {
+            error_report("Failed to read indirect descriptor "
                          "addr %#" PRIx64 " len %zu",
                          (uint64_t)indirect->addr + found *

sizeof(desc), sizeof(desc));
             vring->broken = true;
             return -EFAULT;
         }

hw/virtio/dataplane/vring.c +0 -2

-        copy_in_vring_desc(vdev, desc_ptr, &desc);
-        memory_region_unref(mr);
 
         /* Ensure descriptor has been loaded before accessing fields

*/ barrier(); /* read_barrier_depends(); */

Use address_space_read to make sure we handle the case of an indirect descriptor crossing DIMM boundary correctly.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Warning: compile-tested only.

 hw/virtio/dataplane/vring.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

Use address_space_read to make sure we handle the case of an indirect descriptor crossing DIMM boundary correctly.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Warning: compile-tested only.

 hw/virtio/dataplane/vring.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

Use address_space_read to make sure we handle the case of an indirect descriptor crossing DIMM boundary correctly.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Warning: compile-tested only.

Quoted 6 lines · Michael S. Tsirkin

Use address_space_read to make sure we handle the case of an indirect descriptor crossing DIMM boundary correctly.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Quoted 1 line

Warning: compile-tested only.

Test (with your follow-up patch squashed in) with 6 4KB seqeuential read processes on running successfully.

I didn't test the DIMM boundary case. Igor: what is the easiest way to reproduce the DIMM boundary crossing?