| Message ID | 20170505173507.74077-7-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > Let us use the freshly introduced vmstate migration helpers instead of > saving/loading the config manually. > > To achieve this we need to hack the config_vector which is a 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. > > Still no changes in behavior, but the dead code we added previously is > finally awakening to life. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > --- > hw/s390x/virtio-ccw.c | 116 +++++++++++++++++++------------------------------- > 1 file changed, 44 insertions(+), 72 deletions(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index c2badfe..8ab655c 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -57,6 +57,32 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id) > return 0; > } > > +static int get_config_vector(QEMUFile *f, void *pv, size_t size, > + VMStateField *field) > +{ > + VirtioCcwDevice *dev = pv; > + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); > + > + qemu_get_be16s(f, &vdev->config_vector); > + return 0; > +} > + > +static int put_config_vector(QEMUFile *f, void *pv, size_t size, > + VMStateField *field, QJSON *vmdesc) > +{ > + VirtioCcwDevice *dev = pv; > + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); > + > + qemu_put_be16(f, vdev->config_vector); > + return 0; > +} Again that should be doable using WITH_TMP. (I do wonder if we need a macro for cases where it's just a casting operation to another type). Dave > +const VMStateInfo vmstate_info_config_vector = { > + .name = "config_vector", > + .get = get_config_vector, > + .put = put_config_vector, > +}; > + > const VMStateDescription vmstate_virtio_ccw_dev = { > .name = "s390_virtio_ccw_dev", > .version_id = 1, > @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = { > 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. > + */ > + .name = "virtio/config_vector", > + .info = &vmstate_info_config_vector, > + }, > VMSTATE_STRUCT(routes, VirtioCcwDevice, 1, vmstate_adapter_routes, \ > AdapterRoutes), > VMSTATE_UINT8(thinint_isc, VirtioCcwDevice), > @@ -1272,85 +1306,23 @@ 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); > + /* > + * We save in legacy mode. The components take care of their own > + * compat. representation (based on css_migration_enabled). > + */ > + 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; > + /* > + * We load in legacy mode. The components take take care to read > + * only stuff which is actually there (based on css_migration_enabled). > + */ > + return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1); > } > > static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp) > -- > 2.10.2 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > Let us use the freshly introduced vmstate migration helpers instead of > saving/loading the config manually. > > To achieve this we need to hack the config_vector which is a 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. > > Still no changes in behavior, but the dead code we added previously is > finally awakening to life. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > --- > hw/s390x/virtio-ccw.c | 116 +++++++++++++++++++------------------------------- > 1 file changed, 44 insertions(+), 72 deletions(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index c2badfe..8ab655c 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -57,6 +57,32 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id) > return 0; > } > > +static int get_config_vector(QEMUFile *f, void *pv, size_t size, > + VMStateField *field) > +{ > + VirtioCcwDevice *dev = pv; > + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); > + > + qemu_get_be16s(f, &vdev->config_vector); > + return 0; > +} > + > +static int put_config_vector(QEMUFile *f, void *pv, size_t size, > + VMStateField *field, QJSON *vmdesc) > +{ > + VirtioCcwDevice *dev = pv; > + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); > + > + qemu_put_be16(f, vdev->config_vector); > + return 0; > +} > + > +const VMStateInfo vmstate_info_config_vector = { > + .name = "config_vector", > + .get = get_config_vector, > + .put = put_config_vector, > +}; > + > const VMStateDescription vmstate_virtio_ccw_dev = { > .name = "s390_virtio_ccw_dev", > .version_id = 1, > @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = { > 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. > + */ > + .name = "virtio/config_vector", > + .info = &vmstate_info_config_vector, > + }, I'm a bit confused - isn't just this bit the bit going through the vdev->load_config hook? Dave > VMSTATE_STRUCT(routes, VirtioCcwDevice, 1, vmstate_adapter_routes, \ > AdapterRoutes), > VMSTATE_UINT8(thinint_isc, VirtioCcwDevice), > @@ -1272,85 +1306,23 @@ 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); > + /* > + * We save in legacy mode. The components take care of their own > + * compat. representation (based on css_migration_enabled). > + */ > + 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; > + /* > + * We load in legacy mode. The components take take care to read > + * only stuff which is actually there (based on css_migration_enabled). > + */ > + return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1); > } > > static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp) > -- > 2.10.2 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 05/08/2017 07:36 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >> Let us use the freshly introduced vmstate migration helpers instead of >> saving/loading the config manually. >> >> To achieve this we need to hack the config_vector which is a 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. >> >> Still no changes in behavior, but the dead code we added previously is >> finally awakening to life. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> --- >> hw/s390x/virtio-ccw.c | 116 +++++++++++++++++++------------------------------- >> 1 file changed, 44 insertions(+), 72 deletions(-) >> >> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c >> index c2badfe..8ab655c 100644 >> --- a/hw/s390x/virtio-ccw.c >> +++ b/hw/s390x/virtio-ccw.c >> @@ -57,6 +57,32 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id) >> return 0; >> } >> >> +static int get_config_vector(QEMUFile *f, void *pv, size_t size, >> + VMStateField *field) >> +{ >> + VirtioCcwDevice *dev = pv; >> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); >> + >> + qemu_get_be16s(f, &vdev->config_vector); >> + return 0; >> +} >> + >> +static int put_config_vector(QEMUFile *f, void *pv, size_t size, >> + VMStateField *field, QJSON *vmdesc) >> +{ >> + VirtioCcwDevice *dev = pv; >> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); >> + >> + qemu_put_be16(f, vdev->config_vector); >> + return 0; >> +} >> + >> +const VMStateInfo vmstate_info_config_vector = { >> + .name = "config_vector", >> + .get = get_config_vector, >> + .put = put_config_vector, >> +}; >> + >> const VMStateDescription vmstate_virtio_ccw_dev = { >> .name = "s390_virtio_ccw_dev", >> .version_id = 1, >> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = { >> 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. >> + */ >> + .name = "virtio/config_vector", >> + .info = &vmstate_info_config_vector, >> + }, > > I'm a bit confused - isn't just this bit the bit going > through the vdev->load_config hook? > Exactly! If you examine the part underscored with ============== in virtio_ccw_(load|save)_config (that is what is behind vdev->load_config) you should see that we use vmstate_virtio_ccw_dev (that is this VMStateDescription to implement these function. Actually virtio_ccw_(load|save)_config won't do anything after patch 9 and for new machines since the VirtIOCcwDevice is going to migrate itself via DeviceClass.vmsd like other "normal" devices, and we fall back to the VirtIO specific callbacks only in "legacy mode". I hope this helps! Halil > >> VMSTATE_STRUCT(routes, VirtioCcwDevice, 1, vmstate_adapter_routes, \ >> AdapterRoutes), >> VMSTATE_UINT8(thinint_isc, VirtioCcwDevice), >> @@ -1272,85 +1306,23 @@ 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); >> + /* >> + * We save in legacy mode. The components take care of their own >> + * compat. representation (based on css_migration_enabled). >> + */ >> + 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; >> + /* >> + * We load in legacy mode. The components take take care to read >> + * only stuff which is actually there (based on css_migration_enabled). >> + */ >> + return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1); ======================================================================== >> } >> >> static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp) >> -- >> 2.10.2 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 05/08/2017 07:36 PM, Dr. David Alan Gilbert wrote: > > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >> Let us use the freshly introduced vmstate migration helpers instead of > >> saving/loading the config manually. > >> > >> To achieve this we need to hack the config_vector which is a 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. > >> > >> Still no changes in behavior, but the dead code we added previously is > >> finally awakening to life. > >> > >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >> --- > >> --- > >> hw/s390x/virtio-ccw.c | 116 +++++++++++++++++++------------------------------- > >> 1 file changed, 44 insertions(+), 72 deletions(-) > >> > >> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > >> index c2badfe..8ab655c 100644 > >> --- a/hw/s390x/virtio-ccw.c > >> +++ b/hw/s390x/virtio-ccw.c > >> @@ -57,6 +57,32 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id) > >> return 0; > >> } > >> > >> +static int get_config_vector(QEMUFile *f, void *pv, size_t size, > >> + VMStateField *field) > >> +{ > >> + VirtioCcwDevice *dev = pv; > >> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); > >> + > >> + qemu_get_be16s(f, &vdev->config_vector); > >> + return 0; > >> +} > >> + > >> +static int put_config_vector(QEMUFile *f, void *pv, size_t size, > >> + VMStateField *field, QJSON *vmdesc) > >> +{ > >> + VirtioCcwDevice *dev = pv; > >> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); > >> + > >> + qemu_put_be16(f, vdev->config_vector); > >> + return 0; > >> +} > >> + > >> +const VMStateInfo vmstate_info_config_vector = { > >> + .name = "config_vector", > >> + .get = get_config_vector, > >> + .put = put_config_vector, > >> +}; > >> + > >> const VMStateDescription vmstate_virtio_ccw_dev = { > >> .name = "s390_virtio_ccw_dev", > >> .version_id = 1, > >> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = { > >> 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. > >> + */ > >> + .name = "virtio/config_vector", > >> + .info = &vmstate_info_config_vector, > >> + }, > > > > I'm a bit confused - isn't just this bit the bit going > > through the vdev->load_config hook? > > > > Exactly! If you examine the part underscored with ============== in > virtio_ccw_(load|save)_config (that is what is behind vdev->load_config) > you should see that we use vmstate_virtio_ccw_dev (that is this > VMStateDescription to implement these function. > > Actually virtio_ccw_(load|save)_config won't do anything after patch 9 > and for new machines since the VirtIOCcwDevice is going to migrate > itself via DeviceClass.vmsd like other "normal" devices, and we fall > back to the VirtIO specific callbacks only in "legacy mode". > > I hope this helps! Hmm, still confused! Why are you changing a Virtio device not to use the same migration oddities as all the other virtio devices? I was assuming we'd have to change the virtio core code to solve that for all virtio devices. Dave > Halil > > > > > >> VMSTATE_STRUCT(routes, VirtioCcwDevice, 1, vmstate_adapter_routes, \ > >> AdapterRoutes), > >> VMSTATE_UINT8(thinint_isc, VirtioCcwDevice), > >> @@ -1272,85 +1306,23 @@ 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); > >> + /* > >> + * We save in legacy mode. The components take care of their own > >> + * compat. representation (based on css_migration_enabled). > >> + */ > >> + 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; > >> + /* > >> + * We load in legacy mode. The components take take care to read > >> + * only stuff which is actually there (based on css_migration_enabled). > >> + */ > >> + return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1); > ======================================================================== > > >> } > >> > >> static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp) > >> -- > >> 2.10.2 > >> > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 05/08/2017 07:59 PM, Dr. David Alan Gilbert wrote: >>>> const VMStateDescription vmstate_virtio_ccw_dev = { >>>> .name = "s390_virtio_ccw_dev", >>>> .version_id = 1, >>>> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = { >>>> 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. >>>> + */ >>>> + .name = "virtio/config_vector", >>>> + .info = &vmstate_info_config_vector, >>>> + }, >>> I'm a bit confused - isn't just this bit the bit going >>> through the vdev->load_config hook? >>> >> Exactly! If you examine the part underscored with ============== in >> virtio_ccw_(load|save)_config (that is what is behind vdev->load_config) >> you should see that we use vmstate_virtio_ccw_dev (that is this >> VMStateDescription to implement these function. >> >> Actually virtio_ccw_(load|save)_config won't do anything after patch 9 >> and for new machines since the VirtIOCcwDevice is going to migrate >> itself via DeviceClass.vmsd like other "normal" devices, and we fall >> back to the VirtIO specific callbacks only in "legacy mode". >> >> I hope this helps! > Hmm, still confused! > Why are you changing a Virtio device not to use the same migration > oddities as all the other virtio devices? > > I was assuming we'd have to change the virtio core code to > solve that for all virtio devices. > You can ask difficult questions ;). First I'm not aware of any ongoing effort to solve this for all virtio devices by changing the core, and probably breaking compatibility for all devices (in a sense I break migration compatibility for all virtio-ccw devices). To be honest, I have considered that move unlikely, but the possibility is one of my reasons for seeking an upstream discussion and having You and Michael on cc. Why not use virtio oddities? Because they are oddities. I have figured, it's a good idea to separate the migration of the proxy form the rest: we have two QEMU Device objects and it should be good practice, that these are migrating themselves via DeviceClass.vmsd. That's what I get with this patch set, for new machine versions (since we can not fix the past), and with the notable difference of config_vector, because it is defined as a common infrastructure (struct VirtIODevice) but ain't migrated as a common virtio infrastructure. Would you suggest to rather keep the oddities? Should I expect to see a generic solution to the problem sometime soon? Halil > Dave >
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 05/08/2017 07:59 PM, Dr. David Alan Gilbert wrote: > >>>> const VMStateDescription vmstate_virtio_ccw_dev = { > >>>> .name = "s390_virtio_ccw_dev", > >>>> .version_id = 1, > >>>> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = { > >>>> 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. > >>>> + */ > >>>> + .name = "virtio/config_vector", > >>>> + .info = &vmstate_info_config_vector, > >>>> + }, > >>> I'm a bit confused - isn't just this bit the bit going > >>> through the vdev->load_config hook? > >>> > >> Exactly! If you examine the part underscored with ============== in > >> virtio_ccw_(load|save)_config (that is what is behind vdev->load_config) > >> you should see that we use vmstate_virtio_ccw_dev (that is this > >> VMStateDescription to implement these function. > >> > >> Actually virtio_ccw_(load|save)_config won't do anything after patch 9 > >> and for new machines since the VirtIOCcwDevice is going to migrate > >> itself via DeviceClass.vmsd like other "normal" devices, and we fall > >> back to the VirtIO specific callbacks only in "legacy mode". > >> > >> I hope this helps! > > Hmm, still confused! > > Why are you changing a Virtio device not to use the same migration > > oddities as all the other virtio devices? > > > > I was assuming we'd have to change the virtio core code to > > solve that for all virtio devices. > > > > You can ask difficult questions ;). First I'm not aware of any > ongoing effort to solve this for all virtio devices by changing > the core, and probably breaking compatibility for all devices > (in a sense I break migration compatibility for all virtio-ccw > devices). To be honest, I have considered that move unlikely, > but the possibility is one of my reasons for seeking an upstream > discussion and having You and Michael on cc. Well I have been trying to improve it, but that code is particularly nasty; and I don't want to break compatibility. I had some ideas, for example I was thinking of changing the vdev->load_config to a VMState* and then calling a vmstate_load_state(f, vdc->config,... from virtio_load - but there's some challenging casting and hackery there. > > Why not use virtio oddities? Because they are oddities. I have > figured, it's a good idea to separate the migration of the > proxy form the rest: we have two QEMU Device objects and it > should be good practice, that these are migrating themselves via > DeviceClass.vmsd. That's what I get with this patch set, > for new machine versions (since we can not fix the past), and > with the notable difference of config_vector, because it is > defined as a common infrastructure (struct VirtIODevice) but > ain't migrated as a common virtio infrastructure. Have you got a bit of a description of your classes/structure - it's a little hard to get my head around. > Would you suggest to rather keep the oddities? Should I expect > to see a generic solution to the problem sometime soon? Not soon I fear; virtio_load is just too hairy. Dave > Halil > > > Dave > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >> >> >> On 05/08/2017 07:59 PM, Dr. David Alan Gilbert wrote: >>>>>> const VMStateDescription vmstate_virtio_ccw_dev = { >>>>>> .name = "s390_virtio_ccw_dev", >>>>>> .version_id = 1, >>>>>> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = { >>>>>> 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. >>>>>> + */ >>>>>> + .name = "virtio/config_vector", >>>>>> + .info = &vmstate_info_config_vector, >>>>>> + }, >>>>> I'm a bit confused - isn't just this bit the bit going >>>>> through the vdev->load_config hook? >>>>> >>>> Exactly! If you examine the part underscored with ============== in >>>> virtio_ccw_(load|save)_config (that is what is behind vdev->load_config) >>>> you should see that we use vmstate_virtio_ccw_dev (that is this >>>> VMStateDescription to implement these function. >>>> >>>> Actually virtio_ccw_(load|save)_config won't do anything after patch 9 >>>> and for new machines since the VirtIOCcwDevice is going to migrate >>>> itself via DeviceClass.vmsd like other "normal" devices, and we fall >>>> back to the VirtIO specific callbacks only in "legacy mode". >>>> >>>> I hope this helps! >>> Hmm, still confused! >>> Why are you changing a Virtio device not to use the same migration >>> oddities as all the other virtio devices? >>> >>> I was assuming we'd have to change the virtio core code to >>> solve that for all virtio devices. >>> >> >> You can ask difficult questions ;). First I'm not aware of any >> ongoing effort to solve this for all virtio devices by changing >> the core, and probably breaking compatibility for all devices >> (in a sense I break migration compatibility for all virtio-ccw >> devices). To be honest, I have considered that move unlikely, >> but the possibility is one of my reasons for seeking an upstream >> discussion and having You and Michael on cc. > > Well I have been trying to improve it, but that code is particularly > nasty; and I don't want to break compatibility. I had some ideas, > for example I was thinking of changing the vdev->load_config to > a VMState* and then calling a vmstate_load_state(f, vdc->config,... > from virtio_load - but there's some challenging casting and hackery > there. > >> >> Why not use virtio oddities? Because they are oddities. I have >> figured, it's a good idea to separate the migration of the >> proxy form the rest: we have two QEMU Device objects and it >> should be good practice, that these are migrating themselves via >> DeviceClass.vmsd. That's what I get with this patch set, >> for new machine versions (since we can not fix the past), and >> with the notable difference of config_vector, because it is >> defined as a common infrastructure (struct VirtIODevice) but >> ain't migrated as a common virtio infrastructure. > > Have you got a bit of a description of your classes/structure - it's > a little hard to get my head around. > Unfortunately I do not have any extra description besides the comments and the commit messages. What exactly do you mean by 'my classes/structure'? I would like to provide some helpful developer documentation on how migration works for s390x. There were voices on the internal mailing list too requesting something like that, but I find it hard, because for me, the most challenging part was understanding how qemu migration works in general and the virtio oddities come next. Fore example, I still don't understand why is is (virtio) load_config called like that, when what it mainly does is loading state of the proxy which is basically the reification of the device side of the virtio spec calls the transport within QOM. (I say mainly, because of this config_vector which resides in core but is migrated by via a callback for some strange reason I do not understand). Could tell me to which (specific) questions should I provide an answer? It would make my job much easier. About the general approach. First step was to provide VMStateDescription for the entities which have migration relevant state but no VMStateDescription (patches 3, 4 and 5). This is done so that lots of qemu_put/qem_get calls can be replaced with few vmstate_save_state/vmstate_save_state calls (patch 6 and 7) on one hand, and that state not migrated yet but needed is also included, if the compat. switch (property) added in patch 2 is on. Then in patch 8, I add ORB which is a state we wanted to add for some time now, but we needed vmstate to add it without breaking migration. So we waited. >> Would you suggest to rather keep the oddities? Should I expect >> to see a generic solution to the problem sometime soon? > > Not soon I fear; virtio_load is just too hairy. Of course it ain't a problem for me to omit assigning an vmsd to VirtioCcwDeviceClass, but since I have to introduce a new section anyway (for the css stuff) it ain't hurting me to put the state of VirtioCcwDevice, that is the virtio proxy, into a separate section. I can't think of any decisive benefit in favor of having separate sections for the proxy (transport) providing a virtio bus and the generic virtio device sitting on that bus, but I think it should be slightly more robust. One of the reasons I included this change is to make thinking about the question easier by clearing the questions: is it viable and complex/ugly is it to implement. What is your preference, keep the migration of the two devices lumped together in one section, or rather go with two? Halil > > Dave > >> Halil >> >>> Dave >>> >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote: > > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >> > >> > >> On 05/08/2017 07:59 PM, Dr. David Alan Gilbert wrote: > >>>>>> const VMStateDescription vmstate_virtio_ccw_dev = { > >>>>>> .name = "s390_virtio_ccw_dev", > >>>>>> .version_id = 1, > >>>>>> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = { > >>>>>> 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. > >>>>>> + */ > >>>>>> + .name = "virtio/config_vector", > >>>>>> + .info = &vmstate_info_config_vector, > >>>>>> + }, > >>>>> I'm a bit confused - isn't just this bit the bit going > >>>>> through the vdev->load_config hook? > >>>>> > >>>> Exactly! If you examine the part underscored with ============== in > >>>> virtio_ccw_(load|save)_config (that is what is behind vdev->load_config) > >>>> you should see that we use vmstate_virtio_ccw_dev (that is this > >>>> VMStateDescription to implement these function. > >>>> > >>>> Actually virtio_ccw_(load|save)_config won't do anything after patch 9 > >>>> and for new machines since the VirtIOCcwDevice is going to migrate > >>>> itself via DeviceClass.vmsd like other "normal" devices, and we fall > >>>> back to the VirtIO specific callbacks only in "legacy mode". > >>>> > >>>> I hope this helps! > >>> Hmm, still confused! > >>> Why are you changing a Virtio device not to use the same migration > >>> oddities as all the other virtio devices? > >>> > >>> I was assuming we'd have to change the virtio core code to > >>> solve that for all virtio devices. > >>> > >> > >> You can ask difficult questions ;). First I'm not aware of any > >> ongoing effort to solve this for all virtio devices by changing > >> the core, and probably breaking compatibility for all devices > >> (in a sense I break migration compatibility for all virtio-ccw > >> devices). To be honest, I have considered that move unlikely, > >> but the possibility is one of my reasons for seeking an upstream > >> discussion and having You and Michael on cc. > > > > Well I have been trying to improve it, but that code is particularly > > nasty; and I don't want to break compatibility. I had some ideas, > > for example I was thinking of changing the vdev->load_config to > > a VMState* and then calling a vmstate_load_state(f, vdc->config,... > > from virtio_load - but there's some challenging casting and hackery > > there. > > > >> > >> Why not use virtio oddities? Because they are oddities. I have > >> figured, it's a good idea to separate the migration of the > >> proxy form the rest: we have two QEMU Device objects and it > >> should be good practice, that these are migrating themselves via > >> DeviceClass.vmsd. That's what I get with this patch set, > >> for new machine versions (since we can not fix the past), and > >> with the notable difference of config_vector, because it is > >> defined as a common infrastructure (struct VirtIODevice) but > >> ain't migrated as a common virtio infrastructure. > > > > Have you got a bit of a description of your classes/structure - it's > > a little hard to get my head around. > > > > Unfortunately I do not have any extra description besides the comments > and the commit messages. What exactly do you mean by 'my > classes/structure'? I would like to provide some helpful developer > documentation on how migration works for s390x. There were voices on the > internal mailing list too requesting something like that, but I find it > hard, because for me, the most challenging part was understanding how > qemu migration works in general and the virtio oddities come next. Yes, there are only about 2 people who have the overlap of understanding migration AND s390 IO. > Fore example, I still don't understand why is is (virtio) load_config > called like that, when what it mainly does is loading state of the proxy > which is basically the reification of the device side of the virtio spec > calls the transport within QOM. (I say mainly, because of this > config_vector which resides in core but is migrated by via a callback for > some strange reason I do not understand). I think the idea is that virtio_load is trying to act as a generic save/load with a number of virtual components that are specialised for: a) The device (e.g. rng, serial, gpu, net, blk) b) The transport (PCI, MMIO, CCW etc) c) The virtio queue content d) But has a load of core stuff (features, the virtio ring management) (a) & (b) are very much virtual-function like that doesn't fit that well with the migration macro structure. The split between (a) & (c) isn't necessary clean - gpu does it a different way. And the order of a/b/c/d is very random (aka wrong). > Could tell me to which (specific) questions should I provide an answer? > It would make my job much easier. > > About the general approach. First step was to provide VMStateDescription > for the entities which have migration relevant state but no > VMStateDescription (patches 3, 4 and 5). This is done so that > lots of qemu_put/qem_get calls can be replaced with few > vmstate_save_state/vmstate_save_state calls (patch 6 and 7) on one hand, > and that state not migrated yet but needed is also included, if the > compat. switch (property) added in patch 2 is on. Then in patch 8, I add > ORB which is a state we wanted to add for some time now, but we needed > vmstate to add it without breaking migration. So we waited. I'm most interested at this point in understanding which bits aren't changing behaviour - if we've got stuff that's just converting qemu_get to vmstate then lets go for it, no problem; easy to check. I'm just trying to make sure I understand the bit where you're converting from being a virtio device. > >> Would you suggest to rather keep the oddities? Should I expect > >> to see a generic solution to the problem sometime soon? > > > > Not soon I fear; virtio_load is just too hairy. > > Of course it ain't a problem for me to omit assigning an vmsd to > VirtioCcwDeviceClass, but since I have to introduce a new section anyway > (for the css stuff) it ain't hurting me to put the state of > VirtioCcwDevice, that is the virtio proxy, into a separate section. > > I can't think of any decisive benefit in favor of having separate > sections for the proxy (transport) providing a virtio bus and the generic > virtio device sitting on that bus, but I think it should be slightly more > robust. One of the reasons I included this change is to make thinking > about the question easier by clearing the questions: is it viable and > complex/ugly is it to implement. > > What is your preference, keep the migration of the two devices lumped > together in one section, or rather go with two? I'm not sure! But my main worries with you changing it are: a) What happens when something changes in virtio and they need to add some new virtio field somewhere - if you do it different will it make it harder. b) If you have a virtio device which does it differently, is it going to make cleaning up virtio_load/save even harder? Dave > Halil > > > > > Dave > > > >> Halil > >> > >>> Dave > >>> > >> > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 05/15/2017 09:07 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >> >> >> On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote: >>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >>>> >>>> >>>> On 05/08/2017 07:59 PM, Dr. David Alan Gilbert wrote: [..] >>>> >>>> Why not use virtio oddities? Because they are oddities. I have >>>> figured, it's a good idea to separate the migration of the >>>> proxy form the rest: we have two QEMU Device objects and it >>>> should be good practice, that these are migrating themselves via >>>> DeviceClass.vmsd. That's what I get with this patch set, >>>> for new machine versions (since we can not fix the past), and >>>> with the notable difference of config_vector, because it is >>>> defined as a common infrastructure (struct VirtIODevice) but >>>> ain't migrated as a common virtio infrastructure. >>> >>> Have you got a bit of a description of your classes/structure - it's >>> a little hard to get my head around. >>> >> >> Unfortunately I do not have any extra description besides the comments >> and the commit messages. What exactly do you mean by 'my >> classes/structure'? I would like to provide some helpful developer >> documentation on how migration works for s390x. There were voices on the >> internal mailing list too requesting something like that, but I find it >> hard, because for me, the most challenging part was understanding how >> qemu migration works in general and the virtio oddities come next. > > Yes, there are only about 2 people who have the overlap of understanding > migration AND s390 IO. > >> Fore example, I still don't understand why is is (virtio) load_config >> called like that, when what it mainly does is loading state of the proxy >> which is basically the reification of the device side of the virtio spec >> calls the transport within QOM. (I say mainly, because of this >> config_vector which resides in core but is migrated by via a callback for >> some strange reason I do not understand). > > I think the idea is that virtio_load is trying to act as a generic > save/load with a number of virtual components that are specialised for: > a) The device (e.g. rng, serial, gpu, net, blk) > b) The transport (PCI, MMIO, CCW etc) > c) The virtio queue content > d) But has a load of core stuff (features, the virtio ring management) > > (a) & (b) are very much virtual-function like that doesn't fit that > well with the migration macro structure. > > The split between (a) & (c) isn't necessary clean - gpu does it a > different way. > And the order of a/b/c/d is very random (aka wrong). > I mostly agree with your analysis. Honestly I have forgot abut this load_queue callback (I think its c)), but it's a strange one too. What it does is handling the vector of the queue which is again common infrastructure in a sense that it reside within VirtIODevice, but it may need some proxy specific handling. In my understanding the virtio migration and the migration subsystem (lets call it vmstate) are a misfit in the following aspect. Most importantly it separation of concerns. In my understanding, for vmstate, each device is supposed to load/save itself, and loading state and doing stuff with the state we have loaded are separate concerns. I'm not sure whats the vmstate place for code which is supposed to run as a part of the migration logic, but requires cooperation of devices (e.g. notify in virtio_load which basically generates an interrupt). >> Could tell me to which (specific) questions should I provide an answer? >> It would make my job much easier. >> >> About the general approach. First step was to provide VMStateDescription >> for the entities which have migration relevant state but no >> VMStateDescription (patches 3, 4 and 5). This is done so that >> lots of qemu_put/qem_get calls can be replaced with few >> vmstate_save_state/vmstate_save_state calls (patch 6 and 7) on one hand, >> and that state not migrated yet but needed is also included, if the >> compat. switch (property) added in patch 2 is on. Then in patch 8, I add >> ORB which is a state we wanted to add for some time now, but we needed >> vmstate to add it without breaking migration. So we waited. > > I'm most interested at this point in understanding which bits aren't > changing behaviour - if we've got stuff that's just converting qemu_get > to vmstate then lets go for it, no problem; easy to check. The commit messages should be helpful. Up to patch 8 all I do is converting qemu_get to vmstate as you said. > I'm just trying to make sure I understand the bit where you're > converting from being a virtio device. > By converting from being a virtio device you mean factoring out the transport stuff into a separate section? That's happening in patch 9. Let me try to explain that patch. The think of that patch is the following: * Prior to it css_migration_enabled() always returned false. After patch 9 it returns true for new machine versions and false for old ones. That ensures there is still no change of behavior except for the conversion to vmstate for old machines. It's hunk where the change happens: @@ -196,7 +196,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) s390mc->ri_allowed = true; s390mc->cpu_model_allowed = true; - s390mc->css_migration_enabled = false; /* TODO: set to true */ + s390mc->css_migration_enabled = true; * If css_migration_enabled() we turn virtio_load_config into noop because we use the DeviceClass.vmsd to migrate the stuff that was migrated in virtio_load_config (new section). * We need to to make sure if !css_migration_enabled() we do not have a new section (this is what we rewrote with top level .needed). * ccw_machine_2_10_instance_options makes sure all future machine versions are calling css_register_vmstate() if css_migration_enabled() so we have a new subsection for the common css stuff (not device specific). * css_migration_enabled() returning true has a side effect that some subsections are become needed. That's effectively switching between compat. representation and complete representation. That includes the ORB which was added in patch 8 so that it's subsection can utilize this very same kill switch instead of needing a new property. Otherwise the ORB has no connection with the objective of this patch set whatsoever. >>>> Would you suggest to rather keep the oddities? Should I expect >>>> to see a generic solution to the problem sometime soon? >>> >>> Not soon I fear; virtio_load is just too hairy. >> >> Of course it ain't a problem for me to omit assigning an vmsd to >> VirtioCcwDeviceClass, but since I have to introduce a new section anyway >> (for the css stuff) it ain't hurting me to put the state of >> VirtioCcwDevice, that is the virtio proxy, into a separate section. >> >> I can't think of any decisive benefit in favor of having separate >> sections for the proxy (transport) providing a virtio bus and the generic >> virtio device sitting on that bus, but I think it should be slightly more >> robust. One of the reasons I included this change is to make thinking >> about the question easier by clearing the questions: is it viable and >> complex/ugly is it to implement. >> >> What is your preference, keep the migration of the two devices lumped >> together in one section, or rather go with two? > > I'm not sure! > But my main worries with you changing it are: > a) What happens when something changes in virtio and they need to add > some new virtio field somewhere - if you do it different will it > make it harder. > b) If you have a virtio device which does it differently, is it going > to make cleaning up virtio_load/save even harder? > > Dave My biggest concerns are actually -- let's assign them letters too: c) Possible dependencies between the states of the proxy and the device: If virtio_load should need some transport state, or the other way around, if the proxy device should need some device state in post_load for example we have a problem (AFAIK) because the order in which the states of the two qdev devices are loaded not any more under the control of virtio_load. d) It would be an oddball among the oddballs. I have no idea how would I change the documentation to reflect that properly. I think c) is quite nasty. For instance we have virtio_notify_vector(vdev, VIRTIO_NO_VECTOR) from virtio_load is called which needs to happen after the proxy is loaded. My code works, because the order of load is the same as the order of load, which is the same as the order of device_set_realized calls which is proxy first and then device (e.g. virtio-net). But it's fragile, because of some recent changes with priorities. So should virtio get (high) prio assigned to it this would break pretty bad! I wonder if thinking into the direction of splitting the stuff makes sense any more. If we agree on not splitting up the virtio proxy and the rest into two sections (one per device), I would like to fix that and do a re-spin. Your opinion? I would also like give you my opinion on your concerns, keep on reading if you think it's still relevant: AFAIU a) is a human problem, and I'm not that concerned about it: if it is a generic virtio field, it should be placed into the core, and is completely unaffected by this change (except if something breaks it may be easier to figure out because different sections). If it is a transport specific thing, it's likely to be done by the s390x folks anyway. So I think @Connie should make the call on that one. About b), I guess it depends. If things go towards separate sections for separate qdev devices (that is, for the transport (proxy) and for the generic specific virtio device (e.g. rng, serial, gpu, net, blk) which inherits for VirtIODevice which is a part of what we call the virtio core it may even simplify things. If we are going to keep the state of the two devices in one section, then it would remain an exception, and exceptions are ugly. To sum it up although I'm currently leaning towards abandoning my idea of two sections for two devices, I'm not comfortable with making the call myself. I'm hoping for some maintainer guidance (s390x, virtio and migration). Many thanks! Halil
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 05/15/2017 09:07 PM, Dr. David Alan Gilbert wrote: > > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >> > >> > >> On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote: > >>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >>>> > >>>> > >>>> On 05/08/2017 07:59 PM, Dr. David Alan Gilbert wrote: > [..] > >>>> > >>>> Why not use virtio oddities? Because they are oddities. I have > >>>> figured, it's a good idea to separate the migration of the > >>>> proxy form the rest: we have two QEMU Device objects and it > >>>> should be good practice, that these are migrating themselves via > >>>> DeviceClass.vmsd. That's what I get with this patch set, > >>>> for new machine versions (since we can not fix the past), and > >>>> with the notable difference of config_vector, because it is > >>>> defined as a common infrastructure (struct VirtIODevice) but > >>>> ain't migrated as a common virtio infrastructure. > >>> > >>> Have you got a bit of a description of your classes/structure - it's > >>> a little hard to get my head around. > >>> > >> > >> Unfortunately I do not have any extra description besides the comments > >> and the commit messages. What exactly do you mean by 'my > >> classes/structure'? I would like to provide some helpful developer > >> documentation on how migration works for s390x. There were voices on the > >> internal mailing list too requesting something like that, but I find it > >> hard, because for me, the most challenging part was understanding how > >> qemu migration works in general and the virtio oddities come next. > > > > Yes, there are only about 2 people who have the overlap of understanding > > migration AND s390 IO. > > > >> Fore example, I still don't understand why is is (virtio) load_config > >> called like that, when what it mainly does is loading state of the proxy > >> which is basically the reification of the device side of the virtio spec > >> calls the transport within QOM. (I say mainly, because of this > >> config_vector which resides in core but is migrated by via a callback for > >> some strange reason I do not understand). > > > > I think the idea is that virtio_load is trying to act as a generic > > save/load with a number of virtual components that are specialised for: > > a) The device (e.g. rng, serial, gpu, net, blk) > > b) The transport (PCI, MMIO, CCW etc) > > c) The virtio queue content > > d) But has a load of core stuff (features, the virtio ring management) > > > > (a) & (b) are very much virtual-function like that doesn't fit that > > well with the migration macro structure. > > > > The split between (a) & (c) isn't necessary clean - gpu does it a > > different way. > > And the order of a/b/c/d is very random (aka wrong). > > > > I mostly agree with your analysis. Honestly I have forgot abut this > load_queue callback (I think its c)), but it's a strange one too. What it > does is handling the vector of the queue which is again common > infrastructure in a sense that it reside within VirtIODevice, but it may > need some proxy specific handling. > > In my understanding the virtio migration and the migration subsystem > (lets call it vmstate) are a misfit in the following aspect. Most > importantly it separation of concerns. In my understanding, for vmstate, > each device is supposed to load/save itself, and loading state and doing > stuff with the state we have loaded are separate concerns. I'm not sure > whats the vmstate place for code which is supposed to run as a part of > the migration logic, but requires cooperation of devices (e.g. notify in > virtio_load which basically generates an interrupt). > > > >> Could tell me to which (specific) questions should I provide an answer? > >> It would make my job much easier. > >> > >> About the general approach. First step was to provide VMStateDescription > >> for the entities which have migration relevant state but no > >> VMStateDescription (patches 3, 4 and 5). This is done so that > >> lots of qemu_put/qem_get calls can be replaced with few > >> vmstate_save_state/vmstate_save_state calls (patch 6 and 7) on one hand, > >> and that state not migrated yet but needed is also included, if the > >> compat. switch (property) added in patch 2 is on. Then in patch 8, I add > >> ORB which is a state we wanted to add for some time now, but we needed > >> vmstate to add it without breaking migration. So we waited. > > > > I'm most interested at this point in understanding which bits aren't > > changing behaviour - if we've got stuff that's just converting qemu_get > > to vmstate then lets go for it, no problem; easy to check. > > The commit messages should be helpful. Up to patch 8 all I do is > converting qemu_get to vmstate as you said. > > > I'm just trying to make sure I understand the bit where you're > > converting from being a virtio device. > > > > By converting from being a virtio device you mean factoring out the > transport stuff into a separate section? That's happening in patch > 9. Let me try to explain that patch. > > The think of that patch is the following: > * Prior to it css_migration_enabled() always returned false. After > patch 9 it returns true for new machine versions and false for old > ones. That ensures there is still no change of behavior except > for the conversion to vmstate for old machines. > > It's hunk where the change happens: > @@ -196,7 +196,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) > > s390mc->ri_allowed = true; > s390mc->cpu_model_allowed = true; > - s390mc->css_migration_enabled = false; /* TODO: set to true */ > + s390mc->css_migration_enabled = true; > > * If css_migration_enabled() we turn virtio_load_config into noop because we > use the DeviceClass.vmsd to migrate the stuff that was migrated > in virtio_load_config (new section). > * We need to to make sure if !css_migration_enabled() we do not have > a new section (this is what we rewrote with top level .needed). > * ccw_machine_2_10_instance_options makes sure all future machine versions > are calling css_register_vmstate() if css_migration_enabled() so > we have a new subsection for the common css stuff (not device specific). > * css_migration_enabled() returning true has a side effect that some > subsections are become needed. That's effectively switching between > compat. representation and complete representation. That includes > the ORB which was added in patch 8 so that it's subsection can utilize > this very same kill switch instead of needing a new property. Otherwise > the ORB has no connection with the objective of this patch set whatsoever. > > > >>>> Would you suggest to rather keep the oddities? Should I expect > >>>> to see a generic solution to the problem sometime soon? > >>> > >>> Not soon I fear; virtio_load is just too hairy. > >> > >> Of course it ain't a problem for me to omit assigning an vmsd to > >> VirtioCcwDeviceClass, but since I have to introduce a new section anyway > >> (for the css stuff) it ain't hurting me to put the state of > >> VirtioCcwDevice, that is the virtio proxy, into a separate section. > >> > >> I can't think of any decisive benefit in favor of having separate > >> sections for the proxy (transport) providing a virtio bus and the generic > >> virtio device sitting on that bus, but I think it should be slightly more > >> robust. One of the reasons I included this change is to make thinking > >> about the question easier by clearing the questions: is it viable and > >> complex/ugly is it to implement. > >> > >> What is your preference, keep the migration of the two devices lumped > >> together in one section, or rather go with two? > > > > I'm not sure! > > But my main worries with you changing it are: > > a) What happens when something changes in virtio and they need to add > > some new virtio field somewhere - if you do it different will it > > make it harder. > > b) If you have a virtio device which does it differently, is it going > > to make cleaning up virtio_load/save even harder? > > > > Dave > > My biggest concerns are actually -- let's assign them letters too: > c) Possible dependencies between the states of the proxy and the device: > If virtio_load should need some transport state, or the other way around, > if the proxy device should need some device state in post_load for > example we have a problem (AFAIK) because the order in which the states > of the two qdev devices are loaded not any more under the control of > virtio_load. > d) It would be an oddball among the oddballs. I have no idea how would I > change the documentation to reflect that properly. > > I think c) is quite nasty. For instance we have > virtio_notify_vector(vdev, VIRTIO_NO_VECTOR) from virtio_load is called > which needs to happen after the proxy is loaded. My code works, because > the order of load is the same as the order of load, which is the same > as the order of device_set_realized calls which is proxy first > and then device (e.g. virtio-net). But it's fragile, because of some > recent changes with priorities. So should virtio get (high) prio assigned > to it this would break pretty bad! > > I wonder if thinking into the direction of splitting the stuff makes > sense any more. > > If we agree on not splitting up the virtio proxy and the rest into two > sections (one per device), I would like to fix that and do a re-spin. > Your opinion? > > I would also like give you my opinion on your concerns, keep on reading > if you think it's still relevant: > > AFAIU a) is a human problem, and I'm not that concerned about it: if it > is a generic virtio field, it should be placed into the core, and is > completely unaffected by this change (except if something breaks it may > be easier to figure out because different sections). If it is a transport > specific thing, it's likely to be done by the s390x folks anyway. So I > think @Connie should make the call on that one. > > About b), I guess it depends. If things go towards separate sections for > separate qdev devices (that is, for the transport (proxy) and for the > generic specific virtio device (e.g. rng, serial, gpu, net, blk) which > inherits for VirtIODevice which is a part of what we call the virtio > core it may even simplify things. If we are going to keep the state of > the two devices in one section, then it would remain an exception, and > exceptions are ugly. > > To sum it up although I'm currently leaning towards abandoning my idea > of two sections for two devices, I'm not comfortable with making the > call myself. I'm hoping for some maintainer guidance (s390x, virtio > and migration). <A couple of hours of reading, the CCW and PCI code, chatting with dhildenb etc> OK, so I think: a) First split the series into two separate series; one that VMStatifies the existing stuff without breaking compatibility; and one that adds the new stuff. Lets get the first of those going in while we think about the second. a.1) I'd do this with patches that convert one chunk into vmstate and remove the corresponding C code at the same time. b) While the way PCI devices are done is weird, I think it'll be a lot simpler if you can stick to a structure that's similar to them while diverging. It's hard enough for those of us who don't do Virtio every day to get our minds around virtio-pci without it being different for virtio-ccw/css. c) To do (b) I suggest: c.1) you *don't* add a vmsd to your virtio_ccw_device c.2) but *do* add a VMSTATE_CCW_DEVICE to the start of any non-virtio devices you migrate (like each of the PCI devices have) c.3) You can add extra state for CSS to the ->save_extra_state handle on virtio devices or to the config d) vmstatifying the config is OK as well. I should say I'm no virtio expert, so if any of that's truly mad say so. Dave > Many thanks! > > Halil > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 05/19/2017 07:28 PM, Dr. David Alan Gilbert wrote: >> To sum it up although I'm currently leaning towards abandoning my idea >> of two sections for two devices, I'm not comfortable with making the >> call myself. I'm hoping for some maintainer guidance (s390x, virtio >> and migration). > <A couple of hours of reading, the CCW and PCI code, chatting with > dhildenb etc> > > OK, so I think: > a) First split the series into two separate series; one that > VMStatifies the existing stuff without breaking compatibility; > and one that adds the new stuff. Lets get the first of those > going in while we think about the second. > > a.1) I'd do this with patches that convert one chunk into > vmstate and remove the corresponding C code at the same time. > > b) While the way PCI devices are done is weird, I think it'll > be a lot simpler if you can stick to a structure that's similar > to them while diverging. It's hard enough for those of us > who don't do Virtio every day to get our minds around virtio-pci > without it being different for virtio-ccw/css. > > c) To do (b) I suggest: > c.1) you *don't* add a vmsd to your virtio_ccw_device > > c.2) but *do* add a VMSTATE_CCW_DEVICE to the start of any > non-virtio devices you migrate (like each of the PCI devices > have) > > c.3) You can add extra state for CSS to the ->save_extra_state > handle on virtio devices or to the config > > d) vmstatifying the config is OK as well. > > I should say I'm no virtio expert, so if any of that's truly > mad say so. > > Dave > Agreed (a,b,c,d). Reorganizing my patch set according to a) is going to require some effort, but it should not be too bad. About c.2): I don't think we have any migratable non virtio devices yet, but I'll keep it in mind. I do not understand what you mean by c.3) and extra_sate. Maybe it will settle with time. My idea of extending VirtioCcwDevice is just adding subsections to it's VMStateDescription. The call vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL) in save_config should take care of compatibility. Maybe some staring at virtio-pci is going to help, but right now I can't tell what's the extra_state for, and how/why is it 'extra'. Thanks for your patience! Regards, Halil
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 05/19/2017 07:28 PM, Dr. David Alan Gilbert wrote: > >> To sum it up although I'm currently leaning towards abandoning my idea > >> of two sections for two devices, I'm not comfortable with making the > >> call myself. I'm hoping for some maintainer guidance (s390x, virtio > >> and migration). > > <A couple of hours of reading, the CCW and PCI code, chatting with > > dhildenb etc> > > > > OK, so I think: > > a) First split the series into two separate series; one that > > VMStatifies the existing stuff without breaking compatibility; > > and one that adds the new stuff. Lets get the first of those > > going in while we think about the second. > > > > a.1) I'd do this with patches that convert one chunk into > > vmstate and remove the corresponding C code at the same time. > > > > b) While the way PCI devices are done is weird, I think it'll > > be a lot simpler if you can stick to a structure that's similar > > to them while diverging. It's hard enough for those of us > > who don't do Virtio every day to get our minds around virtio-pci > > without it being different for virtio-ccw/css. > > > > c) To do (b) I suggest: > > c.1) you *don't* add a vmsd to your virtio_ccw_device > > > > c.2) but *do* add a VMSTATE_CCW_DEVICE to the start of any > > non-virtio devices you migrate (like each of the PCI devices > > have) > > > > c.3) You can add extra state for CSS to the ->save_extra_state > > handle on virtio devices or to the config > > > > d) vmstatifying the config is OK as well. > > > > I should say I'm no virtio expert, so if any of that's truly > > mad say so. > > > > Dave > > > > Agreed (a,b,c,d). Reorganizing my patch set according to a) is > going to require some effort, but it should not be too bad. > About c.2): I don't think we have any migratable non virtio devices > yet, but I'll keep it in mind. > > I do not understand what you mean by c.3) and extra_sate. Maybe > it will settle with time. My idea of extending VirtioCcwDevice > is just adding subsections to it's VMStateDescription. The > call vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL) > in save_config should take care of compatibility. Maybe some > staring at virtio-pci is going to help, but right now I can't tell > what's the extra_state for, and how/why is it 'extra'. Yes adding extra subsections into the 'config' is probably fine; but there's also another hook that Jason added, see a6df8adf3, it's an existing subsection at the end of the virtio state that can be linked for transport specific data. Dave > Thanks for your patience! > > Regards, > Halil > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index c2badfe..8ab655c 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -57,6 +57,32 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id) return 0; } +static int get_config_vector(QEMUFile *f, void *pv, size_t size, + VMStateField *field) +{ + VirtioCcwDevice *dev = pv; + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); + + qemu_get_be16s(f, &vdev->config_vector); + return 0; +} + +static int put_config_vector(QEMUFile *f, void *pv, size_t size, + VMStateField *field, QJSON *vmdesc) +{ + VirtioCcwDevice *dev = pv; + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); + + qemu_put_be16(f, vdev->config_vector); + return 0; +} + +const VMStateInfo vmstate_info_config_vector = { + .name = "config_vector", + .get = get_config_vector, + .put = put_config_vector, +}; + const VMStateDescription vmstate_virtio_ccw_dev = { .name = "s390_virtio_ccw_dev", .version_id = 1, @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = { 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. + */ + .name = "virtio/config_vector", + .info = &vmstate_info_config_vector, + }, VMSTATE_STRUCT(routes, VirtioCcwDevice, 1, vmstate_adapter_routes, \ AdapterRoutes), VMSTATE_UINT8(thinint_isc, VirtioCcwDevice), @@ -1272,85 +1306,23 @@ 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); + /* + * We save in legacy mode. The components take care of their own + * compat. representation (based on css_migration_enabled). + */ + 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; + /* + * We load in legacy mode. The components take take care to read + * only stuff which is actually there (based on css_migration_enabled). + */ + return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1); } static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
Let us use the freshly introduced vmstate migration helpers instead of saving/loading the config manually. To achieve this we need to hack the config_vector which is a 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. Still no changes in behavior, but the dead code we added previously is finally awakening to life. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- --- hw/s390x/virtio-ccw.c | 116 +++++++++++++++++++------------------------------- 1 file changed, 44 insertions(+), 72 deletions(-)