From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34380) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZkEw-0007jE-Uy for qemu-devel@nongnu.org; Tue, 07 May 2013 11:55:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UZkEt-0002P8-OC for qemu-devel@nongnu.org; Tue, 07 May 2013 11:55:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2897) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZkEt-0002Oz-HD for qemu-devel@nongnu.org; Tue, 07 May 2013 11:55:39 -0400 Date: Tue, 7 May 2013 18:55:30 +0300 From: "Michael S. Tsirkin" Message-ID: <20130507155530.GA23915@redhat.com> References: <20130507102223.GA16878@redhat.com> <5188F3B2.2040109@greensocs.com> <20130507123313.GC21361@redhat.com> <5188F98E.9080203@greensocs.com> <20130507140056.GA21686@redhat.com> <51891DDE.2070106@greensocs.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <51891DDE.2070106@greensocs.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] virtio-net: require that config size is initialized List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: KONRAD =?iso-8859-1?Q?Fr=E9d=E9ric?= Cc: Peter Maydell , Anthony Liguori , Jason Wang , qemu-devel@nongnu.org, Stefan Hajnoczi , Paolo Bonzini On Tue, May 07, 2013 at 05:29:34PM +0200, KONRAD Fr=E9d=E9ric wrote: > On 07/05/2013 16:00, Michael S. Tsirkin wrote: > >On Tue, May 07, 2013 at 02:54:38PM +0200, KONRAD Fr=E9d=E9ric wrote: > >>On 07/05/2013 14:33, Michael S. Tsirkin wrote: > >>>On Tue, May 07, 2013 at 02:29:38PM +0200, KONRAD Fr=E9d=E9ric wrote: > >>>>Hi, > >>>> > >>>>I think it is not a good idea, as we wanted to make VirtIODevice > >>>>hot-pluggable for virtio-mmio. > >>>And then this hack will break cross version migration. > >>Why? > >> > >>virtio_net_set_config_size is called by each "proxy" devices no? > >If it's called then there's no need for a default, > >so this patch should be fine. >=20 > True but as I told you, we made this refactoring to be able to hot-plug > VirtIODevice on a virtio-bus, so calling this function won't be possibl= e. Going in circles aren't we? If it's not called it's a bug. If it's called default is not needed. > But you are right this must be fixed. >=20 > > > >>>>Maybe we can use a callback which is called by virtio-bus before > >>>>plugging, to ensure that this > >>>>config size is computed? > >>>OK, will you post such a patch? > >>> > >>The issue is as we said in the last thread, host_feature is part of > >>the proxy. > >Maybe you could add documentation on how initialization works? > >If I could fiure it out I would maybe suggest some solutions. >=20 > Yes, where do you think I can add this? Everywhere. Seriously. Start with files. File headers should give some hint as to what's in here * VirtioBus in virtio-bus.h and virtio-bus.c is not helpful. How about VirtioBus - a bus that drives from Newcastle to Edinburgh each Tuesday at noon. Drives back on Wednesdays mostly empty. It serves as a parent class for all the small taxi cars in both of these towns. or some such. Go over the interfaces and document what they do. I mean more than just repeat the name: /* Destroy the VirtIODevice */ void virtio_bus_destroy_device(VirtioBusState *bus) is not really helpful. /* Destroy a device. Assumes you don't have valuables in there. Calls 911 automatically. */ same for /* Plug the VirtIODevice */ int virtio_bus_plug_device(VirtIODevice *vdev) did you mean /* Plug the device so the content does not spill. This way you can take put it on the bus. You must use a corkscrew to unplug it later. */ > >>And if we want to move it to the devices, we must have a kind of > >>property forwarding mechanism. > >> > >>Anthony said he had something about that. > >>>>>All buses do this, and if they don't they get subtle > >>>>>bugs related to cross version migration. > >>>>>Let's add an assert to catch these bugs early. > >>>>> > >>>>>Signed-off-by: Michael S. Tsirkin > >>>>>--- > >>>>> > >>>>>Just a cleanup so not 1.5 material. > >>>>>Seems to work fine here - any opinions? > >>>>> > >>>>> hw/net/virtio-net.c | 7 ++++--- > >>>>> 1 file changed, 4 insertions(+), 3 deletions(-) > >>>>> > >>>>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >>>>>index 908e7b8..f0a9272 100644 > >>>>>--- a/hw/net/virtio-net.c > >>>>>+++ b/hw/net/virtio-net.c > >>>>>@@ -1282,6 +1282,8 @@ static int virtio_net_device_init(VirtIODevi= ce *vdev) > >>>>> DeviceState *qdev =3D DEVICE(vdev); > >>>>> VirtIONet *n =3D VIRTIO_NET(vdev); > >>>>>+ /* Verify that config size has been initialized */ > >>>>>+ assert(n->config_size !=3D (size_t)-1); > >>>>> virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET, > >>>>> n->config_size); > >>>>>@@ -1386,10 +1388,9 @@ static void virtio_net_instance_init(Object= *obj) > >>>>> VirtIONet *n =3D VIRTIO_NET(obj); > >>>>> /* > >>>>>- * The default config_size is sizeof(struct virtio_net_config= ). > >>>>>- * Can be overriden with virtio_net_set_config_size. > >>>>>+ * The config_size must be set later with virtio_net_set_conf= ig_size. > >>>>> */ > >>>>>- n->config_size =3D sizeof(struct virtio_net_config); > >>>>>+ n->config_size =3D (size_t)-1; > >>>>> } > >>>>> static Property virtio_net_properties[] =3D {