pv clock: kvm is incompatible with xen :-(

Hi,

Tried to use kvmclock with xenner and noticed that the kvmclock (MSR_KVM_SYSTEM_TIME msr) is incompatible with xen.

kvm guests do this to translate the tsc delta into nsecs:

   #define get_clock(cpu, field) per_cpu(hv_clock, cpu).field

   static inline u64 kvm_get_delta(u64 last_tsc)
   {
           int cpu = smp_processor_id();
           u64 delta = native_read_tsc() - last_tsc;
           return (delta * get_clock(cpu, tsc_to_system_mul)) >> 22;
   }

whereas xen guests do this (64bit version):

   static inline u64 scale_delta(u64 delta, u32 mul_frac, int shift)
   {
           u64 product;

           if (shift < 0)
                   delta >>= -shift;
           else
                   delta <<= shift;

           __asm__ (
                   "mul %%rdx ; shrd $32,%%rdx,%%rax"
                   : "=a" (product) : "0" (delta), "d" ((u64)mul_frac));
           return product;

}

Note that xen does a 64bit multiply (of the 64bit delta and the 32bit factor) yielding a 128bit result, then picking bits 32-95 for the 64bit return value. In contrast kvm does a simple 64bit multiply, which is equivalent to using the lowest 64 bits. Thus kvm is off by factor 2^32, and that without even considering the ordering of two (shift + multiply) operations and any rounding errors ...

cheers,
Gerd

http://kraxel.fedorapeople.org/xenner/

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Patches are welcome, especially as kvmclock isn't merged yet, so there are no backward compatibility issues.

Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Great, so I'll happily go break kvm guests ;)

Patch revision #1 attached. It changes the way the tsc-delta-scaling fields are calculated to be compatible with xen. Code is taken from xenner (which got it from xen) and adapted a bit. Host only, kvm guest side not done (yet).

With that patch applied xen guests with pv clock enabled happily boot to the login prompt, without complains about time going backwards. Fine.

Wall clock is off a few hours though. Oops.

I think the way wall clock and system clock work together in xen (Jeremy correct me if I'm wrong) is that the wall clock specifies the point in time where the system clock started going. As kvm fills in host system time into the guest system time fields the guest wall clock fields should be filled with the host boot time timestamp I'd say.

Comments?

Gerd

http://kraxel.fedorapeople.org/xenner/

diff -up kvm-65/kernel/x86.c.fix kvm-65/kernel/x86.c
@@ -548,6 +548,44 @@ static void kvm_write_guest_time(struct 
 	mark_page_dirty(v->kvm, vcpu->time >> PAGE_SHIFT);
 }
 
+static uint32_t div_frac(uint32_t dividend, uint32_t divisor)
+{
+	uint32_t quotient, remainder;
+	
+	__asm__ ( "divl %4"
+		  : "=a" (quotient), "=d" (remainder)
+		  : "0" (0), "1" (dividend), "r" (divisor) );
+	return quotient;
+}
+
+static void kvm_set_time_scale(uint32_t tsc_khz, struct kvm_vcpu_time_info *hv_clock)
+{
+	uint64_t nsecs = 1000000000LL;
+	int32_t  shift = 0;
+	uint64_t tps64;
+	uint32_t tps32;
+	
+	tps64 = tsc_khz * 1000LL;
+	while (tps64 > nsecs*2) {
+		tps64 >>= 1;
+		shift--;
+	}
+
+	tps32 = (uint32_t)tps64;
+	while (tps32 <= (uint32_t)nsecs) {
+		tps32 <<= 1;
+		shift++;
+	}
+
+	hv_clock->tsc_shift = shift;
+	hv_clock->tsc_to_system_mul = div_frac(nsecs, tps32);
+
+#if 0
+	printk(KERN_DEBUG "%s: tsc_khz %u, tsc_shift %d, tsc_mul %u\n",
+	       __FUNCTION__, tsc_khz, hv_clock->tsc_shift,
+	       hv_clock->tsc_to_system_mul);
+#endif
+}
 
 int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 {

@@ -596,9 +634,7 @@ int kvm_set_msr_common(struct kvm_vcpu *
 		/* ...but clean it before doing the actual write */
 		vcpu->arch.time_offset = data & ~(PAGE_MASK | 1);
 
-		vcpu->arch.hv_clock.tsc_to_system_mul =
-					clocksource_khz2mult(kvm_tsc_khz, 22);
-		vcpu->arch.hv_clock.tsc_shift = 22;
+		kvm_set_time_scale(kvm_tsc_khz, &vcpu->arch.hv_clock);
 
 		down_read(&current->mm->mmap_sem);
 		vcpu->arch.time_page =

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Following up myself with a quick&dirty patch to tackle this issue too. This one calculates the boot time. That should be solveable better, include/linux/time.h lists two functions which sound promising:

  extern void getboottime(struct timespec *ts);
  extern void monotonic_to_bootbased(struct timespec *ts);

Neither of them is available to modules though, so I can't test without rebooting my laptop ...

monotonic_to_bootbased() sounds like we would get hosts ntp adjustments in the guests for free.

cheers,
Gerd

http://kraxel.fedorapeople.org/xenner/

diff -up kvm-65/kernel/x86.c.fix kvm-65/kernel/x86.c
@@ -490,7 +490,7 @@ static void kvm_write_wall_clock(struct 
 {
 	static int version;
 	struct kvm_wall_clock wc;
-	struct timespec wc_ts;
+	struct timespec now,sys,boot;
 
 	if (!wall_clock)
 		return;

@@ -499,9 +499,11 @@ static void kvm_write_wall_clock(struct 
 
 	kvm_write_guest(kvm, wall_clock, &version, sizeof(version));
 
-	wc_ts = current_kernel_time();
-	wc.wc_sec = wc_ts.tv_sec;
-	wc.wc_nsec = wc_ts.tv_nsec;
+	now = current_kernel_time();
+	ktime_get_ts(&sys);
+	boot = ns_to_timespec(timespec_to_ns(&now) - timespec_to_ns(&sys));
+	wc.wc_sec = boot.tv_sec;
+	wc.wc_nsec = boot.tv_nsec;
 	wc.wc_version = version;
 
 	kvm_write_guest(kvm, wall_clock, &wc, sizeof(wc));

@@ -548,6 +550,44 @@ static void kvm_write_guest_time(struct 
 	mark_page_dirty(v->kvm, vcpu->time >> PAGE_SHIFT);
 }
 
+static uint32_t div_frac(uint32_t dividend, uint32_t divisor)
+{
+	uint32_t quotient, remainder;
+	
+	__asm__ ( "divl %4"
+		  : "=a" (quotient), "=d" (remainder)
+		  : "0" (0), "1" (dividend), "r" (divisor) );
+	return quotient;
+}
+
+static void kvm_set_time_scale(uint32_t tsc_khz, struct kvm_vcpu_time_info *hv_clock)
+{
+	uint64_t nsecs = 1000000000LL;
+	int32_t  shift = 0;
+	uint64_t tps64;
+	uint32_t tps32;
+	
+	tps64 = tsc_khz * 1000LL;
+	while (tps64 > nsecs*2) {
+		tps64 >>= 1;
+		shift--;
+	}
+
+	tps32 = (uint32_t)tps64;
+	while (tps32 <= (uint32_t)nsecs) {
+		tps32 <<= 1;
+		shift++;
+	}
+
+	hv_clock->tsc_shift = shift;
+	hv_clock->tsc_to_system_mul = div_frac(nsecs, tps32);
+
+#if 0
+	printk(KERN_DEBUG "%s: tsc_khz %u, tsc_shift %d, tsc_mul %u\n",
+	       __FUNCTION__, tsc_khz, hv_clock->tsc_shift,
+	       hv_clock->tsc_to_system_mul);
+#endif
+}
 
 int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 {

@@ -596,9 +636,7 @@ int kvm_set_msr_common(struct kvm_vcpu *
 		/* ...but clean it before doing the actual write */
 		vcpu->arch.time_offset = data & ~(PAGE_MASK | 1);
 
-		vcpu->arch.hv_clock.tsc_to_system_mul =
-					clocksource_khz2mult(kvm_tsc_khz, 22);
-		vcpu->arch.hv_clock.tsc_shift = 22;
+		kvm_set_time_scale(kvm_tsc_khz, &vcpu->arch.hv_clock);
 
 		down_read(&current->mm->mmap_sem);
 		vcpu->arch.time_page =

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Yes. The wallclock field in the shared info structure is the wallclock time at boot; you compute the current time by adding the system
timestamp to it. System time changes are effected by retroactively
changing the boot time of the machine, though that can also change because of suspend/resume/migrate.

In general the kernel only reads the wallclock time at boot, and then maintains it for itself from then on. I think.

J

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Thanks.

I'm looking at the guest side of the issue right now, trying to identify common code, and while doing so noticed that xen does the version-check-loop in both get_time_values_from_xen(void) and xen_clocksource_read(void), and I can't see any obvious reason for that.
The loop in xen_clocksource_read(void) is not needed IMHO. Can I drop it?

cheers,
Gerd

http://kraxel.fedorapeople.org/xenner/

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

No. The get_nsec_offset() needs to be atomic with respect to the get_time_values() parameters. There could be a loopless __get_time_values() for use in this case, but given that it almost never loops, I don't think its worthwhile.

J

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Hmm, I somehow fail to see a case where it could be non-atomic ...

get_time_values() copies a consistent snapshot, thus xen_clocksource_read() doesn't race against xen updating the fields. The snapshot is in a per-cpu variable, thus it doesn't race against other guest vcpus running get_time_values() at the same time.

"in this case" ??? I'm confused. There is only a single user of get_nsec_offset(), which is xen_clocksource_read() ...

cheers,
Gerd

http://kraxel.fedorapeople.org/xenner/

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Xen could change the parameters in the instant after get_time_values(). That change could be as a result of suspend-resume, so the parameters and the tsc could be wildly different. It's definitely an edge-case, but it's easy enough to deal with.

Sure, but get_time_values() has several other callers. If xen_clocksource_read() had its own loop to make sure the read_tsc is atomic with respect to get_time_values, then get_time_values itself needn't loop. But, given that it only loops in the very rare case that it races with Xen updating those parameters, it doesn't seem to make much difference either way.

J

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Ah, ok, forgot the rdtsc in the picture. With that in mind I fully agree that the loop is needed. I think kvm guests can even hit that one with the vcpu migrating to a different physical cpu, so we better handle it correctly ;)

Not really. There are only two calls, one in clocksource_read() and one in the init path. The later is superfluous I think because clocksource_read() is the only user of the shadowed time info.

cheers,
Gerd

http://kraxel.fedorapeople.org/xenner/

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Yes, same with Xen.

Hm. It doesn't look like shadow_time needs to be a static percpu at all. It could just be a local to clocksource_read, I think.

J

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Good point, one more cleanup.

thanks,
Gerd

http://kraxel.fedorapeople.org/xenner/

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

It's probably not needed for kvm, since we update everything everytime we get scheduled in the host side, which would cover the case for migration between physical cpus. But it's probably okay to do it to get a common denominator with xen, if needed.

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

No, it wouldn't. The corner case we must catch is: guest reads time info, kvm reschedules the guest to another pcpu, guest reads the tsc. The time info used by the guest for the tsc delta is stale then, it belongs to the previous pcpu.

cheers,
Gerd

http://kraxel.fedorapeople.org/xenner/

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Agreed.

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone