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

[Alon Levy]

From: Uri Lublin <address@hidden>

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