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 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