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

[Markus Armbruster]

Amit Shah <address@hidden> writes:

> This patch migrates virtio-console to the qdev infrastructure and
> creates a new virtio-serial 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.

Old: Two devices virtio-console-pci and virtio-console-s390, as far as I
know converted to qdev except for some chardev hookup bits.

New: qdev bus virtio-serial-bus.  Two devices virtio-serial-pci and
virtio-serial-s390 provide this bus.  Device virtconsole goes on this
bus.

Standard question for a new bus: How are devices addressed on this bus?

If I understand the code correctly, the guest can identify devices by
name (e.g. "org.qemu.console.0") or by ID (which is uint32_t).  Correct?

> This interface enables spawning of multiple virtio consoles as well as generic
> serial ports.
>
> The older -virtconsole argument still works, but when using the new
> functionality, it is recommended to use
>
>     -device virtio-serial-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 in the
> guest.
>
> Care has been taken to ensure compatibility with kernels that do not
> support multiple ports as well as accepting incoming migrations from older
> qemu versions.
>
> Signed-off-by: Amit Shah <address@hidden>
> ---
>  Makefile.target        |    2 +-
>  hw/pc.c                |    9 -
>  hw/ppc440_bamboo.c     |    7 -
>  hw/qdev.c              |    8 +-
>  hw/s390-virtio-bus.c   |   16 +-
>  hw/s390-virtio-bus.h   |    1 +
>  hw/virtio-console.c    |  186 ++++------
>  hw/virtio-console.h    |   19 -
>  hw/virtio-pci.c        |   11 +-
>  hw/virtio-serial-bus.c |  964 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/virtio-serial.h     |  230 ++++++++++++
>  hw/virtio.h            |    2 +-
>  qemu-options.hx        |    4 +
>  sysemu.h               |    6 -
>  vl.c                   |   18 +-
>  15 files changed, 1317 insertions(+), 166 deletions(-)
>  delete mode 100644 hw/virtio-console.h
>  create mode 100644 hw/virtio-serial-bus.c
>  create mode 100644 hw/virtio-serial.h

Patch is huge.  I skimmed it, and looked a bit more closely at the
qdev-related bits, but it's hard to keep track of it among all the other
stuff, and it's quite possible that I missed something.

Please excuse any dumb questions regarding consoles and such; not
exactly my area of expertise.

> diff --git a/Makefile.target b/Makefile.target
> index 7c1f30c..74bb548 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -156,7 +156,7 @@ ifdef CONFIG_SOFTMMU
>  obj-y = vl.o async.o monitor.o pci.o pci_host.o pcie_host.o machine.o 
> gdbstub.o
>  # virtio has to be here due to weird dependency between PCI and virtio-net.
>  # need to fix this properly
> -obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o 
> virtio-pci.o
> +obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-serial-bus.o 
> virtio-console.o virtio-pci.o
>  obj-$(CONFIG_KVM) += kvm.o kvm-all.o
>  obj-$(CONFIG_ISA_MMIO) += isa_mmio.o
>  LIBS+=-lz
> diff --git a/hw/pc.c b/hw/pc.c
> index db7d58e..5e742bf 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1242,15 +1242,6 @@ static void pc_init1(ram_addr_t ram_size,
>          }
>      }
>  
> -    /* 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");
> -            }
> -        }
> -    }
> -
>      rom_load_fw(fw_cfg);
>  }
>  
> diff --git a/hw/ppc440_bamboo.c b/hw/ppc440_bamboo.c
> index a488240..1ab9872 100644
> --- a/hw/ppc440_bamboo.c
> +++ b/hw/ppc440_bamboo.c
> @@ -108,13 +108,6 @@ static void bamboo_init(ram_addr_t ram_size,
>      env = ppc440ep_init(&ram_size, &pcibus, pci_irq_nrs, 1, cpu_model);
>  
>      if (pcibus) {
> -        /* Add virtio console devices */
> -        for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
> -            if (virtcon_hds[i]) {
> -                pci_create_simple(pcibus, -1, "virtio-console-pci");
> -            }
> -        }
> -
>          /* Register network interfaces. */
>          for (i = 0; i < nb_nics; i++) {
>              /* There are no PCI NICs on the Bamboo board, but there are
> diff --git a/hw/qdev.c b/hw/qdev.c
> index b6bd4ae..38c6e15 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -321,13 +321,9 @@ void qdev_machine_creation_done(void)
>  CharDriverState *qdev_init_chardev(DeviceState *dev)
>  {
>      static int next_serial;
> -    static int next_virtconsole;
> +
>      /* FIXME: This is a nasty hack that needs to go away.  */
> -    if (strncmp(dev->info->name, "virtio", 6) == 0) {
> -        return virtcon_hds[next_virtconsole++];
> -    } else {
> -        return serial_hds[next_serial++];
> -    }
> +    return serial_hds[next_serial++];
>  }

I believe the FIXME is about the nasty special case for "virtio".  Since
you fix that, better remove the FIXME.

