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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 5 Jan 2012, Avi Kivity wrote:
> > 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.

Right


> 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?

If it could be done, it would probably be a better solution, but I am
not so sure about the actual feasibility.


> 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.

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


> - 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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel