| Message ID | 20170505173507.74077-4-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
* Halil Pasic (pasic@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 <pasic@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" > > typedef struct CrwContainer { > CRW crw; > QTAILQ_ENTRY(CrwContainer) sibling; > } CrwContainer; > > +static const VMStateDescription vmstate_crw = { > + .name = "s390_crw", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT16(flags, CRW), > + VMSTATE_UINT16(rsid, CRW), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > +static const VMStateDescription vmstate_crw_container = { > + .name = "s390_crw_container", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_STRUCT(crw, CrwContainer, 0, vmstate_crw, CRW), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > typedef struct ChpInfo { > uint8_t in_use; > uint8_t type; > uint8_t is_virtual; > } ChpInfo; > > +static const VMStateDescription vmstate_chp_info = { > + .name = "s390_chp_info", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT8(in_use, ChpInfo), > + VMSTATE_UINT8(type, ChpInfo), > + VMSTATE_UINT8(is_virtual, ChpInfo), > + VMSTATE_END_OF_LIST() > + } > +}; > + > typedef struct SubchSet { > SubchDev *sch[MAX_SCHID + 1]; > unsigned long schids_used[BITS_TO_LONGS(MAX_SCHID + 1)]; > unsigned long devnos_used[BITS_TO_LONGS(MAX_SCHID + 1)]; > } SubchSet; > > +static const VMStateDescription vmstate_scsw = { > + .name = "s390_scsw", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT16(flags, SCSW), > + VMSTATE_UINT16(ctrl, SCSW), > + VMSTATE_UINT32(cpa, SCSW), > + VMSTATE_UINT8(dstat, SCSW), > + VMSTATE_UINT8(cstat, SCSW), > + VMSTATE_UINT16(count, SCSW), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static const VMStateDescription vmstate_pmcw = { > + .name = "s390_pmcw", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(intparm, PMCW), > + VMSTATE_UINT16(flags, PMCW), > + VMSTATE_UINT16(devno, PMCW), > + VMSTATE_UINT8(lpm, PMCW), > + VMSTATE_UINT8(pnom, PMCW), > + VMSTATE_UINT8(lpum, PMCW), > + VMSTATE_UINT8(pim, PMCW), > + VMSTATE_UINT16(mbi, PMCW), > + VMSTATE_UINT8(pom, PMCW), > + VMSTATE_UINT8(pam, PMCW), > + VMSTATE_UINT8_ARRAY(chpid, PMCW, 8), > + VMSTATE_UINT32(chars, PMCW), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static const VMStateDescription vmstate_schib = { > + .name = "s390_schib", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_STRUCT(pmcw, SCHIB, 0, vmstate_pmcw, PMCW), > + VMSTATE_STRUCT(scsw, SCHIB, 0, vmstate_scsw, SCSW), > + VMSTATE_UINT64(mba, SCHIB), > + VMSTATE_UINT8_ARRAY(mda, SCHIB, 4), > + VMSTATE_END_OF_LIST() > + } > +}; > + > + > +static const VMStateDescription vmstate_ccw1 = { > + .name = "s390_ccw1", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT8(cmd_code, CCW1), > + VMSTATE_UINT8(flags, CCW1), > + VMSTATE_UINT16(count, CCW1), > + VMSTATE_UINT32(cda, CCW1), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static const VMStateDescription vmstate_ciw = { > + .name = "s390_ciw", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT8(type, CIW), > + VMSTATE_UINT8(command, CIW), > + VMSTATE_UINT16(count, CIW), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static const VMStateDescription vmstate_sense_id = { > + .name = "s390_sense_id", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT8(reserved, SenseId), > + VMSTATE_UINT16(cu_type, SenseId), > + VMSTATE_UINT8(cu_model, SenseId), > + VMSTATE_UINT16(dev_type, SenseId), > + VMSTATE_UINT8(dev_model, SenseId), > + VMSTATE_UINT8(unused, SenseId), > + VMSTATE_STRUCT_ARRAY(ciw, SenseId, MAX_CIWS, 0, vmstate_ciw, CIW), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static int subch_dev_post_load(void *opaque, int version_id); > +static void subch_dev_pre_save(void *opaque); > + > +const VMStateDescription vmstate_subch_dev = { > + .name = "s390_subch_dev", > + .version_id = 1, > + .minimum_version_id = 1, > + .post_load = subch_dev_post_load, > + .pre_save = subch_dev_pre_save, > + .fields = (VMStateField[]) { > + VMSTATE_UINT8_EQUAL(cssid, SubchDev), > + VMSTATE_UINT8_EQUAL(ssid, SubchDev), > + VMSTATE_UINT16(migrated_schid, SubchDev), > + VMSTATE_UINT16_EQUAL(devno, SubchDev), > + VMSTATE_BOOL(thinint_active, SubchDev), > + VMSTATE_STRUCT(curr_status, SubchDev, 0, vmstate_schib, SCHIB), > + VMSTATE_UINT8_ARRAY(sense_data, SubchDev, 32), > + VMSTATE_UINT64(channel_prog, SubchDev), > + VMSTATE_STRUCT(last_cmd, SubchDev, 0, vmstate_ccw1, CCW1), > + VMSTATE_BOOL(last_cmd_valid, SubchDev), > + VMSTATE_STRUCT(id, SubchDev, 0, vmstate_sense_id, SenseId), > + VMSTATE_BOOL(ccw_fmt_1, SubchDev), > + VMSTATE_UINT8(ccw_no_data_cnt, SubchDev), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +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. > + > typedef struct CssImage { > SubchSet *sch_set[MAX_SSID + 1]; > ChpInfo chpids[MAX_CHPID + 1]; > } CssImage; > > +static const VMStateDescription vmstate_css_img = { > + .name = "s390_css_img", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + /* Subchannel sets have no relevant state. */ > + VMSTATE_STRUCT_ARRAY(chpids, CssImage, MAX_CHPID + 1, 0, > + vmstate_chp_info, ChpInfo), > + VMSTATE_END_OF_LIST() > + } > + > +}; > + > typedef struct IoAdapter { > uint32_t id; > uint8_t type; > @@ -60,10 +262,34 @@ typedef struct ChannelSubSys { > uint64_t chnmon_area; > CssImage *css[MAX_CSSID + 1]; > uint8_t default_cssid; > + /* don't migrate */ > IoAdapter *io_adapters[CSS_IO_ADAPTER_TYPE_NUMS][MAX_ISC + 1]; > + /* don't migrate */ You don't say *why* Dave > QTAILQ_HEAD(, IndAddr) indicator_addresses; > } ChannelSubSys; > > +static const VMStateDescription vmstate_css = { > + .name = "s390_css", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_QTAILQ_V(pending_crws, ChannelSubSys, 1, vmstate_crw_container, > + CrwContainer, sibling), > + VMSTATE_BOOL(sei_pending, ChannelSubSys), > + VMSTATE_BOOL(do_crw_mchk, ChannelSubSys), > + VMSTATE_BOOL(crws_lost, ChannelSubSys), > + /* These were kind of migrated by virtio */ > + VMSTATE_UINT8(max_cssid, ChannelSubSys), > + VMSTATE_UINT8(max_ssid, ChannelSubSys), > + VMSTATE_BOOL(chnmon_active, ChannelSubSys), > + VMSTATE_UINT64(chnmon_area, ChannelSubSys), > + VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(css, ChannelSubSys, MAX_CSSID + 1, > + 0, vmstate_css_img, CssImage), > + VMSTATE_UINT8(default_cssid, ChannelSubSys), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static ChannelSubSys channel_subsys = { > .pending_crws = QTAILQ_HEAD_INITIALIZER(channel_subsys.pending_crws), > .do_crw_mchk = true, > @@ -75,6 +301,56 @@ static ChannelSubSys channel_subsys = { > QTAILQ_HEAD_INITIALIZER(channel_subsys.indicator_addresses), > }; > > +static void subch_dev_pre_save(void *opaque) > +{ > + SubchDev *s = opaque; > + > + /* Prepare remote_schid for save */ > + s->migrated_schid = s->schid; > +} > + > +static int subch_dev_post_load(void *opaque, int version_id) > +{ > + > + SubchDev *s = opaque; > + > + /* Re-assign the subchannel to remote_schid if necessary */ > + if (s->migrated_schid != s->schid) { > + if (css_find_subch(true, s->cssid, s->ssid, s->schid) == s) { > + /* > + * Cleanup the slot before moving to s->migrated_schid provided > + * it still belongs to us, i.e. it was not changed by previous > + * invocation of this function. > + */ > + css_subch_assign(s->cssid, s->ssid, s->schid, s->devno, NULL); > + } > + /* It's OK to re-assign without a prior de-assign. */ > + s->schid = s->migrated_schid; > + css_subch_assign(s->cssid, s->ssid, s->schid, s->devno, s); > + } > + > + if (css_migration_enabled()) { > + /* No compat voodoo to do ;) */ > + return 0; > + } > + /* > + * Hack alert. If we don't migrate the channel subsystem status > + * we still need to find out if the guest enabled mss/mcss-e. > + * If the subchannel is enabled, it certainly was able to access it, > + * so adjust the max_ssid/max_cssid values for relevant ssid/cssid > + * values. This is not watertight, but better than nothing. > + */ > + if (s->curr_status.pmcw.flags & PMCW_FLAGS_MASK_ENA) { > + if (s->ssid) { > + channel_subsys.max_ssid = MAX_SSID; > + } > + if (s->cssid != channel_subsys.default_cssid) { > + channel_subsys.max_cssid = MAX_CSSID; > + } > + } > + return 0; > +} > + > IndAddr *get_indicator(hwaddr ind_addr, int len) > { > IndAddr *indicator; > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h > index f1f0d7f..6a451b2 100644 > --- a/include/hw/s390x/css.h > +++ b/include/hw/s390x/css.h > @@ -87,6 +87,7 @@ struct SubchDev { > bool ccw_fmt_1; > bool thinint_active; > uint8_t ccw_no_data_cnt; > + uint16_t migrated_schid; /* used for missmatch detection */ > /* transport-provided data: */ > int (*ccw_cb) (SubchDev *, CCW1); > void (*disable_cb)(SubchDev *); > @@ -94,14 +95,21 @@ struct SubchDev { > void *driver_data; > }; > > +extern const VMStateDescription vmstate_subch_dev; > + > typedef struct IndAddr { > hwaddr addr; > uint64_t map; > unsigned long refcnt; > - int len; > + int32_t len; > QTAILQ_ENTRY(IndAddr) sibling; > } IndAddr; > > +extern const VMStateInfo vmstate_info_ind_addr; > + > +#define VMSTATE_PTR_TO_IND_ADDR(_f, _s) \ > + VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_ind_addr, IndAddr*) > + > IndAddr *get_indicator(hwaddr ind_addr, int len); > void release_indicator(AdapterInfo *adapter, IndAddr *indicator); > int map_indicator(AdapterInfo *adapter, IndAddr *indicator); > -- > 2.10.2 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@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 <pasic@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.) >> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h >> index f1f0d7f..6a451b2 100644 >> --- a/include/hw/s390x/css.h [..] >> >> +extern const VMStateInfo vmstate_info_ind_addr; >> + >> +#define VMSTATE_PTR_TO_IND_ADDR(_f, _s) \ >> + VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_ind_addr, IndAddr*) >> + >> IndAddr *get_indicator(hwaddr ind_addr, int len); >> void release_indicator(AdapterInfo *adapter, IndAddr *indicator); >> int map_indicator(AdapterInfo *adapter, IndAddr *indicator); >> -- >> 2.10.2 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > Cheers, Halil
On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@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 <pasic@linux.vnet.ibm.com> >> --- >> hw/s390x/css.c | 276 +++++++++++++++++++++++++++++++++++++++++++++++++ >> include/hw/s390x/css.h | 10 +- [..] >> typedef struct IoAdapter { >> uint32_t id; >> uint8_t type; >> @@ -60,10 +262,34 @@ typedef struct ChannelSubSys { >> uint64_t chnmon_area; >> CssImage *css[MAX_CSSID + 1]; >> uint8_t default_cssid; >> + /* don't migrate */ /* populated at bus init time, not subject to migration */ >> IoAdapter *io_adapters[CSS_IO_ADAPTER_TYPE_NUMS][MAX_ISC + 1]; >> + /* don't migrate */ > > You don't say *why* > Because its obvious if you stare at the code for a month or so. Joke aside let me try again (above and below). > Dave > /* migrated by the owning device when get_indicator is called */ >> QTAILQ_HEAD(, IndAddr) indicator_addresses; That is by vmstate_info_ind_addr.get == css_get_ind_addr I know it is convoluted but I don't think it is possible to simplify the interactions with reasonable effort and within this patch set. If things can be simplified, this needs to happen before or after the vmstate conversion IMHO. I would like to keep the mantra no behavior changes until patch 9 because otherwise it will get so complicated that I won't feel comfortable myself. Cheers, Halil >> } ChannelSubSys; >> >> [..]
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote: > > * Halil Pasic (pasic@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 <pasic@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. Dave > >> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h > >> index f1f0d7f..6a451b2 100644 > >> --- a/include/hw/s390x/css.h > > [..] > > >> > >> +extern const VMStateInfo vmstate_info_ind_addr; > >> + > >> +#define VMSTATE_PTR_TO_IND_ADDR(_f, _s) \ > >> + VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_ind_addr, IndAddr*) > >> + > >> IndAddr *get_indicator(hwaddr ind_addr, int len); > >> void release_indicator(AdapterInfo *adapter, IndAddr *indicator); > >> int map_indicator(AdapterInfo *adapter, IndAddr *indicator); > >> -- > >> 2.10.2 > >> > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > > > Cheers, > Halil > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 05/15/2017 08:01 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >> >> >> On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote: >>> * Halil Pasic (pasic@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 <pasic@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? We could also consider making WITH_TMP act as a normal field. Working on the whole state looks like a bit like a corner case: we have some stuff adjacent in the migration stream, and we have to map it on multiple fields (and vice-versa). Getting the whole state with a pointer to a certain field could work via container_of. Btw, I would rather call it get_indicator a factory method or even a constructor than an allocator, but I think we understand each-other anyway. Regards, Halil
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 05/15/2017 08:01 PM, Dr. David Alan Gilbert wrote: > > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >> > >> > >> On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote: > >>> * Halil Pasic (pasic@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 <pasic@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. > We could also consider making WITH_TMP act as a normal field. > Working on the whole state looks like a bit like a corner case: > we have some stuff adjacent in the migration stream, and we have > to map it on multiple fields (and vice-versa). Getting the whole > state with a pointer to a certain field could work via container_of. You do need to know which field you're working on to be able to safely use container_of, so I'm not sure how it would work for you in this case. The other thought I'd had was that perhaps we could change the temporary structure in VMSTATE_WITH_TMP to: struct foo { struct whatever **parent; so now you could write to *parent in cases like these. > Btw, I would rather call it get_indicator a factory method or even a > constructor than an allocator, but I think we understand each-other > anyway. Yes; I'm not too worried about the actual name as long as it's short and obvious. I'd thought of 'allocator' since in most cases it's used where the load-time code allocates memory for the object being loaded. A constructor is normally something I think of as happening after allocation; and a factory, hmm maybe. However, if it does the right thing I wouldn't object to any of those names. Dave P.S. I'm out for about a week. > Regards, > Halil > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote: > Dave > P.S. I'm out for about a week. Thanks for the info! Could you say something about our 'two devices two sections vs two devices one section' dilemma form PATCH 06/10 before leaving? I do not want to be pushy, but I'm also eager to make progress :). Have a good whatever it is next week! Halil
On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote: >> We could also consider making WITH_TMP act as a normal field. >> Working on the whole state looks like a bit like a corner case: >> we have some stuff adjacent in the migration stream, and we have >> to map it on multiple fields (and vice-versa). Getting the whole >> state with a pointer to a certain field could work via container_of. > You do need to know which field you're working on to be able to safely > use container_of, so I'm not sure how it would work for you in this > case. Well, if you have to write to just one field you are good because you already have a pointer to that field (.offset was added). If you need to write to multiple fields in post_load then you just pick one of the fields you are going to write to (probably the first) and use container_of to gain access to the whole state. The logic is specific to the bunch of the fields you are going to touch anyway. In fact any member of the state struct will do it's only important that you use the same when creating the VMStateField and when trying to get a pointer to the parent in pre_save and post_load. I haven't tried, so I'm not 100% sure, but if you like I can try, and send you a patch if it's viable. I think the key to a good solution is really what is intended and typical usage, and what is corner case. My patch in the other reply shows that we can do without changing the ways of VMSTATE_WITH_TMP. I think we can make what I'm trying to do here a bit prettier at the expense of making what you are doing in virtio-net a bit uglier, but whether it's a good idea to do so, I cant tell. > > The other thought I'd had was that perhaps we could change the temporary > structure in VMSTATE_WITH_TMP to: > > struct foo { > struct whatever **parent; > > so now you could write to *parent in cases like these. > Sorry, I do not get your idea. If you have some WIP patch in this direction I would be happy to provide some feedback. >> Btw, I would rather call it get_indicator a factory method or even a >> constructor than an allocator, but I think we understand each-other >> anyway. > Yes; I'm not too worried about the actual name as long as it's short > and obvious. > > I'd thought of 'allocator' since in most cases it's used where the > load-time code allocates memory for the object being loaded. > A constructor is normally something I think of as happening after > allocation; and a factory, hmm maybe. However, if it does the right > thing I wouldn't object to any of those names. > I think we are on the same page. Cheers, Halil > Dave
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote: > >> We could also consider making WITH_TMP act as a normal field. > >> Working on the whole state looks like a bit like a corner case: > >> we have some stuff adjacent in the migration stream, and we have > >> to map it on multiple fields (and vice-versa). Getting the whole > >> state with a pointer to a certain field could work via container_of. > > You do need to know which field you're working on to be able to safely > > use container_of, so I'm not sure how it would work for you in this > > case. > > > Well, if you have to write to just one field you are good because you > already have a pointer to that field (.offset was added). > > If you need to write to multiple fields in post_load then you just pick > one of the fields you are going to write to (probably the first) and use > container_of to gain access to the whole state. The logic is specific to > the bunch of the fields you are going to touch anyway. > > In fact any member of the state struct will do it's only important that > you use the same when creating the VMStateField and when trying to get a > pointer to the parent in pre_save and post_load. > > I haven't tried, so I'm not 100% sure, but if you like I can try, and send > you a patch if it's viable. > > I think the key to a good solution is really what is intended and typical > usage, and what is corner case. My patch in the other reply shows that we > can do without changing the ways of VMSTATE_WITH_TMP. I think we can make > what I'm trying to do here a bit prettier at the expense of making what > you are doing in virtio-net a bit uglier, but whether it's a good idea to > do so, I cant tell. Lets go with what you put in the other patch (I replied to it); I hadn't realised that was possible (hence my comment below). Once we have a bunch of different uses of VMSTATE_WITH_TMP in the code base, I'll step back and see how to tidy them up. Dave > > > > The other thought I'd had was that perhaps we could change the temporary > > structure in VMSTATE_WITH_TMP to: > > > > struct foo { > > struct whatever **parent; > > > > so now you could write to *parent in cases like these. > > > > Sorry, I do not get your idea. If you have some WIP patch in this > direction I would be happy to provide some feedback. > > > >> Btw, I would rather call it get_indicator a factory method or even a > >> constructor than an allocator, but I think we understand each-other > >> anyway. > > Yes; I'm not too worried about the actual name as long as it's short > > and obvious. > > > > I'd thought of 'allocator' since in most cases it's used where the > > load-time code allocates memory for the object being loaded. > > A constructor is normally something I think of as happening after > > allocation; and a factory, hmm maybe. However, if it does the right > > thing I wouldn't object to any of those names. > > > > I think we are on the same page. > > Cheers, > Halil > > > Dave > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 05/19/2017 07:47 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >> >> >> On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote: >>>> We could also consider making WITH_TMP act as a normal field. >>>> Working on the whole state looks like a bit like a corner case: >>>> we have some stuff adjacent in the migration stream, and we have >>>> to map it on multiple fields (and vice-versa). Getting the whole >>>> state with a pointer to a certain field could work via container_of. >>> You do need to know which field you're working on to be able to safely >>> use container_of, so I'm not sure how it would work for you in this >>> case. >> >> >> Well, if you have to write to just one field you are good because you >> already have a pointer to that field (.offset was added). >> >> If you need to write to multiple fields in post_load then you just pick >> one of the fields you are going to write to (probably the first) and use >> container_of to gain access to the whole state. The logic is specific to >> the bunch of the fields you are going to touch anyway. >> >> In fact any member of the state struct will do it's only important that >> you use the same when creating the VMStateField and when trying to get a >> pointer to the parent in pre_save and post_load. >> >> I haven't tried, so I'm not 100% sure, but if you like I can try, and send >> you a patch if it's viable. >> >> I think the key to a good solution is really what is intended and typical >> usage, and what is corner case. My patch in the other reply shows that we >> can do without changing the ways of VMSTATE_WITH_TMP. I think we can make >> what I'm trying to do here a bit prettier at the expense of making what >> you are doing in virtio-net a bit uglier, but whether it's a good idea to >> do so, I cant tell. > > Lets go with what you put in the other patch (I replied to it); I hadn't > realised that was possible (hence my comment below). > Once we have a bunch of different uses of VMSTATE_WITH_TMP in the code > base, I'll step back and see how to tidy them up. > > Dave > Sounds very reasonable. Let's do it like that! Halil >>> >>> The other thought I'd had was that perhaps we could change the temporary >>> structure in VMSTATE_WITH_TMP to: >>> >>> struct foo { >>> struct whatever **parent; >>> >>> so now you could write to *parent in cases like these. >>> >> >> Sorry, I do not get your idea. If you have some WIP patch in this >> direction I would be happy to provide some feedback. >> >> >>>> Btw, I would rather call it get_indicator a factory method or even a >>>> constructor than an allocator, but I think we understand each-other >>>> anyway. >>> Yes; I'm not too worried about the actual name as long as it's short >>> and obvious. >>> >>> I'd thought of 'allocator' since in most cases it's used where the >>> load-time code allocates memory for the object being loaded. >>> A constructor is normally something I think of as happening after >>> allocation; and a factory, hmm maybe. However, if it does the right >>> thing I wouldn't object to any of those names. >>> >> >> I think we are on the same page. >> >> Cheers, >> Halil >> >>> Dave >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
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" typedef struct CrwContainer { CRW crw; QTAILQ_ENTRY(CrwContainer) sibling; } CrwContainer; +static const VMStateDescription vmstate_crw = { + .name = "s390_crw", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT16(flags, CRW), + VMSTATE_UINT16(rsid, CRW), + VMSTATE_END_OF_LIST() + }, +}; + +static const VMStateDescription vmstate_crw_container = { + .name = "s390_crw_container", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_STRUCT(crw, CrwContainer, 0, vmstate_crw, CRW), + VMSTATE_END_OF_LIST() + }, +}; + typedef struct ChpInfo { uint8_t in_use; uint8_t type; uint8_t is_virtual; } ChpInfo; +static const VMStateDescription vmstate_chp_info = { + .name = "s390_chp_info", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT8(in_use, ChpInfo), + VMSTATE_UINT8(type, ChpInfo), + VMSTATE_UINT8(is_virtual, ChpInfo), + VMSTATE_END_OF_LIST() + } +}; + typedef struct SubchSet { SubchDev *sch[MAX_SCHID + 1]; unsigned long schids_used[BITS_TO_LONGS(MAX_SCHID + 1)]; unsigned long devnos_used[BITS_TO_LONGS(MAX_SCHID + 1)]; } SubchSet; +static const VMStateDescription vmstate_scsw = { + .name = "s390_scsw", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT16(flags, SCSW), + VMSTATE_UINT16(ctrl, SCSW), + VMSTATE_UINT32(cpa, SCSW), + VMSTATE_UINT8(dstat, SCSW), + VMSTATE_UINT8(cstat, SCSW), + VMSTATE_UINT16(count, SCSW), + VMSTATE_END_OF_LIST() + } +}; + +static const VMStateDescription vmstate_pmcw = { + .name = "s390_pmcw", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32(intparm, PMCW), + VMSTATE_UINT16(flags, PMCW), + VMSTATE_UINT16(devno, PMCW), + VMSTATE_UINT8(lpm, PMCW), + VMSTATE_UINT8(pnom, PMCW), + VMSTATE_UINT8(lpum, PMCW), + VMSTATE_UINT8(pim, PMCW), + VMSTATE_UINT16(mbi, PMCW), + VMSTATE_UINT8(pom, PMCW), + VMSTATE_UINT8(pam, PMCW), + VMSTATE_UINT8_ARRAY(chpid, PMCW, 8), + VMSTATE_UINT32(chars, PMCW), + VMSTATE_END_OF_LIST() + } +}; + +static const VMStateDescription vmstate_schib = { + .name = "s390_schib", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_STRUCT(pmcw, SCHIB, 0, vmstate_pmcw, PMCW), + VMSTATE_STRUCT(scsw, SCHIB, 0, vmstate_scsw, SCSW), + VMSTATE_UINT64(mba, SCHIB), + VMSTATE_UINT8_ARRAY(mda, SCHIB, 4), + VMSTATE_END_OF_LIST() + } +}; + + +static const VMStateDescription vmstate_ccw1 = { + .name = "s390_ccw1", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT8(cmd_code, CCW1), + VMSTATE_UINT8(flags, CCW1), + VMSTATE_UINT16(count, CCW1), + VMSTATE_UINT32(cda, CCW1), + VMSTATE_END_OF_LIST() + } +}; + +static const VMStateDescription vmstate_ciw = { + .name = "s390_ciw", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT8(type, CIW), + VMSTATE_UINT8(command, CIW), + VMSTATE_UINT16(count, CIW), + VMSTATE_END_OF_LIST() + } +}; + +static const VMStateDescription vmstate_sense_id = { + .name = "s390_sense_id", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT8(reserved, SenseId), + VMSTATE_UINT16(cu_type, SenseId), + VMSTATE_UINT8(cu_model, SenseId), + VMSTATE_UINT16(dev_type, SenseId), + VMSTATE_UINT8(dev_model, SenseId), + VMSTATE_UINT8(unused, SenseId), + VMSTATE_STRUCT_ARRAY(ciw, SenseId, MAX_CIWS, 0, vmstate_ciw, CIW), + VMSTATE_END_OF_LIST() + } +}; + +static int subch_dev_post_load(void *opaque, int version_id); +static void subch_dev_pre_save(void *opaque); + +const VMStateDescription vmstate_subch_dev = { + .name = "s390_subch_dev", + .version_id = 1, + .minimum_version_id = 1, + .post_load = subch_dev_post_load, + .pre_save = subch_dev_pre_save, + .fields = (VMStateField[]) { + VMSTATE_UINT8_EQUAL(cssid, SubchDev), + VMSTATE_UINT8_EQUAL(ssid, SubchDev), + VMSTATE_UINT16(migrated_schid, SubchDev), + VMSTATE_UINT16_EQUAL(devno, SubchDev), + VMSTATE_BOOL(thinint_active, SubchDev), + VMSTATE_STRUCT(curr_status, SubchDev, 0, vmstate_schib, SCHIB), + VMSTATE_UINT8_ARRAY(sense_data, SubchDev, 32), + VMSTATE_UINT64(channel_prog, SubchDev), + VMSTATE_STRUCT(last_cmd, SubchDev, 0, vmstate_ccw1, CCW1), + VMSTATE_BOOL(last_cmd_valid, SubchDev), + VMSTATE_STRUCT(id, SubchDev, 0, vmstate_sense_id, SenseId), + VMSTATE_BOOL(ccw_fmt_1, SubchDev), + VMSTATE_UINT8(ccw_no_data_cnt, SubchDev), + VMSTATE_END_OF_LIST() + } +}; + +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 +}; + typedef struct CssImage { SubchSet *sch_set[MAX_SSID + 1]; ChpInfo chpids[MAX_CHPID + 1]; } CssImage; +static const VMStateDescription vmstate_css_img = { + .name = "s390_css_img", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + /* Subchannel sets have no relevant state. */ + VMSTATE_STRUCT_ARRAY(chpids, CssImage, MAX_CHPID + 1, 0, + vmstate_chp_info, ChpInfo), + VMSTATE_END_OF_LIST() + } + +}; + typedef struct IoAdapter { uint32_t id; uint8_t type; @@ -60,10 +262,34 @@ typedef struct ChannelSubSys { uint64_t chnmon_area; CssImage *css[MAX_CSSID + 1]; uint8_t default_cssid; + /* don't migrate */ IoAdapter *io_adapters[CSS_IO_ADAPTER_TYPE_NUMS][MAX_ISC + 1]; + /* don't migrate */ QTAILQ_HEAD(, IndAddr) indicator_addresses; } ChannelSubSys; +static const VMStateDescription vmstate_css = { + .name = "s390_css", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_QTAILQ_V(pending_crws, ChannelSubSys, 1, vmstate_crw_container, + CrwContainer, sibling), + VMSTATE_BOOL(sei_pending, ChannelSubSys), + VMSTATE_BOOL(do_crw_mchk, ChannelSubSys), + VMSTATE_BOOL(crws_lost, ChannelSubSys), + /* These were kind of migrated by virtio */ + VMSTATE_UINT8(max_cssid, ChannelSubSys), + VMSTATE_UINT8(max_ssid, ChannelSubSys), + VMSTATE_BOOL(chnmon_active, ChannelSubSys), + VMSTATE_UINT64(chnmon_area, ChannelSubSys), + VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(css, ChannelSubSys, MAX_CSSID + 1, + 0, vmstate_css_img, CssImage), + VMSTATE_UINT8(default_cssid, ChannelSubSys), + VMSTATE_END_OF_LIST() + } +}; + static ChannelSubSys channel_subsys = { .pending_crws = QTAILQ_HEAD_INITIALIZER(channel_subsys.pending_crws), .do_crw_mchk = true, @@ -75,6 +301,56 @@ static ChannelSubSys channel_subsys = { QTAILQ_HEAD_INITIALIZER(channel_subsys.indicator_addresses), }; +static void subch_dev_pre_save(void *opaque) +{ + SubchDev *s = opaque; + + /* Prepare remote_schid for save */ + s->migrated_schid = s->schid; +} + +static int subch_dev_post_load(void *opaque, int version_id) +{ + + SubchDev *s = opaque; + + /* Re-assign the subchannel to remote_schid if necessary */ + if (s->migrated_schid != s->schid) { + if (css_find_subch(true, s->cssid, s->ssid, s->schid) == s) { + /* + * Cleanup the slot before moving to s->migrated_schid provided + * it still belongs to us, i.e. it was not changed by previous + * invocation of this function. + */ + css_subch_assign(s->cssid, s->ssid, s->schid, s->devno, NULL); + } + /* It's OK to re-assign without a prior de-assign. */ + s->schid = s->migrated_schid; + css_subch_assign(s->cssid, s->ssid, s->schid, s->devno, s); + } + + if (css_migration_enabled()) { + /* No compat voodoo to do ;) */ + return 0; + } + /* + * Hack alert. If we don't migrate the channel subsystem status + * we still need to find out if the guest enabled mss/mcss-e. + * If the subchannel is enabled, it certainly was able to access it, + * so adjust the max_ssid/max_cssid values for relevant ssid/cssid + * values. This is not watertight, but better than nothing. + */ + if (s->curr_status.pmcw.flags & PMCW_FLAGS_MASK_ENA) { + if (s->ssid) { + channel_subsys.max_ssid = MAX_SSID; + } + if (s->cssid != channel_subsys.default_cssid) { + channel_subsys.max_cssid = MAX_CSSID; + } + } + return 0; +} + IndAddr *get_indicator(hwaddr ind_addr, int len) { IndAddr *indicator; diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h index f1f0d7f..6a451b2 100644 --- a/include/hw/s390x/css.h +++ b/include/hw/s390x/css.h @@ -87,6 +87,7 @@ struct SubchDev { bool ccw_fmt_1; bool thinint_active; uint8_t ccw_no_data_cnt; + uint16_t migrated_schid; /* used for missmatch detection */ /* transport-provided data: */ int (*ccw_cb) (SubchDev *, CCW1); void (*disable_cb)(SubchDev *); @@ -94,14 +95,21 @@ struct SubchDev { void *driver_data; }; +extern const VMStateDescription vmstate_subch_dev; + typedef struct IndAddr { hwaddr addr; uint64_t map; unsigned long refcnt; - int len; + int32_t len; QTAILQ_ENTRY(IndAddr) sibling; } IndAddr; +extern const VMStateInfo vmstate_info_ind_addr; + +#define VMSTATE_PTR_TO_IND_ADDR(_f, _s) \ + VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_ind_addr, IndAddr*) + IndAddr *get_indicator(hwaddr ind_addr, int len); void release_indicator(AdapterInfo *adapter, IndAddr *indicator); int map_indicator(AdapterInfo *adapter, IndAddr *indicator);
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 <pasic@linux.vnet.ibm.com> --- hw/s390x/css.c | 276 +++++++++++++++++++++++++++++++++++++++++++++++++ include/hw/s390x/css.h | 10 +- 2 files changed, 285 insertions(+), 1 deletion(-)