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<amit.s...@redhat.com>
>
>> 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<amit.s...@redhat.com>
>> + *
>> + * Some earlier parts are:
>> + * Copyright IBM, Corp. 2008
>> + * authored by
>> + * Christian Ehrhardt<ehrha...@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<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
