[Qemu-devel] [PULL 0/9] spice patch queue

[Qemu-devel] [PULL 0/9] spice patch queue

[Gerd Hoffmann]

  Hi,

This is the spice patch queue, carrying flow control fixes,
for spice chardevs and (with Amit's ack) virtio-serial.  Also
some new resolutions for qxl.

please pull,
  Gerd

The following changes since commit 24a6e7f4d91e9ed5f8117ecb083431a23f8609a0:

  virtio-balloon: fix dynamic properties. (2013-04-15 17:06:58 -0500)

are available in the git repository at:

  git://anongit.freedesktop.org/spice/qemu spice.v69

for you to fetch changes up to 75c439bc65c07d76f5e74c734ed5432bc6114a3b:

  spice-qemu-char: vmc_write: Don't write more bytes then we're asked too (2013-04-16 11:52:09 +0200)

----------------------------------------------------------------
Alon Levy (2):
      spice: (32 bit only) fix surface cmd tracking destruction
      spice-qemu-char: Remove intermediate buffer

Gerd Hoffmann (2):
      qxl: add 4k + 8k resolutions
      qxl: add 2000x2000 and 2048x2048 video modes

Hans de Goede (5):
      virtio-console: Also throttle when less was written then requested
      virtio-console: Remove any pending watches on close
      spice-qemu-char: Remove #ifdef-ed code for old spice-server compat
      spice-qemu-char: Add watch support
      spice-qemu-char: vmc_write: Don't write more bytes then we're asked too

 hw/char/virtio-console.c          |   32 +++++++++--
 hw/display/qxl.c                  |    8 ++-
 include/hw/virtio/virtio-serial.h |    2 +-
 spice-qemu-char.c                 |  109 +++++++++++++++++++++++--------------
 4 files changed, 104 insertions(+), 47 deletions(-)

[Qemu-devel] [PATCH 1/9] qxl: add 4k + 8k resolutions

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 930b7cf..9d8ab58 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -116,6 +116,10 @@ static QXLMode qxl_modes[] = {
     QXL_MODE_EX(2560, 2048),
     QXL_MODE_EX(2800, 2100),
     QXL_MODE_EX(3200, 2400),
+    QXL_MODE_EX(3840, 2160), /* 4k mainstream */
+    QXL_MODE_EX(4096, 2160), /* 4k            */
+    QXL_MODE_EX(7680, 4320), /* 8k mainstream */
+    QXL_MODE_EX(8192, 4320), /* 8k            */
 };
 
 static void qxl_send_events(PCIQXLDevice *d, uint32_t events);
-- 
1.7.9.7

[Qemu-devel] [PATCH 2/9] qxl: add 2000x2000 and 2048x2048 video modes

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 9d8ab58..1f7c8fe 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -109,7 +109,9 @@ static QXLMode qxl_modes[] = {
     /* these modes need more than 8 MB video memory */
     QXL_MODE_EX(1920, 1200),
     QXL_MODE_EX(1920, 1440),
+    QXL_MODE_EX(2000, 2000),
     QXL_MODE_EX(2048, 1536),
+    QXL_MODE_EX(2048, 2048),
     QXL_MODE_EX(2560, 1440),
     QXL_MODE_EX(2560, 1600),
     /* these modes need more than 16 MB video memory */
-- 
1.7.9.7

[Qemu-devel] [PATCH 3/9] spice: (32 bit only) fix surface cmd tracking destruction

[Gerd Hoffmann]

From: Alon Levy <alevy@redhat.com>

No change for 64 bit arches, but for 32 bit previously we zeroed half
the surfaces cmd array, instead of all of it.

Signed-off-by: Alon Levy <alevy@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 1f7c8fe..cb47995 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -230,7 +230,7 @@ static void qxl_spice_destroy_surfaces_complete(PCIQXLDevice *qxl)
     trace_qxl_spice_destroy_surfaces_complete(qxl->id);
     qemu_mutex_lock(&qxl->track_lock);
     memset(qxl->guest_surfaces.cmds, 0,
-           sizeof(qxl->guest_surfaces.cmds) * qxl->ssd.num_surfaces);
+           sizeof(qxl->guest_surfaces.cmds[0]) * qxl->ssd.num_surfaces);
     qxl->guest_surfaces.count = 0;
     qemu_mutex_unlock(&qxl->track_lock);
 }
