[Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Stefano Stabellini]

Hi all,
this is the fourth 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).


Changes in v4:

- keep a record of the MemoryRegion's name on xenstore;

- print a message when avoiding a memory allocation on restore.


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       |  112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 xen-mapcache.c  |   22 +++++++++-
 xen-mapcache.h  |    9 +++-
 5 files changed, 147 insertions(+), 13 deletions(-)


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

Cheers,

Stefano

[Qemu-devel] [PATCH v4 1/6] vl.c: do not save the RAM state when Xen is enabled

[Stefano Stabellini]

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

[Qemu-devel] [PATCH v4 2/6] xen mapcache: check if memory region has moved.

[Stefano Stabellini]

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

[Qemu-devel] [PATCH v4 3/6] Set runstate to INMIGRATE earlier

[Stefano Stabellini]

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

[Qemu-devel] [PATCH v4 4/6] cirrus_vga: do not reset videoram on resume

[Stefano Stabellini]

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

[Qemu-devel] [PATCH v4 5/6] xen: record physmap changes to xenstore

[Stefano Stabellini]

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 |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 77 insertions(+), 1 deletions(-)

diff --git a/xen-all.c b/xen-all.c
index 507d93d..bb66c82 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -63,7 +63,7 @@ static inline ioreq_t *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
 typedef struct XenPhysmap {
     target_phys_addr_t start_addr;
     ram_addr_t size;
-    MemoryRegion *mr;
+    char *name;
     target_phys_addr_t phys_offset;
 
     QLIST_ENTRY(XenPhysmap) list;
@@ -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;
@@ -291,6 +292,7 @@ go_physmap:
 
     physmap->start_addr = start_addr;
     physmap->size = size;
+    physmap->name = (char *)mr->name;
     physmap->phys_offset = phys_offset;
 
     QLIST_INSERT_HEAD(&state->physmap, physmap, list);
@@ -299,6 +301,30 @@ 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;
+    }
+    if (mr->name) {
+        snprintf(path, sizeof(path),
+                "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
+                xen_domid, (uint64_t)phys_offset);
+        if (!xs_write(state->xenstore, 0, path, mr->name, strlen(mr->name))) {
+            return -1;
+        }
+    }
+
     return 0;
 }
 
@@ -926,6 +952,55 @@ 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);
+
+        snprintf(path, sizeof(path),
+                "/local/domain/0/device-model/%d/physmap/%s/name",
+                xen_domid, entries[i]);
+        physmap->name = xs_read(state->xenstore, 0, path, &len);
+
+        QLIST_INSERT_HEAD(&state->physmap, physmap, list);
+    }
+    free(entries);
+    return;
+}
+
 int xen_hvm_init(void)
 {
     int i, rc;
@@ -998,6 +1073,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

[Qemu-devel] [PATCH v4 6/6] xen: change memory access behavior during migration.

[Stefano Stabellini]

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 |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/xen-all.c b/xen-all.c
index bb66c82..9a80c30 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -190,6 +190,14 @@ 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 */
+        fprintf(stderr, "%s: do not alloc "RAM_ADDR_FMT
+                " bytes of ram at "RAM_ADDR_FMT" when runstate is INMIGRATE\n",
+                __func__, size, ram_addr); 
+        return;
+    }
+
     if (mr == &ram_memory) {
         return;
     }
@@ -255,6 +263,14 @@ 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)) {
+        fprintf(stderr, "%s: do not add "RAM_ADDR_FMT
+                " bytes of ram to physmap at "RAM_ADDR_FMT
+                " when runstate is INMIGRATE\n",
+                __func__, size, start_addr); 
+        return 0;
+    }
+
     if (get_physmapping(state, start_addr, size)) {
         return 0;
     }
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Jan Kiszka]

On 2012-01-20 18:20, Stefano Stabellini wrote:
> Hi all,
> this is the fourth 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).

For my understanding: Refraining from the memset is required as the
already restored vram would then be overwritten? Or what is the ordering
of init, RAM restore, and initial device reset now?

Jan

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

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Stefano Stabellini]

On Fri, 20 Jan 2012, Jan Kiszka wrote:
> On 2012-01-20 18:20, Stefano Stabellini wrote:
> > Hi all,
> > this is the fourth 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).
> 
> For my understanding: Refraining from the memset is required as the
> already restored vram would then be overwritten?

Yep

> Or what is the ordering
> of init, RAM restore, and initial device reset now?

RAM restore (done by Xen)

physmap rebuild (done by xen_hvm_init in qemu)
pc_init()
qemu_system_reset()
load_vmstate()

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Jan Kiszka]

On 2012-01-23 11:47, Stefano Stabellini wrote:
> On Fri, 20 Jan 2012, Jan Kiszka wrote:
>> On 2012-01-20 18:20, Stefano Stabellini wrote:
>>> Hi all,
>>> this is the fourth 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).
>>
>> For my understanding: Refraining from the memset is required as the
>> already restored vram would then be overwritten?
> 
> Yep
> 
>> Or what is the ordering
>> of init, RAM restore, and initial device reset now?
> 
> RAM restore (done by Xen)
> 
> physmap rebuild (done by xen_hvm_init in qemu)
> pc_init()
> qemu_system_reset()
> load_vmstate()

Hmm, are you sure that this is the only case where a device init or
reset handler writes to already restored guest memory? Preloading the
RAM this way is a non-standard scenario for QEMU, thus conceptually
fragile. Does restoring happen before QEMU is even started, or can this
point be controlled from QEMU?

Jan

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

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Stefano Stabellini]

On Mon, 23 Jan 2012, Jan Kiszka wrote:
> >> Or what is the ordering
> >> of init, RAM restore, and initial device reset now?
> > 
> > RAM restore (done by Xen)
> > 
> > physmap rebuild (done by xen_hvm_init in qemu)
> > pc_init()
> > qemu_system_reset()
> > load_vmstate()
> 
> Hmm, are you sure that this is the only case where a device init or
> reset handler writes to already restored guest memory? Preloading the
> RAM this way is a non-standard scenario for QEMU, thus conceptually
> fragile. Does restoring happen before QEMU is even started, or can this
> point be controlled from QEMU?

Consider that this only happens with non-MMIO device memory, in practice
only videoram.
Vmware VGA does not memset the videoram in the reset handler, while QXL
already has the following:

    /* pre loadvm reset must not touch QXLRam.  This lives in
     * device memory, is migrated together with RAM and thus
     * already loaded at this point */
    if (!loadvm) {
        qxl_reset_state(d);
    }

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Jan Kiszka]

On 2012-01-23 12:59, Stefano Stabellini wrote:
> On Mon, 23 Jan 2012, Jan Kiszka wrote:
>>>> Or what is the ordering
>>>> of init, RAM restore, and initial device reset now?
>>>
>>> RAM restore (done by Xen)
>>>
>>> physmap rebuild (done by xen_hvm_init in qemu)
>>> pc_init()
>>> qemu_system_reset()
>>> load_vmstate()
>>
>> Hmm, are you sure that this is the only case where a device init or
>> reset handler writes to already restored guest memory? Preloading the
>> RAM this way is a non-standard scenario for QEMU, thus conceptually
>> fragile. Does restoring happen before QEMU is even started, or can this
>> point be controlled from QEMU?
> 
> Consider that this only happens with non-MMIO device memory, in practice
> only videoram.
> Vmware VGA does not memset the videoram in the reset handler, while QXL
> already has the following:
> 
>     /* pre loadvm reset must not touch QXLRam.  This lives in
>      * device memory, is migrated together with RAM and thus
>      * already loaded at this point */
>     if (!loadvm) {
>         qxl_reset_state(d);
>     }

Yes, but QEMU restores the RAM _after_ device reset, not before it.
That's the problem with the Xen way - it is against the current QEMU
standard.

Jan

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

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Stefano Stabellini]

On Mon, 23 Jan 2012, Jan Kiszka wrote:
> On 2012-01-23 12:59, Stefano Stabellini wrote:
> > On Mon, 23 Jan 2012, Jan Kiszka wrote:
> >>>> Or what is the ordering
> >>>> of init, RAM restore, and initial device reset now?
> >>>
> >>> RAM restore (done by Xen)
> >>>
> >>> physmap rebuild (done by xen_hvm_init in qemu)
> >>> pc_init()
> >>> qemu_system_reset()
> >>> load_vmstate()
> >>
> >> Hmm, are you sure that this is the only case where a device init or
> >> reset handler writes to already restored guest memory? Preloading the
> >> RAM this way is a non-standard scenario for QEMU, thus conceptually
> >> fragile. Does restoring happen before QEMU is even started, or can this
> >> point be controlled from QEMU?
> > 
> > Consider that this only happens with non-MMIO device memory, in practice
> > only videoram.
> > Vmware VGA does not memset the videoram in the reset handler, while QXL
> > already has the following:
> > 
> >     /* pre loadvm reset must not touch QXLRam.  This lives in
> >      * device memory, is migrated together with RAM and thus
> >      * already loaded at this point */
> >     if (!loadvm) {
> >         qxl_reset_state(d);
> >     }
> 
> Yes, but QEMU restores the RAM _after_ device reset, not before it.
> That's the problem with the Xen way - it is against the current QEMU
> standard.

QEMU doesn't save/restore the RAM (and the videoram) at all on Xen.

To reply to your previous question more clearly: at restore time Qemu on
Xen would run in a non-standard scenario; the restore of the RAM happens
before QEMU is even started.

That is unfortunate but it would be very hard to change (I can give you
more details if you are interested in the reasons why it would be so
difficult).

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Jan Kiszka]

On 2012-01-23 15:46, Stefano Stabellini wrote:
> On Mon, 23 Jan 2012, Jan Kiszka wrote:
>> On 2012-01-23 12:59, Stefano Stabellini wrote:
>>> On Mon, 23 Jan 2012, Jan Kiszka wrote:
>>>>>> Or what is the ordering
>>>>>> of init, RAM restore, and initial device reset now?
>>>>>
>>>>> RAM restore (done by Xen)
>>>>>
>>>>> physmap rebuild (done by xen_hvm_init in qemu)
>>>>> pc_init()
>>>>> qemu_system_reset()
>>>>> load_vmstate()
>>>>
>>>> Hmm, are you sure that this is the only case where a device init or
>>>> reset handler writes to already restored guest memory? Preloading the
>>>> RAM this way is a non-standard scenario for QEMU, thus conceptually
>>>> fragile. Does restoring happen before QEMU is even started, or can this
>>>> point be controlled from QEMU?
>>>
>>> Consider that this only happens with non-MMIO device memory, in practice
>>> only videoram.
>>> Vmware VGA does not memset the videoram in the reset handler, while QXL
>>> already has the following:
>>>
>>>     /* pre loadvm reset must not touch QXLRam.  This lives in
>>>      * device memory, is migrated together with RAM and thus
>>>      * already loaded at this point */
>>>     if (!loadvm) {
>>>         qxl_reset_state(d);
>>>     }
>>
>> Yes, but QEMU restores the RAM _after_ device reset, not before it.
>> That's the problem with the Xen way - it is against the current QEMU
>> standard.
> 
> QEMU doesn't save/restore the RAM (and the videoram) at all on Xen.

But it does otherwise, and that's the scenario the code you cited was
written for. It won't work as is under Xen.

> 
> To reply to your previous question more clearly: at restore time Qemu on
> Xen would run in a non-standard scenario; the restore of the RAM happens
> before QEMU is even started.
> 
> That is unfortunate but it would be very hard to change (I can give you
> more details if you are interested in the reasons why it would be so
> difficult).

If you can't change this, you need to properly introduce this new
scenario - pre-initialized RAM - to the QEMU device model. Or you will
see breakage outside cirrus sooner or later as well. So it might be good
to explain the reason why it can't be changed under Xen when motivating
this concept extension to QEMU.

Jan

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

Re: [Qemu-devel] [PATCH v4 1/6] vl.c: do not save the RAM state when Xen is enabled

[Anthony Liguori]

On 01/20/2012 11:21 AM, Stefano Stabellini wrote:
> 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);
> +    }


Why not introduce new Xen specific commands like I suggested on IRC?

This sort of change is extremely non-intuitive.  We should do as much as we can 
to avoid magic #ifdefs like this.

Regards,

Anthony Liguori

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

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Anthony Liguori]

On 01/23/2012 04:47 AM, Stefano Stabellini wrote:
> On Fri, 20 Jan 2012, Jan Kiszka wrote:
>> On 2012-01-20 18:20, Stefano Stabellini wrote:
>>> Hi all,
>>> this is the fourth 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).
>>
>> For my understanding: Refraining from the memset is required as the
>> already restored vram would then be overwritten?
>
> Yep
>
>> Or what is the ordering
>> of init, RAM restore, and initial device reset now?
>
> RAM restore (done by Xen)
>
> physmap rebuild (done by xen_hvm_init in qemu)
> pc_init()
> qemu_system_reset()
> load_vmstate()

That's your problem.  You don't want to do load_vmstate().  You just want to 
load the device model, not RAM.

You should have a separate load_device_state() function and mark anything that 
is RAM as RAM when doing savevm registration.  Better yet, mark devices as 
devices since that's what you really care about.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Stefano Stabellini]

On Mon, 23 Jan 2012, Jan Kiszka wrote:
> On 2012-01-23 15:46, Stefano Stabellini wrote:
> > On Mon, 23 Jan 2012, Jan Kiszka wrote:
> >> On 2012-01-23 12:59, Stefano Stabellini wrote:
> >>> On Mon, 23 Jan 2012, Jan Kiszka wrote:
> >>>>>> Or what is the ordering
> >>>>>> of init, RAM restore, and initial device reset now?
> >>>>>
> >>>>> RAM restore (done by Xen)
> >>>>>
> >>>>> physmap rebuild (done by xen_hvm_init in qemu)
> >>>>> pc_init()
> >>>>> qemu_system_reset()
> >>>>> load_vmstate()
> >>>>
> >>>> Hmm, are you sure that this is the only case where a device init or
> >>>> reset handler writes to already restored guest memory? Preloading the
> >>>> RAM this way is a non-standard scenario for QEMU, thus conceptually
> >>>> fragile. Does restoring happen before QEMU is even started, or can this
> >>>> point be controlled from QEMU?
> >>>
> >>> Consider that this only happens with non-MMIO device memory, in practice
> >>> only videoram.
> >>> Vmware VGA does not memset the videoram in the reset handler, while QXL
> >>> already has the following:
> >>>
> >>>     /* pre loadvm reset must not touch QXLRam.  This lives in
> >>>      * device memory, is migrated together with RAM and thus
> >>>      * already loaded at this point */ if (!loadvm) {
> >>>      qxl_reset_state(d); }
> >>
> >> Yes, but QEMU restores the RAM _after_ device reset, not before it.
> >> That's the problem with the Xen way - it is against the current
> >> QEMU standard.
> > 
> > QEMU doesn't save/restore the RAM (and the videoram) at all on Xen.
> 
> But it does otherwise, and that's the scenario the code you cited was
> written for. It won't work as is under Xen.

Ah, I see your point now.
In that regard, is the comment above even correct?
I am referring to "migrated together with RAM and thus already loaded at
this point"?


