[PATCH] tsc: use kvmclock for calibration

[Gerd Hoffmann]

Use kvmclock for tsc calibration when running on kvm.  Without this the
tsc frequency calibrated by seabios can be *way* off in case the virtual
machine is booted on a loaded host.  I've seen seabios calibrating 27
instead of ca. 2800 MHz, resulting in timeouts being to short by factor
100.  Which in turn leads to disk I/O errors due to timeouts, especially
as I/O requests tend to take a bit longer than usual on a loaded box ...

Signed-off-by: Gerd Hoffmann 
---
 src/clock.c    |    9 +++++
 src/paravirt.c |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/paravirt.h |    1 +
 3 files changed, 100 insertions(+), 0 deletions(-)

diff --git a/src/clock.c b/src/clock.c
index 69e9f17..5883b1a 100644
--- a/src/clock.c
+++ b/src/clock.c
@@ -13,6 +13,7 @@
 #include "bregs.h" // struct bregs
 #include "biosvar.h" // GET_GLOBAL
 #include "usb-hid.h" // usb_check_event
+#include "paravirt.h" // kvm clock

// RTC register flags
 #define RTC_A_UIP 0x80
@@ -80,6 +81,14 @@ calibrate_tsc(void)
         return;
     }

+    if (kvm_para_available()) {
+        u32 khz = kvm_tsc_khz();
+        if (khz != 0) {
+            SET_GLOBAL(cpu_khz, khz);
+            return;
+        }
+    }
+
     // Setup "timer2"
     u8 orig = inb(PORT_PS2_CTRLB);
     outb((orig & ~PPCB_SPKR) | PPCB_T2GATE, PORT_PS2_CTRLB);
diff --git a/src/paravirt.c b/src/paravirt.c
index 2a98d53..942ce11 100644
--- a/src/paravirt.c
+++ b/src/paravirt.c
@@ -12,6 +12,7 @@
 #include "ioport.h" // outw
 #include "paravirt.h" // qemu_cfg_port_probe
 #include "smbios.h" // struct smbios_structure_header
+#include "biosvar.h" // GET_GLOBAL

int qemu_cfg_present;

@@ -346,3 +347,92 @@ void qemu_cfg_romfile_setup(void)
         dprintf(3, "Found fw_cfg file: %s (size=%d)\n", file->name, file->size);
     }
 }
+
+#define KVM_CPUID_SIGNATURE       0x40000000
+#define KVM_CPUID_FEATURES        0x40000001
+#define KVM_FEATURE_CLOCKSOURCE            0
+#define KVM_FEATURE_CLOCKSOURCE2           3
+#define MSR_KVM_SYSTEM_TIME             0x12
+#define MSR_KVM_SYSTEM_TIME_NEW   0x4b564d01
+
+struct pvclock_vcpu_time_info {
+	u32   version;
+	u32   pad0;
+	u64   tsc_timestamp;
+	u64   system_time;
+	u32   tsc_to_system_mul;
+	s8    tsc_shift;
+	u8    flags;
+	u8    pad[2];
+} PACKED;
+
+/*
+ * do_div() is NOT a C function. It wants to return
+ * two values (the quotient and the remainder), but
+ * since that doesn't work very well in C, what it
+ * does is:
+ *
+ * - modifies the 64-bit dividend _in_place_
+ * - returns the 32-bit remainder
+ *
+ * This ends up being the most efficient "calling
+ * convention" on x86.
+ */
+#define do_div(n, base)                                                 \
+    ({                                                                  \
+        unsigned long __upper, __low, __high, __mod, __base;            \
+        __base = (base);                                                \
+        asm("" : "=a" (__low), "=d" (__high) : "A" (n));                \
+        __upper = __high;                                               \
+        if (__high) {                                                   \
+            __upper = __high % (__base);                                \
+            __high = __high / (__base);                                 \
+        }                                                               \
+        asm("divl %2" : "=a" (__low), "=d" (__mod)                      \
+            : "rm" (__base), "0" (__low), "1" (__upper));               \
+        asm("" : "=A" (n) : "a" (__low), "d" (__high));                 \
+        __mod;                                                          \
+    })
+
+static u64 pvclock_tsc_khz(struct pvclock_vcpu_time_info *src)
+{
+    u64 pv_tsc_khz = 1000000ULL << 32;
+
+    do_div(pv_tsc_khz, src->tsc_to_system_mul);
+    if (src->tsc_shift < 0)
+        pv_tsc_khz <<= -src->tsc_shift;
+    else
+        pv_tsc_khz >>= src->tsc_shift;
+    return pv_tsc_khz;
+}
+
+u64 kvm_tsc_khz(void)
+{
+    u32 eax, ebx, ecx, edx, msr;
+    struct pvclock_vcpu_time_info time;
+    u32 addr = (u32)(&time);
+    u64 khz;
+
+    /* check presence and figure msr number */
+    cpuid(KVM_CPUID_FEATURES, &eax, &ebx, &ecx, &edx);
+    if (eax & KVM_FEATURE_CLOCKSOURCE2) {
+        msr = MSR_KVM_SYSTEM_TIME_NEW;
+    } else if (eax & KVM_FEATURE_CLOCKSOURCE) {
+        msr = MSR_KVM_SYSTEM_TIME;
+    } else {
+        return 0;
+    }
+
+    /* ask kvm hypervisor to fill struct */
+    memset(&time, 0, sizeof(time));
+    wrmsr(msr, addr | 1);
+    wrmsr(msr, 0);
+    if (time.version < 2 || time.tsc_to_system_mul == 0)
+        return 0;
+
+    /* go figure tsc frequency */
+    khz = pvclock_tsc_khz(&time);
+    dprintf(1, "Using kvmclock, msr 0x%x, tsc %d MHz\n",
+            msr, (u32)khz / 1000);
+    return khz;
+}
diff --git a/src/paravirt.h b/src/paravirt.h
index a284c41..eedfcc3 100644
--- a/src/paravirt.h
+++ b/src/paravirt.h
@@ -27,6 +27,7 @@ static inline int kvm_para_available(void)

