From c3587ca1a25862628e06cc019f91e7b2dcef40bf Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Thu, 29 Nov 2012 15:44:44 +0530 Subject: [PATCH 1/6] virtio-serial: use uint32_t to count ports Signed-off-by: Amit Shah --- hw/virtio-serial-bus.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 155da58dcd..30f450cd61 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -56,7 +56,7 @@ struct VirtIOSerial { struct { QEMUTimer *timer; - int nr_active_ports; + uint32_t nr_active_ports; struct { VirtIOSerialPort *port; uint8_t host_connected; @@ -637,7 +637,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque) static void virtio_serial_post_load_timer_cb(void *opaque) { - int i; + uint32_t i; VirtIOSerial *s = opaque; VirtIOSerialPort *port; uint8_t host_connected; From 2e575a86abc36764ef34030f423ef118914a01cc Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Thu, 29 Nov 2012 17:02:14 +0530 Subject: [PATCH 2/6] virtio-serial: move active ports loading to separate function The virtio_serial_load() function became too big, split the code that gets the port info from the source into a separate function. Signed-off-by: Amit Shah --- hw/virtio-serial-bus.c | 96 ++++++++++++++++++++++++------------------ 1 file changed, 55 insertions(+), 41 deletions(-) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 30f450cd61..2e0fe3d66f 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -658,10 +658,60 @@ static void virtio_serial_post_load_timer_cb(void *opaque) s->post_load.connected = NULL; } +static int fetch_active_ports_list(QEMUFile *f, int version_id, + VirtIOSerial *s, uint32_t nr_active_ports) +{ + uint32_t i; + + s->post_load.nr_active_ports = nr_active_ports; + s->post_load.connected = + g_malloc0(sizeof(*s->post_load.connected) * nr_active_ports); + + /* Items in struct VirtIOSerialPort */ + for (i = 0; i < nr_active_ports; i++) { + VirtIOSerialPort *port; + uint32_t id; + + id = qemu_get_be32(f); + port = find_port_by_id(s, id); + if (!port) { + return -EINVAL; + } + + port->guest_connected = qemu_get_byte(f); + s->post_load.connected[i].port = port; + s->post_load.connected[i].host_connected = qemu_get_byte(f); + + 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); + } + } + } + qemu_mod_timer(s->post_load.timer, 1); + return 0; +} + static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) { VirtIOSerial *s = opaque; - VirtIOSerialPort *port; uint32_t max_nr_ports, nr_active_ports, ports_map; unsigned int i; int ret; @@ -705,48 +755,12 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) qemu_get_be32s(f, &nr_active_ports); - s->post_load.nr_active_ports = nr_active_ports; - s->post_load.connected = - g_malloc0(sizeof(*s->post_load.connected) * 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); - if (!port) { - return -EINVAL; - } - - port->guest_connected = qemu_get_byte(f); - s->post_load.connected[i].port = port; - s->post_load.connected[i].host_connected = qemu_get_byte(f); - - 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); - } + if (nr_active_ports) { + ret = fetch_active_ports_list(f, version_id, s, nr_active_ports); + if (ret) { + return ret; } } - qemu_mod_timer(s->post_load.timer, 1); return 0; } From bdb917bf8ab187b662c612ee6fb87479c0b82490 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Fri, 30 Nov 2012 00:54:44 +0530 Subject: [PATCH 3/6] virtio-serial: allocate post_load only at load-time This saves us a few bytes in the VirtIOSerial struct. Not a big savings, but since the entire structure is used only during a short while after migration, it's helpful to keep the struct cleaner and smaller. Signed-off-by: Amit Shah --- hw/virtio-serial-bus.c | 63 +++++++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 2e0fe3d66f..09d46593e2 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -36,6 +36,15 @@ struct VirtIOSerialBus { uint32_t max_nr_ports; }; +typedef struct VirtIOSerialPostLoad { + QEMUTimer *timer; + uint32_t nr_active_ports; + struct { + VirtIOSerialPort *port; + uint8_t host_connected; + } *connected; +} VirtIOSerialPostLoad; + struct VirtIOSerial { VirtIODevice vdev; @@ -54,14 +63,7 @@ struct VirtIOSerial { struct virtio_console_config config; - struct { - QEMUTimer *timer; - uint32_t nr_active_ports; - struct { - VirtIOSerialPort *port; - uint8_t host_connected; - } *connected; - } post_load; + struct VirtIOSerialPostLoad *post_load; }; static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id) @@ -642,9 +644,12 @@ static void virtio_serial_post_load_timer_cb(void *opaque) VirtIOSerialPort *port; uint8_t host_connected; - for (i = 0 ; i < s->post_load.nr_active_ports; ++i) { - port = s->post_load.connected[i].port; - host_connected = s->post_load.connected[i].host_connected; + if (!s->post_load) { + return; + } + for (i = 0 ; i < s->post_load->nr_active_ports; ++i) { + port = s->post_load->connected[i].port; + host_connected = s->post_load->connected[i].host_connected; if (host_connected != port->host_connected) { /* * We have to let the guest know of the host connection @@ -654,8 +659,10 @@ static void virtio_serial_post_load_timer_cb(void *opaque) port->host_connected); } } - g_free(s->post_load.connected); - s->post_load.connected = NULL; + g_free(s->post_load->connected); + qemu_free_timer(s->post_load->timer); + g_free(s->post_load); + s->post_load = NULL; } static int fetch_active_ports_list(QEMUFile *f, int version_id, @@ -663,9 +670,14 @@ static int fetch_active_ports_list(QEMUFile *f, int version_id, { uint32_t i; - s->post_load.nr_active_ports = nr_active_ports; - s->post_load.connected = - g_malloc0(sizeof(*s->post_load.connected) * nr_active_ports); + s->post_load = g_malloc0(sizeof(*s->post_load)); + s->post_load->nr_active_ports = nr_active_ports; + s->post_load->connected = + g_malloc0(sizeof(*s->post_load->connected) * nr_active_ports); + + s->post_load->timer = qemu_new_timer_ns(vm_clock, + virtio_serial_post_load_timer_cb, + s); /* Items in struct VirtIOSerialPort */ for (i = 0; i < nr_active_ports; i++) { @@ -679,8 +691,8 @@ static int fetch_active_ports_list(QEMUFile *f, int version_id, } port->guest_connected = qemu_get_byte(f); - s->post_load.connected[i].port = port; - s->post_load.connected[i].host_connected = qemu_get_byte(f); + s->post_load->connected[i].port = port; + s->post_load->connected[i].host_connected = qemu_get_byte(f); if (version_id > 2) { uint32_t elem_popped; @@ -705,7 +717,7 @@ static int fetch_active_ports_list(QEMUFile *f, int version_id, } } } - qemu_mod_timer(s->post_load.timer, 1); + qemu_mod_timer(s->post_load->timer, 1); return 0; } @@ -1003,6 +1015,8 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *conf) vser->qdev = dev; + vser->post_load = NULL; + /* * Register for the savevm section with the virtio-console name * to preserve backward compat @@ -1010,9 +1024,6 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *conf) register_savevm(dev, "virtio-console", -1, 3, virtio_serial_save, virtio_serial_load, vser); - vser->post_load.timer = qemu_new_timer_ns(vm_clock, - virtio_serial_post_load_timer_cb, vser); - return vdev; } @@ -1025,9 +1036,11 @@ void virtio_serial_exit(VirtIODevice *vdev) g_free(vser->ivqs); g_free(vser->ovqs); g_free(vser->ports_map); - g_free(vser->post_load.connected); - qemu_free_timer(vser->post_load.timer); - + if (vser->post_load) { + g_free(vser->post_load->connected); + qemu_free_timer(vser->post_load->timer); + g_free(vser->post_load); + } virtio_cleanup(vdev); } From a75bf146503a94fb900e0dfa0529bd5d1be9fec5 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Thu, 13 Dec 2012 15:54:43 +0530 Subject: [PATCH 4/6] virtio-serial: delete timer if active during exit The post_load timer was being freed, but not deleted. This could cause problems when the timer is armed, but the device is hot-unplugged before the callback is executed. Signed-off-by: Amit Shah --- hw/virtio-serial-bus.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 09d46593e2..fc0166ca7f 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -1038,6 +1038,7 @@ void virtio_serial_exit(VirtIODevice *vdev) g_free(vser->ports_map); if (vser->post_load) { g_free(vser->post_load->connected); + qemu_del_timer(vser->post_load->timer); qemu_free_timer(vser->post_load->timer); g_free(vser->post_load); } From 4e28976e563ad54f6adc5ae00b1fb8224f1a82ca Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Wed, 12 Dec 2012 18:26:09 +0530 Subject: [PATCH 5/6] virtio-serial-bus: send_control_msg() should not deal with cpkts Stuff the cpkt before calling send_control_msg(). This function should not be concerned about contents of the buffer it receives. A few code refactorings recently have made making this change easier than earlier. Coverity and clang have flagged this code several times in the past (cpkt->id not set before send_control_event() passed it on to send_control_msg()). This will finally eliminate the false-positive. CC: Markus Armbruster Reviewed-by: Markus Armbruster Signed-off-by: Amit Shah --- hw/virtio-serial-bus.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index fc0166ca7f..3ea95b8b52 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -217,13 +217,12 @@ static void flush_queued_data(VirtIOSerialPort *port) do_flush_queued_data(port, port->ovq, &port->vser->vdev); } -static size_t send_control_msg(VirtIOSerialPort *port, void *buf, size_t len) +static size_t send_control_msg(VirtIOSerial *vser, void *buf, size_t len) { VirtQueueElement elem; VirtQueue *vq; - struct virtio_console_control *cpkt; - vq = port->vser->c_ivq; + vq = vser->c_ivq; if (!virtio_queue_ready(vq)) { return 0; } @@ -231,25 +230,24 @@ static size_t send_control_msg(VirtIOSerialPort *port, void *buf, size_t len) 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); + virtio_notify(&vser->vdev, vq); return len; } -static size_t send_control_event(VirtIOSerialPort *port, uint16_t event, - uint16_t value) +static size_t send_control_event(VirtIOSerial *vser, uint32_t port_id, + uint16_t event, uint16_t value) { struct virtio_console_control cpkt; + stl_p(&cpkt.id, port_id); stw_p(&cpkt.event, event); stw_p(&cpkt.value, value); - trace_virtio_serial_send_control_event(port->id, event, value); - return send_control_msg(port, &cpkt, sizeof(cpkt)); + trace_virtio_serial_send_control_event(port_id, event, value); + return send_control_msg(vser, &cpkt, sizeof(cpkt)); } /* Functions for use inside qemu to open and read from/write to ports */ @@ -261,7 +259,7 @@ int virtio_serial_open(VirtIOSerialPort *port) } /* Send port open notification to the guest */ port->host_connected = true; - send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1); + send_control_event(port->vser, port->id, VIRTIO_CONSOLE_PORT_OPEN, 1); return 0; } @@ -276,7 +274,7 @@ int virtio_serial_close(VirtIOSerialPort *port) port->throttled = false; discard_vq_data(port->ovq, &port->vser->vdev); - send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0); + send_control_event(port->vser, port->id, VIRTIO_CONSOLE_PORT_OPEN, 0); return 0; } @@ -365,7 +363,7 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) * ports we have here. */ QTAILQ_FOREACH(port, &vser->ports, next) { - send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1); + send_control_event(vser, port->id, VIRTIO_CONSOLE_PORT_ADD, 1); } return; } @@ -396,10 +394,11 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) * up to hvc. */ if (vsc->is_console) { - send_control_event(port, VIRTIO_CONSOLE_CONSOLE_PORT, 1); + send_control_event(vser, port->id, VIRTIO_CONSOLE_CONSOLE_PORT, 1); } if (port->name) { + stl_p(&cpkt.id, port->id); stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME); stw_p(&cpkt.value, 1); @@ -410,12 +409,12 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) memcpy(buffer + sizeof(cpkt), port->name, strlen(port->name)); buffer[buffer_len - 1] = 0; - send_control_msg(port, buffer, buffer_len); + send_control_msg(vser, buffer, buffer_len); g_free(buffer); } if (port->host_connected) { - send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1); + send_control_event(vser, port->id, VIRTIO_CONSOLE_PORT_OPEN, 1); } /* @@ -655,7 +654,7 @@ static void virtio_serial_post_load_timer_cb(void *opaque) * We have to let the guest know of the host connection * status change */ - send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, + send_control_event(s, port->id, VIRTIO_CONSOLE_PORT_OPEN, port->host_connected); } } @@ -841,9 +840,7 @@ static void mark_port_added(VirtIOSerial *vser, uint32_t port_id) static void add_port(VirtIOSerial *vser, uint32_t port_id) { mark_port_added(vser, port_id); - - send_control_event(find_port_by_id(vser, port_id), - VIRTIO_CONSOLE_PORT_ADD, 1); + send_control_event(vser, port_id, VIRTIO_CONSOLE_PORT_ADD, 1); } static void remove_port(VirtIOSerial *vser, uint32_t port_id) @@ -858,7 +855,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id) /* Flush out any unconsumed buffers first */ discard_vq_data(port->ovq, &port->vser->vdev); - send_control_event(port, VIRTIO_CONSOLE_PORT_REMOVE, 1); + send_control_event(vser, port->id, VIRTIO_CONSOLE_PORT_REMOVE, 1); } static int virtser_port_qdev_init(DeviceState *qdev) From 91bdd1cf08f65b7a127c22d4d65ff9d16dcac870 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Tue, 18 Dec 2012 13:08:33 +0530 Subject: [PATCH 6/6] virtio-serial-bus: assert port is non-null in remove_port() remove_port() is called from qdev's unplug callback, and we're certain the port will be found in our list of ports. Adding an assert() documents this. This was flagged by Coverity, fix suggested by Markus. CC: Markus Armbruster Reviewed-by: Markus Armbruster Signed-off-by: Amit Shah --- hw/virtio-serial-bus.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 3ea95b8b52..ce4556fc48 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -852,6 +852,12 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id) vser->ports_map[i] &= ~(1U << (port_id % 32)); port = find_port_by_id(vser, port_id); + /* + * This function is only called from qdev's unplug callback; if we + * get a NULL port here, we're in trouble. + */ + assert(port); + /* Flush out any unconsumed buffers first */ discard_vq_data(port->ovq, &port->vser->vdev);