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

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

[Alon Levy]

v2->v3 changes: (Hans de Goede)
 * minor review fixes
 * fix compilation of qemu with --enable-spice with a target which does
  not have a qxl device

v1->v2 changes:
 * patch 4 rewrite: instead of todos implemented usage of pipe for cursor_set
  and cursor_move.
 * small fixes: (Hans de Goede)
  * fix introducing function in patch 1 and renaming it in patch 2.
   (s/qxl_create_vcpu_to_iothread_pipe/qxl_create_server_to_iothread_pipe/)
  * move prototype defined in patch 1 but used in patch 2 to patch 2.
   (qxl_vga_mode_get_command)

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. Also tested -sdl and -vnc together with -spice.


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, replace with pipe

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

 hw/qxl-render.c    |   25 ++++---
 hw/qxl.c           |  193 +++++++++++++++++++++++++++++++++++++++++-----------
 hw/qxl.h           |    8 +-
 ui/spice-display.c |  115 +++++++++++++++++++++++++------
 ui/spice-display.h |   35 ++++++++++
 5 files changed, 300 insertions(+), 76 deletions(-)

-- 
1.7.4.1

[Qemu-devel] [PATCH v3 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.

Also move the pipe creation function (which is effectively completely rewritten
by this patch anyways) from hw/qxl.c to ui/spice-display.c, since
spice-display will depend on it after the next patch and qemu can be build
with ui/spice-display.c in combination with no hw/qxl.c.
---
 hw/qxl.c           |   22 +++++-----------------
 hw/qxl.h           |    4 ----
 ui/spice-display.c |   21 +++++++++++++++++++++
 ui/spice-display.h |    8 ++++++++
 4 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index fe4212b..201698f 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,10 +1078,11 @@ 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__);
         }
     }
@@ -1089,20 +1090,7 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
 
 static void init_pipe_signaling(PCIQXLDevice *d)
 {
-   if (pipe(d->pipe) < 0) {
-       dprint(d, 1, "%s: pipe creation failed\n", __FUNCTION__);
-       return;
-   }
-#ifdef CONFIG_IOTHREAD
-   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK);
-#else
-   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK /* | O_ASYNC */);
-#endif
-   fcntl(d->pipe[1], F_SETFL, O_NONBLOCK);
-   fcntl(d->pipe[0], F_SETOWN, getpid());
-
-   d->main = pthread_self();
-   qemu_set_fd_handler(d->pipe[0], pipe_read, NULL, d);
+   qxl_create_server_to_iothread_pipe(&d->ssd, pipe_read);
 }
 
 /* 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..ec6e0cb 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -394,6 +394,27 @@ static DisplayChangeListener display_listener = {
     .dpy_refresh = display_refresh,
 };
 
+void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd,
+    IOHandler *pipe_read)
+{
+    if (pipe(ssd->pipe) < 0) {
+        fprintf(stderr, "%s: pipe creation failed\n", __FUNCTION__);
+        return;
+    }
+
+#ifdef CONFIG_IOTHREAD
+    fcntl(ssd->pipe[0], F_SETFL, O_NONBLOCK);
+#else
+    fcntl(ssd->pipe[0], F_SETFL, O_NONBLOCK /* | O_ASYNC */);
+#endif
+    fcntl(ssd->pipe[1], F_SETFL, O_NONBLOCK);
+
+    fcntl(ssd->pipe[0], F_SETOWN, getpid());
+
+    qemu_set_fd_handler(ssd->pipe[0], pipe_read, NULL, ssd);
+    ssd->main = pthread_self();
+}
+
 void qemu_spice_display_init(DisplayState *ds)
 {
     assert(sdpy.ds == NULL);
diff --git a/ui/spice-display.h b/ui/spice-display.h
index aef0464..3e6cf7c 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,6 @@ 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);
+/* used by both qxl and spice-display */
+void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd,
+    IOHandler *pipe_read);
-- 
1.7.4.1

[Qemu-devel] [PATCH v3 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           |   81 +++++++++++++++++++++++++++++++++++++++------------
 ui/spice-display.c |   75 +++++++++++++++++++++++++++++++++++++++++++----
 ui/spice-display.h |   16 ++++++++++
 3 files changed, 146 insertions(+), 26 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 201698f..64580f1 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -340,7 +340,6 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
 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 +347,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 != QXL_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 +1065,50 @@ static void qxl_map(PCIDevice *pci, int region_num,
 
 static void pipe_read(void *opaque)
 {
-    PCIQXLDevice *d = opaque;
-    char dummy;
-    int len;
-
-    do {
-        len = read(d->ssd.pipe[0], &dummy, sizeof(dummy));
-    } while (len == sizeof(dummy));
-    qxl_set_irq(d);
+    SimpleSpiceDisplay *ssd = opaque;
+    unsigned char cmd;
+    int len, set_irq = 0;
+    int create_update = 0;
+
+    while (1) {
+        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();
+        }
+    }
+    /* 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 = QXL_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,9 +1123,11 @@ 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);
     }
 }
 
diff --git a/ui/spice-display.c b/ui/spice-display.c
index ec6e0cb..bdd14b8 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -294,18 +294,39 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
     info->n_surfaces = NUM_SURFACES;
 }
 
+/* Called from spice server thread context (via interface_get_command) */
+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 != QXL_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;
+}
+
 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)
@@ -394,6 +415,45 @@ static DisplayChangeListener display_listener = {
     .dpy_refresh = display_refresh,
 };
 
+static void pipe_read(void *opaque)
+{
+    SimpleSpiceDisplay *ssd = opaque;
+    unsigned char cmd;
+    int len, create_update = 0;
+
+    while (1) {
+        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_CREATE_UPDATE:
+            create_update = 1;
+            break;
+        default:
+            fprintf(stderr, "%s: unknown cmd %u\n", __FUNCTION__, cmd);
+            abort();
+        }
+    }
+    /* no need to do this more than once */
+    if (create_update) {
+        assert(ssd->update == NULL);
+        ssd->update = qemu_spice_create_update(ssd);
+        if (ssd->update == NULL) {
+            ssd->update = QXL_EMPTY_UPDATE;
+        }
+        ssd->worker->wakeup(ssd->worker);
+    }
+}
+
 void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd,
     IOHandler *pipe_read)
 {
@@ -427,6 +487,7 @@ void qemu_spice_display_init(DisplayState *ds)
     qemu_spice_add_interface(&sdpy.qxl.base);
     assert(sdpy.worker);
 
+    qxl_create_server_to_iothread_pipe(&sdpy, pipe_read);
     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 3e6cf7c..2be6a34 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -31,6 +31,15 @@
 
 #define NUM_SURFACES 1024
 
+/* For commands/requests from server thread to iothread */
+#define QXL_EMPTY_UPDATE ((void *)-1)
+enum {
+    QXL_SERVER_SET_IRQ = 1,
+    QXL_SERVER_CREATE_UPDATE,
+};
+
+struct SimpleSpiceUpdate;
+
 typedef struct SimpleSpiceDisplay {
     DisplayState *ds;
     void *buf;
@@ -48,6 +57,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 {
@@ -71,6 +84,9 @@ 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_server_to_iothread_pipe(SimpleSpiceDisplay *ssd,
     IOHandler *pipe_read);
-- 
1.7.4.1

[Qemu-devel] [PATCH v3 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 |   19 +++----------------
 2 files changed, 3 insertions(+), 24 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 64580f1..cf3c938 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -670,10 +670,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);
 
@@ -803,9 +801,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));
 }
 
@@ -874,9 +870,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)
@@ -946,10 +940,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 bdd14b8..ecb22cc 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -62,13 +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.
- */
+/* Called from io-thread context (via pipe_read) */
 SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
 {
     SimpleSpiceUpdate *update;
@@ -78,9 +72,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 +133,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 +160,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 +178,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 +195,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 +219,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 v3 4/4] hw/qxl-render: drop cursor locks, replace with pipe

[Alon Levy]

Switching locking protection of ds->cursor_set/cursor_move to moving
every call to these functions into the iothread and using the ssd->pipe
to transfer that, adding QXL_SERVER_CURSOR_SET, QXL_SERVER_CURSOR_MOVE.

This is tested with both -vnc :0 -spice and -sdl -spice.
---
 hw/qxl-render.c    |   25 +++++++++-----
 hw/qxl.c           |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qxl.h           |    4 ++
 ui/spice-display.h |   13 +++++++-
 4 files changed, 122 insertions(+), 10 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 58965e0..6759edb 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -178,7 +178,6 @@ fail:
     return NULL;
 }
 
-
 /* called from spice server thread context only */
 void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
 {
@@ -209,18 +208,26 @@ 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);
+        qxl_server_request_cursor_set(qxl, c, x, y);
         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();
+        qxl_server_request_cursor_move(qxl, x, y);
         break;
     }
 }
+
+/* called from iothread only (via qxl.c:pipe_read) */
+void qxl_render_cursor_set(SimpleSpiceDisplay *ssd, QEMUCursor *c, int x, int y)
+{
+    ssd->ds->cursor_define(c);
+    ssd->ds->mouse_set(x, y, 1);
+    cursor_put(c);
+}
+
+/* called from iothread only (via qxl.c:pipe_read) */
+void qxl_render_cursor_move(SimpleSpiceDisplay *ssd, int x, int y)
+{
+    ssd->ds->mouse_set(x, y, 1);
+}
diff --git a/hw/qxl.c b/hw/qxl.c
index cf3c938..4c27deb 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -117,6 +117,27 @@ static QXLMode qxl_modes[] = {
 #endif
 };
 
+typedef struct __attribute__ ((__packed__)) {
+    QEMUCursor *c;
+    int x;
+    int y;
+} QXLServerCursorSet;
+
+typedef struct __attribute__ ((__packed__)) {
+    int x;
+    int y;
+} QXLServerCursorMove;
+
+typedef struct __attribute__ ((__packed__)) {
+    unsigned char req;
+    QXLServerCursorMove data;
+} QXLServerCursorMoveRequest;
+
+typedef struct __attribute__ ((__packed__)) {
+    unsigned char req;
+    QXLServerCursorSet data;
+} QXLServerCursorSetRequest;
+
 static PCIQXLDevice *qxl0;
 
 static void qxl_send_events(PCIQXLDevice *d, uint32_t events);
@@ -337,6 +358,33 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
 }
 
 /* called from spice server thread context only */
