[Qemu-devel] [PATCH 0/5] monitor+qxl: async monitor support

[Qemu-devel] [PATCH 0/5] monitor+qxl: async monitor support

[Alon Levy]

This patchset converts the screen_dump command to async across qemu, and then
implements it asynchronously for the qxl device in Native mode. This fixes a
hang that is caused when the spice client is also a qemu monitor client and the
client is single threaded:


  io thread                   worker thread                client

   <---------------------------------------------------- screendump
  do_screen_dump-> read------->
                               flush, read
                               client socket------------->

Monitor maintainers: The first patch changes the monitor command to be async.
The rest are qxl changes only.

Alon Levy (5):
  monitor: screen_dump async
  qxl: s/__FUNCTION__/__func__/, change logging levels
  qxl: support concurrent async commands
  qxl: split qxl_render_display_resized
  qxl: support async monitor screen dump

 console.c          |    5 +-
 console.h          |    7 +-
 hmp-commands.hx    |    3 +-
 hw/g364fb.c        |   12 ++-
 hw/qxl-render.c    |  136 ++++++++++++++++++++----------
 hw/qxl.c           |  236 +++++++++++++++++++++++++++++++++++++--------------
 hw/qxl.h           |   63 +++++++++++++-
 hw/vga.c           |    7 +-
 hw/vmware_vga.c    |    5 +-
 monitor.c          |    5 +-
 qmp-commands.hx    |    3 +-
 ui/spice-display.c |   19 ++--
 ui/spice-display.h |    7 +-
 13 files changed, 363 insertions(+), 145 deletions(-)

-- 
1.7.7

[Qemu-devel] [PATCH 1/5] monitor: screen_dump async

[Alon Levy]

Make screen_dump monitor command an async command to allow next for qxl
to implement it as a initiating call to red_worker and completion on
callback, to fix a deadlock when issueing a screendump command via
libvirt while connected with a libvirt controlled spice-gtk client.

This patch introduces no functional changes.
---
 console.c       |    5 +++--
 console.h       |    7 +++++--
 hmp-commands.hx |    3 ++-
 hw/g364fb.c     |   12 ++++++++----
 hw/qxl.c        |    6 ++++--
 hw/vga.c        |    7 +++++--
 hw/vmware_vga.c |    5 +++--
 monitor.c       |    5 +++--
 qmp-commands.hx |    3 ++-
 9 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/console.c b/console.c
index e43de92..30c667a 100644
--- a/console.c
+++ b/console.c
@@ -173,7 +173,8 @@ void vga_hw_invalidate(void)
         active_console->hw_invalidate(active_console->hw);
 }
 
-void vga_hw_screen_dump(const char *filename)
+void vga_hw_screen_dump(const char *filename, MonitorCompletion cb,
+                        void *opaque)
 {
     TextConsole *previous_active_console;
 
@@ -183,7 +184,7 @@ void vga_hw_screen_dump(const char *filename)
        so always dump the first one.  */
     console_select(0);
     if (consoles[0] && consoles[0]->hw_screen_dump) {
-        consoles[0]->hw_screen_dump(consoles[0]->hw, filename);
+        consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cb, opaque);
     }
 
     console_select(previous_active_console->index);
diff --git a/console.h b/console.h
index 9c1487e..dde4088 100644
--- a/console.h
+++ b/console.h
@@ -343,7 +343,9 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
 
 typedef void (*vga_hw_update_ptr)(void *);
 typedef void (*vga_hw_invalidate_ptr)(void *);
-typedef void (*vga_hw_screen_dump_ptr)(void *, const char *);
+typedef void (*vga_hw_screen_dump_ptr)(void *, const char *,
+                                       MonitorCompletion *,
+                                       void *);
 typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
 
 DisplayState *graphic_console_init(vga_hw_update_ptr update,
@@ -354,7 +356,8 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
 
 void vga_hw_update(void);
 void vga_hw_invalidate(void);
-void vga_hw_screen_dump(const char *filename);
+void vga_hw_screen_dump(const char *filename, MonitorCompletion cb,
+                        void *opaque);
 void vga_hw_text_update(console_ch_t *chardata);
 
 int is_graphic_console(void);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index ab08d58..a7b3494 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -157,7 +157,8 @@ ETEXI
         .params     = "filename",
         .help       = "save screen into PPM image 'filename'",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_screen_dump,
+        .mhandler.cmd_async = do_screen_dump,
+        .flags      = MONITOR_CMD_ASYNC,
     },
 
 STEXI
diff --git a/hw/g364fb.c b/hw/g364fb.c
index f00ee27..5884e20 100644
--- a/hw/g364fb.c
+++ b/hw/g364fb.c
@@ -291,7 +291,8 @@ static void g364fb_reset(G364State *s)
     g364fb_invalidate_display(s);
 }
 
-static void g364fb_screen_dump(void *opaque, const char *filename)
+static void g364fb_screen_dump(void *opaque, const char *filename,
+                               MonitorCompletion *cb, void *cb_opaque)
 {
     G364State *s = opaque;
     int y, x;
@@ -303,12 +304,13 @@ static void g364fb_screen_dump(void *opaque, const char *filename)
 
     if (s->depth != 8) {
         error_report("g364: unknown guest depth %d", s->depth);
-        return;
+        goto end;
     }
 
     f = fopen(filename, "wb");
-    if (!f)
-        return;
+    if (!f) {
+        goto end;
+    }
 
     if (s->ctla & CTLA_FORCE_BLANK) {
         /* blank screen */
@@ -331,6 +333,8 @@ static void g364fb_screen_dump(void *opaque, const char *filename)
     }
 
     fclose(f);
+end:
+    cb(cb_opaque, NULL);
 }
 
 /* called for accesses to io ports */
diff --git a/hw/qxl.c b/hw/qxl.c
index 03848ed..4003e53 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1423,7 +1423,8 @@ static void qxl_hw_invalidate(void *opaque)
     vga->invalidate(vga);
 }
 
-static void qxl_hw_screen_dump(void *opaque, const char *filename)
+static void qxl_hw_screen_dump(void *opaque, const char *filename,
+                               MonitorCompletion *cb, void *cb_opaque)
 {
     PCIQXLDevice *qxl = opaque;
     VGACommonState *vga = &qxl->vga;
@@ -1433,9 +1434,10 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename)
     case QXL_MODE_NATIVE:
         qxl_render_update(qxl);
         ppm_save(filename, qxl->ssd.ds->surface);
+        cb(cb_opaque, NULL);
         break;
     case QXL_MODE_VGA:
-        vga->screen_dump(vga, filename);
+        vga->screen_dump(vga, filename, cb, cb_opaque);
         break;
     default:
         break;
diff --git a/hw/vga.c b/hw/vga.c
index ca79aa1..b257f74 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -148,7 +148,8 @@ static uint32_t expand4[256];
 static uint16_t expand2[256];
 static uint8_t expand4to8[16];
 
-static void vga_screen_dump(void *opaque, const char *filename);
+static void vga_screen_dump(void *opaque, const char *filename,
+                            MonitorCompletion *cb, void *cb_opaque);
 static const char *screen_dump_filename;
 static DisplayChangeListener *screen_dump_dcl;
 
@@ -2404,7 +2405,8 @@ static DisplayChangeListener* vga_screen_dump_init(DisplayState *ds)
 
 /* save the vga display in a PPM image even if no display is
    available */
-static void vga_screen_dump(void *opaque, const char *filename)
+static void vga_screen_dump(void *opaque, const char *filename,
+                            MonitorCompletion *cb, void *cb_opaque)
 {
     VGACommonState *s = opaque;
 
@@ -2415,4 +2417,5 @@ static void vga_screen_dump(void *opaque, const char *filename)
     vga_invalidate_display(s);
     vga_hw_update();
     screen_dump_filename = NULL;
+    cb(cb_opaque, NULL);
 }
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index af70bde..a238fef 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1003,11 +1003,12 @@ static void vmsvga_invalidate_display(void *opaque)
 
 /* save the vga display in a PPM image even if no display is
    available */
-static void vmsvga_screen_dump(void *opaque, const char *filename)
+static void vmsvga_screen_dump(void *opaque, const char *filename,
+                               MonitorCompletion *cb, void *cb_opaque)
 {
     struct vmsvga_state_s *s = opaque;
     if (!s->enable) {
-        s->vga.screen_dump(&s->vga, filename);
+        s->vga.screen_dump(&s->vga, filename, cb, cb_opaque);
         return;
     }
 
diff --git a/monitor.c b/monitor.c
index ffda0fe..daf5e2f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1180,9 +1180,10 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict, QObject **ret_d
     return -1;
 }
 
-static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
+static int do_screen_dump(Monitor *mon, const QDict *params,
+                          MonitorCompletion cb, void *opaque)
 {
-    vga_hw_screen_dump(qdict_get_str(qdict, "filename"));
+    vga_hw_screen_dump(qdict_get_str(params, "filename"), cb, opaque);
     return 0;
 }
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9c11e87..15b04d6 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -155,7 +155,8 @@ EQMP
         .params     = "filename",
         .help       = "save screen into PPM image 'filename'",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_screen_dump,
+        .mhandler.cmd_async = do_screen_dump,
+        .flags      = MONITOR_CMD_ASYNC,
     },
 
 SQMP
-- 
1.7.7

Re: [Qemu-devel] [PATCH 1/5] monitor: screen_dump async

[Gerd Hoffmann]

On 10/24/11 14:02, Alon Levy wrote:
> Make screen_dump monitor command an async command to allow next for qxl
> to implement it as a initiating call to red_worker and completion on
> callback, to fix a deadlock when issueing a screendump command via
> libvirt while connected with a libvirt controlled spice-gtk client.

Approach looks reasonable to me.  Patch breaks the build though, you've
missed a bunch of screen_dump functions in non-x86 targets.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/5] monitor: screen_dump async

[Luiz Capitulino]

On Mon, 24 Oct 2011 17:13:14 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On 10/24/11 14:02, Alon Levy wrote:
> > Make screen_dump monitor command an async command to allow next for qxl
> > to implement it as a initiating call to red_worker and completion on
> > callback, to fix a deadlock when issueing a screendump command via
> > libvirt while connected with a libvirt controlled spice-gtk client.
> 
> Approach looks reasonable to me.  Patch breaks the build though, you've
> missed a bunch of screen_dump functions in non-x86 targets.

There are two problems actually.

The first one is that changing an existing command from synchronous
to asynchronous is an incompatible change because asynchronous commands
semantics is different. For an example of possible problems please
check: https://bugzilla.redhat.com/show_bug.cgi?id=623903.

The second problem is that the existing asynchronous interface in the
monitor is incomplete and has never been used for real. Our plan is to
use QAPI's async support, but that has not landed in master yet and iirc
there wasn't consensus about it. I also think it's a bit late for its
inclusion in 1.0 (and certainly not a candidate for stable).

If all you need here is to delay sending the response, then maybe the
current interface could work (although I honestly don't trust it and
regret not having dropped it). Otherwise our only choice would be to
work on getting the QAPI async support merged.

Re: [Qemu-devel] [PATCH 1/5] monitor: screen_dump async

[Alon Levy]

