Xen devel - Jan 2012 - [PATCH v3 0/6] save/restore on Xen

Hi all,
this is the third version of the Xen save/restore patch series.
We have been discussing this issue for quite a while on #qemu and
qemu-devel:


http://marc.info/?l=qemu-devel&m=132346828427314&w=2
http://marc.info/?l=qemu-devel&m=132377734605464&w=2


A few different approaches were proposed to achieve the goal
of a working save/restore with upstream Qemu on Xen, however after
prototyping some of them I came up with yet another solution, that I
think leads to the best results with the less amount of code
duplications and ugliness.
Far from saying that this patch series is an example of elegance and
simplicity, but it is closer to acceptable anything else I have seen so
far.

What''s new is that Qemu is going to keep track of its own physmap on
xenstore, so that Xen can be fully aware of the changes Qemu makes to
the guest''s memory map at any time.
This is all handled by Xen or Xen support in Qemu internally and can be
used to solve our save/restore framebuffer problem.

From the Qemu common code POV, we still need to avoid saving the
guest''s
ram when running on Xen, and we need to avoid resetting the videoram on
restore (that is a benefit to the generic Qemu case too, because it
saves few cpu cycles).



This is the list of patches with a diffstat:

Anthony PERARD (4):
      vl.c: do not save the RAM state when Xen is enabled
      xen mapcache: check if memory region has moved.
      cirrus_vga: do not reset videoram on resume
      xen: change memory access behavior during migration.

Stefano Stabellini (2):
      Set runstate to INMIGRATE earlier
      xen: record physmap changes to xenstore

 hw/cirrus_vga.c |    9 ++++--
 vl.c            |    8 +++--
 xen-all.c       |   89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 xen-mapcache.c  |   22 ++++++++++++--
 xen-mapcache.h  |    9 ++++-
 5 files changed, 125 insertions(+), 12 deletions(-)


git://xenbits.xen.org/people/sstabellini/qemu-dm.git saverestore-3

Cheers,

Stefano

From: Anthony PERARD <anthony.perard@citrix.com>

In the 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>
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 vl.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index ba55b35..6f0435b 100644
--- a/vl.c
+++ b/vl.c
@@ -3270,8 +3270,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;
-- 
1.7.2.5

From: Anthony PERARD <anthony.perard@citrix.com>

This patch changes 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>
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.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 c86ebf4..507d93d 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -225,6 +225,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,
@@ -964,7 +980,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 9fecc64..d9c995b 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)
 {
 }
 
-- 
1.7.2.5

Set runstate to RUN_STATE_INMIGRATE as soon as we can on resume.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 vl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/vl.c b/vl.c
index 6f0435b..bb0139f 100644
--- a/vl.c
+++ b/vl.c
@@ -2972,6 +2972,7 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_incoming:
                 incoming = optarg;
+                runstate_set(RUN_STATE_INMIGRATE);
                 break;
             case QEMU_OPTION_nodefaults:
                 default_serial = 0;
