From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48787) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWqzJ-0007Mx-7w for qemu-devel@nongnu.org; Mon, 29 Apr 2013 12:31:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWqzF-0002VO-Ar for qemu-devel@nongnu.org; Mon, 29 Apr 2013 12:31:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:25142) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWqzF-0002V1-37 for qemu-devel@nongnu.org; Mon, 29 Apr 2013 12:31:33 -0400 Date: Mon, 29 Apr 2013 19:30:55 +0300 From: "Michael S. Tsirkin" Message-ID: <20130429163055.GA11268@redhat.com> 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> <517E9C71.1050300@greensocs.com> <20130429162106.GA10999@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20130429162106.GA10999@redhat.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 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 forg= ets the > > >>>>>>>>VIRTIO_NET_F_MAC were always set for qemu later. This will le= ad a zero config > > >>>>>>>>len for virtio-net device when both VIRTIO_NET_F_STATUS and V= IRTIO_NET_F_MQ were > > >>>>>>>>disabled form command line. Then qemu will crash when user tr= ies to read the > > >>>>>>>>config of virtio-net. > > >>>>>>>> > > >>>>>>>>Fix this by counting VIRTIO_NET_F_MAC and make sure the confi= g 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_m= ask(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_features= isn't > > >>>>>>properly populated before virtio_net_set_config_size() is calle= d. 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= the device > > >>>>>>is plugged into the bus using virtio_bus_plug_device(). > > >>>>>> > > >>>>>>After talking with mdroth, I think the proper way to fix this w= ould be to > > >>>>>>extend VirtioDeviceClass to include a calculate_config_size() m= ethod that > > >>>>>>can be called at the appropriate time during device initializat= ion 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(vd= ev) < 0) { > > >>>>>>+ return -1; > > >>>>>>+ } > > >>>>>> return 0; > > >>>>>>} > > >>>>>> > > >>>>>>This would ensure that host_features contains the proper data b= efore 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 today= . Any > > >>objection to the method suggested above (extending VirtioDeviceClas= s)? > > >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); > > > } > >=20 > > Seems the interesting part is in virtio_pci_device_plugged at the end= : > >=20 > > 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); > >=20 > > This is and was called after set_config_size, isn't that the issue? >=20 > 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. >=20 > > > > > > > > >>>>>>>> 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, con= fig_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 --- diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index 1596a1c..c75c8ae 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)); @@ -64,12 +64,18 @@ int virtio_bus_plug_device(VirtIODevice *vdev) bus->bindings.set_host_notifier =3D klass->set_host_notifier; bus->bindings.vmstate_change =3D klass->vmstate_change; virtio_bind_device(bus->vdev, &bus->bindings, qbus->parent); +} + +void virtio_bus_plugged(VirtIODevice *vdev) +{ + DeviceState *qdev =3D DEVICE(vdev); + BusState *qbus =3D BUS(qdev_get_parent_bus(qdev)); + VirtioBusState *bus =3D VIRTIO_BUS(qbus); + VirtioBusClass *klass =3D VIRTIO_BUS_GET_CLASS(bus); =20 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 1c2282c..65f69cf 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1088,11 +1088,16 @@ 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); + + virtio_bus_plugged(vdev); + return 0; } =20 diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bu= s.h index 311e8c7..471e55e 100644 --- a/include/hw/virtio/virtio-bus.h +++ b/include/hw/virtio/virtio-bus.h @@ -76,7 +76,8 @@ struct VirtioBusState { VirtIOBindings bindings; }; =20 -int virtio_bus_plug_device(VirtIODevice *vdev); +void virtio_bus_plug_device(VirtIODevice *vdev); +void virtio_bus_plugged(VirtIODevice *vdev); void virtio_bus_reset(VirtioBusState *bus); void virtio_bus_destroy_device(VirtioBusState *bus); /* Get the device id of the plugged device. */