[Qemu-devel] [PATCH 00/15] v3: virtio-serial-bus fixes, new abi for port discovery

[Qemu-devel] [PATCH 00/15] v3: virtio-serial-bus fixes, new abi for port discovery

[Amit Shah]

Hello,

These patches rework the way ports are announced to the guests. A
control message is used to let the guest know a new port is
added. Initial port discovery and port hot-plug work via this way now.

This was done to have the host and guest port numbering in sync to
avoid surprises after several hotplug/unplug operations and
migrations.

The ability to assign a particular port number to ports is also added
so that management software can control the placement of ports.

Differences from v3:
- Add iov.[ch] and move memcpy_to_iovec from virtio-balloon.c and two
  fill_iov implementations from virtio-net.c and virtio-serial-bus.c
  there.

- New fix for not sending data to port if host connection is not open

- Rearranged patches: migration fixes first, some trivial ones next,
  followed by fixes and scatter/gather handling last.

- Plug a console port at id 0 only if an id is not supplied by user

- Let user know if a port or device failed to initialise on the guest

- Add QMP events to let mgmt know port or device addition in the guest
  failed.

Overall:
- Users can set the port id they wish to instantiate ports at by using
  the ,nr= parameter to 'virtserialport' and 'virtconsole' devices

- Migration fixes: refuse migration when:
  - number of active ports is different between the src and destination
  - max_nr_ports a device can support on the src is more

- If a qemu chardev connection to a port is closed on the dest while
  it was open on the src, inform the guest about this. (Also do the
  same for port closed on src but open on dest.)

- Use control messages for relaying new port information instead of
  config space (changes abi)

- Propagate error message from guest in instantiating a port or a
  device to the user as well as mgmt via QMP messages.

- Handle scatter/gather for control output and data output from the
  guest


Amit Shah (15):
  virtio-serial: save/load: Ensure target has enough ports
  virtio-serial: save/load: Ensure nr_ports on src and dest are same.
  virtio-serial: save/load: Ensure we have hot-plugged ports
    instantiated
  virtio-serial: save/load: Send target host connection status if
    different
  virtio-serial: Use control messages to notify guest of new ports
  virtio-serial: whitespace: match surrounding code
  virtio-serial: Remove redundant check for 0-sized write request
  virtio-serial: Update copyright year to 2010
  virtio-serial: Propagate errors in initialising ports / devices in
    guest
  virtio-serial: Add QMP events for failed port/device add
  virtio-serial: Send out guest data to ports only if port is opened
  iov: Introduce a new file for helpers around iovs, add iov_from_buf()
  iov: Add iov_to_buf and iov_size helpers
  virtio-serial: Handle scatter-gather buffers for control messages
  virtio-serial: Handle scatter/gather input from the guest

 Makefile.objs          |    1 +
 QMP/qmp-events.txt     |   48 +++++++++
 hw/iov.c               |   70 ++++++++++++
 hw/iov.h               |   19 ++++
 hw/virtio-balloon.c    |   35 +------
 hw/virtio-console.c    |    4 +-
 hw/virtio-net.c        |   20 +---
 hw/virtio-serial-bus.c |  277 +++++++++++++++++++++++++++++++++++++-----------
 hw/virtio-serial.h     |   19 ++--
 monitor.c              |    3 +
 monitor.h              |    1 +
 11 files changed, 380 insertions(+), 117 deletions(-)
 create mode 100644 hw/iov.c
 create mode 100644 hw/iov.h

[Qemu-devel] [PATCH 01/15] virtio-serial: 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 |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 17c1ec1..9a7f0c1 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -374,10 +374,13 @@ 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)
+    QTAILQ_FOREACH(port, &s->ports, next) {
         nr_active_ports++;
+    }
 
     qemu_put_be32s(f, &nr_active_ports);
 
@@ -399,7 +402,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 +423,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 02/15] virtio-serial: 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 9a7f0c1..d31e62d 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -402,7 +402,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) {
@@ -419,7 +419,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 03/15] 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 d31e62d..5316ef6 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -451,6 +451,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 04/15] virtio-serial: save/load: Send target host connection status if different

[Amit Shah]

If the host connection to a port is closed on the destination machine
after migration, whereas the connection was open on the source, the
guest 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 5316ef6..484dc94 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -395,6 +395,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
          */
         qemu_put_be32s(f, &port->id);
         qemu_put_byte(f, port->guest_connected);
+        qemu_put_byte(f, port->host_connected);
     }
 }
 
@@ -448,6 +449,7 @@ 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);
@@ -460,6 +462,15 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_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] [PATCH 05/15] virtio-serial: Use control messages to notify guest of new 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), we just send a control message to the guest
indicating addition of new ports (hot-plug) or notifying the guest of
the available ports when the guest sends us a DEVICE_READY control
message.

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

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index bd44ec6..17b221d 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 484dc94..00e8616 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -41,6 +41,10 @@ struct VirtIOSerial {
     VirtIOSerialBus *bus;
 
     QTAILQ_HEAD(, VirtIOSerialPort) ports;
+
+    /* bitmap for identifying active ports */
+    uint32_t *ports_map;
+
     struct virtio_console_config config;
 };
 
@@ -48,6 +52,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;
@@ -208,14 +216,25 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
     size_t buffer_len;
 
     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);
 
+    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
+    if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
+        return;
+
     switch(cpkt.event) {
+    case VIRTIO_CONSOLE_DEVICE_READY:
+        /*
+         * The device is up, we can now tell the device about all the
+         * ports we have here.
+         */
+        QTAILQ_FOREACH(port, &vser->ports, next) {
+            send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1);
+        }
+        break;
+
     case VIRTIO_CONSOLE_PORT_READY:
         /*
          * Now that we know the guest asked for the port name, we're
@@ -370,13 +389,16 @@ 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);
+
+    /* The ports map */
 
-    qemu_put_be32s(f, &s->bus->max_nr_ports);
+    qemu_put_buffer(f, (uint8_t *)s->ports_map,
+                    sizeof(uint32_t) * (s->config.max_nr_ports + 31) / 32);
+
+    /* Ports */
 
-    /* Do this because we might have hot-unplugged some ports */
     nr_active_ports = 0;
     QTAILQ_FOREACH(port, &s->ports, next) {
         nr_active_ports++;
@@ -388,11 +410,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);
         qemu_put_byte(f, port->host_connected);
@@ -403,7 +420,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) {
@@ -420,29 +438,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->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);
 
@@ -453,13 +470,6 @@ 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);
         host_connected = qemu_get_byte(f);
@@ -472,7 +482,6 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
                                port->host_connected);
         }
     }
-
     return 0;
 }
 
@@ -507,6 +516,50 @@ 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->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->ports_map[i] |= 1U << (port_id % 32);
+}
+
+static void add_port(VirtIOSerial *vser, uint32_t port_id)
+{
+    mark_port_added(vser, port_id);
+
+    send_control_event(find_port_by_id(vser, port_id),
+                       VIRTIO_CONSOLE_PORT_ADD, 1);
+}
+
+static void remove_port(VirtIOSerial *vser, uint32_t port_id)
+{
+    unsigned int i;
+
+    i = port_id / 32;
+    vser->ports_map[i] &= ~(1U << (port_id % 32));
+
+    send_control_event(find_port_by_id(vser, port_id),
+                       VIRTIO_CONSOLE_PORT_REMOVE, 1);
+}
+
 static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
 {
     VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev);
@@ -525,19 +578,36 @@ 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 (port->id == VIRTIO_CONSOLE_BAD_ID) {
+        if (plugging_port0) {
+            port->id = 0;
+        } else {
+            port->id = find_free_port_id(port->vser);
+            if (port->id == VIRTIO_CONSOLE_BAD_ID) {
+                error_report("virtio-serial-bus: Maximum port limit for this device reached\n");
+                return -1;
+            }
+        }
+    }
+
+    if (port->id >= port->vser->config.max_nr_ports) {
+        error_report("virtio-serial-bus: Out-of-range port id specified, max. allowed: %u\n",
+                     port->vser->config.max_nr_ports - 1);
+        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
@@ -550,6 +620,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];
 
+    add_port(port->vser, port->id);
+
     /* Send an update to the guest about this new port added */
     virtio_notify_config(&port->vser->vdev);
 
@@ -562,26 +634,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);
+    remove_port(port->vser, port->id);
 
-    /*
-     * 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)
@@ -641,11 +695,12 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports)
     }
 
     vser->config.max_nr_ports = max_nr_ports;
+    vser->ports_map = qemu_mallocz((max_nr_ports + 31) / 32);
     /*
      * 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 f297b00..0548689 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -27,6 +27,8 @@
 /* Features supported */
 #define VIRTIO_CONSOLE_F_MULTIPORT	1
 
+#define VIRTIO_CONSOLE_BAD_ID           (~(uint32_t)0)
+
 struct virtio_console_config {
     /*
      * These two fields are used by VIRTIO_CONSOLE_F_SIZE which
@@ -36,7 +38,6 @@ struct virtio_console_config {
     uint16_t rows;
 
     uint32_t max_nr_ports;
-    uint32_t nr_ports;
 } __attribute__((packed));
 
 struct virtio_console_control {
@@ -46,12 +47,14 @@ struct virtio_console_control {
 };
 
 /* 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_PORT_REMOVE	5
+#define VIRTIO_CONSOLE_DEVICE_READY	0
+#define VIRTIO_CONSOLE_PORT_ADD		1
+#define VIRTIO_CONSOLE_PORT_REMOVE	2
+#define VIRTIO_CONSOLE_PORT_READY	3
+#define VIRTIO_CONSOLE_CONSOLE_PORT	4
+#define VIRTIO_CONSOLE_RESIZE		5
+#define VIRTIO_CONSOLE_PORT_OPEN	6
+#define VIRTIO_CONSOLE_PORT_NAME	7
 
 /* == In-qemu interface == */
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 06/15] virtio-serial: whitespace: match surrounding code

