[PATCH 0/2] Fix build on i386 due to the latest tsc changes

[PATCH 0/2] Fix build on i386 due to the latest tsc changes

[Avi Kivity]

The code fixed by the second patch looks suspect though:

  nsdiff = data - kvm->arch.last_tsc_write;
  nsdiff = (nsdiff * 1000) / vcpu->arch.virtual_tsc_khz;

before the division, nsdiff is in tsc units.  Dividing it by
tsc_khz/1000 is equivalent to multiplying it by 1000000 and dividing it by
tsc_hz, so the result is in units of mega-seconds.  I expect we want

  nsdiff /= (u64)tsc_khz * 1000;

(which is somewhat hard on i386)

Avi Kivity (2):
  KVM: Fix 64-bit division in kvm_set_tsc_khz()
  KVM: Fix 64-bit division in kvm_write_tsc()

 arch/x86/kvm/x86.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

-- 
1.7.9

[PATCH 1/2] KVM: Fix 64-bit division in kvm_set_tsc_khz()

[Avi Kivity]

Breaks i386 build.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/x86.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1312f13..1d71b13 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -978,6 +978,13 @@ static inline u64 nsec_to_cycles(struct kvm_vcpu *vcpu, u64 nsec)
 				   vcpu->arch.virtual_tsc_shift);
 }
 
+static u32 adjust_tsc_khz(u32 khz, s32 ppm)
+{
+	u64 v = (u64)khz * (1000000 + ppm);
+	do_div(v, 1000000);
+	return v;
+}
+
 static void kvm_set_tsc_khz(struct kvm_vcpu *vcpu, u32 this_tsc_khz)
 {
 	u32 thresh_lo, thresh_hi;
@@ -995,8 +1002,8 @@ static void kvm_set_tsc_khz(struct kvm_vcpu *vcpu, u32 this_tsc_khz)
 	 * rate being applied is within that bounds of the hardware
 	 * rate.  If so, no scaling or compensation need be done.
 	 */
-	thresh_lo = (u64)tsc_khz * (1000000 - tsc_tolerance_ppm) / 1000000;
-	thresh_hi = (u64)tsc_khz * (1000000 + tsc_tolerance_ppm) / 1000000;
+	thresh_lo = adjust_tsc_khz(tsc_khz, -tsc_tolerance_ppm);
+	thresh_hi = adjust_tsc_khz(tsc_khz, tsc_tolerance_ppm);
 	if (this_tsc_khz < thresh_lo || this_tsc_khz > thresh_hi) {
 		pr_debug("kvm: requested TSC rate %u falls outside tolerance [%u,%u]\n", this_tsc_khz, thresh_lo, thresh_hi);
 		use_scaling = 1;
-- 
1.7.9

[PATCH 2/2] KVM: Fix 64-bit division in kvm_write_tsc()

[Avi Kivity]

Breaks i386 build.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/x86.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1d71b13..c9d99e5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1034,7 +1034,14 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, u64 data)
 
 	/* n.b - signed multiplication and division required */
 	nsdiff = data - kvm->arch.last_tsc_write;
+#ifdef CONFIG_X86_64
 	nsdiff = (nsdiff * 1000) / vcpu->arch.virtual_tsc_khz;
+#else
+	/* do_div() only does unsigned */
+	asm("idiv %2; xor %%edx, %%edx"
+	    : "=A"(nsdiff)
+	    : "A"(nsdiff * 1000), "rm"(vcpu->arch.virtual_tsc_khz));
+#endif
 	nsdiff -= elapsed;
 	if (nsdiff < 0)
 		nsdiff = -nsdiff;
-- 
1.7.9

Re: [PATCH 0/2] Fix build on i386 due to the latest tsc changes

[Avi Kivity]

On 02/16/2012 03:48 PM, Avi Kivity wrote:
> The code fixed by the second patch looks suspect though:
>
>   nsdiff = data - kvm->arch.last_tsc_write;
>   nsdiff = (nsdiff * 1000) / vcpu->arch.virtual_tsc_khz;
>
> before the division, nsdiff is in tsc units.  Dividing it by
> tsc_khz/1000 is equivalent to multiplying it by 1000000 and dividing it by
> tsc_hz, so the result is in units of mega-seconds.  I expect we want
>

Actually it results in units of microseconds, while we want nanoseconds.

So maybe the correct code is

  nsdiff = (nsdiff * 1000000) / vcpu->arch.virtual_tsc_khz;

returning nanoseconds.

I note that if the guest writes a large value into the tsc, this breaks.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/2] Fix build on i386 due to the latest tsc changes

[Marcelo Tosatti]

On Thu, Feb 16, 2012 at 03:53:40PM +0200, Avi Kivity wrote:
> On 02/16/2012 03:48 PM, Avi Kivity wrote:
> > The code fixed by the second patch looks suspect though:
> >
> >   nsdiff = data - kvm->arch.last_tsc_write;
> >   nsdiff = (nsdiff * 1000) / vcpu->arch.virtual_tsc_khz;
> >
> > before the division, nsdiff is in tsc units.  Dividing it by
> > tsc_khz/1000 is equivalent to multiplying it by 1000000 and dividing it by
> > tsc_hz, so the result is in units of mega-seconds.  I expect we want
> >
> 
> Actually it results in units of microseconds, while we want nanoseconds.
> 
> So maybe the correct code is
> 
>   nsdiff = (nsdiff * 1000000) / vcpu->arch.virtual_tsc_khz;
> 
> returning nanoseconds.
> 
> I note that if the guest writes a large value into the tsc, this breaks.

You are right, the division is wrong. I'll fix it and run some tests
later. Nice catch.