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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

In any case - explaining how nestedhvm_enabled() could end up
returning a value different from hvm_svm_enabled() would help
my understanding.

Jan


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