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

[Amit Shah]

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.

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

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

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

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

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

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

                Amit