[Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Amit Shah]

Guests send us one buffer at a time. Current guests send buffers sized
4K bytes. If guest userspace applications sent out > 4K bytes in one
write() syscall, the write request actually sends out multiple buffers,
each of 4K in size.

This usually isn't a problem but for some apps, like VNC, the entire
data has to be sent in one go to make copy/paste work fine. So if an app
on the guest sends out guest clipboard contents, it has to be sent to
the vnc server in one go as the guest app sent it.

For this to be done, we need the guest to send us START and END markers
for each write request so that we can find out complete buffers and send
them off to ports.

This needs us to buffer all the data that comes in from the guests, hold
it off till we see all the data corresponding to one write request,
merge it all in one buffer and then send it to the port the data was
destined for.

Also, we add support for caching of these buffers till a port indicates
it's ready to receive data.

We keep caching data the guest sends us till a port accepts it. However,
this could lead to an OOM condition where a rogue process on the guest
could continue pumping in data while the host continues to cache it.

We introduce a per-port byte-limit property to alleviate this condition.
When this limit is reached, we send a control message to the guest
indicating it to not send us any more data till further indication. When
the number of bytes cached go lesser than the limit specified, we open
tell the guest to restart sending data.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |  305 +++++++++++++++++++++++++++++++++++++++++++++++-
 hw/virtio-serial.c     |   11 +-
 hw/virtio-serial.h     |   39 ++++++
 3 files changed, 348 insertions(+), 7 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 0a953cd..bc47629 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -46,6 +46,20 @@ struct VirtIOSerial {
     uint32_t guest_features;
 };
 
+/* This struct holds individual buffers received for each port */
+typedef struct VirtIOSerialPortBuffer {
+    QTAILQ_ENTRY(VirtIOSerialPortBuffer) next;
+
+    uint8_t *buf;
+
+    size_t len; /* length of the buffer */
+    size_t offset; /* offset from which to consume data in the buffer */
+
+    uint32_t flags; /* Sent by guest (start of data stream, end of stream) */
+
+    bool previous_failure; /* Did sending out this buffer fail previously? */
+} VirtIOSerialPortBuffer;
+
 static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
 {
     VirtIOSerialPort *port;
@@ -159,6 +173,197 @@ static size_t send_control_event(VirtIOSerialPort *port, uint16_t event,
     return send_control_msg(port, &cpkt, sizeof(cpkt));
 }
 
+static void init_buf(VirtIOSerialPortBuffer *buf, uint8_t *buffer, size_t len)
+{
+    buf->buf = buffer;
+    buf->len = len;
+    buf->offset = 0;
+    buf->flags = 0;
+    buf->previous_failure = false;
+}
+
+static VirtIOSerialPortBuffer *alloc_buf(size_t len)
+{
+    VirtIOSerialPortBuffer *buf;
+
+    buf = qemu_malloc(sizeof(*buf));
+    buf->buf = qemu_malloc(len);
+
+    init_buf(buf, buf->buf, len);
+
+    return buf;
+}
+
+static void free_buf(VirtIOSerialPortBuffer *buf)
+{
+    qemu_free(buf->buf);
+    qemu_free(buf);
+}
+
+static size_t get_complete_data_size(VirtIOSerialPort *port)
+{
+    VirtIOSerialPortBuffer *buf;
+    size_t size;
+    bool is_complete, start_seen;
+
+    size = 0;
+    is_complete = false;
+    start_seen = false;
+    QTAILQ_FOREACH(buf, &port->unflushed_buffers, next) {
+        size += buf->len - buf->offset;
+
+        if (buf->flags & VIRTIO_CONSOLE_HDR_END_DATA) {
+            is_complete = true;
+            break;
+        }
+        if (buf == QTAILQ_FIRST(&port->unflushed_buffers)
+            && !(buf->flags & VIRTIO_CONSOLE_HDR_START_DATA)) {
+
+            /* There's some data that arrived without a START flag. Flush it. */
+            is_complete = true;
+            break;
+        }
+
+        if (buf->flags & VIRTIO_CONSOLE_HDR_START_DATA) {
+            if (start_seen) {
+                /*
+                 * There's some data that arrived without an END
+                 * flag. Flush it.
+                 */
+                size -= buf->len + buf->offset;
+                is_complete = true;
+                break;
+            }
+            start_seen = true;
+        }
+    }
+    return is_complete ? size : 0;
+}
+
+/*
+ * The guest could have sent the data corresponding to one write
+ * request split up in multiple buffers. The first buffer has the
+ * VIRTIO_CONSOLE_HDR_START_DATA flag set and the last buffer has the
+ * VIRTIO_CONSOLE_HDR_END_DATA flag set. Using this information, merge
+ * the parts into one buf here to process it for output.
+ */
+static VirtIOSerialPortBuffer *get_complete_buf(VirtIOSerialPort *port)
+{
+    VirtIOSerialPortBuffer *buf, *buf2;
+    uint8_t *outbuf;
+    size_t out_offset, out_size;
+
+    out_size = get_complete_data_size(port);
+    if (!out_size)
+        return NULL;
+
+    buf = QTAILQ_FIRST(&port->unflushed_buffers);
+    if (buf->len - buf->offset == out_size) {
+        QTAILQ_REMOVE(&port->unflushed_buffers, buf, next);
+        return buf;
+    }
+    out_offset = 0;
+    outbuf = qemu_malloc(out_size);
+
+    QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffers, next, buf2) {
+        size_t copy_size;
+
+        copy_size = buf->len - buf->offset;
+        memcpy(outbuf + out_offset, buf->buf + buf->offset, copy_size);
+        out_offset += copy_size;
+
+        QTAILQ_REMOVE(&port->unflushed_buffers, buf, next);
+        qemu_free(buf->buf);
+
+        if (out_offset == out_size) {
+            break;
+        }
+        qemu_free(buf);
+    }
+    init_buf(buf, outbuf, out_size);
+    buf->flags = VIRTIO_CONSOLE_HDR_START_DATA | VIRTIO_CONSOLE_HDR_END_DATA;
+
+    return buf;
+}
+
+/* Call with the unflushed_buffers_lock held */
+static void flush_queue(VirtIOSerialPort *port)
+{
+    VirtIOSerialPortBuffer *buf;
+    size_t out_size;
+    ssize_t ret;
+
+    /*
+     * If a device is interested in buffering packets till it's
+     * opened, cache the data the guest sends us till a connection is
+     * established.
+     */
+    if (!port->host_connected && port->cache_buffers) {
+        return;
+    }
+    while ((buf = get_complete_buf(port))) {
+        out_size = buf->len - buf->offset;
+        if (!port->host_connected) {
+            /*
+             * Caching is disabled and host is not connected, so
+             * discard the buffer. Do this only after merging the
+             * buffer as a port can get connected in the middle of
+             * dropping buffers and the port will end up getting the
+             * incomplete output.
+             */
+            port->nr_bytes -= buf->len + buf->offset;
+            free_buf(buf);
+            continue;
+        }
+
+        ret = port->info->have_data(port, buf->buf + buf->offset, out_size);
+        if (ret < out_size) {
+            QTAILQ_INSERT_HEAD(&port->unflushed_buffers, buf, next);
+        }
+        if (ret <= 0) {
+            /* We're not progressing at all */
+            if (buf->previous_failure) {
+                break;
+            }
+            buf->previous_failure = true;
+        } else {
+            buf->offset += ret;
+            port->nr_bytes -= ret;
+            buf->previous_failure = false;
+        }
+        if (!(buf->len - buf->offset)) {
+            free_buf(buf);
+        }
+    }
+
+    if (port->host_throttled && port->nr_bytes < port->byte_limit) {
+        port->host_throttled = false;
+        send_control_event(port, VIRTIO_CONSOLE_THROTTLE_PORT, 0);
+    }
+}
+
+static void flush_all_ports(VirtIOSerial *vser)
+{
+    struct VirtIOSerialPort *port;
+
+    QTAILQ_FOREACH(port, &vser->ports, next) {
+        if (port->has_activity) {
+            port->has_activity = false;
+            flush_queue(port);
+        }
+    }
+}
+
+static void remove_port_buffers(VirtIOSerialPort *port)
+{
+    struct VirtIOSerialPortBuffer *buf, *buf2;
+
+    QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffers, next, buf2) {
+        QTAILQ_REMOVE(&port->unflushed_buffers, buf, next);
+        free_buf(buf);
+    }
+}
+
 /* Functions for use inside qemu to open and read from/write to ports */
 int virtio_serial_open(VirtIOSerialPort *port)
 {
@@ -170,6 +375,10 @@ int virtio_serial_open(VirtIOSerialPort *port)
     port->host_connected = true;
     send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
 
+    /* Flush any buffers that were cached while the port was closed */
+    if (port->cache_buffers && port->info->have_data) {
+        flush_queue(port);
+    }
     return 0;
 }
 
@@ -178,6 +387,9 @@ int virtio_serial_close(VirtIOSerialPort *port)
     port->host_connected = false;
     send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
 
+    if (!port->cache_buffers) {
+        remove_port_buffers(port);
+    }
     return 0;
 }
 
@@ -265,6 +477,13 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
             send_control_msg(port, buffer, buffer_len);
             qemu_free(buffer);
         }
+        /*
+         * We also want to signal to the guest whether or not the port
+         * is set to caching the buffers when disconnected.
+         */
+        if (port->cache_buffers) {
+            send_control_event(port, VIRTIO_CONSOLE_CACHE_BUFFERS, 1);
+        }
         if (port->host_connected) {
             send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
         }
@@ -312,6 +531,10 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
 
 /*
  * Guest wrote something to some port.
+ *
+ * Flush the data in the entire chunk that we received rather than
+ * splitting it into multiple buffers. VNC clients don't consume split
+ * buffers
  */
 static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
@@ -322,6 +545,7 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
     while (virtqueue_pop(vq, &elem)) {
         VirtIOSerialPort *port;
+        VirtIOSerialPortBuffer *buf;
         struct virtio_console_header header;
         int header_len;
 
@@ -330,6 +554,9 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
         if (elem.out_sg[0].iov_len < header_len) {
             goto next_buf;
         }
+        if (header_len) {
+            memcpy(&header, elem.out_sg[0].iov_base, header_len);
+        }
         port = find_port_by_vq(vser, vq);
         if (!port) {
             goto next_buf;
@@ -344,13 +571,38 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
         }
 
         /* The guest always sends only one sg */
-        port->info->have_data(port, elem.out_sg[0].iov_base + header_len,
-                              elem.out_sg[0].iov_len - header_len);
+        buf = alloc_buf(elem.out_sg[0].iov_len - header_len);
+        memcpy(buf->buf, elem.out_sg[0].iov_base + header_len, buf->len);
+
+        if (header_len) {
+            /*
+             * Only the first buffer in a stream will have this
+             * set. This will help us identify the first buffer and
+             * the remaining buffers in the stream based on length
+             */
+            buf->flags = le32_to_cpu(header.flags)
+                & (VIRTIO_CONSOLE_HDR_START_DATA | VIRTIO_CONSOLE_HDR_END_DATA);
+        } else {
+            /* We always want to flush all the buffers in this case */
+            buf->flags = VIRTIO_CONSOLE_HDR_START_DATA
+                | VIRTIO_CONSOLE_HDR_END_DATA;
+        }
 
+        QTAILQ_INSERT_TAIL(&port->unflushed_buffers, buf, next);
+        port->nr_bytes += buf->len;
+        port->has_activity = true;
+
+        if (!port->host_throttled && port->byte_limit &&
+            port->nr_bytes >= port->byte_limit) {
+
+            port->host_throttled = true;
+            send_control_event(port, VIRTIO_CONSOLE_THROTTLE_PORT, 1);
+        }
     next_buf:
         virtqueue_push(vq, &elem, elem.out_sg[0].iov_len);
     }
     virtio_notify(vdev, vq);
+    flush_all_ports(vser);
 }
 
 static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
@@ -391,6 +643,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
     VirtIOSerial *s = opaque;
     VirtIOSerialPort *port;
     uint32_t nr_active_ports;
+    unsigned int nr_bufs;
 
     /* The virtio device */
     virtio_save(&s->vdev, f);
@@ -413,14 +666,34 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
      * Items in struct VirtIOSerialPort.
      */
     QTAILQ_FOREACH(port, &s->ports, next) {
+        VirtIOSerialPortBuffer *buf;
+
         /*
          * We put the port number because we may not have an active
          * port at id 0 that's reserved for a console port, or in case
          * of ports that might have gotten unplugged
          */
         qemu_put_be32s(f, &port->id);
+        qemu_put_be64s(f, &port->byte_limit);
+        qemu_put_be64s(f, &port->nr_bytes);
         qemu_put_byte(f, port->guest_connected);
+        qemu_put_byte(f, port->host_throttled);
 
+        /* All the pending buffers from active ports */
+        nr_bufs = 0;
+        QTAILQ_FOREACH(buf, &port->unflushed_buffers, next) {
+            nr_bufs++;
+        }
+        qemu_put_be32s(f, &nr_bufs);
+        if (!nr_bufs) {
+            continue;
+        }
+        QTAILQ_FOREACH(buf, &port->unflushed_buffers, next) {
+            qemu_put_be64s(f, &buf->len);
+            qemu_put_be64s(f, &buf->offset);
+            qemu_put_be32s(f, &buf->flags);
+            qemu_put_buffer(f, buf->buf, buf->len);
+        }
     }
 }
 
@@ -452,13 +725,34 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 
     /* Items in struct VirtIOSerialPort */
     for (i = 0; i < nr_active_ports; i++) {
+        VirtIOSerialPortBuffer *buf;
         uint32_t id;
+        unsigned int nr_bufs;
 
         id = qemu_get_be32(f);
         port = find_port_by_id(s, id);
 
+        port->byte_limit = qemu_get_be64(f);
+        port->nr_bytes   = qemu_get_be64(f);
         port->guest_connected = qemu_get_byte(f);
+        port->host_throttled = qemu_get_byte(f);
+
+        /* All the pending buffers from active ports */
+        qemu_get_be32s(f, &nr_bufs);
+        if (!nr_bufs) {
+            continue;
+        }
+        for (; nr_bufs; nr_bufs--) {
+            size_t len;
 
+            qemu_get_be64s(f, &len);
+            buf = alloc_buf(len);
+
+            qemu_get_be64s(f, &buf->offset);
+            qemu_get_be32s(f, &buf->flags);
+            qemu_get_buffer(f, buf->buf, buf->len);
+            QTAILQ_INSERT_TAIL(&port->unflushed_buffers, buf, next);
+        }
     }
     return 0;
 }
@@ -495,6 +789,10 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
                    indent, "", port->guest_connected);
     monitor_printf(mon, "%*s dev-prop-int: host_connected: %d\n",
                    indent, "", port->host_connected);
+    monitor_printf(mon, "%*s dev-prop-int: host_throttled: %d\n",
+                   indent, "", port->host_throttled);
+    monitor_printf(mon, "%*s dev-prop-int: nr_bytes: %zu\n",
+                   indent, "", port->nr_bytes);
 }
 
 static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
@@ -525,6 +823,7 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
     if (ret) {
         return ret;
     }
+    QTAILQ_INIT(&port->unflushed_buffers);
 
     port->id = plugging_port0 ? 0 : port->vser->config.nr_ports++;
 
@@ -567,6 +866,8 @@ static int virtser_port_qdev_exit(DeviceState *qdev)
     if (port->info->exit)
         port->info->exit(dev);
 
+    remove_port_buffers(port);
+
     return 0;
 }
 
diff --git a/hw/virtio-serial.c b/hw/virtio-serial.c
index 470446b..fd27c33 100644
--- a/hw/virtio-serial.c
+++ b/hw/virtio-serial.c
@@ -66,13 +66,14 @@ static int virtconsole_initfn(VirtIOSerialDevice *dev)
 
     port->info = dev->info;
 
-    port->is_console = true;
-
     /*
-     * For console ports, just assume the guest is ready to accept our
-     * data.
+     * We're not interested in data the guest sends while nothing is
+     * connected on the host side. Just ignore it instead of saving it
+     * for later consumption.
      */