> > To reply to your previous question more clearly: at restore time Qemu on
> > Xen would run in a non-standard scenario; the restore of the RAM happens
> > before QEMU is even started.
> > 
> > That is unfortunate but it would be very hard to change (I can give you
> > more details if you are interested in the reasons why it would be so
> > difficult).
> 
> If you can't change this, you need to properly introduce this new
> scenario - pre-initialized RAM - to the QEMU device model. Or you will
> see breakage outside cirrus sooner or later as well. So it might be good
> to explain the reason why it can't be changed under Xen when motivating
> this concept extension to QEMU.
 
OK.
Are you thinking about introducing this concept as a new runstate?
This special runstate could be set at restore time only on Xen.


BTW the main reasons for having Xen saving the RAM are:

- the need to support PV guests, that often run without Qemu;
- the current save format, that is built around the fact that Xen saves the memory;
- the fact that Qemu might be running in a very limited stub-domain.

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Anthony Liguori]

On 01/23/2012 10:16 AM, Stefano Stabellini wrote:
> On Mon, 23 Jan 2012, Jan Kiszka wrote:
>> On 2012-01-23 15:46, Stefano Stabellini wrote:
>>> On Mon, 23 Jan 2012, Jan Kiszka wrote:
>>>> On 2012-01-23 12:59, Stefano Stabellini wrote:
>>>>> On Mon, 23 Jan 2012, Jan Kiszka wrote:
>>>>>>>> Or what is the ordering
>>>>>>>> of init, RAM restore, and initial device reset now?
>>>>>>>
>>>>>>> RAM restore (done by Xen)
>>>>>>>
>>>>>>> physmap rebuild (done by xen_hvm_init in qemu)
>>>>>>> pc_init()
>>>>>>> qemu_system_reset()
>>>>>>> load_vmstate()
>>>>>>
>>>>>> Hmm, are you sure that this is the only case where a device init or
>>>>>> reset handler writes to already restored guest memory? Preloading the
>>>>>> RAM this way is a non-standard scenario for QEMU, thus conceptually
>>>>>> fragile. Does restoring happen before QEMU is even started, or can this
>>>>>> point be controlled from QEMU?
>>>>>
>>>>> Consider that this only happens with non-MMIO device memory, in practice
>>>>> only videoram.
>>>>> Vmware VGA does not memset the videoram in the reset handler, while QXL
>>>>> already has the following:
>>>>>
>>>>>      /* pre loadvm reset must not touch QXLRam.  This lives in
>>>>>       * device memory, is migrated together with RAM and thus
>>>>>       * already loaded at this point */ if (!loadvm) {
>>>>>       qxl_reset_state(d); }
>>>>
>>>> Yes, but QEMU restores the RAM _after_ device reset, not before it.
>>>> That's the problem with the Xen way - it is against the current
>>>> QEMU standard.
>>>
>>> QEMU doesn't save/restore the RAM (and the videoram) at all on Xen.
>>
>> But it does otherwise, and that's the scenario the code you cited was
>> written for. It won't work as is under Xen.
>
> Ah, I see your point now.
> In that regard, is the comment above even correct?
> I am referring to "migrated together with RAM and thus already loaded at
> this point"?
>
>
>>> To reply to your previous question more clearly: at restore time Qemu on
>>> Xen would run in a non-standard scenario; the restore of the RAM happens
>>> before QEMU is even started.
>>>
>>> That is unfortunate but it would be very hard to change (I can give you
>>> more details if you are interested in the reasons why it would be so
>>> difficult).
>>
>> If you can't change this, you need to properly introduce this new
>> scenario - pre-initialized RAM - to the QEMU device model. Or you will
>> see breakage outside cirrus sooner or later as well. So it might be good
>> to explain the reason why it can't be changed under Xen when motivating
>> this concept extension to QEMU.
>
> OK.
> Are you thinking about introducing this concept as a new runstate?
> This special runstate could be set at restore time only on Xen.

A runstate is not the right approach.  Don't abuse existing commands/protocols 
to make them have a different function on Xen.

Just introduce a new command that has the behavior you want.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Stefano Stabellini]

On Mon, 23 Jan 2012, Anthony Liguori wrote:
> On 01/23/2012 04:47 AM, Stefano Stabellini wrote:
> > On Fri, 20 Jan 2012, Jan Kiszka wrote:
> >> On 2012-01-20 18:20, Stefano Stabellini wrote:
> >>> Hi all,
> >>> this is the fourth 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).
> >>
> >> For my understanding: Refraining from the memset is required as the
> >> already restored vram would then be overwritten?
> >
> > Yep
> >
> >> Or what is the ordering
> >> of init, RAM restore, and initial device reset now?
> >
> > RAM restore (done by Xen)
> >
> > physmap rebuild (done by xen_hvm_init in qemu)
> > pc_init()
> > qemu_system_reset()
> > load_vmstate()
> 
> That's your problem.  You don't want to do load_vmstate().  You just want to 
> load the device model, not RAM.

True


> Why not introduce new Xen specific commands like I suggested on IRC?

Introducing a Xen specific command is not an issue, but I didn't want to
duplicate all the functionalities currently in savevm.c.


> You should have a separate load_device_state() function and mark anything that 
> is RAM as RAM when doing savevm registration.  Better yet, mark devices as 
> devices since that's what you really care about.

I dropped this approach because I thought it causes too much code
duplication.
However, following your suggestion, if I add a generic "device" flag in
SaveStateEntry and implement a generic qemu_save_device_state in
savevm.c, I believe that the duplication of code would be small.
And patch #1 could go away.


However the issue of patch #4, "do not reset videoram on resume", still
remains: no matter what parameter I pass to Qemu, if qemu_system_reset
is called on resume the videoram is going to be overwritten by 0xff.
In this regard, don't you think it would be advantageous to Qemu in
general not to reset the videram in resume? It can be pretty large, so
it is a significant waste of a memset.

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Anthony Liguori]

On 01/23/2012 10:46 AM, Stefano Stabellini wrote:
> On Mon, 23 Jan 2012, Anthony Liguori wrote:
>> On 01/23/2012 04:47 AM, Stefano Stabellini wrote:
>>> On Fri, 20 Jan 2012, Jan Kiszka wrote:
>>>> On 2012-01-20 18:20, Stefano Stabellini wrote:
>>>>> Hi all,
>>>>> this is the fourth 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).
>>>>
>>>> For my understanding: Refraining from the memset is required as the
>>>> already restored vram would then be overwritten?
>>>
>>> Yep
>>>
>>>> Or what is the ordering
>>>> of init, RAM restore, and initial device reset now?
>>>
>>> RAM restore (done by Xen)
>>>
>>> physmap rebuild (done by xen_hvm_init in qemu)
>>> pc_init()
>>> qemu_system_reset()
>>> load_vmstate()
>>
>> That's your problem.  You don't want to do load_vmstate().  You just want to
>> load the device model, not RAM.
>
> True
>
>
>> Why not introduce new Xen specific commands like I suggested on IRC?
>
> Introducing a Xen specific command is not an issue, but I didn't want to
> duplicate all the functionalities currently in savevm.c.

The code is fairly reusable since live migration and savevm use the same 
internal bits.  I think you would just need another version of 
qemu_loadvm_state().  That function is only a hundred lines or so so you 
shouldn't be duplicating much at all.

>> You should have a separate load_device_state() function and mark anything that
>> is RAM as RAM when doing savevm registration.  Better yet, mark devices as
>> devices since that's what you really care about.
>
> I dropped this approach because I thought it causes too much code
> duplication.

Then you're doing it wrong :-)

But even if there is, just refactor out the common code.

> However, following your suggestion, if I add a generic "device" flag in
> SaveStateEntry and implement a generic qemu_save_device_state in
> savevm.c, I believe that the duplication of code would be small.
> And patch #1 could go away.

Yup.

>
>
> However the issue of patch #4, "do not reset videoram on resume", still
> remains: no matter what parameter I pass to Qemu, if qemu_system_reset
> is called on resume the videoram is going to be overwritten by 0xff.

