[PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Paolo Bonzini]

Commit cbcf2dd3b3d4 (x86: kvm: Make kvm_get_time_and_clockread() nanoseconds
based, 2014-07-16) forgot to add tk->xtime_sec, thus breaking kvmclock on
hosts that have a reliable TSC.  Add it back; and since the field boot_ns
is not anymore related to the host boot-based clock, rename boot_ns->nsec_base
and the existing nsec_base->snsec_base.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <john.stultz@linaro.org>
Reported-by: Chris J Arges <chris.j.arges@canonical.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8f1e22d3b286..92493e10937c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1020,8 +1020,8 @@ struct pvclock_gtod_data {
 		u32	shift;
 	} clock;
 
-	u64		boot_ns;
 	u64		nsec_base;
+	u64		snsec_base;
 };
 
 static struct pvclock_gtod_data pvclock_gtod_data;
@@ -1042,8 +1042,9 @@ static void update_pvclock_gtod(struct timekeeper *tk)
 	vdata->clock.mult		= tk->tkr.mult;
 	vdata->clock.shift		= tk->tkr.shift;
 
-	vdata->boot_ns			= boot_ns;
-	vdata->nsec_base		= tk->tkr.xtime_nsec;
+	vdata->nsec_base		= tk->xtime_sec * (u64)NSEC_PER_SEC
+					+ boot_ns;
+	vdata->snsec_base		= tk->tkr.xtime_nsec;
 
 	write_seqcount_end(&vdata->seq);
 }
@@ -1413,10 +1414,10 @@ static int do_monotonic_boot(s64 *t, cycle_t *cycle_now)
 	do {
 		seq = read_seqcount_begin(&gtod->seq);
 		mode = gtod->clock.vclock_mode;
-		ns = gtod->nsec_base;
+		ns = gtod->snsec_base;
 		ns += vgettsc(cycle_now);
 		ns >>= gtod->clock.shift;
-		ns += gtod->boot_ns;
+		ns += gtod->nsec_base;
 	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
 	*t = ns;
 
-- 
2.1.0

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Chris J Arges]

On 09/04/2014 07:58 AM, Paolo Bonzini wrote:
> Commit cbcf2dd3b3d4 (x86: kvm: Make kvm_get_time_and_clockread() nanoseconds
> based, 2014-07-16) forgot to add tk->xtime_sec, thus breaking kvmclock on
> hosts that have a reliable TSC.  Add it back; and since the field boot_ns
> is not anymore related to the host boot-based clock, rename boot_ns->nsec_base
> and the existing nsec_base->snsec_base.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Stultz <john.stultz@linaro.org>
> Reported-by: Chris J Arges <chris.j.arges@canonical.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8f1e22d3b286..92493e10937c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1020,8 +1020,8 @@ struct pvclock_gtod_data {
>  		u32	shift;
>  	} clock;
>  
> -	u64		boot_ns;
>  	u64		nsec_base;
> +	u64		snsec_base;
>  };
>  
>  static struct pvclock_gtod_data pvclock_gtod_data;
> @@ -1042,8 +1042,9 @@ static void update_pvclock_gtod(struct timekeeper *tk)
>  	vdata->clock.mult		= tk->tkr.mult;
>  	vdata->clock.shift		= tk->tkr.shift;
>  
> -	vdata->boot_ns			= boot_ns;
> -	vdata->nsec_base		= tk->tkr.xtime_nsec;
> +	vdata->nsec_base		= tk->xtime_sec * (u64)NSEC_PER_SEC
> +					+ boot_ns;
> +	vdata->snsec_base		= tk->tkr.xtime_nsec;
>  
>  	write_seqcount_end(&vdata->seq);
>  }
> @@ -1413,10 +1414,10 @@ static int do_monotonic_boot(s64 *t, cycle_t *cycle_now)
>  	do {
>  		seq = read_seqcount_begin(&gtod->seq);
>  		mode = gtod->clock.vclock_mode;
> -		ns = gtod->nsec_base;
> +		ns = gtod->snsec_base;
>  		ns += vgettsc(cycle_now);
>  		ns >>= gtod->clock.shift;
> -		ns += gtod->boot_ns;
> +		ns += gtod->nsec_base;
>  	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
>  	*t = ns;
>  
> 

Paulo,
I've tested with the above patch and I still have issues with the
kvmclock test offset; however the cycle tests pass now.

Here is trace data:
http://people.canonical.com/~arges/kvm/trace-4.dat.xz

Uptime:
 15:58:02 up  1:00,  1 user,  load average: 0.59, 0.60, 0.31

Here is the output:

./x86-run x86/kvmclock_test.flat -smp 2 --append "10000000 `date +%s`"
qemu-system-x86_64 -enable-kvm -device pc-testdev -device
isa-debug-exit,iobase=0xf4,iosize=0x4 -display none -serial stdio
-device pci-testdev -kernel x86/kvmclock_test.flat -smp 2 --append
10000000 1409846210
enabling apic
enabling apic
kvm-clock: cpu 0, msr 0x:44d4c0
kvm-clock: cpu 0, msr 0x:44d4c0
Wallclock test, threshold 5
Seconds get from host:     1409846210
Seconds get from kvmclock: 2819688866
Offset:                    1409842656
offset too large!
Check the stability of raw cycle ...
Total vcpus: 2
Test  loops: 10000000
Total warps:  0
Total stalls: 0
Worst warp:   0
Raw cycle is stable
Monotonic cycle test:
Total vcpus: 2
Test  loops: 10000000
Total warps:  0
Total stalls: 0
Worst warp:   0
Measure the performance of raw cycle ...
Total vcpus: 2
Test  loops: 10000000
TSC cycles:  1139288710
Measure the performance of adjusted cycle ...
Total vcpus: 2
Test  loops: 10000000
TSC cycles:  1138643774
Return value from qemu: 3

My observation is that the kvmclock value seems to be positively biased
by the boot_ns value.

--chris j arges

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Paolo Bonzini]

Il 04/09/2014 18:00, Chris J Arges ha scritto:
> Uptime:
>  15:58:02 up  1:00,  1 user,  load average: 0.59, 0.60, 0.31
> 
> Here is the output:
> 
> ./x86-run x86/kvmclock_test.flat -smp 2 --append "10000000 `date +%s`"
> qemu-system-x86_64 -enable-kvm -device pc-testdev -device
> isa-debug-exit,iobase=0xf4,iosize=0x4 -display none -serial stdio
> -device pci-testdev -kernel x86/kvmclock_test.flat -smp 2 --append
> 10000000 1409846210
> enabling apic
> enabling apic
> kvm-clock: cpu 0, msr 0x:44d4c0
> kvm-clock: cpu 0, msr 0x:44d4c0
> Wallclock test, threshold 5
> Seconds get from host:     1409846210
> Seconds get from kvmclock: 2819688866
> Offset:                    1409842656

With kvm/queue this would have been roughly -3600, now it's 
host_wallclock-3600.  So the patch hasn't fixed the -3600 part for you.

Can you try applying this patch on top of 3.16?  This is my backport of
Thomas's patch.  If this works for you, we "only" have to find out how
to compute boot_ns and nsec_base in the new timekeeping world order of
3.17...

Thomas, do you have any ideas?  Every time a VM is started, the kvmclock
starts at the boot time of the host, instead of the current wallclock time.

Paolo

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d38abc81db65..70de23f1de51 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1020,9 +1020,8 @@ struct pvclock_gtod_data {
 		u32	shift;
 	} clock;
 
-	/* open coded 'struct timespec' */
-	u64		monotonic_time_snsec;
-	time_t		monotonic_time_sec;
+	u64		boot_ns;
+	u64		nsec_base;
 };
 
 static struct pvclock_gtod_data pvclock_gtod_data;
@@ -1030,6 +1029,12 @@ static struct pvclock_gtod_data pvclock_gtod_data;
 static void update_pvclock_gtod(struct timekeeper *tk)
 {
 	struct pvclock_gtod_data *vdata = &pvclock_gtod_data;
+	u64 boot_ns;
+
+	boot_ns = timespec_to_ns(&tk->total_sleep_time)
+		+ tk->wall_to_monotonic.tv_sec * (u64)NSEC_PER_SEC
+		+ tk->wall_to_monotonic.tv_nsec
+		+ tk->xtime_sec * (u64)NSEC_PER_SEC;
 
 	write_seqcount_begin(&vdata->seq);
 
@@ -1040,17 +1044,8 @@ static void update_pvclock_gtod(struct timekeeper *tk)
 	vdata->clock.mult		= tk->mult;
 	vdata->clock.shift		= tk->shift;
 
-	vdata->monotonic_time_sec	= tk->xtime_sec
-					+ tk->wall_to_monotonic.tv_sec;
-	vdata->monotonic_time_snsec	= tk->xtime_nsec
-					+ (tk->wall_to_monotonic.tv_nsec
-						<< tk->shift);
-	while (vdata->monotonic_time_snsec >=
-					(((u64)NSEC_PER_SEC) << tk->shift)) {
-		vdata->monotonic_time_snsec -=
-					((u64)NSEC_PER_SEC) << tk->shift;
-		vdata->monotonic_time_sec++;
-	}
+	vdata->boot_ns                  = boot_ns;
+	vdata->nsec_base		= tk->xtime_nsec;
 
 	write_seqcount_end(&vdata->seq);
 }
@@ -1414,23 +1409,22 @@ static inline u64 vgettsc(cycle_t *cycle_now)
 	return v * gtod->clock.mult;
 }
 
-static int do_monotonic(struct timespec *ts, cycle_t *cycle_now)
+static int do_monotonic_boot(s64 *t, cycle_t *cycle_now)
 {
+	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
 	unsigned long seq;
-	u64 ns;
 	int mode;
-	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
+	u64 ns;
 
-	ts->tv_nsec = 0;
 	do {
 		seq = read_seqcount_begin(&gtod->seq);
 		mode = gtod->clock.vclock_mode;
-		ts->tv_sec = gtod->monotonic_time_sec;
-		ns = gtod->monotonic_time_snsec;
+		ns = gtod->nsec_base;
 		ns += vgettsc(cycle_now);
 		ns >>= gtod->clock.shift;
+		ns += gtod->boot_ns;
 	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
-	timespec_add_ns(ts, ns);
+	*t = ns;
 
 	return mode;
 }
@@ -1438,19 +1432,11 @@ static int do_monotonic(struct timespec *ts, cycle_t *cycle_now)
 /* returns true if host is using tsc clocksource */
 static bool kvm_get_time_and_clockread(s64 *kernel_ns, cycle_t *cycle_now)
 {
-	struct timespec ts;
-
 	/* checked again under seqlock below */
 	if (pvclock_gtod_data.clock.vclock_mode != VCLOCK_TSC)
 		return false;
 
-	if (do_monotonic(&ts, cycle_now) != VCLOCK_TSC)
-		return false;
-
-	monotonic_to_bootbased(&ts);
-	*kernel_ns = timespec_to_ns(&ts);
-
-	return true;
+	return do_monotonic_boot(kernel_ns, cycle_now) == VCLOCK_TSC;
 }
 #endif
 


> My observation is that the kvmclock value seems to be positively biased
> by the boot_ns value.

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Chris J Arges]

On 09/04/2014 12:14 PM, Paolo Bonzini wrote:
> Il 04/09/2014 18:00, Chris J Arges ha scritto:
>> Uptime:
>>  15:58:02 up  1:00,  1 user,  load average: 0.59, 0.60, 0.31
>>
>> Here is the output:
>>
>> ./x86-run x86/kvmclock_test.flat -smp 2 --append "10000000 `date +%s`"
>> qemu-system-x86_64 -enable-kvm -device pc-testdev -device
>> isa-debug-exit,iobase=0xf4,iosize=0x4 -display none -serial stdio
>> -device pci-testdev -kernel x86/kvmclock_test.flat -smp 2 --append
>> 10000000 1409846210
>> enabling apic
>> enabling apic
>> kvm-clock: cpu 0, msr 0x:44d4c0
>> kvm-clock: cpu 0, msr 0x:44d4c0
>> Wallclock test, threshold 5
>> Seconds get from host:     1409846210
>> Seconds get from kvmclock: 2819688866
>> Offset:                    1409842656
> 
> With kvm/queue this would have been roughly -3600, now it's 
> host_wallclock-3600.  So the patch hasn't fixed the -3600 part for you.
> 
> Can you try applying this patch on top of 3.16?  This is my backport of
> Thomas's patch.  If this works for you, we "only" have to find out how
> to compute boot_ns and nsec_base in the new timekeeping world order of
> 3.17...

Paolo,
The patch below applied to 3.16 still allows the testcase to pass on my
hardware.
--chris

