| Message ID | 1261504146-26018-3-git-send-email-amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <amit.shah@redhat.com> > Please split this patch. I got dizzy after only reading 1/4th of it :-). Alex
On (Tue) Dec 22 2009 [18:55:16], Alexander Graf wrote: > 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 <amit.shah@redhat.com> > > > > Please split this patch. I got dizzy after only reading 1/4th of it :-). Ah, sooner the better I guess. I'll do that soon. The interesting bits for you are in hw/s390-* and vl.c though. Amit
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 <amit.shah@redhat.com> > --- > 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 <ehrhardt@linux.vnet.ibm.com> > + * Amit Shah <amit.shah@redhat.com> > 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
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 <ehrhardt@linux.vnet.ibm.com> > > + * Amit Shah <amit.shah@redhat.com> > > > > 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
On 12/22/2009 11:49 AM, 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<amit.shah@redhat.com> This is really difficult to review. If you're introducing a completely new file, why don't you just call it virtio-serial.c and delete virtio-console.c It will save quite a bit of reviewing head ache. Regards, Anthony Liguori
Anthony Liguori <anthony@codemonkey.ws> writes: > On 12/22/2009 11:49 AM, 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<amit.shah@redhat.com> > > This is really difficult to review. If you're introducing a > completely new file, why don't you just call it virtio-serial.c and > delete virtio-console.c It will save quite a bit of reviewing head > ache. The patch series defines virtio-serial devices, one for PCI (virtio-serial-pci, in virtio-pci.c) and one for s390 (virtio-serial-s390, in s390-virtio-bus.c). The new file defines a virtconsole device, and calling it virtio-serial would be confusing, wouldn't it?
Amit Shah <amit.shah@redhat.com> writes: > On (Tue) Dec 22 2009 [18:55:16], Alexander Graf wrote: >> 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 <amit.shah@redhat.com> >> > >> >> Please split this patch. I got dizzy after only reading 1/4th of it :-). > > Ah, sooner the better I guess. I'll do that soon. [...] Yes, please!
Amit Shah <amit.shah@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? > 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 <amit.shah@redhat.com> > --- > 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 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. Please excuse any dumb questions regarding consoles and such; not exactly my area of expertise. > 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"); > - } > - } > - } > - > 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++]; > } I believe the FIXME is about the nasty special case for "virtio". Since you fix that, better remove the FIXME. > > 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", > .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; Could use a comment. > } 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 [...] As others already noted, this part is hard to review, because you replace the file contents wholesale. Here's the resulting file: /* * Virtio Console and Generic Port Devices * * Copyright Red Hat, Inc. 2009 * * Authors: * Amit Shah <amit.shah@redhat.com> * * This work is licensed under the terms of the GNU GPL, version 2. See * the COPYING file in the top-level directory. */ #include "qemu-char.h" #include "virtio-serial.h" typedef struct VirtConsole { VirtIOSerialPort port; CharDriverState *chr; } VirtConsole; /* Callback function that's called when the guest sends us data */ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len) { VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); return qemu_chr_write(vcon->chr, buf, len); } /* Readiness of the guest to accept data on a port */ static int chr_can_read(void *opaque) { VirtConsole *vcon = opaque; return virtio_serial_guest_ready(&vcon->port); } /* Send data from a char device over to the guest */ static void chr_read(void *opaque, const uint8_t *buf, int size) { VirtConsole *vcon = opaque; virtio_serial_write(&vcon->port, buf, size); } static void chr_event(void *opaque, int event) { VirtConsole *vcon = opaque; switch (event) { case CHR_EVENT_OPENED: { virtio_serial_open(&vcon->port); break; } case CHR_EVENT_CLOSED: virtio_serial_close(&vcon->port); break; } } /* 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. Can we have multiple console devices? if (vcon->chr) { qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event, vcon); } return 0; } static int vcon_exitfn(VirtIOSerialDevice *dev) { VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev); VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); if (vcon->chr) qemu_chr_close(vcon->chr); return 0; } static VirtIOSerialPortInfo virtcon_info = { .qdev.name = "virtconsole", .qdev.size = sizeof(VirtConsole), .init = vcon_initfn, .exit = vcon_exitfn, .have_data = flush_buf, .qdev.props = (Property[]) { DEFINE_PROP_CHR("chardev", VirtConsole, chr), DEFINE_PROP_STRING("name", VirtConsole, port.name), DEFINE_PROP_END_OF_LIST(), }, }; static void virtcon_register(void) { virtio_serial_port_qdev_register(&virtcon_info); } device_init(virtcon_register) > diff --git a/hw/virtio-console.h b/hw/virtio-console.h > deleted file mode 100644 > index 84d0717..0000000 > --- a/hw/virtio-console.h > +++ /dev/null > @@ -1,19 +0,0 @@ > -/* > - * Virtio Console Support > - * > - * Copyright IBM, Corp. 2008 > - * > - * Authors: > - * Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com> > - * > - * This work is licensed under the terms of the GNU GPL, version 2. See > - * the COPYING file in the top-level directory. > - * > - */ > -#ifndef _QEMU_VIRTIO_CONSOLE_H > -#define _QEMU_VIRTIO_CONSOLE_H > - > -/* The ID for virtio console */ > -#define VIRTIO_ID_CONSOLE 3 > - > -#endif > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index 62b46bd..1baca0d 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -92,6 +92,7 @@ typedef struct { > uint32_t nvectors; > DriveInfo *dinfo; > NICConf nic; > + uint32_t max_virtserial_ports; > } VirtIOPCIProxy; > > /* virtio device */ > @@ -481,7 +482,7 @@ static int virtio_blk_exit_pci(PCIDevice *pci_dev) > return virtio_exit_pci(pci_dev); > } > > -static int virtio_console_init_pci(PCIDevice *pci_dev) > +static int virtio_serial_init_pci(PCIDevice *pci_dev) > { > VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > VirtIODevice *vdev; > @@ -491,7 +492,7 @@ static int virtio_console_init_pci(PCIDevice *pci_dev) > proxy->class_code != PCI_CLASS_OTHERS) /* qemu-kvm */ > proxy->class_code = PCI_CLASS_COMMUNICATION_OTHER; > > - vdev = virtio_console_init(&pci_dev->qdev); > + vdev = virtio_serial_init(&pci_dev->qdev, proxy->max_virtserial_ports); > if (!vdev) { > return -1; > } > @@ -569,12 +570,14 @@ static PCIDeviceInfo virtio_info[] = { > }, > .qdev.reset = virtio_pci_reset, > },{ > - .qdev.name = "virtio-console-pci", > + .qdev.name = "virtio-serial-pci", > .qdev.size = sizeof(VirtIOPCIProxy), > - .init = virtio_console_init_pci, > + .init = virtio_serial_init_pci, > .exit = virtio_exit_pci, > .qdev.props = (Property[]) { > DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0), > + DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, max_virtserial_ports, > + 15), > DEFINE_PROP_END_OF_LIST(), > }, > .qdev.reset = virtio_pci_reset, > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c > new file mode 100644 > index 0000000..fd0ea12 > --- /dev/null > +++ b/hw/virtio-serial-bus.c > @@ -0,0 +1,964 @@ > +/* > + * A bus for connecting virtio serial and console ports > + * > + * Copyright (C) 2009 Red Hat, Inc. > + * > + * Author(s): > + * Amit Shah <amit.shah@redhat.com> > + * > + * Some earlier parts are: > + * Copyright IBM, Corp. 2008 > + * authored by > + * Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2. See > + * the COPYING file in the top-level directory. > + */ > + > +#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? > + uint32_t max_nr_ports; Could use a comment. How does this play together with member max_virtserial_ports of VirtIOPCIProxy and VirtIOS390Device? > +}; > + > +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. > + > + /* > + * This lock serialises writes to the guest via the ivq > + */ > + pthread_mutex_t ivq_lock; > + > + uint32_t guest_features; > +}; > + > +/* This struct holds individual buffers received for each port */ > +typedef struct VirtIOSerialPortBuffer { > + QTAILQ_ENTRY(VirtIOSerialPortBuffer) next; > + > + uint8_t *buf; > + > + size_t len; /* length of the buffer */ > + size_t offset; /* offset from which to consume data in the buffer */ > + > + uint32_t flags; /* Sent by guest (start of data stream, end of stream) */ > + > + bool previous_failure; /* Did sending out this buffer fail previously? */ > +} VirtIOSerialPortBuffer; > + > +static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id) > +{ > + VirtIOSerialPort *port; > + > + QTAILQ_FOREACH(port, &vser->ports_head, next) { > + if (port->id == id) > + return port; > + } > + return NULL; > +} > + > +static VirtIOSerialPort *find_port_by_vq(VirtIOSerial *vser, VirtQueue *vq) > +{ > + VirtIOSerialPort *port; > + > + QTAILQ_FOREACH(port, &vser->ports_head, next) { > + if (port->ivq == vq || port->ovq == vq) > + return port; > + } > + return NULL; > +} > + > +static bool use_multiport(VirtIOSerial *vser) > +{ > + return vser->guest_features & (1 << VIRTIO_CONSOLE_F_MULTIPORT); > +} > + > +static size_t write_to_port(VirtIOSerialPort *port, > + const uint8_t *buf, size_t size) > +{ > + VirtQueueElement elem; > + struct virtio_console_header header; > + VirtQueue *vq; > + size_t offset = 0; > + size_t len = 0; > + int header_len; > + > + vq = port->ivq; > + if (!virtio_queue_ready(vq)) { > + return 0; > + } > + if (!size) { > + return 0; > + } > + header.flags = 0; > + header_len = use_multiport(port->vser) ? sizeof(header) : 0; > + > + pthread_mutex_lock(&port->vser->ivq_lock); > + while (offset < size) { > + int i; > + > + if (!virtqueue_pop(vq, &elem)) { > + break; > + } > + if (elem.in_sg[0].iov_len < header_len) { > + /* We can't even store our port number in this buffer. Bug? */ > + qemu_error("virtio-serial: size %zd less than expected\n", > + elem.in_sg[0].iov_len); > + exit(1); > + } > + if (header_len) { > + memcpy(elem.in_sg[0].iov_base, &header, header_len); > + } > + > + for (i = 0; offset < size && i < elem.in_num; i++) { > + /* Copy the header only in the first sg. */ > + len = MIN(elem.in_sg[i].iov_len - header_len, size - offset); > + > + memcpy(elem.in_sg[i].iov_base + header_len, buf + offset, len); > + offset += len; > + header_len = 0; > + } > + header_len = use_multiport(port->vser) ? sizeof(header) : 0; > + virtqueue_push(vq, &elem, len + header_len); > + } > + > + virtio_notify(&port->vser->vdev, vq); > + pthread_mutex_unlock(&port->vser->ivq_lock); > + return offset; > +} > + > +static size_t send_control_event(VirtIOSerialPort *port, void *buf, size_t len) > +{ > + VirtQueueElement elem; > + VirtQueue *vq; > + struct virtio_console_control *cpkt; > + > + vq = port->vser->c_ivq; > + if (!virtio_queue_ready(vq)) { > + return 0; > + } > + if (!virtqueue_pop(vq, &elem)) { > + return 0; > + } > + > + cpkt = (struct virtio_console_control *)buf; > + cpkt->id = port->id; > + memcpy(elem.in_sg[0].iov_base, buf, len); > + > + virtqueue_push(vq, &elem, len); > + virtio_notify(&port->vser->vdev, vq); > + return len; > +} > + > +static size_t get_complete_data_size(VirtIOSerialPort *port) > +{ > + VirtIOSerialPortBuffer *buf; > + size_t size; > + bool is_complete, start_seen; > + > + size = 0; > + is_complete = false; > + start_seen = false; > + QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) { > + size += buf->len - buf->offset; > + > + if (buf->flags & VIRTIO_CONSOLE_HDR_END_DATA) { > + is_complete = true; > + break; > + } > + if (buf == QTAILQ_FIRST(&port->unflushed_buffer_head) > + && !(buf->flags & VIRTIO_CONSOLE_HDR_START_DATA)) { > + > + /* There's some data that arrived without a START flag. Flush it. */ > + is_complete = true; > + break; > + } > + > + if (buf->flags & VIRTIO_CONSOLE_HDR_START_DATA) { > + if (start_seen) { > + /* > + * There's some data that arrived without an END > + * flag. Flush it. > + */ > + size -= buf->len + buf->offset; > + is_complete = true; > + break; > + } > + start_seen = true; > + } > + } > + return is_complete ? size : 0; > +} > + > +/* > + * The guest could have sent the data corresponding to one write > + * request split up in multiple buffers. The first buffer has the > + * VIRTIO_CONSOLE_HDR_START_DATA flag set and the last buffer has the > + * VIRTIO_CONSOLE_HDR_END_DATA flag set. Using this information, merge > + * the parts into one buf here to process it for output. > + */ > +static VirtIOSerialPortBuffer *get_complete_buf(VirtIOSerialPort *port) > +{ > + VirtIOSerialPortBuffer *buf, *buf2; > + uint8_t *outbuf; > + size_t out_offset, out_size; > + > + out_size = get_complete_data_size(port); > + if (!out_size) > + return NULL; > + > + buf = QTAILQ_FIRST(&port->unflushed_buffer_head); > + if (buf->len - buf->offset == out_size) { > + QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next); > + return buf; > + } > + out_offset = 0; > + outbuf = qemu_malloc(out_size); > + > + QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffer_head, next, buf2) { > + size_t copy_size; > + > + copy_size = buf->len - buf->offset; > + memcpy(outbuf + out_offset, buf->buf + buf->offset, copy_size); > + out_offset += copy_size; > + > + QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next); > + qemu_free(buf->buf); > + > + if (out_offset == out_size) { > + break; > + } > + qemu_free(buf); > + } > + buf->buf = outbuf; > + buf->len = out_size; > + buf->offset = 0; > + buf->flags = VIRTIO_CONSOLE_HDR_START_DATA | VIRTIO_CONSOLE_HDR_END_DATA; > + buf->previous_failure = false; > + > + return buf; > +} > + > +/* Call with the buffer_list_lock held */ > +static void flush_queue(VirtIOSerialPort *port) > +{ > + VirtIOSerialPortBuffer *buf; > + size_t out_size; > + ssize_t ret; > + > + /* > + * If a device is interested in buffering packets till it's > + * opened, cache the data the guest sends us till a connection is > + * established. > + */ > + if (!port->host_connected && port->cache_buffers) { > + return; > + } > + while ((buf = get_complete_buf(port))) { > + out_size = buf->len - buf->offset; > + if (!port->host_connected) { > + /* > + * Caching is disabled and host is not connected, so > + * discard the buffer. Do this only after merging the > + * buffer as a port can get connected in the middle of > + * dropping buffers and the port will end up getting the > + * incomplete output. > + */ > + port->nr_bytes -= buf->len + buf->offset; > + qemu_free(buf->buf); > + qemu_free(buf); > + continue; > + } > + > + ret = port->info->have_data(port, buf->buf + buf->offset, out_size); > + if (ret < out_size) { > + QTAILQ_INSERT_HEAD(&port->unflushed_buffer_head, buf, next); > + } > + if (ret <= 0) { > + /* We're not progressing at all */ > + if (buf->previous_failure) { > + break; > + } > + buf->previous_failure = true; > + } else { > + buf->offset += ret; > + port->nr_bytes -= ret; > + buf->previous_failure = false; > + } > + if (!(buf->len - buf->offset)) { > + qemu_free(buf->buf); > + qemu_free(buf); > + } > + } > + > + if (port->host_throttled && port->nr_bytes < port->byte_limit) { > + struct virtio_console_control cpkt; > + > + port->host_throttled = false; > + cpkt.event = VIRTIO_CONSOLE_THROTTLE_PORT; > + cpkt.value = false; > + send_control_event(port, &cpkt, sizeof(cpkt)); > + } > +} > + > +static void flush_all_ports(VirtIOSerial *vser) > +{ > + struct VirtIOSerialPort *port; > + > + QTAILQ_FOREACH(port, &vser->ports_head, next) { > + pthread_mutex_lock(&port->buffer_list_lock); > + if (port->has_activity) { > + port->has_activity = false; > + flush_queue(port); > + } > + pthread_mutex_unlock(&port->buffer_list_lock); > + } > +} > + > +static void remove_port_buffers(VirtIOSerialPort *port) > +{ > + struct VirtIOSerialPortBuffer *buf, *buf2; > + > + pthread_mutex_lock(&port->buffer_list_lock); > + QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffer_head, next, buf2) { > + QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next); > + qemu_free(buf->buf); > + qemu_free(buf); > + } > + pthread_mutex_unlock(&port->buffer_list_lock); > +} > + > +/* Functions for use inside qemu to open and read from/write to ports */ > +int virtio_serial_open(VirtIOSerialPort *port) > +{ > + struct virtio_console_control cpkt; > + > + /* Don't allow opening an already-open port */ > + if (port->host_connected) { > + return 0; > + } > + /* Send port open notification to the guest */ > + port->host_connected = true; > + cpkt.event = VIRTIO_CONSOLE_PORT_OPEN; > + cpkt.value = 1; > + send_control_event(port, &cpkt, sizeof(cpkt)); > + > + /* Flush any buffers that were cached while the port was closed */ > + if (port->cache_buffers && port->info->have_data) { > + pthread_mutex_lock(&port->buffer_list_lock); > + flush_queue(port); > + pthread_mutex_unlock(&port->buffer_list_lock); > + } > + return 0; > +} > + > +int virtio_serial_close(VirtIOSerialPort *port) > +{ > + struct virtio_console_control cpkt; > + > + port->host_connected = false; > + cpkt.event = VIRTIO_CONSOLE_PORT_OPEN; > + cpkt.value = 0; > + send_control_event(port, &cpkt, sizeof(cpkt)); > + > + if (!port->cache_buffers) { > + remove_port_buffers(port); > + } > + return 0; > +} > + > +/* Individual ports/apps call this function to write to the guest. */ > +ssize_t virtio_serial_write(VirtIOSerialPort *port, const uint8_t *buf, > + size_t size) > +{ > + if (!port || !port->host_connected || !port->guest_connected) { > + return 0; > + } > + return write_to_port(port, buf, size); > +} > + > +/* > + * Readiness of the guest to accept data on a port. > + * Returns max. data the guest can receive > + */ > +size_t virtio_serial_guest_ready(VirtIOSerialPort *port) > +{ > + VirtQueue *vq = port->ivq; > + size_t size, header_len; > + > + if (use_multiport(port->vser)) { > + header_len = sizeof(struct virtio_console_header); > + } else { > + header_len = 0; > + } > + if (!virtio_queue_ready(vq) || > + !(port->vser->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) || > + virtio_queue_empty(vq)) { > + return 0; > + } > + if (use_multiport(port->vser) && !port->guest_connected) { > + return 0; > + } > + > + size = TARGET_PAGE_SIZE; > + if (virtqueue_avail_bytes(vq, size, 0)) { > + return size - header_len; > + } > + size = header_len + 1; > + if (virtqueue_avail_bytes(vq, size, 0)) { > + return size - header_len; > + } > + return 0; > +} > + > +/* Guest wants to notify us of some event */ > +static void handle_control_message(VirtIOSerial *vser, void *buf) > +{ > + struct VirtIOSerialPort *port; > + struct virtio_console_control *cpkt; > + uint8_t *buffer; > + size_t buffer_len; > + > + cpkt = buf; > + port = find_port_by_id(vser, cpkt->id); > + if (!port) > + return; > + > + switch(cpkt->event) { > + case VIRTIO_CONSOLE_PORT_READY: > + /* > + * Now that we know the guest asked for the port name, we're > + * sure the guest has initialised whatever state is necessary > + * for this port. Now's a good time to let the guest know if > + * this port is a console port so that the guest can hook it > + * up to hvc. > + */ > + if (port->is_console) { > + cpkt->event = VIRTIO_CONSOLE_CONSOLE_PORT; > + cpkt->value = 1; > + send_control_event(port, cpkt, sizeof(cpkt)); > + } > + if (port->name) { > + cpkt->event = VIRTIO_CONSOLE_PORT_NAME; > + cpkt->value = 1; > + > + buffer_len = sizeof(*cpkt) + strlen(port->name) + 1; > + buffer = qemu_malloc(buffer_len); > + > + memcpy(buffer, cpkt, sizeof(*cpkt)); > + memcpy(buffer + sizeof(*cpkt), port->name, strlen(port->name)); > + buffer[buffer_len - 1] = 0; > + > + send_control_event(port, buffer, buffer_len); > + qemu_free(buffer); > + } > + /* > + * We also want to signal to the guest whether or not the port > + * is set to caching the buffers when disconnected. > + */ > + if (port->cache_buffers) { > + cpkt->event = VIRTIO_CONSOLE_CACHE_BUFFERS; > + cpkt->value = 1; > + send_control_event(port, cpkt, sizeof(cpkt)); > + } > + if (port->host_connected) { > + cpkt->event = VIRTIO_CONSOLE_PORT_OPEN; > + cpkt->value = 1; > + send_control_event(port, cpkt, sizeof(cpkt)); > + } > + /* > + * When the guest has asked us for this information it means > + * the guest is all setup and has its virtqueues > + * initialised. If some app is interested in knowing about > + * this event, let it know > + */ > + if (port->info->guest_ready) { > + port->info->guest_ready(port); > + } > + break; > + case VIRTIO_CONSOLE_PORT_OPEN: > + port->guest_connected = cpkt->value; > + if (cpkt->value && port->info->guest_open) { > + /* Send the guest opened notification if an app is interested */ > + port->info->guest_open(port); > + } > + if (!cpkt->value && port->info->guest_close) { > + /* Send the guest closed notification if an app is interested */ > + port->info->guest_close(port); > + } > + break; > + } > +} > + > +static void control_in(VirtIODevice *vdev, VirtQueue *vq) > +{ > +} > + > +static void control_out(VirtIODevice *vdev, VirtQueue *vq) > +{ > + VirtQueueElement elem; > + VirtIOSerial *vser; > + > + vser = DO_UPCAST(VirtIOSerial, vdev, vdev); > + > + while (virtqueue_pop(vq, &elem)) { > + handle_control_message(vser, elem.out_sg[0].iov_base); > + virtqueue_push(vq, &elem, elem.out_sg[0].iov_len); > + } > + virtio_notify(vdev, vq); > +} > + > +/* > + * Guest wrote something to some port. > + * > + * Flush the data in the entire chunk that we received rather than > + * splitting it into multiple buffers. VNC clients don't consume split > + * buffers > + */ > +static void handle_output(VirtIODevice *vdev, VirtQueue *vq) > +{ > + VirtIOSerial *vser; > + VirtQueueElement elem; > + > + vser = DO_UPCAST(VirtIOSerial, vdev, vdev); > + > + while (virtqueue_pop(vq, &elem)) { > + VirtIOSerialPort *port; > + VirtIOSerialPortBuffer *buf; > + struct virtio_console_header header; > + int header_len; > + > + header_len = use_multiport(vser) ? sizeof(header) : 0; > + > + if (elem.out_sg[0].iov_len < header_len) { > + goto next_buf; > + } > + if (header_len) { > + memcpy(&header, elem.out_sg[0].iov_base, header_len); > + } > + port = find_port_by_vq(vser, vq); > + if (!port) { > + goto next_buf; > + } > + /* > + * A port may not have any handler registered for consuming the > + * data that the guest sends or it may not have a chardev associated > + * with it. Just ignore the data in that case. > + */ > + if (!port->info->have_data) { > + goto next_buf; > + } > + > + /* The guest always sends only one sg */ > + buf = qemu_mallocz(sizeof(*buf)); > + buf->len = elem.out_sg[0].iov_len - header_len; > + buf->buf = qemu_malloc(buf->len); > + memcpy(buf->buf, elem.out_sg[0].iov_base + header_len, buf->len); > + > + if (header_len) { > + /* > + * Only the first buffer in a stream will have this > + * set. This will help us identify the first buffer and > + * the remaining buffers in the stream based on length > + */ > + buf->flags = header.flags & (VIRTIO_CONSOLE_HDR_START_DATA > + | VIRTIO_CONSOLE_HDR_END_DATA); > + } else { > + /* We always want to flush all the buffers in this case */ > + buf->flags = VIRTIO_CONSOLE_HDR_START_DATA > + | VIRTIO_CONSOLE_HDR_END_DATA; > + } > + > + pthread_mutex_lock(&port->buffer_list_lock); > + QTAILQ_INSERT_TAIL(&port->unflushed_buffer_head, buf, next); > + port->nr_bytes += buf->len; > + port->has_activity = true; > + pthread_mutex_unlock(&port->buffer_list_lock); > + > + if (!port->host_throttled && port->byte_limit && > + port->nr_bytes >= port->byte_limit) { > + struct virtio_console_control cpkt; > + > + port->host_throttled = true; > + cpkt.event = VIRTIO_CONSOLE_THROTTLE_PORT; > + cpkt.value = true; > + send_control_event(port, &cpkt, sizeof(cpkt)); > + } > + next_buf: > + virtqueue_push(vq, &elem, elem.out_sg[0].iov_len); > + } > + virtio_notify(vdev, vq); > + flush_all_ports(vser); > +} > + > +static void handle_input(VirtIODevice *vdev, VirtQueue *vq) > +{ > +} > + > +static uint32_t get_features(VirtIODevice *vdev) > +{ > + return 1 << VIRTIO_CONSOLE_F_MULTIPORT; > +} > + > +static void set_features(VirtIODevice *vdev, uint32_t features) > +{ > + VirtIOSerial *vser; > + > + vser = DO_UPCAST(VirtIOSerial, vdev, vdev); > + vser->guest_features = features; > +} > + > +/* Guest requested config info */ > +static void get_config(VirtIODevice *vdev, uint8_t *config_data) > +{ > + VirtIOSerial *vser; > + > + vser = DO_UPCAST(VirtIOSerial, vdev, vdev); > + memcpy(config_data, &vser->config, sizeof(struct virtio_console_config)); > +} > + > +static void set_config(VirtIODevice *vdev, const uint8_t *config_data) > +{ > + struct virtio_console_config config; > + > + memcpy(&config, config_data, sizeof(config)); > +} > + > +static void virtio_serial_save(QEMUFile *f, void *opaque) > +{ > + VirtIOSerial *s = opaque; > + VirtIOSerialPort *port; > + uint32_t nr_active_ports; > + unsigned int nr_bufs; > + > + /* The virtio device */ > + virtio_save(&s->vdev, f); > + /* The config space */ > + qemu_put_be16s(f, &s->config.cols); > + qemu_put_be16s(f, &s->config.rows); > + qemu_put_be32s(f, &s->config.nr_ports); > + > + /* Items in struct VirtIOSerial */ > + qemu_put_be32s(f, &s->guest_features); > + > + /* Do this because we might have hot-unplugged some ports */ > + nr_active_ports = 0; > + QTAILQ_FOREACH(port, &s->ports_head, next) > + nr_active_ports++; > + > + qemu_put_be32s(f, &nr_active_ports); > + > + /* > + * Items in struct VirtIOSerialPort. > + */ > + QTAILQ_FOREACH(port, &s->ports_head, next) { > + VirtIOSerialPortBuffer *buf; > + > + /* > + * We put the port number because we may not have an active > + * port at id 0 that's reserved for a console port, or in case > + * of ports that might have gotten unplugged > + */ > + qemu_put_be32s(f, &port->id); > + qemu_put_be64s(f, &port->byte_limit); > + qemu_put_be64s(f, &port->nr_bytes); > + qemu_put_byte(f, port->guest_connected); > + qemu_put_byte(f, port->host_throttled); > + > + /* All the pending buffers from active ports */ > + nr_bufs = 0; > + QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) { > + nr_bufs++; > + } > + qemu_put_be32s(f, &nr_bufs); > + if (!nr_bufs) { > + continue; > + } > + QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) { > + qemu_put_be64s(f, &buf->len); > + qemu_put_be32s(f, &buf->flags); > + qemu_put_buffer(f, buf->buf, buf->len); > + } > + } > +} > + > +static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) > +{ > + VirtIOSerial *s = opaque; > + VirtIOSerialPort *port; > + uint32_t nr_active_ports; > + unsigned int i; > + > + if (version_id > 2) { > + return -EINVAL; > + } > + /* The virtio device */ > + virtio_load(&s->vdev, f); > + > + if (version_id < 2) { > + return 0; > + } > + /* The config space */ > + qemu_get_be16s(f, &s->config.cols); > + qemu_get_be16s(f, &s->config.rows); > + s->config.nr_ports = qemu_get_be32(f); > + > + /* Items in struct VirtIOSerial */ > + qemu_get_be32s(f, &s->guest_features); > + > + qemu_get_be32s(f, &nr_active_ports); > + > + /* Items in struct VirtIOSerialPort */ > + for (i = 0; i < nr_active_ports; i++) { > + VirtIOSerialPortBuffer *buf; > + uint32_t id; > + unsigned int nr_bufs; > + > + id = qemu_get_be32(f); > + port = find_port_by_id(s, id); > + > + port->byte_limit = qemu_get_be64(f); > + port->nr_bytes = qemu_get_be64(f); > + port->guest_connected = qemu_get_byte(f); > + port->host_throttled = qemu_get_byte(f); > + > + /* All the pending buffers from active ports */ > + qemu_get_be32s(f, &nr_bufs); > + if (!nr_bufs) { > + continue; > + } > + for (; nr_bufs; nr_bufs--) { > + buf = qemu_malloc(sizeof(*buf)); > + > + qemu_get_be64s(f, &buf->len); > + qemu_get_be32s(f, &buf->flags); > + buf->buf = qemu_malloc(buf->len); > + qemu_get_buffer(f, buf->buf, buf->len); > + QTAILQ_INSERT_TAIL(&port->unflushed_buffer_head, buf, next); > + } > + /* > + * Applications might have internal state they track when they > + * receive a guest_open() callback. Apps could also open a > + * connection to this core in that case. So if the guest was > + * open at the time of migration, send a guest_open callback > + */ > + if (port->guest_connected && port->info->guest_open) { > + port->info->guest_open(port); > + } > + } > + return 0; > +} > + > +static void virtser_bus_print(Monitor *mon, DeviceState *qdev, int indent); > + > +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. > + DEFINE_PROP_END_OF_LIST(), > + }, > +}; > + > +static VirtIOSerialBus *virtser_bus_new(DeviceState *dev) > +{ > + VirtIOSerialBus *bus; > + > + bus = FROM_QBUS(VirtIOSerialBus, qbus_create(&virtser_bus_info, dev, NULL)); > + bus->qbus.allow_hotplug = 1; > + > + return bus; > +} > + > +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()? > +{ > + VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev); > + VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev); > + > + monitor_printf(mon, "%*s dev-prop-int: id: %u\n", > + indent, "", port->id); > + monitor_printf(mon, "%*s dev-prop-int: is_console: %d\n", > + indent, "", port->is_console); > + monitor_printf(mon, "%*s dev-prop-int: guest_connected: %d\n", > + indent, "", port->guest_connected); > + monitor_printf(mon, "%*s dev-prop-int: host_connected: %d\n", > + indent, "", port->host_connected); > + monitor_printf(mon, "%*s dev-prop-int: host_throttled: %d\n", > + indent, "", port->host_throttled); > + monitor_printf(mon, "%*s dev-prop-int: nr_bytes: %zu\n", > + indent, "", port->nr_bytes); > +} > + > +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? > + if (port->vser->config.nr_ports > bus->max_nr_ports) { > + qemu_error("virtio-serial-bus: Maximum device limit reached\n"); > + return -1; > + } > + dev->info = info; > + > + ret = info->init(dev); > + if (ret) { > + return ret; > + } > + QTAILQ_INIT(&port->unflushed_buffer_head); > + pthread_mutex_init(&port->buffer_list_lock, NULL); > + > + if (port->is_console && !find_port_by_id(port->vser, 0)) { > + /* > + * 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. > + QTAILQ_INSERT_TAIL(&port->vser->ports_head, port, next); > + port->ivq = port->vser->ivqs[port->id]; > + port->ovq = port->vser->ovqs[port->id]; > + > + /* Send an update to the guest about this new port added */ > + virtio_notify_config(&port->vser->vdev); > + > + return ret; > +} > + > +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? > + * 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? > + QTAILQ_REMOVE(&vser->ports_head, port, next); > + > + if (port->info->exit) > + port->info->exit(dev); > + > + remove_port_buffers(port); > + pthread_mutex_destroy(&port->buffer_list_lock); > + > + return 0; > +} > + > +void virtio_serial_port_qdev_register(VirtIOSerialPortInfo *info) > +{ > + info->qdev.init = virtser_port_qdev_init; > + info->qdev.bus_info = &virtser_bus_info; > + info->qdev.exit = virtser_port_qdev_exit; > + info->qdev.unplug = qdev_simple_unplug_cb; > + qdev_register(&info->qdev); > +} > + > +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. > + 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; > + > + vser->vdev.get_features = get_features; > + vser->vdev.set_features = set_features; > + vser->vdev.get_config = get_config; > + vser->vdev.set_config = set_config; > + > + /* > + * Register for the savevm section with the virtio-console name > + * to preserve backward compat > + */ > + register_savevm("virtio-console", -1, 2, virtio_serial_save, > + virtio_serial_load, vser); > + > + return vdev; > +} > diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h > new file mode 100644 > index 0000000..6cdaa5b > --- /dev/null > +++ b/hw/virtio-serial.h > @@ -0,0 +1,230 @@ > +/* > + * Virtio Serial / Console Support > + * > + * Copyright IBM, Corp. 2008 > + * Copyright Red Hat, Inc. 2009 > + * > + * Authors: > + * Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com> > + * Amit Shah <amit.shah@redhat.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2. See > + * the COPYING file in the top-level directory. > + * > + */ > +#ifndef _QEMU_VIRTIO_SERIAL_H > +#define _QEMU_VIRTIO_SERIAL_H > + > +#include <pthread.h> > +#include <stdbool.h> > +#include "qdev.h" > +#include "virtio.h" > + > +/* == Interface shared between the guest kernel and qemu == */ > + > +/* The Virtio ID for virtio console / serial ports */ > +#define VIRTIO_ID_CONSOLE 3 > + > +/* Features supported */ > +#define VIRTIO_CONSOLE_F_MULTIPORT 1 > + > +struct virtio_console_config { > + /* > + * These two fields are used by VIRTIO_CONSOLE_F_SIZE which > + * isn't implemented here yet > + */ > + uint16_t cols; > + uint16_t rows; > + > + uint32_t max_nr_ports; > + uint32_t nr_ports; > +} __attribute__((packed)); > + > +struct virtio_console_control { > + uint32_t id; /* Port number */ > + uint16_t event; /* The kind of control event (see below) */ > + uint16_t value; /* Extra information for the key */ > +}; > + > +struct virtio_console_header { > + uint32_t flags; /* Some message between host and guest */ > +}; > + > +/* Messages between host and guest */ > +#define VIRTIO_CONSOLE_HDR_START_DATA (1 << 0) > +#define VIRTIO_CONSOLE_HDR_END_DATA (1 << 1) > + > +/* Some events for the internal messages (control packets) */ > +#define VIRTIO_CONSOLE_PORT_READY 0 > +#define VIRTIO_CONSOLE_CONSOLE_PORT 1 > +#define VIRTIO_CONSOLE_RESIZE 2 > +#define VIRTIO_CONSOLE_PORT_OPEN 3 > +#define VIRTIO_CONSOLE_PORT_NAME 4 > +#define VIRTIO_CONSOLE_THROTTLE_PORT 5 > +#define VIRTIO_CONSOLE_CACHE_BUFFERS 6 > +#define VIRTIO_CONSOLE_PORT_REMOVE 7 > + > +/* == In-qemu interface == */ > + > +typedef struct VirtIOSerial VirtIOSerial; > +typedef struct VirtIOSerialBus VirtIOSerialBus; > +typedef struct VirtIOSerialPort VirtIOSerialPort; > +typedef struct VirtIOSerialPortInfo VirtIOSerialPortInfo; > + > +typedef struct VirtIOSerialDevice { > + DeviceState qdev; > + VirtIOSerialPortInfo *info; > +} VirtIOSerialDevice; > + > +/* > + * This is the state that's shared between all the ports. Some of the > + * state is configurable via command-line options. Some of it can be > + * set by individual devices in their initfn routines. Some of the > + * state is set by the generic qdev device init routine. > + */ > +struct VirtIOSerialPort { > + DeviceState dev; > + VirtIOSerialPortInfo *info; > + > + QTAILQ_ENTRY(VirtIOSerialPort) next; > + > + /* > + * This field gives us the virtio device as well as the qdev bus > + * that we are associated with > + */ > + VirtIOSerial *vser; > + > + VirtQueue *ivq, *ovq; > + > + /* > + * This name is sent to the guest and exported via sysfs. > + * The guest could create symlinks based on this information. > + * The name is in the reverse fqdn format, like org.qemu.console.0 > + */ > + char *name; > + > + /* > + * This list holds buffers pushed by the guest in case the guest > + * sent incomplete messages or the host connection was down and > + * the device requested to cache the data. > + */ > + QTAILQ_HEAD(, VirtIOSerialPortBuffer) unflushed_buffer_head; > + > + /* > + * This lock protects the unflushed_buffer_head list > + */ > + pthread_mutex_t buffer_list_lock; > + > + /* > + * This id helps identify ports between the guest and the host. > + * The guest sends a "header" with this id with each data packet > + * that it sends and the host can then find out which associated > + * device to send out this data to > + */ > + uint32_t id; > + > + /* > + * Each port can specify the limit on number of bytes that can be > + * outstanding in the unread buffers. This is to prevent any OOM > + * situtation if a rogue process on the guest keeps injecting > + * data. > + */ > + size_t byte_limit; > + > + /* > + * The number of bytes we have queued up in our unread queue > + */ > + size_t nr_bytes; > + > + /* > + * This boolean, when set, means "queue data that gets sent to > + * this port when the host is not connected". The queued data, if > + * any, is then sent out to the port when the host connection is > + * opened. > + */ > + int cache_buffers; > + > + /* Identify if this is a port that binds with hvc in the guest */ > + bool is_console; > + > + /* Is the corresponding guest device open? */ > + bool guest_connected; > + /* Is this device open for IO on the host? */ > + bool host_connected; > + /* Have we sent a throttle message to the guest? */ > + bool host_throttled; > + > + /* Did this port get data in the recent handle_output call? */ > + bool has_activity; > +}; > + > + > +struct VirtIOSerialPortInfo { > + DeviceInfo qdev; > + /* > + * The per-port (or per-app) init function that's called when a > + * new device is found on the bus. > + */ > + int (*init)(VirtIOSerialDevice *dev); > + /* > + * Per-port exit function that's called when a port gets > + * hot-unplugged or removed > + */ > + int (*exit)(VirtIOSerialDevice *dev); > + > + /* Callbacks for guest events */ > + /* > + * Guest opened device. This could be invoked even when an > + * application thinks the guest is open. This can happen if > + * the host is migrated to another machine when the connection > + * was open and is called from the destination machine if > + * there's any app-specific initialisation to be done in such > + * a case. > + */ > + void (*guest_open)(VirtIOSerialPort *port); > + /* Guest closed device */ > + void (*guest_close)(VirtIOSerialPort *port); > + > + /* Guest is now ready to accept data (virtqueues set up) */ > + void (*guest_ready)(VirtIOSerialPort *port); > + /* > + * Guest wrote some data to the port. This data is handed over to > + * the app via this callback. The data is not maintained anymore > + * after the callback returns. This means the app has to ensure it > + * read all the data and consumes it. > + * > + * If an app needs to have the data for a longer time, it's upto > + * the app to cache it. > + */ > + ssize_t (*have_data)(VirtIOSerialPort *port, const uint8_t *buf, size_t len); > +}; > + > +/* Interface to the virtio-serial bus */ > + > +/* > + * Individual ports/apps should call this function to register the port > + * with the virtio-serial bus > + */ > +void virtio_serial_port_qdev_register(VirtIOSerialPortInfo *info); > +/* > + * Open a connection to the port > + * Returns 0 on success (always) > + */ > +int virtio_serial_open(VirtIOSerialPort *port); > +/* > + * 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"? > + */ > +int virtio_serial_close(VirtIOSerialPort *port); > +/* > + * Send data to Guest > + */ > +ssize_t virtio_serial_write(VirtIOSerialPort *port, const uint8_t *buf, > + size_t size); > +/* > + * Query whether a guest is ready to receive data. > + */ > +size_t virtio_serial_guest_ready(VirtIOSerialPort *port); > + > +#endif > diff --git a/hw/virtio.h b/hw/virtio.h > index 769f540..a5d3147 100644 > --- a/hw/virtio.h > +++ b/hw/virtio.h > @@ -171,7 +171,7 @@ void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding, > /* Base devices. */ > VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo); > VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf); > -VirtIODevice *virtio_console_init(DeviceState *dev); > +VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports); > VirtIODevice *virtio_balloon_init(DeviceState *dev); > > void virtio_net_exit(VirtIODevice *vdev); > diff --git a/qemu-options.hx b/qemu-options.hx > index b8cc375..183b616 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -1873,6 +1873,10 @@ DEF("virtioconsole", HAS_ARG, QEMU_OPTION_virtiocon, \ > STEXI > @item -virtioconsole @var{c} > Set virtio console. > + > +This option is maintained for backward compatibility. > + > +Please use @code{-device virtconsole} for the new way of invocation. > ETEXI > > DEF("show-cursor", 0, QEMU_OPTION_show_cursor, \ > diff --git a/sysemu.h b/sysemu.h > index 9d80bb2..9c3b281 100644 > --- a/sysemu.h > +++ b/sysemu.h > @@ -231,12 +231,6 @@ extern CharDriverState *serial_hds[MAX_SERIAL_PORTS]; > > extern CharDriverState *parallel_hds[MAX_PARALLEL_PORTS]; > > -/* virtio consoles */ > - > -#define MAX_VIRTIO_CONSOLES 1 > - > -extern CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES]; > - > #define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR) > > #ifdef HAS_AUDIO > diff --git a/vl.c b/vl.c > index e606903..523022b 100644 > --- a/vl.c > +++ b/vl.c > @@ -173,6 +173,8 @@ int main(int argc, char **argv) > > #define DEFAULT_RAM_SIZE 128 > > +#define MAX_VIRTIO_CONSOLES 1 > + > static const char *data_dir; > const char *bios_name = NULL; > /* Note: drives_table[MAX_DRIVES] is a dummy block driver if none available > @@ -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? > @@ -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? > if (!display_state) > dumb_display_init(); > /* just use the first displaystate for the moment */
On (Wed) Dec 23 2009 [14:54:55], Markus Armbruster wrote: > Amit Shah <amit.shah@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. > 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. > > 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... > /* 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. > 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. > > +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. > > + * 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.) > > +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. > > + 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?' 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. Thanks a lot for the review, smaller patches coming soon. Amit
Amit Shah <amit.shah@redhat.com> writes: > On (Wed) Dec 23 2009 [14:54:55], Markus Armbruster wrote: >> Amit Shah <amit.shah@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 :)
On (Wed) Dec 23 2009 [20:02:19], Markus Armbruster wrote: > Amit Shah <amit.shah@redhat.com> writes: > > > On (Wed) Dec 23 2009 [14:54:55], Markus Armbruster wrote: > >> Amit Shah <amit.shah@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? Yes; virtserialport, as the next patch in the series introduces. Also, virtserialvnc, etc. > > the serial init can be changed, I guess. > > Okay, Gerd's the authority on this. > >> 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? That too would work. Or I can just send the patch with virtio-serial.c and once the patches are merged, rename virtio-serial.c to virtio-console.c and all will be OK again. > >> > +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...) > > :) BTW please keep an eye out for the way this is done in the next patch series; it's a bit of a hack but I could only think of doing it that way. > >> > + 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? 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. > >> > +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? That's right. I wanted to avoid referencing the device's internal struct data to fetch the max_nr_ports value. So I let the device pass it on to us and I stuck it in as bus-specific data. > >> > + 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. > >> > @@ -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. > >> > 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. Amit
Amit Shah <amit.shah@redhat.com> writes: > On (Wed) Dec 23 2009 [20:02:19], Markus Armbruster wrote: >> Amit Shah <amit.shah@redhat.com> writes: >> >> > On (Wed) Dec 23 2009 [14:54:55], Markus Armbruster wrote: >> >> Amit Shah <amit.shah@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? > > 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. >> > the serial init can be changed, I guess. >> >> Okay, Gerd's the authority on this. > >> >> 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? > > That too would work. Or I can just send the patch with virtio-serial.c > and once the patches are merged, rename virtio-serial.c to > virtio-console.c and all will be OK again. Yes. >> >> > +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...) >> >> :) > > BTW please keep an eye out for the way this is done in the next patch > series; it's a bit of a hack but I could only think of doing it that > way. Okay, but I doubt I can do it before my Christmas vacation. >> >> > + 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? > > 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. >> >> > +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? > > That's right. I wanted to avoid referencing the device's internal struct > data to fetch the max_nr_ports value. So I let the device pass it on to > us and I stuck it in as bus-specific data. Fair enough. >> >> > + 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. > >> >> > @@ -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. >> >> > 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. What happens if you have both -device virtio-serial-pci and -virtioconsole?
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
Amit Shah <amit.shah@redhat.com> writes: > On (Wed) Dec 23 2009 [23:07:24], Markus Armbruster wrote: [...] >> 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). It's a weird thing to do. Perhaps less weird is the case where virtio-serial-pci comes from a configuration file. We could declare that -virtioconsole always creates a bus a feature, but I can do that in good conscience only if the console created along with it always gets attached to it, even when other suitable buses exist. Possibly trivial if you act on -virtioconsole before generic devices (-device & configuration file). Or we could make it create virtio-serial-pci only if no suitable bus exists already. For that it needs to run after generic devices are set up.
Hi, >>>> @@ -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. Yes, serial drivers should use a chardev property instead of qdev_init_chardev(). You can try to zap this function altogether, but I think there are still serial drivers using this so this would break the (full, all archs) build. cheers, Gerd
On 12/24/09 09:26, Markus Armbruster wrote: > We could declare that -virtioconsole always creates a bus a feature, but > I can do that in good conscience only if the console created along with > it always gets attached to it, even when other suitable buses exist. I'd just discourage mixing -virtioconsole and -device virtio-serial-$bus The whole point of -virtioconsole is backward compatibility for management apps / start scripts and the like and it should create a device which can be used by the guest like the qemu 0.11 virtioconsole. If you are using device virtio-serial-$bus you are obviously not needing the backward compatibility stuff ;) cheers, Gerd
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"); - } - } - } - 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", .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 <ehrhardt@linux.vnet.ibm.com> + * Amit Shah <amit.shah@redhat.com> * * This work is licensed under the terms of the GNU GPL, version 2. See * the COPYING file in the top-level directory. - * */ -#include "hw.h" #include "qemu-char.h" -#include "virtio.h" -#include "virtio-console.h" - +#include "virtio-serial.h" -typedef struct VirtIOConsole -{ - VirtIODevice vdev; - VirtQueue *ivq, *ovq; +typedef struct VirtConsole { + VirtIOSerialPort port; CharDriverState *chr; -} VirtIOConsole; +} VirtConsole; -static VirtIOConsole *to_virtio_console(VirtIODevice *vdev) -{ - return (VirtIOConsole *)vdev; -} -static void virtio_console_handle_output(VirtIODevice *vdev, VirtQueue *vq) +/* Callback function that's called when the guest sends us data */ +static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len) { - VirtIOConsole *s = to_virtio_console(vdev); - VirtQueueElement elem; - - while (virtqueue_pop(vq, &elem)) { - ssize_t len = 0; - int d; - - for (d = 0; d < elem.out_num; d++) { - len += qemu_chr_write(s->chr, (uint8_t *)elem.out_sg[d].iov_base, - elem.out_sg[d].iov_len); - } - virtqueue_push(vq, &elem, len); - virtio_notify(vdev, vq); - } -} + VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); -static void virtio_console_handle_input(VirtIODevice *vdev, VirtQueue *vq) -{ + return qemu_chr_write(vcon->chr, buf, len); } -static uint32_t virtio_console_get_features(VirtIODevice *vdev) -{ - return 0; -} -static int vcon_can_read(void *opaque) +/* Readiness of the guest to accept data on a port */ +static int chr_can_read(void *opaque) { - VirtIOConsole *s = (VirtIOConsole *) opaque; - - if (!virtio_queue_ready(s->ivq) || - !(s->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) || - virtio_queue_empty(s->ivq)) - return 0; - - /* current implementations have a page sized buffer. - * We fall back to a one byte per read if there is not enough room. - * It would be cool to have a function that returns the available byte - * instead of checking for a limit */ - if (virtqueue_avail_bytes(s->ivq, TARGET_PAGE_SIZE, 0)) - return TARGET_PAGE_SIZE; - if (virtqueue_avail_bytes(s->ivq, 1, 0)) - return 1; - return 0; -} + VirtConsole *vcon = opaque; -static void vcon_read(void *opaque, const uint8_t *buf, int size) -{ - VirtIOConsole *s = (VirtIOConsole *) opaque; - VirtQueueElement elem; - int offset = 0; - - /* The current kernel implementation has only one outstanding input - * buffer of PAGE_SIZE. Nevertheless, this function is prepared to - * handle multiple buffers with multiple sg element for input */ - while (offset < size) { - int i = 0; - if (!virtqueue_pop(s->ivq, &elem)) - break; - while (offset < size && i < elem.in_num) { - int len = MIN(elem.in_sg[i].iov_len, size - offset); - memcpy(elem.in_sg[i].iov_base, buf + offset, len); - offset += len; - i++; - } - virtqueue_push(s->ivq, &elem, size); - } - virtio_notify(&s->vdev, s->ivq); + return virtio_serial_guest_ready(&vcon->port); } -static void vcon_event(void *opaque, int event) +/* Send data from a char device over to the guest */ +static void chr_read(void *opaque, const uint8_t *buf, int size) { - /* we will ignore any event for the time being */ + VirtConsole *vcon = opaque; + + virtio_serial_write(&vcon->port, buf, size); } -static void virtio_console_save(QEMUFile *f, void *opaque) +static void chr_event(void *opaque, int event) { - VirtIOConsole *s = opaque; + VirtConsole *vcon = opaque; - virtio_save(&s->vdev, f); + switch (event) { + case CHR_EVENT_OPENED: { + virtio_serial_open(&vcon->port); + break; + } + case CHR_EVENT_CLOSED: + virtio_serial_close(&vcon->port); + break; + } } -static int virtio_console_load(QEMUFile *f, void *opaque, int version_id) +/* Virtio Console Ports */ +static int vcon_initfn(VirtIOSerialDevice *dev) { - VirtIOConsole *s = opaque; - - if (version_id != 1) - return -EINVAL; - - virtio_load(&s->vdev, f); + 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; + + if (vcon->chr) { + qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event, + vcon); + } return 0; } -VirtIODevice *virtio_console_init(DeviceState *dev) +static int vcon_exitfn(VirtIOSerialDevice *dev) { - VirtIOConsole *s; - s = (VirtIOConsole *)virtio_common_init("virtio-console", - VIRTIO_ID_CONSOLE, - 0, sizeof(VirtIOConsole)); - s->vdev.get_features = virtio_console_get_features; - - s->ivq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_input); - s->ovq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_output); + VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev); + VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); - s->chr = qdev_init_chardev(dev); - qemu_chr_add_handlers(s->chr, vcon_can_read, vcon_read, vcon_event, s); + if (vcon->chr) + qemu_chr_close(vcon->chr); - register_savevm("virtio-console", -1, 1, virtio_console_save, virtio_console_load, s); + return 0; +} - return &s->vdev; +static VirtIOSerialPortInfo virtcon_info = { + .qdev.name = "virtconsole", + .qdev.size = sizeof(VirtConsole), + .init = vcon_initfn, + .exit = vcon_exitfn, + .have_data = flush_buf, + .qdev.props = (Property[]) { + DEFINE_PROP_CHR("chardev", VirtConsole, chr), + DEFINE_PROP_STRING("name", VirtConsole, port.name), + DEFINE_PROP_END_OF_LIST(), + }, +}; + +static void virtcon_register(void) +{ + virtio_serial_port_qdev_register(&virtcon_info); } +device_init(virtcon_register) diff --git a/hw/virtio-console.h b/hw/virtio-console.h deleted file mode 100644 index 84d0717..0000000 --- a/hw/virtio-console.h +++ /dev/null @@ -1,19 +0,0 @@ -/* - * Virtio Console Support - * - * Copyright IBM, Corp. 2008 - * - * Authors: - * Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com> - * - * This work is licensed under the terms of the GNU GPL, version 2. See - * the COPYING file in the top-level directory. - * - */ -#ifndef _QEMU_VIRTIO_CONSOLE_H -#define _QEMU_VIRTIO_CONSOLE_H - -/* The ID for virtio console */ -#define VIRTIO_ID_CONSOLE 3 - -#endif diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 62b46bd..1baca0d 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -92,6 +92,7 @@ typedef struct { uint32_t nvectors; DriveInfo *dinfo; NICConf nic; + uint32_t max_virtserial_ports; } VirtIOPCIProxy; /* virtio device */ @@ -481,7 +482,7 @@ static int virtio_blk_exit_pci(PCIDevice *pci_dev) return virtio_exit_pci(pci_dev); } -static int virtio_console_init_pci(PCIDevice *pci_dev) +static int virtio_serial_init_pci(PCIDevice *pci_dev) { VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); VirtIODevice *vdev; @@ -491,7 +492,7 @@ static int virtio_console_init_pci(PCIDevice *pci_dev) proxy->class_code != PCI_CLASS_OTHERS) /* qemu-kvm */ proxy->class_code = PCI_CLASS_COMMUNICATION_OTHER; - vdev = virtio_console_init(&pci_dev->qdev); + vdev = virtio_serial_init(&pci_dev->qdev, proxy->max_virtserial_ports); if (!vdev) { return -1; } @@ -569,12 +570,14 @@ static PCIDeviceInfo virtio_info[] = { }, .qdev.reset = virtio_pci_reset, },{ - .qdev.name = "virtio-console-pci", + .qdev.name = "virtio-serial-pci", .qdev.size = sizeof(VirtIOPCIProxy), - .init = virtio_console_init_pci, + .init = virtio_serial_init_pci, .exit = virtio_exit_pci, .qdev.props = (Property[]) { DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0), + DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, max_virtserial_ports, + 15), DEFINE_PROP_END_OF_LIST(), }, .qdev.reset = virtio_pci_reset, diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c new file mode 100644 index 0000000..fd0ea12 --- /dev/null +++ b/hw/virtio-serial-bus.c @@ -0,0 +1,964 @@ +/* + * A bus for connecting virtio serial and console ports + * + * Copyright (C) 2009 Red Hat, Inc. + * + * Author(s): + * Amit Shah <amit.shah@redhat.com> + * + * Some earlier parts are: + * Copyright IBM, Corp. 2008 + * authored by + * Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + */ + +#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; + uint32_t max_nr_ports; +}; + +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; + + /* + * This lock serialises writes to the guest via the ivq + */ + pthread_mutex_t ivq_lock; + + uint32_t guest_features; +}; + +/* This struct holds individual buffers received for each port */ +typedef struct VirtIOSerialPortBuffer { + QTAILQ_ENTRY(VirtIOSerialPortBuffer) next; + + uint8_t *buf; + + size_t len; /* length of the buffer */ + size_t offset; /* offset from which to consume data in the buffer */ + + uint32_t flags; /* Sent by guest (start of data stream, end of stream) */ + + bool previous_failure; /* Did sending out this buffer fail previously? */ +} VirtIOSerialPortBuffer; + +static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id) +{ + VirtIOSerialPort *port; + + QTAILQ_FOREACH(port, &vser->ports_head, next) { + if (port->id == id) + return port; + } + return NULL; +} + +static VirtIOSerialPort *find_port_by_vq(VirtIOSerial *vser, VirtQueue *vq) +{ + VirtIOSerialPort *port; + + QTAILQ_FOREACH(port, &vser->ports_head, next) { + if (port->ivq == vq || port->ovq == vq) + return port; + } + return NULL; +} + +static bool use_multiport(VirtIOSerial *vser) +{ + return vser->guest_features & (1 << VIRTIO_CONSOLE_F_MULTIPORT); +} + +static size_t write_to_port(VirtIOSerialPort *port, + const uint8_t *buf, size_t size) +{ + VirtQueueElement elem; + struct virtio_console_header header; + VirtQueue *vq; + size_t offset = 0; + size_t len = 0; + int header_len; + + vq = port->ivq; + if (!virtio_queue_ready(vq)) { + return 0; + } + if (!size) { + return 0; + } + header.flags = 0; + header_len = use_multiport(port->vser) ? sizeof(header) : 0; + + pthread_mutex_lock(&port->vser->ivq_lock); + while (offset < size) { + int i; + + if (!virtqueue_pop(vq, &elem)) { + break; + } + if (elem.in_sg[0].iov_len < header_len) { + /* We can't even store our port number in this buffer. Bug? */ + qemu_error("virtio-serial: size %zd less than expected\n", + elem.in_sg[0].iov_len); + exit(1); + } + if (header_len) { + memcpy(elem.in_sg[0].iov_base, &header, header_len); + } + + for (i = 0; offset < size && i < elem.in_num; i++) { + /* Copy the header only in the first sg. */ + len = MIN(elem.in_sg[i].iov_len - header_len, size - offset); + + memcpy(elem.in_sg[i].iov_base + header_len, buf + offset, len); + offset += len; + header_len = 0; + } + header_len = use_multiport(port->vser) ? sizeof(header) : 0; + virtqueue_push(vq, &elem, len + header_len); + } + + virtio_notify(&port->vser->vdev, vq); + pthread_mutex_unlock(&port->vser->ivq_lock); + return offset; +} + +static size_t send_control_event(VirtIOSerialPort *port, void *buf, size_t len) +{ + VirtQueueElement elem; + VirtQueue *vq; + struct virtio_console_control *cpkt; + + vq = port->vser->c_ivq; + if (!virtio_queue_ready(vq)) { + return 0; + } + if (!virtqueue_pop(vq, &elem)) { + return 0; + } + + cpkt = (struct virtio_console_control *)buf; + cpkt->id = port->id; + memcpy(elem.in_sg[0].iov_base, buf, len); + + virtqueue_push(vq, &elem, len); + virtio_notify(&port->vser->vdev, vq); + return len; +} + +static size_t get_complete_data_size(VirtIOSerialPort *port) +{ + VirtIOSerialPortBuffer *buf; + size_t size; + bool is_complete, start_seen; + + size = 0; + is_complete = false; + start_seen = false; + QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) { + size += buf->len - buf->offset; + + if (buf->flags & VIRTIO_CONSOLE_HDR_END_DATA) { + is_complete = true; + break; + } + if (buf == QTAILQ_FIRST(&port->unflushed_buffer_head) + && !(buf->flags & VIRTIO_CONSOLE_HDR_START_DATA)) { + + /* There's some data that arrived without a START flag. Flush it. */ + is_complete = true; + break; + } + + if (buf->flags & VIRTIO_CONSOLE_HDR_START_DATA) { + if (start_seen) { + /* + * There's some data that arrived without an END + * flag. Flush it. + */ + size -= buf->len + buf->offset; + is_complete = true; + break; + } + start_seen = true; + } + } + return is_complete ? size : 0; +} + +/* + * The guest could have sent the data corresponding to one write + * request split up in multiple buffers. The first buffer has the + * VIRTIO_CONSOLE_HDR_START_DATA flag set and the last buffer has the + * VIRTIO_CONSOLE_HDR_END_DATA flag set. Using this information, merge + * the parts into one buf here to process it for output. + */ +static VirtIOSerialPortBuffer *get_complete_buf(VirtIOSerialPort *port) +{ + VirtIOSerialPortBuffer *buf, *buf2; + uint8_t *outbuf; + size_t out_offset, out_size; + + out_size = get_complete_data_size(port); + if (!out_size) + return NULL; + + buf = QTAILQ_FIRST(&port->unflushed_buffer_head); + if (buf->len - buf->offset == out_size) { + QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next); + return buf; + } + out_offset = 0; + outbuf = qemu_malloc(out_size); + + QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffer_head, next, buf2) { + size_t copy_size; + + copy_size = buf->len - buf->offset; + memcpy(outbuf + out_offset, buf->buf + buf->offset, copy_size); + out_offset += copy_size; + + QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next); + qemu_free(buf->buf); + + if (out_offset == out_size) { + break; + } + qemu_free(buf); + } + buf->buf = outbuf; + buf->len = out_size; + buf->offset = 0; + buf->flags = VIRTIO_CONSOLE_HDR_START_DATA | VIRTIO_CONSOLE_HDR_END_DATA; + buf->previous_failure = false; + + return buf; +} + +/* Call with the buffer_list_lock held */ +static void flush_queue(VirtIOSerialPort *port) +{ + VirtIOSerialPortBuffer *buf; + size_t out_size; + ssize_t ret; + + /* + * If a device is interested in buffering packets till it's + * opened, cache the data the guest sends us till a connection is + * established. + */ + if (!port->host_connected && port->cache_buffers) { + return; + } + while ((buf = get_complete_buf(port))) { + out_size = buf->len - buf->offset; + if (!port->host_connected) { + /* + * Caching is disabled and host is not connected, so + * discard the buffer. Do this only after merging the + * buffer as a port can get connected in the middle of + * dropping buffers and the port will end up getting the + * incomplete output. + */ + port->nr_bytes -= buf->len + buf->offset; + qemu_free(buf->buf); + qemu_free(buf); + continue; + } + + ret = port->info->have_data(port, buf->buf + buf->offset, out_size); + if (ret < out_size) { + QTAILQ_INSERT_HEAD(&port->unflushed_buffer_head, buf, next); + } + if (ret <= 0) { + /* We're not progressing at all */ + if (buf->previous_failure) { + break; + } + buf->previous_failure = true; + } else { + buf->offset += ret; + port->nr_bytes -= ret; + buf->previous_failure = false; + } + if (!(buf->len - buf->offset)) { + qemu_free(buf->buf); + qemu_free(buf); + } + } + + if (port->host_throttled && port->nr_bytes < port->byte_limit) { + struct virtio_console_control cpkt; + + port->host_throttled = false; + cpkt.event = VIRTIO_CONSOLE_THROTTLE_PORT; + cpkt.value = false; + send_control_event(port, &cpkt, sizeof(cpkt)); + } +} + +static void flush_all_ports(VirtIOSerial *vser) +{ + struct VirtIOSerialPort *port; + + QTAILQ_FOREACH(port, &vser->ports_head, next) { + pthread_mutex_lock(&port->buffer_list_lock); + if (port->has_activity) { + port->has_activity = false; + flush_queue(port); + } + pthread_mutex_unlock(&port->buffer_list_lock); + } +} + +static void remove_port_buffers(VirtIOSerialPort *port) +{ + struct VirtIOSerialPortBuffer *buf, *buf2; + + pthread_mutex_lock(&port->buffer_list_lock); + QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffer_head, next, buf2) { + QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next); + qemu_free(buf->buf); + qemu_free(buf); + } + pthread_mutex_unlock(&port->buffer_list_lock); +} + +/* Functions for use inside qemu to open and read from/write to ports */ +int virtio_serial_open(VirtIOSerialPort *port) +{ + struct virtio_console_control cpkt; + + /* Don't allow opening an already-open port */ + if (port->host_connected) { + return 0; + } + /* Send port open notification to the guest */ + port->host_connected = true; + cpkt.event = VIRTIO_CONSOLE_PORT_OPEN; + cpkt.value = 1; + send_control_event(port, &cpkt, sizeof(cpkt)); + + /* Flush any buffers that were cached while the port was closed */ + if (port->cache_buffers && port->info->have_data) { + pthread_mutex_lock(&port->buffer_list_lock); + flush_queue(port); + pthread_mutex_unlock(&port->buffer_list_lock); + } + return 0; +} + +int virtio_serial_close(VirtIOSerialPort *port) +{ + struct virtio_console_control cpkt; + + port->host_connected = false; + cpkt.event = VIRTIO_CONSOLE_PORT_OPEN; + cpkt.value = 0; + send_control_event(port, &cpkt, sizeof(cpkt)); + + if (!port->cache_buffers) { + remove_port_buffers(port); + } + return 0; +} + +/* Individual ports/apps call this function to write to the guest. */ +ssize_t virtio_serial_write(VirtIOSerialPort *port, const uint8_t *buf, + size_t size) +{ + if (!port || !port->host_connected || !port->guest_connected) { + return 0; + } + return write_to_port(port, buf, size); +} + +/* + * Readiness of the guest to accept data on a port. + * Returns max. data the guest can receive + */ +size_t virtio_serial_guest_ready(VirtIOSerialPort *port) +{ + VirtQueue *vq = port->ivq; + size_t size, header_len; + + if (use_multiport(port->vser)) { + header_len = sizeof(struct virtio_console_header); + } else { + header_len = 0; + } + if (!virtio_queue_ready(vq) || + !(port->vser->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) || + virtio_queue_empty(vq)) { + return 0; + } + if (use_multiport(port->vser) && !port->guest_connected) { + return 0; + } + + size = TARGET_PAGE_SIZE; + if (virtqueue_avail_bytes(vq, size, 0)) { + return size - header_len; + } + size = header_len + 1; + if (virtqueue_avail_bytes(vq, size, 0)) { + return size - header_len; + } + return 0; +} + +/* Guest wants to notify us of some event */ +static void handle_control_message(VirtIOSerial *vser, void *buf) +{ + struct VirtIOSerialPort *port; + struct virtio_console_control *cpkt; + uint8_t *buffer; + size_t buffer_len; + + cpkt = buf; + port = find_port_by_id(vser, cpkt->id); + if (!port) + return; + + switch(cpkt->event) { + case VIRTIO_CONSOLE_PORT_READY: + /* + * Now that we know the guest asked for the port name, we're + * sure the guest has initialised whatever state is necessary + * for this port. Now's a good time to let the guest know if + * this port is a console port so that the guest can hook it + * up to hvc. + */ + if (port->is_console) { + cpkt->event = VIRTIO_CONSOLE_CONSOLE_PORT; + cpkt->value = 1; + send_control_event(port, cpkt, sizeof(cpkt)); + } + if (port->name) { + cpkt->event = VIRTIO_CONSOLE_PORT_NAME; + cpkt->value = 1; + + buffer_len = sizeof(*cpkt) + strlen(port->name) + 1; + buffer = qemu_malloc(buffer_len); + + memcpy(buffer, cpkt, sizeof(*cpkt)); + memcpy(buffer + sizeof(*cpkt), port->name, strlen(port->name)); + buffer[buffer_len - 1] = 0; + + send_control_event(port, buffer, buffer_len); + qemu_free(buffer); + } + /* + * We also want to signal to the guest whether or not the port + * is set to caching the buffers when disconnected. + */ + if (port->cache_buffers) { + cpkt->event = VIRTIO_CONSOLE_CACHE_BUFFERS; + cpkt->value = 1; + send_control_event(port, cpkt, sizeof(cpkt)); + } + if (port->host_connected) { + cpkt->event = VIRTIO_CONSOLE_PORT_OPEN; + cpkt->value = 1; + send_control_event(port, cpkt, sizeof(cpkt)); + } + /* + * When the guest has asked us for this information it means + * the guest is all setup and has its virtqueues + * initialised. If some app is interested in knowing about + * this event, let it know + */ + if (port->info->guest_ready) { + port->info->guest_ready(port); + } + break; + case VIRTIO_CONSOLE_PORT_OPEN: + port->guest_connected = cpkt->value; + if (cpkt->value && port->info->guest_open) { + /* Send the guest opened notification if an app is interested */ + port->info->guest_open(port); + } + if (!cpkt->value && port->info->guest_close) { + /* Send the guest closed notification if an app is interested */ + port->info->guest_close(port); + } + break; + } +} + +static void control_in(VirtIODevice *vdev, VirtQueue *vq) +{ +} + +static void control_out(VirtIODevice *vdev, VirtQueue *vq) +{ + VirtQueueElement elem; + VirtIOSerial *vser; + + vser = DO_UPCAST(VirtIOSerial, vdev, vdev); + + while (virtqueue_pop(vq, &elem)) { + handle_control_message(vser, elem.out_sg[0].iov_base); + virtqueue_push(vq, &elem, elem.out_sg[0].iov_len); + } + virtio_notify(vdev, vq); +} + +/* + * Guest wrote something to some port. + * + * Flush the data in the entire chunk that we received rather than + * splitting it into multiple buffers. VNC clients don't consume split + * buffers + */ +static void handle_output(VirtIODevice *vdev, VirtQueue *vq) +{ + VirtIOSerial *vser; + VirtQueueElement elem; + + vser = DO_UPCAST(VirtIOSerial, vdev, vdev); + + while (virtqueue_pop(vq, &elem)) { + VirtIOSerialPort *port; + VirtIOSerialPortBuffer *buf; + struct virtio_console_header header; + int header_len; + + header_len = use_multiport(vser) ? sizeof(header) : 0; + + if (elem.out_sg[0].iov_len < header_len) { + goto next_buf; + } + if (header_len) { + memcpy(&header, elem.out_sg[0].iov_base, header_len); + } + port = find_port_by_vq(vser, vq); + if (!port) { + goto next_buf; + } + /* + * A port may not have any handler registered for consuming the + * data that the guest sends or it may not have a chardev associated + * with it. Just ignore the data in that case. + */ + if (!port->info->have_data) { + goto next_buf; + } + + /* The guest always sends only one sg */ + buf = qemu_mallocz(sizeof(*buf)); + buf->len = elem.out_sg[0].iov_len - header_len; + buf->buf = qemu_malloc(buf->len); + memcpy(buf->buf, elem.out_sg[0].iov_base + header_len, buf->len); + + if (header_len) { + /* + * Only the first buffer in a stream will have this + * set. This will help us identify the first buffer and + * the remaining buffers in the stream based on length + */ + buf->flags = header.flags & (VIRTIO_CONSOLE_HDR_START_DATA + | VIRTIO_CONSOLE_HDR_END_DATA); + } else { + /* We always want to flush all the buffers in this case */ + buf->flags = VIRTIO_CONSOLE_HDR_START_DATA + | VIRTIO_CONSOLE_HDR_END_DATA; + } + + pthread_mutex_lock(&port->buffer_list_lock); + QTAILQ_INSERT_TAIL(&port->unflushed_buffer_head, buf, next); + port->nr_bytes += buf->len; + port->has_activity = true; + pthread_mutex_unlock(&port->buffer_list_lock); + + if (!port->host_throttled && port->byte_limit && + port->nr_bytes >= port->byte_limit) { + struct virtio_console_control cpkt; + + port->host_throttled = true; + cpkt.event = VIRTIO_CONSOLE_THROTTLE_PORT; + cpkt.value = true; + send_control_event(port, &cpkt, sizeof(cpkt)); + } + next_buf: + virtqueue_push(vq, &elem, elem.out_sg[0].iov_len); + } + virtio_notify(vdev, vq); + flush_all_ports(vser); +} + +static void handle_input(VirtIODevice *vdev, VirtQueue *vq) +{ +} + +static uint32_t get_features(VirtIODevice *vdev) +{ + return 1 << VIRTIO_CONSOLE_F_MULTIPORT; +} + +static void set_features(VirtIODevice *vdev, uint32_t features) +{ + VirtIOSerial *vser; + + vser = DO_UPCAST(VirtIOSerial, vdev, vdev); + vser->guest_features = features; +} + +/* Guest requested config info */ +static void get_config(VirtIODevice *vdev, uint8_t *config_data) +{ + VirtIOSerial *vser; + + vser = DO_UPCAST(VirtIOSerial, vdev, vdev); + memcpy(config_data, &vser->config, sizeof(struct virtio_console_config)); +} + +static void set_config(VirtIODevice *vdev, const uint8_t *config_data) +{ + struct virtio_console_config config; + + memcpy(&config, config_data, sizeof(config)); +} + +static void virtio_serial_save(QEMUFile *f, void *opaque) +{ + VirtIOSerial *s = opaque; + VirtIOSerialPort *port; + uint32_t nr_active_ports; + unsigned int nr_bufs; + + /* The virtio device */ + virtio_save(&s->vdev, f); + /* The config space */ + qemu_put_be16s(f, &s->config.cols); + qemu_put_be16s(f, &s->config.rows); + qemu_put_be32s(f, &s->config.nr_ports); + + /* Items in struct VirtIOSerial */ + qemu_put_be32s(f, &s->guest_features); + + /* Do this because we might have hot-unplugged some ports */ + nr_active_ports = 0; + QTAILQ_FOREACH(port, &s->ports_head, next) + nr_active_ports++; + + qemu_put_be32s(f, &nr_active_ports); + + /* + * Items in struct VirtIOSerialPort. + */ + QTAILQ_FOREACH(port, &s->ports_head, next) { + VirtIOSerialPortBuffer *buf; + + /* + * We put the port number because we may not have an active + * port at id 0 that's reserved for a console port, or in case + * of ports that might have gotten unplugged + */ + qemu_put_be32s(f, &port->id); + qemu_put_be64s(f, &port->byte_limit); + qemu_put_be64s(f, &port->nr_bytes); + qemu_put_byte(f, port->guest_connected); + qemu_put_byte(f, port->host_throttled); + + /* All the pending buffers from active ports */ + nr_bufs = 0; + QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) { + nr_bufs++; + } + qemu_put_be32s(f, &nr_bufs); + if (!nr_bufs) { + continue; + } + QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) { + qemu_put_be64s(f, &buf->len); + qemu_put_be32s(f, &buf->flags); + qemu_put_buffer(f, buf->buf, buf->len); + } + } +} + +static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) +{ + VirtIOSerial *s = opaque; + VirtIOSerialPort *port; + uint32_t nr_active_ports; + unsigned int i; + + if (version_id > 2) { + return -EINVAL; + } + /* The virtio device */ + virtio_load(&s->vdev, f); + + if (version_id < 2) { + return 0; + } + /* The config space */ + qemu_get_be16s(f, &s->config.cols); + qemu_get_be16s(f, &s->config.rows); + s->config.nr_ports = qemu_get_be32(f); + + /* Items in struct VirtIOSerial */ + qemu_get_be32s(f, &s->guest_features); + + qemu_get_be32s(f, &nr_active_ports); + + /* Items in struct VirtIOSerialPort */ + for (i = 0; i < nr_active_ports; i++) { + VirtIOSerialPortBuffer *buf; + uint32_t id; + unsigned int nr_bufs; + + id = qemu_get_be32(f); + port = find_port_by_id(s, id); + + port->byte_limit = qemu_get_be64(f); + port->nr_bytes = qemu_get_be64(f); + port->guest_connected = qemu_get_byte(f); + port->host_throttled = qemu_get_byte(f); + + /* All the pending buffers from active ports */ + qemu_get_be32s(f, &nr_bufs); + if (!nr_bufs) { + continue; + } + for (; nr_bufs; nr_bufs--) { + buf = qemu_malloc(sizeof(*buf)); + + qemu_get_be64s(f, &buf->len); + qemu_get_be32s(f, &buf->flags); + buf->buf = qemu_malloc(buf->len); + qemu_get_buffer(f, buf->buf, buf->len); + QTAILQ_INSERT_TAIL(&port->unflushed_buffer_head, buf, next); + } + /* + * Applications might have internal state they track when they + * receive a guest_open() callback. Apps could also open a + * connection to this core in that case. So if the guest was + * open at the time of migration, send a guest_open callback + */ + if (port->guest_connected && port->info->guest_open) { + port->info->guest_open(port); + } + } + return 0; +} + +static void virtser_bus_print(Monitor *mon, DeviceState *qdev, int indent); + +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), + DEFINE_PROP_END_OF_LIST(), + }, +}; + +static VirtIOSerialBus *virtser_bus_new(DeviceState *dev) +{ + VirtIOSerialBus *bus; + + bus = FROM_QBUS(VirtIOSerialBus, qbus_create(&virtser_bus_info, dev, NULL)); + bus->qbus.allow_hotplug = 1; + + return bus; +} + +static void virtser_bus_print(Monitor *mon, DeviceState *qdev, int indent) +{ + VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev); + VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev); + + monitor_printf(mon, "%*s dev-prop-int: id: %u\n", + indent, "", port->id); + monitor_printf(mon, "%*s dev-prop-int: is_console: %d\n", + indent, "", port->is_console); + monitor_printf(mon, "%*s dev-prop-int: guest_connected: %d\n", + indent, "", port->guest_connected); + monitor_printf(mon, "%*s dev-prop-int: host_connected: %d\n", + indent, "", port->host_connected); + monitor_printf(mon, "%*s dev-prop-int: host_throttled: %d\n", + indent, "", port->host_throttled); + monitor_printf(mon, "%*s dev-prop-int: nr_bytes: %zu\n", + indent, "", port->nr_bytes); +} + +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 */ + if (port->vser->config.nr_ports > bus->max_nr_ports) { + qemu_error("virtio-serial-bus: Maximum device limit reached\n"); + return -1; + } + dev->info = info; + + ret = info->init(dev); + if (ret) { + return ret; + } + QTAILQ_INIT(&port->unflushed_buffer_head); + pthread_mutex_init(&port->buffer_list_lock, NULL); + + if (port->is_console && !find_port_by_id(port->vser, 0)) { + /* + * 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++; + } + QTAILQ_INSERT_TAIL(&port->vser->ports_head, port, next); + port->ivq = port->vser->ivqs[port->id]; + port->ovq = port->vser->ovqs[port->id]; + + /* Send an update to the guest about this new port added */ + virtio_notify_config(&port->vser->vdev); + + return ret; +} + +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 + * 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. + */ + QTAILQ_REMOVE(&vser->ports_head, port, next); + + if (port->info->exit) + port->info->exit(dev); + + remove_port_buffers(port); + pthread_mutex_destroy(&port->buffer_list_lock); + + return 0; +} + +void virtio_serial_port_qdev_register(VirtIOSerialPortInfo *info) +{ + info->qdev.init = virtser_port_qdev_init; + info->qdev.bus_info = &virtser_bus_info; + info->qdev.exit = virtser_port_qdev_exit; + info->qdev.unplug = qdev_simple_unplug_cb; + qdev_register(&info->qdev); +} + +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; + 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; + + vser->vdev.get_features = get_features; + vser->vdev.set_features = set_features; + vser->vdev.get_config = get_config; + vser->vdev.set_config = set_config; + + /* + * Register for the savevm section with the virtio-console name + * to preserve backward compat + */ + register_savevm("virtio-console", -1, 2, virtio_serial_save, + virtio_serial_load, vser); + + return vdev; +} diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h new file mode 100644 index 0000000..6cdaa5b --- /dev/null +++ b/hw/virtio-serial.h @@ -0,0 +1,230 @@ +/* + * Virtio Serial / Console Support + * + * Copyright IBM, Corp. 2008 + * Copyright Red Hat, Inc. 2009 + * + * Authors: + * Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com> + * Amit Shah <amit.shah@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + * + */ +#ifndef _QEMU_VIRTIO_SERIAL_H +#define _QEMU_VIRTIO_SERIAL_H + +#include <pthread.h> +#include <stdbool.h> +#include "qdev.h" +#include "virtio.h" + +/* == Interface shared between the guest kernel and qemu == */ + +/* The Virtio ID for virtio console / serial ports */ +#define VIRTIO_ID_CONSOLE 3 + +/* Features supported */ +#define VIRTIO_CONSOLE_F_MULTIPORT 1 + +struct virtio_console_config { + /* + * These two fields are used by VIRTIO_CONSOLE_F_SIZE which + * isn't implemented here yet + */ + uint16_t cols; + uint16_t rows; + + uint32_t max_nr_ports; + uint32_t nr_ports; +} __attribute__((packed)); + +struct virtio_console_control { + uint32_t id; /* Port number */ + uint16_t event; /* The kind of control event (see below) */ + uint16_t value; /* Extra information for the key */ +}; + +struct virtio_console_header { + uint32_t flags; /* Some message between host and guest */ +}; + +/* Messages between host and guest */ +#define VIRTIO_CONSOLE_HDR_START_DATA (1 << 0) +#define VIRTIO_CONSOLE_HDR_END_DATA (1 << 1) + +/* Some events for the internal messages (control packets) */ +#define VIRTIO_CONSOLE_PORT_READY 0 +#define VIRTIO_CONSOLE_CONSOLE_PORT 1 +#define VIRTIO_CONSOLE_RESIZE 2 +#define VIRTIO_CONSOLE_PORT_OPEN 3 +#define VIRTIO_CONSOLE_PORT_NAME 4 +#define VIRTIO_CONSOLE_THROTTLE_PORT 5 +#define VIRTIO_CONSOLE_CACHE_BUFFERS 6 +#define VIRTIO_CONSOLE_PORT_REMOVE 7 + +/* == In-qemu interface == */ + +typedef struct VirtIOSerial VirtIOSerial; +typedef struct VirtIOSerialBus VirtIOSerialBus; +typedef struct VirtIOSerialPort VirtIOSerialPort; +typedef struct VirtIOSerialPortInfo VirtIOSerialPortInfo; + +typedef struct VirtIOSerialDevice { + DeviceState qdev; + VirtIOSerialPortInfo *info; +} VirtIOSerialDevice; + +/* + * This is the state that's shared between all the ports. Some of the + * state is configurable via command-line options. Some of it can be + * set by individual devices in their initfn routines. Some of the + * state is set by the generic qdev device init routine. + */ +struct VirtIOSerialPort { + DeviceState dev; + VirtIOSerialPortInfo *info; + + QTAILQ_ENTRY(VirtIOSerialPort) next; + + /* + * This field gives us the virtio device as well as the qdev bus + * that we are associated with + */ + VirtIOSerial *vser; + + VirtQueue *ivq, *ovq; + + /* + * This name is sent to the guest and exported via sysfs. + * The guest could create symlinks based on this information. + * The name is in the reverse fqdn format, like org.qemu.console.0 + */ + char *name; + + /* + * This list holds buffers pushed by the guest in case the guest + * sent incomplete messages or the host connection was down and + * the device requested to cache the data. + */ + QTAILQ_HEAD(, VirtIOSerialPortBuffer) unflushed_buffer_head; + + /* + * This lock protects the unflushed_buffer_head list + */ + pthread_mutex_t buffer_list_lock; + + /* + * This id helps identify ports between the guest and the host. + * The guest sends a "header" with this id with each data packet + * that it sends and the host can then find out which associated + * device to send out this data to + */ + uint32_t id; + + /* + * Each port can specify the limit on number of bytes that can be + * outstanding in the unread buffers. This is to prevent any OOM + * situtation if a rogue process on the guest keeps injecting + * data. + */ + size_t byte_limit; + + /* + * The number of bytes we have queued up in our unread queue + */ + size_t nr_bytes; + + /* + * This boolean, when set, means "queue data that gets sent to + * this port when the host is not connected". The queued data, if + * any, is then sent out to the port when the host connection is + * opened. + */ + int cache_buffers; + + /* Identify if this is a port that binds with hvc in the guest */ + bool is_console; + + /* Is the corresponding guest device open? */ + bool guest_connected; + /* Is this device open for IO on the host? */ + bool host_connected; + /* Have we sent a throttle message to the guest? */ + bool host_throttled; + + /* Did this port get data in the recent handle_output call? */ + bool has_activity; +}; + + +struct VirtIOSerialPortInfo { + DeviceInfo qdev; + /* + * The per-port (or per-app) init function that's called when a + * new device is found on the bus. + */ + int (*init)(VirtIOSerialDevice *dev); + /* + * Per-port exit function that's called when a port gets + * hot-unplugged or removed + */ + int (*exit)(VirtIOSerialDevice *dev); + + /* Callbacks for guest events */ + /* + * Guest opened device. This could be invoked even when an + * application thinks the guest is open. This can happen if + * the host is migrated to another machine when the connection + * was open and is called from the destination machine if + * there's any app-specific initialisation to be done in such + * a case. + */ + void (*guest_open)(VirtIOSerialPort *port); + /* Guest closed device */ + void (*guest_close)(VirtIOSerialPort *port); + + /* Guest is now ready to accept data (virtqueues set up) */ + void (*guest_ready)(VirtIOSerialPort *port); + /* + * Guest wrote some data to the port. This data is handed over to + * the app via this callback. The data is not maintained anymore + * after the callback returns. This means the app has to ensure it + * read all the data and consumes it. + * + * If an app needs to have the data for a longer time, it's upto + * the app to cache it. + */ + ssize_t (*have_data)(VirtIOSerialPort *port, const uint8_t *buf, size_t len); +}; + +/* Interface to the virtio-serial bus */ + +/* + * Individual ports/apps should call this function to register the port + * with the virtio-serial bus + */ +void virtio_serial_port_qdev_register(VirtIOSerialPortInfo *info); +/* + * Open a connection to the port + * Returns 0 on success (always) + */ +int virtio_serial_open(VirtIOSerialPort *port); +/* + * 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. + */ +int virtio_serial_close(VirtIOSerialPort *port); +/* + * Send data to Guest + */ +ssize_t virtio_serial_write(VirtIOSerialPort *port, const uint8_t *buf, + size_t size); +/* + * Query whether a guest is ready to receive data. + */ +size_t virtio_serial_guest_ready(VirtIOSerialPort *port); + +#endif diff --git a/hw/virtio.h b/hw/virtio.h index 769f540..a5d3147 100644 --- a/hw/virtio.h +++ b/hw/virtio.h @@ -171,7 +171,7 @@ void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding, /* Base devices. */ VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo); VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf); -VirtIODevice *virtio_console_init(DeviceState *dev); +VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports); VirtIODevice *virtio_balloon_init(DeviceState *dev); void virtio_net_exit(VirtIODevice *vdev); diff --git a/qemu-options.hx b/qemu-options.hx index b8cc375..183b616 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1873,6 +1873,10 @@ DEF("virtioconsole", HAS_ARG, QEMU_OPTION_virtiocon, \ STEXI @item -virtioconsole @var{c} Set virtio console. + +This option is maintained for backward compatibility. + +Please use @code{-device virtconsole} for the new way of invocation. ETEXI DEF("show-cursor", 0, QEMU_OPTION_show_cursor, \ diff --git a/sysemu.h b/sysemu.h index 9d80bb2..9c3b281 100644 --- a/sysemu.h +++ b/sysemu.h @@ -231,12 +231,6 @@ extern CharDriverState *serial_hds[MAX_SERIAL_PORTS]; extern CharDriverState *parallel_hds[MAX_PARALLEL_PORTS]; -/* virtio consoles */ - -#define MAX_VIRTIO_CONSOLES 1 - -extern CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES]; - #define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR) #ifdef HAS_AUDIO diff --git a/vl.c b/vl.c index e606903..523022b 100644 --- a/vl.c +++ b/vl.c @@ -173,6 +173,8 @@ int main(int argc, char **argv) #define DEFAULT_RAM_SIZE 128 +#define MAX_VIRTIO_CONSOLES 1 + static const char *data_dir; const char *bios_name = NULL; /* Note: drives_table[MAX_DRIVES] is a dummy block driver if none available @@ -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]) { @@ -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); + if (!display_state) dumb_display_init(); /* just use the first displaystate for the moment */
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 <amit.shah@redhat.com> --- 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