qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [PATCH v8 1/4] hw/ptimer: Fix issues caused by the adjust


From: Dmitry Osipenko
Subject: Re: [Qemu-arm] [PATCH v8 1/4] hw/ptimer: Fix issues caused by the adjusted timer limit value
Date: Wed, 6 Jan 2016 16:25:48 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0

06.01.2016 15:15, Peter Crosthwaite пишет:
diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index edf077c..035af97 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -34,20 +34,39 @@ static void ptimer_trigger(ptimer_state *s)

  static void ptimer_reload(ptimer_state *s)
  {
-    if (s->delta == 0) {
+    uint32_t period_frac = s->period_frac;
+    uint64_t period = s->period;
+    uint64_t delta = s->delta;
+    uint64_t limit = s->limit;
+

Localising these variables is out of scope of the change. I think you can
just use s->foo and if you want to cleanup, it should be a separate
patch.


Okay

+    if (delta == 0) {
          ptimer_trigger(s);
-        s->delta = s->limit;
+        delta = limit;
      }
-    if (s->delta == 0 || s->period == 0) {
+    if (delta == 0 || period == 0) {
          fprintf(stderr, "Timer with period zero, disabling\n");
          s->enabled = 0;
          return;
      }

+    /*
+     * Artificially limit timeout rate to something
+     * achievable under QEMU.  Otherwise, QEMU spends all
+     * its time generating timer interrupts, and there
+     * is no forward progress.
+     * About ten microseconds is the fastest that really works
+     * on the current generation of host machines.
+     */
+
+    if ((s->enabled == 1) && (limit * period < 10000)) {
+        period = 10000 / limit;
+        period_frac = 0;
+    }
+

I think it should be ok to just update s->period and s->period_frac ...


No, then it would be irreversibly lost. What if'd decide to change the limit to some large value?

      s->last_event = s->next_event;
-    s->next_event = s->last_event + s->delta * s->period;
-    if (s->period_frac) {
-        s->next_event += ((int64_t)s->period_frac * s->delta) >> 32;
+    s->next_event = s->last_event + delta * period;
+    if (period_frac) {
+        s->next_event += ((int64_t)period_frac * delta) >> 32;
      }
      timer_mod(s->timer, s->next_event);
  }
@@ -82,6 +101,8 @@ uint64_t ptimer_get_count(ptimer_state *s)
              uint64_t div;
              int clz1, clz2;
              int shift;
+            uint32_t period_frac = s->period_frac;
+            uint64_t period = s->period;

              /* We need to divide time by period, where time is stored in
                 rem (64-bit integer) and period is stored in period/period_frac
@@ -93,8 +114,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
                 backwards.
              */

+            if ((s->enabled == 1) && (s->limit * period < 10000)) {
+                period = 10000 / s->limit;
+                period_frac = 0;
+            }
+

... and then this (and the local variables) become obsolete. Can get_count()
blindly use the period and period_frac as used by ptimer_reload?

              rem = s->next_event - now;
-            div = s->period;
+            div = period;

              clz1 = clz64(rem);
              clz2 = clz64(div);
@@ -103,13 +129,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
              rem <<= shift;
              div <<= shift;
              if (shift >= 32) {
-                div |= ((uint64_t)s->period_frac << (shift - 32));
+                div |= ((uint64_t)period_frac << (shift - 32));
              } else {
                  if (shift != 0)
-                    div |= (s->period_frac >> (32 - shift));
+                    div |= (period_frac >> (32 - shift));
                  /* Look at remaining bits of period_frac and round div up if
                     necessary.  */
-                if ((uint32_t)(s->period_frac << shift))
+                if ((uint32_t)(period_frac << shift))
                      div += 1;
              }
              counter = rem / div;
@@ -181,19 +207,6 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)
     count = limit.  */
  void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
  {
-    /*
-     * Artificially limit timeout rate to something
-     * achievable under QEMU.  Otherwise, QEMU spends all
-     * its time generating timer interrupts, and there
-     * is no forward progress.
-     * About ten microseconds is the fastest that really works
-     * on the current generation of host machines.
-     */
-
-    if (!use_icount && limit * s->period < 10000 && s->period) {

This original rate limiting code is gated on icount, so I think then
new way should be the same.


Shoot :) That's second time I'm missing it. Good catch!

Regards,
Peter

-        limit = 10000 / s->period;
-    }
-
      s->limit = limit;
      if (reload)
          s->delta = limit;
--
2.6.4



--
Dmitry



reply via email to

[Prev in Thread] Current Thread [Next in Thread]