+void qxl_server_request_cursor_set(PCIQXLDevice *qxl, QEMUCursor *c, int x, int y)
+{
+    QXLServerCursorSetRequest req;
+    int r;
+
+    req.req = QXL_SERVER_CURSOR_SET;
+    req.data.c = c;
+    req.data.x = x;
+    req.data.y = y;
+    r = write(qxl->ssd.pipe[1], &req, sizeof(req));
+    assert(r == sizeof(req));
+}
+
+/* called from spice server thread context only */
+void qxl_server_request_cursor_move(PCIQXLDevice *qxl, int x, int y)
+{
+    QXLServerCursorMoveRequest req;
+    int r;
+
+    req.req = QXL_SERVER_CURSOR_MOVE;
+    req.data.x = x;
+    req.data.y = y;
+    r = write(qxl->ssd.pipe[1], &req, sizeof(req));
+    assert(r == sizeof(req));
+}
+
+/* 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);
@@ -1055,12 +1103,37 @@ static void qxl_map(PCIDevice *pci, int region_num,
     }
 }
 
+static void read_bytes(int fd, void *buf, int len_requested)
+{
+    int len;
+    int total_len = 0;
+
+    do {
+        len = read(fd, buf, len_requested - total_len);
+        if (len < 0) {
+            if (errno == EINTR || errno == EAGAIN) {
+                continue;
+            }
+            perror("qxl: pipe_read: read failed");
+            /* will abort once it's out of the while loop */
+            break;
+        }
+        total_len += len;
+        buf = (uint8_t *)buf + len;
+    } while (total_len < len_requested);
+    assert(total_len == len_requested);
+}
+
 static void pipe_read(void *opaque)
 {
     SimpleSpiceDisplay *ssd = opaque;
     unsigned char cmd;
     int len, set_irq = 0;
     int create_update = 0;
+    int cursor_set = 0;
+    int cursor_move = 0;
+    QXLServerCursorSet cursor_set_data;
+    QXLServerCursorMove cursor_move_data;
 
     while (1) {
         cmd = 0;
@@ -1082,6 +1155,17 @@ static void pipe_read(void *opaque)
         case QXL_SERVER_CREATE_UPDATE:
             create_update = 1;
             break;
+        case QXL_SERVER_CURSOR_SET:
+            if (cursor_set == 1) {
+                cursor_put(cursor_set_data.c);
+            }
+            cursor_set = 1;
+            read_bytes(ssd->pipe[0], &cursor_set_data, sizeof(cursor_set_data));
+            break;
+        case QXL_SERVER_CURSOR_MOVE:
+            cursor_move = 1;
+            read_bytes(ssd->pipe[0], &cursor_move_data, sizeof(cursor_move_data));
+            break;
         default:
             fprintf(stderr, "%s: unknown cmd %u\n", __FUNCTION__, cmd);
             abort();
@@ -1099,6 +1183,12 @@ static void pipe_read(void *opaque)
     if (set_irq) {
         qxl_set_irq(container_of(ssd, PCIQXLDevice, ssd));
     }
+    if (cursor_move) {
+        qxl_render_cursor_move(ssd, cursor_move_data.x, cursor_move_data.y);
+    }
+    if (cursor_set) {
+        qxl_render_cursor_set(ssd, cursor_set_data.c, cursor_set_data.x, cursor_set_data.y);
+    }
 }
 
 static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
diff --git a/hw/qxl.h b/hw/qxl.h
index 701245f..f4f99ec 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -93,6 +93,8 @@ typedef struct PCIQXLDevice {
 
 /* qxl.c */
 void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
+void qxl_server_request_cursor_set(PCIQXLDevice *qxl, QEMUCursor *c, int x, int y);
+void qxl_server_request_cursor_move(PCIQXLDevice *qxl, int x, int y);
 
 /* qxl-logger.c */
 void qxl_log_cmd_cursor(PCIQXLDevice *qxl, QXLCursorCmd *cmd, int group_id);
@@ -102,3 +104,5 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext);
 void qxl_render_resize(PCIQXLDevice *qxl);
 void qxl_render_update(PCIQXLDevice *qxl);
 void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext);
+void qxl_render_cursor_set(SimpleSpiceDisplay *ssd, QEMUCursor *c, int x, int y);
+void qxl_render_cursor_move(SimpleSpiceDisplay *ssd, int x, int y);
diff --git a/ui/spice-display.h b/ui/spice-display.h
index 2be6a34..bbfd689 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -31,11 +31,22 @@
 
 #define NUM_SURFACES 1024
 
-/* For commands/requests from server thread to iothread */
+/*
+ * Commands/requests from server thread to iothread.
+ * Note that CREATE_UPDATE is used both with qxl and without it (spice-display)
+ * the others are only used with the qxl device.
+ *
+ * SET_IRQ - just the request is sent (1 byte)
+ * CREATE_UPDATE - jus the request is sent (1 byte)
+ * CURSOR_SET - send QXLServerRequestCursorSet
+ * CURSOR_MOVE - send QXLServerRequestCursorMove
+ */
 #define QXL_EMPTY_UPDATE ((void *)-1)
 enum {
     QXL_SERVER_SET_IRQ = 1,
     QXL_SERVER_CREATE_UPDATE,
+    QXL_SERVER_CURSOR_SET,
+    QXL_SERVER_CURSOR_MOVE
 };
 
 struct SimpleSpiceUpdate;
-- 
1.7.4.1

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

[Hans de Goede]

Looks good now, ack:

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


On 03/16/2011 04:52 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.
>
> Also move the pipe creation function (which is effectively completely rewritten
> by this patch anyways) from hw/qxl.c to ui/spice-display.c, since
> spice-display will depend on it after the next patch and qemu can be build
> with ui/spice-display.c in combination with no hw/qxl.c.
> ---
>   hw/qxl.c           |   22 +++++-----------------
>   hw/qxl.h           |    4 ----
>   ui/spice-display.c |   21 +++++++++++++++++++++
>   ui/spice-display.h |    8 ++++++++
>   4 files changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/hw/qxl.c b/hw/qxl.c
> index fe4212b..201698f 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,10 +1078,11 @@ 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__);
>           }
>       }
> @@ -1089,20 +1090,7 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
>
>   static void init_pipe_signaling(PCIQXLDevice *d)
>   {
> -   if (pipe(d->pipe)<  0) {
> -       dprint(d, 1, "%s: pipe creation failed\n", __FUNCTION__);
> -       return;
> -   }
> -#ifdef CONFIG_IOTHREAD
> -   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK);
> -#else
> -   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK /* | O_ASYNC */);
> -#endif
> -   fcntl(d->pipe[1], F_SETFL, O_NONBLOCK);
> -   fcntl(d->pipe[0], F_SETOWN, getpid());
> -
> -   d->main = pthread_self();
> -   qemu_set_fd_handler(d->pipe[0], pipe_read, NULL, d);
> +   qxl_create_server_to_iothread_pipe(&d->ssd, pipe_read);
>   }
>
>   /* 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..ec6e0cb 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -394,6 +394,27 @@ static DisplayChangeListener display_listener = {
>       .dpy_refresh = display_refresh,
>   };
>
> +void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd,
> +    IOHandler *pipe_read)
> +{
> +    if (pipe(ssd->pipe)<  0) {
> +        fprintf(stderr, "%s: pipe creation failed\n", __FUNCTION__);
> +        return;
> +    }
> +
> +#ifdef CONFIG_IOTHREAD
> +    fcntl(ssd->pipe[0], F_SETFL, O_NONBLOCK);
> +#else
> +    fcntl(ssd->pipe[0], F_SETFL, O_NONBLOCK /* | O_ASYNC */);
> +#endif
> +    fcntl(ssd->pipe[1], F_SETFL, O_NONBLOCK);
> +
> +    fcntl(ssd->pipe[0], F_SETOWN, getpid());
> +
> +    qemu_set_fd_handler(ssd->pipe[0], pipe_read, NULL, ssd);
> +    ssd->main = pthread_self();
> +}
> +
>   void qemu_spice_display_init(DisplayState *ds)
>   {
>       assert(sdpy.ds == NULL);
> diff --git a/ui/spice-display.h b/ui/spice-display.h
> index aef0464..3e6cf7c 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,6 @@ 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);
> +/* used by both qxl and spice-display */
> +void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd,
> +    IOHandler *pipe_read);

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

[Hans de Goede]

Looks good now, ack:

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


On 03/16/2011 04:52 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           |   81 +++++++++++++++++++++++++++++++++++++++------------
>   ui/spice-display.c |   75 +++++++++++++++++++++++++++++++++++++++++++----
>   ui/spice-display.h |   16 ++++++++++
>   3 files changed, 146 insertions(+), 26 deletions(-)
>
> diff --git a/hw/qxl.c b/hw/qxl.c
> index 201698f..64580f1 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -340,7 +340,6 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
>   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 +347,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 != QXL_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 +1065,50 @@ static void qxl_map(PCIDevice *pci, int region_num,
>
>   static void pipe_read(void *opaque)
>   {
> -    PCIQXLDevice *d = opaque;
> -    char dummy;
> -    int len;
> -
> -    do {
> -        len = read(d->ssd.pipe[0],&dummy, sizeof(dummy));
> -    } while (len == sizeof(dummy));
> -    qxl_set_irq(d);
> +    SimpleSpiceDisplay *ssd = opaque;
> +    unsigned char cmd;
> +    int len, set_irq = 0;
> +    int create_update = 0;
> +
> +    while (1) {
> +        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();
> +        }
> +    }
> +    /* 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 = QXL_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,9 +1123,11 @@ 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);
>       }
>   }
>
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index ec6e0cb..bdd14b8 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -294,18 +294,39 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
>       info->n_surfaces = NUM_SURFACES;
>   }
>
> +/* Called from spice server thread context (via interface_get_command) */
> +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 != QXL_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;
> +}
> +
>   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)
> @@ -394,6 +415,45 @@ static DisplayChangeListener display_listener = {
>       .dpy_refresh = display_refresh,
>   };
>
> +static void pipe_read(void *opaque)
> +{
> +    SimpleSpiceDisplay *ssd = opaque;
> +    unsigned char cmd;
> +    int len, create_update = 0;
> +
> +    while (1) {
> +        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_CREATE_UPDATE:
> +            create_update = 1;
> +            break;
> +        default:
> +            fprintf(stderr, "%s: unknown cmd %u\n", __FUNCTION__, cmd);
> +            abort();
> +        }
> +    }
> +    /* no need to do this more than once */
> +    if (create_update) {
> +        assert(ssd->update == NULL);
> +        ssd->update = qemu_spice_create_update(ssd);
> +        if (ssd->update == NULL) {
> +            ssd->update = QXL_EMPTY_UPDATE;
> +        }
> +        ssd->worker->wakeup(ssd->worker);
> +    }
> +}
> +
>   void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd,
>       IOHandler *pipe_read)
>   {
> @@ -427,6 +487,7 @@ void qemu_spice_display_init(DisplayState *ds)
>       qemu_spice_add_interface(&sdpy.qxl.base);
>       assert(sdpy.worker);
>
> +    qxl_create_server_to_iothread_pipe(&sdpy, pipe_read);
>       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 3e6cf7c..2be6a34 100644
> --- a/ui/spice-display.h
> +++ b/ui/spice-display.h
> @@ -31,6 +31,15 @@
>
>   #define NUM_SURFACES 1024
>
> +/* For commands/requests from server thread to iothread */
> +#define QXL_EMPTY_UPDATE ((void *)-1)
> +enum {
> +    QXL_SERVER_SET_IRQ = 1,
> +    QXL_SERVER_CREATE_UPDATE,
> +};
> +
> +struct SimpleSpiceUpdate;
> +
>   typedef struct SimpleSpiceDisplay {
>       DisplayState *ds;
>       void *buf;
> @@ -48,6 +57,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 {
> @@ -71,6 +84,9 @@ 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_server_to_iothread_pipe(SimpleSpiceDisplay *ssd,
>       IOHandler *pipe_read);

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

[Hans de Goede]

Looks good now, ack:

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


On 03/16/2011 04:52 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 |   19 +++----------------
>   2 files changed, 3 insertions(+), 24 deletions(-)
>
> diff --git a/hw/qxl.c b/hw/qxl.c
> index 64580f1..cf3c938 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -670,10 +670,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);
>
> @@ -803,9 +801,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));
>   }
>
> @@ -874,9 +870,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)
> @@ -946,10 +940,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 bdd14b8..ecb22cc 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -62,13 +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.
> - */
> +/* Called from io-thread context (via pipe_read) */
>   SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
>   {
>       SimpleSpiceUpdate *update;
> @@ -78,9 +72,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 +133,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 +160,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 +178,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 +195,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 +219,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__);

[Qemu-devel] Re: [PATCH v3 4/4] hw/qxl-render: drop cursor locks, replace with pipe

[Hans de Goede]

Looks good now, ack:

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


