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

[Halil Pasic]

On 09/30/2016 04:50 PM, Paolo Bonzini wrote:
> 
> 
> On 30/09/2016 16:19, Halil Pasic wrote:
>> In most cases the functions passed to VMSTATE_VIRTIO_DEVICE
>> only call the virtio_load and virtio_save wrappers. Some include some
>> pre- and post- massaging too. The massaging is better expressed
>> as such in the VMStateDescription.
>>
>> Let us introduce a new macro called VIRTIO_DEF_DEVICE_VMSD and replace
>> VMSTATE_VIRTIO_DEVICE with it gradually.
>>
>> Signed-off-by: Halil Pasic <address@hidden>
>> ---
>>  hw/virtio/virtio.c         | 15 +++++++++++++++
>>  include/hw/virtio/virtio.h | 25 +++++++++++++++++++++++++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 18ce333..ca0a780 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1622,6 +1622,21 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, 
>> size_t size)
>>      virtio_save(VIRTIO_DEVICE(opaque), f);
>>  }
>>  
>> +/* A wrapper for use as a VMState .put function */
>> +void virtio_save_as_vmsi_put(QEMUFile *f, void *opaque, size_t size)
>> +{
>> +    virtio_save(VIRTIO_DEVICE(opaque), f);
>> +}
>> +
>> +/* A wrapper for use as a VMState .get function */
>> +int virtio_load_as_vmsi_get(QEMUFile *f, void *opaque, size_t size)
>> +{
>> +    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
>> +    DeviceClass *dc = DEVICE_CLASS(VIRTIO_DEVICE_GET_CLASS(vdev));
>> +
>> +    return virtio_load(vdev, f, dc->vmsd->version_id);
>> +}
>> +
>>  static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
>>  {
>>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 888c8de..01de49b 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -176,6 +176,31 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
>>  
>>  void virtio_save(VirtIODevice *vdev, QEMUFile *f);
>>  void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size);
>> +void virtio_save_as_vmsi_put(QEMUFile *f, void *opaque, size_t size);
>> +int virtio_load_as_vmsi_get(QEMUFile *f, void *opaque, size_t size);
> 
> These can be called virtio_device_get and virtio_device_put, and can be
> static if...
> 
>> +
>> +#define VMSTATE_VIRTIO_FIELD \
>> +    {                                         \
>> +        .name = "virtio",                     \
>> +        .info = &(const VMStateInfo) {        \
>> +            .name = "virtio",                 \
>> +            .get = virtio_load_as_vmsi_get,   \
>> +            .put = virtio_save_as_vmsi_put,   \
>> +        },                                    \
>> +        .flags = VMS_SINGLE,                  \
>> +    }
> 
> ... you only export the VMStateInfo.
> 

Hi Paolo,

thanks I missed that, you are totally right.

> Also please define the macro like the rest of the VMSTATE macros, i.e.
> 
> #define VMSTATE_PCI_DEVICE(_field, _state) {                           \
>     .name       = (stringify(_field)),                                 \
>     .size       = sizeof(VirtIODevice),                                \
>     .vmsd       = &vmstate_virtio_device,                              \
>     .flags      = VMS_SINGLE,                                          \
>     .offset     = vmstate_offset_value(_state, _field, VirtIODevice),  \
> }
> 
> 

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

Thus I would find using the usual conventions rather misleading here. But
as I said, my understanding may be wrong, so if it is, please correct me.

>> +#define VIRTIO_DEF_DEVICE_VMSD(devname, v, ...) \
>> +    static const VMStateDescription vmstate_virtio_ ## devname = { \
>> +        .name = "virtio-" #devname ,          \
>> +        .minimum_version_id = v,              \
>> +        .version_id = v,                      \
>> +        .fields = (VMStateField[]) {          \
>> +            VMSTATE_VIRTIO_FIELD,             \
>> +            VMSTATE_END_OF_LIST()             \
>> +        },                                    \
>> +        __VA_ARGS__                           \
>> +    };
> 
> and here, don't use the macro, using its expansion everywhere instead.

>From the above I think it is clear, that I would like to keep this macro.
This macro is direct substitute for VMSTATE_VIRTIO_DEVICE, so I really do
not get what is the difference, and why should I use an expansion everywhere
instead, when the previous macro was fine and received no criticism during
the reviews. I think the only thing we gain with the expansion is code
duplication. If you insist on this, could you provide some background, so
I'm also able to understand the why?

Cheers,
Halil

> 
> Paolo
> 
>>  #define VMSTATE_VIRTIO_DEVICE(devname, v, getf, putf) \
>>      static const VMStateDescription vmstate_virtio_ ## devname = { \
>>
>