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

given that there is only one access to the framebuffer after
vga_common_init and before cirrus_post_load, that is the framebuffer
memset to 0xff, and given that it is actually useless to memset the
framebuffer on restore because it is going to be overwritten anyway
afterwards, I suggest keeping the second hunk of
http://marc.info/?l=qemu-devel&m=132346828427314&w=2 instead


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


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

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?

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