[Qemu-devel] [PATCH 0/4] qxl: implement vga mode without locks

[Qemu-devel] [PATCH 0/4] qxl: implement vga mode without locks

[Alon Levy]

This patchset removes all uses of unlock/lock in qxl and spice code.

It does this by reimplementing the one path that required a lock from the
spice-server thread, namely interface_get_command when in vga mode or running
without a qxl device.

Incidentaly it fixes the assert(cpu_single_env) that happen in the unrelated
qemu-kvm repository, because we never drop the global lock in io vmexits.

Tested with winxp 32 bit and linux 64 bit vms, including changes between vga
mode and qxl mode using chvt.

Alon Levy (3):
  qxl/spice-display: move pipe to ssd
  qxl/spice: remove qemu_mutex_{un,}lock_iothread around dispatcher
  hw/qxl-render: drop cursor locks, add TODO's.

Uri Lublin (1):
  qxl: implement get_command in vga mode without locks

 hw/qxl-render.c    |   13 +++--
 hw/qxl.c           |  150 ++++++++++++++++++++++++++++++++++++++++------------
 hw/qxl.h           |    4 --
 ui/spice-display.c |   20 ++------
 ui/spice-display.h |   16 ++++++
 5 files changed, 144 insertions(+), 59 deletions(-)

-- 
1.7.4.1

[Qemu-devel] [PATCH 1/4] qxl/spice-display: move pipe to ssd

[Alon Levy]

this moves the int pipe[2] and pthread_t main data from the
PCIQXLDevice struct to the SimpleSpiceDisplay. This will let us
reuse it in the next patch for both -spice with no -qxl usage and
for vga mode from qxl.
---
 hw/qxl.c           |   32 ++++++++++++++++++++------------
 hw/qxl.h           |    4 ----
 ui/spice-display.c |    1 +
 ui/spice-display.h |   10 ++++++++++
 4 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index fe4212b..d1d3c9c 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1062,7 +1062,7 @@ static void pipe_read(void *opaque)
     int len;
 
     do {
-        len = read(d->pipe[0], &dummy, sizeof(dummy));
+        len = read(d->ssd.pipe[0], &dummy, sizeof(dummy));
     } while (len == sizeof(dummy));
     qxl_set_irq(d);
 }
@@ -1078,31 +1078,39 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
     if ((old_pending & le_events) == le_events) {
         return;
     }
-    if (pthread_self() == d->main) {
+    if (pthread_self() == d->ssd.main) {
+        /* running in io_thread thread */
         qxl_set_irq(d);
     } else {
-        if (write(d->pipe[1], d, 1) != 1) {
+        if (write(d->ssd.pipe[1], d, 1) != 1) {
             dprint(d, 1, "%s: write to pipe failed\n", __FUNCTION__);
         }
     }
 }
 
-static void init_pipe_signaling(PCIQXLDevice *d)
+void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd)
 {
-   if (pipe(d->pipe) < 0) {
-       dprint(d, 1, "%s: pipe creation failed\n", __FUNCTION__);
+   if (pipe(ssd->pipe) < 0) {
+       fprintf(stderr, "%s: pipe creation failed\n", __FUNCTION__);
        return;
    }
+
 #ifdef CONFIG_IOTHREAD
-   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK);
+   fcntl(ssd->pipe[0], F_SETFL, O_NONBLOCK);
 #else
-   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK /* | O_ASYNC */);
+   fcntl(ssd->pipe[0], F_SETFL, O_NONBLOCK /* | O_ASYNC */);
 #endif
-   fcntl(d->pipe[1], F_SETFL, O_NONBLOCK);
-   fcntl(d->pipe[0], F_SETOWN, getpid());
+   fcntl(ssd->pipe[1], F_SETFL, O_NONBLOCK);
 
-   d->main = pthread_self();
-   qemu_set_fd_handler(d->pipe[0], pipe_read, NULL, d);
+   fcntl(ssd->pipe[0], F_SETOWN, getpid());
+
+   qemu_set_fd_handler(ssd->pipe[0], pipe_read, NULL, ssd);
+   ssd->main = pthread_self();
+}
+
+static void init_pipe_signaling(PCIQXLDevice *d)
+{
+   qxl_create_vcpu_to_iothread_pipe(&d->ssd);
 }
 
 /* graphics console */
diff --git a/hw/qxl.h b/hw/qxl.h
index f6c450d..701245f 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -55,10 +55,6 @@ typedef struct PCIQXLDevice {
     } guest_surfaces;
     QXLPHYSICAL        guest_cursor;
 
-    /* thread signaling */
-    pthread_t          main;
-    int                pipe[2];
-
     /* ram pci bar */
     QXLRam             *ram;
     VGACommonState     vga;
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 020b423..e0ac616 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -406,6 +406,7 @@ void qemu_spice_display_init(DisplayState *ds)
     qemu_spice_add_interface(&sdpy.qxl.base);
     assert(sdpy.worker);
 
