Skip to contentSkip to conversation

kvm clock - with the patches this time

PATCH Applied Glauber de Oliveira Costa 3 participants 8 messages Jan 15, 2008 – Jan 16, 2008 5 messages not in the archive

Subsystems (4)
x86 KVM KVM x86 KVM paravirt
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): 1200409817411-git-send-email-gcosta-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

author
#2 lore raw

PATCH 1/3 put kvm_para.h include outside __KERNEL__

kvm_para.h potentially contains definitions that are to be used by kvm-userspace, so it should not be included inside the __KERNEL__ block. To protect its own data structures, kvm_para.h already includes its own __KERNEL__ block.

Signed-off-by Glauber de Oliveira Costa <[email protected]>
Acked-by Amit Shah <amit.shah-atKUWr5tajBWk0Htik3J/[email protected]>
---
 include/linux/kvm_para.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
include/linux/kvm_para.h +1 -1
index 6af91a5..5497aac 100644
@@ -14,12 +14,12 @@
 
 #define KVM_HC_VAPIC_POLL_IRQ            1
 
-#ifdef __KERNEL__
 /*
  * hypercalls use architecture specific
  */
 #include <asm/kvm_para.h>
 
+#ifdef __KERNEL__
 static inline int kvm_para_has_feature(unsigned int feature)
 {
 	if (kvm_arch_para_features() & (1UL << feature))
Signature · 7 lines
1.5.0.6


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

A message that is not in the archive (sent off-list or not yet received): 120040983179-git-send-email-gcosta-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

author
#4 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.

The area is binary compatible with xen, as we use the same shadow_info structure.

Signed-off-by Glauber de Oliveira Costa <[email protected]>
---
 arch/x86/kvm/x86.c         |   79 +++++++++++++++++++++++++++++++++++++++++++-
 include/asm-x86/kvm_host.h |    4 ++
 include/asm-x86/kvm_para.h |   37 ++++++++++++++++++++
 include/linux/kvm.h        |    1 +
 4 files changed, 120 insertions(+), 1 deletions(-)
arch/x86/kvm/x86.c +78 -1
index 8a90403..53b5692 100644
@@ -19,6 +19,7 @@
 #include "irq.h"
 #include "mmu.h"
 
+#include <linux/clocksource.h>
 #include <linux/kvm.h>
 #include <linux/fs.h>
 #include <linux/vmalloc.h>

@@ -412,7 +413,7 @@ static u32 msrs_to_save[] = {
 #ifdef CONFIG_X86_64
 	MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
 #endif
-	MSR_IA32_TIME_STAMP_COUNTER,
+	MSR_IA32_TIME_STAMP_COUNTER, MSR_KVM_PARAVIRT_CLOCK,
 };
 
 static unsigned num_msrs_to_save;

@@ -467,6 +468,60 @@ static int do_set_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
 	return kvm_set_msr(vcpu, index, *data);
 }
 
+#define WC_OFFSET offsetof(struct xen_shared_info, wc_version)
+
+static void kvm_write_guest_time(struct kvm_vcpu *v)
+{
+	struct timespec ts, wc_ts;
+	int wc_args[3]; /* version, wc_sec, wc_nsec */
+	unsigned long flags;
+	struct kvm_vcpu_arch *vcpu = &v->arch;
+	struct xen_shared_info *shared_kaddr;
+
+	if ((!vcpu->shared_page))
+		return;
+
+	/* Keep irq disabled to prevent changes to the clock */
+	local_irq_save(flags);
+	kvm_get_msr(v, MSR_IA32_TIME_STAMP_COUNTER,
+			  &vcpu->hv_clock.tsc_timestamp);
+	wc_ts = current_kernel_time();
+	ktime_get_ts(&ts);
+	local_irq_restore(flags);
+
+	/* With all the info we got, fill in the values */
+	wc_args[1] = wc_ts.tv_sec;
+	wc_args[2] = wc_ts.tv_nsec;
+
+	vcpu->hv_clock.system_time = ts.tv_nsec +
+				     (NSEC_PER_SEC * (u64)ts.tv_sec);
+	/*
+	 * The interface expects us to write an even number signaling that the
+	 * update is finished. Since the guest won't see the intermediate states,
+	 * we just write "2" at the end
+	 */
+	wc_args[0] = 2;
+	vcpu->hv_clock.version = 2;
+
+	preempt_disable();
+
+	shared_kaddr = kmap_atomic(vcpu->shared_page, KM_USER0);
+
+	/*
+	 * We could write everything at once, but it can break future
+	 * implementations. We're just a tiny and lonely clock, so let's
+	 * write only what matters here
+	 */
+	memcpy(&shared_kaddr->wc_version, wc_args, sizeof(wc_args));
+	memcpy(&shared_kaddr->vcpu_info[v->vcpu_id].time, &vcpu->hv_clock,
+		sizeof(vcpu->hv_clock));
+
+	kunmap_atomic(shared_kaddr, KM_USER0);
+	preempt_enable();
+
+	mark_page_dirty(v->kvm, vcpu->shared_info >> PAGE_SHIFT);
+}
+
 
 int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 {

@@ -494,6 +549,20 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 	case MSR_IA32_MISC_ENABLE:
 		vcpu->arch.ia32_misc_enable_msr = data;
 		break;
+	case MSR_KVM_PARAVIRT_CLOCK: {
+		vcpu->arch.shared_info = data;
+
+		vcpu->arch.hv_clock.tsc_to_system_mul =
+					clocksource_khz2mult(tsc_khz, 22);
+		vcpu->arch.hv_clock.tsc_shift = 22;
+
+		down_write(&current->mm->mmap_sem);
+		vcpu->arch.shared_page = gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT);
+		up_write(&current->mm->mmap_sem);
+		if (is_error_page(vcpu->arch.shared_page))
+			vcpu->arch.shared_page = NULL;
+		break;
+	}
 	default:
 		pr_unimpl(vcpu, "unhandled wrmsr: 0x%x data %llx\n", msr, data);
 		return 1;