>  
>  BusState *qdev_get_parent_bus(DeviceState *dev)
> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
> index dc154ed..79ba9fc 100644
> --- a/hw/s390-virtio-bus.c
> +++ b/hw/s390-virtio-bus.c
> @@ -26,7 +26,7 @@
>  #include "loader.h"
>  #include "elf.h"
>  #include "hw/virtio.h"
> -#include "hw/virtio-console.h"
> +#include "hw/virtio-serial.h"
>  #include "hw/sysbus.h"
>  #include "kvm.h"
>  
> @@ -130,7 +130,7 @@ static int s390_virtio_blk_init(VirtIOS390Device *dev)
>      return s390_virtio_device_init(dev, vdev);
>  }
>  
> -static int s390_virtio_console_init(VirtIOS390Device *dev)
> +static int s390_virtio_serial_init(VirtIOS390Device *dev)
>  {
>      VirtIOS390Bus *bus;
>      VirtIODevice *vdev;
> @@ -138,7 +138,7 @@ static int s390_virtio_console_init(VirtIOS390Device *dev)
>  
>      bus = DO_UPCAST(VirtIOS390Bus, bus, dev->qdev.parent_bus);
>  
> -    vdev = virtio_console_init((DeviceState *)dev);
> +    vdev = virtio_serial_init((DeviceState *)dev, dev->max_virtserial_ports);
>      if (!vdev) {
>          return -1;
>      }
> @@ -336,11 +336,13 @@ static VirtIOS390DeviceInfo s390_virtio_blk = {
>      },
>  };
>  
> -static VirtIOS390DeviceInfo s390_virtio_console = {
> -    .init = s390_virtio_console_init,
> -    .qdev.name = "virtio-console-s390",
> +static VirtIOS390DeviceInfo s390_virtio_serial = {
> +    .init = s390_virtio_serial_init,
> +    .qdev.name = "virtio-serial-s390",
>      .qdev.size = sizeof(VirtIOS390Device),
>      .qdev.props = (Property[]) {
> +        DEFINE_PROP_UINT32("max_ports", VirtIOS390Device, 
> max_virtserial_ports,
> +                           15),
>          DEFINE_PROP_END_OF_LIST(),
>      },
>  };
> @@ -364,7 +366,7 @@ static void 
> s390_virtio_bus_register_withprop(VirtIOS390DeviceInfo *info)
>  
>  static void s390_virtio_register(void)
>  {
> -    s390_virtio_bus_register_withprop(&s390_virtio_console);
> +    s390_virtio_bus_register_withprop(&s390_virtio_serial);
>      s390_virtio_bus_register_withprop(&s390_virtio_blk);
>      s390_virtio_bus_register_withprop(&s390_virtio_net);
>  }
> diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
> index ef36714..42e56ce 100644
> --- a/hw/s390-virtio-bus.h
> +++ b/hw/s390-virtio-bus.h
> @@ -40,6 +40,7 @@ typedef struct VirtIOS390Device {
>      VirtIODevice *vdev;
>      DriveInfo *dinfo;
>      NICConf nic;
> +    uint32_t max_virtserial_ports;

Could use a comment.

>  } VirtIOS390Device;
>  
>  typedef struct VirtIOS390Bus {
> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> index 57f8f89..b2e4eb1 100644
> --- a/hw/virtio-console.c
> +++ b/hw/virtio-console.c
[...]

As others already noted, this part is hard to review, because you
replace the file contents wholesale.  Here's the resulting file:

   /*
    * Virtio Console and Generic Port Devices
    *
    * Copyright Red Hat, Inc. 2009
    *
    * Authors:
    *  Amit Shah <address@hidden>
    *
    * This work is licensed under the terms of the GNU GPL, version 2.  See
    * the COPYING file in the top-level directory.
    */

   #include "qemu-char.h"
   #include "virtio-serial.h"

   typedef struct VirtConsole {
       VirtIOSerialPort port;
       CharDriverState *chr;
   } VirtConsole;


   /* Callback function that's called when the guest sends us data */
   static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t 
len)
   {
       VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);

       return qemu_chr_write(vcon->chr, buf, len);
   }

   /* Readiness of the guest to accept data on a port */
   static int chr_can_read(void *opaque)
   {
       VirtConsole *vcon = opaque;

       return virtio_serial_guest_ready(&vcon->port);
   }

   /* Send data from a char device over to the guest */
   static void chr_read(void *opaque, const uint8_t *buf, int size)
   {
       VirtConsole *vcon = opaque;

       virtio_serial_write(&vcon->port, buf, size);
   }

   static void chr_event(void *opaque, int event)
   {
       VirtConsole *vcon = opaque;

       switch (event) {
       case CHR_EVENT_OPENED: {
           virtio_serial_open(&vcon->port);
           break;
       }
       case CHR_EVENT_CLOSED:
           virtio_serial_close(&vcon->port);
           break;
       }
   }

   /* Virtio Console Ports */
   static int vcon_initfn(VirtIOSerialDevice *dev)
   {
       VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
       VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);

       port->info = dev->info;

       /*
        * We're not interested in data the guest sends while nothing is
        * connected on the host side. Just ignore it instead of saving it
        * for later consumption
        */
       port->cache_buffers = 0;

       /* Tell the guest we're a console so it attaches us to an hvc console */
       port->is_console = true;

       /*
        * 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;

I.e. if the port is a console, it gets born connected to /dev/hvc0,
correct?

"Set this here" doesn't help much.  Perhaps you could reword the comment
to state that consoles start life connected.

Can we have multiple console devices?

       if (vcon->chr) {
           qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
                                 vcon);
       }
       return 0;
   }

   static int vcon_exitfn(VirtIOSerialDevice *dev)
   {
       VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
       VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);

       if (vcon->chr)
           qemu_chr_close(vcon->chr);

       return 0;
   }

   static VirtIOSerialPortInfo virtcon_info = {
       .qdev.name     = "virtconsole",
       .qdev.size     = sizeof(VirtConsole),
       .init          = vcon_initfn,
       .exit          = vcon_exitfn,
       .have_data     = flush_buf,
       .qdev.props = (Property[]) {
           DEFINE_PROP_CHR("chardev", VirtConsole, chr),
           DEFINE_PROP_STRING("name", VirtConsole, port.name),
           DEFINE_PROP_END_OF_LIST(),
       },
   };

   static void virtcon_register(void)
   {
       virtio_serial_port_qdev_register(&virtcon_info);
   }
   device_init(virtcon_register)

> diff --git a/hw/virtio-console.h b/hw/virtio-console.h
> deleted file mode 100644
> index 84d0717..0000000
> --- a/hw/virtio-console.h
> +++ /dev/null
> @@ -1,19 +0,0 @@
> -/*
> - * Virtio Console Support
> - *
> - * Copyright IBM, Corp. 2008
> - *
> - * Authors:
> - *  Christian Ehrhardt <address@hidden>
> - *
> - * This work is licensed under the terms of the GNU GPL, version 2.  See
> - * the COPYING file in the top-level directory.
> - *
> - */
> -#ifndef _QEMU_VIRTIO_CONSOLE_H
> -#define _QEMU_VIRTIO_CONSOLE_H
> -
> -/* The ID for virtio console */
> -#define VIRTIO_ID_CONSOLE 3
> -
> -#endif
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 62b46bd..1baca0d 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -92,6 +92,7 @@ typedef struct {
>      uint32_t nvectors;
>      DriveInfo *dinfo;
>      NICConf nic;
> +    uint32_t max_virtserial_ports;
>  } VirtIOPCIProxy;
>  
>  /* virtio device */
> @@ -481,7 +482,7 @@ static int virtio_blk_exit_pci(PCIDevice *pci_dev)
>      return virtio_exit_pci(pci_dev);
>  }
>  
> -static int virtio_console_init_pci(PCIDevice *pci_dev)
> +static int virtio_serial_init_pci(PCIDevice *pci_dev)
>  {
>      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>      VirtIODevice *vdev;
> @@ -491,7 +492,7 @@ static int virtio_console_init_pci(PCIDevice *pci_dev)
>          proxy->class_code != PCI_CLASS_OTHERS)          /* qemu-kvm  */
>          proxy->class_code = PCI_CLASS_COMMUNICATION_OTHER;
>  
> -    vdev = virtio_console_init(&pci_dev->qdev);
> +    vdev = virtio_serial_init(&pci_dev->qdev, proxy->max_virtserial_ports);
>      if (!vdev) {
>          return -1;
>      }
> @@ -569,12 +570,14 @@ static PCIDeviceInfo virtio_info[] = {
>          },
>          .qdev.reset = virtio_pci_reset,
>      },{
> -        .qdev.name = "virtio-console-pci",
> +        .qdev.name = "virtio-serial-pci",
>          .qdev.size = sizeof(VirtIOPCIProxy),
> -        .init      = virtio_console_init_pci,
> +        .init      = virtio_serial_init_pci,
>          .exit      = virtio_exit_pci,
>          .qdev.props = (Property[]) {
>              DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
> +            DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, 
> max_virtserial_ports,
> +                               15),
>              DEFINE_PROP_END_OF_LIST(),
>          },
>          .qdev.reset = virtio_pci_reset,
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> new file mode 100644
> index 0000000..fd0ea12
> --- /dev/null
> +++ b/hw/virtio-serial-bus.c
> @@ -0,0 +1,964 @@
> +/*
> + * A bus for connecting virtio serial and console ports
> + *
> + * Copyright (C) 2009 Red Hat, Inc.
> + *
> + * Author(s):
> + *  Amit Shah <address@hidden>
> + *
> + * Some earlier parts are:
> + *  Copyright IBM, Corp. 2008
> + * authored by
> + *  Christian Ehrhardt <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "monitor.h"
> +#include "qemu-queue.h"
> +#include "sysbus.h"
> +#include "virtio-serial.h"
> +
> +/* The virtio-serial bus on top of which the ports will ride as devices */
> +struct VirtIOSerialBus {
> +    BusState qbus;
> +    VirtIOSerial *vser;

Is this the device providing the bus?

PCIBus calls that parent_dev.  If you don't want to change your name,
what about a comment?

> +    uint32_t max_nr_ports;

Could use a comment.

How does this play together with member max_virtserial_ports of
VirtIOPCIProxy and VirtIOS390Device?

> +};
> +
> +struct VirtIOSerial {
> +    VirtIODevice vdev;
> +
> +    VirtQueue *c_ivq, *c_ovq;
> +    /* Arrays of ivqs and ovqs: one per port */
> +    VirtQueue **ivqs, **ovqs;
> +
> +    VirtIOSerialBus *bus;
> +
> +    QTAILQ_HEAD(, VirtIOSerialPort) ports_head;
> +    struct virtio_console_config config;

Is virtio_console_config still an appropriate name?  It configures a
virtio-serial device, not the virtconsole device.

> +
> +    /*
> +     * This lock serialises writes to the guest via the ivq
> +     */
> +    pthread_mutex_t ivq_lock;
> +
> +    uint32_t guest_features;
> +};
> +
> +/* This struct holds individual buffers received for each port */
> +typedef struct VirtIOSerialPortBuffer {
> +    QTAILQ_ENTRY(VirtIOSerialPortBuffer) next;
> +
> +    uint8_t *buf;
> +
> +    size_t len; /* length of the buffer */
> +    size_t offset; /* offset from which to consume data in the buffer */
> +
> +    uint32_t flags; /* Sent by guest (start of data stream, end of stream) */
> +
> +    bool previous_failure; /* Did sending out this buffer fail previously? */
> +} VirtIOSerialPortBuffer;
> +
> +static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
> +{
> +    VirtIOSerialPort *port;
> +
> +    QTAILQ_FOREACH(port, &vser->ports_head, next) {
> +        if (port->id == id)
> +            return port;
> +    }
> +    return NULL;
> +}
> +
> +static VirtIOSerialPort *find_port_by_vq(VirtIOSerial *vser, VirtQueue *vq)
> +{
> +    VirtIOSerialPort *port;
> +
> +    QTAILQ_FOREACH(port, &vser->ports_head, next) {
> +        if (port->ivq == vq || port->ovq == vq)
> +            return port;
> +    }
> +    return NULL;
> +}
> +
> +static bool use_multiport(VirtIOSerial *vser)
> +{
> +    return vser->guest_features & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
> +}
> +
> +static size_t write_to_port(VirtIOSerialPort *port,
> +                            const uint8_t *buf, size_t size)
> +{
> +    VirtQueueElement elem;
> +    struct virtio_console_header header;
> +    VirtQueue *vq;
> +    size_t offset = 0;
> +    size_t len = 0;
> +    int header_len;
> +
> +    vq = port->ivq;
> +    if (!virtio_queue_ready(vq)) {
> +        return 0;
> +    }
> +    if (!size) {
> +        return 0;
> +    }
> +    header.flags = 0;
> +    header_len = use_multiport(port->vser) ? sizeof(header) : 0;
> +
> +    pthread_mutex_lock(&port->vser->ivq_lock);
> +    while (offset < size) {
> +        int i;
> +
> +        if (!virtqueue_pop(vq, &elem)) {
> +            break;
> +        }
> +        if (elem.in_sg[0].iov_len < header_len) {
> +            /* We can't even store our port number in this buffer. Bug? */
> +            qemu_error("virtio-serial: size %zd less than expected\n",
> +                    elem.in_sg[0].iov_len);
> +            exit(1);
> +        }
> +        if (header_len) {
> +            memcpy(elem.in_sg[0].iov_base, &header, header_len);
> +        }
> +
> +        for (i = 0; offset < size && i < elem.in_num; i++) {
> +            /* Copy the header only in the first sg. */
> +            len = MIN(elem.in_sg[i].iov_len - header_len, size - offset);
> +
> +            memcpy(elem.in_sg[i].iov_base + header_len, buf + offset, len);
> +            offset += len;
> +            header_len = 0;
> +        }
> +        header_len = use_multiport(port->vser) ? sizeof(header) : 0;
> +        virtqueue_push(vq, &elem, len + header_len);
> +    }
> +
> +    virtio_notify(&port->vser->vdev, vq);
> +    pthread_mutex_unlock(&port->vser->ivq_lock);
> +    return offset;
> +}
> +
> +static size_t send_control_event(VirtIOSerialPort *port, void *buf, size_t 
> len)
> +{
> +    VirtQueueElement elem;
> +    VirtQueue *vq;
> +    struct virtio_console_control *cpkt;
> +
> +    vq = port->vser->c_ivq;
> +    if (!virtio_queue_ready(vq)) {
> +        return 0;
> +    }
> +    if (!virtqueue_pop(vq, &elem)) {
> +        return 0;
> +    }
> +
> +    cpkt = (struct virtio_console_control *)buf;
> +    cpkt->id = port->id;
> +    memcpy(elem.in_sg[0].iov_base, buf, len);
> +
> +    virtqueue_push(vq, &elem, len);
> +    virtio_notify(&port->vser->vdev, vq);
> +    return len;
> +}
> +
> +static size_t get_complete_data_size(VirtIOSerialPort *port)
> +{
> +    VirtIOSerialPortBuffer *buf;
> +    size_t size;
> +    bool is_complete, start_seen;
> +
> +    size = 0;
> +    is_complete = false;
> +    start_seen = false;
> +    QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) {
> +        size += buf->len - buf->offset;
> +
> +        if (buf->flags & VIRTIO_CONSOLE_HDR_END_DATA) {
> +            is_complete = true;
> +            break;
> +        }
> +        if (buf == QTAILQ_FIRST(&port->unflushed_buffer_head)
> +            && !(buf->flags & VIRTIO_CONSOLE_HDR_START_DATA)) {
> +
> +            /* There's some data that arrived without a START flag. Flush 
> it. */
> +            is_complete = true;
> +            break;
> +        }
> +
> +        if (buf->flags & VIRTIO_CONSOLE_HDR_START_DATA) {
> +            if (start_seen) {
> +                /*
> +                 * There's some data that arrived without an END
> +                 * flag. Flush it.
> +                 */
> +                size -= buf->len + buf->offset;
> +                is_complete = true;
> +                break;
> +            }
> +            start_seen = true;
> +        }
> +    }
> +    return is_complete ? size : 0;
> +}
> +
> +/*
> + * The guest could have sent the data corresponding to one write
> + * request split up in multiple buffers. The first buffer has the
> + * VIRTIO_CONSOLE_HDR_START_DATA flag set and the last buffer has the
> + * VIRTIO_CONSOLE_HDR_END_DATA flag set. Using this information, merge
> + * the parts into one buf here to process it for output.
> + */
> +static VirtIOSerialPortBuffer *get_complete_buf(VirtIOSerialPort *port)
> +{
> +    VirtIOSerialPortBuffer *buf, *buf2;
> +    uint8_t *outbuf;
> +    size_t out_offset, out_size;
> +
> +    out_size = get_complete_data_size(port);
> +    if (!out_size)
> +        return NULL;
> +
> +    buf = QTAILQ_FIRST(&port->unflushed_buffer_head);
> +    if (buf->len - buf->offset == out_size) {
> +        QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next);
> +        return buf;
> +    }
> +    out_offset = 0;
> +    outbuf = qemu_malloc(out_size);
> +
> +    QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffer_head, next, buf2) {
> +        size_t copy_size;
> +
> +        copy_size = buf->len - buf->offset;
> +        memcpy(outbuf + out_offset, buf->buf + buf->offset, copy_size);
> +        out_offset += copy_size;
> +
> +        QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next);
> +        qemu_free(buf->buf);
> +
> +        if (out_offset == out_size) {
> +            break;
> +        }
> +        qemu_free(buf);
> +    }
> +    buf->buf = outbuf;
> +    buf->len = out_size;
> +    buf->offset = 0;
> +    buf->flags = VIRTIO_CONSOLE_HDR_START_DATA | VIRTIO_CONSOLE_HDR_END_DATA;
> +    buf->previous_failure = false;
> +
> +    return buf;
> +}
> +
> +/* Call with the buffer_list_lock held */
> +static void flush_queue(VirtIOSerialPort *port)
> +{
> +    VirtIOSerialPortBuffer *buf;
> +    size_t out_size;
> +    ssize_t ret;
> +
> +    /*
> +     * If a device is interested in buffering packets till it's
> +     * opened, cache the data the guest sends us till a connection is
> +     * established.
> +     */
> +    if (!port->host_connected && port->cache_buffers) {
> +        return;
> +    }
> +    while ((buf = get_complete_buf(port))) {
> +        out_size = buf->len - buf->offset;
> +        if (!port->host_connected) {
> +            /*
> +             * Caching is disabled and host is not connected, so
> +             * discard the buffer. Do this only after merging the
> +             * buffer as a port can get connected in the middle of
> +             * dropping buffers and the port will end up getting the
> +             * incomplete output.
> +             */
> +            port->nr_bytes -= buf->len + buf->offset;
> +            qemu_free(buf->buf);
> +            qemu_free(buf);
> +            continue;
> +        }
> +
> +        ret = port->info->have_data(port, buf->buf + buf->offset, out_size);
> +        if (ret < out_size) {
> +            QTAILQ_INSERT_HEAD(&port->unflushed_buffer_head, buf, next);
> +        }
> +        if (ret <= 0) {
> +            /* We're not progressing at all */
> +            if (buf->previous_failure) {
> +                break;
> +            }
> +            buf->previous_failure = true;
> +        } else {
> +            buf->offset += ret;
> +            port->nr_bytes -= ret;
> +            buf->previous_failure = false;
> +        }
> +        if (!(buf->len - buf->offset)) {
> +            qemu_free(buf->buf);
> +            qemu_free(buf);
> +        }
> +    }
> +
> +    if (port->host_throttled && port->nr_bytes < port->byte_limit) {
> +        struct virtio_console_control cpkt;
> +
> +        port->host_throttled = false;
> +        cpkt.event = VIRTIO_CONSOLE_THROTTLE_PORT;
> +        cpkt.value = false;
> +        send_control_event(port, &cpkt, sizeof(cpkt));
> +    }
> +}
> +
> +static void flush_all_ports(VirtIOSerial *vser)
> +{
> +    struct VirtIOSerialPort *port;
> +
> +    QTAILQ_FOREACH(port, &vser->ports_head, next) {
> +        pthread_mutex_lock(&port->buffer_list_lock);
> +        if (port->has_activity) {
> +            port->has_activity = false;
> +            flush_queue(port);
> +        }
> +        pthread_mutex_unlock(&port->buffer_list_lock);
> +    }
> +}
> +
> +static void remove_port_buffers(VirtIOSerialPort *port)
> +{
> +    struct VirtIOSerialPortBuffer *buf, *buf2;
> +
> +    pthread_mutex_lock(&port->buffer_list_lock);
> +    QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffer_head, next, buf2) {
> +        QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next);
> +        qemu_free(buf->buf);
> +        qemu_free(buf);
> +    }
> +    pthread_mutex_unlock(&port->buffer_list_lock);
> +}
> +
> +/* Functions for use inside qemu to open and read from/write to ports */
> +int virtio_serial_open(VirtIOSerialPort *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, sizeof(cpkt));
> +
> +    /* Flush any buffers that were cached while the port was closed */
> +    if (port->cache_buffers && port->info->have_data) {
> +        pthread_mutex_lock(&port->buffer_list_lock);
> +        flush_queue(port);
> +        pthread_mutex_unlock(&port->buffer_list_lock);
> +    }
> +    return 0;
> +}
> +
> +int virtio_serial_close(VirtIOSerialPort *port)
> +{
> +    struct virtio_console_control cpkt;
> +
> +    port->host_connected = false;
> +    cpkt.event = VIRTIO_CONSOLE_PORT_OPEN;
> +    cpkt.value = 0;
> +    send_control_event(port, &cpkt, sizeof(cpkt));
> +
> +    if (!port->cache_buffers) {
> +        remove_port_buffers(port);
> +    }
> +    return 0;
> +}
> +
> +/* Individual ports/apps call this function to write to the guest. */
> +ssize_t virtio_serial_write(VirtIOSerialPort *port, const uint8_t *buf,
> +                            size_t size)
> +{
> +    if (!port || !port->host_connected || !port->guest_connected) {
> +        return 0;
> +    }
> +    return write_to_port(port, buf, size);
> +}
> +
> +/*
> + * Readiness of the guest to accept data on a port.
> + * Returns max. data the guest can receive
> + */
> +size_t virtio_serial_guest_ready(VirtIOSerialPort *port)
> +{
> +    VirtQueue *vq = port->ivq;
> +    size_t size, header_len;
> +
> +    if (use_multiport(port->vser)) {
> +        header_len = sizeof(struct virtio_console_header);
> +    } else {
> +        header_len = 0;
> +    }
> +    if (!virtio_queue_ready(vq) ||
> +        !(port->vser->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
> +        virtio_queue_empty(vq)) {
> +        return 0;
> +    }
> +    if (use_multiport(port->vser) && !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;
> +}
> +
> +/* Guest wants to notify us of some event */
> +static void handle_control_message(VirtIOSerial *vser, void *buf)
> +{
> +    struct VirtIOSerialPort *port;
> +    struct virtio_console_control *cpkt;
> +    uint8_t *buffer;
> +    size_t buffer_len;
> +
> +    cpkt = buf;
> +    port = find_port_by_id(vser, cpkt->id);
> +    if (!port)
> +        return;
> +
> +    switch(cpkt->event) {
> +    case VIRTIO_CONSOLE_PORT_READY:
> +        /*
> +         * Now that we know the guest asked for the port name, we're
> +         * sure the guest has initialised whatever state is necessary
> +         * for this port. Now's a good time to let the guest know if
> +         * this port is a console port so that the guest can hook it
> +         * up to hvc.
> +         */
> +        if (port->is_console) {
> +            cpkt->event = VIRTIO_CONSOLE_CONSOLE_PORT;
> +            cpkt->value = 1;
> +            send_control_event(port, cpkt, sizeof(cpkt));
> +        }
> +        if (port->name) {
> +            cpkt->event = VIRTIO_CONSOLE_PORT_NAME;
> +            cpkt->value = 1;
> +
> +            buffer_len = sizeof(*cpkt) + strlen(port->name) + 1;
> +            buffer = qemu_malloc(buffer_len);
> +
> +            memcpy(buffer, cpkt, sizeof(*cpkt));
> +            memcpy(buffer + sizeof(*cpkt), port->name, strlen(port->name));
> +            buffer[buffer_len - 1] = 0;
> +
> +            send_control_event(port, buffer, buffer_len);
> +            qemu_free(buffer);
> +        }
> +        /*
> +         * We also want to signal to the guest whether or not the port
> +         * is set to caching the buffers when disconnected.
> +         */
> +        if (port->cache_buffers) {
> +            cpkt->event = VIRTIO_CONSOLE_CACHE_BUFFERS;
> +            cpkt->value = 1;
> +            send_control_event(port, cpkt, sizeof(cpkt));
> +        }
> +        if (port->host_connected) {
> +            cpkt->event = VIRTIO_CONSOLE_PORT_OPEN;
> +            cpkt->value = 1;
> +            send_control_event(port, cpkt, sizeof(cpkt));
> +        }
> +        /*
> +         * When the guest has asked us for this information it means
> +         * the guest is all setup and has its virtqueues
> +         * initialised. If some app is interested in knowing about
> +         * this event, let it know
> +         */
> +        if (port->info->guest_ready) {
> +            port->info->guest_ready(port);
> +        }
> +        break;
> +    case VIRTIO_CONSOLE_PORT_OPEN:
> +        port->guest_connected = cpkt->value;
> +        if (cpkt->value && port->info->guest_open) {
> +            /* Send the guest opened notification if an app is interested */
> +            port->info->guest_open(port);
> +        }
> +        if (!cpkt->value && port->info->guest_close) {
> +            /* Send the guest closed notification if an app is interested */
> +            port->info->guest_close(port);
> +        }
> +        break;
> +    }
> +}
> +
> +static void control_in(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +}
> +
> +static void control_out(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    VirtQueueElement elem;
> +    VirtIOSerial *vser;
> +
> +    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
> +
> +    while (virtqueue_pop(vq, &elem)) {
> +        handle_control_message(vser, elem.out_sg[0].iov_base);
> +        virtqueue_push(vq, &elem, elem.out_sg[0].iov_len);
> +    }
> +    virtio_notify(vdev, vq);
> +}
> +
> +/*
> + * Guest wrote something to some port.
> + *
> + * Flush the data in the entire chunk that we received rather than
> + * splitting it into multiple buffers. VNC clients don't consume split
> + * buffers
> + */
> +static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    VirtIOSerial *vser;
> +    VirtQueueElement elem;
> +
> +    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
> +
> +    while (virtqueue_pop(vq, &elem)) {
> +        VirtIOSerialPort *port;
> +        VirtIOSerialPortBuffer *buf;
> +        struct virtio_console_header header;
> +        int header_len;
> +
> +        header_len = use_multiport(vser) ? sizeof(header) : 0;
> +
> +        if (elem.out_sg[0].iov_len < header_len) {
> +            goto next_buf;
> +        }
> +        if (header_len) {
> +            memcpy(&header, elem.out_sg[0].iov_base, header_len);
> +        }
> +        port = find_port_by_vq(vser, vq);
> +        if (!port) {
> +            goto next_buf;
> +        }
> +        /*
> +         * A port may not have any handler registered for consuming the
> +         * data that the guest sends or it may not have a chardev associated
> +         * with it. Just ignore the data in that case.
> +         */
> +        if (!port->info->have_data) {
> +            goto next_buf;
> +        }
> +
> +        /* The guest always sends only one sg */
> +        buf = qemu_mallocz(sizeof(*buf));
> +        buf->len = elem.out_sg[0].iov_len - header_len;
> +        buf->buf = qemu_malloc(buf->len);
> +        memcpy(buf->buf, elem.out_sg[0].iov_base + header_len, buf->len);
> +
> +        if (header_len) {
> +            /*
> +             * Only the first buffer in a stream will have this
> +             * set. This will help us identify the first buffer and
> +             * the remaining buffers in the stream based on length
> +             */
> +            buf->flags = header.flags & (VIRTIO_CONSOLE_HDR_START_DATA
> +                                         | VIRTIO_CONSOLE_HDR_END_DATA);
> +        } else {
> +            /* We always want to flush all the buffers in this case */
> +            buf->flags = VIRTIO_CONSOLE_HDR_START_DATA
> +                | VIRTIO_CONSOLE_HDR_END_DATA;
> +        }
> +
> +        pthread_mutex_lock(&port->buffer_list_lock);
> +        QTAILQ_INSERT_TAIL(&port->unflushed_buffer_head, buf, next);
> +        port->nr_bytes += buf->len;
> +        port->has_activity = true;
> +        pthread_mutex_unlock(&port->buffer_list_lock);
> +
> +        if (!port->host_throttled && port->byte_limit &&
> +            port->nr_bytes >= port->byte_limit) {
> +            struct virtio_console_control cpkt;
> +
> +            port->host_throttled = true;
> +            cpkt.event = VIRTIO_CONSOLE_THROTTLE_PORT;
> +            cpkt.value = true;
> +            send_control_event(port, &cpkt, sizeof(cpkt));
> +        }
> +    next_buf:
> +        virtqueue_push(vq, &elem, elem.out_sg[0].iov_len);
> +    }
> +    virtio_notify(vdev, vq);
> +    flush_all_ports(vser);
> +}
> +
> +static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +}
> +
> +static uint32_t get_features(VirtIODevice *vdev)
> +{
> +    return 1 << VIRTIO_CONSOLE_F_MULTIPORT;
> +}
> +
> +static void set_features(VirtIODevice *vdev, uint32_t features)
> +{
> +    VirtIOSerial *vser;
> +
> +    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
> +    vser->guest_features = features;
> +}
> +
> +/* Guest requested config info */
> +static void get_config(VirtIODevice *vdev, uint8_t *config_data)
> +{
> +    VirtIOSerial *vser;
> +
> +    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
> +    memcpy(config_data, &vser->config, sizeof(struct virtio_console_config));
> +}
> +
> +static void set_config(VirtIODevice *vdev, const uint8_t *config_data)
> +{
> +    struct virtio_console_config config;
> +
> +    memcpy(&config, config_data, sizeof(config));
> +}
> +
> +static void virtio_serial_save(QEMUFile *f, void *opaque)
> +{
> +    VirtIOSerial *s = opaque;
> +    VirtIOSerialPort *port;
> +    uint32_t nr_active_ports;
> +    unsigned int 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_ports);
> +
> +    /* Items in struct VirtIOSerial */
> +    qemu_put_be32s(f, &s->guest_features);
> +
> +    /* Do this because we might have hot-unplugged some ports */
> +    nr_active_ports = 0;
> +    QTAILQ_FOREACH(port, &s->ports_head, next)
> +        nr_active_ports++;
> +
> +    qemu_put_be32s(f, &nr_active_ports);
> +
> +    /*
> +     * Items in struct VirtIOSerialPort.
> +     */
> +    QTAILQ_FOREACH(port, &s->ports_head, next) {
> +        VirtIOSerialPortBuffer *buf;
> +
> +        /*
> +         * We put the port number because we may not have an active
> +         * port at id 0 that's reserved for a console port, or in case
> +         * of ports that might have gotten unplugged
> +         */
> +        qemu_put_be32s(f, &port->id);
> +        qemu_put_be64s(f, &port->byte_limit);
> +        qemu_put_be64s(f, &port->nr_bytes);
> +        qemu_put_byte(f, port->guest_connected);
> +        qemu_put_byte(f, port->host_throttled);
> +
> +        /* All the pending buffers from active ports */
> +        nr_bufs = 0;
> +        QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) {
> +            nr_bufs++;
> +        }
> +        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_be32s(f, &buf->flags);
> +            qemu_put_buffer(f, buf->buf, buf->len);
> +        }
> +    }
> +}
> +
> +static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
> +{
> +    VirtIOSerial *s = opaque;
> +    VirtIOSerialPort *port;
> +    uint32_t nr_active_ports;
> +    unsigned int i;
> +
> +    if (version_id > 2) {
> +        return -EINVAL;
> +    }
> +    /* The virtio device */
> +    virtio_load(&s->vdev, f);
> +
> +    if (version_id < 2) {
> +        return 0;
> +    }
> +    /* The config space */
> +    qemu_get_be16s(f, &s->config.cols);
> +    qemu_get_be16s(f, &s->config.rows);
> +    s->config.nr_ports = qemu_get_be32(f);
> +
> +    /* Items in struct VirtIOSerial */
> +    qemu_get_be32s(f, &s->guest_features);
> +
> +    qemu_get_be32s(f, &nr_active_ports);
> +
> +    /* Items in struct VirtIOSerialPort */
> +    for (i = 0; i < nr_active_ports; i++) {
> +        VirtIOSerialPortBuffer *buf;
> +        uint32_t id;
> +        unsigned int nr_bufs;
> +
> +        id = qemu_get_be32(f);
> +        port = find_port_by_id(s, id);
> +
> +        port->byte_limit = qemu_get_be64(f);
> +        port->nr_bytes   = qemu_get_be64(f);
> +        port->guest_connected = qemu_get_byte(f);
> +        port->host_throttled = qemu_get_byte(f);
> +
> +        /* All the pending buffers from active ports */
> +        qemu_get_be32s(f, &nr_bufs);
> +        if (!nr_bufs) {
> +            continue;
> +        }
> +        for (; nr_bufs; nr_bufs--) {
> +            buf = qemu_malloc(sizeof(*buf));
> +
> +            qemu_get_be64s(f, &buf->len);
> +            qemu_get_be32s(f, &buf->flags);
> +            buf->buf = qemu_malloc(buf->len);
> +            qemu_get_buffer(f, buf->buf, buf->len);
> +            QTAILQ_INSERT_TAIL(&port->unflushed_buffer_head, buf, next);
> +        }
> +        /*
> +         * Applications might have internal state they track when they
> +         * receive a guest_open() callback. Apps could also open a
> +         * connection to this core in that case. So if the guest was
> +         * open at the time of migration, send a guest_open callback
> +         */
> +        if (port->guest_connected && port->info->guest_open) {
> +            port->info->guest_open(port);
> +        }
> +    }
> +    return 0;
> +}
> +
> +static void virtser_bus_print(Monitor *mon, DeviceState *qdev, int indent);
> +
> +static struct BusInfo virtser_bus_info = {
> +    .name      = "virtio-serial-bus",
> +    .size      = sizeof(VirtIOSerialBus),
> +    .print_dev = virtser_bus_print,
> +    .props = (Property[]) {
> +        DEFINE_PROP_UINT32("max_nr_ports", VirtIOSerialBus, max_nr_ports, 
> 126),

This doesn't look right.  BusInfo member props defines properties common
to all devices on that bus, not properties of the bus.  But this
property refers to a member of VirtIOSerialBus, not a member of
VirtIOSerialPort, the common part of all devices on that bus.

> +        DEFINE_PROP_END_OF_LIST(),
> +    },
> +};
> +
> +static VirtIOSerialBus *virtser_bus_new(DeviceState *dev)
> +{
> +    VirtIOSerialBus *bus;
> +
> +    bus = FROM_QBUS(VirtIOSerialBus, qbus_create(&virtser_bus_info, dev, 
> NULL));
> +    bus->qbus.allow_hotplug = 1;
> +
> +    return bus;
> +}
> +
> +static void virtser_bus_print(Monitor *mon, DeviceState *qdev, int indent)

