From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43612) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWsFm-00006z-5z for qemu-devel@nongnu.org; Mon, 29 Apr 2013 13:52:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWsFi-00059Y-QH for qemu-devel@nongnu.org; Mon, 29 Apr 2013 13:52:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56339) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWsFi-00059L-F5 for qemu-devel@nongnu.org; Mon, 29 Apr 2013 13:52:38 -0400 Date: Mon, 29 Apr 2013 20:52:17 +0300 From: "Michael S. Tsirkin" Message-ID: <20130429175217.GA18172@redhat.com> References: <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> <20130429170118.GA11497@redhat.com> <517EACA5.3090307@greensocs.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <517EACA5.3090307@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 07:23:49PM +0200, KONRAD Fr=E9d=E9ric wrote: > On 29/04/2013 19:01, Michael S. Tsirkin wrote: >=20 > On Mon, Apr 29, 2013 at 06:41:08PM +0200, KONRAD Fr=E9d=E9ric wrote= : >=20 > On 29/04/2013 18:30, Michael S. Tsirkin wrote: >=20 > On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirki= n wrote: >=20 > On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Fr=E9d= =E9ric wrote: >=20 > On 29/04/2013 18:02, Michael S. Tsirkin wrote: >=20 > On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse= Larrew wrote: >=20 > On 04/29/2013 10:29 AM, KONRAD Fr=E9d=E9ric= wrote: >=20 > On 29/04/2013 17:14, Jesse Larrew wrote= : >=20 > On 04/29/2013 09:55 AM, KONRAD Fr=E9= d=E9ric wrote: >=20 > On 29/04/2013 16:42, Jesse Larr= ew wrote: >=20 > On 04/25/2013 01:59 AM, Michael= S. Tsirkin wrote: >=20 > On Thu, Apr 25, 2013 at 02:21:2= 9PM +0800, Jason Wang wrote: >=20 > 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 se= t 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. The= n qemu will crash when user tries to read the > config of virtio-net. >=20 > Fix this by counting VIRTIO_NET= _F_MAC and make sure the config at least contains > the mac address. >=20 > Cc: Jesse Larrew > Signed-off-by: Jason Wang > --- > hw/net/virtio-net.c | 3 += +- > 1 files changed, 2 insertion= s(+), 1 deletions(-) >=20 > 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 vo= id 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 fea= ture_sizes[0].end; >=20 > This would be cleaner: > host_features |=3D (1 << V= IRTIO_NET_F_MAC); >=20 > no need for a comment then. >=20 >=20 > It seems to me that the real pr= oblem here is that host_features isn't > properly populated before virti= o_net_set_config_size() is called. Looking > at virtio_device_init(), we can= see why: >=20 > static int virtio_device_init(D= eviceState *qdev) > { > VirtIODevice *vdev =3D VI= RTIO_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(vd= ev); > return 0; > } >=20 > virtio_net_set_config_size() is= currently being called as part of the > k->init call, but the host_feat= ures aren't properly setup until the device > is plugged into the bus using v= irtio_bus_plug_device(). >=20 > After talking with mdroth, I th= ink the proper way to fix this would be to > extend VirtioDeviceClass to inc= lude a calculate_config_size() method that > can be called at the appropriat= e time during device initialization like so: >=20 > static int virtio_device_init(D= eviceState *qdev) > { > VirtIODevice *vdev =3D VI= RTIO_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(vd= ev); > + if (k->calculate_config_siz= e && k->calculate_config_size(vdev) < 0) { > + return -1; > + } > return 0; > } >=20 > This would ensure that host_fea= tures contains the proper data before any > devices try to make use of it t= o calculate the config size. >=20 > Good point, I didn't saw that. >=20 > but this was not the case with = commit 14f9b664 no? >=20 >=20 > I suspect this bug was present in 1= 4f9b664 as well. We just hadn't triggered > it yet. I'll confirm this afternoon. >=20 > Ok, I think there is a problem with you= r patch: >=20 > virtio_init(VIRTIO_DEVICE(n), "virt= io-net", VIRTIO_ID_NET, > n->co= nfig_size); >=20 > is called in virtio_net_device_init (k-= >init). >=20 > Is there a way to resize the config aft= er that? >=20 >=20 > Nope. That's definitely a bug. I can send a= patch to fix this today. Any > objection to the method suggested above (ex= tending VirtioDeviceClass)? >=20 > This needs more thought. All devices expected e= verything > is setup during the init call. config size is > likely not the only issue. >=20 > So we need almost all of virtio_bus_plug_device= before init, > and then after init send the signal: >=20 > 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_NOTIF= Y_ON_EMPTY; > proxy->host_features |=3D 0x1 << VIRTIO_F_BAD_F= EATURE; > proxy->host_features =3D virtio_bus_get_vdev_fe= atures(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 i= nit, > if we do, we'll trip on more uses of uninitialized data. >=20 >=20 > for (i =3D 0; feature_si= zes[i].flags !=3D 0; i++) { > if (host_features & = feature_sizes[i].flags) { > config_size =3D = MAX(feature_sizes[i].end, config_size); > -- > 1.7.1 >=20 > Jesse Larrew > Software Engineer, KVM Team > IBM Linux Technology Center > Phone: (512) 973-2052 (T/L: 363-205= 2) > jlarrew@linux.vnet.ibm.com >=20 >=20 > -- >=20 > Jesse Larrew > Software Engineer, KVM Team > IBM Linux Technology Center > Phone: (512) 973-2052 (T/L: 363-2052) > jlarrew@linux.vnet.ibm.com >=20 > Basically this is what I propose. Warning! compile-tested > only. (I also dropped an unused return value). >=20 >=20 > Signed-off-by: Michael S. Tsirkin >=20 > Which tree are you using? Is it up to date? >=20 > Heh, more changes came in. So now, it's even simpler: >=20 > Signed-off-by: Michael S. Tsirkin >=20 > --- >=20 > 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__);= } while (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 >=20 > Not sure this is what you want to do. > The device would be plugged before it is inited :/. I think this is exacly what we want to do. In fact, this is what other buses do, because devices simply can't init properly if they do not know on which bus they reside. E.g. with pci: do_pci_register_device (adds device on bus) init We can add an analog of hotplug bus callback if bus wants to get notified after device initialization. I don't see a need for this though. Do you? --=20 MST