From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55196) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWpmr-0007hb-Nj for qemu-devel@nongnu.org; Mon, 29 Apr 2013 11:14:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWpmp-0001oC-Tu for qemu-devel@nongnu.org; Mon, 29 Apr 2013 11:14:41 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:48581) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWpmp-0001o3-P2 for qemu-devel@nongnu.org; Mon, 29 Apr 2013 11:14:39 -0400 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 29 Apr 2013 11:14:38 -0400 Received: from d01relay01.pok.ibm.com (d01relay01.pok.ibm.com [9.56.227.233]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 318C76E8028 for ; Mon, 29 Apr 2013 11:14:33 -0400 (EDT) Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by d01relay01.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3TFEZCa260692 for ; Mon, 29 Apr 2013 11:14:36 -0400 Received: from d01av03.pok.ibm.com (loopback [127.0.0.1]) by d01av03.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r3TFEZss022498 for ; Mon, 29 Apr 2013 12:14:35 -0300 Message-ID: <517E8E5A.5050202@linux.vnet.ibm.com> Date: Mon, 29 Apr 2013 10:14:34 -0500 From: Jesse Larrew MIME-Version: 1.0 References: <1366870889-950-1-git-send-email-jasowang@redhat.com> <20130425065953.GB7299@redhat.com> <517E86BA.3020800@linux.vnet.ibm.com> <517E89E4.7050304@greensocs.com> In-Reply-To: <517E89E4.7050304@greensocs.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8bit 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: =?ISO-8859-1?Q?KONRAD_Fr=E9d=E9ric?= Cc: qemu-devel@nongnu.org, Jason Wang , aliguori@us.ibm.com, Michael Roth , "Michael S. Tsirkin" 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 >>>> 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? > 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