From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44745) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RiqFe-00039n-Md for qemu-devel@nongnu.org; Thu, 05 Jan 2012 11:33:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RiqFa-00041s-8k for qemu-devel@nongnu.org; Thu, 05 Jan 2012 11:33:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:12081) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RiqFa-00041l-0f for qemu-devel@nongnu.org; Thu, 05 Jan 2012 11:33:10 -0500 Message-ID: <4F05D0BC.70707@redhat.com> Date: Thu, 05 Jan 2012 18:33:00 +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> <4F05BF9E.7000203@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 05:53 PM, Stefano Stabellini wrote: > > > This involves: > > - adding vmstate to xen-all.c so it can migrate the xen vram address > > Easy so far. > > > > - 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) > > Besides restore we would also need to wrap machine->init() with > memory_region_transaction_begin()/memory_region_transaction_commit(), > so that all the mappings are only done at the end of restore, when we do > know the videoram address. > > This seems unfeasable to me: what about all the addresses set in the > meantime using memory_region_get_ram_ptr()? > For example s->vram_ptr set by vga_common_init during machine->init()? > If the actual memory is only allocated at the end of restore, vram_ptr > would be bogus at least until then and we would still need a way to > update it afterwards. One way is to only call it on demand when we actually need to draw or read the framebuffer. Currently this will be slow since we'll search the RAMBlock list, but soon it will be dereference of MemoryRegion. > BTW what you are suggesting is not so different from what was done by > Anthony in the last patch series he sent. See the following (ugly) patch > to cirrus-vga.c to avoid accessing s->vga.vram_ptr before restore is > completed and then update the pointer: > > http://marc.info/?l=qemu-devel&m=132346828427314&w=2 I see. Maybe we can put the xen_address in the cirrus vmstate? That way there is no ordering issue at all. Of course we have to make sure it isn't saved/restored for non-xen, but that's doable with a predicate attached to the field. > > > - updating the mapped address during restore > > > > I think this is cleaner than introducing a new migration stage, but the > > implementation may prove otherwise. > > see patch above, a good summary of the difficulties of this approach > > > > > > 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) > > The issue is that xen_ram_alloc is called by memory_region_init_ram > before xen_register_framebuffer is called (see the order of calls in > vga_common_init). > So when xen_ram_alloc is called framebuffer is still NULL: mr != > framebuffer. Yup. We could invert the order. It's really ugly though to pass the address of an uninitialized structure. -- error compiling committee.c: too many arguments to function From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH V2 5/5] vga-cirrus: Workaround during restore when using Xen. Date: Thu, 05 Jan 2012 18:33:00 +0200 Message-ID: <4F05D0BC.70707@redhat.com> 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> <4F05BF9E.7000203@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org To: Stefano Stabellini Cc: Anthony Perard , Jan Kiszka , Xen Devel , QEMU-devel List-Id: xen-devel@lists.xenproject.org On 01/05/2012 05:53 PM, Stefano Stabellini wrote: > > > This involves: > > - adding vmstate to xen-all.c so it can migrate the xen vram address > > Easy so far. > > > > - 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) > > Besides restore we would also need to wrap machine->init() with > memory_region_transaction_begin()/memory_region_transaction_commit(), > so that all the mappings are only done at the end of restore, when we do > know the videoram address. > > This seems unfeasable to me: what about all the addresses set in the > meantime using memory_region_get_ram_ptr()? > For example s->vram_ptr set by vga_common_init during machine->init()? > If the actual memory is only allocated at the end of restore, vram_ptr > would be bogus at least until then and we would still need a way to > update it afterwards. One way is to only call it on demand when we actually need to draw or read the framebuffer. Currently this will be slow since we'll search the RAMBlock list, but soon it will be dereference of MemoryRegion. > BTW what you are suggesting is not so different from what was done by > Anthony in the last patch series he sent. See the following (ugly) patch > to cirrus-vga.c to avoid accessing s->vga.vram_ptr before restore is > completed and then update the pointer: > > http://marc.info/?l=qemu-devel&m=132346828427314&w=2 I see. Maybe we can put the xen_address in the cirrus vmstate? That way there is no ordering issue at all. Of course we have to make sure it isn't saved/restored for non-xen, but that's doable with a predicate attached to the field. > > > - updating the mapped address during restore > > > > I think this is cleaner than introducing a new migration stage, but the > > implementation may prove otherwise. > > see patch above, a good summary of the difficulties of this approach > > > > > > 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) > > The issue is that xen_ram_alloc is called by memory_region_init_ram > before xen_register_framebuffer is called (see the order of calls in > vga_common_init). > So when xen_ram_alloc is called framebuffer is still NULL: mr != > framebuffer. Yup. We could invert the order. It's really ugly though to pass the address of an uninitialized structure. -- error compiling committee.c: too many arguments to function