On Mon, Oct 24, 2011 at 01:45:16PM -0200, Luiz Capitulino wrote:
> On Mon, 24 Oct 2011 17:13:14 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> > On 10/24/11 14:02, Alon Levy wrote:
> > > Make screen_dump monitor command an async command to allow next for qxl
> > > to implement it as a initiating call to red_worker and completion on
> > > callback, to fix a deadlock when issueing a screendump command via
> > > libvirt while connected with a libvirt controlled spice-gtk client.
> > 
> > Approach looks reasonable to me.  Patch breaks the build though, you've
> > missed a bunch of screen_dump functions in non-x86 targets.
> 
> There are two problems actually.
> 
> The first one is that changing an existing command from synchronous
> to asynchronous is an incompatible change because asynchronous commands
> semantics is different. For an example of possible problems please
> check: https://bugzilla.redhat.com/show_bug.cgi?id=623903.
> 
> The second problem is that the existing asynchronous interface in the
> monitor is incomplete and has never been used for real. Our plan is to
> use QAPI's async support, but that has not landed in master yet and iirc
> there wasn't consensus about it. I also think it's a bit late for its
> inclusion in 1.0 (and certainly not a candidate for stable).
> 
> If all you need here is to delay sending the response, then maybe the
> current interface could work (although I honestly don't trust it and
> regret not having dropped it). Otherwise our only choice would be to
> work on getting the QAPI async support merged.

My problem is that the io thread keeps the global mutex during the wait,
that's why the async monitor is perfect for what I want - this is
exactly what it does. I haven't looked at QAPI async support, but I
understand it's a bit in the future.

Re: [Qemu-devel] [PATCH 1/5] monitor: screen_dump async

[Luiz Capitulino]

On Mon, 24 Oct 2011 19:29:37 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Mon, Oct 24, 2011 at 01:45:16PM -0200, Luiz Capitulino wrote:
> > On Mon, 24 Oct 2011 17:13:14 +0200
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > 
> > > On 10/24/11 14:02, Alon Levy wrote:
> > > > Make screen_dump monitor command an async command to allow next for qxl
> > > > to implement it as a initiating call to red_worker and completion on
> > > > callback, to fix a deadlock when issueing a screendump command via
> > > > libvirt while connected with a libvirt controlled spice-gtk client.
> > > 
> > > Approach looks reasonable to me.  Patch breaks the build though, you've
> > > missed a bunch of screen_dump functions in non-x86 targets.
> > 
> > There are two problems actually.
> > 
> > The first one is that changing an existing command from synchronous
> > to asynchronous is an incompatible change because asynchronous commands
> > semantics is different. For an example of possible problems please
> > check: https://bugzilla.redhat.com/show_bug.cgi?id=623903.
> > 
> > The second problem is that the existing asynchronous interface in the
> > monitor is incomplete and has never been used for real. Our plan is to
> > use QAPI's async support, but that has not landed in master yet and iirc
> > there wasn't consensus about it. I also think it's a bit late for its
> > inclusion in 1.0 (and certainly not a candidate for stable).
> > 
> > If all you need here is to delay sending the response, then maybe the
> > current interface could work (although I honestly don't trust it and
> > regret not having dropped it). Otherwise our only choice would be to
> > work on getting the QAPI async support merged.
> 
> My problem is that the io thread keeps the global mutex during the wait,
> that's why the async monitor is perfect for what I want - this is
> exactly what it does.

Let's not mix internal implementation details with what we want as
an external interface.

Can't you just make a vga_hw_screen_dump() specific callback?

> I haven't looked at QAPI async support, but I
> understand it's a bit in the future.

Yes, it's not for the immediate term.

Re: [Qemu-devel] [PATCH 1/5] monitor: screen_dump async

[Alon Levy]

On Mon, Oct 24, 2011 at 10:31:48PM -0200, Luiz Capitulino wrote:
> On Mon, 24 Oct 2011 19:29:37 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > On Mon, Oct 24, 2011 at 01:45:16PM -0200, Luiz Capitulino wrote:
> > > On Mon, 24 Oct 2011 17:13:14 +0200
> > > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > > 
> > > > On 10/24/11 14:02, Alon Levy wrote:
> > > > > Make screen_dump monitor command an async command to allow next for qxl
> > > > > to implement it as a initiating call to red_worker and completion on
> > > > > callback, to fix a deadlock when issueing a screendump command via
> > > > > libvirt while connected with a libvirt controlled spice-gtk client.
> > > > 
> > > > Approach looks reasonable to me.  Patch breaks the build though, you've
> > > > missed a bunch of screen_dump functions in non-x86 targets.
> > > 
> > > There are two problems actually.
> > > 
> > > The first one is that changing an existing command from synchronous
> > > to asynchronous is an incompatible change because asynchronous commands
> > > semantics is different. For an example of possible problems please
> > > check: https://bugzilla.redhat.com/show_bug.cgi?id=623903.
> > > 
> > > The second problem is that the existing asynchronous interface in the
> > > monitor is incomplete and has never been used for real. Our plan is to
> > > use QAPI's async support, but that has not landed in master yet and iirc
> > > there wasn't consensus about it. I also think it's a bit late for its
> > > inclusion in 1.0 (and certainly not a candidate for stable).
> > > 
> > > If all you need here is to delay sending the response, then maybe the
> > > current interface could work (although I honestly don't trust it and
> > > regret not having dropped it). Otherwise our only choice would be to
> > > work on getting the QAPI async support merged.
> > 
> > My problem is that the io thread keeps the global mutex during the wait,
> > that's why the async monitor is perfect for what I want - this is
> > exactly what it does.
> 
> Let's not mix internal implementation details with what we want as
> an external interface.
> 
> Can't you just make a vga_hw_screen_dump() specific callback?
> 

I don't understand how that would help - if the monitor command doesn't
return (normal sync operation) then the mutex is never dropped, and any
callback won't change that.

On the other hand, thinking a bit about the reference to 623903 baloon
bug, I don't see a problem: the change doesn't affect the semantics for
any other device except qxl, which I've tested. For any other device,
the only difference is that instead of:

do_screen_dump call
 device specific implementation
 return

It becomes

do_screen_dump call
 device specific implementation (not qxl)
  callback called (always - not conditional, no one stores it except
   qxl)
 return

> > I haven't looked at QAPI async support, but I
> > understand it's a bit in the future.
> 
> Yes, it's not for the immediate term.

Re: [Qemu-devel] [PATCH 1/5] monitor: screen_dump async

[Luiz Capitulino]

On Tue, 25 Oct 2011 12:13:09 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Mon, Oct 24, 2011 at 10:31:48PM -0200, Luiz Capitulino wrote:
> > On Mon, 24 Oct 2011 19:29:37 +0200
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > On Mon, Oct 24, 2011 at 01:45:16PM -0200, Luiz Capitulino wrote:
> > > > On Mon, 24 Oct 2011 17:13:14 +0200
> > > > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > > > 
> > > > > On 10/24/11 14:02, Alon Levy wrote:
> > > > > > Make screen_dump monitor command an async command to allow next for qxl
> > > > > > to implement it as a initiating call to red_worker and completion on
> > > > > > callback, to fix a deadlock when issueing a screendump command via
> > > > > > libvirt while connected with a libvirt controlled spice-gtk client.
> > > > > 
> > > > > Approach looks reasonable to me.  Patch breaks the build though, you've
> > > > > missed a bunch of screen_dump functions in non-x86 targets.
> > > > 
> > > > There are two problems actually.
> > > > 
> > > > The first one is that changing an existing command from synchronous
> > > > to asynchronous is an incompatible change because asynchronous commands
> > > > semantics is different. For an example of possible problems please
> > > > check: https://bugzilla.redhat.com/show_bug.cgi?id=623903.
> > > > 
> > > > The second problem is that the existing asynchronous interface in the
> > > > monitor is incomplete and has never been used for real. Our plan is to
> > > > use QAPI's async support, but that has not landed in master yet and iirc
> > > > there wasn't consensus about it. I also think it's a bit late for its
> > > > inclusion in 1.0 (and certainly not a candidate for stable).
> > > > 
> > > > If all you need here is to delay sending the response, then maybe the
> > > > current interface could work (although I honestly don't trust it and
> > > > regret not having dropped it). Otherwise our only choice would be to
> > > > work on getting the QAPI async support merged.
> > > 
> > > My problem is that the io thread keeps the global mutex during the wait,
> > > that's why the async monitor is perfect for what I want - this is
> > > exactly what it does.
> > 
> > Let's not mix internal implementation details with what we want as
> > an external interface.
> > 
> > Can't you just make a vga_hw_screen_dump() specific callback?
> > 
> 
> I don't understand how that would help - if the monitor command doesn't
> return (normal sync operation) then the mutex is never dropped, and any
> callback won't change that.

I'm trying to figure out a different solution.

Our primary motivation for making a QMP command asynchronous must be to
give clients the ability to keep issuing commands while "slow" commands
are running. If that's not what you want nor your primary motivation,
then you're probably taking the wrong approach.

If that is what you want, then you'll have to add a new command, because
changing from asynchronous to synchronous is an incompatible change _and_
you shouldn't use the current interface, because it's botched (actually,
I think I'm going to drop it right now as my last series fixed its only user).

Using a botched interface that doesn't do what's supposed to do but happens to
solve a bug as a side effect will very likely end in tears at some point in
the future.

Now, I did some research and found this description of the problem:

"""
In testing my patches for 'add_client' support with SPICE, I noticed
deadlock in the QEMU/SPICE code. It only happens if I close a SPICE
client and then immediately reconnect within about 1 second. If I
wait a couple of seconds before reconnecting the SPICE client I don't
see the deadlock.
"""
(http://lists.nongnu.org/archive/html/qemu-devel/2011-10/msg02599.html)

Is this accurate? Why does it _work_ after 1 second?

> On the other hand, thinking a bit about the reference to 623903 baloon
> bug, I don't see a problem: the change doesn't affect the semantics for
> any other device except qxl, which I've tested. For any other device,
> the only difference is that instead of:
> 
> do_screen_dump call
>  device specific implementation
>  return
> 
> It becomes
> 
> do_screen_dump call
>  device specific implementation (not qxl)
>   callback called (always - not conditional, no one stores it except
>    qxl)
>  return


> 
> > > I haven't looked at QAPI async support, but I
> > > understand it's a bit in the future.
> > 
> > Yes, it's not for the immediate term.
> 

Re: [Qemu-devel] [PATCH 1/5] monitor: screen_dump async

[Alon Levy]

On Tue, Oct 25, 2011 at 10:51:30AM -0200, Luiz Capitulino wrote:
> On Tue, 25 Oct 2011 12:13:09 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > On Mon, Oct 24, 2011 at 10:31:48PM -0200, Luiz Capitulino wrote:
> > > On Mon, 24 Oct 2011 19:29:37 +0200
> > > Alon Levy <alevy@redhat.com> wrote:
> > > 
> > > > On Mon, Oct 24, 2011 at 01:45:16PM -0200, Luiz Capitulino wrote:
> > > > > On Mon, 24 Oct 2011 17:13:14 +0200
> > > > > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > > > > 
> > > > > > On 10/24/11 14:02, Alon Levy wrote:
> > > > > > > Make screen_dump monitor command an async command to allow next for qxl
> > > > > > > to implement it as a initiating call to red_worker and completion on
> > > > > > > callback, to fix a deadlock when issueing a screendump command via
> > > > > > > libvirt while connected with a libvirt controlled spice-gtk client.
> > > > > > 
> > > > > > Approach looks reasonable to me.  Patch breaks the build though, you've
> > > > > > missed a bunch of screen_dump functions in non-x86 targets.
> > > > > 
> > > > > There are two problems actually.
> > > > > 
> > > > > The first one is that changing an existing command from synchronous
> > > > > to asynchronous is an incompatible change because asynchronous commands
> > > > > semantics is different. For an example of possible problems please
> > > > > check: https://bugzilla.redhat.com/show_bug.cgi?id=623903.
> > > > > 
> > > > > The second problem is that the existing asynchronous interface in the
> > > > > monitor is incomplete and has never been used for real. Our plan is to
> > > > > use QAPI's async support, but that has not landed in master yet and iirc
> > > > > there wasn't consensus about it. I also think it's a bit late for its
> > > > > inclusion in 1.0 (and certainly not a candidate for stable).
> > > > > 
> > > > > If all you need here is to delay sending the response, then maybe the
> > > > > current interface could work (although I honestly don't trust it and
> > > > > regret not having dropped it). Otherwise our only choice would be to
> > > > > work on getting the QAPI async support merged.
> > > > 
> > > > My problem is that the io thread keeps the global mutex during the wait,
> > > > that's why the async monitor is perfect for what I want - this is
> > > > exactly what it does.
> > > 
> > > Let's not mix internal implementation details with what we want as
> > > an external interface.
> > > 
> > > Can't you just make a vga_hw_screen_dump() specific callback?
> > > 
> > 
> > I don't understand how that would help - if the monitor command doesn't
> > return (normal sync operation) then the mutex is never dropped, and any
> > callback won't change that.
> 
> I'm trying to figure out a different solution.
> 
> Our primary motivation for making a QMP command asynchronous must be to
> give clients the ability to keep issuing commands while "slow" commands
> are running. If that's not what you want nor your primary motivation,
> then you're probably taking the wrong approach.

That sounds right, and it was what I assumed the async monitor
implementation would do, boy was I surprised to discover it doesn't do
any such thing, but what it does do is return early, allow *other* io
related events (select returns) to be handled, and keeps the serialized
only-one-command-ongoing monitor usage.

> 
> If that is what you want, then you'll have to add a new command, because
> changing from asynchronous to synchronous is an incompatible change _and_
> you shouldn't use the current interface, because it's botched (actually,
> I think I'm going to drop it right now as my last series fixed its only user).
> 

This is not what I want. I understand of course that it is what one
would design an async monitor / api to allow (it was after all what I
thought async monitor support meant).

> Using a botched interface that doesn't do what's supposed to do but happens to
> solve a bug as a side effect will very likely end in tears at some point in
> the future.
> 

Right, but it's the opposite of the current case.

> Now, I did some research and found this description of the problem:
> 
> """
> In testing my patches for 'add_client' support with SPICE, I noticed
> deadlock in the QEMU/SPICE code. It only happens if I close a SPICE
> client and then immediately reconnect within about 1 second. If I
> wait a couple of seconds before reconnecting the SPICE client I don't
> see the deadlock.
> """
> (http://lists.nongnu.org/archive/html/qemu-devel/2011-10/msg02599.html)
> 
> Is this accurate? Why does it _work_ after 1 second?
>

This is an unrelated bug, I know I said different on the thread but now
rereading the callstack it is the channel_event locking workaround - and
it is fixed by a spice-server only patch (which stops calling
channel_event from the worker thread).

> > On the other hand, thinking a bit about the reference to 623903 baloon
> > bug, I don't see a problem: the change doesn't affect the semantics for
> > any other device except qxl, which I've tested. For any other device,
> > the only difference is that instead of:
> > 
> > do_screen_dump call
> >  device specific implementation
> >  return
> > 
> > It becomes
> > 
> > do_screen_dump call
> >  device specific implementation (not qxl)
> >   callback called (always - not conditional, no one stores it except
> >    qxl)
> >  return
> 
> 
> > 
> > > > I haven't looked at QAPI async support, but I
> > > > understand it's a bit in the future.
> > > 
> > > Yes, it's not for the immediate term.
> > 
> 

Re: [Qemu-devel] [PATCH 1/5] monitor: screen_dump async

[Luiz Capitulino]

On Tue, 25 Oct 2011 15:21:11 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Tue, Oct 25, 2011 at 10:51:30AM -0200, Luiz Capitulino wrote:
> > On Tue, 25 Oct 2011 12:13:09 +0200
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > On Mon, Oct 24, 2011 at 10:31:48PM -0200, Luiz Capitulino wrote:
> > > > On Mon, 24 Oct 2011 19:29:37 +0200
> > > > Alon Levy <alevy@redhat.com> wrote:
> > > > 
> > > > > On Mon, Oct 24, 2011 at 01:45:16PM -0200, Luiz Capitulino wrote:
> > > > > > On Mon, 24 Oct 2011 17:13:14 +0200
> > > > > > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > > > > > 
> > > > > > > On 10/24/11 14:02, Alon Levy wrote:
> > > > > > > > Make screen_dump monitor command an async command to allow next for qxl
> > > > > > > > to implement it as a initiating call to red_worker and completion on
> > > > > > > > callback, to fix a deadlock when issueing a screendump command via
> > > > > > > > libvirt while connected with a libvirt controlled spice-gtk client.
> > > > > > > 
> > > > > > > Approach looks reasonable to me.  Patch breaks the build though, you've
> > > > > > > missed a bunch of screen_dump functions in non-x86 targets.
> > > > > > 
> > > > > > There are two problems actually.
> > > > > > 
> > > > > > The first one is that changing an existing command from synchronous
> > > > > > to asynchronous is an incompatible change because asynchronous commands
> > > > > > semantics is different. For an example of possible problems please
> > > > > > check: https://bugzilla.redhat.com/show_bug.cgi?id=623903.
> > > > > > 
> > > > > > The second problem is that the existing asynchronous interface in the
> > > > > > monitor is incomplete and has never been used for real. Our plan is to
> > > > > > use QAPI's async support, but that has not landed in master yet and iirc
> > > > > > there wasn't consensus about it. I also think it's a bit late for its
> > > > > > inclusion in 1.0 (and certainly not a candidate for stable).
> > > > > > 
> > > > > > If all you need here is to delay sending the response, then maybe the
> > > > > > current interface could work (although I honestly don't trust it and
> > > > > > regret not having dropped it). Otherwise our only choice would be to
> > > > > > work on getting the QAPI async support merged.
> > > > > 
> > > > > My problem is that the io thread keeps the global mutex during the wait,
> > > > > that's why the async monitor is perfect for what I want - this is
> > > > > exactly what it does.
> > > > 
> > > > Let's not mix internal implementation details with what we want as
> > > > an external interface.
> > > > 
> > > > Can't you just make a vga_hw_screen_dump() specific callback?
> > > > 
> > > 
> > > I don't understand how that would help - if the monitor command doesn't
> > > return (normal sync operation) then the mutex is never dropped, and any
> > > callback won't change that.
> > 
> > I'm trying to figure out a different solution.
> > 
> > Our primary motivation for making a QMP command asynchronous must be to
> > give clients the ability to keep issuing commands while "slow" commands
> > are running. If that's not what you want nor your primary motivation,
> > then you're probably taking the wrong approach.
> 
> That sounds right, and it was what I assumed the async monitor
> implementation would do, boy was I surprised to discover it doesn't do
> any such thing, but what it does do is return early, allow *other* io
> related events (select returns) to be handled, and keeps the serialized
> only-one-command-ongoing monitor usage.

Yes, if that turns out to be needed internally then we'll have to add
such functionality (but probably not as part of QMP's async support iiuc).

> > If that is what you want, then you'll have to add a new command, because
> > changing from asynchronous to synchronous is an incompatible change _and_
> > you shouldn't use the current interface, because it's botched (actually,
> > I think I'm going to drop it right now as my last series fixed its only user).
> > 
> 
> This is not what I want. I understand of course that it is what one
> would design an async monitor / api to allow (it was after all what I
> thought async monitor support meant).
> 
> > Using a botched interface that doesn't do what's supposed to do but happens to
> > solve a bug as a side effect will very likely end in tears at some point in
> > the future.
> > 
> 
> Right, but it's the opposite of the current case.
> 
> > Now, I did some research and found this description of the problem:
> > 
> > """
> > In testing my patches for 'add_client' support with SPICE, I noticed
> > deadlock in the QEMU/SPICE code. It only happens if I close a SPICE
> > client and then immediately reconnect within about 1 second. If I
> > wait a couple of seconds before reconnecting the SPICE client I don't
> > see the deadlock.
> > """
> > (http://lists.nongnu.org/archive/html/qemu-devel/2011-10/msg02599.html)
> > 
> > Is this accurate? Why does it _work_ after 1 second?
> >
> 
> This is an unrelated bug, I know I said different on the thread but now
> rereading the callstack it is the channel_event locking workaround - and
> it is fixed by a spice-server only patch (which stops calling
> channel_event from the worker thread).

So, can you try to describe the problem you're having to someone who is
not familiar with qlx and its locking?

> 
> > > On the other hand, thinking a bit about the reference to 623903 baloon
> > > bug, I don't see a problem: the change doesn't affect the semantics for
> > > any other device except qxl, which I've tested. For any other device,
> > > the only difference is that instead of:
> > > 
> > > do_screen_dump call
> > >  device specific implementation
> > >  return
> > > 
> > > It becomes
> > > 
> > > do_screen_dump call
> > >  device specific implementation (not qxl)
> > >   callback called (always - not conditional, no one stores it except
> > >    qxl)
> > >  return
> > 
> > 
> > > 
> > > > > I haven't looked at QAPI async support, but I
> > > > > understand it's a bit in the future.
> > > > 
> > > > Yes, it's not for the immediate term.
> > > 
> > 
> 

Re: [Qemu-devel] [PATCH 1/5] monitor: screen_dump async

[Alon Levy]

On Tue, Oct 25, 2011 at 04:41:18PM -0200, Luiz Capitulino wrote:
> On Tue, 25 Oct 2011 15:21:11 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > On Tue, Oct 25, 2011 at 10:51:30AM -0200, Luiz Capitulino wrote:
> > > On Tue, 25 Oct 2011 12:13:09 +0200
> > > Alon Levy <alevy@redhat.com> wrote:
> > > 
> > > > On Mon, Oct 24, 2011 at 10:31:48PM -0200, Luiz Capitulino wrote:
> > > > > On Mon, 24 Oct 2011 19:29:37 +0200
> > > > > Alon Levy <alevy@redhat.com> wrote:
> > > > > 
> > > > > > On Mon, Oct 24, 2011 at 01:45:16PM -0200, Luiz Capitulino wrote:
> > > > > > > On Mon, 24 Oct 2011 17:13:14 +0200
> > > > > > > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > > > > > > 
> > > > > > > > On 10/24/11 14:02, Alon Levy wrote:
> > > > > > > > > Make screen_dump monitor command an async command to allow next for qxl
> > > > > > > > > to implement it as a initiating call to red_worker and completion on
> > > > > > > > > callback, to fix a deadlock when issueing a screendump command via
> > > > > > > > > libvirt while connected with a libvirt controlled spice-gtk client.
> > > > > > > > 
> > > > > > > > Approach looks reasonable to me.  Patch breaks the build though, you've
> > > > > > > > missed a bunch of screen_dump functions in non-x86 targets.
> > > > > > > 
> > > > > > > There are two problems actually.
> > > > > > > 
> > > > > > > The first one is that changing an existing command from synchronous
> > > > > > > to asynchronous is an incompatible change because asynchronous commands
> > > > > > > semantics is different. For an example of possible problems please
> > > > > > > check: https://bugzilla.redhat.com/show_bug.cgi?id=623903.
> > > > > > > 
> > > > > > > The second problem is that the existing asynchronous interface in the
> > > > > > > monitor is incomplete and has never been used for real. Our plan is to
> > > > > > > use QAPI's async support, but that has not landed in master yet and iirc
> > > > > > > there wasn't consensus about it. I also think it's a bit late for its
> > > > > > > inclusion in 1.0 (and certainly not a candidate for stable).
> > > > > > > 
> > > > > > > If all you need here is to delay sending the response, then maybe the
> > > > > > > current interface could work (although I honestly don't trust it and
> > > > > > > regret not having dropped it). Otherwise our only choice would be to
> > > > > > > work on getting the QAPI async support merged.
> > > > > > 
> > > > > > My problem is that the io thread keeps the global mutex during the wait,
> > > > > > that's why the async monitor is perfect for what I want - this is
> > > > > > exactly what it does.
> > > > > 
> > > > > Let's not mix internal implementation details with what we want as
> > > > > an external interface.
> > > > > 
> > > > > Can't you just make a vga_hw_screen_dump() specific callback?
> > > > > 
> > > > 
> > > > I don't understand how that would help - if the monitor command doesn't
> > > > return (normal sync operation) then the mutex is never dropped, and any
> > > > callback won't change that.
> > > 
> > > I'm trying to figure out a different solution.
> > > 
> > > Our primary motivation for making a QMP command asynchronous must be to
> > > give clients the ability to keep issuing commands while "slow" commands
> > > are running. If that's not what you want nor your primary motivation,
> > > then you're probably taking the wrong approach.
> > 
> > That sounds right, and it was what I assumed the async monitor
> > implementation would do, boy was I surprised to discover it doesn't do
> > any such thing, but what it does do is return early, allow *other* io
> > related events (select returns) to be handled, and keeps the serialized
> > only-one-command-ongoing monitor usage.
> 
> Yes, if that turns out to be needed internally then we'll have to add
> such functionality (but probably not as part of QMP's async support iiuc).
> 
> > > If that is what you want, then you'll have to add a new command, because
> > > changing from asynchronous to synchronous is an incompatible change _and_
> > > you shouldn't use the current interface, because it's botched (actually,
> > > I think I'm going to drop it right now as my last series fixed its only user).
> > > 
> > 
> > This is not what I want. I understand of course that it is what one
> > would design an async monitor / api to allow (it was after all what I
> > thought async monitor support meant).
> > 
> > > Using a botched interface that doesn't do what's supposed to do but happens to
> > > solve a bug as a side effect will very likely end in tears at some point in
> > > the future.
> > > 
> > 
> > Right, but it's the opposite of the current case.
> > 
> > > Now, I did some research and found this description of the problem:
> > > 
> > > """
> > > In testing my patches for 'add_client' support with SPICE, I noticed
> > > deadlock in the QEMU/SPICE code. It only happens if I close a SPICE
> > > client and then immediately reconnect within about 1 second. If I
> > > wait a couple of seconds before reconnecting the SPICE client I don't
> > > see the deadlock.
> > > """
> > > (http://lists.nongnu.org/archive/html/qemu-devel/2011-10/msg02599.html)
> > > 
> > > Is this accurate? Why does it _work_ after 1 second?
> > >
> > 
> > This is an unrelated bug, I know I said different on the thread but now
> > rereading the callstack it is the channel_event locking workaround - and
> > it is fixed by a spice-server only patch (which stops calling
> > channel_event from the worker thread).
> 
> So, can you try to describe the problem you're having to someone who is
> not familiar with qlx and its locking?
> 

I'm suddenly not sure this patchset even fixes the problem that I said
it does, so I'll update once I made sure what it actually fixes.

> > 
> > > > On the other hand, thinking a bit about the reference to 623903 baloon
> > > > bug, I don't see a problem: the change doesn't affect the semantics for
> > > > any other device except qxl, which I've tested. For any other device,
> > > > the only difference is that instead of:
> > > > 
> > > > do_screen_dump call
> > > >  device specific implementation
> > > >  return
> > > > 
> > > > It becomes
> > > > 
> > > > do_screen_dump call
> > > >  device specific implementation (not qxl)
> > > >   callback called (always - not conditional, no one stores it except
> > > >    qxl)
> > > >  return
> > > 
> > > 
> > > > 
> > > > > > I haven't looked at QAPI async support, but I
> > > > > > understand it's a bit in the future.
> > > > > 
> > > > > Yes, it's not for the immediate term.
> > > > 
> > > 
> > 
> 

Re: [Qemu-devel] [PATCH 1/5] monitor: screen_dump async

[Gerd Hoffmann]

  Hi,

> If all you need here is to delay sending the response, then maybe the
> current interface could work (although I honestly don't trust it and
> regret not having dropped it). Otherwise our only choice would be to
> work on getting the QAPI async support merged.

A delayed monitor response is all we need.

The command may take a bit to finish, where "a bit" is in the order of a
few seconds max, usually less than a second.  Blocking the monitor for
that amount of time is fine, but blocking the io thread introduces
insane long I/O latencies for the guest (and device emulation).

cheers,
  Gerd

[Qemu-devel] [PATCH 2/5] qxl: s/__FUNCTION__/__func__/, change logging levels

[Alon Levy]

---
 hw/qxl.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 4003e53..af02cda 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -515,7 +515,7 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
 
     switch (qxl->mode) {
     case QXL_MODE_VGA:
-        dprint(qxl, 2, "%s: vga\n", __FUNCTION__);
+        dprint(qxl, 4, "%s: vga\n", __func__);
         ret = false;
         qemu_mutex_lock(&qxl->ssd.lock);
         if (qxl->ssd.update != NULL) {
@@ -526,19 +526,19 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
         }
         qemu_mutex_unlock(&qxl->ssd.lock);
         if (ret) {
-            dprint(qxl, 2, "%s %s\n", __FUNCTION__, qxl_mode_to_string(qxl->mode));
+            dprint(qxl, 4, "%s %s\n", __func__, qxl_mode_to_string(qxl->mode));
             qxl_log_command(qxl, "vga", ext);
         }
         return ret;
     case QXL_MODE_COMPAT:
     case QXL_MODE_NATIVE:
     case QXL_MODE_UNDEFINED:
-        dprint(qxl, 4, "%s: %s\n", __FUNCTION__, qxl_mode_to_string(qxl->mode));
+        dprint(qxl, 5, "%s: %s\n", __func__, qxl_mode_to_string(qxl->mode));
         ring = &qxl->ram->cmd_ring;
         if (SPICE_RING_IS_EMPTY(ring)) {
             return false;
         }
-        dprint(qxl, 2, "%s: %s\n", __FUNCTION__, qxl_mode_to_string(qxl->mode));
+        dprint(qxl, 4, "%s: %s\n", __func__, qxl_mode_to_string(qxl->mode));
         SPICE_RING_CONS_ITEM(ring, cmd);
         ext->cmd      = *cmd;
         ext->group_id = MEMSLOT_GROUP_GUEST;
-- 
1.7.7

[Qemu-devel] [PATCH 3/5] qxl: support concurrent async commands

[Alon Levy]

change the single pending current_async to a linked list of pending
SpiceAsyncCommand.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl-render.c    |    2 +-
 hw/qxl.c           |  145 +++++++++++++++++++++++++++++++++++-----------------
 hw/qxl.h           |   29 +++++++++--
 ui/spice-display.c |   19 ++++---
 ui/spice-display.h |    7 ++-
 5 files changed, 137 insertions(+), 65 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index c290739..23a4289 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -125,7 +125,7 @@ void qxl_render_update(PCIQXLDevice *qxl)
 
     memset(dirty, 0, sizeof(dirty));
     qxl_spice_update_area(qxl, 0, &update,
-                          dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC);
+                          dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC, 0);
 
     for (i = 0; i < ARRAY_SIZE(dirty); i++) {
         if (qemu_spice_rect_is_empty(dirty+i)) {
diff --git a/hw/qxl.c b/hw/qxl.c
index af02cda..008f5f7 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -120,7 +120,8 @@ static QXLMode qxl_modes[] = {
 static PCIQXLDevice *qxl0;
 
 static void qxl_send_events(PCIQXLDevice *d, uint32_t events);
-static int qxl_destroy_primary(PCIQXLDevice *d, qxl_async_io async);
+static int qxl_destroy_primary(PCIQXLDevice *d, qxl_async_io async,
+                               uint64_t cookie);
 static void qxl_reset_memslots(PCIQXLDevice *d);
 static void qxl_reset_surfaces(PCIQXLDevice *d);
 static void qxl_ring_set_dirty(PCIQXLDevice *qxl);
@@ -140,12 +141,11 @@ void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
     }
 }
 
-
 void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
                            struct QXLRect *area, struct QXLRect *dirty_rects,
                            uint32_t num_dirty_rects,
                            uint32_t clear_dirty_region,
-                           qxl_async_io async)
+                           qxl_async_io async, uint64_t cookie)
 {
     if (async == QXL_SYNC) {
         qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area,
@@ -153,7 +153,7 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
     } else {
 #if SPICE_INTERFACE_QXL_MINOR >= 1
         spice_qxl_update_area_async(&qxl->ssd.qxl, surface_id, area,
-                                    clear_dirty_region, 0);
+                                    clear_dirty_region, cookie);
 #else
         abort();
 #endif
@@ -170,14 +170,13 @@ static void qxl_spice_destroy_surface_wait_complete(PCIQXLDevice *qxl,
 }
 
 static void qxl_spice_destroy_surface_wait(PCIQXLDevice *qxl, uint32_t id,
-                                           qxl_async_io async)
+                                           qxl_async_io async, uint64_t cookie)
 {
     if (async) {
 #if SPICE_INTERFACE_QXL_MINOR < 1
         abort();
 #else
-        spice_qxl_destroy_surface_async(&qxl->ssd.qxl, id,
-                                        (uint64_t)id);
+        spice_qxl_destroy_surface_async(&qxl->ssd.qxl, id, cookie);
 #endif
     } else {
         qxl->ssd.worker->destroy_surface_wait(qxl->ssd.worker, id);
@@ -186,9 +185,9 @@ static void qxl_spice_destroy_surface_wait(PCIQXLDevice *qxl, uint32_t id,
 }
 
 #if SPICE_INTERFACE_QXL_MINOR >= 1
-static void qxl_spice_flush_surfaces_async(PCIQXLDevice *qxl)
+static void qxl_spice_flush_surfaces_async(PCIQXLDevice *qxl, uint64_t cookie)
 {
-    spice_qxl_flush_surfaces_async(&qxl->ssd.qxl, 0);
+    spice_qxl_flush_surfaces_async(&qxl->ssd.qxl, cookie);
 }
 #endif
 
@@ -216,13 +215,14 @@ static void qxl_spice_destroy_surfaces_complete(PCIQXLDevice *qxl)
     qemu_mutex_unlock(&qxl->track_lock);
 }
 
-static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl, qxl_async_io async)
+static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl, qxl_async_io async,
+                                       uint64_t cookie)
 {
     if (async) {
 #if SPICE_INTERFACE_QXL_MINOR < 1
         abort();
 #else
-        spice_qxl_destroy_surfaces_async(&qxl->ssd.qxl, 0);
+        spice_qxl_destroy_surfaces_async(&qxl->ssd.qxl, cookie);
 #endif
     } else {
         qxl->ssd.worker->destroy_surfaces(qxl->ssd.worker);
@@ -736,17 +736,61 @@ static void qxl_create_guest_primary_complete(PCIQXLDevice *d);
 
 #if SPICE_INTERFACE_QXL_MINOR >= 1
 
+SpiceAsyncCommand *push_spice_async_command(PCIQXLDevice *qxl,
+    uint32_t async_io, int size)
+{
+    assert(size >= sizeof(SpiceAsyncCommand));
+    SpiceAsyncCommand *spice_async_command = g_malloc0(size);
+
+    spice_async_command->cookie = qxl->async_next_cookie++;
+    spice_async_command->async_io = async_io;
+    QLIST_INSERT_HEAD(&qxl->async_commands, spice_async_command, next);
+    dprint(qxl, 2, "allocated async cookie %"PRId64"\n",
+           qxl->async_next_cookie - 1);
+    return spice_async_command;
+}
+
+/* caller must call g_free */
+static SpiceAsyncCommand *pop_spice_async_command(PCIQXLDevice *qxl,
+                                                  uint64_t cookie)
+{
+    SpiceAsyncCommand *spice_async_command = NULL;
+
+    qemu_mutex_lock(&qxl->async_lock);
+    QLIST_FOREACH(spice_async_command, &qxl->async_commands, next) {
+        if (spice_async_command->cookie == cookie) {
+            QLIST_REMOVE(spice_async_command, next);
+            break;
+        }
+    }
+    qemu_mutex_unlock(&qxl->async_lock);
+    return spice_async_command;
+}
+
+/* can be called from any thread */
+void complete_spice_async_command(PCIQXLDevice *qxl,
+            SpiceAsyncCommand *spice_async_command)
+{
+    if (!spice_async_command->completion) {
+        return;
+    }
+    spice_async_command->completion(qxl, spice_async_command);
+}
+
 /* called from spice server thread context only */
 static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
 {
     PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
     uint32_t current_async;
+    SpiceAsyncCommand *spice_async_command;
 
-    qemu_mutex_lock(&qxl->async_lock);
-    current_async = qxl->current_async;
-    qxl->current_async = QXL_UNDEFINED_IO;
-    qemu_mutex_unlock(&qxl->async_lock);
-
+    spice_async_command = pop_spice_async_command(qxl, cookie);
+    if (!spice_async_command) {
+        dprint(qxl, 1, "async_complete: ERROR: response to not "
+                       "pending operation, cookie=%ld\n", cookie);
+        return;
+    }
+    current_async = spice_async_command->async_io;
     dprint(qxl, 2, "async_complete: %d (%ld) done\n", current_async, cookie);
     switch (current_async) {
     case QXL_IO_CREATE_PRIMARY_ASYNC:
@@ -756,9 +800,12 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
         qxl_spice_destroy_surfaces_complete(qxl);
         break;
     case QXL_IO_DESTROY_SURFACE_ASYNC:
-        qxl_spice_destroy_surface_wait_complete(qxl, (uint32_t)cookie);
+        qxl_spice_destroy_surface_wait_complete(qxl,
+            (uint64_t)spice_async_command->opaque);
         break;
     }
+    complete_spice_async_command(qxl, spice_async_command);
+    g_free(spice_async_command);
     qxl_send_events(qxl, QXL_INTERRUPT_IO_CMD);
 }
 
@@ -805,7 +852,7 @@ static void qxl_exit_vga_mode(PCIQXLDevice *d)
         return;
     }
     dprint(d, 1, "%s\n", __FUNCTION__);
-    qxl_destroy_primary(d, QXL_SYNC);
+    qxl_destroy_primary(d, QXL_SYNC, 0);
 }
 
 static void qxl_update_irq(PCIQXLDevice *d)
@@ -886,14 +933,14 @@ static void qxl_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 
     if (qxl->mode != QXL_MODE_VGA) {
         dprint(qxl, 1, "%s\n", __FUNCTION__);
-        qxl_destroy_primary(qxl, QXL_SYNC);
+        qxl_destroy_primary(qxl, QXL_SYNC, 0);
         qxl_soft_reset(qxl);
     }
     vga_ioport_write(opaque, addr, val);
 }
 
 static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta,
-                            qxl_async_io async)
+                            qxl_async_io async, uint64_t cookie)
 {
     static const int regions[] = {
         QXL_RAM_RANGE_INDEX,
@@ -963,7 +1010,7 @@ static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta,
            __FUNCTION__, memslot.slot_id,
            memslot.virt_start, memslot.virt_end);
 
-    qemu_spice_add_memslot(&d->ssd, &memslot, async);
+    qemu_spice_add_memslot(&d->ssd, &memslot, async, cookie);
     d->guest_slots[slot_id].ptr = (void*)memslot.virt_start;
     d->guest_slots[slot_id].size = memslot.virt_end - memslot.virt_start;
     d->guest_slots[slot_id].delta = delta;
@@ -988,7 +1035,7 @@ static void qxl_reset_surfaces(PCIQXLDevice *d)
 {
     dprint(d, 1, "%s:\n", __FUNCTION__);
     d->mode = QXL_MODE_UNDEFINED;
-    qxl_spice_destroy_surfaces(d, QXL_SYNC);
+    qxl_spice_destroy_surfaces(d, QXL_SYNC, 0);
 }
 
 /* called from spice server thread context only */
@@ -1020,7 +1067,7 @@ static void qxl_create_guest_primary_complete(PCIQXLDevice *qxl)
 }
 
 static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
-                                     qxl_async_io async)
+                                     qxl_async_io async, uint64_t cookie)
 {
     QXLDevSurfaceCreate surface;
     QXLSurfaceCreate *sc = &qxl->guest_primary.surface;
@@ -1048,7 +1095,7 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
 
     qxl->mode = QXL_MODE_NATIVE;
     qxl->cmdflags = 0;
-    qemu_spice_create_primary_surface(&qxl->ssd, 0, &surface, async);
+    qemu_spice_create_primary_surface(&qxl->ssd, 0, &surface, async, cookie);
 
     if (async == QXL_SYNC) {
         qxl_create_guest_primary_complete(qxl);
@@ -1057,7 +1104,8 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
 
 /* return 1 if surface destoy was initiated (in QXL_ASYNC case) or
  * done (in QXL_SYNC case), 0 otherwise. */
-static int qxl_destroy_primary(PCIQXLDevice *d, qxl_async_io async)
+static int qxl_destroy_primary(PCIQXLDevice *d, qxl_async_io async,
+                               uint64_t cookie)
 {
     if (d->mode == QXL_MODE_UNDEFINED) {
         return 0;
@@ -1066,7 +1114,7 @@ static int qxl_destroy_primary(PCIQXLDevice *d, qxl_async_io async)
     dprint(d, 1, "%s\n", __FUNCTION__);
 
     d->mode = QXL_MODE_UNDEFINED;
-    qemu_spice_destroy_primary_surface(&d->ssd, 0, async);
+    qemu_spice_destroy_primary_surface(&d->ssd, 0, async, cookie);
     return 1;
 }
 
@@ -1097,10 +1145,10 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm)
     }
 
     d->guest_slots[0].slot = slot;
-    qxl_add_memslot(d, 0, devmem, QXL_SYNC);
+    qxl_add_memslot(d, 0, devmem, QXL_SYNC, 0);
 
     d->guest_primary.surface = surface;
-    qxl_create_guest_primary(d, 0, QXL_SYNC);
+    qxl_create_guest_primary(d, 0, QXL_SYNC, 0);
 
     d->mode = QXL_MODE_COMPAT;
     d->cmdflags = QXL_COMMAND_FLAG_COMPAT;
@@ -1122,7 +1170,9 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
     qxl_async_io async = QXL_SYNC;
 #if SPICE_INTERFACE_QXL_MINOR >= 1
     uint32_t orig_io_port = io_port;
+    SpiceAsyncCommand *spice_async_command = NULL;
 #endif
+    uint64_t cookie = 0;
 
     switch (io_port) {
     case QXL_IO_RESET:
@@ -1179,15 +1229,12 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
 async_common:
         async = QXL_ASYNC;
         qemu_mutex_lock(&d->async_lock);
-        if (d->current_async != QXL_UNDEFINED_IO) {
-            qxl_guest_bug(d, "%d async started before last (%d) complete",
-                io_port, d->current_async);
-            qemu_mutex_unlock(&d->async_lock);
-            return;
-        }
-        d->current_async = orig_io_port;
+        spice_async_command = push_spice_async_command(d, orig_io_port,
+                                                sizeof(SpiceAsyncCommand));
         qemu_mutex_unlock(&d->async_lock);
-        dprint(d, 2, "start async %d (%"PRId64")\n", io_port, val);
+        cookie = spice_async_command->cookie;
+        dprint(d, 2, "start async %d (%"PRId64") cookie %"PRId64"\n", io_port,
+               val, cookie);
         break;
     default:
         break;
@@ -1199,7 +1246,7 @@ async_common:
     {
         QXLRect update = d->ram->update_area;
         qxl_spice_update_area(d, d->ram->update_surface,
-                              &update, NULL, 0, 0, async);
+                              &update, NULL, 0, 0, async, cookie);
         break;
     }
     case QXL_IO_NOTIFY_CMD:
@@ -1247,7 +1294,7 @@ async_common:
             break;
         }
         d->guest_slots[val].slot = d->ram->mem_slot;
-        qxl_add_memslot(d, val, 0, async);
+        qxl_add_memslot(d, val, 0, async, cookie);
         break;
     case QXL_IO_MEMSLOT_DEL:
         if (val >= NUM_MEMSLOTS) {
@@ -1264,7 +1311,7 @@ async_common:
         }
         dprint(d, 1, "QXL_IO_CREATE_PRIMARY async=%d\n", async);
         d->guest_primary.surface = d->ram->create_surface;
-        qxl_create_guest_primary(d, 0, async);
+        qxl_create_guest_primary(d, 0, async, cookie);
         break;
     case QXL_IO_DESTROY_PRIMARY:
         if (val != 0) {
@@ -1274,7 +1321,7 @@ async_common:
         }
         dprint(d, 1, "QXL_IO_DESTROY_PRIMARY (async=%d) (%s)\n", async,
                qxl_mode_to_string(d->mode));
-        if (!qxl_destroy_primary(d, async)) {
+        if (!qxl_destroy_primary(d, async, cookie)) {
             dprint(d, 1, "QXL_IO_DESTROY_PRIMARY_ASYNC in %s, ignored\n",
                     qxl_mode_to_string(d->mode));
             goto cancel_async;
@@ -1286,7 +1333,9 @@ async_common:
                              "%d >= NUM_SURFACES", async, val);
             goto cancel_async;
         }
-        qxl_spice_destroy_surface_wait(d, val, async);
+        spice_async_command->opaque = (void *)(uint64_t)val;
+        qxl_spice_destroy_surface_wait(d, val, async,
+                                       spice_async_command->cookie);
         break;
 #if SPICE_INTERFACE_QXL_MINOR >= 1
     case QXL_IO_FLUSH_RELEASE: {
@@ -1307,12 +1356,12 @@ async_common:
                      " (%"PRId64") (%s, s#=%d, res#=%d)\n",
                val, qxl_mode_to_string(d->mode), d->guest_surfaces.count,
                d->num_free_res);
-        qxl_spice_flush_surfaces_async(d);
+        qxl_spice_flush_surfaces_async(d, cookie);
         break;
 #endif
     case QXL_IO_DESTROY_ALL_SURFACES:
         d->mode = QXL_MODE_UNDEFINED;
-        qxl_spice_destroy_surfaces(d, async);
+        qxl_spice_destroy_surfaces(d, async, cookie);
         break;
     default:
         fprintf(stderr, "%s: ioport=0x%x, abort()\n", __FUNCTION__, io_port);
@@ -1324,7 +1373,7 @@ cancel_async:
     if (async) {
         qxl_send_events(d, QXL_INTERRUPT_IO_CMD);
         qemu_mutex_lock(&d->async_lock);
-        d->current_async = QXL_UNDEFINED_IO;
+        pop_spice_async_command(d, spice_async_command->cookie);
         qemu_mutex_unlock(&d->async_lock);
     }
 #else
@@ -1520,7 +1569,8 @@ static int qxl_init_common(PCIQXLDevice *qxl)
     qxl->num_surfaces = NUM_SURFACES;
     qemu_mutex_init(&qxl->track_lock);
     qemu_mutex_init(&qxl->async_lock);
-    qxl->current_async = QXL_UNDEFINED_IO;
+    QLIST_INIT(&qxl->async_commands);
+    qxl->async_next_cookie = 0;
 
     switch (qxl->revision) {
     case 1: /* spice 0.4 -- qxl-1 */
@@ -1697,9 +1747,9 @@ static int qxl_post_load(void *opaque, int version)
             if (!d->guest_slots[i].active) {
                 continue;
             }
-            qxl_add_memslot(d, i, 0, QXL_SYNC);
+            qxl_add_memslot(d, i, 0, QXL_SYNC, 0);
         }
-        qxl_create_guest_primary(d, 1, QXL_SYNC);
+        qxl_create_guest_primary(d, 1, QXL_SYNC, 0);
 
         /* replay surface-create and cursor-set commands */
         cmds = g_malloc0(sizeof(QXLCommandExt) * (NUM_SURFACES + 1));
@@ -1718,7 +1768,6 @@ static int qxl_post_load(void *opaque, int version)
         out++;
         qxl_spice_loadvm_commands(d, cmds, out);
         g_free(cmds);
-
         break;
     case QXL_MODE_COMPAT:
         qxl_set_mode(d, d->shadow_rom.mode, 1);
diff --git a/hw/qxl.h b/hw/qxl.h
index 868db81..4c89e14 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -17,7 +17,22 @@ enum qxl_mode {
 
 #define QXL_UNDEFINED_IO UINT32_MAX
 
-typedef struct PCIQXLDevice {
+typedef struct PCIQXLDevice PCIQXLDevice;
+typedef struct SpiceAsyncCommand SpiceAsyncCommand;
+typedef void (*SpiceAsyncCommandCompletion)(PCIQXLDevice *,
+                                            SpiceAsyncCommand *);
+
+/* Outstanding async request sent to spice server.
+ * cookie must be unique across the list. */
+struct SpiceAsyncCommand {
+    uint64_t                       cookie;
+    void                          *opaque;
+    uint32_t                       async_io;
+    SpiceAsyncCommandCompletion    completion;
+    QLIST_ENTRY(SpiceAsyncCommand) next;
+};
+
+struct PCIQXLDevice {
     PCIDevice          pci;
     SimpleSpiceDisplay ssd;
     int                id;
@@ -32,7 +47,8 @@ typedef struct PCIQXLDevice {
     int32_t            num_memslots;
     int32_t            num_surfaces;
 
-    uint32_t           current_async;
+    QLIST_HEAD(, SpiceAsyncCommand) async_commands;
+    uint64_t           async_next_cookie;
     QemuMutex          async_lock;
 
     struct guest_slots {
@@ -87,7 +103,7 @@ typedef struct PCIQXLDevice {
 
     /* io bar */
     MemoryRegion       io_bar;
-} PCIQXLDevice;
+};
 
 #define PANIC_ON(x) if ((x)) {                         \
     printf("%s: PANIC %s failed\n", __FUNCTION__, #x); \
@@ -116,13 +132,18 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
                            struct QXLRect *area, struct QXLRect *dirty_rects,
                            uint32_t num_dirty_rects,
                            uint32_t clear_dirty_region,
-                           qxl_async_io async);
+                           qxl_async_io async, uint64_t cookie);
 void qxl_spice_loadvm_commands(PCIQXLDevice *qxl, struct QXLCommandExt *ext,
                                uint32_t count);
 void qxl_spice_oom(PCIQXLDevice *qxl);
 void qxl_spice_reset_memslots(PCIQXLDevice *qxl);
 void qxl_spice_reset_image_cache(PCIQXLDevice *qxl);
 void qxl_spice_reset_cursor(PCIQXLDevice *qxl);
+SpiceAsyncCommand *push_spice_async_command(PCIQXLDevice *qxl,
+                                            uint32_t async_io, int size);
+/* complete_spice_async_command: call to cleanup a command */
+void complete_spice_async_command(PCIQXLDevice *qxl,
+                                  SpiceAsyncCommand *spice_async_command);
 
 /* qxl-logger.c */
 void qxl_log_cmd_cursor(PCIQXLDevice *qxl, QXLCursorCmd *cmd, int group_id);
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 6c302a3..71e7305 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -61,11 +61,11 @@ void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r)
 }
 
 void qemu_spice_add_memslot(SimpleSpiceDisplay *ssd, QXLDevMemSlot *memslot,
-                            qxl_async_io async)
+                            qxl_async_io async, uint64_t cookie)
 {
     if (async != QXL_SYNC) {
 #if SPICE_INTERFACE_QXL_MINOR >= 1
-        spice_qxl_add_memslot_async(&ssd->qxl, memslot, 0);
+        spice_qxl_add_memslot_async(&ssd->qxl, memslot, cookie);
 #else
         abort();
 #endif
@@ -81,11 +81,11 @@ void qemu_spice_del_memslot(SimpleSpiceDisplay *ssd, uint32_t gid, uint32_t sid)
 
 void qemu_spice_create_primary_surface(SimpleSpiceDisplay *ssd, uint32_t id,
                                        QXLDevSurfaceCreate *surface,
-                                       qxl_async_io async)
+                                       qxl_async_io async, uint64_t cookie)
 {
     if (async != QXL_SYNC) {
 #if SPICE_INTERFACE_QXL_MINOR >= 1
-        spice_qxl_create_primary_surface_async(&ssd->qxl, id, surface, 0);
+        spice_qxl_create_primary_surface_async(&ssd->qxl, id, surface, cookie);
 #else
         abort();
 #endif
@@ -96,11 +96,12 @@ void qemu_spice_create_primary_surface(SimpleSpiceDisplay *ssd, uint32_t id,
 
 
 void qemu_spice_destroy_primary_surface(SimpleSpiceDisplay *ssd,
-                                        uint32_t id, qxl_async_io async)
+                                        uint32_t id, qxl_async_io async,
+                                        uint64_t cookie)
 {
     if (async != QXL_SYNC) {
 #if SPICE_INTERFACE_QXL_MINOR >= 1
-        spice_qxl_destroy_primary_surface_async(&ssd->qxl, id, 0);
+        spice_qxl_destroy_primary_surface_async(&ssd->qxl, id, cookie);
 #else
         abort();
 #endif
@@ -223,7 +224,7 @@ void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd)
     memset(&memslot, 0, sizeof(memslot));
     memslot.slot_group_id = MEMSLOT_GROUP_HOST;
     memslot.virt_end = ~0;
-    qemu_spice_add_memslot(ssd, &memslot, QXL_SYNC);
+    qemu_spice_add_memslot(ssd, &memslot, QXL_SYNC, 0);
 }
 
 void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
@@ -243,14 +244,14 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
     surface.mem        = (intptr_t)ssd->buf;
     surface.group_id   = MEMSLOT_GROUP_HOST;
 
-    qemu_spice_create_primary_surface(ssd, 0, &surface, QXL_SYNC);
+    qemu_spice_create_primary_surface(ssd, 0, &surface, QXL_SYNC, 0);
 }
 
 void qemu_spice_destroy_host_primary(SimpleSpiceDisplay *ssd)
 {
     dprint(1, "%s:\n", __FUNCTION__);
 
-    qemu_spice_destroy_primary_surface(ssd, 0, QXL_SYNC);
+    qemu_spice_destroy_primary_surface(ssd, 0, QXL_SYNC, 0);
 }
 
 void qemu_spice_vm_change_state_handler(void *opaque, int running,
diff --git a/ui/spice-display.h b/ui/spice-display.h
index 5e52df9..5db7eb8 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -99,14 +99,15 @@ void qemu_spice_display_resize(SimpleSpiceDisplay *ssd);
 void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd);
 
 void qemu_spice_add_memslot(SimpleSpiceDisplay *ssd, QXLDevMemSlot *memslot,
-                            qxl_async_io async);
+                            qxl_async_io async, uint64_t cookie);
 void qemu_spice_del_memslot(SimpleSpiceDisplay *ssd, uint32_t gid,
                             uint32_t sid);
 void qemu_spice_create_primary_surface(SimpleSpiceDisplay *ssd, uint32_t id,
                                        QXLDevSurfaceCreate *surface,
-                                       qxl_async_io async);
+                                       qxl_async_io async, uint64_t cookie);
 void qemu_spice_destroy_primary_surface(SimpleSpiceDisplay *ssd,
-                                        uint32_t id, qxl_async_io async);
+                                        uint32_t id, qxl_async_io async,
+                                        uint64_t cookie);
 void qemu_spice_wakeup(SimpleSpiceDisplay *ssd);
 void qemu_spice_start(SimpleSpiceDisplay *ssd);
 void qemu_spice_stop(SimpleSpiceDisplay *ssd);
-- 
1.7.7

Re: [Qemu-devel] [PATCH 3/5] qxl: support concurrent async commands

[Gerd Hoffmann]

  Hi,

> +SpiceAsyncCommand *push_spice_async_command(PCIQXLDevice *qxl,
> +    uint32_t async_io, int size)

> +/* caller must call g_free */
> +static SpiceAsyncCommand *pop_spice_async_command(PCIQXLDevice *qxl,
> +                                                  uint64_t cookie)
> +{

push/pop naming implies stack-like operation, which isn't true though.
pop will lookup by cookie.  Also an explicit release function would be
good (list unlink and g_free call can go there).

Maybe have spice_async_cmd_{alloc,lookup,free} ?

Have you considered passing down a SpiceAsyncCommand pointer instead of
the cookie value everywhere?  Seems to be a bit cleaner and more
future-proof to me.  Not sure it buys us much in practice though, so
maybe it isn't worth the trouble.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/5] qxl: support concurrent async commands

[Alon Levy]

On Mon, Oct 24, 2011 at 05:26:11PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > +SpiceAsyncCommand *push_spice_async_command(PCIQXLDevice *qxl,
> > +    uint32_t async_io, int size)
> 
> > +/* caller must call g_free */
> > +static SpiceAsyncCommand *pop_spice_async_command(PCIQXLDevice *qxl,
> > +                                                  uint64_t cookie)
> > +{
> 
> push/pop naming implies stack-like operation, which isn't true though.
> pop will lookup by cookie.  Also an explicit release function would be
> good (list unlink and g_free call can go there).
> 
> Maybe have spice_async_cmd_{alloc,lookup,free} ?
> 
Will do.

> Have you considered passing down a SpiceAsyncCommand pointer instead of
> the cookie value everywhere?  Seems to be a bit cleaner and more
> future-proof to me.  Not sure it buys us much in practice though, so
> maybe it isn't worth the trouble.
> 

That would still require casting, since we want to ensure we can later
support 32 bit architectures, so the cookie size of uint64_t will
remain. I guess I'll do it.

> cheers,
>   Gerd

[Qemu-devel] [PATCH 4/5] qxl: split qxl_render_display_resized

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl-render.c |   75 ++++++++++++++++++++++++++++++-------------------------
 1 files changed, 41 insertions(+), 34 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 23a4289..fd3c016 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -70,47 +70,54 @@ void qxl_render_resize(PCIQXLDevice *qxl)
     }
 }
 
+static void qxl_render_display_resized(PCIQXLDevice *qxl)
+{
+    VGACommonState *vga = &qxl->vga;
+    void *ptr;
+
+    qxl->guest_primary.resized = 0;
+
+    if (qxl->guest_primary.flipped) {
+        g_free(qxl->guest_primary.flipped);
+        qxl->guest_primary.flipped = NULL;
+    }
+    qemu_free_displaysurface(vga->ds);
+
+    qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram);
+    if (qxl->guest_primary.stride < 0) {
+        /* spice surface is upside down -> need extra buffer to flip */
+        qxl->guest_primary.stride = -qxl->guest_primary.stride;
+        qxl->guest_primary.flipped = g_malloc(qxl->guest_primary.surface.width *
+                                                 qxl->guest_primary.stride);
+        ptr = qxl->guest_primary.flipped;
+    } else {
+        ptr = qxl->guest_primary.data;
+    }
+    dprint(qxl, 1, "%s: %dx%d, stride %d, bpp %d, depth %d, flip %s\n",
+           __func__,
+           qxl->guest_primary.surface.width,
+           qxl->guest_primary.surface.height,
+           qxl->guest_primary.stride,
+           qxl->guest_primary.bytes_pp,
+           qxl->guest_primary.bits_pp,
+           qxl->guest_primary.flipped ? "yes" : "no");
+    vga->ds->surface =
+        qemu_create_displaysurface_from(qxl->guest_primary.surface.width,
+                                        qxl->guest_primary.surface.height,
+                                        qxl->guest_primary.bits_pp,
+                                        qxl->guest_primary.stride,
+                                        ptr);
+    dpy_resize(vga->ds);
+}
+
 void qxl_render_update(PCIQXLDevice *qxl)
 {
     VGACommonState *vga = &qxl->vga;
     QXLRect dirty[32], update;
-    void *ptr;
     int i;
 
     if (qxl->guest_primary.resized) {
-        qxl->guest_primary.resized = 0;
-
-        if (qxl->guest_primary.flipped) {
-            g_free(qxl->guest_primary.flipped);
-            qxl->guest_primary.flipped = NULL;
-        }
-        qemu_free_displaysurface(vga->ds);
-
-        qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram);
-        if (qxl->guest_primary.stride < 0) {
-            /* spice surface is upside down -> need extra buffer to flip */
-            qxl->guest_primary.stride = -qxl->guest_primary.stride;
-            qxl->guest_primary.flipped = g_malloc(qxl->guest_primary.surface.width *
-                                                     qxl->guest_primary.stride);
-            ptr = qxl->guest_primary.flipped;
-        } else {
-            ptr = qxl->guest_primary.data;
-        }
-        dprint(qxl, 1, "%s: %dx%d, stride %d, bpp %d, depth %d, flip %s\n",
-               __FUNCTION__,
-               qxl->guest_primary.surface.width,
-               qxl->guest_primary.surface.height,
-               qxl->guest_primary.stride,
-               qxl->guest_primary.bytes_pp,
-               qxl->guest_primary.bits_pp,
-               qxl->guest_primary.flipped ? "yes" : "no");
-        vga->ds->surface =
-            qemu_create_displaysurface_from(qxl->guest_primary.surface.width,
-                                            qxl->guest_primary.surface.height,
-                                            qxl->guest_primary.bits_pp,
-                                            qxl->guest_primary.stride,
-                                            ptr);
-        dpy_resize(vga->ds);
+        qxl_render_display_resized(qxl);
     }
 
     if (!qxl->guest_primary.commands) {
-- 
1.7.7

[Qemu-devel] [PATCH 5/5] qxl: support async monitor screen dump

[Alon Levy]

Split qxl_spice_update_area_complete from qxl_render_update, use
SPICE_INTERFACE_QXL_MINOR 2 introduced spice_qxl_update_area_dirty_async
to retrive the dirty rectangles asyncronously (the previous
spice_qxl_update_area_async did not accept a dirty rectangles array).

Introduce SpiceAsyncMonitorScreenDump for a screen_dump.

vga mode screen dumps are still synchronous.

Signed-off-by: Alon Levy <alevy@redhat.com>

---
patchcheck gives one false positive, identifying a point function
argument as a multiplication:

ERROR: need consistent spacing around '*' (ctx:WxV)
 #135: FILE: hw/qxl.c:148:
+                           SpiceAsyncCommand *async_command)
                                              ^
---
 hw/qxl-render.c |   67 +++++++++++++++++++++++++++++---------
 hw/qxl.c        |   95 +++++++++++++++++++++++++++++++++++++++++++-----------
 hw/qxl.h        |   38 ++++++++++++++++++++--
 3 files changed, 161 insertions(+), 39 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index fd3c016..7dfd67f 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -27,6 +27,10 @@ static void qxl_flip(PCIQXLDevice *qxl, QXLRect *rect)
     uint8_t *dst = qxl->guest_primary.flipped;
     int len, i;
 
+    assert(rect->bottom >= 0);
+    assert(rect->bottom <= qxl->guest_primary.surface.height);
+    assert(rect->top >= 0);
+    assert(rect->top <= qxl->guest_primary.surface.height);
     src += (qxl->guest_primary.surface.height - rect->top - 1) *
         qxl->guest_primary.stride;
     dst += rect->top  * qxl->guest_primary.stride;
@@ -110,17 +114,39 @@ static void qxl_render_display_resized(PCIQXLDevice *qxl)
     dpy_resize(vga->ds);
 }
 
