[Qemu-devel] [PATCH RFC 0/5] New VMState table based load/save infrastructure

[Qemu-devel] [PATCH RFC 0/5] New VMState table based load/save infrastructure

[Juan Quintela]

This series implement a new save/load approach for device state.  I want to
start a discussion on how is better to design it.  This is the reason why only
one device has been ported to this new approach.

I increased the version_id of apic state, just to sent the 3 arrays
(isr, tmr, irr) as arrays.  I didn't want to introduce structure support
before there is any discussion about this, not that it is imposible to do
it in a compatible way.

Problems of current approach:
- have to maintain both load and save functions on sync
- it is not type safe
- you can't use easily the device save/load information to do more things.
  For instance, print the state of a device in the monitor. To do this, you
  basically have to copy foo_apic() function.
- You can't examine the saved images in any meaningful way

New approach:
- you only have to declare the state once, in a table, save/load functions are
  generic for all devices.
- it is type safe
- it is qdev-like design (i.e. you don't have to learn yet a completely
  different appearch for a similar problem)
- it is trivial to add new operations to VMStateInfo, and then you can use
  them anywhere (for instance printing in the monitor, etc).

Problems of new approach:
- it is less flexible, you can't progamatically save in any order that you want.
  It is not clear to me that we _want_ that flexibility.  Look for instance
  and hw/msix.c and think if you can be sure that save/load are on sync, and
  as soon as we get new versions, that things could be tested to be wright.

What do I want to discuss:
a- do we want to be able to load state form old versions?
   I am not sure that we are going to get this right for complex devices,
   and I don't completely see how we can have any kind of testing here.
   There was already a discussion about this on the list.
b- Is this the right approach?  What more do we want/need?
   For instance, implementing struct save support, and calling
   other "sub-descriptions" is not difficult, we just have to decide
   if we want it.
c- In the current approach, we have loops to send arrays,  I think that one
   got already done better on new approarch.  But we don't have support
   for ifs (see hw/ide.c
       if (s->identify_set) {
        qemu_get_buffer(f, (uint8_t *)s->identify_data, 512);
    }
   Do we want support for things like that?
d- how aggresive should the new design be?  i.e. be able to be compatible with
   old design is good, or can we start with a clean sheet and just remove the
   gotchas of the previous design?

Comments?

Later, Juan.

P.D. the 1st two patches just improve loadvm error messages.  I included then
     here because they make easier to play with the state/versions.  Thinking
     about proper solution that will be sent in a different patch.

Juan Quintela (5):
  loadvm already call vm_start()
  Don't call vm_start() if there was an error loading
  Don't ignore load_state() error return values.
  New VMstate save/load infrastructure
  Port apic to new VMState design

 hw/apic.c |   96 +++++-------------
 hw/hw.h   |  124 +++++++++++++++++++++++
 savevm.c  |  322 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 vl.c      |    4 +-
 4 files changed, 470 insertions(+), 76 deletions(-)

[Qemu-devel] [PATCH 1/5] loadvm already call vm_start()

[Juan Quintela]

If there are any error.  do_loadvm() will not call vm_start() and we would call it on the next line

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 vl.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/vl.c b/vl.c
index 8b2b289..eb04d1c 100644
--- a/vl.c
+++ b/vl.c
@@ -6036,8 +6036,10 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }

-    if (loadvm)
+    if (loadvm) {
+        autostart = 0;
         do_loadvm(cur_mon, loadvm);
+    }

     if (incoming) {
         autostart = 0;
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/5] Don't call vm_start() if there was an error loading

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/savevm.c b/savevm.c
index 570377f..7779c0e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1211,6 +1211,7 @@ void do_loadvm(Monitor *mon, const char *name)
     qemu_fclose(f);
     if (ret < 0) {
         monitor_printf(mon, "Error %d while loading VM state\n", ret);
+        return;
     }
  the_end:
     if (saved_vm_running)
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/5] Don't ignore load_state() error return values.

[Juan Quintela]

An error value here means that some device was not able to load it state.  There is no hope at this point.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/savevm.c b/savevm.c
index 7779c0e..55a1b50 100644
--- a/savevm.c
+++ b/savevm.c
@@ -955,7 +955,13 @@ int qemu_loadvm_state(QEMUFile *f)
             le->next = first_le;
             first_le = le;

-            le->se->load_state(f, le->se->opaque, le->version_id);
+            ret = le->se->load_state(f, le->se->opaque, le->version_id);
+            if (ret < 0) {
+                fprintf(stderr, "qemu: warning: error while loading state for instance 0x%x of device '%s'\n",
+                        instance_id, idstr);
+                ret = -EINVAL;
+                goto out;
+            }
             break;
         case QEMU_VM_SECTION_PART:
         case QEMU_VM_SECTION_END:
@@ -968,7 +974,13 @@ int qemu_loadvm_state(QEMUFile *f)
                 goto out;
             }

-            le->se->load_state(f, le->se->opaque, le->version_id);
+            ret = le->se->load_state(f, le->se->opaque, le->version_id);
+            if (ret != 0) {
+                fprintf(stderr, "qemu: warning: error while loading state section '%d'\n",
+                        section_id);
+                ret = -EINVAL;
+                goto out;
+            }
             break;
         default:
             fprintf(stderr, "Unknown savevm section type %d\n", section_type);
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/5] New VMstate save/load infrastructure

[Juan Quintela]

This patch introduces VMState infrastructure, to convert the save/load
functions of devices to a table approach.  This new approach has the
advantages:
- it is type-safe
- you can't have load/save functions out of sync
- will allows us to have new interesting commands, like dump <device>, that
  shows all its internal state.
- Just now, the only added type is arrays, but we can add structures.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/hw.h  |  124 +++++++++++++++++++++++++
 savevm.c |  309 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 427 insertions(+), 6 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index 1e5783d..19ebef2 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -269,4 +269,128 @@ typedef int QEMUBootSetHandler(void *opaque, const char *boot_devices);
 void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque);
 int qemu_boot_set(const char *boot_devices);

