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

[Don Slutz <dslutz@xxxxxxxxxxx>]

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