On 06/01/2017 01:19 PM, Christian Borntraeger wrote: > On 06/01/2017 01:02 PM, Halil Pasic wrote: >> >> >> On 06/01/2017 12:52 PM, Cornelia Huck wrote: >>> On Mon, 29 May 2017 15:17:16 +0200 >>> Halil Pasic <pa...@linux.vnet.ibm.com> wrote: >>> >>>> Let's vmstatify virtio_ccw_save_config and virtio_ccw_load_config for >>>> flexibility (extending using subsections) and for fun. >>> >>> You have interesting ideas of fun :) >>> >>>> >>>> To achieve this we need to hack the config_vector, which is VirtIODevice >>>> (that is common virtio) state, in the middle of the VirtioCcwDevice state >>>> representation. This somewhat ugly, but we have no choice because the >>>> stream format needs to be preserved. >>>> >>>> Almost no changes in behavior. Exception is everything that comes with >>>> vmstate like extra bookkeeping about what's in the stream, and maybe some >>>> extra checks and better error reporting. >>>> >>>> Signed-off-by: Halil Pasic <pa...@linux.vnet.ibm.com> >>>> --- >> [..] >>>> --- a/hw/s390x/css.c >>>> +++ b/hw/s390x/css.c >> [..] >>>> +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), >>> >>> This is strictly due to stream format preservation, right? >>> >> >> Yes! >> >> [..] >>>> --- a/hw/s390x/virtio-ccw.c >>>> +++ b/hw/s390x/virtio-ccw.c >>>> @@ -34,9 +34,87 @@ >>>> #include "virtio-ccw.h" >>>> #include "trace.h" >>>> #include "hw/s390x/css-bridge.h" >>>> +#include "hw/s390x/s390-virtio-ccw.h" >>>> >>>> #define NR_CLASSIC_INDICATOR_BITS 64 >>>> >>>> +static int virtio_ccw_dev_post_load(void *opaque, int version_id) >>>> +{ >>>> + VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(opaque); >>>> + CcwDevice *ccw_dev = CCW_DEVICE(dev); >>>> + CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev); >>>> + >>>> + ccw_dev->sch->driver_data = dev; >>>> + if (CCW_DEVICE(dev)->sch->thinint_active) { >>> >>> Please use ccw_dev instead of CCW_DEVICE(dev). >>> >> >> Sure. >> >>>> + dev->routes.adapter.adapter_id = css_get_adapter_id( >>>> + CSS_IO_ADAPTER_VIRTIO, >>>> + dev->thinint_isc); >>>> + } >>>> + /* Re-fill subch_id after loading the subchannel states.*/ >>>> + if (ck->refill_ids) { >>>> + ck->refill_ids(ccw_dev); >>>> + } >>>> + return 0; >>>> +} >>>> + >>>> +typedef struct VirtioCcwDeviceTmp { >>>> + VirtioCcwDevice *parent; >>>> + uint16_t config_vector; >>>> +} VirtioCcwDeviceTmp; >>>> + >>>> +static void virtio_ccw_dev_tmp_pre_save(void *opaque) >>>> +{ >>>> + VirtioCcwDeviceTmp *tmp = opaque; >>>> + VirtioCcwDevice *dev = tmp->parent; >>>> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); >>>> + >>>> + tmp->config_vector = vdev->config_vector; >>>> +} >>>> + >>>> +static int virtio_ccw_dev_tmp_post_load(void *opaque, int version_id) >>>> +{ >>>> + VirtioCcwDeviceTmp *tmp = opaque; >>>> + VirtioCcwDevice *dev = tmp->parent; >>>> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); >>>> + >>>> + vdev->config_vector = tmp->config_vector; >>>> + return 0; >>>> +} >>>> + >>>> +const VMStateDescription vmstate_virtio_ccw_dev_tmp = { >>>> + .name = "s390_virtio_ccw_dev_tmp", >>>> + .pre_save = virtio_ccw_dev_tmp_pre_save, >>>> + .post_load = virtio_ccw_dev_tmp_post_load, >>>> + .fields = (VMStateField[]) { >>>> + VMSTATE_UINT16(config_vector, VirtioCcwDeviceTmp), >>>> + VMSTATE_END_OF_LIST() >>>> + } >>>> +}; >>>> + >>>> +const VMStateDescription vmstate_virtio_ccw_dev = { >>>> + .name = "s390_virtio_ccw_dev", >>>> + .version_id = 1, >>>> + .minimum_version_id = 1, >>>> + .post_load = virtio_ccw_dev_post_load, >>>> + .fields = (VMStateField[]) { >>>> + VMSTATE_CCW_DEVICE(parent_obj, VirtioCcwDevice), >>>> + VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice), >>>> + VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice), >>>> + VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice), >>>> + /* >>>> + * Ugly hack because VirtIODevice does not migrate itself. >>>> + * This also makes legacy via vmstate_save_state possible. >>>> + */ >>>> + VMSTATE_WITH_TMP(VirtioCcwDevice, VirtioCcwDeviceTmp, >>>> + vmstate_virtio_ccw_dev_tmp), >>>> + VMSTATE_STRUCT(routes, VirtioCcwDevice, 1, >>>> vmstate_adapter_routes, \ >>>> + AdapterRoutes), >>>> + VMSTATE_UINT8(thinint_isc, VirtioCcwDevice), >>>> + VMSTATE_INT32(revision, VirtioCcwDevice), >>>> + VMSTATE_END_OF_LIST() >>>> + } >>>> +}; >>>> + >>>> static void virtio_ccw_bus_new(VirtioBusState *bus, size_t bus_size, >>>> VirtioCcwDevice *dev); >>>> >>>> @@ -1234,85 +1312,13 @@ static int virtio_ccw_load_queue(DeviceState *d, >>>> int n, QEMUFile *f) >>>> static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f) >>>> { >>>> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); >>>> - CcwDevice *ccw_dev = CCW_DEVICE(d); >>>> - SubchDev *s = ccw_dev->sch; >>>> - VirtIODevice *vdev = virtio_ccw_get_vdev(s); >>>> - >>>> - subch_device_save(s, f); >>>> - if (dev->indicators != NULL) { >>>> - qemu_put_be32(f, dev->indicators->len); >>>> - qemu_put_be64(f, dev->indicators->addr); >>>> - } else { >>>> - qemu_put_be32(f, 0); >>>> - qemu_put_be64(f, 0UL); >>>> - } >>>> - if (dev->indicators2 != NULL) { >>>> - qemu_put_be32(f, dev->indicators2->len); >>>> - qemu_put_be64(f, dev->indicators2->addr); >>>> - } else { >>>> - qemu_put_be32(f, 0); >>>> - qemu_put_be64(f, 0UL); >>>> - } >>>> - if (dev->summary_indicator != NULL) { >>>> - qemu_put_be32(f, dev->summary_indicator->len); >>>> - qemu_put_be64(f, dev->summary_indicator->addr); >>>> - } else { >>>> - qemu_put_be32(f, 0); >>>> - qemu_put_be64(f, 0UL); >>>> - } >>>> - qemu_put_be16(f, vdev->config_vector); >>>> - qemu_put_be64(f, dev->routes.adapter.ind_offset); >>>> - qemu_put_byte(f, dev->thinint_isc); >>>> - qemu_put_be32(f, dev->revision); >>>> + vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL); >>>> } >>>> >>>> static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f) >>>> { >>>> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); >>>> - CcwDevice *ccw_dev = CCW_DEVICE(d); >>>> - CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev); >>>> - SubchDev *s = ccw_dev->sch; >>>> - VirtIODevice *vdev = virtio_ccw_get_vdev(s); >>>> - int len; >>>> - >>>> - s->driver_data = dev; >>>> - subch_device_load(s, f); >>>> - /* Re-fill subch_id after loading the subchannel states.*/ >>>> - if (ck->refill_ids) { >>>> - ck->refill_ids(ccw_dev); >>>> - } >>>> - len = qemu_get_be32(f); >>>> - if (len != 0) { >>>> - dev->indicators = get_indicator(qemu_get_be64(f), len); >>>> - } else { >>>> - qemu_get_be64(f); >>>> - dev->indicators = NULL; >>>> - } >>>> - len = qemu_get_be32(f); >>>> - if (len != 0) { >>>> - dev->indicators2 = get_indicator(qemu_get_be64(f), len); >>>> - } else { >>>> - qemu_get_be64(f); >>>> - dev->indicators2 = NULL; >>>> - } >>>> - len = qemu_get_be32(f); >>>> - if (len != 0) { >>>> - dev->summary_indicator = get_indicator(qemu_get_be64(f), len); >>>> - } else { >>>> - qemu_get_be64(f); >>>> - dev->summary_indicator = NULL; >>>> - } >>>> - qemu_get_be16s(f, &vdev->config_vector); >>>> - dev->routes.adapter.ind_offset = qemu_get_be64(f); >>>> - dev->thinint_isc = qemu_get_byte(f); >>>> - dev->revision = qemu_get_be32(f); >>>> - if (s->thinint_active) { >>>> - dev->routes.adapter.adapter_id = css_get_adapter_id( >>>> - CSS_IO_ADAPTER_VIRTIO, >>>> - dev->thinint_isc); >>>> - } >>>> - >>>> - return 0; >>>> + return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1); >>>> } >>>> >>>> static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp) >>>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h >>>> index e61fa74d9b..d74e44d312 100644 >>>> --- a/include/hw/s390x/css.h >>>> +++ b/include/hw/s390x/css.h >>>> @@ -88,6 +88,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 *); >>>> @@ -95,22 +96,27 @@ 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 VMStateDescription vmstate_ind_addr; >>>> + >>>> +#define VMSTATE_PTR_TO_IND_ADDR(_f, _s) >>>> \ >>>> + VMSTATE_STRUCT(_f, _s, 1, vmstate_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); >>>> >>>> typedef SubchDev *(*css_subch_cb_func)(uint8_t m, uint8_t cssid, uint8_t >>>> ssid, >>>> uint16_t schid); >>>> -void subch_device_save(SubchDev *s, QEMUFile *f); >>>> -int subch_device_load(SubchDev *s, QEMUFile *f); >>>> int css_create_css_image(uint8_t cssid, bool default_image); >>>> bool css_devno_used(uint8_t cssid, uint8_t ssid, uint16_t devno); >>>> void css_subch_assign(uint8_t cssid, uint8_t ssid, uint16_t schid, >>>> diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h >>>> index f9e6890c90..caa6fc608d 100644 >>>> --- a/include/hw/s390x/s390_flic.h >>>> +++ b/include/hw/s390x/s390_flic.h >>>> @@ -31,6 +31,11 @@ typedef struct AdapterRoutes { >>>> int gsi[ADAPTER_ROUTES_MAX_GSI]; >>>> } AdapterRoutes; >>>> >>>> +extern const VMStateDescription vmstate_adapter_routes; >>>> + >>>> +#define VMSTATE_ADAPTER_ROUTES(_f, _s) \ >>>> + VMSTATE_STRUCT(_f, _s, 1, vmstate_adapter_routes, AdapterRoutes) >>>> + >>>> #define TYPE_S390_FLIC_COMMON "s390-flic" >>>> #define S390_FLIC_COMMON(obj) \ >>>> OBJECT_CHECK(S390FLICState, (obj), TYPE_S390_FLIC_COMMON) >>> >>> Other than the nit above, looks good to me. Especially the migration of >>> the pmcw & co. is now more readable. So, with the nit fixed, >>> >>> Reviewed-by: Cornelia Huck <cornelia.h...@de.ibm.com> >> >> Thanks! >> >>> >>> Christian, can you please take this? >>> >> >> Should I do a re-spin with the nit's picked and the r-b's added? > > Yes please. Can you then give me a confirmation that you have tested a > migration between 2.9 and this patch set? > >
I have tested between 2.5 and this (2.5 binary and compat both). I think that should be equivalent with testing with 2.9, but I can give it a couple of spins with 2.9. Though my tests are semi automated at best. Given the nature of changes I do not expect problems, but from the methodology perspective it ain't really satisfactory. It could might make sense to throw some well designed test-suite at it. Cheers, Halil