The name suggests this prints information about bus.  It prints
information about the device.  Call it virtser_bus_dev_print()?

> +{
> +    VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev);
> +    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
> +
> +    monitor_printf(mon, "%*s dev-prop-int: id: %u\n",
> +                   indent, "", port->id);
> +    monitor_printf(mon, "%*s dev-prop-int: is_console: %d\n",
> +                   indent, "", port->is_console);
> +    monitor_printf(mon, "%*s dev-prop-int: guest_connected: %d\n",
> +                   indent, "", port->guest_connected);
> +    monitor_printf(mon, "%*s dev-prop-int: host_connected: %d\n",
> +                   indent, "", port->host_connected);
> +    monitor_printf(mon, "%*s dev-prop-int: host_throttled: %d\n",
> +                   indent, "", port->host_throttled);
> +    monitor_printf(mon, "%*s dev-prop-int: nr_bytes: %zu\n",
> +                   indent, "", port->nr_bytes);
> +}
> +
> +static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
> +{
> +    VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev);
> +    VirtIOSerialPortInfo *info = DO_UPCAST(VirtIOSerialPortInfo, qdev, base);
> +    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
> +    VirtIOSerialBus *bus = DO_UPCAST(VirtIOSerialBus, qbus, 
> qdev->parent_bus);
> +    int ret;
> +
> +    port->vser = bus->vser;
> +
> +    /* FIXME! handle adding only one virtconsole port properly */

