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

[Suravee Suthikulanit <suravee.suthikulpanit@amd.com>]

On 8/8/2013 1:47 AM, Jan Beulich wrote:
>>>> On 08.08.13 at 00:18, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote:
>> On 8/7/2013 8:17 AM, Jan Beulich wrote:
>>>>>> On 05.08.13 at 10:31, <suravee.suthikulpanit@amd.com> 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