-    port->guest_connected = true;
+    port->cache_buffers = 0;
+
+    port->is_console = true;
 
     if (vcon->chr) {
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index 5505841..acb601d 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -49,12 +49,18 @@ struct virtio_console_header {
     uint32_t flags;		/* Some message between host and guest */
 };
 
+/* Messages between host and guest */
+#define VIRTIO_CONSOLE_HDR_START_DATA	(1 << 0)
+#define VIRTIO_CONSOLE_HDR_END_DATA	(1 << 1)
+
 /* Some events for the internal messages (control packets) */
 #define VIRTIO_CONSOLE_PORT_READY	0
 #define VIRTIO_CONSOLE_CONSOLE_PORT	1
 #define VIRTIO_CONSOLE_RESIZE		2
 #define VIRTIO_CONSOLE_PORT_OPEN	3
 #define VIRTIO_CONSOLE_PORT_NAME	4
+#define VIRTIO_CONSOLE_THROTTLE_PORT	5
+#define VIRTIO_CONSOLE_CACHE_BUFFERS	6
 
 /* == In-qemu interface == */
 
@@ -96,6 +102,13 @@ struct VirtIOSerialPort {
     char *name;
 
     /*
+     * This list holds buffers pushed by the guest in case the guest
+     * sent incomplete messages or the host connection was down and
+     * the device requested to cache the data.
+     */
+    QTAILQ_HEAD(, VirtIOSerialPortBuffer) unflushed_buffers;
+
+    /*
      * This id helps identify ports between the guest and the host.
      * The guest sends a "header" with this id with each data packet
      * that it sends and the host can then find out which associated
@@ -103,6 +116,27 @@ struct VirtIOSerialPort {
      */
     uint32_t id;
 
+    /*
+     * Each port can specify the limit on number of bytes that can be
+     * outstanding in the unread buffers. This is to prevent any OOM
+     * situtation if a rogue process on the guest keeps injecting
+     * data.
+     */
+    size_t byte_limit;
+
+    /*
+     * The number of bytes we have queued up in our unread queue
+     */
+    size_t nr_bytes;
+
+    /*
+     * This boolean, when set, means "queue data that gets sent to
+     * this port when the host is not connected". The queued data, if
+     * any, is then sent out to the port when the host connection is
+     * opened.
+     */
+    uint8_t cache_buffers;
+
     /* Identify if this is a port that binds with hvc in the guest */
     uint8_t is_console;
 
@@ -110,6 +144,11 @@ struct VirtIOSerialPort {
     bool guest_connected;
     /* Is this device open for IO on the host? */
     bool host_connected;
+    /* Have we sent a throttle message to the guest? */
+    bool host_throttled;
+
+    /* Did this port get data in the recent handle_output call? */
+    bool has_activity;
 };
 
 struct VirtIOSerialPortInfo {
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Amit Shah]

Guests send us one buffer at a time. Current guests send buffers sized
4K bytes. If guest userspace applications sent out > 4K bytes in one
write() syscall, the write request actually sends out multiple buffers,
each of 4K in size.

This usually isn't a problem but for some apps, like VNC, the entire
data has to be sent in one go to make copy/paste work fine. So if an app
on the guest sends out guest clipboard contents, it has to be sent to
the vnc server in one go as the guest app sent it.

For this to be done, we need the guest to send us START and END markers
for each write request so that we can find out complete buffers and send
them off to ports.

This needs us to buffer all the data that comes in from the guests, hold
it off till we see all the data corresponding to one write request,
merge it all in one buffer and then send it to the port the data was
destined for.

Also, we add support for caching of these buffers till a port indicates
it's ready to receive data.

We keep caching data the guest sends us till a port accepts it. However,
this could lead to an OOM condition where a rogue process on the guest
could continue pumping in data while the host continues to cache it.

We introduce a per-port byte-limit property to alleviate this condition.
When this limit is reached, we send a control message to the guest
indicating it to not send us any more data till further indication. When
the number of bytes cached go lesser than the limit specified, we open
tell the guest to restart sending data.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |  309 +++++++++++++++++++++++++++++++++++++++++++++++-
 hw/virtio-serial.c     |   11 +-
 hw/virtio-serial.h     |   39 ++++++
 3 files changed, 352 insertions(+), 7 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 20d9580..c947143 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -44,6 +44,20 @@ struct VirtIOSerial {
     struct virtio_console_config config;
 };
 
+/* This struct holds individual buffers received for each port */
+typedef struct VirtIOSerialPortBuffer {
+    QTAILQ_ENTRY(VirtIOSerialPortBuffer) next;
+
+    uint8_t *buf;
+
+    size_t len; /* length of the buffer */
+    size_t offset; /* offset from which to consume data in the buffer */
+
+    uint32_t flags; /* Sent by guest (start of data stream, end of stream) */
+
+    bool previous_failure; /* Did sending out this buffer fail previously? */
+} VirtIOSerialPortBuffer;
+
 static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
 {
     VirtIOSerialPort *port;
@@ -157,6 +171,198 @@ static size_t send_control_event(VirtIOSerialPort *port, uint16_t event,
     return send_control_msg(port, &cpkt, sizeof(cpkt));
 }
 
+static void init_buf(VirtIOSerialPortBuffer *buf, uint8_t *buffer, size_t len)
+{
+    buf->buf = buffer;
+    buf->len = len;
+    buf->offset = 0;
+    buf->flags = 0;
+    buf->previous_failure = false;
+}
+
+static VirtIOSerialPortBuffer *alloc_buf(size_t len)
+{
+    VirtIOSerialPortBuffer *buf;
+
+    buf = qemu_malloc(sizeof(*buf));
+    buf->buf = qemu_malloc(len);
+
+    init_buf(buf, buf->buf, len);
+
+    return buf;
+}
+
+static void free_buf(VirtIOSerialPortBuffer *buf)
+{
+    qemu_free(buf->buf);
+    qemu_free(buf);
+}
+
+static size_t get_complete_data_size(VirtIOSerialPort *port)
+{
+    VirtIOSerialPortBuffer *buf;
+    size_t size;
+    bool is_complete, start_seen;
+
+    size = 0;
+    is_complete = false;
+    start_seen = false;
+    QTAILQ_FOREACH(buf, &port->unflushed_buffers, next) {
+        size += buf->len - buf->offset;
+
+        if (buf->flags & VIRTIO_CONSOLE_HDR_END_DATA) {
+            is_complete = true;
+            break;
+        }
+        if (buf == QTAILQ_FIRST(&port->unflushed_buffers)
+            && !(buf->flags & VIRTIO_CONSOLE_HDR_START_DATA)) {
+
+            /* There's some data that arrived without a START flag. Flush it. */
+            is_complete = true;
+            break;
+        }
+
+        if (buf->flags & VIRTIO_CONSOLE_HDR_START_DATA) {
+            if (start_seen) {
+                /*
+                 * There's some data that arrived without an END
+                 * flag. Flush it.
+                 */
+                size -= buf->len + buf->offset;
+                is_complete = true;
+                break;
+            }
+            start_seen = true;
+        }
+    }
+    return is_complete ? size : 0;
+}
+
+/*
+ * The guest could have sent the data corresponding to one write
+ * request split up in multiple buffers. The first buffer has the
+ * VIRTIO_CONSOLE_HDR_START_DATA flag set and the last buffer has the
+ * VIRTIO_CONSOLE_HDR_END_DATA flag set. Using this information, merge
+ * the parts into one buf here to process it for output.
+ */
+static VirtIOSerialPortBuffer *get_complete_buf(VirtIOSerialPort *port)
+{
+    VirtIOSerialPortBuffer *buf, *buf2;
+    uint8_t *outbuf;
+    size_t out_offset, out_size;
+
+    out_size = get_complete_data_size(port);
+    if (!out_size)
+        return NULL;
+
+    buf = QTAILQ_FIRST(&port->unflushed_buffers);
+    if (buf->len - buf->offset == out_size) {
+        QTAILQ_REMOVE(&port->unflushed_buffers, buf, next);
+        return buf;
+    }
+    out_offset = 0;
+    outbuf = qemu_malloc(out_size);
+
+    QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffers, next, buf2) {
+        size_t copy_size;
+
+        copy_size = buf->len - buf->offset;
+        memcpy(outbuf + out_offset, buf->buf + buf->offset, copy_size);
+        out_offset += copy_size;
+
+        QTAILQ_REMOVE(&port->unflushed_buffers, buf, next);
+        qemu_free(buf->buf);
+
+        if (out_offset == out_size) {
+            break;
+        }
+        qemu_free(buf);
+    }
+    init_buf(buf, outbuf, out_size);
+    buf->flags = VIRTIO_CONSOLE_HDR_START_DATA | VIRTIO_CONSOLE_HDR_END_DATA;
+
+    return buf;
+}
+
+/* Call with the unflushed_buffers_lock held */
+static void flush_queue(VirtIOSerialPort *port)
+{
+    VirtIOSerialPortBuffer *buf;
+    size_t out_size;
+    ssize_t ret;
+
+    /*
+     * If a device is interested in buffering packets till it's
+     * opened, cache the data the guest sends us till a connection is
+     * established.
+     */
+    if (!port->host_connected && port->cache_buffers) {
+        return;
+    }
+
+    while ((buf = get_complete_buf(port))) {
+        out_size = buf->len - buf->offset;
+        if (!port->host_connected) {
+            /*
+             * Caching is disabled and host is not connected, so
+             * discard the buffer. Do this only after merging the
+             * buffer as a port can get connected in the middle of
+             * dropping buffers and the port will end up getting the
+             * incomplete output.
+             */
+            port->nr_bytes -= buf->len + buf->offset;
+            free_buf(buf);
+            continue;
+        }
+
+        ret = port->info->have_data(port, buf->buf + buf->offset, out_size);
+        if (ret < out_size) {
+            QTAILQ_INSERT_HEAD(&port->unflushed_buffers, buf, next);
+        }
+        if (ret <= 0) {
+            /* We're not progressing at all */
+            if (buf->previous_failure) {
+                break;
+            }
+            buf->previous_failure = true;
+        } else {
+            buf->offset += ret;
+            port->nr_bytes -= ret;
+            buf->previous_failure = false;
+        }
+        if (!(buf->len - buf->offset)) {
+            free_buf(buf);
+        }
+    }
+
+    if (port->host_throttled && port->nr_bytes < port->byte_limit) {
+        port->host_throttled = false;
+        send_control_event(port, VIRTIO_CONSOLE_THROTTLE_PORT, 0);
+    }
+}
+
+static void flush_all_ports(VirtIOSerial *vser)
+{
+    struct VirtIOSerialPort *port;
+
+    QTAILQ_FOREACH(port, &vser->ports, next) {
+        if (port->has_activity) {
+            port->has_activity = false;
+            flush_queue(port);
+        }
+    }
+}
+
+static void remove_port_buffers(VirtIOSerialPort *port)
+{
+    struct VirtIOSerialPortBuffer *buf, *buf2;
+
+    QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffers, next, buf2) {
+        QTAILQ_REMOVE(&port->unflushed_buffers, buf, next);
+        free_buf(buf);
+    }
+}
+
 /* Functions for use inside qemu to open and read from/write to ports */
 int virtio_serial_open(VirtIOSerialPort *port)
 {
@@ -168,6 +374,10 @@ int virtio_serial_open(VirtIOSerialPort *port)
     port->host_connected = true;
     send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
 
+    /* Flush any buffers that were cached while the port was closed */
+    if (port->cache_buffers && port->info->have_data) {
+        flush_queue(port);
+    }
     return 0;
 }
 
@@ -176,6 +386,9 @@ int virtio_serial_close(VirtIOSerialPort *port)
     port->host_connected = false;
     send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
 
+    if (!port->cache_buffers) {
+        remove_port_buffers(port);
+    }
     return 0;
 }
 
@@ -265,6 +478,14 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
             qemu_free(buffer);
         }
 
+        /*
+         * We also want to signal to the guest whether or not the port
+         * is set to caching the buffers when disconnected.
+         */
+        if (port->cache_buffers) {
+            send_control_event(port, VIRTIO_CONSOLE_CACHE_BUFFERS, 1);
+        }
+
         if (port->host_connected) {
             send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
         }
@@ -315,6 +536,10 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
 
 /*
  * Guest wrote something to some port.
+ *
+ * Flush the data in the entire chunk that we received rather than
+ * splitting it into multiple buffers. VNC clients don't consume split
+ * buffers
  */
 static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
@@ -325,6 +550,7 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
     while (virtqueue_pop(vq, &elem)) {
         VirtIOSerialPort *port;
+        VirtIOSerialPortBuffer *buf;
         struct virtio_console_header header;
         int header_len;
 
@@ -333,10 +559,14 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
         if (elem.out_sg[0].iov_len < header_len) {
             goto next_buf;
         }
+        if (header_len) {
+            memcpy(&header, elem.out_sg[0].iov_base, header_len);
+        }
         port = find_port_by_vq(vser, vq);
         if (!port) {
             goto next_buf;
         }
+
         /*
          * A port may not have any handler registered for consuming the
          * data that the guest sends or it may not have a chardev associated
@@ -347,13 +577,38 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
         }
 
         /* The guest always sends only one sg */
-        port->info->have_data(port, elem.out_sg[0].iov_base + header_len,
-                              elem.out_sg[0].iov_len - header_len);
+        buf = alloc_buf(elem.out_sg[0].iov_len - header_len);
+        memcpy(buf->buf, elem.out_sg[0].iov_base + header_len, buf->len);
+
+        if (header_len) {
+            /*
+             * Only the first buffer in a stream will have this
+             * set. This will help us identify the first buffer and
+             * the remaining buffers in the stream based on length
+             */
+            buf->flags = ldl_p(&header.flags)
+                & (VIRTIO_CONSOLE_HDR_START_DATA | VIRTIO_CONSOLE_HDR_END_DATA);
+        } else {
+            /* We always want to flush all the buffers in this case */
+            buf->flags = VIRTIO_CONSOLE_HDR_START_DATA
+                | VIRTIO_CONSOLE_HDR_END_DATA;
+        }
+
+        QTAILQ_INSERT_TAIL(&port->unflushed_buffers, buf, next);
+        port->nr_bytes += buf->len;
+        port->has_activity = true;
 
+        if (!port->host_throttled && port->byte_limit &&
+            port->nr_bytes >= port->byte_limit) {
+
+            port->host_throttled = true;
+            send_control_event(port, VIRTIO_CONSOLE_THROTTLE_PORT, 1);
+        }
     next_buf:
         virtqueue_push(vq, &elem, elem.out_sg[0].iov_len);
     }
     virtio_notify(vdev, vq);
+    flush_all_ports(vser);
 }
 
 static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
@@ -386,6 +641,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
     VirtIOSerial *s = opaque;
     VirtIOSerialPort *port;
     uint32_t nr_active_ports;
+    unsigned int nr_bufs;
 
     /* The virtio device */
     virtio_save(&s->vdev, f);
@@ -408,14 +664,35 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
      * Items in struct VirtIOSerialPort.
      */
     QTAILQ_FOREACH(port, &s->ports, next) {
+        VirtIOSerialPortBuffer *buf;
+
         /*
          * We put the port number because we may not have an active
          * port at id 0 that's reserved for a console port, or in case
          * of ports that might have gotten unplugged
          */
         qemu_put_be32s(f, &port->id);
+        qemu_put_be64s(f, &port->byte_limit);
+        qemu_put_be64s(f, &port->nr_bytes);
         qemu_put_byte(f, port->guest_connected);
+        qemu_put_byte(f, port->host_throttled);
+
+        /* All the pending buffers from active ports */
+        nr_bufs = 0;
+        QTAILQ_FOREACH(buf, &port->unflushed_buffers, next) {
+            nr_bufs++;
+        }
+        qemu_put_be32s(f, &nr_bufs);
+        if (!nr_bufs) {
+            continue;
+        }
 
+        QTAILQ_FOREACH(buf, &port->unflushed_buffers, next) {
+            qemu_put_be64s(f, &buf->len);
+            qemu_put_be64s(f, &buf->offset);
+            qemu_put_be32s(f, &buf->flags);
+            qemu_put_buffer(f, buf->buf, buf->len);
+        }
     }
 }
 
@@ -448,13 +725,34 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 
     /* Items in struct VirtIOSerialPort */
     for (i = 0; i < nr_active_ports; i++) {
+        VirtIOSerialPortBuffer *buf;
         uint32_t id;
+        unsigned int nr_bufs;
 
         id = qemu_get_be32(f);
         port = find_port_by_id(s, id);
 
+        port->byte_limit = qemu_get_be64(f);
+        port->nr_bytes   = qemu_get_be64(f);
         port->guest_connected = qemu_get_byte(f);
+        port->host_throttled = qemu_get_byte(f);
+
+        /* All the pending buffers from active ports */
+        qemu_get_be32s(f, &nr_bufs);
+        if (!nr_bufs) {
+            continue;
+        }
+        for (; nr_bufs; nr_bufs--) {
+            size_t len;
 
+            qemu_get_be64s(f, &len);
+            buf = alloc_buf(len);
+
+            qemu_get_be64s(f, &buf->offset);
+            qemu_get_be32s(f, &buf->flags);
+            qemu_get_buffer(f, buf->buf, buf->len);
+            QTAILQ_INSERT_TAIL(&port->unflushed_buffers, buf, next);
+        }
     }
 
     return 0;
@@ -490,6 +788,10 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
                    indent, "", port->guest_connected);
     monitor_printf(mon, "%*s dev-prop-int: host_connected: %d\n",
                    indent, "", port->host_connected);
+    monitor_printf(mon, "%*s dev-prop-int: host_throttled: %d\n",
+                   indent, "", port->host_throttled);
+    monitor_printf(mon, "%*s dev-prop-int: nr_bytes: %zu\n",
+                   indent, "", port->nr_bytes);
 }
 
 static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
@@ -520,6 +822,7 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
     if (ret) {
         return ret;
     }
+    QTAILQ_INIT(&port->unflushed_buffers);
 
     port->id = plugging_port0 ? 0 : port->vser->config.nr_ports++;
 
@@ -570,6 +873,8 @@ static int virtser_port_qdev_exit(DeviceState *qdev)
     if (port->info->exit)
         port->info->exit(dev);
 
+    remove_port_buffers(port);
+
     return 0;
 }
 
diff --git a/hw/virtio-serial.c b/hw/virtio-serial.c
index 470446b..fd27c33 100644
--- a/hw/virtio-serial.c
+++ b/hw/virtio-serial.c
@@ -66,13 +66,14 @@ static int virtconsole_initfn(VirtIOSerialDevice *dev)
 
     port->info = dev->info;
 
-    port->is_console = true;
-
     /*
-     * For console ports, just assume the guest is ready to accept our
-     * data.
+     * We're not interested in data the guest sends while nothing is
+     * connected on the host side. Just ignore it instead of saving it
+     * for later consumption.
      */
-    port->guest_connected = true;
+    port->cache_buffers = 0;
+
+    port->is_console = true;
 
     if (vcon->chr) {
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index 5505841..acb601d 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -49,12 +49,18 @@ struct virtio_console_header {
     uint32_t flags;		/* Some message between host and guest */
 };
 
+/* Messages between host and guest */
+#define VIRTIO_CONSOLE_HDR_START_DATA	(1 << 0)
+#define VIRTIO_CONSOLE_HDR_END_DATA	(1 << 1)
+
 /* Some events for the internal messages (control packets) */
 #define VIRTIO_CONSOLE_PORT_READY	0
 #define VIRTIO_CONSOLE_CONSOLE_PORT	1
 #define VIRTIO_CONSOLE_RESIZE		2
 #define VIRTIO_CONSOLE_PORT_OPEN	3
 #define VIRTIO_CONSOLE_PORT_NAME	4
+#define VIRTIO_CONSOLE_THROTTLE_PORT	5
+#define VIRTIO_CONSOLE_CACHE_BUFFERS	6
 
 /* == In-qemu interface == */
 
@@ -96,6 +102,13 @@ struct VirtIOSerialPort {
     char *name;
 
     /*
+     * This list holds buffers pushed by the guest in case the guest
+     * sent incomplete messages or the host connection was down and
+     * the device requested to cache the data.
+     */
+    QTAILQ_HEAD(, VirtIOSerialPortBuffer) unflushed_buffers;
+
+    /*
      * This id helps identify ports between the guest and the host.
      * The guest sends a "header" with this id with each data packet
      * that it sends and the host can then find out which associated
@@ -103,6 +116,27 @@ struct VirtIOSerialPort {
      */
     uint32_t id;
 
+    /*
+     * Each port can specify the limit on number of bytes that can be
+     * outstanding in the unread buffers. This is to prevent any OOM
+     * situtation if a rogue process on the guest keeps injecting
+     * data.
+     */
+    size_t byte_limit;
+
+    /*
+     * The number of bytes we have queued up in our unread queue
+     */
+    size_t nr_bytes;
+
+    /*
+     * This boolean, when set, means "queue data that gets sent to
+     * this port when the host is not connected". The queued data, if
+     * any, is then sent out to the port when the host connection is
+     * opened.
+     */
+    uint8_t cache_buffers;
+
     /* Identify if this is a port that binds with hvc in the guest */
     uint8_t is_console;
 
@@ -110,6 +144,11 @@ struct VirtIOSerialPort {
     bool guest_connected;
     /* Is this device open for IO on the host? */
     bool host_connected;
+    /* Have we sent a throttle message to the guest? */
+    bool host_throttled;
+
+    /* Did this port get data in the recent handle_output call? */
+    bool has_activity;
 };
 
 struct VirtIOSerialPortInfo {
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Jamie Lokier]

Amit Shah wrote:
> Guests send us one buffer at a time. Current guests send buffers sized
> 4K bytes. If guest userspace applications sent out > 4K bytes in one
> write() syscall, the write request actually sends out multiple buffers,
> each of 4K in size.
> 
> This usually isn't a problem but for some apps, like VNC, the entire
> data has to be sent in one go to make copy/paste work fine. So if an app
> on the guest sends out guest clipboard contents, it has to be sent to
> the vnc server in one go as the guest app sent it.
> 
> For this to be done, we need the guest to send us START and END markers
> for each write request so that we can find out complete buffers and send
> them off to ports.

That looks very dubious.  TCP/IP doesn't maintain write boundaries;
neither do pipes, unix domain sockets, pseudo-terminals, and almost
every other modern byte-oriented transport.

So how does VNC transmit the clipboard over TCP/IP to a VNC client,
without those boundaries, and why is it different with virtserialport?

-- Jamie

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Amit Shah]

On (Fri) Jan 08 2010 [01:12:31], Jamie Lokier wrote:
> Amit Shah wrote:
> > Guests send us one buffer at a time. Current guests send buffers sized
> > 4K bytes. If guest userspace applications sent out > 4K bytes in one
> > write() syscall, the write request actually sends out multiple buffers,
> > each of 4K in size.
> > 
> > This usually isn't a problem but for some apps, like VNC, the entire
> > data has to be sent in one go to make copy/paste work fine. So if an app
> > on the guest sends out guest clipboard contents, it has to be sent to
> > the vnc server in one go as the guest app sent it.
> > 
> > For this to be done, we need the guest to send us START and END markers
> > for each write request so that we can find out complete buffers and send
> > them off to ports.
> 
> That looks very dubious.  TCP/IP doesn't maintain write boundaries;
> neither do pipes, unix domain sockets, pseudo-terminals, and almost
> every other modern byte-oriented transport.
> 
> So how does VNC transmit the clipboard over TCP/IP to a VNC client,
> without those boundaries, and why is it different with virtserialport?

TCP does this in its stack: it waits for the number of bytes written to
be received and then notifies userspace of data availibility.

In this case, consider the case where the guest writes 10k of data. The
guest gives us those 10k in 3 chunks: the first containing 4k (- header
size), the 2nd containing the next 4k (- header size) and the 3rd chunk
the remaining data.

I want to flush out this data only when I get all 10k.

		Amit

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Jamie Lokier]