> 
> Thomas, do you have any ideas?  Every time a VM is started, the kvmclock
> starts at the boot time of the host, instead of the current wallclock time.
> 
> Paolo
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d38abc81db65..70de23f1de51 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1020,9 +1020,8 @@ struct pvclock_gtod_data {
>  		u32	shift;
>  	} clock;
>  
> -	/* open coded 'struct timespec' */
> -	u64		monotonic_time_snsec;
> -	time_t		monotonic_time_sec;
> +	u64		boot_ns;
> +	u64		nsec_base;
>  };
>  
>  static struct pvclock_gtod_data pvclock_gtod_data;
> @@ -1030,6 +1029,12 @@ static struct pvclock_gtod_data pvclock_gtod_data;
>  static void update_pvclock_gtod(struct timekeeper *tk)
>  {
>  	struct pvclock_gtod_data *vdata = &pvclock_gtod_data;
> +	u64 boot_ns;
> +
> +	boot_ns = timespec_to_ns(&tk->total_sleep_time)
> +		+ tk->wall_to_monotonic.tv_sec * (u64)NSEC_PER_SEC
> +		+ tk->wall_to_monotonic.tv_nsec
> +		+ tk->xtime_sec * (u64)NSEC_PER_SEC;
>  
>  	write_seqcount_begin(&vdata->seq);
>  
> @@ -1040,17 +1044,8 @@ static void update_pvclock_gtod(struct timekeeper *tk)
>  	vdata->clock.mult		= tk->mult;
>  	vdata->clock.shift		= tk->shift;
>  
> -	vdata->monotonic_time_sec	= tk->xtime_sec
> -					+ tk->wall_to_monotonic.tv_sec;
> -	vdata->monotonic_time_snsec	= tk->xtime_nsec
> -					+ (tk->wall_to_monotonic.tv_nsec
> -						<< tk->shift);
> -	while (vdata->monotonic_time_snsec >=
> -					(((u64)NSEC_PER_SEC) << tk->shift)) {
> -		vdata->monotonic_time_snsec -=
> -					((u64)NSEC_PER_SEC) << tk->shift;
> -		vdata->monotonic_time_sec++;
> -	}
> +	vdata->boot_ns                  = boot_ns;
> +	vdata->nsec_base		= tk->xtime_nsec;
>  
>  	write_seqcount_end(&vdata->seq);
>  }
> @@ -1414,23 +1409,22 @@ static inline u64 vgettsc(cycle_t *cycle_now)
>  	return v * gtod->clock.mult;
>  }
>  
> -static int do_monotonic(struct timespec *ts, cycle_t *cycle_now)
> +static int do_monotonic_boot(s64 *t, cycle_t *cycle_now)
>  {
> +	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
>  	unsigned long seq;
> -	u64 ns;
>  	int mode;
> -	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
> +	u64 ns;
>  
> -	ts->tv_nsec = 0;
>  	do {
>  		seq = read_seqcount_begin(&gtod->seq);
>  		mode = gtod->clock.vclock_mode;
> -		ts->tv_sec = gtod->monotonic_time_sec;
> -		ns = gtod->monotonic_time_snsec;
> +		ns = gtod->nsec_base;
>  		ns += vgettsc(cycle_now);
>  		ns >>= gtod->clock.shift;
> +		ns += gtod->boot_ns;
>  	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
> -	timespec_add_ns(ts, ns);
> +	*t = ns;
>  
>  	return mode;
>  }
> @@ -1438,19 +1432,11 @@ static int do_monotonic(struct timespec *ts, cycle_t *cycle_now)
>  /* returns true if host is using tsc clocksource */
>  static bool kvm_get_time_and_clockread(s64 *kernel_ns, cycle_t *cycle_now)
>  {
> -	struct timespec ts;
> -
>  	/* checked again under seqlock below */
>  	if (pvclock_gtod_data.clock.vclock_mode != VCLOCK_TSC)
>  		return false;
>  
> -	if (do_monotonic(&ts, cycle_now) != VCLOCK_TSC)
> -		return false;
> -
> -	monotonic_to_bootbased(&ts);
> -	*kernel_ns = timespec_to_ns(&ts);
> -
> -	return true;
> +	return do_monotonic_boot(kernel_ns, cycle_now) == VCLOCK_TSC;
>  }
>  #endif
>  
> 
> 
>> My observation is that the kvmclock value seems to be positively biased
>> by the boot_ns value.
> 

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Paolo Bonzini]

Il 04/09/2014 20:16, Chris J Arges ha scritto:
>> +	boot_ns = timespec_to_ns(&tk->total_sleep_time)
>> +		+ tk->wall_to_monotonic.tv_sec * (u64)NSEC_PER_SEC
>> +		+ tk->wall_to_monotonic.tv_nsec
>> +		+ tk->xtime_sec * (u64)NSEC_PER_SEC;

So this means that the above 3.16-based code is not the same as

        boot_ns = ktime_to_ns(ktime_add(tk->tkr.base_mono, tk->offs_boot));

in 3.17.  Everything else in the patch you tested is the same as the
code that is in 3.17, so that's a start.

Paolo

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Paolo Bonzini]

Il 04/09/2014 21:15, Paolo Bonzini ha scritto:
> Il 04/09/2014 20:16, Chris J Arges ha scritto:
>>> +	boot_ns = timespec_to_ns(&tk->total_sleep_time)
>>> +		+ tk->wall_to_monotonic.tv_sec * (u64)NSEC_PER_SEC
>>> +		+ tk->wall_to_monotonic.tv_nsec
>>> +		+ tk->xtime_sec * (u64)NSEC_PER_SEC;
> 
> So this means that the above 3.16-based code is not the same as
> 
>         boot_ns = ktime_to_ns(ktime_add(tk->tkr.base_mono, tk->offs_boot));
> 
> in 3.17.  Everything else in the patch you tested is the same as the
> code that is in 3.17, so that's a start.
> 
> Paolo
> 

Based on commit 02cba1598a2a3b689e79ad6dad2532521f638271 we have:

   offs_real - offs_boot = wall_to_monotonic + total_sleep_time

The patch I posted this morning separates tk->xtime_sec out of boot_ns, so
all that is missing should be a change in boot_ns from base_mono + offs_boot
to offs_real - offs_boot.

Chris, can you try this patch on top of the previous one:

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 92493e10937c..811eecc43fe8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1031,7 +1031,7 @@ static void update_pvclock_gtod(struct timekeeper *tk)
 	struct pvclock_gtod_data *vdata = &pvclock_gtod_data;
 	u64 boot_ns;
 
-	boot_ns = ktime_to_ns(ktime_add(tk->tkr.base_mono, tk->offs_boot));
+	boot_ns = ktime_to_ns(ktime_sub(tk->offs_real, tk->offs_boot));
 
 	write_seqcount_begin(&vdata->seq);

If it doesn't work, then commit 02cba1598a2a3b689e79ad6dad2532521f638271
is also broken.

Paolo

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Chris J Arges]

On 09/04/2014 02:42 PM, Paolo Bonzini wrote:
> Il 04/09/2014 21:15, Paolo Bonzini ha scritto:
>> Il 04/09/2014 20:16, Chris J Arges ha scritto:
>>>> +	boot_ns = timespec_to_ns(&tk->total_sleep_time)
>>>> +		+ tk->wall_to_monotonic.tv_sec * (u64)NSEC_PER_SEC
>>>> +		+ tk->wall_to_monotonic.tv_nsec
>>>> +		+ tk->xtime_sec * (u64)NSEC_PER_SEC;
>>
>> So this means that the above 3.16-based code is not the same as
>>
>>         boot_ns = ktime_to_ns(ktime_add(tk->tkr.base_mono, tk->offs_boot));
>>
>> in 3.17.  Everything else in the patch you tested is the same as the
>> code that is in 3.17, so that's a start.
>>
>> Paolo
>>
> 
> Based on commit 02cba1598a2a3b689e79ad6dad2532521f638271 we have:
> 
>    offs_real - offs_boot = wall_to_monotonic + total_sleep_time
> 
> The patch I posted this morning separates tk->xtime_sec out of boot_ns, so
> all that is missing should be a change in boot_ns from base_mono + offs_boot
> to offs_real - offs_boot.
> 
> Chris, can you try this patch on top of the previous one:
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 92493e10937c..811eecc43fe8 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1031,7 +1031,7 @@ static void update_pvclock_gtod(struct timekeeper *tk)
>  	struct pvclock_gtod_data *vdata = &pvclock_gtod_data;
>  	u64 boot_ns;
>  
> -	boot_ns = ktime_to_ns(ktime_add(tk->tkr.base_mono, tk->offs_boot));
> +	boot_ns = ktime_to_ns(ktime_sub(tk->offs_real, tk->offs_boot));
>  
>  	write_seqcount_begin(&vdata->seq);
> 
> If it doesn't work, then commit 02cba1598a2a3b689e79ad6dad2532521f638271
> is also broken.
> 
> Paolo
> 

