Re: [Qemu-devel] [PATCH] virtio-net: require that config size is initialized

["KONRAD Frédéric" <fred.konrad@greensocs.com>]

On 07/05/2013 16:00, Michael S. Tsirkin wrote:
> On Tue, May 07, 2013 at 02:54:38PM +0200, KONRAD Frédéric wrote:
>> On 07/05/2013 14:33, Michael S. Tsirkin wrote:
>>> On Tue, May 07, 2013 at 02:29:38PM +0200, KONRAD Frédéric wrote:
>>>> Hi,
>>>>
>>>> I think it is not a good idea, as we wanted to make VirtIODevice
>>>> hot-pluggable for virtio-mmio.
>>> And then this hack will break cross version migration.
>> Why?
>>
>> virtio_net_set_config_size is called by each "proxy" devices no?
> If it's called then there's no need for a default,
> so this patch should be fine.

True but as I told you, we made this refactoring to be able to hot-plug
VirtIODevice on a virtio-bus, so calling this function won't be possible.

But you are right this must be fixed.

>
>>>> Maybe we can use a callback which is called by virtio-bus before
>>>> plugging, to ensure that this
>>>> config size is computed?
>>> OK, will you post such a patch?
>>>
>> The issue is as we said in the last thread, host_feature is part of
>> the proxy.
> Maybe you could add documentation on how initialization works?
> If I could fiure it out I would maybe suggest some solutions.

Yes, where do you think I can add this?

>
>> And if we want to move it to the devices, we must have a kind of
>> property forwarding mechanism.
>>
>> Anthony said he had something about that.
>>>>> All buses do this, and if they don't they get subtle
>>>>> bugs related to cross version migration.
>>>>> Let's add an assert to catch these bugs early.
>>>>>
>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> ---
>>>>>
>>>>> Just a cleanup so not 1.5 material.
>>>>> Seems to work fine here - any opinions?
>>>>>
>>>>>   hw/net/virtio-net.c | 7 ++++---
>>>>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>> index 908e7b8..f0a9272 100644
>>>>> --- a/hw/net/virtio-net.c
>>>>> +++ b/hw/net/virtio-net.c
>>>>> @@ -1282,6 +1282,8 @@ static int virtio_net_device_init(VirtIODevice *vdev)
>>>>>       DeviceState *qdev = DEVICE(vdev);
>>>>>       VirtIONet *n = VIRTIO_NET(vdev);
>>>>> +    /* Verify that config size has been initialized */
>>>>> +    assert(n->config_size != (size_t)-1);
>>>>>       virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
>>>>>                                     n->config_size);
>>>>> @@ -1386,10 +1388,9 @@ static void virtio_net_instance_init(Object *obj)
>>>>>       VirtIONet *n = VIRTIO_NET(obj);
>>>>>       /*
>>>>> -     * The default config_size is sizeof(struct virtio_net_config).
>>>>> -     * Can be overriden with virtio_net_set_config_size.
>>>>> +     * The config_size must be set later with virtio_net_set_config_size.
>>>>>        */
>>>>> -    n->config_size = sizeof(struct virtio_net_config);
>>>>> +    n->config_size = (size_t)-1;
>>>>>   }
>>>>>   static Property virtio_net_properties[] = {