[Qemu-devel] Spice / usb-redir chardev flowcontrol patches v2

[Qemu-devel] Spice / usb-redir chardev flowcontrol patches v2

[Hans de Goede]

Here is v2 of the series adding watch support to the spicevmc chardev backend
and flowcontrol support to the usb-redir device. It also includes a few
virtio-consoled bugfixes which were found during the development of this
series.

Note that this series is based *on top of* Gerd Hoffmann's chardev.5 series
to avoid conflicts with that series.

Changes in v2:
-Address Amit's review of "[PATCH 2/8] virtio-console: Remove any pending watches on close"
-Drop 2 spice-qemu-char cleanup patches as Gerd has added them to his v5 series
-Add a (forward ported) patch from Alon Levy to fix a long standing
 spice-qemu-char migration issue in a way which should be acceptable upstream

Regards,

Hans

[Qemu-devel] [PATCH 1/8] virtio-console: Also throttle when less was written then requested

[Hans de Goede]

This is necessary so that we get properly woken up to write the rest.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index e2d1c58..1d87c5b 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -47,7 +47,7 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
     ret = qemu_chr_fe_write(vcon->chr, buf, len);
     trace_virtio_console_flush_buf(port->id, len, ret);
 
-    if (ret <= 0) {
+    if (ret < len) {
         VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
 
         /*
@@ -56,7 +56,8 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
          * we had a finer-grained message, like -EPIPE, we could close
          * this connection.
          */
-        ret = 0;
+        if (ret < 0)
+            ret = 0;
         if (!k->is_console) {
             virtio_serial_throttle_port(port, true);
             qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT, chr_write_unblocked,
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/8] virtio-console: Remove any pending watches on close

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/virtio-console.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 1d87c5b..8ef76e2 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -18,6 +18,7 @@
 typedef struct VirtConsole {
     VirtIOSerialPort port;
     CharDriverState *chr;
+    guint watch;
 } VirtConsole;
 
 /*
@@ -29,6 +30,7 @@ static gboolean chr_write_unblocked(GIOChannel *chan, GIOCondition cond,
 {
     VirtConsole *vcon = opaque;
 
+    vcon->watch = 0;
     virtio_serial_throttle_port(&vcon->port, false);
     return FALSE;
 }
@@ -60,8 +62,10 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
             ret = 0;
         if (!k->is_console) {
             virtio_serial_throttle_port(port, true);
-            qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT, chr_write_unblocked,
-                                  vcon);
+            if (!vcon->watch) {
+                vcon->watch = qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT,
+                                                    chr_write_unblocked, vcon);
+            }
         }
     }
     return ret;
@@ -116,6 +120,10 @@ static void chr_event(void *opaque, int event)
         virtio_serial_open(&vcon->port);
         break;
     case CHR_EVENT_CLOSED:
+        if (vcon->watch) {
+            g_source_remove(vcon->watch);
+            vcon->watch = 0;
+        }
         virtio_serial_close(&vcon->port);
         break;
     }
@@ -139,6 +147,17 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
     return 0;
 }
 
+static int virtconsole_exitfn(VirtIOSerialPort *port)
+{
+    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+
+    if (vcon->watch) {
+        g_source_remove(vcon->watch);
+    }
+
+    return 0;
+}
+
 static Property virtconsole_properties[] = {
     DEFINE_PROP_CHR("chardev", VirtConsole, chr),
     DEFINE_PROP_END_OF_LIST(),
@@ -151,6 +170,7 @@ static void virtconsole_class_init(ObjectClass *klass, void *data)
 
     k->is_console = true;
     k->init = virtconsole_initfn;
+    k->exit = virtconsole_exitfn;
     k->have_data = flush_buf;
     k->guest_open = guest_open;
     k->guest_close = guest_close;
-- 
1.8.1.4

[Qemu-devel] [PATCH 3/8] spice-qemu-char: Remove #ifdef-ed code for old spice-server compat

[Hans de Goede]

We now require spice-server to be >= 0.12.0 so this is no longer needed.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 spice-qemu-char.c | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 8a9236d..b810b30 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -85,21 +85,6 @@ static void vmc_state(SpiceCharDeviceInstance *sin, int connected)
 {
     SpiceCharDriver *scd = container_of(sin, SpiceCharDriver, sin);
 
-#if SPICE_SERVER_VERSION < 0x000901
-    /*
-     * spice-server calls the state callback for the agent channel when the
-     * spice client connects / disconnects. Given that not the client but
-     * the server is doing the parsing of the messages this is wrong as the
-     * server is still listening. Worse, this causes the parser in the server
-     * to go out of sync, so we ignore state calls for subtype vdagent
-     * spicevmc chardevs. For the full story see:
-     * http://lists.freedesktop.org/archives/spice-devel/2011-July/004837.html
-     */
-    if (strcmp(sin->subtype, "vdagent") == 0) {
-        return;
-    }
-#endif
-
     if ((scd->chr->opened && connected) ||
         (!scd->chr->opened && !connected)) {
         return;
@@ -228,7 +213,6 @@ static CharDriverState *chr_open(const char *subtype)
 
 CharDriverState *qemu_chr_open_spice_vmc(const char *type)
 {
-    CharDriverState *chr;
     const char **psubtype = spice_server_char_device_recognized_subtypes();
 
     if (type == NULL) {
@@ -247,16 +231,7 @@ CharDriverState *qemu_chr_open_spice_vmc(const char *type)
         return NULL;
     }
 
-    chr = chr_open(type);
-
-#if SPICE_SERVER_VERSION < 0x000901
-    /* See comment in vmc_state() */
-    if (strcmp(type, "vdagent") == 0) {
-        qemu_chr_generic_open(chr);
-    }
-#endif
-
-    return chr;
+    return chr_open(type);
 }
 
 #if SPICE_SERVER_VERSION >= 0x000c02
-- 
1.8.1.4

[Qemu-devel] [PATCH 4/8] spice-qemu-char: Add watch support

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 spice-qemu-char.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 63 insertions(+), 4 deletions(-)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index b810b30..ddaf5b3 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -13,12 +13,18 @@ typedef struct SpiceCharDriver {
     SpiceCharDeviceInstance     sin;
     char                  *subtype;
     bool                  active;
+    bool                  blocked;
     uint8_t               *buffer;
     uint8_t               *datapos;
     ssize_t               bufsize, datalen;
     QLIST_ENTRY(SpiceCharDriver) next;
 } SpiceCharDriver;
 
+typedef struct SpiceCharSource {
+    GSource               source;
+    SpiceCharDriver       *scd;
+} SpiceCharSource;
+
 static QLIST_HEAD(, SpiceCharDriver) spice_chars =
     QLIST_HEAD_INITIALIZER(spice_chars);
 
@@ -54,9 +60,10 @@ static int vmc_read(SpiceCharDeviceInstance *sin, uint8_t *buf, int len)
         scd->datapos += bytes;
         scd->datalen -= bytes;
         assert(scd->datalen >= 0);
-        if (scd->datalen == 0) {
-            scd->datapos = 0;
-        }
+    }
+    if (scd->datalen == 0) {
+        scd->datapos = 0;
+        scd->blocked = false;
     }
     trace_spice_vmc_read(bytes, len);
     return bytes;
@@ -129,10 +136,54 @@ static void vmc_unregister_interface(SpiceCharDriver *scd)
     trace_spice_vmc_unregister_interface(scd);
 }
 
+static gboolean spice_char_source_prepare(GSource *source, gint *timeout)
+{
+    SpiceCharSource *src = (SpiceCharSource *)source;
+
+    *timeout = -1;
+
+    return !src->scd->blocked;
+}
+
+static gboolean spice_char_source_check(GSource *source)
+{
+    SpiceCharSource *src = (SpiceCharSource *)source;
+
+    return !src->scd->blocked;
+}
+
+static gboolean spice_char_source_dispatch(GSource *source,
+    GSourceFunc callback, gpointer user_data)
+{
+    GIOFunc func = (GIOFunc)callback;
+
+    return func(NULL, G_IO_OUT, user_data);
+}
+
+GSourceFuncs SpiceCharSourceFuncs = {
+    .prepare  = spice_char_source_prepare,
+    .check    = spice_char_source_check,
+    .dispatch = spice_char_source_dispatch,
+};
+
+static GSource *spice_chr_add_watch(CharDriverState *chr, GIOCondition cond)
+{
+    SpiceCharDriver *scd = chr->opaque;
+    SpiceCharSource *src;
+
+    assert(cond == G_IO_OUT);
+
+    src = (SpiceCharSource *)g_source_new(&SpiceCharSourceFuncs,
+                                          sizeof(SpiceCharSource));
+    src->scd = scd;
+
+    return (GSource *)src;
+}
 
 static int spice_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     SpiceCharDriver *s = chr->opaque;
+    int read_bytes;
 
     vmc_register_interface(s);
     assert(s->datalen == 0);
@@ -144,7 +195,14 @@ static int spice_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
     s->datapos = s->buffer;
     s->datalen = len;
     spice_server_char_device_wakeup(&s->sin);
-    return len;
+    read_bytes = len - s->datalen;
+    if (read_bytes != len) {
+        /* We'll get passed in the unconsumed data with the next call */
+        s->datalen = 0;
+        s->datapos = NULL;
+        s->blocked = true;
+    }
+    return read_bytes;
 }
 
 static void spice_chr_close(struct CharDriverState *chr)
@@ -202,6 +260,7 @@ static CharDriverState *chr_open(const char *subtype)
     s->sin.subtype = g_strdup(subtype);
     chr->opaque = s;
     chr->chr_write = spice_chr_write;
+    chr->chr_add_watch = spice_chr_add_watch;
     chr->chr_close = spice_chr_close;
     chr->chr_guest_open = spice_chr_guest_open;
     chr->chr_guest_close = spice_chr_guest_close;
-- 
1.8.1.4

[Qemu-devel] [PATCH 5/8] spice-qemu-char: Remove intermediate buffer

[Hans de Goede]

From: Alon Levy <alevy@redhat.com>

virtio-serial's buffer is valid when it calls us, and we don't
access it otherwise: vmc_read is only called in response to wakeup,
or else we set datalen=0 and throttle. Then vmc_read is called back,
we return 0 (not accessing the buffer) and set the timer to unthrottle.

Also make datalen int and not ssize_t (to fit spice_chr_write signature).

HdG: Update to apply to spice-qemu-char with new gio-channel based
flowcontrol support.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 spice-qemu-char.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index ddaf5b3..607abb6 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -14,9 +14,8 @@ typedef struct SpiceCharDriver {
     char                  *subtype;
     bool                  active;
     bool                  blocked;
-    uint8_t               *buffer;
-    uint8_t               *datapos;
-    ssize_t               bufsize, datalen;
+    const uint8_t         *datapos;
+    int                   datalen;
     QLIST_ENTRY(SpiceCharDriver) next;
 } SpiceCharDriver;
 
