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 [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?

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

Okay, Gerd's the authority on this.

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

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?

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

Works for me.

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

I see.

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

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?

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

>> > +    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?'

qdev_device_add() could conceivably keep a pointer to something you
overwrite.  That would be bad.  Not saying it does.

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

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

> Thanks a lot for the review, smaller patches coming soon.

Looking forward to them :)