From mboxrd@z Thu Jan  1 00:00:00 1970
From: Suravee Suthikulanit <suravee.suthikulpanit@amd.com>
Subject: Re: [PATCH 1/1 V4] x86/AMD: Fix nested svm crash due to
 assertion in __virt_to_maddr
Date: Mon, 29 Jul 2013 11:27:56 -0500
Message-ID: <51F6980C.2060201@amd.com>
References: <1374875167-2834-1-git-send-email-suravee.suthikulpanit@amd.com>
	<20130729104334.GA37169@ocelot.phlegethon.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <20130729104334.GA37169@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: chegger@amazon.de, JBeulich@suse.com, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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"?

>
>> +    {
>> +        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;
     }

>
>> +        goto inject;
>> +    }
>> +
>> +    svm_vmload_pa(page_to_mfn(page) << PAGE_SHIFT);
> Please use page_to_maddr() for this.
Ah... That's what I needed.  Thanks.

Suravee