Re: [PATCH] DMA-API: Change dma_declare_coherent_memory() CPU address to phys_addr_t

[Bjorn Helgaas <bhelgaas@google.com>]

On Fri, May 02, 2014 at 12:18:07PM +0100, Liviu Dudau wrote:
> On Thu, May 01, 2014 at 08:05:04PM +0100, Bjorn Helgaas wrote:
> > On Thu, May 1, 2014 at 8:08 AM, James Bottomley
> > <jbottomley@parallels.com> wrote:
> > > On Wed, 2014-04-30 at 14:33 -0600, Bjorn Helgaas wrote:
> > >> dma_declare_coherent_memory() takes two addresses for a region of memory: a
> > >> "bus_addr" and a "device_addr".  I think the intent is that "bus_addr" is
> > >> the physical address a *CPU* would use to access the region, and
> > >> "device_addr" is the bus address the *device* would use to address the
> > >> region.
> > >>
> > >> Rename "bus_addr" to "phys_addr" and change its type to phys_addr_t.
> > >
> > > Remind me what the difference between phys_addr_t and dma_addr_t are.
> > >
> > > I thought phys_addr_t was the maximum address the CPU could reach after
> > > address translation and dma_addr_t was the maximum physical address any
> > > bus attached memory mapped devices could appear at. (of course, mostly
> > > they're the same).
> > 
> > I assumed phys_addr_t was for physical addresses generated by a CPU
> > and dma_addr_t was for addresses generated by a device, e.g.,
> > addresses you would see with a PCI bus analyzer or in a PCI BAR.  But
> > ARCH_DMA_ADDR_T_64BIT does seem more connected to processor-specific
> > things, e.g., ARM_LPAE, than to device bus properties like "support
> > 64-bit PCI, not just 32-bit PCI."
> > 
> > DMA-API-HOWTO.txt contains this:
> > 
> >   ... the definition of the dma_addr_t (which can hold any valid DMA
> >   address for the platform) type which should be used everywhere you
> >   hold a DMA (bus) address returned from the DMA mapping functions.
> > 
> > and clearly you use a dma_addr_t from dma_alloc_coherent(), etc., to
> > program the device.  It seems silly to have phys_addr_t and dma_addr_t
> > for two (possibly slightly different) flavors of CPU physical
> > addresses, and then sort of overload dma_addr_t by also using it to
> > hold the addresses devices use for DMA.
> 
> dma_addr_t is a phys_addr_t masked according to the capabilities of the
> *DMA engine* (not device). 

I don't quite agree with this.  phys_addr_t is used for physical
addresses generated by a CPU, e.g., ioremap() takes a phys_addr_t and
returns a void * (a virtual address).  A dma_addr_t is an address you
get from dma_map_page() or a similar interface, and you program into a
device (or DMA engine if you prefer).  You can't generate a dma_addr_t
simply by masking a phys_addr_t because an IOMMU can make arbitrary
mappings of bus addresses to physical memory addresses.

It is meaningless for a CPU to generate a reference to a dma_addr_t.
It will work in some simple cases where there's no IOMMU, but it won't
work in general.

> Quite a lot of PCI devices (all?) now contain
> a DMA engine, so the distinction is somewhat blurred nowadays.

I'll take your word for the DMA engine/device distinction, but I don't
see how it's relevant here.  Linux doesn't have a generic idea of a
DMA engine; we just treat it as part of the device capabilities.

> > I'm not a CPU
> > person, so I don't understand the usefulness of dma_addr_t as a
> > separate type to hold CPU addresses at which memory-mapped devices can
> > appear.  If that's all it is, why not just use phys_addr_t everywhere
> > and get rid of dma_addr_t altogether?
> 
> Because dma_addr_t imposes further restrictions based on what the DMA engine
> can access. You could have a DMA engine that can only access 32bit addresses
> in a 40+ bit CPU addresses system.

Yes; this is an argument for having separate types for bus addresses
and CPU addresses.  I think dma_addr_t *is* for bus addresses.  I
thought James was suggesting that it was a subset of CPU physical
address space, and that any valid dma_addr_t is also a valid
phys_addr_t.  That doesn't seem right to me.

> > dma_declare_coherent_memory() calls ioremap() on bus_addr, which seems
> > a clear indication that bus_addr is really a physical address usable
> > by the CPU.  But I do see your point that bus_addr is a CPU address
> > for a memory-mapped device, and would thus fit into your idea of a
> > dma_addr_t.
> > 
> > I think this is the only part of the DMA API that deals with CPU
> > physical addresses.  I suppose we could side-step this question by
> > having the caller do the ioremap; then dma_declare_coherent_memory()
> > could just take a CPU virtual address (a void *) and a device address
> > (a dma_addr_t).
> 
> And how would that change the meaning of the function?

I think the interesting thing here is that we ioremap() a bus address.
That doesn't work in general because ioremap() is expecting a CPU
physical address, not a bus address.  We don't have a generic way for
dma_declare_coherent_memory() to convert a bus address to a CPU
physical address.  The caller knows a little more and probably *could*
do that, e.g., a PCI driver could use pcibios_bus_to_resource().  If
we made a generic way to do the conversion, it would definitely be
nicer to leave the ioremap() inside dma_declare_coherent_memory().

Bjorn