From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulanit Subject: Re: [PATCH 1/1] x86/AMD: Fix nested svm crash due to assertion in __virt_to_maddr Date: Fri, 5 Jul 2013 16:38:04 -0500 Message-ID: <51D73CBC.7000305@amd.com> References: <1372966572-2703-1-git-send-email-suravee.suthikulpanit@amd.com> <51D5D040.4070501@citrix.com> <20130704214838.GA46646@ocelot.phlegethon.org> <51D697A102000078000E2E55@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51D697A102000078000E2E55@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Andrew Cooper , chegger@amazon.de, Tim Deegan , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 7/5/2013 2:53 AM, Jan Beulich wrote: >>>> On 04.07.13 at 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: >>>> +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