Amit Shah wrote:
> On (Fri) Jan 08 2010 [01:12:31], Jamie Lokier wrote:
> > Amit Shah wrote:
> > > Guests send us one buffer at a time. Current guests send buffers sized
> > > 4K bytes. If guest userspace applications sent out > 4K bytes in one
> > > write() syscall, the write request actually sends out multiple buffers,
> > > each of 4K in size.
> > > 
> > > This usually isn't a problem but for some apps, like VNC, the entire
> > > data has to be sent in one go to make copy/paste work fine. So if an app
> > > on the guest sends out guest clipboard contents, it has to be sent to
> > > the vnc server in one go as the guest app sent it.
> > > 
> > > For this to be done, we need the guest to send us START and END markers
> > > for each write request so that we can find out complete buffers and send
> > > them off to ports.
> > 
> > That looks very dubious.  TCP/IP doesn't maintain write boundaries;
> > neither do pipes, unix domain sockets, pseudo-terminals, and almost
> > every other modern byte-oriented transport.
> > 
> > So how does VNC transmit the clipboard over TCP/IP to a VNC client,
> > without those boundaries, and why is it different with virtserialport?
> 
> TCP does this in its stack: it waits for the number of bytes written to
> be received and then notifies userspace of data availibility.
> 
> In this case, consider the case where the guest writes 10k of data. The
> guest gives us those 10k in 3 chunks: the first containing 4k (- header
> size), the 2nd containing the next 4k (- header size) and the 3rd chunk
> the remaining data.
> 
> I want to flush out this data only when I get all 10k.

No, TCP does not do that.  It does not maintain boundaries, or delay
delivery until a full write is transmitted.  Even if you use TCP_CORK
(Linux specific), that is just a performance hint.

If the sender writes 10k of data in a single write() over TCP, and it
is split into packets size 4k/4k/2k (assume just over 4k MSS :-), the
receiver will be notified of availability any time after the *first*
packet is received, and the read() call may indeed return less than
10k.  In fact it can be split at any byte position, depending on other
activity.

Applications handle this by using their own framing protocol on top of
the TCP byte stream.  For example a simple header saying "expect N
bytes" followed by N bytes, or line delimiters or escape characters.

Sometimes it looks like TCP is maintaining write boundaries, but it is
just an artifact of its behaviour on many systems, and is not reliable
even on those systems where it seems to happen most of the time.  Even
when connecting to localhost, you cannot rely on that.  I have seen
people write code assuming TCP keeps boundaries, and then some weeks
later they are very confused debugging their code because it is not
reliable...

Since VNC is clearly designed to work over TCP, and is written by
people who know this, I'm wondering why you think it needs to be
different for virtio-serial.

-- Jamie

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Anthony Liguori]

On 01/08/2010 07:35 AM, Jamie Lokier wrote:
> Sometimes it looks like TCP is maintaining write boundaries, but it is
> just an artifact of its behaviour on many systems, and is not reliable
> even on those systems where it seems to happen most of the time.  Even
> when connecting to localhost, you cannot rely on that.  I have seen
> people write code assuming TCP keeps boundaries, and then some weeks
> later they are very confused debugging their code because it is not
> reliable...
>
> Since VNC is clearly designed to work over TCP, and is written by
> people who know this, I'm wondering why you think it needs to be
> different for virtio-serial.
>    

I'm confused about why the buffering is needed in the first place.

I would think that any buffering should be pushed back to the guest.  
IOW, if there's available data from the char driver, but the guest 
doesn't have a buffer.  Don't select on the char driver until the guest 
has a buffer available.  If the guest attempts to write data but the 
char driver isn't ready to receive data, don't complete the request 
until the char driver can accept data.

Where does buffering come in?

Regards,

Anthony Liguori

> -- Jamie
>
>
>    

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Amit Shah]

On (Fri) Jan 08 2010 [13:35:03], Jamie Lokier wrote:
> 
> Since VNC is clearly designed to work over TCP, and is written by
> people who know this, I'm wondering why you think it needs to be
> different for virtio-serial.

For vnc putting stuff from a guest clipboard into vnc client clipboard
using the ServerCutText command, the entire buffer has to be provided
after sending the command and the 'length' values.

In this case, if the data from guest arrives in multiple packets, we
really don't want to call into the write function multiple times. A
single clipboard entry has to be created in the client with the entire
contents, so a single write operation has to be invoked.

For this to happen, there has to be some indication from the guest as to
how much data was written in one write() operation, which will let us
make a single write operation to the vnc client.

		Amit

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Amit Shah]

On (Fri) Jan 08 2010 [10:26:59], Anthony Liguori wrote:
> On 01/08/2010 07:35 AM, Jamie Lokier wrote:
>> Sometimes it looks like TCP is maintaining write boundaries, but it is
>> just an artifact of its behaviour on many systems, and is not reliable
>> even on those systems where it seems to happen most of the time.  Even
>> when connecting to localhost, you cannot rely on that.  I have seen
>> people write code assuming TCP keeps boundaries, and then some weeks
>> later they are very confused debugging their code because it is not
>> reliable...
>>
>> Since VNC is clearly designed to work over TCP, and is written by
>> people who know this, I'm wondering why you think it needs to be
>> different for virtio-serial.
>>    
>
> I'm confused about why the buffering is needed in the first place.
>
> I would think that any buffering should be pushed back to the guest.   
> IOW, if there's available data from the char driver, but the guest  
> doesn't have a buffer.  Don't select on the char driver until the guest  
> has a buffer available.  If the guest attempts to write data but the  
> char driver isn't ready to receive data, don't complete the request  
> until the char driver can accept data.

This is a different thing from what Jamie's talking about. A guest or a
host might be interested in communicating data without waiting for the
other end to come up. The other end can just start consuming the data
(even the data that it missed while it wasn't connected) once it's up.

(I can remove this option for now and add it later, if you prefer it
that way.)

		Amit

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Jamie Lokier]

Amit Shah wrote:
> On (Fri) Jan 08 2010 [13:35:03], Jamie Lokier wrote:
> > Since VNC is clearly designed to work over TCP, and is written by
> > people who know this, I'm wondering why you think it needs to be
> > different for virtio-serial.
> 
> For vnc putting stuff from a guest clipboard into vnc client clipboard
> using the ServerCutText command, the entire buffer has to be provided
> after sending the command and the 'length' values.

Are you talking about a VNC protocol command between qemu's VNC server
and the user's VNC client, or a private protocol between the guest and
qemu's VNC server?

> In this case, if the data from guest arrives in multiple packets, we
> really don't want to call into the write function multiple times. A
> single clipboard entry has to be created in the client with the entire
> contents, so a single write operation has to be invoked.

Same question again: *Why do you think the VNC server (in qemu) needs to
see the entire clipboard in a aingle write from the guest?*

You have already told it the total length to expect.  There is no
ambiguity about where it ends.

There is no need to do any more, if the reciever (in qemu) is
implemented correctly with a sane protocol.  That's assuming the guest
sends to qemu's VNC server which then sends it to the user's VNC client.

> For this to happen, there has to be some indication from the guest as to
> how much data was written in one write() operation, which will let us
> make a single write operation to the vnc client.

When it is sent to the user's VNC client, it will be split into
multiple packets by TCP. You *can't* send a single large write over
TCP without getting it split at arbitrary places. It's *impossible*. TCP
doesn't support that. It will split and merge your writes arbitrarily.

So the only interesting part is how it's transmitted from the guest to
qemu's VNC server first. Do you get to design that protocol yourself?

-- Jamie

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Amit Shah]

On (Mon) Jan 11 2010 [10:45:53], Jamie Lokier wrote:
> Amit Shah wrote:
> > On (Fri) Jan 08 2010 [13:35:03], Jamie Lokier wrote:
> > > Since VNC is clearly designed to work over TCP, and is written by
> > > people who know this, I'm wondering why you think it needs to be
> > > different for virtio-serial.
> > 
> > For vnc putting stuff from a guest clipboard into vnc client clipboard
> > using the ServerCutText command, the entire buffer has to be provided
> > after sending the command and the 'length' values.
> 
> Are you talking about a VNC protocol command between qemu's VNC server
> and the user's VNC client, or a private protocol between the guest and
> qemu's VNC server?

What happens is:

1. Guest puts something on its clipboard
2. An agent on the guest gets notified of new clipboard contents
3. This agent sends over the entire clipboard contents to qemu via
   virtio-serial
4. virtio-serial sends off this data to the virtio-serial-vnc code
5. ServerCutText message from the vnc backend is sent to the vnc client
6. vnc client's clipboard gets updated
7. You can see guest's clipboard contents in your client's clipboard.

I'm talking about steps 3, 4, 5 here.

> > In this case, if the data from guest arrives in multiple packets, we
> > really don't want to call into the write function multiple times. A
> > single clipboard entry has to be created in the client with the entire
> > contents, so a single write operation has to be invoked.
> 
> Same question again: *Why do you think the VNC server (in qemu) needs to
> see the entire clipboard in a aingle write from the guest?*
> 
> You have already told it the total length to expect.  There is no
> ambiguity about where it ends.

Where does the total length come from? It has to come from the guest.
Otherwise, the vnc code will not know if a byte stream contains two
separate clipboard entries or just one huge clipboard entry.

Earlier, I used to send the length of one write as issued by a guest to
qemu. I just changed that to send a START and END flag so that I don't
have to send the length.

If this doesn't explain it, then I think we're not understanding each
other here.

		Amit

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Jamie Lokier]

Amit Shah wrote:
> > Are you talking about a VNC protocol command between qemu's VNC server
> > and the user's VNC client, or a private protocol between the guest and
> > qemu's VNC server?
> 
> What happens is:
> 
> 1. Guest puts something on its clipboard
> 2. An agent on the guest gets notified of new clipboard contents
> 3. This agent sends over the entire clipboard contents to qemu via
>    virtio-serial
> 4. virtio-serial sends off this data to the virtio-serial-vnc code
> 5. ServerCutText message from the vnc backend is sent to the vnc client
> 6. vnc client's clipboard gets updated
> 7. You can see guest's clipboard contents in your client's clipboard.
> 
> I'm talking about steps 3, 4, 5 here.

Ok. Let's not worry about 5; it doesn't seem relevant, only that the
guest clipboad is sent to the host somehow.

> > You have already told it the total length to expect.  There is no
> > ambiguity about where it ends.
> 
> Where does the total length come from? It has to come from the guest.
> Otherwise, the vnc code will not know if a byte stream contains two
> separate clipboard entries or just one huge clipboard entry.

I see.  So it's a *really simple* protocol where the clipboard entry
is sent by the guest agent with a single write() without any framing bytes?

> Earlier, I used to send the length of one write as issued by a guest to
> qemu. I just changed that to send a START and END flag so that I don't
> have to send the length.

Why not just have the guest agent send a 4-byte header which is the
integer length of the clipboard blob to follow?

I.e. instead of

    int guest_send_clipboard(const char *data, size_t length)
    {
        return write_full(virtio_fd, data, length);
    }

do this:

    int guest_send_clipboard(const char *data, size_t length)
    {
        u32 encoded_length = cpu_to_be32(length);
        int err = write_full(virtio_serial_fd, &encoded_length,
                             sizeof(encoded_length));
        if (err == 0)
            err = write_full(virtio_serial_fd, data, length);
        return err;
    }

> If this doesn't explain it, then I think we're not understanding each
> other here.

It does explain it very well, thanks.  I think you're misguided about
the solution :-)

What confused me was you mentioned the VNC ServerCutText command
having to receive the whole data in one go.  ServerCutText isn't
really relevant to this, and clearly is encoded with VNC protocol
framing.  If it was RDP or the SDL client instead of VNC, it would be
something else.  All that matters is getting the clipboard blob from
guest to qemu in one piece, right?

Having the guest agent send a few framing bytes seems very simple, and
would have the added bonus that the same guest agent protocol would
work on a "real" emulated serial port, guest->host TCP, etc. where
virtio-serial isn't available in the guest OS (e.g. older kernels).

I really can't see any merit in making virtio-serial not be a serial
port, being instead like a unix datagram socket, to support a specific
user of virtio-serial when a trivial 4-byte header in the guest agent
code would be easier for that user anyway.

If it did that, I think the name virtio-serial would have to change to
virtio-datagram, becuase it wouldn't behave like a serial port any
more.  It would also be less useful for things that _do_ want
something like a pipe/serial port.  But why bother?

-- Jamie

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Anthony Liguori]

On 01/11/2010 05:33 PM, Jamie Lokier wrote:
> Amit Shah wrote:
>    
>>> Are you talking about a VNC protocol command between qemu's VNC server
>>> and the user's VNC client, or a private protocol between the guest and
>>> qemu's VNC server?
>>>        
>> What happens is:
>>
>> 1. Guest puts something on its clipboard
>> 2. An agent on the guest gets notified of new clipboard contents
>> 3. This agent sends over the entire clipboard contents to qemu via
>>     virtio-serial
>> 4. virtio-serial sends off this data to the virtio-serial-vnc code
>> 5. ServerCutText message from the vnc backend is sent to the vnc client
>> 6. vnc client's clipboard gets updated
>> 7. You can see guest's clipboard contents in your client's clipboard.
>>
>> I'm talking about steps 3, 4, 5 here.
>>      
> Ok. Let's not worry about 5; it doesn't seem relevant, only that the
> guest clipboad is sent to the host somehow.
>
>    
>>> You have already told it the total length to expect.  There is no
>>> ambiguity about where it ends.
>>>        
>> Where does the total length come from? It has to come from the guest.
>> Otherwise, the vnc code will not know if a byte stream contains two
>> separate clipboard entries or just one huge clipboard entry.
>>      
> I see.  So it's a *really simple* protocol where the clipboard entry
> is sent by the guest agent with a single write() without any framing bytes?
>
>    
>> Earlier, I used to send the length of one write as issued by a guest to
>> qemu. I just changed that to send a START and END flag so that I don't
>> have to send the length.
>>      
> Why not just have the guest agent send a 4-byte header which is the
> integer length of the clipboard blob to follow?
>
> I.e. instead of
>
>      int guest_send_clipboard(const char *data, size_t length)
>      {
>          return write_full(virtio_fd, data, length);
>      }
>
> do this:
>
>      int guest_send_clipboard(const char *data, size_t length)
>      {
>          u32 encoded_length = cpu_to_be32(length);
>          int err = write_full(virtio_serial_fd,&encoded_length,
>                               sizeof(encoded_length));
>          if (err == 0)
>              err = write_full(virtio_serial_fd, data, length);
>          return err;
>      }
>
>    
>> If this doesn't explain it, then I think we're not understanding each
>> other here.
>>      
> It does explain it very well, thanks.  I think you're misguided about
> the solution :-)
>
> What confused me was you mentioned the VNC ServerCutText command
> having to receive the whole data in one go.  ServerCutText isn't
> really relevant to this, and clearly is encoded with VNC protocol
> framing.  If it was RDP or the SDL client instead of VNC, it would be
> something else.  All that matters is getting the clipboard blob from
> guest to qemu in one piece, right?
>
> Having the guest agent send a few framing bytes seems very simple, and
> would have the added bonus that the same guest agent protocol would
> work on a "real" emulated serial port, guest->host TCP, etc. where
> virtio-serial isn't available in the guest OS (e.g. older kernels).
>
> I really can't see any merit in making virtio-serial not be a serial
> port, being instead like a unix datagram socket, to support a specific
> user of virtio-serial when a trivial 4-byte header in the guest agent
> code would be easier for that user anyway.
>
> If it did that, I think the name virtio-serial would have to change to
> virtio-datagram, becuase it wouldn't behave like a serial port any
> more.  It would also be less useful for things that _do_ want
> something like a pipe/serial port.  But why bother?
>    

I agree wrt a streaming protocol verses a datagram protocol.  The core 
argument IMHO is that the userspace interface is a file descriptor.  
Most programmers are used to assuming that boundaries aren't preserved 
in read/write calls.

Regards,

Anthony Liguori

> -- Jamie
>
>
>    

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Anthony Liguori]

