[Qemu-devel] [PATCH 1/2] dataplane: simplify indirect descriptor read

[Qemu-devel] [PATCH 1/2] dataplane: simplify indirect descriptor read

[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 <mst@redhat.com>
---

Warning: compile-tested only.

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

diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index 68f1994..0b92fcf 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -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 {
-        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),
+                             &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;
         }
-        copy_in_vring_desc(vdev, desc_ptr, &desc);
-        memory_region_unref(mr);
 
         /* Ensure descriptor has been loaded before accessing fields */
         barrier(); /* read_barrier_depends(); */
-- 
MST

[Qemu-devel] [PATCH 2/2] dataplane: support non-contigious s/g

[Michael S. Tsirkin]

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 <stefanha@redhat.com>
Reported-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Warning: compile-tested only.

 hw/virtio/dataplane/vring.c | 68 ++++++++++++++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 22 deletions(-)

diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index 0b92fcf..9ae9424 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -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.
  */
-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);
+    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. */
+    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 */
-    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,
@@ -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
+         * 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
-     * ref in place.  */
-    iov->iov_len = desc->len;
-    *addr = desc->addr;
-    *num += 1;
     return 0;
 }
 
-- 
MST

Re: [Qemu-devel] [PATCH 1/2] dataplane: simplify indirect descriptor read

[Igor Mammedov]

On Wed, 28 Oct 2015 17:48:02 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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 <mst@redhat.com>
> ---
> 
> Warning: compile-tested only.
> 
>  hw/virtio/dataplane/vring.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index 68f1994..0b92fcf 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -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 {
> -        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?


> -                             sizeof(desc), false);
> -        if (!desc_ptr) {
> -            error_report("Failed to map indirect descriptor "
> +        if (!read_vring_desc(vdev, indirect->addr + found *
> sizeof(desc),
> +                             &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;
>          }
> -        copy_in_vring_desc(vdev, desc_ptr, &desc);
> -        memory_region_unref(mr);
>  
>          /* Ensure descriptor has been loaded before accessing fields
> */ barrier(); /* read_barrier_depends(); */

Re: [Qemu-devel] [PATCH 1/2] dataplane: simplify indirect descriptor read

[Igor Mammedov]

On Wed, 28 Oct 2015 17:48:02 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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 <mst@redhat.com>
Tested-by: Igor Mammedov <imammedo@redhat.com>

> ---
> 
> Warning: compile-tested only.
> 
>  hw/virtio/dataplane/vring.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index 68f1994..0b92fcf 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -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 {
> -        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),
> +                             &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;
>          }
> -        copy_in_vring_desc(vdev, desc_ptr, &desc);
> -        memory_region_unref(mr);
>  
>          /* Ensure descriptor has been loaded before accessing fields
> */ barrier(); /* read_barrier_depends(); */

Re: [Qemu-devel] [PATCH 2/2] dataplane: support non-contigious s/g

[Igor Mammedov]

On Wed, 28 Oct 2015 17:48:04 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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 <stefanha@redhat.com>
> Reported-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
with later posted fixup
Tested-by: Igor Mammedov <imammedo@redhat.com>

> ---
> 
> Warning: compile-tested only.
> 
>  hw/virtio/dataplane/vring.c | 68
> ++++++++++++++++++++++++++++++--------------- 1 file changed, 46
> insertions(+), 22 deletions(-)
> 
> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index 0b92fcf..9ae9424 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -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. */
> -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);
> +    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. */
> +    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 */
> -    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,
> @@ -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
> +         * 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
> -     * ref in place.  */
> -    iov->iov_len = desc->len;
> -    *addr = desc->addr;
> -    *num += 1;
>      return 0;
>  }
>  

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] dataplane: simplify indirect descriptor read

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]

On Thu, Oct 29, 2015 at 11:28:05AM +0100, Igor Mammedov wrote:
> On Wed, 28 Oct 2015 17:48:02 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > -        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?

That is how Linux drivers/vhost/vhost.c:get_indirect() does it.

QEMU hw/virtio/virtio.c honors desc.next.

The VIRTIO 1.0 specification says:

  The first indirect descriptor is located at start of the indirect
  descriptor table (index 0), additional indirect descriptors are
  chained by next field.

I think vhost and dataplane do not follow the specification here.
Fixing it is for a separate patch series.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] dataplane: simplify indirect descriptor read

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

On Wed, Oct 28, 2015 at 05:48:02PM +0200, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>
> ---
> 
> Warning: compile-tested only.
> 
>  hw/virtio/dataplane/vring.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

I will test and merge (together with your fixup patch squashed in) on
Monday.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] dataplane: simplify indirect descriptor read

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

On Wed, Oct 28, 2015 at 05:48:02PM +0200, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>
> ---
> 
> Warning: compile-tested only.
> 
>  hw/virtio/dataplane/vring.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] dataplane: simplify indirect descriptor read

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

On Wed, Oct 28, 2015 at 05:48:02PM +0200, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>
> ---
> 
> 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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] dataplane: simplify indirect descriptor read

[Igor Mammedov]

On Mon, 2 Nov 2015 17:43:16 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Wed, Oct 28, 2015 at 05:48:02PM +0200, Michael S. Tsirkin wrote:
> > 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 <mst@redhat.com>
> > ---
> > 
> > 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?

I've tested it, here is QEMU CLI:
qemu-system-x86_64 -enable-kvm -enable-kvm  -m 128M,slots=250,maxmem=32G  -drive if=none,id=hd,file=rhel72,cache=none,aio=native,format=raw -device virtio-blk,drive=hd,scsi=off,config-wce=off,x-data-plane=on `for i in $(seq 0 15); do echo -n "-object memory-backend-ram,id=m$i,size=10M -device pc-dimm,id=dimm$i,memdev=m$i "; done`

boot and then do:
dd if=/dev/vda of=/dev/null bs=16M

without fix it hangs either at boot time or when doing 'dd'