+    qxl_create_vcpu_to_iothread_pipe(&sdpy);
     qemu_add_vm_change_state_handler(qemu_spice_vm_change_state_handler, &sdpy);
     qemu_spice_create_host_memslot(&sdpy);
     qemu_spice_create_host_primary(&sdpy);
diff --git a/ui/spice-display.h b/ui/spice-display.h
index aef0464..a708d59 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -43,6 +43,11 @@ typedef struct SimpleSpiceDisplay {
     QXLRect dirty;
     int notify;
     int running;
+
+    /* thread signaling - used both in qxl (in vga mode
+     * and in native mode) and without qxl */
+    pthread_t          main;
+    int                pipe[2];     /* to iothread */
 } SimpleSpiceDisplay;
 
 typedef struct SimpleSpiceUpdate {
@@ -66,3 +71,8 @@ void qemu_spice_display_update(SimpleSpiceDisplay *ssd,
                                int x, int y, int w, int h);
 void qemu_spice_display_resize(SimpleSpiceDisplay *ssd);
 void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd);
+/* shared with qxl.c in vga mode and ui/spice-display (no qxl mode) */
+int qxl_vga_mode_get_command(
+    SimpleSpiceDisplay *ssd, struct QXLCommandExt *ext);
+/* used by both qxl and spice-display */
+void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd);
-- 
1.7.4.1

[Qemu-devel] [PATCH 2/4] qxl: implement get_command in vga mode without locks

[Alon Levy]

From: Uri Lublin <uril@redhat.com>

This patch and the next drop the requirement to lose the global qemu
mutex during dispatcher calls. This patch enables it, the next drops
the unlock/lock pairs around dispatcher calls.

The current solution of dropping the locks is buggy:
 * it allows multiple dispatcher calls from two vcpu threads, the
 dispatcher doesn't handle that by design (single fd, not locked, can't
 handle writes from two threads)
 * it requires us to keep track of cpu_single_env, which is magic.

The solution implemented in this patch and the next (the next just
drops the locks, this patch allows that to work):
 * the only operation that needed locking was qemu_create_simple_update,
 * it required locking because it was called from the spice-server thread.
 * do it in the iothread by reusing the existing pipe used for set_irq.

The current flow implemented is now:
spice-server thread:
 qxl.c:interface_get_command (called either by polling or from wakeup)
  if update!=NULL:
   waiting_for_update=0
   update=NULL
   return update
  else:
   if not waiting_for_update:
    waiting_for_update=1
    write to pipe, which is read by iothread (main thread)

iothread:
 wakeup from select,
 qxl.c:pipe_read
  update=qemu_create_simple_update()
  wakeup spice-server thread by calling d.worker->wakeup(d.worker)
---
 hw/qxl.c           |  118 +++++++++++++++++++++++++++++++++++++++++++--------
 ui/spice-display.c |   10 +----
 ui/spice-display.h |    8 +++-
 3 files changed, 108 insertions(+), 28 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index d1d3c9c..2419236 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -28,6 +28,8 @@
 
 #include "qxl.h"
 
+#define EMPTY_UPDATE ((void *)-1)
+
 #undef SPICE_RING_PROD_ITEM
 #define SPICE_RING_PROD_ITEM(r, ret) {                                  \
         typeof(r) start = r;                                            \
@@ -117,6 +119,15 @@ static QXLMode qxl_modes[] = {
 #endif
 };
 
+
+/*
+ * commands/requests from server thread to iothread.
+ */
+enum {
+    QXL_SERVER_SET_IRQ = 1,
+    QXL_SERVER_CREATE_UPDATE,
+};
+
 static PCIQXLDevice *qxl0;
 
 static void qxl_send_events(PCIQXLDevice *d, uint32_t events);
@@ -336,11 +347,36 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
     info->n_surfaces = NUM_SURFACES;
 }
 