On 01/11/2010 02:39 AM, Amit Shah wrote:
> On (Fri) Jan 08 2010 [10:26:59], Anthony Liguori wrote:
>    
>> On 01/08/2010 07:35 AM, Jamie Lokier wrote:
>>      
>>> Sometimes it looks like TCP is maintaining write boundaries, but it is
>>> just an artifact of its behaviour on many systems, and is not reliable
>>> even on those systems where it seems to happen most of the time.  Even
>>> when connecting to localhost, you cannot rely on that.  I have seen
>>> people write code assuming TCP keeps boundaries, and then some weeks
>>> later they are very confused debugging their code because it is not
>>> reliable...
>>>
>>> Since VNC is clearly designed to work over TCP, and is written by
>>> people who know this, I'm wondering why you think it needs to be
>>> different for virtio-serial.
>>>
>>>        
>> I'm confused about why the buffering is needed in the first place.
>>
>> I would think that any buffering should be pushed back to the guest.
>> IOW, if there's available data from the char driver, but the guest
>> doesn't have a buffer.  Don't select on the char driver until the guest
>> has a buffer available.  If the guest attempts to write data but the
>> char driver isn't ready to receive data, don't complete the request
>> until the char driver can accept data.
>>      
> This is a different thing from what Jamie's talking about. A guest or a
> host might be interested in communicating data without waiting for the
> other end to come up. The other end can just start consuming the data
> (even the data that it missed while it wasn't connected) once it's up.
>
> (I can remove this option for now and add it later, if you prefer it
> that way.)
>    

If it's not needed by your use case, please remove it.  Doing buffering 
gets tricky because you can't allow an infinite buffer for security 
reasons.  All you end up doing is increasing the size of the buffer 
beyond what the guest and client are capable of doing.  Since you still 
can lose data, apps have to be written to handle this.  I think it adds 
complexity without a lot of benefit.

Regards,

Anthony Liguori

> 		Amit
>    

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Amit Shah]

On (Mon) Jan 11 2010 [18:28:52], Anthony Liguori wrote:
>>>
>>> I would think that any buffering should be pushed back to the guest.
>>> IOW, if there's available data from the char driver, but the guest
>>> doesn't have a buffer.  Don't select on the char driver until the guest
>>> has a buffer available.  If the guest attempts to write data but the
>>> char driver isn't ready to receive data, don't complete the request
>>> until the char driver can accept data.
>>>      
>> This is a different thing from what Jamie's talking about. A guest or a
>> host might be interested in communicating data without waiting for the
>> other end to come up. The other end can just start consuming the data
>> (even the data that it missed while it wasn't connected) once it's up.
>>
>> (I can remove this option for now and add it later, if you prefer it
>> that way.)
>>    
>
> If it's not needed by your use case, please remove it.  Doing buffering  
> gets tricky because you can't allow an infinite buffer for security  
> reasons.  All you end up doing is increasing the size of the buffer  
> beyond what the guest and client are capable of doing.  Since you still  
> can lose data, apps have to be written to handle this.  I think it adds  
> complexity without a lot of benefit.

The buffering has to remain anyway since we can't assume that the ports
will consume the entire buffers we pass on to them. So we'll have to
buffer the data till the entire buffer is consumed.

That, or the buffer management should be passed off to individual ports.
Which might result in a lot of code duplication since we can have a lot
of these ports in different places in the qemu code.

So I guess it's better to leave the buffer management in the bus itself.

Which means we get the 'cache_buffers' functionality essentially for
free.

		Amit

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Amit Shah]

On (Mon) Jan 11 2010 [23:33:56], Jamie Lokier wrote:
> Amit Shah wrote:
> > > Are you talking about a VNC protocol command between qemu's VNC server
> > > and the user's VNC client, or a private protocol between the guest and
> > > qemu's VNC server?
> > 
> > What happens is:
> > 
> > 1. Guest puts something on its clipboard
> > 2. An agent on the guest gets notified of new clipboard contents
> > 3. This agent sends over the entire clipboard contents to qemu via
> >    virtio-serial
> > 4. virtio-serial sends off this data to the virtio-serial-vnc code
> > 5. ServerCutText message from the vnc backend is sent to the vnc client
> > 6. vnc client's clipboard gets updated
> > 7. You can see guest's clipboard contents in your client's clipboard.
> > 
> > I'm talking about steps 3, 4, 5 here.
> 
> Ok. Let's not worry about 5; it doesn't seem relevant, only that the
> guest clipboad is sent to the host somehow.

Actually, it is important...

> > > You have already told it the total length to expect.  There is no
> > > ambiguity about where it ends.
> > 
> > Where does the total length come from? It has to come from the guest.
> > Otherwise, the vnc code will not know if a byte stream contains two
> > separate clipboard entries or just one huge clipboard entry.
> 
> I see.  So it's a *really simple* protocol where the clipboard entry
> is sent by the guest agent with a single write() without any framing bytes?
> 
> > Earlier, I used to send the length of one write as issued by a guest to
> > qemu. I just changed that to send a START and END flag so that I don't
> > have to send the length.
> 
> Why not just have the guest agent send a 4-byte header which is the
> integer length of the clipboard blob to follow?
> 
> I.e. instead of
> 
>     int guest_send_clipboard(const char *data, size_t length)
>     {
>         return write_full(virtio_fd, data, length);
>     }
> 
> do this:
> 
>     int guest_send_clipboard(const char *data, size_t length)
>     {
>         u32 encoded_length = cpu_to_be32(length);
>         int err = write_full(virtio_serial_fd, &encoded_length,
>                              sizeof(encoded_length));
>         if (err == 0)
>             err = write_full(virtio_serial_fd, data, length);
>         return err;
>     }
> 
> > If this doesn't explain it, then I think we're not understanding each
> > other here.
> 
> It does explain it very well, thanks.  I think you're misguided about
> the solution :-)

The above solution you specify works if it's assumed that we hold off
writes to the vnc client till we get a complete buffer according to the
header received.

Now, a header might contain the length 10000, meaning 10000 bytes are to
be expected.

What if the write() on the guest fails after writing 8000 bytes? There's
no way for us to signal that.

So this vnc port might just be waiting for all 10000 bytes to be
received, and it may never receive anything more.

Or, it might receive the start of the next clipboard entry and it could
be interpreted as data from the previous copy.

> What confused me was you mentioned the VNC ServerCutText command
> having to receive the whole data in one go.  ServerCutText isn't
> really relevant to this,

It is relevant. You can't split up one ServerCutText command in multiple
buffers. You can also not execute any other commands while one command
is in progress, so you have to hold off on executing ServerCutText till
all the data is available. And you can't reliably do that from guest
userspace because of the previously-mentioned scenario.

> I really can't see any merit in making virtio-serial not be a serial
> port, being instead like a unix datagram socket, to support a specific
> user of virtio-serial when a trivial 4-byte header in the guest agent
> code would be easier for that user anyway.

BTW I don't really want this too, I can get rid of it if everyone agrees
we won't support clipboard writes > 4k over vnc or if there's a better
idea.

		Amit

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Anthony Liguori]

On 01/12/2010 01:16 AM, Amit Shah wrote:
> BTW I don't really want this too, I can get rid of it if everyone agrees
> we won't support clipboard writes>  4k over vnc or if there's a better
> idea.
>    

Why bother trying to preserve message boundaries?   I think that's the 
fundamental question.

Regards,

Anthony Liguori

> 		Amit
>
>
>    

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Amit Shah]

On (Tue) Jan 12 2010 [09:00:52], Anthony Liguori wrote:
> On 01/12/2010 01:16 AM, Amit Shah wrote:
>> BTW I don't really want this too, I can get rid of it if everyone agrees
>> we won't support clipboard writes>  4k over vnc or if there's a better
>> idea.
>>    
>
> Why bother trying to preserve message boundaries?   I think that's the  
> fundamental question.

For the vnc clipboard copy-paste case, I explained that in the couple of
mails before in this thread.

There might be other use-cases, I don't know about them though.

		Amit

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Anthony Liguori]

On 01/12/2010 09:13 AM, Amit Shah wrote:
> On (Tue) Jan 12 2010 [09:00:52], Anthony Liguori wrote:
>    
>> On 01/12/2010 01:16 AM, Amit Shah wrote:
>>      
>>> BTW I don't really want this too, I can get rid of it if everyone agrees
>>> we won't support clipboard writes>   4k over vnc or if there's a better
>>> idea.
>>>
>>>        
>> Why bother trying to preserve message boundaries?   I think that's the
>> fundamental question.
>>      
> For the vnc clipboard copy-paste case, I explained that in the couple of
> mails before in this thread.
>    

It didn't make sense to me.  I think the assumption has to be that the 
client can send corrupt data and the host has to handle it.

Regards,

Anthony Liguori

> There might be other use-cases, I don't know about them though.
>
> 		Amit
>    

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Amit Shah]

On (Tue) Jan 12 2010 [09:46:55], Anthony Liguori wrote:
> On 01/12/2010 09:13 AM, Amit Shah wrote:
>> On (Tue) Jan 12 2010 [09:00:52], Anthony Liguori wrote:
>>    
>>> On 01/12/2010 01:16 AM, Amit Shah wrote:
>>>      
>>>> BTW I don't really want this too, I can get rid of it if everyone agrees
>>>> we won't support clipboard writes>   4k over vnc or if there's a better
>>>> idea.
>>>>
>>>>        
>>> Why bother trying to preserve message boundaries?   I think that's the
>>> fundamental question.
>>>      
>> For the vnc clipboard copy-paste case, I explained that in the couple of
>> mails before in this thread.
>>    
>
> It didn't make sense to me.  I think the assumption has to be that the  
> client can send corrupt data and the host has to handle it.

You mean if the guest kernel sends the wrong flags? Or doesn't set the
flags? Can you explain what scenario you're talking about?

		Amit

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Anthony Liguori]

On 01/12/2010 09:49 AM, Amit Shah wrote:
> On (Tue) Jan 12 2010 [09:46:55], Anthony Liguori wrote:
>    
>> On 01/12/2010 09:13 AM, Amit Shah wrote:
>>      
>>> On (Tue) Jan 12 2010 [09:00:52], Anthony Liguori wrote:
>>>
>>>        
>>>> On 01/12/2010 01:16 AM, Amit Shah wrote:
>>>>
>>>>          
>>>>> BTW I don't really want this too, I can get rid of it if everyone agrees
>>>>> we won't support clipboard writes>    4k over vnc or if there's a better
>>>>> idea.
>>>>>
>>>>>
>>>>>            
>>>> Why bother trying to preserve message boundaries?   I think that's the
>>>> fundamental question.
>>>>
>>>>          
>>> For the vnc clipboard copy-paste case, I explained that in the couple of
>>> mails before in this thread.
>>>
>>>        
>> It didn't make sense to me.  I think the assumption has to be that the
>> client can send corrupt data and the host has to handle it.
>>      
> You mean if the guest kernel sends the wrong flags? Or doesn't set the
> flags? Can you explain what scenario you're talking about?
>    

It's very likely that you'll have to implement some sort of protocol on 
top of virtio-serial.  It won't always just be simple strings.

If you have a simple datagram protocol, that contains two ints and a 
string, it's going to have to be encoded like <int a><int b><int 
len><char data[len]>.  You need to validate that len fits within the 
boundaries and deal with len being less than the boundary.

If you've got a command protocol where the you send the guest something 
and then expect a response, you have to deal with the fact that the 
guest may never respond.  Having well defined message boundaries does 
not help the general problem and it only helps in the most trivial cases.

Basically, it boils down to a lot of complexity for something that isn't 
going to be helpful in most circumstances.

Regards,

Anthony Liguori

> 		Amit
>    

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Amit Shah]

On (Tue) Jan 12 2010 [09:55:41], Anthony Liguori wrote:
> On 01/12/2010 09:49 AM, Amit Shah wrote:
>> On (Tue) Jan 12 2010 [09:46:55], Anthony Liguori wrote:
>>    
>>> On 01/12/2010 09:13 AM, Amit Shah wrote:
>>>      
>>>> On (Tue) Jan 12 2010 [09:00:52], Anthony Liguori wrote:
>>>>
>>>>        
>>>>> On 01/12/2010 01:16 AM, Amit Shah wrote:
>>>>>
>>>>>          
>>>>>> BTW I don't really want this too, I can get rid of it if everyone agrees
>>>>>> we won't support clipboard writes>    4k over vnc or if there's a better
>>>>>> idea.
>>>>>>
>>>>>>
>>>>>>            
>>>>> Why bother trying to preserve message boundaries?   I think that's the
>>>>> fundamental question.
>>>>>
>>>>>          
>>>> For the vnc clipboard copy-paste case, I explained that in the couple of
>>>> mails before in this thread.
>>>>
>>>>        
>>> It didn't make sense to me.  I think the assumption has to be that the
>>> client can send corrupt data and the host has to handle it.
>>>      
>> You mean if the guest kernel sends the wrong flags? Or doesn't set the
>> flags? Can you explain what scenario you're talking about?
>>    
>
> It's very likely that you'll have to implement some sort of protocol on  
> top of virtio-serial.  It won't always just be simple strings.

Yes, virtio-serial is just meant to be a transport agnostic of whatever
data or protocols that ride over it.

> If you have a simple datagram protocol, that contains two ints and a  
> string, it's going to have to be encoded like <int a><int b><int  
> len><char data[len]>.  You need to validate that len fits within the  
> boundaries and deal with len being less than the boundary.
>
> If you've got a command protocol where the you send the guest something  
> and then expect a response, you have to deal with the fact that the  
> guest may never respond.  Having well defined message boundaries does  
> not help the general problem and it only helps in the most trivial cases.
>
> Basically, it boils down to a lot of complexity for something that isn't  
> going to be helpful in most circumstances.

I don't know why you're saying virtio-serial-bus does (or needs to) do
anything of this.

		Amit

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 01/12/2010 01:16 AM, Amit Shah wrote:
>> BTW I don't really want this too, I can get rid of it if everyone agrees
>> we won't support clipboard writes>  4k over vnc or if there's a better
>> idea.
>>    
>
> Why bother trying to preserve message boundaries?   I think that's the
> fundamental question.

Yes.  Either it's a datagram or a stream pipe.  I always thought it
would be a stream pipe, as the name "serial" suggests.

As to the clipboard use case: same problem exists with any old stream
pipe, including TCP, same solutions apply.  If you told the peer "I'm
going to send you 12345 bytes now", and your stream pipe chokes after
7890 bytes, you retry until everything got through.  If you want to be
able to abort a partial transfer and start a new one, you layer a
protocol suitable for that on top of your stream pipe.

Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Anthony Liguori]

On 01/13/2010 11:14 AM, Markus Armbruster wrote:
> Anthony Liguori<anthony@codemonkey.ws>  writes:
>
>    
>> On 01/12/2010 01:16 AM, Amit Shah wrote:
>>      
>>> BTW I don't really want this too, I can get rid of it if everyone agrees
>>> we won't support clipboard writes>   4k over vnc or if there's a better
>>> idea.
>>>
>>>        
>> Why bother trying to preserve message boundaries?   I think that's the
>> fundamental question.
>>      
> Yes.  Either it's a datagram or a stream pipe.  I always thought it
> would be a stream pipe, as the name "serial" suggests.
>    

And if it's a datagram, then we should accept that there will be a fixed 
max message size which is pretty common in all datagram protocols.  That 
fixed size should be no larger than what the transport supports so in 
this case, it would be 4k.

If a guest wants to send larger messages, it must build a continuation 
protocol on top of the datagram protocol.

Regards,

Anthony Liguori

[Qemu-devel] [PATCH 0/8] virtio-console: Move to qdev, multiple devices, generic ports

[Amit Shah]

Hello people,

This iteration of the series removes the START and END flags (and
hence the header associated with each buffer). That's the major change
since the last submission.

Please review.

Obligatory disclaimer:
This series splits up the patches by functionality. Note, however,
that patches 2-6 introduce some functionality that's advertised to the
guest as having to work all at once or not at all. Also, the savevm
version is bumped only once but save/restore state is added in each of
the patches. They are split only for easier reviewability.

The older virtio-console.c file is completely removed and a new
virtio-serial.c is introduced so that reviewing is easier. I can send a
later patch to rename virtio-serial.c back to virtio-console.c.

Amit Shah (8):
  virtio: Remove duplicate macro definition for max. virtqueues, bump
    up the max
  virtio-console: qdev conversion, new virtio-serial-bus
  virtio-serial-bus: Maintain guest and host port open/close state
  virtio-serial-bus: Add a port 'name' property for port discovery in
    guests
  virtio-serial-bus: Add support for buffering guest output, throttling
    guests
  virtio-serial-bus: Add ability to hot-unplug ports
  virtio-serial: Add a 'virtserialport' device for generic serial port
    support
  Move virtio-serial to Makefile.objs

 Makefile.objs          |    2 +-
 Makefile.target        |    2 +-
 hw/pc.c                |   11 +-
 hw/ppc440_bamboo.c     |    7 -
 hw/qdev.c              |   10 +-
 hw/s390-virtio-bus.c   |   17 +-
 hw/s390-virtio-bus.h   |    2 +
 hw/s390-virtio.c       |    8 -
 hw/virtio-console.c    |  143 ---------
 hw/virtio-console.h    |   19 --
 hw/virtio-pci.c        |   13 +-
 hw/virtio-serial-bus.c |  788 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-serial.c     |  143 +++++++++
 hw/virtio-serial.h     |  199 ++++++++++++
 hw/virtio.c            |    2 -
 hw/virtio.h            |    4 +-
 qemu-options.hx        |    4 +
 sysemu.h               |    6 -
 vl.c                   |    2 +
 19 files changed, 1165 insertions(+), 217 deletions(-)
 delete mode 100644 hw/virtio-console.c
 delete mode 100644 hw/virtio-console.h
 create mode 100644 hw/virtio-serial-bus.c
 create mode 100644 hw/virtio-serial.c
 create mode 100644 hw/virtio-serial.h

[Qemu-devel] [PATCH 1/8] virtio: Remove duplicate macro definition for max. virtqueues, bump up the max

[Amit Shah]

VIRTIO_PCI_QUEUE_MAX is redefined in hw/virtio.c. Let's just keep it in
hw/virtio.h.

Also, bump up the value of the maximum allowed virtqueues to 64. This is
in preparation to allow multiple ports per virtio-console device.

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

diff --git a/hw/virtio.c b/hw/virtio.c
index fa7184a..7c020a3 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -75,8 +75,6 @@ struct VirtQueue
     void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq);
 };
 
-#define VIRTIO_PCI_QUEUE_MAX        16
-
 /* virt queue functions */
 static void virtqueue_init(VirtQueue *vq)
 {
diff --git a/hw/virtio.h b/hw/virtio.h
index 3994cc9..7b2b327 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -90,7 +90,7 @@ typedef struct {
     unsigned (*get_features)(void * opaque);
 } VirtIOBindings;
 
-#define VIRTIO_PCI_QUEUE_MAX 16
+#define VIRTIO_PCI_QUEUE_MAX 64
 
 #define VIRTIO_NO_VECTOR 0xffff
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/8] virtio-console: qdev conversion, new virtio-serial-bus

[Amit Shah]

This commit converts the virtio-console device to create a new
virtio-serial bus that can host console and generic serial ports. The
file hosting this code is now called virtio-serial-bus.c.

The virtio console is now a very simple qdev device that sits on the
virtio-serial-bus and communicates between the bus and qemu's chardevs.

This commit also includes a few changes to the virtio backing code for
pci and s390 to spawn the virtio-serial bus.

As a result of the qdev conversion, we get rid of a lot of legacy code.
The old-style way of instantiating a virtio console using

    -virtioconsole ...

is maintained, but the new, preferred way is to use

    -device virtio-serial -device virtconsole,chardev=...

With this commit, multiple devices as well as multiple ports with a
single device can be supported.

