| Message ID | 1248872192-30881-9-git-send-email-joerg.roedel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com> --- arch/x86/kvm/svm.c | 30 +++++++++++++++++++++++------- 1 files changed, 23 insertions(+), 7 deletions(-)