From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from service87.mimecast.com ([91.220.42.44]:59645 "EHLO service87.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751887AbaEBLSM convert rfc822-to-8bit (ORCPT ); Fri, 2 May 2014 07:18:12 -0400 Date: Fri, 2 May 2014 12:18:07 +0100 From: Liviu Dudau To: Bjorn Helgaas Cc: James Bottomley , "rdunlap@infradead.org" , "linux-pci@vger.kernel.org" , "gregkh@linuxfoundation.org" , "linux-doc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Arnd Bergmann Subject: Re: [PATCH] DMA-API: Change dma_declare_coherent_memory() CPU address to phys_addr_t Message-ID: <20140502111807.GL4750@e106497-lin.cambridge.arm.com> References: <20140430203321.30056.14833.stgit@bhelgaas-glaptop.roam.corp.google.com> <1398953290.2313.15.camel@dabdike.int.hansenpartnership.com> MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=UTF-8 Sender: linux-pci-owner@vger.kernel.org List-ID: On Thu, May 01, 2014 at 08:05:04PM +0100, Bjorn Helgaas wrote: > [+cc Arnd] > > On Thu, May 1, 2014 at 8:08 AM, James Bottomley > 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). Quite a lot of PCI devices (all?) now contain a DMA engine, so the distinction is somewhat blurred nowadays. > > I think the "CPU generates phys_addr_t" and "device generates > dma_addr_t" distinction seems like a useful idea. s/device/DMA engine inside device/ > 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. > > > The intent of dma_declare_coherent_memory() is to take a range of memory > > provided by some device on the bus and allow the CPU to allocate regions > > from it for use as things like descriptors. The CPU treats it as real > > memory, but, in fact, it is a mapped region of an attached device. It is not the CPU that is the user of the bus_addr but the DMA engine. And that can be on the bus as well, so it needs to generate bus addresses. > > > > If my definition is correct, then bus_addr should be dma_addr_t because > > it has to be mapped from a device and dma_addr_t is the correct type for > > device addresses. If I've got the definition wrong, then we should > > document it somewhere, because it's probably confusing other people as > > well. > > 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? Best regards, Liviu > > But I'd still have the question of whether we're using dma_addr_t > correctly in the PCI core. We use it to hold BAR values and the like, > and I've been making assumptions like "if dma_addr_t is only 32 bits, > we can't handle BARs above 4GB." > > Bjorn > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