[Qemu-devel] qemu-timer: Clean code and re-add multimedia timers for windows

[Qemu-devel] qemu-timer: Clean code and re-add multimedia timers for windows

[Stefan Weil]

This patch series contains patches for qemu-timer.

The first 3 patches try to improve readability of the code.
Patch 1 was already sent to qemu-devel.

The last patch fixes a problem reported by a user of my QEMU for Windows
binaries: the multimedia timers are needed when users want to run current
Linux distributions. Instead of reverting the patch which removed these
timers, I re-add them here (so we have 4 timers for windows now) and
cleaned the original code a little bit.

[PATCH 1/4] qemu-timer: Add and use new function qemu_timer_expired_ns
[PATCH 2/4] qemu-timer: Remove unneeded include statement (w32)
[PATCH 3/4] qemu-timer: Avoid type casts
[PATCH 4/4] qemu-timer: Fix timers for w32

[Qemu-devel] [PATCH 1/4] qemu-timer: Add and use new function qemu_timer_expired_ns

[Stefan Weil]

This simply moves code which is used three times
into a new function thus improving readability.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 qemu-timer.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index 50f1943..c3ad72a 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -177,6 +177,11 @@ struct qemu_alarm_timer {
 
 static struct qemu_alarm_timer *alarm_timer;
 
+static bool qemu_timer_expired_ns(QEMUTimer *timer_head, int64_t current_time)
+{
+    return timer_head && (timer_head->expire_time <= current_time);
+}
+
 int qemu_alarm_pending(void)
 {
     return alarm_timer->pending;
@@ -438,10 +443,9 @@ static void qemu_mod_timer_ns(QEMUTimer *ts, int64_t expire_time)
     pt = &active_timers[ts->clock->type];
     for(;;) {
         t = *pt;
-        if (!t)
-            break;
-        if (t->expire_time > expire_time)
+        if (!qemu_timer_expired_ns(t, expire_time)) {
             break;
+        }
         pt = &t->next;
     }
     ts->expire_time = expire_time;
@@ -478,9 +482,7 @@ int qemu_timer_pending(QEMUTimer *ts)
 
 int qemu_timer_expired(QEMUTimer *timer_head, int64_t current_time)
 {
-    if (!timer_head)
-        return 0;
-    return (timer_head->expire_time <= current_time * timer_head->scale);
+    return qemu_timer_expired_ns(timer_head, current_time * timer_head->scale);
 }
 
 static void qemu_run_timers(QEMUClock *clock)
@@ -495,8 +497,9 @@ static void qemu_run_timers(QEMUClock *clock)
     ptimer_head = &active_timers[clock->type];
     for(;;) {
         ts = *ptimer_head;
-        if (!ts || ts->expire_time > current_time)
+        if (!qemu_timer_expired_ns(ts, current_time)) {
             break;
+        }
         /* remove timer from the list before calling the callback */
         *ptimer_head = ts->next;
         ts->next = NULL;
-- 
1.7.2.5

[Qemu-devel] [PULL] qemu-timer: Add and use new function qemu_timer_expired_ns and other patches

[Stefan Weil]

Hello,

the four qemu-timer related patches which I sent to qemu-devel can now 
be pulled.
Maybe this makes the commit to git master easier.

There was no feedback for the first three patches.

The fourth patch changes windows code only and is needed for native windows.

Cheers,
Stefan Weil


The following changes since commit ec444452b8753a372de30b22d9b4765a799db612:

   target-arm: Set Invalid flag for NaN in float-to-int conversions 
(2011-04-20 13:01:05 +0200)

are available in the git repository at:
   git://qemu.weilnetz.de/git/qemu.git/ patches

Stefan Weil (4):
       qemu-timer: Add and use new function qemu_timer_expired_ns
       qemu-timer: Remove unneeded include statement (w32)
       qemu-timer: Avoid type casts
       qemu-timer: Fix timers for w32

  qemu-timer.c |  155 
++++++++++++++++++++++++++++++++++++++++++++++++----------
  qemu-timer.h |    1 -
  2 files changed, 128 insertions(+), 28 deletions(-)

Re: [Qemu-devel] [PULL] qemu-timer: Add and use new function qemu_timer_expired_ns and other patches

[Stefan Weil]

Am 20.04.2011 16:26, schrieb Stefan Weil:
> Hello,
>
> the four qemu-timer related patches which I sent to qemu-devel can now 
> be pulled.
> Maybe this makes the commit to git master easier.
>
> There was no feedback for the first three patches.
>
> The fourth patch changes windows code only and is needed for native 
> windows.
>
> Cheers,
> Stefan Weil
>
>
> The following changes since commit 
> ec444452b8753a372de30b22d9b4765a799db612:
>
>   target-arm: Set Invalid flag for NaN in float-to-int conversions 
> (2011-04-20 13:01:05 +0200)
>
> are available in the git repository at:
>   git://qemu.weilnetz.de/git/qemu.git/ patches
>
> Stefan Weil (4):
>       qemu-timer: Add and use new function qemu_timer_expired_ns
>       qemu-timer: Remove unneeded include statement (w32)
>       qemu-timer: Avoid type casts
>       qemu-timer: Fix timers for w32
>
>  qemu-timer.c |  155 
> ++++++++++++++++++++++++++++++++++++++++++++++++----------
>  qemu-timer.h |    1 -
>  2 files changed, 128 insertions(+), 28 deletions(-)


Ping? Is there anything missing?
The patches were sent to qemu-devel on 2011-04-10.

The second one is trivial. The last one is essential for w32 hosts
(it only changes w32 code). The other two patches try to improve
code quality a little bit.

Re: [Qemu-devel] [PULL] qemu-timer: Add and use new function qemu_timer_expired_ns and other patches

[Blue Swirl]

On Thu, Apr 28, 2011 at 3:18 PM, Stefan Weil <weil@mail.berlios.de> wrote:
> Am 20.04.2011 16:26, schrieb Stefan Weil:
>>
>> Hello,
>>
>> the four qemu-timer related patches which I sent to qemu-devel can now be
>> pulled.
>> Maybe this makes the commit to git master easier.
>>
>> There was no feedback for the first three patches.
>>
>> The fourth patch changes windows code only and is needed for native
>> windows.
>>
>> Cheers,
>> Stefan Weil
>>
>>
>> The following changes since commit
>> ec444452b8753a372de30b22d9b4765a799db612:
>>
>>  target-arm: Set Invalid flag for NaN in float-to-int conversions
>> (2011-04-20 13:01:05 +0200)
>>
>> are available in the git repository at:
>>  git://qemu.weilnetz.de/git/qemu.git/ patches
>>
>> Stefan Weil (4):
>>      qemu-timer: Add and use new function qemu_timer_expired_ns
>>      qemu-timer: Remove unneeded include statement (w32)
>>      qemu-timer: Avoid type casts
>>      qemu-timer: Fix timers for w32
>>
>>  qemu-timer.c |  155
>> ++++++++++++++++++++++++++++++++++++++++++++++++----------
>>  qemu-timer.h |    1 -
>>  2 files changed, 128 insertions(+), 28 deletions(-)
>
>
> Ping? Is there anything missing?
> The patches were sent to qemu-devel on 2011-04-10.
>
> The second one is trivial. The last one is essential for w32 hosts
> (it only changes w32 code). The other two patches try to improve
> code quality a little bit.

Thanks, pulled.

[Qemu-devel] [PATCH 2/4] qemu-timer: Remove unneeded include statement (w32)

[Stefan Weil]

mmsystem.h is not needed in qemu-timer.h, so remove it.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 qemu-timer.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/qemu-timer.h b/qemu-timer.h
index 75d5675..a932b09 100644
--- a/qemu-timer.h
+++ b/qemu-timer.h
@@ -7,7 +7,6 @@
 
 #ifdef _WIN32
 #include <windows.h>
-#include <mmsystem.h>
 #endif
 
 /* timers */
-- 
1.7.2.5

[Qemu-devel] [PATCH 3/4] qemu-timer: Avoid type casts

[Stefan Weil]

The type casts are no longer needed after some small changes
in struct qemu_alarm_timer. This also improves readability
of the code.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 qemu-timer.c |   42 ++++++++++++++++++++++--------------------
 1 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index c3ad72a..26e5ea3 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -169,8 +169,12 @@ struct qemu_alarm_timer {
     int (*start)(struct qemu_alarm_timer *t);
     void (*stop)(struct qemu_alarm_timer *t);
     void (*rearm)(struct qemu_alarm_timer *t);
-    void *priv;
-
+#if defined(__linux__)
+    int fd;
+    timer_t timer;
+#elif defined(_WIN32)
+    HANDLE timer;
+#endif
     char expired;
     char pending;
 };
@@ -289,18 +293,16 @@ static struct qemu_alarm_timer alarm_timers[] = {
 #ifndef _WIN32
 #ifdef __linux__
     {"dynticks", dynticks_start_timer,
-     dynticks_stop_timer, dynticks_rearm_timer, NULL},
+     dynticks_stop_timer, dynticks_rearm_timer},
     /* HPET - if available - is preferred */
-    {"hpet", hpet_start_timer, hpet_stop_timer, NULL, NULL},
+    {"hpet", hpet_start_timer, hpet_stop_timer, NULL},
     /* ...otherwise try RTC */
-    {"rtc", rtc_start_timer, rtc_stop_timer, NULL, NULL},
+    {"rtc", rtc_start_timer, rtc_stop_timer, NULL},
 #endif
-    {"unix", unix_start_timer, unix_stop_timer, NULL, NULL},
+    {"unix", unix_start_timer, unix_stop_timer, NULL},
 #else
-    {"dynticks", win32_start_timer,
-     win32_stop_timer, win32_rearm_timer, NULL},
-    {"win32", win32_start_timer,
-     win32_stop_timer, NULL, NULL},
+    {"dynticks", win32_start_timer, win32_stop_timer, win32_rearm_timer},
+    {"win32", win32_start_timer, win32_stop_timer, NULL},
 #endif
     {NULL, }
 };
@@ -773,7 +775,7 @@ static int hpet_start_timer(struct qemu_alarm_timer *t)
         goto fail;
 
     enable_sigio_timer(fd);
-    t->priv = (void *)(long)fd;
+    t->fd = fd;
 
     return 0;
 fail:
@@ -783,7 +785,7 @@ fail:
 
 static void hpet_stop_timer(struct qemu_alarm_timer *t)
 {
-    int fd = (long)t->priv;
+    int fd = t->fd;
 
     close(fd);
 }
@@ -812,14 +814,14 @@ static int rtc_start_timer(struct qemu_alarm_timer *t)
 
     enable_sigio_timer(rtc_fd);
 
-    t->priv = (void *)(long)rtc_fd;
+    t->fd = rtc_fd;
 
     return 0;
 }
 
 static void rtc_stop_timer(struct qemu_alarm_timer *t)
 {
-    int rtc_fd = (long)t->priv;
+    int rtc_fd = t->fd;
 
     close(rtc_fd);
 }
@@ -854,21 +856,21 @@ static int dynticks_start_timer(struct qemu_alarm_timer *t)
         return -1;
     }
 
