Re: [Qemu-devel] [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports

[Markus Armbruster]

Amit Shah <address@hidden> writes:

> On (Wed) Dec 23 2009 [20:02:19], Markus Armbruster wrote:
>> Amit Shah <address@hidden> writes:
>> 
>> > On (Wed) Dec 23 2009 [14:54:55], Markus Armbruster wrote:
>> >> Amit Shah <address@hidden> 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.
>> 
>> Okay.
>> 
>> Do you expect devices other than "virtconsole" to go on this bus?
>
> Yes; virtserialport, as the next patch in the series introduces.
>
> Also, virtserialvnc, etc.

Since all devices on this bus need the same device address property
"name", it could make sense to define it in one place:
virtser_bus_info.props.

>> > the serial init can be changed, I guess.
>> 
>> Okay, Gerd's the authority on this.
>
>> >> 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...
>> 
>> Rename is good, but I'm sure we can come up with a name that is less
>> misleading than virtio-serial.c.  What about virtconsole.c, just like
>> the device it defines?
>
> That too would work. Or I can just send the patch with virtio-serial.c
> and once the patches are merged, rename virtio-serial.c to
> virtio-console.c and all will be OK again.

Yes.

>> >> > +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...)
>> 
>> :)
>
> BTW please keep an eye out for the way this is done in the next patch
> series; it's a bit of a hack but I could only think of doing it that
> way.

Okay, but I doubt I can do it before my Christmas vacation.

>> >> > +    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.
>> 
>> I see.
>> 
>> >> > +         * 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.)
>> 
>> The port id is used to subscript ivqs[] and ovqs[], so we need id <
>> max_nr_ports.  But the code and comment above suggest that you never
>> reuse port ids.  Don't you run out of port ids?  Am I confused?
>
> Currently that's what I'm doing. After a port is unplugged, its id is
> never re-used. I don't think people will unplug and then hotplug ports
> just because they can. I expect people to close apps and open apps and
> use the ports that are already there.

We'll see how this works out in practice.

>> >> > +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.
>> 
>> I think I got confused here.
>> 
>> We have two devices providing the virtio-serial-bus.  They call this
>> helper function to create the bus.  They copy their max_nr_ports into
>> the bus, because that's where the devices sitting on the bus can more
>> easily access it.  Correct?
>
> That's right. I wanted to avoid referencing the device's internal struct
> data to fetch the max_nr_ports value. So I let the device pass it on to
> us and I stuck it in as bus-specific data.

Fair enough.

>> >> > +    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.
>
>> >> > @@ -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?'
>> 
>> qdev_device_add() could conceivably keep a pointer to something you
>> overwrite.  That would be bad.  Not saying it does.
>
> I doubt that; if it does that, it's a bug. Because qdev_device_add()
> shouldn't expect any more calls; it shouldn't be stateful.

It made me pause.  I need to know what qdev_device_add() doesn't do to
see why this is correct.  Not as obvious as it could be.  Matter of
taste, I guess.

>> >> >      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.
>> 
>> Not sure I understand.  Do you want virtcon_parse() to run after
>> creation of the devices specified with -device?  So that you can
>> automatically supply a bus if it's still missing?  But I can't see that
>> in virtcon_parse().
>
> I just mean that qdev doesn't have any idea what device
> 'virtio-serial-pci' is before the device_init_func is called for all the
> devices that get registered. So moving virtcon_parse below that init
> ensures we can qdev_device_add a virtio-serial-pci device in
> virtcon_parse.

I see.  You need to move beyond module_call_init(MODULE_INIT_DEVICE),
don't you?  And you just move on until you hit similar device setup?
Any reason for putting it behind the generic devices?  I'm asking
because USB is before.

What happens if you have both -device virtio-serial-pci and
-virtioconsole?