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

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

[Alon Levy]

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           |  246 ++++++++++++++++++++++++++++++++++++++++++++--------
 hw/qxl.h           |    8 +-
 ui/spice-display.c |   20 +----
 ui/spice-display.h |   16 ++++
 5 files changed, 251 insertions(+), 64 deletions(-)

-- 
1.7.4.1

[Qemu-devel] [PATCH v2 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 |    7 +++++++
 4 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index fe4212b..f1ee0ae 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_server_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_server_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..d60e7a9 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_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 aef0464..de584d9 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,5 @@ 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);
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 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           |  114 ++++++++++++++++++++++++++++++++++++++++++++--------
 ui/spice-display.c |    8 +---
 ui/spice-display.h |    9 ++++
 3 files changed, 107 insertions(+), 24 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index f1ee0ae..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,9 +1160,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 d60e7a9..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)
diff --git a/ui/spice-display.h b/ui/spice-display.h
index de584d9..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 {
@@ -71,5 +77,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_server_to_iothread_pipe(SimpleSpiceDisplay *ssd);
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 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 v2 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        |   96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qxl.h        |    4 ++
 3 files changed, 116 insertions(+), 9 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 72f204b..8a398e2 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -122,12 +122,39 @@ static QXLMode qxl_modes[] = {
 
 /*
  * commands/requests from server thread to iothread.
+ * 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
  */
 enum {
     QXL_SERVER_SET_IRQ = 1,
     QXL_SERVER_CREATE_UPDATE,
+    QXL_SERVER_CURSOR_SET,
+    QXL_SERVER_CURSOR_MOVE
 };
 
+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);
@@ -347,6 +374,7 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
     info->n_surfaces = NUM_SURFACES;
 }
 
+/* called from spice server thread context only */
 int qxl_vga_mode_get_command(
     SimpleSpiceDisplay *ssd, struct QXLCommandExt *ext)
 {
@@ -374,6 +402,33 @@ int qxl_vga_mode_get_command(
 }
 
 /* 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);
@@ -1092,12 +1147,36 @@ 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;
+    } 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;
 
     do {
         cmd = 0;
@@ -1119,6 +1198,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();
@@ -1136,6 +1226,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);
-- 
1.7.4.1

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

[Alon Levy]

On Wed, Mar 16, 2011 at 02:48:30PM +0200, Alon Levy wrote:
> 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)
> 

NACK self, this is broken for non x86 builds, Hans is working on a fix and
I'll send a v3 with it once it's ready.

Alon

> 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           |  246 ++++++++++++++++++++++++++++++++++++++++++++--------
>  hw/qxl.h           |    8 +-
>  ui/spice-display.c |   20 +----
>  ui/spice-display.h |   16 ++++
>  5 files changed, 251 insertions(+), 64 deletions(-)
> 
> -- 
> 1.7.4.1
> 
>