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

[Anthony Liguori]

On 12/23/2009 01:52 PM, 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-console-pci -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<address@hidden>

> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> new file mode 100644
> index 0000000..9eb60c6
> --- /dev/null
> +++ b/hw/virtio-serial-bus.c
> @@ -0,0 +1,558 @@
> +/*
> + * 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<pthread.h>

Can't include this unguarded (but it can't be needed, can it?).

> +#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;
> +
> +    /*
> +     * This lock serialises writes to the guest via the ivq
> +     */
> +    pthread_mutex_t ivq_lock;

This indicates something is wrong.  You should never need a mutex in a 
device.

> +    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;
> +
> +    pthread_mutex_lock(&port->vser->ivq_lock);
> +    while (offset<  size) {

CodingStyle seems consistently off in a curious way.  Always add spaces 
after arithmetic operators.

The pthread_mutex_lock() can't be right.  qemu already runs with a 
single global lock.  Even if you had another thread, there's no easy way 
you could safely hold this lock without grabbing the global qemu lock.

> +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 = port->id;

You probably need to do endianness conversion on this structure.

> +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 doesn't look used to me.

Regards,

Anthony Liguori