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

[Avi Kivity <avi@redhat.com>]

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

While the fear of accelerator-specific hooks in device code is healthy,
at some point we have to let go.  Designing elaborate abstraction layers
around a specific problem with a not-so-good interface just makes the
problem worse.  If we have to have an if there, so be it.

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

Yes and yes.

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