[Qemu-devel] [STABLE PULL] spice: locking fixes

[Qemu-devel] [STABLE PULL] spice: locking fixes

[Gerd Hoffmann]

  Hi,

Here are the spice locking fixes backported to the 0.14 stable branch.

please pull,
  Gerd

The following changes since commit 56a60dd6d619877e9957ba06b92d2f276e3c229d:

  Version 0.14.1 (2011-05-04 13:50:56 -0500)

are available in the git repository at:
  git://anongit.freedesktop.org/spice/qemu spice.stable.14

Gerd Hoffmann (4):
      spice: enable thread support
      spice: don't create updates in spice server context.
      spice: don't call displaystate callbacks from spice server context.
      spice: drop obsolete iothread locking

Jes Sorensen (1):
      Make spice dummy functions inline to fix calls not checking return values

 configure          |    1 +
 hw/qxl-render.c    |   25 ++++++++++-----------
 hw/qxl.c           |   27 +++++++++++------------
 ui/qemu-spice.h    |   12 ++++++++-
 ui/spice-display.c |   61 +++++++++++++++++++++++++++++++++------------------
 ui/spice-display.h |   24 ++++++++++++++++----
 6 files changed, 94 insertions(+), 56 deletions(-)

[Qemu-devel] [PATCH 1/5] Make spice dummy functions inline to fix calls not checking return values

[Gerd Hoffmann]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

qemu_spice_set_passwd() and qemu_spice_set_pw_expire() dummy functions
needs to be inline, in order to handle the case where they are called
without checking the return value.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
(cherry picked from commit 14da8345b2f7c21bab20fd12b755a61d6277f171)
---
 ui/qemu-spice.h |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
index 916e5dc..3c6f1fe 100644
--- a/ui/qemu-spice.h
+++ b/ui/qemu-spice.h
@@ -47,8 +47,16 @@ CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
 #else  /* CONFIG_SPICE */
 
 #define using_spice 0
-#define qemu_spice_set_passwd(_p, _f1, _f2) (-1)
-#define qemu_spice_set_pw_expire(_e) (-1)
+static inline int qemu_spice_set_passwd(const char *passwd,
+                                        bool fail_if_connected,
+                                        bool disconnect_if_connected)
+{
+    return -1;
+}
+static inline int qemu_spice_set_pw_expire(time_t expires)
+{
+    return -1;
+}
 static inline int qemu_spice_migrate_info(const char *h, int p, int t, const char *s)
 { return -1; }
 
-- 
1.7.1

[Qemu-devel] [PATCH 2/5] spice: enable thread support

[Gerd Hoffmann]

The spice locking fixes cherry-picked from master need qemu mutexes.
Enable CONFIG_THREAD when enabling spice so they get linked in.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 configure |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index f2551b3..b2eade1 100755
--- a/configure
+++ b/configure
@@ -2742,6 +2742,7 @@ fi
 
 if test "$spice" = "yes" ; then
   echo "CONFIG_SPICE=y" >> $config_host_mak
+  echo "CONFIG_THREAD=y" >> $config_host_mak
 fi
 
 # XXX: suppress that
-- 
1.7.1

[Qemu-devel] [PATCH 3/5] spice: don't create updates in spice server context.

[Gerd Hoffmann]

This patch moves the creation of spice screen updates from the spice
server context to qemu iothread context (display refresh timer to be
exact).  This way we avoid accessing qemu internals (display surface)
from spice thread context which in turn allows us to simplify locking.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
(cherry picked from commit e0c64d08d11736dcea7c5a6373e3e7f62db51d9e)
---
 hw/qxl.c           |   17 +++++++++++------
 ui/spice-display.c |   43 +++++++++++++++++++++++++++----------------
 ui/spice-display.h |   21 ++++++++++++++++-----
 3 files changed, 54 insertions(+), 27 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index fe4212b..bd250db 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -343,18 +343,22 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
     SimpleSpiceUpdate *update;
     QXLCommandRing *ring;
     QXLCommand *cmd;
-    int notify;
+    int notify, ret;
 
     switch (qxl->mode) {
     case QXL_MODE_VGA:
         dprint(qxl, 2, "%s: vga\n", __FUNCTION__);
-        update = qemu_spice_create_update(&qxl->ssd);
-        if (update == NULL) {
-            return false;
+        ret = false;
+        qemu_mutex_lock(&qxl->ssd.lock);
+        if (qxl->ssd.update != NULL) {
+            update = qxl->ssd.update;
+            qxl->ssd.update = NULL;
+            *ext = update->ext;
+            ret = true;
         }
-        *ext = update->ext;
+        qemu_mutex_unlock(&qxl->ssd.lock);
         qxl_log_command(qxl, "vga", ext);
-        return true;
+        return ret;
     case QXL_MODE_COMPAT:
     case QXL_MODE_NATIVE:
     case QXL_MODE_UNDEFINED:
@@ -1304,6 +1308,7 @@ static int qxl_init_primary(PCIDevice *dev)
     vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
                                    qxl_hw_screen_dump, qxl_hw_text_update, qxl);
     qxl->ssd.ds = vga->ds;
+    qemu_mutex_init(&qxl->ssd.lock);
     qxl->ssd.bufsize = (16 * 1024 * 1024);
     qxl->ssd.buf = qemu_malloc(qxl->ssd.bufsize);
 
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 020b423..d56dcfc 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -62,14 +62,7 @@ void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r)
     dest->right = MAX(dest->right, r->right);
 }
 
-/*
- * Called from spice server thread context (via interface_get_command).
- *
- * We must aquire the global qemu mutex here to make sure the
- * DisplayState (+DisplaySurface) we are accessing doesn't change
- * underneath us.
- */
-SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
+static SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
 {
     SimpleSpiceUpdate *update;
     QXLDrawable *drawable;
@@ -78,9 +71,7 @@ SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
     uint8_t *src, *dst;
     int by, bw, bh;
 
-    qemu_mutex_lock_iothread();
     if (qemu_spice_rect_is_empty(&ssd->dirty)) {
-        qemu_mutex_unlock_iothread();
         return NULL;
     };
 
@@ -141,7 +132,6 @@ SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
     cmd->data = (intptr_t)drawable;
 
     memset(&ssd->dirty, 0, sizeof(ssd->dirty));
-    qemu_mutex_unlock_iothread();
     return update;
 }
 
@@ -241,6 +231,12 @@ void qemu_spice_display_resize(SimpleSpiceDisplay *ssd)
     qemu_pf_conv_put(ssd->conv);
     ssd->conv = NULL;
 
+    qemu_mutex_lock(&ssd->lock);
+    if (ssd->update != NULL) {
+        qemu_spice_destroy_update(ssd, ssd->update);
+        ssd->update = NULL;
+    }
+    qemu_mutex_unlock(&ssd->lock);
     qemu_spice_destroy_host_primary(ssd);
     qemu_spice_create_host_primary(ssd);
 
