From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:34448) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rqmai-0003aH-FB for qemu-devel@nongnu.org; Fri, 27 Jan 2012 09:15:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rqmac-0002s1-DK for qemu-devel@nongnu.org; Fri, 27 Jan 2012 09:15:48 -0500 Received: from david.siemens.de ([192.35.17.14]:18283) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rqmac-0002rn-1P for qemu-devel@nongnu.org; Fri, 27 Jan 2012 09:15:42 -0500 Message-ID: <4F22B189.9090407@siemens.com> Date: Fri, 27 Jan 2012 15:15:37 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <1327604460-31142-1-git-send-email-aliguori@us.ibm.com> <1327604460-31142-2-git-send-email-aliguori@us.ibm.com> <4F226562.6010300@siemens.com> <4F22A18C.4090608@codemonkey.ws> <4F22A777.5010806@siemens.com> <4F22AF7A.6040601@us.ibm.com> In-Reply-To: <4F22AF7A.6040601@us.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 01/15] pc: merge pc_piix.c into pc.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: "qemu-devel@nongnu.org" , Peter Maydell , Avi Kivity , Markus Armbruster , Paolo Bonzini On 2012-01-27 15:06, Anthony Liguori wrote: > On 01/27/2012 07:32 AM, Jan Kiszka wrote: >> On 2012-01-27 14:07, Anthony Liguori wrote: >>> On 01/27/2012 02:50 AM, Jan Kiszka wrote: >>>> On 2012-01-26 20:00, Anthony Liguori wrote: >>>>> A long time ago, there was a grand plan to merge q35 chipset support. The start >>>>> of that series was a refactoring of pc.c which split a bunch of the "common" >>>>> functionality into a separate file that could be shared by the two. >>>>> >>>>> But q35 never got merged and the refactoring, in retrospect, just made things >>>>> worse. Making things proper objects and using composition is the right way >>>>> to share common devices. >>>>> >>>>> By pulling these files back together, we can start to fix some of this mess. >>>> >>>> There are surely things to clean up and improve, but a clear NACK for >>>> the general direction. >>> >>> Hi Jan, >>> >>> I think you're missing the bigger picture here. Once this refactoring finishes, >>> here's what we'll be left with: >>> >>> 1) pc_init creates an I440FX, any bus devices (ISA serial port, PCI vga and >>> nics, etc.), sets properties appropriately, and realizes the devices. >>> >>> 2) I440FX is-a PCIHost, has-a I440FX-PMC, has-a PIIX3 >>> >>> 3) PIIX3 has-a RTC, has-a I8042, has-a DMAController, etc. >>> >>> Memory creation is done by the I440FX-PMC. >>> >>> Now it's true that a newer chipset is going to be similar. It will likely have >>> a SuperI/O chip that looks similar to PIIX3. The right way to share code would >>> be to move most of the PIIX3 functionality to a base class (PCSuperIO) that >>> PIIX3 inherits from. >>> >>> This is probably how to support ISAPC properly too. >> >> The ISAPC is differently composed. The board creates all those >> individual chips that are otherwise part of the SuperIO block of the >> chipset. And IRQ wiring is different. > > A SuperIO device can still have irq properties for each device that determines > which ISA bus irq is used. > > The other alternative is to stop making devices ISADevice, and then having > SuperIO expose a bunch of device specific IRQs. Those IRQs can then be routed > in whatever way makes sense. > > This is really the Right Approach but it's most likely a compatibility breaker > so I'm trying my best to avoid these types of refactorings right now. > >> So, no, I don't think it is the >> right model. I would rather think of a pc_isa.c that does the proper >> composing. > > Objects compose other objects. Functions should not be creating devices. So if > you're view of pc_isa.c that there's a pc_basic_init() that takes a bunch of > devicestate pointers, then that's definitely not the direction I'm heading. > > Note that if we ever want to get to a board configuration file, we need to have > three explicit steps in device creation: > > 1) device initialization where devices are allocates and the composition tree is > filled out (which makes sure that every device has an addressable path) > > 2) device property setting (which requires all devices have an addressable path) > > 3) device realization where property settings are validated, and then any > initialization that depends on properties is done. > > The problem with the current code is that it doesn't split up these phases. > Modeling composition really helps to get this split because it forces you to > think about things in terms of distinct phases. > >>> Once I sort out interrupts, >>> I'll attempt to tackle that. My guess is that a SuperIO chip could be an >>> ISADevice and that we could simply make the PIIX3 has-a SuperIO. Then the ISAPC >>> would have a trivial ISA chipset that has-a SuperIO. >>> >>> This is fairly trivial to do once we have the right structure to the code. >>> >>> But the current code has the wrong structure which is why there's so much >>> pointer chasing and passing. >> >> Just look at your code and count the generic, PIIX3-independent >> functions. > > I see zero. Then you should look more carefully: - GSI handler - ferr - tsc - smm - pic irq handling - cmos - boot devices - port92 - a20 well, this gets boring. > > "PIIX3-independent" means that some other piece of code would consume it in > exactly the same way. pc.c had a bunch of spaghetti in it doing tricks with if > (pci_enabled) that ended up encoding two very separate paths into one maze of code. Right, the still existing pci_enabled mess should be resolved via separate board instantiations that are based on common building blocks. That avoids also regression in the ISA PC due to refactorings in the PIIX3 as happened recently. > >> Keep them in pc.c, move the rest to pc_piix.c. You could try >> to model the ISA accordingly. I think some pc_isa.c would help to >> establish a good split-up already now, in the absence of a third chipset. > > I think creating a proper ISA model is a good idea and I'll certainly get there. At least here we agree. :) BTW, a third board we already have today is Xen. Same story. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux