From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:49851) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rip7K-0001Lu-95 for qemu-devel@nongnu.org; Thu, 05 Jan 2012 10:20:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rip7I-0006HY-LF for qemu-devel@nongnu.org; Thu, 05 Jan 2012 10:20:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:27420) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rip7I-0006HK-56 for qemu-devel@nongnu.org; Thu, 05 Jan 2012 10:20:32 -0500 Message-ID: <4F05BF9E.7000203@redhat.com> Date: Thu, 05 Jan 2012 17:19:58 +0200 From: Avi Kivity MIME-Version: 1.0 References: <1323467645-24271-1-git-send-email-anthony.perard@citrix.com> <1323467645-24271-6-git-send-email-anthony.perard@citrix.com> <4EE3382D.80903@web.de> <4EE609BF.1070307@siemens.com> <4EE617BA.4030102@siemens.com> <4EEE25DA.2080400@redhat.com> <4F048B10.1060505@redhat.com> <4F059C8C.2030303@redhat.com> <4F05A684.7000509@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V2 5/5] vga-cirrus: Workaround during restore when using Xen. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: Anthony Perard , Jan Kiszka , Xen Devel , QEMU-devel On 01/05/2012 04:34 PM, Stefano Stabellini wrote: > On Thu, 5 Jan 2012, Avi Kivity wrote: > > On 01/05/2012 03:17 PM, Stefano Stabellini wrote: > > > > > The "solution" I am proposing is introducing an early_savevm set of > > > > > save/restore functions so that at restore time we can get to know at > > > > > what address the videoram is mapped into the guest address space. Once we > > > > > know the address we can remap it into qemu's address space and/or move it > > > > > to another guest physical address. > > > > > > > > Why can we not simply track it? For every MemoryRegion, have a field > > > > called xen_address which tracks its location in the Xen address space > > > > (as determined by the last call to xen_set_memory or qemu_ram_alloc). > > > > xen_address would be maintained by callbacks called from the memory API > > > > into xen-all.c. > > > > > > Nice and simple, I like it. > > > However we would still need an early_savevm mechanism to save and restore the > > > MemoryRegions, unless they already gets saved and restored somehow? > > > > MemoryRegions are instantiated by the devices, so they should be there > > (creating a MemoryRegion == calling qemu_ram_alloc() in the old days) > > If the MemoryRegions are re-created by the devices, then we need another > mechanism to find out where the videoram is. > What I am saying is that the suggestion of having a xen_address field > for every MemoryRegion would make the code cleaner but it would not > solve the save/restore issue described in the previous email. Okay, I think I understand now. You're not really allocating memory on restore, you're attaching to already allocated memory, and you need a handle to it, passed from the save side. One way is to add early-save/restore like you suggest. Another is to make the attach late. Can we defer it until after restore is complete and we know everything? This involves: - adding vmstate to xen-all.c so it can migrate the xen vram address - making sure the memory core doesn't do mappings during restore (can be done by wrapping restore with memory_region_transaction_begin()/memory_region_transaction_commit(); beneficial to normal qemu migrations as well) - updating the mapped address during restore I think this is cleaner than introducing a new migration stage, but the implementation may prove otherwise. > > > > xen_register_framebuffer() is slightly less hacky. > > I agree, however xen_register_framebuffer is called after > memory_region_init_ram, so the allocation would have been made already. xen_ram_alloc(MemoryRegion *mr) { if (in_restore && mr == framebuffer && !framebuffer_addr_known) { return; } } xen_framebuffer_address_post_load() { framebuffer_addr_known = true; if (framebuffer) { framebuffer->xen_address = framebuffer_addr; } } (ugly way of avoiding a dependency, but should work) -- error compiling committee.c: too many arguments to function