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
