From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46018) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWsOn-0006uu-UK for qemu-devel@nongnu.org; Mon, 29 Apr 2013 14:02:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWsOm-0008Eh-1U for qemu-devel@nongnu.org; Mon, 29 Apr 2013 14:02:01 -0400 Received: from greensocs.com ([87.106.252.221]:59704 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWsOl-0008EX-Ms for qemu-devel@nongnu.org; Mon, 29 Apr 2013 14:01:59 -0400 Message-ID: <517EB590.9090700@greensocs.com> Date: Mon, 29 Apr 2013 20:01:52 +0200 From: =?ISO-8859-1?Q?KONRAD_Fr=E9d=E9ric?= MIME-Version: 1.0 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> <20130429175217.GA18172@redhat.com> In-Reply-To: <20130429175217.GA18172@redhat.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: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org, Jason Wang , aliguori@us.ibm.com, Jesse Larrew , Michael Roth On 29/04/2013 19:52, Michael S. Tsirkin wrote: > 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: >> >> On Mon, Apr 29, 2013 at 06:41:08PM +0200, KONRAD Fr=E9d=E9ric wro= te: >> >> On 29/04/2013 18:30, Michael S. Tsirkin wrote: >> >> On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsir= kin wrote: >> >> On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Fr=E9= d=E9ric wrote: >> >> On 29/04/2013 18:02, Michael S. Tsirkin wrote: >> >> On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jes= se Larrew wrote: >> >> On 04/29/2013 10:29 AM, KONRAD Fr=E9d=E9r= ic wrote: >> >> On 29/04/2013 17:14, Jesse Larrew wro= te: >> >> On 04/29/2013 09:55 AM, KONRAD Fr= =E9d=E9ric wrote: >> >> On 29/04/2013 16:42, Jesse La= rrew wrote: >> >> On 04/25/2013 01:59 AM, Micha= el S. Tsirkin wrote: >> >> On Thu, Apr 25, 2013 at 02:21= :29PM +0800, Jason Wang wrote: >> >> Commit 14f9b664 (hw/virtio-ne= t.c: set config size using host features) tries to >> calculate config size based o= n the host features. But it forgets the >> VIRTIO_NET_F_MAC were always = set for qemu later. This will lead a zero config >> len for virtio-net device whe= n both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were >> disabled form command line. T= hen qemu will crash when user tries to read the >> config of virtio-net. >> >> Fix this by counting VIRTIO_N= ET_F_MAC and make sure the config at least contains >> the mac address. >> >> Cc: Jesse Larrew >> Signed-off-by: Jason Wang >> --- >> hw/net/virtio-net.c | 3= ++- >> 1 files changed, 2 inserti= ons(+), 1 deletions(-) >> >> diff --git a/hw/net/virtio-ne= t.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_conf= ig_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 f= eature_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 vir= tio_net_set_config_size() is called. Looking >> at virtio_device_init(), we c= an 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 NUL= L); >> 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_fe= atures 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 would be to >> extend VirtioDeviceClass to i= nclude a calculate_config_size() method that >> can be called at the appropri= ate time during device initialization 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 NUL= L); >> if (k->init(vdev) < 0) = { >> return -1; >> } >> virtio_bus_plug_device(= vdev); >> + if (k->calculate_config_s= ize && k->calculate_config_size(vdev) < 0) { >> + return -1; >> + } >> return 0; >> } >> >> This would ensure that host_f= eatures 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 wit= h commit 14f9b664 no? >> >> >> I suspect this bug was present in= 14f9b664 as well. We just hadn't triggered >> it yet. I'll confirm this afterno= on. >> >> Ok, I think there is a problem with y= our patch: >> >> virtio_init(VIRTIO_DEVICE(n), "vi= rtio-net", VIRTIO_ID_NET, >> n->= config_size); >> >> is called in virtio_net_device_init (= k->init). >> >> Is there a way to resize the config a= fter that? >> >> >> Nope. That's definitely a bug. I can send= a patch to fix this today. Any >> objection to the method suggested above (= extending VirtioDeviceClass)? >> >> 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_devi= ce 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_devic= e_plugged at the end: >> >> proxy->host_features |=3D 0x1 << VIRTIO_F_NOT= IFY_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 da= ta. >> >> >> 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 >> >> Jesse Larrew >> Software Engineer, KVM Team >> IBM Linux Technology Center >> Phone: (512) 973-2052 (T/L: 363-2= 052) >> jlarrew@linux.vnet.ibm.com >> >> >> -- >> >> 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__= ); } while (0) >> #endif >> >> /* 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; >> } >> >> /* 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; >> } >> >> >> 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? > > > No, as we don't want to allow virtio-device hotplug. but look at the plugged callback in virtio-pci: pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus))= ; proxy->host_features =3D virtio_bus_get_vdev_features(bus, proxy->host_features); are impossible before the virtio-net init.