From cbe77b616cb73d0a0aedbf9c80cb3dcb7d324535 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Mon, 5 Apr 2010 14:20:29 +0530 Subject: [PATCH 1/7] virtio-console: Factor out common init between console and generic ports The initialisation for generic ports and console ports is similar. Factor out the parts that are the same in a different function that can be called from each of the initfns. Signed-off-by: Amit Shah --- hw/virtio-console.c | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/hw/virtio-console.c b/hw/virtio-console.c index caea11f3a8..d7fe68b298 100644 --- a/hw/virtio-console.c +++ b/hw/virtio-console.c @@ -58,22 +58,26 @@ static void chr_event(void *opaque, int event) } } +static int generic_port_init(VirtConsole *vcon, VirtIOSerialDevice *dev) +{ + vcon->port.info = dev->info; + + if (vcon->chr) { + qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event, + vcon); + vcon->port.info->have_data = flush_buf; + } + return 0; +} + /* 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); - port->info->have_data = flush_buf; - } - return 0; + return generic_port_init(vcon, dev); } static int virtconsole_exitfn(VirtIOSerialDevice *dev) @@ -115,14 +119,7 @@ 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); - port->info->have_data = flush_buf; - } - return 0; + return generic_port_init(vcon, dev); } static VirtIOSerialPortInfo virtserialport_info = { From 28eaf465316491884952f855f7bfc9dab597e6fb Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Fri, 10 Dec 2010 17:10:43 +0530 Subject: [PATCH 2/7] virtio-console: Remove unnecessary braces Remove unnecessary braces around a case statement. Signed-off-by: Amit Shah --- hw/virtio-console.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/virtio-console.c b/hw/virtio-console.c index d7fe68b298..d0b935491d 100644 --- a/hw/virtio-console.c +++ b/hw/virtio-console.c @@ -48,10 +48,9 @@ static void chr_event(void *opaque, int event) VirtConsole *vcon = opaque; switch (event) { - case CHR_EVENT_OPENED: { + case CHR_EVENT_OPENED: virtio_serial_open(&vcon->port); break; - } case CHR_EVENT_CLOSED: virtio_serial_close(&vcon->port); break; From 6bff86560d42a9c391cf1e502ebd764c293c4d02 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Fri, 7 Jan 2011 14:33:36 +0530 Subject: [PATCH 3/7] virtio-serial: move out discard logic in a separate function Instead of combining flush logic into the discard case and not discard case, have one function doing discard case. This will help later when adding flow control logic to the do_flush_queued_data() function. Signed-off-by: Amit Shah --- hw/virtio-serial-bus.c | 61 +++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 74ba5ec3d3..e8c2a168ec 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -113,39 +113,48 @@ static size_t write_to_port(VirtIOSerialPort *port, return offset; } -static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq, - VirtIODevice *vdev, bool discard) +static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev) { VirtQueueElement elem; - assert(port || discard); - assert(virtio_queue_ready(vq)); - - while ((discard || !port->throttled) && virtqueue_pop(vq, &elem)) { - uint8_t *buf; - size_t ret, buf_size; - - if (!discard) { - buf_size = iov_size(elem.out_sg, elem.out_num); - buf = qemu_malloc(buf_size); - ret = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, buf_size); - - port->info->have_data(port, buf, ret); - qemu_free(buf); - } + while (virtqueue_pop(vq, &elem)) { virtqueue_push(vq, &elem, 0); } virtio_notify(vdev, vq); } -static void flush_queued_data(VirtIOSerialPort *port, bool discard) +static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq, + VirtIODevice *vdev) +{ + VirtQueueElement elem; + + assert(port); + assert(virtio_queue_ready(vq)); + + while (!port->throttled && virtqueue_pop(vq, &elem)) { + uint8_t *buf; + size_t ret, buf_size; + + buf_size = iov_size(elem.out_sg, elem.out_num); + buf = qemu_malloc(buf_size); + ret = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, buf_size); + + port->info->have_data(port, buf, ret); + qemu_free(buf); + + virtqueue_push(vq, &elem, 0); + } + virtio_notify(vdev, vq); +} + +static void flush_queued_data(VirtIOSerialPort *port) { assert(port); if (!virtio_queue_ready(port->ovq)) { return; } - do_flush_queued_data(port, port->ovq, &port->vser->vdev, discard); + do_flush_queued_data(port, port->ovq, &port->vser->vdev); } static size_t send_control_msg(VirtIOSerialPort *port, void *buf, size_t len) @@ -204,7 +213,7 @@ int virtio_serial_close(VirtIOSerialPort *port) * consume, reset the throttling flag and discard the data. */ port->throttled = false; - flush_queued_data(port, true); + discard_vq_data(port->ovq, &port->vser->vdev); send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0); @@ -258,7 +267,7 @@ void virtio_serial_throttle_port(VirtIOSerialPort *port, bool throttle) return; } - flush_queued_data(port, false); + flush_queued_data(port); } /* Guest wants to notify us of some event */ @@ -414,11 +423,15 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq) discard = true; } - if (!discard && port->throttled) { + if (discard) { + discard_vq_data(vq, vdev); + return; + } + if (port->throttled) { return; } - do_flush_queued_data(port, vq, vdev, discard); + do_flush_queued_data(port, vq, vdev); } static void handle_input(VirtIODevice *vdev, VirtQueue *vq) @@ -634,7 +647,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id) port = find_port_by_id(vser, port_id); /* Flush out any unconsumed buffers first */ - flush_queued_data(port, true); + discard_vq_data(port->ovq, &port->vser->vdev); send_control_event(port, VIRTIO_CONSOLE_PORT_REMOVE, 1); } From 471344db88cc3e7adf7664aa34d54ce0cacc3419 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Fri, 10 Dec 2010 17:29:49 +0530 Subject: [PATCH 4/7] virtio-serial: Don't copy over guest buffer to host When the guest writes something to a host, we copied over the entire buffer first into the host and then processed it. Do away with that, it could result in a malicious guest causing a DoS on the host. Reported-by: Paul Brook Signed-off-by: Amit Shah --- hw/virtio-serial-bus.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index e8c2a168ec..6726f72b6f 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -132,16 +132,17 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq, assert(virtio_queue_ready(vq)); while (!port->throttled && virtqueue_pop(vq, &elem)) { - uint8_t *buf; - size_t ret, buf_size; + unsigned int i; - buf_size = iov_size(elem.out_sg, elem.out_num); - buf = qemu_malloc(buf_size); - ret = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, buf_size); + for (i = 0; i < elem.out_num; i++) { + size_t buf_size; - port->info->have_data(port, buf, ret); - qemu_free(buf); + buf_size = elem.out_sg[i].iov_len; + port->info->have_data(port, + elem.out_sg[i].iov_base, + buf_size); + } virtqueue_push(vq, &elem, 0); } virtio_notify(vdev, vq); From e300ac275bbf19b31cf5968b8de8abe52c26e163 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Mon, 13 Dec 2010 17:50:07 +0530 Subject: [PATCH 5/7] virtio-serial: Let virtio-serial-bus know if all data was consumed The have_data() API to hand off guest data to apps using virtio-serial so far assumed all the data was consumed. Relax this assumption. Future commits will allow for incomplete writes. Signed-off-by: Amit Shah --- hw/virtio-console.c | 4 ++-- hw/virtio-serial.h | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/hw/virtio-console.c b/hw/virtio-console.c index d0b935491d..62624ec780 100644 --- a/hw/virtio-console.c +++ b/hw/virtio-console.c @@ -20,11 +20,11 @@ typedef struct VirtConsole { /* Callback function that's called when the guest sends us data */ -static void flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len) +static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len) { VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); - qemu_chr_write(vcon->chr, buf, len); + return qemu_chr_write(vcon->chr, buf, len); } /* Readiness of the guest to accept data on a port */ diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h index ff08c40681..9cc0fb3543 100644 --- a/hw/virtio-serial.h +++ b/hw/virtio-serial.h @@ -137,10 +137,11 @@ struct VirtIOSerialPortInfo { /* * Guest wrote some data to the port. This data is handed over to - * the app via this callback. The app is supposed to consume all - * the data that is presented to it. + * the app via this callback. The app can return a size less than + * 'len'. In this case, throttling will be enabled for this port. */ - void (*have_data)(VirtIOSerialPort *port, const uint8_t *buf, size_t len); + ssize_t (*have_data)(VirtIOSerialPort *port, const uint8_t *buf, + size_t len); }; /* Interface to the virtio-serial bus */ From f1925dff7e6c4799f5951cf167a437c0737a266c Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Fri, 10 Dec 2010 16:51:14 +0530 Subject: [PATCH 6/7] virtio-serial: Add support for flow control This commit lets apps signal an incomplete write. When that happens, stop sending out any more data to the app and wait for it to unthrottle the port. Signed-off-by: Amit Shah --- hw/virtio-serial-bus.c | 51 ++++++++++++++++++++++++++++++++---------- hw/virtio-serial.h | 17 ++++++++++++++ 2 files changed, 56 insertions(+), 12 deletions(-) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 6726f72b6f..32938b5ec3 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -126,24 +126,49 @@ static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev) static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq, VirtIODevice *vdev) { - VirtQueueElement elem; - assert(port); assert(virtio_queue_ready(vq)); - while (!port->throttled && virtqueue_pop(vq, &elem)) { + while (!port->throttled) { unsigned int i; - for (i = 0; i < elem.out_num; i++) { - size_t buf_size; - - buf_size = elem.out_sg[i].iov_len; - - port->info->have_data(port, - elem.out_sg[i].iov_base, - buf_size); + /* Pop an elem only if we haven't left off a previous one mid-way */ + if (!port->elem.out_num) { + if (!virtqueue_pop(vq, &port->elem)) { + break; + } + port->iov_idx = 0; + port->iov_offset = 0; } - virtqueue_push(vq, &elem, 0); + + for (i = port->iov_idx; i < port->elem.out_num; i++) { + size_t buf_size; + ssize_t ret; + + buf_size = port->elem.out_sg[i].iov_len - port->iov_offset; + ret = port->info->have_data(port, + port->elem.out_sg[i].iov_base + + port->iov_offset, + buf_size); + if (ret < 0 && ret != -EAGAIN) { + /* We don't handle any other type of errors here */ + abort(); + } + if (ret == -EAGAIN || (ret >= 0 && ret < buf_size)) { + virtio_serial_throttle_port(port, true); + port->iov_idx = i; + if (ret > 0) { + port->iov_offset += ret; + } + break; + } + port->iov_offset = 0; + } + if (port->throttled) { + break; + } + virtqueue_push(vq, &port->elem, 0); + port->elem.out_num = 0; } virtio_notify(vdev, vq); } @@ -709,6 +734,8 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base) port->guest_connected = true; } + port->elem.out_num = 0; + 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.h b/hw/virtio-serial.h index 9cc0fb3543..a308196786 100644 --- a/hw/virtio-serial.h +++ b/hw/virtio-serial.h @@ -102,6 +102,23 @@ struct VirtIOSerialPort { */ uint32_t id; + /* + * This is the elem that we pop from the virtqueue. A slow + * backend that consumes guest data (e.g. the file backend for + * qemu chardevs) can cause the guest to block till all the output + * is flushed. This isn't desired, so we keep a note of the last + * element popped and continue consuming it once the backend + * becomes writable again. + */ + VirtQueueElement elem; + + /* + * The index and the offset into the iov buffer that was popped in + * elem above. + */ + uint32_t iov_idx; + uint64_t iov_offset; + /* Identify if this is a port that binds with hvc in the guest */ uint8_t is_console; From 37f95bf3d063fcfcb3b05f82203629dc50f18615 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Wed, 19 Jan 2011 16:07:10 +0530 Subject: [PATCH 7/7] virtio-serial: save/restore new fields in port struct The new fields that got added as part of not copying over the guest buffer to the host need to be saved/restored across migration. Do that and bump up the version number. Signed-off-by: Amit Shah --- hw/virtio-serial-bus.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 32938b5ec3..2ca97a90fe 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -527,9 +527,24 @@ static void virtio_serial_save(QEMUFile *f, void *opaque) * Items in struct VirtIOSerialPort. */ QTAILQ_FOREACH(port, &s->ports, next) { + uint32_t elem_popped; + qemu_put_be32s(f, &port->id); qemu_put_byte(f, port->guest_connected); qemu_put_byte(f, port->host_connected); + + elem_popped = 0; + if (port->elem.out_num) { + elem_popped = 1; + } + qemu_put_be32s(f, &elem_popped); + if (elem_popped) { + qemu_put_be32s(f, &port->iov_idx); + qemu_put_be64s(f, &port->iov_offset); + + qemu_put_buffer(f, (unsigned char *)&port->elem, + sizeof(port->elem)); + } } } @@ -540,7 +555,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) uint32_t max_nr_ports, nr_active_ports, ports_map; unsigned int i; - if (version_id > 2) { + if (version_id > 3) { return -EINVAL; } @@ -593,6 +608,29 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, port->host_connected); } + + if (version_id > 2) { + uint32_t elem_popped; + + qemu_get_be32s(f, &elem_popped); + if (elem_popped) { + qemu_get_be32s(f, &port->iov_idx); + qemu_get_be64s(f, &port->iov_offset); + + qemu_get_buffer(f, (unsigned char *)&port->elem, + sizeof(port->elem)); + virtqueue_map_sg(port->elem.in_sg, port->elem.in_addr, + port->elem.in_num, 1); + virtqueue_map_sg(port->elem.out_sg, port->elem.out_addr, + port->elem.out_num, 1); + + /* + * Port was throttled on source machine. Let's + * unthrottle it here so data starts flowing again. + */ + virtio_serial_throttle_port(port, false); + } + } } return 0; } @@ -841,7 +879,7 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports) * Register for the savevm section with the virtio-console name * to preserve backward compat */ - register_savevm(dev, "virtio-console", -1, 2, virtio_serial_save, + register_savevm(dev, "virtio-console", -1, 3, virtio_serial_save, virtio_serial_load, vser); return vdev;