On 03/16/2011 04:52 PM, Alon Levy wrote:
> Switching locking protection of ds->cursor_set/cursor_move to moving
> every call to these functions into the iothread and using the ssd->pipe
> to transfer that, adding QXL_SERVER_CURSOR_SET, QXL_SERVER_CURSOR_MOVE.
>
> This is tested with both -vnc :0 -spice and -sdl -spice.
> ---
>   hw/qxl-render.c    |   25 +++++++++-----
>   hw/qxl.c           |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/qxl.h           |    4 ++
>   ui/spice-display.h |   13 +++++++-
>   4 files changed, 122 insertions(+), 10 deletions(-)
>
> diff --git a/hw/qxl-render.c b/hw/qxl-render.c
> index 58965e0..6759edb 100644
> --- a/hw/qxl-render.c
> +++ b/hw/qxl-render.c
> @@ -178,7 +178,6 @@ fail:
>       return NULL;
>   }
>
> -
>   /* called from spice server thread context only */
>   void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
>   {
> @@ -209,18 +208,26 @@ 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);
> +        qxl_server_request_cursor_set(qxl, c, x, y);
>           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();
> +        qxl_server_request_cursor_move(qxl, x, y);
>           break;
>       }
>   }
> +
> +/* called from iothread only (via qxl.c:pipe_read) */
> +void qxl_render_cursor_set(SimpleSpiceDisplay *ssd, QEMUCursor *c, int x, int y)
> +{
> +    ssd->ds->cursor_define(c);
> +    ssd->ds->mouse_set(x, y, 1);
> +    cursor_put(c);
> +}
> +
> +/* called from iothread only (via qxl.c:pipe_read) */
> +void qxl_render_cursor_move(SimpleSpiceDisplay *ssd, int x, int y)
> +{
> +    ssd->ds->mouse_set(x, y, 1);
> +}
> diff --git a/hw/qxl.c b/hw/qxl.c
> index cf3c938..4c27deb 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -117,6 +117,27 @@ static QXLMode qxl_modes[] = {
>   #endif
>   };
>
> +typedef struct __attribute__ ((__packed__)) {
> +    QEMUCursor *c;
> +    int x;
> +    int y;
> +} QXLServerCursorSet;
> +
> +typedef struct __attribute__ ((__packed__)) {
> +    int x;
> +    int y;
> +} QXLServerCursorMove;
> +
> +typedef struct __attribute__ ((__packed__)) {
> +    unsigned char req;
> +    QXLServerCursorMove data;
> +} QXLServerCursorMoveRequest;
> +
> +typedef struct __attribute__ ((__packed__)) {
> +    unsigned char req;
> +    QXLServerCursorSet data;
> +} QXLServerCursorSetRequest;
> +
>   static PCIQXLDevice *qxl0;
>
>   static void qxl_send_events(PCIQXLDevice *d, uint32_t events);
> @@ -337,6 +358,33 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
>   }
>
>   /* called from spice server thread context only */
> +void qxl_server_request_cursor_set(PCIQXLDevice *qxl, QEMUCursor *c, int x, int y)
> +{
> +    QXLServerCursorSetRequest req;
> +    int r;
> +
> +    req.req = QXL_SERVER_CURSOR_SET;
> +    req.data.c = c;
> +    req.data.x = x;
> +    req.data.y = y;
> +    r = write(qxl->ssd.pipe[1],&req, sizeof(req));
> +    assert(r == sizeof(req));
> +}
> +
> +/* called from spice server thread context only */
> +void qxl_server_request_cursor_move(PCIQXLDevice *qxl, int x, int y)
> +{
> +    QXLServerCursorMoveRequest req;
> +    int r;
> +
> +    req.req = QXL_SERVER_CURSOR_MOVE;
> +    req.data.x = x;
> +    req.data.y = y;
> +    r = write(qxl->ssd.pipe[1],&req, sizeof(req));
> +    assert(r == sizeof(req));
> +}
> +
> +/* 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);
> @@ -1055,12 +1103,37 @@ static void qxl_map(PCIDevice *pci, int region_num,
>       }
>   }
>
> +static void read_bytes(int fd, void *buf, int len_requested)
> +{
> +    int len;
> +    int total_len = 0;
> +
> +    do {
> +        len = read(fd, buf, len_requested - total_len);
> +        if (len<  0) {
> +            if (errno == EINTR || errno == EAGAIN) {
> +                continue;
> +            }
> +            perror("qxl: pipe_read: read failed");
> +            /* will abort once it's out of the while loop */
> +            break;
> +        }
> +        total_len += len;
> +        buf = (uint8_t *)buf + len;
> +    } while (total_len<  len_requested);
> +    assert(total_len == len_requested);
> +}
> +
>   static void pipe_read(void *opaque)
>   {
>       SimpleSpiceDisplay *ssd = opaque;
>       unsigned char cmd;
>       int len, set_irq = 0;
>       int create_update = 0;
> +    int cursor_set = 0;
> +    int cursor_move = 0;
> +    QXLServerCursorSet cursor_set_data;
> +    QXLServerCursorMove cursor_move_data;
>
>       while (1) {
>           cmd = 0;
> @@ -1082,6 +1155,17 @@ static void pipe_read(void *opaque)
>           case QXL_SERVER_CREATE_UPDATE:
>               create_update = 1;
>               break;
> +        case QXL_SERVER_CURSOR_SET:
> +            if (cursor_set == 1) {
> +                cursor_put(cursor_set_data.c);
> +            }
> +            cursor_set = 1;
> +            read_bytes(ssd->pipe[0],&cursor_set_data, sizeof(cursor_set_data));
> +            break;
> +        case QXL_SERVER_CURSOR_MOVE:
> +            cursor_move = 1;
> +            read_bytes(ssd->pipe[0],&cursor_move_data, sizeof(cursor_move_data));
> +            break;
>           default:
>               fprintf(stderr, "%s: unknown cmd %u\n", __FUNCTION__, cmd);
>               abort();
> @@ -1099,6 +1183,12 @@ static void pipe_read(void *opaque)
>       if (set_irq) {
>           qxl_set_irq(container_of(ssd, PCIQXLDevice, ssd));
>       }
> +    if (cursor_move) {
> +        qxl_render_cursor_move(ssd, cursor_move_data.x, cursor_move_data.y);
> +    }
> +    if (cursor_set) {
> +        qxl_render_cursor_set(ssd, cursor_set_data.c, cursor_set_data.x, cursor_set_data.y);
> +    }
>   }
>
>   static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
> diff --git a/hw/qxl.h b/hw/qxl.h
> index 701245f..f4f99ec 100644
> --- a/hw/qxl.h
> +++ b/hw/qxl.h
> @@ -93,6 +93,8 @@ typedef struct PCIQXLDevice {
>
>   /* qxl.c */
>   void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
> +void qxl_server_request_cursor_set(PCIQXLDevice *qxl, QEMUCursor *c, int x, int y);
> +void qxl_server_request_cursor_move(PCIQXLDevice *qxl, int x, int y);
>
>   /* qxl-logger.c */
>   void qxl_log_cmd_cursor(PCIQXLDevice *qxl, QXLCursorCmd *cmd, int group_id);
> @@ -102,3 +104,5 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext);
>   void qxl_render_resize(PCIQXLDevice *qxl);
>   void qxl_render_update(PCIQXLDevice *qxl);
>   void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext);
> +void qxl_render_cursor_set(SimpleSpiceDisplay *ssd, QEMUCursor *c, int x, int y);
> +void qxl_render_cursor_move(SimpleSpiceDisplay *ssd, int x, int y);
> diff --git a/ui/spice-display.h b/ui/spice-display.h
> index 2be6a34..bbfd689 100644
> --- a/ui/spice-display.h
> +++ b/ui/spice-display.h
> @@ -31,11 +31,22 @@
>
>   #define NUM_SURFACES 1024
>
> -/* For commands/requests from server thread to iothread */
> +/*
> + * Commands/requests from server thread to iothread.
> + * Note that CREATE_UPDATE is used both with qxl and without it (spice-display)
> + * the others are only used with the qxl device.
> + *
> + * SET_IRQ - just the request is sent (1 byte)
> + * CREATE_UPDATE - jus the request is sent (1 byte)
> + * CURSOR_SET - send QXLServerRequestCursorSet
> + * CURSOR_MOVE - send QXLServerRequestCursorMove
> + */
>   #define QXL_EMPTY_UPDATE ((void *)-1)
>   enum {
>       QXL_SERVER_SET_IRQ = 1,
>       QXL_SERVER_CREATE_UPDATE,
> +    QXL_SERVER_CURSOR_SET,
> +    QXL_SERVER_CURSOR_MOVE
>   };
>
>   struct SimpleSpiceUpdate;

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