-void qxl_render_update(PCIQXLDevice *qxl)
+void qxl_spice_update_area_complete(PCIQXLDevice *qxl, QXLRect *dirty,
+    int n_dirty)
 {
     VGACommonState *vga = &qxl->vga;
-    QXLRect dirty[32], update;
     int i;
 
+    for (i = 0; i < n_dirty; i++) {
+        if (qemu_spice_rect_is_empty(dirty + i)) {
+            break;
+        }
+        if (qxl->guest_primary.flipped) {
+            qxl_flip(qxl, dirty + i);
+        }
+        dpy_update(vga->ds,
+                   dirty[i].left, dirty[i].top,
+                   dirty[i].right - dirty[i].left,
+                   dirty[i].bottom - dirty[i].top);
+    }
+}
+
+void qxl_render_update(PCIQXLDevice *qxl,
+                       SpiceAsyncMonitorScreenDump *async_screen_dump)
+{
+    QXLRect dirty[32], update;
+
     if (qxl->guest_primary.resized) {
         qxl_render_display_resized(qxl);
     }
-
     if (!qxl->guest_primary.commands) {
+        if (async_screen_dump) {
+            dprint(qxl, 2, "%s: no update required\n", __func__);
+            complete_spice_async_command(qxl, &async_screen_dump->base);
+        }
         return;
     }
     qxl->guest_primary.commands = 0;
@@ -131,20 +157,29 @@ void qxl_render_update(PCIQXLDevice *qxl)
     update.bottom = qxl->guest_primary.surface.height;
 
     memset(dirty, 0, sizeof(dirty));
+    if (async_screen_dump) {
+#if SPICE_INTERFACE_QXL_MINOR >= 2
+        dprint(qxl, 2, "A-SYNC update_area 0 (%d,%d,%d,%d)\n",
+               update.top, update.left, update.bottom, update.right);
+        async_screen_dump->n_dirty = 32;
+        async_screen_dump->dirty = g_malloc0(sizeof(QXLRect) *
+            async_screen_dump->n_dirty);
+        qxl_spice_update_area(qxl, 0, &update,
+                              async_screen_dump->dirty,
+                              async_screen_dump->n_dirty, 1,
+                              &async_screen_dump->base);
+        return;
+#else
+        fprintf(stderr, "%s: warning: fallback to sync, spice-server < 0.8.4\n",
+                __func__);
+#endif
+    }
+    dprint(qxl, 2, "SYNC update_area\n");
     qxl_spice_update_area(qxl, 0, &update,
-                          dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC, 0);
-
-    for (i = 0; i < ARRAY_SIZE(dirty); i++) {
-        if (qemu_spice_rect_is_empty(dirty+i)) {
-            break;
-        }
-        if (qxl->guest_primary.flipped) {
-            qxl_flip(qxl, dirty+i);
-        }
-        dpy_update(vga->ds,
-                   dirty[i].left, dirty[i].top,
-                   dirty[i].right - dirty[i].left,
-                   dirty[i].bottom - dirty[i].top);
+                          dirty, ARRAY_SIZE(dirty), 1, NULL);
+    qxl_spice_update_area_complete(qxl, dirty, ARRAY_SIZE(dirty));
+    if (async_screen_dump) {
+        complete_spice_async_command(qxl, &async_screen_dump->base);
     }
 }
 
diff --git a/hw/qxl.c b/hw/qxl.c
index 008f5f7..a3e89cd 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -145,18 +145,32 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
                            struct QXLRect *area, struct QXLRect *dirty_rects,
                            uint32_t num_dirty_rects,
                            uint32_t clear_dirty_region,
-                           qxl_async_io async, uint64_t cookie)
+                           SpiceAsyncCommand *async_command)
 {
-    if (async == QXL_SYNC) {
+    if (async_command == NULL) {
         qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area,
                         dirty_rects, num_dirty_rects, clear_dirty_region);
     } else {
+#if SPICE_INTERFACE_QXL_MINOR >= 2
+        if (num_dirty_rects > 0) {
+            spice_qxl_update_area_dirty_async(&qxl->ssd.qxl, surface_id, area,
+                                        dirty_rects, num_dirty_rects,
+                                        clear_dirty_region,
+                                        async_command->cookie);
+        } else {
+            spice_qxl_update_area_async(&qxl->ssd.qxl, surface_id, area,
+                                        clear_dirty_region,
+                                        async_command->cookie);
+        }
+#else
 #if SPICE_INTERFACE_QXL_MINOR >= 1
         spice_qxl_update_area_async(&qxl->ssd.qxl, surface_id, area,
-                                    clear_dirty_region, cookie);
+                                    clear_dirty_region,
+                                    async_command->cookie);
 #else
         abort();
-#endif
+#endif /* >= 1 */
+#endif /* >= 2 */
     }
 }
 
