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

[Eduard - Gabriel Munteanu]

On Wed, Jun 01, 2011 at 07:01:42AM -0700, Richard Henderson wrote:
> 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.
> 

The idea was to get to the fastpath as soon as possible. I'm not really
concerned about the case where there's an IOMMU present, since
translation/checking does a lot more work. But other people might be
worried about that additional function call when there's no IOMMU.

And these functions are quite small anyway.

Thoughts, anybody else?

> > +#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.
> 

Yeah, it's a bit unconventional, but why do you think that?

The main selling point is there are more chances to screw up if every
bus layer implements these manually. And it's really convenient,
especially if we get to add another ld/st.

I do have one concern about it, though: it might increase compile time
due to additional preprocessing work. I haven't done any benchmarks on
that. But apart from this, are there any other objections?

> > +        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.
> 

If I understand correctly you need some sort of shared state between
IOMMUs or units residing on different buses. Then you should be able to
get to it even with this API, just like I do with my AMD IOMMU state by
upcasting. It doesn't seem to matter whether you've got an opaque, that
opaque could very well be reachable by upcasting.

Did I get this wrong?


	Eduard

> 
> r~