Re: [Qemu-devel] [PATCH V2 5/5] vga-cirrus: Workaround during restore when using Xen.

[Stefano Stabellini]

On Fri, 6 Jan 2012, Jan Kiszka wrote:
> On 2012-01-06 08:50, Stefano Stabellini wrote:
> > On Thu, 5 Jan 2012, Jan Kiszka wrote:
> >> On 2012-01-05 15:50, Avi Kivity wrote:
> >>>> 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.
> >>
> >> To me this still sounds like a cirrus-only xen workaround that
> >> nevertheless spreads widely.
> > 
> > The first two points are still necessary even if we go with the
> > early_savevm option;
> 
> Can't really asses what the best way for Xen is to associate a memory
> region with a particular physical mapping as to be stored in the early
> vmstate. Is the memory API lacking some tag here?

ATM we have our own list of physmaps (physical mappings) maintained in
xen-all.c, but adding a xen specific field to the MemoryRegion struct
should be enough to allow us to remove the physmap list and just use
MemoryRegions for everything.

Regarding save/restore, we could save in an early_savevm record all the
MemoryRegions or just the ones with a special xen mapping.


> > the third point is a good change regardless of the qemu accelerator;
> > the only cirrus-only workaround is the last point, however it certainly
> > doesn't spread widely. It is true that other framebuffer based devices
> > would need a similar change.
> 
> The third point indicates that there is rather more generic room for
> improvements: Why should qemu reset device models before restore at all?
> If we don't run the reset handlers, we don't suffer from that useless
> memset. Testing for "Are we about to restore?" in a cirrus-only way
> looks wrong.

Fair enough.


> >> Again, what speaks against migrating the information Xen needs before
> >> creating the machine or a single device? That would only introduce a
> >> generic concept of an (optional) "early", let's call it
> >> "accelerator-related" vmstate and would allow Xen to deal with all the
> >> specifics behind the curtain.
> > 
> > I am OK with both approaches.
> > The only practical difference between the two is how we pass the xen
> > framebuffer address across save/restore: either in the device specific
> > savevm record or in a xen specific early_savevm record.
> > What is it going to be?
> 
> I'm sure you will appreciate a more generic approach than adding this to
> the device state once supporting more than cirrus over Xen.
 
Of course I do, in fact that is why I like the early_savevm approach, it
is more flexible.
On the other hand it is more work and realistically 99% of our users just
use Cirrus all the time so that is why I am also OK with the other
solution.

Avi, if you think that early_savevm is a decent solution, we'll start
working on it. Also if you would like to save and restore all the
MemoryRegions because you can see some possible future uses for it, please
let me know. Otherwise we might try to save some space and just store the
ones we need.