qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 2/3] virtio-console: Add a virtio-serial bus, su


From: Amit Shah
Subject: Re: [Qemu-devel] [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports
Date: Wed, 23 Dec 2009 20:37:32 +0530
User-agent: Mutt/1.5.19 (2009-01-05)

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.

> 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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]