[Qemu-devel] [PATCH 0/6] virtio: handle non contigious s/g entries

[Qemu-devel] [PATCH 0/6] virtio: handle non contigious s/g entries

[Michael S. Tsirkin]

TL;DR:
This fixes virtio in a way transparent to guest.
We should now be able to revert commits aa8580cd and df0acded19ec which worked
around it in a way that's not transparent.

-----

commit aa8580cd "pc: memhp: force gaps between DIMM's GPA"
introduced gaps in GPA space to work around a bug in virtio,
originally reported by Igor, and I quote:

------
    QEMU aborts during guest reboot with following backtrace:

    Breakpoint 1, virtqueue_map_sg (sg=0x555557cbc6c0, addr=0x555557cb86c0, 
    num_sg=0x12, is_write=0x1) at hw/virtio/virtio.c:453
    453                 error_report("virtio: error trying to map MMIO memory");
    (gdb) bt
    #0  virtqueue_map_sg (sg=0x555557cbc6c0, addr=0x555557cb86c0, num_sg=0x12, 
    is_write=0x1) at hw/virtio/virtio.c:453
    #1  0x000055555569b3ef in virtqueue_pop (vq=0x555558a3fab0, 
    elem=0x555557cb86b0) at hw/virtio/virtio.c:520
    #2  0x0000555555666611 in virtio_blk_get_request (s=0x5555588c7a00) at 
    hw/block/virtio-blk.c:194
    #3  0x00005555556676ec in virtio_blk_handle_output (vdev=0x5555588c7a00, 
    vq=0x555558a3fab0) at hw/block/virtio-blk.c:603
    #4  0x000055555569c5c8 in virtio_queue_notify_vq (vq=0x555558a3fab0) at 
    hw/virtio/virtio.c:921
    #5  0x000055555569e009 in virtio_queue_host_notifier_read (n=0x555558a3faf8) at 
    hw/virtio/virtio.c:1480
    #6  0x000055555591b062 in qemu_iohandler_poll (pollfds=0x555556363800, ret=0x1) 
    at iohandler.c:126
    #7  0x000055555591ad33 in main_loop_wait (nonblocking=0x0) at main-loop.c:503
    #8  0x00005555557466b5 in main_loop () at vl.c:1902
    #9  0x000055555574e69b in main (argc=0x4d, argv=0x7fffffffda18, 
    envp=0x7fffffffdc88) at vl.c:4653

    could be reproduced with following options:

    -enable-kvm  -m 1G,slots=250,maxmem=32G  -drive if=virtio,file=rhel72 -netdev 
    tap,id=foo,ifname=tap0,script=./qemu-ifup -device 
    virtio-net-pci,id=n1,netdev=foo `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` -snapshot -monitor unix:/tmp/m,server,nowait

    boot and login to guest shell and execute 'reboot' command
    on the reboot when guest kernel boots, QEMU will abort in virtio.
    Reproducible in about 80% cases. If QEMU doesn't crash on reboot
    then try again whit freshly started QEMU.


    Reason for crashing is that guest allocates buffer that crosses
    boundary between 2 different memory regions and as result
    cpu_physical_memory_map() maps GPA to HVA for only head of buffer
    that belongs to the first region which makes conditon
     len != sg[i].iov_len
    true since declared buffer size (sg[i].iov_len) isn't what
    cpu_physical_memory_map() has been able to map (len),
    which leads to abort:

    virtqueue_map_sg() {
      ...
      sg[i].iov_base = cpu_physical_memory_map(addr[i], &len, is_write);
      if (sg[i].iov_base == NULL || len != sg[i].iov_len) {
           abort()
------

However, the work-around regressed memory hot-unplug for linux guests
triggering the following within the guest:

------
 =====
 kernel BUG at mm/memory_hotplug.c:703!
 ...
 [<ffffffff81385fa7>] acpi_memory_device_remove+0x79/0xa5
 [<ffffffff81357818>] acpi_bus_trim+0x5a/0x8d
 [<ffffffff81359026>] acpi_device_hotplug+0x1b7/0x418
 ===
    BUG_ON(phys_start_pfn & ~PAGE_SECTION_MASK);
 ===

------

The reason for the crash is that x86-64 linux guest supports memory hotplug in
chunks of 128Mb and assumes that memory sections are also 128Mb aligned.
However gaps forced between 128Mb DIMMs with backend's natural alignment of 2Mb
make the 2nd and following DIMMs not being aligned on 128Mb boundary.

------

Michael S. Tsirkin (6):
  virtio: introduce virtio_map
  virtio: switch to virtio_map
  virtio-blk: convert to virtqueue_map
  virtio-serial: convert to virtio_map
  virtio-scsi: convert to virtqueue_map
  virtio: drop virtqueue_map_sg

 include/hw/virtio/virtio.h  |  3 +--
 hw/block/virtio-blk.c       |  5 +----
 hw/char/virtio-serial-bus.c |  5 +----
 hw/scsi/virtio-scsi.c       | 16 ++------------
 hw/virtio/virtio.c          | 52 +++++++++++++++++++++++++++++++++++----------
 5 files changed, 46 insertions(+), 35 deletions(-)

-- 
MST

[Qemu-devel] [PATCH 1/6] virtio: introduce virtio_map

[Michael S. Tsirkin]

virtio_map_sg currently fails if one of the entries it's mapping is
contigious in GPA but not HVA address space.  Introduce virtio_map which
handles this by splitting sg entries.

This new API generally turns out to be a good idea since it's harder to
misuse: at least in one case the existing one was used incorrectly.

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.

Note: virtio-scsi calls virtio_map_sg on data loaded from network, and
validates input, asserting on failure.  Copy the validating code here -
it will be dropped from virtio-scsi in a follow-up patch.

Reported-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/virtio/virtio.h |  1 +
 hw/virtio/virtio.c         | 56 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 9d09115..9d9abb4 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -153,6 +153,7 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
 
 void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
     size_t num_sg, int is_write);
+void virtqueue_map(VirtQueueElement *elem);
 int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem);
 int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
                           unsigned int out_bytes);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index d0bc72e..a6878c0 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -448,28 +448,66 @@ int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
     return in_bytes <= in_total && out_bytes <= out_total;
 }
 