@@ -187,12 +186,7 @@ static int spice_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 
     vmc_register_interface(s);
     assert(s->datalen == 0);
-    if (s->bufsize < len) {
-        s->bufsize = len;
-        s->buffer = g_realloc(s->buffer, s->bufsize);
-    }
-    memcpy(s->buffer, buf, len);
-    s->datapos = s->buffer;
+    s->datapos = buf;
     s->datalen = len;
     spice_server_char_device_wakeup(&s->sin);
     read_bytes = len - s->datalen;
-- 
1.8.1.4

[Qemu-devel] [PATCH 6/8] spice-qemu-char: Move spice_chr_close down

[Hans de Goede]

This puts it in a more logical place and avoids the need for forward
declarations in the next patch of this series.

Note no code changes, just moving it around.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 spice-qemu-char.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 607abb6..530d9a6 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -199,6 +199,18 @@ static int spice_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
     return read_bytes;
 }
 
+static void spice_chr_guest_open(struct CharDriverState *chr)
+{
+    SpiceCharDriver *s = chr->opaque;
+    vmc_register_interface(s);
+}
+
+static void spice_chr_guest_close(struct CharDriverState *chr)
+{
+    SpiceCharDriver *s = chr->opaque;
+    vmc_unregister_interface(s);
+}
+
 static void spice_chr_close(struct CharDriverState *chr)
 {
     SpiceCharDriver *s = chr->opaque;
@@ -213,18 +225,6 @@ static void spice_chr_close(struct CharDriverState *chr)
     g_free(s);
 }
 
