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/7/2013 8:17 AM, Jan Beulich wrote:
>>>> On 05.08.13 at 10:31, <suravee.suthikulpanit@amd.com> wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> 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@amd.com>
> 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