-void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
-    size_t num_sg, int is_write)
+static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
+                                size_t *num_sg, size_t max_size,
+                                int is_write)
 {
     unsigned int i;
     hwaddr len;
 
-    if (num_sg > VIRTQUEUE_MAX_SIZE) {
-        error_report("virtio: map attempt out of bounds: %zd > %d",
-                     num_sg, VIRTQUEUE_MAX_SIZE);
-        exit(1);
-    }
+    /* Note: this function MUST validate input, some callers
+     * are passing in num_sg values received over the network.
+     */
+    /* TODO: teach all callers that this can fail, and return failure instead
+     * of asserting here.
+     * When we do, we might be able to re-enable NDEBUG below.
+     */
+#ifdef NDEBUG
+#error building with NDEBUG is not supported
+#endif
+    assert(*num_sg <= max_size);
 
-    for (i = 0; i < num_sg; i++) {
+    for (i = 0; i < *num_sg; i++) {
         len = sg[i].iov_len;
         sg[i].iov_base = cpu_physical_memory_map(addr[i], &len, is_write);
-        if (sg[i].iov_base == NULL || len != sg[i].iov_len) {
+        if (!sg[i].iov_base) {
             error_report("virtio: error trying to map MMIO memory");
             exit(1);
         }
+        if (len == sg[i].iov_len) {
+            continue;
+        }
+        if (*num_sg >= max_size) {
+            error_report("virtio: memory split makes iovec too large");
+            exit(1);
+        }
+        memcpy(sg + i + 1, sg + i, sizeof(*sg) * (*num_sg - i));
+        memcpy(addr + i + 1, addr + i, sizeof(*addr) * (*num_sg - i));
+        assert(len < sg[i + 1].iov_len);
+        sg[i].iov_len = len;
+        addr[i + 1] += len;
+        sg[i + 1].iov_len -= len;
+        ++*num_sg;
     }
 }
 
+/* Deprecated: don't use in new code */
+void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
+                      size_t num_sg, int is_write)
+{
+    virtqueue_map_iovec(sg, addr, &num_sg, num_sg, is_write);
+}
+
+void virtqueue_map(VirtQueueElement *elem)
+{
+    virtqueue_map_iovec(elem->in_sg, elem->in_addr, &elem->in_num,
+                        MIN(ARRAY_SIZE(elem->in_sg), ARRAY_SIZE(elem->in_addr)),
+                        1);
+    virtqueue_map_iovec(elem->out_sg, elem->out_addr, &elem->out_num,
+                        MIN(ARRAY_SIZE(elem->out_sg), ARRAY_SIZE(elem->out_addr)),
+                        0);
+}
+
 int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
 {
     unsigned int i, head, max;
-- 
MST

[Qemu-devel] [PATCH 2/6] virtio: switch to virtio_map

[Michael S. Tsirkin]

Drop use of the deprecated virtio_map_sg in virtio core.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index a6878c0..84e2320 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -569,8 +569,7 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
     } while ((i = virtqueue_next_desc(vdev, desc_pa, i, max)) != max);
 
     /* Now map what we have collected */
-    virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);
-    virtqueue_map_sg(elem->out_sg, elem->out_addr, elem->out_num, 0);
+    virtqueue_map(elem);
 
     elem->index = head;
 
-- 
MST

[Qemu-devel] [PATCH 3/6] virtio-blk: convert to virtqueue_map

[Michael S. Tsirkin]

Drop deprecated use of virtqueue_map_sg.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/block/virtio-blk.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 8beb26b..3e230de 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -839,10 +839,7 @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
         req->next = s->rq;
         s->rq = req;
 
-        virtqueue_map_sg(req->elem.in_sg, req->elem.in_addr,
-            req->elem.in_num, 1);
-        virtqueue_map_sg(req->elem.out_sg, req->elem.out_addr,
-            req->elem.out_num, 0);
+        virtqueue_map(&req->elem);
     }
 
     return 0;
-- 
MST

[Qemu-devel] [PATCH 4/6] virtio-serial: convert to virtio_map

[Michael S. Tsirkin]

This also fixes a minor bug:
-                virtqueue_map_sg(port->elem.out_sg, port->elem.out_addr,
-                                 port->elem.out_num, 1);
is wrong: out_sg is not written so should not be marked dirty.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/char/virtio-serial-bus.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index be97058..497b0af 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -705,10 +705,7 @@ static int fetch_active_ports_list(QEMUFile *f, int version_id,
 
                 qemu_get_buffer(f, (unsigned char *)&port->elem,
                                 sizeof(port->elem));
-                virtqueue_map_sg(port->elem.in_sg, port->elem.in_addr,
-                                 port->elem.in_num, 1);
-                virtqueue_map_sg(port->elem.out_sg, port->elem.out_addr,
-                                 port->elem.out_num, 1);
+                virtqueue_map(&port->elem);
 
                 /*
                  *  Port was throttled on source machine.  Let's
-- 
MST

[Qemu-devel] [PATCH 5/6] virtio-scsi: convert to virtqueue_map

[Michael S. Tsirkin]

Note: virtqueue_map already validates input
so virtio-scsi does not have to.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/scsi/virtio-scsi.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 1c33f14..33bd25a 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -207,20 +207,8 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
     assert(n < vs->conf.num_queues);
     req = virtio_scsi_init_req(s, vs->cmd_vqs[n]);
     qemu_get_buffer(f, (unsigned char *)&req->elem, sizeof(req->elem));
-    /* TODO: add a way for SCSIBusInfo's load_request to fail,
-     * and fail migration instead of asserting here.
-     * When we do, we might be able to re-enable NDEBUG below.
-     */
-#ifdef NDEBUG
-#error building with NDEBUG is not supported
-#endif
-    assert(req->elem.in_num <= ARRAY_SIZE(req->elem.in_sg));
-    assert(req->elem.out_num <= ARRAY_SIZE(req->elem.out_sg));
-
-    virtqueue_map_sg(req->elem.in_sg, req->elem.in_addr,
-                     req->elem.in_num, 1);
-    virtqueue_map_sg(req->elem.out_sg, req->elem.out_addr,
-                     req->elem.out_num, 0);
+
+    virtqueue_map(&req->elem);
 
     if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
                               sizeof(VirtIOSCSICmdResp) + vs->sense_size) < 0) {
-- 
MST

[Qemu-devel] [PATCH 6/6] virtio: drop virtqueue_map_sg

[Michael S. Tsirkin]

Deprecated in favor of virtqueue_map.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/virtio/virtio.h | 2 --
 hw/virtio/virtio.c         | 7 -------
 2 files changed, 9 deletions(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 9d9abb4..205fadf 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -151,8 +151,6 @@ void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
 void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
                     unsigned int len, unsigned int idx);
 
-void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
-    size_t num_sg, int is_write);
 void virtqueue_map(VirtQueueElement *elem);
 int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem);
 int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 84e2320..be32145 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -491,13 +491,6 @@ static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
     }
 }
 
