[PATCH 1/4] KVM: x86: Check LMA bit before set_efer

[PATCH 1/4] KVM: x86: Check LMA bit before set_efer

[Sheng Yang]

kvm_x86_ops->set_efer() would execute vcpu->arch.efer = efer, so the
checking of LMA bit didn't work.

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cd8a606..2cae460 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -715,11 +715,11 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 			return 1;
 	}
 
-	kvm_x86_ops->set_efer(vcpu, efer);
-
 	efer &= ~EFER_LMA;
 	efer |= vcpu->arch.efer & EFER_LMA;
 
+	kvm_x86_ops->set_efer(vcpu, efer);
+
 	vcpu->arch.efer = efer;
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
-- 
1.7.0.1

[PATCH 2/4] KVM: Clean up duplicate assignment

[Sheng Yang]

mmu.free() already set root_hpa to INVALID_PAGE, no need to do it again in the
destory_kvm_mmu().

kvm_x86_ops->set_cr4() and set_efer() already assign cr4/efer to
vcpu->arch.cr4/efer, no need to do it again later.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/kvm/mmu.c |    5 ++---
 arch/x86/kvm/x86.c |    4 +---
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 95bee9d..5412185 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2450,10 +2450,9 @@ static int init_kvm_mmu(struct kvm_vcpu *vcpu)
 static void destroy_kvm_mmu(struct kvm_vcpu *vcpu)
 {
 	ASSERT(vcpu);
-	if (VALID_PAGE(vcpu->arch.mmu.root_hpa)) {
+	if (VALID_PAGE(vcpu->arch.mmu.root_hpa))
+		/* mmu.free() should set root_hpa = INVALID_PAGE */
 		vcpu->arch.mmu.free(vcpu);
-		vcpu->arch.mmu.root_hpa = INVALID_PAGE;
-	}
 }
 
 int kvm_mmu_reset_context(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2cae460..764f89b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -486,7 +486,7 @@ int __kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 		return 1;
 
 	kvm_x86_ops->set_cr4(vcpu, cr4);
-	vcpu->arch.cr4 = cr4;
+
 	kvm_mmu_reset_context(vcpu);
 
 	return 0;
@@ -720,8 +720,6 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 
 	kvm_x86_ops->set_efer(vcpu, efer);
 
-	vcpu->arch.efer = efer;
-
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
 	kvm_mmu_reset_context(vcpu);
 
-- 
1.7.0.1

[PATCH 3/4] KVM: x86: Remove kvm_mmu_reset_context() in kvm_set_efer()

[Sheng Yang]

Modify EFER won't result in mode switch directly. After EFER.LME set, the
following set CR0.PG would result in mode switch to IA32e. And the later
action already covered by kvm_set_cr0().

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 764f89b..b59fc67 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -721,7 +721,6 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	kvm_x86_ops->set_efer(vcpu, efer);
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
-	kvm_mmu_reset_context(vcpu);
 
 	return 0;
 }
-- 
1.7.0.1

Re: [PATCH 3/4] KVM: x86: Remove kvm_mmu_reset_context() in kvm_set_efer()

[Marcelo Tosatti]

On Tue, May 11, 2010 at 01:30:06PM +0800, Sheng Yang wrote:
> Modify EFER won't result in mode switch directly. After EFER.LME set, the
> following set CR0.PG would result in mode switch to IA32e. And the later
> action already covered by kvm_set_cr0().
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  arch/x86/kvm/x86.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 764f89b..b59fc67 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -721,7 +721,6 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
>  	kvm_x86_ops->set_efer(vcpu, efer);
>  
>  	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
> -	kvm_mmu_reset_context(vcpu);

But there are different sets of shadow pagetables for NXE on/off. See
commit 9645bb56b31a1b.

Without the reset, after NXE 1->0 transition, a spte retains the NXE
validity check, and subsequent use of such gpte with bit 63 set does not
cause a fault.

[PATCH 4/4] VMX: x86: Only reset MMU when necessary

[Sheng Yang]

Only modifying some bits of CR0/CR4 needs paging mode switch.

Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved bits.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/mmu.c              |   17 ++++++++++++++---
 arch/x86/kvm/x86.c              |   14 ++++++++++++--
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ed48904..c8c8a03 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -553,6 +553,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot);
 void kvm_mmu_zap_all(struct kvm *kvm);
 unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm);
 void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages);
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu);
 
 int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3);
 
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 5412185..98abdcf 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2335,6 +2335,19 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
 	}
 }
 
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu)
+{
+	if (!is_paging(vcpu))
+		return;
+	if (is_long_mode(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
+	else if (is_pae(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
+	else
+		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
+}
+EXPORT_SYMBOL_GPL(update_rsvd_bits_mask);
+
 static int paging64_init_context_common(struct kvm_vcpu *vcpu, int level)
 {
 	struct kvm_mmu *context = &vcpu->arch.mmu;
@@ -2400,18 +2413,16 @@ static int init_kvm_tdp_mmu(struct kvm_vcpu *vcpu)
 		context->gva_to_gpa = nonpaging_gva_to_gpa;
 		context->root_level = 0;
 	} else if (is_long_mode(vcpu)) {
-		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT64_ROOT_LEVEL;
 	} else if (is_pae(vcpu)) {
-		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT32E_ROOT_LEVEL;
 	} else {
-		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
 		context->gva_to_gpa = paging32_gva_to_gpa;
 		context->root_level = PT32_ROOT_LEVEL;
 	}
+	update_rsvd_bits_mask(vcpu);
 
 	return 0;
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b59fc67..1c76e08 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -416,6 +416,9 @@ out:
 
 static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 {
+	unsigned long old_cr0 = kvm_read_cr0(vcpu);
+	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE;
+
 	cr0 |= X86_CR0_ET;
 
 #ifdef CONFIG_X86_64
@@ -449,7 +452,8 @@ static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 
 	kvm_x86_ops->set_cr0(vcpu, cr0);
 
-	kvm_mmu_reset_context(vcpu);
+	if ((cr0 ^ old_cr0) & update_bits)
+		kvm_mmu_reset_context(vcpu);
 	return 0;
 }
 
@@ -487,7 +491,8 @@ int __kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 
 	kvm_x86_ops->set_cr4(vcpu, cr4);
 
-	kvm_mmu_reset_context(vcpu);
+	if ((cr4 ^ old_cr4) & pdptr_bits)
+		kvm_mmu_reset_context(vcpu);
 
 	return 0;
 }
@@ -692,6 +697,8 @@ static u32 emulated_msrs[] = {
 
 static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 {
+	u64 old_efer = vcpu->arch.efer;
+
 	if (efer & efer_reserved_bits)
 		return 1;
 
@@ -722,6 +729,9 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
 
+	if ((efer ^ old_efer) & EFER_NX)
+		update_rsvd_bits_mask(vcpu);
+
 	return 0;
 }
 
-- 
1.7.0.1

Re: [PATCH 4/4] VMX: x86: Only reset MMU when necessary

[Marcelo Tosatti]

On Tue, May 11, 2010 at 01:30:07PM +0800, Sheng Yang wrote:
> Only modifying some bits of CR0/CR4 needs paging mode switch.
> 
> Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved bits.
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  arch/x86/include/asm/kvm_host.h |    1 +
>  arch/x86/kvm/mmu.c              |   17 ++++++++++++++---
>  arch/x86/kvm/x86.c              |   14 ++++++++++++--
>  3 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index ed48904..c8c8a03 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -553,6 +553,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot);
>  void kvm_mmu_zap_all(struct kvm *kvm);
>  unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm);
>  void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages);
> +void update_rsvd_bits_mask(struct kvm_vcpu *vcpu);
>  
>  int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3);
>  
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 5412185..98abdcf 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2335,6 +2335,19 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
>  	}
>  }
>  
> +void update_rsvd_bits_mask(struct kvm_vcpu *vcpu)
> +{
> +	if (!is_paging(vcpu))
> +		return;
> +	if (is_long_mode(vcpu))
> +		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
> +	else if (is_pae(vcpu))
> +		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
> +	else
> +		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
> +}
> +EXPORT_SYMBOL_GPL(update_rsvd_bits_mask);
> +
>  static int paging64_init_context_common(struct kvm_vcpu *vcpu, int level)
>  {
>  	struct kvm_mmu *context = &vcpu->arch.mmu;
> @@ -2400,18 +2413,16 @@ static int init_kvm_tdp_mmu(struct kvm_vcpu *vcpu)
>  		context->gva_to_gpa = nonpaging_gva_to_gpa;
>  		context->root_level = 0;
>  	} else if (is_long_mode(vcpu)) {
> -		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
>  		context->gva_to_gpa = paging64_gva_to_gpa;
>  		context->root_level = PT64_ROOT_LEVEL;
>  	} else if (is_pae(vcpu)) {
> -		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
>  		context->gva_to_gpa = paging64_gva_to_gpa;
>  		context->root_level = PT32E_ROOT_LEVEL;
>  	} else {
> -		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
>  		context->gva_to_gpa = paging32_gva_to_gpa;
>  		context->root_level = PT32_ROOT_LEVEL;
>  	}
> +	update_rsvd_bits_mask(vcpu);
>  
>  	return 0;
>  }
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b59fc67..1c76e08 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -416,6 +416,9 @@ out:
>  
>  static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
>  {
> +	unsigned long old_cr0 = kvm_read_cr0(vcpu);
> +	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE;

If PAE paging would be in use following an execution of MOV to CR0 or
MOV to CR4 (see Section 4.1.1) and the instruction is modifying any of
CR0.CD, CR0.NW, CR0.PG, CR4.PAE, CR4.PGE, or CR4.PSE; then the PDPTEs
are loaded from the address in CR3.

If the PDPTRS changed, the mmu must be reloaded.

Re: [PATCH 4/4] VMX: x86: Only reset MMU when necessary

[Sheng Yang]

On Wednesday 12 May 2010 03:36:26 Marcelo Tosatti wrote:
> On Tue, May 11, 2010 at 01:30:07PM +0800, Sheng Yang wrote:
> > Only modifying some bits of CR0/CR4 needs paging mode switch.
> > 
> > Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved
> > bits.
> > 
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> > 
> >  arch/x86/include/asm/kvm_host.h |    1 +
> >  arch/x86/kvm/mmu.c              |   17 ++++++++++++++---
> >  arch/x86/kvm/x86.c              |   14 ++++++++++++--
> >  3 files changed, 27 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h
> > b/arch/x86/include/asm/kvm_host.h index ed48904..c8c8a03 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -553,6 +553,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm
> > *kvm, int slot);
> > 
> >  void kvm_mmu_zap_all(struct kvm *kvm);
> >  unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm);
> >  void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int
> >  kvm_nr_mmu_pages);
> > 
> > +void update_rsvd_bits_mask(struct kvm_vcpu *vcpu);
> > 
> >  int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3);
> > 
> > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > index 5412185..98abdcf 100644
> > --- a/arch/x86/kvm/mmu.c
> > +++ b/arch/x86/kvm/mmu.c
> > @@ -2335,6 +2335,19 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu
> > *vcpu, int level)
> > 
> >  	}
> >  
> >  }
> > 
> > +void update_rsvd_bits_mask(struct kvm_vcpu *vcpu)
> > +{
> > +	if (!is_paging(vcpu))
> > +		return;
> > +	if (is_long_mode(vcpu))
> > +		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
> > +	else if (is_pae(vcpu))
> > +		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
> > +	else
> > +		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
> > +}
> > +EXPORT_SYMBOL_GPL(update_rsvd_bits_mask);
> > +
> > 
> >  static int paging64_init_context_common(struct kvm_vcpu *vcpu, int
> >  level) {
> >  
> >  	struct kvm_mmu *context = &vcpu->arch.mmu;
> > 
> > @@ -2400,18 +2413,16 @@ static int init_kvm_tdp_mmu(struct kvm_vcpu
> > *vcpu)
> > 
> >  		context->gva_to_gpa = nonpaging_gva_to_gpa;
> >  		context->root_level = 0;
> >  	
> >  	} else if (is_long_mode(vcpu)) {
> > 
> > -		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
> > 
> >  		context->gva_to_gpa = paging64_gva_to_gpa;
> >  		context->root_level = PT64_ROOT_LEVEL;
> >  	
> >  	} else if (is_pae(vcpu)) {
> > 
> > -		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
> > 
> >  		context->gva_to_gpa = paging64_gva_to_gpa;
> >  		context->root_level = PT32E_ROOT_LEVEL;
> >  	
> >  	} else {
> > 
> > -		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
> > 
> >  		context->gva_to_gpa = paging32_gva_to_gpa;
> >  		context->root_level = PT32_ROOT_LEVEL;
> >  	
> >  	}
> > 
> > +	update_rsvd_bits_mask(vcpu);
> > 
> >  	return 0;
> >  
> >  }
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index b59fc67..1c76e08 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > 
> > @@ -416,6 +416,9 @@ out:
> >  static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
> >  {
> > 
> > +	unsigned long old_cr0 = kvm_read_cr0(vcpu);
> > +	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE;
> 
> If PAE paging would be in use following an execution of MOV to CR0 or
> MOV to CR4 (see Section 4.1.1) and the instruction is modifying any of
> CR0.CD, CR0.NW, CR0.PG, CR4.PAE, CR4.PGE, or CR4.PSE; then the PDPTEs
> are loaded from the address in CR3.
> 
> If the PDPTRS changed, the mmu must be reloaded.

Yes... Would add CR0.CD and CR0.NW here.

--
regards
Yang, Sheng

[PATCH 4/4] VMX: x86: Only reset MMU when necessary

[Sheng Yang]

Only modifying some bits of CR0/CR4 needs paging mode switch.

Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved bits.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/mmu.c              |   17 ++++++++++++++---
 arch/x86/kvm/x86.c              |   15 +++++++++++++--
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ed48904..c8c8a03 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -553,6 +553,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot);
 void kvm_mmu_zap_all(struct kvm *kvm);
 unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm);
 void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages);
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu);
 
 int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3);
 
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 5412185..98abdcf 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2335,6 +2335,19 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
 	}
 }
 
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu)
+{
+	if (!is_paging(vcpu))
+		return;
+	if (is_long_mode(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
+	else if (is_pae(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
+	else
+		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
+}
+EXPORT_SYMBOL_GPL(update_rsvd_bits_mask);
+
 static int paging64_init_context_common(struct kvm_vcpu *vcpu, int level)
 {
 	struct kvm_mmu *context = &vcpu->arch.mmu;
@@ -2400,18 +2413,16 @@ static int init_kvm_tdp_mmu(struct kvm_vcpu *vcpu)
 		context->gva_to_gpa = nonpaging_gva_to_gpa;
 		context->root_level = 0;
 	} else if (is_long_mode(vcpu)) {
-		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT64_ROOT_LEVEL;
 	} else if (is_pae(vcpu)) {
-		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT32E_ROOT_LEVEL;
 	} else {
-		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
 		context->gva_to_gpa = paging32_gva_to_gpa;
 		context->root_level = PT32_ROOT_LEVEL;
 	}
+	update_rsvd_bits_mask(vcpu);
 
 	return 0;
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b59fc67..971a295 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -416,6 +416,10 @@ out:
 
 static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 {
+	unsigned long old_cr0 = kvm_read_cr0(vcpu);
+	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE |
+				    X86_CR0_CD | X86_CR0_NW;
+
 	cr0 |= X86_CR0_ET;
 
 #ifdef CONFIG_X86_64
@@ -449,7 +453,8 @@ static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 
 	kvm_x86_ops->set_cr0(vcpu, cr0);
 
-	kvm_mmu_reset_context(vcpu);
+	if ((cr0 ^ old_cr0) & update_bits)
+		kvm_mmu_reset_context(vcpu);
 	return 0;
 }
 
@@ -487,7 +492,8 @@ int __kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 
 	kvm_x86_ops->set_cr4(vcpu, cr4);
 
-	kvm_mmu_reset_context(vcpu);
+	if ((cr4 ^ old_cr4) & pdptr_bits)
+		kvm_mmu_reset_context(vcpu);
 
 	return 0;
 }
@@ -692,6 +698,8 @@ static u32 emulated_msrs[] = {
 
 static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 {
+	u64 old_efer = vcpu->arch.efer;
+
 	if (efer & efer_reserved_bits)
 		return 1;
 
@@ -722,6 +730,9 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
 
+	if ((efer ^ old_efer) & EFER_NX)
+		update_rsvd_bits_mask(vcpu);
+
 	return 0;
 }
 
-- 
1.7.0.1

Re: [PATCH 4/4] VMX: x86: Only reset MMU when necessary

[Avi Kivity]

On 05/12/2010 05:09 AM, Sheng Yang wrote:
> Only modifying some bits of CR0/CR4 needs paging mode switch.
>
> Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved bits.
>    

Can you please repost the whole series?  Due to a problem with my 
mailbox I don't have the patches either in my inbox or on kvm@.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[PATCH 1/4] KVM: x86: Check LMA bit before set_efer

[Sheng Yang]

kvm_x86_ops->set_efer() would execute vcpu->arch.efer = efer, so the
checking of LMA bit didn't work.

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cd8a606..2cae460 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -715,11 +715,11 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 			return 1;
 	}
 
-	kvm_x86_ops->set_efer(vcpu, efer);
-
 	efer &= ~EFER_LMA;
 	efer |= vcpu->arch.efer & EFER_LMA;
 
+	kvm_x86_ops->set_efer(vcpu, efer);
+
 	vcpu->arch.efer = efer;
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
-- 
1.7.0.1

[PATCH 2/4] KVM: Clean up duplicate assignment

[Sheng Yang]

mmu.free() already set root_hpa to INVALID_PAGE, no need to do it again in the
destory_kvm_mmu().

kvm_x86_ops->set_cr4() and set_efer() already assign cr4/efer to
vcpu->arch.cr4/efer, no need to do it again later.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/kvm/mmu.c |    5 ++---
 arch/x86/kvm/x86.c |    4 +---
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 95bee9d..5412185 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2450,10 +2450,9 @@ static int init_kvm_mmu(struct kvm_vcpu *vcpu)
 static void destroy_kvm_mmu(struct kvm_vcpu *vcpu)
 {
 	ASSERT(vcpu);
-	if (VALID_PAGE(vcpu->arch.mmu.root_hpa)) {
+	if (VALID_PAGE(vcpu->arch.mmu.root_hpa))
+		/* mmu.free() should set root_hpa = INVALID_PAGE */
 		vcpu->arch.mmu.free(vcpu);
-		vcpu->arch.mmu.root_hpa = INVALID_PAGE;
-	}
 }
 
 int kvm_mmu_reset_context(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2cae460..764f89b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -486,7 +486,7 @@ int __kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 		return 1;
 
 	kvm_x86_ops->set_cr4(vcpu, cr4);
-	vcpu->arch.cr4 = cr4;
+
 	kvm_mmu_reset_context(vcpu);
 
 	return 0;
@@ -720,8 +720,6 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 
 	kvm_x86_ops->set_efer(vcpu, efer);
 
-	vcpu->arch.efer = efer;
-
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
 	kvm_mmu_reset_context(vcpu);
 
-- 
1.7.0.1

[PATCH 3/4] KVM: x86: Remove kvm_mmu_reset_context() in kvm_set_efer()

[Sheng Yang]

Modify EFER won't result in mode switch directly. After EFER.LME set, the
following set CR0.PG would result in mode switch to IA32e. And the later
action already covered by kvm_set_cr0().

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 764f89b..b59fc67 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -721,7 +721,6 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	kvm_x86_ops->set_efer(vcpu, efer);
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
-	kvm_mmu_reset_context(vcpu);
 
 	return 0;
 }
-- 
1.7.0.1

[PATCH 4/4] VMX: x86: Only reset MMU when necessary

[Sheng Yang]

Only modifying some bits of CR0/CR4 needs paging mode switch.

Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved bits.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/mmu.c              |   17 ++++++++++++++---
 arch/x86/kvm/x86.c              |   15 +++++++++++++--
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ed48904..c8c8a03 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -553,6 +553,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot);
 void kvm_mmu_zap_all(struct kvm *kvm);
 unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm);
 void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages);
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu);
 
 int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3);
 
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 5412185..98abdcf 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2335,6 +2335,19 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
 	}
 }
 
+void update_rsvd_bits_mask(struct kvm_vcpu *vcpu)
+{
+	if (!is_paging(vcpu))
+		return;
+	if (is_long_mode(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
+	else if (is_pae(vcpu))
+		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
+	else
+		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
+}
+EXPORT_SYMBOL_GPL(update_rsvd_bits_mask);
+
 static int paging64_init_context_common(struct kvm_vcpu *vcpu, int level)
 {
 	struct kvm_mmu *context = &vcpu->arch.mmu;
@@ -2400,18 +2413,16 @@ static int init_kvm_tdp_mmu(struct kvm_vcpu *vcpu)
 		context->gva_to_gpa = nonpaging_gva_to_gpa;
 		context->root_level = 0;
 	} else if (is_long_mode(vcpu)) {
-		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT64_ROOT_LEVEL;
 	} else if (is_pae(vcpu)) {
-		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
 		context->gva_to_gpa = paging64_gva_to_gpa;
 		context->root_level = PT32E_ROOT_LEVEL;
 	} else {
-		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
 		context->gva_to_gpa = paging32_gva_to_gpa;
 		context->root_level = PT32_ROOT_LEVEL;
 	}
+	update_rsvd_bits_mask(vcpu);
 
 	return 0;
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b59fc67..971a295 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -416,6 +416,10 @@ out:
 
 static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 {
+	unsigned long old_cr0 = kvm_read_cr0(vcpu);
+	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE |
+				    X86_CR0_CD | X86_CR0_NW;
+
 	cr0 |= X86_CR0_ET;
 
 #ifdef CONFIG_X86_64
@@ -449,7 +453,8 @@ static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 
 	kvm_x86_ops->set_cr0(vcpu, cr0);
 
-	kvm_mmu_reset_context(vcpu);
+	if ((cr0 ^ old_cr0) & update_bits)
+		kvm_mmu_reset_context(vcpu);
 	return 0;
 }
 
@@ -487,7 +492,8 @@ int __kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 
 	kvm_x86_ops->set_cr4(vcpu, cr4);
 
-	kvm_mmu_reset_context(vcpu);
+	if ((cr4 ^ old_cr4) & pdptr_bits)
+		kvm_mmu_reset_context(vcpu);
 
 	return 0;
 }
@@ -692,6 +698,8 @@ static u32 emulated_msrs[] = {
 
 static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 {
+	u64 old_efer = vcpu->arch.efer;
+
 	if (efer & efer_reserved_bits)
 		return 1;
 
@@ -722,6 +730,9 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 
 	vcpu->arch.mmu.base_role.nxe = (efer & EFER_NX) && !tdp_enabled;
 
+	if ((efer ^ old_efer) & EFER_NX)
+		update_rsvd_bits_mask(vcpu);
+
 	return 0;
 }
 
-- 
1.7.0.1

Re: [PATCH 4/4] VMX: x86: Only reset MMU when necessary

[Avi Kivity]

On 05/12/2010 09:33 AM, Sheng Yang wrote:
> Only modifying some bits of CR0/CR4 needs paging mode switch.
>
> Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved bits.
>
>
> @@ -2335,6 +2335,19 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
>   	}
>   }
>
> +void update_rsvd_bits_mask(struct kvm_vcpu *vcpu)
> +{
> +	if (!is_paging(vcpu))
> +		return;
> +	if (is_long_mode(vcpu))
> +		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
> +	else if (is_pae(vcpu))
> +		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
> +	else
> +		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
> +}
> +EXPORT_SYMBOL_GPL(update_rsvd_bits_mask);
>    

Needs a kvm_ prefix if made a global symbol.  But isn't nx switching 
rare enough so we can reload the mmu completely?

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b59fc67..971a295 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -416,6 +416,10 @@ out:
>
>   static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
>   {
> +	unsigned long old_cr0 = kvm_read_cr0(vcpu);
> +	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE |
> +				    X86_CR0_CD | X86_CR0_NW;
>    

PE doesn't affect paging, CD, NW don't either?

What about WP?

> +
>   	cr0 |= X86_CR0_ET;
>
>   #ifdef CONFIG_X86_64
> @@ -449,7 +453,8 @@ static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
>
>   	kvm_x86_ops->set_cr0(vcpu, cr0);
>
> -	kvm_mmu_reset_context(vcpu);
> +	if ((cr0 ^ old_cr0)&  update_bits)
> +		kvm_mmu_reset_context(vcpu);
>   	return 0;
>   }
>
>
> @@ -692,6 +698,8 @@ static u32 emulated_msrs[] = {
>
>   static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
>   {
> +	u64 old_efer = vcpu->arch.efer;
> +
>   	if (efer&  efer_reserved_bits)
>   		return 1;
>
> @@ -722,6 +730,9 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
>
>   	vcpu->arch.mmu.base_role.nxe = (efer&  EFER_NX)&&  !tdp_enabled;
>
> +	if ((efer ^ old_efer)&  EFER_NX)
> +		update_rsvd_bits_mask(vcpu);
> +
>   	return 0;
>   }
>    

I think it's fine to reset the entire mmu context here, most guests 
won't toggle nx all the time.  But it needs to be in patch 3, otherwise 
we have a regression between 3 and 4.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 4/4] VMX: x86: Only reset MMU when necessary

