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

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@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). 
> >  > 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, _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 / dgilb...@redhat.com / Manchester, UK

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

[Halil Pasic]

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). 
>  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, _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

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

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> 
> 
> On 05/15/2017 09:07 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> >>
> >>
> >> On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote:
> >>> * Halil Pasic (pa...@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
> 

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

[Halil Pasic]

On 05/15/2017 09:07 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
>>
>>
>> On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote:
>>> * Halil Pasic (pa...@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 

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

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> 
> 
> On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pa...@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 = _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 

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

[Halil Pasic]

On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pa...@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 = _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 

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

[Cornelia Huck]

On Tue, 9 May 2017 19:05:28 +0200
Halil Pasic  wrote:

> From e92135590ab95cc565b37913de77a9ed17012933 Mon Sep 17 00:00:00 2001
> From: Halil Pasic 
> Date: Tue, 9 May 2017 16:01:50 +0200
> Subject: [PATCH 1/2] virtio-ccw: replace info with VMSTATE_WITH_TMP
> 
> Convert s VMSatateInfo based solution manipulating the migration stream
> directly to VMSTATE_WITH_TMP witch keeps IO and transformation logic
> separate. 
> 
> 
> Signed-off-by: Halil Pasic 
> ---
>  hw/s390x/virtio-ccw.c | 40 +++-
>  1 file changed, 23 insertions(+), 17 deletions(-)

FWIW: Looks reasonable to me.

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

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> 
> 
> On 05/08/2017 06:55 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pa...@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 
> >> ---
> >> ---
> >>  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(>bus);
> >> +
> >> +qemu_get_be16s(f, >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(>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).
> > 
> 
> Yeah this one can be done with WITH_TMP. Below is the patch on top of
> this patch set. It's a bit more verbose (+6 lines) but it looks a bit
> nicer and probably also safer in (terms of symmetric read and write).
> 
> If you think its the way to go I will squash it into this patch for
> the next version.

Yes, I prefer that to using qemu_put/qemu_get - I'm trying to avoid
all new uses of those except where we can't avoid them.

(There's still the separate question of whether reworking a virtio
device migration to be unlike the other virtio devices is right,
but that's separate to whether we should avoid qemu_get/put)

Dave

> -8<-
> 
> 
> From e92135590ab95cc565b37913de77a9ed17012933 Mon Sep 17 00:00:00 2001
> From: Halil Pasic 
> Date: Tue, 9 May 2017 16:01:50 +0200
> Subject: [PATCH 1/2] virtio-ccw: replace info with VMSTATE_WITH_TMP
> 
> Convert s VMSatateInfo based solution manipulating the migration stream
> directly to VMSTATE_WITH_TMP witch keeps IO and transformation logic
> separate. 
> 
> 
> Signed-off-by: Halil Pasic 
> ---
>  hw/s390x/virtio-ccw.c | 40 +++-
>  1 file changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index c611b6f..6ebc78a 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -57,30 +57,38 @@ 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)
> +typedef struct VirtioCcwDeviceTmp {
> +VirtioCcwDevice *parent;
> +uint16_t config_vector;
> +} VirtioCcwDeviceTmp;
> +
> +static void virtio_ccw_dev_tmp_pre_save(void *opaque)
>  {
> -VirtioCcwDevice *dev = pv;
> +VirtioCcwDeviceTmp *tmp = opaque;
> +VirtioCcwDevice *dev = tmp->parent;
>  VirtIODevice *vdev = virtio_bus_get_device(>bus);
>  
> -qemu_get_be16s(f, >config_vector);
> -return 0;
> +tmp->config_vector = vdev->config_vector;
>  }
>  
> -static int put_config_vector(QEMUFile *f, void *pv, size_t size,
> - VMStateField *field, QJSON *vmdesc)
> +static int virtio_ccw_dev_tmp_post_load(void *opaque, int version_id)
>  {
> -VirtioCcwDevice *dev = pv;
> +VirtioCcwDeviceTmp *tmp = opaque;
> +VirtioCcwDevice *dev = tmp->parent;
>  VirtIODevice *vdev = virtio_bus_get_device(>bus);
>  
> -qemu_put_be16(f, vdev->config_vector);
> +vdev->config_vector = tmp->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_tmp = {
> +.name = "s390_virtio_ccw_dev_tmp",
> +.pre_save = virtio_ccw_dev_tmp_pre_save,
> +.post_load 

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

[Halil Pasic]

On 05/08/2017 06:55 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pa...@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 
>> ---
>> ---
>>  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(>bus);
>> +
>> +qemu_get_be16s(f, >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(>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).
> 

Yeah this one can be done with WITH_TMP. Below is the patch on top of
this patch set. It's a bit more verbose (+6 lines) but it looks a bit
nicer and probably also safer in (terms of symmetric read and write).

If you think its the way to go I will squash it into this patch for
the next version.

-8<-


>From e92135590ab95cc565b37913de77a9ed17012933 Mon Sep 17 00:00:00 2001
From: Halil Pasic 
Date: Tue, 9 May 2017 16:01:50 +0200
Subject: [PATCH 1/2] virtio-ccw: replace info with VMSTATE_WITH_TMP

Convert s VMSatateInfo based solution manipulating the migration stream
directly to VMSTATE_WITH_TMP witch keeps IO and transformation logic
separate. 


Signed-off-by: Halil Pasic 
---
 hw/s390x/virtio-ccw.c | 40 +++-
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index c611b6f..6ebc78a 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -57,30 +57,38 @@ 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)
+typedef struct VirtioCcwDeviceTmp {
+VirtioCcwDevice *parent;
+uint16_t config_vector;
+} VirtioCcwDeviceTmp;
+
+static void virtio_ccw_dev_tmp_pre_save(void *opaque)
 {
-VirtioCcwDevice *dev = pv;
+VirtioCcwDeviceTmp *tmp = opaque;
+VirtioCcwDevice *dev = tmp->parent;
 VirtIODevice *vdev = virtio_bus_get_device(>bus);
 
-qemu_get_be16s(f, >config_vector);
-return 0;
+tmp->config_vector = vdev->config_vector;
 }
 
-static int put_config_vector(QEMUFile *f, void *pv, size_t size,
- VMStateField *field, QJSON *vmdesc)
+static int virtio_ccw_dev_tmp_post_load(void *opaque, int version_id)
 {
-VirtioCcwDevice *dev = pv;
+VirtioCcwDeviceTmp *tmp = opaque;
+VirtioCcwDevice *dev = tmp->parent;
 VirtIODevice *vdev = virtio_bus_get_device(>bus);
 
-qemu_put_be16(f, vdev->config_vector);
+vdev->config_vector = tmp->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_tmp = {
+.name = "s390_virtio_ccw_dev_tmp",
+.pre_save = virtio_ccw_dev_tmp_pre_save,
+.post_load = virtio_ccw_dev_tmp_post_load,
+.fields = (VMStateField[]) {
+VMSTATE_UINT16(config_vector, VirtioCcwDeviceTmp),
+VMSTATE_END_OF_LIST()
+}
 };
 
 const VMStateDescription vmstate_virtio_ccw_dev = {
@@ -93,14 +101,12 @@ 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 

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

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@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 = _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 / dgilb...@redhat.com / Manchester, UK

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

[Halil Pasic]

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

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

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> 
> 
> On 05/08/2017 07:36 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pa...@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 
> >> ---
> >> ---
> >>  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(>bus);
> >> +
> >> +qemu_get_be16s(f, >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(>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 = _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 {
> >> 

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

[Halil Pasic]

On 05/08/2017 07:36 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pa...@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 
>> ---
>> ---
>>  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(>bus);
>> +
>> +qemu_get_be16s(f, >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(>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 = _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, _virtio_ccw_dev, dev, NULL);

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

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@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 
> ---
> ---
>  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(>bus);
> +
> +qemu_get_be16s(f, >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(>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 = _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, _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 = 

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

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@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 
> ---
> ---
>  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(>bus);
> +
> +qemu_get_be16s(f, >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(>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 = _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, _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);
> -

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

[Halil Pasic]

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 
---
---
 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(>bus);
+
+qemu_get_be16s(f, >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(>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 = _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, _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,