From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:57844) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QFofX-0002JQ-AI for qemu-devel@nongnu.org; Fri, 29 Apr 2011 10:27:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QFofW-0005qM-Fl for qemu-devel@nongnu.org; Fri, 29 Apr 2011 10:27:43 -0400 Received: from mail-pz0-f45.google.com ([209.85.210.45]:42267) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QFofW-0005qB-94 for qemu-devel@nongnu.org; Fri, 29 Apr 2011 10:27:42 -0400 Received: by pzk30 with SMTP id 30so2689843pzk.4 for ; Fri, 29 Apr 2011 07:27:41 -0700 (PDT) Sender: Richard Henderson Message-ID: <4DBACADA.2000209@twiddle.net> Date: Fri, 29 Apr 2011 07:27:38 -0700 From: Richard Henderson MIME-Version: 1.0 References: <20110421070347.GG11968@yookeroo> <67FBBBAD-991F-48D8-901F-490C17B9DBC2@suse.de> <4DB9E2BA.7050602@twiddle.net> In-Reply-To: <4DB9E2BA.7050602@twiddle.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] Supporting emulation of IOMMUs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Anthony Liguori , aik@ozlabs.ru, Joerg.Rodel@amd.com, qemu-devel@nongnu.org, eduard.munteanu@linux360.ro, David Gibson 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: > 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. > 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. r~