[PATCH 0/12] Nested SVM cleanups

[PATCH 0/12] Nested SVM cleanups

[Joerg Roedel]

Hi,

here is a first round of patches to clean up the code for SVM virtualization
for KVM on AMD. There is more to clean up but since I am on vacation the rest
of the week here is what I have queued up so far. Maybe this saves me from
rebasing this code next week ;-)
I tested these patches with KVM in KVM and it works stable with and without
nested SMP. It doesn't seem to break anything.

	Joerg

Diffstat:

 arch/x86/kvm/svm.c |  422 +++++++++++++++++++++++++++++++--------------------
 1 files changed, 256 insertions(+), 166 deletions(-)

Shortlog:

Joerg Roedel (12):
      kvm/svm: make nested svm debugging runtime configurable
      kvm/svm: add helper functions for global interrupt flag
      kvm/svm: optimize nested #vmexit
      kvm/svm: optimize nested vmrun
      kvm/svm: copy only necessary parts of the control area on vmrun/vmexit
      kvm/svm: complete interrupts after handling nested exits
      kvm/svm: move nested svm state into seperate struct
      kvm/svm: cache nested intercepts
      kvm/svm: consolidate nested_svm_exit_handled
      kvm/svm: do nested vmexit in nested_svm_exit_handled
      kvm/svm: handle #pf intercepts in nested_svm_exit_handled directly
      kvm/svm: simplify nested_svm_check_exception

[PATCH 01/12] kvm/svm: make nested svm debugging runtime configurable

[Joerg Roedel]

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index b720b02..27d26c3 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -49,11 +49,17 @@ MODULE_LICENSE("GPL");
 
 #define DEBUGCTL_RESERVED_BITS (~(0x3fULL))
 
-/* Turn on to get debugging output*/
-/* #define NESTED_DEBUG */
+/* Turn on to get debugging output for nested svm */
+#undef NESTED_DEBUG
 
 #ifdef NESTED_DEBUG
-#define nsvm_printk(fmt, args...) printk(KERN_INFO fmt, ## args)
+
+static int nested_dbg;
+module_param(nested_dbg, bool, 0644);
+
+#define nsvm_printk(fmt, args...)	\
+	if (nested_dbg)			\
+		printk(KERN_INFO fmt, ## args)
 #else
 #define nsvm_printk(fmt, args...) do {} while(0)
 #endif
-- 
1.6.3.3

Re: [PATCH 01/12] kvm/svm: make nested svm debugging runtime configurable

[Alexander Graf]

Joerg Roedel wrote:
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>   

Acked-by: Alexander Graf <agraf@suse.de>

[PATCH 02/12] kvm/svm: add helper functions for global interrupt flag

[Joerg Roedel]

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 27d26c3..9f69b25 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -136,6 +136,21 @@ static inline bool is_nested(struct vcpu_svm *svm)
 	return svm->nested_vmcb;
 }
 
+static inline void enable_gif(struct vcpu_svm *svm)
+{
+	svm->vcpu.arch.hflags |= HF_GIF_MASK;
+}
+
+static inline void disable_gif(struct vcpu_svm *svm)
+{
+	svm->vcpu.arch.hflags &= ~HF_GIF_MASK;
+}
+
+static inline bool gif_set(struct vcpu_svm *svm)
+{
+	return !!(svm->vcpu.arch.hflags & HF_GIF_MASK);
+}
+
 static unsigned long iopm_base;
 
 struct kvm_ldttss_desc {
@@ -630,7 +645,7 @@ static void init_vmcb(struct vcpu_svm *svm)
 	force_new_asid(&svm->vcpu);
 
 	svm->nested_vmcb = 0;
-	svm->vcpu.arch.hflags = HF_GIF_MASK;
+	enable_gif(svm);
 }
 
 static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
@@ -1638,7 +1653,7 @@ static int nested_svm_vmexit_real(struct vcpu_svm *svm, void *arg1,
 	svm->vmcb->save.cpl = 0;
 	svm->vmcb->control.exit_int_info = 0;
 
-	svm->vcpu.arch.hflags &= ~HF_GIF_MASK;
+	disable_gif(svm);
 	/* Exit nested SVM mode */
 	svm->nested_vmcb = 0;
 
@@ -1770,7 +1785,7 @@ static int nested_svm_vmrun(struct vcpu_svm *svm, void *arg1,
 	svm->vmcb->control.event_inj = nested_vmcb->control.event_inj;
 	svm->vmcb->control.event_inj_err = nested_vmcb->control.event_inj_err;
 
-	svm->vcpu.arch.hflags |= HF_GIF_MASK;
+	enable_gif(svm);
 
 	return 0;
 }
@@ -1859,7 +1874,7 @@ static int stgi_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
 	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
 	skip_emulated_instruction(&svm->vcpu);
 
-	svm->vcpu.arch.hflags |= HF_GIF_MASK;
+	enable_gif(svm);
 
 	return 1;
 }
@@ -1872,7 +1887,7 @@ static int clgi_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
 	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
 	skip_emulated_instruction(&svm->vcpu);
 
-	svm->vcpu.arch.hflags &= ~HF_GIF_MASK;
+	disable_gif(svm);
 
 	/* After a CLGI no interrupts should come */
 	svm_clear_vintr(svm);
