* [Qemu-devel] [PATCH 0/8] virtio-console: Move to qdev, multiple devices, generic ports
@ 2010-01-07 7:31 Amit Shah
2010-01-07 7:31 ` [Qemu-devel] [PATCH 1/8] virtio: Remove duplicate macro definition for max. virtqueues, bump up the max Amit Shah
0 siblings, 1 reply; 33+ messages in thread
From: Amit Shah @ 2010-01-07 7:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah
Hello,
This series addresses comments from last time:
- use ldl/stl instead of communicating in little endian.
- virtio-serial-bus is back in Makefile.target as a result of the above
change.
- don't write to guest memory in handle_control_message
- is_console is a property; remove from custom debug output.
- reword FIXME comment for serial chardevs in qdev.c
- no need to maintain copy of guest_features
And a couple of other changes/bugfixes:
- Set serial ports to noncaching behaviour by default
- Ensure console input works in case of older kernel module
(rebase artifact)
Obligatory disclaimer:
This series splits up the patches by functionality. Note, however,
that patches 2-6 introduce some functionality that's advertised to the
guest as having to work all at once or not at all. Also, the savevm
version is bumped only once but save/restore state is added in each of
the patches. They are split only for easier reviewability.
The older virtio-console.c file is completely removed and a new
virtio-serial.c is introduced so that reviewing is easier. I can send a
later patch to rename virtio-serial.c back to virtio-console.c.
Amit Shah (8):
virtio: Remove duplicate macro definition for max. virtqueues, bump
up the max
virtio-console: qdev conversion, new virtio-serial-bus
virtio-serial-bus: Maintain guest and host port open/close state
virtio-serial-bus: Add a port 'name' property for port discovery in
guests
virtio-serial-bus: Add support for buffering guest output, throttling
guests
virtio-serial-bus: Add ability to hot-unplug ports
virtio-serial: Add a 'virtserialport' device for generic serial port
support
Move virtio-serial to Makefile.hw
Makefile.hw | 2 +-
Makefile.target | 2 +-
hw/pc.c | 11 +-
hw/ppc440_bamboo.c | 7 -
hw/qdev.c | 10 +-
hw/s390-virtio-bus.c | 17 +-
hw/s390-virtio-bus.h | 2 +
hw/s390-virtio.c | 8 -
hw/virtio-console.c | 143 --------
hw/virtio-console.h | 19 -
hw/virtio-pci.c | 13 +-
hw/virtio-serial-bus.c | 952 ++++++++++++++++++++++++++++++++++++++++++++++++
hw/virtio-serial.c | 151 ++++++++
hw/virtio-serial.h | 227 ++++++++++++
hw/virtio.c | 2 -
hw/virtio.h | 4 +-
qemu-options.hx | 4 +
sysemu.h | 6 -
vl.c | 17 +-
19 files changed, 1378 insertions(+), 219 deletions(-)
delete mode 100644 hw/virtio-console.c
delete mode 100644 hw/virtio-console.h
create mode 100644 hw/virtio-serial-bus.c
create mode 100644 hw/virtio-serial.c
create mode 100644 hw/virtio-serial.h
^ permalink raw reply [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH 1/8] virtio: Remove duplicate macro definition for max. virtqueues, bump up the max 2010-01-07 7:31 [Qemu-devel] [PATCH 0/8] virtio-console: Move to qdev, multiple devices, generic ports Amit Shah @ 2010-01-07 7:31 ` Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 2/8] virtio-console: qdev conversion, new virtio-serial-bus Amit Shah 2010-01-11 19:57 ` [Qemu-devel] [PATCH 1/8] virtio: Remove duplicate macro definition for max. virtqueues, bump up the max Anthony Liguori 0 siblings, 2 replies; 33+ messages in thread From: Amit Shah @ 2010-01-07 7:31 UTC (permalink / raw) To: qemu-devel; +Cc: Amit Shah VIRTIO_PCI_QUEUE_MAX is redefined in hw/virtio.c. Let's just keep it in hw/virtio.h. Also, bump up the value of the maximum allowed virtqueues to 64. This is in preparation to allow multiple ports per virtio-console device. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- hw/virtio.c | 2 -- hw/virtio.h | 2 +- 2 files changed, 1 insertions(+), 3 deletions(-) diff --git a/hw/virtio.c b/hw/virtio.c index cecd0dc..88f4e78 100644 --- a/hw/virtio.c +++ b/hw/virtio.c @@ -75,8 +75,6 @@ struct VirtQueue void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq); }; -#define VIRTIO_PCI_QUEUE_MAX 16 - /* virt queue functions */ static void virtqueue_init(VirtQueue *vq) { diff --git a/hw/virtio.h b/hw/virtio.h index 35532a6..051910a 100644 --- a/hw/virtio.h +++ b/hw/virtio.h @@ -90,7 +90,7 @@ typedef struct { unsigned (*get_features)(void * opaque); } VirtIOBindings; -#define VIRTIO_PCI_QUEUE_MAX 16 +#define VIRTIO_PCI_QUEUE_MAX 64 #define VIRTIO_NO_VECTOR 0xffff -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 2/8] virtio-console: qdev conversion, new virtio-serial-bus 2010-01-07 7:31 ` [Qemu-devel] [PATCH 1/8] virtio: Remove duplicate macro definition for max. virtqueues, bump up the max Amit Shah @ 2010-01-07 7:31 ` Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 3/8] virtio-serial-bus: Maintain guest and host port open/close state Amit Shah 2010-01-11 19:57 ` [Qemu-devel] [PATCH 1/8] virtio: Remove duplicate macro definition for max. virtqueues, bump up the max Anthony Liguori 1 sibling, 1 reply; 33+ messages in thread From: Amit Shah @ 2010-01-07 7:31 UTC (permalink / raw) To: qemu-devel; +Cc: Amit Shah This commit converts the virtio-console device to create a new virtio-serial bus that can host console and generic serial ports. The file hosting this code is now called virtio-serial-bus.c. The virtio console is now a very simple qdev device that sits on the virtio-serial-bus and communicates between the bus and qemu's chardevs. This commit also includes a few changes to the virtio backing code for pci and s390 to spawn the virtio-serial bus. As a result of the qdev conversion, we get rid of a lot of legacy code. The old-style way of instantiating a virtio console using -virtioconsole ... is maintained, but the new, preferred way is to use -device virtio-serial -device virtconsole,chardev=... With this commit, multiple devices as well as multiple ports with a single device can be supported. For multiple ports support, each port gets an IO vq pair. Since the guest needs to know in advance how many vqs a particular device will need, we have to set this number as a property of the virtio-serial device and also as a config option. In addition, we also spawn a pair of control IO vqs. This is an internal channel meant for guest-host communication for things like port open/close, sending port properties over to the guest, etc. This commit is a part of a series of other commits to get the full implementation of multiport support. Future commits will add other support as well as ride on the savevm version that we bump up here. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- Makefile.target | 2 +- hw/pc.c | 11 +- hw/ppc440_bamboo.c | 7 - hw/qdev.c | 10 +- hw/s390-virtio-bus.c | 17 +- hw/s390-virtio-bus.h | 2 + hw/s390-virtio.c | 8 - hw/virtio-console.c | 143 ------------- hw/virtio-console.h | 19 -- hw/virtio-pci.c | 13 +- hw/virtio-serial-bus.c | 537 ++++++++++++++++++++++++++++++++++++++++++++++++ hw/virtio-serial.c | 107 ++++++++++ hw/virtio-serial.h | 173 ++++++++++++++++ hw/virtio.h | 2 +- qemu-options.hx | 4 + sysemu.h | 6 - vl.c | 17 ++- 17 files changed, 863 insertions(+), 215 deletions(-) delete mode 100644 hw/virtio-console.c delete mode 100644 hw/virtio-console.h create mode 100644 hw/virtio-serial-bus.c create mode 100644 hw/virtio-serial.c create mode 100644 hw/virtio-serial.h diff --git a/Makefile.target b/Makefile.target index 7c1f30c..d217f07 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.o virtio-serial-bus.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 83f8dd0..c1419b5 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -1251,15 +1251,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); } @@ -1317,7 +1308,7 @@ static QEMUMachine pc_machine_v0_10 = { .property = "class", .value = stringify(PCI_CLASS_STORAGE_OTHER), },{ - .driver = "virtio-console-pci", + .driver = "virtio-serial-pci", .property = "class", .value = stringify(PCI_CLASS_DISPLAY_OTHER), },{ 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..c643576 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++]; - } + + /* FIXME: This function needs to go away: use chardev properties! */ + 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..95c516a 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,14 @@ 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.alias = "virtio-serial", .qdev.size = sizeof(VirtIOS390Device), .qdev.props = (Property[]) { + DEFINE_PROP_UINT32("max_ports", VirtIOS390Device, max_virtserial_ports, + 31), DEFINE_PROP_END_OF_LIST(), }, }; @@ -364,7 +367,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..ad85ed3 100644 --- a/hw/s390-virtio-bus.h +++ b/hw/s390-virtio-bus.h @@ -40,6 +40,8 @@ typedef struct VirtIOS390Device { VirtIODevice *vdev; DriveInfo *dinfo; NICConf nic; + /* Max. number of ports we can have for a the virtio-serial device */ + uint32_t max_virtserial_ports; } VirtIOS390Device; typedef struct VirtIOS390Bus { diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c index 0fa6ba6..3582728 100644 --- a/hw/s390-virtio.c +++ b/hw/s390-virtio.c @@ -26,7 +26,6 @@ #include "loader.h" #include "elf.h" #include "hw/virtio.h" -#include "hw/virtio-console.h" #include "hw/sysbus.h" #include "kvm.h" @@ -207,13 +206,6 @@ static void s390_init(ram_addr_t ram_size, strlen(kernel_cmdline), 1); } - /* Create VirtIO console */ - for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) { - if (virtcon_hds[i]) { - qdev_init_nofail(qdev_create((BusState *)s390_bus, "virtio-console-s390")); - } - } - /* Create VirtIO network adapters */ for(i = 0; i < nb_nics; i++) { NICInfo *nd = &nd_table[i]; diff --git a/hw/virtio-console.c b/hw/virtio-console.c deleted file mode 100644 index 57f8f89..0000000 --- a/hw/virtio-console.c +++ /dev/null @@ -1,143 +0,0 @@ -/* - * Virtio Console Device - * - * 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. - * - */ - -#include "hw.h" -#include "qemu-char.h" -#include "virtio.h" -#include "virtio-console.h" - - -typedef struct VirtIOConsole -{ - VirtIODevice vdev; - VirtQueue *ivq, *ovq; - CharDriverState *chr; -} VirtIOConsole; - -static VirtIOConsole *to_virtio_console(VirtIODevice *vdev) -{ - return (VirtIOConsole *)vdev; -} - -static void virtio_console_handle_output(VirtIODevice *vdev, VirtQueue *vq) -{ - 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); - } -} - -static void virtio_console_handle_input(VirtIODevice *vdev, VirtQueue *vq) -{ -} - -static uint32_t virtio_console_get_features(VirtIODevice *vdev) -{ - return 0; -} - -static int vcon_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; -} - -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); -} - -static void vcon_event(void *opaque, int event) -{ - /* we will ignore any event for the time being */ -} - -static void virtio_console_save(QEMUFile *f, void *opaque) -{ - VirtIOConsole *s = opaque; - - virtio_save(&s->vdev, f); -} - -static int virtio_console_load(QEMUFile *f, void *opaque, int version_id) -{ - VirtIOConsole *s = opaque; - - if (version_id != 1) - return -EINVAL; - - virtio_load(&s->vdev, f); - return 0; -} - -VirtIODevice *virtio_console_init(DeviceState *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); - - s->chr = qdev_init_chardev(dev); - qemu_chr_add_handlers(s->chr, vcon_can_read, vcon_read, vcon_event, s); - - register_savevm("virtio-console", -1, 1, virtio_console_save, virtio_console_load, s); - - return &s->vdev; -} 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..ea236bb 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -92,6 +92,8 @@ typedef struct { uint32_t nvectors; DriveInfo *dinfo; NICConf nic; + /* Max. number of ports we can have for a the virtio-serial device */ + uint32_t max_virtserial_ports; } VirtIOPCIProxy; /* virtio device */ @@ -481,7 +483,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 +493,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 +571,15 @@ static PCIDeviceInfo virtio_info[] = { }, .qdev.reset = virtio_pci_reset, },{ - .qdev.name = "virtio-console-pci", + .qdev.name = "virtio-serial-pci", + .qdev.alias = "virtio-serial", .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, + 31), 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..7589d3b --- /dev/null +++ b/hw/virtio-serial-bus.c @@ -0,0 +1,537 @@ +/* + * 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; + + /* This is the parent device that provides the bus for ports. */ + VirtIOSerial *vser; + + /* The maximum number of ports that can ride on top of this bus */ + 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; + struct virtio_console_config config; +}; + +static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id) +{ + VirtIOSerialPort *port; + + QTAILQ_FOREACH(port, &vser->ports, 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, next) { + if (port->ivq == vq || port->ovq == vq) + return port; + } + return NULL; +} + +static bool use_multiport(VirtIOSerial *vser) +{ + return vser->vdev.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; + + 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); + return offset; +} + +static size_t send_control_msg(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; + stl_p(&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 send_control_event(VirtIOSerialPort *port, uint16_t event, + uint16_t value) +{ + struct virtio_console_control cpkt; + + stw_p(&cpkt.event, event); + stw_p(&cpkt.value, value); + + return send_control_msg(port, &cpkt, sizeof(cpkt)); +} + +/* Functions for use inside qemu to open and read from/write to ports */ +int virtio_serial_open(VirtIOSerialPort *port) +{ + return 0; +} + +int virtio_serial_close(VirtIOSerialPort *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) +{ + 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; + + header_len = use_multiport(port->vser) + ? sizeof(struct virtio_console_header) : 0; + + if (!virtio_queue_ready(vq) || + !(port->vser->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) || + virtio_queue_empty(vq)) { + return 0; + } + + size = 4096; + 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, *gcpkt; + + gcpkt = buf; + port = find_port_by_id(vser, ldl_p(&gcpkt->id)); + if (!port) + return; + + cpkt.event = lduw_p(&gcpkt->event); + cpkt.value = lduw_p(&gcpkt->value); + + 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) { + send_control_event(port, VIRTIO_CONSOLE_CONSOLE_PORT, 1); + } + /* + * 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; + } +} + +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. + */ +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; + 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; + } + 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 */ + port->info->have_data(port, elem.out_sg[0].iov_base + header_len, + elem.out_sg[0].iov_len - header_len); + + next_buf: + virtqueue_push(vq, &elem, elem.out_sg[0].iov_len); + } + virtio_notify(vdev, vq); +} + +static void handle_input(VirtIODevice *vdev, VirtQueue *vq) +{ +} + +static uint32_t get_features(VirtIODevice *vdev) +{ + return 1 << VIRTIO_CONSOLE_F_MULTIPORT; +} + +/* 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; + + /* 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); +} + +static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) +{ + VirtIOSerial *s = opaque; + + 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); + + return 0; +} + +static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent); + +static struct BusInfo virtser_bus_info = { + .name = "virtio-serial-bus", + .size = sizeof(VirtIOSerialBus), + .print_dev = virtser_bus_dev_print, +}; + +static VirtIOSerialBus *virtser_bus_new(DeviceState *dev) +{ + VirtIOSerialBus *bus; + + bus = FROM_QBUS(VirtIOSerialBus, qbus_create(&virtser_bus_info, dev, + "virtio-serial-bus")); + bus->qbus.allow_hotplug = 1; + + return bus; +} + +static void virtser_bus_dev_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); +} + +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; + bool plugging_port0; + + port->vser = bus->vser; + + /* + * Is the first console port we're seeing? If so, put it up at + * location 0. This is done for backward compatibility (old + * kernel, new qemu). + */ + plugging_port0 = port->is_console && !find_port_by_id(port->vser, 0); + + if (port->vser->config.nr_ports == bus->max_nr_ports && !plugging_port0) { + qemu_error("virtio-serial-bus: Maximum device limit reached\n"); + return -1; + } + dev->info = info; + + ret = info->init(dev); + if (ret) { + return ret; + } + + port->id = plugging_port0 ? 0 : port->vser->config.nr_ports++; + + QTAILQ_INSERT_TAIL(&port->vser->ports, 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) +{ + VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev); + VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev); + VirtIOSerial *vser = port->vser; + + /* + * 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. + * + * When such a functionality is desired, a control message to add + * a port can be introduced. + */ + QTAILQ_REMOVE(&vser->ports, port, next); + + if (port->info->exit) + port->info->exit(dev); + + 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); + + 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.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.c b/hw/virtio-serial.c new file mode 100644 index 0000000..9d003f8 --- /dev/null +++ b/hw/virtio-serial.c @@ -0,0 +1,107 @@ +/* + * Virtio Console and Generic Serial 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 virtconsole_initfn(VirtIOSerialDevice *dev) +{ + VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev); + VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); + + port->info = dev->info; + + port->is_console = true; + + if (vcon->chr) { + qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event, + vcon); + } + return 0; +} + +static int virtconsole_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 virtconsole_info = { + .qdev.name = "virtconsole", + .qdev.size = sizeof(VirtConsole), + .init = virtconsole_initfn, + .exit = virtconsole_exitfn, + .have_data = flush_buf, + .qdev.props = (Property[]) { + DEFINE_PROP_UINT8("is_console", VirtConsole, port.is_console, 1), + DEFINE_PROP_CHR("chardev", VirtConsole, chr), + DEFINE_PROP_END_OF_LIST(), + }, +}; + +static void virtconsole_register(void) +{ + virtio_serial_port_qdev_register(&virtconsole_info); +} +device_init(virtconsole_register) diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h new file mode 100644 index 0000000..e604abc --- /dev/null +++ b/hw/virtio-serial.h @@ -0,0 +1,173 @@ +/* + * 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 <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 */ +}; + +/* 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 + +/* == 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 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; + + /* Identify if this is a port that binds with hvc in the guest */ + uint8_t is_console; +}; + +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 success (always). + */ +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 051910a..a574928 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 ecd50eb..9f77e4f 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 e881e45..5a0ccbf 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 @@ -286,8 +288,9 @@ static struct { { .driver = "isa-parallel", .flag = &default_parallel }, { .driver = "isa-fdc", .flag = &default_floppy }, { .driver = "ide-drive", .flag = &default_cdrom }, - { .driver = "virtio-console-pci", .flag = &default_virtcon }, - { .driver = "virtio-console-s390", .flag = &default_virtcon }, + { .driver = "virtio-serial-pci", .flag = &default_virtcon }, + { .driver = "virtio-serial-s390", .flag = &default_virtcon }, + { .driver = "virtio-serial", .flag = &default_virtcon }, { .driver = "VGA", .flag = &default_vga }, { .driver = "cirrus-vga", .flag = &default_vga }, { .driver = "vmware-svga", .flag = &default_vga }, @@ -4827,6 +4830,7 @@ static int virtcon_parse(const char *devname) { static int index = 0; char label[32]; + QemuOpts *bus_opts, *dev_opts; if (strcmp(devname, "none") == 0) return 0; @@ -4834,6 +4838,13 @@ static int virtcon_parse(const char *devname) fprintf(stderr, "qemu: too many virtio consoles\n"); exit(1); } + + bus_opts = qemu_opts_create(&qemu_device_opts, NULL, 0); + qemu_opt_set(bus_opts, "driver", "virtio-serial"); + + dev_opts = qemu_opts_create(&qemu_device_opts, NULL, 0); + qemu_opt_set(dev_opts, "driver", "virtconsole"); + snprintf(label, sizeof(label), "virtcon%d", index); virtcon_hds[index] = qemu_chr_open(label, devname, NULL); if (!virtcon_hds[index]) { @@ -4841,6 +4852,8 @@ static int virtcon_parse(const char *devname) devname, strerror(errno)); return -1; } + qemu_opt_set(dev_opts, "chardev", label); + index++; return 0; } -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 3/8] virtio-serial-bus: Maintain guest and host port open/close state 2010-01-07 7:31 ` [Qemu-devel] [PATCH 2/8] virtio-console: qdev conversion, new virtio-serial-bus Amit Shah @ 2010-01-07 7:31 ` Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 4/8] virtio-serial-bus: Add a port 'name' property for port discovery in guests Amit Shah 0 siblings, 1 reply; 33+ messages in thread From: Amit Shah @ 2010-01-07 7:31 UTC (permalink / raw) To: qemu-devel; +Cc: Amit Shah Via control channel messages, the guest can tell us whether a port got opened or closed. Similarly, we can also indicate to the guest of host port open/close events. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- hw/virtio-serial-bus.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ hw/virtio-serial.c | 6 +++ hw/virtio-serial.h | 6 +++ 3 files changed, 103 insertions(+), 0 deletions(-) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 7589d3b..e6b5eb7 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -160,11 +160,22 @@ static size_t send_control_event(VirtIOSerialPort *port, uint16_t event, /* Functions for use inside qemu to open and read from/write to ports */ int virtio_serial_open(VirtIOSerialPort *port) { + /* 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; + send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1); + return 0; } int virtio_serial_close(VirtIOSerialPort *port) { + port->host_connected = false; + send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0); + return 0; } @@ -172,6 +183,9 @@ int virtio_serial_close(VirtIOSerialPort *port) 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); } @@ -192,6 +206,9 @@ size_t virtio_serial_guest_ready(VirtIOSerialPort *port) virtio_queue_empty(vq)) { return 0; } + if (use_multiport(port->vser) && !port->guest_connected) { + return 0; + } size = 4096; if (virtqueue_avail_bytes(vq, size, 0)) { @@ -230,6 +247,11 @@ static void handle_control_message(VirtIOSerial *vser, void *buf) if (port->is_console) { send_control_event(port, VIRTIO_CONSOLE_CONSOLE_PORT, 1); } + + if (port->host_connected) { + send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1); + } + /* * When the guest has asked us for this information it means * the guest is all setup and has its virtqueues @@ -240,6 +262,19 @@ static void handle_control_message(VirtIOSerial *vser, void *buf) 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; } } @@ -332,6 +367,8 @@ static void set_config(VirtIODevice *vdev, const uint8_t *config_data) static void virtio_serial_save(QEMUFile *f, void *opaque) { VirtIOSerial *s = opaque; + VirtIOSerialPort *port; + uint32_t nr_active_ports; /* The virtio device */ virtio_save(&s->vdev, f); @@ -340,15 +377,42 @@ static void virtio_serial_save(QEMUFile *f, void *opaque) 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 */ + + /* Do this because we might have hot-unplugged some ports */ + nr_active_ports = 0; + QTAILQ_FOREACH(port, &s->ports, next) + nr_active_ports++; + + qemu_put_be32s(f, &nr_active_ports); + + /* + * Items in struct VirtIOSerialPort. + */ + QTAILQ_FOREACH(port, &s->ports, next) { + /* + * 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_byte(f, port->guest_connected); + + } } 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); @@ -361,6 +425,21 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) qemu_get_be16s(f, &s->config.rows); s->config.nr_ports = qemu_get_be32(f); + /* Items in struct VirtIOSerial */ + + qemu_get_be32s(f, &nr_active_ports); + + /* Items in struct VirtIOSerialPort */ + for (i = 0; i < nr_active_ports; i++) { + uint32_t id; + + id = qemu_get_be32(f); + port = find_port_by_id(s, id); + + port->guest_connected = qemu_get_byte(f); + + } + return 0; } @@ -390,6 +469,10 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent) monitor_printf(mon, "%*s dev-prop-int: id: %u\n", indent, "", port->id); + 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); } static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base) @@ -423,6 +506,14 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base) port->id = plugging_port0 ? 0 : port->vser->config.nr_ports++; + if (!use_multiport(port->vser)) { + /* + * Allow writes to guest in this case; we have no way of + * knowing if a guest port is connected. + */ + port->guest_connected = true; + } + QTAILQ_INSERT_TAIL(&port->vser->ports, port, next); port->ivq = port->vser->ivqs[port->id]; port->ovq = port->vser->ovqs[port->id]; diff --git a/hw/virtio-serial.c b/hw/virtio-serial.c index 9d003f8..d9a6f32 100644 --- a/hw/virtio-serial.c +++ b/hw/virtio-serial.c @@ -68,6 +68,12 @@ static int virtconsole_initfn(VirtIOSerialDevice *dev) port->is_console = true; + /* + * For console ports, just assume the guest is ready to accept our + * data. + */ + port->guest_connected = true; + if (vcon->chr) { qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event, vcon); diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h index e604abc..b855375 100644 --- a/hw/virtio-serial.h +++ b/hw/virtio-serial.h @@ -53,6 +53,7 @@ struct virtio_console_header { #define VIRTIO_CONSOLE_PORT_READY 0 #define VIRTIO_CONSOLE_CONSOLE_PORT 1 #define VIRTIO_CONSOLE_RESIZE 2 +#define VIRTIO_CONSOLE_PORT_OPEN 3 /* == In-qemu interface == */ @@ -96,6 +97,11 @@ struct VirtIOSerialPort { /* Identify if this is a port that binds with hvc in the guest */ uint8_t is_console; + + /* Is the corresponding guest device open? */ + bool guest_connected; + /* Is this device open for IO on the host? */ + bool host_connected; }; struct VirtIOSerialPortInfo { -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 4/8] virtio-serial-bus: Add a port 'name' property for port discovery in guests 2010-01-07 7:31 ` [Qemu-devel] [PATCH 3/8] virtio-serial-bus: Maintain guest and host port open/close state Amit Shah @ 2010-01-07 7:31 ` Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests Amit Shah 0 siblings, 1 reply; 33+ messages in thread From: Amit Shah @ 2010-01-07 7:31 UTC (permalink / raw) To: qemu-devel; +Cc: Amit Shah The port 'id' or number is internal state between the guest kernel and our bus implementation. This is invocation-dependent and isn't part of the guest-host ABI. To correcly enumerate and map ports between the host and the guest, the 'name' property is used. Example: -device virtserialport,name=org.qemu.port.0 This invocation will get us a char device in the guest at: /dev/virtio-ports/org.qemu.port.0 which can be a symlink to /dev/vport0p3 This 'name' property is exposed by the guest kernel in a sysfs attribute: /sys/kernel/virtio-ports/vport0p3/name A simple udev script can pick up this name and create the symlink mentioned above. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- hw/virtio-serial-bus.c | 17 +++++++++++++++++ hw/virtio-serial.c | 1 + hw/virtio-serial.h | 8 ++++++++ 3 files changed, 26 insertions(+), 0 deletions(-) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index e6b5eb7..20d9580 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -226,6 +226,8 @@ static void handle_control_message(VirtIOSerial *vser, void *buf) { struct VirtIOSerialPort *port; struct virtio_console_control cpkt, *gcpkt; + uint8_t *buffer; + size_t buffer_len; gcpkt = buf; port = find_port_by_id(vser, ldl_p(&gcpkt->id)); @@ -248,6 +250,21 @@ static void handle_control_message(VirtIOSerial *vser, void *buf) send_control_event(port, VIRTIO_CONSOLE_CONSOLE_PORT, 1); } + if (port->name) { + stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME); + stw_p(&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_msg(port, buffer, buffer_len); + qemu_free(buffer); + } + if (port->host_connected) { send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1); } diff --git a/hw/virtio-serial.c b/hw/virtio-serial.c index d9a6f32..470446b 100644 --- a/hw/virtio-serial.c +++ b/hw/virtio-serial.c @@ -102,6 +102,7 @@ static VirtIOSerialPortInfo virtconsole_info = { .qdev.props = (Property[]) { DEFINE_PROP_UINT8("is_console", VirtConsole, port.is_console, 1), DEFINE_PROP_CHR("chardev", VirtConsole, chr), + DEFINE_PROP_STRING("name", VirtConsole, port.name), DEFINE_PROP_END_OF_LIST(), }, }; diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h index b855375..5505841 100644 --- a/hw/virtio-serial.h +++ b/hw/virtio-serial.h @@ -54,6 +54,7 @@ struct virtio_console_header { #define VIRTIO_CONSOLE_CONSOLE_PORT 1 #define VIRTIO_CONSOLE_RESIZE 2 #define VIRTIO_CONSOLE_PORT_OPEN 3 +#define VIRTIO_CONSOLE_PORT_NAME 4 /* == In-qemu interface == */ @@ -88,6 +89,13 @@ struct VirtIOSerialPort { 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 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 -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-07 7:31 ` [Qemu-devel] [PATCH 4/8] virtio-serial-bus: Add a port 'name' property for port discovery in guests Amit Shah @ 2010-01-07 7:31 ` Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 6/8] virtio-serial-bus: Add ability to hot-unplug ports Amit Shah 2010-01-08 1:12 ` [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests Jamie Lokier 0 siblings, 2 replies; 33+ messages in thread From: Amit Shah @ 2010-01-07 7:31 UTC (permalink / raw) To: qemu-devel; +Cc: Amit Shah Guests send us one buffer at a time. Current guests send buffers sized 4K bytes. If guest userspace applications sent out > 4K bytes in one write() syscall, the write request actually sends out multiple buffers, each of 4K in size. This usually isn't a problem but for some apps, like VNC, the entire data has to be sent in one go to make copy/paste work fine. So if an app on the guest sends out guest clipboard contents, it has to be sent to the vnc server in one go as the guest app sent it. For this to be done, we need the guest to send us START and END markers for each write request so that we can find out complete buffers and send them off to ports. This needs us to buffer all the data that comes in from the guests, hold it off till we see all the data corresponding to one write request, merge it all in one buffer and then send it to the port the data was destined for. Also, we add support for caching of these buffers till a port indicates it's ready to receive data. We keep caching data the guest sends us till a port accepts it. However, this could lead to an OOM condition where a rogue process on the guest could continue pumping in data while the host continues to cache it. We introduce a per-port byte-limit property to alleviate this condition. When this limit is reached, we send a control message to the guest indicating it to not send us any more data till further indication. When the number of bytes cached go lesser than the limit specified, we open tell the guest to restart sending data. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- hw/virtio-serial-bus.c | 309 +++++++++++++++++++++++++++++++++++++++++++++++- hw/virtio-serial.c | 11 +- hw/virtio-serial.h | 39 ++++++ 3 files changed, 352 insertions(+), 7 deletions(-) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 20d9580..c947143 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -44,6 +44,20 @@ struct VirtIOSerial { struct virtio_console_config config; }; +/* 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; @@ -157,6 +171,198 @@ static size_t send_control_event(VirtIOSerialPort *port, uint16_t event, return send_control_msg(port, &cpkt, sizeof(cpkt)); } +static void init_buf(VirtIOSerialPortBuffer *buf, uint8_t *buffer, size_t len) +{ + buf->buf = buffer; + buf->len = len; + buf->offset = 0; + buf->flags = 0; + buf->previous_failure = false; +} + +static VirtIOSerialPortBuffer *alloc_buf(size_t len) +{ + VirtIOSerialPortBuffer *buf; + + buf = qemu_malloc(sizeof(*buf)); + buf->buf = qemu_malloc(len); + + init_buf(buf, buf->buf, len); + + return buf; +} + +static void free_buf(VirtIOSerialPortBuffer *buf) +{ + qemu_free(buf->buf); + qemu_free(buf); +} + +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_buffers, next) { + size += buf->len - buf->offset; + + if (buf->flags & VIRTIO_CONSOLE_HDR_END_DATA) { + is_complete = true; + break; + } + if (buf == QTAILQ_FIRST(&port->unflushed_buffers) + && !(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_buffers); + if (buf->len - buf->offset == out_size) { + QTAILQ_REMOVE(&port->unflushed_buffers, buf, next); + return buf; + } + out_offset = 0; + outbuf = qemu_malloc(out_size); + + QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffers, 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_buffers, buf, next); + qemu_free(buf->buf); + + if (out_offset == out_size) { + break; + } + qemu_free(buf); + } + init_buf(buf, outbuf, out_size); + buf->flags = VIRTIO_CONSOLE_HDR_START_DATA | VIRTIO_CONSOLE_HDR_END_DATA; + + return buf; +} + +/* Call with the unflushed_buffers_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; + free_buf(buf); + continue; + } + + ret = port->info->have_data(port, buf->buf + buf->offset, out_size); + if (ret < out_size) { + QTAILQ_INSERT_HEAD(&port->unflushed_buffers, 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)) { + free_buf(buf); + } + } + + if (port->host_throttled && port->nr_bytes < port->byte_limit) { + port->host_throttled = false; + send_control_event(port, VIRTIO_CONSOLE_THROTTLE_PORT, 0); + } +} + +static void flush_all_ports(VirtIOSerial *vser) +{ + struct VirtIOSerialPort *port; + + QTAILQ_FOREACH(port, &vser->ports, next) { + if (port->has_activity) { + port->has_activity = false; + flush_queue(port); + } + } +} + +static void remove_port_buffers(VirtIOSerialPort *port) +{ + struct VirtIOSerialPortBuffer *buf, *buf2; + + QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffers, next, buf2) { + QTAILQ_REMOVE(&port->unflushed_buffers, buf, next); + free_buf(buf); + } +} + /* Functions for use inside qemu to open and read from/write to ports */ int virtio_serial_open(VirtIOSerialPort *port) { @@ -168,6 +374,10 @@ int virtio_serial_open(VirtIOSerialPort *port) port->host_connected = true; send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1); + /* Flush any buffers that were cached while the port was closed */ + if (port->cache_buffers && port->info->have_data) { + flush_queue(port); + } return 0; } @@ -176,6 +386,9 @@ int virtio_serial_close(VirtIOSerialPort *port) port->host_connected = false; send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0); + if (!port->cache_buffers) { + remove_port_buffers(port); + } return 0; } @@ -265,6 +478,14 @@ static void handle_control_message(VirtIOSerial *vser, void *buf) 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) { + send_control_event(port, VIRTIO_CONSOLE_CACHE_BUFFERS, 1); + } + if (port->host_connected) { send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1); } @@ -315,6 +536,10 @@ static void control_out(VirtIODevice *vdev, VirtQueue *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) { @@ -325,6 +550,7 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq) while (virtqueue_pop(vq, &elem)) { VirtIOSerialPort *port; + VirtIOSerialPortBuffer *buf; struct virtio_console_header header; int header_len; @@ -333,10 +559,14 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq) 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 @@ -347,13 +577,38 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq) } /* The guest always sends only one sg */ - port->info->have_data(port, elem.out_sg[0].iov_base + header_len, - elem.out_sg[0].iov_len - header_len); + buf = alloc_buf(elem.out_sg[0].iov_len - header_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 = ldl_p(&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; + } + + QTAILQ_INSERT_TAIL(&port->unflushed_buffers, buf, next); + port->nr_bytes += buf->len; + port->has_activity = true; + if (!port->host_throttled && port->byte_limit && + port->nr_bytes >= port->byte_limit) { + + port->host_throttled = true; + send_control_event(port, VIRTIO_CONSOLE_THROTTLE_PORT, 1); + } 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) @@ -386,6 +641,7 @@ 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); @@ -408,14 +664,35 @@ static void virtio_serial_save(QEMUFile *f, void *opaque) * Items in struct VirtIOSerialPort. */ QTAILQ_FOREACH(port, &s->ports, 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_buffers, next) { + nr_bufs++; + } + qemu_put_be32s(f, &nr_bufs); + if (!nr_bufs) { + continue; + } + QTAILQ_FOREACH(buf, &port->unflushed_buffers, next) { + qemu_put_be64s(f, &buf->len); + qemu_put_be64s(f, &buf->offset); + qemu_put_be32s(f, &buf->flags); + qemu_put_buffer(f, buf->buf, buf->len); + } } } @@ -448,13 +725,34 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) /* 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--) { + size_t len; + qemu_get_be64s(f, &len); + buf = alloc_buf(len); + + qemu_get_be64s(f, &buf->offset); + qemu_get_be32s(f, &buf->flags); + qemu_get_buffer(f, buf->buf, buf->len); + QTAILQ_INSERT_TAIL(&port->unflushed_buffers, buf, next); + } } return 0; @@ -490,6 +788,10 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent) 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) @@ -520,6 +822,7 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base) if (ret) { return ret; } + QTAILQ_INIT(&port->unflushed_buffers); port->id = plugging_port0 ? 0 : port->vser->config.nr_ports++; @@ -570,6 +873,8 @@ static int virtser_port_qdev_exit(DeviceState *qdev) if (port->info->exit) port->info->exit(dev); + remove_port_buffers(port); + return 0; } diff --git a/hw/virtio-serial.c b/hw/virtio-serial.c index 470446b..fd27c33 100644 --- a/hw/virtio-serial.c +++ b/hw/virtio-serial.c @@ -66,13 +66,14 @@ static int virtconsole_initfn(VirtIOSerialDevice *dev) port->info = dev->info; - port->is_console = true; - /* - * For console ports, just assume the guest is ready to accept our - * data. + * 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->guest_connected = true; + port->cache_buffers = 0; + + port->is_console = true; if (vcon->chr) { qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event, diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h index 5505841..acb601d 100644 --- a/hw/virtio-serial.h +++ b/hw/virtio-serial.h @@ -49,12 +49,18 @@ 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 /* == In-qemu interface == */ @@ -96,6 +102,13 @@ struct VirtIOSerialPort { 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_buffers; + + /* * 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 @@ -103,6 +116,27 @@ struct VirtIOSerialPort { */ 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. + */ + uint8_t cache_buffers; + /* Identify if this is a port that binds with hvc in the guest */ uint8_t is_console; @@ -110,6 +144,11 @@ struct VirtIOSerialPort { 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 { -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 6/8] virtio-serial-bus: Add ability to hot-unplug ports 2010-01-07 7:31 ` [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests Amit Shah @ 2010-01-07 7:31 ` Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 7/8] virtio-serial: Add a 'virtserialport' device for generic serial port support Amit Shah 2010-01-08 1:12 ` [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests Jamie Lokier 1 sibling, 1 reply; 33+ messages in thread From: Amit Shah @ 2010-01-07 7:31 UTC (permalink / raw) To: qemu-devel; +Cc: Amit Shah Signed-off-by: Amit Shah <amit.shah@redhat.com> --- hw/virtio-serial-bus.c | 2 ++ hw/virtio-serial.h | 1 + 2 files changed, 3 insertions(+), 0 deletions(-) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index c947143..6b3273d 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -850,6 +850,8 @@ static int virtser_port_qdev_exit(DeviceState *qdev) VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev); VirtIOSerial *vser = port->vser; + send_control_event(port, VIRTIO_CONSOLE_PORT_REMOVE, 1); + /* * Don't decrement nr_ports here; thus we keep a linearly * increasing port id. Not utilising an id again saves us a couple diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h index acb601d..13f6989 100644 --- a/hw/virtio-serial.h +++ b/hw/virtio-serial.h @@ -61,6 +61,7 @@ struct virtio_console_header { #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 == */ -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 7/8] virtio-serial: Add a 'virtserialport' device for generic serial port support 2010-01-07 7:31 ` [Qemu-devel] [PATCH 6/8] virtio-serial-bus: Add ability to hot-unplug ports Amit Shah @ 2010-01-07 7:31 ` Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 8/8] Move virtio-serial to Makefile.hw Amit Shah 0 siblings, 1 reply; 33+ messages in thread From: Amit Shah @ 2010-01-07 7:31 UTC (permalink / raw) To: qemu-devel; +Cc: Amit Shah This patch adds generic serial ports over the virtio serial bus. These ports have a few more options that are not relevant for virtio console ports: the ability to cache buffers that are received for a port while it's disconnected, setting of limits to the bytes that are cached so as to prevent OOM conditions, etc. Sample uses for such a device can be obtaining info from the guest like the file systems used, apps installed, etc. for offline usage and logged-in users, clipboard copy-paste, etc. for online usage. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- hw/virtio-serial.c | 36 ++++++++++++++++++++++++++++++++++++ 1 files changed, 36 insertions(+), 0 deletions(-) diff --git a/hw/virtio-serial.c b/hw/virtio-serial.c index fd27c33..44d8a72 100644 --- a/hw/virtio-serial.c +++ b/hw/virtio-serial.c @@ -113,3 +113,39 @@ static void virtconsole_register(void) virtio_serial_port_qdev_register(&virtconsole_info); } device_init(virtconsole_register) + +/* Generic Virtio Serial Ports */ +static int virtserialport_initfn(VirtIOSerialDevice *dev) +{ + VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev); + VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); + + port->info = dev->info; + + if (vcon->chr) { + qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event, + vcon); + } + return 0; +} + +static VirtIOSerialPortInfo virtserialport_info = { + .qdev.name = "virtserialport", + .qdev.size = sizeof(VirtConsole), + .init = virtserialport_initfn, + .exit = virtconsole_exitfn, + .have_data = flush_buf, + .qdev.props = (Property[]) { + DEFINE_PROP_CHR("chardev", VirtConsole, chr), + DEFINE_PROP_STRING("name", VirtConsole, port.name), + DEFINE_PROP_UINT8("cache_buffers", VirtConsole, port.cache_buffers, 0), + DEFINE_PROP_UINT64("byte_limit", VirtConsole, port.byte_limit, 0), + DEFINE_PROP_END_OF_LIST(), + }, +}; + +static void virtserialport_register(void) +{ + virtio_serial_port_qdev_register(&virtserialport_info); +} +device_init(virtserialport_register) -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 8/8] Move virtio-serial to Makefile.hw 2010-01-07 7:31 ` [Qemu-devel] [PATCH 7/8] virtio-serial: Add a 'virtserialport' device for generic serial port support Amit Shah @ 2010-01-07 7:31 ` Amit Shah 2010-01-08 0:41 ` Andreas Färber 0 siblings, 1 reply; 33+ messages in thread From: Amit Shah @ 2010-01-07 7:31 UTC (permalink / raw) To: qemu-devel; +Cc: Amit Shah There's nothing target-dependent in the virtio-serial code so allow it to be compiled just once for all the targets. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- Makefile.hw | 2 +- Makefile.target | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile.hw b/Makefile.hw index bd252f5..b6323fa 100644 --- a/Makefile.hw +++ b/Makefile.hw @@ -13,7 +13,7 @@ QEMU_CFLAGS+=-I.. -I$(SRC_PATH)/fpu obj-y = obj-y += loader.o -obj-y += virtio.o +obj-y += virtio.o virtio-serial.o obj-y += fw_cfg.o obj-y += watchdog.o obj-$(CONFIG_ECC) += ecc.o diff --git a/Makefile.target b/Makefile.target index d217f07..475362e 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-serial.o virtio-serial-bus.o virtio-pci.o +obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-pci.o virtio-serial-bus.o obj-$(CONFIG_KVM) += kvm.o kvm-all.o obj-$(CONFIG_ISA_MMIO) += isa_mmio.o LIBS+=-lz -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 8/8] Move virtio-serial to Makefile.hw 2010-01-07 7:31 ` [Qemu-devel] [PATCH 8/8] Move virtio-serial to Makefile.hw Amit Shah @ 2010-01-08 0:41 ` Andreas Färber 2010-01-08 5:01 ` Amit Shah 0 siblings, 1 reply; 33+ messages in thread From: Andreas Färber @ 2010-01-08 0:41 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel Am 07.01.2010 um 08:31 schrieb Amit Shah: > There's nothing target-dependent in the virtio-serial code so allow it > to be compiled just once for all the targets. > > Signed-off-by: Amit Shah <amit.shah@redhat.com> > --- > Makefile.hw | 2 +- > Makefile.target | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Makefile.hw b/Makefile.hw > index bd252f5..b6323fa 100644 > --- a/Makefile.hw > +++ b/Makefile.hw > @@ -13,7 +13,7 @@ QEMU_CFLAGS+=-I.. -I$(SRC_PATH)/fpu > > obj-y = > obj-y += loader.o > -obj-y += virtio.o > +obj-y += virtio.o virtio-serial.o > obj-y += fw_cfg.o > obj-y += watchdog.o > obj-$(CONFIG_ECC) += ecc.o Needs to be rebased and applied against Makefile.objs now, s/obj-y/hw- obj-y/. Andreas > diff --git a/Makefile.target b/Makefile.target > index d217f07..475362e 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-serial.o > virtio-serial-bus.o virtio-pci.o > +obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-pci.o > virtio-serial-bus.o > obj-$(CONFIG_KVM) += kvm.o kvm-all.o > obj-$(CONFIG_ISA_MMIO) += isa_mmio.o > LIBS+=-lz > -- > 1.6.2.5 > > > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 8/8] Move virtio-serial to Makefile.hw 2010-01-08 0:41 ` Andreas Färber @ 2010-01-08 5:01 ` Amit Shah 0 siblings, 0 replies; 33+ messages in thread From: Amit Shah @ 2010-01-08 5:01 UTC (permalink / raw) To: Andreas Färber; +Cc: qemu-devel On (Fri) Jan 08 2010 [01:41:48], Andreas Färber wrote: > > Am 07.01.2010 um 08:31 schrieb Amit Shah: > >> There's nothing target-dependent in the virtio-serial code so allow it >> to be compiled just once for all the targets. >> >> Signed-off-by: Amit Shah <amit.shah@redhat.com> >> --- >> Makefile.hw | 2 +- >> Makefile.target | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/Makefile.hw b/Makefile.hw >> index bd252f5..b6323fa 100644 >> --- a/Makefile.hw >> +++ b/Makefile.hw >> @@ -13,7 +13,7 @@ QEMU_CFLAGS+=-I.. -I$(SRC_PATH)/fpu >> >> obj-y = >> obj-y += loader.o >> -obj-y += virtio.o >> +obj-y += virtio.o virtio-serial.o >> obj-y += fw_cfg.o >> obj-y += watchdog.o >> obj-$(CONFIG_ECC) += ecc.o > > Needs to be rebased and applied against Makefile.objs now, s/obj-y/hw- > obj-y/. OK, thanks for the hint. I've rebased my local branch against master. Amit ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-07 7:31 ` [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 6/8] virtio-serial-bus: Add ability to hot-unplug ports Amit Shah @ 2010-01-08 1:12 ` Jamie Lokier 2010-01-08 5:03 ` Amit Shah 1 sibling, 1 reply; 33+ messages in thread From: Jamie Lokier @ 2010-01-08 1:12 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel Amit Shah wrote: > Guests send us one buffer at a time. Current guests send buffers sized > 4K bytes. If guest userspace applications sent out > 4K bytes in one > write() syscall, the write request actually sends out multiple buffers, > each of 4K in size. > > This usually isn't a problem but for some apps, like VNC, the entire > data has to be sent in one go to make copy/paste work fine. So if an app > on the guest sends out guest clipboard contents, it has to be sent to > the vnc server in one go as the guest app sent it. > > For this to be done, we need the guest to send us START and END markers > for each write request so that we can find out complete buffers and send > them off to ports. That looks very dubious. TCP/IP doesn't maintain write boundaries; neither do pipes, unix domain sockets, pseudo-terminals, and almost every other modern byte-oriented transport. So how does VNC transmit the clipboard over TCP/IP to a VNC client, without those boundaries, and why is it different with virtserialport? -- Jamie ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-08 1:12 ` [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests Jamie Lokier @ 2010-01-08 5:03 ` Amit Shah 2010-01-08 13:35 ` Jamie Lokier 0 siblings, 1 reply; 33+ messages in thread From: Amit Shah @ 2010-01-08 5:03 UTC (permalink / raw) To: Jamie Lokier; +Cc: qemu-devel On (Fri) Jan 08 2010 [01:12:31], Jamie Lokier wrote: > Amit Shah wrote: > > Guests send us one buffer at a time. Current guests send buffers sized > > 4K bytes. If guest userspace applications sent out > 4K bytes in one > > write() syscall, the write request actually sends out multiple buffers, > > each of 4K in size. > > > > This usually isn't a problem but for some apps, like VNC, the entire > > data has to be sent in one go to make copy/paste work fine. So if an app > > on the guest sends out guest clipboard contents, it has to be sent to > > the vnc server in one go as the guest app sent it. > > > > For this to be done, we need the guest to send us START and END markers > > for each write request so that we can find out complete buffers and send > > them off to ports. > > That looks very dubious. TCP/IP doesn't maintain write boundaries; > neither do pipes, unix domain sockets, pseudo-terminals, and almost > every other modern byte-oriented transport. > > So how does VNC transmit the clipboard over TCP/IP to a VNC client, > without those boundaries, and why is it different with virtserialport? TCP does this in its stack: it waits for the number of bytes written to be received and then notifies userspace of data availibility. In this case, consider the case where the guest writes 10k of data. The guest gives us those 10k in 3 chunks: the first containing 4k (- header size), the 2nd containing the next 4k (- header size) and the 3rd chunk the remaining data. I want to flush out this data only when I get all 10k. Amit ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-08 5:03 ` Amit Shah @ 2010-01-08 13:35 ` Jamie Lokier 2010-01-08 16:26 ` Anthony Liguori 2010-01-11 8:34 ` Amit Shah 0 siblings, 2 replies; 33+ messages in thread From: Jamie Lokier @ 2010-01-08 13:35 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel Amit Shah wrote: > On (Fri) Jan 08 2010 [01:12:31], Jamie Lokier wrote: > > Amit Shah wrote: > > > Guests send us one buffer at a time. Current guests send buffers sized > > > 4K bytes. If guest userspace applications sent out > 4K bytes in one > > > write() syscall, the write request actually sends out multiple buffers, > > > each of 4K in size. > > > > > > This usually isn't a problem but for some apps, like VNC, the entire > > > data has to be sent in one go to make copy/paste work fine. So if an app > > > on the guest sends out guest clipboard contents, it has to be sent to > > > the vnc server in one go as the guest app sent it. > > > > > > For this to be done, we need the guest to send us START and END markers > > > for each write request so that we can find out complete buffers and send > > > them off to ports. > > > > That looks very dubious. TCP/IP doesn't maintain write boundaries; > > neither do pipes, unix domain sockets, pseudo-terminals, and almost > > every other modern byte-oriented transport. > > > > So how does VNC transmit the clipboard over TCP/IP to a VNC client, > > without those boundaries, and why is it different with virtserialport? > > TCP does this in its stack: it waits for the number of bytes written to > be received and then notifies userspace of data availibility. > > In this case, consider the case where the guest writes 10k of data. The > guest gives us those 10k in 3 chunks: the first containing 4k (- header > size), the 2nd containing the next 4k (- header size) and the 3rd chunk > the remaining data. > > I want to flush out this data only when I get all 10k. No, TCP does not do that. It does not maintain boundaries, or delay delivery until a full write is transmitted. Even if you use TCP_CORK (Linux specific), that is just a performance hint. If the sender writes 10k of data in a single write() over TCP, and it is split into packets size 4k/4k/2k (assume just over 4k MSS :-), the receiver will be notified of availability any time after the *first* packet is received, and the read() call may indeed return less than 10k. In fact it can be split at any byte position, depending on other activity. Applications handle this by using their own framing protocol on top of the TCP byte stream. For example a simple header saying "expect N bytes" followed by N bytes, or line delimiters or escape characters. Sometimes it looks like TCP is maintaining write boundaries, but it is just an artifact of its behaviour on many systems, and is not reliable even on those systems where it seems to happen most of the time. Even when connecting to localhost, you cannot rely on that. I have seen people write code assuming TCP keeps boundaries, and then some weeks later they are very confused debugging their code because it is not reliable... Since VNC is clearly designed to work over TCP, and is written by people who know this, I'm wondering why you think it needs to be different for virtio-serial. -- Jamie ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-08 13:35 ` Jamie Lokier @ 2010-01-08 16:26 ` Anthony Liguori 2010-01-11 8:39 ` Amit Shah 2010-01-11 8:34 ` Amit Shah 1 sibling, 1 reply; 33+ messages in thread From: Anthony Liguori @ 2010-01-08 16:26 UTC (permalink / raw) To: Jamie Lokier; +Cc: Amit Shah, qemu-devel On 01/08/2010 07:35 AM, Jamie Lokier wrote: > Sometimes it looks like TCP is maintaining write boundaries, but it is > just an artifact of its behaviour on many systems, and is not reliable > even on those systems where it seems to happen most of the time. Even > when connecting to localhost, you cannot rely on that. I have seen > people write code assuming TCP keeps boundaries, and then some weeks > later they are very confused debugging their code because it is not > reliable... > > Since VNC is clearly designed to work over TCP, and is written by > people who know this, I'm wondering why you think it needs to be > different for virtio-serial. > I'm confused about why the buffering is needed in the first place. I would think that any buffering should be pushed back to the guest. IOW, if there's available data from the char driver, but the guest doesn't have a buffer. Don't select on the char driver until the guest has a buffer available. If the guest attempts to write data but the char driver isn't ready to receive data, don't complete the request until the char driver can accept data. Where does buffering come in? Regards, Anthony Liguori > -- Jamie > > > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-08 16:26 ` Anthony Liguori @ 2010-01-11 8:39 ` Amit Shah 2010-01-12 0:28 ` Anthony Liguori 0 siblings, 1 reply; 33+ messages in thread From: Amit Shah @ 2010-01-11 8:39 UTC (permalink / raw) To: Anthony Liguori; +Cc: qemu-devel On (Fri) Jan 08 2010 [10:26:59], Anthony Liguori wrote: > On 01/08/2010 07:35 AM, Jamie Lokier wrote: >> Sometimes it looks like TCP is maintaining write boundaries, but it is >> just an artifact of its behaviour on many systems, and is not reliable >> even on those systems where it seems to happen most of the time. Even >> when connecting to localhost, you cannot rely on that. I have seen >> people write code assuming TCP keeps boundaries, and then some weeks >> later they are very confused debugging their code because it is not >> reliable... >> >> Since VNC is clearly designed to work over TCP, and is written by >> people who know this, I'm wondering why you think it needs to be >> different for virtio-serial. >> > > I'm confused about why the buffering is needed in the first place. > > I would think that any buffering should be pushed back to the guest. > IOW, if there's available data from the char driver, but the guest > doesn't have a buffer. Don't select on the char driver until the guest > has a buffer available. If the guest attempts to write data but the > char driver isn't ready to receive data, don't complete the request > until the char driver can accept data. This is a different thing from what Jamie's talking about. A guest or a host might be interested in communicating data without waiting for the other end to come up. The other end can just start consuming the data (even the data that it missed while it wasn't connected) once it's up. (I can remove this option for now and add it later, if you prefer it that way.) Amit ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-11 8:39 ` Amit Shah @ 2010-01-12 0:28 ` Anthony Liguori 2010-01-12 7:08 ` Amit Shah 0 siblings, 1 reply; 33+ messages in thread From: Anthony Liguori @ 2010-01-12 0:28 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel On 01/11/2010 02:39 AM, Amit Shah wrote: > On (Fri) Jan 08 2010 [10:26:59], Anthony Liguori wrote: > >> On 01/08/2010 07:35 AM, Jamie Lokier wrote: >> >>> Sometimes it looks like TCP is maintaining write boundaries, but it is >>> just an artifact of its behaviour on many systems, and is not reliable >>> even on those systems where it seems to happen most of the time. Even >>> when connecting to localhost, you cannot rely on that. I have seen >>> people write code assuming TCP keeps boundaries, and then some weeks >>> later they are very confused debugging their code because it is not >>> reliable... >>> >>> Since VNC is clearly designed to work over TCP, and is written by >>> people who know this, I'm wondering why you think it needs to be >>> different for virtio-serial. >>> >>> >> I'm confused about why the buffering is needed in the first place. >> >> I would think that any buffering should be pushed back to the guest. >> IOW, if there's available data from the char driver, but the guest >> doesn't have a buffer. Don't select on the char driver until the guest >> has a buffer available. If the guest attempts to write data but the >> char driver isn't ready to receive data, don't complete the request >> until the char driver can accept data. >> > This is a different thing from what Jamie's talking about. A guest or a > host might be interested in communicating data without waiting for the > other end to come up. The other end can just start consuming the data > (even the data that it missed while it wasn't connected) once it's up. > > (I can remove this option for now and add it later, if you prefer it > that way.) > If it's not needed by your use case, please remove it. Doing buffering gets tricky because you can't allow an infinite buffer for security reasons. All you end up doing is increasing the size of the buffer beyond what the guest and client are capable of doing. Since you still can lose data, apps have to be written to handle this. I think it adds complexity without a lot of benefit. Regards, Anthony Liguori > Amit > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-12 0:28 ` Anthony Liguori @ 2010-01-12 7:08 ` Amit Shah 0 siblings, 0 replies; 33+ messages in thread From: Amit Shah @ 2010-01-12 7:08 UTC (permalink / raw) To: Anthony Liguori; +Cc: qemu-devel On (Mon) Jan 11 2010 [18:28:52], Anthony Liguori wrote: >>> >>> I would think that any buffering should be pushed back to the guest. >>> IOW, if there's available data from the char driver, but the guest >>> doesn't have a buffer. Don't select on the char driver until the guest >>> has a buffer available. If the guest attempts to write data but the >>> char driver isn't ready to receive data, don't complete the request >>> until the char driver can accept data. >>> >> This is a different thing from what Jamie's talking about. A guest or a >> host might be interested in communicating data without waiting for the >> other end to come up. The other end can just start consuming the data >> (even the data that it missed while it wasn't connected) once it's up. >> >> (I can remove this option for now and add it later, if you prefer it >> that way.) >> > > If it's not needed by your use case, please remove it. Doing buffering > gets tricky because you can't allow an infinite buffer for security > reasons. All you end up doing is increasing the size of the buffer > beyond what the guest and client are capable of doing. Since you still > can lose data, apps have to be written to handle this. I think it adds > complexity without a lot of benefit. The buffering has to remain anyway since we can't assume that the ports will consume the entire buffers we pass on to them. So we'll have to buffer the data till the entire buffer is consumed. That, or the buffer management should be passed off to individual ports. Which might result in a lot of code duplication since we can have a lot of these ports in different places in the qemu code. So I guess it's better to leave the buffer management in the bus itself. Which means we get the 'cache_buffers' functionality essentially for free. Amit ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-08 13:35 ` Jamie Lokier 2010-01-08 16:26 ` Anthony Liguori @ 2010-01-11 8:34 ` Amit Shah 2010-01-11 10:45 ` Jamie Lokier 1 sibling, 1 reply; 33+ messages in thread From: Amit Shah @ 2010-01-11 8:34 UTC (permalink / raw) To: Jamie Lokier; +Cc: qemu-devel On (Fri) Jan 08 2010 [13:35:03], Jamie Lokier wrote: > > Since VNC is clearly designed to work over TCP, and is written by > people who know this, I'm wondering why you think it needs to be > different for virtio-serial. For vnc putting stuff from a guest clipboard into vnc client clipboard using the ServerCutText command, the entire buffer has to be provided after sending the command and the 'length' values. In this case, if the data from guest arrives in multiple packets, we really don't want to call into the write function multiple times. A single clipboard entry has to be created in the client with the entire contents, so a single write operation has to be invoked. For this to happen, there has to be some indication from the guest as to how much data was written in one write() operation, which will let us make a single write operation to the vnc client. Amit ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-11 8:34 ` Amit Shah @ 2010-01-11 10:45 ` Jamie Lokier 2010-01-11 11:04 ` Amit Shah 0 siblings, 1 reply; 33+ messages in thread From: Jamie Lokier @ 2010-01-11 10:45 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel Amit Shah wrote: > On (Fri) Jan 08 2010 [13:35:03], Jamie Lokier wrote: > > Since VNC is clearly designed to work over TCP, and is written by > > people who know this, I'm wondering why you think it needs to be > > different for virtio-serial. > > For vnc putting stuff from a guest clipboard into vnc client clipboard > using the ServerCutText command, the entire buffer has to be provided > after sending the command and the 'length' values. Are you talking about a VNC protocol command between qemu's VNC server and the user's VNC client, or a private protocol between the guest and qemu's VNC server? > In this case, if the data from guest arrives in multiple packets, we > really don't want to call into the write function multiple times. A > single clipboard entry has to be created in the client with the entire > contents, so a single write operation has to be invoked. Same question again: *Why do you think the VNC server (in qemu) needs to see the entire clipboard in a aingle write from the guest?* You have already told it the total length to expect. There is no ambiguity about where it ends. There is no need to do any more, if the reciever (in qemu) is implemented correctly with a sane protocol. That's assuming the guest sends to qemu's VNC server which then sends it to the user's VNC client. > For this to happen, there has to be some indication from the guest as to > how much data was written in one write() operation, which will let us > make a single write operation to the vnc client. When it is sent to the user's VNC client, it will be split into multiple packets by TCP. You *can't* send a single large write over TCP without getting it split at arbitrary places. It's *impossible*. TCP doesn't support that. It will split and merge your writes arbitrarily. So the only interesting part is how it's transmitted from the guest to qemu's VNC server first. Do you get to design that protocol yourself? -- Jamie ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-11 10:45 ` Jamie Lokier @ 2010-01-11 11:04 ` Amit Shah 2010-01-11 23:33 ` Jamie Lokier 0 siblings, 1 reply; 33+ messages in thread From: Amit Shah @ 2010-01-11 11:04 UTC (permalink / raw) To: Jamie Lokier; +Cc: qemu-devel On (Mon) Jan 11 2010 [10:45:53], Jamie Lokier wrote: > Amit Shah wrote: > > On (Fri) Jan 08 2010 [13:35:03], Jamie Lokier wrote: > > > Since VNC is clearly designed to work over TCP, and is written by > > > people who know this, I'm wondering why you think it needs to be > > > different for virtio-serial. > > > > For vnc putting stuff from a guest clipboard into vnc client clipboard > > using the ServerCutText command, the entire buffer has to be provided > > after sending the command and the 'length' values. > > Are you talking about a VNC protocol command between qemu's VNC server > and the user's VNC client, or a private protocol between the guest and > qemu's VNC server? What happens is: 1. Guest puts something on its clipboard 2. An agent on the guest gets notified of new clipboard contents 3. This agent sends over the entire clipboard contents to qemu via virtio-serial 4. virtio-serial sends off this data to the virtio-serial-vnc code 5. ServerCutText message from the vnc backend is sent to the vnc client 6. vnc client's clipboard gets updated 7. You can see guest's clipboard contents in your client's clipboard. I'm talking about steps 3, 4, 5 here. > > In this case, if the data from guest arrives in multiple packets, we > > really don't want to call into the write function multiple times. A > > single clipboard entry has to be created in the client with the entire > > contents, so a single write operation has to be invoked. > > Same question again: *Why do you think the VNC server (in qemu) needs to > see the entire clipboard in a aingle write from the guest?* > > You have already told it the total length to expect. There is no > ambiguity about where it ends. Where does the total length come from? It has to come from the guest. Otherwise, the vnc code will not know if a byte stream contains two separate clipboard entries or just one huge clipboard entry. Earlier, I used to send the length of one write as issued by a guest to qemu. I just changed that to send a START and END flag so that I don't have to send the length. If this doesn't explain it, then I think we're not understanding each other here. Amit ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-11 11:04 ` Amit Shah @ 2010-01-11 23:33 ` Jamie Lokier 2010-01-12 0:27 ` Anthony Liguori 2010-01-12 7:16 ` Amit Shah 0 siblings, 2 replies; 33+ messages in thread From: Jamie Lokier @ 2010-01-11 23:33 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel Amit Shah wrote: > > Are you talking about a VNC protocol command between qemu's VNC server > > and the user's VNC client, or a private protocol between the guest and > > qemu's VNC server? > > What happens is: > > 1. Guest puts something on its clipboard > 2. An agent on the guest gets notified of new clipboard contents > 3. This agent sends over the entire clipboard contents to qemu via > virtio-serial > 4. virtio-serial sends off this data to the virtio-serial-vnc code > 5. ServerCutText message from the vnc backend is sent to the vnc client > 6. vnc client's clipboard gets updated > 7. You can see guest's clipboard contents in your client's clipboard. > > I'm talking about steps 3, 4, 5 here. Ok. Let's not worry about 5; it doesn't seem relevant, only that the guest clipboad is sent to the host somehow. > > You have already told it the total length to expect. There is no > > ambiguity about where it ends. > > Where does the total length come from? It has to come from the guest. > Otherwise, the vnc code will not know if a byte stream contains two > separate clipboard entries or just one huge clipboard entry. I see. So it's a *really simple* protocol where the clipboard entry is sent by the guest agent with a single write() without any framing bytes? > Earlier, I used to send the length of one write as issued by a guest to > qemu. I just changed that to send a START and END flag so that I don't > have to send the length. Why not just have the guest agent send a 4-byte header which is the integer length of the clipboard blob to follow? I.e. instead of int guest_send_clipboard(const char *data, size_t length) { return write_full(virtio_fd, data, length); } do this: int guest_send_clipboard(const char *data, size_t length) { u32 encoded_length = cpu_to_be32(length); int err = write_full(virtio_serial_fd, &encoded_length, sizeof(encoded_length)); if (err == 0) err = write_full(virtio_serial_fd, data, length); return err; } > If this doesn't explain it, then I think we're not understanding each > other here. It does explain it very well, thanks. I think you're misguided about the solution :-) What confused me was you mentioned the VNC ServerCutText command having to receive the whole data in one go. ServerCutText isn't really relevant to this, and clearly is encoded with VNC protocol framing. If it was RDP or the SDL client instead of VNC, it would be something else. All that matters is getting the clipboard blob from guest to qemu in one piece, right? Having the guest agent send a few framing bytes seems very simple, and would have the added bonus that the same guest agent protocol would work on a "real" emulated serial port, guest->host TCP, etc. where virtio-serial isn't available in the guest OS (e.g. older kernels). I really can't see any merit in making virtio-serial not be a serial port, being instead like a unix datagram socket, to support a specific user of virtio-serial when a trivial 4-byte header in the guest agent code would be easier for that user anyway. If it did that, I think the name virtio-serial would have to change to virtio-datagram, becuase it wouldn't behave like a serial port any more. It would also be less useful for things that _do_ want something like a pipe/serial port. But why bother? -- Jamie ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-11 23:33 ` Jamie Lokier @ 2010-01-12 0:27 ` Anthony Liguori 2010-01-12 7:16 ` Amit Shah 1 sibling, 0 replies; 33+ messages in thread From: Anthony Liguori @ 2010-01-12 0:27 UTC (permalink / raw) To: Jamie Lokier; +Cc: Amit Shah, qemu-devel On 01/11/2010 05:33 PM, Jamie Lokier wrote: > Amit Shah wrote: > >>> Are you talking about a VNC protocol command between qemu's VNC server >>> and the user's VNC client, or a private protocol between the guest and >>> qemu's VNC server? >>> >> What happens is: >> >> 1. Guest puts something on its clipboard >> 2. An agent on the guest gets notified of new clipboard contents >> 3. This agent sends over the entire clipboard contents to qemu via >> virtio-serial >> 4. virtio-serial sends off this data to the virtio-serial-vnc code >> 5. ServerCutText message from the vnc backend is sent to the vnc client >> 6. vnc client's clipboard gets updated >> 7. You can see guest's clipboard contents in your client's clipboard. >> >> I'm talking about steps 3, 4, 5 here. >> > Ok. Let's not worry about 5; it doesn't seem relevant, only that the > guest clipboad is sent to the host somehow. > > >>> You have already told it the total length to expect. There is no >>> ambiguity about where it ends. >>> >> Where does the total length come from? It has to come from the guest. >> Otherwise, the vnc code will not know if a byte stream contains two >> separate clipboard entries or just one huge clipboard entry. >> > I see. So it's a *really simple* protocol where the clipboard entry > is sent by the guest agent with a single write() without any framing bytes? > > >> Earlier, I used to send the length of one write as issued by a guest to >> qemu. I just changed that to send a START and END flag so that I don't >> have to send the length. >> > Why not just have the guest agent send a 4-byte header which is the > integer length of the clipboard blob to follow? > > I.e. instead of > > int guest_send_clipboard(const char *data, size_t length) > { > return write_full(virtio_fd, data, length); > } > > do this: > > int guest_send_clipboard(const char *data, size_t length) > { > u32 encoded_length = cpu_to_be32(length); > int err = write_full(virtio_serial_fd,&encoded_length, > sizeof(encoded_length)); > if (err == 0) > err = write_full(virtio_serial_fd, data, length); > return err; > } > > >> If this doesn't explain it, then I think we're not understanding each >> other here. >> > It does explain it very well, thanks. I think you're misguided about > the solution :-) > > What confused me was you mentioned the VNC ServerCutText command > having to receive the whole data in one go. ServerCutText isn't > really relevant to this, and clearly is encoded with VNC protocol > framing. If it was RDP or the SDL client instead of VNC, it would be > something else. All that matters is getting the clipboard blob from > guest to qemu in one piece, right? > > Having the guest agent send a few framing bytes seems very simple, and > would have the added bonus that the same guest agent protocol would > work on a "real" emulated serial port, guest->host TCP, etc. where > virtio-serial isn't available in the guest OS (e.g. older kernels). > > I really can't see any merit in making virtio-serial not be a serial > port, being instead like a unix datagram socket, to support a specific > user of virtio-serial when a trivial 4-byte header in the guest agent > code would be easier for that user anyway. > > If it did that, I think the name virtio-serial would have to change to > virtio-datagram, becuase it wouldn't behave like a serial port any > more. It would also be less useful for things that _do_ want > something like a pipe/serial port. But why bother? > I agree wrt a streaming protocol verses a datagram protocol. The core argument IMHO is that the userspace interface is a file descriptor. Most programmers are used to assuming that boundaries aren't preserved in read/write calls. Regards, Anthony Liguori > -- Jamie > > > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-11 23:33 ` Jamie Lokier 2010-01-12 0:27 ` Anthony Liguori @ 2010-01-12 7:16 ` Amit Shah 2010-01-12 15:00 ` Anthony Liguori 1 sibling, 1 reply; 33+ messages in thread From: Amit Shah @ 2010-01-12 7:16 UTC (permalink / raw) To: Jamie Lokier; +Cc: qemu-devel On (Mon) Jan 11 2010 [23:33:56], Jamie Lokier wrote: > Amit Shah wrote: > > > Are you talking about a VNC protocol command between qemu's VNC server > > > and the user's VNC client, or a private protocol between the guest and > > > qemu's VNC server? > > > > What happens is: > > > > 1. Guest puts something on its clipboard > > 2. An agent on the guest gets notified of new clipboard contents > > 3. This agent sends over the entire clipboard contents to qemu via > > virtio-serial > > 4. virtio-serial sends off this data to the virtio-serial-vnc code > > 5. ServerCutText message from the vnc backend is sent to the vnc client > > 6. vnc client's clipboard gets updated > > 7. You can see guest's clipboard contents in your client's clipboard. > > > > I'm talking about steps 3, 4, 5 here. > > Ok. Let's not worry about 5; it doesn't seem relevant, only that the > guest clipboad is sent to the host somehow. Actually, it is important... > > > You have already told it the total length to expect. There is no > > > ambiguity about where it ends. > > > > Where does the total length come from? It has to come from the guest. > > Otherwise, the vnc code will not know if a byte stream contains two > > separate clipboard entries or just one huge clipboard entry. > > I see. So it's a *really simple* protocol where the clipboard entry > is sent by the guest agent with a single write() without any framing bytes? > > > Earlier, I used to send the length of one write as issued by a guest to > > qemu. I just changed that to send a START and END flag so that I don't > > have to send the length. > > Why not just have the guest agent send a 4-byte header which is the > integer length of the clipboard blob to follow? > > I.e. instead of > > int guest_send_clipboard(const char *data, size_t length) > { > return write_full(virtio_fd, data, length); > } > > do this: > > int guest_send_clipboard(const char *data, size_t length) > { > u32 encoded_length = cpu_to_be32(length); > int err = write_full(virtio_serial_fd, &encoded_length, > sizeof(encoded_length)); > if (err == 0) > err = write_full(virtio_serial_fd, data, length); > return err; > } > > > If this doesn't explain it, then I think we're not understanding each > > other here. > > It does explain it very well, thanks. I think you're misguided about > the solution :-) The above solution you specify works if it's assumed that we hold off writes to the vnc client till we get a complete buffer according to the header received. Now, a header might contain the length 10000, meaning 10000 bytes are to be expected. What if the write() on the guest fails after writing 8000 bytes? There's no way for us to signal that. So this vnc port might just be waiting for all 10000 bytes to be received, and it may never receive anything more. Or, it might receive the start of the next clipboard entry and it could be interpreted as data from the previous copy. > What confused me was you mentioned the VNC ServerCutText command > having to receive the whole data in one go. ServerCutText isn't > really relevant to this, It is relevant. You can't split up one ServerCutText command in multiple buffers. You can also not execute any other commands while one command is in progress, so you have to hold off on executing ServerCutText till all the data is available. And you can't reliably do that from guest userspace because of the previously-mentioned scenario. > I really can't see any merit in making virtio-serial not be a serial > port, being instead like a unix datagram socket, to support a specific > user of virtio-serial when a trivial 4-byte header in the guest agent > code would be easier for that user anyway. BTW I don't really want this too, I can get rid of it if everyone agrees we won't support clipboard writes > 4k over vnc or if there's a better idea. Amit ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-12 7:16 ` Amit Shah @ 2010-01-12 15:00 ` Anthony Liguori 2010-01-12 15:13 ` Amit Shah 2010-01-13 17:14 ` Markus Armbruster 0 siblings, 2 replies; 33+ messages in thread From: Anthony Liguori @ 2010-01-12 15:00 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel On 01/12/2010 01:16 AM, Amit Shah wrote: > BTW I don't really want this too, I can get rid of it if everyone agrees > we won't support clipboard writes> 4k over vnc or if there's a better > idea. > Why bother trying to preserve message boundaries? I think that's the fundamental question. Regards, Anthony Liguori > Amit > > > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-12 15:00 ` Anthony Liguori @ 2010-01-12 15:13 ` Amit Shah 2010-01-12 15:46 ` Anthony Liguori 2010-01-13 17:14 ` Markus Armbruster 1 sibling, 1 reply; 33+ messages in thread From: Amit Shah @ 2010-01-12 15:13 UTC (permalink / raw) To: Anthony Liguori; +Cc: qemu-devel On (Tue) Jan 12 2010 [09:00:52], Anthony Liguori wrote: > On 01/12/2010 01:16 AM, Amit Shah wrote: >> BTW I don't really want this too, I can get rid of it if everyone agrees >> we won't support clipboard writes> 4k over vnc or if there's a better >> idea. >> > > Why bother trying to preserve message boundaries? I think that's the > fundamental question. For the vnc clipboard copy-paste case, I explained that in the couple of mails before in this thread. There might be other use-cases, I don't know about them though. Amit ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-12 15:13 ` Amit Shah @ 2010-01-12 15:46 ` Anthony Liguori 2010-01-12 15:49 ` Amit Shah 0 siblings, 1 reply; 33+ messages in thread From: Anthony Liguori @ 2010-01-12 15:46 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel On 01/12/2010 09:13 AM, Amit Shah wrote: > On (Tue) Jan 12 2010 [09:00:52], Anthony Liguori wrote: > >> On 01/12/2010 01:16 AM, Amit Shah wrote: >> >>> BTW I don't really want this too, I can get rid of it if everyone agrees >>> we won't support clipboard writes> 4k over vnc or if there's a better >>> idea. >>> >>> >> Why bother trying to preserve message boundaries? I think that's the >> fundamental question. >> > For the vnc clipboard copy-paste case, I explained that in the couple of > mails before in this thread. > It didn't make sense to me. I think the assumption has to be that the client can send corrupt data and the host has to handle it. Regards, Anthony Liguori > There might be other use-cases, I don't know about them though. > > Amit > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-12 15:46 ` Anthony Liguori @ 2010-01-12 15:49 ` Amit Shah 2010-01-12 15:55 ` Anthony Liguori 0 siblings, 1 reply; 33+ messages in thread From: Amit Shah @ 2010-01-12 15:49 UTC (permalink / raw) To: Anthony Liguori; +Cc: qemu-devel On (Tue) Jan 12 2010 [09:46:55], Anthony Liguori wrote: > On 01/12/2010 09:13 AM, Amit Shah wrote: >> On (Tue) Jan 12 2010 [09:00:52], Anthony Liguori wrote: >> >>> On 01/12/2010 01:16 AM, Amit Shah wrote: >>> >>>> BTW I don't really want this too, I can get rid of it if everyone agrees >>>> we won't support clipboard writes> 4k over vnc or if there's a better >>>> idea. >>>> >>>> >>> Why bother trying to preserve message boundaries? I think that's the >>> fundamental question. >>> >> For the vnc clipboard copy-paste case, I explained that in the couple of >> mails before in this thread. >> > > It didn't make sense to me. I think the assumption has to be that the > client can send corrupt data and the host has to handle it. You mean if the guest kernel sends the wrong flags? Or doesn't set the flags? Can you explain what scenario you're talking about? Amit ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-12 15:49 ` Amit Shah @ 2010-01-12 15:55 ` Anthony Liguori 2010-01-12 16:04 ` Amit Shah 0 siblings, 1 reply; 33+ messages in thread From: Anthony Liguori @ 2010-01-12 15:55 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel On 01/12/2010 09:49 AM, Amit Shah wrote: > On (Tue) Jan 12 2010 [09:46:55], Anthony Liguori wrote: > >> On 01/12/2010 09:13 AM, Amit Shah wrote: >> >>> On (Tue) Jan 12 2010 [09:00:52], Anthony Liguori wrote: >>> >>> >>>> On 01/12/2010 01:16 AM, Amit Shah wrote: >>>> >>>> >>>>> BTW I don't really want this too, I can get rid of it if everyone agrees >>>>> we won't support clipboard writes> 4k over vnc or if there's a better >>>>> idea. >>>>> >>>>> >>>>> >>>> Why bother trying to preserve message boundaries? I think that's the >>>> fundamental question. >>>> >>>> >>> For the vnc clipboard copy-paste case, I explained that in the couple of >>> mails before in this thread. >>> >>> >> It didn't make sense to me. I think the assumption has to be that the >> client can send corrupt data and the host has to handle it. >> > You mean if the guest kernel sends the wrong flags? Or doesn't set the > flags? Can you explain what scenario you're talking about? > It's very likely that you'll have to implement some sort of protocol on top of virtio-serial. It won't always just be simple strings. If you have a simple datagram protocol, that contains two ints and a string, it's going to have to be encoded like <int a><int b><int len><char data[len]>. You need to validate that len fits within the boundaries and deal with len being less than the boundary. If you've got a command protocol where the you send the guest something and then expect a response, you have to deal with the fact that the guest may never respond. Having well defined message boundaries does not help the general problem and it only helps in the most trivial cases. Basically, it boils down to a lot of complexity for something that isn't going to be helpful in most circumstances. Regards, Anthony Liguori > Amit > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-12 15:55 ` Anthony Liguori @ 2010-01-12 16:04 ` Amit Shah 0 siblings, 0 replies; 33+ messages in thread From: Amit Shah @ 2010-01-12 16:04 UTC (permalink / raw) To: Anthony Liguori; +Cc: qemu-devel On (Tue) Jan 12 2010 [09:55:41], Anthony Liguori wrote: > On 01/12/2010 09:49 AM, Amit Shah wrote: >> On (Tue) Jan 12 2010 [09:46:55], Anthony Liguori wrote: >> >>> On 01/12/2010 09:13 AM, Amit Shah wrote: >>> >>>> On (Tue) Jan 12 2010 [09:00:52], Anthony Liguori wrote: >>>> >>>> >>>>> On 01/12/2010 01:16 AM, Amit Shah wrote: >>>>> >>>>> >>>>>> BTW I don't really want this too, I can get rid of it if everyone agrees >>>>>> we won't support clipboard writes> 4k over vnc or if there's a better >>>>>> idea. >>>>>> >>>>>> >>>>>> >>>>> Why bother trying to preserve message boundaries? I think that's the >>>>> fundamental question. >>>>> >>>>> >>>> For the vnc clipboard copy-paste case, I explained that in the couple of >>>> mails before in this thread. >>>> >>>> >>> It didn't make sense to me. I think the assumption has to be that the >>> client can send corrupt data and the host has to handle it. >>> >> You mean if the guest kernel sends the wrong flags? Or doesn't set the >> flags? Can you explain what scenario you're talking about? >> > > It's very likely that you'll have to implement some sort of protocol on > top of virtio-serial. It won't always just be simple strings. Yes, virtio-serial is just meant to be a transport agnostic of whatever data or protocols that ride over it. > If you have a simple datagram protocol, that contains two ints and a > string, it's going to have to be encoded like <int a><int b><int > len><char data[len]>. You need to validate that len fits within the > boundaries and deal with len being less than the boundary. > > If you've got a command protocol where the you send the guest something > and then expect a response, you have to deal with the fact that the > guest may never respond. Having well defined message boundaries does > not help the general problem and it only helps in the most trivial cases. > > Basically, it boils down to a lot of complexity for something that isn't > going to be helpful in most circumstances. I don't know why you're saying virtio-serial-bus does (or needs to) do anything of this. Amit ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-12 15:00 ` Anthony Liguori 2010-01-12 15:13 ` Amit Shah @ 2010-01-13 17:14 ` Markus Armbruster 2010-01-13 18:31 ` Anthony Liguori 1 sibling, 1 reply; 33+ messages in thread From: Markus Armbruster @ 2010-01-13 17:14 UTC (permalink / raw) To: Anthony Liguori; +Cc: Amit Shah, qemu-devel Anthony Liguori <anthony@codemonkey.ws> writes: > On 01/12/2010 01:16 AM, Amit Shah wrote: >> BTW I don't really want this too, I can get rid of it if everyone agrees >> we won't support clipboard writes> 4k over vnc or if there's a better >> idea. >> > > Why bother trying to preserve message boundaries? I think that's the > fundamental question. Yes. Either it's a datagram or a stream pipe. I always thought it would be a stream pipe, as the name "serial" suggests. As to the clipboard use case: same problem exists with any old stream pipe, including TCP, same solutions apply. If you told the peer "I'm going to send you 12345 bytes now", and your stream pipe chokes after 7890 bytes, you retry until everything got through. If you want to be able to abort a partial transfer and start a new one, you layer a protocol suitable for that on top of your stream pipe. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests 2010-01-13 17:14 ` Markus Armbruster @ 2010-01-13 18:31 ` Anthony Liguori 0 siblings, 0 replies; 33+ messages in thread From: Anthony Liguori @ 2010-01-13 18:31 UTC (permalink / raw) To: Markus Armbruster; +Cc: Amit Shah, qemu-devel On 01/13/2010 11:14 AM, Markus Armbruster wrote: > Anthony Liguori<anthony@codemonkey.ws> writes: > > >> On 01/12/2010 01:16 AM, Amit Shah wrote: >> >>> BTW I don't really want this too, I can get rid of it if everyone agrees >>> we won't support clipboard writes> 4k over vnc or if there's a better >>> idea. >>> >>> >> Why bother trying to preserve message boundaries? I think that's the >> fundamental question. >> > Yes. Either it's a datagram or a stream pipe. I always thought it > would be a stream pipe, as the name "serial" suggests. > And if it's a datagram, then we should accept that there will be a fixed max message size which is pretty common in all datagram protocols. That fixed size should be no larger than what the transport supports so in this case, it would be 4k. If a guest wants to send larger messages, it must build a continuation protocol on top of the datagram protocol. Regards, Anthony Liguori ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 1/8] virtio: Remove duplicate macro definition for max. virtqueues, bump up the max 2010-01-07 7:31 ` [Qemu-devel] [PATCH 1/8] virtio: Remove duplicate macro definition for max. virtqueues, bump up the max Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 2/8] virtio-console: qdev conversion, new virtio-serial-bus Amit Shah @ 2010-01-11 19:57 ` Anthony Liguori 1 sibling, 0 replies; 33+ messages in thread From: Anthony Liguori @ 2010-01-11 19:57 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel On 01/07/2010 01:31 AM, Amit Shah wrote: > VIRTIO_PCI_QUEUE_MAX is redefined in hw/virtio.c. Let's just keep it in > hw/virtio.h. > > Also, bump up the value of the maximum allowed virtqueues to 64. This is > in preparation to allow multiple ports per virtio-console device. > > Signed-off-by: Amit Shah<amit.shah@redhat.com> > These will need rebasing against master. It gets pretty ugly because of the rename. Wait for me to do a review (today or tomorrow) before hand though. I'll explicitly reply and Ack/Nack. Regards, Anthony Liguori ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2010-01-13 18:31 UTC | newest] Thread overview: 33+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-01-07 7:31 [Qemu-devel] [PATCH 0/8] virtio-console: Move to qdev, multiple devices, generic ports Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 1/8] virtio: Remove duplicate macro definition for max. virtqueues, bump up the max Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 2/8] virtio-console: qdev conversion, new virtio-serial-bus Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 3/8] virtio-serial-bus: Maintain guest and host port open/close state Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 4/8] virtio-serial-bus: Add a port 'name' property for port discovery in guests Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 6/8] virtio-serial-bus: Add ability to hot-unplug ports Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 7/8] virtio-serial: Add a 'virtserialport' device for generic serial port support Amit Shah 2010-01-07 7:31 ` [Qemu-devel] [PATCH 8/8] Move virtio-serial to Makefile.hw Amit Shah 2010-01-08 0:41 ` Andreas Färber 2010-01-08 5:01 ` Amit Shah 2010-01-08 1:12 ` [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests Jamie Lokier 2010-01-08 5:03 ` Amit Shah 2010-01-08 13:35 ` Jamie Lokier 2010-01-08 16:26 ` Anthony Liguori 2010-01-11 8:39 ` Amit Shah 2010-01-12 0:28 ` Anthony Liguori 2010-01-12 7:08 ` Amit Shah 2010-01-11 8:34 ` Amit Shah 2010-01-11 10:45 ` Jamie Lokier 2010-01-11 11:04 ` Amit Shah 2010-01-11 23:33 ` Jamie Lokier 2010-01-12 0:27 ` Anthony Liguori 2010-01-12 7:16 ` Amit Shah 2010-01-12 15:00 ` Anthony Liguori 2010-01-12 15:13 ` Amit Shah 2010-01-12 15:46 ` Anthony Liguori 2010-01-12 15:49 ` Amit Shah 2010-01-12 15:55 ` Anthony Liguori 2010-01-12 16:04 ` Amit Shah 2010-01-13 17:14 ` Markus Armbruster 2010-01-13 18:31 ` Anthony Liguori 2010-01-11 19:57 ` [Qemu-devel] [PATCH 1/8] virtio: Remove duplicate macro definition for max. virtqueues, bump up the max Anthony Liguori
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).