[Amit Shah]

The virtio-serial code doesn't mix declarations and definitions, so
separate them out on different lines.

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

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 00e8616..80f0259 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -354,7 +354,10 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
 
 static uint32_t get_features(VirtIODevice *vdev, uint32_t features)
 {
-    VirtIOSerial *vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
+    VirtIOSerial *vser;
+
+    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
+
     if (vser->bus->max_nr_ports > 1) {
         features |= (1 << VIRTIO_CONSOLE_F_MULTIPORT);
     }
-- 
1.6.2.5

[Qemu-devel] [PATCH 07/15] 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 80f0259..4435c62 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -91,9 +91,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 08/15] 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 17b221d..bbbb6b8 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 4435c62..a19e751 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 0548689..f023873 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 09/15] virtio-serial: Propagate errors in initialising ports / devices in guest

[Amit Shah]

If adding of ports or devices in the guest fails we can send out a QMP
event so that management software can deal with it.

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

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index a19e751..33083af 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -223,6 +223,11 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
 
     switch(cpkt.event) {
     case VIRTIO_CONSOLE_DEVICE_READY:
+        if (!cpkt.value) {
+            error_report("virtio-serial-bus: Guest failure in adding device %s\n",
+                vser->bus->qbus.name);
+            break;
+        }
         /*
          * The device is up, we can now tell the device about all the
          * ports we have here.
@@ -233,6 +238,11 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
         break;
 
     case VIRTIO_CONSOLE_PORT_READY:
+        if (!cpkt.value) {
+            error_report("virtio-serial-bus: Guest failure in adding port %u for device %s\n",
+                         port->id, vser->bus->qbus.name);
+            break;
+        }
         /*
          * Now that we know the guest asked for the port name, we're
          * sure the guest has initialised whatever state is necessary
-- 
1.6.2.5

[Qemu-devel] [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Amit Shah]

When adding a port or a device to the guest fails, management software
might be interested in knowing and then cleaning up the host-side of the
port. Introduce QMP events to signal such errors.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
CC: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-events.txt     |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-serial-bus.c |   15 +++++++++++++++
 monitor.c              |    3 +++
 monitor.h              |    1 +
 4 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index a94e9b4..f13cf45 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -188,3 +188,51 @@ Example:
 
 Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
 followed respectively by the RESET, SHUTDOWN, or STOP events.
+
+VIRTIO_SERIAL
+-------------
+
+Emitted when errors occur in guest port add or guest device add.
+
+Data:
+
+- "device": The virtio-serial device that triggered the event {json-string}
+      This is the name given to the bus on the command line:
+        -device virtio-serial,id="foo"
+      here, the device name is "foo"
+
+- "port": The port number that triggered the event {json-number}
+      This is the number given to the port on the command line:
+        -device virtserialport,nr=2
+      here, the port number is 2. If not mentioned on the command
+      line, the number is auto-assigned.
+
+- "result": The result of the operation {json-string}
+      This is one of the following:
+         "pass", "fail"
+
+- "operation": The operation that triggered the event {json-sring}
+      This is one of the following:
+         "port_add", "device_add"
+
+Example:
+
+Port 0 add failure in the guest:
+
+{ "timestamp": {"seconds": 1269438649, "microseconds": 851170},
+  "event": "VIRTIO_SERIAL",
+  "data": {
+            "device": "virtio-serial-bus.0",
+            "port": 0,
+            "result": "fail",
+            "operation": "port_add" } }
+
+Device add failure in the guest:
+
+{ "timestamp": {"seconds": 1269438702, "microseconds": 78114},
+  "event": "VIRTIO_SERIAL",
+  "data": {
+            "device": "virtio-serial-bus.0",
+            "port": 4294967295,
+            "result": "fail",
+            "operation": "device_add" } }
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 33083af..efcc66c 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -16,6 +16,7 @@
  */
 
 #include "monitor.h"
+#include "qemu-objects.h"
 #include "qemu-queue.h"
 #include "sysbus.h"
 #include "virtio-serial.h"
@@ -204,6 +205,17 @@ size_t virtio_serial_guest_ready(VirtIOSerialPort *port)
     return 0;
 }
 
+static void mon_event(const char *device, const uint32_t port_id,
+                      const char *operation, const char *result)
+{
+    QObject *data;
+
+    data = qobject_from_jsonf("{ 'device': %s, 'port': %ld, 'operation': %s, 'result': %s }",
+                              device, (int64_t)port_id, operation, result);
+    monitor_protocol_event(QEVENT_VIRTIO_SERIAL, data);
+    qobject_decref(data);
+}
+
 /* Guest wants to notify us of some event */
 static void handle_control_message(VirtIOSerial *vser, void *buf)
 {
@@ -226,6 +238,8 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
         if (!cpkt.value) {
             error_report("virtio-serial-bus: Guest failure in adding device %s\n",
                 vser->bus->qbus.name);
+            mon_event(vser->bus->qbus.name, VIRTIO_CONSOLE_BAD_ID,
+		      "device_add", "fail");
             break;
         }
         /*
@@ -241,6 +255,7 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
         if (!cpkt.value) {
             error_report("virtio-serial-bus: Guest failure in adding port %u for device %s\n",
                          port->id, vser->bus->qbus.name);
+            mon_event(vser->bus->qbus.name, port->id, "port_add", "fail");
             break;
         }
         /*
diff --git a/monitor.c b/monitor.c
index 0448a70..9e5bfe7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -441,6 +441,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
         case QEVENT_WATCHDOG:
             event_name = "WATCHDOG";
             break;
+        case QEVENT_VIRTIO_SERIAL:
+            event_name = "VIRTIO_SERIAL";
+            break;
         default:
             abort();
             break;
diff --git a/monitor.h b/monitor.h
index bd4ae34..ea4df8a 100644
--- a/monitor.h
+++ b/monitor.h
@@ -28,6 +28,7 @@ typedef enum MonitorEvent {
     QEVENT_BLOCK_IO_ERROR,
     QEVENT_RTC_CHANGE,
     QEVENT_WATCHDOG,
+    QEVENT_VIRTIO_SERIAL,
     QEVENT_MAX,
 } MonitorEvent;
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 11/15] virtio-serial: Send out guest data to ports only if port is opened

[Amit Shah]

Data should be written only when ports are open.

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

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index efcc66c..80fbff4 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -350,6 +350,11 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
             goto next_buf;
         }
 
+	if (!port->host_connected) {
+            ret = 0;
+            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
-- 
1.6.2.5

[Qemu-devel] [PATCH 12/15] iov: Introduce a new file for helpers around iovs, add iov_from_buf()

[Amit Shah]

The virtio-net code uses iov_fill() which fills an iov from a linear
buffer. The virtio-serial-bus code does something similar in an
open-coded function.

Create a new iov.c file that has iov_from_buf().

Convert virtio-net and virtio-serial-bus over to use this functionality.
virtio-net used ints to hold sizes, the new function is going to use
size_t types.

Later commits will add the opposite functionality -- going from an iov
to a linear buffer.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 Makefile.objs          |    1 +
 hw/iov.c               |   33 +++++++++++++++++++++++++++++++++
 hw/iov.h               |   16 ++++++++++++++++
 hw/virtio-net.c        |   20 +++-----------------
 hw/virtio-serial-bus.c |   15 +++++++--------
 5 files changed, 60 insertions(+), 25 deletions(-)
 create mode 100644 hw/iov.c
 create mode 100644 hw/iov.h

diff --git a/Makefile.objs b/Makefile.objs
index 281f7a6..212ae1d 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -127,6 +127,7 @@ user-obj-y += cutils.o cache-utils.o
 # libhw
 
 hw-obj-y =
+hw-obj-y += iov.o
 hw-obj-y += loader.o
 hw-obj-y += virtio.o virtio-console.o virtio-pci.o
 hw-obj-y += fw_cfg.o pci.o pci_host.o pcie_host.o
diff --git a/hw/iov.c b/hw/iov.c
new file mode 100644
index 0000000..07bd499
--- /dev/null
+++ b/hw/iov.c
@@ -0,0 +1,33 @@
+/*
+ * Helpers for getting linearized buffers from iov / filling buffers into iovs
+ *
+ * Copyright IBM, Corp. 2007, 2008
+ * Copyright (C) 2010 Red Hat, Inc.
+ *
+ * Author(s):
+ *  Anthony Liguori <aliguori@us.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.
+ */
+
+#include "iov.h"
+
+size_t iov_from_buf(struct iovec *iov, unsigned int iovcnt,
+                    const void *buf, size_t size)
+{
+    size_t offset;
+    unsigned int i;
+
+    offset = 0;
+    for (i = 0; offset < size && i < iovcnt; i++) {
+        size_t len;
+
+        len = MIN(iov[i].iov_len, size - offset);
+
+        memcpy(iov[i].iov_base, buf + offset, len);
+        offset += len;
+    }
+    return offset;
+}
diff --git a/hw/iov.h b/hw/iov.h
new file mode 100644
index 0000000..5e3e541
--- /dev/null
+++ b/hw/iov.h
@@ -0,0 +1,16 @@
+/*
+ * Helpers for getting linearized buffers from iov / filling buffers into iovs
+ *
+ * Copyright (C) 2010 Red Hat, Inc.
+ *
+ * Author(s):
+ *  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-common.h"
+
+size_t iov_from_buf(struct iovec *iov, unsigned int iovcnt,
+                    const void *buf, size_t size);
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index be33c68..273e7f9 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -11,6 +11,7 @@
  *
  */
 
+#include "iov.h"
 #include "virtio.h"
 #include "net.h"
 #include "net/checksum.h"
@@ -423,21 +424,6 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
     }
 }
 
