[Top][All Lists]
[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: |
Thu, 24 Dec 2009 01:10:20 +0530 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
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
- [Qemu-devel] Re: [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports, (continued)
- [Qemu-devel] Re: [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports, Alexander Graf, 2009/12/22
- [Qemu-devel] Re: [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports, Amit Shah, 2009/12/22
- Re: [Qemu-devel] [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports, Anthony Liguori, 2009/12/22
- Re: [Qemu-devel] [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports, Markus Armbruster, 2009/12/23
- Re: [Qemu-devel] [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports, Markus Armbruster, 2009/12/23
- Re: [Qemu-devel] [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports, Amit Shah, 2009/12/23
- Re: [Qemu-devel] [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports, Markus Armbruster, 2009/12/23
- Re: [Qemu-devel] [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports,
Amit Shah <=
- Re: [Qemu-devel] [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports, Markus Armbruster, 2009/12/23
- Re: [Qemu-devel] [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports, Amit Shah, 2009/12/24
- Re: [Qemu-devel] [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports, Markus Armbruster, 2009/12/24