On Fri, Apr 06, 2018 at 06:46:22PM -0500, Scott Cheloha wrote: > 1 month bump. >
Sorry for the delay. ok mlarkin@ > > On Mar 9, 2018, at 2:31 PM, Scott Cheloha <[email protected]> wrote: > > > >> 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 */ >
