Re: [Qemu-devel] [RFC PATCH 01/13] Generic DMA memory access interface

[Richard Henderson <rth@twiddle.net>]

On 05/31/2011 06:38 PM, Eduard - Gabriel Munteanu wrote:
> +static inline void dma_memory_rw(DMADevice *dev,
> +                                 dma_addr_t addr,
> +                                 void *buf,
> +                                 dma_addr_t len,
> +                                 int is_write)

I don't think this needs to be inline...

> +{
> +    /*
> +     * Fast-path non-iommu.
> +     * More importantly, makes it obvious what this function does.
> +     */
> +    if (!dev || !dev->mmu) {
> +        cpu_physical_memory_rw(addr, buf, len, is_write);
> +        return;
> +    }

... because you'll never be able to eliminate the if or the calls.
You might as well make the overall code smaller by taking the
entire function out of line.

> +#define DEFINE_DMA_LD(prefix, suffix, devtype, dmafield, size)            \
> +static inline uint##size##_t                                              \
> +dma_ld##suffix(DMADevice *dev, dma_addr_t addr)                           \
> +{                                                                         \
> +    int err;                                                              \
> +    dma_addr_t paddr, plen;                                               \
> +                                                                          \
> +    if (!dev || !dev->mmu) {                                              \
> +        return ld##suffix##_phys(addr);                                   \
> +    }                                                                     \

Similarly for all the ld/st functions.

> +#define DEFINE_DMA_MEMORY_RW(prefix, devtype, dmafield)
> +#define DEFINE_DMA_MEMORY_READ(prefix, devtype, dmafield)
> +#define DEFINE_DMA_MEMORY_WRITE(prefix, devtype, dmafield)
> +
> +#define DEFINE_DMA_OPS(prefix, devtype, dmafield)          \

I think this is a bit over the top, really.

> +        err = dev->mmu->translate(dev, addr, &paddr, &plen, is_write);

I see you didn't take my suggestion for using an opaque callback pointer.
Really and truly, I won't be able to use this as-is for Alpha.


r~