-static void spice_chr_guest_open(struct CharDriverState *chr)
-{
-    SpiceCharDriver *s = chr->opaque;
-    vmc_register_interface(s);
-}
-
-static void spice_chr_guest_close(struct CharDriverState *chr)
-{
-    SpiceCharDriver *s = chr->opaque;
-    vmc_unregister_interface(s);
-}
-
 static void print_allowed_subtypes(void)
 {
     const char** psubtype;
-- 
1.8.1.4

[Qemu-devel] [PATCH 7/8] spice-qemu-char: Register interface on post load

[Hans de Goede]

From: Alon Levy <alevy@redhat.com>

The target has not seen the guest_connected event via
spice_chr_guest_open or spice_chr_write, and so spice server wrongly
assumes there is no agent active, while the client continues to send
motion events only by the agent channel, which the server ignores. The
net effect is that the mouse is static in the guest.

By registering the interface on post load spice server will pass on the
agent messages fixing the mouse behavior after migration.

RHBZ #725965

HdG: Port to new chardev code and test

Signed-off-by: Alon Levy <alevy@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 spice-qemu-char.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 530d9a6..bdbe59e 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -2,6 +2,7 @@
 #include "trace.h"
 #include "ui/qemu-spice.h"
 #include "char/char.h"
+#include "migration/vmstate.h"
 #include <spice.h>
 #include <spice-experimental.h>
 #include <spice/protocol.h>
@@ -16,6 +17,8 @@ typedef struct SpiceCharDriver {
     bool                  blocked;
     const uint8_t         *datapos;
     int                   datalen;
+    uint32_t              guest_open;
+    QEMUTimer             *post_load_timer;
     QLIST_ENTRY(SpiceCharDriver) next;
 } SpiceCharDriver;
 
@@ -116,6 +119,8 @@ static SpiceCharDeviceInterface vmc_interface = {
 
 static void vmc_register_interface(SpiceCharDriver *scd)
 {
+    scd->guest_open = 1;
+
     if (scd->active) {
         return;
     }
@@ -127,6 +132,8 @@ static void vmc_register_interface(SpiceCharDriver *scd)
 
 static void vmc_unregister_interface(SpiceCharDriver *scd)
 {
+    scd->guest_open = 0;
+
     if (!scd->active) {
         return;
     }
@@ -211,6 +218,35 @@ static void spice_chr_guest_close(struct CharDriverState *chr)
     vmc_unregister_interface(s);
 }
 
+static void spice_chr_post_load_cb(void *opaque)
+{
+    SpiceCharDriver *s = opaque;
+
+    if (s->chr && s->guest_open)
+        spice_chr_guest_open(s->chr);
+}
+
+static int spice_chr_post_load(void *opaque, int version_id)
+{
+    SpiceCharDriver *s = opaque;
+
+    if (s->chr && s->guest_open)
+        qemu_mod_timer(s->post_load_timer, 1);
+
+    return 0;
+}
+
+static VMStateDescription spice_chr_vmstate = {
+    .name               = "spice-chr",
+    .version_id         = 1,
+    .minimum_version_id = 1,
+    .post_load          = spice_chr_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(guest_open, SpiceCharDriver),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static void spice_chr_close(struct CharDriverState *chr)
 {
     SpiceCharDriver *s = chr->opaque;
@@ -218,6 +254,10 @@ static void spice_chr_close(struct CharDriverState *chr)
     vmc_unregister_interface(s);
     QLIST_REMOVE(s, next);
 
+    qemu_del_timer(s->post_load_timer);
+    qemu_free_timer(s->post_load_timer);
+    vmstate_unregister(NULL, &spice_chr_vmstate, s);
+
     g_free((char *)s->sin.subtype);
 #if SPICE_SERVER_VERSION >= 0x000c02
     g_free((char *)s->sin.portname);
@@ -252,6 +292,8 @@ static CharDriverState *chr_open(const char *subtype)
     s->chr = chr;
     s->active = false;
     s->sin.subtype = g_strdup(subtype);
+    s->post_load_timer = qemu_new_timer_ns(vm_clock,
+                                           spice_chr_post_load_cb, s);
     chr->opaque = s;
     chr->chr_write = spice_chr_write;
     chr->chr_add_watch = spice_chr_add_watch;
@@ -259,6 +301,8 @@ static CharDriverState *chr_open(const char *subtype)
     chr->chr_guest_open = spice_chr_guest_open;
     chr->chr_guest_close = spice_chr_guest_close;
 
+    vmstate_register(NULL, -1, &spice_chr_vmstate, s);
+
     QLIST_INSERT_HEAD(&spice_chars, s, next);
 
     return chr;
-- 
1.8.1.4

[Qemu-devel] [PATCH 8/8] usb-redir: Add flow control support

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/redirect.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index c519b9b..9ba714d 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -104,6 +104,8 @@ struct USBRedirDevice {
     /* Data passed from chardev the fd_read cb to the usbredirparser read cb */
     const uint8_t *read_buf;
     int read_buf_size;
+    /* Active chardev-watch-tag */
+    guint watch;
     /* For async handling of close */
     QEMUBH *chardev_close_bh;
     /* To delay the usb attach in case of quick chardev close + open */
@@ -254,9 +256,21 @@ static int usbredir_read(void *priv, uint8_t *data, int count)
     return count;
 }
 
+static gboolean usbredir_write_unblocked(GIOChannel *chan, GIOCondition cond,
+                                         void *opaque)
+{
+    USBRedirDevice *dev = opaque;
+
+    dev->watch = 0;
+    usbredirparser_do_write(dev->parser);
+
+    return FALSE;
+}
+
 static int usbredir_write(void *priv, uint8_t *data, int count)
 {
     USBRedirDevice *dev = priv;
+    int r;
 
     if (!dev->cs->opened) {
         return 0;
@@ -267,7 +281,16 @@ static int usbredir_write(void *priv, uint8_t *data, int count)
         return 0;
     }
 
-    return qemu_chr_fe_write(dev->cs, data, count);
+    r = qemu_chr_fe_write(dev->cs, data, count);
+    if (r < count) {
+        if (!dev->watch) {
+            dev->watch = qemu_chr_fe_add_watch(dev->cs, G_IO_OUT,
+                                               usbredir_write_unblocked, dev);
+        }
+        if (r < 0)
+            r = 0;
+    }
+    return r;
 }
 
 /*
@@ -1085,6 +1108,10 @@ static void usbredir_chardev_close_bh(void *opaque)
         usbredirparser_destroy(dev->parser);
         dev->parser = NULL;
     }
+    if (dev->watch) {
+        g_source_remove(dev->watch);
+        dev->watch = 0;
+    }
 }
 
 static void usbredir_create_parser(USBRedirDevice *dev)
@@ -1319,6 +1346,9 @@ static void usbredir_handle_destroy(USBDevice *udev)
     if (dev->parser) {
         usbredirparser_destroy(dev->parser);
     }
+    if (dev->watch) {
+        g_source_remove(dev->watch);
+    }
 
     free(dev->filter_rules);
 }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/8] virtio-console: Remove any pending watches on close

[Amit Shah]

On (Thu) 14 Mar 2013 [17:36:51], Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Amit Shah <amit.shah@redhat.com>

		Amit

Re: [Qemu-devel] Spice / usb-redir chardev flowcontrol patches v2

[Amit Shah]

On (Thu) 14 Mar 2013 [17:36:49], Hans de Goede wrote:
> Here is v2 of the series adding watch support to the spicevmc chardev backend
> and flowcontrol support to the usb-redir device. It also includes a few
> virtio-consoled bugfixes which were found during the development of this
> series.
> 
> Note that this series is based *on top of* Gerd Hoffmann's chardev.5 series
> to avoid conflicts with that series.

Looks good.

Acked-by: Amit Shah <amit.shah@redhat.com>

Gerd, will you pick these up via your tree?

		Amit

Re: [Qemu-devel] [PATCH 7/8] spice-qemu-char: Register interface on post load

[Gerd Hoffmann]

On 03/14/13 17:36, Hans de Goede wrote:
> From: Alon Levy <alevy@redhat.com>
> 
> The target has not seen the guest_connected event via
> spice_chr_guest_open or spice_chr_write, and so spice server wrongly
> assumes there is no agent active, while the client continues to send
> motion events only by the agent channel, which the server ignores. The
> net effect is that the mouse is static in the guest.
> 
> By registering the interface on post load spice server will pass on the
> agent messages fixing the mouse behavior after migration.

> +static VMStateDescription spice_chr_vmstate = {
> +    .name               = "spice-chr",
> +    .version_id         = 1,
> +    .minimum_version_id = 1,
> +    .post_load          = spice_chr_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(guest_open, SpiceCharDriver),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};

> +    vmstate_register(NULL, -1, &spice_chr_vmstate, s);
> +

That is a showstopper.  If there are two of these there is no reliable
way to figure which is which.

Do we actually need that in the first place?  I assume virtio-serial
will migrate the guest-open state anyway.  So if we can use that somehow
we don't have to live migrate the chardev state ...

cheers,
  Gerd

Re: [Qemu-devel] Spice / usb-redir chardev flowcontrol patches v2

[Gerd Hoffmann]

On 03/18/13 13:21, Amit Shah wrote:
> On (Thu) 14 Mar 2013 [17:36:49], Hans de Goede wrote:
>> Here is v2 of the series adding watch support to the spicevmc chardev backend
>> and flowcontrol support to the usb-redir device. It also includes a few
>> virtio-consoled bugfixes which were found during the development of this
>> series.
>>
>> Note that this series is based *on top of* Gerd Hoffmann's chardev.5 series
>> to avoid conflicts with that series.
> 
> Looks good.
> 
> Acked-by: Amit Shah <amit.shah@redhat.com>
> 
> Gerd, will you pick these up via your tree?

I'll go pick the usbredir patch into the usb queue.  The spice bits need
some discussion, but with your ack I'll can take them through the spice
queue once the remaining issue is settled.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 7/8] spice-qemu-char: Register interface on post load

[Alon Levy]

> On 03/14/13 17:36, Hans de Goede wrote:
> > From: Alon Levy <alevy@redhat.com>
> > 
> > The target has not seen the guest_connected event via
> > spice_chr_guest_open or spice_chr_write, and so spice server
> > wrongly
> > assumes there is no agent active, while the client continues to
> > send
> > motion events only by the agent channel, which the server ignores.
> > The
> > net effect is that the mouse is static in the guest.
> > 
> > By registering the interface on post load spice server will pass on
> > the
> > agent messages fixing the mouse behavior after migration.
> 
> > +static VMStateDescription spice_chr_vmstate = {
> > +    .name               = "spice-chr",
> > +    .version_id         = 1,
> > +    .minimum_version_id = 1,
> > +    .post_load          = spice_chr_post_load,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT32(guest_open, SpiceCharDriver),
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> 
> > +    vmstate_register(NULL, -1, &spice_chr_vmstate, s);
> > +
> 
> That is a showstopper.  If there are two of these there is no
> reliable
> way to figure which is which.

But they will both get a different state pointer s.

> 
> Do we actually need that in the first place?  I assume virtio-serial
> will migrate the guest-open state anyway.  So if we can use that
> somehow
> we don't have to live migrate the chardev state ...

It is being migrated in virtio-serial-bus (port->guest_connected) but we need to pass that on to spice-server, which there is no hook for in the chardev layer, I could add one. I would need to query the backend, in this case virtio-serial-bus, something like this:

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index e2d1c58..f36a973 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -88,6 +88,16 @@ static void guest_close(VirtIOSerialPort *port)
     qemu_chr_fe_close(vcon->chr);
 }
 
+static void guest_migrated(VirtIOSerialPort *port)
+{
+    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+
+    if (!vcon->chr) {
+        return;
+    }
+    qemu_chr_fe_migrated(vcon->chr, port->guest_connected);
+}
+
 /* Readiness of the guest to accept data on a port */
 static int chr_can_read(void *opaque)
 {
@@ -177,6 +187,7 @@ static void virtserialport_class_init(ObjectClass *klass, void *data)
     k->have_data = flush_buf;
     k->guest_open = guest_open;
     k->guest_close = guest_close;
+    k->guest_migrated = guest_migrated;
     dc->props = virtserialport_properties;
 }
 
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 7d0515f..b6b1c3b 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -664,6 +664,7 @@ static int fetch_active_ports_list(QEMUFile *f, int version_id,
     /* Items in struct VirtIOSerialPort */
     for (i = 0; i < nr_active_ports; i++) {
         VirtIOSerialPort *port;
+        VirtIOSerialPortClass *vsc;
         uint32_t id;
 
         id = qemu_get_be32(f);
@@ -671,6 +672,7 @@ static int fetch_active_ports_list(QEMUFile *f, int version_id,
         if (!port) {
             return -EINVAL;
         }
+        vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
 
         port->guest_connected = qemu_get_byte(f);
         s->post_load->connected[i].port = port;
@@ -698,6 +700,7 @@ static int fetch_active_ports_list(QEMUFile *f, int version_id,
                 virtio_serial_throttle_port(port, false);
             }
         }
+        vsc->guest_migrated(port);
     }
     qemu_mod_timer(s->post_load->timer, 1);
     return 0;
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index d2d9fb7..992ef55 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -107,6 +107,8 @@ typedef struct VirtIOSerialPortClass {
      */
     ssize_t (*have_data)(VirtIOSerialPort *port, const uint8_t *buf,
                          size_t len);
+
+    void (*guest_migrated)(VirtIOSerialPort *port);
 } VirtIOSerialPortClass;
 
 /*
diff --git a/include/char/char.h b/include/char/char.h
index d6a0351..b7c8cb1 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -70,6 +70,7 @@ struct CharDriverState {
     void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
     void (*chr_guest_open)(struct CharDriverState *chr);
     void (*chr_guest_close)(struct CharDriverState *chr);
+    void (*chr_guest_migrated)(struct CharDriverState *chr, int connected);
     void *opaque;
     int idle_tag;
     char *label;
@@ -143,6 +144,8 @@ void qemu_chr_fe_open(struct CharDriverState *chr);
  */
 void qemu_chr_fe_close(struct CharDriverState *chr);
 
+void qemu_chr_fe_migrated(struct CharDriverState *chr, int connected);
+
 /**
  * @qemu_chr_fe_printf:
  *
diff --git a/qemu-char.c b/qemu-char.c
index e633797..cda785f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3390,6 +3390,13 @@ void qemu_chr_fe_open(struct CharDriverState *chr)
     }
 }
 
+void qemu_chr_fe_migrated(struct CharDriverState *chr, int connected)
+{
+    if (chr->chr_guest_migrated) {
+        chr->chr_guest_migrated(chr, connected);
+    }
+}
+
 void qemu_chr_fe_close(struct CharDriverState *chr)
 {
     if (chr->chr_guest_close) {
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 8a9236d..11a1888 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -205,6 +205,14 @@ static void print_allowed_subtypes(void)
     fprintf(stderr, "\n");
 }
 
+static void spice_chr_guest_migrated(struct CharDriverState *chr,
+                                     int connected)
+{
+    if (connected) {
+        // setup timer for spice_chr_guest_open
+    }
+}
+
 static CharDriverState *chr_open(const char *subtype)
 {
     CharDriverState *chr;
@@ -220,6 +228,7 @@ static CharDriverState *chr_open(const char *subtype)
     chr->chr_close = spice_chr_close;
     chr->chr_guest_open = spice_chr_guest_open;
     chr->chr_guest_close = spice_chr_guest_close;
+    chr->chr_guest_migrated = spice_chr_guest_migrated;
 
     QLIST_INSERT_HEAD(&spice_chars, s, next);
 


> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH 7/8] spice-qemu-char: Register interface on post load

[Gerd Hoffmann]

On 03/19/13 10:15, Alon Levy wrote:

>>> +    vmstate_register(NULL, -1, &spice_chr_vmstate, s);
>>> +
>>
>> That is a showstopper.  If there are two of these there is no
>> reliable
>> way to figure which is which.
> 
> But they will both get a different state pointer s.

I mean in the migration data stream.

> It is being migrated in virtio-serial-bus (port->guest_connected)
> but we need to pass that on to spice-server, which there is no hook
> for in the chardev layer, I could add one. I would need to query the
> backend, in this case virtio-serial-bus, something like this:

That is better IMHO.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 8/8] usb-redir: Add flow control support

[Gerd Hoffmann]

On 03/14/13 17:36, Hans de Goede wrote:
> +        if (!dev->watch) {
> +            dev->watch = qemu_chr_fe_add_watch(dev->cs, G_IO_OUT,
> +                                               usbredir_write_unblocked, dev);
> +        }
> +        if (r < 0)
> +            r = 0;
> +    }
> +    return r;

=== checkpatch complains ===
WARNING: braces {} are necessary for all arms of this statement
#48: FILE: hw/usb/redirect.c:290:
+        if (r < 0)
[...]

total: 0 errors, 1 warnings, 65 lines checked

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 8/8] usb-redir: Add flow control support

[Hans de Goede]

Hi,

Oops. v2 is on its way.

Regards,

Hans


On 03/19/2013 01:40 PM, Gerd Hoffmann wrote:
> On 03/14/13 17:36, Hans de Goede wrote:
>> +        if (!dev->watch) {
>> +            dev->watch = qemu_chr_fe_add_watch(dev->cs, G_IO_OUT,
>> +                                               usbredir_write_unblocked, dev);
>> +        }
>> +        if (r < 0)
>> +            r = 0;
>> +    }
>> +    return r;
>
> === checkpatch complains ===
> WARNING: braces {} are necessary for all arms of this statement
> #48: FILE: hw/usb/redirect.c:290:
> +        if (r < 0)
> [...]
>
> total: 0 errors, 1 warnings, 65 lines checked
>
> cheers,
>    Gerd
>