[Qemu-devel] [PATCH 0/9] virtio-serial fixes, ABI updates

[Qemu-devel] [PATCH 0/9] virtio-serial fixes, ABI updates

[Amit Shah]

Hello,

This series fixes a few issues pointed out by Avi and Juan. Avi
pointed out we should do full scatter/gather processing of guest data
even if current (well-behaved) guests don't send multiple iovs per
element.

Juan pointed out a few migration-related bugs.

In handling the migration fixes, I noticed hot-plug/unplug isn't
handled perfectly for the migration case: ports are enumerated and the
port numbering has to be consistent with the guest's numbering. If
there's a mismatch, control messages meant for one port could be
interpreted for another.

To solve this issue, I go back to maintaining a bitmap in the config
space for active ports. Hot-plug and unplug can be added easily via
the config space as a result.

The kernel driver has to be changed as well so that the changes are in
sync with the changes here.

I've tested these patches on my test suite that tests for correctness
and also hot-plug/unplug cases and fixes presented here.

Amit Shah (9):
  virtio-serial-bus: save/load: Ensure target has enough ports
  virtio-serial-bus: save/load: Ensure nr_ports on src and dest are
    same.
  virtio-serial: save/load: Ensure we have hot-plugged ports
    instantiated
  virtio-serial: Handle scatter-gather buffers for control messages
  virtio-serial: Handle scatter/gather input from the guest
  virtio-serial: Remove redundant check for 0-sized write request
  virtio-serial: Update copyright year to 2010
  virtio-serial-bus: Use a bitmap in virtio config space for active
    ports
  virtio-serial-bus: Let the guest know of host connection changes
    after migration

 hw/virtio-console.c    |    4 +-
 hw/virtio-serial-bus.c |  205 ++++++++++++++++++++++++++++++++++++------------
 hw/virtio-serial.h     |    8 +-
 3 files changed, 161 insertions(+), 56 deletions(-)

[Qemu-devel] [PATCH 1/9] virtio-serial-bus: save/load: Ensure target has enough ports

[Amit Shah]

The target could be started with max_nr_ports for a virtio-serial device
lesser than what was available on the source machine. Fail the migration
in such a case.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reported-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio-serial-bus.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 17c1ec1..36985a1 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -374,6 +374,8 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
 
     /* Items in struct VirtIOSerial */
 
+    qemu_put_be32s(f, &s->bus->max_nr_ports);
+
     /* Do this because we might have hot-unplugged some ports */
     nr_active_ports = 0;
     QTAILQ_FOREACH(port, &s->ports, next)
@@ -399,7 +401,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 {
     VirtIOSerial *s = opaque;
     VirtIOSerialPort *port;
-    uint32_t nr_active_ports;
+    uint32_t max_nr_ports, nr_active_ports;
     unsigned int i;
 
     if (version_id > 2) {
@@ -420,6 +422,12 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 
     /* Items in struct VirtIOSerial */
 
+    qemu_get_be32s(f, &max_nr_ports);
+    if (max_nr_ports > s->bus->max_nr_ports) {
+        /* Source could have more ports than us. Fail migration. */
+        return -EINVAL;
+    }
+
     qemu_get_be32s(f, &nr_active_ports);
 
     /* Items in struct VirtIOSerialPort */
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/9] virtio-serial-bus: save/load: Ensure nr_ports on src and dest are same.

[Amit Shah]

The number of ports on the source as well as the destination machines
should match. If they don't, it means some ports that got hotplugged on
the source aren't instantiated on the destination. Or that ports that
were hot-unplugged on the source are created on the destination.

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

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 36985a1..f43d1fc 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -401,7 +401,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 {
     VirtIOSerial *s = opaque;
     VirtIOSerialPort *port;
-    uint32_t max_nr_ports, nr_active_ports;
+    uint32_t max_nr_ports, nr_active_ports, nr_ports;
     unsigned int i;
 
     if (version_id > 2) {
@@ -418,7 +418,21 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     /* 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);
+    nr_ports = qemu_get_be32(f);
+
+    if (nr_ports != s->config.nr_ports) {
+        /*
+         * Source hot-plugged/unplugged ports and we don't have all of
+         * them here.
+         *
+         * Note: This condition cannot check for all hotplug/unplug
+         * events: eg, if one port was hot-plugged and one was
+         * unplugged, the nr_ports remains the same but the port id's
+         * would have changed and we won't catch it here. A later
+         * check for !find_port_by_id() will confirm if this happened.
+         */
+        return -EINVAL;
+    }
 
     /* Items in struct VirtIOSerial */
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/9] virtio-serial: save/load: Ensure we have hot-plugged ports instantiated

[Amit Shah]

If some ports that were hot-plugged on the source are not available on
the destination, fail migration instead of trying to deref a NULL
pointer.

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

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index f43d1fc..830eb75 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -450,6 +450,13 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 
         id = qemu_get_be32(f);
         port = find_port_by_id(s, id);
+        if (!port) {
+            /*
+             * The requested port was hot-plugged on the source but we
+             * don't have it
+             */
+            return -EINVAL;
+        }
 
         port->guest_connected = qemu_get_byte(f);
     }
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/9] virtio-serial: Handle scatter-gather buffers for control messages

