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 10:42:18AM +0200, Arnd Bergmann wrote:
> On Friday 02 May 2014 06:11:42 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.
> 
> I think it's ambiguous at best at the moment, there is no clear
> answer what it should be until we define the semantics more clearly.
> 
> At the moment, we have these callers
> 
> arch/arm/mach-imx/mach-imx27_visstrim_m10.c:    dma = dma_declare_coherent_memory(&pdev->dev,
> arch/arm/mach-imx/mach-imx27_visstrim_m10.c:    dma = dma_declare_coherent_memory(&pdev->dev,
> arch/arm/mach-imx/mach-imx27_visstrim_m10.c:    dma = dma_declare_coherent_memory(&pdev->dev,
> arch/arm/mach-imx/mach-imx27_visstrim_m10.c:    dma = dma_declare_coherent_memory(&pdev->dev,
> arch/arm/mach-imx/mach-mx31_3ds.c:      dma = dma_declare_coherent_memory(&pdev->dev,
> arch/arm/mach-imx/mach-mx31moboard.c:   dma = dma_declare_coherent_memory(&pdev->dev,
> arch/arm/mach-imx/mach-mx35_3ds.c:      dma = dma_declare_coherent_memory(&pdev->dev,
> arch/arm/mach-imx/mach-pcm037.c:        dma = dma_declare_coherent_memory(&pdev->dev,
> arch/arm/plat-samsung/s5p-dev-mfc.c:            if (dma_declare_coherent_memory(area->dev, area->base,
> arch/sh/drivers/pci/fixups-dreamcast.c:         BUG_ON(!dma_declare_coherent_memory(&dev->dev,
> drivers/media/platform/s5p-mfc/s5p_mfc.c:       if (dma_declare_coherent_memory(dev->mem_dev_l, mem_info[
> drivers/media/platform/s5p-mfc/s5p_mfc.c:       if (dma_declare_coherent_memory(dev->mem_dev_r, mem_info[
> drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c:               err = dma_declare_coherent_memory
> drivers/scsi/NCR_Q720.c:        if (dma_declare_coherent_memory(dev, base_addr, base_addr,
> drivers/usb/host/ohci-sm501.c:  if (!dma_declare_coherent_memory(dev, mem->start,
> drivers/usb/host/ohci-tmio.c:   if (!dma_declare_coherent_memory(&dev->dev, sram->start,
> 
> I don't know about NCR_Q720, but all others are only used on machines
> where physical addresses and bus addresses are in the same space.

In general, the driver doesn't know whether physical and bus addresses
are in the same space.  At least, I *hope* it doesn't have to know,
because it can't be very generic if it does.

> There are other machines that may always have to go through an IOMMU,
> or that have a fixed offset between dma addresses and physical addresses
> and that need to call a phys_to_dma() to convert between the two, but
> none of those call dma_declare_coherent_memory.
> ...

> We also have machines on ARM can never do DMA to addresses above the
> 32-bit boundary but can have more than 4GB RAM (using swiotlb or iommu).
> Building a kernel for these machines uses a 32-bit dma_addr_t  and
> a 64-bit phys_addr_t.

The PCI core currently prevents those machines from having MMIO space
above 4GB on the bus, which doesn't sound strictly necessary.

> > Well, first we need to answer whether we need separate phys_addr_t and
> > dma_addr_t ... we know the former represents physical memory addressed
> > by the CPU and the latter bus physical addresses from devices ... but
> > they're essentially the same.
> 
> I think BARs should be phys_addr_t: in the example with 32-bit dma_addr_t
> and 64-bit phys_addr_t, you would be able to put the MMIO registers
> into a high address if the PCI host supports that, the only limitation
> would be that no device could DMA into a high address for doing inter-device
> DMA.

I could imagine using u64 for BARs, but I don't see why we would use
phys_addr_t.  I think it's simpler to describe dma_addr_t as "sufficient
to hold any address on the bus, whether for DMA or MMIO accesses."

There isn't really a connection between the CPU address width and the
bus address width.  A 64-bit CPU could have a 32-bit conventional PCI
bus, and a 32-bit CPU could use a bus with all MMIO space above 4GB
given a host bridge that did appropriate translation.  (I don't know
why one would do the latter, but I don't know why it wouldn't work.)

Bjorn