@@ -742,9 +756,11 @@ SpiceAsyncCommand *push_spice_async_command(PCIQXLDevice *qxl,
     assert(size >= sizeof(SpiceAsyncCommand));
     SpiceAsyncCommand *spice_async_command = g_malloc0(size);
 
+    qemu_mutex_lock(&qxl->async_lock);
     spice_async_command->cookie = qxl->async_next_cookie++;
-    spice_async_command->async_io = async_io;
+    spice_async_command->io = async_io;
     QLIST_INSERT_HEAD(&qxl->async_commands, spice_async_command, next);
+    qemu_mutex_unlock(&qxl->async_lock);
     dprint(qxl, 2, "allocated async cookie %"PRId64"\n",
            qxl->async_next_cookie - 1);
     return spice_async_command;
@@ -781,7 +797,7 @@ void complete_spice_async_command(PCIQXLDevice *qxl,
 static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
 {
     PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
-    uint32_t current_async;
+    uint32_t io;
     SpiceAsyncCommand *spice_async_command;
 
     spice_async_command = pop_spice_async_command(qxl, cookie);
@@ -790,9 +806,9 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
                        "pending operation, cookie=%ld\n", cookie);
         return;
     }
-    current_async = spice_async_command->async_io;
-    dprint(qxl, 2, "async_complete: %d (%ld) done\n", current_async, cookie);
-    switch (current_async) {
+    io = spice_async_command->io;
+    dprint(qxl, 2, "async_complete: %d (%ld) done\n", io, cookie);
+    switch (io) {
     case QXL_IO_CREATE_PRIMARY_ASYNC:
         qxl_create_guest_primary_complete(qxl);
         break;
@@ -806,7 +822,11 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
     }
     complete_spice_async_command(qxl, spice_async_command);
     g_free(spice_async_command);
-    qxl_send_events(qxl, QXL_INTERRUPT_IO_CMD);
+    if (io <= QXL_IO_RANGE_SIZE) {
+        dprint(qxl, 2, "%s: calling qxl_send_events (cookie %"PRIu64")\n",
+               __func__, cookie);
+        qxl_send_events(qxl, QXL_INTERRUPT_IO_CMD);
+    }
 }
 
 #endif
@@ -1162,17 +1182,27 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm)
     qxl_rom_set_dirty(d);
 }
 
