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 <da...@gibson.dropbear.id.au>
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