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

[Suravee Suthikulanit <suravee.suthikulpanit@xxxxxxx>]

On 8/7/2013 8:17 AM, Jan Beulich wrote:
> > > > On 05.08.13 at 10:31, <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>
> Tim - have all your earlier comments been addressed in this version?
>
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -1779,15 +1779,15 @@ static void
> >   svm_vmexit_do_vmrun(struct cpu_user_regs *regs,
> >                       struct vcpu *v, uint64_t vmcbaddr)
> >   {
> > -    if (!nestedhvm_enabled(v->domain)) {
> > +    if ( !nestedhvm_enabled(v->domain) || !hvm_svm_enabled(v) ) {
> Suravee, why is this change needed (here and further down)?
> Can we really get here when hvm_svm_enabled(v) returns false?
> I don't recall this having been there in earlier versions.
Basically, I double checked the logic for all the svm_vmexit_do_vmxxx to make 
sure
that the proper exception has been raised.  We had a discussion whether it 
should
returned #GP or #UD.  In this case, if the L1 vcpu does not have SVME
bit in the EFER set, it should return #UD. Otherewise, it should return #GP.

Here the hvm_svm_enabled(v) is return true when L1 guest enabled SVM in EFER.

#define hvm_svm_enabled(v) (!!((v)->arch.hvm_vcpu.guest_efer & EFER_SVME))

So, I decided to add the check here as well.  Unless you think it is not 
necessary.

> Also, if the change _is_ needed, just like done further down you
> should have taken the opportunity and fix the placement of the
> closing brace (also again later in this function).
Will take care of that if needed.
> > +static struct page_info *
> > +nsvm_get_nvmcb_page(struct vcpu *v, uint64_t vmcbaddr)
> > +{
> > +    p2m_type_t p2mt;
> > +    struct page_info *page;
> > +    struct nestedvcpu *nv = &vcpu_nestedhvm(v);
> > +
> > +    if (!nestedsvm_vmcb_map(v, vmcbaddr))
> Coding style.
OK
> > +       return NULL;
> Hard tab.
>
> Jan
OK

Thanks,

Suravee


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