From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NsJTI-0005EN-Vi for qemu-devel@nongnu.org; Thu, 18 Mar 2010 13:25:25 -0400 Received: from [199.232.76.173] (port=40616 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NsJTI-0005Dc-Bn for qemu-devel@nongnu.org; Thu, 18 Mar 2010 13:25:24 -0400 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NsJTG-0008AA-NH for qemu-devel@nongnu.org; Thu, 18 Mar 2010 13:25:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47464) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NsJTG-0008A2-B9 for qemu-devel@nongnu.org; Thu, 18 Mar 2010 13:25:22 -0400 Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o2IHPL47007268 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 18 Mar 2010 13:25:21 -0400 Date: Thu, 18 Mar 2010 19:21:56 +0200 From: "Michael S. Tsirkin" Message-ID: <20100318172156.GB27805@redhat.com> References: <20100318064015.GA16973@redhat.com> <4BA24A26.5030602@redhat.com> <4BA2569A.3040600@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: Gerd Hoffmann , qemu-devel@nongnu.org On Thu, Mar 18, 2010 at 06:08:06PM +0100, Juan Quintela wrote: > Gerd Hoffmann wrote: > > Hi, > > > >> But I still think that this is independent that VirtIO being 1st (or > >> not) memer of VirtIOBlock. > > > > There is no strong reason for this other than memory allocation. As > > long as virtio_common_init() does the allocation there is no way > > around VirtIODevice being the first member. If this changes (and we > > must change it if we want embed VirtIODevice and superclasses into > > other structs) no reason is left. > > It is, a 1st look at virtio-pci.c shows > > static void virtio_pci_reset(DeviceState *d) > { > VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev); > virtio_reset(proxy->vdev); > msix_reset(&proxy->pci_dev); > } > > We have to call virtio_reset() that needs a virtio object, and the only > thing that we have is a VirtIOPCIProxy. > We would have to implement > something like: > > struct VirtioPCIProxy_fields { > .... current VirtioPCIProxy fields > } > > struct VirtioPCIProxy { > struct VirtioPCIProxy_fields fields; > struct VirtIODevice vdev; > } > > struct VirtioBlockPCI { > struct VirtiPCIProxy_fields field; > struct VirtIOBlock vdev; > } > > > > Why? All code in virtio.c wants/need a VirtIODevice. > All code in virtio-pci.c wants a VirtIODevice also, it is not interested > in the specific bits. > > I can't see how you want to mov VirtIODevice from the 1st place of > VirtIOBlock (and similars) without having to rewrite all of virtio-pci.c > for each device. The whole '1st place' hack is really not necessary, if we know the offset we can use container_of. > > Just changing it for the snake of change isn't a good reason > > either. But if it helps cleaning the code we can change it without > > running into trouble. You can't cast VirtIODevice to VirtIOBlock any > > more, but you don't really want to anyway for type checking reasons. > > As I have show, from the time that virtio-pci only uses the virtio.c > functions (and needs VirtIODevice fields), I don't see how to embed it > and share the current functions in virtio-pci.c. We can just keep the pointer from VirtiPCIProxy to virtio device. > Notice that what we really want is create the devices in > > .qdev.name = "virtio-balloon-pci", > .qdev.size = sizeof(VirtIOPCIProxy), > > as > > .qdev.name = "virtio-balloon-pci", > .qdev.size = sizeof(VirtIOPCIProxy) + sizeof(VirtIOBlock) - sizeof(VirtIODevice), > > And having a single definition of: > > struct VirtioPCIProxy { > > struct VirtIODevice vdev; > } > > Yes, it is ugly/subtle as hell, but no other simple way. You allocate > the struct on each virtio-*.c file, where you know the size, and no > embed. Or you embed the struct, and replicate lots of code to have it > type safe. We don't need to drop the vdev pointer if it's helpful. If we have it around there will be close to no code changes. > Any of the two options are worse than current way IMHO. Having to > export VirtIOBlock only to know its size looks wrong in addition. > > Depending on size of the last element is ... too hackish? Yes. > Later, Juan.