return 0;
 }
+extern u64 kvm_tsc_khz(void);

#define QEMU_CFG_SIGNATURE              0x00
 #define QEMU_CFG_ID                     0x01

[Avi Kivity]

On 08/09/2012 02:57 PM, Gerd Hoffmann wrote:
> Use kvmclock for tsc calibration when running on kvm.  Without this the
> tsc frequency calibrated by seabios can be *way* off in case the virtual
> machine is booted on a loaded host.  I've seen seabios calibrating 27
> instead of ca. 2800 MHz, resulting in timeouts being to short by factor
> 100.  Which in turn leads to disk I/O errors due to timeouts, especially
> as I/O requests tend to take a bit longer than usual on a loaded box ...
> +struct pvclock_vcpu_time_info {
> u32   version;
> u32   pad0;
> u64   tsc_timestamp;
> u64   system_time;
> u32   tsc_to_system_mul;
> s8    tsc_shift;
> u8    flags;
> u8    pad[2];
>
> +} PACKED;
>
> +u64 kvm_tsc_khz(void)
> +{
> u32 eax, ebx, ecx, edx, msr;
> struct pvclock_vcpu_time_info time;
> u32 addr = (u32)(&time);
> u64 khz;
> /* check presence and figure msr number */
> cpuid(KVM_CPUID_FEATURES, &eax, &ebx, &ecx, &edx);
> if (eax & KVM_FEATURE_CLOCKSOURCE2) {
>    msr = MSR_KVM_SYSTEM_TIME_NEW;
> } else if (eax & KVM_FEATURE_CLOCKSOURCE) {
>    msr = MSR_KVM_SYSTEM_TIME;
> } else {
>    return 0;
> }
> /* ask kvm hypervisor to fill struct */
> memset(&time, 0, sizeof(time));
> wrmsr(msr, addr | 1);

How can this work?  There is a 64-byte alignment requirement.
> wrmsr(msr, 0);
> if (time.version < 2 || time.tsc_to_system_mul == 0)
>    return 0;
> /* go figure tsc frequency */
> khz = pvclock_tsc_khz(&time);
> dprintf(1, "Using kvmclock, msr 0x%x, tsc %d MHz\n",
>        msr, (u32)khz / 1000);
> return khz;

That's a meaningless number.  You can be migrated to a cpu or a machine
with very different tsc.

You want accurate time on kvm, don't use the tsc.

[Fred .]

It should be kHz not khz.

-            msr, (u32)khz / 1000);
+            msr, (u32)kHz / 1000);

