|
From: | KONRAD Frédéric |
Subject: | Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len |
Date: | Mon, 29 Apr 2013 16:55:32 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 |
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?
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.1Jesse Larrew Software Engineer, KVM Team IBM Linux Technology Center Phone: (512) 973-2052 (T/L: 363-2052) address@hidden
[Prev in Thread] | Current Thread | [Next in Thread] |