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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.perard@xxxxxxxxxx>
> > ---
> >  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;

- map the videoram into qemu's address space at that point.



Anything else we came up with was even worse, for example:

- independently save the position of the videoram and then when
  vga_common_init tries to allocate it for a second time give back the
  old videoram instead;

- move back the videoram to the original position and then move it again
  to the new position.



> This looks
> highly fragile as specifically the second hunk appears unrelated to Xen.

I think that the second chuck should be in a separate patch because it
is unrelated to Xen. On restore it is useless to memset the videoram, so
for performance reasons it would be a good idea to avoid it on all
platforms. Also it happens to crash Qemu on Xen but that is another
story  ;-)

I think we should also add a comment:

"useles to memset the videoram on restore, the old videoram is going to
be copied over soon anyway"


> Also, is this the only device affected by the shortcoming? What about
> other VGA adapters e.g.?

To my knowledge (in the PC emulator) it is the only device that
allocates memory using memory_region_init_ram/memory_region_get_ram_ptr
and then moves it around. But maybe QXL does it as well?

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