[Qemu-devel] [RFC for-next 0/2] QOM VMStateDescription remix

[Qemu-devel] [RFC for-next 0/2] QOM VMStateDescription remix

[Andreas Färber]

Hello,

Based on a comment from mst, this mini-series proposes to change semantics of
VMStateDescription registration to be more similar to those of static properties.

Today, a device has one VMStateDescription, the last assignment to dc->vmsd wins.
This means that a device must take care to include state of its parent type.
To avoid dealing with individual fields, VMSTATE_STRUCT() and wrappers have
been used. Such fields often require access of the deprecated QOM parent field.

The proposal is that, e.g., TYPE_CPU assigns its own VMStateDescription and
derived types (e.g., TYPE_ALPHA_CPU) register a VMStateDescription with name
and versions to be used and only the fields specific to that type.
In this v1, versions of the parents' vmsd are ignored, so someone changing CPU's
DeviceClass::vmsd (as opposed to DeviceClass::vmsd->fields[0].vmsd) would need
to assure appropriate .field_exists tests or bump the version of derived types'
vmsd as if a field had been added there.

Only rudimentarily tested: I've run some machines that didn't crash on startup.

Regards,
Andreas

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Paolo Bonzini <pbonzini@redhat.com>

Andreas Färber (2):
  qdev: Construct VMStateDescription from type hierarchy
  cpu: Move VMSTATE_CPU() into TYPE_CPU VMStateDescription

 hw/core/qdev.c            | 102 +++++++++++++++++++++++++++++++++++++++++-----
 include/hw/qdev-core.h    |   1 +
 include/qom/cpu.h         |   4 --
 qom/cpu.c                 |  10 +++++
 stubs/vmstate.c           |   1 +
 target-alpha/machine.c    |   1 -
 target-openrisc/machine.c |   1 -
 7 files changed, 103 insertions(+), 17 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [RFC for-next 1/2] qdev: Construct VMStateDescription from type hierarchy

[Andreas Färber]

To avoid subclasses having to explicitly add their parent's state, such
as VMSTATE_PCI_DEVICE(), automatically assemble a VMStateDescription on
QOM realize. Use the most specific name and versions and prepend base
types' fields and subsections respectively.

TBD: Check if any types rely on subclasses overwriting non-NULL dc->vmsd.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/core/qdev.c         | 97 ++++++++++++++++++++++++++++++++++++++++++++------
 include/hw/qdev-core.h |  1 +
 2 files changed, 87 insertions(+), 11 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 9190a7e..0430fba 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -672,13 +672,86 @@ void qdev_property_add_static(DeviceState *dev, Property *prop,
     assert_no_error(local_err);
 }
 
+static void device_register_vmstate(DeviceState *dev, Error **errp)
+{
+    ObjectClass *oc;
+    DeviceClass *dc;
+    const VMStateDescription *vmsd = NULL;
+    VMStateField *field;
+    const VMStateSubsection *sect;
+    int fields = 0, subsections = 0, cur_fields, cur_subsections;
+
+    /* Determine how much there is to do */
+    oc = object_get_class(OBJECT(dev));
+    do {
+        dc = DEVICE_CLASS(oc);
+        if (dc->vmsd != NULL) {
+            if (vmsd == NULL) {
+                vmsd = dc->vmsd;
+            }
+            for (field = dc->vmsd->fields; field && field->name; field++) {
+                fields++;
+            }
+            for (sect = dc->vmsd->subsections; sect && sect->vmsd; sect++) {
+                subsections++;
+            }
+        }
+        oc = object_class_get_parent(oc);
+    } while (oc != object_class_by_name(TYPE_DEVICE));
+
+    if (fields == 0 && subsections == 0) {
+        return;
+    }
+
+    /* Adopt the first vmsd */
+    dev->vmsd = g_malloc(sizeof(VMStateDescription));
+    memcpy(dev->vmsd, vmsd, sizeof(VMStateDescription));
+    if (fields > 0) {
+        dev->vmsd->fields = g_new0(VMStateField, fields + 1);
+    }
+    if (subsections > 0) {
+        dev->vmsd->subsections = g_new0(VMStateSubsection, subsections + 1);
+    }
+
+    /* Copy fields and subsections from back to front */
+    oc = object_get_class(OBJECT(dev));
+    do {
+        dc = DEVICE_CLASS(oc);
+        if (dc->vmsd != NULL) {
+            cur_fields = 0;
+            for (field = dc->vmsd->fields; field && field->name; field++) {
+                cur_fields++;
+            }
+            memcpy(dev->vmsd->fields + (fields - cur_fields),
+                   dc->vmsd->fields,
+                   cur_fields * sizeof(VMStateField));
+            fields -= cur_fields;
+
+            cur_subsections = 0;
+            for (sect = dc->vmsd->subsections; sect && sect->vmsd; sect++) {
+                cur_subsections++;
+            }
+            memcpy((VMStateSubsection *)dev->vmsd->subsections
+                   + (subsections - cur_subsections),
+                   dc->vmsd->subsections,
+                   cur_subsections * sizeof(VMStateSubsection));
+            subsections -= cur_subsections;
+        }
+        oc = object_class_get_parent(oc);
+    } while (oc != object_class_by_name(TYPE_DEVICE));
+
+    vmstate_register_with_alias_id(dev, -1, dev->vmsd, dev,
+                                   dev->instance_id_alias,
+                                   dev->alias_required_for_version);
+}
+
 static bool device_get_realized(Object *obj, Error **err)
 {
     DeviceState *dev = DEVICE(obj);
     return dev->realized;
 }
 
