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

[Eduard - Gabriel Munteanu]

On Thu, Jun 02, 2011 at 09:43:32AM -0700, Richard Henderson wrote:
> On 06/02/2011 08:12 AM, David Gibson wrote:
> > +    err = iommu->translate(dev, addr, &paddr, &plen, is_write);
> > +    if (err) {
> > +        return NULL;
> > +    }
> > +
> > +    /*
> > +     * If this is true, the virtual region is contiguous,
> > +     * but the translated physical region isn't. We just
> > +     * clamp *len, much like cpu_physical_memory_map() does.
> > +     */
> > +    if (plen < *len) {
> > +        *len = plen;
> > +    }
> > +
> > +    buf = cpu_physical_memory_map(paddr, &plen, is_write);
> > +    *len = plen;
> > +
> > +    /* We treat maps as remote TLBs to cope with stuff like AIO. */
> 
> Oh, that reminds me.  There's a bug here in Eduard's original:
> 
> PLEN is set to the maximum length of the transfer by the 
> translate function.  What we do *not* want is to pass a
> very very large region to cpu_physical_memory_map.
> 
> The effects of this are hard to see with the AMD IOMMU, since
> it is entirely page based and thus PLEN will be increased by
> no more than 4k, but Alpha IOMMUs have direct-mapped translation
> windows that can be up to 128GB.
> 
> I'm unsure whether I prefer to force the translator function
> to never increase PLEN (which is what I implemented in my 
> own branch) or whether all callers of the translate function
> must be aware that the returned PLEN can increase.

My latest patches seem to have fixed that:

+    if (plen < *len) {
+        *len = plen;
+    }
+
+    buf = cpu_physical_memory_map(paddr, len, is_write);

I think the callers of translate() should take care of clamping the
length. Note the 'len' passed into translate() is write-only, so there's
nothing to increase in relation to, but I get your point.

The reason is we have two different behaviors to cater for: maps need to
be contiguous, while plain reads/writes try to resolve the whole range
of DMA addresses. In order to do that, translate() tells the caller
where the translation ceases to be valid so it can make informed
choices.

And anyway, translate() isn't supposed to be called from other places,
just the DMA abstraction.

> 
> r~