[Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Jason Wang]

Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
calculate config size based on the host features. But it forgets the
VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
disabled form command line. Then qemu will crash when user tries to read the
config of virtio-net.

Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
the mac address.

Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/virtio-net.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 70c8fce..33a70ef 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
 
 void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
 {
-    int i, config_size = 0;
+    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
+    int i, config_size = feature_sizes[0].end;
     for (i = 0; feature_sizes[i].flags != 0; i++) {
         if (host_features & feature_sizes[i].flags) {
             config_size = MAX(feature_sizes[i].end, config_size);
-- 
1.7.1

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Michael S. Tsirkin]

On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
> Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
> calculate config size based on the host features. But it forgets the
> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
> len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
> disabled form command line. Then qemu will crash when user tries to read the
> config of virtio-net.
> 
> Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
> the mac address.
> 
> Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/virtio-net.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 70c8fce..33a70ef 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>  
>  void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>  {
> -    int i, config_size = 0;
> +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
> +    int i, config_size = feature_sizes[0].end;

This would be cleaner:
	host_features |= (1 << VIRTIO_NET_F_MAC);

no need for a comment then.

>      for (i = 0; feature_sizes[i].flags != 0; i++) {
>          if (host_features & feature_sizes[i].flags) {
>              config_size = MAX(feature_sizes[i].end, config_size);
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Jason Wang]

On 04/25/2013 02:59 PM, Michael S. Tsirkin wrote:
> On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
>> Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>> calculate config size based on the host features. But it forgets the
>> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>> len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>> disabled form command line. Then qemu will crash when user tries to read the
>> config of virtio-net.
>>
>> Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>> the mac address.
>>
>> Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  hw/net/virtio-net.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 70c8fce..33a70ef 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>  
>>  void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>>  {
>> -    int i, config_size = 0;
>> +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>> +    int i, config_size = feature_sizes[0].end;
> This would be cleaner:
> 	host_features |= (1 << VIRTIO_NET_F_MAC);
>
> no need for a comment then.

Sure, will send V2.
>>      for (i = 0; feature_sizes[i].flags != 0; i++) {
>>          if (host_features & feature_sizes[i].flags) {
>>              config_size = MAX(feature_sizes[i].end, config_size);
>> -- 
>> 1.7.1

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Michael S. Tsirkin]

On Thu, Apr 25, 2013 at 03:02:44PM +0800, Jason Wang wrote:
> On 04/25/2013 02:59 PM, Michael S. Tsirkin wrote:
> > On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
> >> Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
> >> calculate config size based on the host features. But it forgets the
> >> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
> >> len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
> >> disabled form command line. Then qemu will crash when user tries to read the
> >> config of virtio-net.
> >>
> >> Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
> >> the mac address.
> >>
> >> Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> ---
> >>  hw/net/virtio-net.c |    3 ++-
> >>  1 files changed, 2 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index 70c8fce..33a70ef 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
> >>  
> >>  void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
> >>  {
> >> -    int i, config_size = 0;
> >> +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
> >> +    int i, config_size = feature_sizes[0].end;
> > This would be cleaner:
> > 	host_features |= (1 << VIRTIO_NET_F_MAC);
> >
> > no need for a comment then.
> 
> Sure, will send V2.

Maybe ass assert(config_size) in core just to make sure
other devices do not have this bug.

> >>      for (i = 0; feature_sizes[i].flags != 0; i++) {
> >>          if (host_features & feature_sizes[i].flags) {
> >>              config_size = MAX(feature_sizes[i].end, config_size);
> >> -- 
> >> 1.7.1

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Jason Wang]

On 04/25/2013 03:06 PM, Michael S. Tsirkin wrote:
> On Thu, Apr 25, 2013 at 03:02:44PM +0800, Jason Wang wrote:
>> On 04/25/2013 02:59 PM, Michael S. Tsirkin wrote:
>>> On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
>>>> Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>>>> calculate config size based on the host features. But it forgets the
>>>> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>>>> len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>>>> disabled form command line. Then qemu will crash when user tries to read the
>>>> config of virtio-net.
>>>>
>>>> Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>>>> the mac address.
>>>>
>>>> Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>>  hw/net/virtio-net.c |    3 ++-
>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index 70c8fce..33a70ef 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>>>  
>>>>  void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>>>>  {
>>>> -    int i, config_size = 0;
>>>> +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>>>> +    int i, config_size = feature_sizes[0].end;
>>> This would be cleaner:
>>> 	host_features |= (1 << VIRTIO_NET_F_MAC);
>>>
>>> no need for a comment then.
>> Sure, will send V2.
> Maybe ass assert(config_size) in core just to make sure
> other devices do not have this bug.

Prepare a patch for this, will send soon.
>>>>      for (i = 0; feature_sizes[i].flags != 0; i++) {
>>>>          if (host_features & feature_sizes[i].flags) {
>>>>              config_size = MAX(feature_sizes[i].end, config_size);
>>>> -- 
>>>> 1.7.1

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Jesse Larrew]

On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
> On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
>> Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>> calculate config size based on the host features. But it forgets the
>> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>> len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>> disabled form command line. Then qemu will crash when user tries to read the
>> config of virtio-net.
>>
>> Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>> the mac address.
>>
>> Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  hw/net/virtio-net.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 70c8fce..33a70ef 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>  
>>  void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>>  {
>> -    int i, config_size = 0;
>> +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>> +    int i, config_size = feature_sizes[0].end;
> 
> This would be cleaner:
> 	host_features |= (1 << VIRTIO_NET_F_MAC);
> 
> no need for a comment then.
> 

It seems to me that the real problem here is that host_features isn't
properly populated before virtio_net_set_config_size() is called. Looking
at virtio_device_init(), we can see why:

static int virtio_device_init(DeviceState *qdev)
{
    VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
    assert(k->init != NULL);
    if (k->init(vdev) < 0) {
        return -1;
    }
    virtio_bus_plug_device(vdev);
    return 0;
}

virtio_net_set_config_size() is currently being called as part of the
k->init call, but the host_features aren't properly setup until the device
is plugged into the bus using virtio_bus_plug_device().

After talking with mdroth, I think the proper way to fix this would be to
extend VirtioDeviceClass to include a calculate_config_size() method that
can be called at the appropriate time during device initialization like so:

static int virtio_device_init(DeviceState *qdev)
{
    VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
    assert(k->init != NULL);
    if (k->init(vdev) < 0) {
        return -1;
    }
    virtio_bus_plug_device(vdev);
+   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
+       return -1;
+   }
    return 0;
}

This would ensure that host_features contains the proper data before any
devices try to make use of it to calculate the config size.

>>      for (i = 0; feature_sizes[i].flags != 0; i++) {
>>          if (host_features & feature_sizes[i].flags) {
>>              config_size = MAX(feature_sizes[i].end, config_size);
>> -- 
>> 1.7.1
> 

Jesse Larrew
Software Engineer, KVM Team
IBM Linux Technology Center
Phone: (512) 973-2052 (T/L: 363-2052)
jlarrew@linux.vnet.ibm.com

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[KONRAD Frédéric]

On 29/04/2013 16:42, Jesse Larrew wrote:
> On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
>> On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
>>> Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>>> calculate config size based on the host features. But it forgets the
>>> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>>> len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>>> disabled form command line. Then qemu will crash when user tries to read the
>>> config of virtio-net.
>>>
>>> Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>>> the mac address.
>>>
>>> Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> ---
>>>   hw/net/virtio-net.c |    3 ++-
>>>   1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index 70c8fce..33a70ef 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>>   
>>>   void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>>>   {
>>> -    int i, config_size = 0;
>>> +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>>> +    int i, config_size = feature_sizes[0].end;
>> This would be cleaner:
>> 	host_features |= (1 << VIRTIO_NET_F_MAC);
>>
>> no need for a comment then.
>>
> It seems to me that the real problem here is that host_features isn't
> properly populated before virtio_net_set_config_size() is called. Looking
> at virtio_device_init(), we can see why:
>
> static int virtio_device_init(DeviceState *qdev)
> {
>      VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>      assert(k->init != NULL);
>      if (k->init(vdev) < 0) {
>          return -1;
>      }
>      virtio_bus_plug_device(vdev);
>      return 0;
> }
>
> virtio_net_set_config_size() is currently being called as part of the
> k->init call, but the host_features aren't properly setup until the device
> is plugged into the bus using virtio_bus_plug_device().
>
> After talking with mdroth, I think the proper way to fix this would be to
> extend VirtioDeviceClass to include a calculate_config_size() method that
> can be called at the appropriate time during device initialization like so:
>
> static int virtio_device_init(DeviceState *qdev)
> {
>      VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>      assert(k->init != NULL);
>      if (k->init(vdev) < 0) {
>          return -1;
>      }
>      virtio_bus_plug_device(vdev);
> +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
> +       return -1;
> +   }
>      return 0;
> }
>
> This would ensure that host_features contains the proper data before any
> devices try to make use of it to calculate the config size.
Good point, I didn't saw that.

but this was not the case with commit 14f9b664 no?

>
>>>       for (i = 0; feature_sizes[i].flags != 0; i++) {
>>>           if (host_features & feature_sizes[i].flags) {
>>>               config_size = MAX(feature_sizes[i].end, config_size);
>>> -- 
>>> 1.7.1
> Jesse Larrew
> Software Engineer, KVM Team
> IBM Linux Technology Center
> Phone: (512) 973-2052 (T/L: 363-2052)
> jlarrew@linux.vnet.ibm.com
>
>

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Jesse Larrew]

On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
> On 29/04/2013 16:42, Jesse Larrew wrote:
>> On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
>>> On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
>>>> Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>>>> calculate config size based on the host features. But it forgets the
>>>> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>>>> len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>>>> disabled form command line. Then qemu will crash when user tries to read the
>>>> config of virtio-net.
>>>>
>>>> Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>>>> the mac address.
>>>>
>>>> Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>>   hw/net/virtio-net.c |    3 ++-
>>>>   1 files changed, 2 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index 70c8fce..33a70ef 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>>>     void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>>>>   {
>>>> -    int i, config_size = 0;
>>>> +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>>>> +    int i, config_size = feature_sizes[0].end;
>>> This would be cleaner:
>>>     host_features |= (1 << VIRTIO_NET_F_MAC);
>>>
>>> no need for a comment then.
>>>
>> It seems to me that the real problem here is that host_features isn't
>> properly populated before virtio_net_set_config_size() is called. Looking
>> at virtio_device_init(), we can see why:
>>
>> static int virtio_device_init(DeviceState *qdev)
>> {
>>      VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>      assert(k->init != NULL);
>>      if (k->init(vdev) < 0) {
>>          return -1;
>>      }
>>      virtio_bus_plug_device(vdev);
>>      return 0;
>> }
>>
>> virtio_net_set_config_size() is currently being called as part of the
>> k->init call, but the host_features aren't properly setup until the device
>> is plugged into the bus using virtio_bus_plug_device().
>>
>> After talking with mdroth, I think the proper way to fix this would be to
>> extend VirtioDeviceClass to include a calculate_config_size() method that
>> can be called at the appropriate time during device initialization like so:
>>
>> static int virtio_device_init(DeviceState *qdev)
>> {
>>      VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>      assert(k->init != NULL);
>>      if (k->init(vdev) < 0) {
>>          return -1;
>>      }
>>      virtio_bus_plug_device(vdev);
>> +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
>> +       return -1;
>> +   }
>>      return 0;
>> }
>>
>> This would ensure that host_features contains the proper data before any
>> devices try to make use of it to calculate the config size.
> Good point, I didn't saw that.
> 
> but this was not the case with commit 14f9b664 no?
> 

I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
it yet. I'll confirm this afternoon.

>>
>>>>       for (i = 0; feature_sizes[i].flags != 0; i++) {
>>>>           if (host_features & feature_sizes[i].flags) {
>>>>               config_size = MAX(feature_sizes[i].end, config_size);
>>>> -- 
>>>> 1.7.1

Jesse Larrew
Software Engineer, KVM Team
IBM Linux Technology Center
Phone: (512) 973-2052 (T/L: 363-2052)
jlarrew@linux.vnet.ibm.com

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[KONRAD Frédéric]

On 29/04/2013 17:14, Jesse Larrew wrote:
> On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
>> On 29/04/2013 16:42, Jesse Larrew wrote:
>>> On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
>>>> On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
>>>>> Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>>>>> calculate config size based on the host features. But it forgets the
>>>>> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>>>>> len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>>>>> disabled form command line. Then qemu will crash when user tries to read the
>>>>> config of virtio-net.
>>>>>
>>>>> Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>>>>> the mac address.
>>>>>
>>>>> Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>> ---
>>>>>    hw/net/virtio-net.c |    3 ++-
>>>>>    1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>> index 70c8fce..33a70ef 100644
>>>>> --- a/hw/net/virtio-net.c
>>>>> +++ b/hw/net/virtio-net.c
>>>>> @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>>>>      void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>>>>>    {
>>>>> -    int i, config_size = 0;
>>>>> +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>>>>> +    int i, config_size = feature_sizes[0].end;
>>>> This would be cleaner:
>>>>      host_features |= (1 << VIRTIO_NET_F_MAC);
>>>>
>>>> no need for a comment then.
>>>>
>>> It seems to me that the real problem here is that host_features isn't
>>> properly populated before virtio_net_set_config_size() is called. Looking
>>> at virtio_device_init(), we can see why:
>>>
>>> static int virtio_device_init(DeviceState *qdev)
>>> {
>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>       assert(k->init != NULL);
>>>       if (k->init(vdev) < 0) {
>>>           return -1;
>>>       }
>>>       virtio_bus_plug_device(vdev);
>>>       return 0;
>>> }
>>>
>>> virtio_net_set_config_size() is currently being called as part of the
>>> k->init call, but the host_features aren't properly setup until the device
>>> is plugged into the bus using virtio_bus_plug_device().
>>>
>>> After talking with mdroth, I think the proper way to fix this would be to
>>> extend VirtioDeviceClass to include a calculate_config_size() method that
>>> can be called at the appropriate time during device initialization like so:
>>>
>>> static int virtio_device_init(DeviceState *qdev)
>>> {
>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>       assert(k->init != NULL);
>>>       if (k->init(vdev) < 0) {
>>>           return -1;
>>>       }
>>>       virtio_bus_plug_device(vdev);
>>> +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
>>> +       return -1;
>>> +   }
>>>       return 0;
>>> }
>>>
>>> This would ensure that host_features contains the proper data before any
>>> devices try to make use of it to calculate the config size.
>> Good point, I didn't saw that.
>>
>> but this was not the case with commit 14f9b664 no?
>>
> I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
> it yet. I'll confirm this afternoon.

Ok, I think there is a problem with your patch:

     virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
                                   n->config_size);

is called in virtio_net_device_init (k->init).

Is there a way to resize the config after that?

>
>>>>>        for (i = 0; feature_sizes[i].flags != 0; i++) {
>>>>>            if (host_features & feature_sizes[i].flags) {
>>>>>                config_size = MAX(feature_sizes[i].end, config_size);
>>>>> -- 
>>>>> 1.7.1
> Jesse Larrew
> Software Engineer, KVM Team
> IBM Linux Technology Center
> Phone: (512) 973-2052 (T/L: 363-2052)
> jlarrew@linux.vnet.ibm.com
>

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Jesse Larrew]

On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
> On 29/04/2013 17:14, Jesse Larrew wrote:
>> On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
>>> On 29/04/2013 16:42, Jesse Larrew wrote:
>>>> On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
>>>>> On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
>>>>>> Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>>>>>> calculate config size based on the host features. But it forgets the
>>>>>> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>>>>>> len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>>>>>> disabled form command line. Then qemu will crash when user tries to read the
>>>>>> config of virtio-net.
>>>>>>
>>>>>> Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>>>>>> the mac address.
>>>>>>
>>>>>> Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>> ---
>>>>>>    hw/net/virtio-net.c |    3 ++-
>>>>>>    1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>> index 70c8fce..33a70ef 100644
>>>>>> --- a/hw/net/virtio-net.c
>>>>>> +++ b/hw/net/virtio-net.c
>>>>>> @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>>>>>      void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>>>>>>    {
>>>>>> -    int i, config_size = 0;
>>>>>> +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>>>>>> +    int i, config_size = feature_sizes[0].end;
>>>>> This would be cleaner:
>>>>>      host_features |= (1 << VIRTIO_NET_F_MAC);
>>>>>
>>>>> no need for a comment then.
>>>>>
>>>> It seems to me that the real problem here is that host_features isn't
>>>> properly populated before virtio_net_set_config_size() is called. Looking
>>>> at virtio_device_init(), we can see why:
>>>>
>>>> static int virtio_device_init(DeviceState *qdev)
>>>> {
>>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>>       assert(k->init != NULL);
>>>>       if (k->init(vdev) < 0) {
>>>>           return -1;
>>>>       }
>>>>       virtio_bus_plug_device(vdev);
>>>>       return 0;
>>>> }
>>>>
>>>> virtio_net_set_config_size() is currently being called as part of the
>>>> k->init call, but the host_features aren't properly setup until the device
>>>> is plugged into the bus using virtio_bus_plug_device().
>>>>
>>>> After talking with mdroth, I think the proper way to fix this would be to
>>>> extend VirtioDeviceClass to include a calculate_config_size() method that
>>>> can be called at the appropriate time during device initialization like so:
>>>>
>>>> static int virtio_device_init(DeviceState *qdev)
>>>> {
>>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>>       assert(k->init != NULL);
>>>>       if (k->init(vdev) < 0) {
>>>>           return -1;
>>>>       }
>>>>       virtio_bus_plug_device(vdev);
>>>> +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
>>>> +       return -1;
>>>> +   }
>>>>       return 0;
>>>> }
>>>>
>>>> This would ensure that host_features contains the proper data before any
>>>> devices try to make use of it to calculate the config size.
>>> Good point, I didn't saw that.
>>>
>>> but this was not the case with commit 14f9b664 no?
>>>
>> I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
>> it yet. I'll confirm this afternoon.
> 
> Ok, I think there is a problem with your patch:
> 
>     virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
>                                   n->config_size);
> 
> is called in virtio_net_device_init (k->init).
> 
> Is there a way to resize the config after that?
>

Nope. That's definitely a bug. I can send a patch to fix this today. Any
objection to the method suggested above (extending VirtioDeviceClass)?

>>
>>>>>>        for (i = 0; feature_sizes[i].flags != 0; i++) {
>>>>>>            if (host_features & feature_sizes[i].flags) {
>>>>>>                config_size = MAX(feature_sizes[i].end, config_size);
>>>>>> -- 
>>>>>> 1.7.1
>> Jesse Larrew
>> Software Engineer, KVM Team
>> IBM Linux Technology Center
>> Phone: (512) 973-2052 (T/L: 363-2052)
>> jlarrew@linux.vnet.ibm.com
>>
> 


-- 

Jesse Larrew
Software Engineer, KVM Team
IBM Linux Technology Center
Phone: (512) 973-2052 (T/L: 363-2052)
jlarrew@linux.vnet.ibm.com

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Michael S. Tsirkin]

On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
> On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
> > On 29/04/2013 17:14, Jesse Larrew wrote:
> >> On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
> >>> On 29/04/2013 16:42, Jesse Larrew wrote:
> >>>> On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
> >>>>> On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
> >>>>>> Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
> >>>>>> calculate config size based on the host features. But it forgets the
> >>>>>> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
> >>>>>> len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
> >>>>>> disabled form command line. Then qemu will crash when user tries to read the
> >>>>>> config of virtio-net.
> >>>>>>
> >>>>>> Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
> >>>>>> the mac address.
> >>>>>>
> >>>>>> Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
> >>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>>>> ---
> >>>>>>    hw/net/virtio-net.c |    3 ++-
> >>>>>>    1 files changed, 2 insertions(+), 1 deletions(-)
> >>>>>>
> >>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>>>> index 70c8fce..33a70ef 100644
> >>>>>> --- a/hw/net/virtio-net.c
> >>>>>> +++ b/hw/net/virtio-net.c
> >>>>>> @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
> >>>>>>      void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
> >>>>>>    {
> >>>>>> -    int i, config_size = 0;
> >>>>>> +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
> >>>>>> +    int i, config_size = feature_sizes[0].end;
> >>>>> This would be cleaner:
> >>>>>      host_features |= (1 << VIRTIO_NET_F_MAC);
> >>>>>
> >>>>> no need for a comment then.
> >>>>>
> >>>> It seems to me that the real problem here is that host_features isn't
> >>>> properly populated before virtio_net_set_config_size() is called. Looking
> >>>> at virtio_device_init(), we can see why:
> >>>>
> >>>> static int virtio_device_init(DeviceState *qdev)
> >>>> {
> >>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> >>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> >>>>       assert(k->init != NULL);
> >>>>       if (k->init(vdev) < 0) {
> >>>>           return -1;
> >>>>       }
> >>>>       virtio_bus_plug_device(vdev);
> >>>>       return 0;
> >>>> }
> >>>>
> >>>> virtio_net_set_config_size() is currently being called as part of the
> >>>> k->init call, but the host_features aren't properly setup until the device
> >>>> is plugged into the bus using virtio_bus_plug_device().
> >>>>
> >>>> After talking with mdroth, I think the proper way to fix this would be to
> >>>> extend VirtioDeviceClass to include a calculate_config_size() method that
> >>>> can be called at the appropriate time during device initialization like so:
> >>>>
> >>>> static int virtio_device_init(DeviceState *qdev)
> >>>> {
> >>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> >>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> >>>>       assert(k->init != NULL);
> >>>>       if (k->init(vdev) < 0) {
> >>>>           return -1;
> >>>>       }
> >>>>       virtio_bus_plug_device(vdev);
> >>>> +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
> >>>> +       return -1;
> >>>> +   }
> >>>>       return 0;
> >>>> }
> >>>>
> >>>> This would ensure that host_features contains the proper data before any
> >>>> devices try to make use of it to calculate the config size.
> >>> Good point, I didn't saw that.
> >>>
> >>> but this was not the case with commit 14f9b664 no?
> >>>
> >> I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
> >> it yet. I'll confirm this afternoon.
> > 
> > Ok, I think there is a problem with your patch:
> > 
> >     virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
> >                                   n->config_size);
> > 
> > is called in virtio_net_device_init (k->init).
> > 
> > Is there a way to resize the config after that?
> >
> 
> Nope. That's definitely a bug. I can send a patch to fix this today. Any
> objection to the method suggested above (extending VirtioDeviceClass)?

This needs more thought. All devices expected everything
is setup during the init call. config size is
likely not the only issue.

So we need almost all of virtio_bus_plug_device before init,
and then after init send the signal:

    if (klass->device_plugged != NULL) {
        klass->device_plugged(qbus->parent);
    }



> >>
> >>>>>>        for (i = 0; feature_sizes[i].flags != 0; i++) {
> >>>>>>            if (host_features & feature_sizes[i].flags) {
> >>>>>>                config_size = MAX(feature_sizes[i].end, config_size);
> >>>>>> -- 
> >>>>>> 1.7.1
> >> Jesse Larrew
> >> Software Engineer, KVM Team
> >> IBM Linux Technology Center
> >> Phone: (512) 973-2052 (T/L: 363-2052)
> >> jlarrew@linux.vnet.ibm.com
> >>
> > 
> 
> 
> -- 
> 
> Jesse Larrew
> Software Engineer, KVM Team
> IBM Linux Technology Center
> Phone: (512) 973-2052 (T/L: 363-2052)
> jlarrew@linux.vnet.ibm.com

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[KONRAD Frédéric]

On 29/04/2013 18:02, Michael S. Tsirkin wrote:
> On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
>> On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
>>> On 29/04/2013 17:14, Jesse Larrew wrote:
>>>> On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
>>>>> On 29/04/2013 16:42, Jesse Larrew wrote:
>>>>>> On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
>>>>>>> On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
>>>>>>>> Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>>>>>>>> calculate config size based on the host features. But it forgets the
>>>>>>>> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>>>>>>>> len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>>>>>>>> disabled form command line. Then qemu will crash when user tries to read the
>>>>>>>> config of virtio-net.
>>>>>>>>
>>>>>>>> Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>>>>>>>> the mac address.
>>>>>>>>
>>>>>>>> Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>>>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>>>> ---
>>>>>>>>     hw/net/virtio-net.c |    3 ++-
>>>>>>>>     1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>>>> index 70c8fce..33a70ef 100644
>>>>>>>> --- a/hw/net/virtio-net.c
>>>>>>>> +++ b/hw/net/virtio-net.c
>>>>>>>> @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>>>>>>>       void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>>>>>>>>     {
>>>>>>>> -    int i, config_size = 0;
>>>>>>>> +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>>>>>>>> +    int i, config_size = feature_sizes[0].end;
>>>>>>> This would be cleaner:
>>>>>>>       host_features |= (1 << VIRTIO_NET_F_MAC);
>>>>>>>
>>>>>>> no need for a comment then.
>>>>>>>
>>>>>> It seems to me that the real problem here is that host_features isn't
>>>>>> properly populated before virtio_net_set_config_size() is called. Looking
>>>>>> at virtio_device_init(), we can see why:
>>>>>>
>>>>>> static int virtio_device_init(DeviceState *qdev)
>>>>>> {
>>>>>>        VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>>>>        VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>>>>        assert(k->init != NULL);
>>>>>>        if (k->init(vdev) < 0) {
>>>>>>            return -1;
>>>>>>        }
>>>>>>        virtio_bus_plug_device(vdev);
>>>>>>        return 0;
>>>>>> }
>>>>>>
>>>>>> virtio_net_set_config_size() is currently being called as part of the
>>>>>> k->init call, but the host_features aren't properly setup until the device
>>>>>> is plugged into the bus using virtio_bus_plug_device().
>>>>>>
>>>>>> After talking with mdroth, I think the proper way to fix this would be to
>>>>>> extend VirtioDeviceClass to include a calculate_config_size() method that
>>>>>> can be called at the appropriate time during device initialization like so:
>>>>>>
>>>>>> static int virtio_device_init(DeviceState *qdev)
>>>>>> {
>>>>>>        VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>>>>        VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>>>>        assert(k->init != NULL);
>>>>>>        if (k->init(vdev) < 0) {
>>>>>>            return -1;
>>>>>>        }
>>>>>>        virtio_bus_plug_device(vdev);
>>>>>> +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
>>>>>> +       return -1;
>>>>>> +   }
>>>>>>        return 0;
>>>>>> }
>>>>>>
>>>>>> This would ensure that host_features contains the proper data before any
>>>>>> devices try to make use of it to calculate the config size.
>>>>> Good point, I didn't saw that.
>>>>>
>>>>> but this was not the case with commit 14f9b664 no?
>>>>>
>>>> I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
>>>> it yet. I'll confirm this afternoon.
>>> Ok, I think there is a problem with your patch:
>>>
>>>      virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
>>>                                    n->config_size);
>>>
>>> is called in virtio_net_device_init (k->init).
>>>
>>> Is there a way to resize the config after that?
>>>
>> Nope. That's definitely a bug. I can send a patch to fix this today. Any
>> objection to the method suggested above (extending VirtioDeviceClass)?
> This needs more thought. All devices expected everything
> is setup during the init call. config size is
> likely not the only issue.
>
> So we need almost all of virtio_bus_plug_device before init,
> and then after init send the signal:
>
>      if (klass->device_plugged != NULL) {
>          klass->device_plugged(qbus->parent);
>      }

Seems the interesting part is in virtio_pci_device_plugged at the end:

     proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
     proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
     proxy->host_features = virtio_bus_get_vdev_features(bus,
proxy->host_features);

This is and was called after set_config_size, isn't that the issue?

>
>
>>>>>>>>         for (i = 0; feature_sizes[i].flags != 0; i++) {
>>>>>>>>             if (host_features & feature_sizes[i].flags) {
>>>>>>>>                 config_size = MAX(feature_sizes[i].end, config_size);
>>>>>>>> -- 
>>>>>>>> 1.7.1
>>>> Jesse Larrew
>>>> Software Engineer, KVM Team
>>>> IBM Linux Technology Center
>>>> Phone: (512) 973-2052 (T/L: 363-2052)
>>>> jlarrew@linux.vnet.ibm.com
>>>>
>>
>> -- 
>>
>> Jesse Larrew
>> Software Engineer, KVM Team
>> IBM Linux Technology Center
>> Phone: (512) 973-2052 (T/L: 363-2052)
>> jlarrew@linux.vnet.ibm.com

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Michael S. Tsirkin]

On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric wrote:
> On 29/04/2013 18:02, Michael S. Tsirkin wrote:
> >On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
> >>On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
> >>>On 29/04/2013 17:14, Jesse Larrew wrote:
> >>>>On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
> >>>>>On 29/04/2013 16:42, Jesse Larrew wrote:
> >>>>>>On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
> >>>>>>>On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
> >>>>>>>>Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
> >>>>>>>>calculate config size based on the host features. But it forgets the
> >>>>>>>>VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
> >>>>>>>>len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
> >>>>>>>>disabled form command line. Then qemu will crash when user tries to read the
> >>>>>>>>config of virtio-net.
> >>>>>>>>
> >>>>>>>>Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
> >>>>>>>>the mac address.
> >>>>>>>>
> >>>>>>>>Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
> >>>>>>>>Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>>>>>>---
> >>>>>>>>    hw/net/virtio-net.c |    3 ++-
> >>>>>>>>    1 files changed, 2 insertions(+), 1 deletions(-)
> >>>>>>>>
> >>>>>>>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>>>>>>index 70c8fce..33a70ef 100644
> >>>>>>>>--- a/hw/net/virtio-net.c
> >>>>>>>>+++ b/hw/net/virtio-net.c
> >>>>>>>>@@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
> >>>>>>>>      void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
> >>>>>>>>    {
> >>>>>>>>-    int i, config_size = 0;
> >>>>>>>>+    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
> >>>>>>>>+    int i, config_size = feature_sizes[0].end;
> >>>>>>>This would be cleaner:
> >>>>>>>      host_features |= (1 << VIRTIO_NET_F_MAC);
> >>>>>>>
> >>>>>>>no need for a comment then.
> >>>>>>>
> >>>>>>It seems to me that the real problem here is that host_features isn't
> >>>>>>properly populated before virtio_net_set_config_size() is called. Looking
> >>>>>>at virtio_device_init(), we can see why:
> >>>>>>
> >>>>>>static int virtio_device_init(DeviceState *qdev)
> >>>>>>{
> >>>>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> >>>>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> >>>>>>       assert(k->init != NULL);
> >>>>>>       if (k->init(vdev) < 0) {
> >>>>>>           return -1;
> >>>>>>       }
> >>>>>>       virtio_bus_plug_device(vdev);
> >>>>>>       return 0;
> >>>>>>}
> >>>>>>
> >>>>>>virtio_net_set_config_size() is currently being called as part of the
> >>>>>>k->init call, but the host_features aren't properly setup until the device
> >>>>>>is plugged into the bus using virtio_bus_plug_device().
> >>>>>>
> >>>>>>After talking with mdroth, I think the proper way to fix this would be to
> >>>>>>extend VirtioDeviceClass to include a calculate_config_size() method that
> >>>>>>can be called at the appropriate time during device initialization like so:
> >>>>>>
> >>>>>>static int virtio_device_init(DeviceState *qdev)
> >>>>>>{
> >>>>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> >>>>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> >>>>>>       assert(k->init != NULL);
> >>>>>>       if (k->init(vdev) < 0) {
> >>>>>>           return -1;
> >>>>>>       }
> >>>>>>       virtio_bus_plug_device(vdev);
> >>>>>>+   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
> >>>>>>+       return -1;
> >>>>>>+   }
> >>>>>>       return 0;
> >>>>>>}
> >>>>>>
> >>>>>>This would ensure that host_features contains the proper data before any
> >>>>>>devices try to make use of it to calculate the config size.
> >>>>>Good point, I didn't saw that.
> >>>>>
> >>>>>but this was not the case with commit 14f9b664 no?
> >>>>>
> >>>>I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
> >>>>it yet. I'll confirm this afternoon.
> >>>Ok, I think there is a problem with your patch:
> >>>
> >>>     virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
> >>>                                   n->config_size);
> >>>
> >>>is called in virtio_net_device_init (k->init).
> >>>
> >>>Is there a way to resize the config after that?
> >>>
> >>Nope. That's definitely a bug. I can send a patch to fix this today. Any
> >>objection to the method suggested above (extending VirtioDeviceClass)?
> >This needs more thought. All devices expected everything
> >is setup during the init call. config size is
> >likely not the only issue.
> >
> >So we need almost all of virtio_bus_plug_device before init,
> >and then after init send the signal:
> >
> >     if (klass->device_plugged != NULL) {
> >         klass->device_plugged(qbus->parent);
> >     }
> 
> Seems the interesting part is in virtio_pci_device_plugged at the end:
> 
>     proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
>     proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
>     proxy->host_features = virtio_bus_get_vdev_features(bus,
> proxy->host_features);
> 
> This is and was called after set_config_size, isn't that the issue?

Basically devices expected everything to be setup at
the point where init is called.
We found one bug but let's fix it properly.
There's no reason to delay parts of setup until after init,
if we do, we'll trip on more uses of uninitialized data.

> >
> >
> >>>>>>>>        for (i = 0; feature_sizes[i].flags != 0; i++) {
> >>>>>>>>            if (host_features & feature_sizes[i].flags) {
> >>>>>>>>                config_size = MAX(feature_sizes[i].end, config_size);
> >>>>>>>>-- 
> >>>>>>>>1.7.1
> >>>>Jesse Larrew
> >>>>Software Engineer, KVM Team
> >>>>IBM Linux Technology Center
> >>>>Phone: (512) 973-2052 (T/L: 363-2052)
> >>>>jlarrew@linux.vnet.ibm.com
> >>>>
> >>
> >>-- 
> >>
> >>Jesse Larrew
> >>Software Engineer, KVM Team
> >>IBM Linux Technology Center
> >>Phone: (512) 973-2052 (T/L: 363-2052)
> >>jlarrew@linux.vnet.ibm.com

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Michael S. Tsirkin]

On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric wrote:
> > On 29/04/2013 18:02, Michael S. Tsirkin wrote:
> > >On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
> > >>On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
> > >>>On 29/04/2013 17:14, Jesse Larrew wrote:
> > >>>>On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
> > >>>>>On 29/04/2013 16:42, Jesse Larrew wrote:
> > >>>>>>On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
> > >>>>>>>On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
> > >>>>>>>>Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
> > >>>>>>>>calculate config size based on the host features. But it forgets the
> > >>>>>>>>VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
> > >>>>>>>>len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
> > >>>>>>>>disabled form command line. Then qemu will crash when user tries to read the
> > >>>>>>>>config of virtio-net.
> > >>>>>>>>
> > >>>>>>>>Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
> > >>>>>>>>the mac address.
> > >>>>>>>>
> > >>>>>>>>Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
> > >>>>>>>>Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >>>>>>>>---
> > >>>>>>>>    hw/net/virtio-net.c |    3 ++-
> > >>>>>>>>    1 files changed, 2 insertions(+), 1 deletions(-)
> > >>>>>>>>
> > >>>>>>>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > >>>>>>>>index 70c8fce..33a70ef 100644
> > >>>>>>>>--- a/hw/net/virtio-net.c
> > >>>>>>>>+++ b/hw/net/virtio-net.c
> > >>>>>>>>@@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
> > >>>>>>>>      void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
> > >>>>>>>>    {
> > >>>>>>>>-    int i, config_size = 0;
> > >>>>>>>>+    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
> > >>>>>>>>+    int i, config_size = feature_sizes[0].end;
> > >>>>>>>This would be cleaner:
> > >>>>>>>      host_features |= (1 << VIRTIO_NET_F_MAC);
> > >>>>>>>
> > >>>>>>>no need for a comment then.
> > >>>>>>>
> > >>>>>>It seems to me that the real problem here is that host_features isn't
> > >>>>>>properly populated before virtio_net_set_config_size() is called. Looking
> > >>>>>>at virtio_device_init(), we can see why:
> > >>>>>>
> > >>>>>>static int virtio_device_init(DeviceState *qdev)
> > >>>>>>{
> > >>>>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> > >>>>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> > >>>>>>       assert(k->init != NULL);
> > >>>>>>       if (k->init(vdev) < 0) {
> > >>>>>>           return -1;
> > >>>>>>       }
> > >>>>>>       virtio_bus_plug_device(vdev);
> > >>>>>>       return 0;
> > >>>>>>}
> > >>>>>>
> > >>>>>>virtio_net_set_config_size() is currently being called as part of the
> > >>>>>>k->init call, but the host_features aren't properly setup until the device
> > >>>>>>is plugged into the bus using virtio_bus_plug_device().
> > >>>>>>
> > >>>>>>After talking with mdroth, I think the proper way to fix this would be to
> > >>>>>>extend VirtioDeviceClass to include a calculate_config_size() method that
> > >>>>>>can be called at the appropriate time during device initialization like so:
> > >>>>>>
> > >>>>>>static int virtio_device_init(DeviceState *qdev)
> > >>>>>>{
> > >>>>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> > >>>>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> > >>>>>>       assert(k->init != NULL);
> > >>>>>>       if (k->init(vdev) < 0) {
> > >>>>>>           return -1;
> > >>>>>>       }
> > >>>>>>       virtio_bus_plug_device(vdev);
> > >>>>>>+   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
> > >>>>>>+       return -1;
> > >>>>>>+   }
> > >>>>>>       return 0;
> > >>>>>>}
> > >>>>>>
> > >>>>>>This would ensure that host_features contains the proper data before any
> > >>>>>>devices try to make use of it to calculate the config size.
> > >>>>>Good point, I didn't saw that.
> > >>>>>
> > >>>>>but this was not the case with commit 14f9b664 no?
> > >>>>>
> > >>>>I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
> > >>>>it yet. I'll confirm this afternoon.
> > >>>Ok, I think there is a problem with your patch:
> > >>>
> > >>>     virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
> > >>>                                   n->config_size);
> > >>>
> > >>>is called in virtio_net_device_init (k->init).
> > >>>
> > >>>Is there a way to resize the config after that?
> > >>>
> > >>Nope. That's definitely a bug. I can send a patch to fix this today. Any
> > >>objection to the method suggested above (extending VirtioDeviceClass)?
> > >This needs more thought. All devices expected everything
> > >is setup during the init call. config size is
> > >likely not the only issue.
> > >
> > >So we need almost all of virtio_bus_plug_device before init,
> > >and then after init send the signal:
> > >
> > >     if (klass->device_plugged != NULL) {
> > >         klass->device_plugged(qbus->parent);
> > >     }
> > 
> > Seems the interesting part is in virtio_pci_device_plugged at the end:
> > 
> >     proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> >     proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
> >     proxy->host_features = virtio_bus_get_vdev_features(bus,
> > proxy->host_features);
> > 
> > This is and was called after set_config_size, isn't that the issue?
> 
> Basically devices expected everything to be setup at
> the point where init is called.
> We found one bug but let's fix it properly.
> There's no reason to delay parts of setup until after init,
> if we do, we'll trip on more uses of uninitialized data.
> 
> > >
> > >
> > >>>>>>>>        for (i = 0; feature_sizes[i].flags != 0; i++) {
> > >>>>>>>>            if (host_features & feature_sizes[i].flags) {
> > >>>>>>>>                config_size = MAX(feature_sizes[i].end, config_size);
> > >>>>>>>>-- 
> > >>>>>>>>1.7.1
> > >>>>Jesse Larrew
> > >>>>Software Engineer, KVM Team
> > >>>>IBM Linux Technology Center
> > >>>>Phone: (512) 973-2052 (T/L: 363-2052)
> > >>>>jlarrew@linux.vnet.ibm.com
> > >>>>
> > >>
> > >>-- 
> > >>
> > >>Jesse Larrew
> > >>Software Engineer, KVM Team
> > >>IBM Linux Technology Center
> > >>Phone: (512) 973-2052 (T/L: 363-2052)
> > >>jlarrew@linux.vnet.ibm.com


Basically this is what I propose. Warning! compile-tested
only. (I also dropped an unused return value).


Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 1596a1c..c75c8ae 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -38,7 +38,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
 #endif
 
 /* Plug the VirtIODevice */
-int virtio_bus_plug_device(VirtIODevice *vdev)
+void virtio_bus_plug_device(VirtIODevice *vdev)
 {
     DeviceState *qdev = DEVICE(vdev);
     BusState *qbus = BUS(qdev_get_parent_bus(qdev));
@@ -64,12 +64,18 @@ int virtio_bus_plug_device(VirtIODevice *vdev)
     bus->bindings.set_host_notifier = klass->set_host_notifier;
     bus->bindings.vmstate_change = klass->vmstate_change;
     virtio_bind_device(bus->vdev, &bus->bindings, qbus->parent);
+}
+
+void virtio_bus_plugged(VirtIODevice *vdev)
+{
+    DeviceState *qdev = DEVICE(vdev);
+    BusState *qbus = BUS(qdev_get_parent_bus(qdev));
+    VirtioBusState *bus = VIRTIO_BUS(qbus);
+    VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
 
     if (klass->device_plugged != NULL) {
         klass->device_plugged(qbus->parent);
     }
-
-    return 0;
 }
 
 /* Reset the virtio_bus */
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 1c2282c..65f69cf 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1088,11 +1088,16 @@ static int virtio_device_init(DeviceState *qdev)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
+
+    virtio_bus_plug_device(vdev);
+
     assert(k->init != NULL);
     if (k->init(vdev) < 0) {
         return -1;
     }
-    virtio_bus_plug_device(vdev);
+
+    virtio_bus_plugged(vdev);
+
     return 0;
 }
 
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 311e8c7..471e55e 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -76,7 +76,8 @@ struct VirtioBusState {
     VirtIOBindings bindings;
 };
 
-int virtio_bus_plug_device(VirtIODevice *vdev);
+void virtio_bus_plug_device(VirtIODevice *vdev);
+void virtio_bus_plugged(VirtIODevice *vdev);
 void virtio_bus_reset(VirtioBusState *bus);
 void virtio_bus_destroy_device(VirtioBusState *bus);
 /* Get the device id of the plugged device. */

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[KONRAD Frédéric]

On 29/04/2013 18:30, Michael S. Tsirkin wrote:
> On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin wrote:
>> On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric wrote:
>>> On 29/04/2013 18:02, Michael S. Tsirkin wrote:
>>>> On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
>>>>> On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
>>>>>> On 29/04/2013 17:14, Jesse Larrew wrote:
>>>>>>> On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
>>>>>>>> On 29/04/2013 16:42, Jesse Larrew wrote:
>>>>>>>>> On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
>>>>>>>>>> On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
>>>>>>>>>>> Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>>>>>>>>>>> calculate config size based on the host features. But it forgets the
>>>>>>>>>>> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>>>>>>>>>>> len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>>>>>>>>>>> disabled form command line. Then qemu will crash when user tries to read the
>>>>>>>>>>> config of virtio-net.
>>>>>>>>>>>
>>>>>>>>>>> Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>>>>>>>>>>> the mac address.
>>>>>>>>>>>
>>>>>>>>>>> Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>>>>>>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>>>>>>> ---
>>>>>>>>>>>     hw/net/virtio-net.c |    3 ++-
>>>>>>>>>>>     1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>>>>>>> index 70c8fce..33a70ef 100644
>>>>>>>>>>> --- a/hw/net/virtio-net.c
>>>>>>>>>>> +++ b/hw/net/virtio-net.c
>>>>>>>>>>> @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>>>>>>>>>>       void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>>>>>>>>>>>     {
>>>>>>>>>>> -    int i, config_size = 0;
>>>>>>>>>>> +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>>>>>>>>>>> +    int i, config_size = feature_sizes[0].end;
>>>>>>>>>> This would be cleaner:
>>>>>>>>>>       host_features |= (1 << VIRTIO_NET_F_MAC);
>>>>>>>>>>
>>>>>>>>>> no need for a comment then.
>>>>>>>>>>
>>>>>>>>> It seems to me that the real problem here is that host_features isn't
>>>>>>>>> properly populated before virtio_net_set_config_size() is called. Looking
>>>>>>>>> at virtio_device_init(), we can see why:
>>>>>>>>>
>>>>>>>>> static int virtio_device_init(DeviceState *qdev)
>>>>>>>>> {
>>>>>>>>>        VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>>>>>>>        VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>>>>>>>        assert(k->init != NULL);
>>>>>>>>>        if (k->init(vdev) < 0) {
>>>>>>>>>            return -1;
>>>>>>>>>        }
>>>>>>>>>        virtio_bus_plug_device(vdev);
>>>>>>>>>        return 0;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> virtio_net_set_config_size() is currently being called as part of the
>>>>>>>>> k->init call, but the host_features aren't properly setup until the device
>>>>>>>>> is plugged into the bus using virtio_bus_plug_device().
>>>>>>>>>
>>>>>>>>> After talking with mdroth, I think the proper way to fix this would be to
>>>>>>>>> extend VirtioDeviceClass to include a calculate_config_size() method that
>>>>>>>>> can be called at the appropriate time during device initialization like so:
>>>>>>>>>
>>>>>>>>> static int virtio_device_init(DeviceState *qdev)
>>>>>>>>> {
>>>>>>>>>        VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>>>>>>>        VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>>>>>>>        assert(k->init != NULL);
>>>>>>>>>        if (k->init(vdev) < 0) {
>>>>>>>>>            return -1;
>>>>>>>>>        }
>>>>>>>>>        virtio_bus_plug_device(vdev);
>>>>>>>>> +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
>>>>>>>>> +       return -1;
>>>>>>>>> +   }
>>>>>>>>>        return 0;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> This would ensure that host_features contains the proper data before any
>>>>>>>>> devices try to make use of it to calculate the config size.
>>>>>>>> Good point, I didn't saw that.
>>>>>>>>
>>>>>>>> but this was not the case with commit 14f9b664 no?
>>>>>>>>
>>>>>>> I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
>>>>>>> it yet. I'll confirm this afternoon.
>>>>>> Ok, I think there is a problem with your patch:
>>>>>>
>>>>>>      virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
>>>>>>                                    n->config_size);
>>>>>>
>>>>>> is called in virtio_net_device_init (k->init).
>>>>>>
>>>>>> Is there a way to resize the config after that?
>>>>>>
>>>>> Nope. That's definitely a bug. I can send a patch to fix this today. Any
>>>>> objection to the method suggested above (extending VirtioDeviceClass)?
>>>> This needs more thought. All devices expected everything
>>>> is setup during the init call. config size is
>>>> likely not the only issue.
>>>>
>>>> So we need almost all of virtio_bus_plug_device before init,
>>>> and then after init send the signal:
>>>>
>>>>      if (klass->device_plugged != NULL) {
>>>>          klass->device_plugged(qbus->parent);
>>>>      }
>>> Seems the interesting part is in virtio_pci_device_plugged at the end:
>>>
>>>      proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
>>>      proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
>>>      proxy->host_features = virtio_bus_get_vdev_features(bus,
>>> proxy->host_features);
>>>
>>> This is and was called after set_config_size, isn't that the issue?
>> Basically devices expected everything to be setup at
>> the point where init is called.
>> We found one bug but let's fix it properly.
>> There's no reason to delay parts of setup until after init,
>> if we do, we'll trip on more uses of uninitialized data.
>>
>>>>
>>>>>>>>>>>         for (i = 0; feature_sizes[i].flags != 0; i++) {
>>>>>>>>>>>             if (host_features & feature_sizes[i].flags) {
>>>>>>>>>>>                 config_size = MAX(feature_sizes[i].end, config_size);
>>>>>>>>>>> -- 
>>>>>>>>>>> 1.7.1
>>>>>>> Jesse Larrew
>>>>>>> Software Engineer, KVM Team
>>>>>>> IBM Linux Technology Center
>>>>>>> Phone: (512) 973-2052 (T/L: 363-2052)
>>>>>>> jlarrew@linux.vnet.ibm.com
>>>>>>>
>>>>> -- 
>>>>>
>>>>> Jesse Larrew
>>>>> Software Engineer, KVM Team
>>>>> IBM Linux Technology Center
>>>>> Phone: (512) 973-2052 (T/L: 363-2052)
>>>>> jlarrew@linux.vnet.ibm.com
>
> Basically this is what I propose. Warning! compile-tested
> only. (I also dropped an unused return value).
>
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 1596a1c..c75c8ae 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -38,7 +38,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>   #endif
>   
>   /* Plug the VirtIODevice */
> -int virtio_bus_plug_device(VirtIODevice *vdev)
> +void virtio_bus_plug_device(VirtIODevice *vdev)
>   {
>       DeviceState *qdev = DEVICE(vdev);
>       BusState *qbus = BUS(qdev_get_parent_bus(qdev));
> @@ -64,12 +64,18 @@ int virtio_bus_plug_device(VirtIODevice *vdev)
>       bus->bindings.set_host_notifier = klass->set_host_notifier;
>       bus->bindings.vmstate_change = klass->vmstate_change;
>       virtio_bind_device(bus->vdev, &bus->bindings, qbus->parent);
> +}
> +
> +void virtio_bus_plugged(VirtIODevice *vdev)
> +{
> +    DeviceState *qdev = DEVICE(vdev);
> +    BusState *qbus = BUS(qdev_get_parent_bus(qdev));
> +    VirtioBusState *bus = VIRTIO_BUS(qbus);
> +    VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
>   
>       if (klass->device_plugged != NULL) {
>           klass->device_plugged(qbus->parent);
>       }
> -
> -    return 0;
>   }
>   
>   /* Reset the virtio_bus */
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 1c2282c..65f69cf 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1088,11 +1088,16 @@ static int virtio_device_init(DeviceState *qdev)
>   {
>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> +
> +    virtio_bus_plug_device(vdev);
> +
>       assert(k->init != NULL);
>       if (k->init(vdev) < 0) {
>           return -1;
>       }
> -    virtio_bus_plug_device(vdev);
> +
> +    virtio_bus_plugged(vdev);
> +
>       return 0;
>   }
>   
> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
> index 311e8c7..471e55e 100644
> --- a/include/hw/virtio/virtio-bus.h
> +++ b/include/hw/virtio/virtio-bus.h
> @@ -76,7 +76,8 @@ struct VirtioBusState {
>       VirtIOBindings bindings;
>   };
>   
> -int virtio_bus_plug_device(VirtIODevice *vdev);
> +void virtio_bus_plug_device(VirtIODevice *vdev);
> +void virtio_bus_plugged(VirtIODevice *vdev);
>   void virtio_bus_reset(VirtioBusState *bus);
>   void virtio_bus_destroy_device(VirtioBusState *bus);
>   /* Get the device id of the plugged device. */
Which tree are you using? Is it up to date?

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Michael S. Tsirkin]

On Mon, Apr 29, 2013 at 06:41:08PM +0200, KONRAD Frédéric wrote:
> On 29/04/2013 18:30, Michael S. Tsirkin wrote:
> >On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin wrote:
> >>On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric wrote:
> >>>On 29/04/2013 18:02, Michael S. Tsirkin wrote:
> >>>>On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
> >>>>>On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
> >>>>>>On 29/04/2013 17:14, Jesse Larrew wrote:
> >>>>>>>On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
> >>>>>>>>On 29/04/2013 16:42, Jesse Larrew wrote:
> >>>>>>>>>On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
> >>>>>>>>>>On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
> >>>>>>>>>>>Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
> >>>>>>>>>>>calculate config size based on the host features. But it forgets the
> >>>>>>>>>>>VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
> >>>>>>>>>>>len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
> >>>>>>>>>>>disabled form command line. Then qemu will crash when user tries to read the
> >>>>>>>>>>>config of virtio-net.
> >>>>>>>>>>>
> >>>>>>>>>>>Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
> >>>>>>>>>>>the mac address.
> >>>>>>>>>>>
> >>>>>>>>>>>Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
> >>>>>>>>>>>Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>>>>>>>>>---
> >>>>>>>>>>>    hw/net/virtio-net.c |    3 ++-
> >>>>>>>>>>>    1 files changed, 2 insertions(+), 1 deletions(-)
> >>>>>>>>>>>
> >>>>>>>>>>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>>>>>>>>>index 70c8fce..33a70ef 100644
> >>>>>>>>>>>--- a/hw/net/virtio-net.c
> >>>>>>>>>>>+++ b/hw/net/virtio-net.c
> >>>>>>>>>>>@@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
> >>>>>>>>>>>      void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
> >>>>>>>>>>>    {
> >>>>>>>>>>>-    int i, config_size = 0;
> >>>>>>>>>>>+    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
> >>>>>>>>>>>+    int i, config_size = feature_sizes[0].end;
> >>>>>>>>>>This would be cleaner:
> >>>>>>>>>>      host_features |= (1 << VIRTIO_NET_F_MAC);
> >>>>>>>>>>
> >>>>>>>>>>no need for a comment then.
> >>>>>>>>>>
> >>>>>>>>>It seems to me that the real problem here is that host_features isn't
> >>>>>>>>>properly populated before virtio_net_set_config_size() is called. Looking
> >>>>>>>>>at virtio_device_init(), we can see why:
> >>>>>>>>>
> >>>>>>>>>static int virtio_device_init(DeviceState *qdev)
> >>>>>>>>>{
> >>>>>>>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> >>>>>>>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> >>>>>>>>>       assert(k->init != NULL);
> >>>>>>>>>       if (k->init(vdev) < 0) {
> >>>>>>>>>           return -1;
> >>>>>>>>>       }
> >>>>>>>>>       virtio_bus_plug_device(vdev);
> >>>>>>>>>       return 0;
> >>>>>>>>>}
> >>>>>>>>>
> >>>>>>>>>virtio_net_set_config_size() is currently being called as part of the
> >>>>>>>>>k->init call, but the host_features aren't properly setup until the device
> >>>>>>>>>is plugged into the bus using virtio_bus_plug_device().
> >>>>>>>>>
> >>>>>>>>>After talking with mdroth, I think the proper way to fix this would be to
> >>>>>>>>>extend VirtioDeviceClass to include a calculate_config_size() method that
> >>>>>>>>>can be called at the appropriate time during device initialization like so:
> >>>>>>>>>
> >>>>>>>>>static int virtio_device_init(DeviceState *qdev)
> >>>>>>>>>{
> >>>>>>>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> >>>>>>>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> >>>>>>>>>       assert(k->init != NULL);
> >>>>>>>>>       if (k->init(vdev) < 0) {
> >>>>>>>>>           return -1;
> >>>>>>>>>       }
> >>>>>>>>>       virtio_bus_plug_device(vdev);
> >>>>>>>>>+   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
> >>>>>>>>>+       return -1;
> >>>>>>>>>+   }
> >>>>>>>>>       return 0;
> >>>>>>>>>}
> >>>>>>>>>
> >>>>>>>>>This would ensure that host_features contains the proper data before any
> >>>>>>>>>devices try to make use of it to calculate the config size.
> >>>>>>>>Good point, I didn't saw that.
> >>>>>>>>
> >>>>>>>>but this was not the case with commit 14f9b664 no?
> >>>>>>>>
> >>>>>>>I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
> >>>>>>>it yet. I'll confirm this afternoon.
> >>>>>>Ok, I think there is a problem with your patch:
> >>>>>>
> >>>>>>     virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
> >>>>>>                                   n->config_size);
> >>>>>>
> >>>>>>is called in virtio_net_device_init (k->init).
> >>>>>>
> >>>>>>Is there a way to resize the config after that?
> >>>>>>
> >>>>>Nope. That's definitely a bug. I can send a patch to fix this today. Any
> >>>>>objection to the method suggested above (extending VirtioDeviceClass)?
> >>>>This needs more thought. All devices expected everything
> >>>>is setup during the init call. config size is
> >>>>likely not the only issue.
> >>>>
> >>>>So we need almost all of virtio_bus_plug_device before init,
> >>>>and then after init send the signal:
> >>>>
> >>>>     if (klass->device_plugged != NULL) {
> >>>>         klass->device_plugged(qbus->parent);
> >>>>     }
> >>>Seems the interesting part is in virtio_pci_device_plugged at the end:
> >>>
> >>>     proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> >>>     proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
> >>>     proxy->host_features = virtio_bus_get_vdev_features(bus,
> >>>proxy->host_features);
> >>>
> >>>This is and was called after set_config_size, isn't that the issue?
> >>Basically devices expected everything to be setup at
> >>the point where init is called.
> >>We found one bug but let's fix it properly.
> >>There's no reason to delay parts of setup until after init,
> >>if we do, we'll trip on more uses of uninitialized data.
> >>
> >>>>
> >>>>>>>>>>>        for (i = 0; feature_sizes[i].flags != 0; i++) {
> >>>>>>>>>>>            if (host_features & feature_sizes[i].flags) {
> >>>>>>>>>>>                config_size = MAX(feature_sizes[i].end, config_size);
> >>>>>>>>>>>-- 
> >>>>>>>>>>>1.7.1
> >>>>>>>Jesse Larrew
> >>>>>>>Software Engineer, KVM Team
> >>>>>>>IBM Linux Technology Center
> >>>>>>>Phone: (512) 973-2052 (T/L: 363-2052)
> >>>>>>>jlarrew@linux.vnet.ibm.com
> >>>>>>>
> >>>>>-- 
> >>>>>
> >>>>>Jesse Larrew
> >>>>>Software Engineer, KVM Team
> >>>>>IBM Linux Technology Center
> >>>>>Phone: (512) 973-2052 (T/L: 363-2052)
> >>>>>jlarrew@linux.vnet.ibm.com
> >
> >Basically this is what I propose. Warning! compile-tested
> >only. (I also dropped an unused return value).
> >
> >
> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Which tree are you using? Is it up to date?

Heh, more changes came in.  So now, it's even simpler:

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index aab72ff..447ba16 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -38,7 +38,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
 #endif
 
 /* Plug the VirtIODevice */
-int virtio_bus_plug_device(VirtIODevice *vdev)
+void virtio_bus_plug_device(VirtIODevice *vdev)
 {
     DeviceState *qdev = DEVICE(vdev);
     BusState *qbus = BUS(qdev_get_parent_bus(qdev));
@@ -51,8 +51,6 @@ int virtio_bus_plug_device(VirtIODevice *vdev)
     if (klass->device_plugged != NULL) {
         klass->device_plugged(qbus->parent);
     }
-
-    return 0;
 }
 
 /* Reset the virtio_bus */
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 0f88c25..c5228e6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1091,11 +1091,11 @@ static int virtio_device_init(DeviceState *qdev)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
+    virtio_bus_plug_device(vdev);
     assert(k->init != NULL);
     if (k->init(vdev) < 0) {
         return -1;
     }
-    virtio_bus_plug_device(vdev);
     return 0;
 }
 
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 9ed60f9..da84141 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -72,7 +72,7 @@ struct VirtioBusState {
     VirtIODevice *vdev;
 };
 
-int virtio_bus_plug_device(VirtIODevice *vdev);
+void virtio_bus_plug_device(VirtIODevice *vdev);
 void virtio_bus_reset(VirtioBusState *bus);
 void virtio_bus_destroy_device(VirtioBusState *bus);
 /* Get the device id of the plugged device. */

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[KONRAD Frédéric]

[-- Attachment #1: Type: text/plain, Size: 9869 bytes --]

On 29/04/2013 19:01, Michael S. Tsirkin wrote:
> On Mon, Apr 29, 2013 at 06:41:08PM +0200, KONRAD Frédéric wrote:
>> On 29/04/2013 18:30, Michael S. Tsirkin wrote:
>>> On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin wrote:
>>>> On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric wrote:
>>>>> On 29/04/2013 18:02, Michael S. Tsirkin wrote:
>>>>>> On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
>>>>>>> On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
>>>>>>>> On 29/04/2013 17:14, Jesse Larrew wrote:
>>>>>>>>> On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
>>>>>>>>>> On 29/04/2013 16:42, Jesse Larrew wrote:
>>>>>>>>>>> On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
>>>>>>>>>>>> On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
>>>>>>>>>>>>> Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>>>>>>>>>>>>> calculate config size based on the host features. But it forgets the
>>>>>>>>>>>>> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>>>>>>>>>>>>> len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>>>>>>>>>>>>> disabled form command line. Then qemu will crash when user tries to read the
>>>>>>>>>>>>> config of virtio-net.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>>>>>>>>>>>>> the mac address.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>>>>>>>>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>     hw/net/virtio-net.c |    3 ++-
>>>>>>>>>>>>>     1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>>>>>>>>> index 70c8fce..33a70ef 100644
>>>>>>>>>>>>> --- a/hw/net/virtio-net.c
>>>>>>>>>>>>> +++ b/hw/net/virtio-net.c
>>>>>>>>>>>>> @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>>>>>>>>>>>>       void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>>>>>>>>>>>>>     {
>>>>>>>>>>>>> -    int i, config_size = 0;
>>>>>>>>>>>>> +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>>>>>>>>>>>>> +    int i, config_size = feature_sizes[0].end;
>>>>>>>>>>>> This would be cleaner:
>>>>>>>>>>>>       host_features |= (1 << VIRTIO_NET_F_MAC);
>>>>>>>>>>>>
>>>>>>>>>>>> no need for a comment then.
>>>>>>>>>>>>
>>>>>>>>>>> It seems to me that the real problem here is that host_features isn't
>>>>>>>>>>> properly populated before virtio_net_set_config_size() is called. Looking
>>>>>>>>>>> at virtio_device_init(), we can see why:
>>>>>>>>>>>
>>>>>>>>>>> static int virtio_device_init(DeviceState *qdev)
>>>>>>>>>>> {
>>>>>>>>>>>        VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>>>>>>>>>        VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>>>>>>>>>        assert(k->init != NULL);
>>>>>>>>>>>        if (k->init(vdev) < 0) {
>>>>>>>>>>>            return -1;
>>>>>>>>>>>        }
>>>>>>>>>>>        virtio_bus_plug_device(vdev);
>>>>>>>>>>>        return 0;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> virtio_net_set_config_size() is currently being called as part of the
>>>>>>>>>>> k->init call, but the host_features aren't properly setup until the device
>>>>>>>>>>> is plugged into the bus using virtio_bus_plug_device().
>>>>>>>>>>>
>>>>>>>>>>> After talking with mdroth, I think the proper way to fix this would be to
>>>>>>>>>>> extend VirtioDeviceClass to include a calculate_config_size() method that
>>>>>>>>>>> can be called at the appropriate time during device initialization like so:
>>>>>>>>>>>
>>>>>>>>>>> static int virtio_device_init(DeviceState *qdev)
>>>>>>>>>>> {
>>>>>>>>>>>        VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>>>>>>>>>        VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>>>>>>>>>        assert(k->init != NULL);
>>>>>>>>>>>        if (k->init(vdev) < 0) {
>>>>>>>>>>>            return -1;
>>>>>>>>>>>        }
>>>>>>>>>>>        virtio_bus_plug_device(vdev);
>>>>>>>>>>> +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
>>>>>>>>>>> +       return -1;
>>>>>>>>>>> +   }
>>>>>>>>>>>        return 0;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> This would ensure that host_features contains the proper data before any
>>>>>>>>>>> devices try to make use of it to calculate the config size.
>>>>>>>>>> Good point, I didn't saw that.
>>>>>>>>>>
>>>>>>>>>> but this was not the case with commit 14f9b664 no?
>>>>>>>>>>
>>>>>>>>> I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
>>>>>>>>> it yet. I'll confirm this afternoon.
>>>>>>>> Ok, I think there is a problem with your patch:
>>>>>>>>
>>>>>>>>      virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
>>>>>>>>                                    n->config_size);
>>>>>>>>
>>>>>>>> is called in virtio_net_device_init (k->init).
>>>>>>>>
>>>>>>>> Is there a way to resize the config after that?
>>>>>>>>
>>>>>>> Nope. That's definitely a bug. I can send a patch to fix this today. Any
>>>>>>> objection to the method suggested above (extending VirtioDeviceClass)?
>>>>>> This needs more thought. All devices expected everything
>>>>>> is setup during the init call. config size is
>>>>>> likely not the only issue.
>>>>>>
>>>>>> So we need almost all of virtio_bus_plug_device before init,
>>>>>> and then after init send the signal:
>>>>>>
>>>>>>      if (klass->device_plugged != NULL) {
>>>>>>          klass->device_plugged(qbus->parent);
>>>>>>      }
>>>>> Seems the interesting part is in virtio_pci_device_plugged at the end:
>>>>>
>>>>>      proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
>>>>>      proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
>>>>>      proxy->host_features = virtio_bus_get_vdev_features(bus,
>>>>> proxy->host_features);
>>>>>
>>>>> This is and was called after set_config_size, isn't that the issue?
>>>> Basically devices expected everything to be setup at
>>>> the point where init is called.
>>>> We found one bug but let's fix it properly.
>>>> There's no reason to delay parts of setup until after init,
>>>> if we do, we'll trip on more uses of uninitialized data.
>>>>
>>>>>>>>>>>>>         for (i = 0; feature_sizes[i].flags != 0; i++) {
>>>>>>>>>>>>>             if (host_features & feature_sizes[i].flags) {
>>>>>>>>>>>>>                 config_size = MAX(feature_sizes[i].end, config_size);
>>>>>>>>>>>>> -- 
>>>>>>>>>>>>> 1.7.1
>>>>>>>>> Jesse Larrew
>>>>>>>>> Software Engineer, KVM Team
>>>>>>>>> IBM Linux Technology Center
>>>>>>>>> Phone: (512) 973-2052 (T/L: 363-2052)
>>>>>>>>> jlarrew@linux.vnet.ibm.com
>>>>>>>>>
>>>>>>> -- 
>>>>>>>
>>>>>>> Jesse Larrew
>>>>>>> Software Engineer, KVM Team
>>>>>>> IBM Linux Technology Center
>>>>>>> Phone: (512) 973-2052 (T/L: 363-2052)
>>>>>>> jlarrew@linux.vnet.ibm.com
>>> Basically this is what I propose. Warning! compile-tested
>>> only. (I also dropped an unused return value).
>>>
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Which tree are you using? Is it up to date?
> Heh, more changes came in.  So now, it's even simpler:
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index aab72ff..447ba16 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -38,7 +38,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>   #endif
>   
>   /* Plug the VirtIODevice */
> -int virtio_bus_plug_device(VirtIODevice *vdev)
> +void virtio_bus_plug_device(VirtIODevice *vdev)
>   {
>       DeviceState *qdev = DEVICE(vdev);
>       BusState *qbus = BUS(qdev_get_parent_bus(qdev));
> @@ -51,8 +51,6 @@ int virtio_bus_plug_device(VirtIODevice *vdev)
>       if (klass->device_plugged != NULL) {
>           klass->device_plugged(qbus->parent);
>       }
> -
> -    return 0;
>   }
>   
>   /* Reset the virtio_bus */
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 0f88c25..c5228e6 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1091,11 +1091,11 @@ static int virtio_device_init(DeviceState *qdev)
>   {
>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> +    virtio_bus_plug_device(vdev);
>       assert(k->init != NULL);
>       if (k->init(vdev) < 0) {
>           return -1;
>       }
> -    virtio_bus_plug_device(vdev);
>       return 0;
>   }

Not sure this is what you want to do.
The device would be plugged before it is inited :/.

You said:

"

This needs more thought. All devices expected everything
is setup during the init call. config size is
likely not the only issue.

So we need almost all of virtio_bus_plug_device before init,
and then after init send the signal:

     if (klass->device_plugged != NULL) {
         klass->device_plugged(qbus->parent);
     }

"

And as virtio_bus_plugged is only this chunk of code:

     if (klass->device_plugged != NULL) {
         klass->device_plugged(qbus->parent);
     }

It is the right place now.

>   
> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
> index 9ed60f9..da84141 100644
> --- a/include/hw/virtio/virtio-bus.h
> +++ b/include/hw/virtio/virtio-bus.h
> @@ -72,7 +72,7 @@ struct VirtioBusState {
>       VirtIODevice *vdev;
>   };
>   
> -int virtio_bus_plug_device(VirtIODevice *vdev);
> +void virtio_bus_plug_device(VirtIODevice *vdev);
>   void virtio_bus_reset(VirtioBusState *bus);
>   void virtio_bus_destroy_device(VirtioBusState *bus);
>   /* Get the device id of the plugged device. */

[-- Attachment #2: Type: text/html, Size: 11822 bytes --]

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Michael S. Tsirkin]

On Mon, Apr 29, 2013 at 07:23:49PM +0200, KONRAD Frédéric wrote:
> On 29/04/2013 19:01, Michael S. Tsirkin wrote:
> 
>     On Mon, Apr 29, 2013 at 06:41:08PM +0200, KONRAD Frédéric wrote:
> 
>         On 29/04/2013 18:30, Michael S. Tsirkin wrote:
> 
>             On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin wrote:
> 
>                 On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric wrote:
> 
>                     On 29/04/2013 18:02, Michael S. Tsirkin wrote:
> 
>                         On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
> 
>                             On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
> 
>                                 On 29/04/2013 17:14, Jesse Larrew wrote:
> 
>                                     On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
> 
>                                         On 29/04/2013 16:42, Jesse Larrew wrote:
> 
>                                         On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
> 
>                                         On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
> 
>                                         Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>                                         calculate config size based on the host features. But it forgets the
>                                         VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>                                         len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>                                         disabled form command line. Then qemu will crash when user tries to read the
>                                         config of virtio-net.
> 
>                                         Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>                                         the mac address.
> 
>                                         Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>                                         Signed-off-by: Jason Wang <jasowang@redhat.com>
>                                         ---
>                                            hw/net/virtio-net.c |    3 ++-
>                                            1 files changed, 2 insertions(+), 1 deletions(-)
> 
>                                         diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>                                         index 70c8fce..33a70ef 100644
>                                         --- a/hw/net/virtio-net.c
>                                         +++ b/hw/net/virtio-net.c
>                                         @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>                                              void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>                                            {
>                                         -    int i, config_size = 0;
>                                         +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>                                         +    int i, config_size = feature_sizes[0].end;
> 
>                                         This would be cleaner:
>                                              host_features |= (1 << VIRTIO_NET_F_MAC);
> 
>                                         no need for a comment then.
> 
> 
>                                         It seems to me that the real problem here is that host_features isn't
>                                         properly populated before virtio_net_set_config_size() is called. Looking
>                                         at virtio_device_init(), we can see why:
> 
>                                         static int virtio_device_init(DeviceState *qdev)
>                                         {
>                                               VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>                                               VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>                                               assert(k->init != NULL);
>                                               if (k->init(vdev) < 0) {
>                                                   return -1;
>                                               }
>                                               virtio_bus_plug_device(vdev);
>                                               return 0;
>                                         }
> 
>                                         virtio_net_set_config_size() is currently being called as part of the
>                                         k->init call, but the host_features aren't properly setup until the device
>                                         is plugged into the bus using virtio_bus_plug_device().
> 
>                                         After talking with mdroth, I think the proper way to fix this would be to
>                                         extend VirtioDeviceClass to include a calculate_config_size() method that
>                                         can be called at the appropriate time during device initialization like so:
> 
>                                         static int virtio_device_init(DeviceState *qdev)
>                                         {
>                                               VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>                                               VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>                                               assert(k->init != NULL);
>                                               if (k->init(vdev) < 0) {
>                                                   return -1;
>                                               }
>                                               virtio_bus_plug_device(vdev);
>                                         +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
>                                         +       return -1;
>                                         +   }
>                                               return 0;
>                                         }
> 
>                                         This would ensure that host_features contains the proper data before any
>                                         devices try to make use of it to calculate the config size.
> 
>                                         Good point, I didn't saw that.
> 
>                                         but this was not the case with commit 14f9b664 no?
> 
> 
>                                     I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
>                                     it yet. I'll confirm this afternoon.
> 
>                                 Ok, I think there is a problem with your patch:
> 
>                                     virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
>                                                                   n->config_size);
> 
>                                 is called in virtio_net_device_init (k->init).
> 
>                                 Is there a way to resize the config after that?
> 
> 
>                             Nope. That's definitely a bug. I can send a patch to fix this today. Any
>                             objection to the method suggested above (extending VirtioDeviceClass)?
> 
>                         This needs more thought. All devices expected everything
>                         is setup during the init call. config size is
>                         likely not the only issue.
> 
>                         So we need almost all of virtio_bus_plug_device before init,
>                         and then after init send the signal:
> 
>                             if (klass->device_plugged != NULL) {
>                                 klass->device_plugged(qbus->parent);
>                             }
> 
>                     Seems the interesting part is in virtio_pci_device_plugged at the end:
> 
>                         proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
>                         proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
>                         proxy->host_features = virtio_bus_get_vdev_features(bus,
>                     proxy->host_features);
> 
>                     This is and was called after set_config_size, isn't that the issue?
> 
>                 Basically devices expected everything to be setup at
>                 the point where init is called.
>                 We found one bug but let's fix it properly.
>                 There's no reason to delay parts of setup until after init,
>                 if we do, we'll trip on more uses of uninitialized data.
> 
> 
>                                                for (i = 0; feature_sizes[i].flags != 0; i++) {
>                                                    if (host_features & feature_sizes[i].flags) {
>                                                        config_size = MAX(feature_sizes[i].end, config_size);
>                                         --
>                                         1.7.1
> 
>                                     Jesse Larrew
>                                     Software Engineer, KVM Team
>                                     IBM Linux Technology Center
>                                     Phone: (512) 973-2052 (T/L: 363-2052)
>                                     jlarrew@linux.vnet.ibm.com
> 
> 
>                             --
> 
>                             Jesse Larrew
>                             Software Engineer, KVM Team
>                             IBM Linux Technology Center
>                             Phone: (512) 973-2052 (T/L: 363-2052)
>                             jlarrew@linux.vnet.ibm.com
> 
>             Basically this is what I propose. Warning! compile-tested
>             only. (I also dropped an unused return value).
> 
> 
>             Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
>         Which tree are you using? Is it up to date?
> 
>     Heh, more changes came in.  So now, it's even simpler:
> 
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
>     ---
> 
>     diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
>     index aab72ff..447ba16 100644
>     --- a/hw/virtio/virtio-bus.c
>     +++ b/hw/virtio/virtio-bus.c
>     @@ -38,7 +38,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>      #endif
> 
>      /* Plug the VirtIODevice */
>     -int virtio_bus_plug_device(VirtIODevice *vdev)
>     +void virtio_bus_plug_device(VirtIODevice *vdev)
>      {
>          DeviceState *qdev = DEVICE(vdev);
>          BusState *qbus = BUS(qdev_get_parent_bus(qdev));
>     @@ -51,8 +51,6 @@ int virtio_bus_plug_device(VirtIODevice *vdev)
>          if (klass->device_plugged != NULL) {
>              klass->device_plugged(qbus->parent);
>          }
>     -
>     -    return 0;
>      }
> 
>      /* Reset the virtio_bus */
>     diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>     index 0f88c25..c5228e6 100644
>     --- a/hw/virtio/virtio.c
>     +++ b/hw/virtio/virtio.c
>     @@ -1091,11 +1091,11 @@ static int virtio_device_init(DeviceState *qdev)
>      {
>          VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>          VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>     +    virtio_bus_plug_device(vdev);
>          assert(k->init != NULL);
>          if (k->init(vdev) < 0) {
>              return -1;
>          }
>     -    virtio_bus_plug_device(vdev);
>          return 0;
>      }
> 
> 
> Not sure this is what you want to do.
> The device would be plugged before it is inited :/.

I think this is exacly what we want to do.
In fact, this is what other buses do, because
devices simply can't init properly if they
do not know on which bus they reside.

E.g. with pci:
	do_pci_register_device (adds device on bus)
	init

We can add an analog of hotplug bus callback
if bus wants to get notified after device initialization.
I don't see a need for this though.
Do you?



-- 
MST

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[KONRAD Frédéric]

On 29/04/2013 19:52, Michael S. Tsirkin wrote:
> On Mon, Apr 29, 2013 at 07:23:49PM +0200, KONRAD Frédéric wrote:
>> On 29/04/2013 19:01, Michael S. Tsirkin wrote:
>>
>>      On Mon, Apr 29, 2013 at 06:41:08PM +0200, KONRAD Frédéric wrote:
>>
>>          On 29/04/2013 18:30, Michael S. Tsirkin wrote:
>>
>>              On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin wrote:
>>
>>                  On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric wrote:
>>
>>                      On 29/04/2013 18:02, Michael S. Tsirkin wrote:
>>
>>                          On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
>>
>>                              On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
>>
>>                                  On 29/04/2013 17:14, Jesse Larrew wrote:
>>
>>                                      On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
>>
>>                                          On 29/04/2013 16:42, Jesse Larrew wrote:
>>
>>                                          On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
>>
>>                                          On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
>>
>>                                          Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>>                                          calculate config size based on the host features. But it forgets the
>>                                          VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>>                                          len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>>                                          disabled form command line. Then qemu will crash when user tries to read the
>>                                          config of virtio-net.
>>
>>                                          Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>>                                          the mac address.
>>
>>                                          Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>>                                          Signed-off-by: Jason Wang <jasowang@redhat.com>
>>                                          ---
>>                                             hw/net/virtio-net.c |    3 ++-
>>                                             1 files changed, 2 insertions(+), 1 deletions(-)
>>
>>                                          diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>                                          index 70c8fce..33a70ef 100644
>>                                          --- a/hw/net/virtio-net.c
>>                                          +++ b/hw/net/virtio-net.c
>>                                          @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>                                               void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>>                                             {
>>                                          -    int i, config_size = 0;
>>                                          +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>>                                          +    int i, config_size = feature_sizes[0].end;
>>
>>                                          This would be cleaner:
>>                                               host_features |= (1 << VIRTIO_NET_F_MAC);
>>
>>                                          no need for a comment then.
>>
>>
>>                                          It seems to me that the real problem here is that host_features isn't
>>                                          properly populated before virtio_net_set_config_size() is called. Looking
>>                                          at virtio_device_init(), we can see why:
>>
>>                                          static int virtio_device_init(DeviceState *qdev)
>>                                          {
>>                                                VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>                                                VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>                                                assert(k->init != NULL);
>>                                                if (k->init(vdev) < 0) {
>>                                                    return -1;
>>                                                }
>>                                                virtio_bus_plug_device(vdev);
>>                                                return 0;
>>                                          }
>>
>>                                          virtio_net_set_config_size() is currently being called as part of the
>>                                          k->init call, but the host_features aren't properly setup until the device
>>                                          is plugged into the bus using virtio_bus_plug_device().
>>
>>                                          After talking with mdroth, I think the proper way to fix this would be to
>>                                          extend VirtioDeviceClass to include a calculate_config_size() method that
>>                                          can be called at the appropriate time during device initialization like so:
>>
>>                                          static int virtio_device_init(DeviceState *qdev)
>>                                          {
>>                                                VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>                                                VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>                                                assert(k->init != NULL);
>>                                                if (k->init(vdev) < 0) {
>>                                                    return -1;
>>                                                }
>>                                                virtio_bus_plug_device(vdev);
>>                                          +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
>>                                          +       return -1;
>>                                          +   }
>>                                                return 0;
>>                                          }
>>
>>                                          This would ensure that host_features contains the proper data before any
>>                                          devices try to make use of it to calculate the config size.
>>
>>                                          Good point, I didn't saw that.
>>
>>                                          but this was not the case with commit 14f9b664 no?
>>
>>
>>                                      I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
>>                                      it yet. I'll confirm this afternoon.
>>
>>                                  Ok, I think there is a problem with your patch:
>>
>>                                      virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
>>                                                                    n->config_size);
>>
>>                                  is called in virtio_net_device_init (k->init).
>>
>>                                  Is there a way to resize the config after that?
>>
>>
>>                              Nope. That's definitely a bug. I can send a patch to fix this today. Any
>>                              objection to the method suggested above (extending VirtioDeviceClass)?
>>
>>                          This needs more thought. All devices expected everything
>>                          is setup during the init call. config size is
>>                          likely not the only issue.
>>
>>                          So we need almost all of virtio_bus_plug_device before init,
>>                          and then after init send the signal:
>>
>>                              if (klass->device_plugged != NULL) {
>>                                  klass->device_plugged(qbus->parent);
>>                              }
>>
>>                      Seems the interesting part is in virtio_pci_device_plugged at the end:
>>
>>                          proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
>>                          proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
>>                          proxy->host_features = virtio_bus_get_vdev_features(bus,
>>                      proxy->host_features);
>>
>>                      This is and was called after set_config_size, isn't that the issue?
>>
>>                  Basically devices expected everything to be setup at
>>                  the point where init is called.
>>                  We found one bug but let's fix it properly.
>>                  There's no reason to delay parts of setup until after init,
>>                  if we do, we'll trip on more uses of uninitialized data.
>>
>>
>>                                                 for (i = 0; feature_sizes[i].flags != 0; i++) {
>>                                                     if (host_features & feature_sizes[i].flags) {
>>                                                         config_size = MAX(feature_sizes[i].end, config_size);
>>                                          --
>>                                          1.7.1
>>
>>                                      Jesse Larrew
>>                                      Software Engineer, KVM Team
>>                                      IBM Linux Technology Center
>>                                      Phone: (512) 973-2052 (T/L: 363-2052)
>>                                      jlarrew@linux.vnet.ibm.com
>>
>>
>>                              --
>>
>>                              Jesse Larrew
>>                              Software Engineer, KVM Team
>>                              IBM Linux Technology Center
>>                              Phone: (512) 973-2052 (T/L: 363-2052)
>>                              jlarrew@linux.vnet.ibm.com
>>
>>              Basically this is what I propose. Warning! compile-tested
>>              only. (I also dropped an unused return value).
>>
>>
>>              Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>>          Which tree are you using? Is it up to date?
>>
>>      Heh, more changes came in.  So now, it's even simpler:
>>
>>      Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>>      ---
>>
>>      diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
>>      index aab72ff..447ba16 100644
>>      --- a/hw/virtio/virtio-bus.c
>>      +++ b/hw/virtio/virtio-bus.c
>>      @@ -38,7 +38,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>>       #endif
>>
>>       /* Plug the VirtIODevice */
>>      -int virtio_bus_plug_device(VirtIODevice *vdev)
>>      +void virtio_bus_plug_device(VirtIODevice *vdev)
>>       {
>>           DeviceState *qdev = DEVICE(vdev);
>>           BusState *qbus = BUS(qdev_get_parent_bus(qdev));
>>      @@ -51,8 +51,6 @@ int virtio_bus_plug_device(VirtIODevice *vdev)
>>           if (klass->device_plugged != NULL) {
>>               klass->device_plugged(qbus->parent);
>>           }
>>      -
>>      -    return 0;
>>       }
>>
>>       /* Reset the virtio_bus */
>>      diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>      index 0f88c25..c5228e6 100644
>>      --- a/hw/virtio/virtio.c
>>      +++ b/hw/virtio/virtio.c
>>      @@ -1091,11 +1091,11 @@ static int virtio_device_init(DeviceState *qdev)
>>       {
>>           VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>           VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>      +    virtio_bus_plug_device(vdev);
>>           assert(k->init != NULL);
>>           if (k->init(vdev) < 0) {
>>               return -1;
>>           }
>>      -    virtio_bus_plug_device(vdev);
>>           return 0;
>>       }
>>
>>
>> Not sure this is what you want to do.
>> The device would be plugged before it is inited :/.
> I think this is exacly what we want to do.
> In fact, this is what other buses do, because
> devices simply can't init properly if they
> do not know on which bus they reside.
>
> E.g. with pci:
> 	do_pci_register_device (adds device on bus)
> 	init
>
> We can add an analog of hotplug bus callback
> if bus wants to get notified after device initialization.
> I don't see a need for this though.
> Do you?
>
>
>
No, as we don't want to allow virtio-device hotplug.

but look at the plugged callback in virtio-pci:

     pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));

     proxy->host_features = virtio_bus_get_vdev_features(bus,
proxy->host_features);

are impossible before the virtio-net init.

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Michael S. Tsirkin]

On Mon, Apr 29, 2013 at 08:01:52PM +0200, KONRAD Frédéric wrote:
> On 29/04/2013 19:52, Michael S. Tsirkin wrote:
> >On Mon, Apr 29, 2013 at 07:23:49PM +0200, KONRAD Frédéric wrote:
> >>On 29/04/2013 19:01, Michael S. Tsirkin wrote:
> >>
> >>     On Mon, Apr 29, 2013 at 06:41:08PM +0200, KONRAD Frédéric wrote:
> >>
> >>         On 29/04/2013 18:30, Michael S. Tsirkin wrote:
> >>
> >>             On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin wrote:
> >>
> >>                 On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric wrote:
> >>
> >>                     On 29/04/2013 18:02, Michael S. Tsirkin wrote:
> >>
> >>                         On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
> >>
> >>                             On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
> >>
> >>                                 On 29/04/2013 17:14, Jesse Larrew wrote:
> >>
> >>                                     On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
> >>
> >>                                         On 29/04/2013 16:42, Jesse Larrew wrote:
> >>
> >>                                         On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
> >>
> >>                                         On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
> >>
> >>                                         Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
> >>                                         calculate config size based on the host features. But it forgets the
> >>                                         VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
> >>                                         len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
> >>                                         disabled form command line. Then qemu will crash when user tries to read the
> >>                                         config of virtio-net.
> >>
> >>                                         Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
> >>                                         the mac address.
> >>
> >>                                         Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
> >>                                         Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>                                         ---
> >>                                            hw/net/virtio-net.c |    3 ++-
> >>                                            1 files changed, 2 insertions(+), 1 deletions(-)
> >>
> >>                                         diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>                                         index 70c8fce..33a70ef 100644
> >>                                         --- a/hw/net/virtio-net.c
> >>                                         +++ b/hw/net/virtio-net.c
> >>                                         @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
> >>                                              void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
> >>                                            {
> >>                                         -    int i, config_size = 0;
> >>                                         +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
> >>                                         +    int i, config_size = feature_sizes[0].end;
> >>
> >>                                         This would be cleaner:
> >>                                              host_features |= (1 << VIRTIO_NET_F_MAC);
> >>
> >>                                         no need for a comment then.
> >>
> >>
> >>                                         It seems to me that the real problem here is that host_features isn't
> >>                                         properly populated before virtio_net_set_config_size() is called. Looking
> >>                                         at virtio_device_init(), we can see why:
> >>
> >>                                         static int virtio_device_init(DeviceState *qdev)
> >>                                         {
> >>                                               VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> >>                                               VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> >>                                               assert(k->init != NULL);
> >>                                               if (k->init(vdev) < 0) {
> >>                                                   return -1;
> >>                                               }
> >>                                               virtio_bus_plug_device(vdev);
> >>                                               return 0;
> >>                                         }
> >>
> >>                                         virtio_net_set_config_size() is currently being called as part of the
> >>                                         k->init call, but the host_features aren't properly setup until the device
> >>                                         is plugged into the bus using virtio_bus_plug_device().
> >>
> >>                                         After talking with mdroth, I think the proper way to fix this would be to
> >>                                         extend VirtioDeviceClass to include a calculate_config_size() method that
> >>                                         can be called at the appropriate time during device initialization like so:
> >>
> >>                                         static int virtio_device_init(DeviceState *qdev)
> >>                                         {
> >>                                               VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> >>                                               VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> >>                                               assert(k->init != NULL);
> >>                                               if (k->init(vdev) < 0) {
> >>                                                   return -1;
> >>                                               }
> >>                                               virtio_bus_plug_device(vdev);
> >>                                         +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
> >>                                         +       return -1;
> >>                                         +   }
> >>                                               return 0;
> >>                                         }
> >>
> >>                                         This would ensure that host_features contains the proper data before any
> >>                                         devices try to make use of it to calculate the config size.
> >>
> >>                                         Good point, I didn't saw that.
> >>
> >>                                         but this was not the case with commit 14f9b664 no?
> >>
> >>
> >>                                     I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
> >>                                     it yet. I'll confirm this afternoon.
> >>
> >>                                 Ok, I think there is a problem with your patch:
> >>
> >>                                     virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
> >>                                                                   n->config_size);
> >>
> >>                                 is called in virtio_net_device_init (k->init).
> >>
> >>                                 Is there a way to resize the config after that?
> >>
> >>
> >>                             Nope. That's definitely a bug. I can send a patch to fix this today. Any
> >>                             objection to the method suggested above (extending VirtioDeviceClass)?
> >>
> >>                         This needs more thought. All devices expected everything
> >>                         is setup during the init call. config size is
> >>                         likely not the only issue.
> >>
> >>                         So we need almost all of virtio_bus_plug_device before init,
> >>                         and then after init send the signal:
> >>
> >>                             if (klass->device_plugged != NULL) {
> >>                                 klass->device_plugged(qbus->parent);
> >>                             }
> >>
> >>                     Seems the interesting part is in virtio_pci_device_plugged at the end:
> >>
> >>                         proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> >>                         proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
> >>                         proxy->host_features = virtio_bus_get_vdev_features(bus,
> >>                     proxy->host_features);
> >>
> >>                     This is and was called after set_config_size, isn't that the issue?
> >>
> >>                 Basically devices expected everything to be setup at
> >>                 the point where init is called.
> >>                 We found one bug but let's fix it properly.
> >>                 There's no reason to delay parts of setup until after init,
> >>                 if we do, we'll trip on more uses of uninitialized data.
> >>
> >>
> >>                                                for (i = 0; feature_sizes[i].flags != 0; i++) {
> >>                                                    if (host_features & feature_sizes[i].flags) {
> >>                                                        config_size = MAX(feature_sizes[i].end, config_size);
> >>                                         --
> >>                                         1.7.1
> >>
> >>                                     Jesse Larrew
> >>                                     Software Engineer, KVM Team
> >>                                     IBM Linux Technology Center
> >>                                     Phone: (512) 973-2052 (T/L: 363-2052)
> >>                                     jlarrew@linux.vnet.ibm.com
> >>
> >>
> >>                             --
> >>
> >>                             Jesse Larrew
> >>                             Software Engineer, KVM Team
> >>                             IBM Linux Technology Center
> >>                             Phone: (512) 973-2052 (T/L: 363-2052)
> >>                             jlarrew@linux.vnet.ibm.com
> >>
> >>             Basically this is what I propose. Warning! compile-tested
> >>             only. (I also dropped an unused return value).
> >>
> >>
> >>             Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >>         Which tree are you using? Is it up to date?
> >>
> >>     Heh, more changes came in.  So now, it's even simpler:
> >>
> >>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >>     ---
> >>
> >>     diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> >>     index aab72ff..447ba16 100644
> >>     --- a/hw/virtio/virtio-bus.c
> >>     +++ b/hw/virtio/virtio-bus.c
> >>     @@ -38,7 +38,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
> >>      #endif
> >>
> >>      /* Plug the VirtIODevice */
> >>     -int virtio_bus_plug_device(VirtIODevice *vdev)
> >>     +void virtio_bus_plug_device(VirtIODevice *vdev)
> >>      {
> >>          DeviceState *qdev = DEVICE(vdev);
> >>          BusState *qbus = BUS(qdev_get_parent_bus(qdev));
> >>     @@ -51,8 +51,6 @@ int virtio_bus_plug_device(VirtIODevice *vdev)
> >>          if (klass->device_plugged != NULL) {
> >>              klass->device_plugged(qbus->parent);
> >>          }
> >>     -
> >>     -    return 0;
> >>      }
> >>
> >>      /* Reset the virtio_bus */
> >>     diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >>     index 0f88c25..c5228e6 100644
> >>     --- a/hw/virtio/virtio.c
> >>     +++ b/hw/virtio/virtio.c
> >>     @@ -1091,11 +1091,11 @@ static int virtio_device_init(DeviceState *qdev)
> >>      {
> >>          VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> >>          VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> >>     +    virtio_bus_plug_device(vdev);
> >>          assert(k->init != NULL);
> >>          if (k->init(vdev) < 0) {
> >>              return -1;
> >>          }
> >>     -    virtio_bus_plug_device(vdev);
> >>          return 0;
> >>      }
> >>
> >>
> >>Not sure this is what you want to do.
> >>The device would be plugged before it is inited :/.
> >I think this is exacly what we want to do.
> >In fact, this is what other buses do, because
> >devices simply can't init properly if they
> >do not know on which bus they reside.
> >
> >E.g. with pci:
> >	do_pci_register_device (adds device on bus)
> >	init
> >
> >We can add an analog of hotplug bus callback
> >if bus wants to get notified after device initialization.
> >I don't see a need for this though.
> >Do you?
> >
> >
> >
> No, as we don't want to allow virtio-device hotplug.
> 
> but look at the plugged callback in virtio-pci:
> 
>     pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
> 
>     proxy->host_features = virtio_bus_get_vdev_features(bus,
> proxy->host_features);
> 
> are impossible before the virtio-net init.


At this point I have to admit I don't understand what's
going on any more.

virtio_net_instance_init sets config size to
some value, then virtio_net_set_config_size overrides it...
Help!

I am guessing it's this hack that backfires somehow.

-- 
MST

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[KONRAD Frédéric]

[-- Attachment #1: Type: text/plain, Size: 14924 bytes --]

On 29/04/2013 20:15, Michael S. Tsirkin wrote:
> On Mon, Apr 29, 2013 at 08:01:52PM +0200, KONRAD Frédéric wrote:
>> On 29/04/2013 19:52, Michael S. Tsirkin wrote:
>>> On Mon, Apr 29, 2013 at 07:23:49PM +0200, KONRAD Frédéric wrote:
>>>> On 29/04/2013 19:01, Michael S. Tsirkin wrote:
>>>>
>>>>      On Mon, Apr 29, 2013 at 06:41:08PM +0200, KONRAD Frédéric wrote:
>>>>
>>>>          On 29/04/2013 18:30, Michael S. Tsirkin wrote:
>>>>
>>>>              On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin wrote:
>>>>
>>>>                  On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric wrote:
>>>>
>>>>                      On 29/04/2013 18:02, Michael S. Tsirkin wrote:
>>>>
>>>>                          On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
>>>>
>>>>                              On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
>>>>
>>>>                                  On 29/04/2013 17:14, Jesse Larrew wrote:
>>>>
>>>>                                      On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
>>>>
>>>>                                          On 29/04/2013 16:42, Jesse Larrew wrote:
>>>>
>>>>                                          On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
>>>>
>>>>                                          On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
>>>>
>>>>                                          Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>>>>                                          calculate config size based on the host features. But it forgets the
>>>>                                          VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>>>>                                          len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>>>>                                          disabled form command line. Then qemu will crash when user tries to read the
>>>>                                          config of virtio-net.
>>>>
>>>>                                          Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>>>>                                          the mac address.
>>>>
>>>>                                          Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>>>>                                          Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>                                          ---
>>>>                                             hw/net/virtio-net.c |    3 ++-
>>>>                                             1 files changed, 2 insertions(+), 1 deletions(-)
>>>>
>>>>                                          diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>                                          index 70c8fce..33a70ef 100644
>>>>                                          --- a/hw/net/virtio-net.c
>>>>                                          +++ b/hw/net/virtio-net.c
>>>>                                          @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>>>                                               void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>>>>                                             {
>>>>                                          -    int i, config_size = 0;
>>>>                                          +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>>>>                                          +    int i, config_size = feature_sizes[0].end;
>>>>
>>>>                                          This would be cleaner:
>>>>                                               host_features |= (1 << VIRTIO_NET_F_MAC);
>>>>
>>>>                                          no need for a comment then.
>>>>
>>>>
>>>>                                          It seems to me that the real problem here is that host_features isn't
>>>>                                          properly populated before virtio_net_set_config_size() is called. Looking
>>>>                                          at virtio_device_init(), we can see why:
>>>>
>>>>                                          static int virtio_device_init(DeviceState *qdev)
>>>>                                          {
>>>>                                                VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>>                                                VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>>                                                assert(k->init != NULL);
>>>>                                                if (k->init(vdev) < 0) {
>>>>                                                    return -1;
>>>>                                                }
>>>>                                                virtio_bus_plug_device(vdev);
>>>>                                                return 0;
>>>>                                          }
>>>>
>>>>                                          virtio_net_set_config_size() is currently being called as part of the
>>>>                                          k->init call, but the host_features aren't properly setup until the device
>>>>                                          is plugged into the bus using virtio_bus_plug_device().
>>>>
>>>>                                          After talking with mdroth, I think the proper way to fix this would be to
>>>>                                          extend VirtioDeviceClass to include a calculate_config_size() method that
>>>>                                          can be called at the appropriate time during device initialization like so:
>>>>
>>>>                                          static int virtio_device_init(DeviceState *qdev)
>>>>                                          {
>>>>                                                VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>>                                                VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>>                                                assert(k->init != NULL);
>>>>                                                if (k->init(vdev) < 0) {
>>>>                                                    return -1;
>>>>                                                }
>>>>                                                virtio_bus_plug_device(vdev);
>>>>                                          +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
>>>>                                          +       return -1;
>>>>                                          +   }
>>>>                                                return 0;
>>>>                                          }
>>>>
>>>>                                          This would ensure that host_features contains the proper data before any
>>>>                                          devices try to make use of it to calculate the config size.
>>>>
>>>>                                          Good point, I didn't saw that.
>>>>
>>>>                                          but this was not the case with commit 14f9b664 no?
>>>>
>>>>
>>>>                                      I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
>>>>                                      it yet. I'll confirm this afternoon.
>>>>
>>>>                                  Ok, I think there is a problem with your patch:
>>>>
>>>>                                      virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
>>>>                                                                    n->config_size);
>>>>
>>>>                                  is called in virtio_net_device_init (k->init).
>>>>
>>>>                                  Is there a way to resize the config after that?
>>>>
>>>>
>>>>                              Nope. That's definitely a bug. I can send a patch to fix this today. Any
>>>>                              objection to the method suggested above (extending VirtioDeviceClass)?
>>>>
>>>>                          This needs more thought. All devices expected everything
>>>>                          is setup during the init call. config size is
>>>>                          likely not the only issue.
>>>>
>>>>                          So we need almost all of virtio_bus_plug_device before init,
>>>>                          and then after init send the signal:
>>>>
>>>>                              if (klass->device_plugged != NULL) {
>>>>                                  klass->device_plugged(qbus->parent);
>>>>                              }
>>>>
>>>>                      Seems the interesting part is in virtio_pci_device_plugged at the end:
>>>>
>>>>                          proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
>>>>                          proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
>>>>                          proxy->host_features = virtio_bus_get_vdev_features(bus,
>>>>                      proxy->host_features);
>>>>
>>>>                      This is and was called after set_config_size, isn't that the issue?
>>>>
>>>>                  Basically devices expected everything to be setup at
>>>>                  the point where init is called.
>>>>                  We found one bug but let's fix it properly.
>>>>                  There's no reason to delay parts of setup until after init,
>>>>                  if we do, we'll trip on more uses of uninitialized data.
>>>>
>>>>
>>>>                                                 for (i = 0; feature_sizes[i].flags != 0; i++) {
>>>>                                                     if (host_features & feature_sizes[i].flags) {
>>>>                                                         config_size = MAX(feature_sizes[i].end, config_size);
>>>>                                          --
>>>>                                          1.7.1
>>>>
>>>>                                      Jesse Larrew
>>>>                                      Software Engineer, KVM Team
>>>>                                      IBM Linux Technology Center
>>>>                                      Phone: (512) 973-2052 (T/L: 363-2052)
>>>>                                      jlarrew@linux.vnet.ibm.com
>>>>
>>>>
>>>>                              --
>>>>
>>>>                              Jesse Larrew
>>>>                              Software Engineer, KVM Team
>>>>                              IBM Linux Technology Center
>>>>                              Phone: (512) 973-2052 (T/L: 363-2052)
>>>>                              jlarrew@linux.vnet.ibm.com
>>>>
>>>>              Basically this is what I propose. Warning! compile-tested
>>>>              only. (I also dropped an unused return value).
>>>>
>>>>
>>>>              Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>
>>>>          Which tree are you using? Is it up to date?
>>>>
>>>>      Heh, more changes came in.  So now, it's even simpler:
>>>>
>>>>      Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>
>>>>      ---
>>>>
>>>>      diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
>>>>      index aab72ff..447ba16 100644
>>>>      --- a/hw/virtio/virtio-bus.c
>>>>      +++ b/hw/virtio/virtio-bus.c
>>>>      @@ -38,7 +38,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>>>>       #endif
>>>>
>>>>       /* Plug the VirtIODevice */
>>>>      -int virtio_bus_plug_device(VirtIODevice *vdev)
>>>>      +void virtio_bus_plug_device(VirtIODevice *vdev)
>>>>       {
>>>>           DeviceState *qdev = DEVICE(vdev);
>>>>           BusState *qbus = BUS(qdev_get_parent_bus(qdev));
>>>>      @@ -51,8 +51,6 @@ int virtio_bus_plug_device(VirtIODevice *vdev)
>>>>           if (klass->device_plugged != NULL) {
>>>>               klass->device_plugged(qbus->parent);
>>>>           }
>>>>      -
>>>>      -    return 0;
>>>>       }
>>>>
>>>>       /* Reset the virtio_bus */
>>>>      diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>>>      index 0f88c25..c5228e6 100644
>>>>      --- a/hw/virtio/virtio.c
>>>>      +++ b/hw/virtio/virtio.c
>>>>      @@ -1091,11 +1091,11 @@ static int virtio_device_init(DeviceState *qdev)
>>>>       {
>>>>           VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>>           VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>>      +    virtio_bus_plug_device(vdev);
>>>>           assert(k->init != NULL);
>>>>           if (k->init(vdev) < 0) {
>>>>               return -1;
>>>>           }
>>>>      -    virtio_bus_plug_device(vdev);
>>>>           return 0;
>>>>       }
>>>>
>>>>
>>>> Not sure this is what you want to do.
>>>> The device would be plugged before it is inited :/.
>>> I think this is exacly what we want to do.
>>> In fact, this is what other buses do, because
>>> devices simply can't init properly if they
>>> do not know on which bus they reside.
>>>
>>> E.g. with pci:
>>> 	do_pci_register_device (adds device on bus)
>>> 	init
>>>
>>> We can add an analog of hotplug bus callback
>>> if bus wants to get notified after device initialization.
>>> I don't see a need for this though.
>>> Do you?
>>>
>>>
>>>
>> No, as we don't want to allow virtio-device hotplug.
>>
>> but look at the plugged callback in virtio-pci:
>>
>>      pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
>>
>>      proxy->host_features = virtio_bus_get_vdev_features(bus,
>> proxy->host_features);
>>
>> are impossible before the virtio-net init.
>
> At this point I have to admit I don't understand what's
> going on any more.
>
> virtio_net_instance_init sets config size to
> some value, then virtio_net_set_config_size overrides it...
> Help!
>
> I am guessing it's this hack that backfires somehow.
>

It would be interferring if virtio_net_set_config_size was not called.

The best I think is to set the config size in the virtio_net_init 
function, then remove the instance init.

The issue is that in the init function, the host_feature is not 
completely computed:

     it lacks the three line in virtio pci plugged function.

Maybe we can move the two firsts line in the virtio_net_pci_init before 
the init if they are usefull in the
config_size computing:

		proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
                 proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;

Then compute the last one directly in the init function which is the harder:

     virtio_net_get_features

Note that there is in virtio_net_get_features:

                              features |= (1 << VIRTIO_NET_F_MAC);

Which is exacty the issue the initial patch is solving.

Fred

[-- Attachment #2: Type: text/html, Size: 15131 bytes --]

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Michael S. Tsirkin]

On Mon, Apr 29, 2013 at 08:45:50PM +0200, KONRAD Frédéric wrote:
> On 29/04/2013 20:15, Michael S. Tsirkin wrote:
> 
>     On Mon, Apr 29, 2013 at 08:01:52PM +0200, KONRAD Frédéric wrote:
> 
>         On 29/04/2013 19:52, Michael S. Tsirkin wrote:
> 
>             On Mon, Apr 29, 2013 at 07:23:49PM +0200, KONRAD Frédéric wrote:
> 
>                 On 29/04/2013 19:01, Michael S. Tsirkin wrote:
> 
>                     On Mon, Apr 29, 2013 at 06:41:08PM +0200, KONRAD Frédéric wrote:
> 
>                         On 29/04/2013 18:30, Michael S. Tsirkin wrote:
> 
>                             On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin wrote:
> 
>                                 On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric wrote:
> 
>                                     On 29/04/2013 18:02, Michael S. Tsirkin wrote:
> 
>                                         On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
> 
>                                             On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
> 
>                                                 On 29/04/2013 17:14, Jesse Larrew wrote:
> 
>                                                     On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
> 
>                                                         On 29/04/2013 16:42, Jesse Larrew wrote:
> 
>                                                         On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
> 
>                                                         On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
> 
>                                                         Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>                                                         calculate config size based on the host features. But it forgets the
>                                                         VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>                                                         len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>                                                         disabled form command line. Then qemu will crash when user tries to read the
>                                                         config of virtio-net.
> 
>                                                         Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>                                                         the mac address.
> 
>                                                         Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>                                                         Signed-off-by: Jason Wang <jasowang@redhat.com>
>                                                         ---
>                                                            hw/net/virtio-net.c |    3 ++-
>                                                            1 files changed, 2 insertions(+), 1 deletions(-)
> 
>                                                         diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>                                                         index 70c8fce..33a70ef 100644
>                                                         --- a/hw/net/virtio-net.c
>                                                         +++ b/hw/net/virtio-net.c
>                                                         @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>                                                              void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>                                                            {
>                                                         -    int i, config_size = 0;
>                                                         +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>                                                         +    int i, config_size = feature_sizes[0].end;
> 
>                                                         This would be cleaner:
>                                                              host_features |= (1 << VIRTIO_NET_F_MAC);
> 
>                                                         no need for a comment then.
> 
> 
>                                                         It seems to me that the real problem here is that host_features isn't
>                                                         properly populated before virtio_net_set_config_size() is called. Looking
>                                                         at virtio_device_init(), we can see why:
> 
>                                                         static int virtio_device_init(DeviceState *qdev)
>                                                         {
>                                                               VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>                                                               VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>                                                               assert(k->init != NULL);
>                                                               if (k->init(vdev) < 0) {
>                                                                   return -1;
>                                                               }
>                                                               virtio_bus_plug_device(vdev);
>                                                               return 0;
>                                                         }
> 
>                                                         virtio_net_set_config_size() is currently being called as part of the
>                                                         k->init call, but the host_features aren't properly setup until the device
>                                                         is plugged into the bus using virtio_bus_plug_device().
> 
>                                                         After talking with mdroth, I think the proper way to fix this would be to
>                                                         extend VirtioDeviceClass to include a calculate_config_size() method that
>                                                         can be called at the appropriate time during device initialization like so:
> 
>                                                         static int virtio_device_init(DeviceState *qdev)
>                                                         {
>                                                               VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>                                                               VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>                                                               assert(k->init != NULL);
>                                                               if (k->init(vdev) < 0) {
>                                                                   return -1;
>                                                               }
>                                                               virtio_bus_plug_device(vdev);
>                                                         +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
>                                                         +       return -1;
>                                                         +   }
>                                                               return 0;
>                                                         }
> 
>                                                         This would ensure that host_features contains the proper data before any
>                                                         devices try to make use of it to calculate the config size.
> 
>                                                         Good point, I didn't saw that.
> 
>                                                         but this was not the case with commit 14f9b664 no?
> 
> 
>                                                     I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
>                                                     it yet. I'll confirm this afternoon.
> 
>                                                 Ok, I think there is a problem with your patch:
> 
>                                                     virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
>                                                                                   n->config_size);
> 
>                                                 is called in virtio_net_device_init (k->init).
> 
>                                                 Is there a way to resize the config after that?
> 
> 
>                                             Nope. That's definitely a bug. I can send a patch to fix this today. Any
>                                             objection to the method suggested above (extending VirtioDeviceClass)?
> 
>                                         This needs more thought. All devices expected everything
>                                         is setup during the init call. config size is
>                                         likely not the only issue.
> 
>                                         So we need almost all of virtio_bus_plug_device before init,
>                                         and then after init send the signal:
> 
>                                             if (klass->device_plugged != NULL) {
>                                                 klass->device_plugged(qbus->parent);
>                                             }
> 
>                                     Seems the interesting part is in virtio_pci_device_plugged at the end:
> 
>                                         proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
>                                         proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
>                                         proxy->host_features = virtio_bus_get_vdev_features(bus,
>                                     proxy->host_features);
> 
>                                     This is and was called after set_config_size, isn't that the issue?
> 
>                                 Basically devices expected everything to be setup at
>                                 the point where init is called.
>                                 We found one bug but let's fix it properly.
>                                 There's no reason to delay parts of setup until after init,
>                                 if we do, we'll trip on more uses of uninitialized data.
> 
> 
>                                                                for (i = 0; feature_sizes[i].flags != 0; i++) {
>                                                                    if (host_features & feature_sizes[i].flags) {
>                                                                        config_size = MAX(feature_sizes[i].end, config_size);
>                                                         --
>                                                         1.7.1
> 
>                                                     Jesse Larrew
>                                                     Software Engineer, KVM Team
>                                                     IBM Linux Technology Center
>                                                     Phone: (512) 973-2052 (T/L: 363-2052)
>                                                     jlarrew@linux.vnet.ibm.com
> 
> 
>                                             --
> 
>                                             Jesse Larrew
>                                             Software Engineer, KVM Team
>                                             IBM Linux Technology Center
>                                             Phone: (512) 973-2052 (T/L: 363-2052)
>                                             jlarrew@linux.vnet.ibm.com
> 
>                             Basically this is what I propose. Warning! compile-tested
>                             only. (I also dropped an unused return value).
> 
> 
>                             Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
>                         Which tree are you using? Is it up to date?
> 
>                     Heh, more changes came in.  So now, it's even simpler:
> 
>                     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
>                     ---
> 
>                     diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
>                     index aab72ff..447ba16 100644
>                     --- a/hw/virtio/virtio-bus.c
>                     +++ b/hw/virtio/virtio-bus.c
>                     @@ -38,7 +38,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>                      #endif
> 
>                      /* Plug the VirtIODevice */
>                     -int virtio_bus_plug_device(VirtIODevice *vdev)
>                     +void virtio_bus_plug_device(VirtIODevice *vdev)
>                      {
>                          DeviceState *qdev = DEVICE(vdev);
>                          BusState *qbus = BUS(qdev_get_parent_bus(qdev));
>                     @@ -51,8 +51,6 @@ int virtio_bus_plug_device(VirtIODevice *vdev)
>                          if (klass->device_plugged != NULL) {
>                              klass->device_plugged(qbus->parent);
>                          }
>                     -
>                     -    return 0;
>                      }
> 
>                      /* Reset the virtio_bus */
>                     diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>                     index 0f88c25..c5228e6 100644
>                     --- a/hw/virtio/virtio.c
>                     +++ b/hw/virtio/virtio.c
>                     @@ -1091,11 +1091,11 @@ static int virtio_device_init(DeviceState *qdev)
>                      {
>                          VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>                          VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>                     +    virtio_bus_plug_device(vdev);
>                          assert(k->init != NULL);
>                          if (k->init(vdev) < 0) {
>                              return -1;
>                          }
>                     -    virtio_bus_plug_device(vdev);
>                          return 0;
>                      }
> 
> 
>                 Not sure this is what you want to do.
>                 The device would be plugged before it is inited :/.
> 
>             I think this is exacly what we want to do.
>             In fact, this is what other buses do, because
>             devices simply can't init properly if they
>             do not know on which bus they reside.
> 
>             E.g. with pci:
>                     do_pci_register_device (adds device on bus)
>                     init
> 
>             We can add an analog of hotplug bus callback
>             if bus wants to get notified after device initialization.
>             I don't see a need for this though.
>             Do you?
> 
> 
> 
> 
>         No, as we don't want to allow virtio-device hotplug.
> 
>         but look at the plugged callback in virtio-pci:
> 
>             pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
> 
>             proxy->host_features = virtio_bus_get_vdev_features(bus,
>         proxy->host_features);
> 
>         are impossible before the virtio-net init.
> 
> 
>     At this point I have to admit I don't understand what's
>     going on any more.
> 
>     virtio_net_instance_init sets config size to
>     some value, then virtio_net_set_config_size overrides it...
>     Help!
> 
>     I am guessing it's this hack that backfires somehow.
> 
> 
> 
> It would be interferring if virtio_net_set_config_size was not called.
> 
> The best I think is to set the config size in the virtio_net_init function,
> then remove the instance init.
> 
> The issue is that in the init function, the host_feature is not completely
> computed:
> 
>     it lacks the three line in virtio pci plugged function.
> 
> Maybe we can move the two firsts line in the virtio_net_pci_init before the
> init if they are usefull in the
> config_size computing:
> 
> 
>                 proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
>                 proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;

Bus can set VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_BAD_FEATURE
later, this is not going to affect anything.

Reason is, VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_BAD_FEATURE
are compatibility hacks which is why we dont
have them for s390. It's probably a bug that
we have them for virtio-ccw.

> 
> Then compute the last one directly in the init function which is the harder:
> 
>                         virtio_net_get_features

The real fix is to set features in init.

Can we move host_features to struct VirtIODevice, and
init to the device init function?

The reason we didn't do this initially is exactly
because we need to specify them in -device flag,
and there was no way to do this for VirtIODevice,
since it's the proxy that is instanciated.
Does the new bus infrastructure allow this?


> Note that there is in virtio_net_get_features:
> 
>                              features |= (1 << VIRTIO_NET_F_MAC);
> 
> Which is exacty the issue the initial patch is solving.
> 
> Fred

Yes. the lifetime of host features is as follows:

- configured in device by user, mostly set to "on" by default
- depending on device specific logic,
  override some features - mostly to "off",
  but we also force some required features to "on"
- Two exceptions (notify on empty+bad) are transport
  specific. they are also not user-controllable.


-- 
MST

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[KONRAD Frédéric]

On 29/04/2013 22:09, Michael S. Tsirkin wrote:
> On Mon, Apr 29, 2013 at 08:45:50PM +0200, KONRAD Frédéric wrote:
>> On 29/04/2013 20:15, Michael S. Tsirkin wrote:
>>
>>      On Mon, Apr 29, 2013 at 08:01:52PM +0200, KONRAD Frédéric wrote:
>>
>>          On 29/04/2013 19:52, Michael S. Tsirkin wrote:
>>
>>              On Mon, Apr 29, 2013 at 07:23:49PM +0200, KONRAD Frédéric wrote:
>>
>>                  On 29/04/2013 19:01, Michael S. Tsirkin wrote:
>>
>>                      On Mon, Apr 29, 2013 at 06:41:08PM +0200, KONRAD Frédéric wrote:
>>
>>                          On 29/04/2013 18:30, Michael S. Tsirkin wrote:
>>
>>                              On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin wrote:
>>
>>                                  On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric wrote:
>>
>>                                      On 29/04/2013 18:02, Michael S. Tsirkin wrote:
>>
>>                                          On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
>>
>>                                              On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
>>
>>                                                  On 29/04/2013 17:14, Jesse Larrew wrote:
>>
>>                                                      On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
>>
>>                                                          On 29/04/2013 16:42, Jesse Larrew wrote:
>>
>>                                                          On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
>>
>>                                                          On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
>>
>>                                                          Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>>                                                          calculate config size based on the host features. But it forgets the
>>                                                          VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>>                                                          len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>>                                                          disabled form command line. Then qemu will crash when user tries to read the
>>                                                          config of virtio-net.
>>
>>                                                          Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>>                                                          the mac address.
>>
>>                                                          Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>>                                                          Signed-off-by: Jason Wang <jasowang@redhat.com>
>>                                                          ---
>>                                                             hw/net/virtio-net.c |    3 ++-
>>                                                             1 files changed, 2 insertions(+), 1 deletions(-)
>>
>>                                                          diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>                                                          index 70c8fce..33a70ef 100644
>>                                                          --- a/hw/net/virtio-net.c
>>                                                          +++ b/hw/net/virtio-net.c
>>                                                          @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>                                                               void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>>                                                             {
>>                                                          -    int i, config_size = 0;
>>                                                          +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>>                                                          +    int i, config_size = feature_sizes[0].end;
>>
>>                                                          This would be cleaner:
>>                                                               host_features |= (1 << VIRTIO_NET_F_MAC);
>>
>>                                                          no need for a comment then.
>>
>>
>>                                                          It seems to me that the real problem here is that host_features isn't
>>                                                          properly populated before virtio_net_set_config_size() is called. Looking
>>                                                          at virtio_device_init(), we can see why:
>>
>>                                                          static int virtio_device_init(DeviceState *qdev)
>>                                                          {
>>                                                                VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>                                                                VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>                                                                assert(k->init != NULL);
>>                                                                if (k->init(vdev) < 0) {
>>                                                                    return -1;
>>                                                                }
>>                                                                virtio_bus_plug_device(vdev);
>>                                                                return 0;
>>                                                          }
>>
>>                                                          virtio_net_set_config_size() is currently being called as part of the
>>                                                          k->init call, but the host_features aren't properly setup until the device
>>                                                          is plugged into the bus using virtio_bus_plug_device().
>>
>>                                                          After talking with mdroth, I think the proper way to fix this would be to
>>                                                          extend VirtioDeviceClass to include a calculate_config_size() method that
>>                                                          can be called at the appropriate time during device initialization like so:
>>
>>                                                          static int virtio_device_init(DeviceState *qdev)
>>                                                          {
>>                                                                VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>                                                                VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>                                                                assert(k->init != NULL);
>>                                                                if (k->init(vdev) < 0) {
>>                                                                    return -1;
>>                                                                }
>>                                                                virtio_bus_plug_device(vdev);
>>                                                          +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
>>                                                          +       return -1;
>>                                                          +   }
>>                                                                return 0;
>>                                                          }
>>
>>                                                          This would ensure that host_features contains the proper data before any
>>                                                          devices try to make use of it to calculate the config size.
>>
>>                                                          Good point, I didn't saw that.
>>
>>                                                          but this was not the case with commit 14f9b664 no?
>>
>>
>>                                                      I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
>>                                                      it yet. I'll confirm this afternoon.
>>
>>                                                  Ok, I think there is a problem with your patch:
>>
>>                                                      virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
>>                                                                                    n->config_size);
>>
>>                                                  is called in virtio_net_device_init (k->init).
>>
>>                                                  Is there a way to resize the config after that?
>>
>>
>>                                              Nope. That's definitely a bug. I can send a patch to fix this today. Any
>>                                              objection to the method suggested above (extending VirtioDeviceClass)?
>>
>>                                          This needs more thought. All devices expected everything
>>                                          is setup during the init call. config size is
>>                                          likely not the only issue.
>>
>>                                          So we need almost all of virtio_bus_plug_device before init,
>>                                          and then after init send the signal:
>>
>>                                              if (klass->device_plugged != NULL) {
>>                                                  klass->device_plugged(qbus->parent);
>>                                              }
>>
>>                                      Seems the interesting part is in virtio_pci_device_plugged at the end:
>>
>>                                          proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
>>                                          proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
>>                                          proxy->host_features = virtio_bus_get_vdev_features(bus,
>>                                      proxy->host_features);
>>
>>                                      This is and was called after set_config_size, isn't that the issue?
>>
>>                                  Basically devices expected everything to be setup at
>>                                  the point where init is called.
>>                                  We found one bug but let's fix it properly.
>>                                  There's no reason to delay parts of setup until after init,
>>                                  if we do, we'll trip on more uses of uninitialized data.
>>
>>
>>                                                                 for (i = 0; feature_sizes[i].flags != 0; i++) {
>>                                                                     if (host_features & feature_sizes[i].flags) {
>>                                                                         config_size = MAX(feature_sizes[i].end, config_size);
>>                                                          --
>>                                                          1.7.1
>>
>>                                                      Jesse Larrew
>>                                                      Software Engineer, KVM Team
>>                                                      IBM Linux Technology Center
>>                                                      Phone: (512) 973-2052 (T/L: 363-2052)
>>                                                      jlarrew@linux.vnet.ibm.com
>>
>>
>>                                              --
>>
>>                                              Jesse Larrew
>>                                              Software Engineer, KVM Team
>>                                              IBM Linux Technology Center
>>                                              Phone: (512) 973-2052 (T/L: 363-2052)
>>                                              jlarrew@linux.vnet.ibm.com
>>
>>                              Basically this is what I propose. Warning! compile-tested
>>                              only. (I also dropped an unused return value).
>>
>>
>>                              Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>>                          Which tree are you using? Is it up to date?
>>
>>                      Heh, more changes came in.  So now, it's even simpler:
>>
>>                      Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>>                      ---
>>
>>                      diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
>>                      index aab72ff..447ba16 100644
>>                      --- a/hw/virtio/virtio-bus.c
>>                      +++ b/hw/virtio/virtio-bus.c
>>                      @@ -38,7 +38,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>>                       #endif
>>
>>                       /* Plug the VirtIODevice */
>>                      -int virtio_bus_plug_device(VirtIODevice *vdev)
>>                      +void virtio_bus_plug_device(VirtIODevice *vdev)
>>                       {
>>                           DeviceState *qdev = DEVICE(vdev);
>>                           BusState *qbus = BUS(qdev_get_parent_bus(qdev));
>>                      @@ -51,8 +51,6 @@ int virtio_bus_plug_device(VirtIODevice *vdev)
>>                           if (klass->device_plugged != NULL) {
>>                               klass->device_plugged(qbus->parent);
>>                           }
>>                      -
>>                      -    return 0;
>>                       }
>>
>>                       /* Reset the virtio_bus */
>>                      diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>                      index 0f88c25..c5228e6 100644
>>                      --- a/hw/virtio/virtio.c
>>                      +++ b/hw/virtio/virtio.c
>>                      @@ -1091,11 +1091,11 @@ static int virtio_device_init(DeviceState *qdev)
>>                       {
>>                           VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>                           VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>                      +    virtio_bus_plug_device(vdev);
>>                           assert(k->init != NULL);
>>                           if (k->init(vdev) < 0) {
>>                               return -1;
>>                           }
>>                      -    virtio_bus_plug_device(vdev);
>>                           return 0;
>>                       }
>>
>>
>>                  Not sure this is what you want to do.
>>                  The device would be plugged before it is inited :/.
>>
>>              I think this is exacly what we want to do.
>>              In fact, this is what other buses do, because
>>              devices simply can't init properly if they
>>              do not know on which bus they reside.
>>
>>              E.g. with pci:
>>                      do_pci_register_device (adds device on bus)
>>                      init
>>
>>              We can add an analog of hotplug bus callback
>>              if bus wants to get notified after device initialization.
>>              I don't see a need for this though.
>>              Do you?
>>
>>
>>
>>
>>          No, as we don't want to allow virtio-device hotplug.
>>
>>          but look at the plugged callback in virtio-pci:
>>
>>              pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
>>
>>              proxy->host_features = virtio_bus_get_vdev_features(bus,
>>          proxy->host_features);
>>
>>          are impossible before the virtio-net init.
>>
>>
>>      At this point I have to admit I don't understand what's
>>      going on any more.
>>
>>      virtio_net_instance_init sets config size to
>>      some value, then virtio_net_set_config_size overrides it...
>>      Help!
>>
>>      I am guessing it's this hack that backfires somehow.
>>
>>
>>
>> It would be interferring if virtio_net_set_config_size was not called.
>>
>> The best I think is to set the config size in the virtio_net_init function,
>> then remove the instance init.
>>
>> The issue is that in the init function, the host_feature is not completely
>> computed:
>>
>>      it lacks the three line in virtio pci plugged function.
>>
>> Maybe we can move the two firsts line in the virtio_net_pci_init before the
>> init if they are usefull in the
>> config_size computing:
>>
>>
>>                  proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
>>                  proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
> Bus can set VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_BAD_FEATURE
> later, this is not going to affect anything.
>
> Reason is, VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_BAD_FEATURE
> are compatibility hacks which is why we dont
> have them for s390. It's probably a bug that
> we have them for virtio-ccw.
>
>> Then compute the last one directly in the init function which is the harder:
>>
>>                          virtio_net_get_features
> The real fix is to set features in init.
>
> Can we move host_features to struct VirtIODevice, and
> init to the device init function?
>
> The reason we didn't do this initially is exactly
> because we need to specify them in -device flag,
> and there was no way to do this for VirtIODevice,
> since it's the proxy that is instanciated.
> Does the new bus infrastructure allow this?

Yes, I think it's possible for PCI and S390, but it seems more difficult 
for CCW.

I don't really understand how it's working with CCW devices, there is an
array of host_features?

Cornelia any idea?

>
>> Note that there is in virtio_net_get_features:
>>
>>                               features |= (1 << VIRTIO_NET_F_MAC);
>>
>> Which is exacty the issue the initial patch is solving.
>>
>> Fred
> Yes. the lifetime of host features is as follows:
>
> - configured in device by user, mostly set to "on" by default
> - depending on device specific logic,
>    override some features - mostly to "off",
>    but we also force some required features to "on"
> - Two exceptions (notify on empty+bad) are transport
>    specific. they are also not user-controllable.
>
>

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Michael S. Tsirkin]

On Tue, Apr 30, 2013 at 10:47:34AM +0200, KONRAD Frédéric wrote:
> >>Then compute the last one directly in the init function which is the harder:
> >>
> >>                         virtio_net_get_features
> >The real fix is to set features in init.
> >
> >Can we move host_features to struct VirtIODevice, and
> >init to the device init function?
> >
> >The reason we didn't do this initially is exactly
> >because we need to specify them in -device flag,
> >and there was no way to do this for VirtIODevice,
> >since it's the proxy that is instanciated.
> >Does the new bus infrastructure allow this?
> 
> Yes, I think it's possible for PCI and S390, but it seems more
> difficult for CCW.

Can you send the patch for pci to let everyone see what
you have in mind? The main issue is passing properties
from proxy to the device.

> I don't really understand how it's working with CCW devices, there is an
> array of host_features?

In practice the array is of size 1. I'd suggest just assuming that for now.
When we extend features (which will happen pretty soon)
we'll do this for all transports and change features to uint64_t
everywhere.
After that, we'll have a bit of breathing space.

> Cornelia any idea?
> 
> >
> >>Note that there is in virtio_net_get_features:
> >>
> >>                              features |= (1 << VIRTIO_NET_F_MAC);
> >>
> >>Which is exacty the issue the initial patch is solving.
> >>
> >>Fred
> >Yes. the lifetime of host features is as follows:
> >
> >- configured in device by user, mostly set to "on" by default
> >- depending on device specific logic,
> >   override some features - mostly to "off",
> >   but we also force some required features to "on"
> >- Two exceptions (notify on empty+bad) are transport
> >   specific. they are also not user-controllable.
> >
> >

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Cornelia Huck]

On Mon, 29 Apr 2013 23:09:52 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Apr 29, 2013 at 08:45:50PM +0200, KONRAD Frédéric wrote:

> > Maybe we can move the two firsts line in the virtio_net_pci_init before the
> > init if they are usefull in the
> > config_size computing:
> > 
> > 
> >                 proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> >                 proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
> 
> Bus can set VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_BAD_FEATURE
> later, this is not going to affect anything.
> 
> Reason is, VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_BAD_FEATURE
> are compatibility hacks which is why we dont
> have them for s390. It's probably a bug that
> we have them for virtio-ccw.
> 

Yes, probably (I used virtio-pci as my inspiration a lot...)

I'll check whether killing this works as expected.

Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Cornelia Huck]

On Tue, 30 Apr 2013 12:02:12 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Apr 30, 2013 at 10:47:34AM +0200, KONRAD Frédéric wrote:
> > >>Then compute the last one directly in the init function which is the harder:
> > >>
> > >>                         virtio_net_get_features
> > >The real fix is to set features in init.
> > >
> > >Can we move host_features to struct VirtIODevice, and
> > >init to the device init function?
> > >
> > >The reason we didn't do this initially is exactly
> > >because we need to specify them in -device flag,
> > >and there was no way to do this for VirtIODevice,
> > >since it's the proxy that is instanciated.
> > >Does the new bus infrastructure allow this?
> > 
> > Yes, I think it's possible for PCI and S390, but it seems more
> > difficult for CCW.
> 
> Can you send the patch for pci to let everyone see what
> you have in mind? The main issue is passing properties
> from proxy to the device.

Should probably not be hard to adapt to ccw.

> 
> > I don't really understand how it's working with CCW devices, there is an
> > array of host_features?
> 
> In practice the array is of size 1. I'd suggest just assuming that for now.
> When we extend features (which will happen pretty soon)
> we'll do this for all transports and change features to uint64_t
> everywhere.
> After that, we'll have a bit of breathing space.

Yes, the idea was to keep features easily extendable. Going 64 bit will
work fine; the transport should be able to handle even larger features.