[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/28/14 04:06, Jan Beulich wrote:
On 28.02.14 at 00:56, Don Slutz <dslutz@xxxxxxxxxxx> wrote:
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)

From your question below, my best guess is that this is just to short an explanation. Here is an expanded one:

The only way that I can see the patch (c/s 13495:e2539ab3580a commit f545359b1c54f59be9d7c27112a68c51c45b06b5 "[HVM] Fix slow wallclock in x64 Vista") fixed the reported issue is by assuming that:

1) x64 Vista wallclock is using hpet in 32bit oneshot mode.
2) very offen the diff would be in the range -0.9765625 milliseconds to zero (0).
3) The sum of these is the amount that x64 Vista wallclock was off by.

The next change (c/s ? commit 73ee2f2e11fcdc27aae4f8caa72d240c4c9ed5ac "hvm: hpet: Fix overflow when converting to nanoseconds.") clearly breaks this by preventing hpet_tick_to_ns() to return tiny negative ns values. And so this change reverted the fix for slow wallclock in x64 Vista.

The third change (c/s ? commit e1845bbe732b5ad5755f0f3a93fb6ea85919e8a2 "hvm: Build guest timers on monotonic system time." ) looks to me to have changed the rate of diff being in the range -0.9765625 milliseconds to zero (0) from a lot of the time to almost never. This is based on the assumption that the 1st patch fixed the reported issue and the same issue was not reported since that time.

This is based on that fact that currently HPET_TINY_TIME_SPAN (0xffff1194 4294906260) converts to 68,718,500,160 ns and -1 converts to 18,014,398,509,481,983 ns so any number in the "tiny" range looks to me to mess up x64 Vista wallclock time and will also cause the linux MP-BIOS error message.

Hope this is clear.
    -Don Slutz



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?
Did you perhaps misunderstand me? I didn't ask for the patch to be
changed. What I asked for is clarification that the issues previously
having caused this code to be the way it is being still taken care of
with your change.

Jan



_______________________________________________
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®.