For multiple ports support, each port gets an IO vq pair. Since the
guest needs to know in advance how many vqs a particular device will
need, we have to set this number as a property of the virtio-serial
device and also as a config option.

In addition, we also spawn a pair of control IO vqs. This is an internal
channel meant for guest-host communication for things like port
open/close, sending port properties over to the guest, etc.

This commit is a part of a series of other commits to get the full
implementation of multiport support. Future commits will add other
support as well as ride on the savevm version that we bump up here.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 Makefile.target        |    2 +-
 hw/pc.c                |   11 +-
 hw/ppc440_bamboo.c     |    7 -
 hw/qdev.c              |   10 +-
 hw/s390-virtio-bus.c   |   17 +-
 hw/s390-virtio-bus.h   |    2 +
 hw/s390-virtio.c       |    8 -
 hw/virtio-console.c    |  143 -------------
 hw/virtio-console.h    |   19 --
 hw/virtio-pci.c        |   13 +-
 hw/virtio-serial-bus.c |  539 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-serial.c     |  107 ++++++++++
 hw/virtio-serial.h     |  162 +++++++++++++++
 hw/virtio.h            |    2 +-
 qemu-options.hx        |    4 +
 sysemu.h               |    6 -
 vl.c                   |    2 +
 17 files changed, 841 insertions(+), 213 deletions(-)
 delete mode 100644 hw/virtio-console.c
 delete mode 100644 hw/virtio-console.h
 create mode 100644 hw/virtio-serial-bus.c
 create mode 100644 hw/virtio-serial.c
 create mode 100644 hw/virtio-serial.h

diff --git a/Makefile.target b/Makefile.target
index e661478..60df16d 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -172,7 +172,7 @@ ifdef CONFIG_SOFTMMU
 obj-y = vl.o async.o monitor.o pci.o pci_host.o pcie_host.o machine.o gdbstub.o
 # virtio has to be here due to weird dependency between PCI and virtio-net.
 # need to fix this properly
-obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o virtio-pci.o
+obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-serial.o virtio-serial-bus.o virtio-pci.o
 obj-$(CONFIG_KVM) += kvm.o kvm-all.o
 obj-$(CONFIG_ISA_MMIO) += isa_mmio.o
 LIBS+=-lz
diff --git a/hw/pc.c b/hw/pc.c
index a93c5f2..3aadfa9 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1018,15 +1018,6 @@ static void pc_init1(ram_addr_t ram_size,
             pci_create_simple(pci_bus, -1, "lsi53c895a");
         }
     }
-
-    /* Add virtio console devices */
-    if (pci_enabled) {
-        for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
-            if (virtcon_hds[i]) {
-                pci_create_simple(pci_bus, -1, "virtio-console-pci");
-            }
-        }
-    }
 }
 
 static void pc_init_pci(ram_addr_t ram_size,
@@ -1102,7 +1093,7 @@ static QEMUMachine pc_machine_v0_10 = {
             .property = "class",
             .value    = stringify(PCI_CLASS_STORAGE_OTHER),
         },{
-            .driver   = "virtio-console-pci",
+            .driver   = "virtio-serial-pci",
             .property = "class",
             .value    = stringify(PCI_CLASS_DISPLAY_OTHER),
         },{
diff --git a/hw/ppc440_bamboo.c b/hw/ppc440_bamboo.c
index a488240..1ab9872 100644
--- a/hw/ppc440_bamboo.c
+++ b/hw/ppc440_bamboo.c
@@ -108,13 +108,6 @@ static void bamboo_init(ram_addr_t ram_size,
     env = ppc440ep_init(&ram_size, &pcibus, pci_irq_nrs, 1, cpu_model);
 
     if (pcibus) {
-        /* Add virtio console devices */
-        for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
-            if (virtcon_hds[i]) {
-                pci_create_simple(pcibus, -1, "virtio-console-pci");
-            }
-        }
-
         /* Register network interfaces. */
         for (i = 0; i < nb_nics; i++) {
             /* There are no PCI NICs on the Bamboo board, but there are
diff --git a/hw/qdev.c b/hw/qdev.c
index b6bd4ae..c643576 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -321,13 +321,9 @@ void qdev_machine_creation_done(void)
 CharDriverState *qdev_init_chardev(DeviceState *dev)
 {
     static int next_serial;
-    static int next_virtconsole;
-    /* FIXME: This is a nasty hack that needs to go away.  */
-    if (strncmp(dev->info->name, "virtio", 6) == 0) {
-        return virtcon_hds[next_virtconsole++];
-    } else {
-        return serial_hds[next_serial++];
-    }
+
+    /* FIXME: This function needs to go away: use chardev properties!  */
+    return serial_hds[next_serial++];
 }
 
 BusState *qdev_get_parent_bus(DeviceState *dev)
diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index 980e7eb..6b6dafc 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -26,7 +26,7 @@
 #include "loader.h"
 #include "elf.h"
 #include "hw/virtio.h"
-#include "hw/virtio-console.h"
+#include "hw/virtio-serial.h"
 #include "hw/sysbus.h"
 #include "kvm.h"
 
@@ -131,7 +131,7 @@ static int s390_virtio_blk_init(VirtIOS390Device *dev)
     return s390_virtio_device_init(dev, vdev);
 }
 
-static int s390_virtio_console_init(VirtIOS390Device *dev)
+static int s390_virtio_serial_init(VirtIOS390Device *dev)
 {
     VirtIOS390Bus *bus;
     VirtIODevice *vdev;
@@ -139,7 +139,7 @@ static int s390_virtio_console_init(VirtIOS390Device *dev)
 
     bus = DO_UPCAST(VirtIOS390Bus, bus, dev->qdev.parent_bus);
 
-    vdev = virtio_console_init((DeviceState *)dev);
+    vdev = virtio_serial_init((DeviceState *)dev, dev->max_virtserial_ports);
     if (!vdev) {
         return -1;
     }
@@ -342,11 +342,14 @@ static VirtIOS390DeviceInfo s390_virtio_blk = {
     },
 };
 
-static VirtIOS390DeviceInfo s390_virtio_console = {
-    .init = s390_virtio_console_init,
-    .qdev.name = "virtio-console-s390",
+static VirtIOS390DeviceInfo s390_virtio_serial = {
+    .init = s390_virtio_serial_init,
+    .qdev.name = "virtio-serial-s390",
+    .qdev.alias = "virtio-serial",
     .qdev.size = sizeof(VirtIOS390Device),
     .qdev.props = (Property[]) {
+        DEFINE_PROP_UINT32("max_ports", VirtIOS390Device, max_virtserial_ports,
+                           31),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
@@ -370,7 +373,7 @@ static void s390_virtio_bus_register_withprop(VirtIOS390DeviceInfo *info)
 
 static void s390_virtio_register(void)
 {
-    s390_virtio_bus_register_withprop(&s390_virtio_console);
+    s390_virtio_bus_register_withprop(&s390_virtio_serial);
     s390_virtio_bus_register_withprop(&s390_virtio_blk);
     s390_virtio_bus_register_withprop(&s390_virtio_net);
 }
diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
index 8ae2065..8e4763a 100644
--- a/hw/s390-virtio-bus.h
+++ b/hw/s390-virtio-bus.h
@@ -41,6 +41,8 @@ typedef struct VirtIOS390Device {
     DriveInfo *dinfo;
     NICConf nic;
     uint32_t host_features;
+    /* Max. number of ports we can have for a the virtio-serial device */
+    uint32_t max_virtserial_ports;
 } VirtIOS390Device;
 
 typedef struct VirtIOS390Bus {
diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index 0fa6ba6..3582728 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -26,7 +26,6 @@
 #include "loader.h"
 #include "elf.h"
 #include "hw/virtio.h"
-#include "hw/virtio-console.h"
 #include "hw/sysbus.h"
 #include "kvm.h"
 
@@ -207,13 +206,6 @@ static void s390_init(ram_addr_t ram_size,
                                strlen(kernel_cmdline), 1);
     }
 
-    /* Create VirtIO console */
-    for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
-        if (virtcon_hds[i]) {
-            qdev_init_nofail(qdev_create((BusState *)s390_bus, "virtio-console-s390"));
-        }
-    }
-
     /* Create VirtIO network adapters */
     for(i = 0; i < nb_nics; i++) {
         NICInfo *nd = &nd_table[i];
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
deleted file mode 100644
index 4f18ef2..0000000
--- a/hw/virtio-console.c
+++ /dev/null
@@ -1,143 +0,0 @@
-/*
- * Virtio Console Device
- *
- * Copyright IBM, Corp. 2008
- *
- * Authors:
- *  Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2.  See
- * the COPYING file in the top-level directory.
- *
- */
-
-#include "hw.h"
-#include "qemu-char.h"
-#include "virtio.h"
-#include "virtio-console.h"
-
-
-typedef struct VirtIOConsole
-{
-    VirtIODevice vdev;
-    VirtQueue *ivq, *ovq;
-    CharDriverState *chr;
-} VirtIOConsole;
-
-static VirtIOConsole *to_virtio_console(VirtIODevice *vdev)
-{
-    return (VirtIOConsole *)vdev;
-}
-
-static void virtio_console_handle_output(VirtIODevice *vdev, VirtQueue *vq)
-{
-    VirtIOConsole *s = to_virtio_console(vdev);
-    VirtQueueElement elem;
-
-    while (virtqueue_pop(vq, &elem)) {
-        ssize_t len = 0;
-        int d;
-
-        for (d = 0; d < elem.out_num; d++) {
-            len += qemu_chr_write(s->chr, (uint8_t *)elem.out_sg[d].iov_base,
-                                  elem.out_sg[d].iov_len);
-        }
-        virtqueue_push(vq, &elem, len);
-        virtio_notify(vdev, vq);
-    }
-}
-
-static void virtio_console_handle_input(VirtIODevice *vdev, VirtQueue *vq)
-{
-}
-
-static uint32_t virtio_console_get_features(VirtIODevice *vdev, uint32_t f)
-{
-    return f;
-}
-
-static int vcon_can_read(void *opaque)
-{
-    VirtIOConsole *s = (VirtIOConsole *) opaque;
-
-    if (!virtio_queue_ready(s->ivq) ||
-        !(s->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
-        virtio_queue_empty(s->ivq))
-        return 0;
-
-    /* current implementations have a page sized buffer.
-     * We fall back to a one byte per read if there is not enough room.
-     * It would be cool to have a function that returns the available byte
-     * instead of checking for a limit */
-    if (virtqueue_avail_bytes(s->ivq, TARGET_PAGE_SIZE, 0))
-        return TARGET_PAGE_SIZE;
-    if (virtqueue_avail_bytes(s->ivq, 1, 0))
-        return 1;
-    return 0;
-}
-
-static void vcon_read(void *opaque, const uint8_t *buf, int size)
-{
-    VirtIOConsole *s = (VirtIOConsole *) opaque;
-    VirtQueueElement elem;
-    int offset = 0;
-
-    /* The current kernel implementation has only one outstanding input
-     * buffer of PAGE_SIZE. Nevertheless, this function is prepared to
-     * handle multiple buffers with multiple sg element for input */
-    while (offset < size) {
-        int i = 0;
-        if (!virtqueue_pop(s->ivq, &elem))
-                break;
-        while (offset < size && i < elem.in_num) {
-            int len = MIN(elem.in_sg[i].iov_len, size - offset);
-            memcpy(elem.in_sg[i].iov_base, buf + offset, len);
-            offset += len;
-            i++;
-        }
-        virtqueue_push(s->ivq, &elem, size);
-    }
-    virtio_notify(&s->vdev, s->ivq);
-}
-
-static void vcon_event(void *opaque, int event)
-{
-    /* we will ignore any event for the time being */
-}
-
-static void virtio_console_save(QEMUFile *f, void *opaque)
-{
-    VirtIOConsole *s = opaque;
-
-    virtio_save(&s->vdev, f);
-}
-
-static int virtio_console_load(QEMUFile *f, void *opaque, int version_id)
-{
-    VirtIOConsole *s = opaque;
-
-    if (version_id != 1)
-        return -EINVAL;
-
-    virtio_load(&s->vdev, f);
-    return 0;
-}
-
-VirtIODevice *virtio_console_init(DeviceState *dev)
-{
-    VirtIOConsole *s;
-    s = (VirtIOConsole *)virtio_common_init("virtio-console",
-                                            VIRTIO_ID_CONSOLE,
-                                            0, sizeof(VirtIOConsole));
-    s->vdev.get_features = virtio_console_get_features;
-
-    s->ivq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_input);
-    s->ovq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_output);
-
-    s->chr = qdev_init_chardev(dev);
-    qemu_chr_add_handlers(s->chr, vcon_can_read, vcon_read, vcon_event, s);
-
-    register_savevm("virtio-console", -1, 1, virtio_console_save, virtio_console_load, s);
-
-    return &s->vdev;
-}
diff --git a/hw/virtio-console.h b/hw/virtio-console.h
deleted file mode 100644
index 84d0717..0000000
--- a/hw/virtio-console.h
+++ /dev/null
@@ -1,19 +0,0 @@
-/*
- * Virtio Console Support
- *
- * Copyright IBM, Corp. 2008
- *
- * Authors:
- *  Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2.  See
- * the COPYING file in the top-level directory.
- *
- */
-#ifndef _QEMU_VIRTIO_CONSOLE_H
-#define _QEMU_VIRTIO_CONSOLE_H
-
-/* The ID for virtio console */
-#define VIRTIO_ID_CONSOLE 3
-
-#endif
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 573c98a..e7fabfb 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -95,6 +95,8 @@ typedef struct {
     DriveInfo *dinfo;
     NICConf nic;
     uint32_t host_features;
+    /* Max. number of ports we can have for a the virtio-serial device */
+    uint32_t max_virtserial_ports;
 } VirtIOPCIProxy;
 
 /* virtio device */
@@ -483,7 +485,7 @@ static int virtio_blk_exit_pci(PCIDevice *pci_dev)
     return virtio_exit_pci(pci_dev);
 }
 
-static int virtio_console_init_pci(PCIDevice *pci_dev)
+static int virtio_serial_init_pci(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
     VirtIODevice *vdev;
@@ -493,7 +495,7 @@ static int virtio_console_init_pci(PCIDevice *pci_dev)
         proxy->class_code != PCI_CLASS_OTHERS)          /* qemu-kvm  */
         proxy->class_code = PCI_CLASS_COMMUNICATION_OTHER;
 
-    vdev = virtio_console_init(&pci_dev->qdev);
+    vdev = virtio_serial_init(&pci_dev->qdev, proxy->max_virtserial_ports);
     if (!vdev) {
         return -1;
     }
@@ -573,13 +575,16 @@ static PCIDeviceInfo virtio_info[] = {
         },
         .qdev.reset = virtio_pci_reset,
     },{
-        .qdev.name = "virtio-console-pci",
+        .qdev.name = "virtio-serial-pci",
+        .qdev.alias = "virtio-serial",
         .qdev.size = sizeof(VirtIOPCIProxy),
-        .init      = virtio_console_init_pci,
+        .init      = virtio_serial_init_pci,
         .exit      = virtio_exit_pci,
         .qdev.props = (Property[]) {
             DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
             DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
+            DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, max_virtserial_ports,
+                               31),
             DEFINE_PROP_END_OF_LIST(),
         },
         .qdev.reset = virtio_pci_reset,
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
new file mode 100644
index 0000000..fcb86a6
--- /dev/null
+++ b/hw/virtio-serial-bus.c
@@ -0,0 +1,539 @@
+/*
+ * A bus for connecting virtio serial and console ports
+ *
+ * Copyright (C) 2009 Red Hat, Inc.
+ *
+ * Author(s):
+ *  Amit Shah <amit.shah@redhat.com>
+ *
+ * Some earlier parts are:
+ *  Copyright IBM, Corp. 2008
+ * authored by
+ *  Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "monitor.h"
+#include "qemu-queue.h"
+#include "sysbus.h"
+#include "virtio-serial.h"
+
+/* The virtio-serial bus on top of which the ports will ride as devices */
+struct VirtIOSerialBus {
+    BusState qbus;
+
+    /* This is the parent device that provides the bus for ports. */
+    VirtIOSerial *vser;
+
+    /* The maximum number of ports that can ride on top of this bus */
+    uint32_t max_nr_ports;
+};
+
+struct VirtIOSerial {
+    VirtIODevice vdev;
+
+    VirtQueue *c_ivq, *c_ovq;
+    /* Arrays of ivqs and ovqs: one per port */
+    VirtQueue **ivqs, **ovqs;
+
+    VirtIOSerialBus *bus;
+
+    QTAILQ_HEAD(, VirtIOSerialPort) ports;
+    struct virtio_console_config config;
+};
+
+static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
+{
+    VirtIOSerialPort *port;
+
+    QTAILQ_FOREACH(port, &vser->ports, next) {
+        if (port->id == id)
+            return port;
+    }
+    return NULL;
+}
+
+static VirtIOSerialPort *find_port_by_vq(VirtIOSerial *vser, VirtQueue *vq)
+{
+    VirtIOSerialPort *port;
+
+    QTAILQ_FOREACH(port, &vser->ports, next) {
+        if (port->ivq == vq || port->ovq == vq)
+            return port;
+    }
+    return NULL;
+}
+
+static bool use_multiport(VirtIOSerial *vser)
+{
+    return vser->vdev.guest_features & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
+}
+
+static size_t write_to_port(VirtIOSerialPort *port,
+                            const uint8_t *buf, size_t size)
+{
+    VirtQueueElement elem;
+    struct virtio_console_header header;
+    VirtQueue *vq;
+    size_t offset = 0;
+    size_t len = 0;
+    int header_len;
+
+    vq = port->ivq;
+    if (!virtio_queue_ready(vq)) {
+        return 0;
+    }
+    if (!size) {
+        return 0;
+    }
+    header.flags = 0;
+    header_len = use_multiport(port->vser) ? sizeof(header) : 0;
+
+    while (offset < size) {
+        int i;
+
+        if (!virtqueue_pop(vq, &elem)) {
+            break;
+        }
+        if (elem.in_sg[0].iov_len < header_len) {
+            /* We can't even store our port number in this buffer. Bug? */
+            qemu_error("virtio-serial: size %zd less than expected\n",
+                    elem.in_sg[0].iov_len);
+            exit(1);
+        }
+        if (header_len) {
+            memcpy(elem.in_sg[0].iov_base, &header, header_len);
+        }
+
+        for (i = 0; offset < size && i < elem.in_num; i++) {
+            /* Copy the header only in the first sg. */
+            len = MIN(elem.in_sg[i].iov_len - header_len, size - offset);
+
+            memcpy(elem.in_sg[i].iov_base + header_len, buf + offset, len);
+            offset += len;
+            header_len = 0;
+        }
+        header_len = use_multiport(port->vser) ? sizeof(header) : 0;
+        virtqueue_push(vq, &elem, len + header_len);
+    }
+
+    virtio_notify(&port->vser->vdev, vq);
+    return offset;
+}
+
+static size_t send_control_msg(VirtIOSerialPort *port, void *buf, size_t len)
+{
+    VirtQueueElement elem;
+    VirtQueue *vq;
+    struct virtio_console_control *cpkt;
+
+    vq = port->vser->c_ivq;
+    if (!virtio_queue_ready(vq)) {
+        return 0;
+    }
+    if (!virtqueue_pop(vq, &elem)) {
+        return 0;
+    }
+
+    cpkt = (struct virtio_console_control *)buf;
+    stl_p(&cpkt->id, port->id);
+    memcpy(elem.in_sg[0].iov_base, buf, len);
+
+    virtqueue_push(vq, &elem, len);
+    virtio_notify(&port->vser->vdev, vq);
+    return len;
+}
+
+static size_t send_control_event(VirtIOSerialPort *port, uint16_t event,
+                                 uint16_t value)
+{
+    struct virtio_console_control cpkt;
+
+    stw_p(&cpkt.event, event);
+    stw_p(&cpkt.value, value);
+
+    return send_control_msg(port, &cpkt, sizeof(cpkt));
+}
+
+/* Functions for use inside qemu to open and read from/write to ports */
+int virtio_serial_open(VirtIOSerialPort *port)
+{
+    return 0;
+}
+
+int virtio_serial_close(VirtIOSerialPort *port)
+{
+    return 0;
+}
+
+/* Individual ports/apps call this function to write to the guest. */
+ssize_t virtio_serial_write(VirtIOSerialPort *port, const uint8_t *buf,
+                            size_t size)
+{
+    return write_to_port(port, buf, size);
+}
+
+/*
+ * Readiness of the guest to accept data on a port.
+ * Returns max. data the guest can receive
+ */
+size_t virtio_serial_guest_ready(VirtIOSerialPort *port)
+{
+    VirtQueue *vq = port->ivq;
+    size_t size, header_len;
+
+    header_len = use_multiport(port->vser)
+        ? sizeof(struct virtio_console_header) : 0;
+
+    if (!virtio_queue_ready(vq) ||
+        !(port->vser->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
+        virtio_queue_empty(vq)) {
+        return 0;
+    }
+
+    size = 4096;
+    if (virtqueue_avail_bytes(vq, size, 0)) {
+        return size - header_len;
+    }
+    size = header_len + 1;
+    if (virtqueue_avail_bytes(vq, size, 0)) {
+        return size - header_len;
+    }
+    return 0;
+}
+
+/* Guest wants to notify us of some event */
+static void handle_control_message(VirtIOSerial *vser, void *buf)
+{
+    struct VirtIOSerialPort *port;
+    struct virtio_console_control cpkt, *gcpkt;
+
+    gcpkt = buf;
+    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
+    if (!port)
+        return;
+
+    cpkt.event = lduw_p(&gcpkt->event);
+    cpkt.value = lduw_p(&gcpkt->value);
+
+    switch(cpkt.event) {
+    case VIRTIO_CONSOLE_PORT_READY:
+        /*
+         * Now that we know the guest asked for the port name, we're
+         * sure the guest has initialised whatever state is necessary
+         * for this port. Now's a good time to let the guest know if
+         * this port is a console port so that the guest can hook it
+         * up to hvc.
+         */
+        if (port->is_console) {
+            send_control_event(port, VIRTIO_CONSOLE_CONSOLE_PORT, 1);
+        }
+        /*
+         * When the guest has asked us for this information it means
+         * the guest is all setup and has its virtqueues
+         * initialised. If some app is interested in knowing about
+         * this event, let it know.
+         */
+        if (port->info->guest_ready) {
+            port->info->guest_ready(port);
+        }
+        break;
+    }
+}
+
+static void control_in(VirtIODevice *vdev, VirtQueue *vq)
+{
+}
+
+static void control_out(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtQueueElement elem;
+    VirtIOSerial *vser;
+
+    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
+
+    while (virtqueue_pop(vq, &elem)) {
+        handle_control_message(vser, elem.out_sg[0].iov_base);
+        virtqueue_push(vq, &elem, elem.out_sg[0].iov_len);
+    }
+    virtio_notify(vdev, vq);
+}
+
+/*
+ * Guest wrote something to some port.
+ */
+static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIOSerial *vser;
+    VirtQueueElement elem;
+
+    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
+
+    while (virtqueue_pop(vq, &elem)) {
+        VirtIOSerialPort *port;
+        struct virtio_console_header header;
+        int header_len;
+
+        header_len = use_multiport(vser) ? sizeof(header) : 0;
+
+        if (elem.out_sg[0].iov_len < header_len) {
+            goto next_buf;
+        }
+        port = find_port_by_vq(vser, vq);
+        if (!port) {
+            goto next_buf;
+        }
+        /*
+         * A port may not have any handler registered for consuming the
+         * data that the guest sends or it may not have a chardev associated
+         * with it. Just ignore the data in that case.
+         */
+        if (!port->info->have_data) {
+            goto next_buf;
+        }
+
+        /* The guest always sends only one sg */
+        port->info->have_data(port, elem.out_sg[0].iov_base + header_len,
+                              elem.out_sg[0].iov_len - header_len);
+
+    next_buf:
+        virtqueue_push(vq, &elem, elem.out_sg[0].iov_len);
+    }
+    virtio_notify(vdev, vq);
+}
+
+static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
+{
+}
+
+static uint32_t get_features(VirtIODevice *vdev, uint32_t features)
+{
+    features |= (1 << VIRTIO_CONSOLE_F_MULTIPORT);
+
+    return features;
+}
+
+/* Guest requested config info */
+static void get_config(VirtIODevice *vdev, uint8_t *config_data)
+{
+    VirtIOSerial *vser;
+
+    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
+    memcpy(config_data, &vser->config, sizeof(struct virtio_console_config));
+}
+
+static void set_config(VirtIODevice *vdev, const uint8_t *config_data)
+{
+    struct virtio_console_config config;
+
+    memcpy(&config, config_data, sizeof(config));
+}
+
+static void virtio_serial_save(QEMUFile *f, void *opaque)
+{
+    VirtIOSerial *s = opaque;
+
+    /* The virtio device */
+    virtio_save(&s->vdev, f);
+
+    /* The config space */
+    qemu_put_be16s(f, &s->config.cols);
+    qemu_put_be16s(f, &s->config.rows);
+    qemu_put_be32s(f, &s->config.nr_ports);
+}
+
+static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
+{
+    VirtIOSerial *s = opaque;
+
+    if (version_id > 2) {
+        return -EINVAL;
+    }
+    /* The virtio device */
+    virtio_load(&s->vdev, f);
+
+    if (version_id < 2) {
+        return 0;
+    }
+
+    /* The config space */
+    qemu_get_be16s(f, &s->config.cols);
+    qemu_get_be16s(f, &s->config.rows);
+    s->config.nr_ports = qemu_get_be32(f);
+
+    return 0;
+}
+
+static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
+
+static struct BusInfo virtser_bus_info = {
+    .name      = "virtio-serial-bus",
+    .size      = sizeof(VirtIOSerialBus),
+    .print_dev = virtser_bus_dev_print,
+};
+
+static VirtIOSerialBus *virtser_bus_new(DeviceState *dev)
+{
+    VirtIOSerialBus *bus;
+
+    bus = FROM_QBUS(VirtIOSerialBus, qbus_create(&virtser_bus_info, dev,
+                                                 "virtio-serial-bus"));
+    bus->qbus.allow_hotplug = 1;
+
+    return bus;
+}
+
+static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
+{
+    VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev);
+    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
+
+    monitor_printf(mon, "%*s dev-prop-int: id: %u\n",
+                   indent, "", port->id);
+}
+
+static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
+{
+    VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev);
+    VirtIOSerialPortInfo *info = DO_UPCAST(VirtIOSerialPortInfo, qdev, base);
+    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
+    VirtIOSerialBus *bus = DO_UPCAST(VirtIOSerialBus, qbus, qdev->parent_bus);
+    int ret;
+    bool plugging_port0;
+
+    port->vser = bus->vser;
+
+    /*
+     * Is the first console port we're seeing? If so, put it up at
+     * location 0. This is done for backward compatibility (old
+     * kernel, new qemu).
+     */
+    plugging_port0 = port->is_console && !find_port_by_id(port->vser, 0);
+
+    if (port->vser->config.nr_ports == bus->max_nr_ports && !plugging_port0) {
+        qemu_error("virtio-serial-bus: Maximum device limit reached\n");
+        return -1;
+    }
+    dev->info = info;
+
+    ret = info->init(dev);
+    if (ret) {
+        return ret;
+    }
+
+    port->id = plugging_port0 ? 0 : port->vser->config.nr_ports++;
+
+    QTAILQ_INSERT_TAIL(&port->vser->ports, port, next);
+    port->ivq = port->vser->ivqs[port->id];
+    port->ovq = port->vser->ovqs[port->id];
+
+    /* Send an update to the guest about this new port added */
+    virtio_notify_config(&port->vser->vdev);
+
+    return ret;
+}
+
+static int virtser_port_qdev_exit(DeviceState *qdev)
+{
+    VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev);
+    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
+    VirtIOSerial *vser = port->vser;
+
+    /*
+     * Don't decrement nr_ports here; thus we keep a linearly
+     * increasing port id. Not utilising an id again saves us a couple
+     * of complications:
+     *
+     * - Not having to bother about sending the port id to the guest
+     *   kernel on hotplug or on addition of new ports; the guest can
+     *   also linearly increment the port number. This is preferable
+     *   because the config space won't have the need to store a
+     *   ports_map.
+     *
+     * - Extra state to be stored for all the "holes" that got created
+     *   so that we keep filling in the ids from the least available
+     *   index.
+     *
+     * When such a functionality is desired, a control message to add
+     * a port can be introduced.
+     */
+    QTAILQ_REMOVE(&vser->ports, port, next);
+
+    if (port->info->exit)
+        port->info->exit(dev);
+
+    return 0;
+}
+
+void virtio_serial_port_qdev_register(VirtIOSerialPortInfo *info)
+{
+    info->qdev.init = virtser_port_qdev_init;
+    info->qdev.bus_info = &virtser_bus_info;
+    info->qdev.exit = virtser_port_qdev_exit;
+    info->qdev.unplug = qdev_simple_unplug_cb;
+    qdev_register(&info->qdev);
+}
+
+VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports)
+{
+    VirtIOSerial *vser;
+    VirtIODevice *vdev;
+    uint32_t i;
+
+    if (!max_nr_ports)
+        return NULL;
+
+    vdev = virtio_common_init("virtio-serial", VIRTIO_ID_CONSOLE,
+                              sizeof(struct virtio_console_config),
+                              sizeof(VirtIOSerial));
+
+    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
+
+    /* Spawn a new virtio-serial bus on which the ports will ride as devices */
+    vser->bus = virtser_bus_new(dev);
+    vser->bus->vser = vser;
+    QTAILQ_INIT(&vser->ports);
+
+    vser->bus->max_nr_ports = max_nr_ports;
+    vser->ivqs = qemu_malloc(max_nr_ports * sizeof(VirtQueue *));
+    vser->ovqs = qemu_malloc(max_nr_ports * sizeof(VirtQueue *));
+
+    /* Add a queue for host to guest transfers for port 0 (backward compat) */
+    vser->ivqs[0] = virtio_add_queue(vdev, 128, handle_input);
+    /* Add a queue for guest to host transfers for port 0 (backward compat) */
+    vser->ovqs[0] = virtio_add_queue(vdev, 128, handle_output);
+
+    /* control queue: host to guest */
+    vser->c_ivq = virtio_add_queue(vdev, 16, control_in);
+    /* control queue: guest to host */
+    vser->c_ovq = virtio_add_queue(vdev, 16, control_out);
+
+    for (i = 1; i < vser->bus->max_nr_ports; i++) {
+        /* Add a per-port queue for host to guest transfers */
+        vser->ivqs[i] = virtio_add_queue(vdev, 128, handle_input);
+        /* Add a per-per queue for guest to host transfers */
+        vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
+    }
+
+    vser->config.max_nr_ports = max_nr_ports;
+    /*
+     * Reserve location 0 for a console port for backward compat
+     * (old kernel, new qemu)
+     */
+    vser->config.nr_ports = 1;
+
+    vser->vdev.get_features = get_features;
+    vser->vdev.get_config = get_config;
+    vser->vdev.set_config = set_config;
+
+    /*
+     * Register for the savevm section with the virtio-console name
+     * to preserve backward compat
+     */
+    register_savevm("virtio-console", -1, 2, virtio_serial_save,
+                    virtio_serial_load, vser);
+
+    return vdev;
+}
diff --git a/hw/virtio-serial.c b/hw/virtio-serial.c
new file mode 100644
index 0000000..9d003f8
--- /dev/null
+++ b/hw/virtio-serial.c
@@ -0,0 +1,107 @@
+/*
+ * Virtio Console and Generic Serial Port Devices
+ *
+ * Copyright Red Hat, Inc. 2009
+ *
+ * Authors:
+ *  Amit Shah <amit.shah@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu-char.h"
+#include "virtio-serial.h"
+
+typedef struct VirtConsole {
+    VirtIOSerialPort port;
+    CharDriverState *chr;
+} VirtConsole;
+
+
+/* Callback function that's called when the guest sends us data */
+static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
+{
+    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+
+    return qemu_chr_write(vcon->chr, buf, len);
+}
+
+/* Readiness of the guest to accept data on a port */
+static int chr_can_read(void *opaque)
+{
+    VirtConsole *vcon = opaque;
+
+    return virtio_serial_guest_ready(&vcon->port);
+}
+
+/* Send data from a char device over to the guest */
+static void chr_read(void *opaque, const uint8_t *buf, int size)
+{
+    VirtConsole *vcon = opaque;
+
+    virtio_serial_write(&vcon->port, buf, size);
+}
+
+static void chr_event(void *opaque, int event)
+{
+    VirtConsole *vcon = opaque;
+
+    switch (event) {
+    case CHR_EVENT_OPENED: {
+        virtio_serial_open(&vcon->port);
+        break;
+    }
+    case CHR_EVENT_CLOSED:
+        virtio_serial_close(&vcon->port);
+        break;
+    }
+}
+
+/* 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->info = dev->info;
+
+    port->is_console = true;
+
+    if (vcon->chr) {
+        qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
+                              vcon);
+    }
+    return 0;
+}
+
+static int virtconsole_exitfn(VirtIOSerialDevice *dev)
+{
+    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
+    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+
+    if (vcon->chr) {
+        qemu_chr_close(vcon->chr);
+    }
+
+    return 0;
+}
+
+static VirtIOSerialPortInfo virtconsole_info = {
+    .qdev.name     = "virtconsole",
+    .qdev.size     = sizeof(VirtConsole),
+    .init          = virtconsole_initfn,
+    .exit          = virtconsole_exitfn,
+    .have_data     = flush_buf,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_UINT8("is_console", VirtConsole, port.is_console, 1),
+        DEFINE_PROP_CHR("chardev", VirtConsole, chr),
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static void virtconsole_register(void)
+{
+    virtio_serial_port_qdev_register(&virtconsole_info);
+}
+device_init(virtconsole_register)
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
new file mode 100644
index 0000000..9aac856
--- /dev/null
+++ b/hw/virtio-serial.h
@@ -0,0 +1,162 @@
+/*
+ * Virtio Serial / Console Support
+ *
+ * Copyright IBM, Corp. 2008
+ * Copyright Red Hat, Inc. 2009
+ *
+ * Authors:
+ *  Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
+ *  Amit Shah <amit.shah@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+#ifndef _QEMU_VIRTIO_SERIAL_H
+#define _QEMU_VIRTIO_SERIAL_H
+
+#include <stdbool.h>
+#include "qdev.h"
+#include "virtio.h"
+
+/* == Interface shared between the guest kernel and qemu == */
+
+/* The Virtio ID for virtio console / serial ports */
+#define VIRTIO_ID_CONSOLE		3
+
+/* Features supported */
+#define VIRTIO_CONSOLE_F_MULTIPORT	1
+
+struct virtio_console_config {
+    /*
+     * These two fields are used by VIRTIO_CONSOLE_F_SIZE which
+     * isn't implemented here yet
+     */
+    uint16_t cols;
+    uint16_t rows;
+
+    uint32_t max_nr_ports;
+    uint32_t nr_ports;
+} __attribute__((packed));
+
+struct virtio_console_control {
+    uint32_t id;		/* Port number */
+    uint16_t event;		/* The kind of control event (see below) */
+    uint16_t value;		/* Extra information for the key */
+};
+
+struct virtio_console_header {
+    uint32_t flags;		/* Some message between host and guest */
+};
+
+/* Some events for the internal messages (control packets) */
+#define VIRTIO_CONSOLE_PORT_READY	0
+#define VIRTIO_CONSOLE_CONSOLE_PORT	1
+#define VIRTIO_CONSOLE_RESIZE		2
+
+/* == In-qemu interface == */
+
+typedef struct VirtIOSerial VirtIOSerial;
+typedef struct VirtIOSerialBus VirtIOSerialBus;
+typedef struct VirtIOSerialPort VirtIOSerialPort;
+typedef struct VirtIOSerialPortInfo VirtIOSerialPortInfo;
+
+typedef struct VirtIOSerialDevice {
+    DeviceState qdev;
+    VirtIOSerialPortInfo *info;
+} VirtIOSerialDevice;
+
+/*
+ * This is the state that's shared between all the ports.  Some of the
+ * state is configurable via command-line options. Some of it can be
+ * set by individual devices in their initfn routines. Some of the
+ * state is set by the generic qdev device init routine.
+ */
+struct VirtIOSerialPort {
+    DeviceState dev;
+    VirtIOSerialPortInfo *info;
+
+    QTAILQ_ENTRY(VirtIOSerialPort) next;
+
+    /*
+     * This field gives us the virtio device as well as the qdev bus
+     * that we are associated with
+     */
+    VirtIOSerial *vser;
+
+    VirtQueue *ivq, *ovq;
+
+    /*
+     * This id helps identify ports between the guest and the host.
+     * The guest sends a "header" with this id with each data packet
+     * that it sends and the host can then find out which associated
+     * device to send out this data to
+     */
+    uint32_t id;
+
+    /* Identify if this is a port that binds with hvc in the guest */
+    uint8_t is_console;
+};
+
+struct VirtIOSerialPortInfo {
+    DeviceInfo qdev;
+    /*
+     * The per-port (or per-app) init function that's called when a
+     * new device is found on the bus.
+     */
+    int (*init)(VirtIOSerialDevice *dev);
+    /*
+     * Per-port exit function that's called when a port gets
+     * hot-unplugged or removed.
+     */
+    int (*exit)(VirtIOSerialDevice *dev);
+
+    /* Callbacks for guest events */
+        /* Guest opened device. */
+    void (*guest_open)(VirtIOSerialPort *port);
+        /* Guest closed device. */
+    void (*guest_close)(VirtIOSerialPort *port);
+
+        /* Guest is now ready to accept data (virtqueues set up). */
+    void (*guest_ready)(VirtIOSerialPort *port);
+
+    /*
+     * Guest wrote some data to the port. This data is handed over to
+     * the app via this callback. The app returns the number of bytes
+     * it successfully consumed or a negative number on error.
+     */
+    ssize_t (*have_data)(VirtIOSerialPort *port, const uint8_t *buf, size_t len);
+};
+
+/* Interface to the virtio-serial bus */
+
+/*
+ * Individual ports/apps should call this function to register the port
+ * with the virtio-serial bus
+ */
+void virtio_serial_port_qdev_register(VirtIOSerialPortInfo *info);
+
+/*
+ * Open a connection to the port
+ *   Returns 0 on success (always).
+ */
+int virtio_serial_open(VirtIOSerialPort *port);
+
+/*
+ * Close the connection to the port
+ *   Returns 0 on success (always).
+ */
+int virtio_serial_close(VirtIOSerialPort *port);
+
+/*
+ * Send data to Guest
+ */
+ssize_t virtio_serial_write(VirtIOSerialPort *port, const uint8_t *buf,
+                            size_t size);
+
+/*
+ * Query whether a guest is ready to receive data.
+ */
+size_t virtio_serial_guest_ready(VirtIOSerialPort *port);
+
+#endif
diff --git a/hw/virtio.h b/hw/virtio.h
index 7b2b327..62e882b 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -171,7 +171,7 @@ void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
 /* Base devices.  */
 VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo);
 VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf);