[Sheng Yang]

On Wednesday 12 May 2010 14:59:14 Avi Kivity wrote:
> On 05/12/2010 09:33 AM, Sheng Yang wrote:
> > Only modifying some bits of CR0/CR4 needs paging mode switch.
> > 
> > Add update_rsvd_bits_mask() to address EFER.NX bit updating for reserved
> > bits.
> > 
> > 
> > @@ -2335,6 +2335,19 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu
> > *vcpu, int level)
> > 
> >   	}
> >   
> >   }
> > 
> > +void update_rsvd_bits_mask(struct kvm_vcpu *vcpu)
> > +{
> > +	if (!is_paging(vcpu))
> > +		return;
> > +	if (is_long_mode(vcpu))
> > +		reset_rsvds_bits_mask(vcpu, PT64_ROOT_LEVEL);
> > +	else if (is_pae(vcpu))
> > +		reset_rsvds_bits_mask(vcpu, PT32E_ROOT_LEVEL);
> > +	else
> > +		reset_rsvds_bits_mask(vcpu, PT32_ROOT_LEVEL);
> > +}
> > +EXPORT_SYMBOL_GPL(update_rsvd_bits_mask);
> 
> Needs a kvm_ prefix if made a global symbol.  But isn't nx switching
> rare enough so we can reload the mmu completely?

