diff mbox

[2/2] qemu-timer: move timeBeginPeriod/timeEndPeriod to os-win32

Message ID 1361367811-13288-3-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Feb. 20, 2013, 1:43 p.m. UTC 
These are needed for any of the Win32 alarm timer implementations.
They are not tied to mmtimer exclusively.

Jacob tested this patch with both mmtimer and Win32 timers.

Cc: qemu-stable@nongnu.org
Tested-by: Jacob Kroon <jacob.kroon@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        Jacob, this is the same as "patch 1" you tested.

 os-win32.c   | 10 ++++++++++
 qemu-timer.c | 26 ++++++--------------------
 2 files changed, 16 insertions(+), 20 deletions(-)

Comments

Jacob Kroon Feb. 20, 2013, 8:46 p.m. UTC | #1 
Paolo,

Just a heads up, I tried the patched qemu (1+2+3) on my laptop at
home, which is running Windows 7 64-bit. I'm seeing qemu "lockups"
appearing randomly.. Will try to debug it.
On the other hand, plain vanilla 1.4.0 in Windows 7 seems to run fine
with my VxWorks image..

Regards
Jacob

On Wed, Feb 20, 2013 at 2:43 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> These are needed for any of the Win32 alarm timer implementations.
> They are not tied to mmtimer exclusively.
>
> Jacob tested this patch with both mmtimer and Win32 timers.
>
> Cc: qemu-stable@nongnu.org
> Tested-by: Jacob Kroon <jacob.kroon@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>         Jacob, this is the same as "patch 1" you tested.
>
>  os-win32.c   | 10 ++++++++++
>  qemu-timer.c | 26 ++++++--------------------
>  2 files changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/os-win32.c b/os-win32.c
> index 3d43604..a0740ef 100644
> --- a/os-win32.c
> +++ b/os-win32.c
> @@ -23,6 +23,7 @@
>   * THE SOFTWARE.
>   */
>  #include <windows.h>
> +#include <mmsystem.h>
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <signal.h>
> @@ -67,9 +67,19 @@ static BOOL WINAPI qemu_ctrl_handler(DWORD type)
>      return TRUE;
>  }
>
> +static TIMECAPS mm_tc;
> +
> +static void os_undo_timer_resolution(void)
> +{
> +    timeEndPeriod(mm_tc.wPeriodMin);
> +}
> +
>  void os_setup_early_signal_handling(void)
>  {
>      SetConsoleCtrlHandler(qemu_ctrl_handler, TRUE);
> +    timeGetDevCaps(&mm_tc, sizeof(mm_tc));
> +    timeBeginPeriod(mm_tc.wPeriodMin);
> +    atexit(os_undo_timer_resolution);
>  }
>
>  /* Look for support files in the same directory as the executable.  */
> diff --git a/qemu-timer.c b/qemu-timer.c
> index 8fb5c75..50109a1 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -623,29 +622,15 @@ static void CALLBACK mm_alarm_handler(UINT uTimerID, UINT uMsg,
>
>  static int mm_start_timer(struct qemu_alarm_timer *t)
>  {
>      timeGetDevCaps(&mm_tc, sizeof(mm_tc));
> -
> -    timeBeginPeriod(mm_tc.wPeriodMin);
> -
> -    mm_timer = timeSetEvent(mm_tc.wPeriodMin,   /* interval (ms) */
> -                            mm_tc.wPeriodMin,   /* resolution */
> -                            mm_alarm_handler,   /* function */
> -                            (DWORD_PTR)t,       /* parameter */
> -                            TIME_ONESHOT | TIME_CALLBACK_FUNCTION);
> -
> -    if (!mm_timer) {
> -        fprintf(stderr, "Failed to initialize win32 alarm timer\n");
> -        timeEndPeriod(mm_tc.wPeriodMin);
> -        return -1;
> -    }
> -
>      return 0;
>  }
>
>  static void mm_stop_timer(struct qemu_alarm_timer *t)
>  {
> -    timeKillEvent(mm_timer);
> -    timeEndPeriod(mm_tc.wPeriodMin);
> +    if (mm_timer) {
> +        timeKillEvent(mm_timer);
> +    }
>  }
>
>  static void mm_rearm_timer(struct qemu_alarm_timer *t, int64_t delta)
> @@ -657,7 +641,9 @@ static void mm_rearm_timer(struct qemu_alarm_timer *t, int64_t delta)
>          nearest_delta_ms = mm_tc.wPeriodMax;
>      }
>
> -    timeKillEvent(mm_timer);
> +    if (mm_timer) {
> +        timeKillEvent(mm_timer);
> +    }
>      mm_timer = timeSetEvent((UINT)nearest_delta_ms,
>                              mm_tc.wPeriodMin,
>                              mm_alarm_handler,
> --
> 1.8.1.2
>
Jacob Kroon Feb. 20, 2013, 11:49 p.m. UTC | #2 
On Wed, Feb 20, 2013 at 9:46 PM, Jacob Kroon <jacob.kroon@gmail.com> wrote:
> Paolo,
>
> Just a heads up, I tried the patched qemu (1+2+3) on my laptop at
> home, which is running Windows 7 64-bit. I'm seeing qemu "lockups"
> appearing randomly.. Will try to debug it.
> On the other hand, plain vanilla 1.4.0 in Windows 7 seems to run fine
> with my VxWorks image..

So the problem seems to be related to wether I pass --enable-debug to
qemu configure script.
With debug enabled it runs fine in Windows 7 aswell. If I leave it out
I get what appears to be a lockup
a couple of seconds into the guest boot process. Best I could do for
now was to attach to the process
with gdb and get a backtrace:

(gdb) bt
#0  0x76fc000d in ntdll!LdrFindResource_U ()
   from /cygdrive/c/Windows/SysWOW64/ntdll.dll
#1  0x7704f896 in ntdll!RtlQueryTimeZoneInformation ()
   from /cygdrive/c/Windows/SysWOW64/ntdll.dll
#2  0x6ce2c99b in ?? ()
#3  0x00000000 in ?? ()

> On Wed, Feb 20, 2013 at 2:43 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> These are needed for any of the Win32 alarm timer implementations.
>> They are not tied to mmtimer exclusively.
>>
>> Jacob tested this patch with both mmtimer and Win32 timers.
>>
>> Cc: qemu-stable@nongnu.org
>> Tested-by: Jacob Kroon <jacob.kroon@gmail.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>         Jacob, this is the same as "patch 1" you tested.
>>
>>  os-win32.c   | 10 ++++++++++
>>  qemu-timer.c | 26 ++++++--------------------
>>  2 files changed, 16 insertions(+), 20 deletions(-)
>>
>> diff --git a/os-win32.c b/os-win32.c
>> index 3d43604..a0740ef 100644
>> --- a/os-win32.c
>> +++ b/os-win32.c
>> @@ -23,6 +23,7 @@
>>   * THE SOFTWARE.
>>   */
>>  #include <windows.h>
>> +#include <mmsystem.h>
>>  #include <unistd.h>
>>  #include <fcntl.h>
>>  #include <signal.h>
>> @@ -67,9 +67,19 @@ static BOOL WINAPI qemu_ctrl_handler(DWORD type)
>>      return TRUE;
>>  }
>>
>> +static TIMECAPS mm_tc;
>> +
>> +static void os_undo_timer_resolution(void)
>> +{
>> +    timeEndPeriod(mm_tc.wPeriodMin);
>> +}
>> +
>>  void os_setup_early_signal_handling(void)
>>  {
>>      SetConsoleCtrlHandler(qemu_ctrl_handler, TRUE);
>> +    timeGetDevCaps(&mm_tc, sizeof(mm_tc));
>> +    timeBeginPeriod(mm_tc.wPeriodMin);
>> +    atexit(os_undo_timer_resolution);
>>  }
>>
>>  /* Look for support files in the same directory as the executable.  */
>> diff --git a/qemu-timer.c b/qemu-timer.c
>> index 8fb5c75..50109a1 100644
>> --- a/qemu-timer.c
>> +++ b/qemu-timer.c
>> @@ -623,29 +622,15 @@ static void CALLBACK mm_alarm_handler(UINT uTimerID, UINT uMsg,
>>
>>  static int mm_start_timer(struct qemu_alarm_timer *t)
>>  {
>>      timeGetDevCaps(&mm_tc, sizeof(mm_tc));
>> -
>> -    timeBeginPeriod(mm_tc.wPeriodMin);
>> -
>> -    mm_timer = timeSetEvent(mm_tc.wPeriodMin,   /* interval (ms) */
>> -                            mm_tc.wPeriodMin,   /* resolution */
>> -                            mm_alarm_handler,   /* function */
>> -                            (DWORD_PTR)t,       /* parameter */
>> -                            TIME_ONESHOT | TIME_CALLBACK_FUNCTION);
>> -
>> -    if (!mm_timer) {
>> -        fprintf(stderr, "Failed to initialize win32 alarm timer\n");
>> -        timeEndPeriod(mm_tc.wPeriodMin);
>> -        return -1;
>> -    }
>> -
>>      return 0;
>>  }
>>
>>  static void mm_stop_timer(struct qemu_alarm_timer *t)
>>  {
>> -    timeKillEvent(mm_timer);
>> -    timeEndPeriod(mm_tc.wPeriodMin);
>> +    if (mm_timer) {
>> +        timeKillEvent(mm_timer);
>> +    }
>>  }
>>
>>  static void mm_rearm_timer(struct qemu_alarm_timer *t, int64_t delta)
>> @@ -657,7 +641,9 @@ static void mm_rearm_timer(struct qemu_alarm_timer *t, int64_t delta)
>>          nearest_delta_ms = mm_tc.wPeriodMax;
>>      }
>>
>> -    timeKillEvent(mm_timer);
>> +    if (mm_timer) {
>> +        timeKillEvent(mm_timer);
>> +    }
>>      mm_timer = timeSetEvent((UINT)nearest_delta_ms,
>>                              mm_tc.wPeriodMin,
>>                              mm_alarm_handler,
>> --
>> 1.8.1.2
>>
>
>
>
> --
>   -- Jacob
Stefan Weil Feb. 21, 2013, 6:25 a.m. UTC | #3 
Am 21.02.2013 00:49, schrieb Jacob Kroon:
> On Wed, Feb 20, 2013 at 9:46 PM, Jacob Kroon <jacob.kroon@gmail.com> wrote:
>> Paolo,
>>
>> Just a heads up, I tried the patched qemu (1+2+3) on my laptop at
>> home, which is running Windows 7 64-bit. I'm seeing qemu "lockups"
>> appearing randomly.. Will try to debug it.
>> On the other hand, plain vanilla 1.4.0 in Windows 7 seems to run fine
>> with my VxWorks image..
> So the problem seems to be related to wether I pass --enable-debug to
> qemu configure script.
> With debug enabled it runs fine in Windows 7 aswell. If I leave it out
> I get what appears to be a lockup
> a couple of seconds into the guest boot process. Best I could do for
> now was to attach to the process
> with gdb and get a backtrace:
>
> (gdb) bt
> #0  0x76fc000d in ntdll!LdrFindResource_U ()
>    from /cygdrive/c/Windows/SysWOW64/ntdll.dll
> #1  0x7704f896 in ntdll!RtlQueryTimeZoneInformation ()
>    from /cygdrive/c/Windows/SysWOW64/ntdll.dll
> #2  0x6ce2c99b in ?? ()
> #3  0x00000000 in ?? ()

The backtrace is usually better for QEMU with TCG interpreter. Run

     configure --enable-tcg-interpreter

The resulting binaries are much slower, but no longer create
code on the fly, so you get normal backtraces.

I also had to remove -fstack-protector-all (in file configure) to
get good backtraces for w32/w64.

Regards,

Stefan
Jacob Kroon Feb. 21, 2013, 1:53 p.m. UTC | #4 
Hi Stefan,

On Thu, Feb 21, 2013 at 7:25 AM, Stefan Weil <sw@weilnetz.de> wrote:
> Am 21.02.2013 00:49, schrieb Jacob Kroon:
>> On Wed, Feb 20, 2013 at 9:46 PM, Jacob Kroon <jacob.kroon@gmail.com> wrote:
>>> Paolo,
>>>
>>> Just a heads up, I tried the patched qemu (1+2+3) on my laptop at
>>> home, which is running Windows 7 64-bit. I'm seeing qemu "lockups"
>>> appearing randomly.. Will try to debug it.
>>> On the other hand, plain vanilla 1.4.0 in Windows 7 seems to run fine
>>> with my VxWorks image..
>> So the problem seems to be related to wether I pass --enable-debug to
>> qemu configure script.
>> With debug enabled it runs fine in Windows 7 aswell. If I leave it out
>> I get what appears to be a lockup
>> a couple of seconds into the guest boot process. Best I could do for
>> now was to attach to the process
>> with gdb and get a backtrace:
>>
>> (gdb) bt
>> #0  0x76fc000d in ntdll!LdrFindResource_U ()
>>    from /cygdrive/c/Windows/SysWOW64/ntdll.dll
>> #1  0x7704f896 in ntdll!RtlQueryTimeZoneInformation ()
>>    from /cygdrive/c/Windows/SysWOW64/ntdll.dll
>> #2  0x6ce2c99b in ?? ()
>> #3  0x00000000 in ?? ()
>
> The backtrace is usually better for QEMU with TCG interpreter. Run
>
>      configure --enable-tcg-interpreter
>
> The resulting binaries are much slower, but no longer create
> code on the fly, so you get normal backtraces.
>
> I also had to remove -fstack-protector-all (in file configure) to
> get good backtraces for w32/w64.

Ok, I enabled the tcg interpreter and removed ssp from qemu. I have an
instance which appears to be stuck. The qemu tcg thread seems to be
executing, but there are 2 other threads that seem to be stalled:

(gdb) bt
#0  0x7c90e514 in ntdll!LdrAccessResource () from
/cygdrive/c/WINDOWS/system32/ntdll.dll
#1  0x7c90df4a in ntdll!ZwWaitForMultipleObjects ()
   from /cygdrive/c/WINDOWS/system32/ntdll.dll
#2  0x76b5aee9 in timeGetTime () from /cygdrive/c/WINDOWS/system32/WINMM.DLL
#3  0x7c80b729 in KERNEL32!GetModuleFileNameA ()
   from /cygdrive/c/WINDOWS/system32/kernel32.dll
#4  0x00000000 in ?? ()

(gdb) bt
#0  0x7c90e514 in ntdll!LdrAccessResource () from
/cygdrive/c/WINDOWS/system32/ntdll.dll
#1  0x7c90df5a in ntdll!ZwWaitForSingleObject () from
/cygdrive/c/WINDOWS/system32/ntdll.dll
#2  0x7c919b23 in ntdll!RtlpWaitForCriticalSection ()
   from /cygdrive/c/WINDOWS/system32/ntdll.dll
#3  0x7c901046 in ntdll!RtlEnumerateGenericTableLikeADirectory ()
   from /cygdrive/c/WINDOWS/system32/ntdll.dll

Regards
Jacob
Paolo Bonzini April 9, 2013, 4:21 p.m. UTC | #5 
Il 20/02/2013 14:43, Paolo Bonzini ha scritto:
> These are needed for any of the Win32 alarm timer implementations.
> They are not tied to mmtimer exclusively.
> 
> Jacob tested this patch with both mmtimer and Win32 timers.
> 
> Cc: qemu-stable@nongnu.org
> Tested-by: Jacob Kroon <jacob.kroon@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>         Jacob, this is the same as "patch 1" you tested.

Stefan, can you pick up this patch again, together with Fabien's series?

Paolo

>  os-win32.c   | 10 ++++++++++
>  qemu-timer.c | 26 ++++++--------------------
>  2 files changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/os-win32.c b/os-win32.c
> index 3d43604..a0740ef 100644
> --- a/os-win32.c
> +++ b/os-win32.c
> @@ -23,6 +23,7 @@
>   * THE SOFTWARE.
>   */
>  #include <windows.h>
> +#include <mmsystem.h>
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <signal.h>
> @@ -67,9 +67,19 @@ static BOOL WINAPI qemu_ctrl_handler(DWORD type)
>      return TRUE;
>  }
>  
> +static TIMECAPS mm_tc;
> +
> +static void os_undo_timer_resolution(void)
> +{
> +    timeEndPeriod(mm_tc.wPeriodMin);
> +}
> +
>  void os_setup_early_signal_handling(void)
>  {
>      SetConsoleCtrlHandler(qemu_ctrl_handler, TRUE);
> +    timeGetDevCaps(&mm_tc, sizeof(mm_tc));
> +    timeBeginPeriod(mm_tc.wPeriodMin);
> +    atexit(os_undo_timer_resolution);
>  }
>  
>  /* Look for support files in the same directory as the executable.  */
> diff --git a/qemu-timer.c b/qemu-timer.c
> index 8fb5c75..50109a1 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -623,29 +622,15 @@ static void CALLBACK mm_alarm_handler(UINT uTimerID, UINT uMsg,
>  
>  static int mm_start_timer(struct qemu_alarm_timer *t)
>  {
>      timeGetDevCaps(&mm_tc, sizeof(mm_tc));
> -
> -    timeBeginPeriod(mm_tc.wPeriodMin);
> -
> -    mm_timer = timeSetEvent(mm_tc.wPeriodMin,   /* interval (ms) */
> -                            mm_tc.wPeriodMin,   /* resolution */
> -                            mm_alarm_handler,   /* function */
> -                            (DWORD_PTR)t,       /* parameter */
> -                            TIME_ONESHOT | TIME_CALLBACK_FUNCTION);
> -
> -    if (!mm_timer) {
> -        fprintf(stderr, "Failed to initialize win32 alarm timer\n");
> -        timeEndPeriod(mm_tc.wPeriodMin);
> -        return -1;
> -    }
> -
>      return 0;
>  }
>  
>  static void mm_stop_timer(struct qemu_alarm_timer *t)
>  {
> -    timeKillEvent(mm_timer);
> -    timeEndPeriod(mm_tc.wPeriodMin);
> +    if (mm_timer) {
> +        timeKillEvent(mm_timer);
> +    }
>  }
>  
>  static void mm_rearm_timer(struct qemu_alarm_timer *t, int64_t delta)
> @@ -657,7 +641,9 @@ static void mm_rearm_timer(struct qemu_alarm_timer *t, int64_t delta)
>          nearest_delta_ms = mm_tc.wPeriodMax;
>      }
>  
> -    timeKillEvent(mm_timer);
> +    if (mm_timer) {
> +        timeKillEvent(mm_timer);
> +    }
>      mm_timer = timeSetEvent((UINT)nearest_delta_ms,
>                              mm_tc.wPeriodMin,
>                              mm_alarm_handler,
>
Stefan Weil April 9, 2013, 8:22 p.m. UTC | #6 
Am 09.04.2013 18:21, schrieb Paolo Bonzini:
> Il 20/02/2013 14:43, Paolo Bonzini ha scritto:
>> These are needed for any of the Win32 alarm timer implementations.
>> They are not tied to mmtimer exclusively.
>>
>> Jacob tested this patch with both mmtimer and Win32 timers.
>>
>> Cc: qemu-stable@nongnu.org
>> Tested-by: Jacob Kroon <jacob.kroon@gmail.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>         Jacob, this is the same as "patch 1" you tested.
> Stefan, can you pick up this patch again, together with Fabien's series?
>
> Paolo


Hi Paolo,

I plan sending a pull request on Friday or Saturday for those patches.
Richard, the same applies to your TCI series (which I did not forget).

Cheers,
Stefan
diff mbox

Patch

diff --git a/os-win32.c b/os-win32.c
index 3d43604..a0740ef 100644
--- a/os-win32.c
+++ b/os-win32.c
@@ -23,6 +23,7 @@ 
  * THE SOFTWARE.
  */
 #include <windows.h>
+#include <mmsystem.h>
 #include <unistd.h>
 #include <fcntl.h>
 #include <signal.h>
@@ -67,9 +67,19 @@  static BOOL WINAPI qemu_ctrl_handler(DWORD type)
     return TRUE;
 }
 
+static TIMECAPS mm_tc;
+
+static void os_undo_timer_resolution(void)
+{
+    timeEndPeriod(mm_tc.wPeriodMin);
+}
+
 void os_setup_early_signal_handling(void)
 {
     SetConsoleCtrlHandler(qemu_ctrl_handler, TRUE);
+    timeGetDevCaps(&mm_tc, sizeof(mm_tc));
+    timeBeginPeriod(mm_tc.wPeriodMin);
+    atexit(os_undo_timer_resolution);
 }
 
 /* Look for support files in the same directory as the executable.  */
diff --git a/qemu-timer.c b/qemu-timer.c
index 8fb5c75..50109a1 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -623,29 +622,15 @@  static void CALLBACK mm_alarm_handler(UINT uTimerID, UINT uMsg,
 
 static int mm_start_timer(struct qemu_alarm_timer *t)
 {
     timeGetDevCaps(&mm_tc, sizeof(mm_tc));
-
-    timeBeginPeriod(mm_tc.wPeriodMin);
-
-    mm_timer = timeSetEvent(mm_tc.wPeriodMin,   /* interval (ms) */
-                            mm_tc.wPeriodMin,   /* resolution */
-                            mm_alarm_handler,   /* function */
-                            (DWORD_PTR)t,       /* parameter */
-                            TIME_ONESHOT | TIME_CALLBACK_FUNCTION);
-
-    if (!mm_timer) {
-        fprintf(stderr, "Failed to initialize win32 alarm timer\n");
-        timeEndPeriod(mm_tc.wPeriodMin);
-        return -1;
-    }
-
     return 0;
 }
 
 static void mm_stop_timer(struct qemu_alarm_timer *t)
 {
-    timeKillEvent(mm_timer);
-    timeEndPeriod(mm_tc.wPeriodMin);
+    if (mm_timer) {
+        timeKillEvent(mm_timer);
+    }
 }
 
 static void mm_rearm_timer(struct qemu_alarm_timer *t, int64_t delta)
@@ -657,7 +641,9 @@  static void mm_rearm_timer(struct qemu_alarm_timer *t, int64_t delta)
         nearest_delta_ms = mm_tc.wPeriodMax;
     }
 
-    timeKillEvent(mm_timer);
+    if (mm_timer) {
+        timeKillEvent(mm_timer);
+    }
     mm_timer = timeSetEvent((UINT)nearest_delta_ms,
                             mm_tc.wPeriodMin,
                             mm_alarm_handler,