+typedef SpiceAsyncCommand *(*SpiceAsyncCommandCreator)(PCIQXLDevice *qxl,
+    uint32_t io, uint32_t val);
+
+static SpiceAsyncCommand *create_async_command(PCIQXLDevice *qxl, uint32_t io,
+                                               uint32_t val)
+{
+    return push_spice_async_command(qxl, io, sizeof(SpiceAsyncCommand));
+}
+
 static void ioport_write(void *opaque, target_phys_addr_t addr,
                          uint64_t val, unsigned size)
 {
     PCIQXLDevice *d = opaque;
     uint32_t io_port = addr;
     qxl_async_io async = QXL_SYNC;
+    SpiceAsyncCommand *spice_async_command = NULL;
 #if SPICE_INTERFACE_QXL_MINOR >= 1
     uint32_t orig_io_port = io_port;
-    SpiceAsyncCommand *spice_async_command = NULL;
 #endif
     uint64_t cookie = 0;
+    SpiceAsyncCommandCreator creator = create_async_command;
 
     switch (io_port) {
     case QXL_IO_RESET:
@@ -1228,10 +1258,7 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
     case QXL_IO_FLUSH_SURFACES_ASYNC:
 async_common:
         async = QXL_ASYNC;
-        qemu_mutex_lock(&d->async_lock);
-        spice_async_command = push_spice_async_command(d, orig_io_port,
-                                                sizeof(SpiceAsyncCommand));
-        qemu_mutex_unlock(&d->async_lock);
+        spice_async_command = creator(d, orig_io_port, val);
         cookie = spice_async_command->cookie;
         dprint(d, 2, "start async %d (%"PRId64") cookie %"PRId64"\n", io_port,
                val, cookie);
@@ -1246,7 +1273,7 @@ async_common:
     {
         QXLRect update = d->ram->update_area;
         qxl_spice_update_area(d, d->ram->update_surface,
-                              &update, NULL, 0, 0, async, cookie);
+                              &update, NULL, 0, 0, spice_async_command);
         break;
     }
     case QXL_IO_NOTIFY_CMD:
@@ -1457,7 +1484,7 @@ static void qxl_hw_update(void *opaque)
         break;
     case QXL_MODE_COMPAT:
     case QXL_MODE_NATIVE:
-        qxl_render_update(qxl);
+        qxl_render_update(qxl, NULL);
         break;
     default:
         break;
@@ -1472,6 +1499,36 @@ static void qxl_hw_invalidate(void *opaque)
     vga->invalidate(vga);
 }
 
