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: Thu, 8 Aug 2013 10:55:31 -0500 Message-ID: <5203BF73.30704@amd.com> References: <1375691514-3426-1-git-send-email-suravee.suthikulpanit@amd.com> <520264F102000078000E9EFA@nat28.tlf.novell.com> <5202C7A5.4070304@amd.com> <52035B0C02000078000EA1B5@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.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1V7SYt-0000V1-Je for xen-devel@lists.xenproject.org; Thu, 08 Aug 2013 15:55:39 +0000 In-Reply-To: <52035B0C02000078000EA1B5@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/8/2013 1:47 AM, Jan Beulich wrote: >>>> On 08.08.13 at 00:18, Suravee Suthikulanit wrote: >> On 8/7/2013 8:17 AM, Jan Beulich wrote: >>>>>> On 05.08.13 at 10:31, wrote: >>>> --- 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. > I don't know enough about the nested HVM state handling to be > certain it's not needed. The change just looks bogus in the > context of the subject of the patch, along with the need for it not > being explained in the patch description. I'll separate the patch to make it more clear. > And of course it doesn't help things that no prior uses of hvm_svm_enabled() exist in the > tree (which I didn't realize before, as the name doesn't even > suggest this is a nested-only construct)... This macro was used in the past. However, the code was removed, but the macro still exist. I'll update the macro name. > So either you fully explain in the patch description why the change > is necessary/correct _here_, or (better imo) you break it out into a > separate patch (making it obvious that then patch title and/or > description will make clear why the change is needed). I'll update the description. Suravee