[PATCH 0/7] Implement Shadow VMCB for Nested SVM

[PATCH 0/7] Implement Shadow VMCB for Nested SVM

[Joerg Roedel]

Hi Avi, Marcelo,

here is a patch-set that implements a shadow-vmcb for nested SVM. The
shadow-vmcb is always used when the L2-guest is invoked. This saves some
memory copys in the nested vmrun and vmexit paths because the contents
of the host-vmcb do not need to be saved/restored anymore.

This patch-set also prepares nested-svm for the emulation of clean-bits.

Regards,

	Joerg

Diffstat:

 arch/x86/kvm/svm.c |  248 ++++++++++++++++++++++++----------------------------
 1 files changed, 114 insertions(+), 134 deletions(-)

[PATCH 1/7] KVM: SVM: Keep seperate pointer to host-vmcb

[Joerg Roedel]

From: Joerg Roedel <joro@8bytes.org>

This patch introduces a new pointer which always points to
the VMCB used for running the L1 guest.

Signed-off-by: Joerg Roedel <joro@8bytes.org>
---
 arch/x86/kvm/svm.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 475d1c9..3d5990f 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -113,7 +113,9 @@ static u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
 struct vcpu_svm {
 	struct kvm_vcpu vcpu;
 	struct vmcb *vmcb;
+	struct vmcb *host_vmcb;
 	unsigned long vmcb_pa;
+	unsigned long host_vmcb_pa;
 	struct svm_cpu_data *svm_data;
 	uint64_t asid_generation;
 	uint64_t sysenter_esp;
@@ -1171,9 +1173,11 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
 	svm->nested.msrpm = page_address(nested_msrpm_pages);
 	svm_vcpu_init_msrpm(svm->nested.msrpm);
 
-	svm->vmcb = page_address(page);
+	svm->host_vmcb = page_address(page);
+	svm->vmcb = svm->host_vmcb;
 	clear_page(svm->vmcb);
 	svm->vmcb_pa = page_to_pfn(page) << PAGE_SHIFT;
+	svm->host_vmcb_pa = page_to_pfn(page) << PAGE_SHIFT;
 	svm->asid_generation = 0;
 	init_vmcb(svm);
 	kvm_write_tsc(&svm->vcpu, 0);
-- 
1.7.4.1

[PATCH 2/7] KVM: SVM: Use host_vmcb_pa for vmload and vmsave

[Joerg Roedel]

This saves copying over the vmload/vmsave switched part from
the host to the guest vmcb later.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kvm/svm.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 3d5990f..dc703ac 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3704,9 +3704,13 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
 
 		/* Enter guest mode */
 		"push %%"R"ax \n\t"
-		"mov %c[vmcb](%[svm]), %%"R"ax \n\t"
+		"mov %c[host_vmcb](%[svm]), %%"R"ax \n\t"
 		__ex(SVM_VMLOAD) "\n\t"
+		"mov (%%"R"sp), %%"R"ax\n\t"
+		"mov %c[vmcb](%[svm]), %%"R"ax \n\t"
 		__ex(SVM_VMRUN) "\n\t"
+		"mov (%%"R"sp), %%"R"ax\n\t"
+		"mov %c[host_vmcb](%[svm]), %%"R"ax \n\t"
 		__ex(SVM_VMSAVE) "\n\t"
 		"pop %%"R"ax \n\t"
 
@@ -3731,6 +3735,7 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
 		:
 		: [svm]"a"(svm),
 		  [vmcb]"i"(offsetof(struct vcpu_svm, vmcb_pa)),
+		  [host_vmcb]"i"(offsetof(struct vcpu_svm, host_vmcb_pa)),
 		  [rbx]"i"(offsetof(struct vcpu_svm, vcpu.arch.regs[VCPU_REGS_RBX])),
 		  [rcx]"i"(offsetof(struct vcpu_svm, vcpu.arch.regs[VCPU_REGS_RCX])),
 		  [rdx]"i"(offsetof(struct vcpu_svm, vcpu.arch.regs[VCPU_REGS_RDX])),
-- 
1.7.4.1

[PATCH 3/7] KVM: SVM: Reorder nested_svm_vmrun

[Joerg Roedel]

From: Joerg Roedel <joro@8bytes.org>

Reorder the function a little bit to move interrupt related
code together and the tlb-flush from the middle of the
function towards the end.

Signed-off-by: Joerg Roedel <joro@8bytes.org>
---
 arch/x86/kvm/svm.c |   31 ++++++++++++++++---------------
 1 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index dc703ac..f81e35e 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2411,6 +2411,20 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 	else
 		svm->vcpu.arch.hflags &= ~HF_HIF_MASK;
 
+	if (nested_vmcb->control.int_ctl & V_INTR_MASKING_MASK)
+		svm->vcpu.arch.hflags |= HF_VINTR_MASK;
+	else
+		svm->vcpu.arch.hflags &= ~HF_VINTR_MASK;
+
+	if (svm->vcpu.arch.hflags & HF_VINTR_MASK) {
+		/* We only want the cr8 intercept bits of the guest */
+		clr_cr_intercept(svm, INTERCEPT_CR8_READ);
+		clr_cr_intercept(svm, INTERCEPT_CR8_WRITE);
+	}
+
+	/* We don't want to see VMMCALLs from a nested guest */
+	clr_intercept(svm, INTERCEPT_VMMCALL);
+
 	if (nested_vmcb->control.nested_ctl) {
 		kvm_mmu_unload(&svm->vcpu);
 		svm->nested.nested_cr3 = nested_vmcb->control.nested_cr3;
@@ -2459,22 +2473,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 	svm->nested.intercept_exceptions = nested_vmcb->control.intercept_exceptions;
 	svm->nested.intercept            = nested_vmcb->control.intercept;
 
-	svm_flush_tlb(&svm->vcpu);
 	svm->vmcb->control.int_ctl = nested_vmcb->control.int_ctl | V_INTR_MASKING_MASK;
-	if (nested_vmcb->control.int_ctl & V_INTR_MASKING_MASK)
-		svm->vcpu.arch.hflags |= HF_VINTR_MASK;
-	else
-		svm->vcpu.arch.hflags &= ~HF_VINTR_MASK;
-
-	if (svm->vcpu.arch.hflags & HF_VINTR_MASK) {
-		/* We only want the cr8 intercept bits of the guest */
-		clr_cr_intercept(svm, INTERCEPT_CR8_READ);
-		clr_cr_intercept(svm, INTERCEPT_CR8_WRITE);
-	}
-
-	/* We don't want to see VMMCALLs from a nested guest */
-	clr_intercept(svm, INTERCEPT_VMMCALL);
-
 	svm->vmcb->control.lbr_ctl = nested_vmcb->control.lbr_ctl;
 	svm->vmcb->control.int_vector = nested_vmcb->control.int_vector;
 	svm->vmcb->control.int_state = nested_vmcb->control.int_state;
@@ -2484,6 +2483,8 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 
 	nested_svm_unmap(page);
 
+	svm_flush_tlb(&svm->vcpu);
+
 	/* Enter Guest-Mode */
 	enter_guest_mode(&svm->vcpu);
 
-- 
1.7.4.1

[PATCH 4/7] KVM: SVM: Use seperate VMCB for L2 guests

[Joerg Roedel]

From: Joerg Roedel <joro@8bytes.org>

Move torwards emulation of VMCB-clean-bits by using a
seperate VMCB when running L2 guests.

Signed-off-by: Joerg Roedel <joro@8bytes.org>
---
 arch/x86/kvm/svm.c |   43 ++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index f81e35e..6dacf59 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -105,6 +105,8 @@ struct nested_state {
 
 	/* Nested Paging related state */
 	u64 nested_cr3;
+
+	struct vmcb *n_vmcb;
 };
 
 #define MSRPM_OFFSETS	16
@@ -974,6 +976,26 @@ static u64 svm_compute_tsc_offset(struct kvm_vcpu *vcpu, u64 target_tsc)
 	return target_tsc - tsc;
 }
 
+static bool init_nested_vmcb(struct vcpu_svm *svm)
+{
+	struct vmcb_control_area *hc, *nc;
+
+	svm->nested.n_vmcb = (void *)get_zeroed_page(GFP_KERNEL);
+	if (svm->nested.n_vmcb == NULL)
+		return false;
+
+	nc = &svm->nested.n_vmcb->control;
+	hc = &svm->host_vmcb->control;
+
+	nc->iopm_base_pa		= hc->iopm_base_pa;
+	nc->msrpm_base_pa		= hc->msrpm_base_pa;
+	nc->nested_ctl			= hc->nested_ctl;
+	nc->pause_filter_count		= hc->pause_filter_count;
+	svm->nested.n_vmcb->save.g_pat	= svm->host_vmcb->save.g_pat;
+
+	return true;
+}
+
 static void init_vmcb(struct vcpu_svm *svm)
 {
 	struct vmcb_control_area *control = &svm->vmcb->control;
@@ -1212,6 +1234,8 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
+	if (svm->nested.n_vmcb != NULL)
+		__free_page(virt_to_page(svm->nested.n_vmcb));
 	__free_page(pfn_to_page(svm->vmcb_pa >> PAGE_SHIFT));
 	__free_pages(virt_to_page(svm->msrpm), MSRPM_ALLOC_ORDER);
 	__free_page(virt_to_page(svm->nested.hsave));
@@ -2179,7 +2203,7 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
 {
 	struct vmcb *nested_vmcb;
 	struct vmcb *hsave = svm->nested.hsave;
-	struct vmcb *vmcb = svm->vmcb;
+	struct vmcb *vmcb = svm->nested.n_vmcb;
 	struct page *page;
 
 	trace_kvm_nested_vmexit_inject(vmcb->control.exit_code,
@@ -2252,8 +2276,12 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
 	if (!(svm->vcpu.arch.hflags & HF_VINTR_MASK))
 		nested_vmcb->control.int_ctl &= ~V_INTR_MASKING_MASK;
 
+	/* Switch VMCB back to host */
+	svm->vmcb = svm->host_vmcb;
+	svm->vmcb_pa = __pa(svm->host_vmcb);
+
 	/* Restore the original control entries */
-	copy_vmcb_control_area(vmcb, hsave);
+	copy_vmcb_control_area(svm->host_vmcb, hsave);
 
 	kvm_clear_exception_queue(&svm->vcpu);
 	kvm_clear_interrupt_queue(&svm->vcpu);
@@ -2431,6 +2459,10 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 		nested_svm_init_mmu_context(&svm->vcpu);
 	}
 
+	/* Switch VMCB */
+	svm->vmcb    = svm->nested.n_vmcb;
+	svm->vmcb_pa = __pa(svm->nested.n_vmcb);
+
 	/* Load the nested guest state */
 	svm->vmcb->save.es = nested_vmcb->save.es;
 	svm->vmcb->save.cs = nested_vmcb->save.cs;
@@ -2477,7 +2509,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 	svm->vmcb->control.lbr_ctl = nested_vmcb->control.lbr_ctl;
 	svm->vmcb->control.int_vector = nested_vmcb->control.int_vector;
 	svm->vmcb->control.int_state = nested_vmcb->control.int_state;
-	svm->vmcb->control.tsc_offset += nested_vmcb->control.tsc_offset;
+	svm->vmcb->control.tsc_offset = svm->host_vmcb->control.tsc_offset + nested_vmcb->control.tsc_offset;
 	svm->vmcb->control.event_inj = nested_vmcb->control.event_inj;
 	svm->vmcb->control.event_inj_err = nested_vmcb->control.event_inj_err;
 
@@ -2566,6 +2598,11 @@ static int vmrun_interception(struct vcpu_svm *svm)
 	if (nested_svm_check_permissions(svm))
 		return 1;
 
+	if (unlikely(svm->nested.n_vmcb == NULL)) {
+		if (!init_nested_vmcb(svm))
+			goto failed;
+	}
+
 	/* Save rip after vmrun instruction */
 	kvm_rip_write(&svm->vcpu, kvm_rip_read(&svm->vcpu) + 3);
 
-- 
1.7.4.1

[PATCH 5/7] KVM: SVM: Remove nested.hsave state

[Joerg Roedel]

From: Joerg Roedel <joro@8bytes.org>

All state is keept in svm->host_vmcb so the hsave is not
necessary anymore, so remote it.

Signed-off-by: Joerg Roedel <joro@8bytes.org>
---
 arch/x86/kvm/svm.c |  151 ++++++++++++++--------------------------------------
 1 files changed, 41 insertions(+), 110 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 6dacf59..f2cca2c 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -82,7 +82,6 @@ static const u32 host_save_user_msrs[] = {
 struct kvm_vcpu;
 
 struct nested_state {
-	struct vmcb *hsave;
 	u64 hsave_msr;
 	u64 vm_cr_msr;
 	u64 vmcb;
@@ -247,8 +246,8 @@ static void recalc_intercepts(struct vcpu_svm *svm)
 	if (!is_guest_mode(&svm->vcpu))
 		return;
 
-	c = &svm->vmcb->control;
-	h = &svm->nested.hsave->control;
+	c = &svm->nested.n_vmcb->control;
+	h = &svm->host_vmcb->control;
 	g = &svm->nested;
 
 	c->intercept_cr = h->intercept_cr | g->intercept_cr;
@@ -257,17 +256,9 @@ static void recalc_intercepts(struct vcpu_svm *svm)
 	c->intercept = h->intercept | g->intercept;
 }
 
-static inline struct vmcb *get_host_vmcb(struct vcpu_svm *svm)
-{
-	if (is_guest_mode(&svm->vcpu))
-		return svm->nested.hsave;
-	else
-		return svm->vmcb;
-}
-
 static inline void set_cr_intercept(struct vcpu_svm *svm, int bit)
 {
-	struct vmcb *vmcb = get_host_vmcb(svm);
+	struct vmcb *vmcb = svm->host_vmcb;
 
 	vmcb->control.intercept_cr |= (1U << bit);
 
@@ -276,7 +267,7 @@ static inline void set_cr_intercept(struct vcpu_svm *svm, int bit)
 
 static inline void clr_cr_intercept(struct vcpu_svm *svm, int bit)
 {
-	struct vmcb *vmcb = get_host_vmcb(svm);
+	struct vmcb *vmcb = svm->host_vmcb;
 
 	vmcb->control.intercept_cr &= ~(1U << bit);
 
@@ -285,14 +276,14 @@ static inline void clr_cr_intercept(struct vcpu_svm *svm, int bit)
 
 static inline bool is_cr_intercept(struct vcpu_svm *svm, int bit)
 {
-	struct vmcb *vmcb = get_host_vmcb(svm);
+	struct vmcb *vmcb = svm->host_vmcb;
 
 	return vmcb->control.intercept_cr & (1U << bit);
 }
 
 static inline void set_dr_intercept(struct vcpu_svm *svm, int bit)
 {
-	struct vmcb *vmcb = get_host_vmcb(svm);
+	struct vmcb *vmcb = svm->host_vmcb;
 
 	vmcb->control.intercept_dr |= (1U << bit);
 
@@ -301,7 +292,7 @@ static inline void set_dr_intercept(struct vcpu_svm *svm, int bit)
 
 static inline void clr_dr_intercept(struct vcpu_svm *svm, int bit)
 {
-	struct vmcb *vmcb = get_host_vmcb(svm);
+	struct vmcb *vmcb = svm->host_vmcb;
 
 	vmcb->control.intercept_dr &= ~(1U << bit);
 
@@ -310,7 +301,7 @@ static inline void clr_dr_intercept(struct vcpu_svm *svm, int bit)
 
 static inline void set_exception_intercept(struct vcpu_svm *svm, int bit)
 {
-	struct vmcb *vmcb = get_host_vmcb(svm);
+	struct vmcb *vmcb = svm->host_vmcb;
 
 	vmcb->control.intercept_exceptions |= (1U << bit);
 
@@ -319,7 +310,7 @@ static inline void set_exception_intercept(struct vcpu_svm *svm, int bit)
 
 static inline void clr_exception_intercept(struct vcpu_svm *svm, int bit)
 {
-	struct vmcb *vmcb = get_host_vmcb(svm);
+	struct vmcb *vmcb = svm->host_vmcb;
 
 	vmcb->control.intercept_exceptions &= ~(1U << bit);
 
@@ -328,7 +319,7 @@ static inline void clr_exception_intercept(struct vcpu_svm *svm, int bit)
 
 static inline void set_intercept(struct vcpu_svm *svm, int bit)
 {
-	struct vmcb *vmcb = get_host_vmcb(svm);
+	struct vmcb *vmcb = svm->host_vmcb;
 
 	vmcb->control.intercept |= (1ULL << bit);
 
@@ -337,7 +328,7 @@ static inline void set_intercept(struct vcpu_svm *svm, int bit)
 
 static inline void clr_intercept(struct vcpu_svm *svm, int bit)
 {
-	struct vmcb *vmcb = get_host_vmcb(svm);
+	struct vmcb *vmcb = svm->host_vmcb;
 
 	vmcb->control.intercept &= ~(1ULL << bit);
 
@@ -947,9 +938,9 @@ static void svm_write_tsc_offset(struct kvm_vcpu *vcpu, u64 offset)
 	u64 g_tsc_offset = 0;
 
 	if (is_guest_mode(vcpu)) {
-		g_tsc_offset = svm->vmcb->control.tsc_offset -
-			       svm->nested.hsave->control.tsc_offset;
-		svm->nested.hsave->control.tsc_offset = offset;
+		g_tsc_offset = svm->host_vmcb->control.tsc_offset -
+			       svm->nested.n_vmcb->control.tsc_offset;
+		svm->nested.n_vmcb->control.tsc_offset = offset;
 	}
 
 	svm->vmcb->control.tsc_offset = offset + g_tsc_offset;
@@ -963,7 +954,7 @@ static void svm_adjust_tsc_offset(struct kvm_vcpu *vcpu, s64 adjustment)
 
 	svm->vmcb->control.tsc_offset += adjustment;
 	if (is_guest_mode(vcpu))
-		svm->nested.hsave->control.tsc_offset += adjustment;
+		svm->nested.n_vmcb->control.tsc_offset += adjustment;
 	mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
 }
 
@@ -1154,7 +1145,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
 	struct vcpu_svm *svm;
 	struct page *page;
 	struct page *msrpm_pages;
-	struct page *hsave_page;
 	struct page *nested_msrpm_pages;
 	int err;
 
@@ -1183,12 +1173,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
 	if (!nested_msrpm_pages)
 		goto free_page2;
 
-	hsave_page = alloc_page(GFP_KERNEL);
-	if (!hsave_page)
-		goto free_page3;
-
-	svm->nested.hsave = page_address(hsave_page);
-
 	svm->msrpm = page_address(msrpm_pages);
 	svm_vcpu_init_msrpm(svm->msrpm);
 
@@ -1206,7 +1190,7 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
 
 	err = fx_init(&svm->vcpu);
 	if (err)
-		goto free_page4;
+		goto free_page3;
 
 	svm->vcpu.arch.apic_base = 0xfee00000 | MSR_IA32_APICBASE_ENABLE;
 	if (kvm_vcpu_is_bsp(&svm->vcpu))
@@ -1214,8 +1198,6 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
 
 	return &svm->vcpu;
 
-free_page4:
-	__free_page(hsave_page);
 free_page3:
 	__free_pages(nested_msrpm_pages, MSRPM_ALLOC_ORDER);
 free_page2:
@@ -1238,7 +1220,6 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
 		__free_page(virt_to_page(svm->nested.n_vmcb));
 	__free_page(pfn_to_page(svm->vmcb_pa >> PAGE_SHIFT));
 	__free_pages(virt_to_page(svm->msrpm), MSRPM_ALLOC_ORDER);
-	__free_page(virt_to_page(svm->nested.hsave));
 	__free_pages(virt_to_page(svm->nested.msrpm), MSRPM_ALLOC_ORDER);
 	kvm_vcpu_uninit(vcpu);
 	kmem_cache_free(kvm_vcpu_cache, svm);
@@ -2169,40 +2150,9 @@ static int nested_svm_exit_handled(struct vcpu_svm *svm)
 	return vmexit;
 }
 
-static inline void copy_vmcb_control_area(struct vmcb *dst_vmcb, struct vmcb *from_vmcb)
-{
-	struct vmcb_control_area *dst  = &dst_vmcb->control;
-	struct vmcb_control_area *from = &from_vmcb->control;
-
-	dst->intercept_cr         = from->intercept_cr;
-	dst->intercept_dr         = from->intercept_dr;
-	dst->intercept_exceptions = from->intercept_exceptions;
-	dst->intercept            = from->intercept;
-	dst->iopm_base_pa         = from->iopm_base_pa;
-	dst->msrpm_base_pa        = from->msrpm_base_pa;
-	dst->tsc_offset           = from->tsc_offset;
-	dst->asid                 = from->asid;
-	dst->tlb_ctl              = from->tlb_ctl;
-	dst->int_ctl              = from->int_ctl;
-	dst->int_vector           = from->int_vector;
-	dst->int_state            = from->int_state;
-	dst->exit_code            = from->exit_code;
-	dst->exit_code_hi         = from->exit_code_hi;
-	dst->exit_info_1          = from->exit_info_1;
-	dst->exit_info_2          = from->exit_info_2;
-	dst->exit_int_info        = from->exit_int_info;
-	dst->exit_int_info_err    = from->exit_int_info_err;
-	dst->nested_ctl           = from->nested_ctl;
-	dst->event_inj            = from->event_inj;
-	dst->event_inj_err        = from->event_inj_err;
-	dst->nested_cr3           = from->nested_cr3;
-	dst->lbr_ctl              = from->lbr_ctl;
-}
-
 static int nested_svm_vmexit(struct vcpu_svm *svm)
 {
 	struct vmcb *nested_vmcb;
-	struct vmcb *hsave = svm->nested.hsave;
 	struct vmcb *vmcb = svm->nested.n_vmcb;
 	struct page *page;
 
@@ -2280,38 +2230,30 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
 	svm->vmcb = svm->host_vmcb;
 	svm->vmcb_pa = __pa(svm->host_vmcb);
 
-	/* Restore the original control entries */
-	copy_vmcb_control_area(svm->host_vmcb, hsave);
-
 	kvm_clear_exception_queue(&svm->vcpu);
 	kvm_clear_interrupt_queue(&svm->vcpu);
 
 	svm->nested.nested_cr3 = 0;
 
 	/* Restore selected save entries */
-	svm->vmcb->save.es = hsave->save.es;
-	svm->vmcb->save.cs = hsave->save.cs;
-	svm->vmcb->save.ss = hsave->save.ss;
-	svm->vmcb->save.ds = hsave->save.ds;
-	svm->vmcb->save.gdtr = hsave->save.gdtr;
-	svm->vmcb->save.idtr = hsave->save.idtr;
-	kvm_set_rflags(&svm->vcpu, hsave->save.rflags);
-	svm_set_efer(&svm->vcpu, hsave->save.efer);
-	svm_set_cr0(&svm->vcpu, hsave->save.cr0 | X86_CR0_PE);
-	svm_set_cr4(&svm->vcpu, hsave->save.cr4);
-	if (npt_enabled) {
-		svm->vmcb->save.cr3 = hsave->save.cr3;
-		svm->vcpu.arch.cr3 = hsave->save.cr3;
-	} else {
-		(void)kvm_set_cr3(&svm->vcpu, hsave->save.cr3);
-	}
-	kvm_register_write(&svm->vcpu, VCPU_REGS_RAX, hsave->save.rax);
-	kvm_register_write(&svm->vcpu, VCPU_REGS_RSP, hsave->save.rsp);
-	kvm_register_write(&svm->vcpu, VCPU_REGS_RIP, hsave->save.rip);
+	kvm_set_rflags(&svm->vcpu, svm->host_vmcb->save.rflags);
+	svm_set_efer(&svm->vcpu, svm->host_vmcb->save.efer);
+	svm_set_cr0(&svm->vcpu, svm->host_vmcb->save.cr0 | X86_CR0_PE);
+	svm_set_cr4(&svm->vcpu, svm->host_vmcb->save.cr4);
+
+	if (npt_enabled)
+		svm->vcpu.arch.cr3 = svm->host_vmcb->save.cr3;
+	else
+		kvm_set_cr3(&svm->vcpu, svm->host_vmcb->save.cr3);
+
 	svm->vmcb->save.dr7 = 0;
 	svm->vmcb->save.cpl = 0;
 	svm->vmcb->control.exit_int_info = 0;
 
+	kvm_register_write(&svm->vcpu, VCPU_REGS_RAX, svm->host_vmcb->save.rax);
+	kvm_register_write(&svm->vcpu, VCPU_REGS_RSP, svm->host_vmcb->save.rsp);
+	kvm_register_write(&svm->vcpu, VCPU_REGS_RIP, svm->host_vmcb->save.rip);
+
 	mark_all_dirty(svm->vmcb);
 
 	nested_svm_unmap(page);
@@ -2373,8 +2315,6 @@ static bool nested_vmcb_checks(struct vmcb *vmcb)
 static bool nested_svm_vmrun(struct vcpu_svm *svm)
 {
 	struct vmcb *nested_vmcb;
-	struct vmcb *hsave = svm->nested.hsave;
-	struct vmcb *vmcb = svm->vmcb;
 	struct page *page;
 	u64 vmcb_gpa;
 
@@ -2414,25 +2354,16 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 	 * Save the old vmcb, so we don't need to pick what we save, but can
 	 * restore everything when a VMEXIT occurs
 	 */
-	hsave->save.es     = vmcb->save.es;
-	hsave->save.cs     = vmcb->save.cs;
-	hsave->save.ss     = vmcb->save.ss;
-	hsave->save.ds     = vmcb->save.ds;
-	hsave->save.gdtr   = vmcb->save.gdtr;
-	hsave->save.idtr   = vmcb->save.idtr;
-	hsave->save.efer   = svm->vcpu.arch.efer;
-	hsave->save.cr0    = kvm_read_cr0(&svm->vcpu);
-	hsave->save.cr4    = svm->vcpu.arch.cr4;
-	hsave->save.rflags = kvm_get_rflags(&svm->vcpu);
-	hsave->save.rip    = kvm_rip_read(&svm->vcpu);
-	hsave->save.rsp    = vmcb->save.rsp;
-	hsave->save.rax    = vmcb->save.rax;
-	if (npt_enabled)
-		hsave->save.cr3    = vmcb->save.cr3;
-	else
-		hsave->save.cr3    = kvm_read_cr3(&svm->vcpu);
+	svm->host_vmcb->save.efer   = svm->vcpu.arch.efer;
+	svm->host_vmcb->save.cr0    = kvm_read_cr0(&svm->vcpu);
+	svm->host_vmcb->save.cr4    = svm->vcpu.arch.cr4;
+	svm->host_vmcb->save.rflags = kvm_get_rflags(&svm->vcpu);
+	svm->host_vmcb->save.rax    = svm->vcpu.arch.regs[VCPU_REGS_RAX];
+	svm->host_vmcb->save.rsp    = svm->vcpu.arch.regs[VCPU_REGS_RSP];
+	svm->host_vmcb->save.rip    = svm->vcpu.arch.regs[VCPU_REGS_RIP];
 
-	copy_vmcb_control_area(hsave, vmcb);
+	if (!npt_enabled)
+		svm->host_vmcb->save.cr3 = kvm_read_cr3(&svm->vcpu);
 
 	if (kvm_get_rflags(&svm->vcpu) & X86_EFLAGS_IF)
 		svm->vcpu.arch.hflags |= HF_HIF_MASK;
@@ -2478,7 +2409,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 		svm->vmcb->save.cr3 = nested_vmcb->save.cr3;
 		svm->vcpu.arch.cr3 = nested_vmcb->save.cr3;
 	} else
-		(void)kvm_set_cr3(&svm->vcpu, nested_vmcb->save.cr3);
+		kvm_set_cr3(&svm->vcpu, nested_vmcb->save.cr3);
 
 	/* Guest paging mode is active - reset mmu */
 	kvm_mmu_reset_context(&svm->vcpu);
@@ -2942,7 +2873,7 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data)
 
 	switch (ecx) {
 	case MSR_IA32_TSC: {
-		struct vmcb *vmcb = get_host_vmcb(svm);
+		struct vmcb *vmcb = svm->host_vmcb;
 
 		*data = vmcb->control.tsc_offset +
 			svm_scale_tsc(vcpu, native_read_tsc());
-- 
1.7.4.1

[PATCH 6/7] KVM: SVM: Rework hflags handling

[Joerg Roedel]

From: Joerg Roedel <joro@8bytes.org>

This patch moves the unsetting of the hflags used for
nesting into the #vmexit path instead of doing everything in
the vmrun path.

Signed-off-by: Joerg Roedel <joro@8bytes.org>
---
 arch/x86/kvm/svm.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index f2cca2c..c83315a 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2256,6 +2256,8 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
 
 	mark_all_dirty(svm->vmcb);
 
+	svm->vcpu.arch.hflags &=  ~(HF_VINTR_MASK | HF_HIF_MASK);
+
 	nested_svm_unmap(page);
 
 	nested_svm_uninit_mmu_context(&svm->vcpu);
@@ -2317,6 +2319,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 	struct vmcb *nested_vmcb;
 	struct page *page;
 	u64 vmcb_gpa;
+	u64 rflags;
 
 	vmcb_gpa = svm->vmcb->save.rax;
 
@@ -2350,6 +2353,8 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 	kvm_clear_exception_queue(&svm->vcpu);
 	kvm_clear_interrupt_queue(&svm->vcpu);
 
+	rflags = kvm_get_rflags(&svm->vcpu);
+
 	/*
 	 * Save the old vmcb, so we don't need to pick what we save, but can
 	 * restore everything when a VMEXIT occurs
@@ -2357,7 +2362,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 	svm->host_vmcb->save.efer   = svm->vcpu.arch.efer;
 	svm->host_vmcb->save.cr0    = kvm_read_cr0(&svm->vcpu);
 	svm->host_vmcb->save.cr4    = svm->vcpu.arch.cr4;
-	svm->host_vmcb->save.rflags = kvm_get_rflags(&svm->vcpu);
+	svm->host_vmcb->save.rflags = rflags;
 	svm->host_vmcb->save.rax    = svm->vcpu.arch.regs[VCPU_REGS_RAX];
 	svm->host_vmcb->save.rsp    = svm->vcpu.arch.regs[VCPU_REGS_RSP];
 	svm->host_vmcb->save.rip    = svm->vcpu.arch.regs[VCPU_REGS_RIP];
@@ -2365,17 +2370,11 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 	if (!npt_enabled)
 		svm->host_vmcb->save.cr3 = kvm_read_cr3(&svm->vcpu);
 
-	if (kvm_get_rflags(&svm->vcpu) & X86_EFLAGS_IF)
-		svm->vcpu.arch.hflags |= HF_HIF_MASK;
-	else
-		svm->vcpu.arch.hflags &= ~HF_HIF_MASK;
+	svm->vcpu.arch.hflags |= rflags & X86_EFLAGS_IF ? HF_HIF_MASK   : 0;
 
-	if (nested_vmcb->control.int_ctl & V_INTR_MASKING_MASK)
+	if (nested_vmcb->control.int_ctl & V_INTR_MASKING_MASK) {
 		svm->vcpu.arch.hflags |= HF_VINTR_MASK;
-	else
-		svm->vcpu.arch.hflags &= ~HF_VINTR_MASK;
 
-	if (svm->vcpu.arch.hflags & HF_VINTR_MASK) {
 		/* We only want the cr8 intercept bits of the guest */
 		clr_cr_intercept(svm, INTERCEPT_CR8_READ);
 		clr_cr_intercept(svm, INTERCEPT_CR8_WRITE);
-- 
1.7.4.1

[PATCH 7/7] KVM: SVM: Don't change host intercepts in vmrun emulation

[Joerg Roedel]

Rather than changing the host intercepts in
nested_svm_vmrun, mask the intercepts we only want to see
from the guest out in recalc_intercepts.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kvm/svm.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index c83315a..ab48dd4 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -102,6 +102,9 @@ struct nested_state {
 	u32 intercept_exceptions;
 	u64 intercept;
 
+	/* Mask of relevant host intercepts for recalculation */
+	u32 intercept_cr_mask;
+
 	/* Nested Paging related state */
 	u64 nested_cr3;
 
@@ -250,10 +253,11 @@ static void recalc_intercepts(struct vcpu_svm *svm)
 	h = &svm->host_vmcb->control;
 	g = &svm->nested;
 
-	c->intercept_cr = h->intercept_cr | g->intercept_cr;
+	c->intercept_cr = (h->intercept_cr & g->intercept_cr_mask) |
+			   g->intercept_cr;
 	c->intercept_dr = h->intercept_dr | g->intercept_dr;
 	c->intercept_exceptions = h->intercept_exceptions | g->intercept_exceptions;
-	c->intercept = h->intercept | g->intercept;
+	c->intercept = (h->intercept & ~(INTERCEPT_VMMCALL)) | g->intercept;
 }
 
 static inline void set_cr_intercept(struct vcpu_svm *svm, int bit)
@@ -2376,13 +2380,12 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 		svm->vcpu.arch.hflags |= HF_VINTR_MASK;
 
 		/* We only want the cr8 intercept bits of the guest */
-		clr_cr_intercept(svm, INTERCEPT_CR8_READ);
-		clr_cr_intercept(svm, INTERCEPT_CR8_WRITE);
+		svm->nested.intercept_cr_mask = ~(INTERCEPT_CR8_READ |
+						  INTERCEPT_CR8_WRITE);
+	} else {
+		svm->nested.intercept_cr_mask = 0ULL;
 	}
 
-	/* We don't want to see VMMCALLs from a nested guest */
-	clr_intercept(svm, INTERCEPT_VMMCALL);
-
 	if (nested_vmcb->control.nested_ctl) {
 		kvm_mmu_unload(&svm->vcpu);
 		svm->nested.nested_cr3 = nested_vmcb->control.nested_cr3;
-- 
1.7.4.1

Re: [PATCH 1/7] KVM: SVM: Keep seperate pointer to host-vmcb

[Avi Kivity]

On 07/13/2011 06:32 PM, Joerg Roedel wrote:
> From: Joerg Roedel<joro@8bytes.org>
>
> This patch introduces a new pointer which always points to
> the VMCB used for running the L1 guest.
>
> Signed-off-by: Joerg Roedel<joro@8bytes.org>
> ---
>   arch/x86/kvm/svm.c |    6 +++++-
>   1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 475d1c9..3d5990f 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -113,7 +113,9 @@ static u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
>   struct vcpu_svm {
>   	struct kvm_vcpu vcpu;
>   	struct vmcb *vmcb;
> +	struct vmcb *host_vmcb;
>   	unsigned long vmcb_pa;
> +	unsigned long host_vmcb_pa;

Why not call it vmcb01, like nvmx?  host_vmcb is ambiguous.

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

Re: [PATCH 2/7] KVM: SVM: Use host_vmcb_pa for vmload and vmsave

[Avi Kivity]

On 07/13/2011 06:32 PM, Joerg Roedel wrote:
> This saves copying over the vmload/vmsave switched part from
> the host to the guest vmcb later.
>
> Signed-off-by: Joerg Roedel<joerg.roedel@amd.com>
> ---
>   arch/x86/kvm/svm.c |    7 ++++++-
>   1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 3d5990f..dc703ac 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3704,9 +3704,13 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
>
>   		/* Enter guest mode */
>   		"push %%"R"ax \n\t"
> -		"mov %c[vmcb](%[svm]), %%"R"ax \n\t"
> +		"mov %c[host_vmcb](%[svm]), %%"R"ax \n\t"
>   		__ex(SVM_VMLOAD) "\n\t"
> +		"mov (%%"R"sp), %%"R"ax\n\t"
> +		"mov %c[vmcb](%[svm]), %%"R"ax \n\t"
>   		__ex(SVM_VMRUN) "\n\t"
> +		"mov (%%"R"sp), %%"R"ax\n\t"
> +		"mov %c[host_vmcb](%[svm]), %%"R"ax \n\t"
>   		__ex(SVM_VMSAVE) "\n\t"
>   		"pop %%"R"ax \n\t"
>

Okay, so the plan is to split L2 state between ->vmcb and ->host_vmcb?  
In that case my suggestion for patch 1 doesn't apply.  But the name 
still is confusing.  If we don't find a better one, I want a fat comment 
explaining how state is split.

(would be good to have documentation for the overall strategy of nsvm, 
like we have for nvmx and nmmu).

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

Re: [PATCH 4/7] KVM: SVM: Use seperate VMCB for L2 guests

[Avi Kivity]

On 07/13/2011 06:32 PM, Joerg Roedel wrote:
> From: Joerg Roedel<joro@8bytes.org>
>
> Move torwards emulation of VMCB-clean-bits by using a
> seperate VMCB when running L2 guests.
>
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index f81e35e..6dacf59 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -105,6 +105,8 @@ struct nested_state {
>
>   	/* Nested Paging related state */
>   	u64 nested_cr3;
> +
> +	struct vmcb *n_vmcb;
>   };
>
>   #define MSRPM_OFFSETS	16
> @@ -974,6 +976,26 @@ static u64 svm_compute_tsc_offset(struct kvm_vcpu *vcpu, u64 target_tsc)
>   	return target_tsc - tsc;
>   }
>
> +static bool init_nested_vmcb(struct vcpu_svm *svm)
> +{
> +	struct vmcb_control_area *hc, *nc;
> +
> +	svm->nested.n_vmcb = (void *)get_zeroed_page(GFP_KERNEL);
> +	if (svm->nested.n_vmcb == NULL)
> +		return false;
> +
> +	nc =&svm->nested.n_vmcb->control;
> +	hc =&svm->host_vmcb->control;
> +
> +	nc->iopm_base_pa		= hc->iopm_base_pa;
> +	nc->msrpm_base_pa		= hc->msrpm_base_pa;
> +	nc->nested_ctl			= hc->nested_ctl;
> +	nc->pause_filter_count		= hc->pause_filter_count;
> +	svm->nested.n_vmcb->save.g_pat	= svm->host_vmcb->save.g_pat;
> +
> +	return true;
> +}
> +

Instead of initializing the non-nested vmcb and then copying it, 
separate out the bits you're copying here into a separate function (i.e. 
init_vmcb_host_state()) and call it for both vmcbs.

I had practically the same comment for nvmx (see 
vmx_set_constant_host_state()).

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

Re: [PATCH 2/7] KVM: SVM: Use host_vmcb_pa for vmload and vmsave

[Joerg Roedel]

On Thu, Jul 14, 2011 at 02:29:36PM +0300, Avi Kivity wrote:
> On 07/13/2011 06:32 PM, Joerg Roedel wrote:
>> This saves copying over the vmload/vmsave switched part from
>> the host to the guest vmcb later.
>>
>> Signed-off-by: Joerg Roedel<joerg.roedel@amd.com>
>> ---
>>   arch/x86/kvm/svm.c |    7 ++++++-
>>   1 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 3d5990f..dc703ac 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -3704,9 +3704,13 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
>>
>>   		/* Enter guest mode */
>>   		"push %%"R"ax \n\t"
>> -		"mov %c[vmcb](%[svm]), %%"R"ax \n\t"
>> +		"mov %c[host_vmcb](%[svm]), %%"R"ax \n\t"
>>   		__ex(SVM_VMLOAD) "\n\t"
>> +		"mov (%%"R"sp), %%"R"ax\n\t"
>> +		"mov %c[vmcb](%[svm]), %%"R"ax \n\t"
>>   		__ex(SVM_VMRUN) "\n\t"
>> +		"mov (%%"R"sp), %%"R"ax\n\t"
>> +		"mov %c[host_vmcb](%[svm]), %%"R"ax \n\t"
>>   		__ex(SVM_VMSAVE) "\n\t"
>>   		"pop %%"R"ax \n\t"
>>
>
> Okay, so the plan is to split L2 state between ->vmcb and ->host_vmcb?

Yes, otherwise we need to copy the vmload/vmsave switched state back and
forth between both VMCBs which is a waste of cycles.

> In that case my suggestion for patch 1 doesn't apply.  But the name  
> still is confusing.  If we don't find a better one, I want a fat comment  
> explaining how state is split.

Hmm, how about naming them l1_vmcb and l2_vmcb? The comment explaining
why vmload/vmsave always happens on l1_vmcb is needed anyway then.

> (would be good to have documentation for the overall strategy of nsvm,  
> like we have for nvmx and nmmu).

There is not much to document about future plans for nested-svm. At the
moment I try to add emulation code for new SVM features when there is
some time left. Live migration support is also on the list.

The long-term plan is certainly to merge code with nested-vmx where
possible and move logic into generic KVM code. The first item that comes
to mind here is to create a single place where a vmexit is emulated and
let all other place which do that today just signal that it is required.

	Joerg

Re: [PATCH 4/7] KVM: SVM: Use seperate VMCB for L2 guests

[Joerg Roedel]

On Thu, Jul 14, 2011 at 02:38:39PM +0300, Avi Kivity wrote:
> On 07/13/2011 06:32 PM, Joerg Roedel wrote:
>> +static bool init_nested_vmcb(struct vcpu_svm *svm)
>> +{
>> +	struct vmcb_control_area *hc, *nc;
>> +
>> +	svm->nested.n_vmcb = (void *)get_zeroed_page(GFP_KERNEL);
>> +	if (svm->nested.n_vmcb == NULL)
>> +		return false;
>> +
>> +	nc =&svm->nested.n_vmcb->control;
>> +	hc =&svm->host_vmcb->control;
>> +
>> +	nc->iopm_base_pa		= hc->iopm_base_pa;
>> +	nc->msrpm_base_pa		= hc->msrpm_base_pa;
>> +	nc->nested_ctl			= hc->nested_ctl;
>> +	nc->pause_filter_count		= hc->pause_filter_count;
>> +	svm->nested.n_vmcb->save.g_pat	= svm->host_vmcb->save.g_pat;
>> +
>> +	return true;
>> +}
>> +
>
> Instead of initializing the non-nested vmcb and then copying it,  
> separate out the bits you're copying here into a separate function (i.e.  
> init_vmcb_host_state()) and call it for both vmcbs.
>
> I had practically the same comment for nvmx (see  
> vmx_set_constant_host_state()).

Makes sense. I'll probably remove the lazy allocation and initialize
both VMCBs at vcpu-creation time. The memory foodprint is the same as
before because the hsave area was also allocated at the beginning.

	Joerg

Re: [PATCH 2/7] KVM: SVM: Use host_vmcb_pa for vmload and vmsave

[Avi Kivity]

On 07/14/2011 04:10 PM, Joerg Roedel wrote:
> On Thu, Jul 14, 2011 at 02:29:36PM +0300, Avi Kivity wrote:
> >  On 07/13/2011 06:32 PM, Joerg Roedel wrote:
> >>  This saves copying over the vmload/vmsave switched part from
> >>  the host to the guest vmcb later.
> >>
> >>  Signed-off-by: Joerg Roedel<joerg.roedel@amd.com>
> >>  ---
> >>    arch/x86/kvm/svm.c |    7 ++++++-
> >>    1 files changed, 6 insertions(+), 1 deletions(-)
> >>
> >>  diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> >>  index 3d5990f..dc703ac 100644
> >>  --- a/arch/x86/kvm/svm.c
> >>  +++ b/arch/x86/kvm/svm.c
> >>  @@ -3704,9 +3704,13 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
> >>
> >>    		/* Enter guest mode */
> >>    		"push %%"R"ax \n\t"
> >>  -		"mov %c[vmcb](%[svm]), %%"R"ax \n\t"
> >>  +		"mov %c[host_vmcb](%[svm]), %%"R"ax \n\t"
> >>    		__ex(SVM_VMLOAD) "\n\t"
> >>  +		"mov (%%"R"sp), %%"R"ax\n\t"
> >>  +		"mov %c[vmcb](%[svm]), %%"R"ax \n\t"
> >>    		__ex(SVM_VMRUN) "\n\t"
> >>  +		"mov (%%"R"sp), %%"R"ax\n\t"
> >>  +		"mov %c[host_vmcb](%[svm]), %%"R"ax \n\t"
> >>    		__ex(SVM_VMSAVE) "\n\t"
> >>    		"pop %%"R"ax \n\t"
> >>
> >
> >  Okay, so the plan is to split L2 state between ->vmcb and ->host_vmcb?
>
> Yes, otherwise we need to copy the vmload/vmsave switched state back and
> forth between both VMCBs which is a waste of cycles.

Just to be sure I understand this: the root cause is because VMRUN 
doesn't actually switch this state.  So we have to copy the state.  Okay.

What about an L2 guest executing VMLOAD or VMSAVE which isn't 
intercepted?  Don't we have to redirect it's reads and writes to host_vmcb?

> >  In that case my suggestion for patch 1 doesn't apply.  But the name
> >  still is confusing.  If we don't find a better one, I want a fat comment
> >  explaining how state is split.
>
> Hmm, how about naming them l1_vmcb and l2_vmcb? The comment explaining
> why vmload/vmsave always happens on l1_vmcb is needed anyway then.

In a later patch you introduce n_vmcb.  I think it makes sense to name 
that vmcb02?

> >  (would be good to have documentation for the overall strategy of nsvm,
> >  like we have for nvmx and nmmu).
>
> There is not much to document about future plans for nested-svm. At the
> moment I try to add emulation code for new SVM features when there is
> some time left. Live migration support is also on the list.
>

Even the exising code would be good to document.  So when a reader sees 
some bit, they can compare it to the document and see why it's that way.

> The long-term plan is certainly to merge code with nested-vmx where
> possible and move logic into generic KVM code. The first item that comes
> to mind here is to create a single place where a vmexit is emulated and
> let all other place which do that today just signal that it is required.

I'm not very concerned about reuse with nvmx except for architectural 
code like interrupts.  Of course, if it turns out simple I'm all for it, 
but if it's hard or uglifies the code, let it be.

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

Re: [PATCH 4/7] KVM: SVM: Use seperate VMCB for L2 guests

[Avi Kivity]

On 07/14/2011 04:12 PM, Joerg Roedel wrote:
> Makes sense. I'll probably remove the lazy allocation and initialize
> both VMCBs at vcpu-creation time. The memory foodprint is the same as
> before because the hsave area was also allocated at the beginning.

Related, would we need a pool of n_vmcbs/vmcb02s?

I guess the condition for reusing an n_vmcb would be: same vmcb_gpa and 
at least one clean bit set?

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

Re: [PATCH 4/7] KVM: SVM: Use seperate VMCB for L2 guests

[Joerg Roedel]

On Thu, Jul 14, 2011 at 04:26:39PM +0300, Avi Kivity wrote:
> On 07/14/2011 04:12 PM, Joerg Roedel wrote:
>> Makes sense. I'll probably remove the lazy allocation and initialize
>> both VMCBs at vcpu-creation time. The memory foodprint is the same as
>> before because the hsave area was also allocated at the beginning.
>
> Related, would we need a pool of n_vmcbs/vmcb02s?

Probably. This depends on how nested-svm will be used I think. It is not
very hard to add if really needed. Some kind of LRU is certainly needed
too then.

> I guess the condition for reusing an n_vmcb would be: same vmcb_gpa and  
> at least one clean bit set?

Same vmcb_gpa is sufficient I think. I nothing is marked clean then it
is the same situation as if the vmcb_gpa is different.

	Joerg

Re: [PATCH 4/7] KVM: SVM: Use seperate VMCB for L2 guests

[Avi Kivity]

On 07/14/2011 04:40 PM, Joerg Roedel wrote:
> On Thu, Jul 14, 2011 at 04:26:39PM +0300, Avi Kivity wrote:
> >  On 07/14/2011 04:12 PM, Joerg Roedel wrote:
> >>  Makes sense. I'll probably remove the lazy allocation and initialize
> >>  both VMCBs at vcpu-creation time. The memory foodprint is the same as
> >>  before because the hsave area was also allocated at the beginning.
> >
> >  Related, would we need a pool of n_vmcbs/vmcb02s?
>
> Probably. This depends on how nested-svm will be used I think. It is not
> very hard to add if really needed. Some kind of LRU is certainly needed
> too then.
>
> >  I guess the condition for reusing an n_vmcb would be: same vmcb_gpa and
> >  at least one clean bit set?
>
> Same vmcb_gpa is sufficient I think. I nothing is marked clean then it
> is the same situation as if the vmcb_gpa is different.

Agree with both.

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

Re: [PATCH 2/7] KVM: SVM: Use host_vmcb_pa for vmload and vmsave

[Joerg Roedel]

On Thu, Jul 14, 2011 at 04:20:03PM +0300, Avi Kivity wrote:
> On 07/14/2011 04:10 PM, Joerg Roedel wrote:

>> Yes, otherwise we need to copy the vmload/vmsave switched state back and
>> forth between both VMCBs which is a waste of cycles.
>
> Just to be sure I understand this: the root cause is because VMRUN  
> doesn't actually switch this state.  So we have to copy the state.  Okay.

Right.

> What about an L2 guest executing VMLOAD or VMSAVE which isn't  
> intercepted?  Don't we have to redirect it's reads and writes to 
> host_vmcb?

Yes, that needs to target the host_vmcb then. This is buggy in the
patch-set. Thanks for pointing this out :)

>> Hmm, how about naming them l1_vmcb and l2_vmcb? The comment explaining
>> why vmload/vmsave always happens on l1_vmcb is needed anyway then.
>
> In a later patch you introduce n_vmcb.  I think it makes sense to name  
> that vmcb02?

Just for my understanding, what stands the first '0' for? The '1' and
'2' make sense, but the '0' seems to be redundant?

> Even the exising code would be good to document.  So when a reader sees  
> some bit, they can compare it to the document and see why it's that way.

I tried to put comments into the code to document the most complicated
parts. But there is certainly room for improvement. Overall, I think the
best place is to keep those comments in the code and not open another
document for it.

>> The long-term plan is certainly to merge code with nested-vmx where
>> possible and move logic into generic KVM code. The first item that comes
>> to mind here is to create a single place where a vmexit is emulated and
>> let all other place which do that today just signal that it is required.
>
> I'm not very concerned about reuse with nvmx except for architectural  
> code like interrupts.  Of course, if it turns out simple I'm all for it,  
> but if it's hard or uglifies the code, let it be.

Yes, the interrupt code is another part that probably can be made
generic.
The nested-mmu code is already generic. Nested-VMX should be able to
make use of it with only minor modifications.

	Joerg

Re: [PATCH 2/7] KVM: SVM: Use host_vmcb_pa for vmload and vmsave

[Avi Kivity]

On 07/14/2011 04:52 PM, Joerg Roedel wrote:
> >  What about an L2 guest executing VMLOAD or VMSAVE which isn't
> >  intercepted?  Don't we have to redirect it's reads and writes to
> >  host_vmcb?
>
> Yes, that needs to target the host_vmcb then. This is buggy in the
> patch-set. Thanks for pointing this out :)

For the low price of an additional test to svm.flat.

> >>  Hmm, how about naming them l1_vmcb and l2_vmcb? The comment explaining
> >>  why vmload/vmsave always happens on l1_vmcb is needed anyway then.
> >
> >  In a later patch you introduce n_vmcb.  I think it makes sense to name
> >  that vmcb02?
>
> Just for my understanding, what stands the first '0' for? The '1' and
> '2' make sense, but the '0' seems to be redundant?

The first number is the level running in host mode, the second is the 
level running guest mode.

vmcb01: host running guest
vmcb02: host running nested guest
vmcb12: guest running nested guest (i.e. the virtual vmcb in guest 
physical address space)

> >  Even the exising code would be good to document.  So when a reader sees
> >  some bit, they can compare it to the document and see why it's that way.
>
> I tried to put comments into the code to document the most complicated
> parts. But there is certainly room for improvement. Overall, I think the
> best place is to keep those comments in the code and not open another
> document for it.

Those are good for the details, but not so good for the master plan.  
Like mmu.txt.

> >>  The long-term plan is certainly to merge code with nested-vmx where
> >>  possible and move logic into generic KVM code. The first item that comes
> >>  to mind here is to create a single place where a vmexit is emulated and
> >>  let all other place which do that today just signal that it is required.
> >
> >  I'm not very concerned about reuse with nvmx except for architectural
> >  code like interrupts.  Of course, if it turns out simple I'm all for it,
> >  but if it's hard or uglifies the code, let it be.
>
> Yes, the interrupt code is another part that probably can be made
> generic.

Yes.

> The nested-mmu code is already generic. Nested-VMX should be able to
> make use of it with only minor modifications.

Yup, just need support for parsing the EPT PTE format.

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