From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=45158 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OnXIa-0002Bb-5z for qemu-devel@nongnu.org; Mon, 23 Aug 2010 09:42:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OnXIY-0001wf-U6 for qemu-devel@nongnu.org; Mon, 23 Aug 2010 09:42:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43101) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OnXIY-0001wY-MG for qemu-devel@nongnu.org; Mon, 23 Aug 2010 09:42:50 -0400 Message-ID: <4C727ACC.7080501@redhat.com> Date: Mon, 23 Aug 2010 16:42:36 +0300 From: Avi Kivity MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup References: <4C6D86F9.3010602@codemonkey.ws> <4C6D98E7.9020109@codemonkey.ws> <4C6DA75D.40303@codemonkey.ws> <4C6ECBB7.7060608@codemonkey.ws> <4C718865.7010807@redhat.com> <4C719080.4030202@codemonkey.ws> <4C720B1F.3030206@redhat.com> <4C727646.3040903@codemonkey.ws> In-Reply-To: <4C727646.3040903@codemonkey.ws> 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: Anthony Liguori Cc: Blue Swirl , "Liu >> \"Liu, Jinsong\"" , qemu-devel , Markus Armbruster , Paul Brook On 08/23/2010 04:23 PM, Anthony Liguori wrote: >>> This is really a fundamental discussion. If you look closely at >>> qdev in it's current form, what it actually models is a device with >>> GPIO input and output whereas the GPIO input and output correspond >>> to qemu_irqs which really model pins that can be raised and lowered. >>> >>> To me, this is insane and I'm looking to move the GPIO stuff out of >>> qdev. There are some devices where it makes sense to model the >>> interactions between pins but not for the vast majority of devices. >> >> I agree, but I don't see the burning need or why it's "insane". >> Seems like a minor design issue, can't you just ignore GPIO when you >> don't need it? > > > In a sane object model, the expectation is that you can meaningfully > interact with base classes using the interfaces provided by the base > class. > > If DeviceState has a GPIO interface, you should be able to use that > interface without knowledge of the subclasses. This implies that all > subclasses implement a GPIO interface and that it can be the primary > interface to interact and connect with devices. Modelling a PCI > device based on a GPIO interface is what I was referring to as insane. The pci device has num_gpio_out == num_gpio_in == 0, so it all works (trivially). Again I agree except for the sense of impending doom. It's silly but not insane - it assumes most devices have >0 gpio pins, which isn't the case for the devices we care about (so it's only subjectively silly). >> GPIO is just one way for a device to talk, same as >> (*bus)_phys_memory_rw() or its netdev or its chardev or its timers. >> It doesn't need to have special status within DeviceState, but it >> doesn't hurt so much that I can tell. > > Everything extra hurts when you're trying to move code in to a library > with unit tests covering the functionality :-) Sure, it's a worthy cleanup. But it's not a reason to go to DEFCON 1. > >>> typedef struct Timer Timer; >>> >>> void timer_init(DeviceState *, void (*fn)(Timer *)); >>> void timer_update_rel_ns(Timer *); >>> void timer_cancel(Timer *); >>> void timer_release(Timer *); >>> >>> Timer objects get embedded into the device's state and container_of >>> can be used to get to the original device state. We could also pass >>> DeviceState. It's not clear to me which is better. >> >> Not embedding the DeviceState is more generic. For example, a device >> with a variable number of timers wouldn't be able to embed them in >> DeviceState. > > Where would they put them? Everything a device does has to be stored > in a DeviceState. It may put them in a container of some form if the > timers are dynamic. Right. In any case, I don't see how passing a DeviceState helps. >>> But being able to associate timers with devices seems like a very >>> good idea to me because it means that you can see which devices are >>> registering timers. >> >> You might also have the timers auto-cancelled and auto-destroyed on >> device removal. But the whole thing seems like a minor coding issue >> rather than something fundamental. > > The fundamental issue is: every function (minus trivial ones) in the > device models code should have a state reference. That state > reference should inherit from a DeviceState. If this statement isn't > true, then the device has been modelled in qdev incorrectly. > > Using this test, quite a lot of the "converted" devices are being > modelled incorrectly. Is a "state reference" allowed to have a pointer to the state, or reach it in some other way (for example, static storage for singleton devices)? Isn't "save/restore works" an equivalent statement to "device state is reachable from the DeviceState"? -- error compiling committee.c: too many arguments to function