Date: Tue, 20 May 2014 21:05:37 +0000 (UTC) From: John Baldwin <jhb@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-10@freebsd.org Subject: svn commit: r266477 - stable/10/sys/amd64/vmm/io Message-ID: <201405202105.s4KL5bLt090083@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: jhb Date: Tue May 20 21:05:36 2014 New Revision: 266477 URL: http://svnweb.freebsd.org/changeset/base/266477 Log: MFC 260237: 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. Modified: stable/10/sys/amd64/vmm/io/vhpet.c Directory Properties: stable/10/ (props changed) Modified: stable/10/sys/amd64/vmm/io/vhpet.c ============================================================================== --- stable/10/sys/amd64/vmm/io/vhpet.c Tue May 20 20:30:28 2014 (r266476) +++ stable/10/sys/amd64/vmm/io/vhpet.c Tue May 20 21:05:36 2014 (r266477) @@ -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 *vhp } 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, u { 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, u } 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, u 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, u 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, u 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, ui } if (offset == HPET_MAIN_COUNTER || offset == HPET_MAIN_COUNTER + 4) { - data = vhpet_counter(vhpet, true); + data = vhpet_counter(vhpet, NULL); goto done; }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201405202105.s4KL5bLt090083>