What exactly needs fixing here?

> +    if (port->vser->config.nr_ports > bus->max_nr_ports) {
> +        qemu_error("virtio-serial-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);
> +    pthread_mutex_init(&port->buffer_list_lock, NULL);
> +
> +    if (port->is_console && !find_port_by_id(port->vser, 0)) {
> +        /*
> +         * This is the first console port we're seeing so put it up at
> +         * location 0. This is done for backward compatibility (old
> +         * kernel, new qemu).
> +         */
> +        port->id = 0;
> +    } else {
> +        port->id = port->vser->config.nr_ports++;
> +    }

Aha.  Port numbers are allocated by the bus first come, first serve.
They're not stable across a reboot.  Like USB addresses, unlike PCI
addresses.

Except for port#0, which is reserved for the first console to
initialize.

> +    QTAILQ_INSERT_TAIL(&port->vser->ports_head, port, next);
> +    port->ivq = port->vser->ivqs[port->id];
> +    port->ovq = port->vser->ovqs[port->id];
> +
> +    /* Send an update to the guest about this new port added */
> +    virtio_notify_config(&port->vser->vdev);
> +
> +    return ret;
> +}
> +
> +static int virtser_port_qdev_exit(DeviceState *qdev)
> +{
> +    struct virtio_console_control cpkt;
> +    VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev);
> +    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
> +    VirtIOSerial *vser = port->vser;
> +
> +    cpkt.event = VIRTIO_CONSOLE_PORT_REMOVE;
> +    cpkt.value = 1;
> +    send_control_event(port, &cpkt, sizeof(cpkt));
> +
> +    /*
> +     * Don't decrement nr_ports here; thus we keep a linearly

You're talking about vser->config.nr_ports, aren't you?

> +     * increasing port id. Not utilising an id again saves us a couple
> +     * of complications:
> +     *
> +     * - Not having to bother about sending the port id to the guest
> +     *   kernel on hotplug or on addition of new ports; the guest can
> +     *   also linearly increment the port number. This is preferable
> +     *   because the config space won't have the need to store a
> +     *   ports_map.
> +     *
> +     * - Extra state to be stored for all the "holes" that got created
> +     *   so that we keep filling in the ids from the least available
> +     *   index.
> +     *
> +     * This places a limitation on the number of ports that can be
> +     * attached: 2^32 (as we store the port id in a u32 type). It's
> +     * very unlikely to have 2^32 ports attached to one virtio device,
> +     * however, so this shouldn't be a big problem.
> +     */

I'm confused.  Aren't port numbers limited to max_nr_ports?

> +    QTAILQ_REMOVE(&vser->ports_head, port, next);
> +
> +    if (port->info->exit)
> +        port->info->exit(dev);
> +
> +    remove_port_buffers(port);
> +    pthread_mutex_destroy(&port->buffer_list_lock);
> +
> +    return 0;
> +}
> +
> +void virtio_serial_port_qdev_register(VirtIOSerialPortInfo *info)
> +{
> +    info->qdev.init = virtser_port_qdev_init;
> +    info->qdev.bus_info = &virtser_bus_info;
> +    info->qdev.exit = virtser_port_qdev_exit;
> +    info->qdev.unplug = qdev_simple_unplug_cb;
> +    qdev_register(&info->qdev);
> +}
> +
> +VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports)
> +{
> +    VirtIOSerial *vser;
> +    VirtIODevice *vdev;
> +    uint32_t i;
> +
> +    if (!max_nr_ports)
> +        return NULL;
> +
> +    vdev = virtio_common_init("virtio-serial", VIRTIO_ID_CONSOLE,
> +                              sizeof(struct virtio_console_config),
> +                              sizeof(VirtIOSerial));
> +
> +    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
> +
> +    /* Spawn a new virtio-serial bus on which the ports will ride as devices 
> */
> +    vser->bus = virtser_bus_new(dev);
> +    vser->bus->vser = vser;
> +    QTAILQ_INIT(&vser->ports_head);
> +
> +    vser->bus->max_nr_ports = max_nr_ports;

Wait a sec!  Each device *overwrites* the bus's max_nr_ports?  That
doesn't make sense to me, please explain.

> +    vser->ivqs = qemu_malloc(max_nr_ports * sizeof(VirtQueue *));
> +    vser->ovqs = qemu_malloc(max_nr_ports * sizeof(VirtQueue *));
> +
> +    /* Add a queue for host to guest transfers for port 0 (backward compat) 
> */
> +    vser->ivqs[0] = virtio_add_queue(vdev, 128, handle_input);
> +    /* Add a queue for guest to host transfers for port 0 (backward compat) 
> */
> +    vser->ovqs[0] = virtio_add_queue(vdev, 128, handle_output);
> +
> +    /* control queue: host to guest */
> +    vser->c_ivq = virtio_add_queue(vdev, 16, control_in);
> +    /* control queue: guest to host */
> +    vser->c_ovq = virtio_add_queue(vdev, 16, control_out);
> +
> +    for (i = 1; i < vser->bus->max_nr_ports; i++) {
> +        /* Add a per-port queue for host to guest transfers */
> +        vser->ivqs[i] = virtio_add_queue(vdev, 128, handle_input);
> +        /* Add a per-per queue for guest to host transfers */
> +        vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
> +    }
> +
> +    vser->config.max_nr_ports = max_nr_ports;
> +    /*
> +     * Reserve location 0 for a console port for backward compat
> +     * (old kernel, new qemu)
> +     */
> +    vser->config.nr_ports = 1;
> +
> +    vser->vdev.get_features = get_features;
> +    vser->vdev.set_features = set_features;
> +    vser->vdev.get_config = get_config;
> +    vser->vdev.set_config = set_config;
> +
> +    /*
> +     * Register for the savevm section with the virtio-console name
> +     * to preserve backward compat
> +     */
> +    register_savevm("virtio-console", -1, 2, virtio_serial_save,
> +                    virtio_serial_load, vser);
> +
> +    return vdev;
> +}
> diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
> new file mode 100644
> index 0000000..6cdaa5b
> --- /dev/null
> +++ b/hw/virtio-serial.h
> @@ -0,0 +1,230 @@
> +/*
> + * Virtio Serial / Console Support
> + *
> + * Copyright IBM, Corp. 2008
> + * Copyright Red Hat, Inc. 2009
> + *
> + * Authors:
> + *  Christian Ehrhardt <address@hidden>
> + *  Amit Shah <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +#ifndef _QEMU_VIRTIO_SERIAL_H
> +#define _QEMU_VIRTIO_SERIAL_H
> +
> +#include <pthread.h>
> +#include <stdbool.h>
> +#include "qdev.h"
> +#include "virtio.h"
> +
> +/* == Interface shared between the guest kernel and qemu == */
> +
> +/* The Virtio ID for virtio console / serial ports */
> +#define VIRTIO_ID_CONSOLE 3
> +
> +/* Features supported */
> +#define VIRTIO_CONSOLE_F_MULTIPORT   1
> +
> +struct virtio_console_config {
> +    /*
> +     * These two fields are used by VIRTIO_CONSOLE_F_SIZE which
> +     * isn't implemented here yet
> +     */
> +    uint16_t cols;
> +    uint16_t rows;
> +
> +    uint32_t max_nr_ports;
> +    uint32_t nr_ports;
> +} __attribute__((packed));
> +
> +struct virtio_console_control {
> +    uint32_t id;             /* Port number */
> +    uint16_t event;          /* The kind of control event (see below) */
> +    uint16_t value;          /* Extra information for the key */
> +};
> +
> +struct virtio_console_header {
> +    uint32_t flags;          /* Some message between host and guest */
> +};
> +
> +/* Messages between host and guest */
> +#define VIRTIO_CONSOLE_HDR_START_DATA        (1 << 0)
> +#define VIRTIO_CONSOLE_HDR_END_DATA  (1 << 1)
> +
> +/* Some events for the internal messages (control packets) */
> +#define VIRTIO_CONSOLE_PORT_READY    0
> +#define VIRTIO_CONSOLE_CONSOLE_PORT  1
> +#define VIRTIO_CONSOLE_RESIZE                2
> +#define VIRTIO_CONSOLE_PORT_OPEN     3
> +#define VIRTIO_CONSOLE_PORT_NAME     4
> +#define VIRTIO_CONSOLE_THROTTLE_PORT 5
> +#define VIRTIO_CONSOLE_CACHE_BUFFERS 6
> +#define VIRTIO_CONSOLE_PORT_REMOVE   7
> +
> +/* == In-qemu interface == */
> +
> +typedef struct VirtIOSerial VirtIOSerial;
> +typedef struct VirtIOSerialBus VirtIOSerialBus;
> +typedef struct VirtIOSerialPort VirtIOSerialPort;
> +typedef struct VirtIOSerialPortInfo VirtIOSerialPortInfo;
> +
> +typedef struct VirtIOSerialDevice {
> +    DeviceState qdev;
> +    VirtIOSerialPortInfo *info;
> +} VirtIOSerialDevice;
> +
> +/*
> + * This is the state that's shared between all the ports.  Some of the
> + * state is configurable via command-line options. Some of it can be
> + * set by individual devices in their initfn routines. Some of the
> + * state is set by the generic qdev device init routine.
> + */
> +struct VirtIOSerialPort {
> +    DeviceState dev;
> +    VirtIOSerialPortInfo *info;
> +
> +    QTAILQ_ENTRY(VirtIOSerialPort) next;
> +
> +    /*
> +     * This field gives us the virtio device as well as the qdev bus
> +     * that we are associated with
> +     */
> +    VirtIOSerial *vser;
> +
> +    VirtQueue *ivq, *ovq;
> +
> +    /*
> +     * This 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(, VirtIOSerialPortBuffer) unflushed_buffer_head;
> +
> +    /*
> +     * This lock protects the unflushed_buffer_head list
> +     */
> +    pthread_mutex_t buffer_list_lock;
> +
> +    /*
> +     * 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;
> +
> +    /*
> +     * Each port can specify the limit on number of bytes that can be
> +     * outstanding in the unread buffers. This is to prevent any OOM
> +     * situtation if a rogue process on the guest keeps injecting
> +     * data.
> +     */
> +    size_t byte_limit;
> +
> +    /*
> +     * The number of bytes we have queued up in our unread queue
> +     */
> +    size_t nr_bytes;
> +
> +    /*
> +     * This boolean, when set, means "queue data that gets sent to
> +     * this port when the host is not connected". The queued data, if
> +     * any, is then sent out to the port when the host connection is
> +     * opened.
> +     */
> +    int cache_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;
> +    /* Have we sent a throttle message to the guest? */
> +    bool host_throttled;
> +
> +    /* Did this port get data in the recent handle_output call? */
> +    bool has_activity;
> +};
> +
> +
> +struct VirtIOSerialPortInfo {
> +    DeviceInfo qdev;
> +    /*
> +     * The per-port (or per-app) init function that's called when a
> +     * new device is found on the bus.
> +     */
> +    int (*init)(VirtIOSerialDevice *dev);
> +    /*
> +     * Per-port exit function that's called when a port gets
> +     * hot-unplugged or removed
> +     */
> +    int (*exit)(VirtIOSerialDevice *dev);
> +
> +    /* Callbacks for guest events */
> +        /*
> +      * Guest opened device. This could be invoked even when an
> +      * application thinks the guest is open. This can happen if
> +      * the host is migrated to another machine when the connection
> +      * was open and is called from the destination machine if
> +      * there's any app-specific initialisation to be done in such
> +      * a case.
> +      */
> +    void (*guest_open)(VirtIOSerialPort *port);
> +       /* Guest closed device */
> +    void (*guest_close)(VirtIOSerialPort *port);
> +
> +    /* Guest is now ready to accept data (virtqueues set up) */
> +    void (*guest_ready)(VirtIOSerialPort *port);
> +    /*
> +     * Guest wrote some data to the port. This data is handed over to
> +     * the app via this callback. The data is not maintained anymore
> +     * after the callback returns. This means the app has to ensure it
> +     * read all the data and consumes it.
> +     *
> +     * If an app needs to have the data for a longer time, it's upto
> +     * the app to cache it.
> +     */
> +    ssize_t (*have_data)(VirtIOSerialPort *port, const uint8_t *buf, size_t 
> len);
> +};
> +
> +/* Interface to the virtio-serial bus */
> +
> +/*
> + * Individual ports/apps should call this function to register the port
> + * with the virtio-serial bus
> + */
> +void virtio_serial_port_qdev_register(VirtIOSerialPortInfo *info);
> +/*
> + * Open a connection to the port
> + *   Returns 0 on success (always)
> + */
> +int virtio_serial_open(VirtIOSerialPort *port);
> +/*
> + * Close the connection to the port
> + *   Returns 0 on successful close, or, if buffer caching is disabled,
> + * -EAGAIN if there's some uncosued data for the app.

"uncosued"?  Do you mean "unconsumed"?

> + */
> +int virtio_serial_close(VirtIOSerialPort *port);
> +/*
> + * Send data to Guest
> + */
> +ssize_t virtio_serial_write(VirtIOSerialPort *port, const uint8_t *buf,
> +                            size_t size);
> +/*
> + * Query whether a guest is ready to receive data.
> + */
> +size_t virtio_serial_guest_ready(VirtIOSerialPort *port);
> +
> +#endif
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 769f540..a5d3147 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -171,7 +171,7 @@ void virtio_bind_device(VirtIODevice *vdev, const 
> VirtIOBindings *binding,
>  /* Base devices.  */
>  VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo);
>  VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf);
> -VirtIODevice *virtio_console_init(DeviceState *dev);
> +VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports);
>  VirtIODevice *virtio_balloon_init(DeviceState *dev);
>  
>  void virtio_net_exit(VirtIODevice *vdev);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index b8cc375..183b616 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1873,6 +1873,10 @@ DEF("virtioconsole", HAS_ARG, QEMU_OPTION_virtiocon, \
>  STEXI
>  @item -virtioconsole @var{c}
>  Set virtio console.
> +
> +This option is maintained for backward compatibility.
> +
> +Please use @code{-device virtconsole} for the new way of invocation.
>  ETEXI
>  
>  DEF("show-cursor", 0, QEMU_OPTION_show_cursor, \
> diff --git a/sysemu.h b/sysemu.h
> index 9d80bb2..9c3b281 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -231,12 +231,6 @@ extern CharDriverState *serial_hds[MAX_SERIAL_PORTS];
>  
>  extern CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>  
> -/* virtio consoles */
> -
> -#define MAX_VIRTIO_CONSOLES 1
> -
> -extern CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
> -
>  #define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
>  
>  #ifdef HAS_AUDIO
> diff --git a/vl.c b/vl.c
> index e606903..523022b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -173,6 +173,8 @@ int main(int argc, char **argv)
>  
>  #define DEFAULT_RAM_SIZE 128
>  
> +#define MAX_VIRTIO_CONSOLES 1
> +
>  static const char *data_dir;
>  const char *bios_name = NULL;
>  /* Note: drives_table[MAX_DRIVES] is a dummy block driver if none available
> @@ -4816,6 +4818,7 @@ static int virtcon_parse(const char *devname)
>  {
>      static int index = 0;
>      char label[32];
> +    QemuOpts *opts;
>  
>      if (strcmp(devname, "none") == 0)
>          return 0;
> @@ -4823,6 +4826,13 @@ static int virtcon_parse(const char *devname)
>          fprintf(stderr, "qemu: too many virtio consoles\n");
>          exit(1);
>      }
> +
> +    opts = qemu_opts_create(&qemu_device_opts, NULL, 0);
> +    qemu_opt_set(opts, "driver", "virtio-serial-pci");
> +    qdev_device_add(opts);
> +
> +    qemu_opt_set(opts, "driver", "virtconsole");
> +
>      snprintf(label, sizeof(label), "virtcon%d", index);
>      virtcon_hds[index] = qemu_chr_open(label, devname, NULL);
>      if (!virtcon_hds[index]) {

You reuse opts for the second device.  Is that safe?

> @@ -4830,6 +4840,9 @@ static int virtcon_parse(const char *devname)
>                  devname, strerror(errno));
>          return -1;
>      }
> +    qemu_opt_set(opts, "chardev", label);
> +    qdev_device_add(opts);
> +
>      index++;
>      return 0;
>  }
> @@ -5914,8 +5927,6 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)
>          exit(1);
> -    if (foreach_device_config(DEV_VIRTCON, virtcon_parse) < 0)
> -        exit(1);
>  
>      module_call_init(MODULE_INIT_DEVICE);
>  
> @@ -5959,6 +5970,9 @@ int main(int argc, char **argv, char **envp)
>      if (qemu_opts_foreach(&qemu_device_opts, device_init_func, NULL, 1) != 0)
>          exit(1);
>  
> +    if (foreach_device_config(DEV_VIRTCON, virtcon_parse) < 0)
> +        exit(1);
> +

Care to explain why you have to move this?

>      if (!display_state)
>          dumb_display_init();
>      /* just use the first displaystate for the moment */