From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41731) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RAJEF-0000pL-NG for qemu-devel@nongnu.org; Sun, 02 Oct 2011 06:25:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RAJEC-0003Po-BG for qemu-devel@nongnu.org; Sun, 02 Oct 2011 06:25:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29848) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RAJEB-0003Pk-OU for qemu-devel@nongnu.org; Sun, 02 Oct 2011 06:25:00 -0400 Date: Sun, 2 Oct 2011 12:25:48 +0200 From: "Michael S. Tsirkin" Message-ID: <20111002102547.GC30747@redhat.com> References: <1315197304-22469-1-git-send-email-david@gibson.dropbear.id.au> <1315197304-22469-2-git-send-email-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1315197304-22469-2-git-send-email-david@gibson.dropbear.id.au> Subject: Re: [Qemu-devel] [PATCH 1/9] Add stub functions for PCI device models to do PCI DMA List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: aliguori@us.ibm.com, kraxel@redhat.com, joerg.roedel@amd.com, qemu-devel@nongnu.org, agraf@suse.de, avi@redhat.com, eduard.munteanu@linux360.ro, rth@twiddle.net On Mon, Sep 05, 2011 at 02:34:56PM +1000, David Gibson wrote: > This patch adds functions to pci.[ch] to perform PCI DMA operations. At > present, these are just stubs which perform directly cpu physical memory > accesses. > > Using these stubs, however, distinguishes PCI device DMA transactions from > other accesses to physical memory, which will allow PCI IOMMU support to > be added in one place, rather than updating every PCI driver at that time. > > That is, it allows us to update individual PCI drivers to support an IOMMU > without having yet determined the details of how the IOMMU emulation will > operate. This will let us remove the most bitrot-sensitive part of an > IOMMU patch in advance. > > Signed-off-by: David Gibson So something I just thought about: all wrappers now go through cpu_physical_memory_rw. This is a problem as e.g. virtio assumes that accesses such as stw are atomic. cpu_physical_memory_rw is a memcpy which makes no such guarantees. > --- > dma.h | 2 ++ > hw/pci.c | 31 +++++++++++++++++++++++++++++++ > hw/pci.h | 33 +++++++++++++++++++++++++++++++++ > 3 files changed, 66 insertions(+), 0 deletions(-) > > diff --git a/dma.h b/dma.h > index a6db5ba..06e91cb 100644 > --- a/dma.h > +++ b/dma.h > @@ -15,6 +15,8 @@ > #include "hw/hw.h" > #include "block.h" > > +typedef target_phys_addr_t dma_addr_t; > + > typedef struct { > target_phys_addr_t base; > target_phys_addr_t len; > diff --git a/hw/pci.c b/hw/pci.c > index 1cdcbb7..0be7611 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -2211,3 +2211,34 @@ MemoryRegion *pci_address_space(PCIDevice *dev) > { > return dev->bus->address_space_mem; > } > + > +#define PCI_DMA_DEFINE_LDST(_lname, _sname, _bits) \ > + uint##_bits##_t ld##_lname##_pci_dma(PCIDevice *dev, dma_addr_t addr) \ > + { \ > + uint##_bits##_t val; \ > + pci_dma_read(dev, addr, &val, sizeof(val)); \ > + return le##_bits##_to_cpu(val); \ > + } \ > + void st##_sname##_pci_dma(PCIDevice *dev, \ > + dma_addr_t addr, uint##_bits##_t val) \ > + { \ > + val = cpu_to_le##_bits(val); \ > + pci_dma_write(dev, addr, &val, sizeof(val)); \ > + } > + I am still not 100% positive why do we do the LE conversions here. st4_phys and friends don't seem to do it ... Has something to do with the fact we pass a value as an array? Probably worth a comment. > +uint8_t ldub_pci_dma(PCIDevice *dev, dma_addr_t addr) > +{ > + uint8_t val; > + > + pci_dma_read(dev, addr, &val, sizeof(val)); > + return val; > +} > + > +void stb_pci_dma(PCIDevice *dev, dma_addr_t addr, uint8_t val) > +{ > + pci_dma_write(dev, addr, &val, sizeof(val)); > +} > + pci_ XXX would be better names? > +PCI_DMA_DEFINE_LDST(uw, w, 16); > +PCI_DMA_DEFINE_LDST(l, l, 32); > +PCI_DMA_DEFINE_LDST(q, q, 64); > diff --git a/hw/pci.h b/hw/pci.h > index 391217e..4426e9d 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -6,6 +6,7 @@ > > #include "qdev.h" > #include "memory.h" > +#include "dma.h" > > /* PCI includes legacy ISA access. */ > #include "isa.h" > @@ -492,4 +493,36 @@ static inline uint32_t pci_config_size(const PCIDevice *d) > return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE; > } > > +/* DMA access functions */ > +static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr, > + void *buf, dma_addr_t len, int is_write) > +{ > + cpu_physical_memory_rw(addr, buf, len, is_write); > + return 0; > +} > + > +static inline int pci_dma_read(PCIDevice *dev, dma_addr_t addr, > + void *buf, dma_addr_t len) > +{ > + return pci_dma_rw(dev, addr, buf, len, 0); > +} > + > +static inline int pci_dma_write(PCIDevice *dev, dma_addr_t addr, > + const void *buf, dma_addr_t len) > +{ > + return pci_dma_rw(dev, addr, (void *) buf, len, 1); > +} > + > +#define PCI_DMA_DECLARE_LDST(_lname, _sname, _bits) \ > + uint##_bits##_t ld##_lname##_pci_dma(PCIDevice *dev, dma_addr_t addr); \ > + void st##_sname##_pci_dma(PCIDevice *dev, dma_addr_t addr, \ > + uint##_bits##_t val); \ > + > +PCI_DMA_DECLARE_LDST(ub, b, 8); > +PCI_DMA_DECLARE_LDST(uw, w, 16); > +PCI_DMA_DECLARE_LDST(l, l, 32); > +PCI_DMA_DECLARE_LDST(q, q, 64); > + > +#undef DECLARE_LDST_DMA > + I think macros should just create stX_phys/ldX_phys calls directly, in the .h file. This will also make it clearer what is going on, with less levels of indirection. > #endif > -- > 1.7.5.4