The memset(0xff) looks dubious to me.  My guess is that this could be moved to 
the vgabios-cirrus which would solve your problem.

> In this regard, don't you think it would be advantageous to Qemu in
> general not to reset the videram in resume? It can be pretty large, so
> it is a significant waste of a memset.

It claims to fix a real bug.  Moving the memset to vgabios would do what you 
want to do in a more robust way I think.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Stefano Stabellini]

On Mon, 23 Jan 2012, Anthony Liguori wrote:
> On 01/23/2012 10:46 AM, Stefano Stabellini wrote:
> > On Mon, 23 Jan 2012, Anthony Liguori wrote:
> >> On 01/23/2012 04:47 AM, Stefano Stabellini wrote:
> >>> On Fri, 20 Jan 2012, Jan Kiszka wrote:
> >>>> On 2012-01-20 18:20, Stefano Stabellini wrote:
> >>>>> Hi all,
> >>>>> this is the fourth 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).
> >>>>
> >>>> For my understanding: Refraining from the memset is required as the
> >>>> already restored vram would then be overwritten?
> >>>
> >>> Yep
> >>>
> >>>> Or what is the ordering
> >>>> of init, RAM restore, and initial device reset now?
> >>>
> >>> RAM restore (done by Xen)
> >>>
> >>> physmap rebuild (done by xen_hvm_init in qemu)
> >>> pc_init()
> >>> qemu_system_reset()
> >>> load_vmstate()
> >>
> >> That's your problem.  You don't want to do load_vmstate().  You just want to
> >> load the device model, not RAM.
> >
> > True
> >
> >
> >> Why not introduce new Xen specific commands like I suggested on IRC?
> >
> > Introducing a Xen specific command is not an issue, but I didn't want to
> > duplicate all the functionalities currently in savevm.c.
> 
> The code is fairly reusable since live migration and savevm use the same 
> internal bits.  I think you would just need another version of 
> qemu_loadvm_state().  That function is only a hundred lines or so so you 
> shouldn't be duplicating much at all.
> 
> >> You should have a separate load_device_state() function and mark anything that
> >> is RAM as RAM when doing savevm registration.  Better yet, mark devices as
> >> devices since that's what you really care about.
> >
> > I dropped this approach because I thought it causes too much code
> > duplication.
> 
> Then you're doing it wrong :-)
> 
> But even if there is, just refactor out the common code.
> 
> > However, following your suggestion, if I add a generic "device" flag in
> > SaveStateEntry and implement a generic qemu_save_device_state in
> > savevm.c, I believe that the duplication of code would be small.
> > And patch #1 could go away.
> 
> Yup.
> 
> >
> >
> > However the issue of patch #4, "do not reset videoram on resume", still
> > remains: no matter what parameter I pass to Qemu, if qemu_system_reset
> > is called on resume the videoram is going to be overwritten by 0xff.
> 
> The memset(0xff) looks dubious to me.  My guess is that this could be moved to 
> the vgabios-cirrus which would solve your problem.
>
> > In this regard, don't you think it would be advantageous to Qemu in
> > general not to reset the videram in resume? It can be pretty large, so
> > it is a significant waste of a memset.
> 
> It claims to fix a real bug.  Moving the memset to vgabios would do what you 
> want to do in a more robust way I think.
 
I think it does fix a bug (Win2K expects RAM to be 0xff at boot, or so a
comment on qemu-xen claims) but certainly it is not supposed to run at
restore time.
I agree that moving the memset to vgabios should be a better way to fix
the problem, I'll give it a look.
Unfortutely it means finding a Win2K install CD to repro the bug, sigh.

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Anthony Liguori]

On 01/23/2012 11:05 AM, Stefano Stabellini wrote:
> On Mon, 23 Jan 2012, Anthony Liguori wrote:
>>> However the issue of patch #4, "do not reset videoram on resume", still
>>> remains: no matter what parameter I pass to Qemu, if qemu_system_reset
>>> is called on resume the videoram is going to be overwritten by 0xff.
>>
>> The memset(0xff) looks dubious to me.  My guess is that this could be moved to
>> the vgabios-cirrus which would solve your problem.
>>
>>> In this regard, don't you think it would be advantageous to Qemu in
>>> general not to reset the videram in resume? It can be pretty large, so
>>> it is a significant waste of a memset.
>>
>> It claims to fix a real bug.  Moving the memset to vgabios would do what you
>> want to do in a more robust way I think.
>
> I think it does fix a bug (Win2K expects RAM to be 0xff at boot, or so a
> comment on qemu-xen claims) but certainly it is not supposed to run at
> restore time.
> I agree that moving the memset to vgabios should be a better way to fix
> the problem, I'll give it a look.
> Unfortutely it means finding a Win2K install CD to repro the bug, sigh.

Right, it's a bit annoying, but a much nicer solution :-)

Thanks,

Anthony Liguori

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Jan Kiszka]

On 2012-01-23 17:16, Stefano Stabellini wrote:
> On Mon, 23 Jan 2012, Jan Kiszka wrote:
>> On 2012-01-23 15:46, Stefano Stabellini wrote:
>>> On Mon, 23 Jan 2012, Jan Kiszka wrote:
>>>> On 2012-01-23 12:59, Stefano Stabellini wrote:
>>>>> On Mon, 23 Jan 2012, Jan Kiszka wrote:
>>>>>>>> Or what is the ordering
>>>>>>>> of init, RAM restore, and initial device reset now?
>>>>>>>
>>>>>>> RAM restore (done by Xen)
>>>>>>>
>>>>>>> physmap rebuild (done by xen_hvm_init in qemu)
>>>>>>> pc_init()
>>>>>>> qemu_system_reset()
>>>>>>> load_vmstate()
>>>>>>
>>>>>> Hmm, are you sure that this is the only case where a device init or
>>>>>> reset handler writes to already restored guest memory? Preloading the
>>>>>> RAM this way is a non-standard scenario for QEMU, thus conceptually
>>>>>> fragile. Does restoring happen before QEMU is even started, or can this
>>>>>> point be controlled from QEMU?
>>>>>
>>>>> Consider that this only happens with non-MMIO device memory, in practice
>>>>> only videoram.
>>>>> Vmware VGA does not memset the videoram in the reset handler, while QXL
>>>>> already has the following:
>>>>>
>>>>>     /* pre loadvm reset must not touch QXLRam.  This lives in
>>>>>      * device memory, is migrated together with RAM and thus
>>>>>      * already loaded at this point */ if (!loadvm) {
>>>>>      qxl_reset_state(d); }
>>>>
>>>> Yes, but QEMU restores the RAM _after_ device reset, not before it.
>>>> That's the problem with the Xen way - it is against the current
>>>> QEMU standard.
>>>
>>> QEMU doesn't save/restore the RAM (and the videoram) at all on Xen.
>>
>> But it does otherwise, and that's the scenario the code you cited was
>> written for. It won't work as is under Xen.
> 
> Ah, I see your point now.
> In that regard, is the comment above even correct?
> I am referring to "migrated together with RAM and thus already loaded at
> this point"?

The comment is correct. It refers to the invocation of the function from
the vmload handler. Here, a flag is passed that says "don't overwrite".

> 
> 
>>> To reply to your previous question more clearly: at restore time Qemu on
>>> Xen would run in a non-standard scenario; the restore of the RAM happens
>>> before QEMU is even started.
>>>
>>> That is unfortunate but it would be very hard to change (I can give you
>>> more details if you are interested in the reasons why it would be so
>>> difficult).
>>
>> If you can't change this, you need to properly introduce this new
>> scenario - pre-initialized RAM - to the QEMU device model. Or you will
>> see breakage outside cirrus sooner or later as well. So it might be good
>> to explain the reason why it can't be changed under Xen when motivating
>> this concept extension to QEMU.
>  
> OK.
> Are you thinking about introducing this concept as a new runstate?
> This special runstate could be set at restore time only on Xen.
> 
> 
> BTW the main reasons for having Xen saving the RAM are:
> 
> - the need to support PV guests, that often run without Qemu;
> - the current save format, that is built around the fact that Xen saves the memory;
> - the fact that Qemu might be running in a very limited stub-domain.

