1. Patchew
  2. QEMU
  3. [Qemu-devel] [PULL 0/7] cocoa queue
  4. View patch

[Qemu-devel] [PULL 1/7] ui/cocoa: Ensure we have the iothread lock when calling into QEMU

Peter Maydell posted 7 patches 6 years, 8 months ago
[Qemu-devel] [PULL 1/7] ui/cocoa: Ensure we have the iothread lock when calling into QEMU
Posted by Peter Maydell 6 years, 8 months ago 
The Cocoa UI should run on the main thread; this is enforced
in OSX Mojave. In order to be able to run on the main thread,
we need to make sure we hold the iothread lock whenever we
call into various QEMU UI midlayer functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Tested-by: Roman Bolshakov <r.bolshakov@yadro.com>
Message-id: 20190225102433.22401-2-peter.maydell@linaro.org
Message-id: 20190214102816.3393-2-peter.maydell@linaro.org
---
 ui/cocoa.m | 91 ++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 65 insertions(+), 26 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index e2567d69466..f1171c48654 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -129,6 +129,21 @@ bool stretch_video;
 NSTextField *pauseLabel;
 NSArray * supportedImageFileTypes;
 
+// Utility function to run specified code block with iothread lock held
+typedef void (^CodeBlock)(void);
+
+static void with_iothread_lock(CodeBlock block)
+{
+    bool locked = qemu_mutex_iothread_locked();
+    if (!locked) {
+        qemu_mutex_lock_iothread();
+    }
+    block();
+    if (!locked) {
+        qemu_mutex_unlock_iothread();
+    }
+}
+
 // Mac to QKeyCode conversion
 const int mac_to_qkeycode_map[] = {
     [kVK_ANSI_A] = Q_KEY_CODE_A,
@@ -306,6 +321,7 @@ static void handleAnyDeviceErrors(Error * err)
 - (void) toggleFullScreen:(id)sender;
 - (void) handleMonitorInput:(NSEvent *)event;
 - (void) handleEvent:(NSEvent *)event;
+- (void) handleEventLocked:(NSEvent *)event;
 - (void) setAbsoluteEnabled:(BOOL)tIsAbsoluteEnabled;
 /* The state surrounding mouse grabbing is potentially confusing.
  * isAbsoluteEnabled tracks qemu_input_is_absolute() [ie "is the emulated
@@ -649,8 +665,14 @@ QemuCocoaView *cocoaView;
 
 - (void) handleEvent:(NSEvent *)event
 {
-    COCOA_DEBUG("QemuCocoaView: handleEvent\n");
+    with_iothread_lock(^{
+        [self handleEventLocked:event];
+    });
+}
 
+- (void) handleEventLocked:(NSEvent *)event
+{
+    COCOA_DEBUG("QemuCocoaView: handleEvent\n");
     int buttons = 0;
     int keycode = 0;
     bool mouse_event = false;
@@ -945,15 +967,18 @@ QemuCocoaView *cocoaView;
  */
 - (void) raiseAllKeys
 {
-    int index;
     const int max_index = ARRAY_SIZE(modifiers_state);
 
-   for (index = 0; index < max_index; index++) {
-       if (modifiers_state[index]) {
-           modifiers_state[index] = 0;
-           qemu_input_event_send_key_qcode(dcl->con, index, false);
-       }
-   }
+    with_iothread_lock(^{
+        int index;
+
+        for (index = 0; index < max_index; index++) {
+            if (modifiers_state[index]) {
+                modifiers_state[index] = 0;
+                qemu_input_event_send_key_qcode(dcl->con, index, false);
+            }
+        }
+    });
 }
 @end
 
@@ -1178,7 +1203,9 @@ QemuCocoaView *cocoaView;
 /* Pause the guest */
 - (void)pauseQEMU:(id)sender
 {
-    qmp_stop(NULL);
+    with_iothread_lock(^{
+        qmp_stop(NULL);
+    });
     [sender setEnabled: NO];
     [[[sender menu] itemWithTitle: @"Resume"] setEnabled: YES];
     [self displayPause];
@@ -1187,7 +1214,9 @@ QemuCocoaView *cocoaView;
 /* Resume running the guest operating system */
 - (void)resumeQEMU:(id) sender
 {
-    qmp_cont(NULL);
+    with_iothread_lock(^{
+        qmp_cont(NULL);
+    });
     [sender setEnabled: NO];
     [[[sender menu] itemWithTitle: @"Pause"] setEnabled: YES];
     [self removePause];
@@ -1215,13 +1244,17 @@ QemuCocoaView *cocoaView;
 /* Restarts QEMU */
 - (void)restartQEMU:(id)sender
 {
-    qmp_system_reset(NULL);
+    with_iothread_lock(^{
+        qmp_system_reset(NULL);
+    });
 }
 
 /* Powers down QEMU */
 - (void)powerDownQEMU:(id)sender
 {
-    qmp_system_powerdown(NULL);
+    with_iothread_lock(^{
+        qmp_system_powerdown(NULL);
+    });
 }
 
 /* Ejects the media.
@@ -1237,9 +1270,11 @@ QemuCocoaView *cocoaView;
         return;
     }
 
-    Error *err = NULL;
-    qmp_eject(true, [drive cStringUsingEncoding: NSASCIIStringEncoding],
-              false, NULL, false, false, &err);
+    __block Error *err = NULL;
+    with_iothread_lock(^{
+        qmp_eject(true, [drive cStringUsingEncoding: NSASCIIStringEncoding],
+                  false, NULL, false, false, &err);
+    });
     handleAnyDeviceErrors(err);
 }
 
@@ -1271,16 +1306,18 @@ QemuCocoaView *cocoaView;
             return;
         }
 
-        Error *err = NULL;
-        qmp_blockdev_change_medium(true,
-                                   [drive cStringUsingEncoding:
-                                          NSASCIIStringEncoding],
-                                   false, NULL,
-                                   [file cStringUsingEncoding:
-                                         NSASCIIStringEncoding],
-                                   true, "raw",
-                                   false, 0,
-                                   &err);
+        __block Error *err = NULL;
+        with_iothread_lock(^{
+            qmp_blockdev_change_medium(true,
+                                       [drive cStringUsingEncoding:
+                                                  NSASCIIStringEncoding],
+                                       false, NULL,
+                                       [file cStringUsingEncoding:
+                                                 NSASCIIStringEncoding],
+                                       true, "raw",
+                                       false, 0,
+                                       &err);
+        });
         handleAnyDeviceErrors(err);
     }
 }
@@ -1419,7 +1456,9 @@ QemuCocoaView *cocoaView;
     // get the throttle percentage
     throttle_pct = [sender tag];
 
-    cpu_throttle_set(throttle_pct);
+    with_iothread_lock(^{
+        cpu_throttle_set(throttle_pct);
+    });
     COCOA_DEBUG("cpu throttling at %d%c\n", cpu_throttle_get_percentage(), '%');
 }
 
-- 
2.20.1
Re: [Qemu-devel] [PULL 1/7] ui/cocoa: Ensure we have the iothread lock when calling into QEMU
Posted by Philippe Mathieu-Daudé 6 years, 8 months ago 
Hi Peter,

On 3/4/19 5:49 PM, Peter Maydell wrote:
> The Cocoa UI should run on the main thread; this is enforced
> in OSX Mojave. In order to be able to run on the main thread,
> we need to make sure we hold the iothread lock whenever we
> call into various QEMU UI midlayer functions.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
> Tested-by: Roman Bolshakov <r.bolshakov@yadro.com>

I got surprised by your 2 Message-id lines (same with other patches from
this pull request):

> Message-id: 20190225102433.22401-2-peter.maydell@linaro.org

^ v3

> Message-id: 20190214102816.3393-2-peter.maydell@linaro.org

^ v2

> ---
>  ui/cocoa.m | 91 ++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 65 insertions(+), 26 deletions(-)
> 
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index e2567d69466..f1171c48654 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -129,6 +129,21 @@ bool stretch_video;
>  NSTextField *pauseLabel;
>  NSArray * supportedImageFileTypes;
>  
> +// Utility function to run specified code block with iothread lock held
> +typedef void (^CodeBlock)(void);
> +
> +static void with_iothread_lock(CodeBlock block)
> +{
> +    bool locked = qemu_mutex_iothread_locked();
> +    if (!locked) {
> +        qemu_mutex_lock_iothread();
> +    }
> +    block();
> +    if (!locked) {
> +        qemu_mutex_unlock_iothread();
> +    }
> +}
> +
>  // Mac to QKeyCode conversion
>  const int mac_to_qkeycode_map[] = {
>      [kVK_ANSI_A] = Q_KEY_CODE_A,
> @@ -306,6 +321,7 @@ static void handleAnyDeviceErrors(Error * err)
>  - (void) toggleFullScreen:(id)sender;
>  - (void) handleMonitorInput:(NSEvent *)event;
>  - (void) handleEvent:(NSEvent *)event;
> +- (void) handleEventLocked:(NSEvent *)event;
>  - (void) setAbsoluteEnabled:(BOOL)tIsAbsoluteEnabled;
>  /* The state surrounding mouse grabbing is potentially confusing.
>   * isAbsoluteEnabled tracks qemu_input_is_absolute() [ie "is the emulated
> @@ -649,8 +665,14 @@ QemuCocoaView *cocoaView;
>  
>  - (void) handleEvent:(NSEvent *)event
>  {
> -    COCOA_DEBUG("QemuCocoaView: handleEvent\n");
> +    with_iothread_lock(^{
> +        [self handleEventLocked:event];
> +    });
> +}
>  
> +- (void) handleEventLocked:(NSEvent *)event
> +{
> +    COCOA_DEBUG("QemuCocoaView: handleEvent\n");
>      int buttons = 0;
>      int keycode = 0;
>      bool mouse_event = false;
> @@ -945,15 +967,18 @@ QemuCocoaView *cocoaView;
>   */
>  - (void) raiseAllKeys
>  {
> -    int index;
>      const int max_index = ARRAY_SIZE(modifiers_state);
>  
> -   for (index = 0; index < max_index; index++) {
> -       if (modifiers_state[index]) {
> -           modifiers_state[index] = 0;
> -           qemu_input_event_send_key_qcode(dcl->con, index, false);
> -       }
> -   }
> +    with_iothread_lock(^{
> +        int index;
> +
> +        for (index = 0; index < max_index; index++) {
> +            if (modifiers_state[index]) {
> +                modifiers_state[index] = 0;
> +                qemu_input_event_send_key_qcode(dcl->con, index, false);
> +            }
> +        }
> +    });
>  }
>  @end
>  
> @@ -1178,7 +1203,9 @@ QemuCocoaView *cocoaView;
>  /* Pause the guest */
>  - (void)pauseQEMU:(id)sender
>  {
> -    qmp_stop(NULL);
> +    with_iothread_lock(^{
> +        qmp_stop(NULL);
> +    });
>      [sender setEnabled: NO];
>      [[[sender menu] itemWithTitle: @"Resume"] setEnabled: YES];
>      [self displayPause];
> @@ -1187,7 +1214,9 @@ QemuCocoaView *cocoaView;
>  /* Resume running the guest operating system */
>  - (void)resumeQEMU:(id) sender
>  {
> -    qmp_cont(NULL);
> +    with_iothread_lock(^{
> +        qmp_cont(NULL);
> +    });
>      [sender setEnabled: NO];
>      [[[sender menu] itemWithTitle: @"Pause"] setEnabled: YES];
>      [self removePause];
> @@ -1215,13 +1244,17 @@ QemuCocoaView *cocoaView;
>  /* Restarts QEMU */
>  - (void)restartQEMU:(id)sender
>  {
> -    qmp_system_reset(NULL);
> +    with_iothread_lock(^{
> +        qmp_system_reset(NULL);
> +    });
>  }
>  
>  /* Powers down QEMU */
>  - (void)powerDownQEMU:(id)sender
>  {
> -    qmp_system_powerdown(NULL);
> +    with_iothread_lock(^{
> +        qmp_system_powerdown(NULL);
> +    });
>  }
>  
>  /* Ejects the media.
> @@ -1237,9 +1270,11 @@ QemuCocoaView *cocoaView;
>          return;
>      }
>  
> -    Error *err = NULL;
> -    qmp_eject(true, [drive cStringUsingEncoding: NSASCIIStringEncoding],
> -              false, NULL, false, false, &err);
> +    __block Error *err = NULL;
> +    with_iothread_lock(^{
> +        qmp_eject(true, [drive cStringUsingEncoding: NSASCIIStringEncoding],
> +                  false, NULL, false, false, &err);
> +    });
>      handleAnyDeviceErrors(err);
>  }
>  
> @@ -1271,16 +1306,18 @@ QemuCocoaView *cocoaView;
>              return;
>          }
>  
> -        Error *err = NULL;
> -        qmp_blockdev_change_medium(true,
> -                                   [drive cStringUsingEncoding:
> -                                          NSASCIIStringEncoding],
> -                                   false, NULL,
> -                                   [file cStringUsingEncoding:
> -                                         NSASCIIStringEncoding],
> -                                   true, "raw",
> -                                   false, 0,
> -                                   &err);
> +        __block Error *err = NULL;
> +        with_iothread_lock(^{
> +            qmp_blockdev_change_medium(true,
> +                                       [drive cStringUsingEncoding:
> +                                                  NSASCIIStringEncoding],
> +                                       false, NULL,
> +                                       [file cStringUsingEncoding:
> +                                                 NSASCIIStringEncoding],
> +                                       true, "raw",
> +                                       false, 0,
> +                                       &err);
> +        });
>          handleAnyDeviceErrors(err);
>      }
>  }
> @@ -1419,7 +1456,9 @@ QemuCocoaView *cocoaView;
>      // get the throttle percentage
>      throttle_pct = [sender tag];
>  
> -    cpu_throttle_set(throttle_pct);
> +    with_iothread_lock(^{
> +        cpu_throttle_set(throttle_pct);
> +    });
>      COCOA_DEBUG("cpu throttling at %d%c\n", cpu_throttle_get_percentage(), '%');
>  }
>  
>
Re: [Qemu-devel] [PULL 1/7] ui/cocoa: Ensure we have the iothread lock when calling into QEMU
Posted by Peter Maydell 6 years, 8 months ago 
On Mon, 4 Mar 2019 at 17:45, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi Peter,
>
> On 3/4/19 5:49 PM, Peter Maydell wrote:
> > The Cocoa UI should run on the main thread; this is enforced
> > in OSX Mojave. In order to be able to run on the main thread,
> > we need to make sure we hold the iothread lock whenever we
> > call into various QEMU UI midlayer functions.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > Tested-by: Roman Bolshakov <r.bolshakov@yadro.com>
>
> I got surprised by your 2 Message-id lines (same with other patches from
> this pull request):
>
> > Message-id: 20190225102433.22401-2-peter.maydell@linaro.org
>
> ^ v3
>
> > Message-id: 20190214102816.3393-2-peter.maydell@linaro.org

I think the 'patches' program tends to add an extra
Message-id: line even if one was already present. I treat
this header as pure noise and do not attempt to either add
it or remove it by hand.

thanks
-- PMM

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.