Xen devel - Dec 2011 - [PATCH V2 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.

- For this, we can also unregister the ram_save_live function later in xen code
  instead of having this extra "if xen" but I''m not sure of
wish one would be
  the best choice.

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.

Change since v1:
  - rename xen_addr_actually_is to xen_phys_offset_to_gaddr.
  - give phys_offset_to_gaddr as a pointer to map_cache_init
    (no more global var in xen-all.c).
  - call xen_phys_offset_to_gaddr only if the first try fail.
  - also change a comment in the last patch.

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  |   16 +++++++++++++---
 qapi-schema.json |    2 +-
 vl.c             |   10 ++++++++--
 xen-all.c        |   31 ++++++++++++++++++++++++++++++-
 xen-mapcache.c   |   22 +++++++++++++++++++---
 xen-mapcache.h   |    9 +++++++--
 6 files changed, 78 insertions(+), 12 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      |   18 +++++++++++++++++-
 xen-mapcache.c |   22 +++++++++++++++++++---
 xen-mapcache.h |    9 +++++++--
 3 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/xen-all.c b/xen-all.c
index b5e28ab..b2e9853 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -218,6 +218,22 @@ static XenPhysmap *get_physmapping(XenIOState *state,
     return NULL;
 }
 
+static target_phys_addr_t xen_phys_offset_to_gaddr(target_phys_addr_t
start_addr,
+                                                   ram_addr_t size, void
*opaque)
+{
+    target_phys_addr_t addr = start_addr & TARGET_PAGE_MASK;
+    XenIOState *xen_io_state = opaque;
+    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,
@@ -938,7 +954,7 @@ int xen_hvm_init(void)
     }
 
     /* Init RAM management */
-    xen_map_cache_init();
+    xen_map_cache_init(xen_phys_offset_to_gaddr, state);
     xen_ram_init(ram_size);
 
     qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
diff --git a/xen-mapcache.c b/xen-mapcache.c
index 7bcb86e..d5f52b2 100644
--- a/xen-mapcache.c
+++ b/xen-mapcache.c
@@ -76,6 +76,9 @@ typedef struct MapCache {
     uint8_t *last_address_vaddr;
     unsigned long max_mcache_size;
     unsigned int mcache_bucket_shift;
+
+    phys_offset_to_gaddr_t phys_offset_to_gaddr;
+    void *opaque;
 } MapCache;
 
 static MapCache *mapcache;
@@ -89,13 +92,16 @@ static inline int test_bits(int nr, int size, const unsigned
long *addr)
         return 0;
 }
 
-void xen_map_cache_init(void)
+void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
 {
     unsigned long size;
     struct rlimit rlimit_as;
 
     mapcache = g_malloc0(sizeof (MapCache));
 
+    mapcache->phys_offset_to_gaddr = f;
+    mapcache->opaque = opaque;
+
     QTAILQ_INIT(&mapcache->locked_entries);
     mapcache->last_address_index = -1;
 
@@ -191,9 +197,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;
+    bool translated = false;
+
+tryagain:
+    address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
+    address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
 
     trace_xen_map_cache(phys_addr);
 
@@ -235,6 +246,11 @@ uint8_t *xen_map_cache(target_phys_addr_t phys_addr,
target_phys_addr_t size,
     if(!test_bits(address_offset >> XC_PAGE_SHIFT, size >>
XC_PAGE_SHIFT,
                 entry->valid_mapping)) {
         mapcache->last_address_index = -1;
+        if (!translated && mapcache->phys_offset_to_gaddr) {
+            phys_addr = mapcache->phys_offset_to_gaddr(phys_addr, size,
mapcache->opaque);
+            translated = true;
+            goto tryagain;
+        }
         trace_xen_map_cache_return(NULL);
         return NULL;
     }
diff --git a/xen-mapcache.h b/xen-mapcache.h
index da874ca..70301a5 100644
--- a/xen-mapcache.h
+++ b/xen-mapcache.h
@@ -11,9 +11,13 @@
 
 #include <stdlib.h>
 
+typedef target_phys_addr_t (*phys_offset_to_gaddr_t)(target_phys_addr_t
start_addr,
+                                                     ram_addr_t size,
+                                                     void *opaque);
 #ifdef CONFIG_XEN
 
-void xen_map_cache_init(void);
+void xen_map_cache_init(phys_offset_to_gaddr_t f,
+                        void *opaque);
 uint8_t *xen_map_cache(target_phys_addr_t phys_addr, target_phys_addr_t size,
                        uint8_t lock);
 ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
@@ -22,7 +26,8 @@ void xen_invalidate_map_cache(void);
 
 #else
 
-static inline void xen_map_cache_init(void)
+static inline void xen_map_cache_init(phys_offset_to_gaddr_t f,
+                                      void *opaque)
 {
 }
 
-- 
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 b2e9853..c1fed87 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -189,6 +189,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);
 
@@ -269,6 +274,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++) {
@@ -283,6 +295,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 |   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;
-- 
Anthony PERARD

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@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? This looks
highly fragile as specifically the second hunk appears unrelated to Xen.

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

Jan

On Fri, 9 Dec 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      |   18 +++++++++++++++++-
>  xen-mapcache.c |   22 +++++++++++++++++++---
>  xen-mapcache.h |    9 +++++++--
>  3 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/xen-all.c b/xen-all.c
> index b5e28ab..b2e9853 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -218,6 +218,22 @@ static XenPhysmap *get_physmapping(XenIOState *state,
>      return NULL;
>  }
>  
> +static target_phys_addr_t xen_phys_offset_to_gaddr(target_phys_addr_t
start_addr,
> +                                                   ram_addr_t size, void
*opaque)
> +{
> +    target_phys_addr_t addr = start_addr & TARGET_PAGE_MASK;
> +    XenIOState *xen_io_state = opaque;
> +    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;
> +}
Considering that xen_phys_offset_to_gaddr can be called with addresses
that are not page aligned, you should be able to return the translated
address with the right offset in the page, rather than just the
translated address, that is incorrect.
Otherwise if start_addr has to be page aligned, just add a BUG_ON at
the beginning of the function, to spot cases in which it is not.

>  #if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 340
>  static int xen_add_to_physmap(XenIOState *state,
>                                target_phys_addr_t start_addr,
> @@ -938,7 +954,7 @@ int xen_hvm_init(void)
>      }
>  
>      /* Init RAM management */
> -    xen_map_cache_init();
> +    xen_map_cache_init(xen_phys_offset_to_gaddr, state);
>      xen_ram_init(ram_size);
This patch is better than the previous version but I think there is
still room for improvement.
For example, the only case in which xen_phys_offset_to_gaddr should
actually be used is for the videoram on restore, right?
In that case, why don''t we set xen_phys_offset_to_gaddr only on restore
rather than all cases?
We could even unset xen_phys_offset_to_gaddr after the videoram address
has been translated correctly so that it is going to be called only once.


>      qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
> diff --git a/xen-mapcache.c b/xen-mapcache.c
> index 7bcb86e..d5f52b2 100644
> --- a/xen-mapcache.c
> +++ b/xen-mapcache.c
> @@ -76,6 +76,9 @@ typedef struct MapCache {
>      uint8_t *last_address_vaddr;
>      unsigned long max_mcache_size;
>      unsigned int mcache_bucket_shift;
> +
> +    phys_offset_to_gaddr_t phys_offset_to_gaddr;
> +    void *opaque;
>  } MapCache;
>  
>  static MapCache *mapcache;
> @@ -89,13 +92,16 @@ static inline int test_bits(int nr, int size, const
unsigned long *addr)
>          return 0;
>  }
>  
> -void xen_map_cache_init(void)
> +void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
>  {
>      unsigned long size;
>      struct rlimit rlimit_as;
>  
>      mapcache = g_malloc0(sizeof (MapCache));
>  
> +    mapcache->phys_offset_to_gaddr = f;
> +    mapcache->opaque = opaque;
> +
>      QTAILQ_INIT(&mapcache->locked_entries);
>      mapcache->last_address_index = -1;
>  
> @@ -191,9 +197,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;
> +    bool translated = false;
> +
> +tryagain:
> +    address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
> +    address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
>  
>      trace_xen_map_cache(phys_addr);
>  
> @@ -235,6 +246,11 @@ uint8_t *xen_map_cache(target_phys_addr_t phys_addr,
target_phys_addr_t size,
>      if(!test_bits(address_offset >> XC_PAGE_SHIFT, size >>
XC_PAGE_SHIFT,
>                  entry->valid_mapping)) {
>          mapcache->last_address_index = -1;
> +        if (!translated && mapcache->phys_offset_to_gaddr) {
> +            phys_addr = mapcache->phys_offset_to_gaddr(phys_addr, size,
mapcache->opaque);
> +            translated = true;
> +            goto tryagain;
> +        }
>          trace_xen_map_cache_return(NULL);
>          return NULL;
>      }
It would be interesting to check how many times we end up needlessly
calling phys_offset_to_gaddr during the normal life of a VM. It should
be very few, may only one, right?

On Fri, 9 Dec 2011, Anthony PERARD wrote:
> 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 b2e9853..c1fed87 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -189,6 +189,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;
> +    }
It is probably worth printing out a message about the address and size
qemu wanted to allocated

>      nr_pfn = size >> TARGET_PAGE_BITS;
>      pfn_list = g_malloc(sizeof (*pfn_list) * nr_pfn);
>  
> @@ -269,6 +274,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;
> +    }
same here, printing a message would be useful

On Fri, 9 Dec 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.
I think it would be a good idea to split this patch into two patches:
one that avoids doing the memset on restore, because it is actually
futile in all cases; and another patch that tries to set the
vga.vram_ptr again in case it is still NULL.

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@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;

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

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.perard@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.
> 
> - 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"
That''s in fact a different story and maybe worth optimizing due to the
size of that buffer. But please do not call the state "PREMIGRATE".
It''s
rather "INCOMING[_MIGRATION]".

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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

On 2011-12-12 15:41, Stefano Stabellini wrote:
> 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.perard@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.
If you run two VMs with identical setup, one from cold start to
operational mode, the other into an incoming migration, the guest
physical memory layout the host sees is different? Hmm, maybe if you
reorder devices in the command line.

Really, I think this is something inherently incompatible with the
current memory API. If Xen has this unfixable special "requirement"
(it''s rather a design issue IMHO), adjust the API and adapt all
devices.
Hot-fixing only a single one this way is no good idea long term.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

On Mon, 12 Dec 2011, Jan Kiszka wrote:
> >>>> 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.
> 
> If you run two VMs with identical setup, one from cold start to
> operational mode, the other into an incoming migration, the guest
> physical memory layout the host sees is different? Hmm, maybe if you
> reorder devices in the command line.
Yes, it is different because the memory allocated for a specific device
(Cirrus) has been moved (map_linear_vram_bank).

> Really, I think this is something inherently incompatible with the
> current memory API. If Xen has this unfixable special
"requirement"
> (it''s rather a design issue IMHO), adjust the API and adapt all
devices.
> Hot-fixing only a single one this way is no good idea long term.
Fair enough.
What about introducing a type of savevm state that is going to be
restored before machine->init?
This way we could save and restore our physmap and we could handle
memory maps and allocations transparently.

On Mon, 12 Dec 2011, Stefano Stabellini wrote:
> > Really, I think this is something inherently incompatible with the
> > current memory API. If Xen has this unfixable special
"requirement"
> > (it''s rather a design issue IMHO), adjust the API and adapt
all devices.
> > Hot-fixing only a single one this way is no good idea long term.
> 
> Fair enough.
> What about introducing a type of savevm state that is going to be
> restored before machine->init?
> This way we could save and restore our physmap and we could handle
> memory maps and allocations transparently.
> 
A bit more context to my suggestion:

- we open the save state file and check the magic number and the
version number before machine->init();

- we add a new set of entries to the save state file that contains
early_savevm functions: they are called before machine->init();

- after reaching the end of the early_savevm functions we stop parsing
the save state file and we call machine->init();

- after machine->init() is completed we continue parsing the save state
file as usual.

On 2011-12-13 12:55, Stefano Stabellini wrote:
> On Mon, 12 Dec 2011, Stefano Stabellini wrote:
>>> Really, I think this is something inherently incompatible with the
>>> current memory API. If Xen has this unfixable special
"requirement"
>>> (it''s rather a design issue IMHO), adjust the API and
adapt all devices.
>>> Hot-fixing only a single one this way is no good idea long term.
>>
>> Fair enough.
>> What about introducing a type of savevm state that is going to be
>> restored before machine->init?
>> This way we could save and restore our physmap and we could handle
>> memory maps and allocations transparently.
>>
> 
> A bit more context to my suggestion:
> 
> - we open the save state file and check the magic number and the
> version number before machine->init();
> 
> - we add a new set of entries to the save state file that contains
> early_savevm functions: they are called before machine->init();
> 
> - after reaching the end of the early_savevm functions we stop parsing
> the save state file and we call machine->init();
> 
> - after machine->init() is completed we continue parsing the save state
> file as usual.
Hmm, just wondering if another use case for this early incoming
migration step is transferring the machine configuration so that the
receiver need not worry about synchronizing it out of band. That may
help motivating this likely not trivial change.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

On Tue, 13 Dec 2011, Jan Kiszka wrote:
> On 2011-12-13 12:55, Stefano Stabellini wrote:
> > On Mon, 12 Dec 2011, Stefano Stabellini wrote:
> >>> Really, I think this is something inherently incompatible with
the
> >>> current memory API. If Xen has this unfixable special
"requirement"
> >>> (it''s rather a design issue IMHO), adjust the API and
adapt all devices.
> >>> Hot-fixing only a single one this way is no good idea long
term.
> >>
> >> Fair enough.
> >> What about introducing a type of savevm state that is going to be
> >> restored before machine->init?
> >> This way we could save and restore our physmap and we could handle
> >> memory maps and allocations transparently.
> >>
> > 
> > A bit more context to my suggestion:
> > 
> > - we open the save state file and check the magic number and the
> > version number before machine->init();
> > 
> > - we add a new set of entries to the save state file that contains
> > early_savevm functions: they are called before machine->init();
> > 
> > - after reaching the end of the early_savevm functions we stop parsing
> > the save state file and we call machine->init();
> > 
> > - after machine->init() is completed we continue parsing the save
state
> > file as usual.
> 
> Hmm, just wondering if another use case for this early incoming
> migration step is transferring the machine configuration so that the
> receiver need not worry about synchronizing it out of band. That may
> help motivating this likely not trivial change.
Actually I was thinking the same thing. I am sure that if we had such a
thing as early_savevm, a few other use cases will certainly show up.

On 12/09/2011 03:54 PM, 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.
>
> 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);
> +    }
Why don''t you just unregister the section in the xen initialization
code?  That
way we don''t have xen_enabled()''s sprinkled all over the
place.

Regards,

Anthony Liguori
>
>       if (nb_numa_nodes>  0) {
>           int i;

On 12/09/2011 03:54 PM, Anthony PERARD wrote:
> 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>
Luiz, please Ack.  In the future, when you make QMP changes, please CC the 
appropriate maintainer.

Regards,

Anthony Liguori
> ---
>   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;

On Thu, 15 Dec 2011 09:14:00 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 12/09/2011 03:54 PM, Anthony PERARD wrote:
> > 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.
How is premigrate different from inmigrate? It looks like the same thing to me.
> >
> > Signed-off-by: Anthony PERARD<anthony.perard@citrix.com>
> 
> Luiz, please Ack.  In the future, when you make QMP changes, please CC the 
> appropriate maintainer.
I should improve my filter too.
> 
> Regards,
> 
> Anthony Liguori
> 
> > ---
> >   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;
>

On 12/12/2011 05:32 PM, Stefano Stabellini wrote:
> > Really, I think this is something inherently incompatible with the
> > current memory API. If Xen has this unfixable special
"requirement"
> > (it''s rather a design issue IMHO), adjust the API and adapt
all devices.
> > Hot-fixing only a single one this way is no good idea long term.
>
> Fair enough.
> What about introducing a type of savevm state that is going to be
> restored before machine->init?
> This way we could save and restore our physmap and we could handle
> memory maps and allocations transparently.
There is no guarantee there is a physical mapping for the framebuffer. 
A guest could unmap the framebuffer, and its display should still be
valid.  It can even update it by using the cirrus bitblt functions.

I suggest doing the following:

1. keep cirrus code unchanged
2. when the framebuffer is first mapped into physical memory (as known
by your CPUPhysMemoryClient), copy it into a temporary buffer, map the
guest memory into memory_region_get_ram_ptr(), and copy the temporary
buffer into memory_region_get_ram_ptr()
3. when the framebuffer is unmapped, do the reverse: copy the
framebuffer out, mmap() some anonymous memory into
memory_region_get_ram_ptr(), and copy the temporary buffer into
memory_region_get_ram_ptr()

We can later add optimizations to avoid the copy, but correctness before
performance.  I think currently a guest moving its cirrus BAR will
break, no?

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

On 12/13/2011 01:55 PM, Stefano Stabellini wrote:
> A bit more context to my suggestion:
>
> - we open the save state file and check the magic number and the
> version number before machine->init();
>
> - we add a new set of entries to the save state file that contains
> early_savevm functions: they are called before machine->init();
>
> - after reaching the end of the early_savevm functions we stop parsing
> the save state file and we call machine->init();
>
> - after machine->init() is completed we continue parsing the save state
> file as usual.
I think this is fragile.  In the general case you can''t describe the
framebuffer BAR with just a single number, it may be partially obscured
by another BAR.  It''s better to fix this behind the scenes.

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

On 12/15/2011 05:12 PM, Anthony Liguori wrote:
> On 12/09/2011 03:54 PM, 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.
>>
>> -    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);
>> +    }
>
> Why don''t you just unregister the section in the xen
initialization
> code?  That way we don''t have xen_enabled()''s sprinkled
all over the
> place.
It''s better to see them up front, having the magical string
"ram"
connect the two is hard to follow.

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

On Thu, 15 Dec 2011, Luiz Capitulino wrote:
> On Thu, 15 Dec 2011 09:14:00 -0600
> Anthony Liguori <anthony@codemonkey.ws> wrote:
>
> > On 12/09/2011 03:54 PM, Anthony PERARD wrote:
> > > 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.
>
> How is premigrate different from inmigrate? It looks like the same thing to
me.
The inmigrate state is used during machine initilisation. So this state
replace the prelauch state (during machine.init) when a migration will be done.

inmigrate is set only when the initilisation of the machine is over.

Regards,

-- 
Anthony PERARD

On Sun, 18 Dec 2011, Avi Kivity wrote:
> On 12/15/2011 05:12 PM, Anthony Liguori wrote:
> > On 12/09/2011 03:54 PM, 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.
> >>
> >> -    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);
> >> +    }
> >
> > Why don''t you just unregister the section in the xen
initialization
> > code?  That way we don''t have xen_enabled()''s
sprinkled all over the
> > place.
>
> It''s better to see them up front, having the magical string
"ram"
> connect the two is hard to follow.
Agreed. Unregister it in xen code was the first things I''ve done. But
I''ve changed to this with the argumment that this tell that the ram is
not saved in QEMU with Xen.

Another things could be done like a parameter to the machine to not save
the RAM.

If you prefere, I can avoid the if(!xen) in vl.c and probably give a
little headache to the one who will want to know why the ram is not in
the state file. :)

Regards,

-- 
Anthony PERARD

On Mon, 19 Dec 2011 17:27:55 +0000
Anthony PERARD <anthony.perard@citrix.com> wrote:
> On Thu, 15 Dec 2011, Luiz Capitulino wrote:
> 
> > On Thu, 15 Dec 2011 09:14:00 -0600
> > Anthony Liguori <anthony@codemonkey.ws> wrote:
> >
> > > On 12/09/2011 03:54 PM, Anthony PERARD wrote:
> > > > 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.
> >
> > How is premigrate different from inmigrate? It looks like the same
thing to me.
> 
> The inmigrate state is used during machine initilisation. So this state
> replace the prelauch state (during machine.init) when a migration will be
done.
> 
> inmigrate is set only when the initilisation of the machine is over.
Do you need both? What about setting inmigrate when initializing the
machine and using it instead?

PS: sorry for the delay, I was on vacation.

On Sun, 18 Dec 2011, Avi Kivity wrote:
> On 12/12/2011 05:32 PM, Stefano Stabellini wrote:
> > > Really, I think this is something inherently incompatible with
the
> > > current memory API. If Xen has this unfixable special
"requirement"
> > > (it''s rather a design issue IMHO), adjust the API and
adapt all devices.
> > > Hot-fixing only a single one this way is no good idea long term.
> >
> > Fair enough.
> > What about introducing a type of savevm state that is going to be
> > restored before machine->init?
> > This way we could save and restore our physmap and we could handle
> > memory maps and allocations transparently.
> 
> There is no guarantee there is a physical mapping for the framebuffer. 
> A guest could unmap the framebuffer, and its display should still be
> valid.  It can even update it by using the cirrus bitblt functions.
That is not an issue, the current code supports this case.

> I suggest doing the following:
> 
> 1. keep cirrus code unchanged
> 2. when the framebuffer is first mapped into physical memory (as known
> by your CPUPhysMemoryClient), copy it into a temporary buffer, map the
> guest memory into memory_region_get_ram_ptr(), and copy the temporary
> buffer into memory_region_get_ram_ptr()
> 3. when the framebuffer is unmapped, do the reverse: copy the
> framebuffer out, mmap() some anonymous memory into
> memory_region_get_ram_ptr(), and copy the temporary buffer into
> memory_region_get_ram_ptr()
I cannot see how this is going to fix the save/restore issue we are
trying to solve.
The problem, unfortunately very complex, is that at restore time the
videoram is already allocated at the physical address it was mapped
before the save operation. If it was not mapped, it is at the end of the
physical memory of the guest (where qemu_ram_alloc_from_ptr decides to
allocate it).

So the issue is that the videoram appears to qemu as part of the
physical memory of the guest at an unknown address.

The proposal of introducing early_savevm would easily solve this last
problem: letting us know where the videoram is. The other problem, the
fact that under Xen the videoram would be already allocated while under
native it would not, remains unsolved. 
We cannot simply allocate the videoram twice because the operation
would fail (Xen would realize that we are trying to allocate more memory
than it we are supposed to, returning an error).
However, once we know where the videoram is, we could probably figure out
a smart (see hacky) way to avoid allocating it twice without changes to
the cirrus code.


> We can later add optimizations to avoid the copy, but correctness before
> performance.  I think currently a guest moving its cirrus BAR will
> break, no?
Nope, a guest moving the cirrus BAR should work correctly even now.

On 01/04/2012 06:38 PM, Stefano Stabellini wrote:
>
> > I suggest doing the following:
> > 
> > 1. keep cirrus code unchanged
> > 2. when the framebuffer is first mapped into physical memory (as known
> > by your CPUPhysMemoryClient), copy it into a temporary buffer, map the
> > guest memory into memory_region_get_ram_ptr(), and copy the temporary
> > buffer into memory_region_get_ram_ptr()
> > 3. when the framebuffer is unmapped, do the reverse: copy the
> > framebuffer out, mmap() some anonymous memory into
> > memory_region_get_ram_ptr(), and copy the temporary buffer into
> > memory_region_get_ram_ptr()
>
> I cannot see how this is going to fix the save/restore issue we are
> trying to solve.
> The problem, unfortunately very complex, is that at restore time the
> videoram is already allocated at the physical address it was mapped
> before the save operation. If it was not mapped, it is at the end of the
> physical memory of the guest (where qemu_ram_alloc_from_ptr decides to
> allocate it).
Sorry, I don''t follow, please be specific as to which type of address
you''re referring to:

ram_addr?
physical address (as seen by guest - but if it is not mapped, what does
your last sentence mean?)
something else?
> So the issue is that the videoram appears to qemu as part of the
> physical memory of the guest at an unknown address.
>
> The proposal of introducing early_savevm would easily solve this last
> problem: letting us know where the videoram is. The other problem, the
> fact that under Xen the videoram would be already allocated while under
> native it would not, remains unsolved. 
> We cannot simply allocate the videoram twice because the operation
> would fail (Xen would realize that we are trying to allocate more memory
> than it we are supposed to, returning an error).
> However, once we know where the videoram is, we could probably figure out
> a smart (see hacky) way to avoid allocating it twice without changes to
> the cirrus code.
I''m missing some context.  Can you please explain in more detail?

Note that with the memory API changes, ram addresses are going away. 
There will not be a linear space for guest RAM.  We''ll have
(MemoryRegion *, offset) pairs that will be mapped into discontiguous
guest physical address ranges (perhaps with overlaps).

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

On Tue, Jan 3, 2012 at 19:05, Luiz Capitulino <lcapitulino@redhat.com>
wrote:
> On Mon, 19 Dec 2011 17:27:55 +0000
> Anthony PERARD <anthony.perard@citrix.com> wrote:
>
>> On Thu, 15 Dec 2011, Luiz Capitulino wrote:
>>
>> > On Thu, 15 Dec 2011 09:14:00 -0600
>> > Anthony Liguori <anthony@codemonkey.ws> wrote:
>> >
>> > > On 12/09/2011 03:54 PM, Anthony PERARD wrote:
>> > > > 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.
>> >
>> > How is premigrate different from inmigrate? It looks like the same
thing to me.
>>
>> The inmigrate state is used during machine initilisation. So this state
>> replace the prelauch state (during machine.init) when a migration will
be done.
>>
>> inmigrate is set only when the initilisation of the machine is over.
>
> Do you need both? What about setting inmigrate when initializing the
> machine and using it instead?
I suppose I can use it, by setting INMIGRATE earlier. I was afraid to
change the meaning of inmigrate, but this seems fine in the QEMU point
of view.
> PS: sorry for the delay, I was on vacation.
This is fine, me too :).

-- 
Anthony PERARD

On Wed, 4 Jan 2012, Avi Kivity wrote:
> On 01/04/2012 06:38 PM, Stefano Stabellini wrote:
> >
> > > I suggest doing the following:
> > > 
> > > 1. keep cirrus code unchanged
> > > 2. when the framebuffer is first mapped into physical memory (as
known
> > > by your CPUPhysMemoryClient), copy it into a temporary buffer,
map the
> > > guest memory into memory_region_get_ram_ptr(), and copy the
temporary
> > > buffer into memory_region_get_ram_ptr()
> > > 3. when the framebuffer is unmapped, do the reverse: copy the
> > > framebuffer out, mmap() some anonymous memory into
> > > memory_region_get_ram_ptr(), and copy the temporary buffer into
> > > memory_region_get_ram_ptr()
> >
> > I cannot see how this is going to fix the save/restore issue we are
> > trying to solve.
> > The problem, unfortunately very complex, is that at restore time the
> > videoram is already allocated at the physical address it was mapped
> > before the save operation. If it was not mapped, it is at the end of
the
> > physical memory of the guest (where qemu_ram_alloc_from_ptr decides to
> > allocate it).
> 
> Sorry, I don''t follow, please be specific as to which type of
address
> you''re referring to:
> 
> ram_addr?
> physical address (as seen by guest - but if it is not mapped, what does
> your last sentence mean?)
> something else?
ram_addr_t as returned by qemu_ram_alloc_from_ptr.

In fact on xen qemu_ram_alloc_from_ptr asks the hypervisor to add
the specified amount of memory to the guest physmap at
new_block->offset. So in a way the videoram is always visible by the
guest, initially at new_block->offset, chosen by find_ram_offset, then
at cirrus_bank_base, when map_linear_vram_bank is called.

> > So the issue is that the videoram appears to qemu as part of the
> > physical memory of the guest at an unknown address.
> >
> > The proposal of introducing early_savevm would easily solve this last
> > problem: letting us know where the videoram is. The other problem, the
> > fact that under Xen the videoram would be already allocated while
under
> > native it would not, remains unsolved. 
> > We cannot simply allocate the videoram twice because the operation
> > would fail (Xen would realize that we are trying to allocate more
memory
> > than it we are supposed to, returning an error).
> > However, once we know where the videoram is, we could probably figure
out
> > a smart (see hacky) way to avoid allocating it twice without changes
to
> > the cirrus code.
> 
> I''m missing some context.  Can you please explain in more detail?
> Note that with the memory API changes, ram addresses are going away. 
> There will not be a linear space for guest RAM.  We''ll have
> (MemoryRegion *, offset) pairs that will be mapped into discontiguous
> guest physical address ranges (perhaps with overlaps).

This is how memory is currently allocated and mapped in qemu on xen:

- qemu_ram_alloc_from_ptr asks the hypervisor to allocate memory for
the guest, the memory is added to the guest p2m (physical to machine
translation table) at the given guest physical address
(new_block->offset, as chosen by find_ram_offset);

- qemu_get_ram_ptr uses the xen mapcache to map guest physical address
ranges into qemu''s address space, so that qemu can read/write guest
memory;

- xen_set_memory, called by the memory_listener interface, effectively
moves a guest physical memory address range from one address to another.
So the memory that was initially allocated at new_block->offset, as
chosen by find_ram_offset, is going to be moved to a new destination,
section->offset_within_address_space.


So the videoram lifecycle is the following:

- qemu_ram_alloc_from_ptr allocates the videoram and adds it to the end
  of the physmap;

- qemu_get_ram_ptr maps the videoram into qemu''s address space;

- xen_set_memory moves the videoram to cirrus_bank_base;



Now let''s introduce save/restore into the picture: the videoram is part
of the guest''s memory from the hypervisor POV, so xen will take care of
saving and restoring it as part of the normal guest memory, out of
qemu''s control.
At restore time, we know that the videoram is already allocated and
mapped somewhere in the guest physical address space: it could be
cirrus_bank_base, which we don''t know yet, or the initial
new_block->offset.
A second videoram allocation by qemu_ram_alloc_from_ptr will fail
because of memory constraints enforced by the hypervisor. Trying to map
the already allocated videoram into qemu''s address space is not easy
because we don''t know where it is yet (keep in mind that
machine->init
is called before the machine restore functions).

The "solution" I am proposing is introducing an early_savevm set of
save/restore functions so that at restore time we can get to know at
what address the videoram is mapped into the guest address space. Once we
know the address we can remap it into qemu''s address space and/or move
it
to another guest physical address.

The problem of avoiding a second allocation remains, but could be
solved by passing the "name" parameter from qemu_ram_alloc_from_ptr to
xen_ram_alloc: xen_ram_alloc could avoid doing any work for anything
called "vga.vram" at restore time, and use the reference to the
already
allocated videoram instead.

On 01/05/2012 02:30 PM, Stefano Stabellini wrote:
> > >
> > > I cannot see how this is going to fix the save/restore issue we
are
> > > trying to solve.
> > > The problem, unfortunately very complex, is that at restore time
the
> > > videoram is already allocated at the physical address it was
mapped
> > > before the save operation. If it was not mapped, it is at the end
of the
> > > physical memory of the guest (where qemu_ram_alloc_from_ptr
decides to
> > > allocate it).
> > 
> > Sorry, I don''t follow, please be specific as to which type of
address
> > you''re referring to:
> > 
> > ram_addr?
> > physical address (as seen by guest - but if it is not mapped, what
does
> > your last sentence mean?)
> > something else?
>
> ram_addr_t as returned by qemu_ram_alloc_from_ptr.
>
> In fact on xen qemu_ram_alloc_from_ptr asks the hypervisor to add
> the specified amount of memory to the guest physmap at
> new_block->offset. So in a way the videoram is always visible by the
> guest, initially at new_block->offset, chosen by find_ram_offset, then
> at cirrus_bank_base, when map_linear_vram_bank is called.
Okay.  So we will need to hook this differently from the memory API.

There are two places we can hook:
- memory_region_init_ram() - similar to qemu_ram_alloc() - at region
construction time
- MemoryListener::region_add() - called the first time the region is
made visible, probably not what we want
> > > So the issue is that the videoram appears to qemu as part of the
> > > physical memory of the guest at an unknown address.
> > >
> > > The proposal of introducing early_savevm would easily solve this
last
> > > problem: letting us know where the videoram is. The other
problem, the
> > > fact that under Xen the videoram would be already allocated while
under
> > > native it would not, remains unsolved. 
> > > We cannot simply allocate the videoram twice because the
operation
> > > would fail (Xen would realize that we are trying to allocate more
memory
> > > than it we are supposed to, returning an error).
> > > However, once we know where the videoram is, we could probably
figure out
> > > a smart (see hacky) way to avoid allocating it twice without
changes to
> > > the cirrus code.
> > 
> > I''m missing some context.  Can you please explain in more
detail?
> > Note that with the memory API changes, ram addresses are going away. 
> > There will not be a linear space for guest RAM.  We''ll have
> > (MemoryRegion *, offset) pairs that will be mapped into discontiguous
> > guest physical address ranges (perhaps with overlaps).
>
>
> This is how memory is currently allocated and mapped in qemu on xen:
>
> - qemu_ram_alloc_from_ptr asks the hypervisor to allocate memory for
> the guest, the memory is added to the guest p2m (physical to machine
> translation table) at the given guest physical address
> (new_block->offset, as chosen by find_ram_offset);
>
> - qemu_get_ram_ptr uses the xen mapcache to map guest physical address
> ranges into qemu''s address space, so that qemu can read/write
guest
> memory;
>
> - xen_set_memory, called by the memory_listener interface, effectively
> moves a guest physical memory address range from one address to another.
> So the memory that was initially allocated at new_block->offset, as
> chosen by find_ram_offset, is going to be moved to a new destination,
> section->offset_within_address_space.
So, where qemu has two different address spaces (ram_addr_t and guest
physical addresses), Xen has just one, and any time the translation
between the two changes, you have to move memory around.

> So the videoram lifecycle is the following:
>
> - qemu_ram_alloc_from_ptr allocates the videoram and adds it to the end
>   of the physmap;
>
> - qemu_get_ram_ptr maps the videoram into qemu''s address space;
>
> - xen_set_memory moves the videoram to cirrus_bank_base;
>
>
>
> Now let''s introduce save/restore into the picture: the videoram is
part
> of the guest''s memory from the hypervisor POV, so xen will take
care of
> saving and restoring it as part of the normal guest memory, out of
> qemu''s control.
> At restore time, we know that the videoram is already allocated and
> mapped somewhere in the guest physical address space: it could be
> cirrus_bank_base, which we don''t know yet, or the initial
> new_block->offset.
> A second videoram allocation by qemu_ram_alloc_from_ptr will fail
> because of memory constraints enforced by the hypervisor. Trying to map
> the already allocated videoram into qemu''s address space is not
easy
> because we don''t know where it is yet (keep in mind that
machine->init
> is called before the machine restore functions).
>
> The "solution" I am proposing is introducing an early_savevm set
of
> save/restore functions so that at restore time we can get to know at
> what address the videoram is mapped into the guest address space. Once we
> know the address we can remap it into qemu''s address space and/or
move it
> to another guest physical address.
Why can we not simply track it?  For every MemoryRegion, have a field
called xen_address which tracks its location in the Xen address space
(as determined by the last call to xen_set_memory or qemu_ram_alloc). 
xen_address would be maintained by callbacks called from the memory API
into xen-all.c.
> The problem of avoiding a second allocation remains, but could be
> solved by passing the "name" parameter from
qemu_ram_alloc_from_ptr to
> xen_ram_alloc: xen_ram_alloc could avoid doing any work for anything
> called "vga.vram" at restore time, and use the reference to the
already
> allocated videoram instead.
Hacky.  The allocation is not driven by qemu then?

For the long term I suggest making qemu control the allocations (perhaps
by rpcing dom0); otherwise how can you do memory hotplug or PCI cards
with RAM (like ivshmem)?

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

On Thu, 5 Jan 2012, Avi Kivity wrote:
> On 01/05/2012 02:30 PM, Stefano Stabellini wrote:
> > > >
> > > > I cannot see how this is going to fix the save/restore issue
we are
> > > > trying to solve.
> > > > The problem, unfortunately very complex, is that at restore
time the
> > > > videoram is already allocated at the physical address it was
mapped
> > > > before the save operation. If it was not mapped, it is at
the end of the
> > > > physical memory of the guest (where qemu_ram_alloc_from_ptr
decides to
> > > > allocate it).
> > > 
> > > Sorry, I don''t follow, please be specific as to which
type of address
> > > you''re referring to:
> > > 
> > > ram_addr?
> > > physical address (as seen by guest - but if it is not mapped,
what does
> > > your last sentence mean?)
> > > something else?
> >
> > ram_addr_t as returned by qemu_ram_alloc_from_ptr.
> >
> > In fact on xen qemu_ram_alloc_from_ptr asks the hypervisor to add
> > the specified amount of memory to the guest physmap at
> > new_block->offset. So in a way the videoram is always visible by
the
> > guest, initially at new_block->offset, chosen by find_ram_offset,
then
> > at cirrus_bank_base, when map_linear_vram_bank is called.
> 
> Okay.  So we will need to hook this differently from the memory API.
> 
> There are two places we can hook:
> - memory_region_init_ram() - similar to qemu_ram_alloc() - at region
> construction time
> - MemoryListener::region_add() - called the first time the region is
> made visible, probably not what we want
memory_region_init_ram seems to be the right place to me

> > > > So the issue is that the videoram appears to qemu as part of
the
> > > > physical memory of the guest at an unknown address.
> > > >
> > > > The proposal of introducing early_savevm would easily solve
this last
> > > > problem: letting us know where the videoram is. The other
problem, the
> > > > fact that under Xen the videoram would be already allocated
while under
> > > > native it would not, remains unsolved. 
> > > > We cannot simply allocate the videoram twice because the
operation
> > > > would fail (Xen would realize that we are trying to allocate
more memory
> > > > than it we are supposed to, returning an error).
> > > > However, once we know where the videoram is, we could
probably figure out
> > > > a smart (see hacky) way to avoid allocating it twice without
changes to
> > > > the cirrus code.
> > > 
> > > I''m missing some context.  Can you please explain in
more detail?
> > > Note that with the memory API changes, ram addresses are going
away.
> > > There will not be a linear space for guest RAM.  We''ll
have
> > > (MemoryRegion *, offset) pairs that will be mapped into
discontiguous
> > > guest physical address ranges (perhaps with overlaps).
> >
> >
> > This is how memory is currently allocated and mapped in qemu on xen:
> >
> > - qemu_ram_alloc_from_ptr asks the hypervisor to allocate memory for
> > the guest, the memory is added to the guest p2m (physical to machine
> > translation table) at the given guest physical address
> > (new_block->offset, as chosen by find_ram_offset);
> >
> > - qemu_get_ram_ptr uses the xen mapcache to map guest physical address
> > ranges into qemu''s address space, so that qemu can read/write
guest
> > memory;
> >
> > - xen_set_memory, called by the memory_listener interface, effectively
> > moves a guest physical memory address range from one address to
another.
> > So the memory that was initially allocated at new_block->offset, as
> > chosen by find_ram_offset, is going to be moved to a new destination,
> > section->offset_within_address_space.
> 
> So, where qemu has two different address spaces (ram_addr_t and guest
> physical addresses), Xen has just one, and any time the translation
> between the two changes, you have to move memory around.
Yes

> > So the videoram lifecycle is the following:
> >
> > - qemu_ram_alloc_from_ptr allocates the videoram and adds it to the
end
> >   of the physmap;
> >
> > - qemu_get_ram_ptr maps the videoram into qemu''s address
space;
> >
> > - xen_set_memory moves the videoram to cirrus_bank_base;
> >
> >
> >
> > Now let''s introduce save/restore into the picture: the
videoram is part
> > of the guest''s memory from the hypervisor POV, so xen will
take care of
> > saving and restoring it as part of the normal guest memory, out of
> > qemu''s control.
> > At restore time, we know that the videoram is already allocated and
> > mapped somewhere in the guest physical address space: it could be
> > cirrus_bank_base, which we don''t know yet, or the initial
> > new_block->offset.
> > A second videoram allocation by qemu_ram_alloc_from_ptr will fail
> > because of memory constraints enforced by the hypervisor. Trying to
map
> > the already allocated videoram into qemu''s address space is
not easy
> > because we don''t know where it is yet (keep in mind that
machine->init
> > is called before the machine restore functions).
> >
> > The "solution" I am proposing is introducing an early_savevm
set of
> > save/restore functions so that at restore time we can get to know at
> > what address the videoram is mapped into the guest address space. Once
we
> > know the address we can remap it into qemu''s address space
and/or move it
> > to another guest physical address.
> 
> Why can we not simply track it?  For every MemoryRegion, have a field
> called xen_address which tracks its location in the Xen address space
> (as determined by the last call to xen_set_memory or qemu_ram_alloc). 
> xen_address would be maintained by callbacks called from the memory API
> into xen-all.c.
Nice and simple, I like it.
However we would still need an early_savevm mechanism to save and restore the
MemoryRegions, unless they already gets saved and restored somehow?
Maybe saving and restoring the list of MemoryRegions could be useful for
the generic case too?

> > The problem of avoiding a second allocation remains, but could be
> > solved by passing the "name" parameter from
qemu_ram_alloc_from_ptr to
> > xen_ram_alloc: xen_ram_alloc could avoid doing any work for anything
> > called "vga.vram" at restore time, and use the reference to
the already
> > allocated videoram instead.
> 
> Hacky
Yes :/

> The allocation is not driven by qemu then?
At restore time, it is not.

> For the long term I suggest making qemu control the allocations (perhaps
> by rpcing dom0); otherwise how can you do memory hotplug or PCI cards
> with RAM (like ivshmem)?
It is only the videoram (well, everything allocated with
qemu_ram_alloc_from_ptr actually) and only at restore time, because
the memory in question is being considered normal guest memory and
therefore it is saved and restored by the hypervisor.
Otherwise Qemu is the one that triggers these allocations, so there are
no issues with memory hotplug and pci passthrough.

On 01/05/2012 03:17 PM, Stefano Stabellini wrote:
> > > The "solution" I am proposing is introducing an
early_savevm set of
> > > save/restore functions so that at restore time we can get to know
at
> > > what address the videoram is mapped into the guest address space.
Once we
> > > know the address we can remap it into qemu''s address
space and/or move it
> > > to another guest physical address.
> > 
> > Why can we not simply track it?  For every MemoryRegion, have a field
> > called xen_address which tracks its location in the Xen address space
> > (as determined by the last call to xen_set_memory or qemu_ram_alloc). 
> > xen_address would be maintained by callbacks called from the memory
API
> > into xen-all.c.
>
> Nice and simple, I like it.
> However we would still need an early_savevm mechanism to save and restore
the
> MemoryRegions, unless they already gets saved and restored somehow?
MemoryRegions are instantiated by the devices, so they should be there
(creating a MemoryRegion == calling qemu_ram_alloc() in the old days)
> Maybe saving and restoring the list of MemoryRegions could be useful for
> the generic case too?
Unneeded, since they''re an integral part of the devices.  However, it
would be good to have a list of the devices so we could send that over
instead of relying on invoking qemu with the same command-line arguments
on both sides - but that''s something that qom is already tackling.
> > > The problem of avoiding a second allocation remains, but could be
> > > solved by passing the "name" parameter from
qemu_ram_alloc_from_ptr to
> > > xen_ram_alloc: xen_ram_alloc could avoid doing any work for
anything
> > > called "vga.vram" at restore time, and use the
reference to the already
> > > allocated videoram instead.
> > 
> > Hacky
>
> Yes :/
xen_register_framebuffer() is slightly less hacky.
> > The allocation is not driven by qemu then?
>
> At restore time, it is not.
>
>
> > For the long term I suggest making qemu control the allocations
(perhaps
> > by rpcing dom0); otherwise how can you do memory hotplug or PCI cards
> > with RAM (like ivshmem)?
>
> It is only the videoram (well, everything allocated with
> qemu_ram_alloc_from_ptr actually) and only at restore time, because
> the memory in question is being considered normal guest memory and
> therefore it is saved and restored by the hypervisor.
> Otherwise Qemu is the one that triggers these allocations, so there are
> no issues with memory hotplug and pci passthrough.
Okay.

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

On Thu, 5 Jan 2012, Avi Kivity wrote:
> On 01/05/2012 03:17 PM, Stefano Stabellini wrote:
> > > > The "solution" I am proposing is introducing an
early_savevm set of
> > > > save/restore functions so that at restore time we can get to
know at
> > > > what address the videoram is mapped into the guest address
space. Once we
> > > > know the address we can remap it into qemu''s
address space and/or move it
> > > > to another guest physical address.
> > > 
> > > Why can we not simply track it?  For every MemoryRegion, have a
field
> > > called xen_address which tracks its location in the Xen address
space
> > > (as determined by the last call to xen_set_memory or
qemu_ram_alloc).
> > > xen_address would be maintained by callbacks called from the
memory API
> > > into xen-all.c.
> >
> > Nice and simple, I like it.
> > However we would still need an early_savevm mechanism to save and
restore the
> > MemoryRegions, unless they already gets saved and restored somehow?
> 
> MemoryRegions are instantiated by the devices, so they should be there
> (creating a MemoryRegion == calling qemu_ram_alloc() in the old days)
If the MemoryRegions are re-created by the devices, then we need another
mechanism to find out where the videoram is.
What I am saying is that the suggestion of having a xen_address field
for every MemoryRegion would make the code cleaner but it would not
solve the save/restore issue described in the previous email.

> > Maybe saving and restoring the list of MemoryRegions could be useful
for
> > the generic case too?
> 
> Unneeded, since they''re an integral part of the devices.  However,
it
> would be good to have a list of the devices so we could send that over
> instead of relying on invoking qemu with the same command-line arguments
> on both sides - but that''s something that qom is already tackling.
> 
> > > > The problem of avoiding a second allocation remains, but
could be
> > > > solved by passing the "name" parameter from
qemu_ram_alloc_from_ptr to
> > > > xen_ram_alloc: xen_ram_alloc could avoid doing any work for
anything
> > > > called "vga.vram" at restore time, and use the
reference to the already
> > > > allocated videoram instead.
> > > 
> > > Hacky
> >
> > Yes :/
> 
> xen_register_framebuffer() is slightly less hacky.
I agree, however xen_register_framebuffer is called after
memory_region_init_ram, so the allocation would have been made already.

On 01/05/2012 04:34 PM, Stefano Stabellini wrote:
> On Thu, 5 Jan 2012, Avi Kivity wrote:
> > On 01/05/2012 03:17 PM, Stefano Stabellini wrote:
> > > > > The "solution" I am proposing is introducing
an early_savevm set of
> > > > > save/restore functions so that at restore time we can
get to know at
> > > > > what address the videoram is mapped into the guest
address space. Once we
> > > > > know the address we can remap it into qemu''s
address space and/or move it
> > > > > to another guest physical address.
> > > > 
> > > > Why can we not simply track it?  For every MemoryRegion,
have a field
> > > > called xen_address which tracks its location in the Xen
address space
> > > > (as determined by the last call to xen_set_memory or
qemu_ram_alloc).
> > > > xen_address would be maintained by callbacks called from the
memory API
> > > > into xen-all.c.
> > >
> > > Nice and simple, I like it.
> > > However we would still need an early_savevm mechanism to save and
restore the
> > > MemoryRegions, unless they already gets saved and restored
somehow?
> > 
> > MemoryRegions are instantiated by the devices, so they should be there
> > (creating a MemoryRegion == calling qemu_ram_alloc() in the old days)
>
> If the MemoryRegions are re-created by the devices, then we need another
> mechanism to find out where the videoram is.
> What I am saying is that the suggestion of having a xen_address field
> for every MemoryRegion would make the code cleaner but it would not
> solve the save/restore issue described in the previous email.
Okay, I think I understand now.

You''re not really allocating memory on restore, you''re
attaching to
already allocated memory, and you need a handle to it, passed from the
save side.

One way is to add early-save/restore like you suggest.  Another is to
make the attach late.  Can we defer it until after restore is complete
and we know everything?

This involves:
- adding vmstate to xen-all.c so it can migrate the xen vram address
- making sure the memory core doesn''t do mappings during restore (can
be
done by wrapping restore with
memory_region_transaction_begin()/memory_region_transaction_commit();
beneficial to normal qemu migrations as well)
- updating the mapped address during restore

I think this is cleaner than introducing a new migration stage, but the
implementation may prove otherwise.
> > 
> > xen_register_framebuffer() is slightly less hacky.
>
> I agree, however xen_register_framebuffer is called after
> memory_region_init_ram, so the allocation would have been made already.
xen_ram_alloc(MemoryRegion *mr)
{
    if (in_restore && mr == framebuffer &&
!framebuffer_addr_known) {
        return;
    }
}

xen_framebuffer_address_post_load()
{
    framebuffer_addr_known = true;
    if (framebuffer) {
        framebuffer->xen_address = framebuffer_addr;
    }
}

(ugly way of avoiding a dependency, but should work)

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

On Thu, 5 Jan 2012, Avi Kivity wrote:
> > If the MemoryRegions are re-created by the devices, then we need
another
> > mechanism to find out where the videoram is.
> > What I am saying is that the suggestion of having a xen_address field
> > for every MemoryRegion would make the code cleaner but it would not
> > solve the save/restore issue described in the previous email.
> 
> Okay, I think I understand now.
> 
> You''re not really allocating memory on restore, you''re
attaching to
> already allocated memory, and you need a handle to it, passed from the
> save side.
Right

> One way is to add early-save/restore like you suggest.  Another is to
> make the attach late.  Can we defer it until after restore is complete
> and we know everything?
If it could be done, it would probably be a better solution, but I am
not so sure about the actual feasibility.

> This involves:
> - adding vmstate to xen-all.c so it can migrate the xen vram address
Easy so far.

> - making sure the memory core doesn''t do mappings during restore
(can be
> done by wrapping restore with
> memory_region_transaction_begin()/memory_region_transaction_commit();
> beneficial to normal qemu migrations as well)
Besides restore we would also need to wrap machine->init() with
memory_region_transaction_begin()/memory_region_transaction_commit(),
so that all the mappings are only done at the end of restore, when we do
know the videoram address.

This seems unfeasable to me: what about all the addresses set in the
meantime using memory_region_get_ram_ptr()?
For example s->vram_ptr set by vga_common_init during machine->init()?
If the actual memory is only allocated at the end of restore, vram_ptr
would be bogus at least until then and we would still need a way to
update it afterwards.

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

> - updating the mapped address during restore
> 
> I think this is cleaner than introducing a new migration stage, but the
> implementation may prove otherwise.
see patch above, a good summary of the difficulties of this approach

> > > xen_register_framebuffer() is slightly less hacky.
> >
> > I agree, however xen_register_framebuffer is called after
> > memory_region_init_ram, so the allocation would have been made
already.
> 
> xen_ram_alloc(MemoryRegion *mr)
> {
>     if (in_restore && mr == framebuffer &&
!framebuffer_addr_known) {
>         return;
>     }
> }
> 
> xen_framebuffer_address_post_load()
> {
>     framebuffer_addr_known = true;
>     if (framebuffer) {
>         framebuffer->xen_address = framebuffer_addr;
>     }
> }
> 
> (ugly way of avoiding a dependency, but should work)
 
The issue is that xen_ram_alloc is called by memory_region_init_ram
before xen_register_framebuffer is called (see the order of calls in
vga_common_init).
So when xen_ram_alloc is called framebuffer is still NULL: mr !framebuffer.

On 01/05/2012 05:53 PM, Stefano Stabellini wrote:
>
> > This involves:
> > - adding vmstate to xen-all.c so it can migrate the xen vram address
>
> Easy so far.
>
>
> > - making sure the memory core doesn''t do mappings during
restore (can be
> > done by wrapping restore with
> > memory_region_transaction_begin()/memory_region_transaction_commit();
> > beneficial to normal qemu migrations as well)
>
> Besides restore we would also need to wrap machine->init() with
> memory_region_transaction_begin()/memory_region_transaction_commit(),
> so that all the mappings are only done at the end of restore, when we do
> know the videoram address.
>
> This seems unfeasable to me: what about all the addresses set in the
> meantime using memory_region_get_ram_ptr()?
> For example s->vram_ptr set by vga_common_init during
machine->init()?
> If the actual memory is only allocated at the end of restore, vram_ptr
> would be bogus at least until then and we would still need a way to
> update it afterwards.
One way is to only call it on demand when we actually need to draw or
read the framebuffer.  Currently this will be slow since we''ll search
the RAMBlock list, but soon it will be dereference of MemoryRegion.
> 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.
>
> > - updating the mapped address during restore
> > 
> > I think this is cleaner than introducing a new migration stage, but
the
> > implementation may prove otherwise.
>
> see patch above, a good summary of the difficulties of this approach
>
>
> > > > xen_register_framebuffer() is slightly less hacky.
> > >
> > > I agree, however xen_register_framebuffer is called after
> > > memory_region_init_ram, so the allocation would have been made
already.
> > 
> > xen_ram_alloc(MemoryRegion *mr)
> > {
> >     if (in_restore && mr == framebuffer &&
!framebuffer_addr_known) {
> >         return;
> >     }
> > }
> > 
> > xen_framebuffer_address_post_load()
> > {
> >     framebuffer_addr_known = true;
> >     if (framebuffer) {
> >         framebuffer->xen_address = framebuffer_addr;
> >     }
> > }
> > 
> > (ugly way of avoiding a dependency, but should work)
>  
> The issue is that xen_ram_alloc is called by memory_region_init_ram
> before xen_register_framebuffer is called (see the order of calls in
> vga_common_init).
> So when xen_ram_alloc is called framebuffer is still NULL: mr !>
framebuffer.
Yup.  We could invert the order.  It''s really ugly though to pass the
address of an uninitialized structure.


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

On Thu, 5 Jan 2012, Avi Kivity wrote:
> On 01/05/2012 05:53 PM, Stefano Stabellini wrote:
> >
> > > This involves:
> > > - adding vmstate to xen-all.c so it can migrate the xen vram
address
> >
> > Easy so far.
> >
> >
> > > - making sure the memory core doesn''t do mappings during
restore (can be
> > > done by wrapping restore with
> > >
memory_region_transaction_begin()/memory_region_transaction_commit();
> > > beneficial to normal qemu migrations as well)
> >
> > Besides restore we would also need to wrap machine->init() with
> > memory_region_transaction_begin()/memory_region_transaction_commit(),
> > so that all the mappings are only done at the end of restore, when we
do
> > know the videoram address.
> >
> > This seems unfeasable to me: what about all the addresses set in the
> > meantime using memory_region_get_ram_ptr()?
> > For example s->vram_ptr set by vga_common_init during
machine->init()?
> > If the actual memory is only allocated at the end of restore, vram_ptr
> > would be bogus at least until then and we would still need a way to
> > update it afterwards.
> 
> One way is to only call it on demand when we actually need to draw or
> read the framebuffer.  Currently this will be slow since we''ll
search
> the RAMBlock list, but soon it will be dereference of MemoryRegion.
given that there is only one access to the framebuffer after
vga_common_init and before cirrus_post_load, that is the framebuffer
memset to 0xff, and given that it is actually useless to memset the
framebuffer on restore because it is going to be overwritten anyway
afterwards, I suggest keeping the second hunk of
http://marc.info/?l=qemu-devel&m=132346828427314&w=2 instead

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

> > > - updating the mapped address during restore
> > > 
> > > I think this is cleaner than introducing a new migration stage,
but the
> > > implementation may prove otherwise.
> >
> > see patch above, a good summary of the difficulties of this approach
> >
> >
> > > > > xen_register_framebuffer() is slightly less hacky.
> > > >
> > > > I agree, however xen_register_framebuffer is called after
> > > > memory_region_init_ram, so the allocation would have been
made already.
> > > 
> > > xen_ram_alloc(MemoryRegion *mr)
> > > {
> > >     if (in_restore && mr == framebuffer &&
!framebuffer_addr_known) {
> > >         return;
> > >     }
> > > }
> > > 
> > > xen_framebuffer_address_post_load()
> > > {
> > >     framebuffer_addr_known = true;
> > >     if (framebuffer) {
> > >         framebuffer->xen_address = framebuffer_addr;
> > >     }
> > > }
> > > 
> > > (ugly way of avoiding a dependency, but should work)
> >  
> > The issue is that xen_ram_alloc is called by memory_region_init_ram
> > before xen_register_framebuffer is called (see the order of calls in
> > vga_common_init).
> > So when xen_ram_alloc is called framebuffer is still NULL: mr !>
> framebuffer.
> 
> 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?

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

On 2012-01-05 15:50, Avi Kivity wrote:
>> 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.
To me this still sounds like a cirrus-only xen workaround that
nevertheless spreads widely.

Again, what speaks against migrating the information Xen needs before
creating the machine or a single device? That would only introduce a
generic concept of an (optional) "early", let''s call it
"accelerator-related" vmstate and would allow Xen to deal with all the
specifics behind the curtain.

Jan

On Thu, 5 Jan 2012, Jan Kiszka wrote:
> On 2012-01-05 15:50, Avi Kivity wrote:
> >> 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.
> 
> To me this still sounds like a cirrus-only xen workaround that
> nevertheless spreads widely.
The first two points are still necessary even if we go with the
early_savevm option;
the third point is a good change regardless of the qemu accelerator;
the only cirrus-only workaround is the last point, however it certainly
doesn''t spread widely. It is true that other framebuffer based devices
would need a similar change.

> Again, what speaks against migrating the information Xen needs before
> creating the machine or a single device? That would only introduce a
> generic concept of an (optional) "early", let''s call it
> "accelerator-related" vmstate and would allow Xen to deal with
all the
> specifics behind the curtain.
I am OK with both approaches.
The only practical difference between the two is how we pass the xen
framebuffer address across save/restore: either in the device specific
savevm record or in a xen specific early_savevm record.
What is it going to be?

On 01/05/2012 08:49 PM, Jan Kiszka wrote:
> To me this still sounds like a cirrus-only xen workaround that
> nevertheless spreads widely.
It is.
> Again, what speaks against migrating the information Xen needs before
> creating the machine or a single device? That would only introduce a
> generic concept of an (optional) "early", let''s call it
> "accelerator-related" vmstate and would allow Xen to deal with
all the
> specifics behind the curtain.
>
Adding more concepts, just to work around a bug (and this is really a
bug in the qemu/xen interface) makes it harder to refactor things later on.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

On 2012-01-06 10:19, Avi Kivity wrote:
> On 01/05/2012 08:49 PM, Jan Kiszka wrote:
>> To me this still sounds like a cirrus-only xen workaround that
>> nevertheless spreads widely.
> 
> It is.
> 
>> Again, what speaks against migrating the information Xen needs before
>> creating the machine or a single device? That would only introduce a
>> generic concept of an (optional) "early", let''s call
it
>> "accelerator-related" vmstate and would allow Xen to deal
with all the
>> specifics behind the curtain.
>>
> 
> Adding more concepts, just to work around a bug (and this is really a
> bug in the qemu/xen interface) makes it harder to refactor things later on.
Well, it''s at least only a single concept, one that could even be used
independently of Xen issues, while it appears to me like the other
proposal comes with multiple ones.

Jan

On 01/06/2012 02:22 PM, Jan Kiszka wrote:
> > 
> > Adding more concepts, just to work around a bug (and this is really a
> > bug in the qemu/xen interface) makes it harder to refactor things
later on.
>
> Well, it''s at least only a single concept, one that could even be
used
> independently of Xen issues,
If it has other uses, sure, I''m all for it.
>  while it appears to me like the other
> proposal comes with multiple ones.
>
The other proposal is a hack, but it''s doesn''t introduce new
concepts.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

On 2012-01-06 08:50, Stefano Stabellini wrote:
> On Thu, 5 Jan 2012, Jan Kiszka wrote:
>> On 2012-01-05 15:50, Avi Kivity wrote:
>>>> 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.
>>
>> To me this still sounds like a cirrus-only xen workaround that
>> nevertheless spreads widely.
> 
> The first two points are still necessary even if we go with the
> early_savevm option;
Can''t really asses what the best way for Xen is to associate a memory
region with a particular physical mapping as to be stored in the early
vmstate. Is the memory API lacking some tag here?
> the third point is a good change regardless of the qemu accelerator;
> the only cirrus-only workaround is the last point, however it certainly
> doesn''t spread widely. It is true that other framebuffer based
devices
> would need a similar change.
The third point indicates that there is rather more generic room for
improvements: Why should qemu reset device models before restore at all?
If we don''t run the reset handlers, we don''t suffer from that
useless
memset. Testing for "Are we about to restore?" in a cirrus-only way
looks wrong.
> 
> 
>> Again, what speaks against migrating the information Xen needs before
>> creating the machine or a single device? That would only introduce a
>> generic concept of an (optional) "early", let''s call
it
>> "accelerator-related" vmstate and would allow Xen to deal
with all the
>> specifics behind the curtain.
> 
> I am OK with both approaches.
> The only practical difference between the two is how we pass the xen
> framebuffer address across save/restore: either in the device specific
> savevm record or in a xen specific early_savevm record.
> What is it going to be?
I''m sure you will appreciate a more generic approach than adding this
to
the device state once supporting more than cirrus over Xen.

Jan

On Fri, 6 Jan 2012, Jan Kiszka wrote:
> On 2012-01-06 08:50, Stefano Stabellini wrote:
> > On Thu, 5 Jan 2012, Jan Kiszka wrote:
> >> On 2012-01-05 15:50, Avi Kivity wrote:
> >>>> 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.
> >>
> >> To me this still sounds like a cirrus-only xen workaround that
> >> nevertheless spreads widely.
> > 
> > The first two points are still necessary even if we go with the
> > early_savevm option;
> 
> Can''t really asses what the best way for Xen is to associate a
memory
> region with a particular physical mapping as to be stored in the early
> vmstate. Is the memory API lacking some tag here?
ATM we have our own list of physmaps (physical mappings) maintained in
xen-all.c, but adding a xen specific field to the MemoryRegion struct
should be enough to allow us to remove the physmap list and just use
MemoryRegions for everything.

Regarding save/restore, we could save in an early_savevm record all the
MemoryRegions or just the ones with a special xen mapping.

> > the third point is a good change regardless of the qemu accelerator;
> > the only cirrus-only workaround is the last point, however it
certainly
> > doesn''t spread widely. It is true that other framebuffer
based devices
> > would need a similar change.
> 
> The third point indicates that there is rather more generic room for
> improvements: Why should qemu reset device models before restore at all?
> If we don''t run the reset handlers, we don''t suffer from
that useless
> memset. Testing for "Are we about to restore?" in a cirrus-only
way
> looks wrong.
Fair enough.

> >> Again, what speaks against migrating the information Xen needs
before
> >> creating the machine or a single device? That would only introduce
a
> >> generic concept of an (optional) "early", let''s
call it
> >> "accelerator-related" vmstate and would allow Xen to
deal with all the
> >> specifics behind the curtain.
> > 
> > I am OK with both approaches.
> > The only practical difference between the two is how we pass the xen
> > framebuffer address across save/restore: either in the device specific
> > savevm record or in a xen specific early_savevm record.
> > What is it going to be?
> 
> I''m sure you will appreciate a more generic approach than adding
this to
> the device state once supporting more than cirrus over Xen.
 
Of course I do, in fact that is why I like the early_savevm approach, it
is more flexible.
On the other hand it is more work and realistically 99% of our users just
use Cirrus all the time so that is why I am also OK with the other
solution.

Avi, if you think that early_savevm is a decent solution, we''ll start
working on it. Also if you would like to save and restore all the
MemoryRegions because you can see some possible future uses for it, please
let me know. Otherwise we might try to save some space and just store the
ones we need.

On 6 January 2012 13:30, Jan Kiszka <jan.kiszka@web.de>
wrote:
> The third point indicates that there is rather more generic room for
> improvements: Why should qemu reset device models before restore at all?
Commit 5a8a49d7aa says:
# if we load from a snapshot, the machine can be in any state. That can
# cause troubles if loading an older image which does not carry all state
# information the executing QEMU requires. Hardly any device takes care of
# this scenario.

I think it''s also easier to reason about devices if you know that the
device before a load was in a known clean state rather than being
anything at all.

-- PMM

On 2012-01-06 13:58, Peter Maydell wrote:
> On 6 January 2012 13:30, Jan Kiszka <jan.kiszka@web.de> wrote:
>> The third point indicates that there is rather more generic room for
>> improvements: Why should qemu reset device models before restore at
all?
> 
> Commit 5a8a49d7aa says:
> # if we load from a snapshot, the machine can be in any state. That can
> # cause troubles if loading an older image which does not carry all state
> # information the executing QEMU requires. Hardly any device takes care of
> # this scenario.
> 
> I think it''s also easier to reason about devices if you know that
the
> device before a load was in a known clean state rather than being
> anything at all.
True, that''s why we do this ATM. But issuing the reset globally without
giving devices chance to handle this more precisely is just a suboptimal
workaround, one that we may want to overcome at some point.

Jan

On 01/06/2012 04:40 PM, Stefano Stabellini wrote:
> Avi, if you think that early_savevm is a decent solution, we''ll
start
> working on it. 
I don''t like early_savevm because it complicates life for devices, for
what is a localized problem.  But if everything else is even more
complicated maybe we should do it.
> Also if you would like to save and restore all the
> MemoryRegions because you can see some possible future uses for it, please
> let me know. Otherwise we might try to save some space and just store the
> ones we need.
MemoryRegions don''t hold their own state, they just mirror state that
exists elsewhere.  They''re 100% implementation artefacts, it''s
totally
wrong to save/restore them.

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

On Sun, 8 Jan 2012, Avi Kivity wrote:
> On 01/06/2012 04:40 PM, Stefano Stabellini wrote:
> > Avi, if you think that early_savevm is a decent solution,
we''ll start
> > working on it. 
> 
> I don''t like early_savevm because it complicates life for devices,
for
> what is a localized problem.  But if everything else is even more
> complicated maybe we should do it.
I have been thinking more about this issue but unfortunately I cannot
come up with anything better than early_savevm.

On 2012-01-09 16:25, Stefano Stabellini wrote:
> On Sun, 8 Jan 2012, Avi Kivity wrote:
>> On 01/06/2012 04:40 PM, Stefano Stabellini wrote:
>>> Avi, if you think that early_savevm is a decent solution,
we''ll start
>>> working on it. 
>>
>> I don''t like early_savevm because it complicates life for
devices, for
>> what is a localized problem.  But if everything else is even more
>> complicated maybe we should do it.
> 
> I have been thinking more about this issue but unfortunately I cannot
> come up with anything better than early_savevm.
And I do not yet see the impact of the early_savevm concept on devices.
Its point is to have some vmstate that is unrelated, actually predates
any device creation (during restore).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

On 01/09/2012 05:28 PM, Jan Kiszka wrote:
> On 2012-01-09 16:25, Stefano Stabellini wrote:
> > On Sun, 8 Jan 2012, Avi Kivity wrote:
> >> On 01/06/2012 04:40 PM, Stefano Stabellini wrote:
> >>> Avi, if you think that early_savevm is a decent solution,
we''ll start
> >>> working on it. 
> >>
> >> I don''t like early_savevm because it complicates life for
devices, for
> >> what is a localized problem.  But if everything else is even more
> >> complicated maybe we should do it.
> > 
> > I have been thinking more about this issue but unfortunately I cannot
> > come up with anything better than early_savevm.
>
> And I do not yet see the impact of the early_savevm concept on devices.
> Its point is to have some vmstate that is unrelated, actually predates
> any device creation (during restore).
Okay then, at least we tried.

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

On 12/13/2011 05:55 AM, Stefano Stabellini wrote:
> On Mon, 12 Dec 2011, Stefano Stabellini wrote:
>>> Really, I think this is something inherently incompatible with the
>>> current memory API. If Xen has this unfixable special
"requirement"
>>> (it''s rather a design issue IMHO), adjust the API and
adapt all devices.
>>> Hot-fixing only a single one this way is no good idea long term.
>>
>> Fair enough.
>> What about introducing a type of savevm state that is going to be
>> restored before machine->init?
>> This way we could save and restore our physmap and we could handle
>> memory maps and allocations transparently.
>>
>
> A bit more context to my suggestion:
>
> - we open the save state file and check the magic number and the
> version number before machine->init();
>
> - we add a new set of entries to the save state file that contains
> early_savevm functions: they are called before machine->init();
>
> - after reaching the end of the early_savevm functions we stop parsing
> the save state file and we call machine->init();
>
> - after machine->init() is completed we continue parsing the save state
> file as usual.

I don''t think this is a good idea.  We shouldn''t present an
ad-hoc mechanism to
configure devices in the device state.

I think this is fundamentally a Xen problem.  Where QEMU locates the buffer it 
uses to represent video RAM is entirely its own business.

What are ya''ll doing that necessitates doing something special with
video RAM?

Regards,

Anthony Liguori