From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NtXk6-0004ba-3N for qemu-devel@nongnu.org; Sun, 21 Mar 2010 22:51:50 -0400 Received: from [199.232.76.173] (port=60308 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NtXk4-0004bS-QD for qemu-devel@nongnu.org; Sun, 21 Mar 2010 22:51:48 -0400 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NtXk2-0000lH-SK for qemu-devel@nongnu.org; Sun, 21 Mar 2010 22:51:48 -0400 Received: from mail-gy0-f173.google.com ([209.85.160.173]:61303) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NtXk2-0000lD-KN for qemu-devel@nongnu.org; Sun, 21 Mar 2010 22:51:46 -0400 Received: by gyd5 with SMTP id 5so2535390gyd.4 for ; Sun, 21 Mar 2010 19:51:45 -0700 (PDT) Message-ID: <4BA6DB3F.7010702@codemonkey.ws> Date: Sun, 21 Mar 2010 21:51:43 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups References: <20100318064015.GA16973@redhat.com> <20100318074239.GA23474@redhat.com> <20100318090711.GB23649@redhat.com> <20100319014159.GB14108@shareable.org> <20100321143103.GD12758@redhat.com> <20100321181143.GE4174@shareable.org> <20100321191631.GA13355@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" On 03/21/2010 08:06 PM, Juan Quintela wrote: > "Michael S. Tsirkin" wrote: > >> On Sun, Mar 21, 2010 at 06:11:43PM +0000, Jamie Lokier wrote: >> >>> Michael S. Tsirkin wrote: >>> >>>> That's version 1 of my patch. Version 2 removed even need for macro >>>> completely by moving allocations to the caller. >>>> >>> The downside of moving allocations are: (1) it's one more call in the >>> caller, to allocate the type, (2) it needs a virtual destructor for >>> each type to free the object, which can clutter the code if there is >>> no other reason for virtual destructors. >>> >>> I don't think those are necessarily bad, but they can remove from the >>> neatness of existing code. Personally I favour an occasional macro >>> using sizeof/offsetof/container_of if the result is a natural and >>> sensible API to all of its callers. >>> >>> -- Jamie >>> >> We need to free in caller anyway because structure is used >> after cleanup. This can be changed by restructuring code ... >> I don't have a strong preference, anything is better >> than the hack relying on field being at offset 0 in structure. >> > It is not a hack. > > Is the only sane way of sharing virtio common field without having to > export all virtio particular structs :( > > I really hate how this is going :( > > a- we have code that assumes that virtio is the 1st element of all > virtio structs. > > b- we have a patch that codifies that virtio is used in that way (using > DO_UPCAST()) (me) > > c- we arrived to the point where being it at the beggining of the struct > is the bigger cast in the universe (I present everybody thinking that > to look at rest of qemu). Patch is done that makes it possible to > alloc memory outside of virtio_common_init(). No code on virtio.c or > virtio-pci.c is changed to use this new offset. (michael). > > d- Anthony arrives to the discussion stating that it should exist a > VirtIOPCIBus, that way virtio devices should be hanging of that bus > (that requires lots of changes). > > e- kraxel arrives to the discussion stating that initializing in the > middle of one struct is the right thing to do, except if you are > qdev, that then it is not. I point to his suggestion that it makes > things still uglier (no answer yet, but it was weekend for him). > > f- I show using kraxel example how ugly things go (sizeof (struct A) + > sizeof (struct B) - sizeof (struct C)). And here is when things fall > apart IMHO. Michael states that it is ok to have to had: > - an offset field > - a pointer in one struct to inside the same struct (vdev) > but assuming and stating and using an offset 0 is ugly. > > Have I lost anything? Is there a realistic way of getting struct > VirtIODevice (and VirtIOBlock, ...) inside VirtIOPCIProxy that don't > imply using an offset + a pointer + exporting the struct definitions > only to calculate its size? > The object model is wrong. A VirtIOBlock device cannot be a VirtIODevice while being a VirtIOPCIProxy (proxy is a poor name, btw). It really ought to be: DeviceState -> VirtIODevice -> VirtIOBlock and: PCIDevice -> VirtIOPCI : implements VirtIOBus The interface between the VirtIODevice and VirtIOBus is the virtio transport. The main reason a separate bus is needed is the same reason it's needed in Linux. VirtIOBlock has to be tied to some bus. It cannot be tied to the PCI bus without having it be part of the implementation detail. Introducing another bus type fixes this (and it's what we do in the kernel). With respect to save and restore, there really ought to be two separate sections. One section containing just virtio information and another section that stores the PCI state. Admittedly, that's going to be a tough change to make but it's the proper approach. Regards, Anthony Liguori