On (Wed) Dec 23 2009 [20:02:19], Markus Armbruster wrote: > Amit Shah <amit.s...@redhat.com> writes: > > > On (Wed) Dec 23 2009 [14:54:55], Markus Armbruster wrote: > >> Amit Shah <amit.s...@redhat.com> 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