[Amit Shah]

Current control messages are small enough to not be split into multiple
buffers but we could run into such a situation in the future or a
malicious guest could cause such a situation.

So handle the entire iov request for control messages.

Also ensure the size of the control request is >= what we expect
otherwise we risk accessing memory that we don't own.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
CC: Avi Kivity <avi@redhat.com>
Reported-by: Avi Kivity <avi@redhat.com>
---
 hw/virtio-serial-bus.c |   43 ++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 830eb75..d5887ab 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -200,7 +200,7 @@ size_t virtio_serial_guest_ready(VirtIOSerialPort *port)
 }
 
 /* Guest wants to notify us of some event */
-static void handle_control_message(VirtIOSerial *vser, void *buf)
+static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
 {
     struct VirtIOSerialPort *port;
     struct virtio_console_control cpkt, *gcpkt;
@@ -208,6 +208,10 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
     size_t buffer_len;
 
     gcpkt = buf;
+    if (len < sizeof(cpkt)) {
+        /* The guest sent an invalid control packet */
+        return;
+    }
     port = find_port_by_id(vser, ldl_p(&gcpkt->id));
     if (!port)
         return;
@@ -281,12 +285,45 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtQueueElement elem;
     VirtIOSerial *vser;
+    uint8_t *buf;
+    size_t len;
 
     vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
 
+    len = 0;
+    buf = NULL;
     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);
+        unsigned int i;
+        size_t cur_len, offset;
+
+        cur_len = 0;
+        for (i = 0; i < elem.out_num; i++) {
+            cur_len += elem.out_sg[i].iov_len;
+        }
+        /*
+         * Allocate a new buf only if we didn't have one previously or
+         * if the size of the buf differs
+         */
+        if (cur_len != len) {
+            if (len) {
+                qemu_free(buf);
+            }
+            buf = qemu_malloc(cur_len);
+        }
+
+        offset = 0;
+        for (i = 0; i < elem.out_num; i++) {
+            memcpy(buf + offset, elem.out_sg[i].iov_base,
+                   elem.out_sg[i].iov_len);
+            offset += elem.out_sg[i].iov_len;
+        }
+        len = cur_len;
+
+        handle_control_message(vser, buf, len);
+        virtqueue_push(vq, &elem, len);
+    }
+    if (len) {
+        qemu_free(buf);
     }
     virtio_notify(vdev, vq);
 }
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/9] virtio-serial: Handle scatter/gather input from the guest

[Amit Shah]

Current guests don't send more than one iov but it can change later.
Ensure we handle that case.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
CC: Avi Kivity <avi@redhat.com>
---
 hw/virtio-serial-bus.c |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index d5887ab..19e8c59 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -338,11 +338,12 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
     while (virtqueue_pop(vq, &elem)) {
         VirtIOSerialPort *port;
-        size_t ret;
+        size_t len;
+        unsigned int i;
 
+        len = 0;
         port = find_port_by_vq(vser, vq);
         if (!port) {
-            ret = 0;
             goto next_buf;
         }
 
@@ -352,16 +353,23 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
          * with it. Just ignore the data in that case.
          */
         if (!port->info->have_data) {
-            ret = 0;
             goto next_buf;
         }
 
-        /* The guest always sends only one sg */
-        ret = port->info->have_data(port, elem.out_sg[0].iov_base,
-                                    elem.out_sg[0].iov_len);
+        for (i = 0; i < elem.out_num; i++) {
+            size_t ret;
+
+            ret = port->info->have_data(port, elem.out_sg[0].iov_base,
+                                        elem.out_sg[0].iov_len);
+            if (ret < elem.out_sg[0].iov_len) {
+                /* We couldn't write the entire iov; stop processing now */
+                break;
+            }
+            len += ret;
+        }
 
     next_buf:
-        virtqueue_push(vq, &elem, ret);
+        virtqueue_push(vq, &elem, len);
     }
     virtio_notify(vdev, vq);
 }
-- 
1.6.2.5

[Qemu-devel] [PATCH 6/9] virtio-serial: Remove redundant check for 0-sized write request

[Amit Shah]

The check for a 0-sized write request to a guest port is not necessary;
the while loop below won't be executed in this case and all will be
fine.

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

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 19e8c59..2603dcd 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -83,9 +83,6 @@ static size_t write_to_port(VirtIOSerialPort *port,
     if (!virtio_queue_ready(vq)) {
         return 0;
     }
-    if (!size) {
-        return 0;
-    }
 
     while (offset < size) {
         int i;
-- 
1.6.2.5

[Qemu-devel] [PATCH 7/9] virtio-serial: Update copyright year to 2010

[Amit Shah]

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

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index bd44ec6..e915491 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -1,7 +1,7 @@
 /*
  * Virtio Console and Generic Serial Port Devices
  *
- * Copyright Red Hat, Inc. 2009
+ * Copyright Red Hat, Inc. 2009, 2010
  *
  * Authors:
  *  Amit Shah <amit.shah@redhat.com>
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 2603dcd..b682b77 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -1,7 +1,7 @@
 /*
  * A bus for connecting virtio serial and console ports
  *
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009, 2010 Red Hat, Inc.
  *
  * Author(s):
  *  Amit Shah <amit.shah@redhat.com>
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index f297b00..632d31b 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -2,7 +2,7 @@
  * Virtio Serial / Console Support
  *
  * Copyright IBM, Corp. 2008
- * Copyright Red Hat, Inc. 2009
+ * Copyright Red Hat, Inc. 2009, 2010
  *
  * Authors:
  *  Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
-- 
1.6.2.5

[Qemu-devel] [PATCH 8/9] virtio-serial-bus: Use a bitmap in virtio config space for active ports

[Amit Shah]

Allow the port 'id's to be set by a user on the command line. This is
needed by management apps that will want a stable port numbering scheme
for hot-plug/unplug and migration.

Since the port numbers are shared with the guest (to identify ports in
control messages), the config space now maintains a bitmap for active
ports instead of a number indicating active ports.

This also means we can just update the config for port hot-plug and
hot-unplug.

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

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index e915491..bbbb6b8 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -99,6 +99,7 @@ static VirtIOSerialPortInfo virtconsole_info = {
     .exit          = virtconsole_exitfn,
     .qdev.props = (Property[]) {
         DEFINE_PROP_UINT8("is_console", VirtConsole, port.is_console, 1),
+        DEFINE_PROP_UINT32("nr", VirtConsole, port.id, VIRTIO_CONSOLE_BAD_ID),
         DEFINE_PROP_CHR("chardev", VirtConsole, chr),
         DEFINE_PROP_STRING("name", VirtConsole, port.name),
         DEFINE_PROP_END_OF_LIST(),
@@ -133,6 +134,7 @@ static VirtIOSerialPortInfo virtserialport_info = {
     .init          = virtserialport_initfn,
     .exit          = virtconsole_exitfn,
     .qdev.props = (Property[]) {
+        DEFINE_PROP_UINT32("nr", VirtConsole, port.id, VIRTIO_CONSOLE_BAD_ID),
         DEFINE_PROP_CHR("chardev", VirtConsole, chr),
         DEFINE_PROP_STRING("name", VirtConsole, port.name),
         DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index b682b77..2242edf 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -48,6 +48,10 @@ static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
 {
     VirtIOSerialPort *port;
 
+    if (id == VIRTIO_CONSOLE_BAD_ID) {
+        return NULL;
+    }
+
     QTAILQ_FOREACH(port, &vser->ports, next) {
         if (port->id == id)
             return port;
@@ -412,13 +416,13 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
     /* 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);
 
-    /* Items in struct VirtIOSerial */
+    qemu_put_be32s(f, &s->config.max_nr_ports);
+    qemu_put_buffer(f, (uint8_t *)s->config.ports_map,
+                    sizeof(uint32_t) * (s->config.max_nr_ports + 31) / 32);
 
-    qemu_put_be32s(f, &s->bus->max_nr_ports);
+    /* Ports */
 
-    /* Do this because we might have hot-unplugged some ports */
     nr_active_ports = 0;
     QTAILQ_FOREACH(port, &s->ports, next)
         nr_active_ports++;
@@ -429,11 +433,6 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
      * 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);
     }
