Kvm clocksource, new spin

PATCH In review Glauber de Oliveira Costa 5 participants 16 messages Nov 8, 2007 – Nov 14, 2007

Subsystems (3)

x86 KVM KVM paravirt

lore

Share & tools

lore

Glauber de Oliveira Costa

author

2007-11-08 21:50 UTC #0 lore raw

cover Kvm clocksource, new spin

Hi folks,

Here's a new spin of the clocksource implementation.
In this new version:
* followed avi's suggestion of:
- letting the cpu itself register its memory area.
- using a gfn instead of a phys addr as a parameter, to be sure we can cover the whole memory area
- write guest time at exits.

Also, I 'm not using an anonymous struct in the kvm_hv_clock union, so the vcpu struct can grab just what it needs, and not the whole padding the guest needs

This is it.

Have fun

Glauber de Oliveira Costa

author

2007-11-08 21:40 UTC #1 lore raw

PATCH 1/3 include files for kvmclock

This patch introduces the include files for kvm clock. They'll be needed for both guest and host part.

Signed-off-by Glauber de Oliveira Costa <[email protected]>

---
 include/asm-x86/kvm_para.h |   25 +++++++++++++++++++++++++
 include/linux/kvm.h        |    1 +
 include/linux/kvm_para.h   |    2 ++
 3 files changed, 28 insertions(+), 0 deletions(-)

include/asm-x86/kvm_para.h +25 -0

index c6f3fd8..0f6b813 100644
@@ -10,15 +10,40 @@
  * paravirtualization, the appropriate feature bit should be checked.
  */
 #define KVM_CPUID_FEATURES	0x40000001
+#define KVM_FEATURE_CLOCKSOURCE 0
 
 #ifdef __KERNEL__
 #include <asm/processor.h>
+extern void kvmclock_init(void);
+
+/*
+ * Guest has page alignment and padding requirements. At the host, it will
+ * only lead to wasted space at the vcpu struct. For this reason, the struct
+ * is not anonymous
+ */
+union kvm_hv_clock {
+	struct kvm_hv_clock_s {
+		u64 tsc_mult;
+		u64 now_ns;
+		/* That's the wall clock, not the water closet */
+		u64 wc_sec;
+		u64 last_tsc;
+		/* At first, we could use the tsc value as a marker, but Jeremy
+		 * well noted that it will cause us locking problems in 32-bit
+		 * sys, so we have a special version field */
+		u32 version;
+	} fields;
+	char page_align[PAGE_SIZE];
+};
+
 
 /* This instruction is vmcall.  On non-VT architectures, it will generate a
  * trap that we will then rewrite to the appropriate instruction.
  */
 #define KVM_HYPERCALL ".byte 0x0f,0x01,0xc1"
 
+#define KVM_HCALL_REGISTER_CLOCK	1
+
 /* For KVM hypercalls, a three-byte sequence of either the vmrun or the vmmrun
  * instruction.  The hypervisor may replace it with something else but only the
  * instructions are guaranteed to be supported.

include/linux/kvm.h +1 -0

index 71d33d6..9862241 100644
@@ -359,6 +359,7 @@ struct kvm_signal_mask {
 #define KVM_CAP_MMU_SHADOW_CACHE_CONTROL 2
 #define KVM_CAP_USER_MEMORY 3
 #define KVM_CAP_SET_TSS_ADDR 4
+#define KVM_CAP_CLOCKSOURCE	  5
 
 /*
  * ioctls for VM fds

include/linux/kvm_para.h +2 -0

index e4db25f..094efc7 100644
@@ -11,6 +11,8 @@
 
 /* Return values for hypercalls */
 #define KVM_ENOSYS		1000
