| Message ID | 1417066891-16789-1-git-send-email-ming.lei@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Ming, Thanks for reviving this patch. We carry a similar one internally for Storm currently to handle the Mellanox part that you are trying to use with Trusty as well. There's also an ACPI variant based upon the _CCA provided for the root complex but the eventual upstream solution needs to be a bit more nuanced on AArch64 since PCI devices can imply properties of coherency in e.g. snoopable capability. So while this is an ok default/fallback/initial patch, PCI on AArch64 will need a bit more than this. We're looking at this at the moment (in addition to having the ACPI specification clarified to indicate that there is no safe default assumption, requiring that DMA masters always indicate their coherency or otherwise via a _CCA whether true or false). Jon.
On Thursday 27 November 2014 13:41:31 Ming Lei wrote: > @@ -37,6 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, > return res->start; > } > > +/* Inherit root controller's dma coherent ops */ > +static void pci_dma_config(struct pci_dev *dev) > +{ > + struct pci_bus *bus = dev->bus; > + struct device *host; > + > + while (!pci_is_root_bus(bus)) { > + bus = bus->parent; > + } > + > + host = bus->dev.parent->parent; > + if (of_dma_is_coherent(host->of_node)) > + set_arch_dma_coherent_ops(&dev->dev); > +} > + I think we need something more generic than this: This is not architecture specific at all, and we have to deal with IOMMU, swiotlb, dma offset and dma mask as well, coherency is definitely not the only issue. We have the of_dma_configure() function that does some of this for platform devices and that have to extend to make it work with IOMMUs, and the common pci_device_add() function does some other subset in architecture independent code for PCI, so I think that's where it should go. We will need an architecture-specific helper to set the dma_map_ops pointer, dma_parms, dma offset, and the iommu, but that is not PCI specific, it just gets called from both the platform device and pci device code. Regarding the pcibios_add_device() function, I'd rather see the existing code moved into the PCI core and the function removed, since it also is not architecture specific. We should be able to delete the entire arm64/pci.c file eventually. Arnd
On Thursday 27 November 2014 02:39:59 Jon Masters wrote: > We're looking at this at the moment (in addition to having the ACPI specification > clarified to indicate that there is no safe default assumption, requiring that > DMA masters always indicate their coherency or otherwise via a _CCA whether true > or false). I think for arm64, this should be easy: since ACPI is only for servers, we can rely on the fact that the devices have cache-coherent DMA all the time. Let's not over-complicate the ACPI case with all the special hacks we need for embedded. Arnd
On Thu, Nov 27, 2014 at 5:03 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > Regarding the pcibios_add_device() function, I'd rather see the existing > code moved into the PCI core and the function removed, since it also > is not architecture specific. We should be able to delete the entire > arm64/pci.c file eventually. I totally agree. Looks pcibios_*() are misused by ARCHs, which should have been designed for addressing ARCH pcibios things, now these functions are just a hook for ARCH code. Specifically pci host controller driver can't be arch independent at all because of ARCH's pcibios_*() implementation and the specific data type defined by ARCH code. Thanks, Ming Lei
On Thu, Nov 27, 2014 at 09:03:38AM +0000, Arnd Bergmann wrote: > On Thursday 27 November 2014 13:41:31 Ming Lei wrote: > > > @@ -37,6 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, > > return res->start; > > } > > > > +/* Inherit root controller's dma coherent ops */ > > +static void pci_dma_config(struct pci_dev *dev) > > +{ > > + struct pci_bus *bus = dev->bus; > > + struct device *host; > > + > > + while (!pci_is_root_bus(bus)) { > > + bus = bus->parent; > > + } > > + > > + host = bus->dev.parent->parent; > > + if (of_dma_is_coherent(host->of_node)) > > + set_arch_dma_coherent_ops(&dev->dev); > > +} > > + > > I think we need something more generic than this: This is not architecture > specific at all, and we have to deal with IOMMU, swiotlb, dma offset and > dma mask as well, coherency is definitely not the only issue. Yeah, and we need to extend the IOMMU binding to express RequesterID -> StreamID (master ID) mappings at the host controller, too. Will
On Thu, Nov 27, 2014 at 09:05:57AM +0000, Arnd Bergmann wrote: > On Thursday 27 November 2014 02:39:59 Jon Masters wrote: > > We're looking at this at the moment (in addition to having the ACPI specification > > clarified to indicate that there is no safe default assumption, requiring that > > DMA masters always indicate their coherency or otherwise via a _CCA whether true > > or false). > > I think for arm64, this should be easy: since ACPI is only for servers, we can > rely on the fact that the devices have cache-coherent DMA all the time. Let's > not over-complicate the ACPI case with all the special hacks we need for embedded. As usual, we need hw vendors to say what they expect/build here. I think in the past (non-ARM) the assumption was that the DMA is coherent on ACPI-capable systems unless otherwise stated but I've seen discussions (as Jon mentioned above) that ACPI should always indicate the coherency on ARM without any assumption. Either way, I don't think it's a problem for the kernel. We just need to change the default DMA ops to coherent when booting with ACPI (using non-coherent ops for a coherent device is not safe as the CPU can corrupt cache lines written by the device).
On Thursday 27 November 2014 11:36:36 Catalin Marinas wrote: > On Thu, Nov 27, 2014 at 09:05:57AM +0000, Arnd Bergmann wrote: > > On Thursday 27 November 2014 02:39:59 Jon Masters wrote: > > > We're looking at this at the moment (in addition to having the ACPI specification > > > clarified to indicate that there is no safe default assumption, requiring that > > > DMA masters always indicate their coherency or otherwise via a _CCA whether true > > > or false). > > > > I think for arm64, this should be easy: since ACPI is only for servers, we can > > rely on the fact that the devices have cache-coherent DMA all the time. Let's > > not over-complicate the ACPI case with all the special hacks we need for embedded. > > As usual, we need hw vendors to say what they expect/build here. > > I think in the past (non-ARM) the assumption was that the DMA is > coherent on ACPI-capable systems unless otherwise stated but I've seen > discussions (as Jon mentioned above) that ACPI should always indicate > the coherency on ARM without any assumption. Yes, that is fine, but I think Linux should just BUG_ON() any device on ACPI that doesn't have this flag set. You definitely want to have the flags work both ways for Windows Phone, but we don't need to support that hardware with Linux. The SBSA is quite specific about requiring coherent DMA to work, so this is a safe assumption to make. We can support all other systems with DT without problems. > Either way, I don't think it's a problem for the kernel. We just need to > change the default DMA ops to coherent when booting with ACPI (using > non-coherent ops for a coherent device is not safe as the CPU can > corrupt cache lines written by the device). We should probably default to the coherent IOMMU dma ops with ACPI eventually, I'd assume that these machines also come with an IOMMU, though if we ever want to support virtual machines with ACPI as well, that probably wants the linear operations, and until the SMMU support for PCI is done, we have to start out using the linear operations as well. Arnd
On Thu, Nov 27, 2014 at 5:03 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thursday 27 November 2014 13:41:31 Ming Lei wrote: > >> @@ -37,6 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, >> return res->start; >> } >> >> +/* Inherit root controller's dma coherent ops */ >> +static void pci_dma_config(struct pci_dev *dev) >> +{ >> + struct pci_bus *bus = dev->bus; >> + struct device *host; >> + >> + while (!pci_is_root_bus(bus)) { >> + bus = bus->parent; >> + } >> + >> + host = bus->dev.parent->parent; >> + if (of_dma_is_coherent(host->of_node)) >> + set_arch_dma_coherent_ops(&dev->dev); >> +} >> + > > I think we need something more generic than this: This is not architecture > specific at all, and we have to deal with IOMMU, swiotlb, dma offset and > dma mask as well, coherency is definitely not the only issue. That may take a bit long since ARCHs, dma, and pci subsystem are involved about the change. Given ARM64 PCI and related host controller driver are merged already, could this patch be applied to fix current problem first? Thanks, Ming Lei
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index ce5836c..4b6444a 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -16,6 +16,7 @@ #include <linux/mm.h> #include <linux/of_pci.h> #include <linux/of_platform.h> +#include <linux/of_address.h> #include <linux/slab.h> #include <asm/pci-bridge.h> @@ -37,6 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, return res->start; } +/* Inherit root controller's dma coherent ops */ +static void pci_dma_config(struct pci_dev *dev) +{ + struct pci_bus *bus = dev->bus; + struct device *host; + + while (!pci_is_root_bus(bus)) { + bus = bus->parent; + } + + host = bus->dev.parent->parent; + if (of_dma_is_coherent(host->of_node)) + set_arch_dma_coherent_ops(&dev->dev); +} + /* * Try to assign the IRQ number from DT when adding a new device */ @@ -44,6 +60,7 @@ int pcibios_add_device(struct pci_dev *dev) { dev->irq = of_irq_parse_and_map_pci(dev, 0, 0); + pci_dma_config(dev); return 0; }
This patch sets pci device's dma ops as coherent if the root controller is dma-coherent. This patch fixes the bug in below link: https://bugs.launchpad.net/bugs/1386490 Cc: Catalin Marinas <Catalin.Marinas@arm.com> Cc: Jon Masters <jcm@redhat.com> Cc: Dann Frazier <dann.frazier@canonical.com> Signed-off-by: Ming Lei <ming.lei@canonical.com> --- arch/arm64/kernel/pci.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)