Your problem is not the fact that guest RAM is restored by an external
component. Your problem is that QEMU has no control over the when. If
you fix this, you could coordinate the restoring with the initial device
reset and would solve all potential current and future issues, not only
this single cirrus related one.

Jan

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

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Anthony Liguori]

On 01/23/2012 11:13 AM, Jan Kiszka wrote:
>>>> To reply to your previous question more clearly: at restore time Qemu on
>>>> Xen would run in a non-standard scenario; the restore of the RAM happens
>>>> before QEMU is even started.
>>>>
>>>> That is unfortunate but it would be very hard to change (I can give you
>>>> more details if you are interested in the reasons why it would be so
>>>> difficult).
>>>
>>> If you can't change this, you need to properly introduce this new
>>> scenario - pre-initialized RAM - to the QEMU device model. Or you will
>>> see breakage outside cirrus sooner or later as well. So it might be good
>>> to explain the reason why it can't be changed under Xen when motivating
>>> this concept extension to QEMU.
>>
>> OK.
>> Are you thinking about introducing this concept as a new runstate?
>> This special runstate could be set at restore time only on Xen.
>>
>>
>> BTW the main reasons for having Xen saving the RAM are:
>>
>> - the need to support PV guests, that often run without Qemu;
>> - the current save format, that is built around the fact that Xen saves the memory;
>> - the fact that Qemu might be running in a very limited stub-domain.
>
> Your problem is not the fact that guest RAM is restored by an external
> component. Your problem is that QEMU has no control over the when. If
> you fix this, you could coordinate the restoring with the initial device
> reset and would solve all potential current and future issues, not only
> this single cirrus related one.

Generally speaking, RAM is an independent device in most useful cases.  Onboard 
RAM is a very special case because it's extremely unusual.

But since some video cards can make use of dedicated external RAM, I don't think 
any video card really depends on initial RAM state.

What's most likely here is that the VGA BIOS of a Cirrus card sets an initial 
RAM state during device initialization.

We really should view RAM as just another device so I don't like the idea of 
propagating a global concept of "when RAM is restored" because that treats it 
specially compared to other devices.

But viewing RAM as just another device, having Xen only restore a subset of 
devices should be a reasonable thing to do moving forward.  The main problem 
here I believe is that we have part of the VGA Bios functionality in the 
hardware emulation.

Regards,

Anthony Liguori

>
> Jan
>

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Jan Kiszka]

On 2012-01-23 18:18, Anthony Liguori wrote:
> On 01/23/2012 11:13 AM, Jan Kiszka wrote:
>>>>> To reply to your previous question more clearly: at restore time Qemu on
>>>>> Xen would run in a non-standard scenario; the restore of the RAM happens
>>>>> before QEMU is even started.
>>>>>
>>>>> That is unfortunate but it would be very hard to change (I can give you
>>>>> more details if you are interested in the reasons why it would be so
>>>>> difficult).
>>>>
>>>> If you can't change this, you need to properly introduce this new
>>>> scenario - pre-initialized RAM - to the QEMU device model. Or you will
>>>> see breakage outside cirrus sooner or later as well. So it might be good
>>>> to explain the reason why it can't be changed under Xen when motivating
>>>> this concept extension to QEMU.
>>>
>>> OK.
>>> Are you thinking about introducing this concept as a new runstate?
>>> This special runstate could be set at restore time only on Xen.
>>>
>>>
>>> BTW the main reasons for having Xen saving the RAM are:
>>>
>>> - the need to support PV guests, that often run without Qemu;
>>> - the current save format, that is built around the fact that Xen saves the memory;
>>> - the fact that Qemu might be running in a very limited stub-domain.
>>
>> Your problem is not the fact that guest RAM is restored by an external
>> component. Your problem is that QEMU has no control over the when. If
>> you fix this, you could coordinate the restoring with the initial device
>> reset and would solve all potential current and future issues, not only
>> this single cirrus related one.
> 
> Generally speaking, RAM is an independent device in most useful cases.  Onboard 
> RAM is a very special case because it's extremely unusual.
> 
> But since some video cards can make use of dedicated external RAM, I don't think 
> any video card really depends on initial RAM state.
> 
> What's most likely here is that the VGA BIOS of a Cirrus card sets an initial 
> RAM state during device initialization.
> 
> We really should view RAM as just another device so I don't like the idea of 
> propagating a global concept of "when RAM is restored" because that treats it 
> specially compared to other devices.
> 
> But viewing RAM as just another device, having Xen only restore a subset of 
> devices should be a reasonable thing to do moving forward.  The main problem 
> here I believe is that we have part of the VGA Bios functionality in the 
> hardware emulation.

To my understanding, QXL will break identically on Xen for the same
reason: the reset handler assumes it can deal with the VRAM as it likes.

Jan

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

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Anthony Liguori]

On 01/23/2012 11:31 AM, Jan Kiszka wrote:
> On 2012-01-23 18:18, Anthony Liguori wrote:
>> On 01/23/2012 11:13 AM, Jan Kiszka wrote:
>>>>>> To reply to your previous question more clearly: at restore time Qemu on
>>>>>> Xen would run in a non-standard scenario; the restore of the RAM happens
>>>>>> before QEMU is even started.
>>>>>>
>>>>>> That is unfortunate but it would be very hard to change (I can give you
>>>>>> more details if you are interested in the reasons why it would be so
>>>>>> difficult).
>>>>>
>>>>> If you can't change this, you need to properly introduce this new
>>>>> scenario - pre-initialized RAM - to the QEMU device model. Or you will
>>>>> see breakage outside cirrus sooner or later as well. So it might be good
>>>>> to explain the reason why it can't be changed under Xen when motivating
>>>>> this concept extension to QEMU.
>>>>
>>>> OK.
>>>> Are you thinking about introducing this concept as a new runstate?
>>>> This special runstate could be set at restore time only on Xen.
>>>>
>>>>
>>>> BTW the main reasons for having Xen saving the RAM are:
>>>>
>>>> - the need to support PV guests, that often run without Qemu;
>>>> - the current save format, that is built around the fact that Xen saves the memory;
>>>> - the fact that Qemu might be running in a very limited stub-domain.
>>>
>>> Your problem is not the fact that guest RAM is restored by an external
>>> component. Your problem is that QEMU has no control over the when. If
>>> you fix this, you could coordinate the restoring with the initial device
>>> reset and would solve all potential current and future issues, not only
>>> this single cirrus related one.
>>
>> Generally speaking, RAM is an independent device in most useful cases.  Onboard
>> RAM is a very special case because it's extremely unusual.
>>
>> But since some video cards can make use of dedicated external RAM, I don't think
>> any video card really depends on initial RAM state.
>>
>> What's most likely here is that the VGA BIOS of a Cirrus card sets an initial
>> RAM state during device initialization.
>>
>> We really should view RAM as just another device so I don't like the idea of
>> propagating a global concept of "when RAM is restored" because that treats it
>> specially compared to other devices.
>>
>> But viewing RAM as just another device, having Xen only restore a subset of
>> devices should be a reasonable thing to do moving forward.  The main problem
>> here I believe is that we have part of the VGA Bios functionality in the
>> hardware emulation.
>
> To my understanding, QXL will break identically on Xen for the same
> reason: the reset handler assumes it can deal with the VRAM as it likes.

QXL also has a VGA BIOS that it could use to initialize VRAM.  A similar change 
could be made for it.

