From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anthony Liguori Subject: Re: [PATCH 2/9] Add helper functions for nested SVM v5 Date: Thu, 30 Oct 2008 12:56:27 -0500 Message-ID: <4909F54B.50100@codemonkey.ws> References: <1224522290-11740-1-git-send-email-agraf@suse.de> <1224522290-11740-2-git-send-email-agraf@suse.de> <1224522290-11740-3-git-send-email-agraf@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, joro@8bytes.org, avi@redhat.com To: Alexander Graf Return-path: Received: from yw-out-2324.google.com ([74.125.46.30]:44430 "EHLO yw-out-2324.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755150AbYJ3R4c (ORCPT ); Thu, 30 Oct 2008 13:56:32 -0400 Received: by yw-out-2324.google.com with SMTP id 9so302333ywe.1 for ; Thu, 30 Oct 2008 10:56:31 -0700 (PDT) In-Reply-To: <1224522290-11740-3-git-send-email-agraf@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: Alexander Graf wrote: > These are helpers for the nested SVM implementation. > > - nsvm_printk implements a debug printk variant > - nested_svm_do calls a handler that can accesses gpa-based memory > > v3 makes use of the new permission checker > > Signed-off-by: Alexander Graf > --- > arch/x86/kvm/svm.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 88 insertions(+), 0 deletions(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 4ee5376..a00421b 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -48,6 +48,16 @@ MODULE_LICENSE("GPL"); > > #define DEBUGCTL_RESERVED_BITS (~(0x3fULL)) > > +/* Turn on to get debugging output*/ > +/* #define NESTED_DEBUG */ > + > +#ifdef NESTED_DEBUG > +#define nsvm_printk(fmt, args...) printk(KERN_INFO fmt, ## args) > +#else > +static inline void nsvm_printk(char *fmt, ...) { > +} > +#endif > > I understand why you have this form, but stylistically the '{' should come on the next line. Personally, I'd stick with the standard do {} while(0) just to avoid confusion although in the context of the kernel, I think pr_debug() would be better although a little more annoying to use. > /* enable NPT for AMD64 and X86 with PAE */ > #if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE) > static bool npt_enabled = true; > @@ -1145,6 +1155,84 @@ static int vmmcall_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run) > return 1; > } > > +static int nested_svm_check_permissions(struct vcpu_svm *svm) > +{ > + if (svm->vmcb->save.cpl) { > + printk(KERN_ERR "%s: invalid cpl 0x%x at ip 0x%lx\n", > + __func__, svm->vmcb->save.cpl, kvm_rip_read(&svm->vcpu)); > + kvm_queue_exception(&svm->vcpu, GP_VECTOR); > GPFs need an error code. Do you really think a GP should be delivered before checking SVME though? I think you ought to switch the order of these checks. > + return 1; > + } > + > + if (!(svm->vcpu.arch.shadow_efer & MSR_EFER_SVME_MASK) > + || !is_paging(&svm->vcpu)) { > + kvm_queue_exception(&svm->vcpu, UD_VECTOR); > + return 1; > + } > + > + return 0; > +} > + > +static struct page *nested_svm_get_page(struct vcpu_svm *svm, u64 gpa) > +{ > + struct page *page; > + > + down_read(¤t->mm->mmap_sem); > + page = gfn_to_page(svm->vcpu.kvm, gpa >> PAGE_SHIFT); > + up_read(¤t->mm->mmap_sem); > + > + if (is_error_page(page)) { > + printk(KERN_ERR "%s: could not find page at 0x%llx\n", > + __func__, gpa); > + kvm_release_page_clean(page); > + kvm_queue_exception(&svm->vcpu, GP_VECTOR); > + return NULL; > + } > + return page; > +} > + > +static int nested_svm_do(struct vcpu_svm *svm, > + u64 arg1_gpa, u64 arg2_gpa, void *opaque, > + int (*handler)(struct vcpu_svm *svm, > + void *arg1, > + void *arg2, > + void *opaque)) > +{ > + struct page *arg1_page; > + struct page *arg2_page = NULL; > + void *arg1; > + void *arg2 = NULL; > + int retval; > + > + arg1_page = nested_svm_get_page(svm, arg1_gpa); > + if(arg1_page == NULL) > + return 1; > + > + if (arg2_gpa) { > + arg2_page = nested_svm_get_page(svm, arg2_gpa); > + if(arg2_page == NULL) { > + kvm_release_page_clean(arg1_page); > + return 1; > + } > + } > + > + arg1 = kmap_atomic(arg1_page, KM_USER0); > + if (arg2_gpa) > + arg2 = kmap_atomic(arg2_page, KM_USER1); > + > + retval = handler(svm, arg1, arg2, opaque); > + > + kunmap_atomic(arg1, KM_USER0); > + if (arg2_gpa) > + kunmap_atomic(arg2, KM_USER1); > + > + kvm_release_page_dirty(arg1_page); > + if (arg2_gpa) > + kvm_release_page_dirty(arg2_page); > + > + return retval; > +} > + > I appreciate small patches but introducing statics that aren't used is going to generate warnings when bisecting. I think that suggests your splitting things at the wrong level. Regards, Anthony Liguori > static int invalid_op_interception(struct vcpu_svm *svm, > struct kvm_run *kvm_run) > { >