Paolo,
That modification do your additional patch didn't work. However I was
able to modify the code as follows to get this test case working. The
only additional modification was:
+	vdata->nsec_base		= tk->xtime_sec * (u64)NSEC_PER_SEC
+					- boot_ns;

--chris j arges

--

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7b25aa2..60c0a9b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1023,8 +1023,8 @@ struct pvclock_gtod_data {
 		u32	shift;
 	} clock;

-	u64		boot_ns;
 	u64		nsec_base;
+	u64		snsec_base;
 };

 static struct pvclock_gtod_data pvclock_gtod_data;
@@ -1034,7 +1034,7 @@ static void update_pvclock_gtod(struct timekeeper *tk)
 	struct pvclock_gtod_data *vdata = &pvclock_gtod_data;
 	u64 boot_ns;

-	boot_ns = ktime_to_ns(ktime_add(tk->tkr.base_mono, tk->offs_boot));
+	boot_ns = ktime_to_ns(ktime_sub(tk->offs_real, tk->offs_boot));

 	write_seqcount_begin(&vdata->seq);

@@ -1045,8 +1045,9 @@ static void update_pvclock_gtod(struct timekeeper *tk)
 	vdata->clock.mult		= tk->tkr.mult;
 	vdata->clock.shift		= tk->tkr.shift;

-	vdata->boot_ns			= boot_ns;
-	vdata->nsec_base		= tk->tkr.xtime_nsec;
+	vdata->nsec_base		= tk->xtime_sec * (u64)NSEC_PER_SEC
+					- boot_ns;
+	vdata->snsec_base		= tk->tkr.xtime_nsec;

 	write_seqcount_end(&vdata->seq);
 }
@@ -1416,10 +1417,10 @@ static int do_monotonic_boot(s64 *t, cycle_t
*cycle_now)
 	do {
 		seq = read_seqcount_begin(&gtod->seq);
 		mode = gtod->clock.vclock_mode;
-		ns = gtod->nsec_base;
+		ns = gtod->snsec_base;
 		ns += vgettsc(cycle_now);
 		ns >>= gtod->clock.shift;
-		ns += gtod->boot_ns;
+		ns += gtod->nsec_base;
 	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
 	*t = ns;

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Paolo Bonzini]

Il 04/09/2014 22:37, Chris J Arges ha scritto:
>> > -	boot_ns = ktime_to_ns(ktime_add(tk->tkr.base_mono, tk->offs_boot));
>> > +	boot_ns = ktime_to_ns(ktime_sub(tk->offs_real, tk->offs_boot));
>> >  
>> >  	write_seqcount_begin(&vdata->seq);
>> > 
>> > If it doesn't work, then commit 02cba1598a2a3b689e79ad6dad2532521f638271
>> > is also broken.
>> > 
>> > Paolo
>> > 
> Paolo,
> That modification do your additional patch didn't work. However I was
> able to modify the code as follows to get this test case working. The
> only additional modification was:
> +	vdata->nsec_base		= tk->xtime_sec * (u64)NSEC_PER_SEC
> +					- boot_ns;

Right, it should have been

	boot_ns = ktime_to_ns(ktime_sub(tk->offs_boot, tk->offs_real));

I'll post the patch shortly.

Thanks!

Paolo

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Chris J Arges]

On 09/04/2014 03:40 PM, Paolo Bonzini wrote:
> Il 04/09/2014 22:37, Chris J Arges ha scritto:
>>>> -	boot_ns = ktime_to_ns(ktime_add(tk->tkr.base_mono, tk->offs_boot));
>>>> +	boot_ns = ktime_to_ns(ktime_sub(tk->offs_real, tk->offs_boot));
>>>>  
>>>>  	write_seqcount_begin(&vdata->seq);
>>>>
>>>> If it doesn't work, then commit 02cba1598a2a3b689e79ad6dad2532521f638271
>>>> is also broken.
>>>>
>>>> Paolo
>>>>
>> Paolo,
>> That modification do your additional patch didn't work. However I was
>> able to modify the code as follows to get this test case working. The
>> only additional modification was:
>> +	vdata->nsec_base		= tk->xtime_sec * (u64)NSEC_PER_SEC
>> +					- boot_ns;
> 
> Right, it should have been
> 
> 	boot_ns = ktime_to_ns(ktime_sub(tk->offs_boot, tk->offs_real));
> 
> I'll post the patch shortly.
> 
> Thanks!
> 
> Paolo
> 

Paolo,

Great, tested that modification really quick and it also works for me!
All test cases are now passing on my machine; thanks for all the
debugging and help.

--chris

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[John Stultz]

