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

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

[Michael S. Tsirkin]

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[] = {
-- 
MST

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

[KONRAD Frédéric]

Hi,

I think it is not a good idea, as we wanted to make VirtIODevice 
hot-pluggable for virtio-mmio.

Maybe we can use a callback which is called by virtio-bus before 
plugging, to ensure that this
config size is computed?

On 07/05/2013 12:22, Michael S. Tsirkin wrote:
> 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[] = {

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

[Michael S. Tsirkin]

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.

> 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?

> On 07/05/2013 12:22, Michael S. Tsirkin wrote:
> >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[] = {

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

[KONRAD Frédéric]

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?

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

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.

>> On 07/05/2013 12:22, Michael S. Tsirkin wrote:
>>> 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[] = {

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

[Michael S. Tsirkin]

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.

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

> 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[] = {

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

[KONRAD Frédéric]

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[] = {

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

[Michael S. Tsirkin]

On Tue, May 07, 2013 at 05:29:34PM +0200, KONRAD Frédéric wrote:
> 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.

Going in circles aren't we? If it's not called
it's a bug. If it's called default is not needed.

> 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?

Everywhere. Seriously.
Start with files. File headers should give some hint
as to what's in here
 * VirtioBus
in virtio-bus.h and virtio-bus.c is not helpful.
How about

	VirtioBus - a bus that drives from Newcastle to Edinburgh
	each Tuesday at noon. Drives back on Wednesdays mostly empty.
	It serves as a parent class for all the small taxi cars
	in both of these towns.

or some such.

Go over the interfaces and document what they do.
I mean more than just repeat the name:
	/* Destroy the VirtIODevice */
	void virtio_bus_destroy_device(VirtioBusState *bus)
is not really helpful.

	/* Destroy a device. Assumes you
	   don't have valuables in there.  Calls 911 automatically. */

same for

/* Plug the VirtIODevice */
int virtio_bus_plug_device(VirtIODevice *vdev)

did you mean

	/* Plug the device so the content does not
	   spill. This way you can take put it on the bus.
	   You must use a corkscrew to unplug it later.
	 */


> >>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[] = {