[Qemu-devel] [RFC/RFT PATCH 0/2] spice-display locking fixes (cursors)?

[Qemu-devel] [RFC/RFT PATCH 0/2] spice-display locking fixes (cursors)?

[Paolo Bonzini]

The first issue was found by Coverity and should be trivial.  The second
however made me wonder how to test the code and whether it has ever
worked, because in theory it should be an instant deadlock whenever
qemu_spice_cursor_refresh_bh is called.

So I'm looking for help.  In fact, the changes are not tested beyond
compilation.

Paolo

Paolo Bonzini (2):
  spice-display: access ptr_x/ptr_y under Mutex
  spice-display: fix qemu_spice_cursor_refresh_bh locking

 ui/spice-display.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/2] spice-display: access ptr_x/ptr_y under Mutex

[Paolo Bonzini]

The OpenGL-enabled SPICE code was not accessing the cursor position
under the SimpleSpiceDisplay lock.  Fix this.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 ui/spice-display.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index fe734821dd..46df673cd7 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -976,8 +976,10 @@ static void qemu_spice_gl_cursor_position(DisplayChangeListener *dcl,
 {
     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
 
+    qemu_mutex_lock(&ssd->lock);
     ssd->ptr_x = pos_x;
     ssd->ptr_y = pos_y;
+    qemu_mutex_unlock(&ssd->lock);
 }
 
 static void qemu_spice_gl_release_dmabuf(DisplayChangeListener *dcl,
@@ -1055,10 +1057,15 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
     }
 
     if (render_cursor) {
+        int x, y;
+        qemu_mutex_lock(&ssd->lock);
+        x = ssd->ptr_x;
+        y = ssd->ptr_y;
+        qemu_mutex_unlock(&ssd->lock);
         egl_texture_blit(ssd->gls, &ssd->blit_fb, &ssd->guest_fb,
                          !y_0_top);
         egl_texture_blend(ssd->gls, &ssd->blit_fb, &ssd->cursor_fb,
-                          !y_0_top, ssd->ptr_x, ssd->ptr_y);
+                          !y_0_top, x, y);
         glFlush();
     }
 
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] spice-display: fix qemu_spice_cursor_refresh_bh locking

[Paolo Bonzini]

spice-display should not call the ui/console.c functions dpy_cursor_define
and dpy_moues_set with the SimpleSpiceDisplay lock taken.  That will cause
a deadlock, because the DisplayChangeListener callbacks will take the lock
again.  It is also in general a bad idea to invoke generic callbacks with a
lock taken, because it can cause AB-BA deadlocks in the long run.  The only
thing that requires care is that the cursor may disappear as soon as the
mutex is released, so you need an extra cursor_get/cursor_put pair.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 ui/spice-display.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 46df673cd7..f1d341091a 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -450,29 +450,35 @@ void qemu_spice_display_switch(SimpleSpiceDisplay *ssd,
     qemu_mutex_unlock(&ssd->lock);
 }
 
-static void qemu_spice_cursor_refresh_unlocked(SimpleSpiceDisplay *ssd)
+void qemu_spice_cursor_refresh_bh(void *opaque)
 {
+    SimpleSpiceDisplay *ssd = opaque;
+
+    qemu_mutex_lock(&ssd->lock);
     if (ssd->cursor) {
+        QEMUCursor *c = ssd->cursor;
         assert(ssd->dcl.con);
+        cursor_get(c);
+        qemu_mutex_unlock(&ssd->lock);
         dpy_cursor_define(ssd->dcl.con, ssd->cursor);
+        qemu_mutex_lock(&ssd->lock);
+        cursor_put(c);
     }
+
     if (ssd->mouse_x != -1 && ssd->mouse_y != -1) {
+        int x, y;
         assert(ssd->dcl.con);
-        dpy_mouse_set(ssd->dcl.con, ssd->mouse_x, ssd->mouse_y, 1);
+        x = ssd->mouse_x;
+        y = ssd->mouse_y;
         ssd->mouse_x = -1;
         ssd->mouse_y = -1;
+        qemu_mutex_unlock(&ssd->lock);
+        dpy_mouse_set(ssd->dcl.con, x, y, 1);
+    } else {
+        qemu_mutex_unlock(&ssd->lock);
     }
 }
 
-void qemu_spice_cursor_refresh_bh(void *opaque)
-{
-    SimpleSpiceDisplay *ssd = opaque;
-
-    qemu_mutex_lock(&ssd->lock);
-    qemu_spice_cursor_refresh_unlocked(ssd);
-    qemu_mutex_unlock(&ssd->lock);
-}
-
 void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
 {
     graphic_hw_update(ssd->dcl.con);
-- 
2.17.1

Re: [Qemu-devel] [RFC/RFT PATCH 0/2] spice-display locking fixes (cursors)?

[Paolo Bonzini]

On 20/07/2018 08:31, Paolo Bonzini wrote:
> The first issue was found by Coverity and should be trivial.  The second
> however made me wonder how to test the code and whether it has ever
> worked, because in theory it should be an instant deadlock whenever
> qemu_spice_cursor_refresh_bh is called.
> 
> So I'm looking for help.  In fact, the changes are not tested beyond
> compilation.
> 
> Paolo
> 
> Paolo Bonzini (2):
>   spice-display: access ptr_x/ptr_y under Mutex
>   spice-display: fix qemu_spice_cursor_refresh_bh locking
> 
>  ui/spice-display.c | 37 +++++++++++++++++++++++++------------
>  1 file changed, 25 insertions(+), 12 deletions(-)
> 

Ping?

Paolo

Re: [Qemu-devel] [RFC/RFT PATCH 0/2] spice-display locking fixes (cursors)?

[Marc-André Lureau]

On Mon, Aug 20, 2018 at 5:29 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 20/07/2018 08:31, Paolo Bonzini wrote:
> > The first issue was found by Coverity and should be trivial.  The second
> > however made me wonder how to test the code and whether it has ever
> > worked, because in theory it should be an instant deadlock whenever
> > qemu_spice_cursor_refresh_bh is called.
> >
> > So I'm looking for help.  In fact, the changes are not tested beyond
> > compilation.
> >
> > Paolo
> >
> > Paolo Bonzini (2):
> >   spice-display: access ptr_x/ptr_y under Mutex
> >   spice-display: fix qemu_spice_cursor_refresh_bh locking
> >
> >  ui/spice-display.c | 37 +++++++++++++++++++++++++------------
> >  1 file changed, 25 insertions(+), 12 deletions(-)
> >
>
> Ping?
>


tested &
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



-- 
Marc-André Lureau

Re: [Qemu-devel] [RFC/RFT PATCH 0/2] spice-display locking fixes (cursors)?

[Gerd Hoffmann]

On Fri, Jul 20, 2018 at 08:31:07AM +0200, Paolo Bonzini wrote:
> The first issue was found by Coverity and should be trivial.  The second
> however made me wonder how to test the code and whether it has ever
> worked, because in theory it should be an instant deadlock whenever
> qemu_spice_cursor_refresh_bh is called.

Added to ui queue.

thanks,
  Gerd