+static void screen_dump_complete(PCIQXLDevice *qxl,
+                                 SpiceAsyncCommand *spice_async_command)
+{
+    SpiceAsyncMonitorScreenDump *async_screen_dump =
+        (SpiceAsyncMonitorScreenDump *)spice_async_command;
+
+    dprint(qxl, 2, "%s: calling screen_dump_cb >%s<\n", __func__,
+           async_screen_dump->filename);
+    qxl_spice_update_area_complete(qxl, async_screen_dump->dirty,
+                                   async_screen_dump->n_dirty);
+    g_free(async_screen_dump->dirty);
+    ppm_save(async_screen_dump->filename, qxl->ssd.ds->surface);
+    g_free((void *)async_screen_dump->filename);
+    async_screen_dump->cb(async_screen_dump->cb_opaque, NULL);
+}
+
+static SpiceAsyncMonitorScreenDump *push_screen_dump(PCIQXLDevice *qxl,
+        const char *filename, MonitorCompletion *cb, void *cb_opaque)
+{
+    SpiceAsyncMonitorScreenDump *async_command =
+        (SpiceAsyncMonitorScreenDump *)
+            push_spice_async_command(qxl, MONITOR_UPDATE_AREA_ASYNC,
+                                 sizeof(SpiceAsyncMonitorScreenDump));
+    async_command->cb = cb;
+    async_command->cb_opaque = cb_opaque;
+    async_command->filename = g_strdup(filename);
+    async_command->base.completion = screen_dump_complete;
+    return async_command;
+}
+
 static void qxl_hw_screen_dump(void *opaque, const char *filename,
                                MonitorCompletion *cb, void *cb_opaque)
 {
@@ -1481,9 +1538,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename,
     switch (qxl->mode) {
     case QXL_MODE_COMPAT:
     case QXL_MODE_NATIVE:
-        qxl_render_update(qxl);
-        ppm_save(filename, qxl->ssd.ds->surface);
-        cb(cb_opaque, NULL);
+        qxl_render_update(qxl, push_screen_dump(qxl, filename, cb, cb_opaque));
         break;
     case QXL_MODE_VGA:
         vga->screen_dump(vga, filename, cb, cb_opaque);
diff --git a/hw/qxl.h b/hw/qxl.h
index 4c89e14..2375c03 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -17,6 +17,26 @@ enum qxl_mode {
 
 #define QXL_UNDEFINED_IO UINT32_MAX
 
+/* The monitor's screen_dump triggers a update_area
+ * command which must be done asynchroniously to prevent the following
+ * deadlock scenario when the spice client is also a monitor client and
+ * is running single threaded:
+ *
+ *  io thread                   worker thread                client
+ *
+ *   <---------------------------------------------------- screendump
+ *  do_screen_dump-> read------->
+ *                               flush, read
+ *                               client socket------------->
+ *
+ * To avoid adding these to the spice-protocol/spice/qxl_dev.h, they
+ * are defined here. */
+enum {
+    /* Must start larger then QXL_IO_RANGE_SIZE, but that is a moving
+     * target, so split in half. */
+    MONITOR_UPDATE_AREA_ASYNC = 0x80000000,
+};
+
 typedef struct PCIQXLDevice PCIQXLDevice;
 typedef struct SpiceAsyncCommand SpiceAsyncCommand;
 typedef void (*SpiceAsyncCommandCompletion)(PCIQXLDevice *,
@@ -27,11 +47,20 @@ typedef void (*SpiceAsyncCommandCompletion)(PCIQXLDevice *,
 struct SpiceAsyncCommand {
     uint64_t                       cookie;
     void                          *opaque;
-    uint32_t                       async_io;
+    uint32_t                       io;
     SpiceAsyncCommandCompletion    completion;
     QLIST_ENTRY(SpiceAsyncCommand) next;
 };
 
+typedef struct SpiceAsyncMonitorScreenDump {
+    SpiceAsyncCommand  base;
+    MonitorCompletion *cb;
+    void              *cb_opaque;
+    const char        *filename;
+    QXLRect           *dirty;
+    int                n_dirty;
+} SpiceAsyncMonitorScreenDump;
+
 struct PCIQXLDevice {
     PCIDevice          pci;
     SimpleSpiceDisplay ssd;
@@ -132,7 +161,7 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
                            struct QXLRect *area, struct QXLRect *dirty_rects,
                            uint32_t num_dirty_rects,
                            uint32_t clear_dirty_region,
-                           qxl_async_io async, uint64_t cookie);
+                           SpiceAsyncCommand *async_command);
 void qxl_spice_loadvm_commands(PCIQXLDevice *qxl, struct QXLCommandExt *ext,
                                uint32_t count);
 void qxl_spice_oom(PCIQXLDevice *qxl);
@@ -151,11 +180,14 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext);
 
 /* qxl-render.c */
 void qxl_render_resize(PCIQXLDevice *qxl);
-void qxl_render_update(PCIQXLDevice *qxl);
+void qxl_render_update(PCIQXLDevice *qxl,
+                       SpiceAsyncMonitorScreenDump *async_screen_dump);
 void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext);
 #if SPICE_INTERFACE_QXL_MINOR >= 1
 void qxl_spice_update_area_async(PCIQXLDevice *qxl, uint32_t surface_id,
                                  struct QXLRect *area,
                                  uint32_t clear_dirty_region,
                                  int is_vga);
+void qxl_spice_update_area_complete(PCIQXLDevice *qxl, QXLRect *dirty,
+    int n_dirty);
 #endif
-- 
1.7.7

Re: [Qemu-devel] [PATCH 5/5] qxl: support async monitor screen dump

[Gerd Hoffmann]

On 10/24/11 14:02, Alon Levy wrote:
> Split qxl_spice_update_area_complete from qxl_render_update, use
> SPICE_INTERFACE_QXL_MINOR 2 introduced spice_qxl_update_area_dirty_async
> to retrive the dirty rectangles asyncronously (the previous
> spice_qxl_update_area_async did not accept a dirty rectangles array).
> 
> Introduce SpiceAsyncMonitorScreenDump for a screen_dump.

That one conflicts with the screendump/SDL fixes pushed to the spice.v44
branch.  Have you seen the mail?  Had you time to look at the patches?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 5/5] qxl: support async monitor screen dump

[Alon Levy]

On Mon, Oct 24, 2011 at 05:29:47PM +0200, Gerd Hoffmann wrote:
> On 10/24/11 14:02, Alon Levy wrote:
> > Split qxl_spice_update_area_complete from qxl_render_update, use
> > SPICE_INTERFACE_QXL_MINOR 2 introduced spice_qxl_update_area_dirty_async
> > to retrive the dirty rectangles asyncronously (the previous
> > spice_qxl_update_area_async did not accept a dirty rectangles array).
> > 
> > Introduce SpiceAsyncMonitorScreenDump for a screen_dump.
> 
> That one conflicts with the screendump/SDL fixes pushed to the spice.v44
> branch.  Have you seen the mail?  Had you time to look at the patches?

Yes and no. I will.

> 
> cheers,
>   Gerd
>