Re: [kvm-devel] [PATCH 8/8] SVM: add support for Nested Paging

[Avi Kivity <avi@qumranet.com>]

Joerg Roedel wrote:
> This patch contains the SVM architecture dependent changes for KVM to enable
> support for the Nested Paging feature of AMD Barcelona and Phenom processors.
>  
> +#ifdef CONFIG_X86_64
> +static bool npt_enabled = true;
> +#else
>  static bool npt_enabled = false;
> +#endif
>   

I think that i386 + pae can also support npt, with no changes, no?

So we should check CONFIG_X86_PAE, not X86_64.

> +
> +	if (npt_enabled) {
> +		/* Setup VMCB for Nested Paging */
> +		control->nested_ctl = 1;
> +		control->intercept_exceptions &= ~(1 << PF_VECTOR);
> +		control->intercept_cr_read &= ~(INTERCEPT_CR0_MASK|
> +						INTERCEPT_CR3_MASK|
> +						INTERCEPT_CR4_MASK);
> +		control->intercept_cr_write &= ~(INTERCEPT_CR0_MASK|
> +						 INTERCEPT_CR3_MASK|
> +						 INTERCEPT_CR4_MASK);
>   

What happens to lazy fpu if we don't trap cr0 changes?

Perhaps it's worth disabling lazy fpu with npt.

>  static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
> @@ -789,6 +812,15 @@ static void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
> +	if (npt_enabled) {
> +		/*
> +		 * re-enable caching here because the QEMU bios
> +		 * does not do it - this results in some delay at
> +		 * reboot
> +		 */
> +		cr0 &= ~(X86_CR0_CD | X86_CR0_NW);
> +		goto set;
> +	}
>  #ifdef CONFIG_X86_64
>  	if (vcpu->arch.shadow_efer & EFER_LME) {
>  		if (!is_paging(vcpu) && (cr0 & X86_CR0_PG)) {
> @@ -812,13 +844,16 @@ static void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
>  	cr0 &= ~(X86_CR0_CD | X86_CR0_NW);
>  	if (!vcpu->fpu_active)
>  		cr0 |= X86_CR0_TS;
> +set:
>  	svm->vmcb->save.cr0 = cr0;
>  }
>   

You could move 'cr0 &= ~(X86_CR0_CD | X86_CR0_NW);' from three lines 
above the 'set' label downwards, and avoid it in the if () above.
>  static int handle_exit(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	u32 exit_code = svm->vmcb->control.exit_code;
>  
> +	if (npt_enabled) {
> +		int mmu_reload = 0;
>   

blank line

> +		if (((vcpu->arch.cr0 ^ svm->vmcb->save.cr0) & X86_CR0_PG)
> +		    || ((vcpu->arch.cr4 ^ svm->vmcb->save.cr4) &
> +			(X86_CR4_PGE|X86_CR4_PAE)))
> +			mmu_reload = 1;
> +		vcpu->arch.cr0 = svm->vmcb->save.cr0;
> +		vcpu->arch.cr4 = svm->vmcb->save.cr4;
> +		vcpu->arch.cr3 = svm->vmcb->save.cr3;
> +		if (mmu_reload) {
> +			kvm_mmu_reset_context(vcpu);
> +			kvm_mmu_load(vcpu);
> +		}
> +		if (is_pae(vcpu) && !is_long_mode(vcpu))
> +			load_pdptrs(vcpu, vcpu->arch.cr3);
>   

load_pdptrs() may fail; you need to check that.


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