@@ -3468,7 +3469,6 @@ int main(int argc, char **argv, char **envp)
     }
 
     if (incoming) {
-        runstate_set(RUN_STATE_INMIGRATE);
         int ret = qemu_start_incoming_migration(incoming);
         if (ret < 0) {
             fprintf(stderr, "Migration failed. Exit code %s(%d),
exiting.\n",
-- 
1.7.2.5

From: Anthony PERARD <anthony.perard@citrix.com>

When resuming we shouldn''t set the videoram to 0xff considering that we
are about to read it from the savefile.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/cirrus_vga.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index f7b1d3d..eec2fc0 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:
@@ -2760,9 +2761,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_INMIGRATE)) {
+        /* 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;
-- 
1.7.2.5

Write to xenstore any physmap changes so that the hypervisor can be
aware of them.
Read physmap changes from xenstore on boot.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen-all.c |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/xen-all.c b/xen-all.c
index 507d93d..c830cb1 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -253,6 +253,7 @@ static int xen_add_to_physmap(XenIOState *state,
     XenPhysmap *physmap = NULL;
     target_phys_addr_t pfn, start_gpfn;
     target_phys_addr_t phys_offset = memory_region_get_ram_addr(mr);
+    char path[80], value[17];
 
     if (get_physmapping(state, start_addr, size)) {
         return 0;
@@ -299,6 +300,22 @@ go_physmap:
                                    start_addr >> TARGET_PAGE_BITS,
                                    (start_addr + size) >>
TARGET_PAGE_BITS,
                                    XEN_DOMCTL_MEM_CACHEATTR_WB);
+
+    snprintf(path, sizeof(path),
+           
"/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
+            xen_domid, (uint64_t)phys_offset);
+    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)start_addr);
+    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
+        return -1;
+    }
+    snprintf(path, sizeof(path),
+           
"/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
+            xen_domid, (uint64_t)phys_offset);
+    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)size);
+    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
+        return -1;
+    }
+
     return 0;
 }
 
@@ -926,6 +943,50 @@ int xen_init(void)
     return 0;
 }
 
+static void xen_read_physmap(XenIOState *state)
+{
+    XenPhysmap *physmap = NULL;
+    unsigned int len, num, i;
+    char path[80], *value = NULL;
+    char **entries = NULL;
+
+    snprintf(path, sizeof(path),
+            "/local/domain/0/device-model/%d/physmap", xen_domid);
+    entries = xs_directory(state->xenstore, 0, path, &num);
+    if (entries == NULL)
+        return;
+
+    for (i = 0; i < num; i++) {
+        physmap = g_malloc(sizeof (XenPhysmap));
+        physmap->phys_offset = strtoull(entries[i], NULL, 16);
+        snprintf(path, sizeof(path),
+               
"/local/domain/0/device-model/%d/physmap/%s/start_addr",
+                xen_domid, entries[i]);
+        value = xs_read(state->xenstore, 0, path, &len);
+        if (value == NULL) {
+            free(physmap);
+            continue;
+        }
+        physmap->start_addr = strtoull(value, NULL, 16);
+        free(value);
+
+        snprintf(path, sizeof(path),
+                "/local/domain/0/device-model/%d/physmap/%s/size",
+                xen_domid, entries[i]);
+        value = xs_read(state->xenstore, 0, path, &len);
+        if (value == NULL) {
+            free(physmap);
+            continue;
+        }
+        physmap->size = strtoull(value, NULL, 16);
+        free(value);
+
+        QLIST_INSERT_HEAD(&state->physmap, physmap, list);
+    }
+    free(entries);
+    return;
+}
+
 int xen_hvm_init(void)
 {
     int i, rc;
@@ -998,6 +1059,7 @@ int xen_hvm_init(void)
     xen_be_register("console", &xen_console_ops);
     xen_be_register("vkbd", &xen_kbdmouse_ops);
     xen_be_register("qdisk", &xen_blkdev_ops);
+    xen_read_physmap(state);
 
     return 0;
 }
-- 
1.7.2.5

From: Anthony PERARD <anthony.perard@citrix.com>

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

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

diff --git a/xen-all.c b/xen-all.c
index c830cb1..bac06fd 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -190,6 +190,11 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
MemoryRegion *mr)
     xen_pfn_t *pfn_list;
     int i;
 
+    if (runstate_check(RUN_STATE_INMIGRATE)) {
+        /* RAM already populated in Xen */
+        return;
+    }
+
     if (mr == &ram_memory) {
         return;
     }
@@ -255,6 +260,10 @@ static int xen_add_to_physmap(XenIOState *state,
     target_phys_addr_t phys_offset = memory_region_get_ram_addr(mr);
     char path[80], value[17];
 
+    if (runstate_check(RUN_STATE_INMIGRATE)) {
+        return 0;
+    }
+
     if (get_physmapping(state, start_addr, size)) {
         return 0;
     }
-- 
1.7.2.5

On Thu, 2012-01-19 at 11:56 +0000, Stefano Stabellini
wrote:
> Write to xenstore any physmap changes so that the hypervisor can be
> aware of them.
What is the structure of the xenstore values? Looks like 
	<domid>/physmap/<original-addr>/start_addr <new-addr>
? Who defines the meaning of original-addr, in particular what happens
if it the original-addr for a device changes in a N->N+1 migration? What
happens if things overlap or if a subsequent call only updates part of a
previously moved mapping?
> Read physmap changes from xenstore on boot.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen-all.c |   62
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 62 insertions(+), 0 deletions(-)
> 
> diff --git a/xen-all.c b/xen-all.c
> index 507d93d..c830cb1 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -253,6 +253,7 @@ static int xen_add_to_physmap(XenIOState *state,
>      XenPhysmap *physmap = NULL;
>      target_phys_addr_t pfn, start_gpfn;
>      target_phys_addr_t phys_offset = memory_region_get_ram_addr(mr);
> +    char path[80], value[17];
>  
>      if (get_physmapping(state, start_addr, size)) {
>          return 0;
> @@ -299,6 +300,22 @@ go_physmap:
>                                     start_addr >> TARGET_PAGE_BITS,
>                                     (start_addr + size) >>
TARGET_PAGE_BITS,
>                                     XEN_DOMCTL_MEM_CACHEATTR_WB);
> +
> +    snprintf(path, sizeof(path),
> +           
"/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
> +            xen_domid, (uint64_t)phys_offset);
> +    snprintf(value, sizeof(value), "%"PRIx64,
(uint64_t)start_addr);
> +    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
> +        return -1;
> +    }
> +    snprintf(path, sizeof(path),
> +           
"/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
> +            xen_domid, (uint64_t)phys_offset);
> +    snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)size);
> +    if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
> +        return -1;
> +    }
> +
>      return 0;
>  }
>  
> @@ -926,6 +943,50 @@ int xen_init(void)
>      return 0;
>  }
>  
> +static void xen_read_physmap(XenIOState *state)
> +{
> +    XenPhysmap *physmap = NULL;
> +    unsigned int len, num, i;
> +    char path[80], *value = NULL;
> +    char **entries = NULL;
> +
> +    snprintf(path, sizeof(path),
> +            "/local/domain/0/device-model/%d/physmap",
xen_domid);
> +    entries = xs_directory(state->xenstore, 0, path, &num);
> +    if (entries == NULL)
> +        return;
> +
> +    for (i = 0; i < num; i++) {
> +        physmap = g_malloc(sizeof (XenPhysmap));
> +        physmap->phys_offset = strtoull(entries[i], NULL, 16);
> +        snprintf(path, sizeof(path),
> +               
"/local/domain/0/device-model/%d/physmap/%s/start_addr",
> +                xen_domid, entries[i]);
> +        value = xs_read(state->xenstore, 0, path, &len);
> +        if (value == NULL) {
> +            free(physmap);
> +            continue;
> +        }
> +        physmap->start_addr = strtoull(value, NULL, 16);
> +        free(value);
> +
> +        snprintf(path, sizeof(path),
> +               
"/local/domain/0/device-model/%d/physmap/%s/size",
> +                xen_domid, entries[i]);
> +        value = xs_read(state->xenstore, 0, path, &len);
> +        if (value == NULL) {
> +            free(physmap);
> +            continue;
> +        }
> +        physmap->size = strtoull(value, NULL, 16);
> +        free(value);
> +
> +        QLIST_INSERT_HEAD(&state->physmap, physmap, list);
> +    }
> +    free(entries);
> +    return;
> +}
> +
>  int xen_hvm_init(void)
>  {
>      int i, rc;
> @@ -998,6 +1059,7 @@ int xen_hvm_init(void)
>      xen_be_register("console", &xen_console_ops);
>      xen_be_register("vkbd", &xen_kbdmouse_ops);
>      xen_be_register("qdisk", &xen_blkdev_ops);
> +    xen_read_physmap(state);
>  
>      return 0;
>  }

On Thu, 19 Jan 2012, Ian Campbell wrote:
> On Thu, 2012-01-19 at 11:56 +0000, Stefano Stabellini wrote:
> > Write to xenstore any physmap changes so that the hypervisor can be
> > aware of them.
> 
> What is the structure of the xenstore values? Looks like 
> 	<domid>/physmap/<original-addr>/start_addr <new-addr>
> ?
Yep, that is the structure

> Who defines the meaning of original-addr, in particular what happens
> if it the original-addr for a device changes in a N->N+1 migration? What
> happens if things overlap or if a subsequent call only updates part of a
> previously moved mapping?
In practice it cannot happen because on restore Qemu is going to emulate
the same set of devices it was emulating before.
The original address is not going to change if the devices and the
amount of guest memory stay the same.

We could add some additional info to better identify the physmap entry,
probably the best we could use is the memory_region name.

On Thu, 2012-01-19 at 13:08 +0000, Stefano Stabellini
wrote:
> On Thu, 19 Jan 2012, Ian Campbell wrote:
> > On Thu, 2012-01-19 at 11:56 +0000, Stefano Stabellini wrote:
> > > Write to xenstore any physmap changes so that the hypervisor can
be
> > > aware of them.
> > 
> > What is the structure of the xenstore values? Looks like 
> > 	<domid>/physmap/<original-addr>/start_addr
<new-addr>
> > ?
> 
> Yep, that is the structure
> 
> 
> > Who defines the meaning of original-addr, in particular what happens
> > if it the original-addr for a device changes in a N->N+1 migration?
What
> > happens if things overlap or if a subsequent call only updates part of
a
> > previously moved mapping?
> 
> In practice it cannot happen because on restore Qemu is going to emulate
> the same set of devices it was emulating before.
> The original address is not going to change if the devices and the
> amount of guest memory stay the same.
If you are migrating to a newer qemu then the <original-addr> could in
principal change, I think.
> We could add some additional info to better identify the physmap entry,
> probably the best we could use is the memory_region name.
And/Or the PCI BDF + BAR?

Ian.

