On (Wed) Dec 23 2009 [14:54:55], Markus Armbruster wrote:
> Amit Shah <amit.s...@redhat.com> 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?
The guest is supposed to only identify by name. The ID isn't guaranteed
to be the same across invocations, and there's no intention to do so.
> 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.
Thanks. Smaller patches are on their way.
> Please excuse any dumb questions regarding consoles and such; not
> exactly my area of expertise.
No question is dumb :-) Please ask more, it can only help.
> > @@ -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.
I did that in a previous submission and Gerd asked me to keep it. Even
the serial init can be changed, I guess.
> > 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.
OK.
> As others already noted, this part is hard to review, because you
> replace the file contents wholesale. Here's the resulting file:
Yes, but I'm going with Anthony's suggestion of just renaming this to
virtio-serial.c so it'll be easier to review. As you also mention,
though, it'll be weird and unintuitive, but as long as it helps
review...
> /* 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.
I had already reworked this comment to
/*
* For console ports, just assume the guest is ready to accept our
* data.
*/
Hope that's better.
> Can we have multiple console devices?
Yes!
> > +#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?
I'll put in a comment here.
> > + uint32_t max_nr_ports;
>
> Could use a comment.
OK.
> How does this play together with member max_virtserial_ports of
> VirtIOPCIProxy and VirtIOS390Device?
That value is copied into this variable.
> > +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.
The kernel header still has this name. We have a copy of the kernel
header, but if one uses the kernel headers for compiling, we'll have to
be consistent.
> > +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.
Yes, it's actually a leftover of the code I was trying. I thought I had
reverted this...
> > +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()?
Sure.
> > +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?
(I've already fixed this in my tree...)
> > + if (port->vser->config.nr_ports > bus->max_nr_ports) {
This condition should be == else we'll init an extra port and try to use
vqs that don't exist.
Now if the > is changed to ==, consider the scenario where:
-device virtio-serial-pci,max_ports=1 -device virtconsole
The bus will be initialised and port->vser->config.nr_ports is set to 1
in the init routine below.
So adding of the virtconsole port will fail, but it should succed as
we've reserved location 0 for its purpose.
> > + * 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.
Yes. With the fix for the above mentioned issue, I've moved this comment
to the start of the function so this is clearer.
> > +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?
Yes.
> > + * 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?
Er, this is also something I noticed after sending. I've reworked the
comment to match the new code.
It now reads:
* When such a functionality is desired, a control message to add
* a port can be introduced.
(Old code has just two vqs for all ports, so the number of ports could
be 2^32, because they were bounded by the uint32_t that we used to store
the port id. The new code uses a vq pair for each port, so we're bound
by the number of vqs we can spawn.)
> > +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.
Each device spawns one bus. So this is a per-device limit, or a per-bus
limit, depending on how you see it.
> > + 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;
.. This is where we reserve a location for port #0 as that always has to
be a console to preserve backward compat.
> > + * 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"?
Unused or unconsumed. But the current code doesn't return anything other
than 0. (I spotted this one as well.)
> > @@ -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?
Yes, as the value "driver" is reinitialised. Or do you mean 'are there
any side-effects like leaking memory?'
I don't think there are side-effects though I can check what it's like
in the latest version.
Also, this code is tested and it surely works fine.
> > @@ -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?
Because we now need the virtio-serial-bus registered as we auto-create
it in virtcon_parse.
Thanks a lot for the review, smaller patches coming soon.
Amit
