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

[Avi Kivity <avi@redhat.com>]

On 01/05/2012 05:53 PM, Stefano Stabellini wrote:
>
> > 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.

One way is to only call it on demand when we actually need to draw or
read the framebuffer.  Currently this will be slow since we'll search
the RAMBlock list, but soon it will be dereference of MemoryRegion.

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

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

Yup.  We could invert the order.  It's really ugly though to pass the
address of an uninitialized structure.


-- 
error compiling committee.c: too many arguments to function