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

["Egger, Christoph" <chegger@amazon.de>]

On 04.07.13 23:48, Tim Deegan wrote:
> At 20:42 +0100 on 04 Jul (1372970576), Andrew Cooper wrote:
>> On 04/07/13 20:36, 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>
>>> ---
>>>  xen/arch/x86/hvm/svm/svm.c        |    4 ++--
>>>  xen/include/asm-x86/hvm/svm/svm.h |   14 ++++++++++++++
>>>  2 files changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>>> index acd2d49..944569a 100644
>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>> @@ -1809,7 +1809,7 @@ svm_vmexit_do_vmload(struct vmcb_struct *vmcb,
>>>          goto inject;
>>>      }
>>>  
>>> -    svm_vmload(nv->nv_vvmcx);
>>> +    nestedsvm_vmload(nv->nv_vvmcxaddr);
>>>      /* State in L1 VMCB is stale now */
>>>      v->arch.hvm_svm.vmcb_in_sync = 0;
>>>  
>>> @@ -1845,7 +1845,7 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
>>>          goto inject;
>>>      }
>>>  
>>> -    svm_vmsave(nv->nv_vvmcx);
>>> +    nestedsvm_vmsave(nv->nv_vvmcxaddr);
>>>  
>>>      __update_guest_eip(regs, inst_len);
>>>      return;
>>> diff --git a/xen/include/asm-x86/hvm/svm/svm.h b/xen/include/asm-x86/hvm/svm/svm.h
>>> index 64e7e25..909e8a1 100644
>>> --- a/xen/include/asm-x86/hvm/svm/svm.h
>>> +++ b/xen/include/asm-x86/hvm/svm/svm.h
>>> @@ -55,6 +55,20 @@ static inline void svm_vmsave(void *vmcb)
>>>          : : "a" (__pa(vmcb)) : "memory" );
>>>  }
>>>  
>>> +static inline void nestedsvm_vmload(uint64_t vmcb)
>>
>> unsigned long if this is actually an address.
> 
> IIUC this is a physical address, so paddr_t is the correct type.

Right.

> Also, it might be nicer to call these svm_vm{save,load}_by_paddr() or similar
> to make it clear what they do.

I agree.

But why did the assertion never trigger when called from elsewhere
in svm_vm{load,save}?

>> But more importantly, if virt_to_maddr() fails an assertion because the
>> virtual address is not a persistent mapping, what is going to happen
>> when the virtual mapping (potentially) changes while the vvmcx is in use?
> 
> I think the virtual mapping is ok from that point of view -- it's mapped
> with map_domain_page_global().  I worry that we might run out of mapping
> slots if we keep a lot of these permanent mappings around, though.

The number of mappings = number of guest hypervisors * number of virtual
cpus per guest hypervisor

number of guest hypervisors = number of domains excluding all domains
where nestedhvm is not used even when turned on

Christoph