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

[Avi Kivity <avi@redhat.com>]

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