-    t->priv = (void *)(long)host_timer;
+    t->timer = host_timer;
 
     return 0;
 }
 
 static void dynticks_stop_timer(struct qemu_alarm_timer *t)
 {
-    timer_t host_timer = (timer_t)(long)t->priv;
+    timer_t host_timer = t->timer;
 
     timer_delete(host_timer);
 }
 
 static void dynticks_rearm_timer(struct qemu_alarm_timer *t)
 {
-    timer_t host_timer = (timer_t)(long)t->priv;
+    timer_t host_timer = t->timer;
     struct itimerspec timeout;
     int64_t nearest_delta_ns = INT64_MAX;
     int64_t current_ns;
@@ -970,13 +972,13 @@ static int win32_start_timer(struct qemu_alarm_timer *t)
         return -1;
     }
 
-    t->priv = (PVOID) hTimer;
+    t->timer = hTimer;
     return 0;
 }
 
 static void win32_stop_timer(struct qemu_alarm_timer *t)
 {
-    HANDLE hTimer = t->priv;
+    HANDLE hTimer = t->timer;
 
     if (hTimer) {
         DeleteTimerQueueTimer(NULL, hTimer, NULL);
@@ -985,7 +987,7 @@ static void win32_stop_timer(struct qemu_alarm_timer *t)
 
 static void win32_rearm_timer(struct qemu_alarm_timer *t)
 {
-    HANDLE hTimer = t->priv;
+    HANDLE hTimer = t->timer;
     int nearest_delta_ms;
     BOOLEAN success;
 
-- 
1.7.2.5

[Qemu-devel] [PATCH 4/4] qemu-timer: Fix timers for w32

[Stefan Weil]

Commit 68c23e5520e8286d79d96ab47c0ea722ceb75041 removed the
multimedia timer, but this timer is needed for certain
Linux kernels. Otherwise Linux boot stops with this error:

    MP-BIOS bug: 8254 timer not connected to IO-APIC

So the multimedia timer is added again here.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 qemu-timer.c |   96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 96 insertions(+), 0 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index 26e5ea3..a57197b 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -209,6 +209,10 @@ static void qemu_rearm_alarm_timer(struct qemu_alarm_timer *t)
 
 #ifdef _WIN32
 
+static int mm_start_timer(struct qemu_alarm_timer *t);
+static void mm_stop_timer(struct qemu_alarm_timer *t);
+static void mm_rearm_timer(struct qemu_alarm_timer *t);
+
 static int win32_start_timer(struct qemu_alarm_timer *t);
 static void win32_stop_timer(struct qemu_alarm_timer *t);
 static void win32_rearm_timer(struct qemu_alarm_timer *t);
@@ -301,6 +305,8 @@ static struct qemu_alarm_timer alarm_timers[] = {
 #endif
     {"unix", unix_start_timer, unix_stop_timer, NULL},
 #else
+    {"mmtimer", mm_start_timer, mm_stop_timer, NULL},
+    {"mmtimer2", mm_start_timer, mm_stop_timer, mm_rearm_timer},
     {"dynticks", win32_start_timer, win32_stop_timer, win32_rearm_timer},
     {"win32", win32_start_timer, win32_stop_timer, NULL},
 #endif
@@ -949,6 +955,96 @@ static void unix_stop_timer(struct qemu_alarm_timer *t)
 
 #ifdef _WIN32
 
+static MMRESULT mm_timer;
+static unsigned mm_period;
+
+static void CALLBACK mm_alarm_handler(UINT uTimerID, UINT uMsg,
+                                      DWORD_PTR dwUser, DWORD_PTR dw1,
+                                      DWORD_PTR dw2)
+{
+    struct qemu_alarm_timer *t = alarm_timer;
+    if (!t) {
+        return;
+    }
+    if (alarm_has_dynticks(t) || qemu_next_alarm_deadline() <= 0) {
+        t->expired = alarm_has_dynticks(t);
+        t->pending = 1;
+        qemu_notify_event();
+    }
+}
+
+static int mm_start_timer(struct qemu_alarm_timer *t)
+{
+    TIMECAPS tc;
+    UINT flags;
+
+    memset(&tc, 0, sizeof(tc));
+    timeGetDevCaps(&tc, sizeof(tc));
+
+    mm_period = tc.wPeriodMin;
+    timeBeginPeriod(mm_period);
+
+    flags = TIME_CALLBACK_FUNCTION;
+    if (alarm_has_dynticks(t)) {
+        flags |= TIME_ONESHOT;
+    } else {
+        flags |= TIME_PERIODIC;
+    }
+
+    mm_timer = timeSetEvent(1,                  /* interval (ms) */
+                            mm_period,          /* resolution */
+                            mm_alarm_handler,   /* function */
+                            (DWORD)t,           /* parameter */
+                            flags);
+
+    if (!mm_timer) {
+        fprintf(stderr, "Failed to initialize win32 alarm timer: %ld\n",
+                GetLastError());
+        timeEndPeriod(mm_period);
+        return -1;
+    }
+
+    return 0;
+}
+
+static void mm_stop_timer(struct qemu_alarm_timer *t)
+{
+    timeKillEvent(mm_timer);
+    timeEndPeriod(mm_period);
+}
+
+static void mm_rearm_timer(struct qemu_alarm_timer *t)
+{
+    int nearest_delta_ms;
+
+    assert(alarm_has_dynticks(t));
+    if (!active_timers[QEMU_CLOCK_REALTIME] &&
+        !active_timers[QEMU_CLOCK_VIRTUAL] &&
+        !active_timers[QEMU_CLOCK_HOST]) {
+        return;
+    }
+
+    timeKillEvent(mm_timer);
+
+    nearest_delta_ms = (qemu_next_alarm_deadline() + 999999) / 1000000;
+    if (nearest_delta_ms < 1) {
+        nearest_delta_ms = 1;
+    }
+    mm_timer = timeSetEvent(nearest_delta_ms,
+                            mm_period,
+                            mm_alarm_handler,
+                            (DWORD)t,
+                            TIME_ONESHOT | TIME_CALLBACK_FUNCTION);
+
+    if (!mm_timer) {
+        fprintf(stderr, "Failed to re-arm win32 alarm timer %ld\n",
+                GetLastError());
+
+        timeEndPeriod(mm_period);
+        exit(1);
+    }
+}
+
 static int win32_start_timer(struct qemu_alarm_timer *t)
 {
     HANDLE hTimer;
-- 
1.7.2.5

[Qemu-devel] Re: [PATCH 4/4] qemu-timer: Fix timers for w32

[Paolo Bonzini]

On 04/10/2011 08:28 PM, Stefan Weil wrote:
> Commit 68c23e5520e8286d79d96ab47c0ea722ceb75041 removed the
> multimedia timer, but this timer is needed for certain
> Linux kernels. Otherwise Linux boot stops with this error:
>
>      MP-BIOS bug: 8254 timer not connected to IO-APIC
>
> So the multimedia timer is added again here.

Which distribution and Windows version is that?  Also, have they tried 
the non-dynticks timer (win32)?

Paolo

Re: [Qemu-devel] [PATCH 4/4] qemu-timer: Fix timers for w32

[Stefan Weil]

Am 11.04.2011 09:36, schrieb Paolo Bonzini:
> On 04/10/2011 08:28 PM, Stefan Weil wrote:
>> Commit 68c23e5520e8286d79d96ab47c0ea722ceb75041 removed the
>> multimedia timer, but this timer is needed for certain
>> Linux kernels. Otherwise Linux boot stops with this error:
>>
>>      MP-BIOS bug: 8254 timer not connected to IO-APIC
>>
>> So the multimedia timer is added again here.
>
> Which distribution and Windows version is that?  Also, have they tried 
> the non-dynticks timer (win32)?
>
> Paolo

The bug was reported for a tinycore 3.5.1 guest (linux kernel 3.6.33-3).
The iso image is available from 
http://distro.ibiblio.org/tinycorelinux/3.x/release/.
QEMU was running on XP SP3.

Other Linux live CD-ROMs (e.g. FC14) were reported to show the same bug
when run as guest.

APIC can be disabled (kernel parameter), without APIC there is no problem.

I see the same bug here with two XP hosts and also tried both timer variants
of current QEMU (without a difference).

I don't get the bug when running on a Linux host using wine.

Regards, Stefan

Re: [Qemu-devel] [PATCH 4/4] qemu-timer: Fix timers for w32

[Jan Kiszka]

On 2011-04-11 19:21, Stefan Weil wrote:
> Am 11.04.2011 09:36, schrieb Paolo Bonzini:
>> On 04/10/2011 08:28 PM, Stefan Weil wrote:
>>> Commit 68c23e5520e8286d79d96ab47c0ea722ceb75041 removed the
>>> multimedia timer, but this timer is needed for certain
>>> Linux kernels. Otherwise Linux boot stops with this error:
>>>
>>>      MP-BIOS bug: 8254 timer not connected to IO-APIC
>>>
>>> So the multimedia timer is added again here.
>>
>> Which distribution and Windows version is that?  Also, have they tried
>> the non-dynticks timer (win32)?
>>
>> Paolo
> 
> The bug was reported for a tinycore 3.5.1 guest (linux kernel 3.6.33-3).
> The iso image is available from
> http://distro.ibiblio.org/tinycorelinux/3.x/release/.
> QEMU was running on XP SP3.
> 
> Other Linux live CD-ROMs (e.g. FC14) were reported to show the same bug
> when run as guest.
> 
> APIC can be disabled (kernel parameter), without APIC there is no problem.
> 
> I see the same bug here with two XP hosts and also tried both timer
> variants
> of current QEMU (without a difference).
> 
> I don't get the bug when running on a Linux host using wine.

Passing no_timer_check to the Linux guest should work around the issue
as well. But this feature is only available since 2.6.20 (excluding
popular legacy 2.6.16 kernels).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 4/4] qemu-timer: Fix timers for w32

[Paolo Bonzini]

On 04/12/2011 02:36 PM, Jan Kiszka wrote:
>> I see the same bug here with two XP hosts and also tried both timer
>> variants
>> of current QEMU (without a difference).
>>
>> I don't get the bug when running on a Linux host using wine.
>
> Passing no_timer_check to the Linux guest should work around the issue
> as well. But this feature is only available since 2.6.20 (excluding
> popular legacy 2.6.16 kernels).

I think adding back MM timers is fine, especially since a bug that only 
happens under native Windows is a bug I'm unlikely to look at...

Paolo

Re: [Qemu-devel] [PATCH 4/4] qemu-timer: Fix timers for w32

[Jan Kiszka]

On 2011-04-12 14:40, Paolo Bonzini wrote:
> On 04/12/2011 02:36 PM, Jan Kiszka wrote:
>>> I see the same bug here with two XP hosts and also tried both timer
>>> variants
>>> of current QEMU (without a difference).
>>>
>>> I don't get the bug when running on a Linux host using wine.
>>
>> Passing no_timer_check to the Linux guest should work around the issue
>> as well. But this feature is only available since 2.6.20 (excluding
>> popular legacy 2.6.16 kernels).
> 
> I think adding back MM timers is fine, especially since a bug that only 
> happens under native Windows is a bug I'm unlikely to look at...

That wasn't meant as a vote against changing the Windows code paths,
just as a further hint how to work-around virtualization related effects.

Even with high-res timers, you may hit that issue once in a while, also
on Linux hosts. When using KVM, latest Linux guest will notice that they
are running on a hypervisor and should skip the test automatically IIRC.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux