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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 24 Nov 2011, 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.

This patch looks really bad, however I am not sure how to improve it.

Let's see if I get this straight: on Xen the videoram is saved and
restored by Xen like normal guest's ram.
Therefore we skip a second videoram allocation in vga_common_init,
returning NULL.
As a consequence vga.vram_ptr is going to be invalid until
cirrus_update_memory_access is called, when the cirrus state is restored
and we finally know the position of the videoram in the guest address
space, so we can map it again.



> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
>  hw/cirrus_vga.c |   20 +++++++++++++++++---
>  1 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index c7e365b..d092c74 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,17 @@ 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) {
> +        /* Try again
> +         * The initial call to get_ram_ptr can fail with Xen during a 
> migration
> +         * because the VRAM has been moved arround.
> +         * (Moved with a set_memory and a xen_add_to_physmap)
> +         * After cirrus_update_memory_access, the Xen function will know were
> +         * the memory actually is, and fix the address before give back a
> +         * ram_ptr.
> +         */
> +        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);

I would change the comment to:

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


> @@ -2784,9 +2796,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);
> +    }
>  

this is not too bad, I suppose that the videoram is going to be written
again at restore time anyway so at least it saves some cycles

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