Re: [PATCH v4 4/8] PCI: brcmstb: Add dma-range mapping for inbound traffic

From: Christoph Hellwig <hch@lst.de>
To: Florian Fainelli <florian.fainelli@broadcom.com>
Cc: Christoph Hellwig <hch@lst.de>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Jim Quinlan <jim2101024@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	Brian Norris <computersforpeace@gmail.com>,
	Russell King <rmk+kernel@armlinux.org.uk>,
	Robin Murphy <robin.murphy@arm.com>,
	Jonas Gorski <jonas.gorski@gmail.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
	<devicetree@vger.kernel.org>,
	Linux-MIPS <linux-mips@linux-mips.org>,
	linux-pci@vger.kernel.org, Kevin Cernekee <cernekee@gmail.co>
Subject: Re: [PATCH v4 4/8] PCI: brcmstb: Add dma-range mapping for inbound traffic
Date: Tue, 23 Jan 2018 14:20:33 +0100	[thread overview]
Message-ID: <20180123132033.GA21438@lst.de> (raw)
In-Reply-To: <d62226a2-a92c-cdcb-4a9b-e69ab677bc60@broadcom.com>

On Fri, Jan 19, 2018 at 11:47:54AM -0800, Florian Fainelli wrote:
> How can this work well in the context of a loadable module for instance?
> For MIPS, this would mean that we have to override phys_to_dma() and
> dma_to_phys() in the platform that is *susceptible* to use this PCIe
> controller (arch/mips/bmips) which is fine, but there, we essentially
> need to find a way to make this dynamic based on whether the PCIe
> controller is loaded or not.
> 
> As you might have seen from this patch, what needs to be done is highly
> dependent on the processor architecture and its memory controller
> physical memory map, so I don't see how we are in any better situation
> if we need to replicate 3 times across MIPS, ARM and ARM64 how the
> addresses need to be mangled.
> 
> Are you suggesting we somehow decouple the memory mangling part into a
> portion that can be built into the kernel image (so phys_to_dma() and
> dma_to_phys() is resolved at vmlinux link time) and can be selected by
> different architectures that need it? If so, yikes.

On architectures with crazy PCIe controllers (this seems to include
mips, arm, arm64 and x86 thanks to the weird SOCs) we will need a
a few different memory maps, yes.  Take a look at
arch/x86/pci/sta2x11-fixup.c, preferably from a tree where the worst
issues are fixed:

http://git.infradead.org/users/hch/misc.git/blob/refs/heads/dma-direct-all:/arch/x86/pci/sta2x11-fixup.c

Overriding phys_to_dma and dma_to_phys is required if you need to
support swiotlb, and chances are with a broken PCIe controller on
arm64 or mips64 you eventuall will.

This sta2x11 code should probably be lifted to common code in
one form or another eventually, althought it will need another
fair round of cleanups for now.

> I can see value in having a generic mechanism, ala X86_DMA_REMAP
> allowing architectures to have the ability to override phys_to_dma() and
> dma_to_phys() but right now, especially if we look at
> arch/x86/pci/sta2x11-fixup.c this really appears to be quite messy and
> equally ugly than stacking operations...
> 
> What is the actual problem you want to avoid with the stacking of DMA
> operations, is it because it becomes harder to audit, or are there are
> other reasons?

Audit, consolidate into a single dma-direct implementation and properly
support swiotlb out of the box.