From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49528) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWuP2-0004kc-VO for qemu-devel@nongnu.org; Mon, 29 Apr 2013 16:10:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWuOz-00005e-6S for qemu-devel@nongnu.org; Mon, 29 Apr 2013 16:10:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:62148) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWuOy-00005X-Su for qemu-devel@nongnu.org; Mon, 29 Apr 2013 16:10:21 -0400 Date: Mon, 29 Apr 2013 23:09:52 +0300 From: "Michael S. Tsirkin" Message-ID: <20130429200952.GA19286@redhat.com> References: <517E9C71.1050300@greensocs.com> <20130429162106.GA10999@redhat.com> <20130429163055.GA11268@redhat.com> <517EA2A4.2030205@greensocs.com> <20130429170118.GA11497@redhat.com> <517EACA5.3090307@greensocs.com> <20130429175217.GA18172@redhat.com> <517EB590.9090700@greensocs.com> <20130429181533.GA18390@redhat.com> <517EBFDE.5090200@greensocs.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <517EBFDE.5090200@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: agraf@suse.de, aliguori@us.ibm.com, Michael Roth , Jason Wang , qemu-devel@nongnu.org, Jesse Larrew , cornelia.huck@de.ibm.com On Mon, Apr 29, 2013 at 08:45:50PM +0200, KONRAD Fr=E9d=E9ric wrote: > On 29/04/2013 20:15, Michael S. Tsirkin wrote: >=20 > On Mon, Apr 29, 2013 at 08:01:52PM +0200, KONRAD Fr=E9d=E9ric wrote= : >=20 > On 29/04/2013 19:52, Michael S. Tsirkin wrote: >=20 > On Mon, Apr 29, 2013 at 07:23:49PM +0200, KONRAD Fr=E9d=E9r= ic wrote: >=20 > 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, M= ichael S. Tsirkin wrote: >=20 > On Mon, Apr 29, 2013 at 06:14:41PM +020= 0, KONRAD Fr=E9d=E9ric wrote: >=20 > On 29/04/2013 18:02, Michael S. Tsi= rkin wrote: >=20 > On Mon, Apr 29, 2013 at 10:55:3= 6AM -0500, Jesse Larrew wrote: >=20 > On 04/29/2013 10:29 AM, KON= RAD Fr=E9d=E9ric wrote: >=20 > On 29/04/2013 17:14, Je= sse Larrew wrote: >=20 > On 04/29/2013 09:55= AM, KONRAD Fr=E9d=E9ric wrote: >=20 > On 29/04/2013 1= 6:42, Jesse Larrew wrote: >=20 > On 04/25/2013 0= 1:59 AM, Michael S. Tsirkin wrote: >=20 > On Thu, Apr 25,= 2013 at 02:21:29PM +0800, Jason Wang wrote: >=20 > Commit 14f9b664= (hw/virtio-net.c: set config size using host features) tries to > calculate confi= g size based on the host features. But it forgets the > VIRTIO_NET_F_MA= C were always set 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 c= ommand line. Then qemu will crash when user tries to read the > config of virti= o-net. >=20 > Fix this by cou= nting VIRTIO_NET_F_MAC and make sure the config at least contains > the mac address. >=20 > Cc: Jesse Larre= w > Signed-off-by: = Jason Wang > --- > hw/net/virti= o-net.c | 3 ++- > 1 files chan= ged, 2 insertions(+), 1 deletions(-) >=20 > diff --git a/hw= /net/virtio-net.c b/hw/net/virtio-net.c > index 70c8fce..= 33a70ef 100644 > --- a/hw/net/vi= rtio-net.c > +++ b/hw/net/vi= rtio-net.c > @@ -1264,7 +126= 4,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int= idx, > void virti= o_net_set_config_size(VirtIONet *n, uint32_t host_features) > { > - int i, con= fig_size =3D 0; > + /* VIRTIO_= NET_F_MAC can't be disabled from qemu side */ > + int i, con= fig_size =3D feature_sizes[0].end; >=20 > This would be c= leaner: > host_featu= res |=3D (1 << VIRTIO_NET_F_MAC); >=20 > no need for a c= omment then. >=20 >=20 > It seems to me = that the real problem here is that host_features isn't > properly popula= ted before virtio_net_set_config_size() is called. Looking > at virtio_devic= e_init(), we can see why: >=20 > static int virt= io_device_init(DeviceState *qdev) > { > VirtIODev= ice *vdev =3D VIRTIO_DEVICE(qdev); > VirtioDev= iceClass *k =3D VIRTIO_DEVICE_GET_CLASS(qdev); > assert(k-= >init !=3D NULL); > if (k->in= it(vdev) < 0) { > retur= n -1; > } > virtio_bu= s_plug_device(vdev); > return 0; > } >=20 > virtio_net_set_= config_size() is currently being called as part of the > k->init call, b= ut the host_features aren't properly setup until the device > is plugged into= the bus using virtio_bus_plug_device(). >=20 > After talking w= ith mdroth, I think the proper way to fix this would be to > extend VirtioDe= viceClass to include a calculate_config_size() method that > can be called a= t the appropriate time during device initialization like so: >=20 > static int virt= io_device_init(DeviceState *qdev) > { > VirtIODev= ice *vdev =3D VIRTIO_DEVICE(qdev); > VirtioDev= iceClass *k =3D VIRTIO_DEVICE_GET_CLASS(qdev); > assert(k-= >init !=3D NULL); > if (k->in= it(vdev) < 0) { > retur= n -1; > } > virtio_bu= s_plug_device(vdev); > + if (k->calc= ulate_config_size && k->calculate_config_size(vdev) < 0) { > + return = -1; > + } > return 0; > } >=20 > This would ensu= re that host_features contains the proper data before any > devices try to = make use of it to calculate the config size. >=20 > Good point, I d= idn't saw that. >=20 > but this was no= t the case with commit 14f9b664 no? >=20 >=20 > I suspect this bug = was present in 14f9b664 as well. We just hadn't triggered > it yet. I'll confir= m this afternoon. >=20 > Ok, I think there is a = problem with your patch: >=20 > virtio_init(VIRTIO_= DEVICE(n), "virtio-net", VIRTIO_ID_NET, > = n->config_size); >=20 > is called in virtio_net= _device_init (k->init). >=20 > Is there a way to resiz= e the config after that? >=20 >=20 > Nope. That's definitely a b= ug. I can send a patch to fix this today. Any > objection to the method sug= gested above (extending VirtioDeviceClass)? >=20 > This needs more thought. All de= vices expected everything > 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 si= gnal: >=20 > if (klass->device_plugged != =3D NULL) { > klass->device_plugged(q= bus->parent); > } >=20 > Seems the interesting part is in vi= rtio_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_co= nfig_size, isn't that the issue? >=20 > Basically devices expected everything t= o be setup at > the point where init is called. > We found one bug but let's fix it prope= rly. > There's no reason to delay parts of set= up until after init, > if we do, we'll trip on more uses of un= initialized data. >=20 >=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, config_size); > -- > 1.7.1 >=20 > Jesse Larrew > Software Engineer, = KVM Team > IBM Linux Technolog= y Center > Phone: (512) 973-20= 52 (T/L: 363-2052) > 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 valu= e). >=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 simpl= er: >=20 > Signed-off-by: Michael S. Tsirkin >=20 > --- >=20 > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/vir= tio-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(qde= v)); > @@ -51,8 +51,6 @@ int virtio_bus_plug_device(VirtIO= Device *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_in= it(DeviceState *qdev) > { > VirtIODevice *vdev =3D VIRTIO_DEVICE(qdev); > VirtioDeviceClass *k =3D VIRTIO_DEVICE_GET_CLA= SS(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 :/. >=20 > 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. >=20 > E.g. with pci: > do_pci_register_device (adds device on bus) > init >=20 > 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 >=20 >=20 >=20 > No, as we don't want to allow virtio-device hotplug. >=20 > but look at the plugged callback in virtio-pci: >=20 > pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev= _id(bus)); >=20 > proxy->host_features =3D virtio_bus_get_vdev_features(bus, > proxy->host_features); >=20 > are impossible before the virtio-net init. >=20 >=20 > At this point I have to admit I don't understand what's > going on any more. >=20 > virtio_net_instance_init sets config size to > some value, then virtio_net_set_config_size overrides it... > Help! >=20 > I am guessing it's this hack that backfires somehow. >=20 >=20 >=20 > It would be interferring if virtio_net_set_config_size was not called. >=20 > The best I think is to set the config size in the virtio_net_init funct= ion, > then remove the instance init. >=20 > The issue is that in the init function, the host_feature is not complet= ely > computed: >=20 > it lacks the three line in virtio pci plugged function. >=20 > Maybe we can move the two firsts line in the virtio_net_pci_init before= the > init if they are usefull in the > config_size computing: >=20 >=20 > proxy->host_features |=3D 0x1 << VIRTIO_F_NOTIFY_ON_EMP= TY; > proxy->host_features |=3D 0x1 << VIRTIO_F_BAD_FEATURE; Bus can set VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_BAD_FEATURE later, this is not going to affect anything. Reason is, VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_BAD_FEATURE are compatibility hacks which is why we dont have them for s390. It's probably a bug that we have them for virtio-ccw. >=20 > Then compute the last one directly in the init function which is the ha= rder: >=20 > virtio_net_get_features The real fix is to set features in init. Can we move host_features to struct VirtIODevice, and init to the device init function? The reason we didn't do this initially is exactly because we need to specify them in -device flag, and there was no way to do this for VirtIODevice, since it's the proxy that is instanciated. Does the new bus infrastructure allow this? > Note that there is in virtio_net_get_features: >=20 > features |=3D (1 << VIRTIO_NET_F_MAC); >=20 > Which is exacty the issue the initial patch is solving. >=20 > Fred Yes. the lifetime of host features is as follows: - configured in device by user, mostly set to "on" by default - depending on device specific logic, override some features - mostly to "off", but we also force some required features to "on" - Two exceptions (notify on empty+bad) are transport specific. they are also not user-controllable. --=20 MST