-static int iov_fill(struct iovec *iov, int iovcnt, const void *buf, int count)
-{
-    int offset, i;
-
-    offset = i = 0;
-    while (offset < count && i < iovcnt) {
-        int len = MIN(iov[i].iov_len, count - offset);
-        memcpy(iov[i].iov_base, buf + offset, len);
-        offset += len;
-        i++;
-    }
-
-    return offset;
-}
-
 static int receive_header(VirtIONet *n, struct iovec *iov, int iovcnt,
                           const void *buf, size_t size, size_t hdr_len)
 {
@@ -573,8 +559,8 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
         }
 
         /* copy in packet.  ugh */
-        len = iov_fill(sg, elem.in_num,
-                       buf + offset, size - offset);
+        len = iov_from_buf(sg, elem.in_num,
+                           buf + offset, size - offset);
         total += len;
 
         /* signal other side */
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 80fbff4..bd1223e 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -15,6 +15,7 @@
  * the COPYING file in the top-level directory.
  */
 
+#include "iov.h"
 #include "monitor.h"
 #include "qemu-objects.h"
 #include "qemu-queue.h"
@@ -85,27 +86,25 @@ static size_t write_to_port(VirtIOSerialPort *port,
 {
     VirtQueueElement elem;
     VirtQueue *vq;
-    size_t offset = 0;
-    size_t len = 0;
+    size_t offset;
 
     vq = port->ivq;
     if (!virtio_queue_ready(vq)) {
         return 0;
     }
 
+    offset = 0;
     while (offset < size) {
-        int i;
+        size_t len;
 
         if (!virtqueue_pop(vq, &elem)) {
             break;
         }
 
-        for (i = 0; offset < size && i < elem.in_num; i++) {
-            len = MIN(elem.in_sg[i].iov_len, size - offset);
+        len = iov_from_buf(elem.in_sg, elem.in_num,
+                           buf + offset, size - offset);
+        offset += len;
 
-            memcpy(elem.in_sg[i].iov_base, buf + offset, len);
-            offset += len;
-        }
         virtqueue_push(vq, &elem, len);
     }
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 13/15] iov: Add iov_to_buf and iov_size helpers

[Amit Shah]

iov_to_buf() puts the buffer contents in the iov in a linearized buffer.

iov_size() gets the length of the contents in the iov.

The iov_to_buf() function is the memcpy_to_iovec() function that was
used in virtio-ballon.c.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/iov.c            |   37 +++++++++++++++++++++++++++++++++++++
 hw/iov.h            |    3 +++
 hw/virtio-balloon.c |   35 ++++-------------------------------
 3 files changed, 44 insertions(+), 31 deletions(-)

diff --git a/hw/iov.c b/hw/iov.c
index 07bd499..d4013cd 100644
--- a/hw/iov.c
+++ b/hw/iov.c
@@ -31,3 +31,40 @@ size_t iov_from_buf(struct iovec *iov, unsigned int iovcnt,
     }
     return offset;
 }
+
+size_t iov_to_buf(struct iovec *iov, unsigned int iovcnt,
+                  void *buf, size_t offset, size_t size)
+{
+    uint8_t *ptr;
+    size_t iov_off, buf_off;
+    unsigned int i;
+
+    ptr = buf;
+    iov_off = 0;
+    buf_off = 0;
+    for (i = 0; i < iovcnt && size; i++) {
+        if (offset < (iov_off + iov[i].iov_len)) {
+            size_t len = MIN((iov_off + iov[i].iov_len) - offset , size);
+
+            memcpy(ptr + buf_off, iov[i].iov_base + (offset - iov_off), len);
+
+            buf_off += len;
+            offset += len;
+            size -= len;
+        }
+        iov_off += iov[i].iov_len;
+    }
+    return buf_off;
+}
+
+size_t iov_size(struct iovec *iov, unsigned int iovcnt)
+{
+    size_t len;
+    unsigned int i;
+
+    len = 0;
+    for (i = 0; i < iovcnt; i++) {
+        len += iov[i].iov_len;
+    }
+    return len;
+}
diff --git a/hw/iov.h b/hw/iov.h
index 5e3e541..c977ff1 100644
--- a/hw/iov.h
+++ b/hw/iov.h
@@ -14,3 +14,6 @@
 
 size_t iov_from_buf(struct iovec *iov, unsigned int iovcnt,
                     const void *buf, size_t size);
+size_t iov_to_buf(struct iovec *iov, unsigned int iovcnt,
+                  void *buf, size_t offset, size_t size);
+size_t iov_size(struct iovec *iov, unsigned int iovcnt);
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 6d12024..4414eae 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -11,6 +11,7 @@
  *
  */
 
+#include "iov.h"
 #include "qemu-common.h"
 #include "virtio.h"
 #include "pc.h"
@@ -91,33 +92,6 @@ static QObject *get_stats_qobject(VirtIOBalloon *dev)
     return QOBJECT(dict);
 }
 
-/* FIXME: once we do a virtio refactoring, this will get subsumed into common
- * code */
-static size_t memcpy_from_iovector(void *data, size_t offset, size_t size,
-                                   struct iovec *iov, int iovlen)
-{
-    int i;
-    uint8_t *ptr = data;
-    size_t iov_off = 0;
-    size_t data_off = 0;
-
-    for (i = 0; i < iovlen && size; i++) {
-        if (offset < (iov_off + iov[i].iov_len)) {
-            size_t len = MIN((iov_off + iov[i].iov_len) - offset , size);
-
-            memcpy(ptr + data_off, iov[i].iov_base + (offset - iov_off), len);
-
-            data_off += len;
-            offset += len;
-            size -= len;
-        }
-
-        iov_off += iov[i].iov_len;
-    }
-
-    return data_off;
-}
-
 static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOBalloon *s = to_virtio_balloon(vdev);
@@ -127,8 +101,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
         size_t offset = 0;
         uint32_t pfn;
 