-VirtIODevice *virtio_console_init(DeviceState *dev);
+VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports);
 VirtIODevice *virtio_balloon_init(DeviceState *dev);
 
 void virtio_net_exit(VirtIODevice *vdev);
diff --git a/qemu-options.hx b/qemu-options.hx
index e2edd71..0c429e5 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1885,6 +1885,10 @@ DEF("virtioconsole", HAS_ARG, QEMU_OPTION_virtiocon, \
 STEXI
 @item -virtioconsole @var{c}
 Set virtio console.
+
+This option is maintained for backward compatibility.
+
+Please use @code{-device virtconsole} for the new way of invocation.
 ETEXI
 
 DEF("show-cursor", 0, QEMU_OPTION_show_cursor, \
diff --git a/sysemu.h b/sysemu.h
index 9d80bb2..9c3b281 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -231,12 +231,6 @@ extern CharDriverState *serial_hds[MAX_SERIAL_PORTS];
 
 extern CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
 
-/* virtio consoles */
-
-#define MAX_VIRTIO_CONSOLES 1
-
-extern CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
-
 #define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
 
 #ifdef HAS_AUDIO
diff --git a/vl.c b/vl.c
index 06cb40d..89663a4 100644
--- a/vl.c
+++ b/vl.c
@@ -173,6 +173,8 @@ int main(int argc, char **argv)
 
 #define DEFAULT_RAM_SIZE 128
 
+#define MAX_VIRTIO_CONSOLES 1
+
 static const char *data_dir;
 const char *bios_name = NULL;
 /* Note: drives_table[MAX_DRIVES] is a dummy block driver if none available
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/8] virtio-serial-bus: Maintain guest and host port open/close state

[Amit Shah]

Via control channel messages, the guest can tell us whether a port got
opened or closed. Similarly, we can also indicate to the guest of host
port open/close events.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |   91 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-serial.c     |    6 +++
 hw/virtio-serial.h     |    6 +++
 3 files changed, 103 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index fcb86a6..276de6a 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -160,11 +160,22 @@ static size_t send_control_event(VirtIOSerialPort *port, uint16_t event,
 /* Functions for use inside qemu to open and read from/write to ports */
 int virtio_serial_open(VirtIOSerialPort *port)
 {
+    /* Don't allow opening an already-open port */
+    if (port->host_connected) {
+        return 0;
+    }
+    /* Send port open notification to the guest */
+    port->host_connected = true;
+    send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
+
     return 0;
 }
 
 int virtio_serial_close(VirtIOSerialPort *port)
 {
+    port->host_connected = false;
+    send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
+
     return 0;
 }
 
@@ -172,6 +183,9 @@ int virtio_serial_close(VirtIOSerialPort *port)
 ssize_t virtio_serial_write(VirtIOSerialPort *port, const uint8_t *buf,
                             size_t size)
 {
+    if (!port || !port->host_connected || !port->guest_connected) {
+        return 0;
+    }
     return write_to_port(port, buf, size);
 }
 
@@ -192,6 +206,9 @@ size_t virtio_serial_guest_ready(VirtIOSerialPort *port)
         virtio_queue_empty(vq)) {
         return 0;
     }
+    if (use_multiport(port->vser) && !port->guest_connected) {
+        return 0;
+    }
 
     size = 4096;
     if (virtqueue_avail_bytes(vq, size, 0)) {
@@ -230,6 +247,11 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
         if (port->is_console) {
             send_control_event(port, VIRTIO_CONSOLE_CONSOLE_PORT, 1);
         }
+
+        if (port->host_connected) {
+            send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
+        }
+
         /*
          * When the guest has asked us for this information it means
          * the guest is all setup and has its virtqueues
@@ -240,6 +262,19 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
             port->info->guest_ready(port);
         }
         break;
+
+    case VIRTIO_CONSOLE_PORT_OPEN:
+        port->guest_connected = cpkt.value;
+        if (cpkt.value && port->info->guest_open) {
+            /* Send the guest opened notification if an app is interested */
+            port->info->guest_open(port);
+        }
+
+        if (!cpkt.value && port->info->guest_close) {
+            /* Send the guest closed notification if an app is interested */
+            port->info->guest_close(port);
+        }
+        break;
     }
 }
 
@@ -334,6 +369,8 @@ static void set_config(VirtIODevice *vdev, const uint8_t *config_data)
 static void virtio_serial_save(QEMUFile *f, void *opaque)
 {
     VirtIOSerial *s = opaque;
+    VirtIOSerialPort *port;
+    uint32_t nr_active_ports;
 
     /* The virtio device */
     virtio_save(&s->vdev, f);
@@ -342,15 +379,42 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
     qemu_put_be16s(f, &s->config.cols);
     qemu_put_be16s(f, &s->config.rows);
     qemu_put_be32s(f, &s->config.nr_ports);
+
+    /* Items in struct VirtIOSerial */
+
+    /* Do this because we might have hot-unplugged some ports */
+    nr_active_ports = 0;
+    QTAILQ_FOREACH(port, &s->ports, next)
+        nr_active_ports++;
+
+    qemu_put_be32s(f, &nr_active_ports);
+
+    /*
+     * Items in struct VirtIOSerialPort.
+     */
+    QTAILQ_FOREACH(port, &s->ports, next) {
+        /*
+         * We put the port number because we may not have an active
+         * port at id 0 that's reserved for a console port, or in case
+         * of ports that might have gotten unplugged
+         */
+        qemu_put_be32s(f, &port->id);
+        qemu_put_byte(f, port->guest_connected);
+
+    }
 }
 
 static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 {
     VirtIOSerial *s = opaque;
+    VirtIOSerialPort *port;
+    uint32_t nr_active_ports;
+    unsigned int i;
 
     if (version_id > 2) {
         return -EINVAL;
     }
+
     /* The virtio device */
     virtio_load(&s->vdev, f);
 
@@ -363,6 +427,21 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     qemu_get_be16s(f, &s->config.rows);
     s->config.nr_ports = qemu_get_be32(f);
 
+    /* Items in struct VirtIOSerial */
+
+    qemu_get_be32s(f, &nr_active_ports);
+
+    /* Items in struct VirtIOSerialPort */
+    for (i = 0; i < nr_active_ports; i++) {
+        uint32_t id;
+
+        id = qemu_get_be32(f);
+        port = find_port_by_id(s, id);
+
+        port->guest_connected = qemu_get_byte(f);
+
+    }
+
     return 0;
 }
 
@@ -392,6 +471,10 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
 
     monitor_printf(mon, "%*s dev-prop-int: id: %u\n",
                    indent, "", port->id);
+    monitor_printf(mon, "%*s dev-prop-int: guest_connected: %d\n",
+                   indent, "", port->guest_connected);
+    monitor_printf(mon, "%*s dev-prop-int: host_connected: %d\n",
+                   indent, "", port->host_connected);
 }
 
 static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
@@ -425,6 +508,14 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
 
     port->id = plugging_port0 ? 0 : port->vser->config.nr_ports++;
 
+    if (!use_multiport(port->vser)) {
+        /*
+         * Allow writes to guest in this case; we have no way of
+         * knowing if a guest port is connected.
+         */
+        port->guest_connected = true;
+    }
+
     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.c b/hw/virtio-serial.c
index 9d003f8..d9a6f32 100644
--- a/hw/virtio-serial.c
+++ b/hw/virtio-serial.c
@@ -68,6 +68,12 @@ static int virtconsole_initfn(VirtIOSerialDevice *dev)
 
     port->is_console = true;
 
