From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH 1/1] hpet: Act more like real hardware Date: Thu, 27 Feb 2014 18:56:58 -0500 Message-ID: <530FD0CA.5080906@terremark.com> References: <1393437647-16694-1-git-send-email-dslutz@verizon.com> <1393437647-16694-2-git-send-email-dslutz@verizon.com> <530F116F020000780011FC45@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <530F116F020000780011FC45@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Don Slutz Cc: Keir Fraser , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 02/27/14 04:20, Jan Beulich wrote: >>>> On 26.02.14 at 19:00, Don Slutz 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 >> --- >> 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 > >