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

[Don Slutz <dslutz@verizon.com>]

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