From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49889) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWsck-0001UR-Uc for qemu-devel@nongnu.org; Mon, 29 Apr 2013 14:16:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWsch-0004rK-6J for qemu-devel@nongnu.org; Mon, 29 Apr 2013 14:16:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59508) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWscg-0004r2-Qi for qemu-devel@nongnu.org; Mon, 29 Apr 2013 14:16:23 -0400 Date: Mon, 29 Apr 2013 21:15:33 +0300 From: "Michael S. Tsirkin" Message-ID: <20130429181533.GA18390@redhat.com> References: <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> <517EB590.9090700@greensocs.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <517EB590.9090700@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 08:01:52PM +0200, KONRAD Fr=E9d=E9ric wrote: > 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 wr= ote: > >> > >> On 29/04/2013 18:30, Michael S. Tsirkin wrote: > >> > >> On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsi= rkin 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, Je= sse Larrew wrote: > >> > >> On 04/29/2013 10:29 AM, KONRAD Fr=E9d=E9= ric wrote: > >> > >> On 29/04/2013 17:14, Jesse Larrew wr= ote: > >> > >> On 04/29/2013 09:55 AM, KONRAD F= r=E9d=E9ric wrote: > >> > >> On 29/04/2013 16:42, Jesse L= arrew wrote: > >> > >> On 04/25/2013 01:59 AM, Mich= ael S. Tsirkin wrote: > >> > >> On Thu, Apr 25, 2013 at 02:2= 1:29PM +0800, Jason Wang wrote: > >> > >> Commit 14f9b664 (hw/virtio-n= et.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= set for qemu later. This will lead a zero config > >> len for virtio-net device wh= en both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were > >> disabled form command line. = Then qemu will crash when user tries to read the > >> config of virtio-net. > >> > >> Fix this by counting VIRTIO_= NET_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 insert= ions(+), 1 deletions(-) > >> > >> diff --git a/hw/net/virtio-n= et.c b/hw/net/virtio-net.c > >> index 70c8fce..33a70ef 10064= 4 > >> --- 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_con= fig_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 vi= rtio_net_set_config_size() is called. Looking > >> at virtio_device_init(), we = can see why: > >> > >> static int virtio_device_ini= t(DeviceState *qdev) > >> { > >> VirtIODevice *vdev =3D= VIRTIO_DEVICE(qdev); > >> VirtioDeviceClass *k =3D= VIRTIO_DEVICE_GET_CLASS(qdev); > >> assert(k->init !=3D NU= LL); > >> 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_f= eatures aren't properly setup until the device > >> is plugged into the bus usin= g 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 appropr= iate time during device initialization like so: > >> > >> static int virtio_device_ini= t(DeviceState *qdev) > >> { > >> VirtIODevice *vdev =3D= VIRTIO_DEVICE(qdev); > >> VirtioDeviceClass *k =3D= VIRTIO_DEVICE_GET_CLASS(qdev); > >> assert(k->init !=3D NU= LL); > >> if (k->init(vdev) < 0)= { > >> return -1; > >> } > >> virtio_bus_plug_device= (vdev); > >> + if (k->calculate_config_= size && k->calculate_config_size(vdev) < 0) { > >> + return -1; > >> + } > >> return 0; > >> } > >> > >> This would ensure that host_= features contains the proper data before any > >> devices try to make use of i= t to calculate the config size. > >> > >> Good point, I didn't saw tha= t. > >> > >> but this was not the case wi= th commit 14f9b664 no? > >> > >> > >> I suspect this bug was present i= n 14f9b664 as well. We just hadn't triggered > >> it yet. I'll confirm this aftern= oon. > >> > >> Ok, I think there is a problem with = your patch: > >> > >> virtio_init(VIRTIO_DEVICE(n), "v= irtio-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 sen= d a patch to fix this today. Any > >> objection to the method suggested above = (extending VirtioDeviceClass)? > >> > >> This needs more thought. All devices expecte= d everything > >> is setup during the init call. config size i= s > >> likely not the only issue. > >> > >> So we need almost all of virtio_bus_plug_dev= ice 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_devi= ce_plugged at the end: > >> > >> proxy->host_features |=3D 0x1 << VIRTIO_F_NO= TIFY_ON_EMPTY; > >> proxy->host_features |=3D 0x1 << VIRTIO_F_BA= D_FEATURE; > >> proxy->host_features =3D virtio_bus_get_vdev= _features(bus, > >> proxy->host_features); > >> > >> This is and was called after set_config_size, is= n'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 afte= r init, > >> if we do, we'll trip on more uses of uninitialized d= ata. > >> > >> > >> 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-= 2052) > >> 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-teste= d > >> 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(DeviceStat= e *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. >=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. At this point I have to admit I don't understand what's going on any more. virtio_net_instance_init sets config size to some value, then virtio_net_set_config_size overrides it... Help! I am guessing it's this hack that backfires somehow. --=20 MST