+    /*
+     * For console ports, just assume the guest is ready to accept our
+     * data.
+     */
+    port->guest_connected = true;
+
     if (vcon->chr) {
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
                               vcon);
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index 9aac856..1576eef 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -53,6 +53,7 @@ struct virtio_console_header {
 #define VIRTIO_CONSOLE_PORT_READY	0
 #define VIRTIO_CONSOLE_CONSOLE_PORT	1
 #define VIRTIO_CONSOLE_RESIZE		2
+#define VIRTIO_CONSOLE_PORT_OPEN	3
 
 /* == In-qemu interface == */
 
@@ -96,6 +97,11 @@ struct VirtIOSerialPort {
 
     /* Identify if this is a port that binds with hvc in the guest */
     uint8_t is_console;
+
+    /* Is the corresponding guest device open? */
+    bool guest_connected;
+    /* Is this device open for IO on the host? */
+    bool host_connected;
 };
 
 struct VirtIOSerialPortInfo {
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/8] virtio-serial-bus: Add a port 'name' property for port discovery in guests

[Amit Shah]

The port 'id' or number is internal state between the guest kernel and
our bus implementation. This is invocation-dependent and isn't part of
the guest-host ABI.

To correcly enumerate and map ports between the host and the guest, the
'name' property is used.

Example:

    -device virtserialport,name=org.qemu.port.0

This invocation will get us a char device in the guest at:

    /dev/virtio-ports/org.qemu.port.0

which can be a symlink to

    /dev/vport0p3

This 'name' property is exposed by the guest kernel in a sysfs
attribute:

    /sys/kernel/virtio-ports/vport0p3/name

A simple udev script can pick up this name and create the symlink
mentioned above.

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

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 276de6a..2576140 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -226,6 +226,8 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
 {
     struct VirtIOSerialPort *port;
     struct virtio_console_control cpkt, *gcpkt;
+    uint8_t *buffer;
+    size_t buffer_len;
 
     gcpkt = buf;
     port = find_port_by_id(vser, ldl_p(&gcpkt->id));
@@ -248,6 +250,21 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
             send_control_event(port, VIRTIO_CONSOLE_CONSOLE_PORT, 1);
         }
 
+        if (port->name) {
+            stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
+            stw_p(&cpkt.value, 1);
+
+            buffer_len = sizeof(cpkt) + strlen(port->name) + 1;
+            buffer = qemu_malloc(buffer_len);
+
+            memcpy(buffer, &cpkt, sizeof(cpkt));
+            memcpy(buffer + sizeof(cpkt), port->name, strlen(port->name));
+            buffer[buffer_len - 1] = 0;
+
+            send_control_msg(port, buffer, buffer_len);
+            qemu_free(buffer);
+        }
+
         if (port->host_connected) {
             send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
         }
diff --git a/hw/virtio-serial.c b/hw/virtio-serial.c
index d9a6f32..470446b 100644
--- a/hw/virtio-serial.c
+++ b/hw/virtio-serial.c
@@ -102,6 +102,7 @@ static VirtIOSerialPortInfo virtconsole_info = {
     .qdev.props = (Property[]) {
         DEFINE_PROP_UINT8("is_console", VirtConsole, port.is_console, 1),
         DEFINE_PROP_CHR("chardev", VirtConsole, chr),
+        DEFINE_PROP_STRING("name", VirtConsole, port.name),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index 1576eef..b669c7f 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -54,6 +54,7 @@ struct virtio_console_header {
 #define VIRTIO_CONSOLE_CONSOLE_PORT	1
 #define VIRTIO_CONSOLE_RESIZE		2
 #define VIRTIO_CONSOLE_PORT_OPEN	3
+#define VIRTIO_CONSOLE_PORT_NAME	4
 
 /* == In-qemu interface == */
 
@@ -88,6 +89,13 @@ struct VirtIOSerialPort {
     VirtQueue *ivq, *ovq;
 
     /*
+     * This name is sent to the guest and exported via sysfs.
+     * The guest could create symlinks based on this information.
+     * The name is in the reverse fqdn format, like org.qemu.console.0
+     */
+    char *name;
+
+    /*
      * This id helps identify ports between the guest and the host.
      * The guest sends a "header" with this id with each data packet
      * that it sends and the host can then find out which associated
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Amit Shah]

We have to buffer data from guest as ports may not consume all the data
in one go or the guest could be fast in sending data and the apps may
not consume at the same rate.

We keep caching data the guest sends us till a port accepts it. However,
this could lead to an OOM condition where a rogue process on the guest
could continue pumping in data while the host continues to cache it.

We introduce a per-port byte-limit property to alleviate this condition.
When this limit is reached, we send a control message to the guest
indicating it to not send us any more data till further indication. When
the number of bytes cached go lesser than the limit specified, we tell
the guest to restart sending data.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |  215 +++++++++++++++++++++++++++++++++++++++---------
 hw/virtio-serial.c     |    6 --
 hw/virtio-serial.h     |   30 ++++++-
 3 files changed, 203 insertions(+), 48 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 2576140..1ec67d2 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -44,6 +44,18 @@ struct VirtIOSerial {
     struct virtio_console_config config;
 };
 
+/* This struct holds individual buffers received for each port */
+typedef struct VirtIOSerialPortBuffer {
+    QTAILQ_ENTRY(VirtIOSerialPortBuffer) next;
+
+    uint8_t *buf;
+
+    size_t len; /* length of the buffer */
+    size_t offset; /* offset from which to consume data in the buffer */
+
+    bool previous_failure; /* Did sending out this buffer fail previously? */
+} VirtIOSerialPortBuffer;
+
 static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
 {
     VirtIOSerialPort *port;
@@ -75,11 +87,9 @@ static size_t write_to_port(VirtIOSerialPort *port,
                             const uint8_t *buf, size_t size)
 {
     VirtQueueElement elem;
-    struct virtio_console_header header;
     VirtQueue *vq;
     size_t offset = 0;
     size_t len = 0;
-    int header_len;
 
     vq = port->ivq;
     if (!virtio_queue_ready(vq)) {
@@ -88,8 +98,6 @@ static size_t write_to_port(VirtIOSerialPort *port,
     if (!size) {
         return 0;
     }
-    header.flags = 0;
-    header_len = use_multiport(port->vser) ? sizeof(header) : 0;
 
     while (offset < size) {
         int i;
@@ -97,26 +105,14 @@ static size_t write_to_port(VirtIOSerialPort *port,
         if (!virtqueue_pop(vq, &elem)) {
             break;
         }
-        if (elem.in_sg[0].iov_len < header_len) {
-            /* We can't even store our port number in this buffer. Bug? */
-            qemu_error("virtio-serial: size %zd less than expected\n",
-                    elem.in_sg[0].iov_len);
-            exit(1);
-        }
-        if (header_len) {
-            memcpy(elem.in_sg[0].iov_base, &header, header_len);
-        }
 
         for (i = 0; offset < size && i < elem.in_num; i++) {
-            /* Copy the header only in the first sg. */
-            len = MIN(elem.in_sg[i].iov_len - header_len, size - offset);
+            len = MIN(elem.in_sg[i].iov_len, size - offset);
 
-            memcpy(elem.in_sg[i].iov_base + header_len, buf + offset, len);
+            memcpy(elem.in_sg[i].iov_base, buf + offset, len);
             offset += len;
-            header_len = 0;
         }
-        header_len = use_multiport(port->vser) ? sizeof(header) : 0;
-        virtqueue_push(vq, &elem, len + header_len);
+        virtqueue_push(vq, &elem, len);
     }
 
     virtio_notify(&port->vser->vdev, vq);
@@ -157,6 +153,96 @@ static size_t send_control_event(VirtIOSerialPort *port, uint16_t event,
     return send_control_msg(port, &cpkt, sizeof(cpkt));
 }
 
+static void init_buf(VirtIOSerialPortBuffer *buf, uint8_t *buffer, size_t len)
+{
+    buf->buf = buffer;
+    buf->len = len;
+    buf->offset = 0;
+    buf->previous_failure = false;
+}
+
+static VirtIOSerialPortBuffer *alloc_buf(size_t len)
+{
+    VirtIOSerialPortBuffer *buf;
+
+    buf = qemu_malloc(sizeof(*buf));
+    buf->buf = qemu_malloc(len);
+
+    init_buf(buf, buf->buf, len);
+
+    return buf;
+}
+
+static void free_buf(VirtIOSerialPortBuffer *buf)
+{
+    qemu_free(buf->buf);
+    qemu_free(buf);
+}
+
+static void flush_queue(VirtIOSerialPort *port)
+{
+    VirtIOSerialPortBuffer *buf;
+    size_t out_size;
+    ssize_t ret;
+
+    while ((buf = QTAILQ_FIRST(&port->unflushed_buffers))) {
+        QTAILQ_REMOVE(&port->unflushed_buffers, buf, next);
+
+        out_size = buf->len - buf->offset;
+        if (!port->host_connected) {
+            port->nr_bytes -= buf->len + buf->offset;
+            free_buf(buf);
+            continue;
+        }
+
+        ret = port->info->have_data(port, buf->buf + buf->offset, out_size);
+        if (ret < out_size) {
+            QTAILQ_INSERT_HEAD(&port->unflushed_buffers, buf, next);
+        }
+        if (ret <= 0) {
+            /* We're not progressing at all */
+            if (buf->previous_failure) {
+                break;
+            }
+            buf->previous_failure = true;
+        } else {
+            buf->offset += ret;
+            port->nr_bytes -= ret;
+            buf->previous_failure = false;
+        }
+        if (!(buf->len - buf->offset)) {
+            free_buf(buf);
+        }
+    }
+
+    if (port->host_throttled && port->nr_bytes < port->byte_limit) {
+        port->host_throttled = false;
+        send_control_event(port, VIRTIO_CONSOLE_THROTTLE_PORT, 0);
+    }
+}
+
+static void flush_all_ports(VirtIOSerial *vser)
+{
+    struct VirtIOSerialPort *port;
+
+    QTAILQ_FOREACH(port, &vser->ports, next) {
+        if (port->has_activity) {
+            port->has_activity = false;
+            flush_queue(port);
+        }
+    }
+}
+
+static void remove_port_buffers(VirtIOSerialPort *port)
+{
+    struct VirtIOSerialPortBuffer *buf, *buf2;
+
+    QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffers, next, buf2) {
+        QTAILQ_REMOVE(&port->unflushed_buffers, buf, next);
+        free_buf(buf);
+    }
+}
+
 /* Functions for use inside qemu to open and read from/write to ports */
 int virtio_serial_open(VirtIOSerialPort *port)
 {
@@ -176,6 +262,7 @@ int virtio_serial_close(VirtIOSerialPort *port)
     port->host_connected = false;
     send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
 
+    remove_port_buffers(port);
     return 0;
 }
 
@@ -196,10 +283,6 @@ ssize_t virtio_serial_write(VirtIOSerialPort *port, const uint8_t *buf,
 size_t virtio_serial_guest_ready(VirtIOSerialPort *port)
 {
     VirtQueue *vq = port->ivq;
-    size_t size, header_len;
-
-    header_len = use_multiport(port->vser)
-        ? sizeof(struct virtio_console_header) : 0;
 
     if (!virtio_queue_ready(vq) ||
         !(port->vser->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
@@ -210,13 +293,11 @@ size_t virtio_serial_guest_ready(VirtIOSerialPort *port)
         return 0;
     }
 
-    size = 4096;
-    if (virtqueue_avail_bytes(vq, size, 0)) {
-        return size - header_len;
+    if (virtqueue_avail_bytes(vq, 4096, 0)) {
+        return 4096;
     }
-    size = header_len + 1;
-    if (virtqueue_avail_bytes(vq, size, 0)) {
-        return size - header_len;
+    if (virtqueue_avail_bytes(vq, 1, 0)) {
+        return 1;
     }
     return 0;
 }
@@ -315,6 +396,10 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
 
 /*
  * Guest wrote something to some port.
+ *
+ * Flush the data in the entire chunk that we received rather than
+ * splitting it into multiple buffers. VNC clients don't consume split
+ * buffers
  */
 static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
@@ -325,18 +410,13 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
     while (virtqueue_pop(vq, &elem)) {
         VirtIOSerialPort *port;
-        struct virtio_console_header header;
-        int header_len;
-
-        header_len = use_multiport(vser) ? sizeof(header) : 0;
+        VirtIOSerialPortBuffer *buf;
 
-        if (elem.out_sg[0].iov_len < header_len) {
-            goto next_buf;
-        }
         port = find_port_by_vq(vser, vq);
         if (!port) {
             goto next_buf;
         }
+
         /*
          * A port may not have any handler registered for consuming the
          * data that the guest sends or it may not have a chardev associated
@@ -347,13 +427,24 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
         }
 
         /* The guest always sends only one sg */
-        port->info->have_data(port, elem.out_sg[0].iov_base + header_len,
-                              elem.out_sg[0].iov_len - header_len);
+        buf = alloc_buf(elem.out_sg[0].iov_len);
+        memcpy(buf->buf, elem.out_sg[0].iov_base, buf->len);
+
+        QTAILQ_INSERT_TAIL(&port->unflushed_buffers, buf, next);
+        port->nr_bytes += buf->len;
+        port->has_activity = true;
+
+        if (!port->host_throttled && port->byte_limit &&
+            port->nr_bytes >= port->byte_limit) {
 
+            port->host_throttled = true;
+            send_control_event(port, VIRTIO_CONSOLE_THROTTLE_PORT, 1);
+        }
     next_buf:
         virtqueue_push(vq, &elem, elem.out_sg[0].iov_len);
     }
     virtio_notify(vdev, vq);
+    flush_all_ports(vser);
 }
 
 static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
@@ -388,6 +479,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
     VirtIOSerial *s = opaque;
     VirtIOSerialPort *port;
     uint32_t nr_active_ports;
+    unsigned int nr_bufs;
 
     /* The virtio device */
     virtio_save(&s->vdev, f);
@@ -410,14 +502,34 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
      * Items in struct VirtIOSerialPort.
      */
     QTAILQ_FOREACH(port, &s->ports, next) {
+        VirtIOSerialPortBuffer *buf;
+
         /*
          * We put the port number because we may not have an active
          * port at id 0 that's reserved for a console port, or in case
          * of ports that might have gotten unplugged
          */
         qemu_put_be32s(f, &port->id);
+        qemu_put_be64s(f, &port->byte_limit);
+        qemu_put_be64s(f, &port->nr_bytes);
         qemu_put_byte(f, port->guest_connected);
+        qemu_put_byte(f, port->host_throttled);
 
+        /* All the pending buffers from active ports */
+        nr_bufs = 0;
+        QTAILQ_FOREACH(buf, &port->unflushed_buffers, next) {
+            nr_bufs++;
+        }
+        qemu_put_be32s(f, &nr_bufs);
+        if (!nr_bufs) {
+            continue;
+        }
+
+        QTAILQ_FOREACH(buf, &port->unflushed_buffers, next) {
+            qemu_put_be64s(f, &buf->len);
+            qemu_put_be64s(f, &buf->offset);
+            qemu_put_buffer(f, buf->buf, buf->len);
+        }
     }
 }
 
@@ -450,13 +562,33 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 
     /* Items in struct VirtIOSerialPort */
     for (i = 0; i < nr_active_ports; i++) {
+        VirtIOSerialPortBuffer *buf;
         uint32_t id;
+        unsigned int nr_bufs;
 
         id = qemu_get_be32(f);
         port = find_port_by_id(s, id);
 
+        port->byte_limit = qemu_get_be64(f);
+        port->nr_bytes   = qemu_get_be64(f);
         port->guest_connected = qemu_get_byte(f);
+        port->host_throttled = qemu_get_byte(f);
+
+        /* All the pending buffers from active ports */
+        qemu_get_be32s(f, &nr_bufs);
+        if (!nr_bufs) {
+            continue;
+        }
+        for (; nr_bufs; nr_bufs--) {
+            size_t len;
 
+            qemu_get_be64s(f, &len);
+            buf = alloc_buf(len);
+
+            qemu_get_be64s(f, &buf->offset);
+            qemu_get_buffer(f, buf->buf, buf->len);
+            QTAILQ_INSERT_TAIL(&port->unflushed_buffers, buf, next);
+        }
     }
 
     return 0;
@@ -492,6 +624,10 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
                    indent, "", port->guest_connected);
     monitor_printf(mon, "%*s dev-prop-int: host_connected: %d\n",
                    indent, "", port->host_connected);
+    monitor_printf(mon, "%*s dev-prop-int: host_throttled: %d\n",
+                   indent, "", port->host_throttled);
+    monitor_printf(mon, "%*s dev-prop-int: nr_bytes: %zu\n",
+                   indent, "", port->nr_bytes);
 }
 
 static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
@@ -522,6 +658,7 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
     if (ret) {
         return ret;
     }
+    QTAILQ_INIT(&port->unflushed_buffers);
 
     port->id = plugging_port0 ? 0 : port->vser->config.nr_ports++;
 
@@ -572,6 +709,8 @@ static int virtser_port_qdev_exit(DeviceState *qdev)
     if (port->info->exit)
         port->info->exit(dev);
 
+    remove_port_buffers(port);
+
     return 0;
 }
 
diff --git a/hw/virtio-serial.c b/hw/virtio-serial.c
index 470446b..9bcf461 100644
--- a/hw/virtio-serial.c
+++ b/hw/virtio-serial.c
@@ -68,12 +68,6 @@ static int virtconsole_initfn(VirtIOSerialDevice *dev)
 
     port->is_console = true;
 
-    /*
-     * For console ports, just assume the guest is ready to accept our
-     * data.
-     */
-    port->guest_connected = true;
-
     if (vcon->chr) {
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
                               vcon);
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index b669c7f..ea87b7d 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -45,16 +45,13 @@ struct virtio_console_control {
     uint16_t value;		/* Extra information for the key */
 };
 
-struct virtio_console_header {
-    uint32_t flags;		/* Some message between host and guest */
-};
-
 /* Some events for the internal messages (control packets) */
 #define VIRTIO_CONSOLE_PORT_READY	0
 #define VIRTIO_CONSOLE_CONSOLE_PORT	1
 #define VIRTIO_CONSOLE_RESIZE		2
 #define VIRTIO_CONSOLE_PORT_OPEN	3
 #define VIRTIO_CONSOLE_PORT_NAME	4
+#define VIRTIO_CONSOLE_THROTTLE_PORT	5
 
 /* == In-qemu interface == */
 
@@ -96,6 +93,13 @@ struct VirtIOSerialPort {
     char *name;
 
     /*
+     * This list holds buffers pushed by the guest in case the guest
+     * sent incomplete messages or the host connection was down and
+     * the device requested to cache the data.
+     */
+    QTAILQ_HEAD(, VirtIOSerialPortBuffer) unflushed_buffers;
+
+    /*
      * This id helps identify ports between the guest and the host.
      * The guest sends a "header" with this id with each data packet
      * that it sends and the host can then find out which associated
@@ -103,6 +107,19 @@ struct VirtIOSerialPort {
      */
     uint32_t id;
 
+    /*
+     * Each port can specify the limit on number of bytes that can be
+     * outstanding in the unread buffers. This is to prevent any OOM
+     * situtation if a rogue process on the guest keeps injecting
+     * data.
+     */
+    size_t byte_limit;
+
+    /*
+     * The number of bytes we have queued up in our unread queue
+     */
+    size_t nr_bytes;
+
     /* Identify if this is a port that binds with hvc in the guest */
     uint8_t is_console;
 
@@ -110,6 +127,11 @@ struct VirtIOSerialPort {
     bool guest_connected;
     /* Is this device open for IO on the host? */
     bool host_connected;
+    /* Have we sent a throttle message to the guest? */
+    bool host_throttled;
+
+    /* Did this port get data in the recent handle_output call? */
+    bool has_activity;
 };
 
 struct VirtIOSerialPortInfo {
-- 
1.6.2.5

[Qemu-devel] [PATCH 6/8] virtio-serial-bus: Add ability to hot-unplug ports

[Amit Shah]

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

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 1ec67d2..d045ea5 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -686,6 +686,8 @@ static int virtser_port_qdev_exit(DeviceState *qdev)
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
     VirtIOSerial *vser = port->vser;
 
+    send_control_event(port, VIRTIO_CONSOLE_PORT_REMOVE, 1);
+
     /*
      * Don't decrement nr_ports here; thus we keep a linearly
      * increasing port id. Not utilising an id again saves us a couple
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index ea87b7d..987835d 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -52,6 +52,7 @@ struct virtio_console_control {
 #define VIRTIO_CONSOLE_PORT_OPEN	3
 #define VIRTIO_CONSOLE_PORT_NAME	4
 #define VIRTIO_CONSOLE_THROTTLE_PORT	5
+#define VIRTIO_CONSOLE_PORT_REMOVE	6
 
 /* == In-qemu interface == */
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 7/8] virtio-serial: Add a 'virtserialport' device for generic serial port support

[Amit Shah]

This patch adds generic serial ports over the virtio serial bus.
These ports have a few more options that are not relevant for
virtio console ports: the ability to cache buffers that are
received for a port while it's disconnected, setting of limits
to the bytes that are cached so as to prevent OOM conditions,
etc.

Sample uses for such a device can be obtaining info from the
guest like the file systems used, apps installed, etc. for
offline usage and logged-in users, clipboard copy-paste, etc.
for online usage.

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

diff --git a/hw/virtio-serial.c b/hw/virtio-serial.c
index 9bcf461..173154b 100644
--- a/hw/virtio-serial.c
+++ b/hw/virtio-serial.c
@@ -106,3 +106,38 @@ static void virtconsole_register(void)
     virtio_serial_port_qdev_register(&virtconsole_info);
 }
 device_init(virtconsole_register)
+
+/* Generic Virtio Serial Ports */
+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);
+    }
+    return 0;
+}
+
+static VirtIOSerialPortInfo virtserialport_info = {
+    .qdev.name     = "virtserialport",
+    .qdev.size     = sizeof(VirtConsole),
+    .init          = virtserialport_initfn,
+    .exit          = virtconsole_exitfn,
+    .have_data     = flush_buf,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_CHR("chardev", VirtConsole, chr),
+        DEFINE_PROP_STRING("name", VirtConsole, port.name),
+        DEFINE_PROP_UINT64("byte_limit", VirtConsole, port.byte_limit, 0),
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static void virtserialport_register(void)
+{
+    virtio_serial_port_qdev_register(&virtserialport_info);
+}
+device_init(virtserialport_register)
-- 
1.6.2.5

[Qemu-devel] [PATCH 8/8] Move virtio-serial to Makefile.objs

[Amit Shah]

There's nothing target-dependent in the virtio-serial code so allow it
to be compiled just once for all the targets.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 Makefile.objs   |    2 +-
 Makefile.target |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index 5802d39..77ff7f6 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -127,7 +127,7 @@ user-obj-y += cutils.o cache-utils.o
 
 hw-obj-y =
 hw-obj-y += loader.o
-hw-obj-y += virtio.o
+hw-obj-y += virtio.o virtio-serial.o
 hw-obj-y += fw_cfg.o
 hw-obj-y += watchdog.o
 hw-obj-$(CONFIG_ECC) += ecc.o
diff --git a/Makefile.target b/Makefile.target
index 60df16d..0bf2253 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -172,7 +172,7 @@ ifdef CONFIG_SOFTMMU
 obj-y = vl.o async.o monitor.o pci.o pci_host.o pcie_host.o machine.o gdbstub.o
 # virtio has to be here due to weird dependency between PCI and virtio-net.
 # need to fix this properly
-obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-serial.o virtio-serial-bus.o virtio-pci.o
+obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-pci.o virtio-serial-bus.o
 obj-$(CONFIG_KVM) += kvm.o kvm-all.o
 obj-$(CONFIG_ISA_MMIO) += isa_mmio.o
 LIBS+=-lz
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 0/8] virtio-console: Move to qdev, multiple devices, generic ports

[Anthony Liguori]

On 01/14/2010 07:17 AM, Amit Shah wrote:
> Hello people,
>
> This iteration of the series removes the START and END flags (and
> hence the header associated with each buffer). That's the major change
> since the last submission.
>    

I think the biggest issue remaining is the buffering.

I think this is a pretty fundamental issue to work out since it 
determines the very nature of the transport (stream vs. datagram).

Because you have to put a max buffer size on the transport, I think 
buffering is a really flawed approach provably equivalent to just 
increasing the message size within the transport.  In general, the later 
is a better approach because then the guest is using it's memory vs. 
using host memory.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 0/8] virtio-console: Move to qdev, multiple devices, generic ports

[Amit Shah]

On (Thu) Jan 14 2010 [08:34:42], Anthony Liguori wrote:
> On 01/14/2010 07:17 AM, Amit Shah wrote:
>> Hello people,
>>
>> This iteration of the series removes the START and END flags (and
>> hence the header associated with each buffer). That's the major change
>> since the last submission.
>>    
>
> I think the biggest issue remaining is the buffering.
>
> I think this is a pretty fundamental issue to work out since it  
> determines the very nature of the transport (stream vs. datagram).

The buffering is done so that the guest copy of the buffer is acked so
that the guest can go about doing other things. (Currently the guest
spins till a buffer is acked by the host and waiting for individual
ports to flush their data to whichever receiver will consume some time.)

This also puts buffer management in one place: not all ports will
consume all the data given to them. There's a need to maintain the
buffer contents till the ports consume all the data. This buffer
management can either be done by the individual ports, or it could be
done by the bus code. I prefer doing it in the bus code since the code
will be the same and be in one place instead of each port doing it
separately all around the place.

> Because you have to put a max buffer size on the transport, I think  
> buffering is a really flawed approach provably equivalent to just  
> increasing the message size within the transport.  In general, the later  
> is a better approach because then the guest is using it's memory vs.  
> using host memory.

OK; alternative solutions on how to handle this?

		Amit

Re: [Qemu-devel] [PATCH 0/8] virtio-console: Move to qdev, multiple devices, generic ports

[Anthony Liguori]

On 01/14/2010 09:34 AM, Amit Shah wrote:
> On (Thu) Jan 14 2010 [08:34:42], Anthony Liguori wrote:
>    
>> On 01/14/2010 07:17 AM, Amit Shah wrote:
>>      
>>> Hello people,
>>>
>>> This iteration of the series removes the START and END flags (and
>>> hence the header associated with each buffer). That's the major change
>>> since the last submission.
>>>
>>>        
>> I think the biggest issue remaining is the buffering.
>>
>> I think this is a pretty fundamental issue to work out since it
>> determines the very nature of the transport (stream vs. datagram).
>>      
> The buffering is done so that the guest copy of the buffer is acked so
> that the guest can go about doing other things. (Currently the guest
> spins till a buffer is acked by the host and waiting for individual
> ports to flush their data to whichever receiver will consume some time.)
>    

The guest already gets an ack when data is consumed.  The host adds the 
consumed buffer to the used ring.

> This also puts buffer management in one place: not all ports will
> consume all the data given to them. There's a need to maintain the
> buffer contents till the ports consume all the data. This buffer
> management can either be done by the individual ports, or it could be
> done by the bus code. I prefer doing it in the bus code since the code
> will be the same and be in one place instead of each port doing it
> separately all around the place.
>    

The rings themselves are buffers.  Since each port has a ring, each port 
has independent buffering right now.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 0/8] virtio-console: Move to qdev, multiple devices, generic ports

[Jamie Lokier]

Anthony Liguori wrote:
> I think this is a pretty fundamental issue to work out since it 
> determines the very nature of the transport (stream vs. datagram).

For the record, I don't think there's anything _wrong_ with a datagram
transport.  It would be quite useful sometimes.  But if there is
datagram support, it should be optional, just like you can choose
between SOCK_STREAM and SOCK_DGRAM for AF_UNIX sockets.

Something else occurred to me with the cut buffer application:

What happens if the guest crashes, kexecs or whatever when it's half
way through sending a cut buffer?  A stream protocol will not have a
nice way to recover from that unless there is an additional "out of
band" way to say "I'm starting again".  Does virtio-serial have an
"I'm starting again" which is passed to the host side application?

> Because you have to put a max buffer size on the transport, I think 
> buffering is a really flawed approach provably equivalent to just 
> increasing the message size within the transport.  In general, the later 
> is a better approach because then the guest is using it's memory vs. 
> using host memory.

I agree, using guest memory for the buffer also means there doesn't
have to be an arbitrary limit on the buffer size, or a time limit.  It
can just wait there until it's consumed or the guest decides to restart.

-- Jamie

Re: [Qemu-devel] [PATCH 0/8] virtio-console: Move to qdev, multiple devices, generic ports

[Amit Shah]

On (Tue) Jan 19 2010 [17:59:33], Jamie Lokier wrote:
> 
> What happens if the guest crashes, kexecs or whatever when it's half
> way through sending a cut buffer?  A stream protocol will not have a
> nice way to recover from that unless there is an additional "out of
> band" way to say "I'm starting again".  Does virtio-serial have an
> "I'm starting again" which is passed to the host side application?

That's what I meant when I was talking about the START and END
delimiters. I've removed those for now though. Something like what you
suggest can be added later on.

		Amit