-- 
1.7.9.7

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

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

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

This patch also changes the len argument to the have_data callback, to
avoid doing an unsigned signed comparison.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/char/virtio-console.c          |    8 +++++---
 include/hw/virtio/virtio-serial.h |    2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 5035030..061f4bd 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -34,7 +34,8 @@ static gboolean chr_write_unblocked(GIOChannel *chan, GIOCondition cond,
 }
 
 /* Callback function that's called when the guest sends us data */
-static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
+static ssize_t flush_buf(VirtIOSerialPort *port,
+                         const uint8_t *buf, ssize_t len)
 {
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
     ssize_t ret;
@@ -47,7 +48,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 +57,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,
diff --git a/include/hw/virtio/virtio-serial.h b/include/hw/virtio/virtio-serial.h
index 7c71304..1d2040b 100644
--- a/include/hw/virtio/virtio-serial.h
+++ b/include/hw/virtio/virtio-serial.h
@@ -104,7 +104,7 @@ typedef struct VirtIOSerialPortClass {
      * 'len'.  In this case, throttling will be enabled for this port.
      */
     ssize_t (*have_data)(VirtIOSerialPort *port, const uint8_t *buf,
-                         size_t len);
+                         ssize_t len);
 } VirtIOSerialPortClass;
 
 /*
-- 
1.7.9.7

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

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

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

diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 061f4bd..6759e51 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/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;
 }
@@ -61,8 +63,10 @@ static ssize_t flush_buf(VirtIOSerialPort *port,
             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;
@@ -106,6 +110,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;
     }
@@ -130,6 +138,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(),
@@ -142,6 +161,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->set_guest_connected = set_guest_connected;
     dc->props = virtconsole_properties;
-- 
1.7.9.7

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

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

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>
Signed-off-by: Gerd Hoffmann <kraxel@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 c9403de..be19917 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->be_open && connected) ||
         (!scd->chr->be_open && !connected)) {
         return;
@@ -224,7 +209,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) {
@@ -243,16 +227,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.7.9.7

[Qemu-devel] [PATCH 7/9] spice-qemu-char: Add watch support

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@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 be19917..7e551bf 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;
 
     assert(s->datalen == 0);
     if (s->bufsize < len) {
@@ -143,7 +194,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)
@@ -199,6 +257,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_set_fe_open = spice_chr_set_fe_open;
 
-- 
1.7.9.7

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

[Gerd Hoffmann]

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>
Signed-off-by: Gerd Hoffmann <kraxel@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 7e551bf..ff95fcb 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;
 
@@ -186,12 +185,7 @@ static int spice_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
     int read_bytes;
 
     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.7.9.7

[Qemu-devel] [PATCH 9/9] spice-qemu-char: vmc_write: Don't write more bytes then we're asked too

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

This one took me eons to debug, but I've finally found it now, oh well.

The usage of the MIN macro in this line:
    last_out = MIN(len, qemu_chr_be_can_write(scd->chr));

Causes qemu_chr_be_can_write to be called *twice*, since the MIN macro
evaluates its arguments twice (bad MIN macro, bad!). And the result of
the call can change between the 2 calls since the guest may have consumed
some data from the virtio ringbuffer between the calls!

When this happens it is possible for qemu_chr_be_can_write to return less
then len in the call made for the comparision, and then to return more then
len in the actual call for the return-value of MIN, after which we will end
up writing len data + some extra garbage, not good.

This patch fixes this by only calling qemu_chr_be_can_write once.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 spice-qemu-char.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index ff95fcb..f10970c 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -35,7 +35,8 @@ static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t *buf, int len)
     uint8_t* p = (uint8_t*)buf;
 
     while (len > 0) {
-        last_out = MIN(len, qemu_chr_be_can_write(scd->chr));
+        int can_write = qemu_chr_be_can_write(scd->chr);
+        last_out = MIN(len, can_write);
         if (last_out <= 0) {
             break;
         }
-- 
1.7.9.7