From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Weil Subject: Re: [Qemu-devel] [PATCH 2/7] pci: memory access API and IOMMU support Date: Wed, 01 Sep 2010 22:10:30 +0200 Message-ID: <4C7EB336.40003@mail.berlios.de> References: <1283119703-9781-1-git-send-email-eduard.munteanu@linux360.ro> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Cc: mst@redhat.com, kvm@vger.kernel.org, joro@8bytes.org, qemu-devel@nongnu.org, blauwirbel@gmail.com, yamahata@valinux.co.jp, paul@codesourcery.com, avi@redhat.com To: Eduard - Gabriel Munteanu Return-path: Received: from moutng.kundenserver.de ([212.227.126.186]:53314 "EHLO moutng.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755881Ab0IAULA (ORCPT ); Wed, 1 Sep 2010 16:11:00 -0400 In-Reply-To: <1283119703-9781-1-git-send-email-eduard.munteanu@linux360.ro> Sender: kvm-owner@vger.kernel.org List-ID: Please see my comments at the end of this mail. Am 30.08.2010 00:08, schrieb Eduard - Gabriel Munteanu: > PCI devices should access memory through pci_memory_*() instead of > cpu_physical_memory_*(). This also provides support for translation and > access checking in case an IOMMU is emulated. > > Memory maps are treated as remote IOTLBs (that is, translation caches > belonging to the IOMMU-aware device itself). Clients (devices) must > provide callbacks for map invalidation in case these maps are > persistent beyond the current I/O context, e.g. AIO DMA transfers. > > Signed-off-by: Eduard - Gabriel Munteanu > --- > hw/pci.c | 191 +++++++++++++++++++++++++++++++++++++++++++++++++++- > hw/pci.h | 69 +++++++++++++++++++ > hw/pci_internals.h | 12 +++ > qemu-common.h | 1 + > 4 files changed, 272 insertions(+), 1 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index 2dc1577..afcb33c 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > > ... > > diff --git a/hw/pci.h b/hw/pci.h > index c551f96..c95863a 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -172,6 +172,8 @@ struct PCIDevice { > char *romfile; > ram_addr_t rom_offset; > uint32_t rom_bar; > + > + QLIST_HEAD(, PCIMemoryMap) memory_maps; > }; > > PCIDevice *pci_register_device(PCIBus *bus, const char *name, > @@ -391,4 +393,71 @@ static inline int ranges_overlap(uint64_t first1, > uint64_t len1, > return !(last2 < first1 || last1 < first2); > } > > +/* > + * Memory I/O and PCI IOMMU definitions. > + */ > + > +#define IOMMU_PERM_READ (1 << 0) > +#define IOMMU_PERM_WRITE (1 << 1) > +#define IOMMU_PERM_RW (IOMMU_PERM_READ | IOMMU_PERM_WRITE) > + > +typedef int PCIInvalidateMapFunc(void *opaque); > +typedef int PCITranslateFunc(PCIDevice *iommu, > + PCIDevice *dev, > + pcibus_t addr, > + target_phys_addr_t *paddr, > + target_phys_addr_t *len, > + unsigned perms); > + > +void pci_memory_rw(PCIDevice *dev, > + pcibus_t addr, > + uint8_t *buf, > + pcibus_t len, > + int is_write); > +void *pci_memory_map(PCIDevice *dev, > + PCIInvalidateMapFunc *cb, > + void *opaque, > + pcibus_t addr, > + target_phys_addr_t *len, > + int is_write); > +void pci_memory_unmap(PCIDevice *dev, > + void *buffer, > + target_phys_addr_t len, > + int is_write, > + target_phys_addr_t access_len); > +void pci_register_iommu(PCIDevice *dev, PCITranslateFunc *translate); > +void pci_memory_invalidate_range(PCIDevice *dev, pcibus_t addr, > pcibus_t len); > + > +#define DECLARE_PCI_LD(suffix, size) \ > +uint##size##_t pci_ld##suffix(PCIDevice *dev, pcibus_t addr); > + > +#define DECLARE_PCI_ST(suffix, size) \ > +void pci_st##suffix(PCIDevice *dev, pcibus_t addr, uint##size##_t val); > + > +DECLARE_PCI_LD(ub, 8) > +DECLARE_PCI_LD(uw, 16) > +DECLARE_PCI_LD(l, 32) > +DECLARE_PCI_LD(q, 64) > + > +DECLARE_PCI_ST(b, 8) > +DECLARE_PCI_ST(w, 16) > +DECLARE_PCI_ST(l, 32) > +DECLARE_PCI_ST(q, 64) > + > +static inline void pci_memory_read(PCIDevice *dev, > + pcibus_t addr, > + uint8_t *buf, > + pcibus_t len) > +{ > + pci_memory_rw(dev, addr, buf, len, 0); > +} > + > +static inline void pci_memory_write(PCIDevice *dev, > + pcibus_t addr, > + const uint8_t *buf, > + pcibus_t len) > +{ > + pci_memory_rw(dev, addr, (uint8_t *) buf, len, 1); > +} > + > #endif The functions pci_memory_read and pci_memory_write not only read or write byte data but many different data types which leads to a lot of type casts in your other patches. I'd prefer "void *buf" and "const void *buf" in the argument lists. Then all those type casts could be removed. Regards Stefan Weil From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=38142 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OqtnB-00028U-6I for qemu-devel@nongnu.org; Wed, 01 Sep 2010 16:20:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Oqte4-0004tl-Tp for qemu-devel@nongnu.org; Wed, 01 Sep 2010 16:10:58 -0400 Received: from moutng.kundenserver.de ([212.227.126.186]:58267) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Oqte4-0004tM-H5 for qemu-devel@nongnu.org; Wed, 01 Sep 2010 16:10:56 -0400 Message-ID: <4C7EB336.40003@mail.berlios.de> Date: Wed, 01 Sep 2010 22:10:30 +0200 From: Stefan Weil MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 2/7] pci: memory access API and IOMMU support References: <1283119703-9781-1-git-send-email-eduard.munteanu@linux360.ro> In-Reply-To: <1283119703-9781-1-git-send-email-eduard.munteanu@linux360.ro> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduard - Gabriel Munteanu Cc: kvm@vger.kernel.org, mst@redhat.com, joro@8bytes.org, qemu-devel@nongnu.org, blauwirbel@gmail.com, yamahata@valinux.co.jp, paul@codesourcery.com, avi@redhat.com Please see my comments at the end of this mail. Am 30.08.2010 00:08, schrieb Eduard - Gabriel Munteanu: > PCI devices should access memory through pci_memory_*() instead of > cpu_physical_memory_*(). This also provides support for translation and > access checking in case an IOMMU is emulated. > > Memory maps are treated as remote IOTLBs (that is, translation caches > belonging to the IOMMU-aware device itself). Clients (devices) must > provide callbacks for map invalidation in case these maps are > persistent beyond the current I/O context, e.g. AIO DMA transfers. > > Signed-off-by: Eduard - Gabriel Munteanu > --- > hw/pci.c | 191 +++++++++++++++++++++++++++++++++++++++++++++++++++- > hw/pci.h | 69 +++++++++++++++++++ > hw/pci_internals.h | 12 +++ > qemu-common.h | 1 + > 4 files changed, 272 insertions(+), 1 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index 2dc1577..afcb33c 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > > ... > > diff --git a/hw/pci.h b/hw/pci.h > index c551f96..c95863a 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -172,6 +172,8 @@ struct PCIDevice { > char *romfile; > ram_addr_t rom_offset; > uint32_t rom_bar; > + > + QLIST_HEAD(, PCIMemoryMap) memory_maps; > }; > > PCIDevice *pci_register_device(PCIBus *bus, const char *name, > @@ -391,4 +393,71 @@ static inline int ranges_overlap(uint64_t first1, > uint64_t len1, > return !(last2 < first1 || last1 < first2); > } > > +/* > + * Memory I/O and PCI IOMMU definitions. > + */ > + > +#define IOMMU_PERM_READ (1 << 0) > +#define IOMMU_PERM_WRITE (1 << 1) > +#define IOMMU_PERM_RW (IOMMU_PERM_READ | IOMMU_PERM_WRITE) > + > +typedef int PCIInvalidateMapFunc(void *opaque); > +typedef int PCITranslateFunc(PCIDevice *iommu, > + PCIDevice *dev, > + pcibus_t addr, > + target_phys_addr_t *paddr, > + target_phys_addr_t *len, > + unsigned perms); > + > +void pci_memory_rw(PCIDevice *dev, > + pcibus_t addr, > + uint8_t *buf, > + pcibus_t len, > + int is_write); > +void *pci_memory_map(PCIDevice *dev, > + PCIInvalidateMapFunc *cb, > + void *opaque, > + pcibus_t addr, > + target_phys_addr_t *len, > + int is_write); > +void pci_memory_unmap(PCIDevice *dev, > + void *buffer, > + target_phys_addr_t len, > + int is_write, > + target_phys_addr_t access_len); > +void pci_register_iommu(PCIDevice *dev, PCITranslateFunc *translate); > +void pci_memory_invalidate_range(PCIDevice *dev, pcibus_t addr, > pcibus_t len); > + > +#define DECLARE_PCI_LD(suffix, size) \ > +uint##size##_t pci_ld##suffix(PCIDevice *dev, pcibus_t addr); > + > +#define DECLARE_PCI_ST(suffix, size) \ > +void pci_st##suffix(PCIDevice *dev, pcibus_t addr, uint##size##_t val); > + > +DECLARE_PCI_LD(ub, 8) > +DECLARE_PCI_LD(uw, 16) > +DECLARE_PCI_LD(l, 32) > +DECLARE_PCI_LD(q, 64) > + > +DECLARE_PCI_ST(b, 8) > +DECLARE_PCI_ST(w, 16) > +DECLARE_PCI_ST(l, 32) > +DECLARE_PCI_ST(q, 64) > + > +static inline void pci_memory_read(PCIDevice *dev, > + pcibus_t addr, > + uint8_t *buf, > + pcibus_t len) > +{ > + pci_memory_rw(dev, addr, buf, len, 0); > +} > + > +static inline void pci_memory_write(PCIDevice *dev, > + pcibus_t addr, > + const uint8_t *buf, > + pcibus_t len) > +{ > + pci_memory_rw(dev, addr, (uint8_t *) buf, len, 1); > +} > + > #endif The functions pci_memory_read and pci_memory_write not only read or write byte data but many different data types which leads to a lot of type casts in your other patches. I'd prefer "void *buf" and "const void *buf" in the argument lists. Then all those type casts could be removed. Regards Stefan Weil