From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulanit Subject: Re: [PATCH 1/1 V4] x86/AMD: Fix nested svm crash due to assertion in __virt_to_maddr Date: Wed, 31 Jul 2013 10:52:00 -0500 Message-ID: <51F932A0.1030002@amd.com> References: <1374875167-2834-1-git-send-email-suravee.suthikulpanit@amd.com> <20130729104334.GA37169@ocelot.phlegethon.org> <51F6980C.2060201@amd.com> <20130729200446.GA43983@ocelot.phlegethon.org> <51F775B8.6090601@amazon.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51F775B8.6090601@amazon.de> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Egger, Christoph" Cc: Tim Deegan , JBeulich@suse.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 7/30/2013 3:13 AM, Egger, Christoph wrote: > On 29.07.13 22:04, Tim Deegan wrote: >> At 11:27 -0500 on 29 Jul (1375097276), Suravee Suthikulanit wrote: >>> On 7/29/2013 5:43 AM, Tim Deegan wrote: >>>> Hi, >>>> >>>> At 16:46 -0500 on 26 Jul (1374857167), suravee.suthikulpanit@amd.com 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 >>>> This looks much better, but I have a few comments still: >>>> >>>>> +static struct page_info * >>>>> +_get_vmcb_page(struct domain *d, uint64_t vmcbaddr) >>>> Can you give this a name that makes it clearer that it's for nested >>>> VMCBs and not part of the handling of 'real' VMCBs? Also, please drop >>>> the leading underscore. >>> What about "get_nvmcb_page"? >> Yes, that would be good. >> >>>>> + { >>>>> + gdprintk(XENLOG_ERR, >>>>> + "VMLOAD: mapping vmcb L1-GPA to MPA failed, injecting >>>>> #UD\n"); >>>>> + ret = TRAP_invalid_op; >>>> The documentation for VMLOAD suggests TRAP_gp_fault for this case. >>> OK, I have also checked other exceptions injected in >>> svm_vmexit_do_vmsave and svm_vm_exit_do_vmload, and the following should >>> probably also changed to #GP as well. >>> >>> if (!nestedsvm_vmcb_map(v, vmcbaddr)) { >>> gdprintk(XENLOG_ERR, "VMSAVE: mapping vmcb failed, injecting >>> #UD\n"); >>> ret = TRAP_invalid_op; >>> goto inject; >>> } >> Yes, that sounds right. > Wait, documentation (http://support.amd.com/us/Processor_TechDocs/24593_APM_v2.pdf, page 470) says: > > VMLOAD and VMSAVE are available only at CPL-0 (#GP otherwise), and in protected mode with SVM enabled in EFER.SVME (#UD otherwise). > > Check the code path if EFER.SVME is guaranteed to be set. If not #UD > is correct. > > Christoph > > I also found more detail documented in the APM v3 (http://support.amd.com/us/Processor_TechDocs/24594_APM_v3.pdf). // This instruction can only be executed in protected mode with SVM enabled IF ((MSR_EFER.SVME == 0) || (!PROTECTED_MODE)) EXCEPTION [#UD] // This instruction is only allowed at CPL 0 IF (CPL != 0) EXCEPTION [#GP] IF (rAX contains an unsupported system-physical address) EXCEPTION [#GP] In this case, in function "svm_vmexit_do_vmload" and "svm_vmexit_do_vmsave", the EFER.SVME should be enabled if nestedhvm_enabled(v->domain) is true. I could also add the check to make sure that L1 guest enable the EFER.SVME (i.e. using the "hvm_svm_enabled(v)" macro). Also, the nestedsvm_vmcb_map() is trying to access VMCB which from a given address. In this case, I also think it should be #GP when it fails. Suravee