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

[Bjorn Helgaas <bhelgaas@google.com>]

[+cc Magnus]

On Fri, May 02, 2014 at 06:11:42AM +0000, James Bottomley wrote:
> On Thu, 2014-05-01 at 13:05 -0600, 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."
> 
> OK, but even in your definition dma_declare_coherent_memory() should
> operate on dma_addr_t because the input is a region of memory you got
> from the device.  Somehow you generate and idea from the device
> configuration of where this piece of memory sits on the device and
> that's what you feed into dma_declare_coherent_memory().  It maps that
> region and makes it available to the CPU allocators.

The device configuration tells you where the region is on the bus.  To
find the corresponding CPU physical address, we have to know about any
address translation performed by the host bridge.  That's a bus-
specific concept, so maybe it belongs in the driver.

The only user of dma_declare_coherent_memory() on a PCI device is
gapspci_fixup_resources(), so we could leave
dma_declare_coherent_memory() alone and do something like this:

-   dma_declare_coherent_memory(&dev->dev, GAPSPCI_DMA_BASE, GAPSPCI_DMA_BASE,

+   struct pci_bus_region region;
+   struct resource res;

+   region.start = GAPSPCI_DMA_BASE;
+   region.end = GAPSPCI_DMA_BASE + GAPSPCI_DMA_SIZE - 1;
+   pcibios_bus_to_resource(dev->bus, &res, &region);
+   dma_declare_coherent_memory(&dev->dev, res->start, GAPSPCI_DMA_BASE,

But res->size is a resource_size_t, which is the same as phys_addr_t
and theoretically might not fit in dma_addr_t.  If we had some generic
way of doing pcibios_bus_to_resource(), we could pass just a single
address (the current "device_addr") and compute the CPU physical
address inside dma_declare_coherent_memory().

The other users implicitly assume the CPU physical address is the same
as the bus address.  I'm a little uneasy because I assume it's
possible to have a platform_device behind an address-translating
bridge, too.  But maybe nobody actually does that.

Bjorn