KVM: x86: fix kvmclock breakage from timers branch merge

Commit Message

Chris J Arges Sept. 4, 2014, 8:37 p.m. UTC

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

--

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Paolo Bonzini Sept. 4, 2014, 8:40 p.m. UTC | #1

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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chris J Arges Sept. 4, 2014, 8:43 p.m. UTC | #2

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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;