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

Re: [Xen-devel] [PATCH 1/1] hpet: Act more like real hardware



On 02/27/14 04:20, Jan Beulich wrote:
On 26.02.14 at 19:00, Don Slutz <dslutz@xxxxxxxxxxx> wrote:
Currently in 32 bit mode the routine hpet_set_timer() will convert a
time in the past to a time in future.  This is done by the uint32_t
cast of diff.

Even without this issue, hpet_tick_to_ns() does not support past
times.

Real hardware does not support past times.

So just do the same thing in 32 bit mode as 64 bit mode.
While the change looks valid at the first glance, what I'm missing
is an explanation of how the problem that the introduction of this
fixed (c/s 13495:e2539ab3580a "[HVM] Fix slow wallclock in x64
Vista") is now being taken care of (or why this is of no concern).
That's pretty relevant considering for how long this code has been
there without causing (known) problems to anyone.

Ok, digging around (the git version):

commit f545359b1c54f59be9d7c27112a68c51c45b06b5
Date:   Thu Jan 18 18:54:28 2007 +0000
    [HVM] Fix slow wallclock in x64 Vista. This is due to confusing a


And one that changed how it worked:

commit 73ee2f2e11fcdc27aae4f8caa72d240c4c9ed5ac
Date:   Tue Jan 8 16:20:04 2008 +0000
   hvm: hpet: Fix overflow when converting to nanoseconds.


Is when a past time was prevented.  Which may well have caused x64 Vista to 
have wallclock issues.

Next:

commit e1845bbe732b5ad5755f0f3a93fb6ea85919e8a2
Date:   Sat May 24 09:27:03 2008 +0100
    hvm: Build guest timers on monotonic system time.


Has a chance to do 2 things:
1) Make the diff < 0 very unlikely
2) Fixed x64 Vista wallclock issues (again)

Looking closer at hpet_tick_to_ns() and doing some math.  I get:


    h->stime_freq = S_TO_NS;
    h->hpet_to_ns_scale = ((S_TO_NS * STIME_PER_HPET_TICK) << 10) / 
h->stime_freq;

I.E.

    h->hpet_to_ns_scale = STIME_PER_HPET_TICK << 10;

And so:

#define hpet_tick_to_ns(h, tick)                        \
    ((s_time_t)((((tick) > (h)->hpet_to_ns_limit) ?     \
        ~0ULL : (tick) * (h)->hpet_to_ns_scale) >> 10))


Is really:

#define hpet_tick_to_ns(h, tick)                        \
    ((s_time_t)((((tick) > (h)->hpet_to_ns_limit) ?     \
        (~0ULL >> 10) : (tick) * STIME_PER_HPET_TICK))

And if you change to using a signed multiply most of the time you will be fine.  If you 
want a complex that is "safer":

#define hpet_tick_to_ns(tick)                                   \
    ((s_time_t)(tick) >= 0 ?                                    \
      (s_time_t)(tick) * (s_time_t)STIME_PER_HPET_TICK >= 0 ?   \
       (s_time_t)(tick) * (s_time_t)STIME_PER_HPET_TICK :       \
       (s_time_t)(~0ULL >> 10) :                                \
      (s_time_t)(tick) * (s_time_t)STIME_PER_HPET_TICK < 0 ?    \
       (s_time_t)(tick) * (s_time_t)STIME_PER_HPET_TICK :       \
       0)

If the signed multiply overflows in the positive case then the old max is 
returned.  Note: this can return larger values then the old max.

So I can re-work the patch to use this and still provide past times.  Which 
path should I go with?

   -Don Slutz








Jan

Without this change it is possible for an HVM guest running linux to
get the message:

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

On the guest console(s); and will panic.

Also Xen hypervisor console with be flooded with:

vioapic.c:352:d1 Unsupported delivery mode 7
vioapic.c:352:d1 Unsupported delivery mode 7
vioapic.c:352:d1 Unsupported delivery mode 7

Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
---
  xen/arch/x86/hvm/hpet.c | 13 +++----------
  1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 4324b52..14b1a39 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -197,10 +197,6 @@ static void hpet_stop_timer(HPETState *h, unsigned int tn)
      hpet_get_comparator(h, tn);
  }
-/* the number of HPET tick that stands for
- * 1/(2^10) second, namely, 0.9765625 milliseconds */
-#define  HPET_TINY_TIME_SPAN  ((h->stime_freq >> 10) / STIME_PER_HPET_TICK)
-
  static void hpet_set_timer(HPETState *h, unsigned int tn)
  {
      uint64_t tn_cmp, cur_tick, diff;
@@ -231,14 +227,11 @@ static void hpet_set_timer(HPETState *h, unsigned int tn)
      diff = tn_cmp - cur_tick;
/*
-     * Detect time values set in the past. This is hard to do for 32-bit
-     * comparators as the timer does not have to be set that far in the future
-     * for the counter difference to wrap a 32-bit signed integer. We fudge
-     * by looking for a 'small' time value in the past.
+     * Detect time values set in the past. Since hpet_tick_to_ns() does
+     * not handle this, use 0 for both 64 and 32 bit mode.
       */
      if ( (int64_t)diff < 0 )
-        diff = (timer_is_32bit(h, tn) && (-diff > HPET_TINY_TIME_SPAN))
-            ? (uint32_t)diff : 0;
+        diff = 0;
if ( (tn <= 1) && (h->hpet.config & HPET_CFG_LEGACY) )
          /* if LegacyReplacementRoute bit is set, HPET specification requires
--
1.8.4




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.