[Qemu-devel] virtio-console-bus, multiport, virtio-console-port

[Qemu-devel] virtio-console-bus, multiport, virtio-console-port

[Amit Shah]

Hello,

This patch series converts the virtio-console device to qdev, adds a
virtio-console bus on which similar devices can ride and shows how such
devices can be used by adding a new virtio-console-port device and also
a virtio-console-vnc device.

This is working well in my testing. I tested old kernel/new qemu, new
kernel/old qemu and new kernel/new qemu.

The ports are also tested by spawning two virtio consoles and one
generic port. Heavy activity on all three ports is sustained for quite a
long time with correct results.

Please review and apply.

[Qemu-devel] [PATCH 1/6] char: Emit 'OPENED' events on char device open

[Amit Shah]

Notify users of the char interface whenever the file / connection is
opened.

The existing RESET event is triggered when the qemu char state is reset
as well; which may not be as interesting as char device open events.

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

diff --git a/qemu-char.c b/qemu-char.c
index 8084a67..e7d091c 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -479,6 +479,8 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
     chr->chr_write = mux_chr_write;
     chr->chr_update_read_handler = mux_chr_update_read_handler;
     chr->chr_accept_input = mux_chr_accept_input;
+
+    qemu_chr_event(chr, CHR_EVENT_OPENED);
     return chr;
 }
 
@@ -627,6 +629,7 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
     chr->chr_close = fd_chr_close;
 
     qemu_chr_reset(chr);
+    qemu_chr_event(chr, CHR_EVENT_OPENED);
 
     return chr;
 }
@@ -925,8 +928,10 @@ static void pty_chr_state(CharDriverState *chr, int connected)
          * the virtual device linked to our pty. */
         qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 1000);
     } else {
-        if (!s->connected)
+        if (!s->connected) {
             qemu_chr_reset(chr);
+            qemu_chr_event(chr, CHR_EVENT_OPENED);
+        }
         s->connected = 1;
     }
 }
@@ -1166,6 +1171,7 @@ static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
     }
     chr->chr_ioctl = tty_serial_ioctl;
     qemu_chr_reset(chr);
+    qemu_chr_event(chr, CHR_EVENT_OPENED);
     return chr;
 }
 #else  /* ! __linux__ && ! __sun__ */
@@ -1312,6 +1318,7 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
     chr->opaque = drv;
 
     qemu_chr_reset(chr);
+    qemu_chr_event(chr, CHR_EVENT_OPENED);
 
     return chr;
 }
@@ -1593,6 +1600,7 @@ static CharDriverState *qemu_chr_open_win(QemuOpts *opts)
         return NULL;
     }
     qemu_chr_reset(chr);
+    qemu_chr_event(chr, CHR_EVENT_OPENED);
     return chr;
 }
 
@@ -1693,6 +1701,7 @@ static CharDriverState *qemu_chr_open_win_pipe(QemuOpts *opts)
         return NULL;
     }
     qemu_chr_reset(chr);
+    qemu_chr_event(chr, CHR_EVENT_OPENED);
     return chr;
 }
 
@@ -1707,6 +1716,7 @@ static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out)
     chr->opaque = s;
     chr->chr_write = win_chr_write;
     qemu_chr_reset(chr);
+    qemu_chr_event(chr, CHR_EVENT_OPENED);
     return chr;
 }
 
@@ -1828,6 +1838,7 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
     chr->chr_write = udp_chr_write;
     chr->chr_update_read_handler = udp_chr_update_read_handler;
     chr->chr_close = udp_chr_close;
+    qemu_chr_event(chr, CHR_EVENT_OPENED);
     return chr;
 
 return_err:
@@ -2034,6 +2045,7 @@ static void tcp_chr_connect(void *opaque)
     qemu_set_fd_handler2(s->fd, tcp_chr_read_poll,
                          tcp_chr_read, NULL, chr);
     qemu_chr_reset(chr);
+    qemu_chr_event(chr, CHR_EVENT_OPENED);
 }
 
 #define IACSET(x,a,b,c) x[0] = a; x[1] = b; x[2] = c;
diff --git a/qemu-char.h b/qemu-char.h
index c0654bc..43e61e0 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -14,6 +14,7 @@
 #define CHR_EVENT_MUX_IN  3 /* mux-focus was set to this terminal */
 #define CHR_EVENT_MUX_OUT 4 /* mux-focus will move on */
 #define CHR_EVENT_CLOSED  5 /* connection closed */
+#define CHR_EVENT_OPENED  6 /* connection opened */
 
 
 #define CHR_IOCTL_SERIAL_SET_PARAMS   1
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/6] qdev: add string property.

[Amit Shah]

From: Gerd Hoffmann <kraxel@redhat.com>

---
 hw/qdev-properties.c |   28 ++++++++++++++++++++++++++++
 hw/qdev.h            |    4 ++++
 2 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 2ecb58d..7e085f6 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -165,6 +165,34 @@ PropertyInfo qdev_prop_hex64 = {
     .print = print_hex64,
 };
 
+/* --- string --- */
+
+static int parse_string(DeviceState *dev, Property *prop, const char *str)
+{
+    char **ptr = qdev_get_prop_ptr(dev, prop);
+
+    if (*ptr)
+        qemu_free(*ptr);
+    *ptr = qemu_strdup(str);
+    return 0;
+}
+
+static int print_string(DeviceState *dev, Property *prop, char *dest, size_t len)
+{
+    char **ptr = qdev_get_prop_ptr(dev, prop);
+    if (!*ptr)
+        return snprintf(dest, len, "<null>");
+    return snprintf(dest, len, "\"%s\"", *ptr);
+}
+
+PropertyInfo qdev_prop_string = {
+    .name  = "string",
+    .type  = PROP_TYPE_STRING,
+    .size  = sizeof(char*),
+    .parse = parse_string,
+    .print = print_string,
+};
+
 /* --- drive --- */
 
 static int parse_drive(DeviceState *dev, Property *prop, const char *str)
diff --git a/hw/qdev.h b/hw/qdev.h
index 623ded5..cd9da17 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -68,6 +68,7 @@ enum PropertyType {
     PROP_TYPE_MACADDR,
     PROP_TYPE_DRIVE,
     PROP_TYPE_CHR,
+    PROP_TYPE_STRING,
     PROP_TYPE_PTR,
 };
 
@@ -161,6 +162,7 @@ extern PropertyInfo qdev_prop_int32;
 extern PropertyInfo qdev_prop_uint64;
 extern PropertyInfo qdev_prop_hex32;
 extern PropertyInfo qdev_prop_hex64;
+extern PropertyInfo qdev_prop_string;
 extern PropertyInfo qdev_prop_chr;
 extern PropertyInfo qdev_prop_ptr;
 extern PropertyInfo qdev_prop_macaddr;
@@ -200,6 +202,8 @@ extern PropertyInfo qdev_prop_pci_devfn;
     DEFINE_PROP(_n, _s, _f, qdev_prop_ptr, void*)
 #define DEFINE_PROP_CHR(_n, _s, _f)             \
     DEFINE_PROP(_n, _s, _f, qdev_prop_chr, CharDriverState*)
+#define DEFINE_PROP_STRING(_n, _s, _f)             \
+    DEFINE_PROP(_n, _s, _f, qdev_prop_string, char*)
 #define DEFINE_PROP_DRIVE(_n, _s, _f)             \
     DEFINE_PROP(_n, _s, _f, qdev_prop_drive, DriveInfo*)
 #define DEFINE_PROP_MACADDR(_n, _s, _f)         \
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Amit Shah]

This patch migrates virtio-console to the qdev infrastructure, and
creates a new virtio-console bus on which multiple ports are exposed as
devices. The bulk of the code now resides in a new file with
virtio-console.c being just a simple qdev device.

This interface extends the virtio-console device to handle
multiple ports from which bits can be sent and read.

The older -virtioconsole argument is now deprecated in favour of:

    -device virtio-console-pci -device virtconsole

The virtconsole device type accepts a chardev as an argument and a 'name'
argument to identify the corresponding consoles on the host as well as the
guest. The name, if given, is exposed via the 'name' sysfs attribute.

Care has been taken to ensure compatibility with kernels that do not
support multiple ports.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 Makefile.target         |    2 +-
 hw/pc.c                 |    9 -
 hw/qdev.c               |    8 +-
 hw/virtio-console-bus.c |  738 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-console.c     |  148 +++-------
 hw/virtio-console.h     |  130 +++++++++
 qemu-options.hx         |    8 -
 sysemu.h                |    6 -
 vl.c                    |   37 ---
 9 files changed, 907 insertions(+), 179 deletions(-)
 create mode 100644 hw/virtio-console-bus.c

diff --git a/Makefile.target b/Makefile.target
index 609015b..22a2644 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -173,7 +173,7 @@ obj-y = vl.o monitor.o pci.o isa_mmio.o machine.o \
         gdbstub.o gdbstub-xml.o
 # virtio has to be here due to weird dependency between PCI and virtio-net.
 # need to fix this properly
-obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o virtio-pci.o
+obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o virtio-console-bus.o virtio-pci.o
 obj-$(CONFIG_KVM) += kvm.o kvm-all.o
 
 LIBS+=-lz
diff --git a/hw/pc.c b/hw/pc.c
index 240cfe0..70b1ce9 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1463,15 +1463,6 @@ static void pc_init1(ram_addr_t ram_size,
 	extboot_init(info->bdrv, 1);
     }
 
-    /* Add virtio console devices */
-    if (pci_enabled) {
-        for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
-            if (virtcon_hds[i]) {
-                pci_create_simple(pci_bus, -1, "virtio-console-pci");
-            }
-        }
-    }
-
 #ifdef CONFIG_KVM_DEVICE_ASSIGNMENT
     if (kvm_enabled()) {
         add_assigned_devices(pci_bus, assigned_devices, assigned_devices_index);
diff --git a/hw/qdev.c b/hw/qdev.c
index 43b1beb..07cb718 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -243,13 +243,9 @@ void qdev_free(DeviceState *dev)
 CharDriverState *qdev_init_chardev(DeviceState *dev)
 {
     static int next_serial;
-    static int next_virtconsole;
+
     /* FIXME: This is a nasty hack that needs to go away.  */
-    if (strncmp(dev->info->name, "virtio", 6) == 0) {
-        return virtcon_hds[next_virtconsole++];
-    } else {
-        return serial_hds[next_serial++];
-    }
+    return serial_hds[next_serial++];
 }
 
 BusState *qdev_get_parent_bus(DeviceState *dev)
diff --git a/hw/virtio-console-bus.c b/hw/virtio-console-bus.c
new file mode 100644
index 0000000..a5b732a
--- /dev/null
+++ b/hw/virtio-console-bus.c
@@ -0,0 +1,738 @@
+/*
+ * A bus for connecting virtio-console ports
+ *
+ * Copyright (c) 2009 Red Hat, Inc.
+ *
+ * Author(s):
+ *  Amit Shah <amit.shah@redhat.com>
+ *
+ * Some earlier parts are:
+ *   Copyright IBM, Corp. 2008
+ * authored by
+ *  Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "hw.h"
+#include "monitor.h"
+#include "qemu-queue.h"
+#include "qemu-char.h"
+#include "sysbus.h"
+#include "virtio.h"
+#include "virtio-console.h"
+
+typedef struct VirtIOConsole
+{
+    VirtIODevice vdev;
+    VirtQueue *ivq, *ovq;
+
+    VirtConDevice* ports[MAX_VIRTIO_CONSOLE_PORTS];
+    struct virtio_console_config config;
+
+    uint32_t guest_features;
+} VirtIOConsole;
+
+/* This struct holds individual buffers received for each port */
+typedef struct VirtConPortBuffer {
+    QTAILQ_ENTRY(VirtConPortBuffer) next;
+
+    uint8_t *buf;
+
+    size_t len; /* length of the buffer */
+
+    /* The size of one write request as issued by the guest. The
+     * buffer could be split in this list but using the size value in
+     * the first buffer for each write we can identify complete
+     * writes
+     */
+    size_t size;
+} VirtConPortBuffer;
+
+
+static VirtIOConsole *virtio_console;
+
+static VirtConPort *get_port_from_id(VirtIOConsole *vcon, uint32_t id)
+{
+    VirtConDevice *dev;
+    VirtConPort *port;
+
+    if (id > MAX_VIRTIO_CONSOLE_PORTS)
+        return NULL;
+
+    dev = vcon->ports[id];
+    port = DO_UPCAST(VirtConPort, dev, &dev->qdev);
+    return port;
+}
+
+static uint32_t get_id_from_port(VirtConPort *port)
+{
+    return port->id;
+}
+
+static bool use_multiport(void)
+{
+    return virtio_console->guest_features & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
+}
+
+static bool is_internal(uint32_t flags)
+{
+    return flags & VIRTIO_CONSOLE_ID_INTERNAL;
+}
+
+static bool has_complete_data(VirtConPort *port)
+{
+    VirtConPortBuffer *buf;
+    size_t len, size;
+
+    len = 0;
+    size = 0;
+    QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) {
+        if (!buf->size && buf == QTAILQ_FIRST(&port->unflushed_buffer_head)) {
+            /* We have a buffer that's lost its way; just flush it */
+            return true;
+        }
+        if (size && buf->size) {
+            /* Start of the next write request */
+            return true;
+        }
+        if (buf->size) {
+            size = buf->size;
+        }
+        len += buf->len;
+        if (len == size) {
+            return true;
+        }
+    }
+    return false;
+}
+
+static void flush_queue(VirtConPort *port)
+{
+    VirtConPortBuffer *buf, *buf2;
+    uint8_t *outbuf;
+    size_t outlen, outsize;
+
+    /*
+     * If the app isn't interested in buffering packets till it's
+     * opened, just drop the data guest sends us till a connection is
+     * established.
+     */
+    if (!port->host_connected && !port->flush_buffers)
+        return;
+
+    while (!QTAILQ_EMPTY(&port->unflushed_buffer_head)) {
+        if (!has_complete_data(port)) {
+            break;
+        }
+
+        buf = QTAILQ_FIRST(&port->unflushed_buffer_head);
+        if (!buf->size) {
+            /* This is a buf that didn't get consumed as part of a
+             * previous data stream. Bad thing, shouldn't
+             * happen. But let's handle it nonetheless
+             */
+            port->info->have_data(port, buf->buf, buf->len);
+            QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next);
+            qemu_free(buf->buf);
+            qemu_free(buf);
+            continue;
+        }
+
+        outlen = 0;
+        outsize = buf->size;
+        outbuf = qemu_mallocz(outsize);
+        QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffer_head, next, buf2) {
+            memcpy(outbuf + outlen, buf->buf, buf->len);
+            outlen += buf->len;
+            QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next);
+            qemu_free(buf->buf);
+            qemu_free(buf);
+
+            if (outlen == outsize)
+                break;
+        }
+        if (port->host_connected) {
+            port->info->have_data(port, outbuf, outlen);
+        }
+        qemu_free(outbuf);
+    }
+}
+
+
+static size_t write_to_port(VirtConPort *port,
+                            const uint8_t *buf, size_t size, uint32_t flags)
+{
+    VirtQueue *vq = virtio_console->ivq;
+    VirtQueueElement elem;
+    size_t offset = 0;
+    size_t len = 0;
+
+    if (!virtio_queue_ready(vq)) {
+        return 0;
+    }
+
+    if (!use_multiport() && is_internal(flags)) {
+        return 0;
+    }
+
+    while (offset < size) {
+        struct virtio_console_header header;
+        int i, header_len;
+
+        header_len = use_multiport() ? sizeof(header) : 0;
+
+        if (!virtqueue_pop(vq, &elem)) {
+            break;
+        }
+        if (elem.in_sg[0].iov_len < header_len) {
+            /* We can't even store our port number in this buffer. Bug? */
+            qemu_error("virtio-console: size %zd less than expected\n",
+                    elem.in_sg[0].iov_len);
+            exit(1);
+        }
+        header.id = get_id_from_port(port);
+        header.flags = flags;
+        memcpy(elem.in_sg[0].iov_base, &header, header_len);
+
+        for (i = 0; offset < size && i < elem.in_num; i++) {
+            len = MIN(elem.in_sg[i].iov_len - header_len, size - offset);
+
+            memcpy(elem.in_sg[i].iov_base + header_len, buf + offset, len);
+            offset += len;
+            header_len = 0;
+        }
+        header_len = use_multiport() ? sizeof(header) : 0;
+        virtqueue_push(vq, &elem, len + header_len);
+    }
+    virtio_notify(&virtio_console->vdev, vq);
+    return offset;
+}
+
+static void send_control_event(VirtConPort *port,
+                               struct virtio_console_control *cpkt)
+{
+    write_to_port(port, (uint8_t *)cpkt, sizeof(*cpkt),
+                  VIRTIO_CONSOLE_ID_INTERNAL);
+}
+
+
+
+/* Functions for use inside qemu to open and read from/write to ports */
+int virtio_console_open(VirtConPort *port)
+{
+    struct virtio_console_control cpkt;
+
+    /* Don't allow opening an already-open port */
+    if (port->host_connected) {
+        return 0;
+    }
+
+    /* Send port open notification to the guest */
+    port->host_connected = true;
+    cpkt.event = VIRTIO_CONSOLE_PORT_OPEN;
+    cpkt.value = 1;
+    send_control_event(port, &cpkt);
+
+    /* Flush any buffers that were pending while the port was closed */
+    flush_queue(port);
+
+    return 0;
+}
+
+void virtio_console_close(VirtConPort *port)
+{
+    struct virtio_console_control cpkt;
+
+    port->host_connected = false;
+    cpkt.event = VIRTIO_CONSOLE_PORT_OPEN;
+    cpkt.value = 0;
+    send_control_event(port, &cpkt);
+}
+
+/*
+ * Individual ports call this function to write to the guest.
+ */
+size_t virtio_console_write(VirtConPort *port, const uint8_t *buf, size_t size)
+{
+    if (!port || !port->host_connected) {
+        return 0;
+    }
+    return write_to_port(port, buf, size, false);
+}
+
+
+/* Guest wants to notify us of some event */
+static void handle_control_message(VirtConPort *port,
+                                   struct virtio_console_control *cpkt)
+{
+    uint8_t *buffer;
+    size_t buffer_len;
+
+    switch(cpkt->event) {
+    case VIRTIO_CONSOLE_PORT_OPEN:
+        port->guest_connected = cpkt->value;
+        if (cpkt->value && port->info->guest_open) {
+            port->info->guest_open(port);
+        }
+        if (!cpkt->value && port->info->guest_close) {
+            port->info->guest_close(port);
+        }
+        break;
+    case VIRTIO_CONSOLE_PORT_NAME:
+        if (port->name) {
+            buffer_len = sizeof(*cpkt) + strlen(port->name) + 1;
+            buffer = qemu_malloc(buffer_len);
+
+            memcpy(buffer, cpkt, sizeof(*cpkt));
+            memcpy(buffer + sizeof(*cpkt), port->name, strlen(port->name));
+            buffer[buffer_len - 1] = 0;
+
+            write_to_port(port, buffer, buffer_len, VIRTIO_CONSOLE_ID_INTERNAL);
+            qemu_free(buffer);
+        }
+        /*
+         * Now that we know the guest asked for the port name, we're
+         * sure the guest has initialised whatever state is necessary
+         * for this port. Now's a good time to let the guest know if
+         * this port is a console port so that the guest can hook it
+         * up to hvc.
+         */
+        if (port->is_console) {
+            cpkt->event = VIRTIO_CONSOLE_CONSOLE_PORT;
+            cpkt->value = true;
+            send_control_event(port, cpkt);
+        }
+        break;
+    }
+}
+
+/* Guest wrote something to some port.
+ *
+ * Flush the data in the entire chunk that we received rather than
+ * splitting it into multiple buffers. VNC clients don't consume split
+ * buffers
+ */
+static void virtio_console_handle_output(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIOConsole *vcon;
+    VirtQueueElement elem;
+
+    vcon = DO_UPCAST(VirtIOConsole, vdev, vdev);
+
+    while (virtqueue_pop(vq, &elem)) {
+        VirtConPort *port;
+        VirtConPortBuffer *buf;
+        struct virtio_console_header header;
+        int header_len;
+
+        buf = qemu_mallocz(sizeof(*buf));
+
+        if (use_multiport()) {
+            header_len = sizeof(header);
+
+            memcpy(&header, elem.out_sg[0].iov_base, header_len);
+            port = get_port_from_id(vcon, header.id);
+            if (!port) {
+                qemu_free(buf);
+                goto next_buf;
+            }
+        } else {
+            header_len = 0;
+            port = get_port_from_id(vcon, 0);
+        }
+
+        /* The guest always sends only one sg */
+        buf->len = elem.out_sg[0].iov_len - header_len;
+        buf->buf = qemu_mallocz(buf->len);
+        memcpy(buf->buf, elem.out_sg[0].iov_base + header_len, buf->len);
+
+        if (use_multiport() && is_internal(header.flags)) {
+            handle_control_message(port,
+                                   (struct virtio_console_control *)buf->buf);
+            qemu_free(buf->buf);
+            qemu_free(buf);
+            goto next_buf;
+        }
+        /*
+         * A port may not have any handler registered for consuming the
+         * data that the guest sends or it may not have a chardev associated
+         * with it. Just ignore the data in that case
+         */
+        if (!port->info->have_data) {
+            goto next_buf;
+        }
+
+        QTAILQ_INSERT_TAIL(&port->unflushed_buffer_head, buf, next);
+        if (use_multiport()) {
+            /* Only the first buffer in a stream will have this
+             * set. This will help us identify the first buffer and
+             * the remaining buffers in the stream based on length
+             */
+            buf->size = header.size;
+        } else {
+            /* We always want to flush all the buffers in this case */
+            buf->size = buf->len;
+        }
+        flush_queue(port);
+    next_buf:
+        virtqueue_push(vq, &elem, elem.out_sg[0].iov_len);
+    }
+    virtio_notify(vdev, vq);
+}
+
+static void virtio_console_handle_input(VirtIODevice *vdev, VirtQueue *vq)
+{
+}
+
+static uint32_t virtio_console_get_features(VirtIODevice *vdev)
+{
+    return 1 << VIRTIO_CONSOLE_F_MULTIPORT;
+}
+
+static void virtio_console_set_features(VirtIODevice *vdev, uint32_t features)
+{
+    virtio_console->guest_features = features;
+}
+
+/* Guest requested config info */
+static void virtio_console_get_config(VirtIODevice *vdev, uint8_t *config_data)
+{
+    VirtIOConsole *vcon;
+
+    vcon = DO_UPCAST(VirtIOConsole, vdev, vdev);
+    memcpy(config_data, &vcon->config, sizeof(struct virtio_console_config));
+}
+
+static void virtio_console_set_config(VirtIODevice *vdev,
+                                      const uint8_t *config_data)
+{
+    struct virtio_console_config config;
+
+    memcpy(&config, config_data, sizeof(config));
+}
+
+/* Readiness of the guest to accept data on a port */
+static int vcon_can_read(void *opaque)
+{
+    VirtConPort *port = opaque;
+    VirtQueue *vq = virtio_console->ivq;
+    int size, header_len;
+
+    if (use_multiport()) {
+        header_len = sizeof(struct virtio_console_header);
+    } else {
+        header_len = 0;
+    }
+
+    if (!virtio_queue_ready(vq) ||
+        !(virtio_console->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
+        virtio_queue_empty(vq)) {
+        return 0;
+    }
+    if (use_multiport() && !port->guest_connected) {
+        return 0;
+    }
+    size = TARGET_PAGE_SIZE;
+    if (virtqueue_avail_bytes(vq, size, 0)) {
+        return size - header_len;
+    }
+    size = header_len + 1;
+    if (virtqueue_avail_bytes(vq, size, 0)) {
+        return size - header_len;
+    }
+    return 0;
+}
+
+/* Send data from a char device over to the guest */
+static void vcon_read(void *opaque, const uint8_t *buf, int size)
+{
+    VirtConPort *port = opaque;
+
+    virtio_console_write(port, buf, size);
+}
+
+static void vcon_event(void *opaque, int event)
+{
+    VirtConPort *port = opaque;
+
+    switch (event) {
+    case CHR_EVENT_OPENED: {
+        virtio_console_open(port);
+        break;
+    }
+    case CHR_EVENT_CLOSED:
+        virtio_console_close(port);
+        break;
+    }
+}
+
+static void set_active_in_map(uint32_t *map, uint32_t idx)
+{
+    int i;
+
+    i = idx / 32;
+    idx %= 32;
+
+    map[i] |= 1U << idx;
+}
+
+/*
+ * Get the guest_connected status after a migration since it's all
+ * packed in the same way as ports_map in the config space is packed.
+ *
+ * return VIRTIO_CONSOLE_BAD_ID if nothing was found in the map, else
+ * return the port number whose guest was connected before the
+ * migration
+ */
+static uint32_t find_next_active_in_map(uint32_t *map, unsigned int i)
+{
+    uint32_t port_nr;
+
+    port_nr = ffs(*map);
+    if (!port_nr) {
+        return VIRTIO_CONSOLE_BAD_ID;
+    }
+    /* We used ffs above */
+    port_nr--;
+
+    *map &= ~(1U << port_nr);
+
+    port_nr += i * 32;
+    return port_nr;
+}
+
+static void virtio_console_save(QEMUFile *f, void *opaque)
+{
+    VirtIOConsole *s = opaque;
+    VirtConPort *port;
+    uint32_t guest_connected_map[MAX_VIRTIO_CONSOLE_PORTS / 32];
+    unsigned int i, nr_bufs;
+
+    /* The virtio device */
+    virtio_save(&s->vdev, f);
+    /* The config space */
+    qemu_put_be16s(f, &s->config.cols);
+    qemu_put_be16s(f, &s->config.rows);
+    qemu_put_be32s(f, &s->config.nr_active_ports);
+
+    /* Items in struct VirtIOConsole */
+    qemu_put_be32s(f, &s->guest_features);
+
+    /*
+     * Items in struct VirtConPort
+     *   guest_connected is a single bit in each port; pack all these
+     *   bits in uint32_t words and then send them across
+     */
+    for (i = 0; i < s->config.nr_active_ports / 32; i++) {
+        guest_connected_map[i] = 0;
+    }
+    for (i = 0; i < s->config.nr_active_ports; i++) {
+        port = get_port_from_id(s, i);
+        if (port->guest_connected) {
+            set_active_in_map(guest_connected_map, i);
+        }
+    }
+    for (i = 0; i < s->config.nr_active_ports / 32; i++) {
+        qemu_put_be32s(f, &guest_connected_map[i]);
+    }
+
+    /* All the pending buffers from active ports */
+    for (i = 0; i < s->config.nr_active_ports; i++) {
+        VirtConPortBuffer *buf;
+
+        nr_bufs = 0;
+        port = get_port_from_id(s, i);
+        QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) {
+            nr_bufs++;
+        }
+        /* First the port number, then the nr of bufs and then the bufs */
+        qemu_put_be32s(f, &i);
+        qemu_put_be32s(f, &nr_bufs);
+        if (!nr_bufs) {
+            continue;
+        }
+        QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) {
+            qemu_put_be64s(f, &buf->len);
+            qemu_put_be64s(f, &buf->size);
+            qemu_put_buffer(f, buf->buf, buf->len);
+        }
+    }
+}
+
+static int virtio_console_load(QEMUFile *f, void *opaque, int version_id)
+{
+    VirtIOConsole *s = opaque;
+    VirtConPort *port;
+    uint32_t guest_connected_map[MAX_VIRTIO_CONSOLE_PORTS / 32];
+    unsigned int i;
+
+    if (version_id > 2)
+        return -EINVAL;
+
+    /* The virtio device */
+    virtio_load(&s->vdev, f);
+
+    if (version_id < 2)
+        return 0;
+
+    /* The config space */
+    qemu_get_be16s(f, &s->config.cols);
+    qemu_get_be16s(f, &s->config.rows);
+    s->config.nr_active_ports = qemu_get_be32(f);
+
+    /* Items in struct VirtIOConsole */
+    qemu_get_be32s(f, &virtio_console->guest_features);
+
+    /* Items in struct VirtConPort */
+    for (i = 0; i < s->config.nr_active_ports / 32; i++) {
+        guest_connected_map[i] = qemu_get_be32(f);
+    }
+
+    for (i = 0; i < s->config.nr_active_ports / 32; i++) {
+        uint32_t port_nr, map;
+
+        port = get_port_from_id(s, i);
+        map = guest_connected_map[i];
+        while (1) {
+            port_nr = find_next_active_in_map(&map, i);
+            if (port_nr == VIRTIO_CONSOLE_BAD_ID) {
+                break;
+            }
+            port->guest_connected = true;
+        }
+    }
+
+    /* All the pending buffers from active ports */
+    for (i = 0; i < s->config.nr_active_ports; i++) {
+        VirtConPortBuffer *buf;
+        unsigned int nr, nr_bufs;
+
+        /* First the port number, then the nr of bufs and then the bufs */
+        qemu_get_be32s(f, &nr);
+        qemu_get_be32s(f, &nr_bufs);
+        if (!nr_bufs) {
+            continue;
+        }
+        port = get_port_from_id(s, nr);
+        for (; nr_bufs; nr_bufs--) {
+            buf = qemu_malloc(sizeof(*buf));
+
+            qemu_get_be64s(f, &buf->len);
+            qemu_get_be64s(f, &buf->size);
+            buf->buf = qemu_malloc(buf->len);
+            qemu_get_buffer(f, buf->buf, buf->len);
+            QTAILQ_INSERT_TAIL(&port->unflushed_buffer_head, buf, next);
+        }
+    }
+
+    return 0;
+}
+
+/* The virtio-console bus on top of which the ports will ride as devices */
+struct VirtConBus {
+    BusState qbus;
+//    uint32_t assigned;
+};
+static VirtConBus *virtcon_bus;
+
+static struct BusInfo virtcon_bus_info = {
+    .name      = "virtio-console-bus",
+    .size      = sizeof(VirtConBus),
+};
+
+static VirtConBus *virtcon_bus_new(DeviceState *dev)
+{
+    if (virtcon_bus) {
+        qemu_error("Can't create a second virtio-console bus\n");
+        return NULL;
+    }
+
+    virtcon_bus = FROM_QBUS(VirtConBus, qbus_create(&virtcon_bus_info, dev, NULL));
+    return virtcon_bus;
+}
+
+static int virtcon_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
+{
+    VirtConDevice *dev = DO_UPCAST(VirtConDevice, qdev, qdev);
+    VirtConPortInfo *info = DO_UPCAST(VirtConPortInfo, qdev, base);
+    VirtConPort *port = DO_UPCAST(VirtConPort, dev, &dev->qdev);
+    int ret;
+
+    if (virtio_console->config.nr_active_ports == MAX_VIRTIO_CONSOLE_PORTS) {
+        qemu_error("virtio-console-bus: Maximum device limit reached\n");
+        return -1;
+    }
+
+    dev->info = info;
+
+    ret = info->init(dev);
+    if (ret) {
+        return ret;
+    }
+    QTAILQ_INIT(&port->unflushed_buffer_head);
+
+    if (port->chr) {
+        qemu_chr_add_handlers(port->chr, vcon_can_read, vcon_read, vcon_event,
+                              port);
+    }
+    port->id = virtio_console->config.nr_active_ports++;
+    virtio_console->ports[port->id] = dev;
+    /* Send an update to the guest about this new port added */
+    virtio_notify_config(&virtio_console->vdev);
+
+    return ret;
+}
+
+void virtcon_port_qdev_register(VirtConPortInfo *info)
+{
+    info->qdev.init = virtcon_port_qdev_init;
+    info->qdev.bus_info = &virtcon_bus_info;
+    qdev_register(&info->qdev);
+}
+
+VirtIODevice *virtio_console_init(DeviceState *dev)
+{
+    VirtIOConsole *s;
+
+    if (virtio_console) {
+        /*
+         * Linux guests don't support more than one virtio-console devices
+         * at the moment
+         */
+        return NULL;
+    }
+
+    if (MAX_VIRTIO_CONSOLE_PORTS % 32) {
+        /* We require MAX_VIRTIO_CONSOLE_PORTS be a multiple of 32:
+         * We anyway use up that much space for the bitmap and it
+         * simplifies some calculations
+         */
+        return NULL;
+    }
+
+    s = (VirtIOConsole *)virtio_common_init("virtio-console",
+                                            VIRTIO_ID_CONSOLE,
+                                            sizeof(struct virtio_console_config),
+                                            sizeof(VirtIOConsole));
+
+    virtio_console = s;
+    s->vdev.get_features = virtio_console_get_features;
+    s->vdev.set_features = virtio_console_set_features;
+    s->vdev.get_config = virtio_console_get_config;
+    s->vdev.set_config = virtio_console_set_config;
+
+    /* Add queue for host to guest transfers */
+    s->ivq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_input);
+    /* Add queue for guest to host transfers */
+    s->ovq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_output);
+
+    register_savevm("virtio-console", -1, 2, virtio_console_save,
+                    virtio_console_load, s);
+
+    /* Spawn a new virtio-console bus on which the ports will ride as devices */
+    virtcon_bus_new(dev);
+
+    return &s->vdev;
+}
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 92c953c..8007279 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -1,10 +1,10 @@
 /*
  * Virtio Console Device
  *
- * Copyright IBM, Corp. 2008
+ * Copyright Red Hat, Inc. 2009
  *
  * Authors:
- *  Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
+ *  Amit Shah <amit.shah@redhat.com>
  *
  * This work is licensed under the terms of the GNU GPL, version 2.  See
  * the COPYING file in the top-level directory.
@@ -16,131 +16,55 @@
 #include "virtio.h"
 #include "virtio-console.h"
 
-
 typedef struct VirtIOConsole
 {
-    VirtIODevice vdev;
-    VirtQueue *ivq, *ovq;
-    CharDriverState *chr;
+    VirtConPort port;
 } VirtIOConsole;
 
-static VirtIOConsole *to_virtio_console(VirtIODevice *vdev)
-{
-    return (VirtIOConsole *)vdev;
-}
-
-static void virtio_console_handle_output(VirtIODevice *vdev, VirtQueue *vq)
-{
-    VirtIOConsole *s = to_virtio_console(vdev);
-    VirtQueueElement elem;
-
-    while (virtqueue_pop(vq, &elem)) {
-        ssize_t len = 0;
-        int d;
-
-        for (d = 0; d < elem.out_num; d++) {
-            len += qemu_chr_write(s->chr, (uint8_t *)elem.out_sg[d].iov_base,
-                                  elem.out_sg[d].iov_len);
-        }
-        virtqueue_push(vq, &elem, len);
-        virtio_notify(vdev, vq);
-    }
-}
-
-static void virtio_console_handle_input(VirtIODevice *vdev, VirtQueue *vq)
-{
-}
-
-static uint32_t virtio_console_get_features(VirtIODevice *vdev)
-{
-    return 0;
-}
-
-static int vcon_can_read(void *opaque)
+static size_t flush_buf(VirtConPort *port, const uint8_t *buf, size_t len)
 {
-    VirtIOConsole *s = (VirtIOConsole *) opaque;
-
-    if (!virtio_queue_ready(s->ivq) ||
-        !(s->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
-        virtio_queue_empty(s->ivq))
-        return 0;
-
-    /* current implementations have a page sized buffer.
-     * We fall back to a one byte per read if there is not enough room.
-     * It would be cool to have a function that returns the available byte
-     * instead of checking for a limit */
-    if (virtqueue_avail_bytes(s->ivq, TARGET_PAGE_SIZE, 0))
-        return TARGET_PAGE_SIZE;
-    if (virtqueue_avail_bytes(s->ivq, 1, 0))
-        return 1;
-    return 0;
-}
-
-static void vcon_read(void *opaque, const uint8_t *buf, int size)
-{
-    VirtIOConsole *s = (VirtIOConsole *) opaque;
-    VirtQueueElement elem;
-    int offset = 0;
-
-    /* The current kernel implementation has only one outstanding input
-     * buffer of PAGE_SIZE. Nevertheless, this function is prepared to
-     * handle multiple buffers with multiple sg element for input */
-    while (offset < size) {
-        int i = 0;
-        if (!virtqueue_pop(s->ivq, &elem))
-                break;
-        while (offset < size && i < elem.in_num) {
-            int len = MIN(elem.in_sg[i].iov_len, size - offset);
-            memcpy(elem.in_sg[i].iov_base, buf + offset, len);
-            offset += len;
-            i++;
-        }
-        virtqueue_push(s->ivq, &elem, size);
-    }
-    virtio_notify(&s->vdev, s->ivq);
+    return qemu_chr_write(port->chr, buf, len);
 }
 
-static void vcon_event(void *opaque, int event)
+static int vcon_port_initfn(VirtConDevice *dev)
 {
-    /* we will ignore any event for the time being */
-}
+    VirtConPort *port = DO_UPCAST(VirtConPort, dev, &dev->qdev);
 
-static void virtio_console_save(QEMUFile *f, void *opaque)
-{
-    VirtIOConsole *s = opaque;
+    port->info = dev->info;
 
-    virtio_save(&s->vdev, f);
-}
+    /*
+     * We're not interested in data the guest sends while nothing's
+     * connected on the host side. Just ignore it instead of saving it
+     * for later consumption
+     */
+    port->flush_buffers = 1;
 
-static int virtio_console_load(QEMUFile *f, void *opaque, int version_id)
-{
-    VirtIOConsole *s = opaque;
+    /* Tell the guest we're a console so it attaches us to an hvc console */
+    port->is_console = true;
 
-    if (version_id != 1)
-        return -EINVAL;
+    /*
+     * For console devices, a tty is spawned on /dev/hvc0 and our
+     * /dev/vconNN will never be opened. Set this here.
+     */
+    port->guest_connected = true;
 
-    virtio_load(&s->vdev, f);
     return 0;
 }
 
-VirtIODevice *virtio_console_init(DeviceState *dev)
+static VirtConPortInfo virtcon_console_info = {
+    .qdev.name     = "virtconsole",
+    .qdev.size     = sizeof(VirtConPort),
+    .init          = vcon_port_initfn,
+    .have_data     = flush_buf,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_CHR("chardev", VirtConPort, chr),
+        DEFINE_PROP_STRING("name", VirtConPort, name),
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static void virtcon_console_register(void)
 {
-    VirtIOConsole *s;
-    s = (VirtIOConsole *)virtio_common_init("virtio-console",
-                                            VIRTIO_ID_CONSOLE,
-                                            0, sizeof(VirtIOConsole));
-    if (s == NULL)
-        return NULL;
-
-    s->vdev.get_features = virtio_console_get_features;
-
-    s->ivq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_input);
-    s->ovq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_output);
-
-    s->chr = qdev_init_chardev(dev);
-    qemu_chr_add_handlers(s->chr, vcon_can_read, vcon_read, vcon_event, s);
-
-    register_savevm("virtio-console", -1, 1, virtio_console_save, virtio_console_load, s);
-
-    return &s->vdev;
+    virtcon_port_qdev_register(&virtcon_console_info);
 }