-/* Deprecated: don't use in new code */
-void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
-                      size_t num_sg, int is_write)
-{
-    virtqueue_map_iovec(sg, addr, &num_sg, num_sg, is_write);
-}
-
 void virtqueue_map(VirtQueueElement *elem)
 {
     virtqueue_map_iovec(elem->in_sg, elem->in_addr, &elem->in_num,
-- 
MST

Re: [Qemu-devel] [PATCH 0/6] virtio: handle non contigious s/g entries

[Michael S. Tsirkin]

On Tue, Oct 27, 2015 at 10:47:54AM +0200, Michael S. Tsirkin wrote:
> TL;DR:
> This fixes virtio in a way transparent to guest.
> We should now be able to revert commits aa8580cd and df0acded19ec which worked
> around it in a way that's not transparent.

Note: I'm clean out of time so this is compiled only.
Igor said he can test. Thanks Igor!

> -----
> 
> commit aa8580cd "pc: memhp: force gaps between DIMM's GPA"
> introduced gaps in GPA space to work around a bug in virtio,
> originally reported by Igor, and I quote:
> 
> ------
>     QEMU aborts during guest reboot with following backtrace:
> 
>     Breakpoint 1, virtqueue_map_sg (sg=0x555557cbc6c0, addr=0x555557cb86c0, 
>     num_sg=0x12, is_write=0x1) at hw/virtio/virtio.c:453
>     453                 error_report("virtio: error trying to map MMIO memory");
>     (gdb) bt
>     #0  virtqueue_map_sg (sg=0x555557cbc6c0, addr=0x555557cb86c0, num_sg=0x12, 
>     is_write=0x1) at hw/virtio/virtio.c:453
>     #1  0x000055555569b3ef in virtqueue_pop (vq=0x555558a3fab0, 
>     elem=0x555557cb86b0) at hw/virtio/virtio.c:520
>     #2  0x0000555555666611 in virtio_blk_get_request (s=0x5555588c7a00) at 
>     hw/block/virtio-blk.c:194
>     #3  0x00005555556676ec in virtio_blk_handle_output (vdev=0x5555588c7a00, 
>     vq=0x555558a3fab0) at hw/block/virtio-blk.c:603
>     #4  0x000055555569c5c8 in virtio_queue_notify_vq (vq=0x555558a3fab0) at 
>     hw/virtio/virtio.c:921
>     #5  0x000055555569e009 in virtio_queue_host_notifier_read (n=0x555558a3faf8) at 
>     hw/virtio/virtio.c:1480
>     #6  0x000055555591b062 in qemu_iohandler_poll (pollfds=0x555556363800, ret=0x1) 
>     at iohandler.c:126
>     #7  0x000055555591ad33 in main_loop_wait (nonblocking=0x0) at main-loop.c:503
>     #8  0x00005555557466b5 in main_loop () at vl.c:1902
>     #9  0x000055555574e69b in main (argc=0x4d, argv=0x7fffffffda18, 
>     envp=0x7fffffffdc88) at vl.c:4653
> 
>     could be reproduced with following options:
> 
>     -enable-kvm  -m 1G,slots=250,maxmem=32G  -drive if=virtio,file=rhel72 -netdev 
>     tap,id=foo,ifname=tap0,script=./qemu-ifup -device 
>     virtio-net-pci,id=n1,netdev=foo `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` -snapshot -monitor unix:/tmp/m,server,nowait
> 
>     boot and login to guest shell and execute 'reboot' command
>     on the reboot when guest kernel boots, QEMU will abort in virtio.
>     Reproducible in about 80% cases. If QEMU doesn't crash on reboot
>     then try again whit freshly started QEMU.
> 
> 
>     Reason for crashing is that guest allocates buffer that crosses
>     boundary between 2 different memory regions and as result
>     cpu_physical_memory_map() maps GPA to HVA for only head of buffer
>     that belongs to the first region which makes conditon
>      len != sg[i].iov_len
>     true since declared buffer size (sg[i].iov_len) isn't what
>     cpu_physical_memory_map() has been able to map (len),
>     which leads to abort:
> 
>     virtqueue_map_sg() {
>       ...
>       sg[i].iov_base = cpu_physical_memory_map(addr[i], &len, is_write);
>       if (sg[i].iov_base == NULL || len != sg[i].iov_len) {
>            abort()
> ------
> 
> However, the work-around regressed memory hot-unplug for linux guests
> triggering the following within the guest:
> 
> ------
>  =====
>  kernel BUG at mm/memory_hotplug.c:703!
>  ...
>  [<ffffffff81385fa7>] acpi_memory_device_remove+0x79/0xa5
>  [<ffffffff81357818>] acpi_bus_trim+0x5a/0x8d
>  [<ffffffff81359026>] acpi_device_hotplug+0x1b7/0x418
>  ===
>     BUG_ON(phys_start_pfn & ~PAGE_SECTION_MASK);
>  ===
> 
> ------
> 
> The reason for the crash is that x86-64 linux guest supports memory hotplug in
> chunks of 128Mb and assumes that memory sections are also 128Mb aligned.
> However gaps forced between 128Mb DIMMs with backend's natural alignment of 2Mb
> make the 2nd and following DIMMs not being aligned on 128Mb boundary.
> 
> ------
> 
> Michael S. Tsirkin (6):
>   virtio: introduce virtio_map
>   virtio: switch to virtio_map
>   virtio-blk: convert to virtqueue_map
>   virtio-serial: convert to virtio_map
>   virtio-scsi: convert to virtqueue_map
>   virtio: drop virtqueue_map_sg
> 
>  include/hw/virtio/virtio.h  |  3 +--
>  hw/block/virtio-blk.c       |  5 +----
>  hw/char/virtio-serial-bus.c |  5 +----
>  hw/scsi/virtio-scsi.c       | 16 ++------------
>  hw/virtio/virtio.c          | 52 +++++++++++++++++++++++++++++++++++----------
>  5 files changed, 46 insertions(+), 35 deletions(-)
> 
> -- 
> MST
> 

Re: [Qemu-devel] [PATCH 0/6] virtio: handle non contigious s/g entries

[Michael S. Tsirkin]

On Tue, Oct 27, 2015 at 10:47:54AM +0200, Michael S. Tsirkin wrote:
> TL;DR:
> This fixes virtio in a way transparent to guest.
> We should now be able to revert commits aa8580cd and df0acded19ec which worked
> around it in a way that's not transparent.

I didn't check dataplane BTW. Igor? Stefan?

> -----
> 
> commit aa8580cd "pc: memhp: force gaps between DIMM's GPA"
> introduced gaps in GPA space to work around a bug in virtio,
> originally reported by Igor, and I quote:
> 
> ------
>     QEMU aborts during guest reboot with following backtrace:
> 
>     Breakpoint 1, virtqueue_map_sg (sg=0x555557cbc6c0, addr=0x555557cb86c0, 
>     num_sg=0x12, is_write=0x1) at hw/virtio/virtio.c:453
>     453                 error_report("virtio: error trying to map MMIO memory");
>     (gdb) bt
>     #0  virtqueue_map_sg (sg=0x555557cbc6c0, addr=0x555557cb86c0, num_sg=0x12, 
>     is_write=0x1) at hw/virtio/virtio.c:453
>     #1  0x000055555569b3ef in virtqueue_pop (vq=0x555558a3fab0, 
>     elem=0x555557cb86b0) at hw/virtio/virtio.c:520
>     #2  0x0000555555666611 in virtio_blk_get_request (s=0x5555588c7a00) at 
>     hw/block/virtio-blk.c:194
>     #3  0x00005555556676ec in virtio_blk_handle_output (vdev=0x5555588c7a00, 
>     vq=0x555558a3fab0) at hw/block/virtio-blk.c:603
>     #4  0x000055555569c5c8 in virtio_queue_notify_vq (vq=0x555558a3fab0) at 
>     hw/virtio/virtio.c:921
>     #5  0x000055555569e009 in virtio_queue_host_notifier_read (n=0x555558a3faf8) at 
>     hw/virtio/virtio.c:1480
>     #6  0x000055555591b062 in qemu_iohandler_poll (pollfds=0x555556363800, ret=0x1) 
>     at iohandler.c:126
>     #7  0x000055555591ad33 in main_loop_wait (nonblocking=0x0) at main-loop.c:503
>     #8  0x00005555557466b5 in main_loop () at vl.c:1902
>     #9  0x000055555574e69b in main (argc=0x4d, argv=0x7fffffffda18, 
>     envp=0x7fffffffdc88) at vl.c:4653
> 
>     could be reproduced with following options:
> 
>     -enable-kvm  -m 1G,slots=250,maxmem=32G  -drive if=virtio,file=rhel72 -netdev 
>     tap,id=foo,ifname=tap0,script=./qemu-ifup -device 
>     virtio-net-pci,id=n1,netdev=foo `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` -snapshot -monitor unix:/tmp/m,server,nowait
> 
>     boot and login to guest shell and execute 'reboot' command
>     on the reboot when guest kernel boots, QEMU will abort in virtio.
>     Reproducible in about 80% cases. If QEMU doesn't crash on reboot
>     then try again whit freshly started QEMU.
> 
> 
>     Reason for crashing is that guest allocates buffer that crosses
>     boundary between 2 different memory regions and as result
>     cpu_physical_memory_map() maps GPA to HVA for only head of buffer
>     that belongs to the first region which makes conditon
>      len != sg[i].iov_len
>     true since declared buffer size (sg[i].iov_len) isn't what
>     cpu_physical_memory_map() has been able to map (len),
>     which leads to abort:
> 
>     virtqueue_map_sg() {
>       ...
>       sg[i].iov_base = cpu_physical_memory_map(addr[i], &len, is_write);
>       if (sg[i].iov_base == NULL || len != sg[i].iov_len) {
>            abort()
> ------
> 
> However, the work-around regressed memory hot-unplug for linux guests
> triggering the following within the guest:
> 
> ------
>  =====
>  kernel BUG at mm/memory_hotplug.c:703!
>  ...
>  [<ffffffff81385fa7>] acpi_memory_device_remove+0x79/0xa5
>  [<ffffffff81357818>] acpi_bus_trim+0x5a/0x8d
>  [<ffffffff81359026>] acpi_device_hotplug+0x1b7/0x418
>  ===
>     BUG_ON(phys_start_pfn & ~PAGE_SECTION_MASK);
>  ===
> 
> ------
> 
> The reason for the crash is that x86-64 linux guest supports memory hotplug in
> chunks of 128Mb and assumes that memory sections are also 128Mb aligned.
> However gaps forced between 128Mb DIMMs with backend's natural alignment of 2Mb
> make the 2nd and following DIMMs not being aligned on 128Mb boundary.
> 
> ------
> 
> Michael S. Tsirkin (6):
>   virtio: introduce virtio_map
>   virtio: switch to virtio_map
>   virtio-blk: convert to virtqueue_map
>   virtio-serial: convert to virtio_map
>   virtio-scsi: convert to virtqueue_map
>   virtio: drop virtqueue_map_sg
> 
>  include/hw/virtio/virtio.h  |  3 +--
>  hw/block/virtio-blk.c       |  5 +----
>  hw/char/virtio-serial-bus.c |  5 +----
>  hw/scsi/virtio-scsi.c       | 16 ++------------
>  hw/virtio/virtio.c          | 52 +++++++++++++++++++++++++++++++++++----------
>  5 files changed, 46 insertions(+), 35 deletions(-)
> 
> -- 
> MST
> 

Re: [Qemu-devel] [PATCH 0/6] virtio: handle non contigious s/g entries

[Cornelia Huck]

On Tue, 27 Oct 2015 10:47:54 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> TL;DR:
> This fixes virtio in a way transparent to guest.
> We should now be able to revert commits aa8580cd and df0acded19ec which worked
> around it in a way that's not transparent.

(...)

> Michael S. Tsirkin (6):
>   virtio: introduce virtio_map
>   virtio: switch to virtio_map
>   virtio-blk: convert to virtqueue_map
>   virtio-serial: convert to virtio_map
>   virtio-scsi: convert to virtqueue_map
>   virtio: drop virtqueue_map_sg
> 
>  include/hw/virtio/virtio.h  |  3 +--
>  hw/block/virtio-blk.c       |  5 +----
>  hw/char/virtio-serial-bus.c |  5 +----
>  hw/scsi/virtio-scsi.c       | 16 ++------------
>  hw/virtio/virtio.c          | 52 +++++++++++++++++++++++++++++++++++----------
>  5 files changed, 46 insertions(+), 35 deletions(-)

Does not compile for me on s390:

/home/cohuck/git/qemu/hw/virtio/virtio.c: In function ‘virtqueue_map’:
/home/cohuck/git/qemu/hw/virtio/virtio.c:498:25: error: passing argument 3 of ‘virtqueue_map_iovec’ from incompatible pointer type [-Werror]
                         1);
                         ^
/home/cohuck/git/qemu/hw/virtio/virtio.c:451:13: note: expected ‘size_t *’ but argument is of type ‘unsigned int *’
 static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
             ^
/home/cohuck/git/qemu/hw/virtio/virtio.c:501:25: error: passing argument 3 of ‘virtqueue_map_iovec’ from incompatible pointer type [-Werror]
                         0);
                         ^
/home/cohuck/git/qemu/hw/virtio/virtio.c:451:13: note: expected ‘size_t *’ but argument is of type ‘unsigned int *’
 static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
             ^

Re: [Qemu-devel] [PATCH 1/6] virtio: introduce virtio_map

[Stefan Hajnoczi]

On Tue, Oct 27, 2015 at 10:47:56AM +0200, Michael S. Tsirkin wrote:
> +    for (i = 0; i < *num_sg; i++) {
>          len = sg[i].iov_len;
>          sg[i].iov_base = cpu_physical_memory_map(addr[i], &len, is_write);
> -        if (sg[i].iov_base == NULL || len != sg[i].iov_len) {
> +        if (!sg[i].iov_base) {
>              error_report("virtio: error trying to map MMIO memory");
>              exit(1);
>          }
> +        if (len == sg[i].iov_len) {
> +            continue;
> +        }
> +        if (*num_sg >= max_size) {
> +            error_report("virtio: memory split makes iovec too large");
> +            exit(1);
> +        }
> +        memcpy(sg + i + 1, sg + i, sizeof(*sg) * (*num_sg - i));
> +        memcpy(addr + i + 1, addr + i, sizeof(*addr) * (*num_sg - i));

These should be memmove() since memcpy() arguments are not allowed to overlap.

Re: [Qemu-devel] [PATCH 1/6] virtio: introduce virtio_map

[Stefan Hajnoczi]

On Tue, Oct 27, 2015 at 10:47:56AM +0200, Michael S. Tsirkin wrote:
> 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.

Don't forget about indirect descriptors.  They can use all 1024 iovecs,
regardless of virtqueue size, so virtqueue size of 256 isn't the true
maximum.

I'm worried that we could now see failures due to non-contiguous HVAs.

Re: [Qemu-devel] [PATCH 0/6] virtio: handle non contigious s/g entries

[Stefan Hajnoczi]

On Tue, Oct 27, 2015 at 01:51:20PM +0200, Michael S. Tsirkin wrote:
> On Tue, Oct 27, 2015 at 10:47:54AM +0200, Michael S. Tsirkin wrote:
> > TL;DR:
> > This fixes virtio in a way transparent to guest.
> > We should now be able to revert commits aa8580cd and df0acded19ec which worked
> > around it in a way that's not transparent.
> 
> I didn't check dataplane BTW. Igor? Stefan?

It doesn't handle it:

    /* 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;
    }

The assumption is that vring_map() can map the full desc->len bytes.

The same logic that you're trying to add in this series could be added to
vring_pop() but it makes the number of sg list entries accepted by QEMU
variable and dependent on DIMMs :(.  Previously guests could be sure that 1024
sgs work.

Re: [Qemu-devel] [PATCH 1/6] virtio: introduce virtio_map

[Michael S. Tsirkin]

On Tue, Oct 27, 2015 at 04:19:54PM +0000, Stefan Hajnoczi wrote:
> On Tue, Oct 27, 2015 at 10:47:56AM +0200, Michael S. Tsirkin wrote:
> > 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.
> 
> Don't forget about indirect descriptors.  They can use all 1024 iovecs,
> regardless of virtqueue size, so virtqueue size of 256 isn't the true
> maximum.

Not according to the spec - virtio spec says vq size is the maximum size
of a chain.

> I'm worried that we could now see failures due to non-contiguous HVAs.

Does linux guest create chains > vq size then? Does it actually
have 1024 hardcoded somewhere?

-- 
MST

Re: [Qemu-devel] [PATCH 1/6] virtio: introduce virtio_map

[Michael S. Tsirkin]

On Tue, Oct 27, 2015 at 04:13:12PM +0000, Stefan Hajnoczi wrote:
> On Tue, Oct 27, 2015 at 10:47:56AM +0200, Michael S. Tsirkin wrote:
> > +    for (i = 0; i < *num_sg; i++) {
> >          len = sg[i].iov_len;
> >          sg[i].iov_base = cpu_physical_memory_map(addr[i], &len, is_write);
> > -        if (sg[i].iov_base == NULL || len != sg[i].iov_len) {
> > +        if (!sg[i].iov_base) {
> >              error_report("virtio: error trying to map MMIO memory");
> >              exit(1);
> >          }
> > +        if (len == sg[i].iov_len) {
> > +            continue;
> > +        }
> > +        if (*num_sg >= max_size) {
> > +            error_report("virtio: memory split makes iovec too large");
> > +            exit(1);
> > +        }
> > +        memcpy(sg + i + 1, sg + i, sizeof(*sg) * (*num_sg - i));
> > +        memcpy(addr + i + 1, addr + i, sizeof(*addr) * (*num_sg - i));
> 
> These should be memmove() since memcpy() arguments are not allowed to overlap.

Oh right. Will fix, thanks!

Re: [Qemu-devel] [PATCH 1/6] virtio: introduce virtio_map

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2015 at 08:34:32PM +0200, Michael S. Tsirkin wrote:
> On Tue, Oct 27, 2015 at 04:19:54PM +0000, Stefan Hajnoczi wrote:
> > On Tue, Oct 27, 2015 at 10:47:56AM +0200, Michael S. Tsirkin wrote:
> > > 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.
> > 
> > Don't forget about indirect descriptors.  They can use all 1024 iovecs,
> > regardless of virtqueue size, so virtqueue size of 256 isn't the true
> > maximum.
> 
> Not according to the spec - virtio spec says vq size is the maximum size
> of a chain.
> 
> > I'm worried that we could now see failures due to non-contiguous HVAs.
> 
> Does linux guest create chains > vq size then? Does it actually
> have 1024 hardcoded somewhere?

You are correct, drivers/virtio/virtio_ring.c:virtqueue_add() says:

  BUG_ON(total_sg > vq->vring.num);

This also makes sense since it means there is a well-known maximum size
for indirect descriptor tables.

So this fix should work fine with indirect descriptors.

Stefan

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

Re: [Qemu-devel] [PATCH 1/6] virtio: introduce virtio_map

[Igor Mammedov]

On Tue, 27 Oct 2015 10:47:56 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> virtio_map_sg currently fails if one of the entries it's mapping is
> contigious in GPA but not HVA address space.  Introduce virtio_map which
> handles this by splitting sg entries.
> 
> This new API generally turns out to be a good idea since it's harder to
> misuse: at least in one case the existing one was used incorrectly.
> 
> 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.
                     ^^ S/is/if/
> 
> 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.
> 
> Note: virtio-scsi calls virtio_map_sg on data loaded from network, and
> validates input, asserting on failure.  Copy the validating code here -
> it will be dropped from virtio-scsi in a follow-up patch.
> 
> Reported-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/virtio/virtio.h |  1 +
>  hw/virtio/virtio.c         | 56 ++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 48 insertions(+), 9 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 9d09115..9d9abb4 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -153,6 +153,7 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
>  
>  void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
>      size_t num_sg, int is_write);
> +void virtqueue_map(VirtQueueElement *elem);
>  int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem);
>  int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
>                            unsigned int out_bytes);
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index d0bc72e..a6878c0 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -448,28 +448,66 @@ int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
>      return in_bytes <= in_total && out_bytes <= out_total;
>  }
>  
> -void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
> -    size_t num_sg, int is_write)
> +static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
> +                                size_t *num_sg, size_t max_size,
> +                                int is_write)
this fails to build with:
hw/virtio/virtio.c:498:25: error: passing argument 3 of ‘virtqueue_map_iovec’ from incompatible pointer type [-Werror]

here is fixup:
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 10cd03a..ef42baa 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -449,7 +449,7 @@ int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
 }
 
 static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
-                                size_t *num_sg, size_t max_size,
+                                unsigned int *num_sg, size_t max_size,
                                 int is_write)


>  {
>      unsigned int i;
>      hwaddr len;
>  
> -    if (num_sg > VIRTQUEUE_MAX_SIZE) {
> -        error_report("virtio: map attempt out of bounds: %zd > %d",
> -                     num_sg, VIRTQUEUE_MAX_SIZE);
> -        exit(1);
> -    }
> +    /* Note: this function MUST validate input, some callers
> +     * are passing in num_sg values received over the network.
> +     */
> +    /* TODO: teach all callers that this can fail, and return failure instead
> +     * of asserting here.
> +     * When we do, we might be able to re-enable NDEBUG below.
> +     */
> +#ifdef NDEBUG
> +#error building with NDEBUG is not supported
> +#endif
> +    assert(*num_sg <= max_size);
>  
> -    for (i = 0; i < num_sg; i++) {
> +    for (i = 0; i < *num_sg; i++) {
>          len = sg[i].iov_len;
>          sg[i].iov_base = cpu_physical_memory_map(addr[i], &len, is_write);
> -        if (sg[i].iov_base == NULL || len != sg[i].iov_len) {
> +        if (!sg[i].iov_base) {
>              error_report("virtio: error trying to map MMIO memory");
>              exit(1);
>          }
> +        if (len == sg[i].iov_len) {
> +            continue;
> +        }
> +        if (*num_sg >= max_size) {
> +            error_report("virtio: memory split makes iovec too large");
> +            exit(1);
> +        }
> +        memcpy(sg + i + 1, sg + i, sizeof(*sg) * (*num_sg - i));
> +        memcpy(addr + i + 1, addr + i, sizeof(*addr) * (*num_sg - i));
> +        assert(len < sg[i + 1].iov_len);
> +        sg[i].iov_len = len;
> +        addr[i + 1] += len;
> +        sg[i + 1].iov_len -= len;
> +        ++*num_sg;
>      }
>  }
>  
> +/* Deprecated: don't use in new code */
> +void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
> +                      size_t num_sg, int is_write)
> +{
> +    virtqueue_map_iovec(sg, addr, &num_sg, num_sg, is_write);
> +}
> +
> +void virtqueue_map(VirtQueueElement *elem)
> +{
> +    virtqueue_map_iovec(elem->in_sg, elem->in_addr, &elem->in_num,
> +                        MIN(ARRAY_SIZE(elem->in_sg), ARRAY_SIZE(elem->in_addr)),
> +                        1);
> +    virtqueue_map_iovec(elem->out_sg, elem->out_addr, &elem->out_num,
> +                        MIN(ARRAY_SIZE(elem->out_sg), ARRAY_SIZE(elem->out_addr)),
> +                        0);
> +}
> +
>  int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
>  {
>      unsigned int i, head, max;

Re: [Qemu-devel] [PATCH 0/6] virtio: handle non contigious s/g entries

[Igor Mammedov]

On Tue, 27 Oct 2015 13:51:20 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Oct 27, 2015 at 10:47:54AM +0200, Michael S. Tsirkin wrote:
> > TL;DR:
> > This fixes virtio in a way transparent to guest.
> > We should now be able to revert commits aa8580cd and df0acded19ec which worked
> > around it in a way that's not transparent.
> 
> I didn't check dataplane BTW. Igor? Stefan?
verified that series fixes virtio-[blk|scsi|net], all of them
hit at least one descriptor(indirect) that crosses DIMM boundary
and QEMU survived it.

However as Stefan has said virtio-blk with dataplane enabled hangs
guest instead of QEMU crashing and QEMU prints following error:

"Failed to map descriptor addr 0x1045eb000 len 106496"

I've used following CLI:
qemu-system-x86_64 -enable-kvm -enable-kvm  -m 128M,slots=250,maxmem=32G  -drive if=none,id=hd,file=rhel72.img,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`

it hangs at boot time or on executing 'dd if=/dev/vda of=/dev/null bs 32M'

Re: [Qemu-devel] [PATCH 2/6] virtio: switch to virtio_map

[Igor Mammedov]

On Tue, 27 Oct 2015 10:47:58 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Drop use of the deprecated virtio_map_sg in virtio core.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/virtio/virtio.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index a6878c0..84e2320 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -569,8 +569,7 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
>      } while ((i = virtqueue_next_desc(vdev, desc_pa, i, max)) != max);
>  
>      /* Now map what we have collected */
> -    virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);
> -    virtqueue_map_sg(elem->out_sg, elem->out_addr, elem->out_num, 0);
> +    virtqueue_map(elem);
>  
>      elem->index = head;
>  

Re: [Qemu-devel] [PATCH 3/6] virtio-blk: convert to virtqueue_map

[Igor Mammedov]

On Tue, 27 Oct 2015 10:48:01 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Drop deprecated use of virtqueue_map_sg.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/block/virtio-blk.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 8beb26b..3e230de 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -839,10 +839,7 @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
>          req->next = s->rq;
>          s->rq = req;
>  
> -        virtqueue_map_sg(req->elem.in_sg, req->elem.in_addr,
> -            req->elem.in_num, 1);
> -        virtqueue_map_sg(req->elem.out_sg, req->elem.out_addr,
> -            req->elem.out_num, 0);
> +        virtqueue_map(&req->elem);
>      }
>  
>      return 0;

Re: [Qemu-devel] [PATCH 4/6] virtio-serial: convert to virtio_map

[Igor Mammedov]

On Tue, 27 Oct 2015 10:48:03 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> This also fixes a minor bug:
> -                virtqueue_map_sg(port->elem.out_sg, port->elem.out_addr,
> -                                 port->elem.out_num, 1);
> is wrong: out_sg is not written so should not be marked dirty.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/char/virtio-serial-bus.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index be97058..497b0af 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -705,10 +705,7 @@ static int fetch_active_ports_list(QEMUFile *f, int version_id,
>  
>                  qemu_get_buffer(f, (unsigned char *)&port->elem,
>                                  sizeof(port->elem));
> -                virtqueue_map_sg(port->elem.in_sg, port->elem.in_addr,
> -                                 port->elem.in_num, 1);
> -                virtqueue_map_sg(port->elem.out_sg, port->elem.out_addr,
> -                                 port->elem.out_num, 1);
> +                virtqueue_map(&port->elem);
>  
>                  /*
>                   *  Port was throttled on source machine.  Let's

Re: [Qemu-devel] [PATCH 5/6] virtio-scsi: convert to virtqueue_map

[Igor Mammedov]

On Tue, 27 Oct 2015 10:48:06 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Note: virtqueue_map already validates input
> so virtio-scsi does not have to.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/scsi/virtio-scsi.c | 16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 1c33f14..33bd25a 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -207,20 +207,8 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
>      assert(n < vs->conf.num_queues);
>      req = virtio_scsi_init_req(s, vs->cmd_vqs[n]);
>      qemu_get_buffer(f, (unsigned char *)&req->elem, sizeof(req->elem));
> -    /* TODO: add a way for SCSIBusInfo's load_request to fail,
> -     * and fail migration instead of asserting here.
> -     * When we do, we might be able to re-enable NDEBUG below.
> -     */
> -#ifdef NDEBUG
> -#error building with NDEBUG is not supported
> -#endif
> -    assert(req->elem.in_num <= ARRAY_SIZE(req->elem.in_sg));
> -    assert(req->elem.out_num <= ARRAY_SIZE(req->elem.out_sg));
> -
> -    virtqueue_map_sg(req->elem.in_sg, req->elem.in_addr,
> -                     req->elem.in_num, 1);
> -    virtqueue_map_sg(req->elem.out_sg, req->elem.out_addr,
> -                     req->elem.out_num, 0);
> +
> +    virtqueue_map(&req->elem);
>  
>      if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
>                                sizeof(VirtIOSCSICmdResp) + vs->sense_size) < 0) {

Re: [Qemu-devel] [PATCH 6/6] virtio: drop virtqueue_map_sg

[Igor Mammedov]

On Tue, 27 Oct 2015 10:48:08 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Deprecated in favor of virtqueue_map.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  include/hw/virtio/virtio.h | 2 --
>  hw/virtio/virtio.c         | 7 -------
>  2 files changed, 9 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 9d9abb4..205fadf 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -151,8 +151,6 @@ void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
>  void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
>                      unsigned int len, unsigned int idx);
>  
> -void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
> -    size_t num_sg, int is_write);
>  void virtqueue_map(VirtQueueElement *elem);
>  int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem);
>  int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 84e2320..be32145 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -491,13 +491,6 @@ static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
>      }
>  }
>  
> -/* Deprecated: don't use in new code */
> -void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
> -                      size_t num_sg, int is_write)
> -{
> -    virtqueue_map_iovec(sg, addr, &num_sg, num_sg, is_write);
> -}
> -
>  void virtqueue_map(VirtQueueElement *elem)
>  {
>      virtqueue_map_iovec(elem->in_sg, elem->in_addr, &elem->in_num,

Re: [Qemu-devel] [PATCH 1/6] virtio: introduce virtio_map

[Igor Mammedov]

On Tue, 27 Oct 2015 10:47:56 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> virtio_map_sg currently fails if one of the entries it's mapping is
> contigious in GPA but not HVA address space.  Introduce virtio_map which
> handles this by splitting sg entries.
> 
> This new API generally turns out to be a good idea since it's harder to
> misuse: at least in one case the existing one was used incorrectly.
> 
> 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.
> 
> Note: virtio-scsi calls virtio_map_sg on data loaded from network, and
> validates input, asserting on failure.  Copy the validating code here -
> it will be dropped from virtio-scsi in a follow-up patch.
> 
> Reported-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

With fixup you've posted and build fix in this thread:
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  include/hw/virtio/virtio.h |  1 +
>  hw/virtio/virtio.c         | 56 ++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 48 insertions(+), 9 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 9d09115..9d9abb4 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -153,6 +153,7 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
>  
>  void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
>      size_t num_sg, int is_write);
> +void virtqueue_map(VirtQueueElement *elem);
>  int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem);
>  int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
>                            unsigned int out_bytes);
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index d0bc72e..a6878c0 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -448,28 +448,66 @@ int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
>      return in_bytes <= in_total && out_bytes <= out_total;
>  }
>  
> -void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
> -    size_t num_sg, int is_write)
> +static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
> +                                size_t *num_sg, size_t max_size,
> +                                int is_write)
>  {
>      unsigned int i;
>      hwaddr len;
>  
> -    if (num_sg > VIRTQUEUE_MAX_SIZE) {
> -        error_report("virtio: map attempt out of bounds: %zd > %d",
> -                     num_sg, VIRTQUEUE_MAX_SIZE);
> -        exit(1);
> -    }
> +    /* Note: this function MUST validate input, some callers
> +     * are passing in num_sg values received over the network.
> +     */
> +    /* TODO: teach all callers that this can fail, and return failure instead
> +     * of asserting here.
> +     * When we do, we might be able to re-enable NDEBUG below.
> +     */
> +#ifdef NDEBUG
> +#error building with NDEBUG is not supported
> +#endif
> +    assert(*num_sg <= max_size);
>  
> -    for (i = 0; i < num_sg; i++) {
> +    for (i = 0; i < *num_sg; i++) {
>          len = sg[i].iov_len;
>          sg[i].iov_base = cpu_physical_memory_map(addr[i], &len, is_write);
> -        if (sg[i].iov_base == NULL || len != sg[i].iov_len) {
> +        if (!sg[i].iov_base) {
>              error_report("virtio: error trying to map MMIO memory");
>              exit(1);
>          }
> +        if (len == sg[i].iov_len) {
> +            continue;
> +        }
> +        if (*num_sg >= max_size) {
> +            error_report("virtio: memory split makes iovec too large");
> +            exit(1);
> +        }
> +        memcpy(sg + i + 1, sg + i, sizeof(*sg) * (*num_sg - i));
> +        memcpy(addr + i + 1, addr + i, sizeof(*addr) * (*num_sg - i));
> +        assert(len < sg[i + 1].iov_len);
> +        sg[i].iov_len = len;
> +        addr[i + 1] += len;
> +        sg[i + 1].iov_len -= len;
> +        ++*num_sg;
>      }
>  }
>  
> +/* Deprecated: don't use in new code */
> +void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
> +                      size_t num_sg, int is_write)
> +{
> +    virtqueue_map_iovec(sg, addr, &num_sg, num_sg, is_write);
> +}
> +
> +void virtqueue_map(VirtQueueElement *elem)
> +{
> +    virtqueue_map_iovec(elem->in_sg, elem->in_addr, &elem->in_num,
> +                        MIN(ARRAY_SIZE(elem->in_sg), ARRAY_SIZE(elem->in_addr)),
> +                        1);
> +    virtqueue_map_iovec(elem->out_sg, elem->out_addr, &elem->out_num,
> +                        MIN(ARRAY_SIZE(elem->out_sg), ARRAY_SIZE(elem->out_addr)),
> +                        0);
> +}
> +
>  int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
>  {
>      unsigned int i, head, max;