* [Qemu-devel] [PATCH 1/4] Make spice dummy functions inline to fix calls not checking return values
2011-05-03 15:06 [Qemu-devel] [PULL] spice: fix locking Gerd Hoffmann
@ 2011-05-03 15:06 ` Gerd Hoffmann
2011-05-03 15:06 ` [Qemu-devel] [PATCH 2/4] spice: don't create updates in spice server context Gerd Hoffmann
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Gerd Hoffmann @ 2011-05-03 15:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Jes Sorensen, Gerd Hoffmann
From: Jes Sorensen <Jes.Sorensen@redhat.com>
qemu_spice_set_passwd() and qemu_spice_set_pw_expire() dummy functions
needs to be inline, in order to handle the case where they are called
without checking the return value.
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
ui/qemu-spice.h | 12 ++++++++++--
1 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
index 916e5dc..3c6f1fe 100644
--- a/ui/qemu-spice.h
+++ b/ui/qemu-spice.h
@@ -47,8 +47,16 @@ CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
#else /* CONFIG_SPICE */
#define using_spice 0
-#define qemu_spice_set_passwd(_p, _f1, _f2) (-1)
-#define qemu_spice_set_pw_expire(_e) (-1)
+static inline int qemu_spice_set_passwd(const char *passwd,
+ bool fail_if_connected,
+ bool disconnect_if_connected)
+{
+ return -1;
+}
+static inline int qemu_spice_set_pw_expire(time_t expires)
+{
+ return -1;
+}
static inline int qemu_spice_migrate_info(const char *h, int p, int t, const char *s)
{ return -1; }
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH 2/4] spice: don't create updates in spice server context.
2011-05-03 15:06 [Qemu-devel] [PULL] spice: fix locking Gerd Hoffmann
2011-05-03 15:06 ` [Qemu-devel] [PATCH 1/4] Make spice dummy functions inline to fix calls not checking return values Gerd Hoffmann
@ 2011-05-03 15:06 ` Gerd Hoffmann
2011-05-03 15:06 ` [Qemu-devel] [PATCH 3/4] spice: don't call displaystate callbacks from " Gerd Hoffmann
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Gerd Hoffmann @ 2011-05-03 15:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
This patch moves the creation of spice screen updates from the spice
server context to qemu iothread context (display refresh timer to be
exact). This way we avoid accessing qemu internals (display surface)
from spice thread context which in turn allows us to simplify locking.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/qxl.c | 17 +++++++++++------
ui/spice-display.c | 43 +++++++++++++++++++++++++++----------------
ui/spice-display.h | 21 ++++++++++++++++-----
3 files changed, 54 insertions(+), 27 deletions(-)
diff --git a/hw/qxl.c b/hw/qxl.c
index fe4212b..bd250db 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -343,18 +343,22 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
SimpleSpiceUpdate *update;
QXLCommandRing *ring;
QXLCommand *cmd;
- int notify;
+ int notify, ret;
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;
+ ret = false;
+ qemu_mutex_lock(&qxl->ssd.lock);
+ if (qxl->ssd.update != NULL) {
+ update = qxl->ssd.update;
+ qxl->ssd.update = NULL;
+ *ext = update->ext;
+ ret = true;
}
- *ext = update->ext;
+ qemu_mutex_unlock(&qxl->ssd.lock);
qxl_log_command(qxl, "vga", ext);
- return true;
+ return ret;
case QXL_MODE_COMPAT:
case QXL_MODE_NATIVE:
case QXL_MODE_UNDEFINED:
@@ -1304,6 +1308,7 @@ static int qxl_init_primary(PCIDevice *dev)
vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
qxl_hw_screen_dump, qxl_hw_text_update, qxl);
qxl->ssd.ds = vga->ds;
+ qemu_mutex_init(&qxl->ssd.lock);
qxl->ssd.bufsize = (16 * 1024 * 1024);
qxl->ssd.buf = qemu_malloc(qxl->ssd.bufsize);
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 020b423..d56dcfc 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -62,14 +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.
- */
-SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
+static SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
{
SimpleSpiceUpdate *update;
QXLDrawable *drawable;
@@ -78,9 +71,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 +132,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;
}
@@ -241,6 +231,12 @@ void qemu_spice_display_resize(SimpleSpiceDisplay *ssd)
qemu_pf_conv_put(ssd->conv);
ssd->conv = NULL;
+ qemu_mutex_lock(&ssd->lock);
+ if (ssd->update != NULL) {
+ qemu_spice_destroy_update(ssd, ssd->update);
+ ssd->update = NULL;
+ }
+ qemu_mutex_unlock(&ssd->lock);
qemu_spice_destroy_host_primary(ssd);
qemu_spice_create_host_primary(ssd);
@@ -252,6 +248,14 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
{
dprint(3, "%s:\n", __FUNCTION__);
vga_hw_update();
+
+ qemu_mutex_lock(&ssd->lock);
+ if (ssd->update == NULL) {
+ ssd->update = qemu_spice_create_update(ssd);
+ ssd->notify++;
+ }
+ qemu_mutex_unlock(&ssd->lock);
+
if (ssd->notify) {
ssd->notify = 0;
ssd->worker->wakeup(ssd->worker);
@@ -298,14 +302,20 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
{
SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
SimpleSpiceUpdate *update;
+ int ret = false;
dprint(3, "%s:\n", __FUNCTION__);
- update = qemu_spice_create_update(ssd);
- if (update == NULL) {
- return false;
+
+ qemu_mutex_lock(&ssd->lock);
+ if (ssd->update != NULL) {
+ update = ssd->update;
+ ssd->update = NULL;
+ *ext = update->ext;
+ ret = true;
}
- *ext = update->ext;
- return true;
+ qemu_mutex_unlock(&ssd->lock);
+
+ return ret;
}
static int interface_req_cmd_notification(QXLInstance *sin)
@@ -398,6 +408,7 @@ void qemu_spice_display_init(DisplayState *ds)
{
assert(sdpy.ds == NULL);
sdpy.ds = ds;
+ qemu_mutex_init(&sdpy.lock);
sdpy.bufsize = (16 * 1024 * 1024);
sdpy.buf = qemu_malloc(sdpy.bufsize);
register_displaychangelistener(ds, &display_listener);
diff --git a/ui/spice-display.h b/ui/spice-display.h
index aef0464..e0cc46e 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -19,6 +19,7 @@
#include <spice/enums.h>
#include <spice/qxl_dev.h>
+#include "qemu-thread.h"
#include "pflib.h"
#define NUM_MEMSLOTS 8
@@ -31,7 +32,10 @@
#define NUM_SURFACES 1024
-typedef struct SimpleSpiceDisplay {
+typedef struct SimpleSpiceDisplay SimpleSpiceDisplay;
+typedef struct SimpleSpiceUpdate SimpleSpiceUpdate;
+
+struct SimpleSpiceDisplay {
DisplayState *ds;
void *buf;
int bufsize;
@@ -43,19 +47,26 @@ typedef struct SimpleSpiceDisplay {
QXLRect dirty;
int notify;
int running;
-} SimpleSpiceDisplay;
-typedef struct SimpleSpiceUpdate {
+ /*
+ * All struct members below this comment can be accessed from
+ * both spice server and qemu (iothread) context and any access
+ * to them must be protected by the lock.
+ */
+ QemuMutex lock;
+ SimpleSpiceUpdate *update;
+};
+
+struct SimpleSpiceUpdate {
QXLDrawable drawable;
QXLImage image;
QXLCommandExt ext;
uint8_t *bitmap;
-} SimpleSpiceUpdate;
+};
int qemu_spice_rect_is_empty(const QXLRect* r);
void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
-SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *sdpy);
void qemu_spice_destroy_update(SimpleSpiceDisplay *sdpy, SimpleSpiceUpdate *update);
void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd);
void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd);
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH 3/4] spice: don't call displaystate callbacks from spice server context.
2011-05-03 15:06 [Qemu-devel] [PULL] spice: fix locking Gerd Hoffmann
2011-05-03 15:06 ` [Qemu-devel] [PATCH 1/4] Make spice dummy functions inline to fix calls not checking return values Gerd Hoffmann
2011-05-03 15:06 ` [Qemu-devel] [PATCH 2/4] spice: don't create updates in spice server context Gerd Hoffmann
@ 2011-05-03 15:06 ` Gerd Hoffmann
2011-05-03 15:06 ` [Qemu-devel] [PATCH 4/4] spice: drop obsolete iothread locking Gerd Hoffmann
2011-05-05 18:25 ` [Qemu-devel] [PULL] spice: fix locking Anthony Liguori
4 siblings, 0 replies; 6+ messages in thread
From: Gerd Hoffmann @ 2011-05-03 15:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
This patch moves the displaystate callback calls for setting the cursor
and the mouse pointer from spice server to qemu (iothread) context.
This allows us to simplify locking.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/qxl-render.c | 25 ++++++++++++-------------
hw/qxl.c | 2 ++
ui/spice-display.c | 12 ++++++++++++
ui/spice-display.h | 3 +++
4 files changed, 29 insertions(+), 13 deletions(-)
diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 58965e0..1316066 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -185,7 +185,6 @@ void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
QXLCursor *cursor;
QEMUCursor *c;
- int x = -1, y = -1;
if (!qxl->ssd.ds->mouse_set || !qxl->ssd.ds->cursor_define) {
return;
@@ -198,8 +197,6 @@ void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
}
switch (cmd->type) {
case QXL_CURSOR_SET:
- x = cmd->u.set.position.x;
- y = cmd->u.set.position.y;
cursor = qxl_phys2virt(qxl, cmd->u.set.shape, ext->group_id);
if (cursor->chunk.data_size != cursor->data_size) {
fprintf(stderr, "%s: multiple chunks\n", __FUNCTION__);
@@ -209,18 +206,20 @@ 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);
+ qemu_mutex_lock(&qxl->ssd.lock);
+ if (qxl->ssd.cursor) {
+ cursor_put(qxl->ssd.cursor);
+ }
+ qxl->ssd.cursor = c;
+ qxl->ssd.mouse_x = cmd->u.set.position.x;
+ qxl->ssd.mouse_y = cmd->u.set.position.y;
+ qemu_mutex_unlock(&qxl->ssd.lock);
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();
+ qemu_mutex_lock(&qxl->ssd.lock);
+ qxl->ssd.mouse_x = cmd->u.position.x;
+ qxl->ssd.mouse_y = cmd->u.position.y;
+ qemu_mutex_unlock(&qxl->ssd.lock);
break;
}
}
diff --git a/hw/qxl.c b/hw/qxl.c
index bd250db..4dfddf0 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1309,6 +1309,8 @@ static int qxl_init_primary(PCIDevice *dev)
qxl_hw_screen_dump, qxl_hw_text_update, qxl);
qxl->ssd.ds = vga->ds;
qemu_mutex_init(&qxl->ssd.lock);
+ qxl->ssd.mouse_x = -1;
+ qxl->ssd.mouse_y = -1;
qxl->ssd.bufsize = (16 * 1024 * 1024);
qxl->ssd.buf = qemu_malloc(qxl->ssd.bufsize);
diff --git a/ui/spice-display.c b/ui/spice-display.c
index d56dcfc..8579bfd 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -254,6 +254,16 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
ssd->update = qemu_spice_create_update(ssd);
ssd->notify++;
}
+ if (ssd->cursor) {
+ ssd->ds->cursor_define(ssd->cursor);
+ cursor_put(ssd->cursor);
+ ssd->cursor = NULL;
+ }
+ if (ssd->mouse_x != -1 && ssd->mouse_y != -1) {
+ ssd->ds->mouse_set(ssd->mouse_x, ssd->mouse_y, 1);
+ ssd->mouse_x = -1;
+ ssd->mouse_y = -1;
+ }
qemu_mutex_unlock(&ssd->lock);
if (ssd->notify) {
@@ -409,6 +419,8 @@ void qemu_spice_display_init(DisplayState *ds)
assert(sdpy.ds == NULL);
sdpy.ds = ds;
qemu_mutex_init(&sdpy.lock);
+ sdpy.mouse_x = -1;
+ sdpy.mouse_y = -1;
sdpy.bufsize = (16 * 1024 * 1024);
sdpy.buf = qemu_malloc(sdpy.bufsize);
register_displaychangelistener(ds, &display_listener);
diff --git a/ui/spice-display.h b/ui/spice-display.h
index e0cc46e..2f95f68 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -20,6 +20,7 @@
#include <spice/qxl_dev.h>
#include "qemu-thread.h"
+#include "console.h"
#include "pflib.h"
#define NUM_MEMSLOTS 8
@@ -55,6 +56,8 @@ struct SimpleSpiceDisplay {
*/
QemuMutex lock;
SimpleSpiceUpdate *update;
+ QEMUCursor *cursor;
+ int mouse_x, mouse_y;
};
struct SimpleSpiceUpdate {
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH 4/4] spice: drop obsolete iothread locking
2011-05-03 15:06 [Qemu-devel] [PULL] spice: fix locking Gerd Hoffmann
` (2 preceding siblings ...)
2011-05-03 15:06 ` [Qemu-devel] [PATCH 3/4] spice: don't call displaystate callbacks from " Gerd Hoffmann
@ 2011-05-03 15:06 ` Gerd Hoffmann
2011-05-05 18:25 ` [Qemu-devel] [PULL] spice: fix locking Anthony Liguori
4 siblings, 0 replies; 6+ messages in thread
From: Gerd Hoffmann @ 2011-05-03 15:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
We don't use qemu internals from spice server context any more.
Thus we don't also need to grab the iothread mutex from spice
server context. And we don't have to temporarely release the
lock to avoid deadlocks. Drop all the calls.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/qxl.c | 8 --------
ui/spice-display.c | 6 ------
2 files changed, 0 insertions(+), 14 deletions(-)
diff --git a/hw/qxl.c b/hw/qxl.c
index 4dfddf0..2bb36c6 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -666,10 +666,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);
@@ -799,9 +797,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));
}
@@ -870,9 +866,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)
@@ -942,10 +936,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 8579bfd..15f0704 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -176,18 +176,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)
@@ -197,9 +193,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;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PULL] spice: fix locking
2011-05-03 15:06 [Qemu-devel] [PULL] spice: fix locking Gerd Hoffmann
` (3 preceding siblings ...)
2011-05-03 15:06 ` [Qemu-devel] [PATCH 4/4] spice: drop obsolete iothread locking Gerd Hoffmann
@ 2011-05-05 18:25 ` Anthony Liguori
4 siblings, 0 replies; 6+ messages in thread
From: Anthony Liguori @ 2011-05-05 18:25 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: qemu-devel
On 05/03/2011 10:06 AM, Gerd Hoffmann wrote:
> Hi,
>
> This is the current spice patch queue ready for pull. Patches have been
> posted a few days ago for review. A minor issue (leftover debug bit,
> spotten by Alon) has been fixed and the patch queue has been rebased to
> latest master, otherwise it is unmodified.
>
> Please pull.
Pulled. Thanks.
Regards,
Anthony Liguori
> cheers,
> Gerd
>
> The following changes since commit d2d979c628e4b2c4a3cb71a31841875795c79043:
>
> NBD: Avoid leaking a couple of strings when the NBD device is closed (2011-05-03 11:29:21 +0200)
>
> are available in the git repository at:
> git://anongit.freedesktop.org/spice/qemu spice.v35
>
> Gerd Hoffmann (3):
> spice: don't create updates in spice server context.
> spice: don't call displaystate callbacks from spice server context.
> spice: drop obsolete iothread locking
>
> Jes Sorensen (1):
> Make spice dummy functions inline to fix calls not checking return values
>
> hw/qxl-render.c | 25 ++++++++++-----------
> hw/qxl.c | 27 +++++++++++------------
> ui/qemu-spice.h | 12 ++++++++-
> ui/spice-display.c | 61 +++++++++++++++++++++++++++++++++------------------
> ui/spice-display.h | 24 ++++++++++++++++----
> 5 files changed, 93 insertions(+), 56 deletions(-)
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread