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
