From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44302) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RirT4-00024L-QH for qemu-devel@nongnu.org; Thu, 05 Jan 2012 12:51:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RirT0-0005pI-Gs for qemu-devel@nongnu.org; Thu, 05 Jan 2012 12:51:10 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59510) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RirT0-0005p1-6V for qemu-devel@nongnu.org; Thu, 05 Jan 2012 12:51:06 -0500 Message-ID: <4F05E301.2020808@redhat.com> Date: Thu, 05 Jan 2012 19:50:57 +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> <4F05D0BC.70707@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 07:21 PM, Stefano Stabellini wrote: > > > 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. > > Introducing a xen_address field to the cirrus vmstate is good: it would > let us avoid playing tricks with the mapcache to understand where to map > what. It would also avoid nasty ordering issues, like you say. > However we would still need a xen specific "if" in cirrus_post_load, to > update vram_ptr correctly, similarly to the first hunk of the patch > linked above. While the fear of accelerator-specific hooks in device code is healthy, at some point we have to let go. Designing elaborate abstraction layers around a specific problem with a not-so-good interface just makes the problem worse. If we have to have an if there, so be it. > > > > Yup. We could invert the order. It's really ugly though to pass the > > address of an uninitialized structure. > > OK. Maybe we have a plan :-) > > > Let me summarize what we have come up with so far: > > - we move the call to xen_register_framebuffer before > memory_region_init_ram in vga_common_init; > > - we prevent xen_ram_alloc from allocating a second framebuffer on > restore, checking for mr == framebuffer; > > - we avoid memsetting the framebuffer on restore (useless anyway, the > videoram is going to be loaded from the savefile in the general case); > > - we introduce a xen_address field to cirrus_vmstate and we use it > to map the videoram into qemu's address space and update vram_ptr in > cirrus_post_load. > > > Does it make sense? Do you agree with the approach? Yes and yes. -- error compiling committee.c: too many arguments to function