From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:55831) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QlzuA-0000io-C7 for qemu-devel@nongnu.org; Wed, 27 Jul 2011 04:55:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qlzu9-0002Io-0n for qemu-devel@nongnu.org; Wed, 27 Jul 2011 04:55:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24667) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qlzu8-0002Ij-Nh for qemu-devel@nongnu.org; Wed, 27 Jul 2011 04:55:48 -0400 Message-ID: <4E2FD28F.2070206@redhat.com> Date: Wed, 27 Jul 2011 10:55:43 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1311558293-5855-1-git-send-email-aliguori@us.ibm.com> <4E2EBA1E.90006@redhat.com> <4E2EC90E.8090409@codemonkey.ws> <4E2ED0AA.3020101@redhat.com> <4E2EDE86.7020807@codemonkey.ws> <4E2F06C8.30403@redhat.com> <4E2F1448.3040106@codemonkey.ws> In-Reply-To: <4E2F1448.3040106@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC][PATCH 0/21] QEMU Object Model List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Peter Maydell , qemu-devel@nongnu.org On 07/26/2011 09:23 PM, Anthony Liguori wrote: > On 07/26/2011 01:26 PM, Paolo Bonzini wrote: >> On 07/26/2011 05:34 PM, Anthony Liguori wrote: >> You could just as well say that real life works like this: >> >> class PciSocket { >> PciBus *pci_bus; >> uint8_t *config; >> uint8_t *cmask; >> uint8_t *wmask; >> uint8_t *w1cmask; >> uint8_t *used; >> uint32_t devfn; >> char name[64]; >> PCIIORegion io_regions[PCI_NUM_REGIONS]; >> ... >> >> }; >> >> class IsaSocket { >> IsaBus *isa_bus; >> uint32_t isairq[2]; // not sure this is a good idea, just >> int nirqs; // mimicking qdev >> uint16_t ioports[32];// statically assigned unlike PCI bars >> int nioports; >> } >> >> class MyWeirdDevice : public MyBaseDevice { >> PciSocket pci; >> IsaSocket isa; >> }; > > Hrm, I'm not sure I buy that entirely. I think it would be: > > class MyWeirdPciView : public PciDevice { > PciBus *bus; > MyWeirdDevice *dev; > }; > > class MyWeirdIsaView : public IsaDevice { > IsaBus *bus; > MyWeirdDevice *dev; > }; > > class MyWeirdDevice : public MyBaseDevice { > MyWeirdPciView pci; > MyWeirdIsaView isa; > } > > The views are basically bridges between PCI/ISA and an internal > interface (MyWeirdDevice). That's yet another possibility. There are two difference with what I had written: 1) the back-pointer from MyWeird*View to MyWeirdDevice replaced by container_of. That's cosmetic; 2) I'm not considering the sockets to be separate devices. That's the difference between "proxying PCI to another device" (wrong, hardware does not do that) and "isolating PCI knowledge within a sub-object of the device" (just an encapsulation choice, hardware is irrelevant). > I don't think the proxy design pattern is the right thing to use. > 95% of the time, the device is intrinsically a PCI device. It's not a proxy. It's replacing inheritance with containment to get the flexibility of data MI without the complexity of diamond hierarchies. > The other 5% of the > time, the device has a well defined interface, and then there is > effectively a PCI bridge. But that PCI bridge isn't generic, it's > specific to that custom interface. Yes, that can be represented by composition if you wish (an ISASerialState containing an 8250 is the canonical example; the 8139 example below is another). >>> There's a pin in the IDE cable that determines master or slave depending >>> on whether it's raised high. >> >> Yes, that's the "newer" way. There used to be jumpers to choose between >> master, slave and cable-select. > > That jumper raises the bit on the wire. Ah ok, I was confusing it with the cable-select wire. My point was that you can choose the jumper representation (low-level) or the cable-select representation (high-level, actually matches modern hardware, even better from the user POV). One is easier to manage and better in all aspects, but both make sense. It's irrelevant to this discussion anyway. >>>>> Interfaces are the right way to do this. Getting MI right is fairly >>>>> hard >>>> >>>> But we don't need is-a, we need has-a. Multiple is-a is harder than >>>> single is-a. Multiple has-a does not add any complication. >>> >>> Yeah, that's what plug properties are for :-) >> >> I agree, but at the cost of pointer chasing and making it harder to >> implement get_device_for_socket for buses that need it (in the above >> sketch it can be a simple container_of). > > Can we be a bit more concrete as I'm having a hard time following your > logic. You're assuming a generic PciSocket class, right? I think that's > not the right approach, as an example: > > class Rtl8139PciBridge : public PciDevice > { > Rtl8139 rtldev; > }; > > class Rtl8139IsaBridge : public IsaDevice > { > Rtl8139 rtldev; > }; > > With Avi's new memory API, we'd have: > > class Rtl8139 : public Device > { > MemoryRegion region[2]; > Pin irq; > }; > > And then the construction code for Rtl8139PciBridge would register the > regions as bars, and connect the PCI lnk to rtldev.irq. > > The ISA code would do something similar. Yes, this looks nice (modulo s/Rtl8139/Rtl8139 */). But it is not that much more flexible than qdev 1.0. You're right that for the case of two parents above we were looking at a contrived example. The Goldfish platform provides a more interesting one. There you have an interrupt controller and a bus enumerator device. Most devices are connected to both, but conflating them is wrong for many reasons; 1) the bus enumerator itself uses an interrupt (raised on hotplug); 2) you can enumerate devices that do not have an interrupt line, and you shouldn't need to connect such a device to an interrupt controller; 3) the interrupt controller and bus enumerator use two separate MMIO areas; 4) in principle, other devices could use the interrupt controller (which is the only component connected to the CPU's interrupt line) without being enumerated. 5) A device with two interrupts has two "interrupt interfaces" and only one "enumerator interface". 6) The enumerator interface does not have bus semantics. The enumerator also enumerates itself so it would act as both the bus host and a device on the bus. Composition then lets you use something like this: class GoldfishPIC : Device { Pin parent; GoldfishInterruptPin *children[32]; Pin (*init_socket_for_irq_num) (GoldfishInterruptPin *, int); }; class GoldfishInterruptPin { GoldfishPIC *pic; Pin irqnum; }; class GoldfishEnumerator : Device { GoldfishInterruptPin irq; GoldfishBusDeviceInfo info; List allDevices; ... }; class GoldfishBusDeviceInfo { GoldfishEnumerator *parent; char *name; int id; ram_addr_t mmio; int mmio_size; int irq_base; int irq_count; }; class GoldfishTTY : Device { GoldfishInterruptPin irq; GoldfishBusDeviceInfo info; CharDev *chardev; }; Here you do not need a bus to mediate the access between the TTY device and its parents, it would only introduce unnecessary complication (boilerplate and pointer chasing). It also would not match hardware at all in the case of the enumerator. >>> Indeed. I think that it's a no brainer for the backends and that's why >>> I'm starting there. >> >> I don't think it's a no brainer. It's simply much easier, but right now >> it is also a solution in search of a problem (if all you want is dynamic >> creation of character devices, you could do that without a generic >> object model). > > And that solves the problem yet again for one layer. But what about > block, fsdev, and DisplayState? We can keep reinventing the wheel over > and over again in slightly different ways or we can try to do something > that will work for more things. > > We need more commonality in QEMU. Things are hard to follow because > every subsystem is an island. I am not saying you're wrong. I am saying it is very hard to do it on a mature (no matter how messy) code base. > We need lots of new transitions, we need to strive to make things > better. But we need to do things in such a way that: > > (1) we have a good idea of what we're going to end up with at the end of > the day > > (2) there is incremental value being added to QEMU at every step of the way > > This cannot be done by simply hacking some bits here and there. It > requires design, planning, and flag days when appropriate. Agreed. The problem I have with QOM is (2). I am not sure we do get incremental value at every step of the way. We do get incremental value in the char layer, but we also get additional complexity until the transition is over. Paolo