+#define KVM_EINVAL		1019
+#define KVM_ENODEV		1022
 
 #ifdef __KERNEL__
 /*

Signature · 1 line

1.5.0.6

Glauber de Oliveira Costa

author

2007-11-08 21:40 UTC #2 lore raw

PATCH 2/3 kvmclock - the host part.

This is the host part of kvm clocksource implementation. As it does not include clockevents, it is a fairly simple implementation. We only have to register a per-vcpu area, and start writting to it periodically.

Signed-off-by Glauber de Oliveira Costa <[email protected]>

---
 drivers/kvm/kvm_main.c |    1 +
 drivers/kvm/x86.c      |   32 ++++++++++++++++++++++++++++++++
 drivers/kvm/x86.h      |    4 ++++
 3 files changed, 37 insertions(+), 0 deletions(-)

drivers/kvm/kvm_main.c +1 -0

index d095002..c2c79b8 100644
@@ -1243,6 +1243,7 @@ static long kvm_dev_ioctl(struct file *filp,
 		case KVM_CAP_MMU_SHADOW_CACHE_CONTROL:
 		case KVM_CAP_USER_MEMORY:
 		case KVM_CAP_SET_TSS_ADDR:
+		case KVM_CAP_CLOCKSOURCE:
 			r = 1;
 			break;
 		default:

drivers/kvm/x86.c +32 -0

index e905d46..ef31fed 100644
@@ -19,6 +19,7 @@
 #include "segment_descriptor.h"
 #include "irq.h"
 
+#include <linux/clocksource.h>
 #include <linux/kvm.h>
 #include <linux/fs.h>
 #include <linux/vmalloc.h>

@@ -1628,6 +1629,28 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_emulate_halt);
 
+static void kvm_write_guest_time(struct kvm_vcpu *vcpu)
+{
+	struct timespec ts;
+	int r;
+
+	if (!vcpu->clock_gpa)
+		return;
+
+	/* Updates version to the next odd number, indicating we're writing */
+	vcpu->hv_clock.version++;
+	kvm_write_guest(vcpu->kvm, vcpu->clock_gpa, &vcpu->hv_clock, PAGE_SIZE);
+
+	kvm_get_msr(vcpu, MSR_IA32_TIME_STAMP_COUNTER,
+			  &vcpu->hv_clock.last_tsc);
+
+	ktime_get_ts(&ts);
+	vcpu->hv_clock.now_ns = ts.tv_nsec + (NSEC_PER_SEC * (u64)ts.tv_sec);
+	vcpu->hv_clock.wc_sec = get_seconds();
+	vcpu->hv_clock.version++;
+	kvm_write_guest(vcpu->kvm, vcpu->clock_gpa, &vcpu->hv_clock, PAGE_SIZE);
+}
+
 int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 {
 	unsigned long nr, a0, a1, a2, a3, ret;

@@ -1648,7 +1671,15 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 		a3 &= 0xFFFFFFFF;
 	}
 
+	ret = 0;
 	switch (nr) {
+	case  KVM_HCALL_REGISTER_CLOCK:
+
+		vcpu->clock_gpa = a0 << PAGE_SHIFT;
+		vcpu->hv_clock.tsc_mult = clocksource_khz2mult(tsc_khz, 22);
+
+		break;
+
 	default:
 		ret = -KVM_ENOSYS;
 		break;

@@ -1924,6 +1955,7 @@ out:
 		goto preempted;
 	}
 
+	kvm_write_guest_time(vcpu);
 	post_kvm_run_save(vcpu, kvm_run);
 
 	return r;

drivers/kvm/x86.h +4 -0

index 663b822..fd77b66 100644
@@ -83,6 +83,10 @@ struct kvm_vcpu {
 	/* emulate context */
 
 	struct x86_emulate_ctxt emulate_ctxt;
+
+	struct kvm_hv_clock_s hv_clock;
+	gpa_t clock_gpa; /* guest frame number, physical addr */
+
 };
 
 int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t gva, u32 error_code);

Signature · 1 line

1.5.0.6

Glauber de Oliveira Costa

author

2007-11-08 21:39 UTC #3 lore raw

PATCH 3/3 kvmclock implementation, the guest part.

This is the guest part of kvm clock implementation It does not do tsc-only timing, as tsc can have deltas between cpus, and it did not seem worthy to me to keep adjusting them.

We do use it, however, for fine-grained adjustment.

Other than that, time comes from the host.

Signed-off-by Glauber de Oliveira Costa <[email protected]>

---
 arch/x86/Kconfig.i386       |   10 +++
 arch/x86/kernel/Makefile_32 |    1 +
 arch/x86/kernel/kvmclock.c  |  171 +++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/setup_32.c  |    5 +
 4 files changed, 187 insertions(+), 0 deletions(-)
 create mode 100644 arch/x86/kernel/kvmclock.c

arch/x86/Kconfig.i386 +10 -0

index 7331efe..5fe4025 100644
@@ -257,6 +257,16 @@ config VMI
 	  at the moment), by linking the kernel to a GPL-ed ROM module
 	  provided by the hypervisor.
 
+config KVM_CLOCK
+	bool "KVM paravirtualized clock"
+	select PARAVIRT
+	help
+	  Turning on this option will allow you to run a paravirtualized clock
+	  when running over the KVM hypervisor. Instead of relying on a PIT
+	  (or probably other) emulation by the underlying device model, the host
+	  provides the guest with timing infrastructure, as time of day, and
+	  timer expiration.
+
 source "arch/x86/lguest/Kconfig"
 
 endif

arch/x86/kernel/Makefile_32 +1 -0

index b9d6798..df76d8c 100644
@@ -43,6 +43,7 @@ obj-$(CONFIG_K8_NB)		+= k8.o
 obj-$(CONFIG_MGEODE_LX)		+= geode_32.o mfgpt_32.o
 
 obj-$(CONFIG_VMI)		+= vmi_32.o vmiclock_32.o
+obj-$(CONFIG_KVM_CLOCK)		+= kvmclock.o
 obj-$(CONFIG_PARAVIRT)		+= paravirt_32.o
 obj-y				+= pcspeaker.o

arch/x86/kernel/kvmclock.c new file +171 -0

171 lines not on this page · raw message

arch/x86/kernel/setup_32.c +5 -0

index e1e18c3..9f13ff6 100644
@@ -44,6 +44,7 @@
 #include <linux/crash_dump.h>
 #include <linux/dmi.h>
 #include <linux/pfn.h>
+#include <linux/kvm_para.h>
 
 #include <video/edid.h>

@@ -614,6 +615,10 @@ void __init setup_arch(char **cmdline_p)
 
 	max_low_pfn = setup_memory();
 
+#ifdef CONFIG_KVM_CLOCK
+	kvmclock_init();
+#endif
+
 #ifdef CONFIG_VMI
 	/*
 	 * Must be after max_low_pfn is determined, and before kernel

Signature · 1 line

1.5.0.6

Avi Kivity

2007-11-11 10:17 UTC #4 lore raw

Re: [PATCH 2/3] kvmclock - the host part.

In reply to Glauber de Oliveira Costa

Quoted 5 lines · Glauber de Oliveira Costa

This is the host part of kvm clocksource implementation. As it does not include clockevents, it is a fairly simple implementation. We only have to register a per-vcpu area, and start writting to it periodically.

Missing live migration support (a way for userspace to read and write the guest clock address). Should probably be in a separate patch.

drivers/kvm/x86.c out: · line 1960

Quoted 7 lines

drivers/kvm/x86.c +1 -0

@@ -1924,6 +1955,7 @@ out:
 		goto preempted;
 	}
 
+	kvm_write_guest_time(vcpu);
 	post_kvm_run_save(vcpu, kvm_run);

Why here? Seems like we're leaving the guest for a while at this place.

Suggest putting it on top of __vcpu_run(), guarded by a flag, and setting the flag every time we put the vcpu.

Signature · 8 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Glauber de Oliveira Costa

author

2007-11-13 11:27 UTC #5 lore raw

Re: [PATCH 2/3] kvmclock - the host part.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Avi Kivity escreveu:

Quoted 10 lines

Quoted 6 lines · Glauber de Oliveira Costa
This is the host part of kvm clocksource implementation. As it does not include clockevents, it is a fairly simple implementation. We only have to register a per-vcpu area, and start writting to it periodically.
Missing live migration support (a way for userspace to read and write the guest clock address). Should probably be in a separate patch.

I think it's a matter of issuing a hypercall for reading the clock address. It's fair simple, and can be done in a later version of this patch. As for writting, the register hypercall itself can be used. It has no special side-effects we should care about.

Quoted 4 lines

drivers/kvm/x86.c +0 -0

@@ -1924,6 +1955,7 @@ out:
         goto preempted;
     }

Quoted 8 lines

drivers/kvm/x86.c out: · line 1960
Quoted 3 lines
drivers/kvm/x86.c +1 -0
+    kvm_write_guest_time(vcpu);
     post_kvm_run_save(vcpu, kvm_run);
 
Why here? Seems like we're leaving the guest for a while at this place.
Suggest putting it on top of __vcpu_run(), guarded by a flag, and setting the flag every time we put the vcpu.

No special preference. It just sounded exity enough to me. I can move to where you suggest.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: Using GnuPG with Remi - http://enigmail.mozdev.org

iD8DBQFHOYpojYI8LaFUWXMRApf8AJ4jQ/ZTBlub1IwFkJrYZyart+f7bwCfT+9m l1Rblsmw96ZatCf60g2dNYY=
=DBpn
-----END PGP SIGNATURE-----

Avi Kivity

2007-11-13 14:44 UTC #6 lore raw

Re: [PATCH 2/3] kvmclock - the host part.

Quoted 14 lines · Glauber de Oliveira Costa

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Avi Kivity escreveu:
Quoted 9 lines
Quoted 7 lines · Glauber de Oliveira Costa
This is the host part of kvm clocksource implementation. As it does not include clockevents, it is a fairly simple implementation. We only have to register a per-vcpu area, and start writting to it periodically.

Quoted 9 lines

Quoted 3 lines
Missing live migration support (a way for userspace to read and write the guest clock address). Should probably be in a separate patch.
I think it's a matter of issuing a hypercall for reading the clock address. It's fair simple, and can be done in a later version of this patch. As for writting, the register hypercall itself can be used. It has no special side-effects we should care about.

kvm live migration is done (at least thus far) without guest involvement. So you need the host to be able to transfer this state.

Quoted 11 lines

Quoted 11 lines
drivers/kvm/x86.c out: · line 1961
Quoted 9 lines
drivers/kvm/x86.c +1 -0
@@ -1924,6 +1955,7 @@ out:
         goto preempted;
     }
 
+    kvm_write_guest_time(vcpu);
     post_kvm_run_save(vcpu, kvm_run);
 
 
Why here? Seems like we're leaving the guest for a while at this place.

Quoted 8 lines

Quoted 3 lines
Suggest putting it on top of __vcpu_run(), guarded by a flag, and setting the flag every time we put the vcpu.
No special preference. It just sounded exity enough to me. I can move to where you suggest.

It's more than a place, it's a set of rules:

- if the vcpu is migrated, we need a new timebase
- ditto if we're descheduled (well that's the same thing)
- if "some time passes"

Signature · 8 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Dong, Eddie

2007-11-13 05:00 UTC #7 lore raw

RE: [kvm-devel] [PATCH 2/3] kvmclock - the host part.

In reply to Glauber de Oliveira Costa

Quoted 13 lines

drivers/kvm/x86.c +8 -0

+static void kvm_write_guest_time(struct kvm_vcpu *vcpu) +{
+	struct timespec ts;
+	int r;
+
+	if (!vcpu->clock_gpa)
+		return;
+
+	/* Updates version to the next odd number, indicating

we're writing */

(quoted diff) +2 -0

+	vcpu->hv_clock.version++;
+	kvm_write_guest(vcpu->kvm, vcpu->clock_gpa,

&vcpu->hv_clock, PAGE_SIZE);

drivers/kvm/x86.c int kvm_emulate_halt(struct kvm_vcpu *vcpu) · line 1647

Quoted 5 lines

drivers/kvm/x86.c +5 -0

+
+	kvm_get_msr(vcpu, MSR_IA32_TIME_STAMP_COUNTER,
+			  &vcpu->hv_clock.last_tsc);
+
+	ktime_get_ts(&ts);

Do we need to disable preemption here?

drivers/kvm/x86.c int kvm_emulate_halt(struct kvm_vcpu *vcpu) · line 1653

Quoted 7 lines

(quoted diff) +1 -0
+	vcpu->hv_clock.now_ns = ts.tv_nsec + (NSEC_PER_SEC *
(u64)ts.tv_sec); + vcpu->hv_clock.wc_sec = get_seconds();
(quoted diff) +2 -0
+	vcpu->hv_clock.version++;
+	kvm_write_guest(vcpu->kvm, vcpu->clock_gpa,
&vcpu->hv_clock, PAGE_SIZE);
drivers/kvm/x86.c +2 -0
+}
+

thx,eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/

Glauber de Oliveira Costa

author

2007-11-13 11:54 UTC #8 lore raw

Re: [kvm-devel] [PATCH 2/3] kvmclock - the host part.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Dong, Eddie escreveu:

Quoted 11 lines

drivers/kvm/x86.c +8 -0

