On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pa...@linux.vnet.ibm.com) wrote: >> >> >> On 05/15/2017 08:01 PM, Dr. David Alan Gilbert wrote: >>> * Halil Pasic (pa...@linux.vnet.ibm.com) wrote: >>>> >>>> >>>> On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote: >>>>> * Halil Pasic (pa...@linux.vnet.ibm.com) wrote: >>>>>> As a preparation for switching to a vmstate based migration let us >>>>>> introduce vmstate entities (e.g. VMStateDescription) for the css entities >>>>>> to be migrated. Alongside some comments explaining or indicating the not >>>>>> migration of certain members are introduced too. >>>>>> >>>>>> No changes in behavior, we just added some dead code -- which should >>>>>> rise to life soon. >>>>>> >>>>>> Signed-off-by: Halil Pasic <pa...@linux.vnet.ibm.com> >>>>>> --- >>>>>> hw/s390x/css.c | 276 >>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> include/hw/s390x/css.h | 10 +- >>>>>> 2 files changed, 285 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >>>>>> index c03bb20..2bda7d0 100644 >>>>>> --- a/hw/s390x/css.c >>>>>> +++ b/hw/s390x/css.c >>>>>> @@ -20,29 +20,231 @@ >>>>>> #include "hw/s390x/css.h" >>>>>> #include "trace.h" >>>>>> #include "hw/s390x/s390_flic.h" >>>>>> +#include "hw/s390x/s390-virtio-ccw.h" >>>>>> >>>> >>>> [..] >>>> >>>>>> +static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size, >>>>>> + VMStateField *field) >>>>>> +{ >>>>>> + int32_t len; >>>>>> + IndAddr **ind_addr = pv; >>>>>> + >>>>>> + len = qemu_get_be32(f); >>>>>> + if (len != 0) { >>>>>> + *ind_addr = get_indicator(qemu_get_be64(f), len); >>>>>> + } else { >>>>>> + qemu_get_be64(f); >>>>>> + *ind_addr = NULL; >>>>>> + } >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size, >>>>>> + VMStateField *field, QJSON *vmdesc) >>>>>> +{ >>>>>> + IndAddr *ind_addr = *(IndAddr **) pv; >>>>>> + >>>>>> + if (ind_addr != NULL) { >>>>>> + qemu_put_be32(f, ind_addr->len); >>>>>> + qemu_put_be64(f, ind_addr->addr); >>>>>> + } else { >>>>>> + qemu_put_be32(f, 0); >>>>>> + qemu_put_be64(f, 0UL); >>>>>> + } >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +const VMStateInfo vmstate_info_ind_addr = { >>>>>> + .name = "s390_ind_addr", >>>>>> + .get = css_get_ind_addr, >>>>>> + .put = css_put_ind_addr >>>>>> +}; >>>>> >>>>> You should be able to avoid this .get/.put by using VMSTATE_WITH_TMP, >>>>> declare a temporary struct something like: >>>>> struct tmp_ind_addr { >>>>> IndAddr *parent; >>>>> uint32_t len; >>>>> uint64_t addr; >>>>> } >>>>> >>>>> and then your .get/.put routines turn into pre_save/post_load >>>>> routines to just setup the len/addr. >>>>> >>>> >>>> I don't think this is going to work -- unfortunately! You can see below, >>>> how this IndAddr* migration stuff is supposed to be used: >>>> the client code just uses the VMSTATE_PTR_TO_IND_ADDR macro as a >>>> field when describing state which needs and IndAddr* migrated. >>>> >>>> The problem is, we do not know in what state will this field >>>> be embedded, the pre_save/post_load called by put_tmp/get_tmp >>>> is however copying the pointer to this state into the parent. >>>> So instead of having a pointer to IndAddr* in those functions >>>> and updating it accordingly, I would have to find the IndAddr* >>>> in some arbitrary state (in our case VirtioCcwDevice) first, >>>> and I lack information for that. >>>> >>>> If it's hard to follow I can give you the patch I was debugging >>>> to come to this conclusion. (By the way I ended up with 10 >>>> lines of code more than in this version, and although I think >>>> it looks nicer, it's simpler only if one knows how WITH_TMP >>>> works. My plan was to ask you which version do you like more >>>> and go with that before I realized it ain't gonna work.) >>>> >>> >>> Yes, I see - I've got some similar other cases; the challenge >>> is it's a custom allocator - 'get_indicator' - and it's used >>> as fields in a few places. Hmm. >>> >>> >> >> The problem can be worked around by wrapping the WITH_TMP into a another >> vmsd and using VMSTATE_STRUCT for describing the field in question. It's >> quite some boilerplate (+16 lines). Should I post the patch here? > > Yes please. > --------------------------------8<------------------------------------------
>From a0b6c725b114745c8434f683133995c4e33d936e Mon Sep 17 00:00:00 2001 From: Halil Pasic <pa...@linux.vnet.ibm.com> Date: Tue, 9 May 2017 12:06:42 +0200 Subject: [PATCH 1/1] s390x/css: replace info with VMSTATE_WITH_TMP Convert s VMSatateInfo based solution manipulating the migration stream directly to VMSTATE_WITH_TMP witch keeps IO and transformation logic separate. Signed-off-by: Halil Pasic <pa...@linux.vnet.ibm.com> --- hw/s390x/css.c | 56 ++++++++++++++++++++++++++++++++------------------ include/hw/s390x/css.h | 4 ++-- 2 files changed, 38 insertions(+), 22 deletions(-) diff --git a/hw/s390x/css.c b/hw/s390x/css.c index 694365b395..71d1b95e3f 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -224,41 +224,57 @@ const VMStateDescription vmstate_subch_dev = { } }; -static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size, - VMStateField *field) -{ +typedef struct IndAddrPtrTmp { + IndAddr **parent; + uint64_t addr; int32_t len; - IndAddr **ind_addr = pv; +} IndAddrPtrTmp; - len = qemu_get_be32(f); - if (len != 0) { - *ind_addr = get_indicator(qemu_get_be64(f), len); +static int post_load_ind_addr(void *opaque, int version_id) +{ + IndAddrPtrTmp *ptmp = opaque; + IndAddr **ind_addr = ptmp->parent; + + if (ptmp->len != 0) { + *ind_addr = get_indicator(ptmp->addr, ptmp->len); } else { - qemu_get_be64(f); *ind_addr = NULL; } return 0; } -static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size, - VMStateField *field, QJSON *vmdesc) +static void pre_save_ind_addr(void *opaque) { - IndAddr *ind_addr = *(IndAddr **) pv; + IndAddrPtrTmp *ptmp = opaque; + IndAddr *ind_addr = *(ptmp->parent); if (ind_addr != NULL) { - qemu_put_be32(f, ind_addr->len); - qemu_put_be64(f, ind_addr->addr); + ptmp->len = ind_addr->len; + ptmp->addr = ind_addr->addr; } else { - qemu_put_be32(f, 0); - qemu_put_be64(f, 0UL); + ptmp->len = 0; + ptmp->addr = 0L; } - return 0; } -const VMStateInfo vmstate_info_ind_addr = { - .name = "s390_ind_addr", - .get = css_get_ind_addr, - .put = css_put_ind_addr +const VMStateDescription vmstate_ind_addr_tmp = { + .name = "s390_ind_addr_tmp", + .pre_save = pre_save_ind_addr, + .post_load = post_load_ind_addr, + + .fields = (VMStateField[]) { + VMSTATE_INT32(len, IndAddrPtrTmp), + VMSTATE_UINT64(addr, IndAddrPtrTmp), + VMSTATE_END_OF_LIST() + } +}; + +const VMStateDescription vmstate_ind_addr = { + .name = "s390_ind_addr_tmp", + .fields = (VMStateField[]) { + VMSTATE_WITH_TMP(IndAddr*, IndAddrPtrTmp, vmstate_ind_addr_tmp), + VMSTATE_END_OF_LIST() + } }; typedef struct CssImage { diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h index be5eb81ece..efe7cafef0 100644 --- a/include/hw/s390x/css.h +++ b/include/hw/s390x/css.h @@ -106,10 +106,10 @@ typedef struct IndAddr { QTAILQ_ENTRY(IndAddr) sibling; } IndAddr; -extern const VMStateInfo vmstate_info_ind_addr; +extern const VMStateDescription vmstate_ind_addr; #define VMSTATE_PTR_TO_IND_ADDR(_f, _s) \ - VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_ind_addr, IndAddr*) + VMSTATE_STRUCT(_f, _s, 1, vmstate_ind_addr, IndAddr*) IndAddr *get_indicator(hwaddr ind_addr, int len); void release_indicator(AdapterInfo *adapter, IndAddr *indicator); -- 2.11.2