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

[Dr. David Alan Gilbert]

* Halil Pasic (address@hidden) 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 / address@hidden / Manchester, UK