@@ -252,6 +248,14 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
 {
     dprint(3, "%s:\n", __FUNCTION__);
     vga_hw_update();
+
+    qemu_mutex_lock(&ssd->lock);
+    if (ssd->update == NULL) {
+        ssd->update = qemu_spice_create_update(ssd);
+        ssd->notify++;
+    }
+    qemu_mutex_unlock(&ssd->lock);
+
     if (ssd->notify) {
         ssd->notify = 0;
         ssd->worker->wakeup(ssd->worker);
@@ -298,14 +302,20 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
 {
     SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
     SimpleSpiceUpdate *update;
+    int ret = false;
 
     dprint(3, "%s:\n", __FUNCTION__);
-    update = qemu_spice_create_update(ssd);
-    if (update == NULL) {
-        return false;
+
+    qemu_mutex_lock(&ssd->lock);
+    if (ssd->update != NULL) {
+        update = ssd->update;
+        ssd->update = NULL;
+        *ext = update->ext;
+        ret = true;
     }
-    *ext = update->ext;
-    return true;
+    qemu_mutex_unlock(&ssd->lock);
+
+    return ret;
 }
 
 static int interface_req_cmd_notification(QXLInstance *sin)
@@ -398,6 +408,7 @@ void qemu_spice_display_init(DisplayState *ds)
 {
     assert(sdpy.ds == NULL);
     sdpy.ds = ds;
+    qemu_mutex_init(&sdpy.lock);
     sdpy.bufsize = (16 * 1024 * 1024);
     sdpy.buf = qemu_malloc(sdpy.bufsize);
     register_displaychangelistener(ds, &display_listener);
diff --git a/ui/spice-display.h b/ui/spice-display.h
index aef0464..e0cc46e 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -19,6 +19,7 @@
 #include <spice/enums.h>
 #include <spice/qxl_dev.h>
 
+#include "qemu-thread.h"
 #include "pflib.h"
 
 #define NUM_MEMSLOTS 8
@@ -31,7 +32,10 @@
 
 #define NUM_SURFACES 1024
 
-typedef struct SimpleSpiceDisplay {
+typedef struct SimpleSpiceDisplay SimpleSpiceDisplay;
+typedef struct SimpleSpiceUpdate SimpleSpiceUpdate;
+
+struct SimpleSpiceDisplay {
     DisplayState *ds;
     void *buf;
     int bufsize;
@@ -43,19 +47,26 @@ typedef struct SimpleSpiceDisplay {
     QXLRect dirty;
     int notify;
     int running;
-} SimpleSpiceDisplay;
 
-typedef struct SimpleSpiceUpdate {
+    /*
+     * All struct members below this comment can be accessed from
+     * both spice server and qemu (iothread) context and any access
+     * to them must be protected by the lock.
+     */
+    QemuMutex lock;
+    SimpleSpiceUpdate *update;
+};
+
+struct SimpleSpiceUpdate {
     QXLDrawable drawable;
     QXLImage image;
     QXLCommandExt ext;
     uint8_t *bitmap;
-} SimpleSpiceUpdate;
+};
 
 int qemu_spice_rect_is_empty(const QXLRect* r);
 void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
 
-SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *sdpy);
 void qemu_spice_destroy_update(SimpleSpiceDisplay *sdpy, SimpleSpiceUpdate *update);
 void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd);
 void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd);
-- 
1.7.1

[Qemu-devel] [PATCH 4/5] spice: don't call displaystate callbacks from spice server context.

[Gerd Hoffmann]

This patch moves the displaystate callback calls for setting the cursor
and the mouse pointer from spice server to qemu (iothread) context.
This allows us to simplify locking.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
(cherry picked from commit 075360945860ad9bdd491921954b383bf762b0e5)
---
 hw/qxl-render.c    |   25 ++++++++++++-------------
 hw/qxl.c           |    2 ++
 ui/spice-display.c |   12 ++++++++++++
 ui/spice-display.h |    3 +++
 4 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 58965e0..1316066 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -185,7 +185,6 @@ void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
     QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
     QXLCursor *cursor;
     QEMUCursor *c;
-    int x = -1, y = -1;
 
     if (!qxl->ssd.ds->mouse_set || !qxl->ssd.ds->cursor_define) {
         return;
@@ -198,8 +197,6 @@ void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
     }
     switch (cmd->type) {
     case QXL_CURSOR_SET:
-        x = cmd->u.set.position.x;
-        y = cmd->u.set.position.y;
         cursor = qxl_phys2virt(qxl, cmd->u.set.shape, ext->group_id);
         if (cursor->chunk.data_size != cursor->data_size) {
             fprintf(stderr, "%s: multiple chunks\n", __FUNCTION__);
@@ -209,18 +206,20 @@ void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
         if (c == NULL) {
             c = cursor_builtin_left_ptr();
         }
-        qemu_mutex_lock_iothread();
-        qxl->ssd.ds->cursor_define(c);
-        qxl->ssd.ds->mouse_set(x, y, 1);
-        qemu_mutex_unlock_iothread();
-        cursor_put(c);
+        qemu_mutex_lock(&qxl->ssd.lock);
+        if (qxl->ssd.cursor) {
+            cursor_put(qxl->ssd.cursor);
+        }
+        qxl->ssd.cursor = c;
+        qxl->ssd.mouse_x = cmd->u.set.position.x;
+        qxl->ssd.mouse_y = cmd->u.set.position.y;
+        qemu_mutex_unlock(&qxl->ssd.lock);
         break;
     case QXL_CURSOR_MOVE:
-        x = cmd->u.position.x;
-        y = cmd->u.position.y;
-        qemu_mutex_lock_iothread();
-        qxl->ssd.ds->mouse_set(x, y, 1);
-        qemu_mutex_unlock_iothread();
+        qemu_mutex_lock(&qxl->ssd.lock);
+        qxl->ssd.mouse_x = cmd->u.position.x;
+        qxl->ssd.mouse_y = cmd->u.position.y;
+        qemu_mutex_unlock(&qxl->ssd.lock);
         break;
     }
 }
diff --git a/hw/qxl.c b/hw/qxl.c
index bd250db..4dfddf0 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1309,6 +1309,8 @@ static int qxl_init_primary(PCIDevice *dev)
                                    qxl_hw_screen_dump, qxl_hw_text_update, qxl);
     qxl->ssd.ds = vga->ds;
     qemu_mutex_init(&qxl->ssd.lock);
+    qxl->ssd.mouse_x = -1;
+    qxl->ssd.mouse_y = -1;
     qxl->ssd.bufsize = (16 * 1024 * 1024);
     qxl->ssd.buf = qemu_malloc(qxl->ssd.bufsize);
 
diff --git a/ui/spice-display.c b/ui/spice-display.c
index d56dcfc..8579bfd 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -254,6 +254,16 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
         ssd->update = qemu_spice_create_update(ssd);
         ssd->notify++;
     }
+    if (ssd->cursor) {
+        ssd->ds->cursor_define(ssd->cursor);
+        cursor_put(ssd->cursor);
+        ssd->cursor = NULL;
+    }
+    if (ssd->mouse_x != -1 && ssd->mouse_y != -1) {
+        ssd->ds->mouse_set(ssd->mouse_x, ssd->mouse_y, 1);
+        ssd->mouse_x = -1;
+        ssd->mouse_y = -1;
+    }
     qemu_mutex_unlock(&ssd->lock);
 
     if (ssd->notify) {
@@ -409,6 +419,8 @@ void qemu_spice_display_init(DisplayState *ds)
     assert(sdpy.ds == NULL);
     sdpy.ds = ds;
     qemu_mutex_init(&sdpy.lock);
+    sdpy.mouse_x = -1;
+    sdpy.mouse_y = -1;
     sdpy.bufsize = (16 * 1024 * 1024);
     sdpy.buf = qemu_malloc(sdpy.bufsize);
     register_displaychangelistener(ds, &display_listener);
diff --git a/ui/spice-display.h b/ui/spice-display.h
index e0cc46e..2f95f68 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -20,6 +20,7 @@
 #include <spice/qxl_dev.h>
 
 #include "qemu-thread.h"
+#include "console.h"
 #include "pflib.h"
 
 #define NUM_MEMSLOTS 8
@@ -55,6 +56,8 @@ struct SimpleSpiceDisplay {
      */
     QemuMutex lock;
     SimpleSpiceUpdate *update;
+    QEMUCursor *cursor;
+    int mouse_x, mouse_y;
 };
 
 struct SimpleSpiceUpdate {
-- 
1.7.1

[Qemu-devel] [PATCH 5/5] spice: drop obsolete iothread locking

[Gerd Hoffmann]

We don't use qemu internals from spice server context any more.
Thus we don't also need to grab the iothread mutex from spice
server context.  And we don't have to temporarely release the
lock to avoid deadlocks.  Drop all the calls.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
(cherry picked from commit 196a778428989217b82de042725dc8eb29c8f8d8)
---
 hw/qxl.c           |    8 --------
 ui/spice-display.c |    6 ------
 2 files changed, 0 insertions(+), 14 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 4dfddf0..2bb36c6 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -666,10 +666,8 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm)
     dprint(d, 1, "%s: start%s\n", __FUNCTION__,
            loadvm ? " (loadvm)" : "");
 
-    qemu_mutex_unlock_iothread();
     d->ssd.worker->reset_cursor(d->ssd.worker);
     d->ssd.worker->reset_image_cache(d->ssd.worker);
-    qemu_mutex_lock_iothread();
     qxl_reset_surfaces(d);
     qxl_reset_memslots(d);
 
@@ -799,9 +797,7 @@ static void qxl_reset_surfaces(PCIQXLDevice *d)
 {
     dprint(d, 1, "%s:\n", __FUNCTION__);
     d->mode = QXL_MODE_UNDEFINED;
-    qemu_mutex_unlock_iothread();
     d->ssd.worker->destroy_surfaces(d->ssd.worker);
-    qemu_mutex_lock_iothread();
     memset(&d->guest_surfaces.cmds, 0, sizeof(d->guest_surfaces.cmds));
 }
 
@@ -870,9 +866,7 @@ static void qxl_destroy_primary(PCIQXLDevice *d)
     dprint(d, 1, "%s\n", __FUNCTION__);
 
     d->mode = QXL_MODE_UNDEFINED;
-    qemu_mutex_unlock_iothread();
     d->ssd.worker->destroy_primary_surface(d->ssd.worker, 0);
-    qemu_mutex_lock_iothread();
 }
 
 static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm)
@@ -942,10 +936,8 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
     case QXL_IO_UPDATE_AREA:
     {
         QXLRect update = d->ram->update_area;
-        qemu_mutex_unlock_iothread();
         d->ssd.worker->update_area(d->ssd.worker, d->ram->update_surface,
                                    &update, NULL, 0, 0);
-        qemu_mutex_lock_iothread();
         break;
     }
     case QXL_IO_NOTIFY_CMD:
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 8579bfd..15f0704 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -176,18 +176,14 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
     surface.mem        = (intptr_t)ssd->buf;
     surface.group_id   = MEMSLOT_GROUP_HOST;
 
-    qemu_mutex_unlock_iothread();
     ssd->worker->create_primary_surface(ssd->worker, 0, &surface);
-    qemu_mutex_lock_iothread();
 }
 
 void qemu_spice_destroy_host_primary(SimpleSpiceDisplay *ssd)
 {
     dprint(1, "%s:\n", __FUNCTION__);
 
-    qemu_mutex_unlock_iothread();
     ssd->worker->destroy_primary_surface(ssd->worker, 0);
-    qemu_mutex_lock_iothread();
 }
 
 void qemu_spice_vm_change_state_handler(void *opaque, int running, int reason)
@@ -197,9 +193,7 @@ void qemu_spice_vm_change_state_handler(void *opaque, int running, int reason)
     if (running) {
         ssd->worker->start(ssd->worker);
     } else {
-        qemu_mutex_unlock_iothread();
         ssd->worker->stop(ssd->worker);
-        qemu_mutex_lock_iothread();
     }
     ssd->running = running;
 }
-- 
1.7.1