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

Avi Kivity
2008-01-27T08:52:30Z

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