From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57405) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWrSJ-0001tP-TQ for qemu-devel@nongnu.org; Mon, 29 Apr 2013 13:01:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWrSG-0004Be-N0 for qemu-devel@nongnu.org; Mon, 29 Apr 2013 13:01:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60533) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWrSG-0004BX-F4 for qemu-devel@nongnu.org; Mon, 29 Apr 2013 13:01:32 -0400 Date: Mon, 29 Apr 2013 20:01:18 +0300 From: "Michael S. Tsirkin" Message-ID: <20130429170118.GA11497@redhat.com> References: <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> <517E9C71.1050300@greensocs.com> <20130429162106.GA10999@redhat.com> <20130429163055.GA11268@redhat.com> <517EA2A4.2030205@greensocs.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <517EA2A4.2030205@greensocs.com> 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: KONRAD =?iso-8859-1?Q?Fr=E9d=E9ric?= Cc: qemu-devel@nongnu.org, Jason Wang , aliguori@us.ibm.com, Jesse Larrew , Michael Roth On Mon, Apr 29, 2013 at 06:41:08PM +0200, KONRAD Fr=E9d=E9ric wrote: > On 29/04/2013 18:30, Michael S. Tsirkin wrote: > >On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin wrote: > >>On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Fr=E9d=E9ric wrote: > >>>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= features) tries to > >>>>>>>>>>>calculate config size based on the host features. But it for= gets the > >>>>>>>>>>>VIRTIO_NET_F_MAC were always set for qemu later. This will l= ead 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 t= ries to read the > >>>>>>>>>>>config of virtio-net. > >>>>>>>>>>> > >>>>>>>>>>>Fix this by counting VIRTIO_NET_F_MAC and make sure the conf= ig 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 =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_feature= s isn't > >>>>>>>>>properly populated before virtio_net_set_config_size() is call= ed. 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 unti= l 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 initializa= tion 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(v= dev) < 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. > >>>>>>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 toda= y. Any > >>>>>objection to the method suggested above (extending VirtioDeviceCla= ss)? > >>>>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 en= d: > >>> > >>> 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? > >>Basically devices expected everything to be setup at > >>the point where init is called. > >>We found one bug but let's fix it properly. > >>There's no reason to delay parts of setup until after init, > >>if we do, we'll trip on more uses of uninitialized data. > >> > >>>> > >>>>>>>>>>> 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, co= nfig_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 > > > >Basically this is what I propose. Warning! compile-tested > >only. (I also dropped an unused return value). > > > > > >Signed-off-by: Michael S. Tsirkin > Which tree are you using? Is it up to date? Heh, more changes came in. So now, it's even simpler: Signed-off-by: Michael S. Tsirkin --- diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index aab72ff..447ba16 100644 --- a/hw/virtio/virtio-bus.c +++ b/hw/virtio/virtio-bus.c @@ -38,7 +38,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } whi= le (0) #endif =20 /* Plug the VirtIODevice */ -int virtio_bus_plug_device(VirtIODevice *vdev) +void virtio_bus_plug_device(VirtIODevice *vdev) { DeviceState *qdev =3D DEVICE(vdev); BusState *qbus =3D BUS(qdev_get_parent_bus(qdev)); @@ -51,8 +51,6 @@ int virtio_bus_plug_device(VirtIODevice *vdev) if (klass->device_plugged !=3D NULL) { klass->device_plugged(qbus->parent); } - - return 0; } =20 /* Reset the virtio_bus */ diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 0f88c25..c5228e6 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1091,11 +1091,11 @@ static int virtio_device_init(DeviceState *qdev) { VirtIODevice *vdev =3D VIRTIO_DEVICE(qdev); VirtioDeviceClass *k =3D VIRTIO_DEVICE_GET_CLASS(qdev); + virtio_bus_plug_device(vdev); assert(k->init !=3D NULL); if (k->init(vdev) < 0) { return -1; } - virtio_bus_plug_device(vdev); return 0; } =20 diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bu= s.h index 9ed60f9..da84141 100644 --- a/include/hw/virtio/virtio-bus.h +++ b/include/hw/virtio/virtio-bus.h @@ -72,7 +72,7 @@ struct VirtioBusState { VirtIODevice *vdev; }; =20 -int virtio_bus_plug_device(VirtIODevice *vdev); +void virtio_bus_plug_device(VirtIODevice *vdev); void virtio_bus_reset(VirtioBusState *bus); void virtio_bus_destroy_device(VirtioBusState *bus); /* Get the device id of the plugged device. */