From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=41882 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OhAiB-0002Y1-Si for qemu-devel@nongnu.org; Thu, 05 Aug 2010 20:23:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OhAiA-0004uu-NC for qemu-devel@nongnu.org; Thu, 05 Aug 2010 20:22:59 -0400 Received: from mail-fx0-f45.google.com ([209.85.161.45]:62659) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OhAiA-0004um-Ii for qemu-devel@nongnu.org; Thu, 05 Aug 2010 20:22:58 -0400 Received: by fxm7 with SMTP id 7so2831787fxm.4 for ; Thu, 05 Aug 2010 17:22:57 -0700 (PDT) Sender: Eduard - Gabriel Munteanu Date: Fri, 6 Aug 2010 03:21:28 +0300 From: Eduard - Gabriel Munteanu Subject: Re: [Qemu-devel] [RFC PATCH 1/4] pci: memory access API and IOMMU support Message-ID: <20100806002128.GA19250@localhost> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: joro@8bytes.org, paul@codesourcery.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, avi@redhat.com On Thu, Aug 05, 2010 at 09:23:30PM +0000, Blue Swirl wrote: > On Wed, Aug 4, 2010 at 10:32 PM, Eduard - Gabriel Munteanu [snip] > > @@ -58,6 +58,10 @@ struct PCIBus { > > ?? ?? ?? ??Keep a count of the number of devices with raised IRQs. ??*/ > > ?? ?? int nirq; > > ?? ?? int *irq_count; > > + > > +#ifdef CONFIG_PCI_IOMMU > > The code should not be conditional. > > > + ?? ??PCIIOMMU *iommu; > > +#endif > > ??}; > > > > ??static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent); > > @@ -2029,6 +2033,147 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent) > > ?? ?? } > > ??} > > > > +#ifdef CONFIG_PCI_IOMMU > > + > > +void pci_register_iommu(PCIDevice *dev, PCIIOMMU *iommu) > > +{ > > + ?? ??dev->bus->iommu = iommu; > > +} > > + > > +void pci_memory_rw(PCIDevice *dev, > > + ?? ?? ?? ?? ?? ?? ?? ?? ?? pci_addr_t addr, > > + ?? ?? ?? ?? ?? ?? ?? ?? ?? uint8_t *buf, > > + ?? ?? ?? ?? ?? ?? ?? ?? ?? pci_addr_t len, > > + ?? ?? ?? ?? ?? ?? ?? ?? ?? int is_write) > > +{ > > + ?? ??int err, plen; > > + ?? ??unsigned perms; > > + ?? ??PCIIOMMU *iommu = dev->bus->iommu; > > + ?? ??target_phys_addr_t paddr; > > + > > + ?? ??if (!iommu || !iommu->translate) > > + ?? ?? ?? ??return cpu_physical_memory_rw(addr, buf, len, is_write); > > Instead of these kind of checks, please add default handlers which > call cpu_physical_memory_rw() etc. > Ok. I'm trying to minimize impact (non-inlineable function calls) when the IOMMU is disabled at compile-time. I think I can do it some other way, as you suggest. > > + > > + ?? ??perms = is_write ? IOMMU_PERM_WRITE : IOMMU_PERM_READ; > > Is this useful? How about just passing is_write as perms? > Only in theory: it might come in handy if we ever support RW operations, like read-modify-write memory maps. Also, write permissions include zero-byte reads for the AMD IOMMU, so IOMMU_PERM_* could be further refined. I'm happy to remove it, though. [snip] > > +/* > > + * Memory I/O and PCI IOMMU definitions. > > + */ > > + > > +typedef target_phys_addr_t pci_addr_t; > > There is already pcibus_t. > Thanks, I'll use that. > > + > > +typedef int PCIInvalidateIOTLBFunc(void *opaque); > > I think some type safety tricks could be used with for example PCIDevice *. > Note that 'opaque' belongs to the caller (the code that requests memory maps). Some device might make multiple maps that can be invalidated separately. The actual stuff that describes the map might not be straightforward to recover from a PCIDevice. We could add another parameter to PCIInvalidateIOTLBFunc(), but since the main user is DMA code, it's going to complicate things further. [snip] Eduard