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?
>> 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 :)
