[Qemu-devel] [PATCH 0/4] virtio-serial: Rework, fix post_load code

[Qemu-devel] [PATCH 0/4] virtio-serial: Rework, fix post_load code

[Amit Shah]

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

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

[Amit Shah]

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 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 155da58..30f450c 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;
-- 
1.8.0.2

[Qemu-devel] [PATCH 2/4] virtio-serial: move active ports loading to separate function

[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 <amit.shah@redhat.com>
---
 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 30f450c..2e0fe3d 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;
 }
 
-- 
1.8.0.2

[Qemu-devel] [PATCH 3/4] virtio-serial: allocate post_load only at load-time

[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 <amit.shah@redhat.com>
---
 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 2e0fe3d..09d4659 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);
 }
 
-- 
1.8.0.2

[Qemu-devel] [PATCH 4/4] virtio-serial: delete timer if active during exit

[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 <amit.shah@redhat.com>
---
 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 09d4659..fc0166c 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);
     }
-- 
1.8.0.2

Re: [Qemu-devel] [PATCH 0/4] virtio-serial: Rework, fix post_load code

[Alon Levy]

All looks good.

Reviewed-by: Alon Levy <alevy@redhat.com>

----- Original Message -----
> 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
> 
> 

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

[Rob Landley]

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