On Thu, Sep 4, 2014 at 9:00 AM, Chris J Arges
<chris.j.arges@canonical.com> wrote:
>
>
> On 09/04/2014 07:58 AM, Paolo Bonzini wrote:
>> Commit cbcf2dd3b3d4 (x86: kvm: Make kvm_get_time_and_clockread() nanoseconds
>> based, 2014-07-16) forgot to add tk->xtime_sec, thus breaking kvmclock on
>> hosts that have a reliable TSC.  Add it back; and since the field boot_ns
>> is not anymore related to the host boot-based clock, rename boot_ns->nsec_base
>> and the existing nsec_base->snsec_base.
>>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: John Stultz <john.stultz@linaro.org>
>> Reported-by: Chris J Arges <chris.j.arges@canonical.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  arch/x86/kvm/x86.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 8f1e22d3b286..92493e10937c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -1020,8 +1020,8 @@ struct pvclock_gtod_data {
>>               u32     shift;
>>       } clock;
>>
>> -     u64             boot_ns;
>>       u64             nsec_base;
>> +     u64             snsec_base;
>>  };
>>
>>  static struct pvclock_gtod_data pvclock_gtod_data;
>> @@ -1042,8 +1042,9 @@ static void update_pvclock_gtod(struct timekeeper *tk)
>>       vdata->clock.mult               = tk->tkr.mult;
>>       vdata->clock.shift              = tk->tkr.shift;
>>
>> -     vdata->boot_ns                  = boot_ns;
>> -     vdata->nsec_base                = tk->tkr.xtime_nsec;
>> +     vdata->nsec_base                = tk->xtime_sec * (u64)NSEC_PER_SEC
>> +                                     + boot_ns;
>> +     vdata->snsec_base               = tk->tkr.xtime_nsec;
>>
>>       write_seqcount_end(&vdata->seq);
>>  }
>> @@ -1413,10 +1414,10 @@ static int do_monotonic_boot(s64 *t, cycle_t *cycle_now)
>>       do {
>>               seq = read_seqcount_begin(&gtod->seq);
>>               mode = gtod->clock.vclock_mode;
>> -             ns = gtod->nsec_base;
>> +             ns = gtod->snsec_base;
>>               ns += vgettsc(cycle_now);
>>               ns >>= gtod->clock.shift;
>> -             ns += gtod->boot_ns;
>> +             ns += gtod->nsec_base;
>>       } while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
>>       *t = ns;
>>
>>
>
> Paulo,
> I've tested with the above patch and I still have issues with the
> kvmclock test offset; however the cycle tests pass now.
>
> Here is trace data:
> http://people.canonical.com/~arges/kvm/trace-4.dat.xz
>
> Uptime:
>  15:58:02 up  1:00,  1 user,  load average: 0.59, 0.60, 0.31
>
> Here is the output:
>
> ./x86-run x86/kvmclock_test.flat -smp 2 --append "10000000 `date +%s`"
> qemu-system-x86_64 -enable-kvm -device pc-testdev -device
> isa-debug-exit,iobase=0xf4,iosize=0x4 -display none -serial stdio
> -device pci-testdev -kernel x86/kvmclock_test.flat -smp 2 --append
> 10000000 1409846210
> enabling apic
> enabling apic
> kvm-clock: cpu 0, msr 0x:44d4c0
> kvm-clock: cpu 0, msr 0x:44d4c0
> Wallclock test, threshold 5
> Seconds get from host:     1409846210
> Seconds get from kvmclock: 2819688866
> Offset:                    1409842656
> offset too large!


Hey, thanks for reporting the issue and sending an initial patch (even
if its not quite all sorted yet).

Is the test you're using here available somewhere? Are there any
special requirements to run it?

thanks
-john

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Paolo Bonzini]

Il 04/09/2014 21:00, John Stultz ha scritto:
> 
> Hey, thanks for reporting the issue and sending an initial patch (even
> if its not quite all sorted yet).
> 
> Is the test you're using here available somewhere? Are there any
> special requirements to run it?

You need KVM on a machine with clocksource=tsc.  Grab the tests from
git://git.kernel.org/pub/scm/virt/kvm/kvm-unit-tests.git

and run them with

    ./configure
    make
    ./x86-run x86/kvmclock_test.flat  --append "10000000 `date +%s`"

Thanks,

Paolo

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Paolo Bonzini]

Il 04/09/2014 14:58, Paolo Bonzini ha scritto:
> Commit cbcf2dd3b3d4 (x86: kvm: Make kvm_get_time_and_clockread() nanoseconds
> based, 2014-07-16) forgot to add tk->xtime_sec, thus breaking kvmclock on
> hosts that have a reliable TSC.  Add it back; and since the field boot_ns
> is not anymore related to the host boot-based clock, rename boot_ns->nsec_base
> and the existing nsec_base->snsec_base.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Stultz <john.stultz@linaro.org>
> Reported-by: Chris J Arges <chris.j.arges@canonical.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8f1e22d3b286..92493e10937c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1020,8 +1020,8 @@ struct pvclock_gtod_data {
>  		u32	shift;
>  	} clock;
>  
> -	u64		boot_ns;
>  	u64		nsec_base;
> +	u64		snsec_base;
>  };
>  
>  static struct pvclock_gtod_data pvclock_gtod_data;
> @@ -1042,8 +1042,9 @@ static void update_pvclock_gtod(struct timekeeper *tk)
>  	vdata->clock.mult		= tk->tkr.mult;
>  	vdata->clock.shift		= tk->tkr.shift;
>  
> -	vdata->boot_ns			= boot_ns;
> -	vdata->nsec_base		= tk->tkr.xtime_nsec;
> +	vdata->nsec_base		= tk->xtime_sec * (u64)NSEC_PER_SEC
> +					+ boot_ns;
> +	vdata->snsec_base		= tk->tkr.xtime_nsec;