+device_init(virtcon_console_register)
diff --git a/hw/virtio-console.h b/hw/virtio-console.h
index 84d0717..b0df6c5 100644
--- a/hw/virtio-console.h
+++ b/hw/virtio-console.h
@@ -2,9 +2,11 @@
  * Virtio Console Support
  *
  * Copyright IBM, Corp. 2008
+ * Copyright Red Hat, Inc. 2009
  *
  * Authors:
  *  Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
+ *  Amit Shah <amit.shah@redhat.com>
  *
  * This work is licensed under the terms of the GNU GPL, version 2.  See
  * the COPYING file in the top-level directory.
@@ -13,7 +15,135 @@
 #ifndef _QEMU_VIRTIO_CONSOLE_H
 #define _QEMU_VIRTIO_CONSOLE_H
 
+#include <stdbool.h>
+#include "qdev.h"
+
+/* Interface shared between the guest kernel and qemu */
+
 /* The ID for virtio console */
 #define VIRTIO_ID_CONSOLE 3
 
+/* Invalid port number */
+#define VIRTIO_CONSOLE_BAD_ID		(~(uint32_t)0)
+
+/* Features supported */
+#define VIRTIO_CONSOLE_F_MULTIPORT	1
+
+struct virtio_console_config
+{
+    /*
+     * These two fields are used by VIRTIO_CONSOLE_F_SIZE which
+     * isn't implemented here yet
+     */
+    uint16_t cols;
+    uint16_t rows;
+
+    uint32_t nr_active_ports;
+} __attribute__((packed));
+
+struct virtio_console_control
+{
+    uint16_t event;
+    uint16_t value;
+};
+
+struct virtio_console_header {
+    uint32_t id; /* Port id */
+    uint32_t flags; /* Some message between host and guest */
+    uint32_t size; /* Size that's sent with the first buffer of each stream */
+} __attribute__((packed));
+
+/* Messages between host and guest */
+#define VIRTIO_CONSOLE_ID_INTERNAL	(1 << 0)
+
+/* Some events for the internal messages (control packets) */
+#define VIRTIO_CONSOLE_PORT_OPEN	0
+#define VIRTIO_CONSOLE_PORT_NAME	1
+#define VIRTIO_CONSOLE_CONSOLE_PORT	2
+
+/* In-qemu interface */
+
+/* Max. virtio console ports per device */
+#define MAX_VIRTIO_CONSOLE_PORTS	64
+
+typedef struct VirtConBus VirtConBus;
+typedef struct VirtConPort VirtConPort;
+typedef struct VirtConPortInfo VirtConPortInfo;
+
+typedef struct VirtConDevice {
+    DeviceState qdev;
+    VirtConPortInfo *info;
+} VirtConDevice;
+
+/*
+ * This is the state that's shared between all the ports.  Some of the
+ * state is configurable via command-line options. Some of it can be
+ * set by individual devices in their initfn routines. Some of the
+ * state is set by the generic qdev device init routine.
+ */
+struct VirtConPort {
+    DeviceState dev;
+    VirtConPortInfo *info;
+
+    /* State for the chardevice associated with this port */
+    CharDriverState *chr;
+
+    /*
+     * This name is sent to the guest and exported via sysfs.
+     * The guest could create symlinks based on this information.
+     * The name is in the reverse fqdn format, like org.qemu.console.0
+     */
+    char *name;
+
+    /*
+     * This list holds buffers pushed by the guest in case the guest
+     * sent incomplete messages or the host connection was down and
+     * the device requested to cache the data.
+     */
+    QTAILQ_HEAD(, VirtConPortBuffer) unflushed_buffer_head;
+
+    /*
+     * This id helps identify ports between the guest and the host.
+     * The guest sends a "header" with this id with each data packet
+     * that it sends and the host can then find out which associated
+     * device to send out this data to
+     */
+    uint32_t id;
+
+    /*
+     * This boolean, when set, means "don't queue data that gets sent
+     * to this port when the host is not connected".
+     */
+    int flush_buffers;
+
+    /* Identify if this is a port that binds with hvc in the guest */
+    bool is_console;
+
+    /* Is the corresponding guest device open? */
+    bool guest_connected;
+    /* Is this device open for IO on the host? */
+    bool host_connected;
+};
+
+
+typedef int (*virtcon_port_qdev_initfn)(VirtConDevice *dev);
+
+struct VirtConPortInfo {
+    DeviceInfo qdev;
+    virtcon_port_qdev_initfn init;
+
+    /* Callbacks for guest events */
+    void (*guest_open)(VirtConPort *port);
+    void (*guest_close)(VirtConPort *port);
+
+    size_t (*have_data)(VirtConPort *port, const uint8_t *buf, size_t len);
+};
+
+void virtcon_port_qdev_register(VirtConPortInfo *info);
+
+int virtio_console_open(VirtConPort *port);
+void virtio_console_close(VirtConPort *port);
+size_t virtio_console_write(VirtConPort *port, const uint8_t *buf, size_t size);
+
+
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index 2f4291d..ca412bf 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1604,14 +1604,6 @@ character to Control-t.
 @end table
 ETEXI
 
-DEF("virtioconsole", HAS_ARG, QEMU_OPTION_virtiocon, \
-    "-virtioconsole c\n" \
-    "                set virtio console\n")
-STEXI
-@item -virtioconsole @var{c}
-Set virtio console.
-ETEXI
-
 DEF("show-cursor", 0, QEMU_OPTION_show_cursor, \
     "-show-cursor    show cursor\n")
 STEXI
diff --git a/sysemu.h b/sysemu.h
index a8d1549..f5b287e 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -233,12 +233,6 @@ extern CharDriverState *serial_hds[MAX_SERIAL_PORTS];
 
 extern CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
 
-/* virtio consoles */
-
-#define MAX_VIRTIO_CONSOLES 1
-
-extern CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
-
 #define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
 
 #ifdef HAS_AUDIO
diff --git a/vl.c b/vl.c
index 3b49c56..b5215b2 100644
--- a/vl.c
+++ b/vl.c
@@ -213,7 +213,6 @@ static int no_frame = 0;
 int no_quit = 0;
 CharDriverState *serial_hds[MAX_SERIAL_PORTS];
 CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
-CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
 #ifdef TARGET_I386
 int win2k_install_hack = 0;
 int rtc_td_hack = 0;
@@ -4690,8 +4689,6 @@ int main(int argc, char **argv, char **envp)
     int serial_device_index;
     const char *parallel_devices[MAX_PARALLEL_PORTS];
     int parallel_device_index;
-    const char *virtio_consoles[MAX_VIRTIO_CONSOLES];
-    int virtio_console_index;
     const char *loadvm = NULL;
     QEMUMachine *machine;
     const char *cpu_model;
@@ -4765,10 +4762,6 @@ int main(int argc, char **argv, char **envp)
         parallel_devices[i] = NULL;
     parallel_device_index = 0;
 
-    for(i = 0; i < MAX_VIRTIO_CONSOLES; i++)
-        virtio_consoles[i] = NULL;
-    virtio_console_index = 0;
-
     monitor_devices[0] = "vc:80Cx24C";
     for (i = 1; i < MAX_MONITOR_DEVICES; i++) {
         monitor_devices[i] = NULL;
@@ -5228,14 +5221,6 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
                 }
                 break;
-            case QEMU_OPTION_virtiocon:
-                if (virtio_console_index >= MAX_VIRTIO_CONSOLES) {
-                    fprintf(stderr, "qemu: too many virtio consoles\n");
-                    exit(1);
-                }
-                virtio_consoles[virtio_console_index] = optarg;
-                virtio_console_index++;
-                break;
             case QEMU_OPTION_parallel:
                 if (parallel_device_index >= MAX_PARALLEL_PORTS) {
                     fprintf(stderr, "qemu: too many parallel ports\n");
@@ -5827,20 +5812,6 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
-    for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
-        const char *devname = virtio_consoles[i];
-        if (devname && strcmp(devname, "none")) {
-            char label[32];
-            snprintf(label, sizeof(label), "virtcon%d", i);
-            virtcon_hds[i] = qemu_chr_open(label, devname, NULL);
-            if (!virtcon_hds[i]) {
-                fprintf(stderr, "qemu: could not open virtio console '%s'\n",
-                        devname);
-                exit(1);
-            }
-        }
-    }
-
     module_call_init(MODULE_INIT_DEVICE);
 
     if (watchdog) {
@@ -5968,14 +5939,6 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
-    for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
-        const char *devname = virtio_consoles[i];
-        if (virtcon_hds[i] && devname) {
-            if (strstart(devname, "vc", 0))
-                qemu_chr_printf(virtcon_hds[i], "virtio console%d\r\n", i);
-        }
-    }
-
     if (gdbstub_dev && gdbserver_start(gdbstub_dev) < 0) {
         fprintf(stderr, "qemu: could not open gdbserver on device '%s'\n",
                 gdbstub_dev);
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/6] virtio-console-port: Add a new device on the virtio-console-bus for generic host-guest communication

[Amit Shah]

This is a new device that uses the virtio-console bus.

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

    For requirements, use-cases and some history see

        http://www.linux-kvm.org/page/VMchannel_Requirements

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 Makefile.target          |    2 +-
 hw/virtio-console-port.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletions(-)
 create mode 100644 hw/virtio-console-port.c

diff --git a/Makefile.target b/Makefile.target
index 22a2644..918275a 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -173,7 +173,7 @@ obj-y = vl.o monitor.o pci.o isa_mmio.o machine.o \
         gdbstub.o gdbstub-xml.o
 # virtio has to be here due to weird dependency between PCI and virtio-net.
 # need to fix this properly
-obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o virtio-console-bus.o virtio-pci.o
+obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o virtio-console-port.o virtio-console-bus.o virtio-pci.o
 obj-$(CONFIG_KVM) += kvm.o kvm-all.o
 
 LIBS+=-lz
diff --git a/hw/virtio-console-port.c b/hw/virtio-console-port.c
new file mode 100644
index 0000000..f8c0bbc
--- /dev/null
+++ b/hw/virtio-console-port.c
@@ -0,0 +1,54 @@
+/*
+ * Virtio Serial Port
+ *
+ * Copyright Red Hat, Inc. 2009
+ *
+ * Authors:
+ *  Amit Shah <amit.shah@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include "hw.h"
+#include "qemu-char.h"
+#include "virtio-console.h"
+
+typedef struct VirtIOConsole
+{
+    VirtConPort port;
+} VirtIOConsole;
+
+static size_t flush_buf(VirtConPort *port, const uint8_t *buf, size_t len)
+{
+    return qemu_chr_write(port->chr, buf, len);
+}
+
+static int vcon_port_initfn(VirtConDevice *dev)
+{
+    VirtConPort *port = DO_UPCAST(VirtConPort, dev, &dev->qdev);
+
+    port->info = dev->info;
+
+    return 0;
+}
+
+static VirtConPortInfo virtcon_port_info = {
+    .qdev.name     = "virtconport",
+    .qdev.size     = sizeof(VirtConPort),
+    .init          = vcon_port_initfn,
+    .have_data     = flush_buf,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_CHR("chardev", VirtConPort, chr),
+        DEFINE_PROP_STRING("name", VirtConPort, name),
+        DEFINE_PROP_INT32("flush_buffers", VirtConPort, flush_buffers, 0),
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static void virtcon_port_register(void)
+{
+    virtcon_port_qdev_register(&virtcon_port_info);
+}
+device_init(virtcon_port_register)
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/6] vnc: add a is_vnc_active() helper

[Amit Shah]

This helper is introduced to query the status of vnc.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 vnc.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/vnc.c b/vnc.c
index 5eaef6a..ff2d4a8 100644
--- a/vnc.c
+++ b/vnc.c
@@ -178,9 +178,17 @@ static void do_info_vnc_client(Monitor *mon, VncState *client)
 #endif
 }
 
-void do_info_vnc(Monitor *mon)
+static int is_vnc_active(void)
 {
     if (vnc_display == NULL || vnc_display->display == NULL) {
+        return 0;
+    }
+    return 1;
+}
+
+void do_info_vnc(Monitor *mon)
+{
+    if (!is_vnc_active()) {
         monitor_printf(mon, "Server: disabled\n");
     } else {
         char *serverAddr = vnc_socket_local_addr("     address: %s:%s\n",
-- 
1.6.2.5

[Qemu-devel] [PATCH 6/6] vnc: Add a virtio-console-bus device to send / receive guest clipboard

[Amit Shah]

This is a simple device on the virtio-console-bus that syncs with the
guest for clipboard activity.

A daemon is needed in the guest to send / receive the clipboard data.

This patch is just meant to show how to use the read/write api is
exposed for devices that ride on top of the virtio-console-bus.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 vnc.c |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/vnc.c b/vnc.c
index ff2d4a8..b6f9cce 100644
--- a/vnc.c
+++ b/vnc.c
@@ -29,6 +29,7 @@
 #include "qemu_socket.h"
 #include "qemu-timer.h"
 #include "acl.h"
+#include "hw/virtio-console.h"
 
 #define VNC_REFRESH_INTERVAL_BASE 30
 #define VNC_REFRESH_INTERVAL_INC  50
@@ -44,6 +45,11 @@
     } \
 }
 
+typedef struct VirtConVnc {
+    VirtConPort port;
+} VirtConVnc;
+
+static VirtConVnc *virtcon_vnc;
 
 static VncDisplay *vnc_display; /* needed for info vnc */
 static DisplayChangeListener *dcl;
@@ -659,6 +665,34 @@ static void send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
     }
 }
 
+static size_t vnc_clipboard_data_from_guest(VirtConPort *port,
+                                            const uint8_t *buf, size_t len)
+{
+    VncState *vs;
+    VncDisplay *vd;
+    DisplayState *ds;
+
+    if (!is_vnc_active())
+        return 0;
+
+    ds = vnc_display->ds;
+    vd = ds->opaque;
+
+    for (vs = vd->clients; vs; vs = vs->next) {
+        vnc_write_u8(vs, 3);  /* ServerCutText */
+        vnc_write_u8(vs, 0);  /* Padding */
+        vnc_write_u8(vs, 0);  /* Padding */
+        vnc_write_u8(vs, 0);  /* Padding */
+        vnc_write_u32(vs, len);
+        while (len--) {
+            vnc_write_u8(vs, *(buf++));
+        }
+        vnc_flush(vs);
+    }
+    return len;
+}
+
+
 static void vnc_copy(VncState *vs, int src_x, int src_y, int dst_x, int dst_y, int w, int h)
 {
     /* send bitblit op to the vnc client */
@@ -1240,6 +1274,7 @@ uint32_t read_u32(uint8_t *data, size_t offset)
 
 static void client_cut_text(VncState *vs, size_t len, uint8_t *text)
 {
+    virtio_console_write(&virtcon_vnc->port, text, len);
 }
 
 static void check_pointer_type_change(VncState *vs, int absolute)
@@ -2197,6 +2232,10 @@ static void vnc_connect(VncDisplay *vd, int csock)
 
     vnc_init_timer(vd);
 
+    if (virtcon_vnc) {
+        virtio_console_open(&virtcon_vnc->port);
+    }
+
     /* vs might be free()ed here */
 }
 
@@ -2516,3 +2555,31 @@ int vnc_display_open(DisplayState *ds, const char *display)
     }
     return qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
 }
+
+static int vcon_vnc_initfn(VirtConDevice *dev)
+{
+    VirtConPort *port;
+    VirtConVnc *vnc;
+
+    port = DO_UPCAST(VirtConPort, dev, &dev->qdev);
+    vnc = DO_UPCAST(VirtConVnc, port, port);
+    virtcon_vnc = vnc;
+
+    port->name = qemu_strdup("org.qemu.clipboard");
+    /* We don't want to keep old data lingering if vnc is not connected */
+    port->flush_buffers = 1;
+    return 0;
+}
+
+static VirtConPortInfo virtcon_vnc_info = {
+    .qdev.name     = "virtconvnc",
+    .qdev.size     = sizeof(VirtConVnc),
+    .init          = vcon_vnc_initfn,
+    .have_data     = vnc_clipboard_data_from_guest,
+};
+
+static void virtcon_port_register(void)
+{
+    virtcon_port_qdev_register(&virtcon_vnc_info);
+}
+device_init(virtcon_port_register)
-- 
1.6.2.5

Re: [Qemu-devel] virtio-console-bus, multiport, virtio-console-port

[Amit Shah]

On (Tue) Sep 29 2009 [17:34:42], Amit Shah wrote:
> Hello,
> 
> This patch series converts the virtio-console device to qdev, adds a
> virtio-console bus on which similar devices can ride and shows how such
> devices can be used by adding a new virtio-console-port device and also
> a virtio-console-vnc device.
> 
> This is working well in my testing. I tested old kernel/new qemu, new
> kernel/old qemu and new kernel/new qemu.
> 
> The ports are also tested by spawning two virtio consoles and one
> generic port. Heavy activity on all three ports is sustained for quite a
> long time with correct results.
> 
> Please review and apply.

BTW I've just kept the names to

-device virtio-console-pci (for the bus)
-device virtconsole (for the console device)
-device virtconport (for a generic port)

I'm open to changing these -- or keeping as they are.

There's also a 'flush_buffers' option to the devices, that could do with
a better name.

Also these patches are against qemu-kvm.git so they won't apply to
qemu.git -- I'll send the patches against qemu.git next time around.

		Amit

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Gerd Hoffmann]

