From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43695) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWqj5-0006nR-Pc for qemu-devel@nongnu.org; Mon, 29 Apr 2013 12:14:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWqj3-0005Ij-Hw for qemu-devel@nongnu.org; Mon, 29 Apr 2013 12:14:51 -0400 Received: from greensocs.com ([87.106.252.221]:50556 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWqj3-0005IF-8U for qemu-devel@nongnu.org; Mon, 29 Apr 2013 12:14:49 -0400 Message-ID: <517E9C71.1050300@greensocs.com> Date: Mon, 29 Apr 2013 18:14:41 +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> <517E89E4.7050304@greensocs.com> <517E8E5A.5050202@linux.vnet.ibm.com> <517E91CD.9080602@greensocs.com> <517E97F8.5000803@linux.vnet.ibm.com> <20130429160213.GA10663@redhat.com> In-Reply-To: <20130429160213.GA10663@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable 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: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org, Jason Wang , aliguori@us.ibm.com, Jesse Larrew , Michael Roth On 29/04/2013 18:02, Michael S. Tsirkin wrote: > On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote: >> On 04/29/2013 10:29 AM, KONRAD Fr=E9d=E9ric wrote: >>> On 29/04/2013 17:14, Jesse Larrew wrote: >>>> On 04/29/2013 09:55 AM, KONRAD Fr=E9d=E9ric 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 fea= tures) 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 VIRT= IO_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 a= t 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 hos= t_features) >>>>>>>> { >>>>>>>> - int i, config_size =3D 0; >>>>>>>> + /* VIRTIO_NET_F_MAC can't be disabled from qemu side */ >>>>>>>> + int i, config_size =3D feature_sizes[0].end; >>>>>>> This would be cleaner: >>>>>>> host_features |=3D (1 << VIRTIO_NET_F_MAC); >>>>>>> >>>>>>> no need for a comment then. >>>>>>> >>>>>> It seems to me that the real problem here is that host_features is= n'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 =3D VIRTIO_DEVICE(qdev); >>>>>> VirtioDeviceClass *k =3D VIRTIO_DEVICE_GET_CLASS(qdev); >>>>>> assert(k->init !=3D 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 th= e device >>>>>> is plugged into the bus using virtio_bus_plug_device(). >>>>>> >>>>>> After talking with mdroth, I think the proper way to fix this woul= d be to >>>>>> extend VirtioDeviceClass to include a calculate_config_size() meth= od that >>>>>> can be called at the appropriate time during device initialization= like so: >>>>>> >>>>>> static int virtio_device_init(DeviceState *qdev) >>>>>> { >>>>>> VirtIODevice *vdev =3D VIRTIO_DEVICE(qdev); >>>>>> VirtioDeviceClass *k =3D VIRTIO_DEVICE_GET_CLASS(qdev); >>>>>> assert(k->init !=3D 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 befo= re 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 t= riggered >>>> 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. A= ny >> objection to the method suggested above (extending VirtioDeviceClass)? > This needs more thought. All devices expected everything > is setup during the init call. config size is > likely not the only issue. > > So we need almost all of virtio_bus_plug_device before init, > and then after init send the signal: > > if (klass->device_plugged !=3D NULL) { > klass->device_plugged(qbus->parent); > } Seems the interesting part is in virtio_pci_device_plugged at the end: proxy->host_features |=3D 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY; proxy->host_features |=3D 0x1 << VIRTIO_F_BAD_FEATURE; proxy->host_features =3D virtio_bus_get_vdev_features(bus, proxy->host_features); This is and was called after set_config_size, isn't that the issue? > > >>>>>>>> for (i =3D 0; feature_sizes[i].flags !=3D 0; i++) { >>>>>>>> if (host_features & feature_sizes[i].flags) { >>>>>>>> config_size =3D MAX(feature_sizes[i].end, config= _size); >>>>>>>> --=20 >>>>>>>> 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 >>>> >> >> --=20 >> >> Jesse Larrew >> Software Engineer, KVM Team >> IBM Linux Technology Center >> Phone: (512) 973-2052 (T/L: 363-2052) >> jlarrew@linux.vnet.ibm.com