[Qemu-devel] [PATCH 0/4] spice: fix locking

[Qemu-devel] [PATCH 0/4] spice: fix locking

[Gerd Hoffmann]

  Hi,

This patch series moves a bunch of work spice has to do from spice
server thread context to iothread context, which in turn allows to
drop the current locking mess as we don't touch qemu internals from
spice server thread any more.

A long-standing warning fix from Jes is included too.

cheers,
  Gerd

The following changes since commit 430a3c18064fd3c007048d757e8bd0fff45fcc99:

  configure: reenable opengl by default (2011-04-26 23:26:49 +0200)

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

Gerd Hoffmann (3):
      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

 hw/qxl-render.c    |   25 ++++++++++----------
 hw/qxl.c           |   27 ++++++++++-----------
 ui/qemu-spice.h    |   12 ++++++++-
 ui/spice-display.c |   63 +++++++++++++++++++++++++++++++++-------------------
 ui/spice-display.h |   24 +++++++++++++++----
 5 files changed, 94 insertions(+), 57 deletions(-)

[Qemu-devel] [PATCH 1/4] 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>
---
 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/4] 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>
---
 hw/qxl.c           |   17 +++++++++++------
 ui/spice-display.c |   45 ++++++++++++++++++++++++++++-----------------
 ui/spice-display.h |   21 ++++++++++++++++-----
 3 files changed, 55 insertions(+), 28 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..39c0ba1 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -27,7 +27,7 @@
 
 #include "spice-display.h"
 
-static int debug = 0;
+static int debug = 1;
 
 static void GCC_FMT_ATTR(2, 3) dprint(int level, const char *fmt, ...)
 {
@@ -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 3/4] 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>
---
 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 39c0ba1..1e1a35d 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 4/4] 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>
---
 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 1e1a35d..34201cd 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

Re: [Qemu-devel] [PATCH 2/4] spice: don't create updates in spice server context.

[Alon Levy]

On Fri, Apr 29, 2011 at 11:38:30AM +0200, Gerd Hoffmann wrote:
> 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>
> ---
>  hw/qxl.c           |   17 +++++++++++------
>  ui/spice-display.c |   45 ++++++++++++++++++++++++++++-----------------
>  ui/spice-display.h |   21 ++++++++++++++++-----
>  3 files changed, 55 insertions(+), 28 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..39c0ba1 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -27,7 +27,7 @@
>  
>  #include "spice-display.h"
>  
> -static int debug = 0;
> +static int debug = 1;
Accident?

>  
>  static void GCC_FMT_ATTR(2, 3) dprint(int level, const char *fmt, ...)
>  {
> @@ -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
> 
> 

Re: [Qemu-devel] [PATCH 0/4] spice: fix locking

[Alon Levy]

On Fri, Apr 29, 2011 at 11:38:28AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> This patch series moves a bunch of work spice has to do from spice
> server thread context to iothread context, which in turn allows to
> drop the current locking mess as we don't touch qemu internals from
> spice server thread any more.
> 
> A long-standing warning fix from Jes is included too.
> 

Tested with winxp, works well with changes between vga and qxl (tested
by setting the debug to 2 in spice-display.c). Just a single comment about
the debug=1 change in the second patch.

Alon

> cheers,
>   Gerd
> 
> The following changes since commit 430a3c18064fd3c007048d757e8bd0fff45fcc99:
> 
>   configure: reenable opengl by default (2011-04-26 23:26:49 +0200)
> 
> are available in the git repository at:
>   git://anongit.freedesktop.org/spice/qemu spice.v34
> 
> Gerd Hoffmann (3):
>       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
> 
>  hw/qxl-render.c    |   25 ++++++++++----------
>  hw/qxl.c           |   27 ++++++++++-----------
>  ui/qemu-spice.h    |   12 ++++++++-
>  ui/spice-display.c |   63 +++++++++++++++++++++++++++++++++-------------------
>  ui/spice-display.h |   24 +++++++++++++++----
>  5 files changed, 94 insertions(+), 57 deletions(-)
> 

Re: [Qemu-devel] [PATCH 2/4] spice: don't create updates in spice server context.

[Gerd Hoffmann]

   Hi,

>> -static int debug = 0;
>> +static int debug = 1;
> Accident?

Oops.  That one wasn't intentional, will fix for the pull request.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 0/4] spice: fix locking

[Alon Levy]

On Fri, Apr 29, 2011 at 02:20:09PM +0300, Alon Levy wrote:
> On Fri, Apr 29, 2011 at 11:38:28AM +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > This patch series moves a bunch of work spice has to do from spice
> > server thread context to iothread context, which in turn allows to
> > drop the current locking mess as we don't touch qemu internals from
> > spice server thread any more.
> > 
> > A long-standing warning fix from Jes is included too.
> > 
> 
> Tested with winxp, works well with changes between vga and qxl (tested
> by setting the debug to 2 in spice-display.c). Just a single comment about
> the debug=1 change in the second patch.
> 

Forgot:

ACK series

> Alon
> 
> > cheers,
> >   Gerd
> > 
> > The following changes since commit 430a3c18064fd3c007048d757e8bd0fff45fcc99:
> > 
> >   configure: reenable opengl by default (2011-04-26 23:26:49 +0200)
> > 
> > are available in the git repository at:
> >   git://anongit.freedesktop.org/spice/qemu spice.v34
> > 
> > Gerd Hoffmann (3):
> >       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
> > 
> >  hw/qxl-render.c    |   25 ++++++++++----------
> >  hw/qxl.c           |   27 ++++++++++-----------
> >  ui/qemu-spice.h    |   12 ++++++++-
> >  ui/spice-display.c |   63 +++++++++++++++++++++++++++++++++-------------------
> >  ui/spice-display.h |   24 +++++++++++++++----
> >  5 files changed, 94 insertions(+), 57 deletions(-)
> > 
> 

[Qemu-devel] [PATCH 2/4] 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>
---
 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

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

[Bing Bu Cao]

[-- Attachment #1: Type: text/plain, Size: 3835 bytes --]

Hi,Gerd

I found the lock has been revert in qemu-kvm repository and
qemu-kvm on RHEL6.3 with spice/qxl has one problem:


Doing some display configuration (such as changing resolution) in guest 
will involve in deadlock in guest,meanwhile
which affect the host hang for a long while.


I wonder whether this patch should be revert back in qemu-kvm again?
Actually, I have no idea why added in QEMU but not in qemu-kvm? :)


On 04/29/2011 05:38 PM, Gerd Hoffmann wrote:
> 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>
> ---
>   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 1e1a35d..34201cd 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;
>   }


[-- Attachment #2: Type: text/html, Size: 4410 bytes --]