[Qemu-devel] [PATCH 0/7] virtio-serial: Don't copy guest buf to host, flow control

[Qemu-devel] [PATCH 0/7] virtio-serial: Don't copy guest buf to host, flow control

[Amit Shah]

Hello,

This series is now separated from the chardev flow control series.
The virtio-serial code now does not copy over data from the guest to
the host.  It instead keeps track of how far we are in consuming the
data and maintains this state.

For flow control, when a user of the virtio-serial port signals it has
consumed less data than given, port throttling is enabled.  The
consumer can then later disable throttling and we can re-start sending
the data from where we left off.

Finally, new fields introduced are added to the save/restore section
to preserve state across live migrations.

Please apply.

Amit Shah (7):
  virtio-console: Factor out common init between console and generic
    ports
  virtio-console: Remove unnecessary braces
  virtio-serial-bus: separate out discard logic in a separate function
  virtio-serial: Don't copy over guest buffer to host
  virtio-serial: Let virtio-serial-bus know if all data was consumed
  virtio-serial: Add support for flow control
  virtio-serial: save/restore new fields in port struct

 hw/virtio-console.c    |   38 +++++++--------
 hw/virtio-serial-bus.c |  123 +++++++++++++++++++++++++++++++++++++++---------
 hw/virtio-serial.h     |   24 ++++++++-
 3 files changed, 139 insertions(+), 46 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 1/7] virtio-console: Factor out common init between console and generic ports

[Amit Shah]

The initialisation for generic ports and console ports is similar.
Factor out the parts that are the same in a different function that can
be called from each of the initfns.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c |   31 ++++++++++++++-----------------
 1 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index caea11f..d7fe68b 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -58,24 +58,28 @@ static void chr_event(void *opaque, int event)
     }
 }
 
-/* Virtio Console Ports */
-static int virtconsole_initfn(VirtIOSerialDevice *dev)
+static int generic_port_init(VirtConsole *vcon, VirtIOSerialDevice *dev)
 {
-    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
-    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
-
-    port->info = dev->info;
-
-    port->is_console = true;
+    vcon->port.info = dev->info;
 
     if (vcon->chr) {
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
                               vcon);
-        port->info->have_data = flush_buf;
+        vcon->port.info->have_data = flush_buf;
     }
     return 0;
 }
 
+/* Virtio Console Ports */
+static int virtconsole_initfn(VirtIOSerialDevice *dev)
+{
+    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
+    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+
+    port->is_console = true;
+    return generic_port_init(vcon, dev);
+}
+
 static int virtconsole_exitfn(VirtIOSerialDevice *dev)
 {
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
@@ -115,14 +119,7 @@ static int virtserialport_initfn(VirtIOSerialDevice *dev)
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
-    port->info = dev->info;
-
-    if (vcon->chr) {
-        qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
-                              vcon);
-        port->info->have_data = flush_buf;
-    }
-    return 0;
+    return generic_port_init(vcon, dev);
 }
 
 static VirtIOSerialPortInfo virtserialport_info = {
-- 
1.7.3.4

[Qemu-devel] [PATCH 2/7] virtio-console: Remove unnecessary braces

[Amit Shah]

Remove unnecessary braces around a case statement.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index d7fe68b..d0b9354 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -48,10 +48,9 @@ static void chr_event(void *opaque, int event)
     VirtConsole *vcon = opaque;
 
     switch (event) {
-    case CHR_EVENT_OPENED: {
+    case CHR_EVENT_OPENED:
         virtio_serial_open(&vcon->port);
         break;
-    }
     case CHR_EVENT_CLOSED:
         virtio_serial_close(&vcon->port);
         break;
-- 
1.7.3.4

[Qemu-devel] [PATCH 3/7] virtio-serial-bus: separate out discard logic in a separate function

[Amit Shah]

Instead of combining flush logic into the discard case and not discard
case, have one function doing discard case.  This will help later when
adding flow control logic to the do_flush_queued_data() function.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |   47 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 74ba5ec..e8c2a16 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -113,39 +113,48 @@ static size_t write_to_port(VirtIOSerialPort *port,
     return offset;
 }
 
+static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
+{
+    VirtQueueElement elem;
+
+    while (virtqueue_pop(vq, &elem)) {
+        virtqueue_push(vq, &elem, 0);
+    }
+    virtio_notify(vdev, vq);
+}
+
 static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
-                                 VirtIODevice *vdev, bool discard)
+                                 VirtIODevice *vdev)
 {
     VirtQueueElement elem;
 
-    assert(port || discard);
+    assert(port);
     assert(virtio_queue_ready(vq));
 
-    while ((discard || !port->throttled) && virtqueue_pop(vq, &elem)) {
+    while (!port->throttled && virtqueue_pop(vq, &elem)) {
         uint8_t *buf;
         size_t ret, buf_size;
 
-        if (!discard) {
-            buf_size = iov_size(elem.out_sg, elem.out_num);
-            buf = qemu_malloc(buf_size);
-            ret = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, buf_size);
+        buf_size = iov_size(elem.out_sg, elem.out_num);
+        buf = qemu_malloc(buf_size);
+        ret = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, buf_size);
+
+        port->info->have_data(port, buf, ret);
+        qemu_free(buf);
 
-            port->info->have_data(port, buf, ret);
-            qemu_free(buf);
-        }
         virtqueue_push(vq, &elem, 0);
     }
     virtio_notify(vdev, vq);
 }
 
