From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:49646) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RqlWe-0000sU-Ay for qemu-devel@nongnu.org; Fri, 27 Jan 2012 08:07:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RqlWa-00013A-Nd for qemu-devel@nongnu.org; Fri, 27 Jan 2012 08:07:32 -0500 Received: from mail-tul01m020-f173.google.com ([209.85.214.173]:47009) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RqlWa-000136-KX for qemu-devel@nongnu.org; Fri, 27 Jan 2012 08:07:28 -0500 Received: by obbup16 with SMTP id up16so1897239obb.4 for ; Fri, 27 Jan 2012 05:07:27 -0800 (PST) Message-ID: <4F22A18C.4090608@codemonkey.ws> Date: Fri, 27 Jan 2012 07:07:24 -0600 From: Anthony Liguori 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> In-Reply-To: <4F226562.6010300@siemens.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed 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: Jan Kiszka Cc: Peter Maydell , Anthony Liguori , qemu-devel@nongnu.org, Markus Armbruster , Avi Kivity , Paolo Bonzini 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. 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. > > It's undoubted that we need a more modern chipset than this ancient > PIIX3, rather sooner than later. And it is clear that there is a good > amount of generic functions in pc.c for building a PC, even a fairly > modern one. So we need a common lib for PC chipsets and would only > revert what you start here. Sorry, but I don't view this as a useful requirement. Today we support two types of PCs: an i440fx based system and an ISA-only system. We should concentrate on modeling those two systems in the most natural way sharing as much code as possible. When we add a third system, we'll figure out the best way to create additional abstractions. But inhibiting the proper modeling of what we support today in order to support some future thing is a bit backwards to me. Regards, Anthony Liguori > Please name what you do not like, and then we can fix that concretely. > > Jan >