[PATCH 0/4] Add support for RDTSCP in VMX

[PATCH 0/4] Add support for RDTSCP in VMX

[Sheng Yang]

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(-)

[PATCH 1/4] KVM: VMX: Remove redundant variable

[Sheng Yang]

It's no longer necessary.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/kvm/vmx.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 9a0a2cf..5c464ed 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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;
-- 
1.5.4.5

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

[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 <sheng@linux.intel.com>
---
 arch/x86/kvm/x86.c |   64 ++++++++++++++++++++++++++++++++++------------------
 1 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index dd15d7a..4262450 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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);
-- 
1.5.4.5

[PATCH 3/4] x86: Add IA32_TSC_AUX MSR

[Sheng Yang]

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

CC: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/include/asm/msr-index.h |    1 +
 arch/x86/include/asm/msr.h       |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 4ffe09b..ac98d29 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -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 */
diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 7e2b6ba..e61fb87 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -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);
-- 
1.5.4.5

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

[Sheng Yang]

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

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 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(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 4f865e8..3a84acf 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -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;
 };
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 2b49454..8c39320 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -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
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 3de0b37..3e67d16 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -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)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 5c464ed..57166b8 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4262450..3b05613 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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 =
-- 
1.5.4.5

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

[Avi Kivity]

On 12/16/2009 07:48 AM, Sheng Yang wrote:
> 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.
>
>
>
> -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.

> +	/* 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);
>    

on_each_cpu() is preferred.

> +	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;
>    

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.


-- 
error compiling committee.c: too many arguments to function

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

[Avi Kivity]

On 12/16/2009 07:48 AM, Sheng Yang wrote:
> Before enabling, execution of "rdtscp" in guest would result in #UD.
>
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 4f865e8..3a84acf 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -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.

> @@ -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.

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

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


-- 
error compiling committee.c: too many arguments to function

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

[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 <sheng@linux.intel.com>
---

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(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index dd15d7a..e3eae50 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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);
-- 
1.5.4.5

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

[Sheng Yang]

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

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---

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(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 4f865e8..c920285 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -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;
 };
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 2b49454..8c39320 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -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
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 3de0b37..052d91a 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -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)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 5c464ed..f60e645 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -136,6 +136,8 @@ struct vcpu_vmx {
 	ktime_t entry_time;
 	s64 vnmi_blocked_time;
 	u32 exit_reason;
+
+	bool control_rdtscp_enabled;
 };
 
 static inline struct vcpu_vmx *to_vmx(struct kvm_vcpu *vcpu)
@@ -210,7 +212,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 +349,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 +886,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_supported(void)
+{
+	return cpu_has_vmx_rdtscp();
+}
+
 /*
  * Swap MSR entry in host/guest MSR entry array.
  */
@@ -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++);
 		/*
 		 * MSR_K6_STAR is only needed on long mode guests, and only
 		 * if efer.sce is enabled.
@@ -1002,6 +1018,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 (!vcpu->arch.cpuid_rdtscp_enabled || !vmx_rdtscp_supported())
+			return 1;
+		/* Otherwise falls through */
 	default:
 		vmx_load_host_state(to_vmx(vcpu));
 		msr = find_msr_entry(to_vmx(vcpu), msr_index);
@@ -1065,7 +1085,15 @@ 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 (!vcpu->arch.cpuid_rdtscp_enabled || !vmx_rdtscp_supported())
+			return 1;
+		/* Check reserved bit */
+		if ((data & 0xffff0000) != 0)
+			return 1;
+		/* Otherwise falls through */
 	default:
 		msr = find_msr_entry(vmx, msr_index);
 		if (msr) {
@@ -1243,7 +1271,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)
@@ -2316,6 +2345,10 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
 			exec_control &= ~SECONDARY_EXEC_UNRESTRICTED_GUEST;
 		if (!ple_gap)
 			exec_control &= ~SECONDARY_EXEC_PAUSE_LOOP_EXITING;
