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

[Suravee Suthikulanit <suravee.suthikulpanit@amd.com>]

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