Re: [Qemu-devel] [PATCH RFC] dma_rw.h (was Re: [PATCH 0/7] AMD IOMMU emulation patchset v4)

[Anthony Liguori <anthony@codemonkey.ws>]

On 09/13/2010 03:01 PM, Michael S. Tsirkin wrote:
> So I think the following will give the idea of what an API
> might look like that will let us avoid the scary hacks in
> e.g. the ide layer and other generic layers that need to do DMA,
> without either binding us to pci, adding more complexity with
> callbacks, or losing type safety with casts and void*.
>
> Basically we have DMADevice that we can use container_of on
> to get a PCIDevice from, and DMAMmu that will get instanciated
> in a specific MMU.
>
> This is not complete code - just a header - I might complete
> this later if/when there's interest or hopefully someone interested
> in iommu emulation will.
>
> Notes:
> the IOMMU_PERM_RW code seem unused, so I replaced
> this with plain is_write. Is it ever useful?
>
> It seems that invalidate callback should be able to
> get away with just a device, so I switched to that
> from a void pointer for type safety.
> Seems enough for the users I saw.
>
> I saw devices do stl_le_phys and such, these
> might need to be wrapped as well.
>
> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>    

One of the troubles with an interface like this is that I'm not sure a 
generic model universally works.

For instance, I know some PCI busses do transparent byte swapping.  For 
this to work, there has to be a notion of generic memory reads/writes 
vs. reads of a 32-bit, 16-bit, and 8-bit value.

With a generic API, we lose the flexibility to do this type of bus 
interface.

Regards,

Anthony Liguori

> ---
>
> diff --git a/hw/dma_rw.h b/hw/dma_rw.h
> new file mode 100644
> index 0000000..d63fd17
> --- /dev/null
> +++ b/hw/dma_rw.h
> @@ -0,0 +1,122 @@
> +#ifndef DMA_RW_H
> +#define DMA_RW_H
> +
> +#include "qemu-common.h"
> +
> +/* We currently only have pci mmus, but using
> +   a generic type makes it possible to use this
> +   e.g. from the generic ide code without callbacks. */
> +typedef uint64_t dma_addr_t;
> +
> +typedef struct DMAMmu DMAMmu;
> +typedef struct DMADevice DMADevice;
> +
> +typedef int DMATranslateFunc(DMAMmu *mmu,
> +                             DMADevice *dev,
> +                             dma_addr_t addr,
> +                             dma_addr_t *paddr,
> +                             dma_addr_t *len,
> +                             int is_write);
> +
> +typedef int DMAInvalidateMapFunc(DMADevice *);
> +struct DMAMmu {
> +	/* invalidate, etc. */
> +	DmaTranslateFunc *translate;
> +};
> +
> +struct DMADevice {
> +	DMAMmu *mmu;
> +	DMAInvalidateMapFunc *invalidate;
> +};
> +
> +void dma_device_init(DMADevice *, DMAMmu *, DMAInvalidateMapFunc *);
> +
> +static inline void dma_memory_rw(DMADevice *dev,
> +				 dma_addr_t addr,
> +				 void *buf,
> +				 uint32_t len,
> +				 int is_write)
> +{
> +    uint32_t plen;
> +    /* Fast-path non-iommu.
> +     * More importantly, makes it obvious what this function does. */
> +    if (!dev->mmu) {
> +    	cpu_physical_memory_rw(paddr, buf, plen, is_write);
> +    	return;
> +    }
> +    while (len) {
> +        err = dev->mmu->translate(iommu, dev, addr,&paddr,&plen, is_write);
> +        if (err) {
> +            return;
> +        }
> +
> +        /* The translation might be valid for larger regions. */
> +        if (plen>  len) {
> +            plen = len;
> +        }
> +
> +        cpu_physical_memory_rw(paddr, buf, plen, is_write);
> +
> +        len -= plen;
> +        addr += plen;
> +        buf += plen;
> +    }
> +}
> +
> +void *dma_memory_map(DMADevice *dev,
> +                            dma_addr_t addr,
> +                            uint32_t *len,
> +                            int is_write);
> +void dma_memory_unmap(DMADevice *dev,
> +		      void *buffer,
> +		      uint32_t len,
> +		      int is_write,
> +		      uint32_t access_len);
> +
> +
> ++#define DEFINE_DMA_LD(suffix, size)                                       \
> ++uint##size##_t dma_ld##suffix(DMADevice *dev, dma_addr_t addr)            \
> ++{                                                                         \
> ++    int err;                                                              \
> ++    target_phys_addr_t paddr, plen;                                       \
> ++    if (!dev->mmu) {                                                      \
> ++        return ld##suffix##_phys(addr, val);                              \
> ++    }                                                                     \
> ++                                                                          \
> ++    err = dev->mmu->translate(dev->bus->iommu, dev,                       \
> ++                              addr,&paddr,&plen, IOMMU_PERM_READ);      \
> ++    if (err || (plen<  size / 8))                                         \
> ++        return 0;                                                         \
> ++                                                                          \
> ++    return ld##suffix##_phys(paddr);                                      \
> ++}
> ++
> ++#define DEFINE_DMA_ST(suffix, size)                                       \
> ++void dma_st##suffix(DMADevice *dev, dma_addr_t addr, uint##size##_t val)  \
> ++{                                                                         \
> ++    int err;                                                              \
> ++    target_phys_addr_t paddr, plen;                                       \
> ++                                                                          \
> ++    if (!dev->mmu) {                                                      \
> ++        st##suffix##_phys(addr, val);                                     \
> ++        return;                                                           \
> ++    }                                                                     \
> ++    err = dev->mmu->translate(dev->bus->iommu, dev,                       \
> ++                              addr,&paddr,&plen, IOMMU_PERM_WRITE);     \
> ++    if (err || (plen<  size / 8))                                         \
> ++        return;                                                           \
> ++                                                                          \
> ++    st##suffix##_phys(paddr, val);                                        \
> ++}
> +
> +DEFINE_DMA_LD(ub, 8)
> +DEFINE_DMA_LD(uw, 16)
> +DEFINE_DMA_LD(l, 32)
> +DEFINE_DMA_LD(q, 64)
> +
> +DEFINE_DMA_ST(b, 8)
> +DEFINE_DMA_ST(w, 16)
> +DEFINE_DMA_ST(l, 32)
> +DEFINE_DMA_ST(q, 64)
> +
> +#endif
> diff --git a/hw/pci.h b/hw/pci.h
> index 1c6075e..9737f0e 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -5,6 +5,7 @@
>   #include "qobject.h"
>
>   #include "qdev.h"
> +#include "dma_rw.h"
>
>   /* PCI includes legacy ISA access.  */
>   #include "isa.h"
> @@ -119,6 +120,10 @@ enum {
>
>   struct PCIDevice {
>       DeviceState qdev;
> +
> +    /* For devices that do DMA. */
> +    DMADevice dma;
> +
>       /* PCI config space */
>       uint8_t *config;
>
>
>