[Qemu-devel] vmstate: struct (VMS_STRUCT) migration

[Qemu-devel] vmstate: struct (VMS_STRUCT) migration

[Alexey Kardashevskiy]

Hi!

VMStateDescription supports enclosed VMStateDescription's via .vmsd. This
is used in multiple places and VMStateDescription definitions look the same
way - name, version_id, minimum_version_id, etc.

QEMU handles first level VMStateDescription and enclosed VMStateDescription
slightly different - for the latter it ignores the version_id from the
source and always uses the version_id on the destination side so version
fields are useless (code is below).

Is that by design? Or a bug? I cannot see how we could fix it without
breaking backward compatibility but I am sure the community does know that :)

If this is by design, it probably makes sense to remove unused
version_id/minimum_version_id fields completely for vmsd's which we know
are enclosed (or embedded?).

What do I miss here? Thanks!



int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                       void *opaque, int version_id)
{
    VMStateField *field = vmsd->fields;
    int ret;

    if (version_id > vmsd->version_id) {
        return -EINVAL;
    }
    if  (version_id < vmsd->minimum_version_id) {
        if (vmsd->load_state_old &&
            version_id >= vmsd->minimum_version_id_old) {
            return vmsd->load_state_old(f, opaque, version_id);
        }
        return -EINVAL;
    }
    if (vmsd->pre_load) {
        int ret = vmsd->pre_load(opaque);
        if (ret) {
            return ret;
        }
    }
    while (field->name) {
        if ((field->field_exists &&
             field->field_exists(opaque, version_id)) ||
            (!field->field_exists &&
             field->version_id <= version_id)) {
            void *base_addr = vmstate_base_addr(opaque, field, true);
            int i, n_elems = vmstate_n_elems(opaque, field);
            int size = vmstate_size(opaque, field);

            for (i = 0; i < n_elems; i++) {
                void *addr = base_addr + size * i;

                if (field->flags & VMS_ARRAY_OF_POINTER) {
                    addr = *(void **)addr;
                }
                if (field->flags & VMS_STRUCT) {
                    ret = vmstate_load_state(f, field->vmsd, addr,
                                             field->vmsd->version_id);



-- 
Alexey

Re: [Qemu-devel] vmstate: struct (VMS_STRUCT) migration

[Alexey Kardashevskiy]

On 06/25/2014 04:36 PM, Alexey Kardashevskiy wrote:
> Hi!
> 
> VMStateDescription supports enclosed VMStateDescription's via .vmsd. This
> is used in multiple places and VMStateDescription definitions look the same
> way - name, version_id, minimum_version_id, etc.
> 
> QEMU handles first level VMStateDescription and enclosed VMStateDescription
> slightly different - for the latter it ignores the version_id from the
> source and always uses the version_id on the destination side so version
> fields are useless (code is below).
> 
> Is that by design? Or a bug? I cannot see how we could fix it without
> breaking backward compatibility but I am sure the community does know that :)
> 
> If this is by design, it probably makes sense to remove unused
> version_id/minimum_version_id fields completely for vmsd's which we know
> are enclosed (or embedded?).
> 
> What do I miss here? Thanks!

Anyone? :)


> 
> 
> 
> int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>                        void *opaque, int version_id)
> {
>     VMStateField *field = vmsd->fields;
>     int ret;
> 
>     if (version_id > vmsd->version_id) {
>         return -EINVAL;
>     }
>     if  (version_id < vmsd->minimum_version_id) {
>         if (vmsd->load_state_old &&
>             version_id >= vmsd->minimum_version_id_old) {
>             return vmsd->load_state_old(f, opaque, version_id);
>         }
>         return -EINVAL;
>     }
>     if (vmsd->pre_load) {
>         int ret = vmsd->pre_load(opaque);
>         if (ret) {
>             return ret;
>         }
>     }
>     while (field->name) {
>         if ((field->field_exists &&
>              field->field_exists(opaque, version_id)) ||
>             (!field->field_exists &&
>              field->version_id <= version_id)) {
>             void *base_addr = vmstate_base_addr(opaque, field, true);
>             int i, n_elems = vmstate_n_elems(opaque, field);
>             int size = vmstate_size(opaque, field);
> 
>             for (i = 0; i < n_elems; i++) {
>                 void *addr = base_addr + size * i;
> 
>                 if (field->flags & VMS_ARRAY_OF_POINTER) {
>                     addr = *(void **)addr;
>                 }
>                 if (field->flags & VMS_STRUCT) {
>                     ret = vmstate_load_state(f, field->vmsd, addr,
>                                              field->vmsd->version_id);
> 
> 
> 


-- 
Alexey

Re: [Qemu-devel] vmstate: struct (VMS_STRUCT) migration

[Peter Maydell]

On 2 July 2014 15:04, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> On 06/25/2014 04:36 PM, Alexey Kardashevskiy wrote:
>> Hi!
>>
>> VMStateDescription supports enclosed VMStateDescription's via .vmsd. This
>> is used in multiple places and VMStateDescription definitions look the same
>> way - name, version_id, minimum_version_id, etc.
>>
>> QEMU handles first level VMStateDescription and enclosed VMStateDescription
>> slightly different - for the latter it ignores the version_id from the
>> source and always uses the version_id on the destination side so version
>> fields are useless (code is below).
>>
>> Is that by design? Or a bug? I cannot see how we could fix it without
>> breaking backward compatibility but I am sure the community does know that :)
>>
>> If this is by design, it probably makes sense to remove unused
>> version_id/minimum_version_id fields completely for vmsd's which we know
>> are enclosed (or embedded?).
>>
>> What do I miss here? Thanks!
>
> Anyone? :)

Ccing Juan would probably help. I'm not sure the current behaviour
makes a great deal of sense...

thanks
-- PMM

Re: [Qemu-devel] vmstate: struct (VMS_STRUCT) migration

[Amit Shah]

On (Wed) 25 Jun 2014 [16:36:59], Alexey Kardashevskiy wrote:
> Hi!
> 
> VMStateDescription supports enclosed VMStateDescription's via .vmsd. This
> is used in multiple places and VMStateDescription definitions look the same
> way - name, version_id, minimum_version_id, etc.
> 
> QEMU handles first level VMStateDescription and enclosed VMStateDescription
> slightly different - for the latter it ignores the version_id from the
> source and always uses the version_id on the destination side so version
> fields are useless (code is below).
> 
> Is that by design? Or a bug? I cannot see how we could fix it without
> breaking backward compatibility but I am sure the community does know that :)
> 
> If this is by design, it probably makes sense to remove unused
> version_id/minimum_version_id fields completely for vmsd's which we know
> are enclosed (or embedded?).
> 
> What do I miss here? Thanks!

No, you're not missing anything.  It's indeed a bug that crept in the
original implementation.

We discussed this detail during the KVM Forum 2013; and since we
anyway do need a new on-wire format, the one that will fix all bugs
(TM), this will be taken care of too...

You can see the slides / videos from mine and Alex Graf's
presentations.

The recommendation, for now, is to bump up the version field of the
parent vmsd when updating a substruct.  But obviously, this can be
easily missed.  The static checker just merged in the qemu tree does
catch these types of errors.

In the meanwhile, what you propose does make sense: getting rid of the
version_id and minimum_version_id fields in substructs.

		Amit