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 <address@hidden>
>>>>>> Signed-off-by: Jason Wang <address@hidden>
>>>>>> ---
>>>>>>    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)
>> address@hidden
>>
> 


-- 

Jesse Larrew
Software Engineer, KVM Team
IBM Linux Technology Center
Phone: (512) 973-2052 (T/L: 363-2052)
address@hidden