Skip to contentSkip to conversation

KVM Paravirt clock - take2

PATCH Glauber de Oliveira Costa 2 participants 2 messages Oct 23, 2007 – Oct 24, 2007 1 message not in the archive

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

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

Hi Folks,

Here's an updated version of the kvm paravirt clock infrastructure. Some of the issues you all raised were addressed, some were not. This work basically focus on general issues / clocksource, so don't expect to see much new things here in the clock events area.

For this, I intend to start implementing avi's idea of using a dedicated
irq chip other than the apic, as nobody seem to have voiced any strong
opposition.

As for the HPET vs clockevents, my solution is as follows: I will implement the clockevents, as it is already on its way, and whoever wants, can do an hpet implementation ;-)

Thinking about this, this patch now makes clocksource and clockevents really decoupled from a functionality pov. You can now turn one of them off by a kernel parameter.

There were some bugs in my previous clocksource being masked by a userspace call to settimeofday() in my distribution image initialization, so the code here fixes it.

Also, With all methods I've tried to keep the wallclock of guest and host n'sync, I experienced a little difference in all situations. The one you'll see in this patch is not the most beautiful thing in the world, but worked quite well, even in situations in which the nsec remainder is quite small.

I'll keep up a git tree for it at
git://git.kernel.org/pub/scm/linux/kernel/git/glommer/kvmclock.git, and for this version, there's also a one big patch at
http://glommer.net/kvm-clock-one-big-patch-v2.patch

It's also below, for your appreciation.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: Using GnuPG with Remi - http://enigmail.mozdev.org

iD8DBQFHHn9TjYI8LaFUWXMRAl2DAKDEDCR+SkGe7dUXWEFQB0ACxT26NwCgicos +sxVhnM2KOBYFqnVQzuJ+WM=
=sOTl
-----END PGP SIGNATURE-----

arch/i386/Kconfig +9 -0
index f6e44fc..b60bea8 100644
@@ -248,6 +248,15 @@ config VMI
 	  (it could be used by other hypervisors in theory too, but is not
 	  at the moment), by linking the kernel to a GPL-ed ROM module
 	  provided by the hypervisor.
+config KVM_CLOCK
+	bool "KVM paravirtualized clock"
+	depends on PARAVIRT && GENERIC_CLOCKEVENTS
+	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.
 
 config ACPI_SRAT
 	bool
arch/x86/kernel/Makefile_32 +1 -0
index ccea590..1718d86 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 +274 -0

274 lines not on this page · raw message

arch/x86/kernel/paravirt_32.c +9 -1
index 6a80d67..fa5d894 100644
@@ -20,6 +20,7 @@
 #include <linux/efi.h>
 #include <linux/bcd.h>
 #include <linux/highmem.h>
+#include <linux/kvm_para.h>
 
 #include <asm/bug.h>
 #include <asm/paravirt.h>
@@ -50,6 +51,13 @@ char *memory_setup(void)
 	return pv_init_ops.memory_setup();
 }
 
+static void paravirt_init_special_features(void)
+{
+#ifdef CONFIG_KVM_CLOCK
+	kvmclock_init();
+#endif
+}
+
 /* Simple instruction patching code. */
 #define DEF_NATIVE(ops, name, code)					\
 	extern const char start_##ops##_##name[], end_##ops##_##name[];	\