+static void kvm_write_guest_time(struct kvm_vcpu *vcpu) +{
+	struct timespec ts;
+	int r;
+
+	if (!vcpu->clock_gpa)
+		return;
+
+	/* Updates version to the next odd number, indicating

we're writing */

(quoted diff) +2 -0

+	vcpu->hv_clock.version++;
+	kvm_write_guest(vcpu->kvm, vcpu->clock_gpa,

Quoted 8 lines

drivers/kvm/x86.c int kvm_emulate_halt(struct kvm_vcpu *vcpu) · line 1647
Quoted 6 lines
&vcpu->hv_clock, PAGE_SIZE);
drivers/kvm/x86.c +5 -0
+
+	kvm_get_msr(vcpu, MSR_IA32_TIME_STAMP_COUNTER,
+			  &vcpu->hv_clock.last_tsc);
+
+	ktime_get_ts(&ts);
Do we need to disable preemption here?

After thinking for a little while, you are theoretically right. In the current state, we could even be preempted between all operations ;-) Maybe after avi's suggestion of moving the call to it it will end up in a preempt safe region, but anyway, it's safer to add the preempt markers here.
I'll put it in next version, thanks

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: Using GnuPG with Remi - http://enigmail.mozdev.org

iD8DBQFHOZBrjYI8LaFUWXMRAo81AKCfbkzhLl7F6BUjzUHVyErCFeHxFACg1teB eqsOnJiAqB3JiYf+2YdMZ4o=
=ENKU
-----END PGP SIGNATURE-----

Izik Eidus

2007-11-13 12:08 UTC #9 lore raw

Re: [kvm-devel] [PATCH 2/3] kvmclock - the host part.

Quoted 25 lines · Glauber de Oliveira Costa

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Dong, Eddie escreveu:
Quoted 11 lines
Quoted 11 lines
drivers/kvm/x86.c +8 -0
+static void kvm_write_guest_time(struct kvm_vcpu *vcpu) +{
+	struct timespec ts;
+	int r;
+
+	if (!vcpu->clock_gpa)
+		return;
+
+	/* Updates version to the next odd number, indicating
we're writing */
(quoted diff) +2 -0
+	vcpu->hv_clock.version++;
+	kvm_write_guest(vcpu->kvm, vcpu->clock_gpa,
Quoted 9 lines
drivers/kvm/x86.c int kvm_emulate_halt(struct kvm_vcpu *vcpu) · line 1647
Quoted 7 lines
&vcpu->hv_clock, PAGE_SIZE);
drivers/kvm/x86.c +5 -0
+
+	kvm_get_msr(vcpu, MSR_IA32_TIME_STAMP_COUNTER,
+			  &vcpu->hv_clock.last_tsc);
+
+	ktime_get_ts(&ts);
Do we need to disable preemption here?

Quoted 7 lines

After thinking for a little while, you are theoretically right. In the current state, we could even be preempted between all operations ;-) Maybe after avi's suggestion of moving the call to it it will end up in a preempt safe region, but anyway, it's safer to add the preempt markers here.
I'll put it in next version, thanks

note that you cant call to kvm_write_guest with preemption disabled (witch you do few lines below)

Avi Kivity

2007-11-13 14:47 UTC #10 lore raw

Re: [kvm-devel] [PATCH 2/3] kvmclock - the host part.

In reply to Glauber de Oliveira Costa

Quoted 25 lines · Glauber de Oliveira Costa

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Dong, Eddie escreveu:
Quoted 11 lines
Quoted 11 lines
drivers/kvm/x86.c +8 -0
+static void kvm_write_guest_time(struct kvm_vcpu *vcpu) +{
+	struct timespec ts;
+	int r;
+
+	if (!vcpu->clock_gpa)
+		return;
+
+	/* Updates version to the next odd number, indicating
we're writing */
(quoted diff) +2 -0
+	vcpu->hv_clock.version++;
+	kvm_write_guest(vcpu->kvm, vcpu->clock_gpa,
Quoted 9 lines
drivers/kvm/x86.c int kvm_emulate_halt(struct kvm_vcpu *vcpu) · line 1647
Quoted 7 lines
&vcpu->hv_clock, PAGE_SIZE);
drivers/kvm/x86.c +5 -0
+
+	kvm_get_msr(vcpu, MSR_IA32_TIME_STAMP_COUNTER,
+			  &vcpu->hv_clock.last_tsc);
+
+	ktime_get_ts(&ts);
Do we need to disable preemption here?

Quoted 8 lines

After thinking for a little while, you are theoretically right. In the current state, we could even be preempted between all operations ;-) Maybe after avi's suggestion of moving the call to it it will end up in a preempt safe region, but anyway, it's safer to add the preempt markers here.
I'll put it in next version, thanks

Well, you can't kvm_write_guest() with preemption enabled.