@@ -553,6 +622,10 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 		data = vcpu->arch.shadow_efer;
 		break;
 #endif
+	case MSR_KVM_PARAVIRT_CLOCK:
+		data = vcpu->arch.shared_info;
+		break;
+
 	default:
 		pr_unimpl(vcpu, "unhandled rdmsr: 0x%x\n", msr);
 		return 1;

@@ -680,6 +753,7 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_USER_MEMORY:
 	case KVM_CAP_SET_TSS_ADDR:
 	case KVM_CAP_EXT_CPUID:
+	case KVM_CAP_CLOCKSOURCE:
 		r = 1;
 		break;
 	case KVM_CAP_VAPIC:

@@ -737,6 +811,7 @@ out:
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 {
 	kvm_x86_ops->vcpu_load(vcpu, cpu);
+	kvm_write_guest_time(vcpu);
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)

@@ -3190,6 +3265,8 @@ static void kvm_free_vcpus(struct kvm *kvm)
 			kvm_unload_vcpu_mmu(kvm->vcpus[i]);
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		if (kvm->vcpus[i]) {
+			if (kvm->vcpus[i]->arch.shared_page)
+				kvm_release_page_dirty(kvm->vcpus[i]->arch.shared_page);
 			kvm_arch_vcpu_free(kvm->vcpus[i]);
 			kvm->vcpus[i] = NULL;
 		}
