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

[Anthony Liguori <anthony@codemonkey.ws>]

On 01/04/2010 11:34 AM, Amit Shah wrote:
> This commit converts the virtio-console device to create a new
> virtio-serial bus that can host console and generic serial ports. The
> file hosting this code is now called virtio-serial-bus.c.
>
> The virtio console is now a very simple qdev device that sits on the
> virtio-serial-bus and communicates between the bus and qemu's chardevs.
>
> This commit also includes a few changes to the virtio backing code for
> pci and s390 to spawn the virtio-serial bus.
>
> As a result of the qdev conversion, we get rid of a lot of legacy code.
> The old-style way of instantiating a virtio console using
>
>      -virtioconsole ...
>
> is maintained, but the new, preferred way is to use
>
>      -device virtio-serial -device virtconsole,chardev=...
>
> With this commit, multiple devices as well as multiple ports with a
> single device can be supported.
>
> For multiple ports support, each port gets an IO vq pair. Since the
> guest needs to know in advance how many vqs a particular device will
> need, we have to set this number as a property of the virtio-serial
> device and also as a config option.
>
> In addition, we also spawn a pair of control IO vqs. This is an internal
> channel meant for guest-host communication for things like port
> open/close, sending port properties over to the guest, etc.
>
> This commit is a part of a series of other commits to get the full
> implementation of multiport support. Future commits will add other
> support as well as ride on the savevm version that we bump up here.
>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
> ---
>   Makefile.target        |    2 +-
>   hw/pc.c                |   11 +-
>   hw/ppc440_bamboo.c     |    7 -
>   hw/qdev.c              |    8 +-
>   hw/s390-virtio-bus.c   |   17 +-
>   hw/s390-virtio-bus.h   |    2 +
>   hw/s390-virtio.c       |    8 -
>   hw/virtio-console.c    |  143 -------------
>   hw/virtio-console.h    |   19 --
>   hw/virtio-pci.c        |   13 +-
>   hw/virtio-serial-bus.c |  554 ++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/virtio-serial.c     |  107 ++++++++++
>   hw/virtio-serial.h     |  173 +++++++++++++++
>   hw/virtio.h            |    2 +-
>   qemu-options.hx        |    4 +
>   sysemu.h               |    6 -
>   vl.c                   |   17 ++-
>   17 files changed, 879 insertions(+), 214 deletions(-)
>   delete mode 100644 hw/virtio-console.c
>   delete mode 100644 hw/virtio-console.h
>   create mode 100644 hw/virtio-serial-bus.c
>   create mode 100644 hw/virtio-serial.c
>   create mode 100644 hw/virtio-serial.h
>
> diff --git a/Makefile.target b/Makefile.target
> index 7c1f30c..d217f07 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.o virtio-serial-bus.o virtio-pci.o
>   obj-$(CONFIG_KVM) += kvm.o kvm-all.o
>   obj-$(CONFIG_ISA_MMIO) += isa_mmio.o
>   LIBS+=-lz
> diff --git a/hw/pc.c b/hw/pc.c
> index db7d58e..c5709e8 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);
>   }
>
> @@ -1308,7 +1299,7 @@ static QEMUMachine pc_machine_v0_10 = {
>               .property = "class",
>               .value    = stringify(PCI_CLASS_STORAGE_OTHER),
>           },{
> -            .driver   = "virtio-console-pci",
> +            .driver   = "virtio-serial-pci",
>    

I don't think we can eliminate the virtio-console-pci device name.  If 
someone used -writeconfig and -virtconsole in 0.12, this change would 
break their written config files.
> @@ -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.  */
>    

Minor nit, this comment is no longer needed.

> -    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-pci.c b/hw/virtio-pci.c
> index 62b46bd..ea236bb 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -92,6 +92,8 @@ typedef struct {
>       uint32_t nvectors;
>       DriveInfo *dinfo;
>       NICConf nic;
> +    /* Max. number of ports we can have for a the virtio-serial device */
> +    uint32_t max_virtserial_ports;
>   } VirtIOPCIProxy;
>
>   /* virtio device */
> @@ -481,7 +483,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 +493,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 +571,15 @@ static PCIDeviceInfo virtio_info[] = {
>           },
>           .qdev.reset = virtio_pci_reset,
>       },{
> -        .qdev.name = "virtio-console-pci",
> +        .qdev.name = "virtio-serial-pci",
> +        .qdev.alias = "virtio-serial",
>           .qdev.size = sizeof(VirtIOPCIProxy),
> -        .init      = virtio_console_init_pci,
> +        .init      = virtio_serial_init_pci,
>           .exit      = virtio_exit_pci,
>           .qdev.props = (Property[]) {
>               DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
> +            DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, max_virtserial_ports,
> +                               31),
>               DEFINE_PROP_END_OF_LIST(),
>           },
>           .qdev.reset = virtio_pci_reset,
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> new file mode 100644
> index 0000000..83bc691
> --- /dev/null
> +++ b/hw/virtio-serial-bus.c
> @@ -0,0 +1,554 @@
> +/*
> + * A bus for connecting virtio serial and console ports
> + *
> + * Copyright (C) 2009 Red Hat, Inc.
> + *
> + * Author(s):
> + *  Amit Shah<amit.shah@redhat.com>
> + *
> + * Some earlier parts are:
> + *  Copyright IBM, Corp. 2008
> + * authored by
> + *  Christian Ehrhardt<ehrhardt@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "monitor.h"
> +#include "qemu-queue.h"
> +#include "sysbus.h"
> +#include "virtio-serial.h"
> +
> +/* The virtio-serial bus on top of which the ports will ride as devices */
> +struct VirtIOSerialBus {
> +    BusState qbus;
> +
> +    /* This is the parent device that provides the bus for ports. */
> +    VirtIOSerial *vser;
> +
> +    /* The maximum number of ports that can ride on top of this bus */
> +    uint32_t max_nr_ports;
> +};
> +
> +struct VirtIOSerial {
> +    VirtIODevice vdev;
> +
> +    VirtQueue *c_ivq, *c_ovq;
> +    /* Arrays of ivqs and ovqs: one per port */
> +    VirtQueue **ivqs, **ovqs;
> +
> +    VirtIOSerialBus *bus;
> +
> +    QTAILQ_HEAD(, VirtIOSerialPort) ports;
> +    struct virtio_console_config config;
> +
> +    uint32_t guest_features;
> +};
> +
> +static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
> +{
> +    VirtIOSerialPort *port;
> +
> +    QTAILQ_FOREACH(port,&vser->ports, next) {
> +        if (port->id == id)
> +            return port;
> +    }
> +    return NULL;
> +}
> +
> +static VirtIOSerialPort *find_port_by_vq(VirtIOSerial *vser, VirtQueue *vq)
> +{
> +    VirtIOSerialPort *port;
> +
> +    QTAILQ_FOREACH(port,&vser->ports, next) {
> +        if (port->ivq == vq || port->ovq == vq)
> +            return port;
> +    }
> +    return NULL;
> +}
> +
> +static bool use_multiport(VirtIOSerial *vser)
> +{
> +    return vser->guest_features&  (1<<  VIRTIO_CONSOLE_F_MULTIPORT);
> +}
> +
> +static size_t write_to_port(VirtIOSerialPort *port,
> +                            const uint8_t *buf, size_t size)
> +{
> +    VirtQueueElement elem;
> +    struct virtio_console_header header;
> +    VirtQueue *vq;
> +    size_t offset = 0;
> +    size_t len = 0;
> +    int header_len;
> +
> +    vq = port->ivq;
> +    if (!virtio_queue_ready(vq)) {
> +        return 0;
> +    }
> +    if (!size) {
> +        return 0;
> +    }
> +    header.flags = 0;
> +    header_len = use_multiport(port->vser) ? sizeof(header) : 0;
> +
> +    while (offset<  size) {
> +        int i;
> +
> +        if (!virtqueue_pop(vq,&elem)) {
> +            break;
> +        }
> +        if (elem.in_sg[0].iov_len<  header_len) {
> +            /* We can't even store our port number in this buffer. Bug? */
> +            qemu_error("virtio-serial: size %zd less than expected\n",
> +                    elem.in_sg[0].iov_len);
> +            exit(1);
> +        }
> +        if (header_len) {
> +            memcpy(elem.in_sg[0].iov_base,&header, header_len);
> +        }
> +
> +        for (i = 0; offset<  size&&  i<  elem.in_num; i++) {
> +            /* Copy the header only in the first sg. */
> +            len = MIN(elem.in_sg[i].iov_len - header_len, size - offset);
> +
> +            memcpy(elem.in_sg[i].iov_base + header_len, buf + offset, len);
> +            offset += len;
> +            header_len = 0;
> +        }
> +        header_len = use_multiport(port->vser) ? sizeof(header) : 0;
> +        virtqueue_push(vq,&elem, len + header_len);
> +    }
> +
> +    virtio_notify(&port->vser->vdev, vq);
> +    return offset;
> +}
> +
> +static size_t send_control_msg(VirtIOSerialPort *port, void *buf, size_t len)
> +{
> +    VirtQueueElement elem;
> +    VirtQueue *vq;
> +    struct virtio_console_control *cpkt;
> +
> +    vq = port->vser->c_ivq;
> +    if (!virtio_queue_ready(vq)) {
> +        return 0;
> +    }
> +    if (!virtqueue_pop(vq,&elem)) {
> +        return 0;
> +    }
> +
> +    cpkt = (struct virtio_console_control *)buf;
> +    cpkt->id = cpu_to_le32(port->id);
>    

This is not the right way to deal with endianness.  The guest should 
write these fields in whatever their native endianness is.  From within 
qemu, we should access this fields with ldl_p/stl_p.  For x86-on-x86, 
the effect is a nop.  For TCG with le-on-be, it becomes a byte 
swap.+static void control_in(VirtIODevice *vdev, VirtQueue *vq)
> +static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
> +{
> +    VirtIOSerial *s = opaque;
> +
> +    if (version_id>  2) {
> +        return -EINVAL;
> +    }
> +    /* The virtio device */
> +    virtio_load(&s->vdev, f);
> +
> +    if (version_id<  2) {
> +        return 0;
> +    }
> +    /* The config space */
> +    qemu_get_be16s(f,&s->config.cols);
> +    qemu_get_be16s(f,&s->config.rows);
> +    s->config.nr_ports = qemu_get_be32(f);
> +
> +    /* Items in struct VirtIOSerial */
> +    qemu_get_be32s(f,&s->guest_features);
>    

Why save a copy of this when you can access it through the virtio device?

> +static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
> +{
> +    VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev);
> +    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev,&dev->qdev);
> +
> +    monitor_printf(mon, "%*s dev-prop-int: id: %u\n",
> +                   indent, "", port->id);
> +    monitor_printf(mon, "%*s dev-prop-int: is_console: %d\n",
> +                   indent, "", port->is_console);
> +}
>    

This will break the build since it's not referenced anywhere.

Regards,

Anthony Liguori