From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:50810) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QJHIs-0007B2-IO for qemu-devel@nongnu.org; Sun, 08 May 2011 23:38:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QJHIq-000729-Qz for qemu-devel@nongnu.org; Sun, 08 May 2011 23:38:38 -0400 Received: from e23smtp02.au.ibm.com ([202.81.31.144]:58613) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QJHIq-00071r-7k for qemu-devel@nongnu.org; Sun, 08 May 2011 23:38:36 -0400 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [202.81.31.245]) by e23smtp02.au.ibm.com (8.14.4/8.13.1) with ESMTP id p493WhDu029184 for ; Mon, 9 May 2011 13:32:43 +1000 Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p493cNFg1048610 for ; Mon, 9 May 2011 13:38:25 +1000 Received: from d23av01.au.ibm.com (loopback [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p493cMaU013721 for ; Mon, 9 May 2011 13:38:23 +1000 Date: Mon, 9 May 2011 13:28:20 +1000 From: David Gibson Message-ID: <20110509032820.GD20682@yookeroo.fritz.box> References: <20110421070347.GG11968@yookeroo> <67FBBBAD-991F-48D8-901F-490C17B9DBC2@suse.de> <4DB9E2BA.7050602@twiddle.net> <4DBACADA.2000209@twiddle.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4DBACADA.2000209@twiddle.net> Subject: Re: [Qemu-devel] Supporting emulation of IOMMUs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: Anthony Liguori , aik@ozlabs.ru, Joerg.Rodel@amd.com, Alexander Graf , qemu-devel@nongnu.org, eduard.munteanu@linux360.ro On Fri, Apr 29, 2011 at 07:27:38AM -0700, Richard Henderson wrote: > On 04/28/2011 02:57 PM, Richard Henderson wrote: > > I've had a read through the patches posted in January. It all does > > seem relatively sane. At least, I can readily see how I would apply > > these interfaces to my Alpha port without trouble. > > I take that back, I see one rather annoying problem: the assumption > that the translate function operates on some sort of standalone device. > > This assumption is present in two places: Yeah, I noticed this problem too, though I think it's in the AMD IOMMU support patch rather than the DMA translation core. > > void pci_register_iommu(PCIDevice *dev, DMATranslateFunc *translate); > > Here, you're assuming that the IOMMU is a device on the PCI bus itself. > > While this may indeed be how the AMD-IOMMU presents itself for the > convenience of the pc-minded operating system, that's certainly not how > the hardware is implemented. In practice it's a function of the PCI > host controller. And indeed, that's how it is presented to the system > for the Sparc and Alpha controllers with which I am familiar. I assume > it's similar for PowerPC, but I've never looked. Yes, that's right. On pSeries platforms IOMMU translations are configured using hcalls. > > struct DMAMmu { > > DeviceState *iommu; > > DMATranslateFunc *translate; > > QLIST_HEAD(memory_maps, DMAMemoryMap) memory_maps; > > }; > > Here, you're assuming that the "iommu state" is a standalone qdev. > > This is probably true most of the time, given that > > FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev)) > > is true. However, the Alpha Typhoon chipset has two pci host > controllers that are tied together in ways that would be > difficult, or at least irritating, to represent as two separate > qdev entities. > > I suggest that, like many other places we have callbacks, this > should be an opaque value private to the translate function. > > In your AMD-IOMMU case you can then do > > pci_register_iommu(dev->bus, amd_iommu_translate, st); > > and avoid > > > PCIDevice *pci_dev = container_of(dev, PCIDevice, dma); > > PCIDevice *iommu_dev = DO_UPCAST(PCIDevice, qdev, dev->mmu->iommu); > > AMDIOMMUState *st = DO_UPCAST(AMDIOMMUState, dev, iommu_dev); > > at the beginning of your translate function. You currently have > three levels of casting and pointer chasing; surely you can agree > that having a single cast from void* is much easier to follow. > > In my Alpha case I can then do > > pci_register_iommu(pci_bus0, typhoon_iommu_translate, &state->pchip0); > pci_register_iommu(pci_bus1, typhoon_iommu_translate, &state->pchip1); > > and be off to the races. Yes, I think that makes sense. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson