Re: [Qemu-devel] [PATCH 01/10] Add stub functions for PCI device models to do PCI DMA

[Anthony Liguori <aliguori@us.ibm.com>]

On 09/01/2011 12:00 AM, 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<david@gibson.dropbear.id.au>

I think this is the wrong approach given the introduction of the memory API.

I think we should have a generic memory access function that takes a 
MemoryRegion as it's first argument.

The PCI bus should then expose one memory region for each device (that's 
how it can figure out where the access is coming from).

Richard/Avi, what do you think?

Regards,

Anthony Liguori

> ---
>   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..842b066 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 DEFINE_LDST_DMA(_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)); \
> +    }
> +
> +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));
> +}
> +
> +DEFINE_LDST_DMA(uw, w, 16);
> +DEFINE_LDST_DMA(l, l, 32);
> +DEFINE_LDST_DMA(q, q, 64);
> diff --git a/hw/pci.h b/hw/pci.h
> index 391217e..401d14a 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 DECLARE_LDST_DMA(_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);            \
> +
> +DECLARE_LDST_DMA(ub, b, 8);
> +DECLARE_LDST_DMA(uw, w, 16);
> +DECLARE_LDST_DMA(l, l, 32);
> +DECLARE_LDST_DMA(q, q, 64);
> +
> +#undef DECLARE_LDST_DMA
> +
>   #endif