From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58422) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWq17-000879-9f for qemu-devel@nongnu.org; Mon, 29 Apr 2013 11:29:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWq15-0006QY-F1 for qemu-devel@nongnu.org; Mon, 29 Apr 2013 11:29:25 -0400 Received: from greensocs.com ([87.106.252.221]:38597 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWq15-0006QI-5Z for qemu-devel@nongnu.org; Mon, 29 Apr 2013 11:29:23 -0400 Message-ID: <517E91CD.9080602@greensocs.com> Date: Mon, 29 Apr 2013 17:29:17 +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> In-Reply-To: <517E8E5A.5050202@linux.vnet.ibm.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: Jesse Larrew Cc: qemu-devel@nongnu.org, Jason Wang , aliguori@us.ibm.com, Michael Roth , "Michael S. Tsirkin" 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 featur= es) tries to >>>>> calculate config size based on the host features. But it forgets th= e >>>>> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a z= ero 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 l= east 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(Vi= rtIODevice *vdev, int idx, >>>>> void virtio_net_set_config_size(VirtIONet *n, uint32_t host_fe= atures) >>>>> { >>>>> - 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 isn't >>> properly populated before virtio_net_set_config_size() is called. Loo= king >>> 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 the d= evice >>> is plugged into the bus using virtio_bus_plug_device(). >>> >>> After talking with mdroth, I think the proper way to fix this would b= e to >>> extend VirtioDeviceClass to include a calculate_config_size() method = that >>> can be called at the appropriate time during device initialization li= ke 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 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 trig= gered > 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? > >>>>> 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_siz= e); >>>>> --=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 >