+		/* Delay the update of field, because we don't know guest CPUID
+		 * at this time */
+		vmx->control_rdtscp_enabled = !!(exec_control &
+				SECONDARY_EXEC_RDTSCP);
 		vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_control);
 	}
 
@@ -3655,6 +3688,7 @@ static void fixup_rmode_irq(struct vcpu_vmx *vmx)
 static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	u32 exec_control;
 
 	/* Record the guest's net vcpu time for enforced NMI injections. */
 	if (unlikely(!cpu_has_virtual_nmis() && vmx->soft_vnmi_blocked))
@@ -3670,6 +3704,14 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	if (test_bit(VCPU_REGS_RIP, (unsigned long *)&vcpu->arch.regs_dirty))
 		vmcs_writel(GUEST_RIP, vcpu->arch.regs[VCPU_REGS_RIP]);
 
+	/* Check if rdtscp reported to guest */
+	if (!vcpu->arch.cpuid_rdtscp_enabled && vmx->control_rdtscp_enabled) {
+		exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
+		exec_control &= ~SECONDARY_EXEC_RDTSCP;
+		vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_control);
+		vmx->control_rdtscp_enabled = 0;
+	}
+
 	/* When single-stepping over STI and MOV SS, we must clear the
 	 * corresponding interruptibility bits in the guest state. Otherwise
 	 * vmentry fails as it then expects bit 14 (BS) in pending debug
@@ -4025,6 +4067,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
 
 	.exit_reasons_str = vmx_exit_reasons_str,
 	.gb_page_enable = vmx_gb_page_enable,
+	.rdtscp_supported = vmx_rdtscp_supported,
 };
 
 static int __init vmx_init(void)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e3eae50..cb6c879 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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 =
-- 
1.5.4.5

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

[Avi Kivity]

On 12/17/2009 11:32 AM, Sheng Yang wrote:
> 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<sheng@linux.intel.com>
> ---
>
> How about this?
>
> Move the all initialization to hardware_enable(). And only initialized once
> for each cpu.
>
>
> -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;
>    

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

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

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


-- 
error compiling committee.c: too many arguments to function

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

[Avi Kivity]

On 12/17/2009 11:33 AM, Sheng Yang wrote:
> Before enabling, execution of "rdtscp" in guest would result in #UD.
>
> Signed-off-by: Sheng Yang<sheng@linux.intel.com>
> ---
>
> 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.

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

> @@ -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!

-- 
error compiling committee.c: too many arguments to function

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

[Sheng Yang]

On Thursday 17 December 2009 18:39:22 Avi Kivity wrote:
> On 12/17/2009 11:33 AM, Sheng Yang wrote:
> > Before enabling, execution of "rdtscp" in guest would result in #UD.
> >
> > Signed-off-by: Sheng Yang<sheng@linux.intel.com>
> > ---
> >
> > 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.
> 
> > -		/* 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...
> 
> > @@ -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...

-- 
regards
Yang, Sheng

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

[Sheng Yang]

On Thursday 17 December 2009 18:32:08 Avi Kivity wrote:
> On 12/17/2009 11:32 AM, Sheng Yang wrote:
> > 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<sheng@linux.intel.com>
> > ---
> >
> > How about this?
> >
> > Move the all initialization to hardware_enable(). And only initialized
> > once for each cpu.
> >
> >
> > -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;
> 
> 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...

> 
> > +	smsr->values[slot].initialized = true;
> > +	put_cpu_var(shared_msrs);
> 
> If you use __get_cpu_var(), you need to remove put_cpu_var().
> 

Sure...

-- 
regards
Yang, Sheng

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

[Avi Kivity]

On 12/17/2009 05:01 PM, Sheng Yang wrote:
>
>>> -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;
>>>        
>> 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.
>
>    

So let's raise vsyscall_init's priority?

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

-- 
error compiling committee.c: too many arguments to function