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

[Alexander Graf]

Amit Shah wrote:
> 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.
>
> This interface enables spawning of multiple virtio consoles as well as generic
> serial ports.
>
> The older -virtconsole argument still works, but when using the new
> functionality, it is recommended to use
>
>     -device virtio-serial-pci -device virtconsole,...
>
> The virtconsole device type accepts a chardev as an argument and a 'name'
> argument to identify the corresponding consoles on the host as well as the
> guest. The name, if given, is exposed via the 'name' sysfs attribute in the
> guest.
>
> Care has been taken to ensure compatibility with kernels that do not
> support multiple ports as well as accepting incoming migrations from older
> qemu versions.
>
> Signed-off-by: Amit Shah <address@hidden>
> ---
>  Makefile.target        |    2 +-
>  hw/pc.c                |    9 -
>  hw/ppc440_bamboo.c     |    7 -
>  hw/qdev.c              |    8 +-
>  hw/s390-virtio-bus.c   |   16 +-
>  hw/s390-virtio-bus.h   |    1 +
>  hw/virtio-console.c    |  186 ++++------
>  hw/virtio-console.h    |   19 -
>  hw/virtio-pci.c        |   11 +-
>  hw/virtio-serial-bus.c |  964 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/virtio-serial.h     |  230 ++++++++++++
>  hw/virtio.h            |    2 +-
>  qemu-options.hx        |    4 +
>  sysemu.h               |    6 -
>  vl.c                   |   18 +-
>  15 files changed, 1317 insertions(+), 166 deletions(-)
>  delete mode 100644 hw/virtio-console.h
>  create mode 100644 hw/virtio-serial-bus.c
>  create mode 100644 hw/virtio-serial.h
>
> diff --git a/Makefile.target b/Makefile.target
> index 7c1f30c..74bb548 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -156,7 +156,7 @@ ifdef CONFIG_SOFTMMU
>  obj-y = vl.o async.o monitor.o pci.o pci_host.o pcie_host.o machine.o 
> gdbstub.o
>  # virtio has to be here due to weird dependency between PCI and virtio-net.
>  # need to fix this properly
> -obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o 
> virtio-pci.o
> +obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-serial-bus.o 
> virtio-console.o virtio-pci.o
>  obj-$(CONFIG_KVM) += kvm.o kvm-all.o
>  obj-$(CONFIG_ISA_MMIO) += isa_mmio.o
>  LIBS+=-lz
> diff --git a/hw/pc.c b/hw/pc.c
> index db7d58e..5e742bf 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1242,15 +1242,6 @@ static void pc_init1(ram_addr_t ram_size,
>          }
>      }
>  
> -    /* 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?

>      rom_load_fw(fw_cfg);
>  }
>  
> diff --git a/hw/ppc440_bamboo.c b/hw/ppc440_bamboo.c
> index a488240..1ab9872 100644
> --- a/hw/ppc440_bamboo.c
> +++ b/hw/ppc440_bamboo.c
> @@ -108,13 +108,6 @@ static void bamboo_init(ram_addr_t ram_size,
>      env = ppc440ep_init(&ram_size, &pcibus, pci_irq_nrs, 1, cpu_model);
>  
>      if (pcibus) {
> -        /* Add virtio console devices */
> -        for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
> -            if (virtcon_hds[i]) {
> -                pci_create_simple(pcibus, -1, "virtio-console-pci");
> -            }
> -        }
> -
>          /* Register network interfaces. */
>          for (i = 0; i < nb_nics; i++) {
>              /* There are no PCI NICs on the Bamboo board, but there are
> diff --git a/hw/qdev.c b/hw/qdev.c
> index b6bd4ae..38c6e15 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -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++];
>  }
>  
>  BusState *qdev_get_parent_bus(DeviceState *dev)
> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
> index dc154ed..79ba9fc 100644
> --- a/hw/s390-virtio-bus.c
> +++ b/hw/s390-virtio-bus.c
> @@ -26,7 +26,7 @@
>  #include "loader.h"
>  #include "elf.h"
>  #include "hw/virtio.h"
> -#include "hw/virtio-console.h"
> +#include "hw/virtio-serial.h"
>  #include "hw/sysbus.h"
>  #include "kvm.h"
>  
> @@ -130,7 +130,7 @@ static int s390_virtio_blk_init(VirtIOS390Device *dev)
>      return s390_virtio_device_init(dev, vdev);
>  }
>  
> -static int s390_virtio_console_init(VirtIOS390Device *dev)
> +static int s390_virtio_serial_init(VirtIOS390Device *dev)
>  {
>      VirtIOS390Bus *bus;
>      VirtIODevice *vdev;
> @@ -138,7 +138,7 @@ static int s390_virtio_console_init(VirtIOS390Device *dev)
>  
>      bus = DO_UPCAST(VirtIOS390Bus, bus, dev->qdev.parent_bus);
>  
> -    vdev = virtio_console_init((DeviceState *)dev);
> +    vdev = virtio_serial_init((DeviceState *)dev, dev->max_virtserial_ports);
>      if (!vdev) {
>          return -1;
>      }
> @@ -336,11 +336,13 @@ static VirtIOS390DeviceInfo s390_virtio_blk = {
>      },
>  };
>  
> -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?

>      .qdev.size = sizeof(VirtIOS390Device),
>      .qdev.props = (Property[]) {
> +        DEFINE_PROP_UINT32("max_ports", VirtIOS390Device, 
> max_virtserial_ports,
> +                           15),
>          DEFINE_PROP_END_OF_LIST(),
>      },
>  };
> @@ -364,7 +366,7 @@ static void 
> s390_virtio_bus_register_withprop(VirtIOS390DeviceInfo *info)
>  
>  static void s390_virtio_register(void)
>  {
> -    s390_virtio_bus_register_withprop(&s390_virtio_console);
> +    s390_virtio_bus_register_withprop(&s390_virtio_serial);
>      s390_virtio_bus_register_withprop(&s390_virtio_blk);
>      s390_virtio_bus_register_withprop(&s390_virtio_net);
>  }
> 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;
>  } VirtIOS390Device;
>  
>  typedef struct VirtIOS390Bus {
> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> index 57f8f89..b2e4eb1 100644
> --- 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.

> @@ -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?

Alex