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 <jlar...@linux.vnet.ibm.com>
>>>> Signed-off-by: Jason Wang <jasow...@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)
jlar...@linux.vnet.ibm.com
