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/15/2017 09:07 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (address@hidden) wrote:
> >>
> >>
> >> On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote:
> >>> * Halil Pasic (address@hidden) 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 / address@hidden / Manchester, UK