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

[Amit Shah]

On (Tue) Dec 22 2009 [19:08:40], Alexander Graf wrote:
> >  
> > -    /* Add virtio console devices */
> > -    if (pci_enabled) {
> > -        for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
> > -            if (virtcon_hds[i]) {
> > -                pci_create_simple(pci_bus, -1, "virtio-console-pci");
> > -            }
> > -        }
> > -    }
> > -
> >   
> 
> We have something pretty similar in s390-virtio.c. I suppose that needs
> to be changed too?

Yes, it'll have to be changed as well then; done in my tree.

> > -static VirtIOS390DeviceInfo s390_virtio_console = {
> > -    .init = s390_virtio_console_init,
> > -    .qdev.name = "virtio-console-s390",
> > +static VirtIOS390DeviceInfo s390_virtio_serial = {
> > +    .init = s390_virtio_serial_init,
> > +    .qdev.name = "virtio-serial-s390",
> >   
> 
> Are you sure you changed all users of the old name too?

There's only virtio-serial-pci and virtio-serial-s390. Is there
something I missed?

The new changes in vl.c, on the other hand, I've not yet fully studied
so there might be something missing there.

> > --- a/hw/virtio-console.c
> > +++ b/hw/virtio-console.c
> > @@ -1,143 +1,121 @@
> >  /*
> > - * Virtio Console Device
> > + * Virtio Console and Generic Port Devices
> >   *
> > - * Copyright IBM, Corp. 2008
> > + * Copyright Red Hat, Inc. 2009
> >   *
> >   * Authors:
> > - *  Christian Ehrhardt <address@hidden>
> > + *  Amit Shah <address@hidden>
> >   
> 
> Please don't remove copyrights.

It seems that way due to file name changes. This is actually a new file
that has nothing in common with the old one. Whatever is left of the old
content is now in virtio-serial-bus.c.

> > @@ -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");
> >   
> 
> As you stated in your comment, this breaks. Maybe something as simple as
> 
> #ifdef TARGET_S390X
> qemu_opt_set(opts, "driver", "virtio-serial-pci");
> #else
> qemu_opt_set(opts, "driver", "virtio-serial-s390");
> #endif
> 
> is enough here?

But it's ugly; Markus said we could do something better but I didn't
fully understand it. On the lines of creating default devices or setting
virtio-console's preferred bus type.

Thanks for looking at this,

                Amit