Hi Robin,

-----Original Message-----
From: Robin Murphy [mailto:robin.murphy@arm.com]
Sent: Friday, March 29, 2019 4:51 PM

On 29/03/2019 14:00, laurentiu.tudor@nxp.com wrote:

quoted

From: Laurentiu Tudor <laurentiu.tudor@nxp.com>

Add a one-to-one iommu mapping for bman private data memory (FBPR).
This is required for BMAN to work without faults behind an iommu.

Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
---
  drivers/soc/fsl/qbman/bman_ccsr.c | 11 +++++++++++
  1 file changed, 11 insertions(+)

diff --git a/drivers/soc/fsl/qbman/bman_ccsr.c

b/drivers/soc/fsl/qbman/bman_ccsr.c

quoted

index 7c3cc968053c..b209c79511bb 100644

--- a/drivers/soc/fsl/qbman/bman_ccsr.c
+++ b/drivers/soc/fsl/qbman/bman_ccsr.c

@@ -29,6 +29,7 @@
   */

  #include "bman_priv.h"
+#include <linux/iommu.h>

  u16 bman_ip_rev;
  EXPORT_SYMBOL(bman_ip_rev);

@@ -178,6 +179,7 @@ static int fsl_bman_probe(struct platform_device

*pdev)

quoted

  	int ret, err_irq;
  	struct device *dev = &pdev->dev;
  	struct device_node *node = dev->of_node;
+	struct iommu_domain *domain;
  	struct resource *res;
  	u16 id, bm_pool_cnt;
  	u8 major, minor;

@@ -225,6 +227,15 @@ static int fsl_bman_probe(struct platform_device

*pdev)

quoted

  	dev_dbg(dev, "Allocated FBPR 0x%llx 0x%zx\n", fbpr_a, fbpr_sz);

+	/* Create an 1-to-1 iommu mapping for FBPR area */
+	domain = iommu_get_domain_for_dev(dev);

If that's expected to be the default domain that you're grabbing, then
this is *incredibly* fragile. There's nothing to stop the IOVA that you
forcibly map from being automatically allocated later and causing some
other DMA mapping to fail noisily and unexpectedly. 

Agree here, we pretty much rely on luck with this implementation. As a side note, I've also experimented using dma_map_resource() instead of directly calling into iommu api and things worked fine, but see below ...

Furthermore, have you tried this with "iommu.passthrough=1"?

Yes. The iommu_map() calls fail and the drivers issue warning messages, but apart from that I don't see any issues.

That said, I really don't understand what's going on here anyway :/

As far as I can tell from qbman_init_private_mem(), fbpr_a comes from
dma_alloc_coherent() and thus would already be a mapped IOVA - isn't
this the stuff that Roy converted to nicely use shared-dma-pool regions
a while ago?

I must say that I'm also unclear on this. The thing is that I don't get to see a smmu mapping being created for the reserved memory as result of calling dma_alloc_coherent(). IIRC, at the time when I looked at this I concluded that the call to dma_alloc_coherent() simply returns the phys address of the device's reserved memory without creating a smmu mapping to back it up. Maybe my understanding was not correct or perhaps there's an issue with this shared-dma-pool mechanism where instead of creating a mapping in the smmu and return an IOVA it just returns the physical address of the reserved memory area.

---
Thanks & Best Regards, Laurentiu

quoted

+	if (domain) {
+		ret = iommu_map(domain, fbpr_a, fbpr_a, fbpr_sz,
+				IOMMU_READ | IOMMU_WRITE | IOMMU_CACHE);
+		if (ret)
+			dev_warn(dev, "failed to iommu_map() %d\n", ret);
+	}
+
  	bm_set_memory(fbpr_a, fbpr_sz);

  	err_irq = platform_get_irq(pdev, 0);