Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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