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

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

On 7/5/2013 2:53 AM, Jan Beulich wrote:
>>>> On 04.07.13 at 23:48, Tim Deegan <tim@xen.org> wrote:
>> At 20:42 +0100 on 04 Jul (1372970576), Andrew Cooper wrote:
>>> On 04/07/13 20:36, suravee.suthikulpanit@amd.com wrote:
>>>> +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.  Also,
>> it might be nicer to call these svm_vm{save,load}_by_paddr() or similar
>> to make it clear what they do.
> So would I think. And the existing functions then could simply
> wrap the new ones.
>
> However, looking at the call sites of svm_vmexit_do_vm(), I don't
> think this is a host physical address in all cases: At least the uses
> from svm_vmexit_do_vm*() in svm.c suggest that these are GPAs,
> and hence can't be passed to vmload/vmsave without translation.
>
>>> 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().
> And anyway, the virtual mapping isn't being used in the resulting
> code.
>
>> I worry that we might run out of mapping
>> slots if we keep a lot of these permanent mappings around, though.
> Afaict there's a single such mapping per vCPU, so not that much to
> worry about I think.
>
> Jan
>
>
Thank you all for comments. I am sending out V2 in a separate thread.

Suravee