Xen devel - Jul 2005 - [PATCH] Broken timer macros breaks VMX

While merging my development tree with the latest hg tree I noticed that
VMX support was broken. When you boot an unmodified Linux kernel it gets
stuck in the "Calibrating delay" loop.  The reason for this is that
the
vmx code is repeatedly delivering timer interrupts.

The reason for this is the following:

#define MILLISECS(_ms)  (((s_time_t)(_ms)) * 1000000ULL )

The resulting type of this macro is ULL (instead of s_time_t). When this
macro is used in

     missed_ticks = (NOW() - vpit->scheduled) / MILLISECS(vpit->period);

and "scheduled > NOW()" (this condition occurs when the timer is
reset)
then this creates a very large number positive number of missed_ticks
rather than a negative. This causes

     if ( missed_ticks > 0 ) {
         vpit->pending_intr_nr += missed_ticks;

pending_intr_nr to be increased by a large positive number and causes
the (seemingly) endless loop of timer interrupt deliveries.

Correcting the MILLISECS() and friends macros to return s_time_t instead
of ULL fixes this problem. The other changes are just to get rid of
redundant code and variables.

	Leendert

Signed-Off-By: Leendert van Doorn <leendert@watson.ibm.com>


diff -r 55a5ad2f028d xen/arch/x86/vmx_intercept.c
--- a/xen/arch/x86/vmx_intercept.c	Fri Jul 29 11:21:39 2005
+++ b/xen/arch/x86/vmx_intercept.c	Sat Jul 30 00:54:51 2005
@@ -197,8 +197,7 @@
 static void pit_timer_fn(void *data)
 {
     struct vmx_virpit_t *vpit = data;
-    s_time_t   next;
-    int        missed_ticks;
+    int missed_ticks;
 
     missed_ticks = (NOW() - vpit->scheduled) / MILLISECS(vpit->period);
 
@@ -208,12 +207,11 @@
 
     /* pick up missed timer tick */
     if ( missed_ticks > 0 ) {
-        vpit->pending_intr_nr+= missed_ticks;
+        vpit->pending_intr_nr += missed_ticks;
         vpit->scheduled += missed_ticks * MILLISECS(vpit->period);
     }
-    next = vpit->scheduled + MILLISECS(vpit->period);
-    set_ac_timer(&vpit->pit_timer, next);
-    vpit->scheduled = next;
+    vpit->scheduled += MILLISECS(vpit->period);
+    set_ac_timer(&vpit->pit_timer, vpit->scheduled);
 }
 
 
diff -r 55a5ad2f028d xen/include/xen/time.h
--- a/xen/include/xen/time.h	Fri Jul 29 11:21:39 2005
+++ b/xen/include/xen/time.h	Sat Jul 30 00:54:51 2005
@@ -51,9 +51,9 @@
 s_time_t get_s_time(void);
 
 #define NOW()           ((s_time_t)get_s_time())
-#define SECONDS(_s)     (((s_time_t)(_s))  * 1000000000ULL )
-#define MILLISECS(_ms)  (((s_time_t)(_ms)) * 1000000ULL )
-#define MICROSECS(_us)  (((s_time_t)(_us)) * 1000ULL )
+#define SECONDS(_s)     ((s_time_t)((_s)  * 1000000000ULL))
+#define MILLISECS(_ms)  ((s_time_t)((_ms) * 1000000ULL))
+#define MICROSECS(_us)  ((s_time_t)((_us) * 1000ULL))
 
 extern void update_dom_time(struct vcpu *v);
 extern void do_settime(

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Leendert van Doorn wrote:
> Correcting the MILLISECS() and friends macros to return s_time_t instead
> of ULL fixes this problem. The other changes are just to get rid of
> redundant code and variables.
> 
Thanks, I have an equivalent change in our internal tree. We also 
changed vpit->period and friends to count in s_time_t instead of 
milliseconds, to reduce some math on every tick.

Now that your patch is already upstream, I''ve rebased our remaining 
changes against your patch.

I''m working to make our internal tree visible to everyone so that if
you
run into such issues, you can check if a pending commit has already 
fixed it.

	-Arun

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel