Fix a bug in the HPET emulation where a timer interrupt could be lost when the

guest disables the HPET.

The HPET timer interrupt is triggered from the callout handler associated with
the timer. It is possible for the callout handler to be delayed before it gets
a chance to execute. If the guest disables the HPET during this window then the
handler never gets a chance to execute and the timer interrupt is lost.

This is now fixed by injecting a timer interrupt into the guest if the callout
time is detected to be in the past when the HPET is disabled.

1 files changed, 84 insertions, 67 deletions

diff --git a/sys/amd64/vmm/io/vhpet.c b/sys/amd64/vmm/io/vhpet.c
index 929b343..993d5e6 100644
--- a/sys/amd64/vmm/io/vhpet.c
+++ b/sys/amd64/vmm/io/vhpet.c
@@ -77,8 +77,8 @@ struct vhpet {
 
 	uint64_t	config;		/* Configuration */
 	uint64_t	isr;		/* Interrupt Status */
-	uint32_t	counter;	/* HPET Counter */
-	sbintime_t	counter_sbt;
+	uint32_t	countbase;	/* HPET counter base value */
+	sbintime_t	countbase_sbt;	/* uptime corresponding to base value */
 
 	struct {
 		uint64_t	cap_config;	/* Configuration */
@@ -86,6 +86,7 @@ struct vhpet {
 		uint32_t	compval;	/* Comparator */
 		uint32_t	comprate;
 		struct callout	callout;
+		sbintime_t	callout_sbt;	/* time when counter==compval */
 		struct vhpet_callout_arg arg;
 	} timer[VHPET_NUM_TIMERS];
 };
@@ -93,6 +94,9 @@ struct vhpet {
 #define	VHPET_LOCK(vhp)		mtx_lock(&((vhp)->mtx))
 #define	VHPET_UNLOCK(vhp)	mtx_unlock(&((vhp)->mtx))
 
+static void vhpet_start_timer(struct vhpet *vhpet, int n, uint32_t counter,
+    sbintime_t now);
+
 static uint64_t
 vhpet_capabilities(void)
 {
@@ -164,30 +168,28 @@ vhpet_timer_ioapic_pin(struct vhpet *vhpet, int n)
 }
 
 static uint32_t
-vhpet_counter(struct vhpet *vhpet, bool latch)
+vhpet_counter(struct vhpet *vhpet, sbintime_t *nowptr)
 {
 	uint32_t val;
-	sbintime_t cur_sbt, delta_sbt;
+	sbintime_t now, delta;
 
-	val = vhpet->counter;
+	val = vhpet->countbase;
 	if (vhpet_counter_enabled(vhpet)) {
-		cur_sbt = sbinuptime();
-		delta_sbt = cur_sbt - vhpet->counter_sbt;
-		KASSERT(delta_sbt >= 0,
-		    ("vhpet counter went backwards: %#lx to %#lx",
-		    vhpet->counter_sbt, cur_sbt));
-		val += delta_sbt / vhpet->freq_sbt;
-
+		now = sbinuptime();
+		delta = now - vhpet->countbase_sbt;
+		KASSERT(delta >= 0, ("vhpet_counter: uptime went backwards: "
+		    "%#lx to %#lx", vhpet->countbase_sbt, now));
+		val += delta / vhpet->freq_sbt;
+		if (nowptr != NULL)
+			*nowptr = now;
+	} else {
 		/*
-		 * Keep track of the last value of the main counter that
-		 * was read by the guest.
+		 * The sbinuptime corresponding to the 'countbase' is
+		 * meaningless when the counter is disabled. Make sure
+		 * that the the caller doesn't want to use it.
 		 */
-		if (latch) {
-			vhpet->counter = val;
-			vhpet->counter_sbt = cur_sbt;
-		}
+		KASSERT(nowptr == NULL, ("vhpet_counter: nowptr must be NULL"));
 	}
-
 	return (val);
 }
 
@@ -305,7 +307,7 @@ vhpet_handler(void *a)
 {
 	int n;
 	uint32_t counter;
-	sbintime_t sbt;
+	sbintime_t now;
 	struct vhpet *vhpet;
 	struct callout *callout;
 	struct vhpet_callout_arg *arg;
@@ -330,14 +332,8 @@ vhpet_handler(void *a)
 	if (!vhpet_counter_enabled(vhpet))
 		panic("vhpet(%p) callout with counter disabled", vhpet);
 
-	counter = vhpet_counter(vhpet, false);
-
-	/* Update the accumulator for periodic timers */
-	if (vhpet->timer[n].comprate != 0)
-		vhpet_adjust_compval(vhpet, n, counter);
-
-	sbt = (vhpet->timer[n].compval - counter) * vhpet->freq_sbt;
-	callout_reset_sbt(callout, sbt, 0, vhpet_handler, arg, 0);
+	counter = vhpet_counter(vhpet, &now);
+	vhpet_start_timer(vhpet, n, counter, now);
 	vhpet_timer_interrupt(vhpet, n);
 done:
 	VHPET_UNLOCK(vhpet);
@@ -345,45 +341,47 @@ done:
 }
 
 static void
-vhpet_stop_timer(struct vhpet *vhpet, int n)
+vhpet_stop_timer(struct vhpet *vhpet, int n, sbintime_t now)
 {
 
+	VM_CTR1(vhpet->vm, "hpet t%d stopped", n);
 	callout_stop(&vhpet->timer[n].callout);
-	vhpet_timer_clear_isr(vhpet, n);
+
+	/*
+	 * If the callout was scheduled to expire in the past but hasn't
+	 * had a chance to execute yet then trigger the timer interrupt
+	 * here. Failing to do so will result in a missed timer interrupt
+	 * in the guest. This is especially bad in one-shot mode because
+	 * the next interrupt has to wait for the counter to wrap around.
+	 */
+	if (vhpet->timer[n].callout_sbt < now) {
+		VM_CTR1(vhpet->vm, "hpet t%d interrupt triggered after "
+		    "stopping timer", n);
+		vhpet_timer_interrupt(vhpet, n);
+	}
 }
 
 static void
-vhpet_start_timer(struct vhpet *vhpet, int n)
+vhpet_start_timer(struct vhpet *vhpet, int n, uint32_t counter, sbintime_t now)
 {
-	uint32_t counter, delta, delta2;
-	sbintime_t sbt;
-
-	counter = vhpet_counter(vhpet, false);
+	sbintime_t delta, precision;
 
 	if (vhpet->timer[n].comprate != 0)
 		vhpet_adjust_compval(vhpet, n, counter);
-
-	delta = vhpet->timer[n].compval - counter;
-
-	/*
-	 * In one-shot mode the guest will typically read the main counter
-	 * before programming the comparator. We can use this heuristic to
-	 * figure out whether the expiration time is in the past. If this
-	 * is the case we schedule the callout to fire immediately.
-	 */
-	if (!vhpet_periodic_timer(vhpet, n)) {
-		delta2 = vhpet->timer[n].compval - vhpet->counter;
-		if (delta > delta2) {
-			VM_CTR3(vhpet->vm, "hpet t%d comparator value is in "
-			    "the past: %u/%u/%u", counter,
-			    vhpet->timer[n].compval, vhpet->counter);
-			delta = 0;
-		}
+	else {
+		/*
+		 * In one-shot mode it is the guest's responsibility to make
+		 * sure that the comparator value is not in the "past". The
+		 * hardware doesn't have any belt-and-suspenders to deal with
+		 * this so we don't either.
+		 */
 	}
 
-	sbt = delta * vhpet->freq_sbt;
-	callout_reset_sbt(&vhpet->timer[n].callout, sbt, 0, vhpet_handler,
-	    &vhpet->timer[n].arg, 0);
+	delta = (vhpet->timer[n].compval - counter) * vhpet->freq_sbt;
+	precision = delta >> tc_precexp;
+	vhpet->timer[n].callout_sbt = now + delta;
+	callout_reset_sbt(&vhpet->timer[n].callout, vhpet->timer[n].callout_sbt,
+	    precision, vhpet_handler, &vhpet->timer[n].arg, C_ABSOLUTE);
 }
 
 static void
@@ -391,18 +389,25 @@ vhpet_start_counting(struct vhpet *vhpet)
 {
 	int i;
 
-	vhpet->counter_sbt = sbinuptime();
-	for (i = 0; i < VHPET_NUM_TIMERS; i++)
-		vhpet_start_timer(vhpet, i);
+	vhpet->countbase_sbt = sbinuptime();
+	for (i = 0; i < VHPET_NUM_TIMERS; i++) {
+		/*
+		 * Restart the timers based on the value of the main counter
+		 * when it stopped counting.
+		 */
+		vhpet_start_timer(vhpet, i, vhpet->countbase,
+		    vhpet->countbase_sbt);
+	}
 }
 
 static void
-vhpet_stop_counting(struct vhpet *vhpet)
+vhpet_stop_counting(struct vhpet *vhpet, uint32_t counter, sbintime_t now)
 {
 	int i;
 
+	vhpet->countbase = counter;
 	for (i = 0; i < VHPET_NUM_TIMERS; i++)
-		vhpet_stop_timer(vhpet, i);
+		vhpet_stop_timer(vhpet, i, now);
 }
 
 static __inline void
@@ -496,7 +501,8 @@ vhpet_mmio_write(void *vm, int vcpuid, uint64_t gpa, uint64_t val, int size,
 {
 	struct vhpet *vhpet;
 	uint64_t data, mask, oldval, val64;
-	uint32_t isr_clear_mask, old_compval, old_comprate;
+	uint32_t isr_clear_mask, old_compval, old_comprate, counter;
+	sbintime_t now, *nowptr;
 	int i, offset;
 
 	vhpet = vm_hpet(vm);
@@ -532,6 +538,14 @@ vhpet_mmio_write(void *vm, int vcpuid, uint64_t gpa, uint64_t val, int size,
 	}
 
 	if (offset == HPET_CONFIG || offset == HPET_CONFIG + 4) {
+		/*
+		 * Get the most recent value of the counter before updating
+		 * the 'config' register. If the HPET is going to be disabled
+		 * then we need to update 'countbase' with the value right
+		 * before it is disabled.
+		 */
+		nowptr = vhpet_counter_enabled(vhpet) ? &now : NULL;
+		counter = vhpet_counter(vhpet, nowptr);
 		oldval = vhpet->config;
 		update_register(&vhpet->config, data, mask);
 		if ((oldval ^ vhpet->config) & HPET_CNF_ENABLE) {
@@ -539,7 +553,7 @@ vhpet_mmio_write(void *vm, int vcpuid, uint64_t gpa, uint64_t val, int size,
 				vhpet_start_counting(vhpet);
 				VM_CTR0(vhpet->vm, "hpet enabled");
 			} else {
-				vhpet_stop_counting(vhpet);
+				vhpet_stop_counting(vhpet, counter, now);
 				VM_CTR0(vhpet->vm, "hpet disabled");
 			}
 		}
@@ -559,9 +573,9 @@ vhpet_mmio_write(void *vm, int vcpuid, uint64_t gpa, uint64_t val, int size,
 
 	if (offset == HPET_MAIN_COUNTER || offset == HPET_MAIN_COUNTER + 4) {
 		/* Zero-extend the counter to 64-bits before updating it */
-		val64 = vhpet->counter;
+		val64 = vhpet_counter(vhpet, NULL);
 		update_register(&val64, data, mask);
-		vhpet->counter = val64;
+		vhpet->countbase = val64;
 		if (vhpet_counter_enabled(vhpet))
 			vhpet_start_counting(vhpet);
 		goto done;
@@ -604,8 +618,11 @@ vhpet_mmio_write(void *vm, int vcpuid, uint64_t gpa, uint64_t val, int size,
 
 			if (vhpet->timer[i].compval != old_compval ||
 			    vhpet->timer[i].comprate != old_comprate) {
-				if (vhpet_counter_enabled(vhpet))
-					vhpet_start_timer(vhpet, i);
+				if (vhpet_counter_enabled(vhpet)) {
+					counter = vhpet_counter(vhpet, &now);
+					vhpet_start_timer(vhpet, i, counter,
+					    now);
+				}
 			}
 			break;
 		}
@@ -666,7 +683,7 @@ vhpet_mmio_read(void *vm, int vcpuid, uint64_t gpa, uint64_t *rval, int size,
 	}
 
 	if (offset == HPET_MAIN_COUNTER || offset == HPET_MAIN_COUNTER + 4) {
-		data = vhpet_counter(vhpet, true);
+		data = vhpet_counter(vhpet, NULL);
 		goto done;
 	}