[Qemu-devel] [PULL] spice patch queue

[Qemu-devel] [PULL] spice patch queue

[Gerd Hoffmann]

  Hi,

Here is the spice patch queue with a collection of bugfixes.

A workaround for the much discussed spice-calls-us-from-wrong-thread
issue is included because it turned out to be not *that* easily fixable
in spice so it will probably take some time.  Also a spice server fix
wouldn't cover already released spice versions.

cheers,
  Gerd

The following changes since commit 344eecf6995f4a0ad1d887cec922f6806f91a3f8:

  mips: Support the MT TCStatus IXMT irq disable flag (2011-09-06 11:09:39 +0200)

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

Gerd Hoffmann (1):
      spice: workaround a spice server bug.

Peter Maydell (2):
      spice-qemu-char.c: Use correct printf format char for ssize_t
      hw/qxl: Fix format string errors

Yonit Halperin (3):
      qxl: send interrupt after migration in case ram->int_pending != 0, RHBZ #732949
      qxl: s/qxl_set_irq/qxl_update_irq/
      spice: set qxl->ssd.running=true before telling spice to start, RHBZ #733993

 hw/qxl-logger.c    |    2 +-
 hw/qxl.c           |   26 ++++++++++++++++----------
 spice-qemu-char.c  |    2 +-
 ui/spice-core.c    |   25 ++++++++++++++++++++++++-
 ui/spice-display.c |    3 ++-
 5 files changed, 44 insertions(+), 14 deletions(-)

[Qemu-devel] [PATCH 1/6] spice-qemu-char.c: Use correct printf format char for ssize_t

[Gerd Hoffmann]

From: Peter Maydell <peter.maydell@linaro.org>

Use the correct printf format string character (%z) for ssize_t.
This fixes a compile failure on 32 bit Linux with spice enabled.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 spice-qemu-char.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index a9323f3..ac52202 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -45,7 +45,7 @@ static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t *buf, int len)
         p += last_out;
     }
 
-    dprintf(scd, 3, "%s: %lu/%zd\n", __func__, out, len + out);
+    dprintf(scd, 3, "%s: %zu/%zd\n", __func__, out, len + out);
     trace_spice_vmc_write(out, len + out);
     return out;
 }
-- 
1.7.1

[Qemu-devel] [PATCH 2/6] hw/qxl: Fix format string errors

[Gerd Hoffmann]

From: Peter Maydell <peter.maydell@linaro.org>

Fix format string errors causing compile failure on 32 bit hosts
when spice is enabled.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qxl-logger.c |    2 +-
 hw/qxl.c        |    8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/qxl-logger.c b/hw/qxl-logger.c
index 74cadba..367aad1 100644
--- a/hw/qxl-logger.c
+++ b/hw/qxl-logger.c
@@ -224,7 +224,7 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext)
     if (!qxl->cmdlog) {
         return;
     }
-    fprintf(stderr, "%ld qxl-%d/%s:", qemu_get_clock_ns(vm_clock),
+    fprintf(stderr, "%" PRId64 " qxl-%d/%s:", qemu_get_clock_ns(vm_clock),
             qxl->id, ring);
     fprintf(stderr, " cmd @ 0x%" PRIx64 " %s%s", ext->cmd.data,
             qxl_name(qxl_type, ext->cmd.type),
diff --git a/hw/qxl.c b/hw/qxl.c
index 45e2401..1fe0b53 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -959,7 +959,7 @@ static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta,
     memslot.generation = d->rom->slot_generation = 0;
     qxl_rom_set_dirty(d);
 
-    dprint(d, 1, "%s: slot %d: host virt 0x%" PRIx64 " - 0x%" PRIx64 "\n",
+    dprint(d, 1, "%s: slot %d: host virt 0x%lx - 0x%lx\n",
            __FUNCTION__, memslot.slot_id,
            memslot.virt_start, memslot.virt_end);
 
@@ -1090,8 +1090,8 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm)
         .mem        = devmem + d->shadow_rom.draw_area_offset,
     };
 
-    dprint(d, 1, "%s: mode %d  [ %d x %d @ %d bpp devmem 0x%lx ]\n", __FUNCTION__,
-           modenr, mode->x_res, mode->y_res, mode->bits, devmem);
+    dprint(d, 1, "%s: mode %d  [ %d x %d @ %d bpp devmem 0x%" PRIx64 " ]\n",
+           __func__, modenr, mode->x_res, mode->y_res, mode->bits, devmem);
     if (!loadvm) {
         qxl_hard_reset(d, 0);
     }
@@ -1229,7 +1229,7 @@ async_common:
         break;
     case QXL_IO_LOG:
         if (d->guestdebug) {
-            fprintf(stderr, "qxl/guest-%d: %ld: %s", d->id,
+            fprintf(stderr, "qxl/guest-%d: %" PRId64 ": %s", d->id,
                     qemu_get_clock_ns(vm_clock), d->ram->log_buf);
         }
         break;
-- 
1.7.1

[Qemu-devel] [PATCH 3/6] qxl: send interrupt after migration in case ram->int_pending != 0, RHBZ #732949

[Gerd Hoffmann]

From: Yonit Halperin <yhalperi@redhat.com>

if qxl_send_events was called from spice server context, and then
migration had completed before a call to pipe_read, the target
guest qxl driver didn't get the interrupt. In addition,
qxl_send_events ignored further interrupts of the same kind, since
ram->int_pending was set. As a result, the guest driver was stacked
or very slow (when the waiting for the interrupt was with timeout).

Signed-off-by: Yonit Halperin <yhalperi@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qxl.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 1fe0b53..7bb2560 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1362,7 +1362,6 @@ static void pipe_read(void *opaque)
     qxl_set_irq(d);
 }
 
-/* called from spice server thread context only */
 static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
 {
     uint32_t old_pending;
@@ -1459,7 +1458,14 @@ static void qxl_vm_change_state_handler(void *opaque, int running, int reason)
     PCIQXLDevice *qxl = opaque;
     qemu_spice_vm_change_state_handler(&qxl->ssd, running, reason);
 
-    if (!running && qxl->mode == QXL_MODE_NATIVE) {
+    if (running) {
+        /*
+         * if qxl_send_events was called from spice server context before
+         * migration ended, qxl_set_irq for these events might not have been
+         * called
+         */
+         qxl_set_irq(qxl);
+    } else if (qxl->mode == QXL_MODE_NATIVE) {
         /* dirty all vram (which holds surfaces) and devram (primary surface)
          * to make sure they are saved */
         /* FIXME #1: should go out during "live" stage */
-- 
1.7.1

[Qemu-devel] [PATCH 4/6] qxl: s/qxl_set_irq/qxl_update_irq/

[Gerd Hoffmann]

From: Yonit Halperin <yhalperi@redhat.com>

Signed-off-by: Yonit Halperin <yhalperi@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qxl.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 7bb2560..a282d23 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -808,7 +808,7 @@ static void qxl_exit_vga_mode(PCIQXLDevice *d)
     qxl_destroy_primary(d, QXL_SYNC);
 }
 
-static void qxl_set_irq(PCIQXLDevice *d)
+static void qxl_update_irq(PCIQXLDevice *d)
 {
     uint32_t pending = le32_to_cpu(d->ram->int_pending);
     uint32_t mask    = le32_to_cpu(d->ram->int_mask);
@@ -1209,7 +1209,7 @@ async_common:
         qemu_spice_wakeup(&d->ssd);
         break;
     case QXL_IO_UPDATE_IRQ:
-        qxl_set_irq(d);
+        qxl_update_irq(d);
         break;
     case QXL_IO_NOTIFY_OOM:
         if (!SPICE_RING_IS_EMPTY(&d->ram->release_ring)) {
@@ -1359,7 +1359,7 @@ static void pipe_read(void *opaque)
     do {
         len = read(d->pipe[0], &dummy, sizeof(dummy));
     } while (len == sizeof(dummy));
-    qxl_set_irq(d);
+    qxl_update_irq(d);
 }
 
 static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
@@ -1373,7 +1373,7 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
         return;
     }
     if (pthread_self() == d->main) {
-        qxl_set_irq(d);
+        qxl_update_irq(d);
     } else {
         if (write(d->pipe[1], d, 1) != 1) {
             dprint(d, 1, "%s: write to pipe failed\n", __FUNCTION__);
@@ -1461,10 +1461,10 @@ static void qxl_vm_change_state_handler(void *opaque, int running, int reason)
     if (running) {
         /*
          * if qxl_send_events was called from spice server context before
-         * migration ended, qxl_set_irq for these events might not have been
+         * migration ended, qxl_update_irq for these events might not have been
          * called
          */
-         qxl_set_irq(qxl);
+         qxl_update_irq(qxl);
     } else if (qxl->mode == QXL_MODE_NATIVE) {
         /* dirty all vram (which holds surfaces) and devram (primary surface)
          * to make sure they are saved */
-- 
1.7.1

[Qemu-devel] [PATCH 5/6] spice: set qxl->ssd.running=true before telling spice to start, RHBZ #733993

[Gerd Hoffmann]

From: Yonit Halperin <yhalperi@redhat.com>

If qxl->ssd.running=true is set after telling spice to start, the spice server
thread can call qxl_send_events while qxl->ssd.running is still false. This leads to
assert(d->ssd.running).

Signed-off-by: Yonit Halperin <yhalperi@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/spice-display.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 4983963..e385361 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -260,11 +260,12 @@ void qemu_spice_vm_change_state_handler(void *opaque, int running, int reason)
     SimpleSpiceDisplay *ssd = opaque;
 
     if (running) {
+        ssd->running = true;
         qemu_spice_start(ssd);
     } else {
         qemu_spice_stop(ssd);
+        ssd->running = false;
     }
-    ssd->running = running;
 }
 
 void qemu_spice_display_init_common(SimpleSpiceDisplay *ssd, DisplayState *ds)
-- 
1.7.1

[Qemu-devel] [PATCH 6/6] spice: workaround a spice server bug.

[Gerd Hoffmann]

spice server might call the channel_event callback from spice server
thread context.  Detect that and aquire iothread lock if needed,
---
 ui/spice-core.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index dba11f0..3cbc721 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -19,6 +19,7 @@
 #include <spice-experimental.h>
 
 #include <netdb.h>
+#include <pthread.h>
 
 #include "qemu-common.h"
 #include "qemu-spice.h"
@@ -44,6 +45,8 @@ static char *auth_passwd;
 static time_t auth_expires = TIME_MAX;
 int using_spice = 0;
 
+static pthread_t me;
+
 struct SpiceTimer {
     QEMUTimer *timer;
     QTAILQ_ENTRY(SpiceTimer) next;
@@ -217,6 +220,20 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
     QDict *server, *client;
     QObject *data;
 
+    /*
+     * Spice server might have called us from spice worker thread
+     * context (happens on display channel disconnects).  Spice should
+     * not do that.  It isn't that easy to fix it in spice and even
+     * when it is fixed we still should cover the already released
+     * spice versions.  So detect that we've been called from another
+     * thread and grab the iothread lock if so before calling qemu
+     * functions.
+     */
+    bool need_lock = !pthread_equal(me, pthread_self());
+    if (need_lock) {
+        qemu_mutex_lock_iothread();
+    }
+
     client = qdict_new();
     add_addr_info(client, &info->paddr, info->plen);
 
@@ -236,6 +253,10 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
                               QOBJECT(client), QOBJECT(server));
     monitor_protocol_event(qevent[event], data);
     qobject_decref(data);
+
+    if (need_lock) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 #else /* SPICE_INTERFACE_CORE_MINOR >= 3 */
@@ -482,7 +503,9 @@ void qemu_spice_init(void)
     spice_image_compression_t compression;
     spice_wan_compression_t wan_compr;
 
-    if (!opts) {
+    me = pthread_self();
+
+   if (!opts) {
         return;
     }
     port = qemu_opt_get_number(opts, "port", 0);
-- 
1.7.1

Re: [Qemu-devel] [PULL] spice patch queue

[Anthony Liguori]

On 09/07/2011 02:38 AM, Gerd Hoffmann wrote:
>    Hi,
>
> Here is the spice patch queue with a collection of bugfixes.
>
> A workaround for the much discussed spice-calls-us-from-wrong-thread
> issue is included because it turned out to be not *that* easily fixable
> in spice so it will probably take some time.  Also a spice server fix
> wouldn't cover already released spice versions.
>
> cheers,
>    Gerd


Pulled.  Thanks.

Regards,

Anthony Liguori

>
> The following changes since commit 344eecf6995f4a0ad1d887cec922f6806f91a3f8:
>
>    mips: Support the MT TCStatus IXMT irq disable flag (2011-09-06 11:09:39 +0200)
>
> are available in the git repository at:
>    git://anongit.freedesktop.org/spice/qemu spice.v42
>
> Gerd Hoffmann (1):
>        spice: workaround a spice server bug.
>
> Peter Maydell (2):
>        spice-qemu-char.c: Use correct printf format char for ssize_t
>        hw/qxl: Fix format string errors
>
> Yonit Halperin (3):
>        qxl: send interrupt after migration in case ram->int_pending != 0, RHBZ #732949
>        qxl: s/qxl_set_irq/qxl_update_irq/
>        spice: set qxl->ssd.running=true before telling spice to start, RHBZ #733993
>
>   hw/qxl-logger.c    |    2 +-
>   hw/qxl.c           |   26 ++++++++++++++++----------
>   spice-qemu-char.c  |    2 +-
>   ui/spice-core.c    |   25 ++++++++++++++++++++++++-
>   ui/spice-display.c |    3 ++-
>   5 files changed, 44 insertions(+), 14 deletions(-)
>
>