kvm: i8254: Fix conversion of in-kernel to userspace state

PATCH Applied Jan Kiszka 2 participants 4 messages Jun 6, 2012 – Jun 11, 2012 KVM

Share & tools

Jan Kiszka

author

2012-06-06 14:28 UTC #0 lore raw

PATCH 1/1 kvm: i8254: Fix conversion of in-kernel to userspace state

Due to a offset between the clock used to generate the in-kernel count_load_time (CLOCK_MONOTONIC) and the clock used for processing this in userspace (vm_clock), reading back the output of PIT channel 2 via port 0x61 was broken. One use cases that suffered from it was the CPU frequency calibration of SeaBIOS, which also affected IDE/AHCI timeouts.

This fixes it by calibrating the offset between both clocks on kvm_pit_get and adjusting the kernel value before saving it in the userspace state. As the calibration only works while the vm_clock is running, we cache the in-kernel state across stopped phases.

Signed-off-by Jan Kiszka <[email protected]>

---

Should go into 1.1 as well if it's fine for master.

 hw/kvm/i8254.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 52 insertions(+), 5 deletions(-)

hw/kvm/i8254.c +52 -5

index bb5fe07..04333d8 100644
@@ -23,31 +23,63 @@
  * THE SOFTWARE.
  */
 #include "qemu-timer.h"
+#include "sysemu.h"
 #include "hw/i8254.h"
 #include "hw/i8254_internal.h"
 #include "kvm.h"
 
 #define KVM_PIT_REINJECT_BIT 0
 
+#define CALIBRATION_ROUNDS   3
+
 typedef struct KVMPITState {
     PITCommonState pit;
     LostTickPolicy lost_tick_policy;
+    bool state_valid;
 } KVMPITState;
 
-static void kvm_pit_get(PITCommonState *s)
+static int64_t abs64(int64_t v)
 {
+    return v < 0 ? -v : v;
+}
+
+static void kvm_pit_get(PITCommonState *pit)
+{
+    KVMPITState *s = DO_UPCAST(KVMPITState, pit, pit);
     struct kvm_pit_state2 kpit;
     struct kvm_pit_channel_state *kchan;
     struct PITChannelState *sc;
+    int64_t offset, clock_offset;
+    struct timespec ts;
     int i, ret;
 
+    if (s->state_valid) {
+        return;
+    }
+
+    /*
+     * Measure the delta between CLOCK_MONOTONIC, the base used for
+     * kvm_pit_channel_state::count_load_time, and vm_clock. Take the
+     * minimum of several samples to filter out scheduling noise.
+     */
+    clock_offset = LLONG_MAX;
+    for (i = 0; i < CALIBRATION_ROUNDS; i++) {
+        offset = qemu_get_clock_ns(vm_clock);
+        clock_gettime(CLOCK_MONOTONIC, &ts);
+        offset -= ts.tv_nsec;
+        offset -= (int64_t)ts.tv_sec * 1000000000;
+        if (abs64(offset) < abs64(clock_offset)) {
+            clock_offset = offset;
+        }
+    }
+
     if (kvm_has_pit_state2()) {
         ret = kvm_vm_ioctl(kvm_state, KVM_GET_PIT2, &kpit);
         if (ret < 0) {
             fprintf(stderr, "KVM_GET_PIT2 failed: %s\n", strerror(ret));
             abort();
         }
-        s->channels[0].irq_disabled = kpit.flags & KVM_PIT_FLAGS_HPET_LEGACY;
+        pit->channels[0].irq_disabled = kpit.flags & KVM_PIT_FLAGS_HPET_LEGACY;
     } else {
         /*
          * kvm_pit_state2 is superset of kvm_pit_state struct,

@@ -61,7 +93,7 @@ static void kvm_pit_get(PITCommonState *s)
     }
     for (i = 0; i < 3; i++) {
         kchan = &kpit.channels[i];
-        sc = &s->channels[i];
+        sc = &pit->channels[i];
         sc->count = kchan->count;
         sc->latched_count = kchan->latched_count;
         sc->count_latched = kchan->count_latched;

@@ -74,10 +106,10 @@ static void kvm_pit_get(PITCommonState *s)
         sc->mode = kchan->mode;
         sc->bcd = kchan->bcd;
         sc->gate = kchan->gate;
-        sc->count_load_time = kchan->count_load_time;
+        sc->count_load_time = kchan->count_load_time + clock_offset;
     }
 
-    sc = &s->channels[0];
+    sc = &pit->channels[0];
     sc->next_transition_time =
         pit_get_next_transition_time(sc, sc->count_load_time);
 }

@@ -173,6 +205,19 @@ static void kvm_pit_irq_control(void *opaque, int n, int enable)
     kvm_pit_put(pit);
 }
 
+static void kvm_pit_vm_state_change(void *opaque, int running,
+                                    RunState state)
+{
+    KVMPITState *s = opaque;
+
+    if (running) {
+        s->state_valid = false;
+    } else {
+        kvm_pit_get(&s->pit);
+        s->state_valid = true;
+    }
+}
+
 static int kvm_pit_initfn(PITCommonState *pit)
 {
     KVMPITState *s = DO_UPCAST(KVMPITState, pit, pit);

@@ -215,6 +260,8 @@ static int kvm_pit_initfn(PITCommonState *pit)
 
     qdev_init_gpio_in(&pit->dev.qdev, kvm_pit_irq_control, 1);
 
+    qemu_add_vm_change_state_handler(kvm_pit_vm_state_change, s);
+
     return 0;
 }

Signature · 1 line

1.7.3.4

Avi Kivity

Applied

2012-06-11 10:07 UTC #1 lore raw

Quoted 10 lines · Jan Kiszka

Due to a offset between the clock used to generate the in-kernel count_load_time (CLOCK_MONOTONIC) and the clock used for processing this in userspace (vm_clock), reading back the output of PIT channel 2 via port 0x61 was broken. One use cases that suffered from it was the CPU frequency calibration of SeaBIOS, which also affected IDE/AHCI timeouts.
This fixes it by calibrating the offset between both clocks on kvm_pit_get and adjusting the kernel value before saving it in the userspace state. As the calibration only works while the vm_clock is running, we cache the in-kernel state across stopped phases.

Applied, thanks.

Quoted 1 line

+ clock_offset = LLONG_MAX;

INT64_MAX would me more strictly correct, but in practice it makes no difference.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2012-06-11 10:30 UTC #2 lore raw

PATCH 1/1 KVM: i8254: Clean up limit constant

Quoted 12 lines · Avi Kivity

Quoted 10 lines · Jan Kiszka
Due to a offset between the clock used to generate the in-kernel count_load_time (CLOCK_MONOTONIC) and the clock used for processing this in userspace (vm_clock), reading back the output of PIT channel 2 via port 0x61 was broken. One use cases that suffered from it was the CPU frequency calibration of SeaBIOS, which also affected IDE/AHCI timeouts.
This fixes it by calibrating the offset between both clocks on kvm_pit_get and adjusting the kernel value before saving it in the userspace state. As the calibration only works while the vm_clock is running, we cache the in-kernel state across stopped phases.

Quoted 6 lines

Applied, thanks.
Quoted 1 line
+ clock_offset = LLONG_MAX;
INT64_MAX would me more strictly correct, but in practice it makes no difference.

Was looking for this, just not long enough. Need to print some cheat sheet. However, let's clean this up immediately:

---8<---

clock_offset is int64_t.

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/kvm/i8254.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

hw/kvm/i8254.c +1 -1

index 04333d8..c5d3711 100644
@@ -62,7 +62,7 @@ static void kvm_pit_get(PITCommonState *pit)
      * kvm_pit_channel_state::count_load_time, and vm_clock. Take the
      * minimum of several samples to filter out scheduling noise.
      */
-    clock_offset = LLONG_MAX;
+    clock_offset = INT64_MAX;
     for (i = 0; i < CALIBRATION_ROUNDS; i++) {
         offset = qemu_get_clock_ns(vm_clock);
         clock_gettime(CLOCK_MONOTONIC, &ts);

Signature · 1 line

1.7.3.4

Avi Kivity

2012-06-11 11:18 UTC #3 lore raw

Re: [PATCH] KVM: i8254: Clean up limit constant

Quoted 20 lines · Jan Kiszka

Quoted 12 lines · Avi Kivity
Quoted 10 lines · Jan Kiszka
Due to a offset between the clock used to generate the in-kernel count_load_time (CLOCK_MONOTONIC) and the clock used for processing this in userspace (vm_clock), reading back the output of PIT channel 2 via port 0x61 was broken. One use cases that suffered from it was the CPU frequency calibration of SeaBIOS, which also affected IDE/AHCI timeouts.
This fixes it by calibrating the offset between both clocks on kvm_pit_get and adjusting the kernel value before saving it in the userspace state. As the calibration only works while the vm_clock is running, we cache the in-kernel state across stopped phases.
Quoted 6 lines
Applied, thanks.
Quoted 1 line
+ clock_offset = LLONG_MAX;
INT64_MAX would me more strictly correct, but in practice it makes no difference.

Quoted 7 lines

Was looking for this, just not long enough. Need to print some cheat sheet. However, let's clean this up immediately:
---8<---
clock_offset is int64_t.

Thanks, folded.

Signature · 1 line

error compiling committee.c: too many arguments to function