Re: [Qemu-devel] [PATCH 2/3] virtio-console: Add a virtio-serial bus, support for multiple devices and ports

[Amit Shah]

On (Wed) Dec 23 2009 [23:07:24], Markus Armbruster wrote:
> >> 
> >> 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.
> 
> Since all devices on this bus need the same device address property
> "name", it could make sense to define it in one place:
> virtser_bus_info.props.

Hm, let me see how that works.

> >> >> 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.
> 
> We'll see how this works out in practice.

True.

> >> >> > @@ -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.
> 
> It made me pause.  I need to know what qdev_device_add() doesn't do to
> see why this is correct.  Not as obvious as it could be.  Matter of
> taste, I guess.

I'll look up the code and do the needful if necessary.

> >> >> >      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.
> 
> I see.  You need to move beyond module_call_init(MODULE_INIT_DEVICE),
> don't you?  And you just move on until you hit similar device setup?
> Any reason for putting it behind the generic devices?  I'm asking
> because USB is before.

Yes, that was the intent but I shuffled it back to the place where it
was and it's working fine. I had this change before the virtcon_parse
function was introduced and while rebasing I moved it around as well..

> What happens if you have both -device virtio-serial-pci and
> -virtioconsole?

Yeah; that's not recommended. But what would happen is two buses would
get spawned and the console from -virtioconsole would get on one of
them (bus #0).

                Amit