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

[Tim Deegan <tim@xen.org>]

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. 

Cheers,

Tim.