From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49651) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWpUR-0004Bm-RL for qemu-devel@nongnu.org; Mon, 29 Apr 2013 10:55:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWpUQ-0003qi-Kt for qemu-devel@nongnu.org; Mon, 29 Apr 2013 10:55:39 -0400 Received: from greensocs.com ([87.106.252.221]:57827 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWpUQ-0003qX-9f for qemu-devel@nongnu.org; Mon, 29 Apr 2013 10:55:38 -0400 Message-ID: <517E89E4.7050304@greensocs.com> Date: Mon, 29 Apr 2013 16:55:32 +0200 From: =?ISO-8859-1?Q?KONRAD_Fr=E9d=E9ric?= MIME-Version: 1.0 References: <1366870889-950-1-git-send-email-jasowang@redhat.com> <20130425065953.GB7299@redhat.com> <517E86BA.3020800@linux.vnet.ibm.com> In-Reply-To: <517E86BA.3020800@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jesse Larrew Cc: qemu-devel@nongnu.org, Jason Wang , aliguori@us.ibm.com, Michael Roth , "Michael S. Tsirkin" 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 >>> Signed-off-by: Jason Wang >>> --- >>> 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 > >