On Thu, Aug 9, 2012 at 2:53 PM, Avi Kivity  wrote:
> On 08/09/2012 02:57 PM, Gerd Hoffmann wrote:
> > Use kvmclock for tsc calibration when running on kvm.  Without this the
> > tsc frequency calibrated by seabios can be *way* off in case the virtual
> > machine is booted on a loaded host.  I've seen seabios calibrating 27
> > instead of ca. 2800 MHz, resulting in timeouts being to short by factor
> > 100.  Which in turn leads to disk I/O errors due to timeouts, especially
> > as I/O requests tend to take a bit longer than usual on a loaded box ...
> > +struct pvclock_vcpu_time_info {
> > u32   version;
> > u32   pad0;
> > u64   tsc_timestamp;
> > u64   system_time;
> > u32   tsc_to_system_mul;
> > s8    tsc_shift;
> > u8    flags;
> > u8    pad[2];
> >
> > +} PACKED;
> >
> > +u64 kvm_tsc_khz(void)
> > +{
> > u32 eax, ebx, ecx, edx, msr;
> > struct pvclock_vcpu_time_info time;
> > u32 addr = (u32)(&time);
> > u64 khz;
> > /* check presence and figure msr number */
> > cpuid(KVM_CPUID_FEATURES, &eax, &ebx, &ecx, &edx);
> > if (eax & KVM_FEATURE_CLOCKSOURCE2) {
> >    msr = MSR_KVM_SYSTEM_TIME_NEW;
> > } else if (eax & KVM_FEATURE_CLOCKSOURCE) {
> >    msr = MSR_KVM_SYSTEM_TIME;
> > } else {
> >    return 0;
> > }
> > /* ask kvm hypervisor to fill struct */
> > memset(&time, 0, sizeof(time));
> > wrmsr(msr, addr | 1);
>
> How can this work?  There is a 64-byte alignment requirement.
> > wrmsr(msr, 0);
> > if (time.version < 2 || time.tsc_to_system_mul == 0)
> >    return 0;
> > /* go figure tsc frequency */
> > khz = pvclock_tsc_khz(&time);
> > dprintf(1, "Using kvmclock, msr 0x%x, tsc %d MHz\n",
> >        msr, (u32)khz / 1000);
> > return khz;
>
> That's a meaningless number.  You can be migrated to a cpu or a machine
> with very different tsc.
>
> You want accurate time on kvm, don't use the tsc.
>
> SeaBIOS mailing list
> [e-mail]
> http://www.seabios.org/mailman/listinfo/seabios

[Gerd Hoffmann]

Hi,
> > +u64 kvm_tsc_khz(void)
> > +{
> > u32 eax, ebx, ecx, edx, msr;
> > struct pvclock_vcpu_time_info time;
> > u32 addr = (u32)(&time);
> > u64 khz;
> > /* check presence and figure msr number */
> > cpuid(KVM_CPUID_FEATURES, &eax, &ebx, &ecx, &edx);
> > if (eax & KVM_FEATURE_CLOCKSOURCE2) {
> >    msr = MSR_KVM_SYSTEM_TIME_NEW;
> > } else if (eax & KVM_FEATURE_CLOCKSOURCE) {
> >    msr = MSR_KVM_SYSTEM_TIME;
> > } else {
> >    return 0;
> > }
> > /* ask kvm hypervisor to fill struct */
> > memset(&time, 0, sizeof(time));
> > wrmsr(msr, addr | 1);
>
> How can this work?

It did in my testing, although maybe by pure luck ...
> There is a 64-byte alignment requirement.

64 bytes?  Sure?  The whole struct is only 32 bytes in size ...

Easily fixable though, just need to grab some memory with memalign
instead of using the stack.
> > wrmsr(msr, 0);
> > if (time.version < 2 || time.tsc_to_system_mul == 0)
> >    return 0;
> > /* go figure tsc frequency */
> > khz = pvclock_tsc_khz(&time);
> > dprintf(1, "Using kvmclock, msr 0x%x, tsc %d MHz\n",
> >        msr, (u32)khz / 1000);
> > return khz;
>
> That's a meaningless number.  You can be migrated to a cpu or a machine
> with very different tsc.
> You want accurate time on kvm, don't use the tsc.

seabios uses the tsc for timeout calculations only, so it doesn't need
to be 100% accurate.  The order of magnitude should be correct though.
The Linux kernel uses the value for delay loops too, so using it for the
given purpose can't be *that* horrible after all ...

It is certainly an improvement over the current code which tries to
calibrate the tsc and gets totally broken results in case the busy host
happens to schedule the guest in the middle of calibration.

So what do you suggest?  The options I see are:

(1) Use this patch (with alignment issue fixed of course).
  (2) Do a full kvmclock implementation.  Feels a bit like overkill.
  (3) SeaBIOS can fallback to the PIT for timing on machines which
      have no TSC.  We could do that too in case we detect kvm ...

cheers,
  Gerd

[Avi Kivity]

On 08/09/2012 04:57 PM, Gerd Hoffmann wrote:
> Hi,
> > > +u64 kvm_tsc_khz(void)
> > > +{
> > > u32 eax, ebx, ecx, edx, msr;
> > > struct pvclock_vcpu_time_info time;
> > > u32 addr = (u32)(&time);
> > > u64 khz;
> > > /* check presence and figure msr number */
> > > cpuid(KVM_CPUID_FEATURES, &eax, &ebx, &ecx, &edx);
> > > if (eax & KVM_FEATURE_CLOCKSOURCE2) {
> > >    msr = MSR_KVM_SYSTEM_TIME_NEW;
> > > } else if (eax & KVM_FEATURE_CLOCKSOURCE) {
> > >    msr = MSR_KVM_SYSTEM_TIME;
> > > } else {
> > >    return 0;
> > > }
> > > /* ask kvm hypervisor to fill struct */
> > > memset(&time, 0, sizeof(time));
> > > wrmsr(msr, addr | 1);
> >
> > How can this work?
>
> It did in my testing, although maybe by pure luck ...
> > There is a 64-byte alignment requirement.
>
> 64 bytes?  Sure?  The whole struct is only 32 bytes in size ...

er, the documentation says 4 bytes (so stack alignment works).  I
distinctly remember having a large alignment requirement so we don't
cross a page or slot boundary... something's wrong here.
> Easily fixable though, just need to grab some memory with memalign
> instead of using the stack.
> > > wrmsr(msr, 0);
> > > if (time.version < 2 || time.tsc_to_system_mul == 0)
> > >    return 0;
> > > /* go figure tsc frequency */
> > > khz = pvclock_tsc_khz(&time);
> > > dprintf(1, "Using kvmclock, msr 0x%x, tsc %d MHz\n",
> > >        msr, (u32)khz / 1000);
> > > return khz;
> >
> > That's a meaningless number.  You can be migrated to a cpu or a machine
> > with very different tsc.
> > You want accurate time on kvm, don't use the tsc.
>
> seabios uses the tsc for timeout calculations only, so it doesn't need
> to be 100% accurate.  The order of magnitude should be correct though.
> The Linux kernel uses the value for delay loops too, so using it for the
> given purpose can't be *that* horrible after all ...
>
> It is certainly an improvement over the current code which tries to
> calibrate the tsc and gets totally broken results in case the busy host
> happens to schedule the guest in the middle of calibration.
>
> So what do you suggest?  The options I see are:
>
> (1) Use this patch (with alignment issue fixed of course).
>   (2) Do a full kvmclock implementation.  Feels a bit like overkill.
>   (3) SeaBIOS can fallback to the PIT for timing on machines which
>       have no TSC.  We could do that too in case we detect kvm ...

What sort of timeouts are these?  If seconds, maybe the rtc would be best.

[Avi Kivity]

On 08/09/2012 05:01 PM, Avi Kivity wrote:
> On 08/09/2012 04:57 PM, Gerd Hoffmann wrote:
> > Hi,
> > > > +u64 kvm_tsc_khz(void)
> > > > +{
> > > > u32 eax, ebx, ecx, edx, msr;
> > > > struct pvclock_vcpu_time_info time;
> > > > u32 addr = (u32)(&time);
> > > > u64 khz;
> > > > /* check presence and figure msr number */
> > > > cpuid(KVM_CPUID_FEATURES, &eax, &ebx, &ecx, &edx);
> > > > if (eax & KVM_FEATURE_CLOCKSOURCE2) {
> > > >    msr = MSR_KVM_SYSTEM_TIME_NEW;
> > > > } else if (eax & KVM_FEATURE_CLOCKSOURCE) {
> > > >    msr = MSR_KVM_SYSTEM_TIME;
> > > > } else {
> > > >    return 0;
> > > > }
> > > > /* ask kvm hypervisor to fill struct */
> > > > memset(&time, 0, sizeof(time));
> > > > wrmsr(msr, addr | 1);
> > >
> > > How can this work?
> >
> > It did in my testing, although maybe by pure luck ...
> > > There is a 64-byte alignment requirement.
> >
> > 64 bytes?  Sure?  The whole struct is only 32 bytes in size ...
>
> er, the documentation says 4 bytes (so stack alignment works).  I
> distinctly remember having a large alignment requirement so we don't
> cross a page or slot boundary... something's wrong here.

case MSR_KVM_SYSTEM_TIME: {
    	kvmclock_reset(vcpu);

vcpu->arch.time = data;
    	kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);

/* we verify if the enable bit is set... */
    	if (!(data & 1))
    		break;

/* ...but clean it before doing the actual write */
    	vcpu->arch.time_offset = data & ~(PAGE_MASK | 1);

vcpu->arch.time_page =
    			gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT);

if (is_error_page(vcpu->arch.time_page))
    		vcpu->arch.time_page = NULL;

break;

So your tests worked by pure luck, but the bug is in kvm.  We need to
grab two pages here.

[Gerd Hoffmann]

Hi,
> > er, the documentation says 4 bytes (so stack alignment works).  I
> > distinctly remember having a large alignment requirement so we don't
> > cross a page or slot boundary... something's wrong here.
>
> case MSR_KVM_SYSTEM_TIME: {

[ ... ]
> So your tests worked by pure luck, but the bug is in kvm.  We need to
> grab two pages here.

Ok, so better use memalign(32,32) to make sure the struct doesn't cross
a page border ...

cheers,
  Gerd

[Avi Kivity]

On 08/09/2012 05:12 PM, Gerd Hoffmann wrote:
> Hi,
> > > er, the documentation says 4 bytes (so stack alignment works).  I
> > > distinctly remember having a large alignment requirement so we don't
> > > cross a page or slot boundary... something's wrong here.
> >
> > case MSR_KVM_SYSTEM_TIME: {
>
> [ ... ]
> > So your tests worked by pure luck, but the bug is in kvm.  We need to
> > grab two pages here.
>
> Ok, so better use memalign(32,32) to make sure the struct doesn't cross
> a page border ...

No, we need to fix kvm, no need to complicate the guest for that.

[Gerd Hoffmann]

Hi,
> > So what do you suggest?  The options I see are:
> >
> > (1) Use this patch (with alignment issue fixed of course).
> >   (2) Do a full kvmclock implementation.  Feels a bit like overkill.
> >   (3) SeaBIOS can fallback to the PIT for timing on machines which
> >       have no TSC.  We could do that too in case we detect kvm ...
>
> What sort of timeouts are these?  If seconds, maybe the rtc would be best.

All sorts of timeouts, from a few miliseconds to seconds.

The problematic ones are the longer timeouts, which wait for I/O stuff
like disk reads complete.  The stuff with smaller timeouts (like waiting
for AHCI link become ready) tend to finish instantly in kvm.

cheers,
  Gerd

[Avi Kivity]

On 08/09/2012 05:18 PM, Gerd Hoffmann wrote:
> Hi,
> > > So what do you suggest?  The options I see are:
> > >
> > > (1) Use this patch (with alignment issue fixed of course).
> > >   (2) Do a full kvmclock implementation.  Feels a bit like overkill.
> > >   (3) SeaBIOS can fallback to the PIT for timing on machines which
> > >       have no TSC.  We could do that too in case we detect kvm ...
> >
> > What sort of timeouts are these?  If seconds, maybe the rtc would be best.
>
> All sorts of timeouts, from a few miliseconds to seconds.
>
> The problematic ones are the longer timeouts, which wait for I/O stuff
> like disk reads complete.  The stuff with smaller timeouts (like waiting
> for AHCI link become ready) tend to finish instantly in kvm.

That's not guaranteed.  The AHCI adapter might be real hardware.  Or the
emulation may change.

What's wrong with having a full kvmclock implementation?  Instead of
issuing rdtsc call a function pointer.

[Marcelo Tosatti]

On Thu, Aug 09, 2012 at 03:53:24PM +0300, Avi Kivity wrote:
> On 08/09/2012 02:57 PM, Gerd Hoffmann wrote:
> > Use kvmclock for tsc calibration when running on kvm.  Without this the
> > tsc frequency calibrated by seabios can be *way* off in case the virtual
> > machine is booted on a loaded host.  I've seen seabios calibrating 27
> > instead of ca. 2800 MHz, resulting in timeouts being to short by factor
> > 100.  Which in turn leads to disk I/O errors due to timeouts, especially
> > as I/O requests tend to take a bit longer than usual on a loaded box ...
> > +struct pvclock_vcpu_time_info {
> > u32   version;
> > u32   pad0;
> > u64   tsc_timestamp;
> > u64   system_time;
> > u32   tsc_to_system_mul;
> > s8    tsc_shift;
> > u8    flags;
> > u8    pad[2];
> >
> > +} PACKED;
> >
> > +u64 kvm_tsc_khz(void)
> > +{
> > u32 eax, ebx, ecx, edx, msr;
> > struct pvclock_vcpu_time_info time;
> > u32 addr = (u32)(&time);
> > u64 khz;
> > /* check presence and figure msr number */
> > cpuid(KVM_CPUID_FEATURES, &eax, &ebx, &ecx, &edx);
> > if (eax & KVM_FEATURE_CLOCKSOURCE2) {
> >    msr = MSR_KVM_SYSTEM_TIME_NEW;
> > } else if (eax & KVM_FEATURE_CLOCKSOURCE) {
> >    msr = MSR_KVM_SYSTEM_TIME;
> > } else {
> >    return 0;
> > }
> > /* ask kvm hypervisor to fill struct */
> > memset(&time, 0, sizeof(time));
> > wrmsr(msr, addr | 1);
>
> How can this work?  There is a 64-byte alignment requirement.
> > wrmsr(msr, 0);
> > if (time.version < 2 || time.tsc_to_system_mul == 0)
> >    return 0;
> > /* go figure tsc frequency */
> > khz = pvclock_tsc_khz(&time);
> > dprintf(1, "Using kvmclock, msr 0x%x, tsc %d MHz\n",
> >        msr, (u32)khz / 1000);
> > return khz;
>
> That's a meaningless number.  You can be migrated to a cpu or a machine
> with very different tsc.

Thats why there exists hardware tsc frequency scaling and the software
equivalent for that on kvm.
> You want accurate time on kvm, don't use the tsc.

[Marcelo Tosatti]

On Thu, Aug 09, 2012 at 05:20:11PM +0300, Avi Kivity wrote:
> On 08/09/2012 05:18 PM, Gerd Hoffmann wrote:
> > Hi,
> > > > So what do you suggest?  The options I see are:
> > > >
> > > > (1) Use this patch (with alignment issue fixed of course).
> > > >   (2) Do a full kvmclock implementation.  Feels a bit like overkill.
> > > >   (3) SeaBIOS can fallback to the PIT for timing on machines which
> > > >       have no TSC.  We could do that too in case we detect kvm ...
> > >
> > > What sort of timeouts are these?  If seconds, maybe the rtc would be best.
> >
> > All sorts of timeouts, from a few miliseconds to seconds.
> >
> > The problematic ones are the longer timeouts, which wait for I/O stuff
> > like disk reads complete.  The stuff with smaller timeouts (like waiting
> > for AHCI link become ready) tend to finish instantly in kvm.
>
> That's not guaranteed.  The AHCI adapter might be real hardware.  Or the
> emulation may change.
>
> What's wrong with having a full kvmclock implementation?  Instead of
> issuing rdtsc call a function pointer.

Its not necessary (someone is going to maintain the kvmclock frequency
retrieve, which patch is already here, versus maintainance of 
full kvmclock).

Frequency scaling (or the software equivalent: TSC trapping) are
required for other reasons anyway.

[Marcelo Tosatti]

On Thu, Aug 09, 2012 at 05:01:34PM +0300, Avi Kivity wrote:
> On 08/09/2012 04:57 PM, Gerd Hoffmann wrote:
> > Hi,
> > > > +u64 kvm_tsc_khz(void)
> > > > +{
> > > > u32 eax, ebx, ecx, edx, msr;
> > > > struct pvclock_vcpu_time_info time;
> > > > u32 addr = (u32)(&time);
> > > > u64 khz;
> > > > /* check presence and figure msr number */
> > > > cpuid(KVM_CPUID_FEATURES, &eax, &ebx, &ecx, &edx);
> > > > if (eax & KVM_FEATURE_CLOCKSOURCE2) {
> > > >    msr = MSR_KVM_SYSTEM_TIME_NEW;
> > > > } else if (eax & KVM_FEATURE_CLOCKSOURCE) {
> > > >    msr = MSR_KVM_SYSTEM_TIME;
> > > > } else {
> > > >    return 0;
> > > > }
> > > > /* ask kvm hypervisor to fill struct */
> > > > memset(&time, 0, sizeof(time));
> > > > wrmsr(msr, addr | 1);
> > >
> > > How can this work?
> >
> > It did in my testing, although maybe by pure luck ...
> > > There is a 64-byte alignment requirement.
> >
> > 64 bytes?  Sure?  The whole struct is only 32 bytes in size ...
>
> er, the documentation says 4 bytes (so stack alignment works).  I
> distinctly remember having a large alignment requirement so we don't
> cross a page or slot boundary... something's wrong here.
> > Easily fixable though, just need to grab some memory with memalign
> > instead of using the stack.
> > > > wrmsr(msr, 0);
> > > > if (time.version < 2 || time.tsc_to_system_mul == 0)
> > > >    return 0;
> > > > /* go figure tsc frequency */
> > > > khz = pvclock_tsc_khz(&time);
> > > > dprintf(1, "Using kvmclock, msr 0x%x, tsc %d MHz\n",
> > > >        msr, (u32)khz / 1000);
> > > > return khz;
> > >
> > > That's a meaningless number.  You can be migrated to a cpu or a machine
> > > with very different tsc.
> > > You want accurate time on kvm, don't use the tsc.
> >
> > seabios uses the tsc for timeout calculations only, so it doesn't need
> > to be 100% accurate.  The order of magnitude should be correct though.
> > The Linux kernel uses the value for delay loops too, so using it for the
> > given purpose can't be *that* horrible after all ...
> >
> > It is certainly an improvement over the current code which tries to
> > calibrate the tsc and gets totally broken results in case the busy host
> > happens to schedule the guest in the middle of calibration.
> >
> > So what do you suggest?  The options I see are:
> >
> > (1) Use this patch (with alignment issue fixed of course).
> >   (2) Do a full kvmclock implementation.  Feels a bit like overkill.
> >   (3) SeaBIOS can fallback to the PIT for timing on machines which
> >       have no TSC.  We could do that too in case we detect kvm ...
>
> What sort of timeouts are these?  If seconds, maybe the rtc would be best.

I vote for 3 so nobody has to maintain kvmclock code in SeaBIOS and Gerd
can fix the in-kernel PIT issues with GRUB (see Michaels message) while testing.

[Gleb Natapov]

On Thu, Aug 09, 2012 at 04:09:13PM -0300, Marcelo Tosatti wrote:
> On Thu, Aug 09, 2012 at 05:01:34PM +0300, Avi Kivity wrote:
> > On 08/09/2012 04:57 PM, Gerd Hoffmann wrote:
> > > Hi,
> > > > > +u64 kvm_tsc_khz(void)
> > > > > +{
> > > > > u32 eax, ebx, ecx, edx, msr;
> > > > > struct pvclock_vcpu_time_info time;
> > > > > u32 addr = (u32)(&time);
> > > > > u64 khz;
> > > > > /* check presence and figure msr number */
> > > > > cpuid(KVM_CPUID_FEATURES, &eax, &ebx, &ecx, &edx);
> > > > > if (eax & KVM_FEATURE_CLOCKSOURCE2) {
> > > > >    msr = MSR_KVM_SYSTEM_TIME_NEW;
> > > > > } else if (eax & KVM_FEATURE_CLOCKSOURCE) {
> > > > >    msr = MSR_KVM_SYSTEM_TIME;
> > > > > } else {
> > > > >    return 0;
> > > > > }
> > > > > /* ask kvm hypervisor to fill struct */
> > > > > memset(&time, 0, sizeof(time));
> > > > > wrmsr(msr, addr | 1);
> > > >
> > > > How can this work?
> > >
> > > It did in my testing, although maybe by pure luck ...
> > > > There is a 64-byte alignment requirement.
> > >
> > > 64 bytes?  Sure?  The whole struct is only 32 bytes in size ...
> >
> > er, the documentation says 4 bytes (so stack alignment works).  I
> > distinctly remember having a large alignment requirement so we don't
> > cross a page or slot boundary... something's wrong here.
> > > Easily fixable though, just need to grab some memory with memalign
> > > instead of using the stack.
> > > > > wrmsr(msr, 0);
> > > > > if (time.version < 2 || time.tsc_to_system_mul == 0)
> > > > >    return 0;
> > > > > /* go figure tsc frequency */
> > > > > khz = pvclock_tsc_khz(&time);
> > > > > dprintf(1, "Using kvmclock, msr 0x%x, tsc %d MHz\n",
> > > > >        msr, (u32)khz / 1000);
> > > > > return khz;
> > > >
> > > > That's a meaningless number.  You can be migrated to a cpu or a machine
> > > > with very different tsc.
> > > > You want accurate time on kvm, don't use the tsc.
> > >
> > > seabios uses the tsc for timeout calculations only, so it doesn't need
> > > to be 100% accurate.  The order of magnitude should be correct though.
> > > The Linux kernel uses the value for delay loops too, so using it for the
> > > given purpose can't be *that* horrible after all ...
> > >
> > > It is certainly an improvement over the current code which tries to
> > > calibrate the tsc and gets totally broken results in case the busy host
> > > happens to schedule the guest in the middle of calibration.
> > >
> > > So what do you suggest?  The options I see are:
> > >
> > > (1) Use this patch (with alignment issue fixed of course).
> > >   (2) Do a full kvmclock implementation.  Feels a bit like overkill.
> > >   (3) SeaBIOS can fallback to the PIT for timing on machines which
> > >       have no TSC.  We could do that too in case we detect kvm ...
> >
> > What sort of timeouts are these?  If seconds, maybe the rtc would be best.
>
> I vote for 3 so nobody has to maintain kvmclock code in SeaBIOS and Gerd

That or pm timer.
> can fix the in-kernel PIT issues with GRUB (see Michaels message) while testing.

What message exactly?

[Gleb Natapov]

On Fri, Aug 10, 2012 at 10:18:00AM +0300, Gleb Natapov wrote:
> > can fix the in-kernel PIT issues with GRUB (see Michaels message) while testing.
>
> What message exactly?

found it.

[Gerd Hoffmann]

Hi,
> > > (1) Use this patch (with alignment issue fixed of course).
> > >   (2) Do a full kvmclock implementation.  Feels a bit like overkill.
> > >   (3) SeaBIOS can fallback to the PIT for timing on machines which
> > >       have no TSC.  We could do that too in case we detect kvm ...
> >
> > What sort of timeouts are these?  If seconds, maybe the rtc would be best.
>
> I vote for 3 so nobody has to maintain kvmclock code in SeaBIOS and Gerd
> can fix the in-kernel PIT issues with GRUB (see Michaels message) while testing.

(2) turned out to be not too bad when taking a shortcut: Go through an
enable/disable cycle each time we read the clock, then just grab
system_time.  Not that efficient, but should be ok for seabios.  Usually
it checks the clock when sitting around idle, waiting for something to
happen.  And it simplifies the implementation alot as we can just skip
all the tsc frequency & delta calculations.

Draft patch attached.  Comments?

cheers,
  Gerd

[Marcelo Tosatti]

On Fri, Aug 10, 2012 at 10:10:27AM +0200, Gerd Hoffmann wrote:
> Hi,
> > > > (1) Use this patch (with alignment issue fixed of course).
> > > >   (2) Do a full kvmclock implementation.  Feels a bit like overkill.
> > > >   (3) SeaBIOS can fallback to the PIT for timing on machines which
> > > >       have no TSC.  We could do that too in case we detect kvm ...
> > >
> > > What sort of timeouts are these?  If seconds, maybe the rtc would be best.
> >
> > I vote for 3 so nobody has to maintain kvmclock code in SeaBIOS and Gerd
> > can fix the in-kernel PIT issues with GRUB (see Michaels message) while testing.
>
> (2) turned out to be not too bad when taking a shortcut: Go through an
> enable/disable cycle each time we read the clock, then just grab
> system_time.  Not that efficient, but should be ok for seabios.  Usually
> it checks the clock when sitting around idle, waiting for something to
> happen.  And it simplifies the implementation alot as we can just skip
> all the tsc frequency & delta calculations.
>
> Draft patch attached.  Comments?

Given the history of problems with kvmclock, would rather see it not
being used for delays, if possible. Your shortcut gets rid of a class of
problems, but there might be others (...).

Isnt pmtimer ioport usable? 14MHz.

Error handling in kvmclock_init is awkward.

Thanks

[Avi Kivity]

On 08/10/2012 11:10 AM, Gerd Hoffmann wrote:
> Hi,
> > > > > > >   (1) Use this patch (with alignment issue fixed of course).
> > > > > > >   (2) Do a full kvmclock implementation.  Feels a bit like overkill.
> > > > > > >   (3) SeaBIOS can fallback to the PIT for timing on machines which
> > > > > > >       have no TSC.  We could do that too in case we detect kvm ...
> > > > >
> > > > > What sort of timeouts are these?  If seconds, maybe the rtc would be best.
> > >
> > > I vote for 3 so nobody has to maintain kvmclock code in SeaBIOS and Gerd
> > > can fix the in-kernel PIT issues with GRUB (see Michaels message) while testing.
>
> (2) turned out to be not too bad when taking a shortcut: Go through an
> enable/disable cycle each time we read the clock, then just grab
> system_time.  Not that efficient, but should be ok for seabios.  Usually
> it checks the clock when sitting around idle, waiting for something to
> happen.  And it simplifies the implementation alot as we can just skip
> all the tsc frequency & delta calculations.
>
> Draft patch attached.  Comments?
>
> +static void kvmclock_fetch(struct pvclock_vcpu_time_info *time)
> +{
> u32 addr = (u32)MAKE_FLATPTR(GET_SEG(SS), time);
> u32 msr = GET_GLOBAL(kvm_systime_msr);
> memset(time, 0, sizeof(*time));
> wrmsr(msr, addr | 1);

I'd put the time calculations in here.  We don't specify what happens to
the data area after disabling kvmclock; it could be in the middle of an
update.
> wrmsr(msr, 0);
>
> +}
>
> +u64 kvmclock_get(void)
> +{
> struct pvclock_vcpu_time_info time;
> kvmclock_fetch(&time);
> return time.system_time;

That's just a random number.  You have to do the full calculation.

[Avi Kivity]

On 08/09/2012 09:59 PM, Marcelo Tosatti wrote:
> > > wrmsr(msr, 0);
> > > if (time.version < 2 || time.tsc_to_system_mul == 0)
> > >    return 0;
> > > /* go figure tsc frequency */
> > > khz = pvclock_tsc_khz(&time);
> > > dprintf(1, "Using kvmclock, msr 0x%x, tsc %d MHz\n",
> > >        msr, (u32)khz / 1000);
> > > return khz;
> >
> > That's a meaningless number.  You can be migrated to a cpu or a machine
> > with very different tsc.
>
> Thats why there exists hardware tsc frequency scaling and the software
> equivalent for that on kvm.

The hardware is only available on a minority of processors in the field.
 The software equivalent isn't there.

[Avi Kivity]

On 08/09/2012 10:02 PM, Marcelo Tosatti wrote:
> On Thu, Aug 09, 2012 at 05:20:11PM +0300, Avi Kivity wrote:
> > On 08/09/2012 05:18 PM, Gerd Hoffmann wrote:
> > > Hi,
> > > > > So what do you suggest?  The options I see are:
> > > > >
> > > > > (1) Use this patch (with alignment issue fixed of course).
> > > > >   (2) Do a full kvmclock implementation.  Feels a bit like overkill.
> > > > >   (3) SeaBIOS can fallback to the PIT for timing on machines which
> > > > >       have no TSC.  We could do that too in case we detect kvm ...
> > > >
> > > > What sort of timeouts are these?  If seconds, maybe the rtc would be best.
> > >
> > > All sorts of timeouts, from a few miliseconds to seconds.
> > >
> > > The problematic ones are the longer timeouts, which wait for I/O stuff
> > > like disk reads complete.  The stuff with smaller timeouts (like waiting
> > > for AHCI link become ready) tend to finish instantly in kvm.
> >
> > That's not guaranteed.  The AHCI adapter might be real hardware.  Or the
> > emulation may change.
> >
> > What's wrong with having a full kvmclock implementation?  Instead of
> > issuing rdtsc call a function pointer.
>
> Its not necessary (someone is going to maintain the kvmclock frequency
> retrieve, which patch is already here, versus maintainance of 
> full kvmclock).

The frequency is meaninless.
> Frequency scaling (or the software equivalent: TSC trapping) are
> required for other reasons anyway.

One thing we can do is enable TSC trapping, then disable it if the guest
activates kvmclock.  That gives us accurate time either way.

[Gerd Hoffmann]

Hi,
> Isnt pmtimer ioport usable? 14MHz.

Can give it a try.  14 MHz looks wrong though, apci.h says:

/* PM Timer ticks per second (HZ) */
#define PM_TIMER_FREQUENCY  3579545

Is this fixed?  Or hardware specific?

cheers,
  Gerd

[Gleb Natapov]

On Mon, Aug 13, 2012 at 12:37:11PM +0200, Gerd Hoffmann wrote:
> Hi,
> > Isnt pmtimer ioport usable? 14MHz.
>
> Can give it a try.  14 MHz looks wrong though, apci.h says:
>
> /* PM Timer ticks per second (HZ) */
> #define PM_TIMER_FREQUENCY  3579545
>
> Is this fixed?  Or hardware specific?

3.579545 MHz clock required by ACPI spec.

[Fred .]

Add a comment about it in the source code.

-#define PM_TIMER_FREQUENCY  3579545
+#define PM_TIMER_FREQUENCY  3579545 // 3.579545 MHz clock required by
ACPI spec.

On Mon, Aug 13, 2012 at 12:46 PM, Gleb Natapov  wrote:
> On Mon, Aug 13, 2012 at 12:37:11PM +0200, Gerd Hoffmann wrote:
> > Hi,
> > > Isnt pmtimer ioport usable? 14MHz.
> >
> > Can give it a try.  14 MHz looks wrong though, apci.h says:
> >
> > /* PM Timer ticks per second (HZ) */
> > #define PM_TIMER_FREQUENCY  3579545
> >
> > Is this fixed?  Or hardware specific?
>
> 3.579545 MHz clock required by ACPI spec.
>
> SeaBIOS mailing list
> [e-mail]
> http://www.seabios.org/mailman/listinfo/seabios