+enum VMStateType {
+    VMSTATE_TYPE_UNSPEC = 0,
+    VMSTATE_TYPE_INT8,
+    VMSTATE_TYPE_INT16,
+    VMSTATE_TYPE_INT32,
+    VMSTATE_TYPE_INT64,
+    VMSTATE_TYPE_UINT8,
+    VMSTATE_TYPE_UINT16,
+    VMSTATE_TYPE_UINT32,
+    VMSTATE_TYPE_UINT64,
+    VMSTATE_TYPE_TIMER,
+};
+
+typedef struct VMStateInfo VMStateInfo;
+
+struct VMStateInfo {
+    const char *name;
+    size_t size;
+    enum VMStateType type;
+    void (*get)(QEMUFile *f, VMStateInfo *info, void *pv);
+    void (*put)(QEMUFile *f, VMStateInfo *info, const void *pv);
+};
+
+typedef struct {
+    const char *name;
+    size_t num;
+    size_t offset;
+    VMStateInfo *info;
+    int version_id;
+} VMStateField;
+
+typedef struct {
+    const char *name;
+    int version_id;
+    int minimum_version_id;
+    VMStateField *fields;
+} VMStateDescription;
+
+extern VMStateInfo vmstate_info_int8;
+extern VMStateInfo vmstate_info_int16;
+extern VMStateInfo vmstate_info_int32;
+extern VMStateInfo vmstate_info_int64;
+
+extern VMStateInfo vmstate_info_uint8;
+extern VMStateInfo vmstate_info_uint16;
+extern VMStateInfo vmstate_info_uint32;
+extern VMStateInfo vmstate_info_uint64;
+
+extern VMStateInfo vmstate_info_timer;
+
+#define typeof_field2(type, field) typeof(((type *)0)->field)
+#define type_check2(t1,t2) ((t1*)0 - (t2*)0)
+#define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0)
+
+#define VMSTATE_FIELD(_field, _state, _version, _info, _type) {           \
+    .name       = (stringify(_field)),                                     \
+    .num        = 1,                                                       \
+    .version_id = (_version),                                              \
+    .info       = &(_info),                                                \
+    .offset     = offsetof(_state, _field)                                 \
+            + type_check2(_type,typeof_field2(_state, _field))            \
+}
+
+#define VMSTATE_FIELD_A(_field, _state, _version, _num, _info, _type) {   \
+    .name       = (stringify(_field)),                                     \
+    .num        = (_num),                                                  \
+    .version_id = (_version),                                              \
+    .info       = &(_info),                                                \
+    .offset     = offsetof(_state, _field)                                 \
+            + type_check_array(_type,typeof_field2(_state, _field),_num)  \
+}
+
+/* _f : field name
+   _s : struct state name
+   _v : version
+   _n : num of elements
+*/
+
+
+#define VMSTATE_INT8_ARRAY(_f, _s, _v, _n)                            \
+    VMSTATE_FIELD_A(_f, _s, _v, _n, vmstate_info_int8, int8_t)
+#define VMSTATE_INT16_ARRAY(_f, _s, _v, _n)                           \
+    VMSTATE_FIELD_A(_f, _s, _v, _n, vmstate_info_int16, int16_t)
+#define VMSTATE_INT32_ARRAY(_f, _s, _v, _n)                           \
+    VMSTATE_FIELD_A(_f, _s, _v, _n, vmstate_info_int32, int32_t)
+#define VMSTATE_INT64_ARRAY(_f, _s, _v, _n)                           \
+    VMSTATE_FIELD_A(_f, _s, _v, _n, vmstate_info_int64, int64_t)
+
+#define VMSTATE_UINT8_ARRAY(_f, _s, _v, _n)                           \
+    VMSTATE_FIELD_A(_f, _s, _v, _n, vmstate_info_uint8, uint8_t)
+#define VMSTATE_UINT16_ARRAY(_f, _s, _v, _n)                          \
+    VMSTATE_FIELD_A(_f, _s, _v, _n, vmstate_info_uint16, uint16_t)
+#define VMSTATE_UINT32_ARRAY(_f, _s, _v, _n)                          \
+    VMSTATE_FIELD_A(_f, _s, _v, _n, vmstate_info_uint32, uint32_t)
+#define VMSTATE_UINT64_ARRAY(_f, _s, _v, _n)                          \
+    VMSTATE_FIELD_A(_f, _s, _v, _n, vmstate_info_uint64, uint64_t)
+
+#define VMSTATE_INT8(_f, _s, _v)                                      \
+    VMSTATE_FIELD(_f, _s, _v, vmstate_info_int8, int8_t)
+#define VMSTATE_INT16(_f, _s, _v)                                     \
+    VMSTATE_FIELD(_f, _s, _v, vmstate_info_int16, int16_t)
+#define VMSTATE_INT32(_f, _s, _v)                                     \
+    VMSTATE_FIELD(_f, _s, _v, vmstate_info_int32, int32_t)
+#define VMSTATE_INT64(_f, _s, _v)                                     \
+    VMSTATE_FIELD(_f, _s, _v, vmstate_info_int64, int64_t)
+
+#define VMSTATE_UINT8(_f, _s, _v)                                     \
+    VMSTATE_FIELD(_f, _s, _v, vmstate_info_uint8, uint8_t)
+#define VMSTATE_UINT16(_f, _s, _v)                                    \
+    VMSTATE_FIELD(_f, _s, _v, vmstate_info_uint16, uint16_t)
+#define VMSTATE_UINT32(_f, _s, _v)                                    \
+    VMSTATE_FIELD(_f, _s, _v, vmstate_info_uint32, uint32_t)
+#define VMSTATE_UINT64(_f, _s, _v)                                    \
+    VMSTATE_FIELD(_f, _s, _v, vmstate_info_uint64, uint64_t)
+
+#define VMSTATE_TIMER(_f, _s, _v)                                     \
+    VMSTATE_FIELD(_f, _s, _v, vmstate_info_timer, QEMUTimer *)
+
+#define VMSTATE_END_OF_LIST()               \
+    {}
+
+extern int vmstate_register(int instance_id, VMStateDescription *vmsd,
+                            void *base);
+extern void vmstate_unregister(const char *idstr, void *opaque);
 #endif
diff --git a/savevm.c b/savevm.c
index 55a1b50..aff4c82 100644
--- a/savevm.c
+++ b/savevm.c
@@ -610,6 +610,204 @@ uint64_t qemu_get_be64(QEMUFile *f)
     return v;
 }