[Jes Sorensen]

On 03/16/11 16:52, 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.
> 
> Also move the pipe creation function (which is effectively completely rewritten
> by this patch anyways) from hw/qxl.c to ui/spice-display.c, since
> spice-display will depend on it after the next patch and qemu can be build
> with ui/spice-display.c in combination with no hw/qxl.c.
> ---
>  hw/qxl.c           |   22 +++++-----------------
>  hw/qxl.h           |    4 ----
>  ui/spice-display.c |   21 +++++++++++++++++++++
>  ui/spice-display.h |    8 ++++++++
>  4 files changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/qxl.c b/hw/qxl.c
> index fe4212b..201698f 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,10 +1078,11 @@ 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 */
[snip]
> diff --git a/ui/spice-display.h b/ui/spice-display.h
> index aef0464..3e6cf7c 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;

This really should be using QemuThread rather than pthread_t directly. I
am not quite sure what impact it will have on the use of pthread_self()
above, there might be a need for an additional wrapper there.

Cheers,
Jes

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

[Alon Levy]

On Wed, Mar 16, 2011 at 05:39:32PM +0100, Jes Sorensen wrote:
> On 03/16/11 16:52, 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.
> > 
> > Also move the pipe creation function (which is effectively completely rewritten
> > by this patch anyways) from hw/qxl.c to ui/spice-display.c, since
> > spice-display will depend on it after the next patch and qemu can be build
> > with ui/spice-display.c in combination with no hw/qxl.c.
> > ---
> >  hw/qxl.c           |   22 +++++-----------------
> >  hw/qxl.h           |    4 ----
> >  ui/spice-display.c |   21 +++++++++++++++++++++
> >  ui/spice-display.h |    8 ++++++++
> >  4 files changed, 34 insertions(+), 21 deletions(-)
> > 
> > diff --git a/hw/qxl.c b/hw/qxl.c
> > index fe4212b..201698f 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,10 +1078,11 @@ 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 */
> [snip]
> > diff --git a/ui/spice-display.h b/ui/spice-display.h
> > index aef0464..3e6cf7c 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;
> 
> This really should be using QemuThread rather than pthread_t directly. I
> am not quite sure what impact it will have on the use of pthread_self()
> above, there might be a need for an additional wrapper there.
> 

Agreed, but it's not introduced by this patch, that code is just being moved
from qxl.h to spice-display.h, from PCIQXLDevice to SimpleSpiceDisplay (which
is embedded in PCIQXLDevice as well as used standalone when there is no qxl device).
I'll make a separate patch for this.

> Cheers,
> Jes

Re: [Qemu-devel] [PATCH v3 4/4] hw/qxl-render: drop cursor locks, replace with pipe

[Jes Sorensen]

On 03/16/11 16:52, Alon Levy wrote:
> +void qxl_server_request_cursor_set(PCIQXLDevice *qxl, QEMUCursor *c, int x, int y)
> +{
> +    QXLServerCursorSetRequest req;
> +    int r;
> +
> +    req.req = QXL_SERVER_CURSOR_SET;
> +    req.data.c = c;
> +    req.data.x = x;
> +    req.data.y = y;
> +    r = write(qxl->ssd.pipe[1], &req, sizeof(req));
> +    assert(r == sizeof(req));
> +}

There's a number of asserts here, which I am not sure is a good thing. I
don't understand how far down the code this is, and if it is really
fatal if this write fails?

