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

[Suravee Suthikulanit <suravee.suthikulpanit@xxxxxxx>]

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@xxxxxxx wrote:
> > > > > From: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> > > > >
> > > > > 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@xxxxxxx>
> > > > 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



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel