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/8/2013 1:47 AM, Jan Beulich wrote:
> > > > On 08.08.13 at 00:18, Suravee Suthikulanit <suravee.suthikulpanit@xxxxxxx> 
> > > > wrote:
> > On 8/7/2013 8:17 AM, Jan Beulich wrote:
> > > > > > On 05.08.13 at 10:31, <suravee.suthikulpanit@xxxxxxx> wrote:
> > > > --- 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.
> I don't know enough about the nested HVM state handling to be
> certain it's not needed. The change just looks bogus in the
> context of the subject of the patch, along with the need for it not
> being explained in the patch description.
I'll separate the patch to make it more clear.
> And of course it doesn't help things that no prior uses of hvm_svm_enabled() 
> exist in the
> tree (which I didn't realize before, as the name doesn't even
> suggest this is a nested-only construct)...
This macro was used in the past. However, the code was removed, but the 
macro still exist. I'll update the macro name.
> So either you fully explain in the patch description why the change
> is necessary/correct _here_, or (better imo) you break it out into a
> separate patch (making it obvious that then patch title and/or
> description will make clear why the change is needed).
I'll update the description.

Suravee


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