@@ -443,7 +442,8 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 {
     VirtIOSerial *s = opaque;
     VirtIOSerialPort *port;
-    uint32_t max_nr_ports, nr_active_ports, nr_ports;
+    size_t ports_map_size;
+    uint32_t max_nr_ports, nr_active_ports, *ports_map;
     unsigned int i;
 
     if (version_id > 2) {
@@ -460,29 +460,28 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     /* The config space */
     qemu_get_be16s(f, &s->config.cols);
     qemu_get_be16s(f, &s->config.rows);
-    nr_ports = qemu_get_be32(f);
 
-    if (nr_ports != s->config.nr_ports) {
-        /*
-         * Source hot-plugged/unplugged ports and we don't have all of
-         * them here.
-         *
-         * Note: This condition cannot check for all hotplug/unplug
-         * events: eg, if one port was hot-plugged and one was
-         * unplugged, the nr_ports remains the same but the port id's
-         * would have changed and we won't catch it here. A later
-         * check for !find_port_by_id() will confirm if this happened.
-         */
+    qemu_get_be32s(f, &max_nr_ports);
+    if (max_nr_ports > s->config.max_nr_ports) {
+        /* Source could have had more ports than us. Fail migration. */
         return -EINVAL;
     }
 
-    /* Items in struct VirtIOSerial */
+    ports_map_size = sizeof(uint32_t) * (max_nr_ports + 31) / 32;
+    ports_map = qemu_malloc(ports_map_size);
+    qemu_get_buffer(f, (uint8_t *)ports_map, ports_map_size);
 
-    qemu_get_be32s(f, &max_nr_ports);
-    if (max_nr_ports > s->bus->max_nr_ports) {
-        /* Source could have more ports than us. Fail migration. */
-        return -EINVAL;
+    for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
+        if (ports_map[i] != s->config.ports_map[i]) {
+            /*
+             * Ports active on source and destination don't
+             * match. Fail migration.
+             */
+            qemu_free(ports_map);
+            return -EINVAL;
+        }
     }
+    qemu_free(ports_map);
 
     qemu_get_be32s(f, &nr_active_ports);
 
@@ -492,20 +491,11 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 
         id = qemu_get_be32(f);
         port = find_port_by_id(s, id);
-        if (!port) {
-            /*
-             * The requested port was hot-plugged on the source but we
-             * don't have it
-             */
-            return -EINVAL;
-        }
 
         port->guest_connected = qemu_get_byte(f);
     }
-
     return 0;
 }
-
 static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
 
 static struct BusInfo virtser_bus_info = {
@@ -537,6 +527,39 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
                    indent, "", port->host_connected);
 }
 
+/* This function is only used if a port id is not provided by the user */
+static uint32_t find_free_port_id(VirtIOSerial *vser)
+{
+    unsigned int i;
+
+    for (i = 0; i < (vser->config.max_nr_ports + 31) / 32; i++) {
+        uint32_t map, bit;
+
+        map = vser->config.ports_map[i];
+        bit = ffs(~map);
+        if (bit) {
+            return (bit - 1) + i * 32;
+        }
+    }
+    return VIRTIO_CONSOLE_BAD_ID;
+}
+
+static void mark_port_added(VirtIOSerial *vser, uint32_t port_id)
+{
+    unsigned int i;
+
+    i = port_id / 32;
+    vser->config.ports_map[i] |= 1U << (port_id % 32);
+}
+
+static void mark_port_removed(VirtIOSerial *vser, uint32_t port_id)
+{
+    unsigned int i;
+
+    i = port_id / 32;
+    vser->config.ports_map[i] &= ~(1U << (port_id % 32));
+}
+
 static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
 {
     VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev);
@@ -555,19 +578,30 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
      */
     plugging_port0 = port->is_console && !find_port_by_id(port->vser, 0);
 
-    if (port->vser->config.nr_ports == bus->max_nr_ports && !plugging_port0) {
-        error_report("virtio-serial-bus: Maximum device limit reached");
+    if (find_port_by_id(port->vser, port->id)) {
+        error_report("virtio-serial-bus: A port already exists at id %u\n",
+                     port->id);
         return -1;
     }
-    dev->info = info;
 
+    if (plugging_port0) {
+        port->id = 0;
+    }
+
+    if (port->id == VIRTIO_CONSOLE_BAD_ID) {
+        port->id = find_free_port_id(port->vser);
+        if (port->id == VIRTIO_CONSOLE_BAD_ID) {
+            error_report("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++;
-
     if (!use_multiport(port->vser)) {
         /*
          * Allow writes to guest in this case; we have no way of
@@ -580,6 +614,8 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
     port->ivq = port->vser->ivqs[port->id];
     port->ovq = port->vser->ovqs[port->id];
 
+    mark_port_added(port->vser, port->id);
+
     /* Send an update to the guest about this new port added */
     virtio_notify_config(&port->vser->vdev);
 
@@ -592,26 +628,9 @@ 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);
+    mark_port_removed(port->vser, port->id);
+    virtio_notify_config(&port->vser->vdev);
 
-    /*
-     * 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)
@@ -675,7 +694,7 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports)
      * Reserve location 0 for a console port for backward compat
      * (old kernel, new qemu)
      */
-    vser->config.nr_ports = 1;
+    mark_port_added(vser, 0);
 
     vser->vdev.get_features = get_features;
     vser->vdev.get_config = get_config;
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index 632d31b..5f1d458 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -27,6 +27,9 @@
 /* Features supported */
 #define VIRTIO_CONSOLE_F_MULTIPORT	1
 
+#define VIRTIO_CONSOLE_BAD_ID           (~(uint32_t)0)
+#define MAX_VIRTIO_CONSOLE_PORTS	((VIRTIO_PCI_QUEUE_MAX / 2) - 1)
+
 struct virtio_console_config {
     /*
      * These two fields are used by VIRTIO_CONSOLE_F_SIZE which
@@ -36,7 +39,7 @@ struct virtio_console_config {
     uint16_t rows;
 
     uint32_t max_nr_ports;
-    uint32_t nr_ports;
+    uint32_t ports_map[(MAX_VIRTIO_CONSOLE_PORTS + 31) / 32];
 } __attribute__((packed));
 
 struct virtio_console_control {
@@ -51,7 +54,6 @@ struct virtio_console_control {
 #define VIRTIO_CONSOLE_RESIZE		2
 #define VIRTIO_CONSOLE_PORT_OPEN	3
 #define VIRTIO_CONSOLE_PORT_NAME	4
-#define VIRTIO_CONSOLE_PORT_REMOVE	5
 
 /* == In-qemu interface == */
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 9/9] virtio-serial-bus: Let the guest know of host connection changes after migration

[Amit Shah]

If the host connection to a port is closed on the destination machine
after migration, when the connection was open on the source, the host
has to be informed of that.

Similar for a host connection open on the destination.

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

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 2242edf..f5ea37e 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -435,6 +435,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
     QTAILQ_FOREACH(port, &s->ports, next) {
         qemu_put_be32s(f, &port->id);
         qemu_put_byte(f, port->guest_connected);
+        qemu_put_byte(f, port->host_connected);
     }
 }
 
@@ -488,11 +489,21 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     /* Items in struct VirtIOSerialPort */
     for (i = 0; i < nr_active_ports; i++) {
         uint32_t id;
+        bool host_connected;
 
         id = qemu_get_be32(f);
         port = find_port_by_id(s, id);
 
         port->guest_connected = qemu_get_byte(f);
+        host_connected = qemu_get_byte(f);
+        if (host_connected != port->host_connected) {
+            /*
+             * We have to let the guest know of the host connection
+             * status change
+             */
+            send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
+                               port->host_connected);
+        }
     }
     return 0;
 }
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH 4/9] virtio-serial: Handle scatter-gather buffers for control messages