-        while (memcpy_from_iovector(&pfn, offset, 4,
-                                    elem.out_sg, elem.out_num) == 4) {
+        while (iov_to_buf(elem.out_sg, elem.out_num, &pfn, offset, 4) == 4) {
             ram_addr_t pa;
             ram_addr_t addr;
 
@@ -180,8 +153,8 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
      */
     reset_stats(s);
 
-    while (memcpy_from_iovector(&stat, offset, sizeof(stat), elem->out_sg,
-                                elem->out_num) == sizeof(stat)) {
+    while (iov_to_buf(elem->out_sg, elem->out_num, &stat, offset, sizeof(stat))
+           == sizeof(stat)) {
         uint16_t tag = tswap16(stat.tag);
         uint64_t val = tswap64(stat.val);
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 14/15] 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 |   34 +++++++++++++++++++++++++++++++---
 1 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index bd1223e..3edfeca 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -216,7 +216,7 @@ static void mon_event(const char *device, const uint32_t port_id,
 }
 
 /* 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;
@@ -225,6 +225,11 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
 
     gcpkt = buf;
 
+    if (len < sizeof(cpkt)) {
+        /* The guest sent an invalid control packet */
+        return;
+    }
+
     cpkt.event = lduw_p(&gcpkt->event);
     cpkt.value = lduw_p(&gcpkt->value);
 
@@ -321,12 +326,35 @@ 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);
+        size_t cur_len, copied;
+
+        cur_len = iov_size(elem.out_sg, elem.out_num);
+        /*
+         * 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);
+            len = cur_len;
+        }
+        copied = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, len);
+
+        handle_control_message(vser, buf, copied);
+        virtqueue_push(vq, &elem, copied);
+    }
+    if (len) {
+        qemu_free(buf);
     }
     virtio_notify(vdev, vq);
 }
-- 
1.6.2.5

[Qemu-devel] [PATCH 15/15] 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 |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 3edfeca..42e4ed0 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -369,7 +369,8 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
     while (virtqueue_pop(vq, &elem)) {
         VirtIOSerialPort *port;
-        size_t ret;
+        uint8_t *buf;
+        size_t ret, buf_size;
 
         port = find_port_by_vq(vser, vq);
         if (!port) {
@@ -392,9 +393,12 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
             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);
+        buf_size = iov_size(elem.out_sg, elem.out_num);
+        buf = qemu_malloc(buf_size);
+        ret = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, buf_size);
+
+        ret = port->info->have_data(port, buf, ret);
+        qemu_free(buf);
 
     next_buf:
         virtqueue_push(vq, &elem, ret);
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Luiz Capitulino]

On Wed, 24 Mar 2010 20:19:28 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> When adding a port or a device to the guest fails, management software
> might be interested in knowing and then cleaning up the host-side of the
> port. Introduce QMP events to signal such errors.

 I'm completely unfamiliar with virtio-serial, so let me ask: how are ports
added? I'd expect the command performing this operation to fail in this case.

[Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Amit Shah]

On (Wed) Mar 24 2010 [17:34:15], Luiz Capitulino wrote:
> On Wed, 24 Mar 2010 20:19:28 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> 
> > When adding a port or a device to the guest fails, management software
> > might be interested in knowing and then cleaning up the host-side of the
> > port. Introduce QMP events to signal such errors.
> 
>  I'm completely unfamiliar with virtio-serial, so let me ask: how are ports
> added? I'd expect the command performing this operation to fail in this case.

If adding the port fails in qemu, then the command will fail. However if
adding the port in the guest fails, we raise this QMP event. Adding in
the guest could fail because of several reasons, like ENOMEM. In this
case, the mgmt might want to hot-unplug the port from qemu so that
resources are freed and also apps are notified of guest side not ready.

		Amit

[Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Luiz Capitulino]

On Thu, 25 Mar 2010 09:17:17 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Wed) Mar 24 2010 [17:34:15], Luiz Capitulino wrote:
> > On Wed, 24 Mar 2010 20:19:28 +0530
> > Amit Shah <amit.shah@redhat.com> wrote:
> > 
> > > When adding a port or a device to the guest fails, management software
> > > might be interested in knowing and then cleaning up the host-side of the
> > > port. Introduce QMP events to signal such errors.
> > 
> >  I'm completely unfamiliar with virtio-serial, so let me ask: how are ports
> > added? I'd expect the command performing this operation to fail in this case.
> 
> If adding the port fails in qemu, then the command will fail. However if
> adding the port in the guest fails, we raise this QMP event. Adding in
> the guest could fail because of several reasons, like ENOMEM. In this
> case, the mgmt might want to hot-unplug the port from qemu so that
> resources are freed and also apps are notified of guest side not ready.

 Ok, what about a disconnect? Does virtio-serial have this kind of action?

[Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Luiz Capitulino]

On Wed, 24 Mar 2010 20:19:28 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> When adding a port or a device to the guest fails, management software
> might be interested in knowing and then cleaning up the host-side of the
> port. Introduce QMP events to signal such errors.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> CC: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  QMP/qmp-events.txt     |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/virtio-serial-bus.c |   15 +++++++++++++++
>  monitor.c              |    3 +++
>  monitor.h              |    1 +
>  4 files changed, 67 insertions(+), 0 deletions(-)
> 
> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> index a94e9b4..f13cf45 100644
> --- a/QMP/qmp-events.txt
> +++ b/QMP/qmp-events.txt
> @@ -188,3 +188,51 @@ Example:
>  
>  Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
>  followed respectively by the RESET, SHUTDOWN, or STOP events.
> +
> +VIRTIO_SERIAL
> +-------------

 It should be VIRTIO_SERIAL_ADD.

> +
> +Emitted when errors occur in guest port add or guest device add.
> +
> +Data:
> +
> +- "device": The virtio-serial device that triggered the event {json-string}
> +      This is the name given to the bus on the command line:
> +        -device virtio-serial,id="foo"
> +      here, the device name is "foo"
> +
> +- "port": The port number that triggered the event {json-number}
> +      This is the number given to the port on the command line:
> +        -device virtserialport,nr=2
> +      here, the port number is 2. If not mentioned on the command
> +      line, the number is auto-assigned.

 We use (json-number) instead of {json-number}.

> +
> +- "result": The result of the operation {json-string}
> +      This is one of the following:
> +         "pass", "fail"

 "result" could be a boolean "success".

> +
> +- "operation": The operation that triggered the event {json-sring}
> +      This is one of the following:
> +         "port_add", "device_add"

 You can drop the '_add', as this information is in the event name.

> +
> +Example:
> +
> +Port 0 add failure in the guest:
> +
> +{ "timestamp": {"seconds": 1269438649, "microseconds": 851170},
> +  "event": "VIRTIO_SERIAL",
> +  "data": {
> +            "device": "virtio-serial-bus.0",
> +            "port": 0,
> +            "result": "fail",
> +            "operation": "port_add" } }

 If you look at the other events you will see that I put the event
first and the timestamp later, I know this is not how the event is
going to be on the wire but improves the readability of this file
(and the spec says that clients should not assume the ordering of
dicts or lists).

 Implementation looks ok.

Re: [Qemu-devel] [PATCH 02/15] virtio-serial: save/load: Ensure nr_ports on src and dest are same.

[Jamie Lokier]

Amit Shah wrote:
> 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.

Surely the set of guest-visible ids must match exactly and be checked
(maybe mapped, if they were given in a different order), in which case
counting the number of ports looks redundant.

I.e. is this check hiding the omission of the proper one?

-- Jamie

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Jamie Lokier]

Luiz Capitulino wrote:
> On Thu, 25 Mar 2010 09:17:17 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> 
> > On (Wed) Mar 24 2010 [17:34:15], Luiz Capitulino wrote:
> > > On Wed, 24 Mar 2010 20:19:28 +0530
> > > Amit Shah <amit.shah@redhat.com> wrote:
> > > 
> > > > When adding a port or a device to the guest fails, management software
> > > > might be interested in knowing and then cleaning up the host-side of the
> > > > port. Introduce QMP events to signal such errors.
> > > 
> > >  I'm completely unfamiliar with virtio-serial, so let me ask: how are ports
> > > added? I'd expect the command performing this operation to fail in this case.
> > 
> > If adding the port fails in qemu, then the command will fail. However if
> > adding the port in the guest fails, we raise this QMP event. Adding in
> > the guest could fail because of several reasons, like ENOMEM. In this
> > case, the mgmt might want to hot-unplug the port from qemu so that
> > resources are freed and also apps are notified of guest side not ready.
> 
>  Ok, what about a disconnect? Does virtio-serial have this kind of action?

Disconnect would be valuable.  E.g. if the guest app dies (but not the
guest kernel), it won't get a chance to send an "I'm going away"
message.

Machine reboot, PCI reset and so on, should probably trigger a
disconnect event too (perhaps annotated with the reason), so that the
host app's byte stream parser can reliably start parsing anew.
Somehow that reset event needs to be synchronised with data transport,
so the host app works when the guest boots, reconnects and sends more
data before the host app has had enough time to process the earlier
reset event.

Connect event is a nice idea for symmetry, so the host knows when the
guest app has started up, but it's not essential as the guest app can
just send a message.

-- Jamie

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Amit Shah]

On (Thu) Mar 25 2010 [15:34:06], Luiz Capitulino wrote:
> On Thu, 25 Mar 2010 09:17:17 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> 
> > On (Wed) Mar 24 2010 [17:34:15], Luiz Capitulino wrote:
> > > On Wed, 24 Mar 2010 20:19:28 +0530
> > > Amit Shah <amit.shah@redhat.com> wrote:
> > > 
> > > > When adding a port or a device to the guest fails, management software
> > > > might be interested in knowing and then cleaning up the host-side of the
> > > > port. Introduce QMP events to signal such errors.
> > > 
> > >  I'm completely unfamiliar with virtio-serial, so let me ask: how are ports
> > > added? I'd expect the command performing this operation to fail in this case.
> > 
> > If adding the port fails in qemu, then the command will fail. However if
> > adding the port in the guest fails, we raise this QMP event. Adding in
> > the guest could fail because of several reasons, like ENOMEM. In this
> > case, the mgmt might want to hot-unplug the port from qemu so that
> > resources are freed and also apps are notified of guest side not ready.
> 
>  Ok, what about a disconnect? Does virtio-serial have this kind of action?

In case of guest disconnect, an event is sent by the guest to the host
and the host sends it to the app. A qmp event is not triggered, one can
be, if desired.

		Amit

Re: [Qemu-devel] [PATCH 02/15] virtio-serial: save/load: Ensure nr_ports on src and dest are same.

[Amit Shah]

On (Fri) Mar 26 2010 [01:09:23], Jamie Lokier wrote:
> Amit Shah wrote:
> > 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.
> 
> Surely the set of guest-visible ids must match exactly and be checked
> (maybe mapped, if they were given in a different order), in which case
> counting the number of ports looks redundant.
> 
> I.e. is this check hiding the omission of the proper one?

Yes, That's added in a later patch (5). That patch changes the guest
abi, so it's separate.

Also, patch 3 checks if all the ports on the src are present on the
dest.

		Amit

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Amit Shah]

On (Fri) Mar 26 2010 [01:17:49], Jamie Lokier wrote:
> Luiz Capitulino wrote:
> > On Thu, 25 Mar 2010 09:17:17 +0530
> > Amit Shah <amit.shah@redhat.com> wrote:
> > 
> > > On (Wed) Mar 24 2010 [17:34:15], Luiz Capitulino wrote:
> > > > On Wed, 24 Mar 2010 20:19:28 +0530
> > > > Amit Shah <amit.shah@redhat.com> wrote:
> > > > 
> > > > > When adding a port or a device to the guest fails, management software
> > > > > might be interested in knowing and then cleaning up the host-side of the
> > > > > port. Introduce QMP events to signal such errors.
> > > > 
> > > >  I'm completely unfamiliar with virtio-serial, so let me ask: how are ports
> > > > added? I'd expect the command performing this operation to fail in this case.
> > > 
> > > If adding the port fails in qemu, then the command will fail. However if
> > > adding the port in the guest fails, we raise this QMP event. Adding in
> > > the guest could fail because of several reasons, like ENOMEM. In this
> > > case, the mgmt might want to hot-unplug the port from qemu so that
> > > resources are freed and also apps are notified of guest side not ready.
> > 
> >  Ok, what about a disconnect? Does virtio-serial have this kind of action?
> 
> Disconnect would be valuable.  E.g. if the guest app dies (but not the
> guest kernel), it won't get a chance to send an "I'm going away"
> message.

That's something applications should be able to handle: If an app on the
guest dies, the app on the host should be able to discover this.

In any case, we have 'open' and 'close' notifications where we trigger
callbacks for the applications if they're interested in such events.
This only works for in-qemu apps, though, so I'm OK with adding a QMP
event for this as well.

> Machine reboot, PCI reset and so on, should probably trigger a

All these messages belong to other subsystems, not virtio-serial. Eg,
libvirt or other mgmt app should know that a reset event, when received,
affects virtio ports as well. Similar for pci events.

		Amit

[Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Amit Shah]

On (Thu) Mar 25 2010 [15:55:41], Luiz Capitulino wrote:
> On Wed, 24 Mar 2010 20:19:28 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> 
> > When adding a port or a device to the guest fails, management software
> > might be interested in knowing and then cleaning up the host-side of the
> > port. Introduce QMP events to signal such errors.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > CC: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  QMP/qmp-events.txt     |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/virtio-serial-bus.c |   15 +++++++++++++++
> >  monitor.c              |    3 +++
> >  monitor.h              |    1 +
> >  4 files changed, 67 insertions(+), 0 deletions(-)
> > 
> > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> > index a94e9b4..f13cf45 100644
> > --- a/QMP/qmp-events.txt
> > +++ b/QMP/qmp-events.txt
> > @@ -188,3 +188,51 @@ Example:
> >  
> >  Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
> >  followed respectively by the RESET, SHUTDOWN, or STOP events.
> > +
> > +VIRTIO_SERIAL
> > +-------------
> 
>  It should be VIRTIO_SERIAL_ADD.

What about other events that VIRTIO_SERIAL generates? Should they have a
different event by themselves? Or should they ride on top of
VIRTIO_SERIAL and mention different 'operations' that caused the event?

> > +Emitted when errors occur in guest port add or guest device add.
> > +
> > +Data:
> > +
> > +- "device": The virtio-serial device that triggered the event {json-string}
> > +      This is the name given to the bus on the command line:
> > +        -device virtio-serial,id="foo"
> > +      here, the device name is "foo"
> > +
> > +- "port": The port number that triggered the event {json-number}
> > +      This is the number given to the port on the command line:
> > +        -device virtserialport,nr=2
> > +      here, the port number is 2. If not mentioned on the command
> > +      line, the number is auto-assigned.
> 
>  We use (json-number) instead of {json-number}.

Fixed.

> > +
> > +- "result": The result of the operation {json-string}
> > +      This is one of the following:
> > +         "pass", "fail"
> 
>  "result" could be a boolean "success".

OK; success/fail? Also, by boolean, do you mean the data type? How is
that represented?

(Note: I put the port number as '%ld' instead of '%u' since %u isn't
parsed..)

> > +- "operation": The operation that triggered the event {json-sring}
> > +      This is one of the following:
> > +         "port_add", "device_add"
> 
>  You can drop the '_add', as this information is in the event name.

The answer to the first question above will answer this too.

> > +Example:
> > +
> > +Port 0 add failure in the guest:
> > +
> > +{ "timestamp": {"seconds": 1269438649, "microseconds": 851170},
> > +  "event": "VIRTIO_SERIAL",
> > +  "data": {
> > +            "device": "virtio-serial-bus.0",
> > +            "port": 0,
> > +            "result": "fail",
> > +            "operation": "port_add" } }
> 
>  If you look at the other events you will see that I put the event
> first and the timestamp later, I know this is not how the event is
> going to be on the wire but improves the readability of this file
> (and the spec says that clients should not assume the ordering of
> dicts or lists).

OK; I'll do that.

>  Implementation looks ok.

OK, thanks.

		Amit

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Jamie Lokier]

Amit Shah wrote:
> On (Fri) Mar 26 2010 [01:17:49], Jamie Lokier wrote:
> > Luiz Capitulino wrote:
> > > On Thu, 25 Mar 2010 09:17:17 +0530
> > > Amit Shah <amit.shah@redhat.com> wrote:
> > > 
> > > > On (Wed) Mar 24 2010 [17:34:15], Luiz Capitulino wrote:
> > > > > On Wed, 24 Mar 2010 20:19:28 +0530
> > > > > Amit Shah <amit.shah@redhat.com> wrote:
> > > > > 
> > > > > > When adding a port or a device to the guest fails, management software
> > > > > > might be interested in knowing and then cleaning up the host-side of the
> > > > > > port. Introduce QMP events to signal such errors.
> > > > > 
> > > > >  I'm completely unfamiliar with virtio-serial, so let me ask: how are ports
> > > > > added? I'd expect the command performing this operation to fail in this case.
> > > > 
> > > > If adding the port fails in qemu, then the command will fail. However if
> > > > adding the port in the guest fails, we raise this QMP event. Adding in
> > > > the guest could fail because of several reasons, like ENOMEM. In this
> > > > case, the mgmt might want to hot-unplug the port from qemu so that
> > > > resources are freed and also apps are notified of guest side not ready.
> > > 
> > >  Ok, what about a disconnect? Does virtio-serial have this kind of action?
> > 
> > Disconnect would be valuable.  E.g. if the guest app dies (but not the
> > guest kernel), it won't get a chance to send an "I'm going away"
> > message.
> 
> That's something applications should be able to handle: If an app on the
> guest dies, the app on the host should be able to discover this.

Sure.  Does the host app see an EOF on its input when that happens?
(I.e. *not* like a real serial port).

If so, there's no need for a disconnect event, otherwise, if it's like
a serial port, there is.

> > Machine reboot, PCI reset and so on, should probably trigger a
> 
> All these messages belong to other subsystems, not virtio-serial. Eg,
b> libvirt or other mgmt app should know that a reset event, when received,
> affects virtio ports as well. Similar for pci events.

Fine (although I don't like that a non-mgmt host app only interested
talking to a guest with an architecture-neutral protocol might need to
know about PCI, or even need to know anything about how the VM was
launched).

But what I really meant was, if the guest resets, and then it boots up
before the host apps manage to process their events (e.g. due to
timing, remoteness, swapping or whatever), it's important that the
virtio-serial using host app knows where the discontinuity in the byte
stream is.  Otherwise the app needs to use a silly overcomplicated
protocol just to provide a reliable layer over the byte stream.

-- Jamie

Re: [Qemu-devel] [PATCH 02/15] virtio-serial: save/load: Ensure nr_ports on src and dest are same.

[Jamie Lokier]

Amit Shah wrote:
> On (Fri) Mar 26 2010 [01:09:23], Jamie Lokier wrote:
> > Amit Shah wrote:
> > > 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.
> > 
> > Surely the set of guest-visible ids must match exactly and be checked
> > (maybe mapped, if they were given in a different order), in which case
> > counting the number of ports looks redundant.
> > 
> > I.e. is this check hiding the omission of the proper one?
> 
> Yes, That's added in a later patch (5). That patch changes the guest
> abi, so it's separate.
> 
> Also, patch 3 checks if all the ports on the src are present on the
> dest.

Great, thanks :-)

-- Jamie

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Amit Shah]

On (Fri) Mar 26 2010 [04:07:54], Jamie Lokier wrote:
> Amit Shah wrote:
> > On (Fri) Mar 26 2010 [01:17:49], Jamie Lokier wrote:
> > > Luiz Capitulino wrote:
> > > > On Thu, 25 Mar 2010 09:17:17 +0530
> > > > Amit Shah <amit.shah@redhat.com> wrote:
> > > > 
> > > > > On (Wed) Mar 24 2010 [17:34:15], Luiz Capitulino wrote:
> > > > > > On Wed, 24 Mar 2010 20:19:28 +0530
> > > > > > Amit Shah <amit.shah@redhat.com> wrote:
> > > > > > 
> > > > > > > When adding a port or a device to the guest fails, management software
> > > > > > > might be interested in knowing and then cleaning up the host-side of the
> > > > > > > port. Introduce QMP events to signal such errors.
> > > > > > 
> > > > > >  I'm completely unfamiliar with virtio-serial, so let me ask: how are ports
> > > > > > added? I'd expect the command performing this operation to fail in this case.
> > > > > 
> > > > > If adding the port fails in qemu, then the command will fail. However if
> > > > > adding the port in the guest fails, we raise this QMP event. Adding in
> > > > > the guest could fail because of several reasons, like ENOMEM. In this
> > > > > case, the mgmt might want to hot-unplug the port from qemu so that
> > > > > resources are freed and also apps are notified of guest side not ready.
> > > > 
> > > >  Ok, what about a disconnect? Does virtio-serial have this kind of action?
> > > 
> > > Disconnect would be valuable.  E.g. if the guest app dies (but not the
> > > guest kernel), it won't get a chance to send an "I'm going away"
> > > message.
> > 
> > That's something applications should be able to handle: If an app on the
> > guest dies, the app on the host should be able to discover this.
> 
> Sure.  Does the host app see an EOF on its input when that happens?
> (I.e. *not* like a real serial port).

If it's an in-qemu app, it gets the guest_closed() callback. So I guess
qmp events for non-qemu apps is what you're looking for?

> If so, there's no need for a disconnect event, otherwise, if it's like
> a serial port, there is.
> 
> > > Machine reboot, PCI reset and so on, should probably trigger a
> > 
> > All these messages belong to other subsystems, not virtio-serial. Eg,
> b> libvirt or other mgmt app should know that a reset event, when received,
> > affects virtio ports as well. Similar for pci events.
> 
> Fine (although I don't like that a non-mgmt host app only interested
> talking to a guest with an architecture-neutral protocol might need to
> know about PCI, or even need to know anything about how the VM was
> launched).
> 
> But what I really meant was, if the guest resets, and then it boots up
> before the host apps manage to process their events (e.g. due to
> timing, remoteness, swapping or whatever), it's important that the
> virtio-serial using host app knows where the discontinuity in the byte
> stream is.  Otherwise the app needs to use a silly overcomplicated
> protocol just to provide a reliable layer over the byte stream.

I'd rather that the apps used the existing QMP notifications for guest
reset so that we don't have to do anything special for virtio-serial
(and for other devices as well).

Also, if it might help, the 'guest device ready' and 'guest port ready'
QMP events can be sent on success as well (right now they're only sent
on failure).

		Amit

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Jamie Lokier]

Amit Shah wrote:
> > Sure.  Does the host app see an EOF on its input when that happens?
> > (I.e. *not* like a real serial port).
> 
> If it's an in-qemu app, it gets the guest_closed() callback. So I guess
> qmp events for non-qemu apps is what you're looking for?

See below.

> > But what I really meant was, if the guest resets, and then it boots up
> > before the host apps manage to process their events (e.g. due to
> > timing, remoteness, swapping or whatever), it's important that the
> > virtio-serial using host app knows where the discontinuity in the byte
> > stream is.  Otherwise the app needs to use a silly overcomplicated
> > protocol just to provide a reliable layer over the byte stream.
> 
> I'd rather that the apps used the existing QMP notifications for guest
> reset so that we don't have to do anything special for virtio-serial
> (and for other devices as well).

I'm trying to understand how to avoid the race condition with that.

1. guest sends a big blob of data to virtio-serial
2. qemu writes to socket for host app
      -> wakes up host app (outside qemu) listening for virtio-serial data
3. guest resets or its kernel crashes
      -> the big blob is only partially sent
4. qemu sends QMP notification
5.    -> wakes up host app listening for QMP events
6. guest boots up.
7. guest opens virtio-serial, and starts by sending a message.
8.    -> host app gets to run, sees the event sent in step 2.
9.    -> host app reads available data from virtio-serial
         data includes bytes sent in step 1 and step 7

Can the host app tell which bytes to discard because they were a
truncated message sent prior to the reset, so that it can find the
start of the new message sent in step 7?

A QMP event has that race condition.

If communication with the external host app is over a local socket
(AF_UNIX or AF_INET or mkpipe), qemu could just disconnect and
reconnect whenever the guest does, which is perfectly logical and
solves this.

If the external host app is fork+exec'd from qemu with a pair of pipes
(",exec="), closing the writing pipe, waiting for EOF from the
reading pipe, and then re-exec-ing the external app would do as well.

If neither of those are used, then a bit more context from QMP is
needed, such as the exact number of bytes transmitted prior to the
reset, presumably in a virtio-serial close event.

-- Jamie

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Luiz Capitulino]

On Fri, 26 Mar 2010 10:26:07 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> Also, if it might help, the 'guest device ready' and 'guest port ready'
> QMP events can be sent on success as well (right now they're only sent
> on failure).

 Although this seems to be make, would be good to have clients' writers
feedback on this or wait until this is really needed.

 I'd like to avoid over designing.

[Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Luiz Capitulino]

On Fri, 26 Mar 2010 07:46:28 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Thu) Mar 25 2010 [15:55:41], Luiz Capitulino wrote:
> > On Wed, 24 Mar 2010 20:19:28 +0530
> > Amit Shah <amit.shah@redhat.com> wrote:
> > 
> > > When adding a port or a device to the guest fails, management software
> > > might be interested in knowing and then cleaning up the host-side of the
> > > port. Introduce QMP events to signal such errors.
> > > 
> > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > CC: Luiz Capitulino <lcapitulino@redhat.com>
> > > ---
> > >  QMP/qmp-events.txt     |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  hw/virtio-serial-bus.c |   15 +++++++++++++++
> > >  monitor.c              |    3 +++
> > >  monitor.h              |    1 +
> > >  4 files changed, 67 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> > > index a94e9b4..f13cf45 100644
> > > --- a/QMP/qmp-events.txt
> > > +++ b/QMP/qmp-events.txt
> > > @@ -188,3 +188,51 @@ Example:
> > >  
> > >  Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
> > >  followed respectively by the RESET, SHUTDOWN, or STOP events.
> > > +
> > > +VIRTIO_SERIAL
> > > +-------------
> > 
> >  It should be VIRTIO_SERIAL_ADD.
> 
> What about other events that VIRTIO_SERIAL generates?

 We don't address this problem currently, maybe an integration with qdev
will do, but I have to think more about it.

> Should they have a different event by themselves?

 With the current code, yes. But would be good to avoid it until we have
a proper solution.

> Or should they ride on top of VIRTIO_SERIAL and mention different
> 'operations' that caused the event?

 I'd prefer having a different name if it's a different event, at least
this is how we've done it so far.

> > > +
> > > +- "result": The result of the operation {json-string}
> > > +      This is one of the following:
> > > +         "pass", "fail"
> > 
> >  "result" could be a boolean "success".
> 
> OK; success/fail? Also, by boolean, do you mean the data type? How is
> that represented?

 In JSON it's true/false. In our parser you can use '%i' with integers,
undocumented, yes, sorry for that.

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Amit Shah]

On (Fri) Mar 26 2010 [10:05:07], Luiz Capitulino wrote:
> On Fri, 26 Mar 2010 10:26:07 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> 
> > Also, if it might help, the 'guest device ready' and 'guest port ready'
> > QMP events can be sent on success as well (right now they're only sent
> > on failure).
> 
>  Although this seems to be make, would be good to have clients' writers
> feedback on this or wait until this is really needed.

Right.

		Amit

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Amit Shah]

On (Fri) Mar 26 2010 [10:14:02], Luiz Capitulino wrote:
> > > > +
> > > > +VIRTIO_SERIAL
> > > > +-------------
> > > 
> > >  It should be VIRTIO_SERIAL_ADD.
> > 
> > What about other events that VIRTIO_SERIAL generates?
> 
>  We don't address this problem currently, maybe an integration with qdev
> will do, but I have to think more about it.

So should I just keep it as VIRTIO_SERIAL for now? With new events also
riding on this one?

> > Should they have a different event by themselves?
> 
>  With the current code, yes. But would be good to avoid it until we have
> a proper solution.
> 
> > Or should they ride on top of VIRTIO_SERIAL and mention different
> > 'operations' that caused the event?
> 
>  I'd prefer having a different name if it's a different event, at least
> this is how we've done it so far.

Erm, now I'm confused.

> > > > +
> > > > +- "result": The result of the operation {json-string}
> > > > +      This is one of the following:
> > > > +         "pass", "fail"
> > > 
> > >  "result" could be a boolean "success".
> > 
> > OK; success/fail? Also, by boolean, do you mean the data type? How is
> > that represented?
> 
>  In JSON it's true/false. In our parser you can use '%i' with integers,
> undocumented, yes, sorry for that.

Oh ok; no problem.

		Amit

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Amit Shah]

On (Fri) Mar 26 2010 [05:23:25], Jamie Lokier wrote:
> Amit Shah wrote:
> > > Sure.  Does the host app see an EOF on its input when that happens?
> > > (I.e. *not* like a real serial port).
> > 
> > If it's an in-qemu app, it gets the guest_closed() callback. So I guess
> > qmp events for non-qemu apps is what you're looking for?
> 
> See below.
> 
> > > But what I really meant was, if the guest resets, and then it boots up
> > > before the host apps manage to process their events (e.g. due to
> > > timing, remoteness, swapping or whatever), it's important that the
> > > virtio-serial using host app knows where the discontinuity in the byte
> > > stream is.  Otherwise the app needs to use a silly overcomplicated
> > > protocol just to provide a reliable layer over the byte stream.
> > 
> > I'd rather that the apps used the existing QMP notifications for guest
> > reset so that we don't have to do anything special for virtio-serial
> > (and for other devices as well).
> 
> I'm trying to understand how to avoid the race condition with that.
> 
> 1. guest sends a big blob of data to virtio-serial
> 2. qemu writes to socket for host app
>       -> wakes up host app (outside qemu) listening for virtio-serial data
> 3. guest resets or its kernel crashes
>       -> the big blob is only partially sent
> 4. qemu sends QMP notification
> 5.    -> wakes up host app listening for QMP events
> 6. guest boots up.
> 7. guest opens virtio-serial, and starts by sending a message.
> 8.    -> host app gets to run, sees the event sent in step 2.
> 9.    -> host app reads available data from virtio-serial
>          data includes bytes sent in step 1 and step 7
> 
> Can the host app tell which bytes to discard because they were a
> truncated message sent prior to the reset, so that it can find the
> start of the new message sent in step 7?
> 
> A QMP event has that race condition.

Problem is we're going to have to maintain a lot of state if we're going
to provide guarantees.

One solution is to always have an in-qemu user of the serial
functionality that sits between the app and the guest and the in-qemu
user can signal to the app about such things.

> If communication with the external host app is over a local socket
> (AF_UNIX or AF_INET or mkpipe), qemu could just disconnect and
> reconnect whenever the guest does, which is perfectly logical and
> solves this.

The virtio-serial code cannot know what kind of a connection it has with
the host app.

> If the external host app is fork+exec'd from qemu with a pair of pipes
> (",exec="), closing the writing pipe, waiting for EOF from the
> reading pipe, and then re-exec-ing the external app would do as well.
> 
> If neither of those are used, then a bit more context from QMP is
> needed, such as the exact number of bytes transmitted prior to the
> reset, presumably in a virtio-serial close event.

Yeah; that's some state to maintain -- and I'm not sure we should do
that. If we start adding some state now, we could end up adding more
later, and it could soon become a mess.

		Amit

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Luiz Capitulino]

On Fri, 26 Mar 2010 18:56:20 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Fri) Mar 26 2010 [10:14:02], Luiz Capitulino wrote:
> > > > > +
> > > > > +VIRTIO_SERIAL
> > > > > +-------------
> > > > 
> > > >  It should be VIRTIO_SERIAL_ADD.
> > > 
> > > What about other events that VIRTIO_SERIAL generates?
> > 
> >  We don't address this problem currently, maybe an integration with qdev
> > will do, but I have to think more about it.
> 
> So should I just keep it as VIRTIO_SERIAL for now? With new events also
> riding on this one?

 I don't like this because with the current events code this will lead
to confusion, as you're using a single event to notify different things.

 My suggestion for the immediate term is to do what we have been doing so
far, ie. call it VIRTIO_SERIAL_ADD. Worst case here is: we add a new way
to group events which requires a new VIRTIO_SERIAL event, in this case we
could emit both, the new VIRTIO_SERIAL and the old VIRTIO_SERIAL_ADD. The
latter would be deprecated too.

 Or, if you can wait I can _try_ to solve this problem next week, although
I have no idea how hard this is going to be.

 Any comments, Anthony?

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Amit Shah]

On (Fri) Mar 26 2010 [11:29:03], Luiz Capitulino wrote:
> On Fri, 26 Mar 2010 18:56:20 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> 
> > On (Fri) Mar 26 2010 [10:14:02], Luiz Capitulino wrote:
> > > > > > +
> > > > > > +VIRTIO_SERIAL
> > > > > > +-------------
> > > > > 
> > > > >  It should be VIRTIO_SERIAL_ADD.
> > > > 
> > > > What about other events that VIRTIO_SERIAL generates?
> > > 
> > >  We don't address this problem currently, maybe an integration with qdev
> > > will do, but I have to think more about it.
> > 
> > So should I just keep it as VIRTIO_SERIAL for now? With new events also
> > riding on this one?
> 
>  I don't like this because with the current events code this will lead
> to confusion, as you're using a single event to notify different things.
> 
>  My suggestion for the immediate term is to do what we have been doing so
> far, ie. call it VIRTIO_SERIAL_ADD. Worst case here is: we add a new way
> to group events which requires a new VIRTIO_SERIAL event, in this case we
> could emit both, the new VIRTIO_SERIAL and the old VIRTIO_SERIAL_ADD. The
> latter would be deprecated too.

I've no problem doing it either way - whatever you prefer is fine.

BTW these are two distinct events already - failure in initialising a
device and failure in initialising a port. Do you think these should be
separate as well?

>  Or, if you can wait I can _try_ to solve this problem next week, although
> I have no idea how hard this is going to be.

I think it's cleaner to club everything; but basically I'll go with
whatever you say. I've no problem waiting.

>  Any comments, Anthony?

		Amit

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Jamie Lokier]

Amit Shah wrote:
> Problem is we're going to have to maintain a lot of state if we're going
> to provide guarantees.
> 
> One solution is to always have an in-qemu user of the serial
> functionality that sits between the app and the guest and the in-qemu
> user can signal to the app about such things.

Isn't that what I suggested?  I don't mind how the app is signalled.
I'm just thinking of the simplest way to do it.  It doesn't have to
live in the virtio-serial driver, just be signalled somehow out of it.

> > If communication with the external host app is over a local socket
> > (AF_UNIX or AF_INET or mkpipe), qemu could just disconnect and
> > reconnect whenever the guest does, which is perfectly logical and
> > solves this.
> 
> The virtio-serial code cannot know what kind of a connection it has with
> the host app.

True, but the qemu internal plumbing could pass an open/close event to
that connection handler, for it to do whatever's appropriate.

I don't think that 1 bit state ("open" or "closed") is too much to
carry around in migration etc.

> > If neither of those are used, then a bit more context from QMP is
> > needed, such as the exact number of bytes transmitted prior to the
> > reset, presumably in a virtio-serial close event.
> 
> Yeah; that's some state to maintain -- and I'm not sure we should do
> that.

I'd rather not count bytes anyway.  That smacks too much of
maintaining long term history. I'd rather it was 1 bit of state, and
the host app connection handler able to use it.

> If we start adding some state now, we could end up adding more
> later, and it could soon become a mess.

Each thing ought to be weighed on its merits.

Without this specific thing, which is an indicator that guest has lost
state outside its control, the guest<->host communication is
unreliable (even for things like "cut and paste"), so every app that
cares has to implement a packet framing protocol with no binary data
(to reserve an escaping byte), or with CRCs like
PPP-over-virtio-serial, which is complicated and silly imho.  If it
were a real serial port, not emulated, that's the sort of thing apps
would actually do (or use timeouts, which are more dubious in
emulator-land).  But I hope we're not that sadistic :-)

*Inband* open/close indication aren't 100% guarantees of reliability,
but I think they raise it to the point where an app can usefully count
on it.

-- Jamie

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Amit Shah]

On (Fri) Mar 26 2010 [14:44:23], Jamie Lokier wrote:
> Amit Shah wrote:
> > Problem is we're going to have to maintain a lot of state if we're going
> > to provide guarantees.
> > 
> > One solution is to always have an in-qemu user of the serial
> > functionality that sits between the app and the guest and the in-qemu
> > user can signal to the app about such things.
> 
> Isn't that what I suggested?  I don't mind how the app is signalled.
> I'm just thinking of the simplest way to do it.  It doesn't have to
> live in the virtio-serial driver, just be signalled somehow out of it.

You did? Well, then we agree on a solution :-)

> > > If communication with the external host app is over a local socket
> > > (AF_UNIX or AF_INET or mkpipe), qemu could just disconnect and
> > > reconnect whenever the guest does, which is perfectly logical and
> > > solves this.
> > 
> > The virtio-serial code cannot know what kind of a connection it has with
> > the host app.
> 
> True, but the qemu internal plumbing could pass an open/close event to
> that connection handler, for it to do whatever's appropriate.

Which means the qemu chardev. Is it smart enough? Dunno.

> I don't think that 1 bit state ("open" or "closed") is too much to
> carry around in migration etc.

If the app sits inside qemu then maintaining open/close isn't a problem
at all. In fact, guest and host open/closed state is already maintained
by virtio-serial.

> > > If neither of those are used, then a bit more context from QMP is
> > > needed, such as the exact number of bytes transmitted prior to the
> > > reset, presumably in a virtio-serial close event.
> > 
> > Yeah; that's some state to maintain -- and I'm not sure we should do
> > that.
> 
> I'd rather not count bytes anyway.  That smacks too much of
> maintaining long term history. I'd rather it was 1 bit of state, and
> the host app connection handler able to use it.
> 
> > If we start adding some state now, we could end up adding more
> > later, and it could soon become a mess.
> 
> Each thing ought to be weighed on its merits.
> 
> Without this specific thing, which is an indicator that guest has lost
> state outside its control, the guest<->host communication is
> unreliable (even for things like "cut and paste"), so every app that
> cares has to implement a packet framing protocol with no binary data
> (to reserve an escaping byte), or with CRCs like
> PPP-over-virtio-serial, which is complicated and silly imho.  If it
> were a real serial port, not emulated, that's the sort of thing apps
> would actually do (or use timeouts, which are more dubious in
> emulator-land).  But I hope we're not that sadistic :-)

I agree. So: ports have in-qemu users, they get guest_open /
guest_close callbacks and get data which they can pass on to external
apps. Looks like we're fine there?

> *Inband* open/close indication aren't 100% guarantees of reliability,
> but I think they raise it to the point where an app can usefully count
> on it.

		Amit

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Anthony Liguori]

On 03/26/2010 09:29 AM, Luiz Capitulino wrote:
> On Fri, 26 Mar 2010 18:56:20 +0530
> Amit Shah<amit.shah@redhat.com>  wrote:
>
>    
>> On (Fri) Mar 26 2010 [10:14:02], Luiz Capitulino wrote:
>>      
>>>>>> +
>>>>>> +VIRTIO_SERIAL
>>>>>> +-------------
>>>>>>              
>>>>>   It should be VIRTIO_SERIAL_ADD.
>>>>>            
>>>> What about other events that VIRTIO_SERIAL generates?
>>>>          
>>>   We don't address this problem currently, maybe an integration with qdev
>>> will do, but I have to think more about it.
>>>        
>> So should I just keep it as VIRTIO_SERIAL for now? With new events also
>> riding on this one?
>>      
>   I don't like this because with the current events code this will lead
> to confusion, as you're using a single event to notify different things.
>
>   My suggestion for the immediate term is to do what we have been doing so
> far, ie. call it VIRTIO_SERIAL_ADD. Worst case here is: we add a new way
> to group events which requires a new VIRTIO_SERIAL event, in this case we
> could emit both, the new VIRTIO_SERIAL and the old VIRTIO_SERIAL_ADD. The
> latter would be deprecated too.
>
>   Or, if you can wait I can _try_ to solve this problem next week, although
> I have no idea how hard this is going to be.
>
>   Any comments, Anthony?
>    

The virtio serial events bother me in a number of ways.  Ports being 
added should just be more generic (we should generate an event any time 
a device is added to a bus).

Port disconnected/reconnect ought to be something that we propagate via 
a char device but I understand the limitations of the current code.

I'd like to see us try to at least address the add/remove case better 
and we may just have to live with the connect/disconnect stuff.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Luiz Capitulino]

On Fri, 26 Mar 2010 20:13:09 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Fri) Mar 26 2010 [11:29:03], Luiz Capitulino wrote:
> > On Fri, 26 Mar 2010 18:56:20 +0530
> > Amit Shah <amit.shah@redhat.com> wrote:
> > 
> > > On (Fri) Mar 26 2010 [10:14:02], Luiz Capitulino wrote:
> > > > > > > +
> > > > > > > +VIRTIO_SERIAL
> > > > > > > +-------------
> > > > > > 
> > > > > >  It should be VIRTIO_SERIAL_ADD.
> > > > > 
> > > > > What about other events that VIRTIO_SERIAL generates?
> > > > 
> > > >  We don't address this problem currently, maybe an integration with qdev
> > > > will do, but I have to think more about it.
> > > 
> > > So should I just keep it as VIRTIO_SERIAL for now? With new events also
> > > riding on this one?
> > 
> >  I don't like this because with the current events code this will lead
> > to confusion, as you're using a single event to notify different things.
> > 
> >  My suggestion for the immediate term is to do what we have been doing so
> > far, ie. call it VIRTIO_SERIAL_ADD. Worst case here is: we add a new way
> > to group events which requires a new VIRTIO_SERIAL event, in this case we
> > could emit both, the new VIRTIO_SERIAL and the old VIRTIO_SERIAL_ADD. The
> > latter would be deprecated too.
> 
> I've no problem doing it either way - whatever you prefer is fine.
> 
> BTW these are two distinct events already - failure in initialising a
> device and failure in initialising a port. Do you think these should be
> separate as well?

 That depends on what you want clients to know/do about it.

 Can ports and devices be added and work independently of each other?
Why is it relevant for a client to know that a _device_ has failed to
initialize?

 If clients connect to a port and all they need to know is "Sorry, but
that port won't be available", then you don't even need to have a port/device
distinction in the event.

 Also note that events can be improved to include more information later,
if needed. So, the best approach is to include as less information as
possible (given that it satisfies current client needs, of course).

> >  Or, if you can wait I can _try_ to solve this problem next week, although
> > I have no idea how hard this is going to be.
> 
> I think it's cleaner to club everything; but basically I'll go with
> whatever you say. I've no problem waiting.

 It's definitely needed to group events some way, we just have to
find a good way to do it. Having each subsystem doing it its own way
is not what we want because of protocol consistency and related
problems.

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Amit Shah]

On (Fri) Mar 26 2010 [14:52:36], Luiz Capitulino wrote:
> > >  My suggestion for the immediate term is to do what we have been doing so
> > > far, ie. call it VIRTIO_SERIAL_ADD. Worst case here is: we add a new way
> > > to group events which requires a new VIRTIO_SERIAL event, in this case we
> > > could emit both, the new VIRTIO_SERIAL and the old VIRTIO_SERIAL_ADD. The
> > > latter would be deprecated too.
> > 
> > I've no problem doing it either way - whatever you prefer is fine.
> > 
> > BTW these are two distinct events already - failure in initialising a
> > device and failure in initialising a port. Do you think these should be
> > separate as well?
> 
>  That depends on what you want clients to know/do about it.
> 
>  Can ports and devices be added and work independently of each other?
> Why is it relevant for a client to know that a _device_ has failed to
> initialize?

I'm not sure what you mean by a client, but let's say libvirt handles
the qemu session. A single device can have multiple ports. If a device
fails to register *in the guest*, all the ports associated with that
device could be hot-unplugged on the host to reduce host resource usage.

If just a single port fails to register *in the guest*, libvirt may just
want to hot-unplug it to free up resources.

So yes, I think both are necessary.

Anyway, I guess the answer is to split both these events.

>  If clients connect to a port and all they need to know is "Sorry, but
> that port won't be available", then you don't even need to have a port/device
> distinction in the event.
> 
>  Also note that events can be improved to include more information later,
> if needed. So, the best approach is to include as less information as
> possible (given that it satisfies current client needs, of course).

Right; that's the reason only the failing port number is given right
now.

> > >  Or, if you can wait I can _try_ to solve this problem next week, although
> > > I have no idea how hard this is going to be.
> > 
> > I think it's cleaner to club everything; but basically I'll go with
> > whatever you say. I've no problem waiting.
> 
>  It's definitely needed to group events some way, we just have to
> find a good way to do it. Having each subsystem doing it its own way
> is not what we want because of protocol consistency and related
> problems.

Yes, that's exactly why I think waiting till you iron it out would help.

		Amit

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Jamie Lokier]

Amit Shah wrote:
> > Without this specific thing, which is an indicator that guest has lost
> > state outside its control, the guest<->host communication is
> > unreliable (even for things like "cut and paste"), so every app that
> > cares has to implement a packet framing protocol with no binary data
> > (to reserve an escaping byte), or with CRCs like
> > PPP-over-virtio-serial, which is complicated and silly imho.  If it
> > were a real serial port, not emulated, that's the sort of thing apps
> > would actually do (or use timeouts, which are more dubious in
> > emulator-land).  But I hope we're not that sadistic :-)
> 
> I agree. So: ports have in-qemu users, they get guest_open /
> guest_close callbacks and get data which they can pass on to external
> apps. Looks like we're fine there?

Provided the guest_open / guest_close callbacks are synchronous with
the data - so that data sent/received following guest_open exactly
matches what the guest sends/receives from its beginning, that
internal interface looks fine to me.

We can tidy up the chardev later as needed :-)

-- Jamie

> > *Inband* open/close indication aren't 100% guarantees of reliability,
> > but I think they raise it to the point where an app can usefully count
> > on it.

Re: [Qemu-devel] Re: [PATCH 10/15] virtio-serial: Add QMP events for failed port/device add

[Luiz Capitulino]

On Sat, 27 Mar 2010 13:33:22 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Fri) Mar 26 2010 [14:52:36], Luiz Capitulino wrote:
> > > >  My suggestion for the immediate term is to do what we have been doing so
> > > > far, ie. call it VIRTIO_SERIAL_ADD. Worst case here is: we add a new way
> > > > to group events which requires a new VIRTIO_SERIAL event, in this case we
> > > > could emit both, the new VIRTIO_SERIAL and the old VIRTIO_SERIAL_ADD. The
> > > > latter would be deprecated too.
> > > 
> > > I've no problem doing it either way - whatever you prefer is fine.
> > > 
> > > BTW these are two distinct events already - failure in initialising a
> > > device and failure in initialising a port. Do you think these should be
> > > separate as well?
> > 
> >  That depends on what you want clients to know/do about it.
> > 
> >  Can ports and devices be added and work independently of each other?
> > Why is it relevant for a client to know that a _device_ has failed to
> > initialize?
> 
> I'm not sure what you mean by a client, but let's say libvirt handles
> the qemu session.

 Client is any application talking to QEMU through QMP.

> A single device can have multiple ports. If a device
> fails to register *in the guest*, all the ports associated with that
> device could be hot-unplugged on the host to reduce host resource usage.
> 
> If just a single port fails to register *in the guest*, libvirt may just
> want to hot-unplug it to free up resources.
> 
> So yes, I think both are necessary.
> 
> Anyway, I guess the answer is to split both these events.

 Yes, that makes sense.

[...]

> > > >  Or, if you can wait I can _try_ to solve this problem next week, although
> > > > I have no idea how hard this is going to be.
> > > 
> > > I think it's cleaner to club everything; but basically I'll go with
> > > whatever you say. I've no problem waiting.
> > 
> >  It's definitely needed to group events some way, we just have to
> > find a good way to do it. Having each subsystem doing it its own way
> > is not what we want because of protocol consistency and related
> > problems.
> 
> Yes, that's exactly why I think waiting till you iron it out would help.

 Ok.

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

[Juan Quintela]

Amit Shah <amit.shah@redhat.com> 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 |   34 +++++++++++++++++++++++++++++++---
>  1 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index bd1223e..3edfeca 100644
>      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);
> +        size_t cur_len, copied;
> +
> +        cur_len = iov_size(elem.out_sg, elem.out_num);
> +        /*
> +         * 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);
> +            len = cur_len;
> +        }

This can be simplified to only allocate the buffer if it is less no?

        if (cur_len > len) {
            if (len) {
                qemu_free(buf);
            }
            buf = qemu_malloc(cur_len);
            len = cur_len;
        }

This way we can elliminate allocations, no?

Later, Juan.

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

[Amit Shah]

On (Tue) Mar 30 2010 [15:44:21], Juan Quintela wrote:
> Amit Shah <amit.shah@redhat.com> 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 |   34 +++++++++++++++++++++++++++++++---
> >  1 files changed, 31 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> > index bd1223e..3edfeca 100644
> >      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);
> > +        size_t cur_len, copied;
> > +
> > +        cur_len = iov_size(elem.out_sg, elem.out_num);
> > +        /*
> > +         * 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);
> > +            len = cur_len;
> > +        }
> 
> This can be simplified to only allocate the buffer if it is less no?

Currently all the control messages are the same size, sizeof(struct
virtio_console_control), so it wouldn't matter.

But I guess this could be done, just have to ensure we don't leak data
meant for one control message to another.

		Amit