include/asm-x86/kvm_host.h +4 -0
index d6db0de..9a66b90 100644
@@ -261,6 +261,10 @@ struct kvm_vcpu_arch {
 	/* emulate context */
 
 	struct x86_emulate_ctxt emulate_ctxt;
+
+	struct xen_vcpu_time_info hv_clock;
+	gpa_t shared_info;
+	struct page *shared_page;
 };
 
 struct kvm_mem_alias {
include/asm-x86/kvm_para.h +37 -0
index c6f3fd8..145107d 100644
@@ -10,10 +10,47 @@
  * paravirtualization, the appropriate feature bit should be checked.
  */
 #define KVM_CPUID_FEATURES	0x40000001
+#define KVM_FEATURE_CLOCKSOURCE 0
+
+#define MSR_KVM_PARAVIRT_CLOCK  0x11
 
 #ifdef __KERNEL__
 #include <asm/processor.h>
 
+/* xen binary-compatible interfaces. See xen headers for details */
+struct xen_vcpu_time_info {
+	uint32_t version;
+	uint32_t pad0;
+	uint64_t tsc_timestamp;
+	uint64_t system_time;
+	uint32_t tsc_to_system_mul;
+	int8_t   tsc_shift;
+	int8_t   pad1[3];
+};
+
+struct xen_vcpu_info {
+	uint8_t  pad[32];
+	struct xen_vcpu_time_info time;
+};
+
+#define XEN_MAX_VIRT_CPUS	32
+
+struct xen_shared_info {
+	struct xen_vcpu_info vcpu_info[XEN_MAX_VIRT_CPUS];
+
+	unsigned long evt[2];
+
+	uint32_t wc_version;      /* Version counter: see vcpu_time_info_t. */
+	uint32_t wc_sec;          /* Secs  00:00:00 UTC, Jan 1, 1970.  */
+	uint32_t wc_nsec;         /* Nsecs 00:00:00 UTC, Jan 1, 1970.  */
+
+	unsigned long pad[12];
+};
+
+
+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/linux/kvm.h +1 -0
index 4de4fd2..78ce53f 100644
@@ -232,6 +232,7 @@ struct kvm_vapic_addr {
 #define KVM_CAP_SET_TSS_ADDR 4
 #define KVM_CAP_EXT_CPUID 5
 #define KVM_CAP_VAPIC 6
+#define KVM_CAP_CLOCKSOURCE 7
 
 /*
  * ioctls for VM fds
Signature · 7 lines
1.5.0.6


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

A message that is not in the archive (sent off-list or not yet received): 12004098373625-git-send-email-gcosta-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

author
#6 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            |   10 +++
 arch/x86/kernel/Makefile_32 |    1 +
 arch/x86/kernel/kvmclock.c  |  161 +++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/setup_32.c  |    5 ++
 4 files changed, 177 insertions(+), 0 deletions(-)
 create mode 100644 arch/x86/kernel/kvmclock.c
arch/x86/Kconfig +10 -0
index ab2df55..968315e 100644
@@ -350,6 +350,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 a7bc93c..f6332b6 100644
@@ -44,6 +44,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 +161 -0

161 lines not on this page · raw message

arch/x86/kernel/setup_32.c +5 -0
index 9c24b45..89c0eb2 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 · 7 lines
1.5.0.6


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
#7 lore raw

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

In reply to a message not in the archive
Quoted 34 lines · Glauber de Oliveira Costa
arch/x86/kvm/x86.c +33 -0
+static void kvm_write_guest_time(struct kvm_vcpu *v)
+{
+	struct timespec ts, wc_ts;
+	int wc_args[3]; /* version, wc_sec, wc_nsec */
+	unsigned long flags;
+	struct kvm_vcpu_arch *vcpu = &v->arch;
+	struct xen_shared_info *shared_kaddr;
+
+	if ((!vcpu->shared_page))
+		return;
+
+	/* Keep irq disabled to prevent changes to the clock */
+	local_irq_save(flags);
+	kvm_get_msr(v, MSR_IA32_TIME_STAMP_COUNTER,
+			  &vcpu->hv_clock.tsc_timestamp);
+	wc_ts = current_kernel_time();
+	ktime_get_ts(&ts);
+	local_irq_restore(flags);
+
+	/* With all the info we got, fill in the values */
+	wc_args[1] = wc_ts.tv_sec;
+	wc_args[2] = wc_ts.tv_nsec;
+
+	vcpu->hv_clock.system_time = ts.tv_nsec +
+				     (NSEC_PER_SEC * (u64)ts.tv_sec);
+	/*
+	 * The interface expects us to write an even number signaling that the
+	 * update is finished. Since the guest won't see the intermediate states,
+	 * we just write "2" at the end
+	 */
+	wc_args[0] = 2;
+	vcpu->hv_clock.version = 2;
+

arch/x86/kvm/x86.c static int do_set_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data) · line 515

Quoted 10 lines
arch/x86/kvm/x86.c +10 -0
+	preempt_disable();
+
+	shared_kaddr = kmap_atomic(vcpu->shared_page, KM_USER0);
+
+	/*
+	 * We could write everything at once, but it can break future
+	 * implementations. We're just a tiny and lonely clock, so let's
+	 * write only what matters here
+	 */
+	memcpy(&shared_kaddr->wc_version, wc_args, sizeof(wc_args));

We want to avoid updating wall clock all the time. As far as I understand, wall clock is just a base which doesn't change. To get the real wall clock, you read the shared_info wall clock and add the current system time. This means that you avoid writing to a shared global (which is expensive in cache lines). The shared_info wall clock is only updated if the host clocked is moved (other than in the way you expect it to).

Also, when you write to the shared clock, you must respect the protocol since it can be read concurrently:

- increment the version
- smp_wmb()
- copy the goodies
- smp_wmb()
- increment the version again

[I think this is the protocol, but better read the sources to double-check]

Quoted 6 lines
(quoted diff) +0 -0
 		}
include/asm-x86/kvm_host.h +0 -0
index d6db0de..9a66b90 100644