@@ -334,7 +342,7 @@ struct pv_info pv_info = {
 struct pv_init_ops pv_init_ops = {
 	.patch = native_patch,
 	.banner = default_banner,
-	.arch_setup = paravirt_nop,
+	.arch_setup = paravirt_init_special_features,
 	.memory_setup = machine_specific_memory_setup,
 };
 
arch/x86/kernel/setup_32.c +1 -2
index cc0e914..f833395 100644
@@ -546,6 +546,7 @@ void __init setup_arch(char **cmdline_p)
 	unsigned long max_low_pfn;
 
 	memcpy(&boot_cpu_data, &new_cpu_data, sizeof(new_cpu_data));
+	parse_early_param();
 	pre_setup_arch_hook();
 	early_cpu_init();
 
@@ -605,8 +606,6 @@ void __init setup_arch(char **cmdline_p)
 	bss_resource.start = virt_to_phys(&__bss_start);
 	bss_resource.end = virt_to_phys(&__bss_stop)-1;
 
-	parse_early_param();
-
 	if (user_defined_memmap) {
 		printk(KERN_INFO "user-defined physical RAM map:\n");
 		print_memory_map("user");
drivers/kvm/irq.c +11 -1
index 22bfeee..fe30099 100644
@@ -33,6 +33,8 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
 {
 	struct kvm_pic *s;
 
+	if (v->timer_vector != -1)
+		return 1;
 	if (kvm_apic_has_interrupt(v) == -1) {	/* LAPIC */
 		if (kvm_apic_accept_pic_intr(v)) {
 			s = pic_irqchip(v->kvm);	/* PIC */
@@ -44,6 +46,12 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
 }
 EXPORT_SYMBOL_GPL(kvm_cpu_has_interrupt);
 
+static int kvm_get_pvclock_interrupt(struct kvm_vcpu *v)
+{
+	int ret = v->timer_vector;
+	v->timer_vector = -1;
+	return	ret;
+}
 /*
  * Read pending interrupt vector and intack.
  */
@@ -52,7 +60,9 @@ int kvm_cpu_get_interrupt(struct kvm_vcpu *v)
 	struct kvm_pic *s;
 	int vector;
 
-	vector = kvm_get_apic_interrupt(v);	/* APIC */
+	vector = kvm_get_pvclock_interrupt(v);
+	if (vector == -1)
+		vector = kvm_get_apic_interrupt(v);	/* APIC */
 	if (vector == -1) {
 		if (kvm_apic_accept_pic_intr(v)) {
 			s = pic_irqchip(v->kvm);
drivers/kvm/kvm.h +4 -0
index 08b5b21..3d2158c 100644
@@ -362,6 +362,10 @@ struct kvm {
 	struct kvm_mem_alias aliases[KVM_ALIAS_SLOTS];
 	int nmemslots;
 	struct kvm_memory_slot memslots[KVM_MEMORY_SLOTS];
+
+	struct page *clock_page;
+	u64 clock_gpa;
+	int time_needs_update;
 	/*
 	 * Hash table of struct kvm_mmu_page.
 	 */
drivers/kvm/kvm_main.c +85 -0

140 lines not on this page · raw message

drivers/kvm/x86.h +3 -0
index 01452b5..27c65bc 100644
@@ -83,6 +83,9 @@ struct kvm_vcpu {
 	/* emulate context */
 
 	struct x86_emulate_ctxt emulate_ctxt;
+	struct hrtimer oneshooter;
+	int timer_vector;
+
 };
 
 static inline int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t gva,
include/asm-x86/kvm_para.h +23 -0
index c6f3fd8..8dec29f 100644
@@ -10,9 +10,12 @@
  * paravirtualization, the appropriate feature bit should be checked.
  */
 #define KVM_CPUID_FEATURES	0x40000001
+#define KVM_FEATURE_CLOCKEVENTS 0
+#define KVM_FEATURE_CLOCKSOURCE 1
 
 #ifdef __KERNEL__
 #include <asm/processor.h>
+extern void kvmclock_init(void);
 
 /* This instruction is vmcall.  On non-VT architectures, it will generate a
  * trap that we will then rewrite to the appropriate instruction.
@@ -29,6 +32,26 @@
  * noted by the particular hypercall.
  */
 
+#define KVM_HCALL_REGISTER_CLOCK 0
+#define KVM_HCALL_SET_ALARM	1
+#define KVM_HCALL_STOP_ONESHOT 2
+
+struct kvm_hv_clock {
+	u64 tsc_mult;
+	u64 now_ns;
+	/* That's the wall clock, not the water closet */
+	u64 wc_sec;
+	u64 wc_nsec;
+	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;
+	u32 stable_tsc; /* use raw tsc for clock_read */
+};
+
 static inline long kvm_hypercall0(unsigned int nr)
 {
 	long ret;
include/linux/kvm_para.h +4 -0
index e4db25f..4c2afd0 100644
@@ -9,6 +9,10 @@
  * - kvm_para_available
  */
 
+#define KVM_CPUID_FEATURES	0x40000001
+#define KVM_FEATURE_CLOCKEVENTS 0
+#define KVM_FEATURE_CLOCKSOURCE 1
+
 /* Return values for hypercalls */
 #define KVM_ENOSYS		1000
 

-------------------------------------------------------------------------
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/

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw
Quoted 11 lines · Glauber de Oliveira Costa

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

Hi Folks,

Here's an updated version of the kvm paravirt clock infrastructure. Some of the issues you all raised were addressed, some were not. This work basically focus on general issues / clocksource, so don't expect to see much new things here in the clock events area.

Me like.

Please separate the guest and host parts; this allows backporting the guest part to older kernels.

arch/x86/kernel/kvmclock.c line 181

Quoted 10 lines
arch/x86/kernel/kvmclock.c new file +9 -0
+
+/* This is our clockevents structure. We only support one shot operation */
+static struct clock_event_device kvm_clockevent = {
+	.name		= "kvm-timer",
+	.features	= CLOCK_EVT_FEAT_ONESHOT,
+	.shift		= 0,
+	.mult		= 1,
+	.max_delta_ns	= 0xffffffff,
+	.min_delta_ns	= 1000000,

Why this particular number? Doesn't it depend on the host's clocksource?

Quoted 5 lines
drivers/kvm/kvm_main.c +1 -0
@@ -1564,6 +1566,36 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_emulate_halt);
 
+static struct kvm_hv_clock hv_clock;

drivers/kvm/kvm_main.c int kvm_emulate_halt(struct kvm_vcpu *vcpu) · line 1579

Quoted 10 lines
drivers/kvm/kvm_main.c +9 -0
+
+static void kvm_write_guest_time(struct kvm_vcpu *vcpu)
+{
+	struct kvm *kvm = vcpu->kvm;
+	struct timespec ts;
+	void *clock_addr;
+
+	if ((!kvm->clock_page) || !(kvm->clock_gpa))
+		return;
 

Why two checks?

drivers/kvm/kvm_main.c int kvm_emulate_halt(struct kvm_vcpu *vcpu)

Quoted 3 lines
drivers/kvm/kvm_main.c +2 -0
+
+	clock_addr = kmap(kvm->clock_page);
 

kmap_atomic() is much faster and more akpm-resistant.

drivers/kvm/kvm_main.c int kvm_emulate_halt(struct kvm_vcpu *vcpu)

Quoted 11 lines
drivers/kvm/kvm_main.c +10 -0
+
+	/* Updates version to the next odd number, indicating we're writing */
+	hv_clock.version++;
+	/* Updating the tsc count is the first thing we do */
+	kvm_get_msr(vcpu, MSR_IA32_TIME_STAMP_COUNTER, &hv_clock.last_tsc);
+	ktime_get_ts(&ts);
+	hv_clock.now_ns = ts.tv_nsec + NSEC_PER_SEC * ts.tv_sec;
+	hv_clock.wc_sec = get_seconds();
+	hv_clock.version++;
+	WARN_ON(hv_clock.version & 1);
 

smp_wmb()s are missing here.

The guest can trigger the WARN_ON() by being naughty with .version.

Quoted 12 lines
drivers/kvm/kvm_main.c +8 -0
+
+	memcpy(clock_addr, &hv_clock, sizeof(hv_clock));
+	mark_page_dirty(kvm, kvm->clock_gpa >> PAGE_SHIFT);
+	kunmap(kvm->clock_page);
+
+	kvm->time_needs_update = 0;
+}
+

int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
{

drivers/kvm/kvm_main.c +0 -0
 	unsigned long nr, a0, a1, a2, a3, ret;

drivers/kvm/kvm_main.c int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) · line 1624

Quoted 9 lines
drivers/kvm/kvm_main.c +4 -0
@@ -1584,7 +1616,37 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 		a3 &= 0xFFFFFFFF;
 	}
 
+	ret = 0;
 	switch (nr) {
+	case  KVM_HCALL_REGISTER_CLOCK: {
+		if (!irqchip_in_kernel(vcpu->kvm)) {
+			ret = -1;
 

Return a proper KVM error.

drivers/kvm/kvm_main.c int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)

Quoted 5 lines
drivers/kvm/kvm_main.c +4 -0
+			break;
+		}
+
+		vcpu->kvm->clock_gpa = a0;
 

i386 gpas are 64-bit. Better define it as a gfn (that absolves you of checking alignment too).

drivers/kvm/kvm_main.c int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)

Quoted 3 lines
drivers/kvm/kvm_main.c +2 -0
+		vcpu->kvm->clock_page = gfn_to_page(vcpu->kvm, a0 >> PAGE_SHIFT);
+
 

You're touching shared state here. Can probably cause problems if the guest turns nasty.

Quoted 19 lines
drivers/kvm/x86.h +0 -0
 static inline int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t gva,
include/asm-x86/kvm_para.h +3 -0
index c6f3fd8..8dec29f 100644
@@ -10,9 +10,12 @@
  * paravirtualization, the appropriate feature bit should be checked.
  */
 #define KVM_CPUID_FEATURES	0x40000001
+#define KVM_FEATURE_CLOCKEVENTS 0
+#define KVM_FEATURE_CLOCKSOURCE 1
 
 #ifdef __KERNEL__
 #include <asm/processor.h>
+extern void kvmclock_init(void);
 
 /* This instruction is vmcall.  On non-VT architectures, it will generate a
  * trap that we will then rewrite to the appropriate instruction.

include/asm-x86/kvm_para.h line 36

Quoted 5 lines
include/asm-x86/kvm_para.h +1 -0
@@ -29,6 +32,26 @@
  * noted by the particular hypercall.
  */
 
+#define KVM_HCALL_REGISTER_CLOCK 0
 

Let's start hypercall numbers at 1 to trap some cases on uninitialized data coming that way.

Quoted 6 lines
include/asm-x86/kvm_para.h +0 -0
 	long ret;
include/linux/kvm_para.h +0 -0
index e4db25f..4c2afd0 100644

include/linux/kvm_para.h line 18

Quoted 10 lines
include/linux/kvm_para.h +4 -0
@@ -9,6 +9,10 @@
  * - kvm_para_available
  */
 
+#define KVM_CPUID_FEATURES	0x40000001
+#define KVM_FEATURE_CLOCKEVENTS 0
+#define KVM_FEATURE_CLOCKSOURCE 1
+
 /* Return values for hypercalls */
 #define KVM_ENOSYS		1000
 

Need to expose the capability to userspace as well via KVM_CHECK_EXTENSION so userspace will know to expose it to the guest.

Finally, how about making this per-vcpu? That solves problems with tscs running differently on different cpus, you don't need the wmb()s on the host side, is faster for the guest due to less cacheline bouncing, I could go on forever.

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/