+int qxl_vga_mode_get_command(
+    SimpleSpiceDisplay *ssd, struct QXLCommandExt *ext)
+{
+    SimpleSpiceUpdate *update;
+    unsigned char req;
+    int r;
+
+    update = ssd->update;
+    if (update != NULL) {
+        ssd->waiting_for_update = 0;
+        ssd->update = NULL;
+        if (update != EMPTY_UPDATE) {
+            *ext = update->ext;
+            return true;
+        }
+    } else {
+        if (!ssd->waiting_for_update) {
+            ssd->waiting_for_update = 1;
+            req = QXL_SERVER_CREATE_UPDATE;
+            r = write(ssd->pipe[1], &req , 1);
+            assert(r == 1);
+        }
+    }
+    return false;
+}
+
 /* called from spice server thread context only */
 static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
 {
     PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
-    SimpleSpiceUpdate *update;
     QXLCommandRing *ring;
     QXLCommand *cmd;
     int notify;
@@ -348,16 +384,25 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
     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;
+        if (qxl_vga_mode_get_command(&qxl->ssd, ext)) {
+            qxl_log_command(qxl, "vga", ext);
+            return true;
         }
-        *ext = update->ext;
-        qxl_log_command(qxl, "vga", ext);
-        return true;
+        return false;
     case QXL_MODE_COMPAT:
     case QXL_MODE_NATIVE:
     case QXL_MODE_UNDEFINED:
+        /* flush any existing updates that we didn't send to the guest.
+         * since update != NULL it means the server didn't get it, and
+         * because we changed mode to != QXL_MODE_VGA, it won't. */
+        if (qxl->ssd.update != NULL) {
+            if (qxl->ssd.update != EMPTY_UPDATE) {
+                qemu_spice_destroy_update(&qxl->ssd, qxl->ssd.update);
+            }
+            qxl->ssd.update = NULL;
+            qxl->ssd.waiting_for_update = 0;
+        }
+        /* */
         dprint(qxl, 2, "%s: %s\n", __FUNCTION__,
                qxl->cmdflags ? "compat" : "native");
         ring = &qxl->ram->cmd_ring;
@@ -1057,17 +1102,50 @@ static void qxl_map(PCIDevice *pci, int region_num,
 
 static void pipe_read(void *opaque)
 {
-    PCIQXLDevice *d = opaque;
-    char dummy;
-    int len;
+    SimpleSpiceDisplay *ssd = opaque;
+    unsigned char cmd;
+    int len, set_irq = 0;
+    int create_update = 0;
 
     do {
-        len = read(d->ssd.pipe[0], &dummy, sizeof(dummy));
-    } while (len == sizeof(dummy));
-    qxl_set_irq(d);
+        cmd = 0;
+        len = read(ssd->pipe[0], &cmd, sizeof(cmd));
+        if (len < 0) {
+            if (errno == EINTR) {
+                continue;
+            }
+            if (errno == EAGAIN) {
+                break;
+            }
+            perror("qxl: pipe_read: read failed");
+            break;
+        }
+        switch (cmd) {
+        case QXL_SERVER_SET_IRQ:
+            set_irq = 1;
+            break;
+        case QXL_SERVER_CREATE_UPDATE:
+            create_update = 1;
+            break;
+        default:
+            fprintf(stderr, "%s: unknown cmd %u\n", __FUNCTION__, cmd);
+            abort();
+        }
+    } while (1);
+    /* no need to do either operation more than once */
+    if (create_update) {
+        assert(ssd->update == NULL);
+        ssd->update = qemu_spice_create_update(ssd);
+        if (ssd->update == NULL) {
+            ssd->update = EMPTY_UPDATE;
+        }
+        ssd->worker->wakeup(ssd->worker);
+    }
+    if (set_irq) {
+        qxl_set_irq(container_of(ssd, PCIQXLDevice, ssd));
+    }
 }
 
-/* called from spice server thread context only */
 static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
 {
     uint32_t old_pending;
@@ -1082,13 +1160,15 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
         /* running in io_thread thread */
         qxl_set_irq(d);
     } else {
-        if (write(d->ssd.pipe[1], d, 1) != 1) {
-            dprint(d, 1, "%s: write to pipe failed\n", __FUNCTION__);
-        }
+        /* called from spice-server thread */
+        int ret;
+        unsigned char ack = QXL_SERVER_SET_IRQ;
+        ret = write(d->ssd.pipe[1], &ack, 1);
+        assert(ret == 1);
     }
 }
 
-void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd)
+void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd)
 {
    if (pipe(ssd->pipe) < 0) {
        fprintf(stderr, "%s: pipe creation failed\n", __FUNCTION__);
@@ -1110,7 +1190,7 @@ void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd)
 
 static void init_pipe_signaling(PCIQXLDevice *d)
 {
-   qxl_create_vcpu_to_iothread_pipe(&d->ssd);
+   qxl_create_server_to_iothread_pipe(&d->ssd);
 }
 
 /* graphics console */
diff --git a/ui/spice-display.c b/ui/spice-display.c
index e0ac616..a9ecee0 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -297,15 +297,9 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
 static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
 {
     SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
-    SimpleSpiceUpdate *update;
 
     dprint(3, "%s:\n", __FUNCTION__);
-    update = qemu_spice_create_update(ssd);
-    if (update == NULL) {
-        return false;
-    }
-    *ext = update->ext;
-    return true;
+    return qxl_vga_mode_get_command(ssd, ext);
 }
 
 static int interface_req_cmd_notification(QXLInstance *sin)
@@ -406,7 +400,7 @@ void qemu_spice_display_init(DisplayState *ds)
     qemu_spice_add_interface(&sdpy.qxl.base);
     assert(sdpy.worker);
 
-    qxl_create_vcpu_to_iothread_pipe(&sdpy);
+    qxl_create_server_to_iothread_pipe(&sdpy);
     qemu_add_vm_change_state_handler(qemu_spice_vm_change_state_handler, &sdpy);
     qemu_spice_create_host_memslot(&sdpy);
     qemu_spice_create_host_primary(&sdpy);
diff --git a/ui/spice-display.h b/ui/spice-display.h
index a708d59..210b0b5 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -31,6 +31,8 @@
 
 #define NUM_SURFACES 1024
 
+struct SimpleSpiceUpdate;
+
 typedef struct SimpleSpiceDisplay {
     DisplayState *ds;
     void *buf;
@@ -48,6 +50,10 @@ typedef struct SimpleSpiceDisplay {
      * and in native mode) and without qxl */
     pthread_t          main;
     int                pipe[2];     /* to iothread */
+
+    /* ssd updates (one request/command at a time) */
+    struct SimpleSpiceUpdate *update;
+    int waiting_for_update;
 } SimpleSpiceDisplay;
 
 typedef struct SimpleSpiceUpdate {
@@ -75,4 +81,4 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd);
 int qxl_vga_mode_get_command(
     SimpleSpiceDisplay *ssd, struct QXLCommandExt *ext);
 /* used by both qxl and spice-display */
-void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd);
+void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd);
-- 
1.7.4.1

[Qemu-devel] [PATCH 3/4] qxl/spice: remove qemu_mutex_{un, }lock_iothread around dispatcher

[Alon Levy]

with the previous patch making sure get_command no longer needs to lock,
there is no reason to drop the qemu iothread mutex in qxl.c and in
ui/spice-display.c

The only location where the lock remains are the cursor related callbacks,
that path is currently broken. It is only triggered if running spice and sdl,
which is broken already before that.
---
 hw/qxl.c           |    8 --------
 ui/spice-display.c |   11 ++---------
 2 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 2419236..72f204b 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -707,10 +707,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);
 
@@ -840,9 +838,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));
 }
 
@@ -911,9 +907,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)
@@ -983,10 +977,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 a9ecee0..f3dfba8 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -78,9 +78,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 +139,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;
 }
 
@@ -169,6 +166,7 @@ void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd)
     ssd->worker->add_memslot(ssd->worker, &memslot);
 }
 
+/* called from iothread (main) or a vcpu-thread */
 void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
 {
     QXLDevSurfaceCreate surface;
@@ -186,18 +184,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)
@@ -207,9 +201,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;
 }
@@ -233,6 +225,7 @@ void qemu_spice_display_update(SimpleSpiceDisplay *ssd,
     qemu_spice_rect_union(&ssd->dirty, &update_area);
 }
 
+/* called only from iothread (main) */
 void qemu_spice_display_resize(SimpleSpiceDisplay *ssd)
 {
     dprint(1, "%s:\n", __FUNCTION__);
-- 
1.7.4.1

[Qemu-devel] [PATCH 4/4] hw/qxl-render: drop cursor locks, add TODO's.

[Alon Levy]

Dropping the locks prevents a deadlock when running with -sdl or -vnc
in addition to -spice.

When server calls get_cursor_command, and we have an active ds
cursor related callback in non vga mode, we need to lock to prevent
the iothread (via sdl/vnc gui_update timer) from touching the ds as well.

Currently (-sdl/-vnc) + -spice seems to work, due to dropping the locking in
qxl-render.c:qxl_render_cursor, but this is just waiting to break because of
touching the cursor from two threads without any locking.
---
 hw/qxl-render.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 58965e0..1065388 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -209,18 +209,23 @@ void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
         if (c == NULL) {
             c = cursor_builtin_left_ptr();
         }
-        qemu_mutex_lock_iothread();
+        /* TODO: move this operation to iothread via pipe
+         * we can't use the global lock here without dropping it
+         * in gui_update (vl.c), or we get a dead lock (gui_update
+         * calls dispatcher, waiting on pipe read, and spice server calls
+         * this function, waiting on the lock that iothread is holding).
+         * But when used with sdl this calls sdl.c:sdl_mouse_define, which
+         * afaict must be locked or called from iothread. Moving to iothread
+         * seems easiest from correctness pov. */
         qxl->ssd.ds->cursor_define(c);
         qxl->ssd.ds->mouse_set(x, y, 1);
-        qemu_mutex_unlock_iothread();
         cursor_put(c);
         break;
     case QXL_CURSOR_MOVE:
         x = cmd->u.position.x;
         y = cmd->u.position.y;
-        qemu_mutex_lock_iothread();
+        /* TODO: move this operation to iothread via pipe. See comment above */
         qxl->ssd.ds->mouse_set(x, y, 1);
-        qemu_mutex_unlock_iothread();
         break;
     }
 }
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 1/4] qxl/spice-display: move pipe to ssd

[Hans de Goede]

Hi,

Looks good, except for one small issue, see my comment at
the end of the patch.

