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

[Amit Shah]

On (Wed) Dec 23 2009 [17:12:22], Anthony Liguori wrote:
> 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?).

I'm just trying to ensure the code is fine when qemu does get
multithreaded and fine-grained locking.

>> +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.

You mean in the no locking needed sense, or not using a mutex sense? If
the latter, what's the recommended way to do the locking?

>> +    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.

Curious case of patches getting modified for whitespace somewhere? (Or
the mail client doing it?)

I see the patch is fine in the copy I got CC'ed on; I deleted the one I
got for the qemu-devel list so I can't verify that...

http://article.gmane.org/gmane.comp.emulators.qemu/60257

shows it made it fine to the list, so guess it's your mail client doing
something funny.

> 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.

I know my current locking scheme is inadequate; but what's the
preference here? I can remove the locking for now but we'll definitely
want qemu to have more fine-grained locking, so when that happens,
revisit all the code to ensure they're safe?

>> +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.

I'll annotate and read/write using the le format.

>> +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.

It's helpful for debugging purposes, mostly: 'info qtree' on the monitor
will print this out and one can examine port state.

                Amit