vmd: drive i8253 with monotonic clock

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

vmd: drive i8253 with monotonic clock

Scott Cheloha
Hi,

Is it bad if the olatch counts up if the host's wall clock
is set backward?  Or if the olatch makes a big jump for
the same reason?

Serious question: I don't have any EPT hardware to test
this (sorry).

While here, it looks like we can (a) roll the olatch-writing
parts of i8253_do_readback into a loop, and (b) use the
timespecsub macro from sys/time.h, to make the code briefer.

Anyone down to test?

--
Scott Cheloha

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 18 Feb 2018 17:53:48 -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,8 +87,10 @@ 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;
+ int readback_channel[3] = { TIMER_RB_C0, TIMER_RB_C1, TIMER_RB_C2 };
+ int i;
 
  /* bits are inverted here - !TIMER_RB_STATUS == enable chan readback */
  if (data & ~TIMER_RB_STATUS) {
@@ -98,73 +101,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) {
- delta.tv_sec--;
- delta.tv_usec += 1000000;
+ for (i = 0; i < 3; i++) {
+ if (data & readback_channel[i]) {
+ clock_gettime(CLOCK_MONOTONIC, &now);
+ timespecsub(&now, &i8253_channel[i].ts, &delta);
+ ns = delta.tv_sec * 1000000000 + delta.tv_nsec;
+ ticks = ns / NS_PER_TICK;
+ if (i8253_channel[i].start)
+ i8253_channel[i].olatch =
+    i8253_channel[i].start -
+    ticks % i8253_channel[i].start;
+ else
+ i8253_channel[i].olatch = 0;
  }
- if (delta.tv_usec > 1000000) {
- delta.tv_sec++;
- delta.tv_usec -= 1000000;
- }
- ns = delta.tv_usec * 1000 + delta.tv_sec * 1000000000;
- ticks = ns / NS_PER_TICK;
- if (i8253_channel[0].start)
- i8253_channel[0].olatch =
-    i8253_channel[0].start -
-    ticks % i8253_channel[0].start;
- else
- i8253_channel[0].olatch = 0;
- }
-
- 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) {
- delta.tv_sec--;
- delta.tv_usec += 1000000;
- }
- if (delta.tv_usec > 1000000) {
- delta.tv_sec++;
- delta.tv_usec -= 1000000;
- }
- ns = delta.tv_usec * 1000 + delta.tv_sec * 1000000000;
- ticks = ns / NS_PER_TICK;
- if (i8253_channel[1].start)
- i8253_channel[1].olatch =
-    i8253_channel[1].start -
-    ticks % i8253_channel[1].start;
- else
- i8253_channel[1].olatch = 0;
- }
-
- 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) {
- delta.tv_sec--;
- delta.tv_usec += 1000000;
- }
- if (delta.tv_usec > 1000000) {
- delta.tv_sec++;
- delta.tv_usec -= 1000000;
- }
- ns = delta.tv_usec * 1000 + delta.tv_sec * 1000000000;
- ticks = ns / NS_PER_TICK;
- if (i8253_channel[2].start)
- i8253_channel[2].olatch =
-    i8253_channel[2].start -
-    ticks % i8253_channel[2].start;
- else
- i8253_channel[2].olatch = 0;
  }
  }
 }