-static void flush_queued_data(VirtIOSerialPort *port, bool discard)
+static void flush_queued_data(VirtIOSerialPort *port)
 {
     assert(port);
 
     if (!virtio_queue_ready(port->ovq)) {
         return;
     }
-    do_flush_queued_data(port, port->ovq, &port->vser->vdev, discard);
+    do_flush_queued_data(port, port->ovq, &port->vser->vdev);
 }
 
 static size_t send_control_msg(VirtIOSerialPort *port, void *buf, size_t len)
@@ -204,7 +213,7 @@ int virtio_serial_close(VirtIOSerialPort *port)
      * consume, reset the throttling flag and discard the data.
      */
     port->throttled = false;
-    flush_queued_data(port, true);
+    discard_vq_data(port->ovq, &port->vser->vdev);
 
     send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
 
@@ -258,7 +267,7 @@ void virtio_serial_throttle_port(VirtIOSerialPort *port, bool throttle)
         return;
     }
 
-    flush_queued_data(port, false);
+    flush_queued_data(port);
 }
 
 /* Guest wants to notify us of some event */
@@ -414,11 +423,15 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
         discard = true;
     }
 
-    if (!discard && port->throttled) {
+    if (discard) {
+        discard_vq_data(vq, vdev);
+        return;
+    }
+    if (port->throttled) {
         return;
     }
 
-    do_flush_queued_data(port, vq, vdev, discard);
+    do_flush_queued_data(port, vq, vdev);
 }
 
 static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
@@ -634,7 +647,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
 
     port = find_port_by_id(vser, port_id);
     /* Flush out any unconsumed buffers first */
-    flush_queued_data(port, true);
+    discard_vq_data(port->ovq, &port->vser->vdev);
 
     send_control_event(port, VIRTIO_CONSOLE_PORT_REMOVE, 1);
 }
-- 
1.7.3.4

[Qemu-devel] [PATCH 4/7] virtio-serial: Don't copy over guest buffer to host

[Amit Shah]

When the guest writes something to a host, we copied over the entire
buffer first into the host and then processed it.  Do away with that, it
could result in a malicious guest causing a DoS on the host.