[Avi Kivity]

On 03/19/2010 01:58 PM, Amit Shah wrote:
> Current control messages are small enough to not be split into multiple
> buffers but we could run into such a situation in the future or a
> malicious guest could cause such a situation.
>
> So handle the entire iov request for control messages.
>
> Also ensure the size of the control request is>= what we expect
> otherwise we risk accessing memory that we don't own.
>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
> CC: Avi Kivity<avi@redhat.com>
> Reported-by: Avi Kivity<avi@redhat.com>
> ---
>   hw/virtio-serial-bus.c |   43 ++++++++++++++++++++++++++++++++++++++++---
>   1 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 830eb75..d5887ab 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -200,7 +200,7 @@ size_t virtio_serial_guest_ready(VirtIOSerialPort *port)
>   }
>
>   /* Guest wants to notify us of some event */
> -static void handle_control_message(VirtIOSerial *vser, void *buf)
> +static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
>   {
>       struct VirtIOSerialPort *port;
>       struct virtio_console_control cpkt, *gcpkt;
> @@ -208,6 +208,10 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
>       size_t buffer_len;
>
>       gcpkt = buf;
> +    if (len<  sizeof(cpkt)) {
> +        /* The guest sent an invalid control packet */
> +        return;
> +    }
>       port = find_port_by_id(vser, ldl_p(&gcpkt->id));
>       if (!port)
>           return;
> @@ -281,12 +285,45 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
>   {
>       VirtQueueElement elem;
>       VirtIOSerial *vser;
> +    uint8_t *buf;
> +    size_t len;
>
>       vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
>
> +    len = 0;
> +    buf = NULL;
>       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);
> +        unsigned int i;
> +        size_t cur_len, offset;
> +
> +        cur_len = 0;
> +        for (i = 0; i<  elem.out_num; i++) {
> +            cur_len += elem.out_sg[i].iov_len;
> +        }
> +        /*
> +         * Allocate a new buf only if we didn't have one previously or
> +         * if the size of the buf differs
> +         */
> +        if (cur_len != len) {
> +            if (len) {
> +                qemu_free(buf);
> +            }
> +            buf = qemu_malloc(cur_len);
> +        }
> +
> +        offset = 0;
> +        for (i = 0; i<  elem.out_num; i++) {
> +            memcpy(buf + offset, elem.out_sg[i].iov_base,
> +                   elem.out_sg[i].iov_len);
> +            offset += elem.out_sg[i].iov_len;
> +        }
> +        len = cur_len;
> +
> +        handle_control_message(vser, buf, len);
> +        virtqueue_push(vq,&elem, len);
> +    }
> +    if (len) {
> +        qemu_free(buf);
>       }
>       virtio_notify(vdev, vq);
>   }
>    

Isn't there some virtio function to linearize requests?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] Re: [PATCH 4/9] virtio-serial: Handle scatter-gather buffers for control messages

[Amit Shah]

On (Sat) Mar 20 2010 [09:40:50], Avi Kivity wrote:
> On 03/19/2010 01:58 PM, Amit Shah wrote:
>> +
>> +        offset = 0;
>> +        for (i = 0; i<  elem.out_num; i++) {
>> +            memcpy(buf + offset, elem.out_sg[i].iov_base,
>> +                   elem.out_sg[i].iov_len);
>> +            offset += elem.out_sg[i].iov_len;
>> +        }
>> +        len = cur_len;
>> +
>> +        handle_control_message(vser, buf, len);
>> +        virtqueue_push(vq,&elem, len);
>> +    }
>> +    if (len) {
>> +        qemu_free(buf);
>>       }
>>       virtio_notify(vdev, vq);
>>   }
>
> Isn't there some virtio function to linearize requests?

