From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47283) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UX6Dx-0002D6-BW for qemu-devel@nongnu.org; Tue, 30 Apr 2013 04:47:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UX6Dt-0004w2-Bs for qemu-devel@nongnu.org; Tue, 30 Apr 2013 04:47:45 -0400 Received: from greensocs.com ([87.106.252.221]:59906 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UX6Ds-0004ub-T0 for qemu-devel@nongnu.org; Tue, 30 Apr 2013 04:47:41 -0400 Message-ID: <517F8526.9070509@greensocs.com> Date: Tue, 30 Apr 2013 10:47:34 +0200 From: =?ISO-8859-1?Q?KONRAD_Fr=E9d=E9ric?= MIME-Version: 1.0 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> <20130429200952.GA19286@redhat.com> In-Reply-To: <20130429200952.GA19286@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: agraf@suse.de, aliguori@us.ibm.com, Michael Roth , Jason Wang , qemu-devel@nongnu.org, Jesse Larrew , cornelia.huck@de.ibm.com On 29/04/2013 22:09, Michael S. Tsirkin wrote: > 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: >> >> On Mon, Apr 29, 2013 at 08:01:52PM +0200, KONRAD Fr=E9d=E9ric wro= te: >> >> On 29/04/2013 19:52, Michael S. Tsirkin wrote: >> >> On Mon, Apr 29, 2013 at 07:23:49PM +0200, KONRAD Fr=E9d=E9= ric 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 wrote: >> >> On 29/04/2013 18:30, Michael S. Tsirkin wrote= : >> >> On Mon, Apr 29, 2013 at 07:21:06PM +0300,= Michael S. Tsirkin wrote: >> >> On Mon, Apr 29, 2013 at 06:14:41PM +0= 200, KONRAD Fr=E9d=E9ric wrote: >> >> On 29/04/2013 18:02, Michael S. T= sirkin wrote: >> >> On Mon, Apr 29, 2013 at 10:55= :36AM -0500, Jesse Larrew wrote: >> >> On 04/29/2013 10:29 AM, K= ONRAD 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 2= 5, 2013 at 02:21:29PM +0800, Jason Wang wrote: >> >> Commit 14f9b6= 64 (hw/virtio-net.c: set config size using host features) tries to >> calculate con= fig 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 virti= o-net device when 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 vir= tio-net. >> >> Fix this by c= ounting VIRTIO_NET_F_MAC and make sure the config at least contains >> the mac addre= ss. >> >> Cc: Jesse Lar= rew >> Signed-off-by= : Jason Wang >> --- >> hw/net/vir= tio-net.c | 3 ++- >> 1 files ch= anged, 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 +1= 264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, i= nt idx, >> void vir= tio_net_set_config_size(VirtIONet *n, uint32_t host_features) >> { >> - int i, c= onfig_size =3D 0; >> + /* VIRTI= O_NET_F_MAC can't be disabled from qemu side */ >> + int i, c= onfig_size =3D feature_sizes[0].end; >> >> This would be= cleaner: >> host_fea= tures |=3D (1 << VIRTIO_NET_F_MAC); >> >> no need for a= comment then. >> >> >> It seems to m= e that the real problem here is that host_features isn't >> properly popu= lated before virtio_net_set_config_size() is called. Looking >> at virtio_dev= ice_init(), we can see why: >> >> static int vi= rtio_device_init(DeviceState *qdev) >> { >> VirtIOD= evice *vdev =3D VIRTIO_DEVICE(qdev); >> VirtioD= eviceClass *k =3D VIRTIO_DEVICE_GET_CLASS(qdev); >> assert(= k->init !=3D NULL); >> if (k->= init(vdev) < 0) { >> ret= urn -1; >> } >> virtio_= bus_plug_device(vdev); >> return = 0; >> } >> >> virtio_net_se= t_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 in= to the bus using virtio_bus_plug_device(). >> >> After talking= with mdroth, I think the proper way to fix this would be to >> extend Virtio= DeviceClass to include a calculate_config_size() method that >> can be called= at the appropriate time during device initialization like so: >> >> static int vi= rtio_device_init(DeviceState *qdev) >> { >> VirtIOD= evice *vdev =3D VIRTIO_DEVICE(qdev); >> VirtioD= eviceClass *k =3D VIRTIO_DEVICE_GET_CLASS(qdev); >> assert(= k->init !=3D NULL); >> if (k->= init(vdev) < 0) { >> ret= urn -1; >> } >> virtio_= bus_plug_device(vdev); >> + if (k->ca= lculate_config_size && k->calculate_config_size(vdev) < 0) { >> + retur= n -1; >> + } >> return = 0; >> } >> >> This would en= sure that host_features contains the proper data before any >> devices try t= o 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 bu= g was present in 14f9b664 as well. We just hadn't triggered >> it yet. I'll conf= irm this afternoon. >> >> Ok, I think there is = a problem with your patch: >> >> virtio_init(VIRTI= O_DEVICE(n), "virtio-net", VIRTIO_ID_NET, >> = n->config_size); >> >> is called in virtio_n= et_device_init (k->init). >> >> Is there a way to res= ize the config after that? >> >> >> Nope. That's definitely a= bug. I can send a patch to fix this today. Any >> objection to the method s= uggested 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 virt= io_bus_plug_device 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_device_plugged at the end: >> >> proxy->host_features |=3D 0x1= << VIRTIO_F_NOTIFY_ON_EMPTY; >> proxy->host_features |=3D 0x1= << VIRTIO_F_BAD_FEATURE; >> proxy->host_features =3D virt= io_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 pro= perly. >> There's no reason to delay parts of s= etup until after init, >> if we do, we'll trip on more uses of = uninitialized data. >> >> >> 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 Technol= ogy Center >> Phone: (512) 973-= 2052 (T/L: 363-2052) >> jlarrew@linux.vne= t.ibm.com >> >> >> -- >> >> Jesse Larrew >> Software Engineer, KVM Te= am >> IBM Linux Technology Cent= er >> Phone: (512) 973-2052 (T/= L: 363-2052) >> jlarrew@linux.vnet.ibm.co= m >> >> Basically this is what I propose. Warning= ! compile-tested >> only. (I also dropped an unused return va= lue). >> >> >> 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 sim= pler: >> >> Signed-off-by: Michael S. Tsirkin >> >> --- >> >> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/v= irtio-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(q= dev)); >> @@ -51,8 +51,6 @@ int virtio_bus_plug_device(Virt= IODevice *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/virti= o.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_C= LASS(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_vd= ev_id(bus)); >> >> proxy->host_features =3D virtio_bus_get_vdev_features(bus= , >> proxy->host_features); >> >> 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. >> >> >> >> It would be interferring if virtio_net_set_config_size was not called. >> >> The best I think is to set the config size in the virtio_net_init func= tion, >> then remove the instance init. >> >> The issue is that in the init function, the host_feature is not comple= tely >> computed: >> >> it lacks the three line in virtio pci plugged function. >> >> Maybe we can move the two firsts line in the virtio_net_pci_init befor= e the >> init if they are usefull in the >> config_size computing: >> >> >> proxy->host_features |=3D 0x1 << VIRTIO_F_NOTIFY_ON_E= MPTY; >> 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. > >> Then compute the last one directly in the init function which is the h= arder: >> >> 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? Yes, I think it's possible for PCI and S390, but it seems more difficult=20 for CCW. I don't really understand how it's working with CCW devices, there is an array of host_features? Cornelia any idea? > >> Note that there is in virtio_net_get_features: >> >> features |=3D (1 << VIRTIO_NET_F_MAC); >> >> Which is exacty the issue the initial patch is solving. >> >> 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. > >