Patchwork [RFC,1/4] add support for machine models to specify their migration format

login
register
mail settings
Submitter Paolo Bonzini
Date June 30, 2011, 3:46 p.m.
Message ID <1309448777-1447-2-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/102782/
State New
Headers show

Comments

Paolo Bonzini - June 30, 2011, 3:46 p.m.
We need to provide a new migration format, and not break migration
in old machine models.  So add a migration_format field to QEMUMachine.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-common.h  |    3 ---
 hw/boards.h   |    1 +
 qemu-common.h |    3 +++
 savevm.c      |    7 +++++--
 4 files changed, 9 insertions(+), 5 deletions(-)
Michael S. Tsirkin - June 30, 2011, 6:11 p.m.
On Thu, Jun 30, 2011 at 05:46:14PM +0200, Paolo Bonzini wrote:
> We need to provide a new migration format, and not break migration
> in old machine models.  So add a migration_format field to QEMUMachine.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Should not machine describe guest behaviour?
It seems that a flag to the migrate/save command to control format
would be better - this way the same machine can
migrate to different formats.

> ---
>  cpu-common.h  |    3 ---
>  hw/boards.h   |    1 +
>  qemu-common.h |    3 +++
>  savevm.c      |    7 +++++--
>  4 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/cpu-common.h b/cpu-common.h
> index b027e43..8c06dbb 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -26,9 +26,6 @@ enum device_endian {
>      DEVICE_LITTLE_ENDIAN,
>  };
>  
> -/* address in the RAM (different from a physical address) */
> -typedef unsigned long ram_addr_t;
> -
>  /* memory API */
>  
>  typedef void CPUWriteMemoryFunc(void *opaque, target_phys_addr_t addr, uint32_t value);
> diff --git a/hw/boards.h b/hw/boards.h
> index 716fd7b..560dbaf 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -19,6 +19,7 @@ typedef struct QEMUMachine {
>      QEMUMachineInitFunc *init;
>      int use_scsi;
>      int max_cpus;
> +    unsigned migration_format;
>      unsigned int no_serial:1,
>          no_parallel:1,
>          use_virtcon:1,
> diff --git a/qemu-common.h b/qemu-common.h
> index 109498d..550fe2c 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -119,6 +119,9 @@ static inline char *realpath(const char *path, char *resolved_path)
>  #define PRIo64 "I64o"
>  #endif
>  
> +/* address in the RAM (different from a physical address) */
> +typedef unsigned long ram_addr_t;
> +
>  /* FIXME: Remove NEED_CPU_H.  */
>  #ifndef NEED_CPU_H
>  
> diff --git a/savevm.c b/savevm.c
> index 8139bc7..74e6e99 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -72,6 +72,7 @@
>  #include "qemu-common.h"
>  #include "hw/hw.h"
>  #include "hw/qdev.h"
> +#include "hw/boards.h"
>  #include "net.h"
>  #include "monitor.h"
>  #include "sysemu.h"
> @@ -1474,7 +1475,7 @@ int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
>      }
>      
>      qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
> -    qemu_put_be32(f, QEMU_VM_FILE_VERSION);
> +    qemu_put_be32(f, current_machine->migration_format ?: QEMU_VM_FILE_VERSION);
>  
>      QTAILQ_FOREACH(se, &savevm_handlers, entry) {
>          int len;
> @@ -1747,8 +1748,10 @@ int qemu_loadvm_state(QEMUFile *f)
>          fprintf(stderr, "SaveVM v2 format is obsolete and don't work anymore\n");
>          return -ENOTSUP;
>      }
> -    if (v != QEMU_VM_FILE_VERSION)
> +    if (v != (current_machine->migration_format ?: QEMU_VM_FILE_VERSION)) {
> +        fprintf(stderr, "Mismatching SaveVM format v%d\n", v);
>          return -ENOTSUP;
> +    }
>  
>      while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
>          uint32_t instance_id, version_id, section_id;
> -- 
> 1.7.5.2
> 
>
Paolo Bonzini - July 1, 2011, 6:10 a.m.
On 06/30/2011 08:11 PM, Michael S. Tsirkin wrote:
> >  Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>
> Should not machine describe guest behaviour?
> It seems that a flag to the migrate/save command to control format
> would be better - this way the same machine can
> migrate to different formats.