> +/* called from spice server thread context only */
> +void qxl_server_request_cursor_move(PCIQXLDevice *qxl, int x, int y)
> +{
> +    QXLServerCursorMoveRequest req;
> +    int r;
> +
> +    req.req = QXL_SERVER_CURSOR_MOVE;
> +    req.data.x = x;
> +    req.data.y = y;
> +    r = write(qxl->ssd.pipe[1], &req, sizeof(req));
> +    assert(r == sizeof(req));

ditto

> +static void read_bytes(int fd, void *buf, int len_requested)
> +{
> +    int len;
> +    int total_len = 0;
> +
> +    do {
> +        len = read(fd, buf, len_requested - total_len);
> +        if (len < 0) {
> +            if (errno == EINTR || errno == EAGAIN) {
> +                continue;
> +            }
> +            perror("qxl: pipe_read: read failed");
> +            /* will abort once it's out of the while loop */
> +            break;
> +        }
> +        total_len += len;
> +        buf = (uint8_t *)buf + len;
> +    } while (total_len < len_requested);
> +    assert(total_len == len_requested);

and here?

Cheers,
Jes

Re: [Qemu-devel] [PATCH v3 4/4] hw/qxl-render: drop cursor locks, replace with pipe

[Alon Levy]

On Wed, Mar 16, 2011 at 05:48:41PM +0100, Jes Sorensen wrote:
> On 03/16/11 16:52, Alon Levy wrote:
> > +void qxl_server_request_cursor_set(PCIQXLDevice *qxl, QEMUCursor *c, int x, int y)
> > +{
> > +    QXLServerCursorSetRequest req;
> > +    int r;
> > +
> > +    req.req = QXL_SERVER_CURSOR_SET;
> > +    req.data.c = c;
> > +    req.data.x = x;
> > +    req.data.y = y;
> > +    r = write(qxl->ssd.pipe[1], &req, sizeof(req));
> > +    assert(r == sizeof(req));
> > +}
> 
> There's a number of asserts here, which I am not sure is a good thing. I
> don't understand how far down the code this is, and if it is really
> fatal if this write fails?

A failure there means we can't write to a pipe between the server thread
and the iothread (main thread). That is not supposed to happen - and if
it does it means some operation by the spice server will never complete.

Same for the asserts below, writes are from spice server thread, reads
are in iothread.

> 
> > +/* called from spice server thread context only */
> > +void qxl_server_request_cursor_move(PCIQXLDevice *qxl, int x, int y)
> > +{
> > +    QXLServerCursorMoveRequest req;
> > +    int r;
> > +
> > +    req.req = QXL_SERVER_CURSOR_MOVE;
> > +    req.data.x = x;
> > +    req.data.y = y;
> > +    r = write(qxl->ssd.pipe[1], &req, sizeof(req));
> > +    assert(r == sizeof(req));
> 
> ditto
> 
> > +static void read_bytes(int fd, void *buf, int len_requested)
> > +{
> > +    int len;
> > +    int total_len = 0;
> > +
> > +    do {
> > +        len = read(fd, buf, len_requested - total_len);
> > +        if (len < 0) {
> > +            if (errno == EINTR || errno == EAGAIN) {
> > +                continue;
> > +            }
> > +            perror("qxl: pipe_read: read failed");
> > +            /* will abort once it's out of the while loop */
> > +            break;
> > +        }
> > +        total_len += len;
> > +        buf = (uint8_t *)buf + len;
> > +    } while (total_len < len_requested);
> > +    assert(total_len == len_requested);
> 
> and here?
> 
> Cheers,
> Jes
> 
> 
> 

Re: [Qemu-devel] [PATCH v3 4/4] hw/qxl-render: drop cursor locks, replace with pipe

[Jes Sorensen]

On 03/17/11 10:32, Alon Levy wrote:
> On Wed, Mar 16, 2011 at 05:48:41PM +0100, Jes Sorensen wrote:
>> > On 03/16/11 16:52, Alon Levy wrote:
>>> > > +void qxl_server_request_cursor_set(PCIQXLDevice *qxl, QEMUCursor *c, int x, int y)
>>> > > +{
>>> > > +    QXLServerCursorSetRequest req;
>>> > > +    int r;
>>> > > +
>>> > > +    req.req = QXL_SERVER_CURSOR_SET;
>>> > > +    req.data.c = c;
>>> > > +    req.data.x = x;
>>> > > +    req.data.y = y;
>>> > > +    r = write(qxl->ssd.pipe[1], &req, sizeof(req));
>>> > > +    assert(r == sizeof(req));
>>> > > +}
>> > 
>> > There's a number of asserts here, which I am not sure is a good thing. I
>> > don't understand how far down the code this is, and if it is really
>> > fatal if this write fails?
> A failure there means we can't write to a pipe between the server thread
> and the iothread (main thread). That is not supposed to happen - and if
> it does it means some operation by the spice server will never complete.
> 
> Same for the asserts below, writes are from spice server thread, reads
> are in iothread.

But shouldn't this make it try to reconnect? Even if the reconnect
fails, it shouldn't kill the guest IMHO.

Cheers,
Jes

Re: [Qemu-devel] [PATCH v3 4/4] hw/qxl-render: drop cursor locks, replace with pipe

[Alon Levy]

On Thu, Mar 17, 2011 at 10:48:43AM +0100, Jes Sorensen wrote:
> On 03/17/11 10:32, Alon Levy wrote:
> > On Wed, Mar 16, 2011 at 05:48:41PM +0100, Jes Sorensen wrote:
> >> > On 03/16/11 16:52, Alon Levy wrote:
> >>> > > +void qxl_server_request_cursor_set(PCIQXLDevice *qxl, QEMUCursor *c, int x, int y)
> >>> > > +{
> >>> > > +    QXLServerCursorSetRequest req;
> >>> > > +    int r;
> >>> > > +
> >>> > > +    req.req = QXL_SERVER_CURSOR_SET;
> >>> > > +    req.data.c = c;
> >>> > > +    req.data.x = x;
> >>> > > +    req.data.y = y;
> >>> > > +    r = write(qxl->ssd.pipe[1], &req, sizeof(req));
> >>> > > +    assert(r == sizeof(req));
> >>> > > +}
> >> > 
> >> > There's a number of asserts here, which I am not sure is a good thing. I
> >> > don't understand how far down the code this is, and if it is really
> >> > fatal if this write fails?
> > A failure there means we can't write to a pipe between the server thread
> > and the iothread (main thread). That is not supposed to happen - and if
> > it does it means some operation by the spice server will never complete.
> > 
> > Same for the asserts below, writes are from spice server thread, reads
> > are in iothread.
> 
> But shouldn't this make it try to reconnect? Even if the reconnect
> fails, it shouldn't kill the guest IMHO.

reconnect? between two threads in the qemu process? why would the write
fail to begin with? this is like saying if I'm failing a kvm ioctl I should
just retry.

> 
> Cheers,
> Jes
> 
> 
> 

Re: [Qemu-devel] [PATCH v3 4/4] hw/qxl-render: drop cursor locks, replace with pipe

[Jes Sorensen]

On 03/17/11 11:27, Alon Levy wrote:
> On Thu, Mar 17, 2011 at 10:48:43AM +0100, Jes Sorensen wrote:
>>> Same for the asserts below, writes are from spice server thread, reads
>>> are in iothread.
>>
>> But shouldn't this make it try to reconnect? Even if the reconnect
>> fails, it shouldn't kill the guest IMHO.
> 
> reconnect? between two threads in the qemu process? why would the write
> fail to begin with? this is like saying if I'm failing a kvm ioctl I should
> just retry.

Ah ok, I missed that part, somehow I had in my mind it was two different
processes, despite you mentioning threads.

Still if gfx handling fails, it shouldn't nuke the guest.

Cheers,
Jes

Re: [Qemu-devel] [PATCH v3 4/4] hw/qxl-render: drop cursor locks, replace with pipe

[Alon Levy]

On Thu, Mar 17, 2011 at 11:29:03AM +0100, Jes Sorensen wrote:
> On 03/17/11 11:27, Alon Levy wrote:
> > On Thu, Mar 17, 2011 at 10:48:43AM +0100, Jes Sorensen wrote:
> >>> Same for the asserts below, writes are from spice server thread, reads
> >>> are in iothread.
> >>
> >> But shouldn't this make it try to reconnect? Even if the reconnect
> >> fails, it shouldn't kill the guest IMHO.
> > 
> > reconnect? between two threads in the qemu process? why would the write
> > fail to begin with? this is like saying if I'm failing a kvm ioctl I should
> > just retry.
> 
> Ah ok, I missed that part, somehow I had in my mind it was two different
> processes, despite you mentioning threads.
> 
> Still if gfx handling fails, it shouldn't nuke the guest.

ok, try to apply that logic to any other device - network, usb, etc., I don't
think it holds.

> 
> Cheers,
> Jes
> 

Re: [Qemu-devel] [PATCH v3 4/4] hw/qxl-render: drop cursor locks, replace with pipe

[Jes Sorensen]

On 03/17/11 11:45, Alon Levy wrote:
> On Thu, Mar 17, 2011 at 11:29:03AM +0100, Jes Sorensen wrote:
>> On 03/17/11 11:27, Alon Levy wrote:
>>> On Thu, Mar 17, 2011 at 10:48:43AM +0100, Jes Sorensen wrote:
>>>>> Same for the asserts below, writes are from spice server thread, reads
>>>>> are in iothread.
>>>>
>>>> But shouldn't this make it try to reconnect? Even if the reconnect
>>>> fails, it shouldn't kill the guest IMHO.
>>>
>>> reconnect? between two threads in the qemu process? why would the write
>>> fail to begin with? this is like saying if I'm failing a kvm ioctl I should
>>> just retry.
>>
>> Ah ok, I missed that part, somehow I had in my mind it was two different
>> processes, despite you mentioning threads.
>>
>> Still if gfx handling fails, it shouldn't nuke the guest.
> 
> ok, try to apply that logic to any other device - network, usb, etc., I don't
> think it holds.

Maybe I am looking at the wrong angle - I would think that is network or
usb breaks, we would still keep running, and for gfx the guest should be
able to keep running even if the monitor is disconnected.

It's not a big issue so if you feel it is fine as is, I won't object.

Cheers,
Jes

Re: [Qemu-devel] [PATCH v3 4/4] hw/qxl-render: drop cursor locks, replace with pipe

[Alon Levy]

On Thu, Mar 17, 2011 at 03:19:28PM +0100, Jes Sorensen wrote:
> On 03/17/11 11:45, Alon Levy wrote:
> > On Thu, Mar 17, 2011 at 11:29:03AM +0100, Jes Sorensen wrote:
> >> On 03/17/11 11:27, Alon Levy wrote:
> >>> On Thu, Mar 17, 2011 at 10:48:43AM +0100, Jes Sorensen wrote:
> >>>>> Same for the asserts below, writes are from spice server thread, reads
> >>>>> are in iothread.
> >>>>
> >>>> But shouldn't this make it try to reconnect? Even if the reconnect
> >>>> fails, it shouldn't kill the guest IMHO.
> >>>
> >>> reconnect? between two threads in the qemu process? why would the write
> >>> fail to begin with? this is like saying if I'm failing a kvm ioctl I should
> >>> just retry.
> >>
> >> Ah ok, I missed that part, somehow I had in my mind it was two different
> >> processes, despite you mentioning threads.
> >>
> >> Still if gfx handling fails, it shouldn't nuke the guest.
> > 
> > ok, try to apply that logic to any other device - network, usb, etc., I don't
> > think it holds.
> 
> Maybe I am looking at the wrong angle - I would think that is network or
> usb breaks, we would still keep running, and for gfx the guest should be
> able to keep running even if the monitor is disconnected.
> 
> It's not a big issue so if you feel it is fine as is, I won't object.

I think it could be marginally useful - I mean if someone is running a vm with
spice it's for desktop use, and that means graphics is important. of course they
could be running a server vm and have it for debugging, but that would
be silly.

This could hide an actual bug, by not aborting here. Not a major point.

I'm also not sure what's the right thing to do - turn on a flag for stopping
handling requests? it would just open up a whole set of states we don't handle
currently, half-working states.

> 
> Cheers,
> Jes
>