Re: [PATCH 1/1 V4] x86/AMD: Fix nested svm crash due to assertion in __virt_to_maddr

["Egger, Christoph" <chegger@amazon.de>]

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