On 03/15/2011 09:17 PM, Alon Levy wrote:
> this moves the int pipe[2] and pthread_t main data from the
> PCIQXLDevice struct to the SimpleSpiceDisplay. This will let us
> reuse it in the next patch for both -spice with no -qxl usage and
> for vga mode from qxl.
> ---
>   hw/qxl.c           |   32 ++++++++++++++++++++------------
>   hw/qxl.h           |    4 ----
>   ui/spice-display.c |    1 +
>   ui/spice-display.h |   10 ++++++++++
>   4 files changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/hw/qxl.c b/hw/qxl.c
> index fe4212b..d1d3c9c 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -1062,7 +1062,7 @@ static void pipe_read(void *opaque)
>       int len;
>
>       do {
> -        len = read(d->pipe[0],&dummy, sizeof(dummy));
> +        len = read(d->ssd.pipe[0],&dummy, sizeof(dummy));
>       } while (len == sizeof(dummy));
>       qxl_set_irq(d);
>   }
> @@ -1078,31 +1078,39 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
>       if ((old_pending&  le_events) == le_events) {
>           return;
>       }
> -    if (pthread_self() == d->main) {
> +    if (pthread_self() == d->ssd.main) {
> +        /* running in io_thread thread */
>           qxl_set_irq(d);
>       } else {
> -        if (write(d->pipe[1], d, 1) != 1) {
> +        if (write(d->ssd.pipe[1], d, 1) != 1) {
>               dprint(d, 1, "%s: write to pipe failed\n", __FUNCTION__);
>           }
>       }
>   }
>
> -static void init_pipe_signaling(PCIQXLDevice *d)
> +void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd)
>   {
> -   if (pipe(d->pipe)<  0) {
> -       dprint(d, 1, "%s: pipe creation failed\n", __FUNCTION__);
> +   if (pipe(ssd->pipe)<  0) {
> +       fprintf(stderr, "%s: pipe creation failed\n", __FUNCTION__);
>          return;
>      }
> +
>   #ifdef CONFIG_IOTHREAD
> -   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK);
> +   fcntl(ssd->pipe[0], F_SETFL, O_NONBLOCK);
>   #else
> -   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK /* | O_ASYNC */);
> +   fcntl(ssd->pipe[0], F_SETFL, O_NONBLOCK /* | O_ASYNC */);
>   #endif
> -   fcntl(d->pipe[1], F_SETFL, O_NONBLOCK);
> -   fcntl(d->pipe[0], F_SETOWN, getpid());
> +   fcntl(ssd->pipe[1], F_SETFL, O_NONBLOCK);
>
> -   d->main = pthread_self();
> -   qemu_set_fd_handler(d->pipe[0], pipe_read, NULL, d);
> +   fcntl(ssd->pipe[0], F_SETOWN, getpid());
> +
> +   qemu_set_fd_handler(ssd->pipe[0], pipe_read, NULL, ssd);
> +   ssd->main = pthread_self();
> +}
> +
> +static void init_pipe_signaling(PCIQXLDevice *d)
> +{
> +   qxl_create_vcpu_to_iothread_pipe(&d->ssd);
>   }
>
>   /* graphics console */
> diff --git a/hw/qxl.h b/hw/qxl.h
> index f6c450d..701245f 100644
> --- a/hw/qxl.h
> +++ b/hw/qxl.h
> @@ -55,10 +55,6 @@ typedef struct PCIQXLDevice {
>       } guest_surfaces;
>       QXLPHYSICAL        guest_cursor;
>
> -    /* thread signaling */
> -    pthread_t          main;
> -    int                pipe[2];
> -
>       /* ram pci bar */
>       QXLRam             *ram;
>       VGACommonState     vga;
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 020b423..e0ac616 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -406,6 +406,7 @@ void qemu_spice_display_init(DisplayState *ds)
>       qemu_spice_add_interface(&sdpy.qxl.base);
>       assert(sdpy.worker);
>
> +    qxl_create_vcpu_to_iothread_pipe(&sdpy);
>       qemu_add_vm_change_state_handler(qemu_spice_vm_change_state_handler,&sdpy);
>       qemu_spice_create_host_memslot(&sdpy);
>       qemu_spice_create_host_primary(&sdpy);
> diff --git a/ui/spice-display.h b/ui/spice-display.h
> index aef0464..a708d59 100644
> --- a/ui/spice-display.h
> +++ b/ui/spice-display.h
> @@ -43,6 +43,11 @@ typedef struct SimpleSpiceDisplay {
>       QXLRect dirty;
>       int notify;
>       int running;
> +
> +    /* thread signaling - used both in qxl (in vga mode
> +     * and in native mode) and without qxl */
> +    pthread_t          main;
> +    int                pipe[2];     /* to iothread */
>   } SimpleSpiceDisplay;
>
>   typedef struct SimpleSpiceUpdate {
> @@ -66,3 +71,8 @@ void qemu_spice_display_update(SimpleSpiceDisplay *ssd,
>                                  int x, int y, int w, int h);
>   void qemu_spice_display_resize(SimpleSpiceDisplay *ssd);
>   void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd);
> +/* shared with qxl.c in vga mode and ui/spice-display (no qxl mode) */
> +int qxl_vga_mode_get_command(
> +    SimpleSpiceDisplay *ssd, struct QXLCommandExt *ext);

This function does not get defined until the next patch, so the adding
of the prototype should be done there too.

> +/* used by both qxl and spice-display */
> +void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd);

Regards,

Hans

Re: [Qemu-devel] [PATCH 2/4] qxl: implement get_command in vga mode without locks

[Hans de Goede]

Hi,

Looks good, except for one small issue again...

On 03/15/2011 09:17 PM, Alon Levy wrote:
> From: Uri Lublin<uril@redhat.com>
>
> This patch and the next drop the requirement to lose the global qemu
> mutex during dispatcher calls. This patch enables it, the next drops
> the unlock/lock pairs around dispatcher calls.
>
> The current solution of dropping the locks is buggy:
>   * it allows multiple dispatcher calls from two vcpu threads, the
>   dispatcher doesn't handle that by design (single fd, not locked, can't
>   handle writes from two threads)
>   * it requires us to keep track of cpu_single_env, which is magic.
>
> The solution implemented in this patch and the next (the next just
> drops the locks, this patch allows that to work):
>   * the only operation that needed locking was qemu_create_simple_update,
>   * it required locking because it was called from the spice-server thread.
>   * do it in the iothread by reusing the existing pipe used for set_irq.
>
> The current flow implemented is now:
> spice-server thread:
>   qxl.c:interface_get_command (called either by polling or from wakeup)
>    if update!=NULL:
>     waiting_for_update=0
>     update=NULL
>     return update
>    else:
>     if not waiting_for_update:
>      waiting_for_update=1
>      write to pipe, which is read by iothread (main thread)
>
> iothread:
>   wakeup from select,
>   qxl.c:pipe_read
>    update=qemu_create_simple_update()
>    wakeup spice-server thread by calling d.worker->wakeup(d.worker)
> ---
>   hw/qxl.c           |  118 +++++++++++++++++++++++++++++++++++++++++++--------
>   ui/spice-display.c |   10 +----
>   ui/spice-display.h |    8 +++-
>   3 files changed, 108 insertions(+), 28 deletions(-)
>
> diff --git a/hw/qxl.c b/hw/qxl.c
> index d1d3c9c..2419236 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -28,6 +28,8 @@
>
>   #include "qxl.h"
>
> +#define EMPTY_UPDATE ((void *)-1)
> +
>   #undef SPICE_RING_PROD_ITEM
>   #define SPICE_RING_PROD_ITEM(r, ret) {                                  \
>           typeof(r) start = r;                                            \
> @@ -117,6 +119,15 @@ static QXLMode qxl_modes[] = {
>   #endif
>   };
>
> +
> +/*
> + * commands/requests from server thread to iothread.
> + */
> +enum {
> +    QXL_SERVER_SET_IRQ = 1,
> +    QXL_SERVER_CREATE_UPDATE,
> +};
> +
>   static PCIQXLDevice *qxl0;
>
>   static void qxl_send_events(PCIQXLDevice *d, uint32_t events);
> @@ -336,11 +347,36 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
>       info->n_surfaces = NUM_SURFACES;
>   }
>
> +int qxl_vga_mode_get_command(
> +    SimpleSpiceDisplay *ssd, struct QXLCommandExt *ext)
> +{
> +    SimpleSpiceUpdate *update;
> +    unsigned char req;
> +    int r;
> +
> +    update = ssd->update;
> +    if (update != NULL) {
> +        ssd->waiting_for_update = 0;
> +        ssd->update = NULL;
> +        if (update != EMPTY_UPDATE) {
> +            *ext = update->ext;
> +            return true;
> +        }
> +    } else {
> +        if (!ssd->waiting_for_update) {
> +            ssd->waiting_for_update = 1;
> +            req = QXL_SERVER_CREATE_UPDATE;
> +            r = write(ssd->pipe[1],&req , 1);
> +            assert(r == 1);
> +        }
> +    }
> +    return false;
> +}
> +
>   /* called from spice server thread context only */
>   static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
>   {
>       PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> -    SimpleSpiceUpdate *update;
>       QXLCommandRing *ring;
>       QXLCommand *cmd;
>       int notify;
> @@ -348,16 +384,25 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
>       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;
> +        if (qxl_vga_mode_get_command(&qxl->ssd, ext)) {
> +            qxl_log_command(qxl, "vga", ext);
> +            return true;
>           }
> -        *ext = update->ext;
> -        qxl_log_command(qxl, "vga", ext);
> -        return true;
> +        return false;
>       case QXL_MODE_COMPAT:
>       case QXL_MODE_NATIVE:
>       case QXL_MODE_UNDEFINED:
> +        /* flush any existing updates that we didn't send to the guest.
> +         * since update != NULL it means the server didn't get it, and
> +         * because we changed mode to != QXL_MODE_VGA, it won't. */
> +        if (qxl->ssd.update != NULL) {
> +            if (qxl->ssd.update != EMPTY_UPDATE) {
> +                qemu_spice_destroy_update(&qxl->ssd, qxl->ssd.update);
> +            }
> +            qxl->ssd.update = NULL;
> +            qxl->ssd.waiting_for_update = 0;
> +        }
> +        /* */
>           dprint(qxl, 2, "%s: %s\n", __FUNCTION__,
>                  qxl->cmdflags ? "compat" : "native");
>           ring =&qxl->ram->cmd_ring;
> @@ -1057,17 +1102,50 @@ static void qxl_map(PCIDevice *pci, int region_num,
>
>   static void pipe_read(void *opaque)
>   {
> -    PCIQXLDevice *d = opaque;
> -    char dummy;
> -    int len;
> +    SimpleSpiceDisplay *ssd = opaque;
> +    unsigned char cmd;
> +    int len, set_irq = 0;
> +    int create_update = 0;
>
>       do {
> -        len = read(d->ssd.pipe[0],&dummy, sizeof(dummy));
> -    } while (len == sizeof(dummy));
> -    qxl_set_irq(d);
> +        cmd = 0;
> +        len = read(ssd->pipe[0],&cmd, sizeof(cmd));
> +        if (len<  0) {
> +            if (errno == EINTR) {
> +                continue;
> +            }
> +            if (errno == EAGAIN) {
> +                break;
> +            }
> +            perror("qxl: pipe_read: read failed");
> +            break;
> +        }
> +        switch (cmd) {
> +        case QXL_SERVER_SET_IRQ:
> +            set_irq = 1;
> +            break;
> +        case QXL_SERVER_CREATE_UPDATE:
> +            create_update = 1;
> +            break;
> +        default:
> +            fprintf(stderr, "%s: unknown cmd %u\n", __FUNCTION__, cmd);
> +            abort();
> +        }
> +    } while (1);
> +    /* no need to do either operation more than once */
> +    if (create_update) {
> +        assert(ssd->update == NULL);
> +        ssd->update = qemu_spice_create_update(ssd);
> +        if (ssd->update == NULL) {
> +            ssd->update = EMPTY_UPDATE;
> +        }
> +        ssd->worker->wakeup(ssd->worker);
> +    }
> +    if (set_irq) {
> +        qxl_set_irq(container_of(ssd, PCIQXLDevice, ssd));
> +    }
>   }
>
> -/* called from spice server thread context only */
>   static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
>   {
>       uint32_t old_pending;
> @@ -1082,13 +1160,15 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
>           /* running in io_thread thread */
>           qxl_set_irq(d);
>       } else {
> -        if (write(d->ssd.pipe[1], d, 1) != 1) {
> -            dprint(d, 1, "%s: write to pipe failed\n", __FUNCTION__);
> -        }
> +        /* called from spice-server thread */
> +        int ret;
> +        unsigned char ack = QXL_SERVER_SET_IRQ;
> +        ret = write(d->ssd.pipe[1],&ack, 1);
> +        assert(ret == 1);
>       }
>   }
>
> -void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd)
> +void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd)
>   {
>      if (pipe(ssd->pipe)<  0) {
>          fprintf(stderr, "%s: pipe creation failed\n", __FUNCTION__);

This function was introduced in your previous patch I agree that
qxl_create_server_to_iothread_pipe is the correct name, but lets name
it that in the first patch then, rather then introduce it under a
different name in patch1, only to rename it in patch2.

> @@ -1110,7 +1190,7 @@ void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd)
>
>   static void init_pipe_signaling(PCIQXLDevice *d)
>   {
> -   qxl_create_vcpu_to_iothread_pipe(&d->ssd);
> +   qxl_create_server_to_iothread_pipe(&d->ssd);
>   }
>
>   /* graphics console */
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index e0ac616..a9ecee0 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -297,15 +297,9 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
>   static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
>   {
>       SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
> -    SimpleSpiceUpdate *update;
>
>       dprint(3, "%s:\n", __FUNCTION__);
> -    update = qemu_spice_create_update(ssd);
> -    if (update == NULL) {
> -        return false;
> -    }
> -    *ext = update->ext;
> -    return true;
> +    return qxl_vga_mode_get_command(ssd, ext);
>   }
>
>   static int interface_req_cmd_notification(QXLInstance *sin)
> @@ -406,7 +400,7 @@ void qemu_spice_display_init(DisplayState *ds)
>       qemu_spice_add_interface(&sdpy.qxl.base);
>       assert(sdpy.worker);
>
> -    qxl_create_vcpu_to_iothread_pipe(&sdpy);
> +    qxl_create_server_to_iothread_pipe(&sdpy);
>       qemu_add_vm_change_state_handler(qemu_spice_vm_change_state_handler,&sdpy);
>       qemu_spice_create_host_memslot(&sdpy);
>       qemu_spice_create_host_primary(&sdpy);
> diff --git a/ui/spice-display.h b/ui/spice-display.h
> index a708d59..210b0b5 100644
> --- a/ui/spice-display.h
> +++ b/ui/spice-display.h
> @@ -31,6 +31,8 @@
>
>   #define NUM_SURFACES 1024
>
> +struct SimpleSpiceUpdate;
> +
>   typedef struct SimpleSpiceDisplay {
>       DisplayState *ds;
>       void *buf;
> @@ -48,6 +50,10 @@ typedef struct SimpleSpiceDisplay {
>        * and in native mode) and without qxl */
>       pthread_t          main;
>       int                pipe[2];     /* to iothread */
> +
> +    /* ssd updates (one request/command at a time) */
> +    struct SimpleSpiceUpdate *update;
> +    int waiting_for_update;
>   } SimpleSpiceDisplay;
>
>   typedef struct SimpleSpiceUpdate {
> @@ -75,4 +81,4 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd);
>   int qxl_vga_mode_get_command(
>       SimpleSpiceDisplay *ssd, struct QXLCommandExt *ext);
>   /* used by both qxl and spice-display */
> -void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd);
> +void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd);

Regards,

Hans

Re: [Qemu-devel] [PATCH 3/4] qxl/spice: remove qemu_mutex_{un, }lock_iothread around dispatcher

[Hans de Goede]

Ack (assuming the issues with the previous 2 patches are fixed):

Acked-by: Hans de Goede <hdegoede@redhat.com>



On 03/15/2011 09:17 PM, Alon Levy wrote:
> with the previous patch making sure get_command no longer needs to lock,
> there is no reason to drop the qemu iothread mutex in qxl.c and in
> ui/spice-display.c
>
> The only location where the lock remains are the cursor related callbacks,
> that path is currently broken. It is only triggered if running spice and sdl,
> which is broken already before that.
> ---
>   hw/qxl.c           |    8 --------
>   ui/spice-display.c |   11 ++---------
>   2 files changed, 2 insertions(+), 17 deletions(-)
>
> diff --git a/hw/qxl.c b/hw/qxl.c
> index 2419236..72f204b 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -707,10 +707,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);
>
> @@ -840,9 +838,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));
>   }
>
> @@ -911,9 +907,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)
> @@ -983,10 +977,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 a9ecee0..f3dfba8 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -78,9 +78,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 +139,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;
>   }
>
> @@ -169,6 +166,7 @@ void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd)
>       ssd->worker->add_memslot(ssd->worker,&memslot);
>   }
>
> +/* called from iothread (main) or a vcpu-thread */
>   void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
>   {
>       QXLDevSurfaceCreate surface;
> @@ -186,18 +184,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)
> @@ -207,9 +201,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;
>   }
> @@ -233,6 +225,7 @@ void qemu_spice_display_update(SimpleSpiceDisplay *ssd,
>       qemu_spice_rect_union(&ssd->dirty,&update_area);
>   }
>
> +/* called only from iothread (main) */
>   void qemu_spice_display_resize(SimpleSpiceDisplay *ssd)
>   {
>       dprint(1, "%s:\n", __FUNCTION__);

Re: [Qemu-devel] [PATCH 4/4] hw/qxl-render: drop cursor locks, add TODO's.

[Hans de Goede]

Hi,

As discussed on irc I think we need to look into this and see
if we can fix it properly while at it.

IOW to be continued...

Regards,

Hans


On 03/15/2011 09:17 PM, Alon Levy wrote:
> Dropping the locks prevents a deadlock when running with -sdl or -vnc
> in addition to -spice.
>
> When server calls get_cursor_command, and we have an active ds
> cursor related callback in non vga mode, we need to lock to prevent
> the iothread (via sdl/vnc gui_update timer) from touching the ds as well.
>
> Currently (-sdl/-vnc) + -spice seems to work, due to dropping the locking in
> qxl-render.c:qxl_render_cursor, but this is just waiting to break because of
> touching the cursor from two threads without any locking.
> ---
>   hw/qxl-render.c |   13 +++++++++----
>   1 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/qxl-render.c b/hw/qxl-render.c
> index 58965e0..1065388 100644
> --- a/hw/qxl-render.c
> +++ b/hw/qxl-render.c
> @@ -209,18 +209,23 @@ void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
>           if (c == NULL) {
>               c = cursor_builtin_left_ptr();
>           }
> -        qemu_mutex_lock_iothread();
> +        /* TODO: move this operation to iothread via pipe
> +         * we can't use the global lock here without dropping it
> +         * in gui_update (vl.c), or we get a dead lock (gui_update
> +         * calls dispatcher, waiting on pipe read, and spice server calls
> +         * this function, waiting on the lock that iothread is holding).
> +         * But when used with sdl this calls sdl.c:sdl_mouse_define, which
> +         * afaict must be locked or called from iothread. Moving to iothread
> +         * seems easiest from correctness pov. */
>           qxl->ssd.ds->cursor_define(c);
>           qxl->ssd.ds->mouse_set(x, y, 1);
> -        qemu_mutex_unlock_iothread();
>           cursor_put(c);
>           break;
>       case QXL_CURSOR_MOVE:
>           x = cmd->u.position.x;
>           y = cmd->u.position.y;
> -        qemu_mutex_lock_iothread();
> +        /* TODO: move this operation to iothread via pipe. See comment above */
>           qxl->ssd.ds->mouse_set(x, y, 1);
> -        qemu_mutex_unlock_iothread();
>           break;
>       }
>   }