virtio-serial: Rework, fix post_load code

PATCH In review Amit Shah 3 participants 7 messages Dec 13, 2012 – Dec 14, 2012

Share & tools

Amit Shah

author

2012-12-13 10:38 UTC #0 lore raw

cover virtio-serial: Rework, fix post_load code

This series reworks the post_load code recently introduced to allocate the structures only when required (i.e. only at load time). This helps keep the VirtIOSerial struct clean, and use less RAM.

Also rearrange the code in virtio_serial_load() for easier readability.

Patch 4 fixes a race with the timer going off after a device got hot-unplugged, and patch 1 uses unsigned int (uint32_t) type to count ports, as in the rest of the code.

Please review.

Amit Shah (4):
virtio-serial: use uint32_t to count ports
virtio-serial: move active ports loading to separate function
virtio-serial: allocate post_load only at load-time
virtio-serial: delete timer if active during exit

 hw/virtio-serial-bus.c | 150 +++++++++++++++++++++++++++++--------------------
 1 file changed, 89 insertions(+), 61 deletions(-)

Signature · 1 line

1.8.0.2

Amit Shah

author

2012-12-13 10:38 UTC #1 lore raw

PATCH 1/4 virtio-serial: use uint32_t to count ports

Signed-off-by Amit Shah <[email protected]>

---
 hw/virtio-serial-bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/virtio-serial-bus.c +2 -2

index 155da58..30f450c 100644
@@ -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;

Signature · 1 line

1.8.0.2

Rob Landley

2012-12-14 05:18 UTC #2 lore raw

Re: [Qemu-devel] [PATCH 1/4] virtio-serial: use uint32_t to count ports

Speaking of virtio-serial, is there a way to get virtio serial ports to attach to qemu's stdin/stdout the way other serial ports can (and which is the default with --nographic for conventional serial ports), or do you still have to make a magic char device on the host?

I never understood why _using_ virtio serial ports was so different, but it's been about a year since I looked at them (because they were so different).

Rob

Amit Shah

author

2012-12-13 10:38 UTC #3 lore raw

PATCH 2/4 virtio-serial: move active ports loading to separate function

In reply to Amit Shah

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 <[email protected]>

---
 hw/virtio-serial-bus.c | 96 +++++++++++++++++++++++++++++---------------------
 1 file changed, 55 insertions(+), 41 deletions(-)

hw/virtio-serial-bus.c +55 ~~-41~~

index 30f450c..2e0fe3d 100644
@@ -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;
 }

Signature · 1 line

1.8.0.2

Amit Shah

author

2012-12-13 10:38 UTC #4 lore raw

PATCH 3/4 virtio-serial: allocate post_load only at load-time

In reply to Amit Shah

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 <[email protected]>

---
 hw/virtio-serial-bus.c | 63 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 38 insertions(+), 25 deletions(-)

hw/virtio-serial-bus.c +38 ~~-25~~

index 2e0fe3d..09d4659 100644
@@ -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);
 }

Signature · 1 line

1.8.0.2

Amit Shah

author

2012-12-13 10:38 UTC #5 lore raw

PATCH 4/4 virtio-serial: delete timer if active during exit

In reply to Amit Shah

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 <[email protected]>

---
 hw/virtio-serial-bus.c | 1 +
 1 file changed, 1 insertion(+)

hw/virtio-serial-bus.c +1 -0

index 09d4659..fc0166c 100644
@@ -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);
     }

Signature · 1 line

1.8.0.2

Alon Levy

Reviewed-by on the v1 series

2012-12-13 10:46 UTC #6 lore raw

In reply to Amit Shah

All looks good.

Reviewed-by Alon Levy <[email protected]>

----- Original Message -----

Quoted 28 lines

This series reworks the post_load code recently introduced to allocate
the structures only when required (i.e. only at load time). This helps keep the VirtIOSerial struct clean, and use less RAM.
Also rearrange the code in virtio_serial_load() for easier readability.
Patch 4 fixes a race with the timer going off after a device got hot-unplugged, and patch 1 uses unsigned int (uint32_t) type to count ports, as in the rest of the code.
Please review.
Amit Shah (4):
virtio-serial: use uint32_t to count ports
virtio-serial: move active ports loading to separate function
virtio-serial: allocate post_load only at load-time
virtio-serial: delete timer if active during exit
 hw/virtio-serial-bus.c | 150
 +++++++++++++++++++++++++++++--------------------
 1 file changed, 89 insertions(+), 61 deletions(-)
--
1.8.0.2