> +typedef struct VirtIOConsole
> +{

You are reusing this struct name a few times.
Better don't do that, it is confusing.

> +static VirtIOConsole *virtio_console;

Why do you need this?

> +static void flush_queue(VirtConPort *port)
> +{
> +    VirtConPortBuffer *buf, *buf2;
> +    uint8_t *outbuf;
> +    size_t outlen, outsize;
> +
> +    /*
> +     * If the app isn't interested in buffering packets till it's
> +     * opened, just drop the data guest sends us till a connection is
> +     * established.
> +     */
> +    if (!port->host_connected&&  !port->flush_buffers)
> +        return;

Hmm.  Who needs that buffering?  Only user seems to be the port driver 
(patch 4/6).  So move the buffering (and the host_connected state 
tracking) from the core to that driver?

> +/* Readiness of the guest to accept data on a port */
> +static int vcon_can_read(void *opaque)

int vcon_can_read(VirtConPort *port)

> +static void vcon_read(void *opaque, const uint8_t *buf, int size)
> +static void vcon_event(void *opaque, int event)

Likewise.

> +static VirtConBus *virtcon_bus;

Why do you need this?

> +static int virtcon_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
> +{

> +    if (port->chr) {
> +        qemu_chr_add_handlers(port->chr, vcon_can_read, vcon_read, vcon_event,
> +                              port);

Should be handled by the VirtConPort drivers.

> +    if (MAX_VIRTIO_CONSOLE_PORTS % 32) {
> +        /* We require MAX_VIRTIO_CONSOLE_PORTS be a multiple of 32:
> +         * We anyway use up that much space for the bitmap and it
> +         * simplifies some calculations
> +         */
> +        return NULL;
> +    }

Huh?  Runtime check for a compile-time constant?

> +    /* Spawn a new virtio-console bus on which the ports will ride as devices */
> +    virtcon_bus_new(dev);

s->bus = virtcon_bus_new(dev);
port0 = qdev_create(s->bus, "virtconsole");  /* console @ port0 for 
backward compat */
qdev_prop_set_*(port0, ...);
qdev_init(port0);

Or does that happen somewhere else?

> +typedef struct VirtConBus VirtConBus;
> +typedef struct VirtConPort VirtConPort;
> +typedef struct VirtConPortInfo VirtConPortInfo;
> +
> +typedef struct VirtConDevice {
> +    DeviceState qdev;
> +    VirtConPortInfo *info;
> +} VirtConDevice;

Leftover from old patch version?

> +/*
> + * This is the state that's shared between all the ports.  Some of the
> + * state is configurable via command-line options. Some of it can be
> + * set by individual devices in their initfn routines. Some of the
> + * state is set by the generic qdev device init routine.
> + */
> +struct VirtConPort {
> +    DeviceState dev;
> +    VirtConPortInfo *info;
> +
> +    /* State for the chardevice associated with this port */
> +    CharDriverState *chr;

That should go to the port driver if needed.

> +typedef int (*virtcon_port_qdev_initfn)(VirtConDevice *dev);
> +
> +struct VirtConPortInfo {
> +    DeviceInfo qdev;
> +    virtcon_port_qdev_initfn init;
> +
> +    /* Callbacks for guest events */
> +    void (*guest_open)(VirtConPort *port);
> +    void (*guest_close)(VirtConPort *port);
> +
> +    size_t (*have_data)(VirtConPort *port, const uint8_t *buf, size_t len);

Maybe it is a good idea to pass a VirtConPortBuffer here instead of 
buf+len, especially when it becomes the port drivers job to do any 
buffering if needed.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 4/6] virtio-console-port: Add a new device on the virtio-console-bus for generic host-guest communication

[Gerd Hoffmann]

> +typedef struct VirtIOConsole
> +{
> +    VirtConPort port;

Your CharDriverState should go here.

> +        DEFINE_PROP_INT32("flush_buffers", VirtConPort, flush_buffers, 0),

Well, we should find a better name for that one.  flush_buffers is very 
unclear.  It actually is dont_cache_data_when_unconnected.  That name is 
insane long though.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 6/6] vnc: Add a virtio-console-bus device to send / receive guest clipboard

[Gerd Hoffmann]

>   static void client_cut_text(VncState *vs, size_t len, uint8_t *text)
>   {
> +    virtio_console_write(&virtcon_vnc->port, text, len);

Needs "if (virtcon_vnc)" ?

> +    if (virtcon_vnc) {
> +        virtio_console_open(&virtcon_vnc->port);
> +    }

Like it is done here?

There is no virtio_console_close() in this patch.  Does this work 
correctly after multiple connects + disconnects (with multiple vnc 
clients connected at the same time)?

I think when moving the buffering and host_connected state tracking into 
the port driver (patch 4/6) which is the only user of the facility you 
don't need these open/close calls at all.

cheers
   Gerd

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Amit Shah]

On (Tue) Sep 29 2009 [20:04:36], Gerd Hoffmann wrote:
>
>> +typedef struct VirtIOConsole
>> +{
>
> You are reusing this struct name a few times.
> Better don't do that, it is confusing.

Yeah; I still have to go through the entire naming thing. I also really
think virtio-console-bus, virtio-console and virtio-console-port are
confusing. I thought it's better to do:

virtio-serial-bus (and the corresponding -device virtio-serial-pci)
virtio-console
virtio-serial-port
virtio-serial-vnc

Is that OK with all?

>> +static VirtIOConsole *virtio_console;
>
> Why do you need this?

Just to keep the current behaviour of restricting to one virtio-console
device. This can be later reworked to allow multiple devices.

>> +static void flush_queue(VirtConPort *port)
>> +{
>> +    VirtConPortBuffer *buf, *buf2;
>> +    uint8_t *outbuf;
>> +    size_t outlen, outsize;
>> +
>> +    /*
>> +     * If the app isn't interested in buffering packets till it's
>> +     * opened, just drop the data guest sends us till a connection is
>> +     * established.
>> +     */
>> +    if (!port->host_connected&&  !port->flush_buffers)
>> +        return;
>
> Hmm.  Who needs that buffering?  Only user seems to be the port driver  
> (patch 4/6).  So move the buffering (and the host_connected state  
> tracking) from the core to that driver?

The buffering could be used by other devices too I guess. The other two
users that we currently have, vnc (which is just a demo patch) and
console, don't need the buffering, but it's a general facility.

If more users need the buffering, the code could get duplicated so I
thought of keeping it here.

>> +/* Readiness of the guest to accept data on a port */
>> +static int vcon_can_read(void *opaque)
>
> int vcon_can_read(VirtConPort *port)

OK

>> +static VirtConBus *virtcon_bus;
>
> Why do you need this?

I could put this in the VirtIOConsole struct.

>> +static int virtcon_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
>> +{
>
>> +    if (port->chr) {
>> +        qemu_chr_add_handlers(port->chr, vcon_can_read, vcon_read, vcon_event,
>> +                              port);
>
> Should be handled by the VirtConPort drivers.

There are two reasons why I didn't put this there: virtio-console as
well as virtio-serial-port will need char drivers. There could be others
as well.

Also, some virtio-specific checks are done in vcon_can_read. So it's
better this is put in the common code.

>> +    if (MAX_VIRTIO_CONSOLE_PORTS % 32) {
>> +        /* We require MAX_VIRTIO_CONSOLE_PORTS be a multiple of 32:
>> +         * We anyway use up that much space for the bitmap and it
>> +         * simplifies some calculations
>> +         */
>> +        return NULL;
>> +    }
>
> Huh?  Runtime check for a compile-time constant?

Yeah; bad enough it was this way but I now think I can get rid of this
entirely as I don't depend on a ports_map.

>> +    /* Spawn a new virtio-console bus on which the ports will ride as devices */
>> +    virtcon_bus_new(dev);
>
> s->bus = virtcon_bus_new(dev);
> port0 = qdev_create(s->bus, "virtconsole");  /* console @ port0 for  
> backward compat */
> qdev_prop_set_*(port0, ...);
> qdev_init(port0);
>
> Or does that happen somewhere else?

I'm nowhere assuming now any port number - function mapping and so
spawning a virtio-console on port#0 is not necessary.

And yeah; as mentioned previously, I'll put the bus into VirtIOConsole
so that it doesn't have to be static.

>> +typedef struct VirtConBus VirtConBus;
>> +typedef struct VirtConPort VirtConPort;
>> +typedef struct VirtConPortInfo VirtConPortInfo;
>> +
>> +typedef struct VirtConDevice {
>> +    DeviceState qdev;
>> +    VirtConPortInfo *info;
>> +} VirtConDevice;
>
> Leftover from old patch version?

You mean this should not be in the .h? Yeah.

>> + * This is the state that's shared between all the ports.  Some of the
>> + * state is configurable via command-line options. Some of it can be
>> + * set by individual devices in their initfn routines. Some of the
>> + * state is set by the generic qdev device init routine.
>> + */
>> +struct VirtConPort {
>> +    DeviceState dev;
>> +    VirtConPortInfo *info;
>> +
>> +    /* State for the chardevice associated with this port */
>> +    CharDriverState *chr;
>
> That should go to the port driver if needed.

Wrote about the char driver above.

>> +typedef int (*virtcon_port_qdev_initfn)(VirtConDevice *dev);
>> +
>> +struct VirtConPortInfo {
>> +    DeviceInfo qdev;
>> +    virtcon_port_qdev_initfn init;
>> +
>> +    /* Callbacks for guest events */
>> +    void (*guest_open)(VirtConPort *port);
>> +    void (*guest_close)(VirtConPort *port);
>> +
>> +    size_t (*have_data)(VirtConPort *port, const uint8_t *buf, size_t len);
>
> Maybe it is a good idea to pass a VirtConPortBuffer here instead of  
> buf+len, especially when it becomes the port drivers job to do any  
> buffering if needed.

Also touched upon above.

Thanks,
		Amit

Re: [Qemu-devel] [PATCH 6/6] vnc: Add a virtio-console-bus device to send / receive guest clipboard

[Amit Shah]

On (Tue) Sep 29 2009 [20:13:56], Gerd Hoffmann wrote:
>>   static void client_cut_text(VncState *vs, size_t len, uint8_t *text)
>>   {
>> +    virtio_console_write(&virtcon_vnc->port, text, len);
>
> Needs "if (virtcon_vnc)" ?

Yes.

>> +    if (virtcon_vnc) {
>> +        virtio_console_open(&virtcon_vnc->port);
>> +    }
>
> Like it is done here?
>
> There is no virtio_console_close() in this patch.  Does this work  
> correctly after multiple connects + disconnects (with multiple vnc  
> clients connected at the same time)?

Yes, I actually don't propose this for inclusion. It's just a way to
show how the api works. I've only done some very basic testing with this
vnc patch (and found some bug in the vnc code that I reported about a
while back).

> I think when moving the buffering and host_connected state tracking into  
> the port driver (patch 4/6) which is the only user of the facility you  
> don't need these open/close calls at all.

True for vnc, but may not be for the future users.

		Amit

Re: [Qemu-devel] [PATCH 4/6] virtio-console-port: Add a new device on the virtio-console-bus for generic host-guest communication

[Nathan Baum]

On Tue, 2009-09-29 at 20:08 +0200, Gerd Hoffmann wrote:
> > +typedef struct VirtIOConsole
> > +{
> > +    VirtConPort port;
> 
> Your CharDriverState should go here.
> 
> > +        DEFINE_PROP_INT32("flush_buffers", VirtConPort, flush_buffers, 0),
> 
> Well, we should find a better name for that one.  flush_buffers is very 
> unclear.  It actually is dont_cache_data_when_unconnected.  That name is 
> insane long though.

volatile_when_unconnected?

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Gerd Hoffmann]

> virtio-serial-bus (and the corresponding -device virtio-serial-pci)
> virtio-console
> virtio-serial-port
> virtio-serial-vnc
>
> Is that OK with all?

Looks fine to me.

>>> +static VirtIOConsole *virtio_console;
>>
>> Why do you need this?
>
> Just to keep the current behaviour of restricting to one virtio-console
> device. This can be later reworked to allow multiple devices.

I'd drop the limit right from the start.

>>> +static void flush_queue(VirtConPort *port)
>>> +{
>>> +    VirtConPortBuffer *buf, *buf2;
>>> +    uint8_t *outbuf;
>>> +    size_t outlen, outsize;
>>> +
>>> +    /*
>>> +     * If the app isn't interested in buffering packets till it's
>>> +     * opened, just drop the data guest sends us till a connection is
>>> +     * established.
>>> +     */
>>> +    if (!port->host_connected&&   !port->flush_buffers)
>>> +        return;
>>
>> Hmm.  Who needs that buffering?  Only user seems to be the port driver
>> (patch 4/6).  So move the buffering (and the host_connected state
>> tracking) from the core to that driver?
>
> The buffering could be used by other devices too I guess. The other two
> users that we currently have, vnc (which is just a demo patch) and
> console, don't need the buffering, but it's a general facility.
>
> If more users need the buffering, the code could get duplicated so I
> thought of keeping it here.

I'd suggest to look what do do when another user which wants buffering 
comes along.  Might be the requirements are different, so it wouldn't 
work anyway.

Also when you pass around pointers to VirtConPortBuffer the port drivers 
can implement buffering very easily.

>>> +static VirtConBus *virtcon_bus;
>>
>> Why do you need this?
>
> I could put this in the VirtIOConsole struct.

Yes, please.  You'll need that for multiple devices anyway.

>>> +static int virtcon_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
>>> +{
>>
>>> +    if (port->chr) {
>>> +        qemu_chr_add_handlers(port->chr, vcon_can_read, vcon_read, vcon_event,
>>> +                              port);
>>
>> Should be handled by the VirtConPort drivers.
>
> There are two reasons why I didn't put this there: virtio-console as
> well as virtio-serial-port will need char drivers. There could be others
> as well.

They are handled differently though.  I think console will not do any 
buffering at all, whereas serial-port provides the option to do buffering.

There is also the option to stick both console and serial-port 
implementations into the same source file and make them share the 
functions which handle the chardev interaction.

> Also, some virtio-specific checks are done in vcon_can_read. So it's
> better this is put in the common code.

Having a helper function for the port drivers which fill the role 
vcon_can_read() fills right now certainly makes sense.

The naming is bad though.  vcon_can_read() is named from the chardevs 
point of view.  It actually checks how many bytes can be written to the 
virtio ring.  It should be named accordingly.

Also the parameters should make sense from a port drivers point of view, 
not tweaked for chardev.  Port drivers which don't need a chardev at all 
might want to use this too.  Port drivers which link to a chardev can 
have a trivial one-liner wrapper function to map chardev callbacks into 
virtio-serial helper functions.

>>> +    /* Spawn a new virtio-console bus on which the ports will ride as devices */
>>> +    virtcon_bus_new(dev);
>>
>> s->bus = virtcon_bus_new(dev);
>> port0 = qdev_create(s->bus, "virtconsole");  /* console @ port0 for
>> backward compat */
>> qdev_prop_set_*(port0, ...);
>> qdev_init(port0);
>>
>> Or does that happen somewhere else?
>
> I'm nowhere assuming now any port number - function mapping and so
> spawning a virtio-console on port#0 is not necessary.

How old kernels which expect a single console are supposed to work if 
you don't create one?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Amit Shah]

On (Wed) Sep 30 2009 [10:59:47], Gerd Hoffmann wrote:
>> virtio-serial-bus (and the corresponding -device virtio-serial-pci)
>> virtio-console
>> virtio-serial-port
>> virtio-serial-vnc
>>
>> Is that OK with all?
>
> Looks fine to me.

OK; that'll replace the current virtio-console-pci device but since it's
not in use yet (from the command line explicity) that doesn't break
anything.

>>>> +static VirtIOConsole *virtio_console;
>>>
>>> Why do you need this?
>>
>> Just to keep the current behaviour of restricting to one virtio-console
>> device. This can be later reworked to allow multiple devices.
>
> I'd drop the limit right from the start.

BTW the kernel too doesn't support multiple devices so far.

>>>> +static void flush_queue(VirtConPort *port)
>>>> +{
>>>> +    VirtConPortBuffer *buf, *buf2;
>>>> +    uint8_t *outbuf;
>>>> +    size_t outlen, outsize;
>>>> +
>>>> +    /*
>>>> +     * If the app isn't interested in buffering packets till it's
>>>> +     * opened, just drop the data guest sends us till a connection is
>>>> +     * established.
>>>> +     */
>>>> +    if (!port->host_connected&&   !port->flush_buffers)
>>>> +        return;
>>>
>>> Hmm.  Who needs that buffering?  Only user seems to be the port driver
>>> (patch 4/6).  So move the buffering (and the host_connected state
>>> tracking) from the core to that driver?
>>
>> The buffering could be used by other devices too I guess. The other two
>> users that we currently have, vnc (which is just a demo patch) and
>> console, don't need the buffering, but it's a general facility.
>>
>> If more users need the buffering, the code could get duplicated so I
>> thought of keeping it here.
>
> I'd suggest to look what do do when another user which wants buffering  
> comes along.  Might be the requirements are different, so it wouldn't  
> work anyway.
>
> Also when you pass around pointers to VirtConPortBuffer the port drivers  
> can implement buffering very easily.

It will mostly be duplicated. Right now, I pass on an entire message as
was passed on by the kernel in one write() request. If I start passing
around buffers, it'll be incomplete messages (eg, an 8K write request in
the kernel will be split in two buffers of 4K each -- or three buffers
with the last one containing a few bytes). I don't think I want the ports
to worry about that. The ports should just get the entire message.

In fact, it is this way because vnc needs the entire message handed out
to it in one go.

So I don't think I want to expose the buffers to individual devices.

>>>> +static int virtcon_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
>>>> +{
>>>
>>>> +    if (port->chr) {
>>>> +        qemu_chr_add_handlers(port->chr, vcon_can_read, vcon_read, vcon_event,
>>>> +                              port);
>>>
>>> Should be handled by the VirtConPort drivers.
>>
>> There are two reasons why I didn't put this there: virtio-console as
>> well as virtio-serial-port will need char drivers. There could be others
>> as well.
>
> They are handled differently though.  I think console will not do any  
> buffering at all, whereas serial-port provides the option to do 
> buffering.

But the buffering is independent of the char drivers.

> There is also the option to stick both console and serial-port  
> implementations into the same source file and make them share the  
> functions which handle the chardev interaction.

Sure; that's a possibility.

>> Also, some virtio-specific checks are done in vcon_can_read. So it's
>> better this is put in the common code.
>
> Having a helper function for the port drivers which fill the role  
> vcon_can_read() fills right now certainly makes sense.

Yeah; if the char stuff is to be taken out of the bus file.

> The naming is bad though.  vcon_can_read() is named from the chardevs  
> point of view.  It actually checks how many bytes can be written to the  
> virtio ring.  It should be named accordingly.
>
> Also the parameters should make sense from a port drivers point of view,  
> not tweaked for chardev.  Port drivers which don't need a chardev at all  
> might want to use this too.  Port drivers which link to a chardev can  
> have a trivial one-liner wrapper function to map chardev callbacks into  
> virtio-serial helper functions.

Yeah; again, if the chardev stuff is to be moved out of the bus file,
then yes.

>>>> +    /* Spawn a new virtio-console bus on which the ports will ride as devices */
>>>> +    virtcon_bus_new(dev);
>>>
>>> s->bus = virtcon_bus_new(dev);
>>> port0 = qdev_create(s->bus, "virtconsole");  /* console @ port0 for
>>> backward compat */
>>> qdev_prop_set_*(port0, ...);
>>> qdev_init(port0);
>>>
>>> Or does that happen somewhere else?
>>
>> I'm nowhere assuming now any port number - function mapping and so
>> spawning a virtio-console on port#0 is not necessary.
>
> How old kernels which expect a single console are supposed to work if  
> you don't create one?

Basically I've now dropped the old -virtioconsole argument.

So one has to do:

-device virtio-console-pci -device virtioconsole ...

to start a console.

		Amit

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Gerd Hoffmann]

> BTW the kernel too doesn't support multiple devices so far.

Needs fixing too ;)

>> I'd suggest to look what do do when another user which wants buffering
>> comes along.  Might be the requirements are different, so it wouldn't
>> work anyway.
>>
>> Also when you pass around pointers to VirtConPortBuffer the port drivers
>> can implement buffering very easily.
>
> It will mostly be duplicated. Right now, I pass on an entire message as
> was passed on by the kernel in one write() request. If I start passing
> around buffers, it'll be incomplete messages (eg, an 8K write request in
> the kernel will be split in two buffers of 4K each -- or three buffers
> with the last one containing a few bytes). I don't think I want the ports
> to worry about that. The ports should just get the entire message.

Sure.  I think there is a misunderstanding here.  I meant only the 
"buffer messages when unconnected" thing.  Reassembling the messages in 
the core and forward only complete messages to the ports is fine.

> So I don't think I want to expose the buffers to individual devices.

My idea was to pass the (reassembled) message to the port drivers in a 
form they can just stick into a list without memcpy()ing the message in 
case they want to keep it.  Maybe to buffer it because the chardev is 
unconnected.  Maybe just keep the most recent message because it 
contains some kind of guest status.

VirtConPortBuffer looked like it could do the job.  Well, not perfectly, 
it has extra status which is needed only by the core for reassembling 
messages.

>> They are handled differently though.  I think console will not do any
>> buffering at all, whereas serial-port provides the option to do
>> buffering.
>
> But the buffering is independent of the char drivers.

See above ;)

> Basically I've now dropped the old -virtioconsole argument.
>
> So one has to do:
>
> -device virtio-console-pci -device virtioconsole ...
>
> to start a console.

If you do '-device virtio-console-pci -device virtio-port' (i.e. no 
console) and boot a old guest kernel which expects a (single) console 
being there, what will happen?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/6] char: Emit 'OPENED' events on char device open

[Anthony Liguori]

Amit Shah wrote:
> Notify users of the char interface whenever the file / connection is
> opened.
>
> The existing RESET event is triggered when the qemu char state is reset
> as well; which may not be as interesting as char device open events.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
>   

This series breaks the build (ppc440_bamboo.c).

You should always build without an explicit --target-list to avoid this 
in the future.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Amit Shah]

On (Wed) Sep 30 2009 [20:39:34], Gerd Hoffmann wrote:
>> BTW the kernel too doesn't support multiple devices so far.
>
> Needs fixing too ;)

Agreed :-) I've fixed multiple devices in qemu.

>>> I'd suggest to look what do do when another user which wants buffering
>>> comes along.  Might be the requirements are different, so it wouldn't
>>> work anyway.
>>>
>>> Also when you pass around pointers to VirtConPortBuffer the port drivers
>>> can implement buffering very easily.
>>
>> It will mostly be duplicated. Right now, I pass on an entire message as
>> was passed on by the kernel in one write() request. If I start passing
>> around buffers, it'll be incomplete messages (eg, an 8K write request in
>> the kernel will be split in two buffers of 4K each -- or three buffers
>> with the last one containing a few bytes). I don't think I want the ports
>> to worry about that. The ports should just get the entire message.
>
> Sure.  I think there is a misunderstanding here.  I meant only the  
> "buffer messages when unconnected" thing.  Reassembling the messages in  
> the core and forward only complete messages to the ports is fine.

OK; so in that case, passing a ptr to the buffer and its length is fine,
no?

>> Basically I've now dropped the old -virtioconsole argument.
>>
>> So one has to do:
>>
>> -device virtio-console-pci -device virtioconsole ...
>>
>> to start a console.
>
> If you do '-device virtio-console-pci -device virtio-port' (i.e. no  
> console) and boot a old guest kernel which expects a (single) console  
> being there, what will happen?

OK -- I get what you're saying now. However, I don't see any problem
here. If there is no virtioconsole specified to qemu, there's no reason
to expect a console in the guest. That was the case in the past and is
the case now as well. The difference is earlier, when probe() in the
guest was called, it definitely meant the existence of a console. Now,
even if probe() is invoked, it doesn't mean a console was found. But
this hardly is a concern.

Also, the way to attach to a virtioconsole is by spawning a tty on
/dev/hvc0. If no -virtioconsole is specified on the host command line,
no hvc device is spawned on the guest. So things are as they are.

		Amit

Re: [Qemu-devel] [PATCH 1/6] char: Emit 'OPENED' events on char device open

[Amit Shah]

On (Wed) Sep 30 2009 [16:13:33], Anthony Liguori wrote:
> Amit Shah wrote:
>> Notify users of the char interface whenever the file / connection is
>> opened.
>>
>> The existing RESET event is triggered when the qemu char state is reset
>> as well; which may not be as interesting as char device open events.
>>
>> Signed-off-by: Amit Shah <amit.shah@redhat.com>
>>   
>
> This series breaks the build (ppc440_bamboo.c).

This patch or this series? If it's this patch, that's surprising!

> You should always build without an explicit --target-list to avoid this  
> in the future.

Sure -- I intend to when I send a request for the merging once all the
comments have been sorted out.

Thanks,
		Amit

Re: [Qemu-devel] [PATCH 1/6] char: Emit 'OPENED' events on char device open

[Amit Shah]

On (Thu) Oct 01 2009 [10:26:22], Amit Shah wrote:
> On (Wed) Sep 30 2009 [16:13:33], Anthony Liguori wrote:
> > Amit Shah wrote:
> >> Notify users of the char interface whenever the file / connection is
> >> opened.
> >>
> >> The existing RESET event is triggered when the qemu char state is reset
> >> as well; which may not be as interesting as char device open events.
> >>
> >> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> >>   
> >
> > This series breaks the build (ppc440_bamboo.c).
> 
> This patch or this series? If it's this patch, that's surprising!

OK; it's the other patch. This ppc440 machine also init'ed the
virtio-console-pci the way pc.c did; I'll remove this as well.

		Amit

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Gerd Hoffmann]

   Hi,

>> Sure.  I think there is a misunderstanding here.  I meant only the
>> "buffer messages when unconnected" thing.  Reassembling the messages in
>> the core and forward only complete messages to the ports is fine.
>
> OK; so in that case, passing a ptr to the buffer and its length is fine,
> no?

Will work.  We need some clear rules for buffer memory though.  Possible 
ways to deal with it:

   (1) core owns the buffer.  If the port driver wants to keep the
       content it has to memcpy() it to own memory.
   (2) have_data() callback transfers buffer ownership from core to
       the port driver.  It is the port drivers job to free the memory
       when it doesn't need it any more.
   (3) reference-count the buffers.

For (1) + (2) both buffer struct and ptr+len will work.

For (3) ptr+len wouldn't work though, you'll need some struct containing 
ptr, len, refcount and helper functions to get/put buffers.

>> If you do '-device virtio-console-pci -device virtio-port' (i.e. no
>> console) and boot a old guest kernel which expects a (single) console
>> being there, what will happen?
>
> OK -- I get what you're saying now. However, I don't see any problem
> here. If there is no virtioconsole specified to qemu, there's no reason
> to expect a console in the guest. That was the case in the past and is
> the case now as well. The difference is earlier, when probe() in the
> guest was called, it definitely meant the existence of a console. Now,
> even if probe() is invoked, it doesn't mean a console was found. But
> this hardly is a concern.

True for new guest kernels, they simply don't create a hvc.
Question is what *old* guest kernels will do in that case.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Amit Shah]

On (Thu) Oct 01 2009 [10:38:18], Gerd Hoffmann wrote:
>   Hi,
>
>>> Sure.  I think there is a misunderstanding here.  I meant only the
>>> "buffer messages when unconnected" thing.  Reassembling the messages in
>>> the core and forward only complete messages to the ports is fine.
>>
>> OK; so in that case, passing a ptr to the buffer and its length is fine,
>> no?
>
> Will work.  We need some clear rules for buffer memory though.  Possible  
> ways to deal with it:
>
>   (1) core owns the buffer.  If the port driver wants to keep the
>       content it has to memcpy() it to own memory.
>   (2) have_data() callback transfers buffer ownership from core to
>       the port driver.  It is the port drivers job to free the memory
>       when it doesn't need it any more.
>   (3) reference-count the buffers.
>
> For (1) + (2) both buffer struct and ptr+len will work.
>
> For (3) ptr+len wouldn't work though, you'll need some struct containing  
> ptr, len, refcount and helper functions to get/put buffers.

Yeah; Currently it's only (1). But I'm now thinking of doing (2). One
concern Yaniv raised was a port driver may not consume all the data
right away, so it's better to let the port do the buffer management.

>>> If you do '-device virtio-console-pci -device virtio-port' (i.e. no
>>> console) and boot a old guest kernel which expects a (single) console
>>> being there, what will happen?
>>
>> OK -- I get what you're saying now. However, I don't see any problem
>> here. If there is no virtioconsole specified to qemu, there's no reason
>> to expect a console in the guest. That was the case in the past and is
>> the case now as well. The difference is earlier, when probe() in the
>> guest was called, it definitely meant the existence of a console. Now,
>> even if probe() is invoked, it doesn't mean a console was found. But
>> this hardly is a concern.
>
> True for new guest kernels, they simply don't create a hvc.
> Question is what *old* guest kernels will do in that case.

If the guest kernel doesn't support the new virtio feature
VIRTIO_F_MULTIPORT, then we disable all this functionality and only
allow one port. That one port has to be the console port. I've tested
this combination, btw.

However, it's possible that some of the checks got lost in the latest
rework and that port 0 isn't actually a console port. I'll go through
init code again to ensure this. Thanks for explaining the scenario!

		Amit

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Amit Shah]

On (Thu) Oct 01 2009 [14:26:20], Amit Shah wrote:
> >>> If you do '-device virtio-console-pci -device virtio-port' (i.e. no
> >>> console) and boot a old guest kernel which expects a (single) console
> >>> being there, what will happen?
> >>
> >> OK -- I get what you're saying now. However, I don't see any problem
> >> here. If there is no virtioconsole specified to qemu, there's no reason
> >> to expect a console in the guest. That was the case in the past and is
> >> the case now as well. The difference is earlier, when probe() in the
> >> guest was called, it definitely meant the existence of a console. Now,
> >> even if probe() is invoked, it doesn't mean a console was found. But
> >> this hardly is a concern.
> >
> > True for new guest kernels, they simply don't create a hvc.
> > Question is what *old* guest kernels will do in that case.
> 
> If the guest kernel doesn't support the new virtio feature
> VIRTIO_F_MULTIPORT, then we disable all this functionality and only
> allow one port. That one port has to be the console port. I've tested
> this combination, btw.
> 
> However, it's possible that some of the checks got lost in the latest
> rework and that port 0 isn't actually a console port. I'll go through
> init code again to ensure this. Thanks for explaining the scenario!

There are a couple of problems with what I said:
- I had to make some changes in the kernel driver because hvc didn't
  work as I expected. If initial consoles were spawned (currently only
  on s390 and powerpc), new ports have to arrive in the same order as
  init consoles were spawned.

  This means /dev/console will be bound to the first console port that
  gets initialised. It's desirable to always have port0 get that
  distinction.

- We won't know if the guest supports multiple ports till the guest
  driver does a probe(). That's much much later than when port drivers
  in qemu are initialised.

So we will have to spawn a console port at id 0 when the bus is
initialised.

There are a couple of problems though:
- How to identify which chardev to associate the port#0 with?
- If there is a virtioconsole device specified on the command line,
  should that be used as port0? Or should that mean we spawn two
  consoles?

I guess for both these cases, some special command line tweaks will be
needed? Or keep the old '-virtioconsole' parameter and put that up as
port0?

		Amit

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Gerd Hoffmann]

   Hi,

> So we will have to spawn a console port at id 0 when the bus is
> initialised.
>
> There are a couple of problems though:
> - How to identify which chardev to associate the port#0 with?

I'd suggest to give the serial-bus a chardev attribute, which is then 
simply passed through, i.e.

   -device virtio-serial-bus,chardev=<name>

automatically creates a virtioconsole with port=0 and chardev=<name> on 
the newly created bus.

> - If there is a virtioconsole device specified on the command line,
>    should that be used as port0? Or should that mean we spawn two
>    consoles?

Two consoles.

> I guess for both these cases, some special command line tweaks will be
> needed? Or keep the old '-virtioconsole' parameter and put that up as
> port0?

See above.

Keeping -virtioconsole for backward compatibility is easy, it would 
basically create a chardev with a virtio<nr> label as it does today, 
then create virtio-serial-bus with chardev=virtio<nr>.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/6] char: Emit 'OPENED' events on char device open

[Anthony Liguori]

Amit Shah wrote:
> On (Wed) Sep 30 2009 [16:13:33], Anthony Liguori wrote:
>   
>> Amit Shah wrote:
>>     
>>> Notify users of the char interface whenever the file / connection is
>>> opened.
>>>
>>> The existing RESET event is triggered when the qemu char state is reset
>>> as well; which may not be as interesting as char device open events.
>>>
>>> Signed-off-by: Amit Shah <amit.shah@redhat.com>
>>>   
>>>       
>> This series breaks the build (ppc440_bamboo.c).
>>     
>
> This patch or this series? If it's this patch, that's surprising!
>
>   
>> You should always build without an explicit --target-list to avoid this  
>> in the future.
>>     
>
> Sure -- I intend to when I send a request for the merging once all the
> comments have been sorted out.
>   

If you don't intend a patch series to be merged, then you should put 
[RFC] in the subject.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/6] char: Emit 'OPENED' events on char device open

[Gerd Hoffmann]

On 10/01/09 14:54, Anthony Liguori wrote:
>>> You should always build without an explicit --target-list to avoid
>>> this in the future.
>>
>> Sure -- I intend to when I send a request for the merging once all the
>> comments have been sorted out.
>
> If you don't intend a patch series to be merged, then you should put
> [RFC] in the subject.

Huh?  I'm a bit surprised you are considering patches for merge which 
are discussed alot on the list and thus most likely not final.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/6] char: Emit 'OPENED' events on char device open

[Anthony Liguori]

Gerd Hoffmann wrote:
> On 10/01/09 14:54, Anthony Liguori wrote:
>>>> You should always build without an explicit --target-list to avoid
>>>> this in the future.
>>>
>>> Sure -- I intend to when I send a request for the merging once all the
>>> comments have been sorted out.
>>
>> If you don't intend a patch series to be merged, then you should put
>> [RFC] in the subject.
>
> Huh?  I'm a bit surprised you are considering patches for merge which 
> are discussed alot on the list and thus most likely not final.

Amit has repeatedly asked me to consider these series for merging.  If 
there is no indicator in the subject that they are not intended to be 
merged, then they're going to go through staging.

I'll remove them from staging once I see that the patches are 
contended.  The reason I do this is to make sure that patches don't get 
dropped.

There's no excuse for a patch to be on the mailing list that isn't 
properly tested (especially build tested) without an appropriate tag 
indicating that like [RFC].

Regards,

Anthony Liguori

> cheers,
>   Gerd

Re: [Qemu-devel] [PATCH 1/6] char: Emit 'OPENED' events on char device open

[Gerd Hoffmann]

On 10/01/09 15:48, Anthony Liguori wrote:

> I'll remove them from staging once I see that the patches are contended.
> The reason I do this is to make sure that patches don't get dropped.

Stick everything into the staging queue, solve conflicts, just to throw 
them out again later (which might produce conflicts again) doesn't look 
like a good workflow to me.  You spend time solving patch conflicts for 
nothing.

There must be better ways to keep track of the patches ...

> There's no excuse for a patch to be on the mailing list that isn't
> properly tested (especially build tested) without an appropriate tag
> indicating that like [RFC].

Indeed.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Amit Shah]

