From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47630) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwDP3-00088c-2j for qemu-devel@nongnu.org; Mon, 17 Oct 2016 15:16:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwDOy-0006dH-PH for qemu-devel@nongnu.org; Mon, 17 Oct 2016 15:16:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39132) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwDOy-0006cx-HO for qemu-devel@nongnu.org; Mon, 17 Oct 2016 15:16:48 -0400 Date: Mon, 17 Oct 2016 20:16:43 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20161017191643.GH12934@work-vm> References: <20161011171833.20803-1-dgilbert@redhat.com> <20161011171833.20803-2-dgilbert@redhat.com> <20161017033134.GM25390@umbus.fritz.box> <61d82189-401b-7063-ffa5-4442c7d11d25@linux.vnet.ibm.com> <20161017185243.GE12934@work-vm> <5a464924-aae1-1219-c30b-034d14a143ea@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5a464924-aae1-1219-c30b-034d14a143ea@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jianjun Duan Cc: David Gibson , qemu-devel@nongnu.org, amit.shah@redhat.com, quintela@redhat.com, pasic@linux.vnet.ibm.com * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: > > > On 10/17/2016 11:52 AM, Dr. David Alan Gilbert wrote: > > * Jianjun Duan (duanj@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" > >>>> > >>>> 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? Dave > > Thanks, > Jianjun > > Dave > > > >> > >> Thanks, > >> Jianjun > >> > >>>> Signed-off-by: Dr. David Alan Gilbert > >>> > >>> 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 > >>> > >>>> --- > >>>> 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 / dgilbert@redhat.com / Manchester, UK > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK