virtio-serial: Don't copy guest buf to host, flow control

PATCH New Amit Shah 1 participant 8 messages Jan 19, 2011

lore

Share & tools

lore

Amit Shah

author

2011-01-19 12:27 UTC #0 lore raw

cover virtio-serial: Don't copy guest buf to host, flow control

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(-)

Signature · 1 line

1.7.3.4

Amit Shah

author

2011-01-19 12:27 UTC #1 lore raw

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

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 <[email protected]>

---
 hw/virtio-console.c |   31 ++++++++++++++-----------------
 1 files changed, 14 insertions(+), 17 deletions(-)

hw/virtio-console.c +14 ~~-17~~

index caea11f..d7fe68b 100644
@@ -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 = {

Signature · 1 line

1.7.3.4

Amit Shah

author

2011-01-19 12:27 UTC #2 lore raw

PATCH 2/7 virtio-console: Remove unnecessary braces

In reply to Amit Shah

Remove unnecessary braces around a case statement.

Signed-off-by Amit Shah <[email protected]>

---
 hw/virtio-console.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

hw/virtio-console.c +1 -2

index d7fe68b..d0b9354 100644
@@ -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;

Signature · 1 line

1.7.3.4

Amit Shah

author

2011-01-19 12:27 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 hw/virtio-serial-bus.c |   47 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 30 insertions(+), 17 deletions(-)

hw/virtio-serial-bus.c +30 ~~-17~~

index 74ba5ec..e8c2a16 100644
@@ -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);
 }

Signature · 1 line

1.7.3.4

Amit Shah

author

2011-01-19 12:27 UTC #4 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Amit Shah <[email protected]>

---
 hw/virtio-serial-bus.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

hw/virtio-serial-bus.c +8 -7

index e8c2a16..6726f72 100644
@@ -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);

Signature · 1 line

1.7.3.4

Amit Shah

author

2011-01-19 12:27 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 hw/virtio-console.c |    4 ++--
 hw/virtio-serial.h  |    7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

hw/virtio-console.c +2 -2

index d0b9354..62624ec 100644
@@ -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 */

hw/virtio-serial.h +4 -3

index ff08c40..9cc0fb3 100644
@@ -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 */

Signature · 1 line

1.7.3.4

Amit Shah

author

2011-01-19 12:27 UTC #6 lore raw

PATCH 6/7 virtio-serial: Add support for flow control

In reply to 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 <[email protected]>

---
 hw/virtio-serial-bus.c |   49 +++++++++++++++++++++++++++++++++++++----------
 hw/virtio-serial.h     |   17 ++++++++++++++++
 2 files changed, 55 insertions(+), 11 deletions(-)

hw/virtio-serial-bus.c +38 ~~-11~~

index 6726f72..32938b5 100644
@@ -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];

hw/virtio-serial.h +17 -0

index 9cc0fb3..a308196 100644
@@ -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;

Signature · 1 line

1.7.3.4

Amit Shah

author

2011-01-19 12:27 UTC #7 lore raw

PATCH 7/7 virtio-serial: save/restore new fields in port struct

In reply to 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 <[email protected]>

---
 hw/virtio-serial-bus.c |   42 ++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 40 insertions(+), 2 deletions(-)

hw/virtio-serial-bus.c +40 -2

index 32938b5..2ca97a9 100644
@@ -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;

Signature · 1 line

1.7.3.4