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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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)
> 
> 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