include/asm-x86/kvm_host.h struct kvm_vcpu_arch { · line 269

Quoted 9 lines
include/asm-x86/kvm_host.h +4 -0
@@ -261,6 +261,10 @@ struct kvm_vcpu_arch {
 	/* emulate context */
 
 	struct x86_emulate_ctxt emulate_ctxt;
+
+	struct xen_vcpu_time_info hv_clock;
+	gpa_t shared_info;
+	struct page *shared_page;
 };

shared_{info,page} is too generic a name for just a clock.

Quoted 7 lines
include/asm-x86/kvm_host.h +5 -0
+/* xen binary-compatible interfaces. See xen headers for details */
+struct xen_vcpu_time_info {
+	uint32_t version;
+	uint32_t pad0;
+	uint64_t tsc_timestamp;

include/asm-x86/kvm_host.h struct kvm_vcpu_arch {

Quoted 10 lines
include/asm-x86/kvm_host.h +10 -0
+	uint64_t system_time;
+	uint32_t tsc_to_system_mul;
+	int8_t   tsc_shift;
+	int8_t   pad1[3];
+};
+
+struct xen_vcpu_info {
+	uint8_t  pad[32];
+	struct xen_vcpu_time_info time;
+};

Please drop xen_vcpu_info...

Quoted 5 lines
include/asm-x86/kvm_host.h +4 -0
+
+#define XEN_MAX_VIRT_CPUS	32
+
+struct xen_shared_info {

include/asm-x86/kvm_host.h

Quoted 10 lines
include/asm-x86/kvm_host.h +10 -0
+	struct xen_vcpu_info vcpu_info[XEN_MAX_VIRT_CPUS];
+
+	unsigned long evt[2];
+
+	uint32_t wc_version;      /* Version counter: see vcpu_time_info_t. */
+	uint32_t wc_sec;          /* Secs  00:00:00 UTC, Jan 1, 1970.  */
+	uint32_t wc_nsec;         /* Nsecs 00:00:00 UTC, Jan 1, 1970.  */
+
+	unsigned long pad[12];
+};

... and everything non-time-related in here. Yes, in means we need two msrs (for wall clock and system time), but it also means we don't impose any layout upon the guest, and do not (for example) restrict the number of vcpus. We could easily put the vcpu clock in a per_cpu() area.

Signature · 7 lines
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

A message that is not in the archive (sent off-list or not yet received): 478DC2CF.4040803-atKUWr5tajBWk0Htik3J/[email protected]

#9 lore raw

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

Hi,

Quoted 2 lines

We want to avoid updating wall clock all the time. As far as I understand, wall clock is just a base which doesn't change.

Yep, it is. Got that wrong first in xenner, with the result that guest time ran at double speed ;)

Quoted 11 lines
Quoted 10 lines
include/asm-x86/kvm_para.h +10 -0
+/* xen binary-compatible interfaces. See xen headers for details */
+struct xen_vcpu_time_info {
+	uint32_t version;
+	uint32_t pad0;
+	uint64_t tsc_timestamp;
+	uint64_t system_time;
+	uint32_t tsc_to_system_mul;
+	int8_t   tsc_shift;
+	int8_t   pad1[3];
+};
Quoted 7 lines

include/asm-x86/kvm_para.h line 34

Quoted 5 lines
include/asm-x86/kvm_para.h +4 -0
+struct xen_vcpu_info {
+	uint8_t  pad[32];
+	struct xen_vcpu_time_info time;
+};

Please drop xen_vcpu_info...

Oh, yeah. No point in assembling the whole xen shared info page. Just xen_vcpu_time_info is enougth, it will work just fine for xenner.

cheers,
Gerd

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

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

#11 lore raw

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

Quoted 33 lines · Gerd Hoffmann

Hi,

Quoted 3 lines

We want to avoid updating wall clock all the time. As far as I understand, wall clock is just a base which doesn't change.

Yep, it is. Got that wrong first in xenner, with the result that guest time ran at double speed ;)

Quoted 13 lines
Quoted 11 lines
include/asm-x86/kvm_para.h +10 -0
+/* xen binary-compatible interfaces. See xen headers for details */
+struct xen_vcpu_time_info {
+	uint32_t version;
+	uint32_t pad0;
+	uint64_t tsc_timestamp;
+	uint64_t system_time;
+	uint32_t tsc_to_system_mul;
+	int8_t   tsc_shift;
+	int8_t   pad1[3];
+};
Quoted 8 lines

include/asm-x86/kvm_para.h line 34

Quoted 6 lines
include/asm-x86/kvm_para.h +4 -0
+struct xen_vcpu_info {
+	uint8_t  pad[32];
+	struct xen_vcpu_time_info time;
+};

Please drop xen_vcpu_info...

Quoted 4 lines

Oh, yeah. No point in assembling the whole xen shared info page. Just xen_vcpu_time_info is enougth, it will work just fine for xenner.

We should also not use the xen_ namespace, that can only cause conflicts.

Signature · 7 lines
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
Applied
#12 lore raw

Re: [PATCH 1/3] put kvm_para.h include outside __KERNEL__

In reply to a message not in the archive
Quoted 5 lines · Glauber de Oliveira Costa

kvm_para.h potentially contains definitions that are to be used by kvm-userspace, so it should not be included inside the __KERNEL__ block. To protect its own data structures, kvm_para.h already includes its own __KERNEL__ block.

Applied this one, thanks.

Signature · 7 lines
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/