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 <address@hidden>
>> Cc: Juan Quintela <address@hidden>
>> Cc: Anthony Liguori <address@hidden>
>> Cc: Paolo Bonzini <address@hidden>
> 
> Overall okay
> Acked-by: Michael S. Tsirkin <address@hidden>
> 
> 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