On Thu, 19 Jan 2012, Ian Campbell wrote:
> On Thu, 2012-01-19 at 13:08 +0000, Stefano Stabellini wrote:
> > On Thu, 19 Jan 2012, Ian Campbell wrote:
> > > On Thu, 2012-01-19 at 11:56 +0000, Stefano Stabellini wrote:
> > > > Write to xenstore any physmap changes so that the hypervisor
can be
> > > > aware of them.
> > > 
> > > What is the structure of the xenstore values? Looks like 
> > > 	<domid>/physmap/<original-addr>/start_addr
<new-addr>
> > > ?
> > 
> > Yep, that is the structure
> > 
> > 
> > > Who defines the meaning of original-addr, in particular what
happens
> > > if it the original-addr for a device changes in a N->N+1
migration? What
> > > happens if things overlap or if a subsequent call only updates
part of a
> > > previously moved mapping?
> > 
> > In practice it cannot happen because on restore Qemu is going to
emulate
> > the same set of devices it was emulating before.
> > The original address is not going to change if the devices and the
> > amount of guest memory stay the same.
> 
> If you are migrating to a newer qemu then the <original-addr> could
in
> principal change, I think.
Not unless the implementation of qemu_ram_alloc_from_ptr or
find_ram_offset change, but these are core qemu functions.
Or the device starts allocating more memory of course, but it wouldn''t
be the same device anymore.
In any case, if we also match on the MemoryRegion name we cannot go
wrong.

> > We could add some additional info to better identify the physmap
entry,
> > probably the best we could use is the memory_region name.
> 
> And/Or the PCI BDF + BAR?
I don''t think we can get the PCI BDF from the Xen MemoryListener, but
we
can get the MemoryRegion. The most identifying info in it I think is the
name, for example for vga would be "vga.vram".

On 01/19/2012 04:16 PM, Stefano Stabellini wrote:
> > 
> > If you are migrating to a newer qemu then the <original-addr>
could in
> > principal change, I think.
>
> Not unless the implementation of qemu_ram_alloc_from_ptr or
> find_ram_offset change, but these are core qemu functions.
Both of these functions will be removed.  There will no longer be a
qemu-internal address space for physical memory; instead memory will be
addressed using a (MemoryRegion, offset) pair.

We can/will hook memory_region_init_ram() to call xen_ram_alloc() which
can then generate those old addresses, but those (like qemu_ram_alloc())
are dependent on allocation order and you shouldn''t depend on them
returning stable values.
> Or the device starts allocating more memory of course, but it
wouldn''t
> be the same device anymore.
> In any case, if we also match on the MemoryRegion name we cannot go
> wrong.
Match on just the MemoryRegion (and match on the object itself, not the
name; see xen_register_framebuffer()).
> > > We could add some additional info to better identify the physmap
entry,
> > > probably the best we could use is the memory_region name.
> > 
> > And/Or the PCI BDF + BAR?
>
> I don''t think we can get the PCI BDF from the Xen MemoryListener,
but we
> can get the MemoryRegion. The most identifying info in it I think is the
> name, for example for vga would be "vga.vram".

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

On Wed, 25 Jan 2012, Avi Kivity wrote:
> On 01/19/2012 04:16 PM, Stefano Stabellini wrote:
> > > 
> > > If you are migrating to a newer qemu then the
<original-addr> could in
> > > principal change, I think.
> >
> > Not unless the implementation of qemu_ram_alloc_from_ptr or
> > find_ram_offset change, but these are core qemu functions.
> 
> Both of these functions will be removed.  There will no longer be a
> qemu-internal address space for physical memory; instead memory will be
> addressed using a (MemoryRegion, offset) pair.
> 
> We can/will hook memory_region_init_ram() to call xen_ram_alloc() which
> can then generate those old addresses, but those (like qemu_ram_alloc())
> are dependent on allocation order and you shouldn''t depend on them
> returning stable values.
> 
> > Or the device starts allocating more memory of course, but it
wouldn''t
> > be the same device anymore.
> > In any case, if we also match on the MemoryRegion name we cannot go
> > wrong.
> 
> Match on just the MemoryRegion (and match on the object itself, not the
> name; see xen_register_framebuffer()).
I agree that would be ideal, but how can that work across save/restore?
Unless we introduce some other kind of identifier for MemoryRegion, the
best we have is the name right now, correct?

On 01/25/2012 01:55 PM, Stefano Stabellini wrote:
> > 
> > Match on just the MemoryRegion (and match on the object itself, not
the
> > name; see xen_register_framebuffer()).
>
> I agree that would be ideal, but how can that work across save/restore?
> Unless we introduce some other kind of identifier for MemoryRegion, the
> best we have is the name right now, correct?
Right, you can''t put the pointer in save/restore, the name is the only
option (and that''s what qemu uses in its native save format).  I
thought
you were talking about internal matching, like in a previous version of
the patch/discussion.

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