I don't see one.

		Amit

Re: [Qemu-devel] Re: [PATCH 4/9] virtio-serial: Handle scatter-gather buffers for control messages

[Michael S. Tsirkin]

On Mon, Mar 22, 2010 at 10:48:02AM +0530, Amit Shah wrote:
> On (Sat) Mar 20 2010 [09:40:50], Avi Kivity wrote:
> > On 03/19/2010 01:58 PM, Amit Shah wrote:
> >> +
> >> +        offset = 0;
> >> +        for (i = 0; i<  elem.out_num; i++) {
> >> +            memcpy(buf + offset, elem.out_sg[i].iov_base,
> >> +                   elem.out_sg[i].iov_len);
> >> +            offset += elem.out_sg[i].iov_len;
> >> +        }
> >> +        len = cur_len;
> >> +
> >> +        handle_control_message(vser, buf, len);
> >> +        virtqueue_push(vq,&elem, len);
> >> +    }
> >> +    if (len) {
> >> +        qemu_free(buf);
> >>       }
> >>       virtio_notify(vdev, vq);
> >>   }
> >
> > Isn't there some virtio function to linearize requests?
> 
> I don't see one.
> 
> 		Amit

virtio-net has iov_fill. Reuse it?

-- 
MST

Re: [Qemu-devel] Re: [PATCH 4/9] virtio-serial: Handle scatter-gather buffers for control messages

[Amit Shah]

On (Tue) Mar 23 2010 [17:51:26], Michael S. Tsirkin wrote:
> On Mon, Mar 22, 2010 at 10:48:02AM +0530, Amit Shah wrote:
> > On (Sat) Mar 20 2010 [09:40:50], Avi Kivity wrote:
> > > On 03/19/2010 01:58 PM, Amit Shah wrote:
> > >> +
> > >> +        offset = 0;
> > >> +        for (i = 0; i<  elem.out_num; i++) {
> > >> +            memcpy(buf + offset, elem.out_sg[i].iov_base,
> > >> +                   elem.out_sg[i].iov_len);
> > >> +            offset += elem.out_sg[i].iov_len;
> > >> +        }
> > >> +        len = cur_len;
> > >> +
> > >> +        handle_control_message(vser, buf, len);
> > >> +        virtqueue_push(vq,&elem, len);
> > >> +    }
> > >> +    if (len) {
> > >> +        qemu_free(buf);
> > >>       }
> > >>       virtio_notify(vdev, vq);
> > >>   }
> > >
> > > Isn't there some virtio function to linearize requests?
> > 
> > I don't see one.
> 
> virtio-net has iov_fill. Reuse it?

Hm, yeah. Any ideas on how to share it? Put it in some common file?

Just copying it seems good for now..

		Amit

Re: [Qemu-devel] Re: [PATCH 4/9] virtio-serial: Handle scatter-gather buffers for control messages

[Michael S. Tsirkin]

On Tue, Mar 23, 2010 at 09:45:08PM +0530, Amit Shah wrote:
> On (Tue) Mar 23 2010 [17:51:26], Michael S. Tsirkin wrote:
> > On Mon, Mar 22, 2010 at 10:48:02AM +0530, Amit Shah wrote:
> > > On (Sat) Mar 20 2010 [09:40:50], Avi Kivity wrote:
> > > > On 03/19/2010 01:58 PM, Amit Shah wrote:
> > > >> +
> > > >> +        offset = 0;
> > > >> +        for (i = 0; i<  elem.out_num; i++) {
> > > >> +            memcpy(buf + offset, elem.out_sg[i].iov_base,
> > > >> +                   elem.out_sg[i].iov_len);
> > > >> +            offset += elem.out_sg[i].iov_len;
> > > >> +        }
> > > >> +        len = cur_len;
> > > >> +
> > > >> +        handle_control_message(vser, buf, len);
> > > >> +        virtqueue_push(vq,&elem, len);
> > > >> +    }
> > > >> +    if (len) {
> > > >> +        qemu_free(buf);
> > > >>       }
> > > >>       virtio_notify(vdev, vq);
> > > >>   }
> > > >
> > > > Isn't there some virtio function to linearize requests?
> > > 
> > > I don't see one.
> > 
> > virtio-net has iov_fill. Reuse it?
> 
> Hm, yeah. Any ideas on how to share it? Put it in some common file?
> 
> Just copying it seems good for now..
> 
> 		Amit


