On 10/17/2016 12:16 PM, Dr. David Alan Gilbert wrote:
> * Jianjun Duan (du...@linux.vnet.ibm.com) wrote:
>>
>>
>> On 10/17/2016 11:52 AM, Dr. David Alan Gilbert wrote:
>>> * Jianjun Duan (du...@linux.vnet.ibm.com) wrote:
>>>>
>>>>
>>>> On 10/16/2016 08:31 PM, David Gibson wrote:
>>>>> On Tue, Oct 11, 2016 at 06:18:30PM +0100, Dr. David Alan Gilbert (git)
>>>>> wrote:
>>>>>> From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
>>>>>>
>>>>>> VMSTATE_WITH_TMP is for handling structures where some calculation
>>>>>> or rearrangement of the data needs to be performed before the data
>>>>>> hits the wire.
>>>>>> For example, where the value on the wire is an offset from a
>>>>>> non-migrated base, but the data in the structure is the actual pointer.
>>>>>>
>>>>>> To use it, a temporary type is created and a vmsd used on that type.
>>>>>> The first element of the type must be 'parent' a pointer back to the
>>>>>> type of the main structure. VMSTATE_WITH_TMP takes care of allocating
>>>>>> and freeing the temporary before running the child vmsd.
>>>>>>
>>>>>> The post_load/pre_save on the child vmsd can copy things from the parent
>>>>>> to the temporary using the parent pointer and do any other calculations
>>>>>> needed; it can then use normal VMSD entries to do the actual data
>>>>>> storage without having to fiddle around with qemu_get_*/qemu_put_*
>>>>>>
>>>> If customized put/get can do transformation and dumping/loading data
>>>> to/from the parent structure, you don't have to go through
>>>> pre_save/post_load, and may get rid of parent pointer.
>>>
>>> Yes but I'd rather try and get rid of the customized put/get from
>>> every device, because then people start using qemu_put/qemu_get in them all.
>>>
>> Then customized handling need to happen in pre_save/post_load. I think
>> you need a way to pass TMP pointer around?
>
> But then why is that better than having the parent pointer?
>
IIUC, from the put_tmp, I didn't see how tmp is filled with data. I
suppose it is to be filled by pre_save. So tmp pointer needs to find a
way from inside pre_save to put_tmp. How does it happen?
Thanks,
Jianjun
> Dave
>
>>
>> Thanks,
>> Jianjun
>>> Dave
>>>
>>>>
>>>> Thanks,
>>>> Jianjun
>>>>
>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
>>>>>
>>>>> The requirement for the parent pointer is a little clunky, but I don't
>>>>> quickly see a better way, and it is compile-time verified. As noted
>>>>> elsewhere I think this is a really useful approach which could allow a
>>>>> bunch of internal state cleanups while preserving migration.
>>>>>
>>>>> Reviewed-by: David Gibson <da...@gibson.dropbear.id.au>
>>>>>
>>>>>> ---
>>>>>> include/migration/vmstate.h | 20 ++++++++++++++++++++
>>>>>> migration/vmstate.c | 38 ++++++++++++++++++++++++++++++++++++++
>>>>>> 2 files changed, 58 insertions(+)
>>>>>>
>>>>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>>>>>> index 9500da1..efb0e90 100644
>>>>>> --- a/include/migration/vmstate.h
>>>>>> +++ b/include/migration/vmstate.h
>>>>>> @@ -259,6 +259,7 @@ extern const VMStateInfo vmstate_info_cpudouble;
>>>>>> extern const VMStateInfo vmstate_info_timer;
>>>>>> extern const VMStateInfo vmstate_info_buffer;
>>>>>> extern const VMStateInfo vmstate_info_unused_buffer;
>>>>>> +extern const VMStateInfo vmstate_info_tmp;
>>>>>> extern const VMStateInfo vmstate_info_bitmap;
>>>>>> extern const VMStateInfo vmstate_info_qtailq;
>>>>>>
>>>>>> @@ -651,6 +652,25 @@ extern const VMStateInfo vmstate_info_qtailq;
>>>>>> .offset = offsetof(_state, _field), \
>>>>>> }
>>>>>>
>>>>>> +/* Allocate a temporary of type 'tmp_type', set tmp->parent to _state
>>>>>> + * and execute the vmsd on the temporary. Note that we're working with
>>>>>> + * the whole of _state here, not a field within it.
>>>>>> + * We compile time check that:
>>>>>> + * That _tmp_type contains a 'parent' member that's a pointer to the
>>>>>> + * '_state' type
>>>>>> + * That the pointer is right at the start of _tmp_type.
>>>>>> + */
>>>>>> +#define VMSTATE_WITH_TMP(_state, _tmp_type, _vmsd) { \
>>>>>> + .name = "tmp", \
>>>>>> + .size = sizeof(_tmp_type) + \
>>>>>> + QEMU_BUILD_BUG_EXPR(offsetof(_tmp_type, parent) !=
>>>>>> 0) + \
>>>>>> + type_check_pointer(_state, \
>>>>>> + typeof_field(_tmp_type, parent)), \
>>>>>> + .vmsd = &(_vmsd), \
>>>>>> + .info = &vmstate_info_tmp, \
>>>>>> + .flags = VMS_LINKED, \
>>>>>> +}
>>>>>> +
>>>>>> #define VMSTATE_UNUSED_BUFFER(_test, _version, _size) { \
>>>>>> .name = "unused", \
>>>>>> .field_exists = (_test), \
>>>>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>>>>> index 2157997..f2563c5 100644
>>>>>> --- a/migration/vmstate.c
>>>>>> +++ b/migration/vmstate.c
>>>>>> @@ -925,6 +925,44 @@ const VMStateInfo vmstate_info_unused_buffer = {
>>>>>> .put = put_unused_buffer,
>>>>>> };
>>>>>>
>>>>>> +/* vmstate_info_tmp, see VMSTATE_WITH_TMP, the idea is that we allocate
>>>>>> + * a temporary buffer and the pre_load/pre_save methods in the child
>>>>>> vmsd
>>>>>> + * copy stuff from the parent into the child and do calculations to fill
>>>>>> + * in fields that don't really exist in the parent but need to be in the
>>>>>> + * stream.
>>>>>> + */
>>>>>> +static int get_tmp(QEMUFile *f, void *pv, size_t size, VMStateField
>>>>>> *field)
>>>>>> +{
>>>>>> + int ret;
>>>>>> + const VMStateDescription *vmsd = field->vmsd;
>>>>>> + int version_id = field->version_id;
>>>>>> + void *tmp = g_malloc(size);
>>>>>> +
>>>>>> + /* Writes the parent field which is at the start of the tmp */
>>>>>> + *(void **)tmp = pv;
>>>>>> + ret = vmstate_load_state(f, vmsd, tmp, version_id);
>>>>>> + g_free(tmp);
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static void put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField
>>>>>> *field,
>>>>>> + QJSON *vmdesc)
>>>>>> +{
>>>>>> + const VMStateDescription *vmsd = field->vmsd;
>>>>>> + void *tmp = g_malloc(size);
>>>>>> +
>>>>>> + /* Writes the parent field which is at the start of the tmp */
>>>>>> + *(void **)tmp = pv;
>>>>>> + vmstate_save_state(f, vmsd, tmp, vmdesc);
>>>>>> + g_free(tmp);
>>>>>> +}
>>>>>> +
>>>>>> +const VMStateInfo vmstate_info_tmp = {
>>>>>> + .name = "tmp",
>>>>>> + .get = get_tmp,
>>>>>> + .put = put_tmp,
>>>>>> +};
>>>>>> +
>>>>>> /* bitmaps (as defined by bitmap.h). Note that size here is the size
>>>>>> * of the bitmap in bits. The on-the-wire format of a bitmap is 64
>>>>>> * bit words with the bits in big endian order. The in-memory format
>>>>>
>>>>
>>> --
>>> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>>>
>>
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>