In general, devices shouldn't make assumptions about the state of other devices 
during reset.  Writing to RAM assumes that RAM has been fully initialized.  We 
don't express reset dependencies right now and it's not clear to me that this is 
something that we should be modeling.

Regards,

Anthony Liguori

>
> Jan
>

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Gerd Hoffmann]

  Hi,

>>> We really should view RAM as just another device so I don't like the
>>> idea of
>>> propagating a global concept of "when RAM is restored" because that
>>> treats it
>>> specially compared to other devices.
>>>
>>> But viewing RAM as just another device, having Xen only restore a
>>> subset of
>>> devices should be a reasonable thing to do moving forward.

I don't think modeling device memory (i.e. vga vram) as something
independent from the device (vga) is a good idea.  Because it isn't.

>> To my understanding, QXL will break identically on Xen for the same
>> reason: the reset handler assumes it can deal with the VRAM as it likes.

Yes.  Some data structures for host <-> guest communication are living
in device memory, and a reset initializes these.

> QXL also has a VGA BIOS that it could use to initialize VRAM.  A similar
> change could be made for it.

Hmm.  Not sure this is a good idea.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Avi Kivity]

On 01/23/2012 07:18 PM, Anthony Liguori wrote:
> Generally speaking, RAM is an independent device in most useful cases.  

Can you give examples?  Do you mean a subdevice with composition, or a
really independent device?

> Onboard RAM is a very special case because it's extremely unusual.

What's unusual (or even extremely unusual) about it?  I don't see a
difference between a few billion bits of state and, say, the few bits of
state in an RTC device.

> But since some video cards can make use of dedicated external RAM, I
> don't think any video card really depends on initial RAM state.
>
> What's most likely here is that the VGA BIOS of a Cirrus card sets an
> initial RAM state during device initialization.

Yes, that's likely.  DRAMs aren't reset and some state survives even a
short power off.

>
> We really should view RAM as just another device so I don't like the
> idea of propagating a global concept of "when RAM is restored" because
> that treats it specially compared to other devices.

Agree.  In fact the first step has been taken as now creating a RAM
region with memory_region_init_ram() doesn't register it for migration. 
The next step would be a VMSTATE_RAM() to make it part of the containing
device.

> But viewing RAM as just another device, having Xen only restore a
> subset of devices should be a reasonable thing to do moving forward. 
> The main problem here I believe is that we have part of the VGA Bios
> functionality in the hardware emulation.

Doesn't the main BIOS clear the screen first thing at boot?  Not even
sure the reset is needed.

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

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Avi Kivity]

On 01/24/2012 12:21 PM, Gerd Hoffmann wrote:
> >>>
> >>> But viewing RAM as just another device, having Xen only restore a
> >>> subset of
> >>> devices should be a reasonable thing to do moving forward.
>
> I don't think modeling device memory (i.e. vga vram) as something
> independent from the device (vga) is a good idea.  Because it isn't.

Right.  We should have VMSTATE_RAM() to express the dependency.

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

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Paolo Bonzini]

On 01/24/2012 12:10 PM, Avi Kivity wrote:
>> But viewing RAM as just another device, having Xen only restore a
>> subset of devices should be a reasonable thing to do moving forward.
>> The main problem here I believe is that we have part of the VGA Bios
>> functionality in the hardware emulation.
>
> Doesn't the main BIOS clear the screen first thing at boot?  Not even
> sure the reset is needed.

Clearing the screen should only write to the RAM at 0xB8000 (and perhaps 
0xA0000 since IIRC it's where text-mode fonts lie).  The option ROM 
cannot even assume that the main BIOS knows about the VESA framebuffer, 
can it?

Paolo

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Avi Kivity]

On 01/24/2012 01:27 PM, Paolo Bonzini wrote:
> On 01/24/2012 12:10 PM, Avi Kivity wrote:
>>> But viewing RAM as just another device, having Xen only restore a
>>> subset of devices should be a reasonable thing to do moving forward.
>>> The main problem here I believe is that we have part of the VGA Bios
>>> functionality in the hardware emulation.
>>
>> Doesn't the main BIOS clear the screen first thing at boot?  Not even
>> sure the reset is needed.
>
> Clearing the screen should only write to the RAM at 0xB8000 (and
> perhaps 0xA0000 since IIRC it's where text-mode fonts lie).  The
> option ROM cannot even assume that the main BIOS knows about the VESA
> framebuffer, can it?

Yes, but why should anything else be needed?

When you switch to a graphics mode, clear as much of the framebuffer as
you need.

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

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Paolo Bonzini]

On 01/24/2012 12:32 PM, Avi Kivity wrote:
>> >  Clearing the screen should only write to the RAM at 0xB8000 (and
>> >  perhaps 0xA0000 since IIRC it's where text-mode fonts lie).  The
>> >  option ROM cannot even assume that the main BIOS knows about the VESA
>> >  framebuffer, can it?
> Yes, but why should anything else be needed?
>
> When you switch to a graphics mode, clear as much of the framebuffer as
> you need.

Based on the comments, Windows apparently disagrees. :)

Paolo

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Stefano Stabellini]

On Tue, 24 Jan 2012, Avi Kivity wrote:
> On 01/24/2012 01:27 PM, Paolo Bonzini wrote:
> > On 01/24/2012 12:10 PM, Avi Kivity wrote:
> >>> But viewing RAM as just another device, having Xen only restore a
> >>> subset of devices should be a reasonable thing to do moving forward.
> >>> The main problem here I believe is that we have part of the VGA Bios
> >>> functionality in the hardware emulation.
> >>
> >> Doesn't the main BIOS clear the screen first thing at boot?  Not even
> >> sure the reset is needed.
> >
> > Clearing the screen should only write to the RAM at 0xB8000 (and
> > perhaps 0xA0000 since IIRC it's where text-mode fonts lie).  The
> > option ROM cannot even assume that the main BIOS knows about the VESA
> > framebuffer, can it?
> 
> Yes, but why should anything else be needed?
> 
> When you switch to a graphics mode, clear as much of the framebuffer as
> you need.

After installing Win2K, I managed to reproduce the issue with the old
qemu-xen and rombios (removing the memset in the vga emulator).
However I cannot reproduce the issue with upstream qemu and seabios
(even if I remove the memset).

I think that the memset was a workaround a bug in rombios, hence it is
not needed anymore.

Removing the cirrus_vga memset would solve the main issue we have left
with save/restore on Xen.
However it wouldn't solve the problem for QXL: as Gerd pointed out, QXL
needs a reset to initialize some memory regions, so another "if we are
restoring don't do that" would be required to make it work on Xen...

Maybe we can extend the loadvm == 1 to all the reset performed before
load_vmstate?

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Stefano Stabellini]

On Tue, 24 Jan 2012, Avi Kivity wrote:
> On 01/24/2012 12:21 PM, Gerd Hoffmann wrote:
> > >>>
> > >>> But viewing RAM as just another device, having Xen only restore a
> > >>> subset of
> > >>> devices should be a reasonable thing to do moving forward.
> >
> > I don't think modeling device memory (i.e. vga vram) as something
> > independent from the device (vga) is a good idea.  Because it isn't.
> 
> Right.  We should have VMSTATE_RAM() to express the dependency.

So if I understand correctly you are planning on removing the call to
vmstate_register_ram_global in hw/vga.c?
In that case it might be better if I wait for your "VMSTATE_RAM" patch
before I can send a new patch to save only non-RAM devices, because I
guess they will clash with one another.

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Anthony Liguori]

On 01/24/2012 05:10 AM, Avi Kivity wrote:
> On 01/23/2012 07:18 PM, Anthony Liguori wrote:
>> Generally speaking, RAM is an independent device in most useful cases.
>
> Can you give examples?  Do you mean a subdevice with composition, or a
> really independent device?