I thought about it, but there is no savevm state variable (only 
migration state, but it's unused for snapshotting).  So it seemed too 
much work for an RFC series I wanted to push out fast.

Besides, you need to store a default somewhere, and the default must be 
the old format for versioned machine models; perhaps not from QEMU where 
new->old migration is a wild bet anyway, but for downstream it must. 
Then I think that you are suggesting a superset of this patch. I'm not 
even sure it is necessary to have it though: it is a correctness fix, it 
should be on for everyone.

Paolo
Anthony Liguori - July 29, 2011, 1:08 p.m.
On 06/30/2011 10:46 AM, Paolo Bonzini wrote:
> We need to provide a new migration format, and not break migration
> in old machine models.  So add a migration_format field to QEMUMachine.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   cpu-common.h  |    3 ---
>   hw/boards.h   |    1 +
>   qemu-common.h |    3 +++
>   savevm.c      |    7 +++++--
>   4 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/cpu-common.h b/cpu-common.h
> index b027e43..8c06dbb 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -26,9 +26,6 @@ enum device_endian {
>       DEVICE_LITTLE_ENDIAN,
>   };
>
> -/* address in the RAM (different from a physical address) */
> -typedef unsigned long ram_addr_t;
> -
>   /* memory API */
>
>   typedef void CPUWriteMemoryFunc(void *opaque, target_phys_addr_t addr, uint32_t value);
> diff --git a/hw/boards.h b/hw/boards.h
> index 716fd7b..560dbaf 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -19,6 +19,7 @@ typedef struct QEMUMachine {
>       QEMUMachineInitFunc *init;
>       int use_scsi;
>       int max_cpus;
> +    unsigned migration_format;
>       unsigned int no_serial:1,
>           no_parallel:1,
>           use_virtcon:1,
> diff --git a/qemu-common.h b/qemu-common.h
> index 109498d..550fe2c 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -119,6 +119,9 @@ static inline char *realpath(const char *path, char *resolved_path)
>   #define PRIo64 "I64o"
>   #endif
>
> +/* address in the RAM (different from a physical address) */
> +typedef unsigned long ram_addr_t;
> +
>   /* FIXME: Remove NEED_CPU_H.  */
>   #ifndef NEED_CPU_H
>
> diff --git a/savevm.c b/savevm.c
> index 8139bc7..74e6e99 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -72,6 +72,7 @@
>   #include "qemu-common.h"
>   #include "hw/hw.h"
>   #include "hw/qdev.h"
> +#include "hw/boards.h"
>   #include "net.h"
>   #include "monitor.h"
>   #include "sysemu.h"
> @@ -1474,7 +1475,7 @@ int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
>       }
>
>       qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
> -    qemu_put_be32(f, QEMU_VM_FILE_VERSION);
> +    qemu_put_be32(f, current_machine->migration_format ?: QEMU_VM_FILE_VERSION);

Please avoid this gcc extension as it's relatively obscure.  But in 
addition, why would use you 0 as the new format instead of 
QEMU_VM_FILE_VERSION + 1?

Regards,

Anthony Liguori

>       QTAILQ_FOREACH(se,&savevm_handlers, entry) {
>           int len;
> @@ -1747,8 +1748,10 @@ int qemu_loadvm_state(QEMUFile *f)
>           fprintf(stderr, "SaveVM v2 format is obsolete and don't work anymore\n");
>           return -ENOTSUP;
>       }
> -    if (v != QEMU_VM_FILE_VERSION)
> +    if (v != (current_machine->migration_format ?: QEMU_VM_FILE_VERSION)) {
> +        fprintf(stderr, "Mismatching SaveVM format v%d\n", v);
>           return -ENOTSUP;
> +    }
>
>       while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
>           uint32_t instance_id, version_id, section_id;
Paolo Bonzini - July 29, 2011, 2:35 p.m.
On 07/29/2011 03:08 PM, Anthony Liguori wrote:
>
> Please avoid this gcc extension as it's relatively obscure.  But in
> addition, why would use you 0 as the new format instead of
> QEMU_VM_FILE_VERSION + 1?

0 is the default.  If a machine doesn't specify a format, it gets the 
newest one automatically.

Paolo

Patch

diff --git a/cpu-common.h b/cpu-common.h
index b027e43..8c06dbb 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -26,9 +26,6 @@  enum device_endian {
     DEVICE_LITTLE_ENDIAN,
 };
 
-/* address in the RAM (different from a physical address) */
-typedef unsigned long ram_addr_t;
-
 /* memory API */
 
 typedef void CPUWriteMemoryFunc(void *opaque, target_phys_addr_t addr, uint32_t value);
diff --git a/hw/boards.h b/hw/boards.h
index 716fd7b..560dbaf 100644
--- a/hw/boards.h
+++ b/hw/boards.h
@@ -19,6 +19,7 @@  typedef struct QEMUMachine {
     QEMUMachineInitFunc *init;
     int use_scsi;
     int max_cpus;
+    unsigned migration_format;
     unsigned int no_serial:1,
         no_parallel:1,
         use_virtcon:1,
diff --git a/qemu-common.h b/qemu-common.h
index 109498d..550fe2c 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -119,6 +119,9 @@  static inline char *realpath(const char *path, char *resolved_path)
 #define PRIo64 "I64o"
 #endif
 
+/* address in the RAM (different from a physical address) */
+typedef unsigned long ram_addr_t;
+
 /* FIXME: Remove NEED_CPU_H.  */
 #ifndef NEED_CPU_H
 
diff --git a/savevm.c b/savevm.c
index 8139bc7..74e6e99 100644
--- a/savevm.c
+++ b/savevm.c
@@ -72,6 +72,7 @@ 
 #include "qemu-common.h"
 #include "hw/hw.h"
 #include "hw/qdev.h"
+#include "hw/boards.h"
 #include "net.h"
 #include "monitor.h"
 #include "sysemu.h"
@@ -1474,7 +1475,7 @@  int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
     }
     
     qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
-    qemu_put_be32(f, QEMU_VM_FILE_VERSION);
+    qemu_put_be32(f, current_machine->migration_format ?: QEMU_VM_FILE_VERSION);
 
     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
         int len;
@@ -1747,8 +1748,10 @@  int qemu_loadvm_state(QEMUFile *f)
         fprintf(stderr, "SaveVM v2 format is obsolete and don't work anymore\n");
         return -ENOTSUP;
     }
-    if (v != QEMU_VM_FILE_VERSION)
+    if (v != (current_machine->migration_format ?: QEMU_VM_FILE_VERSION)) {
+        fprintf(stderr, "Mismatching SaveVM format v%d\n", v);
         return -ENOTSUP;
+    }
 
     while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
         uint32_t instance_id, version_id, section_id;