From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=45070 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OnWzp-0004wa-9S for qemu-devel@nongnu.org; Mon, 23 Aug 2010 09:23:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OnWzj-0007FM-FE for qemu-devel@nongnu.org; Mon, 23 Aug 2010 09:23:29 -0400 Received: from mail-gw0-f45.google.com ([74.125.83.45]:64373) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OnWzj-0007FD-Cw for qemu-devel@nongnu.org; Mon, 23 Aug 2010 09:23:23 -0400 Received: by gwb11 with SMTP id 11so2270957gwb.4 for ; Mon, 23 Aug 2010 06:23:22 -0700 (PDT) Message-ID: <4C727646.3040903@codemonkey.ws> Date: Mon, 23 Aug 2010 08:23:18 -0500 From: Anthony Liguori 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> In-Reply-To: <4C720B1F.3030206@redhat.com> 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: Avi Kivity Cc: Blue Swirl , "Liu >> \"Liu, Jinsong\"" , qemu-devel , Markus Armbruster , Paul Brook On 08/23/2010 12:46 AM, Avi Kivity wrote: > On 08/23/2010 12:02 AM, Anthony Liguori wrote: >> On 08/22/2010 03:28 PM, Avi Kivity wrote: >>> On 08/20/2010 09:38 PM, Anthony Liguori wrote: >>>>> While that might be useful, I don't quite see what makes CPUs so >>>>> special >>>>> that they need to be kept out of qdev. Could be just my >>>>> ignorance, of >>>>> course. >>>> >>>> CPUs have special relationships with things like memory in QEMU. >>>> You can argue that a device is anything with pins and that CPUs are >>>> just like any other chip. >>> >>> We're not modelling chips! If we declare something a device, we do >>> it because it's functionally a device. It could be part of a chip, >>> or spread along multiple chips. >> >> 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. > 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 :-) >> 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. >> 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. Regards, Anthony Liguori