On (Thu) Oct 01 2009 [14:15:53], Gerd Hoffmann wrote:
>   Hi,
>
>> So we will have to spawn a console port at id 0 when the bus is
>> initialised.
>>
>> There are a couple of problems though:
>> - How to identify which chardev to associate the port#0 with?
>
> I'd suggest to give the serial-bus a chardev attribute, which is then  
> simply passed through, i.e.
>
>   -device virtio-serial-bus,chardev=<name>
>
> automatically creates a virtioconsole with port=0 and chardev=<name> on  
> the newly created bus.

Hm, this looks weird. Because on one hand we're talking about decoupling
the char driver from the core (virtio-serial-bus) and here we're
actually attaching a char driver to the bus.

>> I guess for both these cases, some special command line tweaks will be
>> needed? Or keep the old '-virtioconsole' parameter and put that up as
>> port0?
>
> See above.
>
> Keeping -virtioconsole for backward compatibility is easy, it would  
> basically create a chardev with a virtio<nr> label as it does today,  
> then create virtio-serial-bus with chardev=virtio<nr>.

I prefer to remove the -virtioconsole argument because we won't be able
to specify the bus that's to be attached to.

Also, we can't say if a virtio-serial-bus is also specified. If one
is, we'd want the console to attach to that bus. If one isn't, we'd want
to spawn a new one in that case. But as per above, a virtio-serial-bus
should always spawn a virtio-console port.