Reported-by: Paul Brook <paul@codesourcery.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index e8c2a16..6726f72 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -132,16 +132,17 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
     assert(virtio_queue_ready(vq));
 
     while (!port->throttled && virtqueue_pop(vq, &elem)) {
-        uint8_t *buf;
-        size_t ret, buf_size;
+        unsigned int i;
 
-        buf_size = iov_size(elem.out_sg, elem.out_num);
-        buf = qemu_malloc(buf_size);
-        ret = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, buf_size);
+        for (i = 0; i < elem.out_num; i++) {
+            size_t buf_size;
 
-        port->info->have_data(port, buf, ret);
-        qemu_free(buf);
+            buf_size = elem.out_sg[i].iov_len;
 
+            port->info->have_data(port,
+                                  elem.out_sg[i].iov_base,
+                                  buf_size);
+        }
         virtqueue_push(vq, &elem, 0);
     }
     virtio_notify(vdev, vq);
-- 
1.7.3.4

[Qemu-devel] [PATCH 5/7] virtio-serial: Let virtio-serial-bus know if all data was consumed

[Amit Shah]

The have_data() API to hand off guest data to apps using virtio-serial
so far assumed all the data was consumed.  Relax this assumption.
Future commits will allow for incomplete writes.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c |    4 ++--
 hw/virtio-serial.h  |    7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index d0b9354..62624ec 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -20,11 +20,11 @@ typedef struct VirtConsole {
 
 
 /* Callback function that's called when the guest sends us data */
-static void flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
+static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
 {
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
-    qemu_chr_write(vcon->chr, buf, len);
+    return qemu_chr_write(vcon->chr, buf, len);
 }
 
 /* Readiness of the guest to accept data on a port */
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index ff08c40..9cc0fb3 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -137,10 +137,11 @@ struct VirtIOSerialPortInfo {
 
     /*
      * Guest wrote some data to the port. This data is handed over to
-     * the app via this callback.  The app is supposed to consume all
-     * the data that is presented to it.
+     * the app via this callback.  The app can return a size less than
+     * 'len'.  In this case, throttling will be enabled for this port.
      */
-    void (*have_data)(VirtIOSerialPort *port, const uint8_t *buf, size_t len);
+    ssize_t (*have_data)(VirtIOSerialPort *port, const uint8_t *buf,
+                         size_t len);
 };
 
 /* Interface to the virtio-serial bus */
-- 
1.7.3.4

[Qemu-devel] [PATCH 6/7] virtio-serial: Add support for flow control

[Amit Shah]

This commit lets apps signal an incomplete write.  When that happens,
stop sending out any more data to the app and wait for it to unthrottle
the port.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |   49 +++++++++++++++++++++++++++++++++++++----------
 hw/virtio-serial.h     |   17 ++++++++++++++++
 2 files changed, 55 insertions(+), 11 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 6726f72..32938b5 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -126,24 +126,49 @@ static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
 static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
                                  VirtIODevice *vdev)
 {
-    VirtQueueElement elem;
-
     assert(port);
     assert(virtio_queue_ready(vq));
 
-    while (!port->throttled && virtqueue_pop(vq, &elem)) {
+    while (!port->throttled) {
         unsigned int i;
 
-        for (i = 0; i < elem.out_num; i++) {
-            size_t buf_size;
-
-            buf_size = elem.out_sg[i].iov_len;
+        /* Pop an elem only if we haven't left off a previous one mid-way */
+        if (!port->elem.out_num) {
+            if (!virtqueue_pop(vq, &port->elem)) {
+                break;
+            }
+            port->iov_idx = 0;
+            port->iov_offset = 0;
+        }
 
-            port->info->have_data(port,
-                                  elem.out_sg[i].iov_base,
-                                  buf_size);
+        for (i = port->iov_idx; i < port->elem.out_num; i++) {
+            size_t buf_size;
+            ssize_t ret;
+
+            buf_size = port->elem.out_sg[i].iov_len - port->iov_offset;
+            ret = port->info->have_data(port,
+                                        port->elem.out_sg[i].iov_base
+                                          + port->iov_offset,
+                                        buf_size);
+            if (ret < 0 && ret != -EAGAIN) {
+                /* We don't handle any other type of errors here */
+                abort();
+            }
+            if (ret == -EAGAIN || (ret >= 0 && ret < buf_size)) {
+                virtio_serial_throttle_port(port, true);
+                port->iov_idx = i;
+                if (ret > 0) {
+                    port->iov_offset += ret;
+                }
+                break;
+            }
+            port->iov_offset = 0;
         }
-        virtqueue_push(vq, &elem, 0);
+        if (port->throttled) {
+            break;
+        }
+        virtqueue_push(vq, &port->elem, 0);
+        port->elem.out_num = 0;
     }
     virtio_notify(vdev, vq);
 }
@@ -709,6 +734,8 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
         port->guest_connected = true;
     }
 
+    port->elem.out_num = 0;
+
     QTAILQ_INSERT_TAIL(&port->vser->ports, port, next);
     port->ivq = port->vser->ivqs[port->id];
     port->ovq = port->vser->ovqs[port->id];
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index 9cc0fb3..a308196 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -102,6 +102,23 @@ struct VirtIOSerialPort {
      */
     uint32_t id;
 
+    /*
+     * This is the elem that we pop from the virtqueue.  A slow
+     * backend that consumes guest data (e.g. the file backend for
+     * qemu chardevs) can cause the guest to block till all the output
+     * is flushed.  This isn't desired, so we keep a note of the last
+     * element popped and continue consuming it once the backend
+     * becomes writable again.
+     */
+    VirtQueueElement elem;
+
+    /*
+     * The index and the offset into the iov buffer that was popped in
+     * elem above.
+     */
+    uint32_t iov_idx;
+    uint64_t iov_offset;
+
     /* Identify if this is a port that binds with hvc in the guest */
     uint8_t is_console;
 
-- 
1.7.3.4

[Qemu-devel] [PATCH 7/7] virtio-serial: save/restore new fields in port struct

[Amit Shah]

The new fields that got added as part of not copying over the guest
buffer to the host need to be saved/restored across migration.  Do that
and bump up the version number.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |   42 ++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 32938b5..2ca97a9 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -527,9 +527,24 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
      * Items in struct VirtIOSerialPort.
      */
     QTAILQ_FOREACH(port, &s->ports, next) {
+        uint32_t elem_popped;
+
         qemu_put_be32s(f, &port->id);
         qemu_put_byte(f, port->guest_connected);
         qemu_put_byte(f, port->host_connected);
+
+	elem_popped = 0;
+        if (port->elem.out_num) {
+            elem_popped = 1;
+        }
+        qemu_put_be32s(f, &elem_popped);
+        if (elem_popped) {
+            qemu_put_be32s(f, &port->iov_idx);
+            qemu_put_be64s(f, &port->iov_offset);
+
+            qemu_put_buffer(f, (unsigned char *)&port->elem,
+                            sizeof(port->elem));
+        }
     }
 }
 
@@ -540,7 +555,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     uint32_t max_nr_ports, nr_active_ports, ports_map;
     unsigned int i;
 
-    if (version_id > 2) {
+    if (version_id > 3) {
         return -EINVAL;
     }
 
@@ -593,6 +608,29 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
             send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
                                port->host_connected);
         }
+
+        if (version_id > 2) {
+            uint32_t elem_popped;
+
+            qemu_get_be32s(f, &elem_popped);
+            if (elem_popped) {
+                qemu_get_be32s(f, &port->iov_idx);
+                qemu_get_be64s(f, &port->iov_offset);
+
+                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);
+
+                /*
+                 *  Port was throttled on source machine.  Let's
+                 *  unthrottle it here so data starts flowing again.
+                 */
+                virtio_serial_throttle_port(port, false);
+            }
+        }
     }
     return 0;
 }
@@ -841,7 +879,7 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports)
      * Register for the savevm section with the virtio-console name
      * to preserve backward compat
      */
-    register_savevm(dev, "virtio-console", -1, 2, virtio_serial_save,
+    register_savevm(dev, "virtio-console", -1, 3, virtio_serial_save,
                     virtio_serial_load, vser);
 
     return vdev;
-- 
1.7.3.4