[PATCH 0/1] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux

[PATCH 0/1] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux

[Don Slutz]

Based on the proposed fix in QEMU:

http://marc.info/?l=qemu-devel&m=139304386331192&w=2

That was provided for:

http://marc.info/?l=qemu-devel&m=139295851107140&w=2

Which is very close to a bug I have been looking into and asked some
questions about in:

http://lists.xen.org/archives/html/xen-devel/2014-02/msg01787.html

I came up with this change.  It does look like vpt does support
starting a timer in the past and what happens is based on the
setting of timer_mode.  Since real hardware does not support this
and this is the simpler change I went this way.

Don Slutz (1):
  hpet: Act more like real hardware

 xen/arch/x86/hvm/hpet.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

-- 
1.8.4

[PATCH 1/1] hpet: Act more like real hardware

[Don Slutz]

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.

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@verizon.com>
---
 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

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

[Jan Beulich]

>>> On 26.02.14 at 19:00, Don Slutz <dslutz@verizon.com> 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.

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@verizon.com>
> ---
>  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

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

[Don Slutz]

On 02/27/14 04:20, Jan Beulich wrote:
>>>> On 26.02.14 at 19:00, Don Slutz <dslutz@verizon.com> 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@verizon.com>
>> ---
>>   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
>
>

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

[Jan Beulich]

>>> On 28.02.14 at 00:56, Don Slutz <dslutz@verizon.com> wrote:
> On 02/27/14 04:20, Jan Beulich wrote:
>>>>> On 26.02.14 at 19:00, Don Slutz <dslutz@verizon.com> 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

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

[Don Slutz]

On 02/28/14 04:06, Jan Beulich wrote:
>>>> On 28.02.14 at 00:56, Don Slutz <dslutz@verizon.com> wrote:
>> On 02/27/14 04:20, Jan Beulich wrote:
>>>>>> On 26.02.14 at 19:00, Don Slutz <dslutz@verizon.com> 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
>

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

[Don Slutz]

On 02/28/14 09:01, Don Slutz wrote:
> On 02/28/14 04:06, Jan Beulich wrote:
>>>>> On 28.02.14 at 00:56, Don Slutz <dslutz@verizon.com> wrote:
>>> On 02/27/14 04:20, Jan Beulich wrote:
>>>>>>> On 26.02.14 at 19:00, Don Slutz <dslutz@verizon.com> 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
>
>

ping.

[snip]

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

[Jan Beulich]

>>> On 11.03.14 at 20:20, Don Slutz <dslutz@verizon.com> wrote:
> ping.

This hasn't been forgotten, but I am still waiting for at least on of
two things to happen:

- a v2 submission with an extended description
- comments/opinions by other people, particularly ones who
  were involved with the earlier modifications to the code in
  question

If neither happens, my reservations stand against changing the
code in question which has been in place for over 6 years without
any complaints, and could only be eliminated by demonstrating in
practice that either Vista didn't work properly anymore after the
second of the changes you pointed out (as your extended
explanation seems to suggest) or that Vista as well as other
Windows versions continue to work flawlessly (or at least no
worse than before) with your change in place.

Jan

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

[Slutz, Donald Christopher]

Ok, will post a v2.
    -Don Slutz

________________________________________
From: Jan Beulich [JBeulich@suse.com]
Sent: Wednesday, March 12, 2014 4:48 AM
To: Slutz, Donald Christopher
Cc: xen-devel@lists.xen.org; Keir Fraser
Subject: Re: [PATCH 1/1] hpet: Act more like real hardware

>>> On 11.03.14 at 20:20, Don Slutz <dslutz@verizon.com> wrote:
> ping.

This hasn't been forgotten, but I am still waiting for at least on of
two things to happen:

- a v2 submission with an extended description
- comments/opinions by other people, particularly ones who
  were involved with the earlier modifications to the code in
  question

If neither happens, my reservations stand against changing the
code in question which has been in place for over 6 years without
any complaints, and could only be eliminated by demonstrating in
practice that either Vista didn't work properly anymore after the
second of the changes you pointed out (as your extended
explanation seems to suggest) or that Vista as well as other
Windows versions continue to work flawlessly (or at least no
worse than before) with your change in place.