@@ -2359,7 +2374,7 @@ static void svm_set_irq(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
-	BUG_ON(!(svm->vcpu.arch.hflags & HF_GIF_MASK));
+	BUG_ON(!(gif_set(svm)));
 
 	svm->vmcb->control.event_inj = vcpu->arch.interrupt.nr |
 		SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR;
@@ -2390,7 +2405,7 @@ static int svm_interrupt_allowed(struct kvm_vcpu *vcpu)
 	struct vmcb *vmcb = svm->vmcb;
 	return (vmcb->save.rflags & X86_EFLAGS_IF) &&
 		!(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
-		(svm->vcpu.arch.hflags & HF_GIF_MASK) &&
+		gif_set(svm) &&
 		!is_nested(svm);
 }
 
@@ -2405,7 +2420,7 @@ static void enable_irq_window(struct kvm_vcpu *vcpu)
 	 * GIF becomes 1, because that's a separate STGI/VMRUN intercept.
 	 * The next time we get that intercept, this function will be
 	 * called again though and we'll get the vintr intercept. */
-	if (svm->vcpu.arch.hflags & HF_GIF_MASK) {
+	if (gif_set(svm)) {
 		svm_set_vintr(svm);
 		svm_inject_irq(svm, 0x0);
 	}
-- 
1.6.3.3

Re: [PATCH 02/12] kvm/svm: add helper functions for global interrupt flag

[Alexander Graf]

Joerg Roedel wrote:
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  arch/x86/kvm/svm.c |   31 +++++++++++++++++++++++--------
>  1 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 27d26c3..9f69b25 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -136,6 +136,21 @@ static inline bool is_nested(struct vcpu_svm *svm)
>  	return svm->nested_vmcb;
>  }
>  
> +static inline void enable_gif(struct vcpu_svm *svm)
> +{
> +	svm->vcpu.arch.hflags |= HF_GIF_MASK;
> +}
> +
> +static inline void disable_gif(struct vcpu_svm *svm)
> +{
> +	svm->vcpu.arch.hflags &= ~HF_GIF_MASK;
> +}
> +
> +static inline bool gif_set(struct vcpu_svm *svm)
> +{
> +	return !!(svm->vcpu.arch.hflags & HF_GIF_MASK);
> +}
> +
>  static unsigned long iopm_base;
>  
>  struct kvm_ldttss_desc {
> @@ -630,7 +645,7 @@ static void init_vmcb(struct vcpu_svm *svm)
>  	force_new_asid(&svm->vcpu);
>  
>  	svm->nested_vmcb = 0;
> -	svm->vcpu.arch.hflags = HF_GIF_MASK;
> +	enable_gif(svm);
>   

Are we sure hflags is always 0 here?

Re: [PATCH 02/12] kvm/svm: add helper functions for global interrupt flag

[Joerg Roedel]

On Wed, Jul 29, 2009 at 03:15:11PM +0200, Alexander Graf wrote:
> Joerg Roedel wrote:
> > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > ---
> >  arch/x86/kvm/svm.c |   31 +++++++++++++++++++++++--------
> >  1 files changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index 27d26c3..9f69b25 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -136,6 +136,21 @@ static inline bool is_nested(struct vcpu_svm *svm)
> >  	return svm->nested_vmcb;
> >  }
> >  
> > +static inline void enable_gif(struct vcpu_svm *svm)
> > +{
> > +	svm->vcpu.arch.hflags |= HF_GIF_MASK;
> > +}
> > +
> > +static inline void disable_gif(struct vcpu_svm *svm)
> > +{
> > +	svm->vcpu.arch.hflags &= ~HF_GIF_MASK;
> > +}
> > +
> > +static inline bool gif_set(struct vcpu_svm *svm)
> > +{
> > +	return !!(svm->vcpu.arch.hflags & HF_GIF_MASK);
> > +}
> > +
> >  static unsigned long iopm_base;
> >  
> >  struct kvm_ldttss_desc {
> > @@ -630,7 +645,7 @@ static void init_vmcb(struct vcpu_svm *svm)
> >  	force_new_asid(&svm->vcpu);
> >  
> >  	svm->nested_vmcb = 0;
> > -	svm->vcpu.arch.hflags = HF_GIF_MASK;
> > +	enable_gif(svm);
> >   
> 
> Are we sure hflags is always 0 here?

Hmm, yes. Except for the vcpu reset path clear_page is called for before
init_vmcb is called. I'll change it to set hflags to 0 in this function.

	Joerg

[PATCH 03/12] kvm/svm: optimize nested #vmexit

[Joerg Roedel]

It is more efficient to copy only the relevant parts of the vmcb back to
the nested vmcb when we emulate an vmexit.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 9f69b25..3e794f0 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1579,53 +1579,52 @@ static int nested_svm_vmexit_real(struct vcpu_svm *svm, void *arg1,
 {
 	struct vmcb *nested_vmcb = (struct vmcb *)arg1;
 	struct vmcb *hsave = svm->hsave;
-	u64 nested_save[] = { nested_vmcb->save.cr0,
-			      nested_vmcb->save.cr3,
-			      nested_vmcb->save.cr4,
-			      nested_vmcb->save.efer,
-			      nested_vmcb->control.intercept_cr_read,
-			      nested_vmcb->control.intercept_cr_write,
-			      nested_vmcb->control.intercept_dr_read,
-			      nested_vmcb->control.intercept_dr_write,
-			      nested_vmcb->control.intercept_exceptions,
-			      nested_vmcb->control.intercept,
-			      nested_vmcb->control.msrpm_base_pa,
-			      nested_vmcb->control.iopm_base_pa,
-			      nested_vmcb->control.tsc_offset };
+	struct vmcb *vmcb = svm->vmcb;
 
 	/* Give the current vmcb to the guest */
-	memcpy(nested_vmcb, svm->vmcb, sizeof(struct vmcb));
-	nested_vmcb->save.cr0 = nested_save[0];
-	if (!npt_enabled)
-		nested_vmcb->save.cr3 = nested_save[1];
-	nested_vmcb->save.cr4 = nested_save[2];
-	nested_vmcb->save.efer = nested_save[3];
-	nested_vmcb->control.intercept_cr_read = nested_save[4];
-	nested_vmcb->control.intercept_cr_write = nested_save[5];
-	nested_vmcb->control.intercept_dr_read = nested_save[6];
-	nested_vmcb->control.intercept_dr_write = nested_save[7];
-	nested_vmcb->control.intercept_exceptions = nested_save[8];
-	nested_vmcb->control.intercept = nested_save[9];
-	nested_vmcb->control.msrpm_base_pa = nested_save[10];
-	nested_vmcb->control.iopm_base_pa = nested_save[11];
-	nested_vmcb->control.tsc_offset = nested_save[12];
+	disable_gif(svm);
+
+	nested_vmcb->save.es     = vmcb->save.es;
+	nested_vmcb->save.cs     = vmcb->save.cs;
+	nested_vmcb->save.ss     = vmcb->save.ss;
+	nested_vmcb->save.ds     = vmcb->save.ds;
+	nested_vmcb->save.gdtr   = vmcb->save.gdtr;
+	nested_vmcb->save.idtr   = vmcb->save.idtr;
+	if (npt_enabled)
+		nested_vmcb->save.cr3    = vmcb->save.cr3;
+	nested_vmcb->save.cr2    = vmcb->save.cr2;
+	nested_vmcb->save.rflags = vmcb->save.rflags;
+	nested_vmcb->save.rip    = vmcb->save.rip;
+	nested_vmcb->save.rsp    = vmcb->save.rsp;
+	nested_vmcb->save.rax    = vmcb->save.rax;
+	nested_vmcb->save.dr7    = vmcb->save.dr7;
+	nested_vmcb->save.dr6    = vmcb->save.dr6;
+	nested_vmcb->save.cpl    = vmcb->save.cpl;
+
+	nested_vmcb->control.int_ctl           = vmcb->control.int_ctl;
+	nested_vmcb->control.int_vector        = vmcb->control.int_vector;
+	nested_vmcb->control.int_state         = vmcb->control.int_state;
+	nested_vmcb->control.exit_code         = vmcb->control.exit_code;
+	nested_vmcb->control.exit_code_hi      = vmcb->control.exit_code_hi;
+	nested_vmcb->control.exit_info_1       = vmcb->control.exit_info_1;
+	nested_vmcb->control.exit_info_2       = vmcb->control.exit_info_2;
+	nested_vmcb->control.exit_int_info     = vmcb->control.exit_int_info;
+	nested_vmcb->control.exit_int_info_err = vmcb->control.exit_int_info_err;
+	nested_vmcb->control.tlb_ctl           = 0;
+	nested_vmcb->control.event_inj         = 0;
+	nested_vmcb->control.event_inj_err     = 0;
 
 	/* We always set V_INTR_MASKING and remember the old value in hflags */
 	if (!(svm->vcpu.arch.hflags & HF_VINTR_MASK))
 		nested_vmcb->control.int_ctl &= ~V_INTR_MASKING_MASK;
 
-	if ((nested_vmcb->control.int_ctl & V_IRQ_MASK) &&
-	    (nested_vmcb->control.int_vector)) {
-		nsvm_printk("WARNING: IRQ 0x%x still enabled on #VMEXIT\n",
-				nested_vmcb->control.int_vector);
-	}
-
 	/* Restore the original control entries */
 	svm->vmcb->control = hsave->control;
 
 	/* Kill any pending exceptions */
 	if (svm->vcpu.arch.exception.pending == true)
 		nsvm_printk("WARNING: Pending Exception\n");
+
 	kvm_clear_exception_queue(&svm->vcpu);
 	kvm_clear_interrupt_queue(&svm->vcpu);
 
@@ -1653,7 +1652,6 @@ static int nested_svm_vmexit_real(struct vcpu_svm *svm, void *arg1,
 	svm->vmcb->save.cpl = 0;
 	svm->vmcb->control.exit_int_info = 0;
 
-	disable_gif(svm);
 	/* Exit nested SVM mode */
 	svm->nested_vmcb = 0;
 
-- 
1.6.3.3

Re: [PATCH 03/12] kvm/svm: optimize nested #vmexit

[Alexander Graf]

Joerg Roedel wrote:
> It is more efficient to copy only the relevant parts of the vmcb back to
> the nested vmcb when we emulate an vmexit.
>
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>   

Acked-by: Alexander Graf <agraf@suse.de>

[PATCH 04/12] kvm/svm: optimize nested vmrun

[Joerg Roedel]

Only copy the necessary parts of the vmcb save area on vmrun and save
precious time.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 3e794f0..b85da3f 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1688,6 +1688,7 @@ static int nested_svm_vmrun(struct vcpu_svm *svm, void *arg1,
 {
 	struct vmcb *nested_vmcb = (struct vmcb *)arg1;
 	struct vmcb *hsave = svm->hsave;
+	struct vmcb *vmcb = svm->vmcb;
 
 	/* nested_vmcb is our indicator if nested SVM is activated */
 	svm->nested_vmcb = svm->vmcb->save.rax;
@@ -1698,12 +1699,25 @@ static int nested_svm_vmrun(struct vcpu_svm *svm, void *arg1,
 
 	/* Save the old vmcb, so we don't need to pick what we save, but
 	   can restore everything when a VMEXIT occurs */
-	memcpy(hsave, svm->vmcb, sizeof(struct vmcb));
-	/* We need to remember the original CR3 in the SPT case */
-	if (!npt_enabled)
-		hsave->save.cr3 = svm->vcpu.arch.cr3;
-	hsave->save.cr4 = svm->vcpu.arch.cr4;
-	hsave->save.rip = svm->next_rip;
+	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.shadow_efer;
+	hsave->save.cr0    = svm->vcpu.arch.cr0;
+	hsave->save.cr4    = svm->vcpu.arch.cr4;
+	hsave->save.rflags = vmcb->save.rflags;
+	hsave->save.rip    = svm->next_rip;
+	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    = svm->vcpu.arch.cr3;
+
+	hsave->control = vmcb->control;
 
 	if (svm->vmcb->save.rflags & X86_EFLAGS_IF)
 		svm->vcpu.arch.hflags |= HF_HIF_MASK;
-- 
1.6.3.3

Re: [PATCH 04/12] kvm/svm: optimize nested vmrun

[Alexander Graf]

Joerg Roedel wrote:
> Only copy the necessary parts of the vmcb save area on vmrun and save
> precious time.
>
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>   

Acked-by: Alexander Graf <agraf@suse.de>

[PATCH 05/12] kvm/svm: copy only necessary parts of the control area on vmrun/vmexit

[Joerg Roedel]

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index b85da3f..0a0b3e1 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1574,6 +1574,38 @@ static int nested_svm_exit_handled(struct vcpu_svm *svm, bool kvm_override)
 			     nested_svm_exit_handled_real);
 }
 
+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_read    = from->intercept_cr_read;
+	dst->intercept_cr_write   = from->intercept_cr_write;
+	dst->intercept_dr_read    = from->intercept_dr_read;
+	dst->intercept_dr_write   = from->intercept_dr_write;
+	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_real(struct vcpu_svm *svm, void *arg1,
 				  void *arg2, void *opaque)
 {
@@ -1619,7 +1651,7 @@ static int nested_svm_vmexit_real(struct vcpu_svm *svm, void *arg1,
 		nested_vmcb->control.int_ctl &= ~V_INTR_MASKING_MASK;
 
 	/* Restore the original control entries */
-	svm->vmcb->control = hsave->control;
+	copy_vmcb_control_area(vmcb, hsave);
 
 	/* Kill any pending exceptions */
 	if (svm->vcpu.arch.exception.pending == true)
@@ -1717,7 +1749,7 @@ static int nested_svm_vmrun(struct vcpu_svm *svm, void *arg1,
 	else
 		hsave->save.cr3    = svm->vcpu.arch.cr3;
 
-	hsave->control = vmcb->control;
+	copy_vmcb_control_area(hsave, vmcb);
 
 	if (svm->vmcb->save.rflags & X86_EFLAGS_IF)
 		svm->vcpu.arch.hflags |= HF_HIF_MASK;
-- 
1.6.3.3

Re: [PATCH 05/12] kvm/svm: copy only necessary parts of the control area on vmrun/vmexit

[Alexander Graf]

Joerg Roedel wrote:
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>   

What's the reason behind this?

Acked-by: Alexander Graf <agraf@suse.de>

Re: [PATCH 05/12] kvm/svm: copy only necessary parts of the control area on vmrun/vmexit

[Joerg Roedel]

On Wed, Jul 29, 2009 at 03:30:01PM +0200, Alexander Graf wrote:
> Joerg Roedel wrote:
> > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> >   
> 
> What's the reason behind this?
> 
> Acked-by: Alexander Graf <agraf@suse.de>

Reason is to not copy the reserved fields of the control area. These
fields sum up to size of 899 bytes which are unnecessarily copied.

	Joerg

Re: [PATCH 05/12] kvm/svm: copy only necessary parts of the control area on vmrun/vmexit

[Alexander Graf]

Joerg Roedel wrote:
> On Wed, Jul 29, 2009 at 03:30:01PM +0200, Alexander Graf wrote:
>   
>> Joerg Roedel wrote:
>>     
>>> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>>>   
>>>       
>> What's the reason behind this?
>>
>> Acked-by: Alexander Graf <agraf@suse.de>
>>     
>
> Reason is to not copy the reserved fields of the control area. These
> fields sum up to size of 899 bytes which are unnecessarily copied.
>   

Fair enough :-)

[PATCH 06/12] kvm/svm: complete interrupts after handling nested exits

[Joerg Roedel]

The interrupt completion code must run after nested exits are handled
because not injected interrupts or exceptions may be handled by the l1
guest first.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 0a0b3e1..1756a4c 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -118,6 +118,7 @@ static int nested = 0;
 module_param(nested, int, S_IRUGO);
 
 static void svm_flush_tlb(struct kvm_vcpu *vcpu);
+static void svm_complete_interrupts(struct vcpu_svm *svm);
 
 static int nested_svm_exit_handled(struct vcpu_svm *svm, bool kvm_override);
 static int nested_svm_vmexit(struct vcpu_svm *svm);
@@ -2329,6 +2330,8 @@ static int handle_exit(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 		}
 	}
 
+	svm_complete_interrupts(svm);
+
 	if (npt_enabled) {
 		int mmu_reload = 0;
 		if ((vcpu->arch.cr0 ^ svm->vmcb->save.cr0) & X86_CR0_PG) {
@@ -2695,8 +2698,6 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 		vcpu->arch.regs_avail &= ~(1 << VCPU_EXREG_PDPTR);
 		vcpu->arch.regs_dirty &= ~(1 << VCPU_EXREG_PDPTR);
 	}
-
-	svm_complete_interrupts(svm);
 }
 
 #undef R
-- 
1.6.3.3

Re: [PATCH 06/12] kvm/svm: complete interrupts after handling nested exits

[Alexander Graf]

Joerg Roedel wrote:
> The interrupt completion code must run after nested exits are handled
> because not injected interrupts or exceptions may be handled by the l1
> guest first.
>
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>   

If you say so, I believe you :-).

Acked-by: Alexander Graf <agraf@suse.de>

[PATCH 07/12] kvm/svm: move nested svm state into seperate struct

[Joerg Roedel]

This makes it more clear for which purpose these members in the vcpu_svm
exist.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 1756a4c..31467b1 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -76,6 +76,18 @@ static const u32 host_save_user_msrs[] = {
 
 struct kvm_vcpu;
 
+struct nested_state {
+	struct vmcb *hsave;
+	u64 hsave_msr;
+	u64 vmcb;
+
+	/* These are the merged vectors */
+	u32 *msrpm;
+
+	/* gpa pointers to the real vectors */
+	u64 vmcb_msrpm;
+};
+
 struct vcpu_svm {
 	struct kvm_vcpu vcpu;
 	struct vmcb *vmcb;
@@ -92,16 +104,8 @@ struct vcpu_svm {
 	u64 host_gs_base;
 
 	u32 *msrpm;
-	struct vmcb *hsave;
-	u64 hsave_msr;
-
-	u64 nested_vmcb;
 
-	/* These are the merged vectors */
-	u32 *nested_msrpm;
-
-	/* gpa pointers to the real vectors */
-	u64 nested_vmcb_msrpm;
+	struct nested_state nested;
 };
 
 /* enable NPT for AMD64 and X86 with PAE */
@@ -134,7 +138,7 @@ static inline struct vcpu_svm *to_svm(struct kvm_vcpu *vcpu)
 
 static inline bool is_nested(struct vcpu_svm *svm)
 {
-	return svm->nested_vmcb;
+	return svm->nested.vmcb;
 }
 
 static inline void enable_gif(struct vcpu_svm *svm)
@@ -645,7 +649,7 @@ static void init_vmcb(struct vcpu_svm *svm)
 	}
 	force_new_asid(&svm->vcpu);
 
-	svm->nested_vmcb = 0;
+	svm->nested.vmcb = 0;
 	enable_gif(svm);
 }
 
@@ -706,9 +710,9 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
 	hsave_page = alloc_page(GFP_KERNEL);
 	if (!hsave_page)
 		goto uninit;
-	svm->hsave = page_address(hsave_page);
+	svm->nested.hsave = page_address(hsave_page);
 
-	svm->nested_msrpm = page_address(nested_msrpm_pages);
+	svm->nested.msrpm = page_address(nested_msrpm_pages);
 
 	svm->vmcb = page_address(page);
 	clear_page(svm->vmcb);
@@ -738,8 +742,8 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
 
 	__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->hsave));
-	__free_pages(virt_to_page(svm->nested_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);
 }
@@ -1565,13 +1569,13 @@ static int nested_svm_exit_handled(struct vcpu_svm *svm, bool kvm_override)
 
 	switch (svm->vmcb->control.exit_code) {
 	case SVM_EXIT_MSR:
-		return nested_svm_do(svm, svm->nested_vmcb,
-				     svm->nested_vmcb_msrpm, NULL,
+		return nested_svm_do(svm, svm->nested.vmcb,
+				     svm->nested.vmcb_msrpm, NULL,
 				     nested_svm_exit_handled_msr);
 	default: break;
 	}
 
-	return nested_svm_do(svm, svm->nested_vmcb, 0, &k,
+	return nested_svm_do(svm, svm->nested.vmcb, 0, &k,
 			     nested_svm_exit_handled_real);
 }
 
@@ -1611,7 +1615,7 @@ static int nested_svm_vmexit_real(struct vcpu_svm *svm, void *arg1,
 				  void *arg2, void *opaque)
 {
 	struct vmcb *nested_vmcb = (struct vmcb *)arg1;
-	struct vmcb *hsave = svm->hsave;
+	struct vmcb *hsave = svm->nested.hsave;
 	struct vmcb *vmcb = svm->vmcb;
 
 	/* Give the current vmcb to the guest */
@@ -1686,7 +1690,7 @@ static int nested_svm_vmexit_real(struct vcpu_svm *svm, void *arg1,
 	svm->vmcb->control.exit_int_info = 0;
 
 	/* Exit nested SVM mode */
-	svm->nested_vmcb = 0;
+	svm->nested.vmcb = 0;
 
 	return 0;
 }
@@ -1694,7 +1698,7 @@ static int nested_svm_vmexit_real(struct vcpu_svm *svm, void *arg1,
 static int nested_svm_vmexit(struct vcpu_svm *svm)
 {
 	nsvm_printk("VMexit\n");
-	if (nested_svm_do(svm, svm->nested_vmcb, 0,
+	if (nested_svm_do(svm, svm->nested.vmcb, 0,
 			  NULL, nested_svm_vmexit_real))
 		return 1;
 
@@ -1710,8 +1714,8 @@ static int nested_svm_vmrun_msrpm(struct vcpu_svm *svm, void *arg1,
 	int i;
 	u32 *nested_msrpm = (u32*)arg1;
 	for (i=0; i< PAGE_SIZE * (1 << MSRPM_ALLOC_ORDER) / 4; i++)
-		svm->nested_msrpm[i] = svm->msrpm[i] | nested_msrpm[i];
-	svm->vmcb->control.msrpm_base_pa = __pa(svm->nested_msrpm);
+		svm->nested.msrpm[i] = svm->msrpm[i] | nested_msrpm[i];
+	svm->vmcb->control.msrpm_base_pa = __pa(svm->nested.msrpm);
 
 	return 0;
 }
@@ -1720,11 +1724,11 @@ static int nested_svm_vmrun(struct vcpu_svm *svm, void *arg1,
 			    void *arg2, void *opaque)
 {
 	struct vmcb *nested_vmcb = (struct vmcb *)arg1;
-	struct vmcb *hsave = svm->hsave;
+	struct vmcb *hsave = svm->nested.hsave;
 	struct vmcb *vmcb = svm->vmcb;
 
 	/* nested_vmcb is our indicator if nested SVM is activated */
-	svm->nested_vmcb = svm->vmcb->save.rax;
+	svm->nested.vmcb = svm->vmcb->save.rax;
 
 	/* Clear internal status */
 	kvm_clear_exception_queue(&svm->vcpu);
@@ -1802,7 +1806,7 @@ static int nested_svm_vmrun(struct vcpu_svm *svm, void *arg1,
 
 	svm->vmcb->control.intercept |= nested_vmcb->control.intercept;
 
-	svm->nested_vmcb_msrpm = nested_vmcb->control.msrpm_base_pa;
+	svm->nested.vmcb_msrpm = nested_vmcb->control.msrpm_base_pa;
 
 	force_new_asid(&svm->vcpu);
 	svm->vmcb->control.exit_int_info = nested_vmcb->control.exit_int_info;
@@ -1904,7 +1908,7 @@ static int vmrun_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
 			  NULL, nested_svm_vmrun))
 		return 1;
 
-	if (nested_svm_do(svm, svm->nested_vmcb_msrpm, 0,
+	if (nested_svm_do(svm, svm->nested.vmcb_msrpm, 0,
 		      NULL, nested_svm_vmrun_msrpm))
 		return 1;
 
@@ -2114,7 +2118,7 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data)
 		*data = svm->vmcb->save.last_excp_to;
 		break;
 	case MSR_VM_HSAVE_PA:
-		*data = svm->hsave_msr;
+		*data = svm->nested.hsave_msr;
 		break;
 	case MSR_VM_CR:
 		*data = 0;
@@ -2200,7 +2204,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 data)
 			svm_disable_lbrv(svm);
 		break;
 	case MSR_VM_HSAVE_PA:
-		svm->hsave_msr = data;
+		svm->nested.hsave_msr = data;
 		break;
 	case MSR_VM_CR:
 	case MSR_VM_IGNNE:
-- 
1.6.3.3

Re: [PATCH 07/12] kvm/svm: move nested svm state into seperate struct

[Alexander Graf]

Joerg Roedel wrote:
> This makes it more clear for which purpose these members in the vcpu_svm
> exist.
>
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>   

Nice idea!

Acked-by: Alexander Graf <agraf@suse.de>

[PATCH 08/12] kvm/svm: cache nested intercepts

[Joerg Roedel]

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 31467b1..9192c9a 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -86,6 +86,15 @@ struct nested_state {
 
 	/* gpa pointers to the real vectors */
 	u64 vmcb_msrpm;
+
+	/* cache for intercepts of the guest */
+	u16 intercept_cr_read;
+	u16 intercept_cr_write;
+	u16 intercept_dr_read;
+	u16 intercept_dr_write;
+	u32 intercept_exceptions;
+	u64 intercept;
+
 };
 
 struct vcpu_svm {
@@ -1459,7 +1468,6 @@ static int nested_svm_exit_handled_real(struct vcpu_svm *svm,
 					void *arg2,
 					void *opaque)
 {
-	struct vmcb *nested_vmcb = (struct vmcb *)arg1;
 	bool kvm_overrides = *(bool *)opaque;
 	u32 exit_code = svm->vmcb->control.exit_code;
 
@@ -1486,38 +1494,38 @@ static int nested_svm_exit_handled_real(struct vcpu_svm *svm,
 	switch (exit_code) {
 	case SVM_EXIT_READ_CR0 ... SVM_EXIT_READ_CR8: {
 		u32 cr_bits = 1 << (exit_code - SVM_EXIT_READ_CR0);
-		if (nested_vmcb->control.intercept_cr_read & cr_bits)
+		if (svm->nested.intercept_cr_read & cr_bits)
 			return 1;
 		break;
 	}
 	case SVM_EXIT_WRITE_CR0 ... SVM_EXIT_WRITE_CR8: {
 		u32 cr_bits = 1 << (exit_code - SVM_EXIT_WRITE_CR0);
-		if (nested_vmcb->control.intercept_cr_write & cr_bits)
+		if (svm->nested.intercept_cr_write & cr_bits)
 			return 1;
 		break;
 	}
 	case SVM_EXIT_READ_DR0 ... SVM_EXIT_READ_DR7: {
 		u32 dr_bits = 1 << (exit_code - SVM_EXIT_READ_DR0);
-		if (nested_vmcb->control.intercept_dr_read & dr_bits)
+		if (svm->nested.intercept_dr_read & dr_bits)
 			return 1;
 		break;
 	}
 	case SVM_EXIT_WRITE_DR0 ... SVM_EXIT_WRITE_DR7: {
 		u32 dr_bits = 1 << (exit_code - SVM_EXIT_WRITE_DR0);
-		if (nested_vmcb->control.intercept_dr_write & dr_bits)
+		if (svm->nested.intercept_dr_write & dr_bits)
 			return 1;
 		break;
 	}
 	case SVM_EXIT_EXCP_BASE ... SVM_EXIT_EXCP_BASE + 0x1f: {
 		u32 excp_bits = 1 << (exit_code - SVM_EXIT_EXCP_BASE);
-		if (nested_vmcb->control.intercept_exceptions & excp_bits)
+		if (svm->nested.intercept_exceptions & excp_bits)
 			return 1;
 		break;
 	}
 	default: {
 		u64 exit_bits = 1ULL << (exit_code - SVM_EXIT_INTR);
 		nsvm_printk("exit code: 0x%x\n", exit_code);
-		if (nested_vmcb->control.intercept & exit_bits)
+		if (svm->nested.intercept & exit_bits)
 			return 1;
 	}
 	}
@@ -1808,6 +1816,14 @@ static int nested_svm_vmrun(struct vcpu_svm *svm, void *arg1,
 
 	svm->nested.vmcb_msrpm = nested_vmcb->control.msrpm_base_pa;
 
+	/* cache intercepts */
+	svm->nested.intercept_cr_read    = nested_vmcb->control.intercept_cr_read;
+	svm->nested.intercept_cr_write   = nested_vmcb->control.intercept_cr_write;
+	svm->nested.intercept_dr_read    = nested_vmcb->control.intercept_dr_read;
+	svm->nested.intercept_dr_write   = nested_vmcb->control.intercept_dr_write;
+	svm->nested.intercept_exceptions = nested_vmcb->control.intercept_exceptions;
+	svm->nested.intercept            = nested_vmcb->control.intercept;
+
 	force_new_asid(&svm->vcpu);
 	svm->vmcb->control.exit_int_info = nested_vmcb->control.exit_int_info;
 	svm->vmcb->control.exit_int_info_err = nested_vmcb->control.exit_int_info_err;
-- 
1.6.3.3

Re: [PATCH 08/12] kvm/svm: cache nested intercepts

[Avi Kivity]

On 07/29/2009 03:56 PM, Joerg Roedel wrote:
> Signed-off-by: Joerg Roedel<joerg.roedel@amd.com>
> ---
>   arch/x86/kvm/svm.c |   30 +++++++++++++++++++++++-------
>   1 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 31467b1..9192c9a 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -86,6 +86,15 @@ struct nested_state {
>
>   	/* gpa pointers to the real vectors */
>   	u64 vmcb_msrpm;
> +
> +	/* cache for intercepts of the guest */
> +	u16 intercept_cr_read;
> +	u16 intercept_cr_write;
> +	u16 intercept_dr_read;
> +	u16 intercept_dr_write;
> +	u32 intercept_exceptions;
> +	u64 intercept;
> +
>   };
>
>   struct vcpu_svm {
> @@ -1459,7 +1468,6 @@ static int nested_svm_exit_handled_real(struct vcpu_svm *svm,
>   					void *arg2,
>   					void *opaque)
>   {
> -	struct vmcb *nested_vmcb = (struct vmcb *)arg1;
>   	bool kvm_overrides = *(bool *)opaque;
>   	u32 exit_code = svm->vmcb->control.exit_code;
>
> @@ -1486,38 +1494,38 @@ static int nested_svm_exit_handled_real(struct vcpu_svm *svm,
>   	switch (exit_code) {
>   	case SVM_EXIT_READ_CR0 ... SVM_EXIT_READ_CR8: {
>   		u32 cr_bits = 1<<  (exit_code - SVM_EXIT_READ_CR0);
> -		if (nested_vmcb->control.intercept_cr_read&  cr_bits)
> +		if (svm->nested.intercept_cr_read&  cr_bits)
>   			return 1;
>   		break;
>   	}
>   	case SVM_EXIT_WRITE_CR0 ... SVM_EXIT_WRITE_CR8: {
>   		u32 cr_bits = 1<<  (exit_code - SVM_EXIT_WRITE_CR0);
> -		if (nested_vmcb->control.intercept_cr_write&  cr_bits)
> +		if (svm->nested.intercept_cr_write&  cr_bits)
>   			return 1;
>   		break;
>   	}
>   	case SVM_EXIT_READ_DR0 ... SVM_EXIT_READ_DR7: {
>   		u32 dr_bits = 1<<  (exit_code - SVM_EXIT_READ_DR0);
> -		if (nested_vmcb->control.intercept_dr_read&  dr_bits)
> +		if (svm->nested.intercept_dr_read&  dr_bits)
>   			return 1;
>   		break;
>   	}
>   	case SVM_EXIT_WRITE_DR0 ... SVM_EXIT_WRITE_DR7: {
>   		u32 dr_bits = 1<<  (exit_code - SVM_EXIT_WRITE_DR0);
> -		if (nested_vmcb->control.intercept_dr_write&  dr_bits)
> +		if (svm->nested.intercept_dr_write&  dr_bits)
>   			return 1;
>   		break;
>   	}
>   	case SVM_EXIT_EXCP_BASE ... SVM_EXIT_EXCP_BASE + 0x1f: {
>   		u32 excp_bits = 1<<  (exit_code - SVM_EXIT_EXCP_BASE);
> -		if (nested_vmcb->control.intercept_exceptions&  excp_bits)
> +		if (svm->nested.intercept_exceptions&  excp_bits)
>   			return 1;
>   		break;
>   	}
>   	default: {
>   		u64 exit_bits = 1ULL<<  (exit_code - SVM_EXIT_INTR);
>   		nsvm_printk("exit code: 0x%x\n", exit_code);
> -		if (nested_vmcb->control.intercept&  exit_bits)
> +		if (svm->nested.intercept&  exit_bits)
>   			return 1;
>   	}
>   	}
> @@ -1808,6 +1816,14 @@ static int nested_svm_vmrun(struct vcpu_svm *svm, void *arg1,
>
>   	svm->nested.vmcb_msrpm = nested_vmcb->control.msrpm_base_pa;
>
> +	/* cache intercepts */
> +	svm->nested.intercept_cr_read    = nested_vmcb->control.intercept_cr_read;
> +	svm->nested.intercept_cr_write   = nested_vmcb->control.intercept_cr_write;
> +	svm->nested.intercept_dr_read    = nested_vmcb->control.intercept_dr_read;
> +	svm->nested.intercept_dr_write   = nested_vmcb->control.intercept_dr_write;
> +	svm->nested.intercept_exceptions = nested_vmcb->control.intercept_exceptions;
> +	svm->nested.intercept            = nested_vmcb->control.intercept;
> +
>   	force_new_asid(&svm->vcpu);
>   	svm->vmcb->control.exit_int_info = nested_vmcb->control.exit_int_info;
>   	svm->vmcb->control.exit_int_info_err = nested_vmcb->control.exit_int_info_err;
>    

I don't see the benefit of this patch.  Accessing the cache is just as 
expensive as accessing the real vmcb.

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

Re: [PATCH 08/12] kvm/svm: cache nested intercepts

[Joerg Roedel]

On Wed, Jul 29, 2009 at 04:13:35PM +0300, Avi Kivity wrote:
> 
> I don't see the benefit of this patch.  Accessing the cache is just
> as expensive as accessing the real vmcb.

The benefit is that we don't have to gup and map the nested vmcb just
for checking who will take the intercept. Another reason is that with
this patch the behavior of nested SVM is more aligned to real hardware.

	Joerg

Re: [PATCH 08/12] kvm/svm: cache nested intercepts

[Avi Kivity]

On 07/29/2009 04:13 PM, Joerg Roedel wrote:
> On Wed, Jul 29, 2009 at 04:13:35PM +0300, Avi Kivity wrote:
>    
>> I don't see the benefit of this patch.  Accessing the cache is just
>> as expensive as accessing the real vmcb.
>>      
>
> The benefit is that we don't have to gup and map the nested vmcb just
> for checking who will take the intercept.

Makes sense.

> Another reason is that with
> this patch the behavior of nested SVM is more aligned to real hardware.
>    

Even more important, please put this in the commit log.

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

Re: [PATCH 08/12] kvm/svm: cache nested intercepts

[Joerg Roedel]

On Wed, Jul 29, 2009 at 04:26:02PM +0300, Avi Kivity wrote:
> On 07/29/2009 04:13 PM, Joerg Roedel wrote:
> >On Wed, Jul 29, 2009 at 04:13:35PM +0300, Avi Kivity wrote:
> >>I don't see the benefit of this patch.  Accessing the cache is just
> >>as expensive as accessing the real vmcb.
> >
> >The benefit is that we don't have to gup and map the nested vmcb just
> >for checking who will take the intercept.
> 
> Makes sense.
> 
> >Another reason is that with
> >this patch the behavior of nested SVM is more aligned to real hardware.
> 
> Even more important, please put this in the commit log.

Ok, will do.

Re: [PATCH 08/12] kvm/svm: cache nested intercepts

[Alexander Graf]

Joerg Roedel wrote:
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  arch/x86/kvm/svm.c |   30 +++++++++++++++++++++++-------
>  1 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 31467b1..9192c9a 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -86,6 +86,15 @@ struct nested_state {
>  
>  	/* gpa pointers to the real vectors */
>  	u64 vmcb_msrpm;
> +
> +	/* cache for intercepts of the guest */
> +	u16 intercept_cr_read;
> +	u16 intercept_cr_write;
> +	u16 intercept_dr_read;
> +	u16 intercept_dr_write;
> +	u32 intercept_exceptions;
> +	u64 intercept;
> +
>  };
>  
>  struct vcpu_svm {
> @@ -1459,7 +1468,6 @@ static int nested_svm_exit_handled_real(struct vcpu_svm *svm,
>  					void *arg2,
>  					void *opaque)
>  {
> -	struct vmcb *nested_vmcb = (struct vmcb *)arg1;
>   

That's not enough. You actually have to make the caller not pass it as
argument too. Otherwise a good idea.

Re: [PATCH 08/12] kvm/svm: cache nested intercepts

[Joerg Roedel]

On Wed, Jul 29, 2009 at 03:50:39PM +0200, Alexander Graf wrote:
> Joerg Roedel wrote:
> > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > ---
> >  arch/x86/kvm/svm.c |   30 +++++++++++++++++++++++-------
> >  1 files changed, 23 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index 31467b1..9192c9a 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -86,6 +86,15 @@ struct nested_state {
> >  
> >  	/* gpa pointers to the real vectors */
> >  	u64 vmcb_msrpm;
> > +
> > +	/* cache for intercepts of the guest */
> > +	u16 intercept_cr_read;
> > +	u16 intercept_cr_write;
> > +	u16 intercept_dr_read;
> > +	u16 intercept_dr_write;
> > +	u32 intercept_exceptions;
> > +	u64 intercept;
> > +
> >  };
> >  
> >  struct vcpu_svm {
> > @@ -1459,7 +1468,6 @@ static int nested_svm_exit_handled_real(struct vcpu_svm *svm,
> >  					void *arg2,
> >  					void *opaque)
> >  {
> > -	struct vmcb *nested_vmcb = (struct vmcb *)arg1;
> >   
> 
> That's not enough. You actually have to make the caller not pass it as
> argument too. Otherwise a good idea.

Yeah, true. Thats planned but not yet done. Today I just sent out what I have
so far :) This will surely be part of the second cleanup round.

	Joerg

[PATCH 09/12] kvm/svm: consolidate nested_svm_exit_handled

[Joerg Roedel]

When caching guest intercepts there is no need anymore for the
nested_svm_exit_handled_real function. So move its code into
nested_svm_exit_handled.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 9192c9a..263dfa4 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1463,15 +1463,57 @@ static int nested_svm_do(struct vcpu_svm *svm,
 	return retval;
 }
 
-static int nested_svm_exit_handled_real(struct vcpu_svm *svm,
-					void *arg1,
-					void *arg2,
-					void *opaque)
+static int nested_svm_exit_handled_msr(struct vcpu_svm *svm,
+				       void *arg1, void *arg2,
+				       void *opaque)
+{
+	struct vmcb *nested_vmcb = (struct vmcb *)arg1;
+	u8 *msrpm = (u8 *)arg2;
+        u32 t0, t1;
+	u32 msr = svm->vcpu.arch.regs[VCPU_REGS_RCX];
+	u32 param = svm->vmcb->control.exit_info_1 & 1;
+
+	if (!(nested_vmcb->control.intercept & (1ULL << INTERCEPT_MSR_PROT)))
+		return 0;
+
+	switch(msr) {
+	case 0 ... 0x1fff:
+		t0 = (msr * 2) % 8;
+		t1 = msr / 8;
+		break;
+	case 0xc0000000 ... 0xc0001fff:
+		t0 = (8192 + msr - 0xc0000000) * 2;
+		t1 = (t0 / 8);
+		t0 %= 8;
+		break;
+	case 0xc0010000 ... 0xc0011fff:
+		t0 = (16384 + msr - 0xc0010000) * 2;
+		t1 = (t0 / 8);
+		t0 %= 8;
+		break;
+	default:
+		return 1;
+		break;
+	}
+	if (msrpm[t1] & ((1 << param) << t0))
+		return 1;
+
+	return 0;
+}
+
+static int nested_svm_exit_handled(struct vcpu_svm *svm, bool kvm_override)
 {
-	bool kvm_overrides = *(bool *)opaque;
 	u32 exit_code = svm->vmcb->control.exit_code;
 
-	if (kvm_overrides) {
+	switch (svm->vmcb->control.exit_code) {
+	case SVM_EXIT_MSR:
+		return nested_svm_do(svm, svm->nested.vmcb,
+				     svm->nested.vmcb_msrpm, NULL,
+				     nested_svm_exit_handled_msr);
+	default: break;
+	}
+
+	if (kvm_override) {
 		switch (exit_code) {
 		case SVM_EXIT_INTR:
 		case SVM_EXIT_NMI:
@@ -1533,60 +1575,6 @@ static int nested_svm_exit_handled_real(struct vcpu_svm *svm,
 	return 0;
 }
 
-static int nested_svm_exit_handled_msr(struct vcpu_svm *svm,
-				       void *arg1, void *arg2,
-				       void *opaque)
-{
-	struct vmcb *nested_vmcb = (struct vmcb *)arg1;
-	u8 *msrpm = (u8 *)arg2;
-        u32 t0, t1;
-	u32 msr = svm->vcpu.arch.regs[VCPU_REGS_RCX];
-	u32 param = svm->vmcb->control.exit_info_1 & 1;
-
-	if (!(nested_vmcb->control.intercept & (1ULL << INTERCEPT_MSR_PROT)))
-		return 0;
-
-	switch(msr) {
-	case 0 ... 0x1fff:
-		t0 = (msr * 2) % 8;
-		t1 = msr / 8;
-		break;
-	case 0xc0000000 ... 0xc0001fff:
-		t0 = (8192 + msr - 0xc0000000) * 2;
-		t1 = (t0 / 8);
-		t0 %= 8;
-		break;
-	case 0xc0010000 ... 0xc0011fff:
-		t0 = (16384 + msr - 0xc0010000) * 2;
-		t1 = (t0 / 8);
-		t0 %= 8;
-		break;
-	default:
-		return 1;
-		break;
-	}
-	if (msrpm[t1] & ((1 << param) << t0))
-		return 1;
-
-	return 0;
-}
-
-static int nested_svm_exit_handled(struct vcpu_svm *svm, bool kvm_override)
-{
-	bool k = kvm_override;
-
-	switch (svm->vmcb->control.exit_code) {
-	case SVM_EXIT_MSR:
-		return nested_svm_do(svm, svm->nested.vmcb,
-				     svm->nested.vmcb_msrpm, NULL,
-				     nested_svm_exit_handled_msr);
-	default: break;
-	}
-
-	return nested_svm_do(svm, svm->nested.vmcb, 0, &k,
-			     nested_svm_exit_handled_real);
-}
-
 static inline void copy_vmcb_control_area(struct vmcb *dst_vmcb, struct vmcb *from_vmcb)
 {
 	struct vmcb_control_area *dst  = &dst_vmcb->control;
-- 
1.6.3.3

Re: [PATCH 09/12] kvm/svm: consolidate nested_svm_exit_handled

[Alexander Graf]

Joerg Roedel wrote:
> When caching guest intercepts there is no need anymore for the
> nested_svm_exit_handled_real function. So move its code into
> nested_svm_exit_handled.
>
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>   

Acked-by: Alexander Graf <agraf@suse.de>

[PATCH 10/12] kvm/svm: do nested vmexit in nested_svm_exit_handled

[Joerg Roedel]

If this function returns true a nested vmexit is required. Move that
vmexit into the nested_svm_exit_handled function. This also simplifies
the handling of nested #pf intercepts in this function.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 263dfa4..1a44e43 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1373,8 +1373,6 @@ static int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
 		svm->vmcb->control.exit_info_2 = svm->vcpu.arch.cr2;
 		if (nested_svm_exit_handled(svm, false)) {
 			nsvm_printk("VMexit -> EXCP 0x%x\n", nr);
-
-			nested_svm_vmexit(svm);
 			return 1;
 		}
 	}
@@ -1395,7 +1393,6 @@ static inline int nested_svm_intr(struct vcpu_svm *svm)
 
 		if (nested_svm_exit_handled(svm, false)) {
 			nsvm_printk("VMexit -> INTR\n");
-			nested_svm_vmexit(svm);
 			return 1;
 		}
 	}
@@ -1504,14 +1501,7 @@ static int nested_svm_exit_handled_msr(struct vcpu_svm *svm,
 static int nested_svm_exit_handled(struct vcpu_svm *svm, bool kvm_override)
 {
 	u32 exit_code = svm->vmcb->control.exit_code;
-
-	switch (svm->vmcb->control.exit_code) {
-	case SVM_EXIT_MSR:
-		return nested_svm_do(svm, svm->nested.vmcb,
-				     svm->nested.vmcb_msrpm, NULL,
-				     nested_svm_exit_handled_msr);
-	default: break;
-	}
+	bool vmexit = false;
 
 	if (kvm_override) {
 		switch (exit_code) {
@@ -1534,45 +1524,55 @@ static int nested_svm_exit_handled(struct vcpu_svm *svm, bool kvm_override)
 	}
 
 	switch (exit_code) {
+	case SVM_EXIT_MSR:
+		if (nested_svm_do(svm, svm->nested.vmcb, svm->nested.vmcb_msrpm,
+				  NULL, nested_svm_exit_handled_msr))
+			vmexit = true;
+		break;
 	case SVM_EXIT_READ_CR0 ... SVM_EXIT_READ_CR8: {
 		u32 cr_bits = 1 << (exit_code - SVM_EXIT_READ_CR0);
 		if (svm->nested.intercept_cr_read & cr_bits)
-			return 1;
+			vmexit = true;
 		break;
 	}
 	case SVM_EXIT_WRITE_CR0 ... SVM_EXIT_WRITE_CR8: {
 		u32 cr_bits = 1 << (exit_code - SVM_EXIT_WRITE_CR0);
 		if (svm->nested.intercept_cr_write & cr_bits)
-			return 1;
+			vmexit = true;
 		break;
 	}
 	case SVM_EXIT_READ_DR0 ... SVM_EXIT_READ_DR7: {
 		u32 dr_bits = 1 << (exit_code - SVM_EXIT_READ_DR0);
 		if (svm->nested.intercept_dr_read & dr_bits)
-			return 1;
+			vmexit = true;
 		break;
 	}
 	case SVM_EXIT_WRITE_DR0 ... SVM_EXIT_WRITE_DR7: {
 		u32 dr_bits = 1 << (exit_code - SVM_EXIT_WRITE_DR0);
 		if (svm->nested.intercept_dr_write & dr_bits)
-			return 1;
+			vmexit = true;
 		break;
 	}
 	case SVM_EXIT_EXCP_BASE ... SVM_EXIT_EXCP_BASE + 0x1f: {
 		u32 excp_bits = 1 << (exit_code - SVM_EXIT_EXCP_BASE);
 		if (svm->nested.intercept_exceptions & excp_bits)
-			return 1;
+			vmexit = true;
 		break;
 	}
 	default: {
 		u64 exit_bits = 1ULL << (exit_code - SVM_EXIT_INTR);
 		nsvm_printk("exit code: 0x%x\n", exit_code);
 		if (svm->nested.intercept & exit_bits)
-			return 1;
+			vmexit = true;
 	}
 	}
 
-	return 0;
+	if (vmexit) {
+		nsvm_printk("#VMEXIT reason=%04x\n", exit_code);
+		nested_svm_vmexit(svm);
+	}
+
+	return vmexit;
 }
 
 static inline void copy_vmcb_control_area(struct vmcb *dst_vmcb, struct vmcb *from_vmcb)
@@ -2331,11 +2331,8 @@ static int handle_exit(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 		nsvm_printk("nested handle_exit: 0x%x | 0x%lx | 0x%lx | 0x%lx\n",
 			    exit_code, svm->vmcb->control.exit_info_1,
 			    svm->vmcb->control.exit_info_2, svm->vmcb->save.rip);
-		if (nested_svm_exit_handled(svm, true)) {
-			nested_svm_vmexit(svm);
-			nsvm_printk("-> #VMEXIT\n");
+		if (nested_svm_exit_handled(svm, true))
 			return 1;
-		}
 	}
 
 	svm_complete_interrupts(svm);
-- 
1.6.3.3

Re: [PATCH 10/12] kvm/svm: do nested vmexit in nested_svm_exit_handled

[Alexander Graf]

Joerg Roedel wrote:
> If this function returns true a nested vmexit is required. Move that
> vmexit into the nested_svm_exit_handled function. This also simplifies
> the handling of nested #pf intercepts in this function.
>
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>   

Acked-by: Alexander Graf <agraf@suse.de>

[PATCH 11/12] kvm/svm: handle #pf intercepts in nested_svm_exit_handled directly

[Joerg Roedel]

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 1a44e43..381ed38 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1498,6 +1498,9 @@ static int nested_svm_exit_handled_msr(struct vcpu_svm *svm,
 	return 0;
 }
 
+/*
+ * If this function returns true, this #vmexit was already handled
+ */
 static int nested_svm_exit_handled(struct vcpu_svm *svm, bool kvm_override)
 {
 	u32 exit_code = svm->vmcb->control.exit_code;
@@ -1515,8 +1518,27 @@ static int nested_svm_exit_handled(struct vcpu_svm *svm, bool kvm_override)
 			break;
 		/* When we're shadowing, trap PFs */
 		case SVM_EXIT_EXCP_BASE + PF_VECTOR:
-			if (!npt_enabled)
-				return 0;
+			if (!npt_enabled) {
+				u64 fault_address;
+				u32 error_code;
+
+				fault_address  = svm->vmcb->control.exit_info_2;
+				error_code     = svm->vmcb->control.exit_info_1;
+
+				kvm_mmu_page_fault(&svm->vcpu,
+						   fault_address,
+						   error_code);
+
+				/*
+				 * If we are still nested here the pending
+				 * irqs/exceptions must be reinjected
+				 */
+				//if (is_nested(svm))
+				//	svm_complete_interrupts(svm);
+
+				return true;
+			}
+
 			break;
 		default:
 			break;
-- 
1.6.3.3

Re: [PATCH 11/12] kvm/svm: handle #pf intercepts in nested_svm_exit_handled directly

[Alexander Graf]

Joerg Roedel wrote:
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>

Acked-by: Alexander Graf <agraf@suse.de>

I'm really starting to miss comments on the commits ...

Re: [PATCH 11/12] kvm/svm: handle #pf intercepts in nested_svm_exit_handled directly

[Avi Kivity]

On 07/29/2009 05:20 PM, Alexander Graf wrote:
> Joerg Roedel wrote:
>    
>> Signed-off-by: Joerg Roedel<joerg.roedel@amd.com>
>>      
>
> Acked-by: Alexander Graf<agraf@suse.de>
>
> I'm really starting to miss comments on the commits ...
>    

Me too.  Why ack then?

Joerg, not only is this interesting, it is also complicated.  Please 
provide more detailed change logs.  The change, motivation, if a fix, 
what scenario does it fix.

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

Re: [PATCH 11/12] kvm/svm: handle #pf intercepts in nested_svm_exit_handled directly

[Alexander Graf]

Avi Kivity wrote:
> On 07/29/2009 05:20 PM, Alexander Graf wrote:
>> Joerg Roedel wrote:
>>   
>>> Signed-off-by: Joerg Roedel<joerg.roedel@amd.com>
>>>      
>>
>> Acked-by: Alexander Graf<agraf@suse.de>
>>
>> I'm really starting to miss comments on the commits ...
>>    
>
> Me too.  Why ack then?

Because the patch is fine. It's just the description that's missing.
Shouldn't I ack when the only thing I don't like is the description?


Alex

Re: [PATCH 11/12] kvm/svm: handle #pf intercepts in nested_svm_exit_handled directly

[Avi Kivity]

On 07/29/2009 05:41 PM, Alexander Graf wrote:
>> Me too.  Why ack then?
>>      
>
> Because the patch is fine. It's just the description that's missing.
> Shouldn't I ack when the only thing I don't like is the description?
>    

The description is part of the patch, an important one.

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

Re: [PATCH 11/12] kvm/svm: handle #pf intercepts in nested_svm_exit_handled directly

[Alexander Graf]

Avi Kivity wrote:
> On 07/29/2009 05:41 PM, Alexander Graf wrote:
>>> Me too.  Why ack then?
>>>      
>>
>> Because the patch is fine. It's just the description that's missing.
>> Shouldn't I ack when the only thing I don't like is the description?
>>    
>
> The description is part of the patch, an important one.

Ok, no ack for empty description patches then :-).

Alex

[PATCH 12/12] kvm/svm: simplify nested_svm_check_exception

[Joerg Roedel]

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 381ed38..f81de35 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1366,18 +1366,15 @@ static int nested_svm_check_permissions(struct vcpu_svm *svm)
 static int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
 				      bool has_error_code, u32 error_code)
 {
-	if (is_nested(svm)) {
-		svm->vmcb->control.exit_code = SVM_EXIT_EXCP_BASE + nr;
-		svm->vmcb->control.exit_code_hi = 0;
-		svm->vmcb->control.exit_info_1 = error_code;
-		svm->vmcb->control.exit_info_2 = svm->vcpu.arch.cr2;
-		if (nested_svm_exit_handled(svm, false)) {
-			nsvm_printk("VMexit -> EXCP 0x%x\n", nr);
-			return 1;
-		}
-	}
+	if (!is_nested(svm))
+		return 0;
 
-	return 0;
+	svm->vmcb->control.exit_code = SVM_EXIT_EXCP_BASE + nr;
+	svm->vmcb->control.exit_code_hi = 0;
+	svm->vmcb->control.exit_info_1 = error_code;
+	svm->vmcb->control.exit_info_2 = svm->vcpu.arch.cr2;
+
+	return nested_svm_exit_handled(svm, false);
 }
 
 static inline int nested_svm_intr(struct vcpu_svm *svm)
-- 
1.6.3.3

Re: [PATCH 12/12] kvm/svm: simplify nested_svm_check_exception

[Alexander Graf]

Joerg Roedel wrote:
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>   

My poor debug output ... :)

Acked-by: Alexander Graf <agraf@suse.de>

Re: [PATCH 0/12] Nested SVM cleanups

[Avi Kivity]

On 07/29/2009 03:56 PM, Joerg Roedel wrote:
> Hi,
>
> here is a first round of patches to clean up the code for SVM virtualization
> for KVM on AMD. There is more to clean up but since I am on vacation the rest
> of the week here is what I have queued up so far. Maybe this saves me from
> rebasing this code next week ;-)
> I tested these patches with KVM in KVM and it works stable with and without
> nested SMP. It doesn't seem to break anything.
>
>    

I seem to recall nested smp used to be broken.  Am I mistaken, or do 
these patches fix rather than clean up?

>        kvm/svm: make nested svm debugging runtime configurable
>    

Should be migrated to tracepoints.

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

Re: [PATCH 0/12] Nested SVM cleanups

[Joerg Roedel]

On Wed, Jul 29, 2009 at 04:05:50PM +0300, Avi Kivity wrote:
> On 07/29/2009 03:56 PM, Joerg Roedel wrote:
> >Hi,
> >
> >here is a first round of patches to clean up the code for SVM virtualization
> >for KVM on AMD. There is more to clean up but since I am on vacation the rest
> >of the week here is what I have queued up so far. Maybe this saves me from
> >rebasing this code next week ;-)
> >I tested these patches with KVM in KVM and it works stable with and without
> >nested SMP. It doesn't seem to break anything.
> >
> 
> I seem to recall nested smp used to be broken.  Am I mistaken, or do
> these patches fix rather than clean up?

Nested SMP was broken in one of the early version of the nested SVM
code. In the current upstream version it works quite well.

> 
> >       kvm/svm: make nested svm debugging runtime configurable
> 
> Should be migrated to tracepoints.

Ok, I tried to align debugging to the mmu debugging code. But
tracepoints are better of course.

	Joerg

Re: [PATCH 0/12] Nested SVM cleanups

[Avi Kivity]

On 07/29/2009 04:09 PM, Joerg Roedel wrote:
>>>        kvm/svm: make nested svm debugging runtime configurable
>>>        
>> Should be migrated to tracepoints.
>>      
>
> Ok, I tried to align debugging to the mmu debugging code. But
> tracepoints are better of course.
>    

I plan to switch mmu debugging to tracepoints as well (some progress has 
already been made).

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

Re: [PATCH 0/12] Nested SVM cleanups

[Avi Kivity]

On 07/29/2009 04:09 PM, Joerg Roedel wrote:
>> I seem to recall nested smp used to be broken.  Am I mistaken, or do
>> these patches fix rather than clean up?
>>      
>
> Nested SMP was broken in one of the early version of the nested SVM
> code. In the current upstream version it works quite well.
>    

In that case, the next interesting target is nested npt.  This may 
actually make nsvm perform well.  Any plans?

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

Re: [PATCH 0/12] Nested SVM cleanups

[Alexander Graf]

Avi Kivity wrote:
> On 07/29/2009 04:09 PM, Joerg Roedel wrote:
>>> I seem to recall nested smp used to be broken.  Am I mistaken, or do
>>> these patches fix rather than clean up?
>>>      
>>
>> Nested SMP was broken in one of the early version of the nested SVM
>> code. In the current upstream version it works quite well.
>>    
>
> In that case, the next interesting target is nested npt.  This may
> actually make nsvm perform well.  Any plans?

IMHO the next interesting target is nested Hyper-V :-).

But yes, nested NPT is really interesting too and more on the lines of
what Joerg's precious time should be spent on ;-). I'll try and see if I
can get nested Hyper-V running until LPC.

Alex

Re: [PATCH 0/12] Nested SVM cleanups

[Joerg Roedel]

On Wed, Jul 29, 2009 at 04:32:11PM +0300, Avi Kivity wrote:
> On 07/29/2009 04:09 PM, Joerg Roedel wrote:
> >>I seem to recall nested smp used to be broken.  Am I mistaken, or do
> >>these patches fix rather than clean up?
> >
> >Nested SMP was broken in one of the early version of the nested SVM
> >code. In the current upstream version it works quite well.
> 
> In that case, the next interesting target is nested npt.  This may
> actually make nsvm perform well.  Any plans?

Yes. Plan is to implement this when I am through with the cleanups.

	Joerg

Re: [PATCH 0/12] Nested SVM cleanups

[Avi Kivity]

On 07/29/2009 04:38 PM, Joerg Roedel wrote:
>> In that case, the next interesting target is nested npt.  This may
>> actually make nsvm perform well.  Any plans?
>>      
>
> Yes. Plan is to implement this when I am through with the cleanups.
>    

Great, this promises to be the most interesting area of development in 
kvm for those of us who don't read ppc.

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

Re: [PATCH 0/12] Nested SVM cleanups

[Joerg Roedel]

On Wed, Jul 29, 2009 at 04:48:23PM +0300, Avi Kivity wrote:
> On 07/29/2009 04:38 PM, Joerg Roedel wrote:
> >>In that case, the next interesting target is nested npt.  This may
> >>actually make nsvm perform well.  Any plans?
> >
> >Yes. Plan is to implement this when I am through with the cleanups.
> 
> Great, this promises to be the most interesting area of development
> in kvm for those of us who don't read ppc.

Performance isn't too bad even without this. At least if the host
supports nested paging :) But shadow nested paging should give nested
svm a real boost, thats very true.

	Joerg

Re: [PATCH 0/12] Nested SVM cleanups

[Avi Kivity]

On 07/29/2009 04:48 PM, Joerg Roedel wrote:
> Performance isn't too bad even without this. At least if the host
> supports nested paging :) But shadow nested paging should give nested
> svm a real boost, thats very true.
>    

Well, any workload that benefits from npt would benefit from nnpt a lot 
more.

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

Re: [PATCH 0/12] Nested SVM cleanups

[Joerg Roedel]

On Wed, Jul 29, 2009 at 04:59:48PM +0300, Avi Kivity wrote:
> On 07/29/2009 04:48 PM, Joerg Roedel wrote:
> >Performance isn't too bad even without this. At least if the host
> >supports nested paging :) But shadow nested paging should give nested
> >svm a real boost, thats very true.
> 
> Well, any workload that benefits from npt would benefit from nnpt a
> lot more.

Most complicated part will be the two dimensional software page walker.
That will be a somewhat intrusive change (at least if I do it without
code duplication). But lets see how challenging this will become...

	Joerg

Re: [PATCH 0/12] Nested SVM cleanups

[Avi Kivity]

On 07/29/2009 04:58 PM, Joerg Roedel wrote:
> On Wed, Jul 29, 2009 at 04:59:48PM +0300, Avi Kivity wrote:
>    
>> On 07/29/2009 04:48 PM, Joerg Roedel wrote:
>>      
>>> Performance isn't too bad even without this. At least if the host
>>> supports nested paging :) But shadow nested paging should give nested
>>> svm a real boost, thats very true.
>>>        
>> Well, any workload that benefits from npt would benefit from nnpt a
>> lot more.
>>      
>
> Most complicated part will be the two dimensional software page walker.
> That will be a somewhat intrusive change (at least if I do it without
> code duplication). But lets see how challenging this will become...
>    

I think if you change the memory accessors to go through 
kvm_read_guest_virt() again, you'll get it almost for free.  You'll need 
to supply a context so the accessors know which level you're accessing.

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

Re: [PATCH 0/12] Nested SVM cleanups

[Avi Kivity]

On 07/29/2009 03:56 PM, Joerg Roedel wrote:
> Hi,
>
> here is a first round of patches to clean up the code for SVM virtualization
> for KVM on AMD. There is more to clean up but since I am on vacation the rest
> of the week here is what I have queued up so far. Maybe this saves me from
> rebasing this code next week ;-)
> I tested these patches with KVM in KVM and it works stable with and without
> nested SMP. It doesn't seem to break anything.
>    

Alex, I'd appreciate a review of this.

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