On Sun, Feb 18, 2018 at 09:34:50PM -0800, Mike Larkin wrote:
> On Sun, Feb 18, 2018 at 07:42:10PM -0600, Scott Cheloha wrote:
> > On Sun, Feb 18, 2018 at 02:44:43PM -0800, Mike Larkin wrote:
> > > On Sun, Feb 18, 2018 at 12:00:01PM -0600, Scott Cheloha wrote:
> > > > [...]
> > >
> > > Before I read through this, can you briefly explain what this is trying
> > > to fix?
> >
> > The i8253 is a down counter. If we drive it with the system wall clock
> > it could "count up" or make relatively big jumps if the superuser changes
> > the host's wall clock with date(1). Driving it with the monotonic clock
> > should, in principle, ensure it always decrements and that it decrements
> > relatively evenly.
> >
> > I'm not sure what exactly the guest does if the output latch on the PIC
> > increments or jumps between two readbacks, though.
> >
> > Here's a diff without any refactoring. But there's bugs in your timeval
> > normalization code [1], so I think using the subtraction macros in
> > sys/time.h
> > would be a good idea, eventually.
> >
> > -Scott
> >
> > [1] tv_usec > 1000000
> >
> > should be
> >
> > tv_usec >= 1000000
> >
>
> Alright, seems like this is an improvement at least in readability. I don't
> believe that the jumping around of the PIT counter would cause serious issues
> even if it went backwards (aside from perhaps keeping slightly less accurate
> time but we know we have issues there with reading the PIT anyway, due to its
> very small period).
>
> Thanks for spotting the tv_usec thing also.
>
> I'd like to get a few reports that this doesn't break anything before oks
> though. I'll load it on my x230 where I run a bunch of VMs and see what
> happens..
Did this work?
-Scott
> PS, I realize this is w/o refactoring, I like the other one more so that's
> the one I'll use
>
> -ml
>
>
>
> > Index: usr.sbin/vmd/i8253.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/vmd/i8253.c,v
> > retrieving revision 1.17
> > diff -u -p -r1.17 i8253.c
> > --- usr.sbin/vmd/i8253.c 14 Aug 2017 19:46:44 -0000 1.17
> > +++ usr.sbin/vmd/i8253.c 19 Feb 2018 01:36:44 -0000
> > @@ -25,6 +25,7 @@
> > #include <event.h>
> > #include <string.h>
> > #include <stddef.h>
> > +#include <time.h>
> > #include <unistd.h>
> >
> > #include "i8253.h"
> > @@ -53,7 +54,7 @@ void
> > i8253_init(uint32_t vm_id)
> > {
> > memset(&i8253_channel, 0, sizeof(struct i8253_channel));
> > - gettimeofday(&i8253_channel[0].tv, NULL);
> > + clock_gettime(CLOCK_MONOTONIC, &i8253_channel[0].ts);
> > i8253_channel[0].start = 0xFFFF;
> > i8253_channel[0].mode = TIMER_INTTC;
> > i8253_channel[0].last_r = 1;
> > @@ -86,7 +87,7 @@ i8253_init(uint32_t vm_id)
> > void
> > i8253_do_readback(uint32_t data)
> > {
> > - struct timeval now, delta;
> > + struct timespec now, delta;
> > uint64_t ns, ticks;
> >
> > /* bits are inverted here - !TIMER_RB_STATUS == enable chan readback */
> > @@ -99,19 +100,19 @@ i8253_do_readback(uint32_t data)
> > /* !TIMER_RB_COUNT == enable counter readback */
> > if (data & ~TIMER_RB_COUNT) {
> > if (data & TIMER_RB_C0) {
> > - gettimeofday(&now, NULL);
> > - delta.tv_sec = now.tv_sec - i8253_channel[0].tv.tv_sec;
> > - delta.tv_usec = now.tv_usec -
> > - i8253_channel[0].tv.tv_usec;
> > - if (delta.tv_usec < 0) {
> > + clock_gettime(CLOCK_MONOTONIC, &now);
> > + delta.tv_sec = now.tv_sec - i8253_channel[0].ts.tv_sec;
> > + delta.tv_nsec = now.tv_nsec -
> > + i8253_channel[0].ts.tv_nsec;
> > + if (delta.tv_nsec < 0) {
> > delta.tv_sec--;
> > - delta.tv_usec += 1000000;
> > + delta.tv_nsec += 1000000000;
> > }
> > - if (delta.tv_usec > 1000000) {
> > + if (delta.tv_nsec >= 1000000000) {
> > delta.tv_sec++;
> > - delta.tv_usec -= 1000000;
> > + delta.tv_nsec -= 1000000000;
> > }
> > - ns = delta.tv_usec * 1000 + delta.tv_sec * 1000000000;
> > + ns = delta.tv_nsec + delta.tv_sec * 1000000000;
> > ticks = ns / NS_PER_TICK;
> > if (i8253_channel[0].start)
> > i8253_channel[0].olatch =
> > @@ -122,19 +123,19 @@ i8253_do_readback(uint32_t data)
> > }
> >
> > if (data & TIMER_RB_C1) {
> > - gettimeofday(&now, NULL);
> > - delta.tv_sec = now.tv_sec - i8253_channel[1].tv.tv_sec;
> > - delta.tv_usec = now.tv_usec -
> > - i8253_channel[1].tv.tv_usec;
> > - if (delta.tv_usec < 0) {
> > + clock_gettime(CLOCK_MONOTONIC, &now);
> > + delta.tv_sec = now.tv_sec - i8253_channel[1].ts.tv_sec;
> > + delta.tv_nsec = now.tv_nsec -
> > + i8253_channel[1].ts.tv_nsec;
> > + if (delta.tv_nsec < 0) {
> > delta.tv_sec--;
> > - delta.tv_usec += 1000000;
> > + delta.tv_nsec += 1000000000;
> > }
> > - if (delta.tv_usec > 1000000) {
> > + if (delta.tv_nsec >= 1000000000) {
> > delta.tv_sec++;
> > - delta.tv_usec -= 1000000;
> > + delta.tv_nsec -= 1000000000;
> > }
> > - ns = delta.tv_usec * 1000 + delta.tv_sec * 1000000000;
> > + ns = delta.tv_nsec + delta.tv_sec * 1000000000;
> > ticks = ns / NS_PER_TICK;
> > if (i8253_channel[1].start)
> > i8253_channel[1].olatch =
> > @@ -145,19 +146,19 @@ i8253_do_readback(uint32_t data)
> > }
> >
> > if (data & TIMER_RB_C2) {
> > - gettimeofday(&now, NULL);
> > - delta.tv_sec = now.tv_sec - i8253_channel[2].tv.tv_sec;
> > - delta.tv_usec = now.tv_usec -
> > - i8253_channel[2].tv.tv_usec;
> > - if (delta.tv_usec < 0) {
> > + clock_gettime(CLOCK_MONOTONIC, &now);
> > + delta.tv_sec = now.tv_sec - i8253_channel[2].ts.tv_sec;
> > + delta.tv_nsec = now.tv_nsec -
> > + i8253_channel[2].ts.tv_nsec;
> > + if (delta.tv_nsec < 0) {
> > delta.tv_sec--;
> > - delta.tv_usec += 1000000;
> > + delta.tv_nsec += 1000000000;
> > }
> > - if (delta.tv_usec > 1000000) {
> > + if (delta.tv_nsec >= 1000000000) {
> > delta.tv_sec++;
> > - delta.tv_usec -= 1000000;
> > + delta.tv_nsec -= 1000000000;
> > }
> > - ns = delta.tv_usec * 1000 + delta.tv_sec * 1000000000;
> > + ns = delta.tv_nsec + delta.tv_sec * 1000000000;
> > ticks = ns / NS_PER_TICK;
> > if (i8253_channel[2].start)
> > i8253_channel[2].olatch =
> > @@ -188,7 +189,7 @@ vcpu_exit_i8253(struct vm_run_params *vr
> > uint32_t out_data;
> > uint8_t sel, rw, data, mode;
> > uint64_t ns, ticks;
> > - struct timeval now, delta;
> > + struct timespec now, delta;
> > union vm_exit *vei = vrp->vrp_exit;
> >
> > get_input_data(vei, &out_data);
> > @@ -216,21 +217,20 @@ vcpu_exit_i8253(struct vm_run_params *vr
> > * rate.
> > */
> > if (rw == TIMER_LATCH) {
> > - gettimeofday(&now, NULL);
> > + clock_gettime(CLOCK_MONOTONIC, &now);
> > delta.tv_sec = now.tv_sec -
> > - i8253_channel[sel].tv.tv_sec;
> > - delta.tv_usec = now.tv_usec -
> > - i8253_channel[sel].tv.tv_usec;
> > - if (delta.tv_usec < 0) {
> > + i8253_channel[sel].ts.tv_sec;
> > + delta.tv_nsec = now.tv_nsec -
> > + i8253_channel[sel].ts.tv_nsec;
> > + if (delta.tv_nsec < 0) {
> > delta.tv_sec--;
> > - delta.tv_usec += 1000000;
> > + delta.tv_nsec += 1000000000;
> > }
> > - if (delta.tv_usec > 1000000) {
> > + if (delta.tv_nsec >= 1000000000) {
> > delta.tv_sec++;
> > - delta.tv_usec -= 1000000;
> > + delta.tv_nsec -= 1000000000;
> > }
> > - ns = delta.tv_usec * 1000 +
> > - delta.tv_sec * 1000000000;
> > + ns = delta.tv_nsec + delta.tv_sec * 1000000000;
> > ticks = ns / NS_PER_TICK;
> > if (i8253_channel[sel].start) {
> > i8253_channel[sel].olatch =
> > Index: usr.sbin/vmd/i8253.h
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/vmd/i8253.h,v
> > retrieving revision 1.7
> > diff -u -p -r1.7 i8253.h
> > --- usr.sbin/vmd/i8253.h 9 Jul 2017 00:51:40 -0000 1.7
> > +++ usr.sbin/vmd/i8253.h 19 Feb 2018 01:36:44 -0000
> > @@ -29,7 +29,7 @@
> >
> > /* i8253 registers */
> > struct i8253_channel {
> > - struct timeval tv; /* timer start time */
> > + struct timespec ts; /* timer start time */
> > uint16_t start; /* starting value */
> > uint16_t olatch; /* output latch */
> > uint16_t ilatch; /* input latch */
