Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Halil Pasic <pasic@linux.vnet.ibm.com>]

Hi Paolo,

I'm sorry, but I do not get it quite yet, or more exactly I have the
feeling I did not manage to bring my point over. So I will try with
more details.

On 10/03/2016 01:29 PM, Paolo Bonzini wrote:
> 
> 
> On 03/10/2016 12:36, Halil Pasic wrote:
>>>> #define VMSTATE_PCI_DEVICE(_field, _state) {

This was probably supposed to be VMSTATE_VIRTIO_DEVICE.
                           \
>>>>     .name       = (stringify(_field)),                                 \
>>>>     .size       = sizeof(VirtIODevice),                                \
>>>>     .vmsd       = &vmstate_virtio_device,                              \

This one does not exist at least very tricky to write because of the peculiarities
of virtio migration. This one would need to migrate the transport stuff too. And
the specialized device to, which is not normal.

>>>>     .flags      = VMS_SINGLE,                                          \
>>>>     .offset     = vmstate_offset_value(_state, _field, VirtIODevice),  \

This is useless if we keep virtio_save and virtio_load.

>>>> }
>>>>
>>>>
>> I'm not sure if I understand where are you coming from, but if I do, I
>> think I have to disagree here. I think you are coming from the normal 
>> device inheritance direction, where you have the parent storage object
>> (that is its instance data( embedded into the child, and the corresponding
>> parent state is supposed to get migrated as a (vmstate) field of the child.
>>
>> Now if you look at the documentation of virtio migration (or the code) it is
>> pretty obvious that the situation is very different for virtio. Virtio migration
>> is kind of a template method approach (with virtio_load and virtio_save being
>> the template methods), and the parent (core, transport) and the child (device
>> specific) data are intermingled (the device data is in the middle). We can
>> not change this for compatibility reasons (sadly).
> 
> Regarding VMSTATE_VIRTIO_FIELD, it's just a matter of not doing things
> differently for no particular reason.  Your macro is already doing
> exactly the same as other VMSTATE_* macros, just with different
> conventions for the arguments.  I don't see any advantage in changing that.

In my opinion it is not the same. In the case of VMSTATE_PCI_DEVICE we have
(a self contained) parent (in sense of inheritance) device, which is embedded
as _field in the specialized device and is migrated by the vmstatedescription
of the parent. The rest of the specialized devices state is represented by
the other fields.

VMSTATE_VIRTIO_FIELD is however just there to make sure virtio_load and
virtio_save are called at the right time with the right arguments. The specialized
device is then migrated by the save/load callbacks of the device class, or
the vmsd residing in the device class. VMSTATE_VIRTIO_FIELD is supposed
to be the only field, if the virtio device adheres to the virtio-migration
document. VMSTATE_VIRTIO_FIELD has no arguments because it is
a virtual field and does not depend on the offset stuff.

To summarize currently I have no idea how to write up the vmstate
field definition macro VMSTATE_VIRTIO_DEVICE so that it meets your
expectations. 
> 
> Regarding VIRTIO_DEF_DEVICE_VMSD, it's true that virtio migration is
> currently done differently and we will definitely have to do things
> somewhat different due to compatibility, but we can at least evolve
> towards having a normal VMStateDescription (virtio-gpu is already more
> similar to this).  

Virtio-gpu does not adhere to the virtio-migration document because it saves
the specialized device state after the virtio_save is done (that is
after the common virtio subsections). This is however more like the normal
approach -- first you save the parent, then the child.


> Having everything hidden behind a magic macro makes
> things harder to follow than other vmstate definitions.  

Again in my opinion the virtio migration is different that the rest of the
vmstate migration stuff, and it's ugly to some extent. So the idea was
to make it look different and hide the ugliness behind the macro. 
If you insist i will create a version where the macros are expanded so
it's easier to say if this improves or worsens the readability.

> It's okay when
> you have a very small veneer as in commit 5943124cc, but in my opinion
> having field definitions inside macro arguments is already too much.

I think this is matter of taste, and your taste matters more ;). I do 
agree that the variadic for the massaging functions is more complicated
that the two function pointers taken by VMSTATE_VIRTIO_DEVICE. My idea
was that we end up with more readable code on the caller-side, but if you
prefer function pointers and NULLs if no callback is needed needed 
(most cases), I can live with that.

Sorry again for my thick head. 

Cheers,
Halil