preempt notifiers to the rescue! We have a callout during preemption, so you can just zero out a flag there, and when we're scheduled again retry the whole thing.

Signature · 8 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Dong, Eddie

2007-11-13 15:23 UTC #11 lore raw

RE: [kvm-devel] [PATCH 2/3] kvmclock - the host part.

Quoted 32 lines · Avi Kivity

Quoted 23 lines · Glauber de Oliveira Costa
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Dong, Eddie escreveu:
Quoted 9 lines
Quoted 9 lines
drivers/kvm/x86.c +7 -0
+static void kvm_write_guest_time(struct kvm_vcpu *vcpu) +{
+	struct timespec ts; +	int r;
+
+	if (!vcpu->clock_gpa)
+		return;
+
+	/* Updates version to the next odd number, indicating we're
writing */ + vcpu->hv_clock.version++;
(quoted diff) +1 -0
+	kvm_write_guest(vcpu->kvm, vcpu->clock_gpa,
Quoted 9 lines
drivers/kvm/x86.c int kvm_emulate_halt(struct kvm_vcpu *vcpu) · line 1647
Quoted 7 lines
&vcpu->hv_clock, PAGE_SIZE);
drivers/kvm/x86.c +5 -0
+
+	kvm_get_msr(vcpu, MSR_IA32_TIME_STAMP_COUNTER,
+			  &vcpu->hv_clock.last_tsc);
+
+	ktime_get_ts(&ts);
Do we need to disable preemption here?
Quoted 7 lines
After thinking for a little while, you are theoretically right. In the current state, we could even be preempted between all operations ;-) Maybe after avi's suggestion of moving the call to it it will end up in a preempt safe region, but anyway, it's safer to add the preempt markers here. I'll put it in next version, thanks

Quoted 6 lines

Well, you can't kvm_write_guest() with preemption enabled.
preempt notifiers to the rescue! We have a callout during preemption, so you can just zero out a flag there, and when we're scheduled again retry the whole thing.

The preemption issue is within following code which need to be done in a short enough period.

+ kvm_get_msr(vcpu, MSR_IA32_TIME_STAMP_COUNTER,
+ &vcpu->hv_clock.last_tsc);
+
+ ktime_get_ts(&ts);
+ vcpu->hv_clock.now_ns = ts.tv_nsec + (NSEC_PER_SEC *
(u64)ts.tv_sec);
+ vcpu->hv_clock.wc_sec = get_seconds();

I am even thinking we have to disable interrupt between these lines, otherwise
guest wall clock may see backward time source when calculating the delta TSC since last vcpu->hv_clock.now_ns update.

Also why we use both ktime_get_ts(&ts) & get_seconds() together? they are not atomic
and may cause issues?

thx,eddie

Avi Kivity

2007-11-13 16:12 UTC #12 lore raw

Re: [kvm-devel] [PATCH 2/3] kvmclock - the host part.

Quoted 28 lines · Eddie

Quoted 9 lines
Quoted 9 lines
After thinking for a little while, you are theoretically right. In the current state, we could even be preempted between all operations ;-) Maybe after avi's suggestion of moving the call to it it will end up in a preempt safe region, but anyway, it's safer to add the preempt markers here. I'll put it in next version, thanks
Quoted 7 lines
Well, you can't kvm_write_guest() with preemption enabled.
preempt notifiers to the rescue! We have a callout during preemption, so you can just zero out a flag there, and when we're scheduled again retry the whole thing.
The preemption issue is within following code which need to be done in a short enough period.
drivers/kvm/x86.c +5 -0
+	kvm_get_msr(vcpu, MSR_IA32_TIME_STAMP_COUNTER,
+			  &vcpu->hv_clock.last_tsc);
+
+	ktime_get_ts(&ts);
+	vcpu->hv_clock.now_ns = ts.tv_nsec + (NSEC_PER_SEC *
(u64)ts.tv_sec);
drivers/kvm/x86.c +1 -0
+	vcpu->hv_clock.wc_sec = get_seconds();

Quoted 5 lines

I am even thinking we have to disable interrupt between these lines, otherwise
guest wall clock may see backward time source when calculating the delta TSC since last vcpu->hv_clock.now_ns update.

That's true. While we do need to handle vcpu migration and descheduling, the code sequence you note needs to be as atomic as possible.

Signature · 8 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Dong, Eddie

2007-11-14 00:41 UTC #13 lore raw

RE: [kvm-devel] [PATCH 2/3] kvmclock - the host part.

Quoted 8 lines

Quoted 8 lines
drivers/kvm/x86.c +5 -0
+	kvm_get_msr(vcpu, MSR_IA32_TIME_STAMP_COUNTER,
+			  &vcpu->hv_clock.last_tsc);
+
+	ktime_get_ts(&ts);
+	vcpu->hv_clock.now_ns = ts.tv_nsec + (NSEC_PER_SEC *
(u64)ts.tv_sec); + vcpu->hv_clock.wc_sec = get_seconds();

Quoted 9 lines

Quoted 4 lines
I am even thinking we have to disable interrupt between these lines, otherwise guest wall clock may see backward time source when calculating the delta TSC since last vcpu->hv_clock.now_ns update.
That's true. While we do need to handle vcpu migration and descheduling, the code sequence you note needs to be as atomic as possible.

Yes VCPU migration is another issue. Since the physical platform TSC is totally different across migration, so we can't expose host TSC to guest.
For KVM, it should be simple to just use guest_TSC (=HOST_TSC + offset).

thanks, eddie

Gerd Hoffmann

2007-11-09 08:37 UTC #14 lore raw

Re: [PATCH 1/3] include files for kvmclock

In reply to Glauber de Oliveira Costa

Quoted 9 lines

include/asm-x86/kvm_para.h +9 -0

+/*
+ * Guest has page alignment and padding requirements. At the host, it will
+ * only lead to wasted space at the vcpu struct. For this reason, the struct
+ * is not anonymous
+ */
+union kvm_hv_clock {
+	struct kvm_hv_clock_s {
+		u64 tsc_mult;
+		u64 now_ns;

include/asm-x86/kvm_para.h line 37

Quoted 10 lines

include/asm-x86/kvm_para.h +10 -0

+		/* That's the wall clock, not the water closet */
+		u64 wc_sec;
+		u64 last_tsc;
+		/* At first, we could use the tsc value as a marker, but Jeremy
+		 * well noted that it will cause us locking problems in 32-bit
+		 * sys, so we have a special version field */
+		u32 version;
+	} fields;
+	char page_align[PAGE_SIZE];
+};

What is the point in using a whole page per vcpu? You probably don't want struct kvm_hv_clock_s cross a page border. Is that the only reason or are there other constrains too?

As the kvm clock looks quite simliar to what xen does, how about making the structs binary-compatible or simply reuse the xen version (struct vcpu_time_info in xen/interface/xen.h)?

cheers,
Gerd

Avi Kivity

2007-11-11 09:15 UTC #15 lore raw

Re: [PATCH 1/3] include files for kvmclock

Quoted 16 lines · Gerd Hoffmann

Quoted 16 lines

include/asm-x86/kvm_para.h +16 -0

+/*
+ * Guest has page alignment and padding requirements. At the host, it will
+ * only lead to wasted space at the vcpu struct. For this reason, the struct
+ * is not anonymous
+ */
+union kvm_hv_clock {
+	struct kvm_hv_clock_s {
+		u64 tsc_mult;
+		u64 now_ns;
+		/* That's the wall clock, not the water closet */
+		u64 wc_sec;
+		u64 last_tsc;
+		/* At first, we could use the tsc value as a marker, but Jeremy
+		 * well noted that it will cause us locking problems in 32-bit
+		 * sys, so we have a special version field */
+		u32 version;

Quoted 9 lines

include/asm-x86/kvm_para.h line 37
Quoted 4 lines
include/asm-x86/kvm_para.h +3 -0
+	} fields;
+	char page_align[PAGE_SIZE];
+};
What is the point in using a whole page per vcpu? You probably don't want struct kvm_hv_clock_s cross a page border. Is that the only reason or are there other constrains too?

We don't even have the cross-page-boundary constraint.

The real issue is that on i386 physical addresses are 64-bit while hypercall arguments are 32-bit. A page frame number is 32-bit and so poses no issues.

I see two workarounds for this:
- make the first hypercall argument a u64 rather than a long (using two registers on i386)
- use an msr for registering the clock address

The first is more general, while the second has the nice property of taking care of live migration automatically.

Quoted 4 lines

As the kvm clock looks quite simliar to what xen does, how about making the structs binary-compatible or simply reuse the xen version (struct vcpu_time_info in xen/interface/xen.h)?

If there are no technical issues, I have no objections.

Signature · 8 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/