Add iov.c

-- 
MST

[Qemu-devel] Re: [PATCH 0/9] virtio-serial fixes, ABI updates

[Michael S. Tsirkin]

On Fri, Mar 19, 2010 at 05:28:37PM +0530, Amit Shah wrote:
> Hello,
> 
> This series fixes a few issues pointed out by Avi and Juan. Avi
> pointed out we should do full scatter/gather processing of guest data
> even if current (well-behaved) guests don't send multiple iovs per
> element.
> 
> Juan pointed out a few migration-related bugs.
> 
> In handling the migration fixes, I noticed hot-plug/unplug isn't
> handled perfectly for the migration case: ports are enumerated and the
> port numbering has to be consistent with the guest's numbering. If
> there's a mismatch, control messages meant for one port could be
> interpreted for another.

BTW, I think virtio serial migration code needs to be fixed
to be backwards compatible with old qemu if multiport
feature is off.

> To solve this issue, I go back to maintaining a bitmap in the config
> space for active ports. Hot-plug and unplug can be added easily via
> the config space as a result.

As I commented on the kernel driver, I'm not sure this is a good choice.

> The kernel driver has to be changed as well so that the changes are in
> sync with the changes here.
> 
> I've tested these patches on my test suite that tests for correctness
> and also hot-plug/unplug cases and fixes presented here.
> 
> Amit Shah (9):
>   virtio-serial-bus: save/load: Ensure target has enough ports
>   virtio-serial-bus: save/load: Ensure nr_ports on src and dest are
>     same.
>   virtio-serial: save/load: Ensure we have hot-plugged ports
>     instantiated
>   virtio-serial: Handle scatter-gather buffers for control messages
>   virtio-serial: Handle scatter/gather input from the guest
>   virtio-serial: Remove redundant check for 0-sized write request
>   virtio-serial: Update copyright year to 2010
>   virtio-serial-bus: Use a bitmap in virtio config space for active
>     ports
>   virtio-serial-bus: Let the guest know of host connection changes
>     after migration
> 
>  hw/virtio-console.c    |    4 +-
>  hw/virtio-serial-bus.c |  205 ++++++++++++++++++++++++++++++++++++------------
>  hw/virtio-serial.h     |    8 +-
>  3 files changed, 161 insertions(+), 56 deletions(-)

[Qemu-devel] Re: [PATCH 0/9] virtio-serial fixes, ABI updates

[Amit Shah]

On (Sun) Mar 21 2010 [15:47:53], Michael S. Tsirkin wrote:
> On Fri, Mar 19, 2010 at 05:28:37PM +0530, Amit Shah wrote:
> > Hello,
> > 
> > This series fixes a few issues pointed out by Avi and Juan. Avi
> > pointed out we should do full scatter/gather processing of guest data
> > even if current (well-behaved) guests don't send multiple iovs per
> > element.
> > 
> > Juan pointed out a few migration-related bugs.
> > 
> > In handling the migration fixes, I noticed hot-plug/unplug isn't
> > handled perfectly for the migration case: ports are enumerated and the
> > port numbering has to be consistent with the guest's numbering. If
> > there's a mismatch, control messages meant for one port could be
> > interpreted for another.
> 
> BTW, I think virtio serial migration code needs to be fixed
> to be backwards compatible with old qemu if multiport
> feature is off.

This is already done; the savevm version number is bumped up.

> > To solve this issue, I go back to maintaining a bitmap in the config
> > space for active ports. Hot-plug and unplug can be added easily via
> > the config space as a result.
> 
> As I commented on the kernel driver, I'm not sure this is a good choice.

I've replied to that comment; please let me know if it's a concern.
Also, what do others think?

		Amit