+/* 8 bit int */
+
+static void get_int8(QEMUFile *f, VMStateInfo *info, void *pv)
+{
+    int8_t *v = pv;
+    qemu_get_s8s(f, v);
+}
+
+static void put_int8(QEMUFile *f, VMStateInfo *info, const void *pv)
+{
+    const int8_t *v = pv;
+    qemu_put_s8s(f, v);
+}
+
+VMStateInfo vmstate_info_int8 = {
+    .name  = "int8",
+    .type  = VMSTATE_TYPE_INT8,
+    .size  = sizeof(int8_t),
+    .get = get_int8,
+    .put = put_int8,
+};
+
+/* 16 bit int */
+
+static void get_int16(QEMUFile *f, VMStateInfo *info, void *pv)
+{
+    int16_t *v = pv;
+    qemu_get_sbe16s(f, v);
+}
+
+static void put_int16(QEMUFile *f, VMStateInfo *info, const void *pv)
+{
+    const int16_t *v = pv;
+    qemu_put_sbe16s(f, v);
+}
+
+VMStateInfo vmstate_info_int16 = {
+    .name  = "int16",
+    .type  = VMSTATE_TYPE_INT16,
+    .size  = sizeof(int16_t),
+    .get = get_int16,
+    .put = put_int16,
+};
+
+/* 32 bit int */
+
+static void get_int32(QEMUFile *f, VMStateInfo *info, void *pv)
+{
+    int32_t *v = pv;
+    qemu_get_sbe32s(f, v);
+}
+
+static void put_int32(QEMUFile *f, VMStateInfo *info, const void *pv)
+{
+    const int32_t *v = pv;
+    qemu_put_sbe32s(f, v);
+}
+
+VMStateInfo vmstate_info_int32 = {
+    .name  = "int32",
+    .type  = VMSTATE_TYPE_INT32,
+    .size  = sizeof(int32_t),
+    .get = get_int32,
+    .put = put_int32,
+};
+
+/* 64 bit int */
+
+static void get_int64(QEMUFile *f, VMStateInfo *info, void *pv)
+{
+    int64_t *v = pv;
+    qemu_get_sbe64s(f, v);
+}
+
+static void put_int64(QEMUFile *f, VMStateInfo *info, const void *pv)
+{
+    const int64_t *v = pv;
+    qemu_put_sbe64s(f, v);
+}
+
+VMStateInfo vmstate_info_int64 = {
+    .name  = "int64",
+    .type  = VMSTATE_TYPE_INT64,
+    .size  = sizeof(int64_t),
+    .get = get_int64,
+    .put = put_int64,
+};
+
+/* 8 bit unsigned int */
+
+static void get_uint8(QEMUFile *f, VMStateInfo *info, void *pv)
+{
+    uint8_t *v = pv;
+    qemu_get_8s(f, v);
+}
+
+static void put_uint8(QEMUFile *f, VMStateInfo *info, const void *pv)
+{
+    const uint8_t *v = pv;
+    qemu_put_8s(f, v);
+}
+
+VMStateInfo vmstate_info_uint8 = {
+    .name  = "uint8",
+    .type  = VMSTATE_TYPE_UINT8,
+    .size  = sizeof(uint8_t),
+    .get = get_uint8,
+    .put = put_uint8,
+};
+
+/* 16 bit unsigned int */
+
+static void get_uint16(QEMUFile *f, VMStateInfo *info, void *pv)
+{
+    uint16_t *v = pv;
+    qemu_get_be16s(f, v);
+}
+
+static void put_uint16(QEMUFile *f, VMStateInfo *info, const void *pv)
+{
+    const uint16_t *v = pv;
+    qemu_put_be16s(f, v);
+}
+
+VMStateInfo vmstate_info_uint16 = {
+    .name  = "uint16",
+    .type  = VMSTATE_TYPE_UINT16,
+    .size  = sizeof(uint16_t),
+    .get = get_uint16,
+    .put = put_uint16,
+};
+
+/* 32 bit unsigned int */
+
+static void get_uint32(QEMUFile *f, VMStateInfo *info, void *pv)
+{
+    uint32_t *v = pv;
+    qemu_get_be32s(f, v);
+}
+
+static void put_uint32(QEMUFile *f, VMStateInfo *info, const void *pv)
+{
+    const uint32_t *v = pv;
+    qemu_put_be32s(f, v);
+}
+
+VMStateInfo vmstate_info_uint32 = {
+    .name  = "uint32",
+    .type  = VMSTATE_TYPE_UINT32,
+    .size  = sizeof(uint32_t),
+    .get = get_uint32,
+    .put = put_uint32,
+};
+
+/* 64 bit unsigned int */
+
+static void get_uint64(QEMUFile *f, VMStateInfo *info, void *pv)
+{
+    uint64_t *v = pv;
+    qemu_get_be64s(f, v);
+}
+
+static void put_uint64(QEMUFile *f, VMStateInfo *info, const void *pv)
+{
+    const uint64_t *v = pv;
+    qemu_put_be64s(f, v);
+}
+
+VMStateInfo vmstate_info_uint64 = {
+    .name  = "uint64",
+    .type  = VMSTATE_TYPE_UINT64,
+    .size  = sizeof(uint64_t),
+    .get = get_uint64,
+    .put = put_uint64,
+};
+
+/* timers  */
+
+static void get_timer(QEMUFile *f, VMStateInfo *info, void *pv)
+{
+    QEMUTimer **v = pv;
+    qemu_get_timer(f, *v);
+}
+
+static void put_timer(QEMUFile *f, VMStateInfo *info, const void *pv)
+{
+    QEMUTimer **v = (void *)pv;
+    qemu_put_timer(f, *v);
+}
+
+VMStateInfo vmstate_info_timer = {
+    .name  = "timer",
+    .type  = VMSTATE_TYPE_TIMER,
+    .size  = sizeof(uint64_t),
+    .get = get_timer,
+    .put = put_timer,
+};
+
 typedef struct SaveStateEntry {
     char idstr[256];
     int instance_id;
@@ -618,11 +816,13 @@ typedef struct SaveStateEntry {
     SaveLiveStateHandler *save_live_state;
     SaveStateHandler *save_state;
     LoadStateHandler *load_state;
+    VMStateDescription *vmsd;
     void *opaque;
     struct SaveStateEntry *next;
 } SaveStateEntry;

 static SaveStateEntry *first_se;
+static int global_section_id;

 /* TODO: Individual devices generally have very little idea about the rest
    of the system, so instance_id should be removed/replaced.
@@ -637,7 +837,6 @@ int register_savevm_live(const char *idstr,
                          void *opaque)
 {
     SaveStateEntry *se, **pse;
-    static int global_section_id;

     se = qemu_malloc(sizeof(SaveStateEntry));
     pstrcpy(se->idstr, sizeof(se->idstr), idstr);
@@ -648,6 +847,7 @@ int register_savevm_live(const char *idstr,
     se->save_state = save_state;
     se->load_state = load_state;
     se->opaque = opaque;
+    se->vmsd = NULL;
     se->next = NULL;

     /* add at the end of list */
@@ -690,6 +890,88 @@ void unregister_savevm(const char *idstr, void *opaque)
     }
 }

+int vmstate_register(int instance_id, VMStateDescription *vmsd,
+                     void *opaque)
+{
+    SaveStateEntry *se, **pse;
+
+    se = qemu_malloc(sizeof(SaveStateEntry));
+    pstrcpy(se->idstr, sizeof(se->idstr), vmsd->name);
+    se->instance_id = (instance_id == -1) ? 0 : instance_id;
+    se->version_id = vmsd->version_id;
+    se->section_id = global_section_id++;
+    se->save_live_state = NULL;
+    se->save_state = NULL;
+    se->load_state = NULL;
+    se->opaque = opaque;
+    se->vmsd = vmsd;
+    se->next = NULL;
+
+    /* add at the end of list */
+    pse = &first_se;
+    while (*pse != NULL) {
+        if (instance_id == -1
+                && strcmp(se->idstr, (*pse)->idstr) == 0
+                && se->instance_id <= (*pse)->instance_id)
+            se->instance_id = (*pse)->instance_id + 1;
+        pse = &(*pse)->next;
+    }
+    *pse = se;
+    return 0;
+}
+
+void vmstate_unregister(const char *idstr,  void *opaque)
+{
+    SaveStateEntry **pse;
+
+    pse = &first_se;
+    while (*pse != NULL) {
+        if (strcmp((*pse)->idstr, idstr) == 0 && (*pse)->opaque == opaque) {
+            SaveStateEntry *next = (*pse)->next;
+            qemu_free(*pse);
+            *pse = next;
+            continue;
+        }
+        pse = &(*pse)->next;
+    }
+}
+
+static void vmstate_save(QEMUFile *f, VMStateDescription *vmsd, void *base)
+{
+    VMStateField *field = vmsd->fields;
+    int i;
+
+    while(field->name) {
+        for (i = 0; i < field->num; i++) {
+            field->info->put(f, field->info, base + field->offset +
+                             field->info->size * i);
+        }
+        field++;
+    }
+}
+
+static int vmstate_load(QEMUFile *f, VMStateDescription *vmsd, void *base,
+                        int version_id)
+{
+    VMStateField *field = vmsd->fields;
+    int i;
+
+    if (version_id > vmsd->version_id ||
+        version_id < vmsd->minimum_version_id)
+        return -EINVAL;
+
+    while(field->name) {
+        for (i = 0; i < field->num; i++) {
+            if (field->version_id <= version_id) {
+                field->info->get(f, field->info, base + field->offset +
+                                 field->info->size * i);
+            }
+        }
+        field++;
+    }
+    return 0;
+}
+
 #define QEMU_VM_FILE_MAGIC           0x5145564d
 #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
 #define QEMU_VM_FILE_VERSION         0x00000003
@@ -777,7 +1059,7 @@ int qemu_savevm_state_complete(QEMUFile *f)
     for(se = first_se; se != NULL; se = se->next) {
         int len;

-	if (se->save_state == NULL)
+	if (se->save_state == NULL && se->vmsd == NULL)
 	    continue;

         /* Section type */
@@ -792,7 +1074,10 @@ int qemu_savevm_state_complete(QEMUFile *f)
         qemu_put_be32(f, se->instance_id);
         qemu_put_be32(f, se->version_id);

-        se->save_state(f, se->opaque);
+        if (se->vmsd != NULL)
+            vmstate_save(f, se->vmsd, se->opaque);
+        else
+            se->save_state(f, se->opaque);
     }

     qemu_put_byte(f, QEMU_VM_EOF);
@@ -878,7 +1163,11 @@ static int qemu_loadvm_state_v2(QEMUFile *f)
             fprintf(stderr, "qemu: warning: instance 0x%x of device '%s' not present in current VM\n",
                     instance_id, idstr);
         } else {
-            ret = se->load_state(f, se->opaque, version_id);
+            if (se->vmsd) {
+                ret = vmstate_load(f, se->vmsd, se->opaque, version_id);
+            } else {
+                ret = se->load_state(f, se->opaque, version_id);
+            }
             if (ret < 0) {
                 fprintf(stderr, "qemu: warning: error while loading state for instance 0x%x of device '%s'\n",
                         instance_id, idstr);
@@ -955,7 +1244,11 @@ int qemu_loadvm_state(QEMUFile *f)
             le->next = first_le;
             first_le = le;

-            ret = le->se->load_state(f, le->se->opaque, le->version_id);
+            if (le->se->vmsd) {
+                ret = vmstate_load(f, le->se->vmsd, le->se->opaque, le->version_id);
+            } else {
+                ret = le->se->load_state(f, le->se->opaque, le->version_id);
+            }
             if (ret < 0) {
                 fprintf(stderr, "qemu: warning: error while loading state for instance 0x%x of device '%s'\n",
                         instance_id, idstr);
@@ -974,7 +1267,11 @@ int qemu_loadvm_state(QEMUFile *f)
                 goto out;
             }

-            ret = le->se->load_state(f, le->se->opaque, le->version_id);
+            if (le->se->vmsd) {
+                ret = vmstate_load(f, le->se->vmsd, le->se->opaque, le->version_id);
+            } else {
+                ret = le->se->load_state(f, le->se->opaque, le->version_id);
+            }
             if (ret != 0) {
                 fprintf(stderr, "qemu: warning: error while loading state section '%d'\n",
                         section_id);
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 4/5] New VMstate save/load infrastructure

[Blue Swirl]

On Tue, Aug 18, 2009 at 4:34 PM, Juan Quintela<quintela@redhat.com> wrote:
> This patch introduces VMState infrastructure, to convert the save/load
> functions of devices to a table approach.  This new approach has the
> advantages:
> - it is type-safe
> - you can't have load/save functions out of sync
> - will allows us to have new interesting commands, like dump <device>, that
>  shows all its internal state.
> - Just now, the only added type is arrays, but we can add structures.

I think all the structures should be 'const'. Also VMStateField and
VMStateDescription should be private to savevm.c.

[Qemu-devel] Re: [PATCH 4/5] New VMstate save/load infrastructure

[Juan Quintela]

Reply-to: quintela@redhat.com
Blue Swirl <blauwirbel@gmail.com> wrote:
> On Tue, Aug 18, 2009 at 4:34 PM, Juan Quintela<quintela@redhat.com> wrote:
>> This patch introduces VMState infrastructure, to convert the save/load
>> functions of devices to a table approach.  This new approach has the
>> advantages:
>> - it is type-safe
>> - you can't have load/save functions out of sync
>> - will allows us to have new interesting commands, like dump <device>, that
>>  shows all its internal state.
>> - Just now, the only added type is arrays, but we can add structures.
>
> I think all the structures should be 'const'.

This is doable, and will do.

> Also VMStateField and
> VMStateDescription should be private to savevm.c.

They can, as we are trying to create them statically, not
programatically.

Later, Juan.

Re: [Qemu-devel] [PATCH 4/5] New VMstate save/load infrastructure

[Gerd Hoffmann]

> +enum VMStateType {
> +    VMSTATE_TYPE_UNSPEC = 0,
> +    VMSTATE_TYPE_INT8,
> +    VMSTATE_TYPE_INT16,
> +    VMSTATE_TYPE_INT32,
> +    VMSTATE_TYPE_INT64,
> +    VMSTATE_TYPE_UINT8,
> +    VMSTATE_TYPE_UINT16,
> +    VMSTATE_TYPE_UINT32,
> +    VMSTATE_TYPE_UINT64,
> +    VMSTATE_TYPE_TIMER,
> +};
> +
> +typedef struct VMStateInfo VMStateInfo;
> +
> +struct VMStateInfo {
> +    const char *name;
> +    size_t size;
> +    enum VMStateType type;
> +    void (*get)(QEMUFile *f, VMStateInfo *info, void *pv);
> +    void (*put)(QEMUFile *f, VMStateInfo *info, const void *pv);
> +};
> +
> +typedef struct {
> +    const char *name;
> +    size_t num;
> +    size_t offset;
> +    VMStateInfo *info;
> +    int version_id;
> +} VMStateField;

Hmm, I'm not sure VMStateInfo is that useful here.  All the get/put 
callbacks are simple two-liners, and it is unlikely that the number of 
types ever grows.

I think I would stick "enum VMStateType" directly into VMStateField and 
kill one level of indirection.

> +
> +typedef struct {
> +    const char *name;
> +    int version_id;
> +    int minimum_version_id;
> +    VMStateField *fields;
> +} VMStateDescription;

Add compat_load() callback for version_id < minimum_version_id here?

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 4/5] New VMstate save/load infrastructure

[Juan Quintela]

Reply-to: quintela@redhat.com
Gerd Hoffmann <kraxel@redhat.com> wrote:
>> +enum VMStateType {
>> +    VMSTATE_TYPE_UNSPEC = 0,
>> +    VMSTATE_TYPE_INT8,
>> +    VMSTATE_TYPE_INT16,
>> +    VMSTATE_TYPE_INT32,
>> +    VMSTATE_TYPE_INT64,
>> +    VMSTATE_TYPE_UINT8,
>> +    VMSTATE_TYPE_UINT16,
>> +    VMSTATE_TYPE_UINT32,
>> +    VMSTATE_TYPE_UINT64,
>> +    VMSTATE_TYPE_TIMER,
>> +};
>> +
>> +typedef struct VMStateInfo VMStateInfo;
>> +
>> +struct VMStateInfo {
>> +    const char *name;
>> +    size_t size;
>> +    enum VMStateType type;
>> +    void (*get)(QEMUFile *f, VMStateInfo *info, void *pv);
>> +    void (*put)(QEMUFile *f, VMStateInfo *info, const void *pv);
>> +};
>> +
>> +typedef struct {
>> +    const char *name;
>> +    size_t num;
>> +    size_t offset;
>> +    VMStateInfo *info;
>> +    int version_id;
>> +} VMStateField;
>
> Hmm, I'm not sure VMStateInfo is that useful here.  All the get/put
> callbacks are simple two-liners, and it is unlikely that the number of
> types ever grows.

Thinking about this.  VMStateType is not needed at all (see that it is
not used in the patch).  But Ilike the VMStateInfo struct approach.
This is the easier way of dealing with structs.  For instance
pci_device_save().  My idea was to just create a new VMStateInfo for it,
and then everything that needs to call pci_device_save() just add a
field like:

VMSTATE_FILED(dev, ThisPCIDevice, verion_id, pci_vmstate_info, PCIDevice)

and here, we go.  The reaso that I don't need VMStateType anymore is
that now load/save are done:

+static void vmstate_save(QEMUFile *f, VMStateDescription *vmsd, void *base)
+{
+    VMStateField *field = vmsd->fields;
+    int i;
+
+    while(field->name) {
+        for (i = 0; i < field->num; i++) {
+            field->info->put(f, field->info, base + field->offset +
+                             field->info->size * i);
+        }
+        field++;
+    }
+}

No need to have a type if we have the propper info struct.  As this
struct is going to grow (at least 2 new pointers for printing into
monitor/read value from monitor), I think it is a good idea to maintain
the sharing.

Later, Juan.

> I think I would stick "enum VMStateType" directly into VMStateField
> and kill one level of indirection.
>
>> +
>> +typedef struct {
>> +    const char *name;
>> +    int version_id;
>> +    int minimum_version_id;
>> +    VMStateField *fields;
>> +} VMStateDescription;
>
> Add compat_load() callback for version_id < minimum_version_id here?

I am not sure where/what to add.

I liked your approach of having the new version clean, and let the old
code to deal with the mess that used to be here.  Will take a look at
implementing soemething like that.

Thanks for the review, Juan.

[Qemu-devel] Re: [PATCH 4/5] New VMstate save/load infrastructure

[Gerd Hoffmann]

   Hi,

>> Hmm, I'm not sure VMStateInfo is that useful here.  All the get/put
>> callbacks are simple two-liners, and it is unlikely that the number of
>> types ever grows.
>
> Thinking about this.  VMStateType is not needed at all (see that it is
> not used in the patch).  But Ilike the VMStateInfo struct approach.
> This is the easier way of dealing with structs.  For instance
> pci_device_save().  My idea was to just create a new VMStateInfo for it,
> and then everything that needs to call pci_device_save() just add a
> field like:
>
> VMSTATE_FILED(dev, ThisPCIDevice, verion_id, pci_vmstate_info, PCIDevice)

Hmm.  I had a completely different approach in mind:

In pci.c:

vmstatefield pci_state[] = {
    VMSTATE_FIELD(...),
    [ ... ]
    VMSTATE_FIELD_END_OF_LIST()
}

In a pci driver:

vmstatefield device_state[] = {
    VMSTATE_INCLUDE(&pci_state, ...),
    VMSTATE_FIELD(...),
    [ ... ]
    VMSTATE_FIELD_END_OF_LIST()
}

Advantage: you don't have to write new code for each struct you want to 
save away state information from.

> +static void vmstate_save(QEMUFile *f, VMStateDescription *vmsd, void *base)
> +{
> +    VMStateField *field = vmsd->fields;
> +    int i;
> +
> +    while(field->name) {
> +        for (i = 0; i<  field->num; i++) {
> +            field->info->put(f, field->info, base + field->offset +
> +                             field->info->size * i);
> +        }

The other way would be dropping info and have
"switch(field->type) { ... }" here.

You indeed don't need both info and type.

Properties need the additional type field for typechecking in 
qemu_prop_set_$type() which doesn't use the print()/parse() callbacks.

cheers,
   Gerd

[Qemu-devel] [PATCH 5/5] Port apic to new VMState design

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/apic.c |   96 +++++++++++++++++-------------------------------------------
 1 files changed, 27 insertions(+), 69 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index 1927811..2c1dbf4 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -864,75 +864,33 @@ static void apic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
     }
 }

-static void apic_save(QEMUFile *f, void *opaque)
-{
-    APICState *s = opaque;
-    int i;
-
-    qemu_put_be32s(f, &s->apicbase);
-    qemu_put_8s(f, &s->id);
-    qemu_put_8s(f, &s->arb_id);
-    qemu_put_8s(f, &s->tpr);
-    qemu_put_be32s(f, &s->spurious_vec);
-    qemu_put_8s(f, &s->log_dest);
-    qemu_put_8s(f, &s->dest_mode);
-    for (i = 0; i < 8; i++) {
-        qemu_put_be32s(f, &s->isr[i]);
-        qemu_put_be32s(f, &s->tmr[i]);
-        qemu_put_be32s(f, &s->irr[i]);
-    }
-    for (i = 0; i < APIC_LVT_NB; i++) {
-        qemu_put_be32s(f, &s->lvt[i]);
+static VMStateDescription vmstate_apic = {
+    .name = "apic",
+    .version_id = 3,
+    .minimum_version_id = 3,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINT32(apicbase, APICState, 0),
+        VMSTATE_UINT8(id, APICState, 0),
+        VMSTATE_UINT8(arb_id, APICState, 0),
+        VMSTATE_UINT8(tpr, APICState, 0),
+        VMSTATE_UINT32(spurious_vec, APICState, 0),
+        VMSTATE_UINT8(log_dest, APICState, 0),
+        VMSTATE_UINT8(dest_mode, APICState, 0),
+        VMSTATE_UINT32_ARRAY(isr, APICState, 0, 8),
+        VMSTATE_UINT32_ARRAY(tmr, APICState, 0, 8),
+        VMSTATE_UINT32_ARRAY(irr, APICState, 0, 8),
+        VMSTATE_UINT32_ARRAY(lvt, APICState, 0, APIC_LVT_NB),
+        VMSTATE_UINT32(esr, APICState, 0),
+        VMSTATE_UINT32_ARRAY(icr, APICState, 0, 2),
+        VMSTATE_UINT32(divide_conf, APICState, 0),
+        VMSTATE_INT32(count_shift, APICState, 0),
+        VMSTATE_UINT32(initial_count, APICState, 0),
+        VMSTATE_INT64(initial_count_load_time, APICState, 0),
+        VMSTATE_INT64(next_time, APICState, 0),
+        VMSTATE_TIMER(timer, APICState, 2),
+        VMSTATE_END_OF_LIST()
     }
-    qemu_put_be32s(f, &s->esr);
-    qemu_put_be32s(f, &s->icr[0]);
-    qemu_put_be32s(f, &s->icr[1]);
-    qemu_put_be32s(f, &s->divide_conf);
-    qemu_put_be32(f, s->count_shift);
-    qemu_put_be32s(f, &s->initial_count);
-    qemu_put_be64(f, s->initial_count_load_time);
-    qemu_put_be64(f, s->next_time);
-
-    qemu_put_timer(f, s->timer);
-}
-
-static int apic_load(QEMUFile *f, void *opaque, int version_id)
-{
-    APICState *s = opaque;
-    int i;
-
-    if (version_id > 2)
-        return -EINVAL;
-
-    /* XXX: what if the base changes? (registered memory regions) */
-    qemu_get_be32s(f, &s->apicbase);
-    qemu_get_8s(f, &s->id);
-    qemu_get_8s(f, &s->arb_id);
-    qemu_get_8s(f, &s->tpr);
-    qemu_get_be32s(f, &s->spurious_vec);
-    qemu_get_8s(f, &s->log_dest);
-    qemu_get_8s(f, &s->dest_mode);
-    for (i = 0; i < 8; i++) {
-        qemu_get_be32s(f, &s->isr[i]);
-        qemu_get_be32s(f, &s->tmr[i]);
-        qemu_get_be32s(f, &s->irr[i]);
-    }
-    for (i = 0; i < APIC_LVT_NB; i++) {
-        qemu_get_be32s(f, &s->lvt[i]);
-    }
-    qemu_get_be32s(f, &s->esr);
-    qemu_get_be32s(f, &s->icr[0]);
-    qemu_get_be32s(f, &s->icr[1]);
-    qemu_get_be32s(f, &s->divide_conf);
-    s->count_shift=qemu_get_be32(f);
-    qemu_get_be32s(f, &s->initial_count);
-    s->initial_count_load_time=qemu_get_be64(f);
-    s->next_time=qemu_get_be64(f);
-
-    if (version_id >= 2)
-        qemu_get_timer(f, s->timer);
-    return 0;
-}
+};

 static void apic_reset(void *opaque)
 {
@@ -996,7 +954,7 @@ int apic_init(CPUState *env)
     }
     s->timer = qemu_new_timer(vm_clock, apic_timer, s);

-    register_savevm("apic", s->idx, 2, apic_save, apic_load, s);
+    vmstate_register(s->idx, &vmstate_apic, s);
     qemu_register_reset(apic_reset, s);

     local_apics[s->idx] = s;
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 5/5] Port apic to new VMState design