I expect we'll have one Ram device.  It's size will be configurable.  One Ram 
device will hang off of the machine which would be the main ram.

A video card would have a Ram device via composition.

The important consideration about reset is how it propagates.  My expectation is 
that we'll propagate reset through the composition tree in a preorder 
transversal.  That means in the VGA device reset function, it's child device 
(Ram) has not been reset yet.

>> Onboard RAM is a very special case because it's extremely unusual.
>
> What's unusual (or even extremely unusual) about it?  I don't see a
> difference between a few billion bits of state and, say, the few bits of
> state in an RTC device.

Okay, but let's not get philosophical here :-)  There are very few devices that 
have a DRAM chip that is mapped through a bar into the main address space and 
behaves (usually) like normal RAM.

>> We really should view RAM as just another device so I don't like the
>> idea of propagating a global concept of "when RAM is restored" because
>> that treats it specially compared to other devices.
>
> Agree.  In fact the first step has been taken as now creating a RAM
> region with memory_region_init_ram() doesn't register it for migration.
> The next step would be a VMSTATE_RAM() to make it part of the containing
> device.

That's not necessary (or wise).

Let's not confuse a Ram device with a MemoryRegion.  They are separate things 
and should be treated as separate things.  I thought we discussed that 
MemoryRegions are stateless (or at least, there state is all derived) and don't 
need to be serialized?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Anthony Liguori]

On 01/24/2012 05:52 AM, Stefano Stabellini wrote:
> On Tue, 24 Jan 2012, Avi Kivity wrote:
>> On 01/24/2012 01:27 PM, Paolo Bonzini wrote:
>>> On 01/24/2012 12:10 PM, Avi Kivity wrote:
>>>>> But viewing RAM as just another device, having Xen only restore a
>>>>> subset of devices should be a reasonable thing to do moving forward.
>>>>> The main problem here I believe is that we have part of the VGA Bios
>>>>> functionality in the hardware emulation.
>>>>
>>>> Doesn't the main BIOS clear the screen first thing at boot?  Not even
>>>> sure the reset is needed.
>>>
>>> Clearing the screen should only write to the RAM at 0xB8000 (and
>>> perhaps 0xA0000 since IIRC it's where text-mode fonts lie).  The
>>> option ROM cannot even assume that the main BIOS knows about the VESA
>>> framebuffer, can it?
>>
>> Yes, but why should anything else be needed?
>>
>> When you switch to a graphics mode, clear as much of the framebuffer as
>> you need.
>
> After installing Win2K, I managed to reproduce the issue with the old
> qemu-xen and rombios (removing the memset in the vga emulator).
> However I cannot reproduce the issue with upstream qemu and seabios
> (even if I remove the memset).
>
> I think that the memset was a workaround a bug in rombios, hence it is
> not needed anymore.
>
> Removing the cirrus_vga memset would solve the main issue we have left
> with save/restore on Xen.
> However it wouldn't solve the problem for QXL: as Gerd pointed out, QXL
> needs a reset to initialize some memory regions, so another "if we are
> restoring don't do that" would be required to make it work on Xen...

I believe this is a bug in QXL (or misdesign).  It should move this logic into 
the VGA Bios.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Anthony Liguori]

On 01/24/2012 05:13 AM, Avi Kivity wrote:
> On 01/24/2012 12:21 PM, Gerd Hoffmann wrote:
>>>>>
>>>>> But viewing RAM as just another device, having Xen only restore a
>>>>> subset of
>>>>> devices should be a reasonable thing to do moving forward.
>>
>> I don't think modeling device memory (i.e. vga vram) as something
>> independent from the device (vga) is a good idea.  Because it isn't.
>
> Right.  We should have VMSTATE_RAM() to express the dependency.

No, VMSTATE has nothign to do with reset.

Ram should be a device and then you can hook up ram through the composition tree.

But reset is going to propagate preorder, not postorder, so this isn't going to 
help anyway.

Postorder initialization doesn't make a whole lot of sense when you think about 
the semantics of it.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Anthony Liguori]

On 01/24/2012 04:21 AM, Gerd Hoffmann wrote:
>    Hi,
>
>>>> We really should view RAM as just another device so I don't like the
>>>> idea of
>>>> propagating a global concept of "when RAM is restored" because that
>>>> treats it
>>>> specially compared to other devices.
>>>>
>>>> But viewing RAM as just another device, having Xen only restore a
>>>> subset of
>>>> devices should be a reasonable thing to do moving forward.
>
> I don't think modeling device memory (i.e. vga vram) as something
> independent from the device (vga) is a good idea.  Because it isn't.

It is, but probably not in the way you're assuming that I mean it is.

>
>>> To my understanding, QXL will break identically on Xen for the same
>>> reason: the reset handler assumes it can deal with the VRAM as it likes.
>
> Yes.  Some data structures for host<->  guest communication are living
> in device memory, and a reset initializes these.

This should be done by firmware or system software.  Devices don't initialize 
memory with data structures on power up.  The first problem with doing this is 
that it assumes a reset order which would lengthen the quiescing process.  The 
second problem is that this would require fairly sophisticated logic that could 
just as well live in firmware.

The advantage of not enabling a device to behave like this is it lets us do a 
much simpler reset model.  To model this properly, we would have to support 
multiple reset propagation hooks (at least a preorder and postorder hook).  This 
adds a fair amount of complication for not a lot of benefit (the only benefit is 
moving firmware logic into QEMU which is really a downside :-)).

Regards,

Anthony Liguori

>
>> QXL also has a VGA BIOS that it could use to initialize VRAM.  A similar
>> change could be made for it.
>
> Hmm.  Not sure this is a good idea.
>
> cheers,
>    Gerd
>
>

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Avi Kivity]

On 01/24/2012 02:00 PM, Stefano Stabellini wrote:
> On Tue, 24 Jan 2012, Avi Kivity wrote:
> > On 01/24/2012 12:21 PM, Gerd Hoffmann wrote:
> > > >>>
> > > >>> But viewing RAM as just another device, having Xen only restore a
> > > >>> subset of
> > > >>> devices should be a reasonable thing to do moving forward.
> > >
> > > I don't think modeling device memory (i.e. vga vram) as something
> > > independent from the device (vga) is a good idea.  Because it isn't.
> > 
> > Right.  We should have VMSTATE_RAM() to express the dependency.
>
> So if I understand correctly you are planning on removing the call to
> vmstate_register_ram_global in hw/vga.c?

I can't say I'm planning to do so, but that's the direction I think we
need to go.

> In that case it might be better if I wait for your "VMSTATE_RAM" patch
> before I can send a new patch to save only non-RAM devices, because I
> guess they will clash with one another.

I don't understand what sense a patch to save non-RAM devices makes (or
even, what non-RAM devices are).

We have a special case here where Xen manages the RAM even though qemu
thinks it does.  IMO an if (xen) is the best way to express this
specialness.

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

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Avi Kivity]

On 01/24/2012 03:18 PM, Anthony Liguori wrote:
> On 01/24/2012 05:13 AM, Avi Kivity wrote:
>> On 01/24/2012 12:21 PM, Gerd Hoffmann wrote:
>>>>>>
>>>>>> But viewing RAM as just another device, having Xen only restore a
>>>>>> subset of
>>>>>> devices should be a reasonable thing to do moving forward.
>>>
>>> I don't think modeling device memory (i.e. vga vram) as something
>>> independent from the device (vga) is a good idea.  Because it isn't.
>>
>> Right.  We should have VMSTATE_RAM() to express the dependency.
>
> No, VMSTATE has nothign to do with reset.

It's so that the device's post_load happens after the RAM was loaded.

>
> Ram should be a device and then you can hook up ram through the
> composition tree.

I think that's too heavy a hammer.  Think about things like ivshmem. 
Does it really need to be a composite device?  If we keep going in this
direction the amount of know how needed to write a device for qemu will
be overwhelming.

RAM is a large array of bits.  We model an 32-bit register with a
uint32_t, memory_region_init_io(), and some vmstate.  We should be able
to do the same thing for RAM.

>
> But reset is going to propagate preorder, not postorder, so this isn't
> going to help anyway.
>
> Postorder initialization doesn't make a whole lot of sense when you
> think about the semantics of it.

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

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Avi Kivity]

On 01/24/2012 03:25 PM, Anthony Liguori wrote:
>
>>
>>>> To my understanding, QXL will break identically on Xen for the same
>>>> reason: the reset handler assumes it can deal with the VRAM as it
>>>> likes.
>>
>> Yes.  Some data structures for host<->  guest communication are living
>> in device memory, and a reset initializes these.
>
> This should be done by firmware or system software.  

It should be done in the way the device spec says it does.  What does
the qxl spec says?  If it's lacking, we need to fill it up from existing
practice.

> Devices don't initialize memory with data structures on power up.  The
> first problem with doing this is that it assumes a reset order which
> would lengthen the quiescing process.  The second problem is that this
> would require fairly sophisticated logic that could just as well live
> in firmware.

If it's in device firmware, there's no way to tell.  Nothing prevents
the device from clearing its on-board memory during reset.

>
> The advantage of not enabling a device to behave like this is it lets
> us do a much simpler reset model.  To model this properly, we would
> have to support multiple reset propagation hooks (at least a preorder
> and postorder hook).  This adds a fair amount of complication for not
> a lot of benefit (the only benefit is moving firmware logic into QEMU
> which is really a downside :-)).
>

You're arguing about changing a device so it could fit qemu better.  It
should be the other way around.  Granted this is a paravirt device so we
have more leeway, but still qxl is out there and we can't change it.

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

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Avi Kivity]

On 01/24/2012 01:44 PM, Paolo Bonzini wrote:
> On 01/24/2012 12:32 PM, Avi Kivity wrote:
>>> >  Clearing the screen should only write to the RAM at 0xB8000 (and
>>> >  perhaps 0xA0000 since IIRC it's where text-mode fonts lie).  The
>>> >  option ROM cannot even assume that the main BIOS knows about the
>>> VESA
>>> >  framebuffer, can it?
>> Yes, but why should anything else be needed?
>>
>> When you switch to a graphics mode, clear as much of the framebuffer as
>> you need.
>
> Based on the comments, Windows apparently disagrees. :)

Based on the testing, it does not.

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

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Avi Kivity]

On 01/24/2012 03:14 PM, Anthony Liguori wrote:
> On 01/24/2012 05:10 AM, Avi Kivity wrote:
>> On 01/23/2012 07:18 PM, Anthony Liguori wrote:
>>> Generally speaking, RAM is an independent device in most useful cases.
>>
>> Can you give examples?  Do you mean a subdevice with composition, or a
>> really independent device?
>
> I expect we'll have one Ram device.  It's size will be configurable. 
> One Ram device will hang off of the machine which would be the main ram.

We'll also have a hotpluggable variant which talks to some GPIOs.  A
motherboard may support multiple slots with such devices.

> A video card would have a Ram device via composition.

IMO, overkill.

>
> The important consideration about reset is how it propagates.  My
> expectation is that we'll propagate reset through the composition tree
> in a preorder transversal.  That means in the VGA device reset
> function, it's child device (Ram) has not been reset yet.

Doesn't depth first make more sense?  A bus is considered reset after
all devices in the bus have been reset.


>
>>> We really should view RAM as just another device so I don't like the
>>> idea of propagating a global concept of "when RAM is restored" because
>>> that treats it specially compared to other devices.
>>
>> Agree.  In fact the first step has been taken as now creating a RAM
>> region with memory_region_init_ram() doesn't register it for migration.
>> The next step would be a VMSTATE_RAM() to make it part of the containing
>> device.
>
> That's not necessary (or wise).
>
> Let's not confuse a Ram device with a MemoryRegion.  They are separate
> things and should be treated as separate things.  I thought we
> discussed that MemoryRegions are stateless (or at least, there state
> is all derived) and don't need to be serialized?

Well, the actual bits in memory are state.  All other attributes are
indeed derived.

I just think that making any device that has a bit of RAM a composed
device is overkill.  What do we gain from it?  The cost is not trivial.

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

Re: [Qemu-devel] [PATCH v4 0/6] save/restore on Xen

[Anthony Liguori]

On 01/24/2012 09:56 AM, Avi Kivity wrote:
> On 01/24/2012 03:14 PM, Anthony Liguori wrote:
>> On 01/24/2012 05:10 AM, Avi Kivity wrote:
>>> On 01/23/2012 07:18 PM, Anthony Liguori wrote:
>>>> Generally speaking, RAM is an independent device in most useful cases.
>>>
>>> Can you give examples?  Do you mean a subdevice with composition, or a
>>> really independent device?
>>
>> I expect we'll have one Ram device.  It's size will be configurable.
>> One Ram device will hang off of the machine which would be the main ram.
>
> We'll also have a hotpluggable variant which talks to some GPIOs.  A
> motherboard may support multiple slots with such devices.

Yup.  And boards can subclass the Ram device in order to be able to restrict the 
valid RAM sizes.

-m just becomes an analysis to set the ram size for a ram device.  The 
infrastructure around RAMBlock, etc. would go away as each RAMBlock would 
correspond to a Ram device.

>> A video card would have a Ram device via composition.
>
> IMO, overkill.

I think it's a lot of refactoring to get there and it's not the most critical 
infrastructure changes we have coming up, but I think it would be nice to get 
there eventually.

>
>>
>> The important consideration about reset is how it propagates.  My
>> expectation is that we'll propagate reset through the composition tree
>> in a preorder transversal.  That means in the VGA device reset
>> function, it's child device (Ram) has not been reset yet.
>
> Doesn't depth first make more sense?

Yes, it would be depth first, but you're looking for a preorder and post order 
hook.  In otherwords:

void do_reset(DeviceState *dev)
{
     dev->pre_reset(dev);  // this is preorder
     do_reset_all(dev->children);
     dev->post_reset(dev);  // this is postorder
}

The PCI bus overloads the reset handler and does a post-order reset.  I guess I 
can rationalize it after all but it's not entirely clear to me that a pre_reset 
hook isn't necessary too.

>  A bus is considered reset after
> all devices in the bus have been reset.
>
>
>>
>>>> We really should view RAM as just another device so I don't like the
>>>> idea of propagating a global concept of "when RAM is restored" because
>>>> that treats it specially compared to other devices.
>>>
>>> Agree.  In fact the first step has been taken as now creating a RAM
>>> region with memory_region_init_ram() doesn't register it for migration.
>>> The next step would be a VMSTATE_RAM() to make it part of the containing
>>> device.
>>
>> That's not necessary (or wise).
>>
>> Let's not confuse a Ram device with a MemoryRegion.  They are separate
>> things and should be treated as separate things.  I thought we
>> discussed that MemoryRegions are stateless (or at least, there state
>> is all derived) and don't need to be serialized?
>
> Well, the actual bits in memory are state.  All other attributes are
> indeed derived.
>
> I just think that making any device that has a bit of RAM a composed
> device is overkill.  What do we gain from it?  The cost is not trivial.

The cost of composition is pretty trivial.

struct VGADevice {
    DeviceState parent;

    Ram vga_ram;
};

static void vga_device_initfn(Object *obj)
{
    VGADevice *vga = VGA(obj);

    object_property_add_child(obj, "vram", (Object *)&vga_ram,
                              TYPE_RAM, NULL);
}

Now a user can control the VGA ram size by accessing vga/vram.size = 16M.

Regards,

Anthony Liguori