Jan

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

[Slutz, Donald Christopher]

I am having e-mail issues and so may not be formatted right.
________________________________________
From: Jan Beulich [JBeulich@suse.com]
Sent: Wednesday, March 12, 2014 4:48 AM
To: Slutz, Donald Christopher
Cc: xen-devel@lists.xen.org; Keir Fraser
Subject: Re: [PATCH 1/1] hpet: Act more like real hardware

>>> On 11.03.14 at 20:20, Don Slutz <dslutz@verizon.com> wrote:
> ping.

This hasn't been forgotten, but I am still waiting for at least on of
two things to happen:

- a v2 submission with an extended description
- comments/opinions by other people, particularly ones who
  were involved with the earlier modifications to the code in
  question

If neither happens, my reservations stand against changing the
code in question which has been in place for over 6 years without
any complaints, and could only be eliminated by demonstrating in
practice that either Vista didn't work properly anymore after the
second of the changes you pointed out (as your extended
explanation seems to suggest) or that Vista as well as other
Windows versions continue to work flawlessly (or at least no
worse than before) with your change in place.

Jan


While working on v2, I found an error in my statements.  Digging into
it more leads me to a new patch:

>From fa0e4f13237acff73443644e49ecabf549d72a40 Mon Sep 17 00:00:00 2001
From: Don Slutz <dslutz@verizon.com>
Date: Wed, 12 Mar 2014 14:32:58 -0400
Subject: [PATCH] hpet: Only do HPET_TINY_TIME_SPAN check for one shot 32 bit
 mode.

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

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 (max 32 -) to
   -0.9765625 milliseconds.  This is the range that converts from a
   past time into a future time.

Linux is expecting to start a periodic timer and does not expect the
1 interrupt to be 68,718,500,160 ns or more and the rest to be
1,000,000 ns between.

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

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 4324b52..4b7a396 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -229,6 +229,7 @@ static void hpet_set_timer(HPETState *h, unsigned int tn)
     }
 
     diff = tn_cmp - cur_tick;
+    oneshot = !timer_is_periodic(h, tn);
 
     /*
      * Detect time values set in the past. This is hard to do for 32-bit
@@ -237,7 +238,8 @@ static void hpet_set_timer(HPETState *h, unsigned int tn)
      * by looking for a 'small' time value in the past.
      */
     if ( (int64_t)diff < 0 )
-        diff = (timer_is_32bit(h, tn) && (-diff > HPET_TINY_TIME_SPAN))
+        diff = (timer_is_32bit(h, tn) && oneshot &&
+                (-diff > HPET_TINY_TIME_SPAN))
             ? (uint32_t)diff : 0;
 
     if ( (tn <= 1) && (h->hpet.config & HPET_CFG_LEGACY) )
@@ -254,7 +256,6 @@ static void hpet_set_timer(HPETState *h, unsigned int tn)
      * have elapsed between the time the comparator was written and the timer
      * being enabled (now).
      */
-    oneshot = !timer_is_periodic(h, tn);
     create_periodic_time(vhpet_vcpu(h), &h->pt[tn],
                          hpet_tick_to_ns(h, diff),
                          oneshot ? 0 : hpet_tick_to_ns(h, h->hpet.period[tn]),
-- 
1.8.4

Which does fix the linux issue I found and should not break x64 Vista.  I could also do this another way (why this as a v2 has not been sent) by adding code that says the diff must be less then or equal to the periodic time.  However that has a wider impact and I am not 100% sure what real hardware does in this case.

I will be doing some long term windows testing (Vista aka windows server 2003 and Windows server 2012) to see how the wall clock time goes when w32_time is not enabled.

   -Don Slutz

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

[Don Slutz]

On 03/12/14 15:23, Slutz, Donald Christopher wrote:
> I will be doing some long term windows testing (Vista aka windows server 2003 and Windows server 2012) to see how the wall clock time goes when w32_time is not enabled.

New v2 has been posted.  The longer term testing did
not show any issue with this change.  However v2 fixed
this without this code change.

     -Don Slutz