Hmm, I found this comment in kernel/time/timekeeping.c

        /*
         * The xtime based monotonic readout is:
         *      nsec = (xtime_sec + wtm_sec) * 1e9 + wtm_nsec + now();
         * The ktime based monotonic readout is:
         *      nsec = base_mono + now();

so this patch makes no sense.  The offs_boot part must be broken.

Paolo

>  
>  	write_seqcount_end(&vdata->seq);
>  }
> @@ -1413,10 +1414,10 @@ static int do_monotonic_boot(s64 *t, cycle_t *cycle_now)
>  	do {
>  		seq = read_seqcount_begin(&gtod->seq);
>  		mode = gtod->clock.vclock_mode;
> -		ns = gtod->nsec_base;
> +		ns = gtod->snsec_base;
>  		ns += vgettsc(cycle_now);
>  		ns >>= gtod->clock.shift;
> -		ns += gtod->boot_ns;
> +		ns += gtod->nsec_base;
>  	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
>  	*t = ns;
>  
> 

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Thomas Gleixner]

On Thu, 4 Sep 2014, Paolo Bonzini wrote:

> Commit cbcf2dd3b3d4 (x86: kvm: Make kvm_get_time_and_clockread() nanoseconds
> based, 2014-07-16) forgot to add tk->xtime_sec, thus breaking kvmclock on

Errm. How is boottime related to xtime_sec?

> hosts that have a reliable TSC.  Add it back; and since the field boot_ns
> is not anymore related to the host boot-based clock, rename boot_ns->nsec_base
> and the existing nsec_base->snsec_base.

This is simply wrong.

The original code before that changed did:

       vdata->monotonic_time_sec       = tk->xtime_sec
                                       + tk->wall_to_monotonic.tv_sec;
       vdata->monotonic_time_snsec     = tk->xtime_nsec
                                       + (tk->wall_to_monotonic.tv_nsec
                                               << tk->shift);
So this is the momentary monotonic base time

And the readout function did:

       ts->tv_nsec = 0;
       do {
               seq = read_seqcount_begin(&gtod->seq);
               mode = gtod->clock.vclock_mode;
               ts->tv_sec = gtod->monotonic_time_sec;
               ns = gtod->monotonic_time_snsec;
               ns += vgettsc(cycle_now);
               ns >>= gtod->clock.shift;
        } while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
       timespec_add_ns(ts, ns);

So this does:

   now = monotonic_base + delta_nsec

And the caller converted it to boot time with:

       monotonic_to_bootbased(&ts);

So the time calculation does:

  now = monotonic_base + delta_nsec + mono_to_boot

Because:     monotonic_base + mono_to_boot = boot_time_base
 
The calculation can be written as:

  now = boot_time_base + delta_nsec


The new code does

    boot_ns = ktime_to_ns(ktime_add(tk->base_mono, tk->offs_boot));

So thats

   boot_time_base = monotonic_base + mono_to_boot;

    vdata->boot_ns                  = boot_ns;
    vdata->nsec_base                = tk->tkr.xtime_nsec;

And the readout does:

    do {
           seq = read_seqcount_begin(&gtod->seq);
           mode = gtod->clock.vclock_mode;
     	   ns = gtod->nsec_base;
           ns += vgettsc(cycle_now);
           ns >>= gtod->clock.shift;
           ns += gtod->boot_ns;
   } while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
   *t = ns;

Which is:

   boot_time_base + delta_nsec

Now I have no idea why you think it needs to add xtime_sec. If the
result is wrong, then we need to figure out which one of the supplied
values is wrong and not blindly add xtime_sec just because that makes
it magically correct.

Can you please provide a proper background why you think that adding
xtime_sec is a good idea?

Thanks,

	tglx

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Paolo Bonzini]

Il 04/09/2014 22:58, Thomas Gleixner ha scritto:
> This is simply wrong.

It is.

> Now I have no idea why you think it needs to add xtime_sec. If the
> result is wrong, then we need to figure out which one of the supplied
> values is wrong and not blindly add xtime_sec just because that makes
> it magically correct.
> 
> Can you please provide a proper background why you think that adding
> xtime_sec is a good idea?

It's not a good idea indeed.  I didn't fully digest the 3.16->3.17
timekeeping changes and messed up this patch.

However, there is a bug in the "base_mono + offs_boot" formula, given
that:

- bisection leads to the merge commit of John's timers branch

- bisecting within John's timers branch, with a KVM commit on top to
  make the code much easier to trigger, leads to commit cbcf2dd3b3d4
  (x86: kvm: Make kvm_get_time_and_clockread() nanoseconds based,
  2014-07-16).

- I backported your patch to 3.16, using wall_to_monotonic +
  total_sleep_time + xtime_sec (wtm+xtime_sec as in pre-cbcf2dd3b3d4
  code, total_sleep_time from 3.16 monotonic_to_bootbased) and it works

- In v2 of the patch I fixed the bug by changing the formula
  "base_mono + offs_boot" to "offs_boot - offs_real" (and then adding
  xtime_sec separately as in the 3.16 backport), but the two formulas
  "base_mono + offs_boot" and "offs_boot - offs_real + xtime_sec" ought
  to be identical.

I find "offs_boot - offs_real + xtime" more readable than the
alternative "base_mono + offs_boot + xtime_nsec", so the fix doubles as
a cleanup for me and I'm fine with it.  But something must be wrong in
the timekeeping code.

Paolo

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Thomas Gleixner]

On Thu, 4 Sep 2014, Paolo Bonzini wrote:
> Il 04/09/2014 22:58, Thomas Gleixner ha scritto:
> > This is simply wrong.
> 
> It is.
> 
> > Now I have no idea why you think it needs to add xtime_sec. If the
> > result is wrong, then we need to figure out which one of the supplied
> > values is wrong and not blindly add xtime_sec just because that makes
> > it magically correct.
> > 
> > Can you please provide a proper background why you think that adding
> > xtime_sec is a good idea?
> 
> It's not a good idea indeed.  I didn't fully digest the 3.16->3.17
> timekeeping changes and messed up this patch.
> 
> However, there is a bug in the "base_mono + offs_boot" formula, given
> that:
> 
> - bisection leads to the merge commit of John's timers branch
> 
> - bisecting within John's timers branch, with a KVM commit on top to
>   make the code much easier to trigger, leads to commit cbcf2dd3b3d4
>   (x86: kvm: Make kvm_get_time_and_clockread() nanoseconds based,
>   2014-07-16).
> 
> - I backported your patch to 3.16, using wall_to_monotonic +
>   total_sleep_time + xtime_sec (wtm+xtime_sec as in pre-cbcf2dd3b3d4
>   code, total_sleep_time from 3.16 monotonic_to_bootbased) and it works
> 
> - In v2 of the patch I fixed the bug by changing the formula
>   "base_mono + offs_boot" to "offs_boot - offs_real" (and then adding
>   xtime_sec separately as in the 3.16 backport), but the two formulas
>   "base_mono + offs_boot" and "offs_boot - offs_real + xtime_sec" ought
>   to be identical.
> 
> I find "offs_boot - offs_real + xtime" more readable than the
> alternative "base_mono + offs_boot + xtime_nsec", so the fix doubles as
> a cleanup for me and I'm fine with it.  But something must be wrong in
> the timekeeping code.

I think I have a vague idea what happened, but I'm way too tired now
to write it up fully. I'll do that tomorrow morning with brain awake.

Thanks,

	tglx

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Thomas Gleixner]

On Thu, 4 Sep 2014, Paolo Bonzini wrote:

> Il 04/09/2014 22:58, Thomas Gleixner ha scritto:
> > This is simply wrong.
> 
> It is.
> 
> > Now I have no idea why you think it needs to add xtime_sec. If the
> > result is wrong, then we need to figure out which one of the supplied
> > values is wrong and not blindly add xtime_sec just because that makes
> > it magically correct.
> > 
> > Can you please provide a proper background why you think that adding
> > xtime_sec is a good idea?
> 
> It's not a good idea indeed.  I didn't fully digest the 3.16->3.17
> timekeeping changes and messed up this patch.
> 
> However, there is a bug in the "base_mono + offs_boot" formula, given
> that:
> 
> - bisection leads to the merge commit of John's timers branch
> 
> - bisecting within John's timers branch, with a KVM commit on top to
>   make the code much easier to trigger, leads to commit cbcf2dd3b3d4
>   (x86: kvm: Make kvm_get_time_and_clockread() nanoseconds based,
>   2014-07-16).
> 
> - I backported your patch to 3.16, using wall_to_monotonic +
>   total_sleep_time + xtime_sec (wtm+xtime_sec as in pre-cbcf2dd3b3d4
>   code, total_sleep_time from 3.16 monotonic_to_bootbased) and it works
> 
> - In v2 of the patch I fixed the bug by changing the formula
>   "base_mono + offs_boot" to "offs_boot - offs_real" (and then adding
>   xtime_sec separately as in the 3.16 backport), but the two formulas
>   "base_mono + offs_boot" and "offs_boot - offs_real + xtime_sec" ought
>   to be identical.

So lets look at the differences here:

3.16	     	       	    	    3.17

inject_sleeptime(delta)		    inject_sleeptime(delta)

    xtime += delta;			xtime += delta;

    wall_to_mono -= delta;		wall_to_mono -= delta;
    offs_real = -wall_to_mono;		offs_real = -wall_to_mono;

    sleeptime += delta;
    offs_boot = sleeptime;		offs_boot += delta;

getboottime()

    tmp = wall_to_mono + sleeptime;
    boottime = -tmp;

  So:
    boottime = -wall_to_mono - sleeptime;

  Because of the above:
    offs_real = -wall_to_mono;
    offs_boot = sleeptime;

  The result is:

  boottime = offs_real - offs_boot;	  boottime = offs_real - offs_boot;

monotomic_to_bootbased(mono)		monotomic_to_bootbased(mono)

   return mono + sleeptime;

  Because of the above:
    offs_boot = sleeptime;

  The result is:

    return mono + offs_boot;		  return mono + offs_boot;
  
Now on KVM side he have

update_pvclock_gtod()			update_pvclock_gtod()

    mono_base = xtime + wall_to_mono;	   boot_base = mono_base + offs_boot;

and

do_monotonic()				do_monotonic_boot()

    mono_now = mono_base + delta_ns;	   boot_now = boot_base + delta_ns;

kvm_get_time_and_clockread()

    mono_now = do_monotonic()

    boot_now = mono_to_boot(mono_now);

So that means on both side the same:

   boot_now = mono_base + offs_boot + delta_ns;

So that means the code is correct. Now where is the bug?

Well hidden and still so obvious that it's even visible through the
brown paperpag I'm wearing ...

Thanks,

	tglx

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index fb4a9c2cf8d9..ec1791fae965 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -442,11 +442,12 @@ static void timekeeping_update(struct timekeeper *tk, unsigned int action)
 		tk->ntp_error = 0;
 		ntp_clear();
 	}
-	update_vsyscall(tk);
-	update_pvclock_gtod(tk, action & TK_CLOCK_WAS_SET);
 
 	tk_update_ktime_data(tk);
 
+	update_vsyscall(tk);
+	update_pvclock_gtod(tk, action & TK_CLOCK_WAS_SET);
+
 	if (action & TK_MIRROR)
 		memcpy(&shadow_timekeeper, &tk_core.timekeeper,
 		       sizeof(tk_core.timekeeper));

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Paolo Bonzini]

Il 05/09/2014 17:14, Thomas Gleixner ha scritto:
> So that means the code is correct. Now where is the bug?

In kernel/time/timekeeping.c?

We know that we should have

  base_mono             =     wall_to_monotonic + xtime_sec

Instead it is

  base_mono             = wall_to_monotonic + xtime_sec
                          - seconds from boot time

which is... zero.  Given this is the only use of base_mono in a
notifier, I wonder if it is as simple as this (which I don't have time
to test right now):

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index fb4a9c2cf8d9..f6807a85b8c9 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -443,9 +443,9 @@ static void timekeeping_update(struct timekeeper
*tk, unsigned int action)
 		ntp_clear();
 	}
 	update_vsyscall(tk);
-	update_pvclock_gtod(tk, action & TK_CLOCK_WAS_SET);

 	tk_update_ktime_data(tk);
+	update_pvclock_gtod(tk, action & TK_CLOCK_WAS_SET);

 	if (action & TK_MIRROR)
 		memcpy(&shadow_timekeeper, &tk_core.timekeeper,

:)

Paolo

> Well hidden and still so obvious that it's even visible through the
> brown paperpag I'm wearing ...

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Thomas Gleixner]

On Fri, 5 Sep 2014, Paolo Bonzini wrote:

> Il 05/09/2014 17:14, Thomas Gleixner ha scritto:
> > So that means the code is correct. Now where is the bug?
> 
> In kernel/time/timekeeping.c?
> 
> We know that we should have
> 
>   base_mono             =     wall_to_monotonic + xtime_sec
> 
> Instead it is
> 
>   base_mono             = wall_to_monotonic + xtime_sec
>                           - seconds from boot time
> 
> which is... zero.  Given this is the only use of base_mono in a
> notifier, I wonder if it is as simple as this (which I don't have time
> to test right now):
> 
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index fb4a9c2cf8d9..f6807a85b8c9 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -443,9 +443,9 @@ static void timekeeping_update(struct timekeeper
> *tk, unsigned int action)
>  		ntp_clear();
>  	}
>  	update_vsyscall(tk);
> -	update_pvclock_gtod(tk, action & TK_CLOCK_WAS_SET);
> 
>  	tk_update_ktime_data(tk);
> +	update_pvclock_gtod(tk, action & TK_CLOCK_WAS_SET);

Why are you moving the update between vsycall and pvclock update as I
did in my patch? We really need to update everything before calling
somewhere.
 
And yes it is that simple. I instrumented the stuff and its correct
now.

Thanks

	tglx

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Paolo Bonzini]

Il 05/09/2014 20:33, Thomas Gleixner ha scritto:
>> >  	update_vsyscall(tk);
>> > -	update_pvclock_gtod(tk, action & TK_CLOCK_WAS_SET);
>> > 
>> >  	tk_update_ktime_data(tk);
>> > +	update_pvclock_gtod(tk, action & TK_CLOCK_WAS_SET);
> Why are you moving the update between vsycall and pvclock update as I
> did in my patch? We really need to update everything before calling
> somewhere.

Do you mean the call should be moved not just after tk_update_ktime_data
(which sets base_mono), but further down after

        update_fast_timekeeper(tk);

?

Paolo

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Thomas Gleixner]

On Fri, 5 Sep 2014, Paolo Bonzini wrote:

> Il 05/09/2014 20:33, Thomas Gleixner ha scritto:
> >> >  	update_vsyscall(tk);
> >> > -	update_pvclock_gtod(tk, action & TK_CLOCK_WAS_SET);
> >> > 
> >> >  	tk_update_ktime_data(tk);
> >> > +	update_pvclock_gtod(tk, action & TK_CLOCK_WAS_SET);
> > Why are you moving the update between vsycall and pvclock update as I
> > did in my patch? We really need to update everything before calling
> > somewhere.
> 
> Do you mean the call should be moved not just after tk_update_ktime_data
> (which sets base_mono), but further down after
> 
>         update_fast_timekeeper(tk);

No, it needs to be above update_vsyscall(). Here is the patch again
which I sent before. [https://lkml.org/lkml/2014/9/5/395]

Thanks,

	tglx

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index fb4a9c2cf8d9..ec1791fae965 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -442,11 +442,12 @@ static void timekeeping_update(struct timekeeper *tk, unsigned int action)
 		tk->ntp_error = 0;
 		ntp_clear();
 	}
-	update_vsyscall(tk);
-	update_pvclock_gtod(tk, action & TK_CLOCK_WAS_SET);
 
 	tk_update_ktime_data(tk);
 
+	update_vsyscall(tk);
+	update_pvclock_gtod(tk, action & TK_CLOCK_WAS_SET);
+
 	if (action & TK_MIRROR)
 		memcpy(&shadow_timekeeper, &tk_core.timekeeper,
 		       sizeof(tk_core.timekeeper));

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Paolo Bonzini]

Il 05/09/2014 22:41, Thomas Gleixner ha scritto:
> No, it needs to be above update_vsyscall(). Here is the patch again
> which I sent before. [https://lkml.org/lkml/2014/9/5/395]

Ah, I missed it after your signature.  Thanks, I'll test yours then next
week.

Paolo

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Chris J Arges]

On 09/05/2014 04:00 PM, Paolo Bonzini wrote:
> Il 05/09/2014 22:41, Thomas Gleixner ha scritto:
>> No, it needs to be above update_vsyscall(). Here is the patch again
>> which I sent before. [https://lkml.org/lkml/2014/9/5/395]
> 
> Ah, I missed it after your signature.  Thanks, I'll test yours then next
> week.
> 
> Paolo
> 
Paolo, Thomas,

Thanks again. I can confirm that Thomas' "timekeeping: Update timekeeper
before updating vsyscall and pvclock" indeed allows the kvmclock test to
pass on my machine.

--chris j arges

[PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Paolo Bonzini]

Commit cbcf2dd3b3d4 (x86: kvm: Make kvm_get_time_and_clockread() nanoseconds
based, 2014-07-16) used the wrong formula for boot_ns, thus breaking kvmclock on
hosts that have a reliable TSC.

To find the right formula, let's first backport the switch to nanoseconds
to 3.16-era timekeeping logic.  The full patch (which works) is at
https://lkml.org/lkml/2014/9/4/462.  The key line here is

        boot_ns = timespec_to_ns(&tk->total_sleep_time)
                + timespec_to_ns(&tk->wall_to_monotonic)
                + tk->xtime_sec * (u64)NSEC_PER_SEC;

Because the above patch works, the conclusion is that the above formula
is not the same as commit cbcf2dd3b3d4's

        boot_ns = ktime_to_ns(ktime_add(tk->tkr.base_mono, tk->offs_boot));

As to what is the right one, commit 02cba1598a2a (timekeeping: Simplify getboottime(),
2014-07-16) provides a hint:

   offs_real             = -wall-to_monotonic
   offs_boot             =  total_sleep_time

   offs_real - offs_boot = -wall_to_monotonic - total_sleep_time

that is

   offs_boot - offs_real =  wall_to_monotonic + total_sleep_time

which is what this patch uses, adding xtime_sec separately.  The "boot_ns"
moniker is not too clear, so rename boot_ns to nsec_base and the existing
nsec_base to snsec_base.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <john.stultz@linaro.org>
Reported-by: Chris J Arges <chris.j.arges@canonical.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	Thomas/John, the problem with the above explanation is that
	tk_update_ktime_data has "base_mono = xtime_sec + wtm", and from
	there "base_mono + offs_boot = xtime_sec + wtm + total_sleep_time".
	Except that doesn't work, so something must be wrong in
	tk_update_ktime_data's comment.

 arch/x86/kvm/x86.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8f1e22d3b286..c55203bea337 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1020,8 +1020,8 @@ struct pvclock_gtod_data {
 		u32	shift;
 	} clock;
 
-	u64		boot_ns;
 	u64		nsec_base;
+	u64		snsec_base;
 };
 
 static struct pvclock_gtod_data pvclock_gtod_data;
@@ -1031,7 +1031,7 @@ static void update_pvclock_gtod(struct timekeeper *tk)
 	struct pvclock_gtod_data *vdata = &pvclock_gtod_data;
 	u64 boot_ns;
 
-	boot_ns = ktime_to_ns(ktime_add(tk->tkr.base_mono, tk->offs_boot));
+	boot_ns = ktime_to_ns(ktime_sub(tk->tkr.offs_boot, tk->offs_real));
 
 	write_seqcount_begin(&vdata->seq);
 
@@ -1042,8 +1042,9 @@ static void update_pvclock_gtod(struct timekeeper *tk)
 	vdata->clock.mult		= tk->tkr.mult;
 	vdata->clock.shift		= tk->tkr.shift;
 
-	vdata->boot_ns			= boot_ns;
-	vdata->nsec_base		= tk->tkr.xtime_nsec;
+	vdata->nsec_base		= tk->xtime_sec * (u64)NSEC_PER_SEC
+					+ boot_ns;
+	vdata->snsec_base		= tk->tkr.xtime_nsec;
 
 	write_seqcount_end(&vdata->seq);
 }
@@ -1413,10 +1414,10 @@ static int do_monotonic_boot(s64 *t, cycle_t *cycle_now)
 	do {
 		seq = read_seqcount_begin(&gtod->seq);
 		mode = gtod->clock.vclock_mode;
-		ns = gtod->nsec_base;
+		ns = gtod->snsec_base;
 		ns += vgettsc(cycle_now);
 		ns >>= gtod->clock.shift;
-		ns += gtod->boot_ns;
+		ns += gtod->nsec_base;
 	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
 	*t = ns;
 
-- 
1.8.3.1

Re: [PATCH] KVM: x86: fix kvmclock breakage from timers branch merge

[Thomas Gleixner]

On Thu, 4 Sep 2014, Paolo Bonzini wrote:

> Commit cbcf2dd3b3d4 (x86: kvm: Make kvm_get_time_and_clockread() nanoseconds
> based, 2014-07-16) used the wrong formula for boot_ns, thus breaking kvmclock on
> hosts that have a reliable TSC.
> 
> To find the right formula, let's first backport the switch to nanoseconds
> to 3.16-era timekeeping logic.  The full patch (which works) is at
> https://lkml.org/lkml/2014/9/4/462.  The key line here is
> 
>         boot_ns = timespec_to_ns(&tk->total_sleep_time)
>                 + timespec_to_ns(&tk->wall_to_monotonic)
>                 + tk->xtime_sec * (u64)NSEC_PER_SEC;
> 
> Because the above patch works, the conclusion is that the above formula
> is not the same as commit cbcf2dd3b3d4's
> 
>         boot_ns = ktime_to_ns(ktime_add(tk->tkr.base_mono, tk->offs_boot));
> 
> As to what is the right one, commit 02cba1598a2a (timekeeping: Simplify getboottime(),
> 2014-07-16) provides a hint:
> 
>    offs_real             = -wall-to_monotonic
>    offs_boot             =  total_sleep_time
> 
>    offs_real - offs_boot = -wall_to_monotonic - total_sleep_time
> 
> that is
> 
>    offs_boot - offs_real =  wall_to_monotonic + total_sleep_time
> 
> which is what this patch uses, adding xtime_sec separately.  The "boot_ns"
> moniker is not too clear, so rename boot_ns to nsec_base and the existing
> nsec_base to snsec_base.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Stultz <john.stultz@linaro.org>
> Reported-by: Chris J Arges <chris.j.arges@canonical.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	Thomas/John, the problem with the above explanation is that
> 	tk_update_ktime_data has "base_mono = xtime_sec + wtm", and from
> 	there "base_mono + offs_boot = xtime_sec + wtm + total_sleep_time".
> 	Except that doesn't work, so something must be wrong in
> 	tk_update_ktime_data's comment.

Right. I'm staring into it and we need to fix the core code issue and
not the usage site.

Thanks,

	tglx