Xen devel - Nov 2011 - [PATCH 0/5] Have a working migration with Xen

Hi all,

This patch series provide some fix to have migration working with Xen. The main
issue with Xen is that the guest RAM is not handle by QEMU.

So, first of all, the RAM will not be saved in the QEMU state file.

Then, during the initialisation that append before the migration, QEMU should
not try to allocate again the VRAM of the vga emulation, because it''s
already
there. (The guest RAM is restored before calling QEMU)

And last but not least, in QEMU with Xen, a call to set_memory (with different
address for start_addr and phys_offset) actually move the the memory, and the
only way to have a pointer to this memory is to ask a ptr with the new addr.
So, there is a patch that check for the right guest address to map.

There is probably a better way to do some of this.

Regards,


Anthony PERARD (5):
  vl.c: Do not save RAM state when Xen is used.
  xen mapcache: Check if a memory space has moved.
  Introduce premigrate RunState.
  xen: Change memory access behavior during migration.
  vga-cirrus: Workaround during restore when using Xen.

 hw/cirrus_vga.c  |   20 +++++++++++++++++---
 qapi-schema.json |    2 +-
 vl.c             |   10 ++++++++--
 xen-all.c        |   32 ++++++++++++++++++++++++++++++++
 xen-mapcache.c   |    8 ++++++--
 xen-mapcache.h   |    1 +
 6 files changed, 65 insertions(+), 8 deletions(-)

-- 
Anthony PERARD

In Xen case, the guest RAM is not handle by QEMU, and it is saved by Xen tools.
So, we just avoid to register the RAM save state handler.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 vl.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index f5afed4..e7dced2 100644
--- a/vl.c
+++ b/vl.c
@@ -3273,8 +3273,10 @@ int main(int argc, char **argv, char **envp)
     default_drive(default_sdcard, snapshot, machine->use_scsi,
                   IF_SD, 0, SD_OPTS);
 
-    register_savevm_live(NULL, "ram", 0, 4, NULL, ram_save_live,
NULL,
-                         ram_load, NULL);
+    if (!xen_enabled()) {
+        register_savevm_live(NULL, "ram", 0, 4, NULL, ram_save_live,
NULL,
+                             ram_load, NULL);
+    }
 
     if (nb_numa_nodes > 0) {
         int i;
-- 
Anthony PERARD

This patch change the xen_map_cache behavior. Before trying to map a guest
addr, mapcache will look into the list of range of address that have been moved
(physmap/set_memory). There is currently one memory space like this, the vram,
"moved" from were it''s allocated to were the guest will look
into.

This help to have a succefull migration.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 xen-all.c      |   19 +++++++++++++++++++
 xen-mapcache.c |    8 ++++++--
 xen-mapcache.h |    1 +
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/xen-all.c b/xen-all.c
index b5e28ab..40e8869 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -83,6 +83,8 @@ typedef struct XenIOState {
     Notifier exit;
 } XenIOState;
 
+XenIOState *xen_io_state = NULL;
+
 /* Xen specific function for piix pci */
 
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
@@ -218,6 +220,22 @@ static XenPhysmap *get_physmapping(XenIOState *state,
     return NULL;
 }
 
+target_phys_addr_t xen_addr_actually_is(target_phys_addr_t start_addr,
ram_addr_t size)
+{
+    if (xen_io_state) {
+        target_phys_addr_t addr = start_addr & TARGET_PAGE_MASK;
+        XenPhysmap *physmap = NULL;
+
+        QLIST_FOREACH(physmap, &xen_io_state->physmap, list) {
+            if (range_covers_byte(physmap->phys_offset, physmap->size,
addr)) {
+                return physmap->start_addr;
+            }
+        }
+    }
+
+    return start_addr;
+}
+
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 340
 static int xen_add_to_physmap(XenIOState *state,
                               target_phys_addr_t start_addr,
@@ -891,6 +909,7 @@ int xen_hvm_init(void)
     XenIOState *state;
 
     state = g_malloc0(sizeof (XenIOState));
+    xen_io_state = state;
 
     state->xce_handle = xen_xc_evtchn_open(NULL, 0);
     if (state->xce_handle == XC_HANDLER_INITIAL_VALUE) {
diff --git a/xen-mapcache.c b/xen-mapcache.c
index 7bcb86e..73927ab 100644
--- a/xen-mapcache.c
+++ b/xen-mapcache.c
@@ -191,10 +191,14 @@ uint8_t *xen_map_cache(target_phys_addr_t phys_addr,
target_phys_addr_t size,
                        uint8_t lock)
 {
     MapCacheEntry *entry, *pentry = NULL;
-    target_phys_addr_t address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
-    target_phys_addr_t address_offset = phys_addr & (MCACHE_BUCKET_SIZE -
1);
+    target_phys_addr_t address_index;
+    target_phys_addr_t address_offset;
     target_phys_addr_t __size = size;
 
+    phys_addr = xen_addr_actually_is(phys_addr, size);
+    address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
+    address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
+
     trace_xen_map_cache(phys_addr);
 
     if (address_index == mapcache->last_address_index && !lock
&& !__size) {
diff --git a/xen-mapcache.h b/xen-mapcache.h
index da874ca..5e561c5 100644
--- a/xen-mapcache.h
+++ b/xen-mapcache.h
@@ -19,6 +19,7 @@ uint8_t *xen_map_cache(target_phys_addr_t phys_addr,
target_phys_addr_t size,
 ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
 void xen_invalidate_map_cache_entry(uint8_t *buffer);
 void xen_invalidate_map_cache(void);
+target_phys_addr_t xen_addr_actually_is(target_phys_addr_t start_addr,
ram_addr_t size);
 
 #else
 
-- 
Anthony PERARD

This new state will be used by Xen functions to know QEMU will wait for a
migration. This is important to know for memory related function because the
memory is already allocated and reallocated them will not works.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 qapi-schema.json |    2 +-
 vl.c             |    4 ++++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index cb1ba77..bd77444 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -121,7 +121,7 @@
 { ''enum'': ''RunState'',
   ''data'': [ ''debug'',
''inmigrate'', ''internal-error'',
''io-error'', ''paused'',
             ''postmigrate'', ''prelaunch'',
''finish-migrate'', ''restore-vm'',
-            ''running'', ''save-vm'',
''shutdown'', ''watchdog'' ] }
+            ''running'', ''save-vm'',
''shutdown'', ''watchdog'',
''premigrate'' ] }
 
 ##
 # @StatusInfo:
diff --git a/vl.c b/vl.c
index e7dced2..a291416 100644
--- a/vl.c
+++ b/vl.c
@@ -351,8 +351,11 @@ static const RunStateTransition runstate_transitions_def[]
= {
 
     { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
     { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
+    { RUN_STATE_PRELAUNCH, RUN_STATE_PREMIGRATE },
     { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
 
+    { RUN_STATE_PREMIGRATE, RUN_STATE_INMIGRATE },
+
     { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
     { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
 
@@ -2975,6 +2978,7 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_incoming:
                 incoming = optarg;
+                runstate_set(RUN_STATE_PREMIGRATE);
                 break;
             case QEMU_OPTION_nodefaults:
                 default_serial = 0;
-- 
Anthony PERARD

Do not allocate RAM during pre-migration runstate.
Do not actually "do" set_memory during migration.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 xen-all.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/xen-all.c b/xen-all.c
index 40e8869..279651a 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -191,6 +191,11 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size)
 
     trace_xen_ram_alloc(ram_addr, size);
 
+    if (runstate_check(RUN_STATE_PREMIGRATE)) {
+        /* RAM already populated in Xen */
+        return;
+    }
+
     nr_pfn = size >> TARGET_PAGE_BITS;
     pfn_list = g_malloc(sizeof (*pfn_list) * nr_pfn);
 
@@ -271,6 +276,13 @@ go_physmap:
     DPRINTF("mapping vram to %llx - %llx, from %llx\n",
             start_addr, start_addr + size, phys_offset);
 
+    if (runstate_check(RUN_STATE_INMIGRATE)) {
+        /* The mapping should already be done and can not be done a second
+         * time. So we just add to the physmap list instead.
+         */
+        goto done;
+    }
+
     pfn = phys_offset >> TARGET_PAGE_BITS;
     start_gpfn = start_addr >> TARGET_PAGE_BITS;
     for (i = 0; i < size >> TARGET_PAGE_BITS; i++) {
@@ -285,6 +297,7 @@ go_physmap:
         }
     }
 
+done:
     physmap = g_malloc(sizeof (XenPhysmap));
 
     physmap->start_addr = start_addr;
-- 
Anthony PERARD

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@citrix.com>
---
 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);
@@ -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);
+    }
 
     s->cirrus_hidden_dac_lockindex = 5;
     s->cirrus_hidden_dac_data = 0;
-- 
Anthony PERARD

On Thu, 24 Nov 2011, Anthony PERARD wrote:
> In Xen case, the guest RAM is not handle by QEMU, and it is saved by Xen
tools.
> So, we just avoid to register the RAM save state handler.
Maybe we can unregister these handlers in the Xen specific
initialization code, before we start receiving save/restore requests,
otherwise we could have a nasty race condition.

> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  vl.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index f5afed4..e7dced2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3273,8 +3273,10 @@ int main(int argc, char **argv, char **envp)
>      default_drive(default_sdcard, snapshot, machine->use_scsi,
>                    IF_SD, 0, SD_OPTS);
>  
> -    register_savevm_live(NULL, "ram", 0, 4, NULL, ram_save_live,
NULL,
> -                         ram_load, NULL);
> +    if (!xen_enabled()) {
> +        register_savevm_live(NULL, "ram", 0, 4, NULL,
ram_save_live, NULL,
> +                             ram_load, NULL);
> +    }
>  
>      if (nb_numa_nodes > 0) {
>          int i;
> -- 
> Anthony PERARD
>

On Thu, 24 Nov 2011, Anthony PERARD wrote:
> This patch change the xen_map_cache behavior. Before trying to map a guest
> addr, mapcache will look into the list of range of address that have been
moved
> (physmap/set_memory). There is currently one memory space like this, the
vram,
> "moved" from were it''s allocated to were the guest will
look into.
> 
> This help to have a succefull migration.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  xen-all.c      |   19 +++++++++++++++++++
>  xen-mapcache.c |    8 ++++++--
>  xen-mapcache.h |    1 +
>  3 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/xen-all.c b/xen-all.c
> index b5e28ab..40e8869 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -83,6 +83,8 @@ typedef struct XenIOState {
>      Notifier exit;
>  } XenIOState;
>  
> +XenIOState *xen_io_state = NULL;
> +
>  /* Xen specific function for piix pci */
>  
>  int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
> @@ -218,6 +220,22 @@ static XenPhysmap *get_physmapping(XenIOState *state,
>      return NULL;
>  }
>  
> +target_phys_addr_t xen_addr_actually_is(target_phys_addr_t start_addr,
ram_addr_t size)
I really don''t like the name of this function. What about
xen_phys_offset_to_gaddr?

> +{
> +    if (xen_io_state) {
> +        target_phys_addr_t addr = start_addr & TARGET_PAGE_MASK;
> +        XenPhysmap *physmap = NULL;
> +
> +        QLIST_FOREACH(physmap, &xen_io_state->physmap, list) {
> +            if (range_covers_byte(physmap->phys_offset,
physmap->size, addr)) {
> +                return physmap->start_addr;
> +            }
> +        }
> +    }
> +
> +    return start_addr;
> +}
> +
>  #if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 340
>  static int xen_add_to_physmap(XenIOState *state,
>                                target_phys_addr_t start_addr,
> @@ -891,6 +909,7 @@ int xen_hvm_init(void)
>      XenIOState *state;
>  
>      state = g_malloc0(sizeof (XenIOState));
> +    xen_io_state = state;
We should pass a XenIOState pointer as an opaque pointer and
xen_phys_offset_to_gaddr as a function pointer to xen_map_cache_init,
rather than setting this global variable.

>      state->xce_handle = xen_xc_evtchn_open(NULL, 0);
>      if (state->xce_handle == XC_HANDLER_INITIAL_VALUE) {
> diff --git a/xen-mapcache.c b/xen-mapcache.c
> index 7bcb86e..73927ab 100644
> --- a/xen-mapcache.c
> +++ b/xen-mapcache.c
> @@ -191,10 +191,14 @@ uint8_t *xen_map_cache(target_phys_addr_t phys_addr,
target_phys_addr_t size,
>                         uint8_t lock)
>  {
>      MapCacheEntry *entry, *pentry = NULL;
> -    target_phys_addr_t address_index  = phys_addr >>
MCACHE_BUCKET_SHIFT;
> -    target_phys_addr_t address_offset = phys_addr &
(MCACHE_BUCKET_SIZE - 1);
> +    target_phys_addr_t address_index;
> +    target_phys_addr_t address_offset;
>      target_phys_addr_t __size = size;
>  
> +    phys_addr = xen_addr_actually_is(phys_addr, size);
> +    address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
> +    address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
> +
>      trace_xen_map_cache(phys_addr);
>  
>      if (address_index == mapcache->last_address_index && !lock
&& !__size) {
> diff --git a/xen-mapcache.h b/xen-mapcache.h
> index da874ca..5e561c5 100644
> --- a/xen-mapcache.h
> +++ b/xen-mapcache.h
> @@ -19,6 +19,7 @@ uint8_t *xen_map_cache(target_phys_addr_t phys_addr,
target_phys_addr_t size,
>  ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
>  void xen_invalidate_map_cache_entry(uint8_t *buffer);
>  void xen_invalidate_map_cache(void);
> +target_phys_addr_t xen_addr_actually_is(target_phys_addr_t start_addr,
ram_addr_t size);
>  
>  #else
>  
> -- 
> Anthony PERARD
>

On Thu, 24 Nov 2011, Anthony PERARD wrote:
> diff --git a/xen-mapcache.c b/xen-mapcache.c
> index 7bcb86e..73927ab 100644
> --- a/xen-mapcache.c
> +++ b/xen-mapcache.c
> @@ -191,10 +191,14 @@ uint8_t *xen_map_cache(target_phys_addr_t phys_addr,
target_phys_addr_t size,
>                         uint8_t lock)
>  {
>      MapCacheEntry *entry, *pentry = NULL;
> -    target_phys_addr_t address_index  = phys_addr >>
MCACHE_BUCKET_SHIFT;
> -    target_phys_addr_t address_offset = phys_addr &
(MCACHE_BUCKET_SIZE - 1);
> +    target_phys_addr_t address_index;
> +    target_phys_addr_t address_offset;
>      target_phys_addr_t __size = size;
>  
> +    phys_addr = xen_addr_actually_is(phys_addr, size);
> +    address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
> +    address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
> +
>      trace_xen_map_cache(phys_addr);
>  
>      if (address_index == mapcache->last_address_index && !lock
&& !__size) {
it is probably a good idea to call xen_addr_actually_is only in case
the normal mapping failed, for performance reasons

On Thu, Nov 24, 2011 at 17:23, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 24 Nov 2011, Anthony PERARD wrote:
>> In Xen case, the guest RAM is not handle by QEMU, and it is saved by
Xen tools.
>> So, we just avoid to register the RAM save state handler.
>
> Maybe we can unregister these handlers in the Xen specific
> initialization code, before we start receiving save/restore requests,
> otherwise we could have a nasty race condition.
One argument for my patch was: Not register at all the "ram"
save/restore function when Xen is enable make the code more clear: no
ram saved with Xen.

On the other hand, unregister the "ram" save/restore function in the
Xen code, will remove this if(xen) from the generic code.

I''m fine with both.

Regards,

-- 
Anthony PERARD

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@citrix.com>
> ---
>  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

On Thu, Nov 24, 2011 at 18:30, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
>
>> @@ -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
Actually, I think the next time that this vram will be written again
is, when the guest is actually "waked-up" and wrote something there.
Otherwise, the "restore" of the vram is done before QEMU start. So,
the memset could leave some weard stuff the screen (a white screen?).

-- 
Anthony PERARD

> On Thu, Nov 24, 2011 at 18:30, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> >
> >> @@ -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
> 
> Actually, I think the next time that this vram will be written again
> is, when the guest is actually "waked-up" and wrote something
there.
> Otherwise, the "restore" of the vram is done before QEMU start.
So,
> the memset could leave some weard stuff the screen (a white screen?).
 
So this is the succession of events:

- vga_common_init
allocates the videoram

- cirrus_reset
sets set videoram to 0xff

- load_vmstate
the old videoram is copied over, overwriting what cirrus_reset has done

therefore setting the videoram to 0xff when resuming is a waste of
efforts
--8323329-1530592377-1322221888=:31179--

On Fri, Nov 25, 2011 at 11:51, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
>> On Thu, Nov 24, 2011 at 18:30, Stefano Stabellini
>> <stefano.stabellini@eu.citrix.com> wrote:
>> >
>> >> @@ -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
>>
>> Actually, I think the next time that this vram will be written again
>> is, when the guest is actually "waked-up" and wrote something
there.
>> Otherwise, the "restore" of the vram is done before QEMU
start. So,
>> the memset could leave some weard stuff the screen (a white screen?).
>
> So this is the succession of events:
>
> - vga_common_init
> allocates the videoram
>
> - cirrus_reset
> sets set videoram to 0xff
>
> - load_vmstate
> the old videoram is copied over, overwriting what cirrus_reset has done
>
> therefore setting the videoram to 0xff when resuming is a waste of
> efforts
Ooops, I reduce my answer to the only Xen case. So I agree with you.

-- 
Anthony PERARD