Yes, it should be rare enough. I am OK with keeping it, though "reload MMU" seems 
a little misleading meaning here.
 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index b59fc67..971a295 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > 
> > @@ -416,6 +416,10 @@ out:
> >   static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
> >   {
> > 
> > +	unsigned long old_cr0 = kvm_read_cr0(vcpu);
> > +	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE |
> > +				    X86_CR0_CD | X86_CR0_NW;
> 
> PE doesn't affect paging, CD, NW don't either?

Yes, PE can't affect alone.

Marcelo has commented on CD/NW, because we need to reload pdptrs if they changed, 
then we need to reload MMU.
> 
> What about WP?

How WP would affect?

> > +
> > 
> >   	cr0 |= X86_CR0_ET;
> >   
> >   #ifdef CONFIG_X86_64
> > 
> > @@ -449,7 +453,8 @@ static int __kvm_set_cr0(struct kvm_vcpu *vcpu,
> > unsigned long cr0)
> > 
> >   	kvm_x86_ops->set_cr0(vcpu, cr0);
> > 
> > -	kvm_mmu_reset_context(vcpu);
> > +	if ((cr0 ^ old_cr0)&  update_bits)
> > +		kvm_mmu_reset_context(vcpu);
> > 
> >   	return 0;
> >   
> >   }
> > 
> > @@ -692,6 +698,8 @@ static u32 emulated_msrs[] = {
> > 
> >   static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
> >   {
> > 
> > +	u64 old_efer = vcpu->arch.efer;
> > +
> > 
> >   	if (efer&  efer_reserved_bits)
> >   	
> >   		return 1;
> > 
> > @@ -722,6 +730,9 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
> > 
> >   	vcpu->arch.mmu.base_role.nxe = (efer&  EFER_NX)&&  !tdp_enabled;
> > 
> > +	if ((efer ^ old_efer)&  EFER_NX)
> > +		update_rsvd_bits_mask(vcpu);
> > +
> > 
> >   	return 0;
> >   
> >   }
> 
> I think it's fine to reset the entire mmu context here, most guests
> won't toggle nx all the time.  But it needs to be in patch 3, otherwise
> we have a regression between 3 and 4.

OK. Would drop patch 3 and keep mmu reset if you like...
--
regards
Yang, Sheng

Re: [PATCH 4/4] VMX: x86: Only reset MMU when necessary

[Avi Kivity]

On 05/12/2010 10:31 AM, Sheng Yang wrote:
>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index b59fc67..971a295 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>>
>>> @@ -416,6 +416,10 @@ out:
>>>    static int __kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
>>>    {
>>>
>>> +	unsigned long old_cr0 = kvm_read_cr0(vcpu);
>>> +	unsigned long update_bits = X86_CR0_PG | X86_CR0_PE |
>>> +				    X86_CR0_CD | X86_CR0_NW;
>>>        
>> PE doesn't affect paging, CD, NW don't either?
>>      
> Yes, PE can't affect alone.
>
> Marcelo has commented on CD/NW, because we need to reload pdptrs if they changed,
> then we need to reload MMU.
>    

Ah, correct.

>> What about WP?
>>      
> How WP would affect?
>    

If cr0.wp=0 then we can have a pte with gpte.rw=0 but spte.rw=1 (since 
the guest always runs with cr0.wp=1).  So we need to reload the mmu to 
switch page tables.

This won't work now, I'll post a patch adding cr0.wp to sp->role.  But 
please add cr0.wp to the set of bits requiring reload so we won't have a 
regression.

>>> @@ -722,6 +730,9 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
>>>
>>>    	vcpu->arch.mmu.base_role.nxe = (efer&   EFER_NX)&&   !tdp_enabled;
>>>
>>> +	if ((efer ^ old_efer)&   EFER_NX)
>>> +		update_rsvd_bits_mask(vcpu);
>>> +
>>>
>>>    	return 0;
>>>
>>>    }
>>>        
>> I think it's fine to reset the entire mmu context here, most guests
>> won't toggle nx all the time.  But it needs to be in patch 3, otherwise
>> we have a regression between 3 and 4.
>>      
> OK. Would drop patch 3 and keep mmu reset if you like...
>    

Yes please.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.