I fear we'll end up in a mess of tricky if-else conditions.

And I can't think of a nice solution to the problem above right now.

		Amit

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Gerd Hoffmann]

On 10/07/09 11:25, Amit Shah wrote:
>>    -device virtio-serial-bus,chardev=<name>
>>
>> automatically creates a virtioconsole with port=0 and chardev=<name>  on
>> the newly created bus.
>
> Hm, this looks weird. Because on one hand we're talking about decoupling
> the char driver from the core (virtio-serial-bus) and here we're
> actually attaching a char driver to the bus.

We don't actually attach the chardev to the bus though.  It is just 
passed through to the auto-created console port #0.

But it looks a bit weird indeed.  I'm open to better suggestions to 
address the "port #0 must be console for backward compatibility reasons" 
issue.

>> Keeping -virtioconsole for backward compatibility is easy, it would
>> basically create a chardev with a virtio<nr>  label as it does today,
>> then create virtio-serial-bus with chardev=virtio<nr>.
>
> I prefer to remove the -virtioconsole argument because we won't be able
> to specify the bus that's to be attached to.

Oh, I thought it would create a new virtio-serial-bus (plus auto-created 
port0 console) unconditionally.  Just do enougth to keep existing users 
of the switch working.

If you want new features (i.e. two consoles ports attached to one 
virtio-serial-bus device) you must use the new syntax.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Amit Shah]

On (Wed) Oct 07 2009 [11:51:14], Gerd Hoffmann wrote:
> On 10/07/09 11:25, Amit Shah wrote:
>>>    -device virtio-serial-bus,chardev=<name>
>>>
>>> automatically creates a virtioconsole with port=0 and chardev=<name>  on
>>> the newly created bus.
>>
>> Hm, this looks weird. Because on one hand we're talking about decoupling
>> the char driver from the core (virtio-serial-bus) and here we're
>> actually attaching a char driver to the bus.
>
> We don't actually attach the chardev to the bus though.  It is just  
> passed through to the auto-created console port #0.

Yes, but it's unituitive... and weird.

> But it looks a bit weird indeed.  I'm open to better suggestions to  
> address the "port #0 must be console for backward compatibility reasons"  
> issue.

Hm, me too. I'll think about this more.

>>> Keeping -virtioconsole for backward compatibility is easy, it would
>>> basically create a chardev with a virtio<nr>  label as it does today,
>>> then create virtio-serial-bus with chardev=virtio<nr>.
>>
>> I prefer to remove the -virtioconsole argument because we won't be able
>> to specify the bus that's to be attached to.
>
> Oh, I thought it would create a new virtio-serial-bus (plus auto-created  
> port0 console) unconditionally.  Just do enougth to keep existing users  
> of the switch working.
>
> If you want new features (i.e. two consoles ports attached to one  
> virtio-serial-bus device) you must use the new syntax.

So it's better overall to drop the old syntax altogether, right? It
could get easily confusing otherwise.

We can easily end up having:

-virtioconsole <chardev>
<auto-creates a bus and attaches a console port to it>

-device virtio-serial-pci,id=blah
<a second bus>

-device virtport,bus=blah.0

<and no way to connect a device to the bus that got created by
virtioconsole>

		Amit

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Gerd Hoffmann]

On 10/07/09 12:06, Amit Shah wrote:
>> Oh, I thought it would create a new virtio-serial-bus (plus auto-created
>> port0 console) unconditionally.  Just do enougth to keep existing users
>> of the switch working.
>>
>> If you want new features (i.e. two consoles ports attached to one
>> virtio-serial-bus device) you must use the new syntax.
>
> So it's better overall to drop the old syntax altogether, right? It
> could get easily confusing otherwise.
>
> We can easily end up having:
>
> -virtioconsole<chardev>
> <auto-creates a bus and attaches a console port to it>
>
> -device virtio-serial-pci,id=blah
> <a second bus>
>
> -device virtport,bus=blah.0
>
> <and no way to connect a device to the bus that got created by
> virtioconsole>

It isn't that bad.

First, the busses get names based on the bus type by default, i.e. when 
creating a scsi adapter without specifying id=seomthing the bus is 
simply named "scsi.0".  Likewise the -virtioconsole created bus would be 
"port.0" or simliar (depends on the name in BusInfo).

Second, the bus= argument is optional.  If not specified, qdev will pick 
the first bus of a matching type it finds.  So as long you have a single 
port/scsi/usb/... bus only you don't need bus= at all.  You can do:

  -virtioconsole <chardev> -device virtport,<args>

and it will work just fine (creating a bus with the autocreated console 
and additionally a virtport device attached to the same bus).

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Amit Shah]

On (Wed) Oct 07 2009 [13:33:57], Gerd Hoffmann wrote:
> On 10/07/09 12:06, Amit Shah wrote:
>>> Oh, I thought it would create a new virtio-serial-bus (plus auto-created
>>> port0 console) unconditionally.  Just do enougth to keep existing users
>>> of the switch working.
>>>
>>> If you want new features (i.e. two consoles ports attached to one
>>> virtio-serial-bus device) you must use the new syntax.
>>
>> So it's better overall to drop the old syntax altogether, right? It
>> could get easily confusing otherwise.
>>
>> We can easily end up having:
>>
>> -virtioconsole<chardev>
>> <auto-creates a bus and attaches a console port to it>
>>
>> -device virtio-serial-pci,id=blah
>> <a second bus>
>>
>> -device virtport,bus=blah.0
>>
>> <and no way to connect a device to the bus that got created by
>> virtioconsole>
>
> It isn't that bad.
>
> First, the busses get names based on the bus type by default, i.e. when  
> creating a scsi adapter without specifying id=seomthing the bus is  
> simply named "scsi.0".  Likewise the -virtioconsole created bus would be  
> "port.0" or simliar (depends on the name in BusInfo).
>
> Second, the bus= argument is optional.  If not specified, qdev will pick  
> the first bus of a matching type it finds.  So as long you have a single  
> port/scsi/usb/... bus only you don't need bus= at all.  You can do:

The problem with this is that the management solution needs to know then
what is the default bus name (which could change if the code gets
updated).

And also there's the other problem of a console port spawning a bus
(which could end up spawning another console port at #0...)

So IMO it's better to leave that command line param out.

		Amit

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Gerd Hoffmann]

On 10/07/09 13:42, Amit Shah wrote:
> On (Wed) Oct 07 2009 [13:33:57], Gerd Hoffmann wrote:
>> Second, the bus= argument is optional.  If not specified, qdev will pick
>> the first bus of a matching type it finds.  So as long you have a single
>> port/scsi/usb/... bus only you don't need bus= at all.  You can do:
>
> The problem with this is that the management solution needs to know then
> what is the default bus name (which could change if the code gets
> updated).

No problem.  Just don't use -virtioconsole.  Go with -device 
virtio-serial-bus,id=... + -device virtport,bus=.. then and explicitly 
name your devices (and thereby the buses too).

-virtioconsole should *really* be a pure backward compatibility thing. 
Use case:  You have a script starting qemu using -virtioconsole.  After 
upgrading qemu it should continue to work, i.e. create a device which 
the guest can use as before the upgrade and which is linked up to a 
chardev as it was before.

Anything which wants to use the new features can (and should) completely 
ignore -virtioconsole.  I just wanted to point out that mixing old and 
new style is *possible*.  It wasn't my intention to imply that I 
*recommend* doing that.

> And also there's the other problem of a console port spawning a bus
> (which could end up spawning another console port at #0...)

parse error.
I don't understand what problem you are trying to point out.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Amit Shah]

On (Wed) Oct 07 2009 [15:06:08], Gerd Hoffmann wrote:
> On 10/07/09 13:42, Amit Shah wrote:
>> On (Wed) Oct 07 2009 [13:33:57], Gerd Hoffmann wrote:
>>> Second, the bus= argument is optional.  If not specified, qdev will pick
>>> the first bus of a matching type it finds.  So as long you have a single
>>> port/scsi/usb/... bus only you don't need bus= at all.  You can do:
>>
>> The problem with this is that the management solution needs to know then
>> what is the default bus name (which could change if the code gets
>> updated).
>
> No problem.  Just don't use -virtioconsole.  Go with -device  
> virtio-serial-bus,id=... + -device virtport,bus=.. then and explicitly  
> name your devices (and thereby the buses too).
>
> -virtioconsole should *really* be a pure backward compatibility thing.  
> Use case:  You have a script starting qemu using -virtioconsole.  After  
> upgrading qemu it should continue to work, i.e. create a device which  
> the guest can use as before the upgrade and which is linked up to a  
> chardev as it was before.
>
> Anything which wants to use the new features can (and should) completely  
> ignore -virtioconsole.  I just wanted to point out that mixing old and  
> new style is *possible*.  It wasn't my intention to imply that I  
> *recommend* doing that.

There should be some way of deprecating commands in qemu. Maybe in 1-2
release cycles.

>> And also there's the other problem of a console port spawning a bus
>> (which could end up spawning another console port at #0...)
>
> parse error.
> I don't understand what problem you are trying to point out.

Oh; I was stuck at the earlier suggestion made by you:

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

which implies -device virtioconsole (with a port at id 0)

and then someone also doing

-virtioconsole ...

which would end up creating another bus for this current backward compat
reason.

		Amit

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Anthony Liguori]

Gerd Hoffmann wrote:
> On 10/07/09 13:42, Amit Shah wrote:
>> On (Wed) Oct 07 2009 [13:33:57], Gerd Hoffmann wrote:
>>> Second, the bus= argument is optional.  If not specified, qdev will 
>>> pick
>>> the first bus of a matching type it finds.  So as long you have a 
>>> single
>>> port/scsi/usb/... bus only you don't need bus= at all.  You can do:
>>
>> The problem with this is that the management solution needs to know then
>> what is the default bus name (which could change if the code gets
>> updated).
>
> No problem.  Just don't use -virtioconsole.  Go with -device 
> virtio-serial-bus,id=... + -device virtport,bus=.. then and explicitly 
> name your devices (and thereby the buses too).
>
> -virtioconsole should *really* be a pure backward compatibility thing. 
> Use case:  You have a script starting qemu using -virtioconsole.  
> After upgrading qemu it should continue to work, i.e. create a device 
> which the guest can use as before the upgrade and which is linked up 
> to a chardev as it was before.
>
> Anything which wants to use the new features can (and should) 
> completely ignore -virtioconsole.  I just wanted to point out that 
> mixing old and new style is *possible*.  It wasn't my intention to 
> imply that I *recommend* doing that.

I agree.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Gerd Hoffmann]

On 10/07/09 15:53, Amit Shah wrote:
> On (Wed) Oct 07 2009 [15:06:08], Gerd Hoffmann wrote:
> There should be some way of deprecating commands in qemu. Maybe in 1-2
> release cycles.

Yes, we should think about that, especially as the qdev switch brings 
alot of changes here.

> Oh; I was stuck at the earlier suggestion made by you:
>
> -device virtio-serial-pci,chardev=...
>
> which implies -device virtioconsole (with a port at id 0)
>
> and then someone also doing
>
> -virtioconsole ...
>
> which would end up creating another bus for this current backward compat
> reason.

Yes, mixing old and new style is good for surprises.
Better avoid that ;)

cheers,
   Gerd