[Reimar Döffinger]

Hello,
sorry for replying in the middle of the thread, I was to fast and
deleted the other mails already.
And just in case I mention I am new around here, so feel free to ignore
me if you feel I am completely wrong.
One thing I don't like too much about it is that you can't really handle
"calculated" fields. It seems qemu does not use this (much/yet?), but
it seems good design to me that when a device emulation can handle
multiple devices, you store in one place the device name but to avoid
switch...case in lots of places you also have capability flags stored
somewhere.
Saving both seems a bit like a bad design: the value of one implies the
exact value of the other, so it is at least pointless.
More importantly (though I do not know if qemu intends to care about
this) it might be able to hand-craft a saved vm that after loading then
violates some assumptions of the emulation code, possibly being
exploitable.
If nothing else, I'd at least add support for a "verify" function that
gets a "const state *" and can abort loading the VM in case someone
tries something evil (or can print some useful hint instead of having
qemu crash silently on the user, possibly at some later time).
And yes I see that today almost nothing (of what I saw) verifies
anything, but it feels wrong to me to code that into a API design.

Greetings,
Reimar Döffinger

[Qemu-devel] Re: [PATCH 5/5] Port apic to new VMState design

[Reimar Döffinger]

On Tue, Aug 18, 2009 at 04:41:42PM +0200, Juan Quintela wrote:
> Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
> > Hello,
> > sorry for replying in the middle of the thread, I was to fast and
> > deleted the other mails already.
> > And just in case I mention I am new around here, so feel free to ignore
> > me if you feel I am completely wrong.
> > One thing I don't like too much about it is that you can't really handle
> > "calculated" fields.
> 
> Calculated fields are not by definition part of the state :)
> The state are the other fields that are used to save the state.  I
> haven't yet seen calculade fields (but I haven't looked throughfully
> yet).  With the current design, basically you can only save things that
> are in one struct (the way it is stored is an offset against a base
> address).

