From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Egger, Christoph" <chegger@amazon.de>
Subject: Re: [PATCH 1/1 V4] x86/AMD: Fix nested svm crash due to
 assertion in __virt_to_maddr
Date: Tue, 30 Jul 2013 10:13:44 +0200
Message-ID: <51F775B8.6090601@amazon.de>
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>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <20130729200446.GA43983@ocelot.phlegethon.org>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Tim Deegan <tim@xen.org>
Cc: JBeulich@suse.com, Suravee Suthikulanit <suravee.suthikulpanit@amd.com>, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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 <suravee.suthikulpanit@amd.com>
>>>>
>>>> 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 <suravee.suthikulpanit@amd.com>
>>> 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