@@ -188,7 +137,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 +165,9 @@ vcpu_exit_i8253(struct vm_run_params *vr
  * rate.
  */
  if (rw == TIMER_LATCH) {
- gettimeofday(&now, NULL);
- 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) {
- delta.tv_sec--;
- delta.tv_usec += 1000000;
- }
- if (delta.tv_usec > 1000000) {
- delta.tv_sec++;
- delta.tv_usec -= 1000000;
- }
- ns = delta.tv_usec * 1000 +
-    delta.tv_sec * 1000000000;
+ clock_gettime(CLOCK_MONOTONIC, &now);
+ timespecsub(&now, &i8253_channel[sel].ts, &delta);
+ ns = delta.tv_sec * 1000000000 + delta.tv_nsec;
  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 18 Feb 2018 17:53:48 -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 */

Reply | Threaded
Open this post in threaded view
|

Re: vmd: drive i8253 with monotonic clock

Mike Larkin
On Sun, Feb 18, 2018 at 12:00:01PM -0600, Scott Cheloha wrote:

> Hi,
>
> Is it bad if the olatch counts up if the host's wall clock
> is set backward?  Or if the olatch makes a big jump for
> the same reason?
>
> Serious question: I don't have any EPT hardware to test
> this (sorry).
>
> While here, it looks like we can (a) roll the olatch-writing
> parts of i8253_do_readback into a loop, and (b) use the
> timespecsub macro from sys/time.h, to make the code briefer.
>
> Anyone down to test?
>
> --
> Scott Cheloha
>

Before I read through this, can you briefly explain what this is trying
to fix?

-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 18 Feb 2018 17:53:48 -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,8 +87,10 @@ 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;
> + int readback_channel[3] = { TIMER_RB_C0, TIMER_RB_C1, TIMER_RB_C2 };
> + int i;
>  
>   /* bits are inverted here - !TIMER_RB_STATUS == enable chan readback */
>   if (data & ~TIMER_RB_STATUS) {
> @@ -98,73 +101,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) {
> - delta.tv_sec--;
> - delta.tv_usec += 1000000;
> + for (i = 0; i < 3; i++) {
> + if (data & readback_channel[i]) {
> + clock_gettime(CLOCK_MONOTONIC, &now);
> + timespecsub(&now, &i8253_channel[i].ts, &delta);
> + ns = delta.tv_sec * 1000000000 + delta.tv_nsec;
> + ticks = ns / NS_PER_TICK;
> + if (i8253_channel[i].start)
> + i8253_channel[i].olatch =
> +    i8253_channel[i].start -
> +    ticks % i8253_channel[i].start;
> + else
> + i8253_channel[i].olatch = 0;
>   }
> - if (delta.tv_usec > 1000000) {
> - delta.tv_sec++;
> - delta.tv_usec -= 1000000;
> - }
> - ns = delta.tv_usec * 1000 + delta.tv_sec * 1000000000;
> - ticks = ns / NS_PER_TICK;
> - if (i8253_channel[0].start)
> - i8253_channel[0].olatch =
> -    i8253_channel[0].start -
> -    ticks % i8253_channel[0].start;
> - else
> - i8253_channel[0].olatch = 0;
> - }
> -
> - 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) {
> - delta.tv_sec--;
> - delta.tv_usec += 1000000;
> - }
> - if (delta.tv_usec > 1000000) {
> - delta.tv_sec++;
> - delta.tv_usec -= 1000000;
> - }
> - ns = delta.tv_usec * 1000 + delta.tv_sec * 1000000000;
> - ticks = ns / NS_PER_TICK;
> - if (i8253_channel[1].start)
> - i8253_channel[1].olatch =
> -    i8253_channel[1].start -
> -    ticks % i8253_channel[1].start;
> - else
> - i8253_channel[1].olatch = 0;
> - }
> -
> - 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) {
> - delta.tv_sec--;
> - delta.tv_usec += 1000000;
> - }
> - if (delta.tv_usec > 1000000) {
> - delta.tv_sec++;
> - delta.tv_usec -= 1000000;
> - }
> - ns = delta.tv_usec * 1000 + delta.tv_sec * 1000000000;
> - ticks = ns / NS_PER_TICK;
> - if (i8253_channel[2].start)
> - i8253_channel[2].olatch =
> -    i8253_channel[2].start -
> -    ticks % i8253_channel[2].start;
> - else
> - i8253_channel[2].olatch = 0;
>   }
>   }
>  }
> @@ -188,7 +137,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 +165,9 @@ vcpu_exit_i8253(struct vm_run_params *vr
>   * rate.
>   */
>   if (rw == TIMER_LATCH) {
> - gettimeofday(&now, NULL);
> - 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) {
> - delta.tv_sec--;
> - delta.tv_usec += 1000000;
> - }
> - if (delta.tv_usec > 1000000) {
> - delta.tv_sec++;
> - delta.tv_usec -= 1000000;
> - }
> - ns = delta.tv_usec * 1000 +
> -    delta.tv_sec * 1000000000;
> + clock_gettime(CLOCK_MONOTONIC, &now);
> + timespecsub(&now, &i8253_channel[sel].ts, &delta);
> + ns = delta.tv_sec * 1000000000 + delta.tv_nsec;
>   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 18 Feb 2018 17:53:48 -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 */

Reply | Threaded
Open this post in threaded view
|

Re: vmd: drive i8253 with monotonic clock

Scott Cheloha
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:
> > Hi,
> >
> > Is it bad if the olatch counts up if the host's wall clock
> > is set backward?  Or if the olatch makes a big jump for
> > the same reason?
> >
> > Serious question: I don't have any EPT hardware to test
> > this (sorry).
> >
> > While here, it looks like we can (a) roll the olatch-writing
> > parts of i8253_do_readback into a loop, and (b) use the
> > timespecsub macro from sys/time.h, to make the code briefer.
> >
> > Anyone down to test?
> >
> > --
> > Scott Cheloha
> >
>
> 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

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 */

Reply | Threaded
Open this post in threaded view
|

Re: vmd: drive i8253 with monotonic clock

Mike Larkin
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:
> > > Hi,
> > >
> > > Is it bad if the olatch counts up if the host's wall clock
> > > is set backward?  Or if the olatch makes a big jump for
> > > the same reason?
> > >
> > > Serious question: I don't have any EPT hardware to test
> > > this (sorry).
> > >
> > > While here, it looks like we can (a) roll the olatch-writing
> > > parts of i8253_do_readback into a loop, and (b) use the
> > > timespecsub macro from sys/time.h, to make the code briefer.
> > >
> > > Anyone down to test?
> > >
> > > --
> > > Scott Cheloha
> > >
> >
> > 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..

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 */

Reply | Threaded
Open this post in threaded view
|

Re: vmd: drive i8253 with monotonic clock

Scott Cheloha
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 */

Reply | Threaded
Open this post in threaded view
|

Re: vmd: drive i8253 with monotonic clock

Scott Cheloha
1 month bump.

> On Mar 9, 2018, at 2:31 PM, Scott Cheloha <[hidden email]> 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 */

Reply | Threaded
Open this post in threaded view
|

Re: vmd: drive i8253 with monotonic clock

Mike Larkin
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 <[hidden email]> 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 */
>