From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulanit Subject: Re: [PATCH 1/1 V5] x86/AMD: Fix nested svm crash due to assertion in __virt_to_maddr Date: Wed, 7 Aug 2013 17:18:13 -0500 Message-ID: <5202C7A5.4070304@amd.com> References: <1375691514-3426-1-git-send-email-suravee.suthikulpanit@amd.com> <520264F102000078000E9EFA@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1V7C3h-0007ah-KK for xen-devel@lists.xenproject.org; Wed, 07 Aug 2013 22:18:21 +0000 In-Reply-To: <520264F102000078000E9EFA@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , chegger@amazon.de, tim@xen.org List-Id: xen-devel@lists.xenproject.org On 8/7/2013 8:17 AM, Jan Beulich wrote: >>>> On 05.08.13 at 10:31, wrote: >> From: Suravee Suthikulpanit >> >> Fix assertion in __virt_to_maddr when starting nested SVM guest >> in debug mode. Investigation has shown that svm_vmsave/svm_vmload >> make use of __pa() with invalid address. >> >> Signed-off-by: Suravee Suthikulpanit > Tim - have all your earlier comments been addressed in this version? > >> --- a/xen/arch/x86/hvm/svm/svm.c >> +++ b/xen/arch/x86/hvm/svm/svm.c >> @@ -1779,15 +1779,15 @@ static void >> svm_vmexit_do_vmrun(struct cpu_user_regs *regs, >> struct vcpu *v, uint64_t vmcbaddr) >> { >> - if (!nestedhvm_enabled(v->domain)) { >> + if ( !nestedhvm_enabled(v->domain) || !hvm_svm_enabled(v) ) { > Suravee, why is this change needed (here and further down)? > Can we really get here when hvm_svm_enabled(v) returns false? > I don't recall this having been there in earlier versions. Basically, I double checked the logic for all the svm_vmexit_do_vmxxx to make sure that the proper exception has been raised. We had a discussion whether it should returned #GP or #UD. In this case, if the L1 vcpu does not have SVME bit in the EFER set, it should return #UD. Otherewise, it should return #GP. Here the hvm_svm_enabled(v) is return true when L1 guest enabled SVM in EFER. #define hvm_svm_enabled(v) (!!((v)->arch.hvm_vcpu.guest_efer & EFER_SVME)) So, I decided to add the check here as well. Unless you think it is not necessary. > Also, if the change _is_ needed, just like done further down you > should have taken the opportunity and fix the placement of the > closing brace (also again later in this function). Will take care of that if needed. > >> +static struct page_info * >> +nsvm_get_nvmcb_page(struct vcpu *v, uint64_t vmcbaddr) >> +{ >> + p2m_type_t p2mt; >> + struct page_info *page; >> + struct nestedvcpu *nv = &vcpu_nestedhvm(v); >> + >> + if (!nestedsvm_vmcb_map(v, vmcbaddr)) > Coding style. OK > >> + return NULL; > Hard tab. > > Jan OK Thanks, Suravee