I am not sure we 100% understand each other, so I maybe tell the
specific example.
I made a change to the eepro100 driver to fix dumping the network
statistics.
The main problem is, depending on which device you emulate, the size of
the statistics struct changes.
Since it looked ugly etc. I decided not to calculate the size of these
statistics each time but instead save it in the device state.
But instead of adding it to save_vm and load_vm (which also would change
the version) I just set that statistics size again according to which
device is emulated. This also assures that the emulated device and
the statistics size always fit together, even if someone fiddles with
the saved state.
The "problem" with your approach if I understand it right is that I
couldn't do that since the device never knows when it would have to
re-fill these fields.
Basically what I am asking is if you couldn't just add an optional
callback so some additional stuff can be done after the "basic" state
has been loaded - or if that isn't desired at least a callback that
allows verifying the loaded values and aborting execution.

> > Saving both seems a bit like a bad design: the value of one implies the
> > exact value of the other, so it is at least pointless.
> > More importantly (though I do not know if qemu intends to care about
> > this) it might be able to hand-craft a saved vm that after loading then
> > violates some assumptions of the emulation code, possibly being
> > exploitable.
> 
> It is already that way.  This design don't change anything.  And I am
> not sure how to fix it.  We don't have a "is this value safe for this
> field", around yet.  It is possible to add some support for it, but I
> would like to 1st have an use case.

Well, I meant nowadays it is just possible to add a check in load_vm and
fix any values that are off. While it is quite a bit of work there is
nothing in the API stopping you from doing it, you even can return
-EINVAL and hopefully the core will print some somewhat useful message.

> > If nothing else, I'd at least add support for a "verify" function that
> > gets a "const state *" and can abort loading the VM in case someone
> > tries something evil (or can print some useful hint instead of having
> > qemu crash silently on the user, possibly at some later time).
> 
> This is as different problem that is not solved in qemu either.  I agree
> that it would be nice to have such a function, but I am not sure that I
> know how to do it.  and what is worse.  if you can modify the image, you
> can always change anything in the middle of the RAM.  I don't really see
> too much point trying to get a verify function for devices, when we
> can't have it for RAM.

That is completely different from what I meant.
Changing the RAM compromises the VM and only the VM, an exploit in a
device emulation might allow to compromise the _host_. Is it now clearer
what I meant?

Greetings,
Reimar Döffinger

[Qemu-devel] Re: [PATCH 5/5] Port apic to new VMState design

[Juan Quintela]

Reply-to: quintela@redhat.com
Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
> On Tue, Aug 18, 2009 at 04:41:42PM +0200, Juan Quintela wrote:
>> Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
>> > Hello,
>> > sorry for replying in the middle of the thread, I was to fast and
>> > deleted the other mails already.
>> > And just in case I mention I am new around here, so feel free to ignore
>> > me if you feel I am completely wrong.
>> > One thing I don't like too much about it is that you can't really handle
>> > "calculated" fields.
>> 
>> Calculated fields are not by definition part of the state :)
>> The state are the other fields that are used to save the state.  I
>> haven't yet seen calculade fields (but I haven't looked throughfully
>> yet).  With the current design, basically you can only save things that
>> are in one struct (the way it is stored is an offset against a base
>> address).
>
> I am not sure we 100% understand each other, so I maybe tell the
> specific example.
> I made a change to the eepro100 driver to fix dumping the network
> statistics.
> The main problem is, depending on which device you emulate, the size of
> the statistics struct changes.
> Since it looked ugly etc. I decided not to calculate the size of these
> statistics each time but instead save it in the device state.
> But instead of adding it to save_vm and load_vm (which also would change
> the version) I just set that statistics size again according to which
> device is emulated. This also assures that the emulated device and
> the statistics size always fit together, even if someone fiddles with
> the saved state.
> The "problem" with your approach if I understand it right is that I
> couldn't do that since the device never knows when it would have to
> re-fill these fields.
> Basically what I am asking is if you couldn't just add an optional
> callback so some additional stuff can be done after the "basic" state
> has been loaded - or if that isn't desired at least a callback that
> allows verifying the loaded values and aborting execution.

Ah, ok.  I guess we are going to need that callbacks, one before we load
state, and another one after we load it.

Are your changes on upstream hw/eepro100.c?  I can't see anything there
that can't be done in a table approach.

>> It is already that way.  This design don't change anything.  And I am
>> not sure how to fix it.  We don't have a "is this value safe for this
>> field", around yet.  It is possible to add some support for it, but I
>> would like to 1st have an use case.
>
> Well, I meant nowadays it is just possible to add a check in load_vm and
> fix any values that are off. While it is quite a bit of work there is
> nothing in the API stopping you from doing it, you even can return
> -EINVAL and hopefully the core will print some somewhat useful message.

I guess we are going to have an optional callback to be called
before/after loading the state.  You should be able to put your verify
there.

>> > If nothing else, I'd at least add support for a "verify" function that
>> > gets a "const state *" and can abort loading the VM in case someone
>> > tries something evil (or can print some useful hint instead of having
>> > qemu crash silently on the user, possibly at some later time).
>> 
>> This is as different problem that is not solved in qemu either.  I agree
>> that it would be nice to have such a function, but I am not sure that I
>> know how to do it.  and what is worse.  if you can modify the image, you
>> can always change anything in the middle of the RAM.  I don't really see
>> too much point trying to get a verify function for devices, when we
>> can't have it for RAM.
>
> That is completely different from what I meant.
> Changing the RAM compromises the VM and only the VM, an exploit in a
> device emulation might allow to compromise the _host_. Is it now clearer
> what I meant?

yes, I see where you are meaning now.  But I guess that one is needed to
be solved, not only for migration.  Not sure what to do about this.

Later, Juan.

Re: [Qemu-devel] Re: [PATCH 5/5] Port apic to new VMState design

[Reimar Döffinger]

On Tue, Aug 18, 2009 at 05:38:57PM +0200, Juan Quintela wrote:
> Are your changes on upstream hw/eepro100.c?  I can't see anything there
> that can't be done in a table approach.

No, so far noone got around to taking my patches apart (and that is
actually one I have not yet properly submitted, it is mangled into that
patch: http://article.gmane.org/gmane.comp.emulators.qemu/49853

> >> It is already that way.  This design don't change anything.  And I am
> >> not sure how to fix it.  We don't have a "is this value safe for this
> >> field", around yet.  It is possible to add some support for it, but I
> >> would like to 1st have an use case.
> >
> > Well, I meant nowadays it is just possible to add a check in load_vm and
> > fix any values that are off. While it is quite a bit of work there is
> > nothing in the API stopping you from doing it, you even can return
> > -EINVAL and hopefully the core will print some somewhat useful message.
> 
> I guess we are going to have an optional callback to be called
> before/after loading the state.  You should be able to put your verify
> there.

Maybe I'm silly, but what would the callback for before loading state be
good for?

> > That is completely different from what I meant.
> > Changing the RAM compromises the VM and only the VM, an exploit in a
> > device emulation might allow to compromise the _host_. Is it now clearer
> > what I meant?
> 
> yes, I see where you are meaning now.  But I guess that one is needed to
> be solved, not only for migration.  Not sure what to do about this.

I think it is mostly leg-work of finding the assumptions the emulations
do. That really should be left to maintainers where available IMO.
I'm just suggesting that it's better to design the API in a way that
doesn't further discourage fixing this :-).
If the patch is close to being accepted maybe I can help out by writing
such verification code for vmware_vga, there e.g. depth, bypp, wred,
wgreen and wblue must fit together as well as
width/height/new_width/new_height and fb_size (I think)
and width/height/bypp must be limit to ensure no integer overflows...

[Qemu-devel] Re: [PATCH 5/5] Port apic to new VMState design

[Juan Quintela]

Reply-to: quintela@redhat.com
Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
> On Tue, Aug 18, 2009 at 05:38:57PM +0200, Juan Quintela wrote:
>> Are your changes on upstream hw/eepro100.c?  I can't see anything there
>> that can't be done in a table approach.
>
> No, so far noone got around to taking my patches apart (and that is
> actually one I have not yet properly submitted, it is mangled into that
> patch: http://article.gmane.org/gmane.comp.emulators.qemu/49853
>
>> >> It is already that way.  This design don't change anything.  And I am
>> >> not sure how to fix it.  We don't have a "is this value safe for this
>> >> field", around yet.  It is possible to add some support for it, but I
>> >> would like to 1st have an use case.
>> >
>> > Well, I meant nowadays it is just possible to add a check in load_vm and
>> > fix any values that are off. While it is quite a bit of work there is
>> > nothing in the API stopping you from doing it, you even can return
>> > -EINVAL and hopefully the core will print some somewhat useful message.
>> 
>> I guess we are going to have an optional callback to be called
>> before/after loading the state.  You should be able to put your verify
>> there.
>
> Maybe I'm silly, but what would the callback for before loading state be
> good for?

qemu-kvm has in-kernel apic and pit (at least).  You just need to sync
state with the kernel after loading (the other way for saving).

>> > That is completely different from what I meant.
>> > Changing the RAM compromises the VM and only the VM, an exploit in a
>> > device emulation might allow to compromise the _host_. Is it now clearer
>> > what I meant?
>> 
>> yes, I see where you are meaning now.  But I guess that one is needed to
>> be solved, not only for migration.  Not sure what to do about this.
>
> I think it is mostly leg-work of finding the assumptions the emulations
> do. That really should be left to maintainers where available IMO.
> I'm just suggesting that it's better to design the API in a way that
> doesn't further discourage fixing this :-).
> If the patch is close to being accepted maybe I can help out by writing
> such verification code for vmware_vga, there e.g. depth, bypp, wred,
> wgreen and wblue must fit together as well as
> width/height/new_width/new_height and fb_size (I think)
> and width/height/bypp must be limit to ensure no integer overflows...

If you sent a patch, I will take a look.

Later, Juan.

Re: [Qemu-devel] Re: [PATCH 5/5] Port apic to new VMState design

[Gerd Hoffmann]

> Basically what I am asking is if you couldn't just add an optional
> callback so some additional stuff can be done after the "basic" state
> has been loaded - or if that isn't desired at least a callback that
> allows verifying the loaded values and aborting execution.

I think we are going to need post-processing callbacks anyway.  Several 
drivers have to do some actions after loading the state information. 
There you'll be able to set the stats size and also perform sanity 
checks on the loaded state.

> That is completely different from what I meant.
> Changing the RAM compromises the VM and only the VM, an exploit in a
> device emulation might allow to compromise the _host_. Is it now clearer
> what I meant?

When you are able modify the savevm state you already have access to the 
host ...

cheers,
   Gerd

Re: [Qemu-devel] Re: [PATCH 5/5] Port apic to new VMState design

[Reimar Döffinger]

On Wed, Aug 19, 2009 at 10:00:01AM +0200, Gerd Hoffmann wrote:
> > Basically what I am asking is if you couldn't just add an optional
> > callback so some additional stuff can be done after the "basic" state
> > has been loaded - or if that isn't desired at least a callback that
> > allows verifying the loaded values and aborting execution.
> 
> I think we are going to need post-processing callbacks anyway.  Several 
> drivers have to do some actions after loading the state information. 
> There you'll be able to set the stats size and also perform sanity 
> checks on the loaded state.

I don't care what they are called, I was just pointing out that the
patch was not implementing any of it.
And I by now say that e.g. vmware_vga needs it already in the current
implementation, it checks for example that the depth of the current
display and the one when it was saved match.

> > That is completely different from what I meant.
> > Changing the RAM compromises the VM and only the VM, an exploit in a
> > device emulation might allow to compromise the _host_. Is it now clearer
> > what I meant?
> 
> When you are able modify the savevm state you already have access to the 
> host ...

Huh? Being able to modify the savevm state is not the same as being able
to run arbitrary code on the host. At least ideally it shouldn't be.
Currently there is no way you could even consider running a savevm from
an untrusted source, but I think that is just because of qemu's current
implementation, not because it has to be.

Re: [Qemu-devel] Re: [PATCH 5/5] Port apic to new VMState design

[Reimar Döffinger]

On Wed, Aug 19, 2009 at 11:07:19AM +0200, Gerd Hoffmann wrote:
> >> When you are able modify the savevm state you already have access to the
> >> host ...
> >
> > Huh? Being able to modify the savevm state is not the same as being able
> > to run arbitrary code on the host.
> 
> Yes, in theory.  And in practice?  What is the point in allowing remote 
> write access to savevm state?

E.g. migration between entities that do not 100% trust each other?
Or debugging, a user does savevm and a developer can look at it and
debug after loadvm?

> > Currently there is no way you could even consider running a savevm from
> > an untrusted source, but I think that is just because of qemu's current
> > implementation, not because it has to be.
> 
> Getting that right is a pretty big job though ...

I said that already, but I don't think that's a valid excuse to not
consider that _for the design of a new API_. Unless you enjoy designing
a new API every few months...

Re: [Qemu-devel] [PATCH RFC 0/5] New VMState table based load/save infrastructure

[Gerd Hoffmann]

> What do I want to discuss:
> a- do we want to be able to load state form old versions?
>     I am not sure that we are going to get this right for complex devices,
>     and I don't completely see how we can have any kind of testing here.
>     There was already a discussion about this on the list.

Yes, we want.  And I think the right approach to do that is to simply 
use the old, existing code and switch based on version_id.  To stick 
with the apic example:  When loadvm finds a v2 (or older) apic section, 
go call apic_load.  When loadvm finds a v3 (or newer) apic section, call 
the new generic load code with the apic vmstatefield list.

> b- Is this the right approach?  What more do we want/need?
>     For instance, implementing struct save support, and calling
>     other "sub-descriptions" is not difficult, we just have to decide
>     if we want it.

Yes, we want.  PCI devices call a generic function to save pci state. 
We want a common pci vmstatefield list too and have some way to refer to 
them from the device tables.

> c- In the current approach, we have loops to send arrays,  I think that one
>     got already done better on new approarch.  But we don't have support
>     for ifs (see hw/ide.c
>         if (s->identify_set) {
>          qemu_get_buffer(f, (uint8_t *)s->identify_data, 512);
>      }
>     Do we want support for things like that?

No.  We want fixed-sized sections, bonus points for a 'size' field in 
the header.  Just save everything unconditionally.  The current 
save/load functions do way too much stuff conditionally.  Saving a few 
bytes simply isn't worth the complexity of getting that right.

> d- how aggresive should the new design be?  i.e. be able to be compatible with
>     old design is good, or can we start with a clean sheet and just remove the
>     gotchas of the previous design?

Handle compatibility by keeping the old load functions and start over 
with a clean sheet.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH RFC 0/5] New VMState table based load/save infrastructure

[Juan Quintela]

Reply-to: quintela@redhat.com
Gerd Hoffmann <kraxel@redhat.com> wrote:
>> What do I want to discuss:
>> a- do we want to be able to load state form old versions?
>>     I am not sure that we are going to get this right for complex devices,
>>     and I don't completely see how we can have any kind of testing here.
>>     There was already a discussion about this on the list.
>
> Yes, we want.  And I think the right approach to do that is to simply
> use the old, existing code and switch based on version_id.  To stick
> with the apic example:  When loadvm finds a v2 (or older) apic
> section, go call apic_load.  When loadvm finds a v3 (or newer) apic
> section, call the new generic load code with the apic vmstatefield
> list.

Liked it, will implement it in next round.

>> b- Is this the right approach?  What more do we want/need?
>>     For instance, implementing struct save support, and calling
>>     other "sub-descriptions" is not difficult, we just have to decide
>>     if we want it.
>
> Yes, we want.  PCI devices call a generic function to save pci
> state. We want a common pci vmstatefield list too and have some way to
> refer to them from the device tables.

That is the reason that I wanted to maintain the ->info field.  See the
other email.

>> c- In the current approach, we have loops to send arrays,  I think that one
>>     got already done better on new approarch.  But we don't have support
>>     for ifs (see hw/ide.c
>>         if (s->identify_set) {
>>          qemu_get_buffer(f, (uint8_t *)s->identify_data, 512);
>>      }
>>     Do we want support for things like that?
>
> No.  We want fixed-sized sections, bonus points for a 'size' field in
> the header.  Just save everything unconditionally.  The current
> save/load functions do way too much stuff conditionally.  Saving a few
> bytes simply isn't worth the complexity of getting that right.

Problem with this is when things can be NULL :p
>From msix.c

    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
        return;
    }

void msix_save(PCIDevice *dev, QEMUFile *f)
{
    unsigned n = dev->msix_entries_nr;

    qemu_put_buffer(f, dev->msix_table_page, n * MSIX_ENTRY_SIZE);
    qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
}

msix_table_page is not NULL only if QEMU_PCI_CAP_MSIX is present.
I think that optional fields are needed, or a better way of dealing with
things like this.

>> d- how aggresive should the new design be?  i.e. be able to be compatible with
>>     old design is good, or can we start with a clean sheet and just remove the
>>     gotchas of the previous design?
>
> Handle compatibility by keeping the old load functions and start over
> with a clean sheet.

Liked this idea, makes things way, way easier.  I was looking at
virtio_net_load() and had a head-ache.  With your approach, things
become really easy.

Thaks for the review, Juan

[Qemu-devel] Re: [PATCH RFC 0/5] New VMState table based load/save infrastructure

[Gerd Hoffmann]

> void msix_save(PCIDevice *dev, QEMUFile *f)
> {
>      unsigned n = dev->msix_entries_nr;
>
>      qemu_put_buffer(f, dev->msix_table_page, n * MSIX_ENTRY_SIZE);
>      qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
> }
>
> msix_table_page is not NULL only if QEMU_PCI_CAP_MSIX is present.
> I think that optional fields are needed, or a better way of dealing with
> things like this.

This effectively is a array with the length being determined at runtime 
(dev->msix_entries_nr).  Without msi-x length is zero ;)

But, yes, I think we will have to change driver code here and there to 
make it more vmstate friendly.

cheers,
   Gerd