On Mon, 12 Dec 2011, Jan Kiszka wrote:
> On 2011-12-12 14:18, Stefano Stabellini wrote:
> > On Sat, 10 Dec 2011, Jan Kiszka wrote:
> >> On 2011-12-09 22:54, Anthony PERARD wrote:
> >>> During the initialisation of the machine at restore time, the access to
> >>> the
> >>> VRAM will fail because QEMU does not know yet the right guest address to
> >>> map,
> >>> so the vram_ptr is NULL.
> >>>
> >>> So this patch avoid using a NULL pointer during initialisation, and try
> >>> to get
> >>> another vram_ptr if the call failed the first time.
> >>>
> >>> Signed-off-by: Anthony PERARD <anthony.per...@citrix.com>
> >>> ---
> >>> hw/cirrus_vga.c | 16 +++++++++++++---
> >>> 1 files changed, 13 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> >>> index c7e365b..2e049c9 100644
> >>> --- a/hw/cirrus_vga.c
> >>> +++ b/hw/cirrus_vga.c
> >>> @@ -32,6 +32,7 @@
> >>> #include "console.h"
> >>> #include "vga_int.h"
> >>> #include "loader.h"
> >>> +#include "sysemu.h"
> >>>
> >>> /*
> >>> * TODO:
> >>> @@ -2696,6 +2697,13 @@ static int cirrus_post_load(void *opaque, int
> >>> version_id)
> >>> s->vga.gr[0x01] = s->cirrus_shadow_gr1 & 0x0f;
> >>>
> >>> cirrus_update_memory_access(s);
> >>> + if (!s->vga.vram_ptr) {
> >>> + /* At this point vga.vram_ptr can be invalid on Xen because we
> >>> need to
> >>> + * know the position of the videoram in the guest physical
> >>> address space in
> >>> + * order to be able to map it. After cirrus_update_memory_access
> >>> we do know
> >>> + * where the videoram is, so let's map it now. */
> >>> + s->vga.vram_ptr = memory_region_get_ram_ptr(&s->vga.vram);
> >>> + }
> >>> /* force refresh */
> >>> s->vga.graphic_mode = -1;
> >>> cirrus_update_bank_ptr(s, 0);
> >>> @@ -2784,9 +2792,11 @@ static void cirrus_reset(void *opaque)
> >>> }
> >>> s->vga.cr[0x27] = s->device_id;
> >>>
> >>> - /* Win2K seems to assume that the pattern buffer is at 0xff
> >>> - initially ! */
> >>> - memset(s->vga.vram_ptr, 0xff, s->real_vram_size);
> >>> + if (!runstate_check(RUN_STATE_PREMIGRATE)) {
> >>> + /* Win2K seems to assume that the pattern buffer is at 0xff
> >>> + initially ! */
> >>> + memset(s->vga.vram_ptr, 0xff, s->real_vram_size);
> >>> + }
> >>>
> >>> s->cirrus_hidden_dac_lockindex = 5;
> >>> s->cirrus_hidden_dac_data = 0;
> >>
> >> Is there really no way to fix this properly in the Xen layer?
> >
> > We thought about this issue for some time but we couldn't come up with
> > anything better.
> > To summarize the problem:
> >
> > - on restore the videoram has already been loaded in the guest physical
> > address space by Xen;
> >
> > - we don't know exactly where it is yet, because it has been loaded at
> > the *last* address it was mapped to (see map_linear_vram_bank that
> > moves the videoram);
> >
> > - we want to avoid allocating the videoram twice (actually the second
> > allocation would fail because of memory constraints);
> >
> >
> >
> > So the solution (I acknowledge that it looks more like an hack than a
> > solution) is:
> >
> > - wait for cirrus to load its state so that we know where the videoram
> > is;
>
> Why can't you store this information in some additional Xen-specific
> vmstate? The fact that memory_region_get_ram_ptr has to return NULL
> looks bogus to me. It's against the QEMU semantics. Other devices may
> only be fine as they are not (yet) used with Xen.
Unfortunately that cannot work because the allocation is done by
vga_common_init before any state has been loaded.
So even if we saved the physmap QLIST in a vmstate record, it wouldn't
be available yet when vga_common_init tries to allocate the memory.