-static void device_set_realized(Object *obj, bool value, Error **err)
+static void device_set_realized(Object *obj, bool value, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
@@ -699,17 +772,18 @@ static void device_set_realized(Object *obj, bool value, Error **err)
             dc->realize(dev, &local_err);
         }
 
-        if (qdev_get_vmsd(dev) && local_err == NULL) {
-            vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
-                                           dev->instance_id_alias,
-                                           dev->alias_required_for_version);
+        if (local_err == NULL) {
+            device_register_vmstate(dev, errp);
         }
         if (dev->hotplugged && local_err == NULL) {
             device_reset(dev);
         }
     } else if (!value && dev->realized) {
-        if (qdev_get_vmsd(dev)) {
-            vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
+        if (dev->vmsd) {
+            vmstate_unregister(dev, dev->vmsd, dev);
+            g_free(dev->vmsd->fields);
+            g_free((void *)dev->vmsd->subsections);
+            g_free(dev->vmsd);
         }
         if (dc->unrealize) {
             dc->unrealize(dev, &local_err);
@@ -717,7 +791,7 @@ static void device_set_realized(Object *obj, bool value, Error **err)
     }
 
     if (local_err != NULL) {
-        error_propagate(err, local_err);
+        error_propagate(errp, local_err);
         return;
     }
 
@@ -775,12 +849,13 @@ static void device_finalize(Object *obj)
 
 static void device_class_base_init(ObjectClass *class, void *data)
 {
-    DeviceClass *klass = DEVICE_CLASS(class);
+    DeviceClass *dc = DEVICE_CLASS(class);
 
-    /* We explicitly look up properties in the superclasses,
+    /* We explicitly look up properties and VMState in the superclasses,
      * so do not propagate them to the subclasses.
      */
-    klass->props = NULL;
+    dc->props = NULL;
+    dc->vmsd = NULL;
 }
 
 static void device_unparent(Object *obj)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 7fbffcb..2e46fcc 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -125,6 +125,7 @@ struct DeviceState {
     int num_child_bus;
     int instance_id_alias;
     int alias_required_for_version;
+    struct VMStateDescription *vmsd;
 };
 
 #define TYPE_BUS "bus"
-- 
1.8.1.4

[Qemu-devel] [RFC for-next 2/2] cpu: Move VMSTATE_CPU() into TYPE_CPU VMStateDescription

[Andreas Färber]

Assign DeviceClass::vmsd for common CPUState and drop VMSTATE_CPU() from
AlphaCPU and OpenRISCCPU.

Since we have two types of CPUs, those that register their state through
CPUClass::vmsd or CPU_SAVE_VERSION and those that register it through
DeviceClass::vmsd, we must keep device code from registering VMState
when only the base CPU class has DeviceClass::vmsd set.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/core/qdev.c            |  5 +++++
 include/qom/cpu.h         |  4 ----
 qom/cpu.c                 | 10 ++++++++++
 stubs/vmstate.c           |  1 +
 target-alpha/machine.c    |  1 -
 target-openrisc/machine.c |  1 -
 6 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 0430fba..6035f64 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -685,6 +685,11 @@ static void device_register_vmstate(DeviceState *dev, Error **errp)
     oc = object_get_class(OBJECT(dev));
     do {
         dc = DEVICE_CLASS(oc);
+        if (oc == object_class_by_name("cpu") &&
+            fields == 0 && subsections == 0) {
+            /* Old-style CPU with common state as separate VMSD */
+            break;
+        }
         if (dc->vmsd != NULL) {
             if (vmsd == NULL) {
                 vmsd = dc->vmsd;
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 0d6e95c..e0ad8b6 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -507,11 +507,7 @@ void qemu_init_vcpu(CPUState *cpu);
  */
 void cpu_single_step(CPUState *cpu, int enabled);
 
-#ifdef CONFIG_SOFTMMU
 extern const struct VMStateDescription vmstate_cpu_common;
-#else
-#define vmstate_cpu_common vmstate_dummy
-#endif
 
 #define VMSTATE_CPU() {                                                     \
     .name = "parent_obj",                                                   \
diff --git a/qom/cpu.c b/qom/cpu.c
index dbc9fb6..a3e47cb 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -24,6 +24,7 @@
 #include "qemu/notify.h"
 #include "qemu/log.h"
 #include "sysemu/sysemu.h"
+#include "migration/vmstate.h"
 
 typedef struct CPUExistsArgs {
     int64_t id;
@@ -250,6 +251,14 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
     return cpu->cpu_index;
 }
 
+static const VMStateDescription vmstate_cpu_device = {
+    .name = "CPU",
+    .fields = (VMStateField[]) {
+        VMSTATE_CPU(),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static void cpu_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -268,6 +277,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->gdb_write_register = cpu_common_gdb_write_register;
     dc->realize = cpu_common_realizefn;
     dc->no_user = 1;
+    dc->vmsd = &vmstate_cpu_device;
 }
 
 static const TypeInfo cpu_type_info = {
diff --git a/stubs/vmstate.c b/stubs/vmstate.c
index 778bc3f..22e2bd4 100644
--- a/stubs/vmstate.c
+++ b/stubs/vmstate.c
@@ -2,6 +2,7 @@
 #include "migration/vmstate.h"
 
 const VMStateDescription vmstate_dummy = {};
+const VMStateDescription vmstate_cpu_common = {};
 
 int vmstate_register_with_alias_id(DeviceState *dev,
                                    int instance_id,
diff --git a/target-alpha/machine.c b/target-alpha/machine.c
index 889f2fc..e3e75fb 100644
--- a/target-alpha/machine.c
+++ b/target-alpha/machine.c
@@ -77,7 +77,6 @@ static const VMStateDescription vmstate_env = {
 };
 
 static VMStateField vmstate_cpu_fields[] = {
-    VMSTATE_CPU(),
     VMSTATE_STRUCT(env, AlphaCPU, 1, vmstate_env, CPUAlphaState),
     VMSTATE_END_OF_LIST()
 };
diff --git a/target-openrisc/machine.c b/target-openrisc/machine.c
index 6f864fe..372b261 100644
--- a/target-openrisc/machine.c
+++ b/target-openrisc/machine.c
@@ -45,7 +45,6 @@ const VMStateDescription vmstate_openrisc_cpu = {
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields = (VMStateField[]) {
-        VMSTATE_CPU(),
         VMSTATE_STRUCT(env, OpenRISCCPU, 1, vmstate_env, CPUOpenRISCState),
         VMSTATE_END_OF_LIST()
     }
-- 
1.8.1.4

Re: [Qemu-devel] [RFC for-next 2/2] cpu: Move VMSTATE_CPU() into TYPE_CPU VMStateDescription

[Jia Liu]

On Mon, Jul 29, 2013 at 10:03 AM, Andreas Färber <afaerber@suse.de> wrote:
> Assign DeviceClass::vmsd for common CPUState and drop VMSTATE_CPU() from
> AlphaCPU and OpenRISCCPU.
>
> Since we have two types of CPUs, those that register their state through
> CPUClass::vmsd or CPU_SAVE_VERSION and those that register it through
> DeviceClass::vmsd, we must keep device code from registering VMState
> when only the base CPU class has DeviceClass::vmsd set.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  hw/core/qdev.c            |  5 +++++
>  include/qom/cpu.h         |  4 ----
>  qom/cpu.c                 | 10 ++++++++++
>  stubs/vmstate.c           |  1 +
>  target-alpha/machine.c    |  1 -
>  target-openrisc/machine.c |  1 -
>  6 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 0430fba..6035f64 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -685,6 +685,11 @@ static void device_register_vmstate(DeviceState *dev, Error **errp)
>      oc = object_get_class(OBJECT(dev));
>      do {
>          dc = DEVICE_CLASS(oc);
> +        if (oc == object_class_by_name("cpu") &&
> +            fields == 0 && subsections == 0) {
> +            /* Old-style CPU with common state as separate VMSD */
> +            break;
> +        }
>          if (dc->vmsd != NULL) {
>              if (vmsd == NULL) {
>                  vmsd = dc->vmsd;
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 0d6e95c..e0ad8b6 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -507,11 +507,7 @@ void qemu_init_vcpu(CPUState *cpu);
>   */
>  void cpu_single_step(CPUState *cpu, int enabled);
>
> -#ifdef CONFIG_SOFTMMU
>  extern const struct VMStateDescription vmstate_cpu_common;
> -#else
> -#define vmstate_cpu_common vmstate_dummy
> -#endif
>
>  #define VMSTATE_CPU() {                                                     \
>      .name = "parent_obj",                                                   \
> diff --git a/qom/cpu.c b/qom/cpu.c
> index dbc9fb6..a3e47cb 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -24,6 +24,7 @@
>  #include "qemu/notify.h"
>  #include "qemu/log.h"
>  #include "sysemu/sysemu.h"
> +#include "migration/vmstate.h"
>
>  typedef struct CPUExistsArgs {
>      int64_t id;
> @@ -250,6 +251,14 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
>      return cpu->cpu_index;
>  }
>
> +static const VMStateDescription vmstate_cpu_device = {
> +    .name = "CPU",
> +    .fields = (VMStateField[]) {
> +        VMSTATE_CPU(),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  static void cpu_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -268,6 +277,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
>      k->gdb_write_register = cpu_common_gdb_write_register;
>      dc->realize = cpu_common_realizefn;
>      dc->no_user = 1;
> +    dc->vmsd = &vmstate_cpu_device;
>  }
>
>  static const TypeInfo cpu_type_info = {
> diff --git a/stubs/vmstate.c b/stubs/vmstate.c
> index 778bc3f..22e2bd4 100644
> --- a/stubs/vmstate.c
> +++ b/stubs/vmstate.c
> @@ -2,6 +2,7 @@
>  #include "migration/vmstate.h"
>
>  const VMStateDescription vmstate_dummy = {};
> +const VMStateDescription vmstate_cpu_common = {};
>
>  int vmstate_register_with_alias_id(DeviceState *dev,
>                                     int instance_id,
> diff --git a/target-alpha/machine.c b/target-alpha/machine.c
> index 889f2fc..e3e75fb 100644
> --- a/target-alpha/machine.c
> +++ b/target-alpha/machine.c
> @@ -77,7 +77,6 @@ static const VMStateDescription vmstate_env = {
>  };
>
>  static VMStateField vmstate_cpu_fields[] = {
> -    VMSTATE_CPU(),
>      VMSTATE_STRUCT(env, AlphaCPU, 1, vmstate_env, CPUAlphaState),
>      VMSTATE_END_OF_LIST()
>  };
> diff --git a/target-openrisc/machine.c b/target-openrisc/machine.c
> index 6f864fe..372b261 100644
> --- a/target-openrisc/machine.c
> +++ b/target-openrisc/machine.c
> @@ -45,7 +45,6 @@ const VMStateDescription vmstate_openrisc_cpu = {
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,
>      .fields = (VMStateField[]) {
> -        VMSTATE_CPU(),
>          VMSTATE_STRUCT(env, OpenRISCCPU, 1, vmstate_env, CPUOpenRISCState),
>          VMSTATE_END_OF_LIST()
>      }

Tested-by: Jia Liu <proljc@gmail.com>


> --
> 1.8.1.4
>

Regards,
Jia

Re: [Qemu-devel] [RFC for-next 0/2] QOM VMStateDescription remix

[Andreas Färber]

Am 29.07.2013 04:03, schrieb Andreas Färber:
> Hello,
> 
> Based on a comment from mst, this mini-series proposes to change semantics of
> VMStateDescription registration to be more similar to those of static properties.
> 
> Today, a device has one VMStateDescription, the last assignment to dc->vmsd wins.
> This means that a device must take care to include state of its parent type.
> To avoid dealing with individual fields, VMSTATE_STRUCT() and wrappers have
> been used. Such fields often require access of the deprecated QOM parent field.
> 
> The proposal is that, e.g., TYPE_CPU assigns its own VMStateDescription and
> derived types (e.g., TYPE_ALPHA_CPU) register a VMStateDescription with name
> and versions to be used and only the fields specific to that type.
> In this v1, versions of the parents' vmsd are ignored, so someone changing CPU's
> DeviceClass::vmsd (as opposed to DeviceClass::vmsd->fields[0].vmsd) would need
> to assure appropriate .field_exists tests or bump the version of derived types'
> vmsd as if a field had been added there.

Ping?

Andreas

> 
> Only rudimentarily tested: I've run some machines that didn't crash on startup.
> 
> Regards,
> Andreas
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Anthony Liguori <anthony@codemonkey.ws>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> 
> Andreas Färber (2):
>   qdev: Construct VMStateDescription from type hierarchy
>   cpu: Move VMSTATE_CPU() into TYPE_CPU VMStateDescription
> 
>  hw/core/qdev.c            | 102 +++++++++++++++++++++++++++++++++++++++++-----
>  include/hw/qdev-core.h    |   1 +
>  include/qom/cpu.h         |   4 --
>  qom/cpu.c                 |  10 +++++
>  stubs/vmstate.c           |   1 +
>  target-alpha/machine.c    |   1 -
>  target-openrisc/machine.c |   1 -
>  7 files changed, 103 insertions(+), 17 deletions(-)
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC for-next 0/2] QOM VMStateDescription remix

[Michael S. Tsirkin]

On Mon, Jul 29, 2013 at 04:03:56AM +0200, Andreas Färber wrote:
> Hello,
> 
> Based on a comment from mst, this mini-series proposes to change semantics of
> VMStateDescription registration to be more similar to those of static properties.
> 
> Today, a device has one VMStateDescription, the last assignment to dc->vmsd wins.
> This means that a device must take care to include state of its parent type.
> To avoid dealing with individual fields, VMSTATE_STRUCT() and wrappers have
> been used. Such fields often require access of the deprecated QOM parent field.
> 
> The proposal is that, e.g., TYPE_CPU assigns its own VMStateDescription and
> derived types (e.g., TYPE_ALPHA_CPU) register a VMStateDescription with name
> and versions to be used and only the fields specific to that type.
> In this v1, versions of the parents' vmsd are ignored, so someone changing CPU's
> DeviceClass::vmsd (as opposed to DeviceClass::vmsd->fields[0].vmsd) would need
> to assure appropriate .field_exists tests or bump the version of derived types'
> vmsd as if a field had been added there.
> 
> Only rudimentarily tested: I've run some machines that didn't crash on startup.
> 
> Regards,
> Andreas
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Anthony Liguori <anthony@codemonkey.ws>
> Cc: Paolo Bonzini <pbonzini@redhat.com>

Overall okay
Acked-by: Michael S. Tsirkin <mst@redhat.com>

but obviously this needs much more testing, in particular
you need to test migration, not just check that it doesn't
crash :)

Also - are there devices that already set vmstate at several levels?
If yes this will change the wire protocol won't it?

> Andreas Färber (2):
>   qdev: Construct VMStateDescription from type hierarchy
>   cpu: Move VMSTATE_CPU() into TYPE_CPU VMStateDescription
> 
>  hw/core/qdev.c            | 102 +++++++++++++++++++++++++++++++++++++++++-----
>  include/hw/qdev-core.h    |   1 +
>  include/qom/cpu.h         |   4 --
>  qom/cpu.c                 |  10 +++++
>  stubs/vmstate.c           |   1 +
>  target-alpha/machine.c    |   1 -
>  target-openrisc/machine.c |   1 -
>  7 files changed, 103 insertions(+), 17 deletions(-)
> 
> -- 
> 1.8.1.4

Re: [Qemu-devel] [RFC for-next 0/2] QOM VMStateDescription remix

[Andreas Färber]

Peter,

Am 02.09.2013 13:41, schrieb Michael S. Tsirkin:
> On Mon, Jul 29, 2013 at 04:03:56AM +0200, Andreas Färber wrote:
>> Hello,
>>
>> Based on a comment from mst, this mini-series proposes to change semantics of
>> VMStateDescription registration to be more similar to those of static properties.
>>
>> Today, a device has one VMStateDescription, the last assignment to dc->vmsd wins.
>> This means that a device must take care to include state of its parent type.
>> To avoid dealing with individual fields, VMSTATE_STRUCT() and wrappers have
>> been used. Such fields often require access of the deprecated QOM parent field.
>>
>> The proposal is that, e.g., TYPE_CPU assigns its own VMStateDescription and
>> derived types (e.g., TYPE_ALPHA_CPU) register a VMStateDescription with name
>> and versions to be used and only the fields specific to that type.
>> In this v1, versions of the parents' vmsd are ignored, so someone changing CPU's
>> DeviceClass::vmsd (as opposed to DeviceClass::vmsd->fields[0].vmsd) would need
>> to assure appropriate .field_exists tests or bump the version of derived types'
>> vmsd as if a field had been added there.
>>
>> Only rudimentarily tested: I've run some machines that didn't crash on startup.
>>
>> Regards,
>> Andreas
>>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Anthony Liguori <anthony@codemonkey.ws>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> 
> Overall okay
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> but obviously this needs much more testing, in particular
> you need to test migration, not just check that it doesn't
> crash :)
> 
> Also - are there devices that already set vmstate at several levels?

Using my qom-test plus the following code:

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index bd8f6dd..b4a7638 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -708,6 +708,8 @@ static void device_register_vmstate(DeviceState
*dev, Error **errp)
         if (dc->vmsd != NULL) {
             if (vmsd == NULL) {
                 vmsd = dc->vmsd;
+            } else {
+                fprintf(stderr, "%s (%s)\n", object_class_get_name(oc),
vmsd->name);
             }
             for (field = dc->vmsd->fields; field && field->name; field++) {
                 fields++;

I have found a single such case: armv7m_nvic overrides arm_gic_common
with a completely different vmsd. How can we fix that? :)

Thanks,
Andreas

> If yes this will change the wire protocol won't it?
> 
>> Andreas Färber (2):
>>   qdev: Construct VMStateDescription from type hierarchy
>>   cpu: Move VMSTATE_CPU() into TYPE_CPU VMStateDescription
>>
>>  hw/core/qdev.c            | 102 +++++++++++++++++++++++++++++++++++++++++-----
>>  include/hw/qdev-core.h    |   1 +
>>  include/qom/cpu.h         |   4 --
>>  qom/cpu.c                 |  10 +++++
>>  stubs/vmstate.c           |   1 +
>>  target-alpha/machine.c    |   1 -
>>  target-openrisc/machine.c |   1 -
>>  7 files changed, 103 insertions(+), 17 deletions(-)
>>
>> -- 
>> 1.8.1.4

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC for-next 0/2] QOM VMStateDescription remix

[Peter Maydell]

On 7 February 2014 21:28, Andreas Färber <afaerber@suse.de> wrote:
> I have found a single such case: armv7m_nvic overrides arm_gic_common
> with a completely different vmsd. How can we fix that? :)

We can deduce from this that nobody's been using migration
with the Cortex-M3, since it would obviously be totally busted.
So the answer is "any way you like". The M profile code in
QEMU is not really maintained; mostly I just accept patches
that don't make things worse.

What the code is presumably attempting to do is add the
extra fields for the systick timer on top of the base state
for the interrupt controller. Personally I'd be tempted to
split systick out into its own little device. (It's only bundled
in with the NVIC because this code predates memory
regions and at the time you couldn't really do the system
register region any other way.)

thanks
-- PMM