Add support for RDTSCP in VMX

PATCH Superseded Sheng Yang 2 participants 15 messages Dec 16, 2009 – Dec 17, 2009

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Sheng Yang

author

2009-12-16 05:48 UTC #0 lore raw

cover Add support for RDTSCP in VMX

After discussion with Avi, we decided to take this chance to extend the shared MSR framework, discard the limitation of "All CPU should have same value for the MSRs". But the limitation of "The MSRs' value shouldn't be modified after they were read" still apply.

I have tested this patchset using 2.6.32. It can boot well with "rdtscp" feature(-cpu core2duo,+rdtscp) after patching, and a simple "rdtscp" userspace program can running in the guest(it's too simple that can only prove the instruction won't cause #UD, can't guarantee the result, so I think it's not necessary to get it in the unit test). In the past, this would result in guest kernel crash on vsyscall initialization (due to access MSR_TSC_AUX result in #GP fault).

--
regards
Yang, Sheng

 arch/x86/include/asm/kvm_host.h  |    1 +
 arch/x86/include/asm/msr-index.h |    1 +
 arch/x86/include/asm/msr.h       |    4 +-
 arch/x86/include/asm/vmx.h       |    1 +
 arch/x86/kvm/svm.c               |    6 +++
 arch/x86/kvm/vmx.c               |   33 ++++++++++++++++---
 arch/x86/kvm/x86.c               |   67 +++++++++++++++++++++++++-------------
 7 files changed, 83 insertions(+), 30 deletions(-)

Sheng Yang

author

2009-12-16 05:48 UTC #1 lore raw

PATCH 1/4 KVM: VMX: Remove redundant variable

It's no longer necessary.

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/kvm/vmx.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

arch/x86/kvm/vmx.c +0 -2

index 9a0a2cf..5c464ed 100644
@@ -2383,14 +2383,12 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
 	for (i = 0; i < NR_VMX_MSR; ++i) {
 		u32 index = vmx_msr_index[i];
 		u32 data_low, data_high;
-		u64 data;
 		int j = vmx->nmsrs;
 
 		if (rdmsr_safe(index, &data_low, &data_high) < 0)
 			continue;
 		if (wrmsr_safe(index, data_low, data_high) < 0)
 			continue;
-		data = data_low | ((u64)data_high << 32);
 		vmx->guest_msrs[j].index = i;
 		vmx->guest_msrs[j].data = 0;
 		vmx->guest_msrs[j].mask = -1ull;

Signature · 1 line

1.5.4.5

Sheng Yang

author

2009-12-16 05:48 UTC #2 lore raw

PATCH 2/4 KVM: Extended shared_msr_global to per CPU

In reply to Sheng Yang

shared_msr_global saved host value of relevant MSRs, but it have an assumption that all MSRs it tracked shared the value across the different CPUs. It's not true with some MSRs, e.g. MSR_TSC_AUX.

Extend it to per CPU to provide the support of MSR_TSC_AUX, and more alike MSRs.

Notice now the shared_msr_global still have one assumption: it can only deal with the MSRs that won't change in host after KVM module loaded.

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/kvm/x86.c |   64 ++++++++++++++++++++++++++++++++++------------------
 1 files changed, 42 insertions(+), 22 deletions(-)

arch/x86/kvm/x86.c +42 ~~-22~~

index dd15d7a..4262450 100644
@@ -92,16 +92,16 @@ module_param_named(ignore_msrs, ignore_msrs, bool, S_IRUGO | S_IWUSR);
 
 struct kvm_shared_msrs_global {
 	int nr;
-	struct kvm_shared_msr {
-		u32 msr;
-		u64 value;
-	} msrs[KVM_NR_SHARED_MSRS];
+	u32 msrs[KVM_NR_SHARED_MSRS];
 };
 
 struct kvm_shared_msrs {
 	struct user_return_notifier urn;
 	bool registered;
-	u64 current_value[KVM_NR_SHARED_MSRS];
+	struct kvm_shared_msr_values {
+		u64 host;
+		u64 curr;
+	} values[KVM_NR_SHARED_MSRS];
 };
 
 static struct kvm_shared_msrs_global __read_mostly shared_msrs_global;

@@ -146,53 +146,73 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 static void kvm_on_user_return(struct user_return_notifier *urn)
 {
 	unsigned slot;
-	struct kvm_shared_msr *global;
 	struct kvm_shared_msrs *locals
 		= container_of(urn, struct kvm_shared_msrs, urn);
+	struct kvm_shared_msr_values *values;
 
 	for (slot = 0; slot < shared_msrs_global.nr; ++slot) {
-		global = &shared_msrs_global.msrs[slot];
-		if (global->value != locals->current_value[slot]) {
-			wrmsrl(global->msr, global->value);
-			locals->current_value[slot] = global->value;
+		values = &locals->values[slot];
+		if (values->host != values->curr) {
+			wrmsrl(shared_msrs_global.msrs[slot], values->host);
+			values->curr = values->host;
 		}
 	}
 	locals->registered = false;
 	user_return_notifier_unregister(urn);
 }
 
-void kvm_define_shared_msr(unsigned slot, u32 msr)
+static void shared_msr_update(void *data)
 {
-	int cpu;
+	struct kvm_shared_msrs *smsr;
+	u32 msr = *(u32 *)data;
+	int slot;
 	u64 value;
 
+	smsr = &get_cpu_var(shared_msrs);
+	/* only read, and nobody should modify it at this time,
+	 * so don't need lock */
+	for (slot = 0; slot < shared_msrs_global.nr; slot++)
+		if (shared_msrs_global.msrs[slot] == msr)
+			break;
+	if (slot >= shared_msrs_global.nr) {
+		printk(KERN_ERR "kvm: can't find the defined MSR!");
+		return;
+	}
+	rdmsrl_safe(msr, &value);
+	smsr->values[slot].host = value;
+	smsr->values[slot].curr = value;
+	put_cpu_var(shared_msrs);
+}
+
+void kvm_define_shared_msr(unsigned slot, u32 msr)
+{
 	if (slot >= shared_msrs_global.nr)
 		shared_msrs_global.nr = slot + 1;
-	shared_msrs_global.msrs[slot].msr = msr;
-	rdmsrl_safe(msr, &value);
-	shared_msrs_global.msrs[slot].value = value;
-	for_each_online_cpu(cpu)
-		per_cpu(shared_msrs, cpu).current_value[slot] = value;
+	shared_msrs_global.msrs[slot] = msr;
+	/* we need ensured the shared_msr_global have been updated */
+	smp_wmb();
+	smp_call_function(shared_msr_update, &msr, 1);
+	shared_msr_update(&msr);
 }
 EXPORT_SYMBOL_GPL(kvm_define_shared_msr);
 
 static void kvm_shared_msr_cpu_online(void)
 {
 	unsigned i;
-	struct kvm_shared_msrs *locals = &__get_cpu_var(shared_msrs);
+	struct kvm_shared_msrs *smsr = &__get_cpu_var(shared_msrs);
 
 	for (i = 0; i < shared_msrs_global.nr; ++i)
-		locals->current_value[i] = shared_msrs_global.msrs[i].value;
+		smsr->values[i].curr = smsr->values[i].host;
 }
 
 void kvm_set_shared_msr(unsigned slot, u64 value, u64 mask)
 {
 	struct kvm_shared_msrs *smsr = &__get_cpu_var(shared_msrs);
 
-	if (((value ^ smsr->current_value[slot]) & mask) == 0)
+	if (((value ^ smsr->values[slot].curr) & mask) == 0)
 		return;
-	smsr->current_value[slot] = value;
-	wrmsrl(shared_msrs_global.msrs[slot].msr, value);
+	smsr->values[slot].curr = value;
+	wrmsrl(shared_msrs_global.msrs[slot], value);
 	if (!smsr->registered) {
 		smsr->urn.on_user_return = kvm_on_user_return;
 		user_return_notifier_register(&smsr->urn);

Signature · 1 line

1.5.4.5

Avi Kivity

2009-12-16 09:21 UTC #3 lore raw

Re: [PATCH 2/4] KVM: Extended shared_msr_global to per CPU

Quoted 13 lines · Sheng Yang

shared_msr_global saved host value of relevant MSRs, but it have an assumption that all MSRs it tracked shared the value across the different CPUs. It's not true with some MSRs, e.g. MSR_TSC_AUX.
Extend it to per CPU to provide the support of MSR_TSC_AUX, and more alike MSRs.
Notice now the shared_msr_global still have one assumption: it can only deal with the MSRs that won't change in host after KVM module loaded.

arch/x86/kvm/x86.c struct kvm_stats_debugfs_item debugfs_entries[] = {

Quoted 10 lines

arch/x86/kvm/x86.c +5 -2

-void kvm_define_shared_msr(unsigned slot, u32 msr)
+static void shared_msr_update(void *data)
  {
-	int cpu;
+	struct kvm_shared_msrs *smsr;
+	u32 msr = *(u32 *)data;
+	int slot;
  	u64 value;
 
+	smsr =&get_cpu_var(shared_msrs);

Can use __get_cpu_var() since interrupts are disabled.

Quoted 19 lines

arch/x86/kvm/x86.c +17 -0

+	/* only read, and nobody should modify it at this time,
+	 * so don't need lock */
+	for (slot = 0; slot<  shared_msrs_global.nr; slot++)
+		if (shared_msrs_global.msrs[slot] == msr)
+			break;
+	if (slot>= shared_msrs_global.nr) {
+		printk(KERN_ERR "kvm: can't find the defined MSR!");
+		return;
+	}
+	rdmsrl_safe(msr,&value);
+	smsr->values[slot].host = value;
+	smsr->values[slot].curr = value;
+	put_cpu_var(shared_msrs);
+}
+
+void kvm_define_shared_msr(unsigned slot, u32 msr)
+{
  	if (slot>= shared_msrs_global.nr)

arch/x86/kvm/x86.c struct kvm_stats_debugfs_item debugfs_entries[] = {

Quoted 10 lines

arch/x86/kvm/x86.c +4 -5

  		shared_msrs_global.nr = slot + 1;
-	shared_msrs_global.msrs[slot].msr = msr;
-	rdmsrl_safe(msr,&value);
-	shared_msrs_global.msrs[slot].value = value;
-	for_each_online_cpu(cpu)
-		per_cpu(shared_msrs, cpu).current_value[slot] = value;
+	shared_msrs_global.msrs[slot] = msr;
+	/* we need ensured the shared_msr_global have been updated */
+	smp_wmb();
+	smp_call_function(shared_msr_update,&msr, 1);

on_each_cpu() is preferred.

Quoted 4 lines

arch/x86/kvm/x86.c +1 -0

+	shared_msr_update(&msr);
  }
  EXPORT_SYMBOL_GPL(kvm_define_shared_msr);

arch/x86/kvm/x86.c struct kvm_stats_debugfs_item debugfs_entries[] = { · line 205

Quoted 10 lines

arch/x86/kvm/x86.c +2 -2

 
  static void kvm_shared_msr_cpu_online(void)
  {
  	unsigned i;
-	struct kvm_shared_msrs *locals =&__get_cpu_var(shared_msrs);
+	struct kvm_shared_msrs *smsr =&__get_cpu_var(shared_msrs);
 
  	for (i = 0; i<  shared_msrs_global.nr; ++i)
-		locals->current_value[i] = shared_msrs_global.msrs[i].value;
+		smsr->values[i].curr = smsr->values[i].host;

If the cpu is being onlined for the first time, then .host will not have been initialized. Need to call shared_msr_update().

Also need to verify this is called after all the msrs have been initialized by the kernel, or we'll read default values.

Signature · 1 line

error compiling committee.c: too many arguments to function

Sheng Yang

author

2009-12-17 09:32 UTC #4 lore raw

PATCH 2/4 KVM: Extended shared_msr_global to per CPU

shared_msr_global saved host value of relevant MSRs, but it have an assumption that all MSRs it tracked shared the value across the different CPUs. It's not true with some MSRs, e.g. MSR_TSC_AUX.

Extend it to per CPU to provide the support of MSR_TSC_AUX, and more alike MSRs.

Notice now the shared_msr_global still have one assumption: it can only deal with the MSRs that won't change in host after KVM module loaded.

Signed-off-by Sheng Yang <[email protected]>

---

How about this?

Move the all initialization to hardware_enable(). And only initialized once for each cpu.

 arch/x86/kvm/x86.c |   60 ++++++++++++++++++++++++++++++++-------------------
 1 files changed, 38 insertions(+), 22 deletions(-)

arch/x86/kvm/x86.c +38 ~~-22~~

index dd15d7a..e3eae50 100644
@@ -92,16 +92,17 @@ module_param_named(ignore_msrs, ignore_msrs, bool, S_IRUGO | S_IWUSR);
 
 struct kvm_shared_msrs_global {
 	int nr;
-	struct kvm_shared_msr {
-		u32 msr;
-		u64 value;
-	} msrs[KVM_NR_SHARED_MSRS];
+	u32 msrs[KVM_NR_SHARED_MSRS];
 };
 
 struct kvm_shared_msrs {
 	struct user_return_notifier urn;
 	bool registered;
-	u64 current_value[KVM_NR_SHARED_MSRS];
+	struct kvm_shared_msr_values {
+		u64 host;
+		u64 curr;
+		bool initialized;
+	} values[KVM_NR_SHARED_MSRS];
 };
 
 static struct kvm_shared_msrs_global __read_mostly shared_msrs_global;

@@ -146,53 +147,68 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 static void kvm_on_user_return(struct user_return_notifier *urn)
 {
 	unsigned slot;
-	struct kvm_shared_msr *global;
 	struct kvm_shared_msrs *locals
 		= container_of(urn, struct kvm_shared_msrs, urn);
+	struct kvm_shared_msr_values *values;
 
 	for (slot = 0; slot < shared_msrs_global.nr; ++slot) {
-		global = &shared_msrs_global.msrs[slot];
-		if (global->value != locals->current_value[slot]) {
-			wrmsrl(global->msr, global->value);
-			locals->current_value[slot] = global->value;
+		values = &locals->values[slot];
+		if (values->host != values->curr) {
+			wrmsrl(shared_msrs_global.msrs[slot], values->host);
+			values->curr = values->host;
 		}
 	}
 	locals->registered = false;
 	user_return_notifier_unregister(urn);
 }
 
-void kvm_define_shared_msr(unsigned slot, u32 msr)
+static void shared_msr_update(unsigned slot, u32 msr)
 {
-	int cpu;
+	struct kvm_shared_msrs *smsr;
 	u64 value;
 
+	smsr = &__get_cpu_var(shared_msrs);
+	/* only read, and nobody should modify it at this time,
+	 * so don't need lock */
+	if (slot >= shared_msrs_global.nr) {
+		printk(KERN_ERR "kvm: invalid MSR slot!");
+		return;
+	}
+	if (smsr->values[slot].initialized)
+		return;
+	rdmsrl_safe(msr, &value);
+	smsr->values[slot].host = value;
+	smsr->values[slot].curr = value;
+	smsr->values[slot].initialized = true;
+	put_cpu_var(shared_msrs);
+}
+
+void kvm_define_shared_msr(unsigned slot, u32 msr)
+{
 	if (slot >= shared_msrs_global.nr)
 		shared_msrs_global.nr = slot + 1;
-	shared_msrs_global.msrs[slot].msr = msr;
-	rdmsrl_safe(msr, &value);
-	shared_msrs_global.msrs[slot].value = value;
-	for_each_online_cpu(cpu)
-		per_cpu(shared_msrs, cpu).current_value[slot] = value;
+	shared_msrs_global.msrs[slot] = msr;
+	/* we need ensured the shared_msr_global have been updated */
+	smp_wmb();
 }
 EXPORT_SYMBOL_GPL(kvm_define_shared_msr);
 
 static void kvm_shared_msr_cpu_online(void)
 {
 	unsigned i;
-	struct kvm_shared_msrs *locals = &__get_cpu_var(shared_msrs);
 
 	for (i = 0; i < shared_msrs_global.nr; ++i)
-		locals->current_value[i] = shared_msrs_global.msrs[i].value;
+		shared_msr_update(i, shared_msrs_global.msrs[i]);
 }
 
 void kvm_set_shared_msr(unsigned slot, u64 value, u64 mask)
 {
 	struct kvm_shared_msrs *smsr = &__get_cpu_var(shared_msrs);
 
-	if (((value ^ smsr->current_value[slot]) & mask) == 0)
+	if (((value ^ smsr->values[slot].curr) & mask) == 0)
 		return;
-	smsr->current_value[slot] = value;
-	wrmsrl(shared_msrs_global.msrs[slot].msr, value);
+	smsr->values[slot].curr = value;
+	wrmsrl(shared_msrs_global.msrs[slot], value);
 	if (!smsr->registered) {
 		smsr->urn.on_user_return = kvm_on_user_return;
 		user_return_notifier_register(&smsr->urn);

Signature · 1 line

1.5.4.5

Avi Kivity

2009-12-17 10:32 UTC #5 lore raw

Re: [PATCH 2/4] KVM: Extended shared_msr_global to per CPU

Quoted 26 lines · Sheng Yang

shared_msr_global saved host value of relevant MSRs, but it have an assumption that all MSRs it tracked shared the value across the different CPUs. It's not true with some MSRs, e.g. MSR_TSC_AUX.
Extend it to per CPU to provide the support of MSR_TSC_AUX, and more alike MSRs.
Notice now the shared_msr_global still have one assumption: it can only deal with the MSRs that won't change in host after KVM module loaded.
Signed-off-by Sheng Yang<[email protected]>
---
How about this?
Move the all initialization to hardware_enable(). And only initialized once for each cpu.
arch/x86/kvm/x86.c +2 -2
-void kvm_define_shared_msr(unsigned slot, u32 msr)
+static void shared_msr_update(unsigned slot, u32 msr)
  {
-	int cpu;
+	struct kvm_shared_msrs *smsr;
  	u64 value;

arch/x86/kvm/x86.c struct kvm_stats_debugfs_item debugfs_entries[] = { · line 178

Quoted 10 lines

arch/x86/kvm/x86.c +9 -0

 
+	smsr =&__get_cpu_var(shared_msrs);
+	/* only read, and nobody should modify it at this time,
+	 * so don't need lock */
+	if (slot>= shared_msrs_global.nr) {
+		printk(KERN_ERR "kvm: invalid MSR slot!");
+		return;
+	}
+	if (smsr->values[slot].initialized)
+		return;

I don't think .initialized is worthwhile. shared_msr_update is run very rarely.

arch/x86/kvm/x86.c struct kvm_stats_debugfs_item debugfs_entries[] = { · line 183

Quoted 3 lines

arch/x86/kvm/x86.c +2 -0

+	smsr->values[slot].initialized = true;
+	put_cpu_var(shared_msrs);

If you use __get_cpu_var(), you need to remove put_cpu_var().

Signature · 1 line

error compiling committee.c: too many arguments to function

Sheng Yang

author

2009-12-17 15:01 UTC #6 lore raw

Re: [PATCH 2/4] KVM: Extended shared_msr_global to per CPU

Quoted 27 lines · On Thursday 17 December 2009 18:32:08 Avi Kivity wrote:

Quoted 26 lines · Sheng Yang
shared_msr_global saved host value of relevant MSRs, but it have an assumption that all MSRs it tracked shared the value across the different CPUs. It's not true with some MSRs, e.g. MSR_TSC_AUX.
Extend it to per CPU to provide the support of MSR_TSC_AUX, and more alike MSRs.
Notice now the shared_msr_global still have one assumption: it can only deal with the MSRs that won't change in host after KVM module loaded.
Signed-off-by Sheng Yang<[email protected]>
---
How about this?
Move the all initialization to hardware_enable(). And only initialized once for each cpu.
arch/x86/kvm/x86.c +2 -2
-void kvm_define_shared_msr(unsigned slot, u32 msr)
+static void shared_msr_update(unsigned slot, u32 msr)
  {
-	int cpu;
+	struct kvm_shared_msrs *smsr;
  	u64 value;

Quoted 12 lines

arch/x86/kvm/x86.c struct kvm_stats_debugfs_item debugfs_entries[] = { · line 178
Quoted 9 lines
arch/x86/kvm/x86.c +9 -0
+	smsr =&__get_cpu_var(shared_msrs);
+	/* only read, and nobody should modify it at this time,
+	 * so don't need lock */
+	if (slot>= shared_msrs_global.nr) {
+		printk(KERN_ERR "kvm: invalid MSR slot!");
+		return;
+	}
+	if (smsr->values[slot].initialized)
+		return;
I don't think .initialized is worthwhile. shared_msr_update is run very rarely.

The reason is, after cpu hotplug, the MSR_TSC_AUX would be rewritten by vsyscall_init again. But in the hotplug notifier chain, KVM has higher priority(20 vs 0 for vsyscall_init), so maybe the rdmsr() here would get a bogus value... Then I think prevent it from initializing again should be safer.

But I just think of another issue: if we hot plug in a cpu(without hot plug off), it would have a bogus value as well in the same path? Sound troublesome...

Quoted 6 lines

arch/x86/kvm/x86.c struct kvm_stats_debugfs_item debugfs_entries[] = { · line 183
Quoted 3 lines
arch/x86/kvm/x86.c +2 -0
+	smsr->values[slot].initialized = true;
+	put_cpu_var(shared_msrs);
If you use __get_cpu_var(), you need to remove put_cpu_var().

Sure...

Signature · 2 lines

regards
Yang, Sheng

Avi Kivity

2009-12-17 15:06 UTC #7 lore raw

Re: [PATCH 2/4] KVM: Extended shared_msr_global to per CPU

Quoted 21 lines · Sheng Yang

Quoted 8 lines
Quoted 8 lines
arch/x86/kvm/x86.c +2 -2
-void kvm_define_shared_msr(unsigned slot, u32 msr)
+static void shared_msr_update(unsigned slot, u32 msr)
   {
-	int cpu;
+	struct kvm_shared_msrs *smsr;
   	u64 value;
Quoted 13 lines
arch/x86/kvm/x86.c struct kvm_stats_debugfs_item debugfs_entries[] = { · line 178
Quoted 10 lines
arch/x86/kvm/x86.c +9 -0
+	smsr =&__get_cpu_var(shared_msrs);
+	/* only read, and nobody should modify it at this time,
+	 * so don't need lock */
+	if (slot>= shared_msrs_global.nr) {
+		printk(KERN_ERR "kvm: invalid MSR slot!");
+		return;
+	}
+	if (smsr->values[slot].initialized)
+		return;
I don't think .initialized is worthwhile. shared_msr_update is run very rarely.

Quoted 7 lines

The reason is, after cpu hotplug, the MSR_TSC_AUX would be rewritten by vsyscall_init again. But in the hotplug notifier chain, KVM has higher priority(20 vs 0 for vsyscall_init), so maybe the rdmsr() here would get a bogus value... Then I think prevent it from initializing again should be safer.

So let's raise vsyscall_init's priority?

Quoted 4 lines

But I just think of another issue: if we hot plug in a cpu(without hot plug off), it would have a bogus value as well in the same path? Sound troublesome...

Removing .initialized would take care of this issue.

Signature · 1 line

error compiling committee.c: too many arguments to function

Sheng Yang

author

2009-12-16 05:48 UTC #8 lore raw

PATCH 3/4 x86: Add IA32_TSC_AUX MSR

In reply to Sheng Yang

Also replaced the hardcode value in write_tsc() and write_tscp_aux().

CC Ingo Molnar <[email protected]>

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/include/asm/msr-index.h |    1 +
 arch/x86/include/asm/msr.h       |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

arch/x86/include/asm/msr-index.h +1 -0

index 4ffe09b..ac98d29 100644
@@ -12,6 +12,7 @@
 #define MSR_FS_BASE		0xc0000100 /* 64bit FS base */
 #define MSR_GS_BASE		0xc0000101 /* 64bit GS base */
 #define MSR_KERNEL_GS_BASE	0xc0000102 /* SwapGS GS shadow */
+#define MSR_TSC_AUX		0xc0000103 /* Auxiliary TSC */
 
 /* EFER bits: */
 #define _EFER_SCE		0  /* SYSCALL/SYSRET */

arch/x86/include/asm/msr.h +2 -2

index 7e2b6ba..e61fb87 100644
@@ -240,9 +240,9 @@ do {                                                            \
 #define checking_wrmsrl(msr, val) wrmsr_safe((msr), (u32)(val),		\
 					     (u32)((val) >> 32))
 
-#define write_tsc(val1, val2) wrmsr(0x10, (val1), (val2))
+#define write_tsc(val1, val2) wrmsr(MSR_IA32_TSC, (val1), (val2))
 
-#define write_rdtscp_aux(val) wrmsr(0xc0000103, (val), 0)
+#define write_rdtscp_aux(val) wrmsr(MSR_TSC_AUX, (val), 0)
 
 #ifdef CONFIG_SMP
 int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);

Signature · 1 line

1.5.4.5

tip-bot for Sheng Yang

author

2009-12-16 08:21 UTC #9 lore raw

[tip:x86/urgent] x86: Add IA32_TSC_AUX MSR and use it

Commit-ID:  5df974009fe513c664303de24725ea0f8b47f12e
Gitweb:     http://git.kernel.org/tip/5df974009fe513c664303de24725ea0f8b47f12e
Author:     Sheng Yang <[email protected]>
AuthorDate: Wed, 16 Dec 2009 13:48:04 +0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 16 Dec 2009 09:02:42 +0100

x86: Add IA32_TSC_AUX MSR and use it

Clean up write_tsc() and write_tscp_aux() by replacing hardcoded values.

No change in functionality.

Signed-off-by Sheng Yang <[email protected]>

Cc Avi Kivity <[email protected]>

Cc Marcelo Tosatti <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/include/asm/msr-index.h |    1 +
 arch/x86/include/asm/msr.h       |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

arch/x86/include/asm/msr-index.h +1 -0

index 4ffe09b..ac98d29 100644
@@ -12,6 +12,7 @@
 #define MSR_FS_BASE		0xc0000100 /* 64bit FS base */
 #define MSR_GS_BASE		0xc0000101 /* 64bit GS base */
 #define MSR_KERNEL_GS_BASE	0xc0000102 /* SwapGS GS shadow */
+#define MSR_TSC_AUX		0xc0000103 /* Auxiliary TSC */
 
 /* EFER bits: */
 #define _EFER_SCE		0  /* SYSCALL/SYSRET */

arch/x86/include/asm/msr.h +2 -2

index 2d228fc..cf985aa 100644
@@ -240,9 +240,9 @@ do {                                                            \
 #define checking_wrmsrl(msr, val) wrmsr_safe((msr), (u32)(val),		\
 					     (u32)((val) >> 32))
 
-#define write_tsc(val1, val2) wrmsr(0x10, (val1), (val2))
+#define write_tsc(val1, val2) wrmsr(MSR_IA32_TSC, (val1), (val2))
 
-#define write_rdtscp_aux(val) wrmsr(0xc0000103, (val), 0)
+#define write_rdtscp_aux(val) wrmsr(MSR_TSC_AUX, (val), 0)
 
 struct msr *msrs_alloc(void);
 void msrs_free(struct msr *msrs);

Sheng Yang

author

2009-12-16 05:48 UTC #10 lore raw

PATCH 4/4 KVM: VMX: Add instruction rdtscp support for guest

In reply to Sheng Yang

Before enabling, execution of "rdtscp" in guest would result in #UD.

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/include/asm/vmx.h      |    1 +
 arch/x86/kvm/svm.c              |    6 ++++++
 arch/x86/kvm/vmx.c              |   31 ++++++++++++++++++++++++++++---
 arch/x86/kvm/x86.c              |    3 ++-
 5 files changed, 38 insertions(+), 4 deletions(-)

arch/x86/include/asm/kvm_host.h +1 -0

index 4f865e8..3a84acf 100644
@@ -532,6 +532,7 @@ struct kvm_x86_ops {
 	int (*get_tdp_level)(void);
 	u64 (*get_mt_mask)(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio);
 	bool (*gb_page_enable)(void);
+	bool (*rdtscp_enable)(void);
 
 	const struct trace_print_flags *exit_reasons_str;
 };

arch/x86/include/asm/vmx.h +1 -0

index 2b49454..8c39320 100644
@@ -53,6 +53,7 @@
  */
 #define SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES 0x00000001
 #define SECONDARY_EXEC_ENABLE_EPT               0x00000002
+#define SECONDARY_EXEC_RDTSCP			0x00000008
 #define SECONDARY_EXEC_ENABLE_VPID              0x00000020
 #define SECONDARY_EXEC_WBINVD_EXITING		0x00000040
 #define SECONDARY_EXEC_UNRESTRICTED_GUEST	0x00000080

arch/x86/kvm/svm.c +6 -0

index 3de0b37..3e67d16 100644
@@ -2912,6 +2912,11 @@ static bool svm_gb_page_enable(void)
 	return true;
 }
 
+static bool svm_rdtscp_enable(void)
+{
+	return false;
+}
+
 static struct kvm_x86_ops svm_x86_ops = {
 	.cpu_has_kvm_support = has_svm,
 	.disabled_by_bios = is_disabled,

@@ -2978,6 +2983,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 
 	.exit_reasons_str = svm_exit_reasons_str,
 	.gb_page_enable = svm_gb_page_enable,
+	.rdtscp_enable = svm_rdtscp_enable,
 };
 
 static int __init svm_init(void)

arch/x86/kvm/vmx.c +28 -3

index 5c464ed..57166b8 100644
@@ -210,7 +210,7 @@ static const u32 vmx_msr_index[] = {
 #ifdef CONFIG_X86_64
 	MSR_SYSCALL_MASK, MSR_LSTAR, MSR_CSTAR,
 #endif
-	MSR_EFER, MSR_K6_STAR,
+	MSR_EFER, MSR_TSC_AUX, MSR_K6_STAR,
 };
 #define NR_VMX_MSR ARRAY_SIZE(vmx_msr_index)

@@ -347,6 +347,12 @@ static inline int cpu_has_vmx_vpid(void)
 		SECONDARY_EXEC_ENABLE_VPID;
 }
 
+static inline int cpu_has_vmx_rdtscp(void)
+{
+	return vmcs_config.cpu_based_2nd_exec_ctrl &
+		SECONDARY_EXEC_RDTSCP;
+}
+
 static inline int cpu_has_virtual_nmis(void)
 {
 	return vmcs_config.pin_based_exec_ctrl & PIN_BASED_VIRTUAL_NMIS;

@@ -878,6 +884,11 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
 	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, intr_info);
 }
 
+static bool vmx_rdtscp_enable(void)
+{
+	return cpu_has_vmx_rdtscp();
+}
+
 /*
  * Swap MSR entry in host/guest MSR entry array.
  */

@@ -913,6 +924,9 @@ static void setup_msrs(struct vcpu_vmx *vmx)
 		index = __find_msr_index(vmx, MSR_CSTAR);
 		if (index >= 0)
 			move_msr_up(vmx, index, save_nmsrs++);
+		index = __find_msr_index(vmx, MSR_TSC_AUX);
+		if (index >= 0)
+			move_msr_up(vmx, index, save_nmsrs++);
 		/*
 		 * MSR_K6_STAR is only needed on long mode guests, and only
 		 * if efer.sce is enabled.

@@ -1002,6 +1016,10 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
 	case MSR_IA32_SYSENTER_ESP:
 		data = vmcs_readl(GUEST_SYSENTER_ESP);
 		break;
+	case MSR_TSC_AUX:
+		if (!vmx_rdtscp_enable())
+			return 1;
+		/* Otherwise falls through */
 	default:
 		vmx_load_host_state(to_vmx(vcpu));
 		msr = find_msr_entry(to_vmx(vcpu), msr_index);

@@ -1065,7 +1083,12 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
 			vcpu->arch.pat = data;
 			break;
 		}
-		/* Otherwise falls through to kvm_set_msr_common */
+		ret = kvm_set_msr_common(vcpu, msr_index, data);
+		break;
+	case MSR_TSC_AUX:
+		if (!vmx_rdtscp_enable())
+			return 1;
+		/* Otherwise falls through */
 	default:
 		msr = find_msr_entry(vmx, msr_index);
 		if (msr) {

@@ -1243,7 +1266,8 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
 			SECONDARY_EXEC_ENABLE_VPID |
 			SECONDARY_EXEC_ENABLE_EPT |
 			SECONDARY_EXEC_UNRESTRICTED_GUEST |
-			SECONDARY_EXEC_PAUSE_LOOP_EXITING;
+			SECONDARY_EXEC_PAUSE_LOOP_EXITING |
+			SECONDARY_EXEC_RDTSCP;
 		if (adjust_vmx_controls(min2, opt2,
 					MSR_IA32_VMX_PROCBASED_CTLS2,
 					&_cpu_based_2nd_exec_control) < 0)

@@ -4025,6 +4049,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
 
 	.exit_reasons_str = vmx_exit_reasons_str,
 	.gb_page_enable = vmx_gb_page_enable,
+	.rdtscp_enable = vmx_rdtscp_enable,
 };
 
 static int __init vmx_init(void)

arch/x86/kvm/x86.c +2 -1

index 4262450..3b05613 100644
@@ -1653,6 +1653,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 #else
 	unsigned f_lm = 0;
 #endif
+	unsigned f_rdtscp = kvm_x86_ops->rdtscp_enable() ? F(RDTSCP) : 0;
 
 	/* cpuid 1.edx */
 	const u32 kvm_supported_word0_x86_features =

@@ -1672,7 +1673,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		F(MTRR) | F(PGE) | F(MCA) | F(CMOV) |
 		F(PAT) | F(PSE36) | 0 /* Reserved */ |
 		f_nx | 0 /* Reserved */ | F(MMXEXT) | F(MMX) |
-		F(FXSR) | F(FXSR_OPT) | f_gbpages | 0 /* RDTSCP */ |
+		F(FXSR) | F(FXSR_OPT) | f_gbpages | f_rdtscp |
 		0 /* Reserved */ | f_lm | F(3DNOWEXT) | F(3DNOW);
 	/* cpuid 1.ecx */
 	const u32 kvm_supported_word4_x86_features =

Signature · 1 line

1.5.4.5

Avi Kivity

2009-12-16 09:47 UTC #11 lore raw

Re: [PATCH 4/4] KVM: VMX: Add instruction rdtscp support for guest

arch/x86/include/asm/kvm_host.h struct kvm_x86_ops { · line 536

Quoted 13 lines · Sheng Yang

Before enabling, execution of "rdtscp" in guest would result in #UD.

arch/x86/include/asm/kvm_host.h +1 -0

index 4f865e8..3a84acf 100644
@@ -532,6 +532,7 @@ struct kvm_x86_ops {
  	int (*get_tdp_level)(void);
  	u64 (*get_mt_mask)(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio);
  	bool (*gb_page_enable)(void);
+	bool (*rdtscp_enable)(void);

Naming - a better name is rdtsp_supported(). rdtscp_enable sounds like you change it from disabled to enabled.

arch/x86/include/asm/kvm_host.h static void setup_msrs(struct vcpu_vmx *vmx) · line 930

Quoted 8 lines

arch/x86/include/asm/kvm_host.h +3 -0

@@ -913,6 +924,9 @@ static void setup_msrs(struct vcpu_vmx *vmx)
  		index = __find_msr_index(vmx, MSR_CSTAR);
  		if (index>= 0)
  			move_msr_up(vmx, index, save_nmsrs++);
+		index = __find_msr_index(vmx, MSR_TSC_AUX);
+		if (index>= 0)
+			move_msr_up(vmx, index, save_nmsrs++);

Only if rdtscp is enabled in the guest's cpuid, so we don't play with this unnecessarily. If it isn't, we should trap rdtscp and inject #UD. If it is, support the msr and don't trap.

arch/x86/include/asm/kvm_host.h static int vmx_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata) · line 1022

Quoted 11 lines

arch/x86/include/asm/kvm_host.h +0 -0

  		/*
  		 * MSR_K6_STAR is only needed on long mode guests, and only

* if efer.sce is enabled.

arch/x86/include/asm/kvm_host.h +3 -0

@@ -1002,6 +1016,10 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
  	case MSR_IA32_SYSENTER_ESP:
  		data = vmcs_readl(GUEST_SYSENTER_ESP);
  		break;
+	case MSR_TSC_AUX:
+		if (!vmx_rdtscp_enable())
+			return 1;

Again, check the guest rdtscp bit, not (just) the host's.

Signature · 1 line

error compiling committee.c: too many arguments to function

Sheng Yang

author

2009-12-17 09:33 UTC #12 lore raw

PATCH 4/4 KVM: VMX: Add instruction rdtscp support for guest

Before enabling, execution of "rdtscp" in guest would result in #UD.

Signed-off-by Sheng Yang <[email protected]>

---

Reflect guest CPUID on vmcs fields as well, but it involved some more code which would only executed once... Do we need a callback there for post-"cpuid setting"?

 arch/x86/include/asm/kvm_host.h |    3 ++
 arch/x86/include/asm/vmx.h      |    1 +
 arch/x86/kvm/svm.c              |    6 ++++
 arch/x86/kvm/vmx.c              |   49 ++++++++++++++++++++++++++++++++++++--
 arch/x86/kvm/x86.c              |   15 ++++++++++-
 5 files changed, 69 insertions(+), 5 deletions(-)

arch/x86/include/asm/kvm_host.h +3 -0

index 4f865e8..c920285 100644
@@ -374,6 +374,8 @@ struct kvm_vcpu_arch {
 	/* used for guest single stepping over the given code position */
 	u16 singlestep_cs;
 	unsigned long singlestep_rip;
+
+	bool cpuid_rdtscp_enabled;
 };
 
 struct kvm_mem_alias {

@@ -532,6 +534,7 @@ struct kvm_x86_ops {
 	int (*get_tdp_level)(void);
 	u64 (*get_mt_mask)(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio);
 	bool (*gb_page_enable)(void);
+	bool (*rdtscp_supported)(void);
 
 	const struct trace_print_flags *exit_reasons_str;
 };

arch/x86/include/asm/vmx.h +1 -0

index 2b49454..8c39320 100644
@@ -53,6 +53,7 @@
  */
 #define SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES 0x00000001
 #define SECONDARY_EXEC_ENABLE_EPT               0x00000002
+#define SECONDARY_EXEC_RDTSCP			0x00000008
 #define SECONDARY_EXEC_ENABLE_VPID              0x00000020
 #define SECONDARY_EXEC_WBINVD_EXITING		0x00000040
 #define SECONDARY_EXEC_UNRESTRICTED_GUEST	0x00000080

arch/x86/kvm/svm.c +6 -0

index 3de0b37..052d91a 100644
@@ -2912,6 +2912,11 @@ static bool svm_gb_page_enable(void)
 	return true;
 }
 
+static bool svm_rdtscp_supported(void)
+{
+	return false;
+}
+
 static struct kvm_x86_ops svm_x86_ops = {
 	.cpu_has_kvm_support = has_svm,
 	.disabled_by_bios = is_disabled,

@@ -2978,6 +2983,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 
 	.exit_reasons_str = svm_exit_reasons_str,
 	.gb_page_enable = svm_gb_page_enable,
+	.rdtscp_supported = svm_rdtscp_supported,
 };
 
 static int __init svm_init(void)

arch/x86/kvm/vmx.c +46 -3

121 lines not on this page · raw message

arch/x86/kvm/x86.c +13 -2

index e3eae50..cb6c879 100644
@@ -1544,6 +1544,16 @@ static void cpuid_fix_nx_cap(struct kvm_vcpu *vcpu)
 	}
 }
 
+static void update_cpuid_support_rdtscp(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	vcpu->arch.cpuid_rdtscp_enabled = false;
+	best = kvm_find_cpuid_entry(vcpu, 0x80000001, 0);
+	if (best && (best->edx & bit(X86_FEATURE_RDTSCP)))
+		vcpu->arch.cpuid_rdtscp_enabled = true;
+}
+
 /* when an old userspace process fills a new kernel module */
 static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 				    struct kvm_cpuid *cpuid,

@@ -1579,7 +1589,6 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 	cpuid_fix_nx_cap(vcpu);
 	r = 0;
 	kvm_apic_set_version(vcpu);
-
 out_free:
 	vfree(cpuid_entries);
 out:

@@ -1601,6 +1610,7 @@ static int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
 		goto out;
 	vcpu->arch.cpuid_nent = cpuid->nent;
 	kvm_apic_set_version(vcpu);
+	update_cpuid_support_rdtscp(vcpu);
 	return 0;
 
 out:

@@ -1649,6 +1659,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 #else
 	unsigned f_lm = 0;
 #endif
+	unsigned f_rdtscp = kvm_x86_ops->rdtscp_supported() ? F(RDTSCP) : 0;
 
 	/* cpuid 1.edx */
 	const u32 kvm_supported_word0_x86_features =

@@ -1668,7 +1679,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		F(MTRR) | F(PGE) | F(MCA) | F(CMOV) |
 		F(PAT) | F(PSE36) | 0 /* Reserved */ |
 		f_nx | 0 /* Reserved */ | F(MMXEXT) | F(MMX) |
-		F(FXSR) | F(FXSR_OPT) | f_gbpages | 0 /* RDTSCP */ |
+		F(FXSR) | F(FXSR_OPT) | f_gbpages | f_rdtscp |
 		0 /* Reserved */ | f_lm | F(3DNOWEXT) | F(3DNOW);
 	/* cpuid 1.ecx */
 	const u32 kvm_supported_word4_x86_features =

Signature · 1 line

1.5.4.5

Avi Kivity

2009-12-17 10:39 UTC #13 lore raw

Re: [PATCH 4/4] KVM: VMX: Add instruction rdtscp support for guest

Quoted 9 lines · Sheng Yang

Before enabling, execution of "rdtscp" in guest would result in #UD.
Signed-off-by Sheng Yang<[email protected]>
---
Reflect guest CPUID on vmcs fields as well, but it involved some more code which would only executed once... Do we need a callback there for post-"cpuid setting"?

I guess we do need a callback.

arch/x86/kvm/vmx.c static int vmx_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data) · line 1095

Quoted 10 lines

arch/x86/kvm/vmx.c +8 -1

-		/* Otherwise falls through to kvm_set_msr_common */
+		ret = kvm_set_msr_common(vcpu, msr_index, data);
+		break;
+	case MSR_TSC_AUX:
+		if (!vcpu->arch.cpuid_rdtscp_enabled || !vmx_rdtscp_supported())
+			return 1;
+		/* Check reserved bit */
+		if ((data&  0xffff0000) != 0)
+			return 1;

It's 0xffffffff00000000...

arch/x86/kvm/vmx.c static void setup_msrs(struct vcpu_vmx *vmx) · line 932

Quoted 8 lines

arch/x86/kvm/vmx.c +3 -0

@@ -913,6 +926,9 @@ static void setup_msrs(struct vcpu_vmx *vmx)
  		index = __find_msr_index(vmx, MSR_CSTAR);
  		if (index>= 0)
  			move_msr_up(vmx, index, save_nmsrs++);
+		index = __find_msr_index(vmx, MSR_TSC_AUX);
+		if (index>= 0)
+			move_msr_up(vmx, index, save_nmsrs++);

Only do this if rdtscp is enabled!

Signature · 1 line

error compiling committee.c: too many arguments to function

Sheng Yang

author

2009-12-17 14:52 UTC #14 lore raw

Re: [PATCH 4/4] KVM: VMX: Add instruction rdtscp support for guest

Quoted 12 lines · On Thursday 17 December 2009 18:39:22 Avi Kivity wrote:

Quoted 8 lines · Sheng Yang
Before enabling, execution of "rdtscp" in guest would result in #UD.
Signed-off-by Sheng Yang<[email protected]>
---
Reflect guest CPUID on vmcs fields as well, but it involved some more code which would only executed once... Do we need a callback there for post-"cpuid setting"?
I guess we do need a callback.

Quoted 11 lines

arch/x86/kvm/vmx.c static int vmx_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data) · line 1095
Quoted 9 lines
arch/x86/kvm/vmx.c +8 -1
-		/* Otherwise falls through to kvm_set_msr_common */
+		ret = kvm_set_msr_common(vcpu, msr_index, data);
+		break;
+	case MSR_TSC_AUX:
+		if (!vcpu->arch.cpuid_rdtscp_enabled || !vmx_rdtscp_supported())
+			return 1;
+		/* Check reserved bit */
+		if ((data&  0xffff0000) != 0)
+			return 1;
It's 0xffffffff00000000...

... /me blame himself...

Quoted 10 lines

arch/x86/kvm/vmx.c static void setup_msrs(struct vcpu_vmx *vmx) · line 931

Quoted 8 lines

arch/x86/kvm/vmx.c +3 -0

@@ -913,6 +926,9 @@ static void setup_msrs(struct vcpu_vmx *vmx)
  		index = __find_msr_index(vmx, MSR_CSTAR);
  		if (index>= 0)
  			move_msr_up(vmx, index, save_nmsrs++);
+		index = __find_msr_index(vmx, MSR_TSC_AUX);
+		if (index>= 0)
+			move_msr_up(vmx, index, save_nmsrs++);

Only do this if rdtscp is enabled!

/me blame himself again...

Signature · 2 lines

regards
Yang, Sheng