From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:37350) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RaWey-0001IK-Ft for qemu-devel@nongnu.org; Tue, 13 Dec 2011 13:01:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RaWes-00078E-57 for qemu-devel@nongnu.org; Tue, 13 Dec 2011 13:01:00 -0500 Received: from mail-yw0-f45.google.com ([209.85.213.45]:59061) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RaWer-000788-Vs for qemu-devel@nongnu.org; Tue, 13 Dec 2011 13:00:54 -0500 Received: by yhgg71 with SMTP id g71so706809yhg.4 for ; Tue, 13 Dec 2011 10:00:53 -0800 (PST) Message-ID: <4EE792D1.4020704@codemonkey.ws> Date: Tue, 13 Dec 2011 12:00:49 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <4E70EC90.8000904@us.ibm.com> <201112130447.58486.paul@codesourcery.com> <4EE7517D.2060709@codemonkey.ws> <201112131740.29225.paul@codesourcery.com> In-Reply-To: <201112131740.29225.paul@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC] Plan for moving forward with QOM List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paul Brook Cc: Peter Maydell , Anthony Liguori , Jan Kiszka , qemu-devel@nongnu.org, Markus Armbruster , Gerd Hoffmann , "Edgar E. Iglesias" On 12/13/2011 11:40 AM, Paul Brook wrote: >> It's multiple inheritance but interfaces (implements) cannot carry state >> which avoids an inheritance diamond problem. > >>> It sounds like some >>> form of multiple inheritance, but it's unclear to me which should be the >>> primary type. For PCI host bridges like the i440fx we currently have to >>> split them into two, one fot the host bus and the other for the PCI bus. >>> It feels like if we design the model properly this should no longer be >>> necessary. >> >> Yes, that's exactly what this solves. I think we could probably make >> i440fx is-a PCIDevice that implements a PciBus. It's the same thing we >> would do with a PCI Bridge. > > Hmm, so how does that work given all of SysbusDevice, PCIDevice and PciBus > have state? Depends. If you look at PCIBus as an example: struct PCIBus { BusState qbus; uint8_t devfn_min; This is a characteristic that isn't common to all PCI busses. pci_set_irq_fn set_irq; pci_map_irq_fn map_irq; pci_hotplug_fn hotplug; These are essentially overload points and would be more natural if PCIBus was an interface. DeviceState *hotplug_qdev; This is a trick to support hotplug that can be refactored more nicely. void *irq_opaque; This is part of the set_irq interface. PCIDevice *devices[PCI_SLOT_MAX * PCI_FUNC_MAX]; PCIDevice *parent_dev; These are naturally links. MemoryRegion *address_space_mem; MemoryRegion *address_space_io; This would naturally be replaced with a pure virtual function overloaded by the child. A virtual function may be even better that simply used get_system_mem()/get_system_io(). QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */ QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */ Relics of the fact that PCI hasn't been full converted to an object model. /* The bus IRQ state is the logical OR of the connected devices. Keep a count of the number of devices with raised IRQs. */ int nirq; int *irq_count; Probably more naturally expressed as an AND gate. With a stateful pin model, this would end up modeled quite nicely. So at least for PCI, I think PCIBus as an interface works very well. > Making PciBus a separate (composition/shild) object is IMHO not a > bad thing anyway - I dislike saying that the device and the bus are the same > thing. Yeah, in some cases, this will almost certainly be the right thing to do. > That still leaves both PCIDevice and SysBusDevice state. The i440fx > is a fairly simple device, but more complex bridges have additional state that > needs to be accessible from both the PCI and SysBus interfaces. SysBus is so badly abused today I think we would have to reevaluate what to do with it once we fix up some of the devices that make use of it. > Similar examples occur when we have an audio codec with both an i2c command > interface and an SSI data port. > >>> I'm not so sure we should have Is-a at all. >> >> A point that has been repeatedly brought up is that you can avoid primary >> inheritance and just rely on composition using composed interfaces as your >> communication vehicle. >> >> I think in some cases, that will make sense, and the object model allows >> that. > > I'm not sure choice is a good thing here! One of the mistakes I made with > qdev was to insufficiently document how the device model was supposed to fit > together. FWIW, my next TODO is to write up a document on QOM. I will say, I learned a lot about the design of qdev from rewriting it :-) > This resulted in several bastardised qdev "conversions" involving > gratuitous layering violations. Yes, I never understood your comments at first but now I do understand where we went wrong and am very eager to avoid duplicating that. > The end result was that these devices are > effectively useless as qdev objects, and propagate all the problems that qdev > was designed to fix. > > It would be good to at least have guidelines on how and why the different > approaches should be used. Especially for those of us who maybe aren't > convesant with the more theoretical/academic principles/terminology behind OO > design. Yeah, that's the bit I need help with. I'll be putting together some slides for the next KVM call in January and will also post them on qemu-devel. I'm going to try very hard to explain what the design points are in reasonable terms. > If it's only a good idea in some cases then you can pretty much guarantee a > large number of developers will try and use it in the cases where it isn't, > and vice-versa ;-) > >>> It's somewhat unlear what the purpose of composition is. Using >>> composition for piix4::piiix and piix3::i8042 seems wrong. In fact >>> piix3 shouldn't have any knowledge of i8042 at all, it's just annother >>> random ISA device. >> >> i8042 is not a random ISA device. It lives in the piix3 and the piix3 has >> intimate knowledge of it. When model a20 line propagation, this is >> actually an important detail. > > Huh? I see absoutely no relationship between piix3 and i8042. > It happens that on the PC machine they are etched into the same piece of > silicon and one of the i8042 GPIO pins it connected to the A20 gate input, but > the devices have no actual knowledge of each other. Composition == "etching to the same piece of silicon". Nothing more, nothing less. A device has no knowledge of the fact that it is a child of another device. > If you're wanting to provide a convenient way of instantiating common clumps > of devices then I think that should be separate. Likewise defining convenient > machine specific aliases for users. There is a separate container device that lets you create arbitrary collections of devices, but in many cases, we can achieve better code reuse by using composition. Consider the serial device. In QOM, I've got it modelled as: MMSerial has-a UART ISASerial has-a UART And there are lots of cases where a design is compatible with a UART16650A but adds additional logic/features. In those cases, we would do: MCFSerial is-a UART Now you could certainly remove the has-a logic and make it so something had to instantiate a UART separately from the ISASerial device and connect things up but that's overly complicated IMHO. >>> What happens when my device has multiple parts that need to be referred >>> to by other devices? An interrupt controller is the most obvious >>> example, but similar issues occur for any device that has multiple >>> connections of the same type. Am I expected to create a whole bunch of >>> dummy devices and a private interface to bounce everything back to the >>> main device, somethig like: >>> >>> Type: MagicInterruptController >>> >>> Is-a: SystemBusDevice >>> Implements: MagicInterruptInterface >>> >>> Properties: >>> in_pin0: Composition>> controller=this> >>> in_pin1: Composition>> controller=this> >>> out_pin: link >> >> No, there would be a single 'Pin' class that would be used for irqs. There >> is no need to subclass it. >> >>> >>> Type: MagicInterruptController >>> >>> Is-a: SystemBusDevice >>> >>> Properties: >>> in_pin0: Target >>> in_pin1: Target >>> out_pin: Link >> >> So this is more like how it works today except s/Link/child/ and >> s/Target/link/g. > > Isn't that the other way around? The input pin owns the IRQ object, and the > output pin is the board/machine/user configured reference to that object. It just depends on your view of input vs. output. Either way works out just as well. > > So the link is to an Interface, not a device? Devices are Objects All Objects have Types. An interface is just another Type. Types can inherit from other types. All devices inherit from the DeviceState type. A link has a Type associated with it. The Type can be an interface, or it can be a stateful base class, or it can be a Type that isn't derived at all from Device (like a CharDriverState). > To support multiple instances > of the same interface we use a closure-like proxy object? If you needed to, but I think most of the time, that's not necessary. Regards, Anthony Liguori > I guess it we assume the number of interface classes is relatively small that > should work. > > Paul >